diff --git a/packages/cli/package.json b/packages/cli/package.json index 3bb02ce..47f9a30 100644 --- a/packages/cli/package.json +++ b/packages/cli/package.json @@ -50,6 +50,7 @@ "commander": "^14.0.3", "drizzle-orm": "^0.45.2", "open": "^11.0.0", + "parse-diff": "^0.11.1", "zod": "^4.3.6" }, "devDependencies": { diff --git a/packages/cli/src/__tests__/build-other-changes.test.ts b/packages/cli/src/__tests__/build-other-changes.test.ts new file mode 100644 index 0000000..941a332 --- /dev/null +++ b/packages/cli/src/__tests__/build-other-changes.test.ts @@ -0,0 +1,99 @@ +import type { PullRequestFile } from "@stagereview/types/parsed-diff"; +import { describe, expect, it } from "vitest"; +import { buildOtherChangesChapter } from "../build-other-changes.js"; + +function createFile(overrides?: Partial): PullRequestFile { + return { + path: "src/app.ts", + filename: "app.ts", + status: "modified", + additions: 1, + deletions: 0, + hunks: [ + { + header: "@@ -1,1 +1,2 @@", + oldStart: 1, + newStart: 1, + oldLines: 1, + newLines: 2, + lines: [{ type: "addition", content: "x", newLineNumber: 2 }], + }, + ], + patch: "", + ...overrides, + }; +} + +describe("buildOtherChangesChapter", () => { + it("returns null when nothing was excluded", () => { + const result = buildOtherChangesChapter([createFile()], []); + expect(result).toBeNull(); + }); + + it("builds hunkRefs for an excluded file with hunks", () => { + const lockfile = createFile({ + path: "pnpm-lock.yaml", + hunks: [ + { + header: "@@ -1,1 +1,2 @@", + oldStart: 1, + newStart: 1, + oldLines: 1, + newLines: 2, + lines: [], + }, + { + header: "@@ -10,1 +11,2 @@", + oldStart: 10, + newStart: 11, + oldLines: 1, + newLines: 2, + lines: [], + }, + ], + }); + + const result = buildOtherChangesChapter([lockfile], ["pnpm-lock.yaml"]); + + expect(result).not.toBeNull(); + expect(result?.id).toBe("chapter-other-changes"); + expect(result?.title).toBe("Other changes"); + expect(result?.keyChanges).toEqual([]); + expect(result?.hunkRefs).toEqual([ + { filePath: "pnpm-lock.yaml", oldStart: 1 }, + { filePath: "pnpm-lock.yaml", oldStart: 10 }, + ]); + }); + + it("emits no hunkRefs for binary-only excluded files", () => { + const binary = createFile({ path: "public/logo.png", hunks: [] }); + + const result = buildOtherChangesChapter([binary], ["public/logo.png"]); + + expect(result?.hunkRefs).toEqual([]); + }); + + it("ignores files not in excludedByPath", () => { + const code = createFile({ path: "src/app.ts" }); + const lockfile = createFile({ path: "pnpm-lock.yaml" }); + + const result = buildOtherChangesChapter([code, lockfile], ["pnpm-lock.yaml"]); + + expect(result?.hunkRefs.every((ref) => ref.filePath === "pnpm-lock.yaml")).toBe(true); + }); + + it("preserves PR file order in hunkRefs, not excludedByPath order", () => { + const lockfile = createFile({ path: "pnpm-lock.yaml" }); + const image = createFile({ path: "public/logo.png" }); + + const result = buildOtherChangesChapter( + [lockfile, image], + ["public/logo.png", "pnpm-lock.yaml"], + ); + + expect(result?.hunkRefs.map((ref) => ref.filePath)).toEqual([ + "pnpm-lock.yaml", + "public/logo.png", + ]); + }); +}); diff --git a/packages/cli/src/__tests__/diff-parser.test.ts b/packages/cli/src/__tests__/diff-parser.test.ts new file mode 100644 index 0000000..fba40be --- /dev/null +++ b/packages/cli/src/__tests__/diff-parser.test.ts @@ -0,0 +1,308 @@ +import { describe, expect, it } from "vitest"; +import { calculateDiffStats, parseGitDiff } from "../diff-parser.js"; + +const ADDED_FILE_DIFF = `diff --git a/src/utils.ts b/src/utils.ts +new file mode 100644 +index 0000000..abcdef1 +--- /dev/null ++++ b/src/utils.ts +@@ -0,0 +1,3 @@ ++export function hello() { ++ return "world"; ++}`; + +const DELETED_FILE_DIFF = `diff --git a/src/old.ts b/src/old.ts +deleted file mode 100644 +index abcdef1..0000000 +--- a/src/old.ts ++++ /dev/null +@@ -1,3 +0,0 @@ +-export function goodbye() { +- return "world"; +-}`; + +const MOVED_FILE_DIFF = `diff --git a/src/old-name.ts b/src/new-name.ts +similarity index 100% +rename from src/old-name.ts +rename to src/new-name.ts`; + +const RENAMED_FILE_DIFF = `diff --git a/src/old-name.ts b/src/new-name.ts +similarity index 85% +rename from src/old-name.ts +rename to src/new-name.ts +index abc123..def456 100644 +--- a/src/old-name.ts ++++ b/src/new-name.ts +@@ -1,3 +1,4 @@ + export function hello() { +- return "world"; ++ return "universe"; ++ // updated + }`; + +const MODIFIED_FILE_DIFF = `diff --git a/src/app.ts b/src/app.ts +index abc123..def456 100644 +--- a/src/app.ts ++++ b/src/app.ts +@@ -1,5 +1,6 @@ + const a = 1; +-const b = 2; ++const b = 3; ++const c = 4; + const d = 5;`; + +const MULTI_FILE_DIFF = `${ADDED_FILE_DIFF} +${MODIFIED_FILE_DIFF}`; + +describe("parseGitDiff", () => { + it("returns empty array for empty string", () => { + expect(parseGitDiff("")).toEqual([]); + }); + + it("returns empty array for whitespace-only string", () => { + expect(parseGitDiff(" \n\t ")).toEqual([]); + }); + + it("parses an added file", () => { + const result = parseGitDiff(ADDED_FILE_DIFF); + expect(result).toHaveLength(1); + expect(result[0]?.status).toBe("added"); + expect(result[0]?.path).toBe("src/utils.ts"); + expect(result[0]?.filename).toBe("utils.ts"); + expect(result[0]?.additions).toBe(3); + expect(result[0]?.deletions).toBe(0); + expect(result[0]?.hunks).toHaveLength(1); + }); + + it("parses a deleted file with correct path from 'from' field", () => { + const result = parseGitDiff(DELETED_FILE_DIFF); + expect(result).toHaveLength(1); + expect(result[0]?.status).toBe("deleted"); + expect(result[0]?.path).toBe("src/old.ts"); + expect(result[0]?.filename).toBe("old.ts"); + expect(result[0]?.deletions).toBe(3); + }); + + it("parses a moved file (path changed, no content changes)", () => { + const result = parseGitDiff(MOVED_FILE_DIFF); + expect(result).toHaveLength(1); + expect(result[0]?.status).toBe("moved"); + expect(result[0]?.path).toBe("src/new-name.ts"); + expect(result[0]?.oldPath).toBe("src/old-name.ts"); + }); + + it("parses a renamed file (path changed with content changes)", () => { + const result = parseGitDiff(RENAMED_FILE_DIFF); + expect(result).toHaveLength(1); + expect(result[0]?.status).toBe("renamed"); + expect(result[0]?.path).toBe("src/new-name.ts"); + expect(result[0]?.oldPath).toBe("src/old-name.ts"); + expect(result[0]?.additions).toBeGreaterThan(0); + }); + + it("does not set oldPath for non-renamed files", () => { + const result = parseGitDiff(MODIFIED_FILE_DIFF); + expect(result[0]?.oldPath).toBeUndefined(); + }); + + it("parses a modified file with mixed additions and deletions", () => { + const result = parseGitDiff(MODIFIED_FILE_DIFF); + expect(result).toHaveLength(1); + expect(result[0]?.status).toBe("modified"); + expect(result[0]?.additions).toBe(2); + expect(result[0]?.deletions).toBe(1); + }); + + it("parses multiple files from a single diff", () => { + const result = parseGitDiff(MULTI_FILE_DIFF); + expect(result).toHaveLength(2); + expect(result[0]?.path).toBe("src/utils.ts"); + expect(result[1]?.path).toBe("src/app.ts"); + }); + + it("strips a/ and b/ prefixes from paths", () => { + const result = parseGitDiff(MODIFIED_FILE_DIFF); + expect(result[0]?.path).toBe("src/app.ts"); + expect(result[0]?.path).not.toMatch(/^[ab]\//); + }); + + it("transforms hunk lines with correct types and content", () => { + const result = parseGitDiff(MODIFIED_FILE_DIFF); + const lines = result[0]?.hunks[0]?.lines; + + const contextLine = lines?.find((l) => l.type === "context"); + expect(contextLine).toBeDefined(); + expect(contextLine?.content).toBe("const a = 1;"); + + const additionLine = lines?.find((l) => l.type === "addition" && l.content === "const b = 3;"); + expect(additionLine).toBeDefined(); + expect(additionLine?.newLineNumber).toBeDefined(); + expect(additionLine?.oldLineNumber).toBeUndefined(); + + const deletionLine = lines?.find((l) => l.type === "deletion"); + expect(deletionLine).toBeDefined(); + expect(deletionLine?.content).toBe("const b = 2;"); + expect(deletionLine?.oldLineNumber).toBeDefined(); + expect(deletionLine?.newLineNumber).toBeUndefined(); + }); + + it("preserves hunk coordinates from @@ headers", () => { + const twoHunkDiff = `diff --git a/src/app.ts b/src/app.ts +index abc123..def456 100644 +--- a/src/app.ts ++++ b/src/app.ts +@@ -1,3 +1,3 @@ + const a = 1; +-const b = 2; ++const b = 3; + const c = 4; +@@ -100,3 +100,3 @@ + const x = 1; +-const y = 2; ++const y = 3; + const z = 4;`; + + const result = parseGitDiff(twoHunkDiff); + expect(result[0]?.hunks).toHaveLength(2); + expect(result[0]?.hunks[0]?.oldStart).toBe(1); + expect(result[0]?.hunks[1]?.oldStart).toBe(100); + }); + + it("strips +/- prefix from line content", () => { + const result = parseGitDiff(ADDED_FILE_DIFF); + const firstLine = result[0]?.hunks[0]?.lines[0]; + expect(firstLine?.content).toBe("export function hello() {"); + expect(firstLine?.content).not.toMatch(/^\+/); + }); +}); + +describe("parseGitDiff — embedded diff headers", () => { + it("does not split on 'diff --git' inside file content", () => { + const diff = `diff --git a/test-file.ts b/test-file.ts +index abc123..def456 100644 +--- a/test-file.ts ++++ b/test-file.ts +@@ -1,3 +1,5 @@ + const PATCH_HEADER = [ ++ "diff --git a/src/reviews.ts b/src/reviews.ts", ++ "index abc1234..def5678 100644", + ].join("\\n");`; + + const result = parseGitDiff(diff); + expect(result).toHaveLength(1); + expect(result[0]?.path).toBe("test-file.ts"); + expect(result[0]?.additions).toBe(2); + }); + + it("handles diff --git inside content followed by another real file", () => { + const diff = `diff --git a/test-file.ts b/test-file.ts +index abc123..def456 100644 +--- a/test-file.ts ++++ b/test-file.ts +@@ -1,3 +1,5 @@ + const header = [ ++ "diff --git a/inner.ts b/inner.ts", ++ "--- a/inner.ts", + ].join("\\n"); +diff --git a/real-file.ts b/real-file.ts +index 111222..333444 100644 +--- a/real-file.ts ++++ b/real-file.ts +@@ -1,2 +1,2 @@ +-const x = 1; ++const x = 2;`; + + const result = parseGitDiff(diff); + expect(result).toHaveLength(2); + expect(result[0]?.path).toBe("test-file.ts"); + expect(result[1]?.path).toBe("real-file.ts"); + }); +}); + +describe("symlink detection", () => { + it("detects a new symlink and extracts the target", () => { + const diff = `diff --git a/link b/link +new file mode 120000 +index 0000000..abc1234 +--- /dev/null ++++ b/link +@@ -0,0 +1 @@ ++../../path/to/target +\\ No newline at end of file`; + + const [file] = parseGitDiff(diff); + expect(file?.isSymlink).toBe(true); + expect(file?.symlinkTarget).toBe("../../path/to/target"); + expect(file?.oldSymlinkTarget).toBeUndefined(); + }); + + it("detects a deleted symlink and extracts the old target", () => { + const diff = `diff --git a/link b/link +deleted file mode 120000 +index abc1234..0000000 +--- a/link ++++ /dev/null +@@ -1 +0,0 @@ +-../../path/to/target +\\ No newline at end of file`; + + const [file] = parseGitDiff(diff); + expect(file?.isSymlink).toBe(true); + expect(file?.symlinkTarget).toBeUndefined(); + expect(file?.oldSymlinkTarget).toBe("../../path/to/target"); + }); + + it("detects a modified symlink and extracts both targets", () => { + const diff = `diff --git a/link b/link +index abc1234..def5678 120000 +--- a/link ++++ b/link +@@ -1 +1 @@ +-old/target ++new/target`; + + const [file] = parseGitDiff(diff); + expect(file?.isSymlink).toBe(true); + expect(file?.symlinkTarget).toBe("new/target"); + expect(file?.oldSymlinkTarget).toBe("old/target"); + expect(file?.status).toBe("modified"); + }); + + it("does not flag regular files as symlinks", () => { + const files = parseGitDiff(ADDED_FILE_DIFF); + expect(files[0]?.isSymlink).toBeUndefined(); + expect(files[0]?.symlinkTarget).toBeUndefined(); + }); + + it("does not false-positive on paths containing 120000", () => { + const diff = `diff --git a/legacy/120000-config.ts b/legacy/120000-config.ts +new file mode 100644 +index 0000000..abc1234 +--- /dev/null ++++ b/legacy/120000-config.ts +@@ -0,0 +1 @@ ++export const config = {};`; + + const [file] = parseGitDiff(diff); + expect(file?.isSymlink).toBeUndefined(); + }); +}); + +describe("calculateDiffStats", () => { + it("returns zeros for empty array", () => { + expect(calculateDiffStats([])).toEqual({ + totalAdditions: 0, + totalDeletions: 0, + fileCount: 0, + }); + }); + + it("sums additions and deletions across files", () => { + const files = parseGitDiff(MULTI_FILE_DIFF); + const stats = calculateDiffStats(files); + + expect(stats.totalAdditions).toBe(5); + expect(stats.totalDeletions).toBe(1); + expect(stats.fileCount).toBe(2); + }); +}); diff --git a/packages/cli/src/__tests__/filter-files.test.ts b/packages/cli/src/__tests__/filter-files.test.ts new file mode 100644 index 0000000..b5ef1a9 --- /dev/null +++ b/packages/cli/src/__tests__/filter-files.test.ts @@ -0,0 +1,137 @@ +import type { Hunk, PullRequestFile } from "@stagereview/types/parsed-diff"; +import { LINE_TYPE } from "@stagereview/types/parsed-diff"; +import { describe, expect, it } from "vitest"; +import { filterFilesForLlm, shouldIncludeFile } from "../filter-files.js"; + +function makeHunk(lineCount: number, overrides?: Partial): Hunk { + return { + header: `@@ -1,${lineCount} +1,${lineCount} @@`, + oldStart: 1, + newStart: 1, + oldLines: lineCount, + newLines: lineCount, + lines: Array.from({ length: lineCount }, (_, i) => ({ + type: LINE_TYPE.ADDITION, + content: `line ${i}`, + newLineNumber: i + 1, + })), + ...overrides, + }; +} + +function makeFile(overrides?: Partial): PullRequestFile { + return { + path: "src/app.ts", + filename: "app.ts", + status: "modified", + additions: 1, + deletions: 0, + hunks: [makeHunk(5)], + patch: "diff --git ...", + ...overrides, + }; +} + +describe("shouldIncludeFile", () => { + const denylistedFilenames = [ + "package-lock.json", + "yarn.lock", + "pnpm-lock.yaml", + "bun.lockb", + "bun.lock", + "composer.lock", + "Gemfile.lock", + "Cargo.lock", + "poetry.lock", + "Pipfile.lock", + "go.sum", + "flake.lock", + ".DS_Store", + "Thumbs.db", + ]; + + it.each(denylistedFilenames)("excludes lockfile/metadata basename %s", (name) => { + expect(shouldIncludeFile(name)).toBe(false); + }); + + it.each(denylistedFilenames)("excludes %s when nested under a directory", (name) => { + expect(shouldIncludeFile(`packages/web/${name}`)).toBe(false); + }); + + const denylistedExtensions = [ + "bundle.min.js", + "styles.min.css", + "bundle.map", + "Component.snap", + "logo.svg", + "icon.png", + "photo.jpg", + "photo.jpeg", + "sprite.gif", + "favicon.ico", + "font.woff", + "font.woff2", + "font.ttf", + "font.eot", + "video.mp4", + "video.webm", + "doc.pdf", + ]; + + it.each(denylistedExtensions)("excludes binary/generated extension %s", (name) => { + expect(shouldIncludeFile(`assets/${name}`)).toBe(false); + }); + + const normalFiles = [ + "src/index.ts", + "src/app.tsx", + "server/main.py", + "README.md", + "scripts/build.sh", + "docker-compose.yaml", + ]; + + it.each(normalFiles)("includes normal source file %s", (name) => { + expect(shouldIncludeFile(name)).toBe(true); + }); + + it("is case-insensitive for basenames", () => { + expect(shouldIncludeFile("Package-Lock.json")).toBe(false); + expect(shouldIncludeFile(".DS_STORE")).toBe(false); + expect(shouldIncludeFile("THUMBS.DB")).toBe(false); + }); + + it("is case-insensitive for extensions", () => { + expect(shouldIncludeFile("assets/icon.PNG")).toBe(false); + expect(shouldIncludeFile("dist/bundle.Min.Js")).toBe(false); + }); +}); + +describe("filterFilesForLlm", () => { + it("returns empty arrays for empty input", () => { + const result = filterFilesForLlm([]); + expect(result.files).toEqual([]); + expect(result.excludedByPath).toEqual([]); + }); + + it("removes denylisted files and reports them in excludedByPath", () => { + const code = makeFile({ path: "src/app.ts" }); + const lockfile = makeFile({ path: "pnpm-lock.yaml" }); + const image = makeFile({ path: "public/logo.png" }); + + const result = filterFilesForLlm([code, lockfile, image]); + + expect(result.files).toHaveLength(1); + expect(result.files[0]?.path).toBe("src/app.ts"); + expect(result.excludedByPath).toEqual(["pnpm-lock.yaml", "public/logo.png"]); + }); + + it("returns all-denylisted input as empty files with populated excludedByPath", () => { + const result = filterFilesForLlm([ + makeFile({ path: "pnpm-lock.yaml" }), + makeFile({ path: "yarn.lock" }), + ]); + expect(result.files).toEqual([]); + expect(result.excludedByPath).toEqual(["pnpm-lock.yaml", "yarn.lock"]); + }); +}); diff --git a/packages/cli/src/__tests__/format-diff.test.ts b/packages/cli/src/__tests__/format-diff.test.ts new file mode 100644 index 0000000..a78e798 --- /dev/null +++ b/packages/cli/src/__tests__/format-diff.test.ts @@ -0,0 +1,113 @@ +import type { Hunk } from "@stagereview/types/parsed-diff"; +import { LINE_TYPE } from "@stagereview/types/parsed-diff"; +import { describe, expect, it } from "vitest"; +import { formatHunkDiffWithLineNumbers } from "../format-diff.js"; + +describe("formatHunkDiffWithLineNumbers", () => { + it("renders the exact string the narrative model sees for a mixed hunk", () => { + const hunk: Hunk = { + header: "@@ -1,4 +1,5 @@", + oldStart: 1, + newStart: 1, + oldLines: 4, + newLines: 5, + lines: [ + { + type: LINE_TYPE.CONTEXT, + content: "const x = 1;", + oldLineNumber: 1, + newLineNumber: 1, + }, + { + type: LINE_TYPE.DELETION, + content: "const y = 2;", + oldLineNumber: 2, + }, + { + type: LINE_TYPE.ADDITION, + content: "const y = 20;", + newLineNumber: 2, + }, + { + type: LINE_TYPE.ADDITION, + content: "const z = 3;", + newLineNumber: 3, + }, + { + type: LINE_TYPE.CONTEXT, + content: "return x + y + z;", + oldLineNumber: 3, + newLineNumber: 4, + }, + { + type: LINE_TYPE.DELETION, + content: "// unused", + oldLineNumber: 4, + }, + { + type: LINE_TYPE.ADDITION, + content: "// end", + newLineNumber: 5, + }, + ], + }; + + expect(formatHunkDiffWithLineNumbers(hunk)).toBe( + [ + "1 1 | const x = 1;", + "2 |-const y = 2;", + " 2 |+const y = 20;", + " 3 |+const z = 3;", + "3 4 | return x + y + z;", + "4 |-// unused", + " 5 |+// end", + ].join("\n"), + ); + }); + + it("pads single- and multi-digit line numbers to a shared column width", () => { + const hunk: Hunk = { + header: "@@ -9,2 +9,3 @@", + oldStart: 9, + newStart: 9, + oldLines: 2, + newLines: 3, + lines: [ + { + type: LINE_TYPE.CONTEXT, + content: "a", + oldLineNumber: 9, + newLineNumber: 9, + }, + { + type: LINE_TYPE.ADDITION, + content: "b", + newLineNumber: 10, + }, + { + type: LINE_TYPE.CONTEXT, + content: "c", + oldLineNumber: 10, + newLineNumber: 11, + }, + ], + }; + + expect(formatHunkDiffWithLineNumbers(hunk)).toBe( + [" 9 9 | a", " 10 |+b", "10 11 | c"].join("\n"), + ); + }); + + it("uses blank padding when a hunk has no lines", () => { + const hunk: Hunk = { + header: "@@ -1,0 +1,0 @@", + oldStart: 1, + newStart: 1, + oldLines: 0, + newLines: 0, + lines: [], + }; + + expect(formatHunkDiffWithLineNumbers(hunk)).toBe(""); + }); +}); diff --git a/packages/cli/src/build-other-changes.ts b/packages/cli/src/build-other-changes.ts new file mode 100644 index 0000000..9e4fdf6 --- /dev/null +++ b/packages/cli/src/build-other-changes.ts @@ -0,0 +1,29 @@ +import type { HunkReference } from "@stagereview/types/chapters"; +import type { PullRequestFile } from "@stagereview/types/parsed-diff"; + +const OTHER_CHANGES_CHAPTER_ID = "chapter-other-changes"; +const OTHER_CHANGES_TITLE = "Other changes"; +const OTHER_CHANGES_SUMMARY = + "Lockfiles, generated files, and binary assets excluded from the chapters."; + +export function buildOtherChangesChapter(allFiles: PullRequestFile[], excludedByPath: string[]) { + if (excludedByPath.length === 0) return null; + + const excluded = new Set(excludedByPath); + const hunkRefs: HunkReference[] = []; + + for (const file of allFiles) { + if (!excluded.has(file.path)) continue; + for (const hunk of file.hunks) { + hunkRefs.push({ filePath: file.path, oldStart: hunk.oldStart }); + } + } + + return { + id: OTHER_CHANGES_CHAPTER_ID, + title: OTHER_CHANGES_TITLE, + summary: OTHER_CHANGES_SUMMARY, + hunkRefs, + keyChanges: [], + }; +} diff --git a/packages/cli/src/diff-parser.ts b/packages/cli/src/diff-parser.ts new file mode 100644 index 0000000..32413bd --- /dev/null +++ b/packages/cli/src/diff-parser.ts @@ -0,0 +1,215 @@ +import type { + DiffLine, + FileStatus, + Hunk, + LineType, + PullRequestFile, +} from "@stagereview/types/parsed-diff"; +import { FILE_STATUS, LINE_TYPE } from "@stagereview/types/parsed-diff"; +import parseDiff from "parse-diff"; + +interface ParseDiffFile { + from?: string; + to?: string; + additions: number; + deletions: number; + new?: boolean; + deleted?: boolean; + chunks: ParseDiffChunk[]; +} + +interface ParseDiffChunk { + content: string; + changes: ParseDiffChange[]; + oldStart: number; + oldLines: number; + newStart: number; + newLines: number; +} + +interface ParseDiffChange { + type: "add" | "del" | "normal"; + content: string; + ln?: number; + ln1?: number; + ln2?: number; +} + +function determineFileStatus(file: ParseDiffFile): FileStatus { + if (file.new) return FILE_STATUS.ADDED; + if (file.deleted) return FILE_STATUS.DELETED; + if (file.from !== file.to && file.from && file.to) { + const hasContentChanges = file.additions > 0 || file.deletions > 0; + return hasContentChanges ? FILE_STATUS.RENAMED : FILE_STATUS.MOVED; + } + return FILE_STATUS.MODIFIED; +} + +function getFilePath(file: ParseDiffFile): string { + if (file.deleted) { + if (!file.from) throw new Error("Deleted file is missing 'from' path"); + return file.from.replace(/^a\//, ""); + } + const filePath = file.to ?? file.from; + if (!filePath) throw new Error("File is missing both 'to' and 'from' paths"); + return filePath.replace(/^[ab]\//, ""); +} + +function getFilename(filePath: string): string { + return filePath.split("/").at(-1) ?? filePath; +} + +function transformChange(change: ParseDiffChange): DiffLine { + let type: LineType; + let oldLineNumber: number | undefined; + let newLineNumber: number | undefined; + + switch (change.type) { + case "add": + type = LINE_TYPE.ADDITION; + newLineNumber = change.ln; + break; + case "del": + type = LINE_TYPE.DELETION; + oldLineNumber = change.ln; + break; + default: + type = LINE_TYPE.CONTEXT; + oldLineNumber = change.ln1; + newLineNumber = change.ln2; + } + + let content = change.content; + if (content.startsWith("+") || content.startsWith("-") || content.startsWith(" ")) { + content = content.slice(1); + } + + return { type, content, oldLineNumber, newLineNumber }; +} + +function transformChunk(chunk: ParseDiffChunk): Hunk { + return { + header: chunk.content, + oldStart: chunk.oldStart, + newStart: chunk.newStart, + oldLines: chunk.oldLines, + newLines: chunk.newLines, + lines: chunk.changes.map(transformChange), + }; +} + +function getOldPath(file: ParseDiffFile): string | undefined { + if (!file.from) return undefined; + return file.from.replace(/^a\//, ""); +} + +function isSymlinkPatch(patch: string): boolean { + const headerEnd = patch.indexOf("@@"); + const header = headerEnd === -1 ? patch : patch.slice(0, headerEnd); + return /(?:new file mode|deleted file mode|old mode|new mode|index [0-9a-f]+\.\.[0-9a-f]+) 120000\b/m.test( + header, + ); +} + +function extractSymlinkTargets(file: ParseDiffFile): { + symlinkTarget: string | undefined; + oldSymlinkTarget: string | undefined; +} { + let symlinkTarget: string | undefined; + let oldSymlinkTarget: string | undefined; + + for (const chunk of file.chunks) { + for (const change of chunk.changes) { + const content = change.content.replace(/^[+-]/, "").trim(); + if (content === "\\ No newline at end of file") continue; + if (change.type === "add" && file.additions === 1) { + symlinkTarget = content; + } else if (change.type === "del" && file.deletions === 1) { + oldSymlinkTarget = content; + } + } + } + + return { symlinkTarget, oldSymlinkTarget }; +} + +function transformFile(file: ParseDiffFile, patch: string): PullRequestFile { + const filePath = getFilePath(file); + const status = determineFileStatus(file); + const isPathChange = status === FILE_STATUS.RENAMED || status === FILE_STATUS.MOVED; + const isSymlink = isSymlinkPatch(patch); + + return { + path: filePath, + oldPath: isPathChange ? getOldPath(file) : undefined, + filename: getFilename(filePath), + status, + additions: file.additions, + deletions: file.deletions, + hunks: file.chunks.map(transformChunk), + patch, + ...(isSymlink && { + isSymlink: true, + ...extractSymlinkTargets(file), + }), + }; +} + +function findNextDiffMarker(text: string, startIndex: number): number { + const marker = "diff --git "; + let pos = startIndex; + + while (pos < text.length) { + const idx = text.indexOf(marker, pos); + if (idx === -1) return -1; + if (idx === 0 || text[idx - 1] === "\n") return idx; + pos = idx + marker.length; + } + + return -1; +} + +function splitRawPatches(diffOutput: string): string[] { + const patches: string[] = []; + let start = findNextDiffMarker(diffOutput, 0); + + while (start !== -1) { + const next = findNextDiffMarker(diffOutput, start + 1); + const segment = next === -1 ? diffOutput.slice(start) : diffOutput.slice(start, next); + patches.push(segment.trimEnd()); + start = next; + } + + return patches; +} + +export function parseGitDiff(diffOutput: string): PullRequestFile[] { + if (!diffOutput || diffOutput.trim() === "") { + return []; + } + + const rawPatches = splitRawPatches(diffOutput); + return rawPatches.map((patch, index) => { + const parsed = parseDiff(patch) as ParseDiffFile[]; + const file = parsed[0]; + if (!file) { + throw new Error(`Failed to parse patch at index ${index}`); + } + return transformFile(file, patch); + }); +} + +export function calculateDiffStats(files: PullRequestFile[]): { + totalAdditions: number; + totalDeletions: number; + fileCount: number; +} { + return files.reduce( + (acc, file) => ({ + totalAdditions: acc.totalAdditions + file.additions, + totalDeletions: acc.totalDeletions + file.deletions, + fileCount: acc.fileCount + 1, + }), + { totalAdditions: 0, totalDeletions: 0, fileCount: 0 }, + ); +} diff --git a/packages/cli/src/filter-files.ts b/packages/cli/src/filter-files.ts new file mode 100644 index 0000000..c519bbe --- /dev/null +++ b/packages/cli/src/filter-files.ts @@ -0,0 +1,65 @@ +import type { PullRequestFile } from "@stagereview/types/parsed-diff"; + +const IGNORED_FILENAMES = new Set([ + "package-lock.json", + "yarn.lock", + "pnpm-lock.yaml", + "bun.lockb", + "bun.lock", + "composer.lock", + "gemfile.lock", + "cargo.lock", + "poetry.lock", + "pipfile.lock", + "go.sum", + "flake.lock", + ".ds_store", + "thumbs.db", +]); + +const IGNORED_EXTENSIONS = [ + ".min.js", + ".min.css", + ".map", + ".snap", + ".svg", + ".png", + ".jpg", + ".jpeg", + ".gif", + ".ico", + ".woff", + ".woff2", + ".ttf", + ".eot", + ".mp4", + ".webm", + ".pdf", +] as const; + +export function shouldIncludeFile(filePath: string): boolean { + const basename = (filePath.split("/").at(-1) ?? filePath).toLowerCase(); + if (IGNORED_FILENAMES.has(basename)) return false; + const lowerPath = filePath.toLowerCase(); + return !IGNORED_EXTENSIONS.some((ext) => lowerPath.endsWith(ext)); +} + +export interface FilterFilesResult { + files: PullRequestFile[]; + excludedByPath: string[]; +} + +export function filterFilesForLlm(files: PullRequestFile[]): FilterFilesResult { + const excludedByPath: string[] = []; + const reviewable: PullRequestFile[] = []; + + for (const file of files) { + if (!shouldIncludeFile(file.path)) { + excludedByPath.push(file.path); + continue; + } + reviewable.push(file); + } + + return { files: reviewable, excludedByPath }; +} diff --git a/packages/cli/src/format-diff.ts b/packages/cli/src/format-diff.ts new file mode 100644 index 0000000..8c5f9fb --- /dev/null +++ b/packages/cli/src/format-diff.ts @@ -0,0 +1,41 @@ +import { type Hunk, LINE_TYPE, type LineType } from "@stagereview/types/parsed-diff"; + +const LINE_PREFIX: Partial> = { + [LINE_TYPE.ADDITION]: "+", + [LINE_TYPE.DELETION]: "-", +}; + +export function formatHunkDiff(hunk: Hunk): string { + return hunk.lines.map((line) => `${LINE_PREFIX[line.type] ?? " "}${line.content}`).join("\n"); +} + +export function formatHunkDiffWithLineNumbers(hunk: Hunk): string { + const maxOld = hunk.oldStart + Math.max(hunk.oldLines - 1, 0); + const maxNew = hunk.newStart + Math.max(hunk.newLines - 1, 0); + const colWidth = Math.max(String(maxOld).length, String(maxNew).length); + + return hunk.lines + .map((line) => { + const prefix = LINE_PREFIX[line.type] ?? " "; + const oldNum = + line.oldLineNumber != null + ? String(line.oldLineNumber).padStart(colWidth) + : " ".repeat(colWidth); + const newNum = + line.newLineNumber != null + ? String(line.newLineNumber).padStart(colWidth) + : " ".repeat(colWidth); + return `${oldNum} ${newNum} |${prefix}${line.content}`; + }) + .join("\n"); +} + +export function countHunkLines(hunk: Hunk): { added: number; deleted: number } { + let added = 0; + let deleted = 0; + for (const line of hunk.lines) { + if (line.type === LINE_TYPE.ADDITION) added++; + else if (line.type === LINE_TYPE.DELETION) deleted++; + } + return { added, deleted }; +} diff --git a/packages/cli/src/git.ts b/packages/cli/src/git.ts index e46ef6e..f93242a 100644 --- a/packages/cli/src/git.ts +++ b/packages/cli/src/git.ts @@ -1,7 +1,7 @@ import { execFileSync } from "node:child_process"; import path from "node:path"; import type { ChapterRunRow } from "./db/schema/chapter-run.js"; -import { SCOPE_KIND, WORKING_TREE_REF } from "./schema.js"; +import { SCOPE_KIND, type Scope, WORKING_TREE_REF } from "./schema.js"; export class NotInGitRepoError extends Error { constructor() { @@ -63,7 +63,7 @@ export function buildDiffArgs(run: ChapterRunRow): string[] { case WORKING_TREE_REF.STAGED: return ["diff", "--no-color", "--cached"]; case WORKING_TREE_REF.WORK: - return ["diff", "--no-color", "HEAD"]; + return ["diff", "--no-color", run.baseSha]; } } @@ -85,3 +85,161 @@ export function parseRepoName(originUrl: string | null, repoRoot: string): strin } return path.basename(repoRoot); } + +export function detectBaseRef(): string { + const candidates: string[][] = [ + ["rev-parse", "--abbrev-ref", "origin/HEAD"], + ["rev-parse", "--verify", "main"], + ["rev-parse", "--verify", "master"], + ["rev-parse", "--verify", "origin/main"], + ["rev-parse", "--verify", "origin/master"], + ]; + + for (const args of candidates) { + try { + const out = execFileSync("git", args, { + encoding: "utf8", + stdio: ["ignore", "pipe", "ignore"], + }).trim(); + if (out) return out; + } catch { + // try next candidate + } + } + + throw new Error( + "No default branch detected. Tried origin/HEAD, main, master, origin/main, and origin/master.", + ); +} + +export function resolveMergeBase(base: string): string { + return execFileSync("git", ["merge-base", base, "HEAD"], { + encoding: "utf8", + stdio: ["ignore", "pipe", "ignore"], + }).trim(); +} + +export function resolveHead(): string { + return execFileSync("git", ["rev-parse", "HEAD"], { + encoding: "utf8", + stdio: ["ignore", "pipe", "ignore"], + }).trim(); +} + +export function getRawDiff(args: string[]): string { + return execFileSync( + "git", + ["diff", "--no-color", "--src-prefix=a/", "--dst-prefix=b/", ...args], + { + encoding: "utf8", + stdio: ["ignore", "pipe", "ignore"], + maxBuffer: 50 * 1024 * 1024, + }, + ); +} + +export function getUntrackedFiles(): string[] { + const out = execFileSync("git", ["ls-files", "--others", "--exclude-standard"], { + encoding: "utf8", + stdio: ["ignore", "pipe", "ignore"], + }).trim(); + return out ? out.split("\n") : []; +} + +function hasStringStdout(err: unknown): err is { stdout: string } { + return ( + typeof err === "object" && err !== null && "stdout" in err && typeof err.stdout === "string" + ); +} + +export function getUntrackedDiff(files: string[]): string { + const patches: string[] = []; + for (const file of files) { + try { + execFileSync( + "git", + [ + "diff", + "--no-index", + "--no-color", + "--src-prefix=a/", + "--dst-prefix=b/", + "--", + "/dev/null", + file, + ], + { + encoding: "utf8", + stdio: ["ignore", "pipe", "ignore"], + maxBuffer: 50 * 1024 * 1024, + }, + ); + } catch (err: unknown) { + if (hasStringStdout(err)) { + patches.push(err.stdout); + } + } + } + return patches.join("\n"); +} + +export function getCommitMessages(mergeBase: string): string { + return execFileSync("git", ["log", "--oneline", `${mergeBase}..HEAD`], { + encoding: "utf8", + stdio: ["ignore", "pipe", "ignore"], + }).trim(); +} + +export function hasUncommittedChanges(): boolean { + const out = execFileSync("git", ["status", "--porcelain"], { + encoding: "utf8", + stdio: ["ignore", "pipe", "ignore"], + }).trim(); + return out.length > 0; +} + +export interface ResolvedScope { + scope: Scope; + mergeBaseSha: string; + rawDiff: string; +} + +export function resolveScope(baseOverride?: string): ResolvedScope { + const base = baseOverride ?? detectBaseRef(); + const mergeBaseSha = resolveMergeBase(base); + const headSha = resolveHead(); + const uncommitted = hasUncommittedChanges(); + + if (uncommitted) { + let rawDiff = getRawDiff([mergeBaseSha]); + const untrackedFiles = getUntrackedFiles(); + if (untrackedFiles.length > 0) { + const untrackedDiff = getUntrackedDiff(untrackedFiles); + if (untrackedDiff) { + rawDiff = rawDiff ? `${rawDiff}\n${untrackedDiff}` : untrackedDiff; + } + } + return { + scope: { + kind: SCOPE_KIND.WORKING_TREE, + ref: WORKING_TREE_REF.WORK, + baseSha: mergeBaseSha, + headSha, + mergeBaseSha, + }, + mergeBaseSha, + rawDiff, + }; + } + + return { + scope: { + kind: SCOPE_KIND.COMMITTED, + baseSha: mergeBaseSha, + headSha, + mergeBaseSha, + }, + mergeBaseSha, + rawDiff: getRawDiff([`${mergeBaseSha}..${headSha}`]), + }; +} diff --git a/packages/cli/src/index.ts b/packages/cli/src/index.ts index 54b8aba..53ef96a 100644 --- a/packages/cli/src/index.ts +++ b/packages/cli/src/index.ts @@ -1,17 +1,28 @@ #!/usr/bin/env node import { Command } from "commander"; +import { runPrep } from "./prep.js"; import { show } from "./show.js"; const program = new Command(); program.name("stagereview").description("Chapter-style code review against your local git branch."); +program + .command("prep") + .description("Parse the current branch diff and prepare input for chapter generation") + .option("--base ", "Base ref to diff against (default: auto-detect main/master)") + .action((opts: { base?: string }) => { + const filePath = runPrep(opts.base); + process.stdout.write(filePath); + }); + program .command("show") .description("Load a chapters.json file and open it in a local browser") .argument("", "Path to a chapters.json file") - .action(async (jsonPath: string) => { - await show(jsonPath); + .option("--base ", "Base ref to diff against (default: auto-detect main/master)") + .action(async (jsonPath: string, opts: { base?: string }) => { + await show(jsonPath, opts.base); }); program.parseAsync(process.argv).catch((err) => { diff --git a/packages/cli/src/prep.ts b/packages/cli/src/prep.ts new file mode 100644 index 0000000..67b3975 --- /dev/null +++ b/packages/cli/src/prep.ts @@ -0,0 +1,34 @@ +import { writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import path from "node:path"; +import type { Hunk, PullRequestFile } from "@stagereview/types/parsed-diff"; +import { parseGitDiff } from "./diff-parser.js"; +import { filterFilesForLlm } from "./filter-files.js"; +import { formatHunkDiffWithLineNumbers } from "./format-diff.js"; +import { getCommitMessages, resolveScope } from "./git.js"; + +function formatHunkForPrompt(file: PullRequestFile, hunk: Hunk): string { + return `=== File: ${file.path} (${file.status}) | filePath: "${file.path}", oldStart: ${hunk.oldStart} === +=== Hunk @${hunk.oldStart}: ${hunk.header} === +${formatHunkDiffWithLineNumbers(hunk)}`; +} + +export function runPrep(base?: string): string { + const { rawDiff, mergeBaseSha } = resolveScope(base); + + const allFiles = parseGitDiff(rawDiff); + const { files } = filterFilesForLlm(allFiles); + + const formattedHunks = files + .flatMap((file) => file.hunks.map((hunk) => formatHunkForPrompt(file, hunk))) + .join("\n\n"); + + const commitMessages = getCommitMessages(mergeBaseSha); + + const sections = ["=== COMMIT MESSAGES ===", commitMessages, "", "=== HUNKS ===", formattedHunks]; + + const filePath = path.join(tmpdir(), `stage-prep-${Date.now()}.txt`); + writeFileSync(filePath, sections.join("\n"), "utf8"); + + return filePath; +} diff --git a/packages/cli/src/routes/diff.ts b/packages/cli/src/routes/diff.ts index 109fef4..a5bb607 100644 --- a/packages/cli/src/routes/diff.ts +++ b/packages/cli/src/routes/diff.ts @@ -142,7 +142,7 @@ function getContentRefs(run: ChapterRunRow): { oldRef: string; newRef: string | case WORKING_TREE_REF.STAGED: return { oldRef: "HEAD", newRef: "" }; case WORKING_TREE_REF.WORK: - return { oldRef: "HEAD", newRef: "DISK" }; + return { oldRef: run.baseSha, newRef: "DISK" }; default: return { oldRef: "HEAD", newRef: "HEAD" }; } diff --git a/packages/cli/src/schema.ts b/packages/cli/src/schema.ts index 2129e9e..8a3f3bf 100644 --- a/packages/cli/src/schema.ts +++ b/packages/cli/src/schema.ts @@ -67,3 +67,9 @@ export const ChaptersFileSchema = z.strictObject({ generatedAt: z.iso.datetime(), }); export type ChaptersFile = z.infer; + +export const AgentOutputSchema = z.strictObject({ + chapters: z.array(chapterSchema), + prologue: PrologueSchema.optional(), +}); +export type AgentOutput = z.infer; diff --git a/packages/cli/src/show.ts b/packages/cli/src/show.ts index f173fae..9680ec2 100644 --- a/packages/cli/src/show.ts +++ b/packages/cli/src/show.ts @@ -1,14 +1,29 @@ +import { readFileSync } from "node:fs"; +import path from "node:path"; import open from "open"; +import { buildOtherChangesChapter } from "./build-other-changes.js"; import { closeDb, getDb } from "./db/client.js"; +import { parseGitDiff } from "./diff-parser.js"; +import { filterFilesForLlm } from "./filter-files.js"; +import { readRepoContext, resolveScope } from "./git.js"; import { diffRoutes } from "./routes/diff.js"; import { runRoutes } from "./routes/runs.js"; import { viewStateRoutes } from "./routes/view-state.js"; -import { importChaptersFile } from "./runs/import-chapters.js"; +import { insertChaptersFile } from "./runs/import-chapters.js"; +import { + type AgentOutput, + AgentOutputSchema, + type Chapter, + type ChaptersFile, + ChaptersFileSchema, + DIFF_SIDE, +} from "./schema.js"; import { LOOPBACK_HOST, startServer } from "./server.js"; -export async function show(jsonPath: string): Promise { +export async function show(jsonPath: string, base?: string): Promise { const db = getDb(); - const { runId } = importChaptersFile(jsonPath, db); + const chaptersFile = loadChaptersFile(jsonPath, base); + const { runId } = insertChaptersFile(db, chaptersFile, readRepoContext()); const handle = await startServer({ routes: [...runRoutes(db), ...viewStateRoutes(db), ...diffRoutes(db)], @@ -31,6 +46,195 @@ export async function show(jsonPath: string): Promise { closeDb(); } +function loadChaptersFile(jsonPath: string, base?: string): ChaptersFile { + const absolute = path.resolve(jsonPath); + const raw = readFileSync(absolute, "utf8"); + const parsed = JSON.parse(raw) as unknown; + + const fullResult = ChaptersFileSchema.safeParse(parsed); + if (fullResult.success) return fullResult.data; + + const agentResult = AgentOutputSchema.safeParse(parsed); + if (agentResult.success) return assembleChaptersFile(agentResult.data, base); + + throw fullResult.error; +} + +function assembleChaptersFile(agentOutput: AgentOutput, base?: string): ChaptersFile { + const { scope, rawDiff } = resolveScope(base); + const allFiles = parseGitDiff(rawDiff); + const { files: filteredFiles, excludedByPath } = filterFilesForLlm(allFiles); + + validateHunkCoverage(filteredFiles, agentOutput.chapters); + const sanitized = sanitizeLineRefs(agentOutput.chapters, filteredFiles); + + const chapters = [...sanitized]; + const otherChanges = buildOtherChangesChapter(allFiles, excludedByPath); + if (otherChanges) { + chapters.push({ ...otherChanges, order: chapters.length + 1 }); + } + + return { + scope, + chapters, + prologue: agentOutput.prologue, + generatedAt: new Date().toISOString(), + }; +} + +function validateHunkCoverage( + filteredFiles: { path: string; hunks: { oldStart: number }[] }[], + chapters: Chapter[], +): void { + const expected = new Map>(); + for (const file of filteredFiles) { + const starts = new Set(); + for (const hunk of file.hunks) { + starts.add(hunk.oldStart); + } + if (starts.size > 0) { + expected.set(file.path, starts); + } + } + + const actual = new Map>(); + const duplicates: string[] = []; + for (const chapter of chapters) { + for (const ref of chapter.hunkRefs) { + let starts = actual.get(ref.filePath); + if (!starts) { + starts = new Map(); + actual.set(ref.filePath, starts); + } + const count = starts.get(ref.oldStart) ?? 0; + if (count > 0) { + duplicates.push(` filePath: "${ref.filePath}", oldStart: ${ref.oldStart}`); + } + starts.set(ref.oldStart, count + 1); + } + } + + const missing: string[] = []; + for (const [filePath, starts] of expected) { + const actualStarts = actual.get(filePath); + for (const oldStart of starts) { + if (!actualStarts?.has(oldStart)) { + missing.push(` filePath: "${filePath}", oldStart: ${oldStart}`); + } + } + } + + const extra: string[] = []; + for (const [filePath, starts] of actual) { + const expectedStarts = expected.get(filePath); + for (const oldStart of starts.keys()) { + if (!expectedStarts?.has(oldStart)) { + extra.push(` filePath: "${filePath}", oldStart: ${oldStart}`); + } + } + } + + if (missing.length === 0 && extra.length === 0 && duplicates.length === 0) return; + + const lines = ["Hunk coverage validation failed."]; + if (missing.length > 0) { + lines.push(`Missing hunks (${missing.length}) — not assigned to any chapter:`); + lines.push(...missing); + } + if (extra.length > 0) { + lines.push(`Extra hunks (${extra.length}) — not found in the diff:`); + lines.push(...extra); + } + if (duplicates.length > 0) { + lines.push(`Duplicate hunks (${duplicates.length}) — assigned to multiple chapters:`); + lines.push(...duplicates); + } + throw new Error(lines.join("\n")); +} + +interface HunkSpan { + oldStart: number; + oldEnd: number; + newStart: number; + newEnd: number; +} + +function sanitizeLineRefs( + chapters: Chapter[], + filteredFiles: { + path: string; + hunks: { oldStart: number; oldLines: number; newStart: number; newLines: number }[]; + }[], +): Chapter[] { + const hunkSpanIndex = new Map>(); + for (const file of filteredFiles) { + const spans = new Map(); + for (const hunk of file.hunks) { + spans.set(hunk.oldStart, { + oldStart: hunk.oldStart, + oldEnd: hunk.oldStart + hunk.oldLines - 1, + newStart: hunk.newStart, + newEnd: hunk.newStart + hunk.newLines - 1, + }); + } + if (spans.size > 0) { + hunkSpanIndex.set(file.path, spans); + } + } + + return chapters.map((chapter) => { + const chapterSpans = new Map(); + for (const ref of chapter.hunkRefs) { + const fileSpans = hunkSpanIndex.get(ref.filePath); + if (!fileSpans) continue; + const span = fileSpans.get(ref.oldStart); + if (!span) continue; + let spans = chapterSpans.get(ref.filePath); + if (!spans) { + spans = []; + chapterSpans.set(ref.filePath, spans); + } + spans.push(span); + } + + const keyChanges = chapter.keyChanges.flatMap((kc) => { + const validRefs = kc.lineRefs.filter((ref) => { + if (ref.startLine < 1 || ref.endLine < 1) return false; + if (ref.startLine > ref.endLine) return false; + + const spans = chapterSpans.get(ref.filePath); + if (!spans) return false; + + return spans.some((span) => { + const [rangeStart, rangeEnd] = + ref.side === DIFF_SIDE.ADDITIONS + ? [span.newStart, span.newEnd] + : [span.oldStart, span.oldEnd]; + if (rangeStart > rangeEnd) return false; + return ref.startLine >= rangeStart && ref.endLine <= rangeEnd; + }); + }); + + const uniqueRefs: typeof validRefs = []; + for (const ref of validRefs) { + const isDuplicate = uniqueRefs.some( + (existing) => + existing.filePath === ref.filePath && + existing.side === ref.side && + existing.startLine === ref.startLine && + existing.endLine === ref.endLine, + ); + if (!isDuplicate) uniqueRefs.push(ref); + } + + if (uniqueRefs.length === 0) return []; + return [{ content: kc.content, lineRefs: uniqueRefs }]; + }); + + return { ...chapter, keyChanges }; + }); +} + function waitForShutdownSignal(): Promise { return new Promise((resolve) => { const cleanup = () => { diff --git a/packages/types/package.json b/packages/types/package.json index d2957ca..a920cc2 100644 --- a/packages/types/package.json +++ b/packages/types/package.json @@ -8,6 +8,7 @@ ".": "./src/index.ts", "./chapters": "./src/chapters.ts", "./diff": "./src/diff.ts", + "./parsed-diff": "./src/parsed-diff.ts", "./prologue": "./src/prologue.ts", "./view-state": "./src/view-state.ts" }, diff --git a/packages/types/src/index.ts b/packages/types/src/index.ts index e300f69..ee0225d 100644 --- a/packages/types/src/index.ts +++ b/packages/types/src/index.ts @@ -1,4 +1,5 @@ export * from "./chapters.ts"; export * from "./diff.ts"; +export * from "./parsed-diff.ts"; export * from "./prologue.ts"; export * from "./view-state.ts"; diff --git a/packages/types/src/parsed-diff.ts b/packages/types/src/parsed-diff.ts new file mode 100644 index 0000000..d9d529d --- /dev/null +++ b/packages/types/src/parsed-diff.ts @@ -0,0 +1,51 @@ +import { z } from "zod"; + +export const FILE_STATUS = { + ADDED: "added", + MODIFIED: "modified", + DELETED: "deleted", + RENAMED: "renamed", + MOVED: "moved", +} as const; +export type FileStatus = (typeof FILE_STATUS)[keyof typeof FILE_STATUS]; + +export const LINE_TYPE = { + CONTEXT: "context", + ADDITION: "addition", + DELETION: "deletion", + HEADER: "header", +} as const; +export type LineType = (typeof LINE_TYPE)[keyof typeof LINE_TYPE]; + +export const diffLineSchema = z.object({ + type: z.enum(LINE_TYPE), + content: z.string(), + oldLineNumber: z.number().optional(), + newLineNumber: z.number().optional(), +}); +export type DiffLine = z.infer; + +export const hunkSchema = z.object({ + header: z.string(), + oldStart: z.number(), + newStart: z.number(), + oldLines: z.number(), + newLines: z.number(), + lines: z.array(diffLineSchema), +}); +export type Hunk = z.infer; + +export const pullRequestFileSchema = z.object({ + path: z.string(), + oldPath: z.string().optional(), + filename: z.string(), + status: z.enum(FILE_STATUS), + additions: z.number(), + deletions: z.number(), + hunks: z.array(hunkSchema), + patch: z.string().optional(), + isSymlink: z.boolean().optional(), + symlinkTarget: z.string().optional(), + oldSymlinkTarget: z.string().optional(), +}); +export type PullRequestFile = z.infer; diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 53cf24e..6f28d8b 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -47,6 +47,9 @@ importers: open: specifier: ^11.0.0 version: 11.0.0 + parse-diff: + specifier: ^0.11.1 + version: 0.11.1 zod: specifier: ^4.3.6 version: 4.4.2 @@ -3015,6 +3018,9 @@ packages: resolution: {integrity: sha512-smsWv2LzFjP03xmvFoJ331ss6h+jixfA4UUV/Bsiyuu4YJPfN+FIQGOIiv4w9/+MoHkfkJ22UIaQWRVFRfH6Vw==} engines: {node: '>=20'} + parse-diff@0.11.1: + resolution: {integrity: sha512-Oq4j8LAOPOcssanQkIjxosjATBIEJhCxMCxPhMu+Ci4wdNmAEdx0O+a7gzbR2PyKXgKPvRLIN5g224+dJAsKHA==} + parse-entities@4.0.2: resolution: {integrity: sha512-GG2AQYWoLgL877gQIKeRPGO1xF9+eG1ujIb5soS5gPvLQ1y2o8FL90w2QWNdf9I361Mpp7726c+lj3U0qK1uGw==} @@ -6342,6 +6348,8 @@ snapshots: powershell-utils: 0.1.0 wsl-utils: 0.3.1 + parse-diff@0.11.1: {} + parse-entities@4.0.2: dependencies: '@types/unist': 2.0.11 diff --git a/skills/stage-chapters/SKILL.md b/skills/stage-chapters/SKILL.md index facbe15..abc57b9 100644 --- a/skills/stage-chapters/SKILL.md +++ b/skills/stage-chapters/SKILL.md @@ -6,7 +6,7 @@ user-invocable: true # stage-chapters -Generates a Stage chapter run for the current local git branch and opens it in a browser. The skill detects the base ref, computes the diff, generates chapters from it, writes a JSON file matching the `stagereview` schema, and hands the file to `stagereview show` to launch the SPA. +Generates a Stage chapter run for the current local git branch and opens it in a browser. Uses `stagereview prep` to compute the diff, then generates chapters and a prologue, and hands the result to `stagereview show` to launch the SPA. ## Prerequisites @@ -30,46 +30,52 @@ Run these checks before any other work. If either fails, stop with the error mes /stage-chapters must be run inside a git repository. ``` -## Step 1 — Detect base ref +## Step 1 — Run prep -Find the branch the user reviews against. Try each of the following in order; the first that succeeds becomes ``: +```bash +PREP_FILE=$(stagereview prep) +``` -1. `git rev-parse --abbrev-ref origin/HEAD 2>/dev/null` — typically prints `origin/main`. Use the full output (e.g. `origin/main`) as ``; do **not** strip `origin/`, because the bare name (`main`) may not exist locally in single-branch clones. -2. `git rev-parse --verify main 2>/dev/null` — local `main` branch; use `main` as ``. -3. `git rev-parse --verify master 2>/dev/null` — older repos; use `master` as ``. -4. `git rev-parse --verify origin/main 2>/dev/null` — remote-tracking fallback when `origin/HEAD` is unset; use `origin/main` as ``. -5. `git rev-parse --verify origin/master 2>/dev/null` — remote-tracking fallback for older repos; use `origin/master` as ``. +`stagereview prep` auto-detects the base ref (main/master), computes the merge-base, generates the diff (including uncommitted and untracked changes when present), filters out lockfiles/binaries, and formats hunks with line numbers for analysis. It writes a plain-text file and prints only the file path to stdout. -If all five fail, stop with: +If the user specifies a base branch (e.g., "generate chapters against `feature-a`"), pass `--base ` to both `prep` and `show`: -``` -No default branch detected. Tried origin/HEAD, main, master, origin/main, and origin/master. +```bash +PREP_FILE=$(stagereview prep --base feature-a) +# ... later ... +stagereview show --base feature-a "$AGENT_OUTPUT" ``` -`` is whatever ref expression was verified above and is passed verbatim to `git merge-base` / `git rev-parse` in Step 2. +If `prep` exits non-zero, relay its stderr to the user and stop. -## Step 2 — Get the diff +**Do not modify files in the working tree between running `prep` and running `show`.** Both commands independently snapshot the git state. If the diff changes between them, `show` will reject the chapters with a hunk coverage error because the hunks no longer match. -Compute the merge-base and dump the unified diff for the committed range only: +## Step 2 — Read prep output -```bash -MERGE_BASE=$(git merge-base HEAD) -HEAD_SHA=$(git rev-parse HEAD) +Read `$PREP_FILE` via the Read tool (or equivalent). For large diffs, use the Read tool's `offset` and `limit` parameters to read in chunks. -git diff "$MERGE_BASE..HEAD" -``` +The file has two sections separated by headers: -If `git merge-base` exits non-zero or `MERGE_BASE` is empty, stop with an error like `Could not compute merge-base between and HEAD (unrelated histories or shallow clone).` Do **not** continue — running `git diff "..HEAD"` with an empty `MERGE_BASE` produces an empty diff that would silently yield zero chapters. +1. **`=== COMMIT MESSAGES ===`** — `git log --oneline` output for prologue context. +2. **`=== HUNKS ===`** — formatted diff hunks with line numbers. Each hunk looks like: -`git diff ..HEAD` covers exactly the commits on the branch since it diverged from the base — the same range the SPA renders for a `committed` run (`baseSha..headSha` in `packages/cli/src/routes/diff.ts`). Save the full diff text into context for Step 3. +``` +=== File: src/app.ts (modified) | filePath: "src/app.ts", oldStart: 1 === +=== Hunk @1: @@ -1,5 +1,6 @@ === +1 1 | const a = 1; +2 |-const b = 2; + 2 |+const b = 3; + 3 |+const c = 4; +3 4 | const d = 5; +``` -This skill scopes review to *committed* work. If the user has uncommitted changes to tracked files, instruct them to commit first; mixing committed and working-tree changes into a single run would produce `hunkRefs`/`lineRefs` that don't line up with the diff the SPA serves. +The two number columns are the **old line number** (left) and **new line number** (right). A blank column means the line doesn't exist on that side — additions have no old line number, deletions have no new line number. These numbers are used directly for `lineRefs` in key changes (see Step 3d). -`MERGE_BASE` and `HEAD_SHA` are full 40-character SHAs that feed directly into the JSON `scope` field in Step 4. +`commits.txt` contains `git log --oneline` output for prologue context. ## Step 3 — Cluster + narrate -Using the full diff from Step 2, produce a `chapters` array. Each chapter groups related hunks into a coherent story beat, narrates them for a reviewer unfamiliar with this part of the codebase, and flags judgment calls that need human input. +Using the hunks from `hunks.txt`, produce a `chapters` array. Each chapter groups related hunks into a coherent story beat, narrates them for a reviewer unfamiliar with this part of the codebase, and flags judgment calls that need human input. ### 3a — Clustering rules @@ -96,16 +102,16 @@ Consider symbol dependencies between chapters — a chapter that introduces a ty ### 3b — Self-validation rules -Every hunk in the diff **must** appear in exactly one chapter. No hunk may be omitted and no hunk may appear in more than one chapter. +Every hunk in the formatted diff **must** appear in exactly one chapter. No hunk may be omitted and no hunk may appear in more than one chapter. -Identify each hunk by its exact `(filePath, oldStart)` tuple from the unified-diff `@@ -X,Y +A,B @@` header. Use the EXACT `oldStart` value from the `@@` header — do not recount lines yourself. +Each hunk header in the prep output has the format: +``` +=== File: () | filePath: "", oldStart: === +``` -- `filePath` is the path after `b/` in the `diff --git a/... b/...` line. -- `oldStart` is the `X` in `@@ -X,Y +A,B @@`. For newly created files the header is `@@ -0,0 +1,N @@`, so `oldStart` is `0`. +Use the `filePath` and `oldStart` values from these headers to build `hunkRefs`. -After building the chapters array, verify: -1. The total number of `hunkRefs` across all chapters equals the total number of `@@` headers in the diff. -2. Every `(filePath, oldStart)` pair from the diff appears in exactly one chapter's `hunkRefs`. +`stagereview show` validates hunk coverage automatically — it will error with a list of missing or extra hunks if the chapters don't account for every hunk in the diff. If this happens, fix the chapters and retry. ### 3c — Narration rules @@ -126,10 +132,12 @@ Frame each item as a **question**. Each key change includes `lineRefs`: one line range per distinct spot the question depends on. Most questions touch a single location, so use one range; only add more when the judgment genuinely spans related code in different places. -- Use OLD-column line numbers for `side: "deletions"` (left side of the diff). -- Use NEW-column line numbers for `side: "additions"` (right side of the diff). -- Keep ranges tight — point to the specific lines the question is about, not the entire hunk. -- `startLine` and `endLine` must both be positive integers with `endLine >= startLine`. +**Reading line numbers from `hunks.txt`:** Each diff line shows two number columns — old (left) and new (right). Use these numbers directly: +- For `side: "deletions"` — use the **old** (left) column number as `startLine`/`endLine`. +- For `side: "additions"` — use the **new** (right) column number as `startLine`/`endLine`. +- Do **not** count lines yourself — read the numbers from the formatted output. + +Keep ranges tight — point to the specific lines the question is about, not the entire hunk. `startLine` and `endLine` must both be positive integers with `endLine >= startLine`. **Good examples:** @@ -174,18 +182,14 @@ Produce an array of chapter objects. Each chapter: } ``` -- Do **not** invent `hunkRefs` — only use `(filePath, oldStart)` tuples that actually appear in the diff's `@@` headers. +- Do **not** invent `hunkRefs` — only use `(filePath, oldStart)` tuples that actually appear in the formatted hunks. - `keyChanges[].lineRefs` must have at least one entry per key change. ## Step 4 — Generate prologue After building the chapters, generate a **prologue** — a high-level overview of the entire change. The prologue helps reviewers orient themselves before diving into individual chapters. -Read the commit messages for context: - -```bash -git log --oneline "$MERGE_BASE..HEAD" -``` +Use `commits.txt` from the prep output for context. Using the diff, chapters, and commit messages, produce a `prologue` object with the following fields: @@ -236,99 +240,29 @@ Object with: Talk like a coworker, not a changelog. No jargon, no filler phrases, no "this change introduces/implements/adds". Just say what happened and why it matters. -## Step 5 — Write JSON file +## Step 5 — Write agent output -Compute a unique temp path. The trailing `XXXXXX` (with no suffix after) is required by macOS BSD `mktemp` — placing characters after the X's causes BSD `mktemp` to return the template verbatim instead of substituting random characters: +Compute a unique temp path and write the JSON via a bash heredoc: ```bash -TMPFILE=$(mktemp "${TMPDIR:-/tmp}/stage-chapters.XXXXXX") -``` - -`stagereview show` reads JSON regardless of file extension, so the missing `.json` suffix is fine. - -The `${TMPDIR:-/tmp}` fallback matters on macOS, where `os.tmpdir()` resolves to `/var/folders/...` but `$TMPDIR` is not always set in every shell. Avoid `date +%s%N` — the `%N` (nanoseconds) format is a GNU extension and on macOS BSD `date` it emits a literal `N`, breaking uniqueness. - -Write a JSON file at `"$TMPFILE"` matching the shape below. The file must validate against `ChaptersFileSchema` in `packages/cli/src/schema.ts`; mismatched fields will be rejected by `stagereview show`. - -High-level shape: - -``` -{ scope: {...}, chapters: [...], prologue: {...}, generatedAt: "..." } -``` - -Full example: - -```jsonc +AGENT_OUTPUT=$(mktemp "${TMPDIR:-/tmp}/stage-agent-output.XXXXXX") +cat > "$AGENT_OUTPUT" << 'AGENT_EOF' { - "scope": { - "kind": "committed", - // Set baseSha = mergeBaseSha = $MERGE_BASE so the SPA renders - // baseSha..headSha — the same range chapters were generated from. - "baseSha": "", - "headSha": "", - "mergeBaseSha": "" - }, - "chapters": [ - { - "id": "ch-1", - "order": 1, - "title": "Short imperative title", - "summary": "Why this chapter matters to the reviewer.", - "hunkRefs": [ - { "filePath": "path/to/file.ts", "oldStart": 42 } - ], - "keyChanges": [ - { - "content": "A judgment-call question for the reviewer (not source code).", - "lineRefs": [ - { - "filePath": "path/to/file.ts", - "side": "additions", - "startLine": 50, - "endLine": 55 - } - ] - } - ] - } - ], - "prologue": { - "motivation": "What was broken or annoying, in plain English (or null).", - "outcome": "What's better now, in plain English (or null).", - "keyChanges": [ - { - "summary": "Outcome-focused summary, 6-10 words", - "description": "Capitalized sentence, 10-15 words of context" - } - ], - "focusAreas": [ - { - "type": "architecture", - "severity": "info", - "title": "3-5 word noun phrase", - "description": "WHY flagged + action for the reviewer", - "locations": ["path/to/file.ts"] - } - ], - "complexity": { - "level": "medium", - "reasoning": "Brief explanation" - } - }, - "generatedAt": "2026-05-04T12:34:56.000Z" + "chapters": [ ... ], + "prologue": { ... } } +AGENT_EOF ``` +The trailing `XXXXXX` (with no suffix after) is required by macOS BSD `mktemp`. Using `cat` with a heredoc avoids tool-specific file-writing issues. + Field rules: | Field | Constraint | |-------|------------| -| `scope.kind` | `"committed"` or `"workingTree"` | -| `scope.ref` | Required when `kind` is `"workingTree"`; one of `"work"`, `"staged"`, `"unstaged"` | -| `scope.baseSha` / `headSha` / `mergeBaseSha` | Full 40-character lowercase hex SHAs | | `chapters[].id` | Non-empty, unique within the run | | `chapters[].order` | Positive integer (1-indexed) | -| `chapters[].hunkRefs[].oldStart` | Non-negative integer — the pre-image start line from the unified-diff `@@` header (`0` for new files) | +| `chapters[].hunkRefs[].oldStart` | Non-negative integer — the pre-image start line from the `oldStart` in the formatted hunk header (`0` for new files) | | `chapters[].keyChanges[].lineRefs` | Array with at least one entry | | `lineRefs[].side` | `"additions"` (right side) or `"deletions"` (left side) | | `lineRefs[].startLine` / `endLine` | Positive integers; `endLine >= startLine` | @@ -340,16 +274,15 @@ Field rules: | `prologue.focusAreas[].type` | One of: `security`, `breaking-change`, `high-complexity`, `data-integrity`, `new-pattern`, `architecture`, `performance`, `testing-gap` | | `prologue.focusAreas[].severity` | One of: `critical`, `high`, `medium`, `info` | | `prologue.complexity.level` | One of: `low`, `medium`, `high`, `very-high` | -| `generatedAt` | ISO 8601 datetime string | ## Step 6 — Display generated chapters Hand the file to `stagereview`: ```bash -stagereview show "$TMPFILE" +stagereview show "$AGENT_OUTPUT" ``` -`stagereview show` validates the JSON, inserts a new `chapter_run` plus chapters and key changes into the local SQLite database, boots a loopback HTTP server, and opens the browser to the new run. The command stays running and serves the SPA until the user kills it with Ctrl+C — invoke it as the final command in the workflow rather than expecting it to print a value and exit. +`stagereview show` auto-detects the agent output format, independently computes the scope and "Other changes" chapter for filtered files, validates the JSON, inserts the run into the local SQLite database, boots a loopback HTTP server, and opens the browser. -Do not pass a `runId` and do not call a separate `stagereview ingest`. `show ` does ingestion and serving in one step. +**The command blocks until the user presses Ctrl+C.** If your harness requires non-blocking execution, run it in the background (e.g., `run_in_background` in Claude Code). Invoke it as the final command in the workflow.