Repository navigation
docs(#793): document compose/diff bidirectional invariant #809
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,88 @@ | ||
| # Harness Composition | ||
|
|
||
| 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 | ||
| [ADR 0045](../ADRs/0045-forge-portable-harness-schema.md), Consequences → | ||
| "Bidirectional composition"), but the bidirectional invariant remains | ||
| architecturally important — any future re-addition of diff-based operations | ||
| must mirror the current merge semantics. | ||
|
|
||
| ## Why this matters | ||
|
|
||
| The `fullsend agent migrate-customizations` command | ||
| ([cli-internals](../guides/dev/cli-internals.md)) converts deprecated | ||
| `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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [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. |
||
| [ADR 0064](../ADRs/0064-deprecate-customized-directory-overlay.md) envisions a | ||
| diff-based migration flow where overrides are extracted by diffing a customized | ||
| harness against its upstream base. If diff functions are re-added and do not | ||
| mirror the merge logic, overrides would be lost or duplicated — silently | ||
| corrupting harness customizations. | ||
|
|
||
| This constraint is documented in | ||
| [ADR 0045](../ADRs/0045-forge-portable-harness-schema.md) (Consequences → | ||
| "Bidirectional composition") but is not visible during normal documentation | ||
| reads because ADRs are subject to immutability policy. | ||
|
|
||
| ## Compose/merge functions and their expected diff counterparts | ||
|
|
||
| The merge functions below live in `internal/harness/compose.go` (for `base:` | ||
| composition) and `internal/harness/forge.go` (for runtime forge-config | ||
| application). The diff counterparts listed in the right column **do not | ||
| currently exist** — they were removed with the scaffold agent extraction. The | ||
| names shown are the expected counterparts based on ADR 0045; if diff functions | ||
| are re-added, they should follow this naming and scope. | ||
|
|
||
| | 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [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. |
||
| | `mergeForgeConfig` | `diffForgeConfig` | Forge config applied at runtime | | ||
| | `mergeForgeBlocks` | (per-key iteration over `diffForgeConfig`) | Map of platform → ForgeConfig | | ||
| | Field-level helpers (e.g. `mergeSkills`, `mergeHostFiles`) | Corresponding diff helpers | Individual collection fields | | ||
|
|
||
| ## Checklist for changes | ||
|
|
||
| When modifying merge or compose logic: | ||
|
|
||
| 1. **Check whether diff functions exist.** Look for `internal/harness/diff.go` | ||
| or functions matching `Diff*`/`diff*` in the harness package. If they do not | ||
| exist, note in your commit message that the diff side is absent and skip | ||
| steps 2–4. | ||
| 2. **Identify the paired function.** Use the table above or search for the | ||
| counterpart by name pattern (`merge*` ↔ `diff*`, `Diff*`). | ||
| 3. **Mirror the change.** If you add a field to a merge function, add the | ||
| corresponding extraction to the diff function. If you change merge | ||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [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. |
||
| `compose_test.go` cover the merge side; diff tests should follow the | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [low] Technical accuracy Checklist item 3 asks contributors to verify compose(base, diff(composed, base)) produces the original result and claims "diff tests should follow the same patterns." Since the diff functions do not exist, this checklist item is not actionable for the current codebase state. |
||
| same patterns. | ||
| 5. **Check field-level helpers.** Changes to helpers like `mergeSkills` or | ||
| `mergeHostFiles` may also need corresponding diff helpers. | ||
|
|
||
| ## When the diff side is unaffected | ||
|
|
||
| Not every compose change requires a diff change. If the modification is purely | ||
| internal (e.g. performance optimization that preserves semantics) or affects | ||
| only fields that use whole-replace semantics in both directions, the diff side | ||
| may already be correct. In that case, document in your commit message why the | ||
| diff side is unaffected. | ||
|
|
||
| ## Historical context | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [low] Internal consistency The "Historical context" section hedges with "the diff functions may be re-added or may exist in the upstream repository." This ambiguity is unhelpful in an authoritative contributing guide for this repository and compounds confusion from the table listing diff functions as current. |
||
|
|
||
| [ADR 0045](../ADRs/0045-forge-portable-harness-schema.md) introduced `base:` | ||
| composition and its inverse. The ADR's Consequences section states that | ||
| `DiffHarness` was "removed with the scaffold agent extraction." The diff | ||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [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. |
||
| See also: [Issue #662](https://github.com/guyoron1/fullsend/issues/662) | ||
| tracks a broader harness field integration checklist covering expansion, | ||
| environment construction, composition carry-forward, and security pipelines. | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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.