feat(#791): inject pre-script outputs into sandbox environment - #818
feat(#791): inject pre-script outputs into sandbox environment#818fullsend-ai-coder[bot] wants to merge 2 commits into
Conversation
Pre-scripts can now pass computed values (tokens, URLs, pre-fetched data) into the sandbox environment. Non-reserved outputs (everything except skipped/reason) written to FULLSEND_PRESCRIPT_OUTPUT are merged into env.sandbox after the pre-script completes, before sandbox creation. Add prescript.SandboxEnv() to filter outputs suitable for sandbox injection. In runAgent, merge these outputs into h.Env.Sandbox after the skip check (step 4-post), where buildSandboxEnvLines picks them up during bootstrapEnv. Pre-script outputs override static env.sandbox entries when keys collide, since the pre-script computes values from runtime context. Hyphenated keys (valid in the prescript protocol) pass through SandboxEnv but are silently skipped by buildSandboxEnvLines since they are not valid POSIX env var names. Update the normative prescript-output/v1 doc to describe sandbox injection semantics. Note: pre-commit could not run (network unavailable in sandbox). Closes #791
|
🤖 Finished Review · ✅ Success · Started 3:02 AM UTC · Completed 3:21 AM UTC |
ReviewFindingsMedium
Low
Previous runReviewFindingsMedium
Low
Labels: PR modifies sandbox env var injection in the prescript/CLI subsystem and adds a new feature |
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
requires-manual-review
|
/fs-fix |
|
🤖 Finished Fix · ✅ Success · Started 6:48 AM UTC · Completed 6:58 AM UTC |
- Filter non-POSIX keys (e.g. hyphenated) in SandboxEnv so injected count matches what buildSandboxEnvLines exports (key-format-mismatch) - Log when pre-script output overrides a static env.sandbox entry (privilege-escalation visibility) - Derive reservedOutputKeys from reservedKeys to keep sets in sync (maintainability) and move to package-level var section (variable-placement) - Add note to ADR 0055 about pre-script dynamic population (stale-doc) - Add TestSandboxEnv_FiltersHyphenatedKeys unit test Addresses review feedback on #818
🔧 Fix agent — iteration 1 (human-triggered)Addressed 6 of 7 review findings. Disagreed with 1 (insufficient-reserved-key-filtering) because implementing defense-in-depth would require either a circular dependency or a third independently maintained key set, both worse than the current architecture where buildSandboxEnvLines is the single enforcement point. Fixed (6):
Disagreed (1):
Tests: passedNext steps:
|
|
🤖 Finished Review · ✅ Success · Started 7:01 AM UTC · Completed 7:18 AM UTC |
| @@ -289,6 +305,33 @@ func Relay(res Result) (relayed bool, err error) { | |||
| return true, nil | |||
| } | |||
There was a problem hiding this comment.
[medium] defense-in-depth gap
SandboxEnv does not filter reservedSandboxKeys (PATH, HOME, LD_PRELOAD, BASH_ENV, etc.). A pre-script emitting one of these keys passes through SandboxEnv and gets inserted into h.Env.Sandbox, only to be silently dropped downstream by buildSandboxEnvLines. The 'Injected N' log message overcounts because it includes keys that will not actually reach the sandbox.
Suggested fix: Have SandboxEnv also filter against the reserved sandbox key set, or adjust the log to count only keys that survive buildSandboxEnvLines.
| @@ -199,6 +200,133 @@ func TestRunPreScript_CleansUpOutputFile(t *testing.T) { | |||
| assert.Empty(t, entries) | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
[low] test coverage gap
No test covers a pre-script output that collides with a reservedSandboxKey (e.g., PATH). Adding one would document and lock in the expected filtering behavior.
Suggested fix: Add a test that writes a reserved sandbox key via pre-script output and asserts it does not appear in buildSandboxEnvLines output.
| owned by the protocol; a future CLI may interpret additional lowercase | ||
| single-word keys, so scripts should prefix their own outputs (`myagent_pr=123`) | ||
| to avoid colliding with a future directive. | ||
| Any other valid key is parsed, logged, relayed, and — when the key is a valid |
There was a problem hiding this comment.
[low] scope-creep
The spec introductory purpose statement still describes the protocol solely for skip-gating. Consider updating to reflect the broader scope.
| return nil | ||
| } | ||
|
|
||
| // 4-post. Inject pre-script outputs into sandbox environment (#791). |
There was a problem hiding this comment.
[low] code-organization
Override log uses fmt.Fprintf(os.Stderr, ...) while adjacent messages use printer.StepDone. Consider using printer.StepInfo for consistency.
|
This pull request has been automatically marked as stale because it has not had any activity in the last month. It will be closed in 2 weeks if no further activity occurs. Remove the |
1 participant
Summary
Adds pre-script to sandbox data flow: non-reserved key=value outputs written to
$FULLSEND_PRESCRIPT_OUTPUTare now injected as sandbox environment variables before the agent starts.Related Issue
Closes #791
Changes
prescript.SandboxEnv()function that filters pre-script outputs, excluding reserved protocol keys (skipped,reason)runAgent(step 4-post), merge pre-script sandbox outputs intoh.Env.Sandboxafter the skip check and before sandbox creation —buildSandboxEnvLinesinbootstrapEnvpicks them up naturallyenv.sandboxentries on key collision (runtime-computed values take precedence over static config)buildSandboxEnvLinessince they are not valid POSIX env var namesprescript-output/v1doc to describe sandbox injection semanticsTesting
SandboxEnv(nil/empty/reserved-only/mixed outputs)runPreScript→SandboxEnv→buildSandboxEnvLinesenv.sandboxentriesSandboxEnvbut filtered bybuildSandboxEnvLinesrunAgentproving outputs reach sandbox creationgo test ./internal/prescript/... ./internal/cli/... -racepassesgo build ./...succeedsgo vetcleanChecklist
!for breaking changes)Closes #791
Post-script verification
agent/791-prescript-output-flow)ba77dbdbbd17e9bb18c47bf6efa023c98b697158..HEAD)