Retire completed sequence composition plans
This commit is contained in:
@@ -1,353 +0,0 @@
|
||||
# Sequence Composition Review Remediation
|
||||
|
||||
Status: complete; all nine findings addressed in separate validated commits.
|
||||
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
|
||||
|
||||
- [x] Documentation corrected.
|
||||
|
||||
Completion: the public declaration distinguishes registration preflight from
|
||||
input identity validation during strict composition, matching the existing
|
||||
configuration contract. The final everyday check and full race suite passed.
|
||||
|
||||
**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.
|
||||
|
||||
## Remediation validation
|
||||
|
||||
All nine findings have completion evidence above. Final validation passed with
|
||||
Go `go1.26.9 linux/amd64`:
|
||||
|
||||
- `GOTOOLCHAIN=local ./scripts/check.sh`
|
||||
- `GOTOOLCHAIN=local GOPROXY=off go test -race ./...`
|
||||
- Local documentation-link and whitespace checks.
|
||||
|
||||
The original external public-API reproductions were rerun against the completed
|
||||
changes: consumed sensitive evidence is redacted, helper events are chronological
|
||||
and source-specific, replacements explain removals, parsed/indexed version rules
|
||||
agree, imported main-version claims fail, NoSource stays absent, and item conflict
|
||||
paths identify the correct records. Static review and the wide-collection
|
||||
regression verify the targeted lookup changes without adding performance
|
||||
thresholds. No dependencies, public controls, release gates, or downstream
|
||||
repositories were changed by remediation.
|
||||
Reference in New Issue
Block a user