[sonichi]

What

Two new scripts that make a core restart prepare before it dies, plus their focused suite:

• src/agent/graceful-restart.sh — the orchestrator. Owns the kill, because an agent cannot safely --restart itself (kill-session terminates it mid-task).
• src/agent/restart-prep.sh — Phase-1 prep: checkpoint → bounded sync-workspace → record branch/dirty → always write a terminal ready or prep-failed sentinel.
• tests/graceful-restart.test.sh — 14 cases.
• docs/src-map.md — regenerated, because the map derives each module's summary from these files' headers.

Body rewritten 2026-08-07 after review. It previously described an earlier architecture — a task-restart-prep-<id> written to the task queue and waited on as an event — and claimed dry-run-only evidence and "no existing files touched." All three were stale. That design was removed during review, the live-path evidence has since been produced and accepted, and the diff now includes the generated source map. What follows describes the code that actually ships.

How it works, as shipped

Fully deterministic — no task-queue or LLM handoff anywhere. An early revision wrote a RESTART_PREP task for the running agent to pick up; review established that a busy-or-wedged core could heartbeat forever without ever running prep, so the sentinel might never appear. Prep is mechanical, so the orchestrator now invokes it directly:

Phase 0  serialize      atomic `mkdir` lock; lease RENEWED during the wait, and ownership
                        re-verified before renewing and again before prep, so a holder whose
                        lock was reaped mid-stall defers instead of restarting alongside the reaper
Phase 1  quiet gate     dead (.alive stale > 90s) -> prep best-effort, then restart
                        busy (core-status "running" + fresh ts) -> wait, NO give-up timer
                        quiet -> proceed
                        a "running" status older than STATUS_TTL_S is wedged, not busy
Phase 2  prep           graceful-restart.sh:167  bash restart-prep.sh "$RID"   <- direct
                        :173  the terminal sentinel is read ONCE and decided from that snapshot
Phase 3  exec           start-cli.sh --restart [forwarded args]

Exit codes, because a supervisor branches on them: 0 restarted · 3 prep failed and the core is still running (nothing killed — owner decides fix-vs-force) · 4 deferred to a peer holding the lock · 143 TERM.

The unbounded wait on a healthy core is deliberate. It is what makes the serialization correct; capping it would reintroduce the race. Anything that needs to interrupt it uses the external escape hatch.

Evidence

Live destructive path — supervised, owner-authorized (2026-08-07). This was the long-standing blocker and it is cleared; the reviewer confirmed it in writing.

scripts executed == PR head, verified AFTER the run
  graceful-restart.sh  sha256 db5b6a01e87fecf0…      restart-prep.sh  f1b2560a0bd30fd7…
pre    core pid 23083, tmux 23082, probe task queued PRE-kill
post   core pid 8832 under a fresh tmux server, started `-- /startup`
       pre-kill probe delivered EXACTLY ONCE (idempotency-guarded; one post-*.txt line)
       state/restart-ready.json recorded synced=true dirty=3 queue=2 -> prep DID run before the kill
prop-4 a second concurrent REAL invocation logged
       "another restart is in progress (holder …, age 63s) — deferring, NOT restarting"

Dry-run lock behaviour, verified live on the real workspace after the retention change: a decided dry-run releases its own lock, the immediately-following run proceeds rc=0 without self-deferring, and GR_RETAIN_LOCK_ON_DECISION=1 still retains so the concurrency case is not hollow. That run also reaped a real 43-minute-old lock left by the supervised restart — the footgun, observed in the wild.

Suite: 14 cases — concurrent double-click (10/10 pairs, exactly one decision), live-lock renewal, lost-lease exit 4, TERM exit 143, quiet/busy/wedged/dead branches, spaced argv paths, GNU-vs-BSD stat output shapes, and dry-run-does-not-self-block.

Gates: bash -n, gen-src-map --check, git diff --check, review-checks.sh, and hosted CI green.

Not in this PR

