claude-code: memory carrying never leaves a memory folder, never takes the unreadable for gone, and screens every write (issue 367, review of #276)

A value in the state could name a project such as ../../.. and be written outside
the memory folder; a file or folder that could not be read was carried as a
removal; text kept over a removal or merged in a conflict reached the state
unscreened; and a conflict whose file could not be written was merged into the
state again on every look. Names are judged before any file is touched, what
cannot be read is kept and listed, every state write is screened, a conflict is
written to the file before the state and warns once when it cannot be, a file
changed between reading and writing is not written over, non-UTF-8 text is not
carried, and a node that was away takes a removal it holds no change against
(ADR 0343 §9, as clarified).

modules/claude-code/cmd/claude-code/memory_test.go

@@ -676,3 +676,229 @@ func TestTwoVersionsTooLargeForOneFileAreKeptInTwo(t *testing.T) {
 		t.Fatalf("it is not listed as not carried: %v", not)
 	}
 }
+
+// stateHolds says whether any value of the state holds s.
+func (s *memState) holds(sub string) bool {
+	s.mu.Lock()
+	defer s.mu.Unlock()
+	for _, v := range s.values {
+		var mv MemoryVersion
+		_ = json.Unmarshal(v, &mv)
+		if strings.Contains(mv.Text, sub) {
+			return true
+		}
+	}
+	return false
+}
+
+// putRaw puts a value straight into the state, as a holder of a tool runner's credential could.
+func (s *memState) putRaw(t *testing.T, v MemoryVersion) string {
+	t.Helper()
+	key := MemoryKey(v.Kind, v.Project, v.Path)
+	s.mu.Lock()
+	s.write(key, v)
+	s.mu.Unlock()
+	return key
+}
+
+// Review of #276, 1: a value whose project or path leads out of a memory folder is never taken, and nothing is
+// written for it.
+func TestAValueLeadingOutOfAMemoryFolderIsNeverTaken(t *testing.T) {
+	state, c, g14, shanks := twoNodes(t)
+	root := filepath.Dir(filepath.Dir(shanks.Home))
+	for _, bad := range []struct{ project, path string }{
+		{"../../../escaped", "x.md"}, {"..", "x.md"}, {"a/b", "x.md"}, {"", "x.md"},
+		{project, "../x.md"}, {project, "/abs.md"}, {project, "a//x.md"}, {project, "./x.md"}, {project, "a/../../x.md"},
+	} {
+		text := "planted\n"
+		state.putRaw(t, MemoryVersion{Kind: HomeOperator, Project: bad.project, Path: bad.path, Text: text,
+			Digest: digestOf([]byte(text)), Node: "elsewhere", Changed: c.now})
+		if NameProblem(bad.project, bad.path) == "" {
+			t.Errorf("%q %q is taken as a name", bad.project, bad.path)
+		}
+	}
+	state.pump(t)
+	g14.look(t)
+	shanks.look(t)
+	_ = filepath.WalkDir(root, func(p string, d os.DirEntry, err error) error {
+		if err == nil && !d.IsDir() {
+			if raw, _ := os.ReadFile(p); string(raw) == "planted\n" {
+				t.Errorf("a planted value was written at %s", p)
+			}
+		}
+		return nil
+	})
+	if NameProblem(project, "notes/a.md") != "" || NameProblem(project, "MEMORY.md") != "" {
+		t.Fatal("a memory file's own name is refused")
+	}
+}
+
+// Review of #276, 2: a file or a folder that cannot be read is kept everywhere, never carried as a removal, and is
+// said as not carried.
+func TestWhatCannotBeReadIsNeverTakenForGone(t *testing.T) {
+	if os.Geteuid() == 0 {
+		t.Skip("root reads a file whatever its mode")
+	}
+	state, c, g14, shanks := twoNodes(t)
+	carriedOnBoth(t, state, c, g14, shanks, "MEMORY.md", "index\n")
+	carriedOnBoth(t, state, c, g14, shanks, "notes/deep.md", "deep\n")
+	file, folder := g14.path(project, "MEMORY.md"), filepath.Dir(g14.path(project, "notes/deep.md"))
+	_ = os.Chmod(file, 0)
+	_ = os.Chmod(folder, 0)
+	t.Cleanup(func() { _ = os.Chmod(file, 0o644); _ = os.Chmod(folder, 0o755) })
+	g14.look(t)
+	state.pump(t)
+	for _, f := range []string{"MEMORY.md", "notes/deep.md"} {
+		if _, ok := shanks.read(t, project, f); !ok {
+			t.Errorf("%s, unreadable on g14, was removed on shanks", f)
+		}
+	}
+	not, _ := g14.Status()["not carried"].(map[string]string)
+	if len(not) == 0 {
+		t.Fatalf("nothing is said as not carried: %v", not)
+	}
+	keys, _ := state.Keys()
+	for _, k := range keys {
+		raw, _, _, _ := state.Get(k)
+		var v MemoryVersion
+		_ = json.Unmarshal(raw, &v)
+		if v.Removed {
+			t.Errorf("%s/%s was carried as a removal", v.Project, v.Path)
+		}
+	}
+}
+
+// Review of #276, 3: no text reaches the state without being screened, on the removal and the conflict paths too.
+func TestNoUnscreenedTextReachesTheState(t *testing.T) {
+	token := "ghp_" + strings.Repeat("A1b2C3d4", 5) + "\n"
+	big := strings.Repeat("x", MemoryMostBytes+10) + "\n"
+	for _, text := range []string{token, big} {
+		// The removal path: removed on g14, changed on shanks to what is never carried.
+		state, c, g14, shanks := twoNodes(t)
+		carriedOnBoth(t, state, c, g14, shanks, "note.md", "v1\n")
+		_ = os.Remove(g14.path(project, "note.md"))
+		shanks.write(t, project, "note.md", text, c.now.Add(time.Minute))
+		g14.look(t)
+		state.pump(t)
+		if state.holds(text[:20]) {
+			t.Errorf("the removal path put text that is never carried into the state")
+		}
+		if got, _ := shanks.read(t, project, "note.md"); got != text {
+			t.Errorf("the change on shanks was not kept")
+		}
+		// The conflict path: changed on g14, and on shanks to what is never carried.
+		state, c, g14, shanks = twoNodes(t)
+		carriedOnBoth(t, state, c, g14, shanks, "note.md", "v1\n")
+		g14.write(t, project, "note.md", "v2\n", c.now.Add(time.Minute))
+		shanks.write(t, project, "note.md", text, c.now.Add(2*time.Minute))
+		g14.look(t)
+		state.pump(t)
+		shanks.look(t)
+		state.pump(t)
+		if state.holds(text[:20]) {
+			t.Errorf("the conflict path put text that is never carried into the state")
+		}
+		if got, _ := shanks.read(t, project, "note.md"); got != text {
+			t.Errorf("shanks's own version was not kept as it was")
+		}
+	}
+}
+
+// Review of #276, 4: a conflict whose file cannot be written leaves the state as it was, says so once, and merges
+// once when the folder is writable again.
+func TestAConflictThatCannotBeWrittenIsNotMergedAgainAndAgain(t *testing.T) {
+	if os.Geteuid() == 0 {
+		t.Skip("root writes a folder whatever its mode")
+	}
+	state, c, g14, shanks := twoNodes(t)
+	carriedOnBoth(t, state, c, g14, shanks, "MEMORY.md", "base\n")
+	g14.write(t, project, "MEMORY.md", "base\nfrom g14\n", c.now.Add(time.Minute))
+	shanks.write(t, project, "MEMORY.md", "base\nfrom shanks\n", c.now.Add(2*time.Minute))
+	folder := filepath.Dir(shanks.path(project, "MEMORY.md"))
+	_ = os.Chmod(folder, 0o500)
+	t.Cleanup(func() { _ = os.Chmod(folder, 0o700) })
+	g14.look(t)
+	state.pump(t)
+	for i := 0; i < 5; i++ {
+		_ = shanks.Look()
+		state.pump(t)
+	}
+	if state.holds("<!-- novox mesh") {
+		t.Fatal("a merge that could not be written here reached the state")
+	}
+	unwritten := 0
+	for _, n := range shanks.notices {
+		if strings.HasSuffix(n["key"].(string), ".unwritten") {
+			unwritten++
+		}
+	}
+	if unwritten != 1 {
+		t.Fatalf("%d warnings that the file could not be written", unwritten)
+	}
+	_ = os.Chmod(folder, 0o700)
+	shanks.look(t)
+	state.pump(t)
+	g14.look(t)
+	state.pump(t)
+	for _, n := range []*memNode{g14, shanks} {
+		got, _ := n.read(t, project, "MEMORY.md")
+		if strings.Count(got, "<!-- novox mesh") != 1 || !strings.Contains(got, "from g14") || !strings.Contains(got, "from shanks") {
+			t.Fatalf("%s does not hold both versions once:\n%s", n.Node, got)
+		}
+	}
+}
+
+// Review of #276, minor: a file that changed between its reading and its writing is not written over; text that is
+// not UTF-8 is not carried.
+func TestAFileChangedBeforeItsWritingIsNotWrittenOver(t *testing.T) {
+	state, c, g14, shanks := twoNodes(t)
+	_ = state
+	g14.write(t, project, "a.md", "read\n", c.now)
+	g14.write(t, project, "a.md", "changed meanwhile\n", c.now)
+	err := g14.writeLocal(project, "a.md", []byte("incoming\n"), "replaced", digestOf([]byte("read\n")))
+	if !errors.Is(err, errChangedHere) {
+		t.Fatalf("written over a change: %v", err)
+	}
+	if got, _ := g14.read(t, project, "a.md"); got != "changed meanwhile\n" {
+		t.Fatalf("the change was replaced: %q", got)
+	}
+	g14.write(t, project, "latin1.md", "caf\xe9\n", c.now)
+	g14.look(t)
+	state.pump(t)
+	if _, ok := shanks.read(t, project, "latin1.md"); ok {
+		t.Fatal("text that is not UTF-8 was carried")
+	}
+	not, _ := g14.Status()["not carried"].(map[string]string)
+	if !strings.Contains(not[project+"/latin1.md"], "UTF-8") {
+		t.Fatalf("it is not said why: %v", not)
+	}
+}
+
+// §9: a node that was away takes a removal of a version newer than the one it holds, when it holds no change of
+// its own: the removed file does not come back.
+func TestARemovalReachesANodeThatWasAway(t *testing.T) {
+	state, c, g14, shanks := twoNodes(t)
+	carriedOnBoth(t, state, c, g14, shanks, "old.md", "v1\n")
+	state.watchers = []*MemoryCarrier{g14.MemoryCarrier}
+	c.now = c.now.Add(time.Minute)
+	g14.write(t, project, "old.md", "v2\n", c.now)
+	g14.look(t)
+	state.pump(t)
+	_ = os.Remove(g14.path(project, "old.md"))
+	g14.look(t)
+	state.pump(t)
+	state.watchers = append(state.watchers, shanks.MemoryCarrier)
+	state.replay(t, shanks)
+	shanks.look(t)
+	state.pump(t)
+	g14.look(t)
+	if _, ok := shanks.read(t, project, "old.md"); ok {
+		t.Fatal("the removal did not reach the node that was away")
+	}
+	if _, ok := g14.read(t, project, "old.md"); ok {
+		t.Fatal("the removed file came back")
+	}
+	if !holdsText(shanks.copies(t), "v1\n") {
+		t.Fatal("the version removed on the node that was away was not kept")
+	}
+}