diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 63f0814001..92e3ed89cd 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -9836,6 +9836,7 @@ async function maybePublishPrPublicSurface( additions: file.additions, deletions: file.deletions, })), + changedFilesSummaryContext: { repoFullName, pullNumber: pr.number }, } : {}), // review.effort_score (#1955): deterministic, no-AI complexity/time estimate — only computed when the diff --git a/src/review/changed-files-diff-link.ts b/src/review/changed-files-diff-link.ts new file mode 100644 index 0000000000..1ab75541de --- /dev/null +++ b/src/review/changed-files-diff-link.ts @@ -0,0 +1,24 @@ +/** GitHub PR Files-tab diff anchors for changed-files summary links (#2157). */ + +import { createHash } from "node:crypto"; + +const REPO_FULL_NAME = /^[A-Za-z0-9_.-]+\/[A-Za-z0-9_.-]+$/; + +/** PURE: SHA-256 hex of the bare repo-relative path — GitHub's `#diff-…` anchor on the Files tab. */ +export function githubPrFileDiffAnchor(path: string): string | null { + const trimmed = path.trim(); + if (!trimmed || trimmed.includes("\0")) return null; + return createHash("sha256").update(trimmed, "utf8").digest("hex"); +} + +/** PURE: public-safe PR Files-tab URL for one changed file, or null when inputs cannot be anchored. */ +export function githubPrFileDiffUrl( + repoFullName: string, + pullNumber: number, + path: string, +): string | null { + if (!REPO_FULL_NAME.test(repoFullName) || !Number.isInteger(pullNumber) || pullNumber <= 0) return null; + const anchor = githubPrFileDiffAnchor(path); + if (anchor === null) return null; + return `https://github.com/${repoFullName}/pull/${pullNumber}/files#diff-${anchor}`; +} diff --git a/src/review/unified-comment-bridge.ts b/src/review/unified-comment-bridge.ts index 8838741856..7b1126180c 100644 --- a/src/review/unified-comment-bridge.ts +++ b/src/review/unified-comment-bridge.ts @@ -29,6 +29,7 @@ import type { CaptureRoute } from "./visual/capture"; import { PR_PANEL_COMMENT_MARKER } from "../github/comments"; import { GITTENSORY_GATE_CHECK_NAME } from "./check-names"; import { classifyChangedFile, type ReviewFileClass } from "./changed-files-classify"; +import { githubPrFileDiffUrl } from "./changed-files-diff-link"; import { classifyFindingCategory, FINDING_CATEGORIES, type FindingCategory } from "./finding-category-classify"; import { buildUnifiedReviewInput, @@ -315,6 +316,8 @@ export type UnifiedCommentBridgeArgs = { * passes this only when the manifest opts in — see `resolveReviewPromptOverrides`'s `changedFilesSummary`). * (#1957) */ changedFilesSummary?: ChangedFileSummaryInput[] | undefined; + /** Repo + PR number for per-file "View diff" links in the changed-files table (#2157). */ + changedFilesSummaryContext?: ChangedFilesSummaryContext | undefined; /** Deterministic per-PR review-effort estimate (review.effort_score port, `src/review/review-effort.ts`). When * present, a compact `review effort: N/5 (~M min)` chip is appended to the status-chip row (passed straight * through to `buildUnifiedReviewInput`'s `reviewEffort`). No AI. Default OFF (the processor passes this only @@ -454,6 +457,17 @@ export function buildScrollPreviewCollapsible(routes: CaptureRoute[]): UnifiedCo * doesn't drag GitHub's full file-record shape into its pure-rendering surface. */ export type ChangedFileSummaryInput = { path: string; additions: number; deletions: number }; +/** Repo + PR coordinates for per-file "View diff" links on the changed-files table (#2157). */ +export type ChangedFilesSummaryContext = { repoFullName: string; pullNumber: number }; + +function markdownChangedFilePath(value: string): string { + return `\`${value + .replace(/\\/g, "\\\\") + .replace(/`/g, "\\`") + .replace(/\|/g, "\\|") + .replace(/[<>]/g, (char) => (char === "<" ? "<" : ">"))}\``; +} + /** Display order for the "Changed files" table — SOURCE FIRST, mirroring the same source-first priority this * codebase already applies to the AI reviewer's own diff ordering (`diffFilePriority`, * `src/review/review-diff.ts`): the code a maintainer most needs to read leads, generated/mechanical output @@ -469,14 +483,30 @@ const CHANGED_FILE_CATEGORY_LABEL: Record = { }; /** - * Build the "Changed files" collapsible: one row per file category (source/test/docs/config/generated, via - * the deterministic `classifyChangedFile`), with a file count and +/- totals — collapsing an arbitrarily large - * same-category group into a single row so a big PR doesn't turn into a wall of per-file lines. No AI, no - * network — pure grouping over data the caller already has. Returns null when there are no files (nothing to - * summarize), so the caller can unconditionally chain this alongside the other optional collapsibles. + * Build the "Changed files" collapsible. Without `context`, groups by category (source/test/docs/config/generated) + * with file counts and +/- totals — byte-identical to #2145. With `context`, renders one row per file (sorted + * source-first) and a public-safe GitHub Files-tab "View diff" link per row (#2157). */ -export function buildChangedFilesSummaryCollapsible(files: ChangedFileSummaryInput[]): UnifiedCollapsible | null { +export function buildChangedFilesSummaryCollapsible( + files: ChangedFileSummaryInput[], + context?: ChangedFilesSummaryContext | undefined, +): UnifiedCollapsible | null { if (files.length === 0) return null; + if (context) { + const sorted = [...files].sort((left, right) => { + const leftCategory = CHANGED_FILE_CATEGORY_ORDER.indexOf(classifyChangedFile(left.path)); + const rightCategory = CHANGED_FILE_CATEGORY_ORDER.indexOf(classifyChangedFile(right.path)); + if (leftCategory !== rightCategory) return leftCategory - rightCategory; + return left.path.localeCompare(right.path); + }); + const rows = sorted.map((file) => { + const diffUrl = githubPrFileDiffUrl(context.repoFullName, context.pullNumber, file.path); + const diffCell = diffUrl ? `[View diff](${diffUrl})` : "—"; + return `| ${markdownChangedFilePath(file.path)} | +${file.additions} | -${file.deletions} | ${diffCell} |`; + }); + const body = ["| File | Added | Removed | |", "| --- | --- | --- | --- |", ...rows].join("\n"); + return { title: "Changed files", body }; + } const totals = new Map(); for (const file of files) { const category = classifyChangedFile(file.path); @@ -665,7 +695,7 @@ export function buildUnifiedCommentBody(args: UnifiedCommentBridgeArgs): string // before pixels). Flag-OFF (the processor passes undefined) ⇒ extraCollapsibles is unchanged. (#1957) const changedFilesCollapsible = args.changedFilesSummary && args.changedFilesSummary.length > 0 - ? buildChangedFilesSummaryCollapsible(args.changedFilesSummary) + ? buildChangedFilesSummaryCollapsible(args.changedFilesSummary, args.changedFilesSummaryContext) : null; const withChangedFiles = changedFilesCollapsible !== null ? [...(withManifestValidation ?? []), changedFilesCollapsible] : withManifestValidation; diff --git a/test/unit/changed-files-diff-link.test.ts b/test/unit/changed-files-diff-link.test.ts new file mode 100644 index 0000000000..3a712f1915 --- /dev/null +++ b/test/unit/changed-files-diff-link.test.ts @@ -0,0 +1,31 @@ +import { describe, expect, it } from "vitest"; +import { githubPrFileDiffAnchor, githubPrFileDiffUrl } from "../../src/review/changed-files-diff-link"; + +describe("githubPrFileDiffAnchor (#2157)", () => { + it("returns the SHA-256 hex of the bare repo-relative path", () => { + expect(githubPrFileDiffAnchor("src/app.ts")).toBe( + "841254fe75488c1bd4cd7f68f00b4be0e48dcfbc4a16b45847b68295e0e3b27b", + ); + }); + + it("returns null for empty or whitespace-only paths", () => { + expect(githubPrFileDiffAnchor("")).toBeNull(); + expect(githubPrFileDiffAnchor(" ")).toBeNull(); + expect(githubPrFileDiffAnchor("\0bad")).toBeNull(); + }); +}); + +describe("githubPrFileDiffUrl (#2157)", () => { + it("builds a public-safe Files-tab URL with the diff anchor", () => { + expect(githubPrFileDiffUrl("acme/widgets", 42, "src/app.ts")).toBe( + "https://github.com/acme/widgets/pull/42/files#diff-841254fe75488c1bd4cd7f68f00b4be0e48dcfbc4a16b45847b68295e0e3b27b", + ); + }); + + it("returns null for invalid repo, PR number, or path", () => { + expect(githubPrFileDiffUrl("not-a-repo", 1, "src/a.ts")).toBeNull(); + expect(githubPrFileDiffUrl("acme/widgets", 0, "src/a.ts")).toBeNull(); + expect(githubPrFileDiffUrl("acme/widgets", 1.5, "src/a.ts")).toBeNull(); + expect(githubPrFileDiffUrl("acme/widgets", 1, "")).toBeNull(); + }); +}); diff --git a/test/unit/changed-files-summary-collapsible.test.ts b/test/unit/changed-files-summary-collapsible.test.ts index 07201579e0..aa0b2d6a07 100644 --- a/test/unit/changed-files-summary-collapsible.test.ts +++ b/test/unit/changed-files-summary-collapsible.test.ts @@ -28,17 +28,86 @@ const files: ChangedFileSummaryInput[] = [ { path: "package-lock.json", additions: 100, deletions: 50 }, ]; +describe("buildChangedFilesSummaryCollapsible per-file diff links (#2157)", () => { + const context = { repoFullName: "acme/widgets", pullNumber: 42 }; + + it("renders one row per file with a View diff link when context is provided", () => { + const c = buildChangedFilesSummaryCollapsible(files, context); + expect(c).not.toBeNull(); + expect(c?.body).toContain("| File | Added | Removed | |"); + expect(c?.body).toContain( + "| `src/app.ts` | +40 | -10 | [View diff](https://github.com/acme/widgets/pull/42/files#diff-", + ); + expect(c?.body).not.toContain("| Source | 2 | +45 | -10 |"); + }); + + it("sorts same-category files by path when context is provided", () => { + const c = buildChangedFilesSummaryCollapsible( + [ + { path: "src/z.ts", additions: 1, deletions: 0 }, + { path: "src/a.ts", additions: 2, deletions: 0 }, + ], + context, + ); + const body = c?.body ?? ""; + expect(body.indexOf("src/a.ts")).toBeLessThan(body.indexOf("src/z.ts")); + }); + + it("escapes adversarial path characters in per-file rows", () => { + const c = buildChangedFilesSummaryCollapsible( + [{ path: "src/weird\\path|`file<1>.ts", additions: 1, deletions: 0 }], + context, + ); + expect(c?.body).toContain("<1>"); + expect(c?.body).toContain("\\`"); + expect(c?.body).toContain("\\|"); + expect(c?.body).toContain("\\\\"); + }); + + it("omits the View diff link when the path or repo context cannot be anchored", () => { + const unanchored = buildChangedFilesSummaryCollapsible([{ path: " ", additions: 1, deletions: 0 }], context); + expect(unanchored?.body).toContain("| ` ` | +1 | -0 | — |"); + + const badRepo = buildChangedFilesSummaryCollapsible( + [{ path: "src/a.ts", additions: 1, deletions: 0 }], + { repoFullName: "not-a-repo", pullNumber: 1 }, + ); + expect(badRepo?.body).toContain("| `src/a.ts` | +1 | -0 | — |"); + }); + + it("orders per-file rows source-first across categories when context is provided", () => { + const c = buildChangedFilesSummaryCollapsible( + [ + { path: "docs/readme.md", additions: 1, deletions: 0 }, + { path: "src/app.ts", additions: 2, deletions: 0 }, + ], + context, + ); + const body = c?.body ?? ""; + expect(body.indexOf("src/app.ts")).toBeLessThan(body.indexOf("docs/readme.md")); + }); + + it("escapes a greater-than character in per-file paths", () => { + const c = buildChangedFilesSummaryCollapsible([{ path: "src/file>name.ts", additions: 1, deletions: 0 }], context); + expect(c?.body).toContain(">"); + }); + + it("keeps collapsed category rows without links when context is omitted", () => { + const c = buildChangedFilesSummaryCollapsible(files); + expect(c?.body).toContain("| Source | 2 | +45 | -10 |"); + expect(c?.body).not.toContain("[View diff]"); + }); +}); + describe("buildChangedFilesSummaryCollapsible (#2145)", () => { it("groups changed files by category with file counts and +/- totals", () => { const c = buildChangedFilesSummaryCollapsible(files); expect(c).not.toBeNull(); expect(c?.title).toBe("Changed files"); expect(c?.body).toContain("| Category | Files | Added | Removed |"); - // Two source files collapse into ONE row with summed totals (45 = 40 + 5, 10 = 10 + 0). expect(c?.body).toContain("| Source | 2 | +45 | -10 |"); expect(c?.body).toContain("| Test | 1 | +20 | -2 |"); expect(c?.body).toContain("| Docs | 1 | +3 | -1 |"); - // A lockfile classifies as generated. expect(c?.body).toContain("| Generated | 1 | +100 | -50 |"); }); @@ -78,7 +147,18 @@ describe("buildUnifiedCommentBody changedFilesSummary wiring (#1957 / #2145)", ( footerMarkdown: footer, }; - it("appends the Changed files section when changedFilesSummary is present + non-empty", () => { + it("appends per-file View diff links when changedFilesSummaryContext is present (#2157)", () => { + const body = buildUnifiedCommentBody({ + ...base, + changedFilesSummary: files, + changedFilesSummaryContext: { repoFullName: "acme/widgets", pullNumber: 42 }, + }); + expect(body).toContain("Changed files"); + expect(body).toContain("[View diff](https://github.com/acme/widgets/pull/42/files#diff-"); + expect(body).not.toContain("| Source | 2 | +45 | -10 |"); + }); + + it("appends the grouped Changed files section when changedFilesSummary is present without context (#2145)", () => { const body = buildUnifiedCommentBody({ ...base, changedFilesSummary: files }); expect(body).toContain("Changed files"); expect(body).toContain("| Source | 2 | +45 | -10 |"); diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index f61042d03d..4eaa7612a0 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -16974,11 +16974,10 @@ describe("queue processors", () => { expect(calls.comments).toBe(2); expect(postedBody).toContain(""); - // The new deterministic, no-AI collapsible — one row per category, collapsing the source file and the - // doc file into their own rows with the mocked +/- totals. + // The deterministic changed-files collapsible — per-file rows with GitHub Files-tab links (#2157). expect(postedBody).toContain("Changed files"); - expect(postedBody).toContain("| Source | 1 | +5 | -1 |"); - expect(postedBody).toContain("| Docs | 1 | +2 | -0 |"); + expect(postedBody).toContain("| `src/cache.ts` | +5 | -1 | [View diff](https://github.com/JSONbored/gittensory/pull/3/files#diff-"); + expect(postedBody).toContain("| `README.md` | +2 | -0 | [View diff](https://github.com/JSONbored/gittensory/pull/3/files#diff-"); } finally { liveCiSpy.mockRestore(); }