Add Grok Build CLI as an ACP Agent Mode provider - #817
Conversation
Integrates xAI's Grok Build (`grok agent stdio`, ACP protocolVersion 1) as a first-class Agent Mode provider following the Cursor/OpenCode ACP pattern, verified against live grok 1.0.3 probes: - New provider folder Infrastructure/AI/Providers/GrokBuild: config, launch resolver (~/.grok/bin discovery, grok agent --help probe), ACP provider (XAI_API_KEY injection from the existing .grokAPI account, text-only attachment policy, auth error guidance), event normalizer, headless adapter, model polling with one retained discovery session per workspace, and managed/full-access tool preferences backed by a new secure-store permission domain. - Controller: ACPDirectSessionModelProvider capability for Grok's top-level SessionModelState (no configOptions on the wire), session/set_model dispatch, _meta.usage fallback for prompt-response usage, Grok permission-option preferences with an explicit enable-always-approve denylist, and permission/model-keyed controller reuse for Grok's launch-flag full access. - Identities and wiring: AgentProviderKind/ACPProviderID/ AgentProviderBindingID .grokBuild, grok-shell MCP client family, agent_mode_grok_build_engineer policy profile, availability context, settings card + connection test, onboarding row, recommendations, model catalog (static default + discovered options), 500K context fallback, and telemetry mapping. - Shared: extracted ACPToolUpdateResultAdapter from Cursor's normalizer; ACPAgentProviderFactory is now async to resolve the Grok key at construction. Plan: docs/plans/grok-build-acp-provider-2026-08-13.md Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 03fad02d16
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The interactive ACPAgentProviderFactory previously OR'd the global Full Access preference into launch args, so a run under an .mcpSafeDefaults profile (MCP-started sessions, subagents) would still launch `grok agent --always-approve stdio`. The run request's permission binding is authoritative; the headless path keeps the global preference since it has no per-run context. Flagged by chatgpt-codex-connector on PR #817. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Codex P1 fixed in 1537382: the interactive factory no longer ORs the global Full Access preference — the per-run permission binding (which honors .mcpSafeDefaults/custom profiles) is authoritative, with a regression test. Headless keeps the global preference (no per-run context). Sentry thread is a formatter false positive (hoisted try await). |
Grok's background workers (its own configured MCP servers, auth refresh) log fatal-looking transport errors to stderr even when the ACP session is healthy, and every provider's stderr arrived in the transcript with raw ANSI color codes. Add an optional `shouldEmitStderrLine(_:)` provider hook (default surfaces all; GrokBuild suppresses the known worker-transport noise) and strip ANSI CSI sequences before emitting stderr as system events. Diagnostics still record every line. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The stderr filter was declared only in the ACPAgentProvider extension, so the controller's existential call resolved statically to the default (always true) and provider overrides never ran. Declare it as a protocol requirement and cover the dispatch with a regression test driving a controller against a stderr-noisy fake server. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Grok 1.0.4's default leader mode spawns session MCP servers from a shared leader process instead of the ACP session process, so the injected RepoPrompt MCP connection's ancestor chain never contains the expected agent PID and admission is rejected (no pending policy or live run affinity). `grok agent --no-leader stdio` makes the MCP server a direct child of the ACP session process, restoring expected-PID admission for Agent Mode and Context Builder runs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Grok Build presents grok-shell-<injected server name> (e.g. grok-shell-RepoPromptCE) to MCP servers, but the provider advertised the family-only hint "grok-shell". PendingRunScopedContextStore keys are raw client names with no family canonicalization, so the run's frozen tab context — installed under the hint — never popped for the registered connection. Context Builder runs then committed against a tab identity that differed from the record's tabID and were rejected at installCommittedTabSnapshot (context_builder.commit_retain_rejected). Claude/Codex never hit this because their registered names equal their hints. Derive the Grok hint from RepoPromptMCPServerConfiguration's default server name so install and pop keys are identical; the canonical grok-shell family in MCPClientIdentity still covers family-level matching (expected-PID buckets, policy matching). Also enrich the DEBUG-only commit_retain_rejected routing event with workspace IDs/names for both record and snapshot — the diagnostics that pinned this down. Dogfood: Context Builder with Grok now routes policy_installed -> run_route_mapped -> policy_applied -> commit_claim -> commit_outcome(committed), matching the Claude run's event sequence on the same workspace. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Blocking finding: fail closed on Grok model-selection acknowledgments.
In ACPAgentSessionController.setSessionModelSerialized (Sources/RepoPrompt/Infrastructure/AI/ACP/ACPAgentSessionController.swift:697-720), the direct Grok path rejects an explicit _meta.model.Err, but accepts a missing or malformed _meta.model, a missing Ok, or an Ok naming a different model. It then updates discoveredSessionModels and AgentACPModelRegistry as though the requested model were active. That can make RepoPrompt display and persist model authority that Grok never confirmed.
Please require _meta.model.Ok to equal the canonical requested model before updating local state; otherwise fail closed. Add regression coverage for missing metadata, malformed outcomes, and mismatched Ok values. The current direct-model test covers only the expected successful response.
Reviewed on exact PR head ac841740e7a2f48de09936525a78d1067596a085 against current-main synthetic merge 2780350a98682b76c3d5890619abe7e2dd83a6c6.
The direct SessionModelState path only rejected an explicit
_meta.model.Err and otherwise updated discoveredSessionModels and the
AgentACPModelRegistry as though the requested model were active —
accepting a missing _meta.model, a malformed outcome, or an Ok naming
a different model than requested.
Live-verified against grok 1.0.4: session/set_model answers
{"_meta":{"model":{"Ok":"<applied model id>"}}} on success and a
JSON-RPC error for unknown ids. Require the Ok payload to echo the
canonical requested model (case-insensitive) before mutating local
model authority; anything else now throws a protocol violation and
leaves the session's real current model untouched.
Regression coverage: Err outcome, missing acknowledgement, malformed
acknowledgement, and mismatched Ok — each asserts the throw and that
the registry's currentModelRaw is preserved.
Addresses PR #817 review feedback.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
The direct path now requires Wire shapes verified live against grok 1.0.4 just now: success is Regression coverage added in |
baron
left a comment
baron
left a comment
There was a problem hiding this comment.
Re-reviewed the remediation on exact head 53adc33 against current-main synthetic merge 09950e1.\n\nThe Grok session/set_model acknowledgment path now fails closed: local model state advances only after a matching Ok response, while Err, missing, malformed, and mismatched acknowledgments preserve the prior state. The added regression coverage exercises those failure paths, and all exact-head hosted checks are green.\n\nNo blocking findings.
2 participants
Summary
Integrates xAI's Grok Build (
grok agent stdio, ACP protocolVersion 1) as a first-class Agent Mode provider, following the Cursor/OpenCode ACP pattern. Wire contract verified against live probes (grok 1.0.3 + 1.0.4):Infrastructure/AI/Providers/GrokBuild/(7 files): launch resolver (trusted executable identity,~/.grok/bindiscovery,grok agent --helpprobe,--no-leaderso MCP servers stay in the ACP process tree for expected-PID admission), ACP provider (XAI_API_KEY injected from the existing.grokAPIkeychain account, text-only attachment policy, auth guidance), event normalizer, headless adapter, model polling with one retained verified discovery session per workspace, managed/full-access tool preferences with a new secure-store permission domain.ACPDirectSessionModelProvidercapability for Grok's top-levelSessionModelState(noconfigOptionson the wire),session/set_modeldispatch withErr-variant detection,_meta.usagefallback for prompt-response usage, Grok permission-option preferences withenable-always-approvedenylisted from every selection path, permission/model-keyed controller reuse for Grok's launch-flag full access,shouldEmitStderrLineas a protocol requirement (extension-only declarations statically dispatch to the default) + ANSI stripping so Grok's worker noise never reaches transcripts.AgentProviderKind/ACPProviderID/AgentProviderBindingID.grokBuild,grok-shellMCP client family,agent_mode_grok_build_engineerpolicy profile, availability context, settings card + connection test, onboarding row, recommendations, static-default + discovered model catalog, 500K context fallback, telemetry mapping.grok-shell-RepoPromptCE, derived from the injected server name).PendingRunScopedContextStorekeys are raw client names, so the family-only hint never bound run-scoped frozen tab contexts and Context Builder commits were rejected (commit_retain_rejected). Claude/Codex were immune because their registered names equal their hints.ACPToolUpdateResultAdapterextracted from Cursor's normalizer (behavior fixture-equivalent);ACPAgentProviderFactoryand the headless bridge factory are now async to resolve the Grok key at construction.Plan:
docs/plans/grok-build-acp-provider-2026-08-13.md· Review:docs/reviews/grok-acp-provider-impl-review-2026-08-13.md(round 1 CHANGES-REQUIRED → fixes → round 2 SIGN-OFF)Verification
agent_mode_grok_build_engineer), secure storage catalog, settings persistence, ContextBuilder startup/lifecycle, Cursor resolver (regression)make dev-lintcleanDogfood evidence (live beta, grok 1.0.4)
get_file_tree,set_status,read_file,use_toolround-tripspolicy_installed → run_route_mapped → policy_applied → commit_claim → commit_outcome(committed)— identical to the Claude run on the same workspace (previously died atcommit_retain_rejected)pkillapp → relaunch →agent_run steerresumes viasession/loadand recalls the codeword🤖 Generated with Claude Code