diff --git a/internal/memory/escape_test.go b/internal/memory/escape_test.go new file mode 100644 index 000000000..5ff0d8ee4 --- /dev/null +++ b/internal/memory/escape_test.go @@ -0,0 +1,1048 @@ +package memory + +import ( + "errors" + "fmt" + "os" + "os/exec" + "path/filepath" + "runtime" + "strings" + "testing" +) + +// linkDir uses a junction on Windows: it needs no privilege, unlike a symlink, +// so it is both the reachable attack and the only one testable on an ordinary +// Windows account. +func linkDir(t *testing.T, target, link string) { + t.Helper() + if runtime.GOOS == "windows" { + if out, err := exec.Command("cmd", "/c", "mklink", "/J", link, target).CombinedOutput(); err != nil { + t.Skipf("cannot create a junction: %v %s", err, out) + } + return + } + if err := os.Symlink(target, link); err != nil { + t.Skipf("cannot create a symlink: %v", err) + } +} + +// The store used to check only its own directory and file, so a link at the +// ANCESTOR .zero turned every operation into one aimed outside the workspace: +// Write and Forget were arbitrary write and delete, and Read was an arbitrary +// read in a tool the model can call by name. +// +// All three are asserted, because fixing one and leaving the others is exactly +// what the original guard did. +func TestAnAncestorLinkCannotTakeTheStoreOutOfTheWorkspace(t *testing.T) { + base := t.TempDir() + outside := filepath.Join(base, "outside") + workspace := filepath.Join(base, "workspace") + for _, dir := range []string{outside, workspace} { + if err := os.MkdirAll(dir, 0o700); err != nil { + t.Fatal(err) + } + } + // A note already sitting in the external directory, so a successful read + // would be visible rather than merely "no error". + if err := os.MkdirAll(filepath.Join(outside, "memory"), 0o700); err != nil { + t.Fatal(err) + } + secret := filepath.Join(outside, "memory", "secret.md") + if err := os.WriteFile(secret, []byte("do not read me"), 0o600); err != nil { + t.Fatal(err) + } + linkDir(t, outside, filepath.Join(workspace, ".zero")) + + paths := DefaultPaths(workspace) + + if _, err := Write(paths, ScopeProject, "escaped", "d", "b"); err == nil { + t.Error("Write went through the linked ancestor") + } + if _, err := os.Stat(filepath.Join(outside, "memory", "escaped.md")); !os.IsNotExist(err) { + t.Errorf("a note was written outside the workspace, stat error = %v", err) + } + if _, err := Read(paths, ScopeProject, "secret"); err == nil { + t.Error("Read returned a note from outside the workspace") + } + if err := Forget(paths, ScopeProject, "secret"); err == nil { + t.Error("Forget accepted a target outside the workspace") + } + if _, err := os.Stat(secret); err != nil { + t.Errorf("Forget deleted a file outside the workspace: %v", err) + } + notes, listErr := List(paths) + if len(notes) != 0 { + t.Errorf("List surfaced %d note(s) from outside the workspace", len(notes)) + } + // The refusal is REPORTED, not swallowed. A store that cannot be opened + // because a link redirects it out of the workspace is an operational failure; + // returning an empty list for it would tell the caller there are no notes, + // which is how a redirected store looks exactly like an empty one. + if listErr == nil { + t.Error("List reported no problem for a store redirected outside the workspace") + } +} + +// The ordinary path still works, or the test above would pass against a store +// that refused everything. +func TestAnOrdinaryWorkspaceStoreStillRoundTrips(t *testing.T) { + workspace := t.TempDir() + paths := DefaultPaths(workspace) + if _, err := Write(paths, ScopeProject, "note", "a summary", "the body"); err != nil { + t.Fatalf("Write: %v", err) + } + note, err := Read(paths, ScopeProject, "note") + if err != nil { + t.Fatalf("Read: %v", err) + } + if note.Description != "a summary" || strings.TrimSpace(note.Body) != "the body" { + t.Errorf("round trip lost content: %+v", note) + } + notes, listErr := List(paths) + if listErr != nil { + t.Fatal(listErr) + } + if len(notes) != 1 { + t.Errorf("List returned %d notes, want 1", len(notes)) + } + if err := Forget(paths, ScopeProject, "note"); err != nil { + t.Fatalf("Forget: %v", err) + } + if _, err := Read(paths, ScopeProject, "note"); err == nil { + t.Error("the note survived Forget") + } +} + +// A LINK ABOVE THE STORE REDIRECTS EVERYTHING BELOW IT. +// +// os.Root refuses to traverse OUT of the workspace, and the note file itself was +// checked — but a link that resolves back INSIDE the workspace is followed, and +// the store path is checked in. A repository shipping ".zero -> redirected" +// served notes from a directory nobody asked for, with every individual check +// passing, because the only component inspected was the note at the end. +// +// The components are derived from Paths rather than spelled out, so this keeps +// testing every ancestor if the layout moves. +func TestALinkAboveTheStoreIsRefused(t *testing.T) { + layout := DefaultPaths(string(filepath.Separator) + "workspace") + for _, store := range []struct { + scope Scope + dir string + }{{ScopeProject, layout.ProjectDir}, {ScopeLocal, layout.LocalDir}} { + relative, err := filepath.Rel(layout.Root, store.dir) + if err != nil { + t.Fatal(err) + } + parts := strings.Split(relative, string(filepath.Separator)) + for i := range parts { + ancestor := filepath.Join(parts[:i+1]...) + t.Run(string(store.scope)+"/"+ancestor, func(t *testing.T) { + root := t.TempDir() + paths := DefaultPaths(root) + // The target carries a complete store rooted where the link + // lands, so the ONLY thing between the caller and the planted + // note is the ancestor check. + target := filepath.Join(root, "redirected") + planted := filepath.Join(target, filepath.Join(parts[i+1:]...)) + if err := os.MkdirAll(planted, 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(planted, "secret"+fileExt), + []byte("---\nname: secret\ndescription: planted\n---\n\nplanted body\n"), 0o600); err != nil { + t.Fatal(err) + } + linkPath := filepath.Join(root, ancestor) + if err := os.MkdirAll(filepath.Dir(linkPath), 0o700); err != nil { + t.Fatal(err) + } + // linkDir, not os.Symlink: on Windows this has to be a JUNCTION, + // which is a reparse point but not a symlink — the exact case + // RefuseReparse tests ModeIrregular for. Calling os.Symlink + // directly skipped the whole test there, so the guard's Windows + // behaviour went unasserted while the run reported green. + linkDir(t, target, linkPath) + if note, err := Read(paths, store.scope, "secret"); err == nil { + t.Fatalf("a link at %s redirected the read: got %q", ancestor, note.Body) + } else if !errors.Is(err, ErrIsSymlink) { + t.Errorf("read through a link at %s failed with %v, want ErrIsSymlink", ancestor, err) + } + // Writes and deletes go through the same door. + if _, err := Write(paths, store.scope, "secret", "d", "body"); !errors.Is(err, ErrIsSymlink) { + t.Errorf("write through a link at %s = %v, want ErrIsSymlink", ancestor, err) + } + if err := Forget(paths, store.scope, "secret"); !errors.Is(err, ErrIsSymlink) { + t.Errorf("forget through a link at %s = %v, want ErrIsSymlink", ancestor, err) + } + }) + } + } +} + +// THE PRIVACY PROMISE FAILS CLOSED. +// +// O_EXCL cannot tell "a previous run wrote the ignore" from "something else got +// there first", and the old code read every failure as the former. Precreating +// an empty .gitignore made every local write succeed with the store fully +// tracked — the note the user was told stays on this machine, sitting in git +// status. +func TestALocalWriteRefusesAnIneffectiveIgnore(t *testing.T) { + for name, content := range map[string]string{ + "empty": "", + "comments only": "# nothing here\n\n", + "narrower than all": "*.md\n", + "cancelled by a re-inclusion": "*\n!keep.md\n", + } { + t.Run(name, func(t *testing.T) { + root := t.TempDir() + paths := DefaultPaths(root) + if err := os.MkdirAll(paths.LocalDir, 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(paths.LocalDir, ".gitignore"), []byte(content), 0o600); err != nil { + t.Fatal(err) + } + if _, err := Write(paths, ScopeLocal, "private", "d", "secret body"); !errors.Is(err, ErrNotPrivate) { + t.Errorf("Write with a %s ignore = %v, want ErrNotPrivate — the note would be tracked", name, err) + } + }) + } + + // An ignore that DOES cover everything is accepted, including one this store + // did not write itself. + for name, content := range map[string]string{ + "exactly what we write": "# Notes saved to the local scope stay on this machine.\n*\n", + "bare star": "*\n", + "star with a comment": "# mine\n*\n", + } { + t.Run("accepted/"+name, func(t *testing.T) { + root := t.TempDir() + paths := DefaultPaths(root) + if err := os.MkdirAll(paths.LocalDir, 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(paths.LocalDir, ".gitignore"), []byte(content), 0o600); err != nil { + t.Fatal(err) + } + if _, err := Write(paths, ScopeLocal, "private", "d", "secret body"); err != nil { + t.Errorf("Write with a %s ignore = %v, want success", name, err) + } + }) + } + + // And the first write into a clean store still installs one. + clean := t.TempDir() + if _, err := Write(DefaultPaths(clean), ScopeLocal, "private", "d", "body"); err != nil { + t.Fatal(err) + } + written, err := os.ReadFile(filepath.Join(DefaultPaths(clean).LocalDir, ".gitignore")) + if err != nil { + t.Fatal(err) + } + if !ignoresEverything(string(written)) { + t.Errorf("the ignore this store installs does not cover everything: %q", written) + } +} + +// GIT DOES NOT READ A SYMLINKED .gitignore, SO NEITHER DOES THE GATE. +// +// keepLocalScopePrivate saw O_EXCL return EEXIST and then read through the link, +// accepting the target's "*" as proof of a rule git warns about and does not +// apply. An in-workspace RELATIVE link is followed by os.Root — an absolute one +// is refused, which is why an earlier check of exactly this looked clean and was +// not. +func TestALinkedIgnoreIsNotProofOfPrivacy(t *testing.T) { + // A FILE symlink, which is what a .gitignore would actually be. Windows needs + // a privilege for these, so the junction case below carries that platform. + t.Run("file symlink", func(t *testing.T) { + root := t.TempDir() + paths := DefaultPaths(root) + if err := os.MkdirAll(paths.LocalDir, 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(paths.LocalDir, "decoy"), []byte("*\n"), 0o600); err != nil { + t.Fatal(err) + } + if err := os.Symlink("decoy", filepath.Join(paths.LocalDir, ".gitignore")); err != nil { + t.Skipf("cannot create a file symlink here: %v", err) + } + if _, err := Write(paths, ScopeLocal, "private", "d", "secret"); !errors.Is(err, ErrNotPrivate) { + t.Errorf("a symlinked ignore was accepted as privacy: %v", err) + } + }) + + // A DIRECTORY reparse point at the ignore path, which linkDir builds as a + // junction on Windows and a symlink elsewhere. The refusal arrives + // differently by platform — ErrNotPrivate where the link is inspected as a + // link, "is a directory" where the create simply cannot open it — so what is + // asserted is the property that matters on both: the write DOES NOT SUCCEED. + // Asserting the sentinel here is what turned this test red on Windows while + // the behaviour was correct. + t.Run("directory reparse point", func(t *testing.T) { + root := t.TempDir() + paths := DefaultPaths(root) + if err := os.MkdirAll(filepath.Join(paths.LocalDir, "decoy"), 0o700); err != nil { + t.Fatal(err) + } + linkDir(t, "decoy", filepath.Join(paths.LocalDir, ".gitignore")) + if _, err := Write(paths, ScopeLocal, "private", "d", "secret"); err == nil { + t.Error("a reparse point at the ignore path was accepted as privacy") + } + }) +} + +// THE GATE FOLLOWS GIT'S WHITESPACE RULES, and trimming each line had both +// backwards. Every expectation here was measured against git 2.55.0, not read +// off the spec. +func TestTheIgnoreGateAgreesWithGit(t *testing.T) { + for name, tc := range map[string]struct { + content string + covered bool + }{ + // LEADING whitespace is part of the pattern, so this ignores nothing — + // and the gate said it did, which let a note the user was told stays on + // this machine be picked up by a routine `git add -A`. + "leading spaces": {" *\n", false}, + "leading tab": {"\t*\n", false}, + // A BOM is stripped by git, so this rule works and must be accepted. + "utf-8 bom": {"\xef\xbb\xbf*\n", true}, + // TRAILING whitespace is not significant to git. + "trailing spaces": {"* \n", true}, + "plain": {"*\n", true}, + "comment then star": {"# mine\n*\n", true}, + // CRLF, because a .gitignore written on Windows carries them and git does + // not care. The gate strips the carriage return before comparing; if that + // ever stops, every local write on a Windows checkout fails for a file + // that was doing its job. + "crlf": {"*\r\n", true}, + "crlf with comment": {"# mine\r\n*\r\n", true}, + "cancelled": {"*\n!keep.md\n", false}, + } { + if got := ignoresEverything(tc.content); got != tc.covered { + t.Errorf("%s: ignoresEverything(%q) = %v, want %v (git's answer)", name, tc.content, got, tc.covered) + } + } +} + +// ABSENT TO THE HANDLE IS NOT ABSENT ON DISK. +// +// A component the confined handle will not open, while it is present on disk, +// has been REFUSED — and a Windows junction whose target leaves the workspace +// reports exactly that way. The chain read it and everything under it as +// missing, the later open became ErrNotFound, and List drops ErrNotFound, so a +// tampered store presented as an empty one. presentOnDisk is what tells the two +// answers apart, so its own behaviour is pinned here. +func TestPresentOnDiskDistinguishesAbsenceFromRefusal(t *testing.T) { + root := t.TempDir() + if err := os.MkdirAll(filepath.Join(root, ".zero", "memory"), 0o700); err != nil { + t.Fatal(err) + } + if !presentOnDisk(root, ".zero") { + t.Error("an existing directory was reported absent") + } + if !presentOnDisk(root, filepath.Join(".zero", "memory")) { + t.Error("an existing nested directory was reported absent") + } + if presentOnDisk(root, "never-created") { + t.Error("a missing path was reported present") + } + if presentOnDisk("", ".zero") { + t.Error("a blank root was treated as containing something") + } + // A LINK COUNTS AS PRESENT, without being followed — that is the whole case + // this exists for, since the junction's target is outside the workspace. + linkDir(t, filepath.Join(root, ".zero"), filepath.Join(root, "aliased")) + if !presentOnDisk(root, "aliased") { + t.Error("a reparse point was reported absent, so a refusal would read as absence") + } + // And a DANGLING link is still present: Lstat does not follow it. + if err := os.Symlink("nowhere-at-all", filepath.Join(root, "dangling")); err == nil { + if !presentOnDisk(root, "dangling") { + t.Error("a dangling link was reported absent") + } + } +} + +// A clean workspace must keep working: absence really is absence there, and +// treating it as a refusal would break every first write. +func TestAnUncreatedStoreIsStillAbsence(t *testing.T) { + fresh := t.TempDir() + paths := DefaultPaths(fresh) + if _, err := Read(paths, ScopeProject, "nothing"); !errors.Is(err, ErrNotFound) { + t.Errorf("a note in an uncreated store = %v, want ErrNotFound", err) + } + notes, err := List(paths, ScopeProject) + if err != nil || len(notes) != 0 { + t.Errorf("listing an uncreated store = %+v, %v; want empty and no error", notes, err) + } + if _, err := Write(paths, ScopeLocal, "first", "d", "b"); err != nil { + t.Errorf("the first write into a clean workspace failed: %v", err) + } +} + +// REFUSED IS NOT ABSENT, AND THE TEST FOR IT RUNS HERE. +// +// The first attempt at this keyed on the component being ABSENT to the handle +// and only then compared against disk. A junction is not absent to it — +// handle.Lstat does not traverse a reparse point, so it reports the junction as +// being right there, the branch never ran, and the walk continued past the +// component it was meant to refuse. The later open failed as ErrNotFound and +// List dropped it, which is how a tampered store presented as an empty one. +// +// The question that separates the two is what the handle can OPEN, because +// opening is what traverses. A directory that is present but cannot be entered +// produces exactly that disagreement — Lstat sees it, Open refuses it — which is +// the same shape as the junction and, unlike the junction, constructible here. +func TestAPresentButUnopenableStoreIsReportedNotHidden(t *testing.T) { + root := t.TempDir() + paths := DefaultPaths(root) + if err := os.MkdirAll(paths.ProjectDir, 0o700); err != nil { + t.Fatal(err) + } + if _, err := Write(paths, ScopeProject, "note", "d", "body"); err != nil { + t.Fatal(err) + } + // NOT CONSTRUCTIBLE EVERYWHERE, and the platforms invert. On Windows + // os.Chmod only toggles the read-only attribute, so this arm cannot build the + // refusal there — which is the opposite of what an earlier version of this + // comment claimed. The companion below covers Windows with a junction, which + // IS constructible there and is not here. + blocked := filepath.Join(root, ".zero") + if err := os.Chmod(blocked, 0o000); err != nil { + t.Skipf("cannot remove access here: %v", err) + } + t.Cleanup(func() { _ = os.Chmod(blocked, 0o700) }) + // CLOSE IT. Discarding the handle leaked an open descriptor, and on Windows + // an open handle blocks the directory's removal — so the skip below left + // t.TempDir's cleanup failing and the run red on the very platform this test + // is trying to stand in for. + if probe, err := os.Open(filepath.Join(blocked, "memory")); err == nil { + probe.Close() + t.Skip("this environment ignores directory permissions, so the refusal cannot be built") + } + + _, readErr := Read(paths, ScopeProject, "note") + if errors.Is(readErr, ErrNotFound) { + t.Errorf("an unreadable store was reported as a missing note, which is what makes the model overwrite it: %v", readErr) + } + if !errors.Is(readErr, ErrUnreadable) { + t.Errorf("Read of an unreadable store = %v, want ErrUnreadable", readErr) + } + if _, listErr := List(paths, ScopeProject); listErr == nil { + t.Error("List presented an unreadable store as an empty one") + } +} + +// SAVE A NOTE, READ IT STRAIGHT BACK, GET YOUR OWN. +// +// memory_write and memory_forget default to local while an unscoped read +// resolved project first, and scope is optional in every schema. A model that +// omitted it was handed content it never wrote, and told a note was deleted +// after which the name still read. A checked-in project note is the ordinary way +// that happens — it arrives with a clone, and the model has no reason to expect +// it. +func TestAnUnscopedRoundTripReturnsWhatWasWritten(t *testing.T) { + root := t.TempDir() + paths := DefaultPaths(root) + if err := os.MkdirAll(paths.ProjectDir, 0o700); err != nil { + t.Fatal(err) + } + // The checked-in note that used to win. + if err := os.WriteFile(filepath.Join(paths.ProjectDir, "findings"+fileExt), + []byte("---\nname: findings\ndescription: theirs\n---\n\nPROJECT-CONTENT\n"), 0o600); err != nil { + t.Fatal(err) + } + if _, err := Write(paths, ScopeLocal, "findings", "mine", "MY-OWN-CONTENT"); err != nil { + t.Fatal(err) + } + + scopes, err := ResolveScopes("") + if err != nil { + t.Fatal(err) + } + if len(scopes) == 0 || scopes[0] != ScopeLocal { + t.Fatalf("the unscoped order is %v; a write lands in local, so a read must look there first", scopes) + } + var got Note + for _, scope := range scopes { + if note, err := Read(paths, scope, "findings"); err == nil { + got = note + break + } + } + if !strings.Contains(got.Body, "MY-OWN-CONTENT") { + t.Errorf("an unscoped read returned %q, not what the unscoped write saved", got.Body) + } + + // A project note is still reachable when nothing local shadows it — the order + // decides which wins, not what exists. + if err := Forget(paths, ScopeLocal, "findings"); err != nil { + t.Fatal(err) + } + for _, scope := range scopes { + if note, err := Read(paths, scope, "findings"); err == nil { + got = note + break + } + } + if !strings.Contains(got.Body, "PROJECT-CONTENT") { + t.Errorf("after forgetting the local note the project one should be found; got %q", got.Body) + } +} + +// A NOTE THE LISTING DENIES EXISTS MUST NOT BE DESTROYED BY THE NEXT WRITE. +// +// List matches the extension exactly; Read reopened name+".md", and on a +// case-insensitive filesystem that is the same file as a hand-authored +// "findings.MD". So the listing reported an empty store, writing was the +// reasonable next move, and a checked-in file was silently overwritten. +// +// Read now agrees with List, and the write refuses rather than taking the +// occupant's place. +func TestADifferentlySpelledNoteIsNotSilentlyOverwritten(t *testing.T) { + root := t.TempDir() + paths := DefaultPaths(root) + if err := os.MkdirAll(paths.ProjectDir, 0o700); err != nil { + t.Fatal(err) + } + shouty := filepath.Join(paths.ProjectDir, "findings.MD") + original := "---\nname: findings\ndescription: d\n---\n\nHAND-WRITTEN\n" + if err := os.WriteFile(shouty, []byte(original), 0o600); err != nil { + t.Fatal(err) + } + probe := filepath.Join(root, ".CaseProbe") + if err := os.WriteFile(probe, nil, 0o600); err != nil { + t.Fatal(err) + } + _, insensitiveErr := os.Stat(filepath.Join(root, ".caseprobe")) + os.Remove(probe) + if insensitiveErr != nil { + t.Skip("this filesystem is case-sensitive, so the two names are genuinely different files") + } + + // Read agrees with the listing: neither offers it. + notes, err := List(paths, ScopeProject) + if err != nil || len(notes) != 0 { + t.Fatalf("List = %+v, %v; the exact-extension rule should not show findings.MD", notes, err) + } + if _, err := Read(paths, ScopeProject, "findings"); err == nil { + t.Error("Read handed back a note the listing denies exists") + } + + // And the write refuses rather than destroying it. + if _, err := Write(paths, ScopeProject, "findings", "d", "OVERWRITTEN"); !errors.Is(err, ErrNameClash) { + t.Errorf("Write = %v, want ErrNameClash rather than taking the occupant's place", err) + } + after, err := os.ReadFile(shouty) + if err != nil { + t.Fatal(err) + } + if string(after) != original { + t.Errorf("the hand-authored note was modified: %q", string(after)) + } +} + +// THE COMPANION FOR THE PLATFORM THE OTHER ARM CANNOT REACH. +// +// TestAPresentButUnopenableStoreIsReportedNotHidden builds its refusal with +// chmod, which Windows does not honour — os.Chmod there only toggles the +// read-only attribute. So that arm skips on Windows and the branch would have no +// coverage on the one platform where reparse points are ordinary. +// +// A junction is the reverse: constructible on Windows, not here. Between the two +// arms the refusal is exercised everywhere, and neither passes vacuously — +// each skips loudly where its own mechanism is unavailable. +func TestAStoreBehindAReparsePointIsRefusedOnEveryPlatform(t *testing.T) { + root := t.TempDir() + paths := DefaultPaths(root) + target := filepath.Join(root, "elsewhere") + if err := os.MkdirAll(filepath.Join(target, "memory"), 0o700); err != nil { + t.Fatal(err) + } + linkDir(t, target, filepath.Join(root, ".zero")) + + if _, err := Read(paths, ScopeProject, "anything"); errors.Is(err, ErrNotFound) { + t.Error("a store behind a reparse point read as a missing note, which is what makes a model overwrite it") + } else if err == nil { + t.Error("a store behind a reparse point was read through") + } + if _, err := List(paths, ScopeProject); err == nil { + t.Error("List presented a store behind a reparse point as an empty one") + } +} + +// runStoreGit runs one git command in dir and fails the test if it does not +// succeed. Identity comes from the environment rather than repository config so +// nothing here depends on a git new enough for GIT_CONFIG_GLOBAL. +func runStoreGit(t *testing.T, dir string, args ...string) string { + t.Helper() + if _, err := exec.LookPath("git"); err != nil { + t.Skip("git not available") + } + cmd := exec.Command("git", append([]string{"-C", dir}, args...)...) + cmd.Env = append(os.Environ(), + "GIT_AUTHOR_NAME=t", "GIT_AUTHOR_EMAIL=t@example.invalid", + "GIT_COMMITTER_NAME=t", "GIT_COMMITTER_EMAIL=t@example.invalid", + ) + out, err := cmd.CombinedOutput() + if err != nil { + t.Fatalf("git %v: %v\n%s", args, err, out) + } + return string(out) +} + +// seedStoreRepo makes root a repository with one commit, so HEAD exists and the +// index is a real one rather than the empty index of a bare `git init`. +func seedStoreRepo(t *testing.T, root string) { + t.Helper() + runStoreGit(t, root, "init") + if err := os.WriteFile(filepath.Join(root, "README.md"), []byte("readme\n"), 0o600); err != nil { + t.Fatal(err) + } + runStoreGit(t, root, "add", "README.md") + runStoreGit(t, root, "commit", "-m", "seed") +} + +// writeStoreFile creates one file under the local store, making the store +// directory first. +func writeStoreFile(t *testing.T, paths Paths, name, content string) string { + t.Helper() + if err := os.MkdirAll(paths.LocalDir, 0o700); err != nil { + t.Fatal(err) + } + path := filepath.Join(paths.LocalDir, name) + if err := os.WriteFile(path, []byte(content), 0o600); err != nil { + t.Fatal(err) + } + return path +} + +// AN IGNORED PATH IS NOT NECESSARILY A PRIVATE ONE. +// +// git applies no ignore rule to a path already in the INDEX, so "*" in +// local/.gitignore proves privacy only for an untracked path. A clone, an +// earlier `git add -f`, or a hand-authored repository restores both the ignore +// and the note as tracked files; the gate read the ignore, agreed the store was +// private, and the atomic rename replaced a TRACKED file with the body the user +// was told stays on this machine. `git status` showed it as a modification ready +// to commit. Reported by @jatmn. +func TestALocalWriteIsRefusedWhenGitTracksTheNote(t *testing.T) { + root := t.TempDir() + paths := DefaultPaths(root) + const committedBody = "---\nname: private\n---\n\ncommitted body\n" + notePath := writeStoreFile(t, paths, "private.md", committedBody) + writeStoreFile(t, paths, gitignoreName, localIgnoreContent) + + seedStoreRepo(t, root) + // -f, because the ignore this repository is committing covers both files — + // which is the whole point: the rule is in force AND the paths are tracked. + runStoreGit(t, root, "add", "-f", ".zero/memory/local/.gitignore", ".zero/memory/local/private.md") + runStoreGit(t, root, "commit", "-m", "track the local store") + + if _, err := Write(paths, ScopeLocal, "private", "d", "machine-local secret"); !errors.Is(err, ErrNotPrivate) { + t.Fatalf("Write into a tracked local store = %v, want ErrNotPrivate", err) + } else if !strings.Contains(err.Error(), "private.md") { + t.Errorf("refusal %q does not name the tracked note", err) + } + + // The exact committed bytes, not merely "unchanged length": the failure being + // pinned is a body swap of the same shape. + body, readErr := os.ReadFile(notePath) + if readErr != nil { + t.Fatal(readErr) + } + if string(body) != committedBody { + t.Errorf("tracked note body = %q, want the committed %q", body, committedBody) + } + if status := runStoreGit(t, root, "status", "--porcelain"); status != "" { + t.Errorf("git status = %q, want empty — the refused write left an index-visible change", status) + } +} + +// THE SAME REFUSAL ON THE FIRST-WRITE BRANCH. +// +// O_EXCL is what decides whether the gate is installing the ignore or checking +// one, and only the checking branch used to look at anything. A repository that +// tracks the note without the ignore therefore walked straight through: the gate +// created a fresh ignore, reported a clean first write, and overwrote the tracked +// note anyway. Deleting one file from the worktree is all that separates this +// from the case above, so gating one branch and not the other fixes nothing. +func TestALocalWriteIsRefusedWhenTrackedWithNoIgnorePresent(t *testing.T) { + root := t.TempDir() + paths := DefaultPaths(root) + const committedBody = "---\nname: private\n---\n\ncommitted body\n" + notePath := writeStoreFile(t, paths, "private.md", committedBody) + + seedStoreRepo(t, root) + runStoreGit(t, root, "add", ".zero/memory/local/private.md") + runStoreGit(t, root, "commit", "-m", "track the local note") + + if _, err := Write(paths, ScopeLocal, "private", "d", "machine-local secret"); !errors.Is(err, ErrNotPrivate) { + t.Fatalf("Write into a tracked store with no ignore = %v, want ErrNotPrivate", err) + } + body, readErr := os.ReadFile(notePath) + if readErr != nil { + t.Fatal(readErr) + } + if string(body) != committedBody { + t.Errorf("tracked note body = %q, want the committed %q", body, committedBody) + } + if status := runStoreGit(t, root, "status", "--porcelain"); status != "" { + t.Errorf("git status = %q, want empty", status) + } +} + +func TestAFailedIgnoreWriteCanBeRetried(t *testing.T) { + paths := DefaultPaths(t.TempDir()) + priorWriter := writeLocalIgnore + writeLocalIgnore = func(*os.File) error { return errors.New("injected ignore write failure") } + t.Cleanup(func() { writeLocalIgnore = priorWriter }) + + if _, err := Write(paths, ScopeLocal, "private", "d", "first"); err == nil || !strings.Contains(err.Error(), "injected ignore write failure") { + t.Fatalf("first Write error = %v, want injected write failure", err) + } + ignorePath := filepath.Join(paths.LocalDir, gitignoreName) + if _, err := os.Lstat(ignorePath); !os.IsNotExist(err) { + t.Fatalf("failed installation left %s behind: %v", ignorePath, err) + } + + writeLocalIgnore = priorWriter + if _, err := Write(paths, ScopeLocal, "private", "d", "second"); err != nil { + t.Fatalf("retry after failed ignore installation = %v", err) + } + note, err := Read(paths, ScopeLocal, "private") + if err != nil { + t.Fatal(err) + } + if note.Body != "second\n" { + t.Fatalf("retried note body = %q, want %q", note.Body, "second\n") + } +} + +func TestAFailedIgnoreCloseCanBeRetried(t *testing.T) { + paths := DefaultPaths(t.TempDir()) + priorCloser := closeLocalIgnore + closeLocalIgnore = func(file *os.File) error { + if err := file.Close(); err != nil { + t.Fatal(err) + } + return errors.New("injected ignore close failure") + } + t.Cleanup(func() { closeLocalIgnore = priorCloser }) + + if _, err := Write(paths, ScopeLocal, "private", "d", "first"); err == nil || !strings.Contains(err.Error(), "injected ignore close failure") { + t.Fatalf("first Write error = %v, want injected close failure", err) + } + ignorePath := filepath.Join(paths.LocalDir, gitignoreName) + if _, err := os.Lstat(ignorePath); !os.IsNotExist(err) { + t.Fatalf("failed close left %s behind: %v", ignorePath, err) + } + + closeLocalIgnore = priorCloser + if _, err := Write(paths, ScopeLocal, "private", "d", "second"); err != nil { + t.Fatalf("retry after failed ignore close = %v", err) + } +} + +func TestAFailedIgnoreCloseReportsCleanupFailure(t *testing.T) { + paths := DefaultPaths(t.TempDir()) + closeFailure := errors.New("injected ignore close failure") + removeFailure := errors.New("injected ignore cleanup failure") + priorCloser := closeLocalIgnore + priorRemover := removeLocalIgnore + closeLocalIgnore = func(file *os.File) error { + if err := file.Close(); err != nil { + t.Fatal(err) + } + return closeFailure + } + removeLocalIgnore = func(*os.Root, string) error { + return removeFailure + } + t.Cleanup(func() { + closeLocalIgnore = priorCloser + removeLocalIgnore = priorRemover + }) + + _, err := Write(paths, ScopeLocal, "private", "d", "first") + if !errors.Is(err, closeFailure) || !errors.Is(err, removeFailure) { + t.Fatalf("Write error = %v, want close and cleanup failures", err) + } + if !strings.Contains(err.Error(), "remove incomplete") { + t.Fatalf("Write error = %v, want retained-placeholder context", err) + } + ignorePath := filepath.Join(paths.LocalDir, gitignoreName) + if _, statErr := os.Lstat(ignorePath); statErr != nil { + t.Fatalf("retained placeholder %s: %v", ignorePath, statErr) + } +} + +// THE ORDINARY CASE KEEPS WORKING, which is the constraint the refusal above has +// to live inside: a repository whose local store is untracked is the normal +// state, and a note written there must still land and still be invisible to git. +func TestALocalWriteInsideAnUntrackedStoreStillLands(t *testing.T) { + root := t.TempDir() + paths := DefaultPaths(root) + seedStoreRepo(t, root) + + if _, err := Write(paths, ScopeLocal, "private", "d", "machine-local secret"); err != nil { + t.Fatalf("Write into an untracked store inside a repository = %v, want success", err) + } + note, err := Read(paths, ScopeLocal, "private") + if err != nil { + t.Fatal(err) + } + if note.Body != "machine-local secret\n" { + t.Errorf("note body = %q, want %q", note.Body, "machine-local secret\n") + } + if status := runStoreGit(t, root, "status", "--porcelain"); status != "" { + t.Errorf("git status = %q, want empty — the note must not be visible to git", status) + } +} + +// A CHECKED-IN IGNORE IS NOT A LEAK. Committing local/.gitignore hands every +// clone the rule before its first write, and the file carries no note, so it is +// the one tracked entry the store may hold. Refusing it would break a deliberate +// and correct setup in the name of the fix above. +func TestACheckedInIgnoreDoesNotBlockLocalWrites(t *testing.T) { + root := t.TempDir() + paths := DefaultPaths(root) + writeStoreFile(t, paths, gitignoreName, localIgnoreContent) + + seedStoreRepo(t, root) + runStoreGit(t, root, "add", "-f", ".zero/memory/local/.gitignore") + runStoreGit(t, root, "commit", "-m", "check in the ignore") + + if _, err := Write(paths, ScopeLocal, "private", "d", "machine-local secret"); err != nil { + t.Fatalf("Write with only the ignore tracked = %v, want success", err) + } + if status := runStoreGit(t, root, "status", "--porcelain"); status != "" { + t.Errorf("git status = %q, want empty", status) + } +} + +// PROJECT SCOPE IS SUPPOSED TO BE TRACKED. It is checked in beside the repo and +// reviewed like any other file, so the index check must not reach it: a second +// write to a committed project note is an ordinary edit, not a broken promise. +func TestAProjectNoteStaysWritableWhileTracked(t *testing.T) { + root := t.TempDir() + paths := DefaultPaths(root) + if _, err := Write(paths, ScopeProject, "shared", "d", "first"); err != nil { + t.Fatal(err) + } + seedStoreRepo(t, root) + runStoreGit(t, root, "add", ".zero/memory/shared.md") + runStoreGit(t, root, "commit", "-m", "track the project note") + + if _, err := Write(paths, ScopeProject, "shared", "d", "second"); err != nil { + t.Fatalf("Write to a tracked project note = %v, want success", err) + } + note, err := Read(paths, ScopeProject, "shared") + if err != nil { + t.Fatal(err) + } + if note.Body != "second\n" { + t.Errorf("project note body = %q, want %q", note.Body, "second\n") + } +} + +// UNANSWERABLE INSIDE A REPOSITORY IS A REFUSAL. +// +// The repository was found on disk, so the store IS somewhere git can track it; +// without git there is no way to learn whether it already does. Treating that as +// "nothing is tracked" is the fail-open being closed here, so it refuses instead +// — and only here, because a store with no repository above it never reaches +// this call. +func TestALocalWriteIsRefusedWhenGitCannotBeAsked(t *testing.T) { + root := t.TempDir() + paths := DefaultPaths(root) + seedStoreRepo(t, root) + + t.Setenv("PATH", "") + if _, err := Write(paths, ScopeLocal, "private", "d", "machine-local secret"); !errors.Is(err, ErrNotPrivate) { + t.Fatalf("Write with git unreachable inside a repository = %v, want ErrNotPrivate", err) + } +} + +// NO REPOSITORY MEANS NOTHING TO ASK. The common workspace is not a checkout at +// all, and there git may not even be installed — so repository membership is +// decided from the filesystem and no subprocess runs. Removing git from PATH +// must not change the answer. +func TestALocalWriteOutsideAnyRepositoryNeedsNoGit(t *testing.T) { + root := t.TempDir() + paths := DefaultPaths(root) + + t.Setenv("PATH", "") + if _, err := Write(paths, ScopeLocal, "private", "d", "machine-local secret"); err != nil { + t.Fatalf("Write outside a repository with no git on PATH = %v, want success", err) + } + note, err := Read(paths, ScopeLocal, "private") + if err != nil { + t.Fatal(err) + } + if note.Body != "machine-local secret\n" { + t.Errorf("note body = %q, want %q", note.Body, "machine-local secret\n") + } +} + +func TestALocalWriteBelowStrayGitMarkers(t *testing.T) { + for _, marker := range []string{"empty-directory", "invalid-file", "missing-gitdir"} { + for _, withoutGit := range []bool{false, true} { + t.Run(fmt.Sprintf("%s/no-git=%t", marker, withoutGit), func(t *testing.T) { + parent := t.TempDir() + gitPath := filepath.Join(parent, ".git") + if marker == "empty-directory" { + if err := os.Mkdir(gitPath, 0o700); err != nil { + t.Fatal(err) + } + } else { + content := "not a git directory reference\n" + if marker == "missing-gitdir" { + content = "gitdir: missing\n" + } + if err := os.WriteFile(gitPath, []byte(content), 0o600); err != nil { + t.Fatal(err) + } + } + if withoutGit { + t.Setenv("PATH", "") + } + paths := DefaultPaths(filepath.Join(parent, "workspace")) + if _, err := Write(paths, ScopeLocal, "private", "d", "local note"); err != nil { + t.Fatalf("Write below stray %s = %v, want success", marker, err) + } + note, err := Read(paths, ScopeLocal, "private") + if err != nil || note.Body != "local note\n" { + t.Fatalf("Read = %#v, %v", note, err) + } + }) + } + } +} + +func TestAStrayGitMarkerCannotHideAnEnclosingRepository(t *testing.T) { + root := t.TempDir() + seedStoreRepo(t, root) + workspace := filepath.Join(root, "workspace") + paths := DefaultPaths(workspace) + writeStoreFile(t, paths, "private.md", "already tracked\n") + runStoreGit(t, root, "add", "workspace/.zero/memory/local/private.md") + if err := os.Mkdir(filepath.Join(workspace, ".git"), 0o700); err != nil { + t.Fatal(err) + } + if _, err := Write(paths, ScopeLocal, "private", "d", "new secret"); !errors.Is(err, ErrNotPrivate) { + t.Fatalf("Write through stray marker inside real repository = %v, want ErrNotPrivate", err) + } +} + +func TestACorruptRepositoryStillRefusesLocalWrites(t *testing.T) { + root := t.TempDir() + seedStoreRepo(t, root) + if err := os.WriteFile(filepath.Join(root, ".git", "HEAD"), []byte("corrupt\n"), 0o600); err != nil { + t.Fatal(err) + } + if _, err := Write(DefaultPaths(root), ScopeLocal, "private", "d", "secret"); !errors.Is(err, ErrNotPrivate) { + t.Fatalf("Write inside damaged repository = %v, want ErrNotPrivate", err) + } +} + +// AN INHERITED INDEX REDIRECT MUST NOT ANSWER FOR THE CHECKOUT. +// +// A hook, a rebase, or any git wrapper exports GIT_INDEX_FILE and GIT_DIR into +// the environment its children inherit, and zero inherits them like anything +// else. Left in place they point `git ls-files` at a different index — during a +// rebase, one holding none of the worktree's paths — and it answers "nothing is +// tracked" about a store that plainly is. +func TestAnInheritedIndexRedirectCannotHideTracking(t *testing.T) { + root := t.TempDir() + paths := DefaultPaths(root) + writeStoreFile(t, paths, "private.md", "---\nname: private\n---\n\ncommitted body\n") + + seedStoreRepo(t, root) + runStoreGit(t, root, "add", ".zero/memory/local/private.md") + runStoreGit(t, root, "commit", "-m", "track the local note") + + // A path with no index at it reads as an empty index, which is exactly the + // shape that makes a tracked store look clean. + t.Setenv("GIT_INDEX_FILE", filepath.Join(t.TempDir(), "elsewhere.index")) + if _, err := Write(paths, ScopeLocal, "private", "d", "machine-local secret"); !errors.Is(err, ErrNotPrivate) { + t.Fatalf("Write with GIT_INDEX_FILE redirected = %v, want ErrNotPrivate", err) + } +} + +// THE REPOSITORY IS LOOKED FOR WHERE GIT WILL LOOK. +// +// os/exec chdirs into the store, so git discovers the repository from the +// resulting getcwd — the physical path. Walking the lexical path up instead +// visits ancestors the child process never has: with the workspace reached +// through a link into a repository subdirectory, the lexical walk leaves the +// repository immediately, finds no .git, and concludes nothing can be tracked +// about a store git tracks. +func TestTheRepositoryIsFoundThroughALinkedWorkspace(t *testing.T) { + base := t.TempDir() + repo := filepath.Join(base, "repo") + inner := filepath.Join(repo, "inner") + if err := os.MkdirAll(inner, 0o700); err != nil { + t.Fatal(err) + } + workspace := filepath.Join(base, "workspace") + linkDir(t, inner, workspace) + + paths := DefaultPaths(workspace) + writeStoreFile(t, paths, "private.md", "---\nname: private\n---\n\ncommitted body\n") + seedStoreRepo(t, repo) + runStoreGit(t, repo, "add", "inner/.zero/memory/local/private.md") + runStoreGit(t, repo, "commit", "-m", "track the local note") + + if _, err := Write(paths, ScopeLocal, "private", "d", "machine-local secret"); !errors.Is(err, ErrNotPrivate) { + t.Fatalf("Write through a linked workspace = %v, want ErrNotPrivate", err) + } else if !strings.Contains(err.Error(), "tracks private.md") { + t.Fatalf("Write through a linked workspace stopped before consulting the repository index: %v", err) + } + if status := runStoreGit(t, repo, "status", "--porcelain"); status != "" { + t.Errorf("git status = %q, want empty", status) + } +} + +// A LINKED WORKTREE CARRIES .git AS A FILE, NOT A DIRECTORY, and it tracks its +// files exactly like an ordinary checkout. gitDirName's comment says so; nothing +// tested it. A verifier changed the presence check to `err == nil && info.IsDir()` +// and every other test in this file still passed, which means the whole +// linked-worktree and submodule class was resting on a comment. +// +// A REAL FIXTURE RATHER THAN A MOCK: `git worktree add` produces the genuine +// article — a .git FILE whose body is "gitdir: " — so this exercises the +// same code a user's linked worktree does, instead of a stand-in that agrees +// with whatever the check happens to be. +func TestALocalWriteIsRefusedInsideALinkedWorktree(t *testing.T) { + root := t.TempDir() + main := filepath.Join(root, "main") + if err := os.MkdirAll(main, 0o700); err != nil { + t.Fatal(err) + } + seedStoreRepo(t, main) + linked := filepath.Join(root, "linked") + runStoreGit(t, main, "worktree", "add", linked) + + marker, err := os.Lstat(filepath.Join(linked, gitDirName)) + if err != nil { + t.Fatalf("the linked worktree has no %s: %v", gitDirName, err) + } + if marker.IsDir() { + t.Skipf("this git writes %s as a directory in a linked worktree; the case under test does not arise", gitDirName) + } + + paths := DefaultPaths(linked) + writeStoreFile(t, paths, "private.md", "---\nname: private\n---\n\nmachine-local\n") + writeStoreFile(t, paths, ".gitignore", localIgnoreContent) + runStoreGit(t, linked, "add", "-f", filepath.Join(".zero", "memory", "local", "private.md")) + runStoreGit(t, linked, "commit", "-m", "track the local note") + + _, err = Write(paths, ScopeLocal, "private", "d", "replaced") + if !errors.Is(err, ErrNotPrivate) { + t.Fatalf("a tracked local note inside a linked worktree was writable: %v", err) + } + if status := runStoreGit(t, linked, "status", "--porcelain"); strings.TrimSpace(status) != "" { + t.Errorf("the refused write still left an index-visible change:\n%s", status) + } +} diff --git a/internal/memory/memory.go b/internal/memory/memory.go new file mode 100644 index 000000000..f6bb78842 --- /dev/null +++ b/internal/memory/memory.go @@ -0,0 +1,1174 @@ +// Package memory is a durable, scoped note store that survives a session. +// +// WHAT IT IS FOR. A session ends and everything it worked out goes with it — +// the convention this repo actually follows, the decision behind a strange- +// looking guard, the finding an audit already confirmed. AGENTS.md holds what +// someone sat down and wrote; this holds what accumulates, and it is scoped so +// the two do not become one file nobody prunes. +// +// NOT A SECOND PLAN STORE. The "no new store" invariant is about PLAN STATE — +// "plan state is session events", ARCHITECTURE.md — because two stores for one +// fact eventually disagree. Notes are not derivable from any event log, so there +// is nothing here for a second store to contradict. +// +// THE SAFETY RULES ARE PLAN_STORE'S, deliberately reused rather than rewritten: +// an allow-list name that cannot spell a traversal component, containment of +// every operation to a handle on the workspace (internal/pathjail), and an +// O_EXCL temp file renamed into place. A note store is a write primitive pointed +// at a path the model chooses, which is the same shape as "save my plan" and +// needs the same answers. +package memory + +import ( + "bytes" + "context" + "errors" + "fmt" + "io" + "io/fs" + "os" + "os/exec" + "path/filepath" + "regexp" + "sort" + "strings" + "time" + + "github.com/Gitlawb/zero/internal/pathjail" +) + +// Scope is where a note lives, and who else sees it. +type Scope string + +const ( + // ScopeProject is checked in beside the repo: shared with everyone who clones + // it, and therefore reviewed like any other file in the tree. + ScopeProject Scope = "project" + // ScopeLocal is this machine only. The natural home for anything specific to + // one checkout, one operator, or one afternoon. + ScopeLocal Scope = "local" +) + +// fileExt is the stored extension. Markdown with frontmatter, because a note is +// meant to be readable by the person whose repo it is sitting in. +const fileExt = ".md" + +// gitignoreName and localIgnoreContent are what makes the local store private. +const gitignoreName = ".gitignore" + +const localIgnoreContent = "# Notes saved to the local scope stay on this machine.\n*\n" + +// gitDirName is the entry whose presence at a directory makes that directory a +// repository. A FILE by this name counts as much as a directory: that is what a +// linked worktree and a submodule carry, and both track their files exactly like +// an ordinary checkout. +const gitDirName = ".git" + +// gitIndexTimeout bounds the one subprocess this package runs. Reading an index +// is local and takes milliseconds; the bound is here so a wedged or +// filesystem-blocked git cannot hold a note write open forever. +const gitIndexTimeout = 15 * time.Second + +// maxTrackedNamed is how many tracked entries a refusal spells out. A store with +// hundreds of them is still one refusal, and the first few name the problem. +const maxTrackedNamed = 5 + +// tempExt is what an in-progress write carries. Deliberately not fileExt, so a +// temp file a crash left behind is never listed as a note. +const tempExt = ".tmp" + +// maxNoteBytes bounds one note. Generous for prose, small enough that a runaway +// write cannot quietly fill a repo. +const maxNoteBytes = 64 << 10 + +// maxDescriptionBytes bounds the ONE-LINE summary, separately from the note. +// +// The listing prints every note's description, so the field is shared screen +// space: the total-size check alone let a single note carry a 60 KiB single-line +// description and consume the listing everyone else has to fit in. A description +// is a sentence telling a reader whether to open the note — anything past this is +// the note's own body in the wrong field, so it is truncated rather than refused. +const maxDescriptionBytes = 200 + +// namePattern is an ALLOW-LIST, the same rule plan names use +// (specialist/manifest.go): enumerate what is permitted rather than forbidding +// traversal, because every deny-list in this repo has leaked at least once. +// +// LOWERCASE ONLY, and that is the point rather than a style choice. Windows and +// a default APFS volume fold case, so "Findings" and "findings" are one file +// there: writing the second silently replaced the first's body, a read could +// hand the model a note under a name it does not hold, and deleting one removed +// the other. Refusing the second spelling makes the collision unrepresentable +// rather than resolving it after the damage. +var namePattern = regexp.MustCompile(`^[a-z][a-z0-9-]{0,63}$`) + +// reservedDeviceNames are the DOS device names Win32 resolves ahead of any file +// with the same stem. os.Root addresses a note relative to a directory handle +// and bypasses that parsing entirely, so "con.md" is created, listed and read +// like any other note — which is exactly what makes this easy to miss. +// +// GIT is what breaks on them. `git add -A` fails outright on such a path and +// stages NOTHING, including the user's unrelated edits, while `git status` never +// names the file, so there is no route from the symptom back to the cause. The +// other direction is worse: a note committed from macOS makes the repo +// un-checkoutable on Windows — the clone fails and leaves an empty tree, so a +// Windows contributor gets no repository at all, not merely no note. +var reservedDeviceNames = map[string]bool{ + "con": true, "prn": true, "aux": true, "nul": true, + "com0": true, "com1": true, "com2": true, "com3": true, "com4": true, + "com5": true, "com6": true, "com7": true, "com8": true, "com9": true, + "lpt0": true, "lpt1": true, "lpt2": true, "lpt3": true, "lpt4": true, + "lpt5": true, "lpt6": true, "lpt7": true, "lpt8": true, "lpt9": true, +} + +var ( + ErrBadName = errors.New("a memory name must be lowercase, start with a letter, use only letters, digits and hyphen, be at most 64 characters, and not be a reserved device name") + ErrNoStore = errors.New("memory is not available in this run") + ErrTooLarge = fmt.Errorf("a memory note may be at most %d bytes", maxNoteBytes) + ErrNotFound = errors.New("no such memory") + ErrBadScope = errors.New(`scope must be "project" or "local"`) + // ErrNameClash is a note whose stored spelling differs from the one asked + // for. On a case-insensitive filesystem the two are the same file, so writing + // the second destroys the first — and List, which matches the extension + // exactly, does not show it, so the model is told the store is empty right + // before it overwrites something. + ErrNameClash = errors.New("a differently spelled note already occupies this name") + // ErrUnreadable is a path the confined handle will not open while it is + // plainly present on disk. Distinct from ErrNotFound, which is the answer + // that let a tampered store present as an empty one, and distinct from + // ErrIsSymlink, which names a reparse point this process could actually + // identify as one — a junction is not identifiable that way, and neither is + // a directory the process may not enter. + ErrUnreadable = errors.New("the memory store exists but could not be read") + // ErrNotPrivate is returned rather than writing a local note the repository + // would then track. The local scope's whole promise is that the note stays on + // this machine, and a promise that degrades quietly is worse than one that + // refuses. It covers both ways the promise can fail: an ignore rule that does + // not cover the store, and a store git already tracks — where no ignore rule + // applies at all. + ErrNotPrivate = errors.New("a note saved to the local memory store would not stay on this machine") + // ErrIsSymlink is pathjail's refusal, kept under this package's own name so + // callers testing for it keep working. It now covers a Windows junction as + // well as a symlink, which the old ModeSymlink-only check did not. + ErrIsSymlink = pathjail.ErrReparse +) + +// Paths locates the two scopes. An empty directory means that scope is simply +// unavailable, and a write to it is refused with a reason rather than silently +// written somewhere else. +type Paths struct { + // Root is the containment boundary. Every operation below is performed + // relative to a handle on it, so no component of ProjectDir or LocalDir can + // redirect a write or a delete outside the tree. Empty means no store: a + // boundary is not optional, because without one the directories below are + // just strings the filesystem re-resolves on every syscall. + Root string + ProjectDir string + LocalDir string +} + +// DefaultPaths puts project memory beside the repo and local memory in a +// subdirectory of it. +// +// The local store makes itself private on first write (keepLocalScopePrivate) +// rather than relying on a line in the workspace's .gitignore: this runs in +// whatever repository the user opened, and a rule that lives in one repo's +// ignore file protects only that repo. +func DefaultPaths(workspaceRoot string) Paths { + if strings.TrimSpace(workspaceRoot) == "" { + return Paths{} + } + base := filepath.Join(workspaceRoot, ".zero", "memory") + return Paths{Root: workspaceRoot, ProjectDir: base, LocalDir: filepath.Join(base, "local")} +} + +// Available reports whether this run has a usable store. +// +// ROOT COUNTS, and that is the fix rather than a nicety. openScope refuses a +// blank Root with ErrNoStore, and List treats ErrNoStore as "that scope is not +// configured" and moves on — so a Paths carrying both directories and no Root +// produced an empty listing, and the caller told the user there were no notes +// when the store was in fact switched off. Callers ask here instead of testing +// the fields themselves, so the rule has one home and cannot drift. +func (paths Paths) Available() bool { + if strings.TrimSpace(paths.Root) == "" { + return false + } + return paths.ProjectDir != "" || paths.LocalDir != "" +} + +func (paths Paths) dirFor(scope Scope) (string, error) { + switch scope { + case ScopeProject: + if paths.ProjectDir == "" { + return "", ErrNoStore + } + return paths.ProjectDir, nil + case ScopeLocal: + if paths.LocalDir == "" { + return "", ErrNoStore + } + return paths.LocalDir, nil + default: + return "", ErrBadScope + } +} + +// openScope opens a handle on the containment root and returns the scope's +// store directory relative to it. The caller closes the handle. +// +// Every filesystem operation in this file goes through here. The store used to +// Lstat its own directory and file and then hand those same strings to +// MkdirAll, CreateTemp, Rename and Remove, which re-resolve every ancestor: a +// link anywhere above the store redirected the write, and the checks passed +// because they were looking at the wrong components. On Windows they also +// missed a junction outright, since a junction is a reparse point but not a +// symlink. +func (paths Paths) openScope(scope Scope) (*os.Root, string, error) { + dir, err := paths.dirFor(scope) + if err != nil { + return nil, "", err + } + if strings.TrimSpace(paths.Root) == "" { + return nil, "", ErrNoStore + } + handle, relative, err := pathjail.Open(paths.Root, dir) + if err != nil { + return nil, "", err + } + // EVERY COMPONENT, not only the note. os.Root refuses to traverse OUT of the + // workspace, which is what the comment above was relying on — but it happily + // follows a link that resolves back INSIDE it, and the store path is checked + // in. A repository shipping ".zero -> redirected" redirected every read and + // write to another directory in the same tree while each individual check + // passed, because the only component ever inspected was the note file at the + // end. + if err := refuseReparseChain(handle, paths.Root, relative); err != nil { + handle.Close() + return nil, "", err + } + return handle, relative, nil +} + +// storedEntryName returns the spelling the store actually holds for a note, and +// whether anything holds it at all. +// +// LIST AND READ HAVE TO ANSWER THE SAME QUESTION. List matches the extension +// exactly; Read reopens name+".md", and on a case-insensitive filesystem that +// opens "findings.MD" — a file the listing never showed. The model is told the +// store is empty, writes, and a hand-authored checked-in note is destroyed. +// Reading the directory is what lets both sides agree on one spelling. +func storedEntryName(handle *os.Root, relative, name string) (string, bool, error) { + dir, err := handle.Open(relative) + if err != nil { + if errors.Is(err, fs.ErrNotExist) { + return "", false, nil + } + return "", false, err + } + defer dir.Close() + entries, err := dir.ReadDir(-1) + if err != nil { + return "", false, err + } + want := name + fileExt + for _, entry := range entries { + if entry.Name() == want { + return entry.Name(), true, nil + } + } + for _, entry := range entries { + if strings.EqualFold(entry.Name(), want) { + return entry.Name(), true, nil + } + } + return "", false, nil +} + +// presentOnDisk reports whether relative exists under root by ordinary pathname. +// +// Deliberately NOT through the confined handle — the whole point is to ask a +// different question than the handle answers, so that "the handle will not open +// this" can be told apart from "there is nothing here". It stats and never opens +// or reads, so nothing is traversed on the strength of this answer; it only +// decides which ERROR the caller is given. +func presentOnDisk(root string, relative string) bool { + if strings.TrimSpace(root) == "" { + return false + } + _, err := os.Lstat(filepath.Join(root, relative)) + return err == nil +} + +// refuseReparseChain refuses a link or reparse point at every component of +// relative, outermost first. +// +// Built on pathjail.RefuseReparse rather than reimplementing the test, so the +// Windows junction handling and the trailing-separator care stay in one place. +// Outermost first because that is the component whose redirection decides where +// everything below it lands, and it makes the error name the link the caller can +// actually act on. +func refuseReparseChain(handle *os.Root, root string, relative string) error { + relative = filepath.Clean(relative) + if relative == "." || relative == string(filepath.Separator) { + return nil + } + parts := strings.Split(relative, string(filepath.Separator)) + for i := range parts { + component := filepath.Join(parts[:i+1]...) + if err := pathjail.RefuseReparse(handle, component); err != nil { + return err + } + // OPENABLE, NOT MERELY PRESENT. A path the confined handle will not open + // while it is plainly present on disk has been REFUSED; a path absent from + // both is simply not there yet, which is what a first write is for. + // + // WHAT THIS ACTUALLY COVERS, corrected. This was written as a junction + // fix, and it is not one: @Vasanthdev2004 re-measured and every junction + // placement is already refused one layer earlier, because os.Root.Lstat + // reports a junction as ModeIrregular and pathjail.RefuseReparse rejects + // it on the first component — with that neutered, os.Root still refuses + // with "path escapes from parent". Two layers stand in front of this + // branch, and the tampered-store-reads-as-empty outcome it was built for + // does not occur. + // + // What it does cover is a store that is present and cannot be opened for + // an ordinary reason — a directory this process may not enter. That used + // to surface as "no such memory", and a caller cannot tell an empty store + // from an unreadable one on that answer. It is a smaller claim than the + // one this comment used to make, and it is the true one. + // + // RACE: between the open failing and presentOnDisk succeeding, a store + // being created concurrently reads as refused rather than absent. The + // caller is told to look rather than told nothing is there, so the + // direction is the safe one, but it is a real window. + opened, openErr := handle.Open(component) + if openErr == nil { + opened.Close() + continue + } + if presentOnDisk(root, component) { + return fmt.Errorf("%w: %s cannot be opened inside the workspace: %v", ErrUnreadable, component, openErr) + } + // Absent to both. Nothing below it can exist either, so there is nothing + // further to inspect. + return nil + } + return nil +} + +// Note is one stored memory. +type Note struct { + Name string + // Description is the one-line summary from frontmatter. It is what a listing + // shows, so a reader can decide what to open WITHOUT reading everything — + // which is the whole reason notes carry frontmatter at all. + Description string + Scope Scope + Body string +} + +// ValidName reports whether a name is storable. +// +// The reserved check is separate from the pattern because it is a different kind +// of rule: the pattern says which characters may appear, this says which +// otherwise-legal spellings the platform will not let git carry. +func ValidName(name string) bool { + return name != "" && len(name) <= 64 && + namePattern.MatchString(name) && !reservedDeviceNames[name] +} + +// DefaultScopeOrder is the ONE order an unscoped operation uses, and every tool +// that takes an optional scope must use it. +// +// LOCAL FIRST, because local is where an unscoped WRITE lands. It used to be +// project first while memory_write and memory_forget defaulted to local, so a +// model that omitted the scope — which every schema allows — could not read back +// what it had just written: +// +// memory_write{name, content} -> Saved "findings" (local). +// memory_read{name} -> memory "findings" (project) ... someone else's +// memory_forget{name} -> Forgot "findings" (local). +// memory_read{name} -> memory "findings" (project) ... still there +// +// Every one of those messages is accurate about the scope it acted on, and the +// round trip is still broken: the model is handed content it never wrote, and +// told a note was deleted after which the name still reads. A checked-in project +// note is the ordinary way that happens, since it arrives with a clone and the +// model has no reason to expect it. The tool's own approval text says future +// sessions will read and believe these notes, which is exactly what fails. +// +// A project note is still found when no local one shadows it, so nothing becomes +// unreachable — the ordering only decides which wins when both exist. +var defaultScopeOrder = []Scope{ScopeLocal, ScopeProject} + +// DefaultScopeOrder returns a COPY. Handing out the backing slice would let a +// caller reorder the resolution every other caller depends on. +func DefaultScopeOrder() []Scope { return append([]Scope(nil), defaultScopeOrder...) } + +// listingScopeOrder is the order a LISTING presents scopes in, which is not the +// order an unscoped lookup resolves them in. Named rather than left as a literal +// so the difference from DefaultScopeOrder is a stated decision instead of +// something a later reader tidies away. +var listingOrder = []Scope{ScopeProject, ScopeLocal} + +func listingScopeOrder() []Scope { return append([]Scope(nil), listingOrder...) } + +// ResolveScopes turns a requested scope into the scopes to search. +// +// ONE place decides, because there were two and they disagreed: the write path +// validated through memory.Scope and refused an unknown value, while the read +// path mapped every unrecognised spelling to BOTH stores. A typo therefore +// widened access on the only path where widening matters, and a perfectly valid +// "project" was ignored on the listing path, which always read both. An empty +// request means "search everywhere"; anything non-empty must be a scope this +// package knows. +func ResolveScopes(requested string) ([]Scope, error) { + trimmed := strings.TrimSpace(requested) + if trimmed == "" { + return DefaultScopeOrder(), nil + } + switch scope := Scope(strings.ToLower(trimmed)); scope { + case ScopeProject, ScopeLocal: + return []Scope{scope}, nil + default: + return nil, ErrBadScope + } +} + +// List returns every note in the given scopes, project first, each sorted by +// name, along with any store that could not be read. +// +// LOCAL SHADOWS NOTHING. Unlike saved plans, where project shadows user because +// a repo's own plan is what its contributors should get, both scopes are listed: +// they hold different KINDS of thing, and hiding one behind the other would lose +// a note rather than resolve a conflict. +// +// The error is JOINED rather than returned in place of the notes: a store that +// cannot be read must be reported, but the notes that did read are still the +// best answer available, and dropping them would turn one unreadable file into +// an empty memory. +func List(paths Paths, scopes ...Scope) ([]Note, error) { + if len(scopes) == 0 { + // DELIBERATELY NOT DefaultScopeOrder. These are two different decisions + // and the review that flagged the duplication read them as one: + // + // resolution order decides which note WINS when both scopes hold the + // name, and must be local-first or an unscoped write cannot be read + // back + // listing order decides only what a reader sees FIRST — both scopes are + // shown and neither shadows the other, so nothing is being resolved + // + // Project first here puts the checked-in, team-facing notes at the top of + // a long listing, which is what a reader scanning it wants. Making this + // follow the resolution order would change that for no gain, and + // TestListingShowsBothScopesWithoutShadowing asserts it on purpose. + scopes = listingScopeOrder() + } + var out []Note + var problems []error + for _, scope := range scopes { + handle, relative, err := paths.openScope(scope) + if err != nil { + // A scope this store is not configured for is not a failure to + // report; a bad scope name is. + if !errors.Is(err, ErrNoStore) { + problems = append(problems, err) + } + continue + } + directory, err := handle.Open(relative) + if err != nil { + handle.Close() + // A store that has never been written to has no directory yet, and + // that is an empty list rather than a failure. Anything else is an + // operational error and is reported. + if !os.IsNotExist(err) { + problems = append(problems, fmt.Errorf("open %s store: %w", scope, err)) + } + continue + } + entries, err := directory.ReadDir(-1) + directory.Close() + handle.Close() + if err != nil { + problems = append(problems, fmt.Errorf("read %s store: %w", scope, err)) + continue + } + var scoped []Note + for _, entry := range entries { + // The extension is matched EXACTLY, not case-insensitively. Read + // reopens name+fileExt, which is always lowercase, so accepting + // "notes.MD" here listed an entry that the very next Read could not + // open on a case-sensitive filesystem — and the error was swallowed + // below, so the note vanished from the listing with no explanation. + // This store only ever writes lowercase, so an exact match is the + // spelling that keeps List and Read agreeing. + if entry.IsDir() || filepath.Ext(entry.Name()) != fileExt { + continue + } + name := strings.TrimSuffix(entry.Name(), fileExt) + note, err := Read(paths, scope, name) + if err != nil { + // A name the store would not accept, or a note deleted between + // the ReadDir and the read, is legitimately not listable. A + // refused link, an oversized file or a permission error is a + // problem the caller needs told about rather than a note that + // silently vanishes from the listing. + if !errors.Is(err, ErrNotFound) && !errors.Is(err, ErrBadName) { + problems = append(problems, fmt.Errorf("read %s/%s: %w", scope, name, err)) + } + continue + } + scoped = append(scoped, note) + } + sort.Slice(scoped, func(i, j int) bool { return scoped[i].Name < scoped[j].Name }) + out = append(out, scoped...) + } + return out, errors.Join(problems...) +} + +// Read returns one note. +func Read(paths Paths, scope Scope, name string) (Note, error) { + if !ValidName(name) { + return Note{}, ErrBadName + } + handle, relative, err := paths.openScope(scope) + if err != nil { + return Note{}, err + } + defer handle.Close() + relativePath := filepath.Join(relative, name+fileExt) + // Reads are confined by the same rule as writes. A note read through a link + // is an exfiltration primitive in a tool the model can call by name — the + // write hole with the arrow reversed — and Write and Forget both refuse a + // reparse point at this position, so a read that did not was the asymmetry. + // + // WHAT THIS COVERS, PRECISELY. os.Root already refuses a link whose target + // leaves the root, so what this adds is refusing one that stays INSIDE it: + // ".zero/memory/linked.md -> ../../secret.txt" resolves to a path still under + // Paths.Root and was served happily. It is a REPARSE-POINT guard, not an + // identity check — a HARD LINK carries no reparse bit, so neither this nor + // os.Root can see one, and a hard link at a note position still reads through + // to its target. Closing that needs identity (st_dev/st_ino, or the Windows + // file id), which is deliberately not attempted here so that the claim in + // this comment matches what the code actually does. + if err := pathjail.RefuseReparse(handle, relativePath); err != nil { + return Note{}, err + } + // AGREE WITH LIST. List matches the extension exactly, so a differently + // spelled entry is one it never showed — and opening it here would hand back + // a note the caller was told does not exist. + stored, found, entryErr := storedEntryName(handle, relative, name) + if entryErr != nil { + return Note{}, entryErr + } + if found && stored != name+fileExt { + return Note{}, clashError(name, stored) + } + body, err := readBounded(handle, relativePath) + if err != nil { + if os.IsNotExist(err) { + return Note{}, ErrNotFound + } + // Anything else is an operational failure — a permission error, an + // oversized file, a corrupt store — and reaches the caller as itself. + // Reporting it as "no such note" tells the model the note is absent, and + // the next thing it does is write the note again over whatever is there. + return Note{}, err + } + description, text := splitFrontmatter(string(body)) + return Note{Name: name, Description: description, Scope: scope, Body: text}, nil +} + +// keepLocalScopePrivate establishes that a local note will not enter the +// repository: git must be tracking nothing in the store, and the ignore that +// keeps future notes out of it must be installed or already in force. +// +// "local" promises the note stays on this machine, and it did not: the store +// lives at /.zero/memory/local, inside the working tree, so a default +// write showed up in git status and could be committed — and the local scope is +// the DEFAULT, so that is the ordinary path rather than a corner. +// +// The ignore file lives INSIDE the store rather than as a line in the repo's +// .gitignore, because this tool runs in whatever workspace the user opened. A +// line in zero's own .gitignore would protect exactly one repository; a store +// that makes itself private travels with every one. "*" covers the notes and the +// ignore file itself, so the directory contributes nothing to the index. +// +// IT FAILS CLOSED, and used to fail open. This was best-effort with no error +// return, on the reasoning that a store which cannot hold an ignore file is +// still a working store and failing the write would trade privacy for an outage. +// That reasoning is wrong for this particular function, because O_EXCL cannot +// tell "a previous run wrote the ignore" from "something else is already there": +// precreating an empty .gitignore made every subsequent local write succeed with +// the store fully tracked. The promise is the feature here — a note the user was +// told stays on this machine, sitting in git status, is worse than a refused +// write, because the refusal is visible and the leak is not. +func keepLocalScopePrivate(handle *os.Root, scope Scope, relative, dir string) error { + if scope != ScopeLocal { + return nil + } + // THE INDEX DECIDES, NOT THE IGNORE FILE, and the ignore file used to be the + // only thing consulted. Git applies no ignore rule to a path already in the + // index, so "*" proves privacy for an UNTRACKED path and for nothing else: a + // clone, an earlier `git add -f`, or a hand-authored repository restores both + // the ignore AND local/.md as TRACKED files, and the rename below then + // replaced a tracked file with a body the user was told stays on this + // machine — sitting in git status, one `git commit -a` from being shared. + // Reported by @jatmn. + // + // It gates BOTH branches below, deliberately. Refusing only where an existing + // ignore is accepted leaves the same leak reachable by deleting the ignore + // from the worktree: O_EXCL would then create a fresh one, report a clean + // first write, and overwrite the tracked note anyway. + if err := refuseTrackedLocalStore(handle, relative, dir); err != nil { + return err + } + ignorePath := filepath.Join(relative, gitignoreName) + // O_EXCL still decides whether this is the first write, in one syscall — but + // now the "already exists" answer leads to a check rather than to silence. + file, err := handle.OpenFile(ignorePath, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0o600) + switch { + case err == nil: + if writeErr := writeLocalIgnore(file); writeErr != nil { + _ = file.Close() + // Do not leave the exclusive-create placeholder behind. An empty or + // partial ignore makes every retry take the existing-file branch and + // fail as ErrNotPrivate instead of retrying the installation. + return removeIncompleteLocalIgnore(handle, ignorePath, fmt.Errorf("write %s: %w", ignorePath, writeErr)) + } + if closeErr := closeLocalIgnore(file); closeErr != nil { + // A failed close does not establish that the ignore reached disk. Remove + // it for the same reason as a failed write so the next call can retry. If + // cleanup also fails, preserve both errors: callers must know that an + // incomplete placeholder remains and needs manual recovery. + return removeIncompleteLocalIgnore(handle, ignorePath, fmt.Errorf("close %s: %w", ignorePath, closeErr)) + } + return nil + case !errors.Is(err, fs.ErrExist): + return fmt.Errorf("create %s: %w", ignorePath, err) + } + // THE IGNORE FILE ITSELF MUST NOT BE A LINK. git does not follow a symlinked + // .gitignore — it warns and treats the rule as absent — so reading through + // one and accepting the target's "*" certified a privacy rule that is not in + // force. An in-workspace RELATIVE link is followed by os.Root (an absolute + // one is refused, which is why an earlier check of this looked clean), so the + // bypass needed nothing outside the workspace. + if err := pathjail.RefuseReparse(handle, ignorePath); err != nil { + return fmt.Errorf("%w: %s is a link, and git does not read one: %v", ErrNotPrivate, ignorePath, err) + } + existing, err := readBounded(handle, ignorePath) + if err != nil { + return fmt.Errorf("read %s: %w", ignorePath, err) + } + if !ignoresEverything(string(existing)) { + return fmt.Errorf("%w: %s does not ignore the whole store", ErrNotPrivate, ignorePath) + } + return nil +} + +var writeLocalIgnore = func(file *os.File) error { + _, err := file.WriteString(localIgnoreContent) + return err +} + +var closeLocalIgnore = func(file *os.File) error { + return file.Close() +} + +var removeLocalIgnore = func(handle *os.Root, path string) error { + return handle.Remove(path) +} + +func removeIncompleteLocalIgnore(handle *os.Root, path string, cause error) error { + if err := removeLocalIgnore(handle, path); err != nil { + return errors.Join(cause, fmt.Errorf("remove incomplete %s: %w", path, err)) + } + return cause +} + +// ignoresEverything reports whether an existing ignore file actually excludes the +// whole directory. +// +// A bare "*" is what this store writes, so that is what is recognised; anything +// narrower is treated as not covering, because guessing at the effect of an +// arbitrary pattern set is how a privacy check ends up agreeing with a file that +// does not protect anything. A re-inclusion line cancels the cover no matter +// where it sits, so one "!" is enough to fail the whole file. +// +// IT FOLLOWS GIT'S WHITESPACE RULES, and trimming each line got both of them +// backwards. Verified against git 2.55.0 rather than read off the spec: +// +// " *" gate said covered; git does NOT ignore, because LEADING whitespace +// is part of the pattern — so a note the user was told stays on this +// machine was picked up by a routine `git add -A`, which is the exact +// outcome this function exists to prevent, arrived at silently +// "\ufeff*" gate said not covered; git strips a UTF-8 BOM and honours the rule +// — so an ignore written by an ordinary editor failed every local write +// +// Leading whitespace is therefore significant and is NOT stripped; trailing +// whitespace is not significant to git and is; a BOM is stripped once, at the +// start of the file, exactly as git does. +func ignoresEverything(content string) bool { + content = strings.TrimPrefix(content, "\ufeff") + covered := false + for _, line := range strings.Split(content, "\n") { + // TrimRight, not TrimSpace: git drops trailing whitespace from a pattern + // and keeps leading whitespace as part of it. + line = strings.TrimRight(strings.TrimSuffix(line, "\r"), " \t") + if line == "" || strings.HasPrefix(line, "#") { + continue + } + if strings.HasPrefix(line, "!") { + return false + } + if line == "*" { + covered = true + } + } + return covered +} + +// refuseTrackedLocalStore refuses a local write whose store git already TRACKS. +// +// Filesystem discovery first excludes inert .git markers without requiring a +// git installation. A marker containing repository metadata is only a candidate: +// git must then answer the index question. A damaged repository or an unavailable +// index remains a refusal; an empty directory or broken gitdir stub is not by +// itself evidence that the store belongs to a repository. +func refuseTrackedLocalStore(handle *os.Root, relative, dir string) error { + // The physical path, because that is the one git will see: os/exec chdirs + // into it, and git discovers the repository from the resulting getcwd. A + // lexical walk up a symlinked workspace looks at ancestors the child process + // never has, and can find no repository where git finds one. + // + // THIS BRANCH AND THE ONE BELOW ARE DEFENCE IN DEPTH, not live paths, and + // that is worth writing down because a reviewer will otherwise read them as + // untested. Neither is reachable through Write: refuseReparseChain has + // already refused every link in the chain, and opening the rooted handle has + // already failed on an unreadable ancestor, so by the time control arrives + // here the path resolves and the walk can read. Verified by trying both — a + // self-referential store link is refused as a link, and a 000 ancestor fails + // at mkdir, neither reaching this function. + // + // They stay because the reachability is a property of the CALLERS, not of + // this function, and this function's contract is that an unanswerable + // privacy question is a refusal. A future caller that skips the earlier + // guards would otherwise inherit a silent fail-open. + resolved, err := resolvePhysicalPath(handle, relative, dir) + if err != nil { + return fmt.Errorf("%w: cannot resolve %s to ask git about it: %v", ErrNotPrivate, dir, err) + } + repo, err := enclosingGitRepo(resolved) + if err != nil { + return fmt.Errorf("%w: cannot tell whether %s is inside a git repository: %v", ErrNotPrivate, dir, err) + } + if repo == "" { + // Nothing encloses the store, so nothing can be tracked and there is + // nothing to ask. This is also where a machine with no git at all lands, + // which is why the question above is not a subprocess. + return nil + } + tracked, err := trackedStoreEntries(resolved) + if err != nil { + return fmt.Errorf("%w: %s is inside the git repository at %s, and git could not say which files there are tracked: %v", + ErrNotPrivate, dir, repo, err) + } + offenders := make([]string, 0, len(tracked)) + for _, entry := range tracked { + // The store's own ignore file is the one tracked entry that is not a leak: + // it carries no note, and a repository that checks it in hands every clone + // the rule before the first write. Anything else under this directory is a + // note the local scope promised would not be in the repository. + if entry == gitignoreName { + continue + } + offenders = append(offenders, entry) + } + if len(offenders) == 0 { + return nil + } + sort.Strings(offenders) + return fmt.Errorf("%w: the repository at %s tracks %s in %s, and git applies no ignore rule to a path already in the index; remove it with `git rm --cached` before saving a local note", + ErrNotPrivate, repo, namedEntries(offenders), dir) +} + +// enclosingGitRepo returns the nearest ancestor of dir, dir itself included, +// carrying possible repository metadata, or "" when none does. Inert markers +// do not stop the walk: a real enclosing checkout may still track the store. +func enclosingGitRepo(dir string) (string, error) { + for current := filepath.Clean(dir); ; { + candidate, err := gitMarkerHasMetadata(filepath.Join(current, gitDirName)) + if err != nil { + return "", err + } + if candidate { + return current, nil + } + parent := filepath.Dir(current) + if parent == current { + return "", nil + } + current = parent + } +} + +// gitMarkerHasMetadata only rules out markers that cannot name a repository. +// It does not certify one: even partial/corrupt metadata must reach git and fail +// closed if the index cannot be read. Following a gitdir reference is read-only; +// it never grants filesystem access for a note operation. +func gitMarkerHasMetadata(marker string) (bool, error) { + info, err := os.Stat(marker) + if errors.Is(err, fs.ErrNotExist) { + return false, nil + } + if err != nil { + return false, err + } + dir := marker + if !info.IsDir() { + if !info.Mode().IsRegular() || info.Size() > 4096 { + return false, fmt.Errorf("cannot classify git marker %s", marker) + } + file, err := os.Open(marker) + if err != nil { + return false, err + } + content, readErr := io.ReadAll(io.LimitReader(file, 4097)) + closeErr := file.Close() + if err := errors.Join(readErr, closeErr); err != nil { + return false, err + } + if len(content) > 4096 { + return false, fmt.Errorf("git marker %s exceeds 4096 bytes", marker) + } + if !strings.HasPrefix(string(content), "gitdir: ") { + return false, nil + } + dir = strings.TrimRight(string(content[len("gitdir: "):]), "\r\n") + if dir == "" { + return false, nil + } + if !filepath.IsAbs(dir) { + dir = filepath.Join(filepath.Dir(marker), dir) + } + info, err = os.Stat(dir) + if errors.Is(err, fs.ErrNotExist) { + return false, nil + } + if err != nil { + return false, err + } + if !info.IsDir() { + return false, nil + } + } + for _, name := range []string{"HEAD", "objects", "refs", "index", "commondir", "config"} { + if _, err := os.Lstat(filepath.Join(dir, name)); err == nil { + return true, nil + } else if !errors.Is(err, fs.ErrNotExist) { + return false, err + } + } + return false, nil +} + +// trackedStoreEntries lists what git's index holds under dir, named relative to +// it. +// +// The store directory goes in as the WORKING DIRECTORY and nothing goes in as an +// argument: `git ls-files` run inside a subdirectory already lists that +// subdirectory and names its paths relative to it, so there is no pathspec for a +// note name to reach, and no path for this side to spell differently from the +// way the kernel just resolved it. -z because a tracked path may hold anything a +// filename can and git quotes such a name in its default output. Both the +// subcommand and the flag predate any version this project could plausibly meet. +func trackedStoreEntries(dir string) ([]string, error) { + ctx, cancel := context.WithTimeout(context.Background(), gitIndexTimeout) + defer cancel() + cmd := exec.CommandContext(ctx, "git", "ls-files", "-z") + cmd.Dir = dir + // Cancelling the context does not by itself return control here: WaitDelay is + // what stops a git holding its pipes open from outliving the deadline. + cmd.WaitDelay = gitIndexTimeout + cmd.Env = gitDiscoveryEnv(os.Environ()) + var stdout, stderr bytes.Buffer + cmd.Stdout = &stdout + cmd.Stderr = &stderr + if err := cmd.Run(); err != nil { + if detail := singleLine(stderr.String()); detail != "" { + return nil, fmt.Errorf("%w: %s", err, detail) + } + return nil, err + } + entries := make([]string, 0, 4) + for _, entry := range strings.Split(stdout.String(), "\x00") { + if entry != "" { + entries = append(entries, entry) + } + } + return entries, nil +} + +// gitDiscoveryEnv drops the variables that would point git at an index other +// than the one belonging to the checkout the store sits in. +// +// A hook, a rebase, or any `git` wrapper exports GIT_DIR and GIT_INDEX_FILE into +// the environment its children inherit, and zero inherits them like anything +// else. Left in place they answer this question about a different index — during +// a rebase, one holding none of the worktree's paths — and the answer is +// "nothing is tracked", which is precisely the fail-open being closed here. The +// repository was located from the filesystem at the store, so the index is +// looked up the same way. +func gitDiscoveryEnv(environ []string) []string { + redirects := []string{"GIT_DIR=", "GIT_COMMON_DIR=", "GIT_WORK_TREE=", "GIT_INDEX_FILE="} + filtered := make([]string, 0, len(environ)) + for _, entry := range environ { + redirected := false + for _, prefix := range redirects { + if strings.HasPrefix(entry, prefix) { + redirected = true + break + } + } + if !redirected { + filtered = append(filtered, entry) + } + } + return filtered +} + +// namedEntries spells out at most maxTrackedNamed of them and counts the rest. +func namedEntries(entries []string) string { + if len(entries) <= maxTrackedNamed { + return strings.Join(entries, ", ") + } + return fmt.Sprintf("%s and %d more", strings.Join(entries[:maxTrackedNamed], ", "), len(entries)-maxTrackedNamed) +} + +// readBounded reads at most maxNoteBytes, refusing anything larger rather than +// allocating it. +// +// The ceiling used to be enforced only on the way IN, so a note that arrived by +// hand or through a clone — project scope is checked in — was read whole however +// large, and List did that for every note in the store. A memory bound has to +// hold on the path that allocates. +func readBounded(handle *os.Root, relativePath string) ([]byte, error) { + file, err := handle.Open(relativePath) + if err != nil { + return nil, err + } + defer file.Close() + // One byte past the ceiling, so a note exactly at the limit still reads and + // only a genuinely oversized one is refused. + body, err := io.ReadAll(io.LimitReader(file, maxNoteBytes+1)) + if err != nil { + return nil, err + } + if len(body) > maxNoteBytes { + return nil, fmt.Errorf("%w: %s", ErrTooLarge, relativePath) + } + return body, nil +} + +// Write stores a note, replacing any note of the same name in the same scope. +// +// Every step runs against a handle on the containment root, so the directory +// this lands in cannot be changed underneath it: create the tree, refuse a link +// at the destination, write an O_EXCL temp file with an unpredictable name, +// rename into place. An edit is therefore atomic, and a crash mid-write leaves +// the previous note rather than a half-written one. +func Write(paths Paths, scope Scope, name, description, body string) (string, error) { + if !ValidName(name) { + return "", ErrBadName + } + dir, err := paths.dirFor(scope) + if err != nil { + return "", err + } + content := renderNote(name, description, body) + if len(content) > maxNoteBytes { + return "", ErrTooLarge + } + handle, relative, err := paths.openScope(scope) + if err != nil { + return "", err + } + defer handle.Close() + if err := handle.MkdirAll(relative, 0o700); err != nil { + return "", fmt.Errorf("create %s: %w", dir, err) + } + if err := keepLocalScopePrivate(handle, scope, relative, dir); err != nil { + return "", err + } + path := filepath.Join(dir, name+fileExt) + relativePath := filepath.Join(relative, name+fileExt) + if err := pathjail.RefuseReparse(handle, relativePath); err != nil { + return "", err + } + // A REAL CLASH GUARD. On a case-insensitive filesystem "findings.md" and + // "findings.MD" are one file, so writing the first destroys the second — and + // List never showed it, so the model had every reason to think the name was + // free. Refusing names the occupant instead of silently taking its place. + if stored, found, entryErr := storedEntryName(handle, relative, name); entryErr != nil { + return "", entryErr + } else if found && stored != name+fileExt { + return "", clashError(name, stored) + } + file, temp, err := pathjail.CreateTemp(handle, relative, name, tempExt) + if err != nil { + return "", fmt.Errorf("create a temporary file in %s: %w", dir, err) + } + writeErr := func() error { + if _, err := file.WriteString(content); err != nil { + return err + } + return file.Close() + }() + if writeErr != nil { + _ = file.Close() + _ = handle.Remove(temp) + return "", fmt.Errorf("write %s: %w", path, writeErr) + } + if err := handle.Rename(temp, relativePath); err != nil { + _ = handle.Remove(temp) + return "", fmt.Errorf("save %s: %w", path, err) + } + return path, nil +} + +// Forget removes a note. Missing is not an error: the caller asked for it to be +// gone, and it is. +func Forget(paths Paths, scope Scope, name string) error { + if !ValidName(name) { + return ErrBadName + } + handle, relative, err := paths.openScope(scope) + if err != nil { + return err + } + defer handle.Close() + // A delete is the sharpest of these: a write lands a file, a delete removes + // somebody else's. Same handle, same reason. + relativePath := filepath.Join(relative, name+fileExt) + if err := pathjail.RefuseReparse(handle, relativePath); err != nil { + return err + } + // THE SAME CLASH GUARD READ AND WRITE ALREADY HAVE. It belongs here most of + // all: on a case-insensitive filesystem "findings.md" and "findings.MD" are + // one file, so a delete addressed to the first removes the second — and this + // is the operation with nothing left to inspect afterwards. Two doors were + // closed and this one, the sharpest, was open; worse, the refusal the other + // two return said "rename or remove it first", and removing is what this + // function did. The clash message no longer points at a door that destroys + // the file it is describing. Reported by @Vasanthdev2004. + // + // Absence is still not an error. A name nobody has stored has no occupant to + // clash with, so the idempotence this function documents is unchanged. + if stored, found, entryErr := storedEntryName(handle, relative, name); entryErr != nil { + return entryErr + } else if found && stored != name+fileExt { + return clashError(name, stored) + } + if err := handle.Remove(relativePath); err != nil && !os.IsNotExist(err) { + return err + } + return nil +} + +// clashError is the one wording for all three doors. It names the occupant and +// then says the only thing that actually resolves the clash: the stored file has +// a spelling this package cannot address, so it has to be renamed in the store +// directory. It deliberately does NOT suggest deleting the note through this +// package — that was the previous advice, and the delete took the occupant with +// it. +func clashError(name, stored string) error { + return fmt.Errorf("%w: %s is stored as %s; rename that file to %s in the store directory to manage it here", + ErrNameClash, name+fileExt, stored, name+fileExt) +} + +func renderNote(name, description, body string) string { + var b strings.Builder + b.WriteString("---\nname: ") + b.WriteString(name) + if trimmed := strings.TrimSpace(description); trimmed != "" { + b.WriteString("\ndescription: ") + b.WriteString(boundedDescription(singleLine(trimmed))) + } + b.WriteString("\n---\n\n") + b.WriteString(strings.TrimRight(body, "\n")) + b.WriteString("\n") + return b.String() +} + +// splitFrontmatter returns the description and the body. A note without +// frontmatter is not an error — it is a file someone wrote by hand, and losing +// it because it lacks a header would be the store punishing the reader it exists +// to serve. +// +// CRLF is accepted as well as LF. Project-scope notes are checked in, and Git for +// Windows defaults to autocrlf=true, so a note that merely round-trips through a +// clone comes back with "---\r\n" — under an LF-only split the whole header, +// delimiters included, fell through into the body and the description was lost. +// +// LINE ENDINGS ARE NORMALISED TO LF in what this returns, on every path. The +// earlier version normalised only for the split and then returned the body from +// whichever string that path happened to hold, so a note WITH frontmatter came +// back as LF and one WITHOUT kept its CRLF — a difference no caller asked for and +// nothing documented. The file on disk is untouched either way; this is only +// what the reader is handed. +// boundedDescription caps the summary so one note cannot crowd out the listing. +// Truncated on a rune boundary with an ellipsis, so the result stays readable and +// is visibly cut rather than looking like the whole of a short description. +func boundedDescription(text string) string { + if len(text) <= maxDescriptionBytes { + return text + } + const ellipsis = "…" + // WALK FORWARD ONCE. The first version dropped a rune per iteration and + // re-encoded the whole slice each time to measure it, which is quadratic in + // the input — and maxNoteBytes lets a 64 KiB description through the write + // path, so the cap was at its slowest on exactly the input it exists for: + // + // 1 KiB -> 1.6ms 16 KiB -> 381ms 64 KiB -> 13.3s + // + // Ranging over the string yields rune boundaries with their byte offsets + // directly, so the cut point is found in one pass and no intermediate string + // is built. + budget := maxDescriptionBytes - len(ellipsis) + cut := 0 + for index := range text { + if index > budget { + break + } + cut = index + } + return text[:cut] + ellipsis +} + +func splitFrontmatter(content string) (description string, body string) { + normalized := strings.ReplaceAll(content, "\r\n", "\n") + if !strings.HasPrefix(normalized, "---\n") { + return "", normalized + } + rest := normalized[len("---\n"):] + end := strings.Index(rest, "\n---\n") + if end < 0 { + return "", normalized + } + for _, line := range strings.Split(rest[:end], "\n") { + if value, ok := strings.CutPrefix(strings.TrimSpace(line), "description:"); ok { + // Bounded HERE, not only where a note is written. A project-scope + // note arrives with a clone, so its description never passed through + // this process's write path — leaving the listing crowdable by a file + // nobody here created. + description = boundedDescription(strings.TrimSpace(value)) + } + } + return description, strings.TrimLeft(rest[end+len("\n---\n"):], "\n") +} + +func singleLine(text string) string { + return strings.Join(strings.Fields(text), " ") +} diff --git a/internal/memory/memory_test.go b/internal/memory/memory_test.go new file mode 100644 index 000000000..63cff3e7f --- /dev/null +++ b/internal/memory/memory_test.go @@ -0,0 +1,532 @@ +package memory + +import ( + "errors" + "os" + "path/filepath" + "slices" + "strings" + "testing" + "time" + "unicode/utf8" +) + +func testPaths(t *testing.T) Paths { + t.Helper() + root, err := filepath.EvalSymlinks(t.TempDir()) + if err != nil { + t.Fatal(err) + } + return DefaultPaths(root) +} + +func TestANoteRoundTripsThroughItsScope(t *testing.T) { + paths := testPaths(t) + for _, scope := range []Scope{ScopeProject, ScopeLocal} { + if _, err := Write(paths, scope, "conventions", "how this repo does errors", "Wrap with %w.\n"); err != nil { + t.Fatalf("%s: write: %v", scope, err) + } + note, err := Read(paths, scope, "conventions") + if err != nil { + t.Fatalf("%s: read: %v", scope, err) + } + if note.Description != "how this repo does errors" { + t.Errorf("%s: description = %q", scope, note.Description) + } + if !strings.Contains(note.Body, "Wrap with %w.") { + t.Errorf("%s: body = %q", scope, note.Body) + } + if note.Scope != scope { + t.Errorf("scope = %q, want %q", note.Scope, scope) + } + } +} + +// BOTH SCOPES ARE LISTED. Unlike saved plans, where project shadows user, these +// hold different KINDS of thing — hiding one behind the other would lose a note +// rather than resolve a conflict. +func TestListingShowsBothScopesWithoutShadowing(t *testing.T) { + paths := testPaths(t) + // The project notes are written in REVERSE lexical order, so the per-scope + // sort has to do real work to produce the expected result: remove or reverse + // it and these come back in insertion order. Ordering is what a reader relies + // on to find a note in a long listing, so it is worth asserting rather than + // only checking that both scopes appear. + if _, err := Write(paths, ScopeProject, "zebra", "last alphabetically", "x"); err != nil { + t.Fatal(err) + } + if _, err := Write(paths, ScopeProject, "shared", "team convention", "x"); err != nil { + t.Fatal(err) + } + if _, err := Write(paths, ScopeProject, "alpha", "first alphabetically", "x"); err != nil { + t.Fatal(err) + } + if _, err := Write(paths, ScopeLocal, "shared", "my own note", "y"); err != nil { + t.Fatal(err) + } + notes, listErr := List(paths) + if listErr != nil { + t.Fatal(listErr) + } + var got []string + for _, note := range notes { + got = append(got, string(note.Scope)+"/"+note.Name) + } + want := []string{"project/alpha", "project/shared", "project/zebra", "local/shared"} + if !slices.Equal(got, want) { + t.Errorf("listing = %v,\n want %v — project scope first, each scope sorted by name, and neither scope shadowing the other's \"shared\"", got, want) + } +} + +// THE NAME IS THE PATH GUARD, an allow-list for the same reason plan names use +// one: every deny-list in this repo has leaked at least once. +func TestNamesCannotTraverse(t *testing.T) { + paths := testPaths(t) + for _, name := range []string{ + "../escape", "..", ".", "a/b", `a\b`, "a b", "", strings.Repeat("x", 65), + "~/evil", "a;b", "a\x00b", "note.md", + // Uppercase and underscore are refused now, not merely unconventional. + // Windows and a default APFS volume fold case, so "Findings" and + // "findings" would be one file: refusing the second spelling makes that + // collision unrepresentable rather than something to resolve afterwards. + "Findings", "A1", "audit_findings", "1leading", + // Reserved DOS device names. The store itself handles them fine — os.Root + // bypasses Win32 device parsing — but `git add -A` then fails on the path + // and stages NOTHING, and a repo carrying one cannot be checked out on + // Windows at all. + "con", "prn", "aux", "nul", "com1", "lpt9", "CON", "Nul", + } { + if _, err := Write(paths, ScopeProject, name, "d", "b"); !errors.Is(err, ErrBadName) { + t.Errorf("Write accepted %q: %v", name, err) + } + if _, err := Read(paths, ScopeProject, name); !errors.Is(err, ErrBadName) { + t.Errorf("Read accepted %q: %v", name, err) + } + } + // A device name is reserved only as the WHOLE stem; it is ordinary inside a + // longer name, and refusing those too would be a deny-list overreaching. + for _, name := range []string{"conventions", "decision-2", "console", "nullable", "com10"} { + if _, err := Write(paths, ScopeProject, name, "d", "b"); err != nil { + t.Errorf("Write rejected %q: %v", name, err) + } + } +} + +// A LINK AT THE NOTE'S OWN POSITION IS REFUSED. A note store is a write +// primitive pointed at a path the model chooses, which is the same shape as +// "save my plan" and needs the same answer. +// +// Uses linkDir rather than os.Symlink so it runs on WINDOWS too. os.Symlink +// needs SeCreateSymbolicLinkPrivilege, which an ordinary account lacks, so the +// old version skipped there — leaving the reparse guard with no coverage on the +// one platform where a junction makes the attack reachable without privilege. +// Both Write and Forget are checked, since each has its own RefuseReparse call. +func TestWritingRefusesToFollowALinkAtTheNotePosition(t *testing.T) { + paths := testPaths(t) + base := filepath.Dir(paths.ProjectDir) + if err := os.MkdirAll(paths.ProjectDir, 0o700); err != nil { + t.Fatal(err) + } + // A directory target, because a Windows junction can only point at one. + target := filepath.Join(base, "precious") + if err := os.MkdirAll(target, 0o700); err != nil { + t.Fatal(err) + } + sentinel := filepath.Join(target, "keepme") + if err := os.WriteFile(sentinel, []byte("do not clobber"), 0o600); err != nil { + t.Fatal(err) + } + linkDir(t, target, filepath.Join(paths.ProjectDir, "evil"+fileExt)) + + if _, err := Write(paths, ScopeProject, "evil", "d", "b"); !errors.Is(err, ErrIsSymlink) { + t.Fatalf("Write followed a link: %v", err) + } + if err := Forget(paths, ScopeProject, "evil"); !errors.Is(err, ErrIsSymlink) { + t.Fatalf("Forget followed a link: %v", err) + } + body, err := os.ReadFile(sentinel) + if err != nil { + t.Fatalf("the link target was disturbed: %v", err) + } + if string(body) != "do not clobber" { + t.Fatalf("the link target was overwritten: %q", body) + } +} + +// An unavailable scope is refused with a reason rather than written elsewhere. +func TestAnUnavailableScopeIsRefused(t *testing.T) { + if _, err := Write(Paths{}, ScopeProject, "x", "d", "b"); !errors.Is(err, ErrNoStore) { + t.Errorf("a write with no store configured returned %v", err) + } + if _, err := Write(testPaths(t), Scope("elsewhere"), "x", "d", "b"); !errors.Is(err, ErrBadScope) { + t.Errorf("an unknown scope returned %v", err) + } +} + +// A note written by hand, without frontmatter, must still be readable — losing +// it would be the store punishing the reader it exists to serve. +func TestAHandWrittenNoteWithoutFrontmatterStillReads(t *testing.T) { + paths := testPaths(t) + if err := os.MkdirAll(paths.ProjectDir, 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(paths.ProjectDir, "manual"+fileExt), []byte("just some prose\n"), 0o600); err != nil { + t.Fatal(err) + } + note, err := Read(paths, ScopeProject, "manual") + if err != nil { + t.Fatalf("read: %v", err) + } + if !strings.Contains(note.Body, "just some prose") { + t.Errorf("body = %q", note.Body) + } +} + +func TestOversizedNotesAreRefusedAndForgetIsIdempotent(t *testing.T) { + paths := testPaths(t) + if _, err := Write(paths, ScopeProject, "big", "d", strings.Repeat("x", maxNoteBytes+1)); !errors.Is(err, ErrTooLarge) { + t.Errorf("an oversized note was accepted: %v", err) + } + if err := Forget(paths, ScopeProject, "never-existed"); err != nil { + t.Errorf("forgetting a missing note errored: %v", err) + } + if _, err := Write(paths, ScopeLocal, "temp", "d", "b"); err != nil { + t.Fatal(err) + } + if err := Forget(paths, ScopeLocal, "temp"); err != nil { + t.Fatalf("forget: %v", err) + } + if _, err := Read(paths, ScopeLocal, "temp"); !errors.Is(err, ErrNotFound) { + t.Errorf("a forgotten note is still readable: %v", err) + } +} + +// CRLF frontmatter must parse. Project-scope notes are checked in, and Git for +// Windows defaults to autocrlf=true, so a note that merely round-trips through a +// clone comes back with "---\r\n". Under the LF-only split the entire header, +// delimiters included, fell through into the body and the description was lost. +func TestFrontmatterParsesWithCRLFLineEndings(t *testing.T) { + paths := testPaths(t) + if err := os.MkdirAll(paths.ProjectDir, 0o700); err != nil { + t.Fatal(err) + } + content := "---\r\ndescription: what a clone gives back\r\n---\r\n\r\nthe body\r\n" + if err := os.WriteFile(filepath.Join(paths.ProjectDir, "clone-note"+fileExt), []byte(content), 0o600); err != nil { + t.Fatal(err) + } + + note, err := Read(paths, ScopeProject, "clone-note") + if err != nil { + t.Fatal(err) + } + if note.Description != "what a clone gives back" { + t.Errorf("description = %q, want it parsed from the CRLF frontmatter", note.Description) + } + if strings.Contains(note.Body, "description:") || strings.Contains(note.Body, "---") { + t.Errorf("the frontmatter leaked into the body: %q", note.Body) + } +} + +// A link at the note position must be refused on READ too, not only on write and +// delete. A note read through a link is an exfiltration primitive in a tool the +// model can call by name — the write hole with the arrow reversed. +// +// The link points INSIDE the root on purpose: os.Root already refuses one whose +// target leaves the root, so a target it permits is the case that needs this +// guard, and it is the case that was being served. +func TestReadingRefusesALinkAtTheNotePosition(t *testing.T) { + paths := testPaths(t) + if err := os.MkdirAll(paths.ProjectDir, 0o700); err != nil { + t.Fatal(err) + } + secret := filepath.Join(paths.Root, "secret.txt") + if err := os.WriteFile(secret, []byte("do not exfiltrate"), 0o600); err != nil { + t.Fatal(err) + } + // A RELATIVE target that resolves back INSIDE the root. An absolute target, + // or one leaving the root, is refused by os.Root on its own and would let + // this test pass without the guard it exists to check. "../../secret.txt" + // from .zero/memory lands on the workspace root, which os.Root permits — and + // which was therefore served. + if err := os.Symlink(filepath.Join("..", "..", "secret.txt"), filepath.Join(paths.ProjectDir, "linked"+fileExt)); err != nil { + t.Skipf("symlinks unavailable: %v", err) + } + note, err := Read(paths, ScopeProject, "linked") + if !errors.Is(err, ErrIsSymlink) { + t.Fatalf("Read(link) = %v (body %q), want ErrIsSymlink", err, note.Body) + } + if strings.Contains(note.Body, "do not exfiltrate") { + t.Error("Read served the link target's contents") + } +} + +// The same read guard, exercised on WINDOWS. The test above proves the actual +// content leak but needs os.Symlink, which an ordinary Windows account cannot +// call, so it skips on the one platform where a junction makes this reachable +// without privilege — the identical gap Vasanth found in the write test, which +// linkDir exists to close. RefuseReparse runs before the open, so a directory +// target still exercises the guard. +func TestReadingRefusesALinkAtTheNotePositionOnEveryPlatform(t *testing.T) { + paths := testPaths(t) + if err := os.MkdirAll(paths.ProjectDir, 0o700); err != nil { + t.Fatal(err) + } + target := filepath.Join(paths.Root, "target") + if err := os.MkdirAll(target, 0o700); err != nil { + t.Fatal(err) + } + linkDir(t, target, filepath.Join(paths.ProjectDir, "linked"+fileExt)) + + if _, err := Read(paths, ScopeProject, "linked"); !errors.Is(err, ErrIsSymlink) { + t.Fatalf("Read(link) = %v, want ErrIsSymlink", err) + } +} + +// The size ceiling has to hold on the way OUT as well as in. Project notes are +// checked in, so a note larger than the limit arrives by clone or by hand and +// used to be read whole — and List read every note in the store that way. +func TestReadingRefusesAnOversizedNoteWrittenOutOfBand(t *testing.T) { + paths := testPaths(t) + if err := os.MkdirAll(paths.ProjectDir, 0o700); err != nil { + t.Fatal(err) + } + oversized := strings.Repeat("x", maxNoteBytes+1) + if err := os.WriteFile(filepath.Join(paths.ProjectDir, "huge"+fileExt), []byte(oversized), 0o600); err != nil { + t.Fatal(err) + } + if _, err := Read(paths, ScopeProject, "huge"); !errors.Is(err, ErrTooLarge) { + t.Errorf("Read(oversized) = %v, want ErrTooLarge", err) + } + // And the listing reports it BESIDE the notes it could read, rather than + // dropping the oversized one in silence or throwing away the readable ones. + // The memory tool renders whatever List returns and appends the error, so + // one bad file turning this into an empty result is an empty memory. + if _, err := Write(paths, ScopeProject, "fine", "readable", "body"); err != nil { + t.Fatal(err) + } + notes, listErr := List(paths, ScopeProject) + if listErr == nil { + t.Error("List silently skipped an oversized note instead of reporting it") + } + if len(notes) != 1 || notes[0].Name != "fine" { + t.Errorf("List returned %+v; the readable note must survive an unreadable neighbour", notes) + } +} + +// A scope is resolved in ONE place. An unknown spelling must be refused rather +// than widened to every store, and a valid one must actually narrow the search. +func TestResolveScopesRefusesUnknownAndHonoursValid(t *testing.T) { + both, err := ResolveScopes("") + if err != nil || len(both) != 2 { + t.Errorf("ResolveScopes(\"\") = %v, %v; want both scopes", both, err) + } + // The VALUE, not just the count: resolving " local " to one scope is no use + // if that one scope is the project store, and a count-only assertion would + // pass either way. + for requested, want := range map[string]Scope{ + "project": ScopeProject, + "PROJECT": ScopeProject, + " local ": ScopeLocal, + "LOCAL": ScopeLocal, + } { + got, err := ResolveScopes(requested) + if err != nil || len(got) != 1 { + t.Errorf("ResolveScopes(%q) = %v, %v; want exactly one scope", requested, got, err) + continue + } + if got[0] != want { + t.Errorf("ResolveScopes(%q) = %v, want %v", requested, got[0], want) + } + } + for _, requested := range []string{"invalid", "both", "user", "../project"} { + if got, err := ResolveScopes(requested); !errors.Is(err, ErrBadScope) { + t.Errorf("ResolveScopes(%q) = %v, %v; want ErrBadScope rather than a widened search", requested, got, err) + } + } +} + +// A listing restricted to one scope must not read the other. +func TestListHonoursTheRequestedScope(t *testing.T) { + paths := testPaths(t) + if _, err := Write(paths, ScopeProject, "shared", "d", "x"); err != nil { + t.Fatal(err) + } + if _, err := Write(paths, ScopeLocal, "private", "d", "y"); err != nil { + t.Fatal(err) + } + notes, err := List(paths, ScopeProject) + if err != nil { + t.Fatal(err) + } + if len(notes) != 1 || notes[0].Name != "shared" { + t.Errorf("List(project) = %+v, want only the project note", notes) + } +} + +// The extension is matched EXACTLY. Only a comment held this in place, and the +// reason is not obvious enough to survive a refactor that makes the match +// case-insensitive to be helpful: Read reopens name+".md", so listing "note.MD" +// offers the caller an entry that the very next Read cannot open on a +// case-sensitive filesystem. Case-preserving directories mean the entry keeps +// its spelling on every platform, so this reads the same everywhere. +// +// WHAT THIS DOES AND DOES NOT CATCH, measured rather than assumed. Loosening the +// extension check ALONE does not fail this test: the case-sensitive TrimSuffix +// below then leaves "shouty.MD" whole, the name pattern refuses it, and the entry +// is dropped anyway — there is no defect there to catch. It fails on the refactor +// that does reach the listing, where the extension check and the suffix strip are +// made case-insensitive together. +func TestListMatchesTheExtensionExactly(t *testing.T) { + paths := testPaths(t) + if err := os.MkdirAll(paths.ProjectDir, 0o700); err != nil { + t.Fatal(err) + } + for _, name := range []string{"shouty.MD", "sidecar.txt", "backup.md.bak"} { + if err := os.WriteFile(filepath.Join(paths.ProjectDir, name), + []byte("---\nname: shouty\ndescription: d\n---\n\nbody\n"), 0o600); err != nil { + t.Fatal(err) + } + } + notes, err := List(paths, ScopeProject) + if err != nil { + t.Fatalf("List reported a problem for files it should have ignored: %v", err) + } + if len(notes) != 0 { + t.Errorf("List returned %+v; none of those filenames is a note this store would write", notes) + } +} + +// "local" says the note stays on this machine, and the store sits inside the +// working tree, so it has to make itself private wherever it is created rather +// than relying on the workspace having the right .gitignore line. +func TestTheLocalStoreIgnoresItself(t *testing.T) { + paths := testPaths(t) + if _, err := Write(paths, ScopeLocal, "private", "d", "y"); err != nil { + t.Fatal(err) + } + ignore, err := os.ReadFile(filepath.Join(paths.LocalDir, ".gitignore")) + if err != nil { + t.Fatalf("the local store did not make itself private: %v", err) + } + if !strings.Contains(string(ignore), "*") { + t.Errorf("local .gitignore = %q, want it to ignore everything in the directory", ignore) + } + // The project store is checked in on purpose and must NOT be ignored. + if _, err := os.Stat(filepath.Join(paths.ProjectDir, ".gitignore")); err == nil { + t.Error("the project store ignored itself; project notes are meant to be shared") + } +} + +// Line endings are normalised on every path, not just the one with frontmatter. +// The two used to differ: a note WITH a header came back LF and one WITHOUT kept +// its CRLF, which no caller asked for and nothing documented. +func TestBodyLineEndingsAreNormalisedWithAndWithoutFrontmatter(t *testing.T) { + paths := testPaths(t) + if err := os.MkdirAll(paths.ProjectDir, 0o700); err != nil { + t.Fatal(err) + } + write := func(name, content string) { + t.Helper() + if err := os.WriteFile(filepath.Join(paths.ProjectDir, name+fileExt), []byte(content), 0o600); err != nil { + t.Fatal(err) + } + } + write("headed", "---\r\ndescription: d\r\n---\r\n\r\nline one\r\nline two\r\n") + write("bare", "line one\r\nline two\r\n") + + for _, name := range []string{"headed", "bare"} { + note, err := Read(paths, ScopeProject, name) + if err != nil { + t.Fatalf("%s: %v", name, err) + } + if strings.Contains(note.Body, "\r") { + t.Errorf("%s: body kept a carriage return: %q", name, note.Body) + } + } +} + +// The listing prints every description, so the field is shared screen space: one +// note carrying a 60 KiB single-line description consumed the listing everyone +// else has to fit in. Bounded separately from the note's own size limit. +func TestADescriptionCannotConsumeTheListing(t *testing.T) { + paths := testPaths(t) + huge := strings.Repeat("verbose ", 5000) + if _, err := Write(paths, ScopeProject, "noisy", huge, "body"); err != nil { + t.Fatal(err) + } + note, err := Read(paths, ScopeProject, "noisy") + if err != nil { + t.Fatal(err) + } + if len(note.Description) > maxDescriptionBytes { + t.Errorf("description is %d bytes, want at most %d", len(note.Description), maxDescriptionBytes) + } + if note.Description == "" { + t.Error("the description was dropped entirely rather than truncated") + } + // Truncation has to be VISIBLE. A description silently cut at the cap reads + // as the author's own words ending there, and the ellipsis is the only thing + // that says otherwise. + if !strings.HasSuffix(note.Description, "…") { + t.Errorf("a truncated description carries no ellipsis: %q", note.Description) + } + // The body is untouched: only the summary is bounded. + if !strings.Contains(note.Body, "body") { + t.Errorf("the body was disturbed: %q", note.Body) + } +} + +// The cap has to be LINEAR. The first version dropped one rune per iteration and +// re-encoded the slice to measure it, taking 13.3s on a 64 KiB description — +// which maxNoteBytes permits, so it was slowest on exactly the input it exists +// to handle. +func TestTheDescriptionCapIsLinear(t *testing.T) { + for _, size := range []int{1 << 10, 16 << 10, 64 << 10} { + start := time.Now() + got := boundedDescription(strings.Repeat("verbose ", size/8)) + elapsed := time.Since(start) + if len(got) > maxDescriptionBytes { + t.Errorf("%d bytes in: result is %d bytes, over the %d cap", size, len(got), maxDescriptionBytes) + } + if elapsed > time.Second { + t.Errorf("%d bytes in: took %v — the cap is not linear", size, elapsed) + } + } + // Multi-byte runes must not be split, and must not be slow either. + multi := strings.Repeat("é", 16<<10) + start := time.Now() + got := boundedDescription(multi) + if elapsed := time.Since(start); elapsed > time.Second { + t.Errorf("multi-byte input took %v", elapsed) + } + if !utf8.ValidString(got) { + t.Errorf("the cap split a multi-byte rune: %q", got) + } + if len(got) > maxDescriptionBytes { + t.Errorf("multi-byte result is %d bytes, over the %d cap", len(got), maxDescriptionBytes) + } +} + +// A note that arrives with a CLONE never passed through this process's write +// path, so the bound has to run where the description is parsed. Otherwise a +// checked-in note crowds the listing everyone shares. +func TestACheckedInDescriptionIsBoundedOnRead(t *testing.T) { + paths := testPaths(t) + if err := os.MkdirAll(paths.ProjectDir, 0o700); err != nil { + t.Fatal(err) + } + // Written directly to disk, as a clone would deliver it. + huge := strings.Repeat("noise ", 4000) + content := "---\nname: cloned\ndescription: " + huge + "\n---\n\nbody\n" + if err := os.WriteFile(filepath.Join(paths.ProjectDir, "cloned"+fileExt), []byte(content), 0o600); err != nil { + t.Fatal(err) + } + note, err := Read(paths, ScopeProject, "cloned") + if err != nil { + t.Fatal(err) + } + if len(note.Description) > maxDescriptionBytes { + t.Errorf("a checked-in description reached the listing at %d bytes, over the %d cap", + len(note.Description), maxDescriptionBytes) + } +} diff --git a/internal/memory/physical_path_other.go b/internal/memory/physical_path_other.go new file mode 100644 index 000000000..91d180c38 --- /dev/null +++ b/internal/memory/physical_path_other.go @@ -0,0 +1,12 @@ +//go:build !windows + +package memory + +import ( + "os" + "path/filepath" +) + +func resolvePhysicalPath(_ *os.Root, _ string, path string) (string, error) { + return filepath.EvalSymlinks(path) +} diff --git a/internal/memory/physical_path_windows.go b/internal/memory/physical_path_windows.go new file mode 100644 index 000000000..842a28ef0 --- /dev/null +++ b/internal/memory/physical_path_windows.go @@ -0,0 +1,48 @@ +//go:build windows + +package memory + +import ( + "fmt" + "os" + "path/filepath" + "strings" + + "golang.org/x/sys/windows" +) + +// resolvePhysicalPath asks the opened directory handle for its final DOS path. +// filepath.EvalSymlinks cannot resolve children beneath a directory junction on +// Windows even though ordinary opens and git's working-directory discovery can. +func resolvePhysicalPath(handle *os.Root, relative, _ string) (string, error) { + dir, err := handle.Open(relative) + if err != nil { + return "", err + } + defer dir.Close() + + const maxPathUTF16 = 32768 + size := uint32(260) + for size <= maxPathUTF16 { + buffer := make([]uint16, size) + n, err := windows.GetFinalPathNameByHandle(windows.Handle(dir.Fd()), &buffer[0], size, 0) + if err != nil { + return "", err + } + if n < size { + resolved := windows.UTF16ToString(buffer[:n]) + switch { + case strings.HasPrefix(resolved, `\\?\UNC\`): + resolved = `\\` + strings.TrimPrefix(resolved, `\\?\UNC\`) + case strings.HasPrefix(resolved, `\\?\`): + resolved = strings.TrimPrefix(resolved, `\\?\`) + } + return filepath.Clean(resolved), nil + } + if n >= maxPathUTF16 { + break + } + size = n + 1 + } + return "", fmt.Errorf("resolved path exceeds the Windows path limit") +} diff --git a/internal/tools/memory.go b/internal/tools/memory.go new file mode 100644 index 000000000..bfaa27877 --- /dev/null +++ b/internal/tools/memory.go @@ -0,0 +1,303 @@ +package tools + +import ( + "context" + "errors" + "fmt" + "strings" + + "github.com/Gitlawb/zero/internal/memory" +) + +// The memory tools: read freely, write on approval. +// +// THE ASYMMETRY IS THE DESIGN. Reading a note is reading a file the user already +// has, so it needs no more ceremony than read_file. Writing one puts text into +// the user's repo that will be read back in every future session and believed — +// a note that says "this package is safe to change freely" is load-bearing the +// moment anyone trusts it. So memory_write prompts like every other write tool, +// and nothing lands silently. +// +// PLAN TASKS GET NEITHER BY DEFAULT. planReadOnlyTools is the allow-list a plan +// task's grant is validated against, and neither of these is in it — so a task +// cannot read a note (which would let a stale note steer a fan-out) or write one +// (which would let twenty tasks race to describe the same finding). That is a +// default, not a ceiling: a considered decision to grant them is a one-line +// change in that list. + +const ( + MemoryToolName = "memory" + MemoryWriteToolName = "memory_write" + MemoryForgetToolName = "memory_forget" +) + +type memoryTool struct { + baseTool + paths memory.Paths +} + +// NewMemoryTool reads durable notes. Paths with no directories configured makes +// the tool report that memory is unavailable rather than silently finding none — +// "you have no notes" and "notes are switched off here" are different answers. +func NewMemoryTool(paths memory.Paths) Tool { + return memoryTool{ + baseTool: baseTool{ + name: MemoryToolName, + description: "Read durable notes saved in earlier sessions: project conventions, decisions, and confirmed findings. " + + "Call with no arguments to list what exists (name, scope and a one-line description) and with a name to read one. " + + "Prefer listing first: the descriptions are there so you can choose what to open instead of reading everything.", + parameters: Schema{ + Type: "object", + Properties: map[string]PropertySchema{ + "name": {Type: "string", Description: "The note to read. Omit to list every note instead."}, + "scope": {Type: "string", Description: `Which store to read: "project" (shared, checked in) or "local" (this machine). Omit to search both.`}, + }, + AdditionalProperties: false, + }, + safety: readOnlySafety("Reads saved notes."), + capabilities: ToolCapabilities{Effect: EffectReadOnly, ThreadSafe: true}, + }, + paths: paths, + } +} + +func (tool memoryTool) Run(_ context.Context, args map[string]any) Result { + name, err := aliasedStringArg(args, []string{"name", "note", "key"}, "", false, true) + if err != nil { + return errorResult("Error: Invalid arguments for memory: " + err.Error()) + } + scope, err := aliasedStringArg(args, []string{"scope"}, "", false, true) + if err != nil { + return errorResult("Error: Invalid arguments for memory: " + err.Error()) + } + // Asked of the store rather than tested here, because the fields alone are + // not the answer: a blank Root makes every scope refuse with ErrNoStore, + // which List treats as "not configured" — so a Paths with both directories + // and no Root rendered as "No saved notes yet" and told the user the memory + // was empty when it was switched off. + if !tool.paths.Available() { + return errorResult("Error: memory is not available in this run.") + } + + // Resolved ONCE, and refused when the caller names a scope this package does + // not know. The two paths used to disagree: an unrecognised spelling widened + // the named read to BOTH stores, while the listing ignored a perfectly valid + // scope and always read both. + scopes, err := memory.ResolveScopes(scope) + if err != nil { + return errorResult(fmt.Sprintf("Error: %v. Use \"project\" or \"local\", or omit it to search both.", err)) + } + + if strings.TrimSpace(name) == "" { + // A LISTING IS NOT A RESOLUTION, and passing the resolved scopes through + // made it one. ResolveScopes answers "which scope wins when both hold this + // name", which must be local-first or an unscoped write cannot be read + // back; a listing shadows nothing, shows both scopes, and labels every row. + // Forwarding the resolved order meant memory.List's own listing order was + // unreachable from the product — the named variable, its comment, and the + // test that asserts it all described something no user could see. Handing + // List no scopes at all is what asks it to choose. A caller who NAMES a + // scope still gets exactly that one. Reported by @Vasanthdev2004. + listScopes := scopes + if strings.TrimSpace(scope) == "" { + listScopes = nil + } + notes, listErr := memory.List(tool.paths, listScopes...) + // BESIDE the notes, not instead of them. memory.List deliberately returns + // what it could read alongside the failures, and returning an error here + // threw that away — turning the library's careful partial success back + // into total failure, one unreadable directory entry away from an empty + // memory. A store that cannot be read still has to be reported, so it is + // appended rather than substituted. + rendered := renderMemoryList(notes) + if listErr != nil { + rendered += "\n\nSome notes could not be read: " + listErr.Error() + } + return okResult(rendered) + } + // A failure in one scope must not hide a readable note in the next. Unscoped + // reads search local first because that is where an unscoped write lands, then + // fall back to the checked-in project scope. The failure is carried and + // reported only when nothing readable turns up. + var problems []error + for _, candidate := range scopes { + note, readErr := memory.Read(tool.paths, candidate, name) + if readErr != nil { + // Absence is silent; anything else is an operational failure worth + // telling the caller about, since reporting it as "no memory named" is + // how the model concludes the note is gone and writes over it. + if !errors.Is(readErr, memory.ErrNotFound) { + problems = append(problems, fmt.Errorf("%s: %w", candidate, readErr)) + } + continue + } + return okResult(fmt.Sprintf("memory %q (%s)\n\n%s", note.Name, note.Scope, note.Body)) + } + if len(problems) > 0 { + return errorResult(fmt.Sprintf("Error: reading memory %q: %v", name, errors.Join(problems...))) + } + return errorResult(fmt.Sprintf("Error: no memory named %q. Call memory with no arguments to see what exists.", name)) +} + +type memoryWriteTool struct { + baseTool + paths memory.Paths +} + +// NewMemoryWriteTool saves a durable note. +func NewMemoryWriteTool(paths memory.Paths) Tool { + return memoryWriteTool{ + baseTool: baseTool{ + name: MemoryWriteToolName, + description: "Save a durable note for future sessions. " + + "Write what a later session could not work out for itself — a convention, a decision and its reason, a finding already confirmed. " + + "Do NOT write what the code, the tests or git history already say; a note that repeats them is one more thing to keep true. " + + `Use scope "local" by default; "project" is checked in and shared with everyone who clones the repo, so save there only what the whole team should read. ` + + "To delete a note, use " + MemoryForgetToolName + ".", + parameters: Schema{ + Type: "object", + Properties: map[string]PropertySchema{ + "name": {Type: "string", Description: `Short identifier: lowercase letters, digits and hyphen, starting with a letter (for example "error-handling").`}, + "content": {Type: "string", Description: "The note itself."}, + "description": {Type: "string", Description: "One line saying what this note is for. Shown when listing, so a reader can choose without opening it."}, + "scope": {Type: "string", Description: `"local" (this machine, the default) or "project" (checked in, shared).`}, + }, + // content is REQUIRED, and that is the fix rather than a tidy-up. + // It used to be optional with allowEmpty, so a call that merely + // left it out — or sent " \n\t" — fell through to Forget and + // destroyed the note, behind an approval whose text says only + // that this tool saves. Deletion now lives in its own tool with + // its own disclosure. + Required: []string{"name", "content"}, + AdditionalProperties: false, + }, + safety: promptSafety(SideEffectWrite, "Saves a note that future sessions will read and believe."), + capabilities: ToolCapabilities{Effect: EffectWorkspaceWrite, ThreadSafe: false}, + }, + paths: paths, + } +} + +func (tool memoryWriteTool) Run(_ context.Context, args map[string]any) Result { + name, err := aliasedStringArg(args, []string{"name", "note", "key"}, "", true, false) + if err != nil { + return errorResult("Error: Invalid arguments for memory_write: " + err.Error()) + } + // Required and non-empty: an omitted or whitespace-only payload is a + // malformed save, not a request to delete. + content, err := aliasedStringArg(args, []string{"content", "body", "text"}, "", true, false) + if err != nil { + return errorResult("Error: Invalid arguments for memory_write: " + err.Error()) + } + // Checked after trimming as well, because the argument helper only rejects a + // genuinely empty string. " \n\t" used to reach the delete branch and destroy + // the note; it must now be refused rather than saved as a blank one. + if strings.TrimSpace(content) == "" { + return errorResult("Error: memory_write needs the note's content. To delete a note, use " + MemoryForgetToolName + ".") + } + description, err := aliasedStringArg(args, []string{"description", "summary"}, "", false, true) + if err != nil { + return errorResult("Error: Invalid arguments for memory_write: " + err.Error()) + } + rawScope, err := aliasedStringArg(args, []string{"scope"}, "", false, true) + if err != nil { + return errorResult("Error: Invalid arguments for memory_write: " + err.Error()) + } + // LOCAL BY DEFAULT. A note the model chose to keep should not land in a + // shared, checked-in file unless someone said so — the same reasoning that + // makes project config unable to raise a spend ceiling. + scope := memory.ScopeLocal + if trimmed := strings.TrimSpace(strings.ToLower(rawScope)); trimmed != "" { + scope = memory.Scope(trimmed) + } + + if _, err := memory.Write(tool.paths, scope, name, description, content); err != nil { + return errorResult("Error: " + err.Error()) + } + return okResult(fmt.Sprintf("Saved %q (%s).", name, scope)) +} + +type memoryForgetTool struct { + baseTool + paths memory.Paths +} + +// NewMemoryForgetTool deletes a durable note. +// +// A SEPARATE TOOL, not a mode of memory_write, because the approval text is +// fixed per tool. memory_write's says it "saves a note that future sessions will +// read and believe", and deleting under that sentence is what made an omitted +// content field destructive: the human approved a save and got a permanent +// removal, and an "always allow" on that prompt made every later deletion +// unattended. Splitting them lets each prompt say what its tool actually does, +// and keeps a save grant from authorising a delete. +func NewMemoryForgetTool(paths memory.Paths) Tool { + return memoryForgetTool{ + baseTool: baseTool{ + name: MemoryForgetToolName, + description: "Permanently delete a saved note. " + + "There is no undo and no copy is returned, so read the note first if you are not certain it should go.", + parameters: Schema{ + Type: "object", + Properties: map[string]PropertySchema{ + "name": {Type: "string", Description: "The note to delete."}, + "scope": {Type: "string", Description: `"local" (this machine, the default) or "project" (checked in, shared).`}, + }, + Required: []string{"name"}, + AdditionalProperties: false, + }, + safety: promptSafety(SideEffectWrite, "PERMANENTLY DELETES a saved note. There is no undo."), + capabilities: ToolCapabilities{Effect: EffectWorkspaceWrite, ThreadSafe: false}, + }, + paths: paths, + } +} + +func (tool memoryForgetTool) Run(_ context.Context, args map[string]any) Result { + name, err := aliasedStringArg(args, []string{"name", "note", "key"}, "", true, false) + if err != nil { + return errorResult("Error: Invalid arguments for memory_forget: " + err.Error()) + } + rawScope, err := aliasedStringArg(args, []string{"scope"}, "", false, true) + if err != nil { + return errorResult("Error: Invalid arguments for memory_forget: " + err.Error()) + } + scope := memory.ScopeLocal + if trimmed := strings.TrimSpace(strings.ToLower(rawScope)); trimmed != "" { + scope = memory.Scope(trimmed) + } + // Absence is reported as absence. memory.Forget is idempotent by design — a + // missing note is not an error at the store layer — but saying "Forgot" for a + // note that never existed tells a model which misspelled the name that the + // deletion happened, and it stops looking for the real one. + if _, readErr := memory.Read(tool.paths, scope, name); readErr != nil { + if errors.Is(readErr, memory.ErrNotFound) { + return okResult(fmt.Sprintf("No note named %q in %s, so there was nothing to forget.", name, scope)) + } + // Read-before-delete is a safety gate, not only an existence probe. If the + // note cannot be read, deleting it would destroy the only copy without the + // caller ever being able to inspect what the destructive tool removed. + return errorResult(fmt.Sprintf("Error: cannot read memory %q in %s before deleting it: %v", name, scope, readErr)) + } + if err := memory.Forget(tool.paths, scope, name); err != nil { + return errorResult("Error: " + err.Error()) + } + return okResult(fmt.Sprintf("Forgot %q (%s).", name, scope)) +} + +func renderMemoryList(notes []memory.Note) string { + if len(notes) == 0 { + return "No saved notes yet. Use memory_write to keep something a future session could not work out for itself." + } + var b strings.Builder + fmt.Fprintf(&b, "%d saved note(s):\n", len(notes)) + for _, note := range notes { + fmt.Fprintf(&b, "- %s (%s)", note.Name, note.Scope) + if note.Description != "" { + b.WriteString(" — ") + b.WriteString(note.Description) + } + b.WriteString("\n") + } + return strings.TrimRight(b.String(), "\n") +} diff --git a/internal/tools/memory_test.go b/internal/tools/memory_test.go new file mode 100644 index 000000000..d0f584a9d --- /dev/null +++ b/internal/tools/memory_test.go @@ -0,0 +1,306 @@ +package tools + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/Gitlawb/zero/internal/memory" +) + +func memoryTestPaths(t *testing.T) memory.Paths { + t.Helper() + return memory.DefaultPaths(t.TempDir()) +} + +// THE DELETE BLOCKER. An omitted or whitespace-only content used to fall through +// to Forget, so a call that merely left the field out destroyed the note — +// behind an approval whose text says only that this tool SAVES. The save tool +// must now refuse such a call rather than treat it as a deletion. +func TestMemoryWriteRefusesToDeleteThroughAnEmptyContent(t *testing.T) { + paths := memoryTestPaths(t) + if _, err := memory.Write(paths, memory.ScopeLocal, "precious", "d", "keep me"); err != nil { + t.Fatal(err) + } + write := NewMemoryWriteTool(paths) + + for _, args := range []map[string]any{ + {"name": "precious"}, // content omitted entirely + {"name": "precious", "content": ""}, // empty + {"name": "precious", "content": " \n\t"}, // whitespace only + } { + result := write.Run(context.Background(), args) + if result.Status != StatusError { + t.Errorf("memory_write(%v) succeeded; an empty payload must not be a deletion: %q", args, result.Output) + } + if strings.Contains(strings.ToLower(result.Output), "forgot") { + t.Errorf("memory_write(%v) deleted the note: %q", args, result.Output) + } + } + // And the note is still there. + if _, err := memory.Read(paths, memory.ScopeLocal, "precious"); err != nil { + t.Fatalf("the note was destroyed by a save-shaped call: %v", err) + } +} + +// Deletion lives in its own tool so its approval text can say so. The safety +// disclosure is the point: a grant on "saves a note" must not authorise removal. +func TestMemoryForgetIsASeparateToolThatDisclosesDeletion(t *testing.T) { + paths := memoryTestPaths(t) + if _, err := memory.Write(paths, memory.ScopeLocal, "doomed", "d", "bye"); err != nil { + t.Fatal(err) + } + forget := NewMemoryForgetTool(paths) + if forget.Name() == NewMemoryWriteTool(paths).Name() { + t.Fatal("forget and write share a name, so they would share one approval") + } + disclosure := strings.ToLower(forget.Safety().Reason) + if !strings.Contains(disclosure, "delet") { + t.Errorf("the forget approval does not disclose deletion: %q", forget.Safety().Reason) + } + if saved := strings.ToLower(NewMemoryWriteTool(paths).Safety().Reason); strings.Contains(saved, "delet") { + t.Errorf("the save approval mentions deletion, which it no longer performs: %q", saved) + } + + result := forget.Run(context.Background(), map[string]any{"name": "doomed"}) + if result.Status == StatusError { + t.Fatalf("memory_forget failed: %q", result.Output) + } + if _, err := memory.Read(paths, memory.ScopeLocal, "doomed"); err == nil { + t.Error("memory_forget reported success but the note is still readable") + } +} + +// An unrecognised scope must be refused, not silently widened to every store. +func TestMemoryReadRefusesAnUnknownScope(t *testing.T) { + paths := memoryTestPaths(t) + if _, err := memory.Write(paths, memory.ScopeProject, "shared", "d", "x"); err != nil { + t.Fatal(err) + } + read := NewMemoryTool(paths) + result := read.Run(context.Background(), map[string]any{"name": "shared", "scope": "invalid"}) + if result.Status != StatusError { + t.Errorf("an unknown scope was accepted and the search widened: %q", result.Output) + } +} + +// A valid scope must actually narrow the listing; it used to be ignored. +func TestMemoryListHonoursTheRequestedScope(t *testing.T) { + paths := memoryTestPaths(t) + if _, err := memory.Write(paths, memory.ScopeProject, "shared", "team", "x"); err != nil { + t.Fatal(err) + } + if _, err := memory.Write(paths, memory.ScopeLocal, "private", "mine", "y"); err != nil { + t.Fatal(err) + } + result := NewMemoryTool(paths).Run(context.Background(), map[string]any{"scope": "project"}) + if result.Status == StatusError { + t.Fatalf("listing failed: %q", result.Output) + } + if strings.Contains(result.Output, "private") { + t.Errorf("a project-scoped listing included a local note: %q", result.Output) + } + if !strings.Contains(result.Output, "shared") { + t.Errorf("a project-scoped listing dropped the project note: %q", result.Output) + } +} + +// A store with directories but no containment Root is UNAVAILABLE, not empty. +// Every scope refuses with ErrNoStore, which List treats as "not configured", so +// the listing came back empty and told the user they had no notes when the store +// was switched off — the same absence-versus-unavailable confusion these tools +// exist to keep apart. +func TestMemoryReportsUnavailableWhenTheRootIsMissing(t *testing.T) { + base := t.TempDir() + paths := memory.Paths{ + ProjectDir: base + "/.zero/memory", + LocalDir: base + "/.zero/memory/local", + // Root deliberately blank. + } + result := NewMemoryTool(paths).Run(context.Background(), map[string]any{}) + if result.Status != StatusError { + t.Fatalf("a store with no Root reported success: %q", result.Output) + } + if strings.Contains(strings.ToLower(result.Output), "no saved notes") { + t.Errorf("a switched-off store was rendered as an empty one: %q", result.Output) + } + if !strings.Contains(strings.ToLower(result.Output), "not available") { + t.Errorf("the reason was not reported: %q", result.Output) + } +} + +// Forgetting a note that was never there must SAY so. memory.Forget is +// idempotent at the store layer, so the tool used to answer "Forgot" for a name +// that never existed — and a model that misspelled the name took that as +// confirmation and stopped looking for the real note. +func TestMemoryForgetReportsAbsenceRatherThanClaimingSuccess(t *testing.T) { + paths := memoryTestPaths(t) + if _, err := memory.Write(paths, memory.ScopeLocal, "real-note", "d", "body"); err != nil { + t.Fatal(err) + } + result := NewMemoryForgetTool(paths).Run(context.Background(), map[string]any{"name": "reel-note"}) + if result.Status == StatusError { + t.Fatalf("a missing note should not be an error: %q", result.Output) + } + if strings.Contains(result.Output, "Forgot") { + t.Errorf("memory_forget claimed it deleted a note that never existed: %q", result.Output) + } + if !strings.Contains(strings.ToLower(result.Output), "nothing to forget") { + t.Errorf("absence was not reported distinctly: %q", result.Output) + } + // The real note is untouched. + if _, err := memory.Read(paths, memory.ScopeLocal, "real-note"); err != nil { + t.Errorf("the existing note was disturbed: %v", err) + } +} + +func TestMemoryForgetPreservesANoteThatCannotBeRead(t *testing.T) { + paths := memoryTestPaths(t) + if err := os.MkdirAll(paths.LocalDir, 0o700); err != nil { + t.Fatal(err) + } + notePath := filepath.Join(paths.LocalDir, "unreadable.md") + // Oversize input is a deterministic, cross-platform read failure; unlike + // permission bits, it behaves the same when tests run with elevated access. + if err := os.WriteFile(notePath, []byte(strings.Repeat("x", 70<<10)), 0o600); err != nil { + t.Fatal(err) + } + if _, err := memory.Read(paths, memory.ScopeLocal, "unreadable"); err == nil { + t.Fatal("oversized fixture unexpectedly remained readable") + } + + result := NewMemoryForgetTool(paths).Run(context.Background(), map[string]any{"name": "unreadable"}) + if result.Status != StatusError { + t.Fatalf("memory_forget deleted an unreadable note: %q", result.Output) + } + if !strings.Contains(strings.ToLower(result.Output), "cannot read") { + t.Fatalf("memory_forget hid the read failure: %q", result.Output) + } + if _, err := os.Stat(notePath); err != nil { + t.Fatalf("memory_forget removed the unreadable note: %v", err) + } +} + +// One unreadable note must not empty the listing. memory.List deliberately +// returns what it could read alongside the failures; the tool returning an error +// instead threw that away, turning partial success back into total failure one +// bad directory entry away from an empty memory. +func TestAnUnreadableNoteDoesNotEmptyTheListing(t *testing.T) { + paths := memoryTestPaths(t) + if _, err := memory.Write(paths, memory.ScopeProject, "readable", "still here", "body"); err != nil { + t.Fatal(err) + } + // A note too large to read back: the ceiling holds on the way out. + oversized := strings.Repeat("x", 70<<10) + if err := os.WriteFile(filepath.Join(paths.ProjectDir, "huge.md"), []byte(oversized), 0o600); err != nil { + t.Fatal(err) + } + + result := NewMemoryTool(paths).Run(context.Background(), map[string]any{}) + if !strings.Contains(result.Output, "readable") { + t.Errorf("the readable note was dropped because another failed: %q", result.Output) + } + if !strings.Contains(strings.ToLower(result.Output), "could not be read") { + t.Errorf("the failure was not reported alongside the notes: %q", result.Output) + } +} + +// A failure in the local scope must not hide a readable project note. Unscoped +// reads resolve local first, then continue to the checked-in project scope when +// the local entry cannot be read. +func TestALocalFailureDoesNotHideTheProjectNote(t *testing.T) { + paths := memoryTestPaths(t) + if _, err := memory.Write(paths, memory.ScopeProject, "findings", "team", "the project body"); err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(paths.LocalDir, 0o700); err != nil { + t.Fatal(err) + } + // An unreadable local note of the SAME name, searched first. + oversized := strings.Repeat("x", 70<<10) + if err := os.WriteFile(filepath.Join(paths.LocalDir, "findings.md"), []byte(oversized), 0o600); err != nil { + t.Fatal(err) + } + + result := NewMemoryTool(paths).Run(context.Background(), map[string]any{"name": "findings"}) + if result.Status == StatusError { + t.Fatalf("a local-scope failure hid the readable project note: %q", result.Output) + } + if !strings.Contains(result.Output, "the project body") { + t.Errorf("the project note was not returned: %q", result.Output) + } +} + +// A DELETE MUST REFUSE THE NAME THE OTHER TWO DOORS REFUSE. memory_read and +// memory_write both decline a note whose stored spelling differs from the one +// asked for, because on a case-insensitive filesystem they are one file. Forget +// did not, so the operation with nothing recoverable afterwards was the only one +// that ignored the guard — and the refusal the other two return used to say +// "rename or remove it first", which named this tool as the way out. +func TestMemoryForgetRefusesADifferentlySpelledNote(t *testing.T) { + paths := memoryTestPaths(t) + if err := os.MkdirAll(paths.ProjectDir, 0o755); err != nil { + t.Fatal(err) + } + hand := filepath.Join(paths.ProjectDir, "findings.MD") + if err := os.WriteFile(hand, []byte("---\nname: findings\n---\n\nweeks of work\n"), 0o644); err != nil { + t.Fatal(err) + } + + result := NewMemoryForgetTool(paths).Run(context.Background(), map[string]any{"name": "findings", "scope": "project"}) + if result.Status != StatusError { + t.Errorf("forget accepted a clashing name and reported %q", result.Output) + } + if !strings.Contains(result.Output, "findings.MD") { + t.Errorf("the refusal did not name the occupant it declined to delete: %q", result.Output) + } + if strings.Contains(result.Output, MemoryForgetToolName) { + t.Errorf("the refusal pointed back at the deleting tool: %q", result.Output) + } + // AND IT MUST NOT ADVISE REMOVAL. The wording is the other half of this bug: + // all three doors used to answer a clash with "rename or remove it first", + // and removal is the operation that destroys the occupant being described. + // A refusal that routes the reader to the sharpest door is not a refusal. + if strings.Contains(strings.ToLower(result.Output), "remove it") { + t.Errorf("the refusal advised removing the occupant it just protected: %q", result.Output) + } + if !strings.Contains(strings.ToLower(result.Output), "rename") { + t.Errorf("the refusal named no way forward: %q", result.Output) + } + body, err := os.ReadFile(hand) + if err != nil { + t.Fatalf("the hand-authored note was deleted by a refused forget: %v", err) + } + if !strings.Contains(string(body), "weeks of work") { + t.Errorf("the note survived but its contents did not: %q", body) + } +} + +// THE LISTING ORDER HAS TO REACH A USER TO BE AN ORDER. memory.List documents a +// project-first listing and a test asserts it, but the tool resolved scopes +// first and handed the resolution order (local-first) straight through, so the +// documented order applied to nothing anybody could see. +func TestAnUnscopedListingIsProjectFirst(t *testing.T) { + paths := memoryTestPaths(t) + write := NewMemoryWriteTool(paths) + if r := write.Run(context.Background(), map[string]any{"name": "zulu", "scope": "local", "content": "a local note"}); r.Status == StatusError { + t.Fatal(r.Output) + } + if r := write.Run(context.Background(), map[string]any{"name": "alpha", "scope": "project", "content": "a project note"}); r.Status == StatusError { + t.Fatal(r.Output) + } + + result := NewMemoryTool(paths).Run(context.Background(), map[string]any{}) + if result.Status == StatusError { + t.Fatal(result.Output) + } + project, local := strings.Index(result.Output, "alpha (project)"), strings.Index(result.Output, "zulu (local)") + if project < 0 || local < 0 { + t.Fatalf("the unscoped listing lost a scope: %q", result.Output) + } + if project > local { + t.Errorf("the listing put local before project, which is the resolution order, not the listing order:\n%s", result.Output) + } +}