Repository navigation
fix(specialists): one claude CLI on PATH, and specialists run only as CLI subprocesses - #1786
Conversation
a1ae6be to
317c8db
Compare
842fa75 to
09e73ee
Compare
PR #1786 -- fix(specialists): one claude CLI on PATH, and specialists run only as CLI subprocessesWhat it does: GEAK's pip bootstrap installed Claude Code into Blocking issues: none Checked: Ran: nothing locally -- SKIPPED: test execution. This reviewer is on Windows with no WSL and the suite imports LGTM |
xiaofei-zheng
left a comment
There was a problem hiding this comment.
No blocking issues. Review detail and the paths checked are in the comment above.
Two installers placed Claude Code and three resolvers picked between the copies in different orders. GEAK's pip bootstrap installs it into ~/.local/bin, off PATH, and runs before the kernel installer, whose Forge resolver then counted that copy as installed and skipped its own npm install. TopCI containers have since started with no claude on PATH. With HYPERLOOM_CLAUDE_CODE_VERSION set, npm put the pinned copy in /usr/local while GEAK_CLAUDE_BIN still named GEAK's copy, so the pin reached nobody. ensure_claude_cli now runs before ensure_geak. It puts ~/.local/bin first on PATH, reuses the claude PATH then resolves, and otherwise installs with Claude Code's native installer into ~/.local/bin, the same command GEAK's bootstrap runs, so no Node.js is needed. A version pin reinstalls that version, which then wins on PATH. The install fails when it cannot provide claude instead of warning and continuing into a tool-less run. GEAK's bootstrap then finds that claude and only steps in when it is older than GEAK's minimum, installing into the same directory. GEAK_CLAUDE_BIN names the claude on PATH, and the ~/.local/bin entry is always written into kernel-agent.env.sh for every shell that sources it. Co-Authored-By: Claude <noreply@anthropic.com>
…egrading Subprocess dispatch that resolved no claude CLI logged a warning and fell back to the in-process backend, which has no source roots, no permission mode and no PR Monitor tools, so every specialist task came back empty while the session reported success. When neither HYPERLOOM_CLAUDE_CLI_PATH, GEAK_CLAUDE_BIN nor PATH yields one, startup now raises, as a missing codex runtime already does. Co-Authored-By: Claude <noreply@anthropic.com>
Specialists ran either as a CLI subprocess in a per-task worktree or, with --specialist-dispatch-mode inprocess, through a Claude or Codex Agent SDK backend inside the orchestrator, which gives them no source roots, no permission mode and no PR Monitor tools. Production only ever used the subprocess, and with the silent fallback gone nothing reached the in-process path except an explicit flag no launcher passes. The flag and --specialist-per-turn-max-seconds, which only bounded in-process calls, are removed, along with the in-process branch of the specialist executor and CodexAgentBackend, whose only caller it was. ClaudeBackend's attribution_component and attribution_operation fields, which only that branch overrode, give way to the orchestration labels they otherwise always carried. The executor tests that drove a mocked in-process backend now run the real subprocess path against a fake claude CLI shared through conftest. Co-Authored-By: Claude <noreply@anthropic.com>
With the in-process dispatch gone from the CLI, SpecialistRunner's backend_factory path only served tests: it drove a mocked backend turn by turn through a loop production never ran, so those tests did not cover the subprocess path that ships. The runner now takes a subprocess config and nothing else, and the turn loop goes with the helpers only it used: the partial-done writer, the per-turn failure trace and the partial path. The prompt's emit_intent exit, which only the in-process loop could honour, is dropped; every specialist is told to write specialist_done.json. A missing specialist_done is reported as no_specialist_done_emitted rather than max_turns_exhausted, which counted in-process turns and misread the subprocess's single turn. Runner tests that used a mocked backend now run against the fake claude CLI. Co-Authored-By: Claude <noreply@anthropic.com>
09e73ee to
5f169ab
Compare
xiaofei-zheng
left a comment
There was a problem hiding this comment.
Re-reviewed at head 5f169ab after the rebase onto 87bf19e. Conclusion unchanged: no blocking issues.
What moved: main relocated the installer (src/hyperloom/agents/kernel/scripts/install.sh -> src/hyperloom/inference_optimizer/assets/install_kernel_tools.sh) and its two test files into inference_optimizer/tests/. This PR's own 22-file change is byte-identical across the rebase -- I split both PR diffs per file, normalised the renamed paths and compared: same file set, same hunk bodies, with one difference inside a line this PR deletes (INSTALL_SH.parents[4] -> parents[3]), which is main's depth change to code the PR removes. Per-file line counts are unchanged (+639 / -1722).
Re-checked at the new head rather than assumed: ensure_claude_cli still sits immediately before ensure_geak in main() and _ANTHROPIC_KEY_VAL is still assigned at top level (install_kernel_tools.sh:364), so the credential write still sees it; set -euo pipefail is still line 14, so the curl | bash failure still reaches die; _claude_path_dir writer at :1457 against its reader at :1299. Both moved tests resolve the new installer path (test_install_geak_pip.py:15, test_install_home_paths.py:21), so test_claude_cli_is_on_path_before_geak_installs and the _extract_func-driven cases are reading the file they are meant to. The operator-facing message in cli/executors.py:118 was updated to the new path, and git grep finds no remaining reference to the old one anywhere in the tree. _incontainer.sh.in:142's "install.sh does not provide node/npm" still refers to assets/install.sh, the script the carrier actually runs at line 50, so it is still accurate.
CI at 5f169ab: ruff, pylint, mypy, bandit, gitleaks, reuse, byte-compile, wheel/pyproject and the docs build are green; the test shards and CodeQL were still running when I looked.
The section carried over the Unreleased entries verbatim, and those missed changes that ship in 1.1.4 and that an operator upgrading from 1.1.3 will hit: - the single `hyperloom <command>` entry point and the old entry points, flags and critic subcommands it removed (#1736); - the removal of in-process specialist dispatch and its two flags, and the installer owning the one claude CLI (#1786); - kernelforge no longer reading its historical workspace formats, and the `--op-name` alias and old entry-point group it dropped (#1768); - four of the variables #1739 made fail on a malformed value were missing from the strict-parsing list: INFERENCE_OPTIMIZER_CYCLE_RELOOP_MIN_REMAINING_SEC, HYPERLOOM_GEMM_PAIRED_PAIRS, MAX_MODEL_LEN and GPU_MEMORY_UTILIZATION. Co-Authored-By: Claude <noreply@anthropic.com>
* chore(release): cut 1.1.4 Every site that tracks the packaged version moves together: pyproject's [project].version, the docs version_number kept in sync with it, the README badge, the compatibility matrix row, and the three copy-paste `pip install hyperloom-inference-optimizer==` pins, which would otherwise keep installing 1.1.3 after 1.1.4 ships. HYPERLOOM_WHEEL_TAG moves with it, which requires the v1.1.4 GitHub release to carry the rocm-profiler-hotfix-libs.tar.gz asset the bare-metal installer downloads from that tag. The Unreleased section of the release notes becomes the 1.1.4 section, and an empty Unreleased heading is kept for the next cycle. Co-Authored-By: Claude <noreply@anthropic.com> * docs(release-notes): complete the 1.1.4 section The section carried over the Unreleased entries verbatim, and those missed changes that ship in 1.1.4 and that an operator upgrading from 1.1.3 will hit: - the single `hyperloom <command>` entry point and the old entry points, flags and critic subcommands it removed (#1736); - the removal of in-process specialist dispatch and its two flags, and the installer owning the one claude CLI (#1786); - kernelforge no longer reading its historical workspace formats, and the `--op-name` alias and old entry-point group it dropped (#1768); - four of the variables #1739 made fail on a malformed value were missing from the strict-parsing list: INFERENCE_OPTIMIZER_CYCLE_RELOOP_MIN_REMAINING_SEC, HYPERLOOM_GEMM_PAIRED_PAIRS, MAX_MODEL_LEN and GPU_MEMORY_UTILIZATION. Co-Authored-By: Claude <noreply@anthropic.com> * docs(release-notes): cover every operator-visible change in 1.1.4 The 1.1.4 section listed six of the 98 operator-visible changes merged between v1.1.3 and the release branch. Add a "Before upgrading from 1.1.3" section for the removed options, module paths and environment variables, the moved defaults, the installation prerequisites and the persisted-state changes, and add Added, Changed, Removed and Fixed entries for the rest. 1.1.4 is no longer described as a patch release. The fixes for the AgentX agentic-recipe path (#1696, #1749) are left out: the path itself is removed in the same release (#1784). Allow the release notes to name the retired kernel_agents.agent_providers entry-point group and KERNEL_AGENTS_MAX_TURNS in the rename-completeness test, as they already may name the retired kernel-agents console script. Co-Authored-By: Claude <noreply@anthropic.com> * docs(release-notes): correct six 1.1.4 entries against the code - GLM-5.3 corpus: the old corpus stays usable through AGENTX_CANONICAL_DATASET; pinning it with AGENTX_DATASET stamps every result submission_valid=false. - Workload knobs: state that --extra-env pins are now exported under their own names, that a shell PRECISION still overrides a resumed session, and that 1.1.3 already read MAX_MODEL_LEN. - Resume still type-checks crash_count / crash_timestamps; drop that claim. - optimization_journal.json outcomes: name the literals it writes (KEEP, REVERT, no_promote, recorded). - Roofline: the profile runs once and logs one "roofline profile failed" warning. Also quote the real UPDATE_STATE refusal, note that the MLPerf backend measures Kimi-K3 only, scope the warm-replay flag removal to the flag, and write placeholders as <name> like the rest of the page. Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: Zeng <lzeng@amd.com> Co-authored-by: Claude <noreply@anthropic.com>
~/.local/bin, off PATH, and runs before the kernel installer, whose Forge resolver then counted that copy as installed and skipped its own npm install.GEAK_CLAUDE_BINwas written as the first of~/.local/bin/claude,/usr/local/bin/claude, PATH; Forge resolves PATH first; fix(specialists): spawn the Claude CLI the installer recorded #1775 made Hyperloom's agents readGEAK_CLAUDE_BIN. TopCI containers have started without aclaudeon PATH since 2026-09-24 (CLI(s) not on PATH: ['node', 'claude', 'codex']on every run), and withHYPERLOOM_CLAUDE_CODE_VERSIONset, npm put the pinned copy in/usr/localwhileGEAK_CLAUDE_BINstill named GEAK's copy, so the pin reached nobody. fix(specialists): spawn the Claude CLI the installer recorded #1775 already stops the specialists falling back to the in-process backend on TopCI (the 2026-10-09 06:21 UTC DeepSeek-V4.1-Flash run dispatched them as subprocesses), so this PR removes the second copy and the silent fallback that remains when no CLI resolves at all.This PR makes the installer own the one claude CLI every consumer runs:
ensure_claude_cli(renamed fromensure_forge_claude_cli) runs beforeensure_geak. It puts~/.local/binfirst on PATH, reuses theclaudePATH then resolves, and otherwise installs with Claude Code's native installer (curl -fsSL https://claude.ai/install.sh | bash -s <target>) into~/.local/bin— the same command GEAK's bootstrap runs, so no Node.js is needed.~/.local/bingoes first on PATH even when it was already on PATH further back, soHYPERLOOM_CLAUDE_CODE_VERSIONreinstalls that version and it then wins on PATH. The install fails when curl is missing or the installer fails, instead of warning and continuing into a tool-less run. The embedded Python decision, its retry loop and thevalidate_runtimecall go away;ClaudeBackend.validate_runtimestays where Forge runs it.ensure_claude_cli: it finds that claude and only steps in when it is older than GEAK's minimum (2.1.177), installing into the same~/.local/bin, which is first on PATH. GEAK keeps ownership of its own floor, and no second copy lands off PATH.GEAK_CLAUDE_BINiscommand -v claude, and the~/.local/binPATH entry is written intokernel-agent.env.sh, so GEAK, the specialists, Forge and every sourcing shell resolve the same file.HYPERLOOM_CLAUDE_CLI_PATH,GEAK_CLAUDE_BINnor PATH yields a claude CLI, as a missing codex runtime already does, instead of falling back to the in-process backend, which has no source roots, permission mode or PR Monitor tools.With that fallback gone, the in-process specialist dispatch had no caller but an explicit
--specialist-dispatch-mode inprocess, which no launcher passes (no TopCI job log since 2026-09-01 and no script on the shared NFS sets it). It is removed, so every specialist runs as a CLI subprocess in its own worktree:--specialist-dispatch-modeand--specialist-per-turn-max-seconds(which only bounded in-process calls) are gone, with the in-process branch of the specialist executor andCodexAgentBackend, its only consumer.ClaudeBackend'sattribution_component/attribution_operationfields, which only that branch overrode, give way to theorchestrationlabels they otherwise always carried.SpecialistRunnertakes a subprocess config and nothing else. Itsbackend_factoryturn loop, which only tests drove, goes with the helpers only it used (partial-done writer, per-turn failure trace, partial path).emit_intentexit, which only the in-process loop honoured, is dropped: every specialist is told to writespecialist_done.json.specialist_doneis reported asno_specialist_done_emitted;max_turns_exhaustedcompared in-process turns and misread the subprocess's single turn whenever a task asked for one turn. Both classify as the same no-output failure.resolve_claude_executable).test_env_file_points_geak_and_later_shells_at_the_claude_on_pathnow puts a second copy in~/.local/binbeside the claude on PATH; against origin/main's installer it fails (GEAK_CLAUDE_BINnames the~/.localcopy) and it passes here.test_install_home_paths.pydrives the realensure_claude_cliagainst a sandboxed PATH, HOME and a stand-in for the native installer: fresh install into~/.local/binon PATH, reuse of a copy already there, reuse of a claude on PATH, the version pin installing ahead of the image's claude, failure without curl or when the installer fails,--check-only, and credentials under$HOME. The Forge-validation cases go with the mechanism they described; the contract they shared, that the CLI and credentials are ensured whatever kernel backend is selected, holds by construction sinceensure_claude_clireads no backend setting andmain()calls it unconditionally.test_install_geak_pip.pyasserts thatensure_claude_cliruns beforeensure_geak.test_version_pin_wins_when_the_home_bin_dir_is_already_on_path_behind_the_image_claudecovers a~/.local/binalready on PATH behind another claude: before the fix the pinned copy was installed but PATH and the env file still resolved the image's claude.test_cli_executors_wiring.pyasserts the startup refusal and keeps fix(specialists): spawn the Claude CLI the installer recorded #1775's recorded-CLI case. The executor, lifecycle and runner tests that drove a mocked in-process backend now run the real subprocess path against a fakeclaudeCLI (moved fromtest_specialist_subprocess.pyinto the packageconftest.py, plus ano_donebehaviour), so they cover the path that ships: adapter round trip, crash →stale, clean exit without a done →no_specialist_done_emitted, patch vetting off the event loop, dispatcher bookkeeping. Tests of the removed mechanisms are deleted: the in-process Codex/Claude backend selection, the runner's dispatch-exclusivity check, the partial-done writer, and the attribution-field guard (the call-site guard now seesClaudeBackend's literal labels). On Linux, 235 test files covering the kernel installer, the slurm template, the CLI parser and executors, the specialist runner, dispatcher and prompts, and the Codex/Claude roles: origin/main 8749 passed / 12 failed / 1 error, this branch 8722 passed / 12 failed / 1 error, with identical failure sets (TraceLens, critic-backend and loop-runner environment failures on the runner); the 27 fewer passes are the removed cases above less the one added.ruff checkandruff format --checkclean.CodexAgentBackend, the runner's turn loop and itsemit_intentexit), andClaudeBackend's attribution override fields. Preserved contracts: the version pin still reinstalls, credentials still land in$HOME/.claude/config.json, and--check-onlystill only warns.claudeis on PATH (~/.local/bin/claudeunless the image already ships one), soCLI(s) not on PATHno longer lists claude andGEAK_CLAUDE_BINinkernel-agent.env.shnames the same file; the install log showsinstalling the claude CLI (latest) into …/.local/binon images without one, where GEAK's bootstrap used to install its own copy off PATH.HYPERLOOM_CLAUDE_CODE_VERSIONnow takes the native installer's targets (stable,latestor an exactX.Y.Z) rather than an npm version spec. An install that cannot provide claude exits with an error instead of a warning, and a session whose specialists find no claude CLI stops at startup instead of running every specialist without tools.--specialist-dispatch-modeand--specialist-per-turn-max-secondsare no longer accepted. The specialist prompt's output section names only thespecialist_done.jsonexit, and a specialist that ends without one is recorded with reasonno_specialist_done_emitted. LLM spend labels are unchanged: orchestration turns stayorchestration/orchestrate_turnand specialists stayspecialist. The installer no longer installs Node.js: the codex carrier in_incontainer.shneeds the image to provide node/npm. Since 2026-09-24 it already did not get them on images where GEAK had installed claude; it lost them only where GEAK's install also failed.--specialist-dispatch-modeor--specialist-per-turn-max-secondsnow fail argument parsing (no TopCI launcher passes them). Hosts that cannot reachclaude.ai/downloads.claude.aior lack curl now fail the install instead of continuing without a claude CLI (those runs had no working specialists), and anHYPERLOOM_CLAUDE_CODE_VERSIONwritten as an npm range is rejected by the native installer.🤖 Generated with Claude Code