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

[ralphbean]

Migrate fix agent runner_env to env.runner/env.sandbox per ADR 0055. Move passthrough vars and hardcoded config from fix-agent.env into env.sandbox. Trim fix-agent.env to shell conditional only. Update scaffold integration tests.

[qodo-code-review]

PR Summary by Qodo

Migrate fix-agent harness config from runner_env to env.runner/env.sandbox (ADR 0055)

⚙️ Configuration changes 🧪 Tests 🕐 20-40 Minutes

AI Description

• Migrate fix-agent harness YAML from legacy runner_env to env.runner + env.sandbox per ADR 0055.
• Move hardcoded/passthrough sandbox variables from fix-agent.env into harness env.sandbox.
• Update scaffold integration tests to validate merged legacy RunnerEnv and new Env.Runner maps.

Diagram

graph TD
  A["harness/fix.yaml"] --> B["Harness loader"] --> C["env.runner"]
  B --> D["env.sandbox"]
  E["env/fix-agent.env"] --> D
  F["scaffold_integration_test.go"] --> B

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Add a temporary loader-level compatibility shim only

• ➕ Keeps harness YAML simpler during rollout by allowing runner_env as the single source of truth
• ➕ Avoids needing tests to reason about two env maps
• ➖ Conflicts with ADR intent to standardize on env.runner/env.sandbox
• ➖ Extends lifetime of legacy config and can hide missing migrations until later

2. Introduce YAML anchors/shared blocks for repeated sandbox vars

• ➕ Reduces duplication between top-level env.sandbox and forge.github env.sandbox
• ➕ Easier to keep sandbox defaults consistent across forges
• ➖ YAML anchors can reduce readability and are sometimes discouraged in config repos
• ➖ May not be supported/desired by existing harness tooling or style guidance

Recommendation: Proceed with the PR’s approach: moving fix-agent configuration into env.runner/env.sandbox aligns with ADR 0055 and clarifies runner-vs-sandbox variable boundaries (notably keeping write tokens runner-only). Keep the current test strategy (combining legacy RunnerEnv + Env.Runner) as a transitional safeguard, and plan a follow-up to remove legacy RunnerEnv expectations once all harnesses are migrated.

Files changed (3) +54 / -72

Tests (1) +18 / -6

scaffold_integration_test.goAccept both legacy RunnerEnv and new Env.Runner in scaffold merge tests +18/-6

Accept both legacy RunnerEnv and new Env.Runner in scaffold merge tests

• Updates scaffold integration tests to treat environment as satisfied if either legacy RunnerEnv or new Env.Runner is populated. For forge resolution tests, builds a combined map from RunnerEnv and Env.Runner before asserting required keys, making the tests compatible during the migration window.

internal/harness/scaffold_integration_test.go

Other (2) +36 / -66

fix-agent.envTrim fix-agent.env to only conditional TLS CA export +0/-51

Trim fix-agent.env to only conditional TLS CA export

• Removes passthrough and hardcoded exports (tokens, retry/timeout, git identity, Go paths) from the host .env file. Retains only the shell conditional that sets GIT_SSL_CAINFO when the OpenShell TLS CA bundle is present.

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

fix.yamlMigrate fix-agent config to env.runner and env.sandbox (including forge overrides) +36/-15

Migrate fix-agent config to env.runner and env.sandbox (including forge overrides)

• Replaces top-level runner_env with env.runner and adds env.sandbox for previously hardcoded sandbox configuration (retry/timeout/iteration caps, Go paths). Under forge.github, migrates runner_env to env.runner and introduces env.sandbox entries for sandbox-visible variables (including read-only GH_TOKEN and git author/committer identity).

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

[github-actions]

Site preview

Preview: https://4d74630d-site.fullsend-ai.workers.dev

Commit: e6ffdea962eb65c3a3705609de43928a4ada9f60

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 6:08 PM UTC · Completed 6:33 PM UTC
Commit: abe98b7 · View workflow run →

