feat(claude-cli): serve a tool catalog through the capture-only bridge - #5795
RobinBially wants to merge 2 commits into
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: lidge-jun/opencodex/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe shared capture-only MCP bridge is generalized for coding-agent CLI adapters. Claude CLI now uses the bridge for requests with a tool catalog. CodeBuddy and its existing tests use the shared implementation. ChangesCapture-only MCP tool bridge
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant Client
participant ClaudeCLIAdapter
participant CodingAgentTurn
participant ClaudeCodeCLI
participant CodingAgentMCPServer
Client->>ClaudeCLIAdapter: Submit request with tool catalog
ClaudeCLIAdapter->>CodingAgentTurn: Pass bridge input and staged prompt
CodingAgentTurn->>ClaudeCodeCLI: Start CLI with MCP configuration
ClaudeCodeCLI->>CodingAgentMCPServer: Connect and invoke advertised tool
CodingAgentMCPServer->>CodingAgentTurn: Return captured tool call
CodingAgentTurn->>Client: Return call using request wire name
Merge Risk: ⚪ Minimal · up to The new tool bridge does not materially expand access to the signed-in account, and no merge-blocking issue remains established. Merge after normal checks. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Deterministic PR hygiene checks passed. |
✅ READY
Review readiness checklist
✅ 4/4 boxes ticked. This pull request is already Ready for Review. |
|
@coderabbitai review |
|
The Claude Code CLI provider answered every turn tools-disabled, so a request that carried a tool catalog came back as text only. Arming the capture bridge the CodeBuddy rows already use closes that gap: the CLI is launched with --mcp-config plus the exact --allowedTools list, advertises the request's catalog from the isolated stdio server, and a captured call is returned to the client, which keeps approval, sandboxing and execution. The bridge and its MCP server move from src/adapters/codebuddy/ to src/adapters/coding-agent/. Both harnesses speak the same contract (verified against CodeBuddy Code and Claude Code 2.1.281: the init frame reports the server as connected and renders calls as mcp__<server>__<tool>), so the family modules now supply only their own arguments, child environment and MCP server path. The private compiled-binary entrypoint follows the move (__codebuddy-mcp -> __coding-agent-mcp), bridge failures read family-neutrally, and the error a client sees still names its provider. A catalog turn stages the same private prompt file as before, now with the bridge directive folded in, and requests without a catalog keep the previous text-only argument shape.
90f20ab to
1b45e10
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
리뷰 · 우선순위 60 / 80이 풀리퀘스트는 바탕이 다리 파일은 라인 - 라인 - 메인테이너의 판단이 필요한 지점 구독 계정으로 이 프록시를 열어도 되는지는 레지스트리 노트가 이미 맡긴 판단이에요. 이 글부터는 그 계정으로 도구 호출 제안도 돌아요. 실행은 클라이언트가 해요. Claude Code 2.1.281에 턴을 한 번으로 막는 플래그가 있는지도 봐 주세요. 주석은 그런 플래그가 없다고 해요. 없다면 도구가 여러 개일 때 너의 추천 방향은 맞아요. 바탕은 넣기 전에 두 곳만 고치면 좋겠어요. 98–99행 주석은 " 이 댓글은 grok-bot이 작성했습니다 |
… note The adapter comment claimed the CLI exposes no --max-turns. It does carry one, but hidden from --help, and it is not the bound this leg relies on: against a capture-only server on 2.1.282 the flag produced a terminal result frame of subtype error_max_turns while the process stayed open behind the never-answering MCP child. The comment now names the real bound (turn.ts terminating the tree at message_stop) and the test records the same measured reason. Also indents the claude-cli registry note to the four spaces its neighbours use.
|
Both line notes are fixed in a5c5ccd, and I ran the two measurements you asked for. Multi-tool A flag that bounds a turn — one exists, and my comment was wrong about it. On the two judgment calls: the subscription-authorization question stays where the registry note already puts it (flagged for maintainer review, unchanged by this commit), and roster discovery remains out of scope here — it stays in #5147. Verification on this head: |
|
Closing as superseded before review spending continues. The bridge gets the idea right — the client keeps tool ownership and the CLI only proposes calls — but it is built on the wrong substrate: a one-shot The subscription route is being rebuilt on the Claude Agent SDK instead — the same substrate meridian uses: a real managed session per conversation, prompt caching, the harness's own tool/permission machinery, and in-process MCP servers for client tools instead of an argv-level bridge. When that lands, client tools come back through the SDK's own mechanism and this argv path disappears rather than being extended. No maintainer time was spent on this besides the priority review; thanks for that review — the |
Summary
claude-cliis launched with--mcp-configplus an exact--allowedToolslist, the isolated stdio server advertises that catalog,system/initmust report it as connected or the turn fails closed, and a captured call returns to the client under the request's wire name. Approval, sandboxing and execution stay with the external client, and a request without a catalog keeps the previous text-only argument shape.tool-bridge.tsandmcp-server.tsmove fromsrc/adapters/codebuddy/tosrc/adapters/coding-agent/, because both harnesses speak the same capture contract. The family modules keep supplying only their own arguments, child environment and MCP server path. The private compiled-binary entrypoint follows the move (__codebuddy-mcp->__coding-agent-mcp); bridge failure text is family-neutral, and what a client sees still names its provider (Invalid Claude Code tool catalog: ...).Verification
system/initreportsmcp_servers: [{ name: "opencodex", status: "connected", source: "dynamic" }]plus the advertised catalog tools, the model's call arrives asmcp__opencodex__echo, and the process parks on the never-answering capture handler aftermessage_stop(noresultframe ever arrives). A full adapter turn with aread_filecatalog finished in 3.8 s:tool_call_start(read_file), two argument deltas,tool_call_end, thendone { stopReason: "tool_use", endTurn: false, usage: input 2 / output 58 / cache-creation 1366 }.npm i @anthropic-ai/claude-code@latestin a scratch prefix, driven against a two-tool MCP probe):--allowedToolsas one comma-joined argument permits the second listed name (permission_denials: []), while allowing only the first name turns the same prompted call intopermission_denials: [{ tool_name: "mcp__probe__echo_b", ... }]and it never reaches the server.--max-turnsdoes exist —--max-turns <turns>, "maximum number of agentic turns in non-interactive mode" — but it is hidden from--helpand is deliberately not passed: against the capture-only server it ends the turn with a terminalresultframe ofsubtype: "error_max_turns"while the process stays parked behind the never-answering MCP child, so the bound remainsturn.tsterminating the tree atmessage_stop. The adapter comment and the sibling test now record that measurement instead of claiming no such flag exists.bun run typecheckclean.bun test tests/providers/claude-cli-tool-bridge.test.ts tests/providers/claude-cli-adapter.test.ts tests/providers/codebuddy-tool-bridge.test.ts tests/providers/codebuddy-tool-bridge-turn.test.ts tests/adapters/adapter-tool-conformance.test.ts tests/test-layout.test.ts tests/test-layout-tooling.test.ts— 100 pass / 0 fail.tests/providers/codebuddy-mcp-server.test.tsseparately compiles the standalone binary and drives the renamed__coding-agent-mcpentrypoint — 5 pass / 0 fail.bun run structure:check,bun run privacy:scanandbun run skill:surface:checkpass.bun run testcannot complete on this machine today: the4x PARALLELlane aborts with a Bun worker SIGSEGV — the runner itself reports "This indicates a bug in Bun or in a native addon, not in the test itself" — at a file the diff does not touch. Two attempts died at different files (tests/providers/command-code-tool-text.test.ts: passes standalone 40/40;tests/codex-integration/codex-account-label.test.ts: passes standalone 6/6).tests/codex-integration/codex-inject-integration.test.tsadditionally stalls, and stalls identically on a pristineorigin/devworktree (git worktree add .tmp/control-dev --detach origin/dev). I therefore drove the suite through the same runner in 100-file lanes instead. The lanes that reported totals: 28,079 pass / 59 skipped / 119 distinct failing tests. Every one of those failures is pre-existing here rather than caused by this branch: the 16 test files they come from produce byte-identical pass/fail counts on a pristineorigin/devworktree (git worktree add .tmp/control-dev --detach origin/dev; 0 differing files out of 16 compared). They are the WebSocket steering/injection suites (tests/responses/ws-native-injection.test.ts21/24,ws-native-steering17/16,ws-steering-completion21/14,ws-native-result-continuations50/7,ws-steering-stability27/1), the local-CA/picker TLS tests, a few Chat SSE/refusal cases and the Codex-config/gather load cases. Coverage left to CI: the lanes fortests/providers/cursor,tests/windows,tests/vision,tests/videosandtests/web-searchreport every file but their runner process wedges on teardown in this environment, andtests/codex-integration/codex-inject-integration.test.tswedges on the pristine control worktree too. The failures are the pre-existing environment ones:tests/ci-workflows/test-runner.test.ts,tests/claude-integration/claude-intercept-local-ca.test.ts,tests/clients/integrations-state.test.ts,tests/adapters/openai/openai-provider-option-e2e.test.ts,tests/claude-integration/claude-picker-listener.test.tsandtests/cli/update-stop-first.test.tsfail with identical counts on the pristineorigin/devcontrol worktree, and the remaining names are the known load-dependent Codex-config/gather cases. Coverage left to CI: the two lanes above that this runner cannot execute here.Checklist
Review readiness checklist
This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:
Required local validation passed; commands, results, and any full-suite exception are documented.
I pushed my PR to a recent dev commit (at most 10 behind; a maintainer may still ask for the exact tip before merge).
I resolved all correct Codex and CodeRabbit findings.
My PR is ready for review.
Summary by CodeRabbit