feat: multi-agent engine on the Claude Agent SDK#1
Merged
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds ChangesClaude Agent SDK Integration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant WebSocketClient
participant Session
participant ClaudeAgentEngine
participant SDKquery
participant OpenReplMcpServer
WebSocketClient->>Session: run / stop messages
Session->>ClaudeAgentEngine: run(EngineRun)
ClaudeAgentEngine->>SDKquery: query(prompt, Options)
SDKquery->>OpenReplMcpServer: tool call
OpenReplMcpServer-->>SDKquery: tool result
SDKquery-->>ClaudeAgentEngine: streamed SDK messages
ClaudeAgentEngine->>Session: emit UiEvent
ClaudeAgentEngine-->>Session: RunResult
Session->>WebSocketClient: usage_update / done
Poem
🚥 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 |
Collaborator
Author
Codex review — findings addressedA final independent Codex review of the shipped code surfaced 4 findings; all addressed:
Re-verified after the fixes: (An earlier Codex pass was derailed by this environment's deeplake/hivemind SessionStart hooks failing inside the Codex subprocess; the review above is the recovered, complete one.) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/server/src/agent/claude/engine.ts`:
- Around line 138-140: The early-return in claude engine should not synthesize
placeholder text for aborts that produced no output, because finishRun will
persist it as assistant memory. Update the aborted/partial-result branch in
engine.ts so it only preserves actual generated text (or a real partial result)
and leaves result.text empty when the run was stopped before any assistant
content was produced; use the existing result, finalText, and signal.aborted
checks to keep this behavior localized.
In `@packages/server/src/agent/claude/roles.ts`:
- Around line 54-55: The reviewer role is currently not actually read-only
because REVIEWER_BUILTIN includes Bash, which is controlled by the same
commandAllowlist used for orchestrator commands in engine.ts. Update roles.ts so
the reviewer subagent does not get Bash by default, or introduce a separate
read-only command gate for the reviewer path and wire it through the reviewer
role setup. Use the REVIEWER_BUILTIN and READONLY_BUILTIN definitions in
roles.ts to locate the change and keep the reviewer advisory-only.
In `@packages/server/src/agent/claude/tools.ts`:
- Around line 33-37: The command allowlist check in isCommandAllowed currently
uses startsWith(), which can mistakenly approve commands that only share a
prefix with an allowed entry. Update the matching logic in isCommandAllowed so
an allowlist item is accepted only when it matches the full command exactly or
is followed by a command-boundary whitespace, while still preserving the
existing trim() and SHELL_OPERATORS guard.
In `@packages/server/src/config.ts`:
- Around line 26-38: DEFAULT_CONFIG.model/models in config.ts are currently
Claude-specific, but Session.handle('set_provider') only updates
m.config.provider, so the shared config can leak Claude aliases into
OpenRouter/Codex. Update the config flow in Session.handle('set_provider') and
the model resolution path (including DEFAULT_CONFIG.model/models) so each
provider has its own defaults, or clear/reset provider-incompatible model
selections whenever provider changes. Make sure the fix uses the existing
provider/model config symbols so the OpenRouter/Codex path no longer reads
Claude-only aliases.
In `@packages/server/src/providers/registry.ts`:
- Around line 14-16: The ClaudeProvider wiring in registry.ts is using
ANTHROPIC_API_KEY as the active credential, which implicitly routes Claude runs
to PAYG; update the initialization so ClaudeProvider does not automatically fall
back to getSecret('ANTHROPIC_API_KEY') or process.env.ANTHROPIC_API_KEY unless
PAYG is explicitly opted in. Adjust the credential selection in the registry
constructor to use a subscription-first auth strategy for ClaudeProvider, and
keep the key lookup gated behind an explicit opt-in path.
In `@packages/server/src/session.ts`:
- Around line 302-312: The Claude branch in session handling is still inheriting
the generic non-OpenRouter auth fallback, so Claude users see the wrong `codex
login` guidance. Update the auth/error path in `session.ts` around the
`m.config.provider === 'claude'` flow so Claude requests emit Claude-specific
not-ready messaging instead of the earlier shared message, using the existing
`ClaudeAgentEngine`/provider checks to branch the copy correctly.
- Around line 292-296: The session tooling ignores abort state, so stopped turns
can leave shell commands and app probes running. Update the `ToolDeps` wiring in
`session.ts` so `runCommand` and `runApp` receive and pass along
`controller.signal`, then thread that signal through `CommandRunner.run` and
`probeApp` so both command execution and probing can be cancelled cleanly when
Stop is triggered.
In `@PRD-agent-sdk-migration.md`:
- Around line 101-107: Update this design note to match the implemented role
contract in roles.test and the actual agent setup: remove the unsupported
planner/coder/reviewer subagent split and the claim that allowedTools includes
the six built-ins plus mcp__openrepl__run_app. Rephrase it so the orchestrator
remains the only role with Agent plus the six MCP tools, while planner and
reviewer stay read-only and reviewer keeps Bash only if that is what the
contract enforces. Reference the roles contract and the tool-selection wording
around createSdkMcpServer, allowedTools, and canUseTool so the doc reflects the
current implementation instead of the reverted proposal.
In `@scripts/check-claude-engine.ts`:
- Around line 149-153: Phase A timeout handling in the waitFor catch only marks
process.exitCode, so the script continues into the probe and Phase B with a
potentially still-running build. Update the failure path around waitFor in
check-claude-engine.ts to stop execution immediately after reporting the
timeout, using the existing report and credential context so the script does not
proceed to later phases when the initial run did not finish.
- Around line 128-131: The readiness check is too broad and can pick up a stale
`ready` event from `session.init()` instead of the one for the newly opened
project. Update the `waitFor` predicate in `check-claude-engine.ts` so it
matches both `e.type === 'ready'` and the `workspaceDir` for `projectDir`, using
the existing `UiEvent.ready` shape to ensure Phase A runs against the correct
workspace.
- Around line 85-99: The waitFor helper currently only resolves on a matching
message or rejects on timeout, so it can hang until the full timeout if the
WebSocket closes or errors and it leaves the temporary message listener attached
on failure. Update waitFor(ws, events, pred, ms) to also listen for the socket’s
close/error events and reject immediately when the connection dies, and make
sure every exit path (match, timeout, close, error) removes the registered
message listener and any auxiliary listeners to avoid leaks.
- Around line 31-39: The output-directory creation in report is currently using
the async fs.mkdir call inside a try/catch, so it does not block before
writeFileSync runs. Update report to use the synchronous mkdir API for .test-tmp
(or otherwise ensure the directory exists synchronously) before writing OUT,
keeping the existing best-effort error handling around the directory creation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 53a65466-ccfe-486d-97e9-a07258fc3a67
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (16)
PRD-agent-sdk-migration.mdpackages/server/package.jsonpackages/server/src/agent/claude/engine.tspackages/server/src/agent/claude/map-messages.test.tspackages/server/src/agent/claude/map-messages.tspackages/server/src/agent/claude/roles.test.tspackages/server/src/agent/claude/roles.tspackages/server/src/agent/claude/tools.tspackages/server/src/agent/engine.tspackages/server/src/config.tspackages/server/src/providers/claude.tspackages/server/src/providers/registry.tspackages/server/src/session.tspackages/shared/src/protocol.tsscripts/check-claude-engine.tsvitest.config.ts
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out the documentation. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Collaborator
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
Collaborator
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/server/src/agent/probe.ts (1)
36-41: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winThread
signalinto dependency installation.If Stop happens while
det.installis running, this code only observes the abort afterrunOncereturns, so installs can keep running and delay turn cancellation.Proposed direction
- const r = await runOnce(dir, det.install, env); + const r = await runOnce(dir, det.install, env, signal);Also update
runOnceto terminate its spawned process on abort.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server/src/agent/probe.ts` around lines 36 - 41, Thread the abort signal through dependency installation in probe.ts: the install path in probe() currently waits for runOnce(dir, det.install, env) to finish before checking signal?.aborted, so Stop cannot cancel an in-flight install. Update probe() to pass the signal into runOnce, and change runOnce to accept that signal and terminate its spawned process promptly when aborted, using the existing probe/runOnce symbols to locate the flow.
🧹 Nitpick comments (3)
packages/server/src/agent/claude/engine.test.ts (1)
40-64: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLoop-guard coverage limited to one tool.
The loop-guard tests only exercise
mcp__openrepl__list_dir.READONLY_FOR_LOOP_GUARDalso includesRead,Glob,Grep,read_file, andsearch_repo(perengine.ts), none of which are exercised in a repeated-call scenario here. A regression that breaks dedup logic for those tools specifically wouldn't be caught by this file.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server/src/agent/claude/engine.test.ts` around lines 40 - 64, The loop-guard tests in engine.test.ts only cover repeated mcp__openrepl__list_dir calls, but READONLY_FOR_LOOP_GUARD in canUseTool also applies to Read, Glob, Grep, read_file, and search_repo. Extend the read-only loop-guard coverage by adding repeated-call cases for those tool names against makeCanUseTool so the same dedup/deny behavior is verified for each read-only tool, while keeping side-effecting tools like mcp__openrepl__write_file and mcp__openrepl__run_app expected to always allow.packages/server/test/session.test.ts (1)
23-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the emitted
readypayload, not just that one exists.This still passes if
Sessionemits areadyevent for the wrong workspace. MatchingworkspaceDirhere makes the test protect the actualopen_projectcontract.Suggested change
- expect(events.some((e) => e.type === 'ready')).toBe(true); - expect(events.some((e) => e.type === 'tree')).toBe(true); + expect(events).toContainEqual(expect.objectContaining({ type: 'ready', workspaceDir: dir })); + expect(events).toContainEqual(expect.objectContaining({ type: 'tree' }));As per path instructions, "Prefer asserting on specific values (paths, tool names, model tiers) over generic substrings."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server/test/session.test.ts` around lines 23 - 30, The `opening a project emits ready + tree` test in `session.test.ts` only checks that a `ready` event exists, so tighten it to assert the emitted `ready` payload includes the expected `workspaceDir` for the opened project. Update the assertions around `makeSession`, `session.handle({ type: 'open_project', path: dir })`, and the `events` array so the test verifies the specific `ready` event data rather than a generic match.Source: Path instructions
.github/workflows/ci.yaml (1)
13-33: 📐 Maintainability & Code Quality | 🔵 TrivialWire the Claude end-to-end check into automation.
This workflow never invokes
scripts/check-claude-engine.ts, so the new provider path stays covered only by manual runs. Add a secret-gated job, or at least a scheduled/manual one, so Claude session regressions are caught before merge.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yaml around lines 13 - 33, The CI workflow currently runs typecheck, tests, and build but never executes scripts/check-claude-engine.ts, so the Claude provider path is not covered in automation. Add a new job or step in the existing workflow that runs the Claude end-to-end check, gated by the required secret and/or exposed as a scheduled or manual trigger, and keep it separate from the main npm ci/typecheck/test flow so it only runs when credentials are available.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/codeql.yaml:
- Around line 12-16: The CodeQL workflow is only triggering for pull requests
targeting main, so stacked/intermediate PRs are missing SAST scans. Update the
pull_request trigger in the CodeQL workflow to run on every PR instead of
filtering by branches, while keeping the existing schedule intact. Use the
workflow’s pull_request event definition in codeql.yaml as the place to adjust
this trigger.
In `@packages/server/src/runner.ts`:
- Around line 52-57: The abort handling in runner.ts only terminates the shell
wrapper and still reports a successful exit on cancel. Update the logic around
onAbort and close in the runner execution path to kill the entire spawned
process tree using the same detached process-group approach as workflow.ts, and
make sure an aborted command returns a non-zero exit code instead of defaulting
to 0. Keep the fix centered on the process spawn/kill flow in the runner
function and the p.kill/close handling.
- Line 72: The close handler in runner logic is treating aborted child processes
as success by defaulting `code` to 0. Update the `p.on('close')` callback in
`runner.ts` so it checks the `signal` argument as well as `code`, and returns a
non-zero exit status when the process was terminated by the abort path (for
example after `SIGTERM`) instead of mapping it to 0. Keep the fix localized to
the close/abort handling around `done(...)`.
In `@packages/server/src/session.ts`:
- Around line 103-115: The set_provider branch in session.ts is emitting
provider_status as ok unconditionally, even when the selected provider may not
be ready. Update the provider switch flow in the Session command handling to run
the same readiness check used by provider.isReady() before emitting status, and
emit provider_status with ok only when that check passes, otherwise emit error
so the UI reflects the real precondition. Keep the existing model/default
updates and sendModels behavior in place, but base the status payload on the
readiness result for the selected provider.
In `@packages/server/test/models.test.ts`:
- Around line 35-38: The static catalog test in listModels should assert exact
model IDs instead of loose presence checks, so regressions like missing or
renamed entries are caught. Tighten the expectations in the models.test.ts case
for listModels('codex') and listModels('claude') to compare the full returned id
lists exactly, using the listModels helper and the existing static catalog names
(for example codex, claude, haiku, sonnet, opus, gpt-5-mini, fable) as the
unique anchors.
In `@packages/server/test/run-app-e2e.test.ts`:
- Around line 18-23: The e2e server setup in createServer should not hard-code a
fixed port because it can collide during parallel test runs or when the port is
in use. Update the server initialization in run-app-e2e.test.ts to use an
ephemeral port via createServer, and keep relying on server.url for the chosen
port so the rest of the test flow remains unchanged.
---
Outside diff comments:
In `@packages/server/src/agent/probe.ts`:
- Around line 36-41: Thread the abort signal through dependency installation in
probe.ts: the install path in probe() currently waits for runOnce(dir,
det.install, env) to finish before checking signal?.aborted, so Stop cannot
cancel an in-flight install. Update probe() to pass the signal into runOnce, and
change runOnce to accept that signal and terminate its spawned process promptly
when aborted, using the existing probe/runOnce symbols to locate the flow.
---
Nitpick comments:
In @.github/workflows/ci.yaml:
- Around line 13-33: The CI workflow currently runs typecheck, tests, and build
but never executes scripts/check-claude-engine.ts, so the Claude provider path
is not covered in automation. Add a new job or step in the existing workflow
that runs the Claude end-to-end check, gated by the required secret and/or
exposed as a scheduled or manual trigger, and keep it separate from the main npm
ci/typecheck/test flow so it only runs when credentials are available.
In `@packages/server/src/agent/claude/engine.test.ts`:
- Around line 40-64: The loop-guard tests in engine.test.ts only cover repeated
mcp__openrepl__list_dir calls, but READONLY_FOR_LOOP_GUARD in canUseTool also
applies to Read, Glob, Grep, read_file, and search_repo. Extend the read-only
loop-guard coverage by adding repeated-call cases for those tool names against
makeCanUseTool so the same dedup/deny behavior is verified for each read-only
tool, while keeping side-effecting tools like mcp__openrepl__write_file and
mcp__openrepl__run_app expected to always allow.
In `@packages/server/test/session.test.ts`:
- Around line 23-30: The `opening a project emits ready + tree` test in
`session.test.ts` only checks that a `ready` event exists, so tighten it to
assert the emitted `ready` payload includes the expected `workspaceDir` for the
opened project. Update the assertions around `makeSession`, `session.handle({
type: 'open_project', path: dir })`, and the `events` array so the test verifies
the specific `ready` event data rather than a generic match.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e5ae0ada-c531-4b0a-8e53-8067e37a5fa5
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (30)
.coderabbit.yaml.github/pull_request_template.md.github/workflows/ci.yaml.github/workflows/codeql.yaml.gitignore.jscpd.jsonPRD-agent-sdk-migration.mdpackage.jsonpackages/server/src/agent/claude/engine.test.tspackages/server/src/agent/claude/engine.tspackages/server/src/agent/claude/roles.test.tspackages/server/src/agent/claude/roles.tspackages/server/src/agent/claude/tools.tspackages/server/src/agent/probe.tspackages/server/src/agent/tools.tspackages/server/src/config.tspackages/server/src/providers/demo.tspackages/server/src/providers/models.tspackages/server/src/providers/openai-compatible.tspackages/server/src/runner.tspackages/server/src/session.tspackages/server/test/demo-provider.test.tspackages/server/test/models.test.tspackages/server/test/openai-compatible.test.tspackages/server/test/orchestrator.test.tspackages/server/test/run-app-e2e.test.tspackages/server/test/session.test.tspackages/web/src/App.tsxpackages/web/src/store.tsscripts/check-claude-engine.ts
💤 Files with no reviewable changes (5)
- packages/server/test/orchestrator.test.ts
- packages/server/src/providers/demo.ts
- packages/server/src/providers/openai-compatible.ts
- packages/server/test/demo-provider.test.ts
- packages/server/test/openai-compatible.test.ts
✅ Files skipped from review due to trivial changes (5)
- .github/pull_request_template.md
- .jscpd.json
- .gitignore
- packages/web/src/App.tsx
- PRD-agent-sdk-migration.md
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/server/src/agent/claude/engine.ts
- packages/server/src/agent/claude/roles.ts
- packages/server/src/agent/claude/roles.test.ts
- packages/server/src/agent/claude/tools.ts
- packages/server/src/config.ts
- scripts/check-claude-engine.ts
Coverage Report
File Coverage |
…ver, preview, session, shell)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Replaces the hand-rolled multi-agent layer (Vercel AI SDK) with an engine built on Anthropic's Claude Agent SDK (
@anthropic-ai/claude-agent-sdk). Spec:PRD-agent-sdk-migration.md.provider: 'claude'(default), subscription-first auth (each self-hosted user uses their own Claude plan), withANTHROPIC_API_KEYas the pay-as-you-go fallback.Architecture (validated by the real-run e2e)
The end-to-end test against a real Claude subscription proved that an in-process MCP server is only reachable from the main thread — delegated subagents that try to write files are unreliable (
"Stream closed", non-deterministic writes). So the shipped design is:write_file/run_command/run_app) — deterministic, goes throughworkspace.writeFile+probeApp, fires the live-editor watcher, and drives the run → fix self-healing loop.Read/Glob/Grep[/Bash]) — their output is text, so async delegation can't corrupt file state.Verification
End-to-end (real subscription, builds + runs a Flask todo app) — PASS on all criteria:
Run it:
npx tsx scripts/check-claude-engine.ts(skips cleanly without a Claude credential).tsc --noEmit).delegate_to_*tools), sharedisCommandAllowedthat rejects shell-operator chaining past the allowlist (&&/;/|),cwdanchored to the workspace, and env inheritance instead of spreading host config.Notes
packages/server/test/fail onmaintoo (demo provider removed, mock drift) — verified by running them against the unmodified base. Out of scope for this PR.STATUS/PLAN/BACKLOG/...) are intentionally kept out of the public repo.Summary by CodeRabbit