[qodo-code-review]

Code Review by Qodo

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

Context used

✅ Compliance rules (platform): 58 rules

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

1. Comment-only edit in scaffold test 📘 Rule violation ⚙ Maintainability

Description

A comment block was modified without any accompanying non-comment code changes in the same diff
hunk. This violates the requirement to avoid comment-only changes outside code lines changed for the
issue.

Code

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

+// produces the expected merged env.runner (or legacy runner_env) 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 updates (e.g., .gitlint comment fix) without strict “no comment-only hunk”
enforcement.

PR-#2126

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 1062072 forbids comment-only hunks; the cited lines are modified comments
describing merged env.runner/legacy runner_env with no corresponding code changes 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
The PR modifies only comments in a diff hunk (no adjacent code changes). Per compliance, comment changes must be coupled with related code changes in the same hunk, or avoided.

## Issue Context
The comment describing `TestResolveForge_ScaffoldRunnerEnvMerge` was updated to mention `env.runner`/legacy `runner_env`, but the change is comment-only in that hunk.

## 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

---

2. Redundant env file expansion 🐞 Bug ⚙ Maintainability

Description

harness/fix.yaml still copies env/fix-agent.env with expand: true even though fix-agent.env no
longer contains any ${VAR} references, so host-side expansion is now unnecessary work. Keeping
expand enabled also preserves the previously-hit PATH-clobbering footgun if ${PATH} (or similar) is
reintroduced later in this file.

Code

internal/scaffold/fullsend-repo/env/fix-agent.env[R1-3]

# SSL certs — same workaround as code agent for OpenShell TLS termination.
if [ -f /etc/openshell-tls/ca-bundle.pem ]; then
  export GIT_SSL_CAINFO=/etc/openshell-tls/ca-bundle.pem

Relevance

⭐ Low

Prior reviews kept expand:true and fixed footguns by removing PATH/etc, rejecting “disable
expand:true” changes.

PR-#337
PR-#382

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The scaffold still marks fix-agent.env for host-side expansion, but the file was reduced to static
shell logic without any ${VAR} placeholders, so expansion no longer provides value. Past incidents
show this exact pattern can be hazardous if ${PATH} is reintroduced under expand: true, because
runner-side expansion can clobber sandbox PATH.

internal/scaffold/fullsend-repo/harness/fix.yaml[27-33]
internal/scaffold/fullsend-repo/env/fix-agent.env[1-6]
internal/harness/harness.go[27-35]
PR-#337

Agent prompt

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

### Issue description
`internal/scaffold/fullsend-repo/env/fix-agent.env` was trimmed down and no longer contains `${VAR}` placeholders, but `internal/scaffold/fullsend-repo/harness/fix.yaml` still ships it via `host_files` with `expand: true`. This makes the runner do unnecessary expand/rewrite work and keeps the same class of footgun that previously broke sandbox PATH when `${PATH}` was expanded from the runner.

### Issue Context
`HostFile.Expand` is intended only for env files that must resolve `${VAR}` on the host because the sandbox won’t have those vars.

### Fix Focus Areas
- internal/scaffold/fullsend-repo/harness/fix.yaml[27-33]

### Proposed fix
Change the `host_files` entry for `env/fix-agent.env` to `expand: false` (or remove the `expand` field entirely). Keep `expand: true` only on env files that actually contain `${VAR}` placeholders that must be resolved on the runner.

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

---

[fullsend-ai-review]

Looks good to me.

Previous run

Looks good to me.

Low

• [scope-creep] internal/scaffold/fullsend-repo/harness/fix.yaml:56 — The PR moves hardcoded config constants (MAX_RETRIES, TIMEOUT_SECONDS, STRATEGY_ESCALATION_THRESHOLD, ITERATION_CAP, ITERATION_CAP_HUMAN, GOPATH, GOMODCACHE) from fix-agent.env into env.sandbox. ADR 0055 Phase 2 authorizes moving "simple passthrough vars" but these are hardcoded constants, not passthroughs from the host environment. ADR 0055's own example includes a hardcoded constant in env.sandbox (REVIEW_FINDING_SEVERITY_THRESHOLD), so this appears to be an intended use case.

