docs(plans): implementation plan for ADR-0045 forge-portable harness schema

[ggallen]

Summary

• Adds Phase 1 implementation plan for ADR-0045 (forge-portable harness schema) with 7 PRs covering ForgeConfig struct + merge logic, role/slug fields, CLI --forge flag, base composition, full pipeline integration, scaffold updates, and nil-vs-empty YAML semantics tests
• Documents integration with ADR-0038 (universal harness access): base URL resolution reuses internal/resolve infrastructure, lock file records base as DependencyEntry, pipeline ordering places LoadWithBase before resolve.ResolveHarness
• Outlines future Phases 2-4 (adopt, deprecate, remove agents: block from config.yaml)

Test plan

• make lint passes
• Review PR dependency graph for correctness
• Verify ADR-0038 integration approach (reuse resolve.resolveURL for base URLs, not direct fetch.FetchURL)
• Confirm pipeline ordering: LoadWithBase → ResolveForge → ResolveRelativeTo → lock check → ResolveHarness → ValidateRunnerEnv → ValidateFilesExist

🤖 Generated with Claude Code

[github-actions]

Site preview

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

Commit: f0e5d7c12346fb1a7a1250c3a6754e8029ddc7ef

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 10:25 PM UTC · Completed 10:38 PM UTC
Commit: 27b1874 · View workflow run →

[fullsend-ai-review]

Review

Findings

Low

• [edge-case] docs/plans/adr-0045-forge-portable-harness-schema.md:74 — PR 2 duplicates mintcore.RolePattern regex and adds double-hyphen rejection, but neither the existing mintcore.ValidateRoleName nor the proposed harness validation imposes a maximum length on role names. Unbounded role names could cause issues in downstream string formatting (e.g., secret names, label construction). This is consistent with the existing mintcore.RolePattern (which also lacks a length bound) and is a pre-existing latent risk, not a gap introduced by this plan.

• [filename-convention] docs/plans/adr-0045-forge-portable-harness-schema.md — Filename uses a hybrid pattern (adr-0045-forge-portable-harness-schema.md) that differs from the two established conventions: descriptive kebab-case (e.g., agent-execution-environment.md) and date-prefixed ADR references (e.g., 2026-03-06-adr46-drift-scanner.md).

Info

• [internal-consistency] docs/plans/adr-0045-forge-portable-harness-schema.md:103 — PR 3's detectForgePlatform uses GITHUB_ACTIONS=true for auto-detection. Known design trade-off acknowledged in the ADR's open questions, mitigated by the precedence chain (flag > env > skip).

Previous run

Review

Findings

Medium

• [coherence-documentation-index] README.md — The PR adds a new plan file docs/plans/adr-0045-forge-portable-harness-schema.md but does not update the README.md document index. All six existing plan files in docs/plans/ are explicitly listed in README.md (lines 43-49). The new plan should be added to maintain discoverability.

Low

• [missing-authorization] docs/plans/adr-0045-forge-portable-harness-schema.md — The PR adds an implementation plan with no linked issue. ADR-0045 is already in Accepted status, which establishes the authorized scope for this work. Consider adding a reference to ADR-0045's Accepted status in the PR description to make the authorization chain explicit.

• [filename-convention] docs/plans/adr-0045-forge-portable-harness-schema.md — Filename uses a hybrid pattern (adr-0045-forge-portable-harness-schema.md) that differs from the two established conventions: descriptive kebab-case (e.g., agent-execution-environment.md) and date-prefixed ADR references (e.g., 2026-03-06-adr46-drift-scanner.md).

• [edge-case] docs/plans/adr-0045-forge-portable-harness-schema.md:159 — PR 4's mergeHarness describes HostFiles merge as "concatenated, last-writer-wins dedup by Dest" but does not explicitly state which direction dedup keeps. The phrase "last-writer-wins" combined with "base + child" concatenation order implies child overrides base (confirmed by the ADR's merge rules table), but making the dedup direction unambiguous in the plan would prevent implementation errors.

Info

• [logic-error] docs/plans/adr-0045-forge-portable-harness-schema.md:74 — PR 2 duplicates mintcore.RolePattern regex and adds double-hyphen rejection but neither imposes a maximum length on role names. This is consistent with the existing mintcore.RolePattern (which also lacks a length bound) and is a pre-existing latent risk, not a gap introduced by this plan.

• [edge-case] docs/plans/adr-0045-forge-portable-harness-schema.md:145 — PR 4's host_files merge uses exact string comparison for Dest dedup with no path canonicalization. The ADR explicitly documents this as a known limitation deferred to implementation.

