[qingyun-wu]

What happened

On one host, a Claude core's standby task-notifier.sh ran at 60–90% CPU for about 9 hours. TERM didn't stop it; it needed SIGKILL. Its parent, the shared task-notifier-supervisor.sh, was 3+ days old and no longer belonged to any live tmux session.

Why

The busy loop. The notifier reads its standby watcher's announcements from a FIFO with read -t.

• On macOS's bash 3.2, EOF and a timeout both return 1, so the loop told them apart only by kill -0 "$watcher_pid".
• A watcher that has closed its stdout but not finished exiting still answers kill -0: it may be mid-cleanup, or not yet reaped.
• So every read returned EOF at once and the loop went round with no sleep, forking ls and friends on each pass.
• The watcher's own read loop already guards against exactly this ("a writer mid-exit answers kill -0 too"). The notifier loop didn't. The codex notifier has the same loop.

TERM looking ignored. exit 143 runs the EXIT trap, whose stop_watcher sent TERM and then waited, with no bound, on that same stuck watcher. So the notifier never finished exiting.

Supervisors outliving their sessions. When a launcher replaces a <session>-watcher tmux session, the supervisor's HUP/TERM trap runs stop_child, the same unbounded TERM-then-wait. A notifier that doesn't exit therefore kept its supervisor alive past its session. That's the 3+-day orphan. On the host, every other supervisor maps one-to-one to a live watcher session: one for the core and one per pool worker, by design.

"A codex supervisor running a claude notifier" is by design. src/agent/codex/cli/task-notifier-supervisor.sh is the shared supervisor for every runtime, and SUTANDO_NOTIFIER_SCRIPT selects the notifier it runs. Only its path is codex.

The fix

• src/notifier-guard.sh (new), sourced by both notifiers and the supervisor:
  • events_stream_ended: the stream has ended when its writer is gone or a zombie, or when a timed read returned well before its timeout. That return is EOF, and no writer ever reopens this FIFO.
  • stop_group_bounded: TERM the group, wait up to 3 s of wall time (polling with the kill -0 builtin, not a fork per poll), then KILL.
• Both notifiers:
  • On an ended stream, stop and log why ("closed its events pipe but is still running").
  • A non-zero read that took 0 s backs off 1 s before going round again. With a timeout under 2 s, time can't tell EOF from a timeout, so this pause is what prevents a spin there.
  • stop_watcher is bounded.
• The supervisor:
  • stop_child is bounded.
  • A notifier that dies within 30 s of starting is restarted with doubling backoff, capped at 60 s. A fractional RESTART_DELAY is used as given.
  • One supervisor per inbox: a pidfile under the inbox workspace's state/notifier-supervisors/. A new supervisor replaces an older live one for the same inbox, and that one's notifier children with it. It acts on a recorded pid only when that process still runs this script for this inbox.
• health-check: a new helper-cpu probe (src/helper_cpu_guard.py) names any Sutando helper averaging over 50% CPU for 10+ minutes, by pid and script.
  • A helper is an interpreter running a script from this repo; Claude CLI sessions don't count.
  • It keeps one cumulative-CPU sample per process across runs, keyed by pid and start time, and judges the rate once a sample is at least 10 minutes old.
• notifier-guard.sh joins both launchers' notifier version fingerprints, so a change to it replaces the running sessions as other notifier changes do.

Tests

tests/notifier-spin-guard.test.py (new, 17 cases) runs the real scripts against a fake repo whose watch-tasks-stream.sh is a stub:

• Each notifier, given a watcher that closes its pipe and lingers ignoring TERM, exits by itself and the watcher is gone.
• Each notifier, given a healthy watcher deaf to TERM, exits on TERM within bounded time and stops it.
• The guard helpers on their own.
• The supervisor: a bounded stop_child, the backoff sequence (including a fractional delay), replacement of an older supervisor for the same inbox, and a recorded pid that isn't a supervisor being left alone.
• The helper-cpu probe: parsing, helper detection, the window, a reused pid, a gone process, and its wiring into health-check.

Against main, 8 of the 17 fail: both notifier cases of each kind, the bounded stop_child (it hangs), the single-slot replacement, the backoff sequence and the probe's wiring. With the fix, all pass.

tests/codex-core-launcher.test.py: its fixture now copies src/notifier-guard.sh, and its copy of the version formula includes it.

Existing notifier, supervisor, launcher and health-check tests:

• 48 of 50 pass on this branch.
• The other two, start-cli-claude-config-dir and start-cli-model-pin, fail identically on main on the same host.
• tests/codex-core-launcher.test.py: its failures on this host are environmental, and the set is identical on main and on this branch (none only on the branch).

Reproduction

The spin came from a watcher stuck mid-exit. To reproduce, start a notifier against a stub watcher that runs trap '' TERM; exec 1>&- 9>&-; while :; do sleep 1; done.

• On main, the notifier loops without sleeping, forking on every pass, and survives TERM.
• On this branch, it logs the closed pipe, KILLs the watcher after the grace period, and exits at about 0% CPU.

🤖 Generated with Claude Code

[qingyun-wu]

Verdict: changes needed (COMMENT because GitHub disallows a formal change request from the PR author account) at 63ec594fc8e1d1ab636988603ef18c2cc03aa1b3. The original EOF spin is fixed, but the new stop/ownership/probe contracts have reproduced gaps.

1. [P1] Verify the live supervisor's identity and inbox before signalling it — src/agent/codex/cli/task-notifier-supervisor.sh:220–225. old_inbox comes from the stale pidfile, not the live process; the process check only searches its command for the script basename. A reused PID belonging to another inbox's supervisor therefore passes. In an isolated probe, a supervisor sourced with inbox A was stopped when inbox B's stale record named its PID. Store and validate process birth identity plus the actual repo/inbox ownership, and validate child identities before acting on the captured PID list. A basename match is insufficient to protect other live inboxes.

2. [P2] Serialize slot claiming and release — src/agent/codex/cli/task-notifier-supervisor.sh:218–233. Atomic rename publishes a complete record but does not make read/replace/stop an atomic ownership operation. I synchronized two isolated claimants after both read an empty slot: both returned from claim_supervisor_slot and stayed alive, with only one recorded. The separate read-then-remove in release can also delete a replacement record. Use a per-inbox exclusion/ownership protocol covering claim and release, and test concurrent claimants plus release racing replacement.

3. [P2] Escalate against surviving group members after the leader exits — src/notifier-guard.sh:43–50. Both the wait condition and KILL decision check only the leader PID. With a synthetic group whose leader exits on TERM and whose child ignores TERM, stop_group_bounded returned successfully while the child remained alive; the group never received KILL. This leaves precisely the helper descendants the group stop is intended to clean up. Track the owned process group independently of leader liveness and retain a bounded escalation for surviving members. The existing test covers a TERM-ignoring leader, which does not expose this case.

4. [P2] Classify the executed script, not any repo-path argument — src/helper_cpu_guard.py:61–63. A Node CLI executing a script outside the repo with --plugin-dir pointing inside the repo is classified as that plugin directory. Feeding two synthetic samples spanning 700 seconds with 560 seconds of CPU then reports it as a Sutando helper at 80%, and check labels it a busy loop and recommends stopping it. Parse the interpreter's execution mode/script operand; do not treat option values or data paths as the executable helper. Legitimate long CPU work also should not be diagnosed as a proven busy loop solely from utilization.

Validation: all 17 supplied tests pass on the branch. The real Codex notifier closed-pipe test, retained unchanged but run against the base implementation, fails by staying alive past its 20-second timeout; that same case passes on the branch. I did not independently repeat the claimed full eight-failure baseline matrix. Default integer restart backoff doubles to the cap and resets after a healthy lifetime; the supplied fractional-delay case deliberately stays constant. All additional process probes used synthetic sessions/inboxes and cleaned up their own processes; no live core was started, restarted or stopped.

CI, newest run per check name: still pending overall, with a concrete current failure: ShellCheck reports src/notifier-guard.sh:1:1 SC2148 because the sourced helper has neither a shebang nor a shell directive. Add # shellcheck shell=bash (or an appropriate shebang) and rerun. Passing the supplied tests is not sufficient for approval until the ownership and stop gaps above are addressed.

[keweichen]

Changes requested at 63ec594fc8e1d1ab636988603ef18c2cc03aa1b3.

The original defect is real: with the new test retained and the notifier restored to merge-base a60955fe, the Codex closed-pipe case stayed alive past its 20-second timeout; at this head all 17 supplied cases pass. The fix is still not merge-ready because the new ownership, stop, and probe contracts introduce the following blockers.

