ConfigKit/docs/roadmap/remediation.md

Sequence Implementation Review Remediation

Status: remediation in progress. Completed findings are marked below; this record introduces no new release gate.

Reviewed commit: 0be7cdb00eff576fda6b30c0600b2134d4e2e086. Implementation range: bc6b977..0be7cdb. Authority: the sequence roadmap, implementation plan, ADR 0017, and ADR 0018.

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: [x] Complete.

Completion: override metadata now reads the actual target subtree through one shared lookup. Ordinary/layered regressions preserve retained values, including zero and false, their explicit origins, and absence-based resolver behavior. Verified go test ./internal/engine -run 'Override|CLI' -count=1 and ./scripts/check.sh.

Owner: applyDecodedOverrides, 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:

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: [x] Complete.

Completion: sequence origin copying now separates incoming and effective paths for precedence and inheritance. A regression covers reordered matches, appended records, composite origins, and actual resource reads from distinct declaring directories. Verified focused origin/inheritance/overlay/provenance tests, go test -race ./internal/engine, and ./scripts/check.sh.

Owners: applyOverlaySequenceEvidence, particularly lines 369 and 381, and the analogous sequence-origin copying in 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: [x] Complete.

Completion: per-source history now composes strict item projections with the tier operation, including nested application prefixes and local sequence replay. Regressions cover all three operations, multiple peers, retained records, removed fields, source attribution, and a subsequent overlay without a false decoding event. Verified focused history tests, go test -race ./internal/engine, and ./scripts/check.sh.

Owner: applyOverlayWithCollectionPolicy, 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: [x] Complete.

Completion: layered assembly projects incoming source destinations and version classification evidence with the same item assignments used for values/history. Earlier contributions retain their historical coordinates, physical identities and contexts. Main/underlay regressions cover reordered upserts, append, replace, nested helpers, and source-local version evidence. Verified focused destination, origin and source-version tests, focused race coverage, and ./scripts/check.sh.

Owners: projectAcquiredDestinations and the sequence-operation handoff in assembleLayeredInput.

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: [x] Complete.

Completion: inheritance resolves identity against actual child-value presence, discarding parent YAML identity for an explicit replacement while preserving agreement for retained values and authoritative application constraints. Added policy-state and public-load regressions for changed/unkeyed/non-sequence replacements, application agreement/conflict, and mode-only/append/upsert cases. Verified focused inheritance tests and ./scripts/check.sh.

Owners: inheritanceCollectionResolution and mergeInheritanceCollectionPolicies, 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:

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: [x] Complete.

Completion: copying a mapping now visits nested collection operations with absent lower values, preserving ordinary ancestor removal and whole-record semantics. Incoming record-owned policy transfers through the same operation. Regressions cover absent/empty/null ancestors, identity and kind failures, empty and valid initialization, and later inheritance using an imported item's nested identity. Corrected an older test that deferred the required error until a later tier. Verified focused merge/collection/inheritance tests, focused ownership race coverage, and ./scripts/check.sh.

Owners: mergeRecursiveMappings and mergeOverlayMappingsWithCollectionPolicy.

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:

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: [x] Complete.

Completion: shared identity lookup now distinguishes structural records from effective payload/reference mappings. Final validation includes literal extends fields while structural source checks still exclude actual directives. Facade regressions cover defaults and recorded resolution, string/null components, and duplicate rejection. Verified those regressions and existing structural identity and inheritance coverage through ./scripts/check.sh.

Owners: validateFinalSequenceIdentities and newSequenceMappingIndex.

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: [x] Complete.

Completion: layered assembly returns nonfatal main-version diagnostics on success and alongside later failures; the shared caller stops only for errors. Regression coverage verifies ordinary/layered warning parity, exactly one checker call, and warnings retained with later structural or decoding errors. Verified through ./scripts/check.sh, including existing source-version ordering coverage.

Owner: assembleLayeredInput, 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: [x] Complete.

Completion: definition validation retains original one-based registration positions and names both positions in each overlap diagnostic. No input path or new public diagnostic structure is invented. A mixed-selector regression verifies multiple conflicts even after an earlier invalid registration is skipped. Verified registration/facade coverage through ./scripts/check.sh.

Owner: compileSequenceIdentityPolicy, 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: [x] Complete.

Completion: the public definition comment now describes final registered-identity validation and its shape/lifetime boundaries. Selected-overlay prose explicitly separates ordinary replacement from collection policy and links to the canonical operation/presence tables. Verified against the implementation, local-link tests, and git diff --check; no runtime behavior changed.

Owners: Definition.SequenceIdentities, lines 18–21, and the selected-overlay contract.

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, 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.