refactor(harness): migrate code agent to env.runner/env.sandbox (ADR 0055)

[ralphbean]

Summary

• Replace runner_env with env.runner (top-level and forge.github)
• Move simple passthrough and hardcoded vars from code-agent.env to env.sandbox
• Keep code-agent.env trimmed to just the GIT_SSL_CAINFO shell conditional (can't be expressed in env.sandbox)
• Update scaffold integration tests for new env schema

Phase 2 of ADR 0055 (#2582).

Test plan

• go test ./internal/harness/ passes
• go test ./internal/scaffold/ passes
• Trigger a code agent run and verify env vars reach both runner and sandbox

🤖 Generated with Claude Code

[qodo-code-review]

PR Summary by Qodo

Refactor harness: migrate code agent env to env.runner/env.sandbox (ADR 0055)

✨ Enhancement ⚙️ Configuration changes 🧪 Tests 🕐 20-40 Minutes

AI Description

• Migrate code-agent harness env from legacy runner_env to env.runner/env.sandbox.
• Move passthrough/static sandbox vars from code-agent.env into harness YAML.
• Update scaffold integration tests to accept both old and new env schemas.

Diagram

graph TD
  A["harness/code.yaml"] --> B["Harness loader"] --> C["Forge merge"] --> D["env.runner"] --> E["Runner scripts"]
  C --> F["env.sandbox"] --> G["Sandbox agent"]
  H["code-agent.env"] --> G

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Keep sandbox env in code-agent.env (status quo)

• ➕ Avoids harness schema churn and keeps env behavior in one file
• ➕ Supports shell conditionals without new schema features
• ➖ Less declarative and harder to reason about runner vs sandbox scoping
• ➖ Requires careful handling of expand:true and host PATH/env leakage risks

2. Extend env.sandbox to support simple conditionals

• ➕ Would allow removing the remaining code-agent.env shell logic entirely
• ➕ Keeps all env in one declarative YAML schema
• ➖ Adds a mini-language/syntax to harness config (more implementation and validation burden)
• ➖ Conditionals can complicate reproducibility/debugging across platforms

Recommendation: Proceed with the PR’s approach: use env.runner/env.sandbox for the vast majority of variables, and keep a minimal .env file only where shell logic is strictly required (GIT_SSL_CAINFO). This aligns with ADR 0055 while preserving correct OpenShell TLS behavior without over-complicating the harness schema.

Files changed (3) +53 / -58

Tests (1) +22 / -9

scaffold_integration_test.goAllow scaffold tests to validate runner_env or env.runner schemas +22/-9

Allow scaffold tests to validate runner_env or env.runner schemas

• Updates scaffold integration assertions to accept either legacy RunnerEnv or the new Env.Runner map after forge resolution. Extends the runner env merge test table with a per-template switch so code.yaml is validated against env.runner while other templates remain on runner_env.

internal/harness/scaffold_integration_test.go

Other (2) +31 / -49

code-agent.envTrim code-agent.env to only the GIT_SSL_CAINFO conditional +6/-39

Trim code-agent.env to only the GIT_SSL_CAINFO conditional

• Removes passthrough variables (issue metadata, GH_TOKEN), git identity, and retry/timeout/Go config from the env file. Leaves only the OpenShell-specific GIT_SSL_CAINFO shell conditional and documents that the rest has moved to harness YAML under env.runner/env.sandbox.

internal/scaffold/fullsend-repo/env/code-agent.env

code.yamlMigrate code agent harness from runner_env to env.runner/env.sandbox +25/-10

Migrate code agent harness from runner_env to env.runner/env.sandbox

• Replaces top-level runner_env with env.runner and introduces env.sandbox for static sandbox configuration (MAX_RETRIES, TIMEOUT_SECONDS, GOPATH, GOMODCACHE). Moves GitHub forge-specific runner variables under forge.github.env.runner and adds forge.github.env.sandbox for sandbox-visible issue metadata, GH_TOKEN, and git author/committer identity.

internal/scaffold/fullsend-repo/harness/code.yaml

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 6:03 PM UTC · Completed 6:18 PM UTC
Commit: 5d8d3f9 · View workflow run →

[github-actions]

Site preview

Preview: https://ee04de21-site.fullsend-ai.workers.dev

Commit: f47cb9f3596e0cb267e4b270ad8aba555ac69504

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (1) 📜 Skill insights (0)

Context used

✅ Compliance rules (platform): 58 rules

✅ Skills: writing-user-docs, writing-adrs

1. Outdated scaffold RunnerEnv assertions ✓ Resolved 🐞 Bug ≡ Correctness

Description

internal/scaffold/fullsend-repo/harness/code.yaml now defines runner variables under env.runner, but
internal/scaffold/scaffold_test.go still asserts that LoadWithOpts populates Harness.RunnerEnv (and
that code.yaml keys are present there). Because LoadWithOpts/ResolveForge do not merge env.runner
into RunnerEnv, these assertions will fail for code.yaml and mask the new schema behavior.

Code

internal/scaffold/fullsend-repo/harness/code.yaml[R44-53]

+env:
+  runner:
+    CODE_ALLOWED_TARGET_BRANCHES: "${CODE_ALLOWED_TARGET_BRANCHES}"
+    FULLSEND_OUTPUT_SCHEMA: ${FULLSEND_DIR}/schemas/code-result.schema.json
+    FULLSEND_OUTPUT_FILE: code-result.json
+  sandbox:
+    MAX_RETRIES: "1"
+    TIMEOUT_SECONDS: "2100"
+    GOPATH: "/sandbox/go"
+    GOMODCACHE: "/sandbox/go/pkg/mod"

Relevance

⭐⭐⭐ High

Team updates scaffold tests for schema refactors; env.runner work and test changes accepted in PR
#2582/#2260.

PR-#2582
PR-#2260
PR-#1039

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The harness template changed to env.runner (no runner_env), while scaffold tests still assert
RunnerEnv is populated by LoadWithOpts and contains code.yaml keys. But LoadWithOpts only
unmarshals + resolves forge + validates, and ResolveForge merges runner_env and env into
separate fields, so RunnerEnv remains empty for env.runner-only harnesses like code.yaml.

internal/scaffold/fullsend-repo/harness/code.yaml[42-53]
internal/scaffold/scaffold_test.go[628-636]
internal/scaffold/scaffold_test.go[660-708]
internal/harness/harness.go[293-317]
internal/harness/forge.go[113-145]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`code.yaml` migrated from `runner_env` to `env.runner`, but `internal/scaffold/scaffold_test.go` still asserts against `h.RunnerEnv` for all harnesses (including `code.yaml`). `harness.LoadWithOpts`/`ResolveForge` keep `RunnerEnv` and `Env.Runner` separate, so `RunnerEnv` can be empty even when `env.runner` is populated.

### Issue Context
- `internal/scaffold/fullsend-repo/harness/code.yaml` now uses `env.runner` and `env.sandbox`.
- `internal/scaffold/scaffold_test.go` still expects `RunnerEnv` to be non-empty after `LoadWithOpts(... ForgePlatform: "github")` and checks for `code.yaml` keys inside `h.RunnerEnv`.
- `harness.LoadWithOpts` does not compute an “effective runner env” (that merge is done later in `internal/cli/run.go`), so tests that call `LoadWithOpts` should be schema-aware.

### Fix Focus Areas
- internal/scaffold/scaffold_test.go[628-642]
- internal/scaffold/scaffold_test.go[660-709]

### Suggested change
- Update the scaffold tests to accept either:
 - legacy `h.RunnerEnv`, or
 - new `h.Env.Runner`, or
 - a locally-computed effective map: start with `h.RunnerEnv`, then overlay `h.Env.Runner` (same as `internal/cli/run.go` does).
- For the `TestHarnessForgeRunnerEnvMerge` table, add a `useEnvRunner` flag similar to `internal/harness/scaffold_integration_test.go` and assert keys against the correct map for `code.yaml`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Comment-only hunk in scaffold test 📘 Rule violation ⚙ Maintainability

Description

A diff hunk in internal/harness/scaffold_integration_test.go modifies only comments without any
accompanying code changes in the same hunk. This violates the rule against comment-only edits
outside directly changed code for the issue.

Code

internal/harness/scaffold_integration_test.go[R286-288]

+// produces the expected merged runner_env / env.runner for each scaffold
+// template, with both top-level (platform-neutral) and forge.github
+// (platform-specific) keys present in the final merged state.

Relevance

⭐ Low

Repo merges comment-only hunks (comment-only additions accepted in PR #2446 and #1555).

PR-#2446
PR-#1555

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 1062072 disallows comment-only hunks unless accompanied by related code changes in
the same hunk. The referenced lines show only comment text changed, with no non-comment code
modifications in that hunk.

Rule 1062072: Do not add or modify comments outside code lines changed for the issue
internal/harness/scaffold_integration_test.go[286-288]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A diff hunk changes only comments (no functional code changes in that hunk), which violates the compliance requirement to avoid comment-only edits outside the code lines changed for the issue.

## Issue Context
The PR updates the comment describing `runner_env` vs `env.runner`, but the hunk containing this edit includes only comment line changes.

## Fix Focus Areas
- internal/harness/scaffold_integration_test.go[286-288]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[fullsend-ai-review]

Review

Findings

Medium

• [test-coverage-gap] internal/harness/scaffold_integration_test.go — TestResolveForge_ScaffoldRunnerEnvMerge validates only runner-side env keys after forge resolution. The diff adds eleven sandbox variables (four to top-level env.sandbox: MAX_RETRIES, TIMEOUT_SECONDS, GOPATH, GOMODCACHE; and seven to forge.github.env.sandbox: ISSUE_NUMBER, GITHUB_ISSUE_URL, GH_TOKEN, GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, GIT_COMMITTER_NAME, GIT_COMMITTER_EMAIL), but no test asserts that h.Env.Sandbox contains the expected keys after resolution. A regression in sandbox env merging would go undetected for the code agent.
  Remediation: Add a sandboxKeys field to the code.yaml test case and assert that h.Env.Sandbox contains the expected keys after forge resolution.

• [scope-mismatch] internal/scaffold/fullsend-repo/harness/code.yaml — PR description says "Phase 2 of ADR 0055" but only migrates code.yaml. Three other scaffold harnesses (review.yaml, fix.yaml, retro.yaml) still use the legacy runner_env format. The PR title accurately says "migrate code agent" (singular), but the Phase 2 reference may imply broader completion. Consider clarifying in the description that this is one step in an incremental Phase 2 rollout.

Previous run

Review

Findings

Medium

• [test-coverage-gap] internal/scaffold/scaffold_test.go — TestHarnessForgeRunnerEnvMerge (and its counterpart TestResolveForge_ScaffoldRunnerEnvMerge in scaffold_integration_test.go) validates only runner-side env keys after forge resolution. The diff moves seven variables into forge.github.env.sandbox (ISSUE_NUMBER, GITHUB_ISSUE_URL, GH_TOKEN, GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, GIT_COMMITTER_NAME, GIT_COMMITTER_EMAIL) and adds four to top-level env.sandbox (MAX_RETRIES, TIMEOUT_SECONDS, GOPATH, GOMODCACHE). The mergeForgeConfig function merges both Runner and Sandbox sub-maps, so h.Env.Sandbox should contain all eleven keys after resolution, but no test asserts this. A regression in sandbox env merging would go undetected.
  Remediation: Add a sandboxKeys field to the code.yaml test case and assert that h.Env.Sandbox contains the expected keys after forge resolution.

Low

• [test-integrity] internal/harness/scaffold_integration_test.go:133 — The assertion is loosened from assert.NotEmpty(t, h.RunnerEnv) to an OR condition accepting either h.RunnerEnv or h.Env.Runner. If code.yaml's env.runner keys were accidentally parsed into RunnerEnv (or vice versa), this test would still pass. The separate TestResolveForge_ScaffoldRunnerEnvMerge test provides more targeted coverage via the useEnvRunner branch, mitigating the risk.

• [lint-diagnostic-expected] internal/scaffold/fullsend-repo/harness/code.yaml:47 — code.yaml now declares env.sandbox for simple passthrough vars while also delivering code-agent.env via host_files with expand: true into .env.d/. Both mechanisms inject environment into the sandbox. The trimmed code-agent.env only contains a GIT_SSL_CAINFO shell conditional that cannot be expressed in env.sandbox, so this coexistence is intentional.

---

Labels: Refactoring migration PR consistent with sibling ADR 0055 PRs labeled type/chore.

Previous run (2)

Review

Findings

Medium

• [test-coverage-gap] internal/harness/scaffold_integration_test.go:336 — TestResolveForge_ScaffoldRunnerEnvMerge validates env.runner keys for code.yaml but does not verify env.sandbox keys. The diff moves ISSUE_NUMBER, GITHUB_ISSUE_URL, GH_TOKEN, GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, GIT_COMMITTER_NAME, GIT_COMMITTER_EMAIL into forge.github.env.sandbox and adds MAX_RETRIES, TIMEOUT_SECONDS, GOPATH, GOMODCACHE to top-level env.sandbox. The mergeForgeConfig function (forge.go:138-144) merges both Runner and Sandbox sub-maps, so h.Env.Sandbox should contain all these keys after resolution, but no test asserts this.
  Remediation: Add a sandboxKeys field to the code.yaml test case and assert that h.Env.Sandbox contains those keys after forge resolution.

Low

• [test-integrity] internal/harness/scaffold_integration_test.go:133 — The assertion was generalized from assert.NotEmpty(t, h.RunnerEnv) to an OR condition accepting either h.RunnerEnv or h.Env.Runner. This is a correct adaptation for the transition period. Consider adding a separate assertion for env.sandbox content on the code.yaml case in a follow-up.

• [lint-diagnostic-expected] internal/scaffold/fullsend-repo/harness/code.yaml:47 — code.yaml now has env.sandbox alongside a host_files entry delivering code-agent.env to .env.d/. The lint function will emit a SeverityWarning about this coexistence. No existing test runs Lint() on scaffold harnesses, so no test breakage, but the warning is expected during the transition.

• [naming-consistency] internal/harness/scaffold_integration_test.go:296 — Field name useEnvRunner could be read as "use the env.runner field" but means "this harness uses the new Env structure instead of legacy RunnerEnv map." Minor ambiguity.

• [test-structure] internal/harness/scaffold_integration_test.go:339 — The conditional assertion block introduces two parallel code paths for Env.Runner vs RunnerEnv. This asymmetry is inherent to incremental migration and will resolve when all harnesses use env.runner.

• [inline-documentation] internal/scaffold/fullsend-repo/env/code-agent.env:1 — The reduced file loses some documentation context from the original. However, the new header clearly explains where variables moved (ADR 0055) and why the GIT_SSL_CAINFO conditional remains. The removed comments documented variables that no longer live in this file.

---

Labels: PR migrates code agent harness config and sandbox env var delivery per ADR 0055.

[fullsend-ai-review]

🤖 Review · ❌ Terminated · Started 6:53 PM UTC · Ended 7:05 PM UTC
Commit: 657bf48 · View workflow run →

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 6:53 PM UTC · Completed 7:05 PM UTC
Commit: b876a6a · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 7:40 PM UTC · Completed 7:53 PM UTC
Commit: f47cb9f · View workflow run →

[fullsend-ai-review]

See the review comment for full details.

[fullsend-ai-review]

[medium] test-coverage-gap

TestResolveForge_ScaffoldRunnerEnvMerge validates only runner-side env keys after forge resolution. The diff adds eleven sandbox variables but no test asserts that h.Env.Sandbox contains the expected keys after resolution. A regression in sandbox env merging would go undetected for the code agent.

Suggested fix: Add a sandboxKeys field to the code.yaml test case and assert that h.Env.Sandbox contains the expected keys after forge resolution.

[rh-hemartin]

I don't think this is correct. If this gets expanded within the sandbox, there is no GH_TOKEN within the sandbox right? That is why GH_TOKEN was on a file expanded at runner level. Now I'm not sure if other similar PRs to this one are OK. Same for GIT_BOT_EMAIL. I don't think you are testing this locally and you should.