ConfigKit/docs/roadmap/remediation.md

Sequence Composition Review Remediation

Status: open. Reviewed implementation 2051177..0858af1 (0858af1029d07d93a5aa2e4f26cbec61fcfb3a51) against the feature roadmap, implementation plan, and accepted ADR 0014.

The root-owned facade, explicit placement, definition-owned registrations, item/helper scopes, and strict-versus-precedence distinction are implemented. The findings below concern correctness and evidence at their boundaries, plus targeted scaling and documentation gaps. They do not call for new features, stricter final identity validation, broader version policy, or new release gates.

1. Project consumed version evidence through authored sequence appends — P1

• Addressed and regression-tested.

Completion: retained sensitivity paths now project declarations and ancestors through assigned authored-list ranges without changing source-local evidence. Public regressions cover imports and grouped underlays, leaf/container masking, and unclassified declarations. Focused source-version tests, the everyday check, and go test -race ./internal/engine passed.

Owners: projectAcquiredDestinations / projectSourceVersionEvidence in imports.go, and evidence classification in source_version.go and secret.go.

Version evidence is projected for explicitly item-placed sources and their helpers, but not for an ordinary source containing an authored registered list. Its retained source-local path can contain an index shifted by composition. Concatenating only evidence.destination and that path classifies the wrong effective record and can publish sensitive declaration text.

Confirmed reproduction: main contains records: [{id: main, version: main}] and imports a mapping containing records: [{id: child, version: private-version-canary}]. Register /records by id, select required source-version checks at Keys("records").Index(0).Key("version"), and classify /records/1/version as sensitive. Loading succeeds and consumes the child's header, but its successful SourceInfo.VersionCheck exposes the canary with Redacted: false. Its record is at index 1; evidence was classified at index 0.

Required change: project internal evidence paths, including applicable ancestors, through assigned authored-sequence ranges as well as item scopes. Keep public VersionCheck paths and physical locations source-local. Preserve original semantic shapes and existing conservative failure/callback disclosure; do not solve this by masking every declaration or changing source-version policy.

Acceptance: cover an imported authored list and the corresponding grouped underlay handoff. Sensitive shifted evidence is redacted; an unclassified declaration remains inspectable. Existing item/helper evidence tests still pass.

2. Preserve source chronology when expanding item history — P2

• Addressed and regression-tested.

Completion: structural fragments use their local singleton index; item body and helper events are expanded beside their declaring contribution. Mapping history queries ignore unrelated sibling fields. A public regression covers preceding authored records, two item sources, nested helpers, field origins, and root traversal order. Focused facade/history tests and the everyday check passed.

Owner: projectNestedEvidence in imports.go, especially lines 592–608.

The structural source fragment is indexed with the effective append offset: node.Sequence[start]. That fragment is still a source-local singleton, so a later item's start > 0 prevents clearing its completed body. History therefore retains helper values in the outer application and subsequently replays the same item body and helper values again. Nested applications are also appended after all outer applications rather than beside their declaring contribution.

Confirmed reproduction: a manifest imports one.yml and two.yml as items at registered /records. Each item declares its own ordinary helper supplying only text. The second item's Explain(/records/1/text) reports three events, including an unchanged-value event attributed to the item root that never authored text; the first item's explanation has only its helper contribution. Root history visits source IDs 1,2,4,2,3,4,5, placing the second item ahead of the first item's helper despite depth-first acquisition 1,2,3,4,5.

Required change: distinguish local indexes from effective offsets when constructing structural fragments. Expand each item's retained applications in source traversal order, preserving physical field origins without replaying a completed body as an additional source contribution. Do not replace individual source history with an opaque completed-scope snapshot.

Acceptance: use at least two item sources with helpers and a preceding authored-list contribution. Helpers alone define their fields in explanations; root/container history is chronological, final values/origins agree, and nested items retain the same guarantees.

3. Record removals caused by registered-sequence replacement — P2

• Addressed and regression-tested.

