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

[ralphbean]

Migrate prioritize harness from deprecated runner_env to env.runner/env.sandbox per ADR 0055. Move sandbox env vars from prioritize.env host file into forge.github.env.sandbox. Delete the now-unused env file. Update tests for backward compatibility.

[qodo-code-review]

PR Summary by Qodo

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

⚙️ Configuration changes 🧪 Tests 🕐 20-40 Minutes

AI Description

• Migrate prioritize harness from legacy runner_env to env.runner/env.sandbox per ADR 0055.
• Inline sandbox GitHub env vars into harness YAML; remove the unused host env file.
• Update scaffold tests to accept either legacy RunnerEnv or new Env.Runner during migration.

Diagram

graph TD
  A["prioritize.yaml"] --> B["Harness loader/resolver"] --> C["Env.Runner map"] --> D["Runner process"]
  A --> E["Env.Sandbox map"] --> F["Sandbox env"]
  A --> G["Forge github overlay"] --> B
  H["Scaffold tests"] --> B

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Make the migration breaking (drop RunnerEnv everywhere)

• ➕ Simplifies code and tests by removing dual-field compatibility
• ➕ Forces all agents/harnesses to converge immediately on the ADR schema
• ➖ Higher risk of breaking older agents/consumers still reading RunnerEnv
• ➖ Requires coordinated updates across multiple repos/consumers

2. Add a first-class normalized accessor (e.g., EffectiveRunnerEnv())

• ➕ Eliminates repeated 'combine legacy + new' logic in tests and callers
• ➕ Creates a single migration point and clearer deprecation path
• ➖ Adds/changes API surface area
• ➖ Requires follow-up refactors to adopt the accessor broadly

Recommendation: The current approach (migrate YAML to env.runner/env.sandbox while keeping tests tolerant of legacy RunnerEnv) is appropriate for a staged rollout. Consider a small follow-up to centralize the merge logic (e.g., EffectiveRunnerEnv()) to reduce duplication and make the migration semantics explicit, but it is not strictly required for this PR.

Files changed (3) +45 / -18

Tests (2) +33 / -8

scaffold_integration_test.goAccept unified env schema during scaffold forge resolution tests +15/-3

Accept unified env schema during scaffold forge resolution tests

• Updates assertions to treat either legacy RunnerEnv or the new Env.Runner map as valid after merge. For runner-env merge coverage, builds a combined map from both sources before checking expected keys.

internal/harness/scaffold_integration_test.go

scaffold_test.goUpdate scaffold expectations for prioritize harness env schema +18/-5

Update scaffold expectations for prioritize harness env schema

• Removes the expectation that env/prioritize.env exists in the fullsend scaffold. Adjusts harness validation tests to accept either RunnerEnv or Env.Runner, and updates content checks to look for env/runner/sandbox keys instead of runner_env.

internal/scaffold/scaffold_test.go

Other (1) +12 / -10

prioritize.yamlMigrate prioritize harness from runner_env to env.runner/env.sandbox +12/-10

Migrate prioritize harness from runner_env to env.runner/env.sandbox

• Replaces top-level and forge.github runner_env blocks with the unified env.runner mapping. Moves GitHub-related variables previously provided via a host env file into forge.github.env.sandbox, and removes the host file mount.

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

[github-actions]

Site preview

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

