Skip to content

feat(tool-sandbox): caller-declared env pass-through via export_env - #1440

Merged
SequeI merged 1 commit into
nolabs-ai:mainfrom
kipz:kipz/tool-sandbox-inherit-vars
Aug 12, 2026
Merged

feat(tool-sandbox): caller-declared env pass-through via export_env#1440
SequeI merged 1 commit into
nolabs-ai:mainfrom
kipz:kipz/tool-sandbox-inherit-vars

Conversation

@kipz

@kipz kipz commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Linked Issue

Closes #1439

Summary

Caller-declared env pass-through: per-command export_env + top-level session_export_env. A command's named vars are copied verbatim to commands it invokes, bypassing allow_vars filtering and the dangerous-var blocklist. Patterns: exact, PREFIX_*, *. PATH/NONO_* always excluded.

Agent Disclosure (if applicable)

Authored by an AI agent (code, tests, PR, issue). No unwrap/expect/panic in new non-test code; covered by tests.

Test Plan

  • make ci passes (clippy -D warnings, fmt, tests, doc lint).
  • Unit tests: verbatim pass-through; PREFIX_* and * matching; PATH/NONO_* excluded under *; reserved/malformed patterns rejected.
  • Verified end-to-end on Linux: session_export_env and a mediated command's export_env each deliver a named var downstream that default filtering would drop.

Checklist

  • An issue exists and is linked above
  • All commits are signed-off, using DCO
  • All new code follows the project's coding standards (CLAUDE.md) and is covered by tests
  • Public-facing changes are paired with documentation updates
  • Release note has been added to CHANGELOG.md if needed

Agent Compliance Check (Required for AI/Automated PRs)

  • I am not prohibited from contributing under this policy
  • An issue already exists
  • I disclosed that I am an agent in the issue discussion
  • I described my intent and approach in the issue discussion
  • I reviewed repository coding and security rules for the affected area
  • I provided required attribution for reused or adapted code
  • I did not use forbidden patterns such as unwrap/expect
  • I used NonoError where required
  • I validated and canonicalized all relevant paths
  • This PR matches the approved or disclosed issue scope

@github-actions

github-actions Bot commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

PR Review Summary

Size

Metric Value
Lines added +609
Lines removed -31
Total changed 640
Classification Large (> 300 lines)

Affected crates

  • crates/nono-cli — CLI changes. Verify argument parsing, flag documentation, and UX behaviour across supported platforms.

Blast radius — Broad

This PR touches: source code,documentation,configuration / policy files


Updated automatically on each push to this PR.

@kipz
kipz force-pushed the kipz/tool-sandbox-inherit-vars branch from 7482491 to 4eacce8 Compare July 17, 2026 16:39
@kipz kipz changed the title feat(tool-sandbox): add inherit_vars environment escape hatch feat(tool-sandbox): caller-declared env pass-through via export_env Jul 17, 2026
@kipz
kipz force-pushed the kipz/tool-sandbox-inherit-vars branch from 2ce72de to cea7072 Compare July 17, 2026 21:07
@kipz
kipz force-pushed the kipz/tool-sandbox-inherit-vars branch from cea7072 to 529c0b1 Compare July 17, 2026 21:24
@kipz
kipz force-pushed the kipz/tool-sandbox-inherit-vars branch from 529c0b1 to 7687b6b Compare July 17, 2026 21:34
@kipz
kipz marked this pull request as ready for review July 17, 2026 21:37
@gemini-code-assist

Copy link
Copy Markdown
Contributor

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

@nogent-nolabs-ai nogent-nolabs-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nogent code review

No blocking issues; 0 findings.

Findings: none flagged in scope.

Automated code + security review. CI already covers clippy, rustfmt, tests, cargo-audit and commit-lint.

@lukehinds

Copy link
Copy Markdown
Contributor

hey @kipz , just a couple of fix ups

The new session_export_env and export_env fields also need to be added to the published profile schema. The embedded JSON Schema uses additionalProperties: false, so it could currently rejects any profile using these fields, even though the runtime accepts them. I think the corresponding expectations in schema_shape.rs should be updated too.

While there worth getting the validation to also reject patterns containing repeated trailing wildcards. For example, A** currently passes validation, but the runtime matcher silently ignores it, so the profile loads successfully without exporting the expected variables. I would have this enforce the same single-wildcard rule already used for allow_vars

@kipz
kipz force-pushed the kipz/tool-sandbox-inherit-vars branch from 5bffc69 to c1b327d Compare July 20, 2026 12:08
@kipz

kipz commented Jul 20, 2026

Copy link
Copy Markdown
Contributor Author

Makes sense @lukehinds - thanks! All done.