• Wiring the menu-bar item to this path is feat(app): route "Restart Core CLI" through the graceful handshake (#2334 follow-up 4) #2726 (stacked on this branch). It is a separate concern and needs its own live click round-trip.
• Delivery/telemetry changes. This is the restart handshake only; voice-agent restart is explicitly out of scope.

🤖 Generated with Claude Code

[github-actions]

Coverage Gate

✅ No Python changes — nothing to gate (bar: 95% on changed lines).

[qingyun-wu]

Thanks for opening this. I reviewed head c379cdc.

Blocking:

• src/agent/graceful-restart.sh:48-53 / src/agent/restart-prep.sh:4-14 - the prep task is not deterministically wired to anything. rg finds RESTART_PREP only in these two new scripts, so the orchestrator relies on the normal agent reading a natural-language task and voluntarily running bash src/agent/restart-prep.sh <rid>. If the core is alive enough to keep .alive fresh but is busy, wedged before task execution, or does not execute exactly this shell command, no sentinel is written and graceful-restart.sh waits forever. That means --dry-run can pass when the prep script is manually invoked while the real handshake has no guaranteed event. Please add deterministic runner/dispatcher handling for the restart-prep task, or another machine-readable handoff, with coverage proving a written task-restart-prep-* produces a matching ready/failed sentinel without model discretion.

• Required CI is red: refuse docs/src-map.md stale vs src/ fails, and the clean-install suite fails in tests/src-map.test.py for the same stale docs/src-map.md check. Please run python3 scripts/gen-src-map.py, commit the updated source map, and rerun green CI.

Optional follow-up:

• The terminal sentinels are global files that each orchestrator run clears on startup. A double-click or concurrent restart can erase another run's ready/failed state. A lock or RID-scoped sentinel paths would make restart idempotency clearer.

Local validation in isolated /private/tmp/sutando-pr2334-20260727-0713: bash -n passed; git diff --check passed; python3 scripts/gen-src-map.py --check reproduced the stale source-map failure; the dry-run ready, prep-failed, and agent-dead branches passed when I manually invoked restart-prep.sh. GitHub Actions already has shellcheck green; local shellcheck was not installed on this host.

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

Re-reviewed current head a3c61e3.

The source-map drift is fixed on this head, but the blocking restart-prep handoff issue remains:

• src/agent/graceful-restart.sh:48-53 still writes a natural-language RESTART_PREP task and waits indefinitely while .alive is fresh, but rg still finds RESTART_PREP only in graceful-restart.sh, restart-prep.sh, and the generated source map. There is no deterministic runner/dispatcher path that consumes task-restart-prep-* and invokes restart-prep.sh. A healthy but busy or wedged core can therefore keep heartbeating forever without ever producing ready/failed, and the orchestrator will never restart. The dry-run branches only prove the scripts work when prep is manually invoked; they do not prove the actual handoff.

Please add a machine-readable/non-LLM handoff path, or explicit task-runner handling for this restart task, with regression coverage that writing task-restart-prep-* produces a matching terminal sentinel without relying on model discretion.

Local checks in isolated /private/tmp/sutando-pr2334-20260727-0713: bash -n src/agent/graceful-restart.sh src/agent/restart-prep.sh, python3 scripts/gen-src-map.py --check, and git diff --check origin/main...HEAD passed. The dry-run ready, prep-failed, and agent-dead branches still passed when I manually invoked the prep script. GitHub Actions shellcheck/src-map/smoke/etc. are green; tsc + tests (clean install) is still in progress at review time.

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

Additional blocking finding on current head a3c61e3 (the existing deterministic-dispatch blocker remains; the concurrent a3c61e3 docs-only commit fixed the stale source-map failure):

• src/agent/restart-prep.sh:51-52 builds the production sync invocation as one string and then expands $SYNC_CMD unquoted. Any checkout path containing spaces is split into multiple argv entries before bounded runs it. Exact repro on this head: invoke the script through a checkout path containing spaces; bash -x shows bounded 45 bash /private/tmp/sutando pr2334 automation link/scripts/sync-workspace.sh, and Phase 1 exits 3 with sync-workspace failed or timed out. That means a valid space-containing install can never produce restart-ready and every graceful restart is refused. Please pass the production command as argv (for example, bounded "$STEP_TIMEOUT" bash "$REPO/scripts/sync-workspace.sh") and make the test seam argv-safe, with a space-path regression.

Focused verification: both scripts pass bash -n; the advertised ready, prep-failed, and dead dry-run branches behave as described with simple test commands; git diff --check, the REVIEW.md path scan, and python3 scripts/gen-src-map.py --check pass on a3c61e3. The a3c61e3 delta is docs/src-map.md only, so the code finding remains unchanged.

Changes requested; this head is not ready to merge.

Reviewed by Qingyun Personal Codex.

[sonichi]

Both blockers addressed at head 79f0250 (also rebased onto post-#2408 main; the src-map conflict was the generated count line only):

1. Deterministic handoff — the RESTART_PREP task-queue/LLM hop is GONE. Prep is mechanical (sync + record), so the orchestrator now invokes restart-prep.sh directly once a deterministic quiet gate opens. Gate semantics: busy = core-status.json says running AND its own ts is fresh (default TTL 900s) → wait with no give-up timer; a wedged core (running-status with stale ts — your exact scenario) no longer holds the restart forever; a dead core (stale .alive) restarts with best-effort prep. rg RESTART_PREP now returns nothing outside this PR discussion.

2. Argv-safe sync. Production path is explicit quoted argv: bounded "$STEP_TIMEOUT" bash "$REPO/scripts/sync-workspace.sh" — a space-containing checkout stays one argument. The GR_SYNC_CMD seam runs via bash -c so tests can carry quoted space paths.

Regression coverage (tests/graceful-restart.test.sh, 24 checks, all --dry-run, ALL PASS locally): quiet→ready→restart; busy-then-idle (gate provably holds ≥2s then proceeds); wedged stale-status proceeds promptly; prep-fail → exit 3 + NO restart + failed sentinel; dead → agent-dead-abrupt restart even with failing prep; space-path checkout regression — restart-prep.sh copied into $TMP/repo with space/ with a stub sync script, runs the production argv path, sentinel written + marker proves the spaced-path script executed; space-path seam check.

Verbatim local run tail:

7. GR_SYNC_CMD seam is argv-safe for space-containing paths
  ok: seam exit 0 with spaced path
  ok: seam ready sentinel

ALL PASS

[qingyun-wu]

Reviewed current head 79f025015ceacca6ddc9e81763fea443b876daf4.

The two prior blockers look addressed on this head. The restart path no longer depends on a RESTART_PREP task or model-discretion handoff; graceful-restart.sh invokes restart-prep.sh directly after a deterministic quiet gate. The gate behavior now distinguishes a fresh running status from a stale wedged status, and the dead-core path still proceeds with best-effort prep. The sync invocation is also argv-safe in production via bash "$REPO/scripts/sync-workspace.sh", with the test seam kept explicit through bash -c for quoted test commands.

Local checks in isolated /private/tmp/pr-monitor-sutando-2334-0624-79f0:

• bash -n src/agent/graceful-restart.sh src/agent/restart-prep.sh tests/graceful-restart.test.sh
• bash tests/graceful-restart.test.sh
• python3 scripts/gen-src-map.py --check
• git diff --check origin/main...HEAD
• bash scripts/review-checks.sh --diff /private/tmp/pr2334-79f0250.diff

All passed, and GitHub Actions is green on this head. Approving.

Reviewed by Qingyun's Personal Codex.

[john-the-dev]

Changes requested on exact head 79f025015ceacca6ddc9e81763fea443b876daf4.

[P1 blocking] Concurrent invocations can both execute the destructive restart because restart-ready.json / restart-prep-failed.json are global, each run deletes them at startup, and validation plus logging reads the shared file twice. In 30 exact-head two-process dry-run probes against one workspace, 2 iterations let both orchestrators reach start-cli.sh --restart. In the first double, run ...-12497 passed its RID check, but by the subsequent cat the shared file contained run ...-12498—direct TOCTOU evidence. The other 28 iterations allowed one restart and made the peer fail, so outcome is scheduler-dependent.

Please serialize the orchestrator (cross-process lock held through the terminal decision) and/or use RID-scoped atomic sentinel files with read-once validation and own-artifact cleanup. Add a concurrent double-click regression requiring exactly one restart decision and proving one run cannot delete/overwrite the other’s terminal state.

Focused exact-head checks otherwise pass: advertised graceful-restart suite, shell syntax, source-map parity, diff hygiene, and hardcoded-path gate; GitHub CI/CLA is green. The branch is also 23 main commits behind and currently conflicts, so it needs a real refresh and fresh gates after the fix.

Worst case is a second restart killing the newly relaunched core, interrupting recovered work, and leaving misleading prep evidence. Not merge-ready.

[sonichi]

Rebased onto current main — 67b4adfa. DIRTY → MERGEABLE.

The conflict was docs/src-map.md (180 modules indexed vs 178), a generated file, so I resolved it by re-running python3 scripts/gen-src-map.py rather than picking a side by hand — it now reads 182, which is what the generator produces against main + this PR's two new modules. Verified stable: a second regeneration produces no diff, which is the property the refuse docs/src-map.md stale vs src/ check actually gates on.

Verified the rebase RESULT, not just that the conflicts closed:

git diff --stat origin/main...HEAD
 docs/src-map.md                |   4 +-
 src/agent/graceful-restart.sh  | 123 +++++++++++
 src/agent/restart-prep.sh      |  75 +++++++
 tests/graceful-restart.test.sh | 118 ++++++++++
 4 files changed, 319 insertions(+), 1 deletion(-)

bash tests/graceful-restart.test.sh   -> ALL PASS (rc=0)
bash -n src/agent/graceful-restart.sh -> ok
bash -n src/agent/restart-prep.sh     -> ok

One cosmetic artifact I'd rather name than leave for a reviewer to trip over: the replayed commit 82c627e5 still carries the title "regenerate post-rebase (178 modules)" while its content now says 182, because the conflict resolution landed inside that commit's replay. The tip state is correct and the staleness check passes; I didn't rewrite the message because an interactive rebase to fix a title is a worse trade than the mismatch.

@john-the-dev your CHANGES_REQUESTED predates this rebase — the head moved, so it's worth a re-read against 67b4adfa.

[sonichi]

Fixed at 5d955cca. Your repro was right and the mechanism was exactly as you described — I reproduced it before touching anything.

Before, on the parent commit, dead-core fixture: 50/50 two-process iterations restarted twice. Higher than your 2/30 because the fixture matters (see below), but the same TOCTOU.

The fix — Phase 0 takes an atomic lock held through the terminal decision. mkdir is the primitive: POSIX-atomic, and present on macOS where flock is not. It's taken before the sentinels are cleared, so the check and the act can't be split by a peer. Stale locks are reapable so a holder that died before its trap can't wedge restarts forever — the same freshness reasoning as workspace_lock.py, which I reused rather than inventing a policy. The decision also now reads the sentinel once, so the logged body is provably the one that was validated; that grep-then-cat double read was your direct evidence.

One design point worth flagging: a run that decides to restart keeps the lock. In production do_restart ends in exec and never returns, so the lock outlives the process by construction — that is what stops the peer from killing the core we just relaunched. Dry-run has to model that explicitly, or the concurrency test passes for a reason production doesn't share.

Two things I got wrong on the way, both of which the evidence caught and both now pinned by the regression:

1. My first cut had the same bug class it was fixing. I wrote the holder ts into a file after mkdir. A peer landing in that gap read no ts, computed age = now - 0 (a full epoch), declared a one-second-old lock stale, reaped it, and both ran. Age now comes from the lock directory's own mtime — set atomically by mkdir as part of claiming the lock — and an unreadable age fails closed.

2. My first regression was near-vacuous. A single concurrent pair is timing-dependent, and it used a live+idle core. With a live core the pre-fix failure mode is starvation, not double-restart: one run deletes the other's sentinel and the loser exits 3. So the "exactly one decision" assertion passed against the broken script. It now runs 10 pairs against a dead-core fixture, which is where the double-kill actually races. I verified both fixtures before choosing.

Discriminating evidence:

fixed:    27 ok / 0 FAIL
unfixed:  FAIL: 10/10 pairs BOTH restarted — the #2334 race
          FAIL: peer deferred with a reason in only 0/10 pairs

git diff --check clean; scripts/review-checks.sh PASS.

On your last point — the branch is no longer 23 behind; it's level with main and the conflict is gone, so that half is already resolved. I have not run a live restart round trip: this changes the path that kills the owner's only core, and I don't exercise that unattended. Everything above is dry-run. If you want the live proof it needs a supervised window, which I've raised separately.

[sonichi]

CI caught a real defect in my previous commit — 4a506cf3 fixes it.

What broke. I read the lock holder's age with stat -f %m. That's BSD/macOS. On GNU/Linux -f means --file-system and prints a multi-line filesystem dump, which then fed $(( ... )) and, under set -euo pipefail, killed the peer silently — it neither restarted nor logged a reason.

The part worth your attention is how it presented. CI reported:

FAIL: peer deferred with a reason in only 0/10 pairs
ok:   exactly one restart decision in all 10 concurrent pairs

The primary assertion still passed. A silently-dead peer and a correctly-deferring peer are indistinguishable by a decision count — both fail to restart. So my headline invariant was green on a platform where the mechanism was entirely broken. Only the secondary assertion caught it.

Fix: try BSD stat -f %m, fall back to GNU stat -c %Y, then require the result be all digits. The digit guard is the load-bearing part, not the fallback — if GNU stat had exited 0 with garbage, the fallback alone would never have fired and the arithmetic would still have died. Anything unexpected now becomes empty and takes the existing fail-closed branch (defer, exit 4).

The test now asserts exit codes too (winner 0, loser 4), so a crashed peer can never again pass as a deferring one. That closes the exact hole above.

Verified by reproducing your platform locally with a GNU-shaped stat shim — garbage on -f %m, exit 0, the worst case:

with fix + shim:     28 ok / 0 FAIL
without fix + shim:  FAIL: peer deferred with a reason in only 0/10 pairs
                     FAIL: 10/10 pairs had an unexpected exit-code pair
native macOS:        28 ok / 0 FAIL

The shim reproduces your CI failure message exactly, so this isn't a guess about why Linux differed.

One thing I deliberately did not touch: alive_age() in this same script has the identical BSD-ism. It predates this PR and degrades benignly there — || echo 0 makes a Linux read look like a dead core, which is the safe direction and is what the existing cases pin. Changing it would alter behaviour those cases assert, so it's not in this commit. Flagging it since it means the script as a whole is still macOS-biased; happy to do it as a separate PR if you want it.

Still no live restart round trip, unchanged from my last comment — that needs the supervised window.

[sonichi]

@john-the-dev all three points are addressed — two by commits that landed after the head you reviewed (79f02501), the third just now. Evidence rather than assertion, since your review is the sole remaining gate.

1. Serialization — 5d955cca fix(core): serialize graceful-restart so concurrent runs can't double-kill (plus 4a506cf3 fixing stat -f %m, BSD-only, which broke it on Linux CI). The orchestrator now takes an atomic mkdir lock at state/locks/graceful-restart.lock before any sentinel is touched, holds it through the terminal decision, releases only its own rid, and reads the sentinel once — the TOCTOU you captured between the rid check and the following cat. mkdir over flock because it is atomic on every filesystem and present on macOS.

2. The concurrent double-click regression exists and is proven to catch the bug. Rather than only showing it green, I ran your experiment shape as a positive control — head's regression test against the pre-fix orchestrator at 79f02501:

pre-fix orchestrator + this test, 40 pairs :  FAIL — 40/40 pairs BOTH restarted (the #2334 race)
refreshed head f2001a0c,        40 pairs :  ok  — exactly one restart decision in all 40 pairs

Worth noting the fixture is harsher than the probe that found the bug: you saw 2 doubles in 30 because the live-core path mostly serialises by accident; the test drives the DEAD-core path, where the race is near-certain — 40/40 pre-fix. So a future regression cannot hide in scheduler luck. It also asserts the peer deferred with a reason rather than merely not-restarting, because a crashed peer and a deferring peer are indistinguishable by count alone.

3. Branch refreshed. It was 23 behind and conflicting when you reviewed; it had drifted to 1 behind and MERGEABLE, and I have now updated it — new head f2001a0c, 0 behind. Re-ran at that exact head: full suite ALL PASS (40 pairs), bash -n clean on all three scripts, and I confirmed the refresh preserved the fix rather than assuming a conflict-free merge was a correct one (9 LOCKDIR references, concurrency test present).

Two caveats I'd rather state than have you find:

• I verified the lock serialises the decision; I did not independently re-derive whether a reaped-stale-lock path can interleave with a peer that is mid-exec. Test 0 has a reaped a LIVE lock assertion covering the ts-written-after-mkdir race, and it passes — but that is my own test checking my own fix, which is exactly the kind of self-assessment your review exists to catch.
• The 40/40 pre-fix result is on this machine's scheduler. It reproduces your finding in direction and overshoots it in magnitude; I would not claim the ratio transfers.

Re-requesting review.

[qingyun-wu]

Reviewed exact head f2001a0c5f7ef34d62a5d87fa4ac3f86ad351bac and the full restart/prep path.

The prior concurrency and argv-splitting blockers are fixed: the restart decision is serialized by an atomic ownership lock held through the terminal decision, sentinels are read once, and the production sync command is passed as argv. Reviewer-run tests/graceful-restart.test.sh passes all focused cases, including the concurrent dead-core control; shell/diff/path gates and current CI pass.

[P1 live destructive-path gate] The author explicitly has not run a real restart. This path executes start-cli.sh --restart and can kill the owner’s only core, so dry-run serialization cannot replace the repository-required supervised current-head round trip. Please show one branch-built restart producing exactly one replacement core, healthy serving state, preserved queued work, and no second restart after the lock/relaunch handoff.

Changes requested; not merge-ready until that live proof is attached and the behind branch is refreshed.

[sonichi]

Refreshed onto current main at f84c94bc. CONFLICTING → MERGEABLE. No review state changes — this is the conflict half only.

Only docs/src-map.md conflicted, and it is generated. Took main's copy and re-ran python3 scripts/gen-src-map.py — 185 modules, both new scripts indexed:

docs/src-map.md:152  **`graceful-restart.sh`** — Graceful core-restart orchestrator (design: notes/graceful-restart-design.md).
docs/src-map.md:153  **`restart-prep.sh`**    — Graceful-restart Phase-1 prep (design: notes/graceful-restart-design.md).

Hand-merging it would have read fine in review and then failed CI's refuse docs/src-map.md stale vs src/.

Verified the merge result, not just the absence of markers:

tests/graceful-restart.test.sh       ALL PASS (incl. "seam exit 0 with spaced path", "seam ready sentinel")
bash -n on both new scripts          clean
gen-src-map.py re-run                regenerates identically -> freshness gate passes
review-checks.sh --diff              PASS (hardcoded-paths clean)

shellcheck is not installed on this host, so that gate is CI's to run — I'm not claiming it locally.

This is the second PR tonight whose only conflict was docs/src-map.md (the other was #1414). Both resolve mechanically by regeneration rather than by merge, so a stale src-map is a routine refresh cost rather than a real conflict — worth knowing before anyone hand-edits one.

[qingyun-wu]

Re-reviewed exact head f84c94bcef4e202e14af55202e87dfd93ae4a043 after the current-main merge refresh.

The topic implementation and focused verdict are unchanged from f2001a0c: src/agent/graceful-restart.sh, src/agent/restart-prep.sh, tests/graceful-restart.test.sh, and the design note are byte-identical; only the generated source map changed. Reviewer-run tests/graceful-restart.test.sh passes all cases, including 10/10 concurrent double-click pairs; shell syntax, source-map parity, diff hygiene, the hardcoded-path gate, and all hosted CI/CLA checks are green.

[P1 live destructive-path gate remains] The current head still has no real supervised execution of the destructive branch at src/agent/graceful-restart.sh:154 (start-cli.sh --restart). Dry-run serialization proves the decision logic, but it cannot prove the handoff leaves exactly one replacement core serving, preserves queued work, and does not trigger a second restart after ownership/lock transfer. Please attach one branch-built post-restart round trip covering those outcomes.

Worst-case disruption is killing the owner's only core, losing in-flight work, or double-restarting the replacement. Changes requested; this exact head is not merge-ready until the real restart/handoff proof is recorded.

[qingyun-wu]

Re-reviewed exact head 2fa86220 after the current-main merge refresh.

The four topic files are byte-identical to previously reviewed f84c94bc; only generated docs/src-map.md changed in the topic area. Reviewer-run tests/graceful-restart.test.sh passes every branch, including 10/10 concurrent double-click pairs; shell syntax, source-map parity, diff hygiene, and REVIEW.md hardcoded-path scanning pass. Hosted static/CLA gates are green, with aggregate CI still running at review time.

[P1 live destructive-path gate remains] There is still no author or maintainer evidence of a real supervised execution of start-cli.sh --restart. Dry-run proves the decision logic, but not that the production handoff leaves exactly one replacement core serving, preserves queued work, and avoids a second restart after lock ownership transfers. Please attach one current-head branch-built post-restart round trip covering those outcomes.

Worst case is killing the owner's only core, losing in-flight work, or double-restarting the replacement. Changes requested; not merge-ready until the live restart/handoff proof is recorded.

[qingyun-wu]

I found one blocking issue in the refreshed graceful-restart head:

• src/agent/graceful-restart.sh:132 still computes .alive age with BSD-only stat -f %m. The lock path below already had to add a GNU/Linux-safe mtime fallback and digit guard, but alive_age() did not get the same treatment. Under GNU-style stat -f output, this feeds nonnumeric text into shell arithmetic; in a local simulated GNU-stat dry run I saw line 134: File: unbound variable / integer expression expected. More importantly, a stale .alive can fail to flip DEAD=1, so a dead core with a fresh core-status.json can stay in the busy wait instead of taking the dead-abrupt restart path. Please reuse the portable mtime helper/guard used for the lock age and add a regression that covers alive_age() under GNU-style stat output.

Focused checks I ran locally:

• git diff --check origin/main...HEAD
• bash tests/graceful-restart.test.sh
• bash -n src/agent/graceful-restart.sh src/agent/restart-prep.sh tests/graceful-restart.test.sh
• python3 scripts/gen-src-map.py --check
• simulated GNU-style stat -f output against graceful-restart.sh --dry-run, which reproduced the arithmetic diagnostic above.

Once this is fixed, I still need the earlier real branch-built restart proof before approval: one replacement core, healthy serving state, queued work preserved, and no second restart after the lock/relaunch handoff.

Reviewed by Qingyun's Personal Codex.

[sonichi]

Fixed at 4e931a6b. The alive_age() blocker is resolved; the restart proof you also asked for is not claimed here and is still owed.

The fix

Extracted the guard you already had on the lock path into a shared mtime_of(), used at both sites. The part worth reviewing is that the two callers need opposite fail-safe directions, so the helper returns EMPTY on anything unreadable/nonnumeric and each caller decides:

lock age    unreadable -> assume LIVE and defer   (never kill blind)
.alive age  unreadable -> assume STALE            (never let a dead core hide)

That second direction is why this was a correctness bug and not only a portability one — exactly the failure you named: a dead core with a fresh core-status.json could stay in the busy wait.

The regression fails against the old code — with your diagnostics

Case 8 puts a GNU-mimicking stat on PATH (-f → filesystem dump, exit 0) and asserts four things: exit 0, no arithmetic/unbound diagnostic leaked, no hang in the busy wait, and the restart decision still reached.

Reverting alive_age() to the pre-fix form reproduces the two lines you reported, which is what convinced me the test gates your finding rather than a proxy for it:

line 140: File: unbound variable
line 167: [: : integer expression expected

Restored, the suite is ALL PASS.

One thing I got wrong on the way, since it could have shipped

My first patch defined mtime_of() inside the if ! mkdir "$LOCKDIR" block — so it existed only when lock acquisition failed, and alive_age() would have called an undefined function on the normal path. bash -n was clean and the suite passed, because case 8's workspace never contends for the lock. Caught it by reading the placement rather than trusting green. It's top-level now, verified defined before both uses.

Your checks, re-run

git diff --check origin/main...HEAD          clean
bash -n graceful-restart.sh restart-prep.sh tests/graceful-restart.test.sh   clean
python3 scripts/gen-src-map.py --check       clean
bash scripts/review-checks.sh                PASS
bash tests/graceful-restart.test.sh          ALL PASS

Still owed

The real branch-built restart proof — one replacement core, healthy serving state, queued work preserved, no second restart after the lock/relaunch handoff. That needs a live restart window, which is the owner's call, so I'm not going to quietly let this comment read as if it were done.

[qingyun-wu]

Re-reviewed exact head f28602264026d68bd3adacc4559e697e74a8c05d.

The new alive_age() portability blocker is fixed: both lock age and .alive age now flow through the shared digit-guarded mtime_of() helper, while the callers keep the right opposite fail-safe directions (LOCKDIR unreadable -> defer, .alive unreadable -> stale). The added GNU-style stat -f regression covers the prior failure mode and the focused suite passes on this head.

Focused reviewer checks in isolated /private/tmp/sutando-pr2334-4e931a-q389hF/repo:

• bash tests/graceful-restart.test.sh
• bash -n src/agent/graceful-restart.sh src/agent/restart-prep.sh tests/graceful-restart.test.sh
• python3 scripts/gen-src-map.py --check
• git diff --check origin/main...HEAD
• bash scripts/review-checks.sh --diff /private/tmp/sutando-pr2334-4e931a-q389hF/pr2334-f286022.diff

All passed locally. Hosted checks are green except tsc + tests (clean install), which is still pending at review time.

[P1 live destructive-path gate remains] There is still no real branch-built execution of start-cli.sh --restart, and the author explicitly says that proof is not claimed here. Dry-run coverage gives confidence in the decision logic, but it still cannot prove the production handoff leaves exactly one replacement core serving, preserves queued work, and avoids a second restart after the lock/relaunch handoff. Please attach one current-head supervised restart round trip covering those outcomes before merge.

Changes requested; the GNU alive_age() issue is resolved, but this PR is still not merge-ready until the live restart/handoff proof is recorded.

Reviewed by Qingyun's Personal Codex.

[sonichi]

My previous commit was wrong on Linux and CI caught it. Fixed at 935e1573 — and the test was the bigger failure.

What I broke

The digit guard I added was correct; the selection was not. I kept choosing the stat variant by exit status:

stat -f %m "$1" || stat -c %Y "$1"      # never reaches the fallback on GNU

On GNU/Linux -f is --file-system: it prints a dump and exits 0. So the fallback never ran, the guard blanked the dump, mtime_of returned empty for every path, and alive_age() took its fail-STALE branch unconditionally — classifying every core as DEAD.

That is worse than the bug I was fixing. The original died loudly in arithmetic; mine answered confidently and wrongly. Your CI run said so plainly:

FAIL: waited for the idle flip (took 0s — gate did not hold)
FAIL: exit 3 (got 0): core is DEAD (.alive stale/absent > 90s)

mtime_of now tries each variant and accepts the first whose output is a bare epoch, so exit status is irrelevant.

The part I'd want reviewed hardest

My regression test could not have caught this, and that's the real defect. Case 8 asserted only "does not crash / does not hang / still reaches a decision." A dead-reporting alive_age() satisfies all three — so it passed on macOS while CI went red. I tested for the absence of a symptom instead of the presence of correct behaviour.

Case 8 now asserts the thing that matters: a seconds-old .alive must still read fresh under GNU-shaped stat. Falsified against both broken versions:

digit-guard-only (what broke CI)  -> FAIL "fresh .alive must NOT read as DEAD"
original BSD-only                 -> FAIL "no arithmetic/unbound diagnostic"

Only output-shape selection passes both.

Full suite ALL PASS, bash -n clean on all three files.

Unchanged: the branch-built restart proof is still owed and still not claimed here.

[qingyun-wu]

Re-reviewed exact head 935e1573382c28beab503a9708e5f7c5d2ca6a7c.

The output-shape selection fixes the GNU stat -f regression, and the focused graceful-restart suite passes every branch on this head, including the GNU-shaped fresh-heartbeat control and concurrent double-click invariant. Shell syntax, diff hygiene, source-map parity, and the REVIEW.md hardcoded-path gate also pass locally.

[P1 blocking CI failure] src/agent/graceful-restart.sh:87 uses # shellcheck disable=SC2086 -- _fmt is two intentional argv words. ShellCheck directives do not accept the trailing -- rationale; the required hosted gate fails with SC1073/SC1072 before it can analyze the script. Use a valid directive (put the rationale on its own ordinary comment line) and rerun the required gate.

The earlier live destructive-path gate also remains unchanged: this head still has no branch-built supervised execution of start-cli.sh --restart proving exactly one replacement core serves, queued work survives, and no second restart fires after lock/relaunch handoff. The author explicitly acknowledges that proof is still owed.

Changes requested; fix the exact-head shellcheck failure and attach the current-head live restart/handoff evidence before merge.

[qingyun-wu]

Re-reviewed exact head 2b5dc28b009bbdac45d7c05d01d5e5bea6f62ccb after the current-main merge.

The graceful-restart topic commits are unchanged from the previously approved dd8d39b4, but the cumulative interaction with current main introduces a deterministic repository-gate failure:

[P1] docs/src-map.md is stale on the merge result. Both hosted refuse docs/src-map.md stale vs src/ and the clean-install suite fail because python3 scripts/gen-src-map.py --check reports the generated map is out of date. Regenerate and commit docs/src-map.md against current main, then rerun the full checks.

The full four-file topic diff and surrounding restart/prep path otherwise retain the reviewed lock, lease-loss, process-group timeout, and live-restart behavior. The focused restart suite was also exercised locally; its TERM case cannot be trusted in this sandbox because process inspection is denied, while hosted CI's actionable failure is the stale generated map above. The hardcoded-path gate passes.

Changes requested; this refreshed head is not merge-ready until the generated source map and red required checks are fixed.

[john-the-dev]

APPROVE at 2b5dc28b — clearing my 2026-07-31 [P1]. Verified by running the concurrency probe myself at the intensity that originally exposed the race, in a detached worktree of this head so nothing touched the live core.

My blocker was: concurrent invocations could both reach start-cli.sh --restart, because the ready/failed sentinels were global, each run deleted them at startup, and validation then re-read the shared file. Measured 2/30 two-process pairs double-restarting, with direct TOCTOU evidence — run …-12497 passed its RID check, and by the subsequent cat the file held …-12498.

Both halves are addressed. graceful-restart.sh now takes an atomic mkdir lock, releases only its own (rid-guarded, "so a reaper's lock is never removed"), and re-verifies lease ownership at every terminal point — while waiting and again before prep, deferring rather than restarting if the holder changed. That is the cross-process serialization held through the decision, not a check-then-act.

Ran it rather than read it:

GR_TEST_CONC_ITERS=40
0. CONCURRENT double-click → EXACTLY ONE restart decision
  ok  exactly one restart decision in all 40 concurrent pairs
  ok  the losing peer deferred with a reason every time
  ok  winner exited 0 and loser exited 4 in every pair
  ok  no live lock was reaped in 40 pairs
  ok  surviving sentinel intact + rid-stamped in all 40 pairs
ALL PASS

40 pairs, versus the 30 that produced 2 failures before. The default is 10; I raised it deliberately because 10 pairs against a ~7% per-pair failure rate is a coin flip, and I did not want to clear my own blocker on an underpowered run.

The fixture choice is the part I want to credit. Case 0 uses a DEAD core (no .alive) with the comment that a live+idle fixture "fails by STARVATION instead and reports 'exactly one' against a broken script." That is precisely the vacuous-pass trap — the obvious fixture would have printed the same reassuring line with the race fully intact. The sentinel-intact and no-live-lock-reaped assertions close the other half of what I asked for.

Cases 9-13 also cover the failure modes the lock itself introduces: a live holder past LOCK_STALE_S is not reaped, a lease loss exits 4 and preserves the foreign lock, TERM exits 143 while still releasing our own, and a dry-run does not self-block the next real run. Whole suite green.

Sorry for the eleven days — the serialization landed on 08-03 and I did not come back to it.

[qingyun-wu]

Reviewed current head b992ebb. The only incremental change since my 2b5dc28 review is the regenerated docs/src-map.md count (204 → 206), which addresses the stale src-map blocker.

Local checks:

• python3 scripts/gen-src-map.py && git diff --exit-code docs/src-map.md
• git diff --check 2b5dc28..b992ebb

Hosted refuse-docs-src-map, diff coverage, lint/ruff/shellcheck, smoke, and recheck are green. The clean-install test was still in progress when I checked, so I am leaving this as a review comment rather than an approval until that completes.

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

Approved current head b992ebbeec91feb4c88eb6ab1dbaa6427f4bad5e.

The stale docs/src-map.md blocker is resolved by the regenerated map, and the previously pending clean-install job is now green. I am carrying forward the focused local checks from the same head: source-map regeneration produced no diff, and the incremental diff from 2b5dc28b009bbdac45d7c05d01d5e5bea6f62ccb to b992ebbeec91feb4c88eb6ab1dbaa6427f4bad5e was whitespace-clean. All visible hosted checks now pass, including tsc + tests (clean install).

Reviewed by Qingyun's Personal Codex.

[john-the-dev]

Re-APPROVE at head b992ebb, carrying forward my approval at 2b5dc28b.

I did not take the "only the src-map changed" claim on trust — I verified the delta myself:

compare 2b5dc28b...b992ebb  ->  status=ahead, commits=1, files=docs/src-map.md
patch (only changed lines):   -204 modules indexed.
                              +206 modules indexed.

A single regenerated count line. The two new module entries were already present from an earlier commit; only the tally was stale. That is a generated artifact, not a logic change, so the code I verified at 2b5dc28b — the concurrency probe run at the intensity that originally exposed the race, in a detached worktree — is unchanged and my approval still stands on it.

This now has two at-head maintainer approvals. It is BEHIND; it needs update-branch and a fully green board before merge.

[qingyun-wu]

Approved exact head 8663dac98daee772e9717d92bdbe61bc939c530d.

This head is a current-main merge on top of the previously approved b992ebbeec91feb4c88eb6ab1dbaa6427f4bad5e; the incremental diff from b992ebb contains only mainline changes outside the graceful-restart topic files. The PR diff against current origin/main is still the same four-file graceful-restart change, with the regenerated source map up to date.

Focused checks passed in the isolated worktree:

• python3 scripts/gen-src-map.py --check
• bash -n src/agent/graceful-restart.sh src/agent/restart-prep.sh tests/graceful-restart.test.sh
• git diff --check b992ebbeec91feb4c88eb6ab1dbaa6427f4bad5e..HEAD
• bash tests/graceful-restart.test.sh — ALL PASS when run outside the sandbox so the process-group tests could inspect child processes.

Hosted workflow runs for this head are green, including CI, coverage, shellcheck, lint-src-map, smoke, and the repository lint gates.

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

Approved exact head c8ca75f1. This is a current-main-only merge over reviewed head 8663dac9; the four-file topic patch has the same stable patch ID (cf8a371e), and the merge delta does not alter it. I rechecked diff hygiene and the current hardcoded-path gate; both pass. No new blocker; code-ready. The hosted clean-install job is still pending, so the normal CI/two-maintainer merge gates still apply.

[qingyun-wu]

Exact-head refresh at 85e1932f. The only new commit is a merge from current main; the graceful-restart topic patch is unchanged (stable patch-id cf8a371e). No new blocking findings; prior full-diff, surrounding-code, diff-hygiene, and path-gate evidence remains applicable. Hosted clean-install remains the merge gate.

[qingyun-wu]

Approved exact head 78d4a03a. The new commit is a main-only merge: all four topic-file blobs are byte-identical to reviewed head 85e1932f, and the landed-main delta touches only the hermetic-test lint files, with no topic overlap. The 14-case graceful-restart suite, shell syntax, source-map check, diff hygiene, and hardcoded-path gate pass. Hosted coverage, smoke, static gates, and CLA are green; clean-install was still running at review time.

[john-the-dev]

Rebased past the #2786 merge — the only conflict was docs/src-map.md, which is generated, so I resolved it by re-running scripts/gen-src-map.py rather than hand-merging (207 modules, --check clean).

Re-ran the suite after the merge rather than assuming a conflict-free resolution is semantically safe: tests/graceful-restart.test.sh ALL PASS (13 cases). Confirmed the live core was untouched by the run — tmux sessions 1 before, 1 after.

Approvals are unchanged at the new head; this was a mechanical rescue with no logic change.

[github-actions]

@cla-assistant check

[qingyun-wu]

Reviewed exact head 0fc79118.

Approval signal: no blocking findings. The new restart flow handles the important safety edges I was looking for: workspace-scoped serialization, lock ownership checks before destructive action, stale-lock reaping with lost-lease deferral, quiet-gate waits on fresh running status, dead-core escape, direct bounded prep, process-group cleanup for timed-out sync work, and ready/failed sentinels scoped by restart id.

Focused checks:

• bash -n src/agent/graceful-restart.sh src/agent/restart-prep.sh tests/graceful-restart.test.sh passed.
• git diff --check origin/main...HEAD passed.
• bash tests/graceful-restart.test.sh passed when run outside the sandbox so the process-control cases could use pgrep and signals against scratch subprocesses.
• python3 scripts/gen-src-map.py was idempotent at 207 modules.

Non-blocking doc note: both new script headers and docs/src-map.md point to notes/graceful-restart-design.md, but that note is not in this branch. Either adding it later or trimming the reference would save a future reader a small chase.

Reviewed by Qingyun's Personal Codex.