-
Notifications
You must be signed in to change notification settings - Fork 0
docs(#841): document compose/diff bidirectional invariant #862
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
base: main
Are you sure you want to change the base?
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,101 @@ | ||
| # Harness Composition | ||
|
|
||
| When changing merge or compose functions in `internal/harness/`, you must check | ||
| whether corresponding diff functions exist and update them in lockstep. | ||
|
|
||
| ## The bidirectional invariant | ||
|
|
||
| [ADR 0045](../ADRs/0045-forge-portable-harness-schema.md) introduced `base:` | ||
| composition with field-level merge semantics. The merge functions have an | ||
| architectural inverse: diff functions that extract the delta between a composed | ||
| result and its base. 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. | ||
|
|
||
| **Why this matters:** If diff functions are re-added (for example to support | ||
| extracting overrides from a composed harness), they must produce deltas that | ||
| round-trip correctly: `compose(base, diff(composed, base))` must equal the | ||
| original `composed` result. A mismatch silently corrupts harness | ||
| customizations. PR #5450 demonstrated this failure mode: merge logic was | ||
| updated without touching the corresponding diff functions, causing a | ||
| round-trip regression that took 6 iterations to diagnose. | ||
|
|
||
| ## Current merge functions | ||
|
|
||
| These are the merge functions that define the forward direction of | ||
| composition. Any future diff function must be the inverse of the | ||
| corresponding merge function listed here. | ||
|
|
||
| ### `base:` composition (`compose.go`) | ||
|
|
||
| | Function | Purpose | Merge semantics | | ||
| |---|---|---| | ||
| | `mergeBaseIntoChild` | Top-level harness merge | Scalars: child overrides; slices: concatenated; maps: merged (child wins); pointer structs: child replaces if non-nil (except `PreflightCheck` carry-forward — see `mergeForgeConfigInto` below) | | ||
| | `mergeSkills` | Skill path deduplication | Base + child, child overrides base by basename | | ||
| | `mergeHostFiles` | Host file deduplication | Base + child, child overrides base by dest path | | ||
|
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 documentation accuracy 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. This exception is documented for mergeForgeConfigInto in the row below but not for mergeBaseIntoChild, which implements the same behavior. Suggested fix: Update the mergeBaseIntoChild merge semantics column to note the PreflightCheck carry-forward exception, e.g.: 'pointer structs: child replaces if non-nil (validation_loop carries forward preflight_check from base when child omits it)'. |
||
| | `mergeForgeBlocks` | Per-platform forge merge | Key-by-key merge; each platform uses `mergeForgeConfigInto` | | ||
|
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] 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. |
||
| | `mergeForgeConfigInto` | Per-platform forge config merge | Scalars: child overrides; skills: base + child; runner_env: merged (child wins); validation_loop: child replaces (with preflight_check carry-forward) | | ||
|
|
||
| ### Runtime forge resolution (`forge.go`) | ||
|
|
||
| | Function | Purpose | Merge semantics | | ||
| |---|---|---| | ||
| | `mergeForgeConfig` | Applies platform-specific forge config to harness at runtime | Skills: harness + forge, forge overrides harness by basename (via `mergeSkills`); runner_env: merged (forge wins); validation_loop: forge replaces entirely (no preflight_check carry-forward) | | ||
|
|
||
| Note: `mergeForgeConfig` (runtime) and `mergeForgeConfigInto` (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. [low] Technical documentation accuracy The mergeForgeConfig row describes skill merge semantics as 'Skills: appended', but the code calls mergeSkills which performs basename-based deduplication, not simple appending. If harness skills and forge skills share the same basename, the forge skill overrides the harness skill rather than producing a duplicate. Suggested fix: Update the mergeForgeConfig skills description to 'Skills: harness + forge, forge overrides harness by basename (via mergeSkills)' for consistency with the other merge function descriptions. |
||
| have different semantics. `mergeForgeConfigInto` prepends base skills and | ||
| carries forward `PreflightCheck` from base when the child omits it (see | ||
| [#5074](https://github.com/fullsend-ai/fullsend/pull/5074)). | ||
| `mergeForgeConfig` appends forge skills and replaces `ValidationLoop` entirely. | ||
| A diff counterpart would need to account for which merge function produced the | ||
| composed result. | ||
|
|
||
| ### Shared helpers | ||
|
|
||
| | Function | Location | Purpose | | ||
| |---|---|---| | ||
| | `mergeEnvFrom` | `harness.go` | Merges `EnvConfig` sub-maps independently; caller controls precedence | | ||
|
|
||
| ## Checklist for changes | ||
|
|
||
| When modifying merge or compose logic in `internal/harness/`: | ||
|
|
||
| 1. **Check whether diff functions exist.** Search for | ||
| `internal/harness/diff.go` or functions matching `Diff*`/`diff*` in the | ||
| harness package. As of this writing they do not exist (removed with the | ||
| scaffold agent extraction per ADR 0045). | ||
| 2. **If diff functions exist:** identify the paired function using the tables | ||
| above or by name pattern (`merge*` / `Merge*` maps to `diff*` / `Diff*`). | ||
| Mirror your change in the diff counterpart and verify the round-trip | ||
| property. | ||
| 3. **If diff functions do not exist:** note their absence in your commit | ||
| message and skip the remaining steps. No diff-side update is needed. | ||
| 4. **Test the round-trip.** Verify that | ||
| `compose(base, diff(composed, base))` produces the original `composed` | ||
| result. Existing tests in `compose_test.go` cover the merge side; diff | ||
| tests should follow the same patterns. | ||
| 5. **Check field-level helpers.** Changes to helpers like `mergeSkills`, | ||
| `mergeHostFiles`, or `mergeForgeConfigInto` 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. Document in your commit message why the diff side is | ||
| unaffected. | ||
|
|
||
| ## Historical context | ||
|
|
||
| [ADR 0045](../ADRs/0045-forge-portable-harness-schema.md) introduced `base:` | ||
| composition and its inverse. The diff functions (`DiffHarness` and | ||
| counterparts) were removed with the scaffold agent extraction. The | ||
| bidirectional constraint remains documented in ADR 0045 but is not visible | ||
| during normal documentation reads because ADRs are subject to immutability | ||
| policy. | ||
|
|
||
| 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.
[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.