Record sequence implementation review findings

2026-10-23 23:30:27 +00:00
parent 0be7cdb00e
commit af80ccb90c
1 changed files with 329 additions and 0 deletions
--- a/docs/roadmap/remediation.md
+++ 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.