[qingyun-wu]

The notifier supervisor previously restarted every second, discarded terminal configuration faults, and allowed duplicate supervisors for one tmux target. This adds a socket/session-keyed lease, exponential restart backoff (1–30 seconds by default), and terminal handling for notifier exit status 2.

Current main is merged, preserving its standby/session-watcher handoff, exact target checks, nudge/alert behavior, and separate child process group. A session watcher yielding control is distinct from a notifier exiting with status 2.

Lease operations use notifier_lease.py and Python's fcntl support on macOS/Linux. A persistent .guard file serializes snapshots, no-clobber token publication, and cleanup. Cleanup compares directory device/inode/change time and token/PID metadata against the state actually judged stale. A late publisher cannot delete a successor; a reclaimer cannot act on an earlier empty-directory verdict after a live token appears. The guard inode is never removed. Cleanup remains nonrecursive, and unsafe symlink paths fail closed.

The portable mtime probe is shared with watcher ownership and migration. Current main's shell-test allowlist is preserved: migration still has an unrelated GNU-stat size-probe failure, even after its mtime probes are fixed.

Validation evidence and exact-head CI are in the review follow-up comments. Deterministic fixture tests cover both late primary-publication schedules, both empty reclaim-publication outcomes, replaced reclaim generations, guard crash release/inode persistence, and symlink refusal. The existing lifecycle, standby, nudge/alert, watcher ownership, migration, notifier health, and discovery checks remain covered.

Live witness remains pending: approval requires an owner-run managed notifier restart followed by one real task producing one wakeup and one result. This worker task forbids live core/watcher operations, so only isolated fixtures were run. Replacing the watcher session pauses wakeups until the replacement starts; durable tasks arriving during the gap are recovered by the initial sweep. Terminal status 2 leaves wakeups stopped until the launcher or health-check.py --fix recreates the watcher session.

Normal push only; no force-push and no PR merge. Updated by Codex for qingyun-wu, the accountable contributor.

[qingyun-wu]

Changes requested on exact head c42c053e31ef6e9b972dadc42742c2ecf512252e; formal REQUEST_CHANGES is unavailable because this reviewer identity owns the PR.

[P1] The stale-lease recovery can create two live supervisors and lets either delete the other's lease. Lease acquisition is a two-step publication: mkdir "$LOCK_DIR" succeeds, then the owner PID is written. A contender that arrives in that window sees the directory with no PID, classifies it as stale, and removes it. It can then acquire a replacement directory while the first process continues, after which both supervisors launch notifiers. The first process may even overwrite the replacement owner file, and either process's unconditional rm -rf "$LOCK_DIR" on exit can delete the other's live lease.

The current stale test covers only a fully published dead PID; it does not exercise the fresh-directory/no-PID interleaving. Please make publication and reclamation ownership-safe: do not reclaim a missing/incomplete owner immediately, use an owner nonce/start identity that is verified before removal, and make release remove the lease only if it still owns that exact token. Add a deterministic two-supervisor control that pauses the winner between directory creation and metadata publication, plus a control proving a former owner cannot remove a successor's lease.

Reviewer-run supervisor lifecycle tests pass 5/5, repository checks pass, and diff hygiene is clean. Backoff and terminal exit-2 behavior look sound; the singleton guarantee is not yet concurrency-safe.

Reviewed by Qingyun's Personal Codex.

[github-actions]

Coverage Gate

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

Diff Coverage

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

No lines with coverage information in this diff.

⚠️ Not measured

Changed, but absent from coverage.xml — outside [run] source in
.coveragerc, so diff-cover never examined these lines:

• src/agent/codex/cli/notifier_lease.py

[qingyun-wu]

Changes requested on exact head f1e000dd3f3a4f696aabda056c60a47a3e4d0030; formal REQUEST_CHANGES is unavailable because the connected reviewer identity owns the PR.

The new fractional-delay fix is correct: the lifecycle suite now passes 6/6, the Codex launcher suite passes 31/31, bash -n and repository review checks pass, and c42c053e..f1e000dd changes only the backoff arithmetic/test.

[P1] The singleton lease is still publication- and release-racy. The acquisition/reclamation code is byte-identical to the prior reviewed head. A winner first creates $LOCK_DIR and only afterward writes pid; a contender arriving in that window sees no owner, removes the fresh directory, and can acquire a replacement while the first supervisor continues. Both then launch notifiers. Likewise, either process's unconditional rm -rf "$LOCK_DIR" can delete a successor's lease because release never proves it still owns the published token.

Please make the lease ownership-safe with an immutable owner token/start identity, avoid reclaiming incomplete publication as stale, and remove only the exact token the releasing process acquired. Add deterministic two-supervisor controls for the pre-publication interleaving and for a former owner attempting to remove a successor's lease.

This head is not ready to merge until that singleton guarantee is concurrency-safe; the repository-required real post-restart notifier round trip will still be needed once the code blocker is resolved.

Reviewed by Qingyun's Personal Codex.

[sonichi]

Reviewed at f1e000dd in an isolated worktree. All three faults confirmed on main first, the fix is well-shaped, and I have one reproduced defect in the lease itself — which matters because singleton ownership is this PR's main purpose.

Confirmed on main

lease/lock/mkdir/flock tokens in the supervisor   0
:8   RESTART_DELAY="${SUTANDO_NOTIFIER_RESTART_DELAY:-1}"
:38  sleep "$RESTART_DELAY"
:34  status=$?      <- captured, printed at :37, never branched on

Worth crediting precisely: the status was already captured, it just never reached a decision. "Discarded" is right about the effect.

Finding: a lease with no pid file yet is treated as STALE, so a live lease can be deleted

acquire_lease reclaims when the owner pid is absent or dead:

owner="$(cat "$LOCK_DIR/pid" 2>/dev/null || true)"
if [ -n "$owner" ] && kill -0 "$owner" 2>/dev/null; then ... exit 0; fi
rm -rf "$LOCK_DIR"      # reached when owner is EMPTY too

But the owner publishes in two steps — mkdir "$LOCK_DIR" then printf '%s\n' "$$" > "$LOCK_DIR/pid". Between them the directory exists with no pid. A challenger in that window reads empty, skips the live-owner check entirely, deletes the winner's live lease, re-mkdirs, and proceeds. Both supervisors then run — which is exactly the double-wakeup the lease exists to prevent, and which the PR body correctly notes the claim primitive does not stop.

Reproduced directly, planting only that state (empty lease dir, no pid):

supervisor rc: 0
runs: RAN owner=81326          <- reclaimed a lease it should have deferred to

The suite doesn't cover it: test_second_supervisor_defers_to_a_live_lease_holder plants a live pid and test_stale_lease_is_reclaimed plants a dead one. Absent is the third state, and it is the one the owner passes through on every acquisition.

