Skip to content

fix(core-heartbeat): a versioned Claude binary makes pgrep -x miss, so a LIVE core reads dead - #2580

Merged
john-the-dev merged 4 commits into
mainfrom
fix/core-heartbeat-pgrep-versioned-binary
Aug 3, 2026
Merged

john-the-dev merged 4 commits into
mainfrom
fix/core-heartbeat-pgrep-versioned-binary

Conversation

@sonichi

@sonichi sonichi commented Aug 3, 2026

Copy link
Copy Markdown
Owner

The defect

#2488 correctly stopped a dead core from reading healthy, by gating the beat on core_pid() resolving the core. On this host that gate never passes for a healthy core, so .alive is never written at all and the inverse bug is live: a core up for ten minutes, draining tasks normally, reads DEAD to every reader (health-check, dashboard, peers, the scheduler).

core_pid() enumerates candidates with pgrep -x claude. pgrep -x matches the kernel accounting name, and Claude Code runs from a version-named binary:

exe   : ~/.local/share/claude/versions/<ver>
ucomm : <ver>        <- what pgrep -x matches
comm  : claude       <- argv[0], what ps -o comm= shows

pgrep -x claude therefore matches nothing. The runtime == "claude" bail then returns None without the pane fallback — deliberately, because for a Claude session "no matching process" is supposed to mean dead. Here it means "the binary is not named what we grepped for."

_argv_names_session() itself is fine — given the core's argv it returns True. Only the pid enumeration is broken.

Evidence

Before (at origin/main, core pid alive and processing tasks):

$ core_pid() at origin/main -> None
  (core pid 30961 is alive: True )

The predicate #2160 consumes, with a positive control so the negative means something — same function, given a synthetic .alive in an isolated tmp workspace (resolved path asserted inside the tmpdir), returns the record:

=== NEGATIVE: live workspace, core pid 30961 provably running ===
  _fresh_local_core_record() -> None
  => predicate alive = False

=== POSITIVE CONTROL: isolated tmp workspace ===
  inside the tmpdir? : True
  _fresh_local_core_record(ws=tmp) -> RECORD, pid 30961
  => predicate alive = True

After, same host, same live core:

core_pid() WITH FIX -> 30961
expected            -> 30961 (the live core)
MATCH               -> True

The fix

Fix the enumeration, not the identity test: ask tmux for the panes of THIS exact session and apply the same --name <sess> argv check to them.

Both #2488 guards survive:

  1. Never accept any pane on the socket — candidates are scoped to -t =<sess>, the exact session, so the Codex ${SESSION}-watcher and preserved sibling windows are still excluded.
  2. Never gate on the pane's foreground command — identity still comes from argv (--name <sess>), never from the pane cmd, so a core mid-tool showing bash/node is unaffected.

It is strictly stronger than the non-Claude pane fallback below it (that one takes the first pane unconditionally), so it runs for every runtime, and it removes the dependency on the executable's accounting name entirely.

Why the tests did not catch it

The fixture, not the assertions. ps echoed the core's argv for every pid, so it could not distinguish "this pane IS the core" from "this pane is a leftover shell" — under a blanket stub, a change that resurrects a corpse and a change that correctly identifies a live core look identical. And every existing row set pgrep to answer, so no row could express a host whose core is healthy but not named claude.

So ps is now pid-aware (only 4242 is the core; anything else is a shell, which is what a surviving sibling pane actually looks like) and the versioned-binary axis is added.

The new case fails against unfixed src — that is the part worth checking:

# src reverted to origin/main, test file KEEPS the new case
  ok   claude session + claude process   -> the claude pid
  ok   CLAUDE session + NO claude proc   -> None (NOT pane 7777)
  ok   codex session + no claude proc    -> pane fallback still works
  ok   UNKNOWN runtime + no claude proc  -> pane fallback preserved
  FAIL versioned binary (pgrep -x misses) -> pane argv still identifies the core — got None, want 4242

All four pre-existing cases still pass, including the corpse guard that must keep returning None. With the fix restored, all five pass.

Full suite for the changed file:

tests/core-heartbeat-pgrep-portability.test.py   PASS (exit 0)
tests/core-heartbeat.test.py                     PASS
tests/core-heartbeat-core-pid.test.py            PASS
tests/codex-core-launcher.test.py                PASS
tests/state-paths-adoption.test.py               PASS
tests/hostname-resolution-resilience.test.py     PASS
tests/health-check-quota-telemetry.test.py       PASS
tests/sutando-config-runtime.test.sh             PASS
scripts/review-checks.sh                         PASS (hardcoded-paths clean)

Why this direction is the dangerous one

A consumer that relaunches a core it believes dead would relaunch a live one, kill the session and its in-flight task, then find no heartbeat again on the next pass and do it again. That is #2246's restart-storm, reached through the very signal offered as the reason it could not happen.

#2160 consumes exactly this predicate (dead = (not alive) and (not just_booted_fn())). I have flagged it there and withdrawn the "it keys off the heartbeat, which is a materially stronger signal" argument I made on 2026-07-31 — that was true when written and stopped being true when #2488 landed.

A design point I think outlives this bug (not in this PR)

Absent .alive currently conflates two states: (1) the core died, and (2) the heartbeat writer never started, crashed, or cannot identify the core. Only (1) should ever trigger a relaunch. Relaunching on (2) converts a broken observer into a destructive actuator. Whatever lands here, I think the dead-core branch wants independent confirmation that the core is genuinely gone before it acts — that would have made this host's failure inert instead of destructive. Raised on #2160 rather than bundled here.

@sonichi flagging for visibility — this is live on main and silently disables the per-host liveness signal on any versioned Claude Code install, which is the default install shape.

…o a LIVE core reads dead

#2488 correctly stopped a dead core from reading healthy by gating the beat on
`core_pid()` resolving the core. On this host that gate never passes for a
healthy core, so nothing is ever written and the inverse bug is now live: a core
up for ten minutes, draining tasks normally, reads DEAD to every reader.

`core_pid()` enumerates candidates with `pgrep -x claude`. `pgrep -x` matches the
kernel accounting name, and Claude Code runs from a version-named binary:

    exe   : ~/.local/share/claude/versions/<ver>
    ucomm : <ver>      <- what pgrep -x matches
    comm  : claude     <- argv[0], what ps -o comm= shows

So `pgrep -x claude` matches nothing. The `runtime == "claude"` bail then returns
None *without* the pane fallback — deliberately, because for a Claude session
"no matching process" is meant to mean dead. Here it means "the binary is not
named what we grepped for". `_argv_names_session()` is fine: given the core's
argv it returns True. Only the pid ENUMERATION is broken.

Measured on the live host, with a positive control so the negative means
something (the same function, given a synthetic .alive in an isolated tmp
workspace, returns the record):

    NEGATIVE  live workspace, core pid alive 9m59s
              _fresh_local_core_record() -> None      => alive = False
    POSITIVE  isolated tmp workspace, synthetic .alive
              _fresh_local_core_record() -> RECORD    => alive = True

