diff --git a/scripts/review/claude-run.ts b/scripts/review/claude-run.ts new file mode 100644 index 0000000..e89c67a --- /dev/null +++ b/scripts/review/claude-run.ts @@ -0,0 +1,51 @@ +import { isRecord, shellQuote } from "../../src/util.js"; +import { runOnce } from "../lib/subprocess.js"; +import type { ReviewConfig } from "./config.js"; +import type { ReviewRunner } from "./runner.js"; + +function checkResult(stdout: string): void { + for (const line of stdout.split("\n")) { + const trimmed = line.trim(); + if (!trimmed) continue; + let msg: unknown; + try { + msg = JSON.parse(trimmed); + } catch { + continue; + } + if (isRecord(msg) && msg.type === "result") { + if (msg.is_error === true) { + throw new Error( + `claude returned an error: ${typeof msg.result === "string" ? msg.result : "(no message)"}`, + ); + } + return; + } + } + throw new Error( + `claude stream-json output contained no {"type":"result"} line`, + ); +} + +export function createClaudeReviewRunner(config: ReviewConfig): ReviewRunner { + return { + async run(workspaceDir: string, prompt: string): Promise { + const { permissionMode, denyTools } = config.claudeCode; + const effectiveDenyTools = denyTools.length > 0 ? denyTools : []; + + let command = `claude -p --output-format stream-json --verbose --permission-mode ${shellQuote(permissionMode)}`; + if (effectiveDenyTools.length > 0) { + command += ` --disallowedTools ${shellQuote(effectiveDenyTools.join(","))}`; + } + if (config.agent.model) { + command += ` --model ${shellQuote(config.agent.model)}`; + } + + const stdout = await runOnce(command, prompt, { + timeoutMs: config.agent.timeoutMs, + cwd: workspaceDir, + }); + checkResult(stdout); + }, + }; +} diff --git a/scripts/review/config.ts b/scripts/review/config.ts index b6a8189..09353f3 100644 --- a/scripts/review/config.ts +++ b/scripts/review/config.ts @@ -32,6 +32,7 @@ export interface AgentConfig { export interface CopilotConfig { allowAllTools: boolean; allowTools: string[]; + denyTools: string[]; } export interface ClaudeCodeConfig { @@ -223,6 +224,11 @@ export function parseReviewConfig( cp.allow_tools.every((x) => typeof x === "string") ? (cp.allow_tools as string[]) : [], + denyTools: + Array.isArray(cp.deny_tools) && + cp.deny_tools.every((x) => typeof x === "string") + ? (cp.deny_tools as string[]) + : [], }; const cc = section(raw, "claude_code"); diff --git a/scripts/review/copilot-run.ts b/scripts/review/copilot-run.ts new file mode 100644 index 0000000..392a847 --- /dev/null +++ b/scripts/review/copilot-run.ts @@ -0,0 +1,71 @@ +import { randomUUID } from "node:crypto"; +import { shellQuote } from "../../src/util.js"; +import { runOnce } from "../lib/subprocess.js"; +import type { ReviewConfig } from "./config.js"; +import type { ReviewRunner } from "./runner.js"; + +function checkCopilotResult(stdout: string): void { + for (const line of stdout.split("\n")) { + const trimmed = line.trim(); + if (!trimmed) continue; + let msg: unknown; + try { + msg = JSON.parse(trimmed); + } catch { + continue; + } + if ( + typeof msg === "object" && + msg !== null && + "type" in msg && + msg.type === "result" + ) { + const exitCode = + "exitCode" in msg && typeof msg.exitCode === "number" + ? msg.exitCode + : 1; + if (exitCode !== 0) { + throw new Error(`copilot returned a non-zero exitCode: ${exitCode}`); + } + return; + } + } + throw new Error(`copilot json output contained no {"type":"result"} line`); +} + +export function createCopilotReviewRunner(config: ReviewConfig): ReviewRunner { + return { + async run(workspaceDir: string, prompt: string): Promise { + const promptBytes = Buffer.byteLength(prompt, "utf8"); + if (promptBytes > 128 * 1024) { + throw new Error( + `copilot review: prompt too large for argv (${promptBytes} bytes > 131072)`, + ); + } + + const sessionId = randomUUID(); + const { allowAllTools, allowTools, denyTools } = config.copilot; + + let command = `copilot -p ${shellQuote(prompt)} --output-format json --no-ask-user --log-level none --session-id ${sessionId}`; + if (config.agent.model) { + command += ` --model ${shellQuote(config.agent.model)}`; + } + if (allowAllTools) { + command += " --allow-all-tools"; + } else { + for (const tool of allowTools) { + command += ` --allow-tool=${shellQuote(tool)}`; + } + } + for (const tool of denyTools) { + command += ` --deny-tool=${shellQuote(tool)}`; + } + + const stdout = await runOnce(command, undefined, { + timeoutMs: config.agent.timeoutMs, + cwd: workspaceDir, + }); + checkCopilotResult(stdout); + }, + }; +} diff --git a/scripts/review/runner.ts b/scripts/review/runner.ts new file mode 100644 index 0000000..ef03f8f --- /dev/null +++ b/scripts/review/runner.ts @@ -0,0 +1,14 @@ +import { createClaudeReviewRunner } from "./claude-run.js"; +import type { ReviewConfig } from "./config.js"; +import { createCopilotReviewRunner } from "./copilot-run.js"; + +export interface ReviewRunner { + run(workspaceDir: string, prompt: string): Promise; +} + +export function createRunner(config: ReviewConfig): ReviewRunner { + if (config.agent.kind === "copilot") { + return createCopilotReviewRunner(config); + } + return createClaudeReviewRunner(config); +} diff --git a/test/review-runners.test.ts b/test/review-runners.test.ts new file mode 100644 index 0000000..65836d4 --- /dev/null +++ b/test/review-runners.test.ts @@ -0,0 +1,212 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; +import { createClaudeReviewRunner } from "../scripts/review/claude-run.js"; +import type { ReviewConfig } from "../scripts/review/config.js"; +import { createCopilotReviewRunner } from "../scripts/review/copilot-run.js"; + +vi.mock("../scripts/lib/subprocess.js", () => ({ + runOnce: vi.fn(), +})); + +import { runOnce } from "../scripts/lib/subprocess.js"; + +const mockRunOnce = vi.mocked(runOnce); + +function makeConfig(overrides: Partial = {}): ReviewConfig { + return { + tracker: { + token: "token", + owner: "acme", + ownerType: "organization", + projectNumber: 1, + statusField: "Status", + activeStates: ["Todo"], + repos: null, + }, + workspace: { + root: "/tmp/review", + hookTimeoutMs: 120000, + }, + hooks: { + afterCreate: null, + beforeRun: null, + }, + agent: { + kind: "claude_code", + timeoutMs: 30000, + model: null, + maxConcurrent: 1, + }, + copilot: { + allowAllTools: false, + allowTools: [], + denyTools: [], + }, + claudeCode: { + permissionMode: "bypassPermissions", + denyTools: [], + }, + ...overrides, + }; +} + +beforeEach(() => { + mockRunOnce.mockReset(); +}); + +describe("createClaudeReviewRunner", () => { + it("builds command with --verbose and optional flags, then validates result line", async () => { + mockRunOnce.mockResolvedValue( + JSON.stringify({ type: "result", is_error: false, result: "ok" }), + ); + const config = makeConfig({ + agent: { + kind: "claude_code", + timeoutMs: 12345, + model: "claude-opus-4.7", + maxConcurrent: 1, + }, + claudeCode: { + permissionMode: "acceptEdits", + denyTools: ["Bash(rm:*)", "Edit"], + }, + }); + + await createClaudeReviewRunner(config).run("/tmp/ws", "review this"); + + const [command, stdin, opts] = mockRunOnce.mock.calls.at(0) ?? []; + expect(command).toContain("--verbose"); + expect(command).toContain("--permission-mode 'acceptEdits'"); + expect(command).toContain("--disallowedTools 'Bash(rm:*),Edit'"); + expect(command).toContain("--model 'claude-opus-4.7'"); + expect(stdin).toBe("review this"); + expect(opts).toEqual({ timeoutMs: 12345, cwd: "/tmp/ws" }); + }); + + it("throws when stream-json output has no result line", async () => { + mockRunOnce.mockResolvedValue( + JSON.stringify({ type: "assistant", text: "partial" }), + ); + await expect( + createClaudeReviewRunner(makeConfig()).run("/tmp/ws", "p"), + ).rejects.toThrow('{"type":"result"}'); + }); + + it("throws when result line marks is_error=true", async () => { + mockRunOnce.mockResolvedValue( + JSON.stringify({ + type: "result", + is_error: true, + result: "Permission denied", + }), + ); + await expect( + createClaudeReviewRunner(makeConfig()).run("/tmp/ws", "p"), + ).rejects.toThrow("claude returned an error: Permission denied"); + }); +}); + +describe("createCopilotReviewRunner", () => { + it("uses --allow-all-tools for allowAllTools mode", async () => { + mockRunOnce.mockResolvedValue( + JSON.stringify({ type: "result", exitCode: 0 }), + ); + const config = makeConfig({ + agent: { + kind: "copilot", + timeoutMs: 22222, + model: "gpt-5", + maxConcurrent: 1, + }, + copilot: { allowAllTools: true, allowTools: [], denyTools: [] }, + }); + + await createCopilotReviewRunner(config).run("/tmp/ws", "run review"); + + const [command, stdin, opts] = mockRunOnce.mock.calls.at(0) ?? []; + expect(command).toContain("--allow-all-tools"); + expect(command).not.toContain("--allow-tool=*"); + expect(command).toContain("--model 'gpt-5'"); + expect(stdin).toBeUndefined(); + expect(opts).toEqual({ timeoutMs: 22222, cwd: "/tmp/ws" }); + }); + + it("includes allow/deny tool flags when configured", async () => { + mockRunOnce.mockResolvedValue( + JSON.stringify({ type: "result", exitCode: 0 }), + ); + const config = makeConfig({ + agent: { + kind: "copilot", + timeoutMs: 30000, + model: null, + maxConcurrent: 1, + }, + copilot: { + allowAllTools: false, + allowTools: ["Bash", "Read"], + denyTools: ["shell(git:*)", "Edit"], + }, + }); + + await createCopilotReviewRunner(config).run("/tmp/ws", "run review"); + + const [command] = mockRunOnce.mock.calls.at(0) ?? []; + expect(command).toContain("--allow-tool='Bash'"); + expect(command).toContain("--allow-tool='Read'"); + expect(command).toContain("--deny-tool='shell(git:*)'"); + expect(command).toContain("--deny-tool='Edit'"); + }); + + it("throws when prompt exceeds argv limit", async () => { + const oversizedPrompt = "a".repeat(128 * 1024 + 1); + await expect( + createCopilotReviewRunner( + makeConfig({ + agent: { + kind: "copilot", + timeoutMs: 30000, + model: null, + maxConcurrent: 1, + }, + }), + ).run("/tmp/ws", oversizedPrompt), + ).rejects.toThrow("prompt too large"); + expect(mockRunOnce).not.toHaveBeenCalled(); + }); + + it("throws when output has no result line", async () => { + mockRunOnce.mockResolvedValue( + JSON.stringify({ type: "assistant.message", data: { content: "ok" } }), + ); + await expect( + createCopilotReviewRunner( + makeConfig({ + agent: { + kind: "copilot", + timeoutMs: 30000, + model: null, + maxConcurrent: 1, + }, + }), + ).run("/tmp/ws", "p"), + ).rejects.toThrow('{"type":"result"}'); + }); + + it("throws when copilot result exitCode is non-zero", async () => { + mockRunOnce.mockResolvedValue( + JSON.stringify({ type: "result", exitCode: 1 }), + ); + await expect( + createCopilotReviewRunner( + makeConfig({ + agent: { + kind: "copilot", + timeoutMs: 30000, + model: null, + maxConcurrent: 1, + }, + }), + ).run("/tmp/ws", "p"), + ).rejects.toThrow("non-zero exitCode: 1"); + }); +});