From 08cec8cb1cb711c90457ed40efb45feaf0140d1b Mon Sep 17 00:00:00 2001 From: jochen Date: Wed, 21 Oct 2026 19:35:33 +0200 Subject: [PATCH] Route adopt's refusal-counter reset through fsys, and name the wait in the Stop hook's suggestions (issues 1749, 1757) issue 1749: AdoptName reset both session ids' refusal-counter files with os.WriteFile, bypassing fsys; on a node whose agent has its own account (ADR 0266) the module runs as the operator's account and cannot write into the agent's home directly, so the reset silently failed. writeRefusalCounters now goes through fsys (MkdirAll then WriteFile), as every other write into the agent's home does; it is also called from the Stop hook itself, which already runs as the agent's own account, where fsys is its zero value and behaves exactly as the plain write did before. issue 1757: the Stop hook's suggested watched and said calls left out `by`, which claude_code_wait requires on every op, so a session following the suggestion word for word was refused. Both now carry "by" from the wait's own By field (already kept per wait since issue 1629). The SessionStart hook's suggested adopt call already carried by; added a test for it too, alongside the Stop hook's two, each extracting the exact JSON the hook prints and feeding it back to the tool's own dispatch to prove it is accepted as printed. --- .../cmd/claude-code/hook_calls_test.go | 139 ++++++++++++++++++ modules/claude-code/cmd/claude-code/stop.go | 21 ++- .../claude-code/cmd/claude-code/stop_test.go | 28 ++++ 3 files changed, 181 insertions(+), 7 deletions(-) create mode 100644 modules/claude-code/cmd/claude-code/hook_calls_test.go diff --git a/modules/claude-code/cmd/claude-code/hook_calls_test.go b/modules/claude-code/cmd/claude-code/hook_calls_test.go new file mode 100644 index 00000000..ff3a9b7b --- /dev/null +++ b/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) + } +} diff --git a/modules/claude-code/cmd/claude-code/stop.go b/modules/claude-code/cmd/claude-code/stop.go index 774b9e9a..260dd9a1 100644 --- a/modules/claude-code/cmd/claude-code/stop.go +++ b/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,22 @@ 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 writes the counter file beside the waits file, 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, 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). func writeRefusalCounters(waitsDir, session string, m map[string]int) error { 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(refusalCountersPath(waitsDir, session), raw, 0o600) } // notice says a fail-open or let-through reason on both the log (stderr) and the screen: Claude Code shows a diff --git a/modules/claude-code/cmd/claude-code/stop_test.go b/modules/claude-code/cmd/claude-code/stop_test.go index c975cf30..a8f49cc6 100644 --- a/modules/claude-code/cmd/claude-code/stop_test.go +++ b/modules/claude-code/cmd/claude-code/stop_test.go @@ -273,3 +273,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) + } +}