• [internal-consistency] docs/plans/adr-0045-forge-portable-harness-schema.md:200 — PR 5 says to "append [baseDeps] directly" alongside deps from resolve.ResolveHarness, but LoadWithBase returns []lock.DependencyEntry while ResolveHarness returns []resolve.Dependency. The existing runLock code already performs this conversion, so implementers have a pattern to follow.

• [edge-case] docs/plans/adr-0045-forge-portable-harness-schema.md:103 — PR 3's detectForgePlatform uses GITHUB_ACTIONS=true for auto-detection. Known design trade-off acknowledged in the ADR's open questions, mitigated by the precedence chain (flag > env > skip).

Previous run (2)

Review

Findings

Low

• [edge-case] docs/plans/adr-0045-forge-portable-harness-schema.md:159 — PR 4's mergeHarness describes HostFiles merge as "concatenated, last-writer-wins dedup by Dest" but does not explicitly state which direction dedup keeps. The phrase "last-writer-wins" combined with "base + child" concatenation order implies child overrides base (confirmed by the ADR's merge rules table), but making the dedup direction unambiguous in the plan would prevent implementation errors (e.g., iterating from the end and keeping the first seen vs. iterating from the start and keeping the last seen).

Info

• [logic-error] docs/plans/adr-0045-forge-portable-harness-schema.md:74 — PR 2 duplicates mintcore.RolePattern regex and adds double-hyphen rejection but neither imposes a maximum length on role names. This is consistent with the existing mintcore.RolePattern (which also lacks a length bound) and is a pre-existing latent risk, not a gap introduced by this plan.

• [edge-case] docs/plans/adr-0045-forge-portable-harness-schema.md:145 — PR 4's host_files merge uses exact string comparison for Dest dedup with no path canonicalization. The ADR explicitly documents this as a known limitation deferred to implementation.

• [internal-consistency] docs/plans/adr-0045-forge-portable-harness-schema.md:200 — PR 5 says to "append [baseDeps] directly" alongside deps from resolve.ResolveHarness, but LoadWithBase returns []lock.DependencyEntry while ResolveHarness returns []resolve.Dependency. The existing runLock code already performs this conversion, so implementers have a pattern to follow.

• [edge-case] docs/plans/adr-0045-forge-portable-harness-schema.md:103 — PR 3's detectForgePlatform uses GITHUB_ACTIONS=true for auto-detection. Known design trade-off acknowledged in the ADR's open questions, mitigated by the precedence chain (flag > env > skip).

Previous run (3)

Review

Findings

Medium

• [consumer-completeness] docs/plans/adr-0045-forge-portable-harness-schema.md:207 — PR 5's lock.go pipeline does not account for HasURLReferences() short-circuit after base composition. The current fullsend lock (internal/cli/lock.go:81) calls h.HasURLReferences() and returns early if false with "nothing to lock". HasURLReferences() (internal/harness/harness.go:618) only checks Agent, Policy, and Skills — it has no knowledge of the base field. After LoadWithBase resolves and clears base, a harness whose only remote references were in base (all agent/policy/skills are local) would pass this check as "no remote dependencies", causing fullsend lock to skip writing the lock file entirely — losing the base dependency entries.
  Remediation: In PR 5's lock.go modifications, move the HasURLReferences() check after LoadWithBase, and change the condition to !h.HasURLReferences() && len(baseDeps) == 0 before short-circuiting.

Low

• [edge-case] docs/plans/adr-0045-forge-portable-harness-schema.md:159 — PR 4's mergeHarness describes HostFiles merge as "concatenated, last-writer-wins dedup by Dest" but does not explicitly state which direction dedup keeps. The phrase "last-writer-wins" combined with "base + child" concatenation order implies child overrides base, but an explicit statement would prevent implementation ambiguity.

• [logic-error] docs/plans/adr-0045-forge-portable-harness-schema.md:74 — PR 2 duplicates mintcore.RolePattern regex and adds double-hyphen rejection but neither imposes a maximum length. Unbounded role names could cause issues in derived identifiers (secret names, slug composition). The existing mintcore.RolePattern also lacks a length bound, so this is consistent with current behavior but worth noting as a latent risk.

Info

• [edge-case] docs/plans/adr-0045-forge-portable-harness-schema.md:145 — PR 4's host_files merge uses exact string comparison for Dest dedup with no path canonicalization. The plan explicitly documents this as a known limitation.

• [internal-consistency] docs/plans/adr-0045-forge-portable-harness-schema.md:200 — PR 5 says to "append [baseDeps] directly" alongside deps from resolve.ResolveHarness, but LoadWithBase returns []lock.DependencyEntry while ResolveHarness returns []resolve.Dependency. The existing runLock code already converts between these types.

