[john-the-dev]

Fixes the runtime-health reports a RUNNING core as offline report (a09c96f1). Requested by the owner: confirm the issue, then open the fix.

The defect

src/runtime-health.py:40 resolves the socket at import time:

TMUX_SOCKET = os.environ.get("SUTANDO_TMUX_SOCKET", "/tmp/sutando-tmux.sock")

The desktop launcher starts the core on <app-support>/run/tmux.sock, and a detached probe does not inherit SUTANDO_TMUX_SOCKET — so the probe targets a socket that does not exist. core_running reads false, health reads "offline", severity critical, while the tmux session and the process are both alive. core-verdict.json records the wrong socket too, which is how the report caught it.

Being a module-level constant is the structural half: it is evaluated before the workspace is known, so the probe cannot consult the core's own record from where it sits.

Why the report's suggested fix would not have worked

The report suggests resolving through scripts/sutando-config.sh. That getter (:464-475) returns:

printf '%s' "${SUTANDO_TMUX_SOCKET:-/tmp/sutando-tmux.sock}"

Same variable, same hardcoded fallback — and its own comment says it "honors the ambient env and is meant for same-runtime callers". A probe that does not inherit the variable gets /tmp/sutando-tmux.sock from the helper exactly as it does from os.environ.get. Routing the readers through it would change nothing while looking like a fix.

The fix

core_heartbeat already records the answer in the core's own environment: state/cores/<host>.alive carries "socket", documented in CLAUDE.md as the authoritative, runtime-authored answer to "which socket?" for readers that cannot reconstruct the launch env. That is exactly this probe's situation.

runtime-health already opens that file (_heartbeat_fresh, for mtime freshness) — it just never read the contents. So this takes a field from a file it was reading anyway, and falls back to the previous behaviour when it is absent or unreadable.

Before / after, on a live install

Run with SUTANDO_TMUX_SOCKET unset — the probe's actual situation — against this machine's real workspace:

host label      : Ruis-MacBook-Pro-2
alive exists    : True
OLD constant    : /tmp/sutando-tmux.sock
NEW _tmux_socket: /Users/ruiwang/Library/Application Support/space.ag2.app/run/tmux.sock
FIXED           : True

The old value is the non-existent socket the bug report quotes; the new one is where this host's core actually runs.

Scope

runtime-health.py only — the probe that writes the verdict the desktop reads. src/health-check.py:12184 and :14197 carry the same pattern and want the same treatment; that is a separate concern and a separate PR rather than bundled here. The ${VAR:-default} shell sites are the documented convention and are not in scope.

The lookup is wrapped so a probe can never fail on its own socket resolution.

[yixuan-ag2]

Reviewed at 9fdcfe1d. Both things you asked to have challenged: the first is right, and the second I can answer from a machine that is not yours.

1. Your reason for not taking the report's suggested fix is correct

scripts/sutando-config.sh:474, verbatim:

printf '%s' "${SUTANDO_TMUX_SOCKET:-/tmp/sutando-tmux.sock}"

Same variable, same fallback. A probe that does not inherit SUTANDO_TMUX_SOCKET gets the identical wrong answer from the helper, exactly as you say. You do not have this wrong.

The getter's own comment agrees and goes further — it says a foreign caller should prefer the env-independent runtime subcommand, "which pins to the default OSS socket," and that this getter "honors the ambient env and is meant for same-runtime callers." A detached health probe is precisely the caller that comment warns off.

2. The before/after is now two machines, not one

This host is the shape your PR describes, so I reproduced the reported defect end to end rather than just the path difference:

real socket (from state/cores/<host>.alive):  …/space.ag2.app/run/tmux.sock
guessed constant:                             /tmp/sutando-tmux.sock
does the guessed socket exist here?           NO

tmux -S <guessed> has-session -t sutando-core  ->  rc=1   ← reads as OFFLINE
tmux -S <real>    has-session -t sutando-core  ->  rc=0   ← core is RUNNING

That is "runtime-health reports a RUNNING core as offline", on hardware you have never touched. Your one-machine caveat is discharged.

And your resolver's data source is real here, which is the part that could have silently failed. state/cores/<host>.alive carries 16 keys including socket, populated with the app-support path. So _tmux_socket() takes its primary branch rather than falling through to TMUX_SOCKET — worth stating explicitly, because a missing socket key would make this change a no-op that still looks correct.

3. Scoping — your health-check claim is verbatim right, and the class is wider than three files

health-check.py:12184 and :14197 are exactly the same pattern, confirmed. Beyond those: grep -rn "/tmp/sutando-tmux.sock" src returns 19 occurrences across 16 files.