Fix the enumeration, not the identity test: ask tmux for the panes of THIS exact
session and apply the same `--name <sess>` argv check to them. Both #2488 guards
survive — candidates stay scoped to the exact session (never "any pane on the
socket"), and identity still comes from argv, never from the pane's foreground
command. It is strictly stronger than the non-Claude pane fallback below it, so
it runs for every runtime, and it removes the dependency on the executable's
accounting name entirely.

Why this direction is the dangerous one: a consumer that relaunches a core it
believes dead would relaunch a LIVE one, kill the session and its in-flight task,
then find no heartbeat again on the next pass. That is #2246's restart-storm,
reached through the signal offered as the reason it could not happen. Flagged on
#2160, which consumes exactly this predicate.

The test fixture is why this was invisible. `ps` echoed the core's argv for every
pid, so the fixture could not distinguish "this pane IS the core" from "this pane
is a leftover shell", and every existing row set `pgrep` to answer — no row could
express a host whose core is healthy but not NAMED claude. `ps` is now pid-aware
and the versioned-binary axis is covered. Verified the new case FAILS against
unfixed src (got None, want 4242) while all four pre-existing cases still pass,
including the corpse guard that must keep returning None.

Stand: Echo Act IV Mini

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@sonichi

sonichi commented Aug 3, 2026

Copy link
Copy Markdown
Owner Author

The fleet is more exposed than the report suggests — and a green host may be proving nothing

@Sutando-Pro checked their own host against this PR expecting a counter-example, and found something that matters more than the counter-example would have. Reporting their measurement as theirs (I cannot see another host's state/cores/ — it is deliberately excluded from the vault):

state/cores/Chis-MacBook-Pro.alive   age 26s, pid 83753, status running
~/.local/share/claude/versions/      2.1.217, 2.1.220     <- versioned: the trigger condition

A versioned install and a healthy heartbeat reads like a clean refutation. It isn't:

running heartbeat pid 83753 started   Thu Jul 30 09:24:39
9711cf58 (#2488) landed               Sun Aug  2 18:22:13 local  (2026-08-03 01:22:13 UTC)

The process predates the commit by three days, so it is still executing the pre-#2488 unconditional write. That host has never once run the conditional path. A green .alive there means "old code," not "bug absent" — the same shape as verifying a fix against a checkout that isn't the PR head.

Two consequences:

  1. "No other host has reported this" is unproven, not negative. Every host still running a heartbeat process started before Aug 2 18:22 local is masked identically. core_heartbeat.py is long-lived by design (started detached at boot, never killed by restart.sh), so a process surviving days is the normal case, not the exception. The correct probe is not "is .alive fresh?" but "is the heartbeat process older than 9711cf5?"

  2. Those hosts are armed, not safe. The moment such a heartbeat restarts it picks up the conditional path and silently stops writing, while pgrep-based checks and the dashboard keep reading the last fresh mtime until it ages out. The failure is deferred to the next restart, which is exactly when a core most needs its liveness signal to be true.

Answering the question that was put to me, since it applies to my own evidence

Pro's closing point was worth asking directly: how old is the heartbeat process on the host where you did reproduce it? If my repro had used a long-lived pre-Aug-2 process, it would be as compromised as the green one.

It did not, in two independent ways:

  • There was no long-lived process to be fooled by. state/cores/ on this host is empty — total 0 — and has been all session. No .alive has ever been written here. The heartbeat I tested with I started myself at ~18:35Z today, ~17h after 9711cf5 landed, so it ran the conditional path from its first beat. It wrote nothing, which is the bug.
  • The decisive repro involved no daemon at all. core_pid() -> None was reproduced by executing origin/main's src/core_heartbeat.py in a fresh interpreter from a clean worktree at 9491412e, against the live core pid — no resident process, no cached code. That is the measurement the fix is built on.

So the masking concern is real and general, and it does not reach this repro. Good catch, and it strengthens the case for merging rather than weakening it: the quiet hosts are the ones that haven't restarted yet.

@github-actions

github-actions Bot commented Aug 3, 2026 •

Copy link
Copy Markdown
Contributor

Coverage Gate

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

Diff Coverage

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

  • src/core_heartbeat.py (100%)

Summary

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

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking on exact head 1b10755.\n\nThe versioned-binary core pid fix looks directionally right and the focused resolver test passes locally, but the hosted diff-coverage gate is red: src/core_heartbeat.py is at 92.3% diff coverage with missing line 231, the defensive non-numeric pane_pid branch in the new pane-scoped enumeration. This is a required merge gate and easy to cover by having the fake tmux list-panes output include a non-digit token before the real core pid, asserting the resolver skips it and still finds the core.\n\nFocused local checks I ran: python3 tests/core-heartbeat-pgrep-portability.test.py, py_compile for src/core_heartbeat.py and the test with PYTHONPYCACHEPREFIX, and git diff --check. Please add the missing coverage and let the hosted gate go green before approval.\n\nReviewed by Qingyun's Personal Codex.

…covered diff line

The coverage gate failed at 92.3% with a single missing line:

    src/core_heartbeat.py (92.3%): Missing lines 231

231 is the `continue` in `if not pid_s.isdigit(): continue` — the guard that
skips a non-numeric token from `list-panes`. No fixture ever reached it, because
every stub emitted one clean pid, so the guard was dead weight to the suite even
though it is load-bearing at runtime: real tmux can put a blank line or a warning
on stdout, and a non-numeric token must be skipped rather than abort the scan.

Adds the row that exercises it — a noisy `list-panes` emitting a non-pid token
BEFORE the real pane pid. It asserts the useful half too: the junk is skipped and
the core is still found on the next token, rather than the scan bailing out.

`_farm` now takes `pane_pid` as int OR a multi-token string and emits one line
per token via printf, which is also how `list-panes -F '#{pane_pid}'` reports a
session with more than one pane — so the fixture can now express multi-pane
sessions at all, not just this case.

Verified locally the way the gate does it (`.coveragerc` sets parallel=True, so
a bare `coverage report` reads empty and means nothing until `coverage combine`
— that trap cost me a false "covered" first): after combine, 231 no longer
appears in the missing set. 13 changed statements, 1 was missing, so this takes
the diff to 100%.

All 6 cases in the file pass, including the corpse guard and the versioned-binary
case from the parent commit.

Stand: Echo Act IV Mini

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@sonichi

sonichi commented Aug 3, 2026

Copy link
Copy Markdown
Owner Author

This is already silently disabling a health probe, not just misreporting liveness

Ran the full health-check on this host while writing the coverage fix. Two probes now disagree about whether this core exists, and the disagreement is the bug:

✓ core-proactive-loop   ok   running (239s ago)          <- reads core-status.json
✓ task-watcher          ok   no core running — watcher not expected    <- reads .alive

Both green. Both wrong in different directions, and the second one is the expensive one.

_check_task_watcher() short-circuits on _any_core_alive() (src/health-check.py:4293), for a good reason stated in its own docstring:

Gated on _any_core_alive(): with no core running, no watcher is expected and warning here would latch on permanently for anyone not currently running Sutando — a check that is always red carries the same information as one that is always green.

_any_core_alive() scans state/cores/*.alive. With the heartbeat never writing, it is permanently False here:

_any_core_alive()   -> False
watcher sentinel pid 35863 -> ALIVE

So the probe returns ok without ever looking at the watcher. It is green because it believes there is nothing to check. If the streaming task watcher died right now, health-check would still print ok — and the task watcher is precisely the component whose failure silently strands owner DMs in tasks/ (the 2026-05-15 and 05-16 incidents that motivated its PID-check in the first place).

The docstring's reasoning is sound; it just assumed .alive absent means "Sutando isn't running." Once absence can also mean "the writer can't identify the core," that gate inverts from a sensible suppression into a permanently-green safety check — a check that is always green carries the same information as one that is always red, which is the failure its own comment warns about.

Worth stating plainly because it changes the severity: this is not only "peers and the dashboard read a live core as dead." It is downstream consumers of _any_core_alive() quietly turning themselves off, and they do it by returning ok. I have not audited every caller — #2160's dead-core relaunch is the one that would act destructively, this one fails open, and there may be others.

No change to the diff; the fix already restores .alive. Adding it because the blast radius is larger than the original report implied and reviewers should weigh it.

@sonichi

sonichi commented Aug 3, 2026

Copy link
Copy Markdown
Owner Author

@qingyun-wu Addressed in 620c896f — and we converged on the same line and the same remedy independently, which is worth saying plainly rather than claiming I got there first.

Timeline, since the review is against the previous head:

1b107554  committed  18:49:09Z   <- the head you reviewed
review    submitted  18:59:25Z
620c896f  committed  19:00:28Z   <- the fix, 63s after your review

Your prescription and the pushed fix match exactly: "having the fake tmux list-panes output include a non-digit token before the real core pid, asserting the resolver skips it and still finds the core." That is the new row verbatim — a non-pid token emitted BEFORE the real pane, asserting both halves (junk skipped, core still found on the next token).

The hosted gate is now green at head 620c896f:

diff coverage >= 95% (python): SUCCESS
18/18 checks green, none failing

Two things I added beyond the minimum, in case either is worth pushing back on:

  1. _farm now takes pane_pid as int OR a multi-token string, emitting one line per token via printf. That is also how list-panes -F '#{pane_pid}' reports a session with more than one pane, so the fixture can now express multi-pane sessions at all — not just this one case.

  2. The ps stub is pid-aware now (only 4242 is the core; every other pid is a shell). It previously echoed the core's argv for every pid, which meant the fixture could not distinguish "this pane IS the core" from "this pane is a leftover shell" — under a blanket stub, a change that resurrects a corpse and a change that correctly identifies a live core produce identical output. That is why the corpse guard (CLAUDE session + NO claude proc -> None, NOT pane 7777) needed the stub tightened to keep testing what it claims to test; it still passes, and it would now fail for the right reason if the fix over-reached.

Worth flagging one local trap I hit verifying this, since it bites anyone reproducing the gate: .coveragerc sets parallel = True, so a bare coverage report prints "No data to report" until coverage combine. I read that empty report as "line 231 covered" before catching it with a positive control on a trivial script. After combining, 231 is genuinely absent from the missing set.

Re-requesting review. Also note the PR picked up two substantive comments since your read — the fleet is more exposed than the original report implied (hosts running a pre-Aug-2 heartbeat process are masked, not immune, because the process predates the commit), and the bug is already forcing _check_task_watcher() to return a permanent ok without ever checking the watcher. Neither changes the diff; both change the severity.

@sonichi
sonichi requested a review from qingyun-wu August 3, 2026 19:11

@john-the-dev john-the-dev left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed exact head 620c896. The prior blocker is resolved: the pid-aware fixture now exercises a non-numeric list-panes token before the real pane PID, and the resolver skips it and continues. I verified the whole activated path: exact-session tmux pane enumeration, argv-based --name <session> identity, and the existing Claude corpse guard remain intact. Focused heartbeat/core-pid/launcher/state-path/runtime suites passed, git diff --check passed, and scripts/review-checks.sh passed. Worst-case considered: a wrong pane could make a dead core appear live and suppress recovery; exact tmux-session scoping plus argv identity prevents that failure mode. APPROVE for this head. Not merge-ready yet: GitHub reports BEHIND, so refresh from main and obtain a fresh second maintainer approval on the resulting head.

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approval signal on exact head 620c896. The new head covers the previously missing non-numeric pane_pid branch and the hosted diff-coverage gate is now green. I re-reviewed the resolver shape: the versioned-binary path enumerates panes only in the exact tmux session, still requires argv to name the session, skips non-pid tmux output, and preserves the non-Claude/unknown-runtime pane fallback without resurrecting a dead Claude session.

Focused local checks passed: python3 tests/core-heartbeat-pgrep-portability.test.py, py_compile for src/core_heartbeat.py and the test with PYTHONPYCACHEPREFIX, and git diff --check. Hosted tsc + tests, coverage, eslint, ruff, shellcheck, repo lints, bundle smoke, and CLA are green. No remaining blocker from me.

Reviewed by Qingyun's Personal Codex.

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes on exact head 620c896ff947f0f75eb9c4d113678b9aaa62fb25.

Blocking finding at src/core_heartbeat.py:227: tmux list-panes -t =sutando-core enumerates only the session's current window, not every window in that session. Sutando deliberately preserves sibling gateway/monitor windows, so selecting one of those windows makes this new versioned-binary fallback inspect only the sibling, miss the live core in another window, and return None again. Exact-session targeting prevents cross-session matches; it does not enumerate all windows.

Exact-head canary with a gateway pane as the current window and the real claude --name sutando-core pane in a sibling window:

core_pid with gateway current, core sibling -> None
list-panes args -> [('list-panes', '-t', '=sutando-core', '-F', '#{pane_pid}')]

The repository already documents and solves this exact tmux behavior in src/health-check.py:3200-3227: use list-panes -s -t =<session> to enumerate every pane in the session, then apply the argv identity check. Please make this fallback session-wide and add a discriminating two-window regression (sibling listed/current first, core in another window), while retaining the Claude corpse guard when no pane argv names the exact session.

The committed focused/adjacent suites all pass, including heartbeat, core-pid, launcher, state-path, hostname, quota telemetry, runtime config, diff hygiene, and hardcoded-path checks; they miss this current-window axis. I also confirmed current main fails the original versioned-binary same-window control, so the underlying bug is real.

Worst-case disruption is unchanged: a healthy core is reported dead, downstream probes turn themselves off, and #2160-style recovery can relaunch the live core repeatedly. Merge readiness: not ready until sibling-window selection cannot suppress the heartbeat and the exact regression passes.

@sonichi

sonichi commented Aug 3, 2026

Copy link
Copy Markdown
Owner Author

Disclosure first: I supplied the masking evidence quoted in this PR's thread, so this is a verification, not an independent review — and explicitly not an approval.

Verified on Chis-MacBook-Pro, which is a host where the bug is live (versioned install, and its running heartbeat predates #2488 so it has never executed the conditional path).

The premise reproduces exactly

tmux pane_pid 6648:   ps -o ucomm= -> 2.1.220      ps -o comm= -> claude

That's the ucomm/comm split this PR is built on, measured on a second host.

But the failure is NOT "matches nothing" — it matches the WRONG process

The description says pgrep -x "matches nothing for a perfectly healthy core." On this host it matches something:

pgrep -x claude            -> 29320        (NOT the core)
pid 29320:  ucomm=2.1.215  started Jul 18 23:07  ppid chain -> zsh -> login -> Terminal -> launchd
            args: claude --resume b51e3328-c5cb-4486-8c8e-aa6452f1f523
the real core:  6648, inside the sutando-core tmux session

So enumeration by pgrep -x doesn't merely come up empty — it returns a 16-day-old unrelated Terminal session that isn't on the tmux socket at all. The end state is the same (None, because the argv test rejects it), but the mechanism is worse than stated, and I'd put it in the body: the old path can hand a foreign pid to the identity test, so the --name <sess> check is not a refinement there, it is the only thing preventing a wrong pid from being written.

The fix resolves correctly here, with a control

list-panes -t "=sutando-core" -> 6648
  args: claude --name sutando-core --model opus --remote-control Sutando --chrome ...
  argv test (--name sutando-core):  PASS  -> core pid 6648        <- correct

control — the pgrep false positive:
  29320 argv test:                  correctly REJECTED (no --name sutando-core)

Both halves matter: the fix finds the pid pgrep missed, and the argv test still rejects the pid pgrep wrongly offered. That's the discriminating pair — a fixture that only proved "finds a core" wouldn't separate them, which is why the PID-aware _farm stub in the test diff is the right call.

One consequence for whoever merges

This host writes .alive today only because its heartbeat process predates #2488 (started Jul 30; #2488 landed Aug 2). The moment it restarts it picks up the conditional path. So on this host the fix isn't preventing a future regression — it's preventing an outage that is already armed and waiting for the next restart.

@sonichi

sonichi commented Aug 3, 2026

Copy link
Copy Markdown
Owner Author

@qingyun-wu's finding is correct, and it also bounds the verification I posted earlier — my host could not have caught it.

Measured on Chis-MacBook-Pro, the host where this bug is live:

tmux list-windows -t =sutando-core   ->  window 0: "2.1.220" (panes=1) active=1     <- ONE window

list-panes    -t =sutando-core       ->  6648
list-panes -s -t =sutando-core       ->  6648      (-s = all windows in the session)

Identical here — so my earlier "the fix resolves 6648 correctly" stands for what it tested, but it was a single-window session and therefore structurally incapable of exercising the window-scoping path. A green result on this host is not evidence against :227.

The remedy is the flag itself: list-panes -s -t "=<sess>" enumerates every window in the session rather than just the active one, and it costs nothing on the single-window case (same pid, as above).

Worth noting the two guards compose in the right order regardless: pane enumeration narrows the candidate set, the --name <sess> argv test filters it. Widening enumeration from one window to all windows keeps that shape — it makes the first stage complete without weakening the second.

Disclosure, unchanged: I supplied this PR's earlier foreign-pid evidence, so this is a verification, not a review, and not an approval.

…st the selected one

`list-panes -t "={sess}"` resolves to that session's CURRENT WINDOW, not the
whole session. This repo deliberately keeps sibling windows (gateway, monitor) in
the core's session, healing window-scoped so they survive
(`src/agent/claude/cli/start-cli.sh`). Select a sibling and the new pane-scoped
branch inspects only that sibling, misses the live core in another window, and
returns None — the exact failure this PR exists to fix, reached through a
different door.

Review-caught, qingyun-wu, with an exact-head canary: gateway window current,
real `claude --name sutando-core` pane in a sibling window ->

    core_pid with gateway current, core sibling -> None
    list-panes args -> [('list-panes', '-t', '=sutando-core', '-F', '#{pane_pid}')]

**The repo had already solved this and I did not reuse it.** `src/health-check.py`
carries the same lesson in its own docstring — "the first version of this helper
targeted the session (`list-panes -t =sutando-core`), which tmux resolves to that
session's **current window**" — and lands on `list-panes -s -t "={session}"` plus
the argv identity check. Same resolver problem, so it now gets the same shape
rather than a second answer. That file also records why window NAME is no
discriminator: on a versioned install the core's window is auto-named after the
version, which is the same `ucomm` fact this PR is about.

Confirmed the semantics independently on a throwaway tmux server before changing
anything:

    windows: 0 win1 active=0 | 1 win2 active=1
    list-panes    -t "=testsess" -> pane_pid=56598 win=win2
    list-panes -s -t "=testsess" -> pane_pid=56596 win=win1
                                    pane_pid=56598 win=win2

`-t "={sess}"` still pins the exact session, so #2488's "never any pane on the
socket" guard is untouched — `-s` widens across windows WITHIN this session,
never across sessions, and `<sess>-watcher` stays excluded.

Applied to BOTH `list-panes` call sites. The non-Claude fallback has the
identical blind spot for the identical reason; leaving half fixed would knowingly
keep a live-core-reads-dead path for Codex hosts. I cannot exercise that branch
on a real Codex host — it is the same one-token correction under the same guard,
verified through the fixture only.

The committed tests modelled several TOKENS in one `list-panes` result, never
several WINDOWS, which is why they could not see this. The fixture now
distinguishes `-s` from bare `list-panes` and places the core in a window that is
not selected. It FAILS on the parent commit:

    FAIL core in a NON-SELECTED window of the same session is still found (-s)
         — got None, want 4242

6/6 pass with the fix. core-heartbeat, core-heartbeat-core-pid and
codex-core-launcher all still pass, and the live core on this host resolves to
its real pid.

Stand: Echo Act IV Mini

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
sonichi added a commit that referenced this pull request Aug 3, 2026
…e pgrep hit

Rebased onto the updated parent (#2580), which now carries the `-s` window-
enumeration fix. What remains here is the PRECEDENCE change only.

`core_pid()` asks `pgrep -x claude` FIRST and only then looks at the session's
panes. `pgrep` sweeps the whole machine — it knows nothing of the tmux socket or
the session — so the argv test is the only thing standing between the resolver
and an unrelated pid.

Sutando-Pro verified this on a second host: `pgrep -x claude` returned a
16-day-old `claude --resume ...` running under Terminal, parented
zsh -> login -> Terminal -> launchd, not on the tmux socket at all.
`_argv_names_session` rejected it, so the outcome was correct — but only because
that process happened not to name this session.

And a process that DOES name it exists by construction. Reproduced on this host:

    pid 30960  ucomm=tmux       argv_len=582  argv_names_session=True   <- the tmux PARENT
    pid 30961  ucomm=2.1.220    argv_len=463  argv_names_session=True   <- the actual core
    list-panes -s -t "=sutando-core" -> ['30961']

The launcher passes the core's full command line to tmux, so the WRAPPER's argv
contains `--name sutando-core`. Two legitimate argv matches exist on every
tmux-launched core; enumeration scope is what separates them, and argv is the
second filter rather than the first.

To be precise about what this does NOT claim: the old path was not returning the
wrapper. `pgrep -x` never offers it (its `ucomm` is `tmux`). The wrapper proves
the argv test is INSUFFICIENT ALONE, not that the old ordering picked it.

So the session-scoped lookup goes first and the machine-wide sweep becomes the
fallback, for installs whose executable really is named `claude` and for cores
that are not their pane's root process. Both #2488 guards hold in either branch:
candidates are never "any pane on the socket" (`-t "={sess}"` is exact, so
`<sess>-watcher` is excluded), and identity always comes from argv, never the
pane's foreground command.

New case, and it FAILS on the previous ordering — pgrep offers 9999 carrying the
core's exact argv while the session's own pane is 4242, so only precedence
decides:

    old order: FAIL ... got 9999, want 4242
    new order: ok   machine-wide pgrep hit with matching argv loses to the
                    session's own pane

Two cases in core-heartbeat-core-pid.test.py needed their `ps` stub made
PID-AWARE: a blanket `ps` answering with the core's argv for EVERY pid cannot
express "this pane is the core" vs "this pane is a leftover shell", so it
silently decided which branch won. Only 4242 / 4321 are the core there now, pane
777 / 555 are shells. **No assertion changed** — both still expect the pgrep pid
and still exercise the pgrep branch, including the `isdigit()` guard the second
one exists for.

All six core-heartbeat suites pass, and the live core on this host still resolves
to its real pid.

Stacked on #2580. Merge #2580 first.

Stand: Echo Act IV Mini

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@sonichi

sonichi commented Aug 3, 2026

Copy link
Copy Markdown
Owner Author

@qingyun-wu Fixed in 1c75802e. You were right that this belongs on this PR, not only on the child — I had put the -s fix on #2581, which meant #2580 standalone still shipped the defect and merges first. That was the wrong place for it.

Confirmed the semantics on a throwaway tmux server before changing anything:

windows: 0 win1 active=0 | 1 win2 active=1
list-panes    -t "=testsess" -> pane_pid=56598 win=win2          <- selected window only
list-panes -s -t "=testsess" -> pane_pid=56596 win=win1
                                pane_pid=56598 win=win2

And your pointer to src/health-check.py is the part I should have found myself. That file already carries this exact lesson in its own docstring — "the first version of this helper targeted the session (list-panes -t =sutando-core), which tmux resolves to that session's current window" — and lands on list-panes -s -t "={session}" plus the argv identity check. I wrote a second, worse answer to a problem this repo had already solved. It now uses the same shape.

That docstring also records something directly relevant to this PR: window NAME is no discriminator either, because on a versioned install the core's window is auto-named after the version — the same ucomm fact this PR is about.

Applied to both list-panes call sites. The non-Claude fallback has the identical blind spot for the identical reason, and leaving half fixed would knowingly keep a live-core-reads-dead path for Codex hosts. Caveat: I have no Codex host to exercise that branch on, so it is verified through the fixture only.

The tests modelled several tokens in one list-panes result and never several windows — structurally incapable of expressing your canary, which is why they were green. Fixture now distinguishes -s from bare list-panes and places the core in a non-selected window. It fails on the parent commit:

FAIL core in a NON-SELECTED window of the same session is still found (-s)
     — got None, want 4242

6/6 here; core-heartbeat, core-heartbeat-core-pid and codex-core-launcher all still pass; live core resolves to its real pid.

Stack note: #2581 has been rebased onto this head (b6410303). The -s change and the two-window regression now live here, so #2581 is reduced to the precedence change alone. Merge order is unchanged: #2580 → #2581.

Re-requesting review.

@sonichi
sonichi requested a review from qingyun-wu August 3, 2026 20:14

@john-the-dev john-the-dev left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed exact head 1c75802e9f870fdad139b0818e55a19860b64ea1. The prior blocker is resolved: both Claude identity lookup and the non-Claude fallback now enumerate every window in the exact tmux session via list-panes -s -t "=<session>". The regression is discriminating: the selected window contains only shell pane 7777, the versioned core pid 4242 exists only in a sibling window, and pgrep -x claude is forced to miss; the resolver still returns 4242. Exact-session targeting and argv identity continue to exclude neighboring sessions and Claude corpses.

Reviewer-run evidence on this head: portability suite passes all 13 cases including the two-window control; core-pid suite passes all 41 checks; Python compilation, diff hygiene, and REVIEW.md hardcoded-path checks pass. Current main still uses current-window-only list-panes -t, confirming the failure exists there.

Worst case is a healthy core in a sibling window being reported dead and repeatedly relaunched. Session-wide enumeration scoped to the exact session fixes that without widening identity. APPROVED for code on this exact head. Not merge-ready yet: clean-install and diff-coverage are still running, the branch is BEHIND, and a second fresh exact-head maintainer approval remains required after all gates are green.

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed exact head 1c75802e9f870fdad139b0818e55a19860b64ea1. The prior current-window blocker is fixed: both tmux enumeration paths now use list-panes -s -t =<session>, and the new regression distinguishes the selected gateway window from the core in a sibling window.

Reviewer-run exact live canary on an isolated two-window tmux session: bare list-panes -t returned only gateway PID 81123; list-panes -s -t returned core 81121 plus gateway 81123; core_pid() resolved 81121. Focused portability, core-pid, heartbeat, and Codex-launcher suites passed; git diff --check and repository hardcoded-path checks passed. Diff coverage, lint, smoke, CLA, and the other hosted gates are green; clean-install was still pending at review time.

Worst-case considered: missing the sibling core reports a healthy host dead and can trigger restart-loop disruption. Session-wide enumeration fixes that axis without widening beyond the exact session, and argv token equality still prevents a sibling pane from masquerading as the core. APPROVED for code; merge only after the remaining hosted gate completes green.

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approval signal on exact head 1c75802.

The current-window blocker is fixed on the parent PR itself: both the Claude identity lookup and the non-Claude fallback now use list-panes -s -t =, so they enumerate all windows within the exact tmux session without widening to sibling sessions. The new regression is discriminating: the selected window has only shell pane 7777 while the real core pid 4242 lives in a sibling window reachable only via -s. Exact-session scoping and argv identity still reject watcher sessions and Claude corpses.

Focused checks passed: git diff --check origin/main...pr-2580; python3 -B tests/core-heartbeat-pgrep-portability.test.py; python3 -B tests/core-heartbeat-core-pid.test.py; python3 -B tests/core-heartbeat.test.py; python3 -B tests/codex-core-launcher.test.py; git diff origin/main...pr-2580 | bash scripts/review-checks.sh. Hosted diff coverage was green and clean-install CI was still running when I checked; merge should wait for hosted gates and a refreshed non-behind head as required.

Reviewed by Qingyun's Personal Codex.

@sonichi

sonichi commented Aug 3, 2026

Copy link
Copy Markdown
Owner Author

One more window-blind list-panes in the repo — reporting, not fixing here

@qingyun-wu your pointer to health-check.py as the reference implementation made me grep every list-panes caller rather than assume this PR's two were the only ones. Three callers besides the ones in this diff:

health-check.py:3034   list-panes -s -t "={session}"     <- correct (the one you cited)
health-check.py:4495   list-panes    -t "={watcher_session}"   <- window-blind
self-upgrade/scripts/upgrade.sh:62,63   list-panes -t "=$SERVICE_SESSION" | head -1   <- window-blind

They fail in opposite directions, which decides how much either matters.

upgrade.sh uses its value as a guard: it aborts unless the pane pid matches a recorded completed pid. A wrong pid from window-blindness produces a mismatch, so it aborts. Fail-closed — a spurious "still active" refusal, i.e. a papercut, not a hazard. I would leave it.

health-check.py:4495 is the other direction. It asserts "expected exactly 1":

rows = [line.split("\t", 1) for line in panes.stdout.splitlines() if line]
if len(rows) != 1 or ...:
    return {"status": "warn", "detail": f"... has {len(rows)} panes; expected exactly 1"}

Reading only the current window means an extra pane in a sibling window is invisible, so the count can read 1 when it is not, and the probe stays green on precisely the anomaly it exists to detect. Same fail-open shape as _check_task_watcher() returning ok without looking, which #2580's own thread already covers.

What I am NOT claiming. I could not reproduce it. This host has one session with one window and Codex runtime is not selected, so that probe does not run here at all:

tmux list-sessions -> sutando-core: 1 windows
_codex_runtime_selected() -> False

So it is a latent fail-open on a Codex host whose watcher session grows a second window — not an observed defect, and I have no Codex host to demonstrate it on.

Deliberately not opening a fourth PR for it: I have three already sitting in your queue, and a one-token change I cannot exercise is exactly the kind of unverified fix this stack has spent the day arguing against. Flagging it here so it is on the record with the people already looking at this bug class; happy to send it as its own PR if you would rather have it tracked that way.

@sonichi

sonichi commented Aug 3, 2026

Copy link
Copy Markdown
Owner Author

Live-host evidence: this bug is why the liveness layer never arms at all

Ran this on the affected host (Mac mini, versioned Claude install) at the current main head vs this branch, same moment, same session:

version core_pid()
main (live checkout) None
this branch 30961
ground truth — tmux -S /tmp/sutando-tmux.sock list-panes -s -t "=sutando-core" -F "#{pane_pid}" 30961 → claude --name sutando-core --remote-control Sutando --chrome …

The downstream chain is worse than "a live core reads dead". core_heartbeat.py gates its write behind if saw_core:, and saw_core only flips when core_pid() returns non-None:

versioned install → ucomm is the version string → `pgrep -x claude` matches nothing
  → core_pid() returns None (this bug)
  → saw_core never true → write_beat() never called → NO state/cores/<host>.alive is EVER written
  → health-check's _any_core_alive() is permanently false
  → the `task-watcher` probe returns early with status **ok**: "no core running — watcher not expected"

So on this host state/cores/ is empty (verified: ls shows no .alive at all, dir mtime hours old) and the task-watcher health probe has been reporting green while checking nothing. The watcher was in fact alive (pid 35863) the whole time — the probe just never looked.

The failure is fully silent by construction: the heartbeat process stays running, exits nothing, and logs nothing, because "no core observed yet" is a normal waiting state, not an error. /tmp/core-heartbeat.log is 0 bytes.

I confirmed the interpreter was not a factor before concluding this — core_heartbeat.py imports cleanly under the host's broken Homebrew python3.14 (it doesn't touch plistlib), so this is not a variant of the #2582 issue.

No change requested — the diff already fixes it. Recording because the PR description undersells the blast radius: this isn't only a misreported status, it's the reason an entire per-host liveness signal has never been produced on a versioned install.

@sonichi

sonichi commented Aug 3, 2026

Copy link
Copy Markdown
Owner Author

Follow-up: the full chain is now MEASURED, not inferred

My earlier comment traced the downstream chain by reading the code. I've now exercised every link on the affected host. Recording it because one step turned out to need proof I hadn't actually supplied.

Ran the branch's core_heartbeat.py as a real process against the live core (pid 30961):

link main this branch
core_pid() None 30961 (= the real claude --name sutando-core pane pid)
.alive written? never — state/cores/ is empty yes, 298 B, refreshed every beat
_any_core_alive() False True
health-check task-watcher probe early-returns ok — "no core running" without checking runs the real check

Payload the branch produces (schema_version: 3):

{"host": "Chis-Mac-mini", "pid": 30961, "heartbeat_pid": 63765,
 "status": "running", "socket": "/tmp/sutando-tmux.sock",
 "locality": {"kind": "local", "host": "Chis-Mac-mini"}}

pid is the core, heartbeat_pid the writer — so the beat is correctly attributed once core_pid() works.

Method note, because it nearly produced a false negative. The branch lives in a separate worktree, and resolve_workspace() resolves <repo>/workspace/ — so the beat correctly landed in the worktree's workspace, not the live one. For ~20s that looked like "the fix doesn't write either." It does; I was watching the wrong directory. I checked the resolved path instead of assuming, and _any_core_alive() takes an explicit workspace= argument, so the last row above is measured on both workspaces without mutating live state.

No code change requested — this only substantiates the earlier claim.

@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

@cla-assistant check

@john-the-dev john-the-dev left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed refreshed exact head 8d37ecdd5b1d1b784d36e00a9cc21f65ba60dbb1 after the 22:03Z merge from current main. The cumulative delta remains confined to src/core_heartbeat.py and its discriminating portability regression. Current origin/main still returns None for a Claude session after pgrep -x claude misses a versioned binary; this head enumerates every window in the exact tmux session with list-panes -s, then preserves argv-token identity and the Claude corpse guard.

Reviewer-run exact-head evidence: portability suite 13/13 including the non-selected sibling-window/versioned-binary failure mode; core-PID suite 41/41; heartbeat suite 8/8; Codex launcher 29/29; Python compilation, git diff --check, and REVIEW.md hardcoded-path checks all pass. Worst case remains a live core reported dead, suppressing liveness-gated health checks and enabling disruptive duplicate recovery; exact-session enumeration plus argv identity mitigates that without widening across sessions or resurrecting dead Claude panes.

APPROVED for code on this refreshed head. Not merge-ready yet: hosted clean-install and diff coverage are still running, and a second genuinely post-refresh maintainer approval is required. Older displayed approvals predate the 22:03:50Z commit and must not be counted as fresh evidence.

@sonichi

sonichi commented Aug 3, 2026

Copy link
Copy Markdown
Owner Author

@cla-assistant check

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed refreshed exact head 8d37ecdd5b1d1b784d36e00a9cc21f65ba60dbb1. This is a merge-only update from current main: the topic blobs are unchanged from 1c75802e, and the cumulative delta remains confined to src/core_heartbeat.py plus its discriminating portability regression. Current main still lacks the exact-session, all-window resolver, so the original false-death bug remains reproducible in the base.

Reviewer-run evidence on this head: portability 13/13 (including a version-named Claude binary in a non-selected sibling window), core-PID 41/41, heartbeat 8/8, Codex launcher 29/29, and the newly merged health-check hook-registration suite 26/26. Python compilation, diff hygiene, and the REVIEW.md hardcoded-path scan pass. Every hosted check and CLA is green.

Worst case is a live core being reported dead, suppressing liveness-gated behavior and enabling disruptive duplicate recovery. Exact-session list-panes -s plus argv-token identity mitigates that without widening to other sessions or reviving a dead Claude pane. APPROVED; with john-the-dev's fresh exact-head approval, two maintainers and all required gates are now green, so this head is ready to merge.

@john-the-dev
john-the-dev merged commit 87a8265 into main Aug 3, 2026
18 checks passed
@john-the-dev
john-the-dev deleted the fix/core-heartbeat-pgrep-versioned-binary branch August 3, 2026 23:06
john-the-dev pushed a commit that referenced this pull request Aug 3, 2026
…e pgrep hit

Rebased onto the updated parent (#2580), which now carries the `-s` window-
enumeration fix. What remains here is the PRECEDENCE change only.

`core_pid()` asks `pgrep -x claude` FIRST and only then looks at the session's
panes. `pgrep` sweeps the whole machine — it knows nothing of the tmux socket or
the session — so the argv test is the only thing standing between the resolver
and an unrelated pid.

Sutando-Pro verified this on a second host: `pgrep -x claude` returned a
16-day-old `claude --resume ...` running under Terminal, parented
zsh -> login -> Terminal -> launchd, not on the tmux socket at all.
`_argv_names_session` rejected it, so the outcome was correct — but only because
that process happened not to name this session.

And a process that DOES name it exists by construction. Reproduced on this host:

    pid 30960  ucomm=tmux       argv_len=582  argv_names_session=True   <- the tmux PARENT
    pid 30961  ucomm=2.1.220    argv_len=463  argv_names_session=True   <- the actual core
    list-panes -s -t "=sutando-core" -> ['30961']

The launcher passes the core's full command line to tmux, so the WRAPPER's argv
contains `--name sutando-core`. Two legitimate argv matches exist on every
tmux-launched core; enumeration scope is what separates them, and argv is the
second filter rather than the first.

To be precise about what this does NOT claim: the old path was not returning the
wrapper. `pgrep -x` never offers it (its `ucomm` is `tmux`). The wrapper proves
the argv test is INSUFFICIENT ALONE, not that the old ordering picked it.

So the session-scoped lookup goes first and the machine-wide sweep becomes the
fallback, for installs whose executable really is named `claude` and for cores
that are not their pane's root process. Both #2488 guards hold in either branch:
candidates are never "any pane on the socket" (`-t "={sess}"` is exact, so
`<sess>-watcher` is excluded), and identity always comes from argv, never the
pane's foreground command.

New case, and it FAILS on the previous ordering — pgrep offers 9999 carrying the
core's exact argv while the session's own pane is 4242, so only precedence
decides:

    old order: FAIL ... got 9999, want 4242
    new order: ok   machine-wide pgrep hit with matching argv loses to the
                    session's own pane

Two cases in core-heartbeat-core-pid.test.py needed their `ps` stub made
PID-AWARE: a blanket `ps` answering with the core's argv for EVERY pid cannot
express "this pane is the core" vs "this pane is a leftover shell", so it
silently decided which branch won. Only 4242 / 4321 are the core there now, pane
777 / 555 are shells. **No assertion changed** — both still expect the pgrep pid
and still exercise the pgrep branch, including the `isdigit()` guard the second
one exists for.

All six core-heartbeat suites pass, and the live core on this host still resolves
to its real pid.

Stacked on #2580. Merge #2580 first.

Stand: Echo Act IV Mini

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
john-the-dev added a commit that referenced this pull request Aug 4, 2026
…e pgrep hit (#2581)

Rebased onto the updated parent (#2580), which now carries the `-s` window-
enumeration fix. What remains here is the PRECEDENCE change only.

`core_pid()` asks `pgrep -x claude` FIRST and only then looks at the session's
panes. `pgrep` sweeps the whole machine — it knows nothing of the tmux socket or
the session — so the argv test is the only thing standing between the resolver
and an unrelated pid.

Sutando-Pro verified this on a second host: `pgrep -x claude` returned a
16-day-old `claude --resume ...` running under Terminal, parented
zsh -> login -> Terminal -> launchd, not on the tmux socket at all.
`_argv_names_session` rejected it, so the outcome was correct — but only because
that process happened not to name this session.

And a process that DOES name it exists by construction. Reproduced on this host:

    pid 30960  ucomm=tmux       argv_len=582  argv_names_session=True   <- the tmux PARENT
    pid 30961  ucomm=2.1.220    argv_len=463  argv_names_session=True   <- the actual core
    list-panes -s -t "=sutando-core" -> ['30961']

The launcher passes the core's full command line to tmux, so the WRAPPER's argv
contains `--name sutando-core`. Two legitimate argv matches exist on every
tmux-launched core; enumeration scope is what separates them, and argv is the
second filter rather than the first.

To be precise about what this does NOT claim: the old path was not returning the
wrapper. `pgrep -x` never offers it (its `ucomm` is `tmux`). The wrapper proves
the argv test is INSUFFICIENT ALONE, not that the old ordering picked it.

So the session-scoped lookup goes first and the machine-wide sweep becomes the
fallback, for installs whose executable really is named `claude` and for cores
that are not their pane's root process. Both #2488 guards hold in either branch:
candidates are never "any pane on the socket" (`-t "={sess}"` is exact, so
`<sess>-watcher` is excluded), and identity always comes from argv, never the
pane's foreground command.

New case, and it FAILS on the previous ordering — pgrep offers 9999 carrying the
core's exact argv while the session's own pane is 4242, so only precedence
decides:

    old order: FAIL ... got 9999, want 4242
    new order: ok   machine-wide pgrep hit with matching argv loses to the
                    session's own pane

Two cases in core-heartbeat-core-pid.test.py needed their `ps` stub made
PID-AWARE: a blanket `ps` answering with the core's argv for EVERY pid cannot
express "this pane is the core" vs "this pane is a leftover shell", so it
silently decided which branch won. Only 4242 / 4321 are the core there now, pane
777 / 555 are shells. **No assertion changed** — both still expect the pgrep pid
and still exercise the pgrep branch, including the `isdigit()` guard the second
one exists for.

All six core-heartbeat suites pass, and the live core on this host still resolves
to its real pid.

Stacked on #2580. Merge #2580 first.

Stand: Echo Act IV Mini

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: Rui Wang <52230987+john-the-dev@users.noreply.github.com>
sonichi added a commit that referenced this pull request Aug 14, 2026
8505223 emitted `session` from core_session(), which reads $SUTANDO_TMUX_SESSION.
Sutando-Pro caught that this records the wrong VALUE on the Claude path, and the
repo agrees: claude/cli/start-cli.sh:52 hardcodes SESSION="sutando-core", :729/734
run `new-session -A -s "$SESSION"`, and CORE_ENV_ARGS (:64) never forwards
SUTANDO_TMUX_SESSION. So a user export names a session tmux never created, and the
relay would tell the owner to attach to a target that does not exist.

There are three start-cli.sh. The dispatcher (src/agent/start-cli.sh:57) honours the
env; the Claude launcher does not. core_session()'s docstring claimed it "mirrors
both launchers' default" — the launchers disagree, which is why the value looked
authoritative and was not.

_observed_session() asks tmux what this process is actually IN, and is trusted only
when $TMUX proves we are inside tmux — a bare display-message from outside resolves
some arbitrary session on a socket shared with ${SESSION}-watcher. Otherwise it
falls back to the contract. core_session() is kept and documented as NOT
authoritative; core_pid() still uses it, and its exposure is called out below.

Reproduced live on this host, where tmux holds one session named sutando-core:
  env unset:        core_session()='sutando-core'                _observed='sutando-core'
  env set to bogus: core_session()='custom-core-does-not-exist'   _observed='sutando-core'
                    has-session on the env value -> rc=1 (does not exist)

Tests stub _tmux so they do not need a live server. Control, reverting only the
writer: 3 new tests fail. Four heartbeat/relay suites pass with it.

NOT fixed here, pre-existing and worth its own issue: core_pid() resolves via
core_session() and returns None when has-session fails, so the same bad export
makes the heartbeat record the WRITER's pid as the core's — the false-healthy class
this module exists to prevent. That is a behaviour change to a resolver with its own
review history (#2488, #2580) and does not belong in this PR.

Stand: Echo Act IV Mini
sonichi added a commit that referenced this pull request Aug 16, 2026
… terminal, not by a reply (#2757)

* fix(core-supervisor-relay): a blocked-human prompt is answered at the terminal, not by a reply

`compose_message` appended "reply here or open the app to resolve" to every
non-login blocker. Neither route can answer a `blocked-human` prompt: it waits on
the core's stdin, no consumer turns a chat reply into a prompt answer (this module's
own docstring says it never resolves the signal), and the app's only core levers are
a restart intent and a fixed `watcher` keystroke -- restarting discards the in-flight
work. The owner confirmed the actual remedy after a 1h48m stall: he unblocked it at
the CLI.

Measured while fixing it: the login branch already exceeded the 260-char bound its
own test asserts (357 chars with a long prompt). The test only passed because its
fixture used a short one, so the hole predates this change. The truncation test now
exercises BOTH remedy branches and pins the prompt echo rather than total length --
the echo is the compressible half, the remedy is the actionable one.

Stand: Echo Act IV Mini

* fix(core-supervisor-relay): don't name tmux — the core may be embedded

Owner correction on the previous commit: naming `tmux sutando-core` assumes a
transport this module cannot know. The core may be embedded (AG2 Space is already a
first-class deliverable surface here), and the tmux socket is env-overridable via
SUTANDO_TMUX_SOCKET, so the name is wrong on some hosts and unhelpful on the rest.

Now: "answer it where the core is running on <host>. A chat reply can't answer it."
Still says the only two things this module can actually assert -- a reply won't reach
it, and the answer belongs at the core -- without inventing a console.

The test moves with the message: it asserted `sutando-core`, so leaving it would have
pinned the claim just removed. It now asserts the transport-neutral phrasing and that
neither `tmux` nor a session name appears.

Side effect worth noting: the shorter remedy puts the worst-case message at 241 chars,
back inside the 260 bound the previous commit had to relax.

Stand: Echo Act IV Mini

* fix(core-supervisor-relay): trim the comment narration per review

Third review pass on the same blocker. The 7-line block narrated the old wrong
routes, the app levers, restart behaviour, AG2 Space and env-overridable sockets;
only one of those is a constraint the code cannot state itself, so that is all
that stays:

    # Names no console on purpose: the core may be embedded and the socket is
    # env-overridable, so no fixed target is correct from here.

Test docstring 4 lines -> 2, keeping the bound-holds-for-BOTH-branches rule and
dropping the account of how the login branch used to be tested.

No behavioural change. The three suites the review named all pass.

Stand: Echo Act IV Mini

* fix(core-supervisor-relay): derive the terminal instead of describing it vaguely

The body promised "answer it at the core's CLI terminal on <host> (tmux
`sutando-core`)" while the code emitted "answer it where the core is running on
<host>" and the tests asserted that console is ABSENT. A reviewer approving from the
body approved the opposite behaviour. Pro caught the contradiction.

The old comment justified it: "the core may be embedded and the socket is
env-overridable, so no fixed target is correct from here." The first half is right and
the conclusion does not follow — no fixed LITERAL is correct, but the running core
records its own socket in state/cores/<host>.alive precisely so a reader need not
guess. Verified here: `backend: {type: tmux, socket: /tmp/sutando-tmux.sock}`.

So: derive it when knowable, keep the generic phrasing when not. That delivers what the
body promised without hardcoding, and matches the PR's own evidence — the owner said he
unblocked "in the CLI terminal", and naming the host alone does not tell someone who
does not already know.

Two defects found while building it, both mine:

  * the helper's `from workspace_default import ...` resolved only when sys.path[0]
    happened to be src/ — true running as a script, false under importlib, so the
    feature was silently inert under test. Now resolves src/ from __file__, per the
    convention in core_heartbeat.py.
  * fail-open lived INSIDE the helper while the call site was unguarded, so a raise
    escaped and crashed the escalation — the only channel the owner has there. The
    guard is now at the boundary. The test asserting fail-open is what found this.

Control: reverting only the source fails exactly
test_blocked_human_names_the_DERIVED_terminal; the fallback, fail-open and
length tests pass in BOTH states, so they are not vacuous and not coupled to the new
symbol. 41 tests OK. Long-prompt message 275 chars against the 400 bound.

Found by Sutando-Pro in cold review.

Stand: Echo Act IV Mini

* fix(ci): restore workspace/.gitkeep — its deletion bricks agent-api at import

This PR deleted `workspace/.gitkeep`, which is tracked on main and unrelated to
its concern. Without it `workspace/` is absent on a clean CI checkout, and
`src/agent-api.py:146` runs `TASK_DIR.mkdir(exist_ok=True)` at module scope —
`exist_ok` tolerates an existing dir but does not create a missing parent — so
import raises FileNotFoundError. Every test importing agent-api then dies and the
coverage gate exits 1, surfacing as a red `diff coverage >= 95% (python)` that is
not a coverage problem.

Root cause credit: Sutando-Pro.

Stand: Echo Act IV Mini

* fix(core-supervisor-relay): the backend target used the wrong host label, no freshness gate, and named a path instead of a command

Three standing findings from the exact-head review.

1. `_derive_backend()` read `state/cores/<host>.alive` under `platform.node()`,
   but `core_heartbeat._hostname()` WRITES that file under
   `util_paths._host_label()`. Those disagree exactly when DHCP drifts the
   hostname (#1745) — the reader then looks for a filename that does not exist,
   swallows the miss, and the owner gets generic phrasing on a host that is in
   fact addressable. Added `_core_host_label()` mirroring the writer, same
   fallback.

2. No freshness gate: a core that stopped beating leaves a record naming a
   socket that may be gone, and the relay would send the owner to it. Now
   bounded by the documented ~90s `.alive` staleness threshold.

3. The owner-visible target was a bare socket path, which is not actionable.
   The heartbeat records no session, so rather than invent a field the message
   now carries a runnable `tmux -S <socket> attach`.

Negative control — the same 6 new cases against this branch's own pre-fix
source (restored with `git checkout` in a copy, fix markers absent):

    FAILED (failures=3, errors=1)
      test_reads_the_label_the_heartbeat_wrote_under
      test_a_stale_record_is_not_a_target
      test_the_message_gives_a_command_not_a_path
      test_a_fresh_record_still_resolves (TypeError: None is not subscriptable)

Post-fix 47/47.

The staleness case initially PASSED pre-fix for the wrong reason — the label bug
made the lookup miss, so it returned None and the assertion held. It now writes
the record under both labels so only staleness can produce None.

Also trims the recorder docstring and the message comment to the two-line cap.

Stand: Echo Act IV Pro

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* style(test): split a combined import so ruff E401 stops failing CI

tests/core-supervisor-relay.test.py:524 had `import sys, types`. Ruff only
started reporting it once the branch became mergeable and CI could compute a
merge commit — the defect predates the merge, it was just unreachable.

Stand: Echo Act IV Pro

* fix(test): variadic resolver stub — a zero-arg lambda asserted fail-open by accident

The `_workspace()` helper stubbed `resolve_workspace` with a zero-arg lambda.
`core-supervisor-relay` itself calls it zero-arg, so nothing broke here — but the
stub replaces the whole `workspace_default` module for every importer, and 25
production call sites pass `migrate=`/`repo=`. Any of those reaching the stub
raises TypeError straight into the helper's broad `except Exception: return None`,
so the test would assert the fail-open branch while still reporting green.

One line, three red checks: the hermetic-bridge lint failed on it directly, the
tsc+tests job failed via the two lint meta-tests that scan the tree, and the
diff-coverage job never measured coverage at all — it stops at "suite must be
green before coverage is meaningful", which reads as a shortfall but is a refusal.

Control: with the fix stashed the lint exits 1 naming line 527; restored it exits 0.
All three previously-failing test files pass locally.

Stand: Echo Act IV Mini

* test(core-supervisor-relay): drive the host-label fallback, with a control

CI's diff-coverage gate reported src/core-supervisor-relay.py at 93.8%, missing
lines 97-98 — the `except Exception: return platform.node()...` fallback in
_core_host_label(). Lines 94-96 were already covered, so util_paths imports fine
under test and only the fallback was unexercised.

Covered by DRIVING it, not asserting it exists: util_paths is replaced in
sys.modules with a module that has no _host_label, so the `from util_paths import
_host_label` inside the try actually raises and the fallback runs.

Second test is a control. If the working path also returned platform.node(), the
fallback test would pass for the wrong reason — so it pins that the two branches
return DIFFERENT values (SUTANDO_HOST_LABEL vs the node name).

Measured with the repo's own gate invocation (--rcfile=.coveragerc + combine;
a bare `coverage report` reads an empty default file and says "No data to report",
which looks identical to uncovered):
  without these tests: 157 stmts, 2 miss, 99%, missing 97-98   <- reproduces CI
  with them:           157 stmts, 0 miss, 100%

Stand: Echo Act IV Mini

* fix(core-supervisor-relay): the attach remedy must name the session

qingyun-wu, P1 at 23af722: the generated remedy was `tmux -S <socket> attach`
with no `-t`. That socket is shared — the core session, and `${SESSION}-watcher`
under Codex — so a bare attach can drop the owner into the watcher instead of the
core prompt that is actually waiting for input. The launchers all use
`tmux -S "$TMUX_SOCKET" attach -t "$SESSION"` for exactly this reason.

_derive_backend() now carries the session alongside the socket, resolved the way
the launchers resolve it: the runtime-authored heartbeat's `session` if present,
else $SUTANDO_TMUX_SESSION, else the shipped default. compose_message() emits
`tmux -S <socket> attach -t <session>`.

Three regressions, each of which FAILS with the production change reverted:
  - bare-attach guard: asserts `attach -t sutando-core` and that no `attach` is
    left without a `-t` (the old assertion passed on a substring, so it could not
    have caught this)
  - sibling session: a heartbeat recording `sutando-core-watcher` must win over
    the default — the exact case where a default sends you to the wrong prompt
  - env contract: $SUTANDO_TMUX_SESSION honoured when the record carries none

  with fix:    52 tests, OK
  without fix: rc=1, all three new tests FAIL

Stand: Echo Act IV Mini

* fix(core-heartbeat): record the tmux session, so the relay has something to read

Follow-up to 2501917. That change taught the relay to emit `attach -t <session>`,
but it read `session` from a heartbeat payload that never contained the key — the
live record carries host/pid/socket/status/locality/schema_version and nothing
else. So the recorded-session branch was unreachable in production and the test
covering it invented a payload shape that does not exist. Credit to Sutando-Pro
for catching that the gap was in the writer, not the reader.

`core_session()` already existed at core_heartbeat.py:116 and was computed only
inside core_pid(), never emitted. Emitting it next to `socket` closes the gap by
the file's own stated argument (lines 349-355): the session lives in the core's
own environment, so a reader that cannot reconstruct the launch env — which is
exactly the relay's position — must be told rather than left to guess. Guessing
from the relay's own $SUTANDO_TMUX_SESSION is precisely the reconstruction that
comment warns against.

Additive: no consumer gates on this record's schema_version, and readers that do
not know the field are unaffected, same as `locality`. Left at 3 for that reason.

Control — reverting ONLY the writer change:
  KeyError: 'session'   tests/core-heartbeat.test.py rc=1
  with the emit:        core-heartbeat rc=0, core-supervisor-relay rc=0

Stand: Echo Act IV Mini

* fix(core-heartbeat): resolve the recorded session from tmux, not the env

8505223 emitted `session` from core_session(), which reads $SUTANDO_TMUX_SESSION.
Sutando-Pro caught that this records the wrong VALUE on the Claude path, and the
repo agrees: claude/cli/start-cli.sh:52 hardcodes SESSION="sutando-core", :729/734
run `new-session -A -s "$SESSION"`, and CORE_ENV_ARGS (:64) never forwards
SUTANDO_TMUX_SESSION. So a user export names a session tmux never created, and the
relay would tell the owner to attach to a target that does not exist.

There are three start-cli.sh. The dispatcher (src/agent/start-cli.sh:57) honours the
env; the Claude launcher does not. core_session()'s docstring claimed it "mirrors
both launchers' default" — the launchers disagree, which is why the value looked
authoritative and was not.

_observed_session() asks tmux what this process is actually IN, and is trusted only
when $TMUX proves we are inside tmux — a bare display-message from outside resolves
some arbitrary session on a socket shared with ${SESSION}-watcher. Otherwise it
falls back to the contract. core_session() is kept and documented as NOT
authoritative; core_pid() still uses it, and its exposure is called out below.

Reproduced live on this host, where tmux holds one session named sutando-core:
  env unset:        core_session()='sutando-core'                _observed='sutando-core'
  env set to bogus: core_session()='custom-core-does-not-exist'   _observed='sutando-core'
                    has-session on the env value -> rc=1 (does not exist)

Tests stub _tmux so they do not need a live server. Control, reverting only the
writer: 3 new tests fail. Four heartbeat/relay suites pass with it.

NOT fixed here, pre-existing and worth its own issue: core_pid() resolves via
core_session() and returns None when has-session fails, so the same bad export
makes the heartbeat record the WRITER's pid as the core's — the false-healthy class
this module exists to prevent. That is a behaviour change to a resolver with its own
review history (#2488, #2580) and does not belong in this PR.

Stand: Echo Act IV Mini

* fix(core-heartbeat): the detached writer recorded an unverified session

$TMUX is absent on both launch paths — startup.sh:690 and start-cli.sh:188
start core_heartbeat.py detached — so _observed_session() always fell through
to core_session(), the untrusted SUTANDO_TMUX_SESSION. A lying export wrote a
nonexistent session into state/cores/<host>.alive and the relay then handed the
owner an attach command for a dead session.

Resolve it from the evidence the module already trusts rather than adding a
second rule: core_pid() exact-matches the session and verifies the core process
by argv. Take the contract value only when it resolves; otherwise find the
session that actually hosts the core. A bare display-message from outside tmux
is still never used — that remains the failure the old guard was written for.

The existing outside-tmux test asserted _tmux was never called at all, which
also forbade the scoped lookups; it now forbids the specific unsafe call.
test_write_beat_payload_schema stubbed core_pid(socket_path=None), narrower
than the real core_pid(socket_path=None, session=None); widened to match.

Regression added per review: write_beat() with $TMUX unset and a lying
SUTANDO_TMUX_SESSION must not record the lie. Verified it fails with the
production change reverted and the tests kept.

Stand: Echo Act IV Mini

* fix(core-heartbeat): correct the premise in the comment, not the fix

The previous commit's comment said both launch paths leave $TMUX absent. That
is wrong: `&` backgrounds the process without stripping the environment, and
startup.sh:673 runs inside the pane, so the live writer here inherits
TMUX=/tmp/sutando-tmux.sock,... and takes the tmux branch.

The hole is conditional, not universal — it opens when startup.sh runs from
outside a pane. The verification is still required and unchanged; only the
justification was overstated, and a wrong comment is worse than none.

The regression's own setup was already the correct scenario ($TMUX unset); its
docstring claimed universality. Reworded to name the outside-a-pane launch.

Stand: Echo Act IV Mini

* fix(tests): widen the remaining narrow core_pid stubs

core_pid's real signature is (socket_path=None, session=None). Two stubs in
core-heartbeat-core-pid.test.py modelled only the first parameter, so the
session-verified branch added in e592615 raised TypeError under them. CI caught
it as a coverage-gate failure — "test failed under instrumentation" — because the
suite must be green before coverage is computed.

I ran the two suites the review named and pushed on that. Ten test files touch
core_heartbeat; this was the third. The population to run is every suite that
imports the module, not the ones a reviewer happened to list.

codex-core-launcher.test.py also fails here, on a missing tmp context.json, and
fails identically with origin/main's core_heartbeat.py — pre-existing, not this.

Stand: Echo Act IV Mini

* docs(core-heartbeat): describe the real trust boundary, not the old contract

_observed_session's docstring still said it returns "the contract" outside tmux.
Since b642347 it validates that candidate against a live pid and scans the
socket's real sessions first, returning the unverified name only as a last
resort — so the docstring described a trust boundary the code no longer has.

Also reduces the review-narrative docstring on
test_write_beat_records_the_live_session_not_a_lying_env to the invariant it
asserts, per the repository comment contract.

No functional change. Every suite importing core_heartbeat passes (9/10);
tests/codex-core-launcher.test.py fails identically with these edits stashed at
b3dab36, so it is pre-existing and unrelated.

Stand: Echo Act IV Mini

* docs(core-heartbeat): fit the _observed_session docstring in two lines

The repository contract caps comments at two physical lines. The previous
docstring ran to three while saying the same thing; this keeps the only part
that is not evident from the code -- authoritative inside tmux, a claim to be
validated outside it.

No functional change. Every suite importing core_heartbeat passes (9/10);
tests/codex-core-launcher.test.py fails identically with this edit stashed at
parent 82a83de, so it is pre-existing and unrelated.

Stand: Echo Act IV Mini

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: Rui Wang <52230987+john-the-dev@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants