diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md index 926c92f..949225a 100644 --- 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.