Prepare release notes for ConfigKit 1.1.0
This commit is contained in:
@@ -1,290 +0,0 @@
|
||||
# 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. 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](../../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:** 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](../../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:** 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](../../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.
|
||||
|
||||
## 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](../../internal/engine/resolution_correspondence.go),
|
||||
`entryPath` at lines 96–125 and `finalPath` at lines 129–156; consumers include
|
||||
[metadata refresh](../../internal/engine/load_metadata.go), 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](../../internal/engine/resolution_compare.go),
|
||||
`compareResolutionValues` at lines 64–69; compare the existing decorated sort in
|
||||
[shared structural comparison](../../internal/engine/document_compare.go),
|
||||
`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.
|
||||
Reference in New Issue
Block a user