• [edge-case] docs/plans/adr-0045-forge-portable-harness-schema.md:103 — PR 3's detectForgePlatform uses GITHUB_ACTIONS=true for auto-detection. Known design trade-off, mitigated by the precedence chain (flag > env > skip).

Previous run (4)

Review

Findings

Medium

• [consumer-completeness] docs/plans/adr-0045-forge-portable-harness-schema.md:171 — PR 4 records base URL dependencies as DependencyEntry with Field: "base", but the existing resolveFromLock function in internal/cli/lock.go:236-253 dispatches on Field values using a switch/default pattern. A Field: "base" entry would fall through to the default case, fail the skills[%d] parse, and be incorrectly appended to h.Skills as a transitive skill dependency. The plan does not mention updating resolveFromLock to handle the new "base" field value.
  Remediation: Add a case for Field == "base" to resolveFromLock in internal/cli/lock.go (likely a no-op since base composition is resolved before lock-based resolution), or filter out Field: "base" entries before they reach the mutation loop. Document this in PR 4 or PR 5.

Low

• [logic-error] docs/plans/adr-0045-forge-portable-harness-schema.md:140 — PR 4 defines ComposeOpts with fields WorkspaceRoot, FetchPolicy, AuditLogPath, OrgAllowlist, and HarnessAllowlist, but omits ForgePlatform. However, line 150 references opts.ForgePlatform when calling ResolveForge, and PR 5 (line 199) says to pass ForgePlatform through ComposeOpts. The struct definition is missing this field.

• [logic-error] docs/plans/adr-0045-forge-portable-harness-schema.md:110 — PR 3's multi-forge locking behavior (line 117) describes iterating all forge keys when --forge is omitted, but does not specify which load function (LoadWithOpts or loadRaw + per-variant ResolveForge) to use for each variant, or error handling when one variant fails but others succeed.

• [logic-error] docs/plans/adr-0045-forge-portable-harness-schema.md:74 — PR 2 duplicates mintcore.RolePattern regex and adds double-hyphen rejection but neither imposes a maximum length. Unbounded role names could cause issues in derived identifiers (secret names, slug composition).

Info

• [edge-case] docs/plans/adr-0045-forge-portable-harness-schema.md:145 — PR 4's host_files merge uses exact string comparison for Dest dedup with no path canonicalization. The plan explicitly documents this as a known limitation.

• [naming-convention] docs/plans/adr-0045-forge-portable-harness-schema.md — Filename uses adr-0045- prefix. Existing plans use descriptive names or date prefixes, though 2026-03-06-adr46-drift-scanner.md provides some precedent for ADR-prefixed names.

• [internal-consistency] docs/plans/adr-0045-forge-portable-harness-schema.md:200 — PR 5 says to "append [baseDeps] directly" alongside deps from resolve.ResolveHarness, but LoadWithBase returns []lock.DependencyEntry while ResolveHarness returns []resolve.Dependency. The existing runLock code already converts between these types.

• [edge-case] docs/plans/adr-0045-forge-portable-harness-schema.md:103 — PR 3's detectForgePlatform uses GITHUB_ACTIONS=true for auto-detection. Known design trade-off, mitigated by the precedence chain (flag > env > skip).

• [authorization-completeness] docs/plans/adr-0045-forge-portable-harness-schema.md — No linked issue for this implementation plan. ADR-0045 is confirmed Accepted (2026-05-27), which provides architectural authorization. Explicit issue linkage would strengthen traceability for the 7+ Phase 1 PRs.

Previous run (5)

Review

Findings

Medium

• [consumer-completeness] docs/plans/adr-0045-forge-portable-harness-schema.md:171 — PR 4 records base URL dependencies as DependencyEntry with Field: "base", but the existing resolveFromLock function in internal/cli/lock.go:236-253 dispatches on Field values using a switch/default pattern. A Field: "base" entry would fall through to the default case, fail the skills[%d] parse, and be incorrectly appended to h.Skills as a transitive skill dependency. The plan does not mention updating resolveFromLock to handle the new "base" field value.
  Remediation: Add a case for Field == "base" to resolveFromLock in internal/cli/lock.go (likely a no-op since base composition is resolved before lock-based resolution), or filter out Field: "base" entries before they reach the mutation loop. Document this in PR 4 or PR 5.

Low

• [logic-error] docs/plans/adr-0045-forge-portable-harness-schema.md:140 — PR 4 defines ComposeOpts with fields WorkspaceRoot, FetchPolicy, AuditLogPath, OrgAllowlist, and HarnessAllowlist, but omits ForgePlatform. However, line 150 references opts.ForgePlatform when calling ResolveForge, and PR 5 (line 199) says to pass ForgePlatform through ComposeOpts. The struct definition is missing this field.

