From 26db229b0016753be1940ad1e840e402c72ce6db Mon Sep 17 00:00:00 2001 From: Tanisha Aberdeen <32620895+aliasunder@users.noreply.github.com> Date: Sat, 5 Sep 2026 23:48:22 -0400 Subject: [PATCH 01/13] feat: exclude generated files from the review diff by default MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit New diff_exclude_paths input: folder prefixes or globs removed from the review diff before the token budget check, so one oversized generated artifact no longer skips the whole review. Supplied patterns extend a built-in default list (ecosystem lockfiles, minified sources, source maps); a leading "none" drops the defaults. Patterns over 2 stars per segment are rejected — glob matching backtracks exponentially. New respect_linguist_generated input (default on): files the repo's root .gitattributes marks linguist-generated=true are excluded too; negated entries keep a default-list match reviewable. Excluded files stay visible — named with their source in the annotated diff trailer, the status comment's context notes, and the run log. When every changed file matches, the run posts a skip review naming the mechanism. Co-Authored-By: Claude Fable 5 --- AGENTS.md | 4 +- README.md | 44 ++-- action.yml | 28 ++ package-lock.json | 2 +- package.json | 1 + src/__tests__/config.test.ts | 102 +++++++- src/__tests__/orchestrate.test.ts | 195 ++++++++++++++ src/config.ts | 81 +++++- src/context/__tests__/workspace.test.ts | 66 +++++ src/context/workspace.ts | 30 ++- src/diff/__tests__/exclude-diff-files.test.ts | 241 ++++++++++++++++++ src/diff/__tests__/gitattributes.test.ts | 118 +++++++++ src/diff/__tests__/pattern-safety.test.ts | 29 +++ src/diff/exclude-diff-files.ts | 153 +++++++++++ src/diff/gitattributes.ts | 107 ++++++++ src/diff/pattern-safety.ts | 15 ++ src/main.ts | 2 + src/orchestrate.ts | 50 +++- src/review/__tests__/context-notes.test.ts | 43 +++- src/review/context-notes.ts | 15 ++ 20 files changed, 1293 insertions(+), 33 deletions(-) create mode 100644 src/diff/__tests__/exclude-diff-files.test.ts create mode 100644 src/diff/__tests__/gitattributes.test.ts create mode 100644 src/diff/__tests__/pattern-safety.test.ts create mode 100644 src/diff/exclude-diff-files.ts create mode 100644 src/diff/gitattributes.ts create mode 100644 src/diff/pattern-safety.ts diff --git a/AGENTS.md b/AGENTS.md index bf61866..e4c02ca 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -23,8 +23,8 @@ src/ logger.ts # structured JSON logger — levels, child contexts, lazy props github/ # GitHub I/O: event payload → PrContext, octokit wrappers (diff fetch, review posting) openrouter/ # OpenRouter I/O: @openrouter/sdk wrapper, per-attempt deadline, structured-output retry ladder, cost summary - diff/ # pure transforms over parse-diff output - context/ # workspace I/O: conventions file, changed files, import-trace scan, doc-mention scan, priority docs + diff/ # pure transforms over parse-diff output + diff-level exclusion (patterns, gitattributes linguist rules, wildcard safety cap) + context/ # workspace I/O: conventions file, root .gitattributes, changed files, import-trace scan, doc-mention scan, priority docs review/ # pure review logic: finding schema, phases + stage dispatch, prompt, non-finding filter, unknown-file filter, cross-phase merge, path normalization, selection, comment mapping, title similarity, context notes, summary orchestrate.ts # pipeline + createPromptedGenerateFindings — fully testable with stub clients ``` diff --git a/README.md b/README.md index da1db49..46213f6 100644 --- a/README.md +++ b/README.md @@ -75,27 +75,29 @@ The `@umm review` comment trigger lets you re-request a review on any PR by comm ## Inputs -| Input | Default | Description | -| ------------------------- | ----------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| `github_token` | _(required)_ | Token for fetching the diff and posting the review. A GitHub App installation token keeps the bot identity. | -| `openrouter_api_key` | _(required)_ | OpenRouter API key | -| `model` | `anthropic/claude-sonnet-4-6` | OpenRouter model slug exactly as listed on openrouter.ai/models | -| `fallback_model` | `""` | Model to retry with if the primary model fails the structured-output ladder | -| `request_timeout_seconds` | `900` | Per-attempt deadline for a single model request, in seconds. When it elapses the attempt is recorded as `timeout` and the retry/fallback ladder advances whether or not the provider connection closes; the HTTP call is aborted best-effort. A request the provider keeps serving past the deadline is still billed, and its cost-summary row shows no cost | -| `max_findings` | `""` _(uncapped)_ | Cap on posted findings, highest severity first. Empty = all validated findings post. | -| `severity_threshold` | `low` | Minimum severity to post: `low` \| `medium` \| `high` \| `critical` | -| `conventions_file` | `AGENTS.md` | Repo-relative path to the conventions file included in the prompt (truncated at ~8000 tokens). When the file also changed in the PR, deduplication ensures its full text appears exactly once across context channels | -| `phases` | `combined` | How the review dimensions are dispatched: `combined` (one model call carrying every dimension), `parallel` (three focused calls at once — each reads deeper than the single call; wall clock is the slowest of the three and prompt tokens roughly triple), or `sequential` (the same three calls in order, each seeing the earlier findings). Findings two phases report on the same lines collapse to one, keeping the higher severity. Empty = `combined`, so workflows can wire an unset repo variable directly | -| `context_budget_tokens` | `300000` | Approximate token budget for prompt context (file contents + diff — conventions have a separate cap) | -| `trace_related_files` | `true` | Enable heuristic context scanning — import-tracing for caller regressions and mention-matching for doc staleness detection. Does not affect `priority_docs` | -| `priority_docs` | `README.md` | Comma-separated repo-relative paths included in review context with a reserved 10% budget floor, independent of `trace_related_files`. Docs already present in full from other context channels are not re-read; a diff-only changed file is still read here so its full text reaches the model. Subject to the shared doc token budget (the floor prevents starvation by related files but does not guarantee inclusion when the budget is exhausted by the diff and changed files themselves). Empty = disabled | -| `max_scan_files` | `5000` | Maximum files to walk during workspace scan for related file and doc detection | -| `max_scan_bytes` | `262144` | Maximum byte size of a single file to include in the workspace scan | -| `max_related_files` | `8` | Maximum import-traced related files to include in review context | -| `max_related_docs` | `4` | Maximum mention-matched documentation files to include in review context (excludes priority docs) | -| `exclude_paths` | `""` | Comma-separated folder prefixes excluded from the workspace scan. Files under these paths are invisible to import-tracing and doc-mention matching. Changed files in the PR diff and `priority_docs` are never excluded. Example: `evals, fixtures, __snapshots__` | -| `cost_summary` | `true` | Write a per-run cost report (model, prompt/completion tokens, USD) to the workflow step summary | -| `pr_number` | `""` | PR number override — required only when the triggering event does not identify a PR directly | +| Input | Default | Description | +| ---------------------------- | ----------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `github_token` | _(required)_ | Token for fetching the diff and posting the review. A GitHub App installation token keeps the bot identity. | +| `openrouter_api_key` | _(required)_ | OpenRouter API key | +| `model` | `anthropic/claude-sonnet-4-6` | OpenRouter model slug exactly as listed on openrouter.ai/models | +| `fallback_model` | `""` | Model to retry with if the primary model fails the structured-output ladder | +| `request_timeout_seconds` | `900` | Per-attempt deadline for a single model request, in seconds. When it elapses the attempt is recorded as `timeout` and the retry/fallback ladder advances whether or not the provider connection closes; the HTTP call is aborted best-effort. A request the provider keeps serving past the deadline is still billed, and its cost-summary row shows no cost | +| `max_findings` | `""` _(uncapped)_ | Cap on posted findings, highest severity first. Empty = all validated findings post. | +| `severity_threshold` | `low` | Minimum severity to post: `low` \| `medium` \| `high` \| `critical` | +| `conventions_file` | `AGENTS.md` | Repo-relative path to the conventions file included in the prompt (truncated at ~8000 tokens). When the file also changed in the PR, deduplication ensures its full text appears exactly once across context channels | +| `phases` | `combined` | How the review dimensions are dispatched: `combined` (one model call carrying every dimension), `parallel` (three focused calls at once — each reads deeper than the single call; wall clock is the slowest of the three and prompt tokens roughly triple), or `sequential` (the same three calls in order, each seeing the earlier findings). Findings two phases report on the same lines collapse to one, keeping the higher severity. Empty = `combined`, so workflows can wire an unset repo variable directly | +| `context_budget_tokens` | `300000` | Approximate token budget for prompt context (file contents + diff — conventions have a separate cap) | +| `trace_related_files` | `true` | Enable heuristic context scanning — import-tracing for caller regressions and mention-matching for doc staleness detection. Does not affect `priority_docs` | +| `priority_docs` | `README.md` | Comma-separated repo-relative paths included in review context with a reserved 10% budget floor, independent of `trace_related_files`. Docs already present in full from other context channels are not re-read; a diff-only changed file is still read here so its full text reaches the model. Subject to the shared doc token budget (the floor prevents starvation by related files but does not guarantee inclusion when the budget is exhausted by the diff and changed files themselves). Empty = disabled | +| `max_scan_files` | `5000` | Maximum files to walk during workspace scan for related file and doc detection | +| `max_scan_bytes` | `262144` | Maximum byte size of a single file to include in the workspace scan | +| `max_related_files` | `8` | Maximum import-traced related files to include in review context | +| `max_related_docs` | `4` | Maximum mention-matched documentation files to include in review context (excludes priority docs) | +| `exclude_paths` | `""` | Comma-separated folder prefixes excluded from the workspace scan. Files under these paths are invisible to import-tracing and doc-mention matching. Changed files in the PR diff and `priority_docs` are never excluded. Example: `evals, fixtures, __snapshots__` | +| `diff_exclude_paths` | `""` _(built-in list)_ | Comma-separated folder prefixes or globs removed from the review diff before the token budget check. Excluded files are listed by name in the review output, but their content is not reviewed — excluded is not vetted. Supplied patterns extend a built-in default list of generated artifacts (ecosystem lockfiles, `*.min.js`, `*.min.css`, `*.map` — the classes GitHub's linguist auto-collapses; snapshots are deliberately not defaulted, since GitHub renders them expanded). A leading `none` drops the defaults: `none` alone disables exclusion, `none, evals/**` replaces the list. Unlike `exclude_paths`, empty means the default list, not "no exclusions". Patterns with more than 2 `*` in one path segment are rejected (`**` segments exempt) — glob matching backtracks exponentially on such shapes | +| `respect_linguist_generated` | `true` | Also exclude changed files the repo's root `.gitattributes` marks `linguist-generated=true` (nested `.gitattributes` files are not read). Negated entries (`-linguist-generated`) keep a file reviewable even when the default `diff_exclude_paths` list matches it; explicitly supplied `diff_exclude_paths` patterns always win. Rules are read from the PR head, so a PR changing `.gitattributes` reviews under its own rules — every exclusion is named in the review output | +| `cost_summary` | `true` | Write a per-run cost report (model, prompt/completion tokens, USD) to the workflow step summary | +| `pr_number` | `""` | PR number override — required only when the triggering event does not identify a PR directly | ## Outputs diff --git a/action.yml b/action.yml index 2e4883e..1bd637c 100644 --- a/action.yml +++ b/action.yml @@ -78,6 +78,34 @@ inputs: __snapshots__ required: false default: "" + diff_exclude_paths: + description: >- + Comma-separated folder prefixes or globs removed from the review diff + before the token budget check — excluded files are listed by name in + the review output but their content is not reviewed (excluded is not + vetted). Supplied patterns EXTEND a built-in default list of + generated artifacts (ecosystem lockfiles, *.min.js, *.min.css, + *.map — the classes GitHub's linguist auto-collapses); a leading + "none" drops the defaults, so "none" alone disables exclusion and + "none, evals/**" replaces the list outright. Empty = the default + list (unlike exclude_paths, where empty means no exclusions), so + workflows can wire an unset repo variable directly. Patterns with + more than 2 "*" in one path segment are rejected ("**" segments are + exempt) — glob matching backtracks exponentially on such shapes. + Example: none, **/__snapshots__/** + required: false + default: "" + respect_linguist_generated: + description: >- + Also exclude changed files the repo's root .gitattributes marks + linguist-generated=true (nested .gitattributes files are not read). + Negated entries (-linguist-generated) keep a file reviewable even + when the default diff_exclude_paths list matches it; explicitly + supplied diff_exclude_paths patterns always win. Rules are read from + the PR head, so a PR changing .gitattributes reviews under its own + rules — exclusions are always named in the review output + required: false + default: "true" cost_summary: description: Write a per-run cost report (model, prompt/completion tokens, USD) to the workflow step summary required: false diff --git a/package-lock.json b/package-lock.json index 71e5957..1c4c7a9 100644 --- a/package-lock.json +++ b/package-lock.json @@ -13,6 +13,7 @@ "@actions/github": "9.1.1", "@openrouter/sdk": "1.2.85", "env-var": "7.5.0", + "ignore": "^5.3.2", "luxon": "3.7.2", "parse-diff": "0.12.0", "zod": "4.5.4" @@ -1647,7 +1648,6 @@ "version": "5.3.2", "resolved": "https://registry.npmjs.org/ignore/-/ignore-5.3.2.tgz", "integrity": "sha512-hsBTNUqQTDwkWtcdYI2i06Y/nUBEsNEDJKjWdigLvegy8kDuJAS8uRlpkkcQpyEXL0Z/pjDy5HBmMjRCJ2gq+g==", - "dev": true, "license": "MIT", "engines": { "node": ">= 4" diff --git a/package.json b/package.json index 7f8de8b..88a8a69 100644 --- a/package.json +++ b/package.json @@ -34,6 +34,7 @@ "@actions/github": "9.1.1", "@openrouter/sdk": "1.2.85", "env-var": "7.5.0", + "ignore": "^5.3.2", "luxon": "3.7.2", "parse-diff": "0.12.0", "zod": "4.5.4" diff --git a/src/__tests__/config.test.ts b/src/__tests__/config.test.ts index 451dc38..3329883 100644 --- a/src/__tests__/config.test.ts +++ b/src/__tests__/config.test.ts @@ -1,5 +1,9 @@ import { describe, expect, it } from "vitest" -import { parseConfig, type RawInputs } from "../config.js" +import { + DEFAULT_DIFF_EXCLUDE_PATTERNS, + parseConfig, + type RawInputs, +} from "../config.js" const makeRawInputs = (overrides: Partial = {}): RawInputs => ({ githubToken: "ghs_testtoken", @@ -19,6 +23,8 @@ const makeRawInputs = (overrides: Partial = {}): RawInputs => ({ maxRelatedDocs: "4", priorityDocs: "README.md", excludePaths: "", + diffExcludePaths: "", + respectLinguistGenerated: true, costSummary: true, prNumberOverride: "", ...overrides, @@ -46,6 +52,28 @@ describe("parseConfig", () => { maxRelatedDocs: 4, priorityDocs: ["README.md"], excludePaths: [], + diffExcludePaths: { + defaultPatterns: [ + "**/package-lock.json", + "**/npm-shrinkwrap.json", + "**/yarn.lock", + "**/pnpm-lock.yaml", + "**/bun.lock", + "**/bun.lockb", + "**/deno.lock", + "**/composer.lock", + "**/Cargo.lock", + "**/Gemfile.lock", + "**/poetry.lock", + "**/uv.lock", + "**/go.sum", + "**/*.min.js", + "**/*.min.css", + "**/*.map", + ], + operatorPatterns: [], + }, + respectLinguistGenerated: true, costSummary: true, prNumberOverride: undefined, }) @@ -230,6 +258,78 @@ describe("parseConfig", () => { expect(config.excludePaths).toEqual(["evals", "fixtures", "nested/deep"]) }) + it("extends the default diff_exclude_paths list with supplied patterns", () => { + const config = parseConfig( + makeRawInputs({ diffExcludePaths: "evals/**, **/*.snap" }), + ) + + expect(config.diffExcludePaths.operatorPatterns).toEqual([ + "evals/**", + "**/*.snap", + ]) + // The behavioral claim is tier preservation — supplied patterns must not + // replace the built-in list, so identity with the constant is the spec + expect(config.diffExcludePaths.defaultPatterns).toEqual( + DEFAULT_DIFF_EXCLUDE_PATTERNS, + ) + }) + + it("disables the default list with a leading none", () => { + const config = parseConfig(makeRawInputs({ diffExcludePaths: "none" })) + + expect(config.diffExcludePaths).toEqual({ + defaultPatterns: [], + operatorPatterns: [], + }) + }) + + it("replaces the default list via none followed by patterns", () => { + const config = parseConfig( + makeRawInputs({ diffExcludePaths: "none, evals/**" }), + ) + + expect(config.diffExcludePaths).toEqual({ + defaultPatterns: [], + operatorPatterns: ["evals/**"], + }) + }) + + it("rejects a non-leading none in diff_exclude_paths", () => { + expect(() => + parseConfig(makeRawInputs({ diffExcludePaths: "evals/**, none" })), + ).toThrow( + 'diffExcludePaths: "none" disables the default list only in leading position — move it first or remove it', + ) + }) + + it("normalizes diff_exclude_paths pattern spellings", () => { + const config = parseConfig( + makeRawInputs({ diffExcludePaths: "/evals, ./fixtures/, generated//" }), + ) + + expect(config.diffExcludePaths.operatorPatterns).toEqual([ + "evals", + "fixtures", + "generated", + ]) + }) + + it("rejects a diff_exclude_paths pattern over the wildcard cap", () => { + expect(() => + parseConfig(makeRawInputs({ diffExcludePaths: "*a*a*a*b" })), + ).toThrow( + 'diffExcludePaths: pattern(s) exceed the wildcard cap (at most 2 "*" per path segment; "**" segments exempt): *a*a*a*b', + ) + }) + + it("passes a false respect_linguist_generated through unchanged", () => { + const config = parseConfig( + makeRawInputs({ respectLinguistGenerated: false }), + ) + + expect(config.respectLinguistGenerated).toBe(false) + }) + it("rejects a zero max_related_files", () => { expect(() => parseConfig(makeRawInputs({ maxRelatedFiles: "0" }))).toThrow( 'maxRelatedFiles: "0" is not a positive integer', diff --git a/src/__tests__/orchestrate.test.ts b/src/__tests__/orchestrate.test.ts index f2f5874..24f48eb 100644 --- a/src/__tests__/orchestrate.test.ts +++ b/src/__tests__/orchestrate.test.ts @@ -230,6 +230,8 @@ const baseConfig: ActionConfig = { maxRelatedDocs: 4, priorityDocs: [], excludePaths: [], + diffExcludePaths: { defaultPatterns: [], operatorPatterns: [] }, + respectLinguistGenerated: true, costSummary: true, prNumberOverride: undefined, } @@ -409,6 +411,7 @@ const makeOrchestrateDeps = ( readConventionsCalls.push(params) return "# Test conventions" }, + readGitAttributes: async () => null, readChangedFiles: async (params) => { readChangedFilesCalls.push(params) return { files: [fixtureChangedFile], remainingTokens: 40_000 } @@ -612,6 +615,198 @@ describe("orchestrate", () => { }) }) + it("posts skip review when every changed file matches diff_exclude_paths", async () => { + const stubs = makeOrchestrateDeps({ + config: { + diffExcludePaths: { + defaultPatterns: [], + operatorPatterns: ["src/**", "assets/**"], + }, + }, + }) + const logger = createTestLogger() + + const result = await orchestrate(stubs.deps, logger) + + const skipReason = "all 6 changed files match diff_exclude_paths" + expect(result).toEqual({ + findingsCount: 0, + reviewUrl: "https://github.com/test/review/1", + modelUsed: "", + skippedReason: skipReason, + phases: [], + reviewSummaryMarkdown: null, + costSummaryMarkdown: null, + }) + expect(stubs.generateFindingsCalls).toHaveLength(0) + expect(stubs.readChangedFilesCalls).toHaveLength(0) + expect(stubs.submitReviewCalls).toHaveLength(1) + expect(first(stubs.submitReviewCalls)).toEqual({ + prNumber: fixturePrContext.prNumber, + commitId: fixturePrContext.headSha, + body: buildSkipBody(skipReason), + }) + }) + + it("removes an excluded file from the annotated diff, context reads, and changed paths", async () => { + const stubs = makeOrchestrateDeps({ + config: { + diffExcludePaths: { + defaultPatterns: [], + operatorPatterns: ["assets/**"], + }, + }, + }) + const logger = createTestLogger() + + await orchestrate(stubs.deps, logger) + + const keptFiles = fixtureFiles.filter( + (file) => (file.to ?? file.from) !== "assets/logo.png", + ) + const expectedExcludedNote = [ + "1 changed file(s) excluded from review (content not shown):", + "- assets/logo.png (+0/-0, diff_exclude_paths)", + ].join("\n") + const reviewContext = first(stubs.generateFindingsCalls) + expect(reviewContext.annotatedDiff).toBe( + `${annotateDiff(keptFiles)}\n\n${expectedExcludedNote}`, + ) + expect(first(stubs.readChangedFilesCalls).changedPaths).toEqual([ + "src/greeter.ts", + "src/added-file.ts", + "src/new-name.ts", + "src/old-name.ts", + "src/no-trailing-newline.ts", + ]) + }) + + it("adds a context note naming diff-excluded files and their source", async () => { + const stubs = makeOrchestrateDeps({ + config: { + diffExcludePaths: { + defaultPatterns: [], + operatorPatterns: ["assets/**"], + }, + }, + }) + const logger = createTestLogger() + + await orchestrate(stubs.deps, logger) + + expect(stubs.upsertSummaryCommentCalls).toEqual([ + expectedStatus({ + isFirstRun: true, + postedCount: expectedSelection.selected.length, + totalCount: expectedSelection.selected.length, + contextNotes: [ + "1 changed file(s) excluded from review: `assets/logo.png` (diff_exclude_paths)", + ], + }), + ]) + }) + + it("passes the budget check when the oversized files are all excluded", async () => { + // Budget 200 (half = 100) fails against the full fixture diff (341 + // tokens); with every src/ file excluded only the binary asset header + // and the excluded-files trailer remain (94 tokens), which fit + const stubs = makeOrchestrateDeps({ + config: { + contextBudgetTokens: 200, + diffExcludePaths: { + defaultPatterns: [], + operatorPatterns: ["src/**"], + }, + }, + }) + const logger = createTestLogger() + + const result = await orchestrate(stubs.deps, logger) + + // Guard for bar 2: the unfiltered fixture diff must exceed the half + // budget, or this test would pass without the exclusion doing anything + expect(sampleDiffTokens).toBeGreaterThan(100) + expect(result.skippedReason).toBe("") + expect(stubs.generateFindingsCalls).toHaveLength(1) + }) + + it("drops a finding naming an excluded file via the unknown-file filter", async () => { + const excludedFileFinding = makeFinding({ + file: "assets/logo.png", + line: 1, + }) + const stubs = makeOrchestrateDeps({ + config: { + diffExcludePaths: { + defaultPatterns: [], + operatorPatterns: ["assets/**"], + }, + }, + fixtureResult: { + review: { ...fixtureReviewResponse, findings: [excludedFileFinding] }, + }, + }) + const logger = createTestLogger() + + const result = await orchestrate(stubs.deps, logger) + + expect(result.findingsCount).toBe(0) + expect(stubs.postFindingsReviewCalls).toHaveLength(0) + expect(logger.messages).toContainEqual({ + level: "warn", + message: "dropping finding: file not in prompt context", + data: { + phase: "combined", + file: "assets/logo.png", + line: 1, + category: excludedFileFinding.category, + }, + }) + }) + + it("excludes files the repo marks linguist-generated", async () => { + const stubs = makeOrchestrateDeps({ + contextReader: { + readGitAttributes: async () => "assets/* linguist-generated=true\n", + }, + }) + const logger = createTestLogger() + + await orchestrate(stubs.deps, logger) + + const keptFiles = fixtureFiles.filter( + (file) => (file.to ?? file.from) !== "assets/logo.png", + ) + const expectedExcludedNote = [ + "1 changed file(s) excluded from review (content not shown):", + "- assets/logo.png (+0/-0, linguist-generated)", + ].join("\n") + expect(first(stubs.generateFindingsCalls).annotatedDiff).toBe( + `${annotateDiff(keptFiles)}\n\n${expectedExcludedNote}`, + ) + }) + + it("does not read gitattributes when respect_linguist_generated is off", async () => { + const readGitAttributesCalls: unknown[] = [] + const stubs = makeOrchestrateDeps({ + config: { respectLinguistGenerated: false }, + contextReader: { + readGitAttributes: async () => { + readGitAttributesCalls.push({}) + return "assets/* linguist-generated=true\n" + }, + }, + }) + const logger = createTestLogger() + + await orchestrate(stubs.deps, logger) + + expect(readGitAttributesCalls).toHaveLength(0) + expect(first(stubs.generateFindingsCalls).annotatedDiff).toBe( + annotateDiff(fixtureFiles), + ) + }) + it("passes correct prNumber and commitId in skip reviews", async () => { const stubs = makeOrchestrateDeps({ githubClient: { diff --git a/src/config.ts b/src/config.ts index 2bd90b0..e602c24 100644 --- a/src/config.ts +++ b/src/config.ts @@ -1,3 +1,4 @@ +import { hasExcessiveWildcards } from "./diff/pattern-safety.js" import { normalizeWorkspacePath } from "./review/workspace-path.js" import { z } from "zod" @@ -45,6 +46,81 @@ const timerSafeSeconds = z.string().transform((value, ctx) => { return parsed }) +/** + * Built-in diff exclusions — the file classes GitHub's linguist auto-collapses + * via rules no .gitattributes entry expresses (ecosystem lockfiles, minified + * sources, source maps). Snapshots are deliberately absent: linguist has no + * snapshot rule and GitHub renders them expanded, so repos opt them out via + * .gitattributes linguist-generated entries or the diff_exclude_paths input. + */ +export const DEFAULT_DIFF_EXCLUDE_PATTERNS = [ + "**/package-lock.json", + "**/npm-shrinkwrap.json", + "**/yarn.lock", + "**/pnpm-lock.yaml", + "**/bun.lock", + "**/bun.lockb", + "**/deno.lock", + "**/composer.lock", + "**/Cargo.lock", + "**/Gemfile.lock", + "**/poetry.lock", + "**/uv.lock", + "**/go.sum", + "**/*.min.js", + "**/*.min.css", + "**/*.map", +] + +export type DiffExcludeConfig = { + /** The built-in list, or empty when a leading "none" disabled it. Kept + * separate from operatorPatterns: a repo's negated gitattributes entry + * exempts a file from this tier but never from operator patterns. */ + defaultPatterns: string[] + operatorPatterns: string[] +} + +/** "" = the built-in default list (bare repo-variable wiring); a leading + * "none" disables the defaults — with a non-empty default, the empty string + * cannot mean both "default" and "off", so this input carries the action's + * only off sentinel. Supplied patterns extend whichever base survives. */ +const diffExcludePathsInput = z.string().transform((value, ctx) => { + const entries = value + .split(",") + .map((entry) => entry.trim()) + .filter((entry) => entry !== "") + + const defaultsDisabled = entries[0] === "none" + const patternEntries = defaultsDisabled ? entries.slice(1) : entries + + if (patternEntries.includes("none")) { + ctx.addIssue({ + code: "custom", + message: + '"none" disables the default list only in leading position — move it first or remove it', + }) + return z.NEVER + } + + const operatorPatterns = patternEntries + .map(normalizeWorkspacePath) + .filter((pattern) => pattern !== "" && pattern !== ".") + + const unsafePatterns = operatorPatterns.filter(hasExcessiveWildcards) + if (unsafePatterns.length > 0) { + ctx.addIssue({ + code: "custom", + message: `pattern(s) exceed the wildcard cap (at most 2 "*" per path segment; "**" segments exempt): ${unsafePatterns.join(", ")}`, + }) + return z.NEVER + } + + return { + defaultPatterns: defaultsDisabled ? [] : DEFAULT_DIFF_EXCLUDE_PATTERNS, + operatorPatterns, + } +}) + /** Mirrors the action.yml default — keep the two in sync. */ const defaultPhases = "combined" @@ -83,6 +159,8 @@ const configSchema = z.object({ .map(normalizeWorkspacePath) .filter((segment) => segment !== "" && segment !== "."), ), + diffExcludePaths: diffExcludePathsInput, + respectLinguistGenerated: z.boolean(), costSummary: z.boolean(), prNumberOverride: optionalPositiveInteger, }) @@ -100,10 +178,11 @@ export type ActionConfig = z.infer */ export type RawInputs = Omit< Record, - "traceRelatedFiles" | "costSummary" + "traceRelatedFiles" | "costSummary" | "respectLinguistGenerated" > & { traceRelatedFiles: boolean costSummary: boolean + respectLinguistGenerated: boolean } export const parseConfig = (rawInputs: RawInputs): ActionConfig => { diff --git a/src/context/__tests__/workspace.test.ts b/src/context/__tests__/workspace.test.ts index 8938b45..154b37e 100644 --- a/src/context/__tests__/workspace.test.ts +++ b/src/context/__tests__/workspace.test.ts @@ -148,6 +148,72 @@ describe("readConventions", () => { }) }) +describe("readGitAttributes", () => { + it("returns the root .gitattributes content", async () => { + const { root, cleanup } = await makeTempWorkspace({ + ".gitattributes": "*.snap linguist-generated=true\n", + }) + const contextReader = createContextReader( + defaultConfig(root), + createTestLogger(), + ) + + try { + const content = await contextReader.readGitAttributes() + + expect(content).toBe("*.snap linguist-generated=true\n") + } finally { + await cleanup() + } + }) + + it("returns null when the repo has no .gitattributes", async () => { + const { contextReader, logger } = makeReader() + + const content = await contextReader.readGitAttributes() + + expect(content).toBeNull() + expect(logger.messages).toEqual([]) + }) + + it("warns and returns null when .gitattributes symlinks outside the workspace", async () => { + // The outside target must exist: a dangling link would reject with ENOENT + // (the silent missing-file path) and pass this test without the guard + const outsideRoot = await mkdtemp(path.join(tmpdir(), "umm-outside-")) + await writeFile( + path.join(outsideRoot, "attrs"), + "* linguist-generated=true\n", + "utf8", + ) + const { root, cleanup } = await makeTempWorkspace({ + "README.md": "# fixture\n", + }) + await symlink( + path.join(outsideRoot, "attrs"), + path.join(root, ".gitattributes"), + ) + const logger = createTestLogger() + const contextReader = createContextReader(defaultConfig(root), logger) + + try { + const content = await contextReader.readGitAttributes() + + expect(content).toBeNull() + expect(logger.messages).toEqual([ + { + level: "warn", + message: + ".gitattributes resolves outside the reviewable workspace — linguist-generated rules unavailable", + data: {}, + }, + ]) + } finally { + await cleanup() + await rm(outsideRoot, { recursive: true, force: true }) + } + }) +}) + describe("readChangedFiles", () => { it("includes files in full and subtracts their tokens from the budget", async () => { const { contextReader } = makeReader() diff --git a/src/context/workspace.ts b/src/context/workspace.ts index a147a7e..2511e7b 100644 --- a/src/context/workspace.ts +++ b/src/context/workspace.ts @@ -1,6 +1,6 @@ import { readFile, readdir, realpath, stat } from "node:fs/promises" import path, { posix } from "node:path" -import type { Logger } from "../logger.js" +import { describeError, type Logger } from "../logger.js" import { CHARS_PER_TOKEN, estimateTokens, @@ -29,6 +29,9 @@ export type ContextReader = { readConventions: (params: { conventionsFile: string }) => Promise + /** Raw root .gitattributes content, or null when the repo has none; + * parsing stays in the pure diff layer. */ + readGitAttributes: () => Promise readChangedFiles: (params: { changedPaths: string[] budgetTokens: number @@ -201,6 +204,30 @@ export const createContextReader = ( } } + /** The file is repo-committed and PR-author-controlled, so every failure + * degrades to "no rules" instead of failing the run; only a missing file + * is silent — the other cases warn so the degradation is auditable. */ + const readGitAttributes = async (): Promise => { + const absolutePath = resolveUnderRoot(".gitattributes") + try { + const safePath = await realPathIfSafe(absolutePath) + if (!safePath) { + logger.warn( + ".gitattributes resolves outside the reviewable workspace — linguist-generated rules unavailable", + ) + return null + } + return await readFile(safePath, "utf8") + } catch (readError) { + if (isMissingFileError(readError)) return null + logger.warn( + "failed reading .gitattributes — linguist-generated rules unavailable", + { error: describeError(readError) }, + ) + return null + } + } + /** null on any unreadable changed file. A symlink that resolves outside * the safe zone is degraded to diff-only rather than fatal — one hostile * or broken file must not kill the whole review — but gets its own @@ -672,6 +699,7 @@ export const createContextReader = ( return { readConventions, + readGitAttributes, readChangedFiles, findRelatedFiles, readPriorityDocs, diff --git a/src/diff/__tests__/exclude-diff-files.test.ts b/src/diff/__tests__/exclude-diff-files.test.ts new file mode 100644 index 0000000..412f235 --- /dev/null +++ b/src/diff/__tests__/exclude-diff-files.test.ts @@ -0,0 +1,241 @@ +import type { File } from "parse-diff" +import { describe, expect, it } from "vitest" +import { + partitionExcludedFiles, + renderExcludedFilesNote, + type ExcludedDiffFile, +} from "../exclude-diff-files.js" + +const makeFile = (overrides: Partial = {}): File => ({ + chunks: [], + additions: 3, + deletions: 1, + from: "src/app.ts", + to: "src/app.ts", + ...overrides, +}) + +const partition = ( + files: File[], + overrides: { + defaultPatterns?: string[] + operatorPatterns?: string[] + linguistRules?: { pattern: string; generated: boolean }[] + } = {}, +) => { + return partitionExcludedFiles({ + files, + defaultPatterns: overrides.defaultPatterns ?? [], + operatorPatterns: overrides.operatorPatterns ?? [], + linguistRules: overrides.linguistRules ?? [], + }) +} + +const keptPaths = (result: { kept: File[] }): (string | undefined)[] => { + return result.kept.map((file) => file.to ?? file.from) +} + +describe("partitionExcludedFiles", () => { + it("keeps every file when no patterns or rules are configured", () => { + const files = [makeFile(), makeFile({ from: "b.ts", to: "b.ts" })] + + expect(partition(files)).toEqual({ kept: files, excluded: [] }) + }) + + it("excludes a root-anchored folder-prefix match and keeps files outside it", () => { + const generated = makeFile({ + from: "generated/api.ts", + to: "generated/api.ts", + }) + const source = makeFile() + + const result = partition([generated, source], { + operatorPatterns: ["generated"], + }) + + expect(result.kept).toEqual([source]) + expect(result.excluded).toEqual([ + { + path: "generated/api.ts", + additions: 3, + deletions: 1, + source: "operator_pattern", + }, + ]) + }) + + it("excludes a glob match at any depth and keeps non-matching siblings", () => { + const snapshot = makeFile({ + from: "src/a/__tests__/x.snap", + to: "src/a/__tests__/x.snap", + }) + const test = makeFile({ + from: "src/a/__tests__/x.test.ts", + to: "src/a/__tests__/x.test.ts", + }) + + const result = partition([snapshot, test], { + defaultPatterns: ["**/*.snap"], + }) + + expect(keptPaths(result)).toEqual(["src/a/__tests__/x.test.ts"]) + expect(result.excluded).toEqual([ + { + path: "src/a/__tests__/x.snap", + additions: 3, + deletions: 1, + source: "default_pattern", + }, + ]) + }) + + it("excludes a linguist-generated file and reports the source", () => { + const marked = makeFile({ from: "gen/x.json", to: "gen/x.json" }) + + const result = partition([marked, makeFile()], { + linguistRules: [{ pattern: "gen/*.json", generated: true }], + }) + + expect(keptPaths(result)).toEqual(["src/app.ts"]) + expect(result.excluded).toEqual([ + { + path: "gen/x.json", + additions: 3, + deletions: 1, + source: "linguist_generated", + }, + ]) + }) + + it("keeps a default-list match the repo negated in gitattributes", () => { + const lockfile = makeFile({ + from: "package-lock.json", + to: "package-lock.json", + }) + + const result = partition([lockfile], { + defaultPatterns: ["**/package-lock.json"], + linguistRules: [{ pattern: "package-lock.json", generated: false }], + }) + + expect(result).toEqual({ kept: [lockfile], excluded: [] }) + }) + + it("excludes on an operator pattern even when the repo negated the file", () => { + const lockfile = makeFile({ + from: "package-lock.json", + to: "package-lock.json", + }) + + const result = partition([lockfile], { + operatorPatterns: ["**/package-lock.json"], + linguistRules: [{ pattern: "package-lock.json", generated: false }], + }) + + expect(result.kept).toEqual([]) + expect(result.excluded).toEqual([ + { + path: "package-lock.json", + additions: 3, + deletions: 1, + source: "operator_pattern", + }, + ]) + }) + + it("judges a deleted file by its old path", () => { + const deleted = makeFile({ + from: "generated/old.ts", + to: "/dev/null", + deleted: true, + }) + + const result = partition([deleted, makeFile()], { + operatorPatterns: ["generated"], + }) + + expect(keptPaths(result)).toEqual(["src/app.ts"]) + expect(result.excluded).toEqual([ + { + path: "generated/old.ts", + additions: 3, + deletions: 1, + source: "operator_pattern", + }, + ]) + }) + + it("keeps a file renamed out of an excluded folder", () => { + const renamedOut = makeFile({ + from: "generated/api.ts", + to: "src/api.ts", + }) + + const result = partition([renamedOut], { + operatorPatterns: ["generated"], + }) + + expect(result).toEqual({ kept: [renamedOut], excluded: [] }) + }) + + it("excludes a file renamed into an excluded folder", () => { + const renamedIn = makeFile({ + from: "src/api.ts", + to: "generated/api.ts", + }) + + const result = partition([renamedIn], { + operatorPatterns: ["generated"], + }) + + expect(result.kept).toEqual([]) + expect(result.excluded).toEqual([ + { + path: "generated/api.ts", + additions: 3, + deletions: 1, + source: "operator_pattern", + }, + ]) + }) +}) + +describe("renderExcludedFilesNote", () => { + it("returns an empty string for no exclusions", () => { + expect(renderExcludedFilesNote([])).toBe("") + }) + + it("renders one line per file with change counts and source labels", () => { + const excluded: ExcludedDiffFile[] = [ + { + path: "package-lock.json", + additions: 1200, + deletions: 800, + source: "default_pattern", + }, + { + path: "evals/run.json", + additions: 10, + deletions: 0, + source: "operator_pattern", + }, + { + path: "gen/x.json", + additions: 5, + deletions: 5, + source: "linguist_generated", + }, + ] + + // Pinned format: the per-file lines must not resemble the diff's + // "=== path ===" headers, which are the model's only citable anchors + expect(renderExcludedFilesNote(excluded)).toBe( + [ + "3 changed file(s) excluded from review (content not shown):", + "- package-lock.json (+1200/-800, default exclusion)", + "- evals/run.json (+10/-0, diff_exclude_paths)", + "- gen/x.json (+5/-5, linguist-generated)", + ].join("\n"), + ) + }) +}) diff --git a/src/diff/__tests__/gitattributes.test.ts b/src/diff/__tests__/gitattributes.test.ts new file mode 100644 index 0000000..cd7fd55 --- /dev/null +++ b/src/diff/__tests__/gitattributes.test.ts @@ -0,0 +1,118 @@ +import { describe, expect, it } from "vitest" +import { createTestLogger } from "../../__tests__/test-logger.js" +import { + compileLinguistRules, + linguistGeneratedState, + parseLinguistGeneratedRules, +} from "../gitattributes.js" + +const parseRules = (content: string) => { + return parseLinguistGeneratedRules(content, createTestLogger()) +} + +const stateFor = (filePath: string, content: string): boolean | undefined => { + return linguistGeneratedState( + filePath, + compileLinguistRules(parseRules(content)), + ) +} + +describe("parseLinguistGeneratedRules", () => { + it("parses all four attribute spellings into rules", () => { + const content = [ + "a.json linguist-generated", + "b.json linguist-generated=true", + "c.json linguist-generated=false", + "d.json -linguist-generated", + ].join("\n") + + expect(parseRules(content)).toEqual([ + { pattern: "a.json", generated: true }, + { pattern: "b.json", generated: true }, + { pattern: "c.json", generated: false }, + { pattern: "d.json", generated: false }, + ]) + }) + + it("skips comments, blank lines, and lines without the attribute", () => { + const content = [ + "# generated artifacts", + "", + "*.pdf binary", + "*.snap linguist-generated=true", + ].join("\n") + + expect(parseRules(content)).toEqual([ + { pattern: "*.snap", generated: true }, + ]) + }) + + it("skips gitignore-style negation patterns, which gitattributes forbids", () => { + expect(parseRules("!*.snap linguist-generated=true")).toEqual([]) + }) + + it("drops a wildcard-cap-violating pattern with a warn and keeps the rest", () => { + const logger = createTestLogger() + const content = [ + "*a*a*a*b linguist-generated=true", + "*.snap linguist-generated=true", + ].join("\n") + + const rules = parseLinguistGeneratedRules(content, logger) + + expect(rules).toEqual([{ pattern: "*.snap", generated: true }]) + expect(logger.messages).toEqual([ + { + level: "warn", + message: + "gitattributes pattern exceeds the wildcard cap — rule ignored", + data: { pattern: "*a*a*a*b" }, + }, + ]) + }) + + it("keeps a backslash-escaped space inside the pattern token", () => { + expect(parseRules("a\\ b.json linguist-generated=true")).toEqual([ + { pattern: "a\\ b.json", generated: true }, + ]) + }) +}) + +describe("linguistGeneratedState", () => { + it("returns undefined when no rule matches", () => { + expect(stateFor("src/app.ts", "*.snap linguist-generated=true")).toBe( + undefined, + ) + }) + + it("matches a slash-less pattern against basenames at any depth", () => { + expect( + stateFor("deep/nested/x.snap", "*.snap linguist-generated=true"), + ).toBe(true) + }) + + it("applies the last matching rule when rules overlap", () => { + const content = [ + "snapshots/*.json linguist-generated=true", + "snapshots/keep.json -linguist-generated", + ].join("\n") + + expect(stateFor("snapshots/keep.json", content)).toBe(false) + expect(stateFor("snapshots/other.json", content)).toBe(true) + }) + + it("matches directory-style patterns against contained files", () => { + // Deliberate over-approximation: gitattributes itself would not apply a + // "dir/" pattern to contained paths, but excluding more than GitHub + // collapses is visible in the review output and off-switchable + expect( + stateFor( + "__snapshots__/x.json", + "__snapshots__/ linguist-generated=true", + ), + ).toBe(true) + expect( + stateFor("__snapshots__/x.json", "__snapshots__ linguist-generated=true"), + ).toBe(true) + }) +}) diff --git a/src/diff/__tests__/pattern-safety.test.ts b/src/diff/__tests__/pattern-safety.test.ts new file mode 100644 index 0000000..d5c8a67 --- /dev/null +++ b/src/diff/__tests__/pattern-safety.test.ts @@ -0,0 +1,29 @@ +import { describe, expect, it } from "vitest" +import { DEFAULT_DIFF_EXCLUDE_PATTERNS } from "../../config.js" +import { hasExcessiveWildcards } from "../pattern-safety.js" + +describe("hasExcessiveWildcards", () => { + it("accepts every shipped default pattern", () => { + // Production-consistency check across two constants, not a drift test: + // a default the cap itself would reject could never match anything + expect(DEFAULT_DIFF_EXCLUDE_PATTERNS.filter(hasExcessiveWildcards)).toEqual( + [], + ) + }) + + it("accepts globstar segments regardless of how many appear", () => { + expect(hasExcessiveWildcards("**/__snapshots__/**")).toBe(false) + }) + + it("accepts up to two stars in one segment", () => { + expect(hasExcessiveWildcards("*.min.*")).toBe(false) + }) + + it("flags a segment with more than two stars", () => { + expect(hasExcessiveWildcards("*a*a*b")).toBe(true) + }) + + it("flags a multi-star segment at any depth", () => { + expect(hasExcessiveWildcards("src/**/*a*a*a.json")).toBe(true) + }) +}) diff --git a/src/diff/exclude-diff-files.ts b/src/diff/exclude-diff-files.ts new file mode 100644 index 0000000..ee644a7 --- /dev/null +++ b/src/diff/exclude-diff-files.ts @@ -0,0 +1,153 @@ +import { posix } from "node:path" +import type { File } from "parse-diff" +import { newFilePath } from "./commentable-lines.js" +import { + compileLinguistRules, + linguistGeneratedState, + type CompiledLinguistRule, + type LinguistRule, +} from "./gitattributes.js" + +export type DiffExclusionSource = + "default_pattern" | "operator_pattern" | "linguist_generated" + +export type ExcludedDiffFile = { + path: string + additions: number + deletions: number + source: DiffExclusionSource +} + +export type PartitionedDiffFiles = { + kept: File[] + excluded: ExcludedDiffFile[] +} + +/** Operator-facing label for each exclusion source, shown in the excluded- + * files trailer and the status comment's context notes. */ +export const describeExclusionSource = ( + source: DiffExclusionSource, +): string => { + if (source === "default_pattern") return "default exclusion" + if (source === "operator_pattern") return "diff_exclude_paths" + return "linguist-generated" +} + +/** A pattern hits as a root-anchored folder prefix (the exclude_paths rule) + * or as a glob — the union keeps both operator mental models valid. */ +const matchesExcludePattern = (filePath: string, pattern: string): boolean => { + return ( + filePath === pattern || + filePath.startsWith(pattern + "/") || + posix.matchesGlob(filePath, pattern) + ) +} + +const matchesAnyPattern = (filePath: string, patterns: string[]): boolean => { + return patterns.some((pattern) => matchesExcludePattern(filePath, pattern)) +} + +/** The path a file is judged by: the new path, or the old path for + * deletions — a rename out of an excluded folder into reviewable source is + * reviewed, while a rename into one is excluded. */ +const exclusionPath = (file: File): string | null => { + const filePath = newFilePath(file) ?? file.from + if (!filePath || filePath === "/dev/null") return null + // Leading slashes are stripped because ignore().ignores() throws on + // absolute paths, and diff paths are PR-author-influenced + return posix.normalize(filePath).replace(/^\/+/, "") +} + +const resolveExclusionSource = ({ + filePath, + defaultPatterns, + operatorPatterns, + compiledRules, +}: { + filePath: string + defaultPatterns: string[] + operatorPatterns: string[] + compiledRules: CompiledLinguistRule[] +}): DiffExclusionSource | null => { + // Precedence: operator patterns are the most intentional layer and beat a + // repo's negated gitattributes entry; a negated entry in turn exempts the + // file from the built-in default list. + if (matchesAnyPattern(filePath, operatorPatterns)) return "operator_pattern" + + const generatedState = linguistGeneratedState(filePath, compiledRules) + if (generatedState === false) return null + if (generatedState === true) return "linguist_generated" + + if (matchesAnyPattern(filePath, defaultPatterns)) return "default_pattern" + return null +} + +/** + * Splits parsed diff files into the review subject and the excluded rest. + * Runs before diff annotation and the token budget check so excluded files + * consume no budget, no changed-file reads, and no commentable lines. + */ +export const partitionExcludedFiles = ({ + files, + defaultPatterns, + operatorPatterns, + linguistRules, +}: { + files: File[] + defaultPatterns: string[] + operatorPatterns: string[] + linguistRules: LinguistRule[] +}): PartitionedDiffFiles => { + const compiledRules = compileLinguistRules(linguistRules) + const kept: File[] = [] + const excluded: ExcludedDiffFile[] = [] + + for (const file of files) { + const filePath = exclusionPath(file) + if (filePath === null) { + kept.push(file) + continue + } + + const source = resolveExclusionSource({ + filePath, + defaultPatterns, + operatorPatterns, + compiledRules, + }) + if (source === null) { + kept.push(file) + continue + } + + excluded.push({ + path: filePath, + additions: file.additions, + deletions: file.deletions, + source, + }) + } + + return { kept, excluded } +} + +/** + * The changed-but-not-reviewed trailer appended after the annotated diff, so + * the model knows these files changed without seeing their content. Lines + * deliberately do not resemble the "=== path ===" file headers — the + * anchoring contract only lets the model cite real headers and file blocks. + */ +export const renderExcludedFilesNote = ( + excluded: ExcludedDiffFile[], +): string => { + if (excluded.length === 0) return "" + + const fileLines = excluded.map((file) => { + const changeCounts = `+${file.additions}/-${file.deletions}` + return `- ${file.path} (${changeCounts}, ${describeExclusionSource(file.source)})` + }) + return [ + `${excluded.length} changed file(s) excluded from review (content not shown):`, + ...fileLines, + ].join("\n") +} diff --git a/src/diff/gitattributes.ts b/src/diff/gitattributes.ts new file mode 100644 index 0000000..2930dcf --- /dev/null +++ b/src/diff/gitattributes.ts @@ -0,0 +1,107 @@ +import ignoreModule from "ignore" +import type { Logger } from "../logger.js" +import { hasExcessiveWildcards } from "./pattern-safety.js" + +/** ignore ships CommonJS with an ESM-style "export default" declaration, so + * under NodeNext the callable factory sits behind .default in both the type + * and the runtime interop (the package sets module.exports.default itself). */ +const createIgnoreMatcher = ignoreModule.default + +export type LinguistRule = { + pattern: string + generated: boolean +} + +export type CompiledLinguistRule = { + matchesPath: (filePath: string) => boolean + generated: boolean +} + +/** + * Only these spellings carry a linguist-generated signal. Git's + * "!linguist-generated" means "unspecified" and other string values have no + * defined truthiness here, so both produce no rule. + */ +const GENERATED_ATTRIBUTE_STATES = new Map([ + ["linguist-generated", true], + ["linguist-generated=true", true], + ["linguist-generated=false", false], + ["-linguist-generated", false], +]) + +/** Whitespace not preceded by a backslash — a backslash-escaped space is + * git's escaping for paths with spaces and stays inside the pattern token. */ +const UNESCAPED_WHITESPACE = /(? { + return line.split(UNESCAPED_WHITESPACE).filter((token) => token !== "") +} + +/** + * Extracts the linguist-generated rules from .gitattributes content. The + * file arrives from the PR head checkout, so it is untrusted input: a + * malformed or wildcard-cap-violating line drops that rule with a warn and + * never fails the run. + */ +export const parseLinguistGeneratedRules = ( + content: string, + logger: Logger, +): LinguistRule[] => { + const rules: LinguistRule[] = [] + + for (const rawLine of content.split("\n")) { + const line = rawLine.trim() + if (line === "" || line.startsWith("#")) continue + + const [pattern, ...attributes] = splitAttributeLine(line) + if (!pattern) continue + // gitattributes forbids gitignore-style "!" negation patterns — git + // ignores such lines, and so does this parser + if (pattern.startsWith("!")) continue + + const generatedState = attributes + .map((attribute) => GENERATED_ATTRIBUTE_STATES.get(attribute)) + .findLast((state) => state !== undefined) + if (generatedState === undefined) continue + + if (hasExcessiveWildcards(pattern)) { + logger.warn( + "gitattributes pattern exceeds the wildcard cap — rule ignored", + { pattern }, + ) + continue + } + + rules.push({ pattern, generated: generatedState }) + } + + return rules +} + +/** + * One ignore() instance per pattern — load-bearing: a shared instance would + * apply gitignore "!" negation semantics across rules, which the + * gitattributes format forbids. Per-pattern instances keep each rule an + * independent match so last-match-wins stays a plain fold over the rules. + */ +export const compileLinguistRules = ( + rules: LinguistRule[], +): CompiledLinguistRule[] => { + return rules.map((rule) => { + const matcher = createIgnoreMatcher().add(rule.pattern) + return { + matchesPath: (filePath: string) => matcher.ignores(filePath), + generated: rule.generated, + } + }) +} + +/** Last matching rule wins, per gitattributes semantics; undefined means no + * rule matched, so the caller falls through to the default pattern tier. */ +export const linguistGeneratedState = ( + filePath: string, + compiledRules: CompiledLinguistRule[], +): boolean | undefined => { + return compiledRules.filter((rule) => rule.matchesPath(filePath)).at(-1) + ?.generated +} diff --git a/src/diff/pattern-safety.ts b/src/diff/pattern-safety.ts new file mode 100644 index 0000000..06a55e1 --- /dev/null +++ b/src/diff/pattern-safety.ts @@ -0,0 +1,15 @@ +/** + * Matching engines (path.matchesGlob, the ignore package) backtrack + * exponentially when one segment interleaves several "*" wildcards with + * literals — a crafted 40+-char filename hangs a single synchronous, + * unabortable match call for minutes. Both pattern channels (operator + * input and repo .gitattributes) are bounded by this cap; "**" globstar + * segments are exempt because globstar traversal does not backtrack. + */ +export const hasExcessiveWildcards = (pattern: string): boolean => { + return pattern.split("/").some((segment) => { + if (segment === "**") return false + const starCount = (segment.match(/\*/g) ?? []).length + return starCount > 2 + }) +} diff --git a/src/main.ts b/src/main.ts index df39a46..264ca62 100644 --- a/src/main.ts +++ b/src/main.ts @@ -44,6 +44,8 @@ const collectRawInputs = (): RawInputs => ({ maxRelatedDocs: core.getInput("max_related_docs"), priorityDocs: core.getInput("priority_docs"), excludePaths: core.getInput("exclude_paths"), + diffExcludePaths: core.getInput("diff_exclude_paths"), + respectLinguistGenerated: core.getBooleanInput("respect_linguist_generated"), costSummary: core.getBooleanInput("cost_summary"), prNumberOverride: core.getInput("pr_number"), }) diff --git a/src/orchestrate.ts b/src/orchestrate.ts index 3ee6d70..c203ca4 100644 --- a/src/orchestrate.ts +++ b/src/orchestrate.ts @@ -6,6 +6,11 @@ import { newFilePath, } from "./diff/commentable-lines.js" import { annotateDiff } from "./diff/annotate-diff.js" +import { + partitionExcludedFiles, + renderExcludedFilesNote, +} from "./diff/exclude-diff-files.js" +import { parseLinguistGeneratedRules } from "./diff/gitattributes.js" import { describeError, type Logger } from "./logger.js" import type { CheckRunConclusion, @@ -519,8 +524,42 @@ const runReviewPipeline = async ( return postSkipReview("empty diff") } - // Step 6: annotate + token check - const annotatedDiff = annotateDiff(files) + // Step 5.5: diff-level exclusion — generated files leave the review + // subject before the budget check so one oversized artifact cannot + // starve the reviewable rest of the PR + const gitAttributesContent = config.respectLinguistGenerated + ? await contextReader.readGitAttributes() + : null + const linguistRules = gitAttributesContent + ? parseLinguistGeneratedRules(gitAttributesContent, logger) + : [] + const { kept: reviewableFiles, excluded: excludedDiffFiles } = + partitionExcludedFiles({ + files, + defaultPatterns: config.diffExcludePaths.defaultPatterns, + operatorPatterns: config.diffExcludePaths.operatorPatterns, + linguistRules, + }) + if (excludedDiffFiles.length > 0) { + logger.info("changed files excluded from the review diff", { + excludedCount: excludedDiffFiles.length, + excludedPaths: excludedDiffFiles + .map((file) => `${file.path} (${file.source})`) + .join(", "), + }) + } + if (reviewableFiles.length === 0) { + return postSkipReview( + `all ${files.length} changed files match diff_exclude_paths`, + ) + } + + // Step 6: annotate + token check. The excluded-files trailer sits inside + // the annotated diff string, so its (few) tokens debit the diff budget. + const excludedFilesNote = renderExcludedFilesNote(excludedDiffFiles) + const annotatedDiff = excludedFilesNote + ? `${annotateDiff(reviewableFiles)}\n\n${excludedFilesNote}` + : annotateDiff(reviewableFiles) const diffTokens = estimateTokens(annotatedDiff) const budgetHalf = Math.floor(config.contextBudgetTokens / 2) if (diffTokens > budgetHalf) { @@ -530,11 +569,11 @@ const runReviewPipeline = async ( } // Step 7: commentable lines - const commentableByPath = computeCommentableLines(files) + const commentableByPath = computeCommentableLines(reviewableFiles) // Step 8: extract changed paths (includes old path for renames so the // import scanner finds callers that still reference the pre-rename path) - const changedPaths = files + const changedPaths = reviewableFiles .flatMap((file) => { const toPath = newFilePath(file) const isRename = @@ -670,7 +709,7 @@ const runReviewPipeline = async ( // Every path the model can see: diff headers (deleted files render a // header but have no new path, so they are added here), file blocks, and // the conventions section when the file was found. - const deletedPaths = files.flatMap((file) => { + const deletedPaths = reviewableFiles.flatMap((file) => { return file.deleted && file.from ? [file.from] : [] }) const promptFilePaths = [ @@ -728,6 +767,7 @@ const runReviewPipeline = async ( priorityDocsRead: priorityDocFiles, relatedFilesExcludedPaths: relatedFilesResult.excludedByCapPaths, docsExcludedPaths: mentionMatchedDocsResult.excludedByCapPaths, + diffExcludedFiles: excludedDiffFiles, }) // Step 9.5: fetch prior bot comments — needed both for the prompt (the diff --git a/src/review/__tests__/context-notes.test.ts b/src/review/__tests__/context-notes.test.ts index 62e1719..bde51c2 100644 --- a/src/review/__tests__/context-notes.test.ts +++ b/src/review/__tests__/context-notes.test.ts @@ -10,6 +10,7 @@ const makeInput = ( priorityDocsRead: [], relatedFilesExcludedPaths: [], docsExcludedPaths: [], + diffExcludedFiles: [], ...overrides, }) @@ -164,13 +165,52 @@ describe("buildContextNotes", () => { ]) }) - it("orders in-context before not-included before related files before related docs", () => { + it("reports diff-excluded files with each file's exclusion source", () => { + const notes = buildContextNotes( + makeInput({ + diffExcludedFiles: [ + { + path: "package-lock.json", + additions: 1200, + deletions: 800, + source: "default_pattern", + }, + { + path: "evals/run.json", + additions: 10, + deletions: 0, + source: "operator_pattern", + }, + { + path: "gen/x.json", + additions: 5, + deletions: 5, + source: "linguist_generated", + }, + ], + }), + ) + + expect(notes).toEqual([ + "3 changed file(s) excluded from review: `package-lock.json` (default exclusion), `evals/run.json` (diff_exclude_paths), `gen/x.json` (linguist-generated)", + ]) + }) + + it("orders in-context before not-included before related files before related docs before diff exclusions", () => { const notes = buildContextNotes( makeInput({ priorityDocs: ["README.md", "MISSING.md"], priorityDocsInContext: ["README.md"], relatedFilesExcludedPaths: ["src/extra-a.ts"], docsExcludedPaths: ["docs/overflow.md"], + diffExcludedFiles: [ + { + path: "package-lock.json", + additions: 1, + deletions: 1, + source: "default_pattern", + }, + ], }), ) @@ -179,6 +219,7 @@ describe("buildContextNotes", () => { notIncludedNote("`MISSING.md`"), "1 related file(s) excluded by `max_related_files` cap: `src/extra-a.ts`", "1 related doc(s) excluded by `max_related_docs` cap: `docs/overflow.md`", + "1 changed file(s) excluded from review: `package-lock.json` (default exclusion)", ]) }) }) diff --git a/src/review/context-notes.ts b/src/review/context-notes.ts index 33093ef..5dfc252 100644 --- a/src/review/context-notes.ts +++ b/src/review/context-notes.ts @@ -1,4 +1,8 @@ import { posix } from "node:path" +import { + describeExclusionSource, + type ExcludedDiffFile, +} from "../diff/exclude-diff-files.js" import type { PromptFile } from "./prompt.js" export type ContextNotesInput = { @@ -11,6 +15,7 @@ export type ContextNotesInput = { priorityDocsRead: PromptFile[] relatedFilesExcludedPaths: string[] docsExcludedPaths: string[] + diffExcludedFiles: ExcludedDiffFile[] } /** Paths arrive from three sources that spell them differently — action @@ -22,6 +27,10 @@ const normalizePath = (filePath: string): string => posix.normalize(filePath) const renderPaths = (paths: string[]): string => paths.map((filePath) => `\`${filePath}\``).join(", ") +const renderExcludedFile = (file: ExcludedDiffFile): string => { + return `\`${file.path}\` (${describeExclusionSource(file.source)})` +} + /** Priority docs satisfied by a higher-priority channel (changed files, * related files, conventions) — their full text already reached the prompt * so the priority-doc reader skipped them. Returns the configured spelling, @@ -86,6 +95,7 @@ export const buildContextNotes = ({ priorityDocsRead, relatedFilesExcludedPaths, docsExcludedPaths, + diffExcludedFiles, }: ContextNotesInput): string[] => { const inContextDocs = findInContextPriorityDocs({ priorityDocs, @@ -113,11 +123,16 @@ export const buildContextNotes = ({ docsExcludedPaths.length === 0 ? null : `${docsExcludedPaths.length} related doc(s) excluded by \`max_related_docs\` cap: ${renderPaths(docsExcludedPaths)}` + const diffExcludedNote = + diffExcludedFiles.length === 0 + ? null + : `${diffExcludedFiles.length} changed file(s) excluded from review: ${diffExcludedFiles.map(renderExcludedFile).join(", ")}` return [ inContextNote, priorityDocsNote, relatedFilesNote, relatedDocsNote, + diffExcludedNote, ].filter((note) => note !== null) } From 268a239e886ba91c7c0953bf119aa6a17a33fff1 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 6 Sep 2026 21:38:58 +0000 Subject: [PATCH 02/13] fix: keep diff-excluded files out of related scans, name skip sources - Diff-excluded changed files are excluded from the related-file and related-doc scans so their content cannot re-enter the prompt after the partition removed them from the review subject - The all-excluded skip attributes each exclusion to the layer that actually excluded it and lists every excluded file in the skip body - Port the Prettier CHANGELOG.md fix from main (the release flow committed an unformatted entry, failing the checks job on every PR) Co-Authored-By: Claude Fable 5 --- CHANGELOG.md | 1 - src/__tests__/orchestrate.test.ts | 73 +++++++++++++++++-- src/context/__tests__/workspace.test.ts | 40 ++++++++++ src/context/workspace.ts | 10 +++ src/diff/__tests__/exclude-diff-files.test.ts | 49 +++++++++++++ src/diff/exclude-diff-files.ts | 36 +++++++-- src/orchestrate.ts | 53 ++++++++++---- 7 files changed, 236 insertions(+), 26 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a02de3d..aed29c4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,5 @@ # Changelog - ## [0.4.0] — 2026-09-05 ### Features diff --git a/src/__tests__/orchestrate.test.ts b/src/__tests__/orchestrate.test.ts index 24f48eb..15f6e93 100644 --- a/src/__tests__/orchestrate.test.ts +++ b/src/__tests__/orchestrate.test.ts @@ -209,8 +209,16 @@ const expectedStatus = ({ }), }) -const buildSkipBody = (reason: string): string => - `**umm-actually** — review skipped\n\n${reason}\n\n---\n*umm-actually*` +const buildSkipBody = ({ + reason, + detail, +}: { + reason: string + detail?: string +}): string => { + const detailSection = detail ? `\n\n${detail}` : "" + return `**umm-actually** — review skipped\n\n${reason}${detailSection}\n\n---\n*umm-actually*` +} const baseConfig: ActionConfig = { githubToken: "ghp_test", @@ -263,6 +271,7 @@ type ReadChangedFilesParams = { type FindRelatedFilesParams = { changedPaths: string[] budgetTokens: number + excludePaths: string[] } type RequestReviewParams = { @@ -554,7 +563,7 @@ describe("orchestrate", () => { expect(first(stubs.submitReviewCalls)).toEqual({ prNumber: fixturePrContext.prNumber, commitId: fixturePrContext.headSha, - body: buildSkipBody(skipReason), + body: buildSkipBody({ reason: skipReason }), }) }) @@ -583,7 +592,7 @@ describe("orchestrate", () => { expect(first(stubs.submitReviewCalls)).toEqual({ prNumber: fixturePrContext.prNumber, commitId: fixturePrContext.headSha, - body: buildSkipBody(skipReason), + body: buildSkipBody({ reason: skipReason }), }) }) @@ -611,7 +620,7 @@ describe("orchestrate", () => { expect(first(stubs.submitReviewCalls)).toEqual({ prNumber: fixturePrContext.prNumber, commitId: fixturePrContext.headSha, - body: buildSkipBody(skipReason), + body: buildSkipBody({ reason: skipReason }), }) }) @@ -628,7 +637,8 @@ describe("orchestrate", () => { const result = await orchestrate(stubs.deps, logger) - const skipReason = "all 6 changed files match diff_exclude_paths" + const skipReason = + "all 6 changed file(s) excluded from review (6 by diff_exclude_paths)" expect(result).toEqual({ findingsCount: 0, reviewUrl: "https://github.com/test/review/1", @@ -644,10 +654,38 @@ describe("orchestrate", () => { expect(first(stubs.submitReviewCalls)).toEqual({ prNumber: fixturePrContext.prNumber, commitId: fixturePrContext.headSha, - body: buildSkipBody(skipReason), + body: buildSkipBody({ + reason: skipReason, + detail: [ + "- src/greeter.ts (+4/-1, diff_exclude_paths)", + "- src/added-file.ts (+3/-0, diff_exclude_paths)", + "- src/removed-file.ts (+0/-3, diff_exclude_paths)", + "- src/new-name.ts (+1/-1, diff_exclude_paths)", + "- assets/logo.png (+0/-0, diff_exclude_paths)", + "- src/no-trailing-newline.ts (+1/-1, diff_exclude_paths)", + ].join("\n"), + }), }) }) + it("attributes the all-excluded skip to the default list when no operator pattern is set", async () => { + const stubs = makeOrchestrateDeps({ + config: { + diffExcludePaths: { + defaultPatterns: ["src/**", "assets/**"], + operatorPatterns: [], + }, + }, + }) + const logger = createTestLogger() + + const result = await orchestrate(stubs.deps, logger) + + expect(result.skippedReason).toBe( + "all 6 changed file(s) excluded from review (6 by default exclusion)", + ) + }) + it("removes an excluded file from the annotated diff, context reads, and changed paths", async () => { const stubs = makeOrchestrateDeps({ config: { @@ -706,6 +744,27 @@ describe("orchestrate", () => { ]) }) + it("passes diff-excluded paths to the related-file and doc scans as exclusions", async () => { + const stubs = makeOrchestrateDeps({ + config: { + diffExcludePaths: { + defaultPatterns: [], + operatorPatterns: ["assets/**"], + }, + }, + }) + const logger = createTestLogger() + + await orchestrate(stubs.deps, logger) + + expect(first(stubs.findRelatedFilesCalls).excludePaths).toEqual([ + "assets/logo.png", + ]) + expect(first(stubs.findRelatedDocsCalls).excludePaths).toEqual([ + "assets/logo.png", + ]) + }) + it("passes the budget check when the oversized files are all excluded", async () => { // Budget 200 (half = 100) fails against the full fixture diff (341 // tokens); with every src/ file excluded only the binary asset header diff --git a/src/context/__tests__/workspace.test.ts b/src/context/__tests__/workspace.test.ts index 154b37e..88bebe5 100644 --- a/src/context/__tests__/workspace.test.ts +++ b/src/context/__tests__/workspace.test.ts @@ -526,6 +526,7 @@ describe("findRelatedFiles", () => { const relatedFiles = await contextReader.findRelatedFiles({ changedPaths: ["src/greeter.ts", "src/registry.ts"], budgetTokens: 100_000, + excludePaths: [], }) expect(relatedFiles.files).toEqual([ @@ -556,6 +557,7 @@ describe("findRelatedFiles", () => { const relatedFiles = await contextReader.findRelatedFiles({ changedPaths: ["src/lib/index.ts"], budgetTokens: 100_000, + excludePaths: [], }) expect(relatedFiles.files).toEqual([ @@ -574,6 +576,7 @@ describe("findRelatedFiles", () => { const relatedFiles = await contextReader.findRelatedFiles({ changedPaths: ["src/greeter.ts", "src/caller.ts"], budgetTokens: 100_000, + excludePaths: [], }) expect(relatedFiles.files.map((relatedFile) => relatedFile.path)).toEqual([ @@ -588,6 +591,7 @@ describe("findRelatedFiles", () => { const relatedFiles = await contextReader.findRelatedFiles({ changedPaths: ["src/hub.ts"], budgetTokens: 100_000, + excludePaths: [], }) expect(relatedFiles.files.map((relatedFile) => relatedFile.path)).toEqual([ @@ -608,6 +612,7 @@ describe("findRelatedFiles", () => { const relatedFiles = await contextReader.findRelatedFiles({ changedPaths: ["src/greeter.ts", "src/registry.ts"], budgetTokens: estimateTokens(consumerContent), + excludePaths: [], }) expect(relatedFiles.files).toEqual([ @@ -634,6 +639,7 @@ describe("findRelatedFiles", () => { const relatedFiles = await contextReader.findRelatedFiles({ changedPaths: ["target.ts"], budgetTokens: 1_000_000, + excludePaths: [], }) expect(relatedFiles.files.map((relatedFile) => relatedFile.path)).toEqual( @@ -662,6 +668,7 @@ describe("findRelatedFiles", () => { const relatedFiles = await contextReader.findRelatedFiles({ changedPaths: ["target.ts"], budgetTokens: 100_000, + excludePaths: [], }) expect(relatedFiles.files.map((relatedFile) => relatedFile.path)).toEqual( @@ -687,6 +694,7 @@ describe("findRelatedFiles", () => { const relatedFiles = await contextReader.findRelatedFiles({ changedPaths: ["target.ts"], budgetTokens: 100_000, + excludePaths: [], }) // readable.ts still arriving proves the scan carried on past the failure @@ -722,6 +730,7 @@ describe("findRelatedFiles", () => { const relatedFiles = await contextReader.findRelatedFiles({ changedPaths: ["target.ts"], budgetTokens: 100_000, + excludePaths: [], }) expect(relatedFiles.files.map((relatedFile) => relatedFile.path)).toEqual( @@ -747,6 +756,7 @@ describe("findRelatedFiles", () => { await contextReader.findRelatedFiles({ changedPaths: ["filler-00000.ts"], budgetTokens: 100_000, + excludePaths: [], }) expect(logger.messages).toContainEqual({ @@ -777,6 +787,34 @@ describe("findRelatedFiles", () => { const relatedFiles = await contextReader.findRelatedFiles({ changedPaths: ["target.ts"], budgetTokens: 100_000, + excludePaths: [], + }) + + expect(relatedFiles.files.map((file) => file.path)).toEqual([ + "src/legit.ts", + ]) + } finally { + await cleanup() + } + }) + + it("excludes importers whose exact paths are listed in the excludePaths param", async () => { + const importTarget = `import { target } from "../target.js"\nexport const found = target\n` + const { root, cleanup } = await makeTempWorkspace({ + "target.ts": `export const target = "target"\n`, + "src/legit.ts": importTarget, + "generated/api-client.ts": importTarget, + }) + const contextReader = createContextReader( + defaultConfig(root), + createTestLogger(), + ) + + try { + const relatedFiles = await contextReader.findRelatedFiles({ + changedPaths: ["target.ts"], + budgetTokens: 100_000, + excludePaths: ["generated/api-client.ts"], }) expect(relatedFiles.files.map((file) => file.path)).toEqual([ @@ -803,6 +841,7 @@ describe("findRelatedFiles", () => { const relatedFiles = await contextReader.findRelatedFiles({ changedPaths: ["target.ts"], budgetTokens: 100_000, + excludePaths: [], }) expect(relatedFiles.files.map((file) => file.path)).toEqual([ @@ -831,6 +870,7 @@ describe("findRelatedFiles", () => { const relatedFiles = await contextReader.findRelatedFiles({ changedPaths: ["target.ts"], budgetTokens: 100_000, + excludePaths: [], }) expect(relatedFiles.files.map((file) => file.path)).toEqual([ diff --git a/src/context/workspace.ts b/src/context/workspace.ts index 2511e7b..d5c9ee0 100644 --- a/src/context/workspace.ts +++ b/src/context/workspace.ts @@ -40,6 +40,7 @@ export type ContextReader = { findRelatedFiles: (params: { changedPaths: string[] budgetTokens: number + excludePaths: string[] }) => Promise readPriorityDocs: (params: { priorityDocs: string[] @@ -468,19 +469,28 @@ export const createContextReader = ( const scanWorkspaceSourceFiles = (): Promise => scanWorkspaceFiles(SCANNABLE_EXTENSIONS) + /** excludePaths carries the diff-excluded changed files: their content + * must not re-enter the prompt through the import trace after the diff + * partition removed them from the review subject. */ const findRelatedFiles = async ({ changedPaths, budgetTokens, + excludePaths, }: { changedPaths: string[] budgetTokens: number + excludePaths: string[] }): Promise => { const changedPathSet = new Set(changedPaths) + const excludePathSet = new Set( + excludePaths.map((excludePath) => posix.normalize(excludePath)), + ) const scannedPaths = await scanWorkspaceSourceFiles() const importers: ImporterCandidate[] = [] for (const scannedPath of scannedPaths) { if (changedPathSet.has(scannedPath)) continue + if (excludePathSet.has(posix.normalize(scannedPath))) continue const content = await readScannedFileOrNull(scannedPath) if (!content) continue // A NUL byte marks binary content — the same exclusion readChangedFiles diff --git a/src/diff/__tests__/exclude-diff-files.test.ts b/src/diff/__tests__/exclude-diff-files.test.ts index 412f235..348f55a 100644 --- a/src/diff/__tests__/exclude-diff-files.test.ts +++ b/src/diff/__tests__/exclude-diff-files.test.ts @@ -3,6 +3,7 @@ import { describe, expect, it } from "vitest" import { partitionExcludedFiles, renderExcludedFilesNote, + summarizeExclusionSources, type ExcludedDiffFile, } from "../exclude-diff-files.js" @@ -239,3 +240,51 @@ describe("renderExcludedFilesNote", () => { ) }) }) + +describe("summarizeExclusionSources", () => { + it("counts each source with operator patterns first and defaults last", () => { + const excluded: ExcludedDiffFile[] = [ + { + path: "package-lock.json", + additions: 1, + deletions: 1, + source: "default_pattern", + }, + { + path: "yarn.lock", + additions: 1, + deletions: 1, + source: "default_pattern", + }, + { + path: "evals/run.json", + additions: 1, + deletions: 1, + source: "operator_pattern", + }, + { + path: "gen/x.json", + additions: 1, + deletions: 1, + source: "linguist_generated", + }, + ] + + expect(summarizeExclusionSources(excluded)).toBe( + "1 by diff_exclude_paths, 1 by linguist-generated, 2 by default exclusion", + ) + }) + + it("omits sources that excluded nothing", () => { + const excluded: ExcludedDiffFile[] = [ + { + path: "package-lock.json", + additions: 1, + deletions: 1, + source: "default_pattern", + }, + ] + + expect(summarizeExclusionSources(excluded)).toBe("1 by default exclusion") + }) +}) diff --git a/src/diff/exclude-diff-files.ts b/src/diff/exclude-diff-files.ts index ee644a7..86762eb 100644 --- a/src/diff/exclude-diff-files.ts +++ b/src/diff/exclude-diff-files.ts @@ -131,6 +131,36 @@ export const partitionExcludedFiles = ({ return { kept, excluded } } +/** One line per excluded file with change counts and the source that + * excluded it — shared by the prompt trailer and the all-excluded skip + * review so both surfaces name the same facts identically. */ +export const renderExcludedFileLines = ( + excluded: ExcludedDiffFile[], +): string[] => { + return excluded.map((file) => { + const changeCounts = `+${file.additions}/-${file.deletions}` + return `- ${file.path} (${changeCounts}, ${describeExclusionSource(file.source)})` + }) +} + +const SOURCE_SUMMARY_ORDER: DiffExclusionSource[] = [ + "operator_pattern", + "linguist_generated", + "default_pattern", +] + +/** Per-source counts for one-line surfaces (check-run title, skip reason) — + * attributes the exclusion to the layer that actually caused it instead of + * naming an input the operator may never have set. */ +export const summarizeExclusionSources = ( + excluded: ExcludedDiffFile[], +): string => { + return SOURCE_SUMMARY_ORDER.flatMap((source) => { + const count = excluded.filter((file) => file.source === source).length + return count > 0 ? [`${count} by ${describeExclusionSource(source)}`] : [] + }).join(", ") +} + /** * The changed-but-not-reviewed trailer appended after the annotated diff, so * the model knows these files changed without seeing their content. Lines @@ -142,12 +172,8 @@ export const renderExcludedFilesNote = ( ): string => { if (excluded.length === 0) return "" - const fileLines = excluded.map((file) => { - const changeCounts = `+${file.additions}/-${file.deletions}` - return `- ${file.path} (${changeCounts}, ${describeExclusionSource(file.source)})` - }) return [ `${excluded.length} changed file(s) excluded from review (content not shown):`, - ...fileLines, + ...renderExcludedFileLines(excluded), ].join("\n") } diff --git a/src/orchestrate.ts b/src/orchestrate.ts index c203ca4..36f85a9 100644 --- a/src/orchestrate.ts +++ b/src/orchestrate.ts @@ -8,7 +8,9 @@ import { import { annotateDiff } from "./diff/annotate-diff.js" import { partitionExcludedFiles, + renderExcludedFileLines, renderExcludedFilesNote, + summarizeExclusionSources, } from "./diff/exclude-diff-files.js" import { parseLinguistGeneratedRules } from "./diff/gitattributes.js" import { describeError, type Logger } from "./logger.js" @@ -130,8 +132,18 @@ const PRIOR_COMMENT_CAP = 30 const stripAnchorComment = (body: string): string => body.replace(/\n*\s*$/, "") -const buildSkipBody = (reason: string): string => - `**umm-actually** — review skipped\n\n${reason}\n\n---\n*umm-actually*` +/** detail carries multi-line context (e.g. the excluded-file list) that + * belongs in the review body but not in the one-line check-run title. */ +const buildSkipBody = ({ + reason, + detail, +}: { + reason: string + detail?: string | undefined +}): string => { + const detailSection = detail ? `\n\n${detail}` : "" + return `**umm-actually** — review skipped\n\n${reason}${detailSection}\n\n---\n*umm-actually*` +} type InlineCommentState = { anchors: AnchorEntry[] @@ -499,8 +511,14 @@ const runReviewPipeline = async ( ): Promise => { const { config, githubClient, contextReader, generateFindings } = deps - const postSkipReview = async (reason: string): Promise => { - const body = buildSkipBody(reason) + const postSkipReview = async ({ + reason, + detail, + }: { + reason: string + detail?: string | undefined + }): Promise => { + const body = buildSkipBody({ reason, detail }) const { url } = await githubClient.submitReview({ prNumber: prContext.prNumber, commitId: prContext.headSha, @@ -515,13 +533,13 @@ const runReviewPipeline = async ( prNumber: prContext.prNumber, }) if (diffResult.kind === "too_large") { - return postSkipReview("diff exceeds GitHub's diff API limits") + return postSkipReview({ reason: "diff exceeds GitHub's diff API limits" }) } // Step 5: parse diff const files = parseDiff(diffResult.diff) if (files.length === 0) { - return postSkipReview("empty diff") + return postSkipReview({ reason: "empty diff" }) } // Step 5.5: diff-level exclusion — generated files leave the review @@ -549,9 +567,12 @@ const runReviewPipeline = async ( }) } if (reviewableFiles.length === 0) { - return postSkipReview( - `all ${files.length} changed files match diff_exclude_paths`, - ) + // Reason names the layers that actually excluded (an operator on default + // inputs never set diff_exclude_paths); the body names every file. + return postSkipReview({ + reason: `all ${excludedDiffFiles.length} changed file(s) excluded from review (${summarizeExclusionSources(excludedDiffFiles)})`, + detail: renderExcludedFileLines(excludedDiffFiles).join("\n"), + }) } // Step 6: annotate + token check. The excluded-files trailer sits inside @@ -563,14 +584,19 @@ const runReviewPipeline = async ( const diffTokens = estimateTokens(annotatedDiff) const budgetHalf = Math.floor(config.contextBudgetTokens / 2) if (diffTokens > budgetHalf) { - return postSkipReview( - `diff too large for context budget (${diffTokens} tokens, limit ${budgetHalf} of ${config.contextBudgetTokens})`, - ) + return postSkipReview({ + reason: `diff too large for context budget (${diffTokens} tokens, limit ${budgetHalf} of ${config.contextBudgetTokens})`, + }) } // Step 7: commentable lines const commentableByPath = computeCommentableLines(reviewableFiles) + // Diff-excluded files must stay out of every context channel: the trailer + // told the model their content is not shown, so the related-file and doc + // scans may not pull that content back into the prompt. + const diffExcludedPaths = excludedDiffFiles.map((file) => file.path) + // Step 8: extract changed paths (includes old path for renames so the // import scanner finds callers that still reference the pre-rename path) const changedPaths = reviewableFiles @@ -639,6 +665,7 @@ const runReviewPipeline = async ( ? await contextReader.findRelatedFiles({ changedPaths, budgetTokens: relatedFilesBudgetTokens, + excludePaths: diffExcludedPaths, }) : { files: [], excludedByCapPaths: [] } @@ -700,7 +727,7 @@ const runReviewPipeline = async ( changedPaths, budgetTokens: docRemainingTokens, conventionsFile: config.conventionsFile, - excludePaths: config.priorityDocs, + excludePaths: [...config.priorityDocs, ...diffExcludedPaths], }) : { files: [], excludedByCapPaths: [] } From 4999c976a223184e91932ee29457b2e7a1668b8c Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 6 Sep 2026 22:04:00 +0000 Subject: [PATCH 03/13] test: pin **/ default-pattern matching for root-level files MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit matchesGlob's globstar matches zero directories, so the shipped **/-prefixed defaults catch the standard npm layout's root lockfile — pinned so a future matcher swap cannot regress it silently. Co-Authored-By: Claude Fable 5 --- src/diff/__tests__/exclude-diff-files.test.ts | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/src/diff/__tests__/exclude-diff-files.test.ts b/src/diff/__tests__/exclude-diff-files.test.ts index 348f55a..40b3050 100644 --- a/src/diff/__tests__/exclude-diff-files.test.ts +++ b/src/diff/__tests__/exclude-diff-files.test.ts @@ -1,5 +1,6 @@ import type { File } from "parse-diff" import { describe, expect, it } from "vitest" +import { DEFAULT_DIFF_EXCLUDE_PATTERNS } from "../../config.js" import { partitionExcludedFiles, renderExcludedFilesNote, @@ -65,6 +66,30 @@ describe("partitionExcludedFiles", () => { ]) }) + it("excludes a root-level lockfile via the shipped **/ default patterns", () => { + // Pins matchesGlob's "**/ matches zero directories" semantics: the + // shipped defaults must catch the standard npm layout's root lockfile + const lockfile = makeFile({ + from: "package-lock.json", + to: "package-lock.json", + }) + const source = makeFile() + + const result = partition([lockfile, source], { + defaultPatterns: DEFAULT_DIFF_EXCLUDE_PATTERNS, + }) + + expect(result.kept).toEqual([source]) + expect(result.excluded).toEqual([ + { + path: "package-lock.json", + additions: 3, + deletions: 1, + source: "default_pattern", + }, + ]) + }) + it("excludes a glob match at any depth and keeps non-matching siblings", () => { const snapshot = makeFile({ from: "src/a/__tests__/x.snap", From c54973dab703eb08311824a754fbc7912bcc84b9 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 6 Sep 2026 22:21:50 +0000 Subject: [PATCH 04/13] chore: exclude CHANGELOG.md from Prettier The release flow's update-changelog.sh writes it directly to main without a format pass, so checking it fails every PR's checks job after a release until someone hand-formats a generated file. Co-Authored-By: Claude Fable 5 --- .prettierignore | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 .prettierignore diff --git a/.prettierignore b/.prettierignore new file mode 100644 index 0000000..3cd13b2 --- /dev/null +++ b/.prettierignore @@ -0,0 +1,4 @@ +# Written by the release flow's update-changelog.sh, not by hand — its +# formatting is the generator's business, and checking it turns every PR +# red when a release commit lands unformatted on main +CHANGELOG.md From 9d873a0c22f61a4f015b1f76c751f8a64ceabadb Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 6 Sep 2026 22:22:40 +0000 Subject: [PATCH 05/13] chore: exclude package-lock.json from Prettier npm's own output format is the source of truth for the lockfile; formatting it invites churn against what the tool regenerates. Co-Authored-By: Claude Fable 5 --- .prettierignore | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.prettierignore b/.prettierignore index 3cd13b2..f2937e7 100644 --- a/.prettierignore +++ b/.prettierignore @@ -2,3 +2,6 @@ # formatting is the generator's business, and checking it turns every PR # red when a release commit lands unformatted on main CHANGELOG.md +# npm's output format is the source of truth; reformatting a lockfile +# invites churn against what the tool regenerates +package-lock.json From 628fa02b8eb7a80b335049cb665c736e54826052 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 6 Sep 2026 22:52:13 +0000 Subject: [PATCH 06/13] refactor: one casing convention for exclusion-source labels Labels are lowercase noun phrases naming the layer, with the external identifier (input name, gitattributes attribute) verbatim: built-in default list, diff_exclude_paths input, linguist-generated attribute. Co-Authored-By: Claude Fable 5 --- src/__tests__/orchestrate.test.ts | 30 +++++++++---------- src/diff/__tests__/exclude-diff-files.test.ts | 12 ++++---- src/diff/exclude-diff-files.ts | 8 +++-- src/review/__tests__/context-notes.test.ts | 4 +-- 4 files changed, 29 insertions(+), 25 deletions(-) diff --git a/src/__tests__/orchestrate.test.ts b/src/__tests__/orchestrate.test.ts index 15f6e93..8e862b0 100644 --- a/src/__tests__/orchestrate.test.ts +++ b/src/__tests__/orchestrate.test.ts @@ -638,7 +638,7 @@ describe("orchestrate", () => { const result = await orchestrate(stubs.deps, logger) const skipReason = - "all 6 changed file(s) excluded from review (6 by diff_exclude_paths)" + "all 6 changed file(s) excluded from review (6 by diff_exclude_paths input)" expect(result).toEqual({ findingsCount: 0, reviewUrl: "https://github.com/test/review/1", @@ -657,12 +657,12 @@ describe("orchestrate", () => { body: buildSkipBody({ reason: skipReason, detail: [ - "- src/greeter.ts (+4/-1, diff_exclude_paths)", - "- src/added-file.ts (+3/-0, diff_exclude_paths)", - "- src/removed-file.ts (+0/-3, diff_exclude_paths)", - "- src/new-name.ts (+1/-1, diff_exclude_paths)", - "- assets/logo.png (+0/-0, diff_exclude_paths)", - "- src/no-trailing-newline.ts (+1/-1, diff_exclude_paths)", + "- src/greeter.ts (+4/-1, diff_exclude_paths input)", + "- src/added-file.ts (+3/-0, diff_exclude_paths input)", + "- src/removed-file.ts (+0/-3, diff_exclude_paths input)", + "- src/new-name.ts (+1/-1, diff_exclude_paths input)", + "- assets/logo.png (+0/-0, diff_exclude_paths input)", + "- src/no-trailing-newline.ts (+1/-1, diff_exclude_paths input)", ].join("\n"), }), }) @@ -682,7 +682,7 @@ describe("orchestrate", () => { const result = await orchestrate(stubs.deps, logger) expect(result.skippedReason).toBe( - "all 6 changed file(s) excluded from review (6 by default exclusion)", + "all 6 changed file(s) excluded from review (6 by built-in default list)", ) }) @@ -704,7 +704,7 @@ describe("orchestrate", () => { ) const expectedExcludedNote = [ "1 changed file(s) excluded from review (content not shown):", - "- assets/logo.png (+0/-0, diff_exclude_paths)", + "- assets/logo.png (+0/-0, diff_exclude_paths input)", ].join("\n") const reviewContext = first(stubs.generateFindingsCalls) expect(reviewContext.annotatedDiff).toBe( @@ -738,7 +738,7 @@ describe("orchestrate", () => { postedCount: expectedSelection.selected.length, totalCount: expectedSelection.selected.length, contextNotes: [ - "1 changed file(s) excluded from review: `assets/logo.png` (diff_exclude_paths)", + "1 changed file(s) excluded from review: `assets/logo.png` (diff_exclude_paths input)", ], }), ]) @@ -766,12 +766,12 @@ describe("orchestrate", () => { }) it("passes the budget check when the oversized files are all excluded", async () => { - // Budget 200 (half = 100) fails against the full fixture diff (341 + // Budget 220 (half = 110) fails against the full fixture diff (341 // tokens); with every src/ file excluded only the binary asset header - // and the excluded-files trailer remain (94 tokens), which fit + // and the excluded-files trailer remain (101 tokens), which fit const stubs = makeOrchestrateDeps({ config: { - contextBudgetTokens: 200, + contextBudgetTokens: 220, diffExcludePaths: { defaultPatterns: [], operatorPatterns: ["src/**"], @@ -784,7 +784,7 @@ describe("orchestrate", () => { // Guard for bar 2: the unfiltered fixture diff must exceed the half // budget, or this test would pass without the exclusion doing anything - expect(sampleDiffTokens).toBeGreaterThan(100) + expect(sampleDiffTokens).toBeGreaterThan(110) expect(result.skippedReason).toBe("") expect(stubs.generateFindingsCalls).toHaveLength(1) }) @@ -838,7 +838,7 @@ describe("orchestrate", () => { ) const expectedExcludedNote = [ "1 changed file(s) excluded from review (content not shown):", - "- assets/logo.png (+0/-0, linguist-generated)", + "- assets/logo.png (+0/-0, linguist-generated attribute)", ].join("\n") expect(first(stubs.generateFindingsCalls).annotatedDiff).toBe( `${annotateDiff(keptFiles)}\n\n${expectedExcludedNote}`, diff --git a/src/diff/__tests__/exclude-diff-files.test.ts b/src/diff/__tests__/exclude-diff-files.test.ts index 40b3050..3efa83f 100644 --- a/src/diff/__tests__/exclude-diff-files.test.ts +++ b/src/diff/__tests__/exclude-diff-files.test.ts @@ -258,9 +258,9 @@ describe("renderExcludedFilesNote", () => { expect(renderExcludedFilesNote(excluded)).toBe( [ "3 changed file(s) excluded from review (content not shown):", - "- package-lock.json (+1200/-800, default exclusion)", - "- evals/run.json (+10/-0, diff_exclude_paths)", - "- gen/x.json (+5/-5, linguist-generated)", + "- package-lock.json (+1200/-800, built-in default list)", + "- evals/run.json (+10/-0, diff_exclude_paths input)", + "- gen/x.json (+5/-5, linguist-generated attribute)", ].join("\n"), ) }) @@ -296,7 +296,7 @@ describe("summarizeExclusionSources", () => { ] expect(summarizeExclusionSources(excluded)).toBe( - "1 by diff_exclude_paths, 1 by linguist-generated, 2 by default exclusion", + "1 by diff_exclude_paths input, 1 by linguist-generated attribute, 2 by built-in default list", ) }) @@ -310,6 +310,8 @@ describe("summarizeExclusionSources", () => { }, ] - expect(summarizeExclusionSources(excluded)).toBe("1 by default exclusion") + expect(summarizeExclusionSources(excluded)).toBe( + "1 by built-in default list", + ) }) }) diff --git a/src/diff/exclude-diff-files.ts b/src/diff/exclude-diff-files.ts index 86762eb..179a06d 100644 --- a/src/diff/exclude-diff-files.ts +++ b/src/diff/exclude-diff-files.ts @@ -28,9 +28,11 @@ export type PartitionedDiffFiles = { export const describeExclusionSource = ( source: DiffExclusionSource, ): string => { - if (source === "default_pattern") return "default exclusion" - if (source === "operator_pattern") return "diff_exclude_paths" - return "linguist-generated" + // One convention across labels: a lowercase noun phrase naming the layer, + // with the external identifier (input name, gitattributes attribute) verbatim + if (source === "default_pattern") return "built-in default list" + if (source === "operator_pattern") return "diff_exclude_paths input" + return "linguist-generated attribute" } /** A pattern hits as a root-anchored folder prefix (the exclude_paths rule) diff --git a/src/review/__tests__/context-notes.test.ts b/src/review/__tests__/context-notes.test.ts index bde51c2..9b4da5d 100644 --- a/src/review/__tests__/context-notes.test.ts +++ b/src/review/__tests__/context-notes.test.ts @@ -192,7 +192,7 @@ describe("buildContextNotes", () => { ) expect(notes).toEqual([ - "3 changed file(s) excluded from review: `package-lock.json` (default exclusion), `evals/run.json` (diff_exclude_paths), `gen/x.json` (linguist-generated)", + "3 changed file(s) excluded from review: `package-lock.json` (built-in default list), `evals/run.json` (diff_exclude_paths input), `gen/x.json` (linguist-generated attribute)", ]) }) @@ -219,7 +219,7 @@ describe("buildContextNotes", () => { notIncludedNote("`MISSING.md`"), "1 related file(s) excluded by `max_related_files` cap: `src/extra-a.ts`", "1 related doc(s) excluded by `max_related_docs` cap: `docs/overflow.md`", - "1 changed file(s) excluded from review: `package-lock.json` (default exclusion)", + "1 changed file(s) excluded from review: `package-lock.json` (built-in default list)", ]) }) }) From e9343b1557af105fc2f7c2a8e3ac229d75bc82fd Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 6 Sep 2026 23:06:38 +0000 Subject: [PATCH 07/13] fix: scope the 'none' docs claim and cap the .gitattributes read MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - README and action.yml said a bare 'none' disables exclusion; it only drops the built-in default list — linguist-generated exclusions are governed by respect_linguist_generated. Docs now say so. - readGitAttributes stats before reading and degrades to no rules past maxScanBytes, mirroring the layer's other stat-first guards — the file is PR-author-controlled and read on every run. Co-Authored-By: Claude Fable 5 --- README.md | 46 ++++++++++++------------- action.yml | 6 ++-- src/context/__tests__/workspace.test.ts | 26 ++++++++++++++ src/context/workspace.ts | 12 +++++++ 4 files changed, 65 insertions(+), 25 deletions(-) diff --git a/README.md b/README.md index 46213f6..1fc25d6 100644 --- a/README.md +++ b/README.md @@ -75,29 +75,29 @@ The `@umm review` comment trigger lets you re-request a review on any PR by comm ## Inputs -| Input | Default | Description | -| ---------------------------- | ----------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| `github_token` | _(required)_ | Token for fetching the diff and posting the review. A GitHub App installation token keeps the bot identity. | -| `openrouter_api_key` | _(required)_ | OpenRouter API key | -| `model` | `anthropic/claude-sonnet-4-6` | OpenRouter model slug exactly as listed on openrouter.ai/models | -| `fallback_model` | `""` | Model to retry with if the primary model fails the structured-output ladder | -| `request_timeout_seconds` | `900` | Per-attempt deadline for a single model request, in seconds. When it elapses the attempt is recorded as `timeout` and the retry/fallback ladder advances whether or not the provider connection closes; the HTTP call is aborted best-effort. A request the provider keeps serving past the deadline is still billed, and its cost-summary row shows no cost | -| `max_findings` | `""` _(uncapped)_ | Cap on posted findings, highest severity first. Empty = all validated findings post. | -| `severity_threshold` | `low` | Minimum severity to post: `low` \| `medium` \| `high` \| `critical` | -| `conventions_file` | `AGENTS.md` | Repo-relative path to the conventions file included in the prompt (truncated at ~8000 tokens). When the file also changed in the PR, deduplication ensures its full text appears exactly once across context channels | -| `phases` | `combined` | How the review dimensions are dispatched: `combined` (one model call carrying every dimension), `parallel` (three focused calls at once — each reads deeper than the single call; wall clock is the slowest of the three and prompt tokens roughly triple), or `sequential` (the same three calls in order, each seeing the earlier findings). Findings two phases report on the same lines collapse to one, keeping the higher severity. Empty = `combined`, so workflows can wire an unset repo variable directly | -| `context_budget_tokens` | `300000` | Approximate token budget for prompt context (file contents + diff — conventions have a separate cap) | -| `trace_related_files` | `true` | Enable heuristic context scanning — import-tracing for caller regressions and mention-matching for doc staleness detection. Does not affect `priority_docs` | -| `priority_docs` | `README.md` | Comma-separated repo-relative paths included in review context with a reserved 10% budget floor, independent of `trace_related_files`. Docs already present in full from other context channels are not re-read; a diff-only changed file is still read here so its full text reaches the model. Subject to the shared doc token budget (the floor prevents starvation by related files but does not guarantee inclusion when the budget is exhausted by the diff and changed files themselves). Empty = disabled | -| `max_scan_files` | `5000` | Maximum files to walk during workspace scan for related file and doc detection | -| `max_scan_bytes` | `262144` | Maximum byte size of a single file to include in the workspace scan | -| `max_related_files` | `8` | Maximum import-traced related files to include in review context | -| `max_related_docs` | `4` | Maximum mention-matched documentation files to include in review context (excludes priority docs) | -| `exclude_paths` | `""` | Comma-separated folder prefixes excluded from the workspace scan. Files under these paths are invisible to import-tracing and doc-mention matching. Changed files in the PR diff and `priority_docs` are never excluded. Example: `evals, fixtures, __snapshots__` | -| `diff_exclude_paths` | `""` _(built-in list)_ | Comma-separated folder prefixes or globs removed from the review diff before the token budget check. Excluded files are listed by name in the review output, but their content is not reviewed — excluded is not vetted. Supplied patterns extend a built-in default list of generated artifacts (ecosystem lockfiles, `*.min.js`, `*.min.css`, `*.map` — the classes GitHub's linguist auto-collapses; snapshots are deliberately not defaulted, since GitHub renders them expanded). A leading `none` drops the defaults: `none` alone disables exclusion, `none, evals/**` replaces the list. Unlike `exclude_paths`, empty means the default list, not "no exclusions". Patterns with more than 2 `*` in one path segment are rejected (`**` segments exempt) — glob matching backtracks exponentially on such shapes | -| `respect_linguist_generated` | `true` | Also exclude changed files the repo's root `.gitattributes` marks `linguist-generated=true` (nested `.gitattributes` files are not read). Negated entries (`-linguist-generated`) keep a file reviewable even when the default `diff_exclude_paths` list matches it; explicitly supplied `diff_exclude_paths` patterns always win. Rules are read from the PR head, so a PR changing `.gitattributes` reviews under its own rules — every exclusion is named in the review output | -| `cost_summary` | `true` | Write a per-run cost report (model, prompt/completion tokens, USD) to the workflow step summary | -| `pr_number` | `""` | PR number override — required only when the triggering event does not identify a PR directly | +| Input | Default | Description | +| ---------------------------- | ----------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | +| `github_token` | _(required)_ | Token for fetching the diff and posting the review. A GitHub App installation token keeps the bot identity. | +| `openrouter_api_key` | _(required)_ | OpenRouter API key | +| `model` | `anthropic/claude-sonnet-4-6` | OpenRouter model slug exactly as listed on openrouter.ai/models | +| `fallback_model` | `""` | Model to retry with if the primary model fails the structured-output ladder | +| `request_timeout_seconds` | `900` | Per-attempt deadline for a single model request, in seconds. When it elapses the attempt is recorded as `timeout` and the retry/fallback ladder advances whether or not the provider connection closes; the HTTP call is aborted best-effort. A request the provider keeps serving past the deadline is still billed, and its cost-summary row shows no cost | +| `max_findings` | `""` _(uncapped)_ | Cap on posted findings, highest severity first. Empty = all validated findings post. | +| `severity_threshold` | `low` | Minimum severity to post: `low` \| `medium` \| `high` \| `critical` | +| `conventions_file` | `AGENTS.md` | Repo-relative path to the conventions file included in the prompt (truncated at ~8000 tokens). When the file also changed in the PR, deduplication ensures its full text appears exactly once across context channels | +| `phases` | `combined` | How the review dimensions are dispatched: `combined` (one model call carrying every dimension), `parallel` (three focused calls at once — each reads deeper than the single call; wall clock is the slowest of the three and prompt tokens roughly triple), or `sequential` (the same three calls in order, each seeing the earlier findings). Findings two phases report on the same lines collapse to one, keeping the higher severity. Empty = `combined`, so workflows can wire an unset repo variable directly | +| `context_budget_tokens` | `300000` | Approximate token budget for prompt context (file contents + diff — conventions have a separate cap) | +| `trace_related_files` | `true` | Enable heuristic context scanning — import-tracing for caller regressions and mention-matching for doc staleness detection. Does not affect `priority_docs` | +| `priority_docs` | `README.md` | Comma-separated repo-relative paths included in review context with a reserved 10% budget floor, independent of `trace_related_files`. Docs already present in full from other context channels are not re-read; a diff-only changed file is still read here so its full text reaches the model. Subject to the shared doc token budget (the floor prevents starvation by related files but does not guarantee inclusion when the budget is exhausted by the diff and changed files themselves). Empty = disabled | +| `max_scan_files` | `5000` | Maximum files to walk during workspace scan for related file and doc detection | +| `max_scan_bytes` | `262144` | Maximum byte size of a single file to include in the workspace scan | +| `max_related_files` | `8` | Maximum import-traced related files to include in review context | +| `max_related_docs` | `4` | Maximum mention-matched documentation files to include in review context (excludes priority docs) | +| `exclude_paths` | `""` | Comma-separated folder prefixes excluded from the workspace scan. Files under these paths are invisible to import-tracing and doc-mention matching. Changed files in the PR diff and `priority_docs` are never excluded. Example: `evals, fixtures, __snapshots__` | +| `diff_exclude_paths` | `""` _(built-in list)_ | Comma-separated folder prefixes or globs removed from the review diff before the token budget check. Excluded files are listed by name in the review output, but their content is not reviewed — excluded is not vetted. Supplied patterns extend a built-in default list of generated artifacts (ecosystem lockfiles, `*.min.js`, `*.min.css`, `*.map` — the classes GitHub's linguist auto-collapses; snapshots are deliberately not defaulted, since GitHub renders them expanded). A leading `none` drops the defaults: `none` alone disables the built-in list (`.gitattributes` linguist-generated exclusions are governed separately by `respect_linguist_generated`), `none, evals/**` replaces the list. Unlike `exclude_paths`, empty means the default list, not "no exclusions". Patterns with more than 2 `*` in one path segment are rejected (`**` segments exempt) — glob matching backtracks exponentially on such shapes | +| `respect_linguist_generated` | `true` | Also exclude changed files the repo's root `.gitattributes` marks `linguist-generated=true` (nested `.gitattributes` files are not read). Negated entries (`-linguist-generated`) keep a file reviewable even when the default `diff_exclude_paths` list matches it; explicitly supplied `diff_exclude_paths` patterns always win. Rules are read from the PR head, so a PR changing `.gitattributes` reviews under its own rules — every exclusion is named in the review output | +| `cost_summary` | `true` | Write a per-run cost report (model, prompt/completion tokens, USD) to the workflow step summary | +| `pr_number` | `""` | PR number override — required only when the triggering event does not identify a PR directly | ## Outputs diff --git a/action.yml b/action.yml index 1bd637c..01461fe 100644 --- a/action.yml +++ b/action.yml @@ -86,8 +86,10 @@ inputs: vetted). Supplied patterns EXTEND a built-in default list of generated artifacts (ecosystem lockfiles, *.min.js, *.min.css, *.map — the classes GitHub's linguist auto-collapses); a leading - "none" drops the defaults, so "none" alone disables exclusion and - "none, evals/**" replaces the list outright. Empty = the default + "none" drops the defaults, so "none" alone disables the built-in + list (linguist-generated exclusions are governed separately by + respect_linguist_generated) and "none, evals/**" replaces the list + outright. Empty = the default list (unlike exclude_paths, where empty means no exclusions), so workflows can wire an unset repo variable directly. Patterns with more than 2 "*" in one path segment are rejected ("**" segments are diff --git a/src/context/__tests__/workspace.test.ts b/src/context/__tests__/workspace.test.ts index 88bebe5..0a3fec6 100644 --- a/src/context/__tests__/workspace.test.ts +++ b/src/context/__tests__/workspace.test.ts @@ -176,6 +176,32 @@ describe("readGitAttributes", () => { expect(logger.messages).toEqual([]) }) + it("warns and returns null when .gitattributes exceeds the scan size cap", async () => { + const oversizedContent = "*.snap linguist-generated=true\n".repeat(4) + const { root, cleanup } = await makeTempWorkspace({ + ".gitattributes": oversizedContent, + }) + const logger = createTestLogger() + const contextReader = createContextReader( + { ...defaultConfig(root), maxScanBytes: 16 }, + logger, + ) + + try { + const content = await contextReader.readGitAttributes() + + expect(content).toBeNull() + expect(logger.messages).toContainEqual({ + level: "warn", + message: + ".gitattributes exceeds the scan size cap — linguist-generated rules unavailable", + data: { bytes: oversizedContent.length, maxScanBytes: 16 }, + }) + } finally { + await cleanup() + } + }) + it("warns and returns null when .gitattributes symlinks outside the workspace", async () => { // The outside target must exist: a dangling link would reject with ENOENT // (the silent missing-file path) and pass this test without the guard diff --git a/src/context/workspace.ts b/src/context/workspace.ts index d5c9ee0..097380b 100644 --- a/src/context/workspace.ts +++ b/src/context/workspace.ts @@ -218,6 +218,18 @@ export const createContextReader = ( ) return null } + // Stat before reading: the file is PR-author-controlled and read on + // every run, so an oversized commit must degrade like the other + // failures instead of being pulled into memory whole. A failed stat + // falls through to the read path, which already logs per error kind. + const fileStats = await stat(safePath).catch(() => null) + if (fileStats !== null && fileStats.size > config.maxScanBytes) { + logger.warn( + ".gitattributes exceeds the scan size cap — linguist-generated rules unavailable", + { bytes: fileStats.size, maxScanBytes: config.maxScanBytes }, + ) + return null + } return await readFile(safePath, "utf8") } catch (readError) { if (isMissingFileError(readError)) return null From 5d4e6bb2fc70d413c4b9feecf2774e16a7a5ffcc Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 6 Sep 2026 23:21:16 +0000 Subject: [PATCH 08/13] refactor: exact-pin ignore, truthy checks where falsy suffices MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - ignore 5.3.2 pinned exact in package.json (was ^5.3.2) - Null/empty comparisons that a falsy check covers become truthy checks (partition guards, gitattributes line filter, config entry filter, fileStats stat guards); comparisons where a falsy value is legitimate (a linguist-generated=false rule, index 0, an empty changed file) stay explicit - The changed-paths filter drops its type-guard annotation — it masked an undefined member the rename branch could add; the branch now narrows via a fromPath const so the bare null check types honestly Co-Authored-By: Claude Fable 5 --- package-lock.json | 32 +------------------------------- package.json | 2 +- src/config.ts | 2 +- src/context/workspace.ts | 9 +++------ src/diff/exclude-diff-files.ts | 4 ++-- src/diff/gitattributes.ts | 4 ++-- src/orchestrate.ts | 11 ++++++----- 7 files changed, 16 insertions(+), 48 deletions(-) diff --git a/package-lock.json b/package-lock.json index 1c4c7a9..21261f7 100644 --- a/package-lock.json +++ b/package-lock.json @@ -13,7 +13,7 @@ "@actions/github": "9.1.1", "@openrouter/sdk": "1.2.85", "env-var": "7.5.0", - "ignore": "^5.3.2", + "ignore": "5.3.2", "luxon": "3.7.2", "parse-diff": "0.12.0", "zod": "4.5.4" @@ -555,9 +555,6 @@ "arm64" ], "dev": true, - "libc": [ - "glibc" - ], "license": "MIT", "optional": true, "os": [ @@ -575,9 +572,6 @@ "arm64" ], "dev": true, - "libc": [ - "musl" - ], "license": "MIT", "optional": true, "os": [ @@ -595,9 +589,6 @@ "ppc64" ], "dev": true, - "libc": [ - "glibc" - ], "license": "MIT", "optional": true, "os": [ @@ -615,9 +606,6 @@ "s390x" ], "dev": true, - "libc": [ - "glibc" - ], "license": "MIT", "optional": true, "os": [ @@ -635,9 +623,6 @@ "x64" ], "dev": true, - "libc": [ - "glibc" - ], "license": "MIT", "optional": true, "os": [ @@ -655,9 +640,6 @@ "x64" ], "dev": true, - "libc": [ - "musl" - ], "license": "MIT", "optional": true, "os": [ @@ -1897,9 +1879,6 @@ "arm64" ], "dev": true, - "libc": [ - "glibc" - ], "license": "MPL-2.0", "optional": true, "os": [ @@ -1921,9 +1900,6 @@ "arm64" ], "dev": true, - "libc": [ - "musl" - ], "license": "MPL-2.0", "optional": true, "os": [ @@ -1945,9 +1921,6 @@ "x64" ], "dev": true, - "libc": [ - "glibc" - ], "license": "MPL-2.0", "optional": true, "os": [ @@ -1969,9 +1942,6 @@ "x64" ], "dev": true, - "libc": [ - "musl" - ], "license": "MPL-2.0", "optional": true, "os": [ diff --git a/package.json b/package.json index 88a8a69..6bf46f3 100644 --- a/package.json +++ b/package.json @@ -34,7 +34,7 @@ "@actions/github": "9.1.1", "@openrouter/sdk": "1.2.85", "env-var": "7.5.0", - "ignore": "^5.3.2", + "ignore": "5.3.2", "luxon": "3.7.2", "parse-diff": "0.12.0", "zod": "4.5.4" diff --git a/src/config.ts b/src/config.ts index e602c24..24cae65 100644 --- a/src/config.ts +++ b/src/config.ts @@ -88,7 +88,7 @@ const diffExcludePathsInput = z.string().transform((value, ctx) => { const entries = value .split(",") .map((entry) => entry.trim()) - .filter((entry) => entry !== "") + .filter(Boolean) const defaultsDisabled = entries[0] === "none" const patternEntries = defaultsDisabled ? entries.slice(1) : entries diff --git a/src/context/workspace.ts b/src/context/workspace.ts index 097380b..fe8d1c5 100644 --- a/src/context/workspace.ts +++ b/src/context/workspace.ts @@ -223,7 +223,7 @@ export const createContextReader = ( // failures instead of being pulled into memory whole. A failed stat // falls through to the read path, which already logs per error kind. const fileStats = await stat(safePath).catch(() => null) - if (fileStats !== null && fileStats.size > config.maxScanBytes) { + if (fileStats && fileStats.size > config.maxScanBytes) { logger.warn( ".gitattributes exceeds the scan size cap — linguist-generated rules unavailable", { bytes: fileStats.size, maxScanBytes: config.maxScanBytes }, @@ -323,7 +323,7 @@ export const createContextReader = ( // skip it without pulling it into memory. A failed stat falls through to // the read path, which already logs and degrades per error kind. const fileStats = await stat(safePath).catch(() => null) - if (fileStats !== null && fileStats.size > maxBytes) { + if (fileStats && fileStats.size > maxBytes) { logger.warn( "priority doc exceeds remaining context budget — skipping", { @@ -388,10 +388,7 @@ export const createContextReader = ( // A failed stat (e.g. deleted file) falls through to the read path, // which already logs and degrades per error kind. const fileStats = await stat(absolutePath).catch(() => null) - if ( - fileStats !== null && - fileStats.size > remainingTokens * CHARS_PER_TOKEN - ) { + if (fileStats && fileStats.size > remainingTokens * CHARS_PER_TOKEN) { files.push({ path: changedPath, content: "", includedAs: "diff-only" }) continue } diff --git a/src/diff/exclude-diff-files.ts b/src/diff/exclude-diff-files.ts index 179a06d..0e9f4fe 100644 --- a/src/diff/exclude-diff-files.ts +++ b/src/diff/exclude-diff-files.ts @@ -106,7 +106,7 @@ export const partitionExcludedFiles = ({ for (const file of files) { const filePath = exclusionPath(file) - if (filePath === null) { + if (!filePath) { kept.push(file) continue } @@ -117,7 +117,7 @@ export const partitionExcludedFiles = ({ operatorPatterns, compiledRules, }) - if (source === null) { + if (!source) { kept.push(file) continue } diff --git a/src/diff/gitattributes.ts b/src/diff/gitattributes.ts index 2930dcf..e0f7a0f 100644 --- a/src/diff/gitattributes.ts +++ b/src/diff/gitattributes.ts @@ -34,7 +34,7 @@ const GENERATED_ATTRIBUTE_STATES = new Map([ const UNESCAPED_WHITESPACE = /(? { - return line.split(UNESCAPED_WHITESPACE).filter((token) => token !== "") + return line.split(UNESCAPED_WHITESPACE).filter(Boolean) } /** @@ -51,7 +51,7 @@ export const parseLinguistGeneratedRules = ( for (const rawLine of content.split("\n")) { const line = rawLine.trim() - if (line === "" || line.startsWith("#")) continue + if (!line || line.startsWith("#")) continue const [pattern, ...attributes] = splitAttributeLine(line) if (!pattern) continue diff --git a/src/orchestrate.ts b/src/orchestrate.ts index 36f85a9..44cc962 100644 --- a/src/orchestrate.ts +++ b/src/orchestrate.ts @@ -602,14 +602,15 @@ const runReviewPipeline = async ( const changedPaths = reviewableFiles .flatMap((file) => { const toPath = newFilePath(file) + const fromPath = file.from const isRename = toPath !== null && - file.from !== undefined && - file.from !== "/dev/null" && - file.from !== file.to - return isRename ? [toPath, file.from] : [toPath] + fromPath !== undefined && + fromPath !== "/dev/null" && + fromPath !== file.to + return isRename ? [toPath, fromPath] : [toPath] }) - .filter((path): path is string => path !== null) + .filter((path) => path !== null) // Step 9: context reads const conventions = await contextReader.readConventions({ From cc36c37265889b78bea559c1ff5fcde8d34ceab0 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 6 Sep 2026 23:22:02 +0000 Subject: [PATCH 09/13] chore: restore lockfile platform metadata dropped by older npm The pin commit regenerated the lockfile with an older npm that strips the libc fields newer npm writes; only the ignore spec line changes. Co-Authored-By: Claude Fable 5 --- package-lock.json | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/package-lock.json b/package-lock.json index 21261f7..fb69304 100644 --- a/package-lock.json +++ b/package-lock.json @@ -555,6 +555,9 @@ "arm64" ], "dev": true, + "libc": [ + "glibc" + ], "license": "MIT", "optional": true, "os": [ @@ -572,6 +575,9 @@ "arm64" ], "dev": true, + "libc": [ + "musl" + ], "license": "MIT", "optional": true, "os": [ @@ -589,6 +595,9 @@ "ppc64" ], "dev": true, + "libc": [ + "glibc" + ], "license": "MIT", "optional": true, "os": [ @@ -606,6 +615,9 @@ "s390x" ], "dev": true, + "libc": [ + "glibc" + ], "license": "MIT", "optional": true, "os": [ @@ -623,6 +635,9 @@ "x64" ], "dev": true, + "libc": [ + "glibc" + ], "license": "MIT", "optional": true, "os": [ @@ -640,6 +655,9 @@ "x64" ], "dev": true, + "libc": [ + "musl" + ], "license": "MIT", "optional": true, "os": [ @@ -1879,6 +1897,9 @@ "arm64" ], "dev": true, + "libc": [ + "glibc" + ], "license": "MPL-2.0", "optional": true, "os": [ @@ -1900,6 +1921,9 @@ "arm64" ], "dev": true, + "libc": [ + "musl" + ], "license": "MPL-2.0", "optional": true, "os": [ @@ -1921,6 +1945,9 @@ "x64" ], "dev": true, + "libc": [ + "glibc" + ], "license": "MPL-2.0", "optional": true, "os": [ @@ -1942,6 +1969,9 @@ "x64" ], "dev": true, + "libc": [ + "musl" + ], "license": "MPL-2.0", "optional": true, "os": [ From ec0f613668764a14f67850800b6d5d3f49f0b2ca Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 7 Sep 2026 03:23:08 +0000 Subject: [PATCH 10/13] refactor: merge diff exclusion into one module with a compiled matcher MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The exclusion logic lived in three chain-importing files (pattern safety, gitattributes, partition/render), and partitionExcludedFiles trampolined three config params it never used into a per-file resolver. One module now owns the whole concern, and createExclusionMatcher compiles patterns and linguist rules once into classify(path) — the partition takes files plus one collaborator, and orchestrate's wiring drops its parse intermediates. Wrapper-only helpers (matchesAnyPattern, splitAttributeLine, the separately exported compile/evaluate pair) are inlined; behavior is unchanged and every test ported. Co-Authored-By: Claude Fable 5 --- src/config.ts | 2 +- ...e-diff-files.test.ts => exclusion.test.ts} | 192 ++++++++++-- src/diff/__tests__/gitattributes.test.ts | 118 -------- src/diff/__tests__/pattern-safety.test.ts | 29 -- src/diff/exclude-diff-files.ts | 181 ----------- src/diff/exclusion.ts | 284 ++++++++++++++++++ src/diff/gitattributes.ts | 107 ------- src/diff/pattern-safety.ts | 15 - src/orchestrate.ts | 18 +- src/review/context-notes.ts | 2 +- 10 files changed, 467 insertions(+), 481 deletions(-) rename src/diff/__tests__/{exclude-diff-files.test.ts => exclusion.test.ts} (56%) delete mode 100644 src/diff/__tests__/gitattributes.test.ts delete mode 100644 src/diff/__tests__/pattern-safety.test.ts delete mode 100644 src/diff/exclude-diff-files.ts create mode 100644 src/diff/exclusion.ts delete mode 100644 src/diff/gitattributes.ts delete mode 100644 src/diff/pattern-safety.ts diff --git a/src/config.ts b/src/config.ts index 24cae65..332633c 100644 --- a/src/config.ts +++ b/src/config.ts @@ -1,4 +1,4 @@ -import { hasExcessiveWildcards } from "./diff/pattern-safety.js" +import { hasExcessiveWildcards } from "./diff/exclusion.js" import { normalizeWorkspacePath } from "./review/workspace-path.js" import { z } from "zod" diff --git a/src/diff/__tests__/exclude-diff-files.test.ts b/src/diff/__tests__/exclusion.test.ts similarity index 56% rename from src/diff/__tests__/exclude-diff-files.test.ts rename to src/diff/__tests__/exclusion.test.ts index 3efa83f..1e5c9a4 100644 --- a/src/diff/__tests__/exclude-diff-files.test.ts +++ b/src/diff/__tests__/exclusion.test.ts @@ -1,12 +1,15 @@ import type { File } from "parse-diff" import { describe, expect, it } from "vitest" import { DEFAULT_DIFF_EXCLUDE_PATTERNS } from "../../config.js" +import { createTestLogger } from "../../__tests__/test-logger.js" import { + createExclusionMatcher, + hasExcessiveWildcards, partitionExcludedFiles, renderExcludedFilesNote, summarizeExclusionSources, type ExcludedDiffFile, -} from "../exclude-diff-files.js" +} from "../exclusion.js" const makeFile = (overrides: Partial = {}): File => ({ chunks: [], @@ -17,26 +20,179 @@ const makeFile = (overrides: Partial = {}): File => ({ ...overrides, }) -const partition = ( - files: File[], - overrides: { - defaultPatterns?: string[] - operatorPatterns?: string[] - linguistRules?: { pattern: string; generated: boolean }[] - } = {}, -) => { - return partitionExcludedFiles({ - files, - defaultPatterns: overrides.defaultPatterns ?? [], - operatorPatterns: overrides.operatorPatterns ?? [], - linguistRules: overrides.linguistRules ?? [], - }) +type MatcherOverrides = { + defaultPatterns?: string[] + operatorPatterns?: string[] + gitAttributesContent?: string +} + +const makeMatcher = (overrides: MatcherOverrides = {}) => { + return createExclusionMatcher( + { + defaultPatterns: overrides.defaultPatterns ?? [], + operatorPatterns: overrides.operatorPatterns ?? [], + gitAttributesContent: overrides.gitAttributesContent ?? null, + }, + createTestLogger(), + ) +} + +const partition = (files: File[], overrides: MatcherOverrides = {}) => { + return partitionExcludedFiles({ files, matcher: makeMatcher(overrides) }) } const keptPaths = (result: { kept: File[] }): (string | undefined)[] => { return result.kept.map((file) => file.to ?? file.from) } +describe("hasExcessiveWildcards", () => { + it("accepts every shipped default pattern", () => { + // Production-consistency check across two constants, not a drift test: + // a default the cap itself would reject could never match anything + expect(DEFAULT_DIFF_EXCLUDE_PATTERNS.filter(hasExcessiveWildcards)).toEqual( + [], + ) + }) + + it("accepts globstar segments regardless of how many appear", () => { + expect(hasExcessiveWildcards("**/__snapshots__/**")).toBe(false) + }) + + it("accepts up to two stars in one segment", () => { + expect(hasExcessiveWildcards("*.min.*")).toBe(false) + }) + + it("flags a segment with more than two stars", () => { + expect(hasExcessiveWildcards("*a*a*b")).toBe(true) + }) + + it("flags a multi-star segment at any depth", () => { + expect(hasExcessiveWildcards("src/**/*a*a*a.json")).toBe(true) + }) +}) + +describe("createExclusionMatcher — gitattributes rules", () => { + it("honors all four attribute spellings", () => { + const matcher = makeMatcher({ + // c.json and d.json also sit on the default list so the negative + // spellings are observable as exemptions, not merely as no-rule + defaultPatterns: ["**/c.json", "**/d.json"], + gitAttributesContent: [ + "a.json linguist-generated", + "b.json linguist-generated=true", + "c.json linguist-generated=false", + "d.json -linguist-generated", + ].join("\n"), + }) + + expect(matcher.classify("a.json")).toBe("linguist_generated") + expect(matcher.classify("b.json")).toBe("linguist_generated") + expect(matcher.classify("c.json")).toBeNull() + expect(matcher.classify("d.json")).toBeNull() + }) + + it("ignores comments, blank lines, and lines without the attribute", () => { + const matcher = makeMatcher({ + gitAttributesContent: [ + "# generated artifacts", + "", + "*.pdf binary", + "*.snap linguist-generated=true", + ].join("\n"), + }) + + expect(matcher.classify("x.pdf")).toBeNull() + expect(matcher.classify("x.snap")).toBe("linguist_generated") + }) + + it("ignores gitignore-style negation patterns, which gitattributes forbids", () => { + const matcher = makeMatcher({ + gitAttributesContent: "!*.snap linguist-generated=true", + }) + + expect(matcher.classify("x.snap")).toBeNull() + }) + + it("drops a wildcard-cap-violating rule with a warn and keeps the rest", () => { + const logger = createTestLogger() + const matcher = createExclusionMatcher( + { + defaultPatterns: [], + operatorPatterns: [], + gitAttributesContent: [ + "*a*a*a*b linguist-generated=true", + "*.snap linguist-generated=true", + ].join("\n"), + }, + logger, + ) + + expect(matcher.classify("aaaab")).toBeNull() + expect(matcher.classify("x.snap")).toBe("linguist_generated") + expect(logger.messages).toEqual([ + { + level: "warn", + message: + "gitattributes pattern exceeds the wildcard cap — rule ignored", + data: { pattern: "*a*a*a*b" }, + }, + ]) + }) + + it("matches a backslash-escaped space as a literal space in the path", () => { + const matcher = makeMatcher({ + gitAttributesContent: "a\\ b.json linguist-generated=true", + }) + + expect(matcher.classify("a b.json")).toBe("linguist_generated") + }) + + it("returns null when no rule matches", () => { + const matcher = makeMatcher({ + gitAttributesContent: "*.snap linguist-generated=true", + }) + + expect(matcher.classify("src/app.ts")).toBeNull() + }) + + it("matches a slash-less pattern against basenames at any depth", () => { + const matcher = makeMatcher({ + gitAttributesContent: "*.snap linguist-generated=true", + }) + + expect(matcher.classify("deep/nested/x.snap")).toBe("linguist_generated") + }) + + it("applies the last matching rule when rules overlap", () => { + const matcher = makeMatcher({ + gitAttributesContent: [ + "snapshots/*.json linguist-generated=true", + "snapshots/keep.json -linguist-generated", + ].join("\n"), + }) + + expect(matcher.classify("snapshots/keep.json")).toBeNull() + expect(matcher.classify("snapshots/other.json")).toBe("linguist_generated") + }) + + it("matches directory-style patterns against contained files", () => { + // Deliberate over-approximation: gitattributes itself would not apply a + // "dir/" pattern to contained paths, but excluding more than GitHub + // collapses is visible in the review output and off-switchable + const trailingSlash = makeMatcher({ + gitAttributesContent: "__snapshots__/ linguist-generated=true", + }) + const bareName = makeMatcher({ + gitAttributesContent: "__snapshots__ linguist-generated=true", + }) + + expect(trailingSlash.classify("__snapshots__/x.json")).toBe( + "linguist_generated", + ) + expect(bareName.classify("__snapshots__/x.json")).toBe("linguist_generated") + }) +}) + describe("partitionExcludedFiles", () => { it("keeps every file when no patterns or rules are configured", () => { const files = [makeFile(), makeFile({ from: "b.ts", to: "b.ts" })] @@ -119,7 +275,7 @@ describe("partitionExcludedFiles", () => { const marked = makeFile({ from: "gen/x.json", to: "gen/x.json" }) const result = partition([marked, makeFile()], { - linguistRules: [{ pattern: "gen/*.json", generated: true }], + gitAttributesContent: "gen/*.json linguist-generated=true", }) expect(keptPaths(result)).toEqual(["src/app.ts"]) @@ -141,7 +297,7 @@ describe("partitionExcludedFiles", () => { const result = partition([lockfile], { defaultPatterns: ["**/package-lock.json"], - linguistRules: [{ pattern: "package-lock.json", generated: false }], + gitAttributesContent: "package-lock.json -linguist-generated", }) expect(result).toEqual({ kept: [lockfile], excluded: [] }) @@ -155,7 +311,7 @@ describe("partitionExcludedFiles", () => { const result = partition([lockfile], { operatorPatterns: ["**/package-lock.json"], - linguistRules: [{ pattern: "package-lock.json", generated: false }], + gitAttributesContent: "package-lock.json -linguist-generated", }) expect(result.kept).toEqual([]) diff --git a/src/diff/__tests__/gitattributes.test.ts b/src/diff/__tests__/gitattributes.test.ts deleted file mode 100644 index cd7fd55..0000000 --- a/src/diff/__tests__/gitattributes.test.ts +++ /dev/null @@ -1,118 +0,0 @@ -import { describe, expect, it } from "vitest" -import { createTestLogger } from "../../__tests__/test-logger.js" -import { - compileLinguistRules, - linguistGeneratedState, - parseLinguistGeneratedRules, -} from "../gitattributes.js" - -const parseRules = (content: string) => { - return parseLinguistGeneratedRules(content, createTestLogger()) -} - -const stateFor = (filePath: string, content: string): boolean | undefined => { - return linguistGeneratedState( - filePath, - compileLinguistRules(parseRules(content)), - ) -} - -describe("parseLinguistGeneratedRules", () => { - it("parses all four attribute spellings into rules", () => { - const content = [ - "a.json linguist-generated", - "b.json linguist-generated=true", - "c.json linguist-generated=false", - "d.json -linguist-generated", - ].join("\n") - - expect(parseRules(content)).toEqual([ - { pattern: "a.json", generated: true }, - { pattern: "b.json", generated: true }, - { pattern: "c.json", generated: false }, - { pattern: "d.json", generated: false }, - ]) - }) - - it("skips comments, blank lines, and lines without the attribute", () => { - const content = [ - "# generated artifacts", - "", - "*.pdf binary", - "*.snap linguist-generated=true", - ].join("\n") - - expect(parseRules(content)).toEqual([ - { pattern: "*.snap", generated: true }, - ]) - }) - - it("skips gitignore-style negation patterns, which gitattributes forbids", () => { - expect(parseRules("!*.snap linguist-generated=true")).toEqual([]) - }) - - it("drops a wildcard-cap-violating pattern with a warn and keeps the rest", () => { - const logger = createTestLogger() - const content = [ - "*a*a*a*b linguist-generated=true", - "*.snap linguist-generated=true", - ].join("\n") - - const rules = parseLinguistGeneratedRules(content, logger) - - expect(rules).toEqual([{ pattern: "*.snap", generated: true }]) - expect(logger.messages).toEqual([ - { - level: "warn", - message: - "gitattributes pattern exceeds the wildcard cap — rule ignored", - data: { pattern: "*a*a*a*b" }, - }, - ]) - }) - - it("keeps a backslash-escaped space inside the pattern token", () => { - expect(parseRules("a\\ b.json linguist-generated=true")).toEqual([ - { pattern: "a\\ b.json", generated: true }, - ]) - }) -}) - -describe("linguistGeneratedState", () => { - it("returns undefined when no rule matches", () => { - expect(stateFor("src/app.ts", "*.snap linguist-generated=true")).toBe( - undefined, - ) - }) - - it("matches a slash-less pattern against basenames at any depth", () => { - expect( - stateFor("deep/nested/x.snap", "*.snap linguist-generated=true"), - ).toBe(true) - }) - - it("applies the last matching rule when rules overlap", () => { - const content = [ - "snapshots/*.json linguist-generated=true", - "snapshots/keep.json -linguist-generated", - ].join("\n") - - expect(stateFor("snapshots/keep.json", content)).toBe(false) - expect(stateFor("snapshots/other.json", content)).toBe(true) - }) - - it("matches directory-style patterns against contained files", () => { - // Deliberate over-approximation: gitattributes itself would not apply a - // "dir/" pattern to contained paths, but excluding more than GitHub - // collapses is visible in the review output and off-switchable - expect( - stateFor( - "__snapshots__/x.json", - "__snapshots__/ linguist-generated=true", - ), - ).toBe(true) - expect( - stateFor("__snapshots__/x.json", "__snapshots__ linguist-generated=true"), - ).toBe(true) - }) -}) diff --git a/src/diff/__tests__/pattern-safety.test.ts b/src/diff/__tests__/pattern-safety.test.ts deleted file mode 100644 index d5c8a67..0000000 --- a/src/diff/__tests__/pattern-safety.test.ts +++ /dev/null @@ -1,29 +0,0 @@ -import { describe, expect, it } from "vitest" -import { DEFAULT_DIFF_EXCLUDE_PATTERNS } from "../../config.js" -import { hasExcessiveWildcards } from "../pattern-safety.js" - -describe("hasExcessiveWildcards", () => { - it("accepts every shipped default pattern", () => { - // Production-consistency check across two constants, not a drift test: - // a default the cap itself would reject could never match anything - expect(DEFAULT_DIFF_EXCLUDE_PATTERNS.filter(hasExcessiveWildcards)).toEqual( - [], - ) - }) - - it("accepts globstar segments regardless of how many appear", () => { - expect(hasExcessiveWildcards("**/__snapshots__/**")).toBe(false) - }) - - it("accepts up to two stars in one segment", () => { - expect(hasExcessiveWildcards("*.min.*")).toBe(false) - }) - - it("flags a segment with more than two stars", () => { - expect(hasExcessiveWildcards("*a*a*b")).toBe(true) - }) - - it("flags a multi-star segment at any depth", () => { - expect(hasExcessiveWildcards("src/**/*a*a*a.json")).toBe(true) - }) -}) diff --git a/src/diff/exclude-diff-files.ts b/src/diff/exclude-diff-files.ts deleted file mode 100644 index 0e9f4fe..0000000 --- a/src/diff/exclude-diff-files.ts +++ /dev/null @@ -1,181 +0,0 @@ -import { posix } from "node:path" -import type { File } from "parse-diff" -import { newFilePath } from "./commentable-lines.js" -import { - compileLinguistRules, - linguistGeneratedState, - type CompiledLinguistRule, - type LinguistRule, -} from "./gitattributes.js" - -export type DiffExclusionSource = - "default_pattern" | "operator_pattern" | "linguist_generated" - -export type ExcludedDiffFile = { - path: string - additions: number - deletions: number - source: DiffExclusionSource -} - -export type PartitionedDiffFiles = { - kept: File[] - excluded: ExcludedDiffFile[] -} - -/** Operator-facing label for each exclusion source, shown in the excluded- - * files trailer and the status comment's context notes. */ -export const describeExclusionSource = ( - source: DiffExclusionSource, -): string => { - // One convention across labels: a lowercase noun phrase naming the layer, - // with the external identifier (input name, gitattributes attribute) verbatim - if (source === "default_pattern") return "built-in default list" - if (source === "operator_pattern") return "diff_exclude_paths input" - return "linguist-generated attribute" -} - -/** A pattern hits as a root-anchored folder prefix (the exclude_paths rule) - * or as a glob — the union keeps both operator mental models valid. */ -const matchesExcludePattern = (filePath: string, pattern: string): boolean => { - return ( - filePath === pattern || - filePath.startsWith(pattern + "/") || - posix.matchesGlob(filePath, pattern) - ) -} - -const matchesAnyPattern = (filePath: string, patterns: string[]): boolean => { - return patterns.some((pattern) => matchesExcludePattern(filePath, pattern)) -} - -/** The path a file is judged by: the new path, or the old path for - * deletions — a rename out of an excluded folder into reviewable source is - * reviewed, while a rename into one is excluded. */ -const exclusionPath = (file: File): string | null => { - const filePath = newFilePath(file) ?? file.from - if (!filePath || filePath === "/dev/null") return null - // Leading slashes are stripped because ignore().ignores() throws on - // absolute paths, and diff paths are PR-author-influenced - return posix.normalize(filePath).replace(/^\/+/, "") -} - -const resolveExclusionSource = ({ - filePath, - defaultPatterns, - operatorPatterns, - compiledRules, -}: { - filePath: string - defaultPatterns: string[] - operatorPatterns: string[] - compiledRules: CompiledLinguistRule[] -}): DiffExclusionSource | null => { - // Precedence: operator patterns are the most intentional layer and beat a - // repo's negated gitattributes entry; a negated entry in turn exempts the - // file from the built-in default list. - if (matchesAnyPattern(filePath, operatorPatterns)) return "operator_pattern" - - const generatedState = linguistGeneratedState(filePath, compiledRules) - if (generatedState === false) return null - if (generatedState === true) return "linguist_generated" - - if (matchesAnyPattern(filePath, defaultPatterns)) return "default_pattern" - return null -} - -/** - * Splits parsed diff files into the review subject and the excluded rest. - * Runs before diff annotation and the token budget check so excluded files - * consume no budget, no changed-file reads, and no commentable lines. - */ -export const partitionExcludedFiles = ({ - files, - defaultPatterns, - operatorPatterns, - linguistRules, -}: { - files: File[] - defaultPatterns: string[] - operatorPatterns: string[] - linguistRules: LinguistRule[] -}): PartitionedDiffFiles => { - const compiledRules = compileLinguistRules(linguistRules) - const kept: File[] = [] - const excluded: ExcludedDiffFile[] = [] - - for (const file of files) { - const filePath = exclusionPath(file) - if (!filePath) { - kept.push(file) - continue - } - - const source = resolveExclusionSource({ - filePath, - defaultPatterns, - operatorPatterns, - compiledRules, - }) - if (!source) { - kept.push(file) - continue - } - - excluded.push({ - path: filePath, - additions: file.additions, - deletions: file.deletions, - source, - }) - } - - return { kept, excluded } -} - -/** One line per excluded file with change counts and the source that - * excluded it — shared by the prompt trailer and the all-excluded skip - * review so both surfaces name the same facts identically. */ -export const renderExcludedFileLines = ( - excluded: ExcludedDiffFile[], -): string[] => { - return excluded.map((file) => { - const changeCounts = `+${file.additions}/-${file.deletions}` - return `- ${file.path} (${changeCounts}, ${describeExclusionSource(file.source)})` - }) -} - -const SOURCE_SUMMARY_ORDER: DiffExclusionSource[] = [ - "operator_pattern", - "linguist_generated", - "default_pattern", -] - -/** Per-source counts for one-line surfaces (check-run title, skip reason) — - * attributes the exclusion to the layer that actually caused it instead of - * naming an input the operator may never have set. */ -export const summarizeExclusionSources = ( - excluded: ExcludedDiffFile[], -): string => { - return SOURCE_SUMMARY_ORDER.flatMap((source) => { - const count = excluded.filter((file) => file.source === source).length - return count > 0 ? [`${count} by ${describeExclusionSource(source)}`] : [] - }).join(", ") -} - -/** - * The changed-but-not-reviewed trailer appended after the annotated diff, so - * the model knows these files changed without seeing their content. Lines - * deliberately do not resemble the "=== path ===" file headers — the - * anchoring contract only lets the model cite real headers and file blocks. - */ -export const renderExcludedFilesNote = ( - excluded: ExcludedDiffFile[], -): string => { - if (excluded.length === 0) return "" - - return [ - `${excluded.length} changed file(s) excluded from review (content not shown):`, - ...renderExcludedFileLines(excluded), - ].join("\n") -} diff --git a/src/diff/exclusion.ts b/src/diff/exclusion.ts new file mode 100644 index 0000000..efc58da --- /dev/null +++ b/src/diff/exclusion.ts @@ -0,0 +1,284 @@ +import { posix } from "node:path" +import ignoreModule from "ignore" +import type { File } from "parse-diff" +import type { Logger } from "../logger.js" +import { newFilePath } from "./commentable-lines.js" + +/** ignore ships CommonJS with an ESM-style "export default" declaration, so + * under NodeNext the callable factory sits behind .default in both the type + * and the runtime interop (the package sets module.exports.default itself). */ +const createIgnoreMatcher = ignoreModule.default + +export type DiffExclusionSource = + "default_pattern" | "operator_pattern" | "linguist_generated" + +export type ExcludedDiffFile = { + path: string + additions: number + deletions: number + source: DiffExclusionSource +} + +export type PartitionedDiffFiles = { + kept: File[] + excluded: ExcludedDiffFile[] +} + +export type ExclusionMatcher = { + classify: (filePath: string) => DiffExclusionSource | null +} + +/** + * Matching engines (path.matchesGlob, the ignore package) backtrack + * exponentially when one segment interleaves several "*" wildcards with + * literals — a crafted 40+-char filename hangs a single synchronous, + * unabortable match call for minutes. Both pattern channels (operator + * input and repo .gitattributes) are bounded by this cap; "**" globstar + * segments are exempt because globstar traversal does not backtrack. + */ +export const hasExcessiveWildcards = (pattern: string): boolean => { + return pattern.split("/").some((segment) => { + if (segment === "**") return false + const starCount = (segment.match(/\*/g) ?? []).length + return starCount > 2 + }) +} + +type LinguistRule = { + pattern: string + generated: boolean +} + +/** + * Only these spellings carry a linguist-generated signal. Git's + * "!linguist-generated" means "unspecified" and other string values have no + * defined truthiness here, so both produce no rule. + */ +const GENERATED_ATTRIBUTE_STATES = new Map([ + ["linguist-generated", true], + ["linguist-generated=true", true], + ["linguist-generated=false", false], + ["-linguist-generated", false], +]) + +/** Whitespace not preceded by a backslash — a backslash-escaped space is + * git's escaping for paths with spaces and stays inside the pattern token. */ +const UNESCAPED_WHITESPACE = /(? { + const rules: LinguistRule[] = [] + + for (const rawLine of content.split("\n")) { + const line = rawLine.trim() + if (!line || line.startsWith("#")) continue + + const [pattern, ...attributes] = line + .split(UNESCAPED_WHITESPACE) + .filter(Boolean) + if (!pattern) continue + // gitattributes forbids gitignore-style "!" negation patterns — git + // ignores such lines, and so does this parser + if (pattern.startsWith("!")) continue + + const generatedState = attributes + .map((attribute) => GENERATED_ATTRIBUTE_STATES.get(attribute)) + .findLast((state) => state !== undefined) + if (generatedState === undefined) continue + + if (hasExcessiveWildcards(pattern)) { + logger.warn( + "gitattributes pattern exceeds the wildcard cap — rule ignored", + { pattern }, + ) + continue + } + + rules.push({ pattern, generated: generatedState }) + } + + return rules +} + +/** A pattern hits as a root-anchored folder prefix (the exclude_paths rule) + * or as a glob — the union keeps both operator mental models valid. */ +const matchesExcludePattern = (filePath: string, pattern: string): boolean => { + return ( + filePath === pattern || + filePath.startsWith(pattern + "/") || + posix.matchesGlob(filePath, pattern) + ) +} + +/** + * Compiles all three exclusion tiers into one classifier so per-file + * evaluation carries no configuration. Gitattributes patterns get one + * ignore() instance each — load-bearing: a shared instance would apply + * gitignore "!" negation semantics across rules, which the gitattributes + * format forbids; per-pattern instances keep last-match-wins a plain fold. + */ +export const createExclusionMatcher = ( + { + defaultPatterns, + operatorPatterns, + gitAttributesContent, + }: { + defaultPatterns: string[] + operatorPatterns: string[] + gitAttributesContent: string | null + }, + logger: Logger, +): ExclusionMatcher => { + const linguistRules = gitAttributesContent + ? parseLinguistGeneratedRules(gitAttributesContent, logger) + : [] + const compiledLinguistRules = linguistRules.map((rule) => { + const patternMatcher = createIgnoreMatcher().add(rule.pattern) + return { + matchesPath: (filePath: string) => patternMatcher.ignores(filePath), + generated: rule.generated, + } + }) + + const classify = (filePath: string): DiffExclusionSource | null => { + // Precedence: operator patterns are the most intentional layer and beat + // a repo's negated gitattributes entry; a negated entry in turn exempts + // the file from the built-in default list. + if ( + operatorPatterns.some((pattern) => + matchesExcludePattern(filePath, pattern), + ) + ) { + return "operator_pattern" + } + + // Last matching rule wins, per gitattributes semantics + const generated = compiledLinguistRules + .filter((rule) => rule.matchesPath(filePath)) + .at(-1)?.generated + if (generated === false) return null + if (generated === true) return "linguist_generated" + + if ( + defaultPatterns.some((pattern) => + matchesExcludePattern(filePath, pattern), + ) + ) { + return "default_pattern" + } + return null + } + + return { classify } +} + +/** The path a file is judged by: the new path, or the old path for + * deletions — a rename out of an excluded folder into reviewable source is + * reviewed, while a rename into one is excluded. */ +const exclusionPath = (file: File): string | null => { + const filePath = newFilePath(file) ?? file.from + if (!filePath || filePath === "/dev/null") return null + // Leading slashes are stripped because ignore().ignores() throws on + // absolute paths, and diff paths are PR-author-influenced + return posix.normalize(filePath).replace(/^\/+/, "") +} + +/** + * Splits parsed diff files into the review subject and the excluded rest. + * Runs before diff annotation and the token budget check so excluded files + * consume no budget, no changed-file reads, and no commentable lines. + */ +export const partitionExcludedFiles = ({ + files, + matcher, +}: { + files: File[] + matcher: ExclusionMatcher +}): PartitionedDiffFiles => { + const kept: File[] = [] + const excluded: ExcludedDiffFile[] = [] + + for (const file of files) { + const filePath = exclusionPath(file) + const source = filePath ? matcher.classify(filePath) : null + if (filePath && source) { + excluded.push({ + path: filePath, + additions: file.additions, + deletions: file.deletions, + source, + }) + } else { + kept.push(file) + } + } + + return { kept, excluded } +} + +/** Operator-facing label for each exclusion source, shown in the excluded- + * files trailer and the status comment's context notes. One convention + * across labels: a lowercase noun phrase naming the layer, with the + * external identifier (input name, gitattributes attribute) verbatim. */ +export const describeExclusionSource = ( + source: DiffExclusionSource, +): string => { + if (source === "default_pattern") return "built-in default list" + if (source === "operator_pattern") return "diff_exclude_paths input" + return "linguist-generated attribute" +} + +/** One line per excluded file with change counts and the source that + * excluded it — shared by the prompt trailer and the all-excluded skip + * review so both surfaces name the same facts identically. */ +export const renderExcludedFileLines = ( + excluded: ExcludedDiffFile[], +): string[] => { + return excluded.map((file) => { + const changeCounts = `+${file.additions}/-${file.deletions}` + return `- ${file.path} (${changeCounts}, ${describeExclusionSource(file.source)})` + }) +} + +const SOURCE_SUMMARY_ORDER: DiffExclusionSource[] = [ + "operator_pattern", + "linguist_generated", + "default_pattern", +] + +/** Per-source counts for one-line surfaces (check-run title, skip reason) — + * attributes the exclusion to the layer that actually caused it instead of + * naming an input the operator may never have set. */ +export const summarizeExclusionSources = ( + excluded: ExcludedDiffFile[], +): string => { + return SOURCE_SUMMARY_ORDER.flatMap((source) => { + const count = excluded.filter((file) => file.source === source).length + return count > 0 ? [`${count} by ${describeExclusionSource(source)}`] : [] + }).join(", ") +} + +/** + * The changed-but-not-reviewed trailer appended after the annotated diff, so + * the model knows these files changed without seeing their content. Lines + * deliberately do not resemble the "=== path ===" file headers — the + * anchoring contract only lets the model cite real headers and file blocks. + */ +export const renderExcludedFilesNote = ( + excluded: ExcludedDiffFile[], +): string => { + if (excluded.length === 0) return "" + + return [ + `${excluded.length} changed file(s) excluded from review (content not shown):`, + ...renderExcludedFileLines(excluded), + ].join("\n") +} diff --git a/src/diff/gitattributes.ts b/src/diff/gitattributes.ts deleted file mode 100644 index e0f7a0f..0000000 --- a/src/diff/gitattributes.ts +++ /dev/null @@ -1,107 +0,0 @@ -import ignoreModule from "ignore" -import type { Logger } from "../logger.js" -import { hasExcessiveWildcards } from "./pattern-safety.js" - -/** ignore ships CommonJS with an ESM-style "export default" declaration, so - * under NodeNext the callable factory sits behind .default in both the type - * and the runtime interop (the package sets module.exports.default itself). */ -const createIgnoreMatcher = ignoreModule.default - -export type LinguistRule = { - pattern: string - generated: boolean -} - -export type CompiledLinguistRule = { - matchesPath: (filePath: string) => boolean - generated: boolean -} - -/** - * Only these spellings carry a linguist-generated signal. Git's - * "!linguist-generated" means "unspecified" and other string values have no - * defined truthiness here, so both produce no rule. - */ -const GENERATED_ATTRIBUTE_STATES = new Map([ - ["linguist-generated", true], - ["linguist-generated=true", true], - ["linguist-generated=false", false], - ["-linguist-generated", false], -]) - -/** Whitespace not preceded by a backslash — a backslash-escaped space is - * git's escaping for paths with spaces and stays inside the pattern token. */ -const UNESCAPED_WHITESPACE = /(? { - return line.split(UNESCAPED_WHITESPACE).filter(Boolean) -} - -/** - * Extracts the linguist-generated rules from .gitattributes content. The - * file arrives from the PR head checkout, so it is untrusted input: a - * malformed or wildcard-cap-violating line drops that rule with a warn and - * never fails the run. - */ -export const parseLinguistGeneratedRules = ( - content: string, - logger: Logger, -): LinguistRule[] => { - const rules: LinguistRule[] = [] - - for (const rawLine of content.split("\n")) { - const line = rawLine.trim() - if (!line || line.startsWith("#")) continue - - const [pattern, ...attributes] = splitAttributeLine(line) - if (!pattern) continue - // gitattributes forbids gitignore-style "!" negation patterns — git - // ignores such lines, and so does this parser - if (pattern.startsWith("!")) continue - - const generatedState = attributes - .map((attribute) => GENERATED_ATTRIBUTE_STATES.get(attribute)) - .findLast((state) => state !== undefined) - if (generatedState === undefined) continue - - if (hasExcessiveWildcards(pattern)) { - logger.warn( - "gitattributes pattern exceeds the wildcard cap — rule ignored", - { pattern }, - ) - continue - } - - rules.push({ pattern, generated: generatedState }) - } - - return rules -} - -/** - * One ignore() instance per pattern — load-bearing: a shared instance would - * apply gitignore "!" negation semantics across rules, which the - * gitattributes format forbids. Per-pattern instances keep each rule an - * independent match so last-match-wins stays a plain fold over the rules. - */ -export const compileLinguistRules = ( - rules: LinguistRule[], -): CompiledLinguistRule[] => { - return rules.map((rule) => { - const matcher = createIgnoreMatcher().add(rule.pattern) - return { - matchesPath: (filePath: string) => matcher.ignores(filePath), - generated: rule.generated, - } - }) -} - -/** Last matching rule wins, per gitattributes semantics; undefined means no - * rule matched, so the caller falls through to the default pattern tier. */ -export const linguistGeneratedState = ( - filePath: string, - compiledRules: CompiledLinguistRule[], -): boolean | undefined => { - return compiledRules.filter((rule) => rule.matchesPath(filePath)).at(-1) - ?.generated -} diff --git a/src/diff/pattern-safety.ts b/src/diff/pattern-safety.ts deleted file mode 100644 index 06a55e1..0000000 --- a/src/diff/pattern-safety.ts +++ /dev/null @@ -1,15 +0,0 @@ -/** - * Matching engines (path.matchesGlob, the ignore package) backtrack - * exponentially when one segment interleaves several "*" wildcards with - * literals — a crafted 40+-char filename hangs a single synchronous, - * unabortable match call for minutes. Both pattern channels (operator - * input and repo .gitattributes) are bounded by this cap; "**" globstar - * segments are exempt because globstar traversal does not backtrack. - */ -export const hasExcessiveWildcards = (pattern: string): boolean => { - return pattern.split("/").some((segment) => { - if (segment === "**") return false - const starCount = (segment.match(/\*/g) ?? []).length - return starCount > 2 - }) -} diff --git a/src/orchestrate.ts b/src/orchestrate.ts index 44cc962..61a758e 100644 --- a/src/orchestrate.ts +++ b/src/orchestrate.ts @@ -7,12 +7,12 @@ import { } from "./diff/commentable-lines.js" import { annotateDiff } from "./diff/annotate-diff.js" import { + createExclusionMatcher, partitionExcludedFiles, renderExcludedFileLines, renderExcludedFilesNote, summarizeExclusionSources, -} from "./diff/exclude-diff-files.js" -import { parseLinguistGeneratedRules } from "./diff/gitattributes.js" +} from "./diff/exclusion.js" import { describeError, type Logger } from "./logger.js" import type { CheckRunConclusion, @@ -548,16 +548,12 @@ const runReviewPipeline = async ( const gitAttributesContent = config.respectLinguistGenerated ? await contextReader.readGitAttributes() : null - const linguistRules = gitAttributesContent - ? parseLinguistGeneratedRules(gitAttributesContent, logger) - : [] + const exclusionMatcher = createExclusionMatcher( + { ...config.diffExcludePaths, gitAttributesContent }, + logger, + ) const { kept: reviewableFiles, excluded: excludedDiffFiles } = - partitionExcludedFiles({ - files, - defaultPatterns: config.diffExcludePaths.defaultPatterns, - operatorPatterns: config.diffExcludePaths.operatorPatterns, - linguistRules, - }) + partitionExcludedFiles({ files, matcher: exclusionMatcher }) if (excludedDiffFiles.length > 0) { logger.info("changed files excluded from the review diff", { excludedCount: excludedDiffFiles.length, diff --git a/src/review/context-notes.ts b/src/review/context-notes.ts index 5dfc252..9eef60d 100644 --- a/src/review/context-notes.ts +++ b/src/review/context-notes.ts @@ -2,7 +2,7 @@ import { posix } from "node:path" import { describeExclusionSource, type ExcludedDiffFile, -} from "../diff/exclude-diff-files.js" +} from "../diff/exclusion.js" import type { PromptFile } from "./prompt.js" export type ContextNotesInput = { From 7b09484a80ba0bf85e9c84d1ad6480675486955f Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 7 Sep 2026 03:24:34 +0000 Subject: [PATCH 11/13] docs: decomposition must earn its seams MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Wrapper-only functions, config threaded through layers that don't read it, and modules that only import each other add misdirection without benefit — codify the boundary: extract for a second call site or a decision worth naming, compile config into a factory/closure, and keep one concern in one module. Co-Authored-By: Claude Fable 5 --- AGENTS.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/AGENTS.md b/AGENTS.md index e4c02ca..f414fc1 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -100,6 +100,14 @@ files. Prefer SDK-provided types over redefining shapes. visibility when only one function needs the value. - A boolean mode param means the function does two things — split into two single-responsibility functions; the caller owns the gating. +- Decomposition must earn its seams. A function whose body is one + expression with one call site is misdirection — inline it; extract only + for a second call site or a decision worth naming. A parameter a + function only forwards means the seam is wrong — compile configuration + once into a factory/closure and pass the resulting collaborator, never + thread config through layers that don't read it. One concern stays in + one module: files that only ever import each other are fragmentation, + not separation — a module boundary needs an independent consumer. - Type-only imports over structural duplication — don't clone interfaces for "module purity"; type imports are erased at compile time. - Extract multi-step `.map()`/`.reduce()` callbacks into named functions From 7b07be39ff4afb561f36f53bbc9b86169beb914b Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 7 Sep 2026 03:28:41 +0000 Subject: [PATCH 12/13] fix: escaped characters do not count toward the wildcard cap An escaped star is a literal to every matcher and cannot backtrack, so a gitattributes pattern whose stars are escaped was wrongly dropped by the cap; unescaped stars alongside escaped ones still count. Co-Authored-By: Claude Fable 5 --- src/diff/__tests__/exclusion.test.ts | 8 ++++++++ src/diff/exclusion.ts | 5 ++++- 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/src/diff/__tests__/exclusion.test.ts b/src/diff/__tests__/exclusion.test.ts index 1e5c9a4..cfabbdb 100644 --- a/src/diff/__tests__/exclusion.test.ts +++ b/src/diff/__tests__/exclusion.test.ts @@ -69,6 +69,14 @@ describe("hasExcessiveWildcards", () => { it("flags a multi-star segment at any depth", () => { expect(hasExcessiveWildcards("src/**/*a*a*a.json")).toBe(true) }) + + it("does not count escaped stars toward the cap", () => { + expect(hasExcessiveWildcards("a\\*b\\*c\\*d.json")).toBe(false) + }) + + it("still flags unescaped stars alongside escaped ones", () => { + expect(hasExcessiveWildcards("\\*a*a*a*b")).toBe(true) + }) }) describe("createExclusionMatcher — gitattributes rules", () => { diff --git a/src/diff/exclusion.ts b/src/diff/exclusion.ts index efc58da..6b66e84 100644 --- a/src/diff/exclusion.ts +++ b/src/diff/exclusion.ts @@ -39,7 +39,10 @@ export type ExclusionMatcher = { export const hasExcessiveWildcards = (pattern: string): boolean => { return pattern.split("/").some((segment) => { if (segment === "**") return false - const starCount = (segment.match(/\*/g) ?? []).length + // An escaped character is a literal to every matcher — an escaped star + // cannot backtrack, so it must not count toward the cap + const unescapedSegment = segment.replace(/\\./g, "") + const starCount = (unescapedSegment.match(/\*/g) ?? []).length return starCount > 2 }) } From 23868bb95ef0f3282555b7caa40548c357a28de6 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 7 Sep 2026 03:46:29 +0000 Subject: [PATCH 13/13] fix: priority_docs cannot re-include a diff-excluded file The related-file and doc scans already skip diff-excluded files; the priority-doc read was the remaining channel that could pull excluded content back into the prompt, making the trailer's 'content not shown' line false for that file. Diff exclusion now wins over the priority list, documented in README and action.yml. Also covers the unreadable- .gitattributes warn branch with a root-proof test (a directory named .gitattributes fails the read deterministically; permission bits are ignored under root). Co-Authored-By: Claude Fable 5 --- README.md | 2 +- action.yml | 6 +++++- src/__tests__/orchestrate.test.ts | 19 +++++++++++++++++++ src/context/__tests__/workspace.test.ts | 24 ++++++++++++++++++++++++ src/orchestrate.ts | 16 +++++++++++----- 5 files changed, 60 insertions(+), 7 deletions(-) diff --git a/README.md b/README.md index 1fc25d6..0c2ddcb 100644 --- a/README.md +++ b/README.md @@ -88,7 +88,7 @@ The `@umm review` comment trigger lets you re-request a review on any PR by comm | `phases` | `combined` | How the review dimensions are dispatched: `combined` (one model call carrying every dimension), `parallel` (three focused calls at once — each reads deeper than the single call; wall clock is the slowest of the three and prompt tokens roughly triple), or `sequential` (the same three calls in order, each seeing the earlier findings). Findings two phases report on the same lines collapse to one, keeping the higher severity. Empty = `combined`, so workflows can wire an unset repo variable directly | | `context_budget_tokens` | `300000` | Approximate token budget for prompt context (file contents + diff — conventions have a separate cap) | | `trace_related_files` | `true` | Enable heuristic context scanning — import-tracing for caller regressions and mention-matching for doc staleness detection. Does not affect `priority_docs` | -| `priority_docs` | `README.md` | Comma-separated repo-relative paths included in review context with a reserved 10% budget floor, independent of `trace_related_files`. Docs already present in full from other context channels are not re-read; a diff-only changed file is still read here so its full text reaches the model. Subject to the shared doc token budget (the floor prevents starvation by related files but does not guarantee inclusion when the budget is exhausted by the diff and changed files themselves). Empty = disabled | +| `priority_docs` | `README.md` | Comma-separated repo-relative paths included in review context with a reserved 10% budget floor, independent of `trace_related_files`. Docs already present in full from other context channels are not re-read; a diff-only changed file is still read here so its full text reaches the model. A changed file excluded from the review diff stays excluded — `diff_exclude_paths` and linguist rules win over this list. Subject to the shared doc token budget (the floor prevents starvation by related files but does not guarantee inclusion when the budget is exhausted by the diff and changed files themselves). Empty = disabled | | `max_scan_files` | `5000` | Maximum files to walk during workspace scan for related file and doc detection | | `max_scan_bytes` | `262144` | Maximum byte size of a single file to include in the workspace scan | | `max_related_files` | `8` | Maximum import-traced related files to include in review context | diff --git a/action.yml b/action.yml index 01461fe..4501f42 100644 --- a/action.yml +++ b/action.yml @@ -49,7 +49,11 @@ inputs: required: false default: "true" priority_docs: - description: Comma-separated repo-relative paths always included in review context. Docs already present in full from other context channels are not re-read (empty = disabled) + description: >- + Comma-separated repo-relative paths always included in review context. + Docs already present in full from other context channels are not + re-read; a changed file excluded from the review diff stays excluded + (empty = disabled) required: false default: "README.md" max_scan_files: diff --git a/src/__tests__/orchestrate.test.ts b/src/__tests__/orchestrate.test.ts index 8e862b0..a18c1d4 100644 --- a/src/__tests__/orchestrate.test.ts +++ b/src/__tests__/orchestrate.test.ts @@ -744,6 +744,25 @@ describe("orchestrate", () => { ]) }) + it("drops a diff-excluded priority doc from the priority-doc read", async () => { + const stubs = makeOrchestrateDeps({ + config: { + priorityDocs: ["assets/logo.png", "docs/guide.md"], + diffExcludePaths: { + defaultPatterns: [], + operatorPatterns: ["assets/**"], + }, + }, + }) + const logger = createTestLogger() + + await orchestrate(stubs.deps, logger) + + expect(first(stubs.readPriorityDocsCalls).priorityDocs).toEqual([ + "docs/guide.md", + ]) + }) + it("passes diff-excluded paths to the related-file and doc scans as exclusions", async () => { const stubs = makeOrchestrateDeps({ config: { diff --git a/src/context/__tests__/workspace.test.ts b/src/context/__tests__/workspace.test.ts index 0a3fec6..3ffeeb6 100644 --- a/src/context/__tests__/workspace.test.ts +++ b/src/context/__tests__/workspace.test.ts @@ -176,6 +176,30 @@ describe("readGitAttributes", () => { expect(logger.messages).toEqual([]) }) + it("warns and returns null when .gitattributes exists but cannot be read", async () => { + // A directory named .gitattributes makes readFile fail deterministically + // (EISDIR) in every environment — permission bits are ignored under root + const { root, cleanup } = await makeTempWorkspace({ + ".gitattributes/placeholder": "", + }) + const logger = createTestLogger() + const contextReader = createContextReader(defaultConfig(root), logger) + + try { + const content = await contextReader.readGitAttributes() + + expect(content).toBeNull() + expect(logger.messages).toContainEqual({ + level: "warn", + message: + "failed reading .gitattributes — linguist-generated rules unavailable", + data: { error: expect.stringContaining("EISDIR") }, + }) + } finally { + await cleanup() + } + }) + it("warns and returns null when .gitattributes exceeds the scan size cap", async () => { const oversizedContent = "*.snap linguist-generated=true\n".repeat(4) const { root, cleanup } = await makeTempWorkspace({ diff --git a/src/orchestrate.ts b/src/orchestrate.ts index 61a758e..11418c2 100644 --- a/src/orchestrate.ts +++ b/src/orchestrate.ts @@ -589,9 +589,15 @@ const runReviewPipeline = async ( const commentableByPath = computeCommentableLines(reviewableFiles) // Diff-excluded files must stay out of every context channel: the trailer - // told the model their content is not shown, so the related-file and doc - // scans may not pull that content back into the prompt. + // told the model their content is not shown, so neither the related-file + // and doc scans nor the priority-doc read may pull that content back in. const diffExcludedPaths = excludedDiffFiles.map((file) => file.path) + const diffExcludedPathSet = new Set( + diffExcludedPaths.map((excludedPath) => posix.normalize(excludedPath)), + ) + const reviewablePriorityDocs = config.priorityDocs.filter( + (docPath) => !diffExcludedPathSet.has(posix.normalize(docPath)), + ) // Step 8: extract changed paths (includes old path for renames so the // import scanner finds callers that still reference the pre-rename path) @@ -643,8 +649,8 @@ const runReviewPipeline = async ( : []), ]) const needsPriorityDocFloor = - config.priorityDocs.length > 0 && - config.priorityDocs.some( + reviewablePriorityDocs.length > 0 && + reviewablePriorityDocs.some( (docPath) => !preFloorInContext.has(posix.normalize(docPath)), ) const rawFloor = Math.floor( @@ -688,7 +694,7 @@ const runReviewPipeline = async ( const { files: priorityDocFiles, remainingTokens: docRemainingTokens } = await contextReader.readPriorityDocs({ - priorityDocs: config.priorityDocs, + priorityDocs: reviewablePriorityDocs, budgetTokens: docBudgetTokens, excludePaths: priorityDocsInContext, })