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 3 blocking correctness issues found. This PR introduces a complete new Grok provider integration with substantial new code (~3400 lines). As a new feature adding new workflows and integrations, it warrants human review. Additionally, there are 4 unresolved medium-severity review comments identifying potential race conditions and logic issues 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?
| export function extractXAiAskUserQuestions(params: unknown): ReadonlyArray<UserInputQuestion> { | ||
| const root = unwrapParams(params); | ||
| const title = stringField(root, ["title", "header", "toolTitle"]); | ||
| const questions = arrayField(root, ["questions", "items", "prompts"]); | ||
| if (questions.length > 0) { | ||
| return questions.map((question, index) => extractQuestion(question, title, index)); | ||
| } | ||
| const singleQuestion = nestedRecord(root, ["question"]) ?? root; | ||
| const singleQuestionOptions = arrayField(root, ["options", "choices", "answers"]); | ||
| const question = | ||
| singleQuestion === root || singleQuestionOptions.length === 0 | ||
| ? singleQuestion | ||
| : { ...singleQuestion, options: singleQuestionOptions }; | ||
| return [extractQuestion(question, title, 0)]; | ||
| } |
There was a problem hiding this comment.
🟡 Medium acp/XAiAcpExtension.ts:122
In the multi-question code path, when a question has a nested question object with sibling options at the top level, the options are ignored. For example, { question: { prompt: "Pick one" }, options: ["A", "B"] } returns the default [{ label: "OK", description: "Continue" }] instead of the provided options. The questionSource variable points to the nested object, but extractOptions is called on questionSource instead of record, so sibling options are lost. The single-question path in extractXAiAskUserQuestions handles this by merging options before calling extractQuestion, but the multi-question path lacks this step.
export function extractXAiAskUserQuestions(params: unknown): ReadonlyArray<UserInputQuestion> {
const root = unwrapParams(params);
const title = stringField(root, ["title", "header", "toolTitle"]);
const questions = arrayField(root, ["questions", "items", "prompts"]);
if (questions.length > 0) {
- return questions.map((question, index) => extractQuestion(question, title, index));
+ return questions.map((question, index) => {
+ const record = isRecord(question) ? question : {};
+ const nestedQuestion = nestedRecord(record, ["question"]);
+ const options = arrayField(record, ["options", "choices", "answers"]);
+ const questionWithOptions =
+ nestedQuestion && options.length > 0
+ ? { ...nestedQuestion, options }
+ : question;
+ return extractQuestion(questionWithOptions, title, index);
+ });
}
const singleQuestion = nestedRecord(root, ["question"]) ?? root;
const singleQuestionOptions = arrayField(root, ["options", "choices", "answers"]);🤖 Copy this AI Prompt to have your agent fix this:
In file apps/server/src/provider/acp/XAiAcpExtension.ts around lines 122-136:
In the multi-question code path, when a question has a nested `question` object with sibling `options` at the top level, the options are ignored. For example, `{ question: { prompt: "Pick one" }, options: ["A", "B"] }` returns the default `[{ label: "OK", description: "Continue" }]` instead of the provided options. The `questionSource` variable points to the nested object, but `extractOptions` is called on `questionSource` instead of `record`, so sibling `options` are lost. The single-question path in `extractXAiAskUserQuestions` handles this by merging options before calling `extractQuestion`, but the multi-question path lacks this step.
Evidence trail:
File: apps/server/src/provider/acp/XAiAcpExtension.ts at REVIEWED_COMMIT
- Lines 97-120: `extractQuestion` function — line 104 sets `questionSource = nestedQuestion ?? record`, line 118 calls `extractOptions(arrayField(questionSource, ...))` which searches `questionSource` (the nested object) not `record` (the original element)
- Lines 80-95: `extractOptions` returns default `[{ label: "OK", description: "Continue" }]` when given empty array (line 94)
- Lines 126-128: multi-question path passes each question directly to `extractQuestion` without merging sibling options
- Lines 129-135: single-question path explicitly merges sibling options via `{ ...singleQuestion, options: singleQuestionOptions }` at line 134
| const nf = yield* Stream.runDrain( | ||
| Stream.mapEffect(acp.getEvents(), (event) => | ||
| Effect.gen(function* () { | ||
| switch (event._tag) { | ||
| case "AssistantItemStarted": | ||
| yield* offerRuntimeEvent( | ||
| makeAcpAssistantItemEvent({ | ||
| stamp: yield* makeEventStamp(), | ||
| provider: PROVIDER, | ||
| threadId: ctx.threadId, | ||
| turnId: ctx.activeTurnId, | ||
| itemId: event.itemId, | ||
| lifecycle: "item.started", | ||
| }), | ||
| ); | ||
| return; | ||
| case "AssistantItemCompleted": | ||
| yield* offerRuntimeEvent( | ||
| makeAcpAssistantItemEvent({ | ||
| stamp: yield* makeEventStamp(), | ||
| provider: PROVIDER, | ||
| threadId: ctx.threadId, | ||
| turnId: ctx.activeTurnId, | ||
| itemId: event.itemId, | ||
| lifecycle: "item.completed", | ||
| }), | ||
| ); | ||
| return; | ||
| case "PlanUpdated": | ||
| yield* logNative(ctx.threadId, "session/update", event.rawPayload); | ||
| yield* emitPlanUpdate(ctx, event.payload, event.rawPayload, "session/update"); | ||
| return; | ||
| case "ToolCallUpdated": | ||
| yield* logNative(ctx.threadId, "session/update", event.rawPayload); | ||
| yield* offerRuntimeEvent( | ||
| makeAcpToolCallEvent({ | ||
| stamp: yield* makeEventStamp(), | ||
| provider: PROVIDER, | ||
| threadId: ctx.threadId, | ||
| turnId: ctx.activeTurnId, | ||
| toolCall: event.toolCall, | ||
| rawPayload: event.rawPayload, | ||
| }), | ||
| ); | ||
| return; | ||
| case "ContentDelta": | ||
| yield* logNative(ctx.threadId, "session/update", event.rawPayload); | ||
| yield* offerRuntimeEvent( | ||
| makeAcpContentDeltaEvent({ | ||
| stamp: yield* makeEventStamp(), | ||
| provider: PROVIDER, | ||
| threadId: ctx.threadId, | ||
| turnId: ctx.activeTurnId, | ||
| ...(event.itemId ? { itemId: event.itemId } : {}), | ||
| text: event.text, | ||
| rawPayload: event.rawPayload, | ||
| }), | ||
| ); | ||
| return; | ||
| } | ||
| }), | ||
| ), | ||
| ).pipe( | ||
| Effect.catch((cause) => | ||
| Effect.logError("Failed to process Grok runtime notification.", { cause }), | ||
| ), | ||
| Effect.forkChild, | ||
| ); |
There was a problem hiding this comment.
🟡 Medium Layers/GrokAdapter.ts:551
The catch at line 613-616 wraps Stream.runDrain, so any single event failure terminates the entire notification fiber. Once terminated, the session stops receiving all runtime events while still appearing active. Consider moving error handling inside the Stream.mapEffect callback to catch per-event failures without stopping the stream.
- ).pipe(
- Effect.catch((cause) =>
- Effect.logError("Failed to process Grok runtime notification.", { cause }),
- ),
- Effect.forkChild,
- );🤖 Copy this AI Prompt to have your agent fix this:
In file apps/server/src/provider/Layers/GrokAdapter.ts around lines 551-618:
The `catch` at line 613-616 wraps `Stream.runDrain`, so any single event failure terminates the entire notification fiber. Once terminated, the session stops receiving all runtime events while still appearing active. Consider moving error handling inside the `Stream.mapEffect` callback to catch per-event failures without stopping the stream.
Evidence trail:
apps/server/src/provider/Layers/GrokAdapter.ts lines 551-620 (REVIEWED_COMMIT): Stream.runDrain wraps the entire stream; Effect.catch at line 613-616 catches after drain terminates; Effect.forkChild at line 617 forks the recovered effect.
apps/server/src/provider/Layers/GrokAdapter.ts line 99 (REVIEWED_COMMIT): notificationFiber typed as Fiber.Fiber<void, never> — never error type confirms catch fully recovers.
apps/server/src/provider/Layers/GrokAdapter.ts lines 306-316 (REVIEWED_COMMIT): stopSessionInternal is the only path that calls sessions.delete and emits session.exited — not triggered by fiber completion.
apps/server/src/provider/Layers/GrokAdapter.ts line 620: ctx.notificationFiber = nf — session retains reference but has no mechanism to detect fiber completion.
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
New provider stack touches orchestration turn routing and external CLI/ACP processes; model-change gating is a behavioral change for restricted providers but is covered by server and client tests.
Overview
Adds Grok as a first-class provider: a new
GrokDriverruns the Grok CLI over ACP (grok agent stdio), with adapter sessions, permissions, plan/tool streaming,session/set_model, and the_x.ai/ask_user_questionextension mapped to user-input flows. Provider health checks probegrok --versionand discover models via a short ACP startup; snapshots advertiserequiresNewThreadForModelChange.Orchestration and UI now honor that flag: after a thread has started,
ProviderCommandReactorrejects turn starts that change model/instance when either side requires a new thread, and the composer shows a warning toast via shared logic inChatView.Headless text generation for Grok uses the same ACP path (
GrokTextGeneration) for commits, PRs, branch names, and titles. Contracts/settings gainGrokSettings, default modelgrok-build, andServerProvider.requiresNewThreadForModelChange; the web app registers icons, picker entries, and settings forms. Tests and the ACP mock agent are extended for Grok models, custom approval option IDs, and xAI ask-user flows.Reviewed by Cursor Bugbot for commit 2177da0. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Add Grok CLI provider via ACP with text generation and session model switching
grokprovider driver (GrokDriver.ts) that spawns the Grok CLI via ACP, discovers models, checks provider status, and surfaces snapshot enrichment with version advisories.GrokAdapterfor session lifecycle management including approval/user-input flows, xAI extension handling (_x.ai/ask_user_question), and native event logging.GrokTextGenerationfor commit messages, PR content, branch names, and thread titles via ACP prompts with a 180s timeout and structured JSON output.requiresNewThreadForModelChangeflag toServerProvidersnapshots;ProviderCommandReactornow rejects mid-thread model changes when this flag is set on either the current or target provider.grokin contracts, settings, built-in drivers, provider icon map, and the UI provider picker with an Early Access/new badge.Macroscope summarized 2177da0.