feat(harness): unified env var delivery (ADR 0055)#2582
Conversation
Add ADR 0055 introducing env: field with runner/sandbox sub-maps to the harness schema, deprecating runner_env and manual .env file convention. Includes cross-reference annotations on ADRs 0024 and 0049, architecture.md update, and implementation plan. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
Add the env: field to Harness and ForgeConfig per ADR 0055. This is the schema-only change — merge, resolution, and runtime behavior follow in subsequent commits. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
Wire EnvConfig into mergeForgeConfig so forge.<platform>.env sub-maps merge with top-level env following the same per-variable additive merge semantics as runner_env (ADR 0045). Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
Wire EnvConfig into mergeBaseIntoChild and mergeForgeConfigInto so env.runner and env.sandbox sub-maps merge correctly through base: chains following the same per-variable additive rules as runner_env. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
Lint() now emits a warning whenever runner_env is present, regardless of whether env: also exists. When both are present, the warning notes that env.runner takes precedence. Per ADR 0055. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
Extend ValidateRunnerEnvWith to also check env.runner and env.sandbox
var refs. The runner expands ${VAR} references in both sub-maps, then
merges env.runner over runner_env (env.runner wins on collision).
Deprecation warnings are emitted to stderr whenever runner_env is
present. Per ADR 0055.
Assisted-by: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Ralph Bean <rbean@redhat.com>
The runner now exports env.sandbox key-value pairs into the sandbox's .env file at bootstrap. These are placed before the .env.d sourcing loop so that manual .env files (if still present during migration) take precedence per ADR 0055's last-writer-wins guarantee. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
Add a regex guard to skip env var keys that are not valid POSIX shell identifiers. Keys are interpolated into shell export lines, so invalid keys (with spaces or special characters) could produce malformed or injectable shell. Also adds tests for invalid keys and empty values. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
Exercises base composition + forge resolution together to verify env.runner and env.sandbox merge correctly end-to-end. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
PR Summary by Qodofeat(harness): unify env var delivery via env.runner/env.sandbox (ADR 0055) Description
Diagram
High-Level Assessment
Files changed (16)
|
|
🤖 Finished Review · ✅ Success · Started 7:35 PM UTC · Completed 7:50 PM UTC |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Code Review by Qodo
1. False runner_env linting
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| ## 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. |
There was a problem hiding this comment.
1. Adr 0055 context too long 📜 Skill insight ⚙ Maintainability
docs/ADRs/0055-unified-env-var-delivery.md has more than 3 paragraphs in its ## Context section, exceeding the required 1–3 short paragraphs. This reduces ADR scannability and violates the required ADR format constraints.
Agent Prompt
## Issue description
The `## Context` section in ADR 0055 exceeds the allowed length (must be 1–3 short paragraphs).
## Issue Context
Compliance requires ADR Context sections to be concise. ADR 0055 currently contains multiple paragraphs plus a numbered list, pushing it beyond the allowed format.
## Fix Focus Areas
- docs/ADRs/0055-unified-env-var-delivery.md[23-46]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| # 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` and the manual `.env` file | ||
| convention. | ||
|
|
||
| ### Schema | ||
|
|
||
| ```yaml | ||
| env: | ||
| runner: | ||
| FULLSEND_OUTPUT_SCHEMA: "${FULLSEND_DIR}/schemas/review-result.schema.json" | ||
| sandbox: | ||
| 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. Replaces manual `.env` files | ||
| delivered via `host_files` with `expand: true`. | ||
| - 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. | ||
|
|
||
| ### Runner behavior | ||
|
|
||
| When `env.sandbox` is present (after all merges), the runner: | ||
|
|
||
| 1. Expands `${VAR}` references from the host environment. | ||
| 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. | ||
|
|
||
| ### 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`. | ||
|
|
||
| Manually-authored `.env` files delivered via `host_files` are not | ||
| automatically removed or skipped. Users migrate those entries into | ||
| `env.sandbox` at their own pace and remove the `host_files` entries | ||
| themselves. Both mechanisms coexist safely during migration. | ||
|
|
||
| ### Migration phases | ||
|
|
||
| **Phase 1 — Schema extension (this ADR):** Add `env:` to `Harness` and | ||
| `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 vars from manual `.env` files into | ||
| `env.sandbox`. Remove redundant `.env` host_files entries and `.env` files | ||
| from the scaffold. | ||
|
|
||
| **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. |
There was a problem hiding this comment.
2. Adr 0055 exceeds 100 lines 📜 Skill insight ⚙ Maintainability
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
## Issue description
ADR 0055 is longer than the allowed maximum of 100 content lines (excluding frontmatter).
## Issue Context
Compliance requires ADRs to stay concise; longer material should be moved to other docs (e.g., architecture docs) or shortened.
## Fix Focus Areas
- docs/ADRs/0055-unified-env-var-delivery.md[13-158]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| // Build effective runner env: start with runner_env, overlay env.runner. | ||
| effectiveRunnerEnv := make(map[string]string) | ||
| for k, v := range h.RunnerEnv { | ||
| effectiveRunnerEnv[k] = v | ||
| } | ||
| if h.Env != nil { | ||
| for k, v := range h.Env.Runner { | ||
| effectiveRunnerEnv[k] = v | ||
| } | ||
| } | ||
| h.RunnerEnv = effectiveRunnerEnv | ||
|
|
There was a problem hiding this comment.
3. False runner_env linting 🐞 Bug ≡ Correctness
runAgent overwrites h.RunnerEnv with the merged env.runner values and then calls h.Lint(),
so Lint()’s len(h.RunnerEnv) > 0 check can emit a runner_env deprecation warning even when the
YAML never had runner_env. This also prevents reliably warning on runner_env: {}
(present-but-empty) and makes deprecation reporting inconsistent with the intended migration
behavior.
Agent Prompt
## Issue description
`runAgent` currently merges `env.runner` into `h.RunnerEnv` (overwriting the original meaning of `RunnerEnv`) before running `h.Lint()`. Since `Lint()` decides whether to warn about deprecated `runner_env` using `len(h.RunnerEnv) > 0`, this can produce false deprecation warnings for harnesses that only use `env.runner`.
## Issue Context
The runner needs an *effective* runner env map (runner_env overlaid by env.runner), but `Harness.Lint()` needs to know whether deprecated `runner_env` was actually provided in the harness YAML.
## Fix Focus Areas
- internal/cli/run.go[347-403]
- internal/harness/lint.go[40-56]
### Suggested approach
- In `runAgent`, compute `effectiveRunnerEnv` as a local variable and use it for `envToList(...)` / downstream execution without overwriting `h.RunnerEnv`; OR capture a `hadRunnerEnv := (h.RunnerEnv != nil)` (or equivalent “present in YAML” signal) *before* any merging and ensure `Lint()` uses that signal.
- Ensure lint/deprecation warnings are evaluated before `RunnerEnv` is repurposed, and update the logic so `runner_env: {}` (present-but-empty) still emits a deprecation warning (per ADR intent).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
ReviewFindingsHigh
Medium
Previous runReviewFindingsHigh
Medium
Labels: PR modifies harness schema, runner bootstrap logic, and multiple ADRs/docs |
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
| // buildSandboxEnvLines generates export lines for env.sandbox values (ADR 0055). | ||
| // Values have already been expanded by the caller. Each value is single-quoted | ||
| // with internal single quotes escaped. Keys that are not valid shell identifiers | ||
| // are silently skipped. |
There was a problem hiding this comment.
[medium] injection
env.sandbox values can shadow security-critical infrastructure env vars (PATH, FULLSEND_FETCH_TOKEN, FULLSEND_FETCH_URL, FULLSEND_OUTPUT_DIR, FULLSEND_TARGET_REPO_DIR) in the generated sandbox .env file. buildSandboxEnvLines output is appended after infrastructure exports, so last-writer-wins shell semantics allow overriding runner-generated values.
Suggested fix: Add a deny-list of infrastructure-reserved variable names and either skip them with a warning or return a validation error.
fullsend-ai-review
Bot
added
component/harness
When child.Env is nil during base composition, the previous code assigned child.Env = base.Env, sharing the pointer and underlying maps. This creates aliasing where downstream mutations (e.g., forge resolution) would mutate the base's maps. Clone defensively instead, matching the pattern used for RunnerEnv. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
buildSandboxEnvLines silently dropped keys that failed the POSIX identifier regex. Add a stderr warning so harness authors get feedback when a key like MY-VAR is ignored instead of wondering why it didn't take effect. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
|
🤖 Finished Review · ✅ Success · Started 8:02 PM UTC · Completed 8:15 PM UTC |
| } | ||
| } | ||
| h.RunnerEnv = effectiveRunnerEnv | ||
|
|
There was a problem hiding this comment.
[high] logic-error
False positive deprecation warning from Lint(). The effectiveRunnerEnv merge copies env.runner entries into h.RunnerEnv before Lint() is called. A harness that declares only env.runner (the new recommended path, never using runner_env) will have h.RunnerEnv populated via the merge, and Lint() will see len(h.RunnerEnv) > 0, emitting a spurious 'runner_env is deprecated' warning. This was flagged in the prior review and remains unaddressed.
Suggested fix: Either (a) move the Lint() call before the effectiveRunnerEnv merge, (b) save the original RunnerEnv length before the merge and use that in Lint(), or (c) have Lint check a separate flag that records whether the original YAML contained runner_env.
| } | ||
|
|
||
| // ADR 0055: env.runner takes precedence over runner_env. | ||
| // Emit deprecation warning when runner_env is present. |
There was a problem hiding this comment.
[medium] logic-error
Duplicate deprecation warning for runner_env. When runner_env is present, the inline fmt.Fprintln emits a deprecation warning to stderr, and then Lint() also emits a deprecation diagnostic via emitDiagnostic. Both fire in the same run, producing two nearly identical warnings for the same issue.
Suggested fix: Remove the inline fmt.Fprintln deprecation warning and rely solely on Lint() (after fixing the false-positive ordering issue), or suppress the Lint diagnostic when the runtime warning has already been emitted.
| @@ -1261,6 +1329,10 @@ func bootstrapEnv(sandboxName, remoteRepositoryDir string, h *harness.Harness, r | |||
| lines = append(lines, fmt.Sprintf("export FULLSEND_FETCH_TOKEN='%s'", escToken)) | |||
There was a problem hiding this comment.
[medium] privilege-escalation
env.sandbox values can shadow security-critical infrastructure env vars (PATH, FULLSEND_FETCH_TOKEN, FULLSEND_FETCH_URL, FULLSEND_OUTPUT_DIR, FULLSEND_TARGET_REPO_DIR) in the generated sandbox .env file. In bootstrapEnv(), buildSandboxEnvLines output is appended after the infrastructure exports, so last-writer-wins shell semantics let a harness with env.sandbox.FULLSEND_FETCH_TOKEN override the runner-generated fetch token. The validEnvKeyRe regex validates key format but does not check for collisions with reserved names.
Suggested fix: Add a deny-list of infrastructure-reserved variable names and either skip them with a warning or reject during validation.
|
|
||
| When `env.sandbox` is present (after all merges), the runner: | ||
|
|
||
| 1. Expands `${VAR}` references from the host environment. |
There was a problem hiding this comment.
We need to define what that means, which kinds of expansion syntax do we support? only plain references? What about e.g. value prefix/suffix removal?
2 participants
Summary
env:top-level field to harness YAML withrunnerandsandboxsub-maps, per ADR 0055EnvConfiginto forge resolution, base composition, validation, and lint diagnosticsrunner_envemits deprecation warnings whenever present;env.runnertakes precedenceenv.sandboxat bootstrap (before.env.dsourcing so manual.envfiles win during migration)Closes #2575
Changes
internal/harness/harness.go—EnvConfigstruct,Envfield onHarness, extendedValidateRunnerEnvWithinternal/harness/forge.go—Envfield onForgeConfig, merge inmergeForgeConfiginternal/harness/compose.go— env merge inmergeBaseIntoChildandmergeForgeConfigIntointernal/harness/lint.go—Lint()deprecation diagnostics forrunner_envinternal/cli/run.go— env expansion, deprecation stderr warnings,env.runnerprecedence,buildSandboxEnvLinesTest plan
LoadWithBasepipeline withenv:+base:+forge:togethergo vetclean,make lintclean🤖 Generated with Claude Code