Commit: ca30e8851db3986724e2b0a63322061cd2ae5802

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 6:03 PM UTC · Completed 6:15 PM UTC
Commit: ca30e88 · View workflow run →

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[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. Forge-only sandbox env 🐞 Bug ≡ Correctness

Description

prioritize.yaml now sets GITHUB_ISSUE_URL and GH_TOKEN only under forge.github.env.sandbox, so when
forge platform is empty (no --forge and not in CI), ResolveForge is skipped and the sandbox never
receives these env vars, breaking the prioritize agent’s gh issue view step.

Code

internal/scaffold/fullsend-repo/harness/prioritize.yaml[R38-46]

+    env:
+      runner:
+        GITHUB_ISSUE_URL: ${GITHUB_ISSUE_URL}
+        GH_TOKEN: ${GH_TOKEN}
+        ORG: ${ORG}
+        PROJECT_NUMBER: ${PROJECT_NUMBER}
+      sandbox:
+        GITHUB_ISSUE_URL: "${GITHUB_ISSUE_URL}"
+        GH_TOKEN: "${GH_TOKEN}"

Relevance

⭐⭐⭐ High

Team previously fixed missing/fallback env context vars (accepted in PR #171); PR #2260 stresses
non---forge local defaults.

PR-#171
PR-#2260

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

In non-CI runs without --forge, the runner sets ForgePlatform to empty, so the forge.github
block in prioritize.yaml is not merged; since bootstrap only exports env.sandbox, the sandbox
won’t get GH_TOKEN/GITHUB_ISSUE_URL and the prioritize agent’s gh issue view will fail.

internal/scaffold/fullsend-repo/harness/prioritize.yaml[28-46]
internal/cli/run.go[181-228]
internal/cli/run.go[2054-2071]
internal/cli/run.go[1339-1393]
internal/scaffold/fullsend-repo/agents/prioritize.md[14-23]

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 prioritize scaffold harness provides required sandbox environment variables (GITHUB_ISSUE_URL, GH_TOKEN) only under `forge.github.env.sandbox`. When `fullsend run` is executed locally (or any non-CI environment) without `--forge`, `detectForgePlatform()` returns an empty string, so forge resolution is skipped and those sandbox env vars are never exported into the sandbox. The prioritize agent then fails because `gh issue view` cannot run without these inputs.

## Issue Context
- `detectForgePlatform()` returns "" when not in CI and no flag is provided.
- `bootstrapEnv()` exports only `env.sandbox` vars into the sandbox `.env` script (it does not export host OS env vars or `env.runner` into the sandbox).
- The prioritize agent requires `GITHUB_ISSUE_URL` and uses `gh issue view`.

## Fix Focus Areas
- internal/scaffold/fullsend-repo/harness/prioritize.yaml[28-46]

### Suggested change
Move the required sandbox env vars to top-level `env.sandbox` (and optionally top-level `env.runner` if you want host scripts to receive them via the harness env merge), so they are available even when forge resolution is skipped.
Example direction:
- Add:
 - `env.sandbox.GITHUB_ISSUE_URL: ${GITHUB_ISSUE_URL}`
 - `env.sandbox.GH_TOKEN: ${GH_TOKEN}`
- Optionally also add these to `env.runner` (if you want them explicitly included in the effective runner env rather than relying on `os.Environ()`).
- Then remove the duplicated `forge.github.env.sandbox` entries (or keep them if you want explicit forge overrides, but they should not be the only source of these sandbox vars).

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

---

[fullsend-ai-review]

Looks good to me — clean mechanical migration of the prioritize harness from deprecated runner_env to env.runner/env.sandbox per ADR 0055. Env var coverage is preserved, host_file removal is consistent with the new env.sandbox block, stale references are cleaned up, and tests are updated for backward compatibility.

Findings

Low

• [missing-authorization] internal/scaffold/fullsend-repo/harness/prioritize.yaml — No linked issue for this migration work. ADR 0055 Phase 2 explicitly authorizes the direction, so this is a process nit rather than a scope concern. Consider linking a tracking issue for the Phase 2 migration.

• [incomplete-migration] internal/scaffold/fullsend-repo/harness/prioritize.yaml — Only the prioritize harness is migrated; five others (review, code, fix, triage, retro) still use runner_env. The runtime supports both mechanisms with deprecation warnings, so the incremental approach is safe. Consider tracking remaining harnesses in a follow-up issue.

• [test-assertion-consistency] internal/harness/scaffold_integration_test.go:133, internal/scaffold/scaffold_test.go:635 — The hasRunnerEnv := len(h.RunnerEnv) > 0 || ... + assert.True pattern differs from the established assert.NotEmpty/assert.Contains patterns elsewhere in the test suite. The pattern is functionally correct for backward compatibility during migration, but separate assertions per field would match existing conventions and give clearer failure messages.

• [test-adequacy] internal/harness/scaffold_integration_test.go:335, internal/scaffold/scaffold_test.go:703 — The combined-map pattern in TestResolveForge_ScaffoldRunnerEnvMerge and TestHarnessForgeRunnerEnvMerge masks which field (RunnerEnv vs Env.Runner) each key lands in. Acceptable during migration since other tests (e.g., forge_test.go) verify field placement directly.

---

Labels: PR modifies harness config and scaffold templates specifically for the prioritize agent's env migration.

[fullsend-ai-review]

[low] test-assertion-consistency

The boolean variable + assert.True pattern for checking env presence differs from the established assert.NotEmpty/assert.Contains patterns elsewhere. Functionally correct for backward compatibility but inconsistent with test conventions. Same pattern at internal/scaffold/scaffold_test.go:635.

[fullsend-ai-review]

[low] test adequacy

The combined-map assertion pattern in TestResolveForge_ScaffoldRunnerEnvMerge masks which field (RunnerEnv vs Env.Runner) each key lands in after forge resolution. Acceptable during migration since other tests verify field placement directly. Same pattern at internal/scaffold/scaffold_test.go:703.

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 11:59 AM UTC · Completed 12:05 PM UTC
Commit: ca30e88 · View workflow run →

[fullsend-ai-retro]

Retro: PR #2759 — Migrate prioritize agent to env.runner/env.sandbox

Verdict: Clean workflow, no new proposals.

This was a human-authored refactoring PR (ralphbean, assisted by Claude Opus 4.6) that migrated the prioritize harness from deprecated runner_env to env.runner/env.sandbox per ADR 0055. Single commit, 4 files changed (45+/20-), merged in ~18 hours with zero rework.

What went well

• Review agent approved with 4 low-severity findings, all proportionate and non-blocking. No false escalations.
• Human reviewer (rh-hemartin) approved silently — consistent with the review agent's assessment that no blocking issues existed.
• Test coverage confirmed by Codecov — all modified lines covered.
• Time to resolution was efficient for a human-authored PR: ~18h from open to merge with one review cycle.

Points of interest

• Qodo flagged a potential bug about sandbox env vars (GITHUB_ISSUE_URL, GH_TOKEN) only existing under forge.github.env.sandbox, claiming local runs without --forge would break. Analysis suggests this is a false positive — these variables are inherently CI/forge-specific (the issue URL comes from the dispatch event, the token from CI). Placing them under forge scope is the correct design per ADR 0055.
• Review agent noise: 2 of 4 findings were arguably non-actionable ("missing-authorization" on a human refactoring PR, "incomplete-migration" noting other harnesses aren't migrated yet — expected per phased rollout). However, these are already covered by existing issues:
  • #2607 — recognize intentionally unlinked PRs
  • #2115 — use COMMENT verdict for human PRs with only low findings
  • #2665 — don't post inline comments for non-actionable findings

No new proposals filed — all improvement opportunities identified are already tracked in existing open issues.