Completion: replacement queries include missing descendants and use the replacing container's origin. Public regressions verify shorter lists, empty lists, and removed fields; existing append-relevance tests remain green. Focused facade tests and the everyday check passed.

Owner: applicationOperationPath in history.go, lines 134–149.

Query relevance filters every sequence operation to the incoming assigned range, including sequenceHistoryReplace. This is appropriate for an append but not a replacement: replacement also removes records outside the incoming range. Replay removes them correctly while Explain silently omits the removal event.

Confirmed reproduction: a strictly composed underlay supplies records [{id: one}, {id: two}]; main supplies [{id: main}] at the same registered path. Explain(/records/1/id) says the value is absent, but its only history event defines two in the underlay. No main-tier removal appears. Replacing with [] has the same problem. Existing cross-cutting tests check absence/redaction but do not assert that the removing operation is actually recorded.

Required change: make replacement relevant to removed descendants and attribute those transitions to the replacing container/source when an incoming leaf no longer exists. Keep append relevance limited to newly contributed items.

Acceptance: explanations show present-to-absent transitions and the correct replacing origin for shorter/empty lists and removed fields; ordinary same-tier appends do not falsely claim to change earlier records.

4. Honor contextual parsed version-path indexes — P2

• Addressed and regression-tested.

Completion: protected-path sequence traversal uses the existing canonical contextual index interpreter. Public import and grouped-underlay parity tests cover parsed and explicitly indexed rules; existing version-path tests preserve numeric-key intent and invalid index behavior. Focused tests and the everyday check passed.

Owner: fragmentVersionClaimProjected in directives.go, lines 190–194.

Sequence traversal accepts only pathSegmentIndex. Canonical numeric tokens from public ParsePath are contextual parsed segments, accepted by the existing version lookup and other semantic-path operations. The new protection code instead treats them as a claim on the entire ancestor sequence.

Confirmed reproduction: use the two-source required-version fixture from finding 1, without the sensitivity predicate. A rule built with Keys("records").Index(0).Key("version") succeeds. The equivalent ParsePath("/records/0/version") fails with import.version, even though the import appends to index 1 and does not claim main's index 0 header.

Required change: use existing contextual sequence-index interpretation while preserving explicit mapping-key versus index intent and canonical token rules. Do not narrow VersionRule to programmatically indexed paths or relax real ancestor-replacement protection.

Acceptance: equivalent parsed/indexed rules give equivalent ownership results across imports and grouped underlays, with existing literal numeric-key, wrong-container, and noncanonical-index cases preserved. The current divergence is an unintended restriction beyond the requested feature.

5. Keep default-mode main version ownership inside item scopes — P2

• Addressed and regression-tested.

Owner: the mainIsItem branch of imports.go, lines 228–235.

The helper ownership check applies versionState.localRule directly to item-local documents. In default mode that rule is an effective application path, not a local item path, so the check misses a helper's protected contribution.

Confirmed reproduction: load main as an item at /records, enable imports, and configure the default-mode rule at Keys("records").Index(0).Key("version"). Main contains only id plus an ordinary helper import. The helper supplies version: imported-version. Loading succeeds and the main checker receives the helper's version, although imported fragments are not allowed to supply the protected main version path.

Required change: translate the protected effective path into the item scope before checking its individual helper contributions, or check correctly projected contributions at the enclosing boundary. Retain default effective-path interpretation and enabled-mode source-local checking/header consumption.

Acceptance: the fixture fails with import.version in both public entry points. Helpers outside the actual protected path remain permitted, and enabled source-version-mode behavior is unchanged. Do not add a general ban on fields named version.

6. Preserve absence for ordinary defaults-only loads — P2

• Addressed and regression-tested.

Owner: executeLoadRequest in load.go, lines 210–216, and compositionForInputWithPolicy in load_input.go.

Ordinary loads now unconditionally initialize a composition, including when NoSource() supplied no document. Its synthetic empty mapping is assigned back to input.document and decoded as explicit input. This changes callers that have not opted into either feature.

