diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md new file mode 100644 index 0000000..5070aca --- /dev/null +++ b/docs/roadmap/remediation.md @@ -0,0 +1,81 @@ +# 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. diff --git a/internal/stage/extract_references.go b/internal/stage/extract_references.go index 4e66303..6ccabc0 100644 --- a/internal/stage/extract_references.go +++ b/internal/stage/extract_references.go @@ -32,6 +32,11 @@ func materializeExtractReferenceSnapshots( paths artifacts.SessionPaths, references resolvedExtractReferences, ) ([]notarius.ReferenceBinding, error) { + var err error + paths, err = absoluteExtractSnapshotPaths(paths) + if err != nil { + return nil, err + } if len(references.Bindings) != len(references.Identities) { return nil, fmt.Errorf("reference bindings and identities have different lengths") } @@ -106,6 +111,11 @@ func cleanupInvalidExtractSnapshot(paths artifacts.SessionPaths, identity extrac } func verifyExtractReferenceSnapshots(paths artifacts.SessionPaths, references resolvedExtractReferences) error { + var err error + paths, err = absoluteExtractSnapshotPaths(paths) + if err != nil { + return err + } verified := make(map[string]extractReferenceIdentity, len(references.Identities)) for _, identity := range references.Identities { destination, err := extractReferenceSnapshotDestination(paths, identity) @@ -129,6 +139,21 @@ func verifyExtractReferenceSnapshots(paths artifacts.SessionPaths, references re return nil } +// Resolve both roots against the caller's working directory before comparing +// paths. A session-prefixed relative destination is not relative to Root. +func absoluteExtractSnapshotPaths(paths artifacts.SessionPaths) (artifacts.SessionPaths, error) { + var err error + paths.Root, err = filepath.Abs(paths.Root) + if err != nil { + return paths, fmt.Errorf("resolve snapshot session root: %w", err) + } + paths.CacheDir, err = filepath.Abs(paths.CacheDir) + if err != nil { + return paths, fmt.Errorf("resolve snapshot cache root: %w", err) + } + return paths, nil +} + func extractReferenceSnapshotDestination(paths artifacts.SessionPaths, identity extractReferenceIdentity) (string, error) { basename := filepath.Base(filepath.FromSlash(identity.Path)) return artifacts.SessionNotariusReferenceSnapshotPath(paths, identity.Checksum, basename) diff --git a/internal/stage/extract_test.go b/internal/stage/extract_test.go index 75a1e6e..ca73867 100644 --- a/internal/stage/extract_test.go +++ b/internal/stage/extract_test.go @@ -433,6 +433,77 @@ func TestExtractReferenceSnapshotsDeduplicateEquivalentDestinations(t *testing.T } } +func TestExtractReferenceSnapshotsWithRelativeWorkspace(t *testing.T) { + env, m, _ := setupExtractEnv(t) + prepared := recordPreparedExtractInput(t, env, m, artifactpolicy.SourceInputParty, "party bytes\n") + env.Config.Pipeline.Notarius.References = map[string]string{"party": artifactpolicy.SourceInputParty} + absPaths := sessionPathsForEnv(env, m.SessionID) + references, err := resolveExtractReferences(absPaths, m, env.Config.Pipeline.Notarius, m.SessionID) + if err != nil { + t.Fatal(err) + } + t.Chdir(env.Config.Pipeline.Workspace.Root) + paths := artifacts.NewLocalStore(".").SessionPathsFor(m.Campaign, m.SessionID) + first, err := materializeExtractReferenceSnapshots(paths, references) + if err != nil { + t.Fatal(err) + } + if !filepath.IsAbs(first[0].Path) { + t.Fatalf("adapter reference remains relative: %q", first[0].Path) + } + m.RunID = "20260809T020203Z-abcdef13" + second, err := materializeExtractReferenceSnapshots(paths, references) + if err != nil || !reflect.DeepEqual(first, second) { + t.Fatalf("second invocation bindings = %#v, error = %v", second, err) + } + if err := verifyExtractReferenceSnapshots(paths, references); err != nil { + t.Fatal(err) + } + if _, err := os.Stat(filepath.Join(absPaths.Root, paths.Root)); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("duplicated session prefix exists or cannot be inspected: %v", err) + } + + // A controlled executable proves the production adapter accepts the paths. + // It intentionally produces no receipt; receipt validation is covered elsewhere. + marker := filepath.Join(t.TempDir(), "started") + binary := filepath.Join(t.TempDir(), "notarius") + if err := os.WriteFile(binary, []byte("#!/bin/sh\nprintf started > '"+marker+"'\n"), 0o755); err != nil { + t.Fatal(err) + } + diagnostics := t.TempDir() + _, err = notarius.NewSubprocessRunner().Run(context.Background(), notarius.RunRequest{ + Binary: binary, PipelineID: "dnd-session", ConfigPath: env.Config.Pipeline.Notarius.ConfigPath, + InputPath: prepared, OutputRoot: t.TempDir(), WorkingDirectory: env.Config.Pipeline.Workspace.Root, + Timeout: time.Second * 10, References: first, + ReceiptPath: filepath.Join(diagnostics, "receipt.json"), LogPath: filepath.Join(diagnostics, "stderr.log"), + }) + if err == nil { + t.Fatal("expected missing receipt error") + } + if _, err := os.Stat(marker); err != nil { + t.Fatalf("adapter did not launch with the snapshot bindings: %v", err) + } + + // A symlink on the actual cache path must not be missed by a duplicated walk. + if err := os.Remove(first[0].Path); err != nil { + t.Fatal(err) + } + digestDir := filepath.Dir(first[0].Path) + if err := os.Remove(digestDir); err != nil { + t.Fatal(err) + } + outside := t.TempDir() + if err := os.Symlink(outside, digestDir); err != nil { + t.Fatal(err) + } + if _, err := materializeExtractReferenceSnapshots(paths, references); err == nil { + t.Fatal("accepted symlink on actual cache path") + } + if _, err := os.Stat(filepath.Join(outside, filepath.Base(first[0].Path))); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("unsafe cache path wrote outside session: %v", err) + } +} + func TestExtractReferenceSnapshotsRejectUnsafeConflicts(t *testing.T) { for _, test := range []struct { name string