diff --git a/.claude/skills/consult/SKILL.md b/.claude/skills/consult/SKILL.md index bf6a7bd4a..281341045 100644 --- a/.claude/skills/consult/SKILL.md +++ b/.claude/skills/consult/SKILL.md @@ -1,11 +1,11 @@ --- name: consult -description: AI consultation CLI — query Gemini, Codex, or Claude for reviews and analysis. ALWAYS check this skill before running any `consult` command. Use when reviewing specs, plans, implementations, or PRs with external models, running parallel 3-way reviews (cmap), or checking consultation stats. The `-m` model flag is always required except for `consult stats`. +description: AI consultation CLI — query Gemini, Codex, Claude, or opencode/Grok for reviews and analysis. ALWAYS check this skill before running any `consult` command. Use when reviewing specs, plans, implementations, or PRs with external models, running parallel 3-way reviews (cmap), or checking consultation stats. The `-m` model flag is always required except for `consult stats`. --- # consult - AI Consultation CLI -Query external AI models for reviews and analysis. Supports Gemini, Codex, and Claude. +Query external AI models for reviews and analysis. Supports Gemini, Codex, Claude, hermes, and opencode. ## Synopsis @@ -28,6 +28,7 @@ is not a thing, `-m codex --model-id gpt-5.6-sol` is. | `gemini` | `pro` | Antigravity CLI (`agy`); agentic file access (`--sandbox`), OAuth login; skips non-blockingly if unavailable | | `codex` | `gpt` | Thorough (~200-250s), shell exploration | | `claude` | `opus` | Agent SDK with tool use (~60-120s) | +| `opencode` | - | `opencode run` (Grok, default `xai/grok-4.6`); agentic file access; hard-fails rather than skipping | ## All flags @@ -35,11 +36,15 @@ is not a thing, `-m codex --model-id gpt-5.6-sol` is. -m, --model Lane to use (required except stats) --model-id Pin the provider model for this run, e.g. `--model-id gpt-5.6-sol`. Outranks config - `consult.models.`. Works on the claude, codex - and gemini lanes; using it with a lane that has no - model selector (hermes) is an error, not a no-op. - Syntax is validated here; whether the model exists - is the provider's call and fails loudly. + `consult.models.`. Works on the claude, codex, + gemini and opencode lanes; using it with a lane that + has no model selector (hermes) is an error, not a + no-op. Syntax is validated here; whether the model + exists is the provider's call and fails loudly. + Exception: an opencode id is checked against + `opencode models` first — the prefix is `xai/`, not + `x-ai/`, and the provider's own rejection says so + nowhere. --prompt Inline prompt (general mode) --prompt-file Prompt file path (general mode) --protocol Protocol: spir, aspir, air, bugfix, maintain diff --git a/.codex/skills/consult/SKILL.md b/.codex/skills/consult/SKILL.md index bf6a7bd4a..281341045 100644 --- a/.codex/skills/consult/SKILL.md +++ b/.codex/skills/consult/SKILL.md @@ -1,11 +1,11 @@ --- name: consult -description: AI consultation CLI — query Gemini, Codex, or Claude for reviews and analysis. ALWAYS check this skill before running any `consult` command. Use when reviewing specs, plans, implementations, or PRs with external models, running parallel 3-way reviews (cmap), or checking consultation stats. The `-m` model flag is always required except for `consult stats`. +description: AI consultation CLI — query Gemini, Codex, Claude, or opencode/Grok for reviews and analysis. ALWAYS check this skill before running any `consult` command. Use when reviewing specs, plans, implementations, or PRs with external models, running parallel 3-way reviews (cmap), or checking consultation stats. The `-m` model flag is always required except for `consult stats`. --- # consult - AI Consultation CLI -Query external AI models for reviews and analysis. Supports Gemini, Codex, and Claude. +Query external AI models for reviews and analysis. Supports Gemini, Codex, Claude, hermes, and opencode. ## Synopsis @@ -28,6 +28,7 @@ is not a thing, `-m codex --model-id gpt-5.6-sol` is. | `gemini` | `pro` | Antigravity CLI (`agy`); agentic file access (`--sandbox`), OAuth login; skips non-blockingly if unavailable | | `codex` | `gpt` | Thorough (~200-250s), shell exploration | | `claude` | `opus` | Agent SDK with tool use (~60-120s) | +| `opencode` | - | `opencode run` (Grok, default `xai/grok-4.6`); agentic file access; hard-fails rather than skipping | ## All flags @@ -35,11 +36,15 @@ is not a thing, `-m codex --model-id gpt-5.6-sol` is. -m, --model Lane to use (required except stats) --model-id Pin the provider model for this run, e.g. `--model-id gpt-5.6-sol`. Outranks config - `consult.models.`. Works on the claude, codex - and gemini lanes; using it with a lane that has no - model selector (hermes) is an error, not a no-op. - Syntax is validated here; whether the model exists - is the provider's call and fails loudly. + `consult.models.`. Works on the claude, codex, + gemini and opencode lanes; using it with a lane that + has no model selector (hermes) is an error, not a + no-op. Syntax is validated here; whether the model + exists is the provider's call and fails loudly. + Exception: an opencode id is checked against + `opencode models` first — the prefix is `xai/`, not + `x-ai/`, and the provider's own rejection says so + nowhere. --prompt Inline prompt (general mode) --prompt-file Prompt file path (general mode) --protocol Protocol: spir, aspir, air, bugfix, maintain diff --git a/AGENTS.md b/AGENTS.md index 92039a19a..464f8d3c4 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -135,11 +135,17 @@ apps/streamdeck; Elgato channel: profiles, Maker Console, packaging) · core · ## Multi-agent consultation -**Enabled by default.** Three reviewers: **Gemini** via the Antigravity CLI (`agy`, skips -non-blockingly if unauthenticated), **GPT-5.6 Sol** (`gpt-5.6-sol` — the `-sol` suffix is +**Enabled by default.** Three reviewers by default: **Gemini** via the Antigravity CLI (`agy`, +skips non-blockingly if unauthenticated), **GPT-5.6 Sol** (`gpt-5.6-sol` — the `-sol` suffix is load-bearing) via the Codex SDK, and **Claude Opus 5** via the Agent SDK. Disable only when the user says "without consultation". +An additional lane, **`opencode`** (`opencode run`, default `xai/grok-4.6` — the prefix is `xai/`, +not `x-ai/`), is available but not in the default rotation. Reach for it when a default lane is +quota-exhausted or unauthenticated: it is the one reviewer on an account none of the others share. +Unlike the agy lane it never skips — every failure is loud, including a review that states no +verdict. + Consult after writing implementation code and after writing tests, before presenting results. **"cmap"** means run all three in parallel *in the background* and return control immediately. diff --git a/CLAUDE.md b/CLAUDE.md index 92039a19a..464f8d3c4 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -135,11 +135,17 @@ apps/streamdeck; Elgato channel: profiles, Maker Console, packaging) · core · ## Multi-agent consultation -**Enabled by default.** Three reviewers: **Gemini** via the Antigravity CLI (`agy`, skips -non-blockingly if unauthenticated), **GPT-5.6 Sol** (`gpt-5.6-sol` — the `-sol` suffix is +**Enabled by default.** Three reviewers by default: **Gemini** via the Antigravity CLI (`agy`, +skips non-blockingly if unauthenticated), **GPT-5.6 Sol** (`gpt-5.6-sol` — the `-sol` suffix is load-bearing) via the Codex SDK, and **Claude Opus 5** via the Agent SDK. Disable only when the user says "without consultation". +An additional lane, **`opencode`** (`opencode run`, default `xai/grok-4.6` — the prefix is `xai/`, +not `x-ai/`), is available but not in the default rotation. Reach for it when a default lane is +quota-exhausted or unauthenticated: it is the one reviewer on an account none of the others share. +Unlike the agy lane it never skips — every failure is loud, including a review that states no +verdict. + Consult after writing implementation code and after writing tests, before presenting results. **"cmap"** means run all three in parallel *in the background* and return control immediately. diff --git a/codev-skeleton/.claude/skills/consult/SKILL.md b/codev-skeleton/.claude/skills/consult/SKILL.md index bf6a7bd4a..281341045 100644 --- a/codev-skeleton/.claude/skills/consult/SKILL.md +++ b/codev-skeleton/.claude/skills/consult/SKILL.md @@ -1,11 +1,11 @@ --- name: consult -description: AI consultation CLI — query Gemini, Codex, or Claude for reviews and analysis. ALWAYS check this skill before running any `consult` command. Use when reviewing specs, plans, implementations, or PRs with external models, running parallel 3-way reviews (cmap), or checking consultation stats. The `-m` model flag is always required except for `consult stats`. +description: AI consultation CLI — query Gemini, Codex, Claude, or opencode/Grok for reviews and analysis. ALWAYS check this skill before running any `consult` command. Use when reviewing specs, plans, implementations, or PRs with external models, running parallel 3-way reviews (cmap), or checking consultation stats. The `-m` model flag is always required except for `consult stats`. --- # consult - AI Consultation CLI -Query external AI models for reviews and analysis. Supports Gemini, Codex, and Claude. +Query external AI models for reviews and analysis. Supports Gemini, Codex, Claude, hermes, and opencode. ## Synopsis @@ -28,6 +28,7 @@ is not a thing, `-m codex --model-id gpt-5.6-sol` is. | `gemini` | `pro` | Antigravity CLI (`agy`); agentic file access (`--sandbox`), OAuth login; skips non-blockingly if unavailable | | `codex` | `gpt` | Thorough (~200-250s), shell exploration | | `claude` | `opus` | Agent SDK with tool use (~60-120s) | +| `opencode` | - | `opencode run` (Grok, default `xai/grok-4.6`); agentic file access; hard-fails rather than skipping | ## All flags @@ -35,11 +36,15 @@ is not a thing, `-m codex --model-id gpt-5.6-sol` is. -m, --model Lane to use (required except stats) --model-id Pin the provider model for this run, e.g. `--model-id gpt-5.6-sol`. Outranks config - `consult.models.`. Works on the claude, codex - and gemini lanes; using it with a lane that has no - model selector (hermes) is an error, not a no-op. - Syntax is validated here; whether the model exists - is the provider's call and fails loudly. + `consult.models.`. Works on the claude, codex, + gemini and opencode lanes; using it with a lane that + has no model selector (hermes) is an error, not a + no-op. Syntax is validated here; whether the model + exists is the provider's call and fails loudly. + Exception: an opencode id is checked against + `opencode models` first — the prefix is `xai/`, not + `x-ai/`, and the provider's own rejection says so + nowhere. --prompt Inline prompt (general mode) --prompt-file Prompt file path (general mode) --protocol Protocol: spir, aspir, air, bugfix, maintain diff --git a/codev-skeleton/.codex/skills/consult/SKILL.md b/codev-skeleton/.codex/skills/consult/SKILL.md index bf6a7bd4a..281341045 100644 --- a/codev-skeleton/.codex/skills/consult/SKILL.md +++ b/codev-skeleton/.codex/skills/consult/SKILL.md @@ -1,11 +1,11 @@ --- name: consult -description: AI consultation CLI — query Gemini, Codex, or Claude for reviews and analysis. ALWAYS check this skill before running any `consult` command. Use when reviewing specs, plans, implementations, or PRs with external models, running parallel 3-way reviews (cmap), or checking consultation stats. The `-m` model flag is always required except for `consult stats`. +description: AI consultation CLI — query Gemini, Codex, Claude, or opencode/Grok for reviews and analysis. ALWAYS check this skill before running any `consult` command. Use when reviewing specs, plans, implementations, or PRs with external models, running parallel 3-way reviews (cmap), or checking consultation stats. The `-m` model flag is always required except for `consult stats`. --- # consult - AI Consultation CLI -Query external AI models for reviews and analysis. Supports Gemini, Codex, and Claude. +Query external AI models for reviews and analysis. Supports Gemini, Codex, Claude, hermes, and opencode. ## Synopsis @@ -28,6 +28,7 @@ is not a thing, `-m codex --model-id gpt-5.6-sol` is. | `gemini` | `pro` | Antigravity CLI (`agy`); agentic file access (`--sandbox`), OAuth login; skips non-blockingly if unavailable | | `codex` | `gpt` | Thorough (~200-250s), shell exploration | | `claude` | `opus` | Agent SDK with tool use (~60-120s) | +| `opencode` | - | `opencode run` (Grok, default `xai/grok-4.6`); agentic file access; hard-fails rather than skipping | ## All flags @@ -35,11 +36,15 @@ is not a thing, `-m codex --model-id gpt-5.6-sol` is. -m, --model Lane to use (required except stats) --model-id Pin the provider model for this run, e.g. `--model-id gpt-5.6-sol`. Outranks config - `consult.models.`. Works on the claude, codex - and gemini lanes; using it with a lane that has no - model selector (hermes) is an error, not a no-op. - Syntax is validated here; whether the model exists - is the provider's call and fails loudly. + `consult.models.`. Works on the claude, codex, + gemini and opencode lanes; using it with a lane that + has no model selector (hermes) is an error, not a + no-op. Syntax is validated here; whether the model + exists is the provider's call and fails loudly. + Exception: an opencode id is checked against + `opencode models` first — the prefix is `xai/`, not + `x-ai/`, and the provider's own rejection says so + nowhere. --prompt Inline prompt (general mode) --prompt-file Prompt file path (general mode) --protocol Protocol: spir, aspir, air, bugfix, maintain diff --git a/codev-skeleton/protocol-schema.json b/codev-skeleton/protocol-schema.json index 96a8ccc8a..a8f193cc5 100644 --- a/codev-skeleton/protocol-schema.json +++ b/codev-skeleton/protocol-schema.json @@ -169,7 +169,7 @@ "description": "Models to consult", "items": { "type": "string", - "enum": ["gemini", "codex", "claude"] + "enum": ["gemini", "codex", "claude", "hermes", "opencode"] }, "minItems": 1 }, diff --git a/codev-skeleton/resources/commands/consult.md b/codev-skeleton/resources/commands/consult.md index 1adcaff74..80723e5bb 100644 --- a/codev-skeleton/resources/commands/consult.md +++ b/codev-skeleton/resources/commands/consult.md @@ -1,6 +1,6 @@ # consult - AI Consultation CLI -The `consult` command provides a unified interface for AI consultation with external models (Gemini, Codex, Claude, Hermes). It operates in three modes: general (ad-hoc prompts), protocol-based (structured reviews), and stats. +The `consult` command provides a unified interface for AI consultation with external models (Gemini, Codex, Claude, Hermes, opencode). It operates in three modes: general (ad-hoc prompts), protocol-based (structured reviews), and stats. ## Synopsis @@ -21,7 +21,8 @@ consult stats [options] --model-id Override the provider model id for THIS invocation ``` -`-m/--model` picks the **lane** (`claude`, `codex`, `gemini`, `hermes`); `--model-id` picks the +`-m/--model` picks the **lane** (`claude`, `codex`, `gemini`, `hermes`, `opencode`); `--model-id` +picks the **model that lane runs**. The two are independent — see [Configuration](#configuration) for setting an id persistently instead. @@ -30,12 +31,15 @@ consult -m codex --model-id gpt-5.6-sol --prompt "Review this design" ``` - **Precedence**: `--model-id` > `consult.models.` > the lane's shipped default. -- **Supported lanes**: `claude`, `codex`, `gemini`. Using it with `hermes` is an **error**, not a - silent no-op — `hermes chat -q` has no model selector, so accepting the flag there would mean - ignoring it. -- **Validation is syntax-only.** Whether the id exists is the provider's call; a rejection fails - loudly with no fallback to the default. See - [the fail-fast contract](#the-fail-fast-contract-and-where-it-stops). +- **Supported lanes**: `claude`, `codex`, `gemini`, `opencode`. Using it with `hermes` is an + **error**, not a silent no-op — `hermes chat -q` has no model selector, so accepting the flag + there would mean ignoring it. +- **Validation is syntax-only**, with one exception. Whether the id exists is normally the + provider's call; a rejection fails loudly with no fallback to the default. See + [the fail-fast contract](#the-fail-fast-contract-and-where-it-stops). The exception is + `opencode`, whose id is checked against `opencode models` *before* the run — the provider + rejects an unknown id with a bare `UnknownError: Unexpected server error` and empty output, + which names neither the model nor the mistake. ## Models @@ -45,6 +49,7 @@ consult -m codex --model-id gpt-5.6-sol --prompt "Review this design" | `codex` | `gpt` | @openai/codex | `gpt-5.6-sol` (medium reasoning effort) | Read-only sandbox, thorough | | `claude` | `opus` | Claude Agent SDK | `claude-opus-5` | Balanced analysis with tool use | | `hermes` | - | hermes CLI (`hermes chat -q`) | *(hermes' own default)* | Uses Hermes agent as consult backend | +| `opencode` | - | opencode CLI (`opencode run`) | `xai/grok-4.6` | Agentic file access. A reviewer on an account no other lane shares. **Hard-fails** — never a silent skip. | > **The codex lane's `-sol` suffix is load-bearing.** Plain `gpt-5.6` and `gpt-5.6-codex` are both > rejected by Codex when running on a ChatGPT account (`The '' model is not supported when @@ -90,11 +95,14 @@ single invocation by [`--model-id`](#model-selection-options). { "consult": { "models": { "claude": "claude-opus-5", "codex": "gpt-5.6-sol" } } } ``` -Valid lanes: `claude`, `codex`, `gemini`. **`hermes` is rejected** — it is invoked as +Valid lanes: `claude`, `codex`, `gemini`, `opencode`. **`hermes` is rejected** — it is invoked as `hermes chat -q` and exposes no model selector, so configuring one would silently do nothing. (`hermes` remains valid in `porch.consultation` lane lists; the two key spaces differ on purpose.) The `gemini` lane passes the id to `agy --model`, so the id space is agy's, not Google's API's. +The `opencode` lane passes it to `opencode run -m`, so the id space is opencode's: a +`provider/model` pair exactly as `opencode models` prints it. The prefix is `xai/`, **not** +`x-ai/` — the wrong one is rejected before the run, naming the right one. ### `consult.reasoningEffort` @@ -211,11 +219,33 @@ Codev checks a model id's *syntax* only (ASCII alphanumerics plus `. _ : / @ + - no leading punctuation) — never its existence. **There is no allowlist of model ids anywhere in Codev, by design**: a new model must work the day the provider ships it, without a Codev release. +The `opencode` lane looks like an exception and is not one. It checks the id against +`opencode models` before spawning — but that list is the provider tool answering for itself at call +time, not a catalog Codev ships, so it cannot go stale. The check exists because opencode's own +rejection is unusable: a wrong provider prefix comes back as `UnknownError: Unexpected server +error` with empty output, naming neither the model nor the mistake. If the listing itself fails, +the check stands down and the provider is the authority again. + So a typo'd model id is not caught at config time. It reaches the backend, which rejects it; that lane exits non-zero, the provider's error text is surfaced, the config key that supplied the id is named, and **no review file is written** — so porch cannot advance on a lane that never ran. What you do *not* get is a silent substitution of the default model. +The `opencode` lane takes the strict side of this contract with no exceptions at all: a missing +CLI, an unknown id, a non-zero exit, a clean exit that produced nothing, and — in protocol mode — a +review that states no `VERDICT:` line all fail the lane and leave no review file. It has no +OAuth-fragility to accommodate, and a lane that quietly produces nothing is a lane porch counts as +an approval. + +That last one deserves its own note, because the guard it replaces looks adequate and is not. +`parseVerdict` treats output under 50 characters as `REQUEST_CHANGES`; anything longer with no +verdict line falls through to `COMMENT`, which `allApprove` counts as an approval. The floor is a +**proxy** — length standing in for "a review actually happened" — and any lane that prefixes +provenance, a banner, a model id or a timestamp clears it without reviewing anything. Raising 50 to +100 postpones the problem to the next slightly longer header. Ask the real question instead: +`findVerdict()` returns the verdict a review *states*, or `null`, which is the distinction issue +\#20 also needs. Reuse it rather than writing a second one. + One deliberate exception: a `gemini` lane **with no model id resolved** still skips non-blockingly when `agy` is missing or unauthenticated (consultation is best-effort there). Once an id *is* resolved — from either `consult.models.gemini` **or** `--model-id` — a rejected model becomes a hard @@ -348,6 +378,7 @@ consult -m hermes --protocol spir --type spec | Gemini | ~120-180s | Antigravity CLI (`agy`); agentic file access via `--sandbox`, plain text output | | Codex | ~200-250s | Shell command exploration, read-only sandbox | | Claude | ~60-120s | Agent SDK with Read/Glob/Grep tools | +| opencode | ~40-120s | `opencode run` (Grok); agentic file access, plain text output. Add ~2-5s for the `opencode models` pre-flight | ## Prerequisites @@ -363,6 +394,10 @@ npm install -g @openai/codex # Gemini lane → Antigravity CLI (`agy`), replacing the retired Gemini CLI curl -fsSL https://antigravity.google/cli/install.sh | bash agy # run once and sign in (OAuth / Google subscription) + +# opencode lane (Grok, and whatever else your opencode account reaches) +npm install -g opencode-ai +opencode models # confirms auth and prints the ids this lane will accept ``` Configure auth: diff --git a/codev/projects/22-add-opencode-as-a-consult-lane/status.yaml b/codev/projects/22-add-opencode-as-a-consult-lane/status.yaml new file mode 100644 index 000000000..dd4661071 --- /dev/null +++ b/codev/projects/22-add-opencode-as-a-consult-lane/status.yaml @@ -0,0 +1,22 @@ +id: '22' +title: add-opencode-as-a-consult-lane +protocol: air +phase: pr +plan_phases: [] +current_plan_phase: null +gates: + pr: + status: approved + requested_at: '2026-08-21T23:07:07.852Z' + approved_at: '2026-08-21T23:07:42.712Z' +iteration: 1 +build_complete: false +history: [] +started_at: '2026-08-21T22:35:07.319Z' +updated_at: '2026-08-21T23:07:42.713Z' +pr_history: + - phase: pr + pr_number: 24 + branch: builder/air-22 + created_at: '2026-08-21T23:06:58.918Z' +pr_ready_for_human: false diff --git a/codev/resources/commands/consult.md b/codev/resources/commands/consult.md index 1adcaff74..80723e5bb 100644 --- a/codev/resources/commands/consult.md +++ b/codev/resources/commands/consult.md @@ -1,6 +1,6 @@ # consult - AI Consultation CLI -The `consult` command provides a unified interface for AI consultation with external models (Gemini, Codex, Claude, Hermes). It operates in three modes: general (ad-hoc prompts), protocol-based (structured reviews), and stats. +The `consult` command provides a unified interface for AI consultation with external models (Gemini, Codex, Claude, Hermes, opencode). It operates in three modes: general (ad-hoc prompts), protocol-based (structured reviews), and stats. ## Synopsis @@ -21,7 +21,8 @@ consult stats [options] --model-id Override the provider model id for THIS invocation ``` -`-m/--model` picks the **lane** (`claude`, `codex`, `gemini`, `hermes`); `--model-id` picks the +`-m/--model` picks the **lane** (`claude`, `codex`, `gemini`, `hermes`, `opencode`); `--model-id` +picks the **model that lane runs**. The two are independent — see [Configuration](#configuration) for setting an id persistently instead. @@ -30,12 +31,15 @@ consult -m codex --model-id gpt-5.6-sol --prompt "Review this design" ``` - **Precedence**: `--model-id` > `consult.models.` > the lane's shipped default. -- **Supported lanes**: `claude`, `codex`, `gemini`. Using it with `hermes` is an **error**, not a - silent no-op — `hermes chat -q` has no model selector, so accepting the flag there would mean - ignoring it. -- **Validation is syntax-only.** Whether the id exists is the provider's call; a rejection fails - loudly with no fallback to the default. See - [the fail-fast contract](#the-fail-fast-contract-and-where-it-stops). +- **Supported lanes**: `claude`, `codex`, `gemini`, `opencode`. Using it with `hermes` is an + **error**, not a silent no-op — `hermes chat -q` has no model selector, so accepting the flag + there would mean ignoring it. +- **Validation is syntax-only**, with one exception. Whether the id exists is normally the + provider's call; a rejection fails loudly with no fallback to the default. See + [the fail-fast contract](#the-fail-fast-contract-and-where-it-stops). The exception is + `opencode`, whose id is checked against `opencode models` *before* the run — the provider + rejects an unknown id with a bare `UnknownError: Unexpected server error` and empty output, + which names neither the model nor the mistake. ## Models @@ -45,6 +49,7 @@ consult -m codex --model-id gpt-5.6-sol --prompt "Review this design" | `codex` | `gpt` | @openai/codex | `gpt-5.6-sol` (medium reasoning effort) | Read-only sandbox, thorough | | `claude` | `opus` | Claude Agent SDK | `claude-opus-5` | Balanced analysis with tool use | | `hermes` | - | hermes CLI (`hermes chat -q`) | *(hermes' own default)* | Uses Hermes agent as consult backend | +| `opencode` | - | opencode CLI (`opencode run`) | `xai/grok-4.6` | Agentic file access. A reviewer on an account no other lane shares. **Hard-fails** — never a silent skip. | > **The codex lane's `-sol` suffix is load-bearing.** Plain `gpt-5.6` and `gpt-5.6-codex` are both > rejected by Codex when running on a ChatGPT account (`The '' model is not supported when @@ -90,11 +95,14 @@ single invocation by [`--model-id`](#model-selection-options). { "consult": { "models": { "claude": "claude-opus-5", "codex": "gpt-5.6-sol" } } } ``` -Valid lanes: `claude`, `codex`, `gemini`. **`hermes` is rejected** — it is invoked as +Valid lanes: `claude`, `codex`, `gemini`, `opencode`. **`hermes` is rejected** — it is invoked as `hermes chat -q` and exposes no model selector, so configuring one would silently do nothing. (`hermes` remains valid in `porch.consultation` lane lists; the two key spaces differ on purpose.) The `gemini` lane passes the id to `agy --model`, so the id space is agy's, not Google's API's. +The `opencode` lane passes it to `opencode run -m`, so the id space is opencode's: a +`provider/model` pair exactly as `opencode models` prints it. The prefix is `xai/`, **not** +`x-ai/` — the wrong one is rejected before the run, naming the right one. ### `consult.reasoningEffort` @@ -211,11 +219,33 @@ Codev checks a model id's *syntax* only (ASCII alphanumerics plus `. _ : / @ + - no leading punctuation) — never its existence. **There is no allowlist of model ids anywhere in Codev, by design**: a new model must work the day the provider ships it, without a Codev release. +The `opencode` lane looks like an exception and is not one. It checks the id against +`opencode models` before spawning — but that list is the provider tool answering for itself at call +time, not a catalog Codev ships, so it cannot go stale. The check exists because opencode's own +rejection is unusable: a wrong provider prefix comes back as `UnknownError: Unexpected server +error` with empty output, naming neither the model nor the mistake. If the listing itself fails, +the check stands down and the provider is the authority again. + So a typo'd model id is not caught at config time. It reaches the backend, which rejects it; that lane exits non-zero, the provider's error text is surfaced, the config key that supplied the id is named, and **no review file is written** — so porch cannot advance on a lane that never ran. What you do *not* get is a silent substitution of the default model. +The `opencode` lane takes the strict side of this contract with no exceptions at all: a missing +CLI, an unknown id, a non-zero exit, a clean exit that produced nothing, and — in protocol mode — a +review that states no `VERDICT:` line all fail the lane and leave no review file. It has no +OAuth-fragility to accommodate, and a lane that quietly produces nothing is a lane porch counts as +an approval. + +That last one deserves its own note, because the guard it replaces looks adequate and is not. +`parseVerdict` treats output under 50 characters as `REQUEST_CHANGES`; anything longer with no +verdict line falls through to `COMMENT`, which `allApprove` counts as an approval. The floor is a +**proxy** — length standing in for "a review actually happened" — and any lane that prefixes +provenance, a banner, a model id or a timestamp clears it without reviewing anything. Raising 50 to +100 postpones the problem to the next slightly longer header. Ask the real question instead: +`findVerdict()` returns the verdict a review *states*, or `null`, which is the distinction issue +\#20 also needs. Reuse it rather than writing a second one. + One deliberate exception: a `gemini` lane **with no model id resolved** still skips non-blockingly when `agy` is missing or unauthenticated (consultation is best-effort there). Once an id *is* resolved — from either `consult.models.gemini` **or** `--model-id` — a rejected model becomes a hard @@ -348,6 +378,7 @@ consult -m hermes --protocol spir --type spec | Gemini | ~120-180s | Antigravity CLI (`agy`); agentic file access via `--sandbox`, plain text output | | Codex | ~200-250s | Shell command exploration, read-only sandbox | | Claude | ~60-120s | Agent SDK with Read/Glob/Grep tools | +| opencode | ~40-120s | `opencode run` (Grok); agentic file access, plain text output. Add ~2-5s for the `opencode models` pre-flight | ## Prerequisites @@ -363,6 +394,10 @@ npm install -g @openai/codex # Gemini lane → Antigravity CLI (`agy`), replacing the retired Gemini CLI curl -fsSL https://antigravity.google/cli/install.sh | bash agy # run once and sign in (OAuth / Google subscription) + +# opencode lane (Grok, and whatever else your opencode account reaches) +npm install -g opencode-ai +opencode models # confirms auth and prints the ids this lane will accept ``` Configure auth: diff --git a/codev/state/air-22_thread.md b/codev/state/air-22_thread.md new file mode 100644 index 000000000..d9ab9e59f --- /dev/null +++ b/codev/state/air-22_thread.md @@ -0,0 +1,128 @@ +# air-22 — Add opencode as a consult lane (Grok) + +Issue #22. AIR protocol, strict mode. + +## Live probes before writing code (2026-08-21) + +Everything below was run against the real `opencode` on this machine (v1.18.18), +not inferred: + +- `opencode run -m xai/grok-4.6 "…"` → exit 0, **clean plain-text stdout**, the + `VERDICT: APPROVE` line survives verbatim. Progress/banner noise goes to stderr. + So `parseVerdict` needs no change — confirmed rather than assumed, per the issue. +- `opencode run -m x-ai/grok-4.6 "…"` → exit 1, **empty stdout**, stderr carries + `UnknownError: Unexpected server error`. Provider gives no usable hint that the + prefix is wrong, which is exactly why the lane pre-validates. +- `opencode models` → 19 ids, authoritative and live (`xai/grok-4.6`, `xai/grok-4.3`, …). + That is the catalog to validate against. +- Agentic file reads work with no `--auto` and no extra flags, cwd = workspace root. + +## Design decisions + +**Pre-flight model validation against `opencode models`, not a hardcoded list.** +`consult-lanes.ts` opens with an explicit rule: model ids are never validated +against a local catalog, because a catalog goes stale the day a provider ships a +model. That rule is about *hardcoded* catalogs. `opencode models` is the provider +tool answering for itself at call time, so it is the same authority the rule +defers to — just reachable before the spawn instead of after. Validating there +lets `x-ai/grok-4.6` be rejected by name with the right prefix, which the +provider's own `UnknownError` never does. + +**The lane hard-fails; it does not skip.** The gemini/agy lane degrades to a +non-blocking COMMENT skip because it is OAuth-fragile. opencode has no such +property, and #20 is the standing evidence that a lane which produces nothing +gets counted as an approval. So: unknown model, missing CLI, non-zero exit, or +empty output all throw. + +**Default model `xai/grok-4.6`** — the whole point of the lane is a reviewer on a +different account from every existing one, and 4.6 is the strongest Grok listed. + +## What changed + +- `consult-lanes.ts` — `opencode` joins `VALID_LANE_NAMES` and `MODEL_CONFIGURABLE_LANES`; + new pure `assertOpencodeModelAvailable(id, available, key)`. +- `consult/index.ts` — `MODEL_CONFIGS.opencode`, `DEFAULT_OPENCODE_MODEL`, + `resolveOpencodeBin()`, `listOpencodeModels()`, `opencodeReviewHeader()`, + `runOpencodeConsultation()`, plus dispatch and the file-access hint. +- `test-env.ts` — `assertOpencodeLaneAllowedUnderTest()`. Not incidental: without it a + suite that forgets to pin `CODEV_OPENCODE_BIN` bills a real Grok call per spawn. The + agy lane already had exactly this guard for exactly this reason (#1323). +- Docs: `resources/commands/consult.md`, the consult SKILL (4 byte-identical copies), + CLAUDE.md + AGENTS.md, and the `protocol-schema.json` lane enum — which was missing + `hermes` too, so it now lists every lane `VALID_LANE_NAMES` accepts. + +`codev doctor` already probed for OpenCode as an AI CLI dependency. Nothing to add there, +and further evidence for the issue's read that the lane's absence was an oversight, not a +decision. + +`porch/next.ts` needed no change — it iterates whatever lanes are configured rather than +matching against a hardcoded list, so `opencode` in `porch.consultation.models` works. + +## End-to-end verification (the issue asks for this explicitly, not unit tests alone) + +Real `opencode`, real Grok, built CLI: + +- `consult -m opencode --prompt "…"` → exit 0, 36.3s, review written. Fed the resulting + file to porch's own `parseVerdict` → `APPROVE`. The banner opencode writes to stderr + stays out of the review; the model provenance line is at the top and carries no + `VERDICT` token, so it cannot shadow the real verdict (which is found last→first). +- `consult -m opencode --model-id x-ai/grok-4.6` → exit 1, and the message names + `xai/grok-4.6` as the id meant, lists the machine's real catalog, and says Codev does + not fall back. + +## Defaults left alone + +`porch.consultation.models` still defaults to `["gemini", "codex", "claude"]`. Changing the +default rotation is an architectural call the issue does not make, so opencode is available +but opt-in. + +## The lane found a real bug in itself, on its first run + +CMAP on PR #24 came back: claude=APPROVE, opencode=COMMENT, codex=quota-blocked +(resets Aug 27), gemini=skipped (agy exited 1, which emits the non-blocking COMMENT +that `allApprove` counts as an approval — #20, live, in the review of the PR about #20). + +The opencode lane's finding, verified before acting on it: + + parseVerdict('ok') -> REQUEST_CHANGES + parseVerdict(header + 'ok') -> COMMENT // header is 76 chars + +`opencodeReviewHeader` — the provenance banner added to satisfy the issue's "record +which model the lane used" — lifts a short verdict-less review over `parseVerdict`'s +50-character floor. REQUEST_CHANGES becomes COMMENT, and COMMENT counts as approval. +The PR that claims to reduce #20's blast radius was opening a fresh #20 hole. + +**The deeper point, which the architect named and which belongs in the PR body:** the +50-char floor is a *proxy* — length standing in for "a review actually happened". It was +never measuring the right thing. It held by luck, and any lane prefixing provenance, a +banner, a model id or a timestamp defeats it. My header did not break a working guard; it +exposed one that was already wrong. Anyone "fixing" this by bumping 50 to 100 reintroduces +it with the next slightly longer header. + +Fix: extract `findVerdict()` from `parseVerdict()` — it returns the verdict a review +*states*, or `null`. That is the distinction `parseVerdict` structurally cannot express +(a stated COMMENT and no verdict at all return the same value), and it is the same +distinction #20 needs. Whoever picks up #20 should reuse it rather than write a second one. +The lane then hard-fails a protocol-mode review with no verdict — a reviewer that produced +output but no verdict has not reviewed. General mode (`--prompt`) is exempt: a question is +not a review. + +Worth stating plainly rather than hiding: a lane approving the PR that adds it is weak +evidence. A lane finding a real defect in its own implementation, in the exact failure +class the PR exists to reduce, is much stronger — and stronger than an APPROVE from it +would have been. + +Also fixed from claude's review: a stale `CODEV_OPENCODE_BIN` said "install opencode" +instead of naming the override; `listOpencodeModels` now keeps only `provider/model`-shaped +lines, so a future decorated listing degrades to "catalog unreadable" (provider stays the +authority) instead of turning a valid id into a hard failure; tests added for the +large-prompt temp-file branch; `persistent-output.test.ts` and `lane-models.test.ts` lane +loops extended. + +## Flaky tests + +`src/terminal/__tests__/session-manager.test.ts > auto-restart logic > respects maxRestarts +limit` timed out once in the full run (5533 passed / 1 failed), while I had two vitest +suites running concurrently. Passes standalone: 91/91 in 28s. Timing-sensitive under load, +unrelated to consult. Not skipped — a test that passes cleanly on its own should not be +annotated away. diff --git a/packages/codev/src/commands/consult/__tests__/default-models.test.ts b/packages/codev/src/commands/consult/__tests__/default-models.test.ts index b4584ebf9..6e99e45b1 100644 --- a/packages/codev/src/commands/consult/__tests__/default-models.test.ts +++ b/packages/codev/src/commands/consult/__tests__/default-models.test.ts @@ -3,6 +3,7 @@ import { DEFAULT_CLAUDE_MODEL, DEFAULT_CODEX_MODEL, DEFAULT_CODEX_REASONING_EFFORT, + DEFAULT_OPENCODE_MODEL, computeCodexCost, } from '../index.js'; @@ -28,6 +29,13 @@ describe('shipped consult lane defaults', () => { expect(DEFAULT_CODEX_MODEL).toBe('gpt-5.6-sol'); expect(DEFAULT_CODEX_REASONING_EFFORT).toBe('medium'); }); + + it('pins the opencode lane to xai/grok-4.6', () => { + // The `xai/` prefix is load-bearing in the other direction from `-sol`: `x-ai/grok-4.6`, the + // spelling most other tooling uses, is rejected by the provider with a bare + // `UnknownError: Unexpected server error`. Live-probed 2026-08-21. + expect(DEFAULT_OPENCODE_MODEL).toBe('xai/grok-4.6'); + }); }); describe('computeCodexCost', () => { diff --git a/packages/codev/src/commands/consult/__tests__/lane-models.test.ts b/packages/codev/src/commands/consult/__tests__/lane-models.test.ts index 8f7ba802c..6e515ad87 100644 --- a/packages/codev/src/commands/consult/__tests__/lane-models.test.ts +++ b/packages/codev/src/commands/consult/__tests__/lane-models.test.ts @@ -227,13 +227,13 @@ describe('--model-id is refused by lanes with no model selector', () => { } catch (err) { message = (err as Error).message; } - for (const lane of ['claude', 'codex', 'gemini']) expect(message).toContain(lane); + for (const lane of ['claude', 'codex', 'gemini', 'opencode']) expect(message).toContain(lane); }); it('accepts every configurable lane, gemini included', () => { // gemini is configurable by spec; its passthrough lands in phase_3. Asserting it here means // phase_3 cannot narrow this contract without failing a test. - for (const lane of ['claude', 'codex', 'gemini']) { + for (const lane of ['claude', 'codex', 'gemini', 'opencode']) { expect(() => assertLaneAcceptsModelOverride(lane)).not.toThrow(); } }); diff --git a/packages/codev/src/commands/consult/__tests__/opencode-lane.test.ts b/packages/codev/src/commands/consult/__tests__/opencode-lane.test.ts new file mode 100644 index 000000000..e5b6b2a2f --- /dev/null +++ b/packages/codev/src/commands/consult/__tests__/opencode-lane.test.ts @@ -0,0 +1,440 @@ +/** + * The opencode consult lane (#22) — a Grok reviewer on an account no other lane shares. + * + * Two properties carry this lane, and both are here because of what they prevent: + * + * 1. **An unknown model id is named before the spawn.** Live-probed 2026-08-21: + * `opencode run -m x-ai/grok-4.6` exits 1 with EMPTY stdout and + * `UnknownError: Unexpected server error` on stderr — text that identifies neither the model + * nor the mistake. The pre-flight against `opencode models` is the only place a message can + * say "you wrote `x-ai/`, this machine has `xai/`". + * + * 2. **Nothing degrades into a passing review.** #20: porch counts a lane that never produced + * a verdict as an approval. So a missing CLI, a rejected id, a non-zero exit, and empty + * output all throw, and none of them leaves a review file behind for porch to find. + * + * Uses a real fake `opencode` binary (the `agy-lane-model.test.ts` pattern) rather than a module + * mock, so argv, exit codes and stream routing are genuinely exercised — `opencode` writes its + * banner to stderr and only the review to stdout, and that split is load-bearing. + */ + +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import * as fs from 'node:fs'; +import * as os from 'node:os'; +import * as path from 'node:path'; +import { + _runOpencodeConsultation, + resolveOpencodeBin, + listOpencodeModels, + opencodeReviewHeader, + resolveLaneModelChoice, + DEFAULT_OPENCODE_MODEL, +} from '../index.js'; +import { findVerdict, parseVerdict } from '../../porch/verdict.js'; +import { + assertOpencodeModelAvailable, + MODEL_CONFIGURABLE_LANES, + VALID_LANE_NAMES, + validateLaneList, + validateConsultModels, +} from '../../../lib/consult-lanes.js'; + +const ENV_KEYS = [ + 'CODEV_OPENCODE_BIN', + 'FAKE_OPENCODE_ARGV_LOG', + 'FAKE_OPENCODE_MODE', + 'HOME', + 'CODEV_METRICS_DB', +] as const; + +/** + * Fake opencode. Records argv, answers `models` with a catalog shaped like the real one (note + * `xai/`, not `x-ai/`), then behaves per FAKE_OPENCODE_MODE. + * + * The banner on stderr is not decoration: it reproduces the real CLI's stream split, so a test + * that asserts "stdout IS the review" is asserting something real. + */ +const FAKE_OPENCODE_SOURCE = `#!/usr/bin/env node +const fs = require('node:fs'); +const argv = process.argv.slice(2); +if (argv[0] === 'models') { + process.stdout.write('opencode/big-pickle\\nxai/grok-4.3\\nxai/grok-4.6\\n'); + process.exit(0); +} +fs.writeFileSync(process.env.FAKE_OPENCODE_ARGV_LOG, JSON.stringify(argv)); +process.stderr.write('\\n> build · fake\\n'); +const mode = process.env.FAKE_OPENCODE_MODE || 'ok'; +if (mode === 'reject') { + process.stderr.write('Error: {"name":"UnknownError","data":{"message":"Unexpected server error."}}\\n'); + process.exit(1); +} +if (mode === 'empty') { process.exit(0); } +if (mode === 'no-verdict-short') { process.stdout.write('ok'); process.exit(0); } +if (mode === 'no-verdict-long') { + process.stdout.write('A long review that discusses the code at length but never states a verdict line.\\n'); + process.exit(0); +} +process.stdout.write('Looks fine to me.\\n\\nVERDICT: APPROVE\\nSUMMARY: ok\\nCONFIDENCE: HIGH\\n'); +process.exit(0); +`; + +let dir: string; +let savedEnv: Record; +let argvLog: string; + +function writeConfig(config: unknown): void { + fs.mkdirSync(path.join(dir, '.codev'), { recursive: true }); + fs.writeFileSync(path.join(dir, '.codev', 'config.json'), JSON.stringify(config)); +} + +function opencodeArgv(): string[] { + return JSON.parse(fs.readFileSync(argvLog, 'utf-8')); +} + +beforeEach(() => { + savedEnv = {}; + for (const k of ENV_KEYS) savedEnv[k] = process.env[k]; + + dir = fs.mkdtempSync(path.join(os.tmpdir(), 'opencode-lane-')); + const fakeBin = path.join(dir, 'opencode'); + fs.writeFileSync(fakeBin, FAKE_OPENCODE_SOURCE, { mode: 0o755 }); + argvLog = path.join(dir, 'argv.json'); + + process.env.CODEV_OPENCODE_BIN = fakeBin; + process.env.FAKE_OPENCODE_ARGV_LOG = argvLog; + process.env.FAKE_OPENCODE_MODE = 'ok'; + // A real ~/.codev/config.json would otherwise leak an opencode model into every assertion. + process.env.HOME = path.join(dir, 'fake-home'); + + vi.spyOn(process.stdout, 'write').mockImplementation(() => true); + vi.spyOn(console, 'error').mockImplementation(() => {}); +}); + +afterEach(() => { + for (const k of ENV_KEYS) { + if (savedEnv[k] === undefined) delete process.env[k]; + else process.env[k] = savedEnv[k]; + } + vi.restoreAllMocks(); + fs.rmSync(dir, { recursive: true, force: true }); +}); + +// --- registration --------------------------------------------------------------------- + +describe('opencode is a first-class lane', () => { + it('is selectable in porch.consultation lane lists', () => { + expect(VALID_LANE_NAMES).toContain('opencode'); + expect(() => validateLaneList(['gemini', 'claude', 'opencode'], 'porch.consultation.models')) + .not.toThrow(); + }); + + it('accepts a configured model id, which is the point of adding it', () => { + expect(MODEL_CONFIGURABLE_LANES).toContain('opencode'); + expect(() => validateConsultModels({ opencode: 'xai/grok-4.6' })).not.toThrow(); + }); + + it('rejects a syntactically invalid id like any other configurable lane', () => { + expect(() => validateConsultModels({ opencode: '-leading-dash' })).toThrow(/Invalid model id/); + }); +}); + +// --- argv ----------------------------------------------------------------------------- + +describe('argv', () => { + it('runs `opencode run -m `', async () => { + await _runOpencodeConsultation('the query', 'the role', dir); + const argv = opencodeArgv(); + expect(argv[0]).toBe('run'); + expect(argv).toContain('-m'); + expect(argv[argv.indexOf('-m') + 1]).toBe(DEFAULT_OPENCODE_MODEL); + }); + + it('folds the role into the prompt — opencode has no system-prompt flag', async () => { + await _runOpencodeConsultation('the query', 'the role', dir); + const prompt = opencodeArgv().at(-1)!; + expect(prompt).toContain('the role'); + expect(prompt).toContain('the query'); + }); + + it('sends the configured model id rather than the shipped default', async () => { + writeConfig({ consult: { models: { opencode: 'xai/grok-4.3' } } }); + await _runOpencodeConsultation('q', 'role', dir); + const argv = opencodeArgv(); + expect(argv[argv.indexOf('-m') + 1]).toBe('xai/grok-4.3'); + }); + + it('lets --model-id outrank config', async () => { + writeConfig({ consult: { models: { opencode: 'xai/grok-4.3' } } }); + const choice = resolveLaneModelChoice(dir, 'opencode', DEFAULT_OPENCODE_MODEL, 'xai/grok-4.6'); + await _runOpencodeConsultation('q', 'role', dir, undefined, undefined, choice); + const argv = opencodeArgv(); + expect(argv[argv.indexOf('-m') + 1]).toBe('xai/grok-4.6'); + }); +}); + +// --- the review ----------------------------------------------------------------------- + +describe('the review output', () => { + it('is stdout, with the VERDICT line intact', async () => { + const outputPath = path.join(dir, 'review.md'); + await _runOpencodeConsultation('q', 'role', dir, outputPath); + const content = fs.readFileSync(outputPath, 'utf-8'); + expect(content).toContain('VERDICT: APPROVE'); + // The banner opencode writes to stderr must not end up in the review. + expect(content).not.toContain('> build · fake'); + }); + + it('names the model that produced it — Grok 4.6 and Grok 4.3 are not interchangeable evidence', async () => { + writeConfig({ consult: { models: { opencode: 'xai/grok-4.3' } } }); + const outputPath = path.join(dir, 'review.md'); + await _runOpencodeConsultation('q', 'role', dir, outputPath); + const content = fs.readFileSync(outputPath, 'utf-8'); + expect(content).toContain('xai/grok-4.3'); + expect(content).toContain('consult.models.opencode'); + }); + + it('puts the header where it cannot shadow the verdict', () => { + const header = opencodeReviewHeader({ + id: 'xai/grok-4.6', key: null, source: null, fromFlag: false, + }); + // parseVerdict scans last→first; a header carrying its own VERDICT token would win. + expect(header).not.toContain('VERDICT'); + expect(header).toContain('xai/grok-4.6'); + }); +}); + +// --- failing loudly -------------------------------------------------------------------- + +describe('an unknown model id fails before the spawn', () => { + it('names the right prefix when only the prefix is wrong', () => { + const available = ['xai/grok-4.6', 'xai/grok-4.3', 'opencode/big-pickle']; + expect(() => assertOpencodeModelAvailable('x-ai/grok-4.6', available, 'consult.models.opencode')) + .toThrow(/xai\/grok-4\.6/); + expect(() => assertOpencodeModelAvailable('x-ai/grok-4.6', available, 'consult.models.opencode')) + .toThrow(/Did you mean/); + }); + + it('names where the bad id came from', () => { + expect(() => assertOpencodeModelAvailable('x-ai/grok-4.6', ['xai/grok-4.6'], '--model-id')) + .toThrow(/--model-id/); + }); + + it('lists the real catalog when the name is wrong too', () => { + expect(() => assertOpencodeModelAvailable('xai/grok-9', ['xai/grok-4.6'], null)) + .toThrow(/opencode models.* on this machine offers/s); + }); + + it('never falls back to a working id', () => { + expect(() => assertOpencodeModelAvailable('x-ai/grok-4.6', ['xai/grok-4.6'], null)) + .toThrow(/does not fall back/); + }); + + it('passes an id the catalog does offer', () => { + expect(() => assertOpencodeModelAvailable('xai/grok-4.6', ['xai/grok-4.6'], null)).not.toThrow(); + }); + + it('treats an unreadable catalog as unknown, not as "nothing is valid"', () => { + // A broken `opencode models` must not fail every review — the provider stays the authority. + expect(() => assertOpencodeModelAvailable('xai/anything', [], null)).not.toThrow(); + }); + + it('rejects through the lane, without spawning a review', async () => { + writeConfig({ consult: { models: { opencode: 'x-ai/grok-4.6' } } }); + await expect(_runOpencodeConsultation('q', 'role', dir)).rejects.toThrow(/Unknown opencode model/); + expect(fs.existsSync(argvLog)).toBe(false); + }); +}); + +describe('nothing degrades into a passing review (#20)', () => { + it('a non-zero exit rejects', async () => { + process.env.FAKE_OPENCODE_MODE = 'reject'; + await expect(_runOpencodeConsultation('q', 'role', dir)) + .rejects.toThrow(/opencode exited with code 1/); + }); + + it('a non-zero exit carries opencode\'s own diagnostic text', async () => { + process.env.FAKE_OPENCODE_MODE = 'reject'; + await expect(_runOpencodeConsultation('q', 'role', dir)).rejects.toThrow(/UnknownError/); + }); + + it('empty output rejects rather than passing as a silent skip', async () => { + process.env.FAKE_OPENCODE_MODE = 'empty'; + await expect(_runOpencodeConsultation('q', 'role', dir)) + .rejects.toThrow(/produced no review output/); + }); + + it('an unresolvable binary rejects rather than skipping', async () => { + // The agy lane emits a COMMENT skip here; this one refuses. Which of the two resolution + // failures produced it is asserted separately, under "a stale CODEV_OPENCODE_BIN says so" — + // the bare not-on-PATH branch is unreachable from a suite by design, because the isolation + // guard fires before resolution can reach the real install. + process.env.CODEV_OPENCODE_BIN = path.join(dir, 'not-installed'); + await expect(_runOpencodeConsultation('q', 'role', dir)).rejects.toThrow(/does not exist/); + }); + + it('leaves no stale review file for porch to accept', async () => { + // The failure mode this guards: consult writes to a deterministic per-iteration path, so a + // review from an EARLIER run of the same iteration would be read as this one's. + const outputPath = path.join(dir, 'review.md'); + fs.writeFileSync(outputPath, 'stale review from a previous run\nVERDICT: APPROVE\n'); + process.env.FAKE_OPENCODE_MODE = 'reject'; + + await _runOpencodeConsultation('q', 'role', dir, outputPath).catch(() => {}); + + expect(fs.existsSync(outputPath)).toBe(false); + }); + + it('discards a stale review on a rejected model id too', async () => { + const outputPath = path.join(dir, 'review.md'); + fs.writeFileSync(outputPath, 'stale review\nVERDICT: APPROVE\n'); + writeConfig({ consult: { models: { opencode: 'x-ai/grok-4.6' } } }); + + await _runOpencodeConsultation('q', 'role', dir, outputPath).catch(() => {}); + + expect(fs.existsSync(outputPath)).toBe(false); + }); +}); + +// --- binary resolution ------------------------------------------------------------------ + +describe('binary resolution', () => { + it('honours CODEV_OPENCODE_BIN', () => { + expect(resolveOpencodeBin()).toBe(process.env.CODEV_OPENCODE_BIN); + }); + + it('returns null for an override that does not exist, rather than silently using PATH', () => { + process.env.CODEV_OPENCODE_BIN = path.join(dir, 'nope'); + expect(resolveOpencodeBin()).toBeNull(); + }); + + it('refuses to reach the real binary from an unpinned test', () => { + delete process.env.CODEV_OPENCODE_BIN; + // A billed Grok call per spawn is not something a suite should reach by omission. + expect(() => resolveOpencodeBin()).toThrow(/CODEV_OPENCODE_BIN/); + }); + + it('reads the catalog from the resolved binary', () => { + expect(listOpencodeModels(process.env.CODEV_OPENCODE_BIN!)).toEqual([ + 'opencode/big-pickle', 'xai/grok-4.3', 'xai/grok-4.6', + ]); + }); + + it('returns an empty catalog rather than throwing when the listing fails', () => { + expect(listOpencodeModels(path.join(dir, 'nope'))).toEqual([]); + }); +}); + +// --- the verdict requirement ----------------------------------------------------------- + +/** + * A protocol-mode review that states no verdict is refused. + * + * This is the defect the lane found in its own first review, and it is worth stating precisely + * because the obvious "fix" is wrong. `parseVerdict` treats output under 50 characters as + * REQUEST_CHANGES — a floor meant to catch a lane that produced nothing useful. But length was + * always a *proxy* for "a review happened", and this lane's 76-character provenance banner clears + * the floor on its own: a two-word non-answer becomes COMMENT, and `allApprove` counts COMMENT as + * an approval. The header did not break a working guard; it exposed one that was already measuring + * the wrong thing, and raising 50 to 100 would only postpone the next banner. + * + * So the lane asks the real question — did the reviewer state a verdict? — via `findVerdict`. + */ +describe('a protocol-mode review must state a verdict', () => { + it('rejects a short verdict-less review the header would otherwise smuggle past the floor', async () => { + process.env.FAKE_OPENCODE_MODE = 'no-verdict-short'; + await expect(_runOpencodeConsultation('q', 'role', dir, undefined, undefined, undefined, true)) + .rejects.toThrow(/no VERDICT line/); + }); + + it('rejects a LONG verdict-less review too — this is not about length', async () => { + process.env.FAKE_OPENCODE_MODE = 'no-verdict-long'; + await expect(_runOpencodeConsultation('q', 'role', dir, undefined, undefined, undefined, true)) + .rejects.toThrow(/no VERDICT line/); + }); + + it('leaves no review file behind when it rejects', async () => { + process.env.FAKE_OPENCODE_MODE = 'no-verdict-long'; + const outputPath = path.join(dir, 'review.md'); + await _runOpencodeConsultation('q', 'role', dir, outputPath, undefined, undefined, true) + .catch(() => {}); + expect(fs.existsSync(outputPath)).toBe(false); + }); + + it('does not require a verdict in general mode, where none was asked for', async () => { + // `consult -m opencode --prompt "..."` is a question, not a review. + process.env.FAKE_OPENCODE_MODE = 'no-verdict-long'; + await expect(_runOpencodeConsultation('q', 'role', dir, undefined, undefined, undefined, false)) + .resolves.toBeUndefined(); + }); + + it('accepts a review that does state one', async () => { + const outputPath = path.join(dir, 'review.md'); + await _runOpencodeConsultation('q', 'role', dir, outputPath, undefined, undefined, true); + expect(fs.readFileSync(outputPath, 'utf-8')).toContain('VERDICT: APPROVE'); + }); + + it('checks the verdict against the review, not against the header', () => { + // Guards the regression directly: were the check applied to header+review, a future header + // carrying the word VERDICT would satisfy it without the reviewer saying anything. + const header = opencodeReviewHeader({ + id: 'xai/grok-4.6', key: null, source: null, fromFlag: false, + }); + expect(findVerdict(header + 'ok')).toBeNull(); + expect(parseVerdict(header + 'ok')).toBe('COMMENT'); // what porch would have believed + }); +}); + +// --- large prompts ---------------------------------------------------------------------- + +describe('a prompt too large for argv', () => { + it('goes to a temp file that opencode is pointed at', async () => { + const huge = 'x'.repeat(150_000); + await _runOpencodeConsultation(huge, 'role', dir); + const promptArg = opencodeArgv().at(-1)!; + // ARG_MAX: the prompt must NOT be inline. + expect(promptArg.length).toBeLessThan(1000); + expect(promptArg).toMatch(/Read the full consultation prompt from this file: .*\.md/); + + const tempFile = promptArg.match(/from this file: (\S+\.md)/)![1]; + // Cleaned up after the run — the file existed only for opencode to read. + expect(fs.existsSync(tempFile)).toBe(false); + }); + + it('keeps a normal prompt inline', async () => { + await _runOpencodeConsultation('a short query', 'role', dir); + expect(opencodeArgv().at(-1)!).toContain('a short query'); + }); +}); + +// --- catalog parsing -------------------------------------------------------------------- + +describe('catalog parsing tolerates a decorated listing', () => { + it('keeps only provider/model lines', () => { + const decorated = path.join(dir, 'opencode-decorated'); + fs.writeFileSync(decorated, `#!/usr/bin/env node +process.stdout.write('Available models:\\n\\n xai/grok-4.6 (default)\\nxai/grok-4.3\\n'); +process.exit(0); +`, { mode: 0o755 }); + // The header and the annotated line are dropped rather than parsed as ids. + expect(listOpencodeModels(decorated)).toEqual(['xai/grok-4.3']); + }); + + it('a listing with no parseable line reads as "unknown", never as "nothing is valid"', () => { + // Otherwise a cosmetic change in someone else's CLI turns every valid id into a hard failure. + const garbage = path.join(dir, 'opencode-garbage'); + fs.writeFileSync(garbage, `#!/usr/bin/env node +process.stdout.write('Error: could not reach the model registry\\n'); +process.exit(0); +`, { mode: 0o755 }); + expect(listOpencodeModels(garbage)).toEqual([]); + expect(() => assertOpencodeModelAvailable('xai/grok-4.6', [], null)).not.toThrow(); + }); +}); + +describe('a stale CODEV_OPENCODE_BIN says so', () => { + it('does not tell you to install opencode when the override is the problem', async () => { + process.env.CODEV_OPENCODE_BIN = path.join(dir, 'moved-away'); + await expect(_runOpencodeConsultation('q', 'role', dir)) + .rejects.toThrow(/CODEV_OPENCODE_BIN points at .*moved-away, which does not exist/); + }); +}); diff --git a/packages/codev/src/commands/consult/__tests__/persistent-output.test.ts b/packages/codev/src/commands/consult/__tests__/persistent-output.test.ts index e1dd8c073..8007a8f6e 100644 --- a/packages/codev/src/commands/consult/__tests__/persistent-output.test.ts +++ b/packages/codev/src/commands/consult/__tests__/persistent-output.test.ts @@ -80,11 +80,11 @@ describe('computePersistentOutputPath', () => { projectDir: '/workspace/codev/projects/0073-my-feature', }; - for (const model of ['gemini', 'codex', 'claude', 'hermes']) { + for (const model of ['gemini', 'codex', 'claude', 'hermes', 'opencode']) { const result = computePersistentOutputPath(state, model); const fileName = result.split('/').pop()!; // Must match: --iter-.txt - expect(fileName).toMatch(/^0073-phase_1-iter1-(gemini|codex|claude|hermes)\.txt$/); + expect(fileName).toMatch(/^0073-phase_1-iter1-(gemini|codex|claude|hermes|opencode)\.txt$/); } }); }); diff --git a/packages/codev/src/commands/consult/cli-options.ts b/packages/codev/src/commands/consult/cli-options.ts index 7a169a209..c39733fe9 100644 --- a/packages/codev/src/commands/consult/cli-options.ts +++ b/packages/codev/src/commands/consult/cli-options.ts @@ -28,7 +28,7 @@ export const STATS_ONLY_FLAGS = ['days', 'project', 'last', 'json'] as const; /** Register every `consult` flag on a command. */ export function registerConsultOptions(cmd: Command): Command { return cmd - .option('-m, --model ', 'Model to use (gemini, codex, claude, hermes, or aliases: pro, gpt, opus)') + .option('-m, --model ', 'Model to use (gemini, codex, claude, hermes, opencode, or aliases: pro, gpt, opus)') .option('--prompt ', 'Inline prompt (general mode)') .option('--prompt-file ', 'Prompt file path (general mode)') .option('--protocol ', 'Protocol name: spir, aspir, air, bugfix, pir, maintain') @@ -36,7 +36,7 @@ export function registerConsultOptions(cmd: Command): Command { .option('--issue ', 'Issue number (required from architect context)') .option('--branch ', 'Read spec/plan artifacts from this git ref instead of the local workspace (e.g. `origin/builder/777-foo` or `builder/777-foo`). Defaults to the PR\'s head branch when --issue resolves to a PR. Note: this only changes the artifact source — for --type impl, the diff scope is always the PR\'s head→base, not the --branch ref.') .option('--base ', 'For --type integration: anchor the diff on this base branch (e.g. `ci`), computed locally as `git diff origin/...origin/` (three-dot). Use in repos with a long-lived integration branch ahead of the default branch so the review sees only the PR\'s actual change, not the whole integration-over-trunk delta. Defaults to config `consult.integrationBranch`; unset → the PR\'s host base (`gh pr diff`).') - .option('--model-id ', 'Override the provider model id for this invocation, outranking config `consult.models.` (e.g. `--model-id gpt-5.6-sol`). Supported for the claude, codex, and gemini lanes; using it with a lane that has no model selector (hermes) is an error rather than a silent no-op. Codev validates syntax only — whether the id exists is the provider\'s call, and a rejection fails loudly with no fallback.') + .option('--model-id ', 'Override the provider model id for this invocation, outranking config `consult.models.` (e.g. `--model-id gpt-5.6-sol`). Supported for the claude, codex, gemini, and opencode lanes; using it with a lane that has no model selector (hermes) is an error rather than a silent no-op. Codev validates syntax only — whether the id exists is the provider\'s call, and a rejection fails loudly with no fallback. The one exception is opencode, whose id is checked against `opencode models` before the run, because the provider rejects an unknown one with an untraceable server error.') .option('--output ', 'Write consultation output to file (used by porch)') .option('--plan-phase ', 'Scope review to a specific plan phase (used by porch)') .option('--context ', 'Context file with previous iteration feedback (used by porch)') diff --git a/packages/codev/src/commands/consult/index.ts b/packages/codev/src/commands/consult/index.ts index 2952771c5..1dc96434a 100644 --- a/packages/codev/src/commands/consult/index.ts +++ b/packages/codev/src/commands/consult/index.ts @@ -22,15 +22,17 @@ import { resolveReasoningEffort, validateModelId, assertLaneAcceptsModelOverride, + assertOpencodeModelAvailable, type ConfigurableLane, } from '../../lib/consult-lanes.js'; import type { ModelReasoningEffort } from '@openai/codex-sdk'; import { getResolver, GitRefResolver, type ArtifactResolver } from '../porch/artifacts.js'; +import { findVerdict } from '../porch/verdict.js'; import { MetricsDB } from './metrics.js'; import { extractUsage, extractReviewText, type SDKResultLike, type UsageData } from './usage-extractor.js'; import { executeForgeCommandSync } from '../../lib/forge.js'; import { preflightAgyAuth, recordAgyAuthState, type AgyAuthState } from './agy-auth-cache.js'; -import { assertAgyLaneAllowedUnderTest } from '../../lib/test-env.js'; +import { assertAgyLaneAllowedUnderTest, assertOpencodeLaneAllowedUnderTest } from '../../lib/test-env.js'; // Content reference — resolved artifact content with a display label interface ContentRef { @@ -51,6 +53,10 @@ const MODEL_CONFIGS: Record = { // cli/args are NOT used for dispatch (agy's binary path is resolved at runtime). gemini: { cli: 'agy', args: [], envVar: null }, hermes: { cli: 'hermes', args: ['chat', '-q'], envVar: null }, + // opencode dispatches via runOpencodeConsultation (it needs `-m` and a pre-flight + // catalog check), so cli/args here are the shape the runner builds on, not a + // literal argv — the prompt and `-m ` are appended by the runner. + opencode: { cli: 'opencode', args: ['run'], envVar: null }, }; // Models that use an Agent SDK instead of CLI subprocess @@ -418,6 +424,20 @@ export const DEFAULT_CODEX_REASONING_EFFORT = 'medium' as const; /** Shipped default model id for the claude consult lane (#1288). */ export const DEFAULT_CLAUDE_MODEL = 'claude-opus-5'; +/** + * Shipped default model id for the opencode consult lane (#22). + * + * The `xai/` prefix is LOAD-BEARING. `x-ai/grok-4.6` — the spelling most other tooling uses — was + * live-probed on 2026-08-21 and rejected by the provider with a bare `UnknownError: Unexpected + * server error` and empty stdout. `assertOpencodeModelAvailable` exists so that mistake is named + * before the spawn instead of arriving as that. + * + * The lane exists to supply a reviewer on an account shared with no other lane, so it defaults to + * the strongest Grok `opencode models` lists rather than to whatever opencode would pick — an + * unpinned default could silently land on a model from a provider a sibling lane already uses. + */ +export const DEFAULT_OPENCODE_MODEL = 'xai/grok-4.6'; + interface CodexModelPricing { inputPer1M: number; cachedInputPer1M: number; @@ -1249,6 +1269,290 @@ async function runAgyConsultation( }); } +// --- opencode lane (#22) ------------------------------------------------------ + +/** Codev-owned hard cap on a single `opencode run`. Matches the agy lane's budget. */ +const OPENCODE_TIMEOUT_MS = 6 * 60 * 1000; + +/** How long `opencode models` gets to print its catalog before the pre-flight gives up. */ +const OPENCODE_MODELS_TIMEOUT_MS = 30_000; + +/** Bounded tail of the lane's output, retained so a failure can quote why it failed. */ +const OPENCODE_FAILURE_TAIL_MAX_CHARS = 2000; + +/** + * Resolve the `opencode` binary, or `null` when it isn't installed. + * + * The single chokepoint for the test-isolation guard, and it has to be resolution rather than the + * spawn: the pre-flight below *executes* the binary (`opencode models`) before any review runs, so + * guarding only `runOpencodeConsultation`'s spawn would still let a suite reach the real CLI. + */ +export function resolveOpencodeBin(): string | null { + // Explicit override (tests, or a non-PATH install): honoured as given, never quietly replaced + // with a different binary the caller did not ask for. + const override = process.env.CODEV_OPENCODE_BIN; + if (override) return fs.existsSync(override) ? override : null; + + assertOpencodeLaneAllowedUnderTest(); + + return commandExists('opencode') ? 'opencode' : null; +} + +/** + * The model ids `opencode` offers on this machine, or `[]` if the catalog could not be read. + * + * `[]` deliberately means "unknown", not "none": the pre-flight below then skips the existence + * check and lets the provider be the authority, which is the pre-1286 behaviour. Failing the lane + * because a *catalog listing* broke would turn a diagnostic into an outage. + */ +export function listOpencodeModels(bin = 'opencode'): string[] { + let out: string; + try { + out = execFileSync(bin, ['models'], { + encoding: 'utf-8', + timeout: OPENCODE_MODELS_TIMEOUT_MS, + stdio: ['ignore', 'pipe', 'pipe'], + }); + } catch { + return []; + } + // Keep only `provider/model`-shaped lines. Today `opencode models` prints nothing else (probed + // 2026-08-21), but if it ever gains a header or an annotation, parsing those as ids would turn a + // *valid* model into "Unknown opencode model" — a hard failure caused by a cosmetic change in + // someone else's CLI. Filtering means a decorated listing degrades to "catalog unreadable", which + // hands authority back to the provider instead of blocking the lane. (Raised by claude at review.) + return out + .split('\n') + .map(l => l.trim()) + .filter(l => OPENCODE_MODEL_LINE_RE.test(l)); +} + +/** A bare `provider/model` line, the entire shape `opencode models` emits. */ +const OPENCODE_MODEL_LINE_RE = /^[A-Za-z0-9][A-Za-z0-9._-]*\/[A-Za-z0-9][A-Za-z0-9._-]*$/; + +/** + * Provenance banner prepended to an opencode review. + * + * The issue's requirement, verbatim: "Record which model the lane used in the review output. `Grok + * 4.6` and `Grok 4.3` are not interchangeable evidence." Stderr logging is not enough — the review + * file is what outlives the run and what a later reader actually opens. + * + * Safe to prepend: `parseVerdict` scans lines LAST→FIRST, so a header cannot shadow the verdict at + * the end, and this line carries no `VERDICT:` token of its own. + */ +export function opencodeReviewHeader(choice: LaneModelChoice): string { + const from = choice.key ? ` (from ${choice.key})` : ' (shipped default)'; + return `_Reviewed by the opencode lane — model: \`${choice.id}\`${from}._\n\n`; +} + +/** + * Run the `opencode` consult lane (`opencode run -m `). + * + * ## Why this lane hard-fails where the agy lane skips + * + * The gemini/agy lane degrades to a non-blocking COMMENT skip because it is OAuth-fragile: an + * unauthenticated `agy` is a routine state on a developer's machine, and wedging every phase on it + * would be worse than losing a lane. opencode has no equivalent failure mode — it authenticates + * once and stays that way. + * + * So the trade-off runs the other way here, and #20 is why it matters: porch counts a lane that + * produced nothing as an approval. A lane that quietly emits a skip is a lane that quietly lowers + * the bar. Missing CLI, unknown model, non-zero exit, and empty output all throw. + */ +export async function runOpencodeConsultation( + queryText: string, + role: string, + workspaceRoot: string, + outputPath?: string, + metricsCtx?: MetricsContext, + modelChoice?: LaneModelChoice, + requireVerdict = false, +): Promise { + const startTime = Date.now(); + const choice = modelChoice + ?? resolveLaneModelChoice(workspaceRoot, 'opencode', DEFAULT_OPENCODE_MODEL); + + const bin = resolveOpencodeBin(); + if (!bin) { + // A missing CLI is a hard failure here, unlike the agy lane's skip. See the header: a lane that + // silently produces nothing is counted as an approval (#20), and "not installed" is a + // configuration mistake with an obvious fix, not a transient environment state. + // + // A stale CODEV_OPENCODE_BIN reaches this same branch, so it gets its own message: "install + // opencode" is the wrong instruction for someone whose override points at a path that moved. + discardStaleOutput(outputPath); + const override = process.env.CODEV_OPENCODE_BIN; + throw new Error( + override + ? `CODEV_OPENCODE_BIN points at ${override}, which does not exist. ` + + `Correct it or unset it to fall back to opencode on PATH.` + : 'opencode not found. Install it (https://opencode.ai), or drop "opencode" from ' + + 'porch.consultation in .codev/config.json.' + ); + } + + // Pre-flight the id against opencode's own catalog. The provider's rejection is + // `UnknownError: Unexpected server error` with empty stdout — useless for finding a typo'd + // prefix — so the check has to happen while we still know what was asked for. + try { + assertOpencodeModelAvailable(choice.id, listOpencodeModels(bin), choice.key); + } catch (err) { + discardStaleOutput(outputPath); + throw err; + } + + // opencode has no system-prompt flag, so the role folds into the prompt (hermes/agy precedent). + const prompt = `${role}\n\n---\n\n${queryText}`; + let tempFile: string | null = null; + let promptArg = prompt; + // Large inline argv can exceed ARG_MAX (E2BIG) — write it out and point opencode at the file. + // The temp file lands in the consult sandbox dir, the same one the agy lane uses. + if (prompt.length > CLI_PROMPT_INLINE_MAX_CHARS) { + tempFile = path.join(consultSandboxDir(), `codev-consult-prompt-${Date.now()}.md`); + fs.writeFileSync(tempFile, prompt); + promptArg = [ + `Read the full consultation prompt from this file: ${tempFile}`, + 'You have file access. Read files directly from disk to review code.', + ].join('\n\n'); + } + + const args = [...MODEL_CONFIGS.opencode.args, '-m', choice.id, promptArg]; + + const cleanup = () => { + if (tempFile && fs.existsSync(tempFile)) { + try { fs.unlinkSync(tempFile); } catch { /* best-effort */ } + } + }; + + return new Promise((resolve, reject) => { + const proc = spawn(bin, args, { + cwd: workspaceRoot, + // stderr is piped, not inherited: opencode writes its banner and its tool-call trace there, + // and that trace is the only text explaining a rejection, so it is retained rather than + // spilled into the parent's stream. + stdio: ['ignore', 'pipe', 'pipe'], + }); + + const outChunks: Buffer[] = []; + let outputTail = ''; + let settled = false; + + const fail = (message: string, exitCode: number) => { + if (settled) return; + settled = true; + clearTimeout(timer); + try { proc.kill('SIGTERM'); } catch { /* already gone */ } + cleanup(); + recordOpencodeMetrics(metricsCtx, startTime, exitCode, message, choice.id); + console.error(`\n[opencode FAILED: ${message}]`); + // "No review file" must mean none EXISTS, not merely that this run wrote none — porch keys + // off the file's presence and would otherwise advance on an earlier iteration's review. + discardStaleOutput(outputPath); + const err = new Error( + `${message}` + + (outputTail.trim() + ? `\n\nopencode output (last ${OPENCODE_FAILURE_TAIL_MAX_CHARS} chars):\n${outputTail.trim()}` + : '') + ); + reject(annotateModelError(err, 'opencode', choice)); + }; + + const timer = setTimeout( + () => fail(`opencode timed out after ${OPENCODE_TIMEOUT_MS / 1000}s`, 1), + OPENCODE_TIMEOUT_MS, + ); + + const watch = (buf: Buffer, isStdout: boolean) => { + if (isStdout) outChunks.push(buf); + outputTail = (outputTail + buf.toString('utf-8')).slice(-OPENCODE_FAILURE_TAIL_MAX_CHARS); + }; + proc.stdout?.on('data', (b: Buffer) => watch(b, true)); + proc.stderr?.on('data', (b: Buffer) => watch(b, false)); + + proc.on('error', (err) => fail(`opencode failed to start: ${err.message}`, 1)); + + proc.on('close', (code) => { + if (settled) return; + const raw = Buffer.concat(outChunks).toString('utf-8').trim(); + + // fail() owns settling on both error paths — it clears the timer and cleans up itself, so + // this handler must NOT pre-settle or those paths would be swallowed by its own guard. + if (code !== 0) { + // `code === null` means a signal killed it (OOM, external kill). Still a hard failure — this + // lane has no skip path — but saying "exited with code null" sends the reader hunting for a + // provider error that was never printed. + fail( + code === null + ? 'opencode was killed by a signal before producing a review' + : `opencode exited with code ${code}`, + code ?? 1, + ); + return; + } + // A zero exit with nothing on stdout is the #20 shape exactly: no review, but nothing that + // looks like a failure either. Refuse to let it pass as one. + if (raw.length === 0) { + fail('opencode produced no review output', 0); + return; + } + // Same shape one step further in: a protocol-mode run that answered, but never stated a + // verdict. `parseVerdict` cannot distinguish that from a stated COMMENT, and `allApprove` + // counts COMMENT as an approval — so silence would become consent. + // + // This guard also covers what the header would otherwise break. `parseVerdict` treats output + // under 50 characters as REQUEST_CHANGES, a floor that exists to catch exactly this; the + // 76-character provenance banner lifts a two-word non-answer over it and converts a would-be + // REQUEST_CHANGES into an approval. Found by the opencode lane reviewing its own PR, which is + // a better argument for the lane than anything in the PR body. + if (requireVerdict && findVerdict(raw) === null) { + fail( + 'opencode produced a review with no VERDICT line. A review that states no verdict is ' + + 'not a verdict — porch would read it as a non-blocking COMMENT and count it as an ' + + 'approval.', + 0, + ); + return; + } + + settled = true; + clearTimeout(timer); + cleanup(); + + // `opencode run` prints the assistant's plain text to stdout (its banner and tool trace go to + // stderr), so stdout IS the review. Live-probed 2026-08-21, including the `VERDICT:` line + // surviving verbatim — which is why verdict parsing needs no opencode-specific case. + const content = opencodeReviewHeader(choice) + raw; + process.stdout.write(content); + writeConsultOutput(outputPath, content); + recordOpencodeMetrics(metricsCtx, startTime, 0, null, choice.id); + console.error(`\n[opencode completed in ${((Date.now() - startTime) / 1000).toFixed(1)}s]`); + resolve(); + }); + }); +} + +/** Metrics row for the opencode lane. `opencode run` reports no token usage, so those stay null. */ +function recordOpencodeMetrics( + metricsCtx: MetricsContext | undefined, + startTime: number, + exitCode: number, + errorMessage: string | null, + modelId: string, +): void { + if (!metricsCtx) return; + recordMetrics(metricsCtx, { + // Always a real id — unlike hermes, this lane never runs without one. + modelId, + durationSeconds: (Date.now() - startTime) / 1000, + inputTokens: null, + cachedInputTokens: null, + outputTokens: null, + costUsd: null, + exitCode, + errorMessage, + }); +} + /** * Record the model a lane actually ran, so a transcript answers "what did this use?". * @@ -1318,6 +1622,21 @@ async function runConsultation( return; } + // opencode lane → `opencode run` (#22). Dispatched here rather than through the generic + // MODEL_CONFIGS path below because it needs `-m` and a pre-flight catalog check. + if (model === 'opencode') { + const startTime = Date.now(); + const choice = resolveLaneModelChoice(workspaceRoot, 'opencode', DEFAULT_OPENCODE_MODEL, modelIdOverride); + logResolvedModel(model, choice.id, choice.key); + // `generalMode` is an ad-hoc `--prompt`, where no verdict is expected or asked for. Protocol + // mode is a review, and a review owes a verdict. + await runOpencodeConsultation( + query, role, workspaceRoot, outputPath, metricsCtx, choice, !generalMode, + ); + logQuery(workspaceRoot, model, query, (Date.now() - startTime) / 1000); + return; + } + const config = MODEL_CONFIGS[model]; if (!config) { @@ -2372,8 +2691,8 @@ export async function consult(options: ConsultOptions): Promise { } } - // Add file access instruction for Gemini - if (model === 'gemini' || model === 'hermes') { + // Add file access instruction for the agentic CLI lanes + if (model === 'gemini' || model === 'hermes' || model === 'opencode') { query += '\n\nYou have file access. Read files directly from disk to review code.'; } @@ -2426,5 +2745,6 @@ export { MODEL_CONFIGS as _MODEL_CONFIGS, MODEL_ALIASES as _MODEL_ALIASES, runAgyConsultation as _runAgyConsultation, + runOpencodeConsultation as _runOpencodeConsultation, agySkipContent as _agySkipContent, }; diff --git a/packages/codev/src/commands/porch/__tests__/parse-verdict.test.ts b/packages/codev/src/commands/porch/__tests__/parse-verdict.test.ts index a4f7eafb4..9badf17de 100644 --- a/packages/codev/src/commands/porch/__tests__/parse-verdict.test.ts +++ b/packages/codev/src/commands/porch/__tests__/parse-verdict.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect } from 'vitest'; -import { parseVerdict } from '../verdict'; +import { parseVerdict, findVerdict } from '../verdict'; describe('parseVerdict', () => { it('returns REQUEST_CHANGES for empty output', () => { @@ -121,3 +121,58 @@ But it does not contain any VERDICT: line because the reviewer went off-task or expect(parseVerdict(output)).toBe('COMMENT'); }); }); + +/** + * `findVerdict` — "did the reviewer state a verdict?", which `parseVerdict` cannot answer (#22). + * + * The distinction matters because of what sits downstream: `allApprove` counts COMMENT as an + * approval, and `parseVerdict` returns COMMENT both when a reviewer wrote `VERDICT: COMMENT` and + * when a reviewer wrote no verdict at all. Silence and consent are indistinguishable there. + * + * The 50-character floor was the only thing standing between those two cases, and it was a proxy — + * length standing in for "a review happened". Any lane that prefixes provenance, a banner, a model + * id or a timestamp clears the floor without reviewing anything. This function measures the thing + * itself instead. + */ +describe('findVerdict', () => { + it('returns null when the reviewer stated no verdict', () => { + expect(findVerdict('Some long review text with no verdict line anywhere in it at all.')).toBeNull(); + }); + + it('distinguishes a stated COMMENT from a missing verdict', () => { + const stated = 'Review text long enough to clear the floor.\n\nVERDICT: COMMENT'; + const missing = 'Review text long enough to clear the floor, but stating nothing.'; + // parseVerdict collapses these two into the same answer; that collapse is the bug. + expect(parseVerdict(stated)).toBe(parseVerdict(missing)); + expect(findVerdict(stated)).toBe('COMMENT'); + expect(findVerdict(missing)).toBeNull(); + }); + + it('ignores a template placeholder, as parseVerdict does', () => { + expect(findVerdict('VERDICT: [APPROVE | REQUEST_CHANGES | COMMENT]')).toBeNull(); + }); + + it('reads the LAST verdict, as parseVerdict does', () => { + expect(findVerdict('VERDICT: REQUEST_CHANGES\n\nlater...\n\nVERDICT: APPROVE')).toBe('APPROVE'); + }); + + it('strips markdown emphasis, as parseVerdict does', () => { + expect(findVerdict('**VERDICT: APPROVE**')).toBe('APPROVE'); + }); + + it('leaves parseVerdict behaviour unchanged for short output', () => { + // The refactor must not move the floor: parseVerdict still shortcuts before consulting this. + expect(findVerdict('ok')).toBeNull(); + expect(parseVerdict('ok')).toBe('REQUEST_CHANGES'); + }); + + it('shows why a provenance header is dangerous without a verdict check', () => { + // The concrete defect the opencode lane found in its own implementation: a 76-character banner + // lifts a two-word non-answer over the floor, and REQUEST_CHANGES silently becomes an approval. + const header = '_Reviewed by the opencode lane — model: `xai/grok-4.6` (shipped default)._\n\n'; + expect(parseVerdict('ok')).toBe('REQUEST_CHANGES'); + expect(parseVerdict(header + 'ok')).toBe('COMMENT'); + // findVerdict is indifferent to length, so it catches what the floor cannot. + expect(findVerdict(header + 'ok')).toBeNull(); + }); +}); diff --git a/packages/codev/src/commands/porch/verdict.ts b/packages/codev/src/commands/porch/verdict.ts index 8465638e4..54828371a 100644 --- a/packages/codev/src/commands/porch/verdict.ts +++ b/packages/codev/src/commands/porch/verdict.ts @@ -7,9 +7,9 @@ import type { Verdict, ReviewResult } from './types.js'; /** - * Parse verdict from consultation output. + * The verdict a review explicitly states, or `null` when it states none. * - * Looks for the verdict line in format: + * Recognises the verdict line in the format: * VERDICT: APPROVE * VERDICT: REQUEST_CHANGES * VERDICT: COMMENT @@ -18,15 +18,12 @@ import type { Verdict, ReviewResult } from './types.js'; * **VERDICT: APPROVE** * *VERDICT: APPROVE* * - * Safety: If no explicit verdict found (empty output, crash, malformed), - * defaults to REQUEST_CHANGES to prevent proceeding with unverified code. + * Split out of `parseVerdict` so a caller can tell "the reviewer said COMMENT" apart from "the + * reviewer said nothing and COMMENT is what we defaulted to". `parseVerdict` cannot express that + * difference — both come back as COMMENT, and `allApprove` counts COMMENT as an approval — so a + * lane that wants to refuse a verdict-less review has to ask this instead (#22). */ -export function parseVerdict(output: string): Verdict { - // Empty or very short output = something went wrong - if (!output || output.trim().length < 50) { - return 'REQUEST_CHANGES'; - } - +export function findVerdict(output: string): Verdict | null { // Scan lines LAST→FIRST so the actual verdict (at the end) takes priority // over template text echoed by codex CLI at the start of output. // Skip template lines containing "[" (e.g., "VERDICT: [APPROVE | REQUEST_CHANGES | COMMENT]") @@ -42,9 +39,23 @@ export function parseVerdict(output: string): Verdict { if (value.startsWith('COMMENT')) return 'COMMENT'; } } + return null; +} + +/** + * Parse verdict from consultation output. + * + * Safety: If no explicit verdict found (empty output, crash, malformed), + * defaults to REQUEST_CHANGES to prevent proceeding with unverified code. + */ +export function parseVerdict(output: string): Verdict { + // Empty or very short output = something went wrong + if (!output || output.trim().length < 50) { + return 'REQUEST_CHANGES'; + } // No valid VERDICT: line found but the consult ran — treat as COMMENT (non-blocking skip) - return 'COMMENT'; + return findVerdict(output) ?? 'COMMENT'; } /** diff --git a/packages/codev/src/lib/config.ts b/packages/codev/src/lib/config.ts index 714bc165c..67d4a68fe 100644 --- a/packages/codev/src/lib/config.ts +++ b/packages/codev/src/lib/config.ts @@ -81,7 +81,7 @@ export interface CodevConfig { * local catalog of ids goes stale the moment a provider ships a new model. The provider is the * authority: a rejected id fails the consultation loudly, with no fallback to the default. */ - models?: Partial>; + models?: Partial>; /** Codex-only; a closed enum bound to the SDK's ModelReasoningEffort union. */ reasoningEffort?: { codex?: ModelReasoningEffort }; /** Codex-only per-1M token rates; all three required together. */ diff --git a/packages/codev/src/lib/consult-lanes.ts b/packages/codev/src/lib/consult-lanes.ts index 812135a4e..a76591be3 100644 --- a/packages/codev/src/lib/consult-lanes.ts +++ b/packages/codev/src/lib/consult-lanes.ts @@ -26,7 +26,7 @@ import { canonicalProtocolName, listProtocolNames, listReviewTypes } from './ske // --------------------------------------------------------------------------- /** Lanes whose model id can be configured. `hermes` is absent: `hermes chat -q` has no model selector. */ -export const MODEL_CONFIGURABLE_LANES = ['claude', 'codex', 'gemini'] as const; +export const MODEL_CONFIGURABLE_LANES = ['claude', 'codex', 'gemini', 'opencode'] as const; export type ConfigurableLane = (typeof MODEL_CONFIGURABLE_LANES)[number]; /** Lanes exposing a reasoning-effort knob. Deliberately narrower than MODEL_CONFIGURABLE_LANES. */ @@ -57,7 +57,7 @@ const _REASONING_EFFORTS_ARE_EXHAUSTIVE: UncoveredEffort extends never ? true : void _REASONING_EFFORTS_ARE_EXHAUSTIVE; /** Lane names accepted in `porch.consultation.*` lists (includes hermes — it IS a review backend). */ -export const VALID_LANE_NAMES = ['gemini', 'codex', 'claude', 'hermes']; +export const VALID_LANE_NAMES = ['gemini', 'codex', 'claude', 'hermes', 'opencode']; /** Whole-value special modes, accepted wherever a lane list is accepted. */ export const SPECIAL_MODES = ['none', 'parent'] as const; @@ -163,6 +163,46 @@ export function assertLaneAcceptsModelOverride(lane: string, flag = '--model-id' ); } +/** + * Reject an `opencode` model id the local `opencode` install does not offer. + * + * This is NOT the hardcoded catalog the header of this file forbids. `available` is whatever + * `opencode models` printed on this machine at call time — the provider tool answering for itself, + * which is the same authority the no-catalog rule defers to. The only difference is that it is + * reachable *before* the spawn. + * + * Reaching it before the spawn is the whole point. A wrong provider prefix (`x-ai/` for `xai/`) + * comes back from the provider as `UnknownError: Unexpected server error` with empty stdout — text + * naming neither the model nor the mistake (live-probed 2026-08-21). So the useful message can only + * be built here, where the intended id and the real catalog are both in hand. + * + * An empty `available` means the catalog could not be read; the caller decides what that means + * rather than this function reading "no list" as "nothing is valid". + */ +export function assertOpencodeModelAvailable( + id: string, + available: readonly string[], + key: string | null, +): void { + if (available.length === 0 || available.includes(id)) return; + + // Same model name, different provider prefix — by far the likeliest way to get here, and exactly + // what the provider's own error is useless for. + const bare = (m: string) => m.slice(m.indexOf('/') + 1); + const samePart = available.filter(m => bare(m) === bare(id)); + + const where = key ? ` (from ${key})` : ''; + const hint = samePart.length > 0 + ? `\nDid you mean ${quoted(samePart)}? The model name is right; the provider prefix is not.` + : ''; + + fail( + `Unknown opencode model ${JSON.stringify(id)}${where}.\n` + + `\`opencode models\` on this machine offers: ${quoted([...available].sort())}.${hint}\n` + + `Codev does not fall back to a default model — correct the id at the source above.` + ); +} + export function validateConsultModels(models: unknown): void { if (models === undefined) return; if (typeof models !== 'object' || models === null || Array.isArray(models)) { diff --git a/packages/codev/src/lib/test-env.ts b/packages/codev/src/lib/test-env.ts index 40e9e6285..b16fb2fe6 100644 --- a/packages/codev/src/lib/test-env.ts +++ b/packages/codev/src/lib/test-env.ts @@ -92,6 +92,39 @@ export function assertAgyLaneAllowedUnderTest(): void { ); } +/** + * Explicit opt-in for deliberately exercising the REAL `opencode` binary from a test. + * + * Same shape as `realAgyOptIn`, for the same reason with a different cost: an unpinned opencode + * lane in a suite does not open a browser window, it bills a real Grok call and takes minutes. + */ +export function realOpencodeOptIn(): boolean { + const raw = process.env.CODEV_ALLOW_REAL_OPENCODE; + return raw === '1' || raw === 'true'; +} + +/** + * Guard the opencode lane against reaching the real binary from a test (#22). + * + * The agy guard's reasoning applies unchanged: the single call site is `resolveOpencodeBin()`, in + * the branch taken when `CODEV_OPENCODE_BIN` is unset, because resolution is not passive — the + * lane's pre-flight *executes* the candidate with `models` before any review runs. + * + * The lane hard-fails rather than skipping, so an unpinned suite would surface as a test failure + * either way; this makes the failure say what is actually wrong instead of "opencode exited 1". + */ +export function assertOpencodeLaneAllowedUnderTest(): void { + if (!isUnderTestRunner()) return; + if (realOpencodeOptIn()) return; + if (process.env.CODEV_OPENCODE_BIN) return; + throw new Error( + 'Refusing to resolve the opencode binary under a test runner without a pinned ' + + 'CODEV_OPENCODE_BIN (#22). This test reached the opencode consult lane by omission and ' + + 'would have spawned the real CLI — a billed Grok call per spawn. Pin a fake binary, or set ' + + 'CODEV_ALLOW_REAL_OPENCODE=1 if this test genuinely means to run the real CLI.', + ); +} + /** * True when cloud-mutating side effects must be refused because we are running * under a test (#1515).