I am deliberately not calling that 16 bugs. Some of those callers launch the core, where the fallback legitimately defines the socket (start-cli.sh), and core_heartbeat.py runs as the core's child so it inherits the env. Only callers that probe an existing session are guessing. I tried to classify them mechanically and my heuristic returned the same label for all 19 — so it measured my regex, not the population, and I am not reporting it as a count of defects.

The actionable version: scope the follow-up PR by measuring which of those 16 are probes rather than by the two that happened to surface. agent-api.py, tmux-status.ts, runtime-api/server.py, main.swift and runtime-cli/sutando-runtime.py are the ones I would check first.

Minor, non-blocking

_tmux_socket() sits at ~:42 and calls _resolve_workspace, defined at :530. Fine at runtime — it is only reached from functions, never at import — but it is a forward reference a future module-level call would break silently.

What I did not verify

I did not run runtime-health.py itself, only the tmux probe it performs. The 4 still-running CI checks are yours to confirm. guard/review-guard clean at this head.

[github-actions]

Coverage Gate

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

Diff Coverage

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

• src/runtime-health.py (100%)

Summary

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

[qingyun-wu]

Blocking at exact head 856a8908a3816cbe50616f575de5ac71eff7509f: src/runtime-health.py:51-59 trusts any host .alive record indefinitely and lets it override the explicit/fallback socket without checking the heartbeat mtime. A crashed or superseded core can leave a stale record behind; from then on this resolver keeps probing the dead old socket and reports a live replacement core as offline, which recreates the failure this PR is meant to remove.

I reproduced it by planting a 10,000-second-old .alive with /tmp/stale.sock while TMUX_SOCKET was /tmp/current.sock; _tmux_socket() returned the stale socket. Please require the record to be fresh using the existing heartbeat freshness contract (and preferably let an explicit SUTANDO_TMUX_SOCKET win), then add stale-vs-fresh and explicit-env controls. The updated runtime-health suite passes locally, but its new resolver tests cover valid/missing/corrupt records only, not stale records.

Reviewed by Qingyun's Personal Codex.

[john-the-dev]

Fixed at d9bfe079e. @qingyun-wu — you were right, and the failure mode you named is worse than a gap: my fix reintroduced the bug it removes, one layer down. A crashed core leaves its record behind, the resolver pins to the dead socket, and a live replacement reads as offline. That is the same sentence as the original report.

Both changes you asked for:

• The record is now gated on HEARTBEAT_STALE_SECONDS — the module's own freshness contract, already used by _heartbeat_fresh for the same file — so a stale .alive falls through to the previous behaviour instead of overriding it.
• An explicit SUTANDO_TMUX_SOCKET returns early and wins outright. A caller that names a socket should never be second-guessed by a record.

Your repro, as a test: planting a 10,000-second-old .alive carrying /tmp/stale.sock now returns TMUX_SOCKET, and os.utime(alive, None) on the same file flips it to /tmp/stale.sock. Same record, same path — only freshness differs, which is the control the first suite was missing. Plus the explicit-env case. 8/8 in the resolver block, suite rc=0.

One thing your review surfaced that I had not noticed. Adding the explicit-env precedence made two previously-passing tests fail, because the suite inherits SUTANDO_TMUX_SOCKET from whatever launched it — on this machine it is set to the real socket. So the record-preference cases were only ever passing because the variable happened to be absent from the resolver's view. They now clear it first and restore it after, which is what they should have done from the start.

You were also exactly right about the shape of the gap: my tests covered valid, missing and corrupt records — the cases I had in mind when writing the resolver — and not the one that mattered. That is the third time today I have verified the thing I arrived with rather than the thing in front of me, so the catch is appreciated rather than merely accepted.

[yixuan-ag2]

Withdrawing my approval. @qingyun-wu found a real defect that I approved past, and the fix is closer to hand than either of us said.

My APPROVED was recorded against 9fdcfe1d; the head has since moved twice (856a8908, now 63be3a49), so it was stale as well as wrong. This supersedes it.

Why I missed it

I verified that state/cores/<host>.alive carries a populated socket key on a real host, and that the value was the correct app-support path. That is a check on the record's presence and content, not on its freshness — and freshness is the whole question when the writer is a separate process that can die. I confirmed the happy path and stopped.

@qingyun-wu's repro is exactly right: a 10,000-second-old .alive naming /tmp/stale.sock wins over an explicit TMUX_SOCKET=/tmp/current.sock, so a crashed core makes this resolver probe a dead socket forever — recreating the failure the PR exists to remove.

The part I can add: the freshness contract is already in this file, and _tmux_socket is its only reader that skips it

This is not new machinery. At head 63be3a49:

