feat(harness): wire ResolveForge into load pipeline (ADR-0045 PR 3/7)

[ggallen]

Summary

• Add LoadWithOpts(path, LoadOpts) and LoadRaw(path) to the harness package for forge-aware loading and raw unmarshaling
• Add --forge flag to fullsend run and fullsend lock with auto-detection from CI env vars (GITHUB_ACTIONS, GITLAB_CI)
• Wire LoadWithOpts into the run pipeline (replaces Load) so forge overrides are applied before validation
• Lock command iterates all forge variants when --forge is omitted, locking the union of dependencies across platforms
• Log Role and Slug in run output when present

Context

This is PR 3 of 7 for ADR-0045 (forge-portable harness schema). It depends on PR 1 (ForgeConfig struct, #2127) and PR 2 (role/slug fields, #2128), both merged.

Pipeline ordering

LoadWithOpts runs: Unmarshal → ResolveForge → Validate. This ordering is required because Validate() would reject sentinel zero-value structs (e.g., empty validation_loop) that ResolveForge needs to process first.

LoadRaw provides unmarshal-only loading for two consumers:

• LoadWithBase (PR 4) — loads base harnesses without consuming their forge maps before merging
• lockForgePlatforms — discovers forge keys without resolving them, enabling multi-forge locking

Forge detection precedence

detectForgePlatform: explicit --forge flag → GITHUB_ACTIONS=true → GITLAB_CI=true → empty (skip)

Test plan

• LoadWithOpts applies forge overrides before validation
• LoadWithOpts with empty platform preserves forge map (no-op)
• LoadWithOpts rejects invalid/unconfigured platforms
• LoadRaw returns raw harness without validation (missing agent field OK)
• LoadRaw preserves forge map
• detectForgePlatform flag overrides env vars
• detectForgePlatform auto-detects from GITHUB_ACTIONS and GITLAB_CI
• --forge flag registered on both run and lock commands
• All existing lock tests pass with new forgeFlag parameter
• make go-test — all tests pass
• make go-vet — clean
• make lint — passes

🤖 Generated with Claude Code

[github-actions]

Site preview

Preview: https://59f9555e-site.fullsend-ai.workers.dev

Commit: 072653961469697d0dbb0dd8867332906488d7d6

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 9:01 PM UTC · Completed 9:13 PM UTC
Commit: 2011827 · View workflow run →

[fullsend-ai-review]

Review

Findings

Low

• [edge-case] internal/cli/lock.go:237 — When --forge specifies a valid platform but the harness has no forge: section, lockForgePlatforms bypasses LoadRaw and returns the requested platform directly. ResolveForge is then a no-op, so the base harness is locked silently with no indication that --forge had no effect.
  Remediation: Consider emitting a warning when --forge is passed but the harness has no forge section.

• [error-message-consistency] internal/harness/forge.go:31 — Error message format changed from valid keys are: github, gitlab to valid: github, gitlab in both validateForge and ResolveForge. The new format uses ForgeKeyList() to avoid hardcoding platform names, which is an improvement. Internally consistent across all call sites in this PR.

• [code-organization] internal/cli/lock.go:190 — Two-step load pattern: LoadRaw (to discover forge keys) then LoadWithOpts (to resolve each variant). This is inherent to the multi-forge locking design and the lockForgePlatforms doc comment already explains the pattern.

• [scope-coherence] internal/cli/lock.go — The multi-forge iteration behavior is substantial but explicitly authorized by the ADR-0045 implementation plan (PR 3 scope).

• [naming-consistency] internal/harness/harness.go — LoadRaw is exported despite the implementation plan specifying unexported loadRaw. The export is necessary because lockForgePlatforms in internal/cli (a different package) calls it.

• [helper-function-naming] internal/harness/forge.go:29 — ForgeKeyList() (exported, lists all valid platform names) overlaps in name with existing forgeKeyList(m) (private, lists keys from a specific forge map). Different semantics justify separate functions.

• [error-message-consistency] internal/cli/run.go:707 — Error format --forge: %q is not a valid forge platform uses colon-after-flag-name pattern. Consistent within this PR (same format in lock.go).

Info

• [logic-error] internal/cli/lock.go:130 — Dependency deduplication by URL keeps only the first-encountered Field name for cross-variant duplicates. Fully mitigated at runtime: resolveFromLock resolves by URL, not field name.

• [edge-case] internal/cli/run.go:1683 — detectForgePlatform auto-detection returns a platform from CI env vars even when the harness has no forge: section; ResolveForge is a no-op in that case. Intentional per ADR-0045.

• [prior-finding-resolved] internal/cli/lock.go — Prior medium-severity finding (staleness check running after forge resolution) has been resolved. The refactored code computes the harness hash and checks staleness BEFORE entering the forge variant loop.

• [prior-finding-resolved] internal/cli/lock_test.go — Prior low-severity finding about zero test coverage for forge-loop logic has been addressed. Three new tests cover multi-variant union, single-variant selection, and cross-variant deduplication.

• [stale-api-reference] docs/plans/universal-harness-access.md — Planning document references harness.Load() which has been supplemented by LoadWithOpts(). Load() still exists; planning documents capture state at time of writing.

• [stale-error-message] docs/plans/adr-0045-forge-portable-harness-schema.md:61 — ADR describes error format as valid keys are: github, gitlab while the implementation uses valid: <key-list> via ForgeKeyList(). Cosmetic divergence in a historical decision record.

Previous run

Review

Findings

Low

• [missing-authorization] N/A — This PR has no linked issue. The PR references ADR-0045 (accepted) and identifies itself as PR 3/7 in a planned implementation sequence, which provides architectural authorization. However, a linked issue would improve traceability.
  Remediation: Create and link a tracking issue, or reference the ADR-0045 implementation tracking issue.

• [edge-case] internal/cli/lock.go:237 — When --forge specifies a valid platform but the harness has no forge: section, lockForgePlatforms bypasses LoadRaw and returns the requested platform directly. ResolveForge is then a no-op, so the base harness is locked silently with no indication that --forge had no effect. Contrast with the case where the harness HAS a forge section but lacks the requested platform — that produces an error.
  Remediation: Consider logging a warning when --forge is specified but the harness has no forge: section.

• [stale-api-reference] docs/plans/universal-harness-access.md:1108 — Planning document references harness.Load() which has been supplemented by LoadWithOpts(). Since this is a planning document (not user-facing docs) and Load() still exists in the codebase, this is cosmetic rather than misleading.
  Remediation: Update the code snippet in the planning doc when the multi-PR series completes.

Info

• [logic-error] internal/cli/lock.go:130 — Dependency deduplication by URL keeps only the first-encountered Field name for cross-variant duplicates. Fully mitigated at runtime: lock-based resolution with --forge re-runs LoadWithOpts for the specific platform, producing correct skill indices.

• [stale-error-message] docs/plans/adr-0045-forge-portable-harness-schema.md:61 — ADR describes error format as valid keys are: github, gitlab while the implementation uses valid: <key-list> via ForgeKeyList(). ADRs are historical decision records — cosmetic divergence from implementation is expected.

• [prior-finding-resolved] internal/cli/lock.go — Prior medium-severity finding (staleness check running after forge resolution) has been resolved. The refactored code computes the harness hash and checks staleness BEFORE entering the forge variant loop.

• [prior-finding-resolved] internal/cli/lock_test.go — Prior low-severity finding about zero test coverage for forge-loop logic has been addressed. Three new tests cover multi-variant union, single-variant selection, and cross-variant deduplication.

• [edge-case] internal/cli/run.go — detectForgePlatform auto-detection returns a platform from CI env vars even when the harness has no forge: section; ResolveForge is a no-op in that case. Intentional per ADR-0045: missing forge: means platform-agnostic.

Previous run (2)

Review

Findings

Low

• [missing-authorization] N/A — This PR has no linked issue. The PR references ADR-0045 (accepted) and identifies itself as PR 3/7 in a planned implementation sequence, which provides architectural authorization. However, a linked issue would improve traceability.
  Remediation: Create and link a tracking issue, or reference the ADR-0045 implementation tracking issue.

• [error-message-format] internal/cli/lock.go:128 — Error message format inconsistency: the CLI layer (lock.go, run.go) uses (valid: %s) while the harness package (forge.go) uses (valid keys are: %s). Both use ForgeKeyList() but with different surrounding text.
  Remediation: Standardize to (valid keys are: %s) across CLI and harness packages.

• [logic-error] internal/cli/lock.go:130 — Dependency deduplication by URL loses field-name information for cross-variant duplicates. When the same URL appears in two forge variants at different skill indices, only the first-encountered Field name is kept. Lock integrity (URL + SHA256) is unaffected.

• [edge-case] internal/cli/lock.go:237 — When --forge specifies a valid platform but the harness has no forge: section, lockForgePlatforms bypasses LoadRaw and returns the requested platform directly. ResolveForge is then a no-op, so the base harness is locked silently with no indication that --forge had no effect. Contrast with the case where the harness HAS a forge section but lacks the requested platform — that produces an error.

Info

• [prior-finding-resolved] internal/cli/lock.go — Prior medium-severity finding (staleness check running after forge resolution) has been resolved. The refactored code computes the harness hash and checks staleness BEFORE entering the forge variant loop.

• [prior-finding-resolved] internal/cli/lock_test.go — Prior low-severity finding about zero test coverage for forge-loop logic has been addressed. Three new tests cover multi-variant union, single-variant selection, and cross-variant deduplication.

• [scope-coherence] internal/cli/run.go:654 — detectForgePlatform implements forge detection precedence (flag > GITHUB_ACTIONS > GITLAB_CI > empty) which resolves an ADR-0045 open question without updating the ADR.

• [scope-alignment] internal/cli/lock.go — The lock command's multi-forge iteration logic resolves all forge variants when --forge is omitted, producing a union of dependencies. Architecturally coherent with ADR-0045.

• [naming-alignment] internal/harness/harness.go:240 — LoadRaw is exported with a doc comment describing both current and planned usage (base composition in PR 4). Acceptable for a planned API in a multi-PR series.

Previous run (3)

Review

Findings

Medium

• [edge-case] internal/cli/lock.go:188 — The staleness check (load existing lock file, compare harness hash, skip if current) now runs AFTER all forge variants have been resolved via network/cache. In the original code, the staleness check ran before resolution, providing an efficient early exit. With the new forge loop preceding the hash check, fullsend lock always resolves all forge variants even when the lock entry is already up to date, defeating the staleness optimization for the common case.
  Remediation: Move the harness hash computation and lock staleness check before the forge-variant loop so that the early exit still avoids unnecessary resolution work.

Low

• [logic-error] internal/cli/lock.go:100 — Dependency deduplication by URL loses field-name information for cross-variant duplicates. When the same URL appears in two forge variants at different skill indices, only the first-encountered Field name is kept in the lock file. In practice this is unlikely since base skills share indices and forge-specific skills have distinct URLs, but it could cause incorrect slot patching in contrived configurations.

• [test-inadequate] internal/cli/lock_test.go — The new forge-loop logic in runLock is the most complex part of this PR but has zero dedicated test coverage. Existing lock tests are updated only to add the empty-string forgeFlag parameter. No tests exercise multi-forge-variant resolution, deduplication across variants, or --forge selecting a single variant.

• [validation-gap] internal/cli/lock.go:237 — lockForgePlatforms calls LoadRaw to discover forge keys when --forge is omitted. Unrecognized forge keys (e.g., bitbucket) pass through to LoadWithOpts which catches them at validateForge. The error is surfaced, but early validation inside lockForgePlatforms would produce a clearer error path.

Previous run (4)

Review

Findings

High

• [logic-error] internal/cli/lock.go:75 — The lock command calls detectForgePlatform(forgeFlag) which auto-detects the forge platform from CI environment variables (GITHUB_ACTIONS, GITLAB_CI) when --forge is omitted. This contradicts the documented behavior ("omit to lock all forge variants") and the ADR-0045 implementation plan. When fullsend lock runs in GitHub Actions CI without --forge, detectForgePlatform("") returns "github", so lockForgePlatforms receives "github" and only locks that single variant instead of all forge variants. The lock file will be missing dependencies from other forge variants, causing fullsend run to fail on those platforms.
  Remediation: Do not call detectForgePlatform in runLock. Pass forgeFlag directly to lockForgePlatforms without auto-detection. Auto-detection is appropriate for run (where you need to pick one platform) but not for lock (where omission means "lock all").

Medium

• [validation-gap] internal/harness/harness.go:249 — LoadWithOpts runs ResolveForge before Validate. ResolveForge sets h.Forge = nil after merging, so when Validate calls validateForge, the forge map is nil and validation is skipped entirely. Malformed forge configs for non-selected platforms (e.g., unrecognized forge keys like "bitbuckt" from a typo) are silently accepted.
  Remediation: Call h.validateForge() between LoadRaw and ResolveForge in LoadWithOpts to validate the structure of all forge entries before consuming the map.

• [stale-doc] docs/guides/user/running-agents-locally.md:187 — User guide documents remote resource flags for fullsend run and fullsend lock but does not mention the new --forge flag.
  Remediation: Add --forge flag documentation to the flags section.

• [stale-doc] docs/guides/dev/cli-internals.md — CLI command tree documents fullsend run and fullsend lock flags but does not include the new --forge flag for either command.
  Remediation: Add --forge <platform> to both the run and lock flag lists in the CLI command tree.

• [naming-inconsistency] internal/cli/run.go — detectForgePlatform error message hardcodes "valid: github, gitlab" instead of deriving the list from the source of truth (validForgeKeys map). If a new forge platform is added, the error message will be stale.
  Remediation: Export a ForgeKeyList() function from internal/harness/forge.go and use it in the error message.

• [architectural-misalignment] internal/harness/harness.go:547 — LoadRaw is exported but the ADR-0045 implementation plan specifies "Add unexported loadRaw(path string)". This breaks the planned encapsulation. The LoadRaw godoc also references "PR 4" which is unusual in production code (existing comments reference ADRs or issue numbers, not PR numbers).
  Remediation: Rename to loadRaw (unexported) per the plan, or update the plan to reflect exported LoadRaw. Remove the "(PR 4)" reference from the godoc.

Low

• [edge-case] internal/cli/lock.go:125 — loadOrgConfig is called inside the forge-variant loop, reading and parsing the org config YAML from disk on every iteration. Functionally correct but wasteful when iterating multiple forge variants.

• [test-inadequate] internal/cli/run_test.go — No test for the case where both GITHUB_ACTIONS=true and GITLAB_CI=true are set simultaneously. The code's precedence (GitHub first) is implicit and should be documented via a test.

• [printer-usage] internal/cli/lock.go:83 — StepFail for early validation error (determining forge variants) is inconsistent with some existing patterns where early validation errors return without a StepFail call, though the codebase is mixed on this.

• [parameter-ordering] internal/cli/lock.go:51 — forgeFlag parameter placed between fullsendDir and update in the runLock signature. Minor style point — groups string parameters together before the bool.

Previous run (5)

Review

Findings

Low

• [missing-authorization] N/A — This PR has no linked issue. The PR references ADR-0045 (accepted) and identifies itself as PR 3/7 in a planned implementation sequence, which provides architectural authorization. However, a linked issue would improve traceability.
  Remediation: Create and link a tracking issue, or reference the ADR-0045 implementation tracking issue.

• [error-message-format] internal/cli/lock.go:128 — Error message format inconsistency: the CLI layer (lock.go, run.go) uses (valid: %s) while the harness package (forge.go) uses (valid keys are: %s). Both use ForgeKeyList() but with different surrounding text.
  Remediation: Standardize to (valid keys are: %s) across CLI and harness packages.

• [logic-error] internal/cli/lock.go:130 — Dependency deduplication by URL loses field-name information for cross-variant duplicates. When the same URL appears in two forge variants at different skill indices, only the first-encountered Field name is kept. Lock integrity (URL + SHA256) is unaffected.

• [edge-case] internal/cli/lock.go:237 — When --forge specifies a valid platform but the harness has no forge: section, lockForgePlatforms bypasses LoadRaw and returns the requested platform directly. ResolveForge is then a no-op, so the base harness is locked silently with no indication that --forge had no effect. Contrast with the case where the harness HAS a forge section but lacks the requested platform — that produces an error.

Info

• [prior-finding-resolved] internal/cli/lock.go — Prior medium-severity finding (staleness check running after forge resolution) has been resolved. The refactored code computes the harness hash and checks staleness BEFORE entering the forge variant loop.

• [prior-finding-resolved] internal/cli/lock_test.go — Prior low-severity finding about zero test coverage for forge-loop logic has been addressed. Three new tests cover multi-variant union, single-variant selection, and cross-variant deduplication.

• [scope-coherence] internal/cli/run.go:654 — detectForgePlatform implements forge detection precedence (flag > GITHUB_ACTIONS > GITLAB_CI > empty) which resolves an ADR-0045 open question without updating the ADR.

• [scope-alignment] internal/cli/lock.go — The lock command's multi-forge iteration logic resolves all forge variants when --forge is omitted, producing a union of dependencies. Architecturally coherent with ADR-0045.

• [naming-alignment] internal/harness/harness.go:240 — LoadRaw is exported with a doc comment describing both current and planned usage (base composition in PR 4). Acceptable for a planned API in a multi-PR series.

Previous run (6)

Review

Findings

Medium

• [edge-case] internal/cli/lock.go:188 — The staleness check (load existing lock file, compare harness hash, skip if current) now runs AFTER all forge variants have been resolved via network/cache. In the original code, the staleness check ran before resolution, providing an efficient early exit. With the new forge loop preceding the hash check, fullsend lock always resolves all forge variants even when the lock entry is already up to date, defeating the staleness optimization for the common case.
  Remediation: Move the harness hash computation and lock staleness check before the forge-variant loop so that the early exit still avoids unnecessary resolution work.

Low

• [logic-error] internal/cli/lock.go:100 — Dependency deduplication by URL loses field-name information for cross-variant duplicates. When the same URL appears in two forge variants at different skill indices, only the first-encountered Field name is kept in the lock file. In practice this is unlikely since base skills share indices and forge-specific skills have distinct URLs, but it could cause incorrect slot patching in contrived configurations.

• [test-inadequate] internal/cli/lock_test.go — The new forge-loop logic in runLock is the most complex part of this PR but has zero dedicated test coverage. Existing lock tests are updated only to add the empty-string forgeFlag parameter. No tests exercise multi-forge-variant resolution, deduplication across variants, or --forge selecting a single variant.

• [validation-gap] internal/cli/lock.go:237 — lockForgePlatforms calls LoadRaw to discover forge keys when --forge is omitted. Unrecognized forge keys (e.g., bitbucket) pass through to LoadWithOpts which catches them at validateForge. The error is surfaced, but early validation inside lockForgePlatforms would produce a clearer error path.

Previous run (7)

Review

Findings

High

• [logic-error] internal/cli/lock.go:75 — The lock command calls detectForgePlatform(forgeFlag) which auto-detects the forge platform from CI environment variables (GITHUB_ACTIONS, GITLAB_CI) when --forge is omitted. This contradicts the documented behavior ("omit to lock all forge variants") and the ADR-0045 implementation plan. When fullsend lock runs in GitHub Actions CI without --forge, detectForgePlatform("") returns "github", so lockForgePlatforms receives "github" and only locks that single variant instead of all forge variants. The lock file will be missing dependencies from other forge variants, causing fullsend run to fail on those platforms.
  Remediation: Do not call detectForgePlatform in runLock. Pass forgeFlag directly to lockForgePlatforms without auto-detection. Auto-detection is appropriate for run (where you need to pick one platform) but not for lock (where omission means "lock all").

Medium

• [validation-gap] internal/harness/harness.go:249 — LoadWithOpts runs ResolveForge before Validate. ResolveForge sets h.Forge = nil after merging, so when Validate calls validateForge, the forge map is nil and validation is skipped entirely. Malformed forge configs for non-selected platforms (e.g., unrecognized forge keys like "bitbuckt" from a typo) are silently accepted.
  Remediation: Call h.validateForge() between LoadRaw and ResolveForge in LoadWithOpts to validate the structure of all forge entries before consuming the map.

• [stale-doc] docs/guides/user/running-agents-locally.md:187 — User guide documents remote resource flags for fullsend run and fullsend lock but does not mention the new --forge flag.
  Remediation: Add --forge flag documentation to the flags section.

• [stale-doc] docs/guides/dev/cli-internals.md — CLI command tree documents fullsend run and fullsend lock flags but does not include the new --forge flag for either command.
  Remediation: Add --forge <platform> to both the run and lock flag lists in the CLI command tree.

• [naming-inconsistency] internal/cli/run.go — detectForgePlatform error message hardcodes "valid: github, gitlab" instead of deriving the list from the source of truth (validForgeKeys map). If a new forge platform is added, the error message will be stale.
  Remediation: Export a ForgeKeyList() function from internal/harness/forge.go and use it in the error message.

• [architectural-misalignment] internal/harness/harness.go:547 — LoadRaw is exported but the ADR-0045 implementation plan specifies "Add unexported loadRaw(path string)". This breaks the planned encapsulation. The LoadRaw godoc also references "PR 4" which is unusual in production code (existing comments reference ADRs or issue numbers, not PR numbers).
  Remediation: Rename to loadRaw (unexported) per the plan, or update the plan to reflect exported LoadRaw. Remove the "(PR 4)" reference from the godoc.

Low

• [edge-case] internal/cli/lock.go:125 — loadOrgConfig is called inside the forge-variant loop, reading and parsing the org config YAML from disk on every iteration. Functionally correct but wasteful when iterating multiple forge variants.

• [test-inadequate] internal/cli/run_test.go — No test for the case where both GITHUB_ACTIONS=true and GITLAB_CI=true are set simultaneously. The code's precedence (GitHub first) is implicit and should be documented via a test.

• [printer-usage] internal/cli/lock.go:83 — StepFail for early validation error (determining forge variants) is inconsistent with some existing patterns where early validation errors return without a StepFail call, though the codebase is mixed on this.

• [parameter-ordering] internal/cli/lock.go:51 — forgeFlag parameter placed between fullsendDir and update in the runLock signature. Minor style point — groups string parameters together before the bool.

[fullsend-ai-review]

See the review comment for full details.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 9:40 PM UTC · Completed 9:51 PM UTC
Commit: 54daebf · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 9:59 PM UTC · Completed 10:09 PM UTC
Commit: 83956d5 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 10:16 PM UTC · Completed 10:28 PM UTC
Commit: 534c210 · View workflow run →

[github-actions]

E2E tests did not run

E2E tests run automatically for org/repo members and collaborators on pull requests.

For other contributors, a maintainer must add the ok-to-test label after the latest push.

See E2E testing guide for details.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 11:07 AM UTC · Completed 11:18 AM UTC
Commit: 0726539 · View workflow run →

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 2:52 PM UTC · Completed 2:58 PM UTC
Commit: 0726539 · View workflow run →

[fullsend-ai-retro]

Retro: PR #2142 — feat(harness): wire ResolveForge into load pipeline

Overall assessment: The workflow performed well. The review agent caught a genuine high-severity logic error on the first pass (auto-detection in lock conflicting with documented multi-variant behavior), which the author fixed promptly. The PR went through 5 review iterations over ~14 hours before merging, with a human approval from rh-hemartin.

Main inefficiency — repeated findings (already tracked): The same low-severity edge-case finding ("--forge with no forge section is a silent no-op") was raised in 3 separate review iterations. The author acknowledged and dismissed it each time, explaining it was intentional behavior. The review agent even correctly tagged some findings as [prior-finding-resolved], showing partial awareness of prior iterations — but it failed to suppress findings the author had explicitly dismissed. This is directly covered by existing issues:

• #1672 — Review agent should honor explicit author dismissals of findings
• #1583 — Review agent should recognize human-resolved findings and stop re-flagging
• #1013 — Review agent should deduplicate findings across iterations

Redundant review runs (already tracked): Reviews 3-5 appear to have been triggered by comment activity or non-code events rather than meaningful code changes. Covered by #893 and #1418.

No new proposals — all identified improvements are already tracked by open issues.