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

[ralphbean]

Migrates review agent harness from deprecated runner_env to unified env.runner/env.sandbox schema per ADR 0055. Moves sandbox env vars from env/review.env host_files into declarative env.sandbox sections. Updates scaffold integration tests.

[qodo-code-review]

PR Summary by Qodo

Migrate review harness to env.runner/env.sandbox (ADR 0055)

⚙️ Configuration changes 🧪 Tests 📝 Documentation 🕐 20-40 Minutes

AI Description

• Migrate review harness YAML from deprecated runner_env to env.runner/env.sandbox.
• Replace host_files-based env/review.env injection with declarative sandbox variables.
• Update scaffold integration tests and docs to match the new env schema.

Diagram

graph TD
  A["harness/review.yaml"] --> B["env.runner"] --> C["Review agent"]
  A --> D["env.sandbox"] --> C
  A --> E["forge.github.env"] --> F["GitHub forge"]
  E --> G["env.runner"] --> F
  E --> H["env.sandbox"] --> F
  subgraph Legend
    direction LR
    _cfg["Config (YAML)"] ~~~ _proc["Process"]
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Loader-level compatibility shim (normalize runner_env → env.runner)

• ➕ Centralizes backward compatibility in one place
• ➕ Avoids updating tests to understand dual fields
• ➕ Reduces risk of partial migrations across harnesses
• ➖ Adds long-lived tech debt and dual-schema support surface area
• ➖ Can mask incomplete migrations and delay full adoption of ADR 0055

2. Keep .env host_file injection (generate from env.sandbox at build time)

• ➕ Preserves existing sandbox tooling that expects .env files
• ➕ Could ease transition for other consumers of env/review.env
• ➖ Reintroduces indirection that ADR 0055 is trying to remove
• ➖ Requires additional generation step and careful escaping/quoting

Recommendation: The PR’s approach (moving variables into declarative env.runner/env.sandbox and updating tests to tolerate the transition) is the best fit for ADR 0055 and reduces reliance on host_files .env injection. If additional harnesses still depend on runner_env, consider a time-bounded loader shim as a migration aid, but avoid keeping dual support indefinitely.

Files changed (3) +34 / -14

Tests (1) +15 / -3

scaffold_integration_test.goMake scaffold tests accept both legacy RunnerEnv and new Env.Runner +15/-3

Make scaffold tests accept both legacy RunnerEnv and new Env.Runner

• Updates scaffold integration tests to treat runner environment variables as present if either the legacy h.RunnerEnv map is populated or the new h.Env.Runner map is populated. For forge resolution tests, builds a combined env map from both sources to validate expected merged keys regardless of which schema produced them.

internal/harness/scaffold_integration_test.go

Documentation (1) +1 / -1

SKILL.mdUpdate docs to reference env.sandbox instead of env/review.env +1/-1

Update docs to reference env.sandbox instead of env/review.env

• Adjusts documentation comments to reflect that PR metadata variables are now provided via env.sandbox in the review harness YAML rather than through an injected env/review.env file.

internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md

Other (1) +18 / -10

review.yamlMigrate review harness env config to env.runner/env.sandbox +18/-10

Migrate review harness env config to env.runner/env.sandbox

• Removes the host_files injection of env/review.env and replaces runner_env with env.runner at the top level. Adds env.sandbox for generic sandbox variables and introduces forge.github.env.runner plus forge.github.env.sandbox for GitHub-specific runner and sandbox variables.

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

[github-actions]

Site preview

Preview: https://2143fe08-site.fullsend-ai.workers.dev

Commit: ee28d40bcc864236693efead9e36a4e0b12f0d93

[fullsend-ai-review]

🤖 Review · ❌ Terminated · Started 5:58 PM UTC · Ended 6:21 PM UTC
Commit: 657bf48 · 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. Unset threshold fails validation 🐞 Bug ☼ Reliability

Description

internal/scaffold/fullsend-repo/harness/review.yaml now references
${REVIEW_FINDING_SEVERITY_THRESHOLD} in env.sandbox, and fullsend run validates that every
${VAR} reference exists in the host environment. The reusable review workflow does not define
REVIEW_FINDING_SEVERITY_THRESHOLD, so the review job can fail early with an environment validation
error.

Code

internal/scaffold/fullsend-repo/harness/review.yaml[R37-43]

