diff --git a/AGENTS.md b/AGENTS.md index f6b84eea4..72f3504ca 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -311,7 +311,7 @@ During normal plan review, an Archive sidebar tab provides the same browsing via | `/api/external-annotations` | POST | Add external annotations (single or batch `{ annotations: [...] }`) | | `/api/external-annotations` | PATCH | Update fields on a single annotation (`?id=`) | | `/api/external-annotations` | DELETE | Remove by `?id=`, `?source=`, or clear all | -| `/api/agents/capabilities` | GET | Check available agent providers (claude, codex, tour) | +| `/api/agents/capabilities` | GET | Check available agent providers (claude, codex, tour, cursor, opencode) | | `/api/agents/review-profiles` | GET | List launchable review profiles (enabled skills + builtin default) | | `/api/agents/skills` | GET | List all discovered skills for the add-a-review picker (each flagged `enabled`) | | `/api/agents/review-skills` | POST | Enable a skill as a review (body: `{ name }`); writes `review-skills.json` | diff --git a/apps/pi-extension/server/agent-jobs.ts b/apps/pi-extension/server/agent-jobs.ts index fd91d6854..2eee5b079 100644 --- a/apps/pi-extension/server/agent-jobs.ts +++ b/apps/pi-extension/server/agent-jobs.ts @@ -8,7 +8,10 @@ */ import type { IncomingMessage, ServerResponse } from "node:http"; -import { spawn, execFileSync, type ChildProcess } from "node:child_process"; +import { spawn, execFileSync, execFile, type ChildProcess } from "node:child_process"; +import { promisify } from "node:util"; + +const execFileAsync = promisify(execFile); import { type AgentJobInfo, type AgentJobEvent, @@ -21,6 +24,12 @@ import { AGENT_HEARTBEAT_INTERVAL_MS, } from "../generated/agent-jobs.js"; import { formatClaudeLogEvent } from "../generated/claude-review.js"; +import { + MARKER_ENGINES, + formatMarkerLogEvent, + type MarkerEngine, + type MarkerModel, +} from "../generated/marker-review.js"; import { json, parseBody } from "./helpers.js"; // --------------------------------------------------------------------------- @@ -32,6 +41,16 @@ const JOBS = `${BASE}/jobs`; const JOBS_STREAM = `${JOBS}/stream`; const CAPABILITIES = `${BASE}/capabilities`; +// Providers whose command is owned by the server. Client-supplied argv is never +// spawned for these — buildCommand must produce the command or the launch fails. +const SERVER_BUILT_PROVIDERS: ReadonlySet = new Set([ + "claude", + "codex", + "tour", + "cursor", + "opencode", +]); + // --------------------------------------------------------------------------- // which() helper for Node.js // --------------------------------------------------------------------------- @@ -54,7 +73,7 @@ export interface AgentJobHandlerOptions { mode: "plan" | "review" | "annotate"; getServerUrl: () => string; getCwd: () => string; - /** Server-side command builder for known providers (codex, claude, tour). */ + /** Build the command server-side for a given provider. */ buildCommand?: (provider: string, config?: Record) => Promise<{ command: string[]; outputPath?: string; @@ -88,6 +107,27 @@ export interface AgentJobHandlerOptions { onJobComplete?: (job: AgentJobInfo, meta: { outputPath?: string; stdout?: string; cwd?: string }) => void | Promise; } +/** + * Best-effort model catalog for a marker engine, spawned once. The spawn lives + * HERE (per-runtime — child_process execFile) rather than in marker-review.ts, + * which must stay Bun-free for the Pi vendor build. ASYNC so it never blocks the + * event loop on the /capabilities request path (a slow/hanging CLI would otherwise + * freeze every other in-flight request for up to the timeout). Empty when discovery + * fails or the CLI is unauthenticated / has no providers configured — the UI falls + * back to the engine's default picker. Account/config-specific, so never hardcoded. + */ +async function discoverMarkerModels(engine: MarkerEngine): Promise { + try { + const { stdout } = await execFileAsync(engine.binary, engine.modelsArgv, { + timeout: 5000, + encoding: "utf8", + }); + return engine.parseModels(stdout); + } catch { + return []; + } +} + export function createAgentJobHandler(options: AgentJobHandlerOptions) { const { mode, getServerUrl, getCwd } = options; @@ -103,11 +143,32 @@ export function createAgentJobHandler(options: AgentJobHandlerOptions) { { id: "codex", name: "Codex CLI", available: whichCmd("codex") }, { id: "tour", name: "Code Tour", available: whichCmd("claude") || whichCmd("codex") }, ]; - const capabilitiesResponse: AgentCapabilities = { - mode, - providers: capabilities, - available: capabilities.some((c) => c.available), - }; + // Marker engines (Cursor, OpenCode) — same shape, one loop. Available only in + // review mode when the binary is on PATH (NOTE: cursor's binary is `agent`). + // Model catalogs are discovered LAZILY (see buildCapabilitiesResponse) so a + // slow/unauthenticated ` models` spawn never blocks startup. + for (const engine of Object.values(MARKER_ENGINES)) { + capabilities.push({ + id: engine.id, + name: engine.name, + available: mode === "review" && whichCmd(engine.binary), + }); + } + + const markerModelsCache = new Map(); + async function buildCapabilitiesResponse(): Promise { + const providers = await Promise.all(capabilities.map(async (c) => { + const engine = MARKER_ENGINES[c.id as "cursor" | "opencode"]; + if (!engine || !c.available) return c; + let models = markerModelsCache.get(engine.id); + if (!models) { + models = await discoverMarkerModels(engine); + markerModelsCache.set(engine.id, models); + } + return { ...c, models }; + })); + return { mode, providers, available: providers.some((p) => p.available) }; + } // --- SSE broadcasting --- function broadcast(event: AgentJobEvent): void { @@ -190,31 +251,47 @@ export function createAgentJobHandler(options: AgentJobHandlerOptions) { if (spawnOptions?.cwd) jobOutputPaths.set(`${id}:cwd`, spawnOptions.cwd); broadcast({ type: "job:started", job: { ...info } }); - // --- Stdout capture (Claude JSONL streaming) --- + // --- Stdout capture (Claude/Cursor stream-json) --- let stdoutBuf = ""; if (captureStdout && proc.stdout) { + // Format one complete JSONL line into a live-log delta (skip result + // events — handled in onJobComplete). + const emitLogLine = (line: string) => { + if (!line.trim()) return; + // Tour jobs with the Claude engine also stream Claude JSONL. + if (provider === "claude" || spawnOptions?.engine === "claude") { + const formatted = formatClaudeLogEvent(line); + if (formatted !== null) broadcast({ type: "job:log", jobId: id, delta: formatted + '\n' }); + return; + } + // Marker engines (Cursor, OpenCode): map their NDJSON stream events + // into readable log deltas via the engine's own formatter (Cursor + // applies the partial-output dedup rule; OpenCode reads text parts). + const markerEngine = MARKER_ENGINES[provider as "cursor" | "opencode"]; + if (markerEngine) { + const formatted = formatMarkerLogEvent(line, markerEngine); + if (formatted !== null) broadcast({ type: "job:log", jobId: id, delta: formatted + '\n' }); + return; + } + try { + const event = JSON.parse(line); + if (event.type === 'result') return; + } catch { /* not JSON — forward as raw log */ } + broadcast({ type: "job:log", jobId: id, delta: line + '\n' }); + }; + // stream-json output is NDJSON and chunk boundaries are arbitrary — + // carry the trailing partial line until a later chunk completes it, + // otherwise records split across chunks are dropped from live logs. + let logLineCarry = ""; proc.stdout.on("data", (chunk: Buffer) => { const text = chunk.toString(); stdoutBuf += text; - - // Forward JSONL lines as log events - const lines = text.split('\n'); - for (const line of lines) { - if (!line.trim()) continue; - // Tour jobs with the Claude engine also stream Claude JSONL. - if (provider === "claude" || spawnOptions?.engine === "claude") { - const formatted = formatClaudeLogEvent(line); - if (formatted !== null) { - broadcast({ type: "job:log", jobId: id, delta: formatted + '\n' }); - } - continue; - } - try { - const event = JSON.parse(line); - if (event.type === 'result') continue; - } catch { /* not JSON — forward as raw log */ } - broadcast({ type: "job:log", jobId: id, delta: line + '\n' }); - } + const lines = (logLineCarry + text).split('\n'); + logLineCarry = lines.pop() ?? ""; + for (const line of lines) emitLogLine(line); + }); + proc.stdout.on("end", () => { + if (logLineCarry) emitLogLine(logLineCarry); }); } @@ -272,8 +349,15 @@ export function createAgentJobHandler(options: AgentJobHandlerOptions) { stdout: captureStdout ? stdoutBuf : undefined, cwd: jobCwd, }); - } catch { - // Result ingestion failure shouldn't prevent job completion broadcast + } catch (err) { + // Claude/Codex are fail-open; Cursor and OpenCode are fail-closed — an + // unexpected throw during prompt-enforced ingestion must fail the job, + // not pass it. (Their handlers normally fail by mutation and never + // throw; this guards future refactors.) + if (MARKER_ENGINES[provider as "cursor" | "opencode"]) { + entry.info.status = "failed"; + entry.info.error = err instanceof Error ? err.message : `${provider} result ingestion failed`; + } } } jobOutputPaths.delete(id); @@ -350,7 +434,7 @@ export function createAgentJobHandler(options: AgentJobHandlerOptions) { ): Promise { // --- GET /api/agents/capabilities --- if (url.pathname === CAPABILITIES && req.method === "GET") { - json(res, capabilitiesResponse); + json(res, await buildCapabilitiesResponse()); return true; } @@ -440,6 +524,19 @@ export function createAgentJobHandler(options: AgentJobHandlerOptions) { return true; } + // Fail-closed enforcement for server-owned providers: the command MUST + // be built server-side. Client-supplied argv is never spawned for these + // providers — a null/throwing builder becomes an error, not a fallback. + if (SERVER_BUILT_PROVIDERS.has(provider)) { + if (!options.buildCommand) { + json(res, { error: `Provider ${provider} requires server-built command` }, 400); + return true; + } + // Discard any client-supplied argv so a null build cleanly hits the + // `command.length === 0` guard below instead of falling through. + command = []; + } + // Try server-side command building for known providers let captureStdout = false; let stdinPrompt: string | undefined; diff --git a/apps/pi-extension/server/serverReview.ts b/apps/pi-extension/server/serverReview.ts index 6c4f518fa..43ee06590 100644 --- a/apps/pi-extension/server/serverReview.ts +++ b/apps/pi-extension/server/serverReview.ts @@ -90,6 +90,15 @@ import { transformClaudeFindings, } from "../generated/claude-review.js"; import { createTourSession, TOUR_EMPTY_OUTPUT_ERROR } from "../generated/tour-review.js"; +import { + MARKER_ENGINES, + composeMarkerReviewPrompt, + buildMarkerCommand, + parseMarkerStreamOutput, + transformMarkerFindings, + makeMarkerNonce, + extractMarkerNonce, +} from "../generated/marker-review.js"; import { WorkspaceReviewSession, type WorkspaceDiffType, @@ -588,6 +597,24 @@ export async function startReviewServer(options: { return { command, stdinPrompt, prompt, cwd, label: jobLabel, captureStdout: true, model, effort, prUrl: launchPrUrl, diffScope: launchDiffScope, diffContext, reviewProfileId: reviewProfile.id, reviewProfileLabel: reviewProfile.label }; } + // Marker engines (Cursor, OpenCode) — one branch, same shape as Claude. + // Neither CLI has a schema flag, so composeMarkerReviewPrompt ALWAYS + // appends the marker-block output contract (even for a custom profile — + // it's the only thing that makes their prose output parseable). The + // engine's buildArgv passes the prompt as the trailing positional arg and + // threads the spawn cwd (--workspace for Cursor, --dir for OpenCode). + // captureStdout is required: the marker block comes back on stdout NDJSON. + const markerEngine = MARKER_ENGINES[provider as "cursor" | "opencode"]; + if (markerEngine) { + const model = typeof config?.model === "string" && config.model ? config.model : undefined; + // Per-job nonce embedded in the marker contract; recovered from job.prompt + // at parse time so echoed/quoted bare tags can't be mistaken for the payload. + const nonce = makeMarkerNonce(); + const prompt = composeMarkerReviewPrompt(reviewProfile, userMessage, nonce); + const { command } = buildMarkerCommand(markerEngine, prompt, model, cwd); + return { command, prompt, cwd, label: jobLabel, captureStdout: true, model, prUrl: launchPrUrl, diffScope: launchDiffScope, diffContext, reviewProfileId: reviewProfile.id, reviewProfileLabel: reviewProfile.label }; + } + return null; }, @@ -612,7 +639,7 @@ export async function startReviewServer(options: { // Map findings onto annotations and ingest. Shared by both engine branches; // no-ops on an empty set so a clean (zero-finding) review stays "done". const ingest = (transformed: readonly T[], logTag: string) => { - if (transformed.length === 0) return; + if (transformed.length === 0) return undefined; const annotations = transformed.map((a) => ({ ...a, ...jobPrContext, @@ -621,6 +648,7 @@ export async function startReviewServer(options: { })); const result = externalAnnotations.addAnnotations({ annotations }); if ("error" in result) console.error(`[${logTag}] addAnnotations error:`, result.error); + return result; }; if (job.provider === "codex") { @@ -682,6 +710,56 @@ export async function startReviewServer(options: { return; } + // --- Marker path (Cursor, OpenCode) --- + // FAIL-CLOSED: marker output is prompt-enforced (no schema flag), so any + // missing/malformed/schema/transform/insertion failure must MUTATE the job + // to failed — NEVER throw (agent-jobs.ts swallows throws, silently leaving + // an exit-0 job marked done). Mirrors the Tour fail-closed pattern below. + // Findings carry nullable file/line, classified into line/whole-file/ + // general by transformMarkerFindings — nothing is dropped (same as Claude). + const markerEngine = MARKER_ENGINES[job.provider as "cursor" | "opencode"]; + if (markerEngine) { + // Recover the per-job nonce embedded in the prompt; without it no block + // can be trusted, so parse fails closed below. + const nonce = extractMarkerNonce(job.prompt ?? ""); + const output = nonce && meta.stdout ? parseMarkerStreamOutput(meta.stdout, markerEngine, nonce) : null; + if (!output) { + job.status = "failed"; + job.error = `${markerEngine.author} review output missing or unparseable (no valid marker JSON).`; + return; + } + + // Derive the verdict from finding severities (like Claude) rather than + // trusting the model's free-form `correctness` string. Marker engines + // have no schema flag, so a model value like "not correct" would be + // stored verbatim and the detail panel (any string containing "correct" + // except "incorrect" → green) would invert the displayed result. + const hasImportant = output.findings.some((f) => f.severity === "important"); + job.summary = { + correctness: hasImportant ? "Issues Found" : "Correct", + explanation: output.summary.explanation, + confidence: output.summary.confidence, + }; + + // Reuse the shared ingest() decoration; add a fail-closed check on result. + const result = ingest( + transformMarkerFindings( + output.findings, + job.source, + markerEngine.author, + cwd, + workspace ? (filePath) => workspace.normalizeAnnotationPath(filePath) : undefined, + ), + `${markerEngine.id}-review`, + ); + if (result && "error" in result) { + job.status = "failed"; + job.error = `${markerEngine.author} annotation insertion failed: ${result.error}`; + return; + } + return; + } + if (job.provider === "tour") { const { summary } = await tour.onJobComplete({ job, meta }); if (summary) { diff --git a/apps/pi-extension/vendor.sh b/apps/pi-extension/vendor.sh index c6f71aef2..a78493a03 100755 --- a/apps/pi-extension/vendor.sh +++ b/apps/pi-extension/vendor.sh @@ -13,7 +13,7 @@ for f in feedback-templates prompts review-core diff-paths cli-pagination jj-cor done # Vendor review agent modules from packages/server/ — rewrite imports for generated/ layout -for f in agent-review-message codex-review claude-review path-utils review-skill-loader; do +for f in agent-review-message codex-review claude-review review-findings marker-review path-utils review-skill-loader; do src="../../packages/server/$f.ts" printf '// @generated — DO NOT EDIT. Source: packages/server/%s.ts\n' "$f" | cat - "$src" \ | sed 's|from "./vcs"|from "./review-core.js"|' \ diff --git a/packages/review-editor/dock/panels/ReviewAgentJobDetailPanel.tsx b/packages/review-editor/dock/panels/ReviewAgentJobDetailPanel.tsx index cafd28d37..a0cdc547e 100644 --- a/packages/review-editor/dock/panels/ReviewAgentJobDetailPanel.tsx +++ b/packages/review-editor/dock/panels/ReviewAgentJobDetailPanel.tsx @@ -352,7 +352,7 @@ function ProviderPill({ provider, engine, model }: { provider: string; engine?: const engineLabel = engine === 'codex' ? 'Codex' : 'Claude'; label = model && engine !== 'codex' ? `Tour · ${engineLabel} ${model.charAt(0).toUpperCase() + model.slice(1)}` : `Tour · ${engineLabel}`; } else { - label = provider === 'claude' ? 'Claude' : provider === 'codex' ? 'Codex' : 'Shell'; + label = provider === 'claude' ? 'Claude' : provider === 'codex' ? 'Codex' : provider === 'cursor' ? 'Cursor' : provider === 'opencode' ? 'OpenCode' : 'Shell'; } return ( = new Set([ + "claude", + "codex", + "tour", + "cursor", + "opencode", +]); + // --------------------------------------------------------------------------- // Factory // --------------------------------------------------------------------------- @@ -99,6 +115,32 @@ export interface AgentJobHandlerOptions { onJobComplete?: (job: AgentJobInfo, meta: { outputPath?: string; stdout?: string; cwd?: string }) => void | Promise; } + +/** + * Best-effort model catalog for a marker engine, spawned once. The spawn lives + * HERE (per-runtime — Bun.spawn) rather than in marker-review.ts, which must stay + * Bun-free for the Pi vendor build. ASYNC so it never blocks the event loop on + * the /capabilities request path (a slow/hanging CLI would otherwise freeze every + * other in-flight request for up to the timeout). Empty when discovery fails or + * the CLI is unauthenticated / has no providers configured — the UI falls back to + * the engine's default picker. Account/config-specific, so never hardcoded. + */ +async function discoverMarkerModels(engine: MarkerEngine): Promise { + try { + const proc = Bun.spawn([engine.binary, ...engine.modelsArgv], { + stdout: "pipe", + stderr: "ignore", + timeout: 5000, + }); + const stdout = await new Response(proc.stdout).text(); + const exitCode = await proc.exited; + if (exitCode !== 0) return []; + return engine.parseModels(stdout); + } catch { + return []; + } +} + export function createAgentJobHandler(options: AgentJobHandlerOptions): AgentJobHandler { const { mode, getServerUrl, getCwd } = options; @@ -109,17 +151,39 @@ export function createAgentJobHandler(options: AgentJobHandlerOptions): AgentJob const encoder = new TextEncoder(); let version = 0; - // --- Capability detection (run once) --- + // --- Capability detection: binaries are probed once at construction; marker + // model catalogs are discovered LAZILY (see buildCapabilitiesResponse) so a + // slow/unauthenticated ` models` spawn never blocks review-server + // startup — it runs at most once, on the first /capabilities request. --- const capabilities: AgentCapability[] = [ { id: "claude", name: "Claude Code", available: !!Bun.which("claude") }, { id: "codex", name: "Codex CLI", available: !!Bun.which("codex") }, { id: "tour", name: "Code Tour", available: !!Bun.which("claude") || !!Bun.which("codex") }, ]; - const capabilitiesResponse: AgentCapabilities = { - mode, - providers: capabilities, - available: capabilities.some((c) => c.available), - }; + // Marker engines (Cursor, OpenCode) — same shape, one loop. Available only in + // review mode when the binary is on PATH (NOTE: cursor's binary is `agent`). + for (const engine of Object.values(MARKER_ENGINES)) { + capabilities.push({ + id: engine.id, + name: engine.name, + available: mode === "review" && !!Bun.which(engine.binary), + }); + } + + const markerModelsCache = new Map(); + async function buildCapabilitiesResponse(): Promise { + const providers = await Promise.all(capabilities.map(async (c) => { + const engine = MARKER_ENGINES[c.id as "cursor" | "opencode"]; + if (!engine || !c.available) return c; + let models = markerModelsCache.get(engine.id); + if (!models) { + models = await discoverMarkerModels(engine); + markerModelsCache.set(engine.id, models); + } + return { ...c, models }; + })); + return { mode, providers, available: providers.some((p) => p.available) }; + } // --- SSE broadcasting --- function broadcast(event: AgentJobEvent): void { @@ -241,32 +305,46 @@ export function createAgentJobHandler(options: AgentJobHandlerOptions): AgentJob let stdoutBuf = ""; const stdoutDone = (captureStdout && proc.stdout && typeof proc.stdout !== "number") ? (async () => { + // Format one complete JSONL line into a live-log delta (skip result + // events — those are handled in onJobComplete). + const emitLogLine = (line: string) => { + if (!line.trim()) return; + // Claude: format JSONL into readable text. Tour jobs with the + // Claude engine also stream Claude JSONL, so key off engine too. + if (provider === "claude" || spawnOptions?.engine === "claude") { + const formatted = formatClaudeLogEvent(line); + if (formatted !== null) broadcast({ type: "job:log", jobId: id, delta: formatted + '\n' }); + return; + } + // Marker engines (Cursor, OpenCode): map their NDJSON stream events + // into readable log deltas via the engine's own formatter (Cursor + // applies the partial-output dedup rule; OpenCode reads text parts). + const markerEngine = MARKER_ENGINES[provider as "cursor" | "opencode"]; + if (markerEngine) { + const formatted = formatMarkerLogEvent(line, markerEngine); + if (formatted !== null) broadcast({ type: "job:log", jobId: id, delta: formatted + '\n' }); + return; + } + try { + const event = JSON.parse(line); + if (event.type === 'result') return; + } catch { /* not JSON — forward as raw log */ } + broadcast({ type: "job:log", jobId: id, delta: line + '\n' }); + }; try { const reader = proc!.stdout as unknown as AsyncIterable; + // stream-json output is NDJSON and chunk boundaries are arbitrary — + // carry the trailing partial line until a later chunk completes it, + // otherwise records split across chunks are dropped from live logs. + let logLineCarry = ""; for await (const chunk of reader) { const text = typeof chunk === "string" ? chunk : new TextDecoder().decode(chunk); stdoutBuf += text; - - // Forward JSONL lines as log events (skip result events) - const lines = text.split('\n'); - for (const line of lines) { - if (!line.trim()) continue; - // Claude: format JSONL into readable text. Tour jobs with the - // Claude engine also stream Claude JSONL, so key off engine too. - if (provider === "claude" || spawnOptions?.engine === "claude") { - const formatted = formatClaudeLogEvent(line); - if (formatted !== null) { - broadcast({ type: "job:log", jobId: id, delta: formatted + '\n' }); - } - continue; - } - try { - const event = JSON.parse(line); - if (event.type === 'result') continue; // handled in onJobComplete - } catch { /* not JSON — forward as raw log */ } - broadcast({ type: "job:log", jobId: id, delta: line + '\n' }); - } + const lines = (logLineCarry + text).split('\n'); + logLineCarry = lines.pop() ?? ""; + for (const line of lines) emitLogLine(line); } + if (logLineCarry) emitLogLine(logLineCarry); } catch { // Stream closed } @@ -299,8 +377,17 @@ export function createAgentJobHandler(options: AgentJobHandlerOptions): AgentJob stdout: captureStdout ? stdoutBuf : undefined, cwd: jobCwd, }); - } catch { - // Result ingestion failure shouldn't prevent job completion broadcast + } catch (err) { + // Claude/Codex are fail-open: an ingestion error is logged but does + // not change the terminal state. Cursor and OpenCode are fail-closed + // — their findings are prompt-enforced, so an unexpected throw here + // must surface as a failed job rather than a green one. (Their + // handlers normally fail by mutation and never throw; this guards + // future refactors.) + if (MARKER_ENGINES[provider as "cursor" | "opencode"]) { + entry.info.status = "failed"; + entry.info.error = err instanceof Error ? err.message : `${provider} result ingestion failed`; + } } } jobOutputPaths.delete(id); @@ -370,7 +457,7 @@ export function createAgentJobHandler(options: AgentJobHandlerOptions): AgentJob ): Promise { // --- GET /api/agents/capabilities --- if (url.pathname === CAPABILITIES && req.method === "GET") { - return Response.json(capabilitiesResponse); + return Response.json(await buildCapabilitiesResponse()); } // --- SSE stream --- @@ -467,6 +554,22 @@ export function createAgentJobHandler(options: AgentJobHandlerOptions): AgentJob ); } + // Fail-closed enforcement for server-owned providers: the command MUST + // be built server-side. Client-supplied argv is never spawned for these + // providers — a null/throwing builder becomes an error, not a fallback. + const isServerBuiltProvider = SERVER_BUILT_PROVIDERS.has(provider); + if (isServerBuiltProvider) { + if (!options.buildCommand) { + return Response.json( + { error: `Provider ${provider} requires server-built command` }, + { status: 400 }, + ); + } + // Discard any client-supplied argv so a null build cleanly hits the + // `command.length === 0` guard below instead of falling through. + command = []; + } + // Try server-side command building for known providers let captureStdout = false; let stdinPrompt: string | undefined; diff --git a/packages/server/claude-review.ts b/packages/server/claude-review.ts index a996e043f..28fe57aef 100644 --- a/packages/server/claude-review.ts +++ b/packages/server/claude-review.ts @@ -1,9 +1,13 @@ -import { toRelativePath } from "./path-utils"; import { composeReviewPrompt, type ResolvedReviewProfile, } from "@plannotator/shared/review-profiles"; -import { classifyFindingPlacement } from "@plannotator/shared/external-annotation"; +import { + transformSeverityFindings, + type ReviewSeverity, + type ReviewFinding, + type ReviewAnnotationInput, +} from "./review-findings"; /** * Claude Code Review Agent — prompt, command builder, and JSONL output parser. @@ -21,16 +25,10 @@ import { classifyFindingPlacement } from "@plannotator/shared/external-annotatio // Types // --------------------------------------------------------------------------- -export type ClaudeSeverity = "important" | "nit" | "pre_existing"; - -export interface ClaudeFinding { - severity: ClaudeSeverity; - file?: string | null; // null for a general (review-level) comment - line?: number | null; // null for a whole-file or general comment - end_line?: number | null; - description: string; - reasoning: string; -} +// Claude findings ARE review findings — reuse the one shared shape from +// review-findings.ts rather than keeping a byte-identical copy that can drift. +export type ClaudeSeverity = ReviewSeverity; +export type ClaudeFinding = ReviewFinding; export interface ClaudeReviewOutput { findings: ClaudeFinding[]; @@ -313,42 +311,10 @@ export function transformClaudeFindings( source: string, cwd?: string, pathTransform?: (path: string) => string, -): Array<{ - source: string; - filePath: string; - lineStart: number; - lineEnd: number; - type: string; - side: string; - scope: string; - text: string; - severity: ClaudeSeverity; - reasoning: string; - author: string; -}> { - // Route every finding by what it carries — nothing is dropped. A finding - // with no usable file becomes a general comment; with a file but no line, a - // whole-file comment; otherwise a line comment. - return findings.map(f => { - const rawFile = typeof f.file === "string" ? f.file : ""; - const filePath = rawFile - ? (pathTransform ? pathTransform(toRelativePath(rawFile, cwd)) : toRelativePath(rawFile, cwd)) - : ""; - const placement = classifyFindingPlacement(filePath, f.line, f.end_line); - return { - source, - filePath: placement.filePath, - lineStart: placement.lineStart, - lineEnd: placement.lineEnd, - type: "comment", - side: "new", - scope: placement.scope, - text: `[${f.severity}] ${f.description}`, - severity: f.severity, - reasoning: f.reasoning, - author: "Claude Code", - }; - }); +): ReviewAnnotationInput[] { + // Routing (line / whole-file / general) is shared with the marker engines in + // review-findings.ts — nothing is dropped; only the author differs. + return transformSeverityFindings(findings, source, "Claude Code", cwd, pathTransform); } // --------------------------------------------------------------------------- diff --git a/packages/server/marker-review.test.ts b/packages/server/marker-review.test.ts new file mode 100644 index 000000000..f46e120d7 --- /dev/null +++ b/packages/server/marker-review.test.ts @@ -0,0 +1,443 @@ +import { describe, expect, test } from "bun:test"; +import { + markerOpen, + markerClose, + makeMarkerNonce, + extractMarkerNonce, + buildMarkerOutputContract, + MARKER_ENGINES, + buildMarkerCommand, + reduceMarkerStream, + parseMarkerStreamOutput, + formatMarkerLogEvent, + transformMarkerFindings, + validateMarkerReviewOutput, + composeMarkerReviewPrompt, + type MarkerEngine, + type MarkerFinding, + type MarkerReviewOutput, +} from "./marker-review"; +import type { ResolvedReviewProfile } from "@plannotator/shared/review-profiles"; + +const cursor = MARKER_ENGINES.cursor; +const opencode = MARKER_ENGINES.opencode; + +// A fixed, valid nonce (pn + 12 hex) for deterministic tests. +const NONCE = "pn0123456789ab"; +const OPEN = markerOpen(NONCE); +const CLOSE = markerClose(NONCE); + +// --------------------------------------------------------------------------- +// NDJSON builders, mirroring each engine's real stream shape. +// --------------------------------------------------------------------------- + +function ndjson(obj: unknown): string { + return JSON.stringify(obj) + "\n"; +} + +/** Cursor: a real partial-output assistant delta (timestamp_ms, no model_call_id). */ +function cursorDelta(text: string, ts = 1): string { + return ndjson({ type: "assistant", timestamp_ms: ts, message: { content: text } }); +} +/** Cursor: a duplicate flush (model_call_id present) — must be skipped. */ +function cursorDuplicate(text: string, ts = 1): string { + return ndjson({ type: "assistant", timestamp_ms: ts, model_call_id: "call_1", message: { content: text } }); +} +/** OpenCode: a finalized text part (the only events carrying assistant text). */ +function opencodeText(text: string): string { + return ndjson({ type: "text", timestamp: 1, sessionID: "s1", part: { type: "text", text, time: { start: 1, end: 2 } } }); +} + +function markerBlock(payload: unknown): string { + return `${OPEN}\n${JSON.stringify(payload, null, 2)}\n${CLOSE}`; +} + +function chunkify(s: string, parts: number): string[] { + const size = Math.ceil(s.length / parts); + const out: string[] = []; + for (let i = 0; i < s.length; i += size) out.push(s.slice(i, i + size)); + return out; +} + +const validReview: MarkerReviewOutput = { + findings: [ + { + file: "packages/server/review.ts", + line: 123, + end_line: 130, + severity: "important", + description: "Null deref on the unhappy path.", + reasoning: "The guard above only covers the happy path.", + }, + ], + summary: { correctness: "Issues Found", explanation: "One important issue.", confidence: 0.85 }, +}; + +// =========================================================================== +// buildArgv — both descriptors +// =========================================================================== + +describe("buildMarkerCommand: cursor", () => { + test("uses the `agent` binary, read-only flags, prompt as trailing argv", () => { + const { command } = buildMarkerCommand(cursor, "review this", "gpt-5", "/repo"); + expect(command[0]).toBe("agent"); + expect(command).toContain("-p"); + expect(command[command.indexOf("--mode") + 1]).toBe("ask"); + expect(command[command.indexOf("--sandbox") + 1]).toBe("enabled"); + expect(command).not.toContain("--force"); + expect(command).not.toContain("--yolo"); + expect(command).toContain("--trust"); + expect(command[command.indexOf("--output-format") + 1]).toBe("stream-json"); + expect(command).toContain("--stream-partial-output"); + expect(command[command.indexOf("--workspace") + 1]).toBe("/repo"); + expect(command[command.indexOf("--model") + 1]).toBe("gpt-5"); + expect(command[command.length - 1]).toBe("review this"); + }); + + test("omits --model for Auto/empty/undefined; --workspace when no cwd", () => { + expect(buildMarkerCommand(cursor, "p", "Auto", "/repo").command).not.toContain("--model"); + expect(buildMarkerCommand(cursor, "p", "", "/repo").command).not.toContain("--model"); + expect(buildMarkerCommand(cursor, "p", undefined, "/repo").command).not.toContain("--model"); + expect(buildMarkerCommand(cursor, "p").command).not.toContain("--workspace"); + }); +}); + +describe("buildMarkerCommand: opencode", () => { + test("uses `opencode run --format json --agent plan`, prompt trailing", () => { + const { command } = buildMarkerCommand(opencode, "review this", "opencode/glm-5.1", "/repo"); + expect(command[0]).toBe("opencode"); + expect(command[1]).toBe("run"); + expect(command[command.indexOf("--format") + 1]).toBe("json"); + expect(command[command.indexOf("--agent") + 1]).toBe("plan"); + expect(command[command.indexOf("--dir") + 1]).toBe("/repo"); + expect(command[command.indexOf("--model") + 1]).toBe("opencode/glm-5.1"); + expect(command).not.toContain("--dangerously-skip-permissions"); + expect(command[command.length - 1]).toBe("review this"); + }); + + test("omits --model when empty; --dir when no cwd", () => { + expect(buildMarkerCommand(opencode, "p", "", "/repo").command).not.toContain("--model"); + expect(buildMarkerCommand(opencode, "p").command).not.toContain("--dir"); + }); +}); + +// =========================================================================== +// parseModels — both descriptors +// =========================================================================== + +describe("parseModels: cursor", () => { + test("parses `id - Label` lines, skips header/tip, dedupes", () => { + const out = [ + "Available models", + "", + "auto - Auto", + "gpt-5.2 - GPT-5.2", + "claude-opus-4-8-thinking-high - Opus 4.8 1M Thinking", + "auto - Auto Again", + "Tip: use --model to switch.", + ].join("\n"); + expect(cursor.parseModels(out)).toEqual([ + { id: "auto", label: "Auto" }, + { id: "gpt-5.2", label: "GPT-5.2" }, + { id: "claude-opus-4-8-thinking-high", label: "Opus 4.8 1M Thinking" }, + ]); + }); + + test("returns [] for unauthenticated / empty output", () => { + expect(cursor.parseModels("No models available for this account.")).toEqual([]); + expect(cursor.parseModels("")).toEqual([]); + }); +}); + +describe("parseModels: opencode", () => { + test("parses provider/model lines (incl. nested slashes), dedupes, skips junk", () => { + const out = ["opencode/big-pickle", "openrouter/deepseek/deepseek-chat-v3", "opencode/big-pickle", "", "Some header line"].join("\n"); + expect(opencode.parseModels(out)).toEqual([ + { id: "opencode/big-pickle", label: "opencode/big-pickle" }, + { id: "openrouter/deepseek/deepseek-chat-v3", label: "openrouter/deepseek/deepseek-chat-v3" }, + ]); + }); + + test("returns [] for empty / no provider lines", () => { + expect(opencode.parseModels("")).toEqual([]); + expect(opencode.parseModels("No models configured")).toEqual([]); + }); +}); + +// =========================================================================== +// reduceMarkerStream / extractText — both descriptors +// =========================================================================== + +describe("reduceMarkerStream: cursor", () => { + test("reconstructs canonical text regardless of chunk boundaries", () => { + const stdout = + ndjson({ type: "system", subtype: "init", model: "Auto" }) + + cursorDelta("Looking at ") + + cursorDelta("the diff.") + + ndjson({ type: "result", result: " Done." }); + + expect(reduceMarkerStream(stdout, cursor).canonicalText).toBe("Looking at the diff. Done."); + for (const parts of [2, 3, 7, stdout.length]) { + const rejoined = chunkify(stdout, parts).join(""); + expect(reduceMarkerStream(rejoined, cursor).canonicalText).toBe("Looking at the diff. Done."); + } + }); + + test("filters duplicate flushes (model_call_id present), skips malformed", () => { + const stdout = cursorDelta("real ") + cursorDuplicate("DUP") + "not json\n" + cursorDelta("text"); + const { canonicalText, recordCount } = reduceMarkerStream(stdout, cursor); + expect(canonicalText).toBe("real text"); + expect(canonicalText).not.toContain("DUP"); + expect(recordCount).toBe(3); // malformed line not counted + }); +}); + +describe("reduceMarkerStream: opencode", () => { + test("concatenates type:text part.text, ignores tool/step events", () => { + const stdout = + opencodeText("Looking at ") + + ndjson({ type: "tool_use", sessionID: "s1", part: { type: "tool", tool: "read", state: { status: "completed" } } }) + + opencodeText("the diff.") + + ndjson({ type: "step_finish", sessionID: "s1", part: { type: "step-finish" } }); + expect(reduceMarkerStream(stdout, opencode).canonicalText).toBe("Looking at the diff."); + }); + + test("survives chunk-joined buffer and malformed lines", () => { + const stdout = opencodeText("a") + "not json\n" + opencodeText("b"); + expect(reduceMarkerStream(stdout, opencode).canonicalText).toBe("ab"); + }); +}); + +// =========================================================================== +// parseMarkerStreamOutput — NDJSON split, last block wins, failure → null +// =========================================================================== + +describe("parseMarkerStreamOutput: last block wins, chunk-safe", () => { + test("cursor: extracts the LAST complete marker block across messages", () => { + const stale = { findings: [], summary: { correctness: "Correct", explanation: "draft", confidence: 0.1 } }; + const stdout = + cursorDelta("Draft:\n" + markerBlock(stale) + "\n") + + cursorDelta("Final:\n" + markerBlock(validReview)); + const out = parseMarkerStreamOutput(stdout, cursor, NONCE); + expect(out!.summary.correctness).toBe("Issues Found"); + expect(out!.findings).toHaveLength(1); + }); + + test("opencode: a single block split across text parts + raw chunks parses", () => { + const full = "Commentary first.\n" + markerBlock(validReview); + const half = Math.floor(full.length / 2); + const stdout = opencodeText(full.slice(0, half)) + opencodeText(full.slice(half)); + const out = parseMarkerStreamOutput(chunkify(stdout, 9).join(""), opencode, NONCE); + expect(out!.findings).toHaveLength(1); + }); +}); + +describe("parseMarkerStreamOutput: failure modes → null", () => { + test("cursor: missing / unclosed / malformed-JSON / schema-invalid / empty", () => { + expect(parseMarkerStreamOutput(cursorDelta("no marker here"), cursor, NONCE)).toBeNull(); + expect(parseMarkerStreamOutput(cursorDelta(`${OPEN}\n{"findings":[]`), cursor, NONCE)).toBeNull(); + expect(parseMarkerStreamOutput(cursorDelta(`${OPEN}\n{ bad json }\n${CLOSE}`), cursor, NONCE)).toBeNull(); + const badSeverity = { findings: [{ file: "x.ts", line: 1, severity: "blocker", description: "d", reasoning: "" }], summary: { correctness: "x", explanation: "y", confidence: 1 } }; + expect(parseMarkerStreamOutput(cursorDelta(markerBlock(badSeverity)), cursor, NONCE)).toBeNull(); + expect(parseMarkerStreamOutput("", cursor, NONCE)).toBeNull(); + expect(parseMarkerStreamOutput(" \n ", cursor, NONCE)).toBeNull(); + }); + + test("cursor: a complete block followed by a truncated block fails closed (no stale fallback)", () => { + // Model emitted a draft block, then started a replacement that got cut off. + // We must NOT silently return the earlier block. + const stdout = cursorDelta( + "Draft:\n" + markerBlock(validReview) + "\nFinal:\n" + `${OPEN}\n{"findings":[`, + ); + expect(parseMarkerStreamOutput(stdout, cursor, NONCE)).toBeNull(); + }); + + test("cursor: bare static sentinels in prose don't corrupt extraction (real-run regression)", () => { + // Reproduces the live failure: reviewing this module, Composer's prose quoted + // the bare `` tag (no nonce) BEFORE its real payload. + // The static parser latched onto that mention; the nonce makes it inert. + const bareOpen = ""; + const bareClose = ""; + const stdout = cursorDelta( + `Findings are extracted from a ${bareOpen} marker block; see ${bareClose}.\n` + + "Here is my review.\n" + + markerBlock(validReview) + + "\nTo run the full panel, switch to Agent mode.", + ); + const out = parseMarkerStreamOutput(stdout, cursor, NONCE); + expect(out!.summary.correctness).toBe("Issues Found"); + expect(out!.findings).toHaveLength(1); + }); + + test("nonce: makeMarkerNonce round-trips through a composed prompt; wrong/absent nonce fails closed", () => { + const nonce = makeMarkerNonce(); + expect(nonce).toMatch(/^pn[0-9a-f]{12}$/); + const prompt = composeMarkerReviewPrompt(undefined, "review the diff", nonce); + expect(extractMarkerNonce(prompt)).toBe(nonce); + expect(extractMarkerNonce("no marker here")).toBeNull(); + // A payload tagged with the real nonce parses only under that nonce. + const stdout = cursorDelta(`${markerOpen(nonce)}\n${JSON.stringify(validReview)}\n${markerClose(nonce)}`); + expect(parseMarkerStreamOutput(stdout, cursor, nonce)!.findings).toHaveLength(1); + expect(parseMarkerStreamOutput(stdout, cursor, "pnffffffffffff")).toBeNull(); + expect(parseMarkerStreamOutput(stdout, cursor, "")).toBeNull(); + }); + + test("opencode: missing summary field → null; valid empty findings accepted", () => { + const missingSummary = { findings: [], summary: { correctness: "x", explanation: "y" } }; + expect(parseMarkerStreamOutput(opencodeText(markerBlock(missingSummary)), opencode, NONCE)).toBeNull(); + const clean = { findings: [], summary: { correctness: "Correct", explanation: "ok", confidence: 1 } }; + const out = parseMarkerStreamOutput(opencodeText(markerBlock(clean)), opencode, NONCE); + expect(out!.findings).toEqual([]); + }); +}); + +// =========================================================================== +// validateMarkerReviewOutput — nullable placement schema (mirrors Claude) +// =========================================================================== + +describe("validateMarkerReviewOutput: nullable file/line/end_line", () => { + test("accepts a line finding, a whole-file finding (line null), and a general note (file+line null)", () => { + const out = validateMarkerReviewOutput({ + findings: [ + { file: "a.ts", line: 5, end_line: 5, severity: "important", description: "line issue", reasoning: "r" }, + { file: "a.ts", line: null, end_line: null, severity: "nit", description: "whole-file", reasoning: "r" }, + { file: null, line: null, end_line: null, severity: "nit", description: "general note", reasoning: "r" }, + // Absent placement fields are equivalent to null. + { severity: "nit", description: "general via omission", reasoning: "r" }, + ], + summary: { correctness: "Issues Found", explanation: "e", confidence: 0.5 }, + }); + expect(out).not.toBeNull(); + expect(out!.findings).toHaveLength(4); + expect(out!.findings[1].line).toBeNull(); + expect(out!.findings[2].file).toBeNull(); + expect(out!.findings[3].file).toBeNull(); + expect(out!.findings[3].line).toBeNull(); + }); + + test("rejects non-object, missing findings array, bad severity, wrong placement type", () => { + expect(validateMarkerReviewOutput(null)).toBeNull(); + expect(validateMarkerReviewOutput({ summary: { correctness: "c", explanation: "e", confidence: 1 } })).toBeNull(); + expect(validateMarkerReviewOutput({ findings: [{ severity: "high", description: "d" }], summary: { correctness: "c", explanation: "e", confidence: 1 } })).toBeNull(); + // line must be number|null — a string is invalid. + expect(validateMarkerReviewOutput({ findings: [{ file: "a", line: "x", severity: "important", description: "d" }], summary: { correctness: "c", explanation: "e", confidence: 1 } })).toBeNull(); + }); + + test("defaults reasoning to empty string", () => { + const out = validateMarkerReviewOutput({ + findings: [{ file: "a.ts", line: 5, severity: "nit", description: "d" }], + summary: { correctness: "Issues Found", explanation: "e", confidence: 0.5 }, + }); + expect(out!.findings[0].reasoning).toBe(""); + }); +}); + +// =========================================================================== +// formatMarkerLogEvent — both descriptors +// =========================================================================== + +describe("formatMarkerLogEvent", () => { + test("cursor: init / delta / tool_call / result; skips dup, flush, malformed", () => { + expect(formatMarkerLogEvent(ndjson({ type: "system", subtype: "init", model: "Auto", session_id: "s1" }).trim(), cursor)).toContain("model=Auto"); + expect(formatMarkerLogEvent(cursorDelta("hello").trim(), cursor)).toBe("hello"); + expect(formatMarkerLogEvent(ndjson({ type: "tool_call", subtype: "completed", name: "read_file" }).trim(), cursor)).toBe("[read_file] completed"); + expect(formatMarkerLogEvent(ndjson({ type: "result", duration_ms: 42 }).trim(), cursor)).toContain("42ms"); + expect(formatMarkerLogEvent(cursorDuplicate("dup").trim(), cursor)).toBeNull(); + expect(formatMarkerLogEvent(ndjson({ type: "assistant", message: { content: "FULL FLUSH" } }).trim(), cursor)).toBeNull(); + expect(formatMarkerLogEvent("not json", cursor)).toBeNull(); + }); + + test("opencode: text / tool_use / error; skips step, malformed", () => { + expect(formatMarkerLogEvent(opencodeText("hello").trim(), opencode)).toBe("hello"); + expect(formatMarkerLogEvent(ndjson({ type: "tool_use", part: { type: "tool", tool: "read", state: { status: "completed" } } }).trim(), opencode)).toBe("[read] completed"); + expect(formatMarkerLogEvent(ndjson({ type: "error", error: {} }).trim(), opencode)).toContain("[error]"); + expect(formatMarkerLogEvent(ndjson({ type: "step_start", part: { type: "step-start" } }).trim(), opencode)).toBeNull(); + expect(formatMarkerLogEvent("not json", opencode)).toBeNull(); + }); +}); + +// =========================================================================== +// transformMarkerFindings — classifyFindingPlacement; nothing dropped +// =========================================================================== + +describe("transformMarkerFindings: line/file/general, nothing dropped", () => { + const findings: MarkerFinding[] = [ + { file: "/repo/packages/server/review.ts", line: 10, end_line: 12, severity: "important", description: "line bug", reasoning: "r" }, + { file: "/repo/packages/server/review.ts", line: null, end_line: null, severity: "nit", description: "whole-file note", reasoning: "r" }, + { file: null, line: null, end_line: null, severity: "nit", description: "general note", reasoning: "r" }, + ]; + + test("cursor: produces line/file/general placements with author Cursor", () => { + const out = transformMarkerFindings(findings, "agent-x", cursor.author, "/repo"); + expect(out).toHaveLength(3); // nothing dropped + + const [line, file, general] = out; + expect(line.scope).toBe("line"); + expect(line.filePath).toBe("packages/server/review.ts"); + expect(line.lineStart).toBe(10); + expect(line.lineEnd).toBe(12); + expect(line.author).toBe("Cursor"); + expect(line.text).toBe("[important] line bug"); + + expect(file.scope).toBe("file"); + expect(file.filePath).toBe("packages/server/review.ts"); + expect(file.lineStart).toBe(0); + + expect(general.scope).toBe("general"); + expect(general.filePath).toBe(""); + }); + + test("opencode: same placements with author OpenCode; path transform applied", () => { + const out = transformMarkerFindings(findings, "src", opencode.author, "/repo", (p) => p ? `child/${p}` : p); + expect(out[0].author).toBe("OpenCode"); + expect(out[0].filePath).toBe("child/packages/server/review.ts"); + // general note has no file → transform receives "" and placement stays general + expect(out[2].scope).toBe("general"); + }); + + test("defaults lineEnd to lineStart when end_line absent on a line finding", () => { + const f: MarkerFinding[] = [{ file: "/repo/a.ts", line: 7, severity: "nit", description: "d", reasoning: "" }]; + expect(transformMarkerFindings(f, "src", cursor.author, "/repo")[0].lineEnd).toBe(7); + }); +}); + +// =========================================================================== +// composeMarkerReviewPrompt — contract ALWAYS appended (default AND custom) +// =========================================================================== + +describe("composeMarkerReviewPrompt", () => { + const userMessage = "Review the diff for branch X."; + + test("default profile: methodology + contract + user message", () => { + const prompt = composeMarkerReviewPrompt(undefined, userMessage, NONCE); + expect(prompt).toContain("# Code Review"); + expect(prompt).toContain(buildMarkerOutputContract(NONCE)); + expect(prompt).toContain(OPEN); + expect(prompt.endsWith(userMessage)).toBe(true); + }); + + test("builtin:default profile behaves like no profile (contract present)", () => { + const builtin: ResolvedReviewProfile = { id: "builtin:default", label: "Default", instructions: "", source: "builtin", default: true }; + const prompt = composeMarkerReviewPrompt(builtin, userMessage, NONCE); + expect(prompt).toContain("# Code Review"); + expect(prompt).toContain(buildMarkerOutputContract(NONCE)); + }); + + test("custom profile REPLACES methodology but KEEPS the output contract", () => { + const custom: ResolvedReviewProfile = { + id: "user:security", + label: "Security", + instructions: "ONLY look for SQL injection and auth bypasses.", + source: "user", + }; + const prompt = composeMarkerReviewPrompt(custom, userMessage, NONCE); + expect(prompt).toContain("ONLY look for SQL injection and auth bypasses."); + // Custom instructions replace the default methodology... + expect(prompt).not.toContain("You are a senior engineer reviewing a code change."); + // ...but the marker contract is still appended — it's the only thing that + // makes marker output parseable. + expect(prompt).toContain(buildMarkerOutputContract(NONCE)); + expect(prompt).toContain(OPEN); + expect(prompt.endsWith(userMessage)).toBe(true); + }); +}); diff --git a/packages/server/marker-review.ts b/packages/server/marker-review.ts new file mode 100644 index 000000000..90bb6068f --- /dev/null +++ b/packages/server/marker-review.ts @@ -0,0 +1,823 @@ +import { + profileHasCustomSection, + type ResolvedReviewProfile, +} from "@plannotator/shared/review-profiles"; +import { + transformSeverityFindings, + type ReviewSeverity, + type ReviewFinding, + type ReviewAnnotationInput, +} from "./review-findings"; + +/** + * Marker Review Engines — the shared machinery for review CLIs that expose NO + * schema-validation flag, so their final review output is prose. Cursor (the + * `agent` binary) and OpenCode (`opencode run`) are the two. Because they can't + * be told to emit validated structured output, they are instead told to emit a + * marker-delimited JSON block, and we extract the LAST complete block from the + * reconstructed canonical text. + * + * Everything else — the finding model (nullable file/line/end_line, classified + * into line/whole-file/general by classifyFindingPlacement), custom review + * profiles, and the transform into external annotations — is IDENTICAL to + * Claude/Codex. The ONLY per-engine differences are captured in the + * `MarkerEngine` descriptor: the binary, how to build argv, how to pull text + * out of one stream event, how to discover models, and how to format a live log + * line. There is exactly ONE copy of every shared piece below. + * + * PI-SAFETY: This file is vendored into the Pi (Node) build via vendor.sh, so it + * MUST contain ZERO Bun.* / Bun-only APIs. The model-discovery SPAWN lives in + * each runtime's agent-jobs adapter; this module only provides parseModels + + * buildArgv + modelsArgv (pure data/functions). + */ + +// --------------------------------------------------------------------------- +// Per-job marker — the JSON payload is delimited by tags carrying a random, +// per-job nonce: . +// +// Why the nonce: in review mode the diff (and the model's own prose) is +// untrusted text that routinely contains the bare string "plannotator-review-json" +// — e.g. when reviewing this very module, or when the model echoes the contract +// example. A static tag let those echoed mentions be mistaken for the real +// delimiter, so extraction grabbed prose instead of the payload and valid +// reviews failed. The nonce is generated at prompt-build time, embedded in the +// output contract the model copies, and recovered from the stored prompt at +// parse time, so only the model's genuine payload can match. +// --------------------------------------------------------------------------- + +const MARKER_TAG = "plannotator-review-json"; + +/** Open delimiter for a job's nonce. */ +export function markerOpen(nonce: string): string { + return `<${MARKER_TAG}:${nonce}>`; +} +/** Close delimiter for a job's nonce. */ +export function markerClose(nonce: string): string { + return ``; +} + +/** Generate an unguessable per-job marker nonce (matches html-assets token style). */ +export function makeMarkerNonce(): string { + return "pn" + crypto.randomUUID().replace(/-/g, "").slice(0, 12); +} + +/** + * Recover the nonce embedded in a composed prompt's marker contract. Returns + * null when absent — the parser then fails closed, since no payload can be + * trusted without the expected delimiter. + */ +export function extractMarkerNonce(prompt: string): string | null { + const m = new RegExp(`<${MARKER_TAG}:(pn[0-9a-f]{12})>`).exec(prompt); + return m ? m[1] : null; +} + +// --------------------------------------------------------------------------- +// Finding model — IDENTICAL to ClaudeFinding: nullable file/line/end_line so a +// finding can be a line issue (file + line), a whole-file issue (file, line +// null), or a general review-level note (both null). Nothing is dropped. +// --------------------------------------------------------------------------- + +// Marker findings ARE review findings — reuse the one shared shape from +// review-findings.ts (this module already imports its transform) rather than +// keeping a byte-identical copy that could silently diverge. +export type MarkerSeverity = ReviewSeverity; +export type MarkerFinding = ReviewFinding; + +export interface MarkerReviewSummary { + correctness: string; + explanation: string; + confidence: number; +} + +export interface MarkerReviewOutput { + findings: MarkerFinding[]; + summary: MarkerReviewSummary; +} + +const VALID_SEVERITIES: ReadonlySet = new Set([ + "important", + "nit", + "pre_existing", +]); + +// --------------------------------------------------------------------------- +// Schema validator — hand-rolled (no Ajv): marker output is prompt-enforced, so +// validation is the floor that turns prose back into a trusted object. Mirrors +// the claude-review.ts schema, which uses type [string,null] for file/line/ +// end_line: a missing or null placement is VALID (whole-file / general note). +// Empty findings are valid as long as the JSON is valid and the summary is. +// --------------------------------------------------------------------------- + +/** + * Read a line coordinate that may be a finite integer, null, or absent. The + * marker block is prompt-enforced, so a fractional/NaN/Infinity line is garbage + * we will not trust as an annotation coordinate — it is rejected here. + */ +function optionalNullableInteger(value: unknown): { ok: true; value: number | null } | { ok: false } { + if (value === undefined || value === null) return { ok: true, value: null }; + if (typeof value === "number" && Number.isInteger(value)) return { ok: true, value }; + return { ok: false }; +} + +/** Read a value that may be a string, null, or absent — anything else is invalid. */ +function optionalNullableString(value: unknown): { ok: true; value: string | null } | { ok: false } { + if (value === undefined || value === null) return { ok: true, value: null }; + if (typeof value === "string") return { ok: true, value }; + return { ok: false }; +} + +/** + * Validate a parsed object against the marker review schema. Returns the typed + * output, or null if the shape is invalid. file/line/end_line are nullable — + * absent or null is accepted (whole-file or general findings). + */ +export function validateMarkerReviewOutput(parsed: unknown): MarkerReviewOutput | null { + if (!parsed || typeof parsed !== "object") return null; + const obj = parsed as Record; + + if (!Array.isArray(obj.findings)) return null; + + const summary = obj.summary; + if (!summary || typeof summary !== "object") return null; + const s = summary as Record; + if (typeof s.correctness !== "string") return null; + if (typeof s.explanation !== "string") return null; + if (typeof s.confidence !== "number" || !Number.isFinite(s.confidence)) return null; + + const findings: MarkerFinding[] = []; + for (const raw of obj.findings) { + if (!raw || typeof raw !== "object") return null; + const f = raw as Record; + + if (typeof f.severity !== "string" || !VALID_SEVERITIES.has(f.severity)) return null; + if (typeof f.description !== "string") return null; + + const file = optionalNullableString(f.file); + if (!file.ok) return null; + const line = optionalNullableInteger(f.line); + if (!line.ok) return null; + const endLine = optionalNullableInteger(f.end_line); + if (!endLine.ok) return null; + + findings.push({ + severity: f.severity as MarkerSeverity, + file: file.value, + line: line.value, + end_line: endLine.value, + description: f.description, + reasoning: typeof f.reasoning === "string" ? f.reasoning : "", + }); + } + + return { + findings, + summary: { + correctness: s.correctness, + explanation: s.explanation, + // Clamp to [0,1] — the model occasionally reports out-of-range confidence. + confidence: Math.max(0, Math.min(1, s.confidence)), + }, + }; +} + +// --------------------------------------------------------------------------- +// MarkerEngine descriptor — the ONLY per-engine surface. Six fields. +// --------------------------------------------------------------------------- + +export interface MarkerModel { + id: string; + label: string; +} + +/** One parsed stream event, opaque to the shared reducer (each engine reads it). */ +export type MarkerStreamEvent = Record; + +export interface MarkerEngine { + /** Stable engine id — also the provider id used by the server. */ + id: "cursor" | "opencode"; + /** Display name for the capabilities/provider listing (e.g. "Cursor CLI"). */ + name: string; + /** The CLI binary to spawn (NOTE: cursor's binary is `agent`). */ + binary: string; + /** Author string stamped on every annotation this engine produces. */ + author: string; + /** Build the full argv (binary + flags + trailing prompt) for a review run. */ + buildArgv: (prompt: string, model?: string, cwd?: string) => string[]; + /** Pull readable text out of one parsed stream event, or null if none. */ + extractText: (event: MarkerStreamEvent) => string | null; + /** Argv (after the binary) for model discovery, e.g. ["models"]. */ + modelsArgv: string[]; + /** Parse the model-discovery stdout into a catalog. */ + parseModels: (stdout: string) => MarkerModel[]; + /** Format one stream line for the live log, or null to skip it. */ + formatLogEvent: (event: MarkerStreamEvent) => string | null; +} + +// --------------------------------------------------------------------------- +// Cursor engine helpers +// --------------------------------------------------------------------------- + +/** + * A Cursor partial-output assistant event is a real new text delta only when + * `timestamp_ms` is present AND `model_call_id` is absent. Every other assistant + * flush is a duplicate re-emission (pre-tool-call flush or end-of-turn flush). + */ +function cursorIsRealAssistantDelta(event: MarkerStreamEvent): boolean { + return event.timestamp_ms !== undefined && event.model_call_id === undefined; +} + +/** Pull readable text out of a Cursor assistant event's content (string or parts). */ +function cursorAssistantText(event: MarkerStreamEvent): string { + if (typeof event.text === "string") return event.text; + const message = event.message as { content?: unknown } | undefined; + const content = message?.content; + if (typeof content === "string") return content; + if (Array.isArray(content)) { + return content + .filter((p): p is { type?: string; text?: string } => !!p && typeof p === "object") + .filter((p) => p.type === "text" && typeof p.text === "string") + .map((p) => p.text as string) + .join(""); + } + return ""; +} + +/** + * Cursor `extractText` for the canonical-text reducer: append a real new delta + * (timestamp_ms present, model_call_id absent). The no-timestamp branch KEEPS + * the end-of-turn flush — it covers both partial-streaming-disabled output (the + * full message arrives once) and the enabled-mode final flush, and is a + * parse-robustness safety net (extractLastMarkerBlock takes the LAST block, so a + * duplicate is harmless). result events carry the final text too. + * + * This is deliberately MORE lenient than cursorFormatLogEvent, which drops the + * flush so live logs don't repeat the whole assistant output. Do not unify them. + */ +function cursorExtractText(event: MarkerStreamEvent): string | null { + if (event.type === "assistant") { + if (event.timestamp_ms !== undefined) { + if (cursorIsRealAssistantDelta(event)) return cursorAssistantText(event); + return null; + } + if (event.model_call_id === undefined) return cursorAssistantText(event); + return null; + } + if (event.type === "result") { + if (typeof event.result === "string") return event.result; + if (typeof event.text === "string") return event.text; + return null; + } + return null; +} + +/** + * Parse `agent models` / `agent --list-models` output into a model catalog. The + * CLI prints one model per line as ` -