From ef49197a62e4557ac127496cfebd67f7d6678d25 Mon Sep 17 00:00:00 2001 From: Ralph Bean Date: Tue, 23 Jun 2026 14:37:35 -0400 Subject: [PATCH 01/14] docs: ADR 0055 unified env var delivery and implementation plan 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 Signed-off-by: Ralph Bean --- docs/ADRs/0024-harness-definitions.md | 4 + ...-agent-configuration-env-var-convention.md | 7 +- docs/ADRs/0055-unified-env-var-delivery.md | 158 +++ docs/architecture.md | 11 +- .../2026-06-23-unified-env-var-delivery.md | 952 ++++++++++++++++++ 5 files changed, 1128 insertions(+), 4 deletions(-) create mode 100644 docs/ADRs/0055-unified-env-var-delivery.md create mode 100644 docs/superpowers/plans/2026-06-23-unified-env-var-delivery.md diff --git a/docs/ADRs/0024-harness-definitions.md b/docs/ADRs/0024-harness-definitions.md index bf9f13425..232439e15 100644 --- a/docs/ADRs/0024-harness-definitions.md +++ b/docs/ADRs/0024-harness-definitions.md @@ -20,6 +20,10 @@ Date: 2026-04-07 Accepted +*Amended by [ADR 0055](0055-unified-env-var-delivery.md), which introduces a +unified `env:` key with `runner`/`sandbox` sub-maps and deprecates `runner_env` +and the manual `.env` file convention.* + ## Context Each agent invocation requires configuration that ties together several moving diff --git a/docs/ADRs/0049-agent-configuration-env-var-convention.md b/docs/ADRs/0049-agent-configuration-env-var-convention.md index 3c61f41aa..c282f1321 100644 --- a/docs/ADRs/0049-agent-configuration-env-var-convention.md +++ b/docs/ADRs/0049-agent-configuration-env-var-convention.md @@ -71,7 +71,12 @@ documentation: config vars are behavioral knobs listed in ### Where config vars live in the harness Config vars are carried the same way as other agent env vars — no new schema -fields are needed. The `.env` file and `runner_env` serve different +fields are needed. *Note: [ADR 0055](0055-unified-env-var-delivery.md) +introduces a unified `env:` key with `runner`/`sandbox` sub-maps that +replaces `runner_env` and manual `.env` files. The delivery mechanism below +still works but is deprecated in favor of `env.runner` and `env.sandbox`.* + +The `.env` file and `runner_env` serve different audiences: the `.env` file delivers vars into the sandbox for the agent at inference time, while `runner_env` makes vars available to pre/post scripts on the host. A config var needed by both must appear in both places. diff --git a/docs/ADRs/0055-unified-env-var-delivery.md b/docs/ADRs/0055-unified-env-var-delivery.md new file mode 100644 index 000000000..ef7d300e3 --- /dev/null +++ b/docs/ADRs/0055-unified-env-var-delivery.md @@ -0,0 +1,158 @@ +--- +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` 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.` +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.` 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..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. diff --git a/docs/architecture.md b/docs/architecture.md index bc1148c1b..a08fae25b 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -91,10 +91,15 @@ The harness draws its configuration from the adopting organization's **`.fullsen runner_env) from platform-neutral fields. Forge blocks inherit from top-level defaults and override only deltas ([ADR 0045](ADRs/0045-forge-portable-harness-schema.md)). +- Unified env var delivery: a single `env:` key with `runner` and `sandbox` + sub-maps replaces `runner_env` and manual `.env` files. The runner generates + the sandbox `.env` file from `env.sandbox` at bootstrap. `runner_env` is + deprecated ([ADR 0055](ADRs/0055-unified-env-var-delivery.md), amending + [ADR 0024](ADRs/0024-harness-definitions.md)). - Agent configuration env vars: behavioral knobs use `{AGENT}_{SETTING_NAME}` - naming (e.g., `REVIEW_SEVERITY_THRESHOLD`), delivered via existing env var - mechanisms (`.env` files, `runner_env`). Each agent documents its config - vars in `docs/agents/.md` + naming (e.g., `REVIEW_SEVERITY_THRESHOLD`), delivered via `env.runner` and + `env.sandbox` in the harness YAML. Each agent documents its config vars in + `docs/agents/.md` ([ADR 0049](ADRs/0049-agent-configuration-env-var-convention.md)). - Agent-driven branch targeting: the code agent writes its chosen target branch to structured output. The post-script validates the choice against diff --git a/docs/superpowers/plans/2026-06-23-unified-env-var-delivery.md b/docs/superpowers/plans/2026-06-23-unified-env-var-delivery.md new file mode 100644 index 000000000..b0bcffb51 --- /dev/null +++ b/docs/superpowers/plans/2026-06-23-unified-env-var-delivery.md @@ -0,0 +1,952 @@ +# Unified Env Var Delivery Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Add `env:` field with `runner`/`sandbox` sub-maps to the harness schema, deprecating `runner_env` and manual `.env` files per ADR 0055. + +**Architecture:** Add `EnvConfig` struct to the harness package. Wire it into forge resolution, base composition, and validation. Add `Lint()` deprecation diagnostics for `runner_env`. Update the runner to expand and apply `env.runner` and generate sandbox `.env` files from `env.sandbox`. Emit deprecation warnings at runtime when `runner_env` is present. + +**Tech Stack:** Go, YAML (`gopkg.in/yaml.v3`), existing harness/envfile packages + +--- + +### Task 1: Add `EnvConfig` struct and wire into `Harness` / `ForgeConfig` + +**Files:** +- Modify: `internal/harness/harness.go:195-224` (Harness struct) +- Modify: `internal/harness/forge.go:9-20` (ForgeConfig struct) +- Test: `internal/harness/harness_test.go` + +- [ ] **Step 1: Write failing test for EnvConfig parsing** + +In `internal/harness/harness_test.go`, add: + +```go +func TestEnvConfig_ParsesFromYAML(t *testing.T) { + yaml := ` +agent: agents/test.md +role: test +env: + runner: + FOO: bar + sandbox: + BAZ: qux +` + h, err := parseRaw([]byte(yaml)) + require.NoError(t, err) + require.NotNil(t, h.Env) + assert.Equal(t, map[string]string{"FOO": "bar"}, h.Env.Runner) + assert.Equal(t, map[string]string{"BAZ": "qux"}, h.Env.Sandbox) +} +``` + +- [ ] **Step 2: Run test to verify it fails** + +Run: `cd /home/rbean/code/fullsend-0 && go test ./internal/harness/ -run TestEnvConfig_ParsesFromYAML -v` +Expected: FAIL — `h.Env` is nil because the struct field doesn't exist yet. + +- [ ] **Step 3: Add EnvConfig struct and field to Harness** + +In `internal/harness/harness.go`, add the struct before the `Harness` type: + +```go +// EnvConfig holds environment variable maps for runner and sandbox targets. +// Replaces runner_env (ADR 0055). Values support ${VAR} expansion from the +// host environment. +type EnvConfig struct { + Runner map[string]string `yaml:"runner,omitempty"` + Sandbox map[string]string `yaml:"sandbox,omitempty"` +} +``` + +Add the field to the `Harness` struct, after `RunnerEnv`: + +```go +Env *EnvConfig `yaml:"env,omitempty"` +``` + +- [ ] **Step 4: Run test to verify it passes** + +Run: `cd /home/rbean/code/fullsend-0 && go test ./internal/harness/ -run TestEnvConfig_ParsesFromYAML -v` +Expected: PASS + +- [ ] **Step 5: Write failing test for EnvConfig in ForgeConfig** + +In `internal/harness/forge_test.go`, add: + +```go +func TestForgeConfig_EnvParsesFromYAML(t *testing.T) { + yaml := ` +agent: agents/test.md +role: test +forge: + github: + env: + runner: + GH_TOKEN: "${GH_TOKEN}" + sandbox: + GITHUB_PR_URL: "${GITHUB_PR_URL}" +` + h, err := parseRaw([]byte(yaml)) + require.NoError(t, err) + require.NotNil(t, h.Forge["github"]) + require.NotNil(t, h.Forge["github"].Env) + assert.Equal(t, map[string]string{"GH_TOKEN": "${GH_TOKEN}"}, h.Forge["github"].Env.Runner) + assert.Equal(t, map[string]string{"GITHUB_PR_URL": "${GITHUB_PR_URL}"}, h.Forge["github"].Env.Sandbox) +} +``` + +- [ ] **Step 6: Run test to verify it fails** + +Run: `cd /home/rbean/code/fullsend-0 && go test ./internal/harness/ -run TestForgeConfig_EnvParsesFromYAML -v` +Expected: FAIL — `ForgeConfig` has no `Env` field. + +- [ ] **Step 7: Add Env field to ForgeConfig** + +In `internal/harness/forge.go`, add to `ForgeConfig`: + +```go +Env *EnvConfig `yaml:"env,omitempty"` +``` + +- [ ] **Step 8: Run test to verify it passes** + +Run: `cd /home/rbean/code/fullsend-0 && go test ./internal/harness/ -run TestForgeConfig_EnvParsesFromYAML -v` +Expected: PASS + +- [ ] **Step 9: Run full harness test suite** + +Run: `cd /home/rbean/code/fullsend-0 && go test ./internal/harness/ -v` +Expected: All tests pass. No existing tests should break. + +- [ ] **Step 10: Commit** + +```bash +git add internal/harness/harness.go internal/harness/forge.go internal/harness/harness_test.go internal/harness/forge_test.go +git commit -S -s -m "$(cat <<'EOF' +feat(harness): add EnvConfig struct with runner/sandbox sub-maps + +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 +EOF +)" +``` + +--- + +### Task 2: Wire `env:` into forge resolution (`mergeForgeConfig`) + +**Files:** +- Modify: `internal/harness/forge.go:112-136` (mergeForgeConfig) +- Test: `internal/harness/forge_test.go` + +- [ ] **Step 1: Write failing test for env merge in forge resolution** + +In `internal/harness/forge_test.go`, add: + +```go +func TestResolveForge_MergesEnv(t *testing.T) { + h := &Harness{ + Agent: "agents/test.md", + Role: "test", + Env: &EnvConfig{ + Runner: map[string]string{"SHARED": "base"}, + Sandbox: map[string]string{"SHARED_SB": "base"}, + }, + Forge: map[string]*ForgeConfig{ + "github": { + Env: &EnvConfig{ + Runner: map[string]string{"GH_TOKEN": "tok"}, + Sandbox: map[string]string{"PR_URL": "url"}, + }, + }, + }, + } + + require.NoError(t, h.ResolveForge("github")) + + require.NotNil(t, h.Env) + assert.Equal(t, "base", h.Env.Runner["SHARED"]) + assert.Equal(t, "tok", h.Env.Runner["GH_TOKEN"]) + assert.Equal(t, "base", h.Env.Sandbox["SHARED_SB"]) + assert.Equal(t, "url", h.Env.Sandbox["PR_URL"]) +} + +func TestResolveForge_EnvForgeOverridesTopLevel(t *testing.T) { + h := &Harness{ + Agent: "agents/test.md", + Role: "test", + Env: &EnvConfig{ + Runner: map[string]string{"KEY": "top"}, + }, + Forge: map[string]*ForgeConfig{ + "github": { + Env: &EnvConfig{ + Runner: map[string]string{"KEY": "forge"}, + }, + }, + }, + } + + require.NoError(t, h.ResolveForge("github")) + assert.Equal(t, "forge", h.Env.Runner["KEY"]) +} + +func TestResolveForge_EnvInheritedWhenForgeNil(t *testing.T) { + h := &Harness{ + Agent: "agents/test.md", + Role: "test", + Env: &EnvConfig{ + Runner: map[string]string{"INHERITED": "yes"}, + Sandbox: map[string]string{"ALSO": "inherited"}, + }, + Forge: map[string]*ForgeConfig{ + "github": {}, + }, + } + + require.NoError(t, h.ResolveForge("github")) + + require.NotNil(t, h.Env) + assert.Equal(t, "yes", h.Env.Runner["INHERITED"]) + assert.Equal(t, "inherited", h.Env.Sandbox["ALSO"]) +} +``` + +- [ ] **Step 2: Run tests to verify they fail** + +Run: `cd /home/rbean/code/fullsend-0 && go test ./internal/harness/ -run TestResolveForge_.*Env -v` +Expected: FAIL — `mergeForgeConfig` doesn't handle `Env`. + +- [ ] **Step 3: Add env merge logic to `mergeForgeConfig`** + +In `internal/harness/forge.go`, add to `mergeForgeConfig` after the `ValidationLoop` block: + +```go + // Env: merge sub-maps independently; forge keys win (ADR 0055) + if fc.Env != nil { + if h.Env == nil { + h.Env = &EnvConfig{} + } + if fc.Env.Runner != nil { + if h.Env.Runner == nil { + h.Env.Runner = make(map[string]string, len(fc.Env.Runner)) + } + for k, v := range fc.Env.Runner { + h.Env.Runner[k] = v + } + } + if fc.Env.Sandbox != nil { + if h.Env.Sandbox == nil { + h.Env.Sandbox = make(map[string]string, len(fc.Env.Sandbox)) + } + for k, v := range fc.Env.Sandbox { + h.Env.Sandbox[k] = v + } + } + } +``` + +- [ ] **Step 4: Run tests to verify they pass** + +Run: `cd /home/rbean/code/fullsend-0 && go test ./internal/harness/ -run TestResolveForge_.*Env -v` +Expected: PASS + +- [ ] **Step 5: Run full harness test suite** + +Run: `cd /home/rbean/code/fullsend-0 && go test ./internal/harness/ -v` +Expected: All tests pass. + +- [ ] **Step 6: Commit** + +```bash +git add internal/harness/forge.go internal/harness/forge_test.go +git commit -S -s -m "$(cat <<'EOF' +feat(harness): merge env: in forge resolution + +Wire EnvConfig into mergeForgeConfig so forge..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 +EOF +)" +``` + +--- + +### Task 3: Wire `env:` into base composition (`mergeBaseIntoChild`) + +**Files:** +- Modify: `internal/harness/compose.go:372-477` (mergeBaseIntoChild) +- Modify: `internal/harness/compose.go:827-864` (mergeForgeConfigInto) +- Test: `internal/harness/compose_test.go` + +- [ ] **Step 1: Write failing test for env merge in base composition** + +In `internal/harness/compose_test.go`, add: + +```go +func TestMergeBaseIntoChild_Env(t *testing.T) { + base := &Harness{ + Env: &EnvConfig{ + Runner: map[string]string{"BASE_R": "r1"}, + Sandbox: map[string]string{"BASE_S": "s1"}, + }, + } + child := &Harness{ + Env: &EnvConfig{ + Sandbox: map[string]string{"CHILD_S": "s2"}, + }, + } + + mergeBaseIntoChild(base, child) + + require.NotNil(t, child.Env) + assert.Equal(t, "r1", child.Env.Runner["BASE_R"]) + assert.Equal(t, "s1", child.Env.Sandbox["BASE_S"]) + assert.Equal(t, "s2", child.Env.Sandbox["CHILD_S"]) +} + +func TestMergeBaseIntoChild_EnvChildWins(t *testing.T) { + base := &Harness{ + Env: &EnvConfig{ + Runner: map[string]string{"KEY": "base"}, + }, + } + child := &Harness{ + Env: &EnvConfig{ + Runner: map[string]string{"KEY": "child"}, + }, + } + + mergeBaseIntoChild(base, child) + assert.Equal(t, "child", child.Env.Runner["KEY"]) +} + +func TestMergeBaseIntoChild_EnvInheritedWhenChildNil(t *testing.T) { + base := &Harness{ + Env: &EnvConfig{ + Runner: map[string]string{"R": "val"}, + Sandbox: map[string]string{"S": "val"}, + }, + } + child := &Harness{} + + mergeBaseIntoChild(base, child) + + require.NotNil(t, child.Env) + assert.Equal(t, "val", child.Env.Runner["R"]) + assert.Equal(t, "val", child.Env.Sandbox["S"]) +} +``` + +- [ ] **Step 2: Run tests to verify they fail** + +Run: `cd /home/rbean/code/fullsend-0 && go test ./internal/harness/ -run TestMergeBaseIntoChild_Env -v` +Expected: FAIL — `mergeBaseIntoChild` doesn't handle `Env`. + +- [ ] **Step 3: Add env merge logic to `mergeBaseIntoChild`** + +In `internal/harness/compose.go`, in the `mergeBaseIntoChild` function, after the `RunnerEnv` merge block (around line 460), add: + +```go + // Env: merge sub-maps independently, child keys win (ADR 0055) + if base.Env != nil { + if child.Env == nil { + child.Env = base.Env + } else { + if base.Env.Runner != nil { + if child.Env.Runner == nil { + child.Env.Runner = make(map[string]string, len(base.Env.Runner)) + } + for k, v := range base.Env.Runner { + if _, exists := child.Env.Runner[k]; !exists { + child.Env.Runner[k] = v + } + } + } + if base.Env.Sandbox != nil { + if child.Env.Sandbox == nil { + child.Env.Sandbox = make(map[string]string, len(base.Env.Sandbox)) + } + for k, v := range base.Env.Sandbox { + if _, exists := child.Env.Sandbox[k]; !exists { + child.Env.Sandbox[k] = v + } + } + } + } + } +``` + +- [ ] **Step 4: Run tests to verify they pass** + +Run: `cd /home/rbean/code/fullsend-0 && go test ./internal/harness/ -run TestMergeBaseIntoChild_Env -v` +Expected: PASS + +- [ ] **Step 5: Add env merge to `mergeForgeConfigInto` (for base forge blocks)** + +In `internal/harness/compose.go`, in the `mergeForgeConfigInto` function, after the `RunnerEnv` block, add: + +```go + // Env: merge sub-maps, child keys win (ADR 0055) + if base.Env != nil { + if child.Env == nil { + child.Env = base.Env + } else { + if base.Env.Runner != nil { + if child.Env.Runner == nil { + child.Env.Runner = make(map[string]string, len(base.Env.Runner)) + } + for k, v := range base.Env.Runner { + if _, exists := child.Env.Runner[k]; !exists { + child.Env.Runner[k] = v + } + } + } + if base.Env.Sandbox != nil { + if child.Env.Sandbox == nil { + child.Env.Sandbox = make(map[string]string, len(base.Env.Sandbox)) + } + for k, v := range base.Env.Sandbox { + if _, exists := child.Env.Sandbox[k]; !exists { + child.Env.Sandbox[k] = v + } + } + } + } + } +``` + +- [ ] **Step 6: Run full harness test suite** + +Run: `cd /home/rbean/code/fullsend-0 && go test ./internal/harness/ -v` +Expected: All tests pass. + +- [ ] **Step 7: Commit** + +```bash +git add internal/harness/compose.go internal/harness/compose_test.go +git commit -S -s -m "$(cat <<'EOF' +feat(harness): merge env: in base composition + +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 +EOF +)" +``` + +--- + +### Task 4: Add `Lint()` deprecation diagnostics for `runner_env` + +**Files:** +- Modify: `internal/harness/lint.go:40-42` (Lint method) +- Test: `internal/harness/lint_test.go` + +- [ ] **Step 1: Write failing tests for Lint deprecation warnings** + +In `internal/harness/lint_test.go`, add: + +```go +func TestLint_RunnerEnvDeprecated(t *testing.T) { + h := &Harness{ + Agent: "agents/test.md", + Role: "test", + RunnerEnv: map[string]string{"FOO": "bar"}, + } + + diags := h.Lint() + require.Len(t, diags, 1) + assert.Equal(t, SeverityWarning, diags[0].Severity) + assert.Equal(t, "runner_env", diags[0].Field) + assert.Contains(t, diags[0].Message, "deprecated") + assert.Contains(t, diags[0].Message, "env.runner") +} + +func TestLint_RunnerEnvAndEnvBothPresent(t *testing.T) { + h := &Harness{ + Agent: "agents/test.md", + Role: "test", + RunnerEnv: map[string]string{"FOO": "bar"}, + Env: &EnvConfig{Runner: map[string]string{"BAZ": "qux"}}, + } + + diags := h.Lint() + require.Len(t, diags, 1) + assert.Equal(t, SeverityWarning, diags[0].Severity) + assert.Contains(t, diags[0].Message, "env.runner takes precedence") +} + +func TestLint_NoWarningWithoutRunnerEnv(t *testing.T) { + h := &Harness{ + Agent: "agents/test.md", + Role: "test", + Env: &EnvConfig{Runner: map[string]string{"FOO": "bar"}}, + } + + diags := h.Lint() + assert.Empty(t, diags) +} +``` + +- [ ] **Step 2: Run tests to verify they fail** + +Run: `cd /home/rbean/code/fullsend-0 && go test ./internal/harness/ -run TestLint_ -v` +Expected: FAIL — `Lint()` returns nil. + +- [ ] **Step 3: Implement Lint deprecation checks** + +Replace the `Lint()` method body in `internal/harness/lint.go`: + +```go +func (h *Harness) Lint() []Diagnostic { + var diags []Diagnostic + + if len(h.RunnerEnv) > 0 { + msg := "runner_env is deprecated; use env.runner instead (see ADR 0055)" + if h.Env != nil && len(h.Env.Runner) > 0 { + msg = "runner_env is deprecated and env.runner takes precedence; migrate to env.runner (see ADR 0055)" + } + diags = append(diags, Diagnostic{ + Severity: SeverityWarning, + Field: "runner_env", + Message: msg, + }) + } + + return diags +} +``` + +- [ ] **Step 4: Run tests to verify they pass** + +Run: `cd /home/rbean/code/fullsend-0 && go test ./internal/harness/ -run TestLint_ -v` +Expected: PASS + +- [ ] **Step 5: Run full harness test suite** + +Run: `cd /home/rbean/code/fullsend-0 && go test ./internal/harness/ -v` +Expected: All tests pass. + +- [ ] **Step 6: Commit** + +```bash +git add internal/harness/lint.go internal/harness/lint_test.go +git commit -S -s -m "$(cat <<'EOF' +feat(harness): lint deprecation warnings for runner_env + +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 +EOF +)" +``` + +--- + +### Task 5: Extend `ValidateRunnerEnvWith` to check `env:` and expand in the runner + +**Files:** +- Modify: `internal/harness/harness.go:490-519` (ValidateRunnerEnvWith) +- Modify: `internal/cli/run.go:327-348` (expand env.runner, apply precedence) +- Test: `internal/harness/harness_test.go` + +- [ ] **Step 1: Write failing test for ValidateRunnerEnvWith checking env field** + +In `internal/harness/harness_test.go`, add: + +```go +func TestValidateRunnerEnvWith_ChecksEnvRunner(t *testing.T) { + h := &Harness{ + Agent: "agents/test.md", + Role: "test", + Env: &EnvConfig{ + Runner: map[string]string{"KEY": "${MISSING_VAR}"}, + }, + } + lookup := func(key string) (string, bool) { return "", false } + err := h.ValidateRunnerEnvWith(lookup) + require.Error(t, err) + assert.Contains(t, err.Error(), "MISSING_VAR") +} + +func TestValidateRunnerEnvWith_ChecksEnvSandbox(t *testing.T) { + h := &Harness{ + Agent: "agents/test.md", + Role: "test", + Env: &EnvConfig{ + Sandbox: map[string]string{"KEY": "${ALSO_MISSING}"}, + }, + } + lookup := func(key string) (string, bool) { return "", false } + err := h.ValidateRunnerEnvWith(lookup) + require.Error(t, err) + assert.Contains(t, err.Error(), "ALSO_MISSING") +} + +func TestValidateRunnerEnvWith_EnvAllSet(t *testing.T) { + h := &Harness{ + Agent: "agents/test.md", + Role: "test", + Env: &EnvConfig{ + Runner: map[string]string{"KEY": "${SET_VAR}"}, + Sandbox: map[string]string{"KEY2": "literal"}, + }, + } + lookup := func(key string) (string, bool) { + if key == "SET_VAR" { + return "val", true + } + return "", false + } + err := h.ValidateRunnerEnvWith(lookup) + require.NoError(t, err) +} + +func TestValidateRunnerEnvWith_NilEnvNoError(t *testing.T) { + h := &Harness{Agent: "agents/test.md", Role: "test"} + err := h.ValidateRunnerEnvWith(func(string) (string, bool) { return "", false }) + require.NoError(t, err) +} +``` + +- [ ] **Step 2: Run tests to verify they fail** + +Run: `cd /home/rbean/code/fullsend-0 && go test ./internal/harness/ -run TestValidateRunnerEnvWith_ChecksEnv -v` +Expected: FAIL — `ValidateRunnerEnvWith` doesn't check `Env`. + +- [ ] **Step 3: Extend `ValidateRunnerEnvWith` to check `Env` field** + +In `internal/harness/harness.go`, in `ValidateRunnerEnvWith`, after the `HostFiles` loop and before the `return nil`, add: + +```go + if h.Env != nil { + for k, v := range h.Env.Runner { + if err := checkVarRefs(fmt.Sprintf("env.runner[%s]", k), v); err != nil { + return err + } + } + for k, v := range h.Env.Sandbox { + if err := checkVarRefs(fmt.Sprintf("env.sandbox[%s]", k), v); err != nil { + return err + } + } + } +``` + +- [ ] **Step 4: Run tests to verify they pass** + +Run: `cd /home/rbean/code/fullsend-0 && go test ./internal/harness/ -run TestValidateRunnerEnvWith -v` +Expected: PASS + +- [ ] **Step 5: Wire env.runner expansion and precedence into the runner** + +In `internal/cli/run.go`, find the block (around line 342) that expands `RunnerEnv`. After the existing expansion loop for `RunnerEnv`, add: + +```go + // Expand ${VAR} references in env.runner and env.sandbox (ADR 0055). + if h.Env != nil { + for k, v := range h.Env.Runner { + h.Env.Runner[k] = os.Expand(v, expander) + } + for k, v := range h.Env.Sandbox { + h.Env.Sandbox[k] = os.Expand(v, expander) + } + } + + // ADR 0055: env.runner takes precedence over runner_env. + // Emit deprecation warning when runner_env is present. + if len(h.RunnerEnv) > 0 { + if h.Env != nil && len(h.Env.Runner) > 0 { + fmt.Fprintln(os.Stderr, "WARNING: runner_env is deprecated and env.runner takes precedence; migrate to env.runner (see ADR 0055)") + } else { + fmt.Fprintln(os.Stderr, "WARNING: runner_env is deprecated; use env.runner instead (see ADR 0055)") + } + } + + // 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 +``` + +This preserves backward compatibility: `RunnerEnv` is still what gets passed to `envToList()` everywhere, but now it's the merged result with `env.runner` winning on collision. + +- [ ] **Step 6: Run full test suite** + +Run: `cd /home/rbean/code/fullsend-0 && go test ./internal/harness/ ./internal/cli/ -v` +Expected: All tests pass. + +- [ ] **Step 7: Commit** + +```bash +git add internal/harness/harness.go internal/harness/harness_test.go internal/cli/run.go +git commit -S -s -m "$(cat <<'EOF' +feat(runner): validate, expand, and apply env.runner with precedence + +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 +EOF +)" +``` + +--- + +### Task 6: Generate sandbox `.env` file from `env.sandbox` + +**Files:** +- Modify: `internal/cli/run.go:1217-1268` (bootstrapEnv function) +- Test: `internal/cli/run_test.go` + +- [ ] **Step 1: Write failing test for sandbox env generation** + +In `internal/cli/run_test.go`, add (or find the appropriate test location): + +```go +func TestBuildSandboxEnvLines_FromEnvSandbox(t *testing.T) { + h := &harness.Harness{ + Agent: "agents/test.md", + Role: "test", + Env: &harness.EnvConfig{ + Sandbox: map[string]string{ + "GITHUB_PR_URL": "https://github.com/org/repo/pull/1", + "GH_TOKEN": "tok123", + }, + }, + } + + lines := buildSandboxEnvLines(h) + assert.Contains(t, lines, "export GITHUB_PR_URL='https://github.com/org/repo/pull/1'") + assert.Contains(t, lines, "export GH_TOKEN='tok123'") +} +``` + +- [ ] **Step 2: Run test to verify it fails** + +Run: `cd /home/rbean/code/fullsend-0 && go test ./internal/cli/ -run TestBuildSandboxEnvLines -v` +Expected: FAIL — function doesn't exist. + +- [ ] **Step 3: Implement `buildSandboxEnvLines`** + +In `internal/cli/run.go`, add a helper function: + +```go +// 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. +func buildSandboxEnvLines(h *harness.Harness) []string { + if h.Env == nil || len(h.Env.Sandbox) == 0 { + return nil + } + keys := make([]string, 0, len(h.Env.Sandbox)) + for k := range h.Env.Sandbox { + keys = append(keys, k) + } + sort.Strings(keys) + + lines := make([]string, 0, len(keys)) + for _, k := range keys { + v := h.Env.Sandbox[k] + escaped := strings.ReplaceAll(v, "'", "'\\''") + lines = append(lines, fmt.Sprintf("export %s='%s'", k, escaped)) + } + return lines +} +``` + +Add `"sort"` to the import block if not already present. + +- [ ] **Step 4: Run test to verify it passes** + +Run: `cd /home/rbean/code/fullsend-0 && go test ./internal/cli/ -run TestBuildSandboxEnvLines -v` +Expected: PASS + +- [ ] **Step 5: Wire `buildSandboxEnvLines` into `bootstrapEnv`** + +In `internal/cli/run.go`, in the `bootstrapEnv` function, find the line that appends the `.env.d` sourcing loop (around line 1264): + +```go + // Source all env files from .env.d/ (populated by host_files with expand: true). + lines = append(lines, fmt.Sprintf("for f in %s/.env.d/*.env; do [ -f \"$f\" ] && . \"$f\"; done", sandbox.SandboxWorkspace)) +``` + +Add the `env.sandbox` lines **before** the `.env.d` sourcing loop so that manual `.env` files (if still present) override generated values (last-writer-wins per ADR 0055): + +```go + // ADR 0055: export env.sandbox vars. Placed before .env.d sourcing so + // manual .env files (if still present during migration) win on collision. + lines = append(lines, buildSandboxEnvLines(h)...) + + // Source all env files from .env.d/ (populated by host_files with expand: true). + lines = append(lines, fmt.Sprintf("for f in %s/.env.d/*.env; do [ -f \"$f\" ] && . \"$f\"; done", sandbox.SandboxWorkspace)) +``` + +- [ ] **Step 6: Run full test suite** + +Run: `cd /home/rbean/code/fullsend-0 && go test ./internal/cli/ ./internal/harness/ -v` +Expected: All tests pass. + +- [ ] **Step 7: Commit** + +```bash +git add internal/cli/run.go internal/cli/run_test.go +git commit -S -s -m "$(cat <<'EOF' +feat(runner): generate sandbox env from env.sandbox + +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 +EOF +)" +``` + +--- + +### Task 7: Integration test — full harness load with env: + +**Files:** +- Test: `internal/harness/integration_test.go` + +- [ ] **Step 1: Write integration test** + +In `internal/harness/integration_test.go`, add a test that exercises the full load pipeline with `env:`, `forge:`, and `base:`: + +```go +func TestLoadWithBase_EnvMergesThroughFullPipeline(t *testing.T) { + dir := t.TempDir() + + baseYAML := ` +agent: agents/test.md +role: test +env: + runner: + BASE_R: base_r + SHARED: from_base + sandbox: + BASE_S: base_s +forge: + github: + env: + runner: + GH_R: gh_r + sandbox: + GH_S: gh_s +` + childYAML := ` +base: base.yaml +env: + runner: + SHARED: from_child + CHILD_R: child_r + sandbox: + CHILD_S: child_s +` + + require.NoError(t, os.WriteFile(filepath.Join(dir, "base.yaml"), []byte(baseYAML), 0o644)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "child.yaml"), []byte(childYAML), 0o644)) + // Create the referenced agent file + require.NoError(t, os.MkdirAll(filepath.Join(dir, "agents"), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "agents/test.md"), []byte("# test"), 0o644)) + + ctx := context.Background() + h, _, err := LoadWithBase(ctx, filepath.Join(dir, "child.yaml"), ComposeOpts{ + WorkspaceRoot: dir, + ForgePlatform: "github", + }) + require.NoError(t, err) + + require.NotNil(t, h.Env) + + // Base composition: child wins on SHARED + assert.Equal(t, "from_child", h.Env.Runner["SHARED"]) + // Base composition: BASE_R inherited + assert.Equal(t, "base_r", h.Env.Runner["BASE_R"]) + // Child's own + assert.Equal(t, "child_r", h.Env.Runner["CHILD_R"]) + // Forge resolution: GH_R merged in + assert.Equal(t, "gh_r", h.Env.Runner["GH_R"]) + + // Sandbox side + assert.Equal(t, "base_s", h.Env.Sandbox["BASE_S"]) + assert.Equal(t, "child_s", h.Env.Sandbox["CHILD_S"]) + assert.Equal(t, "gh_s", h.Env.Sandbox["GH_S"]) +} +``` + +- [ ] **Step 2: Run test** + +Run: `cd /home/rbean/code/fullsend-0 && go test ./internal/harness/ -run TestLoadWithBase_EnvMergesThroughFullPipeline -v` +Expected: PASS (all prior tasks should make this work). + +- [ ] **Step 3: Run full test suite** + +Run: `cd /home/rbean/code/fullsend-0 && go test ./... 2>&1 | tail -30` +Expected: All tests pass. + +- [ ] **Step 4: Commit** + +```bash +git add internal/harness/integration_test.go +git commit -S -s -m "$(cat <<'EOF' +test(harness): integration test for env: through full load pipeline + +Exercises base composition + forge resolution together to verify +env.runner and env.sandbox merge correctly end-to-end. + +Assisted-by: Claude Opus 4.6 +EOF +)" +``` + +--- + +### Task 8: Stage and lint all changes + +- [ ] **Step 1: Run linters** + +```bash +cd /home/rbean/code/fullsend-0 && git add -A && make lint +``` + +Expected: All linters pass. Fix any issues. + +- [ ] **Step 2: Run full test suite one more time** + +```bash +cd /home/rbean/code/fullsend-0 && make go-test +``` + +Expected: All tests pass. + +- [ ] **Step 3: Run go vet** + +```bash +cd /home/rbean/code/fullsend-0 && make go-vet +``` + +Expected: No issues. From c0f825a3a5cd69a3a5f4ac8649ae711c24a81f42 Mon Sep 17 00:00:00 2001 From: Ralph Bean Date: Tue, 23 Jun 2026 14:51:03 -0400 Subject: [PATCH 02/14] feat(harness): add EnvConfig struct with runner/sandbox sub-maps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: Ralph Bean --- internal/harness/forge.go | 1 + internal/harness/forge_test.go | 20 ++++++++++++++++++++ internal/harness/harness.go | 9 +++++++++ internal/harness/harness_test.go | 17 +++++++++++++++++ 4 files changed, 47 insertions(+) diff --git a/internal/harness/forge.go b/internal/harness/forge.go index 0cce45e65..6d50ae167 100644 --- a/internal/harness/forge.go +++ b/internal/harness/forge.go @@ -17,6 +17,7 @@ type ForgeConfig struct { Skills []string `yaml:"skills,omitempty"` ValidationLoop *ValidationLoop `yaml:"validation_loop,omitempty"` RunnerEnv map[string]string `yaml:"runner_env,omitempty"` + Env *EnvConfig `yaml:"env,omitempty"` } var validForgeKeys = map[string]bool{ diff --git a/internal/harness/forge_test.go b/internal/harness/forge_test.go index 4bac21ec9..2f21335d7 100644 --- a/internal/harness/forge_test.go +++ b/internal/harness/forge_test.go @@ -422,6 +422,26 @@ forge: assert.Equal(t, "scripts/pre-gl.sh", h.Forge["gitlab"].PreScript) } +func TestForgeConfig_EnvParsesFromYAML(t *testing.T) { + yaml := ` +agent: agents/test.md +role: test +forge: + github: + env: + runner: + GH_TOKEN: "${GH_TOKEN}" + sandbox: + GITHUB_PR_URL: "${GITHUB_PR_URL}" +` + h, err := parseRaw([]byte(yaml)) + require.NoError(t, err) + require.NotNil(t, h.Forge["github"]) + require.NotNil(t, h.Forge["github"].Env) + assert.Equal(t, map[string]string{"GH_TOKEN": "${GH_TOKEN}"}, h.Forge["github"].Env.Runner) + assert.Equal(t, map[string]string{"GITHUB_PR_URL": "${GITHUB_PR_URL}"}, h.Forge["github"].Env.Sandbox) +} + func TestLoad_WithoutForgeSection(t *testing.T) { content := ` agent: agents/test.md diff --git a/internal/harness/harness.go b/internal/harness/harness.go index 21c99b022..192117fce 100644 --- a/internal/harness/harness.go +++ b/internal/harness/harness.go @@ -192,6 +192,14 @@ type ValidationLoop struct { FeedbackMode string `yaml:"feedback_mode,omitempty"` } +// EnvConfig holds environment variable maps for runner and sandbox targets. +// Replaces runner_env (ADR 0055). Values support ${VAR} expansion from the +// host environment. +type EnvConfig struct { + Runner map[string]string `yaml:"runner,omitempty"` + Sandbox map[string]string `yaml:"sandbox,omitempty"` +} + // Harness is the per-agent configuration that the runner reads to provision // a sandbox and launch one agent. It follows the ADR-0017 schema. type Harness struct { @@ -214,6 +222,7 @@ type Harness struct { AgentInput string `yaml:"agent_input,omitempty"` ValidationLoop *ValidationLoop `yaml:"validation_loop,omitempty"` RunnerEnv map[string]string `yaml:"runner_env,omitempty"` + Env *EnvConfig `yaml:"env,omitempty"` TimeoutMinutes int `yaml:"timeout_minutes,omitempty"` SandboxTimeoutSeconds int `yaml:"sandbox_timeout_seconds,omitempty"` Security *SecurityConfig `yaml:"security,omitempty"` diff --git a/internal/harness/harness_test.go b/internal/harness/harness_test.go index 110e9b692..dd3a3bcef 100644 --- a/internal/harness/harness_test.go +++ b/internal/harness/harness_test.go @@ -1458,3 +1458,20 @@ func TestValidForgePlatform(t *testing.T) { assert.False(t, ValidForgePlatform("bitbucket")) assert.False(t, ValidForgePlatform("")) } + +func TestEnvConfig_ParsesFromYAML(t *testing.T) { + yaml := ` +agent: agents/test.md +role: test +env: + runner: + FOO: bar + sandbox: + BAZ: qux +` + h, err := parseRaw([]byte(yaml)) + require.NoError(t, err) + require.NotNil(t, h.Env) + assert.Equal(t, map[string]string{"FOO": "bar"}, h.Env.Runner) + assert.Equal(t, map[string]string{"BAZ": "qux"}, h.Env.Sandbox) +} From 24f69cf8a350d45ae1f3278f37f33fcfaf75c2c0 Mon Sep 17 00:00:00 2001 From: Ralph Bean Date: Tue, 23 Jun 2026 14:53:27 -0400 Subject: [PATCH 03/14] feat(harness): merge env: in forge resolution Wire EnvConfig into mergeForgeConfig so forge..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 Signed-off-by: Ralph Bean --- internal/harness/forge.go | 23 ++++++++++++ internal/harness/forge_test.go | 67 ++++++++++++++++++++++++++++++++++ 2 files changed, 90 insertions(+) diff --git a/internal/harness/forge.go b/internal/harness/forge.go index 6d50ae167..fafc10662 100644 --- a/internal/harness/forge.go +++ b/internal/harness/forge.go @@ -134,6 +134,29 @@ func mergeForgeConfig(h *Harness, fc *ForgeConfig) { if fc.ValidationLoop != nil { h.ValidationLoop = fc.ValidationLoop } + + // Env: merge sub-maps independently; forge keys win (ADR 0055) + if fc.Env != nil { + if h.Env == nil { + h.Env = &EnvConfig{} + } + if fc.Env.Runner != nil { + if h.Env.Runner == nil { + h.Env.Runner = make(map[string]string, len(fc.Env.Runner)) + } + for k, v := range fc.Env.Runner { + h.Env.Runner[k] = v + } + } + if fc.Env.Sandbox != nil { + if h.Env.Sandbox == nil { + h.Env.Sandbox = make(map[string]string, len(fc.Env.Sandbox)) + } + for k, v := range fc.Env.Sandbox { + h.Env.Sandbox[k] = v + } + } + } } func forgeKeyList(m map[string]*ForgeConfig) string { diff --git a/internal/harness/forge_test.go b/internal/harness/forge_test.go index 2f21335d7..3a815cce3 100644 --- a/internal/harness/forge_test.go +++ b/internal/harness/forge_test.go @@ -386,6 +386,73 @@ func TestValidate_ForgeSkillURLWithHash(t *testing.T) { require.NoError(t, h.Validate()) } +func TestResolveForge_MergesEnv(t *testing.T) { + h := &Harness{ + Agent: "agents/test.md", + Role: "test", + Env: &EnvConfig{ + Runner: map[string]string{"SHARED": "base"}, + Sandbox: map[string]string{"SHARED_SB": "base"}, + }, + Forge: map[string]*ForgeConfig{ + "github": { + Env: &EnvConfig{ + Runner: map[string]string{"GH_TOKEN": "tok"}, + Sandbox: map[string]string{"PR_URL": "url"}, + }, + }, + }, + } + + require.NoError(t, h.ResolveForge("github")) + + require.NotNil(t, h.Env) + assert.Equal(t, "base", h.Env.Runner["SHARED"]) + assert.Equal(t, "tok", h.Env.Runner["GH_TOKEN"]) + assert.Equal(t, "base", h.Env.Sandbox["SHARED_SB"]) + assert.Equal(t, "url", h.Env.Sandbox["PR_URL"]) +} + +func TestResolveForge_EnvForgeOverridesTopLevel(t *testing.T) { + h := &Harness{ + Agent: "agents/test.md", + Role: "test", + Env: &EnvConfig{ + Runner: map[string]string{"KEY": "top"}, + }, + Forge: map[string]*ForgeConfig{ + "github": { + Env: &EnvConfig{ + Runner: map[string]string{"KEY": "forge"}, + }, + }, + }, + } + + require.NoError(t, h.ResolveForge("github")) + assert.Equal(t, "forge", h.Env.Runner["KEY"]) +} + +func TestResolveForge_EnvInheritedWhenForgeNil(t *testing.T) { + h := &Harness{ + Agent: "agents/test.md", + Role: "test", + Env: &EnvConfig{ + Runner: map[string]string{"INHERITED": "yes"}, + Sandbox: map[string]string{"ALSO": "inherited"}, + }, + Forge: map[string]*ForgeConfig{ + "github": {}, + }, + } + + require.NoError(t, h.ResolveForge("github")) + + require.NotNil(t, h.Env) + assert.Equal(t, "yes", h.Env.Runner["INHERITED"]) + assert.Equal(t, "inherited", h.Env.Sandbox["ALSO"]) +} + func TestLoad_WithForgeSection(t *testing.T) { content := ` agent: agents/test.md From c5b4f408ded09926325862a0061730bfc61fd8b4 Mon Sep 17 00:00:00 2001 From: Ralph Bean Date: Tue, 23 Jun 2026 14:55:39 -0400 Subject: [PATCH 04/14] feat(harness): merge env: in base composition 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 Signed-off-by: Ralph Bean --- internal/harness/compose.go | 56 ++++++++++++++++++++++++++++++++ internal/harness/compose_test.go | 53 ++++++++++++++++++++++++++++++ 2 files changed, 109 insertions(+) diff --git a/internal/harness/compose.go b/internal/harness/compose.go index c56270a39..2b8d2255b 100644 --- a/internal/harness/compose.go +++ b/internal/harness/compose.go @@ -459,6 +459,34 @@ func mergeBaseIntoChild(base, child *Harness) { child.RunnerEnv = merged } + // Env: merge sub-maps independently, child keys win (ADR 0055) + if base.Env != nil { + if child.Env == nil { + child.Env = base.Env + } else { + if base.Env.Runner != nil { + if child.Env.Runner == nil { + child.Env.Runner = make(map[string]string, len(base.Env.Runner)) + } + for k, v := range base.Env.Runner { + if _, exists := child.Env.Runner[k]; !exists { + child.Env.Runner[k] = v + } + } + } + if base.Env.Sandbox != nil { + if child.Env.Sandbox == nil { + child.Env.Sandbox = make(map[string]string, len(base.Env.Sandbox)) + } + for k, v := range base.Env.Sandbox { + if _, exists := child.Env.Sandbox[k]; !exists { + child.Env.Sandbox[k] = v + } + } + } + } + } + // Pointer structs: child replaces if non-nil if child.ValidationLoop == nil { child.ValidationLoop = base.ValidationLoop @@ -857,6 +885,34 @@ func mergeForgeConfigInto(base, child *ForgeConfig) { } } + // Env: merge sub-maps, child keys win (ADR 0055) + if base.Env != nil { + if child.Env == nil { + child.Env = base.Env + } else { + if base.Env.Runner != nil { + if child.Env.Runner == nil { + child.Env.Runner = make(map[string]string, len(base.Env.Runner)) + } + for k, v := range base.Env.Runner { + if _, exists := child.Env.Runner[k]; !exists { + child.Env.Runner[k] = v + } + } + } + if base.Env.Sandbox != nil { + if child.Env.Sandbox == nil { + child.Env.Sandbox = make(map[string]string, len(base.Env.Sandbox)) + } + for k, v := range base.Env.Sandbox { + if _, exists := child.Env.Sandbox[k]; !exists { + child.Env.Sandbox[k] = v + } + } + } + } + } + // ValidationLoop: child replaces if non-nil if child.ValidationLoop == nil { child.ValidationLoop = base.ValidationLoop diff --git a/internal/harness/compose_test.go b/internal/harness/compose_test.go index 3f6902689..8b58eec15 100644 --- a/internal/harness/compose_test.go +++ b/internal/harness/compose_test.go @@ -1981,3 +1981,56 @@ base: base.yaml assert.Nil(t, h.MaxRuntimeFetches) assert.Empty(t, h.AllowedRemoteResources) } + +func TestMergeBaseIntoChild_Env(t *testing.T) { + base := &Harness{ + Env: &EnvConfig{ + Runner: map[string]string{"BASE_R": "r1"}, + Sandbox: map[string]string{"BASE_S": "s1"}, + }, + } + child := &Harness{ + Env: &EnvConfig{ + Sandbox: map[string]string{"CHILD_S": "s2"}, + }, + } + + mergeBaseIntoChild(base, child) + + require.NotNil(t, child.Env) + assert.Equal(t, "r1", child.Env.Runner["BASE_R"]) + assert.Equal(t, "s1", child.Env.Sandbox["BASE_S"]) + assert.Equal(t, "s2", child.Env.Sandbox["CHILD_S"]) +} + +func TestMergeBaseIntoChild_EnvChildWins(t *testing.T) { + base := &Harness{ + Env: &EnvConfig{ + Runner: map[string]string{"KEY": "base"}, + }, + } + child := &Harness{ + Env: &EnvConfig{ + Runner: map[string]string{"KEY": "child"}, + }, + } + + mergeBaseIntoChild(base, child) + assert.Equal(t, "child", child.Env.Runner["KEY"]) +} + +func TestMergeBaseIntoChild_EnvInheritedWhenChildNil(t *testing.T) { + base := &Harness{ + Env: &EnvConfig{ + Runner: map[string]string{"R": "val"}, + Sandbox: map[string]string{"S": "val"}, + }, + } + child := &Harness{} + + mergeBaseIntoChild(base, child) + + require.NotNil(t, child.Env) + assert.Equal(t, "val", child.Env.Runner["R"]) + assert.Equal(t, "val", child.Env.Sandbox["S"]) +} From a1754f98ab8938ca28861ac9b656d63ac65f8911 Mon Sep 17 00:00:00 2001 From: Ralph Bean Date: Tue, 23 Jun 2026 14:57:19 -0400 Subject: [PATCH 05/14] feat(harness): lint deprecation warnings for runner_env 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 Signed-off-by: Ralph Bean --- internal/harness/lint.go | 16 +++++++++++++- internal/harness/lint_test.go | 41 +++++++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 1 deletion(-) diff --git a/internal/harness/lint.go b/internal/harness/lint.go index 4dcfbfffe..07e31874b 100644 --- a/internal/harness/lint.go +++ b/internal/harness/lint.go @@ -38,5 +38,19 @@ func (d Diagnostic) String() string { // results are meaningless on an invalid harness. // Returns nil when no diagnostics are found. func (h *Harness) Lint() []Diagnostic { - return nil + var diags []Diagnostic + + if len(h.RunnerEnv) > 0 { + msg := "runner_env is deprecated; use env.runner instead (see ADR 0055)" + if h.Env != nil && len(h.Env.Runner) > 0 { + msg = "runner_env is deprecated and env.runner takes precedence; migrate to env.runner (see ADR 0055)" + } + diags = append(diags, Diagnostic{ + Severity: SeverityWarning, + Field: "runner_env", + Message: msg, + }) + } + + return diags } diff --git a/internal/harness/lint_test.go b/internal/harness/lint_test.go index 1a1653d9f..206149cb2 100644 --- a/internal/harness/lint_test.go +++ b/internal/harness/lint_test.go @@ -4,6 +4,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestLint(t *testing.T) { @@ -18,6 +19,46 @@ func TestLint(t *testing.T) { }) } +func TestLint_RunnerEnvDeprecated(t *testing.T) { + h := &Harness{ + Agent: "agents/test.md", + Role: "test", + RunnerEnv: map[string]string{"FOO": "bar"}, + } + + diags := h.Lint() + require.Len(t, diags, 1) + assert.Equal(t, SeverityWarning, diags[0].Severity) + assert.Equal(t, "runner_env", diags[0].Field) + assert.Contains(t, diags[0].Message, "deprecated") + assert.Contains(t, diags[0].Message, "env.runner") +} + +func TestLint_RunnerEnvAndEnvBothPresent(t *testing.T) { + h := &Harness{ + Agent: "agents/test.md", + Role: "test", + RunnerEnv: map[string]string{"FOO": "bar"}, + Env: &EnvConfig{Runner: map[string]string{"BAZ": "qux"}}, + } + + diags := h.Lint() + require.Len(t, diags, 1) + assert.Equal(t, SeverityWarning, diags[0].Severity) + assert.Contains(t, diags[0].Message, "env.runner takes precedence") +} + +func TestLint_NoWarningWithoutRunnerEnv(t *testing.T) { + h := &Harness{ + Agent: "agents/test.md", + Role: "test", + Env: &EnvConfig{Runner: map[string]string{"FOO": "bar"}}, + } + + diags := h.Lint() + assert.Empty(t, diags) +} + func TestDiagnostic_String(t *testing.T) { t.Run("warning", func(t *testing.T) { d := Diagnostic{Severity: SeverityWarning, Field: "role", Message: "msg"} From e88108a4be6bbf2562d42a90626a2aed50650429 Mon Sep 17 00:00:00 2001 From: Ralph Bean Date: Tue, 23 Jun 2026 15:07:10 -0400 Subject: [PATCH 06/14] feat(runner): validate, expand, and apply env.runner with precedence 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 Signed-off-by: Ralph Bean --- internal/cli/run.go | 33 ++++++++++++++++++++ internal/harness/harness.go | 12 ++++++++ internal/harness/harness_test.go | 53 ++++++++++++++++++++++++++++++++ 3 files changed, 98 insertions(+) diff --git a/internal/cli/run.go b/internal/cli/run.go index 4eed8b3a7..8ca964ef7 100644 --- a/internal/cli/run.go +++ b/internal/cli/run.go @@ -346,6 +346,39 @@ func runAgent(ctx context.Context, agentName, fullsendDir, outputBase, targetRep for k, v := range h.RunnerEnv { h.RunnerEnv[k] = os.Expand(v, expander) } + + // Expand ${VAR} references in env.runner and env.sandbox (ADR 0055). + if h.Env != nil { + for k, v := range h.Env.Runner { + h.Env.Runner[k] = os.Expand(v, expander) + } + for k, v := range h.Env.Sandbox { + h.Env.Sandbox[k] = os.Expand(v, expander) + } + } + + // ADR 0055: env.runner takes precedence over runner_env. + // Emit deprecation warning when runner_env is present. + if len(h.RunnerEnv) > 0 { + if h.Env != nil && len(h.Env.Runner) > 0 { + fmt.Fprintln(os.Stderr, "WARNING: runner_env is deprecated and env.runner takes precedence; migrate to env.runner (see ADR 0055)") + } else { + fmt.Fprintln(os.Stderr, "WARNING: runner_env is deprecated; use env.runner instead (see ADR 0055)") + } + } + + // 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 + if err := h.ValidateFilesExist(); err != nil { printer.StepFail("File validation failed") return fmt.Errorf("validating files: %w", err) diff --git a/internal/harness/harness.go b/internal/harness/harness.go index 192117fce..3639d4ef2 100644 --- a/internal/harness/harness.go +++ b/internal/harness/harness.go @@ -524,6 +524,18 @@ func (h *Harness) ValidateRunnerEnvWith(lookup func(string) (string, bool)) erro return err } } + if h.Env != nil { + for k, v := range h.Env.Runner { + if err := checkVarRefs(fmt.Sprintf("env.runner[%s]", k), v); err != nil { + return err + } + } + for k, v := range h.Env.Sandbox { + if err := checkVarRefs(fmt.Sprintf("env.sandbox[%s]", k), v); err != nil { + return err + } + } + } return nil } diff --git a/internal/harness/harness_test.go b/internal/harness/harness_test.go index dd3a3bcef..236db54c5 100644 --- a/internal/harness/harness_test.go +++ b/internal/harness/harness_test.go @@ -445,6 +445,59 @@ func TestValidateRunnerEnv_PartialExpansion(t *testing.T) { assert.Contains(t, err.Error(), "DEFINITELY_NOT_SET_VAR_XYZ") } +func TestValidateRunnerEnvWith_ChecksEnvRunner(t *testing.T) { + h := &Harness{ + Agent: "agents/test.md", + Role: "test", + Env: &EnvConfig{ + Runner: map[string]string{"KEY": "${MISSING_VAR}"}, + }, + } + lookup := func(key string) (string, bool) { return "", false } + err := h.ValidateRunnerEnvWith(lookup) + require.Error(t, err) + assert.Contains(t, err.Error(), "MISSING_VAR") +} + +func TestValidateRunnerEnvWith_ChecksEnvSandbox(t *testing.T) { + h := &Harness{ + Agent: "agents/test.md", + Role: "test", + Env: &EnvConfig{ + Sandbox: map[string]string{"KEY": "${ALSO_MISSING}"}, + }, + } + lookup := func(key string) (string, bool) { return "", false } + err := h.ValidateRunnerEnvWith(lookup) + require.Error(t, err) + assert.Contains(t, err.Error(), "ALSO_MISSING") +} + +func TestValidateRunnerEnvWith_EnvAllSet(t *testing.T) { + h := &Harness{ + Agent: "agents/test.md", + Role: "test", + Env: &EnvConfig{ + Runner: map[string]string{"KEY": "${SET_VAR}"}, + Sandbox: map[string]string{"KEY2": "literal"}, + }, + } + lookup := func(key string) (string, bool) { + if key == "SET_VAR" { + return "val", true + } + return "", false + } + err := h.ValidateRunnerEnvWith(lookup) + require.NoError(t, err) +} + +func TestValidateRunnerEnvWith_NilEnvNoError(t *testing.T) { + h := &Harness{Agent: "agents/test.md", Role: "test"} + err := h.ValidateRunnerEnvWith(func(string) (string, bool) { return "", false }) + require.NoError(t, err) +} + func TestValidate_AgentNameInvalid(t *testing.T) { h := &Harness{Agent: "agents/test';echo hack;echo '.md"} err := h.Validate() From cd291a7b6d9bf4703efda3773766cf7bbfeb48a1 Mon Sep 17 00:00:00 2001 From: Ralph Bean Date: Tue, 23 Jun 2026 15:16:41 -0400 Subject: [PATCH 07/14] feat(runner): generate sandbox env from env.sandbox 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 Signed-off-by: Ralph Bean --- internal/cli/run.go | 26 +++++++++++++++++ internal/cli/run_test.go | 63 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 89 insertions(+) diff --git a/internal/cli/run.go b/internal/cli/run.go index 8ca964ef7..75b602fdc 100644 --- a/internal/cli/run.go +++ b/internal/cli/run.go @@ -1247,6 +1247,28 @@ func setupFetchService(ctx context.Context, forgeClient forge.Client, h *harness return fetchServiceEnv{addr: addr, token: token}, shutdown, nil } +// 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. +func buildSandboxEnvLines(h *harness.Harness) []string { + if h.Env == nil || len(h.Env.Sandbox) == 0 { + return nil + } + keys := make([]string, 0, len(h.Env.Sandbox)) + for k := range h.Env.Sandbox { + keys = append(keys, k) + } + sort.Strings(keys) + + lines := make([]string, 0, len(keys)) + for _, k := range keys { + v := h.Env.Sandbox[k] + escaped := strings.ReplaceAll(v, "'", "'\\''") + lines = append(lines, fmt.Sprintf("export %s='%s'", k, escaped)) + } + return lines +} + func bootstrapEnv(sandboxName, remoteRepositoryDir string, h *harness.Harness, runtimeEnvExports []string, fetchEnv ...fetchServiceEnv) error { remoteEnvFile := sandbox.SandboxWorkspace + "/.env" outputDir := sandbox.SandboxWorkspace + "/output" @@ -1294,6 +1316,10 @@ func bootstrapEnv(sandboxName, remoteRepositoryDir string, h *harness.Harness, r lines = append(lines, fmt.Sprintf("export FULLSEND_FETCH_TOKEN='%s'", escToken)) } + // ADR 0055: export env.sandbox vars. Placed before .env.d sourcing so + // manual .env files (if still present during migration) win on collision. + lines = append(lines, buildSandboxEnvLines(h)...) + // Source all env files from .env.d/ (populated by host_files with expand: true). lines = append(lines, fmt.Sprintf("for f in %s/.env.d/*.env; do [ -f \"$f\" ] && . \"$f\"; done", sandbox.SandboxWorkspace)) diff --git a/internal/cli/run_test.go b/internal/cli/run_test.go index 99ed160b4..1dcc67478 100644 --- a/internal/cli/run_test.go +++ b/internal/cli/run_test.go @@ -1348,6 +1348,69 @@ func TestBootstrapEnv_SkipsFetchVarsWhenEmpty(t *testing.T) { assert.Contains(t, err.Error(), "copying .env file to sandbox") } +func TestBuildSandboxEnvLines_FromEnvSandbox(t *testing.T) { + h := &harness.Harness{ + Agent: "agents/test.md", + Role: "test", + Env: &harness.EnvConfig{ + Sandbox: map[string]string{ + "GITHUB_PR_URL": "https://github.com/org/repo/pull/1", + "GH_TOKEN": "tok123", + }, + }, + } + + lines := buildSandboxEnvLines(h) + assert.Contains(t, lines, "export GH_TOKEN='tok123'") + assert.Contains(t, lines, "export GITHUB_PR_URL='https://github.com/org/repo/pull/1'") +} + +func TestBuildSandboxEnvLines_NilEnv(t *testing.T) { + h := &harness.Harness{Agent: "agents/test.md", Role: "test"} + lines := buildSandboxEnvLines(h) + assert.Nil(t, lines) +} + +func TestBuildSandboxEnvLines_EmptySandbox(t *testing.T) { + h := &harness.Harness{ + Agent: "agents/test.md", + Role: "test", + Env: &harness.EnvConfig{Runner: map[string]string{"FOO": "bar"}}, + } + lines := buildSandboxEnvLines(h) + assert.Nil(t, lines) +} + +func TestBuildSandboxEnvLines_EscapesSingleQuotes(t *testing.T) { + h := &harness.Harness{ + Agent: "agents/test.md", + Role: "test", + Env: &harness.EnvConfig{ + Sandbox: map[string]string{"MSG": "it's a test"}, + }, + } + lines := buildSandboxEnvLines(h) + require.Len(t, lines, 1) + assert.Equal(t, "export MSG='it'\\''s a test'", lines[0]) +} + +func TestBuildSandboxEnvLines_SortedKeys(t *testing.T) { + h := &harness.Harness{ + Agent: "agents/test.md", + Role: "test", + Env: &harness.EnvConfig{ + Sandbox: map[string]string{ + "ZZZ": "last", + "AAA": "first", + }, + }, + } + lines := buildSandboxEnvLines(h) + require.Len(t, lines, 2) + assert.Equal(t, "export AAA='first'", lines[0]) + assert.Equal(t, "export ZZZ='last'", lines[1]) +} + func TestShouldStartFetchService_AllowRuntimeFetch(t *testing.T) { h := &harness.Harness{ Agent: "agents/test.md", From 44f320876f89aea7650a85bbf2b8d025f9befb1e Mon Sep 17 00:00:00 2001 From: Ralph Bean Date: Tue, 23 Jun 2026 15:20:10 -0400 Subject: [PATCH 08/14] fix(runner): validate env var keys in buildSandboxEnvLines 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 Signed-off-by: Ralph Bean --- internal/cli/run.go | 15 +++++++++++++-- internal/cli/run_test.go | 30 ++++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 2 deletions(-) diff --git a/internal/cli/run.go b/internal/cli/run.go index 75b602fdc..0248a2b24 100644 --- a/internal/cli/run.go +++ b/internal/cli/run.go @@ -9,6 +9,7 @@ import ( "os" "os/exec" "path/filepath" + "regexp" "runtime" "sort" "strings" @@ -1247,16 +1248,26 @@ func setupFetchService(ctx context.Context, forgeClient forge.Client, h *harness return fetchServiceEnv{addr: addr, token: token}, shutdown, nil } +// validEnvKeyRe matches POSIX-portable environment variable names. +// Keys that don't match are skipped to prevent shell injection. +var validEnvKeyRe = regexp.MustCompile(`^[A-Za-z_][A-Za-z0-9_]*$`) + // 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. +// with internal single quotes escaped. Keys that are not valid shell identifiers +// are silently skipped. func buildSandboxEnvLines(h *harness.Harness) []string { if h.Env == nil || len(h.Env.Sandbox) == 0 { return nil } keys := make([]string, 0, len(h.Env.Sandbox)) for k := range h.Env.Sandbox { - keys = append(keys, k) + if validEnvKeyRe.MatchString(k) { + keys = append(keys, k) + } + } + if len(keys) == 0 { + return nil } sort.Strings(keys) diff --git a/internal/cli/run_test.go b/internal/cli/run_test.go index 1dcc67478..2a253a311 100644 --- a/internal/cli/run_test.go +++ b/internal/cli/run_test.go @@ -1411,6 +1411,36 @@ func TestBuildSandboxEnvLines_SortedKeys(t *testing.T) { assert.Equal(t, "export ZZZ='last'", lines[1]) } +func TestBuildSandboxEnvLines_SkipsInvalidKeys(t *testing.T) { + h := &harness.Harness{ + Agent: "agents/test.md", + Role: "test", + Env: &harness.EnvConfig{ + Sandbox: map[string]string{ + "VALID_KEY": "ok", + "bad key": "spaces", + "'; rm -rf ": "inject", + }, + }, + } + lines := buildSandboxEnvLines(h) + require.Len(t, lines, 1) + assert.Equal(t, "export VALID_KEY='ok'", lines[0]) +} + +func TestBuildSandboxEnvLines_EmptyValue(t *testing.T) { + h := &harness.Harness{ + Agent: "agents/test.md", + Role: "test", + Env: &harness.EnvConfig{ + Sandbox: map[string]string{"EMPTY": ""}, + }, + } + lines := buildSandboxEnvLines(h) + require.Len(t, lines, 1) + assert.Equal(t, "export EMPTY=''", lines[0]) +} + func TestShouldStartFetchService_AllowRuntimeFetch(t *testing.T) { h := &harness.Harness{ Agent: "agents/test.md", From 9942ac0f087c0a7faa6b37bb1ca8af2abf65aa24 Mon Sep 17 00:00:00 2001 From: Ralph Bean Date: Tue, 23 Jun 2026 15:22:20 -0400 Subject: [PATCH 09/14] test(harness): integration test for env: through full load pipeline Exercises base composition + forge resolution together to verify env.runner and env.sandbox merge correctly end-to-end. Assisted-by: Claude Opus 4.6 Signed-off-by: Ralph Bean --- internal/harness/integration_test.go | 59 ++++++++++++++++++++++++++++ 1 file changed, 59 insertions(+) diff --git a/internal/harness/integration_test.go b/internal/harness/integration_test.go index b4ae9668c..cb8590e1f 100644 --- a/internal/harness/integration_test.go +++ b/internal/harness/integration_test.go @@ -156,6 +156,65 @@ forge: }, h.Skills) } +// TestLoadWithBase_EnvMergesThroughFullPipeline exercises the full load pipeline +// with env:, base:, and forge: together, verifying that base composition (child +// wins) and forge resolution (forge wins) produce the expected merged env maps. +func TestLoadWithBase_EnvMergesThroughFullPipeline(t *testing.T) { + dir := t.TempDir() + + // Create the referenced agent file so Validate doesn't fail. + require.NoError(t, os.MkdirAll(filepath.Join(dir, "agents"), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "agents/test.md"), []byte("# test"), 0o644)) + + writeTestHarness(t, dir, "base.yaml", ` +agent: agents/test.md +role: test +env: + runner: + BASE_R: base_r + SHARED: from_base + sandbox: + BASE_S: base_s +forge: + github: + env: + runner: + GH_R: gh_r + sandbox: + GH_S: gh_s +`) + + path := writeTestHarness(t, dir, "child.yaml", ` +base: base.yaml +env: + runner: + SHARED: from_child + CHILD_R: child_r + sandbox: + CHILD_S: child_s +`) + + h, _, err := LoadWithBase(context.Background(), path, ComposeOpts{ + ForgePlatform: "github", + }) + require.NoError(t, err) + require.NotNil(t, h.Env) + + // Base composition: child wins on SHARED. + assert.Equal(t, "from_child", h.Env.Runner["SHARED"]) + // Base composition: BASE_R inherited from base. + assert.Equal(t, "base_r", h.Env.Runner["BASE_R"]) + // Child's own key. + assert.Equal(t, "child_r", h.Env.Runner["CHILD_R"]) + // Forge resolution: GH_R merged in (forge wins). + assert.Equal(t, "gh_r", h.Env.Runner["GH_R"]) + + // Sandbox side. + assert.Equal(t, "base_s", h.Env.Sandbox["BASE_S"]) + assert.Equal(t, "child_s", h.Env.Sandbox["CHILD_S"]) + assert.Equal(t, "gh_s", h.Env.Sandbox["GH_S"]) +} + // TestLoadWithBase_NoBaseIdenticalToLoadWithOpts verifies that loading the same // harness (no base field) through both LoadWithOpts and LoadWithBase produces // identical results. From 56fc33fc8f8799b8b52e4343131bdf1713c96a81 Mon Sep 17 00:00:00 2001 From: Ralph Bean Date: Tue, 23 Jun 2026 15:57:36 -0400 Subject: [PATCH 10/14] fix(harness): clone EnvConfig in nil-child merge path 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 Signed-off-by: Ralph Bean --- internal/harness/compose.go | 70 ++++++++++++++++++------------------- 1 file changed, 34 insertions(+), 36 deletions(-) diff --git a/internal/harness/compose.go b/internal/harness/compose.go index 2b8d2255b..430bfd622 100644 --- a/internal/harness/compose.go +++ b/internal/harness/compose.go @@ -462,26 +462,25 @@ func mergeBaseIntoChild(base, child *Harness) { // Env: merge sub-maps independently, child keys win (ADR 0055) if base.Env != nil { if child.Env == nil { - child.Env = base.Env - } else { - if base.Env.Runner != nil { - if child.Env.Runner == nil { - child.Env.Runner = make(map[string]string, len(base.Env.Runner)) - } - for k, v := range base.Env.Runner { - if _, exists := child.Env.Runner[k]; !exists { - child.Env.Runner[k] = v - } - } + child.Env = &EnvConfig{} + } + if base.Env.Runner != nil { + if child.Env.Runner == nil { + child.Env.Runner = make(map[string]string, len(base.Env.Runner)) } - if base.Env.Sandbox != nil { - if child.Env.Sandbox == nil { - child.Env.Sandbox = make(map[string]string, len(base.Env.Sandbox)) + for k, v := range base.Env.Runner { + if _, exists := child.Env.Runner[k]; !exists { + child.Env.Runner[k] = v } - for k, v := range base.Env.Sandbox { - if _, exists := child.Env.Sandbox[k]; !exists { - child.Env.Sandbox[k] = v - } + } + } + if base.Env.Sandbox != nil { + if child.Env.Sandbox == nil { + child.Env.Sandbox = make(map[string]string, len(base.Env.Sandbox)) + } + for k, v := range base.Env.Sandbox { + if _, exists := child.Env.Sandbox[k]; !exists { + child.Env.Sandbox[k] = v } } } @@ -888,26 +887,25 @@ func mergeForgeConfigInto(base, child *ForgeConfig) { // Env: merge sub-maps, child keys win (ADR 0055) if base.Env != nil { if child.Env == nil { - child.Env = base.Env - } else { - if base.Env.Runner != nil { - if child.Env.Runner == nil { - child.Env.Runner = make(map[string]string, len(base.Env.Runner)) - } - for k, v := range base.Env.Runner { - if _, exists := child.Env.Runner[k]; !exists { - child.Env.Runner[k] = v - } - } + child.Env = &EnvConfig{} + } + if base.Env.Runner != nil { + if child.Env.Runner == nil { + child.Env.Runner = make(map[string]string, len(base.Env.Runner)) } - if base.Env.Sandbox != nil { - if child.Env.Sandbox == nil { - child.Env.Sandbox = make(map[string]string, len(base.Env.Sandbox)) + for k, v := range base.Env.Runner { + if _, exists := child.Env.Runner[k]; !exists { + child.Env.Runner[k] = v } - for k, v := range base.Env.Sandbox { - if _, exists := child.Env.Sandbox[k]; !exists { - child.Env.Sandbox[k] = v - } + } + } + if base.Env.Sandbox != nil { + if child.Env.Sandbox == nil { + child.Env.Sandbox = make(map[string]string, len(base.Env.Sandbox)) + } + for k, v := range base.Env.Sandbox { + if _, exists := child.Env.Sandbox[k]; !exists { + child.Env.Sandbox[k] = v } } } From 343194e3ab1364269573af4f2b710909c118a792 Mon Sep 17 00:00:00 2001 From: Ralph Bean Date: Tue, 23 Jun 2026 15:59:01 -0400 Subject: [PATCH 11/14] fix(runner): warn on invalid env.sandbox key names 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 Signed-off-by: Ralph Bean --- internal/cli/run.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/internal/cli/run.go b/internal/cli/run.go index 0248a2b24..578842734 100644 --- a/internal/cli/run.go +++ b/internal/cli/run.go @@ -1264,6 +1264,8 @@ func buildSandboxEnvLines(h *harness.Harness) []string { for k := range h.Env.Sandbox { if validEnvKeyRe.MatchString(k) { keys = append(keys, k) + } else { + fmt.Fprintf(os.Stderr, "WARNING: env.sandbox key %q is not a valid POSIX identifier; skipping\n", k) } } if len(keys) == 0 { From a02412032437e8a37fac96af32b90e593ebc1830 Mon Sep 17 00:00:00 2001 From: Ralph Bean Date: Wed, 24 Jun 2026 12:50:38 -0400 Subject: [PATCH 12/14] fix(runner): address PR review feedback on env delivery - Move Lint() call before the effectiveRunnerEnv merge so it sees the original YAML state and only warns when runner_env was actually declared, not when env.runner entries get merged in (false positive). - Remove the inline fmt.Fprintln deprecation warning that duplicated the Lint() diagnostic. Lint() is the single source of deprecation warnings now. - Add a deny-list of reserved infrastructure env var names (PATH, FULLSEND_FETCH_TOKEN, etc.) to buildSandboxEnvLines so env.sandbox cannot shadow runner-generated infrastructure vars. - Clarify ${VAR} expansion syntax in ADR 0055: Go's os.Expand supports $VAR and ${VAR} only, no shell parameter expansion features. Assisted-by: Claude claude-opus-4-6 Signed-off-by: Ralph Bean --- docs/ADRs/0055-unified-env-var-delivery.md | 5 +- internal/cli/run.go | 62 +++++++++++++--------- internal/cli/run_test.go | 18 +++++++ 3 files changed, 58 insertions(+), 27 deletions(-) diff --git a/docs/ADRs/0055-unified-env-var-delivery.md b/docs/ADRs/0055-unified-env-var-delivery.md index ef7d300e3..9d89297fb 100644 --- a/docs/ADRs/0055-unified-env-var-delivery.md +++ b/docs/ADRs/0055-unified-env-var-delivery.md @@ -105,7 +105,10 @@ ADR 0045 for `runner_env`: When `env.sandbox` is present (after all merges), the runner: -1. Expands `${VAR}` references from the host environment. +1. Expands `${VAR}` references from the host environment using Go's + `os.Expand`, which supports `$VAR` and `${VAR}` syntax only — no + 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. diff --git a/internal/cli/run.go b/internal/cli/run.go index 578842734..efe11845b 100644 --- a/internal/cli/run.go +++ b/internal/cli/run.go @@ -358,28 +358,6 @@ func runAgent(ctx context.Context, agentName, fullsendDir, outputBase, targetRep } } - // ADR 0055: env.runner takes precedence over runner_env. - // Emit deprecation warning when runner_env is present. - if len(h.RunnerEnv) > 0 { - if h.Env != nil && len(h.Env.Runner) > 0 { - fmt.Fprintln(os.Stderr, "WARNING: runner_env is deprecated and env.runner takes precedence; migrate to env.runner (see ADR 0055)") - } else { - fmt.Fprintln(os.Stderr, "WARNING: runner_env is deprecated; use env.runner instead (see ADR 0055)") - } - } - - // 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 - if err := h.ValidateFilesExist(); err != nil { printer.StepFail("File validation failed") return fmt.Errorf("validating files: %w", err) @@ -396,11 +374,26 @@ func runAgent(ctx context.Context, agentName, fullsendDir, outputBase, targetRep } printer.StepDone(fmt.Sprintf("Harness loaded (%.1fs)", time.Since(harnessStart).Seconds())) - // Run lint checks and report any diagnostics (non-fatal). + // Run lint checks before merging env.runner into RunnerEnv so that + // Lint() sees the original YAML state and only warns when runner_env + // was actually declared (not when env.runner entries are merged in). for _, diag := range h.Lint() { emitDiagnostic(printer, diag) } + // ADR 0055: build effective runner env — start with runner_env, + // overlay env.runner so the new field takes precedence. + 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 + // Print plan. printer.KeyValue("Agent", h.Agent) if h.Role != "" { @@ -1252,6 +1245,19 @@ func setupFetchService(ctx context.Context, forgeClient forge.Client, h *harness // Keys that don't match are skipped to prevent shell injection. var validEnvKeyRe = regexp.MustCompile(`^[A-Za-z_][A-Za-z0-9_]*$`) +// reservedSandboxKeys are infrastructure env vars that env.sandbox must not +// shadow. These are set by the runner in bootstrapEnv and overriding them +// from harness YAML could break sandbox operation. +var reservedSandboxKeys = map[string]bool{ + "PATH": true, + "FULLSEND_FETCH_URL": true, + "FULLSEND_FETCH_TOKEN": true, + "FULLSEND_OUTPUT_DIR": true, + "FULLSEND_OUTPUT_SCHEMA": true, + "FULLSEND_OUTPUT_FILE": true, + "FULLSEND_TARGET_REPO_DIR": true, +} + // 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 @@ -1262,11 +1268,15 @@ func buildSandboxEnvLines(h *harness.Harness) []string { } keys := make([]string, 0, len(h.Env.Sandbox)) for k := range h.Env.Sandbox { - if validEnvKeyRe.MatchString(k) { - keys = append(keys, k) - } else { + if !validEnvKeyRe.MatchString(k) { fmt.Fprintf(os.Stderr, "WARNING: env.sandbox key %q is not a valid POSIX identifier; skipping\n", k) + continue + } + if reservedSandboxKeys[k] { + fmt.Fprintf(os.Stderr, "WARNING: env.sandbox key %q is reserved for runner infrastructure; skipping\n", k) + continue } + keys = append(keys, k) } if len(keys) == 0 { return nil diff --git a/internal/cli/run_test.go b/internal/cli/run_test.go index 2a253a311..7aa7d1f07 100644 --- a/internal/cli/run_test.go +++ b/internal/cli/run_test.go @@ -1441,6 +1441,24 @@ func TestBuildSandboxEnvLines_EmptyValue(t *testing.T) { assert.Equal(t, "export EMPTY=''", lines[0]) } +func TestBuildSandboxEnvLines_SkipsReservedKeys(t *testing.T) { + h := &harness.Harness{ + Agent: "agents/test.md", + Role: "test", + Env: &harness.EnvConfig{ + Sandbox: map[string]string{ + "CUSTOM_VAR": "allowed", + "PATH": "/evil", + "FULLSEND_FETCH_TOKEN": "stolen", + "FULLSEND_OUTPUT_DIR": "/tmp/bad", + }, + }, + } + lines := buildSandboxEnvLines(h) + require.Len(t, lines, 1) + assert.Equal(t, "export CUSTOM_VAR='allowed'", lines[0]) +} + func TestShouldStartFetchService_AllowRuntimeFetch(t *testing.T) { h := &harness.Harness{ Agent: "agents/test.md", From 0e1aec41b686a8d3cab38dcaa7ac4afe913a85e8 Mon Sep 17 00:00:00 2001 From: Ralph Bean Date: Wed, 24 Jun 2026 13:12:40 -0400 Subject: [PATCH 13/14] fix(runner): apply gofmt alignment to reservedSandboxKeys gofmt requires map literal values to align to the longest key. The FULLSEND_TARGET_REPO_DIR entry widened the column but the other entries were not re-aligned. Assisted-by: Claude claude-opus-4-6 Signed-off-by: Ralph Bean --- internal/cli/run.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/internal/cli/run.go b/internal/cli/run.go index efe11845b..86809815a 100644 --- a/internal/cli/run.go +++ b/internal/cli/run.go @@ -1249,12 +1249,12 @@ var validEnvKeyRe = regexp.MustCompile(`^[A-Za-z_][A-Za-z0-9_]*$`) // shadow. These are set by the runner in bootstrapEnv and overriding them // from harness YAML could break sandbox operation. var reservedSandboxKeys = map[string]bool{ - "PATH": true, - "FULLSEND_FETCH_URL": true, - "FULLSEND_FETCH_TOKEN": true, - "FULLSEND_OUTPUT_DIR": true, - "FULLSEND_OUTPUT_SCHEMA": true, - "FULLSEND_OUTPUT_FILE": true, + "PATH": true, + "FULLSEND_FETCH_URL": true, + "FULLSEND_FETCH_TOKEN": true, + "FULLSEND_OUTPUT_DIR": true, + "FULLSEND_OUTPUT_SCHEMA": true, + "FULLSEND_OUTPUT_FILE": true, "FULLSEND_TARGET_REPO_DIR": true, } From ec312e8019b072f90ab2777cf96f23240150cc86 Mon Sep 17 00:00:00 2001 From: Ralph Bean Date: Fri, 26 Jun 2026 11:52:01 -0400 Subject: [PATCH 14/14] fix(harness): address review feedback on env delivery (ADR 0055) - Reposition host_files env delivery as permanently complementary to env.sandbox (not deprecated). env.sandbox is convenient for simple per-harness vars; host_files provides file-level composability. - env.sandbox takes precedence over host_files .env on key collision, matching the expected override use case. - Expand reservedSandboxKeys deny-list with HOME, SHELL, LD_PRELOAD, LD_LIBRARY_PATH, BASH_ENV, ENV to prevent sandbox execution influence. - Extract EnvConfig.mergeEnvFrom helper to deduplicate the env merge pattern from compose.go (x2) and forge.go (x1). - Add Lint() diagnostic when env.sandbox coexists with host_files entries targeting .env.d/ with expand: true. - Document merge-is-additive limitation and schema evolution risk in ADR Consequences. - Fix ValidateRunnerEnvWith godoc to mention env.runner/env.sandbox. - Add comment noting h.RunnerEnv post-merge mutation in run.go. Assisted-by: Claude claude-opus-4-6 Signed-off-by: Ralph Bean --- docs/ADRs/0055-unified-env-var-delivery.md | 51 +++++++++++++++++----- internal/cli/run.go | 22 +++++++--- internal/harness/compose.go | 42 +----------------- internal/harness/forge.go | 17 +------- internal/harness/harness.go | 42 ++++++++++++++++-- internal/harness/lint.go | 21 ++++++++- internal/harness/lint_test.go | 31 +++++++++++++ 7 files changed, 149 insertions(+), 77 deletions(-) diff --git a/docs/ADRs/0055-unified-env-var-delivery.md b/docs/ADRs/0055-unified-env-var-delivery.md index 9d89297fb..6b6a260c3 100644 --- a/docs/ADRs/0055-unified-env-var-delivery.md +++ b/docs/ADRs/0055-unified-env-var-delivery.md @@ -48,8 +48,14 @@ 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. +`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 @@ -66,8 +72,8 @@ env: - `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`. + 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. @@ -101,6 +107,12 @@ ADR 0045 for `runner_env`: - **`forge.` 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: @@ -117,6 +129,15 @@ When `env.sandbox` is present (after all merges), the runner: 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 @@ -127,10 +148,10 @@ of whether `env:` also exists: "migrate to env.runner." - Same rules apply to `forge..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. +`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 @@ -139,9 +160,9 @@ themselves. Both mechanisms coexist safely during migration. 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. +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 @@ -159,3 +180,11 @@ for harnesses that still reference it. `runner_env` deprecation. - ADR 0049's env var naming convention applies unchanged — the delivery mechanism changes but the `{AGENT}_{SETTING_NAME}` convention does not. +- 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. diff --git a/internal/cli/run.go b/internal/cli/run.go index 86809815a..08a2b050a 100644 --- a/internal/cli/run.go +++ b/internal/cli/run.go @@ -392,6 +392,8 @@ func runAgent(ctx context.Context, agentName, fullsendDir, outputBase, targetRep effectiveRunnerEnv[k] = v } } + // NOTE: after this point h.RunnerEnv contains the merged effective set + // (runner_env + env.runner), not just the declared runner_env entries. h.RunnerEnv = effectiveRunnerEnv // Print plan. @@ -1247,9 +1249,18 @@ var validEnvKeyRe = regexp.MustCompile(`^[A-Za-z_][A-Za-z0-9_]*$`) // reservedSandboxKeys are infrastructure env vars that env.sandbox must not // shadow. These are set by the runner in bootstrapEnv and overriding them -// from harness YAML could break sandbox operation. +// from harness YAML could break sandbox operation, or are security-sensitive +// vars that could influence sandbox execution (e.g. shared library injection, +// auto-sourced shell startup files). +// NOTE: keep in sync with bootstrapEnv exports below for FULLSEND_* keys. var reservedSandboxKeys = map[string]bool{ "PATH": true, + "HOME": true, + "SHELL": true, + "LD_PRELOAD": true, + "LD_LIBRARY_PATH": true, + "BASH_ENV": true, + "ENV": true, "FULLSEND_FETCH_URL": true, "FULLSEND_FETCH_TOKEN": true, "FULLSEND_OUTPUT_DIR": true, @@ -1339,13 +1350,14 @@ func bootstrapEnv(sandboxName, remoteRepositoryDir string, h *harness.Harness, r lines = append(lines, fmt.Sprintf("export FULLSEND_FETCH_TOKEN='%s'", escToken)) } - // ADR 0055: export env.sandbox vars. Placed before .env.d sourcing so - // manual .env files (if still present during migration) win on collision. - lines = append(lines, buildSandboxEnvLines(h)...) - // Source all env files from .env.d/ (populated by host_files with expand: true). lines = append(lines, fmt.Sprintf("for f in %s/.env.d/*.env; do [ -f \"$f\" ] && . \"$f\"; done", sandbox.SandboxWorkspace)) + // ADR 0055: export env.sandbox vars. Placed after .env.d sourcing so + // env.sandbox takes precedence on collision — the common use case is + // overriding a single var from a shared host_files .env file. + lines = append(lines, buildSandboxEnvLines(h)...) + content := strings.Join(lines, "\n") + "\n" tmpFile, err := os.CreateTemp("", "fullsend-env-*.sh") diff --git a/internal/harness/compose.go b/internal/harness/compose.go index 430bfd622..092bf7d96 100644 --- a/internal/harness/compose.go +++ b/internal/harness/compose.go @@ -464,26 +464,7 @@ func mergeBaseIntoChild(base, child *Harness) { if child.Env == nil { child.Env = &EnvConfig{} } - if base.Env.Runner != nil { - if child.Env.Runner == nil { - child.Env.Runner = make(map[string]string, len(base.Env.Runner)) - } - for k, v := range base.Env.Runner { - if _, exists := child.Env.Runner[k]; !exists { - child.Env.Runner[k] = v - } - } - } - if base.Env.Sandbox != nil { - if child.Env.Sandbox == nil { - child.Env.Sandbox = make(map[string]string, len(base.Env.Sandbox)) - } - for k, v := range base.Env.Sandbox { - if _, exists := child.Env.Sandbox[k]; !exists { - child.Env.Sandbox[k] = v - } - } - } + child.Env.mergeEnvFrom(base.Env, false) } // Pointer structs: child replaces if non-nil @@ -889,26 +870,7 @@ func mergeForgeConfigInto(base, child *ForgeConfig) { if child.Env == nil { child.Env = &EnvConfig{} } - if base.Env.Runner != nil { - if child.Env.Runner == nil { - child.Env.Runner = make(map[string]string, len(base.Env.Runner)) - } - for k, v := range base.Env.Runner { - if _, exists := child.Env.Runner[k]; !exists { - child.Env.Runner[k] = v - } - } - } - if base.Env.Sandbox != nil { - if child.Env.Sandbox == nil { - child.Env.Sandbox = make(map[string]string, len(base.Env.Sandbox)) - } - for k, v := range base.Env.Sandbox { - if _, exists := child.Env.Sandbox[k]; !exists { - child.Env.Sandbox[k] = v - } - } - } + child.Env.mergeEnvFrom(base.Env, false) } // ValidationLoop: child replaces if non-nil diff --git a/internal/harness/forge.go b/internal/harness/forge.go index fafc10662..81fda3461 100644 --- a/internal/harness/forge.go +++ b/internal/harness/forge.go @@ -140,22 +140,7 @@ func mergeForgeConfig(h *Harness, fc *ForgeConfig) { if h.Env == nil { h.Env = &EnvConfig{} } - if fc.Env.Runner != nil { - if h.Env.Runner == nil { - h.Env.Runner = make(map[string]string, len(fc.Env.Runner)) - } - for k, v := range fc.Env.Runner { - h.Env.Runner[k] = v - } - } - if fc.Env.Sandbox != nil { - if h.Env.Sandbox == nil { - h.Env.Sandbox = make(map[string]string, len(fc.Env.Sandbox)) - } - for k, v := range fc.Env.Sandbox { - h.Env.Sandbox[k] = v - } - } + h.Env.mergeEnvFrom(fc.Env, true) } } diff --git a/internal/harness/harness.go b/internal/harness/harness.go index 3639d4ef2..f1e27a1ae 100644 --- a/internal/harness/harness.go +++ b/internal/harness/harness.go @@ -200,6 +200,40 @@ type EnvConfig struct { Sandbox map[string]string `yaml:"sandbox,omitempty"` } +// mergeEnvFrom merges src into dst. When srcWins is true, src keys overwrite +// dst keys on collision (used for forge resolution). When srcWins is false, +// dst keys are preserved on collision (used for base composition where child +// keys win). +func (dst *EnvConfig) mergeEnvFrom(src *EnvConfig, srcWins bool) { + if src == nil { + return + } + if src.Runner != nil { + if dst.Runner == nil { + dst.Runner = make(map[string]string, len(src.Runner)) + } + for k, v := range src.Runner { + if srcWins { + dst.Runner[k] = v + } else if _, exists := dst.Runner[k]; !exists { + dst.Runner[k] = v + } + } + } + if src.Sandbox != nil { + if dst.Sandbox == nil { + dst.Sandbox = make(map[string]string, len(src.Sandbox)) + } + for k, v := range src.Sandbox { + if srcWins { + dst.Sandbox[k] = v + } else if _, exists := dst.Sandbox[k]; !exists { + dst.Sandbox[k] = v + } + } + } +} + // Harness is the per-agent configuration that the runner reads to provision // a sandbox and launch one agent. It follows the ADR-0017 schema. type Harness struct { @@ -496,10 +530,10 @@ func (h *Harness) ResolveRelativeTo(baseDir string) error { return nil } -// ValidateRunnerEnvWith checks that all ${VAR} references in RunnerEnv and -// HostFiles.Src are defined in the host environment using the provided lookup -// function. Variables set to an empty string are allowed; only truly unset -// variables produce an error. +// ValidateRunnerEnvWith checks that all ${VAR} references in RunnerEnv, +// Env.Runner, Env.Sandbox, and HostFiles.Src are defined in the host +// environment using the provided lookup function. Variables set to an empty +// string are allowed; only truly unset variables produce an error. func (h *Harness) ValidateRunnerEnvWith(lookup func(string) (string, bool)) error { checkVarRefs := func(source, value string) error { for _, match := range envVarRef.FindAllStringSubmatch(value, -1) { diff --git a/internal/harness/lint.go b/internal/harness/lint.go index 07e31874b..722fe236a 100644 --- a/internal/harness/lint.go +++ b/internal/harness/lint.go @@ -1,6 +1,9 @@ package harness -import "fmt" +import ( + "fmt" + "strings" +) // DiagnosticSeverity indicates whether a diagnostic is a warning or an error. type DiagnosticSeverity int @@ -52,5 +55,21 @@ func (h *Harness) Lint() []Diagnostic { }) } + // Warn when env.sandbox is present alongside host_files entries that + // deliver .env files to .env.d/ with expand: true, since env.sandbox + // takes precedence on key collision (may shadow host_files values). + if h.Env != nil && len(h.Env.Sandbox) > 0 { + for _, hf := range h.HostFiles { + if hf.Expand && strings.Contains(hf.Dest, ".env.d/") { + diags = append(diags, Diagnostic{ + Severity: SeverityWarning, + Field: "env.sandbox", + Message: fmt.Sprintf("env.sandbox coexists with host_files entry %s (dest: %s); env.sandbox values take precedence on key collision", hf.Src, hf.Dest), + }) + break // one warning is enough + } + } + } + return diags } diff --git a/internal/harness/lint_test.go b/internal/harness/lint_test.go index 206149cb2..04252ebb1 100644 --- a/internal/harness/lint_test.go +++ b/internal/harness/lint_test.go @@ -59,6 +59,37 @@ func TestLint_NoWarningWithoutRunnerEnv(t *testing.T) { assert.Empty(t, diags) } +func TestLint_EnvSandboxWithHostFilesEnvOverlap(t *testing.T) { + h := &Harness{ + Agent: "agents/test.md", + Role: "test", + Env: &EnvConfig{Sandbox: map[string]string{"GH_TOKEN": "${GH_TOKEN}"}}, + HostFiles: []HostFile{ + {Src: "${FULLSEND_DIR}/env/review.env", Dest: "/sandbox/workspace/.env.d/review.env", Expand: true}, + }, + } + + diags := h.Lint() + require.Len(t, diags, 1) + assert.Equal(t, SeverityWarning, diags[0].Severity) + assert.Equal(t, "env.sandbox", diags[0].Field) + assert.Contains(t, diags[0].Message, "env.sandbox values take precedence") +} + +func TestLint_EnvSandboxWithHostFilesNoOverlap(t *testing.T) { + h := &Harness{ + Agent: "agents/test.md", + Role: "test", + Env: &EnvConfig{Sandbox: map[string]string{"GH_TOKEN": "${GH_TOKEN}"}}, + HostFiles: []HostFile{ + {Src: "/path/to/ca.crt", Dest: "/sandbox/workspace/certs/ca.crt"}, + }, + } + + diags := h.Lint() + assert.Empty(t, diags) +} + func TestDiagnostic_String(t *testing.T) { t.Run("warning", func(t *testing.T) { d := Diagnostic{Severity: SeverityWarning, Field: "role", Message: "msg"}