From 87986a12d229bbb4455133892534c17fa4278f2b Mon Sep 17 00:00:00 2001
From: Eric Rakestraw <eric@rakestraw.us>
Date: Sat, 17 Oct 2026 01:50:02 +0000
Subject: [PATCH] Record sequence composition review findings

---
 docs/roadmap/remediation.md | 282 ++++++++++++++++++++++++++++++++++++
 1 file changed, 282 insertions(+)
 create mode 100644 docs/roadmap/remediation.md

diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md
new file mode 100644
index 0000000..0d8458e
--- /dev/null
+++ b/docs/roadmap/remediation.md
@@ -0,0 +1,282 @@
+# 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
+
+- [ ] Addressed and regression-tested.
+
+**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
+
+- [ ] Addressed and regression-tested.
+
+**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
+
+- [ ] Addressed and regression-tested.
+
+**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
+
+- [ ] Addressed and regression-tested.
+
+**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
+
+- [ ] Addressed and regression-tested.
+
+**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
+
+- [ ] Addressed and regression-tested.
+
+**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
+
+- [ ] Addressed and behaviorally validated.
+
+**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
+
+- [ ] Addressed and regression-tested.
+
+**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.