Merge pull request 'Name the wait in the Stop hook's suggested calls, route adopt's counter reset through fsys, and stop writing empty counter files' (#421) from fix/1749-1757-wait-list into main

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

@@ -0,0 +1,139 @@
+package main
+
+// Every call a hook prints for a session to copy must be accepted by claude_code_wait's own argument check
+// (issue 1757): the Stop hook's suggested watched and said calls, and the SessionStart hook's suggested
+// adopt call.
+
+import (
+	"encoding/json"
+	"strings"
+	"testing"
+	"time"
+)
+
+// suggestedCall extracts the one JSON object embedded in a hook's message, right after "claude_code_wait "
+// — what a session is expected to copy verbatim — and parses it into the arguments a tool call takes.
+func suggestedCall(t *testing.T, msg string) map[string]any {
+	t.Helper()
+	i := strings.Index(msg, "claude_code_wait ")
+	if i < 0 {
+		t.Fatalf("no suggested call in: %s", msg)
+	}
+	rest := msg[i+len("claude_code_wait "):]
+	start := strings.Index(rest, "{")
+	if start < 0 {
+		t.Fatalf("no JSON object in: %s", msg)
+	}
+	depth, end := 0, -1
+	for j, c := range rest[start:] {
+		switch c {
+		case '{':
+			depth++
+		case '}':
+			depth--
+			if depth == 0 {
+				end = start + j + 1
+			}
+		}
+		if end >= 0 {
+			break
+		}
+	}
+	if end < 0 {
+		t.Fatalf("unbalanced JSON object in: %s", msg)
+	}
+	var args map[string]any
+	if err := json.Unmarshal([]byte(rest[start:end]), &args); err != nil {
+		t.Fatalf("the suggested call is not valid JSON: %v (%s)", err, rest[start:end])
+	}
+	return args
+}
+
+// Issue 1757: the Stop hook's suggested watched call carries `by`, and the tool accepts it as printed.
+func TestTheStopHooksSuggestedWatchedCallIsAcceptedByTheTool(t *testing.T) {
+	p, _ := node(t, "g14")
+	store := newMemWaits()
+	waiting = &Waiting{Store: store, Now: func() time.Time { return t0 }}
+	t.Cleanup(func() { waiting = nil })
+	s := serve(t, p, &bus{kept: map[string]map[string]any{}}, memConfig{})
+
+	text, isErr := s.call(t, "claude_code_wait", map[string]any{"op": "add", "session": "verify-0489-test", "by": "fix-1757@g14",
+		"what": "the operator's Approve", "read": map[string]any{"address": "mesh-delivery.recheck"}, "bound_minutes": 10})
+	if isErr {
+		t.Fatalf("add: %s", text)
+	}
+
+	f, err := readWaitsFile(waitsFilePath(p, "verify-0489-test"))
+	if err != nil || len(f.Waits) != 1 {
+		t.Fatalf("%+v %v", f, err)
+	}
+	_, reason := waitBlocksStop("verify-0489-test", f.Waits[0], t0.Add(time.Minute))
+	if !strings.Contains(reason, "\"by\":\"fix-1757\"") {
+		t.Fatalf("the suggested call does not carry by: %s", reason)
+	}
+	args := suggestedCall(t, reason)
+
+	text, isErr = s.call(t, "claude_code_wait", args)
+	if isErr {
+		t.Fatalf("the suggested watched call, as printed, was refused: %s", text)
+	}
+}
+
+// Issue 1757: the Stop hook's suggested said call (past its bound) carries `by` too.
+func TestTheStopHooksSuggestedSaidCallIsAcceptedByTheTool(t *testing.T) {
+	p, _ := node(t, "g14")
+	store := newMemWaits()
+	clock := t0
+	waiting = &Waiting{Store: store, Now: func() time.Time { return clock }}
+	t.Cleanup(func() { waiting = nil })
+	s := serve(t, p, &bus{kept: map[string]map[string]any{}}, memConfig{})
+
+	text, isErr := s.call(t, "claude_code_wait", map[string]any{"op": "add", "session": "verify-0489-test", "by": "fix-1757@g14",
+		"what": "the operator's Approve", "read": map[string]any{"address": "mesh-delivery.recheck"}, "bound_minutes": 1})
+	if isErr {
+		t.Fatalf("add: %s", text)
+	}
+
+	clock = t0.Add(2 * time.Minute) // past its 1-minute bound, for the live tool call too
+	f, err := readWaitsFile(waitsFilePath(p, "verify-0489-test"))
+	if err != nil || len(f.Waits) != 1 {
+		t.Fatalf("%+v %v", f, err)
+	}
+	_, reason := waitBlocksStop("verify-0489-test", f.Waits[0], clock)
+	if !strings.Contains(reason, "\"by\":\"fix-1757\"") {
+		t.Fatalf("the suggested call does not carry by: %s", reason)
+	}
+	args := suggestedCall(t, reason)
+
+	text, isErr = s.call(t, "claude_code_wait", args)
+	if isErr {
+		t.Fatalf("the suggested said call, as printed, was refused: %s", text)
+	}
+}
+
+// Issue 1757: the SessionStart hook's suggested adopt call already carries `by` — checked, as asked, so a
+// regression here is caught the same way as the Stop hook's.
+func TestTheSessionStartHooksSuggestedAdoptCallIsAcceptedByTheTool(t *testing.T) {
+	p, _ := node(t, "g14")
+	store := newMemWaits()
+	waiting = &Waiting{Store: store, Now: func() time.Time { return t0 }}
+	t.Cleanup(func() { waiting = nil })
+	s := serve(t, p, &bus{kept: map[string]map[string]any{}}, memConfig{})
+
+	text, isErr := s.call(t, "claude_code_wait", map[string]any{"op": "add", "session": "sess-old", "by": "fix-1757@g14",
+		"what": "the operator's Approve", "read": map[string]any{"address": "mesh-delivery.recheck"}, "bound_minutes": 10})
+	if isErr {
+		t.Fatalf("add: %s", text)
+	}
+
+	msg := SessionStartMessage("sess-new", WaitsDirPath(p), t0.Add(time.Minute))
+	if !strings.Contains(msg, "\"by\":\"fix-1757@g14\"") {
+		t.Fatalf("the suggested adopt call does not carry by: %s", msg)
+	}
+	args := suggestedCall(t, msg)
+
+	text, isErr = s.call(t, "claude_code_wait", args)
+	if isErr {
+		t.Fatalf("the suggested adopt call, as printed, was refused: %s", text)
+	}
+}

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

@@ -52,12 +52,12 @@ func waitBlocksStop(session string, w Wait, now time.Time) (bool, string) {
 		}
 		return true, fmt.Sprintf("wait %s (%s) is open and nothing follows it yet. Read where it stands with %s, then "+
 			"%s, and mark it watched: claude_code_wait "+
-			"{\"op\":\"watched\",\"session\":%q,\"id\":%q,\"watcher\":\"...\"}.", w.ID, w.What, w.Read.Address, watcherAdvice(w),
-			session, w.ID)
+			"{\"op\":\"watched\",\"session\":%q,\"by\":%q,\"id\":%q,\"watcher\":\"...\"}.", w.ID, w.What, w.Read.Address,
+			watcherAdvice(w), session, w.By, w.ID)
 	}
 	return true, fmt.Sprintf("wait %s (%s) is past its bound and the operator has not been told. Tell the operator what "+
 		"waits, since when, and what you are doing about it, then mark it: claude_code_wait "+
-		"{\"op\":\"said\",\"session\":%q,\"id\":%q}.", w.ID, w.What, session, w.ID)
+		"{\"op\":\"said\",\"session\":%q,\"by\":%q,\"id\":%q}.", w.ID, w.What, session, w.By, w.ID)
 }
 
 // watcherAdvice is what a session is told to do to follow a wait (ADR 0489 §3, review round 1 item 3): a
