diff --git a/.gitignore b/.gitignore index 2f347c7..a4c6665 100644 --- a/.gitignore +++ b/.gitignore @@ -1,5 +1,9 @@ node_modules/ dist/ +# Fixture decoys carry real ignored-directory names so the related-files +# scan's pruning is tested against the genuine article — re-include them +!fixtures/workspace/node_modules/ +!fixtures/workspace/dist/ .env *.log .DS_Store diff --git a/AGENTS.md b/AGENTS.md index a53cb96..1240026 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -19,10 +19,10 @@ src/ main.ts # entrypoint — collects/validates inputs; pipeline + outputs land next PR config.ts # action inputs → validated ActionConfig logger.ts # structured JSON logger — levels, child contexts, lazy props - github/ # GitHub I/O: event payload → PrContext (octokit wrappers planned — next PR) - openrouter/ # (planned — next PR) OpenRouter I/O: @openrouter/sdk wrapper, structured-output ladder + github/ # GitHub I/O: event payload → PrContext, octokit wrappers (diff fetch, review posting) + openrouter/ # OpenRouter I/O: @openrouter/sdk wrapper, structured-output retry ladder, cost summary diff/ # pure transforms over parse-diff output - context/ # (planned — next PR) workspace reads: conventions file, changed files, related files + context/ # workspace I/O: conventions file, changed files, related-files reverse-import scan review/ # pure review logic: finding schema, phases, prompt, selection, comment mapping orchestrate.ts # (planned — next PR) the pipeline — fully testable with stub clients ``` diff --git a/fixtures/openrouter.chat-result.json b/fixtures/openrouter.chat-result.json new file mode 100644 index 0000000..e8c005e --- /dev/null +++ b/fixtures/openrouter.chat-result.json @@ -0,0 +1,15 @@ +{ + "id": "gen-fixture-0001", + "object": "chat.completion", + "model": "openai/gpt-5-mini", + "choices": [ + { + "finishReason": "stop", + "message": { + "role": "assistant", + "content": "{\"analysis\":\"src/greeter.ts: verified the empty-key guard and traced register() callers.\",\"findings\":[]}" + } + } + ], + "usage": { "promptTokens": 12000, "completionTokens": 800, "cost": 0.0421 } +} diff --git a/fixtures/openrouter.invalid-json.json b/fixtures/openrouter.invalid-json.json new file mode 100644 index 0000000..4f617a6 --- /dev/null +++ b/fixtures/openrouter.invalid-json.json @@ -0,0 +1,15 @@ +{ + "id": "gen-fixture-0002", + "object": "chat.completion", + "model": "openai/gpt-5-mini", + "choices": [ + { + "finishReason": "length", + "message": { + "role": "assistant", + "content": "The model rambled instead of emitting JSON {" + } + } + ], + "usage": { "promptTokens": 11800, "completionTokens": 42, "cost": null } +} diff --git a/fixtures/openrouter.schema-mismatch.json b/fixtures/openrouter.schema-mismatch.json new file mode 100644 index 0000000..98591df --- /dev/null +++ b/fixtures/openrouter.schema-mismatch.json @@ -0,0 +1,15 @@ +{ + "id": "gen-fixture-0003", + "object": "chat.completion", + "model": "openai/gpt-5-mini", + "choices": [ + { + "finishReason": "stop", + "message": { + "role": "assistant", + "content": "{\"verdict\":\"lgtm\"}" + } + } + ], + "usage": { "promptTokens": 11900, "completionTokens": 12, "cost": 0.001 } +} diff --git a/fixtures/pull.get.json b/fixtures/pull.get.json new file mode 100644 index 0000000..540c15d --- /dev/null +++ b/fixtures/pull.get.json @@ -0,0 +1,12 @@ +{ + "number": 7, + "state": "open", + "title": "feat: trim names before greeting", + "body": "Trims whitespace from names and validates registry keys.", + "head": { + "sha": "abc123def456abc123def456abc123def456abc1", + "ref": "feat/trim-names" + }, + "base": { "ref": "main" }, + "html_url": "https://github.com/aliasunder/fixture/pull/7" +} diff --git a/fixtures/workspace/AGENTS.md b/fixtures/workspace/AGENTS.md new file mode 100644 index 0000000..1371098 --- /dev/null +++ b/fixtures/workspace/AGENTS.md @@ -0,0 +1,3 @@ +# Fixture conventions + +Use exact assertions. Never mock what you can stub. diff --git a/fixtures/workspace/dist/stale.js b/fixtures/workspace/dist/stale.js new file mode 100644 index 0000000..dbdd02c --- /dev/null +++ b/fixtures/workspace/dist/stale.js @@ -0,0 +1,3 @@ +import { greet } from "../src/greeter.js" + +export const staleGreeting = greet("stale") diff --git a/fixtures/workspace/node_modules/decoy/index.js b/fixtures/workspace/node_modules/decoy/index.js new file mode 100644 index 0000000..bf48162 --- /dev/null +++ b/fixtures/workspace/node_modules/decoy/index.js @@ -0,0 +1,3 @@ +import { greet } from "../../src/greeter.js" + +export const decoyGreeting = greet("decoy") diff --git a/fixtures/workspace/src/__tests__/greeter.test.ts b/fixtures/workspace/src/__tests__/greeter.test.ts new file mode 100644 index 0000000..3c15a52 --- /dev/null +++ b/fixtures/workspace/src/__tests__/greeter.test.ts @@ -0,0 +1,3 @@ +import { greet } from "../greeter.js" + +export const greeting = greet("fixture") diff --git a/fixtures/workspace/src/barrel-consumer.ts b/fixtures/workspace/src/barrel-consumer.ts new file mode 100644 index 0000000..33c6fa5 --- /dev/null +++ b/fixtures/workspace/src/barrel-consumer.ts @@ -0,0 +1,3 @@ +import { LIB_NAME } from "./lib" + +export const describeLib = (): string => `lib: ${LIB_NAME}` diff --git a/fixtures/workspace/src/caller.ts b/fixtures/workspace/src/caller.ts new file mode 100644 index 0000000..916ee5c --- /dev/null +++ b/fixtures/workspace/src/caller.ts @@ -0,0 +1,6 @@ +import { greet } from "./greeter.js" +import { registry } from "./registry.js" + +/** Fixture importer that touches two changed files. */ +export const greetAll = (): string[] => + [...registry.keys()].map((registeredName) => greet(registeredName)) diff --git a/fixtures/workspace/src/consumer.ts b/fixtures/workspace/src/consumer.ts new file mode 100644 index 0000000..b9a6eaa --- /dev/null +++ b/fixtures/workspace/src/consumer.ts @@ -0,0 +1,3 @@ +import { greet } from "./greeter.js" + +export const shout = (name: string): string => greet(name).toUpperCase() diff --git a/fixtures/workspace/src/data.bin b/fixtures/workspace/src/data.bin new file mode 100644 index 0000000..f1fa21b Binary files /dev/null and b/fixtures/workspace/src/data.bin differ diff --git a/fixtures/workspace/src/greeter.ts b/fixtures/workspace/src/greeter.ts new file mode 100644 index 0000000..2f6b453 --- /dev/null +++ b/fixtures/workspace/src/greeter.ts @@ -0,0 +1 @@ +export const greet = (name: string): string => `Hello, ${name.trim()}!` diff --git a/fixtures/workspace/src/hub.ts b/fixtures/workspace/src/hub.ts new file mode 100644 index 0000000..5f940fc --- /dev/null +++ b/fixtures/workspace/src/hub.ts @@ -0,0 +1 @@ +export const HUB = "hub" diff --git a/fixtures/workspace/src/importers/importer-1.ts b/fixtures/workspace/src/importers/importer-1.ts new file mode 100644 index 0000000..7ca66ad --- /dev/null +++ b/fixtures/workspace/src/importers/importer-1.ts @@ -0,0 +1,3 @@ +import { HUB } from "../hub.js" + +export const importerOne = HUB diff --git a/fixtures/workspace/src/importers/importer-2.ts b/fixtures/workspace/src/importers/importer-2.ts new file mode 100644 index 0000000..40eb3b4 --- /dev/null +++ b/fixtures/workspace/src/importers/importer-2.ts @@ -0,0 +1,3 @@ +import { HUB } from "../hub.js" + +export const importerTwo = HUB diff --git a/fixtures/workspace/src/importers/importer-3.ts b/fixtures/workspace/src/importers/importer-3.ts new file mode 100644 index 0000000..cedfe22 --- /dev/null +++ b/fixtures/workspace/src/importers/importer-3.ts @@ -0,0 +1,3 @@ +import { HUB } from "../hub.js" + +export const importerThree = HUB diff --git a/fixtures/workspace/src/importers/importer-4.ts b/fixtures/workspace/src/importers/importer-4.ts new file mode 100644 index 0000000..35467d1 --- /dev/null +++ b/fixtures/workspace/src/importers/importer-4.ts @@ -0,0 +1,3 @@ +import { HUB } from "../hub.js" + +export const importerFour = HUB diff --git a/fixtures/workspace/src/importers/importer-5.ts b/fixtures/workspace/src/importers/importer-5.ts new file mode 100644 index 0000000..54b1666 --- /dev/null +++ b/fixtures/workspace/src/importers/importer-5.ts @@ -0,0 +1,3 @@ +import { HUB } from "../hub.js" + +export const importerFive = HUB diff --git a/fixtures/workspace/src/importers/importer-6.ts b/fixtures/workspace/src/importers/importer-6.ts new file mode 100644 index 0000000..e1c8421 --- /dev/null +++ b/fixtures/workspace/src/importers/importer-6.ts @@ -0,0 +1,3 @@ +import { HUB } from "../hub.js" + +export const importerSix = HUB diff --git a/fixtures/workspace/src/importers/importer-7.ts b/fixtures/workspace/src/importers/importer-7.ts new file mode 100644 index 0000000..368a2e4 --- /dev/null +++ b/fixtures/workspace/src/importers/importer-7.ts @@ -0,0 +1,3 @@ +import { HUB } from "../hub.js" + +export const importerSeven = HUB diff --git a/fixtures/workspace/src/importers/importer-8.ts b/fixtures/workspace/src/importers/importer-8.ts new file mode 100644 index 0000000..924a48d --- /dev/null +++ b/fixtures/workspace/src/importers/importer-8.ts @@ -0,0 +1,3 @@ +import { HUB } from "../hub.js" + +export const importerEight = HUB diff --git a/fixtures/workspace/src/importers/importer-9.ts b/fixtures/workspace/src/importers/importer-9.ts new file mode 100644 index 0000000..98f5287 --- /dev/null +++ b/fixtures/workspace/src/importers/importer-9.ts @@ -0,0 +1,3 @@ +import { HUB } from "../hub.js" + +export const importerNine = HUB diff --git a/fixtures/workspace/src/lib/index.ts b/fixtures/workspace/src/lib/index.ts new file mode 100644 index 0000000..a536ecb --- /dev/null +++ b/fixtures/workspace/src/lib/index.ts @@ -0,0 +1 @@ +export const LIB_NAME = "fixture-lib" diff --git a/fixtures/workspace/src/registry.ts b/fixtures/workspace/src/registry.ts new file mode 100644 index 0000000..97a8f35 --- /dev/null +++ b/fixtures/workspace/src/registry.ts @@ -0,0 +1 @@ +export const registry = new Map() diff --git a/src/context/__tests__/import-resolution.test.ts b/src/context/__tests__/import-resolution.test.ts new file mode 100644 index 0000000..412a2c3 --- /dev/null +++ b/src/context/__tests__/import-resolution.test.ts @@ -0,0 +1,120 @@ +import { describe, expect, it } from "vitest" +import { + extractImportSpecifiers, + resolveImportSpecifier, +} from "../import-resolution.js" + +describe("extractImportSpecifiers", () => { + it("extracts static, dynamic, and require specifiers from one source", () => { + const source = [ + `import { a } from "./a.js"`, + `import type { B } from '../b.js'`, + `const lazy = await import("./lazy.js")`, + `const legacy = require("./legacy.cjs")`, + `import zod from "zod"`, + ].join("\n") + + const specifiers = extractImportSpecifiers(source) + + expect(specifiers).toEqual([ + "./a.js", + "../b.js", + "./lazy.js", + "./legacy.cjs", + "zod", + ]) + }) + + it("extracts a side-effect import specifier", () => { + const specifiers = extractImportSpecifiers(`import "./register.js"`) + + expect(specifiers).toEqual(["./register.js"]) + }) + + it("returns an empty list for a source with no imports", () => { + const specifiers = extractImportSpecifiers(`export const answer = 42`) + + expect(specifiers).toEqual([]) + }) + + it("extracts a quoted specifier inside a comment (grep-style by design)", () => { + const specifiers = extractImportSpecifiers(`// import { x } from "./x.js"`) + + expect(specifiers).toEqual(["./x.js"]) + }) +}) + +describe("resolveImportSpecifier", () => { + const scenarios = [ + { + name: "remaps a sibling .js specifier to .ts and .tsx", + importerPath: "src/caller.ts", + specifier: "./greeter.js", + expected: ["src/greeter.js", "src/greeter.ts", "src/greeter.tsx"], + }, + { + name: "resolves a parent-directory .js specifier", + importerPath: "src/a/b.ts", + specifier: "../util.js", + expected: ["src/util.js", "src/util.ts", "src/util.tsx"], + }, + { + name: "remaps .mjs to .mts", + importerPath: "src/caller.ts", + specifier: "./worker.mjs", + expected: ["src/worker.mjs", "src/worker.mts"], + }, + { + name: "remaps .cjs to .cts", + importerPath: "src/caller.ts", + specifier: "./legacy.cjs", + expected: ["src/legacy.cjs", "src/legacy.cts"], + }, + { + name: "keeps an explicit .ts specifier as the only candidate", + importerPath: "src/caller.ts", + specifier: "./greeter.ts", + expected: ["src/greeter.ts"], + }, + { + name: "expands an extensionless specifier with suffixes and index files", + importerPath: "src/caller.ts", + specifier: "./lib", + expected: [ + "src/lib", + "src/lib.ts", + "src/lib.tsx", + "src/lib.js", + "src/lib.jsx", + "src/lib/index.ts", + "src/lib/index.tsx", + "src/lib/index.js", + "src/lib/index.jsx", + ], + }, + { + name: "returns no candidates for a package specifier", + importerPath: "src/caller.ts", + specifier: "zod", + expected: [], + }, + { + name: "returns no candidates for a node built-in", + importerPath: "src/caller.ts", + specifier: "node:path", + expected: [], + }, + { + name: "returns no candidates when the join escapes the workspace root", + importerPath: "src/caller.ts", + specifier: "../../outside.js", + expected: [], + }, + ] + + it.each(scenarios)("$name", ({ importerPath, specifier, expected }) => { + const candidates = resolveImportSpecifier({ importerPath, specifier }) + + expect(candidates).toEqual(expected) + }) +}) diff --git a/src/context/__tests__/workspace.test.ts b/src/context/__tests__/workspace.test.ts new file mode 100644 index 0000000..dd33d78 --- /dev/null +++ b/src/context/__tests__/workspace.test.ts @@ -0,0 +1,587 @@ +import { readFileSync } from "node:fs" +import { chmod, mkdir, mkdtemp, rm, symlink, writeFile } from "node:fs/promises" +import { tmpdir } from "node:os" +import path from "node:path" +import { fileURLToPath } from "node:url" +import { describe, expect, it } from "vitest" +import { createTestLogger } from "../../__tests__/test-logger.js" +import { estimateTokens } from "../../review/prompt.js" +import { createContextReader, MAX_SCAN_FILES } from "../workspace.js" + +const workspaceRoot = fileURLToPath( + new URL("../../../fixtures/workspace", import.meta.url), +) + +const readFixture = (workspaceRelativePath: string): string => + readFileSync(path.join(workspaceRoot, workspaceRelativePath), "utf8") + +const greeterContent = readFixture("src/greeter.ts") +const registryContent = readFixture("src/registry.ts") +const callerContent = readFixture("src/caller.ts") +const consumerContent = readFixture("src/consumer.ts") +const barrelConsumerContent = readFixture("src/barrel-consumer.ts") +const greeterTestContent = readFixture("src/__tests__/greeter.test.ts") + +const makeReader = () => { + const logger = createTestLogger() + const contextReader = createContextReader({ workspaceRoot }, logger) + return { contextReader, logger } +} + +/** Writes a throwaway workspace tree for tests that need sizes or file + * counts too unwieldy to commit as fixtures. */ +const makeTempWorkspace = async ( + files: Record, +): Promise<{ root: string; cleanup: () => Promise }> => { + const root = await mkdtemp(path.join(tmpdir(), "umm-workspace-")) + await Promise.all( + Object.entries(files).map(async ([filePath, content]) => { + await mkdir(path.dirname(path.join(root, filePath)), { recursive: true }) + await writeFile(path.join(root, filePath), content, "utf8") + }), + ) + return { root, cleanup: () => rm(root, { recursive: true, force: true }) } +} + +describe("readConventions", () => { + it("returns the conventions file content", async () => { + const { contextReader } = makeReader() + + const conventions = await contextReader.readConventions({ + conventionsFile: "AGENTS.md", + }) + + expect(conventions).toBe( + "# Fixture conventions\n\nUse exact assertions. Never mock what you can stub.\n", + ) + }) + + it("returns null when the conventions file is missing", async () => { + const { contextReader } = makeReader() + + const conventions = await contextReader.readConventions({ + conventionsFile: "MISSING.md", + }) + + expect(conventions).toBeNull() + }) + + it("throws when the conventions path escapes the workspace", async () => { + const { contextReader } = makeReader() + + await expect( + contextReader.readConventions({ conventionsFile: "../outside.md" }), + ).rejects.toThrow(new Error("path escapes the workspace: ../outside.md")) + }) + + it("follows a symlinked conventions file whose target is inside the workspace", async () => { + const { root, cleanup } = await makeTempWorkspace({ + "CLAUDE.md": "# Conventions behind a symlink\n", + }) + await symlink(path.join(root, "CLAUDE.md"), path.join(root, "AGENTS.md")) + const contextReader = createContextReader( + { workspaceRoot: root }, + createTestLogger(), + ) + + try { + const conventions = await contextReader.readConventions({ + conventionsFile: "AGENTS.md", + }) + + expect(conventions).toBe("# Conventions behind a symlink\n") + } finally { + await cleanup() + } + }) + + it("throws when the conventions file symlinks to a target outside the workspace", async () => { + // The outside target must exist: a dangling link would reject with ENOENT + // (the missing-file path) and pass this test without the escape guard + const outsideRoot = await mkdtemp(path.join(tmpdir(), "umm-outside-")) + await writeFile( + path.join(outsideRoot, "secret.md"), + "runner secret", + "utf8", + ) + const { root, cleanup } = await makeTempWorkspace({ + "README.md": "# fixture\n", + }) + await symlink( + path.join(outsideRoot, "secret.md"), + path.join(root, "AGENTS.md"), + ) + const contextReader = createContextReader( + { workspaceRoot: root }, + createTestLogger(), + ) + + try { + await expect( + contextReader.readConventions({ conventionsFile: "AGENTS.md" }), + ).rejects.toThrow(new Error("path escapes the workspace: AGENTS.md")) + } finally { + await cleanup() + await rm(outsideRoot, { recursive: true, force: true }) + } + }) +}) + +describe("readChangedFiles", () => { + it("includes files in full and subtracts their tokens from the budget", async () => { + const { contextReader } = makeReader() + + const result = await contextReader.readChangedFiles({ + changedPaths: ["src/greeter.ts", "src/registry.ts"], + budgetTokens: 10_000, + }) + + expect(result).toEqual({ + files: [ + { path: "src/greeter.ts", content: greeterContent, includedAs: "full" }, + { + path: "src/registry.ts", + content: registryContent, + includedAs: "full", + }, + ], + remainingTokens: + 10_000 - + estimateTokens(greeterContent) - + estimateTokens(registryContent), + }) + }) + + it("flips an over-budget file to diff-only while a later smaller file still fits", async () => { + const { contextReader } = makeReader() + + const result = await contextReader.readChangedFiles({ + changedPaths: ["src/caller.ts", "src/greeter.ts"], + budgetTokens: estimateTokens(greeterContent), + }) + + expect(result).toEqual({ + files: [ + { path: "src/caller.ts", content: "", includedAs: "diff-only" }, + { path: "src/greeter.ts", content: greeterContent, includedAs: "full" }, + ], + remainingTokens: 0, + }) + }) + + it("demotes a file whose byte size exceeds the byte budget before reading it", async () => { + // Multibyte content: 400 chars fit a 100-token budget, but 1200 UTF-8 + // bytes exceed it — only the stat-first byte guard demotes this file; + // the post-read token check would have included it. + const multibyteContent = "あ".repeat(400) + const { root, cleanup } = await makeTempWorkspace({ + "src/large.ts": multibyteContent, + }) + const contextReader = createContextReader( + { workspaceRoot: root }, + createTestLogger(), + ) + + try { + const result = await contextReader.readChangedFiles({ + changedPaths: ["src/large.ts"], + budgetTokens: estimateTokens(multibyteContent), + }) + + expect(result).toEqual({ + files: [{ path: "src/large.ts", content: "", includedAs: "diff-only" }], + remainingTokens: estimateTokens(multibyteContent), + }) + } finally { + await cleanup() + } + }) + + it("includes a missing file as diff-only with an info log, not a warning", async () => { + const { contextReader, logger } = makeReader() + + const result = await contextReader.readChangedFiles({ + changedPaths: ["src/missing.ts"], + budgetTokens: 10_000, + }) + + expect(result.files).toEqual([ + { path: "src/missing.ts", content: "", includedAs: "diff-only" }, + ]) + expect(logger.messages).toContainEqual({ + level: "info", + message: "changed file missing from checkout — including as diff-only", + data: { path: "src/missing.ts" }, + }) + expect( + logger.messages.filter((message) => message.level === "warn"), + ).toEqual([]) + }) + + it("includes an unreadable file as diff-only with a warning", async () => { + const { root, cleanup } = await makeTempWorkspace({ + "src/locked.ts": "export const locked = 1\n", + }) + await chmod(path.join(root, "src/locked.ts"), 0o000) + const logger = createTestLogger() + const contextReader = createContextReader({ workspaceRoot: root }, logger) + + try { + const result = await contextReader.readChangedFiles({ + changedPaths: ["src/locked.ts"], + budgetTokens: 10_000, + }) + + expect(result.files).toEqual([ + { path: "src/locked.ts", content: "", includedAs: "diff-only" }, + ]) + expect(logger.messages).toContainEqual({ + level: "warn", + message: "changed file unreadable — including as diff-only", + data: { + path: "src/locked.ts", + error: expect.stringContaining("EACCES"), + }, + }) + } finally { + await cleanup() + } + }) + + it("includes a binary file as diff-only without a warning", async () => { + const { contextReader, logger } = makeReader() + + const result = await contextReader.readChangedFiles({ + changedPaths: ["src/data.bin"], + budgetTokens: 10_000, + }) + + expect(result.files).toEqual([ + { path: "src/data.bin", content: "", includedAs: "diff-only" }, + ]) + expect( + logger.messages.filter((message) => message.level === "warn"), + ).toEqual([]) + }) + + it("throws when a changed path escapes the workspace", async () => { + const { contextReader } = makeReader() + + await expect( + contextReader.readChangedFiles({ + changedPaths: ["../../etc/passwd"], + budgetTokens: 10_000, + }), + ).rejects.toThrow(new Error("path escapes the workspace: ../../etc/passwd")) + }) + + it("degrades a changed file that symlinks outside the workspace to diff-only", async () => { + // The outside target must exist: a dangling link would take the + // unreadable path and pass this test without the escape guard + const outsideRoot = await mkdtemp(path.join(tmpdir(), "umm-outside-")) + await writeFile( + path.join(outsideRoot, "secret.txt"), + "runner secret", + "utf8", + ) + const { root, cleanup } = await makeTempWorkspace({ + "src/ok.ts": "export const ok = 1\n", + }) + await symlink( + path.join(outsideRoot, "secret.txt"), + path.join(root, "src/leak.ts"), + ) + const logger = createTestLogger() + const contextReader = createContextReader({ workspaceRoot: root }, logger) + + try { + const result = await contextReader.readChangedFiles({ + changedPaths: ["src/leak.ts"], + budgetTokens: 10_000, + }) + + expect(result.files).toEqual([ + { path: "src/leak.ts", content: "", includedAs: "diff-only" }, + ]) + expect(logger.messages).toContainEqual({ + level: "warn", + message: + "changed file resolves outside the reviewable workspace — including as diff-only", + data: { path: "src/leak.ts" }, + }) + } finally { + await cleanup() + await rm(outsideRoot, { recursive: true, force: true }) + } + }) + + it("degrades a changed file that symlinks into .git to diff-only", async () => { + const { root, cleanup } = await makeTempWorkspace({ + ".git/config": "[http]\n\textraheader = AUTHORIZATION: basic abc123\n", + }) + await symlink(path.join(root, ".git/config"), path.join(root, "leak.ts")) + const logger = createTestLogger() + const contextReader = createContextReader({ workspaceRoot: root }, logger) + + try { + const result = await contextReader.readChangedFiles({ + changedPaths: ["leak.ts"], + budgetTokens: 10_000, + }) + + expect(result.files).toEqual([ + { path: "leak.ts", content: "", includedAs: "diff-only" }, + ]) + expect(logger.messages).toContainEqual({ + level: "warn", + message: + "changed file resolves outside the reviewable workspace — including as diff-only", + data: { path: "leak.ts" }, + }) + } finally { + await cleanup() + } + }) +}) + +describe("findRelatedFiles", () => { + it("ranks importers by changed-file import count with test files last", async () => { + const { contextReader } = makeReader() + + const relatedFiles = await contextReader.findRelatedFiles({ + changedPaths: ["src/greeter.ts", "src/registry.ts"], + budgetTokens: 100_000, + }) + + expect(relatedFiles).toEqual([ + { + path: "src/caller.ts", + content: callerContent, + includedAs: "full", + reason: "imports src/greeter.ts, src/registry.ts", + }, + { + path: "src/consumer.ts", + content: consumerContent, + includedAs: "full", + reason: "imports src/greeter.ts", + }, + { + path: "src/__tests__/greeter.test.ts", + content: greeterTestContent, + includedAs: "full", + reason: "imports src/greeter.ts", + }, + ]) + }) + + it("resolves a barrel import to the changed index file", async () => { + const { contextReader } = makeReader() + + const relatedFiles = await contextReader.findRelatedFiles({ + changedPaths: ["src/lib/index.ts"], + budgetTokens: 100_000, + }) + + expect(relatedFiles).toEqual([ + { + path: "src/barrel-consumer.ts", + content: barrelConsumerContent, + includedAs: "full", + reason: "imports src/lib/index.ts", + }, + ]) + }) + + it("excludes files that are themselves changed", async () => { + const { contextReader } = makeReader() + + const relatedFiles = await contextReader.findRelatedFiles({ + changedPaths: ["src/greeter.ts", "src/caller.ts"], + budgetTokens: 100_000, + }) + + expect(relatedFiles.map((relatedFile) => relatedFile.path)).toEqual([ + "src/consumer.ts", + "src/__tests__/greeter.test.ts", + ]) + }) + + it("caps related files at eight, dropping the lowest-ranked importer", async () => { + const { contextReader } = makeReader() + + const relatedFiles = await contextReader.findRelatedFiles({ + changedPaths: ["src/hub.ts"], + budgetTokens: 100_000, + }) + + expect(relatedFiles.map((relatedFile) => relatedFile.path)).toEqual([ + "src/importers/importer-1.ts", + "src/importers/importer-2.ts", + "src/importers/importer-3.ts", + "src/importers/importer-4.ts", + "src/importers/importer-5.ts", + "src/importers/importer-6.ts", + "src/importers/importer-7.ts", + "src/importers/importer-8.ts", + ]) + }) + + it("skips an over-budget candidate but keeps smaller ones that fit", async () => { + const { contextReader } = makeReader() + + const relatedFiles = await contextReader.findRelatedFiles({ + changedPaths: ["src/greeter.ts", "src/registry.ts"], + budgetTokens: estimateTokens(consumerContent), + }) + + expect(relatedFiles).toEqual([ + { + path: "src/consumer.ts", + content: consumerContent, + includedAs: "full", + reason: "imports src/greeter.ts", + }, + ]) + }) + + it("skips files larger than the scan byte cap", async () => { + const oversizedContent = `import { target } from "./target.js"\n${"x".repeat(300_000)}` + const { root, cleanup } = await makeTempWorkspace({ + "target.ts": `export const target = "target"\n`, + "big.ts": oversizedContent, + "small.ts": `import { target } from "./target.js"\nexport const small = target\n`, + }) + const logger = createTestLogger() + const contextReader = createContextReader({ workspaceRoot: root }, logger) + + try { + const relatedFiles = await contextReader.findRelatedFiles({ + changedPaths: ["target.ts"], + budgetTokens: 1_000_000, + }) + + expect(relatedFiles.map((relatedFile) => relatedFile.path)).toEqual([ + "small.ts", + ]) + } finally { + await cleanup() + } + }) + + it("excludes importers under pruned and hidden directories from the scan", async () => { + const importTarget = `import { target } from "../target.js"\nexport const found = target\n` + const { root, cleanup } = await makeTempWorkspace({ + "target.ts": `export const target = "target"\n`, + "src/legit.ts": importTarget, + "node_modules/dependency.ts": importTarget, + "dist/compiled.ts": importTarget, + ".hidden/covert.ts": importTarget, + }) + const contextReader = createContextReader( + { workspaceRoot: root }, + createTestLogger(), + ) + + try { + const relatedFiles = await contextReader.findRelatedFiles({ + changedPaths: ["target.ts"], + budgetTokens: 100_000, + }) + + expect(relatedFiles.map((relatedFile) => relatedFile.path)).toEqual([ + "src/legit.ts", + ]) + } finally { + await cleanup() + } + }) + + it("excludes an unreadable scanned file from related files with a warning", async () => { + const importTarget = `import { target } from "./target.js"\nexport const found = target\n` + const { root, cleanup } = await makeTempWorkspace({ + "target.ts": `export const target = "target"\n`, + "readable.ts": importTarget, + "locked.ts": importTarget, + }) + await chmod(path.join(root, "locked.ts"), 0o000) + const logger = createTestLogger() + const contextReader = createContextReader({ workspaceRoot: root }, logger) + + try { + const relatedFiles = await contextReader.findRelatedFiles({ + changedPaths: ["target.ts"], + budgetTokens: 100_000, + }) + + // readable.ts still arriving proves the scan carried on past the failure + expect(relatedFiles.map((relatedFile) => relatedFile.path)).toEqual([ + "readable.ts", + ]) + expect(logger.messages).toContainEqual({ + level: "warn", + message: "scanned file unreadable — excluding from related files", + data: { + path: "locked.ts", + error: expect.stringContaining("EACCES"), + }, + }) + } finally { + await cleanup() + } + }) + + it("excludes an importer carrying a NUL byte, matching the changed-file binary rule", async () => { + const importTarget = `import { target } from "./target.js"\nexport const found = target\n` + const { root, cleanup } = await makeTempWorkspace({ + "target.ts": `export const target = "target"\n`, + "clean.ts": importTarget, + "binary.ts": `${importTarget}export const marker = "\x00"\n`, + }) + const contextReader = createContextReader( + { workspaceRoot: root }, + createTestLogger(), + ) + + try { + const relatedFiles = await contextReader.findRelatedFiles({ + changedPaths: ["target.ts"], + budgetTokens: 100_000, + }) + + expect(relatedFiles.map((relatedFile) => relatedFile.path)).toEqual([ + "clean.ts", + ]) + } finally { + await cleanup() + } + }) + + it("stops scanning at the file cap and warns that detection may be incomplete", async () => { + const fillerFiles = Object.fromEntries( + Array.from({ length: MAX_SCAN_FILES + 1 }, (_, fillerIndex) => [ + `filler-${String(fillerIndex).padStart(5, "0")}.ts`, + "export {}\n", + ]), + ) + const { root, cleanup } = await makeTempWorkspace(fillerFiles) + const logger = createTestLogger() + const contextReader = createContextReader({ workspaceRoot: root }, logger) + + try { + await contextReader.findRelatedFiles({ + changedPaths: ["filler-00000.ts"], + budgetTokens: 100_000, + }) + + expect(logger.messages).toContainEqual({ + level: "warn", + message: + "workspace scan capped — related-file detection may be incomplete", + data: { maxScanFiles: MAX_SCAN_FILES }, + }) + } finally { + await cleanup() + } + }) +}) diff --git a/src/context/import-resolution.ts b/src/context/import-resolution.ts new file mode 100644 index 0000000..bc37bda --- /dev/null +++ b/src/context/import-resolution.ts @@ -0,0 +1,66 @@ +import { posix } from "node:path" + +/** + * Matches static imports (`from "x"` and side-effect `import "x"`), dynamic + * `import("x")`, and CommonJS `require("x")`, capturing the quoted specifier. + * Deliberately grep-style: a false positive inside a comment or string only + * costs a harmless extra candidate lookup, never a wrong review. + */ +const IMPORT_SPECIFIER_PATTERN = + /(?:from\s+|import\s*\(\s*|require\s*\(\s*|import\s+)["']([^"']+)["']/g + +export const extractImportSpecifiers = (source: string): string[] => { + return [...source.matchAll(IMPORT_SPECIFIER_PATTERN)].flatMap( + (specifierMatch) => + specifierMatch[1] === undefined ? [] : [specifierMatch[1]], + ) +} + +/** Compiled `.js`-family specifiers and the source extensions they may compile from. */ +const SOURCE_EXTENSION_REMAPS: Record = { + ".js": [".ts", ".tsx"], + ".mjs": [".mts"], + ".cjs": [".cts"], +} + +const EXTENSIONLESS_SUFFIXES = [".ts", ".tsx", ".js", ".jsx"] +const INDEX_BASENAMES = ["index.ts", "index.tsx", "index.js", "index.jsx"] + +/** + * Expands a relative import specifier into every workspace-relative posix + * path it could resolve to: the exact join, `.js`→`.ts`-family remaps (the + * ESM convention of importing compiled names from TS source), extensionless + * resolution, and directory-index resolution. Non-relative specifiers + * (packages, node built-ins) return no candidates. + */ +export const resolveImportSpecifier = ({ + importerPath, + specifier, +}: { + importerPath: string + specifier: string +}): string[] => { + if (!specifier.startsWith("./") && !specifier.startsWith("../")) return [] + + const joined = posix.join(posix.dirname(importerPath), specifier) + // A join that climbs above the workspace root can never match a changed file + if (joined.startsWith("../")) return [] + + const extension = posix.extname(joined) + if (extension === "") { + return [ + joined, + ...EXTENSIONLESS_SUFFIXES.map((suffix) => `${joined}${suffix}`), + ...INDEX_BASENAMES.map((indexBasename) => `${joined}/${indexBasename}`), + ] + } + + const remappedExtensions = SOURCE_EXTENSION_REMAPS[extension] ?? [] + const withoutExtension = joined.slice(0, -extension.length) + return [ + joined, + ...remappedExtensions.map( + (remappedExtension) => `${withoutExtension}${remappedExtension}`, + ), + ] +} diff --git a/src/context/workspace.ts b/src/context/workspace.ts new file mode 100644 index 0000000..a456bf6 --- /dev/null +++ b/src/context/workspace.ts @@ -0,0 +1,352 @@ +import { readFile, readdir, realpath, stat } from "node:fs/promises" +import path, { posix } from "node:path" +import type { Logger } from "../logger.js" +import { + CHARS_PER_TOKEN, + estimateTokens, + type PromptFile, +} from "../review/prompt.js" +import { + extractImportSpecifiers, + resolveImportSpecifier, +} from "./import-resolution.js" + +export type BudgetedFiles = { files: PromptFile[]; remainingTokens: number } + +export type ContextReader = { + /** null when the file is missing — the prompt renders a "(no conventions…)" block. */ + readConventions: (params: { + conventionsFile: string + }) => Promise + readChangedFiles: (params: { + changedPaths: string[] + budgetTokens: number + }) => Promise + findRelatedFiles: (params: { + changedPaths: string[] + budgetTokens: number + }) => Promise +} + +const SCANNABLE_EXTENSIONS = new Set([ + ".ts", + ".tsx", + ".mts", + ".cts", + ".js", + ".jsx", + ".mjs", + ".cjs", +]) + +/** Dependency, VCS, and build-output trees — never review context. */ +const PRUNED_DIRECTORIES = new Set([ + "node_modules", + ".git", + "dist", + "build", + "out", + "coverage", +]) + +/** Scan bounds: a related-files miss on a pathological repo beats an unbounded walk. */ +export const MAX_SCAN_FILES = 5_000 +export const MAX_SCAN_BYTES = 262_144 +export const RELATED_FILES_MAX = 8 + +type ImporterCandidate = { + path: string + importedChangedPaths: string[] + content: string +} + +const isTestFile = (filePath: string): boolean => + filePath.includes("__tests__/") || /\.test\.[^./]+$/.test(filePath) + +/** + * Import-count desc, then non-test files before test files (test files feed + * the test-quality dimension but carry less caller context), then path asc + * for determinism. + */ +const byRelevance = (a: ImporterCandidate, b: ImporterCandidate): number => { + if (a.importedChangedPaths.length !== b.importedChangedPaths.length) { + return b.importedChangedPaths.length - a.importedChangedPaths.length + } + const aIsTest = isTestFile(a.path) + if (aIsTest !== isTestFile(b.path)) return aIsTest ? 1 : -1 + return a.path < b.path ? -1 : 1 +} + +const isMissingFileError = (error: unknown): boolean => { + return ( + typeof error === "object" && + error !== null && + "code" in error && + error.code === "ENOENT" + ) +} + +export const createContextReader = ( + { workspaceRoot }: { workspaceRoot: string }, + logger: Logger, +): ContextReader => { + const resolvedRoot = path.resolve(workspaceRoot) + + /** Changed-file paths come from the diff and are PR-author-influenced — + * a traversal outside the workspace is an attack, not a lookup miss. */ + const resolveUnderRoot = (relativePath: string): string => { + const resolvedPath = path.resolve(resolvedRoot, relativePath) + if (!resolvedPath.startsWith(resolvedRoot + path.sep)) { + throw new Error(`path escapes the workspace: ${relativePath}`) + } + return resolvedPath + } + + /** + * The lexical check above can't see symlinks: a PR can commit a link whose + * target sits outside the workspace, or inside `.git/` (where the checkout + * token lives in `.git/config` under persist-credentials) — and readFile + * would follow it straight into the prompt, which is echoed into a public + * review. Resolve the real path and re-check before any content is read. + * In-workspace links (e.g. AGENTS.md → CLAUDE.md) stay readable. The root + * is realpath'd too — it may itself sit behind a symlink (macOS tmpdir). + * Missing files reject with ENOENT so callers keep their existing handling. + */ + const realPathIfSafe = async ( + absolutePath: string, + ): Promise => { + const [realRoot, realPath] = await Promise.all([ + realpath(resolvedRoot), + realpath(absolutePath), + ]) + const isUnderRoot = realPath.startsWith(realRoot + path.sep) + const isInGitDirectory = realPath.startsWith( + path.join(realRoot, ".git") + path.sep, + ) + return isUnderRoot && !isInGitDirectory ? realPath : null + } + + const readConventions = async ({ + conventionsFile, + }: { + conventionsFile: string + }): Promise => { + const absolutePath = resolveUnderRoot(conventionsFile) + try { + const safePath = await realPathIfSafe(absolutePath) + if (safePath === null) { + throw new Error(`path escapes the workspace: ${conventionsFile}`) + } + return await readFile(safePath, "utf8") + } catch (readError) { + if (isMissingFileError(readError)) { + logger.info("no conventions file found", { path: conventionsFile }) + return null + } + throw readError + } + } + + /** null on any unreadable changed file. A symlink that resolves outside + * the safe zone is degraded to diff-only rather than fatal — one hostile + * or broken file must not kill the whole review — but gets its own + * warning so the exclusion is auditable. A missing file logs at info, + * not warn: PRs routinely delete files, and the deleted path still + * appears in changedPaths. */ + const readChangedFileOrNull = async ( + absolutePath: string, + changedPath: string, + ): Promise => { + try { + const safePath = await realPathIfSafe(absolutePath) + if (safePath !== null) return await readFile(safePath, "utf8") + logger.warn( + "changed file resolves outside the reviewable workspace — including as diff-only", + { path: changedPath }, + ) + return null + } catch (readError) { + if (isMissingFileError(readError)) { + logger.info( + "changed file missing from checkout — including as diff-only", + { + path: changedPath, + }, + ) + return null + } + logger.warn("changed file unreadable — including as diff-only", { + path: changedPath, + error: String(readError), + }) + return null + } + } + + /** Best-effort read for scan candidates — an unreadable file drops out of + * related-files context, logged so the exclusion is auditable. */ + const readScannedFileOrNull = async ( + scannedPath: string, + ): Promise => { + try { + return await readFile(path.join(resolvedRoot, scannedPath), "utf8") + } catch (readError) { + logger.warn("scanned file unreadable — excluding from related files", { + path: scannedPath, + error: String(readError), + }) + return null + } + } + + const readChangedFiles = async ({ + changedPaths, + budgetTokens, + }: { + changedPaths: string[] + budgetTokens: number + }): Promise => { + const files: PromptFile[] = [] + // Sequential state by design: diff order is priority order, and each + // file's inclusion depends on the budget left by the files before it. + let remainingTokens = budgetTokens + + for (const changedPath of changedPaths) { + const absolutePath = resolveUnderRoot(changedPath) + // Stat before reading: a changed lockfile or bundle can be arbitrarily + // large, and the token check below only runs after the full read. UTF-8 + // bytes ≥ chars, so a file whose bytes exceed the remaining character + // budget can never fit — demote it without pulling it into memory. + // A failed stat (e.g. deleted file) falls through to the read path, + // which already logs and degrades per error kind. + const fileStats = await stat(absolutePath).catch(() => null) + if ( + fileStats !== null && + fileStats.size > remainingTokens * CHARS_PER_TOKEN + ) { + files.push({ path: changedPath, content: "", includedAs: "diff-only" }) + continue + } + const content = await readChangedFileOrNull(absolutePath, changedPath) + if (content === null) { + files.push({ path: changedPath, content: "", includedAs: "diff-only" }) + continue + } + // A NUL byte marks binary content — meaningless in a text prompt + if (content.includes("\x00")) { + files.push({ path: changedPath, content: "", includedAs: "diff-only" }) + continue + } + const contentTokens = estimateTokens(content) + if (contentTokens > remainingTokens) { + files.push({ path: changedPath, content: "", includedAs: "diff-only" }) + continue + } + remainingTokens -= contentTokens + files.push({ path: changedPath, content, includedAs: "full" }) + } + + return { files, remainingTokens } + } + + const scanWorkspaceSourceFiles = async (): Promise => { + const sourceFilePaths: string[] = [] + // Mutable queue by design: a breadth-first walk — subdirectories found + // during iteration join the queue and are visited by the advancing index. + const directoryQueue = [""] + + for (let queueIndex = 0; queueIndex < directoryQueue.length; queueIndex++) { + const currentDirectory = directoryQueue[queueIndex] + if (currentDirectory === undefined) continue + + const entries = await readdir(path.join(resolvedRoot, currentDirectory), { + withFileTypes: true, + }) + // readdir order is platform-dependent — sort so scan-cap cutoffs are deterministic + const sortedEntries = [...entries].sort((a, b) => + a.name < b.name ? -1 : 1, + ) + + for (const entry of sortedEntries) { + const entryPath = posix.join(currentDirectory, entry.name) + if (entry.isDirectory()) { + const isPruned = + PRUNED_DIRECTORIES.has(entry.name) || entry.name.startsWith(".") + if (!isPruned) directoryQueue.push(entryPath) + continue + } + if (!entry.isFile()) continue + if (!SCANNABLE_EXTENSIONS.has(posix.extname(entry.name))) continue + + if (sourceFilePaths.length >= MAX_SCAN_FILES) { + logger.warn( + "workspace scan capped — related-file detection may be incomplete", + { maxScanFiles: MAX_SCAN_FILES }, + ) + return sourceFilePaths + } + const fileStats = await stat(path.join(resolvedRoot, entryPath)) + if (fileStats.size > MAX_SCAN_BYTES) continue + sourceFilePaths.push(entryPath) + } + } + + return sourceFilePaths + } + + const findRelatedFiles = async ({ + changedPaths, + budgetTokens, + }: { + changedPaths: string[] + budgetTokens: number + }): Promise => { + const changedPathSet = new Set(changedPaths) + const scannedPaths = await scanWorkspaceSourceFiles() + + const importers: ImporterCandidate[] = [] + for (const scannedPath of scannedPaths) { + if (changedPathSet.has(scannedPath)) continue + const content = await readScannedFileOrNull(scannedPath) + if (content === null) continue + // A NUL byte marks binary content — the same exclusion readChangedFiles + // applies; a source-extension file can still carry one in a string literal + if (content.includes("\x00")) continue + + const importedChangedPaths = [ + ...new Set( + extractImportSpecifiers(content) + .flatMap((specifier) => + resolveImportSpecifier({ importerPath: scannedPath, specifier }), + ) + .filter((candidatePath) => changedPathSet.has(candidatePath)), + ), + ].sort() + if (importedChangedPaths.length === 0) continue + importers.push({ path: scannedPath, importedChangedPaths, content }) + } + + const rankedImporters = [...importers].sort(byRelevance) + + const relatedFiles: PromptFile[] = [] + // Sequential state by design: rank order is priority order, and an + // over-budget candidate is skipped so smaller candidates still fit. + let remainingTokens = budgetTokens + for (const importer of rankedImporters) { + if (relatedFiles.length >= RELATED_FILES_MAX) break + const contentTokens = estimateTokens(importer.content) + if (contentTokens > remainingTokens) continue + remainingTokens -= contentTokens + relatedFiles.push({ + path: importer.path, + content: importer.content, + includedAs: "full", + reason: `imports ${importer.importedChangedPaths.join(", ")}`, + }) + } + + return relatedFiles + } + + return { readConventions, readChangedFiles, findRelatedFiles } +} diff --git a/src/github/__tests__/client.test.ts b/src/github/__tests__/client.test.ts new file mode 100644 index 0000000..52681c3 --- /dev/null +++ b/src/github/__tests__/client.test.ts @@ -0,0 +1,379 @@ +import { readFileSync } from "node:fs" +import type { getOctokit } from "@actions/github" +import { describe, expect, expectTypeOf, it } from "vitest" +import { createTestLogger } from "../../__tests__/test-logger.js" +import type { ReviewComment } from "../../review/comment-mapping.js" +import { createGithubClient, type OctokitLike } from "../client.js" + +const pullGetResponse: Record = JSON.parse( + readFileSync( + new URL("../../../fixtures/pull.get.json", import.meta.url), + "utf8", + ), +) + +type StubResponse = { data: unknown } | { error: unknown } + +/** Records every call; replays queued responses in order, throwing queued errors. */ +const makeOctokitStub = ({ + getResponses = [], + createReviewResponses = [], +}: { + getResponses?: StubResponse[] + createReviewResponses?: StubResponse[] +} = {}) => { + const getCalls: Record[] = [] + const createReviewCalls: Record[] = [] + + const takeNext = ( + queue: StubResponse[], + callCount: number, + operation: string, + ): { data: unknown } => { + const next = queue[callCount - 1] + if (next === undefined) { + throw new Error(`stub: unexpected ${operation} call #${callCount}`) + } + if ("error" in next) throw next.error + return next + } + + const octokit: OctokitLike = { + rest: { + pulls: { + get: async (params) => { + getCalls.push(params) + return takeNext(getResponses, getCalls.length, "pulls.get") + }, + createReview: async (params) => { + createReviewCalls.push(params) + return takeNext( + createReviewResponses, + createReviewCalls.length, + "pulls.createReview", + ) + }, + }, + }, + } + + return { octokit, getCalls, createReviewCalls } +} + +const makeClient = (stub: { octokit: OctokitLike }) => { + const logger = createTestLogger() + const client = createGithubClient( + { octokit: stub.octokit, owner: "aliasunder", repo: "fixture" }, + logger, + ) + return { client, logger } +} + +const makeStatusError = (status: number): Error => + Object.assign(new Error(`HTTP ${status}`), { status }) + +const inlineComment: ReviewComment = { + path: "src/greeter.ts", + line: 145, + side: "RIGHT", + body: "Whitespace-only keys pass the empty-key guard", +} + +describe("OctokitLike", () => { + it("stays assignable from the real getOctokit() instance", () => { + // Compile-time assertion: fails `tsc` (and this suite) if an + // @actions/github upgrade drifts the real octokit's shape away from + // the structural type our client accepts. + expectTypeOf>().toExtend() + }) +}) + +describe("fetchPullRequest", () => { + it("maps the pulls.get response onto a PrContext", async () => { + const stub = makeOctokitStub({ getResponses: [{ data: pullGetResponse }] }) + const { client } = makeClient(stub) + + const prContext = await client.fetchPullRequest({ prNumber: 7 }) + + expect(stub.getCalls).toEqual([ + { owner: "aliasunder", repo: "fixture", pull_number: 7 }, + ]) + expect(prContext).toEqual({ + prNumber: 7, + title: "feat: trim names before greeting", + body: "Trims whitespace from names and validates registry keys.", + headSha: "abc123def456abc123def456abc123def456abc1", + headRef: "feat/trim-names", + baseRef: "main", + }) + }) + + it("preserves a null PR body", async () => { + const stub = makeOctokitStub({ + getResponses: [{ data: { ...pullGetResponse, body: null } }], + }) + const { client } = makeClient(stub) + + const prContext = await client.fetchPullRequest({ prNumber: 7 }) + + expect(prContext.body).toBeNull() + }) + + it("throws on a response missing the pull request fields", async () => { + const stub = makeOctokitStub({ + getResponses: [{ data: { message: "Not Found" } }], + }) + const { client } = makeClient(stub) + + await expect(client.fetchPullRequest({ prNumber: 7 })).rejects.toThrow( + "unexpected pull request response shape", + ) + }) +}) + +describe("fetchDiff", () => { + it("requests the diff media type and returns the diff string", async () => { + const diff = "diff --git a/src/greeter.ts b/src/greeter.ts\n" + const stub = makeOctokitStub({ getResponses: [{ data: diff }] }) + const { client } = makeClient(stub) + + const result = await client.fetchDiff({ prNumber: 7 }) + + expect(stub.getCalls).toEqual([ + { + owner: "aliasunder", + repo: "fixture", + pull_number: 7, + mediaType: { format: "diff" }, + }, + ]) + expect(result).toEqual({ kind: "ok", diff }) + }) + + it("maps a 406 to too_large", async () => { + const stub = makeOctokitStub({ + getResponses: [{ error: makeStatusError(406) }], + }) + const { client, logger } = makeClient(stub) + + const result = await client.fetchDiff({ prNumber: 7 }) + + expect(result).toEqual({ kind: "too_large" }) + expect(logger.messages).toContainEqual({ + level: "warn", + message: "diff exceeds GitHub's diff API limits", + data: { prNumber: 7 }, + }) + }) + + it("rethrows a non-406 error", async () => { + const stub = makeOctokitStub({ + getResponses: [{ error: makeStatusError(500) }], + }) + const { client } = makeClient(stub) + + await expect(client.fetchDiff({ prNumber: 7 })).rejects.toThrow("HTTP 500") + }) + + it("rethrows an error that carries no status property", async () => { + const stub = makeOctokitStub({ + getResponses: [{ error: new Error("socket hang up") }], + }) + const { client } = makeClient(stub) + + await expect(client.fetchDiff({ prNumber: 7 })).rejects.toThrow( + "socket hang up", + ) + }) + + it("throws when the diff response is not a string", async () => { + const stub = makeOctokitStub({ getResponses: [{ data: { diff: true } }] }) + const { client } = makeClient(stub) + + await expect(client.fetchDiff({ prNumber: 7 })).rejects.toThrow( + "expected a unified diff string from the diff media type", + ) + }) +}) + +describe("submitReview", () => { + const reviewUrl = + "https://github.com/aliasunder/fixture/pull/7#pullrequestreview-1" + + it("posts body and comments and returns the review url", async () => { + const stub = makeOctokitStub({ + createReviewResponses: [{ data: { html_url: reviewUrl } }], + }) + const { client } = makeClient(stub) + + const result = await client.submitReview({ + prNumber: 7, + commitId: "abc123", + body: "review body", + comments: [inlineComment], + fallbackBody: "fallback body", + }) + + expect(stub.createReviewCalls).toEqual([ + { + owner: "aliasunder", + repo: "fixture", + pull_number: 7, + commit_id: "abc123", + event: "COMMENT", + body: "review body", + comments: [inlineComment], + }, + ]) + expect(result).toEqual({ url: reviewUrl, usedFallbackBody: false }) + }) + + it("omits the comments key entirely on a zero-comment review", async () => { + const stub = makeOctokitStub({ + createReviewResponses: [{ data: { html_url: reviewUrl } }], + }) + const { client } = makeClient(stub) + + await client.submitReview({ + prNumber: 7, + commitId: "abc123", + body: "zero findings", + comments: [], + fallbackBody: "fallback body", + }) + + // toStrictEqual: toEqual ignores undefined-valued keys, so it cannot + // tell `comments` absent apart from `comments: undefined` + expect(stub.createReviewCalls).toStrictEqual([ + { + owner: "aliasunder", + repo: "fixture", + pull_number: 7, + commit_id: "abc123", + event: "COMMENT", + body: "zero findings", + }, + ]) + }) + + it("falls back to a body-only review when inline comments are rejected with 422", async () => { + const stub = makeOctokitStub({ + createReviewResponses: [ + { error: makeStatusError(422) }, + { data: { html_url: reviewUrl } }, + ], + }) + const { client, logger } = makeClient(stub) + + const result = await client.submitReview({ + prNumber: 7, + commitId: "abc123", + body: "review body", + comments: [inlineComment], + fallbackBody: "fallback body", + }) + + // Whole-array assert: proves the first call carried the inline comments, + // the retry dropped them (strictly — no `comments: undefined` residue), + // and no third attempt followed + expect(stub.createReviewCalls).toStrictEqual([ + { + owner: "aliasunder", + repo: "fixture", + pull_number: 7, + commit_id: "abc123", + event: "COMMENT", + body: "review body", + comments: [inlineComment], + }, + { + owner: "aliasunder", + repo: "fixture", + pull_number: 7, + commit_id: "abc123", + event: "COMMENT", + body: "fallback body", + }, + ]) + expect(result).toEqual({ url: reviewUrl, usedFallbackBody: true }) + expect(logger.messages).toContainEqual({ + level: "warn", + message: "inline comments rejected (422) — posting body-only fallback", + data: { prNumber: 7, rejectedCommentCount: 1 }, + }) + }) + + it("rethrows a 422 on a zero-comment review without a second attempt", async () => { + const stub = makeOctokitStub({ + createReviewResponses: [{ error: makeStatusError(422) }], + }) + const { client } = makeClient(stub) + + await expect( + client.submitReview({ + prNumber: 7, + commitId: "abc123", + body: "zero findings", + comments: [], + fallbackBody: "fallback body", + }), + ).rejects.toThrow("HTTP 422") + expect(stub.createReviewCalls).toHaveLength(1) + }) + + it("rethrows a non-422 error without a second attempt", async () => { + const stub = makeOctokitStub({ + createReviewResponses: [{ error: makeStatusError(403) }], + }) + const { client } = makeClient(stub) + + await expect( + client.submitReview({ + prNumber: 7, + commitId: "abc123", + body: "review body", + comments: [inlineComment], + fallbackBody: "fallback body", + }), + ).rejects.toThrow("HTTP 403") + expect(stub.createReviewCalls).toHaveLength(1) + }) + + it("rethrows a 422 on the fallback attempt without a third attempt", async () => { + const stub = makeOctokitStub({ + createReviewResponses: [ + { error: makeStatusError(422) }, + { error: makeStatusError(422) }, + ], + }) + const { client } = makeClient(stub) + + await expect( + client.submitReview({ + prNumber: 7, + commitId: "abc123", + body: "review body", + comments: [inlineComment], + fallbackBody: "fallback body", + }), + ).rejects.toThrow("HTTP 422") + expect(stub.createReviewCalls).toHaveLength(2) + }) + + it("throws when the review response has no html_url", async () => { + const stub = makeOctokitStub({ + createReviewResponses: [{ data: { id: 1 } }], + }) + const { client } = makeClient(stub) + + await expect( + client.submitReview({ + prNumber: 7, + commitId: "abc123", + body: "review body", + comments: [inlineComment], + fallbackBody: "fallback body", + }), + ).rejects.toThrow("unexpected review response shape") + }) +}) diff --git a/src/github/client.ts b/src/github/client.ts new file mode 100644 index 0000000..b3d2e09 --- /dev/null +++ b/src/github/client.ts @@ -0,0 +1,193 @@ +import { z } from "zod" +import type { Logger } from "../logger.js" +import type { ReviewComment } from "../review/comment-mapping.js" +import type { PrContext } from "./event.js" + +/** + * Minimal structural type for injection — the real getOctokit() instance is + * structurally compatible (compiler-enforced by an expectTypeOf test in + * client.test.ts); test stubs are plain objects. `data: unknown` is + * deliberate: it forces Zod parsing and makes the diff media type (which + * returns a string, not JSON) legal without a cast. Method syntax keeps the + * real octokit assignable under strictFunctionTypes. + */ +export type OctokitLike = { + rest: { + pulls: { + get(params: { + owner: string + repo: string + pull_number: number + mediaType?: { format: string } + }): Promise<{ data: unknown }> + createReview(params: { + owner: string + repo: string + pull_number: number + commit_id: string + event: "COMMENT" + body: string + comments?: ReviewComment[] + }): Promise<{ data: unknown }> + } + } +} +export type DiffFetchResult = + { kind: "ok"; diff: string } | { kind: "too_large" } + +export type SubmitReviewResult = { url: string; usedFallbackBody: boolean } + +export type GithubClient = { + fetchPullRequest: (params: { prNumber: number }) => Promise + fetchDiff: (params: { prNumber: number }) => Promise + submitReview: (params: { + prNumber: number + commitId: string + body: string + comments: ReviewComment[] + /** Precomputed by the caller (pure code); posted body-only if GitHub + * rejects the inline anchors. */ + fallbackBody: string + }) => Promise +} + +/** The pullRequestEventSchema fields minus the event wrapper — what the REST + * pulls.get response must provide to build a PrContext. */ +const prResponseSchema = z.object({ + number: z.int().positive(), + title: z.string(), + body: z.string().nullable(), + head: z.object({ sha: z.string(), ref: z.string() }), + base: z.object({ ref: z.string() }), +}) + +const reviewUrlSchema = z.object({ html_url: z.string() }) + +/** Octokit request errors carry a numeric `status` — duck-typed so stubs and + * future octokit versions need no instanceof on octokit internals. */ +const errorStatus = (error: unknown): number | undefined => { + if (typeof error !== "object" || error === null) return undefined + if (!("status" in error) || typeof error.status !== "number") return undefined + return error.status +} + +export const createGithubClient = ( + { + octokit, + owner, + repo, + }: { octokit: OctokitLike; owner: string; repo: string }, + logger: Logger, +): GithubClient => { + const parseReviewUrl = (data: unknown): string => { + const parsed = reviewUrlSchema.safeParse(data) + if (!parsed.success) throw new Error("unexpected review response shape") + return parsed.data.html_url + } + + const fetchPullRequest = async ({ + prNumber, + }: { + prNumber: number + }): Promise => { + const response = await octokit.rest.pulls.get({ + owner, + repo, + pull_number: prNumber, + }) + const parsed = prResponseSchema.safeParse(response.data) + if (!parsed.success) { + throw new Error("unexpected pull request response shape") + } + const pullRequest = parsed.data + return { + prNumber: pullRequest.number, + title: pullRequest.title, + body: pullRequest.body, + headSha: pullRequest.head.sha, + headRef: pullRequest.head.ref, + baseRef: pullRequest.base.ref, + } + } + + const fetchDiff = async ({ + prNumber, + }: { + prNumber: number + }): Promise => { + try { + const response = await octokit.rest.pulls.get({ + owner, + repo, + pull_number: prNumber, + mediaType: { format: "diff" }, + }) + if (typeof response.data !== "string") { + throw new Error( + "expected a unified diff string from the diff media type", + ) + } + return { kind: "ok", diff: response.data } + } catch (fetchError) { + // GitHub answers 406 when the diff exceeds its API limits (>300 files or >20k lines) + if (errorStatus(fetchError) === 406) { + logger.warn("diff exceeds GitHub's diff API limits", { prNumber }) + return { kind: "too_large" } + } + throw fetchError + } + } + + const submitReview = async ({ + prNumber, + commitId, + body, + comments, + fallbackBody, + }: { + prNumber: number + commitId: string + body: string + comments: ReviewComment[] + fallbackBody: string + }): Promise => { + try { + const response = await octokit.rest.pulls.createReview({ + owner, + repo, + pull_number: prNumber, + commit_id: commitId, + event: "COMMENT", + body, + ...(comments.length > 0 ? { comments } : {}), + }) + return { url: parseReviewUrl(response.data), usedFallbackBody: false } + } catch (submitError) { + // GitHub answers 422 when an inline anchor is not part of the diff — + // e.g. a force-push racing the run. Only that case gets the body-only + // retry; a 422 on a zero-comment review is a different bug and rethrows. + const inlineAnchorsRejected = + comments.length > 0 && errorStatus(submitError) === 422 + if (!inlineAnchorsRejected) throw submitError + + logger.warn( + "inline comments rejected (422) — posting body-only fallback", + { + prNumber, + rejectedCommentCount: comments.length, + }, + ) + const response = await octokit.rest.pulls.createReview({ + owner, + repo, + pull_number: prNumber, + commit_id: commitId, + event: "COMMENT", + body: fallbackBody, + }) + return { url: parseReviewUrl(response.data), usedFallbackBody: true } + } + } + + return { fetchPullRequest, fetchDiff, submitReview } +} diff --git a/src/main.ts b/src/main.ts index c0b6e12..ca0a6e8 100644 --- a/src/main.ts +++ b/src/main.ts @@ -23,10 +23,15 @@ const collectRawInputs = (): RawInputs => ({ }) // Input collection and config validation are wired now so bad inputs fail -// loudly today; the V1 pipeline (orchestrate.ts + I/O clients) lands in the -// next PR and replaces the setFailed stub below. +// loudly today; orchestrate.ts lands in the next PR and replaces the +// setFailed stub below. try { - parseConfig(collectRawInputs()) + const config = parseConfig(collectRawInputs()) + // Register both credentials with the runner's masker before anything can + // log — our JSON logger writes raw to stdout, bypassing the masking the + // runner applies only to values passed through ::add-mask::. + core.setSecret(config.githubToken) + core.setSecret(config.openrouterApiKey) core.setFailed( "umm-actually: review pipeline not yet implemented (scaffold only)", ) diff --git a/src/openrouter/__tests__/client.test.ts b/src/openrouter/__tests__/client.test.ts new file mode 100644 index 0000000..531424f --- /dev/null +++ b/src/openrouter/__tests__/client.test.ts @@ -0,0 +1,478 @@ +import { readFileSync } from "node:fs" +import { describe, expect, it } from "vitest" +import { createTestLogger } from "../../__tests__/test-logger.js" +import { reviewResponseJsonSchema } from "../../review/finding.js" +import { + createOpenRouterClient, + type ChatRequestSubset, + type OpenRouterLike, +} from "../client.js" + +const readJsonFixture = (name: string): unknown => + JSON.parse( + readFileSync(new URL(`../../../fixtures/${name}`, import.meta.url), "utf8"), + ) + +const acceptedChatResult = readJsonFixture("openrouter.chat-result.json") +const invalidJsonChatResult = readJsonFixture("openrouter.invalid-json.json") +const schemaMismatchChatResult = readJsonFixture( + "openrouter.schema-mismatch.json", +) + +/** The review JSON embedded in the accepted chat-result fixture, parsed. */ +const acceptedReview = { + analysis: + "src/greeter.ts: verified the empty-key guard and traced register() callers.", + findings: [], +} + +/** Accepted-shape chat result whose usage carries no cost — the case that + * routes the client to the generation cost lookup. */ +const makeNoCostChatResult = (): unknown => ({ + id: "gen-no-cost", + model: "openai/gpt-5-mini", + choices: [{ message: { content: JSON.stringify(acceptedReview) } }], + usage: { promptTokens: 12000, completionTokens: 800, cost: null }, +}) + +type StubResponse = { value: unknown } | { error: unknown } + +const makeSdkStub = ({ + sendResponses, + generationResponses = [], +}: { + sendResponses: StubResponse[] + generationResponses?: StubResponse[] +}) => { + const sendCalls: { chatRequest: ChatRequestSubset }[] = [] + const generationCalls: { id: string }[] = [] + + const takeNext = ( + queue: StubResponse[], + callCount: number, + operation: string, + ): unknown => { + const next = queue[callCount - 1] + if (next === undefined) { + throw new Error(`stub: unexpected ${operation} call #${callCount}`) + } + if ("error" in next) throw next.error + return next.value + } + + const sdk: OpenRouterLike = { + chat: { + send: async (request) => { + sendCalls.push(request) + return takeNext(sendResponses, sendCalls.length, "chat.send") + }, + }, + generations: { + getGeneration: async (request) => { + generationCalls.push(request) + return takeNext( + generationResponses, + generationCalls.length, + "generations.getGeneration", + ) + }, + }, + } + + return { sdk, sendCalls, generationCalls } +} + +const makeClient = (stub: { sdk: OpenRouterLike }) => { + const logger = createTestLogger() + const client = createOpenRouterClient({ sdk: stub.sdk }, logger) + return { client, logger } +} + +const makeStatusError = (statusCode: number): Error => + Object.assign(new Error(`HTTP ${statusCode}`), { statusCode }) + +const requestParams = { + systemPrompt: "system prompt", + userPrompt: "user prompt", + model: "openai/gpt-5-mini", + fallbackModel: "anthropic/claude-haiku-4.5", +} + +describe("requestReview", () => { + it("sends the strict json_schema response format with the review schema", async () => { + const stub = makeSdkStub({ sendResponses: [{ value: acceptedChatResult }] }) + const { client } = makeClient(stub) + + await client.requestReview(requestParams) + + expect(stub.sendCalls).toEqual([ + { + chatRequest: { + model: "openai/gpt-5-mini", + messages: [ + { role: "system", content: "system prompt" }, + { role: "user", content: "user prompt" }, + ], + responseFormat: { + type: "json_schema", + jsonSchema: { + name: "review_response", + strict: true, + schema: reviewResponseJsonSchema, + }, + }, + stream: false, + }, + }, + ]) + }) + + it("returns the parsed review and a single accepted attempt on first success", async () => { + const stub = makeSdkStub({ sendResponses: [{ value: acceptedChatResult }] }) + const { client } = makeClient(stub) + + const result = await client.requestReview(requestParams) + + expect(result).toEqual({ + review: acceptedReview, + modelUsed: "openai/gpt-5-mini", + attempts: [ + { + model: "openai/gpt-5-mini", + outcome: "accepted", + promptTokens: 12000, + completionTokens: 800, + costUsd: 0.0421, + errorSummary: null, + }, + ], + }) + }) + + it("retries the same model once on invalid JSON, recording both attempts", async () => { + const stub = makeSdkStub({ + sendResponses: [ + { value: invalidJsonChatResult }, + { value: acceptedChatResult }, + ], + }) + const { client } = makeClient(stub) + + const result = await client.requestReview(requestParams) + + expect( + stub.sendCalls.map((sendCall) => sendCall.chatRequest.model), + ).toEqual(["openai/gpt-5-mini", "openai/gpt-5-mini"]) + expect(result.attempts).toEqual([ + { + model: "openai/gpt-5-mini", + outcome: "invalid_json", + promptTokens: 11800, + completionTokens: 42, + costUsd: null, + errorSummary: "response content is not valid JSON", + }, + { + model: "openai/gpt-5-mini", + outcome: "accepted", + promptTokens: 12000, + completionTokens: 800, + costUsd: 0.0421, + errorSummary: null, + }, + ]) + }) + + it("retries the same model once on empty content", async () => { + const emptyContentResult = { + id: "gen-empty", + model: "openai/gpt-5-mini", + choices: [{ message: { content: "" } }], + usage: { promptTokens: 10, completionTokens: 0, cost: null }, + } + const stub = makeSdkStub({ + sendResponses: [ + { value: emptyContentResult }, + { value: acceptedChatResult }, + ], + }) + const { client } = makeClient(stub) + + const result = await client.requestReview(requestParams) + + expect(result.attempts.map((attempt) => attempt.outcome)).toEqual([ + "empty_content", + "accepted", + ]) + }) + + it("advances to the fallback model after two schema mismatches", async () => { + const stub = makeSdkStub({ + sendResponses: [ + { value: schemaMismatchChatResult }, + { value: schemaMismatchChatResult }, + { value: acceptedChatResult }, + ], + }) + const { client } = makeClient(stub) + + const result = await client.requestReview(requestParams) + + expect( + stub.sendCalls.map((sendCall) => sendCall.chatRequest.model), + ).toEqual([ + "openai/gpt-5-mini", + "openai/gpt-5-mini", + "anthropic/claude-haiku-4.5", + ]) + expect(result.attempts.map((attempt) => attempt.outcome)).toEqual([ + "schema_mismatch", + "schema_mismatch", + "accepted", + ]) + }) + + it("retries the same model once on a 429", async () => { + const stub = makeSdkStub({ + sendResponses: [ + { error: makeStatusError(429) }, + { value: acceptedChatResult }, + ], + }) + const { client } = makeClient(stub) + + const result = await client.requestReview(requestParams) + + expect( + stub.sendCalls.map((sendCall) => sendCall.chatRequest.model), + ).toEqual(["openai/gpt-5-mini", "openai/gpt-5-mini"]) + expect(result.attempts[0]).toEqual({ + model: "openai/gpt-5-mini", + outcome: "api_error", + promptTokens: null, + completionTokens: null, + costUsd: null, + errorSummary: "HTTP 429: HTTP 429", + }) + }) + + it("retries a status-less network error and records its bare message", async () => { + const stub = makeSdkStub({ + sendResponses: [ + { error: new Error("socket hang up") }, + { value: acceptedChatResult }, + ], + }) + const { client } = makeClient(stub) + + const result = await client.requestReview(requestParams) + + expect( + stub.sendCalls.map((sendCall) => sendCall.chatRequest.model), + ).toEqual(["openai/gpt-5-mini", "openai/gpt-5-mini"]) + expect(result.attempts[0]).toEqual({ + model: "openai/gpt-5-mini", + outcome: "api_error", + promptTokens: null, + completionTokens: null, + costUsd: null, + errorSummary: "socket hang up", + }) + }) + + it("truncates an error message longer than 200 characters in the attempt summary", async () => { + const stub = makeSdkStub({ + sendResponses: [ + { + error: Object.assign(new Error("x".repeat(250)), { + statusCode: 429, + }), + }, + { value: acceptedChatResult }, + ], + }) + const { client } = makeClient(stub) + + const result = await client.requestReview(requestParams) + + expect(result.attempts[0]?.errorSummary).toBe( + `HTTP 429: ${"x".repeat(200)}…`, + ) + }) + + it("advances to the fallback model without a retry on a 404", async () => { + const stub = makeSdkStub({ + sendResponses: [ + { error: makeStatusError(404) }, + { value: acceptedChatResult }, + ], + }) + const { client } = makeClient(stub) + + const result = await client.requestReview(requestParams) + + expect( + stub.sendCalls.map((sendCall) => sendCall.chatRequest.model), + ).toEqual(["openai/gpt-5-mini", "anthropic/claude-haiku-4.5"]) + expect(result.attempts.map((attempt) => attempt.outcome)).toEqual([ + "api_error", + "accepted", + ]) + }) + + it.each([401, 402, 403])( + "aborts the ladder after exactly one call on a %d", + async (statusCode) => { + const stub = makeSdkStub({ + sendResponses: [{ error: makeStatusError(statusCode) }], + }) + const { client } = makeClient(stub) + + await expect(client.requestReview(requestParams)).rejects.toThrow( + `OpenRouter auth/credit error — aborting without fallback: openai/gpt-5-mini: api_error (HTTP ${statusCode}: HTTP ${statusCode})`, + ) + expect(stub.sendCalls).toHaveLength(1) + }, + ) + + it("throws a per-attempt summary after exhausting a fallback-less ladder", async () => { + const stub = makeSdkStub({ + sendResponses: [ + { error: makeStatusError(500) }, + { error: makeStatusError(503) }, + ], + }) + const { client } = makeClient(stub) + + await expect( + client.requestReview({ ...requestParams, fallbackModel: null }), + ).rejects.toThrow( + "review request failed after 2 attempt(s): openai/gpt-5-mini: api_error (HTTP 500: HTTP 500); openai/gpt-5-mini: api_error (HTTP 503: HTTP 503)", + ) + expect(stub.sendCalls).toHaveLength(2) + }) + + it("makes at most four calls across both ladder models", async () => { + const stub = makeSdkStub({ + sendResponses: [ + { value: invalidJsonChatResult }, + { value: invalidJsonChatResult }, + { value: invalidJsonChatResult }, + { value: invalidJsonChatResult }, + ], + }) + const { client } = makeClient(stub) + + await expect(client.requestReview(requestParams)).rejects.toThrow( + "review request failed after 4 attempt(s)", + ) + expect(stub.sendCalls).toHaveLength(4) + }) + + it("treats an unrecognized chat response shape as a retryable api_error", async () => { + const stub = makeSdkStub({ + sendResponses: [ + { value: { unexpected: true } }, + { value: acceptedChatResult }, + ], + }) + const { client } = makeClient(stub) + + const result = await client.requestReview(requestParams) + + expect(result.attempts[0]).toEqual({ + model: "openai/gpt-5-mini", + outcome: "api_error", + promptTokens: null, + completionTokens: null, + costUsd: null, + errorSummary: "unexpected chat response shape", + }) + }) + + it("looks up the generation cost when the accepted attempt has no usage cost", async () => { + const stub = makeSdkStub({ + sendResponses: [{ value: makeNoCostChatResult() }], + generationResponses: [{ value: { data: { totalCost: 0.0399 } } }], + }) + const { client } = makeClient(stub) + + const result = await client.requestReview(requestParams) + + expect(stub.generationCalls).toEqual([{ id: "gen-no-cost" }]) + expect(result.attempts[0]?.costUsd).toBe(0.0399) + }) + + it("degrades to a null cost with a warning when the generation lookup fails", async () => { + const stub = makeSdkStub({ + sendResponses: [{ value: makeNoCostChatResult() }], + generationResponses: [{ error: makeStatusError(500) }], + }) + const { client, logger } = makeClient(stub) + + const result = await client.requestReview(requestParams) + + expect(result.attempts[0]?.costUsd).toBeNull() + expect(logger.messages).toContainEqual({ + level: "warn", + message: "generation cost lookup failed", + data: { error: "HTTP 500" }, + }) + }) + + it("degrades to a null cost with a warning when the generation response has an unexpected shape", async () => { + const stub = makeSdkStub({ + sendResponses: [{ value: makeNoCostChatResult() }], + generationResponses: [{ value: { data: {} } }], + }) + const { client, logger } = makeClient(stub) + + const result = await client.requestReview(requestParams) + + expect(result.attempts[0]?.costUsd).toBeNull() + expect(logger.messages).toContainEqual({ + level: "warn", + message: "unexpected generation response shape", + data: {}, + }) + }) + + it("records null token counts when the response omits the usage block", async () => { + const noUsageResult = { + id: "gen-no-usage", + model: "openai/gpt-5-mini", + choices: [{ message: { content: JSON.stringify(acceptedReview) } }], + } + const stub = makeSdkStub({ + sendResponses: [{ value: noUsageResult }], + generationResponses: [{ value: { data: { totalCost: 0.0399 } } }], + }) + const { client } = makeClient(stub) + + const result = await client.requestReview(requestParams) + + // costUsd from the lookup proves the missing usage block routed the + // accepted attempt through the generation-cost fallback + expect(result.attempts).toEqual([ + { + model: "openai/gpt-5-mini", + outcome: "accepted", + promptTokens: null, + completionTokens: null, + costUsd: 0.0399, + errorSummary: null, + }, + ]) + }) + + it("skips the cost lookup entirely when the sdk has no generations surface", async () => { + const sdkWithoutGenerations: OpenRouterLike = { + chat: { send: async () => makeNoCostChatResult() }, + } + const { client } = makeClient({ sdk: sdkWithoutGenerations }) + + const result = await client.requestReview(requestParams) + + expect(result.attempts[0]?.costUsd).toBeNull() + }) +}) diff --git a/src/openrouter/__tests__/cost-summary.test.ts b/src/openrouter/__tests__/cost-summary.test.ts new file mode 100644 index 0000000..5667879 --- /dev/null +++ b/src/openrouter/__tests__/cost-summary.test.ts @@ -0,0 +1,146 @@ +import { describe, expect, it } from "vitest" +import type { ModelAttempt } from "../client.js" +import { renderCostSummary } from "../cost-summary.js" + +const acceptedAttempt: ModelAttempt = { + model: "openai/gpt-5-mini", + outcome: "accepted", + promptTokens: 12000, + completionTokens: 800, + costUsd: 0.0421, + errorSummary: null, +} + +describe("renderCostSummary", () => { + it("renders a well-formed empty table for zero attempts", () => { + const summary = renderCostSummary({ + attempts: [], + modelUsed: "openai/gpt-5-mini", + }) + + expect(summary).toBe( + [ + "### umm-actually cost summary", + "", + "Model used: openai/gpt-5-mini", + "", + "| attempt | model | outcome | prompt tokens | completion tokens | cost |", + "| --- | --- | --- | --- | --- | --- |", + "", + "Total cost: n/a", + ].join("\n"), + ) + }) + + it("renders a single accepted attempt with its total", () => { + const summary = renderCostSummary({ + attempts: [acceptedAttempt], + modelUsed: "openai/gpt-5-mini", + }) + + expect(summary).toBe( + [ + "### umm-actually cost summary", + "", + "Model used: openai/gpt-5-mini", + "", + "| attempt | model | outcome | prompt tokens | completion tokens | cost |", + "| --- | --- | --- | --- | --- | --- |", + "| 1 | openai/gpt-5-mini | accepted | 12000 | 800 | $0.042100 |", + "", + "Total cost: $0.042100", + ].join("\n"), + ) + }) + + it("renders n/a cells and flags a partially priced total", () => { + const failedAttempt: ModelAttempt = { + model: "openai/gpt-5-mini", + outcome: "api_error", + promptTokens: null, + completionTokens: null, + costUsd: null, + errorSummary: "HTTP 500: boom", + } + + const summary = renderCostSummary({ + attempts: [failedAttempt, acceptedAttempt], + modelUsed: "openai/gpt-5-mini", + }) + + expect(summary).toBe( + [ + "### umm-actually cost summary", + "", + "Model used: openai/gpt-5-mini", + "", + "| attempt | model | outcome | prompt tokens | completion tokens | cost |", + "| --- | --- | --- | --- | --- | --- |", + "| 1 | openai/gpt-5-mini | api_error | n/a | n/a | n/a |", + "| 2 | openai/gpt-5-mini | accepted | 12000 | 800 | $0.042100 |", + "", + "Total cost: $0.042100 (some attempts unpriced)", + ].join("\n"), + ) + }) + + it("sums costs across attempts when every attempt is priced", () => { + const fallbackAttempt: ModelAttempt = { + model: "anthropic/claude-haiku-4.5", + outcome: "accepted", + promptTokens: 11000, + completionTokens: 600, + costUsd: 0.01, + errorSummary: null, + } + + const summary = renderCostSummary({ + attempts: [ + { ...acceptedAttempt, outcome: "schema_mismatch" }, + fallbackAttempt, + ], + modelUsed: "anthropic/claude-haiku-4.5", + }) + + expect(summary).toBe( + [ + "### umm-actually cost summary", + "", + "Model used: anthropic/claude-haiku-4.5", + "", + "| attempt | model | outcome | prompt tokens | completion tokens | cost |", + "| --- | --- | --- | --- | --- | --- |", + "| 1 | openai/gpt-5-mini | schema_mismatch | 12000 | 800 | $0.042100 |", + "| 2 | anthropic/claude-haiku-4.5 | accepted | 11000 | 600 | $0.010000 |", + "", + "Total cost: $0.052100", + ].join("\n"), + ) + }) + + it("renders an n/a total when no attempt carries a cost", () => { + const unpricedAttempt: ModelAttempt = { + ...acceptedAttempt, + costUsd: null, + } + + const summary = renderCostSummary({ + attempts: [unpricedAttempt], + modelUsed: "openai/gpt-5-mini", + }) + + expect(summary).toBe( + [ + "### umm-actually cost summary", + "", + "Model used: openai/gpt-5-mini", + "", + "| attempt | model | outcome | prompt tokens | completion tokens | cost |", + "| --- | --- | --- | --- | --- | --- |", + "| 1 | openai/gpt-5-mini | accepted | 12000 | 800 | n/a |", + "", + "Total cost: n/a", + ].join("\n"), + ) + }) +}) diff --git a/src/openrouter/client.ts b/src/openrouter/client.ts new file mode 100644 index 0000000..030732e --- /dev/null +++ b/src/openrouter/client.ts @@ -0,0 +1,380 @@ +import { z } from "zod" +import type { Logger } from "../logger.js" +import { + reviewResponseJsonSchema, + reviewResponseSchema, + type ReviewResponse, +} from "../review/finding.js" + +export type AttemptOutcome = + | "accepted" + | "api_error" + | "empty_content" + | "invalid_json" + | "schema_mismatch" + +export type ModelAttempt = { + /** The model slug requested for this attempt (ladder position, not routing). */ + model: string + outcome: AttemptOutcome + promptTokens: number | null + completionTokens: number | null + costUsd: number | null + /** Short human summary — never the raw response body. */ + errorSummary: string | null +} + +export type StructuredReviewResult = { + review: ReviewResponse + /** The model the accepted response reports — OpenRouter's routed slug. */ + modelUsed: string + /** Every attempt, failed ones included — they were billed too. */ + attempts: ModelAttempt[] +} + +/** Structural subset of the SDK's ChatRequest — exactly what we send. */ +export type ChatRequestSubset = { + model: string + messages: { role: "system" | "user"; content: string }[] + responseFormat: { + type: "json_schema" + jsonSchema: { + name: string + strict: boolean + schema: Record + } + } + stream: false +} + +/** + * Structural subset of the OpenRouter SDK — test stubs are plain objects. + * Method syntax keeps the real SDK assignable under strictFunctionTypes; + * responses are `unknown` on purpose: Zod parses across the boundary. + */ +export type OpenRouterLike = { + chat: { + send(request: { chatRequest: ChatRequestSubset }): Promise + } + generations?: { + getGeneration(request: { id: string }): Promise + } +} + +export type OpenRouterClient = { + requestReview: (params: { + systemPrompt: string + userPrompt: string + model: string + fallbackModel: string | null + }) => Promise +} + +const chatResultSchema = z.object({ + id: z.string(), + model: z.string(), + choices: z.array(z.object({ message: z.object({ content: z.unknown() }) })), + usage: z + .object({ + promptTokens: z.number(), + completionTokens: z.number(), + cost: z.number().nullable().optional(), + }) + .optional(), +}) + +const generationResponseSchema = z.object({ + data: z.object({ totalCost: z.number() }), +}) + +/** Auth/credit failures abort the ladder — the fallback model shares the key. */ +const ABORT_STATUSES = new Set([401, 402, 403]) + +/** Transient by nature: timeout, rate limit. 5xx and status-less network + * errors are retryable too; any other 4xx is structural and skips the retry. */ +const RETRYABLE_STATUSES = new Set([408, 429]) + +const MAX_ATTEMPTS_PER_MODEL = 2 + +/** OpenRouter SDK errors carry a numeric `statusCode` — duck-typed so stubs + * and future SDK versions need no instanceof on SDK internals. */ +const errorStatusCode = (error: unknown): number | undefined => { + if (typeof error !== "object" || error === null) return undefined + if (!("statusCode" in error) || typeof error.statusCode !== "number") { + return undefined + } + return error.statusCode +} + +const summarizeError = (error: unknown): string => { + const message = error instanceof Error ? error.message : String(error) + return message.length > 200 ? `${message.slice(0, 200)}…` : message +} + +/** Converts a throwing promise into a discriminated result — avoids + * try/catch nesting at every SDK call site. */ +const toResult = async ( + promise: Promise, +): Promise<{ ok: true; value: T } | { ok: false; error: unknown }> => { + try { + const value = await promise + return { ok: true, value } + } catch (error) { + return { ok: false, error } + } +} + +/** Wrapped in `{ parsed }` because JSON `null` is a valid parse result. */ +const parseJsonOrNull = (text: string): { parsed: unknown } | null => { + try { + const parsed: unknown = JSON.parse(text) + return { parsed } + } catch { + return null + } +} + +const buildChatRequest = ({ + systemPrompt, + userPrompt, + model, +}: { + systemPrompt: string + userPrompt: string + model: string +}): ChatRequestSubset => ({ + model, + messages: [ + { role: "system", content: systemPrompt }, + { role: "user", content: userPrompt }, + ], + responseFormat: { + type: "json_schema", + jsonSchema: { + name: "review_response", + strict: true, + schema: reviewResponseJsonSchema, + }, + }, + stream: false, +}) + +type SingleAttempt = + | { + kind: "accepted" + review: ReviewResponse + routedModel: string + generationId: string + attempt: ModelAttempt + } + | { + kind: "failed" + attempt: ModelAttempt + retryable: boolean + abort: boolean + } + +const describeAttempt = (attempt: ModelAttempt): string => { + const errorSuffix = + attempt.errorSummary === null ? "" : ` (${attempt.errorSummary})` + return `${attempt.model}: ${attempt.outcome}${errorSuffix}` +} + +const summarizeAttempts = (attempts: ModelAttempt[]): string => + attempts.map(describeAttempt).join("; ") + +export const createOpenRouterClient = ( + { sdk }: { sdk: OpenRouterLike }, + logger: Logger, +): OpenRouterClient => { + const attemptOnce = async ({ + chatRequest, + model, + }: { + chatRequest: ChatRequestSubset + model: string + }): Promise => { + const sendResult = await toResult(sdk.chat.send({ chatRequest })) + if (!sendResult.ok) { + const statusCode = errorStatusCode(sendResult.error) + const abort = statusCode !== undefined && ABORT_STATUSES.has(statusCode) + const retryable = + statusCode === undefined || + statusCode >= 500 || + RETRYABLE_STATUSES.has(statusCode) + return { + kind: "failed", + attempt: { + model, + outcome: "api_error", + promptTokens: null, + completionTokens: null, + costUsd: null, + errorSummary: + statusCode === undefined + ? summarizeError(sendResult.error) + : `HTTP ${statusCode}: ${summarizeError(sendResult.error)}`, + }, + retryable: abort ? false : retryable, + abort, + } + } + + const parsedResult = chatResultSchema.safeParse(sendResult.value) + if (!parsedResult.success) { + return { + kind: "failed", + attempt: { + model, + outcome: "api_error", + promptTokens: null, + completionTokens: null, + costUsd: null, + errorSummary: "unexpected chat response shape", + }, + retryable: true, + abort: false, + } + } + + const chatResult = parsedResult.data + // Failed-validation attempts still carry usage — they were billed too + const usage = { + promptTokens: chatResult.usage?.promptTokens ?? null, + completionTokens: chatResult.usage?.completionTokens ?? null, + costUsd: chatResult.usage?.cost ?? null, + } + const failedValidation = ( + outcome: AttemptOutcome, + errorSummary: string, + ): SingleAttempt => ({ + kind: "failed", + attempt: { model, outcome, ...usage, errorSummary }, + retryable: true, + abort: false, + }) + + const content = chatResult.choices[0]?.message.content + if (typeof content !== "string" || content === "") { + return failedValidation("empty_content", "response had no text content") + } + + const parsedContent = parseJsonOrNull(content) + if (parsedContent === null) { + return failedValidation( + "invalid_json", + "response content is not valid JSON", + ) + } + + const parsedReview = reviewResponseSchema.safeParse(parsedContent.parsed) + if (!parsedReview.success) { + const firstIssue = parsedReview.error.issues[0] + return failedValidation( + "schema_mismatch", + firstIssue === undefined + ? "response JSON does not match the review schema" + : `schema mismatch at ${firstIssue.path.join(".")}: ${firstIssue.message}`, + ) + } + + return { + kind: "accepted", + review: parsedReview.data, + routedModel: chatResult.model, + generationId: chatResult.id, + attempt: { model, outcome: "accepted", ...usage, errorSummary: null }, + } + } + + /** Best-effort: a cost lookup failure must never fail a completed review. */ + const lookupGenerationCost = async ( + generationId: string, + ): Promise => { + if (sdk.generations === undefined) return null + const lookup = await toResult( + sdk.generations.getGeneration({ id: generationId }), + ) + if (!lookup.ok) { + logger.warn("generation cost lookup failed", { + error: summarizeError(lookup.error), + }) + return null + } + const parsed = generationResponseSchema.safeParse(lookup.value) + if (!parsed.success) { + logger.warn("unexpected generation response shape") + return null + } + return parsed.data.data.totalCost + } + + const requestReview = async ({ + systemPrompt, + userPrompt, + model, + fallbackModel, + }: { + systemPrompt: string + userPrompt: string + model: string + fallbackModel: string | null + }): Promise => { + const modelLadder = + fallbackModel === null ? [model] : [model, fallbackModel] + const attempts: ModelAttempt[] = [] + + for (const ladderModel of modelLadder) { + const chatRequest = buildChatRequest({ + systemPrompt, + userPrompt, + model: ladderModel, + }) + + let attemptNumber = 1 + while (attemptNumber <= MAX_ATTEMPTS_PER_MODEL) { + const attemptResult = await attemptOnce({ + chatRequest, + model: ladderModel, + }) + + if (attemptResult.kind === "accepted") { + const costUsd = + attemptResult.attempt.costUsd ?? + (await lookupGenerationCost(attemptResult.generationId)) + attempts.push({ ...attemptResult.attempt, costUsd }) + logger.info("review response accepted", { + model: ladderModel, + attemptCount: attempts.length, + }) + return { + review: attemptResult.review, + modelUsed: attemptResult.routedModel, + attempts, + } + } + + attempts.push(attemptResult.attempt) + logger.warn("review attempt failed", { + model: ladderModel, + attemptNumber, + outcome: attemptResult.attempt.outcome, + errorSummary: attemptResult.attempt.errorSummary, + }) + if (attemptResult.abort) { + throw new Error( + `OpenRouter auth/credit error — aborting without fallback: ${summarizeAttempts(attempts)}`, + ) + } + if (!attemptResult.retryable) break + attemptNumber++ + } + } + + throw new Error( + `review request failed after ${attempts.length} attempt(s): ${summarizeAttempts(attempts)}`, + ) + } + + return { requestReview } +} diff --git a/src/openrouter/cost-summary.ts b/src/openrouter/cost-summary.ts new file mode 100644 index 0000000..f80dcc5 --- /dev/null +++ b/src/openrouter/cost-summary.ts @@ -0,0 +1,45 @@ +import type { ModelAttempt } from "./client.js" + +const formatCost = (costUsd: number | null): string => + costUsd === null ? "n/a" : `$${costUsd.toFixed(6)}` + +const formatTokens = (tokens: number | null): string => + tokens === null ? "n/a" : String(tokens) + +/** Markdown table for the workflow job summary — one row per attempt, + * failed ones included (any attempt that reached the provider was billed). */ +export const renderCostSummary = ({ + attempts, + modelUsed, +}: { + attempts: ModelAttempt[] + modelUsed: string +}): string => { + const rows = attempts.map( + (attempt, attemptIndex) => + `| ${attemptIndex + 1} | ${attempt.model} | ${attempt.outcome} | ${formatTokens(attempt.promptTokens)} | ${formatTokens(attempt.completionTokens)} | ${formatCost(attempt.costUsd)} |`, + ) + + const knownCosts = attempts.flatMap((attempt) => + attempt.costUsd === null ? [] : [attempt.costUsd], + ) + const totalCost = knownCosts.reduce((sum, costUsd) => sum + costUsd, 0) + const unpricedSuffix = + knownCosts.length < attempts.length ? " (some attempts unpriced)" : "" + const totalLine = + knownCosts.length === 0 + ? "Total cost: n/a" + : `Total cost: ${formatCost(totalCost)}${unpricedSuffix}` + + return [ + "### umm-actually cost summary", + "", + `Model used: ${modelUsed}`, + "", + "| attempt | model | outcome | prompt tokens | completion tokens | cost |", + "| --- | --- | --- | --- | --- | --- |", + ...rows, + "", + totalLine, + ].join("\n") +} diff --git a/src/review/prompt.ts b/src/review/prompt.ts index bcc20d3..76da20a 100644 --- a/src/review/prompt.ts +++ b/src/review/prompt.ts @@ -12,7 +12,7 @@ export type PromptFile = { } /** ~4 chars per token — the standard rough heuristic; we only need order-of-magnitude. */ -const CHARS_PER_TOKEN = 4 +export const CHARS_PER_TOKEN = 4 const CONVENTIONS_TOKEN_CAP = 8_000 const IDENTITY_AND_SCOPE = `You are umm-actually, a code review bot. You review the changes in a pull