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

[ralphbean]

Migrate retro harness from deprecated runner_env to env.runner/env.sandbox per ADR 0055. Move env/retro.env passthrough variables into the harness YAML directly. Delete retro.env and its host_files entry. Update scaffold_integration_test.go to check Env.Runner for retro template.

[qodo-code-review]

PR Summary by Qodo

Refactor retro harness to env.runner/env.sandbox (ADR 0055)

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

AI Description

• Migrate retro harness template from runner_env to env.runner/env.sandbox (ADR 0055).
• Inline retro.env passthrough variables into retro.yaml; remove retro.env host_files usage.
• Update scaffold integration tests to validate Env.Runner merge behavior for retro.

Diagram

graph TD
  A["retro.yaml"] --> B(["Harness loader"]) --> C(["Forge resolver"])
  D{{"GitHub forge config"}} --> C
  C --> E["env.runner vars"] & F["env.sandbox vars"]
  subgraph Legend
    direction LR
    _file["YAML template"] ~~~ _comp(["Component"]) ~~~ _ext{{"External"}}
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Dual-write runner_env + env.runner during transition

• ➕ Eases rollout if downstream consumers still expect RunnerEnv only
• ➕ Reduces risk of missing variables in older execution paths
• ➖ Prolongs the deprecated schema and increases config drift
• ➖ More keys to keep consistent across templates and forge blocks

2. Keep retro.env as a host_file and source it in scripts

• ➕ Minimizes YAML duplication; keeps env wiring in one file
• ➕ Simpler diffs for future env var tweaks
• ➖ Continues reliance on host_files passthrough for plain env vars
• ➖ Fights the direction of ADR 0055 and complicates auditing where vars come from

3. Add a loader-level auto-migration (runner_env -> env.runner) and defer template edits

• ➕ Allows incremental template migration with fewer immediate config changes
• ➕ Centralizes compatibility logic in one place
• ➖ Adds hidden behavior that can mask misconfigurations
• ➖ Still requires eventual template cleanups to remove deprecated schema

Recommendation: Proceed with the PR’s approach: updating the retro template to explicitly use env.runner/env.sandbox is the most direct ADR 0055 compliance path and makes env scoping (runner vs sandbox) visible in config. Consider a temporary dual-write only if there is confirmed downstream breakage requiring RunnerEnv specifically.

Files changed (2) +26 / -14

Tests (1) +12 / -5

scaffold_integration_test.goAdjust scaffold tests for Env.Runner migration +12/-5

Adjust scaffold tests for Env.Runner migration

• Updates scaffold integration assertions to accept either deprecated RunnerEnv or the new Env.Runner map after forge/base merges. Extends the RunnerEnv merge test matrix to verify retro.yaml now populates Env.Runner keys (ADR 0055) and no longer expects retro keys in deprecated RunnerEnv.

internal/harness/scaffold_integration_test.go

Other (1) +14 / -9

retro.yamlMigrate retro harness env to env.runner/env.sandbox +14/-9

Migrate retro harness env to env.runner/env.sandbox

• Replaces top-level runner_env with env.runner and adds env.sandbox for RETRO_COMMENT. Moves previously host_file-sourced variables (ORIGINATING_URL, REPO_FULL_NAME, GH_TOKEN) into forge.github.env (runner + sandbox), and removes the retro.env host_files entry.

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

[github-actions]

Site preview

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

Commit: f1d276ab4fa6ef29ec6d11a4856cb9ef9d5cddf6

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 6:05 PM UTC · Completed 6:29 PM UTC
Commit: 9511de4 · View workflow run →

[qodo-code-review]

Code Review by Qodo

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

Context used

✅ Compliance rules (platform): 58 rules

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

1. RETRO_COMMENT now required 🐞 Bug ☼ Reliability

Description

retro.yaml sets env.sandbox.RETRO_COMMENT to ${RETRO_COMMENT}, and fullsend run validates
all ${VAR} references up front; if RETRO_COMMENT is unset, the run fails before the sandbox
starts. This contradicts the retro scripts/docs that treat RETRO_COMMENT as optional (empty/absent
for automatic triggers).

Code

internal/scaffold/fullsend-repo/harness/retro.yaml[R34-38]

+env:
+  runner:
+    FULLSEND_OUTPUT_SCHEMA: ${FULLSEND_DIR}/schemas/retro-result.schema.json
+  sandbox:
+    RETRO_COMMENT: "${RETRO_COMMENT}"

Relevance

⭐ Low

Similar “optional ${VAR} causes ValidateRunnerEnvWith failure” fix was rejected in PR #2346; pattern
not enforced.

PR-#2346

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The retro harness now requires ${RETRO_COMMENT} during environment validation, but the validation
code errors on any unset variable reference while the retro scripts/docs explicitly treat
RETRO_COMMENT as optional and handle the unset case.

