Repository navigation
feat(grok): scaffold xAI Grok CLI worker - #378
Conversation
Add the grok worker: the xAI Grok CLI exposed as iii functions, modeled on the codex worker but driving the headless streaming-json path (grok --print <prompt> --output-format streaming-json), not ACP. - grok::run/start/stop/status/sessions::list with typed request/response schemas; raw events mirrored verbatim onto grok::events, normalized AgentEvent frames onto agent::events. - Grok CLI arg surface: --print, --output-format streaming-json, --no-alt-screen, --no-auto-update, --model, --cwd, --always-approve, --add-dir, --session (resume). Auth via XAI_API_KEY. - iii runtime context prepended to the first prompt of a session (Grok has no developer-instructions flag); suppressed on resume. - Config: model / cwd / always_approve defaults, stream names, CLI path, optional base_url. - README, SKILL.md, iii-permissions, repo Modules row. The Grok streaming-json event schema is not formally published; the typed event model is provisional and lenient (unknown shapes pass through on grok::events, skipped on the translated stream), to be finalized against a real capture. fmt + clippy -D warnings clean, 29 tests pass.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a new Changesgrok iii Worker
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
skill-check — worker0 verified, 28 skipped (no docs/).
Four for four. Nicely done. |
Verified end-to-end against the live grok CLI (run, resume, status, and the
agent::events stream all confirmed on the bus). Corrected the scaffold's
guessed surface to the captured reality and removed dead codex carryover.
Flags (from `grok --help`, 0.2.77):
- prompt is `--single` (not `--print`); resume is `--resume` (not `--session`).
- Dropped the nonexistent `--no-auto-update` and `--add-dir`, and the
`additional_directories` request field with them.
Events (captured from `--output-format streaming-json`):
- Replaced the provisional codex thread/item model with Grok's actual schema:
`{type:"text",data}` deltas, `{type:"thought",data}` reasoning deltas
(reasoning models), `{type:"end",stopReason,sessionId,requestId}`, and
`{type:"error",message}`.
- Text deltas accumulate into the answer; thought deltas render as a leading
`thinking` block; one `message_complete` is emitted at `end`. `sessionId`
becomes `grok_thread_id` for `--resume`.
Dead codex code removed:
- `Usage` type and every `usage` field/response key — headless Grok output
carries no token usage.
- `map.rs` (codex item→function mapping) and its test.
- `toml` / `tempfile` direct deps, the output-schema temp-file path.
fmt + clippy -D warnings clean, 23 tests pass.
Captured from a live run against Grok CLI 0.2.77: a grok::run turn over the bus, the typed request schema from `grok::run --help`, and the iii-context discovery run where Grok queries engine::workers::list itself to report the live mesh.
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (3)
grok/skills/SKILL.md (1)
44-47: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClarify that
always_approve: trueis the default, not mandatory.The current phrasing ("headless turns run with
always_approve: trueso they don't block") could be read as always-on. Consider adding "by default" to match the README's clearer wording: "set it tofalseto let the CLI's approval policy gate tool execution."🤖 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 `@grok/skills/SKILL.md` around lines 44 - 47, Update the wording in SKILL.md around the approval policy to make it clear that `always_approve: true` is the default behavior for headless turns, not a requirement. Adjust the description near the tool/command execution guidance so it reads as “by default” and still points users to set `always_approve: false` when they want the CLI’s approval policy to gate execution.grok/tests/args.rs (1)
90-95: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise the config-default branch too.
This test never reaches
Config::default()fallback because it always supplies prior values, so half of the precedence contract ingrok/src/grok/args.rs:63-90can regress unnoticed.Proposed test extension
#[test] fn resolve_falls_back_to_prior_then_config_defaults() { let req = RunRequest::default(); let cfg = Config::default(); let o = resolve(&req, &cfg, Some("prior-model"), Some("/prior"), None); assert_eq!(o.model, "prior-model"); assert_eq!(o.cwd, "/prior"); + + let from_cfg = resolve(&req, &cfg, None, None, None); + assert_eq!(from_cfg.model, cfg.defaults.model); + assert_eq!(from_cfg.cwd, cfg.defaults.cwd); }🤖 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 `@grok/tests/args.rs` around lines 90 - 95, The test only covers the prior-value fallback path and misses the Config::default() branch in resolve, so extend resolve_falls_back_to_prior_then_config_defaults to also assert behavior when no prior model/cwd is provided and resolution falls through to the config defaults. Use the existing resolve function plus Config::default() and RunRequest::default() in grok/tests/args.rs to add a case that verifies the default precedence contract still works when prior values are None.grok/src/configuration.rs (1)
155-157: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winThe retry delay is linear, not exponential.
This schedules 250ms, 500ms, 750ms. If the intent is exponential backoff for
configuration::*outages, this will retry more aggressively than described and with less spreading under contention.Suggested fix
last_err = e.to_string(); if attempt < CONFIG_RETRIES { - tokio::time::sleep(Duration::from_millis(250 * u64::from(attempt))).await; + let backoff_ms = 250_u64.saturating_mul(1_u64 << (attempt - 1)); + tokio::time::sleep(Duration::from_millis(backoff_ms)).await; } } }🤖 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 `@grok/src/configuration.rs` around lines 155 - 157, The retry sleep in the configuration retry loop is using a linear delay instead of exponential backoff. Update the retry timing in the configuration loading logic (the block around CONFIG_RETRIES in configuration::*) so each successive retry waits exponentially longer rather than 250ms, 500ms, 750ms; keep the retry count and surrounding control flow the same, but adjust the Duration calculation to use an exponential growth pattern.
🤖 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 `@grok/iii.worker.yaml`:
- Line 7: The manifest description is advertising the wrong exported function
name for the sessions list surface. Update the description in the iii worker
manifest so it matches the actual registered symbol, `grok::sessions::list`,
alongside the other `grok::run/start/stop/status` entries, to keep the
registry/docs aligned with the worker implementation.
In `@grok/README.md`:
- Line 118: The runtime-capabilities sentence in the README uses the wrong
engine listing command; update the `iii trigger engine::functions::list`
reference to `engine::workers::list` so it matches the worker-discovery flow
described alongside `directory::registry::workers::list/info` and `worker::add`.
Keep the surrounding guidance the same, just align the command name with the
worker context used elsewhere in the document.
- Line 3: The README introduction references the wrong headless CLI flag for the
`grok::run` flow. Update the description of the `grok` binary invocation so it
matches the implementation and Quickstart by using `--single` with
`--output-format streaming-json`, and keep the wording aligned with the
`grok::run` behavior and the `grok::events` / `agent::events` streams.
In `@grok/src/events.rs`:
- Around line 16-24: The session-scoped map in next_item_id is leaking entries
because SEQ permanently stores every session_id. Replace the HashMap-based
per-session counter with a process-wide atomic sequence (or other
non-session-tracking unique counter) in events.rs, and update next_item_id and
its callers so emit still generates unique item_id values without retaining
per-session state.
In `@grok/src/functions/mod.rs`:
- Around line 127-141: The grok::stop response contract is inconsistent: the
implementation in the stop handler returns reason as null on success, but the
response_format still declares reason as a string. Update the schema in the
grok::stop builder so reason is nullable or change the handler to always emit a
string, and make sure the request/response definitions in mod.rs stay aligned
with the actual payload.
- Around line 79-107: The current `grok::start` flow spawns `grok::run` in the
background and returns success before `run` has actually accepted the request,
so the caller can be told `started: true` even when validation or live-session
reservation fails. Change the `grok::start` handler to either perform the
preflight checks synchronously before spawning, or wait for an acceptance
signal/result from `grok::run` before returning. Use the existing `grok::start`,
`grok::run`, and `mark_error` paths to ensure only accepted runs return success
and rejected ones are surfaced immediately.
In `@grok/src/functions/types.rs`:
- Around line 55-61: The last-content parsing in types::extract_prompt (the
match on last.content) currently treats any Value::Array as valid and can return
an empty string when no text blocks are present. Update this branch to detect
when the filtered text pieces are empty and return an error instead of Ok(""),
so grok::run surfaces the request-shape failure rather than building an empty
--single prompt. Use the existing last.content handling and the Value::Array
path as the place to enforce this validation.
In `@grok/src/grok/mod.rs`:
- Around line 193-197: The stderr handling in the grok runner currently reads
the entire stream into memory even though only the last 2000 characters are
used; update the stderr collection logic in the code path around
outcome/result_text so it maintains a bounded rolling tail instead of using an
unbounded string buffer. Apply the same fix in the other stderr-tail path noted
in the diff, and keep the final trimming/truncation behavior in place so the
returned text still uses only the last 2000 characters.
- Around line 122-128: The load-and-resume path in grok::mod::turn should not
silently continue when load_session fails. Update the error handling around
load_session so a failed read returns an error for the turn, or otherwise
prevents the later SessionRecord rebuild and save path from running. Make sure
the SessionRecord creation/persistence logic is gated on a successfully loaded
prior state, and keep the resume flow in sync with the existing session_id and
grok_thread_id handling.
In `@grok/src/state.rs`:
- Around line 63-70: The state parsing in state::list is swallowing storage
corruption by returning an empty Vec for non-array input and silently skipping
undecodable items. Update the SessionRecord deserialization flow in
grok::state::list so that a non-array response or any malformed record returns
an error instead of collapsing into []; keep the typed contract explicit and
propagate the failure up to grok::sessions::list.
In `@grok/tests/prompt_config.rs`:
- Around line 57-64: The test config_defaults_when_file_missing currently uses a
hard-coded Unix-only missing path, which makes it non-portable and tied to host
filesystem behavior. Update this test to create a guaranteed-missing path inside
a temporary directory using tempdir(), and pass that path into Config::load so
the missing-file case stays isolated and works across platforms.
---
Nitpick comments:
In `@grok/skills/SKILL.md`:
- Around line 44-47: Update the wording in SKILL.md around the approval policy
to make it clear that `always_approve: true` is the default behavior for
headless turns, not a requirement. Adjust the description near the tool/command
execution guidance so it reads as “by default” and still points users to set
`always_approve: false` when they want the CLI’s approval policy to gate
execution.
In `@grok/src/configuration.rs`:
- Around line 155-157: The retry sleep in the configuration retry loop is using
a linear delay instead of exponential backoff. Update the retry timing in the
configuration loading logic (the block around CONFIG_RETRIES in
configuration::*) so each successive retry waits exponentially longer rather
than 250ms, 500ms, 750ms; keep the retry count and surrounding control flow the
same, but adjust the Duration calculation to use an exponential growth pattern.
In `@grok/tests/args.rs`:
- Around line 90-95: The test only covers the prior-value fallback path and
misses the Config::default() branch in resolve, so extend
resolve_falls_back_to_prior_then_config_defaults to also assert behavior when no
prior model/cwd is provided and resolution falls through to the config defaults.
Use the existing resolve function plus Config::default() and
RunRequest::default() in grok/tests/args.rs to add a case that verifies the
default precedence contract still works when prior values are None.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4fefbb69-248d-4a39-b76c-0dc63eff4f98
⛔ Files ignored due to path filters (4)
grok/Cargo.lockis excluded by!**/*.lockgrok/assets/cli-help.pngis excluded by!**/*.pnggrok/assets/cli-run.pngis excluded by!**/*.pnggrok/assets/iii-context.pngis excluded by!**/*.png
📒 Files selected for processing (27)
README.mdgrok/.gitignoregrok/Cargo.tomlgrok/README.mdgrok/build.rsgrok/config.yamlgrok/iii-permissions.yamlgrok/iii.worker.yamlgrok/skills/SKILL.mdgrok/src/config.rsgrok/src/configuration.rsgrok/src/events.rsgrok/src/functions/mod.rsgrok/src/functions/types.rsgrok/src/grok/args.rsgrok/src/grok/events_types.rsgrok/src/grok/mod.rsgrok/src/grok/translate.rsgrok/src/iii_prompt.rsgrok/src/lib.rsgrok/src/main.rsgrok/src/manifest.rsgrok/src/state.rsgrok/src/wire.rsgrok/tests/args.rsgrok/tests/prompt_config.rsgrok/tests/translate.rs
Verified each review item against current code; fixed the still-valid ones,
skipped the rest with reasons below.
Fixed:
- events.rs: per-session HashMap counter leaked an entry per session forever;
replaced with a process-wide AtomicU64 (still globally unique, no retained
per-session state).
- functions/mod.rs: grok::stop response schema declared `reason` as string but
the handler returns null on success — made it `["string","null"]`.
- functions/types.rs: extract_prompt returned Ok("") for a messages array with
no text blocks; now errors so the bad request shape surfaces.
- grok/mod.rs: stderr drain read the whole stream into memory; now a bounded
rolling tail (8KiB cap, last 2000 chars still surfaced).
- configuration.rs: retry backoff was linear (250/500/750ms) → exponential.
- tests: portable missing-file test via tempdir; added the no-prior →
config-defaults branch for resolve.
- skills: clarified always_approve is the default, not a requirement.
Release wiring (SOP §6): grok added to create-tag.yml options + release.yml
tag patterns.
Skipped (still-valid check):
- iii.worker.yaml/README function shorthand: `grok::run/start/stop/status/
sessions::list` is the codex-parity shorthand; the full `grok::sessions::list`
symbol is spelled out in the Functions table.
- README engine::functions::list: correct for "find function ids"; worker
discovery is the adjacent registry clause.
- README --print: already `--single` in current code.
- grok::start fire-and-forget: returns immediately by contract; rejected/failed
runs surface via the mark_error supervisor + grok::status + agent::events.
- load_session / state::list resilience: deliberate — a transient state blip
must not fail a new turn, and listing skips a single corrupt row rather than
failing the whole list (single-record load already propagates corruption).
fmt + clippy -D warnings clean, 24 tests pass.
code-simplifier pass over the grok worker: - events_types.rs: drop unused EndEvent.request_id (parsed, never read; serde ignores the wire field regardless). - remove the base_url config field end-to-end — it was published in the schema and seeded but never wired to the grok CLI argv (grok takes no base-url flag; auth is XAI_API_KEY). Dropped from config.rs, manifest.rs, config.yaml, configuration.rs description, and README. - configuration.rs: fix stale 'sandbox defaults' doc copied from codex. No behavior change. fmt + clippy -D warnings clean, 24 tests pass.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
grok/skills/SKILL.md (1)
39-53: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winStrengthen the warning about default auto-approval.
The current text documents
always_approve: trueas a boundary, but given the Grok CLI's filesystem and shell access, the default auto-approval poses a genuine risk of destructive operations without human verification. The static analysis hint flags this as excessive agency (EA2). Consider adding an explicit security warning that the defaultalways_approve: trueallows ungated tool execution, and strongly recommendalways_approve: falsefor sensitive environments.- Tool/command execution is the Grok CLI's own; headless turns auto-approve - by default (`always_approve: true`) so they don't block on an interactive - approval prompt. Set `always_approve: false` to let the CLI's approval - policy gate execution. + Tool/command execution is the Grok CLI's own; headless turns auto-approve + by default (`always_approve: true`) so they don't block on an interactive + approval prompt. ⚠️ This allows ungated filesystem and shell operations. + Set `always_approve: false` to require explicit approval before destructive + or high-impact commands execute.🤖 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 `@grok/skills/SKILL.md` around lines 39 - 53, The Boundaries section underlines `always_approve: true` as a default, but it should explicitly warn that this enables ungated Grok CLI tool execution with filesystem and shell access. Update the wording around `always_approve` to call out the security risk of destructive actions without human verification and clearly recommend `always_approve: false` for sensitive environments, using the existing `always_approve` and `grok::run` guidance as the anchor points.Source: Linters/SAST tools
🤖 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.
Nitpick comments:
In `@grok/skills/SKILL.md`:
- Around line 39-53: The Boundaries section underlines `always_approve: true` as
a default, but it should explicitly warn that this enables ungated Grok CLI tool
execution with filesystem and shell access. Update the wording around
`always_approve` to call out the security risk of destructive actions without
human verification and clearly recommend `always_approve: false` for sensitive
environments, using the existing `always_approve` and `grok::run` guidance as
the anchor points.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fda9b46b-8d5e-4f96-a858-355c0ec7a0c1
📒 Files selected for processing (15)
.github/workflows/create-tag.yml.github/workflows/release.ymlgrok/README.mdgrok/config.yamlgrok/skills/SKILL.mdgrok/src/config.rsgrok/src/configuration.rsgrok/src/events.rsgrok/src/functions/mod.rsgrok/src/functions/types.rsgrok/src/grok/events_types.rsgrok/src/grok/mod.rsgrok/src/manifest.rsgrok/tests/args.rsgrok/tests/prompt_config.rs
💤 Files with no reviewable changes (4)
- grok/config.yaml
- grok/src/manifest.rs
- grok/src/config.rs
- grok/README.md
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/release.yml
🚧 Files skipped from review as they are similar to previous changes (5)
- grok/tests/prompt_config.rs
- grok/src/functions/types.rs
- grok/src/functions/mod.rs
- grok/src/configuration.rs
- grok/src/grok/mod.rs
A load_session error (corruption/transient store failure) previously fell through to a fresh SessionRecord, which then overwrote the existing session's turns + grok_thread_id on save and broke the next --resume. Genuinely-absent sessions are Ok(None) and still proceed fresh; only Err now fails the turn, leaving stored state untouched.
…e inputs The console builds the worker-config form from the schemars-generated JSON schema; fields with no doc comment emit no `description`, so the form showed unlabeled/empty inputs. Added a /// description to every Defaults + Config field (the pattern the configurable workers like context-manager already follow), so each renders as a labeled, editable field.
What
Adds the
grokworker: the xAI Grok CLI exposed as iii functions and streams on the bus, nothing else. Modeled on thecodexworker, driving the headless streaming-json path (grok --single <prompt> --output-format streaming-json) — no ACP, everything on iii primitives.Verified end-to-end
Validated against the live Grok CLI 0.2.77 on a running engine:
grok::runreturns the correct result over the bus;grok::statusreports turns/state.grok::runwith the returnedsession_idcontinues the same Grok session via--resume(turn 2 recalled turn 1's reply).agent::eventscarriesmessage_completewith['thinking','text']content blocks, thenturn_end+agent_end.The arg surface and event schema were captured from the actual CLI, not guessed:
--single(prompt),--resume(continuation),--output-format streaming-json,--no-alt-screen,--model,--cwd,--always-approve. Auth viaXAI_API_KEY.{type:"text",data}answer deltas,{type:"thought",data}reasoning deltas,{type:"end",stopReason,sessionId,requestId},{type:"error",message}. Raw lines mirror verbatim ontogrok::events; the worker accumulates deltas and emits onemessage_complete(thinking block first) onagent::eventsatend.end.sessionIdbecomesgrok_thread_idfor the next--resume.Surface
grok::run/grok::start/grok::stop/grok::status/grok::sessions::list, all with typed request/response schemas (noAnyValue). README,skills/SKILL.md,iii-permissions.yaml, repo Modules row.Notes
Usagetype).grok::eventsand are skipped on the translated stream rather than failing the turn.create-tag.yml/release.yml(release wiring is a follow-up perdocs/sops/new-worker.md§6) — left as draft for that + maintainer review.Checks
cargo fmt --all -- --checkclean ·cargo clippy --all-targets --all-features -- -D warningsclean · 23 tests pass.Summary by CodeRabbit