feat(harness): add ForgeConfig struct and merge logic (ADR-0045 PR 1/7)

[ggallen]

Summary

• Adds ForgeConfig struct with PreScript, PostScript, Skills, ValidationLoop, and RunnerEnv fields for platform-specific harness configuration
• Adds Forge map[string]*ForgeConfig field to Harness struct with YAML support
• Implements ResolveForge(platform) merge method following ADR-0045 inheritance rules: scalars override, skills concatenate, runner_env merges (forge wins), validation_loop replaces entirely
• Adds validateForge() to reject unrecognized forge keys and URL-based scripts in forge blocks
• Pure library code — no callers in the load pipeline yet (wired in PR 3)

Phase 1 PR 1 of 7 for the ADR-0045 implementation plan.

Test plan

• 20 new tests in forge_test.go covering all merge behaviors, validation, YAML round-trip, edge cases
• All existing harness tests pass unchanged (backward compatibility)
• Scaffold TestHarnessesLoadAndValidate passes (no regression)
• make go-vet clean
• make lint clean

🤖 Generated with Claude Code

[github-actions]

Site preview

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

Commit: 00ede605d2d7ad079973c0f9a07779b2af4d14fc

[fullsend-ai-review]

🤖 Review · Started 4:02 PM UTC
Commit: 4ed6da4 · View workflow run →

[ggallen]

/fs-review

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 4:55 PM UTC · Completed 5:09 PM UTC
Commit: 4ed6da4 · View workflow run →

[fullsend-ai-review]

Review

Findings

Medium

• [path-traversal] internal/harness/forge.go:71 — mergeForgeConfig copies forge PreScript, PostScript, Skills, and ValidationLoop.Script into top-level Harness fields. ResolveRelativeTo (which enforces directory traversal protection) does not cover forge paths before the merge. If the planned pipeline ordering is violated and ResolveForge runs after ResolveRelativeTo, the merged paths bypass traversal validation. The documented pipeline ordering (Unmarshal → ResolveForge → Validate → ResolveRelativeTo) runs ResolveRelativeTo on the already-merged top-level fields, which is correct — but this ordering is not enforced programmatically.
  Remediation: Either add traversal validation for local paths directly inside validateForge, or enforce call ordering with a state flag (e.g., forgeResolved bool) that ResolveRelativeTo checks.

Low

• [missing-handler] internal/harness/harness.go:332 — ResolveRelativeTo and ValidateFilesExist do not iterate over h.Forge entries to resolve/validate forge-level paths. By design, ResolveForge merges forge paths into top-level fields before these methods run. This is correct for the planned pipeline ordering but creates an implicit ordering dependency that should be documented when PR 3 wires the pipeline.

• [environment-variable-injection] internal/harness/forge.go:78 — mergeForgeConfig merges forge RunnerEnv with forge values winning on key conflict. No denylist prevents overriding security-sensitive env vars (e.g., PATH, LD_PRELOAD). Consistent with existing top-level RunnerEnv behavior — not a new vulnerability, but a defense-in-depth opportunity.

• [incomplete-schema-documentation] docs/ADRs/0024-harness-definitions.md:307 — ADR-0024 documents the harness YAML schema but does not cross-reference ADR-0045 for the new forge: section. Appropriately deferred to PR 5 when the feature is fully integrated.

• [incomplete-example] docs/guides/user/customizing-agents.md:11 — User guide harness examples do not mention the forge: section. Should be addressed in PR 3 or PR 5 when the feature is user-facing.

Info

• [implementation-ordering-clarification] internal/harness/harness.go — The validateForge() call inside Validate() will see h.Forge = nil after ResolveForge runs (PR 3). This interaction is correct by design (the nil-map guard makes it a no-op), but an inline comment at the call site would help future maintainers understand the temporal dependency.

• [cross-reference-opportunity] docs/guides/dev/cli-internals.md:34 — CLI docs will need updating once PR 3 adds the --forge flag. No action needed for PR 1.

• [prior-finding-resolved] internal/harness/forge.go:125 — Prior finding (forgeKeyList panic on empty map) is resolved. The function now uses strings.Join(keys, ", ") instead of direct indexing.

• [prior-finding-resolved] internal/harness/forge.go:44 — Prior finding (forge Skills URL without integrity hash not validated) is resolved. validateForge now checks each fc.Skills URL entry for #sha256=... integrity hash.

• [prior-finding-resolved] internal/harness/forge.go:76 — Prior finding (ResolveForge does not validate platform against validForgeKeys) is resolved. Lines 76-78 now check validForgeKeys[platform] and return an error for unrecognized platforms.

Previous run

Review

Findings

Medium

• [logic-error] internal/harness/forge.go:101 — forgeKeyList assumes the map has at most 2 entries and uses direct indexing (keys[0], keys[1]) without bounds checking. If h.Forge is an empty non-nil map (reachable via YAML unmarshaling of forge: {}), keys[0] will panic with index out of range. If the map has 3+ entries, keys after the second are silently dropped. While the 3+ case is blocked if validateForge runs first (only 2 valid keys), the empty-map panic is a real defect.
  Remediation: Use strings.Join(keys, ", ") instead of manual concatenation.

