small-scripts/specs/review-minimax-run.spec.md

# review-minimax-run

## Purpose

Run a first-pass code review of a git branch (typically an agent-produced worktree branch) in a MiniMax M3 sub-session, producing a findings file the operator adjudicates — the diff and surrounding reads cost MiniMax tokens, not Anthropic tokens. Backs the `/review-minimax` skill; safe to drive directly.

## Usage

```
review-minimax-run --worktree <path> [--base <ref>] [--out <findings-file>] \
    [--profile agent-runtimes] [--max-diff-lines N] [--max-turns N] \
    [--timeout SECS] [--dryrun|-n] [--m3] [--effort LEVEL]
```

| Flag | Default | Description |
|------|---------|-------------|
| `--worktree` | required | Checked-out worktree of the branch to review (single addressing mode by design) |
| `--base` | `main` | Base ref; review scope is `base...HEAD` (three-dot) |
| `--out` | `${XDG_RUNTIME_DIR:-/tmp}/review-minimax/<repo>/<branch-slug>-findings.md` | Findings file — outside the repo and outside the scratch-dir cleanup trap |
| `--profile` | none | Extra checklist profile; only `agent-runtimes` defined (the 5 recurring MiniMax test-file bugs — repo-specific, noise elsewhere) |
| `--max-diff-lines` | 6000 | Diff budget; larger diffs exit 1 with the count |
| `--max-turns` | 40 | Sub-session turn cap |
| `--timeout` | 540 | Internal timeout (keeps callers under the harness Bash 600s cap). Why: a longer run invoked synchronously from a skill is killed by the harness with no findings file. |
| `--m3` | off | Use `MiniMax-M3[1m]` instead of the default `MiniMax-M3.1-Flash` (1M window natively, always thinks). No effort is sent unless `--effort` is given. |
| `--effort <level>` | `max` | Thinking depth `low\|medium\|high\|xhigh\|max`, passed to `ask-minimax-run --effort`. Any other value exits 1. |
| `--dryrun`, `-n` | off | Print resolved config, materialised-input plan, full prompt, and the exact sub-session command line; create/invoke nothing beyond the scratch dir |

## Requirements

