fix(grok): address adversarial-review findings across the grok-CLI conversion

A 7-dimension adversarial review (find -> independently refute) surfaced 14 real
issues; fixed each:

Runtime bugs
- GrokCliSession.send drained stdout fully BEFORE stderr — a >64KB stderr burst
  would deadlock the turn forever (spinner never clears). Drain stderr
  concurrently, and add a per-turn watchdog (ROBOCO_GROK_TURN_TIMEOUT_SECONDS,
  default 600s) that kills a wedged process and emits error+turn_end.
- Crash-restarted grok agents launched `grok -p ""` (empty prompt) — Claude gets
  a scan-for-work fallback. Default the prompt in _spawn_container so every
  dedicated provider gets it too.
- _grok_usage_json read /data/grok-usage unconditionally while its writers branch
  compose-vs-local, so a local-mode agent finalized at $0 and the cost-cap was
  inert. Single-source the path in a new _grok_usage_dir helper (read == write).
- GrokCliSession secretary role fell through to "unknown" (get_agent_role returns
  a truthy sentinel, never None), defeating the ROBOCO_AGENT_ROLE fallback.

Parity / hardening
- --deny set was missing `git tag -d` / `git reflog delete` that the Claude
  bash-guard blocks — added them (the "same set" claim is now true).
- Interactive mains now install the bash-guard hook too (defense-in-depth).
- Compose: collapse the GROK_AUTH_DIR / ROBOCO_HOST_GROK_DIR auth-mount pair into
  one canonical var so a partial override can't silently break agent auth.

Docs / comments
- Panel routing card + architecture security doc no longer say Grok runs on the
  deleted opencode runtime; orchestrator comments point at the renamed entrypoint.

Tests
- Cover the interactive _render_grok_config MCP wiring (ModuleNotFound guard +
  secretary HMAC env), the cost-cap kill-failure + interactive relay-close paths,
  the local-mode usage read, the role fallback, the turn timeout, and the new
  git denies. (#13 — a separate grok "Write" tool — investigated: grok's only
  built-in file-mutation tool is search_replace, already removed; no gap.)

