diff --git a/modules/claude-code/cmd/claude-code/stop.go b/modules/claude-code/cmd/claude-code/stop.go index 5e2c5db6..774b9e9a 100644 --- a/modules/claude-code/cmd/claude-code/stop.go +++ b/modules/claude-code/cmd/claude-code/stop.go @@ -13,8 +13,10 @@ package main import ( "encoding/json" + "errors" "fmt" "io" + "io/fs" "os" "path/filepath" "strings" @@ -72,8 +74,12 @@ func watcherAdvice(w Wait) string { // DecideStop is the Stop hook's decision, pure of IO: f and readErr are what reading the session's waits // file gave, previous the refusal counters read beside it, and now the time. Only the first wait that still // blocks the stop is named; the others are read again next time. +// +// A missing file (review round 2, item 1) is the normal state for a session with no waits — every stop of +// every such session, not an exceptional one — so it says nothing and simply allows, the same as a file that +// reads back with no open waits. Only a file that exists but cannot be read, or does not parse, says so. func DecideStop(session string, f WaitsFile, readErr error, previous map[string]int, now time.Time) StopDecision { - if readErr != nil { + if readErr != nil && !errors.Is(readErr, fs.ErrNotExist) { return StopDecision{Reason: fmt.Sprintf("the wait list could not be read (%v); allowed", readErr), Counters: map[string]int{}} } for _, w := range f.Waits { diff --git a/modules/claude-code/cmd/claude-code/stop_test.go b/modules/claude-code/cmd/claude-code/stop_test.go index a9dfcb82..c975cf30 100644 --- a/modules/claude-code/cmd/claude-code/stop_test.go +++ b/modules/claude-code/cmd/claude-code/stop_test.go @@ -7,6 +7,7 @@ import ( "encoding/json" "errors" "fmt" + "io/fs" "os" "path/filepath" "strings" @@ -65,14 +66,17 @@ func TestStopRefusesPastBoundUntilSaidWatchedOrNot(t *testing.T) { } } -// 1743/AC4: a missing, unreadable or malformed waits file is allowed, saying so on stderr; the decision does -// not touch the network and answers at once. -func TestStopAllowsWhenTheWaitsFileCannotBeRead(t *testing.T) { +// 1743/AC4: an unreadable or malformed waits file is allowed, saying so on stderr and as a systemMessage on +// stdout; the decision does not touch the network and answers at once. +func TestStopAllowsAndSaysSoWhenTheWaitsFileIsUnreadableOrMalformed(t *testing.T) { d := DecideStop("sess-4", WaitsFile{}, errors.New("no such file"), map[string]int{}, stopT0) if d.Refuse || !strings.Contains(d.Reason, "could not be read") { t.Fatalf("%+v", d) } - waitsDir := t.TempDir() // no file at all for sess-4b + waitsDir := t.TempDir() + if err := os.WriteFile(filepath.Join(waitsDir, "sess-4b.json"), []byte("{not json"), 0o600); err != nil { + t.Fatal(err) + } var stdout, stderr bytes.Buffer stdin := strings.NewReader(`{"session_id":"sess-4b","stop_hook_active":false}`) start := time.Now() @@ -89,6 +93,25 @@ func TestStopAllowsWhenTheWaitsFileCannotBeRead(t *testing.T) { } } +// Review round 2, item 1 (1743/AC2, ADR 0490 §2): a missing waits file is the normal state for a session +// with no waits — every stop of every such session — so it must print nothing, on either stream, and +// simply allow; a notice there would fire constantly for nothing wrong. +func TestStopPrintsNothingWhenTheWaitsFileIsSimplyMissing(t *testing.T) { + d := DecideStop("sess-4c", WaitsFile{}, fs.ErrNotExist, map[string]int{}, stopT0) + if d.Refuse || d.Reason != "" { + t.Fatalf("a missing file (fs.ErrNotExist) said something: %+v", d) + } + waitsDir := t.TempDir() // no file at all for sess-4d + var stdout, stderr bytes.Buffer + stdin := strings.NewReader(`{"session_id":"sess-4d","stop_hook_active":false}`) + if code := runStop(waitsDir, stdin, &stdout, &stderr); code != 0 { + t.Fatalf("code %d", code) + } + if stdout.Len() != 0 || stderr.Len() != 0 { + t.Fatalf("a missing waits file printed something: stdout %q, stderr %q", stdout.String(), stderr.String()) + } +} + // 1743/AC5: the same stop refused three times running for the same wait is allowed the fourth time, saying // it was left unwatched; the counter resets when the wait changes. func TestStopAllowsTheFourthRefusalInARow(t *testing.T) { diff --git a/modules/claude-code/cmd/claude-code/waits.go b/modules/claude-code/cmd/claude-code/waits.go index f44711ad..3929fc2a 100644 --- a/modules/claude-code/cmd/claude-code/waits.go +++ b/modules/claude-code/cmd/claude-code/waits.go @@ -247,8 +247,18 @@ func updateNamedWaits(store WaitStore, key string, mutate func(NamedWaits) (Name return NamedWaits{}, errors.New("the wait list kept changing elsewhere; try again") } -// writeSessionFile rewrites one session id's mirrored file whole, atomically (temp file, then rename): it -// holds exactly the open waits given and the time of this write (1629/AC3). +// tempSuffix is a short random name for a temp file, so two writes racing for the same path never share one +// (review round 2, item a): each gets its own, and only the rename is a single atomic step. +func tempSuffix() string { + var b [8]byte + _, _ = rand.Read(b[:]) + return hex.EncodeToString(b[:]) +} + +// writeSessionFile rewrites one session id's mirrored file whole, atomically (its own temp file, then +// rename): it holds exactly the open waits given and the time of this write (1629/AC3). waits is always the +// union of every name currently bound to sessionID (sessionWaitsUnion), never only the name an op acted on: +// a second name bound to the same id must stay enforced (ADR 0490 §1, review round 2 item 2). func writeSessionFile(p Paths, sessionID string, waits []Wait, now time.Time) error { path := waitsFilePath(p, sessionID) if err := fsys.MkdirAll(filepath.Dir(path), 0o700); err != nil { @@ -261,13 +271,51 @@ func writeSessionFile(p Paths, sessionID string, waits []Wait, now time.Time) er if err != nil { return err } - tmp := path + ".mesh-tmp" + tmp := path + ".mesh-tmp-" + tempSuffix() if err := fsys.WriteFile(tmp, raw, 0o600); err != nil { return err } return fsys.Rename(tmp, path) } +// sessionWaitsUnion is every open wait of every name on this node currently bound to session (ADR 0490 §1): +// what that session id's mirrored file must hold, whole — never only the one name an op just acted on, or a +// second name bound to the same id would stop being enforced the moment any op touched the first. +func sessionWaitsUnion(store WaitStore, node, session string) ([]Wait, error) { + keys, err := store.Keys() + if err != nil { + return nil, err + } + prefix := node + "/" + var out []Wait + names := make([]string, 0, len(keys)) + for _, k := range keys { + if strings.HasPrefix(k, prefix) { + names = append(names, k) + } + } + sort.Strings(names) // deterministic order: tests, and a session reading its own file, see the same thing + for _, k := range names { + nw, _, found, err := readNamedWaits(store, k) + if err != nil || !found || nw.CurrentSession != session { + continue + } + out = append(out, nw.Waits...) + } + return out, nil +} + +// writeSessionWaits rewrites session's mirrored file with sessionWaitsUnion: every call that changes a +// name's waits or its binding goes through this, never writeSessionFile directly with one name's waits +// alone. +func writeSessionWaits(p Paths, store WaitStore, session string, now time.Time) error { + union, err := sessionWaitsUnion(store, p.Node, session) + if err != nil { + return fmt.Errorf("the session's other names could not be read: %w", err) + } + return writeSessionFile(p, session, union, now) +} + // writeNodeIndex rewrites the node-wide index whole, atomically, from every name's current state on this // node (novox/hq ADR 0490 §2): a full scan of the `waits` state, bounded by how many names are ever used on // one node at once. Best-effort for its caller: a name's own file is already correct by the time this runs. @@ -299,7 +347,7 @@ func writeNodeIndex(p Paths, store WaitStore, now time.Time) error { if err != nil { return err } - tmp := path + ".mesh-tmp" + tmp := path + ".mesh-tmp-" + tempSuffix() if err := fsys.WriteFile(tmp, raw, 0o600); err != nil { return err } @@ -394,7 +442,7 @@ func AddWait(p Paths, in WaitInput, store WaitStore, now time.Time) (map[string] if err != nil { return nil, err } - if err := writeSessionFile(p, session, nw.Waits, now); err != nil { + if err := writeSessionWaits(p, store, session, now); err != nil { return nil, fmt.Errorf("the wait was kept, but its file could not be written: %w", err) } if err := writeNodeIndex(p, store, now); err != nil { @@ -422,7 +470,7 @@ func WatchedWait(p Paths, session, by, id, watcher string, store WaitStore, now return nil, errors.New("watcher is required: what follows this wait (mesh_wait on its family, or a watcher agent)") } key := waitsKey(p, name) - nw, err := updateNamedWaits(store, key, func(nw NamedWaits) (NamedWaits, error) { + _, err = updateNamedWaits(store, key, func(nw NamedWaits) (NamedWaits, error) { if err := bindCaller(&nw, name, p.Node, by, session, now); err != nil { return nw, err } @@ -436,7 +484,7 @@ func WatchedWait(p Paths, session, by, id, watcher string, store WaitStore, now if err != nil { return nil, err } - if err := writeSessionFile(p, session, nw.Waits, now); err != nil { + if err := writeSessionWaits(p, store, session, now); err != nil { return nil, fmt.Errorf("the wait was watched, but its file could not be written: %w", err) } _ = writeNodeIndex(p, store, now) @@ -458,7 +506,7 @@ func DoneWait(p Paths, session, by, id, outcome string, store WaitStore, now tim return nil, errors.New("id is required: the one add answered") } key := waitsKey(p, name) - nw, err := updateNamedWaits(store, key, func(nw NamedWaits) (NamedWaits, error) { + _, err = updateNamedWaits(store, key, func(nw NamedWaits) (NamedWaits, error) { if err := bindCaller(&nw, name, p.Node, by, session, now); err != nil { return nw, err } @@ -472,7 +520,7 @@ func DoneWait(p Paths, session, by, id, outcome string, store WaitStore, now tim if err != nil { return nil, err } - if err := writeSessionFile(p, session, nw.Waits, now); err != nil { + if err := writeSessionWaits(p, store, session, now); err != nil { return nil, fmt.Errorf("the wait was done, but its file could not be written: %w", err) } _ = writeNodeIndex(p, store, now) @@ -496,7 +544,7 @@ func SaidWait(p Paths, session, by, id string, store WaitStore, now time.Time) ( return nil, errors.New("id is required: the one add answered") } key := waitsKey(p, name) - nw, err := updateNamedWaits(store, key, func(nw NamedWaits) (NamedWaits, error) { + _, err = updateNamedWaits(store, key, func(nw NamedWaits) (NamedWaits, error) { if err := bindCaller(&nw, name, p.Node, by, session, now); err != nil { return nw, err } @@ -513,14 +561,17 @@ func SaidWait(p Paths, session, by, id string, store WaitStore, now time.Time) ( if err != nil { return nil, err } - if err := writeSessionFile(p, session, nw.Waits, now); err != nil { + if err := writeSessionWaits(p, store, session, now); err != nil { return nil, fmt.Errorf("the wait was said, but its file could not be written: %w", err) } _ = writeNodeIndex(p, store, now) return map[string]any{"id": id, "said": true}, nil } -// ListWaits answers a name's open waits, with their age and whether they are past their bound (1629/AC1). +// ListWaits answers a name's open waits, with their age and whether they are past their bound (1629/AC1), +// and who currently holds the name. Read-only (review round 2, item c): unlike every other op it never +// claims an unbound name, and never refuses a session that is not (or not yet) the one bound to it — a +// session may want to look before adopting. func ListWaits(p Paths, session, by string, store WaitStore, now time.Time) (map[string]any, error) { session, err := requireSessionID(session) if err != nil { @@ -530,15 +581,9 @@ func ListWaits(p Paths, session, by string, store WaitStore, now time.Time) (map if err != nil { return nil, err } - key := waitsKey(p, name) - nw, err := updateNamedWaits(store, key, func(nw NamedWaits) (NamedWaits, error) { - if err := bindCaller(&nw, name, p.Node, by, session, now); err != nil { - return nw, err - } - return nw, nil - }) + nw, _, _, err := readNamedWaits(store, waitsKey(p, name)) if err != nil { - return nil, err + return nil, fmt.Errorf("the wait list could not be read: %w", err) } out := []map[string]any{} for _, w := range nw.Waits { @@ -553,7 +598,11 @@ func ListWaits(p Paths, session, by string, store WaitStore, now time.Time) (map } out = append(out, entry) } - return map[string]any{"session": session, "name": name, "open": out}, nil + answer := map[string]any{"session": session, "name": name, "open": out} + if nw.CurrentSession != "" { + answer["held-by"] = map[string]any{"session": nw.CurrentSession, "last-called": nw.LastCall.UTC().Format(time.RFC3339)} + } + return answer, nil } // AdoptName binds name to session, always taking it over from whoever held it (novox/hq ADR 0490 §3): the @@ -582,16 +631,25 @@ func AdoptName(p Paths, by, session string, store WaitStore, now time.Time) (map } tookOver := oldSession != "" && oldSession != session if tookOver { - if err := writeSessionFile(p, oldSession, nil, now); err != nil { + // oldSession's file is rewritten from the union of whatever names remain bound to it (ADR 0490 §1, + // review round 2 item 2): name no longer is one of them, but another might still be — never simply + // emptied. + if err := writeSessionWaits(p, store, oldSession, now); err != nil { return nil, fmt.Errorf("%s was adopted, but the session it was taken from could not be told: %w", name, err) } } - if err := writeSessionFile(p, session, nw.Waits, now); err != nil { + if err := writeSessionWaits(p, store, session, now); err != nil { return nil, fmt.Errorf("the name was adopted, but its file could not be written: %w", err) } if err := writeNodeIndex(p, store, now); err != nil { say("writing the node-wide wait index after %s was adopted: %v", name, err) } + // Review round 2, item b: a fresh three-strikes count starts under the new binding, on both ids. + waitsDir := WaitsDirPath(p) + _ = writeRefusalCounters(waitsDir, session, map[string]int{}) + if tookOver { + _ = writeRefusalCounters(waitsDir, oldSession, map[string]int{}) + } answer := map[string]any{"name": name, "session": session, "open": len(nw.Waits)} if tookOver { answer["took-over-from"] = map[string]any{"session": oldSession, "last-called": oldLastCall.UTC().Format(time.RFC3339)} diff --git a/modules/claude-code/cmd/claude-code/waits_test.go b/modules/claude-code/cmd/claude-code/waits_test.go index cfcafe26..a86f6f3b 100644 --- a/modules/claude-code/cmd/claude-code/waits_test.go +++ b/modules/claude-code/cmd/claude-code/waits_test.go @@ -322,14 +322,20 @@ func TestADifferentSessionIsRefusedWithoutAdopting(t *testing.T) { by := "fix-1629@g14" addApproveWait(t, p, store, "fix-1629", "sess-old", t0) - if _, err := ListWaits(p, "sess-new", by, store, t0.Add(time.Minute)); err == nil || - !strings.Contains(err.Error(), "sess-old") || !strings.Contains(err.Error(), "adopt") { - t.Fatalf("a different session was not refused, or did not name the one it holds to: %v", err) - } if _, err := AddWait(p, WaitInput{Session: "sess-new", By: by, What: "another", Read: WaitRead{Address: "mesh-delivery.recheck"}, Bound: 10}, store, t0.Add(time.Minute)); err == nil || !strings.Contains(err.Error(), "sess-old") { t.Fatalf("add from a different session was not refused: %v", err) } + + // list, read-only (review round 2 item c), never refuses: it just says who holds the name. + out, err := ListWaits(p, "sess-new", by, store, t0.Add(time.Minute)) + if err != nil { + t.Fatalf("list from a different session was refused: %v", err) + } + held, _ := out["held-by"].(map[string]any) + if held == nil || held["session"] != "sess-old" { + t.Fatalf("list did not say who holds the name: %v", out) + } } // ADR 0490 §2/§3: adopt right after a simulated /clear binds the name to the new session id at once; the @@ -397,7 +403,8 @@ func TestTheEarlierIDIsToldOnItsNextCall(t *testing.T) { if _, err := AdoptName(p, by, "sess-new", store, t0.Add(time.Minute)); err != nil { t.Fatal(err) } - _, err := ListWaits(p, "sess-old", by, store, t0.Add(2*time.Minute)) + _, err := AddWait(p, WaitInput{Session: "sess-old", By: by, What: "another", Read: WaitRead{Address: "mesh-delivery.recheck"}, + Bound: 10}, store, t0.Add(2*time.Minute)) if err == nil || !strings.Contains(err.Error(), "sess-new") || !strings.Contains(err.Error(), "adopt") { t.Fatalf("the earlier id's next call did not name the new session: %v", err) } @@ -457,3 +464,114 @@ func TestAdoptIsServedThroughTheModulesDispatch(t *testing.T) { t.Fatalf("adopt: %v %s", isErr, text) } } + +// ---- Review round 2: a session id's mirrored file is the union of every name bound to it ----------------- + +// Review round 2, item 2 (ADR 0490 §1): one session id with two different names bound to it holds both +// names' waits in its one mirrored file, not only the name an op last acted on. +func TestASessionsFileHoldsEveryNameBoundToIt(t *testing.T) { + p, _ := node(t, "g14") + store := newMemWaits() + idX := addApproveWait(t, p, store, "fix-x", "sess-shared", t0) + idY := addApproveWait(t, p, store, "fix-y", "sess-shared", t0.Add(time.Minute)) + + f, err := readWaitsFile(waitsFilePath(p, "sess-shared")) + if err != nil { + t.Fatal(err) + } + ids := map[string]bool{} + for _, w := range f.Waits { + ids[w.ID] = true + } + if len(f.Waits) != 2 || !ids[idX] || !ids[idY] { + t.Fatalf("the session's file does not hold both names' waits: %+v", f) + } +} + +// Review round 2, item 2: adopting one of two names bound to the same session away keeps that session's +// file holding the other name's waits — it is never emptied wholesale. +func TestAdoptingOneNameAwayKeepsTheSessionsOtherNamesWaits(t *testing.T) { + p, _ := node(t, "g14") + store := newMemWaits() + idX := addApproveWait(t, p, store, "fix-x", "sess-shared", t0) + idY := addApproveWait(t, p, store, "fix-y", "sess-shared", t0.Add(time.Minute)) + + if _, err := AdoptName(p, "fix-y@g14", "sess-y-new", store, t0.Add(2*time.Minute)); err != nil { + t.Fatal(err) + } + + sharedFile, err := readWaitsFile(waitsFilePath(p, "sess-shared")) + if err != nil || len(sharedFile.Waits) != 1 || sharedFile.Waits[0].ID != idX { + t.Fatalf("sess-shared should still hold fix-x's wait: %+v %v", sharedFile, err) + } + newFile, err := readWaitsFile(waitsFilePath(p, "sess-y-new")) + if err != nil || len(newFile.Waits) != 1 || newFile.Waits[0].ID != idY { + t.Fatalf("sess-y-new should hold fix-y's wait: %+v %v", newFile, err) + } +} + +// Review round 2, item b: adopt clears the refusal counter of both the earlier id and the new one, so a +// fresh three-strikes count starts under the new binding. +func TestAdoptClearsTheRefusalCounterOfBothIDs(t *testing.T) { + p, _ := node(t, "g14") + store := newMemWaits() + by := "fix-1629@g14" + addApproveWait(t, p, store, "fix-1629", "sess-old", t0) + waitsDir := WaitsDirPath(p) + if err := writeRefusalCounters(waitsDir, "sess-old", map[string]int{"w-1": 2}); err != nil { + t.Fatal(err) + } + if err := writeRefusalCounters(waitsDir, "sess-new", map[string]int{"w-2": 3}); err != nil { + t.Fatal(err) + } + if _, err := AdoptName(p, by, "sess-new", store, t0.Add(time.Minute)); err != nil { + t.Fatal(err) + } + if c := readRefusalCounters(waitsDir, "sess-old"); len(c) != 0 { + t.Errorf("sess-old's counter was not cleared: %v", c) + } + if c := readRefusalCounters(waitsDir, "sess-new"); len(c) != 0 { + t.Errorf("sess-new's counter was not cleared: %v", c) + } +} + +// Review round 2, item c: list is read-only — it never binds an unbound name, and never refuses a +// mismatched session; it answers the name's waits and who currently holds it. +func TestListNeverBindsAndAnswersWhoHoldsTheName(t *testing.T) { + p, _ := node(t, "g14") + store := newMemWaits() + by := "fix-1629@g14" + + // An unbound name: list answers no waits, and binds nothing (a later add from a different session still + // succeeds, as the first caller for an unbound name). + out, err := ListWaits(p, "sess-reader", by, store, t0) + if err != nil || len(out["open"].([]map[string]any)) != 0 { + t.Fatalf("an unbound name: %v %v", out, err) + } + if _, found, _ := readSessionBinding(store, waitsKey(p, "fix-1629")); found { + t.Fatal("list bound an unbound name") + } + if _, err := AddWait(p, WaitInput{Session: "sess-first", By: by, What: "x", Read: WaitRead{Address: "mesh-delivery.recheck"}, + Bound: 10}, store, t0); err != nil { + t.Fatalf("add after a read-only list: %v", err) + } + + // A name bound to another session: list still answers (never refused), and says who holds it. + out, err = ListWaits(p, "sess-other", by, store, t0.Add(time.Minute)) + if err != nil { + t.Fatalf("list for a name held by another session was refused: %v", err) + } + if len(out["open"].([]map[string]any)) != 1 { + t.Fatalf("list did not answer the name's waits: %v", out) + } + held, _ := out["held-by"].(map[string]any) + if held == nil || held["session"] != "sess-first" { + t.Fatalf("list did not say who holds the name: %v", out) + } +} + +// readSessionBinding is a thin test helper reading the raw NamedWaits at key, without claiming it. +func readSessionBinding(store WaitStore, key string) (NamedWaits, bool, error) { + nw, _, found, err := readNamedWaits(store, key) + return nw, found, err +}