13 KiB
Sequence resolution implementation review
Reviewed on 2026-11-02 against the feature roadmap, implementation plan, and ADR 0021.
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.
A follow-up integration and algorithm audit at 27be6ba, including the same
pre-existing working-tree changes, added Findings 4 and 5. It preserved the
decided comparison, identity, ordering, and history policies. Findings 1–3 remain
open; Finding 3 already covers the principal collection-size quadratic loop and
is not duplicated below.
Finding 1 — Metadata history rejects a valid matched historical field
Priority: P2. Status: Open.
Code: engine metadata,
Metadata.History at lines 50–55; compare
shared history selection,
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:
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,
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:
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,
classifySecretPathsWithEvidence, working-tree lines 83–122; associated
correspondence translations.
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.
Finding 4 — Correspondence lookup repeatedly serializes growing path prefixes
Priority: P2. Status: Open.
Code: correspondence translations,
entryPath at lines 96–125 and finalPath at lines 129–156; consumers include
metadata refresh, resolution record
validation/snapshots, shared history queries, diagnostic locations, and sensitivity.
Both translation methods search for a containing collection by constructing and
serializing successively longer prefixes of the requested path. For an unmatched
path with d constant-width segments, prefix serialization performs work
proportional to 0 + 1 + ... + (d-1), or Theta(d²), per lookup. Each nonempty
serialization also allocates a pointer string. The final owned path copy only
needs Theta(d) work. This prefix search is not an algorithm selected by the
roadmap or plan.
This also affects ordinary configurations with no registered collections:
entryPath still formats every prefix even when collections is empty.
applyResolutionMetadata calls it while seeding every final field and again
while refreshing explicitness. Thus a resolution callback introduces this cost
across ordinary metadata, not only when records actually move. On a chain of
depth d, the lookups at all levels contribute Theta(d³) prefix-serialization
work, compared with Theta(d²) for linear lookup at each level. Existing depth
limits bound the magnitude; this is an avoidable extra depth factor within those
limits, not a claim of unbounded supported input depth.
Remediation: return an owned positional path immediately when there is no
eligible correspondence. For registered scopes, use a segment-prefix index, or
serialize the requested pointer once and query the existing pathPrefixIndex
for matching ancestors. Retain concrete path-kind verification so numeric keys
and sequence indexes remain distinct, and preserve additions, removals, and
wholly absent historical-position behavior. Because registration selectors do
not pass through sequence indexes, the relevant matching record prefix does
not require an exhaustive scan of every collection. This can make translation
linear in encoded path length while retaining the current compact pair tables.
Also avoid the preparatory correspondence tree walk when the compiled
application policy contains no registrations. Verify behavioral equivalence
through existing translation, shape, history, and ownership tests rather than
adding timing thresholds or exact allocation assertions.
Finding 5 — Delta sorting formats pointers inside every comparison
Priority: P3. Status: Open; lower-priority efficiency improvement.
Code: resolution comparison,
compareResolutionValues at lines 64–69; compare the existing decorated sort in
shared structural comparison,
compareConfigurationValues.
The new resolution comparator invokes Path.String() on both operands during
every sorting comparison. With k deltas, comparison sorting can invoke this
Theta(k log k) times. For pointer length L, that contributes Theta(k L log k)
serialization work and Theta(k log k) non-root pointer allocations in the worst
case. This is newly avoidable overhead; the existing shared comparator already
formats one pointer per delta before sorting.
Remediation: follow the existing decorate/sort/unwrap pattern: compute each delta's pointer once, sort by the retained pointer and existing kind tie-break, then return the original semantic paths. Pointer construction becomes Theta(k L) with Theta(k) pointer allocations. Sorting still performs O(k log k) comparisons with normal string-comparison costs; this does not claim a better worst-case bound for string sorting itself. Keep canonical ordering as expressly selected in the plan. Reuse ordering tests; no alternate sort policy, persistent cache, benchmark gate, or API change is needed.
Validation performed
- The current working tree passed
./scripts/check.shwith 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.
- The follow-up traced identity-table construction, resolution comparison, coverage and snapshot handoffs, metadata refresh, history selection/replay, and sensitivity propagation. Endpoint matching uses tuple lookup tables rather than pairwise record searches. Existing whole-tree explanation replay and canonical ordering were retained as planned choices, not reconsidered.
- Findings 4 and 5 follow directly from the loops,
Path.Stringimplementation, and existing prefix-index/decorated-sort alternatives. No new tests or benchmarks were needed; prior successful checks apply to the unchanged production and test files. The amended document's links and whitespace were checked separately.