diff --git a/review-enrichment/package.json b/review-enrichment/package.json index 250c6c6505..b0468948da 100644 --- a/review-enrichment/package.json +++ b/review-enrichment/package.json @@ -11,7 +11,7 @@ "build": "tsc -p tsconfig.json", "start": "node dist/server.js", "dev": "node --experimental-strip-types --watch src/server.ts", - "test": "node --test --experimental-strip-types" + "test": "npm run build && node --test --experimental-strip-types \"test/**/*.test.ts\"" }, "dependencies": { "@hono/node-server": "^1.13.7", diff --git a/review-enrichment/src/analyzers/dependency-scan.ts b/review-enrichment/src/analyzers/dependency-scan.ts new file mode 100644 index 0000000000..3f31207919 --- /dev/null +++ b/review-enrichment/src/analyzers/dependency-scan.ts @@ -0,0 +1,175 @@ +// Dependency-diff + OSV.dev CVE analyzer (#1474). Parses the changed manifests in the PR diff for added/upgraded +// dependencies, then queries OSV.dev (free, no key) for known vulnerabilities in the NEW versions. This is the +// heavy/external work the no-checkout `claude --print` reviewer cannot do (Bash/WebFetch disallowed, no CVE DB). +import type { EnrichRequest, DependencyFinding, Cve } from "../types.js"; + +interface DepChange { + ecosystem: string; + package: string; + from: string | null; + to: string; +} + +// Per-manifest line parsers. Each returns [name, version] for a `+`/`-` diff line, or null. Heuristic (line-based, +// not a full manifest parse) — good enough to flag the deps a PR adds/bumps without resolving the whole tree. +const NPM_RE = /^"([^"]+)"\s*:\s*"([\^~>=<\s]*[0-9][^"]*)"/; +const PYPI_RE = /^([A-Za-z0-9._-]+)\s*==\s*([0-9][^\s;]*)/; +const GO_RE = /^([a-z0-9.\/-]+)\s+v([0-9][^\s]*)/; + +function parseLine( + manifest: string, + body: string, +): { name: string; version: string } | null { + if (manifest === "package.json") { + const m = NPM_RE.exec(body); + if (m) + return { name: m[1]!, version: m[2]!.replace(/^[\^~>=<\s]+/, "").trim() }; + } else if (manifest === "requirements.txt") { + const m = PYPI_RE.exec(body); + if (m) return { name: m[1]!, version: m[2]! }; + } else if (manifest === "go.mod") { + const m = GO_RE.exec(body.replace(/^require\s+/, "").trim()); + if (m) return { name: m[1]!, version: m[2]! }; + } + return null; +} + +const ECOSYSTEM: Record = { + "package.json": "npm", + "requirements.txt": "PyPI", + "go.mod": "Go", +}; + +/** Extract added/changed (not removed) dependency versions from the changed manifests in the diff. Pure. */ +export function extractDependencyChanges( + files: NonNullable, +): DepChange[] { + const byKey = new Map< + string, + { ecosystem: string; package: string; added?: string; removed?: string } + >(); + for (const file of files) { + const manifest = file.path.split("/").pop() ?? file.path; + const ecosystem = ECOSYSTEM[manifest]; + if (!ecosystem || !file.patch) continue; + for (const line of file.patch.split("\n")) { + const sign = line[0]; + if ( + (sign !== "+" && sign !== "-") || + line.startsWith("+++") || + line.startsWith("---") + ) + continue; + const parsed = parseLine(manifest, line.slice(1).trim()); + if (!parsed) continue; + const key = ecosystem + "::" + parsed.name; + const entry = byKey.get(key) ?? { ecosystem, package: parsed.name }; + if (sign === "+") entry.added = parsed.version; + else entry.removed = parsed.version; + byKey.set(key, entry); + } + } + const changes: DepChange[] = []; + for (const entry of byKey.values()) { + // Only scan a version that's present after the change, and only when it actually changed. + if (!entry.added || entry.added === entry.removed) continue; + changes.push({ + ecosystem: entry.ecosystem, + package: entry.package, + from: entry.removed ?? null, + to: entry.added, + }); + } + return changes; +} + +interface OsvVuln { + id: string; + summary?: string; + details?: string; + severity?: Array<{ type: string; score: string }>; + database_specific?: { severity?: string }; + affected?: Array<{ ranges?: Array<{ events?: Array<{ fixed?: string }> }> }>; +} + +function severityOf(vuln: OsvVuln): Cve["severity"] { + const label = vuln.database_specific?.severity?.toLowerCase(); + if ( + label === "critical" || + label === "high" || + label === "medium" || + label === "low" + ) + return label; + const score = Number( + vuln.severity?.find((s) => s.type?.startsWith("CVSS"))?.score, + ); + if (!Number.isFinite(score)) return "unknown"; + return score >= 9 + ? "critical" + : score >= 7 + ? "high" + : score >= 4 + ? "medium" + : "low"; +} + +function fixedOf(vuln: OsvVuln): string | null { + for (const affected of vuln.affected ?? []) { + for (const range of affected.ranges ?? []) { + for (const event of range.events ?? []) { + if (event.fixed) return event.fixed; + } + } + } + return null; +} + +/** Query OSV.dev for vulnerabilities affecting a specific package version. Best-effort: returns [] on any error. */ +export async function queryOsv( + ecosystem: string, + name: string, + version: string, + fetchImpl: typeof fetch = fetch, +): Promise { + const response = await fetchImpl("https://api.osv.dev/v1/query", { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ package: { name, ecosystem }, version }), + }); + if (!response.ok) return []; + const data = (await response.json()) as { vulns?: OsvVuln[] }; + return (data.vulns ?? []).map((vuln) => ({ + id: vuln.id, + severity: severityOf(vuln), + summary: (vuln.summary ?? vuln.details ?? "") + .replace(/\s+/g, " ") + .slice(0, 180), + fixedIn: fixedOf(vuln), + })); +} + +/** Analyzer entrypoint: changed deps → OSV → only the deps that carry vulnerabilities. */ +export async function scanDependencies( + req: EnrichRequest, + fetchImpl: typeof fetch = fetch, +): Promise { + const changes = extractDependencyChanges(req.files ?? []); + const findings: DependencyFinding[] = []; + for (const change of changes) { + const cves = await queryOsv( + change.ecosystem, + change.package, + change.to, + fetchImpl, + ); + if (cves.length) { + findings.push({ + ...change, + direction: change.from ? "change" : "add", + cves, + }); + } + } + return findings; +} diff --git a/review-enrichment/src/brief.ts b/review-enrichment/src/brief.ts new file mode 100644 index 0000000000..e4ceb6657a --- /dev/null +++ b/review-enrichment/src/brief.ts @@ -0,0 +1,80 @@ +// Orchestrator: fan out the enabled analyzers under a time budget, assemble the ReviewBrief, render the prompt +// block. Each analyzer is independent + best-effort — one that throws/times out marks the brief `partial` and the +// others still contribute, so the engine always gets a usable (possibly empty) brief and never blocks on us. +import type { + EnrichRequest, + ReviewBrief, + BriefFindings, + AnalyzerStatus, +} from "./types.js"; +import { scanDependencies } from "./analyzers/dependency-scan.js"; +import { renderBrief } from "./render.js"; + +type AnalyzerFn = (req: EnrichRequest) => Promise; + +// The analyzer registry. More land behind this same shape: license (#1475), secret (#1476), static (#1477), history (#1478). +const ANALYZERS: Record = { + dependency: (req) => scanDependencies(req), +}; + +function withTimeout(promise: Promise, ms: number): Promise { + return new Promise((resolve, reject) => { + const timer = setTimeout(() => reject(new Error("analyzer_timeout")), ms); + promise.then( + (value) => { + clearTimeout(timer); + resolve(value); + }, + (error) => { + clearTimeout(timer); + reject(error); + }, + ); + }); +} + +export async function buildBrief(req: EnrichRequest): Promise { + const start = Date.now(); + const all = Object.keys(ANALYZERS) as Array; + const requested = req.analyzers?.length + ? all.filter((name) => req.analyzers!.includes(name)) + : all; + const budgetMs = req.budget?.timeoutMs ?? 8000; + + const findings: BriefFindings = {}; + const analyzerStatus: Record = {}; + let partial = false; + + await Promise.all( + requested.map(async (name) => { + try { + const result = await withTimeout(ANALYZERS[name](req), budgetMs); + findings[name] = result as never; + analyzerStatus[name] = "ok"; + } catch { + analyzerStatus[name] = "degraded"; + partial = true; + } + }), + ); + for (const name of all) + if (!requested.includes(name)) analyzerStatus[name] = "skipped"; + + const { promptSection, systemSuffix } = renderBrief( + findings, + req.budget?.maxBriefChars ?? 6000, + ); + return { + schemaVersion: 1, + repoFullName: req.repoFullName, + prNumber: req.prNumber, + headSha: req.headSha ?? null, + generatedAtIso: new Date().toISOString(), + elapsedMs: Date.now() - start, + partial, + analyzerStatus, + findings, + promptSection, + systemSuffix, + }; +} diff --git a/review-enrichment/src/render.ts b/review-enrichment/src/render.ts new file mode 100644 index 0000000000..9ac2939649 --- /dev/null +++ b/review-enrichment/src/render.ts @@ -0,0 +1,48 @@ +// Render structured findings into the public-safe prompt block the engine splices into the review. Kept separate +// so each analyzer's rendering is one function and the brief stays deterministic + cap-bounded. +import type { BriefFindings } from "./types.js"; + +const SEVERITY_RANK: Record = { + critical: 0, + high: 1, + medium: 2, + low: 3, + unknown: 4, +}; + +/** Build the `promptSection` (verbatim splice) + a one-line `systemSuffix` from the findings. Empty when nothing found. */ +export function renderBrief( + findings: BriefFindings, + maxChars = 6000, +): { promptSection: string; systemSuffix: string } { + const lines: string[] = []; + + const deps = findings.dependency ?? []; + if (deps.length) { + lines.push("### Dependency vulnerabilities (OSV.dev)"); + const flat = deps + .flatMap((dep) => dep.cves.map((cve) => ({ dep, cve }))) + .sort( + (a, b) => + (SEVERITY_RANK[a.cve.severity] ?? 4) - + (SEVERITY_RANK[b.cve.severity] ?? 4), + ); + for (const { dep, cve } of flat) { + const fix = cve.fixedIn ? ` — fixed in ${cve.fixedIn}` : ""; + lines.push( + `- \`${dep.package}@${dep.to}\` (${dep.ecosystem}): **${cve.severity}** ${cve.id} — ${cve.summary}${fix}`, + ); + } + } + + if (!lines.length) return { promptSection: "", systemSuffix: "" }; + + const header = + "## EXTERNAL REVIEW BRIEF (heavy/external analysis the in-prompt reviewer cannot run)"; + let body = `${header}\n${lines.join("\n")}\n`; + if (body.length > maxChars) + body = body.slice(0, maxChars) + "\n…(brief truncated)\n"; + const systemSuffix = + "When the EXTERNAL REVIEW BRIEF lists a CVE for a package+version, treat it as verified ground truth — do not re-derive it."; + return { promptSection: body, systemSuffix }; +} diff --git a/review-enrichment/src/server.ts b/review-enrichment/src/server.ts index d77d07b9f0..ea41382444 100644 --- a/review-enrichment/src/server.ts +++ b/review-enrichment/src/server.ts @@ -1,54 +1,17 @@ -// Gittensory review-enrichment service (REES) — #1473 scaffold. +// Gittensory review-enrichment service (REES). // // Given a PR (repo, number, headSha, diff, files, short-lived token), this service runs the heavy/external/ // historical analysis the no-checkout `claude --print` reviewer is blind to, and returns a pre-rendered, // public-safe "review brief" the engine splices into the prompt next to grounding + RAG. The engine treats any // timeout/error as "no brief" and proceeds — so this service is strictly additive and fully fail-safe. // -// THIS scaffold ships the contract + transport only: /health, /ready, and an authenticated /v1/enrich that -// returns an empty (non-partial) brief. The analyzers — dependency/CVE (#1474), license (#1475), secret (#1476), -// static+complexity (#1477), history (#1478) — land behind this stable contract, each filling one `findings` key. +// Transport + contract here; the analysis lives in brief.ts (orchestrator) + analyzers/* — dependency/CVE (#1474), +// then license (#1475), secret (#1476), static+complexity (#1477), history (#1478), each filling one findings key. import { serve } from "@hono/node-server"; import { Hono } from "hono"; import { verifyBearer } from "./auth.js"; - -/** Engine → service request. The engine already has the diff + files, so the service needs NO repo checkout. */ -export interface EnrichRequest { - repoFullName: string; - prNumber: number; - headSha?: string; - baseSha?: string; - title?: string; - body?: string; - author?: string; - files?: Array<{ - path: string; - status?: string; - patch?: string; - additions?: number; - deletions?: number; - }>; - diff?: string; - /** Short-lived broker token for OSV/license/history fetches. Never logged. */ - githubToken?: string; - budget?: { timeoutMs?: number; maxBriefChars?: number }; - analyzers?: string[]; -} - -/** Service → engine response. `promptSection` is spliced verbatim; `findings` is the structured backing data. */ -export interface ReviewBrief { - schemaVersion: 1; - repoFullName: string; - prNumber: number; - headSha: string | null; - generatedAtIso: string; - elapsedMs: number; - partial: boolean; - analyzerStatus: Record; - findings: Record; - promptSection: string; - systemSuffix: string; -} +import type { EnrichRequest } from "./types.js"; +import { buildBrief } from "./brief.js"; const app = new Hono(); @@ -58,7 +21,6 @@ app.get("/health", (c) => app.get("/ready", (c) => c.json({ ready: true })); app.post("/v1/enrich", async (c) => { - const start = Date.now(); const secret = process.env.REES_SHARED_SECRET; // No secret configured ⇒ the service is not ready to authenticate anything; fail closed. if (!secret) return c.json({ error: "service_not_configured" }, 503); @@ -76,22 +38,7 @@ app.post("/v1/enrich", async (c) => { return c.json({ error: "bad_request" }, 400); } - // Scaffold: no analyzers wired yet (#1474-#1478). Return an empty, non-partial brief so the engine seam - // (#1472) can integrate and smoke-test end-to-end. As analyzers land they populate `findings`/`analyzerStatus` - // and render into `promptSection`. - const brief: ReviewBrief = { - schemaVersion: 1, - repoFullName: payload.repoFullName, - prNumber: payload.prNumber, - headSha: payload.headSha ?? null, - generatedAtIso: new Date().toISOString(), - elapsedMs: Date.now() - start, - partial: false, - analyzerStatus: {}, - findings: {}, - promptSection: "", - systemSuffix: "", - }; + const brief = await buildBrief(payload); return c.json(brief); }); diff --git a/review-enrichment/src/types.ts b/review-enrichment/src/types.ts new file mode 100644 index 0000000000..1a0e7d35ea --- /dev/null +++ b/review-enrichment/src/types.ts @@ -0,0 +1,65 @@ +// Shared contract types for the review-enrichment service (REES). Kept separate from server.ts so analyzers and +// the orchestrator can import them without a circular dependency through the HTTP layer. + +/** Engine → service request. The engine already has the diff + files, so the service needs NO repo checkout. */ +export interface EnrichRequest { + repoFullName: string; + prNumber: number; + headSha?: string; + baseSha?: string; + title?: string; + body?: string; + author?: string; + files?: Array<{ + path: string; + status?: string; + patch?: string; + additions?: number; + deletions?: number; + }>; + diff?: string; + /** Short-lived broker token for OSV/license/history fetches. Never logged. */ + githubToken?: string; + budget?: { timeoutMs?: number; maxBriefChars?: number }; + analyzers?: string[]; +} + +/** A known vulnerability for a dependency version, sourced from OSV.dev. */ +export interface Cve { + id: string; + severity: "critical" | "high" | "medium" | "low" | "unknown"; + summary: string; + fixedIn: string | null; +} + +/** One added/changed dependency that carries at least one known vulnerability. */ +export interface DependencyFinding { + ecosystem: string; + package: string; + from: string | null; + to: string; + direction: "add" | "change"; + cves: Cve[]; +} + +/** Structured analyzer output. Each analyzer fills its own key; more land as analyzers ship (#1475–#1478). */ +export interface BriefFindings { + dependency?: DependencyFinding[]; +} + +export type AnalyzerStatus = "ok" | "degraded" | "skipped"; + +/** Service → engine response. `promptSection` is spliced verbatim; `findings` is the structured backing data. */ +export interface ReviewBrief { + schemaVersion: 1; + repoFullName: string; + prNumber: number; + headSha: string | null; + generatedAtIso: string; + elapsedMs: number; + partial: boolean; + analyzerStatus: Record; + findings: BriefFindings; + promptSection: string; + systemSuffix: string; +} diff --git a/review-enrichment/test/enrichment.test.ts b/review-enrichment/test/enrichment.test.ts new file mode 100644 index 0000000000..a47482228a --- /dev/null +++ b/review-enrichment/test/enrichment.test.ts @@ -0,0 +1,172 @@ +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { + extractDependencyChanges, + queryOsv, + scanDependencies, +} from "../dist/analyzers/dependency-scan.js"; +import { renderBrief } from "../dist/render.js"; +import { buildBrief } from "../dist/brief.js"; + +const okFetch = (vulns) => async () => ({ + ok: true, + json: async () => ({ vulns }), +}); + +test("extractDependencyChanges: npm change vs add, ignores removed + non-version lines", () => { + const changes = extractDependencyChanges([ + { + path: "package.json", + patch: [ + '- "lodash": "^4.17.20",', + '+ "lodash": "^4.17.21",', + '+ "left-pad": "1.0.0",', + '- "gone": "1.0.0",', + '+ "name": "my-app",', + ].join("\n"), + }, + ]); + const byPkg = Object.fromEntries(changes.map((c) => [c.package, c])); + assert.equal(byPkg.lodash.to, "4.17.21"); + assert.equal(byPkg.lodash.from, "4.17.20"); + assert.equal(byPkg["left-pad"].to, "1.0.0"); + assert.equal(byPkg["left-pad"].from, null); + assert.equal(byPkg.gone, undefined); // removed-only → not scanned + assert.equal(byPkg.name, undefined); // not a version string +}); + +test("extractDependencyChanges: PyPI + Go ecosystems", () => { + const changes = extractDependencyChanges([ + { path: "requirements.txt", patch: "+requests==2.31.0\n-requests==2.30.0" }, + { path: "go.mod", patch: "+\texample.com/foo v1.2.3" }, + ]); + const eco = Object.fromEntries(changes.map((c) => [c.ecosystem, c])); + assert.equal(eco.PyPI.to, "2.31.0"); + assert.equal(eco.Go.package, "example.com/foo"); + assert.equal(eco.Go.to, "1.2.3"); +}); + +test("queryOsv: maps vulns; severity from database_specific; fixedIn from affected; [] on non-ok", async () => { + const cves = await queryOsv( + "npm", + "lodash", + "4.17.20", + okFetch([ + { + id: "GHSA-x", + summary: "Prototype pollution", + database_specific: { severity: "HIGH" }, + affected: [ + { ranges: [{ events: [{ introduced: "0" }, { fixed: "4.17.21" }] }] }, + ], + }, + ]), + ); + assert.equal(cves.length, 1); + assert.equal(cves[0].severity, "high"); + assert.equal(cves[0].fixedIn, "4.17.21"); + const none = await queryOsv("npm", "x", "1", async () => ({ + ok: false, + json: async () => ({}), + })); + assert.deepEqual(none, []); +}); + +test("queryOsv: CVSS numeric score bucketed when no database_specific", async () => { + const cves = await queryOsv( + "npm", + "x", + "1", + okFetch([{ id: "Y", severity: [{ type: "CVSS_V3", score: "9.8" }] }]), + ); + assert.equal(cves[0].severity, "critical"); +}); + +test("scanDependencies: only deps with vulns are returned", async () => { + const findings = await scanDependencies( + { + repoFullName: "o/r", + prNumber: 1, + files: [{ path: "package.json", patch: '+ "lodash": "4.17.20",' }], + }, + okFetch([{ id: "GHSA-x", database_specific: { severity: "CRITICAL" } }]), + ); + assert.equal(findings.length, 1); + assert.equal(findings[0].direction, "add"); + assert.equal(findings[0].cves[0].severity, "critical"); +}); + +test("renderBrief: sorts by severity, empty when no findings", () => { + const empty = renderBrief({}); + assert.equal(empty.promptSection, ""); + const rendered = renderBrief({ + dependency: [ + { + ecosystem: "npm", + package: "a", + from: null, + to: "1", + direction: "add", + cves: [{ id: "LOW-1", severity: "low", summary: "x", fixedIn: null }], + }, + { + ecosystem: "npm", + package: "b", + from: null, + to: "2", + direction: "add", + cves: [ + { id: "CRIT-1", severity: "critical", summary: "y", fixedIn: "3" }, + ], + }, + ], + }); + assert.match(rendered.promptSection, /EXTERNAL REVIEW BRIEF/); + assert.ok( + rendered.promptSection.indexOf("CRIT-1") < + rendered.promptSection.indexOf("LOW-1"), + "critical before low", + ); + assert.match(rendered.systemSuffix, /verified ground truth/); +}); + +test("buildBrief: runs dependency analyzer, marks others skipped, partial=false on success", async () => { + const realFetch = globalThis.fetch; + globalThis.fetch = okFetch([ + { id: "GHSA-z", database_specific: { severity: "HIGH" } }, + ]); + try { + const brief = await buildBrief({ + repoFullName: "o/r", + prNumber: 7, + headSha: "abc", + files: [{ path: "package.json", patch: '+ "lodash": "4.17.20",' }], + }); + assert.equal(brief.schemaVersion, 1); + assert.equal(brief.partial, false); + assert.equal(brief.analyzerStatus.dependency, "ok"); + assert.equal(brief.findings.dependency.length, 1); + assert.match(brief.promptSection, /GHSA-z/); + } finally { + globalThis.fetch = realFetch; + } +}); + +test("buildBrief: analyzer throw → degraded + partial, still returns a brief", async () => { + const realFetch = globalThis.fetch; + globalThis.fetch = async () => { + throw new Error("network down"); + }; + try { + const brief = await buildBrief({ + repoFullName: "o/r", + prNumber: 8, + files: [{ path: "package.json", patch: '+ "lodash": "4.17.20",' }], + }); + assert.equal(brief.partial, true); + assert.equal(brief.analyzerStatus.dependency, "degraded"); + assert.equal(brief.promptSection, ""); + } finally { + globalThis.fetch = realFetch; + } +});