Record performance implementation review findings
This commit is contained in:
@@ -1,82 +1,119 @@
|
||||
# ReleaseKit adoption review findings
|
||||
# Performance optimization review findings
|
||||
|
||||
Reviewed commits `1917dcf` through `f887fac` against the
|
||||
[adoption roadmap](releasekit.md) and [implementation plan](implementation.md).
|
||||
The selected module, profile, validation sequence, and tag-only publication
|
||||
match the intended architecture. Address the following documentation 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.
|
||||
|
||||
## 1. Preserve the captured endpoint during manual recovery (P2)
|
||||
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.
|
||||
|
||||
Status: Complete. The runbook uses the captured endpoint and checks Git's URL
|
||||
resolution before querying refs. Its documented command returned the exact tag
|
||||
in a temporary fixture and rejected a rewritten endpoint before querying refs.
|
||||
The older, completed ReleaseKit review record remains in Git history at
|
||||
`6c62226:docs/roadmap/remediation.md`.
|
||||
|
||||
Affected: [release procedure](../release.md#5-recover-from-a-failed-or-uncertain-tag-push),
|
||||
lines 180–199.
|
||||
## 1. Complete parsed-path reuse in sensitivity collection (P2)
|
||||
|
||||
The manual recovery command queries a literal URL without checking whether Git
|
||||
rewrites it. ReleaseKit v0.2.0's own recovery procedure verifies that the captured
|
||||
endpoint still resolves to itself before querying it. Without that check, the
|
||||
documented query can inspect a different repository and falsely establish that
|
||||
the published tag is absent. The runbook then permits deletion of the retained
|
||||
local tag and a retry based on that incorrect conclusion.
|
||||
Status: Open.
|
||||
|
||||
Review reproduced this with two temporary bare repositories: the actual endpoint
|
||||
contained the published annotated tag; an `insteadOf` rewrite sent the runbook's
|
||||
query to a repository with the same `main` commit but no tag. The query succeeded
|
||||
and reported only `main`.
|
||||
Affected: [secret.go](../../internal/engine/secret.go), `addSecretPath`
|
||||
(lines 85–89); implementation Stage 9, item 3.
|
||||
|
||||
Use the endpoint captured in the publication report and follow ReleaseKit's
|
||||
existing URL-resolution check before querying its refs, for example:
|
||||
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.
|
||||
|
||||
```sh
|
||||
authoritative='COPY_CAPTURED_ENDPOINT'
|
||||
release_version='vMAJOR.MINOR.PATCH'
|
||||
test "$(git ls-remote --get-url -- "$authoritative")" = "$authoritative" &&
|
||||
git ls-remote --refs -- "$authoritative" refs/heads/main "refs/tags/$release_version"
|
||||
```
|
||||
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.
|
||||
|
||||
A failed equality check or query leaves the result unresolved. Retain the
|
||||
existing bounded recovery and confirmed-absence requirements. Verify this
|
||||
correction with a temporary URL-rewrite fixture; it adds no recurring release
|
||||
gate or ConfigKit-owned recovery implementation.
|
||||
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. Remove the unsupported prohibition on `go tool` (P3)
|
||||
## 2. Correct the unindexed-sensitivity regression fixture (P2)
|
||||
|
||||
Status: Complete. The runbook retains the built-executable workflow and limits
|
||||
its exit-status explanation to `go run`. A fresh `go tool` check preserved
|
||||
ReleaseKit's status 2 and publication diagnostics.
|
||||
Status: Open.
|
||||
|
||||
Affected: [release procedure](../release.md#1-select-go-and-prepare-the-tool),
|
||||
lines 84–86.
|
||||
Affected: [secret_propagation_test.go](../../internal/engine/secret_propagation_test.go),
|
||||
`TestSecretDependencyClosureMatchesFixedPointScan`, the
|
||||
“unindexed seed only activates inherited relations” case (lines 38–48).
|
||||
|
||||
The runbook prohibits both `go run` and `go tool` on an exit-status rationale.
|
||||
That rationale applies to `go run`, but the review's Go 1.26.8 invocation
|
||||
`GOTOOLCHAIN=local GOPROXY=off go tool releasekit publish invalid` returned
|
||||
ReleaseKit's status 2 and its publication diagnostics unchanged. The blanket
|
||||
`go tool` prohibition unnecessarily extends the selected native-executable
|
||||
workflow into an unsupported restriction.
|
||||
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.
|
||||
|
||||
Keep the documented temporary executable build and publication commands.
|
||||
Remove `go tool` from the prohibition and explain the exit-status limitation
|
||||
specifically for `go run`. No additional execution mode or test matrix is needed.
|
||||
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.
|
||||
|
||||
## 3. Reconcile completed implementation status (P3)
|
||||
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.
|
||||
|
||||
Status: Complete. The roadmap and implementation plan now identify the migration
|
||||
as complete, and the roadmap describes fixture verification as completed.
|
||||
Completed documents remain available for retirement during release preparation.
|
||||
## 3. Finish moving reference-only machinery out of production (P3)
|
||||
|
||||
Affected: the adoption roadmap's opening status and final migration-verification
|
||||
bullet, and the implementation plan's opening status.
|
||||
Status: Open.
|
||||
|
||||
All seven stage headings are marked complete, but the plan still says
|
||||
"Status: Planned" and the roadmap says isolated fixture verification remains
|
||||
unfinished. The final two commits identify that verification as completed;
|
||||
this review also reproduced validation failure without a tag, successful
|
||||
annotated-tag publication with each selected gate once, and wrong-endpoint
|
||||
rejection before any Go validation or tag creation.
|
||||
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`.
|
||||
|
||||
Mark the documents complete and describe the verification as completed.
|
||||
Keep retirement during release preparation under the existing release policy.
|
||||
Do not add a permanent migration-verification gate.
|
||||
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.
|
||||
|
||||
Reference in New Issue
Block a user