79 lines
4.1 KiB
Markdown
79 lines
4.1 KiB
Markdown
# Resolution provenance remediation
|
|
|
|
Review baseline: `715364b`. Reviewed implementation commits `88f21d0` through
|
|
`715364b` against `818d36f`. Status: finding resolved.
|
|
|
|
## R1 — Avoid copying unused history into the resolution entry view
|
|
|
|
Status: resolved. Priority: medium.
|
|
|
|
### Evidence and impact
|
|
|
|
At the review baseline, `newResolutionInput` in [load_metadata.go](../../load_metadata.go) called
|
|
`cloneMetadata(metadata)` before adding its effective-path index. `cloneMetadata`
|
|
deep-copies `metadata.history`, including each `Change` and its nested records,
|
|
as well as any historical-path index. At this lifecycle boundary,
|
|
`metadataFromDecoded` and `applyOverlayMetadata` have already populated history.
|
|
|
|
The entry view only exposes `Metadata.Lookup` and source-ID lookup. Neither query
|
|
uses history. Every load with a resolver therefore duplicates historical records
|
|
that are unnecessary for the requested entry-state queries, including records for
|
|
displaced/default paths that the view cannot query. This adds allocation and copy
|
|
work proportional to retained history even for a resolver that never calls the
|
|
new methods. No wall-clock or memory threshold was measured or is asserted here.
|
|
|
|
The Stage 5 completion record in [implementation.md](implementation.md) also states
|
|
that the view "does not retain history"; the constructor contradicts that claim.
|
|
|
|
### Correction
|
|
|
|
Construct the entry view from the effective field/path information and source
|
|
records needed by its queries, avoiding the unused deep copy of history. A small
|
|
field-only copy using the existing record clone helpers is one straightforward
|
|
solution. Immutable sharing or another ownership arrangement is also acceptable
|
|
if it preserves the agreed stable queries and detached public returns.
|
|
|
|
Do not change `cloneMetadata` globally to drop history: final result metadata
|
|
still requires it. Do not alter public APIs, weaken detachment, introduce resource
|
|
caps, or make a particular representation/complexity bound a public guarantee.
|
|
Correct the completion record to describe the verified implementation.
|
|
|
|
This finding concerns demonstrably unused work, not a requirement for eager
|
|
copying, zero allocations, or a new security restriction.
|
|
|
|
### Acceptance and validation
|
|
|
|
- Inspection of the entry-view construction confirms it no longer deep-copies
|
|
history solely for these queries.
|
|
- Existing public tests still prove fixed entry information during normalization,
|
|
moves, and deletions; detached field/source returns; and correct final history.
|
|
- Update the completion record only after verifying its claims. Existing behavioral
|
|
tests are sufficient unless the chosen fix exposes a new risk; do not freeze a
|
|
private struct layout in a test just to prove this optimization.
|
|
- Run `go test . -run 'Resolution|Metadata|Provenance|Historical|Explain'`,
|
|
`./scripts/check.sh`, and `go test -race ./...`. Use the full release checker
|
|
when preparing a candidate under the release procedure.
|
|
|
|
## Review results and boundaries
|
|
|
|
Resolution: the entry constructor now copies only effective `FieldInfo` records
|
|
using `cloneFieldInfo`, builds the existing effective-path index, and retains its
|
|
detached source records. It no longer invokes `cloneMetadata`; final-result
|
|
metadata cloning and history are unchanged. The Stage 5 completion record has
|
|
been corrected. No public API, resource limit, or security restriction was added.
|
|
|
|
Post-fix validation passed on Linux with Go 1.26.7: the focused resolution,
|
|
metadata, provenance, historical, and explanation tests; `./scripts/check.sh`;
|
|
and `go test -race ./...`.
|
|
|
|
The full Linux release checker passed on `715364b` using Go 1.25.5 and Go 1.26.7,
|
|
including race tests, consumer examples, documentation links, and cross-builds.
|
|
No functional provenance, detachment, credential-timing, or application-policy
|
|
restriction regression was identified in this review. This is not native macOS
|
|
release evidence or publication approval.
|
|
|
|
The warning-and-retain fallback in the maintained Notarius-shaped example is
|
|
explicitly documented as application policy, not a new library requirement.
|
|
The existing callback-lifetime and redaction contracts remain authoritative.
|
|
No production code or existing tests were changed during review.
|