+env:
+  runner:
+    FULLSEND_OUTPUT_SCHEMA: ${FULLSEND_DIR}/schemas/review-result.schema.json
+  sandbox:
+    PRIOR_REVIEW_SHA: "${PRIOR_REVIEW_SHA}"
+    PRIOR_REVIEW_PROVENANCE: "${PRIOR_REVIEW_PROVENANCE}"
+    REVIEW_FINDING_SEVERITY_THRESHOLD: "${REVIEW_FINDING_SEVERITY_THRESHOLD}"

Relevance

⭐⭐ Medium

Similar “unset env var fails ValidateRunnerEnvWith” suggestion was rejected in PR #2346; but
threshold treated optional in PR #2341.

PR-#2346
PR-#2341

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The harness now references ${REVIEW_FINDING_SEVERITY_THRESHOLD} in env.sandbox, and the runner
validates all ${VAR} references via ValidateRunnerEnvWith, which errors if os.LookupEnv
reports the variable is unset. The reusable workflow that invokes the review agent does not define
REVIEW_FINDING_SEVERITY_THRESHOLD, so the validation can fail at runtime.

internal/scaffold/fullsend-repo/harness/review.yaml[37-43]
internal/cli/run.go[343-392]
internal/harness/harness.go[533-573]
.github/workflows/reusable-review.yml[167-177]

Agent prompt

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

## Issue description
`fullsend run` calls `Harness.ValidateRunnerEnvWith(os.LookupEnv)` which errors when any `${VAR}` reference in `env.runner`/`env.sandbox` is *unset* (missing from the environment, even if conceptually optional).

This PR introduces `env.sandbox.REVIEW_FINDING_SEVERITY_THRESHOLD: "${REVIEW_FINDING_SEVERITY_THRESHOLD}"` but the reusable workflow that runs the review agent does not define `REVIEW_FINDING_SEVERITY_THRESHOLD` at all, so the workflow can fail before running pre_script/agent.

## Issue Context
`REVIEW_FINDING_SEVERITY_THRESHOLD` is documented as optional (empty/unset should behave like `low`), but the runner’s validation requires the host variable to exist if referenced.

## Fix Focus Areas
- .github/workflows/reusable-review.yml[167-177]
- internal/scaffold/fullsend-repo/harness/review.yaml[37-43]

## Suggested fix
Add `REVIEW_FINDING_SEVERITY_THRESHOLD` to the reusable workflow env for the “Run review agent” step, setting it to an empty string by default (or to a repo/org variable), e.g.:

```yaml
env:
 ...
 REVIEW_FINDING_SEVERITY_THRESHOLD: ${{ vars.REVIEW_FINDING_SEVERITY_THRESHOLD }}
```

This ensures the variable is always present (possibly empty), satisfying validation while preserving optional behavior.

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

---

2. REPO_FULL_NAME comment-only edit 📘 Rule violation ⚙ Maintainability

Description

A comment line in a shell snippet was modified without any accompanying non-comment code change in
the same diff hunk. This violates the policy against comment-only changes and makes it harder to
audit whether the change is tied to functional work.

Code

internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md[169]

+# REPO_FULL_NAME and PR_NUMBER are set via env.sandbox in harness/review.yaml

Relevance

⭐ Low

