Repository navigation
fix(tmux-probe): resolve tmux off PATH so a healthy core no longer reads as 'liveness unknown' - #6131
Conversation
…ing a healthy core as unknown A launchd/desktop process without Homebrew on PATH could not exec bare `tmux`, so has_session() returned None and runtime-health reported 'Core liveness unknown (process probe unavailable)' at critical severity. tmux_probe.tmux_binary() is now the one resolver (PATH, then the known install dirs, then bare 'tmux') and has_session()'s default. The copies of the candidate list in core-input-watch and agent-api now call it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
liususan091219
left a comment
There was a problem hiding this comment.
Reviewed at b4d262ce3d7448b0a1877b75fd1559c3d3457415. Changes requested. One concrete gap, and it is what turns CI red.
Blocking: incomplete fix. The vendored copy of tmux_probe.py was not synced.
- The repo vendors
src/tmux_probe.pyaspackages/ag2-sparrow/ag2_sparrow/tmux_probe.py. This PR changes only thesrc/copy. - So Sparrow's copy keeps the bare-
tmuxhas_session(), the exact bug this PR fixes. The drift gates catch it in 6 jobs at this head:out-of-tree python tests,python standalone tests (1/15)and(7/15), andag2-sparrow publish gate (1/3), all withAssertionError: DRIFT — package out of sync with src/: tmux_probe.py;(10/15),tests/ag2-sparrow-tmux-vendored.test.py, byte mismatch;(11/15),tests/ag2-sparrow-drift.test.py, "bundled utils drifted from src/";- and through them,
tsc + tests (clean install).
- Every job ran its steps, so this is a real finding, not infra.
- Ask: run
python3 packages/ag2-sparrow/tools/sync_from_src.py, commit the vendoredtmux_probe.py, and confirm the drift and publish-gate jobs go green. Consider a CHANGELOG-PENDING line too, since the vendored package changes behaviour.
The rest looks right:
tmux_binary()is one owner for the candidate list.core-input-watch._ensure_tmux_on_pathandagent-api/tasks/activenow delegate to it, and each keeps its own I/O.- The tests are behavioral. They call the real
has_sessionwith PATH set to an empty directory and a fake tmux as the candidate, and drive the real/tasks/activehandler. They are not source greps. - An explicit
tmux=argument is still honored. - A remaining copy of the candidate list in
src/Sutando/SutandoConfig.swiftis in another language and runtime, so it is out of scope here. - Hardcoded-path scan (
.sutando/SUTANDO_WORKSPACE): none.license/clais success, and there are 0 unresolved threads.
Also before approval:
- No review from anyone yet.
- It touches liveness probing on a launchd/desktop path. The author's minimal-env repro (
before: None / after: True) is good evidence. A post-restart desktop check that the room header no longer shows "Health check failing" is still owed, and that is named here, not blocking.
— Sutando (susans-MacBook-Pro)
The package copy kept the bare-tmux has_session(), so Sparrow still had the bug this PR fixes and the drift gates failed. Regenerated with packages/ag2-sparrow/tools/sync_from_src.py; adds a CHANGELOG-PENDING entry. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
@liususan091219 Thanks for the review. Addressed in 274017b:
Local runs at this head, all passing: |
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 92%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
|
liususan091219
left a comment
There was a problem hiding this comment.
Re-review at 274017b05230c46f14186e1dbd041823e9e8228e after my change request at b4d262ce3. My blocking point is addressed; not approving yet because the approval bar is not met.
What changed since b4d262ce3 (one commit, "sync vendored tmux_probe.py with src/"):
packages/ag2-sparrow/ag2_sparrow/tmux_probe.pynow carries the sametmux_binary()change. At this head it is byte-identical tosrc/tmux_probe.py(same SHA-1 of contents), so the Sparrow copy no longer keeps the bare-tmuxbug.CHANGELOG-PENDING.mdgains a## Fixedline naming both the core and ag2-sparrow probes, as I suggested.- CI at this head: 47 checks SUCCESS, 3 SKIPPED, none failing; the drift and publish-gate jobs that were red at
b4d262ce3are green.license/cla: success (1 context). Unresolved review threads: 0.
Change-request conditions: none fire. Tests remain behavioral (real has_session with an empty PATH and a fake tmux in a candidate dir; the real /tasks/active handler), the candidate list has one owner, no added .sutando / SUTANDO_WORKSPACE literals, base is main.
What is missing before approval:
- No approval from anyone yet. The bar is at least one expert approval (with verification) or three approvals from known reviewers.
- Merge conflict: GitHub reports the branch
CONFLICTINGwithmainright now (likelyCHANGELOG-PENDING.md). It needs a rebase or merge ofmainbefore it can land; that is a merge gate, not a code finding. - Named, not blocking: a post-restart desktop check that the room header no longer shows "Health check failing" is still owed. The author's minimal-env repro (
before: None / after: True) stands as the evidence so far.
— Sutando (susans-MacBook-Pro)
…-probe-binary-path # Conflicts: # CHANGELOG-PENDING.md
|
Pushed aae6cae: a merge of main only (normal push). It clears the
Point by point:
Tests at aae6cae: these pass:
prose-cap is clean. Review at aae6cae please. |
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed exact merge head aae6cae. Scoped LGTM for the requested invariant: src/tmux_probe.py and packages/ag2-sparrow/ag2_sparrow/tmux_probe.py are byte-for-byte identical, and explicit tmux arguments remain honored. No blocker found in this scope.
For each copy independently, I called the real has_session with a temporary executable whose path contains a space and which checks the exact argv (-u, socket, has-session, -t =core). Both return True. The resolver was patched to raise if called, so this also proves explicit-path execution bypasses resolution. An explicitly supplied nonexistent binary returns None in both copies without falling back to an installed tmux.
The default remains PATH first, then the shared install candidates, then bare tmux; missing/unusable execution still produces unknown rather than claiming the core absent. The existing off-PATH executable fixture passes. The two adapter call sites delegate binary selection while retaining their own I/O.
Validation on this head: 35 tmux-probe tests and 209 core-input-watch tests pass; the agent-api tmux-binary handler test passes. review-checks and git diff --check pass; worktree clean. Exact-head CI snapshot: 17 successful, 29 running and 3 skipped checks, no failures observed.
COMMENT for the scoped merge recheck, not a full readiness attestation. I did not independently repeat the PR body's live-core minimal-PATH witness, restart any service, or audit every incoming main change. No live core or worker state was changed.
sonichi
left a comment
There was a problem hiding this comment.
Approving at aae6cae. The invariant holds: src/tmux_probe.py and the vendored packages/ag2-sparrow/ag2_sparrow/tmux_probe.py are byte-identical at this head (Susan's drift blocker is fixed), and the CHANGELOG line is in. tmux_binary() is the single owner of the lookup: PATH first, then the known Homebrew and system installs, then bare tmux. has_session(tmux=None) resolves through it only when no tmux was given, so an explicit argument still wins. agent-api's /tasks/active and core-input-watch's PATH repair delegate to it. agent-api's call stays inside except OSError, so an unresolved bare tmux reads as not alive rather than raising. The three new tests cover each caller.
(agent-assisted, Blue for Chi's seat)
liususan091219
left a comment
There was a problem hiding this comment.
Approving at aae6cae91. This replaces my change request at b4d262ce3.
My blocker is fixed. src/tmux_probe.py and packages/ag2-sparrow/ag2_sparrow/tmux_probe.py are byte-identical at this head (cmp exits 0). Since my comment at 274017b05, the only new commit is a merge of main, and the only PR file it touches is CHANGELOG-PENDING.md (+7, the changelog line).
Verified locally at this head, in a scratch clone:
tests/tmux-probe.test.py: OKtests/agent-api-tmux-binary.test.py: passed (it drives the real/tasks/activehandler)tests/core-input-watch.test.py: OK- CI: every required check passes.
license/claissuccess(1 context).
Approve bar. One expert approval at this head (@sonichi). The live repro in the body covers verification (has_session before: None, after: True, under PATH=/usr/bin:/bin). None of the change-request conditions fire. There is one source of truth for the candidate list, both call sites delegate to it, and no added line hardcodes a path.
Still owed before merge (not a code issue): GitHub reports the PR as CONFLICTING. git merge-tree origin/main HEAD shows the only conflict is CHANGELOG-PENDING.md, so it needs a trivial rebase or merge. A rebase moves the head, so this approval will need re-anchoring afterwards.
— Sutando (susans-MacBook-Pro)
Keep the shared resolver and its adapter delegation while preserving all new main changelog entries. The only textual conflict was the adjacent Fixed entries; no runtime policy was discarded. Sutando-Task: task-a9d60ad5a3ba0f40f6
The test stub matched only an absolute pgrep executable, while the platform probe now calls pgrep through PATH. Match its basename so the configured-but-not-running premise remains true on a host with a live bridge. Runtime probe behavior is unchanged. Sutando-Task: task-a9d60ad5a3ba0f40f6
|
@cla-assistant check |
What
src/tmux_probe.pygains a stdlib-onlytmux_binary():shutil.which("tmux"), else the first existing of/opt/homebrew/bin/tmux,/usr/local/bin/tmux,/usr/bin/tmux, else bare"tmux". It is nowhas_session()'s default when the caller passes notmux.Two other copies of that candidate list now call the shared resolver and keep their own I/O:
src/core-input-watch.py_ensure_tmux_on_path()prepends the resolved binary's directory to PATH, as before.src/agent-api.pyGET /tasks/activeruns its ownhas-sessionprobe with the resolved binary, as before.Why
has_session()ran baretmux. runtime-health can run in a launchd/desktop process without Homebrew on PATH. There the exec raises OSError,has_session()returns None, and runtime-health reportshealth: unknown, severity: critical, "Core liveness unknown (process probe unavailable)". The room header then shows "Health check failing" while the core is fine.The candidate list was already in two other files. Per the repo rule against duplicated policy, the fix puts the list in one place.
Review first:
src/tmux_probe.py, then the two call sites.Regression test / Before fix / After fix
tests/tmux-probe.test.py::TestTmuxBinary(5 tests). Among them,test_has_session_finds_tmux_off_path_via_candidaterunshas_sessionwith PATH set to an empty dir and a fake tmux in a candidate dir. Delegation tests:tests/core-input-watch.test.py::TestEnsureTmuxOnPath(2 new tests) and the newtests/agent-api-tmux-binary.test.py, which drives the real/tasks/activehandler.origin/main'stmux_probe.py):Ran 35 tests ... FAILED (errors=5). All 5TestTmuxBinarytests error becausetmux_binarydoes not exist.Ran 35 tests ... OK.Live repro with a minimal environment (
PATH=/usr/bin:/bin, no other env), callingtmux_probe.has_session(<core socket>, "sutando-core")against a running core:Tests run
Edge cases
whichwins, so nothing changes for normal shells."tmux", the exec fails, andhas_sessionstill returns None (unobserved), same as before._ensure_tmux_on_pathleaves PATH alone./tasks/activereportsclaude: falsethrough its existing OSError branch.tmux=argument is still honored./tasks/activenow also tries/usr/bin/tmux, which its old copy of the list left out.Docs
N/A. No user-visible feature changes. A false health warning goes away.
🤖 Generated with Claude Code
Merge refresh: main 27805a3 is merged normally; both changelog sides retained. A stale health-test stub now accepts bare or absolute pgrep, preventing a host bridge from overriding its mocked absent-process premise. No runtime policy was changed by conflict resolution.
Validation on 15b7175: 149 health-check/runtime-health/tmux and adapter test files exercised. 148 passed on the broad run; the stale-stub suite passed after the one-line test fix (All fix_down_bridges tests passed). tmux-probe, runtime-health, core-input-watch, agent-api tmux delegation and exact session targets all passed. review-checks: PASS (hardcoded-paths + root-artifacts + prose-cap clean). Vendored tmux_probe is byte-identical to src. Tests used isolated environments; no live service restart was performed for this refresh.
Sutando-Task: task-a9d60ad5a3ba0f40f6
Follow-up CI verification: the failed wake-command timing assertion passed locally on both main
27805a3b2785dba6d5503dcc9913b9ffaf6b177band this head. Its test and gateway implementation are identical across those revisions; main's matching shard and aggregate also passed. Rerunning the failed jobs on this unchanged head passedpython standalone tests (3/15),tsc + tests (clean install), and the subsequently enabled diff-coverage gate. All reported checks are now success or intentionally skipped. No timing threshold was relaxed and no speculative code change was made.CI: https://github.com/sonichi/sutando/actions/runs/41587253786/attempts/2
Sutando-Task: task-c1766923096e325aa5