From 7694bd53c138f7fa83d0e5da7fbd1af96f7c2276 Mon Sep 17 00:00:00 2001 From: bilikas Date: Sun, 7 Jun 2026 16:07:12 +0300 Subject: [PATCH 1/6] dist added for devDependancies --- .dockerignore | 1 + .github/workflows/ci.yml | 35 +++++ README.md | 14 +- dist/cli.js | 113 +++++++++++++++ dist/ctx.js | 122 +++++++++++++++++ dist/lib/git.js | 97 +++++++++++++ dist/llm/call.js | 160 +++++++++++++++++++++ dist/llm/client.js | 20 +++ dist/llm/model.js | 24 ++++ dist/llm/stream.js | 96 +++++++++++++ dist/llm/tools/bash.js | 62 +++++++++ dist/llm/tools/index.js | 51 +++++++ dist/llm/tools/read.js | 73 ++++++++++ dist/llm/tools/shared.js | 7 + dist/logger/console.js | 44 ++++++ dist/logger/index.js | 4 + dist/logger/memory.js | 37 +++++ dist/stages/fetch/index.js | 221 ++++++++++++++++++++++++++++++ dist/stages/fetch/standards.js | 68 +++++++++ dist/stages/reconcile/index.js | 208 ++++++++++++++++++++++++++++ dist/stages/reconcile/prompt.md | 45 ++++++ dist/stages/reconcile/schema.json | 37 +++++ dist/stages/review/index.js | 173 +++++++++++++++++++++++ dist/stages/review/prompt.md | 52 +++++++ dist/stages/review/schema.json | 54 ++++++++ dist/stages/shared.js | 26 ++++ dist/stages/summarize/index.js | 148 ++++++++++++++++++++ dist/stages/types.js | 7 + dist/vcs/bitbucket.js | 152 ++++++++++++++++++++ dist/vcs/github.js | 195 ++++++++++++++++++++++++++ dist/vcs/gitlab.js | 170 +++++++++++++++++++++++ dist/vcs/mock.js | 205 +++++++++++++++++++++++++++ dist/vcs/types.js | 14 ++ examples/review.yml | 5 +- package.json | 9 +- scripts/copy-assets.mjs | 27 ++++ src/cli.ts | 2 +- tsconfig.build.json | 10 ++ 38 files changed, 2777 insertions(+), 11 deletions(-) create mode 100644 .github/workflows/ci.yml create mode 100644 dist/cli.js create mode 100644 dist/ctx.js create mode 100644 dist/lib/git.js create mode 100644 dist/llm/call.js create mode 100644 dist/llm/client.js create mode 100644 dist/llm/model.js create mode 100644 dist/llm/stream.js create mode 100644 dist/llm/tools/bash.js create mode 100644 dist/llm/tools/index.js create mode 100644 dist/llm/tools/read.js create mode 100644 dist/llm/tools/shared.js create mode 100644 dist/logger/console.js create mode 100644 dist/logger/index.js create mode 100644 dist/logger/memory.js create mode 100644 dist/stages/fetch/index.js create mode 100644 dist/stages/fetch/standards.js create mode 100644 dist/stages/reconcile/index.js create mode 100644 dist/stages/reconcile/prompt.md create mode 100644 dist/stages/reconcile/schema.json create mode 100644 dist/stages/review/index.js create mode 100644 dist/stages/review/prompt.md create mode 100644 dist/stages/review/schema.json create mode 100644 dist/stages/shared.js create mode 100644 dist/stages/summarize/index.js create mode 100644 dist/stages/types.js create mode 100644 dist/vcs/bitbucket.js create mode 100644 dist/vcs/github.js create mode 100644 dist/vcs/gitlab.js create mode 100644 dist/vcs/mock.js create mode 100644 dist/vcs/types.js create mode 100644 scripts/copy-assets.mjs create mode 100644 tsconfig.build.json diff --git a/.dockerignore b/.dockerignore index 6530c65..d67d4b2 100644 --- a/.dockerignore +++ b/.dockerignore @@ -1,4 +1,5 @@ node_modules +dist .ci-artifacts .git .old diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml new file mode 100644 index 0000000..444fad6 --- /dev/null +++ b/.github/workflows/ci.yml @@ -0,0 +1,35 @@ +# Guards the committed build output. Because dist/ is checked in (so consumers +# of the git dependency get prebuilt JS with no install-time build), it can +# drift from src/ if someone edits source without rebuilding. This job rebuilds +# and fails if the committed dist/ doesn't match. +name: CI + +on: + pull_request: + push: + branches: [main] + +jobs: + dist-fresh: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + + - uses: pnpm/action-setup@v4 + + - uses: actions/setup-node@v4 + with: + node-version: 24 + cache: pnpm + + - run: pnpm install --frozen-lockfile + + - run: pnpm typecheck + + - name: Rebuild dist and check it matches what's committed + run: | + pnpm build + if ! git diff --exit-code -- dist; then + echo "::error::dist/ is stale — run 'pnpm build' and commit the result." >&2 + exit 1 + fi diff --git a/README.md b/README.md index d80c960..f8d5e0d 100644 --- a/README.md +++ b/README.md @@ -33,17 +33,17 @@ The workflow builds the reviewer image from source on each run and triggers on e ## Quick start (npm dependency) -For Node projects, consume the reviewer as a `devDependency` instead of building the image — the CLI runs directly (no Docker, no build step; it executes its TypeScript at runtime). Works with npm, yarn, or pnpm. +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): ```json "devDependencies": { - "@reviewer/cli": "github:bilikaz/code-reviewer#v0.1.0" + "@reviewer/cli": "github:bilikaz/code-reviewer#v0.1.1" } ``` -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. +On install the package builds itself (a `prepare` step compiles `src/` to `dist/`), so the consumer ends up with plain JavaScript — Node refuses to run TypeScript from inside `node_modules`, so shipping compiled output is required. 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. Run it locally the same way: @@ -51,7 +51,7 @@ Run it locally the same way: npx reviewer run --pr https://github.com/owner/repo/pull/123 # yarn: yarn reviewer ... | pnpm: pnpm exec reviewer ... ``` -Requires Node ≥24 in the consumer (the CLI uses TS transforms at runtime). +Requires Node ≥24 in the consumer (used to build on install and to run). ## Local usage @@ -98,11 +98,15 @@ All config is env-only and read once at startup. See [.env.example](.env.example ## Development ```bash -pnpm install +pnpm install # also runs `prepare` -> builds dist/ pnpm typecheck +pnpm build # tsc -> dist/ (+ copies prompt.md / schema.json assets) +pnpm start --pr # run from source (node --experimental-transform-types) pnpm test # builds the test image, mounts node_modules, runs vitest ``` +The package ships compiled JavaScript: `tsconfig.build.json` emits `src/` to `dist/`, and [scripts/copy-assets.mjs](scripts/copy-assets.mjs) copies the runtime `.md`/`.json` assets next to the compiled modules. `bin` and `files` both point at `dist/`; `dist/` is gitignored and regenerated on install. The Docker image is the exception — it runs the CLI from `src/` directly via the entrypoint, so it skips the build (`--ignore-scripts`). + Tests require `tests/.env` (see [tests/.env.example](tests/.env.example)) — a real LLM endpoint is needed because the E2E suite drives the full pipeline against fixture repos via the `mock://` VCS provider. ### Layout diff --git a/dist/cli.js b/dist/cli.js new file mode 100644 index 0000000..affbdaf --- /dev/null +++ b/dist/cli.js @@ -0,0 +1,113 @@ +#!/usr/bin/env node +// Reviewer CLI. +// +// Subcommands: +// run — fetch → reconcile (if unresolved bot threads) → review → summarize +// fetch — fetch only +// reconcile — fetch + reconcile +// review — fetch + review (skips reconcile) +// summarize — fetch + summarize (skips review and reconcile; existing +// unresolved bot threads count as unaddressed) +// +// Flags: +// --pr required (or set PR_URL env) +// --llm-url overrides LLM_URL +// --llm-key overrides LLM_KEY +// --llm-model overrides LLM_MODEL (otherwise auto-detected) +import { buildCtx } from "./ctx.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"; +const FLAG_ALIASES = { + "--pr": "pr", + "--llm-url": "llmUrl", + "--llm-key": "llmKey", + "--llm-model": "llmModel", +}; +function parseArgs(argv) { + const command = argv[2]; + if (!command || command === "--help" || command === "-h") + usageAndExit(); + const flags = {}; + for (let i = 3; i < argv.length; i++) { + const a = argv[i]; + const key = FLAG_ALIASES[a]; + if (!key) + die(`unknown flag: ${a}`); + const val = argv[i + 1]; + if (val === undefined || val.startsWith("--")) + die(`flag ${a} requires a value`); + flags[key] = val; + i++; + } + return { command, flags }; +} +function usageAndExit() { + process.stderr.write("usage: reviewer --pr [--llm-url ] [--llm-key ] [--llm-model ]\n"); + process.exit(2); +} +function die(msg) { + process.stderr.write(`error: ${msg}\n`); + process.exit(2); +} +function buildOverrides(flags) { + const prUrl = flags.pr ?? process.env.PR_URL ?? ""; + if (!prUrl) + die("missing --pr (or PR_URL env)"); + return { + prUrl, + llmUrl: flags.llmUrl, + llmKey: flags.llmKey, + llmModel: flags.llmModel, + }; +} +// ---- Pipeline orchestration -------------------------------------------- +function hasUnresolvedBotThreads(state) { + return state.comments.some((c) => c.by === "bot" && c.inline && !c.resolved && !c.parentId); +} +async function runCommand(command, ctx) { + switch (command) { + case "run": { + let state = await runFetch(ctx); + if (hasUnresolvedBotThreads(state)) { + ctx.logger.info("pipeline.followup", { reason: "unresolved_bot_threads" }); + state = await runReconcile(ctx, state); + } + else { + ctx.logger.info("pipeline.initial"); + } + state = await runReview(ctx, state); + return runSummarize(ctx, state); + } + case "fetch": { + return runFetch(ctx); + } + case "reconcile": { + const state = await runFetch(ctx); + return runReconcile(ctx, state); + } + case "review": { + const state = await runFetch(ctx); + return runReview(ctx, state); + } + case "summarize": { + const state = await runFetch(ctx); + return runSummarize(ctx, state); + } + default: + die(`unknown command: ${command}`); + } +} +// ---- Entry -------------------------------------------------------------- +const { command, flags } = parseArgs(process.argv); +const ctx = await buildCtx(buildOverrides(flags)); +try { + const state = await runCommand(command, ctx); + if (state.verdict?.fatal) + process.exit(1); +} +catch (e) { + ctx.logger.error("cli.fatal", { error: e.message }); + process.exit(1); +} diff --git a/dist/ctx.js b/dist/ctx.js new file mode 100644 index 0000000..33fdb10 --- /dev/null +++ b/dist/ctx.js @@ -0,0 +1,122 @@ +// 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) +// - logger: structured logger (swap sinks here) +// +// Single rule: process.env is read ONLY in loadConfig(). Everything +// downstream takes values from the typed config object. Env arrives via +// Docker's --env-file (see package.json's `test` script); no host-side +// .env loading. +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 { MockVcsProvider } from "./vcs/mock.js"; +import { detectProvider } from "./vcs/types.js"; +// ---- Defaults ------------------------------------------------------------ +const REVIEW_DEFAULTS = { + diffFilter: { + includeExtensions: [], + fullFileThresholdLines: 800, + bigFileHeaderLines: 30, + fullFileCoverageThreshold: 0.8, + narrowContextLines: 3, + }, + verdictGuard: { approveBlockingSeverities: ["warning", "blocker"] }, + standardsRoot: "", + projectContext: { source: "", sections: [] }, + reviewChecklistPath: "", + preloadedFileMaxBytes: 200_000, + summaryCommentCharLimit: 200, +}; +// ---- Config loader (the ONLY place process.env is read) ----------------- +function pick(...vals) { + for (const v of vals) + if (v !== undefined && v !== "") + return v; + return ""; +} +function asNumber(v, fallback) { + if (v === undefined || v === "") + return fallback; + const n = Number(v); + return Number.isFinite(n) ? n : fallback; +} +function requireField(value, name, hint) { + if (!value) + throw new Error(`missing ${name}. ${hint}`); + return value; +} +export function loadConfig(opts) { + const e = process.env; + if (!opts.prUrl) + throw new Error("loadConfig: prUrl required"); + const provider = detectProvider(opts.prUrl); + const config = { + pr: { url: opts.prUrl, provider }, + llm: { + baseUrl: requireField(pick(opts.llmUrl, e.LLM_URL).replace(/\/$/, ""), "LLM_URL", "Pass --llm-url or set LLM_URL."), + // Optional — local self-hosted LLMs typically don't auth. When empty, + // the Authorization header is omitted from requests. + apiKey: pick(opts.llmKey, e.LLM_KEY), + model: pick(opts.llmModel, e.LLM_MODEL), // empty → auto-detected in buildCtx + temperature: asNumber(e.LLM_TEMPERATURE, 0.2), + // 32k by default — large enough for reasoning models that burn tokens + // on internal "thinking" before emitting the JSON output. Non-thinking + // 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", + }, + github: { token: pick(e.GITHUB_TOKEN, e.GH_TOKEN) }, + gitlab: { token: pick(e.GITLAB_TOKEN), baseUrl: pick(e.GITLAB_BASE_URL) }, + bitbucket: { token: pick(e.BITBUCKET_TOKEN), baseUrl: pick(e.BITBUCKET_BASE_URL) }, + review: { ...REVIEW_DEFAULTS, ...opts.review }, + }; + // Per-provider validation. Only the selected provider's credentials must be present. + switch (provider) { + case "github": + requireField(config.github.token, "GITHUB_TOKEN", "Set GITHUB_TOKEN (or GH_TOKEN)."); + break; + case "gitlab": + requireField(config.gitlab.token, "GITLAB_TOKEN", "Set GITLAB_TOKEN."); + break; + case "bitbucket": + requireField(config.bitbucket.token, "BITBUCKET_TOKEN", "Set BITBUCKET_TOKEN."); + break; + case "mock": + break; + } + return config; +} +// ---- VCS construction (plain dispatch — no env reads) ------------------- +function makeVcs(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); + } +} +// ---- buildCtx (thin orchestrator) --------------------------------------- +export async function buildCtx(opts) { + const config = loadConfig(opts); + const [vcs] = await Promise.all([ + makeVcs(config), + (async () => { + if (!config.llm.model) { + config.llm.model = await resolveModel({ + baseUrl: config.llm.baseUrl, + apiKey: config.llm.apiKey, + }); + } + })(), + ]); + return { + config, + vcs, + logger: opts.logger ?? new ConsoleLogger(), + }; +} diff --git a/dist/lib/git.js b/dist/lib/git.js new file mode 100644 index 0000000..eee4e5a --- /dev/null +++ b/dist/lib/git.js @@ -0,0 +1,97 @@ +// Local git invocation helper. Shared by VCS providers (GitHub/GitLab/ +// Bitbucket) when they need to drive `git diff` against the checked-out +// consuming repo to produce per-file diffs with controlled context width. +// +// The consuming repo is expected to be a real git checkout with both base +// and head SHAs available locally. +import { spawn } from "node:child_process"; +export async function runGit(args, cwd) { + return new Promise((resolve, reject) => { + const proc = spawn("git", args, { cwd: cwd ?? process.cwd(), stdio: ["ignore", "pipe", "pipe"] }); + let out = "", err = ""; + proc.stdout.on("data", (b) => { out += b.toString("utf-8"); }); + proc.stderr.on("data", (b) => { err += b.toString("utf-8"); }); + proc.on("close", (code) => { + if (code !== 0) { + reject(new Error(`git ${args.join(" ")} exited ${code}: ${err.trim().slice(0, 1024)}`)); + } + else { + resolve(out); + } + }); + proc.on("error", (e) => reject(new Error(`failed to spawn git: ${e.message}`))); + }); +} +export async function gitFileDiff(opts) { + // Three-dot syntax: diff between merge-base(base,head) and head — matches + // what GitHub/GitLab show on the PR. + return runGit(["diff", `-U${opts.context}`, `${opts.baseSha}...${opts.headSha}`, "--", opts.path], opts.cwd); +} +export async function gitChangedFiles(args) { + // -M: rename detection at git's default 50% similarity. Deliberately + // strict — if git isn't confident it's a rename, downstream treats the + // comment as being on a deleted file (auto-addressed). Loosening the + // threshold would misclassify refactors as renames and relocate bot + // comments inappropriately. --name-status: "A\tpath" / "M\tpath" / + // "D\tpath" / "R\told\tnew". + const raw = await runGit(["diff", "-M", "--name-status", `${args.baseSha}...${args.headSha}`], args.cwd); + const out = []; + for (const line of raw.split("\n")) { + if (!line.trim()) + continue; + const parts = line.split("\t"); + const code = parts[0]; + if (code === "A") + out.push({ path: parts[1], status: "added" }); + else if (code === "M") + out.push({ path: parts[1], status: "modified" }); + else if (code === "D") + out.push({ path: parts[1], status: "deleted" }); + else if (code.startsWith("R")) + out.push({ path: parts[2], status: "renamed", oldPath: parts[1] }); + // T (type change), C (copy), U (unmerged) — rare on PR diffs, skip. + } + return out; +} +export async function gitPotentialRenames(args) { + const raw = await runGit(["diff", "-M30", "--name-status", `${args.baseSha}...${args.headSha}`], args.cwd); + const out = []; + for (const line of raw.split("\n")) { + if (!line.startsWith("R")) + continue; + const parts = line.split("\t"); + const head = parts[0]; + // R\told\tnew + const score = parseInt(head.slice(1), 10); + if (!Number.isFinite(score) || score >= 50) + continue; // ≥50 = confident, already handled + out.push({ oldPath: parts[1], newPath: parts[2], similarityPct: score }); + } + return out; +} +// Counts head-side lines represented in a unified diff (context + added). +// Used by fetch's coverage check to decide whether a "big" file is so +// extensively changed that it's worth showing in full. +export function headLinesCovered(diff) { + let inHunk = false; + let count = 0; + for (const line of diff.split("\n")) { + if (line.startsWith("@@")) { + inHunk = true; + continue; + } + if (!inHunk) + continue; + if (line.startsWith("+++")) + continue; + if (line.startsWith("+") || line.startsWith(" ")) + count++; + // "-" lines are base-only; "\" are no-newline markers. + } + return count; +} +// New-side starting line of the first hunk in `diff`, or null if no hunk. +export function firstHunkNewLine(diff) { + const m = diff.match(/^@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@/m); + return m ? parseInt(m[1], 10) : null; +} diff --git a/dist/llm/call.js b/dist/llm/call.js new file mode 100644 index 0000000..f070e61 --- /dev/null +++ b/dist/llm/call.js @@ -0,0 +1,160 @@ +// 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()); + for (const c of candidates) { + try { + return JSON.parse(c); + } + catch { /* try next */ } + } + // Longest-span scan: shrink suffixes until JSON.parse succeeds. + 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 best.value; + throw new Error("no JSON object or array could be parsed from response"); +} +function healPrompt(error) { + return [ + "Your previous response could not be used:", + "", + ` Error: ${error}`, + "", + "Re-emit your response exactly conforming to the schema you were given.", + "Same content — only fix the structure. Output the JSON only, no prose, no 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 parsed; + try { + parsed = extractJson(finalText); + } + catch (e) { + lastError = `JSON parse: ${e.message}`; + } + if (parsed !== undefined) { + if (validate(parsed)) { + log.info("llm.done", { + prompt_tokens: totalPrompt, + completion_tokens: totalCompletion, + heal_attempts: healAttempts, + tool_calls: toolCalls, + }); + return { + validated: parsed, + usage: { promptTokens: totalPrompt, completionTokens: totalCompletion, totalTokens: totalAll }, + healAttempts, + toolCalls, + rawText: lastText, + }; + } + lastError = `schema: ${ajv.errorsText(validate.errors, { separator: "; " })}`; + } + 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 new file mode 100644 index 0000000..7d0aea0 --- /dev/null +++ b/dist/llm/client.js @@ -0,0 +1,20 @@ +// 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", + }; + 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; +} diff --git a/dist/llm/model.js b/dist/llm/model.js new file mode 100644 index 0000000..03ff663 --- /dev/null +++ b/dist/llm/model.js @@ -0,0 +1,24 @@ +// 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/stream.js b/dist/llm/stream.js new file mode 100644 index 0000000..8d4d5c6 --- /dev/null +++ b/dist/llm/stream.js @@ -0,0 +1,96 @@ +// SSE parser for OpenAI-compatible /chat/completions streams. +// +// 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) { + if (!resp.body) + throw new Error("LLM response has no body"); + const reader = resp.body.getReader(); + const decoder = new TextDecoder("utf-8"); + let buffer = ""; + const acc = { + content: "", + tool_calls: [], + finish_reason: null, + usage: null, + }; + // Tool calls arrive as fragments addressed by `index`. Build an array of + // partial pieces and finalize at the end. + const partial = []; + function processLine(line) { + // Returns true if [DONE] sentinel seen — caller can stop early. + if (!line.startsWith("data:")) + return false; + const payload = line.slice(5).trim(); + if (payload === "[DONE]") + return true; + if (!payload) + return false; + let chunk; + try { + chunk = JSON.parse(payload); + } + catch { + return false; // malformed line — skip + } + if (chunk.usage) + acc.usage = chunk.usage; + const choice = chunk.choices?.[0]; + if (!choice) + return false; + if (choice.finish_reason) + acc.finish_reason = choice.finish_reason; + const delta = choice.delta; + if (!delta) + return false; + if (typeof delta.content === "string" && delta.content.length > 0) { + acc.content += delta.content; + opts?.onText?.(delta.content); + } + if (delta.tool_calls) { + for (const tc of delta.tool_calls) { + const slot = (partial[tc.index] ??= { id: "", name: "", arguments: "" }); + if (tc.id) + slot.id = tc.id; + if (tc.function?.name) + slot.name += tc.function.name; + if (tc.function?.arguments) + slot.arguments += tc.function.arguments; + } + } + return false; + } + outer: while (true) { + const { done, value } = await reader.read(); + if (done) + break; + buffer += decoder.decode(value, { stream: true }); + // SSE messages are separated by blank lines; we process line-by-line. + let idx; + while ((idx = buffer.indexOf("\n")) >= 0) { + const line = buffer.slice(0, idx).replace(/\r$/, ""); + buffer = buffer.slice(idx + 1); + if (processLine(line)) + break outer; + } + } + // Flush any trailing partial line. + if (buffer.length > 0) + processLine(buffer); + for (const p of partial) { + if (p.id || p.name || p.arguments) { + acc.tool_calls.push({ + id: p.id, + type: "function", + function: { name: p.name, arguments: p.arguments }, + }); + } + } + return acc; +} diff --git a/dist/llm/tools/bash.js b/dist/llm/tools/bash.js new file mode 100644 index 0000000..0fbb472 --- /dev/null +++ b/dist/llm/tools/bash.js @@ -0,0 +1,62 @@ +// Bash tool — run a shell command in the working directory. +// +// The container is throwaway and isolated, so this tool doesn't sandbox +// 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"; +export const bashTool = { + schema: { + type: "function", + function: { + name: "Bash", + description: "Run a shell command in the working directory. Returns combined stdout+stderr plus the exit code. Use for `git log`, `git blame`, `git show`, `grep -rn`, `find`, listing dirs, etc. The container is throwaway — no path restrictions, but state does not persist between calls.", + parameters: { + type: "object", + additionalProperties: false, + properties: { + command: { type: "string", description: "The shell command to run. Executed via `bash -lc`." }, + timeout_seconds: { type: "number", description: "Optional. Defaults to 60." }, + }, + required: ["command"], + }, + }, + }, + async execute(args) { + const command = String(args.command ?? ""); + const timeoutMs = typeof args.timeout_seconds === "number" ? args.timeout_seconds * 1000 : 60_000; + if (!command) { + return { + ok: false, + output: `Bash call rejected: missing required \`command\` argument. ` + + `Got: ${JSON.stringify(args)}. ` + + `Example: {"command": "ls -la"}`, + }; + } + return runBash(command, timeoutMs); + }, +}; +function runBash(command, timeoutMs) { + return new Promise((resolve) => { + const proc = spawn("bash", ["-lc", command], { stdio: ["ignore", "pipe", "pipe"] }); + let out = ""; + let timedOut = false; + const timer = setTimeout(() => { + timedOut = true; + proc.kill("SIGKILL"); + }, timeoutMs); + proc.stdout.on("data", (b) => { out += b.toString("utf-8"); }); + proc.stderr.on("data", (b) => { out += b.toString("utf-8"); }); + proc.on("close", (code) => { + clearTimeout(timer); + const suffix = timedOut + ? `\n[exit: killed after ${timeoutMs}ms timeout]` + : `\n[exit: ${code}]`; + resolve({ ok: !timedOut && code === 0, output: cap(out) + suffix }); + }); + proc.on("error", (e) => { + clearTimeout(timer); + resolve({ ok: false, output: `error spawning bash: ${e.message}` }); + }); + }); +} diff --git a/dist/llm/tools/index.js b/dist/llm/tools/index.js new file mode 100644 index 0000000..70a9a89 --- /dev/null +++ b/dist/llm/tools/index.js @@ -0,0 +1,51 @@ +// LLM tool dispatcher. Each tool exposes a uniform { schema, execute } shape; +// this file just collects them and routes a tool_call to the right one by name. +// +// Error messages go straight back to the model in the tool-response message, +// so they spell out what went wrong, what we received, and what to do next. +// Smaller / weaker models recover well from explicit feedback; terse +// "error: X" messages leave them looping. Each tool's executor follows the +// same convention for its own missing-arg errors. +// +// Add a new tool: create tools/.ts exporting `Tool: Tool`, then +// add it to the TOOLS array below. +import { bashTool } from "./bash.js"; +import { readTool } from "./read.js"; +const TOOLS = [readTool, bashTool]; +const BY_NAME = new Map(TOOLS.map((t) => [t.schema.function.name, t])); +const VALID_NAMES = TOOLS.map((t) => t.schema.function.name); +export const TOOL_SCHEMAS = TOOLS.map((t) => t.schema); +export async function execTool(call) { + const name = call.function.name?.trim(); + if (!name) { + return { + ok: false, + output: `tool call rejected: empty function name. ` + + `Specify one of: ${VALID_NAMES.join(", ")}.`, + }; + } + let args; + try { + args = JSON.parse(call.function.arguments || "{}"); + } + catch (e) { + return { + ok: false, + output: [ + `tool call rejected: arguments are not valid JSON.`, + `Tool: ${name}`, + `Received arguments: ${call.function.arguments}`, + `Parse error: ${e.message}`, + `Retry with a valid JSON object matching the tool's schema.`, + ].join("\n"), + }; + } + const tool = BY_NAME.get(name); + if (!tool) { + return { + ok: false, + output: `tool call rejected: unknown tool "${name}". Specify one of: ${VALID_NAMES.join(", ")}.`, + }; + } + return tool.execute(args); +} diff --git a/dist/llm/tools/read.js b/dist/llm/tools/read.js new file mode 100644 index 0000000..2ccc235 --- /dev/null +++ b/dist/llm/tools/read.js @@ -0,0 +1,73 @@ +// Read tool — fetch a file's content from the working directory. +// +// Returns the first MAX_LINES lines numbered (1-based), plus a header with +// the visible range and total count. For longer files, appends a footer +// 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"; +const MAX_LINES = 300; +export const readTool = { + schema: { + type: "function", + function: { + name: "Read", + description: "Read a file from the working directory. Returns up to the first 300 lines, " + + "each prefixed with its line number, plus a header showing the visible range " + + "and total line count. For files longer than 300 lines, use Bash to fetch " + + "the rest with `sed -n 'START,ENDp' path`, `tail -n N path`, or " + + "`grep -n 'pattern' path`. The footer of every Read result includes a " + + "ready-to-copy sed command for the next chunk. Path is relative to the " + + "working directory.", + parameters: { + type: "object", + additionalProperties: false, + properties: { + path: { type: "string", description: "Path to the file, relative to the working directory." }, + }, + required: ["path"], + }, + }, + }, + async execute(args) { + const path = String(args.path ?? ""); + if (!path) { + return { + ok: false, + output: `Read call rejected: missing required \`path\` argument. ` + + `Got: ${JSON.stringify(args)}. ` + + `Example: {"path": "src/foo.ts"}`, + }; + } + try { + const content = await readFile(path, "utf-8"); + return { ok: true, output: cap(formatRead(path, content)) }; + } + catch (e) { + return { + ok: false, + output: `error reading ${path}: ${e.message}. ` + + `Check the path (relative to the working directory) and try Bash ls/find if unsure.`, + }; + } + }, +}; +function formatRead(path, content) { + const lines = content.split(/\r?\n/); + const total = lines.length; + const visible = lines.slice(0, MAX_LINES); + const width = String(visible.length).length; + const numbered = visible.map((l, i) => `${String(i + 1).padStart(width, " ")}: ${l}`).join("\n"); + const header = `# ${path} (lines 1-${visible.length} of ${total})`; + if (total <= MAX_LINES) + return `${header}\n${numbered}`; + const nextStart = MAX_LINES + 1; + const nextEnd = Math.min(total, MAX_LINES * 2); + return [ + header, + numbered, + ``, + `[... ${total - MAX_LINES} more lines]`, + `# Next chunk: Bash {"command": "sed -n '${nextStart},${nextEnd}p' ${path}"}`, + ].join("\n"); +} diff --git a/dist/llm/tools/shared.js b/dist/llm/tools/shared.js new file mode 100644 index 0000000..e70a2f8 --- /dev/null +++ b/dist/llm/tools/shared.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/logger/console.js b/dist/logger/console.js new file mode 100644 index 0000000..4415662 --- /dev/null +++ b/dist/logger/console.js @@ -0,0 +1,44 @@ +// Default sink: pretty indented output to stderr. Each event on its own +// line, structured `data` rendered as indented JSON below it. +const LEVEL_TAG = { + debug: "DBG", + info: "INF", + warn: "WRN", + error: "ERR", +}; +function render(level, scope, event, data) { + const tag = LEVEL_TAG[level]; + const prefix = scope ? `[${tag} ${scope}]` : `[${tag}]`; + if (!data || Object.keys(data).length === 0) { + return `${prefix} ${event}\n`; + } + const body = JSON.stringify(data, null, 2) + .split("\n") + .map((l) => ` ${l}`) + .join("\n"); + return `${prefix} ${event}\n${body}\n`; +} +export class ConsoleLogger { + scope; + constructor(scope) { + this.scope = scope; + } + debug(event, data) { + process.stderr.write(render("debug", this.scope, event, data)); + } + info(event, data) { + process.stderr.write(render("info", this.scope, event, data)); + } + warn(event, data) { + process.stderr.write(render("warn", this.scope, event, data)); + } + error(event, data) { + process.stderr.write(render("error", this.scope, event, data)); + } + stream(chunk) { + process.stderr.write(chunk); + } + child(scope) { + return new ConsoleLogger(this.scope ? `${this.scope}.${scope}` : scope); + } +} diff --git a/dist/logger/index.js b/dist/logger/index.js new file mode 100644 index 0000000..1aa76db --- /dev/null +++ b/dist/logger/index.js @@ -0,0 +1,4 @@ +// Logger interface + factory. Swap sinks by passing a different Logger +// instance into the Ctx; everything downstream uses the interface only. +export { ConsoleLogger } from "./console.js"; +export { MemoryLogger } from "./memory.js"; diff --git a/dist/logger/memory.js b/dist/logger/memory.js new file mode 100644 index 0000000..4efdab5 --- /dev/null +++ b/dist/logger/memory.js @@ -0,0 +1,37 @@ +// In-memory sink. Records every event for test assertions. `stream` chunks +// are concatenated into a single field per scope. +export class MemoryLogger { + scope; + entries; + streamBuffer; + constructor(scope, sharedEntries, sharedStreamRef) { + this.scope = scope; + this.entries = sharedEntries ?? []; + this._streamRef = sharedStreamRef ?? { value: "" }; + this.streamBuffer = ""; + } + _streamRef; + record(level, event, data) { + this.entries.push({ + level, + ts: new Date().toISOString(), + scope: this.scope, + event, + ...(data ? { data } : {}), + }); + } + debug(event, data) { this.record("debug", event, data); } + info(event, data) { this.record("info", event, data); } + warn(event, data) { this.record("warn", event, data); } + error(event, data) { this.record("error", event, data); } + stream(chunk) { + this._streamRef.value += chunk; + } + get streamedText() { + return this._streamRef.value; + } + child(scope) { + const next = this.scope ? `${this.scope}.${scope}` : scope; + return new MemoryLogger(next, this.entries, this._streamRef); + } +} diff --git a/dist/stages/fetch/index.js b/dist/stages/fetch/index.js new file mode 100644 index 0000000..3a92534 --- /dev/null +++ b/dist/stages/fetch/index.js @@ -0,0 +1,221 @@ +// fetch — builds the initial StageState from VCS + local repo. Pure setup; +// no LLM, no mutations to the PR. +// +// Per-file diff strategy (mirrors the Python reference): +// - Small file (lines ≤ fullFileThresholdLines): full file with ±/space +// markers via `-U10000`. Goes in diffs with type="full_file". +// - Big file: +// - High coverage (≥ fullFileCoverageThreshold): promote to `-U10000`, +// treat as full_file. +// - First change at line ≤ bigFileHeaderLines: widen to +// `-U`; git's context naturally pulls in the +// file top. type="big_file". +// - Change deeper than header range: narrow diff + plain head text. +// type="big_file". +// - Binary / non-source extension: listed by path in binary_files. When +// old/new sizes are suspiciously close, attach a warning — file may +// actually be text misclassified due to a missing extension. +import { existsSync, readFileSync, statSync } from "node:fs"; +import { extname } from "node:path"; +import { firstHunkNewLine, headLinesCovered, runGit } from "../../lib/git.js"; +import { loadStandards, renderStandards } from "./standards.js"; +import { emptyRenderingContext, } from "../types.js"; +const BINARY_SIZE_SIMILARITY_THRESHOLD = 0.8; +export async function runFetch(ctx) { + const t0 = Date.now(); + 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(), + ]); + const changed = await ctx.vcs.getChangedFiles(meta); + const rendering = await assembleRendering(ctx, meta, changed, log); + const standards = loadStandards(ctx.config.review.standardsRoot, log); + const standardsBlock = renderStandards(standards); + let inlineProjectContext = ""; + const pc = ctx.config.review.projectContext; + if (pc.source && existsSync(pc.source)) { + inlineProjectContext = readFileSync(pc.source, "utf-8"); + if (pc.sections.length > 0) + inlineProjectContext = sliceSections(inlineProjectContext, pc.sections); + } + const projectContext = [inlineProjectContext, standardsBlock].filter(Boolean).join("\n\n"); + let reviewChecklist = ""; + if (ctx.config.review.reviewChecklistPath) { + const p = ctx.config.review.reviewChecklistPath; + if (existsSync(p)) + reviewChecklist = readFileSync(p, "utf-8"); + else + log.warn("checklist.missing", { path: p }); + } + // Legacy concatenated diff text for the slot (still used by + // some prompts as the "main view"). Derived from the rendering entries. + const concatenated = rendering.diffs.map((d) => d.content).join("\n"); + log.info("done", { + files: changed.length, + diffs: rendering.diffs.length, + binary_files: rendering.binary_files.length, + comments: comments.length, + }); + return { + meta, + diff: concatenated, + comments, + context: { + projectContext, + reviewChecklist, + workingDir: { cwd: process.cwd() }, + rendering, + }, + metrics: [{ stage: "fetch", durationMs: Date.now() - t0 }], + }; +} +// ---- per-file diff assembly --------------------------------------------- +async function assembleRendering(ctx, meta, changed, log) { + const cfg = ctx.config.review.diffFilter; + const exts = new Set(cfg.includeExtensions.map((e) => e.toLowerCase())); + const out = emptyRenderingContext(); + for (const f of changed) { + // Non-whitelisted extension → binary bucket. Compare sizes; if they're + // similar enough the file might be text the operator forgot to allow. + if (exts.size > 0 && !exts.has(extname(f.path).toLowerCase())) { + const entry = await buildBinaryEntry(meta, f); + if (entry.suspicious) { + const ratio = Math.min(entry.oldSize, entry.size) / Math.max(entry.oldSize, entry.size); + log.warn("binary.suspicious", { + path: f.path, + old_bytes: entry.oldSize, + new_bytes: entry.size, + similarity_pct: Math.round(ratio * 100), + note: "old/new sizes nearly identical — file may be text whose extension isn't in includeExtensions", + }); + } + out.binary_files.push(entry); + continue; + } + // 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 }); + pushDiff(out.diffs, f.path, "full_file", content); + continue; + } + const headLineCount = countLinesOnDisk(f.path); + if (headLineCount === null) { + out.binary_files.push({ path: f.path, size: 0, oldSize: 0 }); + continue; + } + // Small file → inline full file. + if (headLineCount <= cfg.fullFileThresholdLines) { + const content = await ctx.vcs.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 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 }); + 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 }); + pushDiff(out.diffs, f.path, "big_file", content); + continue; + } + // Change is deep in the file: keep the narrow diff and append a plain + // head preview so the LLM still sees the file's top-level shape. + const head = readFirstNLines(f.path, cfg.bigFileHeaderLines); + const composed = [ + narrow, + "", + `--- HEAD PREVIEW: ${f.path} (lines 1–${head.split("\n").length} of ${headLineCount}) ---`, + head, + ].join("\n"); + pushDiff(out.diffs, f.path, "big_file", composed); + } + return out; +} +async function buildBinaryEntry(meta, f) { + const newSize = tryFileSize(f.path); + const oldSize = await tryOldSize(meta.baseSha, f.oldPath ?? f.path); + const entry = { path: f.path, size: newSize, oldSize }; + if (f.status === "modified" && oldSize > 0 && newSize > 0) { + const ratio = Math.min(oldSize, newSize) / Math.max(oldSize, newSize); + if (ratio >= BINARY_SIZE_SIMILARITY_THRESHOLD) { + entry.suspicious = true; + } + } + return entry; +} +function pushDiff(bucket, path, type, content) { + bucket.push({ path, type, content, lineCount: content.split("\n").length, size: content.length }); +} +function tryFileSize(path) { + try { + if (!existsSync(path)) + return 0; + return statSync(path).size; + } + catch { + return 0; + } +} +async function tryOldSize(baseSha, path) { + try { + const out = (await runGit(["cat-file", "-s", `${baseSha}:${path}`])).trim(); + const n = parseInt(out, 10); + return Number.isFinite(n) ? n : 0; + } + catch { + return 0; + } +} +function countLinesOnDisk(path) { + try { + if (!existsSync(path)) + return null; + return readFileSync(path, "utf-8").split("\n").length; + } + catch { + return null; + } +} +function readFirstNLines(path, n) { + try { + if (!existsSync(path) || n <= 0) + return ""; + return readFileSync(path, "utf-8").split("\n").slice(0, n).join("\n"); + } + catch { + return ""; + } +} +function sliceSections(text, sections) { + const lines = text.split("\n"); + const out = []; + let keep = false; + let keepDepth = 0; + for (const line of lines) { + const m = line.match(/^(#+)\s+(.+?)\s*$/); + if (m) { + const depth = m[1].length; + const heading = `${"#".repeat(depth)} ${m[2]}`; + if (sections.includes(heading)) { + keep = true; + keepDepth = depth; + out.push(line); + continue; + } + if (keep && depth <= keepDepth) + keep = false; + } + if (keep) + out.push(line); + } + return out.join("\n").trim(); +} diff --git a/dist/stages/fetch/standards.js b/dist/stages/fetch/standards.js new file mode 100644 index 0000000..c5084a2 --- /dev/null +++ b/dist/stages/fetch/standards.js @@ -0,0 +1,68 @@ +// Standards loader — reads an index.yml + topic yamls under standardsRoot +// and renders them into a single block for the system prompt. +// +// Expected YAML shape: +// +// # /index.yml +// topics: +// - file: security.yml +// description: Secrets handling, input validation, injection prevention +// - file: errors.yml +// description: Error handling and propagation conventions +// +// # /security.yml +// rules: +// - rule: Never interpolate user input directly into SQL strings. +// example: | +// // bad +// db.query("SELECT * FROM users WHERE id = '" + userId + "'"); +// // good +// db.query("SELECT * FROM users WHERE id = ?", [userId]); +import { existsSync, readFileSync } from "node:fs"; +import { join, resolve } from "node:path"; +import YAML from "yaml"; +export function loadStandards(standardsRoot, log) { + if (!standardsRoot) + return null; + const root = resolve(process.cwd(), standardsRoot); + const indexPath = join(root, "index.yml"); + if (!existsSync(indexPath)) { + log.warn("standards.index_missing", { path: indexPath }); + return null; + } + const index = YAML.parse(readFileSync(indexPath, "utf-8")); + if (!index?.topics) + return { topics: [] }; + const topics = []; + for (const entry of index.topics) { + const filePath = join(root, entry.file); + if (!existsSync(filePath)) { + log.warn("standards.topic_missing", { file: entry.file }); + continue; + } + const parsed = YAML.parse(readFileSync(filePath, "utf-8")); + topics.push({ + description: entry.description ?? entry.file, + rules: parsed?.rules ?? [], + }); + } + return { topics }; +} +export function renderStandards(loaded) { + if (!loaded || loaded.topics.length === 0) + return ""; + const lines = ["## Standards"]; + for (const t of loaded.topics) { + lines.push(`\n### ${t.description}`); + for (const r of t.rules) { + lines.push(`- ${r.rule}`); + if (r.example) { + lines.push(" Example:"); + lines.push(" ```"); + lines.push(r.example.split("\n").map((l) => ` ${l}`).join("\n")); + lines.push(" ```"); + } + } + } + return lines.join("\n"); +} diff --git a/dist/stages/reconcile/index.js b/dist/stages/reconcile/index.js new file mode 100644 index 0000000..ec7f206 --- /dev/null +++ b/dist/stages/reconcile/index.js @@ -0,0 +1,208 @@ +// reconcile — judges prior bot inline threads against the new diff. Three phases: +// +// 1. Rename relocation — for any thread on an old path that was renamed, +// post a notice on the new path at line 1 and resolve the orphan. +// 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); +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) + continue; + if (c.inline.path && !existsSync(c.inline.path)) + auto.push(c); + else + rest.push(c); + } + return { auto, rest }; +} +function gatherReplies(comments, threadIds) { + return comments.filter((c) => c.parentId && threadIds.has(c.parentId)); +} +function hasRenameNotice(comments, newPath, oldPath) { + const expectedPrefix = `${RENAME_BANNER_PREFIX}\`${oldPath}\` → \`${newPath}\``; + for (const c of comments) { + if (c.by !== "bot") + continue; + if (!c.inline || c.inline.path !== newPath || c.inline.line !== 1) + continue; + if ((c.body || "").startsWith(expectedPrefix)) + return true; + } + return false; +} +async function handleRenames(ctx, comments, renames) { + if (!Object.keys(renames).length) + return 0; + const log = ctx.logger.child("reconcile.rename"); + let count = 0; + for (const c of comments) { + if (c.by !== "bot" || c.resolved) + continue; + if (!c.inline) + continue; + const oldPath = c.inline.path; + if (!oldPath || !(oldPath in renames)) + continue; + const newPath = renames[oldPath]; + if (!existsSync(newPath)) { + log.warn("target.missing", { thread: c.id, oldPath, newPath }); + continue; + } + const alreadyPosted = hasRenameNotice(comments, newPath, oldPath); + try { + if (!alreadyPosted) { + const notice = `${RENAME_BANNER_PREFIX}\`${oldPath}\` → \`${newPath}\`. ` + + `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.vcs.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 }); + } + } + return count; +} +async function applyResolutions(ctx, byCommentId, decisions) { + const log = ctx.logger.child("reconcile.resolve"); + let resolved = 0, failed = 0, skipped = 0; + for (const d of decisions) { + if (!d.addressed) + continue; + const comment = byCommentId.get(d.comment_id); + if (!comment?.threadId) { + log.warn("missing_thread_id", { comment_id: d.comment_id }); + skipped++; + continue; + } + try { + await ctx.vcs.resolveThread(comment.threadId); + resolved++; + } + catch (e) { + log.error("resolve.failed", { thread: comment.threadId, error: e.message }); + failed++; + } + } + if (resolved || skipped || failed) { + log.info("summary", { resolved, skipped, failed }); + } +} +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); + if (Object.keys(renames).length > 0) { + const relocated = await handleRenames(ctx, comments, renames); + if (relocated > 0) + comments = await ctx.vcs.getPRComments(); + } + const { auto, rest } = splitThreads(comments); + const byCommentId = new Map(comments.map((c) => [c.id, c])); + // Before auto-addressing threads on vanished files, warn if any of those + // 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); + if (potential.length > 0) { + const potByOld = new Map(potential.map((p) => [p.oldPath, p])); + for (const c of auto) { + const oldPath = c.inline?.path; + if (oldPath && potByOld.has(oldPath)) { + const p = potByOld.get(oldPath); + log.warn("potential_rename.auto_address", { + thread: c.id, + oldPath, + newPathGuess: p.newPath, + similarityPct: p.similarityPct, + note: "git's similarity score is below the 50% confident-rename threshold; treating as deletion. If this is in fact a rename, the original concern may still apply on the new path — verify manually.", + }); + } + } + } + } + const autoDecisions = auto.map((c) => ({ + thread_id: c.threadId ?? c.id, + comment_id: c.id, + addressed: true, + reason: `File \`${c.inline?.path}\` no longer exists in the PR head; the concern no longer applies.`, + })); + if (auto.length > 0) + log.info("auto_addressed", { count: auto.length }); + await applyResolutions(ctx, byCommentId, autoDecisions); + if (rest.length === 0) { + log.info("no_llm_needed", { reason: auto.length > 0 ? "all_paths_gone" : "no_unresolved_bot_threads" }); + return { + ...state, + comments, + decisions: autoDecisions, + metrics: [...state.metrics, { stage: "reconcile", durationMs: Date.now() - t0, skipped: true }], + }; + } + 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 result = await callLLM({ + ctx, + stage: "reconcile", + systemSections, + userSections, + schema: SCHEMA, + }); + const llmDecisions = result.validated.decisions ?? []; + await applyResolutions(ctx, byCommentId, llmDecisions); + return { + ...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, + }], + }; +} diff --git a/dist/stages/reconcile/prompt.md b/dist/stages/reconcile/prompt.md new file mode 100644 index 0000000..1321b0a --- /dev/null +++ b/dist/stages/reconcile/prompt.md @@ -0,0 +1,45 @@ +# Verify fixes + +You are running the verify pre-step of a follow-up code review. Your job: judge which prior bot inline threads have been addressed by the new diff. Emit JSON conforming exactly to `` — nothing else. + +## Inputs + +- `` — authoritative. +- `` — own-code boundary, conventions, infrastructure, project standards. +- `` — `{title, description, author, ...}`. +- `` — unified diff concatenated across files (legacy view). Authoritative per-file content lives in ``. +- `` — structured PR content: + - `` contains `` blocks. `full_file` shows every line of a small changed file (with `+`/`-`/` ` markers — do NOT `Read` these). `big_file` shows the head plus partial hunks; `Read` only when you need regions outside what's shown. + - `` lists changed binary / non-source files as ``. `suspicious="true"` flags a file fetch thinks is likely text misclassified as binary — informational, no action needed in this stage. + - `` contains `` blocks with `` and `` children. +- `` — two kinds of entries: + - **(a) unresolved bot inline threads** (`by == "bot"`, no `parent_id`, has `inline`) — judge each one. + - **(b) reply comments** (any author, `parent_id` points at a thread's `id`) — treat as context for the parent thread; **do not emit a decision for a reply**. + +`inline.side: "head"` is anchored to a surviving head-side line. `inline.side: "base"` is anchored to a base-side (deleted) line. + +You can call `Read` to inspect any file in the working directory (e.g. a caller, fixture, config) and `Bash` for things like `git log`, `git show`, `grep -rn`. Use them sparingly — most of what you need is already in ``. + +## Emitting decisions + +For each judged bot inline thread emit one decision: +- `addressed: true` with a 1–2 sentence `reason` citing the line(s) or change that resolved it. +- `addressed: false` with a 1–2 sentence `reason` stating why the concern still stands. + +Keep `reason` short — these aren't review bodies, they're approve/reject judgments. No essays, no quoting whole hunks, no restating the original finding. + +Set both `thread_id` and `comment_id` to the prior comment's `id` as a string. + +For `inline.side: "base"` threads: `addressed: true` if the code stayed deleted, `addressed: false` if the deletion was reverted. + +## What counts as addressed + +- The new code does what the bot asked for (input validation added, raw SQL replaced with a prepared query, escape applied, etc.). +- A developer reply explains why the concern doesn't apply AND the explanation is technically correct (verify against the diff or a file you Read). +- The flagged code has been deleted or moved out of scope. + +If a change touches the file but doesn't address the specific concern, mark `addressed: false`. + +When in doubt, mark `addressed: false` — including dev replies you can't independently verify. + +Output the JSON object only. No prose, no fences. diff --git a/dist/stages/reconcile/schema.json b/dist/stages/reconcile/schema.json new file mode 100644 index 0000000..0a5d1a8 --- /dev/null +++ b/dist/stages/reconcile/schema.json @@ -0,0 +1,37 @@ +{ + "$schema": "https://json-schema.org/draft-07/schema#", + "title": "verify_fixes", + "description": "Per-prior-bot-thread judgments of whether the new diff addresses each finding. Mirrors review.json's wrapper-object shape so artifacts read symmetrically across stages.", + "type": "object", + "required": ["decisions"], + "additionalProperties": false, + "properties": { + "decisions": { + "type": "array", + "items": { + "type": "object", + "required": ["thread_id", "comment_id", "addressed", "reason"], + "additionalProperties": false, + "properties": { + "thread_id": { + "type": "string", + "description": "VCS thread id (string form of the numeric id, or GraphQL node id)." + }, + "comment_id": { + "type": "string", + "description": "Same as thread_id." + }, + "addressed": { + "type": "boolean", + "description": "True iff the new diff resolves the concern." + }, + "reason": { + "type": "string", + "minLength": 1, + "description": "1–2 sentences max. Cite line(s) or change when addressed=true; why the concern still stands when addressed=false. No essays, no quoting whole hunks." + } + } + } + } + } +} diff --git a/dist/stages/review/index.js b/dist/stages/review/index.js new file mode 100644 index 0000000..ed9da76 --- /dev/null +++ b/dist/stages/review/index.js @@ -0,0 +1,173 @@ +// review — runs the actual code review. Assembles the prompt with PR data, +// calls the LLM, anchors each finding's snippet to a head-side line, and +// posts inline review comments via the VCS provider. +// +// Anchor resolution: LLMs miscount line numbers but quote source faithfully, +// 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", +}; +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) }, + ]; + 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, + }); + const findings = result.validated.comments; + await postFindings(ctx, findings); + log.info("done", { findings: findings.length }); + 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, + }], + }; +} +async function postFindings(ctx, findings) { + const log = ctx.logger.child("review.post"); + const blocking = new Set(ctx.config.review.verdictGuard.approveBlockingSeverities); + for (const c of findings) { + if (!blocking.has(c.severity)) + continue; + try { + await postFinding(ctx, log, c); + } + catch (e) { + log.error("post.failed", { path: c.path, error: e.message }); + } + } +} +// 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, log, c) { + if (!c.path || !existsSync(c.path)) { + const body = wrapWithBanner(c.anchor.snippet, c.body, "anchor not located"); + log.warn("anchor.unresolved", { path: c.path, fallback: "top_level" }); + await ctx.vcs.postSummaryComment(body); + return; + } + let line = findUniqueMatch(c.path, c.anchor); + let body = c.body; + if (line == null) { + line = 1; + log.warn("anchor.unresolved", { path: c.path, fallback: "line_1" }); + body = wrapWithBanner(c.anchor.snippet, c.body, "snippet not uniquely matched on head"); + } + await ctx.vcs.postInlineComment({ path: c.path, line, body }); +} +function normalizeLines(text) { + // Strip leading whitespace per line (prompt instructs the LLM to do the + // same on its end, so the comparison is whitespace-insensitive at the + // line head). + return text.split(/\r?\n/).map((l) => l.replace(/^\s+/, "")); +} +function findAllMatches(haystack, needle) { + if (needle.length === 0) + return []; + const out = []; + for (let i = 0; i <= haystack.length - needle.length; i++) { + let ok = true; + for (let j = 0; j < needle.length; j++) { + if (haystack[i + j] !== needle[j]) { + ok = false; + break; + } + } + if (ok) + out.push(i); + } + return out; +} +// Returns the 1-based head-side line if the snippet matches exactly one place +// (using context_before/after to disambiguate), else null. +function findUniqueMatch(path, anchor) { + const fileLines = normalizeLines(readFileSync(path, "utf-8")); + const snippetLines = normalizeLines(anchor.snippet).filter((l) => l !== ""); + if (snippetLines.length === 0) + return null; + const matches = findAllMatches(fileLines, snippetLines); + if (matches.length === 0) + return null; + if (matches.length === 1) + return matches[0] + 1; + // Multiple matches — disambiguate with context_before/after. + const before = anchor.context_before ? normalizeLines(anchor.context_before).filter((l) => l !== "") : []; + const after = anchor.context_after ? normalizeLines(anchor.context_after).filter((l) => l !== "") : []; + const filtered = matches.filter((i) => { + if (before.length > 0) { + const start = i - before.length; + if (start < 0) + return false; + for (let j = 0; j < before.length; j++) { + if (fileLines[start + j] !== before[j]) + return false; + } + } + if (after.length > 0) { + const start = i + snippetLines.length; + if (start + after.length > fileLines.length) + return false; + for (let j = 0; j < after.length; j++) { + if (fileLines[start + j] !== after[j]) + return false; + } + } + return true; + }); + return filtered.length === 1 ? filtered[0] + 1 : null; +} +function wrapWithBanner(snippet, body, reason) { + const quoted = snippet.split("\n").slice(0, 6).join("\n"); + return [ + `> [!NOTE] Could not anchor this comment inline (${reason}). Snippet:`, + "", + "```", + quoted, + "```", + "", + body, + ].join("\n"); +} diff --git a/dist/stages/review/prompt.md b/dist/stages/review/prompt.md new file mode 100644 index 0000000..9972ad2 --- /dev/null +++ b/dist/stages/review/prompt.md @@ -0,0 +1,52 @@ +# Code review + +You are reviewing a pull request. Emit JSON conforming exactly to `` — nothing else. + +## Inputs + +You receive (some sections may be absent on a given run): + +- `` — the JSON shape you must emit. Authoritative. +- `` — own-code boundary, conventions, infrastructure, and the project's standards (always-applied inlined; an index of additional topics may be listed). Apply them. +- `` — `{title, description, author, baseBranch, headBranch, ...}`. +- `` — unified diff concatenated across files (legacy view). Authoritative per-file content lives in ``. +- `` — existing PR comments as `{id, by: "bot"|"human", author, body, created, inline?: {path, line, side}}`. +- `` — followup runs only: prior bot threads already judged; do not re-review them. +- `` — structured PR content the harness pre-rendered for you: + - `` contains `` blocks. `full_file` shows every line of a small changed file (with `+`/`-`/` ` markers — do NOT `Read` these). `big_file` shows the head plus partial hunks; `Read` the file only when you need regions outside what's shown. + - `` lists changed binary / non-source files as ``. **`suspicious="true"`** means fetch flagged this file: old and new byte sizes are nearly identical, but a real binary edit (image, font, archive) usually shuffles most bytes — so the file is likely text whose extension isn't in the project's allowlist. When you see this flag, raise an `info` finding suggesting the operator add the extension to `includeExtensions` so future PRs review the file normally. + - `` contains `` blocks with `` and `` children. + +## Review priorities (in order) + +1. **Correctness** — edge cases, off-by-ones, wrong assumptions, unhandled errors. +2. **Security** — input validation, output escaping, auth checks on state-mutating endpoints, exposed credentials, SQL/HTML injection. +3. **Maintainability** — unclear naming, hidden side effects, tangled responsibilities, duplicated logic, misleading intent. +4. **Performance** — queries inside loops, work on every request, missing caches, accidental N+1. +5. **Project standards** — apply standards from ``. + +## Anchoring rule + +Every finding anchors to a snippet you can quote verbatim from ``, ``, or a file you Read. The harness — not you — converts the snippet to a line number. Include `context_before` / `context_after` only when the snippet alone is ambiguous (e.g. a repeated `}` or import). + +Findings about an **absent** thing ("should also validate Y") anchor to the most relevant existing line; explain in `body`. + +## Severity (mirror ``) + +- `info` — nit, naming, doc drift, style. Every maintainability concern lands here, even when citing a standard. +- `warning` — likely runtime bug, measurable perf smell, security smell short of exploitable. +- `blocker` — exploitable security, data-loss risk, correctness bug certain to break production. + +If you're tempted to label maintainability as `warning`, it's `info`. Reserve `warning` for things that COULD CAUSE A RUNTIME PROBLEM. + +## What NOT to emit + +Emit a finding only when you want the author to change or reconsider something. Do NOT post comments that praise, approve, confirm, summarize, or restate a change — silence is the correct response to code that is fine. An empty `comments` array is a valid and expected result. `info` is for a nit worth fixing, never for "this looks good". + +## `body` content + +One short paragraph. Cite the reason (correctness / security / perf / maintainability / a specific standard or checklist item). No headers, no bullet lists inside `body`, no markdown beyond inline code spans. + +## Output + +A single JSON object matching ``. No prose, no markdown fences. diff --git a/dist/stages/review/schema.json b/dist/stages/review/schema.json new file mode 100644 index 0000000..05e7881 --- /dev/null +++ b/dist/stages/review/schema.json @@ -0,0 +1,54 @@ +{ + "$schema": "https://json-schema.org/draft-07/schema#", + "title": "review", + "description": "Inline review findings, anchored by code snippet (not line number).", + "type": "object", + "required": ["comments"], + "additionalProperties": false, + "properties": { + "comments": { + "type": "array", + "items": { + "type": "object", + "required": ["path", "anchor", "body", "severity"], + "additionalProperties": false, + "properties": { + "path": { + "type": "string", + "description": "Repo-relative path of the file the comment is about." + }, + "anchor": { + "type": "object", + "required": ["snippet"], + "additionalProperties": false, + "properties": { + "snippet": { + "type": "string", + "minLength": 1, + "description": "Verbatim line(s) from the file. The harness strips leading whitespace; internal whitespace must match." + }, + "context_before": { + "type": "string", + "description": "Optional. Line(s) immediately before snippet, to disambiguate when snippet repeats." + }, + "context_after": { + "type": "string", + "description": "Optional. Line(s) immediately after snippet; same role as context_before." + } + } + }, + "body": { + "type": "string", + "minLength": 1, + "description": "One short paragraph; cite the standard or reason. No headers." + }, + "severity": { + "type": "string", + "enum": ["info", "warning", "blocker"], + "description": "info = nit / maintainability / readability. warning = likely runtime bug. blocker = must fix before merge (exploitable security, data-loss risk, correctness bug certain to break production). Reserve warning for things that COULD CAUSE A RUNTIME PROBLEM; if you're tempted to label maintainability as warning, it's info." + } + } + } + } + } +} diff --git a/dist/stages/shared.js b/dist/stages/shared.js new file mode 100644 index 0000000..d249e82 --- /dev/null +++ b/dist/stages/shared.js @@ -0,0 +1,26 @@ +// Helpers shared between LLM-using stages (reconcile, review). +// 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. +export function renderRenderingContext(rc) { + const blocks = []; + if (rc.diffs.length > 0) { + const items = rc.diffs.map((d) => `\n${d.content}\n`); + blocks.push(`\n${items.join("\n\n")}\n`); + } + if (rc.binary_files.length > 0) { + const items = rc.binary_files.map((b) => { + const tag = b.suspicious ? ` suspicious="true"` : ""; + return ``; + }); + blocks.push(`\n${items.join("\n")}\n`); + } + if (rc.conventions.length > 0) { + const items = rc.conventions.map((c) => `\n ${c.context}\n ${c.example}\n`); + blocks.push(`\n${items.join("\n")}\n`); + } + return blocks.join("\n\n"); +} +export function hasRendering(rc) { + return rc.diffs.length + rc.binary_files.length + rc.conventions.length > 0; +} diff --git a/dist/stages/summarize/index.js b/dist/stages/summarize/index.js new file mode 100644 index 0000000..22981cb --- /dev/null +++ b/dist/stages/summarize/index.js @@ -0,0 +1,148 @@ +// 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)", +]; +function isPriorSummary(c) { + if (c.by !== "bot" || c.inline) + return false; + return SUMMARY_FRAGMENTS.some((f) => c.body.includes(f)); +} +function deriveVerdict(ctx, state) { + const blockingSeverities = new Set(ctx.config.review.verdictGuard.approveBlockingSeverities); + const findings = state.findings ?? []; + const blocking = findings.filter((c) => blockingSeverities.has(c.severity)); + const infoFindings = findings.filter((c) => !blockingSeverities.has(c.severity)); + let unaddressed; + if (state.decisions !== undefined) { + unaddressed = state.decisions.filter((d) => !d.addressed); + } + else { + // 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) + .map((c) => ({ + thread_id: c.threadId ?? c.id, + comment_id: c.id, + addressed: false, + reason: "Reconcile did not run; prior bot finding has not been re-judged against the current head.", + })); + } + const noReviewableSource = !/^diff --git /m.test(state.diff); + const verdict = blocking.length || unaddressed.length || noReviewableSource ? "reject" : "approve"; + return { verdict, blocking, unaddressed, infoFindings, noReviewableSource }; +} +function clip(s, cap) { + return s.length > cap ? s.slice(0, cap) + "…" : s; +} +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:**`, ""]; + for (const dec of d.unaddressed) { + const tid = dec.thread_id || dec.comment_id; + const reason = clip((dec.reason || "").trim(), charLimit); + lines.push(`- thread \`${tid}\` — ${reason}`); + } + 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."); + } + 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):`, + "", + ]; + for (const c of d.infoFindings) { + const path = c.path || "?"; + const first = c.body.split("\n")[0] ?? ""; + lines.push(`- \`${path}\` — ${clip(first, charLimit)}`); + } + blocks.push(lines.join("\n")); + } + return blocks.join("\n\n"); +} +function totals(state) { + let tokens = 0, healAttempts = 0; + for (const m of state.metrics) { + if (m.tokens) + tokens += m.tokens.total; + healAttempts += m.healAttempts ?? 0; + } + return { tokens, healAttempts }; +} +export async function runSummarize(ctx, state) { + const t0 = Date.now(); + const log = ctx.logger.child("summarize"); + const d = deriveVerdict(ctx, state); + log.info("verdict.derived", { + verdict: d.verdict, + blocking: d.blocking.length, + unaddressed: d.unaddressed.length, + no_reviewable_source: d.noReviewableSource, + }); + let deleted = 0; + for (const c of state.comments) { + if (!isPriorSummary(c)) + continue; + try { + await ctx.vcs.deleteComment(c.id, "issue"); + deleted++; + } + catch (e) { + log.warn("delete.prior_summary_failed", { id: c.id, error: e.message }); + } + } + if (deleted) + log.info("prior_summaries.deleted", { count: deleted }); + const summary = buildSummary(d, ctx.config.review.summaryCommentCharLimit); + let verdictError = null; + if (d.verdict === "approve") { + try { + await ctx.vcs.approve(summary || undefined); + } + 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; + } + } + // Fatal gates CI: approve verdict is the only non-fatal outcome. + // reject / unknown / verdict-call failure → exit 1. + const fatal = d.verdict !== "approve" || verdictError !== null; + if (fatal) { + if (verdictError) + log.error("fatal.verdict_failed", { verdict: d.verdict, error: verdictError.message }); + else if (d.verdict === "unknown") + log.error("fatal.no_review_result"); + else if (d.verdict === "reject") + log.error("fatal.rejected"); + } + const verdict = { + verdict: d.verdict, + findingsCount: (state.findings ?? []).length, + blockingFindingsCount: d.blocking.length, + unaddressedThreadCount: d.unaddressed.length, + noReviewableSource: d.noReviewableSource, + fatal, + totals: totals(state), + }; + return { + ...state, + verdict, + metrics: [...state.metrics, { stage: "summarize", durationMs: Date.now() - t0 }], + }; +} diff --git a/dist/stages/types.js b/dist/stages/types.js new file mode 100644 index 0000000..e5a92a1 --- /dev/null +++ b/dist/stages/types.js @@ -0,0 +1,7 @@ +// Pipeline state — uniform shape passed through every stage. Each stage +// takes StageState, returns StageState. Stages mutate `comments` and +// `context.preloadedFiles`; everything else is additive (set once by its +// producing stage). +export function emptyRenderingContext() { + return { diffs: [], binary_files: [], conventions: [] }; +} diff --git a/dist/vcs/bitbucket.js b/dist/vcs/bitbucket.js new file mode 100644 index 0000000..846f66b --- /dev/null +++ b/dist/vcs/bitbucket.js @@ -0,0 +1,152 @@ +// Bitbucket implementation of VcsProvider. 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; + owner; + repo; + number; + botLogin; + name = "bitbucket"; + constructor(token, baseUrl, owner, repo, number, botLogin) { + this.token = token; + this.baseUrl = baseUrl; + this.owner = owner; + this.repo = repo; + this.number = number; + this.botLogin = botLogin; + } + static async create(config) { + const { url } = config.pr; + 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); + } + prPath() { + return `/repositories/${this.owner}/${this.repo}/pullrequests/${this.number}`; + } + async getPRMetadata() { + const pr = await this.api(this.prPath()); + return { + title: pr.title, + description: pr.description ?? "", + author: pr.author.username, + baseBranch: pr.destination.branch.name, + headBranch: pr.source.branch.name, + baseSha: pr.destination.commit.hash, + headSha: pr.source.commit.hash, + url: pr.links.html.href, + 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`); + for (const c of comments.values ?? []) { + const body = c.content?.raw ?? c.body?.raw ?? ""; + out.push({ + id: String(c.id), + threadId: String(c.id), + by: c.author.username === this.botLogin ? "bot" : "human", + author: c.author.username, + body, + createdAt: c.created_on, + inline: c.inline + ? { path: c.inline.path, line: c.inline.line?.to ?? c.inline.line?.from ?? 0, side: "head" } + : undefined, + parentId: c.parent ? String(c.parent.id) : undefined, + resolved: c.resolved, + }); + } + return out; + } + async postInlineComment(c) { + await this.api(`${this.prPath()}/comments`, { + method: "POST", + body: JSON.stringify({ + content: { raw: c.body }, + inline: { path: c.path, to: c.line, type: "inline" }, + }), + }); + } + async postSummaryComment(body) { + await this.api(`${this.prPath()}/comments`, { + method: "POST", + body: JSON.stringify({ content: { raw: body } }), + }); + } + async approve(summary) { + if (summary) + await this.postSummaryComment(summary); + await this.api(`${this.prPath()}/approve`, { method: "POST" }); + } + async reject(summary) { + // Bitbucket has no native request_changes action — only the comment lands. + if (summary) + await this.postSummaryComment(summary); + } + async resolveThread(threadId) { + await this.api(`${this.prPath()}/comments/${threadId}`, { + method: "PUT", + body: JSON.stringify({ resolved: true }), + }); + } + async deleteComment(commentId, _kind) { + await this.api(`${this.prPath()}/comments/${commentId}`, { method: "DELETE" }); + } + async getRenames(_meta) { + const diff = await this.api(`${this.prPath()}/diff`); + const out = {}; + for (const file of diff.values ?? []) { + if (file.status === "renamed" && file.old?.path && file.new?.path) { + out[file.old.path] = file.new.path; + } + } + return out; + } +} diff --git a/dist/vcs/github.js b/dist/vcs/github.js new file mode 100644 index 0000000..a9cbab0 --- /dev/null +++ b/dist/vcs/github.js @@ -0,0 +1,195 @@ +// GitHub implementation of VcsProvider. 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). +// +// Token needs: +// - `pull-requests: write` to comment + submit reviews +// - `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 { + client; + owner; + repo; + number; + botLogin; + name = "github"; + constructor(client, owner, repo, number, botLogin) { + this.client = client; + this.owner = owner; + this.repo = repo; + this.number = number; + this.botLogin = botLogin; + } + static async create(config, octokit) { + const { url } = config.pr; + const { token } = config.github; + if (!token) + throw new Error("GitHubProvider: token required"); + const m = url.match(/^https?:\/\/github\.com\/([^/]+)\/([^/]+)\/pull\/(\d+)/); + if (!m) + throw new Error(`GitHubProvider: not a GitHub PR URL: ${url}`); + const client = octokit ?? new Octokit({ auth: token }); + // Under Actions the GITHUB_TOKEN is an App installation token — + // GET /user returns 403 — but the bot identity is fixed. Elsewhere + // (PATs etc.) we query /user normally. + const botLogin = process.env.GITHUB_ACTIONS === "true" + ? "github-actions[bot]" + : (await client.users.getAuthenticated()).data.login; + return new GitHubProvider(client, m[1], m[2], parseInt(m[3], 10), botLogin); + } + async getPRMetadata() { + const { data } = await this.client.pulls.get({ + owner: this.owner, repo: this.repo, pull_number: this.number, + }); + return { + title: data.title, + description: data.body ?? "", + author: data.user?.login ?? "(unknown)", + baseBranch: data.base.ref, + headBranch: data.head.ref, + baseSha: data.base.sha, + headSha: data.head.sha, + url: data.html_url, + 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, { + owner: this.owner, repo: this.repo, issue_number: this.number, per_page: 100, + }); + for (const c of issueComments) { + out.push({ + id: String(c.id), + by: c.user?.login === this.botLogin ? "bot" : "human", + author: c.user?.login ?? "(unknown)", + body: c.body ?? "", + createdAt: c.created_at, + }); + } + const threads = await this.loadReviewThreads(); + const reviewComments = await this.client.paginate(this.client.pulls.listReviewComments, { + owner: this.owner, repo: this.repo, pull_number: this.number, per_page: 100, + }); + for (const c of reviewComments) { + const thread = threads.byCommentDbId.get(c.id); + out.push({ + id: String(c.id), + threadId: thread?.id, + by: c.user?.login === this.botLogin ? "bot" : "human", + author: c.user?.login ?? "(unknown)", + body: c.body, + createdAt: c.created_at, + inline: c.line + ? { path: c.path, line: c.line, side: c.side === "LEFT" ? "base" : "head" } + : undefined, + parentId: c.in_reply_to_id ? String(c.in_reply_to_id) : undefined, + resolved: thread?.isResolved ?? false, + }); + } + return out; + } + async postInlineComment(c) { + const meta = await this.getPRMetadata(); + await this.client.pulls.createReviewComment({ + owner: this.owner, repo: this.repo, pull_number: this.number, + body: c.body, commit_id: meta.headSha, + path: c.path, line: c.line, side: "RIGHT", + }); + } + async postSummaryComment(body) { + await this.client.issues.createComment({ + owner: this.owner, repo: this.repo, issue_number: this.number, body, + }); + } + async approve(summary) { + // Bots don't formally approve on GitHub — the Actions token is blocked + // from self-approval on public repos as a security guardrail, and even + // automation accounts shouldn't carry that authority. Post the summary + // as a COMMENT review when there's something to say; otherwise stay + // silent (matches GitLab/Bitbucket behavior). The workflow's exit + // code is the gate either way. + if (!summary) + return; + await this.client.pulls.createReview({ + owner: this.owner, repo: this.repo, pull_number: this.number, + event: "COMMENT", + body: summary, + }); + } + async reject(summary) { + // REQUEST_CHANGES makes the verdict visible on the PR, so always post + // it — even if there's no summary text (blocking findings live in + // inline comments). GitHub 422s on empty body, hence the fallback. + await this.client.pulls.createReview({ + owner: this.owner, repo: this.repo, pull_number: this.number, + event: "REQUEST_CHANGES", + body: summary || "Changes requested — see inline comments.", + }); + } + async resolveThread(threadId) { + await this.client.graphql(`mutation($id: ID!) { + resolveReviewThread(input: { threadId: $id }) { thread { id isResolved } } + }`, { id: threadId }); + } + async deleteComment(commentId, kind) { + const id = parseInt(commentId, 10); + if (kind === "issue") { + await this.client.issues.deleteComment({ owner: this.owner, repo: this.repo, comment_id: id }); + } + else { + await this.client.pulls.deleteReviewComment({ owner: this.owner, repo: this.repo, comment_id: id }); + } + } + async getRenames(meta) { + const resp = await this.client.repos.compareCommits({ + owner: this.owner, repo: this.repo, base: meta.baseSha, head: meta.headSha, + }); + const out = {}; + for (const file of resp.data.files ?? []) { + if (file.status === "renamed" && file.filename && file.previous_filename) { + out[file.previous_filename] = file.filename; + } + } + return out; + } + async loadReviewThreads() { + const byCommentDbId = new Map(); + let cursor = null; + while (true) { + const resp = await this.client.graphql(`query($owner:String!,$repo:String!,$n:Int!,$cursor:String) { + repository(owner:$owner, name:$repo) { + pullRequest(number:$n) { + reviewThreads(first: 100, after: $cursor) { + pageInfo { hasNextPage endCursor } + nodes { id isResolved comments(first: 100) { nodes { databaseId } } } + } + } + } + }`, { owner: this.owner, repo: this.repo, n: this.number, cursor }); + const page = resp.repository.pullRequest.reviewThreads; + for (const t of page.nodes) { + for (const c of t.comments.nodes) { + if (c.databaseId != null) { + byCommentDbId.set(c.databaseId, { id: t.id, isResolved: t.isResolved }); + } + } + } + if (!page.pageInfo.hasNextPage) + break; + cursor = page.pageInfo.endCursor; + } + return { byCommentDbId }; + } +} diff --git a/dist/vcs/gitlab.js b/dist/vcs/gitlab.js new file mode 100644 index 0000000..394143b --- /dev/null +++ b/dist/vcs/gitlab.js @@ -0,0 +1,170 @@ +// GitLab implementation of VcsProvider. Plain fetch — no official SDK needed +// for the subset we use. +// +// Token needs: +// - `api` scope to read/write MR comments and submit reviews +// - `read_repository` scope to read diffs +// +// `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; + projectPath; + number; + botLogin; + name = "gitlab"; + constructor(token, baseUrl, projectPath, // {host}/{owner}/{repo} + number, botLogin) { + this.token = token; + this.baseUrl = baseUrl; + this.projectPath = projectPath; + this.number = number; + this.botLogin = botLogin; + } + static async create(config) { + const { url } = config.pr; + const { token, baseUrl } = config.gitlab; + if (!token) + throw new Error("GitLabProvider: token required"); + // {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 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); + } + mrPath() { + return `/projects/${encodeURIComponent(this.projectPath)}/merge_requests/${this.number}`; + } + async getPRMetadata() { + const mr = await this.api(this.mrPath()); + return { + title: mr.title, + description: mr.description ?? "", + author: mr.author.username, + baseBranch: mr.target_branch, + headBranch: mr.source_branch, + baseSha: mr.diff_refs.base_sha, + headSha: mr.diff_refs.head_sha, + url: mr.web_url, + 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`); + for (const n of notes) { + if (n.position) + continue; + out.push({ + id: String(n.id), + by: n.author.username === this.botLogin ? "bot" : "human", + author: n.author.username, + body: n.body, + createdAt: n.created_at, + resolved: n.resolvable ? n.resolved : undefined, + threadId: n.thread_id ? String(n.thread_id) : undefined, + }); + } + for (const n of notes.filter((nn) => nn.position != null)) { + const pos = n.position; + const line = pos.new_line ?? pos.old_line ?? 0; + out.push({ + id: String(n.id), + threadId: n.thread_id ? String(n.thread_id) : undefined, + by: n.author.username === this.botLogin ? "bot" : "human", + author: n.author.username, + body: n.body, + createdAt: n.created_at, + inline: { path: pos.new_path || pos.old_path, line, side: "head" }, + parentId: n.parent_id ? String(n.parent_id) : undefined, + resolved: n.resolvable ? n.resolved : undefined, + }); + } + return out; + } + async postInlineComment(c) { + const meta = await this.getPRMetadata(); + await this.api(`${this.mrPath()}/discussions`, { + method: "POST", + body: JSON.stringify({ + body: c.body, + position: { + base_sha: meta.baseSha, + start_sha: meta.baseSha, + head_sha: meta.headSha, + position_type: "text", + new_path: c.path, + new_line: c.line, + }, + }), + }); + } + async postSummaryComment(body) { + await this.api(`${this.mrPath()}/notes`, { + method: "POST", + body: JSON.stringify({ body }), + }); + } + async approve(summary) { + if (summary) + await this.postSummaryComment(summary); + await this.api(`${this.mrPath()}/approve`, { method: "POST" }); + } + async reject(summary) { + // GitLab has no native request_changes action — only the comment lands. + if (summary) + await this.postSummaryComment(summary); + } + async resolveThread(threadId) { + await this.api(`${this.mrPath()}/discussions/${threadId}/resolve`, { method: "PUT" }); + } + async deleteComment(commentId, _kind) { + await this.api(`${this.mrPath()}/notes/${commentId}`, { method: "DELETE" }); + } + async getRenames(_meta) { + const changes = await this.api(`${this.mrPath()}/changes`); + const out = {}; + for (const file of changes.changes ?? []) { + if (file.renamed_to && file.renamed_from) { + out[file.renamed_from] = file.renamed_to; + } + else if (file.old_path && file.new_path && file.old_path !== file.new_path) { + out[file.old_path] = file.new_path; + } + } + return out; + } +} diff --git a/dist/vcs/mock.js b/dist/vcs/mock.js new file mode 100644 index 0000000..8f0f659 --- /dev/null +++ b/dist/vcs/mock.js @@ -0,0 +1,205 @@ +// Mock VCS provider. For tests + local debugging via `mock://` URLs. +// +// Setup trick: instead of re-implementing diff/rename detection in TS, the +// mock spins up a throwaway local git repo: +// +// 1. Create a temp dir. +// 2. Copy tests/fixtures//old/ → repo, commit ("base"). +// 3. Replace working tree with tests/fixtures//new/, commit ("head"). +// 4. Chdir to the temp repo so stages' relative file reads resolve there. +// +// After setup, getChangedFiles / getFileDiff delegate to the same gitChanged- +// Files / gitFileDiff helpers the real providers use — no parallel diff +// algorithm to maintain, renames detected by `git -M` automatically. +import { spawnSync } from "node:child_process"; +import { cpSync, existsSync, mkdtempSync, readFileSync, readdirSync, rmSync } from "node:fs"; +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"; +function findProjectRoot() { + let dir = dirname(fileURLToPath(import.meta.url)); + for (let i = 0; i < 10; i++) { + if (existsSync(join(dir, "tests", "fixtures"))) + return dir; + const parent = dirname(dir); + if (parent === dir) + break; + dir = parent; + } + return join(dirname(fileURLToPath(import.meta.url)), "..", ".."); +} +function loadFixtureConfig(scenarioPath) { + const p = join(scenarioPath, "config.yaml"); + if (!existsSync(p)) + return {}; + try { + return parseYaml(readFileSync(p, "utf-8")); + } + catch (e) { + console.warn(`mock: failed to parse ${p}: ${e.message}`); + return {}; + } +} +// ---- Temp git repo setup ------------------------------------------------ +function git(cwd, ...args) { + const r = spawnSync("git", args, { cwd, encoding: "utf-8" }); + if (r.status !== 0) { + throw new Error(`mock: git ${args.join(" ")} failed (exit ${r.status}): ${(r.stderr || "").trim()}`); + } + return (r.stdout || "").trim(); +} +function rmAllExceptGit(dir) { + for (const entry of readdirSync(dir, { withFileTypes: true })) { + if (entry.name === ".git") + continue; + rmSync(join(dir, entry.name), { recursive: true, force: true }); + } +} +function setupRepo(scenarioPath) { + const cwd = mkdtempSync(join(tmpdir(), "reviewer-mock-")); + git(cwd, "init", "-q", "--initial-branch=main"); + git(cwd, "config", "user.email", "mock@reviewer.local"); + git(cwd, "config", "user.name", "mock"); + git(cwd, "config", "commit.gpgsign", "false"); + const oldDir = join(scenarioPath, "old"); + if (existsSync(oldDir)) + cpSync(oldDir, cwd, { recursive: true }); + git(cwd, "add", "-A"); + git(cwd, "commit", "-q", "--allow-empty", "-m", "base"); + const baseSha = git(cwd, "rev-parse", "HEAD"); + rmAllExceptGit(cwd); + const newDir = join(scenarioPath, "new"); + if (existsSync(newDir)) + cpSync(newDir, cwd, { recursive: true }); + git(cwd, "add", "-A"); + git(cwd, "commit", "-q", "--allow-empty", "-m", "head"); + const headSha = git(cwd, "rev-parse", "HEAD"); + process.on("exit", () => { + try { + rmSync(cwd, { recursive: true, force: true }); + } + catch { /* ignore */ } + }); + return { cwd, baseSha, headSha }; +} +// ---- Provider ------------------------------------------------------------ +const DEFAULT_META = { + title: "Mock PR", + description: "", + author: "mock-author", + baseBranch: "main", + headBranch: "feature", + baseSha: "", + headSha: "", + url: "mock://scenario", + state: "open", +}; +export class MockVcsProvider { + name = "mock"; + repo; + fixture; + // Side-effect logs for test assertions. + _postedInline = []; + _postedSummaries = []; + _resolvedThreads = []; + _deletedComments = []; + _verdicts = []; + constructor(repo, fixture) { + this.repo = repo; + 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); + } + static async create(config) { + return MockVcsProvider.fromUrl(config.pr.url); + } + static fromUrl(url) { + 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, {}); + } + 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); + } + // ---- VcsProvider -------------------------------------------------------- + async getPRMetadata() { + const fromFixture = this.fixture.metadata ?? {}; + return { + ...DEFAULT_META, + ...fromFixture, + baseSha: this.repo?.baseSha ?? fromFixture.baseSha ?? "", + headSha: this.repo?.headSha ?? fromFixture.headSha ?? "", + }; + } + async getChangedFiles(meta) { + if (!this.repo) + return []; + return gitChangedFiles({ baseSha: meta.baseSha, headSha: meta.headSha, cwd: this.repo.cwd }); + } + async getPotentialRenames(meta) { + if (!this.repo) + return []; + return gitPotentialRenames({ baseSha: meta.baseSha, headSha: meta.headSha, cwd: this.repo.cwd }); + } + 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 }); + } + async getPRComments() { + return (this.fixture.comments ?? []).map((c) => ({ ...c })); + } + async postInlineComment(c) { + this._postedInline.push({ c: { ...c } }); + } + async postSummaryComment(body) { + this._postedSummaries.push({ body }); + } + async approve(summary) { + this._verdicts.push({ verdict: "approve", summary }); + } + async reject(summary) { + this._verdicts.push({ verdict: "reject", summary }); + } + async resolveThread(threadId) { + this._resolvedThreads.push({ threadId }); + } + 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; } + get postedSummaries() { return this._postedSummaries; } + get resolvedThreads() { return this._resolvedThreads; } + get deletedComments() { return this._deletedComments; } + get verdicts() { return this._verdicts; } +} diff --git a/dist/vcs/types.js b/dist/vcs/types.js new file mode 100644 index 0000000..67f0d3d --- /dev/null +++ b/dist/vcs/types.js @@ -0,0 +1,14 @@ +// Dispatch helper: returns which provider's `create()` should handle a URL. +export function detectProvider(url) { + if (/^mock:\/\//.test(url)) + return "mock"; + if (/^https?:\/\/github\.com\/[^/]+\/[^/]+\/pull\/\d+/.test(url)) + return "github"; + if (/^https?:\/\/[^/]+\/[^/]+\/[^/]+\/-\/merge_requests\/\d+/.test(url)) + return "gitlab"; + if (/^https?:\/\/bitbucket\.org\/[^/]+\/[^/]+\/pull-requests\/\d+/.test(url)) + return "bitbucket"; + if (/^https?:\/\/[^/]+\/projects\/[^/]+\/repos\/[^/]+\/pull-requests\/\d+/.test(url)) + return "bitbucket"; + throw new Error(`unsupported PR URL: ${url}`); +} diff --git a/examples/review.yml b/examples/review.yml index 21bbd81..f10b3b9 100644 --- a/examples/review.yml +++ b/examples/review.yml @@ -9,9 +9,12 @@ # identically for npm, yarn, and pnpm: # # "devDependencies": { -# "@reviewer/cli": "github:bilikaz/code-reviewer#v0.1.0" +# "@reviewer/cli": "github:bilikaz/code-reviewer#v0.1.1" # } # +# The package ships prebuilt JavaScript (dist/ is committed), so the +# install does no build step and needs no toolchain beyond your own. +# # - Public reviewer repo -> this just works in CI, no extra auth. # - Private reviewer repo -> the install step needs read access. Add a PAT # with `repo` scope as a secret (e.g. REVIEWER_INSTALL_TOKEN) and diff --git a/package.json b/package.json index 3c7af79..a2f8113 100644 --- a/package.json +++ b/package.json @@ -1,14 +1,15 @@ { "name": "@reviewer/cli", - "version": "0.1.0", + "version": "0.1.1", "description": "LLM-driven PR reviewer. Pluggable VCS destinations. OpenAI-compatible LLM API.", "type": "module", "bin": { - "reviewer": "src/cli.ts" + "reviewer": "dist/cli.js" }, - "files": ["src"], + "files": ["dist"], "scripts": { - "start": "node src/cli.ts", + "start": "node --experimental-transform-types --disable-warning=ExperimentalWarning src/cli.ts", + "build": "tsc -p tsconfig.build.json && node scripts/copy-assets.mjs", "typecheck": "tsc --noEmit", "test:inner": "vitest run", "test": "docker build --build-arg BUILD_TARGET=test -t reviewer-test . && docker run --rm --entrypoint pnpm --env-file tests/.env -v \"$PWD/node_modules:/reviewer/node_modules\" -w /reviewer reviewer-test run test:inner" diff --git a/scripts/copy-assets.mjs b/scripts/copy-assets.mjs new file mode 100644 index 0000000..3cf7e39 --- /dev/null +++ b/scripts/copy-assets.mjs @@ -0,0 +1,27 @@ +// Copy non-TS runtime assets (prompt.md, schema.json) from src/ into dist/, +// preserving directory structure. tsc only emits .js, but the stages load +// these files at runtime via readFileSync(resolve(HERE, ...)), so they must +// sit next to the compiled modules. Pure Node, no deps — runs cross-platform +// during `prepare` on any consumer's machine. + +import { readdir, mkdir, copyFile } from "node:fs/promises"; +import { join, relative, dirname } from "node:path"; + +const SRC = "src"; +const DIST = "dist"; +const ASSET = /\.(md|json)$/; + +async function walk(dir) { + for (const entry of await readdir(dir, { withFileTypes: true })) { + const path = join(dir, entry.name); + if (entry.isDirectory()) { + await walk(path); + } else if (ASSET.test(entry.name)) { + const dest = join(DIST, relative(SRC, path)); + await mkdir(dirname(dest), { recursive: true }); + await copyFile(path, dest); + } + } +} + +await walk(SRC); diff --git a/src/cli.ts b/src/cli.ts index 460c362..cd3158a 100755 --- a/src/cli.ts +++ b/src/cli.ts @@ -1,4 +1,4 @@ -#!/usr/bin/env -S node --experimental-transform-types --disable-warning=ExperimentalWarning +#!/usr/bin/env node // Reviewer CLI. // // Subcommands: diff --git a/tsconfig.build.json b/tsconfig.build.json new file mode 100644 index 0000000..5e220fc --- /dev/null +++ b/tsconfig.build.json @@ -0,0 +1,10 @@ +{ + "extends": "./tsconfig.json", + "compilerOptions": { + "noEmit": false, + "outDir": "dist", + "rootDir": "src" + }, + "include": ["src/**/*.ts"], + "exclude": ["src/**/*.test.ts", "tests/**"] +} From 839c25376c10051c7a31780bc21199a092b8c157 Mon Sep 17 00:00:00 2001 From: bilikas Date: Sun, 7 Jun 2026 16:10:18 +0300 Subject: [PATCH 2/6] pnpm version added --- package.json | 1 + 1 file changed, 1 insertion(+) diff --git a/package.json b/package.json index a2f8113..651b21a 100644 --- a/package.json +++ b/package.json @@ -3,6 +3,7 @@ "version": "0.1.1", "description": "LLM-driven PR reviewer. Pluggable VCS destinations. OpenAI-compatible LLM API.", "type": "module", + "packageManager": "pnpm@10.33.0", "bin": { "reviewer": "dist/cli.js" }, From cc3624c5eadc7be6f702edf7e9e2b5dd10069073 Mon Sep 17 00:00:00 2001 From: bilikas Date: Sun, 7 Jun 2026 16:19:26 +0300 Subject: [PATCH 3/6] prompt fixed --- README.md | 8 ++++---- dist/stages/review/prompt.md | 6 ++++++ src/stages/review/prompt.md | 6 ++++++ 3 files changed, 16 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index f8d5e0d..b69da63 100644 --- a/README.md +++ b/README.md @@ -43,7 +43,7 @@ Add it to the consumer project, pinned to a tag (the git form is the same for ev } ``` -On install the package builds itself (a `prepare` step compiles `src/` to `dist/`), so the consumer ends up with plain JavaScript — Node refuses to run TypeScript from inside `node_modules`, so shipping compiled output is required. 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. +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. Run it locally the same way: @@ -51,7 +51,7 @@ Run it locally the same way: npx reviewer run --pr https://github.com/owner/repo/pull/123 # yarn: yarn reviewer ... | pnpm: pnpm exec reviewer ... ``` -Requires Node ≥24 in the consumer (used to build on install and to run). +Requires Node ≥24 in the consumer (to run the compiled CLI). ## Local usage @@ -98,14 +98,14 @@ All config is env-only and read once at startup. See [.env.example](.env.example ## Development ```bash -pnpm install # also runs `prepare` -> builds dist/ +pnpm install pnpm typecheck pnpm build # tsc -> dist/ (+ copies prompt.md / schema.json assets) pnpm start --pr # run from source (node --experimental-transform-types) pnpm test # builds the test image, mounts node_modules, runs vitest ``` -The package ships compiled JavaScript: `tsconfig.build.json` emits `src/` to `dist/`, and [scripts/copy-assets.mjs](scripts/copy-assets.mjs) copies the runtime `.md`/`.json` assets next to the compiled modules. `bin` and `files` both point at `dist/`; `dist/` is gitignored and regenerated on install. The Docker image is the exception — it runs the CLI from `src/` directly via the entrypoint, so it skips the build (`--ignore-scripts`). +The package ships compiled JavaScript: `tsconfig.build.json` emits `src/` to `dist/`, and [scripts/copy-assets.mjs](scripts/copy-assets.mjs) copies the runtime `.md`/`.json` assets next to the compiled modules. `bin` and `files` both point at `dist/`. **`dist/` is committed** so git-dependency consumers get prebuilt JS with no install-time build; after changing `src/`, run `pnpm build` and commit the result (CI's `dist-fresh` job fails if it's stale). The Docker image is the exception — it runs the CLI from `src/` directly via the entrypoint, so it ignores `dist/` entirely. Tests require `tests/.env` (see [tests/.env.example](tests/.env.example)) — a real LLM endpoint is needed because the E2E suite drives the full pipeline against fixture repos via the `mock://` VCS provider. diff --git a/dist/stages/review/prompt.md b/dist/stages/review/prompt.md index 9972ad2..bcf30b7 100644 --- a/dist/stages/review/prompt.md +++ b/dist/stages/review/prompt.md @@ -31,6 +31,12 @@ Every finding anchors to a snippet you can quote verbatim from ``, ``) - `info` — nit, naming, doc drift, style. Every maintainability concern lands here, even when citing a standard. diff --git a/src/stages/review/prompt.md b/src/stages/review/prompt.md index 9972ad2..bcf30b7 100644 --- a/src/stages/review/prompt.md +++ b/src/stages/review/prompt.md @@ -31,6 +31,12 @@ Every finding anchors to a snippet you can quote verbatim from ``, ``) - `info` — nit, naming, doc drift, style. Every maintainability concern lands here, even when citing a standard. From 3ba0f4c0cd324cea6d603b0d006526862e294574 Mon Sep 17 00:00:00 2001 From: bilikas Date: Sun, 7 Jun 2026 17:58:58 +0300 Subject: [PATCH 4/6] added local runner --- README.md | 19 ++++- dist/cli.js | 9 ++- dist/ctx.js | 24 ++++++ dist/llm/call.js | 55 ++++++++++---- dist/stages/review/index.js | 8 +- dist/stages/review/prompt.md | 4 +- dist/stages/review/schema.json | 33 ++++---- dist/vcs/local.js | 114 ++++++++++++++++++++++++++++ dist/vcs/types.js | 2 + src/cli.ts | 9 ++- src/ctx.ts | 28 +++++++ src/llm/call.ts | 61 +++++++++++---- src/stages/review/index.ts | 8 +- src/stages/review/prompt.md | 4 +- src/stages/review/schema.json | 33 ++++---- src/stages/types.ts | 4 +- src/vcs/local.ts | 135 +++++++++++++++++++++++++++++++++ src/vcs/types.ts | 6 +- tests/e2e/summarize.test.ts | 4 +- 19 files changed, 471 insertions(+), 89 deletions(-) create mode 100644 dist/vcs/local.js create mode 100644 src/vcs/local.ts diff --git a/README.md b/README.md index b69da63..7fc2bb8 100644 --- a/README.md +++ b/README.md @@ -62,6 +62,22 @@ docker run --rm --env-file .env -v "$PWD:/workspace" -w /workspace \ reviewer run --pr https://github.com/owner/repo/pull/123 ``` +### Local branch review (`local://`) + +Review the **current checkout's branch against a base ref** — no PR, no VCS API, no token. Instead of posting, the reviewer **echoes each action to the console** (inline comments with `file:line`, the summary, and a green/red verdict) — like a real provider's posts, just printed. Handy for a fast pre-push pass, especially pointed at a small local model while CI uses a heavier one. + +```bash +docker build -t reviewer . # once (or after changing the reviewer) +docker run --rm -t --env-file .env -v "$PWD:/workspace" -w /workspace \ + reviewer review --pr local://main +``` + +(`-t` gives a TTY so the output is colored; without it, or under `NO_COLOR`, it prints plain.) + +`local://` diffs `...HEAD` (base defaults to `main`). Use `local://` to pick a different base. + +> **Must run inside a container.** Local review gives the LLM the `Bash`/`Read` tools against the mounted checkout; the container is the sandbox. The CLI refuses `local://` unless it detects a container (`/.dockerenv` / `/run/.containerenv`, created by the runtime — not a flag a caller can set), so it can't hand an LLM shell access to a bare host. + ### Commands | Command | What runs | @@ -75,7 +91,8 @@ docker run --rm --env-file .env -v "$PWD:/workspace" -w /workspace \ ### Flags ``` ---pr required (or set PR_URL env) +--pr required (or set PR_URL env). Real PR URL, mock://, + or local:// (review current branch vs base; Docker only) --llm-url overrides LLM_URL --llm-key overrides LLM_KEY --llm-model overrides LLM_MODEL diff --git a/dist/cli.js b/dist/cli.js index affbdaf..95024e3 100644 --- a/dist/cli.js +++ b/dist/cli.js @@ -10,7 +10,12 @@ // unresolved bot threads count as unaddressed) // // Flags: -// --pr required (or set PR_URL env) +// --pr required (or set PR_URL env). A real PR URL (github/ +// gitlab/bitbucket), `mock://` for tests, or +// `local://` to review the current checkout's +// branch against (default main) with no VCS — +// findings print to the console (comments + verdict) +// instead of posting. Must run inside a container. // --llm-url overrides LLM_URL // --llm-key overrides LLM_KEY // --llm-model overrides LLM_MODEL (otherwise auto-detected) @@ -44,7 +49,7 @@ function parseArgs(argv) { return { command, flags }; } function usageAndExit() { - process.stderr.write("usage: reviewer --pr [--llm-url ] [--llm-key ] [--llm-model ]\n"); + process.stderr.write("usage: reviewer --pr [--llm-url ] [--llm-key ] [--llm-model ]\n"); process.exit(2); } function die(msg) { diff --git a/dist/ctx.js b/dist/ctx.js index 33fdb10..187112d 100644 --- a/dist/ctx.js +++ b/dist/ctx.js @@ -8,11 +8,13 @@ // downstream takes values from the typed config object. Env arrives via // Docker's --env-file (see package.json's `test` script); no host-side // .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"; // ---- Defaults ------------------------------------------------------------ @@ -49,6 +51,13 @@ function requireField(value, name, hint) { throw new Error(`missing ${name}. ${hint}`); return value; } +// True when running inside a container. Docker writes `/.dockerenv` at the +// container root; Podman writes `/run/.containerenv`. Both are created by the +// runtime, not the caller — a far higher bar than an env flag (which is why +// `local://` review gates on this, not on a variable anyone can export). +function inContainer() { + return existsSync("/.dockerenv") || existsSync("/run/.containerenv"); +} export function loadConfig(opts) { const e = process.env; if (!opts.prUrl) @@ -88,6 +97,20 @@ export function loadConfig(opts) { break; case "mock": break; + case "local": + // Local review gives the LLM the Bash/Read tools against the real + // checkout. Those tools assume a throwaway sandbox; running on a bare + // host would hand an LLM unsandboxed shell + file access to your + // machine. Require an actual container — detect it rather than trust an + // env flag (which any caller could set on the host). + if (!inContainer()) { + throw new Error("local review must run inside a container (the LLM gets shell + file access, " + + "and the container is the sandbox). Run:\n" + + " docker build -t reviewer .\n" + + " docker run --rm -v \"$PWD:/workspace\" -w /workspace --env-file .env \\\n" + + " reviewer review --pr local://main"); + } + break; } return config; } @@ -98,6 +121,7 @@ function makeVcs(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); } } // ---- buildCtx (thin orchestrator) --------------------------------------- diff --git a/dist/llm/call.js b/dist/llm/call.js index f070e61..ecd21d0 100644 --- a/dist/llm/call.js +++ b/dist/llm/call.js @@ -25,13 +25,19 @@ function extractJson(text) { 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 JSON.parse(c); + return { value: JSON.parse(c), clean: true }; + } + catch (e) { + if (!parseError) + parseError = e.message; } - catch { /* try next */ } } - // Longest-span scan: shrink suffixes until JSON.parse succeeds. + // 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); @@ -50,17 +56,27 @@ function extractJson(text) { } } if (best) - return best.value; - throw new Error("no JSON object or array could be parsed from response"); + 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 response could not be used:", + "Your previous message (directly above) was rejected — it did not produce usable output.", "", - ` Error: ${error}`, + ` Why it failed: ${error}`, "", - "Re-emit your response exactly conforming to the schema you were given.", - "Same content — only fix the structure. Output the JSON only, no prose, no fences.", + fix, + "Output the corrected JSON only — no prose, no markdown fences.", ].join("\n"); } export async function callLLM(args) { @@ -123,15 +139,15 @@ export async function callLLM(args) { lastText = finalText; break; } - let parsed; + let extracted = null; try { - parsed = extractJson(finalText); + extracted = extractJson(finalText); } catch (e) { lastError = `JSON parse: ${e.message}`; } - if (parsed !== undefined) { - if (validate(parsed)) { + if (extracted) { + if (validate(extracted.value)) { log.info("llm.done", { prompt_tokens: totalPrompt, completion_tokens: totalCompletion, @@ -139,14 +155,23 @@ export async function callLLM(args) { tool_calls: toolCalls, }); return { - validated: parsed, + validated: extracted.value, usage: { promptTokens: totalPrompt, completionTokens: totalCompletion, totalTokens: totalAll }, healAttempts, toolCalls, rawText: lastText, }; } - lastError = `schema: ${ajv.errorsText(validate.errors, { separator: "; " })}`; + // 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++; diff --git a/dist/stages/review/index.js b/dist/stages/review/index.js index ed9da76..d5bbaf8 100644 --- a/dist/stages/review/index.js +++ b/dist/stages/review/index.js @@ -84,19 +84,19 @@ async function postFindings(ctx, findings) { // can't be uniquely located we post on line 1 with a banner explaining why. async function postFinding(ctx, log, c) { if (!c.path || !existsSync(c.path)) { - const body = wrapWithBanner(c.anchor.snippet, c.body, "anchor not located"); + 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); return; } - let line = findUniqueMatch(c.path, c.anchor); + let line = findUniqueMatch(c.path, c); let body = c.body; if (line == null) { line = 1; log.warn("anchor.unresolved", { path: c.path, fallback: "line_1" }); - body = wrapWithBanner(c.anchor.snippet, c.body, "snippet not uniquely matched on head"); + body = wrapWithBanner(c.snippet, c.body, "snippet not uniquely matched on head"); } - await ctx.vcs.postInlineComment({ path: c.path, line, body }); + await ctx.vcs.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/review/prompt.md b/dist/stages/review/prompt.md index bcf30b7..22c1f35 100644 --- a/dist/stages/review/prompt.md +++ b/dist/stages/review/prompt.md @@ -27,7 +27,9 @@ You receive (some sections may be absent on a given run): ## Anchoring rule -Every finding anchors to a snippet you can quote verbatim from ``, ``, or a file you Read. The harness — not you — converts the snippet to a line number. Include `context_before` / `context_after` only when the snippet alone is ambiguous (e.g. a repeated `}` or import). +Every finding anchors to a snippet you can quote verbatim from ``, ``, or a file you Read. The harness — not you — converts the snippet to a line number. + +`snippet` MUST be a **single line** — the one most specific line the finding is about — never a multi-line block. A single line is unique enough to locate and trivial to embed as valid JSON; multi-line snippets fail to anchor and are easy to mis-escape. If that one line repeats in the file, add `context_before` / `context_after` (also single lines) to disambiguate. `snippet`, `context_before`, and `context_after` are direct properties of the comment object — do not nest them. Findings about an **absent** thing ("should also validate Y") anchor to the most relevant existing line; explain in `body`. diff --git a/dist/stages/review/schema.json b/dist/stages/review/schema.json index 05e7881..a5fed8b 100644 --- a/dist/stages/review/schema.json +++ b/dist/stages/review/schema.json @@ -10,32 +10,25 @@ "type": "array", "items": { "type": "object", - "required": ["path", "anchor", "body", "severity"], + "required": ["path", "snippet", "body", "severity"], "additionalProperties": false, "properties": { "path": { "type": "string", "description": "Repo-relative path of the file the comment is about." }, - "anchor": { - "type": "object", - "required": ["snippet"], - "additionalProperties": false, - "properties": { - "snippet": { - "type": "string", - "minLength": 1, - "description": "Verbatim line(s) from the file. The harness strips leading whitespace; internal whitespace must match." - }, - "context_before": { - "type": "string", - "description": "Optional. Line(s) immediately before snippet, to disambiguate when snippet repeats." - }, - "context_after": { - "type": "string", - "description": "Optional. Line(s) immediately after snippet; same role as context_before." - } - } + "snippet": { + "type": "string", + "minLength": 1, + "description": "A SINGLE verbatim line from the file — the most specific line the finding is about, never a multi-line block. The harness strips leading whitespace; internal whitespace must match. Escape double quotes as \\\" and there must be no raw newline." + }, + "context_before": { + "type": "string", + "description": "Optional. The single line immediately before snippet, to disambiguate when snippet repeats. Same escaping rules." + }, + "context_after": { + "type": "string", + "description": "Optional. The single line immediately after snippet; same role as context_before." }, "body": { "type": "string", diff --git a/dist/vcs/local.js b/dist/vcs/local.js new file mode 100644 index 0000000..aa40bd9 --- /dev/null +++ b/dist/vcs/local.js @@ -0,0 +1,114 @@ +// Local provider — review the current git checkout's branch against a base +// ref (default `main`) with no VCS API and no network. Selected by a +// `local://` PR URL (e.g. `--pr local://main`). +// +// Changed-files / diffs reuse the exact same `lib/git` helpers the real +// providers use, so there's no new diff logic. There's no remote to post to, +// so the write methods instead echo each action to the console — the same +// 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"; +// 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; +const sgr = (code, s) => (COLOR ? `\x1b[${code}m${s}\x1b[0m` : s); +const bold = (s) => sgr("1", s); +const dim = (s) => sgr("2", s); +const red = (s) => sgr("31", s); +const green = (s) => sgr("32", s); +const yellow = (s) => sgr("33", s); +const cyan = (s) => sgr("36", s); +function out(line) { + process.stdout.write(line + "\n"); +} +// Severity badge, colored: blocker red, warning yellow, info cyan. +function badge(severity) { + switch (severity) { + case "blocker": return red("blocker"); + case "warning": return yellow("warning"); + default: return cyan("info"); + } +} +export class LocalVcsProvider { + meta; + name = "local"; + constructor(meta) { + this.meta = meta; + } + static async create(config) { + const m = config.pr.url.match(/^local:\/\/(.*)$/); + const base = (m?.[1] ?? "").trim() || "main"; + const headSha = (await runGit(["rev-parse", "HEAD"])).trim(); + let baseSha; + try { + baseSha = (await runGit(["rev-parse", base])).trim(); + } + catch { + throw new Error(`local: base ref "${base}" not found in this repo. Use --pr local:// with an existing ref (default: main).`); + } + 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({ + title: title || `${headBranch} vs ${base}`, + description: "", + author: author || "local", + baseBranch: base, + headBranch, + baseSha, + headSha, + url: config.pr.url, + state: "open", + }); + } + 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 []; + } + // No remote to post to — echo each action to the console instead. + async postInlineComment(c) { + out(`\n${badge(c.severity)} ${bold(`${c.path}:${c.line}`)}`); + out(c.body); + } + async postSummaryComment(body) { + out(`\n${cyan("● summary")}`); + out(body); + } + async approve(summary) { + out(`\n${green("✓ approved")}`); + if (summary) + out(summary); + } + async reject(summary) { + out(`\n${red("✗ rejected")}`); + if (summary) + out(summary); + } + async resolveThread(threadId) { + out(dim(`↳ resolved thread ${threadId}`)); + } + 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/types.js b/dist/vcs/types.js index 67f0d3d..eba5e4c 100644 --- a/dist/vcs/types.js +++ b/dist/vcs/types.js @@ -2,6 +2,8 @@ export function detectProvider(url) { if (/^mock:\/\//.test(url)) return "mock"; + if (/^local:\/\//.test(url)) + return "local"; if (/^https?:\/\/github\.com\/[^/]+\/[^/]+\/pull\/\d+/.test(url)) return "github"; if (/^https?:\/\/[^/]+\/[^/]+\/[^/]+\/-\/merge_requests\/\d+/.test(url)) diff --git a/src/cli.ts b/src/cli.ts index cd3158a..da0da26 100755 --- a/src/cli.ts +++ b/src/cli.ts @@ -10,7 +10,12 @@ // unresolved bot threads count as unaddressed) // // Flags: -// --pr required (or set PR_URL env) +// --pr required (or set PR_URL env). A real PR URL (github/ +// gitlab/bitbucket), `mock://` for tests, or +// `local://` to review the current checkout's +// branch against (default main) with no VCS — +// findings print to the console (comments + verdict) +// instead of posting. Must run inside a container. // --llm-url overrides LLM_URL // --llm-key overrides LLM_KEY // --llm-model overrides LLM_MODEL (otherwise auto-detected) @@ -54,7 +59,7 @@ function parseArgs(argv: string[]): ParsedArgs { function usageAndExit(): never { process.stderr.write( - "usage: reviewer --pr [--llm-url ] [--llm-key ] [--llm-model ]\n", + "usage: reviewer --pr [--llm-url ] [--llm-key ] [--llm-model ]\n", ); process.exit(2); } diff --git a/src/ctx.ts b/src/ctx.ts index 0bb41de..2ecac82 100644 --- a/src/ctx.ts +++ b/src/ctx.ts @@ -9,12 +9,15 @@ // Docker's --env-file (see package.json's `test` script); no host-side // .env loading. +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"; @@ -123,6 +126,14 @@ function requireField(value: string, name: string, hint: string): string { return value; } +// True when running inside a container. Docker writes `/.dockerenv` at the +// container root; Podman writes `/run/.containerenv`. Both are created by the +// runtime, not the caller — a far higher bar than an env flag (which is why +// `local://` review gates on this, not on a variable anyone can export). +function inContainer(): boolean { + return existsSync("/.dockerenv") || existsSync("/run/.containerenv"); +} + export function loadConfig(opts: CliOverrides): Config { const e = process.env; if (!opts.prUrl) throw new Error("loadConfig: prUrl required"); @@ -168,6 +179,22 @@ export function loadConfig(opts: CliOverrides): Config { break; case "mock": break; + case "local": + // Local review gives the LLM the Bash/Read tools against the real + // checkout. Those tools assume a throwaway sandbox; running on a bare + // host would hand an LLM unsandboxed shell + file access to your + // machine. Require an actual container — detect it rather than trust an + // env flag (which any caller could set on the host). + if (!inContainer()) { + throw new Error( + "local review must run inside a container (the LLM gets shell + file access, " + + "and the container is the sandbox). Run:\n" + + " docker build -t reviewer .\n" + + " docker run --rm -v \"$PWD:/workspace\" -w /workspace --env-file .env \\\n" + + " reviewer review --pr local://main", + ); + } + break; } return config; @@ -181,6 +208,7 @@ function makeVcs(c: Config): Promise { case "gitlab": return GitLabProvider.create(c); case "bitbucket": return BitbucketProvider.create(c); case "mock": return MockVcsProvider.create(c); + case "local": return LocalVcsProvider.create(c); } } diff --git a/src/llm/call.ts b/src/llm/call.ts index ccb7274..b4fb1c8 100644 --- a/src/llm/call.ts +++ b/src/llm/call.ts @@ -35,14 +35,26 @@ function serializeSections(sections: Section[]): string { const JSON_FENCE_RE = /```(?:json)?\s*\n?([\s\S]*?)```/g; -function extractJson(text: string): unknown { +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 JSON.parse(c); } catch { /* try next */ } + 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. + // 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); @@ -58,18 +70,28 @@ function extractJson(text: string): unknown { i = text.indexOf(opener, i + 1); } } - if (best) return best.value; - throw new Error("no JSON object or array could be parsed from response"); + 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 response could not be used:", + "Your previous message (directly above) was rejected — it did not produce usable output.", "", - ` Error: ${error}`, + ` Why it failed: ${error}`, "", - "Re-emit your response exactly conforming to the schema you were given.", - "Same content — only fix the structure. Output the JSON only, no prose, no fences.", + fix, + "Output the corrected JSON only — no prose, no markdown fences.", ].join("\n"); } @@ -154,14 +176,14 @@ export async function callLLM(args: CallLLMArgs): Promise(args: CallLLMArgs): Promise // 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 { if (!c.path || !existsSync(c.path)) { - const body = wrapWithBanner(c.anchor.snippet, c.body, "anchor not located"); + 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); return; } - let line = findUniqueMatch(c.path, c.anchor); + let line = findUniqueMatch(c.path, c); let body = c.body; if (line == null) { line = 1; log.warn("anchor.unresolved", { path: c.path, fallback: "line_1" }); - body = wrapWithBanner(c.anchor.snippet, c.body, "snippet not uniquely matched on head"); + body = wrapWithBanner(c.snippet, c.body, "snippet not uniquely matched on head"); } - await ctx.vcs.postInlineComment({ path: c.path, line, body }); + await ctx.vcs.postInlineComment({ path: c.path, line, body, severity: c.severity }); } // ---- anchor resolution -------------------------------------------------- diff --git a/src/stages/review/prompt.md b/src/stages/review/prompt.md index bcf30b7..22c1f35 100644 --- a/src/stages/review/prompt.md +++ b/src/stages/review/prompt.md @@ -27,7 +27,9 @@ You receive (some sections may be absent on a given run): ## Anchoring rule -Every finding anchors to a snippet you can quote verbatim from ``, ``, or a file you Read. The harness — not you — converts the snippet to a line number. Include `context_before` / `context_after` only when the snippet alone is ambiguous (e.g. a repeated `}` or import). +Every finding anchors to a snippet you can quote verbatim from ``, ``, or a file you Read. The harness — not you — converts the snippet to a line number. + +`snippet` MUST be a **single line** — the one most specific line the finding is about — never a multi-line block. A single line is unique enough to locate and trivial to embed as valid JSON; multi-line snippets fail to anchor and are easy to mis-escape. If that one line repeats in the file, add `context_before` / `context_after` (also single lines) to disambiguate. `snippet`, `context_before`, and `context_after` are direct properties of the comment object — do not nest them. Findings about an **absent** thing ("should also validate Y") anchor to the most relevant existing line; explain in `body`. diff --git a/src/stages/review/schema.json b/src/stages/review/schema.json index 05e7881..a5fed8b 100644 --- a/src/stages/review/schema.json +++ b/src/stages/review/schema.json @@ -10,32 +10,25 @@ "type": "array", "items": { "type": "object", - "required": ["path", "anchor", "body", "severity"], + "required": ["path", "snippet", "body", "severity"], "additionalProperties": false, "properties": { "path": { "type": "string", "description": "Repo-relative path of the file the comment is about." }, - "anchor": { - "type": "object", - "required": ["snippet"], - "additionalProperties": false, - "properties": { - "snippet": { - "type": "string", - "minLength": 1, - "description": "Verbatim line(s) from the file. The harness strips leading whitespace; internal whitespace must match." - }, - "context_before": { - "type": "string", - "description": "Optional. Line(s) immediately before snippet, to disambiguate when snippet repeats." - }, - "context_after": { - "type": "string", - "description": "Optional. Line(s) immediately after snippet; same role as context_before." - } - } + "snippet": { + "type": "string", + "minLength": 1, + "description": "A SINGLE verbatim line from the file — the most specific line the finding is about, never a multi-line block. The harness strips leading whitespace; internal whitespace must match. Escape double quotes as \\\" and there must be no raw newline." + }, + "context_before": { + "type": "string", + "description": "Optional. The single line immediately before snippet, to disambiguate when snippet repeats. Same escaping rules." + }, + "context_after": { + "type": "string", + "description": "Optional. The single line immediately after snippet; same role as context_before." }, "body": { "type": "string", diff --git a/src/stages/types.ts b/src/stages/types.ts index 7af2497..3c36b6b 100644 --- a/src/stages/types.ts +++ b/src/stages/types.ts @@ -73,7 +73,9 @@ export interface StageContext { export interface ReviewFinding { path: string; - anchor: { snippet: string; context_before?: string; context_after?: string }; + snippet: string; + context_before?: string; + context_after?: string; body: string; severity: "info" | "warning" | "blocker"; } diff --git a/src/vcs/local.ts b/src/vcs/local.ts new file mode 100644 index 0000000..facd716 --- /dev/null +++ b/src/vcs/local.ts @@ -0,0 +1,135 @@ +// Local provider — review the current git checkout's branch against a base +// ref (default `main`) with no VCS API and no network. Selected by a +// `local://` PR URL (e.g. `--pr local://main`). +// +// Changed-files / diffs reuse the exact same `lib/git` helpers the real +// providers use, so there's no new diff logic. There's no remote to post to, +// so the write methods instead echo each action to the console — the same +// 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 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"; + +// 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; +const sgr = (code: string, s: string): string => (COLOR ? `\x1b[${code}m${s}\x1b[0m` : s); +const bold = (s: string): string => sgr("1", s); +const dim = (s: string): string => sgr("2", s); +const red = (s: string): string => sgr("31", s); +const green = (s: string): string => sgr("32", s); +const yellow = (s: string): string => sgr("33", s); +const cyan = (s: string): string => sgr("36", s); + +function out(line: string): void { + process.stdout.write(line + "\n"); +} + +// Severity badge, colored: blocker red, warning yellow, info cyan. +function badge(severity?: "info" | "warning" | "blocker"): string { + switch (severity) { + case "blocker": return red("blocker"); + case "warning": return yellow("warning"); + default: return cyan("info"); + } +} + +export class LocalVcsProvider implements VcsProvider { + readonly name = "local" as const; + + private constructor(private readonly meta: PRMetadata) {} + + static async create(config: Config): Promise { + const m = config.pr.url.match(/^local:\/\/(.*)$/); + const base = (m?.[1] ?? "").trim() || "main"; + + const headSha = (await runGit(["rev-parse", "HEAD"])).trim(); + let baseSha: string; + try { + baseSha = (await runGit(["rev-parse", base])).trim(); + } catch { + throw new Error( + `local: base ref "${base}" not found in this repo. Use --pr local:// with an existing ref (default: main).`, + ); + } + 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({ + title: title || `${headBranch} vs ${base}`, + description: "", + author: author || "local", + baseBranch: base, + headBranch, + baseSha, + headSha, + url: config.pr.url, + state: "open", + }); + } + + async getPRMetadata(): Promise { + 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 []; + } + + // No remote to post to — echo each action to the console instead. + async postInlineComment(c: InlinePost): Promise { + out(`\n${badge(c.severity)} ${bold(`${c.path}:${c.line}`)}`); + out(c.body); + } + async postSummaryComment(body: string): Promise { + out(`\n${cyan("● summary")}`); + out(body); + } + async approve(summary?: string): Promise { + out(`\n${green("✓ approved")}`); + if (summary) out(summary); + } + async reject(summary?: string): Promise { + out(`\n${red("✗ rejected")}`); + if (summary) out(summary); + } + async resolveThread(threadId: string): Promise { + out(dim(`↳ resolved thread ${threadId}`)); + } + 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/types.ts b/src/vcs/types.ts index 51744c9..c811d51 100644 --- a/src/vcs/types.ts +++ b/src/vcs/types.ts @@ -1,4 +1,4 @@ -export type ProviderKind = "github" | "gitlab" | "bitbucket" | "mock"; +export type ProviderKind = "github" | "gitlab" | "bitbucket" | "mock" | "local"; export interface PRMetadata { title: string; @@ -31,6 +31,9 @@ export interface InlinePost { path: string; line: number; body: string; + // Optional context. Real providers ignore it (severity isn't part of a VCS + // comment); the local provider uses it to color console output. + severity?: "info" | "warning" | "blocker"; } export interface ChangedFile { @@ -92,6 +95,7 @@ export interface VcsProvider { // Dispatch helper: returns which provider's `create()` should handle a URL. export function detectProvider(url: string): ProviderKind { if (/^mock:\/\//.test(url)) return "mock"; + if (/^local:\/\//.test(url)) return "local"; if (/^https?:\/\/github\.com\/[^/]+\/[^/]+\/pull\/\d+/.test(url)) return "github"; if (/^https?:\/\/[^/]+\/[^/]+\/[^/]+\/-\/merge_requests\/\d+/.test(url)) return "gitlab"; if (/^https?:\/\/bitbucket\.org\/[^/]+\/[^/]+\/pull-requests\/\d+/.test(url)) return "bitbucket"; diff --git a/tests/e2e/summarize.test.ts b/tests/e2e/summarize.test.ts index 454542c..1a5240e 100644 --- a/tests/e2e/summarize.test.ts +++ b/tests/e2e/summarize.test.ts @@ -78,7 +78,7 @@ describe("summarize", () => { const state = baseState({ findings: [{ path: "src/auth.ts", - anchor: { snippet: "const KEY = 'sk-abc'" }, + snippet: "const KEY = 'sk-abc'", body: "Hardcoded credential.", severity: "blocker", }], @@ -139,7 +139,7 @@ describe("summarize", () => { const state = baseState({ findings: [{ path: "src/x.ts", - anchor: { snippet: "x" }, + snippet: "x", body: longBody, severity: "info", }], From 02fc1aacaeab365d57a74ae64b718014cf045771 Mon Sep 17 00:00:00 2001 From: bilikas Date: Sun, 7 Jun 2026 18:11:56 +0300 Subject: [PATCH 5/6] actions bumped to v6 --- .github/workflows/ci.yml | 6 +++--- .github/workflows/review.yml | 2 +- examples/review.yml | 6 +++--- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 444fad6..14b5e65 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -13,11 +13,11 @@ jobs: dist-fresh: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@v6 - - uses: pnpm/action-setup@v4 + - uses: pnpm/action-setup@v6 - - uses: actions/setup-node@v4 + - uses: actions/setup-node@v6 with: node-version: 24 cache: pnpm diff --git a/.github/workflows/review.yml b/.github/workflows/review.yml index 1bc53b7..93dc990 100644 --- a/.github/workflows/review.yml +++ b/.github/workflows/review.yml @@ -14,7 +14,7 @@ jobs: review: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@v6 with: # Full history so `git diff base...head` and rename detection work. fetch-depth: 0 diff --git a/examples/review.yml b/examples/review.yml index f10b3b9..cc10314 100644 --- a/examples/review.yml +++ b/examples/review.yml @@ -45,15 +45,15 @@ jobs: review: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@v6 with: # Full history so `git diff base...head` and rename detection work. fetch-depth: 0 # pnpm only: install pnpm before setup-node so its cache can resolve. - # - uses: pnpm/action-setup@v4 + # - uses: pnpm/action-setup@v6 - - uses: actions/setup-node@v4 + - uses: actions/setup-node@v6 with: node-version: 24 # reviewer needs Node >=24 (TS transforms at runtime) cache: npm # yarn: cache: yarn | pnpm: cache: pnpm From ef52e70660d5fa7e9722585f9d31088d451adce3 Mon Sep 17 00:00:00 2001 From: bilikas Date: Sun, 7 Jun 2026 18:27:39 +0300 Subject: [PATCH 6/6] pat toke added --- .github/workflows/review.yml | 8 +++++++- README.md | 3 ++- examples/review.yml | 7 ++++++- 3 files changed, 15 insertions(+), 3 deletions(-) diff --git a/.github/workflows/review.yml b/.github/workflows/review.yml index 93dc990..33beec1 100644 --- a/.github/workflows/review.yml +++ b/.github/workflows/review.yml @@ -24,7 +24,13 @@ jobs: - name: Run reviewer on this PR env: - GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + # Default GITHUB_TOKEN can POST comments but, as a GitHub App + # (integration) token, cannot `resolveReviewThread` — so reconcile + # can't clean up addressed bot threads ("Resource not accessible by + # integration"). A user PAT can. Set REVIEWER_GH_TOKEN (fine-grained: + # Pull requests = read+write) to enable thread resolution; falls back + # to the default token (post-only) when unset. + GITHUB_TOKEN: ${{ secrets.REVIEWER_GH_TOKEN || secrets.GITHUB_TOKEN }} LLM_URL: ${{ secrets.LLM_URL }} LLM_KEY: ${{ secrets.LLM_KEY }} LLM_MODEL: ${{ vars.LLM_MODEL }} diff --git a/README.md b/README.md index 7fc2bb8..70cf697 100644 --- a/README.md +++ b/README.md @@ -26,8 +26,9 @@ Copy [.github/workflows/review.yml](.github/workflows/review.yml) into your repo | Secret | `LLM_URL` | `https:///v1` | | Secret | `LLM_KEY` | `sk-…` | | Variable | `LLM_MODEL` | `deepseek-ai/DeepSeek-V4-Flash` (optional — auto-detected if unset) | +| Secret | `REVIEWER_GH_TOKEN` | optional PAT — see below | -`GITHUB_TOKEN` is injected automatically. +`GITHUB_TOKEN` is injected automatically and is enough to **post** comments. It can't **resolve** review threads, though: the default token is a GitHub App (integration) token, and `resolveReviewThread` isn't available to integrations ("Resource not accessible by integration"). So on follow-up pushes, reconcile decides a thread is addressed but can't clean it up. To enable thread resolution, add a user **PAT** as `REVIEWER_GH_TOKEN` (fine-grained: *Pull requests → read & write*, or classic `repo`); the workflow uses it when present and falls back to `GITHUB_TOKEN` otherwise. The workflow builds the reviewer image from source on each run and triggers on every PR open / synchronize / reopen. (A pre-built image on GHCR is planned — until then, consumers should vendor the Dockerfile or build from a tagged release.) diff --git a/examples/review.yml b/examples/review.yml index cc10314..9117a46 100644 --- a/examples/review.yml +++ b/examples/review.yml @@ -65,7 +65,12 @@ jobs: - name: Review this PR env: - GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + # The default GITHUB_TOKEN can post comments but, being a GitHub App + # (integration) token, can't resolve review threads — so the reviewer + # can't clean up addressed bot threads on follow-up pushes. To enable + # that, set a user PAT secret REVIEWER_GH_TOKEN (fine-grained: + # Pull requests = read+write). Falls back to the default token when unset. + GITHUB_TOKEN: ${{ secrets.REVIEWER_GH_TOKEN || secrets.GITHUB_TOKEN }} LLM_URL: ${{ secrets.LLM_URL }} LLM_KEY: ${{ secrets.LLM_KEY }} LLM_MODEL: ${{ vars.LLM_MODEL }}