From af80ccb90c21f350c28ec4bb0e394c8b22139389 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Fri, 23 Oct 2026 23:30:27 +0000 Subject: [PATCH] Record sequence implementation review findings --- docs/roadmap/remediation.md | 329 ++++++++++++++++++++++++++++++++++++ 1 file changed, 329 insertions(+) create mode 100644 docs/roadmap/remediation.md diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md new file mode 100644 index 0000000..9629c35 --- /dev/null +++ b/docs/roadmap/remediation.md @@ -0,0 +1,329 @@ +# Sequence Implementation Review Remediation + +Status: eleven open findings. This is a review record, not an implementation +change or a new release gate. + +Reviewed commit: `0be7cdb00eff576fda6b30c0600b2134d4e2e086`. +Implementation range: `bc6b977..0be7cdb`. +Authority: the [sequence roadmap](sequences.md), +[implementation plan](implementation.md), +[ADR 0017](../adr/0017-collection-sequence-policy.md), and +[ADR 0018](../adr/0018-cli-after-structural-resolution.md). + +The public facade, shared identity/composition machinery, scoped policies, +identified-record inheritance, post-decode CLI phase, and final registered +identity validation are present. The findings below concern remaining +correctness, contract-alignment, documentation, and scaling gaps. + +Validation on the reviewed commit: + +- `./scripts/check.sh` passed with Go 1.26.10 on Linux. +- `go test -race ./...` passed. +- Public-API probes outside the repository reproduced findings 1–9. The probes + used labeled byte sources, defaults, and an in-memory filesystem; they did not + change implementation files or use external services. +- Finding 10 follows directly from the maintained text; finding 11 follows from + the prefix-scan loop, not a timing comparison or performance threshold. + +P1 denotes the highest-priority correctness issue here; P2 denotes normal-priority +work needed to complete the selected behavior. Preserve the agreed scope while +fixing these issues. In particular, do not resolve failures by restricting valid +identity fields, requiring repeated declarations, changing merge semantics, or +adding limits. + +## 1. Preserve Retained Explicit Values In Nested CLI Mapping Assignments — P1 + +Status: [ ] Open. + +Owner: [`applyDecodedOverrides`](../../internal/engine/override_decode.go), +especially lines 59–60 and the presence/origin update helpers. + +The helpers receive the complete configuration projection as `existing`, but +interpret it as the subtree at the override path. A partial nested mapping +therefore treats retained input children as absent and drops their presence and +origins. `fillDefaultOrigins` subsequently labels those retained values as defaults. + +Reproduction: load the following into ordinary nested Go structs: + +```yaml +settings: + nested: + a: 1 + b: 2 +``` + +Apply an override at `[settings]` with value +`map[string]any{"nested": map[string]any{"a": 3}}`. The typed value of `b` +initially remains 2, but `Metadata.Lookup([settings,nested,b])` reports +`Explicit: false` and a default origin. Calling `Default` for that field in +application resolution then changes the explicitly supplied 2 to the fallback +value. This violates presence-based defaulting, not merely presentation. + +Required remediation: use the actual existing target subtree when updating +presence/origins, preserving retained descendants and removing genuinely replaced +ones. Keep override batches detached and preserve the selected post-decode order. +Add regressions for nested partial mappings, retained zero/false values, metadata, +and an absence-based resolver default that must not overwrite retained input. + +## 2. Read Upsert Origins At Input Paths And Write Them At Output Paths — P2 + +Status: [ ] Open. + +Owners: [`applyOverlaySequenceEvidence`](../../internal/engine/overlay.go), +particularly lines 369 and 381, and the analogous sequence-origin copying in +[`inheritance_collection.go`](../../internal/engine/inheritance_collection.go). + +The overlay helper supplies an output-index path to an origin lookup whose keys +are still incoming-index paths. When those positions differ and another incoming +record occupies the output index, metadata takes that other record's source +location. The same defect was reproduced through mapping-sibling inheritance; +its helper uses the same single-path copying pattern. + +Reproduction: an underlay supplies IDs `[a,b]`; the main source declares `key: id` +and supplies `[b*,a*]`. The result is correctly `[a*,b*]`, but the value origin +for final `a*` is the source line of `b*`, and vice versa. In the probe the two +value lines were 7 and 9; final index 0 incorrectly reported line 7 instead of 9. +With separately authored records this can select a different source/context. + +Required remediation: use the operation's input-to-output projection for origin +subtree reads as well as writes. Preserve composite contributing origins, not +just node-location fallbacks. Test reordered upserts and appended records with +distinct source files/locations, including nested fields, inheritance operations, +and declaring resource contexts. + +## 3. Apply Tier Sequence Operations To Historical Contributions — P2 + +Status: [ ] Open. + +Owner: [`applyOverlayWithCollectionPolicy`](../../internal/engine/overlay.go), +lines 76–84, and existing sequence-history projection/replay. + +For composed tiers, `applications` is nonempty. This branch appends the tier's +original strict-composition applications without incorporating the newly computed +precedence sequence descriptors. Replay consequently performs a whole-list +replacement or strict append rather than the actual cross-tier upsert/append. +The later decoding reconciliation conceals the mismatch by attributing a +structural combination to typed defaults/decoding. + +Reproduction: lower `[a,b]` and higher `[b*,a*]` correctly yield `[a*,b*]`. +`Explain([items])` instead shows the main contribution producing `[b*,a*]`, then +a `decoding` event reordering it to `[a*,b*]` with the reason "applied retained +defaults and canonical typed decoding". No decoder performed that reordering. + +Required remediation: project each retained per-source application through the +tier's selected operation while keeping peer composition distinct from tier +precedence. Do not collapse authored sources or fake decoding events. Cover +upsert, append, unmatched retained records, reordered matches, removed fields, +multiple higher-tier peers, and a subsequent selected overlay. Replay should +agree with structural assembly before real decoding reconciliation. + +## 4. Reproject Nested Source Destinations After Cross-Tier Item Movement — P2 + +Status: [ ] Open. + +Owners: [`projectAcquiredDestinations`](../../internal/engine/imports.go) and the +sequence-operation handoff in [`assembleLayeredInput`](../../internal/engine/load_request.go). + +Source destinations are projected while composing their own tier, but are not +reprojected when that tier's records move to accumulated-list positions. Nested +helper source records can therefore point at a different final record. + +Reproduction: lower `items` contains `[a,b]`. Main imports a single `b` record +with `placement: sequence-item` into `[items]`; that record imports a helper +mapping supplying its value. Upsert retains `a` at index 0 and places the higher +`b` at index 1. `Sources()` nevertheless reports the surviving helper's destination +as `/items/0`, its tier-local position, rather than `/items/1`. + +Required remediation: carry the sequence operation's projection through nested +source-destination/evidence updates at precedence boundaries. Keep physical +source identity and passive resource contexts unchanged, and preserve historical +meaning for displaced contributions rather than relabeling them as survivors. +Test a retained lower record followed by an imported higher record, reordered +matches, and nested helpers. Check version-evidence projection where applicable. + +## 5. Honor Identity Reset On Actual Inheritance Replacement — P2 + +Status: [ ] Open. + +Owners: [`inheritanceCollectionResolution` and `mergeInheritanceCollectionPolicies`](../../internal/engine/inheritance_collection.go), +especially lines 79–80 and 121. + +These paths reconcile the parent's YAML identity before accounting for actual +replacement, and treat the parent's collection as retained even when the child +supplies a replacing value. This is stronger than the selected identity lifetime +and differs from source-tier replacement. + +Reproduction: + +```yaml +configkit: + sequences: + - destination: [base, items] + key: id + - destination: [child, items] + key: name + mode: replace +base: + items: + - id: a + value: parent +child: + extends: base + items: + - name: b + value: child +``` + +Loading fails with `sequence.conflict` at `/child/items`. It should succeed: +actual whole-sequence replacement may discard the parent's YAML identity and +establish `name`. Authoritative application constraints must still apply. + +Required remediation: distinguish actual replacement from a mode-only declaration +before identity agreement and transfer. Test a changed key, replacement with no +new key becoming unkeyed, permitted non-sequence replacement, and an agreeing or +conflicting application registration. Preserve rejection of identity changes +without a replacement value and during append/upsert. + +## 6. Enforce Collection Operations Beneath Newly Added Ancestors — P2 + +Status: [ ] Open. + +Owners: [`mergeRecursiveMappings`](../../internal/engine/recursive_merge.go) and +[`mergeOverlayMappingsWithCollectionPolicy`](../../internal/engine/overlay.go). + +When an incoming ancestor mapping is absent below, the recursive merger clones +the entire subtree and invokes only its ancestor-level added hook. Nested +collection resolutions are not visited. This bypasses their operation rules; +the subsequent declaration-only policy update does not replace those checks. + +Reproduction: combine a lower `{}` with this main tier: + +```yaml +configkit: + sequences: + - destination: [group, items] + mode: append +group: + items: + - id: a +``` + +Loading succeeds despite append having no identity definition. Placing the same +sequence directly at `[items]` correctly fails. The validity of an operation +must not depend on whether an enclosing mapping already existed. + +Required remediation: account for applicable nested collection policies when +adding/copying an ancestor subtree, including owned-policy transfer. Test absent +and present enclosing mappings, append/upsert without identity, explicit empty +lists, invalid non-sequence contributions, and valid identified initialization. +Do not turn this into a general path-existence or shape constraint: ordinary +ancestor replacement/removal retains its documented meaning. + +## 7. Validate Final Typed Identity Fields As Payload, Not Directives — P2 + +Status: [ ] Open. + +Owners: [`validateFinalSequenceIdentities`](../../internal/engine/final_identity.go) +and [`newSequenceMappingIndex`](../../internal/engine/sequence_identity.go). + +The final validator reuses structural record lookup, which excludes every +`extends` entry. At this phase there is no structural processing: typed defaults +and application resolution can produce ordinary payload with that literal name. +The roadmap expressly distinguishes structural-control exclusion from final +typed-payload validation; this reuse introduces an extra restriction. + +Reproduction: a defaults-only `Catalog` with `Items []map[string]any` containing +`{"extends": "application-data"}`, registered with +`SequenceIdentityAt(Keys("items"), "extends")`, fails with a missing identity +field at `/items/0/extends`, even though the effective field is present. + +Required remediation: distinguish structural-payload lookup from final payload +lookup. Preserve rejection when a source tries to use an actual consumed +directive as identity. Test final fields supplied by defaults and recorded +resolution, including a null component, without adding a broader name ban. + +## 8. Retain Nonfatal Main-Version Diagnostics In Layered Loads — P2 + +Status: [ ] Open. + +Owner: [`assembleLayeredInput`](../../internal/engine/load_request.go), lines 143–152. + +The moved default-mode version-check call returns its diagnostics only when they +contain errors. Warning-only diagnostics are discarded. Ordinary loading still +retains the same warnings. + +Reproduction: a `VersionRule.Check` that calls +`validation.Warning(Keys("version"), "test warning")` and returns nil yields one +`version.warning` through ordinary `Load`, but zero diagnostics through +`LoadLayered` with the same main input and default source-version mode. + +Required remediation: carry warnings from the single main-tier check into the +shared diagnostic accumulator on successful layered loads. Preserve checker +ownership, invocation count, and the selected acquisition/composition ordering. +Add ordinary/layered parity and warning-plus-later-error regressions. + +## 9. Identify Conflicting Application Registrations In Diagnostics — P2 + +Status: [ ] Open. + +Owner: [`compileSequenceIdentityPolicy`](../../internal/engine/sequence_registration.go), +lines 119–128. + +Incompatible dynamic overlaps produce only a generic `options.invalid` message, +an empty configuration path, and no related information. Neither registration +is identified. The roadmap requires definition-time errors to identify the +conflicting registrations without inventing a concrete input destination. + +Reproduction: register `collections -> AnyMappingKey -> items` using `id`, and +`collections -> foo -> items` using `name`. Even with `NoSource`, the error is +only "overlapping sequence registrations must use the same identity fields". +With multiple registrations the caller cannot identify the conflicting pair. + +Required remediation: retain original registration positions and identify both +in safe definition diagnostics, for example through positional message context. +No new public diagnostic structure is required solely for this. Test mixed exact +and dynamic registrations and multiple conflicts, without asserting complete +English wording or exposing identity values. + +## 10. Remove Stale Public Contract Descriptions — P2 + +Status: [ ] Open. + +Owners: [`Definition.SequenceIdentities`](../../lifecycle.go), lines 18–21, and +the [selected-overlay contract](../config.md#selected-overlays). + +The exported definition-field comment still says registrations do not validate +identities introduced by defaults, CLI, or callbacks. Final validation is now +implemented and the registration constructors correctly document it. + +The selected-overlay table and following paragraph also still unconditionally +describe sequences as atomic replacements and `[]` as clearing. A later paragraph +describes collection operations, but does not make the earlier unconditional +claims accurate: identified collections infer upsert, and `[]` then contributes +no records rather than clearing. + +Required remediation: make the symbol comment describe its implemented final +guarantee and boundaries, and qualify the overlay table/paragraph for collection +policy. Link to the canonical collection tables rather than duplicating them. +Keep YAML-only enforcement distinct from registered final validation. + +## 11. Remove The Quadratic Prefix Scan In Item Policy Projection — P2 + +Status: [ ] Open. + +Owner: [`projectNestedCollectionPolicies`](../../internal/engine/imports.go), +especially line 968; the scanned helper is `priorSequenceLength` in the same file. + +For every item with local collection policy, the loop scans all preceding +contributions to calculate `itemIndex`, then overwrites that result when the +already-indexed sequence range is available. For N such items this performs +`0 + 1 + ... + (N-1)` contribution visits, excluding path lookup costs, despite +the common case already having the exact assigned index. This is introduced +quadratic work in the feature's independently authored-record path. + +Required remediation: use the existing assigned range first. If a genuine +fallback is necessary, maintain per-destination prefix lengths in one pass +rather than rescanning every prefix. Preserve scope and ordering semantics; +do not add limits, pooling, or a new public cache. Keep ownership/placement +regressions and add a representative many-item/local-policy benchmark only if +needed to exercise the path. A wall-clock threshold is not required.