Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/queue/processors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
24 changes: 24 additions & 0 deletions src/review/changed-files-diff-link.ts
Original file line number Diff line number Diff line change
@@ -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}`;
}
44 changes: 37 additions & 7 deletions src/review/unified-comment-bridge.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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 === "<" ? "&lt;" : "&gt;"))}\``;
}

/** 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
Expand All @@ -469,14 +483,30 @@ const CHANGED_FILE_CATEGORY_LABEL: Record<ReviewFileClass, string> = {
};

/**
* 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<ReviewFileClass, { count: number; additions: number; deletions: number }>();
for (const file of files) {
const category = classifyChangedFile(file.path);
Expand Down Expand Up @@ -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;
Expand Down
31 changes: 31 additions & 0 deletions test/unit/changed-files-diff-link.test.ts
Original file line number Diff line number Diff line change
@@ -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();
});
});
86 changes: 83 additions & 3 deletions test/unit/changed-files-summary-collapsible.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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("&lt;1&gt;");
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("&gt;");
});

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 |");
});

Expand Down Expand Up @@ -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 |");
Expand Down
7 changes: 3 additions & 4 deletions test/unit/queue.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16974,11 +16974,10 @@ describe("queue processors", () => {

expect(calls.comments).toBe(2);
expect(postedBody).toContain("<!-- gittensory-pr-panel:v1 -->");
// 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();
}
Expand Down