fix(notify): stop nudging about answered prompts, and rate-limit finished-turn DMs - #173
Conversation
…essages
The idle threshold is a floor, not a rate limit. Once you cross it the condition
stays true for the whole time you are away, so every event passed it
independently — the setting answers "are you away?" and nothing answered "how
often should I interrupt you while you are?"
Diagnosed against a real event log: 1284 records, 96% of them `stop`, with busy
hours running 50 to 56 events. With "also notify on finished turns" on, an hour
away was around fifty DMs rather than the six a ten-minute idle setting suggests.
Finished turns now collapse into one message per 15 minutes, and that message
says what it swallowed ("finished a turn · 7 more while you were away") so a
quiet hour reads as one message about eight turns rather than looking like eight
went missing. The cooldown resets whenever the user is back at the machine, so
the first stop of each absence still arrives promptly instead of being eaten by
a window left running from the previous one. With the idle gate set to Always
there is no "present" to detect and the cooldown simply runs continuously, which
is what that setting asks for.
Permission prompts are deliberately exempt. Each blocks an agent until it is
answered, they are rare — 45 of those 1284 — and repeats of any single one are
already bounded by AttentionPolicy.maxReminders. Throttling them would withhold
precisely the notifications that are actionable.
The burst test asserts conservation rather than a message count: every finished
turn is either reported in a sent message or still pending in the suppressed
count. A count alone would have passed while silently dropping events.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…bout it Approving a plan in the terminal left the notification standing, locally and in Slack, and kept re-nudging about a prompt that had already been answered. The panel treated the existence of notify.sh's FIFO as proof a prompt was still blocking. That holds only while the hook that created it is alive to read a decision, and the cleanup is a trap on EXIT — which bash honours for SIGTERM (verified) but nothing honours for SIGKILL, which is what the agent does to the hook when the user answers in its own UI. The file then outlives the process. The evidence was sitting on disk: 536 leaked FIFO directories going back three months, every single one still holding a live FIFO, so the trap had not run once, and the newest was from yesterday. So liveness is now checked alongside the file. The FIFO answers "was a prompt raised?"; the hook's pid, which now travels with the event, answers "is anyone still listening?". A prompt whose hook has gone cannot be answered from the panel — there is nothing left to read the decision — so it retires within a tick and stops being counted and reminded about. An event from an older notify.sh carries no pid, and falls back to the previous behaviour rather than having its Allow/Deny silently disabled for the first event after an upgrade. Also traps INT/TERM/HUP explicitly and sweeps leaked directories older than 30 minutes, comfortably past the 550s a prompt can live for so one in use is never in range. Neither is load-bearing now that the panel no longer trusts the trap, but 536 leaked FIFOs is its own small problem. Considered and rejected: probing the FIFO with open(O_WRONLY|O_NONBLOCK) and reading ENXIO as "no reader". It needs no new field, but the hook selects on the read end, so a probe opening and closing the write end delivers EOF, the hook exits with an empty decision, and the panel's Allow/Deny stops working — a worse bug than the one being fixed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adversarial review found the throttle's headline property was false end to end. resetStopThrottleIfPresent zeroed suppressedStopCount along with the cooldown, so turns that finished while the user was away and had not been reported yet were discarded rather than folded into the next message. Idle is measured from the last HID event, which makes this easy to hit without ever returning: a stray trackpad bump or a notification click reads as "present", the pending count is wiped, and the next DM claims a bare "finished a turn" as though nothing preceded it. The burst test asserted conservation over throttleStop alone and so never saw it. Only the cooldown is cleared now. Keeping the count is accurate rather than merely safe: it is incremented only past the idle gate in notifySlack, so it can only ever hold turns that genuinely finished while the user was away, however many brief presences the absence is split across. Also records why pid reuse is bounded, since it is the obvious objection to the liveness check: a watch is created with firstSeenAt taken from the event and retired in the same pass once that exceeds promptLifetime, so a reused pid cannot resurrect a stale prompt — the window is 550s from the prompt, not the 30 minute sweep interval, and inside it the worst case is the behaviour this replaced. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Ran an adversarial pass over this before asking anyone else to look. It found one real bug, now fixed in Fixed — the throttle's headline property was false end to end
Easy to hit without ever really returning: idle is measured from the last HID event, so a stray trackpad bump or a notification click reads as "present", the pending count is wiped, and the next DM claims a bare "finished a turn" as though nothing preceded it. The burst test asserted conservation over Only the cooldown is cleared now. Keeping the count is accurate rather than merely safe: it's incremented only past the idle gate in Checked and dismissed
Known gap, not closed hereThe real That's evidence, not coverage. A bash test harness for
|
StuBehan
left a comment
StuBehan
left a comment
There was a problem hiding this comment.
Solid pair of fixes and I think both diagnoses are right - the pid and the fifo answering two different questions is the bit that makes it work, rather than trying to make the trap more reliable.
Pulled it and ran make test-without-xcode, 679 tests / 1775 assertions / 0 failures, same as you got.
Two things I'd want sorting before this merges - the sweep sitting on the hot path, and the suppressed count outliving the absence it belongs to. Rest is questions and nits, flagged as such.
Also on the zombie point in your adversarial pass - it self corrects because node reaps its children more or less straight away, but that's the load bearing bit isn't it? Worth a clause in the comment, a SIGKILLed child still answering kill(0) is exactly the case this whole thing exists for.
…that means it
Nine review threads. The two blockers:
The stale-FIFO sweep ran before post_to_panel, so it sat on the path to the
banner. Measured on a large $TMPDIR: 2.2s cold, i.e. two seconds before the user
learns an agent is blocked, on the one path where latency is the product. It is
backgrounded now — the dir just created is seconds old so can never be in range
of -mmin +30, and the hook lives 550s, so the sweep has all the time it needs.
Keeping the suppressed count on any presence traded one bug for another: a count
from yesterday's absence could be reported on tomorrow's first DM. The
discriminator is how long the presence lasted. A single stray HID event keeps
idle under the threshold for exactly the threshold and not a second more, so a
presence that outlasts it cannot be one event — it takes two, far enough apart,
which is a person. A brief presence now resets only the cooldown; a sustained
one clears what was missed, because the panel has had it on screen throughout.
Also from review:
The hook pid was unvalidated while fifo_path on the same socket was not. Verified
against the real syscall: kill(0, 0) signals the caller's process group and
kill(-1, 0) every process it may signal, both returning 0, so either value made
every prompt read as alive and undid the fix. Worse and unflagged — pid_t is
Int32 and pid_t(4_000_000_000) traps, so an oversized number in the payload
crashed the panel. Both rejected now, with the conversion made non-trapping as
well rather than left one guard away.
A dead prompt kept offering Allow/Deny, where writeFIFO gets ENXIO and returns
silently — a button that does nothing. Now the two are distinguishable, the
affordance goes with the liveness. The check moved to NudgeEvent.isStillBlocking
so the view layer and the controller ask it the same way.
Two comments asserted the belief this PR disproves ("the file existing means
nobody has answered yet", "retire on the FIFO alone"). Corrected. The zombie
window now says why reaping is load-bearing rather than incidental.
Dropped a test assertion that checked a local assigned on the line above, which
could not fail. Fair hit, on a PR arguing for conservation over counting.
README: Always removes the idle gate, not the rate limit; and the limit is
global, so the DM means "go and look" rather than "look here specifically".
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
All nine addressed in 686 tests, 1806 assertions, 0 failures · Both blockers were right, and the sweep one I should have caught myself — I reasoned "it's only a The two I'd single out: The presence discriminator is your design, not mine. I'd fixed "a bump discards the backlog" and created "yesterday's backlog reports tomorrow" without noticing I'd only moved the problem. Using the duration of the presence separates them cleanly, and it's testable in a way my version wasn't — the test now walks the entire life of a single bump and asserts it never once looks sustained, rather than spot-checking either side of a boundary. The pid finding was worse than flagged. You called it "degrades to the old behaviour, not blocking". It doesn't: Also fixed the two comments that asserted the belief this PR disproves — that's the kind of thing that outlives the code and misleads whoever reads it next. On the zombie point from your summary: agreed, and it's in the comment now. It self-corrects because the agent reaps promptly, and that's load-bearing rather than incidental — a SIGKILLed child still answering |
Approving a plan in Claude Code's own UI left stack-nudge reminding about it, locally and in Slack, for about nine more minutes. ExitPlanMode took the blocking permission path: notify.sh created a FIFO and waited on it for a decision. But answering in CC's UI doesn't end that hook, so it ran to its full 550s timeout — and for the whole of that the panel saw a live hook holding a live FIFO, which is exactly its definition of a prompt still waiting. #173 retires a prompt whose hook has died; this one hadn't. ExitPlanMode belongs with AskUserQuestion instead. Both are answered in the agent's own UI, and for both an "allow" written from the panel means "take the default" — here, approving a plan the user hasn't read. With has_action=false there's no FIFO, so panel Enter focuses the editor and no reminder loop starts. Verified against the installed hook: ExitPlanMode now returns in 0s creating no FIFO, AskUserQuestion is unchanged, and a Bash permission still creates its FIFO and blocks. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
#177) * fix(notify): stop nudging about plans already approved in the terminal Approving a plan in Claude Code's own UI left stack-nudge reminding about it, locally and in Slack, for about nine more minutes. ExitPlanMode took the blocking permission path: notify.sh created a FIFO and waited on it for a decision. But answering in CC's UI doesn't end that hook, so it ran to its full 550s timeout — and for the whole of that the panel saw a live hook holding a live FIFO, which is exactly its definition of a prompt still waiting. #173 retires a prompt whose hook has died; this one hadn't. ExitPlanMode belongs with AskUserQuestion instead. Both are answered in the agent's own UI, and for both an "allow" written from the panel means "take the default" — here, approving a plan the user hasn't read. With has_action=false there's no FIFO, so panel Enter focuses the editor and no reminder loop starts. Verified against the installed hook: ExitPlanMode now returns in 0s creating no FIFO, AskUserQuestion is unchanged, and a Bash permission still creates its FIFO and blocks. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(notify): repair FIFO cleanup and don't depend on jq for it Review of the ExitPlanMode change turned up two defects underneath it. The EXIT trap's cleanup has never run. Its body referenced `$fifo`, a local of the function that installed it, and a single-quoted trap body expands when the trap fires — by then the function has returned and the local is gone, so it ran as `rm -f ""` and `rmdir .`. Every permission prompt leaked its FIFO directory until the 30-minute sweep collected it. Verified both ways on the normal completion path: before, the directory survives an answered prompt; after, it's removed. That also invalidates a comment in AttentionPolicy: 536 leaked directories were read as proof the trap had been skipped, when they were proof it had run and done nothing. The conclusion there still holds — a SIGKILLed hook skips the trap outright — but the stated evidence for it did not, so it's corrected. And `is_question_event` bailed out when jq was missing, which meant ExitPlanMode went back on the blocking path and the panel offered Allow on a plan nobody had read. Only macOS 15+ preinstalls jq; post_to_panel already parses the same JSON with python3 for exactly this reason. tool_name now falls back the same way, confirmed to still return ExitPlanMode with jq off PATH. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs: correct the FIFO-cleanup claim the trap fix invalidates The README said the cleanup 'runs on exit' and is only skipped on SIGKILL. It never ran at all: the trap body referenced a local that was out of scope by the time it fired. Says so now, alongside the SIGKILL case which is still true. * fix(notify): correct which cleanup paths were actually broken A reviewer falsified the story I told about the trap, and reproducing it proves them right. Bash defers a trapped signal until the foreground child exits, so on INT/TERM/HUP the handler runs *inside* wait_for_permission_response, where the local is still in scope: SIGTERM while blocked: TRAP fifo=[/tmp/scopetest.../fifo] -> cleaned up clean exit: TRAP fifo=[] -> leaked So the old trap worked on signals and failed only on the clean-exit path — answered or timed-out prompts. "It cleaned up nothing on any prompt" was wrong, and I'd written it into the code comment, the README and AttentionPolicy. All three now say which path was broken. The 536 leaked directories don't settle it either way: SIGKILL runs no trap at all and produces exactly the same evidence. That was the original mistake — an inference the data couldn't support — and I repeated it in the other direction. Also corrects the INT/TERM/HUP comment, which claimed bash runs the EXIT trap for a plain SIGTERM. It does, but not promptly: a trapped signal doesn't terminate bash, so the hook runs on to its 550s timeout and keeps answering kill(0) regardless. The fix itself is unchanged and still correct — the clean-exit path was genuinely leaking, and PERM_FIFO fixes it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
2 participants
Two notification bugs found in daily use, both diagnosed from live evidence on the reporting machine rather than from reading the code.
1. Answering a prompt elsewhere didn't clear it
Symptom: approve a plan in the terminal and the notification stays — in the panel and in Slack — and keeps re-nudging.
Cause: the panel treated "the FIFO file exists" as proof a prompt was still blocking. That holds only while the hook reading it is alive, and
notify.shcleans up with atrap … EXIT— which bash honours forSIGTERM(I tested this) but nothing honours forSIGKILL, which is what the agent does to the hook when you answer in its own UI. The file outlives the process, so the panel kept the prompt in the menu-bar count and fired reminders at the user and at Slack for the full 550s.Evidence: 536 leaked FIFO directories in
$TMPDIRgoing back three months, every one still holding a live FIFO — so the trap had not run once. Newest was from the previous day.Fix: the hook's pid now travels with the event. The FIFO answers "was a prompt raised?"; the pid answers "is anyone still listening?". Both are required, so a prompt whose hook has gone retires within a tick. An event from an older
notify.shcarries no pid and falls back to the previous behaviour, rather than having Allow/Deny silently disabled for the first event after an upgrade.Also traps
INT/TERM/HUPexplicitly and sweeps leaked dirs older than 30 minutes — comfortably past the 550s a prompt can live, so one in use is never in range. Neither is load-bearing now the panel doesn't trust the trap, but 536 leaked FIFOs is its own small problem.2. An hour away was fifty Slack DMs
Cause: the idle threshold is a floor, not a rate limit. Once you cross it the condition stays true for the whole absence, so every event passed it independently. The setting answers "are you away?" and nothing answered "how often should I interrupt you while you are?"
Evidence: the reporting machine's event log — 1284 records, 96% of them
stop, with busy hours running 50–56 events. With "also notify on finished turns" on, an hour away was ~50 DMs, not the six a ten-minute idle setting implies.Fix: finished turns collapse into one message per 15 minutes, and that message says what it swallowed ("finished a turn · 7 more while you were away") so a quiet hour reads as one message about eight turns rather than looking like eight went missing. The cooldown resets while the user is at the machine, so the first stop of each absence still arrives promptly. With the idle gate on
Alwaysthere's no "present" to detect and it runs continuously, which is what that setting asks for.Permission prompts are deliberately exempt — each blocks an agent until answered, they're rare (45 of those 1284), and repeats of a single one are already capped by
maxReminders. Throttling them would withhold precisely the notifications that are actionable.Testing
./build.shclean ·make test-without-xcode→ 678 tests, 1773 assertions, 0 failuresNew coverage is policy-level and injectable (
AttentionPolicy.isAnswerable,SlackDelivery.throttleStop), including:The
findsweep was dry-run against the real 536 directories: all matches our own pattern, nothing else caught.Note for whoever merges
feat/slack-permission-responsesalso modifiesnotifySlack, so whichever lands second needs a small manual resolution.