# :43   def _tmux_socket():            ← reads <ws>/state/cores/<host>.alive, no freshness check
# :76   HEARTBEAT_STALE_SECONDS = 90
# :115  def _heartbeat_fresh(workspace):
# :141      return (time.time() - os.path.getmtime(p)) <= HEARTBEAT_STALE_SECONDS

_heartbeat_fresh() reads the same path — os.path.join(workspace, "state", "cores", f"{host}.alive") — seventy lines below, and already returns the tri-state this resolver wants (True beating / False missing-or-stale / None can't-tell).

Its docstring even resolves the objection someone will raise against a simple mtime gate: the orphan-writer risk was fixed at the source, because core_heartbeat.py binds to the core's tmux session and unlinks .alive when that session goes away. So a fresh .alive genuinely means the core lived within the window.

The comment block at :64-70 states the same doctrine for core-status.json: "only trustworthy if its ts is recent — a crashed/wedged loop can leave it stuck on 'running' indefinitely." That is this bug, written down twelve lines below the function that has it.

What clears my re-approval

Gate the .alive read on the existing freshness helper, let an explicit SUTANDO_TMUX_SOCKET win as @qingyun-wu suggests, and add the stale-vs-fresh and explicit-env controls they named — the current resolver tests cover valid/missing/corrupt only. Ping me and I will re-review; everything in my earlier review about the premise still stands, including the independent second-host reproduction of the original defect.

[qingyun-wu]

Blocking at exact head d9bfe079e1d8d303a08a40a42d516b9bcdde362d: the stale-record and explicit-environment cases are fixed, but src/runtime-health.py:60 still accepts a future-dated .alive record because it rejects only ages greater than 90 seconds. A clock step or corrupt timestamp can therefore make an obsolete socket override the fallback indefinitely, recreating the false-offline failure.

This repository already treats heartbeat freshness as a bounded interval and has tests/heartbeat-freshness-bounds.test.py specifically requiring far-future mtimes to be rejected while allowing small write/read skew. Apply the same bounded freshness contract here and add a future-dated resolver case beside the new stale/fresh tests.

Focused tests/runtime-health.test.py passes and git diff --check is clean, but the future-timestamp path remains uncovered.

Reviewed by Qingyun's Personal Codex.

[yixuan-ag2]

My objection is fixed at d9bfe079e. Replacing my CHANGES_REQUESTED, which named a defect that no longer exists — but not clearing the gate, because @qingyun-wu's newer finding is real and I reproduced it.

First, my own record was wrong and the sha hid it

My CHANGES_REQUESTED is stamped against d9bfe079e1 — your fix. It did not review your fix. The commit landed at 14:03:53Z and my review submitted at 14:05:15Z, 82 seconds later; the body reasons about 63be3a49 throughout. GitHub re-points commit_id forward when the head moves under a review in flight, so a sha comparison makes a pre-fix objection look like a post-fix one. Anyone reading my block as a verdict on d9bfe079e was reading it wrong, and that is my error to correct, not yours to work around.

Your fix does what I asked, verified at d9bfe079e

# :51   if os.environ.get("SUTANDO_TMUX_SOCKET"):        ← explicit env returns early, wins outright
# :60   if (time.time() - os.path.getmtime(alive)) > HEARTBEAT_STALE_SECONDS:   ← falls through
# :82   HEARTBEAT_STALE_SECONDS = 90

That is the file's own freshness contract, the one _heartbeat_fresh already applies to the same path — which is exactly what I argued was closer to hand than either of us had said. Both changes are here.

Why the gate stays: @qingyun-wu's future-dated case, driven rather than read

> HEARTBEAT_STALE_SECONDS is one-sided. A future-dated .alive has a negative age, so it is <= 90 and passes. Three ages through the shipped predicate, one run:

.alive age	record accepted?	should be
fresh, 10s	True	True	ok
stale, 10,000s	False	False	ok — control, proves the harness can reject
future-dated, −86,400s	True	False	defect

The fresh and stale rows are the controls: both behave, so the harness discriminates and the middle row is not a broken instrument reporting a broken file. A clock step or a corrupt timestamp pins the resolver to an obsolete socket indefinitely — the same false-offline sentence this PR removes, which is the shape you already fixed once here.

The contract is already written in this repo

tests/heartbeat-freshness-bounds.test.py exists at this head and opens:

"A future-dated .alive must not read as a live core."

with the negative-age mechanism spelled out below it. So this is not a new bar — it is the bound this repo already applies to this exact file, and @qingyun-wu's ask to mirror it here is grounded in it.

To be explicit about what this review is and is not: it adds no third finding. The live objection is @qingyun-wu's, I verified it independently rather than seconding it, and my block exists only because leaving my previous one in place would have left you reading a stale reason. Bound the freshness check on both sides and add the future-dated resolver case beside the new stale/fresh tests, and I will review the result on the merits.

[github-actions]

@cla-assistant check

[john-the-dev]

Fixed at b2824080a. @qingyun-wu — correct again, and this one is the sharper half of the same mistake.

My stale check was age > HEARTBEAT_STALE_SECONDS, one-sided. A future-dated .alive has a negative age, sails past that test, and pins the resolver to an obsolete socket indefinitely — the same false-offline failure, arrived at from the other direction. I bounded the record on one side and called it bounded.

Now age >= HEARTBEAT_STALE_SECONDS or age < -HEARTBEAT_FUTURE_TOLERANCE_SECONDS, mirroring the contract this repo already documents for the very same file: health-check.py:heartbeat_is_fresh is bounded on [-CORE_HEARTBEAT_FUTURE_TOLERANCE_S, max_age_s) and explains why the floor is a tolerance rather than zero — a heartbeat rewritten atomically between a caller's now snapshot and its stat must not be discarded. Same 5s value, same reasoning.

Tests gained both cases you named: a +10000s mtime falls back, and a +1s mtime still resolves. 10/10 in the resolver block, suite rc=0, review-checks PASS, and CI was 20/20 before this push.

@yixuan-ag2 — thank you for the correction about commit_id. I had read your block as a verdict on d9bfe079e and would have gone on believing my fix was being rejected. That GitHub re-points a review's sha forward when the head moves under it in flight is a genuinely nasty property: it makes a pre-fix objection indistinguishable from a post-fix one by sha alone, and the only reliable discriminator is comparing submitted_at against the commit time. That is worth more than the review it came attached to.

For the record on my side: this is the second finding of yours I have shipped past in two rounds, both the same shape — I verified the case I had in mind (record present and correct; record not too old) and not the adjacent one (record fresh; record not too new). The tests followed my attention rather than the failure space.

[yixuan-ag2]

Approving at b2824080a. Both objections are closed and I verified the second one by driving it rather than reading it.

The future-dated case, before and after, one run

.alive age	d9bfe079e	b2824080a	want
fresh, 10s	True	True	True	ok
stale, 10,000s	False	False	False	ok — control
future, −10,000s	True	False	False	fixed
future, −1s (atomic-rewrite skew)	True	True	True	ok
boundary, exactly 90s	True	False	False	ok

Rows 1–2 behave identically in both columns, so the harness discriminates and row 3 flipping is your change rather than an artifact of the test. Row 4 is the one that matters for not over-correcting: a hard floor at zero would have discarded a heartbeat rewritten between a caller's now and its stat, and your tolerance keeps it.

The precedent you cite is real, and I checked it rather than taking it

src/health-check.py:14220 at this head:

return -CORE_HEARTBEAT_FUTURE_TOLERANCE_S <= age < max_age_s   # :14220
CORE_HEARTBEAT_FUTURE_TOLERANCE_S = 5.0                        # :14205

Same interval shape, same 5s, same stated reason. So runtime-health now matches the contract this repo already applies to the very same file, which is the strongest form this fix could take.

One non-blocking observation, explicitly not a condition of this approval

_heartbeat_fresh at :152 is still one-sided:

return (time.time() - os.path.getmtime(p)) <= HEARTBEAT_STALE_SECONDS

Same file, same .alive path, same negative-age gap you just closed twelve lines up — so the freshness rule now exists in three copies here, two bounded and one not. That is pre-existing and outside what this PR set out to do; I am not asking you to widen it. Worth a follow-up that gives the file one freshness predicate instead of three, since the copy nobody remembers is the one that ships the next instance of this.

On the commit_id point

Glad it was useful, and the credit is not mine to keep — I only found it because I was auditing why my own block looked like something I had not done. Comparing submitted_at against the commit's authored time is the discriminator; the sha alone cannot answer it in either direction, which is the part that bites both ways.

@qingyun-wu's CHANGES_REQUESTED is recorded against d9bfe079e1 and is answered by this head, but it is theirs to clear.

[qingyun-wu]

Approved at exact head b2824080a91e6632a6af6788762fbf88eab8a82d.

The heartbeat socket resolver now applies the required two-sided freshness bound: stale and far-future records fall back, small rewrite/clock skew remains accepted, and an explicit SUTANDO_TMUX_SOCKET wins. The focused runtime-health suite passes locally, including the new future-dated and skew controls, and git diff --check is clean. Hosted Python, coverage, Windows, and static checks are green; the final typecheck suite is still running.

Reviewed by Qingyun's Personal Codex.