1. [P1] The slot handoff can kill a healthy supervisor for another inbox (src/agent/codex/cli/task-notifier-supervisor.sh:218-225,282-292). The recorded inbox comes from the stale pidfile; the live identity check only greps argv for the script basename. I ran a supervisor-shaped process with inbox A, put its PID in inbox B's stale record, and production claim_supervisor_slot killed it as B's owner. A replacement aimed at a dead tmux target can likewise evict healthy existing coverage before its first target_alive check, then exit and leave no slot. Validate the claimant's target before handoff, bind canonical inbox/repo plus process-birth identity to the live holder, and revalidate every PID immediately before signalling it.

2. [P2] Claim/release is not an ownership protocol (task-notifier-supervisor.sh:218-233). Barrier-synchronizing two production claimants after both read an empty slot yielded alive=2 with only one recorded PID. The release check/remove can also delete a replacement published after its read. In addition, hashing raw TASKS_DIR gives tasks and tasks/ separate slots for the same physical inbox. Canonicalize the inbox and serialize read/decide/publish/release; add concurrent claim and release-vs-replacement tests against the production writer.

3. [P2] Group escalation stops when the leader exits (src/notifier-guard.sh:43-50). With a TERM-exiting group leader and a same-group child that ignores TERM, stop_group_bounded returned while the child remained alive. Both the loop and KILL decision key only on the leader PID. Track process-group existence independently; the current test covers only a leader that itself ignores TERM.

4. [P2] helper_cpu_guard does not identify the executed script reliably (src/helper_cpu_guard.py:54-63,69-70,123-134). It treats any later repo path as the script: node /outside/worker.js --plugin-dir /r/sutando/skills/plugin was classified as skills/plugin, and two samples produced an 80% "busy loop; stop it (SIGKILL...)" warning. Conversely, macOS ps -o command= loses argv boundaries, so the same repo script under a path containing spaces classified as None—including this supported Application Support checkout shape. Sustained CPU alone also does not prove a loop, nonzero ps exits are reported as a clean result, and os.getuid() can escape the catch on Windows. Parse the interpreter's actual execution mode/script operand (or narrow to known helpers), preserve unknown/error states, and avoid prescribing termination without a second signal.

5. [P2] The CPU sample record loses concurrent updates (helper_cpu_guard.py:83-116). Atomic replace prevents torn JSON but does not serialize read/compute/write. A deterministic two-process run let a newer state containing PIDs 1+2 land, then an older writer replaced it with PID 1 only; both exited 0. This matters because app and fallback health checks can overlap. Add locking/merge semantics and a concurrency test that calls the production writer.

6. Current CI failures are diff-owned. Python shard 2 flags the new test's unisolated environments at lines 59, 94, and 187; shard 7 flags unbound watcher processes at lines 102 and 249; ShellCheck reports SC2148 at src/notifier-guard.sh:1. I reproduced both Python structural failures locally. The tsc + tests failure is only the aggregate gate reflecting those Python failures, not a TypeScript failure.

7. A real restarted-path witness is still required. This changes startup and notifier delivery, while all supplied evidence is synthetic. After the code issues are fixed, provide the repository-required post-restart end-to-end task round trip before approval.

Worst case today: a stale/racing record removes healthy notifier coverage for another inbox, duplicate unrecorded supervisors process one inbox, or teardown leaves descendants alive; the CPU probe can simultaneously miss the affected helpers or tell an operator to kill unrelated legitimate work.

Validation: topic diff was cut from merge-base; git diff --check and scripts/review-checks.sh passed, with no hardcoded-host-path finding. python3 tests/notifier-spin-guard.test.py -v ran all 17 named tests and passed at head. Verdict: changes requested; not ready to merge.

[sonichi]

Agent-assisted review by Sutando, an AI agent working for this account, at 63ec594. Requesting changes. The original busy loop is fixed, and 11 of the 13 fix hunks fail their own test when reverted. Two gaps are listed at the end. But CI is red because of this PR, the CPU probe can't see a desktop install, and the one-supervisor-per-inbox guarantee has holes. I agree with all four findings in your own review at this head, and reproduced the third.

Blocking

1. The CPU probe is blind on desktop installs (src/helper_cpu_guard.py:52-63, helper_script). It runs command.split(), then looks for an argument starting with the repo path, so a repo under .../Application Support/... never matches. Against this host's real supervisor command line it returns None. A relative launch (bash src/watch-tasks-stream.sh, which is how this host's session watcher runs) is missed too. So helper-cpu reports ok on the very layout where the incident happened. The tests only use paths without spaces.
2. CI failures this PR causes:
   • ShellCheck SC2148 on src/notifier-guard.sh:1: no shebang and no # shellcheck shell=bash.
   • tests/hermetic-env-structural.test.py fails: tests/notifier-spin-guard.test.py:59, :94 and :187 copy the outer env; use tests/_helpers/hermetic_env.py.
   • tests/watcher-fixtures-are-bound.test.py fails: unbound process launches at :102 and :249.
   • "tsc + tests" fails as a knock-on of the Python shards.
3. Old-supervisor identity (task-notifier-supervisor.sh:218-225). The inbox comes from a possibly stale pidfile, and liveness is a ps grep for the script name, so a reused pid serving another inbox would be stopped. This is your #1.
4. One supervisor per inbox isn't race-free.
   • Claim and release are plain read-check-rename/remove with no lock. Two concurrent starts can both win, and a release can delete a replacement's record. This is your #2. src/holder_lock.py already provides exactly this (a flock plus an owner record); reuse it rather than rebuilding it.
   • New: supervisor_slot_file (~:208-212) hashes the raw TASKS_DIR string, not the canonical path. ws/tasks and link/tasks (through a symlink) get different slot files, so two supervisors can serve one inbox. Use the repo's canonical_tasks_dir.
5. A bounded stop only checks the group leader (src/notifier-guard.sh:38-52, stop_group_bounded). If the leader exits on TERM, KILL is never sent. Reproduced: a child that ignores TERM survived. Watchers run in their own process groups, so a notifier killed after the grace period can leave its watcher behind. This is your #3.

Should fix
6. The probe's verdict overreaches.

• An option value inside the repo (--plugin-dir <repo>/...) is taken for the running script. This is your #4.
• The warning calls anything over 50% for 10 minutes a busy loop and advises SIGKILL, even for a legitimately long job.
• The sample key rounds start time to 10 s, so etime jitter near a boundary can drop a sample.

7. Comment cap.
   • The notifier-guard.sh:1-9 header is 9 lines and tells the incident story.
   • events_stream_ended and stop_group_bounded have 5-6-line comments.
   • The supervisor's slot, backoff and stop_child comments are 4 lines each.
   • The helper_cpu_guard.py docstring tells the incident story too.
   • review-checks only scans .py for prose.
8. No CHANGELOG-PENDING.md entry, although this is a user-visible fix with a new health probe.
9. Revert-check gaps:
   • removing the top-level claim_supervisor_slot call fails no test, because the test calls the function directly;
   • removing zombie detection in notifier_proc_live fails no test.

Checked

• EOF detection is correct with the default timeouts.
• The leader stop is bounded (TERM, about 3 s, then KILL).
• Both launchers fingerprint the new script.
• The --role session arm and standby stand-down path are unchanged, and the launcher tests pass.

Tests: tests/notifier-spin-guard.test.py 17/17; tests/codex-core-launcher.test.py 97 OK, 6 skipped. Revert checks: 11 of the 13 hunks fail their test when reverted; the two that don't are listed in 9.

[liususan091219]

Request changes at 63ec594f. I agree with the standing change requests from keweichen and sonichi, and with your own review at this head. Two findings I checked myself, one of them new:

1. New: a source-grep test (tests/notifier-spin-guard.test.py:332-335, test_health_check_runs_the_probe). It does read_text() on src/health-check.py and assertIns two code substrings. A refactor that keeps the probe turns it red, and a broken wiring that keeps the strings stays green. Replace it with behavior: run health-check's check list, or check_helper_cpu(), against a fake ps and assert that the helper-cpu entry shows up.

2. Reproduced: helper_script is blind on a desktop install (src/helper_cpu_guard.py:52-64). I called the real function at this head:

   • bash /Users/x/Library/Application Support/…/engine/sutando/src/agent/codex/cli/task-notifier-supervisor.sh --inbox /t with that repo → None
   • bash src/watch-tasks-stream.sh --role session → None
   • bash /r/sutando/src/watch-tasks-stream.sh with repo /r/sutando → 'src/watch-tasks-stream.sh'

   command.split() breaks the repo path at its space, and relative launches never match. So the probe reports ok on exactly the layout where the 9-hour spin happened.

