Skip to content

feat(#839): inject pre-script outputs into sandbox environment - #870

Open
fullsend-ai-coder[bot] wants to merge 2 commits into
mainfrom
agent/839-prescript-sandbox-flow
Open

feat(#839): inject pre-script outputs into sandbox environment#870
fullsend-ai-coder[bot] wants to merge 2 commits into
mainfrom
agent/839-prescript-sandbox-flow

Conversation

@fullsend-ai-coder

Copy link
Copy Markdown

Summary

Adds pre-script to sandbox data flow: non-reserved key=value outputs written to $FULLSEND_PRESCRIPT_OUTPUT are now injected as sandbox environment variables before the agent starts. This enables pre-scripts to pass computed values (resolved URLs, API tokens, feature flags) into the sandbox without requiring the harness to declare them statically in env.sandbox.

Related Issue

Closes #839

Changes

  • Add prescript.SandboxEnv() function that filters pre-script outputs, excluding reserved protocol keys (skipped, reason)
  • In runAgent (step 4-post), merge pre-script sandbox outputs into h.Env.Sandbox after the skip check and before sandbox creation — buildSandboxEnvLines in bootstrapEnv picks them up naturally
  • Pre-script outputs override static env.sandbox entries on key collision (runtime-computed values take precedence over static config)
  • Hyphenated keys (valid in the prescript protocol) pass through but are silently skipped by buildSandboxEnvLines since they are not valid POSIX env var names
  • Update normative prescript-output/v1 doc to describe sandbox injection semantics
  • Update ADR 0055 with a cross-reference to sandbox injection

Testing

  • Unit tests for SandboxEnv (nil/empty/reserved-only/mixed outputs/hyphenated keys)
  • Integration tests verifying outputs flow through runPreScriptSandboxEnvbuildSandboxEnvLines
  • Test that pre-script outputs override static env.sandbox entries
  • Test that hyphenated keys are accepted by SandboxEnv but filtered by buildSandboxEnvLines
  • Integration test via runAgent proving outputs reach sandbox creation
  • go test ./internal/prescript/... -race passes
  • go test ./internal/cli/... -race passes (targeted tests)
  • go build ./... succeeds
  • go vet clean

Checklist

  • PR title follows Conventional Commits (correct type, ! for breaking changes)
  • Commits are signed off (DCO) — human and human-directed agent sessions only

Closes #839

Post-script verification

  • Branch is not main/master (agent/839-prescript-sandbox-flow)
  • Secret scan passed (gitleaks — c887fc47c59d8b87b5282ca44044d2933d30df41..HEAD)
  • PR body secret scan passed (gitleaks — no-git)
  • Pre-commit hooks passed (authoritative run on runner)
  • Tests ran inside sandbox

Add prescript.SandboxEnv() to extract non-reserved key=value outputs
from pre-script results (excluding protocol keys skipped/reason).
In runAgent, merge these outputs into h.Env.Sandbox after the skip
check and before sandbox creation, so buildSandboxEnvLines picks
them up naturally. Pre-script outputs override static env.sandbox
entries on key collision — runtime-computed values take precedence
over static config.

Update normative prescript-output/v1 doc with sandbox injection
semantics and ADR 0055 with a cross-reference.

Note: pre-commit could not run in sandbox (network error fetching
hook repos). The post-script runs pre-commit authoritatively.

Closes #839
@fullsend-ai-review

fullsend-ai-review Bot commented Aug 2, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 9:45 AM UTC · Completed 10:01 AM UTC
Commit: 4012be5 · View workflow run →

@fullsend-ai-review

fullsend-ai-review Bot commented Aug 2, 2026

Copy link
Copy Markdown

Review

Findings

High

Medium

  • [adr-immutability-violation] docs/ADRs/0055-unified-env-var-delivery.md:132 — ADR 0055 has status Accepted on main. This PR replaces the Precedence subsection, changing a two-source model (env.sandbox > host_files) to a three-source model (pre-script outputs > static env.sandbox > host_files). Per docs/contributing/adrs.md: "Once an ADR on main has status Accepted, it is a point-in-time record. Do not substantially rewrite its Context, Decision, or Consequences sections." Write a new ADR that amends 0055 to introduce the pre-script output precedence tier; 0055 may receive a cross-reference annotation.

Low

  • [test-adequacy] internal/cli/prescript_run_test.go — No integration test covers the defense-in-depth reservedSandboxKeys filtering in runAgent. A test where a pre-script outputs PATH=evil and verifying it gets filtered before reaching h.Env.Sandbox would strengthen the contract.
  • [incomplete-reserved-key-list] internal/cli/run.goreservedSandboxKeys blocks LD_PRELOAD, BASH_ENV, etc. but omits other execution-influencing variables (NODE_OPTIONS, JAVA_TOOL_OPTIONS, IFS, PROMPT_COMMAND, GIT_CONFIG_GLOBAL). Pre-scripts are trusted (repo-owner authored), so this is defense-in-depth hardening, not a live vulnerability.
  • [stale-documentation] docs/guides/dev/cli-internals.md:439 — The bootstrapEnv() diagram omits env.sandbox exports (ADR 0055) and will also miss the new pre-script output injection. Pre-existing gap widened by this PR.

Next steps:

  • /fs-fix — agent addresses review findings automatically
  • /fs-fix <your instruction> — agent fixes with your specific guidance
  • Push commits directly — review re-runs automatically on push
  • /fs-fix-stop — disable automatic fix runs for this PR
Previous run

Review

Findings

Medium

  • [precedence-documentation-gap] docs/ADRs/0055-unified-env-var-delivery.md:132 — The pre-script output injection adds a third source of sandbox env vars (alongside static env.sandbox and host_files .env), but ADR 0055’s Precedence section only documents the two-way relationship between env.sandbox and host_files. The amendment paragraph establishes that pre-script outputs override static entries, but this three-way hierarchy is not reflected in the canonical Precedence section.
    Remediation: Update the Precedence section to document all three sources in order: (1) runtime pre-script outputs (highest), (2) static env.sandbox, (3) host_files .env entries (lowest).

Low

  • [misleading-output] internal/cli/run.go:976 — The StepDone message reports len(sandboxOutputs) as the count of injected sandbox env vars. SandboxEnv returns all non-reserved keys including hyphenated ones and keys matching reservedSandboxKeys, which buildSandboxEnvLines silently drops. The reported count can overstate the number of variables actually exported into the sandbox.
    Remediation: Adjust the log message to say "merged" rather than "injected," or filter non-POSIX keys in SandboxEnv so the count matches reality.

  • [defense-in-depth] internal/prescript/prescript.go:292SandboxEnv() filters only prescript-protocol reserved keys (skipped, reason) but does not filter sandbox-infrastructure reserved keys (PATH, LD_PRELOAD, FULLSEND_FETCH_TOKEN, etc.). While buildSandboxEnvLines() correctly filters these at rendering time, pre-script outputs land in h.Env.Sandbox unfiltered via maps.Copy. If a future code path reads h.Env.Sandbox directly, the reserved-key protection would be bypassed.
    Remediation: Add sandbox reservedSandboxKeys filtering to SandboxEnv() for defense-in-depth.

  • [comment-style] internal/cli/run.go:965 — The step label "4-post" is inconsistent with the established numbering convention in runAgent (0, 1, 1b, 1c, 2, 2a, 4, 4a, etc.). A label like "4b" would align with the existing pattern.


Labels: PR modifies sandbox environment injection in internal/cli and internal/prescript

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-review fullsend-ai-review Bot added requires-manual-review Review requires human judgment component/sandbox labels Aug 2, 2026
@guyoron1

guyoron1 commented Aug 2, 2026

Copy link
Copy Markdown
Owner

/fs-fix

@fullsend-ai-coder

fullsend-ai-coder Bot commented Aug 2, 2026

Copy link
Copy Markdown
Author

🤖 Finished Fix · ✅ Success · Started 10:32 AM UTC · Completed 10:42 AM UTC
Commit: 4012be5 · View workflow run →

- Update ADR 0055 Precedence section to document three-way sandbox env
  hierarchy: pre-script outputs > static env.sandbox > host_files .env
- Add defense-in-depth filtering of reservedSandboxKeys in the pre-script
  sandbox injection path, preventing reserved keys from reaching
  h.Env.Sandbox even if future code bypasses buildSandboxEnvLines
- Change StepDone message from "injected" to "merged" to accurately
  reflect that some outputs (hyphenated, reserved) may be filtered
  downstream
- Rename step comment "4-post" to "4a" and existing "4a" to "4b" to
  align with the established numbering convention in runAgent

Addresses review feedback on #870
@fullsend-ai-coder

Copy link
Copy Markdown
Author

🔧 Fix agent — iteration 1 (human-triggered)

Addressed all 4 review findings: updated ADR 0055 Precedence section for three-way hierarchy, changed misleading log message wording, added defense-in-depth filtering of reserved sandbox keys at the call site, and fixed step comment numbering convention.

Fixed (4):

  1. precedence-documentation-gap: ADR 0055 Precedence section missing three-way hierarchy (docs/ADRs/0055-unified-env-var-delivery.md): Updated the Precedence section to document all three sources of sandbox environment variables in priority order: (1) pre-script outputs (highest), (2) static env.sandbox, (3) host_files .env entries (lowest)
  2. misleading-output: StepDone count overstates actual injected vars (internal/cli/run.go): Changed log message from 'injected' to 'merged' to accurately reflect that some outputs (hyphenated keys, reserved sandbox keys) may be filtered downstream by buildSandboxEnvLines
  3. defense-in-depth: SandboxEnv does not filter sandbox-infrastructure reserved keys (internal/cli/run.go): Added defense-in-depth filtering of reservedSandboxKeys at the call site in runAgent, before maps.Copy into h.Env.Sandbox. This prevents reserved infrastructure keys (PATH, LD_PRELOAD, FULLSEND_FETCH_TOKEN, etc.) from reaching h.Env.Sandbox even if a future code path reads the map directly. Filtering is done at the call site rather than in SandboxEnv() because reservedSandboxKeys is a CLI-layer concern that the prescript package should not depend on.
  4. comment-style: step label 4-post inconsistent with numbering convention (internal/cli/run.go): Renamed step comment from '4-post' to '4a' and existing '4a. Create sandbox' to '4b. Create sandbox' to align with the established letter-suffix numbering convention in runAgent

Tests: passed

Next steps:

  • /fs-review — request a re-review of the changes
  • /fs-fix <your instruction> — run another fix pass with specific guidance
  • Push commits directly — review re-runs automatically on push
    Updated by fullsend fix agent

@fullsend-ai-review

fullsend-ai-review Bot commented Aug 2, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 10:44 AM UTC · Completed 11:01 AM UTC
Commit: b81e6c2 · View workflow run →

@fullsend-ai-review fullsend-ai-review 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.

See the review comment for full details.

executing pre/post scripts and the validation loop — identical to current
`runner_env` behavior.

Pre-script outputs (non-reserved keys written to `$FULLSEND_PRESCRIPT_OUTPUT`)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[medium] adr-immutability-violation

ADR 0055 has status Accepted on main. This PR replaces the Precedence subsection — changing a two-source model to a three-source model. Per docs/contributing/adrs.md, substantial rewrites to Context, Decision, or Consequences of Accepted ADRs are not permitted.

Suggested fix: Write a new ADR that amends 0055 to introduce the pre-script output precedence tier. ADR 0055 may receive a cross-reference annotation.

@fullsend-ai-review fullsend-ai-review Bot removed the requires-manual-review Review requires human judgment label Aug 2, 2026
@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown

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 stale label to reset the inactivity timer.

@github-actions github-actions Bot added the stale label Sep 2, 2026
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.

Support pre-script to sandbox data flow in the harness

1 participant