feat(harness): move GitHub-specific fields into forge.github blocks

[ggallen]

Summary

• Restructure all 6 scaffold harness templates (triage, code, review, fix, retro, prioritize) to use forge.github: blocks for platform-specific fields (GitHub-specific runner_env keys like GH_TOKEN, PUSH_TOKEN, REPO_FULL_NAME, etc.)
• pre_script and post_script are kept at the top level as defaults (for local dev without --forge) AND duplicated in forge.github: (forge overrides when platform is set)
• Platform-neutral runner_env keys (FULLSEND_OUTPUT_SCHEMA, TARGET_BRANCH, etc.) remain at the top level as shared defaults
• Templates produce identical runtime behavior when loaded with --forge github via Phase 1's ResolveForge merge logic

ADR-0045 Phase 2 — forge.github migration. See docs/plans/adr-0045-forge-portable-harness-phase2.md (PR 2 in the Phase 2 dependency graph: "Move GitHub-specific fields into forge.github: in scaffold templates").

Test plan

• TestHarnessesLoadAndValidate updated: verifies both Load() (backward compat — pre/post scripts at top level as defaults, forge block present) and LoadWithOpts with github (forge resolved, pre/post scripts populated, files exist)
• New TestHarnessForgeRunnerEnvMerge verifies merged RunnerEnv contains both top-level and forge.github keys for all 6 templates
• Full scaffold, harness, and CLI test suites pass
• go vet and make lint clean
• All forge code paths at 100% coverage

🤖 Generated with Claude Code

[github-actions]

Site preview

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

Commit: 3483fbc399c9d213358569f5e35e25dae250d3a7