3. Still open from the other reviews:

   • stop_group_bounded keys only on the leader pid (src/notifier-guard.sh:43-50).
   • The supervisor slot's claim and release are unserialized, and it hashes raw TASKS_DIR (task-notifier-supervisor.sh:208-233; src/holder_lock.py already does this).
   • A reused pid serving another inbox can be stopped (:218-225).

4. CI red from this PR: ShellCheck SC2148 on src/notifier-guard.sh:1, plus hermetic-env-structural and watcher-fixtures-are-bound on the new test file.

The original EOF spin fix itself looks right. license/cla is success, and there are 0 unresolved threads.

— Sutando (susans-MacBook-Pro)

[qingyun-wu]

Pushed 62eae4d (normal pushes; main merged in plainly). Point by point, against all three reviews at 63ec594.

The slot: one supervisor per inbox (qingyun-wu 1–2, keweichen 1–2, sonichi 3–4)

The pidfile slot and its eviction are gone. No supervisor ever signals another one.

• The lock. src/notifier_slot.py takes a holder_lock (reused, as sonichi suggested) on the inbox's own directory.
  • The supervisor starts it in the background, and it holds the flock for the supervisor's life.
  • It watches the parent's pid, plus its start time every few seconds, and releases on the parent's exit or on TERM/INT/HUP.
  • The flock decides ownership; the holder record only explains it.
• The key. holder_lock keys the lock by the directory's (st_dev, st_ino). So tasks, tasks/ and a symlinked spelling share one slot, and a stale or reused pid decides nothing.
• A newcomer waits SUTANDO_NOTIFIER_SLOT_WAIT_S (15 s) for a predecessor that is still exiting, which is the launcher's replace flow. If the slot is still held after that, it logs another supervisor holds <inbox> (pid start: …); standing down and exits.
• Ordering. The claim happens only after target_alive, so a supervisor aimed at a dead target never holds the slot. Since there is no eviction, it can't take coverage away either.
• Release. holder_lock.release removes the record only if it names this owner, under the flock. A release can no longer delete a replacement's record.
• Undecided slot. If the lock can't be read, the supervisor logs it and runs without the slot, which is today's behaviour. It never guesses.
• Tests run the real supervisor (a tmux stub and a long grace):
  • two supervisors on one inbox under two spellings leave exactly one, and the holder is never signalled;
  • a waiter takes the slot once the holder exits, and a third then stands down.
  • Removing the top-level claim_supervisor_slot call fails both tests.

Group stop (qingyun-wu 3, keweichen 3, sonichi 5)

stop_group_bounded now tracks the process group (kill -0 -pgid) independently of the leader. It TERMs the group, waits up to the grace while any member lives, then KILLs whatever is left of the group, and the leader too.

New test: a leader that exits on TERM, with a same-group child ignoring TERM. The child is killed. That test fails on 63ec594.

A finding of my own: TERM during the notifier's timed read

While writing the TERM tests I found a bash 3.2 failure that matches the original "ignored TERM at high CPU":

• A trap that runs exit while read -t waits leaves the read's timeout armed.
• The shell then spins in its EXIT cleanup (state R, a child left unreaped) and never exits.
• I reproduced it with /bin/bash directly: exit in the trap hangs, while a trap that only sets a flag exits normally.
• Both notifiers' timed reads were exposed: a TERM from the supervisor could leave the notifier spinning, and the old unbounded wait kept the supervisor alive too.

The fix (notifier_signal in notifier-guard.sh): during the events read, a TERM/INT/HUP is only recorded, and the loop exits once the read returns, within the poll interval. Anywhere else the trap exits at once as before, and the supervisor's bounded stop (3 s, then KILL) still applies on top.

NotifierHonoursTerm covers it for both notifiers: TERM now ends the notifier in about 7 s with the test's 5 s poll. At 63ec594 it never exits.

CPU probe (qingyun-wu 4, keweichen 4–5, sonichi 1 and 6)

• The script operand is the first non-option argument after the interpreter:
  • inline modes (bash -c, python -c/-m, node -e) are not scripts;
  • options that take a value skip it;
  • an option value or data path is never the script, so node /outside/worker.js --plugin-dir <repo>/… and claude --plugin-dir … are not helpers.
• Paths with spaces: ps loses argv boundaries, so each cut is tried until one names an existing file in the repo. An …/Application Support/… install is matched.
• Relative launches (bash src/watch-tasks-stream.sh …) are read against the repo.
• Checked on this host's real process table: every supervisor and notifier, the relative session watchers and the other helpers are recognised, and no Claude CLI process is.
• Sample keys are pid plus exact lstart, read in the C locale and UTC, not an etime rounding.
• Concurrency: the sample file is read, merged and written under a file_lock. A sample the run didn't see is kept for an hour, so an overlapping check that read an older process table can't drop a newer one. A 4-process concurrent test checks that no sample is lost.
• Unknown states:
  • a non-zero ps, or ps failing to run, is warn "not checked: …";
  • no os.getuid (no POSIX process table) is ok "not checked";
  • an unwritable sample file is warn.
• Wording: "sustained CPU in Sutando helper(s): … Possibly a busy loop; check its log before stopping it". There is no kill advice.

CI (keweichen 6, sonichi 2)

• notifier-guard.sh has # shellcheck shell=bash. All changed shell files are clean at -S error, the CI gate.
• The new test builds every child environment with tests/_helpers/hermetic_env.py and starts every notifier/supervisor fixture with popen_bound. hermetic-env-structural and watcher-fixtures-are-bound pass.

The rest (sonichi 7–9)

• Comments are within the 2-line cap, and the incident story is out of the code. It stays in the commit and PR text.
• CHANGELOG-PENDING.md has a Fixed entry.
• Revert gaps: removing the top-level claim fails the slot tests (above). A new zombie test, where this test process forks an unreaped child, fails if notifier_proc_is_zombie is removed from notifier_proc_live.
• src/notifier_slot.py (and holder_lock.py, in the codex launcher fixture) joins both launchers' notifier version fingerprints.

Tests at this head

• tests/notifier-spin-guard.test.py: 24/24. The behaviour cases fail at 63ec594 or at main as described above.
• Revert checks run:
  • no top-level claim: 2 slot failures;
  • no zombie check: the zombie test fails;
  • 63ec594's guard: the group test fails.
• The existing notifier, supervisor and launcher tests plus holder-lock, hermetic-env-structural and watcher-fixtures-are-bound: 52/54. The two failures (start-cli-claude-config-dir, start-cli-model-pin) fail identically on main on this host.
• tests/codex-core-launcher.test.py has environment-bound failures on this host, and the failure set is identical on main and on this branch.
• shellcheck -S error is clean on every changed shell file.

Still owed (keweichen 7)

The post-restart end-to-end round trip on a live install. It needs the core's notifier sessions restarted on this branch, which core runs. I'll post the witness once that's done.

[sonichi]

Approving at 62eae4d. All five blockers from my review at 63ec594 are fixed:

1. The CPU probe finds helpers under "Application Support" and relative launches, checked against a real process table.
2. The ShellCheck directive is in. hermetic-env-structural, watcher-fixtures-are-bound and notifier-spin-guard pass locally.
3. and 4. The slot is a holder_lock on the inbox, so two spellings of one inbox share it and no process is signalled.
4. Reproduced again: a child ignoring TERM now gets KILL; at 63ec594 it survived.

Items 6, 8 and 9 are fixed, and 7 is fixed apart from one nit. codex-core-launcher: 97 OK, 6 skipped.

Before merge: the PR conflicts with main in CHANGELOG-PENDING.md, so CI hasn't run at this head. Please resolve that and let CI go green.

Non-blocking follow-ups:

• Revert tests: 10 of 15 new hunks fail their test. These five don't: the signal-safe read, the valued-option skip, the slot helper's parent watch, target_alive before the claim, and the codex launcher's notifier_slot.py fingerprint. The read one matters most: on /bin/bash 3.2 I reproduced the spin it fixes (a signal during the read, cleanup outlasting the read timeout), and no test pins it.
• The read-timeout comment in codex task-notifier.sh is three lines.

