Record sequence feature integration audit findings

2026-10-24 00:38:04 +00:00
parent 8d40983271
commit d2e20a304a
1 changed files with 243 additions and 3 deletions
--- a/docs/roadmap/remediation.md
+++ b/docs/roadmap/remediation.md
@@ -1,7 +1,8 @@
 # Sequence Implementation Review Remediation

-Status: all eleven findings complete. Each finding has a separate remediation
-commit and completion note below. This record introduces no new release gate.
+Status: findings 1–11 are complete. The subsequent integration audit at
+`8d40983271de5239312eb0a2896737be88c5abdc` identified six additional open findings,
+numbered 12–17 below. This record introduces no new release gate.

 Reviewed commit: `0be7cdb00eff576fda6b30c0600b2134d4e2e086`.
 Implementation range: `bc6b977..0be7cdb`.
@@ -25,7 +26,7 @@ Validation on the reviewed commit:
 - Finding 10 follows directly from the maintained text; finding 11 follows from
  the prefix-scan loop, not a timing comparison or performance threshold.

-Final validation after remediation: `./scripts/check.sh` and
+Validation after the initial remediation: `./scripts/check.sh` and
 `go test -race ./...` passed with Go 1.26.10 on Linux. The original public-API
 probes for findings 1–9 now demonstrate the required corrected behavior.