[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 7:26 PM UTC · Completed 7:38 PM UTC
Commit: 47f82d3 · View workflow run →

[fullsend-ai-review]

Review

Findings

Medium

• [architectural-coherence] internal/scaffold/fullsend-repo/harness/code.yaml — pre_script and post_script are duplicated identically at top level AND in forge.github. ADR-0045's inheritance model specifies that forge blocks override top-level defaults — its canonical example (lines 213–214) shows forge.github.pre_script containing a different value from the top-level pre_script. Duplicating the same value at both levels adds maintenance burden (two places to update) without benefit under the inheritance model. This applies to all 6 harness files.
  Remediation: Either remove pre_script/post_script from forge.github blocks (letting inheritance provide them) or remove them from top level if the intent is forge-specificity. Identical values at both levels serves no purpose.

Low

• [missing-authorization] N/A — Non-trivial change with no linked issue. The PR references ADR-0045 Phase 2, which is an accepted ADR describing exactly this work scope, so authorization is implicit in the ADR. A tracking issue would improve process hygiene.

• [stale-harness-schema-documentation] docs/guides/user/customizing-agents.md — Harness examples show GitHub-specific runner_env keys (PUSH_TOKEN, REPO_FULL_NAME, REPO_DIR) at top level. Per ADR-0045 Phase 2, top-level placement remains valid for single-forge harnesses, but examples could be updated to show the forge.github structure as the recommended pattern.

• [stale-harness-schema-documentation] docs/guides/user/building-custom-agents.md — Custom agent building guide shows GH_TOKEN in top-level runner_env without mentioning forge.github.runner_env as an alternative. Valid per ADR-0045 Phase 2 backward compatibility, but could note the multi-forge option.

• [incomplete-migration-documentation] docs/ADRs/0045-forge-portable-harness-schema.md — ADR-0045 classifies which fields can appear at both levels but doesn't classify individual runner_env keys as GitHub-specific vs platform-neutral. This PR establishes that classification implicitly — documenting it in the ADR would help future contributors.

Info

• [scope-alignment] N/A — PR body correctly references "ADR-0045 Phase 2" without numbered PR labels. The actual work matches Phase 2 Adopt scope.

Previous run

Review

Findings

Medium

• [protected-path] internal/scaffold/fullsend-repo/harness/*.yaml — All 6 modified harness YAML files are under the harness/ protected path. The PR references ADR-0045 Phase 2 as authorization and the description explains the rationale. Human approval is always required for protected-path changes, regardless of context.

• [architectural-coherence] internal/scaffold/fullsend-repo/harness/code.yaml — Some runner_env keys moved to forge.github (e.g., REPO_FULL_NAME, ISSUE_NUMBER) represent generic concepts that could exist on any forge, not just GitHub. If GitLab or Forgejo need equivalent variables with different names, the current split is correct. If the scripts are expected to use the same variable names across forges, consider using platform-neutral names at the top level. The REPO_DIR key references ${GITHUB_WORKSPACE} which is GitHub-specific, supporting the current classification for that key.

Low

• [stale-harness-schema-documentation] docs/guides/user/customizing-agents.md — Two harness examples (lines ~43 and ~278) show GitHub-specific runner_env keys (PUSH_TOKEN, REPO_FULL_NAME, REPO_DIR) at the top level. Per ADR-0045, top-level placement remains valid for single-forge harnesses, but the examples could be enhanced to mention the forge.github option for multi-forge deployments.

• [stale-harness-schema-documentation] docs/guides/user/building-custom-agents.md — Custom agent building guide shows GH_TOKEN in top-level runner_env without mentioning forge.github.runner_env as an alternative. Valid for single-forge but could note the multi-forge option.

• [incomplete-migration-documentation] docs/ADRs/0045-forge-portable-harness-schema.md — ADR-0045 operates at the schema level (runner_env as a whole can be forge-specific) but doesn't list specific key classification examples. The scaffold harness files now serve as the canonical reference for which keys are platform-neutral vs platform-specific.

• [field-ordering-convention] internal/scaffold/fullsend-repo/harness/code.yaml — ADR-0045's canonical example shows a specific field order that this PR doesn't follow. The existing codebase already has inconsistent ordering. YAML is order-independent, so this is a style preference.

Info

• [prior-finding-resolution] internal/scaffold/fullsend-repo/harness/code.yaml — Prior medium finding (top-level pre_script/post_script missing) is resolved. All six YAML files now keep pre_script and post_script at the top level as defaults, addressing the local-dev regression identified in the previous review.

• [scope-alignment] N/A — PR body says "Phase 2 PR 2" but the ADR-0045 plan uses "PR 1-7" for Phase 1 only; Phase 2 is described as "3-4 PRs" without numbered labels. The PR's actual work matches Phase 2 Adopt scope.

• [documentation-consistency] internal/scaffold/fullsend-repo/harness/code.yaml — The security boundary comment was updated from "post_script" to "pre/post scripts" which is accurate. Consider also documenting the split between top-level (platform-neutral) and forge.github (platform-specific) runner_env keys.

Previous run

Review

Findings

Medium

• [protected-path] internal/scaffold/fullsend-repo/harness/*.yaml — All 6 modified harness YAML files are under the harness/ protected path. The PR references ADR-0045 Phase 2 as authorization and the description explains the rationale. Human approval is always required for protected-path changes, regardless of context.

• [architectural-coherence] internal/scaffold/fullsend-repo/harness/code.yaml — Some runner_env keys moved to forge.github (e.g., REPO_FULL_NAME, ISSUE_NUMBER) represent generic concepts that could exist on any forge, not just GitHub. If GitLab or Forgejo need equivalent variables with different names, the current split is correct. If the scripts are expected to use the same variable names across forges, consider using platform-neutral names at the top level. The REPO_DIR key references ${GITHUB_WORKSPACE} which is GitHub-specific, supporting the current classification for that key.

Low

• [stale-harness-schema-documentation] docs/guides/user/customizing-agents.md — Two harness examples (lines ~43 and ~278) show GitHub-specific runner_env keys (PUSH_TOKEN, REPO_FULL_NAME, REPO_DIR) at the top level. Per ADR-0045, top-level placement remains valid for single-forge harnesses, but the examples could be enhanced to mention the forge.github option for multi-forge deployments.

• [stale-harness-schema-documentation] docs/guides/user/building-custom-agents.md — Custom agent building guide shows GH_TOKEN in top-level runner_env without mentioning forge.github.runner_env as an alternative. Valid for single-forge but could note the multi-forge option.

• [incomplete-migration-documentation] docs/ADRs/0045-forge-portable-harness-schema.md — ADR-0045 operates at the schema level (runner_env as a whole can be forge-specific) but doesn't list specific key classification examples. The scaffold harness files now serve as the canonical reference for which keys are platform-neutral vs platform-specific.

• [field-ordering-convention] internal/scaffold/fullsend-repo/harness/code.yaml — ADR-0045's canonical example shows a specific field order that this PR doesn't follow. The existing codebase already has inconsistent ordering. YAML is order-independent, so this is a style preference.

Info

• [prior-finding-resolution] internal/scaffold/fullsend-repo/harness/code.yaml — Prior medium finding (top-level pre_script/post_script missing) is resolved. All six YAML files now keep pre_script and post_script at the top level as defaults, addressing the local-dev regression identified in the previous review.

• [scope-alignment] N/A — PR body says "Phase 2 PR 2" but the ADR-0045 plan uses "PR 1-7" for Phase 1 only; Phase 2 is described as "3-4 PRs" without numbered labels. The PR's actual work matches Phase 2 Adopt scope.

• [documentation-consistency] internal/scaffold/fullsend-repo/harness/code.yaml — The security boundary comment was updated from "post_script" to "pre/post scripts" which is accurate. Consider also documenting the split between top-level (platform-neutral) and forge.github (platform-specific) runner_env keys.

Previous run (2)

Review

Findings

Medium

• [logic-error] internal/scaffold/fullsend-repo/harness/code.yaml — All six harness YAMLs move pre_script and post_script exclusively inside forge.github, with no top-level fallback. When fullsend run is invoked locally (no --forge flag, no CI env vars), detectForgePlatform returns "" and ResolveForge is a no-op, leaving h.PreScript and h.PostScript empty. The runner silently skips them. This is a behavioral regression for local development workflows where pre/post scripts perform essential setup and teardown.
  Remediation: Either (a) keep a top-level pre_script/post_script as the default and let forge.github override them, or (b) make detectForgePlatform default to "github" when no CI env is detected, or (c) emit a warning when forge blocks exist but no platform was selected.

Low

• [process] N/A — The PR body references "ADR-0045 Phase 2 PR 2" but docs/plans/adr-0045-forge-portable-harness-schema.md defines "PR 2" as "Role and slug fields" (already implemented), while this PR's content matches Phase 2 Adopt scope ("Move GitHub-specific fields into forge.github blocks in scaffold templates"). The referenced plan file docs/plans/adr-0045-forge-portable-harness-phase2.md does not exist.
  Remediation: Update PR description to reference the correct plan document and clarify this implements Phase 2 Adopt scope, not PR 2.

• [scope-alignment] N/A — The PR title says "move GitHub-specific fields into forge.github blocks" which matches Phase 2 Adopt scope in the plan, but the PR body references "Phase 2 PR 2" which in the plan refers to "Role and slug fields". The scope confusion is a documentation issue.

• [comment-removal] internal/scaffold/fullsend-repo/harness/code.yaml — The diff removes the comment block "Environment variables available to post_script on the runner. These are expanded from the runner environment and NEVER enter the sandbox." This comment documents a security boundary for runner_env.
  Remediation: Consider preserving or relocating this security-boundary comment.

Info

• [architectural-coherence] internal/scaffold/fullsend-repo/harness/*.yaml — Variable names like GITHUB_ISSUE_URL and GITHUB_PR_URL contain "GITHUB" despite being in forge-specific blocks. ADR-0045 documents this as the intended pattern.

• [implementation-completeness] — The scaffold templates now use forge.github blocks, but Phase 2 adoption plan also includes: (1) update fullsend install to write role/slug, (2) generate thin base: wrappers. This PR only addresses the forge.github migration portion.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 7:48 PM UTC · Completed 8:02 PM UTC
Commit: 3e39880 · View workflow run →

[ralphbean]

LGTM.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 1:02 PM UTC · Completed 1:14 PM UTC
Commit: 3483fbc · View workflow run →

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 2:08 PM UTC · Completed 2:15 PM UTC
Commit: 3483fbc · View workflow run →

[fullsend-ai-retro]

Retro: PR #2260 — feat(harness): move GitHub-specific fields into forge.github blocks

Timeline

1. June 12, 19:23 UTC — PR created by ggallen (human-authored, single commit on feat/adr-0045-phase2-pr2-forge-github)
2. June 12, 19:23–19:38 — Review run 1 on commit 47f82d3 (~15 min). Found a real medium-severity bug: pre_script/post_script were only inside forge.github with no top-level fallback, breaking local dev.
3. June 12, 19:45–20:02 — Review run 2 on commit 3e39880 (~18 min). Author had pushed a fix adding top-level defaults back.
4. June 12, 20:31 — Human approval by ralphbean ("LGTM").
5. June 15, 12:58–13:14 — Review run 3 on commit 3483fbc (~16 min). PR was already human-approved; this run was unnecessary.
6. June 15, 14:03 — PR merged.

Assessment

Review run 1 delivered value — it caught a real regression where top-level pre/post scripts were missing. The author fixed it before run 2.

Review run 3 was pure waste (~16 min of agent time) — the PR was already human-approved on June 12, and the final commit 3483fbc had already been reviewed. This is a known issue tracked by #963.

The final medium finding was a false positive. The review flagged identical pre_script/post_script values at both top level and in forge.github as unnecessary duplication. However, the PR body explicitly explains this is intentional design: scripts are kept at the top level as defaults for local dev without --forge, and duplicated in forge.github for forge resolution. The review agent read ADR-0045 but did not weigh the author's stated rationale. This pattern is covered by #1273 (better incorporate PR description context).

Proposals

No new proposals — all identified improvement opportunities are already tracked by existing open issues:

• Redundant review after human approval: #963
• PR description context not incorporated into findings: #1273
• Review deduplication on rapid pushes: #1418, #1422