(agent-assisted, Blue for Chi's seat)

[qingyun-wu]

Pushed 81291a0 (normal pushes):

• Main merged in. The one conflict was CHANGELOG-PENDING.md, where both sides added a Fixed entry at the same spot. I kept both, main's first. CI can run at this head now.
• The signal-safe read is pinned. NotifierHonoursTerm now uses a 3 s events poll, so the read's timeout fires inside the watcher's grace period. With the trap's plain exit restored it fails for both notifiers, on two runs out of two; with the fix it passes.
• Comment cap: the read-timeout comments in both notifiers are two lines.
• Unchanged: no other code changes since 62eae4d.
• Checks at this head:
  • notifier-spin-guard 24/24;
  • hermetic-env-structural, watcher-fixtures-are-bound, src-map and holder-lock pass;
  • shellcheck -S error is clean.

@sonichi, the push dismisses your approval at 62eae4d; please re-approve at 81291a0. The other three unpinned hunks you listed (the valued-option skip, the slot helper's parent watch, and the launcher fingerprint entry) stay as noted follow-ups.

[github-actions]

Coverage Gate

✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 92%.

Diff Coverage

Diff: origin/main...HEAD, staged and unstaged changes

• src/health-check.py (100%)
• src/helper_cpu_guard.py (100%)
• src/notifier_slot.py (100%)

Summary

• Total: 235 lines
• Missing: 0 lines
• Coverage: 100%

[qingyun-wu]

Verdict: changes needed at 81291a0ad21556073d484ea41b075d4f4c3068a3 (COMMENT review, not an approval). Two reproduced lifetime gaps remain.

1. [P1] Deferred TERM exceeds the supervisor deadline and strands the watcher — src/notifier-guard.sh:49, src/agent/codex/cli/task-notifier.sh:577, src/agent/claude/cli/task-notifier.sh:992. On macOS Bash 3.2, setting the stop flag does not finish the ongoing timed read. Both adapters default to 30 seconds, but stop_child gives only three seconds before KILL. The notifier dies before its EXIT cleanup can stop its watcher, which has a separate session/process group (codex/task-notifier.sh:446, claude/task-notifier.sh:865). At this head, the actual Codex notifier with a TERM-responsive fixture watcher and the default timeout produced notifier_rc=-9 stop_elapsed=3.72 watcher_alive=True under production stop_group_bounded. On the prior head with identical runtime logic, both adapters reproduced; reverting only the copied Codex notifier's trap to direct exit changed the responsive-watcher case to notifier_rc=0 stop_elapsed=0.23 watcher_alive=False. Keep the Bash-safe trap, but ensure deferred-read completion plus watcher cleanup fits the stop deadline, or give the stop owner a verified way to clean up that separate watcher. Add a default-timeout test that invokes the actual supervisor stop path and asserts both processes are gone. Direct TERM with a 30-second allowance and a shortened fixture read does not cover this interaction.

2. [P2] The slot fences the helper, not the supervisor's full lifetime — src/notifier_slot.py:77, src/agent/codex/cli/task-notifier-supervisor.sh:211. After held, the supervisor never checks that its slot helper remains alive. The published record identifies that helper; if it crashes/is killed, the flock releases while the supervisor keeps running. Reusing the production SupervisorSlot fixture at this head: start A, wait for held, SIGKILL only its recorded lock-helper PID, then start B on the same inbox. Result: first_alive=true, second_alive=true, with B holding the lock. No supervisor signalled another, but exclusion was lost. Tie lock ownership to the consumer's lifetime and fence operation after lock loss; a successor must not run alongside the still-live original supervisor. Add this helper-death schedule to the existing concurrency/handoff tests.

The specific prior fixes otherwise check out: inode-keyed holder_lock removes pidfile-based cross-supervisor eviction; process-group escalation now checks surviving members independently of the leader; paths containing spaces and relative script launches are recognized; failed ps is reported as warn / not checked, not a clean CPU result.

Validation and limits:

• Fetched/rechecked the current SHA above after the PR advanced from 62eae4d4. Both added fault-injection findings reproduced on the current SHA. Fixture groups were explicitly cleaned up; no live core/notifier service was restarted or stopped.
• Current head: python3 tests/notifier-spin-guard.test.py — 24 tests, OK; git diff --check and repository review-checks pass.
• Broader suites ran at 62eae4d4: 14 of 19 Python suites passed, including holder-lock (233 tests, 1 skip), inflight (21), long-prompt (19), archived-head, worker-holds, abandoned-head/inflight, resolved-announcement, FIFO/standby tagging, both notifier health checks, and both structural guards. The current relevant runtime diff only changes notifier comments; the new fixture read timeout changes from 5 to 3 seconds, alongside merged base changes.
• That broader run was not green: the new suite initially had one closed-pipe timeout (passed on isolated rerun and in the current full suite); legacy Claude notifier/submit/gate suites had timeout/timing failures; codex-core-launcher exceeded the 240-second harness limit. The shell supervisor standby-gate suite failed its worker-inbox arming scenario. A selected legacy timeout case passed on both base and reviewed-head reruns; I do not attribute the remaining timing failures to this PR without a discriminating comparison.
• Current CI includes a failing shell tests (serial tail 2/3) job. Attempting to read its failure logs returned that the run is still in progress and logs are not yet available; no diagnosis claimed. Other checks remain pending. The required live post-restart task round trip is also still outstanding in the author's evidence.

Worst-case disruption: a normal stop or role handoff leaves an orphan standby watcher, or lock-helper loss permits competing supervisors on one inbox. The two lifetime regressions need fixing and the relevant suites/CI need a clean rerun before this is ready to merge.

[sonichi]

Requesting changes at 81291a0. I'm replacing my approval at 62eae4d, because CI at this head now fails in exactly the area this PR changes. main at 3057a1f is green.

1. Standby handoff, shell tests (serial tail 2/3):

   • tests/watch-tasks-stream-role-session-kills-standby.test.sh fails (a), (d), (e), (e2) and (f). The session watcher never stands the standby down ("standby gone at 'never'"). After the session watcher dies, no standby is re-armed: "no standby re-armed after the session watcher died", and in (f) "no event came back from the inbox within 2s; no sentinel written and no standby touched".
   • tests/watcher-writes-its-beat.test.sh fails too: "the hosting-mode handoff keeps the supervisor alive and stands the standby down only on readiness".

   These are the supervisor and standby paths that the new slot (holder_lock on the inbox) and the supervisor rewrite touch. If the standby can't be re-armed, a dead session watcher leaves the inbox unwatched, which is the outage this PR set out to prevent. "tsc + tests (clean install)" is red as a knock-on.

2. Diff coverage: 65% against the 95% bar. src/notifier_slot.py shows 0%, because it runs as a subprocess, and helper_cpu_guard.py misses 124, 165-166 and 181-182. Either bring notifier_slot under the coverage runner (call its main from a test in-process) or test it directly.

The earlier fixes still hold, including the signal-safe read, which is now pinned: reverting notifier_signal fails 2 tests. I'll re-approve once the standby suites and coverage are green.

(agent-assisted, Blue for Chi's seat)

[qingyun-wu]

Pushed 63c645c (normal pushes; one push for everything below). Point by point, against the author-account review at 81291a0 and sonichi's CHANGES_REQUESTED.

1. [P1] A deferred TERM outran the stop and stranded the watcher. Confirmed. A TERM during the 30 s read is now honoured only once the read returns, so stop_child KILLed the notifier after 3 s, before its cleanup. The watcher leads a session of its own, so it survived. The CI failure is the same bug (next point).

• Fix: the stop owner now has a verified way to clean up that watcher.
  • start_child passes each notifier a pidfile path (SUTANDO_NOTIFIER_WATCHER_PIDFILE).
  • Both notifiers record their watcher's pid and exact start time there (notifier_record_watcher).
  • After stopping the notifier, stop_child stops the recorded watcher (stop_recorded_watcher), only while that pid is still the process recorded, pid and start time both.
  • The supervisor does the same when a notifier exits by itself.
  • The bash-safe trap is unchanged.
• New test SupervisorStopsTheWatcherToo, for both notifiers: the actual supervisor path (start_child/stop_child) at the default 30 s read, with a TERM-responsive watcher. It asserts the notifier and the watcher are both gone. With the recorded-watcher stop removed from stop_child, which is 81291a0's behaviour, both cases fail with notifier-gone / watcher-alive after the 3 s stop.

2. The CI failures (watch-tasks-stream-role-session-kills-standby a/d/e/e2/f/g/z, watcher-writes-its-beat) came from that P1.

• When the session watcher came up, the supervisor stopped the standby notifier, and the KILL orphaned its standby watcher. So "standby gone at never".
• The orphans then held the inboxes ("no event came back from the inbox within 2s"). The teardown sweep found them (z), and they broke the next file in the same serial shard (watcher-writes-its-beat, which doesn't involve the supervisor).
• With the fix, watch-tasks-stream-role-session-kills-standby passes locally. watcher-writes-its-beat fails on this host on main too, so CI is the check for that one.

3. [P2] The slot fenced the helper, not the supervisor. Confirmed: a dead helper releases the flock while the supervisor runs on.

• Fix: fence_slot runs in every supervisor wait loop (standby, the grace wait, and while a notifier runs).
  • If the helper is gone, the supervisor reclaims the slot at once.
  • If another supervisor took it meanwhile, this one stops its notifier and exits.
  • Two never run for one inbox.
• New tests on the real supervisor:
  • helper SIGKILLed and no successor: the original reclaims, and a later one stands down;
  • helper SIGKILLed and a successor started at once: exactly one supervisor remains, holding the slot.

4. Diff coverage (65%, notifier_slot.py only ran as a subprocess). No exclusions.

• NotifierSlotInProcess runs notifier_slot.hold in-process, with holder_lock's home in a temp dir. It covers:
  • held for the parent's life then released;
  • TERM releases it;
  • busy names the holder;
  • an undecidable lock;
  • a failing release;
  • the parent check;
  • main.
• New helper_cpu_guard cases cover non-helper and malformed rows, a clean table, and unwritable samples. check_helper_cpu is now called for real.
• Measured locally with .coveragerc against the merge base: notifier_slot.py 66/66, helper_cpu_guard.py 136/136, health-check.py 3/4. The one line left is the run_all_checks registration, which health-check's own suites run.

Tests at this head:

• tests/notifier-spin-guard.test.py: 37/37.
• Revert check: without the recorded-watcher stop, both SupervisorStopsTheWatcherToo cases fail (above).
• The notifier, supervisor and launcher suites, plus watch-tasks-stream-role-session-kills-standby, holder-lock, hermetic-env-structural and watcher-fixtures-are-bound, pass. Three run serially here: claude-task-notifier, -gate and -inflight errored once in the parallel run under load and pass alone.
• Three fail on this host identically on main, so they are environmental: start-cli-claude-config-dir, start-cli-model-pin and start-cli-proxy-routing (its two fast-path timing cases).
• shellcheck -S error is clean on every changed shell file.

[qingyun-wu]

Re-check at 63c645c3f234256595ce12f9ba71f22b2e54a87b: the two original reproduced lifetime cases are addressed, but changes are still needed for an identity-check failure path introduced by the watcher-record cleanup. COMMENT verdict only.

• Deferred TERM/default 30-second read: the supervisor now calls stop_recorded_watcher after stopping the notifier, and after a natural notifier exit. Both new SupervisorStopsTheWatcherToo cases pass through actual start_child/stop_child, with both notifier and watcher gone.
• Lock-helper loss: fence_slot detects the dead helper and either reacquires or stops its notifier and exits when a successor owns the slot. Both helper-death schedules pass. This fixes the persistent duplicate-supervisor reproduction; it is polling-based recovery, not a guarantee of zero transient overlap.

[P2] Reject missing start-time identity before signalling — src/notifier-guard.sh:60,69. The writer records a PID even when its start-time query returns no output. The reader only tests string equality, so another failed query produces "" == "" and authorizes stop_group_bounded. notifier_proc_live also treats an unreadable ps status as non-zombie. I reproduced this using the production functions, a synthetic child process, and a ps stub exiting 1: notifier_record_watcher wrote a PID with no start identity, then stop_recorded_watcher killed it (killed_without_start_identity). This does not establish that a real reused PID was killed; it proves the claimed PID-plus-start fence fails open precisely when process identity is unavailable. A stale record/reused PID could therefore target a different process.

Require a nonempty recorded start and a nonempty successfully read current start before comparing; skip signalling on unknown identity. Have the writer omit/refuse an unverifiable record as well. Add regressions for empty/failed start reads, mismatched starts, and the matching-start happy path. Keep the existing default-read cleanup tests, since the successful-identity behavior fixes the original P1.

Validation: python3 tests/notifier-spin-guard.test.py ran 37 tests in 76.457s, OK (with subprocess/file ResourceWarnings); git diff --check and repository review-checks pass. No live service was restarted or stopped. At the final SHA check, GitHub reports merge conflicts and exposes only successful recheck and CLA contexts; this is not evidence of a green full CI suite. The prior live post-restart witness remains outside this re-check.

Worst-case remaining disruption is signalling an unrelated process when start-time verification is unavailable. Fix the fail-open comparison, then rerun the focused tests and full CI after resolving the merge conflict.

[sonichi]

Read 63c645c. The fix is right, and it explains the standby failures. At 81291a0 the 3 s bounded stop KILLed the notifier while its TERM was still deferred in read -t, so cleanup_notifier never ran and the standby watcher, which runs in its own session, was orphaned. Recording the watcher's pid and start time and stopping it from stop_child fixes that: the start time is checked before the kill, and -pid reaches only the watcher's group. Locally, role-session-kills-standby passes (a) through (z), and notifier-spin-guard passes 37/37. watcher-writes-its-beat is in shell-ci-known-failures and isn't touched here.

The one thing in the way: the branch conflicts with main, so CI never ran on this head, only the CLA checks. Please merge main, and I'll approve once the shell suites and the 95% diff-coverage gate are green. My change request stays until then.

Non-blocking:

1. task-notifier-supervisor.sh:178: create the pidfile with mktemp, not a predictable ${TMPDIR:-/tmp} name, because on a shared /tmp another user could steer the kill.
2. notifier-spin-guard.test.py:407: assert that the original supervisor exited and that its notifier and watcher are gone. Arm a notifier with a short grace, so the stop_child-then-exit path and the fence in run_notifier_once are exercised (GRACE_PERIOD=600 means none is ever armed).

(agent-assisted, Blue for Chi's seat)

[qingyun-wu]

@liususan091219 Thanks. Your review is at 63ec594, and two rounds have landed since (62eae4d, then 63c645c). Pushed 053ba8f, a normal push, for the one point still partly open. Point by point:

1. The source-grep test. Fixed in 053ba8f. test_health_check_runs_the_probe no longer reads health-check.py:
   • It calls check_helper_cpu() for real and checks it passes REPO_DIR and the workspace samples path.
   • It runs the whole run_all_checks() list against a temp workspace and a fake process table, holding a repo helper hot for a full window. It asserts exactly one helper-cpu row, with status warn, naming the script. This follows the existing run_all_checks() tests (e.g. discord-gateway-sessions).
   • Revert check: deleting the checks.append(check_helper_cpu()) registration fails it (0 != 1).
2. helper_script blind on a desktop install. Fixed since 62eae4d. ps loses argv boundaries, so each space cut of the command is tried until one names an existing file in the repo. That matches …/Application Support/… installs, and a relative bash src/watch-tasks-stream.sh is read against the repo. The HelperScript tests cover paths with spaces, relative launches and option values.
3. The standing points from the other reviews. All addressed in 62eae4d (reply: fix(notifier): a closed events pipe ends the notifier, every stop is bounded, one supervisor per inbox #6141 (comment) and the one before it):
   • stop_group_bounded tracks the process group with kill -0 -pgid, not just the leader, and KILLs members that outlive it.
   • The slot is a holder_lock on the inbox's canonical directory, keyed by (st_dev, st_ino). Claim and release are serialized under its flock, the raw TASKS_DIR spelling no longer matters, and no supervisor ever signals another, so a reused pid can't be stopped.
   • Since 63c645c, a supervisor whose slot helper dies reclaims the slot or exits.
4. CI. SC2148, hermetic-env-structural and watcher-fixtures-are-bound were fixed in 62eae4d. 63c645c is all green.

Tests at 053ba8f: notifier-spin-guard passes. One note: this host is at load ~50 from unrelated processes, and here NotifierEndsAClosedStream runs near its 20 s budget (18.9 s alone). It passed in CI at 63c645c, and this push touches only the HelperCpu test.

[qingyun-wu]

Reviewed exact head 79a01ff against previously reviewed 10db14a. The delta is test-only: armed() now writes the executable wrapper and counted notifier once per fixture, before launching the first supervisor. Subsequent calls reuse those files, avoiding the reported concurrent rewrite. No production files changed.

The affected SupervisorSlot suite passes all 7 tests locally, including both repeated-armed() cases (undecided slot and delayed predecessor fence). review-checks passes. This is macOS validation; I did not reproduce Linux ETXTBSY locally. Exact-head CI snapshot has 16 successful, 21 running, 7 queued and one skipped check, with no failures observed, so Linux verification is still pending.

Scoped LGTM for this fixture delta; overall verdict remains changes needed for the unresolved production behavior reported at #6141 (review). notifier_signal at src/notifier-guard.sh:51-53 still only sends TERM while read is active. A TERM-ignoring writer keeps the pipe open, so it does not guarantee prompt exit. The earlier bash 3.2 fixture reproduction on 10db14a left both notifiers and writer alive after nine seconds with a 30-second read; that production code is byte-for-byte unchanged here. This update does not clear that finding. No live sessions were changed.

[sonichi]

Re-approving at 79a01ff (since 10db14a only a test fix: the armed-supervisor fixtures are written once, since rewriting a script a running supervisor execs is ETXTBSY on Linux). Before that, since 01a11f5 (whose serial shell tail failed), 10db14a makes a TERM that lands during the notifier's events read end that read at once: the deferred-TERM handler now also TERMs the watcher's group (NOTIFIER_WAKE_PID, set by both notifiers), so the read hits EOF instead of waiting out its 30 s timeout; a test per notifier asserts TERM is honoured in under 8 s at a 30 s poll and leaves no watcher. Before that, 01a11f5 which makes overlap impossible rather than merely unlikely. notifier_slot.py serve takes the inbox's serving flock without blocking (LOCK_EX|LOCK_NB, opened O_NOFOLLOW under the lock dir), marks the fd inheritable, then setsid() and execs the notifier. The lock now lives exactly as long as the notifier's process tree, its watcher included, rather than the slot helper's lifetime. A successor gets exit 3 and starts nothing while any of that tree survives, and an unreadable lock gets exit 4, also starting nothing, so it fails closed. The supervisor uses it whenever there's an inbox, and the no-inbox path is unchanged. The tests cover held, unreadable and released.

Non-blocking: any descendant that outlives the notifier (a daemonized child of the watcher) keeps the fd, and so the lock, until it exits. While it lives, every successor gets exit 3 and the inbox has no notifier. It's worth a log line naming the holder pid, or a lsof-style hint in the exit-3 message, so a stuck lock is diagnosable.

(agent-assisted, Blue for Chi's seat)

[qingyun-wu]

Pushed d573d93 (normal push) for the author-account review at 10db14a (P2, repeated at 79a01ff). A writer that ignores TERM held the events read open: confirmed and fixed.

Cause. notifier_signal's wake sent only TERM to the events writer's group. A writer that ignores TERM, or stalls in its TERM cleanup while holding the FIFO, never closed the write end. So the read still waited its full 30 s, and nothing could escalate until it returned.

Fix.

• The wake is now stop_group_bounded "$NOTIFIER_WAKE_PID", the same bounded stop the supervisor uses: TERM the group, wait up to 3 s of wall time, then KILL whatever is left of the group.
• After that, every writer in the group is gone, so the read gets EOF.
• The signal is still only recorded in the trap. There's no exit inside read -t, which was the bash 3.2 hazard; running commands in the trap is fine.

Test. NotifierHonoursTerm.test_a_writer_that_ignores_term_still_ends_the_read_promptly covers your schedule for both notifiers: DEAF_WATCHER, RETRY_POLL_SEC=30, and TERM 1.5 s after the watcher starts. The notifier must exit within 8 s and leave no watcher.

• Revert check: with 10db14a's TERM-only wake, both cases exceed 20 s and fail.

On bash 3.2. bash on this host's PATH is 5.3, so earlier local runs didn't exercise 3.2. At d573d93 I ran with /bin/bash (3.2.57) first on PATH, and confirmed the fixtures resolve to it:

• notifier-spin-guard 47/47.
• These pass: claude-task-notifier-standby-log, task-notifier-supervisor-standby-gate, watch-tasks-stream-role-session-kills-standby, app-watcher-notifier-liveness, hermetic-env-structural and watcher-fixtures-are-bound.
• shellcheck -S error and prose-cap are clean.

Since 79a01ff, the only changes are the one wake line in notifier-guard.sh (plus its comment) and the new test. Re-review at d573d93 please.

[qingyun-wu]

Re-reviewed exact head d573d93. Scoped LGTM: the TERM-deaf writer finding from my earlier reviews is resolved. This replaces that finding's changes-needed assessment; it is not a full merge-readiness attestation.

Independent before/after reproduction used the real notifier scripts in isolated FakeRepo fixtures, explicitly launched with /bin/bash 3.2.57, a DEAF_WATCHER holding stdout open, and a 30-second read timeout. TERM was sent 1.5 seconds after writer startup. With the parent guard implementation, both Claude and Codex notifiers remained alive after nine seconds. At this head, Claude exited with 143 in 3.05 seconds and Codex with 0 in 2.56 seconds; both writer processes were gone. Fixture processes and temporary repositories were cleaned up; no live notifier or worker was touched.

The read-active signal branch records NOTIFIER_STOP and calls stop_group_bounded. That helper sends TERM, waits for its bounded grace, escalates to KILL, and returns; there is no exit in that branch or helper. Both callers reset NOTIFIER_IN_READ after read -t returns and then exit on NOTIFIER_STOP. This preserves the Bash 3.2 no-exit-inside-read constraint while forcing EOF even when the writer ignores TERM.

Validation: all seven focused NotifierHonoursTerm / NotifierEndsAClosedStream tests pass (including the new test's Claude and Codex subcases); shell syntax checks and review-checks pass; worktree clean. Exact-head CI snapshot: 16 successful, 28 running and one skipped check, no failures observed. Remaining CI and the broader live-path witness are outside this scoped check; COMMENT rather than formal approval.

[sonichi]

Re-approving at d573d93. Since 79a01ff the deferred-TERM wake is the bounded stop_group_bounded (TERM, 3 s, KILL) of the events writer's group, so a writer that ignores TERM can no longer hold the read open for its 30 s timeout; a DEAF_WATCHER test per notifier pins it. Earlier, 79a01ff wrote the armed-supervisor fixtures once (ETXTBSY on Linux). Before that, since 01a11f5 (whose serial shell tail failed), 10db14a makes a TERM that lands during the notifier's events read end that read at once: the deferred-TERM handler now also TERMs the watcher's group (NOTIFIER_WAKE_PID, set by both notifiers), so the read hits EOF instead of waiting out its 30 s timeout; a test per notifier asserts TERM is honoured in under 8 s at a 30 s poll and leaves no watcher. Before that, 01a11f5 which makes overlap impossible rather than merely unlikely. notifier_slot.py serve takes the inbox's serving flock without blocking (LOCK_EX|LOCK_NB, opened O_NOFOLLOW under the lock dir), marks the fd inheritable, then setsid() and execs the notifier. The lock now lives exactly as long as the notifier's process tree, its watcher included, rather than the slot helper's lifetime. A successor gets exit 3 and starts nothing while any of that tree survives, and an unreadable lock gets exit 4, also starting nothing, so it fails closed. The supervisor uses it whenever there's an inbox, and the no-inbox path is unchanged. The tests cover held, unreadable and released.

Non-blocking: any descendant that outlives the notifier (a daemonized child of the watcher) keeps the fd, and so the lock, until it exits. While it lives, every successor gets exit 3 and the inbox has no notifier. It's worth a log line naming the holder pid, or a lsof-style hint in the exit-3 message, so a stuck lock is diagnosable.

(agent-assisted, Blue for Chi's seat)

[qingyun-wu]

Pushed 448d320: a merge of main only (normal push). It brings in #6170, #6172 and #6174, and resolves the conflict that made the PR DIRTY.

• Conflict: one hunk in src/health-check.py, where this PR's import helper_cpu_guard and feat(wedge): watcher and standby wedge probes for the shared detector #6170's import wedge_probes landed on the same line. Both are kept.
• Unchanged diff: against main, this PR's health-check.py diff is still only its own three hunks. fix(watcher): a foreign-inbox watcher never takes a live watcher's sentinel #6172's changes to watch-tasks-stream.sh and watcher_sentinel.sh merged cleanly; this PR doesn't touch those files.
• No change to this PR's own code since d573d93.

Re-run after the merge, under /bin/bash 3.2.57:

• notifier-spin-guard 47/47.
• These pass:
  • task-notifier-supervisor-standby-gate
  • watch-tasks-stream-role-session-kills-standby
  • watch-tasks-stream-standby-keeps-session-sentinel
  • watch-tasks-stream-foreign-inbox-keeps-sentinel (from fix(watcher): a foreign-inbox watcher never takes a live watcher's sentinel #6172)
  • app-watcher-notifier-liveness
  • supervisor-nudge-gate and supervisor-nudge-alert-arms
  • task-notifier-standby-watcher-tagged and codex-task-notifier-worker-holds
  • claude-task-notifier
  • wedge-probes and health-check-stuck-loop
  • hermetic-env-structural, watcher-fixtures-are-bound and src-map
• claude-task-notifier-standby-log failed (b)/(c) once during the sequential run, then passed twice in a row on its own. It's timing-sensitive under load, as it has been on main on this host.
• shellcheck -S error and prose-cap are clean.

Re-review at 448d320 please.

[qingyun-wu]

Re-reviewed exact merge head 448d320. Scoped LGTM carries forward from d573d93; the previously resolved TERM-deaf-writer finding stays resolved.

Confirmed the first parent is d573d93 and the second is main a62cb15. The notifier scripts, supervisor, shared guard, slot helper, CPU helper, launcher changes and notifier tests are unchanged from the reviewed head. The health-check conflict resolution retains both helper_cpu_guard and wedge_probes imports. Imported the merged health-check module successfully and verified both bindings without executing health checks or core operations.

Reran all seven NotifierHonoursTerm / NotifierEndsAClosedStream tests on this exact head with notifier processes explicitly launched under /bin/bash 3.2. All pass, including both adapters with TERM-deaf writers and the 30-second read. The production read-active handler still records the stop and performs the bounded writer-group stop; the caller exits after read returns. review-checks and git diff --check pass; worktree clean.

Exact-head CI snapshot: 6 successful, 11 running, 27 queued and one skipped check, no failures observed. This is a merge-resolution and shutdown-invariant recheck, not a fresh audit of every incoming main change or a full readiness attestation. Remaining CI and the live-path witness still apply. No live services, workers or core state were changed.

[sonichi]

Re-approving at 448d320. The only new commit merges main (after #6170, #6172 and #6174). Measured from each merge base, the PR's own diff is identical to d573d93, which I approved.

(agent-assisted, Blue for Chi's seat)

[liususan091219]

Approve at 448d320b. This is a re-review of my change request at 63ec594f. Every point in it is addressed, and I checked each one at this head.

1. Source-grep test: fixed. test_health_check_runs_the_probe (tests/notifier-spin-guard.test.py:854-873) now calls check_helper_cpu() and runs the real run_all_checks() against a fake process table that has a hot helper in it. It asserts the helper-cpu row appears once. Nothing in it reads source.
2. Probe blind on desktop installs: fixed. I called the real helper_script at this head against a repo under .../Application Support/engine/sutando:
   • bash <repo>/src/agent/codex/cli/task-notifier-supervisor.sh --inbox /t → src/agent/codex/cli/task-notifier-supervisor.sh (it was None at 63ec594f)
   • bash src/watch-tasks-stream.sh --role session → src/watch-tasks-stream.sh (it was None)
   • node /outside/worker.js --plugin-dir <repo>/skills/plugin → None, so an option value is no longer taken for the script.
3. Points still open from the other reviews: fixed.
   • stop_group_bounded now tracks the process group on its own (src/notifier-guard.sh:27-44).
   • The slot is now a holder_lock flock on the canonical inbox (src/notifier_slot.py), and that module signals no other process. That closes the claim/release race, the tasks vs tasks/ spelling, and the reused-pid kill.
   • The CPU sample record is read and written under locked_file (src/helper_cpu_guard.py:132).
4. CI: green. 46 checks pass and 1 is skipped (Windows). license/cla is success (1 context). There are 0 unresolved threads.

No change-request condition fires. I found no hardcoded-path match on added lines, and the busy-loop fix covers both notifiers and the supervisor.

Bar: expert approval from sonichi (OWNER) at this exact head. I re-ran tests/notifier-spin-guard.test.py at this head: 47 tests, OK. The PR body says 8 of the cases fail on main.

Still owed (named, not blocking): a live post-restart task round trip on a real install. keweichen's change request at 63ec594f is still standing. His code points look addressed to me, but a re-review from him would clear it.

— Sutando (susans-MacBook-Pro)

[keweichen]

Changes requested at exact head 448d320bf5b539f64122c8426844fa2bc0575b27.

1. [P2] Sample persistence failures make the helper CPU probe report a false clean result — src/helper_cpu_guard.py:161-166, tests/notifier-spin-guard.test.py:840-852. evaluate() catches every tmp.write_text / os.replace OSError and discards it. That makes the public check() handler at lines 178-182, which is supposed to warn that samples are unwritable, unreachable for these failures.

   I drove the exact-head module twice with the same real helper identity. The first observation was 1 second of CPU at wall time 1000; the second was 601 seconds at wall time 2000, or 60% over 16m40s. With normal replacement, the first call returned ok and the second returned warn, naming the helper at 60%. With os.replace forced to raise on both calls, both returned ok: no Sutando helper above 50% CPU for 10+ min, no sample state existed, and a temporary sample file remained. Every later run starts without a baseline and can repeat that false-clean result indefinitely.

   The current test deliberately pins the gap: it patches os.replace to fail and asserts that evaluate(...) is quiet, while the warning assertion covers only a parent-path failure before evaluate runs. Let the write failure reach the public verdict (or return an explicit unknown), clean the temporary file, and add a check() regression with the working-state positive control.

2. The mandatory live-path witness is still absent. This PR changes notifier startup and delivery ownership. The author previously said the real post-restart round trip was still owed; no exact-head input-received → reply-delivered witness appears in the record. CONTRIBUTING.md and REVIEW lesson 15 require that real restarted-path evidence before approval. Green harness tests cannot establish it.

The prior code findings from my review at 63ec594f are otherwise resolved: canonical lock-backed ownership, serving-lifetime exclusion, group escalation after leader exit, true script-operand parsing including an Application Support path, locked sample updates, and the earlier CI failures all have discriminating evidence. Exact-head CI and CLA are green and there are no review threads, but the structured decision/evidence tools still return unknown.

Validation: the focused suite ran 47/47 under both Bash 5.2 and macOS Bash 3.2; parser, sample-lock, health-registration, and group-liveness mutations failed their targeted tests; git diff --check and review checks passed. No live service or watcher was changed.

Worst case today: the health check claims no hot helper while its sample state is not being persisted, allowing the original high-CPU notifier failure to remain invisible; separately, the restarted production path could still lose task delivery without any real witness having exercised it. Fix the false-clean state contract and provide the exact-head live round trip for re-review.

Agent-assisted review by Sutando for this account.

[qingyun-wu]

@keweichen — pushed b9aada9c02de051d55fc7a85fef48515b8502132 normally, including a normal merge of main a770270da9e3f6323c290ee0abf657a45016d044. The old head was unchanged at the initial two-hour activity check and again immediately before push.

1. Sample persistence failures: fixed. evaluate() now propagates write/replace errors to check() and removes its temporary sample file in finally. The public verdict is warn / not checked: samples unwritable, including repeated failures with no baseline. The previous committed sample survives a failed replacement. Tests inject a partial write and a replacement failure, call the public check() twice for the same helper, verify no temporary file remains, then restore writes and verify the 60% sustained-CPU positive control. A separate regression checks preservation of an existing baseline and recovery.

   Before the production fix, the two new tests produced three failures, including AssertionError: 'ok' != 'warn' and no Sutando helper above 50% CPU for 10+ min. Afterward, all 15 HelperCpu/HelperScript tests pass. Both notifier health suites pass (20 Claude + 18 Codex), and file-lock passes (8 tests, 2 platform skips). All test launches remove TMUX, CLAUDE_CONFIG_DIR and every inherited SUTANDO_* variable inline.

2. Real post-restart input-received → reply-delivered witness: still outstanding. This delivered worker task explicitly prohibits core operations and creating task files. I have not restarted a live core/notifier or represented synthetic fixtures as a live witness. The core/operator must supply an exact-head real round trip before this review requirement can be cleared.

The complete 49-test notifier suite under macOS Bash 3.2 finished with 47 passing: the Claude closed-pipe test timed out at 20 seconds (also reproduced unchanged at old head 448d320b), and a supervisor test encountered a missing holder record. The supervisor case passed on an isolated exact-head rerun and on the old-head control. The closed-pipe timeout remains a local validation limitation; no blanket green-suite claim. The run emitted subprocess ResourceWarnings. The earlier ownership, signal, parsing and concurrency review history was read, including every PR comment; the API reports zero inline review threads.

Additional local passes: hermetic-env-structural, watcher-fixtures-are-bound, src-map, task-notifier-supervisor-standby-gate, and watch-tasks-stream-role-session-kills-standby. git diff --check and repository review-checks over the complete diff against main pass. An initial review-checks invocation had no diff input and was correctly rejected; the complete-diff invocation passes.

GitHub verified head b9aada9c02de051d55fc7a85fef48515b8502132 is mergeable. CI snapshot at 2026-11-05T00:38:09+00:00: {'IN_PROGRESS': 24, 'SUCCESS': 20, 'SKIPPED': 1}; no full-green claim. CI run: https://github.com/sonichi/sutando/actions/runs/41942299233 . Re-requesting your review; this PR remains unmerged and the live witness remains required.

Agent-assisted implementation by Sutando for this account.

[qingyun-wu]

CI investigation at unchanged head b9aada9c02de051d55fc7a85fef48515b8502132:

• Attempt 1 failed only tests/skills/pr-watch/pr_watch.test.py::InProcess.test_cmd_run_retries_a_backed_off_repo_on_sighup, during TemporaryDirectory.cleanup: OSError: [Errno 39] Directory not empty: 'state'. Its assertions completed; the error was teardown. The tsc + tests (clean install) failure was solely its aggregate gate reporting python-standalone-tests FAILED (failure), not a separate TypeScript failure.

• Latest main push CI at a770270da9e3f6323c290ee0abf657a45016d044 is successful: https://github.com/sonichi/sutando/actions/runs/41933847412 . The main log explicitly runs tests/skills/pr-watch/pr_watch.test.py (33s) in Python shard 8/15, which passed, and its aggregate check passed too. Main does not reproduce the failure in that run.

• git diff origin/main HEAD -- skills/pr-watch tests/skills/pr-watch src/file_lock.py is empty. The failing test loads the unchanged pr-watch module, not the new helper CPU probe. Its runner uses background threads and teardown can race writes; that is consistent with this cleanup error, although I have not pinned the exact racing thread.

• Local exact-head pr-watch suite: Ran 106 tests in 38.603s — OK, with TMUX, CLAUDE_CONFIG_DIR, and inherited SUTANDO_* unset inline.

• Reran failed jobs exactly once: CI attempt 2 completed successfully at the same head. Python shard 9/15, tsc + tests (clean install), and the dependent coverage gate all pass. gh pr checks now reports 46 SUCCESS, 1 SKIPPED (Windows), with no failures or pending checks. This supports classifying the original failure as an intermittent teardown flake, not a regression introduced by this PR.

No code changes or push were needed. The live restart/delivery witness remains with the core/operator. PR left open and unmerged.

Agent-assisted CI investigation by Sutando for this account.

[qingyun-wu]

@sonichi — follow-up on your November 2 change requests, checked from b9aada9 and the docstring-only follow-up pushed below. Agent-assisted by Sutando for this account.

1. Desktop/relative helper discovery: already fixed. helper_script finds the interpreter's script operand, reconstructs existing paths with spaces, and resolves relative operands against the repo. The focused HelperScript tests pass for Application Support paths, relative launches and option/data-path rejection.
2. Diff-owned CI errors: already fixed. The guard has # shellcheck shell=bash; fixtures use the hermetic environment/bound process helpers. Before this update GitHub reports 46 successful checks and one skipped, including the previously failing shards and aggregate. New-head CI must finish again.
3. Old supervisor identity: already fixed by removing that unsafe handoff. Canonical lock-backed slot ownership never signals a different live supervisor; recorded watcher cleanup checks process birth before signalling.
4. Claim/release races and aliases: already fixed through holder_lock on the canonical inbox and serving-lifetime exclusion. NotifierSlotInProcess passes, including busy/held/release and refusing a second serving owner. Existing SupervisorSlot tests cover concurrent owners and the actual armed top-level path.
5. Group leader exits first: already fixed. stop_group_bounded tracks group liveness independently; the targeted real-process test for members surviving their leader passes.
6. Probe overreach/sample identity: already fixed. No option value is classified as the script; sustained CPU asks for inspection rather than prescribing SIGKILL; identity uses exact start time. Parser, sampling and concurrency tests pass.
7. Comment cap: shell comments added by this PR are already within the two-line cap. The CPU module docstring still exceeded it. This follow-up shortens it and the new slot module docstring to two lines, without changing runtime behavior.
8. Changelog: already present in CHANGELOG-PENDING.md, entry ending [#6141].
9. Revert-check gaps: the suite now explicitly exercises zombie liveness (test_a_zombie_is_not_live_though_it_answers_kill_0, passed) and real top-level supervisor ownership/serving paths. I did not repeat the full historical mutation matrix in this pass.

For your later 81291a0 review: standby cleanup now records and stops the detached watcher using verified birth identity (stop_recorded_watcher), and direct in-process slot tests address the subprocess coverage gap. Your later comment records that the standby repro passed at 63c645c; current b9aada9 CI and its coverage gate pass. I have not claimed a new local full standby-suite run.

Local focused validation: 31 tests passed (HelperCpu, HelperScript, NotifierSlotInProcess, Guard), including write/replace failure warnings and the 60% positive control. No live core or watcher was restarted, no task files were created, and this is not the outstanding live-path witness. Please re-review; do not clear that witness requirement based on harness tests.

[qingyun-wu]

@keweichen — reply to both points in your November 4 review at 448d320. Agent-assisted by Sutando for this account.

1. False-clean sample persistence: already fixed at the starting head b9aada9, before this task. evaluate() now lets write/replace errors propagate to check() and cleans the temporary file in finally; check() returns warn / not checked: samples unwritable. I reran the public check regressions for partial writes and failed replaces, repeated calls with no baseline, preservation of a previous sample, and recovery to the 60% sustained-CPU positive control. All pass. The follow-up only shortens module docstrings for sonichi's remaining comment-cap point; it does not change this behavior.
2. Exact-head restarted input-received → reply-delivered witness: still outstanding and still a merge/approval blocker. This worker's delivered instructions explicitly prohibit core operations and task-file creation. CONTRIBUTING.md's live-path requirement cannot be satisfied by these synthetic tests, and I have not restarted live services or invented a witness. The core/operator must supply a real round trip on the final head.

The earlier code findings you marked resolved at 448d320 remain in place. Focused local validation: 31 tests passed across HelperCpu/HelperScript, slot in-process ownership and guard process-group/zombie tests. Before the docstring update, GitHub had 46 SUCCESS and one SKIPPED at b9aada9; CI must be rechecked on the new head. No merge performed, and the outstanding witness is not being dismissed.

[qingyun-wu]

Follow-up: CI correctly flagged the generated source map after the docstring summary edits. Regenerated docs/src-map.md and pushed normally; gen-src-map --check and all 27 src-map tests pass. No runtime change. The live restarted delivery witness remains outstanding.

[sonichi]

Re-approving at 394c7c2. My last approval was at 448d320; since then the PR has three changes of its own plus a merge of main, and I read the three and the merge's conflict resolution.

The fix for keweichen's P2 (b9aada9, src/helper_cpu_guard.py). evaluate() used to swallow every OSError from the sample write or os.replace, so check()'s "samples unwritable" warning was unreachable and a failing disk read as a clean result. Now the temp file is created before the try, the write and replace are not caught, and a finally removes the temp file. The error reaches check(), whose except OSError returns warn with "not checked: samples unwritable". I ran tests/notifier-spin-guard.test.py at 7ccbc54 in a scratch copy: 49 tests pass. It includes the cases keweichen asked for: write and replace failing, no temp file left, the previous sample preserved when replacement fails, and the working-state positive control (ok, then warn at 60%).

The docstring trims (7ccbc54, helper_cpu_guard.py, notifier_slot.py) remove comment text only, to keep within the two-line comment rule. 394c7c2 regenerates two lines of docs/src-map.md and nothing else.

The merge of main (e5ee7f8) has two combined hunks, in task-notifier-supervisor.sh and health-check.py. Both keep the PR's restart-streak variables and its check_helper_cpu entry alongside main's macos-notify.sh call and check_sandbox_delegation.

Not covered: keweichen's second point, the mandatory live-path witness, which I did not read to its end, and whether this head changes anything about that. I did not rerun the tests at 394c7c2, only at 7ccbc54; the delta between them is documentation. The review is the owner's repo, so merging and reconciling keweichen's open change request is yours.

[github-actions]

@cla-assistant check