feat(grok): add Grok CLI provider via ACP#2809
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 |
github-actions
Bot
added
vouch:unvouched
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
ApprovabilityVerdict: Needs human review 1 blocking correctness issue found. This PR adds a complete new Grok CLI provider integration with ~3000 lines of new code spanning driver, adapter, provider layer, text generation, and UI components. Additionally, unresolved review comments identify a potential race condition in the adapter's interruptTurn method that should be addressed. You can customize Macroscope's approvability policy. Learn more. |
Wires the Grok CLI (https://x.ai/cli) into the existing ACP runtime so X Premium / SuperGrok users get a first-class provider. - GrokDriver registered through builtInDrivers - GrokProvider / GrokAdapter handle session lifecycle, event streams, and model switching via session/set_model - GrokAcpSupport probes auth methods and drives the xai.api_key flow - GrokTextGeneration adds Grok as an option (not a default) for git commit/branch generation - grok-build registered as the only Grok model id (only one currently exposed by the CLI) - UI: Grok icon, provider selector entry, settings driver metadata, provider icon mapping Defaults are unchanged: Codex remains the default provider. Closes pingdotgg#2808
Jaaneek
force-pushed
the
feat/grok-provider
branch
from
30e7372 to
a58e850
Compare
|
was looking for this, would really appreciate if it's tested well and merged to main soon. |
- Use crypto-backed UUIDs and normalize ACP callback failures - Fix chat composer refs and browser secret record updates - Tighten runtime catalog hydration test setup
- Use provider-supplied approval option IDs when responding - Keep streaming if native notification logging fails
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| const interruptTurn: GrokAdapterShape["interruptTurn"] = (threadId) => | ||
| Effect.gen(function* () { | ||
| const ctx = yield* requireSession(threadId); | ||
| yield* settlePendingApprovalsAsCancelled(ctx.pendingApprovals); | ||
| yield* Effect.ignore( | ||
| ctx.acp.cancel.pipe( | ||
| Effect.mapError((error) => | ||
| mapAcpToAdapterError(PROVIDER, threadId, "session/cancel", error), | ||
| ), | ||
| ), | ||
| ); | ||
| }); |
There was a problem hiding this comment.
🟡 Medium Layers/GrokAdapter.ts:732
interruptTurn modifies session state without acquiring the thread lock, so it can race with sendTurn or stopSession on the same threadId. This allows concurrent mutation of ctx.pendingApprovals and overlapping calls to ctx.acp.cancel while another operation is mid-execution. Consider wrapping the body in withThreadLock(threadId, ...) to match the other session-modifying operations.
- const interruptTurn: GrokAdapterShape["interruptTurn"] = (threadId) =>
- Effect.gen(function* () {
- const ctx = yield* requireSession(threadId);
- yield* settlePendingApprovalsAsCancelled(ctx.pendingApprovals);
- yield* Effect.ignore(
- ctx.acp.cancel.pipe(
- Effect.mapError((error) =>
- mapAcpToAdapterError(PROVIDER, threadId, "session/cancel", error),
- ),
- ),
- );
- });🤖 Copy this AI Prompt to have your agent fix this:
In file apps/server/src/provider/Layers/GrokAdapter.ts around lines 732-743:
`interruptTurn` modifies session state without acquiring the thread lock, so it can race with `sendTurn` or `stopSession` on the same `threadId`. This allows concurrent mutation of `ctx.pendingApprovals` and overlapping calls to `ctx.acp.cancel` while another operation is mid-execution. Consider wrapping the body in `withThreadLock(threadId, ...)` to match the other session-modifying operations.
Evidence trail:
GrokAdapter.ts line 209-210: `withThreadLock` definition uses per-thread Semaphore with permit=1. Line 305: `startSession` uses `withThreadLock`. Lines 581, 688: `sendTurn` uses `withThreadLock`. Line 797: `stopSession` uses `withThreadLock`. Lines 732-743: `interruptTurn` does NOT use `withThreadLock` but mutates `ctx.pendingApprovals` via `settlePendingApprovalsAsCancelled` and calls `ctx.acp.cancel`. Lines 285-302: `stopSessionInternal` (called from `stopSession` under lock) also calls `settlePendingApprovalsAsCancelled(ctx.pendingApprovals)`.
|
Thanks for pushing this forward. I think the concurrency comments are valid and not really Grok-specific; they belong at the shared ACP adapter boundary. A safe shape is:
I opened #2932 with that shape for review. It extracts Cursor/Grok into a shared |
Handle provider model-change restrictions
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit b6b8e4a. Configure here.
| ), | ||
| ), | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Interrupt lacks thread lock
Medium Severity
interruptTurn does not use withThreadLock while sendTurn can be mid-session/prompt outside the lock. Cancel and a new turn can race on the same session without coordinated serialization.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit b6b8e4a. Configure here.
There was a problem hiding this comment.
this is also like that in other providers I think?
4 participants
What Changed
#2808
Add Grok Build via ACL
Why
Grok Build was recently released and its accessible via X Premium Subs and Supergrok Subs
UI Changes
Checklist
Note
Medium Risk
Touches orchestration turn-start paths and a large new ACP child-process adapter; model-change gating is user-visible but localized to provider metadata and composer checks.
Overview
Adds Grok as a first-class provider: a new driver runs
grok agent stdioover ACP (GrokAdapter,GrokProvider, health/model discovery), with headless text generation via the same ACP path and contracts/settings/UI wiring (icons, picker,GrokSettings, defaults).ACP integration gains typed
session/set_model(runtime + mock agent) and permission replies use agent-supplied option ids. Provider snapshots can setrequiresNewThreadForModelChange; the server reactor rejects mid-thread model switches when required, and the web composer shows a warning toast for the same rule.Test harnesses gain a
ProviderRegistrymock layer; integration/orchestration tests cover the new restriction.Reviewed by Cursor Bugbot for commit f50d50a. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Add Grok CLI provider via ACP with full session, text generation, and UI support
GrokDriverprovider that spawnsgrok agent stdiovia ACP, manages session lifecycle, discovers available models, and reports provider status by runninggrok --version.GrokTextGenerationbacked by the ACP runtime to handle commit messages, PR content, branch names, and thread titles with a 180s timeout and schema-validated JSON output.GrokAcpSupportutilities includingmakeGrokAcpRuntime,resolveGrokAcpBaseModelId, andapplyGrokAcpModelSelectionfor model switching viasession/set_model.requiresNewThreadForModelChangepolicy inProviderCommandReactorandChatView: mid-session model changes are blocked with an error/toast when either the current or next provider sets this flag.Macroscope summarized f50d50a.