From 27be6bab2333f4c945e6601b8e43cb97e663cbe2 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Mon, 2 Nov 2026 16:36:30 +0000 Subject: [PATCH] Record sequence resolution review findings --- docs/roadmap/remediation.md | 159 ++++++++++++++++++++++++++++++++++++ 1 file changed, 159 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..aff889f --- /dev/null +++ b/docs/roadmap/remediation.md @@ -0,0 +1,159 @@ +# 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.