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, + ); + }); +});