From a919b5bd80362a4e22dd3334024525f2bb97a56d Mon Sep 17 00:00:00 2001 From: JohnnyVicious Date: Sun, 12 Apr 2026 17:59:07 +0200 Subject: [PATCH] fix(review): replace built-in plan agent with custom review agent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit OpenCode's built-in `plan` agent isn't just read-only — it injects a synthetic user-message directive on every turn ("Plan mode ACTIVE — STRICTLY FORBIDDEN... produce an implementation plan") which overrides whatever system prompt the caller sends and makes the model return an implementation plan instead of the requested review. This has been the root cause of `/opencode:review` and `/opencode:adversarial-review` returning implementation plans since the plugin was first ported from codex-plugin-cc, where the equivalent was `sandbox: "read-only"` — a syscall-level filesystem sandbox that doesn't touch the system prompt. This commit ships a dedicated `review` agent inside the plugin and threads OPENCODE_CONFIG_DIR through `ensureServer` so OpenCode discovers it when the companion spawns the server. The agent is read-only at the permission layer (deny `*`, allow only read/grep/glob/list) and has a neutral prompt body that tells the model to follow the caller's brief exactly without inventing implementation steps. Both review handlers and the stop-review-gate hook now resolve the agent through a shared `resolveReviewAgent` helper. When the custom agent isn't available (user already had `opencode serve` running without our config dir), the helper falls back to `build` + per-call `tools: {write:false, edit:false, ...}` overrides with a warning log, so reviews still work correctly even if the preferred path isn't taken. Also fixes a latent bug surfaced during end-to-end testing: the CLI passed `--model` as a plain string through to OpenCode's HTTP API, which expects `{providerID, modelID}` and rejected every invocation with HTTP 400. A new `parseModelString` helper splits on the first slash (preserving nested model ids like `openrouter/anthropic/...`) and callers convert at the boundary. Tests: - 10 unit tests for `resolveReviewAgent` covering array/object list shapes, missing agent fallback, listAgents errors, and tool mutation safety - 9 unit tests for `parseModelString` covering provider splitting, trimming, empty/invalid input, and non-string rejection - End-to-end verified by running the companion script against this branch's own working tree with `--model openrouter/anthropic/claude-haiku-4.5`: server spawned with OPENCODE_CONFIG_DIR, custom agent was resolved (job log shows `agent: review`), adversarial review returned with a structured needs-attention verdict + two findings (review-shaped, not plan-shaped) --- .github/workflows/ci.yml | 3 + .../opencode/opencode-config/agent/review.md | 35 +++++ plugins/opencode/scripts/lib/model.mjs | 45 ++++++ .../opencode/scripts/lib/opencode-server.mjs | 59 ++++++-- plugins/opencode/scripts/lib/review-agent.mjs | 90 ++++++++++++ .../opencode/scripts/opencode-companion.mjs | 40 +++++- .../scripts/stop-review-gate-hook.mjs | 9 +- tests/model.test.mjs | 63 +++++++++ tests/review-agent.test.mjs | 133 ++++++++++++++++++ 9 files changed, 459 insertions(+), 18 deletions(-) create mode 100644 plugins/opencode/opencode-config/agent/review.md create mode 100644 plugins/opencode/scripts/lib/model.mjs create mode 100644 plugins/opencode/scripts/lib/review-agent.mjs create mode 100644 tests/model.test.mjs create mode 100644 tests/review-agent.test.mjs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 784617a..4fa6629 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -44,10 +44,13 @@ jobs: - name: Syntax-check companion scripts run: | node --check plugins/opencode/scripts/opencode-companion.mjs + node --check plugins/opencode/scripts/stop-review-gate-hook.mjs node --check plugins/opencode/scripts/lib/git.mjs node --check plugins/opencode/scripts/lib/prompts.mjs node --check plugins/opencode/scripts/lib/process.mjs node --check plugins/opencode/scripts/lib/opencode-server.mjs + node --check plugins/opencode/scripts/lib/review-agent.mjs + node --check plugins/opencode/scripts/lib/model.mjs node --check scripts/bump-version.mjs - name: Run tests diff --git a/plugins/opencode/opencode-config/agent/review.md b/plugins/opencode/opencode-config/agent/review.md new file mode 100644 index 0000000..33da513 --- /dev/null +++ b/plugins/opencode/opencode-config/agent/review.md @@ -0,0 +1,35 @@ +--- +description: Read-only code review agent for opencode-plugin-cc. Faithfully follows the per-call system prompt provided by the plugin, without injecting plan-mode instructions that would turn review briefs into implementation plans. +mode: primary +permission: + "*": deny + read: + "*": allow + grep: allow + glob: allow + list: allow + edit: deny + write: deny + patch: deny + bash: + "*": deny + webfetch: deny + websearch: deny + task: + "*": deny + external_directory: + "*": deny +--- +You are the opencode-plugin-cc review agent. + +Your job is to perform the code review described in the user message exactly as specified. The user message contains a complete review brief — the target, the focus, the repository context, and the required output format. Treat that brief as authoritative and follow it literally. + +Operating rules: +- Do not modify any files. Do not run any shell commands. Do not create, edit, or delete anything. +- Do not ask clarifying questions. Produce the review with the information provided in the brief. +- If the brief asks for structured JSON output, return only valid JSON matching the requested schema. Do not wrap it in prose or markdown unless explicitly asked. +- If the brief asks for narrative output, follow the tone and structure it prescribes. +- Do not produce an implementation plan, step-by-step instructions, or suggestions for follow-up work unless the brief explicitly requests them. +- Ground every finding in the repository context provided in the brief. Do not invent files, lines, or behaviors you cannot point to. + +You are read-only at the permission layer. Do not attempt to invoke write tools — they will be denied. diff --git a/plugins/opencode/scripts/lib/model.mjs b/plugins/opencode/scripts/lib/model.mjs new file mode 100644 index 0000000..45287ea --- /dev/null +++ b/plugins/opencode/scripts/lib/model.mjs @@ -0,0 +1,45 @@ +// Model-string parsing for OpenCode's HTTP API. +// +// The CLI accepts `--model /` as a plain string +// (e.g. `openrouter/anthropic/claude-haiku-4.5`), but OpenCode's +// `POST /session/:id/message` endpoint rejects a string in the `model` +// field with HTTP 400: +// +// {"error":[{"expected":"object","path":["model"], +// "message":"Invalid input: expected object, received string"}]} +// +// It expects `{ providerID, modelID }` instead. This helper parses the +// CLI string into that shape. The first `/` splits provider from model +// id, so `openrouter/anthropic/claude-haiku-4.5` → providerID +// "openrouter", modelID "anthropic/claude-haiku-4.5". Any remaining +// slashes belong to the model id because providers frequently namespace +// their models (e.g. `anthropic/...`). +// +// (Apache License 2.0 §4(b) modification notice — see NOTICE.) + +/** + * Parse a `"provider/model-id"` CLI string into OpenCode's expected + * `{providerID, modelID}` shape. Returns null for empty/invalid input + * so callers can leave the model field unset and let OpenCode use the + * user's configured default. + * + * @param {string|undefined|null} input + * @returns {{ providerID: string, modelID: string } | null} + */ +export function parseModelString(input) { + if (typeof input !== "string") return null; + const trimmed = input.trim(); + if (!trimmed) return null; + + const slash = trimmed.indexOf("/"); + if (slash === -1) { + // No provider prefix — opencode can't route this, so treat as invalid. + return null; + } + + const providerID = trimmed.slice(0, slash); + const modelID = trimmed.slice(slash + 1); + if (!providerID || !modelID) return null; + + return { providerID, modelID }; +} diff --git a/plugins/opencode/scripts/lib/opencode-server.mjs b/plugins/opencode/scripts/lib/opencode-server.mjs index 14614bd..4c17208 100644 --- a/plugins/opencode/scripts/lib/opencode-server.mjs +++ b/plugins/opencode/scripts/lib/opencode-server.mjs @@ -2,21 +2,42 @@ // Unlike codex-plugin-cc which uses JSON-RPC over stdin/stdout, // OpenCode exposes a REST API + SSE. This module wraps that API. // -// Modified by JohnnyVicious (2026): `ensureServer` now spawns opencode -// with `stdio: "ignore"` instead of piping stdout/stderr that nothing -// reads. The piped streams were ref'd handles on the parent event loop, -// which deadlocked any long-lived parent (e.g. `node:test`) once -// opencode wrote enough log output to fill the pipe buffer. In normal -// CLI usage the deadlock was masked because the companion script exited -// before the buffer filled. (Apache License 2.0 §4(b) modification -// notice — see NOTICE.) +// Modified by JohnnyVicious (2026): +// - `ensureServer` spawns opencode with `stdio: "ignore"` instead of +// piping stdout/stderr that nothing reads. The piped streams were +// ref'd handles on the parent event loop, which deadlocked any +// long-lived parent (e.g. `node:test`) once opencode wrote enough +// log output to fill the pipe buffer. In normal CLI usage the +// deadlock was masked because the companion script exited before +// the buffer filled. +// - `ensureServer` also threads `OPENCODE_CONFIG_DIR` into the spawned +// server so our bundled `opencode-config/agent/review.md` custom +// agent is discovered. We prefer a dedicated read-only agent over +// OpenCode's built-in `plan` agent for reviews: `plan` injects a +// synthetic user-message directive ("Plan mode ACTIVE... produce an +// implementation plan") that overrides our review prompt and causes +// OpenCode to return plans instead of reviews. +// (Apache License 2.0 §4(b) modification notice — see NOTICE.) import { spawn } from "node:child_process"; +import path from "node:path"; const DEFAULT_PORT = 4096; const DEFAULT_HOST = "127.0.0.1"; const SERVER_START_TIMEOUT = 30_000; +/** + * Resolve the bundled opencode config directory shipped inside the plugin. + * This is what we pass as OPENCODE_CONFIG_DIR so the custom `review` agent + * (at `opencode-config/agent/review.md`) gets discovered. + * @returns {string|null} + */ +export function getBundledConfigDir() { + const pluginRoot = process.env.CLAUDE_PLUGIN_ROOT; + if (!pluginRoot) return null; + return path.join(pluginRoot, "opencode-config"); +} + /** * Check if an OpenCode server is already running on the given port. * @param {string} host @@ -56,10 +77,24 @@ export async function ensureServer(opts = {}) { // them creates ref'd file descriptors on the parent that prevent any // long-lived parent (notably `node:test`) from exiting cleanly once // opencode writes enough output to fill the pipe buffer. + // + // `OPENCODE_CONFIG_DIR` points opencode at our bundled config dir so + // the custom `review` agent is discovered. We only set it when we + // actually spawn the server — if the user already has a server + // running, they get whatever config that server was started with, and + // the caller is expected to fall back to `build` when `review` is + // unavailable. + const env = { ...process.env }; + const bundledConfigDir = getBundledConfigDir(); + if (bundledConfigDir) { + env.OPENCODE_CONFIG_DIR = bundledConfigDir; + } + const proc = spawn("opencode", ["serve", "--port", String(port)], { stdio: "ignore", detached: true, cwd: opts.cwd, + env, }); proc.unref(); @@ -148,6 +183,10 @@ export function createClient(baseUrl, opts = {}) { if (opts.agent) body.agent = opts.agent; if (opts.model) body.model = opts.model; if (opts.system) body.system = opts.system; + // `tools` is a per-call override map: `{ write: false, edit: false, ... }`. + // Used by the review fallback path to enforce read-only behavior when + // the custom `review` agent isn't available on a pre-running server. + if (opts.tools) body.tools = opts.tools; const res = await fetch(`${baseUrl}/session/${sessionId}/message`, { method: "POST", @@ -204,7 +243,7 @@ export function createClient(baseUrl, opts = {}) { * @returns {Promise & { serverInfo: object }>} */ export async function connect(opts = {}) { - const { url } = await ensureServer(opts); + const { url, alreadyRunning } = await ensureServer(opts); const client = createClient(url, { directory: opts.cwd }); - return { ...client, serverInfo: { url } }; + return { ...client, serverInfo: { url, alreadyRunning } }; } diff --git a/plugins/opencode/scripts/lib/review-agent.mjs b/plugins/opencode/scripts/lib/review-agent.mjs new file mode 100644 index 0000000..187a37a --- /dev/null +++ b/plugins/opencode/scripts/lib/review-agent.mjs @@ -0,0 +1,90 @@ +// Shared review-agent resolution for the OpenCode companion. +// +// The plugin reviews code (both ordinary and adversarial) by sending a +// carefully constructed system prompt to OpenCode. We used to route +// those requests through OpenCode's built-in `plan` agent on the +// assumption that it was the read-only counterpart to `build`. It is +// read-only, but it also injects a synthetic user-message directive on +// every turn ("Plan mode ACTIVE — STRICTLY FORBIDDEN... produce an +// implementation plan") which overrides our review prompt and makes +// the model return implementation plans instead of reviews. +// +// The fix is a dedicated `review` agent shipped inside the plugin at +// `plugins/opencode/opencode-config/agent/review.md`, discovered via +// OPENCODE_CONFIG_DIR when the plugin spawns `opencode serve`. +// +// This module centralizes the "pick the right agent" decision so the +// companion script and the stop-review-gate hook cannot drift. +// +// (Apache License 2.0 §4(b) modification notice — see NOTICE.) + +/** + * Read-only tool overrides used when the custom `review` agent isn't + * available and we have to fall back to the `build` agent. This + * enforces read-only behavior at the per-call level so a misbehaving + * model can't edit files or shell out, even though `build` is + * normally read-write. + * + * Frozen so callers cannot accidentally mutate the shared object — + * each consumer should spread it if they need to add keys. + */ +export const READ_ONLY_TOOL_OVERRIDES = Object.freeze({ + write: false, + edit: false, + patch: false, + multiedit: false, + bash: false, + task: false, + webfetch: false, +}); + +/** + * Extract agent names from a `listAgents()` response. OpenCode's + * `/agent` endpoint has returned both array-shaped and object-shaped + * payloads across versions, so we handle both. + * @param {unknown} agents + * @returns {string[]} + */ +function extractAgentNames(agents) { + if (Array.isArray(agents)) { + return agents.map((a) => a?.name).filter((name) => typeof name === "string"); + } + if (agents && typeof agents === "object") { + return Object.keys(agents); + } + return []; +} + +/** + * Decide which agent to use for reviews. Prefer the custom `review` + * agent shipped inside the plugin. If it's not available on the + * server we're talking to — typically because the user already had + * `opencode serve` running without our OPENCODE_CONFIG_DIR — fall + * back to `build` with per-call read-only tool overrides and log a + * warning so the caller knows why. + * + * @param {{ listAgents: () => Promise }} client + * @param {(msg: string) => void} [log] + * @returns {Promise<{ agent: string, tools?: object }>} + */ +export async function resolveReviewAgent(client, log = () => {}) { + try { + const agents = await client.listAgents(); + const names = extractAgentNames(agents); + + if (names.includes("review")) { + return { agent: "review" }; + } + + log( + "Custom `review` agent not found on this server. Falling back to " + + "`build` with read-only tool overrides. To get the preferred path, " + + "stop any pre-existing `opencode serve` so the plugin can restart " + + "it with OPENCODE_CONFIG_DIR pointing at the bundled config." + ); + } catch (err) { + log(`Could not list agents (${err.message}); falling back to build + tool overrides.`); + } + + return { agent: "build", tools: { ...READ_ONLY_TOOL_OVERRIDES } }; +} diff --git a/plugins/opencode/scripts/opencode-companion.mjs b/plugins/opencode/scripts/opencode-companion.mjs index 2e99b80..137d04e 100644 --- a/plugins/opencode/scripts/opencode-companion.mjs +++ b/plugins/opencode/scripts/opencode-companion.mjs @@ -16,7 +16,23 @@ // user's configured credentials; // - extract the model OpenCode actually used (from `response.info.model`) // and prepend it as a `**Model:** ...` header to every review output -// so users always see which model produced the review. +// so users always see which model produced the review; +// - switch reviews from OpenCode's built-in `plan` agent to a custom +// `review` agent shipped in the plugin. OpenCode's `plan` agent +// injects a synthetic user-message directive ("Plan mode ACTIVE — +// STRICTLY FORBIDDEN... produce an implementation plan") on every +// turn, which dominates our review system prompt and makes OpenCode +// return implementation plans instead of the requested review. A +// custom agent with read-only permissions and a neutral prompt body +// sidesteps the injection entirely. When our agent isn't available +// (e.g. the user already had a server running without our config +// dir), we fall back to the `build` agent with per-call tool +// restrictions and a warning; +// - parse `--model /` into OpenCode's required +// `{providerID, modelID}` object before sending. Passing the raw +// CLI string caused HTTP 400 ("expected object, received string") +// on every `--model` invocation — the original threading commit +// wired the argument through but never adapted the shape. // (Apache License 2.0 §4(b) modification notice.) import path from "node:path"; @@ -41,6 +57,8 @@ import { import { buildReviewPrompt, buildTaskPrompt } from "./lib/prompts.mjs"; import { getDiff, getStatus as getGitStatus, detectPrReference } from "./lib/git.mjs"; import { readJson } from "./lib/fs.mjs"; +import { resolveReviewAgent } from "./lib/review-agent.mjs"; +import { parseModelString } from "./lib/model.mjs"; const PLUGIN_ROOT = process.env.CLAUDE_PLUGIN_ROOT || path.resolve(import.meta.dirname, ".."); @@ -169,12 +187,16 @@ async function handleReview(argv) { adversarial: false, }, PLUGIN_ROOT); + const reviewAgent = await resolveReviewAgent(client, log); + const model = parseModelString(options.model); + report("reviewing", "Running review..."); - log(`Prompt length: ${prompt.length} chars${options.model ? `, model: ${options.model}` : ""}${prNumber ? `, pr: #${prNumber}` : ""}`); + log(`Prompt length: ${prompt.length} chars, agent: ${reviewAgent.agent}${options.model ? `, model: ${options.model}` : ""}${prNumber ? `, pr: #${prNumber}` : ""}`); const response = await client.sendPrompt(session.id, prompt, { - agent: "plan", // read-only agent for reviews - model: options.model, + agent: reviewAgent.agent, + model, + tools: reviewAgent.tools, }); report("finalizing", "Processing review output..."); @@ -249,12 +271,16 @@ async function handleAdversarialReview(argv) { focus, }, PLUGIN_ROOT); + const reviewAgent = await resolveReviewAgent(client, log); + const model = parseModelString(options.model); + report("reviewing", "Running adversarial review..."); - log(`Prompt length: ${prompt.length} chars, focus: ${focus || "(none)"}${options.model ? `, model: ${options.model}` : ""}${prNumber ? `, pr: #${prNumber}` : ""}`); + log(`Prompt length: ${prompt.length} chars, agent: ${reviewAgent.agent}, focus: ${focus || "(none)"}${options.model ? `, model: ${options.model}` : ""}${prNumber ? `, pr: #${prNumber}` : ""}`); const response = await client.sendPrompt(session.id, prompt, { - agent: "plan", - model: options.model, + agent: reviewAgent.agent, + model, + tools: reviewAgent.tools, }); report("finalizing", "Processing review output..."); diff --git a/plugins/opencode/scripts/stop-review-gate-hook.mjs b/plugins/opencode/scripts/stop-review-gate-hook.mjs index 66a01d5..699d21a 100644 --- a/plugins/opencode/scripts/stop-review-gate-hook.mjs +++ b/plugins/opencode/scripts/stop-review-gate-hook.mjs @@ -10,6 +10,7 @@ import path from "node:path"; import { resolveWorkspace } from "./lib/workspace.mjs"; import { loadState } from "./lib/state.mjs"; import { isServerRunning, connect } from "./lib/opencode-server.mjs"; +import { resolveReviewAgent } from "./lib/review-agent.mjs"; const PLUGIN_ROOT = process.env.CLAUDE_PLUGIN_ROOT || path.resolve(import.meta.dirname, ".."); @@ -60,8 +61,14 @@ async function main() { const client = await connect({ cwd: workspace }); const session = await client.createSession({ title: "Stop Review Gate" }); + // Prefer the custom `review` agent; fall back to `build` + tool + // overrides if the running server doesn't have our custom agent. + // See lib/review-agent.mjs for the rationale. + const reviewAgent = await resolveReviewAgent(client); + const response = await client.sendPrompt(session.id, prompt, { - agent: "plan", // read-only review + agent: reviewAgent.agent, + tools: reviewAgent.tools, }); // Extract the verdict diff --git a/tests/model.test.mjs b/tests/model.test.mjs new file mode 100644 index 0000000..c6454c7 --- /dev/null +++ b/tests/model.test.mjs @@ -0,0 +1,63 @@ +// Unit tests for scripts/lib/model.mjs. +// +// These assert that the CLI `--model /` string is +// transformed into the `{providerID, modelID}` object shape that +// OpenCode's `POST /session/:id/message` endpoint expects. + +import { describe, it } from "node:test"; +import assert from "node:assert/strict"; +import { parseModelString } from "../plugins/opencode/scripts/lib/model.mjs"; + +describe("parseModelString", () => { + it("splits on the first slash so nested model ids stay intact", () => { + assert.deepEqual(parseModelString("openrouter/anthropic/claude-haiku-4.5"), { + providerID: "openrouter", + modelID: "anthropic/claude-haiku-4.5", + }); + }); + + it("handles a flat provider/model pair", () => { + assert.deepEqual(parseModelString("anthropic/claude-sonnet-4-5"), { + providerID: "anthropic", + modelID: "claude-sonnet-4-5", + }); + }); + + it("handles provider/model ids that contain many slashes", () => { + assert.deepEqual(parseModelString("openrouter/meta-llama/llama-3.1/70b-instruct"), { + providerID: "openrouter", + modelID: "meta-llama/llama-3.1/70b-instruct", + }); + }); + + it("trims surrounding whitespace", () => { + assert.deepEqual(parseModelString(" openrouter/anthropic/claude-haiku-4.5 "), { + providerID: "openrouter", + modelID: "anthropic/claude-haiku-4.5", + }); + }); + + it("returns null for an empty string", () => { + assert.equal(parseModelString(""), null); + assert.equal(parseModelString(" "), null); + }); + + it("returns null for a missing provider prefix", () => { + assert.equal(parseModelString("claude-haiku-4.5"), null); + }); + + it("returns null when the provider is empty (leading slash)", () => { + assert.equal(parseModelString("/claude-haiku-4.5"), null); + }); + + it("returns null when the model id is empty (trailing slash)", () => { + assert.equal(parseModelString("openrouter/"), null); + }); + + it("returns null for non-string input", () => { + assert.equal(parseModelString(null), null); + assert.equal(parseModelString(undefined), null); + assert.equal(parseModelString(42), null); + assert.equal(parseModelString({ providerID: "x", modelID: "y" }), null); + }); +}); diff --git a/tests/review-agent.test.mjs b/tests/review-agent.test.mjs new file mode 100644 index 0000000..979e0da --- /dev/null +++ b/tests/review-agent.test.mjs @@ -0,0 +1,133 @@ +// Unit tests for scripts/lib/review-agent.mjs. +// +// These tests mock the opencode client's `listAgents` method directly +// rather than standing up a real server. That keeps them fast and +// deterministic — they assert the resolver's decision logic, not the +// HTTP round trip. + +import { describe, it } from "node:test"; +import assert from "node:assert/strict"; +import { + resolveReviewAgent, + READ_ONLY_TOOL_OVERRIDES, +} from "../plugins/opencode/scripts/lib/review-agent.mjs"; + +describe("resolveReviewAgent", () => { + it("returns {agent: 'review'} when review is in an array-shaped listAgents response", async () => { + const client = { + listAgents: async () => [ + { name: "build" }, + { name: "plan" }, + { name: "review" }, + ], + }; + const logs = []; + const result = await resolveReviewAgent(client, (m) => logs.push(m)); + + assert.equal(result.agent, "review"); + assert.equal(result.tools, undefined); + assert.deepEqual(logs, []); + }); + + it("returns {agent: 'review'} when review is in an object-shaped listAgents response", async () => { + const client = { + listAgents: async () => ({ + build: { description: "..." }, + review: { description: "..." }, + plan: { description: "..." }, + }), + }; + const result = await resolveReviewAgent(client, () => {}); + assert.equal(result.agent, "review"); + }); + + it("falls back to {agent: 'build', tools: READ_ONLY_TOOL_OVERRIDES} when review is missing", async () => { + const client = { + listAgents: async () => [{ name: "build" }, { name: "plan" }], + }; + const logs = []; + const result = await resolveReviewAgent(client, (m) => logs.push(m)); + + assert.equal(result.agent, "build"); + assert.deepEqual(result.tools, { ...READ_ONLY_TOOL_OVERRIDES }); + assert.equal(logs.length, 1); + assert.match(logs[0], /Custom `review` agent not found/); + assert.match(logs[0], /OPENCODE_CONFIG_DIR/); + }); + + it("falls back when listAgents throws and logs the error message", async () => { + const client = { + listAgents: async () => { + throw new Error("network exploded"); + }, + }; + const logs = []; + const result = await resolveReviewAgent(client, (m) => logs.push(m)); + + assert.equal(result.agent, "build"); + assert.deepEqual(result.tools, { ...READ_ONLY_TOOL_OVERRIDES }); + assert.equal(logs.length, 1); + assert.match(logs[0], /Could not list agents/); + assert.match(logs[0], /network exploded/); + }); + + it("tolerates array entries with non-string/missing name fields", async () => { + const client = { + listAgents: async () => [ + null, + { name: null }, + { name: 42 }, + { name: "review" }, + ], + }; + const result = await resolveReviewAgent(client, () => {}); + assert.equal(result.agent, "review"); + }); + + it("treats a null listAgents response as 'review not available'", async () => { + const client = { listAgents: async () => null }; + const logs = []; + const result = await resolveReviewAgent(client, (m) => logs.push(m)); + assert.equal(result.agent, "build"); + assert.ok(result.tools); + assert.equal(logs.length, 1); + }); + + it("defaults log to a no-op when none is provided", async () => { + const client = { + listAgents: async () => { + throw new Error("boom"); + }, + }; + // Must not throw. + const result = await resolveReviewAgent(client); + assert.equal(result.agent, "build"); + }); + + it("returned tools object is a fresh copy, not a reference to the frozen constant", async () => { + const client = { listAgents: async () => [] }; + const result = await resolveReviewAgent(client, () => {}); + assert.notEqual(result.tools, READ_ONLY_TOOL_OVERRIDES); + // Mutating the returned tools must not affect the frozen constant. + result.tools.custom = true; + assert.equal(READ_ONLY_TOOL_OVERRIDES.custom, undefined); + }); +}); + +describe("READ_ONLY_TOOL_OVERRIDES", () => { + it("denies every write-capable tool", () => { + for (const tool of ["write", "edit", "patch", "multiedit", "bash", "task", "webfetch"]) { + assert.equal(READ_ONLY_TOOL_OVERRIDES[tool], false, `${tool} must be denied`); + } + }); + + it("is frozen so callers can't accidentally mutate the shared constant", () => { + assert.ok(Object.isFrozen(READ_ONLY_TOOL_OVERRIDES)); + assert.throws( + () => { + READ_ONLY_TOOL_OVERRIDES.write = true; + }, + TypeError, + ); + }); +});