configkit/docs/roadmap/remediation.md

# Performance optimization review findings

Reviewed `5279330..6c62226838a3658e3802ed2926e9273fbd7c34ea` against the
[performance audit roadmap](performance-audit.md) and the
[implementation plan](implementation.md). The private prefix indexes, worklist
sensitivity closure, frozen inspection lookups, prepared replay, and ordered
membership maps broadly match the selected architecture. Whole-tree replay and
the expressly deferred ownership/trace changes remain unchanged.

The review found no reproduced functional regression or unintentionally stronger
public contract. The following bounded implementation and test-quality findings
remain to be addressed. They do not authorize changes to public semantics,
dependencies, resource limits, release gates, or credential handling.

The older, completed ReleaseKit review record remains in Git history at
`6c62226:docs/roadmap/remediation.md`.

## 1. Complete parsed-path reuse in sensitivity collection (P2)

Status: Complete. Parsed accumulator paths are reused, while typed historical
paths still receive the later parsed replacement. Repeated root/escaped/numeric
contributions and both numeric mapping/sequence transitions are covered.
Focused sensitivity/inheritance/rendering tests, `./scripts/check.sh`, and
`go test -race ./internal/engine` passed.

Affected: [secret.go](../../internal/engine/secret.go), `addSecretPath`
(lines 85–89); implementation Stage 9, item 3.

Temporary node indexes and the redundant initial final-tree walk were removed,
but `addSecretPath` still unconditionally calls `ParsePath` and replaces the
stored path on every visit. Paths common to multiple source applications,
baseline/decoded projections, and the final tree are therefore repeatedly split,
unescaped, and allocated even when the accumulator already holds the identical
parsed representation. This leaves an expressly selected part of candidate 5
unimplemented, despite Stage 9 being marked complete.

Reuse the existing accumulator path only when it already has the same valid
parsed representation. If history or inheritance has installed an explicit
key/index path, the later tree/pointer contribution must still replace it with
parsed segments as before. Do not skip merely because the pointer key exists,
substitute walker-derived typed segments, or change collection phase precedence.

Add focused behavioral coverage for a repeated parsed contribution and a typed
historical contribution subsequently normalized to parsed form, including numeric
mapping/sequence transitions. Reuse existing tag/predicate/ownership coverage.
No allocation threshold or traversal-count test is needed. Re-run the sensitivity,
inheritance, and rendering tests and the everyday development check, then reconcile
the completion evidence with the actual implementation.

## 2. Correct the unindexed-sensitivity regression fixture (P2)

Status: Complete. The fixture now uses canonical `/untracked` while leaving it
unindexed, and explicitly checks that `/copy` is sensitive and `/ordinary` is
not. The new assertion failed with the original malformed seed and passed after
correction. The closure equivalence test and `./scripts/check.sh` passed.

Affected: [secret_propagation_test.go](../../internal/engine/secret_propagation_test.go),
`TestSecretDependencyClosureMatchesFixedPointScan`, the
“unindexed seed only activates inherited relations” case (lines 38–48).

The seed is `map[string]bool{"untracked": true}`, but the dependency and
inheritance paths spell `/untracked`. These are different keys; the seed also
violates the canonical-pointer convention used by the new private trie. Both
algorithms leave the inherited target unclassified, so their equality does not
verify the case's intended behavior. The suite can pass without covering the
selected distinction between an unindexed classified path and an invalid seed.

Use `secretSet(Keys("untracked"))` or the literal `"/untracked"`, while keeping
that path absent from the collected paths map. Assert that the inherited target
`/copy` becomes sensitive and the ordinary target `/ordinary` does not, in
addition to the reference-set comparison. This verifies the existing filtering
difference; it does not impose a new public guarantee or require malformed-seed
validation in production.

Review exercised this corrected canonical fixture in a disposable checkout;
the current production implementation produces the intended result. The finding
is missing regression protection, not an observed sensitivity leak.

## 3. Finish moving reference-only machinery out of production (P3)

Status: Open.

Affected: [resolution.go](../../internal/engine/resolution.go),
`newResolutionCoverageIndex` (lines 373–382), `appendUniqueString`,
`appendUniquePath` (lines 439–453);
[path.go](../../internal/engine/path.go), `pathSpellingEqual`;
[diagnostic.go](../../internal/engine/diagnostic.go), `compareDiagnostic`.
Relevant tests: `resolution_index_test.go` and `path_test.go`.

After the caller migrations, the slice-based coverage constructor has only a
test caller. The old list-deduplication helpers likewise serve only the
coalescing reference/test fixtures, and `pathSpellingEqual` exists solely to
support that reference-only path helper. The standalone diagnostic comparator
wrapper has no callers; production sorting uses `compareDiagnosticWithPaths`.
Keeping these in production leaves redundant mechanisms and test scaffolding
behind after Stage 18's requested cleanup.

Move useful reference-only helpers into test files, or make the coverage test
exercise a production record/snapshot adapter directly. Keep the old reference
deduplication relation independent and String-based where it acts as an oracle.
Remove unused production wrappers and helpers once callers are accounted for.
Consolidate or retarget the obsolete helper-specific deduplication test toward
the actual resolution membership behavior; existing repeated-record coverage
may already be sufficient.

Retain required `clonePaths`, token comparison, both live coverage adapters,
immutable inspection evidence, and the legitimate unprepared-metadata history
fallback. This is a narrow cleanup, not permission to remove behavior or useful
equivalence tests. Re-run affected tests and the everyday development check.

## Review validation

- `./scripts/check.sh` passed at the reviewed commit: formatting, whitespace,
  all-package tests, vet, and build.
- `GOTOOLCHAIN=local GOPROXY=off GOWORK=off go test -race ./...` passed.
- A fresh, uncached `GOTOOLCHAIN=local GOPROXY=off GOWORK=off go test -count=1 ./...`
  also passed.
- A disposable checkout outside the repository passed
  `go test -mod=readonly ./internal/engine -run '^TestReview' -count=1` with
  the corrected canonical unindexed-seed case and 1,500 deterministic generated
  sensitivity graphs compared with the prior fixed-point procedure.
- Reviewed all changed production files and their added/modified test coverage.
  Root public API, configuration contract, adopted policies, module dependencies,
  and release profile are unchanged by the implementation commits.
- No performance measurements were required or collected. Algorithmic changes
  and retained-index memory tradeoffs are not quantified runtime guarantees.