Files
configkit/docs/roadmap/remediation.md

16 KiB
Raw Blame History

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. All five findings below have now been addressed. Original audit observations are retained for context; completion notes describe the fixes.

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 were still open at that audit; Finding 3 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: Complete.

Completion: metadata history now delegates correspondence-aware eligibility to the shared selector. A regression verifies agreement with explanation for a removed field absent from the previous final-slot occupant, while effective lookup, unknown paths, and explicit key/index distinctions remain correct. The regression failed before the fix; focused tests and ./scripts/check.sh passed after it.

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: Complete.

Completion: pre-resolution operation paths now use the matched record-prefix translation, preserving ancestor scope and suffixes. Resolution records already in final coordinates are left unchanged. A regression covers deep inherited fields, record/container queries, original source coordinates, and overlapping final-position records. Focused history checks and ./scripts/check.sh passed.

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: Complete.

Completion: matching edges and real dependencies now share one indexed work queue. Candidate coordinates are registered once, newly reached historical coordinates are registered on demand, and each trigger fires at most once; classification no longer rebuilds closure in full-map passes. Conservative historical masking is retained. The existing earlier-matched-value regression is retained in this fix, and a cyclic rotation regression covers removed fields and dependency-derived values. Focused sensitivity tests and ./scripts/check.sh passed.

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: Complete.

Completion: eligible collection prefixes now use the existing prefix index. Translation formats the requested pointer once, retains concrete path-kind checks, and copies the resulting path independently. No-eligible cases return positionally without formatting prefixes; no-registration loads skip matching construction entirely. Cloning rebuilds an independently owned index. Existing matching/history/ownership coverage and a numeric-key/escaped-key regression passed, along with ./scripts/check.sh.

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: Complete.

Completion: both comparison operations share the existing decorated delta sort, which constructs one pointer per delta and preserves canonical pointer ordering and the kind tie-break. Existing comparison and diff coverage passed, along with ./scripts/check.sh and the final go test -race ./... run. No performance thresholds, persistent cache, or new public contract were introduced.

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.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.
  • 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.String implementation, 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.

Remediation validation

All five issues were addressed sequentially in separate nonempty commits, each with its completion note and relevant verification. Final validation passed ./scripts/check.sh and go test -race ./... with Go 1.27.0 on Linux. The development check includes formatting, whitespace, tests, local documentation links, vet, and build checks. No benchmark thresholds or release gates were added.

The directly related pending sensitivity regression was retained with Finding 3. Unrelated pre-existing documentation and public-comment changes were preserved outside the remediation commits.