fix(agent-status): guard the Claude hooks against a spawned agent repainting the spawner's row - #461
Open
x9x9x9x9x9x91 wants to merge 3 commits into
Open
Conversation
…repaint A worker agent spawned from inside a session — a headless `claude -p` from a tool call, a second CLI agent — inherits the spawner's AGTERM_* environment. Its own four hooks therefore address the SPAWNER's row, so the coordinator's session goes green the moment a worker's Stop fires, and blinks whenever a worker submits a prompt or finishes a tool call. The new agterm-claude-status.sh decides ownership from process topology rather than terminal state, and delegates to the generic wrapper only when it holds: the hook is a descendant of the agent that fired it, so exactly one agent binary between the hook and the pane boundary means the firing agent owns the pane, and a second one means another agent spawned it. A tty test cannot answer this in either direction — a headless lane that legitimately owns its pane has none, and a worker under script/expect is handed a fresh pty — whereas the walk passes through a pty wrapper and still finds the spawner above it. The guard fails OPEN, reporting whenever the chain is unreadable or severed (a detached worker whose spawner already exited resolves to one agent): its worst case is the behavior without it, while a false silence would be a bug with no symptom. Names are matched exactly rather than by glob, because `claude*` also matches wrapper scripts such as claude-opus-worker, and node/bun/deno/python argv[0] resolves through argv[1] for runtime-hosted CLIs. The adapter never reads stdin, so a payload another hook wants is left intact; it exits 0 on every path; and the `exec` hands the wrapper its argv verbatim, so --blink, --auto-reset and anything a user appended survive unchanged. The already-installed probe now accepts EITHER path, the adapter or the generic wrapper. Narrowing it to the adapter alone would change the answer it gives for an entry that still names the wrapper — a customized one would read as "not installed" and gain a stock entry beside it, so both would fire and post the row twice. Its owner keeps the setup they edited, unguarded by their own choice, which is the answer they have always had. The shared wrapper is untouched, so Codex, Pi, OpenCode and the recipes keep their behavior byte for byte. bakeAgtermctlPath deliberately skips the new script: it delegates rather than calling agtermctl, so it has no path to bake. The two merge tests that assert the generated entry shape now name the adapter path — their intent is what the installer writes, which is what changed.
…dapter An installer change alone would reach new users only. The merge skips an event whose entries already invoke us, and the Claude side has no refresh path — the one that exists, refreshManagedCodexBlock, is Codex-only — so every settings.json written before the adapter would keep four entries pointing straight at the generic wrapper, and the ownership guard would never reach the installs that have the problem. The merge now rewrites those entries in place first. The match is byte-exact against the command this installer generates for that same event, the quoted wrapper path plus the state, which is what makes the rewrite safe to run over a file someone has edited: an entry carrying an extra flag, a hand-written entry, and a user's own hook that merely mentions the wrapper all fail the comparison and are left untouched. Only the command string is replaced, so the Notification entry keeps its matcher and every sibling key survives. It is idempotent because a rewritten entry names the adapter and no longer matches byte-exactly, so a second run reports no change. An entry the rewrite declines still reads as installed, because the probe accepts the wrapper path too, so it keeps its edited command instead of gaining a stock one beside it.
The other three shipped hook resources each have a suite that runs the real script against a stub; the Claude adapter had none, and its guard is the one claim among them that cannot be checked by reading the file, because it is about the shape of the process tree above it. Each case therefore builds that tree for real: a nested chain of processes carrying the argv[0] the walk will read, with the adapter run underneath the innermost one and a recording stub standing in for the shared wrapper. Covered are the pane's own agent delegating with its argv intact, a spawned worker staying silent, a runtime-hosted agent being counted through argv[1] rather than missed as `node`, the walk stopping at the pane boundary, and no session id exiting 0 with nothing said. Every chain names its own `login` boundary. Without one a fixture would inherit whatever ancestry the runner happens to have, so `swift test` run from inside a Claude session — the case this adapter exists for — would place a real agent above the fixture and turn the owner cases silent: green on CI, red on the machine of anyone reproducing the bug. Naming the boundary makes each case depend on the processes it builds and nothing else.
umputun
reviewed
Aug 19, 2026
umputun
left a comment
umputun
left a comment
Owner
There was a problem hiding this comment.
two things on the ownership walk.
is_agent in agterm/Resources/agent-status/agterm-claude-status.sh:25 covers claude|codex|kimi|opencode|pi, but shell/integration.sh:29 defaults AGTERM_AGENT_RE to gemini|cursor-agent|aider|crush|goose, and its header says claude/codex/pi/opencode are left out of that one because their own hooks drive finer state. The two lists are one set split by detection mechanism, and the walk got half of it. login -> gemini -> claude -p counts a single agent and reports, so the worker's completed --auto-reset turns the gemini pane green mid-run. Same with aider, crush, goose or cursor-agent as the outer agent.
not a regression, the unguarded wrapper did this for every coordinator before. Fix is the five names in the case:
case "$1" in claude | codex | kimi | opencode | pi | gemini | cursor-agent | aider | crush | goose) return 0 ;; esacI checked the reverse direction on a patched copy: login -> claude and login -> zsh -> claude still report, login -> goose -> claude goes silent.
second, ClaudeStatusHookTests has no case for the script/expect topology. That is the one the tty test was rejected over and the claim the walk rests on, so worth pinning: chain ["login","claude","script","claude"] expecting silence. A chain long enough to exhaust the 8-hop bound, expecting delegation, would cover the fail-open half.
umputun
added a commit
that referenced
this pull request
Aug 19, 2026
…view The Claude hook merge round-trips the user's whole settings.json rather than editing the four entries in place, harmless while it only fires on first install. codexWrapperPath is public with no caller outside the module.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The defect from the ownership-guard thread in #456: a worker agent spawned from inside a session — a headless
claude -pfrom a tool call, a second CLI agent — inherits the spawner'sAGTERM_*environment, so its own four hooks address the SPAWNER's row. A coordinator's session goes green the moment a worker's Stop fires, and blinks with every worker tool call.Per the discussion verdict: the guard lives in a Claude-specific adapter, the shared wrapper is untouched byte for byte, and ownership is decided from process topology, never terminal state.
Commit 1 — the adapter.
agterm-claude-status.sh, beside the Codex adapter and following its shape: walks its ppid chain toward the pane boundary counting agent binaries, then delegates argv verbatim to the generic wrapper. Exactly one agent found (its own) → the pane's agent, report; a second → another agent spawned mine, stay silent. The walk passes through pty wrappers —script/expect/unbufferadd a pty but don't break parentage, so the spawner is still found — and a deliberately headless reporter with an exported session id has no agent ancestor, so it still reports. Both directions the rejected tty test got wrong come out right. It fails OPEN whenever the chain is unreadable or severed (a detached worker whose spawner already exited): the worst case is today's behavior, never a silent false-silence. Cost ~25 ms per hook fire on a ~1000-process machine.Commit 2 — the migration, without which this shouldn't merge. An installer change alone reaches fresh installs only:
mergeClaudeSettingsskips an event whose entries already invoke us, and the Claude side has norefreshManagedCodexBlockcounterpart — the same objection that routed the Stop classifier to the cookbook. The merge now first rewrites an entry whose command is byte-exactly what this installer generated for that event, swapping the wrapper path for the adapter path and touching nothing else; a hand-edited entry, an entry with extra flags, and a user hook that merely mentions the wrapper all fail the comparison and are left alone. The idempotency probe accepts either path, so a declined entry still reads as installed and never gains a stock duplicate beside it.Commit 3 — hermetic tests for the shipped script.
ClaudeStatusHookTests(5 tests) builds real process chains withexec -aper level and a recording stub as the wrapper, fixtures wrapped behind aloginboundary — so a maintainer runningswift testfrom inside a Claude session, with a realclaudein the runner's own ancestry, can't have the owner case flipped. Covers owner-delegates-verbatim, nested-worker silence, anode-hosted agent counted through argv[1], the walk stopping at the pane boundary, and the outside-agterm no-op.2605 tests in 97 suites pass;
swiftlint --strictclean. shellcheck reports the same SC1007 false positive the shipped Codex adapter carries on its byte-identicalCDPATH=line; left consistent with the sibling. The uncovered case is named in the shim's header rather than discovered later: a worker that outlives its spawner reparents to launchd and reads as an owner — fails open, andenv -u AGTERM_SESSION_IDstays the advice for deliberate detachers (the tmux paragraph indocs/troubleshooting.mdcould name this carrier too; happy to add the sentence here if wanted).Long-term,
foregroundPidon the tree node (offered in #456) would delete the agent-name table and the severed-chain hole entirely; this PR is the fix that works without an app release.