test: Phase 2 integration tests for ADR-0045 forge-portable harness

[ggallen]

Closes #2328

Summary

• Adds internal/harness/phase2_integration_test.go with 8 end-to-end integration tests verifying the full Phase 2 pipeline for ADR-0045
• Tests cover: wrapper → base composition → forge resolution, all scaffold templates through forge.github resolution, backward compatibility without forge flag, DiscoverAgents on scaffold directory, base URL integrity (SHA-256 hash verification), generated wrapper format parsing, and per-template forge runner_env merge detail

Context

This is PR 6 of the ADR-0045 Phase 2 implementation plan — the final PR that provides end-to-end verification after PRs 1–5 have merged the implementation.

Test plan

• make go-test — all new and existing tests pass
• make go-vet — clean
• make lint — passes

🤖 Generated with Claude Code

[github-actions]

Site preview

Preview: https://10bdf22e-site.fullsend-ai.workers.dev

Commit: 61f467ddb4978310abc9e24fd549b8563c301106

[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 11:41 AM UTC · Completed 11:53 AM UTC
Commit: f9caf81 · View workflow run →

[fullsend-ai-review]

Looks good to me

Previous run

Review

Findings

Low

• [file-naming-convention] internal/harness/phase2_integration_test.go — The filename phase2_integration_test.go uses a phase-based naming scheme unique in this package. Other test files are named after the subject they test (e.g., harness_test.go, forge_test.go, compose_test.go, discover_test.go). Consider renaming to a subject-based name like scaffold_integration_test.go to align with the established pattern, unless the phase designation is intentionally temporary.

Info

• [authorization] internal/harness/phase2_integration_test.go — PR is properly authorized by Issue ADR-0045 Phase 2 PR 6: integration tests and end-to-end verification #2328 and aligned with the ADR-0045 Phase 2 implementation plan (PR 6 of 6).

• [architectural-coherence] internal/harness/phase2_integration_test.go — The test file placement follows established patterns in the harness package, grouping Phase 2-specific end-to-end tests separately from the ongoing integration_test.go.

• [test-coverage-alignment] internal/harness/phase2_integration_test.go — The 8 tests in the PR cover the deliverables defined in the implementation plan with appropriate division of test responsibility.

• [infrastructure-readiness] internal/harness/phase2_integration_test.go — PRs 1–5 infrastructure is confirmed present: DiscoverAgents, HarnessBaseURL, HarnessContentHash, LoadWithBase, ResolveForge all exist.

Previous run (2)

Review

Findings

Low

• [test-naming-convention] internal/harness/phase2_integration_test.go — Test function names use TestPhase2_ prefix, which diverges from the established pattern of TestFunctionName_Scenario used throughout this package (e.g., TestLoadWithBase_BackwardCompat, TestResolveForge_ScalarOverride, TestDiscoverAgents). The file-naming aspect is weaker — integration_test.go already exists as a non-subject-based filename — but the function naming divergence is clear.

Info

• [helper-function-naming] internal/harness/phase2_integration_test.go:19 — The helper function extractScaffoldHarnessDir follows the established camelCase pattern for helper functions and includes t.Helper(), consistent with the codebase.

Previous run (3)

Review

Findings

Low

• [missing-authorization] N/A — This PR has no linked issue. The work is clearly authorized by the ADR-0045 Phase 2 implementation plan (which explicitly enumerates PR 6 with this exact file and test scenarios), but linking an issue is good practice for traceability.

• [test-naming-convention] internal/harness/phase2_integration_test.go — Test function names use TestPhase2_ prefix and the file uses a phase2_ prefix, which is inconsistent with existing test naming patterns (e.g., TestLoadWithBase_, TestResolveForge_). The prefix is defensible as a grouping mechanism for cross-cutting integration tests, but the project has no precedent for phase-prefixed test names.

• [helper-function-naming] internal/harness/phase2_integration_test.go:19 — The helper function extractScaffold returns the harness subdirectory path (not the extraction root), which the name doesn't make clear. Consider extractScaffoldHarnessDir for clarity.

Info

• [file-mode-consistency] internal/harness/phase2_integration_test.go:24 — Uses modern 0o755/0o644 octal literals consistently, which is the recommended Go style. The existing codebase is mixed (some files use 0755). No action needed.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 12:22 PM UTC · Completed 12:30 PM UTC
Commit: 087e6ef · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 12:45 PM UTC · Completed 12:57 PM UTC
Commit: 4f0de7b · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 2:00 PM UTC · Completed 2:10 PM UTC
Commit: 61f467d · View workflow run →

[ralphbean]

LGTM.

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 7:16 PM UTC · Completed 7:28 PM UTC
Commit: 61f467d · View workflow run →

[fullsend-ai-retro]

Retro: PR #2321 — Phase 2 integration tests for ADR-0045

Overall assessment: Workflow was healthy. The review agent behaved well — correct severity calibration (all low/info, never blocking), proper incremental review scoping, and appropriate approvals throughout. The human author addressed feedback promptly and the PR merged cleanly.

Timeline

• 4 review runs across 4 commits over ~2.5 hours of active iteration
• Run 1: 3 low findings (missing issue link, TestPhase2_ naming, helper naming)
• Run 2: 1 low remaining after human fixes
• Run 3: 1 NEW low finding (file-naming-convention) surfaced for the first time
• Run 4: Clean LGTM
• Human reviewer approved ~2.5h later; merged ~7.5h after PR creation

One improvement area identified (already tracked)

The review agent found TestPhase2_ function naming on run 1 but didn't flag the related phase2_integration_test.go filename until run 3 — after the human had already fixed the function names. These are logically related naming-convention findings that should have been surfaced together, saving one rework cycle.

This pattern is already tracked by #1582 ("Review agent should catch all findings in the first pass to reduce rework cycles") and partially by #2332 ("Review agent should respect tool-specific filename conventions"). No new proposal needed.

No new proposals

All improvement opportunities identified are covered by existing open issues. The review agent's behavior was appropriate for a test-only PR — findings were useful, severity was calibrated correctly, and the agent never unnecessarily blocked progress.