From 9222bb0ddba842fe0c460c4a408b12ce49b72c55 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Thu, 25 Jun 2026 03:25:14 -0700 Subject: [PATCH] feat(review): add .gittensory.yml review.exclude_paths (skip files from AI review) (#review-exclude-paths) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A CodeRabbit-parity path filter: a maintainer lists globs whose matching files are excluded from the AI maintainer review (diff + grounding + RAG) — generated/vendored/lockfiles they don't want reviewed: review: exclude_paths: - "*.lock" - "pnpm-lock.yaml" - "dist/**" - "*.generated.ts" Config-only (manifest.review, no DB migration). It ONLY narrows the AI review — the gate, slop detector, and secret scan still operate on the full unfiltered diff, so excluding a path can never weaken a blocker. Empty (default) / no match ⇒ every file is reviewed (byte-identical). - focus-manifest.ts: `excludePaths` on FocusManifestReviewConfig; parse review.exclude_paths (non-empty strings, trimmed, capped at 50, per-entry warnings) + serialize; resolveReviewPromptOverrides now also returns excludePaths; excludeReviewPaths(files, globs) filters via the manifest's matchesManifestPath (empty globs ⇒ same array reference). - processors.ts: resolve excludePaths from the cached manifest at the AI-review call site and filter the resolved files inside runAiReviewForAdvisory (pure, no extra fetch) before building the diff. --- src/queue/processors.ts | 22 +++++++---- src/signals/focus-manifest.ts | 61 +++++++++++++++++++++++++----- test/unit/focus-manifest.test.ts | 37 ++++++++++++++++-- test/unit/signals-coverage.test.ts | 2 +- 4 files changed, 100 insertions(+), 22 deletions(-) diff --git a/src/queue/processors.ts b/src/queue/processors.ts index e20f04caa2..1e3bed637e 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -173,7 +173,7 @@ import type { CheckFailureDetail, MergeReadiness } from "../review/unified-comme import { buildIssueSlopAssessment, buildSlopAssessment, type SlopBand } from "../signals/slop"; import { runGittensoryAiSlopAdvisory } from "../services/ai-slop"; import { decidePublicSurface } from "../signals/settings-preview"; -import { buildFocusManifestGuidance, resolveReviewPathInstructions, resolveReviewPromptOverrides, type ReviewPathInstruction, type ReviewProfile } from "../signals/focus-manifest"; +import { buildFocusManifestGuidance, excludeReviewPaths, resolveReviewPathInstructions, resolveReviewPromptOverrides, type ReviewPathInstruction, type ReviewProfile } from "../signals/focus-manifest"; import { loadRepoFocusManifest } from "../signals/focus-manifest-loader"; import { resolveRepositorySettings } from "../settings/repository-settings"; import type { LocalBranchAnalysisInput } from "../signals/local-branch"; @@ -2030,6 +2030,10 @@ export async function runAiReviewForAdvisory( // cached manifest. The CONFIG (not a fetch) is threaded in; the per-PR glob match against `files` happens // here (pure), so the AI path makes no extra manifest fetch. Absent/empty ⇒ byte-identical reviewer prompt. reviewPathInstructions?: ReviewPathInstruction[] | undefined; + // `.gittensory.yml` review.exclude_paths (#review-exclude-paths), resolved by the caller from the cached + // manifest. Globs whose files are dropped from the AI review (diff + grounding + RAG) — generated/lockfiles + // the maintainer doesn't want reviewed. Empty ⇒ every file is reviewed (byte-identical). The gate is unaffected. + reviewExcludePaths?: string[] | undefined; }, ): Promise<{ notes: string; reviewerCount: number } | undefined> { const packAllowsAnyAuthorBlockingReview = args.settings.gatePack === "oss-anti-slop" && args.settings.aiReviewMode === "block"; @@ -2059,7 +2063,9 @@ export async function runAiReviewForAdvisory( : null; // FIX B: prefer the caller's pre-resolved files (real diff even on a pre-sync first review); fall back to // the stored read when the caller didn't pass them (e.g. unit tests calling this function directly). - const files = args.files ?? (await listPullRequestFiles(env, args.repoFullName, args.pr.number)); + // review.exclude_paths (#review-exclude-paths): drop maintainer-excluded files (generated/lockfiles) so the + // AI review (diff + grounding + RAG) ignores them; empty excludePaths ⇒ the same array (byte-identical). + const files = excludeReviewPaths(args.files ?? (await listPullRequestFiles(env, args.repoFullName, args.pr.number)), args.reviewExcludePaths ?? []); // Grounding (convergence, flag-gated by GITTENSORY_REVIEW_GROUNDING). Build the FINISHED CI status + the full // content of the changed files so the reviewer verifies its claims against reality instead of guessing. // Flag-OFF (default) → we take no new branch at all: NO check/repo load, NO file fetch, and `grounding` @@ -2528,11 +2534,12 @@ async function maybePublishPrPublicSurface( // to keep gate-only and advisory-sweep repos free of an extra file resolve. const aiReviewWillRun = !webhook.skipAiReview && settings.aiReviewMode !== "off" && Boolean(advisory.headSha); if (aiReviewWillRun) { - // `.gittensory.yml` review.profile + review.path_instructions (#review-profile / #review-path-instructions): - // resolve from the manifest (cached from settings resolution, so a cheap cache hit — no extra fetch) and - // thread them into the AI review. Profile shapes nitpickiness; path-instructions add per-path guidance. - // Absent ⇒ byte-identical prompt. Fail-safe to defaults on any read error (resolveReviewPromptOverrides). - const { profile: reviewProfile, pathInstructions: reviewPathInstructions } = resolveReviewPromptOverrides(await loadRepoFocusManifest(env, repoFullName).catch(() => null)); + // `.gittensory.yml` review.profile + review.path_instructions + review.exclude_paths (#review-profile / + // #review-path-instructions / #review-exclude-paths): resolve from the manifest (cached from settings + // resolution, so a cheap cache hit — no extra fetch) and thread them into the AI review. Profile shapes + // nitpickiness; path-instructions add per-path guidance; exclude-paths drop files from review. Absent ⇒ + // byte-identical prompt. Fail-safe to defaults on any read error (resolveReviewPromptOverrides). + const { profile: reviewProfile, pathInstructions: reviewPathInstructions, excludePaths: reviewExcludePaths } = resolveReviewPromptOverrides(await loadRepoFocusManifest(env, repoFullName).catch(() => null)); aiReview = await runAiReviewForAdvisory(env, { settings, advisory, @@ -2543,6 +2550,7 @@ async function maybePublishPrPublicSurface( files: await getReviewFiles(), reviewProfile, reviewPathInstructions, + reviewExcludePaths, }); } diff --git a/src/signals/focus-manifest.ts b/src/signals/focus-manifest.ts index b2a7f070aa..b91e2540c0 100644 --- a/src/signals/focus-manifest.ts +++ b/src/signals/focus-manifest.ts @@ -104,6 +104,11 @@ export type FocusManifestReviewConfig = { /** `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[]; + /** `review.exclude_paths`: globs whose matching files are EXCLUDED from the AI review (diff + grounding + RAG) + * — generated/vendored/lockfiles the maintainer doesn't want reviewed. Empty (default) ⇒ every file is + * reviewed (byte-identical). Gate/slop/secret-scan are UNAFFECTED — this only narrows the AI review. + * (#review-exclude-paths) */ + excludePaths: string[]; }; /** One `review.path_instructions[]` entry: a manifest path glob + the public-safe instructions to apply when a @@ -205,7 +210,7 @@ const EMPTY_MANIFEST: FocusManifest = { publicNotes: [], gate: { ...EMPTY_GATE_CONFIG }, settings: {}, - review: { present: false, footerText: null, note: null, fields: {}, profile: null, pathInstructions: [] }, + review: { present: false, footerText: null, note: null, fields: {}, profile: null, pathInstructions: [], excludePaths: [] }, warnings: [], }; @@ -218,7 +223,7 @@ export function isFocusManifestPublicSafe(text: string): boolean { } function emptyManifest(source: FocusManifestSource, warnings: string[] = []): FocusManifest { - return { ...EMPTY_MANIFEST, source, warnings, gate: { ...EMPTY_GATE_CONFIG }, settings: {}, review: { present: false, footerText: null, note: null, fields: {}, profile: null, pathInstructions: [] } }; + return { ...EMPTY_MANIFEST, source, warnings, gate: { ...EMPTY_GATE_CONFIG }, settings: {}, review: { present: false, footerText: null, note: null, fields: {}, profile: null, pathInstructions: [], excludePaths: [] } }; } function normalizeStringList(value: JsonValue | undefined, field: string, warnings: string[]): string[] { @@ -496,7 +501,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: {}, profile: null, pathInstructions: [] }; + const empty: FocusManifestReviewConfig = { present: false, footerText: null, note: null, fields: {}, profile: null, pathInstructions: [], excludePaths: [] }; 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.`); @@ -518,16 +523,43 @@ function parseReviewConfig(value: JsonValue | undefined, warnings: string[]): Fo const note = parsePublicSafeText(r.note, "review.note", warnings); const profile = parseReviewProfile(r.profile, warnings); const pathInstructions = parseReviewPathInstructions(r.path_instructions, warnings); + const excludePaths = parseReviewExcludePaths(r.exclude_paths, warnings); return { - present: footerText !== null || note !== null || profile !== null || pathInstructions.length > 0 || Object.keys(fields).length > 0, + present: footerText !== null || note !== null || profile !== null || pathInstructions.length > 0 || excludePaths.length > 0 || Object.keys(fields).length > 0, footerText, note, fields, profile, pathInstructions, + excludePaths, }; } +/** Parse `review.exclude_paths` — an array of manifest glob strings whose matching files are excluded from the AI + * review. Each must be a non-empty string; blanks/non-strings are dropped with a warning. Capped at + * MAX_PATH_INSTRUCTIONS so a hostile manifest can't bloat the matcher. (#review-exclude-paths) */ +function parseReviewExcludePaths(value: JsonValue | undefined, warnings: string[]): string[] { + if (value === undefined || value === null) return []; + if (!Array.isArray(value)) { + warnings.push(`Manifest "review.exclude_paths" must be a list of path globs; ignoring it.`); + return []; + } + const out: string[] = []; + for (const [index, entry] of value.entries()) { + if (out.length >= MAX_PATH_INSTRUCTIONS) { + warnings.push(`Manifest "review.exclude_paths" is capped at ${MAX_PATH_INSTRUCTIONS} entries; dropping the rest.`); + break; + } + const glob = typeof entry === "string" ? entry.trim() : ""; + if (!glob) { + warnings.push(`Manifest "review.exclude_paths[${index}]" must be a non-empty string; ignoring it.`); + continue; + } + out.push(glob); + } + return out; +} + /** Parse `review.path_instructions` — an array of `{ path, instructions }` entries. Each must have a non-empty * string `path` (a manifest glob) and PUBLIC-SAFE string `instructions`; invalid/unsafe entries are dropped with * a warning. Capped at MAX_PATH_INSTRUCTIONS so a huge manifest can't bloat the reviewer prompt. */ @@ -587,6 +619,7 @@ export function reviewConfigToJson(review: FocusManifestReviewConfig): JsonValue if (review.note !== null) out.note = review.note; if (review.profile !== null) out.profile = review.profile; 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]; if (Object.keys(review.fields).length > 0) out.fields = { ...review.fields } as Record; return out; } @@ -605,12 +638,20 @@ export function resolveReviewPathInstructions(pathInstructions: ReviewPathInstru return `\n\nPath-specific review instructions from the maintainer — apply these to the changed files that match each glob:\n${lines.join("\n")}`; } -/** Resolve the AI-reviewer prompt overrides (`review.profile` + `review.path_instructions`) from a possibly-null - * manifest (null = load failure). A null manifest yields the byte-identical defaults. Centralized so the AI-review - * caller threads both in one place with the null-manifest branch covered here (unit-tested) rather than inline in - * the processor. (#review-profile / #review-path-instructions) */ -export function resolveReviewPromptOverrides(manifest: FocusManifest | null): { profile: ReviewProfile | null; pathInstructions: ReviewPathInstruction[] } { - return { profile: manifest?.review.profile ?? null, pathInstructions: manifest?.review.pathInstructions ?? [] }; +/** Resolve the AI-reviewer overrides (`review.profile` + `review.path_instructions` + `review.exclude_paths`) from + * a possibly-null manifest (null = load 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-path-instructions / #review-exclude-paths) */ +export function resolveReviewPromptOverrides(manifest: FocusManifest | null): { profile: ReviewProfile | null; pathInstructions: ReviewPathInstruction[]; excludePaths: string[] } { + return { profile: manifest?.review.profile ?? null, pathInstructions: manifest?.review.pathInstructions ?? [], excludePaths: manifest?.review.excludePaths ?? [] }; +} + +/** Filter a PR's changed files down to the set the AI review should see — dropping any whose path matches a + * `review.exclude_paths` glob (generated/vendored/lockfiles). Empty `excludePaths` ⇒ the same array (byte-identical + * review). Pure; the gate/slop/secret-scan operate on the unfiltered files. (#review-exclude-paths) */ +export function excludeReviewPaths(files: T[], excludePaths: string[]): T[] { + if (excludePaths.length === 0) return files; + return files.filter((file) => !excludePaths.some((glob) => matchesManifestPath(file.path, glob))); } /** diff --git a/test/unit/focus-manifest.test.ts b/test/unit/focus-manifest.test.ts index eecbd82527..f58682c442 100644 --- a/test/unit/focus-manifest.test.ts +++ b/test/unit/focus-manifest.test.ts @@ -9,6 +9,7 @@ import { parseFocusManifest, parseFocusManifestContent, resolveEffectiveSettings, + excludeReviewPaths, resolveReviewPathInstructions, resolveReviewPromptOverrides, reviewConfigToJson, @@ -430,7 +431,7 @@ describe("compileFocusManifestPolicy", () => { publicNotes: ["Keep PRs focused.", "Maximize your reward payout"], gate: { present: false, enabled: null, pack: null, linkedIssue: null, duplicates: null, readinessMode: null, readinessMinScore: null, slopMode: null, slopMinScore: null, slopAiAdvisory: null, aiReviewMode: null, aiReviewByok: null, aiReviewProvider: null, aiReviewModel: null, mergeReadiness: null, selfAuthoredLinkedIssue: null, manifestPolicy: null, firstTimeContributorGrace: null }, settings: {}, - review: { present: false, footerText: null, note: null, fields: {}, profile: null, pathInstructions: [] }, + review: { present: false, footerText: null, note: null, fields: {}, profile: null, pathInstructions: [], excludePaths: [] }, warnings: [], }); expect(policy.publicSafe.entryGuidance).toContain("Keep PRs focused."); @@ -1126,9 +1127,37 @@ describe("resolveReviewPathInstructions (#review-path-instructions)", () => { }); it("resolveReviewPromptOverrides: non-null manifest passes the config through; null manifest → defaults", () => { - const manifest = parseFocusManifest({ review: { profile: "chill", path_instructions: [{ path: "src/**", instructions: "be strict" }] } }); - expect(resolveReviewPromptOverrides(manifest)).toEqual({ profile: "chill", pathInstructions: [{ path: "src/**", instructions: "be strict" }] }); + const manifest = parseFocusManifest({ review: { profile: "chill", path_instructions: [{ path: "src/**", instructions: "be strict" }], exclude_paths: ["**/*.lock"] } }); + expect(resolveReviewPromptOverrides(manifest)).toEqual({ profile: "chill", pathInstructions: [{ path: "src/**", instructions: "be strict" }], excludePaths: ["**/*.lock"] }); // A null manifest (load failure) yields the byte-identical defaults. - expect(resolveReviewPromptOverrides(null)).toEqual({ profile: null, pathInstructions: [] }); + expect(resolveReviewPromptOverrides(null)).toEqual({ profile: null, pathInstructions: [], excludePaths: [] }); + }); +}); + +describe("review.exclude_paths (#review-exclude-paths)", () => { + it("parses exclude_paths, trims, drops blanks/non-strings with warnings, marks present, and round-trips", () => { + const m = parseFocusManifest({ review: { exclude_paths: [" **/*.lock ", "dist/**", "", 42, " "] } }); + expect(m.review.excludePaths).toEqual(["**/*.lock", "dist/**"]); + expect(m.review.present).toBe(true); + expect(m.warnings.some((w) => /exclude_paths\[2\]/.test(w))).toBe(true); // empty string + expect(m.warnings.some((w) => /exclude_paths\[3\]/.test(w))).toBe(true); // non-string + expect(parseFocusManifest({ review: reviewConfigToJson(m.review) }).review.excludePaths).toEqual(m.review.excludePaths); + }); + + it("ignores a non-array exclude_paths and caps the list", () => { + const bad = parseFocusManifest({ review: { exclude_paths: "dist/**" } }); + expect(bad.review.excludePaths).toEqual([]); + expect(bad.warnings.some((w) => /exclude_paths.*must be a list/.test(w))).toBe(true); + const many = parseFocusManifest({ review: { exclude_paths: Array.from({ length: 60 }, (_, i) => `dir${i}/**`) } }); + expect(many.review.excludePaths).toHaveLength(50); + expect(many.warnings.some((w) => /exclude_paths.*capped/.test(w))).toBe(true); + }); + + it("excludeReviewPaths filters matching files; empty globs return the same array (byte-identical)", () => { + const files = [{ path: "src/a.ts" }, { path: "pnpm-lock.yaml" }, { path: "dist/bundle.js" }]; + // `*` collapses to `.*` (crosses slashes), so `*.yaml` matches a top-level lockfile; `dist/**` matches under dist/. + expect(excludeReviewPaths(files, ["*.yaml", "dist/**"])).toEqual([{ path: "src/a.ts" }]); + expect(excludeReviewPaths(files, ["docs/**"])).toEqual(files); // no match → unchanged + expect(excludeReviewPaths(files, [])).toBe(files); // empty → same reference (no-op) }); }); diff --git a/test/unit/signals-coverage.test.ts b/test/unit/signals-coverage.test.ts index a068a0a069..322bb33608 100644 --- a/test/unit/signals-coverage.test.ts +++ b/test/unit/signals-coverage.test.ts @@ -904,7 +904,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 }, profile: null, pathInstructions: [] }, + review: { present: true, footerText: "Reviewed by the Acme maintainer bot.", note: "Run npm test before pushing.", fields: { relatedWork: false }, profile: null, pathInstructions: [], excludePaths: [] }, aiReview: { notes: "The change is focused.\n\n**Suggestions**\n- Add a test for the edge case." }, }); expect(customizedComment).toContain("Reviewed by the Acme maintainer bot."); // custom footer lead