82 lines
4.3 KiB
Markdown
82 lines
4.3 KiB
Markdown
# Notarius Cache Reuse Review Remediation
|
|
|
|
Status: Open
|
|
|
|
Review scope: commits `5086596` through `a0312ed`, compared with the accepted
|
|
cache roadmap and implementation plan. The core cache integration works,
|
|
including the real Notarius v0.7.0 integration test. Address the following
|
|
bounded findings; no broader cache redesign is required.
|
|
|
|
## 1. P2 — Preserve Relative Workspace Support at the Snapshot Boundary
|
|
|
|
Status: Complete. Snapshot materialization and post-run verification normalize
|
|
their local session/cache roots to absolute paths. Regression coverage proves
|
|
stable absolute adapter bindings, successful production-adapter launch, absence
|
|
of the duplicated directory tree, and rejection of a symlink on the actual
|
|
cache path for a relative workspace.
|
|
|
|
Location: `internal/stage/extract_references.go`, especially
|
|
`extractReferenceSnapshotDestination` and `checkExtractSnapshotDirectory`;
|
|
`internal/artifacts/paths.go` snapshot path construction.
|
|
|
|
The new snapshot destination inherits `SessionPaths.CacheDir` verbatim. A
|
|
relative workspace root therefore produces a relative reference binding,
|
|
whereas the previous extract implementation explicitly converted its snapshot
|
|
root to an absolute path. The Notarius adapter requires absolute reference
|
|
paths and rejects the new binding before launch. Configuration does not require
|
|
an absolute workspace root.
|
|
|
|
There is a related path-composition defect: `checkExtractSnapshotDirectory`
|
|
passes an already session-prefixed relative destination to
|
|
`pathsafe.SlashRelativeFromRoot`, which interprets relative targets as relative
|
|
to the supplied root. The resulting directory walk duplicates the session
|
|
prefix instead of checking the actual destination ancestry. Subsequent copying
|
|
can create the intended directories through a different path, leaving the
|
|
wrong extra directory tree and bypassing the intended component checks there.
|
|
|
|
Evidence: a temporary focused test created a valid prepared party reference,
|
|
expressed the session/cache roots relative to the working directory, and called
|
|
the production snapshot materializer. It returned a binding beginning
|
|
`work/campaign-a/.../cache/notarius/references/.../party.yml`, not an absolute
|
|
path. The diagnostic test was removed after the review.
|
|
|
|
Required change:
|
|
|
|
- Normalize the session root and snapshot destination consistently to absolute
|
|
paths before filesystem checks and adapter delivery, preserving the existing
|
|
meaning of relative workspace configuration.
|
|
- Ensure the component walk checks the same path subsequently read/written.
|
|
- Do not fix this by requiring operators to configure absolute workspace roots
|
|
or by changing global path semantics unnecessarily.
|
|
- Add a regression using a relative workspace, real snapshot materialization,
|
|
and the adapter validation boundary. Verify absolute stable bindings across
|
|
invocations, no duplicated session-prefix directory, and rejection of an
|
|
unsafe component on the actual cache path.
|
|
|
|
## 2. P3 — Remove the Accidental Reference-Version Requirement
|
|
|
|
Location: `docs/integrations/notarius.md`, Invocation section, line 28.
|
|
|
|
The documentation now says Narratio requires the Notarius v0.7.0 CLI contract
|
|
when references are configured. References were already supported under the
|
|
documented v0.6.0 contract. The implementation plan selected v0.7.0 as the
|
|
tested cache integration baseline and explicitly did not require rejecting
|
|
older compatible executables. The revised sentence unnecessarily suggests an
|
|
upgrade is mandatory for existing reference-only use.
|
|
|
|
Required change: distinguish the tested v0.7.0 cache integration baseline from
|
|
the reference CLI contract. Document required capabilities for the selected
|
|
flags without claiming a new unconditional minimum version. Keep runtime
|
|
behavior unchanged; do not add a version probe or gate.
|
|
|
|
## Validation Recorded
|
|
|
|
- `GOWORK=off go test ./...` passed.
|
|
- Race tests for stage, app, config, artifacts, and Notarius adapter passed.
|
|
- `GOWORK=off go vet ./...` passed.
|
|
- The opt-in `TestNotariusCacheReuseIntegration` passed against the previously
|
|
built local v0.7.0 binary with its local test provider, covering actual reuse,
|
|
reference-change misses, partial recovery, and forced chunk refresh.
|
|
- The additional relative-path diagnostic failed as described above; this case
|
|
was not covered by the existing absolute-path fixtures.
|