• [logic-error] docs/plans/adr-0045-forge-portable-harness-schema.md:110 — PR 3's multi-forge locking behavior (line 117) describes iterating all forge keys when --forge is omitted, but does not specify which load function (LoadWithOpts or loadRaw + per-variant ResolveForge) to use for each variant, or error handling when one variant fails but others succeed.

• [logic-error] docs/plans/adr-0045-forge-portable-harness-schema.md:74 — PR 2 duplicates mintcore.RolePattern regex and adds double-hyphen rejection but neither imposes a maximum length. Unbounded role names could cause issues in derived identifiers (secret names, slug composition).

Info

• [edge-case] docs/plans/adr-0045-forge-portable-harness-schema.md:145 — PR 4's host_files merge uses exact string comparison for Dest dedup with no path canonicalization. The plan explicitly documents this as a known limitation.

• [naming-convention] docs/plans/adr-0045-forge-portable-harness-schema.md — Filename uses adr-0045- prefix. Existing plans use descriptive names or date prefixes, though 2026-03-06-adr46-drift-scanner.md provides some precedent for ADR-prefixed names.

• [internal-consistency] docs/plans/adr-0045-forge-portable-harness-schema.md:200 — PR 5 says to "append [baseDeps] directly" alongside deps from resolve.ResolveHarness, but LoadWithBase returns []lock.DependencyEntry while ResolveHarness returns []resolve.Dependency. The existing runLock code already converts between these types.

• [edge-case] docs/plans/adr-0045-forge-portable-harness-schema.md:103 — PR 3's detectForgePlatform uses GITHUB_ACTIONS=true for auto-detection. Known design trade-off, mitigated by the precedence chain (flag > env > skip).

• [authorization-completeness] docs/plans/adr-0045-forge-portable-harness-schema.md — No linked issue for this implementation plan. ADR-0045 is confirmed Accepted (2026-05-27), which provides architectural authorization. Explicit issue linkage would strengthen traceability for the 7+ Phase 1 PRs.

Previous run (6)

Review

Findings

Medium

• [logic-error] docs/plans/adr-0045-forge-portable-harness-schema.md:133 — PR 4's LoadWithBase returns []resolve.Dependency but the lock file uses lock.DependencyEntry. These are different types: Dependency has LocalPath and CacheHit; DependencyEntry has TransitiveDeps. The plan does not describe the conversion between these types, and no existing conversion function was found in the codebase. Callers need a mapping function to write base dependencies into lock.yaml as DependencyEntry values.
  Remediation: Either have LoadWithBase return []lock.DependencyEntry directly (since it already has access to SHA256 and fetch time), or document a conversion helper in PR 4 or PR 5 that maps resolve.Dependency to lock.DependencyEntry.

• [logic-error] docs/plans/adr-0045-forge-portable-harness-schema.md:110 — PR 3 adds --forge flag to fullsend lock but does not address what happens when a user runs fullsend lock without --forge on a harness that has forge-specific URL resources (e.g., forge.github.skills contains a URL). Forge resolution is skipped, the forge-specific URLs are never seen by the resolver, and the lock file will be incomplete. When the user later runs fullsend run --forge github, the runtime will encounter unlocked URLs.
  Remediation: Specify the behavior of fullsend lock with multi-forge harnesses: either require --forge and lock one variant, or iterate all forge keys and lock resources for each variant. Document the chosen approach in PR 3 or PR 5.

Low

• [logic-error] docs/plans/adr-0045-forge-portable-harness-schema.md:74 — PR 2 duplicates mintcore.RolePattern regex and adds double-hyphen rejection to match mintcore.ValidateRoleName, but neither imposes a maximum length. Unbounded role names could cause issues downstream in secret names or label construction. The plan is internally consistent with mintcore, but both lack a length bound.

• [edge-case] docs/plans/adr-0045-forge-portable-harness-schema.md:145 — PR 4's host_files merge uses exact string comparison with no path canonicalization. Paths like /tmp/workspace/file and /tmp/workspace/../workspace/file would not be detected as duplicates. The ADR acknowledges this and defers it.

Info

• [edge-case] docs/plans/adr-0045-forge-portable-harness-schema.md:103 — PR 3's detectForgePlatform uses GITHUB_ACTIONS=true for auto-detection, which is a known design trade-off acknowledged in the ADR's open questions. Mitigated by the precedence chain (flag > env > config).

• [internal-consistency] docs/plans/adr-0045-forge-portable-harness-schema.md:126 — PR 4's LoadWithBase returns (*Harness, []resolve.Dependency, error) but when base is absent it degrades to LoadWithOpts behavior, which has signature (*Harness, error). Cosmetic — the implementation would return nil for deps.

• [authorization-completeness] docs/plans/adr-0045-forge-portable-harness-schema.md — No linked issue for this implementation plan. ADR-0045 is confirmed Accepted (2026-05-27), which provides architectural authorization. Explicit issue linkage would strengthen traceability for the 7+ Phase 1 PRs.

Previous run (7)

Review

Findings

Medium

• [internal-consistency] docs/plans/adr-0045-forge-portable-harness-schema.md:196 — PR 5 states that forge resolution happens "inside Load for each harness in the base chain", but the ADR (line 346) specifies base harness (recursive) → child overrides → ResolveForge(platform) — i.e., ResolveForge runs once on the final merged result. If ResolveForge runs inside Load() for each harness, it calls h.Forge = nil (PR 1, line 67) before mergeHarness executes, so the forge map is always nil when merge runs, and PR 4's forge merge rule never triggers.
  Remediation: Align the plan with the ADR: LoadWithBase should load each harness in the chain WITHOUT forge resolution, merge the base chain (including forge maps via PR 4's merge rules), and then call ResolveForge(platform) once on the final merged harness. Update PR 5's pipeline description accordingly.

• [logic-error] docs/plans/adr-0045-forge-portable-harness-schema.md:150 — PR 4's LoadWithBase calls harness.Load(path) to load each base harness. Load() calls Validate(), which requires agent to be non-empty (harness.go line 236). This means base harnesses must be complete, valid harnesses with an agent field. The ADR's open question (line 703-707) explicitly asks whether base should support partial YAML fragments without agent. The plan silently forecloses this option without acknowledging the design decision.
  Remediation: Acknowledge the design decision explicitly: either state that base harnesses must be complete valid harnesses (not fragments), or propose a separate load path that relaxes validation for base harnesses.

Low

• [internal-consistency] docs/plans/adr-0045-forge-portable-harness-schema.md:19 — The ADR-0038 implementation status table lists Phase 3 as "In progress" with fullsend lock CLI on feat/adr0038-phase3-lock-cli. However, PR feat: add fullsend lock CLI and lock-aware resolution #2082 from that branch has already merged to main. The status table is stale, and several conditional statements in PRs 3–5 ("if ADR-0038 Phase 3 PR 2 has merged") are misleading — the condition is already true.

• [edge-case] docs/plans/adr-0045-forge-portable-harness-schema.md:172 — PR 4's merge rule for host_files says "last-writer-wins dedup by Dest" with "exact string comparison, no path canonicalization". Paths like /tmp/workspace/file and /tmp/workspace/../workspace/file would not be detected as duplicates. The ADR's open questions acknowledge this but defer it.

• [scope-alignment] docs/plans/adr-0045-forge-portable-harness-schema.md — The PR body states "Adds Phase 1 implementation plan" yet the plan includes outlines for Phases 2–4. The filename does not include a phase suffix.

• [naming-pattern-consistency] docs/plans/adr-0045-forge-portable-harness-schema.md — Filename uses adr-0045- prefix without a date, creating a hybrid pattern. Existing plans use either descriptive names (universal-harness-access.md) or date-prefixed with ADR number (2026-03-06-adr46-drift-scanner.md).

Info

• [authorization-completeness] docs/plans/adr-0045-forge-portable-harness-schema.md — No linked issue for this implementation plan. ADR-0045 is confirmed Accepted (2026-05-27), which provides architectural authorization. Explicit issue linkage would strengthen traceability for the 7+ Phase 1 PRs.

Previous run (8)

Review

Findings

Medium

• [internal-consistency] docs/plans/adr-0045-forge-portable-harness-schema.md:218 — PR 5's lock pipeline description lists ResolveForge as a separate step after LoadWithBase ("Same pipeline ordering for fullsend lock: LoadWithBase → ResolveForge → ResolveRelativeTo → resolve.ResolveHarness"), but the plan establishes in PR 3 and PR 5's own run pipeline that ResolveForge runs inside Load() (which is called internally by LoadWithBase). This contradicts the plan's core design that ResolveForge must NOT be called externally after Load().
  Remediation: Remove ResolveForge as a separate step from the lock pipeline description. The correct ordering is: LoadWithBase (which internally does unmarshal → ResolveForge → Validate for each harness in the base chain) → ResolveRelativeTo → resolve.ResolveHarness.

Low

• [logic-error] docs/plans/adr-0045-forge-portable-harness-schema.md:150 — PR 4's LoadWithBase calls harness.Load(path) (the original signature without forge support) to load base harnesses. PR 3 introduces LoadWithOpts which adds forge resolution inside Load. The dependency graph does not show PR 4 depending on PR 3. This is by design — PR 5 is the integration PR that depends on both and wires ForgePlatform through ComposeOpts — but the plan could be more explicit about this intermediate state.

• [edge-case] docs/plans/adr-0045-forge-portable-harness-schema.md:155 — PR 4's LoadWithBase returns (*Harness, []resolve.Dependency, error) (three return values) while the existing Load() returns (*Harness, error). PR 5 updates both run.go and lock.go to use LoadWithBase, but the plan could be more explicit about exhaustive call-site enumeration.

• [edge-case] docs/plans/adr-0045-forge-portable-harness-schema.md:172 — PR 4's merge rule for host_files says "last-writer-wins dedup by Dest" but does not specify whether deduplication uses exact string comparison or canonicalized paths. The implementation PR can decide, but noting the ambiguity here would prevent bugs.

• [scope-alignment] docs/plans/adr-0045-forge-portable-harness-schema.md — The PR body states "Adds Phase 1 implementation plan" yet the plan includes outlines for Phases 2-4. The filename does not include a phase suffix. This is clearly intentional (Phase 1 in detail, future phases as outlines), but the PR description could be updated to say "comprehensive plan" rather than "Phase 1 plan."

Info

• [documentation-pattern] docs/plans/adr-0045-forge-portable-harness-schema.md — Filename follows the adr-NNNN prefix pattern, one of three naming conventions used in docs/plans/. No single dominant convention exists.

• [prior-findings-resolved] docs/plans/adr-0045-forge-portable-harness-schema.md — Four prior medium/low findings from the previous review have been addressed: malformed YAML tag syntax (now correctly specifies separate yaml:"role,omitempty" and yaml:"slug,omitempty" tags), missing double-hyphen rejection (now includes strings.Contains(role, "--")), pipeline ordering (now correctly places ResolveForge inside Load()), and ValidateResourceTypes gap (now notes Validate() sees post-merge state).

Previous run

Review

Findings

Medium

• [logic-error] docs/plans/adr-0045-forge-portable-harness-schema.md:75 — PR 2 specifies the YAML tag as yaml:"role/slug,omitempty" for the Role and Slug fields. This is a single malformed tag — the slash syntax is not valid Go struct tag syntax for separate fields. It would try to unmarshal a YAML key literally named role/slug. Each struct field needs its own YAML tag. The ADR itself (lines 473-475) correctly shows separate tags.
  Remediation: Clarify that each field gets its own tag: Role string yaml:"role,omitempty"`` and Slug string yaml:"slug,omitempty"`.

• [logic-error] docs/plans/adr-0045-forge-portable-harness-schema.md:73 — PR 2 proposes validRole = regexp.MustCompile('^[a-z][a-z0-9_-]*$') described as "consistent with mintcore.RolePattern". While the regex matches, mintcore.ValidateRoleName also rejects double-hyphens (strings.Contains(role, "--")). The plan omits this check, creating a behavioral gap: role values like my--role would pass harness validation but fail at runtime in mintcore.
  Remediation: Add a double-hyphen check to the harness-side role validation, or document the intentional omission.

Low

• [pipeline-ordering] docs/plans/adr-0045-forge-portable-harness-schema.md:105 — PR 3 places ResolveForge between Load() and ResolveRelativeTo() in the CLI. However, the ADR's Go doc comment says to insert it between Unmarshal and Validate inside Load() and states it "Must NOT be called externally after Load()". This contradiction could cause confusion during implementation.

• [edge-case] docs/plans/adr-0045-forge-portable-harness-schema.md:58 — PR 1 does not address how forge-overridden PreScript/PostScript/ValidationLoop.Script paths interact with ValidateResourceTypes(). If ResolveForge runs after Load() (which calls Validate()), forge-overridden script paths skip the URL rejection check.

• [edge-case] docs/plans/adr-0045-forge-portable-harness-schema.md:130 — PR 4 describes LoadWithBase as a "drop-in replacement" for Load (returning (*Harness, error)) but also says it returns a list of resolve.Dependency for lock file integration. These are incompatible return signatures.

• [coherence-with-adr] docs/plans/adr-0045-forge-portable-harness-schema.md — ADR-0045 already includes detailed migration phases and implementation details. The separate implementation plan should cross-reference the ADR to avoid divergence.

Info

• [documentation-pattern] docs/plans/adr-0045-forge-portable-harness-schema.md — Filename naming convention varies across docs/plans/. No single dominant convention exists.

• [forward-reference-only] docs/ADRs/0024-harness-definitions.md — ADR-0024 does not forward-reference ADR-0045's extensions. Consistent with ADR-as-point-in-time-record convention.

• [future-state-documentation] docs/ADRs/0011-admin-install-org-config-yaml-v1.md — ADR-0011 establishes config.yaml v1 including the agents: block that ADR-0045 Phase 4 will remove. No action needed now.

Previous run (9)

Review

Findings

Medium

• [internal-consistency] docs/plans/adr-0045-forge-portable-harness-schema.md:218 — PR 5's lock pipeline description lists ResolveForge as a separate step after LoadWithBase ("Same pipeline ordering for fullsend lock: LoadWithBase → ResolveForge → ResolveRelativeTo → resolve.ResolveHarness"), but the plan establishes in PR 3 and PR 5's own run pipeline that ResolveForge runs inside Load() (which is called internally by LoadWithBase). This contradicts the plan's core design that ResolveForge must NOT be called externally after Load().
  Remediation: Remove ResolveForge as a separate step from the lock pipeline description. The correct ordering is: LoadWithBase (which internally does unmarshal → ResolveForge → Validate for each harness in the base chain) → ResolveRelativeTo → resolve.ResolveHarness.

Low

• [logic-error] docs/plans/adr-0045-forge-portable-harness-schema.md:150 — PR 4's LoadWithBase calls harness.Load(path) (the original signature without forge support) to load base harnesses. PR 3 introduces LoadWithOpts which adds forge resolution inside Load. The dependency graph does not show PR 4 depending on PR 3. This is by design — PR 5 is the integration PR that depends on both and wires ForgePlatform through ComposeOpts — but the plan could be more explicit about this intermediate state.

• [edge-case] docs/plans/adr-0045-forge-portable-harness-schema.md:155 — PR 4's LoadWithBase returns (*Harness, []resolve.Dependency, error) (three return values) while the existing Load() returns (*Harness, error). PR 5 updates both run.go and lock.go to use LoadWithBase, but the plan could be more explicit about exhaustive call-site enumeration.

• [edge-case] docs/plans/adr-0045-forge-portable-harness-schema.md:172 — PR 4's merge rule for host_files says "last-writer-wins dedup by Dest" but does not specify whether deduplication uses exact string comparison or canonicalized paths. The implementation PR can decide, but noting the ambiguity here would prevent bugs.

• [scope-alignment] docs/plans/adr-0045-forge-portable-harness-schema.md — The PR body states "Adds Phase 1 implementation plan" yet the plan includes outlines for Phases 2-4. The filename does not include a phase suffix. This is clearly intentional (Phase 1 in detail, future phases as outlines), but the PR description could be updated to say "comprehensive plan" rather than "Phase 1 plan."

Info

• [documentation-pattern] docs/plans/adr-0045-forge-portable-harness-schema.md — Filename follows the adr-NNNN prefix pattern, one of three naming conventions used in docs/plans/. No single dominant convention exists.

• [prior-findings-resolved] docs/plans/adr-0045-forge-portable-harness-schema.md — Four prior medium/low findings from the previous review have been addressed: malformed YAML tag syntax (now correctly specifies separate yaml:"role,omitempty" and yaml:"slug,omitempty" tags), missing double-hyphen rejection (now includes strings.Contains(role, "--")), pipeline ordering (now correctly places ResolveForge inside Load()), and ValidateResourceTypes gap (now notes Validate() sees post-merge state).

Previous run (10)

Review

Findings

Medium

• [logic-error] docs/plans/adr-0045-forge-portable-harness-schema.md:75 — PR 2 specifies the YAML tag as yaml:"role/slug,omitempty" for the Role and Slug fields. This is a single malformed tag — the slash syntax is not valid Go struct tag syntax for separate fields. It would try to unmarshal a YAML key literally named role/slug. Each struct field needs its own YAML tag. The ADR itself (lines 473-475) correctly shows separate tags.
  Remediation: Clarify that each field gets its own tag: Role string yaml:"role,omitempty" and `Slug string `yaml:"slug,omitempty".

• [logic-error] docs/plans/adr-0045-forge-portable-harness-schema.md:73 — PR 2 proposes validRole = regexp.MustCompile('^[a-z][a-z0-9_-]*$') described as "consistent with mintcore.RolePattern". While the regex matches, mintcore.ValidateRoleName also rejects double-hyphens (strings.Contains(role, "--")). The plan omits this check, creating a behavioral gap: role values like my--role would pass harness validation but fail at runtime in mintcore.
  Remediation: Add a double-hyphen check to the harness-side role validation, or document the intentional omission.

Low

• [pipeline-ordering] docs/plans/adr-0045-forge-portable-harness-schema.md:105 — PR 3 places ResolveForge between Load() and ResolveRelativeTo() in the CLI. However, the ADR's Go doc comment says to insert it between Unmarshal and Validate inside Load() and states it "Must NOT be called externally after Load()". This contradiction could cause confusion during implementation.

• [edge-case] docs/plans/adr-0045-forge-portable-harness-schema.md:58 — PR 1 does not address how forge-overridden PreScript/PostScript/ValidationLoop.Script paths interact with ValidateResourceTypes(). If ResolveForge runs after Load() (which calls Validate()), forge-overridden script paths skip the URL rejection check.

• [edge-case] docs/plans/adr-0045-forge-portable-harness-schema.md:130 — PR 4 describes LoadWithBase as a "drop-in replacement" for Load (returning (*Harness, error)) but also says it returns a list of resolve.Dependency for lock file integration. These are incompatible return signatures.

• [coherence-with-adr] docs/plans/adr-0045-forge-portable-harness-schema.md — ADR-0045 already includes detailed migration phases and implementation details. The separate implementation plan should cross-reference the ADR to avoid divergence.

Info

• [documentation-pattern] docs/plans/adr-0045-forge-portable-harness-schema.md — Filename naming convention varies across docs/plans/. No single dominant convention exists.

• [forward-reference-only] docs/ADRs/0024-harness-definitions.md — ADR-0024 does not forward-reference ADR-0045's extensions. Consistent with ADR-as-point-in-time-record convention.

• [future-state-documentation] docs/ADRs/0011-admin-install-org-config-yaml-v1.md — ADR-0011 establishes config.yaml v1 including the agents: block that ADR-0045 Phase 4 will remove. No action needed now.

[fullsend-ai-review]

See the review comment for full details.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 10:48 PM UTC · Completed 10:58 PM UTC
Commit: 4ed6da4 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 11:07 PM UTC · Completed 11:20 PM UTC
Commit: 4ed6da4 · View workflow run →

[fullsend-ai-review]

See the review comment for full details.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 12:43 AM UTC · Completed 12:56 AM UTC
Commit: 4ed6da4 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 1:07 AM UTC · Completed 1:17 AM UTC
Commit: 4ed6da4 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 2:01 AM UTC · Completed 2:15 AM UTC
Commit: 4ed6da4 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 3:08 AM UTC · Completed 3:18 AM UTC
Commit: 4ed6da4 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 11:43 AM UTC · Completed 11:52 AM UTC
Commit: 4ed6da4 · View workflow run →

[ralphbean]

LGTM. One note inline.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 12:48 PM UTC · Completed 12:58 PM UTC
Commit: 4ed6da4 · View workflow run →

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 3:41 PM UTC · Completed 3:47 PM UTC
Commit: 4ed6da4 · View workflow run →

[fullsend-ai-retro]

Retro: PR #2100 — ADR-0045 implementation plan

This docs-only PR (1 new markdown file + 1 README line) went through 10 automated review cycles and 8+ force pushes over ~17 hours before merging. The review bot found genuine issues in the implementation plan's pseudo-code (YAML tag syntax, role validation gaps, ResolveForge ordering) across two rounds, then produced informational-only findings in subsequent rounds. A human reviewer (ralphbean) approved with one layering suggestion.

Key observations

1. First-pass incompleteness: The bot found 5 findings in round 1, then 4 new findings in round 2. If all 9 had been caught in round 1, it would have saved an entire review cycle.
2. Approval revocation on informational pushes: After bot approval at 03:18 UTC, the author force-pushed to address informational findings at 11:42 UTC. The bot revoked approval and re-labeled requires-manual-review, triggering another full review cycle for no substantive reason.
3. Review dispatches on comment events: 14 workflow runs were triggered on this branch, many by pull_request_review events (author responding to review threads) rather than code pushes — generating review dispatches with no new code to review.
4. Token cost: 10 full review runs on a single 288-line markdown file represents significant token spend for diminishing returns after round 2.

Why no proposals

All observations are well-covered by existing open issues:

• First-pass completeness → #1582
• Approval revocation on informational re-review → #2029, #2057
• Label flip-flopping → #1574, #1319
• Docs-only fast path → #1900
• ADR/plan review behavior → #1659
• Review dispatch filtering on comment events → #1125, #1271, #893
• Dedup of rapid pushes → #1418, #1452
• Skip re-review when already approved → #963

The highest-impact issues to prioritize from this list would be #1582 (catch all findings first pass), #1125 (filter pull_request_review triggers), and #963 (skip re-review when already approved) — addressing these three alone would have reduced this PR's 10 review cycles to approximately 2-3.