@@ -113,15 +113,34 @@ func readRefusalCounters(waitsDir, session string) map[string]int {
 	return m
 }
 
-// writeRefusalCounters writes the counter file beside the waits file. Its caller decides what an error here
-// means: harmless when the stop is already allowed, but when the stop was about to be refused, a cap that
-// cannot be written is a cap that cannot take effect (ADR 0489 §5 "never loops", review round 1 item 2).
+// writeRefusalCounters keeps the counter file beside the waits file in step with m, through fsys like every
+// other write into the agent's home (novox/hq ADR 0266, issue 1749): called from the Stop hook, which
+// already runs as the agent's own account (fsys is then its zero value, a plain write or remove, as
+// before), and from the tool's AdoptName, which runs as the operator's account and must reach a confined
+// node's agent home through the sudo helper fsys carries such a write through. Its caller decides what an
+// error here means: harmless when the stop is already allowed, but when the stop was about to be refused, a
+// cap that cannot be written is a cap that cannot take effect (ADR 0489 §5 "never loops", review round 1
+// item 2).
+//
+// Empty, m is removed rather than written as "{}" (issue 1749, follow-up): a stop with nothing to record —
+// every stop of every session with no waits, the ordinary case — must leave neither the file nor, the first
+// time, the waits folder itself; removing a file that was never there is not an error.
 func writeRefusalCounters(waitsDir, session string, m map[string]int) error {
+	path := refusalCountersPath(waitsDir, session)
+	if len(m) == 0 {
+		if err := fsys.Remove(path); err != nil && !errors.Is(err, fs.ErrNotExist) {
+			return err
+		}
+		return nil
+	}
 	raw, err := json.Marshal(m)
 	if err != nil {
 		return err
 	}
-	return os.WriteFile(refusalCountersPath(waitsDir, session), raw, 0o600)
+	if err := fsys.MkdirAll(waitsDir, 0o700); err != nil {
+		return err
+	}
+	return fsys.WriteFile(path, raw, 0o600)
 }
 
 // notice says a fail-open or let-through reason on both the log (stderr) and the screen: Claude Code shows a

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