Repo merges comment-only edits (e.g., PR #2126), suggesting “no comment-only changes” policy isn’t
enforced here.

PR-#2126

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 1062072 requires that comment edits not occur in isolation; the modified hunk
changes only a # comment line in the bash block, with no other code line changes in that hunk.

Rule 1062072: Do not add or modify comments outside code lines changed for the issue
internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md[169-169]

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 changes only a comment line in `internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md` without changing any non-comment code line in the same hunk, which violates the "no comment-only changes" rule.

## Issue Context
This change is in a bash snippet under the prior-review compare logic; the surrounding commands are unchanged.

## Fix Focus Areas
- internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md[169-172]

Suggested approach:
- Either revert the comment-only change, OR
- Add a small, relevant non-comment line in the snippet (e.g., a guard like `: "${REPO_FULL_NAME:?}"` / `: "${PR_NUMBER:?}"`) so the hunk is not comment-only and the snippet becomes more robust.

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

---

[qodo-code-review]

2. Unset threshold fails validation 🐞 Bug ☼ Reliability

internal/scaffold/fullsend-repo/harness/review.yaml now references
${REVIEW_FINDING_SEVERITY_THRESHOLD} in env.sandbox, and fullsend run validates that every
${VAR} reference exists in the host environment. The reusable review workflow does not define
REVIEW_FINDING_SEVERITY_THRESHOLD, so the review job can fail early with an environment validation
error.

Agent Prompt

## Issue description
`fullsend run` calls `Harness.ValidateRunnerEnvWith(os.LookupEnv)` which errors when any `${VAR}` reference in `env.runner`/`env.sandbox` is *unset* (missing from the environment, even if conceptually optional).

This PR introduces `env.sandbox.REVIEW_FINDING_SEVERITY_THRESHOLD: "${REVIEW_FINDING_SEVERITY_THRESHOLD}"` but the reusable workflow that runs the review agent does not define `REVIEW_FINDING_SEVERITY_THRESHOLD` at all, so the workflow can fail before running pre_script/agent.

## Issue Context
`REVIEW_FINDING_SEVERITY_THRESHOLD` is documented as optional (empty/unset should behave like `low`), but the runner’s validation requires the host variable to exist if referenced.

## Fix Focus Areas
- .github/workflows/reusable-review.yml[167-177]
- internal/scaffold/fullsend-repo/harness/review.yaml[37-43]

## Suggested fix
Add `REVIEW_FINDING_SEVERITY_THRESHOLD` to the reusable workflow env for the “Run review agent” step, setting it to an empty string by default (or to a repo/org variable), e.g.:

```yaml
env:
  ...
  REVIEW_FINDING_SEVERITY_THRESHOLD: ${{ vars.REVIEW_FINDING_SEVERITY_THRESHOLD }}
```

This ensures the variable is always present (possibly empty), satisfying validation while preserving optional behavior.

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

[fullsend-ai-review]

🤖 Finished Review · ❌ Failure · Started 5:58 PM UTC · Completed 6:20 PM UTC
Commit: 2b8dbaf · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 6:54 PM UTC · Completed 7:07 PM UTC
Commit: 3ab61da · 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]

Looks good to me

Previous run

Review

Findings

Medium

• [test-integrity] internal/harness/scaffold_integration_test.go:133 — The assertion weakening from assert.NotEmpty(t, h.RunnerEnv) to len(h.RunnerEnv) > 0 || (h.Env != nil && len(h.Env.Runner) > 0) applies to ALL scaffold harnesses in the loop (via scaffold.HarnessNames()), not just review.yaml. Since only review.yaml is migrated in this PR, the other 5 harnesses still use runner_env exclusively. A future regression that empties RunnerEnv for an unmigrated harness but accidentally populates Env.Runner would silently pass. Additionally, the test does not positively verify that review.yaml actually uses env.runner (not runner_env), so the migration could be reverted without test failure.
  Remediation: Use a conditional assertion — for "review", assert h.Env.Runner is populated and h.RunnerEnv is empty; for other harnesses, keep the original assert.NotEmpty(t, h.RunnerEnv).

• [test-integrity] internal/scaffold/scaffold_test.go:635 — Same loosened assertion issue in TestHarnessesLoadAndValidate. The OR-check applies uniformly to all harnesses, weakening coverage for the five that still use runner_env.
  Remediation: Apply the same conditional assertion pattern as above.

Low

• [missing-authorization] No linked issue. The PR references ADR 0055 which establishes architectural direction, but non-trivial implementation work benefits from an explicit tracking issue.

• [incomplete-migration] Only review.yaml is migrated while other scaffold harnesses (triage.yaml, code.yaml, fix.yaml, retro.yaml, prioritize.yaml) still use deprecated runner_env. The PR title clearly scopes to the review agent, but a note in the description about phased rollout would clarify intent.

• [pattern-duplication] internal/scaffold/scaffold_test.go:704 — The 10-line combined env map construction is duplicated between scaffold_integration_test.go and scaffold_test.go. Consider extracting to a test helper if this pattern will recur as other harnesses migrate.

---

Labels: PR modifies review agent harness YAML config and scaffold integration tests for env var delivery migration.

Previous run

Looks good to me

Previous run (2)

Review

Findings

Medium

• [test-integrity] internal/harness/scaffold_integration_test.go:133 — The assertion weakening from assert.NotEmpty(t, h.RunnerEnv) to len(h.RunnerEnv) > 0 || (h.Env != nil && len(h.Env.Runner) > 0) applies to ALL scaffold harnesses in the loop (via scaffold.HarnessNames()), not just review.yaml. Since only review.yaml is migrated in this PR, the other 5 harnesses still use runner_env exclusively. A future regression that empties RunnerEnv for an unmigrated harness but accidentally populates Env.Runner would silently pass. Additionally, the test does not positively verify that review.yaml actually uses env.runner (not runner_env), so the migration could be reverted without test failure.
  Remediation: Use a conditional assertion — for "review", assert h.Env.Runner is populated and h.RunnerEnv is empty; for other harnesses, keep the original assert.NotEmpty(t, h.RunnerEnv).

