diff --git a/modules/ai/skills/autoreview/SKILL.md b/modules/ai/skills/autoreview/SKILL.md index bfda8ad..93fca8f 100644 --- a/modules/ai/skills/autoreview/SKILL.md +++ b/modules/ai/skills/autoreview/SKILL.md @@ -7,11 +7,11 @@ description: "Auto Review closeout for Git and Jujutsu changes. Uses Amp by defa Run the bundled structured review helper as a closeout check. This is code review, not Guardian `auto_review` approval routing. -Amp is the default inside an Amp orb, detected by the documented `AMP_ORB=1` environment variable. Codex is the default elsewhere and usually delivers the best local review results. An explicit `--engine` always wins; `AUTOREVIEW_ENGINE` overrides the environment-based default. Codex defaults to `gpt-5.6-sol` and retries once with `gpt-5.6-terra` only when the account cannot access Sol; thinking follows the Codex CLI config. Claude defaults to `claude-fable-5`. Amp defaults to `openai/gpt-5.6-sol` at `high` reasoning through a generated adapter plugin that reuses the existing `amp login`. Pi and opencode use the model their own CLI is configured for. +Amp is the default inside an Amp orb, detected by the documented `AMP_ORB=1` environment variable. Codex is the default elsewhere and usually delivers the best local review results. An explicit `--engine` always wins; `AUTOREVIEW_ENGINE` overrides the environment-based default. Codex defaults to `gpt-5.6-sol` and retries once with `gpt-5.6-terra` only when the account cannot access Sol; thinking follows the Codex CLI config. Claude defaults to `fable`, the claude CLI alias for the latest Fable. Amp defaults to `openai/gpt-5.6-sol` at `high` reasoning through a generated adapter plugin that reuses the existing `amp login`. Grok defaults to `grok-4.6`. Pi and opencode use the model their own CLI is configured for. Use when: -- user asks for Codex review / Claude review / Amp review / Pi review / autoreview / second-model review +- user asks for Codex review / Claude review / Amp review / Grok review / Pi review / autoreview / second-model review - after non-trivial code edits, before final/commit/ship - reviewing a local branch or PR branch after fixes @@ -175,6 +175,13 @@ Inline syntax is also supported: --reviewers codex:gpt-5.6-sol:high,claude:sonnet:max ``` +The same engine may appear more than once with different models, which is how +an Amp orb runs the codex+grok panel through amp's model providers: + +```bash + --reviewers amp:openai/gpt-5.6-sol:xhigh,amp:xai/grok-4.6:xhigh +``` + `AUTOREVIEW_MODEL` and `AUTOREVIEW_THINKING` env vars accept the same keyed syntax (`codex=gpt-5.5,claude=sonnet` or a bare global value) and sit between CLI flags and built-in defaults. @@ -182,7 +189,8 @@ CLI flags and built-in defaults. Thinking per engine: Codex maps to `model_reasoning_effort` (`low`-`max`). Claude maps to `--effort` (`low`-`max`). Amp maps to the adapter plugin's `reasoningEffort` (`none`-`max`, default `high`) and its model must be a -`provider/model` id (default `openai/gpt-5.6-sol`). Pi maps to `--thinking` +`provider/model` id (default `openai/gpt-5.6-sol`). Grok maps to `--effort` +(`low`-`xhigh`). Pi maps to `--thinking` (`off`-`max`). OpenCode maps to `--variant` (`minimal`-`max`). Engines without a real thinking knob reject `--thinking`. @@ -215,7 +223,7 @@ The helper: fetch errors; use `--no-fetch` only when intentionally reviewing local refs - exits successfully without invoking any engine when the computed diff has no changed paths -- supports `--engine codex`, `claude`, `amp`, `droid`, `copilot`, `pi`, and `opencode`; default precedence is explicit `--engine`, `AUTOREVIEW_ENGINE`, Amp when `AMP_ORB=1`, then Codex +- supports `--engine codex`, `claude`, `amp`, `grok`, `droid`, `copilot`, `pi`, and `opencode`; default precedence is explicit `--engine`, `AUTOREVIEW_ENGINE`, Amp when `AMP_ORB=1`, then Codex - use `--mode commit --commit ` for already-committed work, especially clean `main` after landing - should be left in `--mode auto` or forced to `--mode branch` for PR/branch work; do not force `--mode local` after committing - writes only to stdout unless `--output` or `--json-output` is set @@ -223,7 +231,7 @@ The helper: - supports `--dry-run` as a real preflight: builds the bundle, applies the same input validation as a live run, resolves each reviewer binary, and exits nonzero on a broken setup without invoking any engine - supports `--parallel-tests`, `--prompt`, `--prompt-file`, `--dataset`, `--no-tools`, `--no-web-search`, and commit refs - supports opt-in review panels with `--panel` / `--reviewers`, plus per-engine `--model` and `--thinking` (also via `AUTOREVIEW_MODEL` / `AUTOREVIEW_THINKING`) -- allows read-only tools and web search by default where the selected CLI supports them; forbids nested review in the prompt; Codex is run through `codex exec` with read-only sandbox and structured output; amp reviews the bundle alone through a generated `amp.ai.generate` adapter plugin (temp config dir, so no personal plugins load; existing `amp login` credentials are reused); pi gets only its read tool; opencode runs its read-only plan agent +- allows read-only tools and web search by default where the selected CLI supports them; forbids nested review in the prompt; Codex is run through `codex exec` with read-only sandbox and structured output; amp reviews the bundle alone through a generated `amp.ai.generate` adapter plugin (temp config dir, so no personal plugins load; existing `amp login` credentials are reused); grok runs headless with only `read_file`/`grep`/`list_dir` (plus web tools), subagents and MCP meta-tools disabled, prompt via file, session deleted afterwards (grok has no ephemeral mode), and rejects `--no-tools` (grok has no reliable tool-off switch); like claude and codex it runs inside the reviewed checkout under the local trust model, so project hooks/config there apply; pi gets only its read tool; opencode runs its read-only plan agent - keeps droid and copilot adapters even though upstream disabled them; they review with the local trust model, not upstream's isolation contract - prints `review still running: elapsed=s pid=` to stderr at long-running intervals while waiting for the selected review engine; `--stream-engine-output` streams live engine text instead - prints `autoreview clean: no accepted/actionable findings reported` when the selected review command exits 0 diff --git a/modules/ai/skills/autoreview/UPSTREAM.txt b/modules/ai/skills/autoreview/UPSTREAM.txt index 042a6f5..2ffc293 100644 --- a/modules/ai/skills/autoreview/UPSTREAM.txt +++ b/modules/ai/skills/autoreview/UPSTREAM.txt @@ -9,4 +9,5 @@ upstream_synced_commit = 7c49348abc51e820a344868254ef0f5c35443720 sync_policy = diverged fork; sync concepts, not code. Do not attempt a file-level merge from upstream; port features selectively and keep the script lean. local_changes = adjusted helper paths; removed cache-repair guidance; reject out-of-scope findings (upstream ignores them); native jj local/branch/commit bundles and non-colocated harness coverage; --head/--remote/--no-fetch; empty-diff early exit; 5-round cap + nitpick escape hatch in SKILL.md synced_from_upstream_2026_08 = engines amp/pi/opencode (amp uses upstream's generated amp.ai.generate adapter plugin but reuses the existing amp login via real XDG data dir + temp config dir, no plugin-inventory attestation; pi read-tool only; opencode plan agent); model defaults gpt-5.6-sol/claude-fable-5 + Codex Sol-to-Terra account-access fallback; AUTOREVIEW_MODEL/AUTOREVIEW_THINKING env defaults; P0-P3 --max-priority filter (default P2 here; upstream defaults P0) + threshold prompt; real --dry-run preflight; --stream-engine-output with filtered Codex/Claude displays; SKILL.md scope governor + P0 contract + prose-only exemption +local_engines = grok (headless `grok --prompt-file --output-format json --json-schema`, read-only tool allowlist, no subagents; not in upstream) intentionally_not_synced = TruffleHog secret scanning + redaction pipeline; reviewer workspace/env isolation and process-group machinery; kimi + cursor engines; upstream's refusal of droid/copilot/opencode (kept working under local trust model); resumable --run-id/--run-root chunked passes; Windows harnesses; hardening test suite diff --git a/modules/ai/skills/autoreview/scripts/autoreview b/modules/ai/skills/autoreview/scripts/autoreview index b04f5a0..49c6f85 100755 --- a/modules/ai/skills/autoreview/scripts/autoreview +++ b/modules/ai/skills/autoreview/scripts/autoreview @@ -14,17 +14,19 @@ import tempfile import textwrap import threading import time +import uuid from pathlib import Path from typing import Any from urllib.parse import urlparse -ENGINES = ("codex", "claude", "amp", "droid", "copilot", "pi", "opencode") +ENGINES = ("codex", "claude", "amp", "grok", "droid", "copilot", "pi", "opencode") THINKING_LEVELS_BY_ENGINE = { "codex": {"low", "medium", "high", "xhigh", "max"}, "claude": {"low", "medium", "high", "xhigh", "max"}, # amp maps thinking to amp.ai.generate reasoningEffort in the adapter plugin "amp": {"none", "low", "medium", "high", "xhigh", "max"}, + "grok": {"low", "medium", "high", "xhigh"}, "droid": set(), "copilot": set(), "pi": {"off", "minimal", "low", "medium", "high", "xhigh", "max"}, @@ -33,8 +35,9 @@ THINKING_LEVELS_BY_ENGINE = { } DEFAULT_MODEL_BY_ENGINE = { "codex": "gpt-5.6-sol", - "claude": "claude-fable-5", + "claude": "fable", # claude CLI alias for the latest Fable "amp": "openai/gpt-5.6-sol", + "grok": "grok-4.6", } DEFAULT_THINKING_BY_ENGINE = { "amp": "high", @@ -49,6 +52,17 @@ AMP_ADAPTER_MODE = "autoreview" AMP_ADAPTER_AGENT = "autoreview-adapter" AMP_OUTER_MODEL = "openai/gpt-5.6-luna" AMP_MAX_OUTPUT_CHARS = 2_000_000 +# grok's built-in tool names; review stays read-only. +GROK_READ_TOOLS = ("read_file", "grep", "list_dir") +GROK_WEB_TOOLS = ("web_search", "web_fetch") +# --tools only allowlists built-ins; the MCP meta-tools stay unless denied. +GROK_DENY_TOOLS = ("search_tool", "use_tool") +# Engines with no reliable tool-off switch; validated before dry-run so preflight matches live runs. +NO_TOOLS_UNSUPPORTED = { + "codex": "--no-tools is not supported by the Codex engine; use --engine claude --no-tools for a no-tools run", + "grok": "--no-tools is not supported by the grok engine; use --engine claude --no-tools for a no-tools run", + "copilot": "--no-tools is not supported by the copilot engine; copilot requires a read-only file view tool to load the review bundle without exposing it in argv", +} AMP_MODEL_PATTERN = re.compile( r"(?:amp|anthropic|baseten|fireworks|openai|vertexai|xai)/" r"[A-Za-z0-9][A-Za-z0-9._-]*(?:/[A-Za-z0-9][A-Za-z0-9._-]*)*" @@ -715,9 +729,6 @@ def codex_model_access_failure(result: subprocess.CompletedProcess[str], model: def run_codex(args: argparse.Namespace, repo: Path, prompt: str) -> str: - if not args.tools: - raise SystemExit("--no-tools is not supported by the Codex engine; use --engine claude --no-tools for a no-tools run") - def attempt(model: str | None) -> tuple[subprocess.CompletedProcess[str], str]: schema_path = write_json_temp(SCHEMA) output_path = Path(tempfile.NamedTemporaryFile("w", suffix=".json", delete=False).name) @@ -812,6 +823,70 @@ def run_claude(args: argparse.Namespace, repo: Path, prompt: str) -> str: return result.stdout +def run_grok(args: argparse.Namespace, repo: Path, prompt: str) -> str: + # An empty or unknown --tools allowlist falls back to grok's default toolset and + # plan mode still allows writes, so the allowlist below is the only read-only gate. + # Bundles can exceed ARG_MAX, so the prompt goes through a file. + prompt_path = Path(tempfile.NamedTemporaryFile("w", suffix=".txt", delete=False).name) + prompt_path.write_text(prompt) + os.chmod(prompt_path, 0o600) + # grok has no ephemeral mode; a preassigned id lets cleanup delete the session even on failure. + session_id = str(uuid.uuid4()) + cmd = [ + args.grok_bin, + "--session-id", + session_id, + "--prompt-file", + str(prompt_path), + "--output-format", + "json", + "--json-schema", + json.dumps(SCHEMA), + "--cwd", + str(repo), + "--no-subagents", + "--disallowed-tools", + ",".join(GROK_DENY_TOOLS), + ] + tools = list(GROK_READ_TOOLS) + if args.web_search: + tools.extend(GROK_WEB_TOOLS) + cmd.extend(["--tools", ",".join(tools)]) + if not args.web_search: + cmd.append("--disable-web-search") + if args.model: + cmd.extend(["--model", args.model]) + if args.thinking: + cmd.extend(["--effort", args.thinking]) + # grok's json output arrives only at exit, so streaming is passthrough of stderr chatter. + result = None + try: + result = run_with_heartbeat( + cmd, + repo, + label="grok", + stream_display=passthrough_stream_display if args.stream_engine_output else None, + ) + finally: + prompt_path.unlink(missing_ok=True) + # On a failed run the session may not exist; report but keep the engine error primary. + delete_grok_session(args.grok_bin, session_id, repo, strict=result is not None and result.returncode == 0) + if result.returncode != 0: + raise SystemExit(f"grok engine failed ({result.returncode})\n{result.stderr or result.stdout}") + return result.stdout + + +def delete_grok_session(grok_bin: str, session_id: str, repo: Path, *, strict: bool) -> None: + """Drop the headless session so the review bundle does not persist under ~/.grok.""" + deleted = subprocess.run([grok_bin, "sessions", "delete", session_id], cwd=repo, text=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE) + if deleted.returncode == 0: + return + message = f"grok: failed to delete session {session_id}: {deleted.stderr.strip() or deleted.stdout.strip()}" + if strict: + raise SystemExit(message) + print(message, file=sys.stderr) + + def amp_review_plugin_source( prompt_path: Path, result_path: Path, @@ -1047,8 +1122,6 @@ def run_droid(args: argparse.Namespace, repo: Path, prompt: str) -> str: def run_copilot(args: argparse.Namespace, repo: Path, prompt: str) -> str: if args.thinking: raise SystemExit("--thinking is not supported by the copilot engine") - if not args.tools: - raise SystemExit("--no-tools is not supported by the copilot engine; copilot requires a read-only file view tool to load the review bundle without exposing it in argv") with tempfile.TemporaryDirectory(prefix="autoreview-copilot.") as tempdir: prompt_path = Path(tempdir) / "prompt.txt" prompt_path.write_text(prompt) @@ -1110,6 +1183,12 @@ def extract_json(text: str) -> dict[str, Any]: return parsed if isinstance(parsed, dict) and isinstance(parsed.get("structured_output"), dict): return parsed["structured_output"] + if isinstance(parsed, dict) and isinstance(parsed.get("structuredOutput"), dict): + # grok: a max-turns/cancelled run still carries the last provisional object. + stop_reason = parsed.get("stopReason") + if stop_reason not in (None, "end_turn"): + raise SystemExit(f"review engine stopped early ({stop_reason}); refusing partial structured output") + return parsed["structuredOutput"] if isinstance(parsed, dict) and isinstance(parsed.get("result"), dict) and "findings" in parsed["result"]: return parsed["result"] if isinstance(parsed, dict) and isinstance(parsed.get("result"), str): @@ -1371,7 +1450,7 @@ def parse_args() -> argparse.Namespace: action="append", help=( "Thinking/effort for all reviewers or engine=level. Repeatable. Overrides AUTOREVIEW_THINKING. " - "Codex/Claude: low, medium, high, xhigh, max. Amp (agent mode): low, medium, high, ultra. " + "Codex/Claude: low, medium, high, xhigh, max. Amp (agent mode): low, medium, high, ultra. Grok: low, medium, high, xhigh. " "Pi: off, minimal, low, medium, high, xhigh, max. OpenCode: minimal, low, medium, high, max." ), ) @@ -1385,6 +1464,7 @@ def parse_args() -> argparse.Namespace: parser.add_argument("--codex-bin", default=os.environ.get("CODEX_BIN", "codex")) parser.add_argument("--claude-bin", default=os.environ.get("CLAUDE_BIN", "claude")) parser.add_argument("--amp-bin", default=os.environ.get("AMP_BIN", "amp")) + parser.add_argument("--grok-bin", default=os.environ.get("GROK_BIN", "grok")) parser.add_argument("--droid-bin", default=os.environ.get("DROID_BIN", "droid")) parser.add_argument("--copilot-bin", default=os.environ.get("COPILOT_BIN", "copilot")) parser.add_argument("--pi-bin", default=os.environ.get("PI_BIN", "pi")) @@ -1395,7 +1475,7 @@ def parse_args() -> argparse.Namespace: default=os.environ.get("AUTOREVIEW_STREAM_ENGINE_OUTPUT") == "1", help="Stream engine output while preserving buffered output for validation. Codex and Claude filter noisy tool/status chatter.", ) - parser.add_argument("--no-tools", dest="tools", action="store_false", default=True, help="Disable tools for engines that support it. Codex and copilot reject no-tools review. Amp always reviews the bundle alone; pi gets read-only tools.") + parser.add_argument("--no-tools", dest="tools", action="store_false", default=True, help="Disable tools for engines that support it. Codex, grok, and copilot reject no-tools review. Amp always reviews the bundle alone; pi gets read-only tools.") parser.add_argument("--no-web-search", dest="web_search", action="store_false", default=True) parser.add_argument( "--claude-allowed-tools", @@ -1428,6 +1508,8 @@ def run_engine(args: argparse.Namespace, repo: Path, prompt: str) -> str: return run_claude(args, repo, prompt) if args.engine == "amp": return run_amp(args, repo, prompt) + if args.engine == "grok": + return run_grok(args, repo, prompt) if args.engine == "droid": return run_droid(args, repo, prompt) if args.engine == "copilot": @@ -1503,12 +1585,9 @@ def reviewer_args(args: argparse.Namespace) -> list[argparse.Namespace]: else: reviewers = [(args.engine, None, None)] - seen: set[str] = set() + seen: set[tuple[str, str | None]] = set() result: list[argparse.Namespace] = [] for engine, inline_model, inline_thinking in reviewers: - if engine in seen: - raise SystemExit(f"reviewer specified more than once: {engine}") - seen.add(engine) model = ( inline_model or model_by_engine.get(engine) @@ -1525,6 +1604,10 @@ def reviewer_args(args: argparse.Namespace) -> list[argparse.Namespace]: or env_global_thinking or DEFAULT_THINKING_BY_ENGINE.get(engine) ) + # The same engine may appear with different models (amp panels: sol + grok). + if (engine, model) in seen: + raise SystemExit(f"reviewer specified more than once: {engine} model={model}") + seen.add((engine, model)) if engine == "codex" and model == DEFAULT_MODEL_BY_ENGINE["codex"]: fallback_model = DEFAULT_CODEX_ACCESS_FALLBACK_MODEL else: @@ -1621,6 +1704,7 @@ ENGINE_BIN_ATTR = { "codex": "codex_bin", "claude": "claude_bin", "amp": "amp_bin", + "grok": "grok_bin", "droid": "droid_bin", "copilot": "copilot_bin", "pi": "pi_bin", @@ -1628,6 +1712,13 @@ ENGINE_BIN_ATTR = { } +def validate_engine_options(args: argparse.Namespace, reviewers: list[argparse.Namespace]) -> None: + """Engine-runtime checks shared by dry-run and live runs; after the empty-diff guard.""" + for reviewer in reviewers: + if not args.tools and reviewer.engine in NO_TOOLS_UNSUPPORTED: + raise SystemExit(NO_TOOLS_UNSUPPORTED[reviewer.engine]) + + def dry_run_preflight(reviewers: list[argparse.Namespace]) -> int: """Validate everything a real run needs except the engine calls: the bundle/prompt built above and each reviewer's CLI binary.""" @@ -1693,6 +1784,7 @@ def main() -> int: if not changed_paths: print("empty diff: nothing to review") return 0 + validate_engine_options(args, reviewers) if args.dry_run: return dry_run_preflight(reviewers) diff --git a/modules/ai/skills/autoreview/scripts/test-review-harness b/modules/ai/skills/autoreview/scripts/test-review-harness index 53862c4..da682a2 100755 --- a/modules/ai/skills/autoreview/scripts/test-review-harness +++ b/modules/ai/skills/autoreview/scripts/test-review-harness @@ -3,7 +3,7 @@ set -euo pipefail usage() { cat <<'EOF' -Usage: test-review-harness [--fixture malicious|benign] [--vcs git|jj] [--engine codex|claude|amp|droid|copilot|pi|opencode]... +Usage: test-review-harness [--fixture malicious|benign] [--vcs git|jj] [--engine codex|claude|amp|grok|droid|copilot|pi|opencode]... Creates a temporary Git or non-colocated jj repo with either a deliberately unsafe patch or a security-sensitive-but-safe patch, then verifies each selected engine through