Confirmed reproduction: Load[map[string]any](NoSource()) reports Metadata().IsExplicit(Keys()) == true, while the equivalent LoadLayered call reports false. No source contributed a root definition. This violates the existing NoSource absence distinction rather than adding useful feature behavior.

Required change: retain the absent-input handoff when no real contribution exists, while still validating actual singleton registered sequences. Preserve synthetic assembly only where existing overlay/CLI behavior needs it.

Acceptance: ordinary/layered defaults-only loads agree on root presence and origin, with and without registrations/default factories; an actual empty source mapping is still explicit. Defaults-only overrides keep their existing behavior.

7. Remove new quadratic scans from sequence construction — P2

• Addressed and behaviorally validated.

Owners: compose.go, especially validateRegisteredSequences and application construction; range lookups in imports.go; sequenceHistoryAt in history.go.

Identity comparison uses a map, but surrounding new machinery reintroduces avoidable nonlinear work:

• Validation looks each sequence claim up again with unindexed documentAtPath. One source with N sibling registered sequences performs N linear root-mapping searches: O(N²) at fixed depth and constant record size.
• Application construction scans all ranges for each source: O(S × R). Item evidence/source projection repeats full range scans for each contribution.
• sequenceHistoryAt linearly scans the application's sequence descriptors repeatedly during retained-tree traversal and replay. A wide document with many registered sequence paths similarly accumulates quadratic work.

These are structural findings; no timing benchmark claim is made. They diverge from the plan's near-linear construction objective and the user's explicit efficiency priority, without requiring that public behavior change.

Required change: retain encountered node references or reuse existing indexed document lookup; group/index ranges by source and path once; use a small owned per-application path lookup where repeatedly needed. Keep order in slices and ownership unchanged. Avoid a new global cache/index framework or unrelated performance refactoring.

Acceptance: static review no longer finds these nested whole-set scans on the new construction path. Existing ordering, evidence, cloning, and behavior tests pass; optional scaling measurements may help but are not a new gate or timing-threshold test requirement.

8. Do not fabricate index zero for item-local conflict diagnostics — P2

• Addressed and regression-tested.

Owner: visitItemSource in imports.go, lines 519–525.

Helper composition errors are rebased with destination.Index(0) before the item's actual range is assigned. Nested rebasing also omits an enclosing item index. Physical locations are retained, but the application path points at a different record.

Confirmed reproduction: main authors records: [{id: first}] and imports an item with id: second whose ordinary helper also defines id. The conflict is reported at /records/0/id, incorrectly naming the existing first record; the imported contribution would be at index 1.

Required change: carry scoped diagnostic paths until they can be projected through the enclosing assignment, rather than injecting a fabricated index. Preserve safe failure source evidence and actual physical conflicting locations; do not require publishing a usable partial result to compute a diagnostic.

Acceptance: a helper conflict after preceding authored/imported records names the correct item, and nested item conflicts retain enclosing semantic indexes. The recently added malformed-registered-record regression must continue to pass.

9. Correct the public identity-validation phase statement — P3

• Documentation corrected.

Owner: Definition.SequenceCompositions comment in lifecycle.go, line 20.

The comment says repeated identities and conflicting registrations both fail option validation. Only invalid/conflicting registrations are preflight errors; duplicate identities in input are compose.conflict after source acquisition and applicable helper/header processing. The declaration comment inadvertently promises an earlier failure phase than the approved design or implementation.

Required change: distinguish registration preflight from record validation during strict composition. Correct the comment rather than moving data checks earlier or adding a new pre-acquisition guarantee. Keep the canonical configuration contract and declaration comments consistent.

Review validation

Passed on the reviewed tree with Go go1.26.9 linux/amd64:

• GOTOOLCHAIN=local ./scripts/check.sh
• GOTOOLCHAIN=local GOPROXY=off go test -race ./...
• git diff --check 2051177..HEAD

The behavioral findings were independently reproduced through public API calls using disposable in-memory filesystems in a temporary Go program outside the repository. The scaling finding is based on the inspected loops and lookup implementation, not host timing. No production code, tests, dependencies, release machinery, or downstream repositories were modified during this review.