kipz added a commit to kipz/nono that referenced this pull request Jul 20, 2026
@lukehinds lukehinds added this to the 0.70 milestone Jul 27, 2026
kipz added a commit to kipz/nono that referenced this pull request Aug 3, 2026
@kipz
kipz force-pushed the kipz/tool-sandbox-inherit-vars branch from cec9c39 to 4ce3c94 Compare August 3, 2026 13:55
@kipz
kipz force-pushed the kipz/tool-sandbox-inherit-vars branch from 4ce3c94 to 8144357 Compare August 3, 2026 15:03
@kipz
kipz requested a review from lukehinds August 3, 2026 15:09
@kipz
kipz force-pushed the kipz/tool-sandbox-inherit-vars branch from 8144357 to ef281a9 Compare August 3, 2026 21:44
@kipz
kipz force-pushed the kipz/tool-sandbox-inherit-vars branch from ef281a9 to a931f5d Compare August 6, 2026 10:31
@kipz
kipz force-pushed the kipz/tool-sandbox-inherit-vars branch from a931f5d to f9a77e8 Compare August 9, 2026 11:14
kipz added a commit to kipz/nono that referenced this pull request Aug 9, 2026
kipz added a commit to kipz/nono that referenced this pull request Aug 9, 2026
@kipz
kipz force-pushed the kipz/tool-sandbox-inherit-vars branch from f9a77e8 to 7c77d60 Compare August 10, 2026 10:59
A command's `export_env` (or the top-level `session_export_env` when the
resolved caller is the session) lists env vars the caller forwards
verbatim to the commands it invokes, bypassing `allow_vars` and the
dangerous-var blocklist. This is the escape hatch for tools that must
pass an interpreter variable such as NODE_OPTIONS or PYTHONPATH to what
they launch.

Guardrails: `PATH` and `NONO_*` are never forwarded, 1Password
credentials are never forwarded by any pattern, and `LD_*`/`DYLD_*`
require an exact-name pattern so a bare `*` cannot preload code into a
credential-bearing child. Patterns are exact names, a trailing-`*`
prefix, or a bare `*`; multiple wildcards are rejected at validation
rather than silently never matching.

Signed-off-by: James Carnegie <me@kipz.org>
@kipz

kipz commented Aug 10, 2026

Copy link
Copy Markdown
Contributor Author

@lukehinds all three classes are now enforced, plus docs and the missing test coverage.

Actual secrets — always excluded, not forwardable by any pattern. is_forbidden_secret_env_var in exec_strategy/env_sanitization.rs covers OP_SERVICE_ACCOUNT_TOKEN, OP_CONNECT_TOKEN, OP_CONNECT_HOST and OP_SESSION_*, and apply_export_env checks it after pattern matching, so naming one exactly doesn't help. Test: export_env_never_forwards_1password_secrets asserts an empty result even for the list ["OP_SERVICE_ACCOUNT_TOKEN", "OP_SESSION_my", "*"].

Loader injection — exact name only, as you suggested. is_loader_injection_env_var matches LD_*/DYLD_*; those are skipped unless a pattern equals the variable name, so * and LD_* both miss them and a deliberate forward has to be spelled out and is therefore auditable in the profile. Tests: export_env_star_does_not_forward_loader_vars (list ["*", "LD_*", "DYLD_*"] → nothing) and export_env_exact_name_pattern_forwards_loader_var (["LD_PRELOAD"] → forwarded).

Interpreter/shell-scoped — unchanged. NODE_OPTIONS, PYTHONPATH, BASH_ENV still forward under *, which is the motivating use case. PATH and NONO_* remain excluded under every pattern.

Docs are in: docs/cli/features/tool-sandbox.mdx and the authoring guide both carry the export_env/session_export_env rows and the exclusion rules, and the fields are in the published JSON schema with matching schema_shape.rs expectations (your earlier point about additionalProperties: false, and the repeated-trailing-wildcard rule, are both done).

Since your review I've also closed two coverage gaps a second review pass flagged:

  • caller_export_env had no direct test on either platform — added on both, covering session → session_export_env, a named caller → its own export_env, and an unknown caller → empty. That last case matters: an unmediated wrapper must not inherit the session's exports by falling back.
  • export_env_cannot_displace_the_chaining_control_env — naming NONO_TOOL_SANDBOX_SOCKET exactly must not beat the NONO_* exclusion, otherwise a child could point its own shim at a socket it controls. Previously only the bare-* case was covered.

On trimming: env.rs came down as you asked in the inline thread.

Posted by an agent on behalf of @kipz.

@lukehinds lukehinds left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks , lgtm!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Let a command pass named env vars to the commands it invokes (caller-declared export_env)

3 participants