diff --git a/README.md b/README.md index 70cf697..1907900 100644 --- a/README.md +++ b/README.md @@ -2,7 +2,7 @@ LLM-driven pull-request reviewer. Posts inline findings, reconciles prior bot threads on follow-up pushes, and writes a summary verdict. -- **VCS**: GitHub, GitLab, Bitbucket (pluggable; `mock://` for tests) +- **VCS**: GitHub, GitLab, Bitbucket (pluggable; `local://` for console-only branch review, `mock://` for tests) - **LLM**: any OpenAI-compatible `/v1` endpoint (Anthropic, OpenAI, vLLM, Ollama, OpenRouter, Groq, …) - **Runtime**: ships as a self-contained Docker image — consumers don't need Node or pnpm @@ -36,15 +36,17 @@ The workflow builds the reviewer image from source on each run and triggers on e For Node projects, consume the reviewer as a `devDependency` instead of building the image — no Docker required. Works with npm, yarn, or pnpm. -Add it to the consumer project, pinned to a tag (the git form is the same for every package manager): +Add it to the consumer project from the npm registry: ```json "devDependencies": { - "@reviewer/cli": "github:bilikaz/code-reviewer#v0.1.1" + "@bilikaz/code-reviewer": "^0.2.0" } ``` -The package ships prebuilt JavaScript — `dist/` is committed — so the install does no build step (Node refuses to run TypeScript from inside `node_modules`, so compiled output is shipped instead). Then copy [examples/review.yml](examples/review.yml) into `/.github/workflows/` and set the same `LLM_URL` / `LLM_KEY` / `LLM_MODEL` secrets as above. If this reviewer repo is **private**, the consumer's install step needs a PAT — see the comments in that file. +(A git-tag form also works for unpublished revisions — `"@bilikaz/code-reviewer": "github:bilikaz/code-reviewer#"` — because `dist/` is committed: prebuilt JavaScript, no install-time build. Node refuses to run TypeScript from inside `node_modules`, so compiled output is shipped instead. A private repo consumed this way needs a PAT in the install step.) + +Then copy [examples/review.yml](examples/review.yml) into `/.github/workflows/` and set the same `LLM_URL` / `LLM_KEY` / `LLM_MODEL` secrets as above. Run it locally the same way: @@ -106,12 +108,13 @@ All config is env-only and read once at startup. See [.env.example](.env.example | Variable | Required | Default | Notes | | --- | --- | --- | --- | | `LLM_URL` | yes | — | OpenAI-compatible base URL | -| `LLM_KEY` | yes | — | empty allowed for self-hosted endpoints with no auth | +| `LLM_KEY` | no | — | omit for self-hosted endpoints with no auth (Authorization header is skipped) | | `LLM_MODEL` | no | auto-detect from `/models` | | | `GITHUB_TOKEN` / `GITLAB_TOKEN` / `BITBUCKET_TOKEN` | one | — | the one matching the PR URL | | `LLM_TEMPERATURE` | no | `0.2` | | | `LLM_MAX_OUTPUT_TOKENS` | no | `32768` | | | `LLM_HEAL_RETRIES` | no | `2` | retries when LLM output fails schema validation | +| `LLM_DEBUG` | no | off | `1`/`true` dumps the full request body per LLM call (very chatty) | ## Development @@ -138,11 +141,17 @@ src/ reconcile/ re-judge prior bot threads review/ LLM review + inline findings summarize/ final verdict + summary comment - llm/ OpenAI-compatible streaming client, tool loop, schema healing - vcs/ github / gitlab / bitbucket / mock providers - lib/ git diff parsing - logger/ structured JSON logging + llm/ client (callLLM + resolveModel, schema healing), transport (HTTP + SSE), tools (Read/Bash) + providers/ Provider port + github / gitlab / bitbucket / local / mock adapters + lib/ host-level helpers (git diffs, error normalization) + logger/ Logger port + console / in-memory sinks tests/ e2e/ vitest end-to-end tests fixtures/ mock old/new repo pairs per scenario +docs/ + ARCHITECTURE.md the map: structure, patterns, flows + conventions/ portable engineering rules (naming, types placement, …) + adr/ dated architecture decision records ``` + +Documentation is layered: [docs/ARCHITECTURE.md](docs/ARCHITECTURE.md) is this repo's map, [docs/conventions/](docs/conventions/) holds the portable rule set (reusable across projects), and [docs/adr/](docs/adr/) is the decision log that adopts them. diff --git a/dist/cli.js b/dist/cli.js index 95024e3..8100aaf 100644 --- a/dist/cli.js +++ b/dist/cli.js @@ -20,10 +20,12 @@ // --llm-key overrides LLM_KEY // --llm-model overrides LLM_MODEL (otherwise auto-detected) import { buildCtx } from "./ctx.js"; +import { errorMessage } from "./lib/errors.js"; import { runFetch } from "./stages/fetch/index.js"; import { runReconcile } from "./stages/reconcile/index.js"; import { runReview } from "./stages/review/index.js"; import { runSummarize } from "./stages/summarize/index.js"; +import { isOpenBotThread } from "./providers/types.js"; const FLAG_ALIASES = { "--pr": "pr", "--llm-url": "llmUrl", @@ -69,7 +71,7 @@ function buildOverrides(flags) { } // ---- Pipeline orchestration -------------------------------------------- function hasUnresolvedBotThreads(state) { - return state.comments.some((c) => c.by === "bot" && c.inline && !c.resolved && !c.parentId); + return state.comments.some(isOpenBotThread); } async function runCommand(command, ctx) { switch (command) { @@ -113,6 +115,6 @@ try { process.exit(1); } catch (e) { - ctx.logger.error("cli.fatal", { error: e.message }); + ctx.logger.error("cli.fatal", { error: errorMessage(e) }); process.exit(1); } diff --git a/dist/ctx.js b/dist/ctx.js index 187112d..dd44b3d 100644 --- a/dist/ctx.js +++ b/dist/ctx.js @@ -1,7 +1,7 @@ // Ctx — dependency-injection container. Built once at CLI entry, passed to // every stage. Holds: // - config: immutable settings (all env reads + CLI overrides live here) -// - vcs: instantiated provider (already authenticated) +// - provider: instantiated review-platform adapter (already authenticated) // - logger: structured logger (swap sinks here) // // Single rule: process.env is read ONLY in loadConfig(). Everything @@ -10,15 +10,17 @@ // .env loading. import { existsSync } from "node:fs"; import { ConsoleLogger } from "./logger/console.js"; -import { resolveModel } from "./llm/model.js"; -import { BitbucketProvider } from "./vcs/bitbucket.js"; -import { GitHubProvider } from "./vcs/github.js"; -import { GitLabProvider } from "./vcs/gitlab.js"; -import { LocalVcsProvider } from "./vcs/local.js"; -import { MockVcsProvider } from "./vcs/mock.js"; -import { detectProvider } from "./vcs/types.js"; +import { resolveModel } from "./llm/client.js"; +import { BitbucketProvider } from "./providers/bitbucket.js"; +import { GitHubProvider } from "./providers/github.js"; +import { GitLabProvider } from "./providers/gitlab.js"; +import { LocalProvider } from "./providers/local.js"; +import { MockProvider } from "./providers/mock.js"; +import { detectProvider } from "./providers/types.js"; // ---- Defaults ------------------------------------------------------------ -const REVIEW_DEFAULTS = { +// Exported so tests build their configs from the real defaults instead of +// re-typing them (see docs/conventions/configuration.md). +export const REVIEW_DEFAULTS = { diffFilter: { includeExtensions: [], fullFileThresholdLines: 800, @@ -46,6 +48,9 @@ function asNumber(v, fallback) { const n = Number(v); return Number.isFinite(n) ? n : fallback; } +function asBool(v) { + return v === "1" || (v ?? "").toLowerCase() === "true"; +} function requireField(value, name, hint) { if (!value) throw new Error(`missing ${name}. ${hint}`); @@ -77,7 +82,7 @@ export function loadConfig(opts) { // models will simply finish well under the cap. maxOutputTokens: asNumber(e.LLM_MAX_OUTPUT_TOKENS, 32_768), healRetries: asNumber(e.LLM_HEAL_RETRIES, 2), - debug: pick(e.LLM_DEBUG) === "1" || pick(e.LLM_DEBUG).toLowerCase() === "true", + debug: asBool(e.LLM_DEBUG), }, github: { token: pick(e.GITHUB_TOKEN, e.GH_TOKEN) }, gitlab: { token: pick(e.GITLAB_TOKEN), baseUrl: pick(e.GITLAB_BASE_URL) }, @@ -115,20 +120,20 @@ export function loadConfig(opts) { return config; } // ---- VCS construction (plain dispatch — no env reads) ------------------- -function makeVcs(c) { +function makeProvider(c) { switch (c.pr.provider) { case "github": return GitHubProvider.create(c); case "gitlab": return GitLabProvider.create(c); case "bitbucket": return BitbucketProvider.create(c); - case "mock": return MockVcsProvider.create(c); - case "local": return LocalVcsProvider.create(c); + case "mock": return MockProvider.create(c); + case "local": return LocalProvider.create(c); } } // ---- buildCtx (thin orchestrator) --------------------------------------- export async function buildCtx(opts) { const config = loadConfig(opts); - const [vcs] = await Promise.all([ - makeVcs(config), + const [provider] = await Promise.all([ + makeProvider(config), (async () => { if (!config.llm.model) { config.llm.model = await resolveModel({ @@ -140,7 +145,7 @@ export async function buildCtx(opts) { ]); return { config, - vcs, + provider, logger: opts.logger ?? new ConsoleLogger(), }; } diff --git a/dist/lib/errors.js b/dist/lib/errors.js new file mode 100644 index 0000000..c501121 --- /dev/null +++ b/dist/lib/errors.js @@ -0,0 +1,16 @@ +// Error-message normalization (see docs/conventions/error-handling.md). `catch (e)` binds unknown; +// `(e as Error).message` lies when a non-Error was thrown (yields +// undefined and the log line loses its cause). Every catch site that +// needs the message goes through this helper instead. +export function errorMessage(e) { + if (e instanceof Error) + return e.message; + if (typeof e === "string") + return e; + try { + return JSON.stringify(e); + } + catch { + return String(e); + } +} diff --git a/dist/llm/call.js b/dist/llm/call.js deleted file mode 100644 index ecd21d0..0000000 --- a/dist/llm/call.js +++ /dev/null @@ -1,185 +0,0 @@ -// One LLM "call" per stage. Internally a loop: stream a chat completion → -// if it returned tool_calls, execute them locally and feed the results back, -// then re-stream. When the LLM returns final content (no tool_calls), parse -// the JSON, validate against the schema, and either return or heal. -import { Ajv } from "ajv"; -import { postChat } from "./client.js"; -import { readStream } from "./stream.js"; -import { execTool, TOOL_SCHEMAS } from "./tools/index.js"; -export class LLMValidationFailed extends Error { -} -function serializeSections(sections) { - const parts = []; - for (const { tag, content } of sections) { - const body = content.trimEnd(); - if (!tag) - parts.push(body); - else - parts.push(`<${tag}>\n${body}\n`); - } - return parts.join("\n\n"); -} -const JSON_FENCE_RE = /```(?:json)?\s*\n?([\s\S]*?)```/g; -function extractJson(text) { - const candidates = []; - for (const m of text.matchAll(JSON_FENCE_RE)) - candidates.push(m[1].trim()); - candidates.push(text.trim()); - let parseError = ""; - for (const c of candidates) { - try { - return { value: JSON.parse(c), clean: true }; - } - catch (e) { - if (!parseError) - parseError = e.message; - } - } - // Longest-span scan: shrink suffixes until JSON.parse succeeds. This only - // runs when no whole candidate parsed — i.e. the JSON is malformed — so a - // hit here is a salvaged fragment, not the intended object. - let best = null; - for (const opener of ["{", "["]) { - let i = text.indexOf(opener); - while (i >= 0) { - for (let end = text.length; end > i + 1; end--) { - try { - const v = JSON.parse(text.slice(i, end)); - const span = end - i; - if (!best || span > best.span) - best = { span, value: v }; - break; - } - catch { /* shrink */ } - } - i = text.indexOf(opener, i + 1); - } - } - if (best) - return { value: best.value, clean: false }; - throw new Error(`response is not valid JSON: ${parseError}`); -} -function healPrompt(error) { - // Tailor the instruction to the actual failure. A bad-JSON error and a - // schema error need different fixes — telling a model with malformed JSON to - // "fix the schema" sends it down the wrong path. - const jsonError = error.startsWith("invalid JSON") || error.startsWith("JSON parse"); - const fix = jsonError - ? "The cause is malformed JSON — almost always an unescaped double quote, or a raw newline, " + - "inside a string value. Re-emit the SAME findings, but escape every double quote inside a " + - "string as \\\" and every newline as \\n so the whole thing is valid JSON." - : "Your output was valid JSON but did not match the required schema. Re-emit the SAME findings " + - "as an object with exactly the schema's required properties and no extra ones."; - return [ - "Your previous message (directly above) was rejected — it did not produce usable output.", - "", - ` Why it failed: ${error}`, - "", - fix, - "Output the corrected JSON only — no prose, no markdown fences.", - ].join("\n"); -} -export async function callLLM(args) { - const { ctx } = args; - const log = ctx.logger.child(args.stage); - // validateSchema: false skips draft-07 meta-schema verification. We ship - // the schemas; we don't need ajv to verify their grammar at runtime. - const ajv = new Ajv({ allErrors: true, strict: false, validateSchema: false }); - const validate = ajv.compile(args.schema); - const system = serializeSections(args.systemSections); - const user = serializeSections(args.userSections); - const messages = [ - { role: "system", content: system }, - { role: "user", content: user }, - ]; - const { baseUrl, apiKey, model, healRetries, temperature, maxOutputTokens } = ctx.config.llm; - const useTools = args.tools !== false; - let healAttempts = 0; - let toolCalls = 0; - let lastError = null; - let lastText = ""; - let totalPrompt = 0, totalCompletion = 0, totalAll = 0; - for (let attempt = 0; attempt <= healRetries; attempt++) { - let finalText = ""; - while (true) { - const promptBytes = messages.reduce((n, m) => n + (typeof m.content === "string" ? m.content.length : 0), 0); - log.info("llm.request", { model, attempt, prompt_bytes: promptBytes, messages: messages.length }); - const req = { - model, - messages, - tools: useTools ? TOOL_SCHEMAS : undefined, - tool_choice: useTools ? "auto" : undefined, - temperature: Number.isFinite(temperature) ? temperature : 0.2, - max_tokens: maxOutputTokens, - stream: true, - stream_options: { include_usage: true }, - }; - // LLM_DEBUG=1 dumps the full JSON body (no URL, no headers, no - // credentials). Lets the operator verify what the LLM actually sees. - if (ctx.config.llm.debug) - log.debug("llm.request.body", { body: req }); - const resp = await postChat({ baseUrl, apiKey, req }); - const msg = await readStream(resp, { onText: (s) => ctx.logger.stream(s) }); - if (msg.usage) { - totalPrompt += msg.usage.prompt_tokens; - totalCompletion += msg.usage.completion_tokens; - totalAll += msg.usage.total_tokens; - } - if (msg.tool_calls.length > 0) { - messages.push({ role: "assistant", content: msg.content || null, tool_calls: msg.tool_calls }); - for (const tc of msg.tool_calls) { - toolCalls++; - log.info("tool.call", { name: tc.function.name, args_preview: tc.function.arguments.slice(0, 200) }); - const result = await execTool(tc); - messages.push({ role: "tool", content: result.output, tool_call_id: tc.id }); - } - continue; - } - finalText = msg.content; - lastText = finalText; - break; - } - let extracted = null; - try { - extracted = extractJson(finalText); - } - catch (e) { - lastError = `JSON parse: ${e.message}`; - } - if (extracted) { - if (validate(extracted.value)) { - log.info("llm.done", { - prompt_tokens: totalPrompt, - completion_tokens: totalCompletion, - heal_attempts: healAttempts, - tool_calls: toolCalls, - }); - return { - validated: extracted.value, - usage: { promptTokens: totalPrompt, completionTokens: totalCompletion, totalTokens: totalAll }, - healAttempts, - toolCalls, - rawText: lastText, - }; - } - // A clean parse that fails the schema is a genuine structure problem. - // But if we only salvaged a fragment (clean=false), the JSON itself was - // malformed — the schema error is misleading (it's validating a recovered - // sub-object). Heal on the real cause: invalid JSON, usually an unescaped - // quote or newline inside a string value. - lastError = extracted.clean - ? `schema: ${ajv.errorsText(validate.errors, { separator: "; " })}` - : `invalid JSON: your output did not parse as a whole — only a fragment was recoverable. ` + - `This is almost always an unescaped " or a raw newline inside a string value. ` + - `Escape every double quote as \\" and every newline as \\n.`; - } - if (attempt < healRetries) { - healAttempts++; - log.warn("llm.heal", { attempt: healAttempts, reason: lastError ?? "unknown" }); - messages.push({ role: "assistant", content: finalText }); - messages.push({ role: "user", content: healPrompt(lastError ?? "unknown") }); - } - } - log.error("llm.exhausted", { heal_retries: healRetries, last_error: lastError }); - throw new LLMValidationFailed(`stage=${args.stage} exhausted ${healRetries} heal attempts; last: ${lastError}`); -} diff --git a/dist/llm/client.js b/dist/llm/client.js index 7d0aea0..2cec12d 100644 --- a/dist/llm/client.js +++ b/dist/llm/client.js @@ -1,20 +1,213 @@ -// Minimal LLM transport. POSTs to an OpenAI-compatible /chat/completions -// endpoint, returns the raw Response. SSE parsing lives in stream.ts. -export async function postChat(args) { - const headers = { - "Content-Type": "application/json", - "Accept": args.req.stream ? "text/event-stream" : "application/json", - }; +// The LLM client — everything consumers call. Two entry points: +// +// callLLM() — one "call" per stage. Internally a loop: stream a chat +// completion → if it returned tool_calls, execute them +// locally and feed the results back, then re-stream. When +// the LLM returns final content (no tool_calls), parse the +// JSON, validate against the schema, and either return or +// heal. +// resolveModel() — model auto-detection via GET /models (used by buildCtx +// when no model is configured). +// +// Wire-level plumbing lives below this file in transport.ts (message types, +// HTTP POST, SSE parsing). +import { Ajv } from "ajv"; +import { errorMessage } from "../lib/errors.js"; +import { receive, send } from "./transport.js"; +import { execTool, TOOL_SCHEMAS } from "./tools/index.js"; +export class LLMValidationError extends Error { +} +function serializeSections(sections) { + const parts = []; + for (const { tag, content } of sections) { + const body = content.trimEnd(); + if (!tag) + parts.push(body); + else + parts.push(`<${tag}>\n${body}\n`); + } + return parts.join("\n\n"); +} +const JSON_FENCE_RE = /```(?:json)?\s*\n?([\s\S]*?)```/g; +function extractJson(text) { + const candidates = []; + for (const m of text.matchAll(JSON_FENCE_RE)) + candidates.push(m[1].trim()); + candidates.push(text.trim()); + let parseError = ""; + for (const c of candidates) { + try { + return { value: JSON.parse(c), clean: true }; + } + catch (e) { + if (!parseError) + parseError = errorMessage(e); + } + } + // Longest-span scan: shrink suffixes until JSON.parse succeeds. This only + // runs when no whole candidate parsed — i.e. the JSON is malformed — so a + // hit here is a salvaged fragment, not the intended object. + let best = null; + for (const opener of ["{", "["]) { + let i = text.indexOf(opener); + while (i >= 0) { + for (let end = text.length; end > i + 1; end--) { + try { + const v = JSON.parse(text.slice(i, end)); + const span = end - i; + if (!best || span > best.span) + best = { span, value: v }; + break; + } + catch { /* shrink */ } + } + i = text.indexOf(opener, i + 1); + } + } + if (best) + return { value: best.value, clean: false }; + throw new Error(`response is not valid JSON: ${parseError}`); +} +function healPrompt(error) { + // Tailor the instruction to the actual failure. A bad-JSON error and a + // schema error need different fixes — telling a model with malformed JSON to + // "fix the schema" sends it down the wrong path. + const jsonError = error.startsWith("invalid JSON") || error.startsWith("JSON parse"); + const fix = jsonError + ? "The cause is malformed JSON — almost always an unescaped double quote, or a raw newline, " + + "inside a string value. Re-emit the SAME findings, but escape every double quote inside a " + + "string as \\\" and every newline as \\n so the whole thing is valid JSON." + : "Your output was valid JSON but did not match the required schema. Re-emit the SAME findings " + + "as an object with exactly the schema's required properties and no extra ones."; + return [ + "Your previous message (directly above) was rejected — it did not produce usable output.", + "", + ` Why it failed: ${error}`, + "", + fix, + "Output the corrected JSON only — no prose, no markdown fences.", + ].join("\n"); +} +export async function callLLM(args) { + const { ctx } = args; + const log = ctx.logger.child(args.stage); + // validateSchema: false skips draft-07 meta-schema verification. We ship + // the schemas; we don't need ajv to verify their grammar at runtime. + const ajv = new Ajv({ allErrors: true, strict: false, validateSchema: false }); + const validate = ajv.compile(args.schema); + const system = serializeSections(args.systemSections); + const user = serializeSections(args.userSections); + const messages = [ + { role: "system", content: system }, + { role: "user", content: user }, + ]; + const { baseUrl, apiKey, model, healRetries, temperature, maxOutputTokens } = ctx.config.llm; + const useTools = args.tools !== false; + let healAttempts = 0; + let toolCalls = 0; + let lastError = null; + let lastText = ""; + let totalPrompt = 0, totalCompletion = 0, totalAll = 0; + for (let attempt = 0; attempt <= healRetries; attempt++) { + let finalText = ""; + while (true) { + const promptBytes = messages.reduce((n, m) => n + (typeof m.content === "string" ? m.content.length : 0), 0); + log.info("llm.request", { model, attempt, prompt_bytes: promptBytes, messages: messages.length }); + const req = { + model, + messages, + tools: useTools ? TOOL_SCHEMAS : undefined, + tool_choice: useTools ? "auto" : undefined, + temperature: Number.isFinite(temperature) ? temperature : 0.2, + max_tokens: maxOutputTokens, + stream: true, + stream_options: { include_usage: true }, + }; + // LLM_DEBUG=1 dumps the full JSON body (no URL, no headers, no + // credentials). Lets the operator verify what the LLM actually sees. + if (ctx.config.llm.debug) + log.debug("llm.request.body", { body: req }); + const resp = await send({ baseUrl, apiKey, req }); + const msg = await receive(resp, { onText: (s) => ctx.logger.stream(s) }); + if (msg.usage) { + totalPrompt += msg.usage.prompt_tokens; + totalCompletion += msg.usage.completion_tokens; + totalAll += msg.usage.total_tokens; + } + if (msg.tool_calls.length > 0) { + messages.push({ role: "assistant", content: msg.content || null, tool_calls: msg.tool_calls }); + for (const tc of msg.tool_calls) { + toolCalls++; + log.info("tool.call", { name: tc.function.name, args_preview: tc.function.arguments.slice(0, 200) }); + const result = await execTool(tc); + messages.push({ role: "tool", content: result.output, tool_call_id: tc.id }); + } + continue; + } + finalText = msg.content; + lastText = finalText; + break; + } + let extracted = null; + try { + extracted = extractJson(finalText); + } + catch (e) { + lastError = `JSON parse: ${errorMessage(e)}`; + } + if (extracted) { + if (validate(extracted.value)) { + log.info("llm.done", { + prompt_tokens: totalPrompt, + completion_tokens: totalCompletion, + heal_attempts: healAttempts, + tool_calls: toolCalls, + }); + return { + validated: extracted.value, + usage: { promptTokens: totalPrompt, completionTokens: totalCompletion, totalTokens: totalAll }, + healAttempts, + toolCalls, + rawText: lastText, + }; + } + // A clean parse that fails the schema is a genuine structure problem. + // But if we only salvaged a fragment (clean=false), the JSON itself was + // malformed — the schema error is misleading (it's validating a recovered + // sub-object). Heal on the real cause: invalid JSON, usually an unescaped + // quote or newline inside a string value. + lastError = extracted.clean + ? `schema: ${ajv.errorsText(validate.errors, { separator: "; " })}` + : `invalid JSON: your output did not parse as a whole — only a fragment was recoverable. ` + + `This is almost always an unescaped " or a raw newline inside a string value. ` + + `Escape every double quote as \\" and every newline as \\n.`; + } + if (attempt < healRetries) { + healAttempts++; + log.warn("llm.heal", { attempt: healAttempts, reason: lastError ?? "unknown" }); + messages.push({ role: "assistant", content: finalText }); + messages.push({ role: "user", content: healPrompt(lastError ?? "unknown") }); + } + } + log.error("llm.exhausted", { heal_retries: healRetries, last_error: lastError }); + throw new LLMValidationError(`stage=${args.stage} exhausted ${healRetries} heal attempts; last: ${lastError}`); +} +export async function resolveModel(args) { + const url = `${args.baseUrl.replace(/\/$/, "")}/models`; + const headers = {}; if (args.apiKey) headers["Authorization"] = `Bearer ${args.apiKey}`; - const resp = await fetch(`${args.baseUrl}/chat/completions`, { - method: "POST", - headers, - body: JSON.stringify(args.req), - }); + const resp = await fetch(url, { headers }); if (!resp.ok) { const text = await resp.text().catch(() => ""); - throw new Error(`LLM HTTP ${resp.status}: ${text.slice(0, 1024)}`); + throw new Error(`model auto-detection failed: GET ${url} → HTTP ${resp.status}. ` + + `Pass --llm-model explicitly or set LLM_MODEL. ${text.slice(0, 200)}`); + } + const data = (await resp.json()); + const first = data.data?.[0]?.id; + if (!first) { + throw new Error(`model auto-detection failed: ${url} returned no models. ` + + `Pass --llm-model explicitly or set LLM_MODEL.`); } - return resp; + return first; } diff --git a/dist/llm/model.js b/dist/llm/model.js deleted file mode 100644 index 03ff663..0000000 --- a/dist/llm/model.js +++ /dev/null @@ -1,24 +0,0 @@ -// Resolves which model to use. Explicit override wins; otherwise we ask the -// LLM endpoint via `GET /models` and pick the first id. No hardcoded provider -// heuristics — if neither path yields a model, error out. -export async function resolveModel(args) { - if (args.override) - return args.override; - const url = `${args.baseUrl.replace(/\/$/, "")}/models`; - const headers = {}; - if (args.apiKey) - headers["Authorization"] = `Bearer ${args.apiKey}`; - const resp = await fetch(url, { headers }); - if (!resp.ok) { - const text = await resp.text().catch(() => ""); - throw new Error(`model auto-detection failed: GET ${url} → HTTP ${resp.status}. ` + - `Pass --llm-model explicitly or set LLM_MODEL. ${text.slice(0, 200)}`); - } - const data = (await resp.json()); - const first = data.data?.[0]?.id; - if (!first) { - throw new Error(`model auto-detection failed: ${url} returned no models. ` + - `Pass --llm-model explicitly or set LLM_MODEL.`); - } - return first; -} diff --git a/dist/llm/tools/bash.js b/dist/llm/tools/bash.js index 0fbb472..92e77d2 100644 --- a/dist/llm/tools/bash.js +++ b/dist/llm/tools/bash.js @@ -4,7 +4,7 @@ // paths or restrict commands — the LLM has whatever the container ships // with (git, grep, find, rg if installed, etc.). import { spawn } from "node:child_process"; -import { cap } from "./shared.js"; +import { cap } from "./types.js"; export const bashTool = { schema: { type: "function", diff --git a/dist/llm/tools/index.js b/dist/llm/tools/index.js index 70a9a89..c86c726 100644 --- a/dist/llm/tools/index.js +++ b/dist/llm/tools/index.js @@ -9,6 +9,7 @@ // // Add a new tool: create tools/.ts exporting `Tool: Tool`, then // add it to the TOOLS array below. +import { errorMessage } from "../../lib/errors.js"; import { bashTool } from "./bash.js"; import { readTool } from "./read.js"; const TOOLS = [readTool, bashTool]; @@ -35,7 +36,7 @@ export async function execTool(call) { `tool call rejected: arguments are not valid JSON.`, `Tool: ${name}`, `Received arguments: ${call.function.arguments}`, - `Parse error: ${e.message}`, + `Parse error: ${errorMessage(e)}`, `Retry with a valid JSON object matching the tool's schema.`, ].join("\n"), }; diff --git a/dist/llm/tools/read.js b/dist/llm/tools/read.js index 2ccc235..13fdf93 100644 --- a/dist/llm/tools/read.js +++ b/dist/llm/tools/read.js @@ -5,7 +5,8 @@ // with a ready-to-copy `sed` command for the next chunk. Smaller models // loop on big files when they can't see where to ask next. import { readFile } from "node:fs/promises"; -import { cap } from "./shared.js"; +import { errorMessage } from "../../lib/errors.js"; +import { cap } from "./types.js"; const MAX_LINES = 300; export const readTool = { schema: { @@ -46,7 +47,7 @@ export const readTool = { catch (e) { return { ok: false, - output: `error reading ${path}: ${e.message}. ` + + output: `error reading ${path}: ${errorMessage(e)}. ` + `Check the path (relative to the working directory) and try Bash ls/find if unsure.`, }; } diff --git a/dist/llm/tools/types.js b/dist/llm/tools/types.js new file mode 100644 index 0000000..e70a2f8 --- /dev/null +++ b/dist/llm/tools/types.js @@ -0,0 +1,7 @@ +// Shared types + helpers for LLM tools. +export const OUTPUT_CAP = 64 * 1024; // 64 KB per tool result is plenty; truncate beyond. +export function cap(s) { + if (s.length <= OUTPUT_CAP) + return s; + return s.slice(0, OUTPUT_CAP) + `\n\n[...output truncated; ${s.length - OUTPUT_CAP} more bytes dropped]`; +} diff --git a/dist/llm/stream.js b/dist/llm/transport.js similarity index 65% rename from dist/llm/stream.js rename to dist/llm/transport.js index 8d4d5c6..b71bf31 100644 --- a/dist/llm/stream.js +++ b/dist/llm/transport.js @@ -1,14 +1,36 @@ -// SSE parser for OpenAI-compatible /chat/completions streams. +// LLM transport — the wire level, both directions: +// - send(): POST to an OpenAI-compatible /chat/completions endpoint, +// returns the raw Response. +// - receive(): SSE parser for the streamed response. Each chunk is +// `data: {json}\n\n`, terminated by `data: [DONE]`. We +// accumulate `delta.content` (the visible text) and +// `delta.tool_calls` (function calls the LLM asks us to +// execute) into a final shape that looks like a +// non-streaming completion's `choices[0].message`. // -// Each chunk is `data: {json}\n\n`, terminated by `data: [DONE]`. We -// accumulate `delta.content` (the visible text) and `delta.tool_calls` -// (function calls the LLM is asking us to execute) into a final shape that -// looks like a non-streaming completion's `choices[0].message`. -// -// Side effect: while streaming, write `delta.content` to stderr live so the -// CI log shows the LLM's output as it's produced — the same UX the old -// Python harness gave us via stream-json + jq. -export async function readStream(resp, opts) { +// The two halves are one contract: receive parses exactly what send's +// `stream: true` request produces (incl. stream_options.include_usage → +// usage accumulation). Consumer API lives in client.ts; the wire shapes +// themselves live in types.ts. +export async function send(args) { + const headers = { + "Content-Type": "application/json", + "Accept": args.req.stream ? "text/event-stream" : "application/json", + }; + if (args.apiKey) + headers["Authorization"] = `Bearer ${args.apiKey}`; + const resp = await fetch(`${args.baseUrl}/chat/completions`, { + method: "POST", + headers, + body: JSON.stringify(args.req), + }); + if (!resp.ok) { + const text = await resp.text().catch(() => ""); + throw new Error(`LLM HTTP ${resp.status}: ${text.slice(0, 1024)}`); + } + return resp; +} +export async function receive(resp, opts) { if (!resp.body) throw new Error("LLM response has no body"); const reader = resp.body.getReader(); diff --git a/dist/llm/types.js b/dist/llm/types.js new file mode 100644 index 0000000..37f31d8 --- /dev/null +++ b/dist/llm/types.js @@ -0,0 +1,6 @@ +// Shared LLM contract — the vocabulary used across llm/ and by stages. +// +// Wire shapes mirror the OpenAI /chat/completions JSON (snake_case fields +// are not renamed); Section and LLMResult are the client API's input/output +// shapes. Behavior lives in client.ts (consumer API) and transport.ts (wire). +export {}; diff --git a/dist/logger/console.js b/dist/logger/console.js index 4415662..8912ab7 100644 --- a/dist/logger/console.js +++ b/dist/logger/console.js @@ -1,5 +1,6 @@ // Default sink: pretty indented output to stderr. Each event on its own // line, structured `data` rendered as indented JSON below it. +import { joinScope } from "./types.js"; const LEVEL_TAG = { debug: "DBG", info: "INF", @@ -39,6 +40,6 @@ export class ConsoleLogger { process.stderr.write(chunk); } child(scope) { - return new ConsoleLogger(this.scope ? `${this.scope}.${scope}` : scope); + return new ConsoleLogger(joinScope(this.scope, scope)); } } diff --git a/dist/logger/index.js b/dist/logger/index.js index 1aa76db..bff02d6 100644 --- a/dist/logger/index.js +++ b/dist/logger/index.js @@ -1,4 +1,3 @@ -// Logger interface + factory. Swap sinks by passing a different Logger -// instance into the Ctx; everything downstream uses the interface only. +// Public face of the logger folder: contract (types.ts) + sinks. export { ConsoleLogger } from "./console.js"; export { MemoryLogger } from "./memory.js"; diff --git a/dist/logger/memory.js b/dist/logger/memory.js index 4efdab5..9d735a4 100644 --- a/dist/logger/memory.js +++ b/dist/logger/memory.js @@ -1,5 +1,6 @@ // In-memory sink. Records every event for test assertions. `stream` chunks // are concatenated into a single field per scope. +import { joinScope } from "./types.js"; export class MemoryLogger { scope; entries; @@ -31,7 +32,6 @@ export class MemoryLogger { return this._streamRef.value; } child(scope) { - const next = this.scope ? `${this.scope}.${scope}` : scope; - return new MemoryLogger(next, this.entries, this._streamRef); + return new MemoryLogger(joinScope(this.scope, scope), this.entries, this._streamRef); } } diff --git a/dist/logger/types.js b/dist/logger/types.js new file mode 100644 index 0000000..fc9b93c --- /dev/null +++ b/dist/logger/types.js @@ -0,0 +1,8 @@ +// Logger contract. Swap sinks by passing a different Logger instance into +// the Ctx; everything downstream uses the interface only. +// Scope-join format for child(). One definition so every sink produces the +// same scope strings — tests and log readers depend on the dotted form +// (essential duplication, consolidated — see docs/conventions/consolidation.md). +export function joinScope(parent, child) { + return parent ? `${parent}.${child}` : child; +} diff --git a/dist/providers/base.js b/dist/providers/base.js new file mode 100644 index 0000000..c609331 --- /dev/null +++ b/dist/providers/base.js @@ -0,0 +1,35 @@ +// Shared plumbing for providers whose diff data comes from the local git +// checkout — which is all of them (ADR-0004). Subclasses inherit +// changed-files / per-file diffs / potential-rename scanning; `getRenames` +// defaults to git's confident (-M ≥50%) detection and is overridden by +// providers whose API exposes richer rename data. +import { gitChangedFiles, gitFileDiff, gitPotentialRenames } from "../lib/git.js"; +export class BaseProvider { + // Directory the git commands run in. undefined = process cwd (the real + // providers run against the checked-out consuming repo). The mock provider + // points this at its throwaway fixture repo. + gitCwd; + async getChangedFiles(meta) { + return gitChangedFiles({ baseSha: meta.baseSha, headSha: meta.headSha, cwd: this.gitCwd }); + } + async getPotentialRenames(meta) { + return gitPotentialRenames({ baseSha: meta.baseSha, headSha: meta.headSha, cwd: this.gitCwd }); + } + async getFileDiff(meta, path, opts) { + return gitFileDiff({ + baseSha: meta.baseSha, + headSha: meta.headSha, + path, + context: opts.context, + cwd: this.gitCwd, + }); + } + async getRenames(meta) { + const renames = {}; + for (const f of await this.getChangedFiles(meta)) { + if (f.status === "renamed" && f.oldPath) + renames[f.oldPath] = f.path; + } + return renames; + } +} diff --git a/dist/vcs/bitbucket.js b/dist/providers/bitbucket.js similarity index 77% rename from dist/vcs/bitbucket.js rename to dist/providers/bitbucket.js index 846f66b..b6c589d 100644 --- a/dist/vcs/bitbucket.js +++ b/dist/providers/bitbucket.js @@ -1,22 +1,41 @@ -// Bitbucket implementation of VcsProvider. REST API v2 via fetch. +// Bitbucket implementation of Provider. REST API v2 via fetch. // // Token needs: // - Cloud (bitbucket.org): App Password or OAuth token with `pullrequest:write`, `repository:read`. // - Server (self-hosted): PAT with `REPO_READ` + `PULL_REQUEST_WRITE`. // // baseUrl defaults to https://api.bitbucket.org/2.0; self-hosted instances pass their own. -import { gitChangedFiles, gitFileDiff, gitPotentialRenames } from "../lib/git.js"; -export class BitbucketProvider { - token; - baseUrl; +import { BaseProvider } from "./base.js"; +// Fetch wrapper owned by this provider — deliberately NOT shared with +// gitlab.ts: the two wrappers look alike today only by coincidence, and +// the APIs will diverge (pagination, rate limits, auth). See docs/conventions/consolidation.md. +function makeApi(baseUrl, token) { + return async (path, init) => { + const resp = await fetch(`${baseUrl}${path}`, { + ...init, + headers: { + Authorization: `Bearer ${token}`, + "Content-Type": "application/json", + ...(init?.headers ?? {}), + }, + }); + if (!resp.ok) { + const text = await resp.text().catch(() => ""); + throw new Error(`Bitbucket API ${resp.status}: ${text.slice(0, 1024)}`); + } + return resp.json(); + }; +} +export class BitbucketProvider extends BaseProvider { + api; owner; repo; number; botLogin; name = "bitbucket"; - constructor(token, baseUrl, owner, repo, number, botLogin) { - this.token = token; - this.baseUrl = baseUrl; + constructor(api, owner, repo, number, botLogin) { + super(); + this.api = api; this.owner = owner; this.repo = repo; this.number = number; @@ -27,36 +46,14 @@ export class BitbucketProvider { const { token, baseUrl } = config.bitbucket; if (!token) throw new Error("BitbucketProvider: token required"); - let owner, repo, num; const cloud = url.match(/^https?:\/\/bitbucket\.org\/([^/]+)\/([^/]+)\/pull-requests\/(\d+)/); const server = url.match(/^https?:\/\/[^/]+\/projects\/([^/]+)\/repos\/([^/]+)\/pull-requests\/(\d+)/); const m = cloud ?? server; if (!m) throw new Error(`BitbucketProvider: not a Bitbucket PR URL: ${url}`); - owner = m[1]; - repo = m[2]; - num = parseInt(m[3], 10); - const resolvedBaseUrl = (baseUrl || "https://api.bitbucket.org/2.0").replace(/\/$/, ""); - const me = await BitbucketProvider.api(resolvedBaseUrl, token, "/user"); - return new BitbucketProvider(token, resolvedBaseUrl, owner, repo, num, me.username); - } - static async api(baseUrl, token, path, opts) { - const resp = await fetch(`${baseUrl}${path}`, { - ...opts, - headers: { - Authorization: `Bearer ${token}`, - "Content-Type": "application/json", - ...(opts?.headers ?? {}), - }, - }); - if (!resp.ok) { - const text = await resp.text().catch(() => ""); - throw new Error(`Bitbucket API ${resp.status}: ${text.slice(0, 1024)}`); - } - return resp.json(); - } - api(path, opts) { - return BitbucketProvider.api(this.baseUrl, this.token, path, opts); + const api = makeApi((baseUrl || "https://api.bitbucket.org/2.0").replace(/\/$/, ""), token); + const me = await api("/user"); + return new BitbucketProvider(api, m[1], m[2], parseInt(m[3], 10), me.username); } prPath() { return `/repositories/${this.owner}/${this.repo}/pullrequests/${this.number}`; @@ -75,15 +72,6 @@ export class BitbucketProvider { state: pr.state.name === "MERGED" ? "merged" : (pr.state.name === "OPEN" ? "open" : "closed"), }; } - async getChangedFiles(meta) { - return gitChangedFiles({ baseSha: meta.baseSha, headSha: meta.headSha }); - } - async getPotentialRenames(meta) { - return gitPotentialRenames({ baseSha: meta.baseSha, headSha: meta.headSha }); - } - async getFileDiff(meta, path, opts) { - return gitFileDiff({ baseSha: meta.baseSha, headSha: meta.headSha, path, context: opts.context }); - } async getPRComments() { const out = []; const comments = await this.api(`${this.prPath()}/comments?pagelen=100`); @@ -139,6 +127,8 @@ export class BitbucketProvider { async deleteComment(commentId, _kind) { await this.api(`${this.prPath()}/comments/${commentId}`, { method: "DELETE" }); } + // Bitbucket's diffstat reports renames directly — richer than the base + // class's git -M derivation. async getRenames(_meta) { const diff = await this.api(`${this.prPath()}/diff`); const out = {}; diff --git a/dist/vcs/github.js b/dist/providers/github.js similarity index 93% rename from dist/vcs/github.js rename to dist/providers/github.js index 8db54ed..98798dd 100644 --- a/dist/vcs/github.js +++ b/dist/providers/github.js @@ -1,4 +1,4 @@ -// GitHub implementation of VcsProvider. REST via @octokit/rest for the basic +// GitHub implementation of Provider. REST via @octokit/rest for the basic // PR data and posting; GraphQL for review-thread resolution (the REST API // doesn't expose `isResolved` or a resolve endpoint on review threads). // @@ -7,8 +7,8 @@ // - `contents: read` to read the diff // - (read:user) implied by getAuthenticated for non-Actions runs. import { Octokit } from "@octokit/rest"; -import { gitChangedFiles, gitFileDiff, gitPotentialRenames } from "../lib/git.js"; -export class GitHubProvider { +import { BaseProvider } from "./base.js"; +export class GitHubProvider extends BaseProvider { client; owner; repo; @@ -16,6 +16,7 @@ export class GitHubProvider { botLogin; name = "github"; constructor(client, owner, repo, number, botLogin) { + super(); this.client = client; this.owner = owner; this.repo = repo; @@ -64,15 +65,6 @@ export class GitHubProvider { state: data.merged ? "merged" : data.state, }; } - async getChangedFiles(meta) { - return gitChangedFiles({ baseSha: meta.baseSha, headSha: meta.headSha }); - } - async getPotentialRenames(meta) { - return gitPotentialRenames({ baseSha: meta.baseSha, headSha: meta.headSha }); - } - async getFileDiff(meta, path, opts) { - return gitFileDiff({ baseSha: meta.baseSha, headSha: meta.headSha, path, context: opts.context }); - } async getPRComments() { const out = []; const issueComments = await this.client.paginate(this.client.issues.listComments, { @@ -161,6 +153,8 @@ export class GitHubProvider { await this.client.pulls.deleteReviewComment({ owner: this.owner, repo: this.repo, comment_id: id }); } } + // GitHub's compare API has its own rename detection — richer than the + // base class's git -M derivation. async getRenames(meta) { const resp = await this.client.repos.compareCommits({ owner: this.owner, repo: this.repo, base: meta.baseSha, head: meta.headSha, diff --git a/dist/vcs/gitlab.js b/dist/providers/gitlab.js similarity index 79% rename from dist/vcs/gitlab.js rename to dist/providers/gitlab.js index 394143b..5557f24 100644 --- a/dist/vcs/gitlab.js +++ b/dist/providers/gitlab.js @@ -1,4 +1,4 @@ -// GitLab implementation of VcsProvider. Plain fetch — no official SDK needed +// GitLab implementation of Provider. Plain fetch — no official SDK needed // for the subset we use. // // Token needs: @@ -7,18 +7,37 @@ // // `baseUrl` defaults to https://gitlab.com/api/v4; self-hosted instances pass // their own. -import { gitChangedFiles, gitFileDiff, gitPotentialRenames } from "../lib/git.js"; -export class GitLabProvider { - token; - baseUrl; +import { BaseProvider } from "./base.js"; +// Fetch wrapper owned by this provider — deliberately NOT shared with +// bitbucket.ts: the two wrappers look alike today only by coincidence, and +// the APIs will diverge (pagination, rate limits, auth). See docs/conventions/consolidation.md. +function makeApi(baseUrl, token) { + return async (path, init) => { + const resp = await fetch(`${baseUrl}${path}`, { + ...init, + headers: { + "PRIVATE-TOKEN": token, + "Content-Type": "application/json", + ...(init?.headers ?? {}), + }, + }); + if (!resp.ok) { + const text = await resp.text().catch(() => ""); + throw new Error(`GitLab API ${resp.status}: ${text.slice(0, 1024)}`); + } + return resp.json(); + }; +} +export class GitLabProvider extends BaseProvider { + api; projectPath; number; botLogin; name = "gitlab"; - constructor(token, baseUrl, projectPath, // {host}/{owner}/{repo} + constructor(api, projectPath, // {host}/{owner}/{repo} number, botLogin) { - this.token = token; - this.baseUrl = baseUrl; + super(); + this.api = api; this.projectPath = projectPath; this.number = number; this.botLogin = botLogin; @@ -32,29 +51,10 @@ export class GitLabProvider { const m = url.match(/^https?:\/\/([^/]+)\/([^/]+)\/([^/]+)\/-\/merge_requests\/(\d+)/); if (!m) throw new Error(`GitLabProvider: not a GitLab MR URL: ${url}`); - const resolvedBaseUrl = (baseUrl || "https://gitlab.com/api/v4").replace(/\/$/, ""); + const api = makeApi((baseUrl || "https://gitlab.com/api/v4").replace(/\/$/, ""), token); const projectPath = `${m[1]}/${m[2]}/${m[3]}`; - const me = await GitLabProvider.api(resolvedBaseUrl, token, "/user"); - return new GitLabProvider(token, resolvedBaseUrl, projectPath, parseInt(m[4], 10), me.username); - } - // Static so create() can use it before the instance exists. - static async api(baseUrl, token, path, opts) { - const resp = await fetch(`${baseUrl}${path}`, { - ...opts, - headers: { - "PRIVATE-TOKEN": token, - "Content-Type": "application/json", - ...(opts?.headers ?? {}), - }, - }); - if (!resp.ok) { - const text = await resp.text().catch(() => ""); - throw new Error(`GitLab API ${resp.status}: ${text.slice(0, 1024)}`); - } - return resp.json(); - } - api(path, opts) { - return GitLabProvider.api(this.baseUrl, this.token, path, opts); + const me = await api("/user"); + return new GitLabProvider(api, projectPath, parseInt(m[4], 10), me.username); } mrPath() { return `/projects/${encodeURIComponent(this.projectPath)}/merge_requests/${this.number}`; @@ -73,15 +73,6 @@ export class GitLabProvider { state: mr.state === "merged" ? "merged" : mr.state, }; } - async getChangedFiles(meta) { - return gitChangedFiles({ baseSha: meta.baseSha, headSha: meta.headSha }); - } - async getPotentialRenames(meta) { - return gitPotentialRenames({ baseSha: meta.baseSha, headSha: meta.headSha }); - } - async getFileDiff(meta, path, opts) { - return gitFileDiff({ baseSha: meta.baseSha, headSha: meta.headSha, path, context: opts.context }); - } async getPRComments() { const out = []; const notes = await this.api(`${this.mrPath()}/notes?per_page=100&sort=asc`); @@ -154,6 +145,8 @@ export class GitLabProvider { async deleteComment(commentId, _kind) { await this.api(`${this.mrPath()}/notes/${commentId}`, { method: "DELETE" }); } + // GitLab's changes API reports renames directly — richer than the base + // class's git -M derivation. async getRenames(_meta) { const changes = await this.api(`${this.mrPath()}/changes`); const out = {}; diff --git a/dist/vcs/local.js b/dist/providers/local.js similarity index 79% rename from dist/vcs/local.js rename to dist/providers/local.js index aa40bd9..2546d00 100644 --- a/dist/vcs/local.js +++ b/dist/providers/local.js @@ -8,7 +8,8 @@ // comments / verdict a real provider would post, just printed (like git's // own progress output). Pairs well with running a small local model // (LLM_URL / LLM_MODEL) for fast pre-push review while CI uses a heavier one. -import { gitChangedFiles, gitFileDiff, gitPotentialRenames, runGit } from "../lib/git.js"; +import { runGit } from "../lib/git.js"; +import { BaseProvider } from "./base.js"; // ANSI color, on only for an interactive stdout (TTY, no NO_COLOR) so // redirected / piped output stays plain. Raw escapes — no dependency. const COLOR = Boolean(process.stdout.isTTY) && !process.env.NO_COLOR; @@ -30,10 +31,11 @@ function badge(severity) { default: return cyan("info"); } } -export class LocalVcsProvider { +export class LocalProvider extends BaseProvider { meta; name = "local"; constructor(meta) { + super(); this.meta = meta; } static async create(config) { @@ -50,7 +52,7 @@ export class LocalVcsProvider { const headBranch = (await runGit(["rev-parse", "--abbrev-ref", "HEAD"])).trim(); const author = (await runGit(["log", "-1", "--format=%an"]).catch(() => "")).trim(); const title = (await runGit(["log", "-1", "--format=%s"]).catch(() => "")).trim(); - return new LocalVcsProvider({ + return new LocalProvider({ title: title || `${headBranch} vs ${base}`, description: "", author: author || "local", @@ -65,15 +67,6 @@ export class LocalVcsProvider { async getPRMetadata() { return this.meta; } - async getChangedFiles(meta) { - return gitChangedFiles({ baseSha: meta.baseSha, headSha: meta.headSha }); - } - async getPotentialRenames(meta) { - return gitPotentialRenames({ baseSha: meta.baseSha, headSha: meta.headSha }); - } - async getFileDiff(meta, path, opts) { - return gitFileDiff({ baseSha: meta.baseSha, headSha: meta.headSha, path, context: opts.context }); - } async getPRComments() { return []; } @@ -102,13 +95,4 @@ export class LocalVcsProvider { async deleteComment(commentId, kind) { out(dim(`↳ deleted ${kind} comment ${commentId}`)); } - async getRenames(meta) { - const changed = await gitChangedFiles({ baseSha: meta.baseSha, headSha: meta.headSha }); - const out = {}; - for (const f of changed) { - if (f.status === "renamed" && f.oldPath) - out[f.oldPath] = f.path; - } - return out; - } } diff --git a/dist/vcs/mock.js b/dist/providers/mock.js similarity index 84% rename from dist/vcs/mock.js rename to dist/providers/mock.js index 8f0f659..12a1bed 100644 --- a/dist/vcs/mock.js +++ b/dist/providers/mock.js @@ -17,7 +17,8 @@ import { tmpdir } from "node:os"; import { dirname, join } from "node:path"; import { fileURLToPath } from "node:url"; import { parse as parseYaml } from "yaml"; -import { gitChangedFiles, gitFileDiff, gitPotentialRenames } from "../lib/git.js"; +import { errorMessage } from "../lib/errors.js"; +import { BaseProvider } from "./base.js"; function findProjectRoot() { let dir = dirname(fileURLToPath(import.meta.url)); for (let i = 0; i < 10; i++) { @@ -38,7 +39,9 @@ function loadFixtureConfig(scenarioPath) { return parseYaml(readFileSync(p, "utf-8")); } catch (e) { - console.warn(`mock: failed to parse ${p}: ${e.message}`); + // console.warn (not the Logger port): the provider is constructed + // before a logger exists, and this path is test-only — deliberate exception. + console.warn(`mock: failed to parse ${p}: ${errorMessage(e)}`); return {}; } } @@ -96,7 +99,7 @@ const DEFAULT_META = { url: "mock://scenario", state: "open", }; -export class MockVcsProvider { +export class MockProvider extends BaseProvider { name = "mock"; repo; fixture; @@ -107,17 +110,19 @@ export class MockVcsProvider { _deletedComments = []; _verdicts = []; constructor(repo, fixture) { + super(); this.repo = repo; + this.gitCwd = repo?.cwd; this.fixture = fixture; } // Bare provider — no scenario, no git repo. For unit tests that drive a // single stage (e.g. summarize) with a hand-built StageState and only need // the side-effect recorders. static empty(fixture = {}) { - return new MockVcsProvider(null, fixture); + return new MockProvider(null, fixture); } static async create(config) { - return MockVcsProvider.fromUrl(config.pr.url); + return MockProvider.fromUrl(config.pr.url); } static fromUrl(url) { const m = url.match(/^mock:\/\/(.+)$/); @@ -127,16 +132,16 @@ export class MockVcsProvider { const scenarioPath = join(findProjectRoot(), "tests", "fixtures", scenario); if (!existsSync(scenarioPath)) { console.warn(`mock: scenario ${scenario} not found at ${scenarioPath}; provider has no fixture data`); - return new MockVcsProvider(null, {}); + return new MockProvider(null, {}); } const fixture = loadFixtureConfig(scenarioPath); const repo = setupRepo(scenarioPath); // Stages read changed files via relative paths; pin cwd to the temp repo // so existsSync / readFileSync resolve against the mock head tree. process.chdir(repo.cwd); - return new MockVcsProvider(repo, fixture); + return new MockProvider(repo, fixture); } - // ---- VcsProvider -------------------------------------------------------- + // ---- Provider -------------------------------------------------------- async getPRMetadata() { const fromFixture = this.fixture.metadata ?? {}; return { @@ -146,20 +151,27 @@ export class MockVcsProvider { headSha: this.repo?.headSha ?? fromFixture.headSha ?? "", }; } + // Bare (`empty()`) mocks have no git repo — guard, then defer to the + // base class's delegation against gitCwd. async getChangedFiles(meta) { if (!this.repo) return []; - return gitChangedFiles({ baseSha: meta.baseSha, headSha: meta.headSha, cwd: this.repo.cwd }); + return super.getChangedFiles(meta); } async getPotentialRenames(meta) { if (!this.repo) return []; - return gitPotentialRenames({ baseSha: meta.baseSha, headSha: meta.headSha, cwd: this.repo.cwd }); + return super.getPotentialRenames(meta); } async getFileDiff(meta, path, opts) { if (!this.repo) return ""; - return gitFileDiff({ baseSha: meta.baseSha, headSha: meta.headSha, path, context: opts.context, cwd: this.repo.cwd }); + return super.getFileDiff(meta, path, opts); + } + async getRenames(meta) { + if (!this.repo) + return {}; + return super.getRenames(meta); } async getPRComments() { return (this.fixture.comments ?? []).map((c) => ({ ...c })); @@ -182,19 +194,6 @@ export class MockVcsProvider { async deleteComment(commentId, kind) { this._deletedComments.push({ commentId, kind }); } - // Renames come from git's -M detection in getChangedFiles; mock keeps this - // method coherent with the others (callers use getChangedFiles directly). - async getRenames(meta) { - if (!this.repo) - return {}; - const changed = await gitChangedFiles({ baseSha: meta.baseSha, headSha: meta.headSha, cwd: this.repo.cwd }); - const out = {}; - for (const f of changed) { - if (f.status === "renamed" && f.oldPath) - out[f.oldPath] = f.path; - } - return out; - } // ---- Accessors for test assertions ------------------------------------- get cwd() { return this.repo?.cwd ?? null; } get postedInline() { return this._postedInline; } diff --git a/dist/vcs/types.js b/dist/providers/types.js similarity index 62% rename from dist/vcs/types.js rename to dist/providers/types.js index eba5e4c..c9cdfeb 100644 --- a/dist/vcs/types.js +++ b/dist/providers/types.js @@ -1,3 +1,10 @@ +// One bot inline finding awaiting action: the root comment of an unresolved +// inline thread authored by the bot. The pipeline gate (cli.ts), reconcile's +// candidate selection, and summarize's no-reconcile fallback all share this +// predicate — they must agree or the verdict drifts (ADR-0011). +export function isOpenBotThread(c) { + return c.by === "bot" && c.inline !== undefined && !c.resolved && !c.parentId; +} // Dispatch helper: returns which provider's `create()` should handle a URL. export function detectProvider(url) { if (/^mock:\/\//.test(url)) diff --git a/dist/stages/fetch/index.js b/dist/stages/fetch/index.js index 3a92534..79043bb 100644 --- a/dist/stages/fetch/index.js +++ b/dist/stages/fetch/index.js @@ -26,10 +26,10 @@ export async function runFetch(ctx) { const log = ctx.logger.child("fetch"); log.info("start", { pr: ctx.config.pr.url }); const [meta, comments] = await Promise.all([ - ctx.vcs.getPRMetadata(), - ctx.vcs.getPRComments(), + ctx.provider.getPRMetadata(), + ctx.provider.getPRComments(), ]); - const changed = await ctx.vcs.getChangedFiles(meta); + const changed = await ctx.provider.getChangedFiles(meta); const rendering = await assembleRendering(ctx, meta, changed, log); const standards = loadStandards(ctx.config.review.standardsRoot, log); const standardsBlock = renderStandards(standards); @@ -96,7 +96,7 @@ async function assembleRendering(ctx, meta, changed, log) { } // Deletion: head-side file is gone. Full deletion patch via -U10000. if (f.status === "deleted") { - const content = await ctx.vcs.getFileDiff(meta, f.path, { context: 10000 }); + const content = await ctx.provider.getFileDiff(meta, f.path, { context: 10000 }); pushDiff(out.diffs, f.path, "full_file", content); continue; } @@ -107,23 +107,23 @@ async function assembleRendering(ctx, meta, changed, log) { } // Small file → inline full file. if (headLineCount <= cfg.fullFileThresholdLines) { - const content = await ctx.vcs.getFileDiff(meta, f.path, { context: 10000 }); + const content = await ctx.provider.getFileDiff(meta, f.path, { context: 10000 }); pushDiff(out.diffs, f.path, "full_file", content); continue; } // Big file. Probe with narrow context to find first change + coverage. - const narrow = await ctx.vcs.getFileDiff(meta, f.path, { context: cfg.narrowContextLines }); + const narrow = await ctx.provider.getFileDiff(meta, f.path, { context: cfg.narrowContextLines }); const firstChange = firstHunkNewLine(narrow); const coverage = headLineCount > 0 ? headLinesCovered(narrow) / headLineCount : 0; if (coverage >= cfg.fullFileCoverageThreshold) { // High coverage → promote to full file display. - const content = await ctx.vcs.getFileDiff(meta, f.path, { context: 10000 }); + const content = await ctx.provider.getFileDiff(meta, f.path, { context: 10000 }); pushDiff(out.diffs, f.path, "full_file", content); continue; } if (firstChange !== null && firstChange <= cfg.bigFileHeaderLines) { // Earliest change near top — widening context pulls in lines 1..change. - const content = await ctx.vcs.getFileDiff(meta, f.path, { context: cfg.bigFileHeaderLines }); + const content = await ctx.provider.getFileDiff(meta, f.path, { context: cfg.bigFileHeaderLines }); pushDiff(out.diffs, f.path, "big_file", content); continue; } diff --git a/dist/stages/fetch/standards.js b/dist/stages/fetch/standards.js index c5084a2..824b81b 100644 --- a/dist/stages/fetch/standards.js +++ b/dist/stages/fetch/standards.js @@ -20,7 +20,7 @@ // db.query("SELECT * FROM users WHERE id = ?", [userId]); import { existsSync, readFileSync } from "node:fs"; import { join, resolve } from "node:path"; -import YAML from "yaml"; +import { parse as parseYaml } from "yaml"; export function loadStandards(standardsRoot, log) { if (!standardsRoot) return null; @@ -30,7 +30,7 @@ export function loadStandards(standardsRoot, log) { log.warn("standards.index_missing", { path: indexPath }); return null; } - const index = YAML.parse(readFileSync(indexPath, "utf-8")); + const index = parseYaml(readFileSync(indexPath, "utf-8")); if (!index?.topics) return { topics: [] }; const topics = []; @@ -40,7 +40,7 @@ export function loadStandards(standardsRoot, log) { log.warn("standards.topic_missing", { file: entry.file }); continue; } - const parsed = YAML.parse(readFileSync(filePath, "utf-8")); + const parsed = parseYaml(readFileSync(filePath, "utf-8")); topics.push({ description: entry.description ?? entry.file, rules: parsed?.rules ?? [], diff --git a/dist/stages/reconcile/index.js b/dist/stages/reconcile/index.js index ec7f206..ecfa18e 100644 --- a/dist/stages/reconcile/index.js +++ b/dist/stages/reconcile/index.js @@ -5,30 +5,18 @@ // 2. Auto-address — threads whose target file no longer exists in the // working tree are marked addressed=true deterministically (no LLM). // 3. verify() — LLM judges the remaining threads. -import { existsSync, readFileSync } from "node:fs"; -import { resolve } from "node:path"; -import { callLLM } from "../../llm/call.js"; -import { hasRendering, renderRenderingContext } from "../shared.js"; -const HERE = import.meta.dirname; -const PROMPT = readFileSync(resolve(HERE, "prompt.md"), "utf-8"); -const SCHEMA_TEXT = readFileSync(resolve(HERE, "schema.json"), "utf-8"); -const SCHEMA = JSON.parse(SCHEMA_TEXT); +import { existsSync } from "node:fs"; +import { errorMessage } from "../../lib/errors.js"; +import { callLLM } from "../../llm/client.js"; +import { isOpenBotThread } from "../../providers/types.js"; +import { baseSystemSections, baseUserSections, llmStageMetric, loadPromptAssets } from "../shared.js"; +const ASSETS = loadPromptAssets(import.meta.dirname); const RENAME_BANNER_PREFIX = "⚠ **File renamed:** "; -const SLOT = { - outputSchema: "output_schema", - projectContext: "project_context", - prMetadata: "pr_metadata", - prDiff: "pr_diff", - prComments: "pr_comments", - renderingContext: "rendering_context", -}; function splitThreads(comments) { const auto = []; const rest = []; for (const c of comments) { - if (c.by !== "bot" || c.resolved || c.parentId) - continue; - if (!c.inline) + if (!isOpenBotThread(c)) continue; if (c.inline.path && !existsSync(c.inline.path)) auto.push(c); @@ -77,14 +65,14 @@ async function handleRenames(ctx, comments, renames) { `This was a prior bot finding on the old path; please verify ` + `whether it still applies in the new location.\n\n` + `**Original finding:**\n\n${c.body || ""}`; - await ctx.vcs.postInlineComment({ path: newPath, line: 1, body: notice }); + await ctx.provider.postInlineComment({ path: newPath, line: 1, body: notice }); } - await ctx.vcs.resolveThread(c.threadId ?? c.id); + await ctx.provider.resolveThread(c.threadId ?? c.id); count++; log.info(alreadyPosted ? "re_resolved" : "relocated", { thread: c.id, oldPath, newPath }); } catch (e) { - log.error("relocate.failed", { thread: c.id, oldPath, newPath, error: e.message }); + log.error("relocate.failed", { thread: c.id, oldPath, newPath, error: errorMessage(e) }); } } return count; @@ -102,11 +90,11 @@ async function applyResolutions(ctx, byCommentId, decisions) { continue; } try { - await ctx.vcs.resolveThread(comment.threadId); + await ctx.provider.resolveThread(comment.threadId); resolved++; } catch (e) { - log.error("resolve.failed", { thread: comment.threadId, error: e.message }); + log.error("resolve.failed", { thread: comment.threadId, error: errorMessage(e) }); failed++; } } @@ -118,11 +106,11 @@ export async function runReconcile(ctx, state) { const t0 = Date.now(); const log = ctx.logger.child("reconcile"); let comments = state.comments; - const renames = await ctx.vcs.getRenames(state.meta); + const renames = await ctx.provider.getRenames(state.meta); if (Object.keys(renames).length > 0) { const relocated = await handleRenames(ctx, comments, renames); if (relocated > 0) - comments = await ctx.vcs.getPRComments(); + comments = await ctx.provider.getPRComments(); } const { auto, rest } = splitThreads(comments); const byCommentId = new Map(comments.map((c) => [c.id, c])); @@ -130,7 +118,7 @@ export async function runReconcile(ctx, state) { // paths look like a sub-confident rename — they'd otherwise be silently // marked addressed when in fact a moved file may still carry the concern. if (auto.length > 0) { - const potential = await ctx.vcs.getPotentialRenames(state.meta); + const potential = await ctx.provider.getPotentialRenames(state.meta); if (potential.length > 0) { const potByOld = new Map(potential.map((p) => [p.oldPath, p])); for (const c of auto) { @@ -169,27 +157,14 @@ export async function runReconcile(ctx, state) { const threadIds = new Set(rest.map((c) => c.id)); const replies = gatherReplies(comments, threadIds); log.info("llm_judging", { threads: rest.length, reply_context: replies.length }); - const systemSections = [ - { tag: "", content: PROMPT }, - { tag: SLOT.outputSchema, content: SCHEMA_TEXT }, - ]; - if (state.context.projectContext.trim()) { - systemSections.push({ tag: SLOT.projectContext, content: state.context.projectContext }); - } - if (hasRendering(state.context.rendering)) { - systemSections.push({ tag: SLOT.renderingContext, content: renderRenderingContext(state.context.rendering) }); - } - const userSections = [ - { tag: SLOT.prMetadata, content: JSON.stringify(state.meta, null, 2) }, - { tag: SLOT.prDiff, content: state.diff }, - { tag: SLOT.prComments, content: JSON.stringify([...rest, ...replies], null, 2) }, - ]; + const systemSections = baseSystemSections(ASSETS, state); + const userSections = baseUserSections(state, [...rest, ...replies]); const result = await callLLM({ ctx, stage: "reconcile", systemSections, userSections, - schema: SCHEMA, + schema: ASSETS.schema, }); const llmDecisions = result.validated.decisions ?? []; await applyResolutions(ctx, byCommentId, llmDecisions); @@ -197,12 +172,6 @@ export async function runReconcile(ctx, state) { ...state, comments, decisions: [...autoDecisions, ...llmDecisions], - metrics: [...state.metrics, { - stage: "reconcile", - durationMs: Date.now() - t0, - tokens: { input: result.usage.promptTokens, output: result.usage.completionTokens, total: result.usage.totalTokens }, - healAttempts: result.healAttempts, - toolCalls: result.toolCalls, - }], + metrics: [...state.metrics, llmStageMetric("reconcile", t0, result)], }; } diff --git a/dist/stages/review/index.js b/dist/stages/review/index.js index d5bbaf8..7e6a6ce 100644 --- a/dist/stages/review/index.js +++ b/dist/stages/review/index.js @@ -6,49 +6,20 @@ // so the prompt asks for a verbatim snippet (plus optional context_before/ // after to disambiguate) and we do the line-number work here. import { existsSync, readFileSync } from "node:fs"; -import { resolve } from "node:path"; -import { callLLM } from "../../llm/call.js"; -import { hasRendering, renderRenderingContext } from "../shared.js"; -const HERE = import.meta.dirname; -const PROMPT = readFileSync(resolve(HERE, "prompt.md"), "utf-8"); -const SCHEMA_TEXT = readFileSync(resolve(HERE, "schema.json"), "utf-8"); -const SCHEMA = JSON.parse(SCHEMA_TEXT); -const SLOT = { - outputSchema: "output_schema", - projectContext: "project_context", - prMetadata: "pr_metadata", - prDiff: "pr_diff", - prComments: "pr_comments", - threadDecisions: "thread_decisions", - reviewChecklist: "review_checklist", - renderingContext: "rendering_context", -}; +import { errorMessage } from "../../lib/errors.js"; +import { callLLM } from "../../llm/client.js"; +import { baseSystemSections, baseUserSections, llmStageMetric, loadPromptAssets, SLOT } from "../shared.js"; +const ASSETS = loadPromptAssets(import.meta.dirname); export async function runReview(ctx, state) { const t0 = Date.now(); const log = ctx.logger.child("review"); - const systemSections = [ - { tag: "", content: PROMPT }, - { tag: SLOT.outputSchema, content: SCHEMA_TEXT }, - ]; - if (state.context.projectContext.trim()) { - systemSections.push({ tag: SLOT.projectContext, content: state.context.projectContext }); - } - if (state.context.reviewChecklist) { - systemSections.push({ tag: SLOT.reviewChecklist, content: state.context.reviewChecklist }); - } - if (hasRendering(state.context.rendering)) { - systemSections.push({ tag: SLOT.renderingContext, content: renderRenderingContext(state.context.rendering) }); - } - const userSections = [ - { tag: SLOT.prMetadata, content: JSON.stringify(state.meta, null, 2) }, - { tag: SLOT.prDiff, content: state.diff }, - { tag: SLOT.prComments, content: JSON.stringify(state.comments, null, 2) }, - ]; + const systemSections = baseSystemSections(ASSETS, state, { includeChecklist: true }); + const userSections = baseUserSections(state, state.comments); if (state.decisions?.length) { userSections.push({ tag: SLOT.threadDecisions, content: JSON.stringify(state.decisions, null, 2) }); } const result = await callLLM({ - ctx, stage: "review", systemSections, userSections, schema: SCHEMA, + ctx, stage: "review", systemSections, userSections, schema: ASSETS.schema, }); const findings = result.validated.comments; await postFindings(ctx, findings); @@ -56,13 +27,7 @@ export async function runReview(ctx, state) { return { ...state, findings, - metrics: [...state.metrics, { - stage: "review", - durationMs: Date.now() - t0, - tokens: { input: result.usage.promptTokens, output: result.usage.completionTokens, total: result.usage.totalTokens }, - healAttempts: result.healAttempts, - toolCalls: result.toolCalls, - }], + metrics: [...state.metrics, llmStageMetric("review", t0, result)], }; } async function postFindings(ctx, findings) { @@ -75,7 +40,7 @@ async function postFindings(ctx, findings) { await postFinding(ctx, log, c); } catch (e) { - log.error("post.failed", { path: c.path, error: e.message }); + log.error("post.failed", { path: c.path, error: errorMessage(e) }); } } } @@ -86,7 +51,7 @@ async function postFinding(ctx, log, c) { if (!c.path || !existsSync(c.path)) { const body = wrapWithBanner(c.snippet, c.body, "anchor not located"); log.warn("anchor.unresolved", { path: c.path, fallback: "top_level" }); - await ctx.vcs.postSummaryComment(body); + await ctx.provider.postSummaryComment(body); return; } let line = findUniqueMatch(c.path, c); @@ -96,7 +61,7 @@ async function postFinding(ctx, log, c) { log.warn("anchor.unresolved", { path: c.path, fallback: "line_1" }); body = wrapWithBanner(c.snippet, c.body, "snippet not uniquely matched on head"); } - await ctx.vcs.postInlineComment({ path: c.path, line, body, severity: c.severity }); + await ctx.provider.postInlineComment({ path: c.path, line, body, severity: c.severity }); } function normalizeLines(text) { // Strip leading whitespace per line (prompt instructs the LLM to do the diff --git a/dist/stages/shared.js b/dist/stages/shared.js index d249e82..322f5fd 100644 --- a/dist/stages/shared.js +++ b/dist/stages/shared.js @@ -1,4 +1,70 @@ -// Helpers shared between LLM-using stages (reconcile, review). +// Helpers shared between LLM-using stages (reconcile, review): prompt-asset +// loading, the slot-tag table, base section assembly, and metric construction +// (ADR-0010). Helpers encode the envelope (order + presence rules); stages +// keep policy — they append their stage-specific sections to the returned +// arrays. +import { readFileSync } from "node:fs"; +import { resolve } from "node:path"; +// Loads a stage's colocated prompt.md + schema.json (ADR-0006). Call at +// module init with import.meta.dirname — a missing asset fails fast at +// import time. +export function loadPromptAssets(dir) { + const prompt = readFileSync(resolve(dir, "prompt.md"), "utf-8"); + const schemaText = readFileSync(resolve(dir, "schema.json"), "utf-8"); + return { prompt, schemaText, schema: JSON.parse(schemaText) }; +} +// ---- Prompt slots -------------------------------------------------------- +// Every XML tag any stage may inject. One table so tag names can't drift +// between stages; prompts document the tags they consume. +export const SLOT = { + outputSchema: "output_schema", + projectContext: "project_context", + prMetadata: "pr_metadata", + prDiff: "pr_diff", + prComments: "pr_comments", + threadDecisions: "thread_decisions", + reviewChecklist: "review_checklist", + renderingContext: "rendering_context", +}; +// System envelope: prompt → output_schema → project_context → (checklist) → +// rendering_context. Empty sections are omitted. +export function baseSystemSections(assets, state, opts) { + const sections = [ + { tag: "", content: assets.prompt }, + { tag: SLOT.outputSchema, content: assets.schemaText }, + ]; + if (state.context.projectContext.trim()) { + sections.push({ tag: SLOT.projectContext, content: state.context.projectContext }); + } + if (opts?.includeChecklist && state.context.reviewChecklist) { + sections.push({ tag: SLOT.reviewChecklist, content: state.context.reviewChecklist }); + } + if (hasRendering(state.context.rendering)) { + sections.push({ tag: SLOT.renderingContext, content: renderRenderingContext(state.context.rendering) }); + } + return sections; +} +// User envelope: pr_metadata → pr_diff → pr_comments. The comment set is a +// parameter because stages scope it differently (review sends all comments; +// reconcile sends only the threads under judgment plus their replies). +export function baseUserSections(state, comments) { + return [ + { tag: SLOT.prMetadata, content: JSON.stringify(state.meta, null, 2) }, + { tag: SLOT.prDiff, content: state.diff }, + { tag: SLOT.prComments, content: JSON.stringify(comments, null, 2) }, + ]; +} +// ---- Metrics -------------------------------------------------------------- +export function llmStageMetric(stage, t0, r) { + return { + stage, + durationMs: Date.now() - t0, + tokens: { input: r.usage.promptTokens, output: r.usage.completionTokens, total: r.usage.totalTokens }, + healAttempts: r.healAttempts, + toolCalls: r.toolCalls, + }; +} +// ---- Rendering context ---------------------------------------------------- // Renders a RenderingContext into the body of a slot. // Each sub-bucket becomes its own block; empty buckets are skipped. The LLM // sees structured sections it can scan by type and walk by file. diff --git a/dist/stages/summarize/index.js b/dist/stages/summarize/index.js index 22981cb..37eab53 100644 --- a/dist/stages/summarize/index.js +++ b/dist/stages/summarize/index.js @@ -1,11 +1,16 @@ // summarize — derives the final verdict, posts the summary comment, and // sets the PR verdict on the provider. No file writes — verdict signal is // state.verdict + (in cli.ts) the process exit code. -const SUMMARY_FRAGMENTS = [ - "prior bot finding(s) still unaddressed", - "No reviewable source content", - "informational finding(s)", -]; +import { errorMessage } from "../../lib/errors.js"; +import { isOpenBotThread } from "../../providers/types.js"; +// Fragments that identify a bot comment as one of OUR summary comments. +// buildSummary interpolates these exact constants into the text it posts, +// and isPriorSummary matches on the same list — construction and detection +// can't drift apart (ADR-0012). Reword a block by editing its constant. +const FRAG_UNADDRESSED = "prior bot finding(s) still unaddressed"; +const FRAG_NO_SOURCE = "No reviewable source content"; +const FRAG_INFO = "informational finding(s)"; +const SUMMARY_FRAGMENTS = [FRAG_UNADDRESSED, FRAG_NO_SOURCE, FRAG_INFO]; function isPriorSummary(c) { if (c.by !== "bot" || c.inline) return false; @@ -24,7 +29,7 @@ function deriveVerdict(ctx, state) { // Reconcile didn't run. Treat every unresolved bot inline thread as // still pending — we don't know whether the head addresses them. unaddressed = state.comments - .filter((c) => c.by === "bot" && c.inline && !c.resolved && !c.parentId) + .filter(isOpenBotThread) .map((c) => ({ thread_id: c.threadId ?? c.id, comment_id: c.id, @@ -42,7 +47,7 @@ function clip(s, cap) { function buildSummary(d, charLimit) { const blocks = []; if (d.verdict === "reject" && d.unaddressed.length > 0) { - const lines = [`**${d.unaddressed.length} prior bot finding(s) still unaddressed:**`, ""]; + const lines = [`**${d.unaddressed.length} ${FRAG_UNADDRESSED}:**`, ""]; for (const dec of d.unaddressed) { const tid = dec.thread_id || dec.comment_id; const reason = clip((dec.reason || "").trim(), charLimit); @@ -51,11 +56,11 @@ function buildSummary(d, charLimit) { blocks.push(lines.join("\n")); } if (d.verdict === "reject" && d.noReviewableSource) { - blocks.push("**No reviewable source content** in this PR — all changed files are binary or non-whitelisted (translations, lockfiles, assets, etc.). Bot can't read them; a human reviewer must confirm the change set is intentional before merge."); + blocks.push(`**${FRAG_NO_SOURCE}** in this PR — all changed files are binary or non-whitelisted (translations, lockfiles, assets, etc.). Bot can't read them; a human reviewer must confirm the change set is intentional before merge.`); } if (d.infoFindings.length > 0) { const lines = [ - `**${d.infoFindings.length} informational finding(s)** (nits / suggestions, not blocking — listed here as summary, not tracked as individual threads):`, + `**${d.infoFindings.length} ${FRAG_INFO}** (nits / suggestions, not blocking — listed here as summary, not tracked as individual threads):`, "", ]; for (const c of d.infoFindings) { @@ -91,33 +96,26 @@ export async function runSummarize(ctx, state) { if (!isPriorSummary(c)) continue; try { - await ctx.vcs.deleteComment(c.id, "issue"); + await ctx.provider.deleteComment(c.id, "issue"); deleted++; } catch (e) { - log.warn("delete.prior_summary_failed", { id: c.id, error: e.message }); + log.warn("delete.prior_summary_failed", { id: c.id, error: errorMessage(e) }); } } if (deleted) log.info("prior_summaries.deleted", { count: deleted }); const summary = buildSummary(d, ctx.config.review.summaryCommentCharLimit); let verdictError = null; - if (d.verdict === "approve") { + if (d.verdict !== "unknown") { try { - await ctx.vcs.approve(summary || undefined); + const post = d.verdict === "approve" ? ctx.provider.approve(summary || undefined) + : ctx.provider.reject(summary || undefined); + await post; } catch (e) { - log.error("verdict.failed", { verdict: d.verdict, error: e.message }); - verdictError = e; - } - } - else if (d.verdict === "reject") { - try { - await ctx.vcs.reject(summary || undefined); - } - catch (e) { - log.error("verdict.failed", { verdict: d.verdict, error: e.message }); - verdictError = e; + verdictError = errorMessage(e); + log.error("verdict.failed", { verdict: d.verdict, error: verdictError }); } } // Fatal gates CI: approve verdict is the only non-fatal outcome. @@ -125,7 +123,7 @@ export async function runSummarize(ctx, state) { const fatal = d.verdict !== "approve" || verdictError !== null; if (fatal) { if (verdictError) - log.error("fatal.verdict_failed", { verdict: d.verdict, error: verdictError.message }); + log.error("fatal.verdict_failed", { verdict: d.verdict, error: verdictError }); else if (d.verdict === "unknown") log.error("fatal.no_review_result"); else if (d.verdict === "reject") diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md new file mode 100644 index 0000000..b7fff06 --- /dev/null +++ b/docs/ARCHITECTURE.md @@ -0,0 +1,245 @@ +# Architecture + +LLM-driven pull-request reviewer. One binary (`reviewer`), four pipeline stages, pluggable +VCS destinations, any OpenAI-compatible LLM endpoint. Designed to run as a CI gate: the +process exit code *is* the verdict. + +This document is the map: structure as it is and the patterns the codebase commits to. +Portable engineering rules live in [docs/conventions/](conventions/); dated decisions +(and their trade-offs) in [docs/adr/](adr/). + +--- + +## 1. System overview + +```mermaid +flowchart TD + cli["cli.ts
argv parsing + pipeline orchestration"] + ctx["ctx.ts
Ctx: config · provider · logger
(DI container, built once)"] + cli -->|"buildCtx()"| ctx + + subgraph pipeline["stages/ — StageState flows through, each stage appends"] + direction LR + fetch --> reconcile --> review --> summarize + end + ctx -->|passed to every stage| pipeline + + subgraph providers["providers/ — Provider port"] + direction LR + github + gitlab + bitbucket + localp[local] + mock + end + + subgraph llm["llm/"] + direction LR + client["client.ts
callLLM + resolveModel"] + wire["transport.ts
HTTP + SSE wire"] + tools["tools/
Read · Bash"] + client --> wire + client --> tools + end + + pipeline --> providers + pipeline --> llm + git["lib/git.ts — local git diff helpers
(shared by all providers via BaseProvider)"] + providers --> git +``` + +Two external worlds, each behind one port: + +- **VCS** (`providers/types.ts` → `Provider`) — where the PR lives and where findings are + posted. Five adapters: GitHub, GitLab, Bitbucket, `local://` (console echo), `mock://` + (test recorder). +- **LLM** (`llm/`) — any OpenAI-compatible `/chat/completions` endpoint, streamed, with a + local tool loop (`Read`, `Bash`) and schema-validated JSON output. + +Everything else is orchestration around a single state object. + +## 2. Runtime flow (`reviewer run`) + +```mermaid +flowchart LR + fetch --> q{"unresolved
bot threads?"} + q -- yes --> reconcile --> review + q -- no --> review + review --> summarize --> v{"verdict"} + v -- approve --> ok(["exit 0"]) + v -- "reject / fatal" --> fail(["exit 1"]) +``` + +1. `parseArgs` → `buildCtx` — config is loaded (env + CLI overrides), the provider is + chosen from the PR URL shape, the model is auto-detected from `GET /models` if unset. +2. **fetch** — PR metadata, comments, changed files; classifies each changed file into a + rendering strategy (`full_file` / `big_file` / binary bucket); loads project standards + and checklist. Pure setup: no LLM, no writes to the PR. +3. **reconcile** (only when unresolved bot threads exist) — deterministic passes first + (rename relocation, auto-address on deleted files), then one LLM call judging the + surviving threads. Resolves threads it deems addressed. +4. **review** — one LLM call with the diff + rendering context + tools; anchors each + finding's verbatim snippet to a head-side line locally; posts inline comments. +5. **summarize** — derives the verdict from accumulated state (no LLM), deletes stale + bot summaries, posts the new summary, calls `approve()`/`reject()` on the provider. + `verdict.fatal` drives `process.exit(1)`. + +Every stage is independently addressable as a subcommand (each re-runs fetch first). + +## 3. Module map + +| Path | Role | +| --- | --- | +| `src/cli.ts` | argv parsing, command → stage-sequence dispatch, exit code | +| `src/ctx.ts` | `Config` shape, `loadConfig` (the **only** `process.env` reader), provider dispatch, `buildCtx` | +| `src/stages/types.ts` | `StageState` and everything inside it — the pipeline's shared vocabulary | +| `src/stages/shared.ts` | helpers shared by LLM-using stages (prompt assets, slots, sections, metrics) | +| `src/stages//index.ts` | one `run(ctx, state)` function per stage | +| `src/stages//prompt.md`, `schema.json` | the stage's LLM contract, colocated | +| `src/providers/types.ts` | `Provider` port + wire types + `detectProvider` URL dispatch | +| `src/providers/.ts` | one adapter per destination | +| `src/llm/types.ts` | shared LLM contract: wire shapes (`ChatMessage`, `ToolCall`, …) + `Section` / `LLMResult` | +| `src/llm/client.ts` | consumer API: `callLLM` (tool loop, JSON extraction, schema healing) + `resolveModel` | +| `src/llm/transport.ts` | the wire: `send` (HTTP POST), `receive` (SSE accumulator) | +| `src/llm/tools/` | LLM-callable tools, registry + one file per tool | +| `src/lib/` | host-level helpers with no domain knowledge (`git.ts`) | +| `src/logger/` | `Logger` port + console / memory sinks | +| `tests/e2e/` | vitest suites driving real stages against `mock://` fixtures | +| `tests/fixtures///` | `old/` + `new/` repo pair + `config.yaml` per scenario | + +## 4. Core patterns + +### 4.1 Ctx — single DI container (ADR-0001) + +All dependencies (`config`, `provider`, `logger`) ride in one `Ctx` built once at entry and +passed explicitly to every stage. No globals, no service locator, no module-level +singletons holding state. Tests swap parts by passing overrides into `buildCtx` +(`logger`, `review` config) — never by monkey-patching. + +**Corollary — env isolation:** `process.env` is read in exactly one function, +`loadConfig`. Everything downstream consumes the typed `Config`. A new setting means a +new typed field + default, not a scattered `process.env.X` read. + +### 4.2 Stage pipeline — uniform `StageState` (ADR-0002) + +Every stage has the same signature: + +```ts +runX(ctx: Ctx, state: StageState): Promise // fetch takes only ctx +``` + +Rules the shape encodes: + +- Stage outputs are **additive**: each optional field (`decisions`, `findings`, + `verdict`) is set once by exactly one producing stage. +- Stages return `{ ...state, , metrics: [...state.metrics, metric] }` — + spread-and-extend, no in-place mutation of the state object itself. +- Telemetry is uniform: every stage appends one `StageMetric` (duration, tokens, heal + attempts, tool calls, `skipped`). +- The orchestrator (`cli.ts`) holds the sequencing policy (when reconcile runs); stages + hold no knowledge of each other. + +### 4.3 Provider — port with static async factories (ADR-0003) + +One interface, five adapters. Key commitments: + +- **One PR per instance.** Identity (owner/repo/number, resolved bot login) is captured + in `static async create(config)`; methods take no ref parameter. +- **Constructors are private and synchronous; `create()` does the async work** (token → + bot identity resolution, URL parsing, fixture/repo setup). +- **Semantic outcomes, not mechanics.** `approve()`/`reject()` encapsulate per-platform + degradation (GitHub bots post a COMMENT review instead of approving; GitLab/Bitbucket + have no native request-changes). Callers never branch on provider name. +- **Diffs are computed locally.** All providers delegate changed-files/diff/rename work + to `lib/git.ts` against the checked-out repo (the shared base class in + `providers/base.ts` carries these methods — ADR-0004); the VCS API supplies only + metadata, comments, and write operations. +- URL shape → provider is decided in one place: `detectProvider`. + +### 4.4 LLM call — tool loop with schema healing (ADR-0005) + +`callLLM` is the only way stages talk to the model. Per call: + +1. Serialize prompt sections (4.5) into system + user messages. +2. Stream the completion; while the model returns `tool_calls`, execute them locally + (`llm/tools/`) and feed results back — repeat until final text. +3. Extract JSON (fenced block → whole text → longest-parsing-span salvage), validate + against the stage's JSON Schema (ajv). +4. On failure, **heal**: append the bad output + a failure-specific instruction and + retry, up to `healRetries`. Heal prompts distinguish malformed-JSON from + schema-mismatch — the fix instructions differ. + +Tools follow a registry pattern: each tool is `{ schema, execute }` in its own file, +collected in `tools/index.ts`. Tool errors are written *for the model* — explicit about +what was received and what to do next, because weaker models loop on terse errors. + +### 4.5 Prompt assembly — colocated assets + tagged slots (ADR-0006) + +A stage's LLM contract is two files next to its code: `prompt.md` (instructions) and +`schema.json` (output shape, also embedded into the prompt as ``). +Dynamic data is injected as XML-tagged sections (``, ``, +``, …) via the `Section { tag, content }` list; empty sections are +omitted. The prompt names every tag it may receive, so prompt ↔ code stay reviewable +side by side. + +### 4.6 Snippet anchoring — LLMs quote, the harness counts (ADR-0007) + +Findings never carry line numbers. The model emits a verbatim single-line `snippet` +(plus optional `context_before/after`); `review/index.ts` locates it in the head file +(whitespace-lenient, uniqueness-checked) and converts to a line number. Degradation is +explicit: file gone → top-level comment with a banner; ambiguous → line 1 with a banner. + +### 4.7 Logger — port + scoped children (ADR-0008) + +Structured, event-named logging (`scope.event` via `child()` chains, snake_case event +names, data as a flat object). Two sinks: `ConsoleLogger` (pretty, stderr) and +`MemoryLogger` (test assertions). The `stream()` channel carries raw LLM tokens so CI +logs show output live; structured sinks may ignore it. + +### 4.8 Deterministic-first, LLM-last + +Wherever a judgment can be made mechanically, it is — before any model call. Reconcile +auto-addresses threads on deleted files and relocates renamed ones deterministically and +only sends the remainder to the LLM; summarize derives the verdict purely from state; +fetch classifies files by size/coverage heuristics. The LLM is reserved for judgment +that genuinely needs reading code. + +### 4.9 Safety gates are environmental, not flags + +`local://` review (LLM gets unsandboxed `Bash`/`Read`) requires an actual container, +detected via runtime-created files (`/.dockerenv`, `/run/.containerenv`) — not an env +flag anyone could export. Same idea elsewhere: bot identity is derived from the token +(`GET /user`), not from a CI-shaped env variable. + +### 4.10 Test strategy — real stages, mock destination, real git + +E2E tests drive real stage functions through `mock://` fixtures. The mock +provider builds a **real throwaway git repo** from `old/` + `new/` fixture trees, so +diff/rename behavior is git's own, not a reimplementation. Side effects are recorded on +the mock for assertions. Assertions favor structural guarantees (what got posted, which +bucket a file landed in) over fragile text matching of LLM output. Pure logic +(summarize) is unit-tested with hand-built state — no LLM, no fixture. + +## 5. Conventions + +The portable rule set (naming, types placement, consolidation, error handling, +configuration, logging, testing, documentation) lives in +[docs/conventions/](conventions/) and is adopted by ADR-0013. Below are only the +**repo-specific** conventions on top of it. + + +**Language / compiler.** TypeScript, `strict` + `noUncheckedIndexedAccess`, ESM, +Node ≥ 24. Source runs directly via `--experimental-transform-types` (Docker, `pnpm +start`); `dist/` is compiled output committed for npm consumers. Imports use explicit +`.ts` extensions (`rewriteRelativeImportExtensions` handles emit). `import type` for +type-only imports. Node built-ins use the `node:` prefix. + +**Naming.** `run` for stage entry points; `Provider` for VCS adapters; +`UPPER_SNAKE` for module-level constants; log events are `dot.scoped` snake_case +(`anchor.unresolved`, `llm.heal`); config fields are camelCase; LLM wire types mirror +the OpenAI JSON (snake_case) and are not renamed. + +**Data shapes.** Plain interfaces + functions; classes only where identity/lifecycle +matter (providers, loggers). Wire formats (`PRComment`, `ChangedFile`) are defined once +— in the module that produces them — and re-exported by the port that exposes them +(ADR-0009). Schema/LLM-facing fields are snake_case; internal fields camelCase. diff --git a/docs/adr/0001-ctx-dependency-injection-and-env-isolation.md b/docs/adr/0001-ctx-dependency-injection-and-env-isolation.md new file mode 100644 index 0000000..8e6f9e4 --- /dev/null +++ b/docs/adr/0001-ctx-dependency-injection-and-env-isolation.md @@ -0,0 +1,34 @@ +# ADR-0001: Ctx dependency-injection container and env isolation + +Status: Accepted (2026-06-10) + +## Context + +The pipeline needs three dependencies everywhere: typed configuration, the VCS +provider, and a logger. Alternatives were module-level singletons (hidden coupling, +hard to test), per-module env reads (untyped, scattered), or a DI framework (overkill +for a CLI of this size). + +## Decision + +A single `Ctx { config, provider, logger }` object is built once in `buildCtx()` at CLI +entry and passed explicitly as the first argument to every stage and to `callLLM`. + +`process.env` is read in exactly one function — `loadConfig()` in `src/ctx.ts`. All +downstream code consumes the typed `Config`. Defaults live next to the loader +(`REVIEW_DEFAULTS`). CLI flags arrive as a `CliOverrides` object and win over env. + +Tests construct a `Ctx` through the same `buildCtx`, overriding only what they need +(`logger`, `review` config) — no monkey-patching, no special test wiring inside +production code. + +## Consequences + +- Any function's dependencies are visible in its signature; nothing reaches around the + container. +- Adding a setting = one typed field + default + (optionally) one env read in + `loadConfig`. Grep for `process.env` outside `ctx.ts` should return nothing + (the `NO_COLOR`/TTY check in the local provider's console styling is the one + tolerated exception — it is presentation, not configuration). +- Per-provider credential validation happens at load time with actionable messages, + so misconfiguration fails before any network call. diff --git a/docs/adr/0002-stage-pipeline-uniform-stagestate.md b/docs/adr/0002-stage-pipeline-uniform-stagestate.md new file mode 100644 index 0000000..a7f9655 --- /dev/null +++ b/docs/adr/0002-stage-pipeline-uniform-stagestate.md @@ -0,0 +1,43 @@ +# ADR-0002: Stage pipeline with a uniform StageState + +Status: Accepted (2026-06-10) + +## Context + +The reviewer is a sequence of phases (fetch → reconcile → review → summarize) that +must also be runnable individually as subcommands, and whose intermediate results +(findings, decisions, metrics) need to flow forward without stages knowing about each +other. + +## Decision + +Every stage is a single async function with a uniform signature: + +```ts +runX(ctx: Ctx, state: StageState): Promise // runFetch takes only ctx +``` + +`StageState` (in `src/stages/types.ts`) is the pipeline's entire shared vocabulary: + +- **Additive outputs.** Optional fields (`decisions`, `findings`, `verdict`) are each + set once, by exactly one producing stage. Consumers treat absence as "that stage + didn't run" and degrade explicitly (e.g. summarize falls back to raw comments when + `decisions` is undefined). +- **Spread-and-extend returns.** Stages return + `{ ...state, , metrics: [...state.metrics, metric] }`; they don't mutate the + incoming object. (Interior collections like `comments` may be refreshed by + reassignment, never by splicing the caller's array.) +- **Uniform telemetry.** Each stage appends one `StageMetric` — duration, optional + token usage / heal attempts / tool calls / `skipped`. + +Sequencing policy lives only in `cli.ts` (e.g. reconcile runs only when unresolved bot +threads exist). Stages never call each other. + +## Consequences + +- New stages slot in without touching existing ones; subcommands are trivial + compositions. +- Tests can drive any stage with a hand-built `StageState` (see + `tests/e2e/summarize.test.ts`). +- The cost is a wide state type; discipline required: a field's producer must stay + unique, and `stages/types.ts` is the only place to grow the shape. diff --git a/docs/adr/0003-provider-port-with-static-async-factories.md b/docs/adr/0003-provider-port-with-static-async-factories.md new file mode 100644 index 0000000..297236d --- /dev/null +++ b/docs/adr/0003-provider-port-with-static-async-factories.md @@ -0,0 +1,38 @@ +# ADR-0003: Provider port with static async factories + +Status: Accepted (2026-06-10) + +## Context + +The reviewer posts to GitHub, GitLab, and Bitbucket, plus a console-echo `local://` +mode and a `mock://` test double. Each platform differs in auth, comment/thread +models, and what verdict actions a bot may perform. Construction needs async work +(resolving the bot identity from the token, parsing the URL, setting up fixture +repos). + +## Decision + +One interface — `Provider` in `src/providers/types.ts` — five adapters. Commitments: + +- **One PR per instance.** PR identity is captured at construction; methods take no + ref parameter. +- **Private constructor + `static async create(config)`.** Async setup happens in the + factory; instances are always fully initialized. `detectProvider(url)` is the single + URL-shape → provider dispatch point, used by `loadConfig` and `makeVcs`. +- **Semantic outcomes over mechanics.** `approve()` / `reject()` internally apply + platform rules (GitHub bots post a COMMENT review instead of approving; GitLab and + Bitbucket have no native request-changes, so only the summary lands). Callers never + branch on provider name; the process exit code is the universal gate. +- **Diff work is local.** Changed files, per-file diffs, and rename detection always + come from `git` against the checkout (ADR-0004); the VCS API supplies metadata, + comments, and writes. This keeps diff semantics identical across providers. +- **Bot identity comes from the token** (`GET /user`), not from environment shape — + so reconcile recognizes its own prior comments regardless of how the run is hosted. + +## Consequences + +- Adding a destination = one file implementing the port + one `detectProvider` branch + + one `makeVcs` case + a token field in config. +- Platform degradation rules are encapsulated where the platform knowledge lives. +- The mock provider records side effects (`postedInline`, `verdicts`, …), giving tests + a structural view of what the pipeline did without network access. diff --git a/docs/adr/0004-base-provider-shared-git-plumbing.md b/docs/adr/0004-base-provider-shared-git-plumbing.md new file mode 100644 index 0000000..6dd0201 --- /dev/null +++ b/docs/adr/0004-base-provider-shared-git-plumbing.md @@ -0,0 +1,41 @@ +# ADR-0004: BaseProvider — shared local-git plumbing for all providers + +Status: Accepted (2026-06-10) + +## Context + +All five providers implement `getChangedFiles`, `getPotentialRenames`, and +`getFileDiff` as byte-identical delegations to `lib/git.ts` (the mock adds a `cwd` +and a null-repo guard). On top of that, `LocalProvider.getRenames` and +`MockProvider.getRenames` contained the same derive-renames-from-changed-files +loop. That's ~20 copy-pasted methods whose only purpose is plumbing — every new +provider would copy them again, and a fix (e.g. a new diff flag) would need five +edits. + +## Decision + +Introduce `src/providers/base.ts` with an abstract `BaseProvider`: + +- Implements `getChangedFiles` / `getPotentialRenames` / `getFileDiff` once, + delegating to `lib/git.ts` against `this.gitCwd` (a `protected` field, `undefined` = + process cwd; the mock sets its temp repo path). +- Implements a default `getRenames` derived from `getChangedFiles` (git `-M` + detection). Providers whose API gives richer rename data (GitHub compare, GitLab + changes, Bitbucket diffstat) **override** it. +- All five providers `extend BaseProvider implements Provider`. The mock keeps + thin guard overrides (`if (!this.repo) return []` then `super.…`) because a bare + mock (`MockProvider.empty()`) has no repo by design. + +Inheritance is justified here (vs. composition) because the methods are part of the +same port the classes already implement, the base carries no state beyond `gitCwd`, +and the hierarchy is exactly one level deep. This is a "shared adapter plumbing" base, +not a framework. + +## Consequences + +- Diff semantics are guaranteed identical across providers — there is now only one + implementation to be identical to. +- A new provider gets diff plumbing for free; it implements only the genuinely + platform-specific surface (metadata, comments, writes, verdicts). +- The mock's guards make the "no repo" mode explicit instead of being interleaved + with the delegation logic. diff --git a/docs/adr/0005-llm-client-loop-with-schema-healing.md b/docs/adr/0005-llm-client-loop-with-schema-healing.md new file mode 100644 index 0000000..854ddb0 --- /dev/null +++ b/docs/adr/0005-llm-client-loop-with-schema-healing.md @@ -0,0 +1,36 @@ +# ADR-0005: LLM client loop with tool execution and schema healing + +Status: Accepted (2026-06-10) + +## Context + +Stages need structured (JSON-schema-conforming) output from arbitrary +OpenAI-compatible endpoints, including small self-hosted models that emit malformed +JSON, wrap output in fences, or need several tool calls before answering. The +endpoint may not support native structured output or tool streaming reliably. + +## Decision + +`callLLM` (`src/llm/client.ts`) is the only path from a stage to the model. Shared shapes (wire messages, `Section`, `LLMResult`) live in `llm/types.ts`. Layers: + +- `transport.ts` — the wire, both directions: `send` (HTTP POST returning the raw `Response`), and `receive` — an SSE accumulator that reassembles streamed deltas (including fragmented tool-call arguments) into a non-streaming-shaped message, mirroring raw text to the logger's `stream()` channel so CI shows output live. +- `client.ts` — the consumer API: `callLLM` (the loop: while the model returns `tool_calls`, execute locally and feed + results back; on final text, extract JSON (fenced block → whole text → longest + parsing-span salvage), validate with ajv against the stage's schema; on failure + append the bad output plus a failure-specific heal prompt and retry up to + `healRetries`. Malformed-JSON and schema-mismatch get different heal instructions + because the fixes differ. Exhaustion throws `LLMValidationError`. + +Tools follow a registry pattern (`llm/tools/`): each tool is a `{ schema, execute }` +object in its own file, listed once in `tools/index.ts`. Tool errors are written for +the model — they state what was received and what to do next, because weak models loop +on terse errors. Tool output is capped (64 KB) before being returned. + +## Consequences + +- Stages stay declarative: sections in, validated typed object out, usage/heal/tool + counts attached for metrics. +- Works against any OpenAI-compatible endpoint with no per-vendor branches; the price + is maintaining the salvage/heal machinery ourselves. +- Adding a tool = one file + one array entry; the dispatcher and conventions are + fixed. diff --git a/docs/adr/0006-prompt-asset-colocation-and-tagged-sections.md b/docs/adr/0006-prompt-asset-colocation-and-tagged-sections.md new file mode 100644 index 0000000..385214f --- /dev/null +++ b/docs/adr/0006-prompt-asset-colocation-and-tagged-sections.md @@ -0,0 +1,28 @@ +# ADR-0006: Prompt/schema colocation and tagged prompt sections + +Status: Accepted (2026-06-10) + +## Context + +Each LLM stage has a substantial prompt and a JSON output schema. Embedding them as TS +template strings makes them unreviewable and untestable as documents; a central +`prompts/` folder separates them from the code that fills their slots. + +## Decision + +A stage's LLM contract lives next to its code: `src/stages//prompt.md` and +`schema.json`, loaded at module init and shipped to `dist/` by +`scripts/copy-assets.mjs`. The schema text is also injected into the prompt as +``, so the instructions and the validator can never disagree. + +Dynamic data is injected as XML-tagged sections (`Section { tag, content }`), +serialized in order with empty sections omitted. Tag names are declared in the shared +`SLOT` table (`stages/shared.ts`); the prompt documents every tag it may receive. + +## Consequences + +- Prompt changes are reviewable diffs of a markdown file, side by side with the code + that populates its slots. +- The model always sees the same schema ajv validates against. +- Build must copy non-TS assets (handled once in `copy-assets.mjs`); module-init file + reads mean a missing asset fails fast at import time. diff --git a/docs/adr/0007-snippet-anchoring-over-line-numbers.md b/docs/adr/0007-snippet-anchoring-over-line-numbers.md new file mode 100644 index 0000000..d5bb9b9 --- /dev/null +++ b/docs/adr/0007-snippet-anchoring-over-line-numbers.md @@ -0,0 +1,33 @@ +# ADR-0007: Snippet anchoring instead of LLM line numbers + +Status: Accepted (2026-06-10) + +## Context + +Inline comments need a `path:line` anchor. LLMs reliably *quote* source but +unreliably *count* lines — asking for line numbers yields confidently wrong anchors, +and unified-diff line bookkeeping makes it worse. + +## Decision + +The review schema asks for a verbatim **single-line** `snippet` (plus optional +single-line `context_before` / `context_after` for disambiguation). The harness +(`stages/review/index.ts`) does the line math: normalize leading whitespace, find all +matches in the head-side file, disambiguate with context, and post at the unique +match. + +Degradation is explicit and visible: + +- File doesn't exist on head → top-level summary comment wrapping the snippet with a + banner explaining the anchor failure. +- Snippet not uniquely located → inline comment at line 1 with the same banner. + +Single-line snippets are mandated in the schema because multi-line blocks both anchor +worse and break JSON escaping more often. + +## Consequences + +- Anchors are correct whenever the model quotes correctly, which is its strong suit. +- Unanchorable findings are still delivered (never silently dropped), marked as such. +- The matcher is whitespace-lenient at line heads only; pathological files with many + identical lines may fall back to line 1 — accepted trade-off. diff --git a/docs/adr/0008-logger-port-with-scoped-children.md b/docs/adr/0008-logger-port-with-scoped-children.md new file mode 100644 index 0000000..8314d0c --- /dev/null +++ b/docs/adr/0008-logger-port-with-scoped-children.md @@ -0,0 +1,30 @@ +# ADR-0008: Logger port with scoped children and a stream channel + +Status: Accepted (2026-06-10) + +## Context + +The reviewer runs in CI where logs are the only observability, and in tests where +log events are assertion targets. LLM token streams should appear live in CI but +would pollute structured sinks. + +## Decision + +A small `Logger` interface (`src/logger/types.ts`): four leveled methods taking +`(event, data?)`, a `child(scope)` deriving a scoped logger (`fetch`, `review.post`, +…), and a separate `stream(chunk)` channel for raw LLM text that structured sinks may +ignore. + +Two sinks: `ConsoleLogger` (pretty, indented, stderr — keeps stdout clean for the +local provider's review output) and `MemoryLogger` (records `LogEntry[]` for tests). + +Event names are dot-scoped snake_case (`llm.heal`, `anchor.unresolved`, +`potential_rename.auto_address`); payload is a flat data object — no string +interpolation into messages. + +## Consequences + +- Tests assert on events structurally instead of regexing console text. +- Swapping/adding sinks (e.g. JSON-lines for log aggregation) touches only `logger/`. +- Stage code reads as instrumented logic: `log.info("done", {...})` with the scope + carried by the child. diff --git a/docs/adr/0009-producer-defines-port-reexports.md b/docs/adr/0009-producer-defines-port-reexports.md new file mode 100644 index 0000000..fbaeff4 --- /dev/null +++ b/docs/adr/0009-producer-defines-port-reexports.md @@ -0,0 +1,31 @@ +# ADR-0009: Wire shapes — producer defines, the port re-exports + +Status: Accepted (2026-06-10) + +## Context + +`ChangedFile` and `PotentialRename` were each defined **twice**, structurally +identically: in `src/lib/git.ts` (the producer of the values) and again in +`src/providers/types.ts` (the port that exposes them). TypeScript's structural typing made +this compile, which is exactly the danger — the copies could drift silently, and a +reader can't tell which one is canonical. + +## Decision + +A wire shape is defined **once, in the module that produces it**, and ports that +expose it **re-export** the type rather than re-declaring it. + +Applied: `ChangedFile` and `PotentialRename` are defined in `lib/git.ts` (where the +parsing that creates them lives, including the field-by-field doc comments) and +re-exported from `providers/types.ts` so provider/stage code keeps importing them from the +port unchanged. + +Dependency direction stays clean: `lib/` knows nothing about `providers/`; `providers/` builds on +`lib/`. + +## Consequences + +- One place to change a shape; impossible drift. +- Import sites are untouched — the port remains the public face. +- Rule of thumb for future types: producer defines, port re-exports, consumers import + from the port. diff --git a/docs/adr/0010-shared-llm-stage-scaffolding.md b/docs/adr/0010-shared-llm-stage-scaffolding.md new file mode 100644 index 0000000..875804f --- /dev/null +++ b/docs/adr/0010-shared-llm-stage-scaffolding.md @@ -0,0 +1,41 @@ +# ADR-0010: Shared scaffolding for LLM-using stages + +Status: Accepted (2026-06-10) + +## Context + +`stages/review/index.ts` and `stages/reconcile/index.ts` each repeated, with small +variations: + +- the prompt-asset loading triple (`readFileSync` of `prompt.md`, `schema.json`, + `JSON.parse`), +- their own `SLOT` tag table (overlapping keys, same values), +- assembly of system sections (prompt → output_schema → project_context → + rendering_context) and base user sections (pr_metadata → pr_diff → pr_comments), +- construction of the post-LLM `StageMetric` from an `LLMResult`. + +A third LLM stage would copy all four again. Drift had already begun (slot tables +diverged by one key each). + +## Decision + +Grow `src/stages/shared.ts` (already the home for cross-stage LLM helpers) with: + +```ts +loadPromptAssets(dir) // → { prompt, schemaText, schema } +SLOT // single tag table for all stages +baseSystemSections(prompt, schemaText, state, opts?) // + optional checklist slot +baseUserSections(state, comments) // meta + diff + comments +llmStageMetric(stage, t0, result) // tokens/heal/tool metric +``` + +Stages keep full control: helpers return arrays the stage may append to (review adds +``; reconcile passes a filtered comment set). The helpers encode +*order and presence rules* (empty sections omitted), not stage policy. + +## Consequences + +- A new LLM stage is its prompt, its schema, and its policy code — no scaffolding. +- Section order and slot naming can no longer drift between stages. +- `stages/shared.ts` is the single place to evolve the prompt envelope (e.g. adding a + global slot) — one edit, all stages. diff --git a/docs/adr/0011-canonical-open-bot-thread-predicate.md b/docs/adr/0011-canonical-open-bot-thread-predicate.md new file mode 100644 index 0000000..28cdac2 --- /dev/null +++ b/docs/adr/0011-canonical-open-bot-thread-predicate.md @@ -0,0 +1,33 @@ +# ADR-0011: Canonical predicate for open bot threads + +Status: Accepted (2026-06-10) + +## Context + +The condition "unresolved bot inline root thread" — +`c.by === "bot" && c.inline && !c.resolved && !c.parentId` — was hand-written in three +places: `cli.ts` (`hasUnresolvedBotThreads`, decides whether reconcile runs), +`reconcile/index.ts` (`splitThreads`, selects judgment candidates), and +`summarize/index.ts` (fallback when reconcile didn't run). These three **must** agree: +if cli's copy says "no open threads" but summarize's copy finds one, the verdict is +wrong. Nothing enforced that agreement. + +## Decision + +Define it once where the comment shape lives: + +```ts +// src/providers/types.ts +export function isOpenBotThread(c: PRComment): boolean; +``` + +All three sites call the predicate. Any future refinement (e.g. ignoring +bot-authored rename notices) is one edit that automatically keeps the pipeline's +gate, the judge's candidate set, and the verdict's fallback consistent. + +## Consequences + +- A semantic invariant ("what counts as an outstanding bot finding") is now code, not + convention. +- Behavior-affecting tweaks to the predicate are visible in one diff hunk and one ADR + amendment, instead of three scattered conditionals. diff --git a/docs/adr/0012-summary-comment-fragment-constants.md b/docs/adr/0012-summary-comment-fragment-constants.md new file mode 100644 index 0000000..5177e9c --- /dev/null +++ b/docs/adr/0012-summary-comment-fragment-constants.md @@ -0,0 +1,30 @@ +# ADR-0012: Shared fragment constants for summary-comment detection + +Status: Accepted (2026-06-10) + +## Context + +`summarize` deletes its own stale summary comments from previous runs before posting +a fresh one. Detection (`isPriorSummary`) matched on a list of literal fragments +(`SUMMARY_FRAGMENTS`) while construction (`buildSummary`) embedded the same phrases +as separate string literals. Nothing tied the two together: rewording a summary line +would silently break deletion, and old summaries would pile up on long-lived PRs — +a drift bug waiting in the gap between two literals. + +## Decision + +Each summary block type gets one exported-nowhere module constant +(`FRAG_UNADDRESSED`, `FRAG_NO_SOURCE`, `FRAG_INFO`); `buildSummary` interpolates the +constant into the text it posts, and `SUMMARY_FRAGMENTS` (the detector's list) is +built from the same constants. Construction and detection can no longer disagree. + +A content-invisible marker (e.g. an HTML comment ``) was +considered and deferred: it is the more robust scheme, but switching detection to it +would orphan summaries posted by already-deployed versions. Fragment constants fix +the drift risk with zero migration. If the marker is ever adopted, detection must +keep matching the legacy fragments for at least one release. + +## Consequences + +- Rewording a summary block is a one-constant edit that keeps deletion working. +- The coupling is now visible in the code's shape instead of being tribal knowledge. diff --git a/docs/adr/0013-adopt-shared-conventions.md b/docs/adr/0013-adopt-shared-conventions.md new file mode 100644 index 0000000..30f904e --- /dev/null +++ b/docs/adr/0013-adopt-shared-conventions.md @@ -0,0 +1,46 @@ +# ADR-0013: Adopt the shared conventions set + +Status: Accepted (2026-06-10) + +## Context + +A full-codebase pattern review settled a set of engineering rules — naming, type +placement, consolidation thresholds, error handling, configuration, logging, +testing, documentation. The rules are project-agnostic and intended for reuse in +other repos (and eventually as machine-readable standards for the reviewer's own +`standardsRoot` input). Embedding them in dated ADR prose made them hard to reuse +and mixed living rules into an immutable log. + +## Decision + +The rules live in [docs/conventions/](../conventions/) — one topic per file, stated +without reference to this codebase wherever possible. This ADR adopts the whole set +for this repository: + +[naming](../conventions/naming.md) · +[types-placement](../conventions/types-placement.md) · +[consolidation](../conventions/consolidation.md) · +[error-handling](../conventions/error-handling.md) · +[configuration](../conventions/configuration.md) · +[logging](../conventions/logging.md) · +[testing](../conventions/testing.md) · +[documentation](../conventions/documentation.md) + +Repo-specific tolerated exceptions under those rules: + +- `providers/local.ts` reads TTY/`NO_COLOR` directly — presentation, not config + (configuration.md's stated exception). +- `providers/mock.ts` warns via `console` — constructed before a logger exists, + test-only. +- `providers/gitlab.ts` / `providers/bitbucket.ts` each own a private `makeApi` + wrapper — deliberate duplication per consolidation.md. +- `logger/index.ts` is a barrel whose contract sits in `logger/types.ts` — aligned + with naming.md rule 5. + +## Consequences + +- Convention changes edit one topic file; this ADR stays valid as the adoption + record. +- Other projects copy `docs/conventions/` and write their own one-page adoption + ADR; deviations are recorded in the adopting repo, never by editing the shared + rule. diff --git a/docs/adr/README.md b/docs/adr/README.md new file mode 100644 index 0000000..3c5f4a9 --- /dev/null +++ b/docs/adr/README.md @@ -0,0 +1,22 @@ +# Architecture Decision Records + +Dated, immutable decision log. Format `NNNN-title.md`; statuses +Proposed → Accepted → Superseded. Current-state rules live in +[../conventions/](../conventions/); the system map in +[../ARCHITECTURE.md](../ARCHITECTURE.md). + +| # | Decision | +| --- | --- | +| [0001](0001-ctx-dependency-injection-and-env-isolation.md) | Ctx DI container; `process.env` read only in `loadConfig` | +| [0002](0002-stage-pipeline-uniform-stagestate.md) | Stage pipeline with uniform `StageState` | +| [0003](0003-provider-port-with-static-async-factories.md) | `Provider` port, static async factories, semantic verdict ops | +| [0004](0004-base-provider-shared-git-plumbing.md) | `BaseProvider`: all diffs from local git, shared plumbing | +| [0005](0005-llm-client-loop-with-schema-healing.md) | LLM client loop: tools, JSON salvage, schema healing | +| [0006](0006-prompt-asset-colocation-and-tagged-sections.md) | Prompt/schema colocated per stage; tagged prompt sections | +| [0007](0007-snippet-anchoring-over-line-numbers.md) | Snippet anchoring instead of LLM line numbers | +| [0008](0008-logger-port-with-scoped-children.md) | Logger port with scoped children + stream channel | +| [0009](0009-producer-defines-port-reexports.md) | Wire shapes defined by producer, re-exported by the port | +| [0010](0010-shared-llm-stage-scaffolding.md) | Shared LLM-stage scaffolding (assets, slots, sections, metrics) | +| [0011](0011-canonical-open-bot-thread-predicate.md) | Canonical `isOpenBotThread` predicate | +| [0012](0012-summary-comment-fragment-constants.md) | Shared fragment constants for summary detection | +| [0013](0013-adopt-shared-conventions.md) | Adopt `docs/conventions/` (naming, types placement, consolidation, errors, config, logging, testing, docs) | diff --git a/docs/conventions/README.md b/docs/conventions/README.md new file mode 100644 index 0000000..1c0f61a --- /dev/null +++ b/docs/conventions/README.md @@ -0,0 +1,27 @@ +# Conventions + +Portable engineering rules — deliberately **project-agnostic** so they can be copied +into any repo (or ingested into a standards knowledgebase). Each file is one topic: +the rule, why it exists, and how to apply it, with examples. + +This repo's instance-specific documentation lives in +[../ARCHITECTURE.md](../ARCHITECTURE.md); the dated decision log in [../adr/](../adr/) +(ADR-0013 adopts this folder by reference). + +| Topic | Rule in one line | +| --- | --- | +| [naming.md](naming.md) | Name modules and types by role, not implementation; the bells test gates every name | +| [types-placement.md](types-placement.md) | `types.ts` holds vocabulary (promotion test: 2+ importers / boundary / family); `shared.ts` holds cross-cutting helpers; everything else colocates | +| [consolidation.md](consolidation.md) | Extract only essential duplication or near-universal sharing; 2-of-N look-alikes stay duplicated | +| [error-handling.md](error-handling.md) | Normalize unknown throws; context-prefixed messages; per-item catch in batches | +| [configuration.md](configuration.md) | One env read point; typed config; exported defaults; fail fast with actionable hints | +| [logging.md](logging.md) | Structured events with dot-scoped children; data objects, never interpolation | +| [testing.md](testing.md) | Mock at the port with side-effect recorders; real engines; structural assertions | +| [documentation.md](documentation.md) | Three doc layers (map / conventions / ADRs); Mermaid for diagrams; why-comments | + +## Adopting in a new project + +Copy the folder, delete topics that don't apply, add an ADR in the target repo +recording the adoption. Rules here are stated without reference to this codebase's +modules wherever possible; examples cite generic shapes. When a project must deviate, +record the deviation in that project's ADR — don't edit the convention to fit. diff --git a/docs/conventions/configuration.md b/docs/conventions/configuration.md new file mode 100644 index 0000000..b0214ce --- /dev/null +++ b/docs/conventions/configuration.md @@ -0,0 +1,27 @@ +# Configuration + +**Rules.** + +1. **One env read point.** `process.env` (or the platform equivalent) is read in + exactly one loader function. Everything downstream consumes a typed config + object. A new setting = one typed field + a default + one read in the loader — + never a scattered `process.env.X`. +2. **Precedence is explicit:** CLI flag > environment > default, implemented once in + the loader, not re-decided per call site. +3. **Defaults are exported** so tests build their configs from the real defaults and + override only what the test exercises. A test that hand-retypes the default + config silently stops matching production the day a default changes. +4. **Parse helpers, not ad-hoc coercion** — `asNumber(v, fallback)`, + `asBool(v)` ("1"/"true"), `pick(...candidates)`. One definition of what counts + as true. +5. **Validate per selected mode.** Only the credentials/settings the chosen mode + needs are required; validation messages say exactly what to set and how. +6. **Safety gates are environmental, not flags.** When a mode is dangerous unless + sandboxed, detect the sandbox by artifacts the runtime creates (e.g. + `/.dockerenv`), never by an env variable any caller can export. Same principle + for identity: derive "who am I" from the credential (`GET /user`), not from + CI-shaped environment hints. + +**Tolerated exception:** presentation concerns (TTY detection, `NO_COLOR`) may read +the environment directly at the presentation site — they are output formatting, not +configuration. diff --git a/docs/conventions/consolidation.md b/docs/conventions/consolidation.md new file mode 100644 index 0000000..a685276 --- /dev/null +++ b/docs/conventions/consolidation.md @@ -0,0 +1,34 @@ +# Consolidation: essential vs. incidental duplication + +**Rule.** Extract shared code only when at least one of these holds; otherwise leave +the copies with their owners. + +1. **Essential duplication** — the copies must stay in sync *for correctness*; + divergence is a bug, not an evolution. (A scope-join format every log sink must + produce identically; a predicate three call sites must agree on or a verdict goes + wrong; a prompt envelope two pipeline stages must emit identically.) +2. **Universal sharing** — all (or all-but-justified-exceptions) of the sites share + it, and the shared concept is stable. + +Do **not** extract *incidental* duplication — code that looks alike today because +both instances are currently simple, with no requirement to stay alike. Two REST +wrappers that differ only in an auth header owe each other nothing: pagination, +rate limits, and auth refresh will diverge per service. + +## The failure mode this prevents + +A shared helper extracted from a minority of sites (the "2 of 8" case) has two +futures once one consumer's needs change: it grows option flags for every variant, +or the consumer forks away and orphans a "shared" module with a single user. Both +outcomes are worse than the duplicated lines were. + +## How to apply + +- Before extracting, ask: *must* these stay identical, or do they merely *happen* to + be identical? Only the first justifies coupling. +- Tolerated duplication gets a comment at the site marking it deliberate (and + pointing at this rule), so a future cleanup pass doesn't re-couple it. +- Revisit when a third consumer appears with the same needs (rule of three) — and + then re-check criterion 1, not just the count. +- The same logic governs types — see [types-placement.md](types-placement.md): + shared-by-many promotes, used-by-one stays local. diff --git a/docs/conventions/documentation.md b/docs/conventions/documentation.md new file mode 100644 index 0000000..6389587 --- /dev/null +++ b/docs/conventions/documentation.md @@ -0,0 +1,32 @@ +# Documentation + +**Three layers, separated by lifecycle:** + +| Layer | File(s) | Tense / lifecycle | +| --- | --- | --- | +| Map | `docs/ARCHITECTURE.md` | present tense; updated with the code; instance-specific | +| Rules | `docs/conventions/` | present tense; portable across projects; one topic per file | +| Decisions | `docs/adr/` | dated, immutable log; statuses Proposed → Accepted → Superseded | + +The separation exists because mixing them corrupts both: rules buried in dated +records go stale-looking the moment a supersession lands; history rewritten into +"current state" docs loses the why. ADRs record *that and why* something was +adopted; conventions state *what the rule is now*; the map shows *where things are*. + +**Rules.** + +1. **Diagrams are Mermaid**, never ASCII art — they render on the hosting platform + and in IDEs, and survive edits. Quote node labels containing `?`, `/`, or + `
` (flowchart parser traps). +2. **ADR hygiene:** one decision per ADR; supersede, don't rewrite (exception: a + never-published doc set may be cleaned wholesale — history that was never shared + isn't history). Convention adoption is one ADR pointing at `conventions/`, not a + restatement. +3. **File-header comments.** Every source file opens with a comment stating its role + and any non-obvious contract. Inline comments explain *why* and contract edges — + never what the next line does, and never why a change was correct (that's + reviewer-talk, noise after merge). +4. **Heuristics carry their rationale inline** — a threshold or magic decision in + code states why that value, right where it's read. +5. **README is the operator's view** (install, run, configure); architecture detail + links out to the map rather than duplicating it. diff --git a/docs/conventions/error-handling.md b/docs/conventions/error-handling.md new file mode 100644 index 0000000..1684780 --- /dev/null +++ b/docs/conventions/error-handling.md @@ -0,0 +1,35 @@ +# Error handling + +**Rules.** + +1. **Normalize unknown throws.** `catch (e)` binds `unknown`; `(e as Error).message` + is a cast, not a check — it yields `undefined` when a non-Error was thrown, and + the log line silently loses its cause. Every catch site goes through one helper: + + ```ts + export function errorMessage(e: unknown): string { + if (e instanceof Error) return e.message; + if (typeof e === "string") return e; + try { return JSON.stringify(e); } catch { return String(e); } + } + ``` + + The `(e as Error)` cast is banned; grep for it should return nothing. + +2. **Context-prefixed messages.** Throw `Error` with `context: detail` + ("`GitLab API 403: …`", "`missing LLM_URL. Pass --llm-url or set LLM_URL.`"). + The message must let the operator act without a stack trace. + +3. **Per-item catch in batches.** When processing N independent items (posting N + comments, resolving N threads), one failure is caught, logged with its item's + identity, counted — and the loop continues. One bad item must not abort the run. + +4. **Fail fast at startup.** Validate configuration/credentials at load time with + actionable hints, before any network or work happens. + +5. **Exit codes are the contract** for CLI tools running as gates: success / policy + failure / usage error get distinct codes; logs explain, codes decide. + +6. **Error classes are named `Error`** and reserved for failures a caller might + meaningfully catch by type; everything else throws plain `Error` with a good + message. diff --git a/docs/conventions/logging.md b/docs/conventions/logging.md new file mode 100644 index 0000000..17bf9c4 --- /dev/null +++ b/docs/conventions/logging.md @@ -0,0 +1,22 @@ +# Logging + +**Rules.** + +1. **Structured events, not sentences.** Log calls are `log.level(event, data?)` + where `event` is dot-scoped snake_case (`anchor.unresolved`, `llm.heal`) and + `data` is a flat object. Never interpolate values into the event string — sinks + and tests need the fields, not a sentence to regex. +2. **Scoped children.** A component takes a logger and derives its scope via + `child(scope)`; scopes join with dots (`review.post`). The join format is defined + once and shared by every sink (essential duplication — see + [consolidation.md](consolidation.md)). +3. **Logger is a port.** Components depend on the interface; sinks are adapters + (console for humans/CI, in-memory for tests). Swapping or adding a sink touches + only the logger folder. +4. **Raw streams get their own channel.** Live token/progress streams go through a + dedicated `stream(chunk)` method that structured sinks may ignore — they must not + pollute the event stream. +5. **Logs to stderr, payload to stdout** for CLI tools whose stdout is consumed + (piped output, machine-read results). +6. **Tests assert on entries, not rendered text** — the in-memory sink records + `{level, scope, event, data}`; assertions target those fields structurally. diff --git a/docs/conventions/naming.md b/docs/conventions/naming.md new file mode 100644 index 0000000..184ed02 --- /dev/null +++ b/docs/conventions/naming.md @@ -0,0 +1,40 @@ +# Naming: by role, not implementation + +**Rule.** A name must answer the question a reader has at the *use site* — what role +does this thing play — not record how it happens to be implemented. + +**The bells test.** If a name makes the reader ask a question it should have answered +("what's git-backed?", "is this the client or the transport?"), rename it before +merging. Both directions count: a name must not raise false expectations either +(`listen` implies subscription semantics; an awaited one-shot consumer is `receive`). + +## Rules + +1. **Ports are bare domain nouns** — `Provider`, `Logger`. No qualifier that names the + technology behind them (a port named after its implementation can't absorb a second + implementation). +2. **Shared base classes are `Base` in `base.ts`** — `BaseProvider`. The base is + named for its role (the thing adapters extend), not for what it shares (e.g. not + `GitBackedProvider` — implementation rationale belongs in the class comment). +3. **Adapters are ``** — `GitHubProvider`, `MockProvider`, `ConsoleLogger`. + No extra infixes; the test double follows the same pattern as real adapters. +4. **Within a subsystem, the consumer-facing API owns the `client` name**; wire + plumbing is `transport`, with directional verbs `send` / `receive` (the canonical + channel pair — never HTTP verbs like `postX`, never `listen` for an awaited + one-shot). A reader opening `client.ts` must find the thing they call. +5. **Contracts live in `types.ts`** (see [types-placement.md](types-placement.md)); + `shared.ts` is reserved for genuine cross-cutting helper collections; `index.ts` is + only ever a folder's public face — a barrel or a registry. +6. **Error subclasses are `Error`** (`LLMValidationError`), never sentence + fragments (`Failed`). +7. **One-function consumer-facing modules fold into the client module** rather than + existing as a file whose name must be guessed. +8. **File names match their primary export** where a file has one + (`model.ts` ↔ `resolveModel`); multi-export files are named for their role. + +## Why + +Import sites become role statements: `from "../llm/client.ts"` means "I call the +LLM"; `extends BaseProvider` means "I am a provider". Implementation-flavored names +rot the moment the implementation shifts, and mislead every reader until someone +pays to rename them. diff --git a/docs/conventions/testing.md b/docs/conventions/testing.md new file mode 100644 index 0000000..a211430 --- /dev/null +++ b/docs/conventions/testing.md @@ -0,0 +1,24 @@ +# Testing + +**Rules.** + +1. **Mock at the port, record side effects.** The test double implements the same + port as real adapters and records every write (`postedInline`, `verdicts`, …) for + assertions. Don't stub internals; swap the adapter. +2. **Use the real engine where one exists.** Don't reimplement diff/rename logic (or + any engine) in the mock — drive the real tool against fixture data (e.g. build a + throwaway git repo from `old/` + `new/` trees and let `git -M` do rename + detection). The mock fakes the *destination*, not the *mechanics*. +3. **Structural assertions over text matching.** Assert facts the pipeline + guarantees (file landed in the right bucket, every blocker was posted inline, the + thread was resolved) rather than regexing free-form output. With LLMs in the loop + this is mandatory: assert on what the system *did* with the output, not the + output's wording. +4. **Deterministic paths get deterministic tests.** Where a pipeline has + deterministic short-circuits, test those without the expensive/flaky dependency; + reserve end-to-end-with-model tests for genuinely model-dependent judgments, and + keep their assertions tolerant of model variance. +5. **Test configs come from production defaults** — spread the exported defaults and + override the field under test (see [configuration.md](configuration.md) rule 3). +6. **Fixtures are data, not code** — scenario folders with declarative config; adding + a case means adding files, not test logic. diff --git a/docs/conventions/types-placement.md b/docs/conventions/types-placement.md new file mode 100644 index 0000000..ed296cc --- /dev/null +++ b/docs/conventions/types-placement.md @@ -0,0 +1,60 @@ +# Types placement: `types.ts` holds vocabulary + +**Rule.** Every folder with a shared contract has a `types.ts` — but it holds +*vocabulary* (names multiple modules must speak), not every type in the folder. +Small domain functions that belong to the contract are welcome there (URL dispatch, +canonical predicates). + +## Promotion test — a type moves to `types.ts` when ANY of: + +1. **Two-importer rule** — imported *by name* from 2+ modules in the folder. +2. **Boundary rule** — imported from outside the folder; it is the folder's public + contract. +3. **Family rule** — structurally connected to types that qualify above; families + move together (a request root type rides with the message/tool-call shapes it + composes — splitting one vocabulary across files scatters it). + +## Stays beside its behavior when BOTH hold: + +1. **No name consumers** — its only use is structural: a parameter type checked via + object literals, an inferred return type, a throw nobody catches by type, or an + unexported internal. +2. **Single-owner change cycle** — it describes one function's surface or internals + and should change freely whenever that function does. + +## Why + +Placement communicates **blast radius**. `types.ts` says "shared and stable — +changing me ripples outward"; colocation says "mine — changes freely with its +function". Filing single-owner types in `types.ts` breaks both signals: readers need +two files to understand one function, editors treat local changes with false +caution, and the contract file degrades into a junk drawer that no longer answers +"what is this subsystem's vocabulary". + +Note the parameter/return asymmetry: callers pass object literals (parameter types +acquire no import sites) but *name* result types when passing them onward — so a +function's result type often promotes while its args type stays put. That's the +rule working, not an inconsistency. + +## Where functions live: `types.ts` vs `shared.ts` vs colocation + +The same promotion logic governs functions, with one extra distinction — does the +function define *meaning* or do *work*? + +- **`types.ts`** also holds small functions that are part of the vocabulary: they + define what a domain term means (a canonical predicate like "open bot thread", a + URL→implementation dispatch, a scope-join format every sink must reproduce). + Changing one changes the *meaning* of the contract, so it lives with the contract. +- **`shared.ts`** holds cross-cutting *helpers* used by multiple sibling modules: + multi-step assembly and orchestration work (build the prompt envelope, load + colocated assets, construct a metric from a result). They encode shared mechanics, + not domain meaning. A contract is **never** placed in `shared.ts`, and a folder + that has no such helpers has no `shared.ts`. +- **Colocation** — a helper with a single consumer stays in (or next to) that + consumer, exactly like a single-owner type. It promotes to `shared.ts` only when a + second sibling genuinely needs it (see [consolidation.md](consolidation.md) — + essential or universal, not incidental). + +Litmus: if rewording the function would change what a domain statement *means*, +it's vocabulary → `types.ts`. If it would only change *how* work gets done, it's a +helper → `shared.ts` (if shared) or colocated (if not). diff --git a/package.json b/package.json index 9d4fb70..409c307 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@bilikaz/code-reviewer", - "version": "0.1.2", + "version": "0.2.0", "description": "LLM-driven PR reviewer. Pluggable VCS destinations. OpenAI-compatible LLM API.", "type": "module", "packageManager": "pnpm@10.33.0", diff --git a/src/cli.ts b/src/cli.ts index da0da26..76e891e 100755 --- a/src/cli.ts +++ b/src/cli.ts @@ -21,11 +21,13 @@ // --llm-model overrides LLM_MODEL (otherwise auto-detected) import { buildCtx, type CliOverrides, type Ctx } from "./ctx.ts"; +import { errorMessage } from "./lib/errors.ts"; import { runFetch } from "./stages/fetch/index.ts"; import { runReconcile } from "./stages/reconcile/index.ts"; import { runReview } from "./stages/review/index.ts"; import { runSummarize } from "./stages/summarize/index.ts"; import type { StageState } from "./stages/types.ts"; +import { isOpenBotThread } from "./providers/types.ts"; // ---- Argv parser -------------------------------------------------------- @@ -83,9 +85,7 @@ function buildOverrides(flags: Record): CliOverrides { // ---- Pipeline orchestration -------------------------------------------- function hasUnresolvedBotThreads(state: StageState): boolean { - return state.comments.some( - (c) => c.by === "bot" && c.inline && !c.resolved && !c.parentId, - ); + return state.comments.some(isOpenBotThread); } async function runCommand(command: string, ctx: Ctx): Promise { @@ -129,6 +129,6 @@ try { const state = await runCommand(command, ctx); if (state.verdict?.fatal) process.exit(1); } catch (e) { - ctx.logger.error("cli.fatal", { error: (e as Error).message }); + ctx.logger.error("cli.fatal", { error: errorMessage(e) }); process.exit(1); } diff --git a/src/ctx.ts b/src/ctx.ts index 2ecac82..c22a878 100644 --- a/src/ctx.ts +++ b/src/ctx.ts @@ -1,7 +1,7 @@ // Ctx — dependency-injection container. Built once at CLI entry, passed to // every stage. Holds: // - config: immutable settings (all env reads + CLI overrides live here) -// - vcs: instantiated provider (already authenticated) +// - provider: instantiated review-platform adapter (already authenticated) // - logger: structured logger (swap sinks here) // // Single rule: process.env is read ONLY in loadConfig(). Everything @@ -13,14 +13,14 @@ import { existsSync } from "node:fs"; import { ConsoleLogger } from "./logger/console.ts"; import type { Logger } from "./logger/index.ts"; -import { resolveModel } from "./llm/model.ts"; -import { BitbucketProvider } from "./vcs/bitbucket.ts"; -import { GitHubProvider } from "./vcs/github.ts"; -import { GitLabProvider } from "./vcs/gitlab.ts"; -import { LocalVcsProvider } from "./vcs/local.ts"; -import { MockVcsProvider } from "./vcs/mock.ts"; -import type { ProviderKind, VcsProvider } from "./vcs/types.ts"; -import { detectProvider } from "./vcs/types.ts"; +import { resolveModel } from "./llm/client.ts"; +import { BitbucketProvider } from "./providers/bitbucket.ts"; +import { GitHubProvider } from "./providers/github.ts"; +import { GitLabProvider } from "./providers/gitlab.ts"; +import { LocalProvider } from "./providers/local.ts"; +import { MockProvider } from "./providers/mock.ts"; +import type { ProviderKind, Provider } from "./providers/types.ts"; +import { detectProvider } from "./providers/types.ts"; // ---- Config shape ------------------------------------------------------- @@ -75,7 +75,7 @@ export interface Config { export interface Ctx { config: Config; - vcs: VcsProvider; + provider: Provider; logger: Logger; } @@ -92,7 +92,9 @@ export interface CliOverrides { // ---- Defaults ------------------------------------------------------------ -const REVIEW_DEFAULTS: ReviewConfig = { +// Exported so tests build their configs from the real defaults instead of +// re-typing them (see docs/conventions/configuration.md). +export const REVIEW_DEFAULTS: ReviewConfig = { diffFilter: { includeExtensions: [], fullFileThresholdLines: 800, @@ -121,6 +123,10 @@ function asNumber(v: string | undefined, fallback: number): number { return Number.isFinite(n) ? n : fallback; } +function asBool(v: string | undefined): boolean { + return v === "1" || (v ?? "").toLowerCase() === "true"; +} + function requireField(value: string, name: string, hint: string): string { if (!value) throw new Error(`missing ${name}. ${hint}`); return value; @@ -158,7 +164,7 @@ export function loadConfig(opts: CliOverrides): Config { // models will simply finish well under the cap. maxOutputTokens: asNumber(e.LLM_MAX_OUTPUT_TOKENS, 32_768), healRetries: asNumber(e.LLM_HEAL_RETRIES, 2), - debug: pick(e.LLM_DEBUG) === "1" || pick(e.LLM_DEBUG).toLowerCase() === "true", + debug: asBool(e.LLM_DEBUG), }, github: { token: pick(e.GITHUB_TOKEN, e.GH_TOKEN) }, gitlab: { token: pick(e.GITLAB_TOKEN), baseUrl: pick(e.GITLAB_BASE_URL) }, @@ -202,13 +208,13 @@ export function loadConfig(opts: CliOverrides): Config { // ---- VCS construction (plain dispatch — no env reads) ------------------- -function makeVcs(c: Config): Promise { +function makeProvider(c: Config): Promise { switch (c.pr.provider) { case "github": return GitHubProvider.create(c); case "gitlab": return GitLabProvider.create(c); case "bitbucket": return BitbucketProvider.create(c); - case "mock": return MockVcsProvider.create(c); - case "local": return LocalVcsProvider.create(c); + case "mock": return MockProvider.create(c); + case "local": return LocalProvider.create(c); } } @@ -216,8 +222,8 @@ function makeVcs(c: Config): Promise { export async function buildCtx(opts: CliOverrides): Promise { const config = loadConfig(opts); - const [vcs] = await Promise.all([ - makeVcs(config), + const [provider] = await Promise.all([ + makeProvider(config), (async () => { if (!config.llm.model) { config.llm.model = await resolveModel({ @@ -230,7 +236,7 @@ export async function buildCtx(opts: CliOverrides): Promise { return { config, - vcs, + provider, logger: opts.logger ?? new ConsoleLogger(), }; } diff --git a/src/lib/errors.ts b/src/lib/errors.ts new file mode 100644 index 0000000..5513e46 --- /dev/null +++ b/src/lib/errors.ts @@ -0,0 +1,14 @@ +// Error-message normalization (see docs/conventions/error-handling.md). `catch (e)` binds unknown; +// `(e as Error).message` lies when a non-Error was thrown (yields +// undefined and the log line loses its cause). Every catch site that +// needs the message goes through this helper instead. + +export function errorMessage(e: unknown): string { + if (e instanceof Error) return e.message; + if (typeof e === "string") return e; + try { + return JSON.stringify(e); + } catch { + return String(e); + } +} diff --git a/src/llm/call.ts b/src/llm/call.ts deleted file mode 100644 index b4fb1c8..0000000 --- a/src/llm/call.ts +++ /dev/null @@ -1,225 +0,0 @@ -// One LLM "call" per stage. Internally a loop: stream a chat completion → -// if it returned tool_calls, execute them locally and feed the results back, -// then re-stream. When the LLM returns final content (no tool_calls), parse -// the JSON, validate against the schema, and either return or heal. - -import { Ajv } from "ajv"; - -import type { Ctx } from "../ctx.ts"; -import type { ChatMessage } from "./client.ts"; -import { postChat } from "./client.ts"; -import { readStream } from "./stream.ts"; -import { execTool, TOOL_SCHEMAS } from "./tools/index.ts"; - -export type Section = { tag: string; content: string }; - -export interface LLMResult { - validated: T; - usage: { promptTokens: number; completionTokens: number; totalTokens: number }; - healAttempts: number; - toolCalls: number; - rawText: string; -} - -export class LLMValidationFailed extends Error {} - -function serializeSections(sections: Section[]): string { - const parts: string[] = []; - for (const { tag, content } of sections) { - const body = content.trimEnd(); - if (!tag) parts.push(body); - else parts.push(`<${tag}>\n${body}\n`); - } - return parts.join("\n\n"); -} - -const JSON_FENCE_RE = /```(?:json)?\s*\n?([\s\S]*?)```/g; - -interface Extracted { - value: unknown; - // true → a whole candidate parsed cleanly (trustworthy). - // false → only a sub-span parsed; the surrounding JSON was malformed, so the - // value is a salvaged fragment and likely doesn't match the schema. - clean: boolean; -} - -function extractJson(text: string): Extracted { - const candidates: string[] = []; - for (const m of text.matchAll(JSON_FENCE_RE)) candidates.push(m[1]!.trim()); - candidates.push(text.trim()); - let parseError = ""; - for (const c of candidates) { - try { return { value: JSON.parse(c), clean: true }; } - catch (e) { if (!parseError) parseError = (e as Error).message; } - } - // Longest-span scan: shrink suffixes until JSON.parse succeeds. This only - // runs when no whole candidate parsed — i.e. the JSON is malformed — so a - // hit here is a salvaged fragment, not the intended object. - let best: { span: number; value: unknown } | null = null; - for (const opener of ["{", "["]) { - let i = text.indexOf(opener); - while (i >= 0) { - for (let end = text.length; end > i + 1; end--) { - try { - const v = JSON.parse(text.slice(i, end)); - const span = end - i; - if (!best || span > best.span) best = { span, value: v }; - break; - } catch { /* shrink */ } - } - i = text.indexOf(opener, i + 1); - } - } - if (best) return { value: best.value, clean: false }; - throw new Error(`response is not valid JSON: ${parseError}`); -} - -function healPrompt(error: string): string { - // Tailor the instruction to the actual failure. A bad-JSON error and a - // schema error need different fixes — telling a model with malformed JSON to - // "fix the schema" sends it down the wrong path. - const jsonError = error.startsWith("invalid JSON") || error.startsWith("JSON parse"); - const fix = jsonError - ? "The cause is malformed JSON — almost always an unescaped double quote, or a raw newline, " + - "inside a string value. Re-emit the SAME findings, but escape every double quote inside a " + - "string as \\\" and every newline as \\n so the whole thing is valid JSON." - : "Your output was valid JSON but did not match the required schema. Re-emit the SAME findings " + - "as an object with exactly the schema's required properties and no extra ones."; - return [ - "Your previous message (directly above) was rejected — it did not produce usable output.", - "", - ` Why it failed: ${error}`, - "", - fix, - "Output the corrected JSON only — no prose, no markdown fences.", - ].join("\n"); -} - -export interface CallLLMArgs { - ctx: Ctx; - stage: string; // logger scope name - systemSections: Section[]; - userSections: Section[]; - schema: object; - // When true, expose Read + Bash tools. Default true. - tools?: boolean; -} - -export async function callLLM(args: CallLLMArgs): Promise> { - const { ctx } = args; - const log = ctx.logger.child(args.stage); - - // validateSchema: false skips draft-07 meta-schema verification. We ship - // the schemas; we don't need ajv to verify their grammar at runtime. - const ajv = new Ajv({ allErrors: true, strict: false, validateSchema: false }); - const validate = ajv.compile(args.schema); - - const system = serializeSections(args.systemSections); - const user = serializeSections(args.userSections); - - const messages: ChatMessage[] = [ - { role: "system", content: system }, - { role: "user", content: user }, - ]; - - const { baseUrl, apiKey, model, healRetries, temperature, maxOutputTokens } = ctx.config.llm; - const useTools = args.tools !== false; - - let healAttempts = 0; - let toolCalls = 0; - let lastError: string | null = null; - let lastText = ""; - let totalPrompt = 0, totalCompletion = 0, totalAll = 0; - - for (let attempt = 0; attempt <= healRetries; attempt++) { - let finalText = ""; - while (true) { - const promptBytes = messages.reduce( - (n, m) => n + (typeof m.content === "string" ? m.content.length : 0), - 0, - ); - log.info("llm.request", { model, attempt, prompt_bytes: promptBytes, messages: messages.length }); - const req = { - model, - messages, - tools: useTools ? TOOL_SCHEMAS : undefined, - tool_choice: useTools ? "auto" as const : undefined, - temperature: Number.isFinite(temperature) ? temperature : 0.2, - max_tokens: maxOutputTokens, - stream: true, - stream_options: { include_usage: true }, - }; - // LLM_DEBUG=1 dumps the full JSON body (no URL, no headers, no - // credentials). Lets the operator verify what the LLM actually sees. - if (ctx.config.llm.debug) log.debug("llm.request.body", { body: req }); - const resp = await postChat({ baseUrl, apiKey, req }); - const msg = await readStream(resp, { onText: (s) => ctx.logger.stream(s) }); - if (msg.usage) { - totalPrompt += msg.usage.prompt_tokens; - totalCompletion += msg.usage.completion_tokens; - totalAll += msg.usage.total_tokens; - } - - if (msg.tool_calls.length > 0) { - messages.push({ role: "assistant", content: msg.content || null, tool_calls: msg.tool_calls }); - for (const tc of msg.tool_calls) { - toolCalls++; - log.info("tool.call", { name: tc.function.name, args_preview: tc.function.arguments.slice(0, 200) }); - const result = await execTool(tc); - messages.push({ role: "tool", content: result.output, tool_call_id: tc.id }); - } - continue; - } - - finalText = msg.content; - lastText = finalText; - break; - } - - let extracted: Extracted | null = null; - try { - extracted = extractJson(finalText); - } catch (e) { - lastError = `JSON parse: ${(e as Error).message}`; - } - if (extracted) { - if (validate(extracted.value)) { - log.info("llm.done", { - prompt_tokens: totalPrompt, - completion_tokens: totalCompletion, - heal_attempts: healAttempts, - tool_calls: toolCalls, - }); - return { - validated: extracted.value as T, - usage: { promptTokens: totalPrompt, completionTokens: totalCompletion, totalTokens: totalAll }, - healAttempts, - toolCalls, - rawText: lastText, - }; - } - // A clean parse that fails the schema is a genuine structure problem. - // But if we only salvaged a fragment (clean=false), the JSON itself was - // malformed — the schema error is misleading (it's validating a recovered - // sub-object). Heal on the real cause: invalid JSON, usually an unescaped - // quote or newline inside a string value. - lastError = extracted.clean - ? `schema: ${ajv.errorsText(validate.errors, { separator: "; " })}` - : `invalid JSON: your output did not parse as a whole — only a fragment was recoverable. ` + - `This is almost always an unescaped " or a raw newline inside a string value. ` + - `Escape every double quote as \\" and every newline as \\n.`; - } - - if (attempt < healRetries) { - healAttempts++; - log.warn("llm.heal", { attempt: healAttempts, reason: lastError ?? "unknown" }); - messages.push({ role: "assistant", content: finalText }); - messages.push({ role: "user", content: healPrompt(lastError ?? "unknown") }); - } - } - - log.error("llm.exhausted", { heal_retries: healRetries, last_error: lastError }); - throw new LLMValidationFailed( - `stage=${args.stage} exhausted ${healRetries} heal attempts; last: ${lastError}`, - ); -} diff --git a/src/llm/client.ts b/src/llm/client.ts index f04a5db..d29eebe 100644 --- a/src/llm/client.ts +++ b/src/llm/client.ts @@ -1,51 +1,258 @@ -// Minimal LLM transport. POSTs to an OpenAI-compatible /chat/completions -// endpoint, returns the raw Response. SSE parsing lives in stream.ts. +// The LLM client — everything consumers call. Two entry points: +// +// callLLM() — one "call" per stage. Internally a loop: stream a chat +// completion → if it returned tool_calls, execute them +// locally and feed the results back, then re-stream. When +// the LLM returns final content (no tool_calls), parse the +// JSON, validate against the schema, and either return or +// heal. +// resolveModel() — model auto-detection via GET /models (used by buildCtx +// when no model is configured). +// +// Wire-level plumbing lives below this file in transport.ts (message types, +// HTTP POST, SSE parsing). -export interface ChatRequest { - model: string; - messages: ChatMessage[]; - tools?: ToolSchema[]; - tool_choice?: "auto" | "required" | "none"; - temperature?: number; - max_tokens?: number; - stream?: boolean; - stream_options?: { include_usage?: boolean }; +import { Ajv } from "ajv"; + +import type { Ctx } from "../ctx.ts"; +import { errorMessage } from "../lib/errors.ts"; +import { receive, send } from "./transport.ts"; +import { execTool, TOOL_SCHEMAS } from "./tools/index.ts"; +import type { ChatMessage, LLMResult, Section } from "./types.ts"; + +export class LLMValidationError extends Error {} + +function serializeSections(sections: Section[]): string { + const parts: string[] = []; + for (const { tag, content } of sections) { + const body = content.trimEnd(); + if (!tag) parts.push(body); + else parts.push(`<${tag}>\n${body}\n`); + } + return parts.join("\n\n"); +} + +const JSON_FENCE_RE = /```(?:json)?\s*\n?([\s\S]*?)```/g; + +interface Extracted { + value: unknown; + // true → a whole candidate parsed cleanly (trustworthy). + // false → only a sub-span parsed; the surrounding JSON was malformed, so the + // value is a salvaged fragment and likely doesn't match the schema. + clean: boolean; } -export type ChatMessage = - | { role: "system" | "user"; content: string } - | { role: "assistant"; content: string | null; tool_calls?: ToolCall[] } - | { role: "tool"; content: string; tool_call_id: string }; +function extractJson(text: string): Extracted { + const candidates: string[] = []; + for (const m of text.matchAll(JSON_FENCE_RE)) candidates.push(m[1]!.trim()); + candidates.push(text.trim()); + let parseError = ""; + for (const c of candidates) { + try { return { value: JSON.parse(c), clean: true }; } + catch (e) { if (!parseError) parseError = errorMessage(e); } + } + // Longest-span scan: shrink suffixes until JSON.parse succeeds. This only + // runs when no whole candidate parsed — i.e. the JSON is malformed — so a + // hit here is a salvaged fragment, not the intended object. + let best: { span: number; value: unknown } | null = null; + for (const opener of ["{", "["]) { + let i = text.indexOf(opener); + while (i >= 0) { + for (let end = text.length; end > i + 1; end--) { + try { + const v = JSON.parse(text.slice(i, end)); + const span = end - i; + if (!best || span > best.span) best = { span, value: v }; + break; + } catch { /* shrink */ } + } + i = text.indexOf(opener, i + 1); + } + } + if (best) return { value: best.value, clean: false }; + throw new Error(`response is not valid JSON: ${parseError}`); +} -export interface ToolSchema { - type: "function"; - function: { name: string; description: string; parameters: object }; +function healPrompt(error: string): string { + // Tailor the instruction to the actual failure. A bad-JSON error and a + // schema error need different fixes — telling a model with malformed JSON to + // "fix the schema" sends it down the wrong path. + const jsonError = error.startsWith("invalid JSON") || error.startsWith("JSON parse"); + const fix = jsonError + ? "The cause is malformed JSON — almost always an unescaped double quote, or a raw newline, " + + "inside a string value. Re-emit the SAME findings, but escape every double quote inside a " + + "string as \\\" and every newline as \\n so the whole thing is valid JSON." + : "Your output was valid JSON but did not match the required schema. Re-emit the SAME findings " + + "as an object with exactly the schema's required properties and no extra ones."; + return [ + "Your previous message (directly above) was rejected — it did not produce usable output.", + "", + ` Why it failed: ${error}`, + "", + fix, + "Output the corrected JSON only — no prose, no markdown fences.", + ].join("\n"); } -export interface ToolCall { - id: string; - type: "function"; - function: { name: string; arguments: string }; +export interface CallLLMArgs { + ctx: Ctx; + stage: string; // logger scope name + systemSections: Section[]; + userSections: Section[]; + schema: object; + // When true, expose Read + Bash tools. Default true. + tools?: boolean; +} + +export async function callLLM(args: CallLLMArgs): Promise> { + const { ctx } = args; + const log = ctx.logger.child(args.stage); + + // validateSchema: false skips draft-07 meta-schema verification. We ship + // the schemas; we don't need ajv to verify their grammar at runtime. + const ajv = new Ajv({ allErrors: true, strict: false, validateSchema: false }); + const validate = ajv.compile(args.schema); + + const system = serializeSections(args.systemSections); + const user = serializeSections(args.userSections); + + const messages: ChatMessage[] = [ + { role: "system", content: system }, + { role: "user", content: user }, + ]; + + const { baseUrl, apiKey, model, healRetries, temperature, maxOutputTokens } = ctx.config.llm; + const useTools = args.tools !== false; + + let healAttempts = 0; + let toolCalls = 0; + let lastError: string | null = null; + let lastText = ""; + let totalPrompt = 0, totalCompletion = 0, totalAll = 0; + + for (let attempt = 0; attempt <= healRetries; attempt++) { + let finalText = ""; + while (true) { + const promptBytes = messages.reduce( + (n, m) => n + (typeof m.content === "string" ? m.content.length : 0), + 0, + ); + log.info("llm.request", { model, attempt, prompt_bytes: promptBytes, messages: messages.length }); + const req = { + model, + messages, + tools: useTools ? TOOL_SCHEMAS : undefined, + tool_choice: useTools ? "auto" as const : undefined, + temperature: Number.isFinite(temperature) ? temperature : 0.2, + max_tokens: maxOutputTokens, + stream: true, + stream_options: { include_usage: true }, + }; + // LLM_DEBUG=1 dumps the full JSON body (no URL, no headers, no + // credentials). Lets the operator verify what the LLM actually sees. + if (ctx.config.llm.debug) log.debug("llm.request.body", { body: req }); + const resp = await send({ baseUrl, apiKey, req }); + const msg = await receive(resp, { onText: (s) => ctx.logger.stream(s) }); + if (msg.usage) { + totalPrompt += msg.usage.prompt_tokens; + totalCompletion += msg.usage.completion_tokens; + totalAll += msg.usage.total_tokens; + } + + if (msg.tool_calls.length > 0) { + messages.push({ role: "assistant", content: msg.content || null, tool_calls: msg.tool_calls }); + for (const tc of msg.tool_calls) { + toolCalls++; + log.info("tool.call", { name: tc.function.name, args_preview: tc.function.arguments.slice(0, 200) }); + const result = await execTool(tc); + messages.push({ role: "tool", content: result.output, tool_call_id: tc.id }); + } + continue; + } + + finalText = msg.content; + lastText = finalText; + break; + } + + let extracted: Extracted | null = null; + try { + extracted = extractJson(finalText); + } catch (e) { + lastError = `JSON parse: ${errorMessage(e)}`; + } + if (extracted) { + if (validate(extracted.value)) { + log.info("llm.done", { + prompt_tokens: totalPrompt, + completion_tokens: totalCompletion, + heal_attempts: healAttempts, + tool_calls: toolCalls, + }); + return { + validated: extracted.value as T, + usage: { promptTokens: totalPrompt, completionTokens: totalCompletion, totalTokens: totalAll }, + healAttempts, + toolCalls, + rawText: lastText, + }; + } + // A clean parse that fails the schema is a genuine structure problem. + // But if we only salvaged a fragment (clean=false), the JSON itself was + // malformed — the schema error is misleading (it's validating a recovered + // sub-object). Heal on the real cause: invalid JSON, usually an unescaped + // quote or newline inside a string value. + lastError = extracted.clean + ? `schema: ${ajv.errorsText(validate.errors, { separator: "; " })}` + : `invalid JSON: your output did not parse as a whole — only a fragment was recoverable. ` + + `This is almost always an unescaped " or a raw newline inside a string value. ` + + `Escape every double quote as \\" and every newline as \\n.`; + } + + if (attempt < healRetries) { + healAttempts++; + log.warn("llm.heal", { attempt: healAttempts, reason: lastError ?? "unknown" }); + messages.push({ role: "assistant", content: finalText }); + messages.push({ role: "user", content: healPrompt(lastError ?? "unknown") }); + } + } + + log.error("llm.exhausted", { heal_retries: healRetries, last_error: lastError }); + throw new LLMValidationError( + `stage=${args.stage} exhausted ${healRetries} heal attempts; last: ${lastError}`, + ); } -export async function postChat(args: { +// ---- Model auto-detection ------------------------------------------------- +// Ask the endpoint via `GET /models` and pick the first id. Only called when +// no explicit model is configured (buildCtx branches before calling); no +// hardcoded provider heuristics — if the endpoint yields nothing, error out +// with a pointer to --llm-model. + +export interface ResolveModelArgs { baseUrl: string; apiKey: string; - req: ChatRequest; -}): Promise { - const headers: Record = { - "Content-Type": "application/json", - "Accept": args.req.stream ? "text/event-stream" : "application/json", - }; +} + +export async function resolveModel(args: ResolveModelArgs): Promise { + const url = `${args.baseUrl.replace(/\/$/, "")}/models`; + const headers: Record = {}; if (args.apiKey) headers["Authorization"] = `Bearer ${args.apiKey}`; - const resp = await fetch(`${args.baseUrl}/chat/completions`, { - method: "POST", - headers, - body: JSON.stringify(args.req), - }); + const resp = await fetch(url, { headers }); if (!resp.ok) { const text = await resp.text().catch(() => ""); - throw new Error(`LLM HTTP ${resp.status}: ${text.slice(0, 1024)}`); + throw new Error( + `model auto-detection failed: GET ${url} → HTTP ${resp.status}. ` + + `Pass --llm-model explicitly or set LLM_MODEL. ${text.slice(0, 200)}`, + ); + } + const data = (await resp.json()) as { data?: Array<{ id: string }> }; + const first = data.data?.[0]?.id; + if (!first) { + throw new Error( + `model auto-detection failed: ${url} returned no models. ` + + `Pass --llm-model explicitly or set LLM_MODEL.`, + ); } - return resp; + return first; } diff --git a/src/llm/model.ts b/src/llm/model.ts deleted file mode 100644 index b620105..0000000 --- a/src/llm/model.ts +++ /dev/null @@ -1,34 +0,0 @@ -// Resolves which model to use. Explicit override wins; otherwise we ask the -// LLM endpoint via `GET /models` and pick the first id. No hardcoded provider -// heuristics — if neither path yields a model, error out. - -export interface ResolveModelArgs { - baseUrl: string; - apiKey: string; - override?: string; -} - -export async function resolveModel(args: ResolveModelArgs): Promise { - if (args.override) return args.override; - - const url = `${args.baseUrl.replace(/\/$/, "")}/models`; - const headers: Record = {}; - if (args.apiKey) headers["Authorization"] = `Bearer ${args.apiKey}`; - const resp = await fetch(url, { headers }); - if (!resp.ok) { - const text = await resp.text().catch(() => ""); - throw new Error( - `model auto-detection failed: GET ${url} → HTTP ${resp.status}. ` + - `Pass --llm-model explicitly or set LLM_MODEL. ${text.slice(0, 200)}`, - ); - } - const data = (await resp.json()) as { data?: Array<{ id: string }> }; - const first = data.data?.[0]?.id; - if (!first) { - throw new Error( - `model auto-detection failed: ${url} returned no models. ` + - `Pass --llm-model explicitly or set LLM_MODEL.`, - ); - } - return first; -} diff --git a/src/llm/tools/bash.ts b/src/llm/tools/bash.ts index da73d33..2a67779 100644 --- a/src/llm/tools/bash.ts +++ b/src/llm/tools/bash.ts @@ -6,7 +6,7 @@ import { spawn } from "node:child_process"; -import { cap, type Tool, type ToolResult } from "./shared.ts"; +import { cap, type Tool, type ToolResult } from "./types.ts"; export const bashTool: Tool = { schema: { diff --git a/src/llm/tools/index.ts b/src/llm/tools/index.ts index ec8aae2..7fa4705 100644 --- a/src/llm/tools/index.ts +++ b/src/llm/tools/index.ts @@ -10,12 +10,13 @@ // Add a new tool: create tools/.ts exporting `Tool: Tool`, then // add it to the TOOLS array below. -import type { ToolCall } from "../client.ts"; +import { errorMessage } from "../../lib/errors.ts"; +import type { ToolCall } from "../types.ts"; import { bashTool } from "./bash.ts"; import { readTool } from "./read.ts"; -import type { Tool, ToolResult } from "./shared.ts"; +import type { Tool, ToolResult } from "./types.ts"; -export type { ToolResult } from "./shared.ts"; +export type { ToolResult } from "./types.ts"; const TOOLS: Tool[] = [readTool, bashTool]; @@ -45,7 +46,7 @@ export async function execTool(call: ToolCall): Promise { `tool call rejected: arguments are not valid JSON.`, `Tool: ${name}`, `Received arguments: ${call.function.arguments}`, - `Parse error: ${(e as Error).message}`, + `Parse error: ${errorMessage(e)}`, `Retry with a valid JSON object matching the tool's schema.`, ].join("\n"), }; diff --git a/src/llm/tools/read.ts b/src/llm/tools/read.ts index 42d4df2..1b869ce 100644 --- a/src/llm/tools/read.ts +++ b/src/llm/tools/read.ts @@ -7,7 +7,8 @@ import { readFile } from "node:fs/promises"; -import { cap, type Tool } from "./shared.ts"; +import { errorMessage } from "../../lib/errors.ts"; +import { cap, type Tool } from "./types.ts"; const MAX_LINES = 300; @@ -53,7 +54,7 @@ export const readTool: Tool = { return { ok: false, output: - `error reading ${path}: ${(e as Error).message}. ` + + `error reading ${path}: ${errorMessage(e)}. ` + `Check the path (relative to the working directory) and try Bash ls/find if unsure.`, }; } diff --git a/src/llm/tools/shared.ts b/src/llm/tools/types.ts similarity index 93% rename from src/llm/tools/shared.ts rename to src/llm/tools/types.ts index 405aaec..15d4e1c 100644 --- a/src/llm/tools/shared.ts +++ b/src/llm/tools/types.ts @@ -1,6 +1,6 @@ // Shared types + helpers for LLM tools. -import type { ToolSchema } from "../client.ts"; +import type { ToolSchema } from "../types.ts"; export interface ToolResult { ok: boolean; diff --git a/src/llm/stream.ts b/src/llm/transport.ts similarity index 61% rename from src/llm/stream.ts rename to src/llm/transport.ts index 817653d..055f201 100644 --- a/src/llm/stream.ts +++ b/src/llm/transport.ts @@ -1,15 +1,46 @@ -// SSE parser for OpenAI-compatible /chat/completions streams. +// LLM transport — the wire level, both directions: +// - send(): POST to an OpenAI-compatible /chat/completions endpoint, +// returns the raw Response. +// - receive(): SSE parser for the streamed response. Each chunk is +// `data: {json}\n\n`, terminated by `data: [DONE]`. We +// accumulate `delta.content` (the visible text) and +// `delta.tool_calls` (function calls the LLM asks us to +// execute) into a final shape that looks like a +// non-streaming completion's `choices[0].message`. // -// Each chunk is `data: {json}\n\n`, terminated by `data: [DONE]`. We -// accumulate `delta.content` (the visible text) and `delta.tool_calls` -// (function calls the LLM is asking us to execute) into a final shape that -// looks like a non-streaming completion's `choices[0].message`. -// -// Side effect: while streaming, write `delta.content` to stderr live so the -// CI log shows the LLM's output as it's produced — the same UX the old -// Python harness gave us via stream-json + jq. +// The two halves are one contract: receive parses exactly what send's +// `stream: true` request produces (incl. stream_options.include_usage → +// usage accumulation). Consumer API lives in client.ts; the wire shapes +// themselves live in types.ts. + +import type { ChatRequest, ToolCall } from "./types.ts"; -import type { ToolCall } from "./client.ts"; +export async function send(args: { + baseUrl: string; + apiKey: string; + req: ChatRequest; +}): Promise { + const headers: Record = { + "Content-Type": "application/json", + "Accept": args.req.stream ? "text/event-stream" : "application/json", + }; + if (args.apiKey) headers["Authorization"] = `Bearer ${args.apiKey}`; + const resp = await fetch(`${args.baseUrl}/chat/completions`, { + method: "POST", + headers, + body: JSON.stringify(args.req), + }); + if (!resp.ok) { + const text = await resp.text().catch(() => ""); + throw new Error(`LLM HTTP ${resp.status}: ${text.slice(0, 1024)}`); + } + return resp; +} + +// ---- SSE stream parsing --------------------------------------------------- +// +// Side effect: while streaming, the caller-provided onText hook receives +// delta.content live so the CI log shows the LLM's output as it's produced. export interface AccumulatedMessage { content: string; @@ -40,7 +71,7 @@ interface Chunk { usage?: { prompt_tokens: number; completion_tokens: number; total_tokens: number }; } -export async function readStream(resp: Response, opts?: { onText?: (s: string) => void }): Promise { +export async function receive(resp: Response, opts?: { onText?: (s: string) => void }): Promise { if (!resp.body) throw new Error("LLM response has no body"); const reader = resp.body.getReader(); const decoder = new TextDecoder("utf-8"); diff --git a/src/llm/types.ts b/src/llm/types.ts new file mode 100644 index 0000000..122fe6b --- /dev/null +++ b/src/llm/types.ts @@ -0,0 +1,48 @@ +// Shared LLM contract — the vocabulary used across llm/ and by stages. +// +// Wire shapes mirror the OpenAI /chat/completions JSON (snake_case fields +// are not renamed); Section and LLMResult are the client API's input/output +// shapes. Behavior lives in client.ts (consumer API) and transport.ts (wire). + +// ---- Wire shapes (OpenAI-compatible) -------------------------------------- + +export interface ChatRequest { + model: string; + messages: ChatMessage[]; + tools?: ToolSchema[]; + tool_choice?: "auto" | "required" | "none"; + temperature?: number; + max_tokens?: number; + stream?: boolean; + stream_options?: { include_usage?: boolean }; +} + +export type ChatMessage = + | { role: "system" | "user"; content: string } + | { role: "assistant"; content: string | null; tool_calls?: ToolCall[] } + | { role: "tool"; content: string; tool_call_id: string }; + +export interface ToolSchema { + type: "function"; + function: { name: string; description: string; parameters: object }; +} + +export interface ToolCall { + id: string; + type: "function"; + function: { name: string; arguments: string }; +} + +// ---- Client API shapes ----------------------------------------------------- + +// One tagged prompt section; serialized as content, empty tag = +// untagged prose (ADR-0006). +export type Section = { tag: string; content: string }; + +export interface LLMResult { + validated: T; + usage: { promptTokens: number; completionTokens: number; totalTokens: number }; + healAttempts: number; + toolCalls: number; + rawText: string; +} diff --git a/src/logger/console.ts b/src/logger/console.ts index d6ebf3e..d5153a2 100644 --- a/src/logger/console.ts +++ b/src/logger/console.ts @@ -1,7 +1,7 @@ // Default sink: pretty indented output to stderr. Each event on its own // line, structured `data` rendered as indented JSON below it. -import type { Logger, LogLevel } from "./index.ts"; +import { joinScope, type Logger, type LogLevel } from "./types.ts"; const LEVEL_TAG: Record = { debug: "DBG", @@ -42,6 +42,6 @@ export class ConsoleLogger implements Logger { process.stderr.write(chunk); } child(scope: string): Logger { - return new ConsoleLogger(this.scope ? `${this.scope}.${scope}` : scope); + return new ConsoleLogger(joinScope(this.scope, scope)); } } diff --git a/src/logger/index.ts b/src/logger/index.ts index 55c6e39..c3d0330 100644 --- a/src/logger/index.ts +++ b/src/logger/index.ts @@ -1,26 +1,5 @@ -// Logger interface + factory. Swap sinks by passing a different Logger -// instance into the Ctx; everything downstream uses the interface only. - -export type LogLevel = "debug" | "info" | "warn" | "error"; - -export interface LogEntry { - level: LogLevel; - ts: string; - scope?: string; - event: string; - data?: Record; -} - -export interface Logger { - debug(event: string, data?: Record): void; - info(event: string, data?: Record): void; - warn(event: string, data?: Record): void; - error(event: string, data?: Record): void; - // Raw streamed text (LLM token stream, etc). Structured sinks may ignore it. - stream(chunk: string): void; - // Scoped child — events from the child are tagged with the given scope. - child(scope: string): Logger; -} +// Public face of the logger folder: contract (types.ts) + sinks. +export type { Logger, LogEntry, LogLevel } from "./types.ts"; export { ConsoleLogger } from "./console.ts"; export { MemoryLogger } from "./memory.ts"; diff --git a/src/logger/memory.ts b/src/logger/memory.ts index 005bff0..61daa94 100644 --- a/src/logger/memory.ts +++ b/src/logger/memory.ts @@ -1,7 +1,7 @@ // In-memory sink. Records every event for test assertions. `stream` chunks // are concatenated into a single field per scope. -import type { Logger, LogEntry } from "./index.ts"; +import { joinScope, type Logger, type LogEntry } from "./types.ts"; export class MemoryLogger implements Logger { readonly entries: LogEntry[]; @@ -43,7 +43,6 @@ export class MemoryLogger implements Logger { } child(scope: string): Logger { - const next = this.scope ? `${this.scope}.${scope}` : scope; - return new MemoryLogger(next, this.entries, this._streamRef); + return new MemoryLogger(joinScope(this.scope, scope), this.entries, this._streamRef); } } diff --git a/src/logger/types.ts b/src/logger/types.ts new file mode 100644 index 0000000..cc4b511 --- /dev/null +++ b/src/logger/types.ts @@ -0,0 +1,30 @@ +// Logger contract. Swap sinks by passing a different Logger instance into +// the Ctx; everything downstream uses the interface only. + +export type LogLevel = "debug" | "info" | "warn" | "error"; + +export interface LogEntry { + level: LogLevel; + ts: string; + scope?: string; + event: string; + data?: Record; +} + +export interface Logger { + debug(event: string, data?: Record): void; + info(event: string, data?: Record): void; + warn(event: string, data?: Record): void; + error(event: string, data?: Record): void; + // Raw streamed text (LLM token stream, etc). Structured sinks may ignore it. + stream(chunk: string): void; + // Scoped child — events from the child are tagged with the given scope. + child(scope: string): Logger; +} + +// Scope-join format for child(). One definition so every sink produces the +// same scope strings — tests and log readers depend on the dotted form +// (essential duplication, consolidated — see docs/conventions/consolidation.md). +export function joinScope(parent: string | undefined, child: string): string { + return parent ? `${parent}.${child}` : child; +} diff --git a/src/providers/base.ts b/src/providers/base.ts new file mode 100644 index 0000000..c9b1609 --- /dev/null +++ b/src/providers/base.ts @@ -0,0 +1,41 @@ +// Shared plumbing for providers whose diff data comes from the local git +// checkout — which is all of them (ADR-0004). Subclasses inherit +// changed-files / per-file diffs / potential-rename scanning; `getRenames` +// defaults to git's confident (-M ≥50%) detection and is overridden by +// providers whose API exposes richer rename data. + +import { gitChangedFiles, gitFileDiff, gitPotentialRenames } from "../lib/git.ts"; +import type { ChangedFile, FileDiffOpts, PotentialRename, PRMetadata } from "./types.ts"; + +export abstract class BaseProvider { + // Directory the git commands run in. undefined = process cwd (the real + // providers run against the checked-out consuming repo). The mock provider + // points this at its throwaway fixture repo. + protected gitCwd?: string; + + async getChangedFiles(meta: PRMetadata): Promise { + return gitChangedFiles({ baseSha: meta.baseSha, headSha: meta.headSha, cwd: this.gitCwd }); + } + + async getPotentialRenames(meta: PRMetadata): Promise { + return gitPotentialRenames({ baseSha: meta.baseSha, headSha: meta.headSha, cwd: this.gitCwd }); + } + + async getFileDiff(meta: PRMetadata, path: string, opts: FileDiffOpts): Promise { + return gitFileDiff({ + baseSha: meta.baseSha, + headSha: meta.headSha, + path, + context: opts.context, + cwd: this.gitCwd, + }); + } + + async getRenames(meta: PRMetadata): Promise<{ [oldPath: string]: string }> { + const renames: { [oldPath: string]: string } = {}; + for (const f of await this.getChangedFiles(meta)) { + if (f.status === "renamed" && f.oldPath) renames[f.oldPath] = f.path; + } + return renames; + } +} diff --git a/src/vcs/bitbucket.ts b/src/providers/bitbucket.ts similarity index 74% rename from src/vcs/bitbucket.ts rename to src/providers/bitbucket.ts index 8f95aaf..ec49e4d 100644 --- a/src/vcs/bitbucket.ts +++ b/src/providers/bitbucket.ts @@ -1,4 +1,4 @@ -// Bitbucket implementation of VcsProvider. REST API v2 via fetch. +// Bitbucket implementation of Provider. REST API v2 via fetch. // // Token needs: // - Cloud (bitbucket.org): App Password or OAuth token with `pullrequest:write`, `repository:read`. @@ -7,16 +7,31 @@ // baseUrl defaults to https://api.bitbucket.org/2.0; self-hosted instances pass their own. import type { Config } from "../ctx.ts"; -import { gitChangedFiles, gitFileDiff, gitPotentialRenames } from "../lib/git.ts"; -import type { - ChangedFile, - FileDiffOpts, - InlinePost, - PotentialRename, - PRComment, - PRMetadata, - VcsProvider, -} from "./types.ts"; +import { BaseProvider } from "./base.ts"; +import type { InlinePost, PRComment, PRMetadata, Provider } from "./types.ts"; + +type Api = (path: string, init?: RequestInit) => Promise; + +// Fetch wrapper owned by this provider — deliberately NOT shared with +// gitlab.ts: the two wrappers look alike today only by coincidence, and +// the APIs will diverge (pagination, rate limits, auth). See docs/conventions/consolidation.md. +function makeApi(baseUrl: string, token: string): Api { + return async (path, init) => { + const resp = await fetch(`${baseUrl}${path}`, { + ...init, + headers: { + Authorization: `Bearer ${token}`, + "Content-Type": "application/json", + ...(init?.headers ?? {}), + }, + }); + if (!resp.ok) { + const text = await resp.text().catch(() => ""); + throw new Error(`Bitbucket API ${resp.status}: ${text.slice(0, 1024)}`); + } + return resp.json() as Promise; + }; +} interface BitbucketPR { id: number; @@ -41,51 +56,30 @@ interface BitbucketComment { resolved?: boolean; } -export class BitbucketProvider implements VcsProvider { +export class BitbucketProvider extends BaseProvider implements Provider { readonly name = "bitbucket" as const; private constructor( - private readonly token: string, - private readonly baseUrl: string, + private readonly api: Api, private readonly owner: string, private readonly repo: string, private readonly number: number, private readonly botLogin: string, - ) {} + ) { + super(); + } static async create(config: Config): Promise { const { url } = config.pr; const { token, baseUrl } = config.bitbucket; if (!token) throw new Error("BitbucketProvider: token required"); - let owner: string, repo: string, num: number; const cloud = url.match(/^https?:\/\/bitbucket\.org\/([^/]+)\/([^/]+)\/pull-requests\/(\d+)/); const server = url.match(/^https?:\/\/[^/]+\/projects\/([^/]+)\/repos\/([^/]+)\/pull-requests\/(\d+)/); const m = cloud ?? server; if (!m) throw new Error(`BitbucketProvider: not a Bitbucket PR URL: ${url}`); - owner = m[1]!; repo = m[2]!; num = parseInt(m[3]!, 10); - const resolvedBaseUrl = (baseUrl || "https://api.bitbucket.org/2.0").replace(/\/$/, ""); - const me = await BitbucketProvider.api(resolvedBaseUrl, token, "/user") as { username: string }; - return new BitbucketProvider(token, resolvedBaseUrl, owner, repo, num, me.username); - } - - private static async api(baseUrl: string, token: string, path: string, opts?: RequestInit): Promise { - const resp = await fetch(`${baseUrl}${path}`, { - ...opts, - headers: { - Authorization: `Bearer ${token}`, - "Content-Type": "application/json", - ...(opts?.headers ?? {}), - }, - }); - if (!resp.ok) { - const text = await resp.text().catch(() => ""); - throw new Error(`Bitbucket API ${resp.status}: ${text.slice(0, 1024)}`); - } - return resp.json() as unknown; - } - - private api(path: string, opts?: RequestInit): Promise { - return BitbucketProvider.api(this.baseUrl, this.token, path, opts); + const api = makeApi((baseUrl || "https://api.bitbucket.org/2.0").replace(/\/$/, ""), token); + const me = await api("/user") as { username: string }; + return new BitbucketProvider(api, m[1]!, m[2]!, parseInt(m[3]!, 10), me.username); } private prPath(): string { @@ -107,18 +101,6 @@ export class BitbucketProvider implements VcsProvider { }; } - async getChangedFiles(meta: PRMetadata): Promise { - return gitChangedFiles({ baseSha: meta.baseSha, headSha: meta.headSha }); - } - - async getPotentialRenames(meta: PRMetadata): Promise { - return gitPotentialRenames({ baseSha: meta.baseSha, headSha: meta.headSha }); - } - - async getFileDiff(meta: PRMetadata, path: string, opts: FileDiffOpts): Promise { - return gitFileDiff({ baseSha: meta.baseSha, headSha: meta.headSha, path, context: opts.context }); - } - async getPRComments(): Promise { const out: PRComment[] = []; const comments = await this.api(`${this.prPath()}/comments?pagelen=100`) as { values?: BitbucketComment[] }; @@ -179,7 +161,9 @@ export class BitbucketProvider implements VcsProvider { await this.api(`${this.prPath()}/comments/${commentId}`, { method: "DELETE" }); } - async getRenames(_meta: PRMetadata): Promise<{ [oldPath: string]: string }> { + // Bitbucket's diffstat reports renames directly — richer than the base + // class's git -M derivation. + override async getRenames(_meta: PRMetadata): Promise<{ [oldPath: string]: string }> { const diff = await this.api(`${this.prPath()}/diff`) as { values?: { status: string; old?: { path: string }; new?: { path: string } }[]; }; diff --git a/src/vcs/github.ts b/src/providers/github.ts similarity index 89% rename from src/vcs/github.ts rename to src/providers/github.ts index 54a75f0..95c89ca 100644 --- a/src/vcs/github.ts +++ b/src/providers/github.ts @@ -1,4 +1,4 @@ -// GitHub implementation of VcsProvider. REST via @octokit/rest for the basic +// GitHub implementation of Provider. REST via @octokit/rest for the basic // PR data and posting; GraphQL for review-thread resolution (the REST API // doesn't expose `isResolved` or a resolve endpoint on review threads). // @@ -10,16 +10,8 @@ import { Octokit } from "@octokit/rest"; import type { Config } from "../ctx.ts"; -import { gitChangedFiles, gitFileDiff, gitPotentialRenames } from "../lib/git.ts"; -import type { - ChangedFile, - FileDiffOpts, - InlinePost, - PotentialRename, - PRComment, - PRMetadata, - VcsProvider, -} from "./types.ts"; +import { BaseProvider } from "./base.ts"; +import type { InlinePost, PRComment, PRMetadata, Provider } from "./types.ts"; interface ThreadGql { id: string; @@ -27,7 +19,7 @@ interface ThreadGql { comments: { nodes: { databaseId: number | null }[] }; } -export class GitHubProvider implements VcsProvider { +export class GitHubProvider extends BaseProvider implements Provider { readonly name = "github" as const; private constructor( @@ -36,7 +28,9 @@ export class GitHubProvider implements VcsProvider { private readonly repo: string, private readonly number: number, private readonly botLogin: string, - ) {} + ) { + super(); + } static async create(config: Config, octokit?: Octokit): Promise { const { url } = config.pr; @@ -79,18 +73,6 @@ export class GitHubProvider implements VcsProvider { }; } - async getChangedFiles(meta: PRMetadata): Promise { - return gitChangedFiles({ baseSha: meta.baseSha, headSha: meta.headSha }); - } - - async getPotentialRenames(meta: PRMetadata): Promise { - return gitPotentialRenames({ baseSha: meta.baseSha, headSha: meta.headSha }); - } - - async getFileDiff(meta: PRMetadata, path: string, opts: FileDiffOpts): Promise { - return gitFileDiff({ baseSha: meta.baseSha, headSha: meta.headSha, path, context: opts.context }); - } - async getPRComments(): Promise { const out: PRComment[] = []; @@ -189,7 +171,9 @@ export class GitHubProvider implements VcsProvider { } } - async getRenames(meta: PRMetadata): Promise<{ [oldPath: string]: string }> { + // GitHub's compare API has its own rename detection — richer than the + // base class's git -M derivation. + override async getRenames(meta: PRMetadata): Promise<{ [oldPath: string]: string }> { const resp = await this.client.repos.compareCommits({ owner: this.owner, repo: this.repo, base: meta.baseSha, head: meta.headSha, }); diff --git a/src/vcs/gitlab.ts b/src/providers/gitlab.ts similarity index 76% rename from src/vcs/gitlab.ts rename to src/providers/gitlab.ts index 34b4e53..3b78cf9 100644 --- a/src/vcs/gitlab.ts +++ b/src/providers/gitlab.ts @@ -1,4 +1,4 @@ -// GitLab implementation of VcsProvider. Plain fetch — no official SDK needed +// GitLab implementation of Provider. Plain fetch — no official SDK needed // for the subset we use. // // Token needs: @@ -9,16 +9,31 @@ // their own. import type { Config } from "../ctx.ts"; -import { gitChangedFiles, gitFileDiff, gitPotentialRenames } from "../lib/git.ts"; -import type { - ChangedFile, - FileDiffOpts, - InlinePost, - PotentialRename, - PRComment, - PRMetadata, - VcsProvider, -} from "./types.ts"; +import { BaseProvider } from "./base.ts"; +import type { InlinePost, PRComment, PRMetadata, Provider } from "./types.ts"; + +type Api = (path: string, init?: RequestInit) => Promise; + +// Fetch wrapper owned by this provider — deliberately NOT shared with +// bitbucket.ts: the two wrappers look alike today only by coincidence, and +// the APIs will diverge (pagination, rate limits, auth). See docs/conventions/consolidation.md. +function makeApi(baseUrl: string, token: string): Api { + return async (path, init) => { + const resp = await fetch(`${baseUrl}${path}`, { + ...init, + headers: { + "PRIVATE-TOKEN": token, + "Content-Type": "application/json", + ...(init?.headers ?? {}), + }, + }); + if (!resp.ok) { + const text = await resp.text().catch(() => ""); + throw new Error(`GitLab API ${resp.status}: ${text.slice(0, 1024)}`); + } + return resp.json() as Promise; + }; +} interface GitLabMR { id: number; @@ -51,16 +66,17 @@ interface GitLabNote { thread_id?: number; } -export class GitLabProvider implements VcsProvider { +export class GitLabProvider extends BaseProvider implements Provider { readonly name = "gitlab" as const; private constructor( - private readonly token: string, - private readonly baseUrl: string, + private readonly api: Api, private readonly projectPath: string, // {host}/{owner}/{repo} private readonly number: number, private readonly botLogin: string, - ) {} + ) { + super(); + } static async create(config: Config): Promise { const { url } = config.pr; @@ -69,31 +85,10 @@ export class GitLabProvider implements VcsProvider { // {host}/{owner}/{repo}/-/merge_requests/{n} const m = url.match(/^https?:\/\/([^/]+)\/([^/]+)\/([^/]+)\/-\/merge_requests\/(\d+)/); if (!m) throw new Error(`GitLabProvider: not a GitLab MR URL: ${url}`); - const resolvedBaseUrl = (baseUrl || "https://gitlab.com/api/v4").replace(/\/$/, ""); + const api = makeApi((baseUrl || "https://gitlab.com/api/v4").replace(/\/$/, ""), token); const projectPath = `${m[1]!}/${m[2]!}/${m[3]!}`; - const me = await GitLabProvider.api(resolvedBaseUrl, token, "/user") as { username: string }; - return new GitLabProvider(token, resolvedBaseUrl, projectPath, parseInt(m[4]!, 10), me.username); - } - - // Static so create() can use it before the instance exists. - private static async api(baseUrl: string, token: string, path: string, opts?: RequestInit): Promise { - const resp = await fetch(`${baseUrl}${path}`, { - ...opts, - headers: { - "PRIVATE-TOKEN": token, - "Content-Type": "application/json", - ...(opts?.headers ?? {}), - }, - }); - if (!resp.ok) { - const text = await resp.text().catch(() => ""); - throw new Error(`GitLab API ${resp.status}: ${text.slice(0, 1024)}`); - } - return resp.json() as unknown; - } - - private api(path: string, opts?: RequestInit): Promise { - return GitLabProvider.api(this.baseUrl, this.token, path, opts); + const me = await api("/user") as { username: string }; + return new GitLabProvider(api, projectPath, parseInt(m[4]!, 10), me.username); } private mrPath(): string { @@ -115,18 +110,6 @@ export class GitLabProvider implements VcsProvider { }; } - async getChangedFiles(meta: PRMetadata): Promise { - return gitChangedFiles({ baseSha: meta.baseSha, headSha: meta.headSha }); - } - - async getPotentialRenames(meta: PRMetadata): Promise { - return gitPotentialRenames({ baseSha: meta.baseSha, headSha: meta.headSha }); - } - - async getFileDiff(meta: PRMetadata, path: string, opts: FileDiffOpts): Promise { - return gitFileDiff({ baseSha: meta.baseSha, headSha: meta.headSha, path, context: opts.context }); - } - async getPRComments(): Promise { const out: PRComment[] = []; const notes = await this.api(`${this.mrPath()}/notes?per_page=100&sort=asc`) as GitLabNote[]; @@ -203,7 +186,9 @@ export class GitLabProvider implements VcsProvider { await this.api(`${this.mrPath()}/notes/${commentId}`, { method: "DELETE" }); } - async getRenames(_meta: PRMetadata): Promise<{ [oldPath: string]: string }> { + // GitLab's changes API reports renames directly — richer than the base + // class's git -M derivation. + override async getRenames(_meta: PRMetadata): Promise<{ [oldPath: string]: string }> { const changes = await this.api(`${this.mrPath()}/changes`) as { changes?: { renamed_to?: string; renamed_from?: string; old_path?: string; new_path?: string }[]; }; diff --git a/src/vcs/local.ts b/src/providers/local.ts similarity index 73% rename from src/vcs/local.ts rename to src/providers/local.ts index facd716..069c496 100644 --- a/src/vcs/local.ts +++ b/src/providers/local.ts @@ -10,16 +10,9 @@ // (LLM_URL / LLM_MODEL) for fast pre-push review while CI uses a heavier one. import type { Config } from "../ctx.ts"; -import { gitChangedFiles, gitFileDiff, gitPotentialRenames, runGit } from "../lib/git.ts"; -import type { - ChangedFile, - FileDiffOpts, - InlinePost, - PotentialRename, - PRComment, - PRMetadata, - VcsProvider, -} from "./types.ts"; +import { runGit } from "../lib/git.ts"; +import { BaseProvider } from "./base.ts"; +import type { InlinePost, PRComment, PRMetadata, Provider } from "./types.ts"; // ANSI color, on only for an interactive stdout (TTY, no NO_COLOR) so // redirected / piped output stays plain. Raw escapes — no dependency. @@ -45,12 +38,14 @@ function badge(severity?: "info" | "warning" | "blocker"): string { } } -export class LocalVcsProvider implements VcsProvider { +export class LocalProvider extends BaseProvider implements Provider { readonly name = "local" as const; - private constructor(private readonly meta: PRMetadata) {} + private constructor(private readonly meta: PRMetadata) { + super(); + } - static async create(config: Config): Promise { + static async create(config: Config): Promise { const m = config.pr.url.match(/^local:\/\/(.*)$/); const base = (m?.[1] ?? "").trim() || "main"; @@ -67,7 +62,7 @@ export class LocalVcsProvider implements VcsProvider { const author = (await runGit(["log", "-1", "--format=%an"]).catch(() => "")).trim(); const title = (await runGit(["log", "-1", "--format=%s"]).catch(() => "")).trim(); - return new LocalVcsProvider({ + return new LocalProvider({ title: title || `${headBranch} vs ${base}`, description: "", author: author || "local", @@ -84,18 +79,6 @@ export class LocalVcsProvider implements VcsProvider { return this.meta; } - async getChangedFiles(meta: PRMetadata): Promise { - return gitChangedFiles({ baseSha: meta.baseSha, headSha: meta.headSha }); - } - - async getPotentialRenames(meta: PRMetadata): Promise { - return gitPotentialRenames({ baseSha: meta.baseSha, headSha: meta.headSha }); - } - - async getFileDiff(meta: PRMetadata, path: string, opts: FileDiffOpts): Promise { - return gitFileDiff({ baseSha: meta.baseSha, headSha: meta.headSha, path, context: opts.context }); - } - async getPRComments(): Promise { return []; } @@ -123,13 +106,4 @@ export class LocalVcsProvider implements VcsProvider { async deleteComment(commentId: string, kind: "issue" | "review"): Promise { out(dim(`↳ deleted ${kind} comment ${commentId}`)); } - - async getRenames(meta: PRMetadata): Promise<{ [oldPath: string]: string }> { - const changed = await gitChangedFiles({ baseSha: meta.baseSha, headSha: meta.headSha }); - const out: { [oldPath: string]: string } = {}; - for (const f of changed) { - if (f.status === "renamed" && f.oldPath) out[f.oldPath] = f.path; - } - return out; - } } diff --git a/src/vcs/mock.ts b/src/providers/mock.ts similarity index 80% rename from src/vcs/mock.ts rename to src/providers/mock.ts index 197da72..f34c4dc 100644 --- a/src/vcs/mock.ts +++ b/src/providers/mock.ts @@ -21,7 +21,8 @@ import { fileURLToPath } from "node:url"; import { parse as parseYaml } from "yaml"; import type { Config } from "../ctx.ts"; -import { gitChangedFiles, gitFileDiff, gitPotentialRenames } from "../lib/git.ts"; +import { errorMessage } from "../lib/errors.ts"; +import { BaseProvider } from "./base.ts"; import type { ChangedFile, FileDiffOpts, @@ -29,7 +30,7 @@ import type { PotentialRename, PRComment, PRMetadata, - VcsProvider, + Provider, } from "./types.ts"; // ---- Fixture loading ----------------------------------------------------- @@ -58,7 +59,9 @@ function loadFixtureConfig(scenarioPath: string): FixtureConfig { try { return parseYaml(readFileSync(p, "utf-8")) as FixtureConfig; } catch (e) { - console.warn(`mock: failed to parse ${p}: ${(e as Error).message}`); + // console.warn (not the Logger port): the provider is constructed + // before a logger exists, and this path is test-only — deliberate exception. + console.warn(`mock: failed to parse ${p}: ${errorMessage(e)}`); return {}; } } @@ -128,7 +131,7 @@ const DEFAULT_META: PRMetadata = { state: "open", }; -export class MockVcsProvider implements VcsProvider { +export class MockProvider extends BaseProvider implements Provider { readonly name: "mock" = "mock"; private readonly repo: RepoSetup | null; @@ -142,39 +145,41 @@ export class MockVcsProvider implements VcsProvider { private _verdicts: Array<{ verdict: MockAction; summary?: string }> = []; private constructor(repo: RepoSetup | null, fixture: FixtureConfig) { + super(); this.repo = repo; + this.gitCwd = repo?.cwd; this.fixture = fixture; } // Bare provider — no scenario, no git repo. For unit tests that drive a // single stage (e.g. summarize) with a hand-built StageState and only need // the side-effect recorders. - static empty(fixture: FixtureConfig = {}): MockVcsProvider { - return new MockVcsProvider(null, fixture); + static empty(fixture: FixtureConfig = {}): MockProvider { + return new MockProvider(null, fixture); } - static async create(config: Config): Promise { - return MockVcsProvider.fromUrl(config.pr.url); + static async create(config: Config): Promise { + return MockProvider.fromUrl(config.pr.url); } - static fromUrl(url: string): MockVcsProvider { + static fromUrl(url: string): MockProvider { const m = url.match(/^mock:\/\/(.+)$/); if (!m) throw new Error(`Invalid mock URL: ${url}. Expected: mock://`); const scenario = m[1]!; const scenarioPath = join(findProjectRoot(), "tests", "fixtures", scenario); if (!existsSync(scenarioPath)) { console.warn(`mock: scenario ${scenario} not found at ${scenarioPath}; provider has no fixture data`); - return new MockVcsProvider(null, {}); + return new MockProvider(null, {}); } const fixture = loadFixtureConfig(scenarioPath); const repo = setupRepo(scenarioPath); // Stages read changed files via relative paths; pin cwd to the temp repo // so existsSync / readFileSync resolve against the mock head tree. process.chdir(repo.cwd); - return new MockVcsProvider(repo, fixture); + return new MockProvider(repo, fixture); } - // ---- VcsProvider -------------------------------------------------------- + // ---- Provider -------------------------------------------------------- async getPRMetadata(): Promise { const fromFixture = this.fixture.metadata ?? {}; @@ -186,19 +191,26 @@ export class MockVcsProvider implements VcsProvider { }; } - async getChangedFiles(meta: PRMetadata): Promise { + // Bare (`empty()`) mocks have no git repo — guard, then defer to the + // base class's delegation against gitCwd. + override async getChangedFiles(meta: PRMetadata): Promise { if (!this.repo) return []; - return gitChangedFiles({ baseSha: meta.baseSha, headSha: meta.headSha, cwd: this.repo.cwd }); + return super.getChangedFiles(meta); } - async getPotentialRenames(meta: PRMetadata): Promise { + override async getPotentialRenames(meta: PRMetadata): Promise { if (!this.repo) return []; - return gitPotentialRenames({ baseSha: meta.baseSha, headSha: meta.headSha, cwd: this.repo.cwd }); + return super.getPotentialRenames(meta); } - async getFileDiff(meta: PRMetadata, path: string, opts: FileDiffOpts): Promise { + override async getFileDiff(meta: PRMetadata, path: string, opts: FileDiffOpts): Promise { if (!this.repo) return ""; - return gitFileDiff({ baseSha: meta.baseSha, headSha: meta.headSha, path, context: opts.context, cwd: this.repo.cwd }); + return super.getFileDiff(meta, path, opts); + } + + override async getRenames(meta: PRMetadata): Promise<{ [oldPath: string]: string }> { + if (!this.repo) return {}; + return super.getRenames(meta); } async getPRComments(): Promise { @@ -223,17 +235,6 @@ export class MockVcsProvider implements VcsProvider { async deleteComment(commentId: string, kind: "issue" | "review"): Promise { this._deletedComments.push({ commentId, kind }); } - // Renames come from git's -M detection in getChangedFiles; mock keeps this - // method coherent with the others (callers use getChangedFiles directly). - async getRenames(meta: PRMetadata): Promise<{ [oldPath: string]: string }> { - if (!this.repo) return {}; - const changed = await gitChangedFiles({ baseSha: meta.baseSha, headSha: meta.headSha, cwd: this.repo.cwd }); - const out: { [oldPath: string]: string } = {}; - for (const f of changed) { - if (f.status === "renamed" && f.oldPath) out[f.oldPath] = f.path; - } - return out; - } // ---- Accessors for test assertions ------------------------------------- diff --git a/src/vcs/types.ts b/src/providers/types.ts similarity index 81% rename from src/vcs/types.ts rename to src/providers/types.ts index c811d51..d34c7d2 100644 --- a/src/vcs/types.ts +++ b/src/providers/types.ts @@ -1,3 +1,10 @@ +import type { ChangedFile, PotentialRename } from "../lib/git.ts"; + +// Git-derived wire shapes are defined where they're produced (lib/git.ts) +// and re-exported here so consumers keep importing them from the port +// (ADR-0009). +export type { ChangedFile, PotentialRename } from "../lib/git.ts"; + export type ProviderKind = "github" | "gitlab" | "bitbucket" | "mock" | "local"; export interface PRMetadata { @@ -36,31 +43,15 @@ export interface InlinePost { severity?: "info" | "warning" | "blocker"; } -export interface ChangedFile { - path: string; - status: "added" | "modified" | "deleted" | "renamed"; - oldPath?: string; -} - export interface FileDiffOpts { context: number; } -// Pairs git flagged as renames at a relaxed similarity threshold (30%–49%) -// — below the confident bar but worth surfacing so the operator can spot -// "this should have been a rename" cases (typically small files where git's -// score is penalized by per-file overhead). -export interface PotentialRename { - oldPath: string; - newPath: string; - similarityPct: number; -} - // Each provider stores its PR identity (owner/repo/number) and token at // construction time via a `static create(config)` async factory. Methods // don't take a ref parameter — the provider serves exactly one PR for its // lifetime. -export interface VcsProvider { +export interface Provider { readonly name: ProviderKind; getPRMetadata(): Promise; @@ -92,6 +83,16 @@ export interface VcsProvider { getRenames(meta: PRMetadata): Promise<{ [oldPath: string]: string }>; } +// One bot inline finding awaiting action: the root comment of an unresolved +// inline thread authored by the bot. The pipeline gate (cli.ts), reconcile's +// candidate selection, and summarize's no-reconcile fallback all share this +// predicate — they must agree or the verdict drifts (ADR-0011). +export function isOpenBotThread( + c: PRComment, +): c is PRComment & { inline: NonNullable } { + return c.by === "bot" && c.inline !== undefined && !c.resolved && !c.parentId; +} + // Dispatch helper: returns which provider's `create()` should handle a URL. export function detectProvider(url: string): ProviderKind { if (/^mock:\/\//.test(url)) return "mock"; diff --git a/src/stages/fetch/index.ts b/src/stages/fetch/index.ts index 3e6b0a2..899f9f7 100644 --- a/src/stages/fetch/index.ts +++ b/src/stages/fetch/index.ts @@ -21,8 +21,9 @@ import { extname } from "node:path"; import type { Ctx } from "../../ctx.ts"; import { firstHunkNewLine, headLinesCovered, runGit } from "../../lib/git.ts"; +import type { Logger } from "../../logger/index.ts"; import { loadStandards, renderStandards } from "./standards.ts"; -import type { ChangedFile, PRMetadata } from "../../vcs/types.ts"; +import type { ChangedFile, PRMetadata } from "../../providers/types.ts"; import { emptyRenderingContext, type BinaryEntry, @@ -39,10 +40,10 @@ export async function runFetch(ctx: Ctx): Promise { log.info("start", { pr: ctx.config.pr.url }); const [meta, comments] = await Promise.all([ - ctx.vcs.getPRMetadata(), - ctx.vcs.getPRComments(), + ctx.provider.getPRMetadata(), + ctx.provider.getPRComments(), ]); - const changed = await ctx.vcs.getChangedFiles(meta); + const changed = await ctx.provider.getChangedFiles(meta); const rendering = await assembleRendering(ctx, meta, changed, log); const standards = loadStandards(ctx.config.review.standardsRoot, log); @@ -93,7 +94,7 @@ async function assembleRendering( ctx: Ctx, meta: PRMetadata, changed: ChangedFile[], - log: ReturnType, + log: Logger, ): Promise { const cfg = ctx.config.review.diffFilter; const exts = new Set(cfg.includeExtensions.map((e) => e.toLowerCase())); @@ -120,7 +121,7 @@ async function assembleRendering( // Deletion: head-side file is gone. Full deletion patch via -U10000. if (f.status === "deleted") { - const content = await ctx.vcs.getFileDiff(meta, f.path, { context: 10000 }); + const content = await ctx.provider.getFileDiff(meta, f.path, { context: 10000 }); pushDiff(out.diffs, f.path, "full_file", content); continue; } @@ -133,26 +134,26 @@ async function assembleRendering( // Small file → inline full file. if (headLineCount <= cfg.fullFileThresholdLines) { - const content = await ctx.vcs.getFileDiff(meta, f.path, { context: 10000 }); + const content = await ctx.provider.getFileDiff(meta, f.path, { context: 10000 }); pushDiff(out.diffs, f.path, "full_file", content); continue; } // Big file. Probe with narrow context to find first change + coverage. - const narrow = await ctx.vcs.getFileDiff(meta, f.path, { context: cfg.narrowContextLines }); + const narrow = await ctx.provider.getFileDiff(meta, f.path, { context: cfg.narrowContextLines }); const firstChange = firstHunkNewLine(narrow); const coverage = headLineCount > 0 ? headLinesCovered(narrow) / headLineCount : 0; if (coverage >= cfg.fullFileCoverageThreshold) { // High coverage → promote to full file display. - const content = await ctx.vcs.getFileDiff(meta, f.path, { context: 10000 }); + const content = await ctx.provider.getFileDiff(meta, f.path, { context: 10000 }); pushDiff(out.diffs, f.path, "full_file", content); continue; } if (firstChange !== null && firstChange <= cfg.bigFileHeaderLines) { // Earliest change near top — widening context pulls in lines 1..change. - const content = await ctx.vcs.getFileDiff(meta, f.path, { context: cfg.bigFileHeaderLines }); + const content = await ctx.provider.getFileDiff(meta, f.path, { context: cfg.bigFileHeaderLines }); pushDiff(out.diffs, f.path, "big_file", content); continue; } diff --git a/src/stages/fetch/standards.ts b/src/stages/fetch/standards.ts index b9403b7..d99e77c 100644 --- a/src/stages/fetch/standards.ts +++ b/src/stages/fetch/standards.ts @@ -21,7 +21,7 @@ import { existsSync, readFileSync } from "node:fs"; import { join, resolve } from "node:path"; -import YAML from "yaml"; +import { parse as parseYaml } from "yaml"; import type { Logger } from "../../logger/index.ts"; @@ -52,7 +52,7 @@ export function loadStandards(standardsRoot: string, log: Logger): LoadedStandar log.warn("standards.index_missing", { path: indexPath }); return null; } - const index = YAML.parse(readFileSync(indexPath, "utf-8")) as { topics?: IndexEntry[] } | null; + const index = parseYaml(readFileSync(indexPath, "utf-8")) as { topics?: IndexEntry[] } | null; if (!index?.topics) return { topics: [] }; const topics: LoadedTopic[] = []; @@ -62,7 +62,7 @@ export function loadStandards(standardsRoot: string, log: Logger): LoadedStandar log.warn("standards.topic_missing", { file: entry.file }); continue; } - const parsed = YAML.parse(readFileSync(filePath, "utf-8")) as { rules?: Rule[] } | null; + const parsed = parseYaml(readFileSync(filePath, "utf-8")) as { rules?: Rule[] } | null; topics.push({ description: entry.description ?? entry.file, rules: parsed?.rules ?? [], diff --git a/src/stages/reconcile/index.ts b/src/stages/reconcile/index.ts index 57c7b25..78be21e 100644 --- a/src/stages/reconcile/index.ts +++ b/src/stages/reconcile/index.ts @@ -6,31 +6,19 @@ // working tree are marked addressed=true deterministically (no LLM). // 3. verify() — LLM judges the remaining threads. -import { existsSync, readFileSync } from "node:fs"; -import { resolve } from "node:path"; +import { existsSync } from "node:fs"; import type { Ctx } from "../../ctx.ts"; -import { callLLM, type Section } from "../../llm/call.ts"; -import type { PRComment } from "../../vcs/types.ts"; -import { hasRendering, renderRenderingContext } from "../shared.ts"; +import { errorMessage } from "../../lib/errors.ts"; +import { callLLM } from "../../llm/client.ts"; +import { isOpenBotThread, type PRComment } from "../../providers/types.ts"; +import { baseSystemSections, baseUserSections, llmStageMetric, loadPromptAssets } from "../shared.ts"; import type { StageState, ThreadDecision } from "../types.ts"; -const HERE = import.meta.dirname; -const PROMPT = readFileSync(resolve(HERE, "prompt.md"), "utf-8"); -const SCHEMA_TEXT = readFileSync(resolve(HERE, "schema.json"), "utf-8"); -const SCHEMA = JSON.parse(SCHEMA_TEXT) as object; +const ASSETS = loadPromptAssets(import.meta.dirname); const RENAME_BANNER_PREFIX = "⚠ **File renamed:** "; -const SLOT = { - outputSchema: "output_schema", - projectContext: "project_context", - prMetadata: "pr_metadata", - prDiff: "pr_diff", - prComments: "pr_comments", - renderingContext: "rendering_context", -} as const; - interface VerifyPayload { decisions: ThreadDecision[]; } @@ -39,8 +27,7 @@ function splitThreads(comments: PRComment[]): { auto: PRComment[]; rest: PRComme const auto: PRComment[] = []; const rest: PRComment[] = []; for (const c of comments) { - if (c.by !== "bot" || c.resolved || c.parentId) continue; - if (!c.inline) continue; + if (!isOpenBotThread(c)) continue; if (c.inline.path && !existsSync(c.inline.path)) auto.push(c); else rest.push(c); } @@ -88,13 +75,13 @@ async function handleRenames( `This was a prior bot finding on the old path; please verify ` + `whether it still applies in the new location.\n\n` + `**Original finding:**\n\n${c.body || ""}`; - await ctx.vcs.postInlineComment({ path: newPath, line: 1, body: notice }); + await ctx.provider.postInlineComment({ path: newPath, line: 1, body: notice }); } - await ctx.vcs.resolveThread(c.threadId ?? c.id); + await ctx.provider.resolveThread(c.threadId ?? c.id); count++; log.info(alreadyPosted ? "re_resolved" : "relocated", { thread: c.id, oldPath, newPath }); } catch (e) { - log.error("relocate.failed", { thread: c.id, oldPath, newPath, error: (e as Error).message }); + log.error("relocate.failed", { thread: c.id, oldPath, newPath, error: errorMessage(e) }); } } return count; @@ -116,10 +103,10 @@ async function applyResolutions( continue; } try { - await ctx.vcs.resolveThread(comment.threadId); + await ctx.provider.resolveThread(comment.threadId); resolved++; } catch (e) { - log.error("resolve.failed", { thread: comment.threadId, error: (e as Error).message }); + log.error("resolve.failed", { thread: comment.threadId, error: errorMessage(e) }); failed++; } } @@ -134,10 +121,10 @@ export async function runReconcile(ctx: Ctx, state: StageState): Promise 0) { const relocated = await handleRenames(ctx, comments, renames); - if (relocated > 0) comments = await ctx.vcs.getPRComments(); + if (relocated > 0) comments = await ctx.provider.getPRComments(); } const { auto, rest } = splitThreads(comments); @@ -147,7 +134,7 @@ export async function runReconcile(ctx: Ctx, state: StageState): Promise 0) { - const potential = await ctx.vcs.getPotentialRenames(state.meta); + const potential = await ctx.provider.getPotentialRenames(state.meta); if (potential.length > 0) { const potByOld = new Map(potential.map((p) => [p.oldPath, p])); for (const c of auto) { @@ -190,29 +177,15 @@ export async function runReconcile(ctx: Ctx, state: StageState): Promise({ ctx, stage: "reconcile", systemSections, userSections, - schema: SCHEMA, + schema: ASSETS.schema, }); const llmDecisions = result.validated.decisions ?? []; await applyResolutions(ctx, byCommentId, llmDecisions); @@ -221,12 +194,6 @@ export async function runReconcile(ctx: Ctx, state: StageState): Promise({ - ctx, stage: "review", systemSections, userSections, schema: SCHEMA, + ctx, stage: "review", systemSections, userSections, schema: ASSETS.schema, }); const findings = result.validated.comments; @@ -72,13 +42,7 @@ export async function runReview(ctx: Ctx, state: StageState): Promise try { await postFinding(ctx, log, c); } catch (e) { - log.error("post.failed", { path: c.path, error: (e as Error).message }); + log.error("post.failed", { path: c.path, error: errorMessage(e) }); } } } @@ -99,11 +63,11 @@ async function postFindings(ctx: Ctx, findings: ReviewFinding[]): Promise // Anchor the finding to a head-side line and post it. If the file is gone // we degrade to a top-level summary comment; if it exists but the snippet // can't be uniquely located we post on line 1 with a banner explaining why. -async function postFinding(ctx: Ctx, log: ReturnType, c: ReviewFinding): Promise { +async function postFinding(ctx: Ctx, log: Logger, c: ReviewFinding): Promise { if (!c.path || !existsSync(c.path)) { const body = wrapWithBanner(c.snippet, c.body, "anchor not located"); log.warn("anchor.unresolved", { path: c.path, fallback: "top_level" }); - await ctx.vcs.postSummaryComment(body); + await ctx.provider.postSummaryComment(body); return; } @@ -114,7 +78,7 @@ async function postFinding(ctx: Ctx, log: ReturnType, c: log.warn("anchor.unresolved", { path: c.path, fallback: "line_1" }); body = wrapWithBanner(c.snippet, c.body, "snippet not uniquely matched on head"); } - await ctx.vcs.postInlineComment({ path: c.path, line, body, severity: c.severity }); + await ctx.provider.postInlineComment({ path: c.path, line, body, severity: c.severity }); } // ---- anchor resolution -------------------------------------------------- diff --git a/src/stages/shared.ts b/src/stages/shared.ts index dda7a59..82b3706 100644 --- a/src/stages/shared.ts +++ b/src/stages/shared.ts @@ -1,6 +1,95 @@ -// Helpers shared between LLM-using stages (reconcile, review). +// Helpers shared between LLM-using stages (reconcile, review): prompt-asset +// loading, the slot-tag table, base section assembly, and metric construction +// (ADR-0010). Helpers encode the envelope (order + presence rules); stages +// keep policy — they append their stage-specific sections to the returned +// arrays. -import type { RenderingContext } from "./types.ts"; +import { readFileSync } from "node:fs"; +import { resolve } from "node:path"; + +import type { LLMResult, Section } from "../llm/types.ts"; +import type { PRComment } from "../providers/types.ts"; +import type { RenderingContext, StageMetric, StageState } from "./types.ts"; + +// ---- Prompt assets ------------------------------------------------------- + +export interface PromptAssets { + prompt: string; + schemaText: string; + schema: object; +} + +// Loads a stage's colocated prompt.md + schema.json (ADR-0006). Call at +// module init with import.meta.dirname — a missing asset fails fast at +// import time. +export function loadPromptAssets(dir: string): PromptAssets { + const prompt = readFileSync(resolve(dir, "prompt.md"), "utf-8"); + const schemaText = readFileSync(resolve(dir, "schema.json"), "utf-8"); + return { prompt, schemaText, schema: JSON.parse(schemaText) as object }; +} + +// ---- Prompt slots -------------------------------------------------------- + +// Every XML tag any stage may inject. One table so tag names can't drift +// between stages; prompts document the tags they consume. +export const SLOT = { + outputSchema: "output_schema", + projectContext: "project_context", + prMetadata: "pr_metadata", + prDiff: "pr_diff", + prComments: "pr_comments", + threadDecisions: "thread_decisions", + reviewChecklist: "review_checklist", + renderingContext: "rendering_context", +} as const; + +// System envelope: prompt → output_schema → project_context → (checklist) → +// rendering_context. Empty sections are omitted. +export function baseSystemSections( + assets: PromptAssets, + state: StageState, + opts?: { includeChecklist?: boolean }, +): Section[] { + const sections: Section[] = [ + { tag: "", content: assets.prompt }, + { tag: SLOT.outputSchema, content: assets.schemaText }, + ]; + if (state.context.projectContext.trim()) { + sections.push({ tag: SLOT.projectContext, content: state.context.projectContext }); + } + if (opts?.includeChecklist && state.context.reviewChecklist) { + sections.push({ tag: SLOT.reviewChecklist, content: state.context.reviewChecklist }); + } + if (hasRendering(state.context.rendering)) { + sections.push({ tag: SLOT.renderingContext, content: renderRenderingContext(state.context.rendering) }); + } + return sections; +} + +// User envelope: pr_metadata → pr_diff → pr_comments. The comment set is a +// parameter because stages scope it differently (review sends all comments; +// reconcile sends only the threads under judgment plus their replies). +export function baseUserSections(state: StageState, comments: PRComment[]): Section[] { + return [ + { tag: SLOT.prMetadata, content: JSON.stringify(state.meta, null, 2) }, + { tag: SLOT.prDiff, content: state.diff }, + { tag: SLOT.prComments, content: JSON.stringify(comments, null, 2) }, + ]; +} + +// ---- Metrics -------------------------------------------------------------- + +export function llmStageMetric(stage: string, t0: number, r: LLMResult): StageMetric { + return { + stage, + durationMs: Date.now() - t0, + tokens: { input: r.usage.promptTokens, output: r.usage.completionTokens, total: r.usage.totalTokens }, + healAttempts: r.healAttempts, + toolCalls: r.toolCalls, + }; +} + +// ---- Rendering context ---------------------------------------------------- // Renders a RenderingContext into the body of a slot. // Each sub-bucket becomes its own block; empty buckets are skipped. The LLM diff --git a/src/stages/summarize/index.ts b/src/stages/summarize/index.ts index 25ec854..ed06578 100644 --- a/src/stages/summarize/index.ts +++ b/src/stages/summarize/index.ts @@ -3,14 +3,18 @@ // state.verdict + (in cli.ts) the process exit code. import type { Ctx } from "../../ctx.ts"; -import type { PRComment } from "../../vcs/types.ts"; +import { errorMessage } from "../../lib/errors.ts"; +import { isOpenBotThread, type PRComment } from "../../providers/types.ts"; import type { ReviewFinding, StageState, ThreadDecision, Verdict, VerdictResult } from "../types.ts"; -const SUMMARY_FRAGMENTS = [ - "prior bot finding(s) still unaddressed", - "No reviewable source content", - "informational finding(s)", -]; +// Fragments that identify a bot comment as one of OUR summary comments. +// buildSummary interpolates these exact constants into the text it posts, +// and isPriorSummary matches on the same list — construction and detection +// can't drift apart (ADR-0012). Reword a block by editing its constant. +const FRAG_UNADDRESSED = "prior bot finding(s) still unaddressed"; +const FRAG_NO_SOURCE = "No reviewable source content"; +const FRAG_INFO = "informational finding(s)"; +const SUMMARY_FRAGMENTS = [FRAG_UNADDRESSED, FRAG_NO_SOURCE, FRAG_INFO]; function isPriorSummary(c: PRComment): boolean { if (c.by !== "bot" || c.inline) return false; @@ -39,7 +43,7 @@ function deriveVerdict(ctx: Ctx, state: StageState): VerdictDecision { // Reconcile didn't run. Treat every unresolved bot inline thread as // still pending — we don't know whether the head addresses them. unaddressed = state.comments - .filter((c) => c.by === "bot" && c.inline && !c.resolved && !c.parentId) + .filter(isOpenBotThread) .map((c) => ({ thread_id: c.threadId ?? c.id, comment_id: c.id, @@ -62,7 +66,7 @@ function clip(s: string, cap: number): string { function buildSummary(d: VerdictDecision, charLimit: number): string { const blocks: string[] = []; if (d.verdict === "reject" && d.unaddressed.length > 0) { - const lines = [`**${d.unaddressed.length} prior bot finding(s) still unaddressed:**`, ""]; + const lines = [`**${d.unaddressed.length} ${FRAG_UNADDRESSED}:**`, ""]; for (const dec of d.unaddressed) { const tid = dec.thread_id || dec.comment_id; const reason = clip((dec.reason || "").trim(), charLimit); @@ -72,12 +76,12 @@ function buildSummary(d: VerdictDecision, charLimit: number): string { } if (d.verdict === "reject" && d.noReviewableSource) { blocks.push( - "**No reviewable source content** in this PR — all changed files are binary or non-whitelisted (translations, lockfiles, assets, etc.). Bot can't read them; a human reviewer must confirm the change set is intentional before merge.", + `**${FRAG_NO_SOURCE}** in this PR — all changed files are binary or non-whitelisted (translations, lockfiles, assets, etc.). Bot can't read them; a human reviewer must confirm the change set is intentional before merge.`, ); } if (d.infoFindings.length > 0) { const lines = [ - `**${d.infoFindings.length} informational finding(s)** (nits / suggestions, not blocking — listed here as summary, not tracked as individual threads):`, + `**${d.infoFindings.length} ${FRAG_INFO}** (nits / suggestions, not blocking — listed here as summary, not tracked as individual threads):`, "", ]; for (const c of d.infoFindings) { @@ -115,29 +119,24 @@ export async function runSummarize(ctx: Ctx, state: StageState): Promise { it("auto_address_deleted: thread on a deleted file is auto-resolved (no LLM)", async () => { - const { ctx, vcs } = await buildTestCtx("reconcile/auto_address_deleted"); + const { ctx, provider } = await buildTestCtx("reconcile/auto_address_deleted"); let state = await runFetch(ctx); state = await runReconcile(ctx, state); @@ -20,7 +20,7 @@ describe("reconcile", () => { expect(decisions[0]!.addressed).toBe(true); expect(decisions[0]!.reason).toMatch(/no longer exists/i); - expect(vcs.resolvedThreads.some((r) => r.threadId === "t100")).toBe(true); + expect(provider.resolvedThreads.some((r) => r.threadId === "t100")).toBe(true); // No LLM call — reconcile's stage metric should be skipped. const m = state.metrics.find((x) => x.stage === "reconcile"); @@ -28,17 +28,17 @@ describe("reconcile", () => { }); it("renamed_file: rename banner posted on new path + old thread resolved (no LLM)", async () => { - const { ctx, vcs } = await buildTestCtx("reconcile/renamed_file"); + const { ctx, provider } = await buildTestCtx("reconcile/renamed_file"); let state = await runFetch(ctx); state = await runReconcile(ctx, state); // Notice posted on the new path at line 1. - const notice = vcs.postedInline.find((p) => p.c.path === "src/authentication.ts" && p.c.line === 1); + const notice = provider.postedInline.find((p) => p.c.path === "src/authentication.ts" && p.c.line === 1); expect(notice).toBeDefined(); expect(notice!.c.body).toMatch(/File renamed:.*src\/auth\.ts.*src\/authentication\.ts/s); // Original thread resolved. - expect(vcs.resolvedThreads.some((r) => r.threadId === "t200")).toBe(true); + expect(provider.resolvedThreads.some((r) => r.threadId === "t200")).toBe(true); // No LLM judgment needed for the renamed thread. const m = state.metrics.find((x) => x.stage === "reconcile"); @@ -46,29 +46,29 @@ describe("reconcile", () => { }); it("fix_applied: LLM marks thread addressed when the diff implements the fix", async () => { - const { ctx, vcs } = await buildTestCtx("reconcile/fix_applied"); + const { ctx, provider } = await buildTestCtx("reconcile/fix_applied"); let state = await runFetch(ctx); state = await runReconcile(ctx, state); const decision = (state.decisions ?? []).find((d) => d.comment_id === "500"); expect(decision).toBeDefined(); expect(decision!.addressed).toBe(true); - expect(vcs.resolvedThreads.some((r) => r.threadId === "t500")).toBe(true); + expect(provider.resolvedThreads.some((r) => r.threadId === "t500")).toBe(true); }); it("user_valid_reply: LLM accepts a verifiable dev explanation as addressed", async () => { - const { ctx, vcs } = await buildTestCtx("reconcile/user_valid_reply"); + const { ctx, provider } = await buildTestCtx("reconcile/user_valid_reply"); let state = await runFetch(ctx); state = await runReconcile(ctx, state); const decision = (state.decisions ?? []).find((d) => d.comment_id === "300"); expect(decision).toBeDefined(); expect(decision!.addressed).toBe(true); - expect(vcs.resolvedThreads.some((r) => r.threadId === "t300")).toBe(true); + expect(provider.resolvedThreads.some((r) => r.threadId === "t300")).toBe(true); }); it("user_invalid_reply: LLM rejects a bogus dev rebuttal — thread stays unaddressed", async () => { - const { ctx, vcs } = await buildTestCtx("reconcile/user_invalid_reply"); + const { ctx, provider } = await buildTestCtx("reconcile/user_invalid_reply"); let state = await runFetch(ctx); state = await runReconcile(ctx, state); @@ -76,6 +76,6 @@ describe("reconcile", () => { expect(decision).toBeDefined(); expect(decision!.addressed).toBe(false); // Unaddressed → thread NOT resolved. - expect(vcs.resolvedThreads.some((r) => r.threadId === "t400")).toBe(false); + expect(provider.resolvedThreads.some((r) => r.threadId === "t400")).toBe(false); }); }); diff --git a/tests/e2e/review.test.ts b/tests/e2e/review.test.ts index d5aa63b..17fb1ef 100644 --- a/tests/e2e/review.test.ts +++ b/tests/e2e/review.test.ts @@ -11,18 +11,18 @@ import { buildTestCtx } from "../helpers.ts"; describe("review", () => { it("clean: produces no blocking findings", async () => { - const { ctx, vcs } = await buildTestCtx("review/clean"); + const { ctx, provider } = await buildTestCtx("review/clean"); let state = await runFetch(ctx); state = await runReview(ctx, state); const blockingSeverities = new Set(ctx.config.review.verdictGuard.approveBlockingSeverities); const blocking = (state.findings ?? []).filter((f) => blockingSeverities.has(f.severity)); expect(blocking).toHaveLength(0); - expect(vcs.postedInline).toHaveLength(0); + expect(provider.postedInline).toHaveLength(0); }); it("exposed_secret: flags blocker findings + posts them inline", async () => { - const { ctx, vcs } = await buildTestCtx("review/exposed_secret"); + const { ctx, provider } = await buildTestCtx("review/exposed_secret"); let state = await runFetch(ctx); state = await runReview(ctx, state); @@ -31,18 +31,18 @@ describe("review", () => { const blockers = findings.filter((f) => f.severity === "blocker"); expect(blockers.length).toBeGreaterThanOrEqual(1); // Every blocker goes to inline post (review.ts filters by verdictGuard). - expect(vcs.postedInline.length).toBeGreaterThanOrEqual(blockers.length); - expect(vcs.postedInline.every((p) => p.c.path === "src/payments.ts")).toBe(true); + expect(provider.postedInline.length).toBeGreaterThanOrEqual(blockers.length); + expect(provider.postedInline.every((p) => p.c.path === "src/payments.ts")).toBe(true); }); it("dangerous_shell: flags at least one warning or blocker", async () => { - const { ctx, vcs } = await buildTestCtx("review/dangerous_shell"); + const { ctx, provider } = await buildTestCtx("review/dangerous_shell"); let state = await runFetch(ctx); state = await runReview(ctx, state); const elevated = (state.findings ?? []).filter((f) => f.severity === "warning" || f.severity === "blocker"); expect(elevated.length).toBeGreaterThan(0); - expect(vcs.postedInline.length).toBeGreaterThan(0); + expect(provider.postedInline.length).toBeGreaterThan(0); }); it("non_allowlisted_text: .jsx file lands in binary_files; LLM may Read it", async () => { diff --git a/tests/e2e/summarize.test.ts b/tests/e2e/summarize.test.ts index 1a5240e..994561d 100644 --- a/tests/e2e/summarize.test.ts +++ b/tests/e2e/summarize.test.ts @@ -1,38 +1,30 @@ // Summarize verdict logic — unit-style, no LLM, no fixture. // // Each test hand-builds a StageState, points runSummarize at a bare -// MockVcsProvider, and asserts on the returned verdict + recorded side effects. +// MockProvider, and asserts on the returned verdict + recorded side effects. import { describe, it, expect } from "vitest"; -import type { Config, Ctx } from "../../src/ctx.ts"; +import { REVIEW_DEFAULTS, type Config, type Ctx } from "../../src/ctx.ts"; import { MemoryLogger } from "../../src/logger/memory.ts"; import { runSummarize } from "../../src/stages/summarize/index.ts"; import { emptyRenderingContext, type StageState } from "../../src/stages/types.ts"; -import { MockVcsProvider } from "../../src/vcs/mock.ts"; -import type { PRComment } from "../../src/vcs/types.ts"; +import { MockProvider } from "../../src/providers/mock.ts"; +import type { PRComment } from "../../src/providers/types.ts"; // ---- Minimal Ctx + StageState builders --------------------------------- function makeCtx(opts: { charLimit?: number } = {}): Ctx { - const vcs = MockVcsProvider.empty(); + const provider = MockProvider.empty(); const config: Config = { pr: { url: "mock://summarize", provider: "mock" }, llm: { baseUrl: "", apiKey: "", model: "", temperature: 0.2, maxOutputTokens: 8192, healRetries: 2, debug: false }, github: { token: "" }, gitlab: { token: "", baseUrl: "" }, bitbucket: { token: "", baseUrl: "" }, - review: { - diffFilter: { includeExtensions: [], fullFileThresholdLines: 800, bigFileHeaderLines: 30, fullFileCoverageThreshold: 0.8, narrowContextLines: 3 }, - verdictGuard: { approveBlockingSeverities: ["warning", "blocker"] }, - standardsRoot: "", - projectContext: { source: "", sections: [] }, - reviewChecklistPath: "", - preloadedFileMaxBytes: 200_000, - summaryCommentCharLimit: opts.charLimit ?? 200, - }, + review: { ...REVIEW_DEFAULTS, summaryCommentCharLimit: opts.charLimit ?? 200 }, }; - return { config, vcs, logger: new MemoryLogger() }; + return { config, provider, logger: new MemoryLogger() }; } function baseState(overrides: Partial = {}): StageState { @@ -68,9 +60,9 @@ describe("summarize", () => { expect(out.verdict?.unaddressedThreadCount).toBe(0); expect(out.verdict?.fatal).toBe(false); - const vcs = ctx.vcs as MockVcsProvider; - expect(vcs.verdicts).toHaveLength(1); - expect(vcs.verdicts[0]!.verdict).toBe("approve"); + const recorder = ctx.provider as MockProvider; + expect(recorder.verdicts).toHaveLength(1); + expect(recorder.verdicts[0]!.verdict).toBe("approve"); }); it("requests changes when a blocker finding is present", async () => { @@ -146,8 +138,8 @@ describe("summarize", () => { }); await runSummarize(ctx, state); - const vcs = ctx.vcs as MockVcsProvider; - const summaryArg = vcs.verdicts[0]!.summary ?? ""; + const recorder = ctx.provider as MockProvider; + const summaryArg = recorder.verdicts[0]!.summary ?? ""; // ellipsis present → truncation happened expect(summaryArg).toContain("…"); // truncated chunk should be roughly cap+1 'a's (cap=50 + …) diff --git a/tests/helpers.ts b/tests/helpers.ts index b4f54d5..373ca33 100644 --- a/tests/helpers.ts +++ b/tests/helpers.ts @@ -4,16 +4,16 @@ // (LLM_URL, optionally LLM_KEY / LLM_MODEL). Each test: // - builds a Ctx targeting a mock:// URL, // - drives one or more stages, -// - asserts on what the MockVcsProvider received + on the returned +// - asserts on what the MockProvider received + on the returned // StageState. -import { buildCtx, type Ctx } from "../src/ctx.ts"; +import { buildCtx, REVIEW_DEFAULTS, type Ctx } from "../src/ctx.ts"; import { MemoryLogger } from "../src/logger/memory.ts"; -import type { MockVcsProvider } from "../src/vcs/mock.ts"; +import type { MockProvider } from "../src/providers/mock.ts"; export interface TestCtx { ctx: Ctx; - vcs: MockVcsProvider; + provider: MockProvider; logger: MemoryLogger; } @@ -28,16 +28,8 @@ export interface TestCtxOpts { export async function buildTestCtx(scenario: string, opts: TestCtxOpts = {}): Promise { const logger = new MemoryLogger(); const review = opts.includeExtensions !== undefined - ? { - diffFilter: { - includeExtensions: opts.includeExtensions, - fullFileThresholdLines: 800, - bigFileHeaderLines: 30, - fullFileCoverageThreshold: 0.8, - narrowContextLines: 3, - }, - } + ? { diffFilter: { ...REVIEW_DEFAULTS.diffFilter, includeExtensions: opts.includeExtensions } } : undefined; const ctx = await buildCtx({ prUrl: `mock://${scenario}`, logger, review }); - return { ctx, vcs: ctx.vcs as MockVcsProvider, logger }; + return { ctx, provider: ctx.provider as MockProvider, logger }; }