Add Grok CLI connect support#1015
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (15)
📝 WalkthroughWalkthroughThis PR adds comprehensive support for Grok (xAI) as a supported CLI provider throughout the relay codebase. Provider aliases map ChangesGrok/xAI Provider Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for the Grok CLI and xAI provider across the broker, CLI, cloud, and SDK packages, including authentication, MCP configuration, and path resolution. The review feedback identifies two key issues in the broker's MCP setup: a potential deadlock risk caused by piping stderr without reading it, and a blocking synchronous process call on an async thread that should be converted to use Tokio's async command execution.
| let mut mcp_cmd = Command::new(&exe); | ||
| mcp_cmd.args(grok_mcp_add_args( | ||
| api_key, | ||
| base_url, | ||
| agent_name, | ||
| agent_token, | ||
| workspaces_json, | ||
| default_workspace, | ||
| )); | ||
| mcp_cmd | ||
| .stdin(Stdio::null()) | ||
| .stdout(Stdio::null()) | ||
| .stderr(Stdio::piped()); | ||
|
|
||
| match mcp_cmd.spawn() { | ||
| Ok(mut child) => match tokio::time::timeout(Duration::from_secs(15), child.wait()).await { | ||
| Ok(Ok(status)) if !status.success() => { | ||
| anyhow::bail!( | ||
| "failed to configure relaycast MCP for {cli}: `{cli} mcp add` exited with code {:?}. \ | ||
| Please configure the relaycast MCP server manually:\n {manual_cmd}", | ||
| status.code() | ||
| ); | ||
| } | ||
| Ok(Err(error)) => { | ||
| anyhow::bail!( | ||
| "failed to configure relaycast MCP for {cli}: {error}. \ | ||
| Please configure the relaycast MCP server manually:\n {manual_cmd}" | ||
| ); | ||
| } | ||
| Err(_) => { | ||
| let _ = child.kill().await; | ||
| anyhow::bail!( | ||
| "failed to configure relaycast MCP for {cli}: `{cli} mcp add` timed out after 15s. \ | ||
| Please configure the relaycast MCP server manually:\n {manual_cmd}" | ||
| ); | ||
| } | ||
| _ => {} | ||
| }, | ||
| Err(error) => { | ||
| anyhow::bail!( | ||
| "failed to configure relaycast MCP for {cli}: could not run `{cli} mcp add`: {error}. \ | ||
| Please configure the relaycast MCP server manually:\n {manual_cmd}" | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Piping stderr without reading it introduces a deadlock risk if the child process writes more than the OS pipe buffer limit (typically 64KB), causing it to block indefinitely.
To prevent this and provide better error reporting, we can use child.wait_with_output() to safely read the output into memory, and include the captured stderr in the error message when the command fails. Additionally, configuring .kill_on_drop(true) on the command ensures the child process is automatically cleaned up if the timeout expires.
let mut mcp_cmd = Command::new(&exe);
mcp_cmd.kill_on_drop(true);
mcp_cmd.args(grok_mcp_add_args(
api_key,
base_url,
agent_name,
agent_token,
workspaces_json,
default_workspace,
));
mcp_cmd
.stdin(Stdio::null())
.stdout(Stdio::null())
.stderr(Stdio::piped());
match mcp_cmd.spawn() {
Ok(child) => match tokio::time::timeout(Duration::from_secs(15), child.wait_with_output()).await {
Ok(Ok(output)) if !output.status.success() => {
let stderr = String::from_utf8_lossy(&output.stderr);
anyhow::bail!(
"failed to configure relaycast MCP for {cli}: `{cli} mcp add` exited with code {:?}.\nStderr: {stderr}\n\
Please configure the relaycast MCP server manually:\n {manual_cmd}",
output.status.code()
);
}
Ok(Err(error)) => {
anyhow::bail!(
"failed to configure relaycast MCP for {cli}: {error}. \
Please configure the relaycast MCP server manually:\n {manual_cmd}"
);
}
Err(_) => {
anyhow::bail!(
"failed to configure relaycast MCP for {cli}: `{cli} mcp add` timed out after 15s. \
Please configure the relaycast MCP server manually:\n {manual_cmd}"
);
}
_ => {}
},
Err(error) => {
anyhow::bail!(
"failed to configure relaycast MCP for {cli}: could not run `{cli} mcp add`: {error}. \
Please configure the relaycast MCP server manually:\n {manual_cmd}"
);
}
}| let _ = std::process::Command::new(&exe) | ||
| .args(["mcp", "remove", "relaycast"]) | ||
| .stdin(Stdio::null()) | ||
| .stdout(Stdio::null()) | ||
| .stderr(Stdio::null()) | ||
| .spawn() | ||
| .and_then(|mut child| child.wait()); |
There was a problem hiding this comment.
In an asynchronous context, spawning a synchronous process and blocking the thread with std::process::Command and .wait() can block the Tokio executor thread. Since tokio::process::Command is already imported as Command in this file, we should use it to perform the mcp remove operation asynchronously and non-blockingly.
| let _ = std::process::Command::new(&exe) | |
| .args(["mcp", "remove", "relaycast"]) | |
| .stdin(Stdio::null()) | |
| .stdout(Stdio::null()) | |
| .stderr(Stdio::null()) | |
| .spawn() | |
| .and_then(|mut child| child.wait()); | |
| let _ = Command::new(&exe) | |
| .args(["mcp", "remove", "relaycast"]) | |
| .stdin(Stdio::null()) | |
| .stdout(Stdio::null()) | |
| .stderr(Stdio::null()) | |
| .status() | |
| .await; |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 15 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="crates/broker/src/snippets.rs">
<violation number="1" location="crates/broker/src/snippets.rs:1213">
P2: `configure_grok_mcp` duplicates the subprocess orchestration/error-handling logic already implemented in `configure_gemini_droid_mcp`, which increases maintenance drift risk. Consider extracting a shared helper for "remove existing MCP + run mcp add with timeout + standardized error mapping".</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| args | ||
| } | ||
|
|
||
| async fn configure_grok_mcp( |
There was a problem hiding this comment.
P2: configure_grok_mcp duplicates the subprocess orchestration/error-handling logic already implemented in configure_gemini_droid_mcp, which increases maintenance drift risk. Consider extracting a shared helper for "remove existing MCP + run mcp add with timeout + standardized error mapping".
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/broker/src/snippets.rs, line 1213:
<comment>`configure_grok_mcp` duplicates the subprocess orchestration/error-handling logic already implemented in `configure_gemini_droid_mcp`, which increases maintenance drift risk. Consider extracting a shared helper for "remove existing MCP + run mcp add with timeout + standardized error mapping".</comment>
<file context>
@@ -1148,6 +1160,127 @@ async fn configure_gemini_droid_mcp(
+ args
+}
+
+async fn configure_grok_mcp(
+ cli: &str,
+ api_key: Option<&str>,
</file context>
1 participant
Summary
grok mcp addwith Relay env vars and track~/.grok/config.tomlside effectsTests
npm --prefix packages/config test -- cli-auth-config.test.tsnpm test -- --run packages/cloud/src/connect.test.tsnpm test -- --run packages/cli/src/cli/commands/cloud.test.tscargo test -p agent-relay-broker grok_mcp_add_args_use_equals_form_for_dash_argsnpm run build:config && npm --prefix packages/cloud run buildnpm --prefix packages/github-primitive run build && npm --prefix packages/slack-primitive run build && npm run build:utils && npm run build:trajectory && npm run build:policy && npm run build:hooks && npm run build:memory && npm run build:telemetry && npm run build:sdk && cd packages/cli && npx tsc --noEmit