Repository navigation
Use host Claude Code throughout the retained ACP integration - #1082
Conversation
PR #1082: Use host Claude Code throughout the retained ACP integration28 files, +3888 / -2819 Scope🔴 PR has 6707 lines changed. Split into focused PRs. 🟡 6707 lines changed. PRs under 400 receive more thorough review. 🟡 28 files changed. Are all changes related? 🟡 Changes span 9 directories. Structural🟡 2 console statements. Oxlint structural signals:
Slop✅ Slop indicators look low. Static AnalysisOxlint: 51 diagnostics across 8 files (11 rules) consistent-function-scoping (16): packages/cli/src/plan-writer-profile.ts, packages/acp/src/provider.ts CorrectnessNo extraneous code patterns detected. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| yield* stat(candidate); | ||
| return candidate; | ||
| } catch { | ||
| // A missing PATH entry does not hide a later launcher. |
There was a problem hiding this comment.
Redundant comment — restates what the code does.
| // A missing PATH entry does not hide a later launcher. |
taras
left a comment
taras
left a comment
There was a problem hiding this comment.
Independent review by Claude Fable (claude-fable-5, high effort), published by the coordinator.
Verdict: PASS on 287c5b738da7a03abc108f0c36cadd78c1e79eac. All nine recorded acceptance clauses were examined; no blocking findings.
287c5b7 changes only three test/fixture paths and, with its parents, satisfies all nine clauses. Host selection (node-claude-executable.ts) resolves CLAUDE_CODE_EXECUTABLE-or-PATH claude, refuses actionably on missing/unlaunchable with no SDK fallback, and treats --version as diagnostic (join(), not expect(), so nonzero exit and unfamiliar output do not gate). The env flows only through the pre-existing #566 child-only agentProcessEnv option into all four profiles; HC tests prove no global process.env mutation and no host path in durable records/journal/checkpoints. Claude materialization omits optional platform executables while HC5 proves SDK library/required deps survive; digest-keyed caches leave existing materializations untouched. ACPv1, adapter 0.70.0/SDK 0.3.238, acceptance/checkpoint contracts and the approved agentSessionId amendment are retained; native allocation stays client-allocated. The provider election remains race-safe: no suspension between the held-partition read and electRuntime reuse. The disposal witness passively observes the unchanged SDK launcher; dispose() joins startingQueries, backend and helper closure (sessions is a plain object, so the emptiness check is real). CI green recorded for this exact head; first failures preserved. No blocking finding.
Nonblocking observations:
-
packages/acp/tests/adapter-protocol.test.ts:159 (onStdout kept attached through close) — the permissive-answer branch writes to processChild.stdin after teardown has already ended it; if an adapter ever issued a client request after EOF, the write-after-end stream error has no listener and would crash the test process. Not reachable with the current adapter (dispose sends no requests; all 23 cases green on three runtimes), but guarding the write with stdin.writableEnded would remove the latent flake window.
-
packages/acp/tests/adapter-protocol.test.ts:95 — for provider "claude" teardown only ends stdin and waits for close with no kill escalation, so an adapter regression that stops exiting on EOF hangs the case until the runner timeout instead of failing crisply. Deliberate (SIGTERM preempts the EOF disposal under test) and acceptable; a bounded fallback kill after a grace period would keep the evidence while failing faster.
-
packages/acp/src/provider.ts:1309 — the electRuntime "released during preparation" throw is unreachable: options is undefined only when the held entry was read in the same synchronous step, so the re-read cannot miss. Harmless defensive guard; could be dropped or asserted differently. (Assessed existing public observation: the comment at packages/runtime/node-executable-path.ts:27 states why all stat errors — including EACCES — are swallowed during the PATH scan; it conveys a real constraint under code rule 4, so keeping it is acceptable and no correction is warranted.)
The completed original response is preserved. Its summary exceeded the local wrapper’s length limit by 99 characters; the coordinator reconciled that formatting failure without another model prompt or changing the verdict/findings.
Why
Claude reviews can select the Agent SDK's older bundled executable even when the host Claude Code supports the requested model. This affects workflow reviews as well as ordinary agent use. Fixes #1080.
What changes
Before: the retained Claude adapter can launch its SDK executable, and fresh materialization installs platform executable packages.
After: the embedded Claude integration selects the explicit nonempty
CLAUDE_CODE_EXECUTABLEor resolvesclaudethrough the host PATH. Missing, invalid or unlaunchable selections refuse before a model prompt, with no bundled fallback. Fresh materialization keeps the SDK library and required dependencies while omitting its optional platform executables.How it works
Host adapters resolve executable selection against the execution directory and pass it through the existing child-only environment seam to the matching probe and live runtime. Existing custom-agent commands retain their selection; native-bound selection remains authoritative.
The required ACP-only workflow path also needs a canonical Claude conversation assertion. The retained ACP v1 snapshot now obtains its existing
agentSessionIdmetadata from that Query's successful early backend SessionStart confirmation, using a fixed no-op settings hook. Missing, conflicting or mismatched confirmation refuses. This is the explicitly approved identity amendment; confirmation remains separate from first-turn acceptance, asserting save, mapping and disclosure.Review guide
Start with the host-selection and materialization changes in
packages/acp/src/adapter-snapshots.ts, then the host adapters and provider runtime ownership. Inspect the Claude source patch and provenance inpackages/acp/vendor/adapters/PROVENANCE.mdbefore the generated snapshot bytes.Look carefully at exact-history confirmation, pending-Query cleanup, preservation of user hooks/settings, native client allocation, runtime selection ownership, and completed offline replay.
What must stay true
How to verify it
Focused implementation evidence passes for the production workflow route with distinct host/SDK executables, refusal without fallback, ordinary run/Plan/REPL assemblies, native selection precedence, fresh package omission, protocol identity and lifecycle cases, retained mapping/checkpoints, and offline replay. A local real-host Claude journey through the changed adapter and production ACPX establishes history, joins shutdown, then reopens that history in a new adapter; its model service is local and makes no external model calls.
Vendor/generated-byte checks and the npm distribution witness pass. The new portable assembly boundary also passes under Node. Earlier failed composite commands and their corrections are preserved; passing scenarios are attributed to their actual candidate revisions.
Delivery verification: CI run 2365 succeeds on exact head
287c5b738da7a03abc108f0c36cadd78c1e79eac: all 55 applicable jobs pass, with the expected composability skip. This includes the Linux held-startup cancellation and backend/helper disposal regressions. All 57 exact-head checks were read completely.Claude Fable’s independent review, using
claude-fable-5at high effort, passes all nine recorded acceptance clauses with no blockers and three nonblocking observations. The original Planner also approves exact287c5b7. The Architect’s feature approval remains attributed tob6388a9; subsequent corrections contain fixture type annotations, generated formatting and test/fixture changes.deno task verify:cleanpassed onb6388a9, proving isolated preparation, target preparation, offline builds, dependency fingerprints, resolution and concurrent interference. Later corrections preserve production build/dependency logic and decoded archive bytes, so that ownership proof remains applicable; it is not relabelled as a run on the child commit. Original CI failures, the pre-prompt model-catalog refusal and the review-wrapper summary-length refusal remain preserved with their exact diagnoses and corrections. No automatic review retries or additional measurement runs were used.Generated or mechanical changes
The Claude archive and
generated/snapshots.tsrepresent patched source54df80cbb41f2aa56195fd56b4dc2a88295049af; provenance includes its patch, inventory and build method. Codex snapshot bytes and root dependency files are unchanged. No test corpus file, runtime exclusion or shard runner is added or removed.Risks and limitations
Backend lifecycle confirmation must arrive from the owning Query before ACP creation or reattachment returns. Disabled lifecycle hooks or missing confirmation refuse; they never infer identity from a request or transport ID. Existing materialized sessions are not rewritten.
Scope confirmation