Repository navigation
fix(restart): find the bundled node and fail loudly when claude is missing (#6117) - #6222
Conversation
…ssing (#6117) health-check's out-of-process restart env added <engine>/runtime/bin, but the desktop app ships node at <engine>/runtime/node/bin, so a launchd-run restart could find neither node nor claude. The launch PATH now has one owner, src/agent/launch-path.sh, used by both runtime launchers and by _resolve_launch_env. The claude launcher now checks that `claude` resolves on the PATH its tmux pane will actually run under before creating the session or healing the window, and says so on stderr and in restart-attempts.log instead of the generic "core did not come up within ~5s". Sutando-Task: 20261104-004 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
sonichi
left a comment
There was a problem hiding this comment.
Changes requested at 90289c9.
The design is sound. launch-path.sh is the right owner. The pane-PATH check is correct: it checks the session env, then the global env, then $PATH before the server exists. Both new tests fail on main and pass here. Two things block:
-
CI is red because of this PR.
src/health-check.py:14478-14488:_engine_launch_path_dirsruns the helper throughsubprocess.run.tests/health-check-codex-task-notifier.test.py:404patcheshc.subprocess.runto count launcher calls, sofix_codex_task_notifiernow sees 2 calls instead of 1. The failure reproduces locally at this head and passes with main'shealth-check.py. That same mock shows a second problem: the stub's stdout (sutando-core already running) was parsed as a PATH dir. Please:- patch
_engine_launch_path_dirsin that test, or ignore thelaunch-path.shcall there; - keep only absolute-path lines from the helper's output.
- patch
-
One copy of the policy is left behind.
skills/worker-pool/scripts/launch-codex-worker-session.sh:22-29still has its own loop over$REPO/../runtime/node/bin,~/.npm-global/binand~/.local/bin. That is the same policylaunch-path.shnow owns. Per CLAUDE.md, the duplicate is the defect, so please source the helper there too. Related:skills/worker-pool/scripts/launch-worker-session.shgets the newclaude_cli_resolvescheck throughlaunch_claude_session, but never appends the launch path. A launchd-spawned Claude worker now fails loudly, but it still cannot find the bundled node or~/.local/bin.
Non-blocking:
- Heal-path refusal is untested.
src/agent/claude/cli/start-cli.sh:585-588(claude_cli_resolvesthenexit 127) has no test of its own; the immediate-exit test only gained aclaudestub so the branch passes. A case with noclaudeon the heal branch would pin the exit code and the stderr line. - PATH order differs.
src/health-check.py:14474prepends the bundled node dir; the launchers append it. Before this PR, the dir health-check prepended never existed on a desktop install, so this is new behavior: on a desktop install whose user has their own node (for example nvm), a restart through health-check runs the bundled node first, while a direct launch runs the user's. If that is intended, say so in the comment; otherwise make them match. - A running tmux server with a minimal PATH. You already list this as not fixed. Passing
-e PATH=...onnew-session/new-windowwould close it; a follow-up is fine.
Checked and fine: the desktop layout (<engine>/sutando/../runtime/node/bin), the older runtime/bin layout, a missing dir, paths containing spaces, bash 3.2, and set -euo pipefail in the codex launcher. The restart-attempts.log message now gives the specific reason, and the codex CODEX_NODE_BIN override still works.
(agent-assisted, Blue for Chi's seat)
…aunch-path owner for workers too (#6117) - launch-path.sh appends the caller's override dirs before its defaults, so CODEX_NODE_BIN beats the bundled node and CODEX_INSTALL_BIN beats ~/.local/bin/codex; with no override, the codex order is the one main had. - health-check builds the engine dirs in Python (no subprocess; a mocked subprocess.run counted it as a second launcher call) and appends them, as the launchers do. tests/launch-path.test.sh pins the Python list to the helper. - Both worker-pool launchers take their PATH from launch-path.sh. - Tests: both override collisions on the real codex launcher, the claude heal path with no claude (exit 127 + named cause); fixtures copy launch-path.sh. Sutando-Task: 20261104-004 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 92%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
|
|
@sonichi thanks. All of it is addressed at ac40ac7, and CI is green at that head (46 pass, 1 skipped). Blocking:
Non-blocking, both done:
Also folded in, from Codex verification: The running-tmux-server-with-minimal-PATH case is still out of scope (follow-up). Live launchd restart on a desktop install is not yet verified. |
|
Exact-head follow-up at
Focused evidence is otherwise positive: notifier 18/18, launch-path 5/5, recovery 28/28, restart verification 11/11, worker bootstrap 9/9, notifier wiring 9/9, Codex PATH-order 2/2, and Codex-worker fail-open 14/14; review checks and the hardcoded-path positive control discriminate. Structured readiness remains unknown and required aggregate/diff-coverage gates were still pending. I attempted to record this as a formal (agent-assisted, Sutando for Kewei seat) |
sonichi
left a comment
There was a problem hiding this comment.
Approving at ac40ac7.
Both blocking items from 90289c9 are fixed:
src/health-check.py:14477_engine_launch_path_dirsno longer runs a subprocess, sotests/health-check-codex-task-notifier.test.pypasses again and no helper stdout can end up on PATH. The Python copy of the dir list is pinned tolaunch-path.shby the new case intests/launch-path.test.sh. That is a fair trade for dropping the subprocess.skills/worker-pool/scripts/launch-codex-worker-session.sh:22-24now sourceslaunch-path.sh, with its four dirs passed as overrides in their old order.launch-worker-session.sh:41-43now appends the launch path for Claude workers.
The non-blocking notes are addressed too:
- The heal-path refusal has its own test (
case_heal_without_claude_exits_127), and it fails against main's launcher. - PATH order is now the same everywhere: the inherited PATH first, then explicit overrides, then the shared defaults. health-check appends like the launchers (homebrew dirs still go in front). The two new codex-core-launcher ordering tests fail with the 90289c9 helper, where
~/.local/bin/codexshadowedCODEX_INSTALL_BIN.
All changed and added test files pass in an isolated env. review-checks passes, and CI is green.
(agent-assisted, Blue for Chi's seat)
keweichen
left a comment
There was a problem hiding this comment.
Changes requested at exact head ac40ac72f47eabdfb78afe7821cd43b9db811fdd. This head is not ready to merge.
Two independent blockers remain:
src/health-check.py:14477-14482repeats the three default launch directories owned bysrc/agent/launch-path.sh:7-16. The parity test detects drift but leaves two policy implementations. Please keep one dependency-light executable or data owner and have both callers delegate to it.- The PR still has no real post-restart input-to-result round trip through the affected launchd/startup path. REVIEW lesson 15 and CONTRIBUTING require that exact-head live witness before approval; green harness and CI results do not substitute for it.
The prior notifier, worker-launch, override-order, and heal-path findings are fixed, and current-head CI is green. The verdict changes when the directory defaults have one owner and the real exact-head post-restart witness is attached.
(agent-assisted, Sutando for Kewei seat)
…d overrides win end to end (#6117) - health-check's restart env no longer injects the bundled node, runtime/bin or ~/.local/bin; it keeps only homebrew for its own tmux calls. A default injected there sat ahead of CODEX_NODE_BIN / CODEX_INSTALL_BIN, because the launcher keeps the incoming PATH first. - The dispatcher (src/agent/start-cli.sh) heals its PATH from launch-path.sh before its tmux guard, and a second call (the runtime launcher's) drops the defaults the first appended so its overrides go ahead of them. - The Python mirror of the dir list and its parity test are gone. - Tests: the real health-check -> dispatcher -> codex launcher chain keeps both overrides first (fails at ac40ac7); a launchd-minimal restart through health-check's env still finds a ~/.local/bin claude; dispatcher fixtures copy launch-path.sh. Sutando-Task: 20261104-004 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
keweichen
left a comment
There was a problem hiding this comment.
Changes requested at exact head 72cd818f544600821faf0f71823771cc20246767; this head is not merge-ready.
The duplicated launch-directory policy is fixed: src/agent/launch-path.sh is now the one owner and health-check no longer mirrors the defaults. Three blockers remain.
-
PR-caused CI regression. The new
spec_from_file_location(...health-check.py)imports in both modified test files cross the credential-isolation guard withouthermetic_env.hermetic-env-structuralis 26/28 at this head versus 28/28 atac40ac72; CI shard 2/15 fails on these two cases, the required aggregate is red, and diff coverage is skipped. Please use the repository hermetic import fixture. (Shard 3/15 separately hit a pr-watch tempdir cleanup race; I am not attributing that one to this diff.) -
The canonical startup chain still defeats
CODEX_NODE_BIN.src/startup.sh:99prepends the bundled node before it execs the dispatcher.sutando_append_launch_pathremoves only dirs recorded in_SUTANDO_LAUNCH_PATH_ADDED, so that startup-owned default survives as incoming PATH and stays ahead of the explicit override. The exact-head startup-chain repro resolves node to<engine>/runtime/node/bin/nodeand prints the bundled marker, with the override directory later on PATH. The new health-check→dispatcher test does not traversestartup.sh. Please preserve explicit override precedence through the full startup chain and add that control. -
The mandatory live witness remains absent. The body still says no real launchd-run restart was performed. REVIEW lesson 15 and CONTRIBUTING require an exact-head post-restart input-to-result/reply round trip through the affected desktop path.
Focused positive suites otherwise pass 81/81, the new chain control discriminates ac40ac72, and review checks pass. The verdict changes when the startup override is truly end-to-end, the exact-head required gates are green, and the real post-restart witness is attached.
(agent-assisted, Sutando for keweichen's seat)
… ~/.local/bin tmux itself (#6117) - launch-path.sh records only the default dirs it appended, so a nested call (dispatcher, or a worker under a core) drops just those and re-places its overrides ahead of them; CODEX_NODE_BIN / CODEX_INSTALL_BIN dirs inherited from a parent are never stripped. - health-check resolves its own tmux binary (homebrew, then ~/.local/bin, then PATH) instead of relying on ~/.local/bin in the launch env, which stays free of default dirs. - The health-check -> launcher chain tests move to an isolated file (tests/health-check-restart-path-chain.test.py, isolate_os_environ first) so the launcher suites no longer import health-check and stay within the hermetic-environment guard. - Tests: worker --print-env under a core keeps both override dirs ahead of the defaults (fails at 72cd818); a fake tmux only in ~/.local/bin is used by _run_tmux (fails at 72cd818). Sutando-Task: 20261104-004 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
sonichi
left a comment
There was a problem hiding this comment.
Re-approve at 17529d4.
- launch-path.sh: overrides are now appended in their own loop and are never recorded in
_SUTANDO_LAUNCH_PATH_ADDED. A nested launch strips only the defaults an earlier call added, so an override set by a parent core survives the child's call. That also narrows the edge case I raised last round to the default dirs only. The new launch-path.test.sh cases pin it. - health-check
_resolve_tmux_binnow also finds~/.local/bin/tmux. That's needed, since the launch env no longer adds~/.local/binfor health-check's own tmux probes. The new health-check-restart-path-chain test covers the restart chain end to end.
keweichen
left a comment
There was a problem hiding this comment.
Changes requested at exact head 17529d4870b698b3593580800067430b6edfd22f.
The hermetic-import regression is fixed: the isolated health-check tests now satisfy the structural guard, local hermetic coverage is 28/28, and the current Python CI shards pass. The nested-worker override retention and tmux lookup changes also look sound.
One code blocker remains in the real activated path. src/startup.sh still prepends the bundled node before execing the dispatcher, but src/agent/launch-path.sh removes only entries recorded in _SUTANDO_LAUNCH_PATH_ADDED. That startup-owned prepend is unrecorded, so the later explicit CODEX_NODE_BIN directory is appended behind it. An exact-head startup-chain control still resolves node to the bundled binary and prints the bundled marker instead of the explicit override. The new health-check-to-dispatcher test passes because it never traverses startup.sh.
The mandatory live-path gate also remains open: the PR body still says no live launchd restart was performed or observed, and no exact-head post-restart input-to-result/reply witness appears in the thread.
Worst-case disruption: an install that intentionally selects a different Node runtime can silently keep executing the bundled runtime after restart, so recovery can report healthy while running the wrong binary. Fix the full startup-chain precedence, add a test that actually traverses startup.sh, and attach the real exact-head post-restart round-trip witness. At that point this is ready for another review.
Evidence: narrow local suites passed 64/64 (hermetic 28, restart-chain 2, launch-path 6, recovery 28); review checks passed; required aggregate tests, diff coverage, and CLA are green. Structured readiness receipts remain unknown, so no receipt-backed readiness claim is made.
…NODE_BIN (#6117) src/startup.sh prepended the app node dir (and SUTANDO_NODE's dir) as plain incoming PATH, so launch-path.sh kept it ahead of the codex launcher's CODEX_NODE_BIN. It now goes through sutando_prepend_default_dir: still first for startup's own services, but recorded as a launch default, so the launcher moves it behind the overrides (and still keeps it, last, when the repo does not name that dir itself). Tests: startup.sh's own node step, then the dispatcher and codex core, keeps CODEX_NODE_BIN ahead of the bundled node (fails at 17529d4); launch-path covers a prepended default going behind an override. Sutando-Task: 20261104-004 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
sonichi
left a comment
There was a problem hiding this comment.
Re-approve at c4aa30b.
- startup.sh still puts the app-bundled node dir (and
SUTANDO_NODE's dir) first for its own services, but now records it in_SUTANDO_LAUNCH_PATH_ADDEDthroughsutando_prepend_default_dir. So when the core launcher exec'd at the end callssutando_append_launch_path, that dir is stripped like any default,CODEX_NODE_BINand the other overrides go ahead of it, and the dir goes back last instead of disappearing (thedroppedre-add). Before, startup's unrecorded prepend beatCODEX_NODE_BIN. - Re-added dropped dirs are recorded in
addedagain, so a further nested launch handles them the same way. The new launch-path.test.sh case (paths with spaces) and the restart-path-chain test pin the order.
keweichen
left a comment
There was a problem hiding this comment.
Exact-head follow-up at c4aa30b45204103628d0d442cd34b6dd58946db7.
The startup-chain code blocker from my prior review is fixed. startup.sh now records both startup-inserted Node directories through the shared launch-path owner, and the dispatcher/Codex chain moves only those marked defaults behind explicit CODEX_NODE_BIN / CODEX_INSTALL_BIN overrides. The retained exact-head tests discriminate the prior head: the startup-chain case fails there with the bundled directory at index 0 and the override at index 4, while this head passes. I also verified the launch-path cases (including spaces, pre-existing PATH, repeated calls, and a nested Codex worker), the hermetic-env structural suite, and the review checks. Exact-head CI is complete and green apart from the expected Windows skip.
The worst-case disruption is still a service that appears recovered in harnesses but cannot boot or deliver through the shipped launchd path. This PR is therefore not ready to merge yet under REVIEW.md §15: the PR body still explicitly says no real launchd post-restart input→result/reply round trip was run, and the thread has no such exact-head witness.
My existing CHANGES_REQUESTED remains active solely for that live witness. Once a real exact-head post-restart round trip is attached, I can re-review; I found no remaining code-correctness blocker.
|
Live restart witness at c4aa30b (all times UTC, 2026-11-04). The owner ran it on a second desktop install, a Mac running a separate Sutando core. Running revision
Restart
Message round trip after the restart
Restored afterwards: checkout back on main (e20a381), rebuilt at 15:09:46Z. Caveat for reviewers. This exercises the heal path (defect B's |
…ex (#6117) A launchd-run start-cli.sh --force-restart at c4aa30b on a desktop install failed with "claude not found on PATH": the desktop ships claude only in <app support>/cli/claude/bin, which launch-path.sh never added. Append <app support>/cli/claude/bin and cli/codex/bin (repo-relative, like the bundled node dir), ahead of ~/.local/bin. Existing-dir and append-only rules are unchanged, so a caller's own PATH still wins. Sutando-Task: task-s_dspace~task-5b394fea027ab9984e Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Round 6 (a92f98a): a gap found by the launchd witness. The launchd-run
On that install Fix: New case in The live launchd witness will be re-run at this head. |
sonichi
left a comment
There was a problem hiding this comment.
Re-approve at a92f98a. The only change since c4aa30b is two more default dirs in sutando_launch_path_dirs, plus a test.
$1/../../cli/claude/binand$1/../../cli/codex/binare the desktop app's bundledclaudeandcodex(under the app-support dir, two levels above the engine checkout), the only copy on a desktop install. They go ahead of~/.local/binin the default list.- They're defaults like the others: appended only if the directory exists (
sutando_append_launch_pathchecks-dand normalizes withcd && pwd), after the incoming PATH and any override, so a user's ownclaudeorcodexstill wins. They're recorded in_SUTANDO_LAUNCH_PATH_ADDEDthe same way, so nested launches treat them like any other default. tests/launch-path.test.shpins the new order.
|
Launchd / minimal-PATH witness at a92f98a: PASS (UTC, 2026-11-04). Run on a second desktop install that has a separate Sutando core. Running revision The launchd job
The restart
Input to delivered reply after the restart
Afterwards: the one-shot job is no longer loaded (LaunchOnlyOnce). The install is being switched back to main and rebuilt next; that step is not part of this witness. |
john-the-dev
left a comment
There was a problem hiding this comment.
Codex-core independent review: recommend approval, at exact head a92f98ae8f31cf5be40cae3549a8d4a67dd6ef15. No remaining source blocker found in this review.
The supplied operator witness in #6222 (comment) binds Git HEAD and ENGINE_MANIFEST to this head. It reports a one-shot launchd job with the shipped minimal PATH, force-restart exit 0 and a fresh core at 16:58:29Z using desktop Claude and bundled Node. It contrasts the actual c4 exit-1 failure. The subsequent input at 17:03:01Z maps to its task/result at 17:03:43Z and delivered reply at 17:03:44Z (43 seconds). This satisfies my previously outstanding exact-head launchd/reply witness requirement as supplied operator evidence; I did not independently operate or inspect that desktop.
Retained exact-source review verifies all 24 cumulative blobs. Eight helper checks, three registered chains and 12 independent PATH controls passed, plus six parent controls; the same helper tests with actual c4 source fail exactly the desktop-directory regression. These immutable tests were not rerun for this receipt review. Fresh direct GitHub reads confirm all 15 required contexts successful, 45 successful check runs/one expected skip and successful CLA. The source head/base remain unchanged.
Worst-case disruption is selecting the wrong CLI/runtime or failing to recover a core. Defaults are existing-directory-only and retain incoming PATH/explicit override precedence, with desktop defaults now ahead of the home fallback. Full-chain source controls and the supplied launchd witness mitigate this risk; an existing tmux server PATH and untested host layouts remain disclosed limits. No universal desktop/Codex runtime acceptance or full-suite baseline certification is claimed.
GitHub currently reports CHANGES_REQUESTED/BLOCKED; the separate reviewer’s standing block is not cleared by this recommendation. The authenticated account is also the PR author, so this is a COMMENT review: GitHub does not permit a formal self-approval, and this review contributes no approving vote. An eligible distinct reviewer must supply any formal approval. No merge or runtime operation performed.
(agent-assisted, Codex-core review)
…6117) The round-6 dirs were derived only from <repo>/../.., so a foreign checkout (self-development) that names its desktop root with SUTANDO_APP_SUPPORT still could not resolve the app-managed claude/codex under a launchd PATH. launch-path.sh now owns the desktop-root rule (env first, else the parent of the repo's engine/ dir) and start-cli.sh uses it instead of its own copy. Tests: a foreign checkout with SUTANDO_APP_SUPPORT in launch-path.test.sh, and a real-launcher --force-restart chain case with claude only in that root's cli/claude/bin; both fail on the previous head. Sutando-Task: task-s_dspace~task-3a0eb16a70f124f9c2 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Round 7 (bc0adda): keweichen's blocker, fixed. The round-6 desktop CLI dirs were derived only from
The live launchd witness was taken at a92f98a on a canonical install. For that layout this change gives the same dirs, because the env-named root and |
liususan091219
left a comment
There was a problem hiding this comment.
Reviewed at bc0adda. Not ready to approve yet; no code blocker found in the round-7 delta.
Round 7 (a92f98a → bc0adda) reads correctly:
src/agent/launch-path.shsutando_desktop_app_supportbecomes the single owner of the desktop-root rule ($SUTANDO_APP_SUPPORT, else the parent of<repo>'sengine/), andsrc/agent/start-cli.sh:108now calls it instead of its own copy. The helper is sourced atstart-cli.sh:16, before that use. One rule, one owner, which is the right shape.- The new cases discriminate:
tests/launch-path.test.sh(foreign checkout +SUTANDO_APP_SUPPORT, minimal PATH) andtests/health-check-restart-path-chain.test.py(real-launcher--force-restartwithclaudeonly in the env-named root) both fail at a92f98a per the round-7 comment. The test file now unsets a hostSUTANDO_APP_SUPPORT, so fixtures stay hermetic. - No hardcoded install/workspace path in the added lines; dirs stay existing-only and append-only, so incoming PATH and explicit overrides still win.
What is missing before approval (approve bar not met at this head):
- No approval at the current head. @sonichi's latest approval is at a92f98a; bc0adda changed
launch-path.shandstart-cli.shafter it. - @keweichen's CHANGES_REQUESTED at 17529d4 is still their standing verdict. Their later COMMENT at c4aa30b said the only remaining item was an exact-head launchd post-restart round trip. @john-the-dev's witness at a92f98a (launchd one-shot job, minimal PATH,
--force-restartexit 0, reply delivered 43 s after input) answers that; a re-review from @keweichen is needed to clear the block. - CI is still queued at bc0adda (license/cla is green). Approval should follow green required checks.
Named, not blocking: the live launchd witness was taken at a92f98a on a canonical install, not at bc0adda. For that layout the env-named root and <repo>/../.. resolve to the same path, so the same dirs apply, but the foreign-checkout path itself has harness coverage only. A tmux server already running with a minimal global PATH remains a disclosed limit.
— Sutando (susans-MacBook-Pro)
sonichi
left a comment
There was a problem hiding this comment.
Re-approve at bc0adda. The change since a92f98a makes the desktop CLI dirs honor SUTANDO_APP_SUPPORT and removes a duplicated copy of that logic.
sutando_desktop_app_support <repo>in launch-path.sh is now the one place that finds the desktop root:$SUTANDO_APP_SUPPORTif set, else the parent ofengine/when the repo sits at<root>/engine/<repo>, and nothing outside a desktop install or when the directory is missing.start-cli.shcalls it instead of carrying its own copy of the same derivation, so the policy now lives in one place.- The default dirs are built from that root:
<root>/engine/runtime/node/bin,<root>/cli/claude/binand<root>/cli/codex/bin, then~/.local/bin, so an app-support override moves all of them together. They are still appended last, existence-checked, and recorded as defaults, so a user's ownclaudeorcodexstill wins. - Outside a desktop install the two
cli/*dirs are no longer added (before, they were probed relative to the repo regardless). That matches the intent, and the tests pin both the desktop and the override cases.
sonichi
left a comment
There was a problem hiding this comment.
Re-approve at 4069df9. The change since bc0adda swaps dirname and basename in sutando_desktop_app_support for pure Bash, so early boot works before PATH holds them. I ran it rather than reading it.
- Same answers where it matters. I ran the old and new function side by side over: the desktop layout, with one and two trailing slashes, a path with a space in it, a checkout directly under
engine/, and a non-engine checkout. Every one gives the same output. The tests (tests/launch-path.test.sh) pass, including the new ones for a trailing slash and a bare name. - It needs no external commands. With a PATH that contains nothing, the function runs and returns 0.
- Two degenerate inputs change, in the safer direction.
/engine/sutando(a root-level checkout) and a relativeengine/sutandoused to print/and., which would have made the filesystem root or the current directory the app-support root. They now print nothing, meaning "not a desktop install". Callers pass an absolute resolved repo path, so neither occurs in practice. - The
[ ... ] && d=...line sits inside anif, and the function ends in an explicitreturn 0, so it stays safe underset -e.
sutando_desktop_app_support called external dirname/basename, which early boot (start-cli.sh before PATH is known-good) may not have: the cli dirs were silently dropped. Parameter expansion keeps the env-first rule and needs no tools. Regression: env unset, canonical <root>/engine/sutando, PATH without dirname or basename; plus trailing-slash, bare-name and non-engine controls. Both fail at bc0adda and pass here. Sutando-Task: task-s_dspace~task-a67ceb909b642c1cc4 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
4069df9 to
60a783e
Compare
|
@cla-assistant check |
keweichen
left a comment
There was a problem hiding this comment.
Approved at exact head 60a783ebbc6248846f2752de0794267da5e609b5.
Round 8 resolves the early-boot PATH blocker: the desktop-root fallback is pure Bash, the 11-case launch-path suite and restart-chain checks pass, and the retained round-8 tests fail against the prior helper exactly where expected. All activated callers provide an absolute canonical repository path; the disclosed relative-input lexical difference is therefore not activated.
The amended commit carries the required Sutando-Task trailer and has the same tree as the reviewed 4069df9 head. Exact-head CI is terminal with 45 successful checks, one expected Windows skip, successful CLA, and all 15 required contexts green. I found no remaining code or repository-contract blocker.
The canonical decision/evidence projections still report unknown, so this approval does not claim a structured readiness receipt.
The launcher and six test fixtures gained launch-path.sh on main (sonichi#6222) at the lines where this branch adds core-workspace-log.sh; both stay. The monitor-log test now stubs claude on PATH, since the launcher refuses a core without it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Closes part of #6117 (defects A and B). C and D are not in this PR; see "Not in this PR".
Written by Sutando (Claude Code) for Rui, who is accountable for it.
Problem
After a scheduled restart the Claude core did not come back, and the engine's own restart paths could not recover it. A launchd-run
start-cli.sh --force-restartfailed three times. Each time it logged onlycore did not come up within ~5s, and the only stderr hint wassession hooks: node unavailable. Under launchd's PATH, neithernodenorclauderesolved.Root cause
src/health-check.py_resolve_launch_env()(main, ~line 14474) addedREPO_DIR.parent / "runtime" / "bin". The desktop app ships node at<engine>/runtime/node/bin. On a desktop install,engine/runtimeholdsnode/,python/andservices/, and has nobin/. So the out-of-process restart env never contained the bundled node.The same directory was also hard-coded separately in
src/agent/codex/cli/start-cli.sh(CODEX_NODE_BIN, ~line 140). The Claude launcher added nothing at all, so a launchd job running it directly kept the minimal PATH.src/agent/claude/cli/session-launch.shlaunch_claude_session()(~line 401) and the heal path insrc/agent/claude/cli/start-cli.sh(~line 578) start a bareclaudein tmux without checking that it resolves. tmux accepts the command, the pane dies at once, and the 5 s liveness poll reports the generic failure. Only the no-tmux branch checked (start-cli.sh~line 651).Change
New
src/agent/launch-path.shis the single owner of the engine-owned launch dirs:<engine>/runtime/node/bin, the older<engine>/runtime/bin, the desktop app's<app support>/cli/claude/binandcli/codex/bin, and~/.local/bin.sutando_append_launch_pathappends the ones that exist and are not already on PATH. It only appends, so a caller's own node or CLI keeps winning.The Claude launcher sources it and appends at startup.
The Codex launcher uses it in place of its private copy, passing
CODEX_NODE_BIN(or the bundled node) andCODEX_INSTALL_BINas overrides._resolve_launch_env()no longer adds any engine dir (bundled node,runtime/binor~/.local/bin). It keeps only the homebrew dirs, for its own tmux calls. Anything injected there would sit ahead of the launcher's overrides. Nowlaunch-path.shis the only owner. The dispatcher (src/agent/start-cli.sh) adds those dirs to PATH itself, before its tmux guard. When the runtime launcher then calls the helper again, it drops the defaults the first call appended, so its overrides go ahead of them.Overrides beat defaults.
sutando_append_launch_pathappends the caller's override dirs before its own defaults. SoCODEX_NODE_BINbeats the bundled node, andCODEX_INSTALL_BINbeats~/.local/bin/codex. With no override set, the codex order is the same as on main.Worker-pool launchers.
launch-codex-worker-session.shtakes its PATH from the helper instead of its own loop, andlaunch-worker-session.shnow appends it too.claude_cli_resolves(session-launch.sh) checksclaudeagainst the PATH the pane will actually run under. tmux gives a pane the session's or the server's global environment, never the launcher shell's, so the check readstmux show-environmentand falls back to$PATH. It runs:new-sessioninlaunch_claude_session, which covers the fresh-start path, the restart path and pool workers;new-windowon the heal path, which exits 127.A failure now prints
claude not found on PATH=...on stderr and writesFAILED: claude not found on PATH=...torestart-attempts.log, instead ofcore did not come up within ~5s.Fixture tests that copy the launcher's sibling scripts now also copy
launch-path.sh.shutdown-sentinel-immediate-exit-controlsgets aclaudestub, because its heal branch now correctly refuses with noclaudeon PATH.shutdown-sentinel-restored-by-real-launcheruses a tempHOME, because the launcher now reads~/.local/binand the real one leaked a hosttmuxinto its no-tmux sandbox.docs/src-map.mdis regenerated for the new module.Files to look at first:
src/agent/launch-path.sh, thensrc/agent/claude/cli/session-launch.sh(claude_pane_path,claude_cli_resolves), then the_resolve_launch_envhunk.Relation to #5672
#5672 (open, CHANGES_REQUESTED, DIRTY) touches the same files. It replaces the bare
claudeinlaunch_claude_sessionwith"${SUTANDO_CORE_CLI_BIN:-claude}"and adds a runtime-compat gate in the launchers. It does not add a resolve check insidelaunch_claude_sessionor on the heal path, and it does not touch_resolve_launch_env's node dir, so it does not cover A or B.The two will conflict textually in
launch_claude_session. When both land,claude_cli_resolvesshould test${SUTANDO_CORE_CLI_BIN:-claude}instead ofclaude.Tests
Round 5 (c4aa30b):
src/startup.shnow adds the app node dir and theSUTANDO_NODEdir throughsutando_prepend_default_dir.CODEX_NODE_BIN.CODEX_NODE_BINstays ahead of the bundled node. It fails at 17529d4.startup.shpass, andhermetic-env-structuralpasses.Round 4 (17529d4):
launch-path.shrecords only the default dirs it appended. So a later call (the dispatcher, or a worker started under a core) drops only those, and never drops override dirs it inherited.--print-envunder a core keeps both override dirs ahead of the defaults. It fails at 72cd818.~/.local/bin, then PATH. The env it passes to the launcher still carries no default dirs.health-check-recover-corecase o3): a fake tmux that exists only in~/.local/binis used by_run_tmux. It fails at 72cd818.tests/health-check-restart-path-chain.test.py, which callsisolate_os_environfirst.hermetic-env-structuralpasses (28 tests).pr_watch.test.py(temp-dir cleanup) is not touched by this PR.Round 3 (72cd818):
CODEX_NODE_BINandCODEX_INSTALL_BINstay first. It fails at ac40ac7 with'local' != 'install'.claudethat exists only in~/.local/bin. This check fails if the launcher's own PATH step is removed.codex-core-launcher(102 tests) andstart-cli-model-pinstill show only the failures that main shows on this host.Round 2 (ac40ac7). These pass at ac40ac7:
health-check-codex-task-notifier, which was the CI failure: a mockedsubprocess.runcounted the helper call as a second launcher call. Python no longer shells out, and the test is unchanged.'local' != 'install'.launch-path.test.sh5/5, including the Python and shell parity check. That check fails when the Python list is altered.spawn-worker-resume,spawn-worker-launcher,pool-remedy,spawn-worker-runtime-and-sentinel,codex-worker-auto-reset-launcher,launch-codex-worker-session-failopen(14/14),launch-worker-session-absolute-paths,launch-worker-session-forwards-worker-id,worker-mode-is-gated-on-env,start-cli-worker-bootstrap.shutdown-sentinel-cleared-by-real-launcher,core-restart-heartbeat-and-cron-hold,start-cli-claude-task-notifier, plus every suite listed below.codex-core-launcherandstart-cli-model-pinstill show only the failures that main shows on this host.Round 1 (90289c9):
Before (origin/main 704a42f, new test case run against main's launcher):
That reproduces the #6117 symptoms exactly.
After (this head):
The other launcher and health-check suites pass at this head:
Two suites fail on this host, and fail identically on origin/main, so the failures are not from this change:
codex-core-launcher.test.py: the same 7 failures. The sorted FAIL lists from main and this head diff to empty.start-cli-model-pin.test.py: the same 3 failures (default, env-ignored, pinned-attach).shellcheck -S warningis clean on all changed shell files.bash scripts/review-checks.sh --diff <(git diff origin/main...HEAD)passes (hardcoded-paths, root-artifacts, prose-cap).python3 scripts/gen-src-map.py --checkreports the map is up to date.Not verified
engine/runtime. It has not been observed in a run.claude. This PR makes that failure say so; it does not change the pane's PATH.claudelocations itself: the desktop app's<app support>/cli/claude/bin(andcli/codex/bin), then~/.local/bin. These are defaults: when both exist, the desktop's copy comes first, but an entry already on the incoming PATH and an explicit override (CODEX_NODE_BIN,CODEX_INSTALL_BIN) still win. The homebrew dirs stay in health-check's env only.Not in this PR
app-node-dirdefault inscripts/sutando-config.shstill namesspace.ag2.app. fix(paths): engine data root follows the desktop data-folder move (space.ag2.desktop) #5888 is moving the data root tospace.ag2.desktop. This PR's repo-relative lookup does not depend on that default.Docs: N/A. Internal launch behaviour only; no user-visible capability changes.
🤖 Generated with Claude Code