Uh oh!
There was an error while loading. Please reload this page.
feat: add Grok Build provider via shared ACP adapter - #2932
Conversation
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit d4e5db1. Configure here.
| ? { message: "Grok Build ACP model discovery returned no built-in models." } | ||
| : {}), | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Grok probe ignores models list
Medium Severity
checkGrokBuildProviderStatus discovers models only from ACP configOptions, but Grok Build can expose models via a models session payload without config options. In that case discovery returns no models and the probe warns even though in-session session/set_model works.
Reviewed by Cursor Bugbot for commit d4e5db1. Configure here.
| }, | ||
| }); | ||
| return yield* Effect.failCause(promptExit.cause); | ||
| } |
There was a problem hiding this comment.
No terminal turn after stop
Medium Severity
sendTurn emits turn.started then runs session/prompt outside the per-thread lock. If the session is stopped while the prompt is in flight, the finalize step finds no session and returns without emitting turn.completed, leaving the thread stuck in a started turn.
Reviewed by Cursor Bugbot for commit d4e5db1. Configure here.
ApprovabilityVerdict: Needs human review This PR introduces a new Grok Build provider integration with ~2500 lines of new code spanning driver, adapter, text generation, settings, and UI layers. New feature additions of this scope require human review, and two unresolved comments identify potential bugs in model discovery and turn state handling. You can customize Macroscope's approvability policy. Learn more. |
| stopped: false, | ||
| }; | ||
| const nf = yield* Stream.runDrain( |
There was a problem hiding this comment.
🟡 MediumLayers/AcpProviderAdapter.ts:834
When a single event fails to process (e.g., makeEventStamp() throws), the Effect.catch at line 919-924 catches the error and terminates the entire notificationFiber. The session continues running but silently stops emitting all runtime events because the stream is drained. The catch should wrap individual event processing inside mapEffect so one bad event doesn't kill the whole stream.
🤖 Copy this AI Prompt to have your agent fix this:
In file apps/server/src/provider/Layers/AcpProviderAdapter.ts around line 834:
When a single event fails to process (e.g., `makeEventStamp()` throws), the `Effect.catch` at line 919-924 catches the error and terminates the entire `notificationFiber`. The session continues running but silently stops emitting all runtime events because the stream is drained. The `catch` should wrap individual event processing inside `mapEffect` so one bad event doesn't kill the whole stream.
Evidence trail:
apps/server/src/provider/Layers/AcpProviderAdapter.ts lines 834-928: Stream.runDrain wraps entire stream, Effect.catch at lines 919-924 catches runDrain failure, Effect.forkChild at line 925 forks it as child fiber, fiber stored at line 928. makeEventStamp defined at line 390 as an Effect that could fail. Session stored at line 929 and continues running after fiber completes.
Context
This draft PR is a supporting/reference implementation for #2809 and the review concerns raised there. The goal is to help get Grok Build support landed in T3 Code with the ACP lifecycle issues addressed in a shared adapter; maintainers or the #2809 author should feel free to use, cherry-pick, supersede, or close this if it creates review noise.
Summary
grok --agent build agent stdio, with settings, model selection, x.ai branding, web UI, and mobile provider icon support.makeAcpProviderAdapter(...)so Cursor and Grok Build share turn dispatch, resume, interrupt, and approval handling.--worktreeflag.ACP lifecycle hardening
session/promptoutside the per-thread lock so approval/cancel flows do not deadlock.turn.startedemits a terminal failedturn.completedevent before surfacing the original failure.interruptTurnwith the same per-thread state lock used by send/stop.session/set_modelfallback when an ACP server exposes models but no config option.Test plan
bun fmtbun lint(9 warnings, 0 errors)bun typecheck(14/14 packages)bun run --cwd apps/server test src/provider/acp/AcpJsonRpcConnection.test.ts src/provider/acp/GrokAcpSupport.test.ts src/provider/Layers/ProviderInstanceRegistryLive.test.ts src/provider/Layers/CursorAdapter.test.ts src/provider/Layers/GrokBuildProvider.test.tsbun run --cwd apps/web test src/components/chat/providerIconUtils.test.ts src/components/settings/providerDriverMeta.test.ts src/components/settings/ProviderSettingsForm.test.tsbun run --cwd packages/contracts test src/settings.test.tsNote
Medium Risk
Large refactor of Cursor’s ACP session/turn path affects all ACP providers; new external CLI dependency and child-process auth/model discovery add operational failure modes.
Overview
Adds Grok Build as a first-class provider (
grokBuild) wired through contracts, server driver/registry, ACP runtime, structured text generation, and web/mobile picker/settings with x.ai branding.Server: New
GrokBuildDriverspawnsgrok --agent build agent stdio, probes health/models via ACP, and reuses Cursor-style model discovery. Cursor’s large ACP adapter implementation moves into sharedmakeAcpProviderAdapter; Cursor and Grok thin-wrap it. ACP hardening for all consumers: per-thread turn exclusivity (prompt runs outside the lock), failedturn.completedafter prompt errors,session/set_modelwhen sessions expose models without config options, and auth-method fallback from initialize.Clients:
GrokBuildSettings(default off), model defaults, composer draft keys, provider icons, and registry tests include the fifth driver.Reviewed by Cursor Bugbot for commit d4e5db1. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Add Grok Build as a new provider via shared ACP adapter
grokBuildprovider driver in GrokBuildDriver.ts, registered in the built-in driver registry alongside Cursor and OpenCode.grokCLI binary via ACP stdio withgrok_loginauth.CursorAdapterto delegate to a new sharedmakeAcpProviderAdapterin AcpProviderAdapter.ts, whichGrokBuildAdapteralso uses.grokBuildwith display name 'Grok Build', a new XAI icon, and an 'Early Access' badge.AcpSessionRuntime.setModelnow issues asession/set_modelRPC when the session exposes models without a config option, rather than always writing a config option.📊 Macroscope summarized 8ae98ac. 2 files reviewed, 0 issues evaluated, 0 issues filtered, 0 comments posted
🗂️ Filtered Issues
No issues evaluated.