diff --git a/.prettierignore b/.prettierignore new file mode 100644 index 0000000..f2937e7 --- /dev/null +++ b/.prettierignore @@ -0,0 +1,7 @@ +# 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 +# npm's output format is the source of truth; reformatting a lockfile +# invites churn against what the tool regenerates +package-lock.json diff --git a/AGENTS.md b/AGENTS.md index bf61866..f414fc1 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 ``` @@ -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 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/README.md b/README.md index da1db49..0c2ddcb 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. 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 | +| `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 2e4883e..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: @@ -78,6 +82,36 @@ 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 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 + 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..fb69304 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..6bf46f3 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..a18c1d4 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", @@ -230,6 +238,8 @@ const baseConfig: ActionConfig = { maxRelatedDocs: 4, priorityDocs: [], excludePaths: [], + diffExcludePaths: { defaultPatterns: [], operatorPatterns: [] }, + respectLinguistGenerated: true, costSummary: true, prNumberOverride: undefined, } @@ -261,6 +271,7 @@ type ReadChangedFilesParams = { type FindRelatedFilesParams = { changedPaths: string[] budgetTokens: number + excludePaths: string[] } type RequestReviewParams = { @@ -409,6 +420,7 @@ const makeOrchestrateDeps = ( readConventionsCalls.push(params) return "# Test conventions" }, + readGitAttributes: async () => null, readChangedFiles: async (params) => { readChangedFilesCalls.push(params) return { files: [fixtureChangedFile], remainingTokens: 40_000 } @@ -551,7 +563,7 @@ describe("orchestrate", () => { expect(first(stubs.submitReviewCalls)).toEqual({ prNumber: fixturePrContext.prNumber, commitId: fixturePrContext.headSha, - body: buildSkipBody(skipReason), + body: buildSkipBody({ reason: skipReason }), }) }) @@ -580,7 +592,7 @@ describe("orchestrate", () => { expect(first(stubs.submitReviewCalls)).toEqual({ prNumber: fixturePrContext.prNumber, commitId: fixturePrContext.headSha, - body: buildSkipBody(skipReason), + body: buildSkipBody({ reason: skipReason }), }) }) @@ -608,8 +620,269 @@ describe("orchestrate", () => { expect(first(stubs.submitReviewCalls)).toEqual({ prNumber: fixturePrContext.prNumber, commitId: fixturePrContext.headSha, - body: buildSkipBody(skipReason), + body: buildSkipBody({ reason: skipReason }), + }) + }) + + 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 file(s) excluded from review (6 by diff_exclude_paths input)" + 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({ + reason: skipReason, + detail: [ + "- 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"), + }), + }) + }) + + 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 built-in default list)", + ) + }) + + 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 input)", + ].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 input)", + ], + }), + ]) + }) + + 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: { + 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 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 (101 tokens), which fit + const stubs = makeOrchestrateDeps({ + config: { + contextBudgetTokens: 220, + 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(110) + 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 attribute)", + ].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 () => { diff --git a/src/config.ts b/src/config.ts index 2bd90b0..332633c 100644 --- a/src/config.ts +++ b/src/config.ts @@ -1,3 +1,4 @@ +import { hasExcessiveWildcards } from "./diff/exclusion.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(Boolean) + + 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..3ffeeb6 100644 --- a/src/context/__tests__/workspace.test.ts +++ b/src/context/__tests__/workspace.test.ts @@ -148,6 +148,122 @@ 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 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({ + ".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 + 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() @@ -460,6 +576,7 @@ describe("findRelatedFiles", () => { const relatedFiles = await contextReader.findRelatedFiles({ changedPaths: ["src/greeter.ts", "src/registry.ts"], budgetTokens: 100_000, + excludePaths: [], }) expect(relatedFiles.files).toEqual([ @@ -490,6 +607,7 @@ describe("findRelatedFiles", () => { const relatedFiles = await contextReader.findRelatedFiles({ changedPaths: ["src/lib/index.ts"], budgetTokens: 100_000, + excludePaths: [], }) expect(relatedFiles.files).toEqual([ @@ -508,6 +626,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([ @@ -522,6 +641,7 @@ describe("findRelatedFiles", () => { const relatedFiles = await contextReader.findRelatedFiles({ changedPaths: ["src/hub.ts"], budgetTokens: 100_000, + excludePaths: [], }) expect(relatedFiles.files.map((relatedFile) => relatedFile.path)).toEqual([ @@ -542,6 +662,7 @@ describe("findRelatedFiles", () => { const relatedFiles = await contextReader.findRelatedFiles({ changedPaths: ["src/greeter.ts", "src/registry.ts"], budgetTokens: estimateTokens(consumerContent), + excludePaths: [], }) expect(relatedFiles.files).toEqual([ @@ -568,6 +689,7 @@ describe("findRelatedFiles", () => { const relatedFiles = await contextReader.findRelatedFiles({ changedPaths: ["target.ts"], budgetTokens: 1_000_000, + excludePaths: [], }) expect(relatedFiles.files.map((relatedFile) => relatedFile.path)).toEqual( @@ -596,6 +718,7 @@ describe("findRelatedFiles", () => { const relatedFiles = await contextReader.findRelatedFiles({ changedPaths: ["target.ts"], budgetTokens: 100_000, + excludePaths: [], }) expect(relatedFiles.files.map((relatedFile) => relatedFile.path)).toEqual( @@ -621,6 +744,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 @@ -656,6 +780,7 @@ describe("findRelatedFiles", () => { const relatedFiles = await contextReader.findRelatedFiles({ changedPaths: ["target.ts"], budgetTokens: 100_000, + excludePaths: [], }) expect(relatedFiles.files.map((relatedFile) => relatedFile.path)).toEqual( @@ -681,6 +806,7 @@ describe("findRelatedFiles", () => { await contextReader.findRelatedFiles({ changedPaths: ["filler-00000.ts"], budgetTokens: 100_000, + excludePaths: [], }) expect(logger.messages).toContainEqual({ @@ -711,6 +837,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([ @@ -737,6 +891,7 @@ describe("findRelatedFiles", () => { const relatedFiles = await contextReader.findRelatedFiles({ changedPaths: ["target.ts"], budgetTokens: 100_000, + excludePaths: [], }) expect(relatedFiles.files.map((file) => file.path)).toEqual([ @@ -765,6 +920,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 a147a7e..fe8d1c5 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 @@ -37,6 +40,7 @@ export type ContextReader = { findRelatedFiles: (params: { changedPaths: string[] budgetTokens: number + excludePaths: string[] }) => Promise readPriorityDocs: (params: { priorityDocs: string[] @@ -201,6 +205,42 @@ 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 + } + // 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 && 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 + 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 @@ -283,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", { @@ -348,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 } @@ -441,19 +478,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 @@ -672,6 +718,7 @@ export const createContextReader = ( return { readConventions, + readGitAttributes, readChangedFiles, findRelatedFiles, readPriorityDocs, diff --git a/src/diff/__tests__/exclusion.test.ts b/src/diff/__tests__/exclusion.test.ts new file mode 100644 index 0000000..cfabbdb --- /dev/null +++ b/src/diff/__tests__/exclusion.test.ts @@ -0,0 +1,481 @@ +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 "../exclusion.js" + +const makeFile = (overrides: Partial = {}): File => ({ + chunks: [], + additions: 3, + deletions: 1, + from: "src/app.ts", + to: "src/app.ts", + ...overrides, +}) + +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) + }) + + 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", () => { + 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" })] + + 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 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", + 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()], { + gitAttributesContent: "gen/*.json linguist-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"], + gitAttributesContent: "package-lock.json -linguist-generated", + }) + + 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"], + gitAttributesContent: "package-lock.json -linguist-generated", + }) + + 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, built-in default list)", + "- evals/run.json (+10/-0, diff_exclude_paths input)", + "- gen/x.json (+5/-5, linguist-generated attribute)", + ].join("\n"), + ) + }) +}) + +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 input, 1 by linguist-generated attribute, 2 by built-in default list", + ) + }) + + 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 built-in default list", + ) + }) +}) diff --git a/src/diff/exclusion.ts b/src/diff/exclusion.ts new file mode 100644 index 0000000..6b66e84 --- /dev/null +++ b/src/diff/exclusion.ts @@ -0,0 +1,287 @@ +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 + // 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 + }) +} + +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/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..11418c2 100644 --- a/src/orchestrate.ts +++ b/src/orchestrate.ts @@ -6,6 +6,13 @@ import { newFilePath, } from "./diff/commentable-lines.js" import { annotateDiff } from "./diff/annotate-diff.js" +import { + createExclusionMatcher, + partitionExcludedFiles, + renderExcludedFileLines, + renderExcludedFilesNote, + summarizeExclusionSources, +} from "./diff/exclusion.js" import { describeError, type Logger } from "./logger.js" import type { CheckRunConclusion, @@ -125,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[] @@ -494,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, @@ -510,41 +533,86 @@ 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 + // 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 exclusionMatcher = createExclusionMatcher( + { ...config.diffExcludePaths, gitAttributesContent }, + logger, + ) + const { kept: reviewableFiles, excluded: excludedDiffFiles } = + partitionExcludedFiles({ files, matcher: exclusionMatcher }) + 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) { + // 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 - const annotatedDiff = annotateDiff(files) + // 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) { - 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(files) + 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 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) - const changedPaths = files + 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({ @@ -581,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( @@ -600,6 +668,7 @@ const runReviewPipeline = async ( ? await contextReader.findRelatedFiles({ changedPaths, budgetTokens: relatedFilesBudgetTokens, + excludePaths: diffExcludedPaths, }) : { files: [], excludedByCapPaths: [] } @@ -625,7 +694,7 @@ const runReviewPipeline = async ( const { files: priorityDocFiles, remainingTokens: docRemainingTokens } = await contextReader.readPriorityDocs({ - priorityDocs: config.priorityDocs, + priorityDocs: reviewablePriorityDocs, budgetTokens: docBudgetTokens, excludePaths: priorityDocsInContext, }) @@ -661,7 +730,7 @@ const runReviewPipeline = async ( changedPaths, budgetTokens: docRemainingTokens, conventionsFile: config.conventionsFile, - excludePaths: config.priorityDocs, + excludePaths: [...config.priorityDocs, ...diffExcludedPaths], }) : { files: [], excludedByCapPaths: [] } @@ -670,7 +739,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 +797,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..9b4da5d 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` (built-in default list), `evals/run.json` (diff_exclude_paths input), `gen/x.json` (linguist-generated attribute)", + ]) + }) + + 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` (built-in default list)", ]) }) }) diff --git a/src/review/context-notes.ts b/src/review/context-notes.ts index 33093ef..9eef60d 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/exclusion.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) }