From 0a9ea831fd2e4c776dbd4e3648a59be0652583d3 Mon Sep 17 00:00:00 2001 From: KRATOS <84986124+gnanam1990@users.noreply.github.com> Date: Thu, 13 Aug 2026 00:20:00 +0530 Subject: [PATCH 01/20] feat(memory): durable note store and memory tools MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Split 2/3 of #829, stacked on the pathjail primitive from #891 (1/3). - internal/memory: a durable note store confined to a pathjail handle, so every read, write, rename and delete stays on the confined handle rather than being re-resolved by pathname. Writes publish through an O_EXCL temporary file. - internal/tools: memory (read/list), memory_write (save), memory_forget (delete). Reads are confined by the same rule as writes, the size ceiling holds on the way out as well as in, scope is resolved in one place, and deletion is its own tool so its approval can disclose it — memory_write's says only that it saves, and an omitted or blank content used to fall through to Forget under that sentence. PARTIAL SUCCESS IS PRESERVED END TO END. The earlier fix for "errors reported as absence" overshot in two places, and both are corrected here: - The listing returned an error INSTEAD of the notes whenever any note failed, so the library's careful partial success became total failure one unreadable directory entry away from an empty memory. The failure is now appended to the rendered notes rather than substituted for them. - The named read returned on the first non-ErrNotFound failure, and project is searched before local. So one unreadable project note called "findings" made the user's own local "findings" unreachable — the shared, externally-supplied scope masking the private one. Failures are carried and reported only when nothing readable turns up. A description is bounded separately from the note. The listing prints every description, so the field is shared screen space: the total-size check alone let one note carry a 60 KiB single-line description and consume the listing everyone else has to fit in. Origin-Session: local-de382f | Claude Code | 9 prompts Origin-Snapshot: 92ae33c95cd1 Origin-Session: local-13d543 | Claude Code | 5 prompts Origin-Snapshot: 6b5eb8ba4e5b Origin-Session: local-c962d7 | Claude Code | 5 prompts Origin-Snapshot: 3bb420a0a97b --- internal/memory/escape_test.go | 113 +++++++ internal/memory/memory.go | 565 +++++++++++++++++++++++++++++++++ internal/memory/memory_test.go | 414 ++++++++++++++++++++++++ internal/tools/memory.go | 286 +++++++++++++++++ internal/tools/memory_test.go | 207 ++++++++++++ 5 files changed, 1585 insertions(+) create mode 100644 internal/memory/escape_test.go create mode 100644 internal/memory/memory.go create mode 100644 internal/memory/memory_test.go create mode 100644 internal/tools/memory.go create mode 100644 internal/tools/memory_test.go diff --git a/internal/memory/escape_test.go b/internal/memory/escape_test.go new file mode 100644 index 000000000..55d68c312 --- /dev/null +++ b/internal/memory/escape_test.go @@ -0,0 +1,113 @@ +package memory + +import ( + "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") + } +} diff --git a/internal/memory/memory.go b/internal/memory/memory.go new file mode 100644 index 000000000..d17d59d4b --- /dev/null +++ b/internal/memory/memory.go @@ -0,0 +1,565 @@ +// 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 ( + "errors" + "fmt" + "io" + "os" + "path/filepath" + "regexp" + "sort" + "strings" + + "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" + +// 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"`) + // 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 + } + return pathjail.Open(paths.Root, dir) +} + +// 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] +} + +// 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 []Scope{ScopeProject, ScopeLocal}, 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 { + scopes = []Scope{ScopeProject, ScopeLocal} + } + 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 + } + 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 drops a self-ignoring .gitignore into the local store +// the first time one is written. +// +// "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. +// +// Best effort by design: a store that cannot hold an ignore file is still a +// working store, and failing the write would trade a privacy improvement for an +// outage. +// NO ERROR RETURN, because there is no failure here a caller should act on: the +// store works whether or not the ignore file exists, and failing a note's write +// over it would trade a privacy improvement for an outage. A signature that +// cannot fail says that plainly, rather than asking every caller to check a +// value that is always nil. +func keepLocalScopePrivate(handle *os.Root, scope Scope, relative string) { + if scope != ScopeLocal { + return + } + // O_EXCL alone decides whether this is the first write: an existing file + // fails the create, which is the same answer a prior Stat would have given + // and one syscall rather than two. + file, err := handle.OpenFile(filepath.Join(relative, ".gitignore"), os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0o600) + if err != nil { + return + } + defer file.Close() + _, _ = file.WriteString("# Notes saved to the local scope stay on this machine.\n*\n") +} + +// 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) + } + keepLocalScopePrivate(handle, scope, relative) + path := filepath.Join(dir, name+fileExt) + relativePath := filepath.Join(relative, name+fileExt) + if err := pathjail.RefuseReparse(handle, relativePath); err != nil { + return "", err + } + 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 + } + if err := handle.Remove(relativePath); err != nil && !os.IsNotExist(err) { + return err + } + return nil +} + +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 = "…" + runes := []rune(text) + // Reserve the ellipsis's own width — it is three bytes in UTF-8, not one, so + // budgeting a single byte for it overshot the cap it exists to enforce. + for len(runes) > 0 && len(string(runes))+len(ellipsis) > maxDescriptionBytes { + runes = runes[:len(runes)-1] + } + return string(runes) + 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 { + description = 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..597f7904e --- /dev/null +++ b/internal/memory/memory_test.go @@ -0,0 +1,414 @@ +package memory + +import ( + "errors" + "os" + "path/filepath" + "slices" + "strings" + "testing" +) + +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 rather than dropping the note in silence. + if _, listErr := List(paths, ScopeProject); listErr == nil { + t.Error("List silently skipped an oversized note instead of reporting it") + } +} + +// 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) + } + for _, requested := range []string{"project", "PROJECT", " local "} { + got, err := ResolveScopes(requested) + if err != nil || len(got) != 1 { + t.Errorf("ResolveScopes(%q) = %v, %v; want exactly one scope", requested, got, err) + } + } + 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) + } +} + +// "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") + } + // The body is untouched: only the summary is bounded. + if !strings.Contains(note.Body, "body") { + t.Errorf("the body was disturbed: %q", note.Body) + } +} diff --git a/internal/tools/memory.go b/internal/tools/memory.go new file mode 100644 index 000000000..74f71bd1b --- /dev/null +++ b/internal/tools/memory.go @@ -0,0 +1,286 @@ +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) == "" { + notes, listErr := memory.List(tool.paths, scopes...) + // 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. Project is + // searched first, is checked in, and arrives with a clone; local is the user's + // own default write scope. An unreadable project note called "findings" would + // otherwise make the user's own local "findings" unreachable — the shared, + // externally-supplied scope masking the private one. 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 _, err := memory.Read(tool.paths, scope, name); errors.Is(err, memory.ErrNotFound) { + return okResult(fmt.Sprintf("No note named %q in %s, so there was nothing to forget.", name, scope)) + } + 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..759653030 --- /dev/null +++ b/internal/tools/memory_test.go @@ -0,0 +1,207 @@ +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) + } +} + +// 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 project scope must not hide the user's own local note. +// Project is searched first and arrives with a clone; local is the default write +// scope, so the shared scope masking the private one is the wrong way round. +func TestAProjectFailureDoesNotHideTheLocalNote(t *testing.T) { + paths := memoryTestPaths(t) + if _, err := memory.Write(paths, memory.ScopeLocal, "findings", "mine", "the local body"); err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(paths.ProjectDir, 0o700); err != nil { + t.Fatal(err) + } + // An unreadable project note of the SAME name, searched first. + oversized := strings.Repeat("x", 70<<10) + if err := os.WriteFile(filepath.Join(paths.ProjectDir, "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 project-scope failure hid the readable local note: %q", result.Output) + } + if !strings.Contains(result.Output, "the local body") { + t.Errorf("the local note was not returned: %q", result.Output) + } +} From 5e19fc696296a14ff67e71a5a30523f17f33fc90 Mon Sep 17 00:00:00 2001 From: KRATOS <84986124+gnanam1990@users.noreply.github.com> Date: Sun, 16 Aug 2026 14:47:49 +0530 Subject: [PATCH 02/20] fix(memory): the description cap is linear, and it runs where the description is read MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both raised by @Vasanthdev2004 against the cap added in the previous commit, and they compound: the read path is where an unbounded description shows up, and the read path was not where the bound ran. THE CAP WAS QUADRATIC. It dropped one rune per iteration and re-encoded the whole slice each time to measure it: 1 KiB -> 1.6ms 16 KiB -> 381ms 64 KiB -> 13.3s maxNoteBytes is 64 KiB, so a 64 KiB description is a note the write path accepts — the cap was at its slowest on exactly the input it exists to handle, and multi-byte runes were worse per byte. Ranging over the string yields rune boundaries with their byte offsets, so the cut point is now found in one pass with no intermediate string. THE CAP ONLY RAN ON WRITE. A project-scope note arrives with a clone and never passes through this process's write path, so a checked-in note with an enormous single-line description was unbounded in the listing. The bound now runs where the description is PARSED, which covers both paths. Both regressions fail against the old behaviour: the quadratic loop takes ~5s on 64 KiB, and bounding only on write lets a 23,999-byte checked-in description reach the listing. Origin-Session: local-abff1c | Claude Code | 1 prompt Origin-Snapshot: 0e7ed28981cb Origin-Session: local-13d543 | Claude Code | 5 prompts Origin-Snapshot: 6b5eb8ba4e5b Origin-Session: local-c962d7 | Claude Code | 5 prompts Origin-Snapshot: 3bb420a0a97b --- internal/memory/memory.go | 30 +++++++++++++----- internal/memory/memory_test.go | 57 ++++++++++++++++++++++++++++++++++ 2 files changed, 80 insertions(+), 7 deletions(-) diff --git a/internal/memory/memory.go b/internal/memory/memory.go index d17d59d4b..3fe316a80 100644 --- a/internal/memory/memory.go +++ b/internal/memory/memory.go @@ -533,13 +533,25 @@ func boundedDescription(text string) string { return text } const ellipsis = "…" - runes := []rune(text) - // Reserve the ellipsis's own width — it is three bytes in UTF-8, not one, so - // budgeting a single byte for it overshot the cap it exists to enforce. - for len(runes) > 0 && len(string(runes))+len(ellipsis) > maxDescriptionBytes { - runes = runes[:len(runes)-1] + // 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 string(runes) + ellipsis + return text[:cut] + ellipsis } func splitFrontmatter(content string) (description string, body string) { @@ -554,7 +566,11 @@ func splitFrontmatter(content string) (description string, body string) { } for _, line := range strings.Split(rest[:end], "\n") { if value, ok := strings.CutPrefix(strings.TrimSpace(line), "description:"); ok { - description = strings.TrimSpace(value) + // 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") diff --git a/internal/memory/memory_test.go b/internal/memory/memory_test.go index 597f7904e..0243018a1 100644 --- a/internal/memory/memory_test.go +++ b/internal/memory/memory_test.go @@ -7,6 +7,8 @@ import ( "slices" "strings" "testing" + "time" + "unicode/utf8" ) func testPaths(t *testing.T) Paths { @@ -412,3 +414,58 @@ func TestADescriptionCannotConsumeTheListing(t *testing.T) { 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) + } +} From dba6d5712844b9dc1d409bb1298c0e6908afce7e Mon Sep 17 00:00:00 2001 From: KRATOS <84986124+gnanam1990@users.noreply.github.com> Date: Sun, 16 Aug 2026 15:05:29 +0530 Subject: [PATCH 03/20] test(memory): pin the extension rule, the resolved scope, and partial listing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four gaps CodeRabbit found in the tests, each covering behaviour that only a comment held in place: - a truncated description now has to END WITH the ellipsis, not merely fit the cap: a silent cut reads as the author's own words stopping there - ResolveScopes is asserted on the VALUE it resolves to, not just the count — " local " returning one scope proves nothing if that scope is the project store - the exact ".md" match, which decides that List and Read agree about what a note is - List returning readable notes BESIDE the error, which is the contract the memory tool depends on to avoid rendering one bad file as an empty memory Both of the last two were mutation-checked, and the extension test only half holds: loosening the extension check alone does NOT fail it, because the case-sensitive TrimSuffix then leaves "shouty.MD" whole and the name pattern refuses it, so nothing reaches the listing to catch. It fails on the refactor that does reach it — extension check and suffix strip made case-insensitive together. That limit is written into the test rather than left for the next reader to discover. Origin-Session: local-abff1c | Claude Code | 1 prompt Origin-Snapshot: 0e7ed28981cb Origin-Session: local-13d543 | Claude Code | 5 prompts Origin-Snapshot: 6b5eb8ba4e5b Origin-Session: local-c962d7 | Claude Code | 5 prompts Origin-Snapshot: 3bb420a0a97b --- internal/memory/memory_test.go | 67 ++++++++++++++++++++++++++++++++-- 1 file changed, 64 insertions(+), 3 deletions(-) diff --git a/internal/memory/memory_test.go b/internal/memory/memory_test.go index 0243018a1..63cff3e7f 100644 --- a/internal/memory/memory_test.go +++ b/internal/memory/memory_test.go @@ -297,10 +297,20 @@ func TestReadingRefusesAnOversizedNoteWrittenOutOfBand(t *testing.T) { if _, err := Read(paths, ScopeProject, "huge"); !errors.Is(err, ErrTooLarge) { t.Errorf("Read(oversized) = %v, want ErrTooLarge", err) } - // And the listing reports it rather than dropping the note in silence. - if _, listErr := List(paths, ScopeProject); listErr == nil { + // 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 @@ -310,10 +320,22 @@ func TestResolveScopesRefusesUnknownAndHonoursValid(t *testing.T) { if err != nil || len(both) != 2 { t.Errorf("ResolveScopes(\"\") = %v, %v; want both scopes", both, err) } - for _, requested := range []string{"project", "PROJECT", " local "} { + // 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"} { @@ -341,6 +363,39 @@ func TestListHonoursTheRequestedScope(t *testing.T) { } } +// 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. @@ -409,6 +464,12 @@ func TestADescriptionCannotConsumeTheListing(t *testing.T) { 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) From 7e24afb7855361fdb62352852afbc5a42b00a7f7 Mon Sep 17 00:00:00 2001 From: KRATOS <84986124+gnanam1990@users.noreply.github.com> Date: Sun, 16 Aug 2026 16:18:06 +0530 Subject: [PATCH 04/20] fix(memory): refuse a link at any store ancestor, and fail closed on privacy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both P1s from @anandh8x, both reproduced exactly as he described before any change was made. A LINK ABOVE THE STORE REDIRECTED EVERYTHING BELOW IT. The comment on openScope claimed this class was closed by routing every operation through os.Root, and it is only half closed: os.Root refuses to traverse OUT of the workspace but follows a link that resolves back INSIDE it, and the store path is checked in. A repository shipping ".zero -> redirected" served notes from a directory nobody asked for — Read returned the planted note — while every individual check passed, because the only component ever inspected was the note file at the end. Every component of the store path is now checked, outermost first, composed from pathjail.RefuseReparse so the Windows-junction and trailing-separator handling stays in one place rather than being restated here. THE PRIVACY PROMISE FAILED OPEN. keepLocalScopePrivate read every failure of its O_EXCL create as "a previous run wrote the ignore", and O_EXCL cannot tell that from "something else got there first": precreating an empty .gitignore made every later local write succeed with the store fully tracked. The old comment argued best-effort was right because failing a write would trade privacy for an outage — that reasoning does not hold for this function, because the promise IS the feature. A note the user was told stays on this machine, sitting in git status, is worse than a refused write: the refusal is visible and the leak is not. The ignore is now installed or verified, and a store that cannot be made private returns ErrNotPrivate naming the file. The effectiveness check recognises a bare "*" — what this store writes — and nothing narrower, because guessing at the effect of an arbitrary pattern set is how such a check ends up agreeing with a file that protects nothing. A single re-inclusion line fails the file wherever it sits. Both mutation-checked: removing the ancestor chain lets the redirected read through again, and ignoring the verify result lets an empty ignore pass. The ancestor test derives its components from Paths rather than naming them, so it keeps covering every ancestor if the layout moves — and writing them by hand is how the first version of it tested a path that was not a store ancestor at all. Origin-Session: local-abff1c | Claude Code | 3 prompts Origin-Snapshot: 07900397c62e Origin-Session: local-13d543 | Claude Code | 5 prompts Origin-Snapshot: 6b5eb8ba4e5b Origin-Session: local-c962d7 | Claude Code | 5 prompts Origin-Snapshot: 3bb420a0a97b --- internal/memory/escape_test.go | 128 ++++++++++++++++++++++++++++++++ internal/memory/memory.go | 131 +++++++++++++++++++++++++++------ 2 files changed, 238 insertions(+), 21 deletions(-) diff --git a/internal/memory/escape_test.go b/internal/memory/escape_test.go index 55d68c312..3717fcc91 100644 --- a/internal/memory/escape_test.go +++ b/internal/memory/escape_test.go @@ -1,6 +1,7 @@ package memory import ( + "errors" "os" "os/exec" "path/filepath" @@ -111,3 +112,130 @@ func TestAnOrdinaryWorkspaceStoreStillRoundTrips(t *testing.T) { 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) + } + if err := os.Symlink(target, linkPath); err != nil { + t.Skipf("cannot create a symlink here: %v", err) + } + 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) + } +} diff --git a/internal/memory/memory.go b/internal/memory/memory.go index 3fe316a80..d08eebfc7 100644 --- a/internal/memory/memory.go +++ b/internal/memory/memory.go @@ -23,6 +23,7 @@ import ( "errors" "fmt" "io" + "io/fs" "os" "path/filepath" "regexp" @@ -48,6 +49,11 @@ const ( // 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" + // 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" @@ -102,6 +108,11 @@ var ( 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"`) + // 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. + ErrNotPrivate = errors.New("the local memory store is not ignored by git, so a note saved there 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. @@ -187,7 +198,44 @@ func (paths Paths) openScope(scope Scope) (*os.Root, string, error) { if strings.TrimSpace(paths.Root) == "" { return nil, "", ErrNoStore } - return pathjail.Open(paths.Root, dir) + 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, relative); err != nil { + handle.Close() + return nil, "", err + } + return handle, relative, 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, 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 { + if err := pathjail.RefuseReparse(handle, filepath.Join(parts[:i+1]...)); err != nil { + return err + } + } + return nil } // Note is one stored memory. @@ -355,8 +403,8 @@ func Read(paths Paths, scope Scope, name string) (Note, error) { return Note{Name: name, Description: description, Scope: scope, Body: text}, nil } -// keepLocalScopePrivate drops a self-ignoring .gitignore into the local store -// the first time one is written. +// keepLocalScopePrivate installs, or verifies, the ignore that keeps local notes +// out of the repository. // // "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 @@ -369,27 +417,66 @@ func Read(paths Paths, scope Scope, name string) (Note, error) { // that makes itself private travels with every one. "*" covers the notes and the // ignore file itself, so the directory contributes nothing to the index. // -// Best effort by design: a store that cannot hold an ignore file is still a -// working store, and failing the write would trade a privacy improvement for an -// outage. -// NO ERROR RETURN, because there is no failure here a caller should act on: the -// store works whether or not the ignore file exists, and failing a note's write -// over it would trade a privacy improvement for an outage. A signature that -// cannot fail says that plainly, rather than asking every caller to check a -// value that is always nil. -func keepLocalScopePrivate(handle *os.Root, scope Scope, relative string) { +// 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 string) error { if scope != ScopeLocal { - return + return nil + } + 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: + defer file.Close() + if _, writeErr := file.WriteString(localIgnoreContent); writeErr != nil { + return fmt.Errorf("write %s: %w", ignorePath, writeErr) + } + return nil + case !errors.Is(err, fs.ErrExist): + return fmt.Errorf("create %s: %w", ignorePath, err) } - // O_EXCL alone decides whether this is the first write: an existing file - // fails the create, which is the same answer a prior Stat would have given - // and one syscall rather than two. - file, err := handle.OpenFile(filepath.Join(relative, ".gitignore"), os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0o600) + existing, err := readBounded(handle, ignorePath) if err != nil { - return + return fmt.Errorf("read %s: %w", ignorePath, err) } - defer file.Close() - _, _ = file.WriteString("# Notes saved to the local scope stay on this machine.\n*\n") + if !ignoresEverything(string(existing)) { + return fmt.Errorf("%w: %s", ErrNotPrivate, ignorePath) + } + return nil +} + +// 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. +func ignoresEverything(content string) bool { + covered := false + for _, line := range strings.Split(content, "\n") { + line = strings.TrimSpace(strings.TrimSuffix(line, "\r")) + if line == "" || strings.HasPrefix(line, "#") { + continue + } + if strings.HasPrefix(line, "!") { + return false + } + if line == "*" { + covered = true + } + } + return covered } // readBounded reads at most maxNoteBytes, refusing anything larger rather than @@ -444,7 +531,9 @@ func Write(paths Paths, scope Scope, name, description, body string) (string, er if err := handle.MkdirAll(relative, 0o700); err != nil { return "", fmt.Errorf("create %s: %w", dir, err) } - keepLocalScopePrivate(handle, scope, relative) + if err := keepLocalScopePrivate(handle, scope, relative); err != nil { + return "", err + } path := filepath.Join(dir, name+fileExt) relativePath := filepath.Join(relative, name+fileExt) if err := pathjail.RefuseReparse(handle, relativePath); err != nil { From 5921f1199a416bf3aca1294e719d1f3e3bd85d96 Mon Sep 17 00:00:00 2001 From: KRATOS <84986124+gnanam1990@users.noreply.github.com> Date: Sun, 16 Aug 2026 20:02:22 +0530 Subject: [PATCH 05/20] test(memory): exercise the ancestor guard against a Windows junction MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit's catch. The ancestor-link regression called os.Symlink directly, so on Windows it skipped — and a junction is precisely the case that guard is built for: a reparse point that is NOT a symlink, which is why RefuseReparse tests ModeIrregular alongside ModeSymlink. The Windows behaviour went unasserted while the run reported green. linkDir already existed in this file for exactly this reason and creates a junction there. Using it is the whole change. Same shape as the sandbox helper that reached for /Users/Shared and skipped off macOS: a skip is not a pass, and a guard whose platform-specific case is never exercised is a guard nobody has tested. Origin-Session: local-abff1c | Claude Code | 5 prompts Origin-Snapshot: dddd3415c4e0 Origin-Session: local-13d543 | Claude Code | 5 prompts Origin-Snapshot: 6b5eb8ba4e5b Origin-Session: local-c962d7 | Claude Code | 5 prompts Origin-Snapshot: 3bb420a0a97b --- internal/memory/escape_test.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/internal/memory/escape_test.go b/internal/memory/escape_test.go index 3717fcc91..e543a8c24 100644 --- a/internal/memory/escape_test.go +++ b/internal/memory/escape_test.go @@ -155,9 +155,12 @@ func TestALinkAboveTheStoreIsRefused(t *testing.T) { if err := os.MkdirAll(filepath.Dir(linkPath), 0o700); err != nil { t.Fatal(err) } - if err := os.Symlink(target, linkPath); err != nil { - t.Skipf("cannot create a symlink here: %v", 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) { From 5d3eb6c67dbfd46a0432cab9a5f843e3579cb5dc Mon Sep 17 00:00:00 2001 From: KRATOS <84986124+gnanam1990@users.noreply.github.com> Date: Sun, 16 Aug 2026 22:47:31 +0530 Subject: [PATCH 06/20] fix(memory): the ignore gate follows git's rules, and refuses a linked ignore MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three findings, all reproduced before changing anything, and the first two are the same mistake: certifying a privacy rule that git does not actually apply. GIT DOES NOT READ A SYMLINKED .gitignore. @anandh8x's P1. 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 ignores. An in-workspace RELATIVE link is followed by os.Root; an ABSOLUTE one is refused, which is exactly why my own review of this looked clean and was not — I attacked it with an absolute target and reported the refusal as the general answer. The ignore path now goes through the same no-reparse rule as a note before its contents are read. THE GATE DISAGREED WITH GIT IN BOTH DIRECTIONS. @Vasanthdev2004's finding, re-verified here against git 2.55.0 rather than taken from 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 precisely what this function exists to prevent, and it failed silently "*" 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 Both came from validating a pattern language by trimming and comparing strings. Leading whitespace is significant and is no longer stripped; trailing whitespace is not and still is; a BOM is stripped once at the start of the file, as git does. REFUSED IS NOT ABSENT. Also @Vasanthdev2004's: a junction at an ancestor made the store read as empty, so the caller was never told, and for this feature that means the model concludes its notes are gone and writes over what is there. A component cannot exist inside one that does not, so where absence does not hold all the way down the handle is refusing to traverse rather than reporting an empty path, and that now surfaces as a refusal. NOT VERIFIED, and this is the limitation to weigh: the junction behaviour itself is Windows-only and I am on darwin. The anomaly the check keys on is platform-independent, but that it is the shape a junction produces is his measurement, not mine. Origin-Session: local-abff1c | Claude Code | 7 prompts Origin-Snapshot: b7d0806d49f9 Origin-Session: local-13d543 | Claude Code | 5 prompts Origin-Snapshot: 6b5eb8ba4e5b Origin-Session: local-c962d7 | Claude Code | 5 prompts Origin-Snapshot: 3bb420a0a97b --- internal/memory/escape_test.go | 49 +++++++++++++++++++++++++++ internal/memory/memory.go | 62 ++++++++++++++++++++++++++++++++-- 2 files changed, 109 insertions(+), 2 deletions(-) diff --git a/internal/memory/escape_test.go b/internal/memory/escape_test.go index e543a8c24..3cc89af27 100644 --- a/internal/memory/escape_test.go +++ b/internal/memory/escape_test.go @@ -242,3 +242,52 @@ func TestALocalWriteRefusesAnIneffectiveIgnore(t *testing.T) { 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) { + 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) + } + linkDir(t, "decoy", filepath.Join(paths.LocalDir, ".gitignore")) + if _, err := Write(paths, ScopeLocal, "private", "d", "secret"); !errors.Is(err, ErrNotPrivate) { + t.Errorf("a symlinked ignore was accepted as privacy: %v", err) + } +} + +// 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}, + "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) + } + } +} diff --git a/internal/memory/memory.go b/internal/memory/memory.go index d08eebfc7..7d7230c02 100644 --- a/internal/memory/memory.go +++ b/internal/memory/memory.go @@ -230,14 +230,46 @@ func refuseReparseChain(handle *os.Root, relative string) error { return nil } parts := strings.Split(relative, string(filepath.Separator)) + absentAt := "" for i := range parts { - if err := pathjail.RefuseReparse(handle, filepath.Join(parts[:i+1]...)); err != nil { + component := filepath.Join(parts[:i+1]...) + if err := pathjail.RefuseReparse(handle, component); err != nil { return err } + // A COMPONENT CANNOT EXIST INSIDE ONE THAT DOES NOT. RefuseReparse reports + // absence as fine, which is right on its own — a store that has not been + // created yet is what a first write is for. But absence must then hold all + // the way down, and where it does not, the handle is refusing to traverse + // something rather than telling us the path is empty: a Windows junction + // at an ancestor reports that way, and the store beneath it then read as + // simply having no notes. "Refused" and "absent" are different answers, + // and only one of them should let a caller conclude its notes are gone. + exists, err := componentExists(handle, component) + if err != nil { + return err + } + switch { + case !exists && absentAt == "": + absentAt = component + case exists && absentAt != "": + return fmt.Errorf("%w: %s is unreadable, so %s cannot be inspected", + ErrIsSymlink, absentAt, component) + } } return nil } +// componentExists reports whether relative is present, without following a link. +func componentExists(handle *os.Root, relative string) (bool, error) { + if _, err := handle.Lstat(relative); err != nil { + if errors.Is(err, fs.ErrNotExist) { + return false, nil + } + return false, fmt.Errorf("inspect %s: %w", relative, err) + } + return true, nil +} + // Note is one stored memory. type Note struct { Name string @@ -444,6 +476,15 @@ func keepLocalScopePrivate(handle *os.Root, scope Scope, relative string) error 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) @@ -462,10 +503,27 @@ func keepLocalScopePrivate(handle *os.Root, scope Scope, relative string) error // 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") { - line = strings.TrimSpace(strings.TrimSuffix(line, "\r")) + // 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 } From 5ca491f059631d92a0e4903f2d38f68c2cac75cf Mon Sep 17 00:00:00 2001 From: KRATOS <84986124+gnanam1990@users.noreply.github.com> Date: Sun, 16 Aug 2026 23:03:39 +0530 Subject: [PATCH 07/20] test(memory): assert that a linked ignore is refused, not which error says so MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit My test, not the production code — it turned Windows red while the behaviour was correct. linkDir builds a JUNCTION on Windows, which is a directory, so the ignore path became a directory and the create failed with "is a directory" rather than reaching the reparse check that returns ErrNotPrivate. The write still failed closed; the assertion was simply about the wrong thing. Split in two, so each platform exercises the case it can actually build: file symlink what a .gitignore would really be; Windows needs a privilege for these, so it skips there directory reparse point junction on Windows, symlink elsewhere; the refusal arrives differently by platform, so what is asserted is the property that holds on both — THE WRITE DOES NOT SUCCEED Still mutation-checked: removing the reparse refusal makes the write succeed and both subtests fail. Origin-Session: local-abff1c | Claude Code | 7 prompts Origin-Snapshot: b7d0806d49f9 Origin-Session: local-13d543 | Claude Code | 5 prompts Origin-Snapshot: 6b5eb8ba4e5b Origin-Session: local-c962d7 | Claude Code | 5 prompts Origin-Snapshot: 3bb420a0a97b --- internal/memory/escape_test.go | 49 +++++++++++++++++++++++++--------- 1 file changed, 37 insertions(+), 12 deletions(-) diff --git a/internal/memory/escape_test.go b/internal/memory/escape_test.go index 3cc89af27..ed421cef0 100644 --- a/internal/memory/escape_test.go +++ b/internal/memory/escape_test.go @@ -251,18 +251,43 @@ func TestALocalWriteRefusesAnIneffectiveIgnore(t *testing.T) { // is refused, which is why an earlier check of exactly this looked clean and was // not. func TestALinkedIgnoreIsNotProofOfPrivacy(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) - } - linkDir(t, "decoy", filepath.Join(paths.LocalDir, ".gitignore")) - if _, err := Write(paths, ScopeLocal, "private", "d", "secret"); !errors.Is(err, ErrNotPrivate) { - t.Errorf("a symlinked ignore was accepted as privacy: %v", err) - } + // 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 From 9ade3b30e73d79e31f7222157698cddac4b9486a Mon Sep 17 00:00:00 2001 From: KRATOS <84986124+gnanam1990@users.noreply.github.com> Date: Mon, 17 Aug 2026 11:57:02 +0530 Subject: [PATCH 08/20] fix(memory): a refused ancestor is reported as a refusal, not as an empty store MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit @Vasanthdev2004 and @anandh8x both, and @Vasanthdev2004 named the real problem with my last attempt: I answered it by loosening the test rather than changing what the caller is told, and the commit title said so. He is right. This changes the behaviour. @anandh8x supplied the detail that made it tractable — the junction's target is OUTSIDE the workspace. So the confined handle reports the component absent while it is plainly present on disk, the chain read it and everything under it as missing, the later open became ErrNotFound, and List drops ErrNotFound. A tampered store presented as an empty one, which is exactly what memory.go's own contract forbids: 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. The harm is specific: the model concludes its notes are gone and writes over whatever is really there. Absence and refusal are now different answers. A component the handle will not open, while it is present on disk by ordinary pathname, has been refused and returns the reparse sentinel — which Read propagates and List reports, because neither suppresses anything but ErrNotFound. presentOnDisk stats and never opens, so nothing is traversed on the strength of it; it decides only which error the caller is given. An uncreated store is still absence, and that is asserted: a clean workspace must read ErrNotFound, list empty with no error, and accept its first write. NOT VERIFIED, and this is the limitation to weigh. I am on darwin, where a symlink is caught by the reparse check one step earlier, so the branch this adds is unreachable locally — the junction path is your measurement, not mine. What IS verified here is presentOnDisk itself: it reports a directory, a nested directory, a reparse point and a dangling link as present, a missing path and a blank root as absent. If either of you can re-run the junction case on Windows I would rather have that confirmation than my inference. Origin-Session: local-ae96ee | Claude Code | 4 prompts Origin-Snapshot: 701aaeb8081a Origin-Session: local-13d543 | Claude Code | 5 prompts Origin-Snapshot: 6b5eb8ba4e5b Origin-Session: local-c962d7 | Claude Code | 5 prompts Origin-Snapshot: 3bb420a0a97b --- internal/memory/escape_test.go | 56 ++++++++++++++++++++++++++++++++++ internal/memory/memory.go | 45 +++++++++++++++++++++++---- 2 files changed, 95 insertions(+), 6 deletions(-) diff --git a/internal/memory/escape_test.go b/internal/memory/escape_test.go index ed421cef0..492f6f5df 100644 --- a/internal/memory/escape_test.go +++ b/internal/memory/escape_test.go @@ -316,3 +316,59 @@ func TestTheIgnoreGateAgreesWithGit(t *testing.T) { } } } + +// 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) + } +} diff --git a/internal/memory/memory.go b/internal/memory/memory.go index 7d7230c02..e7cc75c64 100644 --- a/internal/memory/memory.go +++ b/internal/memory/memory.go @@ -209,7 +209,7 @@ func (paths Paths) openScope(scope Scope) (*os.Root, string, error) { // 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, relative); err != nil { + if err := refuseReparseChain(handle, paths.Root, relative); err != nil { handle.Close() return nil, "", err } @@ -224,7 +224,7 @@ func (paths Paths) openScope(scope Scope) (*os.Root, string, error) { // 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, relative string) error { +func refuseReparseChain(handle *os.Root, root string, relative string) error { relative = filepath.Clean(relative) if relative == "." || relative == string(filepath.Separator) { return nil @@ -248,10 +248,28 @@ func refuseReparseChain(handle *os.Root, relative string) error { if err != nil { return err } - switch { - case !exists && absentAt == "": - absentAt = component - case exists && absentAt != "": + if !exists { + // ABSENT TO THE HANDLE IS NOT ABSENT. A store that has not been + // created yet is what a first write is for, and reporting that as a + // problem would break every clean workspace. But 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. + // + // That is the outcome memory.go's own contract forbids, because the + // model then concludes its notes are gone and writes over whatever is + // really there. Absence and refusal have to be different answers. + if presentOnDisk(root, component) { + return fmt.Errorf("%w: %s cannot be opened inside the workspace", ErrIsSymlink, component) + } + if absentAt == "" { + absentAt = component + } + continue + } + if absentAt != "" { return fmt.Errorf("%w: %s is unreadable, so %s cannot be inspected", ErrIsSymlink, absentAt, component) } @@ -259,6 +277,21 @@ func refuseReparseChain(handle *os.Root, relative string) error { return 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 +} + // componentExists reports whether relative is present, without following a link. func componentExists(handle *os.Root, relative string) (bool, error) { if _, err := handle.Lstat(relative); err != nil { From f35133eba1ca743270162680f1cece6c6aebb4d2 Mon Sep 17 00:00:00 2001 From: KRATOS <84986124+gnanam1990@users.noreply.github.com> Date: Tue, 18 Aug 2026 12:46:18 +0530 Subject: [PATCH 09/20] fix(memory): decide refusal on what the handle can OPEN, not on what it can Lstat MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit @Vasanthdev2004 measured why my last attempt did not take, and he is exactly right. The branch was keyed on the component being ABSENT to the handle, and a junction is not absent to it: presentOnDisk(root, ".zero\\memory\\project") = true componentExists(handle, ".zero\\memory\\project") = true <- Lstat succeeds Lstat does not traverse a reparse point, so it reports the junction as being right there. !exists was false, presentOnDisk was never consulted, and the walk continued past the very component it was meant to refuse — the later open failed as ErrNotFound and List dropped it, exactly as before the "fix". The reasoning in the comment was right and the condition guarding it was wrong. OPENING IS WHAT TRAVERSES, so opening is the question. 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. A Windows junction is not a symlink and does not answer like one — os.ModeSymlink misses it, EvalSymlinks returns it unchanged, Lstat says it is there. Only the open disagrees. That is the third time this shape has bitten this branch. ErrUnreadable is its own sentinel rather than ErrIsSymlink, because this path cannot identify what it refused: a junction and a directory the process may not enter are indistinguishable here, and claiming "is a link" for the second would be a guess stated as fact. THE TEST RUNS ON THIS PLATFORM NOW. A directory that is present but cannot be entered produces the same disagreement as the junction — Lstat sees it, Open refuses it — and unlike the junction it is constructible on darwin and Linux. It skips where directory permissions are not enforced rather than passing vacuously. Reverting to the Lstat condition fails it. Absence is asserted separately: a clean workspace must still read ErrNotFound, list empty without error, and take its first write. Still unverified by me: the junction itself, which remains @Vasanthdev2004's measurement on Windows rather than mine. Origin-Session: local-13d543 | Claude Code | 5 prompts Origin-Snapshot: 6b5eb8ba4e5b Origin-Session: local-c962d7 | Claude Code | 5 prompts Origin-Snapshot: 3bb420a0a97b --- internal/memory/escape_test.go | 61 ++++++++++++++++++ internal/memory/memory.go | 109 +++++++++++++++------------------ 2 files changed, 109 insertions(+), 61 deletions(-) diff --git a/internal/memory/escape_test.go b/internal/memory/escape_test.go index 492f6f5df..618ab36e9 100644 --- a/internal/memory/escape_test.go +++ b/internal/memory/escape_test.go @@ -372,3 +372,64 @@ func TestAnUncreatedStoreIsStillAbsence(t *testing.T) { 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) + } + 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) }) + if _, err := os.Open(filepath.Join(blocked, "memory")); err == nil { + 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") + } +} + +// And absence is still absence: a workspace that has never held a store must +// read as missing, list empty without an error, and accept its first write. +// Treating that as a refusal would break every clean checkout. +func TestAnUncreatedStoreIsStillOrdinaryAbsence(t *testing.T) { + fresh := t.TempDir() + paths := DefaultPaths(fresh) + if _, err := Read(paths, ScopeProject, "nothing"); !errors.Is(err, ErrNotFound) { + t.Errorf("Read in a clean workspace = %v, want ErrNotFound", err) + } + notes, err := List(paths, ScopeProject) + if err != nil || len(notes) != 0 { + t.Errorf("List in a clean workspace = %+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) + } +} diff --git a/internal/memory/memory.go b/internal/memory/memory.go index e7cc75c64..ef38ea287 100644 --- a/internal/memory/memory.go +++ b/internal/memory/memory.go @@ -112,6 +112,13 @@ var ( // 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. + // 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 = errors.New("the local memory store is not ignored by git, so a note saved there 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 @@ -224,85 +231,65 @@ func (paths Paths) openScope(scope Scope) (*os.Root, string, error) { // 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. +// 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 +} + 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)) - absentAt := "" for i := range parts { component := filepath.Join(parts[:i+1]...) if err := pathjail.RefuseReparse(handle, component); err != nil { return err } - // A COMPONENT CANNOT EXIST INSIDE ONE THAT DOES NOT. RefuseReparse reports - // absence as fine, which is right on its own — a store that has not been - // created yet is what a first write is for. But absence must then hold all - // the way down, and where it does not, the handle is refusing to traverse - // something rather than telling us the path is empty: a Windows junction - // at an ancestor reports that way, and the store beneath it then read as - // simply having no notes. "Refused" and "absent" are different answers, - // and only one of them should let a caller conclude its notes are gone. - exists, err := componentExists(handle, component) - if err != nil { - return err - } - if !exists { - // ABSENT TO THE HANDLE IS NOT ABSENT. A store that has not been - // created yet is what a first write is for, and reporting that as a - // problem would break every clean workspace. But 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. - // - // That is the outcome memory.go's own contract forbids, because the - // model then concludes its notes are gone and writes over whatever is - // really there. Absence and refusal have to be different answers. - if presentOnDisk(root, component) { - return fmt.Errorf("%w: %s cannot be opened inside the workspace", ErrIsSymlink, component) - } - if absentAt == "" { - absentAt = component - } + // OPENABLE, NOT MERELY PRESENT. The previous version asked whether the + // component was ABSENT to the handle and only then compared against disk — + // and 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 + // !exists branch never ran, presentOnDisk was never consulted, and the walk + // continued straight past the component it was meant to refuse. The later + // open failed as ErrNotFound and List dropped it, exactly as before the + // fix. + // + // The question that separates absence from refusal is what the handle can + // OPEN, because opening is what traverses. A path the confined handle will + // not open while it is plainly present on disk has been REFUSED; a path + // that is absent from both is simply not there yet, which is what a first + // write is for. + // + // A Windows junction is not a symlink and does not answer like one: + // os.ModeSymlink misses it, EvalSymlinks returns it unchanged, and Lstat + // says it is there. Only the open disagrees. + opened, openErr := handle.Open(component) + if openErr == nil { + opened.Close() continue } - if absentAt != "" { - return fmt.Errorf("%w: %s is unreadable, so %s cannot be inspected", - ErrIsSymlink, absentAt, component) + 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 } -// 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 -} - -// componentExists reports whether relative is present, without following a link. -func componentExists(handle *os.Root, relative string) (bool, error) { - if _, err := handle.Lstat(relative); err != nil { - if errors.Is(err, fs.ErrNotExist) { - return false, nil - } - return false, fmt.Errorf("inspect %s: %w", relative, err) - } - return true, nil -} - // Note is one stored memory. type Note struct { Name string From defa01aa8dec5cfbe64312024f0ce601e955ff6b Mon Sep 17 00:00:00 2001 From: KRATOS <84986124+gnanam1990@users.noreply.github.com> Date: Tue, 18 Aug 2026 12:56:00 +0530 Subject: [PATCH 10/20] test(memory): close the probe handle that turned the Windows skip into a failure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit My own defect, and it broke the run on the platform the test exists to stand in for. The skip-check opened the blocked directory to find out whether this environment enforces directory permissions, and discarded the handle without closing it. Windows will not remove a directory while a handle is open, so t.TempDir's cleanup failed and the smoke job went red — after the test had correctly decided to skip. escape_test.go:404: this environment ignores directory permissions... testing.go:1464: TempDir RemoveAll cleanup: unlinkat ...\.zero\memory: The process cannot access the file because it is being used by another process. Origin-Session: local-13d543 | Claude Code | 5 prompts Origin-Snapshot: 6b5eb8ba4e5b Origin-Session: local-c962d7 | Claude Code | 5 prompts Origin-Snapshot: 3bb420a0a97b --- internal/memory/escape_test.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/internal/memory/escape_test.go b/internal/memory/escape_test.go index 618ab36e9..afb5f2779 100644 --- a/internal/memory/escape_test.go +++ b/internal/memory/escape_test.go @@ -400,7 +400,12 @@ func TestAPresentButUnopenableStoreIsReportedNotHidden(t *testing.T) { t.Skipf("cannot remove access here: %v", err) } t.Cleanup(func() { _ = os.Chmod(blocked, 0o700) }) - if _, err := os.Open(filepath.Join(blocked, "memory")); err == nil { + // 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") } From 9dd9d80995ef8e20c0c654b3ea542d89c124dec9 Mon Sep 17 00:00:00 2001 From: KRATOS <84986124+gnanam1990@users.noreply.github.com> Date: Wed, 19 Aug 2026 19:13:23 +0530 Subject: [PATCH 11/20] fix(memory): make the unscoped round trip return what it wrote, and stop a case clash destroying a note MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two findings from @Vasanthdev2004, both reproduced before changing anything, plus the corrections he asked for on the previous commit. SAVE A NOTE, READ IT STRAIGHT BACK, GET SOMEONE ELSE'S. memory_write and memory_forget default to local while an unscoped read resolved project FIRST, and scope is optional in every schema: memory_write{name, content} -> Saved "findings" (local). memory_read{name} -> memory "findings" (project) ... not what was written memory_forget{name} -> Forgot "findings" (local). memory_read{name} -> memory "findings" (project) ... still there Every message is accurate about the scope it acted on and the round trip is still broken. A checked-in project note is the ordinary way it happens, since it arrives with a clone and the model has no reason to expect it. The tool's own approval text is "Saves a note that future sessions will read and believe", which is exactly what fails. There is now ONE order, local first, matching where an unscoped write lands; a project note is still found when nothing local shadows it, so the ordering decides which wins rather than what exists. A NOTE THE LISTING DENIES EXISTS WAS DESTROYED BY THE NEXT WRITE. List matches the extension exactly; Read reopened name+".md", which on a case-insensitive filesystem 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 — verified: the note came back holding "OVERWRITTEN". Read now agrees with List, and the write refuses with ErrNameClash naming the occupant rather than taking its place. AND THE CORRECTION HE ASKED FOR. My previous commit was written as a junction fix. It is not one: he re-measured and every junction placement is already refused a layer earlier, because os.Root.Lstat reports a junction as ModeIrregular and RefuseReparse rejects it on the first component. The comment now says what the branch actually covers — a store present but unopenable for an ordinary reason, which used to surface as "no such memory" — and records the race he spotted, where a store being created concurrently reads as refused rather than absent. The chmod arm cannot build its refusal on Windows, since os.Chmod there only toggles the read-only attribute — the opposite of what my comment claimed. A junction companion covers that platform, so between the two arms the branch is exercised everywhere and neither passes vacuously. Mutation-checked: project-first breaks the round trip, and removing the clash guard destroys the hand-authored note again. Origin-Session: local-13d543 | Claude Code | 5 prompts Origin-Snapshot: 6b5eb8ba4e5b Origin-Session: local-c962d7 | Claude Code | 5 prompts Origin-Snapshot: 3bb420a0a97b --- internal/memory/escape_test.go | 143 +++++++++++++++++++++++++++++++++ internal/memory/memory.go | 123 ++++++++++++++++++++++++---- 2 files changed, 249 insertions(+), 17 deletions(-) diff --git a/internal/memory/escape_test.go b/internal/memory/escape_test.go index afb5f2779..b126a9078 100644 --- a/internal/memory/escape_test.go +++ b/internal/memory/escape_test.go @@ -395,6 +395,11 @@ func TestAPresentButUnopenableStoreIsReportedNotHidden(t *testing.T) { 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) @@ -438,3 +443,141 @@ func TestAnUncreatedStoreIsStillOrdinaryAbsence(t *testing.T) { t.Errorf("the first write into a clean workspace failed: %v", err) } } + +// 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") + } +} diff --git a/internal/memory/memory.go b/internal/memory/memory.go index ef38ea287..338092845 100644 --- a/internal/memory/memory.go +++ b/internal/memory/memory.go @@ -118,6 +118,12 @@ var ( // 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. + // 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 = errors.New("the memory store exists but could not be read") ErrNotPrivate = errors.New("the local memory store is not ignored by git, so a note saved there would not stay on this machine") // ErrIsSymlink is pathjail's refusal, kept under this package's own name so @@ -231,6 +237,41 @@ func (paths Paths) openScope(scope Scope) (*os.Root, string, error) { // 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. +// 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 @@ -257,24 +298,29 @@ func refuseReparseChain(handle *os.Root, root string, relative string) error { if err := pathjail.RefuseReparse(handle, component); err != nil { return err } - // OPENABLE, NOT MERELY PRESENT. The previous version asked whether the - // component was ABSENT to the handle and only then compared against disk — - // and 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 - // !exists branch never ran, presentOnDisk was never consulted, and the walk - // continued straight past the component it was meant to refuse. The later - // open failed as ErrNotFound and List dropped it, exactly as before the - // fix. + // 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. // - // The question that separates absence from refusal is what the handle can - // OPEN, because opening is what traverses. A path the confined handle will - // not open while it is plainly present on disk has been REFUSED; a path - // that is absent from both is simply not there yet, which is what a first - // write is for. + // 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. // - // A Windows junction is not a symlink and does not answer like one: - // os.ModeSymlink misses it, EvalSymlinks returns it unchanged, and Lstat - // says it is there. Only the open disagrees. + // 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() @@ -311,6 +357,30 @@ func ValidName(name string) bool { 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} + // ResolveScopes turns a requested scope into the scopes to search. // // ONE place decides, because there were two and they disagreed: the write path @@ -323,7 +393,7 @@ func ValidName(name string) bool { func ResolveScopes(requested string) ([]Scope, error) { trimmed := strings.TrimSpace(requested) if trimmed == "" { - return []Scope{ScopeProject, ScopeLocal}, nil + return append([]Scope(nil), DefaultScopeOrder...), nil } switch scope := Scope(strings.ToLower(trimmed)); scope { case ScopeProject, ScopeLocal: @@ -440,6 +510,16 @@ func Read(paths Paths, scope Scope, name string) (Note, error) { 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{}, fmt.Errorf("%w: %s is stored as %s", ErrNameClash, name+fileExt, stored) + } body, err := readBounded(handle, relativePath) if err != nil { if os.IsNotExist(err) { @@ -617,6 +697,15 @@ func Write(paths Paths, scope Scope, name, description, body string) (string, er 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 "", fmt.Errorf("%w: %s is stored as %s; rename or remove it first", ErrNameClash, name+fileExt, 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) From 0932672f87fb8100e99776c297eaacf594a6538a Mon Sep 17 00:00:00 2001 From: KRATOS <84986124+gnanam1990@users.noreply.github.com> Date: Thu, 20 Aug 2026 17:16:54 +0530 Subject: [PATCH 12/20] fix(memory): one resolution order, a named listing order, and each sentinel doc on its own error MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit's review of the current head. Three real, one declined with reasoning. THE SCOPE FIX WAS INCOMPLETE. List kept its own project-first literal, so making ResolveScopes authoritative fixed the read round trip and left the listing walking the other way — two copies of one decision, which is exactly how they drift. Correctly flagged. BUT THEY ARE NOT ONE DECISION, and following the suggestion literally would have changed behaviour a test asserts on purpose: 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 in a listing puts the checked-in, team-facing notes at the top of a long one. So the listing order is now NAMED rather than left as a literal, with the difference from the resolution order written down — the duplication CodeRabbit saw was real, and the fix is to state the distinction rather than erase it. DefaultScopeOrder now returns a copy through an accessor, with the backing slice private: handing out the slice let any caller reorder the resolution every other caller depends on. EACH GODOC BACK ON ITS OWN ERROR. Inserting two sentinels above the block left every comment attached to the wrong error — ErrNotPrivate's text sat above ErrNameClash, and so on down. Cosmetic, and the kind of cosmetic that misleads whoever reads the package docs next. Also removed a duplicate uncreated-store test I added without noticing the existing one, and added CRLF cases to the ignore-gate table: a .gitignore written on Windows carries them, git does not care, and if the gate ever stops stripping them every local write on a Windows checkout fails for a file doing its job. Not done: reusing List's ReadDir entries to skip storedEntryName per note. It is a real saving, but it adds a second read path next to the one that just fixed a correctness bug, and the listing is bounded by the store size. Origin-Session: local-8cd239 | Claude Code | 1 prompt Origin-Snapshot: 627f6f8664c6 Origin-Session: local-13d543 | Claude Code | 5 prompts Origin-Snapshot: 6b5eb8ba4e5b Origin-Session: local-c962d7 | Claude Code | 5 prompts Origin-Snapshot: 3bb420a0a97b --- internal/memory/escape_test.go | 24 ++++------------ internal/memory/memory.go | 51 +++++++++++++++++++++++++--------- 2 files changed, 44 insertions(+), 31 deletions(-) diff --git a/internal/memory/escape_test.go b/internal/memory/escape_test.go index b126a9078..0ca3d7eb9 100644 --- a/internal/memory/escape_test.go +++ b/internal/memory/escape_test.go @@ -309,6 +309,12 @@ func TestTheIgnoreGateAgreesWithGit(t *testing.T) { "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 { @@ -426,24 +432,6 @@ func TestAPresentButUnopenableStoreIsReportedNotHidden(t *testing.T) { } } -// And absence is still absence: a workspace that has never held a store must -// read as missing, list empty without an error, and accept its first write. -// Treating that as a refusal would break every clean checkout. -func TestAnUncreatedStoreIsStillOrdinaryAbsence(t *testing.T) { - fresh := t.TempDir() - paths := DefaultPaths(fresh) - if _, err := Read(paths, ScopeProject, "nothing"); !errors.Is(err, ErrNotFound) { - t.Errorf("Read in a clean workspace = %v, want ErrNotFound", err) - } - notes, err := List(paths, ScopeProject) - if err != nil || len(notes) != 0 { - t.Errorf("List in a clean workspace = %+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) - } -} - // SAVE A NOTE, READ IT STRAIGHT BACK, GET YOUR OWN. // // memory_write and memory_forget default to local while an unscoped read diff --git a/internal/memory/memory.go b/internal/memory/memory.go index 338092845..f2e53d3a0 100644 --- a/internal/memory/memory.go +++ b/internal/memory/memory.go @@ -108,23 +108,23 @@ var ( 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"`) - // 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. + // 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. - // 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 = 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. ErrNotPrivate = errors.New("the local memory store is not ignored by git, so a note saved there 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 @@ -379,7 +379,19 @@ func ValidName(name string) bool { // // 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} +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. // @@ -393,7 +405,7 @@ var DefaultScopeOrder = []Scope{ScopeLocal, ScopeProject} func ResolveScopes(requested string) ([]Scope, error) { trimmed := strings.TrimSpace(requested) if trimmed == "" { - return append([]Scope(nil), DefaultScopeOrder...), nil + return DefaultScopeOrder(), nil } switch scope := Scope(strings.ToLower(trimmed)); scope { case ScopeProject, ScopeLocal: @@ -417,7 +429,20 @@ func ResolveScopes(requested string) ([]Scope, error) { // an empty memory. func List(paths Paths, scopes ...Scope) ([]Note, error) { if len(scopes) == 0 { - scopes = []Scope{ScopeProject, ScopeLocal} + // 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 From a44e9d895207a2ff947cd6d93027c5c93d7c3af3 Mon Sep 17 00:00:00 2001 From: KRATOS <84986124+gnanam1990@users.noreply.github.com> Date: Thu, 20 Aug 2026 21:06:35 +0530 Subject: [PATCH 13/20] fix(memory): the delete door takes the clash guard the other two already had MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both reported by @Vasanthdev2004. ## memory_forget destroyed what read and write refuse to touch Same hand-authored findings.MD, project scope, three calls at 6df0c097: 1. Read -> a differently spelled note already occupies this name 2. Write -> a differently spelled note already occupies this name; rename or remove it first 3. Forget -> ok 4. on disk: nothing left Read and Write both consult storedEntryName and refuse when the stored spelling differs, because on a case-insensitive filesystem findings.md and findings.MD are one file. Forget did not, so the one operation with nothing left to inspect afterwards was the one that ignored the guard. The tool layer gates only on ErrNotFound; a clash returns ErrNameClash, falls through, and handle.Remove takes the occupant with it. This is not a regression: forget behaved this way before the change too. It matters more now because the change establishes a contract that forget contradicts, and because the refusal the other two doors return said "rename or remove it first" while memory_forget is the obvious way to remove. The message routed the reader from a refusal to the destructive door. The guard now lives in memory.Forget rather than in the tool, so every caller gets it and not just this one. Absence is still not an error: a name nobody has stored has no occupant to clash with, and the documented idempotence is unchanged. All three doors share one wording, which names the occupant and asks for a rename in the store directory — the only step that actually resolves the clash, and not a suggestion to delete the file being described. ## The named listing order applied to nothing anyone could see ResolveScopes("") returns [local project], so memory.List's len(scopes) == 0 branch was unreachable from the product and listingScopeOrder never ran. The variable, its comment, and TestListingShowsBothScopesWithoutShadowing all described a project-first listing that no user was getting. Wired up rather than deleted, because the distinction it draws is real: resolution order decides which scope WINS a name and must be local-first; listing order decides only what a reader sees first and shadows nothing. The listing tool now hands List no scopes when the caller named none, which is what asks List to choose. A caller who names a scope still gets exactly that one, and the unknown-scope refusal is unaffected. Three mutations, each caught by the test written for it: dropping the Forget guard deletes the note; forwarding the resolved order lists local first; restoring the "remove it first" wording fails the assertion that a refusal must not route the reader to the sharpest door. Pre-existing on this branch and on its merge-base, in this environment only: TestRunDoctorFormatsRedactedProviderDiagnostics and TestRunDoctorConnectivityProbesProvider both exit 3. Origin-Session: local-8cd239 | Claude Code | 11 prompts Origin-Snapshot: 365efe3045f2 Origin-Session: local-13d543 | Claude Code | 5 prompts Origin-Snapshot: 6b5eb8ba4e5b Origin-Session: local-c962d7 | Claude Code | 5 prompts Origin-Snapshot: 3bb420a0a97b --- internal/memory/memory.go | 31 ++++++++++++++- internal/tools/memory.go | 15 +++++++- internal/tools/memory_test.go | 72 +++++++++++++++++++++++++++++++++++ 3 files changed, 115 insertions(+), 3 deletions(-) diff --git a/internal/memory/memory.go b/internal/memory/memory.go index f2e53d3a0..fa74554d5 100644 --- a/internal/memory/memory.go +++ b/internal/memory/memory.go @@ -543,7 +543,7 @@ func Read(paths Paths, scope Scope, name string) (Note, error) { return Note{}, entryErr } if found && stored != name+fileExt { - return Note{}, fmt.Errorf("%w: %s is stored as %s", ErrNameClash, name+fileExt, stored) + return Note{}, clashError(name, stored) } body, err := readBounded(handle, relativePath) if err != nil { @@ -729,7 +729,7 @@ func Write(paths Paths, scope Scope, name, description, body string) (string, er if stored, found, entryErr := storedEntryName(handle, relative, name); entryErr != nil { return "", entryErr } else if found && stored != name+fileExt { - return "", fmt.Errorf("%w: %s is stored as %s; rename or remove it first", ErrNameClash, name+fileExt, stored) + return "", clashError(name, stored) } file, temp, err := pathjail.CreateTemp(handle, relative, name, tempExt) if err != nil { @@ -770,12 +770,39 @@ func Forget(paths Paths, scope Scope, name string) error { 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: ") diff --git a/internal/tools/memory.go b/internal/tools/memory.go index 74f71bd1b..03650e141 100644 --- a/internal/tools/memory.go +++ b/internal/tools/memory.go @@ -89,7 +89,20 @@ func (tool memoryTool) Run(_ context.Context, args map[string]any) Result { } if strings.TrimSpace(name) == "" { - notes, listErr := memory.List(tool.paths, scopes...) + // 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 diff --git a/internal/tools/memory_test.go b/internal/tools/memory_test.go index 759653030..b9517e036 100644 --- a/internal/tools/memory_test.go +++ b/internal/tools/memory_test.go @@ -205,3 +205,75 @@ func TestAProjectFailureDoesNotHideTheLocalNote(t *testing.T) { t.Errorf("the local 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) + } +} From a142329e9eeaeafdc8d490291f0f44d47f0b33a7 Mon Sep 17 00:00:00 2001 From: KRATOS <84986124+gnanam1990@users.noreply.github.com> Date: Mon, 24 Aug 2026 14:19:38 +0530 Subject: [PATCH 14/20] fix(memory): the index decides whether a local note is private, not the ignore file MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reported by @jatmn. keepLocalScopePrivate treated a local/.gitignore containing "*" as proof that later writes could not enter git. That holds for an UNTRACKED path and for nothing else: git applies no ignore rule to a path already in the index. A clone, an earlier `git add -f`, or a hand-authored repository restores both the ignore AND local/.md as tracked, and the atomic rename 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. refuseTrackedLocalStore asks git what its index actually holds under the store, and gates BOTH branches. Refusing only where an existing ignore is accepted would leave the same leak reachable by deleting the ignore from the worktree: O_EXCL would create a fresh one, report a clean first write, and overwrite the tracked note anyway. The regression jatmn asked for is there: a repository with a committed ignore and a committed local note refuses the write and leaves nothing index-visible. ## A linked worktree carries .git as a FILE Found by a verification pass, and it was the real gap. gitDirName's comment says a file counts as much as a directory — that is what a linked worktree and a submodule carry — and nothing tested it. Changing the check to `err == nil && info.IsDir()` compiled and all nine tests still passed, so the whole linked-worktree and submodule class was resting on a comment. TestALocalWriteIsRefusedInsideALinkedWorktree builds the real article with `git worktree add` rather than a stand-in, so it exercises the same code a user's linked worktree does. That mutation now fails it. ## Two branches that are deliberately unreachable The same pass reported the EvalSymlinks-failure branch and enclosingGitRepo's unreadable-ancestor branch as untested fail-open points. Both mutate cleanly and break nothing, and I could not build a fixture that reaches either — because 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: a self-referential store link is refused as a link, and a 000 ancestor fails at mkdir. Measured both. They stay, and the code now says why: reachability is a property of the CALLERS, not of this function, whose 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. Writing a mock seam purely to reach dead code would have tested the mock. Rebased onto ad34dc8d. Pre-existing here and on main: TestRunDoctorFormatsRedactedProviderDiagnostics and TestRunDoctorConnectivityProbesProvider exit 3 in this environment. Origin-Session: local-c962d7 | Claude Code | 5 prompts Origin-Snapshot: 3bb420a0a97b --- internal/memory/escape_test.go | 328 +++++++++++++++++++++++++++++++++ internal/memory/memory.go | 213 ++++++++++++++++++++- 2 files changed, 534 insertions(+), 7 deletions(-) diff --git a/internal/memory/escape_test.go b/internal/memory/escape_test.go index 0ca3d7eb9..ae938c733 100644 --- a/internal/memory/escape_test.go +++ b/internal/memory/escape_test.go @@ -569,3 +569,331 @@ func TestAStoreBehindAReparsePointIsRefusedOnEveryPlatform(t *testing.T) { 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) + } +} + +// 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") + } +} + +// 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) + } + 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 index fa74554d5..61ffa9db1 100644 --- a/internal/memory/memory.go +++ b/internal/memory/memory.go @@ -20,15 +20,19 @@ 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" ) @@ -54,6 +58,21 @@ 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" @@ -124,8 +143,10 @@ var ( // 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. - ErrNotPrivate = errors.New("the local memory store is not ignored by git, so a note saved there would not stay on this machine") + // 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. @@ -560,8 +581,9 @@ func Read(paths Paths, scope Scope, name string) (Note, error) { return Note{Name: name, Description: description, Scope: scope, Body: text}, nil } -// keepLocalScopePrivate installs, or verifies, the ignore that keeps local notes -// out of the repository. +// 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 @@ -583,10 +605,26 @@ func Read(paths Paths, scope Scope, name string) (Note, error) { // 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 string) error { +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(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. @@ -615,7 +653,7 @@ func keepLocalScopePrivate(handle *os.Root, scope Scope, relative string) error return fmt.Errorf("read %s: %w", ignorePath, err) } if !ignoresEverything(string(existing)) { - return fmt.Errorf("%w: %s", ErrNotPrivate, ignorePath) + return fmt.Errorf("%w: %s does not ignore the whole store", ErrNotPrivate, ignorePath) } return nil } @@ -662,6 +700,167 @@ func ignoresEverything(content string) bool { return covered } +// refuseTrackedLocalStore refuses a local write whose store git already TRACKS. +// +// The two questions are asked by different means on purpose. Whether a +// repository encloses the store is answered from the FILESYSTEM, because "this +// is not a repository" and "git is not installed" both come back from a +// subprocess as an error, and a check that cannot tell them apart has to choose +// between failing open on the first and breaking every non-repository workspace +// on the second. Looking for .git separates them without running anything. Only +// once a repository is found does the INDEX get consulted, and only there does +// an unanswerable question become a refusal. +func refuseTrackedLocalStore(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 := filepath.EvalSymlinks(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 a .git entry, or "" when none does. +func enclosingGitRepo(dir string) (string, error) { + for current := filepath.Clean(dir); ; { + _, err := os.Lstat(filepath.Join(current, gitDirName)) + switch { + case err == nil: + return current, nil + case !errors.Is(err, fs.ErrNotExist): + // Unreadable is not absent. Reporting "no repository above this" from a + // permission error is the same fail-open shape in a smaller place. + return "", err + } + parent := filepath.Dir(current) + if parent == current { + return "", nil + } + current = parent + } +} + +// 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. // @@ -714,7 +913,7 @@ func Write(paths Paths, scope Scope, name, description, body string) (string, er if err := handle.MkdirAll(relative, 0o700); err != nil { return "", fmt.Errorf("create %s: %w", dir, err) } - if err := keepLocalScopePrivate(handle, scope, relative); err != nil { + if err := keepLocalScopePrivate(handle, scope, relative, dir); err != nil { return "", err } path := filepath.Join(dir, name+fileExt) From 007b39f1bdab6e766eaffcb804723f4d019f7743 Mon Sep 17 00:00:00 2001 From: KRATOS <84986124+gnanam1990@users.noreply.github.com> Date: Wed, 9 Sep 2026 07:55:36 +0530 Subject: [PATCH 15/20] fix(memory): resolve linked workspaces by handle --- internal/memory/escape_test.go | 2 + internal/memory/memory.go | 22 +++++------ internal/memory/physical_path_other.go | 12 ++++++ internal/memory/physical_path_windows.go | 48 ++++++++++++++++++++++++ 4 files changed, 73 insertions(+), 11 deletions(-) create mode 100644 internal/memory/physical_path_other.go create mode 100644 internal/memory/physical_path_windows.go diff --git a/internal/memory/escape_test.go b/internal/memory/escape_test.go index ae938c733..b63785a3f 100644 --- a/internal/memory/escape_test.go +++ b/internal/memory/escape_test.go @@ -849,6 +849,8 @@ func TestTheRepositoryIsFoundThroughALinkedWorkspace(t *testing.T) { 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) diff --git a/internal/memory/memory.go b/internal/memory/memory.go index 61ffa9db1..a70bffa55 100644 --- a/internal/memory/memory.go +++ b/internal/memory/memory.go @@ -250,14 +250,6 @@ func (paths Paths) openScope(scope Scope) (*os.Root, string, error) { return handle, relative, 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. // storedEntryName returns the spelling the store actually holds for a note, and // whether anything holds it at all. // @@ -308,6 +300,14 @@ func presentOnDisk(root string, relative string) bool { 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) { @@ -622,7 +622,7 @@ func keepLocalScopePrivate(handle *os.Root, scope Scope, relative, dir string) e // 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(dir); err != nil { + if err := refuseTrackedLocalStore(handle, relative, dir); err != nil { return err } ignorePath := filepath.Join(relative, gitignoreName) @@ -710,7 +710,7 @@ func ignoresEverything(content string) bool { // on the second. Looking for .git separates them without running anything. Only // once a repository is found does the INDEX get consulted, and only there does // an unanswerable question become a refusal. -func refuseTrackedLocalStore(dir string) error { +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 @@ -729,7 +729,7 @@ func refuseTrackedLocalStore(dir string) error { // 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 := filepath.EvalSymlinks(dir) + 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) } 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") +} From 45292e0a2bbc2e388c109e40865f06626a8392b2 Mon Sep 17 00:00:00 2001 From: KRATOS <84986124+gnanam1990@users.noreply.github.com> Date: Sat, 12 Sep 2026 11:18:34 +0530 Subject: [PATCH 16/20] fix(memory): distinguish inert git markers from repository metadata --- internal/memory/escape_test.go | 62 ++++++++++++++++++++++++ internal/memory/memory.go | 88 +++++++++++++++++++++++++++------- 2 files changed, 134 insertions(+), 16 deletions(-) diff --git a/internal/memory/escape_test.go b/internal/memory/escape_test.go index b63785a3f..7a8bd93a3 100644 --- a/internal/memory/escape_test.go +++ b/internal/memory/escape_test.go @@ -2,6 +2,7 @@ package memory import ( "errors" + "fmt" "os" "os/exec" "path/filepath" @@ -799,6 +800,67 @@ func TestALocalWriteOutsideAnyRepositoryNeedsNoGit(t *testing.T) { } } +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 diff --git a/internal/memory/memory.go b/internal/memory/memory.go index a70bffa55..e70fb4884 100644 --- a/internal/memory/memory.go +++ b/internal/memory/memory.go @@ -702,14 +702,11 @@ func ignoresEverything(content string) bool { // refuseTrackedLocalStore refuses a local write whose store git already TRACKS. // -// The two questions are asked by different means on purpose. Whether a -// repository encloses the store is answered from the FILESYSTEM, because "this -// is not a repository" and "git is not installed" both come back from a -// subprocess as an error, and a check that cannot tell them apart has to choose -// between failing open on the first and breaking every non-repository workspace -// on the second. Looking for .git separates them without running anything. Only -// once a repository is found does the INDEX get consulted, and only there does -// an unanswerable question become a refusal. +// 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 @@ -768,18 +765,17 @@ func refuseTrackedLocalStore(handle *os.Root, relative, dir string) error { } // enclosingGitRepo returns the nearest ancestor of dir, dir itself included, -// carrying a .git entry, or "" when none does. +// 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); ; { - _, err := os.Lstat(filepath.Join(current, gitDirName)) - switch { - case err == nil: - return current, nil - case !errors.Is(err, fs.ErrNotExist): - // Unreadable is not absent. Reporting "no repository above this" from a - // permission error is the same fail-open shape in a smaller place. + 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 @@ -788,6 +784,66 @@ func enclosingGitRepo(dir string) (string, error) { } } +// 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. // From 0a4c36bb06c8a608dfd3f2601447c108969775ca Mon Sep 17 00:00:00 2001 From: KRATOS <84986124+gnanam1990@users.noreply.github.com> Date: Sat, 12 Sep 2026 23:25:40 +0530 Subject: [PATCH 17/20] fix(memory): recover failed ignore installation --- internal/memory/escape_test.go | 27 +++++++++++++++++++++++++++ internal/memory/memory.go | 15 ++++++++++++--- internal/tools/memory.go | 8 +++----- internal/tools/memory_test.go | 22 +++++++++++----------- 4 files changed, 53 insertions(+), 19 deletions(-) diff --git a/internal/memory/escape_test.go b/internal/memory/escape_test.go index 7a8bd93a3..93baed563 100644 --- a/internal/memory/escape_test.go +++ b/internal/memory/escape_test.go @@ -692,6 +692,33 @@ func TestALocalWriteIsRefusedWhenTrackedWithNoIgnorePresent(t *testing.T) { } } +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") + } +} + // 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. diff --git a/internal/memory/memory.go b/internal/memory/memory.go index e70fb4884..b00eeae77 100644 --- a/internal/memory/memory.go +++ b/internal/memory/memory.go @@ -631,11 +631,15 @@ func keepLocalScopePrivate(handle *os.Root, scope Scope, relative, dir string) e file, err := handle.OpenFile(ignorePath, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0o600) switch { case err == nil: - defer file.Close() - if _, writeErr := file.WriteString(localIgnoreContent); writeErr != 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. + _ = handle.Remove(ignorePath) return fmt.Errorf("write %s: %w", ignorePath, writeErr) } - return nil + return file.Close() case !errors.Is(err, fs.ErrExist): return fmt.Errorf("create %s: %w", ignorePath, err) } @@ -658,6 +662,11 @@ func keepLocalScopePrivate(handle *os.Root, scope Scope, relative, dir string) e return nil } +var writeLocalIgnore = func(file *os.File) error { + _, err := file.WriteString(localIgnoreContent) + return err +} + // ignoresEverything reports whether an existing ignore file actually excludes the // whole directory. // diff --git a/internal/tools/memory.go b/internal/tools/memory.go index 03650e141..4ee5bde8d 100644 --- a/internal/tools/memory.go +++ b/internal/tools/memory.go @@ -115,11 +115,9 @@ func (tool memoryTool) Run(_ context.Context, args map[string]any) Result { } return okResult(rendered) } - // A failure in one scope must not hide a readable note in the next. Project is - // searched first, is checked in, and arrives with a clone; local is the user's - // own default write scope. An unreadable project note called "findings" would - // otherwise make the user's own local "findings" unreachable — the shared, - // externally-supplied scope masking the private one. The failure is carried and + // 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 { diff --git a/internal/tools/memory_test.go b/internal/tools/memory_test.go index b9517e036..5aac9d7f0 100644 --- a/internal/tools/memory_test.go +++ b/internal/tools/memory_test.go @@ -180,29 +180,29 @@ func TestAnUnreadableNoteDoesNotEmptyTheListing(t *testing.T) { } } -// A failure in the project scope must not hide the user's own local note. -// Project is searched first and arrives with a clone; local is the default write -// scope, so the shared scope masking the private one is the wrong way round. -func TestAProjectFailureDoesNotHideTheLocalNote(t *testing.T) { +// 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.ScopeLocal, "findings", "mine", "the local body"); err != nil { + if _, err := memory.Write(paths, memory.ScopeProject, "findings", "team", "the project body"); err != nil { t.Fatal(err) } - if err := os.MkdirAll(paths.ProjectDir, 0o700); err != nil { + if err := os.MkdirAll(paths.LocalDir, 0o700); err != nil { t.Fatal(err) } - // An unreadable project note of the SAME name, searched first. + // An unreadable local note of the SAME name, searched first. oversized := strings.Repeat("x", 70<<10) - if err := os.WriteFile(filepath.Join(paths.ProjectDir, "findings.md"), []byte(oversized), 0o600); err != nil { + 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 project-scope failure hid the readable local note: %q", result.Output) + t.Fatalf("a local-scope failure hid the readable project note: %q", result.Output) } - if !strings.Contains(result.Output, "the local body") { - t.Errorf("the local note was not returned: %q", result.Output) + if !strings.Contains(result.Output, "the project body") { + t.Errorf("the project note was not returned: %q", result.Output) } } From 969b3c9350e7adb82413e69624cda414725771e4 Mon Sep 17 00:00:00 2001 From: KRATOS <84986124+gnanam1990@users.noreply.github.com> Date: Sat, 12 Sep 2026 23:43:24 +0530 Subject: [PATCH 18/20] fix(memory): remove ignore after close failure --- internal/memory/escape_test.go | 25 +++++++++++++++++++++++++ internal/memory/memory.go | 12 +++++++++++- 2 files changed, 36 insertions(+), 1 deletion(-) diff --git a/internal/memory/escape_test.go b/internal/memory/escape_test.go index 93baed563..b81baf84a 100644 --- a/internal/memory/escape_test.go +++ b/internal/memory/escape_test.go @@ -719,6 +719,31 @@ func TestAFailedIgnoreWriteCanBeRetried(t *testing.T) { } } +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) + } +} + // 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. diff --git a/internal/memory/memory.go b/internal/memory/memory.go index b00eeae77..9848d29a4 100644 --- a/internal/memory/memory.go +++ b/internal/memory/memory.go @@ -639,7 +639,13 @@ func keepLocalScopePrivate(handle *os.Root, scope Scope, relative, dir string) e _ = handle.Remove(ignorePath) return fmt.Errorf("write %s: %w", ignorePath, writeErr) } - return file.Close() + 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. + _ = handle.Remove(ignorePath) + return fmt.Errorf("close %s: %w", ignorePath, closeErr) + } + return nil case !errors.Is(err, fs.ErrExist): return fmt.Errorf("create %s: %w", ignorePath, err) } @@ -667,6 +673,10 @@ var writeLocalIgnore = func(file *os.File) error { return err } +var closeLocalIgnore = func(file *os.File) error { + return file.Close() +} + // ignoresEverything reports whether an existing ignore file actually excludes the // whole directory. // From d9da8385f9c3fed2558fd4bf6a7d04dd3423b9ec Mon Sep 17 00:00:00 2001 From: KRATOS <84986124+gnanam1990@users.noreply.github.com> Date: Sat, 12 Sep 2026 23:55:58 +0530 Subject: [PATCH 19/20] fix(memory): report ignore cleanup failures --- internal/memory/escape_test.go | 33 +++++++++++++++++++++++++++++++++ internal/memory/memory.go | 21 ++++++++++++++++----- 2 files changed, 49 insertions(+), 5 deletions(-) diff --git a/internal/memory/escape_test.go b/internal/memory/escape_test.go index b81baf84a..5ff0d8ee4 100644 --- a/internal/memory/escape_test.go +++ b/internal/memory/escape_test.go @@ -744,6 +744,39 @@ func TestAFailedIgnoreCloseCanBeRetried(t *testing.T) { } } +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. diff --git a/internal/memory/memory.go b/internal/memory/memory.go index 9848d29a4..f6bb78842 100644 --- a/internal/memory/memory.go +++ b/internal/memory/memory.go @@ -636,14 +636,14 @@ func keepLocalScopePrivate(handle *os.Root, scope Scope, relative, dir string) e // 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. - _ = handle.Remove(ignorePath) - return fmt.Errorf("write %s: %w", ignorePath, writeErr) + 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. - _ = handle.Remove(ignorePath) - return fmt.Errorf("close %s: %w", ignorePath, closeErr) + // 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): @@ -677,6 +677,17 @@ 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. // From ee9776c35ea1ff6c18589e95ec21d7b43d5de9eb Mon Sep 17 00:00:00 2001 From: KRATOS <84986124+gnanam1990@users.noreply.github.com> Date: Sun, 13 Sep 2026 00:00:10 +0530 Subject: [PATCH 20/20] fix(memory): refuse deletion after read failure --- internal/tools/memory.go | 10 ++++++++-- internal/tools/memory_test.go | 27 +++++++++++++++++++++++++++ 2 files changed, 35 insertions(+), 2 deletions(-) diff --git a/internal/tools/memory.go b/internal/tools/memory.go index 4ee5bde8d..bfaa27877 100644 --- a/internal/tools/memory.go +++ b/internal/tools/memory.go @@ -270,8 +270,14 @@ func (tool memoryForgetTool) Run(_ context.Context, args map[string]any) Result // 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 _, err := memory.Read(tool.paths, scope, name); errors.Is(err, memory.ErrNotFound) { - return okResult(fmt.Sprintf("No note named %q in %s, so there was nothing to forget.", name, scope)) + 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()) diff --git a/internal/tools/memory_test.go b/internal/tools/memory_test.go index 5aac9d7f0..d0f584a9d 100644 --- a/internal/tools/memory_test.go +++ b/internal/tools/memory_test.go @@ -156,6 +156,33 @@ func TestMemoryForgetReportsAbsenceRatherThanClaimingSuccess(t *testing.T) { } } +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