feat: add Grok CLI as a new agent provider#57
Merged
Conversation
Adds xAI's Grok CLI ("Grok Build", https://x.ai/cli) as a first-class
agent provider alongside Claude, Codex, and Gemini.
Backend:
- New GrokProvider implementing the CLIProvider strategy interface
(electron/providers/grok-provider.ts), registered in the provider
registry. Commands use the official `grok` binary: headless `-p`,
`-m <model>`, and `--output-format streaming-json`. MCP follows the
`grok mcp add` convention with a JSON settings.json fallback; skills,
scheduled scripts, and --add-dir mirror the existing providers.
- 'grok' added to the AgentProvider union and CLIPaths (electron +
frontend types), CLI path auto-detection, and the agent PATH builder.
Frontend:
- Provider selector, model picker (grok-4 / grok-4-fast /
grok-code-fast-1), provider badge (angular-X Grok mark), CLI Paths
settings input, and install-detection all wired for Grok.
Tests:
- 14 new GrokProvider unit tests; full suite green (882 passing).
Notes: model IDs and MCP/hook config shapes track the Grok Build public
beta and are isolated to the provider for easy updates. Plan Mode is the
CLI's default safety gate, so auto/bypass emit no extra flag yet.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@ajdriggs is attempting to deploy a commit to the Charlie Rabiller's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds Grok as a supported provider across shared types, CLI path handling, provider execution, MCP configuration, and several UI surfaces. It also updates default settings, memory-path handling, and tests for Grok-specific behavior. ChangesGrok provider support
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
src/components/Settings/CLIPathsSection.tsx (1)
46-46: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant
DetectedPathscast.
cliPaths.detect()now typesgrokin its return shape, so(paths as DetectedPaths).grokis unnecessary and can usepaths.grokdirectly (same applies to the pre-existingpicast on Line 48). Keeping the cast suppresses future type-mismatch detection on this object.♻️ Proposed cleanup
- if (!updatedPaths.grok && (paths as DetectedPaths).grok) updatedPaths.grok = (paths as DetectedPaths).grok; + if (!updatedPaths.grok && paths.grok) updatedPaths.grok = paths.grok;🤖 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 `@src/components/Settings/CLIPathsSection.tsx` at line 46, The CLI paths handling in CLIPathsSection.tsx still uses redundant DetectedPaths casts when copying optional values into updatedPaths. Update the logic in the CLI paths merge block to read grok directly from paths.grok, and remove the same unnecessary cast for pi as well, so the existing cliPaths.detect() return typing is used instead of suppressing type checks.__tests__/electron/providers/grok-provider.test.ts (1)
120-128: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for prompt/model escaping edge cases.
The suite doesn't exercise prompts/models containing single quotes or shell metacharacters, which is exactly where the
buildScheduledScriptandbuildOneShotCommandissues surface. A test asserting that a prompt with a'is properly escaped would prevent regressions once those are fixed.🤖 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 `@__tests__/electron/providers/grok-provider.test.ts` around lines 120 - 128, Add test coverage in grok-provider.test.ts for escaping edge cases in buildOneShotCommand and buildScheduledScript. Create assertions around Provider.buildOneShotCommand and the scheduled script path that use prompts and model values containing single quotes and shell metacharacters, and verify the generated command/script safely escapes them rather than interpolating raw text. Keep the existing buildOneShotCommand test and extend it with inputs that would have exposed the shell-escaping bug.
🤖 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 `@electron/providers/grok-provider.ts`:
- Around line 113-126: `buildOneShotCommand` is appending `params.model`
directly into the shell command without the same validation and quoting used by
`buildInteractiveCommand`. Add the allow-list validation for `params.model`
before interpolation, and wrap the model value in shell-safe quotes/escaping
when building the `-m` argument. Update the existing `buildOneShotCommand` test
expectations to match the quoted, validated form instead of the current unquoted
output.
- Around line 286-308: The generated bash in buildScheduledScript interpolates
params.prompt directly inside single quotes, so a quote in the prompt can break
the script or enable injection. Apply the same escaping used by
buildScheduledCommand before inserting the prompt into the
"${params.binaryPath}" invocation, and keep the shell quoting safe in
buildScheduledScript. Also review the other interpolated values in this script
(projectPath, logPath, binaryPath, homeDir) to ensure they are safely escaped or
quoted consistently.
- Around line 159-195: Replace the shell-based `execSync` usage in
`GrokProvider.registerMcpServer` (and the corresponding `removeMcpServer` path)
with `execFileSync` so `name`, `command`, and `args` are passed as structured
arguments instead of interpolated into a shell string. Keep the Grok CLI
positional order intact (`grok mcp add <server-name> -- <command> [args...]`) by
building the argv array explicitly, and preserve the existing native-CLI-first,
settings.json fallback behavior.
In `@src/components/NewChatModal/StepModel.tsx`:
- Line 158: The Grok selection highlight is using a dynamic background utility
that Tailwind does not emit, so the active state stays invisible. Update the
styling used by the selection logic in StepModel to rely on a statically
referenced utility, or safelist bg-foreground/10 in the Tailwind configuration
so the class is generated at build time. Keep the fix aligned with the Grok
provider entry and the active-selection class computation in
NewChatModal/StepModel.
---
Nitpick comments:
In `@__tests__/electron/providers/grok-provider.test.ts`:
- Around line 120-128: Add test coverage in grok-provider.test.ts for escaping
edge cases in buildOneShotCommand and buildScheduledScript. Create assertions
around Provider.buildOneShotCommand and the scheduled script path that use
prompts and model values containing single quotes and shell metacharacters, and
verify the generated command/script safely escapes them rather than
interpolating raw text. Keep the existing buildOneShotCommand test and extend it
with inputs that would have exposed the shell-escaping bug.
In `@src/components/Settings/CLIPathsSection.tsx`:
- Line 46: The CLI paths handling in CLIPathsSection.tsx still uses redundant
DetectedPaths casts when copying optional values into updatedPaths. Update the
logic in the CLI paths merge block to read grok directly from paths.grok, and
remove the same unnecessary cast for pi as well, so the existing
cliPaths.detect() return typing is used instead of suppressing type checks.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 006f6617-5ddd-436a-9949-e8b5f49328e8
📒 Files selected for processing (18)
README.md__tests__/electron/handlers/scheduler-handlers.test.ts__tests__/electron/providers/grok-provider.test.tselectron/core/agent-manager.tselectron/handlers/cli-paths-handlers.tselectron/main.tselectron/providers/grok-provider.tselectron/providers/index.tselectron/types/index.tssrc/app/agents/constants.tssrc/components/NewChatModal/StepModel.tsxsrc/components/NewChatModal/StepTools.tsxsrc/components/NewChatModal/index.tsxsrc/components/ProviderBadge.tsxsrc/components/Settings/CLIPathsSection.tsxsrc/components/Settings/constants.tssrc/components/Settings/types.tssrc/types/electron.d.ts
Comment on lines
+113
to
+126
| buildOneShotCommand(params: OneShotCommandParams): string { | ||
| let command = `'${params.binaryPath.replace(/'/g, "'\\''")}'`; | ||
|
|
||
| command += ' -p'; | ||
|
|
||
| if (params.model) { | ||
| command += ` -m ${params.model}`; | ||
| } | ||
|
|
||
| const escaped = params.prompt.replace(/'/g, "'\\''"); | ||
| command += ` '${escaped}'`; | ||
|
|
||
| return command; | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
buildOneShotCommand interpolates model unquoted and unvalidated — shell injection / breakage.
Unlike buildInteractiveCommand (Lines 57-62), this path neither validates params.model against the allow-list regex nor wraps it in quotes. A model value containing spaces or shell metacharacters (e.g. grok-4; rm -rf ~) is injected directly into the command string.
🔒 Proposed fix: validate and quote, mirroring buildInteractiveCommand
command += ' -p';
if (params.model) {
- command += ` -m ${params.model}`;
+ if (!/^[a-zA-Z0-9._:/-]+$/.test(params.model)) {
+ throw new Error('Invalid model name');
+ }
+ command += ` -m '${params.model}'`;
}Note: the existing test at Lines 123-125 asserts the unquoted form (-m grok-4-fast) and would need updating.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| buildOneShotCommand(params: OneShotCommandParams): string { | |
| let command = `'${params.binaryPath.replace(/'/g, "'\\''")}'`; | |
| command += ' -p'; | |
| if (params.model) { | |
| command += ` -m ${params.model}`; | |
| } | |
| const escaped = params.prompt.replace(/'/g, "'\\''"); | |
| command += ` '${escaped}'`; | |
| return command; | |
| } | |
| buildOneShotCommand(params: OneShotCommandParams): string { | |
| let command = `'${params.binaryPath.replace(/'/g, "'\\''")}'`; | |
| command += ' -p'; | |
| if (params.model) { | |
| if (!/^[a-zA-Z0-9._:/-]+$/.test(params.model)) { | |
| throw new Error('Invalid model name'); | |
| } | |
| command += ` -m '${params.model}'`; | |
| } | |
| const escaped = params.prompt.replace(/'/g, "'\\''"); | |
| command += ` '${escaped}'`; | |
| return command; | |
| } |
🤖 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 `@electron/providers/grok-provider.ts` around lines 113 - 126,
`buildOneShotCommand` is appending `params.model` directly into the shell
command without the same validation and quoting used by
`buildInteractiveCommand`. Add the allow-list validation for `params.model`
before interpolation, and wrap the model value in shell-safe quotes/escaping
when building the `-m` argument. Update the existing `buildOneShotCommand` test
expectations to match the quoted, validated form instead of the current unquoted
output.
Verified the provider end-to-end against an installed Grok CLI (0.2.64) and corrected several assumptions that came from beta web docs: - MCP config lives in ~/.grok/config.toml (TOML), not settings.json — reworked register/remove/isRegistered to mirror the Codex provider's config.toml ([mcp_servers.<name>]) handling, and fixed the add syntax to `grok mcp add <name> <command> -- <args>`. - `-p/--single` consumes the prompt as its value: reordered one-shot to `grok -m <model> -p '<prompt>'` so the model flag isn't swallowed. - Grok has no `--add-dir`/multi-root flag: dropped it (secondary project and Obsidian vault mounting are not emitted); getAddDirFlag() now ''. - Wired real flags that DO exist: `--permission-mode auto|bypassPermissions` for auto/bypass modes (previously emitted nothing), `--effort <level>`, and `--debug` for verbose. Scheduled/autonomous runs now bypass the gate. - Default model is `grok-build` (per `grok models`), not grok-4; model picker updated accordingly. - CLI auto-detection now also scans ~/.grok/bin (the installer's default). - Fixed doc comments: auth is `grok login` / GROK_DEPLOYMENT_KEY. Tests expanded to 18 (886 total passing); electron tsc + lint clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@electron/providers/grok-provider.ts`:
- Around line 99-111: The command construction in grok-provider.ts is only
wrapping paths in double quotes, which still allows shell expansion; update the
command-building logic around the binaryPath-based command and the related path
interpolations (binaryPath, homeDir, binaryDir, projectPath, logPath) to use the
same single-quote escaping approach already used for prompt handling. Locate the
shell command assembly in the provider methods and ensure every scheduled
command/script path is safely escaped before interpolation, including the other
affected block referenced by the review.
- Around line 189-191: The TOML serialization in grok-provider’s section builder
is inserting raw command and args strings, which can break on backslashes or
embedded quotes. Update the logic in the section construction path to serialize
command and each args entry with a proper string escaper such as JSON.stringify
instead of manual double-quoting, and make escapeTomlKey use the same escaping
approach for quoted keys containing special characters.
- Around line 226-229: The path check in the MCP server lookup is too broad
because `content.includes(expectedServerPath)` can match a different server
section. In `grok-provider.ts`, update the logic around the
`headerRegex`/`expectedServerPath` check to extract only the
`[mcp_servers.<name>]` block for the matched `sectionKey`, then verify the path
within that scoped section instead of the full content. Keep the existing
escaping/header matching behavior, but make the final containment check operate
on the matched section text only.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: fb9645bf-60ea-408f-a867-473cfc3fd2fc
📒 Files selected for processing (4)
__tests__/electron/providers/grok-provider.test.tselectron/handlers/cli-paths-handlers.tselectron/providers/grok-provider.tssrc/components/NewChatModal/StepModel.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- electron/handlers/cli-paths-handlers.ts
`grok models` (authenticated) reports grok-composer-2.5-fast (default) and grok-build. Updated getModels() + the new-chat picker/default accordingly. Verified the provider's generated headless commands run live against grok 0.2.64: grok -m grok-composer-2.5-fast -p '<prompt>' → plain output grok --output-format streaming-json -p '<prompt>' → NDJSON stream Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replace the placeholder angular-X mark with xAI's official Grok icon (black rounded square + white glyph) in the provider badge and the new-chat provider picker. Verified live: a Grok agent created in the Dorothy dev app successfully built and ran a website end-to-end. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Several feature allowlists predated the Grok provider and silently excluded it where codex/gemini were included. Bring Grok to parity: - Custom MCP (Settings): add a Grok tab (McpSection.tsx) and a 'grok' case to the mcp-config handlers — Grok shares Codex's TOML schema, so read/write reuse the parameterized config.toml helpers pointed at ~/.grok/config.toml. - Automations: include 'grok' in the default-provider allowlist so a grok default no longer silently falls back to claude. - Memory: add ~/.grok/projects to PROVIDER_MEMORY_DIRS so the Memory page surfaces Grok project memory (verified the dir exists/structure). - Skills: add 'grok' to the install/link provider list; combined with GrokProvider.getSkillDirectories()/getInstalledSkills(), skills can now be linked to ~/.grok/skills and detected. Dorothy-managed MCP servers (kanban, vault, orchestrator) already auto-register with all providers via getAllProviders(), so those worked for Grok already. Usage and Plugins remain Claude-only by existing design (they also exclude codex/gemini) — out of scope here. 886 tests pass; electron tsc clean; no new lint. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Addresses the 6 open Critical/Major CodeRabbit findings on PR Charlie85270#57, all in electron/providers/grok-provider.ts: - Critical: buildScheduledScript interpolated params.prompt (and all paths) unescaped inside the generated bash — a single quote could break the script or inject commands. Now shell-quotes prompt, homeDir, binaryDir, projectPath, logPath, binaryPath via a shellQuote() helper. - Major: buildOneShotCommand injected params.model unquoted/unvalidated. Now validates against the allow-list regex and single-quotes, mirroring buildInteractiveCommand. - Major: registerMcpServer/removeMcpServer used execSync with interpolated name/command/args. Switched to execFileSync with an argv array (no shell). - Major: buildScheduledCommand only double-quoted binaryPath. Now uses shellQuote(). - Major: MCP TOML section built command/args with raw double quotes, breaking on Windows backslashes/embedded quotes. Now serialized via tomlString() (JSON.stringify); escapeTomlKey uses the same. - Major: isMcpServerRegistered matched expectedServerPath against the whole file, risking a false positive from another server section. Now scopes the containment check to the matched [mcp_servers.<name>] block. Tests updated for the execFileSync argv call and quoted -m form. 886 tests green; electron tsc --noEmit clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
electron/providers/grok-provider.ts (1)
166-173: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
grok mcp addargv order is wrong
grok mcp addexpects-- <command> [args...]; this call passescommandbefore the separator, so native registration can fail and only the fallback config path will run.Proposed fix
- execFileSync('grok', ['mcp', 'add', name, command, '--', ...args], { + execFileSync('grok', ['mcp', 'add', name, '--', command, ...args], { encoding: 'utf-8', stdio: 'pipe', });🤖 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 `@electron/providers/grok-provider.ts` around lines 166 - 173, The argv order in Grok MCP registration is incorrect, causing the native add path to fail and fall back unnecessarily. Update the execFileSync call in grok-provider.ts inside the Grok MCP add flow so the `grok mcp add` arguments match the expected `-- <command> [args...]` shape, with the separator placed before the command and its args. Keep the fix localized to the registration logic around the `execFileSync('grok', ...)` call so the native path succeeds before the fallback config path is used.
🤖 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.
Duplicate comments:
In `@electron/providers/grok-provider.ts`:
- Around line 166-173: The argv order in Grok MCP registration is incorrect,
causing the native add path to fail and fall back unnecessarily. Update the
execFileSync call in grok-provider.ts inside the Grok MCP add flow so the `grok
mcp add` arguments match the expected `-- <command> [args...]` shape, with the
separator placed before the command and its args. Keep the fix localized to the
registration logic around the `execFileSync('grok', ...)` call so the native
path succeeds before the fallback config path is used.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 97258f4d-ebbc-4c7f-828c-2036b56da78b
📒 Files selected for processing (2)
__tests__/electron/providers/grok-provider.test.tselectron/providers/grok-provider.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/electron/providers/grok-provider.test.ts
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
Adds xAI's Grok CLI ("Grok Build" — https://x.ai/cli) as a first-class agent provider, alongside Claude, Codex, and Gemini. Follows the established
CLIProviderstrategy pattern, so Grok agents flow through the same orchestration, scheduling, MCP, skills, and memory plumbing as the other providers.Verified end-to-end against an installed
grokbinary (v0.2.64) — every flag, thegrok mcp add/remove/listsyntax, the~/.grok/config.tomlMCP schema, and the model IDs were checked against the real CLI. A Grok agent created in the dev app successfully built and ran a website.What's included
Provider —
GrokProvider(electron/providers/grok-provider.ts), registered in the provider registry:grok [-m <model>] [--permission-mode auto|bypassPermissions] [--effort <lvl>] [--debug] "<prompt>"grok -p "<prompt>" [--output-format streaming-json]grok mcp add <name> <command> -- <args>with a~/.grok/config.toml([mcp_servers.<name>]) fallback — same TOML schema as Codex--permission-mode,--effort,--debugwired; no--add-dir(Grok has no multi-root flag)'grok'added to theAgentProviderunion +CLIPaths, CLI auto-detection (scans~/.grok/bin), and the agent PATH builderUI — provider selector + model picker (
grok-composer-2.5-fastdefault,grok-build), official Grok logo badge, CLI-path settings, install detection.Feature coverage
Traced how each Dorothy feature dispatches on the provider and brought Grok to parity with codex/gemini:
~/.grok/config.tomlgrokcase (reuses Codex TOML helpers)~/.grok/projectsadded to the scanned dirsgetSkillDirectories()→~/.grok/skillsTests
GrokProviderunit tests; full suite green at 886 passing. Electrontscclean; no new frontend type errors; no new lint.Notes for reviewers
getModels()returns the authenticated default set; the full per-account list comes fromgrok modelsand is isolated to one method.🤖 Generated with Claude Code
Summary by CodeRabbit