-
Notifications
You must be signed in to change notification settings - Fork 92
feat(harness): unified env var delivery (ADR 0055) #2582
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
ef49197
docs: ADR 0055 unified env var delivery and implementation plan
ralphbean c0f825a
feat(harness): add EnvConfig struct with runner/sandbox sub-maps
ralphbean 24f69cf
feat(harness): merge env: in forge resolution
ralphbean c5b4f40
feat(harness): merge env: in base composition
ralphbean a1754f9
feat(harness): lint deprecation warnings for runner_env
ralphbean e88108a
feat(runner): validate, expand, and apply env.runner with precedence
ralphbean cd291a7
feat(runner): generate sandbox env from env.sandbox
ralphbean 44f3208
fix(runner): validate env var keys in buildSandboxEnvLines
ralphbean 9942ac0
test(harness): integration test for env: through full load pipeline
ralphbean 56fc33f
fix(harness): clone EnvConfig in nil-child merge path
ralphbean 343194e
fix(runner): warn on invalid env.sandbox key names
ralphbean a024120
fix(runner): address PR review feedback on env delivery
ralphbean 0e1aec4
fix(runner): apply gofmt alignment to reservedSandboxKeys
ralphbean ec312e8
fix(harness): address review feedback on env delivery (ADR 0055)
ralphbean 1c7dfef
merge: resolve conflict with main in compose_test.go
ralphbean File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,190 @@ | ||
| --- | ||
| title: "55. Unified environment variable delivery for harness runner and sandbox" | ||
| status: Accepted | ||
| relates_to: | ||
| - agent-architecture | ||
| - agent-infrastructure | ||
| topics: | ||
| - harness | ||
| - configuration | ||
| - environment | ||
| --- | ||
|
|
||
| # 55. Unified environment variable delivery for harness runner and sandbox | ||
|
|
||
| Date: 2026-06-23 | ||
|
|
||
| Amends: [ADR 0024](0024-harness-definitions.md), [ADR 0049](0049-agent-configuration-env-var-convention.md) | ||
|
|
||
| ## Status | ||
|
|
||
| Accepted | ||
|
|
||
| ## Context | ||
|
|
||
| Setting an environment variable that needs to reach both the runner (pre/post | ||
| scripts) and the sandbox (agent inference) requires specifying it in two | ||
| independent mechanisms with different formats: | ||
|
|
||
| 1. `runner_env:` in the harness YAML — a key-value map for host-side scripts. | ||
| 2. A `.env` file under `env/` — shell `export` syntax, delivered via | ||
| `host_files` with `expand: true`. | ||
|
|
||
| ADR 0049 acknowledges this explicitly: "A config var needed by both must | ||
| appear in both places." | ||
|
|
||
| The `.env` file is especially painful to customize. It contains all | ||
| passthrough context vars (`GITHUB_PR_URL`, `GH_TOKEN`, `PR_NUMBER`, etc.). | ||
| Adding a single custom var like `REVIEW_FINDING_SEVERITY_THRESHOLD` forces | ||
| forking the entire file and maintaining all those passthroughs — see | ||
| [fullsend-ai/.fullsend#84](https://github.com/fullsend-ai/.fullsend/pull/84). | ||
|
|
||
| This separation was not an intentional design choice. It fell out of the | ||
| original `fullsend run` implementation (PR #231), which solved two different | ||
| runtime problems at different execution points and was later codified into | ||
| ADR 0024 without anyone asking whether a user should have to specify the same | ||
| var in two places. | ||
|
|
||
| ## Decision | ||
|
|
||
| Add a new `env:` top-level field to the harness schema with `runner` and | ||
| `sandbox` sub-maps. Deprecate `runner_env` in favor of `env.runner`. | ||
|
|
||
| `host_files` env delivery (`.env` files with `expand: true`) remains | ||
| permanently supported alongside `env.sandbox`. The two mechanisms are | ||
| complementary: `env.sandbox` is convenient for simple per-harness vars, | ||
| while `host_files` provides file-level composability that `env.sandbox` | ||
| cannot match (e.g. one `.env` file per tool, mix-and-matched across | ||
| harnesses without duplication). | ||
|
|
||
| ### Schema | ||
|
|
||
| ```yaml | ||
| env: | ||
| runner: | ||
| FULLSEND_OUTPUT_SCHEMA: "${FULLSEND_DIR}/schemas/review-result.schema.json" | ||
| sandbox: | ||
|
maruiz93 marked this conversation as resolved.
|
||
| GITHUB_PR_URL: "${GITHUB_PR_URL}" | ||
| GH_TOKEN: "${GH_TOKEN}" | ||
| REVIEW_FINDING_SEVERITY_THRESHOLD: "medium" | ||
| ``` | ||
|
|
||
| - `env.runner` — key-value pairs set in the host process environment for | ||
| pre/post scripts and the validation loop. Replaces `runner_env`. | ||
| - `env.sandbox` — key-value pairs the runner writes into a generated `.env` | ||
| file and copies into the sandbox at bootstrap. Complements (does not | ||
| replace) `.env` files delivered via `host_files`. | ||
| - Values in both sub-maps support `${VAR}` expansion from the host | ||
| environment, same as `runner_env` and `expand: true` host_files today. | ||
|
|
||
| The `env:` field can appear at the top level and inside `forge.<platform>` | ||
| blocks, replacing `runner_env` at both levels | ||
| ([ADR 0045](0045-forge-portable-harness-schema.md)). | ||
|
|
||
| Go struct: | ||
|
|
||
| ```go | ||
| type EnvConfig struct { | ||
| Runner map[string]string `yaml:"runner,omitempty"` | ||
| Sandbox map[string]string `yaml:"sandbox,omitempty"` | ||
| } | ||
| ``` | ||
|
|
||
| Added to both `Harness` and `ForgeConfig`: | ||
|
|
||
| ```go | ||
| Env *EnvConfig `yaml:"env,omitempty"` | ||
| ``` | ||
|
|
||
| ### Merge semantics | ||
|
|
||
| `env:` follows the same per-variable additive merge rules established by | ||
| ADR 0045 for `runner_env`: | ||
|
|
||
| - **`base:` composition** — parent map merged with child map; child keys win | ||
| on collision. Each sub-map (`runner`, `sandbox`) merges independently. A | ||
| child that declares only one sub-map inherits the other from the parent. | ||
| - **`forge.<platform>` resolution** — identical rules. Forge sub-maps merge | ||
| with top-level sub-maps; forge keys win. | ||
|
|
||
| **Limitation:** merge is strictly additive — there is no mechanism for a | ||
| child to remove a key inherited from its base. A child that inherits | ||
| `GITHUB_ISSUE_URL` from a base cannot suppress it; it can only override | ||
| the value. If removal semantics are needed in the future, a YAML `null` | ||
| / `~` sentinel could be added. | ||
|
|
||
| ### Runner behavior | ||
|
|
||
| When `env.sandbox` is present (after all merges), the runner: | ||
|
|
||
| 1. Expands `${VAR}` references from the host environment using Go's | ||
| `os.Expand`, which supports `$VAR` and `${VAR}` syntax only — no | ||
|
maruiz93 marked this conversation as resolved.
|
||
| default values, substring operations, or other shell parameter | ||
| expansion features. | ||
| 2. Writes the result as `KEY=value` lines to a generated `.env` file inside | ||
| the sandbox (e.g. `/sandbox/workspace/.env.d/generated.env`). | ||
| 3. The sandbox's `envfile.Load` picks it up normally. | ||
|
|
||
| `env.runner` sets key-value pairs in the host process environment before | ||
| executing pre/post scripts and the validation loop — identical to current | ||
| `runner_env` behavior. | ||
|
|
||
| ### Precedence | ||
|
|
||
| When both `env.sandbox` and `host_files` `.env` entries define the same | ||
| key, `env.sandbox` takes precedence. This is enforced by bootstrap | ||
| ordering: `.env.d/` files are sourced first, then `env.sandbox` exports | ||
| are emitted, so `env.sandbox` wins on collision. This matches the | ||
| expected use case: a harness inherits a shared `.env` file via | ||
| `host_files` and overrides a single var with `env.sandbox`. | ||
|
|
||
| ### Deprecation | ||
|
|
||
| `runner_env` **always** emits a deprecation warning when present, regardless | ||
| of whether `env:` also exists: | ||
|
|
||
| - When `env:` is also present: `env.runner` wins; warning says so. | ||
| - When `env:` is absent: `runner_env` still works; warning says | ||
| "migrate to env.runner." | ||
| - Same rules apply to `forge.<platform>.runner_env`. | ||
|
|
||
| `host_files` env delivery is **not deprecated**. It provides file-level | ||
| composability (one `.env` file per tool, mixed across harnesses) that | ||
| `env.sandbox` cannot structurally replicate. The two mechanisms coexist | ||
| permanently. | ||
|
|
||
| ### Migration phases | ||
|
|
||
| **Phase 1 — Schema extension (this ADR):** Add `env:` to `Harness` and | ||
|
maruiz93 marked this conversation as resolved.
|
||
| `ForgeConfig`. `runner_env` emits deprecation warnings whenever present. When | ||
| both exist, `env.runner` wins. Runner generates `.env` from `env.sandbox`. | ||
|
|
||
| **Phase 2 — Migrate scaffold harnesses:** Update all scaffold harnesses to | ||
| use `env:` instead of `runner_env`. Move simple passthrough vars from manual | ||
| `.env` files into `env.sandbox` where appropriate. Harnesses that use | ||
| modular per-tool `.env` files via `host_files` keep them. | ||
|
|
||
| **Phase 3 — Remove `runner_env`:** Remove `runner_env` from the Go structs. | ||
| `yaml.Unmarshal` silently ignores it in old files. `Lint()` emits an error | ||
| for harnesses that still reference it. | ||
|
|
||
| ## Consequences | ||
|
|
||
| - Adding a config var that both runner and sandbox need is a change to one | ||
| file (the harness YAML), not a fork of an entire `.env` file. | ||
| - `base:` composition works naturally — adding one config knob to a | ||
| customized harness is a few lines, not a full env file fork. | ||
| - No runner changes are needed for Phase 1 beyond generating the `.env` file | ||
| from `env.sandbox` and emitting deprecation warnings for `runner_env`. | ||
| - Existing harnesses continue to work unchanged; they just get noisier about | ||
| `runner_env` deprecation. | ||
| - ADR 0049's env var naming convention applies unchanged — the delivery | ||
| mechanism changes but the `{AGENT}_{SETTING_NAME}` convention does not. | ||
|
Comment on lines
+13
to
+182
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 2. Adr 0055 exceeds 100 lines ADR docs/ADRs/0055-unified-env-var-delivery.md exceeds the 100-line maximum content limit (excluding frontmatter), indicating it is too long for the required ADR format. This increases maintenance burden and makes the decision record harder to review. Agent Prompt
|
||
| - Modular `.env` files via `host_files` remain the right choice for | ||
| per-tool env groups shared across multiple harnesses. | ||
| - This change extends the harness schema; runners older than Phase 1 will | ||
| silently ignore `env:` and fall back to `runner_env` / `host_files` only. | ||
| Harness schema versioning ([#235](https://github.com/fullsend-ai/fullsend/issues/235)) | ||
| would make this evolution explicit. | ||
| - Env merge is strictly additive. A child cannot remove a key inherited from | ||
| its base — it can only override the value. | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1. Adr 0055 context too long
📜 Skill insight⚙ MaintainabilityAgent Prompt
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools