docs(#841): document compose/diff bidirectional invariant

[fullsend-ai-coder]

Summary

Documents the bidirectional invariant between merge/compose functions (compose.go, forge.go) and their corresponding diff functions in internal/harness/. This constraint was previously only documented in ADR-0045's Consequences section, which agents do not read during normal operation — causing a round-trip regression on PR fullsend-ai#5450 that took 6 iterations to resolve before being abandoned.

Related Issue

Closes #841

Changes

• Created docs/contributing/harness-composition.md with:
  • Explanation of the bidirectional invariant and why it matters (round-trip property)
  • Separate tables for base: composition functions (compose.go) and runtime forge resolution functions (forge.go), clarifying the semantic differences between mergeForgeConfigInto and mergeForgeConfig
  • Actionable checklist that gates on diff function existence — agents check whether diff functions exist before requiring diff-side updates
  • Guidance on when the diff side is unaffected
  • Historical context referencing ADR 0045 and related issue Document harness field integration pipeline as a contributor checklist #662
• Added a row to the AGENTS.md guidance table with trigger: "Changing merge/compose or diff functions in internal/harness/"

Testing

• No Go code changed — documentation only
• Secret scan passed
• gitlint passed

Checklist

• PR title follows Conventional Commits (correct type, ! for breaking changes)
• Commits are signed off (DCO) — human and human-directed agent sessions only
• I wrote this contribution myself and can explain all changes in it

---

Closes #841

Post-script verification

• Branch is not main/master (agent/841-compose-diff-invariant)
• Secret scan passed (gitleaks — c887fc47c59d8b87b5282ca44044d2933d30df41..HEAD)
• PR body secret scan passed (gitleaks — no-git)
• Pre-commit hooks passed (authoritative run on runner)
• Tests ran inside sandbox

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 9:44 AM UTC · Completed 9:59 AM UTC
Commit: 70192c4 · View workflow run →

[fullsend-ai-review]

Review

Findings

Medium

• [Incomplete documentation with correctness surface area] docs/contributing/harness-composition.md:37 — The merge semantics tables for mergeBaseIntoChild, mergeForgeConfigInto, and mergeForgeConfig omit the Env field (EnvConfig), which all three functions merge with differing precedence rules. mergeBaseIntoChild and mergeForgeConfigInto use mergeEnvFrom(src, false) (child/dst wins), while mergeForgeConfig uses mergeEnvFrom(src, true) (forge/src wins). Since the document serves as the authoritative reference for implementing diff counterparts, omitting Env could cause a diff implementor to miss a field entirely — producing the exact round-trip regression the document is designed to prevent.
  Remediation: Add Env merge semantics to each of the three merge function rows in the tables.

• [coherence-adr-conflict] docs/contributing/harness-composition.md — The document's framing — including the title "bidirectional invariant," the round-trip formula, and the multi-step checklist — gives disproportionate weight to diff functions that do not currently exist. While the checklist correctly gates on existence (step 1) and provides an exit path (step 3), the overall presentation could mislead readers into thinking maintaining diff function parity is a current requirement rather than a conditional future one.
  Remediation: Consider restructuring to lead with the merge function reference (the primary value today) and frame the diff-parity guidance as a conditional addendum.

• [protected-path] AGENTS.md — This PR modifies AGENTS.md, a protected governance file. The PR links to issue Document compose/diff bidirectional invariant in contributing docs #841 and explains the rationale (adding a guidance table entry for harness composition). Human approval is always required for protected-path changes regardless of context.

Low

• [Internal consistency] docs/contributing/harness-composition.md:11 — The sentence "ADR 0045 (Consequences, 'Bidirectional composition') notes that DiffHarness was removed with the scaffold agent extraction, but the constraint is that any re-introduced diff function must mirror the current merge semantics" is ambiguous about whether the normative constraint is from ADR 0045 or the document's own addition. The ADR records the historical fact of removal; the normative constraint is this document's contribution.
  Remediation: Rephrase to clearly separate the ADR citation from the document's own constraint, e.g., "ADR 0045 notes that DiffHarness was removed. This document establishes the constraint that..."

• [scope-misalignment] — Issue Document compose/diff bidirectional invariant in contributing docs #841 states that diff.go "still exists in internal/harness/diff.go," but no such file exists in the current codebase. The document correctly reflects the current state ("As of this writing they do not exist"), so this is a minor discrepancy in the issue's description, not in the PR itself.

Previous run

Review

Findings

Medium

• [protected-path] AGENTS.md — This PR modifies AGENTS.md, a protected governance file. The PR is linked to issue Document compose/diff bidirectional invariant in contributing docs #841 which explicitly requests adding a row to the AGENTS.md guidance table, providing sufficient context. Human approval is always required for protected-path changes, regardless of context.

Low

• [Technical documentation accuracy] docs/contributing/harness-composition.md:36 — The merge semantics summary for mergeBaseIntoChild describes pointer structs as "child replaces if non-nil", but the actual code in compose.go carries forward PreflightCheck from the base ValidationLoop when the child overrides validation_loop without setting its own preflight_check — the same behavior documented for mergeForgeConfigInto in the row below. Consider noting this exception in the mergeBaseIntoChild row for consistency.

• [Technical documentation accuracy] docs/contributing/harness-composition.md:46 — The mergeForgeConfig row describes skill merge semantics as "Skills: appended", but the code calls mergeSkills which performs basename-based deduplication, not simple appending. Consider updating to "Skills: harness + forge, forge overrides harness by basename (via mergeSkills)" for consistency with the other merge function descriptions.

• [stale-reference-to-removed-code] AGENTS.md:38 — The guidance row says "Changing merge/compose or diff functions in internal/harness/", but no diff functions currently exist. The linked guide handles this (explicitly stating diff functions were removed on three occasions), but the AGENTS.md trigger could be rephrased to clarify, e.g., "Changing merge/compose functions in internal/harness/ (or adding diff functions)".

---

Labels: PR adds new contributing documentation and modifies AGENTS.md guidance table

[guyoron1]

/fs-fix

[fullsend-ai-coder]

🤖 Finished Fix · ✅ Success · Started 10:32 AM UTC · Completed 10:37 AM UTC
Commit: 70192c4 · View workflow run →

[fullsend-ai-coder]

🔧 Fix agent — iteration 1 (human-triggered)

Addressed all 4 review findings. Fixed 3 documentation accuracy issues (PreflightCheck carry-forward exception, mergeForgeConfig skills dedup semantics, AGENTS.md stale trigger wording). Disagreed with 1 informational finding about protected-path labeling (already handled by existing label).

Fixed (3):

1. mergeBaseIntoChild merge semantics missing PreflightCheck carry-forward exception (docs/contributing/harness-composition.md): Updated the mergeBaseIntoChild row to note the PreflightCheck carry-forward exception for pointer structs, matching the actual code behavior at compose.go:575-583
2. mergeForgeConfig skills described as appended but actually uses mergeSkills with dedup (docs/contributing/harness-composition.md): Updated from 'Skills: appended' to 'Skills: harness + forge, forge overrides harness by basename (via mergeSkills)' to match the actual code at forge.go:124-126
3. AGENTS.md trigger mentions diff functions that do not currently exist (AGENTS.md): Rephrased trigger from 'Changing merge/compose or diff functions' to 'Changing merge/compose functions in internal/harness/ (or adding diff functions)' to clarify diff functions are not present

Disagreed (1):

1. AGENTS.md is a protected governance file requiring human approval: This is an informational finding about labels/approval requirements, not actionable code feedback. The PR already has the requires-manual-review label. The AGENTS.md change itself (trigger wording) is addressed in a separate finding below.

Tests: passed

Next steps:

• /fs-review — request a re-review of the changes
• /fs-fix <your instruction> — run another fix pass with specific guidance
• Push commits directly — review re-runs automatically on push
  _{Updated by fullsend fix agent}

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 10:40 AM UTC · Completed 10:55 AM UTC
Commit: f925f2f · View workflow run →

[fullsend-ai-review]

See the review comment for full details.

[fullsend-ai-review]

[medium] Incomplete documentation with correctness surface area

The merge semantics tables for mergeBaseIntoChild, mergeForgeConfigInto, and mergeForgeConfig omit the Env field (EnvConfig), which all three functions merge with differing precedence rules. mergeBaseIntoChild and mergeForgeConfigInto use mergeEnvFrom(src, false) (child/dst wins), while mergeForgeConfig uses mergeEnvFrom(src, true) (forge/src wins). Since the document serves as the authoritative reference for implementing diff counterparts, omitting Env could cause a diff implementor to miss a field entirely.

Suggested fix: Add Env merge semantics to each of the three merge function rows in the tables.

[fullsend-ai-review]

[low] Internal consistency

The sentence attributing the normative constraint ('any re-introduced diff function must mirror the current merge semantics') to ADR 0045 is inaccurate. The ADR records the historical fact that DiffHarness was removed; the normative constraint is this document's own addition.

Suggested fix: Rephrase to clearly separate the ADR citation from the document's own constraint.