Fix review round 2 of the wait list and its hooks (issues 1629, 1743, 1744, ADR 0490)

The Stop hook treated a missing waits file exactly like one that could
not be read: a notice on every stop of every session with no waits —
the normal state, not an exceptional one. DecideStop now treats
os.ErrNotExist as the ordinary case of no open waits, silently; the
notice is still said for a file that exists but cannot be read or does
not parse.

ADR 0490 §1 keeps waits under a name, but the file a hook reads is kept
per session id: writing it from only the name an op just acted on meant
a second name bound to the same session id had its waits silently drop
out of enforcement the moment the first name was touched, and adopting
one name away emptied the id's file outright even when another name was
still bound to it. Every write now rewrites a session's file from the
union of every name currently bound to it (sessionWaitsUnion), adopt's
rewrite of the superseded id included.

Also: each atomic write gets its own temp file name, so two writes to
the same path can never interleave on one; adopt clears the refusal
counter of both the superseded and the adopting session, so a fresh
three-strikes count starts under the new binding; and list is read-only
again — it no longer claims an unbound name or refuses a session that
is not (yet) the one bound to it, and instead says who currently holds
the name.

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 {

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
+// 1743/AC4: an unreadable or malformed waits file is allowed, saying so on stderr and as a systemMessage on
-// not touch the network and answers at once.
+// stdout; the decision does not touch the network and answers at once.
-func TestStopAllowsWhenTheWaitsFileCannotBeRead(t *testing.T) {
+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) {

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
+// tempSuffix is a short random name for a temp file, so two writes racing for the same path never share one
-// holds exactly the open waits given and the time of this write (1629/AC3).
+// (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 := readNamedWaits(store, 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
-	})
 	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)}

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
+}