From a549cd24b91ad1dbf7585179e5f1a52a573411c2 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Sun, 5 Jul 2026 11:54:39 -0700 Subject: [PATCH] feat(review): render a deterministic changed-files summary in the unified comment (#1957) Groups changed files by category (source/test/docs/config/generated via the existing classifyChangedFile classifier) into one collapsible table with per-category file counts and +/- totals -- no AI, no extra fetch. Gated by review.changed_files_summary in .gittensory.yml (default off, manifest-only like review.suggestions/review.tone) on top of the existing unifiedComment convergence flag, so it's configurable per-repo without any new global env var or DB/OpenAPI surface. --- README.md | 2 +- src/queue/processors.ts | 17 ++ src/review/unified-comment-bridge.ts | 67 ++++++- src/signals/focus-manifest.ts | 22 ++- .../changed-files-summary-collapsible.test.ts | 116 ++++++++++++ test/unit/focus-manifest.test.ts | 29 ++- test/unit/queue.test.ts | 174 ++++++++++++++++++ test/unit/signals-coverage.test.ts | 2 +- 8 files changed, 416 insertions(+), 13 deletions(-) create mode 100644 test/unit/changed-files-summary-collapsible.test.ts diff --git a/README.md b/README.md index 7ebab03cbd..cb56e9ac86 100644 --- a/README.md +++ b/README.md @@ -34,7 +34,7 @@ Gittensory CI and gittensory review score, gate, and comment on pull requests. T - **CI + full-file grounding** — grounds the AI reviewer with the PR's finished CI status and the full post-change content of the changed files, so claims are verified against reality instead of predicted. - **Codebase RAG** — retrieval-augmented context that queries the codebase vector index for related callers, modules, and conventions and appends them to the reviewer prompt (additive only; inert until an index exists). - **Submitter-reputation gating** — an internal-only spend control that downgrades new / burst / low-reputation submitters to a deterministic-only review, never surfaced on any public comment, label, or check. -- **Unified review comment** — renders the public PR feedback as one in-place comment instead of multiple panels. +- **Unified review comment** — renders the public PR feedback as one in-place comment instead of multiple panels. With `.gittensory.yml`'s `review.changed_files_summary` also on (off by default), it gains a deterministic, no-AI "Changed files" collapsible: one row per file category (source/test/docs/config/generated), with file counts and +/- totals. - **Per-repo activation** — capabilities roll forward (and back) one flag and one repo at a time via the `GITTENSORY_REVIEW_REPOS` allowlist. **Check-run and comment surfaces, disambiguated** (a common point of confusion — these are three independent, separately-configured things, not layers of the same feature): diff --git a/src/queue/processors.ts b/src/queue/processors.ts index db50f5b25e..c56688becd 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -7639,6 +7639,7 @@ async function maybePublishPrPublicSurface( | undefined; let inlineCommentsEnabledForReview = false; let suggestionsEnabledForReview = false; + let changedFilesSummaryEnabledForReview = false; let aiReviewExpected = false; let aiReviewWasReused = false; let gateFinalized = false; @@ -8104,6 +8105,13 @@ async function maybePublishPrPublicSurface( deliveryId: webhook.deliveryId, headSha: advisory.headSha ?? null, })); + // review.changed_files_summary (#1957): deterministic, no-AI — resolve it here, UNCONDITIONALLY, rather than + // inside the aiReviewWillRun-gated closure below. This table must still render whenever the manifest opts + // in even when the AI review itself is skipped this pass (author blacklisted, frozen for manual review, or + // AI review disabled for the repo) — it has nothing to do with the AI pipeline. Captured into the + // outer-scoped `changedFilesSummaryEnabledForReview` (mirroring inlineCommentsEnabledForReview/ + // suggestionsEnabledForReview) so it survives past this try block to the publish step below. + changedFilesSummaryEnabledForReview = resolveReviewPromptOverrides(reviewManifestForAutoReview).changedFilesSummary; const aiReviewWillRun = !authorBlacklisted && !isFrozenForManualReview && @@ -9294,6 +9302,15 @@ async function maybePublishPrPublicSurface( }), reRunLabel: `${PR_PANEL_RETRIGGER_MARKER} Re-run Gittensory review`, ...(beforeAfter.length > 0 ? { beforeAfter } : {}), + ...(changedFilesSummaryEnabledForReview + ? { + changedFilesSummary: unifiedFiles.map((file) => ({ + path: file.path, + additions: file.additions, + deletions: file.deletions, + })), + } + : {}), }); } else { deterministicBody = buildPublicPrIntelligenceComment(commentArgs); diff --git a/src/review/unified-comment-bridge.ts b/src/review/unified-comment-bridge.ts index f18ab44fc5..162613de2c 100644 --- a/src/review/unified-comment-bridge.ts +++ b/src/review/unified-comment-bridge.ts @@ -27,6 +27,7 @@ import type { CaptureRoute } from "./visual/capture"; // verbatim or `createOrUpdatePrIntelligenceComment` posts a DUPLICATE instead of updating in place. 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 { buildUnifiedReviewInput, renderUnifiedReviewComment, @@ -306,6 +307,12 @@ export type UnifiedCommentBridgeArgs = { * Public-safe: only URLs + route paths — no private terms. Default OFF (the processor passes this only * when screenshotsAllowed + the PR touches web-visible files). */ beforeAfter?: CaptureRoute[] | undefined; + /** Changed-file path + additions/deletions, one entry per file (review.changed_files_summary port). When + * present + non-empty, a "Changed files" collapsible (one row per source/test/docs/config/generated + * category, with file counts and +/- totals) is appended. Deterministic, no AI. Default OFF (the processor + * passes this only when the manifest opts in — see `resolveReviewPromptOverrides`'s `changedFilesSummary`). + * (#1957) */ + changedFilesSummary?: ChangedFileSummaryInput[] | undefined; /** The disposition holds this PR for owner review because its diff touches a hard-guardrail path — so an * otherwise-ready comment renders "held for review" instead of "safe to merge". (#guarded-hold-comment) */ heldForReview?: boolean | undefined; @@ -361,6 +368,55 @@ export function buildBeforeAfterCollapsible(routes: CaptureRoute[]): UnifiedColl return { title: "Visual preview", body, rawHtml: true }; } +/** A changed file's path + line deltas — everything `buildChangedFilesSummaryCollapsible` needs to group and + * total. Deliberately narrower than `PullRequestFileRecord` (path/additions/deletions only) so the bridge + * doesn't drag GitHub's full file-record shape into its pure-rendering surface. */ +export type ChangedFileSummaryInput = { path: string; additions: number; deletions: number }; + +/** 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 + * trails. A category absent from the PR's changed files is simply omitted (no zero rows). */ +const CHANGED_FILE_CATEGORY_ORDER: ReviewFileClass[] = ["source", "test", "docs", "config", "generated"]; + +const CHANGED_FILE_CATEGORY_LABEL: Record = { + source: "Source", + test: "Test", + docs: "Docs", + config: "Config", + generated: "Generated", +}; + +/** + * 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. + */ +export function buildChangedFilesSummaryCollapsible(files: ChangedFileSummaryInput[]): UnifiedCollapsible | null { + if (files.length === 0) return null; + const totals = new Map(); + for (const file of files) { + const category = classifyChangedFile(file.path); + const entry = totals.get(category); + if (entry) { + entry.count += 1; + entry.additions += file.additions; + entry.deletions += file.deletions; + } else { + totals.set(category, { count: 1, additions: file.additions, deletions: file.deletions }); + } + } + const rows = CHANGED_FILE_CATEGORY_ORDER.flatMap((category) => { + const entry = totals.get(category); + if (!entry) return []; + return [`| ${CHANGED_FILE_CATEGORY_LABEL[category]} | ${entry.count} | +${entry.additions} | -${entry.deletions} |`]; + }); + const body = ["| Category | Files | Added | Removed |", "| --- | --- | --- | --- |", ...rows].join("\n"); + return { title: "Changed files", body }; +} + /** * Build the unified PR-review comment body from gittensory's live data. Returns a string that STARTS with * the panel marker (so the existing upsert updates in place) followed by the rendered unified comment. @@ -414,11 +470,20 @@ export function buildUnifiedCommentBody(args: UnifiedCommentBridgeArgs): string const visibleRows = args.panelRows.filter((row) => args.reviewFields?.[row.key] !== false); const signals = panelRowsToSignalRows(visibleRows); + // review.changed_files_summary port: when the manifest opts in, the processor hands us every changed file's + // path + deltas here; append the grouped "Changed files" collapsible ahead of the visual preview (structure + // before pixels). Flag-OFF (the processor passes undefined) ⇒ extraCollapsibles is unchanged. (#1957) + const changedFilesCollapsible = + args.changedFilesSummary && args.changedFilesSummary.length > 0 + ? buildChangedFilesSummaryCollapsible(args.changedFilesSummary) + : null; + const withChangedFiles = + changedFilesCollapsible !== null ? [...(args.extraCollapsibles ?? []), changedFilesCollapsible] : args.extraCollapsibles; // Visual-capture port: when before/after routes are present, append a "Visual preview" collapsible to the // extra sections. Flag-OFF (the processor passes no beforeAfter) ⇒ extraCollapsibles is unchanged. const visualCollapsible = args.beforeAfter && args.beforeAfter.length > 0 ? buildBeforeAfterCollapsible(args.beforeAfter) : null; const extraCollapsibles = - visualCollapsible !== null ? [...(args.extraCollapsibles ?? []), visualCollapsible] : args.extraCollapsibles; + visualCollapsible !== null ? [...(withChangedFiles ?? []), visualCollapsible] : withChangedFiles; const body = renderUnifiedReviewComment(input, { brand: args.brand ?? "Gittensory review", diff --git a/src/signals/focus-manifest.ts b/src/signals/focus-manifest.ts index 1a168ba280..e9b86655d2 100644 --- a/src/signals/focus-manifest.ts +++ b/src/signals/focus-manifest.ts @@ -322,6 +322,12 @@ export type FocusManifestReviewConfig = { * otherwise) — this is an ADDITIONAL opt-in on top of `review.inline_comments`, not a replacement gate. * null/false (default, absent) = no suggestion blocks = byte-identical behavior. (#1956) */ suggestions: boolean | null; + /** `review.changed_files_summary`: when true, the unified review comment (only rendered at all when the + * `unifiedComment` convergence feature is on) gains a deterministic, no-AI "Changed files" collapsible: one + * row per file category (source/test/docs/config/generated), with file counts and +/- totals, via the + * existing `classifyChangedFile` classifier (`src/review/changed-files-classify.ts`, built for this table + * under #2143). null/false (default, absent) = no changed-files section = byte-identical behavior. (#1957) */ + changedFilesSummary: boolean | null; /** `review.path_instructions`: per-path natural-language guidance handed to the AI reviewer when the PR's * changed files match the glob. Empty (default) ⇒ byte-identical reviewer prompt. (#review-path-instructions) */ pathInstructions: ReviewPathInstruction[]; @@ -574,7 +580,7 @@ const EMPTY_MANIFEST: FocusManifest = { publicNotes: [], gate: { ...EMPTY_GATE_CONFIG }, settings: {}, - review: { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, suggestions: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG }, labelingRules: [], aiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG } }, + review: { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, suggestions: null, changedFilesSummary: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG }, labelingRules: [], aiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG } }, features: { ...EMPTY_FEATURES_CONFIG }, contentLane: { ...EMPTY_CONTENT_LANE_CONFIG }, repoDocGeneration: { ...EMPTY_REPO_DOC_GENERATION_CONFIG }, @@ -604,7 +610,7 @@ function emptyManifest(source: FocusManifestSource, warnings: string[] = []): Fo warnings, gate: { ...EMPTY_GATE_CONFIG }, settings: {}, - review: { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, suggestions: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG }, labelingRules: [], aiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG } }, + review: { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, suggestions: null, changedFilesSummary: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG }, labelingRules: [], aiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG } }, features: { ...EMPTY_FEATURES_CONFIG }, contentLane: { ...EMPTY_CONTENT_LANE_CONFIG }, repoDocGeneration: { ...EMPTY_REPO_DOC_GENERATION_CONFIG }, @@ -1535,7 +1541,7 @@ function parsePublicSafeText(value: JsonValue | undefined, field: string, warnin * throws; invalid/unsafe values are dropped with warnings. */ function parseReviewConfig(value: JsonValue | undefined, warnings: string[]): FocusManifestReviewConfig { - const empty: FocusManifestReviewConfig = { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, suggestions: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG }, labelingRules: [], aiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG } }; + const empty: FocusManifestReviewConfig = { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, suggestions: null, changedFilesSummary: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG }, labelingRules: [], aiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG } }; if (value === undefined || value === null) return empty; if (typeof value !== "object" || Array.isArray(value)) { warnings.push(`Manifest field "review" must be a mapping; ignoring it.`); @@ -1573,6 +1579,7 @@ function parseReviewConfig(value: JsonValue | undefined, warnings: string[]): Fo const securityFocus = normalizeOptionalBoolean(r.security_focus, "review.security_focus", warnings); const inlineComments = normalizeOptionalBoolean(r.inline_comments, "review.inline_comments", warnings); const suggestions = normalizeOptionalBoolean(r.suggestions, "review.suggestions", warnings); + const changedFilesSummary = normalizeOptionalBoolean(r.changed_files_summary, "review.changed_files_summary", warnings); const pathInstructions = parseReviewPathInstructions(r.path_instructions, warnings); const instructions = parsePublicSafeText(r.instructions, "review.instructions", warnings); const excludePaths = parseReviewExcludePaths(r.exclude_paths, warnings); @@ -1590,6 +1597,7 @@ function parseReviewConfig(value: JsonValue | undefined, warnings: string[]): Fo securityFocus !== null || inlineComments !== null || suggestions !== null || + changedFilesSummary !== null || pathInstructions.length > 0 || instructions !== null || excludePaths.length > 0 || @@ -1611,6 +1619,7 @@ function parseReviewConfig(value: JsonValue | undefined, warnings: string[]): Fo securityFocus, inlineComments, suggestions, + changedFilesSummary, pathInstructions, instructions, excludePaths, @@ -1929,6 +1938,7 @@ export function reviewConfigToJson(review: FocusManifestReviewConfig): JsonValue if (review.securityFocus !== null) out.security_focus = review.securityFocus; if (review.inlineComments !== null) out.inline_comments = review.inlineComments; if (review.suggestions !== null) out.suggestions = review.suggestions; + if (review.changedFilesSummary !== null) out.changed_files_summary = review.changedFilesSummary; if (review.instructions !== null) out.instructions = review.instructions; if (review.pathInstructions.length > 0) out.path_instructions = review.pathInstructions.map((entry) => ({ path: entry.path, instructions: entry.instructions })); if (review.excludePaths.length > 0) out.exclude_paths = [...review.excludePaths]; @@ -2067,13 +2077,15 @@ export function composeManifestReviewInstructions(instructions: string | null, t * failure). A null manifest yields the byte-identical defaults. Centralized so the AI-review caller threads them * in one place with the null-manifest branch covered here (unit-tested) rather than inline in the processor. * (#review-profile / #review-tone / #review-security-focus / #review-path-instructions / #review-exclude-paths / #2043 / #selfhost-ai-model-override / #1956) */ -export function resolveReviewPromptOverrides(manifest: FocusManifest | null): { profile: ReviewProfile | null; tone: string | null; securityFocus: boolean; inlineComments: boolean; suggestions: boolean; pathInstructions: ReviewPathInstruction[]; instructions: string | null; excludePaths: string[]; pathFilters: string[]; selfHostAiModel: SelfHostAiModelConfig } { +export function resolveReviewPromptOverrides(manifest: FocusManifest | null): { profile: ReviewProfile | null; tone: string | null; securityFocus: boolean; inlineComments: boolean; suggestions: boolean; changedFilesSummary: boolean; pathInstructions: ReviewPathInstruction[]; instructions: string | null; excludePaths: string[]; pathFilters: string[]; selfHostAiModel: SelfHostAiModelConfig } { // inlineComments resolves to a strict boolean — true ONLY when the manifest explicitly set review.inline_comments: // true; null/false/absent ⇒ false. The caller ANDs this per-repo toggle with the operator flag + cutover allowlist. // securityFocus resolves the same way — true ONLY when the manifest explicitly set review.security_focus: true. // suggestions resolves the same way (#1956) — the caller further ANDs it with the already-resolved // inlineComments gate, since a suggestion has nothing to attach to without an inline comment. - return { profile: manifest?.review.profile ?? null, tone: manifest?.review.tone ?? null, securityFocus: manifest?.review.securityFocus === true, inlineComments: manifest?.review.inlineComments === true, suggestions: manifest?.review.suggestions === true, pathInstructions: manifest?.review.pathInstructions ?? [], instructions: manifest?.review.instructions ?? null, excludePaths: manifest?.review.excludePaths ?? [], pathFilters: manifest?.review.pathFilters ?? [], selfHostAiModel: resolveReviewSelfHostAiModel(manifest) }; + // changedFilesSummary resolves the same way (#1957) — independent of inlineComments/suggestions; it only + // needs the unified-comment convergence feature itself to be on (the caller's own outer gate). + return { profile: manifest?.review.profile ?? null, tone: manifest?.review.tone ?? null, securityFocus: manifest?.review.securityFocus === true, inlineComments: manifest?.review.inlineComments === true, suggestions: manifest?.review.suggestions === true, changedFilesSummary: manifest?.review.changedFilesSummary === true, pathInstructions: manifest?.review.pathInstructions ?? [], instructions: manifest?.review.instructions ?? null, excludePaths: manifest?.review.excludePaths ?? [], pathFilters: manifest?.review.pathFilters ?? [], selfHostAiModel: resolveReviewSelfHostAiModel(manifest) }; } /** Resolve `review.pre_merge_checks` from a possibly-null manifest (null = load failure ⇒ no checks). Centralized diff --git a/test/unit/changed-files-summary-collapsible.test.ts b/test/unit/changed-files-summary-collapsible.test.ts new file mode 100644 index 0000000000..b752097efc --- /dev/null +++ b/test/unit/changed-files-summary-collapsible.test.ts @@ -0,0 +1,116 @@ +import { describe, expect, it } from "vitest"; +import { buildChangedFilesSummaryCollapsible, buildUnifiedCommentBody, type ChangedFileSummaryInput } from "../../src/review/unified-comment-bridge"; +import type { GateCheckEvaluation } from "../../src/rules/advisory"; +import type { PublicPrPanelSignalRow } from "../../src/signals/engine"; + +function gate(over: Partial = {}): GateCheckEvaluation { + return { + enabled: true, + conclusion: "success", + title: "Gittensory Orb Review Agent passed", + summary: "No configured hard blocker was found.", + blockers: [], + warnings: [], + ...over, + }; +} + +const panelRows: PublicPrPanelSignalRow[] = [ + { key: "gateResult", cells: ["Gate result", "✅ Passing", "No configured blocker found.", "No action."] }, +]; +const footer = "💰 Earn for open-source contributions. Checked by Gittensory."; + +const files: ChangedFileSummaryInput[] = [ + { path: "src/app.ts", additions: 40, deletions: 10 }, + { path: "src/util.ts", additions: 5, deletions: 0 }, + { path: "test/unit/app.test.ts", additions: 20, deletions: 2 }, + { path: "docs/guide.md", additions: 3, deletions: 1 }, + { path: "package-lock.json", additions: 100, deletions: 50 }, +]; + +describe("buildChangedFilesSummaryCollapsible", () => { + 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 |"); + }); + + it("orders rows source-first, generated-last, regardless of input order", () => { + const c = buildChangedFilesSummaryCollapsible([...files].reverse()); + const body = c?.body ?? ""; + const order = ["| Source", "| Test", "| Docs", "| Generated"].map((marker) => body.indexOf(marker)); + expect(order).toEqual([...order].sort((a, b) => a - b)); + order.forEach((index) => expect(index).toBeGreaterThan(-1)); + }); + + it("omits a category with no changed files (no zero rows)", () => { + const c = buildChangedFilesSummaryCollapsible([{ path: "src/app.ts", additions: 1, deletions: 1 }]); + expect(c?.body).toContain("| Source | 1 | +1 | -1 |"); + expect(c?.body).not.toContain("Test"); + expect(c?.body).not.toContain("Docs"); + expect(c?.body).not.toContain("Config"); + expect(c?.body).not.toContain("Generated"); + }); + + it("returns null for an empty file list (no empty table)", () => { + expect(buildChangedFilesSummaryCollapsible([])).toBeNull(); + }); + + it("is not marked as raw HTML (plain markdown table)", () => { + const c = buildChangedFilesSummaryCollapsible(files); + expect(c?.rawHtml).toBeUndefined(); + }); +}); + +describe("buildUnifiedCommentBody changedFilesSummary wiring (#1957)", () => { + const base = { + gate: gate(), + panelRows, + readinessTotal: 90, + changedFiles: 3, + footerMarkdown: footer, + }; + + it("appends the Changed files section when changedFilesSummary is present + non-empty", () => { + const body = buildUnifiedCommentBody({ ...base, changedFilesSummary: files }); + expect(body).toContain("Changed files"); + expect(body).toContain("| Source | 2 | +45 | -10 |"); + }); + + it("does NOT add a Changed files section when changedFilesSummary is absent (flag-OFF parity)", () => { + const body = buildUnifiedCommentBody(base); + expect(body).not.toContain("Changed files"); + }); + + it("does NOT add a Changed files section when changedFilesSummary is empty", () => { + const body = buildUnifiedCommentBody({ ...base, changedFilesSummary: [] }); + expect(body).not.toContain("Changed files"); + }); + + it("preserves pre-existing extraCollapsibles alongside the Changed files section", () => { + const body = buildUnifiedCommentBody({ + ...base, + extraCollapsibles: [{ title: "Signal definitions", body: "what each row means" }], + changedFilesSummary: files, + }); + expect(body).toContain("Signal definitions"); + expect(body).toContain("Changed files"); + }); + + it("coexists with the Visual preview section (both collapsibles render)", () => { + const body = buildUnifiedCommentBody({ + ...base, + changedFilesSummary: files, + beforeAfter: [{ path: "/", afterUrl: "https://api.example.dev/gittensory/shot?key=gittensory/shots/x.png" }], + }); + expect(body).toContain("Changed files"); + expect(body).toContain("Visual preview"); + }); +}); diff --git a/test/unit/focus-manifest.test.ts b/test/unit/focus-manifest.test.ts index 94ed003595..b55a0a66a2 100644 --- a/test/unit/focus-manifest.test.ts +++ b/test/unit/focus-manifest.test.ts @@ -552,7 +552,7 @@ describe("compileFocusManifestPolicy", () => { publicNotes: ["Keep PRs focused.", "Maximize your reward payout"], gate: { present: false, enabled: null, checkMode: null, pack: null, linkedIssue: null, duplicates: null, readinessMode: null, readinessMinScore: null, slopMode: null, slopMinScore: null, slopAiAdvisory: null, sizeMode: null, lockfileIntegrityMode: null, aiReviewMode: null, aiReviewByok: null, aiReviewProvider: null, aiReviewModel: null, aiReviewAllAuthors: null, aiReviewCloseConfidence: null, aiReviewCombine: null, aiReviewOnMerge: null, aiReviewReviewers: null, mergeReadiness: null, selfAuthoredLinkedIssue: null, manifestPolicy: null, dryRun: null, firstTimeContributorGrace: null, premergeContentRecheck: null, requireFreshRebaseWindowMinutes: null, claMode: null, claConsentPhrase: null, claCheckRunName: null, claCheckRunAppSlug: null, expectedCiContexts: null }, settings: {}, - review: { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, suggestions: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG }, labelingRules: [], aiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG } }, + review: { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, suggestions: null, changedFilesSummary: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG }, labelingRules: [], aiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG } }, features: { present: false, rag: null, reputation: null, unifiedComment: null, safety: null }, contentLane: { present: false, entryFileGlob: null, providerFileGlob: null, artifactGlob: null, collectionField: null, maxAppendedEntries: null, duplicateKeyFields: [], validatorId: null }, repoDocGeneration: { present: false, enabled: false, scope: ["agents"], allowOverwriteExisting: false, refreshIntervalDays: 7 }, @@ -2656,15 +2656,17 @@ describe("resolveReviewPathInstructions (#review-path-instructions)", () => { }); it("resolveReviewPromptOverrides: non-null manifest passes the config through; null manifest → defaults", () => { - const manifest = parseFocusManifest({ review: { profile: "chill", security_focus: true, inline_comments: true, suggestions: true, path_instructions: [{ path: "src/**", instructions: "be strict" }], instructions: "Follow our async-error conventions.", exclude_paths: ["**/*.lock"], path_filters: ["src/**", "!src/generated/**"] } }); - expect(resolveReviewPromptOverrides(manifest)).toEqual({ profile: "chill", tone: null, securityFocus: true, inlineComments: true, suggestions: true, pathInstructions: [{ path: "src/**", instructions: "be strict" }], instructions: "Follow our async-error conventions.", excludePaths: ["**/*.lock"], pathFilters: ["src/**", "!src/generated/**"], selfHostAiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG } }); - // A null manifest (load failure) yields the byte-identical defaults; inline comments + suggestions + security focus default OFF. - expect(resolveReviewPromptOverrides(null)).toEqual({ profile: null, tone: null, securityFocus: false, inlineComments: false, suggestions: false, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], selfHostAiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG } }); + const manifest = parseFocusManifest({ review: { profile: "chill", security_focus: true, inline_comments: true, suggestions: true, changed_files_summary: true, path_instructions: [{ path: "src/**", instructions: "be strict" }], instructions: "Follow our async-error conventions.", exclude_paths: ["**/*.lock"], path_filters: ["src/**", "!src/generated/**"] } }); + expect(resolveReviewPromptOverrides(manifest)).toEqual({ profile: "chill", tone: null, securityFocus: true, inlineComments: true, suggestions: true, changedFilesSummary: true, pathInstructions: [{ path: "src/**", instructions: "be strict" }], instructions: "Follow our async-error conventions.", excludePaths: ["**/*.lock"], pathFilters: ["src/**", "!src/generated/**"], selfHostAiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG } }); + // A null manifest (load failure) yields the byte-identical defaults; inline comments + suggestions + changed-files summary + security focus default OFF. + expect(resolveReviewPromptOverrides(null)).toEqual({ profile: null, tone: null, securityFocus: false, inlineComments: false, suggestions: false, changedFilesSummary: false, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], selfHostAiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG } }); // An explicit false / absent toggle both resolve to the strict-boolean false. expect(resolveReviewPromptOverrides(parseFocusManifest({ review: { inline_comments: false } })).inlineComments).toBe(false); expect(resolveReviewPromptOverrides(parseFocusManifest({ review: { profile: "chill" } })).inlineComments).toBe(false); expect(resolveReviewPromptOverrides(parseFocusManifest({ review: { suggestions: false } })).suggestions).toBe(false); expect(resolveReviewPromptOverrides(parseFocusManifest({ review: { profile: "chill" } })).suggestions).toBe(false); + expect(resolveReviewPromptOverrides(parseFocusManifest({ review: { changed_files_summary: false } })).changedFilesSummary).toBe(false); + expect(resolveReviewPromptOverrides(parseFocusManifest({ review: { profile: "chill" } })).changedFilesSummary).toBe(false); expect(resolveReviewPromptOverrides(parseFocusManifest({ review: { security_focus: false } })).securityFocus).toBe(false); expect(resolveReviewPromptOverrides(parseFocusManifest({ review: { profile: "chill" } })).securityFocus).toBe(false); }); @@ -2702,6 +2704,23 @@ describe("resolveReviewPathInstructions (#review-path-instructions)", () => { expect(bad.review.suggestions).toBeNull(); expect(bad.warnings.some((w) => /review\.suggestions.*must be a boolean/.test(w))).toBe(true); }); + + it("parses review.changed_files_summary (default OFF), marks present, round-trips, and warns on a non-boolean (#1957)", () => { + expect(parseFocusManifest({ review: { changed_files_summary: true } }).review.changedFilesSummary).toBe(true); + const on = parseFocusManifest({ review: { changed_files_summary: true } }); + expect(on.review.present).toBe(true); // a changed-files-summary-only manifest IS present + expect(parseFocusManifest({ review: reviewConfigToJson(on.review) }).review).toEqual(on.review); // survives round-trip + // Explicit false is retained (and marks present, since the maintainer set it). + const off = parseFocusManifest({ review: { changed_files_summary: false } }); + expect(off.review.changedFilesSummary).toBe(false); + expect(off.review.present).toBe(true); + // Absent ⇒ null (the byte-identical default), config not present. + expect(parseFocusManifest({ review: {} }).review.changedFilesSummary).toBeNull(); + // A non-boolean is ignored with a warning. + const bad = parseFocusManifest({ review: { changed_files_summary: "yes" } }); + expect(bad.review.changedFilesSummary).toBeNull(); + expect(bad.warnings.some((w) => /review\.changed_files_summary.*must be a boolean/.test(w))).toBe(true); + }); }); describe("review.exclude_paths (#review-exclude-paths)", () => { diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index a4fb718dd7..085f5d24ff 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -15506,6 +15506,180 @@ describe("queue processors", () => { } }); + // #1957: with the unified comment on AND `.gittensory.yml` opting into `review.changed_files_summary`, the + // rendered comment gains the deterministic "Changed files" collapsible built from the SAME PR-files fetch the + // unified branch already does for the readiness chip — no separate call, no AI. Mirrors the base unified-comment + // test above but adds the manifest opt-in and asserts the new section's presence + content. + it("renders the Changed files summary when review.changed_files_summary is on in .gittensory.yml", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), GITTENSORY_REVIEW_UNIFIED_COMMENT: "1" }); + await persistRegistrySnapshot( + env, + normalizeRegistryPayload( + { "JSONbored/gittensory": { emission_share: 0.01, issue_discovery_share: 0 } }, + { kind: "raw-github", url: "https://example.test" }, + "2026-05-23T00:00:00.000Z", + ), + ); + await upsertRepositorySettings(env, { + repoFullName: "JSONbored/gittensory", + commentMode: "detected_contributors_only", + publicAudienceMode: "gittensor_only", + publicSignalLevel: "standard", + publicSurface: "comment_and_label", + autoLabelEnabled: false, + checkRunMode: "off", + checkRunDetailLevel: "minimal", + gateCheckMode: "enabled", + backfillEnabled: true, + privateTrustEnabled: true, + autonomy: { update_branch: "auto" }, + }); + let postedBody = ""; + const calls = { comments: 0, gateChecks: 0 }; + let gateFinalized = false; + let failedPostGateMint = false; + const liveCiSpy = vi + .spyOn(backfillModule, "fetchLiveCiAggregatePreferGraphQl") + .mockRejectedValueOnce(new Error("transient CI read failed")) + .mockResolvedValue({ + ciState: "passed", + hasPending: false, + hasVisiblePending: false, + hasMissingRequiredContext: false, + failingDetails: [], + nonRequiredFailingDetails: [], + ciCompletenessWarning: null, + }); + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + if (url === "https://api.gittensor.io/miners") { + return Response.json([ + { + uid: 7, + githubUsername: "oktofeesh1", + githubId: "123", + totalPrs: 4, + totalMergedPrs: 3, + totalOpenPrs: 1, + totalClosedPrs: 0, + totalOpenIssues: 0, + totalClosedIssues: 0, + totalSolvedIssues: 0, + totalValidSolvedIssues: 0, + isEligible: true, + credibility: 1, + eligibleRepoCount: 1, + hotkey: "must-not-leak", + }, + ]); + } + if (url === "https://api.gittensor.io/miners/123") { + return Response.json({ + repositories: [ + { + repositoryFullName: "JSONbored/gittensory", + totalPrs: "4", + totalMergedPrs: "3", + totalOpenPrs: "1", + totalClosedPrs: "0", + totalOpenIssues: "0", + totalClosedIssues: "0", + isEligible: true, + credibility: "1.000000", + }, + ], + }); + } + if (url === "https://api.gittensor.io/miners/123/prs") return Response.json([]); + if (url === "https://mirror.gittensor.io/api/v1/miners/123/issues") return Response.json({ issues: [] }); + if (url.endsWith("/users/oktofeesh1")) return Response.json({ login: "oktofeesh1", public_repos: 2, followers: 1 }); + if (url.includes("/users/oktofeesh1/repos")) return Response.json([{ language: "TypeScript" }]); + // .gittensory.yml opts into the deterministic changed-files summary — no AI involved. + if (url === "https://raw.githubusercontent.com/JSONbored/gittensory/HEAD/.gittensory.yml") { + return new Response("review:\n changed_files_summary: true\n"); + } + if (url.includes("/access_tokens")) { + if (gateFinalized && !failedPostGateMint) { + failedPostGateMint = true; + return new Response("mint failed", { status: 500 }); + } + return Response.json({ token: "installation-token", expires_at: "2026-05-28T00:04:00.000Z" }); + } + // PR files — the unified branch (re)fetches them to count changed files AND (with the toggle above) to + // build the "Changed files" summary. A doc + a source file so the summary shows 2 distinct category rows. + if (url.includes("/pulls/3/files")) + return Response.json([ + { filename: "src/cache.ts", additions: 5, deletions: 1, status: "modified" }, + { filename: "README.md", additions: 2, deletions: 0, status: "modified" }, + ]); + if (/\/pulls\/3(?:\?|$)/.test(url)) return Response.json({ number: 3, mergeable_state: "clean" }); + // Gate check-run — must succeed so `gateEvaluation` is produced and the flag-ON branch runs. + // The pending check is POSTed (in_progress), then PATCHed to its completed conclusion. + if (url.includes("/check-runs") && method === "GET") return Response.json({ total_count: 0, check_runs: [] }); + if (url.includes("/check-runs") && method === "POST") { + calls.gateChecks += 1; + const body = JSON.parse(String(init?.body ?? "{}")) as { status?: string; conclusion?: string }; + if (body.status !== "in_progress" || body.conclusion) { + gateFinalized = true; + clearInstallationTokenCacheForTest(); + } + return Response.json({ id: 901 }, { status: 201 }); + } + if (url.includes("/check-runs/901") && method === "PATCH") { + calls.gateChecks += 1; + gateFinalized = true; + clearInstallationTokenCacheForTest(); + return Response.json({ id: 901 }); + } + if (url.includes("/issues/3/comments") && method === "GET") return Response.json([]); + if (url.includes("/issues/3/comments") && method === "POST") { + calls.comments += 1; + postedBody = String((JSON.parse(String(init?.body ?? "{}")) as { body?: string }).body ?? ""); + return Response.json({ id: 1, html_url: "https://github.com/comment/1" }, { status: 201 }); + } + return new Response("not found", { status: 404 }); + }); + + try { + await processJob(env, { + type: "github-webhook", + deliveryId: "pr-unified-comment-changed-files", + eventName: "pull_request", + payload: { + action: "synchronize", + installation: { + id: 123, + account: { login: "JSONbored", id: 1, type: "User" }, + repository_selection: "selected", + permissions: { metadata: "read", pull_requests: "read", issues: "write", checks: "write" }, + events: ["issues", "issue_comment", "pull_request", "repository", "installation_repositories"], + }, + repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, + pull_request: { + number: 3, + title: "Fix webhook duplicate delivery again", + state: "open", + user: { login: "oktofeesh1" }, + head: { sha: "unified456" }, + labels: [{ name: "bug" }], + body: "Fixes #1\n\nValidation: npm test", + }, + }, + }); + + 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. + expect(postedBody).toContain("Changed files"); + expect(postedBody).toContain("| Source | 1 | +5 | -1 |"); + expect(postedBody).toContain("| Docs | 1 | +2 | -0 |"); + } finally { + liveCiSpy.mockRestore(); + } + }); + // FIX B + FIX D3 at the processor call site: a unified comment for a PR whose CI has a FAILED check, with the // PR's files only available from GitHub (stored rows empty) — proves (B) the inline file fetch populates the // real diff/changed-file count on the first review, and (D3) the failing check name + its per-check WHY render diff --git a/test/unit/signals-coverage.test.ts b/test/unit/signals-coverage.test.ts index 60085202fa..9cf9fc9232 100644 --- a/test/unit/signals-coverage.test.ts +++ b/test/unit/signals-coverage.test.ts @@ -1127,7 +1127,7 @@ describe("signal coverage edge cases", () => { collisions: buildCollisionReport(directRepo.fullName, [], [currentPr]), preflight: buildPreflightResult({ repoFullName: directRepo.fullName, title: "Fix isolated issue", body: "Fixes #99", linkedIssues: [99] }, directRepo, [], [currentPr]), settings: gateSettings, - review: { present: true, footerText: "Reviewed by the Acme maintainer bot.", note: "Run npm test before pushing.", fields: { relatedWork: false }, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, suggestions: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { skipDrafts: null, ignoreAuthors: [], ignoreTitleKeywords: [], baseBranches: [], autoPauseAfterReviewedCommits: null }, labelingRules: [], aiModel: { claudeModel: null, claudeEffort: null, codexModel: null, codexEffort: null } }, + review: { present: true, footerText: "Reviewed by the Acme maintainer bot.", note: "Run npm test before pushing.", fields: { relatedWork: false }, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, suggestions: null, changedFilesSummary: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { skipDrafts: null, ignoreAuthors: [], ignoreTitleKeywords: [], baseBranches: [], autoPauseAfterReviewedCommits: null }, labelingRules: [], aiModel: { claudeModel: null, claudeEffort: null, codexModel: null, codexEffort: null } }, aiReview: { notes: "The change is focused.\n\n**Nits (2)**\n- Add a test for the edge case.\n- Keep the validator helper scoped." }, }); expect(customizedComment).toContain("Reviewed by the Acme maintainer bot."); // custom footer lead