@@ -407,3 +408,242 @@ 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.
+
+## Integration Audit Baseline And Scope
+
+Reviewed commit: `8d40983271de5239312eb0a2896737be88c5abdc`.
+
+This follow-up reviews the interaction of collection policy, identified-record
+inheritance, dynamic registrations, and post-decode CLI assignment with existing
+source placement, source-version checking, typed decoding, inspection, resource
+contexts, and ownership boundaries. The accepted ADRs and current configuration
+contract remain the authority; the findings require integration corrections,
+not additional consumer restrictions or new feature choices.
+
+Validation:
+
+- `./scripts/check.sh` and `go test -race ./...` pass with Go 1.26.10 on Linux.
+- Isolated public-API probes outside the repository reproduce findings 12–15.
+  They use labeled byte sources and an in-memory filesystem, with no external
+  services, real credentials, or repository implementation changes.
+- Finding 16 follows from the per-item deep-copy loop; no timing claim or
+  performance threshold is needed. Finding 17 follows from maintained contract
+  text that contradicts the implemented collection operations.
+- Positive comparisons include sequence-valued identity replacement, ancestor
+  retirement through source tiers, quoted versions after item movement, and an
+  append overlay that preserves the main record's version.
+
+## 12. Complete Identity Retirement Across Replacement Shapes — P2
+
+Status: [ ] Open.
+
+Owners: [`resolveStrictCollectionPolicies`](../../internal/engine/compose.go),
+especially its `sequencePresent` reset condition;
+[`mergeInheritanceCollectionPolicies`](../../internal/engine/inheritance_collection.go);
+and ancestor-replacement handling in
+[`applyInheritanceScopeTemplates`](../../internal/engine/inheritance_schedule.go).
+
+Policy agreement and retirement still treat some actual replacements as retained
+collections. This has three observable effects under the same selected lifetime
+rule:
+
+1. Source precedence accepts a replacement sequence with a new YAML key, but
+   rejects the equivalent replacement by null or a scalar. For example, a lower
+   tier declaring `key: id` for `[items]` with `items: [{id: old}]`, followed by
+   a main tier declaring `key: name, mode: replace` with `items: null`, fails with
+   `sequence.conflict` at `/items`. Replacing null with `42` also fails; replacing
+   it with `[{name: new}]` succeeds. Null/scalar replacement should discard the
+   lower identity and retain the newly declared key as pending policy, subject
+   to the application's schema and constraints.
+2. Removing an ancestor while declaring a new pending key for its removed
+   descendant fails before the replacement occurs. A lower
+   `group: {items: [{id: old}]}` with key `id`, followed by `group: null` and a
+   root declaration of key `name` for `[group, items]`, fails at `/group/items`.
+   The analogous `base`/`child` inheritance case fails at `/child/group/items`.
+   The old collection is being removed rather than retained.
+3. Inheritance can resurrect removed identity even without a conflicting new
+   declaration. This input fails with a missing `id` at
+   `/grandchild/group/items/0/id`:
+
+   ```yaml
+   configkit:
+     sequences: [{destination: [base, group, items], key: id}]
+   base: {group: {items: [{id: old}]}}
+   child:
+     extends: base
+     group: null
+   grandchild:
+     extends: child
+     group: {items: [{name: new}]}
+   ```
+
+   The same remove/reintroduce operations through source tiers succeed. The
+   inherited descendant identity remains in policy state after its enclosing
+   mapping is removed, so the grandchild incorrectly receives the old key.
+
+Required remediation: account for actual value and ancestor replacement before
+deciding which lower identity participates in agreement, and retire removed
+descendant/record-owned state during inheritance as during source precedence.
+Retained inheritance templates must not reestablish discarded identity later.
+Preserve authoritative application constraints, independently owned mode
+persistence, pending declarations, and rejection of key changes when values are
+genuinely retained. Cover direct non-sequence replacement, ancestor removal with
+a new pending key, and removal followed by reintroduction across both contexts.
+Do not require repeated keys or narrow permitted null/scalar shapes to avoid
+these cases.
+
+## 13. Keep Metadata History In Post-Decode CLI Order — P2
+
+Status: [ ] Open.
+
+Owners: [`historyWithInheritance`](../../internal/engine/inheritance_history.go)
+and the source/CLI metadata handoff in
+[`preparePostDecodeOverrideContext`](../../internal/engine/override_context.go).
+
+The value explanation follows the new lifecycle, but `Metadata.History` still
+inserts inheritance after stored CLI changes. `historyWithInheritance` splits
+only at `LayerResolution`, reflecting the former CLI-before-inheritance order.
+The reconstructed source view can also append an authored source origin after
+the CLI event.
+
+Reproduction: load `base: {value: parent}` and
+`child: {extends: base, value: authored}`, then override `[child, value]` with
+`"cli"`. The final value and `Metadata.Lookup` correctly report CLI provenance.
+`Explain` reports source → inheritance → CLI. `Metadata.History` instead reports
+source → CLI → source → inheritance and ends with a source/inheritance origin.
+This violates chronological metadata history and disagrees with the effective
+origin and the value explanation.
+
+Required remediation: assemble metadata history in the implemented source,
+inheritance, CLI, and resolution order, preserving the real origins and
+dependencies of those events. Cover ordinary/layered loading, mapping and record
+inheritance, equal-valued CLI writes, and a later recorded resolver change at the
+appropriate existing test owners. Do not impose a new exact event-count or
+message-format contract.
+
+## 14. Carry Main-Version Coordinates Through Sequence Movement — P2
+
+Status: [ ] Open.
+
+Owners: [`effectiveSourceVersionRuleForPlacement`](../../internal/engine/source_version.go),
+the main-tier projection handoff in
+[`assembleLayeredInput`](../../internal/engine/load_request.go), and use of
+the effective rule in [`executeLoadRequest`](../../internal/engine/load.go).
+
+Source destinations and version-evidence sensitivity paths follow item movement,
+but the rule used for integer-version decoding retains the original item index.
+Consequently an accepted integer header can fail decoding after an otherwise
+valid upsert placement.
+
+Reproduction with the public API:
+
+- Main input is `id: main` and `version: 4`, placed as one sequence item at
+  `Keys("items")`.
+- A lower input contributes the sequence item `id: lower` at the same destination.
+- Register identity `id` for `[items]`, select `SourceVersionModeIfPresent`, and
+  use `VersionRule.Path = Keys("version")` with an accepting checker.
+- Decode into records with `ID string` and `Version string`.
+
+The checker accepts `4` exactly once. Upsert places the main record at index 1,
+but decoding fails with `decode.type` at `/items/1/version`. The integer-to-string
+exception is still tied to `/items/0/version`. Quoting the identical version as
+`'4'` succeeds. A source-local indexed header path, without item placement,
+reproduces the same problem when its record moves during upsert.
+
+Required remediation: carry the main declaration's effective coordinates through
+the existing item projections into decoding and audit the associated ownership
+protection handoffs for the same stale-coordinate assumption. Preserve
+source-local checker/evidence paths, checker invocation count, main-only
+ownership, and the narrow integer-to-string exception. Cover moved existing and
+newly appended main records, integer/string parity, and protection of the actual
+main header after movement. Do not prohibit supported indexed paths or mounted
+main records as a workaround.
+
+## 15. Exclude Parent Identity From Inherited History Payload — P2
+
+Status: [ ] Open.
+
+Owners: inheritance operation construction in
+[`applyInheritanceScopeTemplates`](../../internal/engine/inheritance_schedule.go),
+[`appendInheritanceExplanation`](../../internal/engine/inheritance_explain.go),
+and transferred-origin projection in
+[`inheritance_history.go`](../../internal/engine/inheritance_history.go).
+
+Identified-record value construction excludes the parent's identity fields, but
+the retained history operation still supplies the complete parent record and
+its identity origins to the older mapping-inheritance replay machinery.
+
+Reproduction:
+
+```yaml
+configkit:
+  sequences: [{destination: [items], key: id}]
+items:
+  - id: base
+    value: parent
+  - id: child
+    extends: {id: base}
+```
+
+The effective child ID and origin are correct. However,
+`Explain(Keys("items").Index(1).Key("id"))` reports `child` → `base` → `child`,
+attributing the intermediate value to an inheritance operation and the parent's
+ID location. `Metadata.History` likewise claims an inherited parent-ID
+contribution. That identity is expressly excluded from inherited payload; it was
+never an applied value at the child path.
+
+Required remediation: make history replay and transferred-origin projection use
+the same identity exclusion as value construction. Preserve the child's authored
+identity and presence, ordinary inherited payload history, and the parent record's
+own source history at its original path. Cover compound identities and identity
+fields alongside inherited payload at both record and ancestor queries; keep the
+existing inspection API and sensitivity behavior.
+
+## 16. Avoid Deep-Copying Accumulated Item Policies Per Record — P2
+
+Status: [ ] Open.
+
+Owners: [`projectNestedCollectionPolicies`](../../internal/engine/imports.go)
+and its use of copying `collectionPolicyScope.at` / `set` helpers in
+[`collection_policy_state.go`](../../internal/engine/collection_policy_state.go).
+
+The prefix-scan correction in finding 11 is present, but a separate quadratic
+cost remains on the same independently authored-record path. For every imported
+item with local policy, `at` deep-clones the destination's accumulated record
+order and every retained record's nested scope. After adding one owner, `set`
+deep-clones the growing policy again. With N fixed-size item-local policies at
+one destination, this copies approximately `1 + 2 + ... + N` retained scopes,
+even when every assigned sequence range is available.
+
+This follows directly from the loop and `cloneCollectionPolicy`; no busy-host
+timing comparison is required. It is an integration cost from reusing detached
+read/write helpers within private incremental construction, rather than from
+the required final ownership copies.
+
+Required remediation: construct each destination's owned record-policy state
+without repeatedly cloning all earlier records, retaining necessary copies at
+ownership boundaries. Preserve record ordering, distinct owners, sparse assigned
+positions, fallback offsets, and cross-load detachment. Reuse the existing item
+policy and ownership tests; add allocation/scaling evidence only where useful.
+Do not introduce public caches, pooling, new caps, or performance thresholds.
+
+## 17. Reconcile Legacy Sequence-History Contract Text — P2
+
+Status: [ ] Open.
+
+Owner: the metadata/provenance and explanation sections of
+[`docs/config.md`](../config.md#layered-metadata).
+
+The metadata section still says that a later tier supplying a sequence records
+replacement at the sequence path and that history does not imply identity-based
+merging across tiers. That describes the former unconditional replacement model,
+not the implemented append/upsert history or the collection operation tables in
+the same document. The explanation paragraph about atomic sequences also needs
+review so readers can distinguish snapshot/diff granularity from collection
+combination semantics.
+
+Required remediation: describe history for the selected collection operation and
+link to the canonical operation tables. Preserve the existing atomic sequence
+snapshot/diff behavior and whole-record upsert semantics; correcting the prose
+does not authorize recursive record-field merging, a new query API, or broader
+history guarantees.