@@ -170,13 +170,72 @@ func TestRunStopKeepsItsCounterBesideTheWaitsFile(t *testing.T) {
 	if !strings.Contains(stdout.String(), "systemMessage") || !strings.Contains(stdout.String(), "unwatched") {
 		t.Fatalf("the let-through notice did not reach stdout: %s", stdout.String())
 	}
-	raw, err := os.ReadFile(refusalCountersPath(waitsDir, "sess-6"))
+	// Once let through, the counters are empty again: the file is removed, not left holding "{}" (issue
+	// 1749, follow-up).
+	if _, err := os.Stat(refusalCountersPath(waitsDir, "sess-6")); !os.IsNotExist(err) {
+		t.Fatalf("the counter file was not removed once its counters emptied: %v", err)
+	}
+}
+
+// Issue 1749, follow-up: a stop for a session with no waits at all leaves nothing behind — not the counter
+// file, not even the waits folder a node that never added a wait would otherwise gain on every stop of
+// every session.
+func TestStopWithNoWaitsLeavesNoFolderAndNoFile(t *testing.T) {
+	waitsDir := filepath.Join(t.TempDir(), "waits") // not created: this is what the hook is handed cold
+	var stdout, stderr bytes.Buffer
+	stdin := strings.NewReader(`{"session_id":"sess-none","stop_hook_active":false}`)
+	if code := runStop(waitsDir, stdin, &stdout, &stderr); code != 0 {
+		t.Fatalf("code %d, stderr %s", code, stderr.String())
+	}
+	if _, err := os.Stat(waitsDir); !os.IsNotExist(err) {
+		t.Fatalf("a stop with no waits created the waits folder: %v", err)
+	}
+}
+
+// Issue 1749, follow-up: a refusal writes the counter file (there is something to record).
+func TestStopWithARefusalWritesTheCounterFile(t *testing.T) {
+	waitsDir := t.TempDir()
+	f := WaitsFile{Session: "sess-7", At: stopT0, Waits: []Wait{openWait("w-7", 10)}}
+	writeWaitsFixture(t, filepath.Join(waitsDir, "sess-7.json"), f)
+	var stdout, stderr bytes.Buffer
+	stdin := strings.NewReader(`{"session_id":"sess-7","stop_hook_active":false}`)
+	if code := runStop(waitsDir, stdin, &stdout, &stderr); code != 2 {
+		t.Fatalf("code %d, stderr %s", code, stderr.String())
+	}
+	raw, err := os.ReadFile(refusalCountersPath(waitsDir, "sess-7"))
 	if err != nil {
 		t.Fatal(err)
 	}
 	var counters map[string]int
-	if json.Unmarshal(raw, &counters) != nil || len(counters) != 0 {
-		t.Fatalf("the counter file was not reset: %s", raw)
+	if json.Unmarshal(raw, &counters) != nil || counters["w-7"] != 1 {
+		t.Fatalf("the counter file: %s", raw)
+	}
+}
+
+// Issue 1749, follow-up: once the wait is done (no longer in the waits file), the next stop removes the
+// counter file rather than leaving a stale "{}" behind.
+func TestStopAfterTheWaitIsDoneRemovesTheCounterFile(t *testing.T) {
+	waitsDir := t.TempDir()
+	path := filepath.Join(waitsDir, "sess-8.json")
+	writeWaitsFixture(t, path, WaitsFile{Session: "sess-8", At: stopT0, Waits: []Wait{openWait("w-8", 10)}})
+	stdin := strings.NewReader(`{"session_id":"sess-8","stop_hook_active":false}`)
+	var stdout, stderr bytes.Buffer
+	if code := runStop(waitsDir, stdin, &stdout, &stderr); code != 2 {
+		t.Fatalf("refusal: code %d, stderr %s", code, stderr.String())
+	}
+	if _, err := os.Stat(refusalCountersPath(waitsDir, "sess-8")); err != nil {
+		t.Fatalf("the counter file was not written: %v", err)
+	}
+
+	// The wait is done: it is no longer in the waits file (DoneWait removes it, waits.go).
+	writeWaitsFixture(t, path, WaitsFile{Session: "sess-8", At: stopT0, Waits: []Wait{}})
+	var stdout2, stderr2 bytes.Buffer
+	stdin2 := strings.NewReader(`{"session_id":"sess-8","stop_hook_active":false}`)
+	if code := runStop(waitsDir, stdin2, &stdout2, &stderr2); code != 0 {
+		t.Fatalf("after done: code %d, stderr %s", code, stderr2.String())
+	}
+	if _, err := os.Stat(refusalCountersPath(waitsDir, "sess-8")); !os.IsNotExist(err) {
+		t.Fatalf("the counter file was not removed after the wait was done: %v", err)
 	}
 }
 
@@ -273,3 +332,31 @@ func TestHookRegisterStillRefusesEveryHook(t *testing.T) {
 		t.Fatalf("a Stop hook was not refused: %v %s", isError, text)
 	}
 }
