docs(#793): document compose/diff bidirectional invariant - #809
fullsend-ai-coder[bot] wants to merge 2 commits into
Conversation
Add docs/contributing/harness-composition.md documenting the requirement that changes to merge/compose functions in compose.go and forge.go must be mirrored in corresponding diff functions in diff.go. This invariant supports the round-trip property used by migrate-customizations (ADR 0064). The doc lists all current paired functions (mergeBaseIntoChild ↔ DiffHarness, mergeForgeConfigInto ↔ diffForgeConfig, etc.), provides a checklist for changes, and explains when the diff side may be unaffected. A row is added to the AGENTS.md guidance table so agents discover this constraint when modifying internal/harness/ merge or diff functions. Note: pre-commit could not run (network restriction in sandbox). The post-script runs an authoritative check. Closes #793
|
🤖 Finished Review · ✅ Success · Started 2:57 AM UTC · Completed 3:13 AM UTC |
ReviewFindingsHigh
Medium
Low
Next steps:
Previous runReviewFindingsMedium
Low
Labels: PR adds contributing documentation and modifies AGENTS.md guidance table Next steps:
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
the
documentation
|
/fs-fix |
|
🤖 Finished Fix · ✅ Success · Started 6:48 AM UTC · Completed 6:53 AM UTC |
Revise harness-composition.md to accurately reflect the current codebase: - Intro: state diff functions were removed rather than implying they exist - Why this matters: describe actual migrate-customizations implementation (rewriteCustomizedPaths) and frame diff-based flow as future/envisioned - Paired functions table: rename section and mark diff counterparts as "not currently present" with explanatory header text - Checklist: add step 1 to check whether diff functions exist before following the remaining steps - Historical context: remove ambiguous hedging; state directly that diff functions do not currently exist - Title: shorten to "Harness Composition" matching sibling doc format Addresses review feedback on #809
🔧 Fix agent — iteration 1 (human-triggered)Addressed 5 of 6 review findings by revising harness-composition.md to accurately reflect the current codebase state: diff functions were removed, migrate-customizations uses rewriteCustomizedPaths not diff, and the checklist now gates on whether diff functions exist. Disagreed with 1 finding (AGENTS.md protected path is a process matter requiring human approval, not an agent fix). Fixed (5):
Disagreed (1):
Tests: passedNext steps:
|
|
🤖 Finished Review · ✅ Success · Started 6:59 AM UTC · Completed 7:14 AM UTC |
| `customized/` directory overlays into config-driven agents with `base:` | ||
| composition. The current implementation moves files from `customized/` to | ||
| regular directories via `rewriteCustomizedPaths` and registers the agent in | ||
| config — it does not use diff functions today. However, |
There was a problem hiding this comment.
[high] technical accuracy
The document claims ADR 0064 'envisions a diff-based migration flow where overrides are extracted by diffing a customized harness against its upstream base.' ADR 0064 does not describe or envision a diff-based migration flow. It describes migration via base: composition, URL references, and config-based registration. ADR 0045 references DiffHarness in past tense, not as a future vision. The claimed design intent is fabricated.
Suggested fix: Remove or correct the claim about ADR 0064. Replace lines 18-22 with an accurate description of the migration approach.
| When changing merge or compose functions in `internal/harness/`, be aware that | ||
| these functions have an architectural counterpart: diff functions that extract | ||
| the delta between a composed result and its base. The diff functions were | ||
| removed with the scaffold agent extraction (see |
There was a problem hiding this comment.
[medium] internal consistency / issue alignment
The document states diff functions 'were removed with the scaffold agent extraction' and 'do not currently exist.' Issue #793 (which this PR closes) states the opposite: 'DiffHarness still exists in internal/harness/diff.go.' The document's framing as theoretical/future guidance contradicts the issue's framing as practical/current guidance. The timeline discrepancy is unexplained.
Suggested fix: Add a note explaining the timeline - whether diff.go was removed after issue #793 was filed.
| | Compose / Merge (compose.go, forge.go) | Expected diff counterpart (not currently present) | Scope | | ||
| |---|---|---| | ||
| | `mergeBaseIntoChild` | `DiffHarness` | Top-level harness fields | | ||
| | `mergeForgeConfigInto` | `diffForgeConfig` | Per-platform forge config in `base:` composition | |
There was a problem hiding this comment.
[medium] algorithm logic consistency
The table lists mergeForgeConfig (forge.go) and mergeForgeConfigInto (compose.go) as having the same expected diff counterpart diffForgeConfig. These functions have different semantics operating on different type pairs. A single diffForgeConfig cannot correctly invert both operations.
Suggested fix: Differentiate the expected diff counterparts for these two functions.
| semantics (e.g. from whole-replace to field-level merge), update the | ||
| diff to produce the correct delta under the new semantics. | ||
| 4. **Test the round-trip.** Verify that `compose(base, diff(composed, base))` | ||
| produces the original `composed` result. Existing tests in |
There was a problem hiding this comment.
[low] edge case correctness
The round-trip test formula assumes diff is a pure inverse of compose, but actual merge semantics include lossy operations (e.g., mergeForgeConfig sets h.Forge = nil). The round-trip property may not hold for all merge functions.
| functions do not currently exist in this repository. The bidirectional | ||
| constraint remains architecturally important — any future re-addition of diff | ||
| functions must mirror the current merge semantics. | ||
|
|
There was a problem hiding this comment.
[low] cross-reference-consistency
Issue reference uses guyoron1/fullsend for Issue #662, but existing documentation references use fullsend-ai/fullsend. The URL should point to the upstream canonical repository.
Suggested fix: Change the issue URL to use fullsend-ai/fullsend.
|
This pull request has been automatically marked as stale because it has not had any activity in the last month. It will be closed in 2 weeks if no further activity occurs. Remove the |
|
This pull request has been closed due to inactivity. Feel free to reopen it if the changes are still needed. |
1 participant
Summary
Documents the bidirectional invariant between merge/compose functions (
compose.go,forge.go) and their corresponding diff functions (diff.go) ininternal/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 #793
Changes
docs/contributing/harness-composition.mdwith:migrate-customizations)mergeBaseIntoChild↔DiffHarness,mergeForgeConfigInto↔diffForgeConfig, etc.)internal/harness/"Testing
Checklist
!for breaking changes)Closes #793
Post-script verification
agent/793-compose-diff-invariant)ba77dbdbbd17e9bb18c47bf6efa023c98b697158..HEAD)