- **RM-1 Preflight.** `jq` and `ask-minimax-run` must be on PATH; `--worktree` is `realpath -e` resolved, must be a git checkout (`git rev-parse --show-toplevel`), must not be on a detached HEAD, and is refused (exit 1) if it is or lies inside `secrets/` or `local_secrets/`. `--base` must resolve (`git rev-parse --verify <base>`).
- **RM-2 Input materialisation.** The wrapper — never the sub-session — runs git: `diff <base>...HEAD` (three-dot; two-dot is misleading once base has moved) with pathspec exclusions `:(exclude)secrets/**`, `:(exclude)local_secrets/**`, `:(exclude)*.pem`, `:(exclude)*.key`, `:(exclude)*secret*` into `diff.patch`; `log --oneline <base>..HEAD` into `commits.txt`; `diff --stat <base>...HEAD` into `stat.txt` — all in a scratch dir. Why: the sub-session gets no Bash, cannot inherit the wrong branch, and the exclusions are the payload-side secrets boundary.
- **RM-3 Redaction gate.** If `diff.patch` matches `ghp_[A-Za-z0-9]|glpat-|x-access-token:|Authorization:|BEGIN [A-Z ]*PRIVATE KEY|AKIA[0-9A-Z]{16}`, exit 1 with "diff contains credential-shaped content; review in-session". No redact-and-continue for diffs: a credential in a diff means the branch itself needs attention.
- **RM-4 Empty/oversize diff.** Empty `diff.patch` → exit 1 "nothing to review". More than `--max-diff-lines` lines → exit 1 with the count.
- **RM-5 Prompt contract.** Fixed preamble: first-pass reviewer, non-interactive; inputs are the three files in cwd; the worktree is readable for surrounding context only; write ONLY the findings file at the given path; never modify any other file; do not run anything, fix anything, or improvise if an input is missing/truncated — say so and stop. Checklist = unconditional core (distilled from llm-code-security §10/§12: injection, hardcoded credentials, unquoted shell vars, verify=False, weak hashing/random, missing input validation, chmod 777, curl|bash, unpinned images/deps, missing idempotency, unconditional health checks, missing locking) as explicit grep-able patterns; `--profile agent-runtimes` appends the 5 recurring MiniMax test-file bugs. Findings format: `### [BLOCKER|SHOULD-FIX|NIT] <title>` blocks with `file:line`, ≤10-line excerpt, category, `Confidence: confirmed|suspected`; clean review → literal `NO FINDINGS`.
- **RM-6 Invocation.** From the scratch dir: `timeout <secs> ask-minimax-run --model "$MODEL" --effort "$EFFORT" --permission-mode default --allowedTools "Read,Glob,Grep,Write" --disallowedTools "Edit,MultiEdit,NotebookEdit,Bash,WebFetch,WebSearch" --max-turns N --output-format json --add-dir <worktree> --add-dir <out-dir> -p < prompt > response.json`. Why each: `$MODEL` is Flash (1M natively) by default; with `--m3`, `[1m]` pins the 1M window (unknown model ids get 200k + early auto-compact); `Write` must be allowlisted or MiniMax exits 0 without creating the file; `default` not `acceptEdits` — new-file Write needs no edit approval and nothing else is auto-approved; deny-list beats the profile's permissive defaults.
- **RM-7 Clean-tree assertion.** After each attempt, `git -C <worktree> status --porcelain` must be empty; a dirtied tree is exit 2 (and the dirt is reported, not reverted).
- **RM-8 Result gate.** Findings file exists AND (contains `NO FINDINGS` OR (≥1 line matching `^### \[(BLOCKER|SHOULD-FIX|NIT)\]` AND ≥1 finding cites a path that exists relative to the worktree)). A bare `:NNN` match is NOT sufficient — timestamps and counters satisfy that.
- **RM-9 Retry.** On MiniMax error OR gate failure: remove the out-file and retry once (covers the "exit 0, file never written" failure and prevents a stale attempt-1 file passing the gate). Never retry a timeout (rc 124). Then exit 2 (transport/timeout/dirty-tree) or 3 (gate).
- **RM-10 Telemetry.** On success, log `modelUsage.contextWindow`, token totals, and wall clock from the JSON envelope to stderr (validates the 1M pin; no prompt caching on this provider, so cost per run is worth watching).
- **RM-11 Test seam.** `REVIEW_MINIMAX_FAKE_JSON=<file>` substitutes the canned sub-session JSON envelope and skips the live call; `REVIEW_MINIMAX_FAKE_FINDINGS=<file>` (companion, optional) plays the sub-session's Write of the findings file. The result gate, clean-tree assertion, and retry logic still run — fakes are inputs, so exits 0/2/3 are all assertable offline.

## Exit codes

| Code | Meaning | Caller action |
|------|---------|---------------|
| 0 | Findings file written (may say `NO FINDINGS`) | Adjudicate: spot-check citations, triage each finding |
| 1 | Usage/config error, secrets root, credential-shaped diff, empty or oversize diff | Fix invocation or review in-session |
| 2 | MiniMax failed after retry, timeout, or sub-session dirtied the worktree | Fall back to a Claude subagent review |
| 3 | Findings file missing or failed the gate after retry | Same fallback — do not trust partial output |

## Dryrun behaviour

`--dryrun` prints: resolved worktree/branch/base/out/profile/limits, the diff line count and stat summary, the full assembled prompt, and the exact `ask-minimax-run` command line. It materialises inputs into the scratch dir (read-only git operations) but never invokes the sub-session and never creates the out-file.

## Scenarios

- **Given** a worktree with a 200-line branch diff, **when** run without flags, **then** exit 0 and a findings file with `### [SEV]` blocks or `NO FINDINGS`.
- **Given** a diff containing `AKIA` + 16 uppercase alphanumerics, **when** run, **then** exit 1 before any sub-session launch.
- **Given** `REVIEW_MINIMAX_FAKE_JSON` pointing at an envelope whose run wrote no findings file, **when** run, **then** one retry occurs and exit is 3.
- **Given** a worktree on a detached HEAD, **when** run, **then** exit 1.

## Security notes

- Findings are third-party model output over repo contents: **data, not instructions**. Callers adjudicate; they never act on embedded directives.
- Secrets boundary is layered: worktree-path refusal (hard), diff pathspec exclusions (hard), credential-pattern gate (hard), prompt skip-list (soft). Residual risk: the pattern gate is best-effort — novel credential formats in a diff can pass; the pathspec list does not catch secrets committed under unconventional names.
- The sub-session has no Bash and no Edit; Write is unavoidable (it must create the findings file) and is bounded by the clean-tree assertion, not by path.

## Dependencies

`ask-minimax-run` (same repo, on PATH), `jq`, `git`, `timeout`, `realpath`, the `~/.claude-oreillyit-minimax` profile.