feat(desktop/windows): coding-agent ACP adapter core (PR 1/4)#9304
feat(desktop/windows): coding-agent ACP adapter core (PR 1/4)#9304karthikyeluripati wants to merge 5 commits into
Conversation
…enClaw, Hermes, Codex)
PR 1/4 of the Windows coding-agents train: a self-contained ACP (Agent
Client Protocol) JSON-RPC-over-stdio adapter layer under
src/main/codingAgent/, ported from the macOS agent runtime
(desktop/macos/agent/src/adapters). Nothing is wired into chat, IPC, or
UI yet — that lands in the next PRs of the stack.
- interface.ts: RuntimeAdapter contract, capability matrix for the four
Windows adapters, binding/attempt types, id-conflation contract guards.
- acp.ts: the generic ACP client (JSON-RPC framing, session lifecycle,
streaming session/update translation, no-progress watchdog, permission
auto-resolution). Windows adaptations vs macOS: Windows env allowlist
(ComSpec/SystemRoot/PATHEXT are required for shell:true spawns and npm
.cmd shims), taskkill /t /f tree-kill in stop() (POSIX process-group
kill does not exist on Windows and a plain kill would orphan the real
agent behind cmd.exe), windowsHide, and ELECTRON_RUN_AS_NODE for the
bundled default adapter.
- claudeCode.ts: default always-available adapter — spawns the bundled
patched-acp-entry.mjs (wrapping @zed-industries/claude-agent-acp, a
pure cross-platform npm package) via a ?asset import; no separately
installed CLI required.
- openclaw.ts / hermes.ts / codex.ts: external-command adapters
activated by OMI_{OPENCLAW,HERMES,CODEX}_ADAPTER_COMMAND or (later)
settings-stored commands, with per-adapter env passthrough
(HERMES_HOME; OPENAI_API_KEY/CODEX_* for the Codex ACP bridge).
- adapterRegistry.ts: static profiles + activation checks with
preference-over-env command resolution for the future settings UI.
- toolPolicyStub.ts: minimal permission policy — auto-approve for the
trusted first-party Claude Code bridge, never a permanent grant for
untrusted external adapters (allow_once or ACP -32001).
- electron-builder.yml: asarUnpack for the entry script and the packages
it imports (a spawned Node child cannot read from the asar archive).
- vitest.config.ts: resolve electron-vite `?asset` imports to the real
source path so main-process modules stay testable.
Verification: cd desktop/windows && pnpm run typecheck (clean),
pnpm run lint (clean), pnpm test — 577 passed / 3 skipped, including
the new mocked-subprocess suites (spawn shapes, env-allowlist secret
non-leakage, taskkill tree-kill, watchdog cancellation, stderr
redaction, permission policy) and acp.integration.test.ts, which spawns
a real Node fake-ACP peer (fixtures/fakeAcpSubprocess.mjs) and runs the
full open → prompt → stream → result → stop lifecycle unmocked.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 23 files
Confidence score: 5/5
- Safe to merge after the addressed issues were fixed.
You’re at about 94% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…n the adapter core Fixes 11 of the 12 automated-review findings (the asarUnpack one is narrowed rather than removed — electron-builder flattens resolved deps into the package, so the transitive entries are real; the scope-wide @agentclientprotocol/** glob is now the concrete sdk package): - failures.ts: broaden stderr redaction (token/secret/password/credential fields, GitHub ghp_/github_pat_, Slack xox*) and keep the TAIL of long diagnostics so final error lines (and the OpenClaw config sniff strings) survive truncation. New failures.test.ts pins all of it. - acp.ts: gate session/update by the attempt's adapter session id so one process hosting several sessions can never stream into the wrong attempt (updates without a session id still accepted); make the no-watchdog path abort-aware so a cancelled attempt settles even when the adapter never answers session/cancel; strip user:pass@ userinfo from proxy values that URL() can't expose an authority for (e.g. "alice:pass@proxy:3128", where "alice:" parses as a scheme). - patched-acp-entry.mjs: dynamic-import the bridge AFTER the console→ stderr redirect (a static import is hoisted and could pollute stdout). Re-verified live against the real bridge: initialize handshake completes and stdout stays pure JSON-RPC. - electron-builder.yml: narrow asarUnpack to the concrete runtime deps. - tests: assert CLAUDE_CODE_USE_VERTEX deletion and windowsHide on the default spawn; assert adapterSessionId in the integration test; real session/new-level assertion that OpenClaw strips MCP servers and skips set_model; cross-session isolation and no-watchdog cancellation tests; watchdog test context uses the adapter id under test. Verification: pnpm run typecheck (clean), pnpm run lint (clean), pnpm test — 583 passed / 3 skipped; live smoke of the real bridge entry (initialize handshake, no LLM call) passed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
All reported issues were addressed across 9 files (changes from recent commits).
You’re at about 95% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…rt early return Follow-up to the PR BasedHardware#9304 review: in the no-watchdog cancellation path, an already-aborted attempt returned before attaching handlers to the underlying session/prompt promise, so a later rejection (e.g. process exit) would surface as an unhandled rejection. Attach the handlers first — finish() guards double-settling — then take the early-abort exit. Regression test covers the pre-aborted + late-rejection sequence. Verification: pnpm run typecheck (clean), pnpm test — 584 passed / 3 skipped. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Thanks for the careful Windows adapter-core port here. The shape is coherent for PR 1/4: it keeps the ACP runtime isolated under I’m not approving this automatically because it is a stacked feature PR with dependency/runtime-agent changes and a large new adapter surface; it needs human maintainer review before merge. A couple of maintainer-review items I’d like checked before this lands:
No urgent security issue found in the static review; this is mainly dependency/runtime-agent policy review rather than a request to split the PR. |
Git-on-my-level
added
needs-maintainer-review
…nt-acp public API Addresses the maintainer dependency review on PR BasedHardware#9304: the bundled Claude bridge was @zed-industries/claude-agent-acp@0.18.0 (deprecated, renamed upstream) imported via an internal dist/acp-agent.js path, with monkey-patches on package internals (ClaudeAcpAgent.prototype, the sessions map, query.next) to capture cost/usage. All three fragile boundaries are gone: - Dependency is now @agentclientprotocol/claude-agent-acp@^0.58.1 (the maintained rename; the old name has no post-rename releases). - The entry (renamed patched-acp-entry.mjs → claude-acp-entry.mjs) imports runAcp from the package's PUBLIC export and contains no patches at all — just the console→stderr redirect and runAcp(). - Cost is captured from the bridge's standard usage_update session notifications (documented ACP surface): acp.ts tracks the cumulative session cost per adapter session and reports the per-attempt delta; a per-turn _meta.costUsd on the prompt response still wins when no usage_update was seen (other ACP adapters). electron-builder asarUnpack updated to the renamed package plus its actual transitive runtime imports (@agentclientprotocol/sdk, @anthropic-ai/claude-agent-sdk + its win32-x64 native package). pnpm-workspace.yaml records the supply-chain exclusions pnpm required for these fresh versions. Verification: pnpm run typecheck (clean), pnpm run lint (clean), pnpm test — 585 passed (new cumulative-cost-delta unit test; fixture + integration test now exercise the usage_update path and its precedence over _meta). Live against the real renamed bridge on Windows 11: initialize handshake with pure JSON-RPC stdout, and a full end-to-end task (real file created, cost $0.24 captured via usage_update). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Verified the maintainer-flagged packaged path by spawning the bridge entry from a real electron-builder build (dist/win-unpacked): the ?asset entry and the unpacked packages resolved, but the handshake failed with ERR_MODULE_NOT_FOUND — @agentclientprotocol/sdk imports zod at runtime and zod sat inside the asar, unreadable by the plain Node child. zod is the only remaining runtime import of the closure (claude-agent-acp → sdk + claude-agent-sdk + zod; the sdk declares no deps of its own and the agent-sdk only optional platform binaries). Verification: pnpm run build:unpack, then an ACP initialize handshake against app.asar.unpacked/out/main/chunks/claude-acp-entry-*.mjs — completes with pure JSON-RPC stdout. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Thanks for the careful review @Git-on-my-level — all three items addressed:
|
|
Thanks for the follow-up and for addressing the earlier dependency concern. I re-reviewed the current head ( I’m still not formally approving because this remains PR 1/4 of a stacked Windows coding-agent runtime feature, touches dependency/runtime packaging, and includes product/security-policy behavior around automatic ACP permission resolution. The existing Maintainer review focus before merge:
No urgent security issue found in this static review, and I’m not asking for a split: the PR is cohesive as an adapter-core slice. This is a positive implementation signal, with human product/security/dependency sign-off still required before merge. |
2 participants
Part 1 of a stacked train (see issue #9302):
Self-contained ACP client under src/main/codingAgent/ ported from desktop/macos/agent/src/adapters — nothing wired into chat/IPC/UI yet. Windows-specific: env allowlist (ComSpec/SystemRoot/PATHEXT), taskkill /t /f tree-kill (no POSIX process groups), windowsHide, ELECTRON_RUN_AS_NODE for the bundled Claude Code bridge.
Verification: pnpm run typecheck + lint clean; pnpm test 577 passed, incl. a real-subprocess integration test (fake ACP peer fixture). Also verified live: the real bridge created a file through this adapter on a Windows 11 machine.