feat(provider): add official xAI Grok CLI support - #596
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #596 +/- ##
=======================================
Coverage ? 20.40%
=======================================
Files ? 139
Lines ? 18088
Branches ? 0
=======================================
Hits ? 3690
Misses ? 14398
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
fanhongy
left a comment
fanhongy
left a comment
There was a problem hiding this comment.
Summary
The provider registration, command construction, restrictions, MCP configuration, and focused tests are in place. I found two reproducible P2 defects in multi-turn status handling and restart cleanup.
Findings
P2: Long turns with the same elapsed-time marker can never complete
In src/cli_agent_orchestrator/providers/grok_cli.py:438, when the current query has fallen outside the 8,192-character status tail, turn_start is set to the completion marker itself. The fingerprint at lines 440-447 then contains only text such as Worked for 2.0s. If a later long turn reports the same elapsed duration, its otherwise valid completion is treated as the previous turn and get_status() returns PROCESSING indefinitely.
I reproduced this with two different queries and two different 9,000-character response streams, each ending in a structurally valid raw Worked for 2.0s marker and ready footer: the first returned completed, while the second returned processing. Because send_input() clears the rolling buffer and the status monitor consumes its processing-revert arm, handoff/inbox workflows can then wait until timeout despite Grok being visibly ready.
Preserve a current-turn discriminator before the query is evicted from the tail, or otherwise tie completion evidence to the dispatch/buffer generation instead of reducing the fingerprint to the elapsed-time marker. Add a regression test with two long, distinct turns sharing the same duration.
P2: Deleting a restored terminal leaves its private Grok home behind
src/cli_agent_orchestrator/providers/grok_cli.py:509 makes cleanup a no-op whenever _grok_home is None. That field is only assigned by _prepare_grok_home() during initialization. After cao-server restarts, ProviderManager reconstructs the provider from database metadata without running initialization, so the restored provider has _grok_home=None; if no provider has yet been reconstructed, cleanup_provider() is also a no-op. Deleting that terminal therefore leaves <CAO_HOME>/grok/terminals/<id>-<hash> permanently on disk.
I reproduced the lifecycle by preparing a home, discarding the provider object to simulate restart, constructing the restored provider, and calling cleanup(); home.exists() remained true. The retained directory contains config.toml and may contain MCP headers/environment values plus the live auth.json symlink, contradicting the documented cleanup behavior.
Make Grok cleanup reconstruct the deterministic path from terminal_id and ensure terminal deletion invokes that cleanup even when the in-memory provider map was lost. Add a restart/delete regression test.
Validation
- Read all acquisition artifacts and the complete 40-file diff at head
a7c09a4ae467a09bdbe3a64875beb1ec24d32bf0. - Ran focused provider, manager, tool-mapping, and terminal-service tests:
130 passed. - Ran
git diff --check: passed. - Ran two read-only Python reproductions for the findings above.
- Acquisition validation reported 340 focused Python tests, 175 web tests, and the production web build passing.
- Live Grok E2E was not run because
grokis not installed in the review environment.
gutosantos82
left a comment
gutosantos82
left a comment
There was a problem hiding this comment.
PR Review: #596 — feat(provider): add official xAI Grok CLI support
Summary
Adds the official xAI Grok Build CLI as a first-class grok_cli provider: registration across CLI/API/manager/web, an isolated per-terminal GROK_HOME with atomic 0600 MCP config and symlinked (never copied) auth, hard tool restrictions via native --deny rules that survive --always-approve, careful multi-turn TUI status detection with raw/rendered fixtures, and thorough docs. The implementation quality is well above the bar for a new provider — it follows the established claude_code/copilot_cli hard-enforcement pattern and the codex/antigravity auto-approve pattern, ships a 619-line unit suite plus a full e2e matrix, and the security-relevant file handling (atomic write, mode repair, cleanup on init failure) is unusually careful. Recommend approve; two questions below are worth an author response but neither is a demonstrated defect.
Important (should fix or answer)
- [security] src/cli_agent_orchestrator/utils/tool_mapping.py (grok_cli.execute_bash) —
execute_bashmaps to only["Bash"], while the claude_code mapping neededBash, BashOutput, KillShell, Task, Agent, Monitorafter live escapes through subagent/background-shell tools. Grok's subagent path is closed by unconditional--no-subagents(and CAO owns theconfig.tomlit could be re-enabled from), but please confirm Grok 1.0.0's full native tool inventory contains no other execution-capable surface (background-shell output/kill companions, monitor-style tools, or anything that can shell out) that would stay approvable under--always-approveon a restricted profile. The passing restricted-supervisor e2e covers theBashand write paths; it can't prove the inventory is exhaustively covered. Weighted up because this is the hard-enforcement security boundary. - [correctness] src/cli_agent_orchestrator/providers/grok_cli.py:get_status (stale-completion fingerprint guard) — the guard hashes the last-query→
Worked forslice and reports PROCESSING while the fingerprint is unchanged after a new dispatch. If two consecutive turns produce a byte-identical slice (same prompt, same response, same thought/work durations — plausible for short repeated orchestration prompts like a heartbeat "ok"), the second completion is indistinguishable from the stale frame and the turn wedges in PROCESSING until any differing frame arrives. The tests show this trade-off is deliberate and the window is narrow, but consider mixing a monotonic cue (e.g., completion-match count in the full buffer) into the guard, or documenting the failure mode and its recovery. Weighted up because it sits in provider status detection.
Nits (optional)
- [security] grok_cli.py:_build_grok_command — the full profile system prompt plus runtime skill catalog is passed as a single
--rulesargv element, visible to other local users viapsand bounded by ARG_MAX for very large skill catalogs. Antigravity does the same via-i, so there's precedent, but codex's file-based developer-instructions approach is the more robust pattern if Grok supports rules-from-file. - [correctness] grok_cli.py:_prepare_grok_home (auth symlink) — if a future Grok build refreshes tokens by atomically replacing
auth.json, the replace converts the symlink into a regular file inside the disposable home: the refreshed token is deleted at cleanup and the real~/.grok/auth.jsonkeeps the stale one. Works with current observed behavior (in-place write-through); worth a one-line comment so the assumption is explicit. - [correctness] grok_cli.py:initialize — an unauthenticated launch parks on the login picker (correctly classified WAITING_USER_ANSWER) and then surfaces as a generic "initialization timed out" error. Including a hint ("is Grok authenticated? run
grok login") in the timeout message would save operators a diagnosis step; docs do cover the prerequisite. - [consistency] test/e2e/test_allowed_tools.py — the added
Path(BASH_MARKER_FILE).unlink(missing_ok=True)cleanup in the shared helpers changes behavior for all providers' tests, not just Grok. It's a genuine leak fix and harmless, just slightly out of the PR's stated scope.
Tests
Excellent coverage for a provider PR. The 619-line unit suite exercises status detection across rendered and raw pipe-pane fixtures (idle, processing, completed, permission picker, login, telemetry banner, error, second turn, ANSI/CUP redraws), stale-marker ordering, the multi-turn fingerprint guard, message extraction with chrome/thought/timestamp stripping, command construction (flag set, model precedence, deny mapping, web kill-switch, empty allowlist), private-home isolation, atomic 0600 config writes with mode repair, auth symlinking (including custom GROK_HOME), idempotent/retryable cleanup, and async init success/timeout/cleanup paths. Registration is covered in test_constants.py, test_provider_manager_unit.py, test_terminal_service.py, API and launch tests, and the tool-mapping table in test_tool_mapping.py. The e2e matrix adds Grok classes to allowed-tools (restricted + unrestricted + a new read-only reviewer write-denial probe), assign, handoff, send_message, skills, and supervisor orchestration, all gated on a require_grok fixture that reuses auth by symlink without copying credentials. Fixtures are sanitized (login code is XXXX-XXXX; no PII observed).
Verification
Ran the focused suites in the PR worktree with the repo venv (Python 3.12):
- ✓ VERIFIED
test/providers/test_grok_cli_unit.py,test_provider_manager_unit.py,test/utils/test_tool_mapping.py,test/test_constants.py,test/services/test_terminal_service.py— 194 passed. - ✓ VERIFIED
test/api/test_api_endpoints.py,test/cli/commands/test_launch.py— 146 passed. - ✓ VERIFIED the PR body's "Focused Python suite: 340 passed" claim (194 + 146 = 340, matching exactly).
- ⁇ NOT VERIFIED: the Grok tmux e2e matrix (14/14 claimed) and live TUI marker behavior — the
grokbinary is not installed on this host; e2e tests skip viarequire_grok. Manual: install Grok Build 1.0.0, authenticate, thenuv run pytest -m e2e test/e2e/ -k grok -v. - ⁇ NOT VERIFIED: frontend tests (Node unavailable here, same as the author's environment); the web change is a one-element fallback-list addition with a matching test update — CI should exercise it.
Verdict
Approve with nits — a carefully engineered provider addition with strong isolation, hard enforcement, and verification; the two Important items are questions/hardening in sensitive paths rather than demonstrated defects.
|
|
||
| ## E2E Testing | ||
|
|
||
| The `data_analyst` and `report_generator` profiles from this directory are used in the E2E test suite to validate assign and handoff flows across all providers (codex, claude_code, kiro_cli, kimi_cli). |
There was a problem hiding this comment.
@thuanlm215 why are we crossing out the existing providers ?
There was a problem hiding this comment.
No intent to cross out any provider. That wording was corrected in the merge update at 4b19e7c; the README now says the examples validate the supported providers, including grok_cli. Thanks for catching it.
There was a problem hiding this comment.
@thuanlm215 thanks for the great work! I tested this provider against the official Grok Build 1.0.0 binary, not only the fixtures. I am requesting changes for five independently reproduced P2 issues.
The previously reported long-turn status wedge and restart cleanup leak are both real. I also reproduced three additional gaps: Grok workflows can still start native subagents in CAO-controlled mode, project-local configuration stops startup at an unhandled trust screen, and SSE MCP profiles are rewritten as ordinary HTTP profiles.
I checked the separate monitor concern as well: Grok reports that --deny Bash blocks the monitor tool, so I am not treating that as a defect. The focused Python suite (340 tests), web suite (175 tests), and production web build pass at this head. I could not run the account-backed assign/handoff E2E because no authorized Grok account was available.
| binary, | ||
| "--no-alt-screen", | ||
| "--always-approve", | ||
| "--no-subagents", |
There was a problem hiding this comment.
[P2] Make CAO-controlled mode cover workflow subagents
--no-subagents hides Grok's direct spawn_subagent tool, but it does not stop the workflow tool or /goal from starting Grok-native workers. With Grok 1.0.0 and these exact TUI flags, I had the model call an inline workflow containing one agent() step; the workflow completed and produced a separate Grok Build subagent model request. /goal also displayed Ran 1 subagent.
Those workers are invisible to CAO's terminal/group accounting and do not use CAO's role profiles or assign callbacks, so a supervisor can delegate work outside CAO even though this flag and the docs say that path is closed. Please make CAO-controlled mode actually disable workflow-backed workers. Keep native loops available through an explicit mode so /goal and provider-native workflows remain usable when the user chooses them, rather than silently mixing the two control models.
There was a problem hiding this comment.
Fixed in 3bcab70. Root cause: --no-subagents only covered the direct tool, not workflow-backed workers or /goal. CAO-controlled launch now also sets GROK_SUBAGENTS=0, GROK_WORKFLOWS=0, and GROK_GOAL=0; native behavior requires the typed per-profile grokNativeWorkflows: true opt-in, which sets them to 1 and omits --no-subagents. Regression tests cover default and opt-in command construction/profile validation. I also ran focused Grok 1.0.0 probes for the default and opt-in paths; the default did not start a native worker, while the explicit opt-in did. This is documented with the version caveat because these controls are not all exposed by grok --help.
| await asyncio.to_thread( | ||
| get_backend().send_keys, self.session_name, self.window_name, command | ||
| ) | ||
| if not await wait_until_status( |
There was a problem hiding this comment.
[P2] Handle Grok's directory-trust screen before waiting for ready
Every terminal gets a fresh private GROK_HOME, so it has no saved folder-trust decision. In a working directory containing only .mcp.json, official Grok 1.0.0 stops at Do you trust the contents of this directory? before it renders the composer. That screen matches neither WAITING_USER_PATTERN nor a ready state, so this wait runs until provider_init_timeout, then initialization deletes the terminal. I reproduced this in the real TUI.
Please handle this startup screen using an explicit safe policy and add a regression fixture. Simply pressing Yes unconditionally needs care because the process also runs with --always-approve, and trusting the folder enables repository MCP/hooks.
There was a problem hiding this comment.
Fixed in 3bcab70. Root cause: initialization waited only for ready/completed status, so the project directory-trust surface was treated as a generic timeout. The ready wait now inspects the status buffer first and raises an actionable ProviderError on the exact trust screen; initialization cleanup then removes the private home. CAO deliberately never answers Yes because that trusts repository-local MCP, LSP, and hooks under the terminal user. Added trust-screen fixture/regression coverage and the operator guidance in docs/grok-cli.md. Focused live Grok 1.0.0 trust-screen probing confirmed detection follows the safe fail-closed path.
| table = f"mcp_servers.{_toml_string(name)}" | ||
| lines.extend(["", f"[{table}]"]) | ||
| if config.get("url"): | ||
| lines.append(f"url = {_toml_string(config['url'])}") |
There was a problem hiding this comment.
[P2] Keep the SSE transport in generated MCP config
This URL branch drops the profile's type field. Grok 1.0.0 requires type = "sse" for an SSE server; its own grok mcp add --transport sse ... command writes that field. Without it, Grok treats the same URL as ordinary HTTP. I rendered an SSE profile with this code and confirmed with grok mcp list --json that the generated entry was no longer SSE. Preserve the transport field and add an SSE config test.
There was a problem hiding this comment.
Fixed in 3bcab70. Root cause: the URL MCP rendering branch emitted only url, silently relying on Grok HTTP defaulting and thereby changing an SSE profile. Generated TOML now preserves explicit type = "http" or type = "sse" and rejects unsupported URL transports rather than emitting a changed configuration. Regression coverage asserts both HTTP and SSE output.
| completion_match = completion_matches[-1] | ||
| query_matches = list(QUERY_PATTERN.finditer(tail[: completion_match.start()])) | ||
| turn_start = ( | ||
| query_matches[-1].start() if query_matches else completion_match.start() |
There was a problem hiding this comment.
[P2] Do not identify a long turn only by its elapsed-time text
Once a response is long enough to push its query outside this 8,192-character tail, turn_start becomes the completion marker itself. The fingerprint then contains only text such as Worked for 2.0s. A later, different long turn with the same reported duration is mistaken for the old completion and remains PROCESSING forever. I reproduced two different 9,000-character turns: the first returned COMPLETED, the second returned PROCESSING. Tie completion to the current dispatch/buffer generation instead, and add that two-turn regression case.
There was a problem hiding this comment.
Fixed in 3bcab70. Root cause: the stale guard used a fingerprint from the 8 KiB display tail; once the query was evicted, it degraded to the shared Worked for 2.0s marker. The provider now tracks a monotonic normalized-stream position across rolling-buffer overlap and combines it with full-transcript completion identity/current-turn activity. A retained stale completion remains PROCESSING, while a later identical marker at an advanced stream position completes. Regressions cover two >9 KiB distinct turns with equal durations, raw and rendered output, byte-identical consecutive turns, and 1 KiB rolling-buffer eviction. Focused regression suite passed locally (223 tests); full static validation also passed (318 passed, 10 skipped).
| def cleanup(self) -> None: | ||
| self._initialized = False | ||
| home = self._grok_home | ||
| if home is None: |
There was a problem hiding this comment.
[P2] Remove the private Grok home after a server restart
_grok_home exists only on the provider object that ran initialization. After cao-server restarts, deletion either finds no provider in the manager or reconstructs one with _grok_home = None, so this early return leaves the deterministic terminal home on disk. I reproduced prepare -> discard provider -> reconstruct -> cleanup, and the directory remained. The retained directory can contain generated MCP environment/header values and the live auth.json symlink. Derive the path from terminal_id during cleanup and make deletion call that cleanup even when the in-memory provider map was lost.
There was a problem hiding this comment.
Fixed across 3bcab70 and 6d9e392. Root cause: cleanup depended on the initialized provider object, which is absent after a server restart. Grok homes are now deterministically reconstructed from terminal id, and ProviderManager creates a cleanup-only Grok adapter from persisted terminal metadata when its in-memory map is empty. Cleanup validates the exact managed path (including symlinked-ancestor defenses), waits for exact-home residual Grok/MCP processes to stop, then removes the home. Terminal/flow teardown now kills the owning tmux process before provider cleanup to avoid updater recreation races. Regression coverage includes restart/delete, idempotency, retryable process cleanup, and symlink-escape handling; focused live cleanup probes and the focused unit suite passed.
|
@haofeif All five requested P2 fixes are now pushed in 3bcab70 and 6d9e392, with inline root-cause and test details on each thread. Validation now includes:
The full E2E run covered allowed-tools, assign, handoff, inbox messaging, skills, and supervisor orchestration. Could you please re-review when convenient? |
There was a problem hiding this comment.
Thanks for the thorough fixes @thuanlm215 . The native-workflow opt-in, trust-screen handling, SSE transport, and the original distinct long-turn case now behave as intended. The Linux restart-cleanup happy path also works.
I found two remaining P2 correctness gaps in the new state and cleanup logic: a fast repeated turn can still stay PROCESSING forever after the real buffer clear, and cleanup permanently leaks every private Grok home when Linux /proc is unavailable (including supported macOS). Both are independently reproduced in the inline comments.
I checked this exact head (6d9e392) against official Grok Build 1.0.0 and ran the focused provider/lifecycle/profile tests (124 passed). Account-backed Grok E2E was not available in this review environment.
|
Think we are getting very close. Very keen to get this PR merged |
|
@haofeif Thanks again for the careful review. I’ve addressed the two remaining P2s and pushed the fixes in |
fanhongy
left a comment
fanhongy
left a comment
There was a problem hiding this comment.
Summary
Reviewed head 78960b2f5b9da9206b8185bba5985bff2c684473 against the PR intent and base. The provider registration, permission mapping, isolated home setup, and retryable cleanup are broadly covered, but I found two P2 behavioral defects: raw status parsing can complete a turn on ordinary prose, and deferred cleanup is still reported as a successful delete by user-facing clients.
Findings
P2: Keep raw completion ordinals aligned with the status tail
src/cli_agent_orchestrator/providers/grok_cli.py:679
Raw structural completion ordinals are counted over the full output, while clean_counts restarts at zero inside the last 8 KiB tail. Once an earlier raw Worked for 2.0s marker has scrolled outside that tail, second-turn prose containing the same phrase is ordinal zero in the tail and incorrectly matches the old marker's structural ordinal. With a retained ready footer and a preceding processing marker, get_status() then returns COMPLETED even though the current turn has emitted no structural completion marker. This can make orchestration consume or act on an incomplete response. Preserve the full-buffer ordinal offset when scanning the tail (or associate raw structural markers with absolute normalized positions), and add a regression with an old marker outside _STATUS_TAIL_CHARS plus same-duration prose in the active turn.
P2: Propagate deferred cleanup as a failed delete
src/cli_agent_orchestrator/services/terminal_service.py:1717
The new cleanup contract returns False after killing the backend window but retaining the terminal row and Grok home for a required retry. DELETE /terminals/{id} exposes that as HTTP 200 with {"success": false}, but both dashboard delete handlers ignore the payload and show success. The session path similarly records an error and omits the session from deleted (src/cli_agent_orchestrator/services/session_service.py:175), while the API still returns "success": true; the web UI and cao shutdown therefore also report deletion complete. A protected/orphaned process triggers this path, leaving the retry row and private home behind without telling the user to retry. Make deferred cleanup a non-success API result, or require every client to inspect success/errors, preserve the retry identifier, and add terminal/session client tests for the deferred case.
Validation
366 passed: focused provider, manager, lifecycle, status monitor, MCP cleanup, and tool-mapping tests.2 passed: the two flow-service tests noted as failures in the acquisition context, rerun directly under the repository's normal test contract.175 passed: frontend tests (with expected jsdom canvas/error-boundary diagnostics).- Grok provider mypy and Markdown link validation passed.
- A direct parser reproduction returned
completedfor a second turn containing no current structural completion marker. - Live Grok E2E was not run because
grokis not installed in the review environment.
An evicted structural Worked-for marker was still ordinal 0 in the full buffer, while the 8 KiB tail restarted that count. Same-duration prose in the active turn then matched the old marker and returned COMPLETED.
DELETE /terminals and DELETE /sessions now return HTTP 409 when cleanup must be retried, and the dashboard, shutdown CLI, and MCP client inspect that result instead of reporting a successful delete.
|
@fanhongy thanks for catching these — both made sense. I pushed two follow-ups on this:
Would you mind taking another look when you have a chance? |
|
Sure. Thanks for the PR, LGTM, I have approved, pending on @haofeif |
haofeif
left a comment
haofeif
left a comment
There was a problem hiding this comment.
LGTM. Thank you @thuanlm215 for your great contribution!
|
@thuanlm215 can you please help to fix the CI errors ? |
The deferred-cleanup 409 check used `session_name not in deleted`. A bool or other non-sequence mock then raised TypeError and became HTTP 500.
|
Fixed the CI failure in |
5 participants
Closes #578.
Adds the official xAI Grok Build CLI as a first-class CAO provider.
What changed
grok_cliprovider registration across the CLI, API, provider manager, workspace access, and web provider selector.GROK_HOME, auth symlinking, atomic MCP TOML configuration, CAO terminal identity, profile/model/rules support, and cleanup.--denyrules and--disable-web-searchwhenweb_fetchis unavailable.Orchestration verification
The Grok tmux E2E matrix passed 14/14 locally. It covers:
This exercises the
examples/assigntopology requested in #578: analysts complete their delegated work and communicate results back, while the supervisor coordinates and produces the report rather than doing the analyst work itself.Additional verification
git diff --check: passedScope
This PR targets the official xAI Grok CLI only, as agreed in #578. It does not add a provider-specific CI workflow or include the separate experimental Herdr work.