• [test-integrity] internal/scaffold/scaffold_test.go:635 — Same loosened assertion issue in TestHarnessesLoadAndValidate. The OR-check applies uniformly to all harnesses, weakening coverage for the five that still use runner_env.
  Remediation: Apply the same conditional assertion pattern as above.

Low

• [missing-authorization] No linked issue. The PR references ADR 0055 which establishes architectural direction, but non-trivial implementation work benefits from an explicit tracking issue.

• [incomplete-migration] Only review.yaml is migrated while other scaffold harnesses (triage.yaml, code.yaml, fix.yaml, retro.yaml, prioritize.yaml) still use deprecated runner_env. The PR title clearly scopes to the review agent, but a note in the description about phased rollout would clarify intent.

• [pattern-duplication] internal/scaffold/scaffold_test.go:704 — The 10-line combined env map construction is duplicated between scaffold_integration_test.go and scaffold_test.go. Consider extracting to a test helper if this pattern will recur as other harnesses migrate.

---

Labels: PR modifies review agent harness YAML config and scaffold integration tests for env var delivery migration.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 7:40 PM UTC · Completed 7:52 PM UTC
Commit: 6297ef7 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 12:34 PM UTC · Completed 12:44 PM UTC
Commit: ee28d40 · View workflow run →

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 3:04 PM UTC · Completed 3:14 PM UTC
Commit: ee28d40 · View workflow run →

[fullsend-ai-retro]

Retro: PR #2758 — migrate review agent to env.runner/env.sandbox (ADR 0055)

Timeline

Time	Event
Jun 29 17:55	PR opened by ralphbean (+19/−18, 3 files)
Jun 29 17:58–18:21	Review run 1 FAILED — agent ran 2 iterations but never produced agent-result.json
Jun 29 18:03	Qodo bot flagged a real bug: REVIEW_FINDING_SEVERITY_THRESHOLD referenced in env.sandbox but not defined in the reusable workflow
Jun 29 18:54–19:07	Review run 2 succeeded — found 2 medium (test-integrity) + 3 low findings
Jun 30 09:07	Human reviewer (rh-hemartin) approved
Jun 30 19:40–19:52	Review run 3 succeeded — identical findings to run 2
Jul 2 12:34–12:44	Review run 4 succeeded — "Looks good to me" (test-integrity findings addressed)
Jul 2 15:01	PR merged

Key findings

1. Review agent missed a real bug that Qodo caught. PR refactor(harness): migrate review agent to env.runner/env.sandbox (ADR 0055) #2758 moved REVIEW_FINDING_SEVERITY_THRESHOLD from env/review.env (delivered via host_files with expand: true, which tolerates unset vars) into env.sandbox (which validates via os.LookupEnv and errors on unset vars). The reusable review workflow does not define this variable. Any repo using the default workflow without externally defining this variable will hit a validation error before the review agent starts. The review agent approved the final commit without flagging this. This bug was merged unfixed.

2. First review run wasted ~26 minutes of compute. The agent completed without crashing but failed to write the required output file after 2 iterations. Existing issue #2711 covers auto re-dispatch after infrastructure failures.

3. 4 review runs for a 3-file PR. Runs 2 and 3 produced identical findings on different commits. Existing issues #1370, #1452, and #2599 cover redundant review deduplication.

4. Review agent's test-integrity findings were high quality. The medium findings about weakened assertions in scaffold integration tests were legitimate and were addressed before the final review run.

Proposals

• 1 new proposal for fixing the merged bug and preventing recurrence via the reusable workflow.
• 1 new proposal for improving the review agent's cross-file consistency checking on harness/workflow migrations.
• Redundant-run and first-run-failure issues were skipped — already covered by Review agent accumulates redundant internal iterations on clean diffs #1370, Deduplicate review dispatches for the same HEAD SHA #1452, Add per-PR review budget to cap runaway review dispatches #2599, Auto re-dispatch review after consecutive infrastructure failures with backoff #2711.

Proposals filed

• Fix: reusable-review.yml missing REVIEW_FINDING_SEVERITY_THRESHOLD env var (in fullsend-ai/fullsend)
• Review agent should detect env.sandbox references unsatisfied by calling workflows (in fullsend-ai/fullsend)