• [implementation-ordering] internal/harness/harness.go — The ADR-0045 implementation plan specifies ResolveForge must run before Validate(). In this PR, validateForge() is called inside Validate(). When PR 3 wires ResolveForge before Validate(), validateForge will see h.Forge = nil (already consumed). This works correctly due to the nil-map guard, but the interaction should be documented to prevent confusion — validateForge validates the pre-merge forge map structure, while ValidateResourceTypes validates the post-merge top-level fields.

• [incomplete-schema-documentation] docs/ADRs/0024-harness-definitions.md:307 — ADR-0024 documents the harness YAML schema but does not cross-reference ADR-0045 for the new forge: section. This can reasonably be deferred to PR 5 (full pipeline integration) when the feature is complete.
  Remediation: Add a note referencing ADR-0045 for forge-portable extensions.

Low

• [missing-handler] internal/harness/harness.go:332 — ResolveRelativeTo and ValidateFilesExist do not iterate over h.Forge entries to resolve/validate forge-level paths. By design (per the implementation plan), ResolveForge merges forge paths into top-level fields before these functions run. This is correct for the planned pipeline ordering but should be documented.

• [validation-gap] internal/harness/forge.go:27 — validateForge does not check forge Skills[] entries for integrity hashes on URL values. ValidateResourceTypes enforces this for top-level skills, and the planned pipeline runs ResolveForge (which merges forge skills into top-level) before Validate(). Defense-in-depth: consider adding the check in validateForge to catch issues regardless of call ordering.

• [validation-gap] internal/harness/forge.go:56 — ResolveForge does not validate the platform parameter against validForgeKeys. It only checks presence in h.Forge. If called with an invalid platform on an unvalidated harness containing that key, it would merge without error. Low risk since the planned pipeline validates first.

• [environment-variable-injection] internal/harness/forge.go:78 — mergeForgeConfig merges forge RunnerEnv with forge values winning on key conflict. No denylist prevents overriding security-sensitive env vars. Consistent with existing top-level RunnerEnv behavior.

• [incomplete-example] docs/guides/user/customizing-agents.md:11 — User guide harness examples do not mention the forge: section. Should be addressed in PR 3 or PR 5 when the feature is wired into the CLI.

Info

• [variable-naming] internal/harness/forge.go:13 — validForgeKeys uses map[string]bool while other validators use regexp. The map approach is idiomatic Go for a finite enumeration of 2 values.

• [cross-reference-opportunity] docs/guides/dev/cli-internals.md:34 — CLI docs will need updating once PR 3 adds the --forge flag. No action needed for PR 1.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 5:17 PM UTC · Completed 5:30 PM UTC
Commit: 4ed6da4 · View workflow run →

[ralphbean]

LGTM.

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 8:13 PM UTC · Completed 8:21 PM UTC
Commit: 00ede60 · View workflow run →

[fullsend-ai-retro]

Retro: PR #2127 — feat(harness): add ForgeConfig struct and merge logic

Human-authored PR (ggallen) adding ForgeConfig struct for ADR-0045. Merged after ~4h with 1 human approval.

Workflow summary

Run	Trigger	Outcome	Notes
Review run 1	Auto (PR open)	❌ Failed	Agent completed (exit 0, schema valid) but post-review.sh got HTTP 401 — token expired. Review results lost.
Review run 2	Manual /fs-review	✅ Success	Re-triggered 50 min later by author. Posted medium-severity path-traversal finding.
Review run 3	Auto (force-push)	✅ Success	Redundant — same findings as run 2 on rebased commit.

Waste: 3 review runs consumed ~35 min of agent compute; only run 2 was productive. Run 1's results were computed but never posted. Run 3 reviewed the same content as run 2.

Existing issues covering observed problems

• Status comment uses minted agent token and fails to update when token expires #2130 — Token expiry causing status comment / post-script failures (root cause of run 1 failure)
• Post-scripts should retry on transient HTTP errors before giving up #1296 — Post-scripts should retry on transient HTTP errors (would have mitigated run 1 failure)
• Deduplicate review dispatches for the same HEAD SHA #1452 / Skip review dispatch when HEAD SHA was already reviewed and approved #963 — Deduplicate review dispatches for same SHA / already-reviewed content (would prevent run 3)
• Review agent should alert when human approves with unresolved Medium+ findings #2099 — Alert when human approves with unresolved Medium+ findings

New proposal

One gap not covered by existing issues: when the agent completes successfully but the post-script fails, the entire agent must be re-run from scratch. Uploading agent output as an artifact before post-script execution would enable post-script-only retries, saving significant tokens and time.

Proposals filed

• Enable post-script-only retry when agent succeeds but post-script fails (in fullsend-ai/fullsend)