Record sequence composition review findings
This commit is contained in:
282
docs/roadmap/remediation.md
Normal file
282
docs/roadmap/remediation.md
Normal file
@@ -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.
|
||||
Reference in New Issue
Block a user