160 lines
8.2 KiB
Markdown
160 lines
8.2 KiB
Markdown
# Sequence resolution implementation review
|
||
|
||
Reviewed on 2026-11-02 against the [feature roadmap](resolution-sequences.md),
|
||
[implementation plan](implementation.md), and
|
||
[ADR 0021](../adr/0021-sequence-resolution-correspondence.md).
|
||
|
||
Scope: commits `1cee127..b066090` plus the pre-existing working-tree changes,
|
||
including final documentation, an additional sensitivity regression, and the
|
||
sensitivity fixed-point loop in `internal/engine/secret.go`. The latter loop is
|
||
not present in `b066090`; Finding 3 specifically concerns the reviewed working
|
||
tree. Findings 1 and 2 also affect the committed implementation. Existing work
|
||
was preserved; this review changes no production code or tests.
|
||
|
||
The architecture now separates resolution comparison from the atomic comparator
|
||
used by public diff. A private entry-to-final correspondence for registered
|
||
collections feeds auditing, snapshots, metadata refresh, and shared history
|
||
selection/replay. The main design is in place, but the following issues remain.
|
||
|
||
## Finding 1 — Metadata history rejects a valid matched historical field
|
||
|
||
**Priority:** P2. **Status:** Open.
|
||
|
||
**Code:** [engine metadata](../../internal/engine/metadata.go),
|
||
`Metadata.History` at lines 50–55; compare
|
||
[shared history selection](../../internal/engine/history_select.go),
|
||
`selectHistory` at lines 43–49.
|
||
|
||
`selectHistory` accepts a query whose matched entry coordinate has an observed
|
||
shape, even if its final coordinate never had that field. `Metadata.History`
|
||
still requires the requested coordinate itself to have been observed before
|
||
calling that selector. Consequently metadata history and explanation disagree
|
||
on a removed descendant of a moved record.
|
||
|
||
Reproduction using `Items []map[string]any`, with an application registration
|
||
of `id` at `/items`:
|
||
|
||
```yaml
|
||
items: [{id: a, extra: own}, {id: b}]
|
||
```
|
||
|
||
During resolution, delete `extra` from `a`, swap the records, and record a
|
||
normalization at `/items`. Query `/items/1/extra`, which follows surviving
|
||
record `a` back to entry position 0.
|
||
|
||
**Expected:** both inspection APIs retain `a`'s earlier field evidence and its
|
||
removal. Effective lookup remains absent. The roadmap and plan explicitly cover
|
||
removed descendants of surviving records through correspondence.
|
||
|
||
**Actual, verified through the public API:** `Explain` succeeds with two history
|
||
events, including a resolution transition from `own` to absence, while
|
||
`Metadata.History` returns an empty slice. The requested physical path did not
|
||
exist on either endpoint, although its matched entry path did.
|
||
|
||
**Remediation:** make metadata history use the shared selector's correspondence-
|
||
aware eligibility rather than its stronger preliminary shape check. Preserve
|
||
unknown-path and explicit key/index-shape handling. Add a focused regression
|
||
where the old occupant of the final position never had the removed field;
|
||
the existing removed-field test gives both original records that field and does
|
||
not expose this inconsistency. This requires no synthetic historical-path
|
||
enumeration or new public addressing mechanism.
|
||
|
||
## Finding 2 — Deep descendant history reports the old record's operation path
|
||
|
||
**Priority:** P2. **Status:** Open.
|
||
|
||
**Code:** [shared history selection](../../internal/engine/history_select.go),
|
||
`selectHistory`, especially the reported-path translation at lines 116–120.
|
||
|
||
Selection follows the correct entry record, but reported-path translation uses
|
||
the queried leaf and a depth heuristic rather than the matched record prefix.
|
||
An operation reported at an entry record or other ancestor more than one level
|
||
above the queried leaf can retain its old index. That `Change.Path` is neither
|
||
the final queried path nor its ancestor. This diverges from Stage 7's requirement
|
||
to project affected paths into the final coordinate while preserving original
|
||
source locations separately.
|
||
|
||
Verified reproduction with registered `id` identity:
|
||
|
||
```yaml
|
||
items:
|
||
- id: parent
|
||
nested: {more: {value: old}}
|
||
- id: child
|
||
extends: {id: parent}
|
||
```
|
||
|
||
Swap the records during resolution, record `/items`, then inspect
|
||
`Metadata.History(/items/0/nested/more/value)`. The child is now at position 0.
|
||
|
||
**Expected:** the inherited operation's reported path identifies the affected
|
||
region under final `/items/0`, or its actual common container ancestor; source
|
||
`Location` coordinates continue to describe the original input.
|
||
|
||
**Actual:** history contains a main-layer inherited operation reported at
|
||
`/items/1`, followed by the resolution operation at `/items`. The first reported
|
||
path now identifies the other record. Source/value matching alone does not
|
||
repair this public path inconsistency.
|
||
|
||
**Remediation:** translate reported paths using the actual entry/final item-prefix
|
||
pair, preserving the operation's relative suffix and ancestor scope. Distinguish
|
||
entry-coordinate source/inheritance paths from resolution records that already
|
||
use final coordinates, so the latter are not translated twice. Verify a deep
|
||
descendant of a moved inherited record, a containing operation, and an unchanged
|
||
common collection ancestor. Avoid substituting the queried leaf for every
|
||
operation path solely to satisfy an ancestor check.
|
||
|
||
## Finding 3 — Sensitivity propagation rescans the whole collection quadratically
|
||
|
||
**Priority:** P2. **Status:** Open.
|
||
|
||
**Code:** [secret classification](../../internal/engine/secret.go),
|
||
`classifySecretPathsWithEvidence`, working-tree lines 83–122; associated
|
||
[correspondence translations](../../internal/engine/resolution_correspondence.go).
|
||
|
||
The working-tree fix repeatedly projects every currently sensitive path in both
|
||
directions through the permutation, merges those paths into the same positional
|
||
mask, and rebuilds dependency closure until no mask changes. Each pass scans all
|
||
candidate paths and reruns `closeSecretDependencies` from scratch.
|
||
|
||
For an `n`-record cyclic rotation with one initially sensitive field and no
|
||
dependency edges, the first pass reaches neighboring positions. Subsequent
|
||
passes treat those newly projected positions as fresh entry/final seeds and
|
||
advance around the cycle. There are Theta(n) passes over Theta(n) paths, giving
|
||
Theta(n²) work at fixed record width and depth. This does not require a dense
|
||
dependency graph or unusually deep configuration. The plan called for endpoint
|
||
projection from entry evidence and explicitly cautioned against treating item
|
||
matching as arbitrary transitive mutation edges.
|
||
|
||
A public-API probe with entry identities `[a, b, c, d]`, final identities
|
||
`[d, a, b, c]`, and only `/items/0/value` predicate-classified confirms that all
|
||
four final values become masked. Some masking at reused historical coordinates
|
||
is deliberately conservative; the issue is reusing projected positional masks
|
||
as further correspondence seeds and rescanning the complete population. This
|
||
finding does not introduce a promise to disclose more values or weaken the
|
||
existing historical sensitivity contract.
|
||
|
||
**Remediation:** retain the protection provided by the additional
|
||
`TestMovedDependencyProtectsEarlierMatchedValue` regression, including backward
|
||
protection of earlier matched values after dependency classification. Represent
|
||
entry/final sensitivity correspondence explicitly and process newly classified
|
||
states with indexed dependency/matching edges or another bounded worklist,
|
||
rather than rebuilding full closure on every projection pass. Keep original
|
||
historical-coordinate masks and required conservative redaction. Check cyclic
|
||
rotations and interactions with dependencies without performance thresholds or
|
||
tests of incidental traversal counts. Reverting to a single incomplete
|
||
projection pass would lose the dependency-history protection the working-tree
|
||
change was added to provide.
|
||
|
||
## Validation performed
|
||
|
||
- The current working tree passed `./scripts/check.sh` with Go 1.27.0 on Linux.
|
||
- `go test -race ./...` passed with the same toolchain.
|
||
- Separate disposable programs outside the repository reproduced Findings 1
|
||
and 2 through the public facade and confirmed the cyclic masking behavior
|
||
described in Finding 3. Its complexity follows from the implemented loop;
|
||
no timing benchmarks were collected.
|
||
- The review found no additional public API restrictions or earlier identity
|
||
validation requirements. Shared atomic diff comparison, callback entry views,
|
||
registered selector scope, and YAML-only identity lifetime remain preserved.
|