ConfigKit/docs/roadmap/remediation.md

# Sequence Composition Review Remediation

Status: open. Reviewed implementation `2051177..0858af1`
(`0858af1029d07d93a5aa2e4f26cbec61fcfb3a51`) against the
[feature roadmap](sequence-composition.md), [implementation plan](implementation.md),
and accepted [ADR 0014](../adr/0014-sequence-placement-and-composition.md).

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

- [x] 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](../../internal/engine/imports.go), and evidence classification in
[source_version.go](../../internal/engine/source_version.go) and
[secret.go](../../internal/engine/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

- [x] 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](../../internal/engine/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

- [x] 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](../../internal/engine/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

- [x] 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](../../internal/engine/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

- [x] Addressed and regression-tested.

Completion: default-mode effective protection is translated into the main item's
local scope, preserving segment intent and effective diagnostic paths. Both
public entry points reject helper-supplied main declarations and permit unrelated
version fields. Focused source-version tests and the everyday check passed.

**Owner:** the `mainIsItem` branch of
[imports.go](../../internal/engine/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

- [x] Addressed and regression-tested.

Completion: absent ordinary input stays absent until an existing explicit layer
needs assembly. Public parity tests cover values, root presence/origin, optional
registrations/defaults, and real empty mappings. Existing defaults-only override
coverage and the everyday check passed.

**Owner:** `executeLoadRequest` in
[load.go](../../internal/engine/load.go), lines 210–216, and
`compositionForInputWithPolicy` in [load_input.go](../../internal/engine/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

- [x] Addressed and behaviorally validated.

Completion: claims retain encountered immutable nodes; history construction
groups ranges by source; scope/version projection uses source/path range
lookups; retained-history traversal and replay use per-application path indexes.
Ordered slices and detached clone ownership remain intact. Static review confirms
the listed nested scans are removed. A wide-collection replay/origin/ownership
regression, the everyday check, and `go test -race ./...` passed. No timing
threshold or release gate was added.

**Owners:** [compose.go](../../internal/engine/compose.go), especially
`validateRegisteredSequences` and application construction; range lookups in
[imports.go](../../internal/engine/imports.go); `sequenceHistoryAt` in
[history.go](../../internal/engine/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

- [x] Addressed and regression-tested.

Completion: failed item composition is projected through each enclosing scope
using preceding contributions, including prior grouped inputs. Only composition
diagnostics are rebased; physical locations and source-local access/syntax
diagnostics are retained. Regressions cover authored/imported predecessors,
nested items, item mains, grouped item sources, and failure evidence. Focused
facade/engine tests and the everyday check passed.

**Owner:** `visitItemSource` in
[imports.go](../../internal/engine/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](../../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.