• [test coverage gap] internal/scaffold/scaffold_test.go:682 — TestHarnessForgeRunnerEnvMerge for fix.yaml only asserts that env.runner keys (topLevelKeys) and forge.github.env.runner keys (forgeGithubKeys) are present in the combined map. The new env.sandbox keys (MAX_RETRIES, TIMEOUT_SECONDS, etc.) and forge.github.env.sandbox keys (PR_NUMBER, REPO_FULL_NAME, GH_TOKEN, GIT_AUTHOR_NAME, etc.) introduced in this PR have no scaffold-level integration test coverage.

• [architectural-inconsistency] internal/scaffold/fullsend-repo/harness/fix.yaml:45 — Only fix.yaml is migrated while 5 other scaffold harnesses still use runner_env, creating temporary inconsistency during Phase 2 rollout.

• [incomplete-migration] internal/harness/scaffold_integration_test.go:133 — The test update uses a dual-check accepting either RunnerEnv or Env.Runner. This is correct transitional logic, but adding a TODO comment to remove the fallback once all harnesses complete Phase 2 would aid cleanup tracking.

---

Labels: PR specifically migrates fix agent env configuration per ADR 0055.

Previous run (2)

Review

Findings

Medium

• [scope-creep] internal/scaffold/fullsend-repo/harness/fix.yaml:56 — The PR moves hardcoded config constants (MAX_RETRIES, TIMEOUT_SECONDS, STRATEGY_ESCALATION_THRESHOLD, ITERATION_CAP, ITERATION_CAP_HUMAN, GOPATH, GOMODCACHE) from fix-agent.env into env.sandbox. ADR 0055 Phase 2 authorizes moving "simple passthrough vars" but these are hardcoded constants, not passthroughs from the host environment. The PR body acknowledges this scope ("Move passthrough vars and hardcoded config from fix-agent.env into env.sandbox"), but the distinction matters for understanding the change's intent.

• [stale-identifier-reference] docs/guides/user/building-custom-agents.md:146 — User guide contains example harness YAML using deprecated runner_env: key. Line 146 shows runner_env: in a harness example, and line 465 instructs users that "All runner_env variables must appear in the workflow env block." While runner_env still works during Phase 2, user-facing documentation should lead with the canonical pattern.

• [stale-identifier-reference] docs/guides/user/customizing-agents.md:43 — User guide contains example harness YAML using deprecated runner_env: key at lines 43-46 and 361-364. Same concern as above — user guides should demonstrate the current recommended pattern.

Low

• [missing-authorization] N/A — This PR implements a partial migration (fix agent only) of ADR 0055 Phase 2 without a linked issue. The ADR itself provides authorization for the migration, and phased rollouts are standard practice. A linked issue would improve traceability.

• [incomplete-migration] internal/harness/scaffold_integration_test.go:133 — The test update uses a dual-check accepting either RunnerEnv or Env.Runner. This is correct transitional logic, but adding a TODO comment to remove the fallback once all harnesses complete Phase 2 would aid cleanup tracking.

• [test coverage gap] internal/harness/scaffold_integration_test.go:334 — TestResolveForge_ScaffoldRunnerEnvMerge builds a combined map from RunnerEnv and Env.Runner but does not verify that new env.sandbox keys (MAX_RETRIES, TIMEOUT_SECONDS, GH_TOKEN, GIT_AUTHOR_NAME) are present after forge resolution. The sandbox env merge path for fix.yaml has no integration test coverage.

• [test-assertion-style] internal/harness/scaffold_integration_test.go:133 — Test introduces a two-line pattern (intermediate variable + assert.True) that differs from the surrounding direct assertion style (assert.NotEmpty, assert.Nil). Minor deviation, functionally motivated by the OR condition.

