feat(grok): plan mode via /plan text mapping - #5424
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 Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| if (isRecord(initializeModelState) && Array.isArray(initializeModelState.availableModels)) { | ||
| return pick( | ||
| initializeModelState.availableModels as ReadonlyArray<{ | ||
| modelId: string; | ||
| _meta?: unknown; | ||
| }>, | ||
| input.preferredModelId ?? | ||
| (typeof initializeModelState.currentModelId === "string" | ||
| ? initializeModelState.currentModelId | ||
| : undefined), | ||
| ); | ||
| } |
There was a problem hiding this comment.
🟠 High Layers/GrokAdapter.ts:298
initializeMeta.modelState.availableModels is only checked with Array.isArray, then each entry is cast to { modelId: string; _meta?: unknown } without validation. If the array contains a malformed entry such as null, pick calls model.modelId inside find and throws, failing session startup instead of ignoring unusable metadata. Validate each entry as a record with a string modelId before passing it to pick.
| if (isRecord(initializeModelState) && Array.isArray(initializeModelState.availableModels)) { | |
| return pick( | |
| initializeModelState.availableModels as ReadonlyArray<{ | |
| modelId: string; | |
| _meta?: unknown; | |
| }>, | |
| input.preferredModelId ?? | |
| (typeof initializeModelState.currentModelId === "string" | |
| ? initializeModelState.currentModelId | |
| : undefined), | |
| ); | |
| } | |
| const initializeModelState = input.initializeMeta?.modelState; | |
| if (isRecord(initializeModelState) && Array.isArray(initializeModelState.availableModels)) { | |
| + const models = initializeModelState.availableModels.filter( | |
| + (model): model is { modelId: string; _meta?: unknown } => | |
| + isRecord(model) && typeof model.modelId === "string", | |
| + ); | |
| return pick( | |
| models, | |
| input.preferredModelId ?? | |
| (typeof initializeModelState.currentModelId === "string" | |
| ? initializeModelState.currentModelId | |
| : undefined), | |
| ); | |
| } |
Also found in 1 other location(s)
apps/server/src/provider/Layers/GrokProvider.ts:285
initializeMeta.modelStateis unchecked metadata but is cast directly toSessionModelStateand passed tobuildGrokDiscoveredModelsFromSessionModelState. If Grok supplies a malformed or partial_meta.modelState(for example{}), the helper evaluatesmodelState.availableModels.lengthand throws, turning an otherwise successful ACP startup into a failed provider health check instead of simply ignoring the fallback metadata.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/provider/Layers/GrokAdapter.ts around lines 298-309:
`initializeMeta.modelState.availableModels` is only checked with `Array.isArray`, then each entry is cast to `{ modelId: string; _meta?: unknown }` without validation. If the array contains a malformed entry such as `null`, `pick` calls `model.modelId` inside `find` and throws, failing session startup instead of ignoring unusable metadata. Validate each entry as a record with a string `modelId` before passing it to `pick`.
Also found in 1 other location(s):
- apps/server/src/provider/Layers/GrokProvider.ts:285 -- `initializeMeta.modelState` is unchecked metadata but is cast directly to `SessionModelState` and passed to `buildGrokDiscoveredModelsFromSessionModelState`. If Grok supplies a malformed or partial `_meta.modelState` (for example `{}`), the helper evaluates `modelState.availableModels.length` and throws, turning an otherwise successful ACP startup into a failed provider health check instead of simply ignoring the fallback metadata.
| ).flatMap((command) => { | ||
| const name = typeof command.name === "string" ? command.name.trim() : ""; |
There was a problem hiding this comment.
🟠 High Layers/GrokProvider.ts:296
When initializeMeta.availableCommands contains null or a primitive element (e.g. ["help", null, 42]), the flatMap callback accesses command.name without first checking that command is an object, which throws a TypeError. This crashes the entire Grok ACP discovery and causes the provider health check to fail instead of skipping the malformed command. Add a guard like typeof command !== "object" || command === null before accessing command.name.
| ).flatMap((command) => { | |
| const name = typeof command.name === "string" ? command.name.trim() : ""; | |
| ).flatMap((command) => { | |
| if (typeof command !== "object" || command === null) return []; | |
| const name = typeof command.name === "string" ? command.name.trim() : ""; |
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/provider/Layers/GrokProvider.ts around lines 296-297:
When `initializeMeta.availableCommands` contains `null` or a primitive element (e.g. `["help", null, 42]`), the `flatMap` callback accesses `command.name` without first checking that `command` is an object, which throws a `TypeError`. This crashes the entire Grok ACP discovery and causes the provider health check to fail instead of skipping the malformed command. Add a guard like `typeof command !== "object" || command === null` before accessing `command.name`.
| if (!models || models.length === 0) return undefined; | ||
| const preferredMatch = preferred | ||
| ? models.find((model) => model.modelId === preferred) | ||
| : undefined; | ||
| return preferredMatch?._meta ?? models[0]?._meta; |
There was a problem hiding this comment.
🟡 Medium Layers/GrokAdapter.ts:286
pick falls back to models[0]._meta when the preferred model is found but has no _meta field, so it returns a different model's metadata. This causes the session to be initialized with the wrong model's totalContextTokens and reasoningEffort, producing an incorrect context window limit and reasoning-effort state. The fallback to models[0]?._meta should only apply when no preferred model matches, not when the matched model lacks _meta.
| if (!models || models.length === 0) return undefined; | |
| const preferredMatch = preferred | |
| ? models.find((model) => model.modelId === preferred) | |
| : undefined; | |
| return preferredMatch?._meta ?? models[0]?._meta; | |
| if (!models || models.length === 0) return undefined; | |
| if (preferred) { | |
| const preferredMatch = models.find((model) => model.modelId === preferred); | |
| if (preferredMatch) return preferredMatch._meta; | |
| } | |
| return models[0]?._meta; |
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/provider/Layers/GrokAdapter.ts around lines 286-290:
`pick` falls back to `models[0]._meta` when the preferred model is found but has no `_meta` field, so it returns a *different* model's metadata. This causes the session to be initialized with the wrong model's `totalContextTokens` and `reasoningEffort`, producing an incorrect context window limit and reasoning-effort state. The fallback to `models[0]?._meta` should only apply when no preferred model matches, not when the matched model lacks `_meta`.
| env: processEnv, | ||
| }); | ||
|
|
||
| const commandCatalogRef = yield* Ref.make({ |
There was a problem hiding this comment.
🟡 Medium Drivers/GrokDriver.ts:113
mergeCommandCatalog treats an empty live catalog as "not initialized" and falls back to the previous snapshot's arrays. When onAvailableCommands receives an empty available_commands_update (i.e., the provider removed all commands), the catalog ref is correctly cleared to empty arrays, but mergeCommandCatalog sees length === 0 and keeps the stale slashCommands/skills from the last health probe. Removed commands and skills therefore remain advertised to clients indefinitely instead of being cleared.
The length > 0 guard conflates "no live data yet" with "live data says empty." Track whether the live catalog has been initialized with a separate boolean flag (or Option), then use the ref arrays unconditionally once initialized.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/provider/Drivers/GrokDriver.ts around line 113:
`mergeCommandCatalog` treats an empty live catalog as "not initialized" and falls back to the previous snapshot's arrays. When `onAvailableCommands` receives an empty `available_commands_update` (i.e., the provider removed all commands), the catalog ref is correctly cleared to empty arrays, but `mergeCommandCatalog` sees `length === 0` and keeps the stale `slashCommands`/`skills` from the last health probe. Removed commands and skills therefore remain advertised to clients indefinitely instead of being cleared.
The `length > 0` guard conflates "no live data yet" with "live data says empty." Track whether the live catalog has been initialized with a separate boolean flag (or `Option`), then use the ref arrays unconditionally once initialized.
| const trimmed = input.text?.trim(); | ||
| if (!trimmed) { | ||
| return trimmed; | ||
| } | ||
| if (input.interactionMode === "plan") { | ||
| if (/^\/plan(?:\s|$)/i.test(trimmed)) { | ||
| return trimmed; | ||
| } | ||
| return `/plan ${trimmed}`; | ||
| } | ||
| return trimmed; |
There was a problem hiding this comment.
🟠 High acp/GrokAcpSupport.ts:96
When interactionMode is "plan" and the user's text is empty (e.g. an attachment-only prompt), applyGrokPlanModeToPromptText returns an empty string instead of "/plan", so the request runs in default mode despite the caller requesting plan mode. The early return at line 97-99 skips the plan-mode branch entirely for blank text. Consider returning "/plan" when interactionMode is "plan" and the trimmed text is empty.
const trimmed = input.text?.trim();
- if (!trimmed) {
- return trimmed;
- }
- if (input.interactionMode === "plan") {
+ if (input.interactionMode === "plan") {
+ if (!trimmed) {
+ return "/plan";
+ }
if (/^\/plan(?:\s|$)/i.test(trimmed)) {
return trimmed;
}
return `/plan ${trimmed}`;
}
+ if (!trimmed) {
+ return trimmed;
+ }
return trimmed;🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/provider/acp/GrokAcpSupport.ts around lines 96-106:
When `interactionMode` is `"plan"` and the user's text is empty (e.g. an attachment-only prompt), `applyGrokPlanModeToPromptText` returns an empty string instead of `"/plan"`, so the request runs in default mode despite the caller requesting plan mode. The early return at line 97-99 skips the plan-mode branch entirely for blank text. Consider returning `"/plan"` when `interactionMode` is `"plan"` and the trimmed text is empty.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| yield* offerGrokPromptTokenUsage(live, prepared.turnId, promptResult._meta); | ||
| }), | ||
| ).pipe(Effect.catch(() => Effect.void)); | ||
| } |
There was a problem hiding this comment.
Duplicate Grok token usage events
Medium Severity
Successful session/prompt RPCs can emit duplicate thread.token-usage.updated events. Token usage is published once when the RPC returns and again in the Effect.ensuring block, leading to double-counting in the context meter and activity timeline.
Reviewed by Cursor Bugbot for commit 0c02a19. Configure here.
| threadId: ctx.threadId, | ||
| turnId: notificationTurnId, | ||
| usedTokens: event.usage.used, | ||
| ...(maxTokens !== undefined ? { maxTokens } : {}), |
There was a problem hiding this comment.
Usage updates dropped after turn
Medium Severity
New UsageUpdated handling runs only after a guard that requires ctx.activeTurnId. When the turn is settled early (for example via xAI prompt_complete) and activeTurnId is cleared before a late usage_update notification arrives, that usage is discarded even though prompt _meta may not have carried complete token fields.
Reviewed by Cursor Bugbot for commit 0c02a19. Configure here.
ApprovabilityVerdict: Needs human review 5 blocking correctness issues found. This PR introduces significant new feature functionality (plan mode, reasoning effort selection, exit plan approval workflows) with ~1900 new lines across multiple files. Multiple unresolved High severity review comments identify potential crashes from malformed metadata and incorrect plan mode behavior for empty prompts. You can customize Macroscope's approvability policy. Learn more. |
|
Review findings from Bugbot/Macroscope for this slice are fixed on the Zoen product tip ( |
Shared AcpRuntimeModel/CoreRuntimeEvents/SessionRuntime and xAI extension hooks used by the Grok adapter (and related fixtures).
Grok driver/adapter/provider core: slash catalog, process-scoped effort, usage meter, auth surface, set_model. Plan toggle and multi-agent task mapping land in later stack layers.
0c02a19 to
2447738
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.
There are 4 total unresolved issues (including 2 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 2447738. Configure here.
| }): string | undefined { | ||
| const trimmed = input.text?.trim(); | ||
| if (!trimmed) { | ||
| return trimmed; |
There was a problem hiding this comment.
Plan mode skips empty prompts
High Severity
When the composer is in Plan mode and the prompt text is empty or whitespace, applyGrokPlanModeToPromptText returns early. This prevents the /plan prefix from being added. As GrokAdapter only includes text for truthy results, attachment-only messages sent in Plan mode reach Grok without the /plan command, causing plan entry to fail.
Reviewed by Cursor Bugbot for commit 2447738. Configure here.
| runtimeMode: restartDecision.runtimeMode, | ||
| ...(restartDecision.resumeCursor ? { resumeCursor: restartDecision.resumeCursor } : {}), | ||
| ...(input.modelSelection ? { modelSelection: input.modelSelection } : {}), | ||
| }); |
There was a problem hiding this comment.
Effort restart emits session exited
Medium Severity
Changing Grok reasoning effort mid-sendTurn calls stopSessionInternal, which publishes session.exited and marks the orchestration session stopped, then startSession publishes session.started again—all before the turn’s turn.started for that send completes.
Reviewed by Cursor Bugbot for commit 2447738. Configure here.
1 participant
What Changed
Map T3 Plan/Build interaction mode onto Grok's
/planslash command on send, and show the composer interaction-mode toggle for Grok (live ACP does not advertise session modes).Stacked on #5423. Fixes #5419
Why
Grok Build enters plan mode via
/plantext, not ACP session modes. Without this mapping, the Plan toggle is a no-op on Grok threads. Complementary to #5409 (plan surface: exit_plan_mode + proposed-plan cards); this PR is plan entry only.UI Changes
Composer Plan/Build toggle becomes available for Grok (existing control; no new chrome). Screenshot pending when UI PR is rebased clean.
Checklist
Known review findings
/plan(should send/planalone)Test plan
Model: grok-4.5 (Grok Build)
Note
Medium Risk
Large changes to Grok turn/session lifecycle (process restarts, prompt settlement, catalog pubsub); no auth or billing, but regressions could affect live Grok threads and the context meter.
Overview
Plan entry (PR focus): Grok no longer relies on ACP session modes for Plan/Build—the provider snapshot enables the composer interaction-mode toggle, and
sendTurnmapsinteractionMode: "plan"to a/plan-prefixed prompt (without double-prefixing). Empty or attachment-only plan sends still skip/planalone (known gap).Broader Grok ACP changes in the diff: The adapter and provider layer are aligned with live Grok 0.2.x wire behavior: CLI spawn flags for model,
--reasoning-effort, and--always-approve; process restart + session resume when reasoning effort changes mid-thread (blocked while a turn is in flight). Token usage is emitted fromusage_updateand prompt result_metafor the context meter;session_info_updatetitles becomethread.metadata.updated; initialize/available_commands_updatefeed slash commands and skills into the provider snapshot (with live snapshot stream updates fromGrokDriver). xAIexit_plan_modeis handled as plan approval withturn.proposed.completed. ACP parsing adds thought chunks, usage, commands, config, and session-info updates; the mock agent and tests cover these paths.Reviewed by Cursor Bugbot for commit 2447738. Bugbot is set up for automated code reviews on this repo. Configure here.