+
+// Issue 1749: adopt's refusal-counter reset writes into the agent's home (beside the waits file) and must
+// go through fsys like every other such write (novox/hq ADR 0266) — on a confined node the module runs as
+// the operator's account, which cannot write there directly; only the helper, as the agent's own account,
+// can. The confined-node fake (agentFiles, agent_account_test.go) proves it: a write that reaches the real
+// filesystem only through the helper's request/response protocol bumps its count.
+func TestWriteRefusalCountersGoesThroughTheHelper(t *testing.T) {
+	f, home, counted := agentFiles(t)
+	was := fsys
+	fsys = f
+	t.Cleanup(func() { fsys = was })
+
+	waitsDir := filepath.Join(home, ".claude", "nox-mesh", "waits")
+	if err := writeRefusalCounters(waitsDir, "sess-1", map[string]int{"w-1": 2}); err != nil {
+		t.Fatal(err)
+	}
+	if counted.asked.Load() == 0 {
+		t.Fatal("the counter file was written directly, never through the helper")
+	}
+	raw, err := os.ReadFile(filepath.Join(waitsDir, "sess-1.refusals.json"))
+	if err != nil {
+		t.Fatal(err)
+	}
+	var m map[string]int
+	if json.Unmarshal(raw, &m) != nil || m["w-1"] != 2 {
+		t.Fatalf("the written counters: %s", raw)
+	}
+}