• [comment-terminology-consistency] internal/harness/scaffold_integration_test.go:284 — Test comment updated to reference "env.runner (or legacy runner_env)" — reasonable transitional language during Phase 2.

• [test-assertion-message-clarity] internal/harness/scaffold_integration_test.go:349 — Assertion messages changed from "merged RunnerEnv should contain" to "merged env should contain", dropping the struct field name reference used elsewhere.

• [stale-identifier-reference] docs/ADRs/0024-harness-definitions.md:75 — References runner_env throughout; ADR 0055 amends this ADR but doesn't update all field descriptions. Amendment chain is clear to readers.

• [stale-identifier-reference] docs/ADRs/0045-forge-portable-harness-schema.md:165 — Extensive runner_env references in merge semantics documentation. ADR 0055 references this ADR in its text.

• [stale-identifier-reference] docs/ADRs/0049-agent-configuration-env-var-convention.md:34 — Uses runner_env in examples; ADR 0055 explicitly amends this ADR.

• [stale-identifier-reference] docs/ADRs/0053-agent-driven-branch-targeting.md:130 — Instructs users to add variables to runner_env; predates ADR 0055.

• [stale-identifier-reference] docs/ADRs/0030-openshell-sandbox-interaction-model.md:117 — Mentions runner_env in passing when describing host-side variable availability.

• [stale-identifier-reference] docs/architecture.md:92 — Architecture overview mentions runner_env; already describes the transition to env: sub-maps.

• [architectural-inconsistency] internal/scaffold/fullsend-repo/harness/fix.yaml:45 — Only fix.yaml is migrated while 5 other scaffold harnesses still use runner_env, creating temporary inconsistency during Phase 2 rollout.

---

Labels: PR modifies harness config (fix.yaml) and restructures sandbox env var delivery per ADR 0055.

[fullsend-ai-review]

🤖 Review · ❌ Terminated · Started 6:53 PM UTC · Ended 7:06 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 · ❌ Failure · Started 6:53 PM UTC · Completed 7:06 PM UTC
Commit: 98892e2 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 7:41 PM UTC · Completed 7:48 PM UTC
Commit: e6ffdea · View workflow run →

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 8:01 PM UTC · Completed 8:10 PM UTC
Commit: e6ffdea · View workflow run →

[fullsend-ai-retro]

Retro: PR #2762 — Migrate fix-agent harness config to env.runner/env.sandbox

This was a clean, human-authored PR by ralphbean that migrated the fix agent's harness configuration from legacy runner_env to the unified env schema per ADR 0055. The workflow completed successfully with minimal friction.

Timeline

1. 2026-06-29 18:05 — Initial commit pushed
2. 18:08–18:33 — Review bot runs, approves with "Looks good to me" + one medium-severity inline comment (scope-creep) + low-severity findings in summary
3. 18:53–19:06 — After force pushes, review terminates then reports failure on same run (known bug #2572)
4. 2026-06-30 19:37 — Final force push to squashed commit e6ffdea
5. 19:41 — Fresh review run, success
6. 19:55 — Merged via merge queue

Key observations

• Multiple review runs from force pushes — 5 review status comments across the PR lifecycle. This pattern is well-covered by existing issues: #1418, #1422, #1372, #963, #2599.
• Terminated → Failure on same run — Already tracked in #2572.
• Inline severity vs summary severity mismatch — The scope-creep finding was posted as [medium] inline but listed under "Low" in the summary. Partially covered by #2054.
• Scope-creep finding was a false positive — See proposal below.

Overall this workflow went well. The review agent approved correctly, the human merged without issues, and token cost was reasonable for a 3-file change. One proposal filed for a review calibration improvement.

Proposals filed

• Review agent should not flag scope-creep against ADR phase boundaries for human-authored PRs (in fullsend-ai/fullsend)