Gate green: ruff, mypy, xenon, tests.
This commit is contained in:
Renn F
2026-06-19 06:09:15 +02:00
parent 0aa21639ab
commit a402de61bc
16 changed files with 378 additions and 68 deletions
+48 -3
View File
@@ -1,19 +1,27 @@
"""grok_cli_session — the pure streaming-json → StreamChunk mapper.
The subprocess runner (``GrokCliSession``) needs the live grok binary, so it is
not gate-covered; the turn-mapping logic lives in the pure ``_StreamAssembler``
and is fully exercised here by feeding it parsed events.
The subprocess runner (``GrokCliSession.send``) needs the live grok binary, so it
is not gate-covered; the turn-mapping logic lives in the pure ``_StreamAssembler``
and is fully exercised here by feeding it parsed events. The synchronous
``__init__`` (role resolution, per-role flags, timeout) IS pure and tested.
"""
from __future__ import annotations
import json
from typing import TYPE_CHECKING
from roboco.agent_sdk.grok_cli_session import (
GrokCliSession,
_classify_failure,
_parse_event,
_StreamAssembler,
_turn_timeout_seconds,
)
from roboco.llm.providers.grok_cli_config import grok_cli_args_for_role
if TYPE_CHECKING:
import pytest
def _kinds(chunks: list) -> list[str]:
@@ -93,3 +101,40 @@ def test_classify_failure_generic_uses_last_stderr_line() -> None:
assert "boom: the model exploded" in msg
# With no stderr, the exit code is surfaced.
assert "exit code 2" in _classify_failure(2, "")
def test_session_resolves_role_from_env_when_id_is_a_uuid(
monkeypatch: pytest.MonkeyPatch,
) -> None:
# The secretary's ROBOCO_AGENT_ID is a UUID; get_agent_role returns the
# "unknown" sentinel for it, so the role must fall back to ROBOCO_AGENT_ROLE
# (not silently use "unknown").
monkeypatch.setenv("ROBOCO_AGENT_ROLE", "secretary")
monkeypatch.delenv("ROBOCO_GROK_REASONING_EFFORT", raising=False)
session = GrokCliSession(cwd="/app", agent_id="0192-uuid-not-a-slug")
assert session._role_args == grok_cli_args_for_role("secretary")
def test_session_uses_slug_role_when_id_maps(
monkeypatch: pytest.MonkeyPatch,
) -> None:
monkeypatch.delenv("ROBOCO_AGENT_ROLE", raising=False)
monkeypatch.delenv("ROBOCO_GROK_REASONING_EFFORT", raising=False)
# intake-1 maps to the prompter role -> subagents allowed (not disallowed).
session = GrokCliSession(cwd="/ws", agent_id="intake-1")
dis = session._role_args[session._role_args.index("--disallowed-tools") + 1]
assert "Agent" not in dis
def test_turn_timeout_seconds_env_and_default(
monkeypatch: pytest.MonkeyPatch,
) -> None:
monkeypatch.delenv("ROBOCO_GROK_TURN_TIMEOUT_SECONDS", raising=False)
assert _turn_timeout_seconds() == 600.0 # noqa: PLR2004
monkeypatch.setenv("ROBOCO_GROK_TURN_TIMEOUT_SECONDS", "120")
assert _turn_timeout_seconds() == 120.0 # noqa: PLR2004
# Garbage / non-positive falls back to the default.
monkeypatch.setenv("ROBOCO_GROK_TURN_TIMEOUT_SECONDS", "nope")
assert _turn_timeout_seconds() == 600.0 # noqa: PLR2004
monkeypatch.setenv("ROBOCO_GROK_TURN_TIMEOUT_SECONDS", "0")
assert _turn_timeout_seconds() == 600.0 # noqa: PLR2004
@@ -0,0 +1,71 @@
"""Interactive grok entrypoints render the load-bearing MCP wiring into config.toml.
``_render_grok_config`` is the only synchronous, testable part of the interactive
mains (``main()`` needs the live container). It must produce the exact MCP
invocation the branch depends on — ``uv run --directory /app --no-sync`` (the
ModuleNotFound guard), ``UV_PROJECT_ENVIRONMENT=/app/.venv``, and (secretary) the
HMAC identity env the directive tools authenticate with.
"""
from __future__ import annotations
import tomllib
from typing import TYPE_CHECKING
from roboco.agent_sdk import grok_intake_main, grok_secretary_main
if TYPE_CHECKING:
from pathlib import Path
import pytest
def test_intake_render_wires_roboco_intake_mcp(
tmp_path: Path, monkeypatch: pytest.MonkeyPatch
) -> None:
cfg = tmp_path / ".grok" / "config.toml"
monkeypatch.setattr(grok_intake_main, "GROK_CONFIG_PATH", cfg)
grok_intake_main._render_grok_config("http://orch:8000", "sess-1")
parsed = tomllib.loads(cfg.read_text())
server = parsed["mcp_servers"]["roboco-intake"]
assert server["command"] == "uv"
# The ModuleNotFound guard: --directory /app + --no-sync, installed module.
assert server["args"] == [
"run",
"--directory",
"/app",
"--no-sync",
"python",
"-m",
"roboco.mcp.intake_server",
]
assert server["env"]["UV_PROJECT_ENVIRONMENT"] == "/app/.venv"
assert server["env"]["ROBOCO_API_URL"] == "http://orch:8000"
assert server["env"]["ROBOCO_PROMPTER_SESSION_ID"] == "sess-1"
def test_secretary_render_wires_mcp_and_hmac_identity(
tmp_path: Path, monkeypatch: pytest.MonkeyPatch
) -> None:
cfg = tmp_path / ".grok" / "config.toml"
monkeypatch.setattr(grok_secretary_main, "GROK_CONFIG_PATH", cfg)
monkeypatch.setenv("ROBOCO_AGENT_ID", "uuid-sec")
monkeypatch.setenv("ROBOCO_AGENT_ROLE", "secretary")
monkeypatch.setenv("ROBOCO_AGENT_TOKEN", "hmac-xyz")
grok_secretary_main._render_grok_config("http://orch:8000")
parsed = tomllib.loads(cfg.read_text())
server = parsed["mcp_servers"]["roboco-secretary"]
assert server["args"] == [
"run",
"--directory",
"/app",
"--no-sync",
"python",
"-m",
"roboco.mcp.secretary_server",
]
# The HMAC identity the directive tools authenticate with must flow through.
assert server["env"]["ROBOCO_AGENT_TOKEN"] == "hmac-xyz"
assert server["env"]["ROBOCO_AGENT_ID"] == "uuid-sec"
assert server["env"]["ROBOCO_AGENT_ROLE"] == "secretary"
assert server["env"]["UV_PROJECT_ENVIRONMENT"] == "/app/.venv"
@@ -130,18 +130,21 @@ def test_max_turns_is_emitted() -> None:
def test_bash_roles_deny_the_full_git_mutation_set() -> None:
# Graceful native --deny rules (the agent recovers) covering the same git
# network / branch / history ops the Claude bash-guard blocks.
# Graceful native --deny rules (the agent recovers) covering the SAME git
# network / branch / history ops the Claude bash-guard blocks — including the
# tag-deletion / reflog-deletion the hook matches.
args = gc.grok_cli_args_for_role("developer")
for op in ("push", "fetch", "clone", "checkout", "merge", "rebase", "revert"):
assert f"Bash(git {op}*)" in args
assert "Bash(git tag -d*)" in args
assert "Bash(git reflog delete*)" in args
assert "Bash(rm -rf*)" in args
def test_bash_guard_hook_config_skips_git() -> None:
handler = gc.bash_guard_hook_config("/app/scripts/bash-guard-hook.sh")[
"hooks"
]["PreToolUse"][0]
handler = gc.bash_guard_hook_config("/app/scripts/bash-guard-hook.sh")["hooks"][
"PreToolUse"
][0]
assert handler["matcher"] == "Bash"
inner = handler["hooks"][0]
assert inner["command"] == "/app/scripts/bash-guard-hook.sh"
+50 -4
View File
@@ -14,12 +14,18 @@ from unittest.mock import AsyncMock
import pytest
from roboco.models.runtime import AgentInstance
from roboco.runtime.orchestrator import AgentOrchestrator, AgentState
from roboco.runtime.orchestrator import (
INTAKE_AGENT_ID,
AgentOrchestrator,
AgentState,
)
def _grok_instance(provider_type: str = "grok") -> AgentInstance:
def _grok_instance(
provider_type: str = "grok", agent_id: str = "be-dev-1"
) -> AgentInstance:
cfg = type("C", (), {"provider_type": provider_type, "model": "grok-build"})()
return AgentInstance(agent_id="be-dev-1", state=AgentState.ACTIVE, config=cfg)
return AgentInstance(agent_id=agent_id, state=AgentState.ACTIVE, config=cfg)
def _orch(
@@ -28,11 +34,12 @@ def _orch(
cap: float,
cost: float,
provider_type: str = "grok",
agent_id: str = "be-dev-1",
) -> tuple[AgentOrchestrator, AsyncMock]:
"""A bare orchestrator with the cost reader + container removal stubbed."""
orch = AgentOrchestrator.__new__(AgentOrchestrator)
orch._grok_max_cost_usd = cap
orch._instances = {"be-dev-1": _grok_instance(provider_type)}
orch._instances = {agent_id: _grok_instance(provider_type, agent_id)}
monkeypatch.setattr(orch, "_grok_cost_usd", lambda _agent_id: cost)
remove_mock = AsyncMock()
monkeypatch.setattr(orch, "_remove_container", remove_mock)
@@ -79,3 +86,42 @@ async def test_non_grok_container_is_ignored(monkeypatch: pytest.MonkeyPatch) ->
remove_mock.assert_not_awaited()
assert "be-dev-1" in orch._instances
@pytest.mark.asyncio
async def test_kill_failure_keeps_instance_for_retry(
monkeypatch: pytest.MonkeyPatch,
) -> None:
# If `docker rm` raises, the over-budget container must STAY registered so the
# sweep retries next tick (the except `continue` resilience contract).
orch, remove_mock = _orch(monkeypatch, cap=5.0, cost=7.5)
remove_mock.side_effect = RuntimeError("docker rm failed")
await orch._enforce_grok_cost_budget()
remove_mock.assert_awaited_once()
assert "be-dev-1" in orch._instances # not evicted -> retried next tick
@pytest.mark.asyncio
async def test_interactive_kill_closes_the_relay(
monkeypatch: pytest.MonkeyPatch,
) -> None:
# Killing an interactive (intake/secretary) container over budget must close
# its panel relay with a reason so the chat ends cleanly, not a frozen SSE.
orch, remove_mock = _orch(monkeypatch, cap=5.0, cost=9.0, agent_id=INTAKE_AGENT_ID)
registry = type("R", (), {"calls": []})()
registry.close_by_agent = lambda agent_id, error: registry.calls.append(
(agent_id, error)
)
monkeypatch.setattr(
"roboco.services.prompter_live.get_live_registry", lambda: registry
)
await orch._enforce_grok_cost_budget()
remove_mock.assert_awaited_once_with(f"roboco-agent-{INTAKE_AGENT_ID}")
assert INTAKE_AGENT_ID not in orch._instances
assert len(registry.calls) == 1
assert registry.calls[0][0] == INTAKE_AGENT_ID
assert "cost" in registry.calls[0][1].lower()
@@ -10,10 +10,12 @@ output (it bills at the output rate).
from __future__ import annotations
import json
import tempfile
from typing import TYPE_CHECKING
import pytest
from roboco.models.runtime import AgentInstance
from roboco.runtime import orchestrator as orch_mod
from roboco.runtime.orchestrator import AgentOrchestrator
if TYPE_CHECKING:
@@ -75,3 +77,35 @@ async def test_resolve_final_usage_routes_grok_to_usage_json(
# No SDK fetch / transcript read for GROK — usage comes from usage.json.
assert await orch._resolve_final_token_usage("be-dev-1") == (0, 12, 0, 0)
def test_grok_usage_dir_branches_compose_vs_local(
monkeypatch: pytest.MonkeyPatch,
) -> None:
monkeypatch.setattr(orch_mod, "PROJECT_HOST_PATH", "")
local = AgentOrchestrator._grok_usage_dir("be-dev-1")
assert "roboco-grok-usage" in str(local)
assert local.name == "be-dev-1"
monkeypatch.setattr(orch_mod, "PROJECT_HOST_PATH", "/volume1/roboco")
monkeypatch.setattr(orch_mod, "GROK_USAGE_DATA_DIR", "/data/grok-usage")
assert str(AgentOrchestrator._grok_usage_dir("be-dev-1")) == (
"/data/grok-usage/be-dev-1"
)
def test_grok_usage_json_reads_the_real_local_dir(
tmp_path: Path, monkeypatch: pytest.MonkeyPatch
) -> None:
# The un-mocked read path must find usage.json in the SAME branched dir the
# writer mounts (the local-mode fix: read side mirrors the write side).
monkeypatch.setattr(orch_mod, "PROJECT_HOST_PATH", "")
monkeypatch.setattr(tempfile, "gettempdir", lambda: str(tmp_path))
udir = tmp_path / "roboco-grok-usage" / "be-dev-1"
udir.mkdir(parents=True)
(udir / "usage.json").write_text(
json.dumps({"total_tokens": 55, "cost_usd": 0.1}), encoding="utf-8"
)
orch = AgentOrchestrator.__new__(AgentOrchestrator)
assert orch._grok_usage_tokens("be-dev-1") == (0, 55, 0, 0)
assert orch._grok_cost_usd("be-dev-1") == 0.1 # noqa: PLR2004