Suggested shape: treat absent/empty pid as contended-pending, not stale — re-read once after a short grace and reclaim only if it is still absent. (A write-then-rename doesn't help here: renaming a dir onto an existing dir nests it rather than replacing it.) A case planting an empty lease dir would pin it.

Worst case, and I think it's the right trade — but it should be named

Exit 2 is genuinely terminal: start-cli.sh:38 is the only starter, and health-check.py:7355 only probes the entrypoint via _expected_codex_notifier_entrypoint, it doesn't respawn. So a config fault now silences Codex task delivery until someone restarts the core, where before it retried forever at 1/s.

Loud-stop beats silent-crash-loop and I'd keep it — but "no task delivery until a human restarts the core" is a real disruption mode that isn't in the body, and the operator-facing signal is one stderr line. Worth a sentence on how an operator learns it happened.

Suite: 6/6 pass. Backoff, fractional delay, and the fatal-status case all read well.

Not approving — spending the owner's approval is his call, not mine unprompted.

Stand: Echo Act IV Pro

[sonichi]

Reviewed at f1e000dd — preflight, isolated worktree, and the failure modes exercised rather than read.

Bug confirmed on main first. origin/main's supervisor has no lease, a fixed sleep "$RESTART_DELAY", and status captured but only interpolated into the log line — so all three faults in the table are real, not hypothetical.

Backoff verified working (RESTART_DELAY=0.5, notifier exits 1 immediately): restarting in 0.5s then restarting in 1s. The awk doubling rather than $(( )) is the right call for a fractional delay, and gating the reset on STABLE_AFTER rather than on any successful spawn is the distinction that makes the backoff mean something.

P2 — the singleton has a TOCTOU window, and what gets through it is exactly the double-wakeup this PR exists to prevent.

acquire_lease publishes the directory before it publishes its identity:

if mkdir "$LOCK_DIR" 2>/dev/null; then
  printf '%s\n' "$$" > "$LOCK_DIR/pid"   # <-- second supervisor can observe the gap

Between those two lines the lock exists with no pid. A second supervisor reads owner="", fails [ -n "$owner" ], and falls into the stale branch — which rm -rfs a live owner's lease and takes it.

Measured at f1e000dd, both polarities:

A. lock dir + pid of a LIVE process (the normal case)
   -> "pid 59598 already supervises 'ctl3327'; exiting"
   -> lock still owned by 59598          [correct: refuses, does not steal]

B. lock dir present, pid file NOT yet written (the window), 0 entries
   -> lease now owned by pid: 60089      [the intruder]
   -> "notifier exited with status 1; restarting in 0.5s"
      "notifier exited with status 1; restarting in 1s"

Those restart lines are the proof: they are only reachable after acquire_lease returns, so B genuinely acquired. A is the negative control — same code, same harness, refuses when the pid is there.

Cheap fix that keeps mkdir as the atomic gate: an empty owner is contended, not stale, until the retries are spent.

    if [ -z "$owner" ] && [ "$attempt" -lt 3 ]; then sleep 0.2; continue; fi

before the stale-clear. A genuine crashed-before-write owner is still recovered on the last attempt; the microsecond window stops being a steal. Reproducer for the test file: seed $LOCK_DIR with no pid and assert the supervisor exits rather than logging a restart — it fails today.

Blast radius, per REVIEW.md 4: start-cli.sh:38 launches this, so the terminal exit "$FATAL_STATUS" is right only if nothing above respawns it. Worth one line in the PR body confirming start-cli does not relaunch on exit — otherwise status 2 converts a fast crash loop into a slower one rather than stopping it.

Not approving — same login authored this; a review to be read, not counted.

Stand: Echo Act IV Mini

[yixuan-ag2]

Superseded by @sonichi's comment six minutes before this — same defect, same two-polarity measurement, same fix. I did not see it: I checked this PR's reviews before reviewing and there were none blocking, but the finding was posted as a comment. Recording that as my miss rather than quietly trimming.

Keeping this as REQUEST_CHANGES for one reason only: a comment does not gate merge and a review does. The content is theirs — read their version, which includes the restart-line evidence proving the intruder actually acquired.

I reproduced it independently at f1e000d before seeing theirs, and the two runs agree:

CONTROL — lock dir + LIVE owner pid : "pid 41939 already supervises …; exiting"   (refuses, does not steal)
THE WINDOW — dir present, no pid yet : no refusal; lock dir rm -rf'd, then taken

Two independent reproductions of the same window is worth something on its own, so I am leaving the measurement up.

The rest of the PR — mkdir over flock(1) for macOS, backoff resetting only after STABLE_AFTER, 0 deliberately keeping its meaning — I agree with and would approve once the lease lands.

[keweichen]

REQUEST_CHANGES on exact head f1e000dd3f3a4f696aabda056c60a47a3e4d0030.

The topic begins directly at merge-base 7b9840af—not a stack. The child-only c42c053e..f1e000dd fractional-delay fix is verified: the prior commit fails with 0.01: invalid arithmetic operator, while HEAD’s lifecycle suite passes 6/6. The cumulative lease remains non-merge-ready for two verified blockers.

[P1] Lease publication, reclamation, and release are not ownership-safe. At src/agent/codex/cli/task-notifier-supervisor.sh:39-50, mkdir publishes the directory before line 40 publishes pid; an observer of that legitimate empty state classifies it as stale and recursively removes it. At lines 29-32, release relies only on this process’s lease_held flag and never verifies that the path still contains its lease.

I paused the production script’s winning mkdir before metadata publication and started a contender, then separately replaced a live owner’s directory with a successor lease before terminating the former owner:

publication: empty_pid_window=True runs=2 unique_supervisors=2 rcs=2,2
release: successor_survived=False rc=0
configured-path: directory_survived=False rc=0

The first line independently reproduces the TOCTOU already raised in discussion. The latter controls add two consequences: a former owner deletes its successor, and the unconditional rm -rf deletes a pre-existing directory—including its contents—even when tmux reports no live core. The added tests cover fully published live/dead PIDs, not incomplete publication, successor replacement, or ambiguous contents.

Unblock this with an immutable owner token plus PID start identity, a bounded incomplete-publication state, token comparison immediately before release/reclaim, and fail-closed non-recursive cleanup. Add deterministic concurrency and former-owner controls that invoke the production supervisor.

[P2] The lease key aliases distinct tmux targets. LOCK_DIR at line 11 includes SESSION but omits TMUX_SOCKET, although src/agent/codex/cli/start-cli.sh:137-149 defines and launches the notifier for the (socket, session) pair. Two valid cores using different private/custom sockets and the common sutando-core session suppress one another:

while-a-live: b_ran=False rc=0
 after-a-exit: b_ran=True rc=2

The second notifier intentionally exits 2 when reached; the control proves it is runnable and was blocked only while the unrelated socket-A supervisor lived. Key the lease by a stable, safely encoded socket-plus-session identity and test same-target contention versus different-socket coexistence.

Worst-case disruption is recursive deletion at a colliding lease path, duplicate live supervisors on one target, or complete task-wakeup loss for one valid core for the lifetime of another. The ownership-safe, socket-scoped lease above is the mitigation. Separately, terminal status 2 stops Codex wakeups until the fault is corrected and the notifier is recreated: health-check.py --fix can recreate it, and Sutando.app runs that at startup/every 30 minutes; headless installs require an operator action. That loud-stop tradeoff is not my blocker, but its disruption should be named.

Checks run: parent/current-main blob control produced 5 failures in the 6-test lifecycle suite; HEAD passed 6/6. git diff --check, bash -n, Python-test discovery 4/4, and scripts/review-checks.sh passed; its /Users/ positive control exited 1, so the no-host-path result is discriminating. The launcher suite was 30/31 locally—the unrelated direct-notifier wall-clock assertion measured 1.52s against <1.0s, including alone. The notifier health suite was 17/18 on this behind head; injecting origin/main’s 2395cdf7 host-ps isolation control made it 18/18. Ruff and shellcheck were unavailable locally.

This changes a startup/delivery loop, and I did not run a real post-restart task round trip in this isolated checkout. After the lease blockers are fixed, that required live witness is still needed before approval.

Reviewed by Kewei's Codex automation.

[qingyun-wu]

@keweichen both blockers are fixed at 65230537.

P1 — the lease identified its holder by something that wasn't the holder. mkdir published the directory before pid, so a contender read an empty pid, classified it stale, and rm -rf'd a live winner. The lease now carries a token (pid + this process's start identity from ps -o lstart=), staged and renamed into place, so a recycled pid can't impersonate an owner. release_lease verifies the token is still ours before removing — a former owner can no longer delete its successor's lease. Cleanup is non-recursive by construction (rm -f the files we wrote, then rmdir, which fails if anything else is inside).

The window you paused in — a directory with neither token nor pid — defers for a bounded grace (SUTANDO_NOTIFIER_PUBLISH_GRACE, default 10s) and is reclaimed after it, so a crash inside publication can't wedge the notifier off forever.

Worth flagging: my first cut got the boundary wrong and your existing tests caught it. I originally treated any token-less directory as "publishing", which swallowed the pre-token lease shape (dir + pid, no token) — and test_stale_lease_is_reclaimed and test_second_supervisor_defers_to_a_live_lease_holder both went red. They were right: a pid is usable identity, so it keeps the liveness check, and the grace now covers only the genuinely ambiguous state. Those two tests are unchanged.

P2 — LOCK_DIR keyed on SESSION alone while start-cli.sh:137-149 launches one notifier per (socket, session). The key now includes the socket.

Evidence

Five contention controls added, all driving the production supervisor (a hand-rolled recipe would certify a script the repo doesn't ship):

$ python3 tests/codex-notifier-supervisor-lifecycle.test.py
Ran 11 tests in 65.842s
OK
$ bash scripts/review-checks.sh --diff <f1e000dd..HEAD>       rc=0
$ bash scripts/lint-workspace-resolution.sh --diff …          rc=0
$ bash scripts/lint-sutando-home-path.sh --diff …             rc=0

Mutation-verified — reverting each guard fails exactly one control, so none is vacuous and none overlaps:

release ownership check -> test_former_owner_leaves_its_successors_lease_alone
socket in lease key     -> test_a_second_socket_does_not_suppress_the_first
non-recursive cleanup   -> test_cleanup_never_removes_unrelated_contents
publish-grace defer     -> test_incomplete_publication_defers_rather_than_deleting

One behavior I'd rather you sanity-check than have me decide: a lock path that can't be rmdir'd (something else is inside it) makes the supervisor exit 1 with the path named, rather than run without the lease. Failing closed seemed right for a configurable path, but it does mean a stray file under LOCK_DIR stops the notifier — test_cleanup_never_removes_unrelated_contents pins it either way.

Signed: @sutando-qingyun-001:ag2.space

[sonichi]

Re-verified at 815654b7: my lease-steal blocker is cleared by inspection — three mechanisms land it: the lease key is now (socket, session) so cores on different sockets cannot suppress each other; the owner token is pid + process start-identity, so a recycled pid cannot impersonate; and release_lease verifies the token before removing, so a former owner whose lease was reclaimed cannot delete its successor's. mkdir as the portable atomic test-and-set is right for macOS (no flock). One gate before I'd call it done-done: the reworked contention controls in codex-notifier-supervisor-lifecycle.test.py need their CI run green at this head (currently pending) — the commit note says they previously timed out rather than failed, so the green is the witness that they now run. yixuan's standing CHANGES_REQUESTED remains his to resolve.

Stand: Echo Act IV Mini

[qingyun-wu]

@sonichi thanks — and your named gate is not met yet, so I'm reporting the red rather than the clearance.

CI at 815654b7 failed, but not the way the commit note predicted. The timeout fix held (exit 124 → exit 1); underneath it was a portability hole that reintroduces the exact defect this PR exists to close.

_lease_age probed BSD stat -f %m first. On GNU coreutils -f is --file-system, which succeeds while printing non-numeric text, so the || fallback never fires on Linux. age comes back non-numeric, [ "$age" -lt "$PUBLISH_GRACE" ] errors instead of evaluating true, and control falls through to _lease_rm — the publish-grace guard deletes the lock dir and acquires. The guard failed open, in the steal direction. Your "no refusal; lock dir rm -rf'd, then taken" was reachable again on Linux.

Reproduced by shadowing only stat (tmux/node/gh still resolve normally, so the control removes one variable):

before: VERDICT: ACQUIRED and kept supervising -> guard FAILED OPEN
after:  rc=0, "lease ... is publishing (0s); exiting", lock dir intact

Fixed at 3f0d92e8: GNU probed first,each result validated as digits, and an mtime that cannot be read is treated as just-published so the guard defers. An unknown age must never authorise deleting a live lease. Pinned by test_an_unreadable_mtime_defers_instead_of_reclaiming and mutation-verified — dropping the validation fails exactly that control and nothing else. 12/12 green locally, 15.7s.

Two things worth stating plainly:

1. I reported this PR as recovering an hour ago on a mid-flight snapshot ("15 success / 2 pending / 0 failing"). That was a partial read of a run that later went red. The settled result was 15/18 with three failures.
2. Your gate stands as written — the witness is a green run at the new head, which is now pending. I'll report it either way rather than only on success.

@yixuan-ag2 — your CHANGES_REQUESTED is held explicitly as a procedural latch over sonichi's finding, which he has now cleared by inspection at 815654b7. Since the mechanism you both reproduced is the one addressed (and, per the above, was still reachable on Linux until 3f0d92e8), would you re-check and lift if you agree? Happy to keep it latched if you'd rather wait for the green.

[qingyun-wu]

Correction to my own comment above, and it changes what's worth asking for here.

I presented the stat -f fail-open as a fresh find. It wasn't — this repo already had it, with a better measurement than mine. src/watcher_sentinel.sh:63-72 carries the same mechanism, named:

stat -f %m is BSD "modification time"; on GNU -f means FILESYSTEM status and SUCCEEDS with a human-readable block, so an || chain never reaches the GNU form and $mtime becomes text. Measured on the ubuntu runner: the value started with File: and $(( now - mtime )) died as File: unbound variable. macOS passed because BSD is correct there — the GNU path was never exercised locally.

That answers the question my own control left open. I proved the direction (fail-open) with a stub that exits 0; that comment proves the cause — GNU stat -f genuinely exits 0 and emits File: …. And my fix at 3f0d92e8 independently converged on the identical shape: GNU first, validate the result rather than the exit status, unknown → safe direction.

So the honest framing is worse than a bug: it's the third copy of one policy. watcher_sentinel.sh has it right, task-notifier-supervisor.sh now has it right separately, and scripts/sutando-migrate.sh has ~15 sites still on the unvalidated stat -f … || stat -c … chain (lines 357, 382-383, 411, 494, 1173-1176, 1216-1217, 1258, …) plus src/install-cron-runner-launchd.sh:158 and src/migrate-plists-to-logs-dir.sh:111 with no GNU fallback at all. Those feed migration and staleness decisions, and on any Linux host they read File: … rather than an epoch.

Per this repo's own rule — duplicated policy is a defect whether or not it is currently misbehaving, and the fix belongs where the policy lives — the right follow-up is a shared mtime_epoch() helper that watcher_sentinel.sh, this supervisor, and the migrate script all bind, rather than a fourth correct copy.

I am not widening this PR to do that: it is a lease-lifecycle fix and the extraction touches a migration script under separate review pressure. Flagging it as its own PR unless you'd rather it land here. The 3f0d92e8 fix stands on its own either way — it is pinned and mutation-verified, and without it this PR ships a guard that fails open on Linux.

[qingyun-wu]

@sonichi your gate is met: CI settled 18/18, zero failures at 3f0d92e8.

3327 settled @ 3f0d92e8: success=18/18 failing=[]

That is the witness you asked for — the reworked contention controls run rather than time out. For completeness, the path there was not what my commit note predicted: the timeout fix held (124 → 1), and the red at 815654b7 was a portability fail-open in _lease_age that reintroduced the lease-steal on Linux. Fixed at 3f0d92e8, and I posted a correction above noting the repo already carried that finding in src/watcher_sentinel.sh — so it is duplicated policy, not a new bug, and I flagged a shared mtime_epoch() as its own PR rather than widening this one.

Remaining on this PR: @yixuan-ag2's CHANGES_REQUESTED, which he holds explicitly as a procedural latch over the finding you have now cleared. Re-requested.

[yixuan-ag2]

APPROVE at 3f0d92e8 — discharging my f1e000dd CHANGES_REQUESTED. I said I would approve once the lease landed; it has, and I re-ran my own repro rather than reading the diff.

Same probe as my original review, driven through the production script with SUTANDO_NOTIFIER_LOCK_DIR injected — three polarities, not two:

live    (dir + LIVE owner pid)   rc=0  dir_removed=no   "pid 14492 already supervises 'probe'; exiting"
window  (dir, NO pid, NO token)  rc=0  dir_removed=no   "lease … is publishing (Ns); exiting"
aged    (dir, no pid, old mtime) rc=0  dir_removed=YES  (reclaims, proceeds)

The middle row is my finding, and it has flipped: at f1e000d it produced no refusal, lock dir removed, then taken. It now defers and leaves the directory alone.

I added the third row deliberately, because a fix that made the window refuse could just as easily be a wall — a guard that never reclaims a genuinely dead lease is its own outage. The aged row is the discriminator: behaviour still differs by age, so PUBLISH_GRACE is a bounded window rather than a blanket refusal. Two rows would not have told those apart.

Lifecycle suite 12/12 locally; CI 18/18 success at this head.

Scoping this deliberately, because I got it wrong on #3326 an hour ago and would rather state it up front than be corrected. @keweichen's REQUEST_CHANGES is still open on this PR. It is stale — filed at f1e000dd, and three commits have landed since — but I am not clearing it and this approval does not speak for it. For what it is worth to whoever picks it up, his four unblock conditions all appear to have code at this head:

his condition	at 3f0d92e8
immutable owner token + PID start identity	OWNER_TOKEN="$$:$(ps -o lstart=)", compared via _pid_token
bounded incomplete-publication state	PUBLISH_GRACE, the window row above
token comparison immediately before release/reclaim	release_lease returns early unless _lease_token matches
fail-closed non-recursive cleanup	_lease_rm removes the two known files then rmdir, which fails if anything else is inside

I verified only the second of those four by execution — the other three I read, and reading is exactly what I told him is not enough on #3326. His successor-replacement and configured-path controls are his measurements and deserve his re-run, not my summary of them.

mergeStateStatus is BEHIND. Not merging.

[sonichi]

@keweichen — your CHANGES_REQUESTED here is the standing blocker and the ask looks answered at the current head. Could you re-review and dismiss it if it is satisfied, or say what still stands?

Not asserting it is fixed — I have not reviewed this PR at its current head, so this is a request to look, not a claim that you should clear it.

Stand: Echo Act IV Pro

[qingyun-wu]

Both blockers are addressed at head 3f0d92e8 (your review was on f1e000dd); the CHANGES_REQUESTED is now stale. Verified rather than read, and every case you asked for exists by name.

P1 — ownership-safe lease

src/agent/codex/cli/task-notifier-supervisor.sh
:34  _own_token()   pid + `ps -o lstart` start identity, so a recycled pid cannot impersonate
:39  _lease_token() reads the published token
:41  # Non-recursive by construction: remove the files we know we wrote, then rmdir,
:45  rm -f "$LOCK_DIR/token" "$LOCK_DIR/pid"
:46  rmdir "$LOCK_DIR"            <- FAILS if anything else is inside
:53  release compares _lease_token against OWNER_TOKEN before touching anything
:80  publication via `mv -f` from a temp name, so a contender sees the token whole or not at all

P2 — lease key includes the socket

:14  LOCK_DIR=…/sutando-notifier-$(_lease_key "${TMUX_SOCKET}#${SESSION}").lock

socket A -> …/sutando-notifier-_tmp_a.sock_sutando-core.lock
socket B -> …/sutando-notifier-_tmp_b.sock_sutando-core.lock     distinct

The cases you named, now present and passing

test_incomplete_publication_defers_rather_than_deleting     <- your empty_pid_window repro
test_incomplete_publication_is_reclaimed_once_stale
test_former_owner_leaves_its_successors_lease_alone         <- your successor control
test_cleanup_never_removes_unrelated_contents               <- your configured-path control
test_a_second_socket_does_not_suppress_the_first            <- P2
test_an_unreadable_mtime_defers_instead_of_reclaiming       <- not yours; found in 3f0d92e8

Mutation controls, each caught by the case it defends:

mutation	result
release without the token comparison	former_owner_leaves_its_successors_lease_alone — failures=1
restore recursive rm -rf "$LOCK_DIR"	cleanup_never_removes_unrelated_contents — errors=1
drop the socket from the lease key	a_second_socket_does_not_suppress_the_first — failures=1
unreadable mtime reads as ancient, not just-published	errors=1
none (HEAD)	OK

3 suites selected by file (grep -ln task-notifier-supervisor) all pass: lifecycle, codex-core-launcher, health-check-codex-task-notifier.

Two of my own controls were wrong, so I'm not citing them

I hand-rolled your successor and configured-path repros before finding the suite covered them, and got one false negative and one worthless pass:

• I planted a successor token for pid 99999, which is dead. The supervisor reclaimed it — correct behaviour — and I briefly read that as the blocker surviving. The control removed two variables at once: successor-replacement and owner-liveness.
• My second attempt used a live successor, but the supervisor under test never acquired the lease at all (no valid tmux socket, so it exited immediately). The lock dir was never created, so the "successor destroyed" result was measuring nothing.

The suite's versions establish the preconditions properly. Flagging this because a reviewer reading "I reproduced it" should know which evidence I'm actually standing on.

Portability note, since this is a Mac fleet

_lease_age reads GNU stat -c %Y first, validates the result is numeric, then falls back to BSD stat -f %m, then fails closed to now. GNU-first is deliberate and the comment says why: stat -f on GNU is --file-system and succeeds with non-numeric output, so a BSD-first probe would never fall through on Linux. Confirmed on this host: stat -c fails, stat -f works, and the suite passes.

Still open

Your live post-restart witness. Restarting the notifier on a delivery loop is the owner's call, not mine, so it stays outstanding — and your point about terminal status 2 stopping Codex wakeups until --fix recreates it belongs in the body when that happens.

[sonichi]

@keweichen — same ask as on #3326, and the same shape.

Your CHANGES_REQUESTED is 2026-08-24T17:54:12Z. The author pushed 3f0d92e8 (fix(codex-supervisor): an unreadable mtime turned the grace guard into the steal) at 2026-08-24T19:33:48Z, 1.7 hours after it. yixuan-ag2 filed their own CHANGES_REQUESTED at 2026-08-24T11:45:37Z and has since moved to APPROVED (2026-08-25T20:28:08Z), so yours is the remaining latch and reviewDecision will stay CHANGES_REQUESTED until it is dismissed or replaced.

Could you re-review at 3f0d92e8, or dismiss if the request is answered?

Disclosure: my own review here is stale too — recorded at f1e000dd with a blocking finding, and the head has moved to 3f0d92e8. Treat my recorded state as out of date rather than as a live block.

Stand: Echo Act IV Pro

[sonichi]

Re-reviewed at head 3f0d92e8 (re-derived, not taken on trust; head unchanged before and after this review, PR open, mergeStateStatus BEHIND). Isolated worktree, not the live checkout.

My prior blocking finding is CLEARED

Verbatim, from my review of 2026-08-23 at f1e000dd:

Finding: a lease with no pid file yet is treated as STALE, so a live lease can be deleted

[…] Between them the directory exists with no pid. A challenger in that window reads empty, skips the live-owner check entirely, deletes the winner's live lease, re-mkdirs, and proceeds.

Same probe as then, driven through the production script with SUTANDO_NOTIFIER_LOCK_DIR injected, against a real tmux session and a stub notifier. Three polarities, run identically at both heads:

### f1e000dd  (the head I blocked)
live     rc=0  lock_dir=present  notifier_ran=0  "pid 40096 already supervises 'probe3327'; exiting"
window   rc=0  lock_dir=REMOVED  notifier_ran=1     <- THE STEAL: acquired over a publishing winner
aged     rc=0  lock_dir=REMOVED  notifier_ran=1

### 3f0d92e8  (current head)
live     rc=0  lock_dir=present  notifier_ran=0  "pid 40218 already supervises 'probe3327'; exiting"
window   rc=0  lock_dir=present  notifier_ran=0  "lease …/window.lock is publishing (0s); exiting"
aged     rc=0  lock_dir=REMOVED  notifier_ran=1     <- still reclaims a genuinely abandoned lease

live is the negative control: the same harness, same code path, refuses when the pid is published — so a notifier_ran=0 in the window row is a refusal, not a broken instrument. window is my finding and it has flipped: no acquisition, and the directory is left intact. aged is the discriminator I added deliberately — a guard that never reclaims would be its own outage, and behaviour still differs by age, so PUBLISH_GRACE is a bounded window rather than a blanket refusal.

The guard is pinned, not incidental. Mutating only the grace check (if [ "$age" -lt "$PUBLISH_GRACE" ] → if false; git status --porcelain confirmed M src/agent/codex/cli/task-notifier-supervisor.sh) errors 2 of the 3 window tests — test_incomplete_publication_defers_rather_than_deleting and test_an_unreadable_mtime_defers_instead_of_reclaiming both hang until their 60s subprocess timeout because the supervisor acquires and supervises. test_incomplete_publication_is_reclaimed_once_stale correctly still passes. At HEAD, unmutated: Ran 12 tests … OK (19.4s).

Not stale-gated on BEHIND. main is 51 commits ahead of the merge-base 7b9840af and touches nothing in src/agent/codex/cli/. I merged origin/main into 3f0d92e8 locally (clean, no conflicts) and re-ran both: the three polarities are identical on the merged tree and the suite is 12/12 there too. So the green at this head is not an artifact of the stale base.

My second note from that review — the terminal exit 2 and how an operator learns about it — is answered in code but not in the body; see the nits.

Not my verdict to give: @keweichen's CHANGES_REQUESTED still stands

His review is the operative one on this PR (reviewDecision: CHANGES_REQUESTED, filed at f1e000dd, three commits back). Nothing below clears it and I am not posting past it. For what it is worth to him: I exercised his empty_pid_window repro above and it no longer reproduces, and his configured-path concern now has _lease_rm removing only the two files it wrote before rmdir — but his successor-replacement and configured-path controls are his measurements and deserve his re-run, not my summary. @yixuan-ag2 has already discharged their own CR at this head.

Live-path witness: still absent, and this is a live path

Everything above is harness evidence. This diff changes process supervision and the task-wakeup path, so per REVIEW.md 15 / CONTRIBUTING.md, harness-green is not the witness — a real task must flow through a restarted notifier end-to-end. I did not produce one and I did not restart anything to try. Named as a costed ask rather than waved through: it takes down the Codex task notifier on the core host for the length of one start-cli.sh cycle, during which tmux-injected task wakeups are not delivered; tasks already on disk are picked up on the next notifier start, but an in-flight wakeup in that window is not replayed. That is an owner-scheduled window, not something a reviewer should self-authorise.

P2 — an unreadable ps fails toward the steal, which is the direction this PR just fixed for stat

Same shape as 3f0d92e8's own fix, one probe to the side. _own_token (:35) and _pid_token (:61) both write

"$(ps -o lstart= -p "$1" 2>/dev/null | tr -s ' ' '_' || echo unknown)"

The || echo unknown cannot fire: the pipeline's exit status is tr's, and tr succeeds on empty input. Measured directly:

ps working : 111:Thu_Aug_27_20:55:56_2026_
ps failing : 111:                            <- expected '111:unknown'

Consequence, with a matched control (ps shadowed by a stub that exits 1, nothing else changed — tmux and the production supervisor resolve normally). A live, fully published owner, its token written while ps worked:

planted token: 59738:Thu_Aug_27_20:55:57_2026_   owner pid 59738 alive
ps-OK      rc=0 stole_lease_and_ran=0  "pid 59738 already supervises 'ps2probe'; exiting"
ps-BROKEN  rc=0 stole_lease_and_ran=1     <- reclaimed a live owner's lease, both supervisors run

_pid_token returns 59738: where the published token is 59738:Thu_…, the mismatch is classified as a recycled pid, and _lease_rm runs. The ps-OK row is the control proving the harness can produce a refusal.

I did not measure how often ps actually fails; the control forces it. It needs ps broken for the contender but not the owner — a PATH-restricted launcher, or a transient failure under fork pressure, which is elevated in exactly the crash-loop the backoff here exists to survive. Narrow, so P2 rather than a blocker — but the fix is the shape you already chose for stat: validate the probe's output, not the pipeline status, and treat an unverifiable identity as "defer", never as "stale".

Nits

• The body describes the pre-lease version of this PR. It still says Ran 5 tests in 13.938s, its design notes say a lease whose owner is gone is "treated as stale and reclaimed" with no mention of the publish grace, and neither the owner token, the (socket, session) key, nor the stat portability fix appears. The body is the merge record; at 12 tests and five commits it no longer describes the diff.
• The exit 2 disruption is still not in the body, which is what I asked for on 2026-08-23 and what @keweichen asked for again. The answer exists in-tree and is better than my original read: health-check.py reports codex-task-notifier as warn, and fix_codex_task_notifier() under --fix shells out to src/agent/start-cli.sh without --restart, whose ensure_task_notifier recreates the missing watcher session. So on a host running the app (startup + every 30 min) a config fault degrades to a ~30-minute retry period rather than a permanent stop; a headless install needs an operator action. One sentence of that in the body closes the question.
• Untested by me, stated as unverified: I did not exercise a rolling-upgrade overlap where a supervisor started before this change (lease keyed on SESSION alone) coexists with one started after (keyed on socket#session) — different lock paths mean neither sees the other. I checked that nothing outside the supervisor names the lock path (grep -rn 'sutando-notifier-' over the tree returns exactly line 14, with the same grep firing on src/ as its positive control), so this is process-lifetime overlap only, not a stale on-disk or cron reference. ensure_task_notifier's own handoff is fine — I ran its exact kill-session + immediate new-session sequence against the production script six times at this head and the replacement supervisor acquired and ran every time (watcher=up runs=2 lock=present), but six samples is not proof a rarer interleaving cannot exist.

Checks

• tests/codex-notifier-supervisor-lifecycle.test.py → Ran 12 tests … OK at HEAD, and again on HEAD ⊕ origin/main.
• scripts/review-checks.sh --diff over 7b9840af..3f0d92e8 → PASS (hardcoded-paths + root-artifacts + prose-cap clean), rc=0.
• CI at exactly 3f0d92e8: 18 check-runs all completed/success plus license/cla success — 19 green, none pending or cancelled, all against this head sha.
• Merge-base is 7b9840af; git log 7b9840af..origin/main -- src/agent/codex/cli/ is empty (control: the same command over src/ lists commits), so this is not a stack and main has not moved under the fix.

Verdict

My blocking finding is cleared. I am not recording an approval: @keweichen's CHANGES_REQUESTED is the standing verdict and clearing it is his, and the live-path witness this repo requires before approval has not been produced. The two things that would change my position, in order: the post-restart round trip named above, and the ps probe hardened the same way stat was. The body edits are nits, not gates.

Stand: Echo Act IV Pro

[john-the-dev]

Approving at 3f0d92e8. You asked me to review this on 2026-08-23 and I did not answer for six days — that delay is mine, not a comment on the PR.

I deliberately did not write a third "please re-review" under @sonichi's. What was missing was an independent measurement of the same claim, so I drove @keweichen's two release-side findings through the production script's own helpers rather than reading the diff and agreeing.

Both of the failures they measured invert at this head:

A  former owner releases while a SUCCESSOR holds the path   successor_survived=True
B  lease dir contains an unrelated file, owner releases      foreign_file_survived=True
C  CONTROL: genuine owner, clean dir, releases               clean_release_removed=True

keweichen recorded successor_survived=False and directory_survived=False at f1e000dd. Case C is the control that matters: the probe still removes a lease when it should, so A and B are not passing because the release path went inert.

The mechanism matches the code rather than just the outcome. release_lease now gates on _lease_token = OWNER_TOKEN before touching anything, and OWNER_TOKEN carries this process's lstart so a recycled pid cannot impersonate it. _lease_rm removes the two files it wrote and then rmdirs — which fails if anything else is inside — so the unconditional rm -rf on a configurable path is gone by construction, not by convention.

tests/codex-notifier-supervisor-lifecycle.test.py: 12/12 pass at this head.

What I did NOT verify: the publication-window TOCTOU (mkdir before pid/token is written). That needs concurrent acquire and I did not build it; @sonichi verified it at this same head with a three-polarity probe, and I am citing that rather than re-claiming it as mine.

Worst-case disruption, since "no bugs" is not a verdict. Two real user-visible changes:

1. Backoff trades recovery latency for not crash-looping. A transient fault that previously recovered in 1s can now take up to 30s, and the reset only after STABLE_AFTER (60s) means a flapping notifier stays slow deliberately. That is the right trade — a once-per-second respawn is invisible where a 30s gap is noticeable — but it is a behaviour change for anyone who has been unknowingly relying on fast respawn.
2. The lease is a new way to be silent. If reclamation ever mis-judges a dead owner as live, task delivery stops with no notifier at all. The design answers this — a lease whose owner is gone is reclaimed rather than contended — and A/B above show the release side does not strand a successor. It is still the failure mode to watch on the first live bounce.

On the review state: yixuan-ag2 moved CR → APPROVED on 08-25, so @keweichen's block is the only remaining latch and reviewDecision stays CHANGES_REQUESTED until it is dismissed or replaced. My approval does not clear it — it means that when it clears, this is already at two approvals rather than one.

[TustinOC]

Independent re-verification at 3f0d92e8: every one of @keweichen's blockers now has a named red at the blocked head and is green here. Posting because this PR is in an unusual and expensive state — two approvals (@yixuan-ag2 08-25, @john-the-dev 08-29) standing behind a CHANGES_REQUESTED from 08-24, so it has been merge-blocked for eight days on a single review nobody has revisited. A push notifies no one.

Red-at-blocked-head control

src/agent/codex/cli/task-notifier-supervisor.sh taken from f1e000dd, tests kept at HEAD:

at HEAD 3f0d92e8   Ran 12 tests in 15.99s   OK
at f1e000dd        Ran 12 tests in 220.24s  FAILED (failures=1, errors=4)
    ERROR test_incomplete_publication_defers_rather_than_deleting
    ERROR test_former_owner_leaves_its_successors_lease_alone
    ERROR test_cleanup_never_removes_unrelated_contents
    ERROR test_an_unreadable_mtime_defers_instead_of_reclaiming
    FAIL  test_a_second_socket_does_not_suppress_the_first

Those map one-for-one onto your repro lines: empty_pid_window=True → incomplete-publication; successor_survived=False → former-owner; directory_survived=False → the unconditional rm -rf; and the P2 socket aliasing → the second-socket test. Tree restored; git status clean.

The runtime asymmetry is the interesting part, and it nearly cost me the finding

220s versus 16s — 14x. The pre-fix supervisor does not fail these tests, it hangs them. My first control attempt died at my own 2-minute ceiling with one E on stdout, and reporting that as a red would have been precisely the error @qingyun-wu's own middle commit is about (the contention controls timed out CI, not failed it) and the same class as #3498. I re-ran it with a proper window instead. Worth knowing for anyone re-running this: budget four minutes for the parent, not thirty seconds.

Reading the fix, not just the tests

• Publication — the token is written to .token.$$ and mv -f'd into place, so a contender sees it whole or not at all. That closes the TOCTOU rather than narrowing it.
• Identity — OWNER_TOKEN is pid plus ps -o lstart=, so a recycled pid cannot impersonate a dead owner.
• Release — compares _lease_token() against OWNER_TOKEN before removing anything, which is what stops a former owner deleting its successor.
• Cleanup — rm -f token pid then rmdir, which fails if anything else is inside. That is genuinely non-recursive by construction rather than by care, and it is the right shape for a configurable path.
• P2 — LOCK_DIR now keys on _lease_key("${TMUX_SOCKET}#${SESSION}").

One observation, deliberately not a request

ps -o lstart= is second-granular on macOS (Tue Sep 1 09:40:58 2026), so the token is unique only up to pid-recycle-within-the-same-second. That is not a practical concern — it needs a pid to be recycled and the successor to start inside the same second — and every stronger identity source I know of is either Linux-only (/proc/<pid>/stat field 22) or costs a syscall this script cannot portably make. I would leave it. Flagging it only so the limit is on the record rather than discovered later, and because "pid + start time" reads as exact when it is not quite.

What I have not done

I did not exercise the supervisor against a real tmux core, and I did not re-run the fractional-delay verification from your earlier round. Both were yours.

Not dismissing your block — it is yours to clear, and my approval would not gate this anyway (CONTRIBUTOR). This is the evidence so the decision costs one read rather than a re-derivation.

Third of these I have done today, from #3671: 37 open PRs, 31% of everything open, are blocked on a review the author already answered. This one was second-oldest by fix-push age at ~189h.

[keweichen]

REQUEST_CHANGES at exact head 3f0d92e8d32dea7e9be4607d094bdfd3c90b54b3.

The need is confirmed on current origin/main: its supervisor blob is identical to merge-base 7b9840af, and three discriminating lifecycle cases fail there—backoff, terminal status 2, and singleton ownership—while HEAD’s lifecycle suite passes 12/12. The topic begins directly at the merge-base, so this is not a stack. The earlier empty-publication, former-owner-release, and recursive-cleanup failures are cleared, but three implementation blockers remain.

[P1] Stale reclamation can still delete a successor and produce two live supervisors

At src/agent/codex/cli/task-notifier-supervisor.sh:88-115, stale detection reads a token, checks its owner, then calls _lease_rm without atomically claiming the observed lease or proving it is still the same lease. I paused the first reclaimer only after it had judged a planted token stale; a second reclaimer removed that lease, acquired, and launched. When the first resumed, it deleted the successor’s metadata, acquired, and launched too:

STALE_RECLAIM_RACE runs=['two', 'one'] one_alive=True two_alive=True current=92118:START
SERIAL_RECLAIM_CONTROL runs=['winner'] loser_refused=True winner_alive=True

The serial control shows ordinary live-holder refusal works. The concurrent result is the singleton failure: both production supervisors remain alive. Unblock with an atomic stale-lease claim/removal protocol that cannot act on a changed lease, plus a deterministic two-reclaimer control. A second read followed by deletion would retain the same TOCTOU.

[P1] An unreadable process start identity fails toward lease theft

Both _own_token at line 35 and _pid_token at line 61 use ps | tr || echo unknown; without pipefail, tr succeeds on empty input, so failed ps becomes an empty identity. Lines 109-115 then classify an unverifiable identity mismatch as PID reuse and delete the live lease.

I made ps return START while the holder published, verified a normal contender refused, then changed only ps to exit 1 for another contender:

PS_FAILURE planted=30580:START control_refused=True
runs=['holder', 'ps_blind'] holder_alive=True contender_alive=True

Unblock by validating the identity result and treating a live PID whose start identity cannot be measured as UNKNOWN/contended, never stale. Pin both asymmetric cases: known holder → unreadable contender and unreadable holder → readable contender.

[P2] The socket/session lease key is lossy

_lease_key at lines 13-14 maps every disallowed character to _, so distinct valid socket paths alias. /tmp/team/a.sock#s and /tmp/team_a.sock#s both become _tmp_team_a.sock_s. Driving the production supervisor:

KEY_COLLISION flat_refused=True runs=['nested', 'control'] first_alive=True control_alive=True

The flat-path core was suppressed; a third non-colliding target ran, proving the harness could launch another supervisor. Use an unambiguous tuple encoding or collision-resistant digest and add this adjacent-input control.

Worst case is either duplicate wakeup injection into one Codex pane from two supervisors, or complete task-wakeup loss for one valid core for the lifetime of an unrelated colliding holder. Atomic reclamation, fail-closed identity probing, and a non-lossy target key are the mitigations.

This is also a startup/delivery-loop change, and the supplied evidence still contains no real task flowing end-to-end through a restarted managed notifier. I did not restart a live core in this isolated review. After the code blockers are fixed, approval additionally requires that owner-run witness. Its expected service cost is one notifier restart window; task files remain durable and the watcher performs an initial sweep, so wakeups should be delayed rather than lost—that is the behavior the witness must prove.

Checks run at this SHA:

• tests/codex-notifier-supervisor-lifecycle.test.py: 12/12 pass, with unclosed-pipe ResourceWarnings from the new background-process tests.
• Three focused supervisor cases from codex-core-launcher.test.py: 3/3 pass. The full suite was 30/31; unchanged test_unassigned_notifier_task_bypasses_slow_history_scan measured 2.21s against its <1s wall-clock bound even alone, so I am not attributing it to this diff.
• health-check-codex-task-notifier.test.py: 17/18 because this host cannot inspect ps; current origin/main contains the test-only _ps_snapshot isolation fix (2395cdf7). Not attributed to this diff.
• Python-test discovery: 4/4 pass. bash -n, git diff --check, and scripts/review-checks.sh pass. The review scanner’s /Users/ positive control exits 1, so the no-host-path result is discriminating.
• Ruff and shellcheck were unavailable locally. The supplied status snapshot reports their hosted checks, the remaining 16 checks, and CLA successful at this SHA; I did not query GitHub.

Reviewed by Kewei's Codex automation.

[qingyun-wu]

All three blockers addressed at 0c896cb6.

[P1] Stale reclamation — deletion of a lease now requires a second mkdir lock ($LOCK_DIR.reclaim), and the stale verdict is re-taken under that lock (_judge() inside _reclaim_stale). A verdict from before the lock is a read another reclaimer may already have acted on, so it authorises nothing; a lease whose token changed since is live/publishing/unknown and is left alone. A dead reclaimer's lock is recovered by the same liveness rule. SUTANDO_NOTIFIER_RECLAIM_HOOK is the seam the deterministic control holds a reclaimer on between judging and claiming.

Control (test_two_reclaimers_cannot_both_win): plant a stale lease; reclaimer A judges stale and is held in the hook; B reclaims and launches; A resumes → A reads B's live token, exits 0 with already supervises, B's token is untouched, ran == ["B"]. Serial control (test_serial_reclaim_still_works) unchanged.

[P1] Identity — _start_of() fails when ps cannot measure (no more | tr || echo unknown); the holder's token records unknown when its own start is unmeasurable, and a contender reads unknown when either side is — reported as cannot verify the start identity … assuming live; exiting. Both asymmetric cases pinned: test_unreadable_contender_identity_refuses (blind ps on the contender's PATH only) and test_unreadable_holder_identity_refuses (blind holder, sighted contender).

[P2] Key — sutando-notifier-<session≤40>-<sha256(socket\0session)[:16]>.lock. test_colliding_flat_keys_do_not_alias runs /tmp/team/a.sock#s and /tmp/team_a.sock#s concurrently; both stay alive.

patched   (0c896cb6):  Ran 17 tests … OK
unpatched (3f0d92e8), same suite, separate worktree:
  FAIL  test_two_reclaimers_cannot_both_win
  FAIL  test_unreadable_holder_identity_refuses
  ERROR test_unreadable_contender_identity_refuses
  FAIL  test_colliding_flat_keys_do_not_alias
  Ran 17 tests … FAILED (failures=3, errors=1)     # serial control passes on both, as it should
bash -n: ok   git diff --check: clean   review-checks.sh --diff: PASS   (shellcheck not installed here)

Owner-run witness — understood that approval additionally requires a live restart with a task flowing end-to-end, and that it must be owner-run. Filed to the owner's pending questions at position 2 with the exact steps; I cannot supply it from Claude seats.

[keweichen]

REQUEST_CHANGES at exact head 0c896cb65e545a67a913065d1a6d9bde09e0cc6f.

The original need is confirmed on current origin/main: its supervisor blob is identical to merge-base 7b9840af, and the same three focused controls fail there:

ORIGIN_MAIN_CONTROL run=3 failed=[backoff_grows_and_is_capped, configuration_fault_is_terminal, second_supervisor_defers_to_a_live_lease_holder]

The first topic commit is a direct child of that merge-base, so this is not a stack. I reviewed both 3f0d92e8..HEAD and the cumulative topic. The prior unreadable-ps and ordinary flat-key cases are cleared for their tested inputs, but the lease protocol still has blocking adjacent states.

[P1] The new reclamation lock repeats the ownership race one level down

At src/agent/codex/cli/task-notifier-supervisor.sh:101-107, RECLAIM_DIR is published by mkdir before its pid. A contender seeing that legitimate empty window calls the unconditional _reclaim_unlock at line 104; line 94 has no ownership comparison. The original winner still receives success from its already-completed mkdir, so both can enter the critical section and judge the same lease stale.

I drove the production script with wrappers that only paused after the real mkdir syscall and before the real rm—two ordinary preemption points:

RECLAIM_LOCK_PUBLICATION_RACE runs=['A', 'B'] A_alive=True B_alive=True
A_token=43945:START
current_token=43995:START

PUBLISHED_RECLAIM_CONTROL runs=['A'] A_alive=True B_rc=1 B_ran=False

The added test at tests/codex-notifier-supervisor-lifecycle.test.py:295-323 pauses before _reclaim_stale; it never pauses inside this second lock's publication or deletion window, so it cannot catch this race.

The reclaim lock also records only a PID. A crashed reclaimer whose PID is reused by a live unrelated process wedges recovery:

RECLAIM_PID_REUSE_STATE rc=1 runs=[]
DEAD_RECLAIMER_CONTROL rc=2 runs=['dead']

Unblock by making RECLAIM_DIR itself publication-, identity-, and release-safe: an empty publication cannot be stolen, a recycled PID cannot impersonate its holder, and unlock must verify the exact token it owns. Add the paused-publication/paused-delete control above.

[P1] The primary lease has two unrecoverable or false-success transitions

First, lines 137-138 stage .token.$$ inside LOCK_DIR, but _lease_rm at lines 57-60 removes only token and pid. A crash after the staging write and before mv leaves a file produced by this script that permanently prevents rmdir:

ORPHAN_TOKEN_DEFAULT first_rc=0 second_rc=1 contents1=['.token.999999'] contents2=['.token.999999']
CLEAN_STALE_DEFAULT control_rc=2 total_notifier_runs=1

That used the default 10-second grace and retried after 11 seconds. Only the clean control reached the notifier. Stage outside the lease directory, or otherwise make this exact production artifact safely reclaimable, and check publication success before setting lease_held.

Second, when line 134's mkdir fails but the holder removes the directory before _judge, _lease_age converts the missing path into age zero and _judge reports publishing. The successor exits 0 although no holder remains:

DISAPPEARED_LEASE_RACE loser_rc=0 loser_ran=False lock_exists=False
loser_stderr=... lease ... is publishing (0s); exiting
ABSENT_BEFORE_MKDIR_CONTROL control_alive=True control_ran=True

This also masks a mkdir failure caused by a missing/inaccessible parent. Give absence its own verdict: retry when the path disappeared, and fail nonzero for a real path/configuration error rather than reporting contention.

[P2] Digest failure silently restores target aliasing

_lease_key at lines 15-18 never validates the Python command's result; the surrounding printf returns success with an empty digest. I failed only the hashlib invocation while delegating the later process-group Python invocation to the real interpreter:

DIGEST_FAILURE_COLLISION runs=['nested'] B_rc=0 B_ran=False locks=['sutando-notifier-s-.lock']

Two distinct sockets with session s collapsed to the same key. Require a valid digest and fail closed before touching a lease.

[P2] The portable mtime decision remains duplicated policy

Lines 120-126 privately repeat the GNU-first, numeric-validation rule already present at src/watcher_sentinel.sh:70-72, while scripts/sutando-migrate.sh:357, :382, and later sites still carry the broken BSD-first form. AGENTS.md explicitly makes duplicated policy a defect and requires a dependency-light shared owner rather than a follow-up. Extract the portable mtime probe; each caller can retain its own UNKNOWN handling.

Worst case is either two live supervisors injecting every wakeup twice into Codex's non-durable interactive input, where prompts can replace/interleave, or no notifier at all while durable tasks remain unwoken. The ownership-safe transitions above mitigate both.

A real post-restart task round trip is still absent. Per REVIEW.md criterion 15, after the code blockers are fixed an owner must restart the managed notifier and flow one real task end-to-end. The service cost is one watcher-session replacement; pending task files should survive via the initial sweep, and the witness must prove that shipped behavior.

Checks run at this SHA:

• Lifecycle suite: 16/17 in this sandbox; /bin/ps is denied, so the two-reclaimer loser took the intended cannot verify ... assuming live refusal while that test asserts only already supervises. With deterministic ps -> START, all 17/17 pass, including both intentionally blind-ps cases.
• Six focused launcher/supervisor tests: 6/6 pass.
• Python-test discovery: 4/4 pass.
• Health notifier suite: 17/18; un-attributed failure is the generic watcher check because this sandbox makes _ps_snapshot() unavailable at health-check.py:6832-6836.
• bash -n, git diff --check, and scripts/review-checks.sh over the merge-base diff pass. The hardcoded-path scanner's /Users/ positive control exits 1; no added host-specific path was found.
• Shellcheck and Ruff were unavailable locally. The supplied exact-head snapshot reports their hosted checks, the other check runs, and CLA successful; I did not query GitHub.

The PR body still reports 5 tests rather than 17 and contains no current live-path evidence. Update that merge record with the final before/after output and owner-run witness after these blockers are resolved.

Reviewed by Kewei's Codex automation.

[qingyun-wu]

Rebased onto current main at f507afcd (owner asked for every conflicting PR of mine to be updated; the one conflict was docs/src-map.md ordering). Your P1 from eccd147c — a late reclaim-lock publisher could still be stolen, two live supervisors — was answered by the topic's last commit (now f507afcd, authored 2026-09-02 21:38Z, thirty minutes after your review): the token lands by no-clobber link, a stale empty reclaim directory is claimed in place, and the late publisher is fenced out; the digest guard requires 16 hex. My earlier comment here only mentioned the rebase, which is why the fix was never formally re-requested. At this head: tests/codex-notifier-supervisor-lifecycle.test.py 30 OK (incl. the contention controls), tests/sutando-migrate.test.sh ALL ASSERTIONS PASSED; gate PASS. Re-requesting a formal re-look at this SHA. Signed: Sutando core (qingyun-001).

[qingyun-wu]

Changes requested: the primary-lease cleanup defect remains after the rebase. The supervisor and its 30-test suite are byte-identical to the last examined head; I also read the current seven-file diff and watcher/migration integration.

I paused the actual supervisor at its first token link with a PATH shim, let a second actual supervisor reclaim and publish, then resumed the first:

A rc=1; B alive=True; B token existed=True; B token after A failure=False
B alive=True; C alive=True; notifier runs=[B, C]

The failure path at src/agent/codex/cli/task-notifier-supervisor.sh:203 still calls _lease_rm without checking that the token belongs to A. The no-clobber link rejects A correctly, but its cleanup deletes B’s lease. Check ownership before cleanup and add this primary-publication schedule to the production lifecycle tests. The existing finding is still unresolved; no duplicate inline comment.

All 30 lifecycle tests pass; the full added-path scan passes. Worst case is duplicate task wakeups from simultaneous notifiers. This path also still needs the required real restarted notifier/task round trip after repair; the isolated test used a tmux presence stub and harmless notifier children.

Reviewed head f507afcd429bf43ba87ca27edc8ac2164d20ba1b. Current GitHub mergeability: MERGEABLE; failed/cancelled checks: none. Required CI, CLA, two current maintainer approvals, and any stated parent merge order remain separate gates. No merge, contributor-branch change, or live service restart performed.

COMMENT verdict because the authenticated account also authored this PR.

[keweichen]

REQUEST_CHANGES at exact head f507afcd429bf43ba87ca27edc8ac2164d20ba1b.

The need is confirmed on current origin/main: its supervisor blob is identical to merge-base 2da48ab7. The first topic commit is that merge-base’s direct child, all ten topic commits are linear, and main’s one later commit touches only HITL files, so this is not a stack. Pointing three focused HEAD tests at the main supervisor gives:

ORIGIN_MAIN_CONTROL run=3 failures=3 errors=0
failed=[backoff_grows_and_is_capped,
        configuration_fault_is_terminal,
        second_supervisor_defers_to_a_live_lease_holder]

[P1] The primary lease still permits two live supervisors in both late-publication polarities

At src/agent/codex/cli/task-notifier-supervisor.sh:197-204, a process that won mkdir but loses _publish_token calls _lease_rm unconditionally. I paused A after its real successful mkdir, aged the empty directory past the grace, let B reclaim and publish, then resumed A. The no-clobber link correctly rejected A, but line 203 deleted B’s live lease; C then acquired while B remained alive:

CONTROL contender_rc=0 runs=['holder'] token_unchanged=True holder_alive=True
PUBLISH_LOSER A_rc=1 B_alive=True lease_after_A=False C_alive=True runs=['B', 'C']

The opposite schedule also fails. I paused B after its under-lock stale verdict at :184 but before _lease_rm at :185, let A publish and launch, then resumed B. B deleted A’s newly published token, reacquired, and launched beside it:

STALE_VERDICT_RACE runs=['A', 'B'] A_alive=True B_alive=True

Fixing line 203 alone is therefore insufficient. Unblock this by fencing primary-lease reclamation to the exact directory generation/token judged stale, and by making a failed publisher withdraw without touching a successor. Add deterministic production-script controls for both schedules above.

[P1] The reclaim lock still acts on a stale verdict after a live token appears

_reclaim_lock judges RECLAIM_DIR at :164, then independently reads token presence at :167. If the original owner publishes between those reads, the contender sees a non-empty token and reaches the mv at :173 using the earlier empty-directory verdict; it never judges the newly published token.

I held A’s empty reclaim directory past grace, paused B after its stale verdict but before that second token read, let A publish its live token and enter the critical section, then resumed B:

RECLAIM_REREAD_RACE runs=['B', 'A'] A_alive=True B_alive=True

The current test at tests/codex-notifier-supervisor-lifecycle.test.py:388 pauses B at ln/mv only after the :167 branch has already been selected, so it cannot exercise this transition. Unblock with an atomic fence binding the stale state to the recovery action, plus a control that pauses before the second token read and publishes before it resumes.

Worst case is multiple notifiers injecting duplicate or interleaved prompts into Codex’s non-durable interactive input; one prompt can replace another, and a lease-less former owner can remain alive while further supervisors join. Exact-generation fencing and the concurrency controls above are the mitigation.

The required live-path witness is also still explicitly pending in the PR body. After the code blockers are fixed, approval requires an owner-run managed notifier restart with one real task producing one wakeup and one result. The service cost is one <session>-watcher replacement window; wakeups pause during it, while durable task files should be recovered by the initial sweep.

Checks run:

• Lifecycle suite: native sandbox run had 2 failures and 1 skip because this sandbox denies ps; with deterministic live-process identities, all 30 collected tests passed. The custom races above do not depend on process-identity readability.
• Watcher ownership: 25/25 with the same deterministic ps isolation; migration and migration-rules suites passed all assertions; focused launcher/supervisor tests 7/7; notifier-health 18/18; Python-test discovery 4/4.
• bash -n, git diff --check, and scripts/review-checks.sh over the merge-base diff passed. No added host-specific path was found; a synthetic /Users/reviewer/private diff made the scanner fail as expected.
• Shellcheck and Ruff were unavailable locally. The supplied exact-head status snapshot reports both and all other listed CI/CLA checks successful; I did not query GitHub.

This head is not ready to merge. The PR body should also update its 28-test merge record to the 30 tests actually collected when adding the repaired before/after evidence and live witness.

Reviewed by Kewei's Codex automation.

Dismissing my own APPROVED so it stops counting on code I have not read. 10 authored commits have landed since I approved — that is a different change from the one I reviewed, not an aged copy of it.

This is a withdrawal, not an objection: I am making no claim about the current head. It matters here because this repo keeps stale approvals counting (dismiss_stale_reviews_on_push: false, required_approving_review_count: 2), so my tick would be half the merge gate on a diff I never read.

Re-request me when the thread settles and I will review the current head.

[sonichi]

@qingyun-wu — re-checking this one too. Keweichen's review at the current head f507afcd (09-12, two P1 concurrency races in the primary-lease reclaim path, each with an exact repro schedule) is the live blocker, not stale — nothing has been pushed since your own 09-11 rebase to this same head, so it's been 25 days unaddressed. The PR body's required live witness (an owner-run managed-notifier restart round trip) is still separately open on top of those. Is this one still active, or should it be closed/deprioritized?

[qingyun-wu]

Review fixes are in 3ad2a00, merged normally with main ff8f1134aef3ea78106687946cffafa727503daf.

@keweichen @yixuan-ag2: the primary and reclaim lease races now have a shared atomic boundary. notifier_lease.py serializes publication and cleanup under a persistent .guard file. A removal must match the exact captured device/inode/change-time/token/PID snapshot. Reclaim branching uses that captured token, not a later token read. A failed publisher releases only its own published token. Thus a live token arriving after the stale verdict invalidates removal, and a late primary publisher cannot erase its successor. The guard is never unlinked; the kernel releases the lock on process death. Symlink metadata/guard paths fail closed, and cleanup remains nonrecursive. Existing strict digest validation and socket/session keying are preserved.

All 36 lifecycle tests pass, including the two late-primary schedules, both empty-reclaim publication outcomes, replacement of a judged reclaim generation, guard crash release/inode persistence, and symlink refusal. All original 30 tests remain. Current-main standby, nudge/alert, target checks, and separate notifier process group are preserved. The standby fixture now waits for its prior lease to disappear before reusing the same target; all seven scenarios pass.

Also passed: watcher ownership (27 assertions), both migration suites, notifier health, Python discovery, nudge gate/alert tests, source-map checks (27), shell syntax, and diff hygiene. Canonical hosting documentation records the lease/backoff and terminal-status disruption. Current-head CI will be reported when complete.

Still unresolved: the required owner-run live managed-notifier restart and one-task/one-wakeup/one-result witness. This delivered worker task explicitly excludes live core/watcher operations, so no live restart or room task was sent. The isolated tests are not claimed as that witness. Reviewer approval remains pending; no merge will be performed.

(agent-assisted, Codex for qingyun-wu’s seat)

[qingyun-wu]

Follow-up 52a8b0a retains main’s existing migration allowlist entry. The Ubuntu failure is the unchanged BSD-first stat size probe, so correcting mtime alone does not justify removing that exemption. The first CI run also hit an unrelated sync-conflicts-report incremental timing assertion; its 12 tests pass locally. The scanner passes and full CI is rerunning at the new head. (agent-assisted, Codex for qingyun-wu’s seat)

[sonichi]

Reviewed at 52a8b0a; the code is the same as 3ad2a00 apart from the known-failures allowlist (the new head only restores that entry). Source only, by a delegated read, and I re-ran the one probe that matters for the finding below. The delegate ran tests/codex-notifier-supervisor-lifecycle.test.py in a temp fixture with a fake tmux (36 tests, OK) and did not run the standby-gate test, which needs a real tmux. I ran nothing else.

The lease design holds up in the tests I could trace: every snapshot, publish and remove goes through flock on a persistent .guard, and remove requires an exact device, inode, ctime, token and pid match, so a late publisher cannot delete a successor and a reclaimer cannot act on an earlier empty-directory verdict. A dead pid reads as stale, so a crash does not block forever. The standby handoff (return 2 for the session watcher) is separate from a notifier exit status 2, and the backoff default is correct (1, 2, 4 and so on, capped at 30 s, reset only after a 60 s healthy run). keweichen's and yixuan-ag2's earlier findings are fixed at this code.

Requesting changes for one finding I reproduced. The lease owner token is the output of ps -o lstart= (_start_of, task-notifier-supervisor.sh:208-214), and the judge compares it as a string with the contender's own ps output. lstart is localized: for the same shell pid on this Mac it prints Fri Nov 6 03:23:08 2026 under LC_ALL=C, 五 11月/ 6 03:23:08 2026 under zh_CN.UTF-8 and Fr. 6 Nov. 03:23:08 2026 under de_DE.UTF-8. A holder that wrote its token in one locale is therefore judged stale by a contender in another, which reclaims the live holder's lease and runs, so two supervisors for one target. Two launchers with different LANG or LC_* (a launchd or app start with none, against a terminal start) are enough. The delegate drove it with _judge from two locales and got live and stale for the same live pid. Every test runs both sides in one environment, so none catches it. Run ps under LC_ALL=C in _start_of, or use a locale-free identity, and add a two-locale case.

Lower, not blocking: the lease path is ${TMPDIR:-/tmp}/sutando-notifier-<key>.lock, so supervisors with different TMPDIR (launchd's /tmp against a shell's /var/folders) take different leases for one socket and session. Exit status 2 means "configuration fault" only by convention, and a bare failing command or bash syntax error under set -e would also stop the supervisor permanently. sutando-migrate.sh still has stat -f %z ... || stat -c %s at several lines, so whether its suite passes unlisted on ubuntu depends on CI. The real post-restart witness is still owed.

I will re-review at the new head.

[qingyun-wu]

Follow-up 50caaa9 supplies notifier_lease.py and portable_mtime.sh to copied launcher fixtures, keeps their leases inside the fixture temp directory, and supplies the migration fixture dependencies. The three supervisor launcher cases that failed on missing files now pass (3/3); migration hook coverage passes, and the scanner passes. The full launcher suite and current-head CI are running. (agent-assisted, Codex for qingyun-wu’s seat)

[github-actions]

@cla-assistant check

[qingyun-wu]

Final verification at 50caaa9: CI completed with 46 successful checks (including CLA), 1 skipped, and no failures. The branch is mergeable. Local lifecycle 36/36, focused supervisor launcher 3/3, standby 7 scenarios, and related migration/ownership/health/discovery checks pass. Full local launcher run was 102/103: the unchanged unassigned-delivery timing test exceeded 4 seconds, also reproduced on unmodified main at 8.77 seconds; its CI suite passes. The required real post-restart one-task/one-wakeup/one-result witness and fresh approval remain pending. No live core/watcher restart or PR merge performed. (agent-assisted, Codex for qingyun-wu’s seat)

[liususan091219]

Requesting changes at 50caaa92d3b258f75c7d774fd02d4f3ca76721fa.

sonichi's change request from 52a8b0a2 still stands at this head. The only commit since then is 50caaa92, and it changes tests and fixtures only (tests/codex-core-launcher.test.py, tests/sutando-migrate-test-hook-covers-config-dir.test.sh).

[P1] The lease token depends on the locale, so two supervisors can run for one target. _start_of (src/agent/codex/cli/task-notifier-supervisor.sh:208-213) builds the owner token from ps -o lstart= with no LC_ALL=C. _judge_dir then compares that token as a string with the contender's own ps output (:267-268). Reproduced on macOS with one live pid read under three locales:

C           Fri_Nov_6_13:43:31_2026_
zh_CN.UTF-8 五_11月/_6_13:43:31_2026_
de_DE.UTF-8 Fr._6_Nov._13:43:31_2026_

Say one supervisor starts from launchd or the app, with no locale set, and another starts from a terminal running zh_CN. The second judges the live holder stale, reclaims the lease and launches a second notifier. That is the duplicate-supervisor case this PR exists to prevent. Every test runs both sides in the same environment, so none of them can see this.

To unblock: run ps under LC_ALL=C in _start_of, or use a start identity that doesn't depend on the locale. Then add a test that publishes the token in one locale and judges it in another, through the production script.

Lower, not blocking: the lease path is ${TMPDIR:-/tmp}/... (:150). Supervisors with different TMPDIR values take different leases for the same socket and session.

Also still open: keweichen's change request from f507afcd hasn't been re-reviewed by him. The owner-run post-restart witness is still owed; that is named here, not blocking.

Ran: tests/codex-notifier-supervisor-lifecycle.test.py at this head, 36/36 OK, plus the locale probe above. CI: 46 pass, 1 skipped. license/cla is success. No unresolved threads. The hardcoded-path scan found nothing in live code.
Not verified: the standby-gate test (needs real tmux), and the full two-supervisor locale race end to end. The finding rests on the token mismatch plus reading _judge_dir.

— Sutando (susans-MacBook-Pro)