internal/scaffold/fullsend-repo/harness/retro.yaml[34-38]
internal/harness/harness.go[533-572]
internal/cli/run.go[361-392]
internal/scaffold/fullsend-repo/scripts/pre-retro.sh[7-29]
internal/scaffold/fullsend-repo/agents/retro.md[20-25]

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/harness/retro.yaml` references `${RETRO_COMMENT}` in `env.sandbox`. The runner calls `ValidateRunnerEnvWith` before expansion/execution, so an unset `RETRO_COMMENT` causes a hard failure, even though the retro flow treats it as optional.

### Issue Context
- `ValidateRunnerEnvWith` rejects any `${VAR}` that is not set in the host environment.
- `pre-retro.sh` and `agents/retro.md` both document `RETRO_COMMENT` as optional and handle it via `${RETRO_COMMENT:-}` semantics.

### Fix Focus Areas
- internal/scaffold/fullsend-repo/harness/retro.yaml[34-38]

### Suggested fix directions (pick one)
1) **Make the harness stop referencing RETRO_COMMENT unless in GitHub forge context**: move `RETRO_COMMENT` under `forge.github.env.sandbox` (and remove it from top-level `env.sandbox`). This keeps local/non-forge runs from failing env validation when RETRO_COMMENT is unset.

2) **Guarantee the variable is always set by the runner entrypoints**: ensure every invocation path sets `RETRO_COMMENT` in the host env (even to an empty string), so `os.LookupEnv("RETRO_COMMENT")` returns `(value, true)`.

(Option 1 is harness-only and preserves optionality semantics without requiring every caller to remember to export an empty var.)

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

---

[fullsend-ai-review]

Review

Findings

Medium

• [test-integrity] internal/scaffold/scaffold_test.go:685 — TestHarnessForgeRunnerEnvMerge still lists retro.yaml keys under topLevelKeys (FULLSEND_OUTPUT_SCHEMA) and forgeGithubKeys (ORIGINATING_URL, REPO_FULL_NAME, GH_TOKEN). After this PR, those keys move to Env.Runner, but the test only checks the combined map (merging both RunnerEnv and Env.Runner). The test still passes because the combined map includes Env.Runner, but it does not verify the keys landed in Env.Runner specifically rather than the deprecated RunnerEnv. The companion integration test (scaffold_integration_test.go) was correctly updated with envRunnerKeys, but this parallel test was not.

Previous run

Review

Findings

Medium

• [behavior-change] internal/scaffold/fullsend-repo/harness/retro.yaml:36 — The deleted retro.env used RETRO_COMMENT="${RETRO_COMMENT:-}" (bash default-to-empty when unset). The new env.sandbox uses RETRO_COMMENT: "${RETRO_COMMENT}", which is validated by ValidateRunnerEnvWith (harness.go:567-570) using os.LookupEnv. In production (GHA), RETRO_COMMENT is always set via reusable-retro.yml:159 (${{ fromJSON(inputs.event_payload).comment.body || '' }}), so this will not fail. But in local/non-GHA contexts where RETRO_COMMENT is truly unset, validation will now reject the harness where the old shell-based approach silently defaulted to empty.

Low

• [missing-authorization] No linked issue. The PR references ADR 0055 and describes Phase 2 of a multi-phase migration, but does not link to a tracking issue. Consider creating or linking one for the overall Phase 2 scaffold migration effort.
• [stale-reference] docs/superpowers/plans/2026-05-04-retro-agent.md contains references to env/retro.env (lines 22, 184, 197, 202, 669-670). This planning document now refers to a deleted file.
• [test-integrity] internal/harness/scaffold_integration_test.go:133 — The assertion is weakened from assert.NotEmpty(t, h.RunnerEnv) to an OR condition checking either RunnerEnv or Env.Runner. This is necessary during incremental migration and the companion TestResolveForge_ScaffoldRunnerEnvMerge test provides per-template granularity, mitigating the risk.
• [redundancy] internal/scaffold/fullsend-repo/harness/retro.yaml:43 — forge.github.env.sandbox duplicates ORIGINATING_URL, REPO_FULL_NAME, and GH_TOKEN from forge.github.env.runner. This is consistent with prior behavior and expected per ADR 0055 runner/sandbox separation.
• [documentation-drift] No tracking document records that Phase 2 has begun and retro.yaml is the first completed migration. Consider adding migration status tracking to ADR 0055 or a dedicated plan doc.

---

Labels: PR migrates retro agent harness config and updates harness integration tests.

Previous run

Review

Findings

Medium

• [behavior-change] internal/scaffold/fullsend-repo/harness/retro.yaml:36 — The deleted retro.env used RETRO_COMMENT="${RETRO_COMMENT:-}" (bash default-to-empty when unset). The new env.sandbox uses RETRO_COMMENT: "${RETRO_COMMENT}", which is validated by ValidateRunnerEnvWith (harness.go:537) using os.LookupEnv. In production (GHA), RETRO_COMMENT is always set via reusable-retro.yml (to comment body or empty string via || ""), so this will not fail. But in local/non-GHA contexts where RETRO_COMMENT is truly unset, validation will now reject the harness where the old shell-based approach silently defaulted to empty. Consider using a default value mechanism or marking RETRO_COMMENT as optional in the env.sandbox section.

Low

• [test-integrity] internal/scaffold/scaffold_test.go:686 — TestHarnessForgeRunnerEnvMerge still lists retro.yaml keys under topLevelKeys and forgeGithubKeys (semantically indicating they belong in the deprecated runner_env), while the companion integration test TestResolveForge_ScaffoldRunnerEnvMerge correctly empties those slices and moves the keys to envRunnerKeys. The combined-map approach makes this test pass, but it no longer verifies that the retro keys landed in the correct location (env.runner vs runner_env). The integration test provides the granular check, mitigating the risk, but these two test files are now inconsistent in how they describe the retro template's expected key placement.
• [test-assertion-consistency] internal/scaffold/scaffold_test.go:703 — The test introduces a combined map pattern to merge RunnerEnv and Env.Runner for assertions, but this pattern doesn't exist elsewhere in the codebase. Other tests (e.g., scaffold_integration_test.go:342-346) check deprecated and new fields separately with distinct assertions. See also: [test-integrity] finding at this location.
• [test-assertion-pattern] internal/scaffold/scaffold_test.go:635 — The assertion uses an intermediate boolean variable (hasRunnerEnv := ...) followed by assert.True(), while scaffold_integration_test.go:133-134 uses the condition directly in assert.True(). Minor style inconsistency between the two test files.

Previous run (2)

Review

Findings

Medium

• [behavior-change] internal/scaffold/fullsend-repo/harness/retro.yaml:36 — The deleted retro.env used RETRO_COMMENT="${RETRO_COMMENT:-}" (bash default-to-empty when unset). The new env.sandbox uses RETRO_COMMENT: "${RETRO_COMMENT}", which is validated by ValidateRunnerEnvWith (harness.go:567-570) using os.LookupEnv. In production (GHA), RETRO_COMMENT is always set via reusable-retro.yml:159 (${{ fromJSON(inputs.event_payload).comment.body || '' }}), so this will not fail. But in local/non-GHA contexts where RETRO_COMMENT is truly unset, validation will now reject the harness where the old shell-based approach silently defaulted to empty.

Low

• [missing-authorization] No linked issue. The PR references ADR 0055 and describes Phase 2 of a multi-phase migration, but does not link to a tracking issue. Consider creating or linking one for the overall Phase 2 scaffold migration effort.
• [stale-reference] docs/superpowers/plans/2026-05-04-retro-agent.md contains references to env/retro.env (lines 22, 184, 197, 202, 669-670). This planning document now refers to a deleted file.
• [test-integrity] internal/harness/scaffold_integration_test.go:133 — The assertion is weakened from assert.NotEmpty(t, h.RunnerEnv) to an OR condition checking either RunnerEnv or Env.Runner. This is necessary during incremental migration and the companion TestResolveForge_ScaffoldRunnerEnvMerge test provides per-template granularity, mitigating the risk.
• [redundancy] internal/scaffold/fullsend-repo/harness/retro.yaml:43 — forge.github.env.sandbox duplicates ORIGINATING_URL, REPO_FULL_NAME, and GH_TOKEN from forge.github.env.runner. This is consistent with prior behavior and expected per ADR 0055 runner/sandbox separation.
• [documentation-drift] No tracking document records that Phase 2 has begun and retro.yaml is the first completed migration. Consider adding migration status tracking to ADR 0055 or a dedicated plan doc.

---

Labels: PR migrates retro agent harness config and updates harness integration tests.

[fullsend-ai-review]

🤖 Review · ❌ Terminated · Started 6:54 PM UTC · Ended 7:07 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:54 PM UTC · Completed 7:07 PM UTC
Commit: 1c2ffb2 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 7:42 PM UTC · Completed 7:54 PM UTC
Commit: f1d276a · View workflow run →

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 3:49 PM UTC · Completed 3:56 PM UTC
Commit: f1d276a · View workflow run →

[fullsend-ai-retro]

PR #2761 was a small refactoring (3 files, 42 lines) migrating the retro harness from deprecated runner_env to env.runner/env.sandbox per ADR 0055, authored by ralphbean. The review agent performed well — it identified two medium-severity issues (RETRO_COMMENT behavior change losing default-to-empty semantics, and scaffold_test.go not updated to verify keys in Env.Runner) and several low-severity issues (stale doc references to deleted env/retro.env, missing tracking issue). The Qodo bot independently confirmed the RETRO_COMMENT finding. However, the human reviewer approved without comments, and all medium-severity findings were merged unresolved. Post-merge investigation confirms both medium findings are real: scaffold_test.go still expects retro keys in the old runner_env pattern, and 5 stale references to the deleted retro.env persist in docs. The review agent added more value than the human reviewer on this PR.

Proposals filed

• Fix scaffold_test.go retro test case to match ADR 0055 migration (in fullsend-ai/fullsend)
• Remove stale references to deleted env/retro.env in retro agent plan doc (in fullsend-ai/fullsend)
• Review agent should use CHANGES_REQUESTED for medium-severity behavior-change and test-integrity findings (in fullsend-ai/fullsend)