From e653f369abd39c026823f5d793df2630d195b2d8 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Tue, 27 Oct 2026 16:56:55 -0700 Subject: [PATCH 1/9] fix(triage-workflows): read every patch for the rules that run in code The facts step no longer stops reading patches at the judge's budget; the judge's view applies that budget and the per-file cut itself. Code-pattern rules check every file with a patch and name the files the host sent none for, instead of refusing the pull request. A Waiting on line shared by several rules is said once. --- docs/ARCHITECTURE.md | 9 +++- .../Corbits Triage User Manual.md | 2 +- docs/DECISION_MODEL.md | 10 ++--- .../triage-workflows/src/actions/facts.ts | 18 ++++---- .../triage-workflows/src/logic/checks.test.ts | 13 +++++- packages/triage-workflows/src/logic/checks.ts | 6 +-- .../triage-workflows/src/logic/facts.test.ts | 41 +++++++++++++++++- packages/triage-workflows/src/logic/facts.ts | 34 +++++---------- packages/triage-workflows/src/logic/hunks.ts | 4 +- .../triage-workflows/src/logic/patterns.ts | 42 ++++++++++++------- .../src/logic/quality.test.ts | 8 ++-- .../triage-workflows/src/logic/quality.ts | 22 +++++++--- packages/triage-workflows/src/logic/render.ts | 5 ++- 13 files changed, 140 insertions(+), 74 deletions(-) diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 5646746da..8e9ae0529 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -548,8 +548,13 @@ label off that head, so it never stands on a head not yet triaged. JavaScript file's head content its `files` and `ignores` globs admit (ast-grep's glob rules, checked when the policy is read), and keeps matches on lines the patch adds. - A file whose patch or content was not read, or a rule ast-grep refuses, - leaves the rule unconfirmed; `vendor/`, `dist/`, `build/` and + The facts step reads every changed file's patch whole, across as many + tool pages as it takes; only the judge's view of the diff is bounded + (`judgedFiles` in `logic/quality.ts`). A file the host sent no patch for + (GitHub omits it for a very large or binary file) is listed in the + evidence as not checked and the rule runs on the rest; with nothing else + to check, or a file whose content was not read, or a rule ast-grep + refuses, the rule is unconfirmed; `vendor/`, `dist/`, `build/` and `generated/` at the repository root and `node_modules/` anywhere are skipped and listed as skipped in the evidence, and a `?? 0` inside arithmetic is a counter's real default. With such a rule on, the facts diff --git a/docs/Corbits Triage User Manual/Corbits Triage User Manual.md b/docs/Corbits Triage User Manual/Corbits Triage User Manual.md index f8188ccc5..231cd64b9 100644 --- a/docs/Corbits Triage User Manual/Corbits Triage User Manual.md +++ b/docs/Corbits Triage User Manual/Corbits Triage User Manual.md @@ -258,7 +258,7 @@ A rule asks one question about a pull request and answers pass, fail or unconfir - **Flag changes to sensitive paths.** Checks changed files against the file's named groups of globs, each `high` or `medium`, written as `groups:`, such as `infra: { level: high, paths: ["infra/**", "**/*.tf"] }`. Without `groups` there are four: `schema` (`**/migrations/**`, `**/*.sql`) and `auth` (`**/auth/**`, `**/secrets/**`), which are high, and `ci` (`.github/**`, `**/Dockerfile`, `**/railway.json`, `**/railway.toml`) and `locks` (`**/bun.lock`, `**/package-lock.json`, `**/yarn.lock`, `**/pnpm-lock.yaml`), which are medium, since every dependency bump changes a lockfile. Globs read as in **Never accept changes to**. A change to a group at `failAt` or above, high unless the file says, fails the rule, which asks a maintainer unless the file gives it another `onFail`. Below that, from `labelAt`, medium unless the file says, it passes but labels the pull request `sensitive: medium` and notes it under **Notes:** (on Bitbucket Data Center, only the note). Its evidence names each group with its files, at most five, such as `schema: db/migrations/0004.sql`, then how many more. Regression risk keeps its own migration globs. - **Flag high-risk pull requests and label medium-risk ones.** Rates each pull request low, medium or high risk; see [Risk](#risk). - **Fail when …**, a text rule of your own, such as "Fail when any commit message contains "Co-Authored-By:"". It names where to look (the title, the description, any commit message, or several of them), whether the rule fails when the text **contains** or **does not contain** its phrases, and the phrases. Case does not matter. A **contains** rule on the description also looks at its hidden parts, such as HTML comments and the addresses behind links, while **does not contain** reads only what a reader sees. Only the first 100 commits are read. When a **contains** rule fails, Triage names each place and the phrases found there, asks for them to be removed, and quotes each matching line once. Text rules count toward the eight rules of your own. - - **Fail when added code matches …**, a code-pattern rule of your own, such as one that flags `?? 0`. It checks only the lines the pull request adds to TypeScript and JavaScript files (`.ts`, `.tsx`, `.mts`, `.cts`, `.js`, `.jsx`, `.mjs`, `.cjs`), so code already on the branch never fails it. On any other language it passes with **nothing checked: no TypeScript or JavaScript code added**, so don't add one to a repository written in Go, Python or similar. Turn on the shipped `convention` preset, which flags invented fallbacks (`?? 0`, `?? []`), nested ternaries, functions called where they are written, `.then`/`.catch` chains, empty `catch` blocks and lint or type suppression comments in both languages, or write your own patterns in [ast-grep](https://ast-grep.github.io/guide/rule-config.html)'s rule format. Your own pattern replaces a preset's pattern with the same `id` in both languages; to keep checking JavaScript, add a `language: javascript` pattern under the same id. A pattern takes ast-grep's own `files` and `ignores` lists to check only some paths or skip some. They follow ast-grep's glob rules: paths are from the repository root, `*` also matches across folders, and dotfiles are matched like any other file. Write each glob as a string; the `{ glob, caseInsensitive }` form is not read. Write a negated class as `[^a]`; a glob with `[!a]`, or one that isn't valid, refuses the policy. The preset's `?? 0` and `?? []` patterns ignore test files (`**/*.test.ts`, `**/*.test.tsx`, `**/*.test.js`, `**/*.test.jsx`, `**/*.test.mjs`, `**/*.test.cjs`). Each match is listed as evidence, as `path:line` and the pattern's message. Vendored and built code at the repository root (`vendor/`, `dist/`, `build/`, `generated/`) and installed code (`node_modules/` anywhere) is skipped, and each skipped file is listed in the evidence. The same folders deeper in the repository, such as `src/build/`, are checked. When a file can't be read whole or a pattern is not valid ast-grep, the rule is unconfirmed rather than passing. Triage reads at most 300 files per pull request, so on one that adds code to more, the rule is unconfirmed with a reason such as **code patterns could not be checked: 412 files with added code, more than the 300 read per pull request**. On Bitbucket Data Center, which does not give Triage each file's changed lines, it is unconfirmed whenever a TypeScript or JavaScript file of your own is added, edited, moved or copied. To give some patterns a different `onFail`, put them in a rule of their own. Code-pattern rules count toward the eight rules of your own, and the browser extension leaves them out. + - **Fail when added code matches …**, a code-pattern rule of your own, such as one that flags `?? 0`. It checks only the lines the pull request adds to TypeScript and JavaScript files (`.ts`, `.tsx`, `.mts`, `.cts`, `.js`, `.jsx`, `.mjs`, `.cjs`), so code already on the branch never fails it. On any other language it passes with **nothing checked: no TypeScript or JavaScript code added**, so don't add one to a repository written in Go, Python or similar. Turn on the shipped `convention` preset, which flags invented fallbacks (`?? 0`, `?? []`), nested ternaries, functions called where they are written, `.then`/`.catch` chains, empty `catch` blocks and lint or type suppression comments in both languages, or write your own patterns in [ast-grep](https://ast-grep.github.io/guide/rule-config.html)'s rule format. Your own pattern replaces a preset's pattern with the same `id` in both languages; to keep checking JavaScript, add a `language: javascript` pattern under the same id. A pattern takes ast-grep's own `files` and `ignores` lists to check only some paths or skip some. They follow ast-grep's glob rules: paths are from the repository root, `*` also matches across folders, and dotfiles are matched like any other file. Write each glob as a string; the `{ glob, caseInsensitive }` form is not read. Write a negated class as `[^a]`; a glob with `[!a]`, or one that isn't valid, refuses the policy. The preset's `?? 0` and `?? []` patterns ignore test files (`**/*.test.ts`, `**/*.test.tsx`, `**/*.test.js`, `**/*.test.jsx`, `**/*.test.mjs`, `**/*.test.cjs`). Each match is listed as evidence, as `path:line` and the pattern's message. Vendored and built code at the repository root (`vendor/`, `dist/`, `build/`, `generated/`) and installed code (`node_modules/` anywhere) is skipped, and each skipped file is listed in the evidence. The same folders deeper in the repository, such as `src/build/`, are checked. When the host sends no changed lines for a file, as GitHub does for a very large or binary one, that file is listed in the evidence as not checked and every other file is checked; if no other file adds code, the rule is unconfirmed. When a file's content can't be read or a pattern is not valid ast-grep, the rule is unconfirmed rather than passing. Triage reads at most 300 files per pull request, so on one that adds code to more, the rule is unconfirmed with a reason such as **code patterns could not be checked: 412 files with added code, more than the 300 read per pull request**. On Bitbucket Data Center, which does not give Triage each file's changed lines, it is unconfirmed whenever a TypeScript or JavaScript file of your own is added, edited, moved or copied. To give some patterns a different `onFail`, put them in a rule of their own. Code-pattern rules count toward the eight rules of your own, and the browser extension leaves them out. - **Judgement**, questions the decision model answers from the diff and the documentation: - **Check the change is focused on one thing.** The decision model is asked whether the pull request does one thing. When it fails, the verdict comment says it mixes unrelated changes and asks the author to split them out, unless the file sets another `onFail`. A pull request based on another open pull request's branch is checked on its own changes, and the comment says so, such as "Stacked on #1573: only this pull request's own changes were checked." On a branch another pull request was merged into, this rule is skipped, and the comment says why, such as "stacked branch, includes #1521". - **Check docs are updated when behavior changes.** Changing a documentation file passes it. Otherwise the decision model is asked, file by file, whether the docs stay correct, a statement in them is now wrong, or the change adds setup the docs must describe. When it fails, the verdict comment asks the author to update the documentation and names the files, unless the file sets another `onFail`. The default label, when `onFail` is `label`, is `docs: check`. diff --git a/docs/DECISION_MODEL.md b/docs/DECISION_MODEL.md index 9b4181729..a492345b6 100644 --- a/docs/DECISION_MODEL.md +++ b/docs/DECISION_MODEL.md @@ -98,18 +98,18 @@ Two different numbers come out of the answers: | Requests per pull request | 64 (about 5.8 MB of state, $0.17 at most) | `MAX_JUDGE_REQUESTS` | | Repository docs | 24,000 bytes, default branch | `REPO_DOCS_MAX_BYTES` | | Contribution rules | 8,000 bytes, default branch | `REPO_RULES_MAX_BYTES` | -| Patch read per file | 65,536 chars | `MAX_PATCH_INPUT_CHARS` | -| Patch read per pull request | 270,000 chars | `MAX_PATCH_CHARS` | +| Patch the judge reads per file | 65,536 chars | `MAX_PATCH_INPUT_CHARS` | +| Patch the judge reads per pull request | 270,000 chars | `MAX_PATCH_CHARS` | | GitHub tool page | 9,000 chars | `PR_FILES_PAGE_CHARS` | - **Docs.** Docs are read from the default branch in this order: root `README`, then `docs/`, then other root markdown, then nested markdown, each in path order. Reading stops at 24,000 bytes; a doc that crosses the cap is cut and later docs are not seen. The same docs are in every request's state. - **Contribution rules.** Read under their own 8,000-byte budget, in this order: `AGENTS.md`, `CLAUDE.md`, `CONTRIBUTING` and `PULL_REQUEST_TEMPLATE` at the root, then `CONTRIBUTING` and `PULL_REQUEST_TEMPLATE` in `.github/` (including `PULL_REQUEST_TEMPLATE/`), then the same two in `docs/`. An `AGENTS.md` or `CLAUDE.md` inside a package stays an ordinary doc. Only requests that ask a custom rule carry them and give up room for them; requests that ask only built-in rules are unchanged. - **Request splitting.** Files fill each request's state in order up to the budget left after metadata and docs. A file too large for one request keeps its leading hunks there for every rule, and focused and docs read its remaining hunks in the next requests, docs asking about each part (any part needing docs fails it); a hunk too large for any request is cut at line ends across several. Each request asks only about the files it holds; whole-PR questions go in the first, and focused asks in each. If the title, body, commits, path list and docs leave less than 8 KB of the budget for files, nothing is asked and the model rules stay unconfirmed ("pull request too large for the decision model (N KB)", the size of that metadata). The same holds when the split needs more than 64 requests ("… (N requests)"). -- **Oversized patches.** A patch larger than one tool page is read across pages and joined. Reading stops at 65,536 chars for a file, which is then marked cut. Once the pull request's 270,000-char budget is spent, later files are listed without patches. -- **Unread files.** A file with lines changed but no patch to read (GitHub left it out, the per-PR budget was spent, or not even its first hunk fits a request) is unread, and a rule that would ask about it stays unconfirmed. Treatment differs by rule: +- **Oversized patches.** A patch larger than one tool page is read across pages and joined, and every patch is read whole for the rules that run in code. The judge reads 65,536 chars of a file, which is then marked cut. Once the judge's 270,000-char budget is spent, later files are sent to it without patches. +- **Unread files.** A file with lines changed but no patch to read (GitHub left it out, the judge's budget was spent, or not even its first hunk fits a request) is unread, and a rule that would ask about it stays unconfirmed. Treatment differs by rule: - Docs reads a file too large for one request across requests as focused does. A file whose patch was cut while reading is asked about from the hunks that were read and is unread, so it can fail docs but never pass it. A changed doc file passes docs by rule regardless. - Scoped custom rules ask about a cut file (its patch cut while reading, or too large for one request) from the hunks that were read and treat it as unread. - - Focused reads every line of every patch it has. Its only unread files are those whose patch it does not have whole: GitHub left it out, the pull request's 270,000-char budget was spent, or the file passed 65,536 chars. Any unread file leaves focused unconfirmed, listing them, unless another slice already fails it. + - Focused reads every line of every patch it has. Its only unread files are those whose patch it does not have whole: GitHub left it out, the judge's 270,000-char budget was spent, or the file passed 65,536 chars. Any unread file leaves focused unconfirmed, listing them, unless another slice already fails it. - Binary files and pure renames have no lines and are never unread. ## Reused answers diff --git a/packages/triage-workflows/src/actions/facts.ts b/packages/triage-workflows/src/actions/facts.ts index d1942a0ff..9be44ff18 100644 --- a/packages/triage-workflows/src/actions/facts.ts +++ b/packages/triage-workflows/src/actions/facts.ts @@ -4,9 +4,9 @@ import { type } from "arktype"; import { ACTION_KINDS, catalogCheckEnabled, classificationSources, customCheckEnabled, DEFAULT_RISK, emptyPack, isPatternsCheck, PolicySourceShape, repoPolicy, repoSourceOf, triageEventsOf, trustedRequestOf, visibleText, type ActionKind, type CheckPack, type PolicySource, type RepoSource, type TriageEvent } from "@corbits/triage-contracts"; import { repoDocs, type ClosedPr, type IssueComment, type RepoDocsPage, type Review, type RuleSpan } from "@corbits/github-reads"; import { isRecord } from "@corbits/common"; -import { DEPLOYMENT_UNREADABLE, hasModelChecks, NEEDS_SETUP_REASON, packFromInput, type PrFacts } from "../logic/checks.js"; +import { DEPLOYMENT_UNREADABLE, hasModelChecks, NEEDS_SETUP_REASON, packFromInput, type PrFacts, type PrFileFacts } from "../logic/checks.js"; import { asText } from "../logic/extract.js"; -import { addFilesPage, assembleItem, fileFacts, FIRST_FILES_PAGE, optionalCount, namedTeams, type CheckRun, type FilesCursor, type FilesPage, type ListedFiles, type PrData, type PullData, type RepoDocs } from "../logic/facts.js"; +import { addFilesPage, assembleItem, fileFacts, FIRST_FILES_PAGE, optionalCount, namedTeams, type CheckRun, type FilesCursor, type FilesPage, type PrData, type PullData, type RepoDocs } from "../logic/facts.js"; import { couldRepeat, MAX_OPEN_PULLS, overlapping, type OpenPull, type OpenPulls, type PullPrint } from "../logic/duplicates.js"; import type { History } from "../logic/history.js"; import { filesToRead, patternsResult, type PatternsResult } from "../logic/patterns.js"; @@ -239,7 +239,7 @@ async function fetchTargets({ host, stamp }: Host, repo: string, numbers: number const teams = namedTeams(pack, repo); const focused = catalogCheckEnabled(pack, "focused"); const risk = catalogCheckEnabled(pack, "regression-risk") || catalogCheckEnabled(pack, "risk"); - const listed = new Map(); + const listed = new Map(); const filesErrors = new Map(); function filesId(n: number, { offset, patchOffset }: FilesCursor): string { @@ -247,8 +247,8 @@ async function fetchTargets({ host, stamp }: Host, repo: string, numbers: number } function filesCall([n, cursor]: [number, FilesCursor]): ToolCall { - const { offset, patchOffset, patches } = cursor; - return call(tool("list_pr_files"), { repo, number: n, offset, ...(patchOffset === undefined ? {} : { patchOffset }), ...(patches ? {} : { patches: false }) }, filesId(n, cursor)); + const { offset, patchOffset } = cursor; + return call(tool("list_pr_files"), { repo, number: n, offset, ...(patchOffset === undefined ? {} : { patchOffset }) }, filesId(n, cursor)); } function prCalls(n: number): ToolCall[] { @@ -264,7 +264,7 @@ async function fetchTargets({ host, stamp }: Host, repo: string, numbers: number function addFiles(n: number, cursor: FilesCursor, result: ToolResult | undefined): FilesCursor | undefined { const page = filesPage(tool("list_pr_files"), result); - const read = listed.get(n) ?? { files: [], patchChars: 0 }; + const read = listed.get(n) ?? []; listed.set(n, read); const next = typeof page === "string" ? page : addFilesPage(read, cursor, page); if (typeof next !== "string") return next; @@ -297,7 +297,7 @@ async function fetchTargets({ host, stamp }: Host, repo: string, numbers: number } function historyPathsOf(n: number): string[] { - return historyPaths(listed.get(n)?.files.map((file) => file.path) ?? []); + return historyPaths(listed.get(n)?.map((file) => file.path) ?? []); } function historyCalls(): ToolCall[] { @@ -309,7 +309,7 @@ async function fetchTargets({ host, stamp }: Host, repo: string, numbers: number /** A pull request the host read whole, as the duplicate rules compare it. */ function ownPull(n: number): OwnPull[] { const pr = prs.get(n); - const files = listed.get(n)?.files; + const files = listed.get(n); if (!pr?.sha || !pr.base || pr.title === undefined || !files || filesErrors.has(n)) return []; return [{ number: n, title: visibleText(pr.title), headSha: pr.sha, base: pr.base, paths: files.map((file) => file.path) }]; } @@ -355,7 +355,7 @@ async function fetchTargets({ host, stamp }: Host, repo: string, numbers: number comments: data<{ comments: IssueComment[] }>(r1.get(`comments:${n}`))?.comments ?? [], commits: data<{ commits: Array<{ message?: string }> }>(r1.get(`commits:${n}`))?.commits ?? [], stack: data>(r1.get(`stack:${n}`)), - files: filesErrors.get(n) ?? listed.get(n)?.files ?? [], + files: filesErrors.get(n) ?? listed.get(n) ?? [], authorTeams: data>(r2.get(`teams:${prs.get(n)?.author}`)), fixes: fixesOf(n), }, { repo, policy, pack, events, docs, templates, source: stamp.repoSource }); diff --git a/packages/triage-workflows/src/logic/checks.test.ts b/packages/triage-workflows/src/logic/checks.test.ts index 9d216ea9b..a1c365298 100644 --- a/packages/triage-workflows/src/logic/checks.test.ts +++ b/packages/triage-workflows/src/logic/checks.test.ts @@ -447,7 +447,7 @@ rules: test("a file without its patch, as on Bitbucket Data Center, an unread content or a broken rule never passes", () => { const unpatched = { path: "src/a.ts", status: "modified", additions: 0, deletions: 0 }; - expect(filesToRead([unpatched])).toEqual({ unread: "src/a.ts: its patch was not read in full" }); + expect(filesToRead([unpatched])).toEqual({ unread: "the host sent no diff for src/a.ts" }); expect(patterns([unpatched], {}).checks.find((check) => check.check === "conventions")).toMatchObject({ result: "unconfirmed", output: { kind: "could-not-check" } }); expect(patterns([file], {}).checks.find((check) => check.check === "conventions")).toMatchObject({ result: "unconfirmed" }); const nested = patterns([{ ...file, path: "src/build/a.ts" }], {}); @@ -457,6 +457,17 @@ rules: expect(patterns([file], { "src/a.ts": content }, broken).checks.find((check) => check.check === "broken")).toMatchObject({ result: "unconfirmed" }); }); + test("a file the host sent no diff for is named as not checked, and every other file is checked", () => { + const large = { path: "src/large.ts", status: "modified", additions: 9_000, deletions: 0 }; + const clean = { path: "src/b.ts", status: "added", additions: 1, deletions: 0, patch: "@@ -0,0 +1 @@\n+const b = 1;" }; + const left = "src/large.ts: not checked, the host sent no diff for it"; + expect(filesToRead([file, large, clean])).toEqual(["src/a.ts", "src/b.ts"]); + const failed = patterns([file, large], { "src/a.ts": content }); + expect(failed.checks.find((check) => check.check === "conventions")).toMatchObject({ result: "fail", evidence: expect.arrayContaining([left]), skipped: [left] }); + const passed = patterns([clean, large], { "src/b.ts": "const b = 1;\n" }); + expect(passed.checks.find((check) => check.check === "conventions")).toMatchObject({ result: "pass", reason: "no code pattern matched; 1 file was not checked because the host sent no diff for it", evidence: [left] }); + }); + test("an unknown preset is refused", () => { expect(() => readPolicyFile(FILE.replace("[convention]", "[nope]"), "acme/widgets")).toThrow(); }); diff --git a/packages/triage-workflows/src/logic/checks.ts b/packages/triage-workflows/src/logic/checks.ts index fd61e697c..3c449dbfa 100644 --- a/packages/triage-workflows/src/logic/checks.ts +++ b/packages/triage-workflows/src/logic/checks.ts @@ -55,8 +55,6 @@ export interface PrFileFacts { additions?: number; deletions?: number; patch?: string; - /** Past the patch budget. */ - patchTruncated?: true; } export interface PrFacts { @@ -642,8 +640,10 @@ function riskResult(facts: PrFacts, rules: CheckRules): CheckResult { return { ...base, result: "pass", ...(outcome === "label" && level !== "low" && { label: levelLabel("risk", level) }) }; } -function patternsReason(failed: boolean, found: { skipped: string[]; checked: number }): string { +function patternsReason(failed: boolean, found: { skipped: string[]; checked: number; unpatched?: number }): string { if (failed) return "added code matches a code pattern"; + if (found.unpatched === 1) return "no code pattern matched; 1 file was not checked because the host sent no diff for it"; + if (found.unpatched) return `no code pattern matched; ${found.unpatched} files were not checked because the host sent no diff for them`; if (found.checked === 0 && found.skipped.length > 0) return "nothing checked: the only TypeScript or JavaScript changed is vendored, built or installed code"; if (found.checked === 0) return "nothing checked: no TypeScript or JavaScript code added"; return "no code pattern matched"; diff --git a/packages/triage-workflows/src/logic/facts.test.ts b/packages/triage-workflows/src/logic/facts.test.ts index ac9c6b41e..29b96cb59 100644 --- a/packages/triage-workflows/src/logic/facts.test.ts +++ b/packages/triage-workflows/src/logic/facts.test.ts @@ -1,7 +1,11 @@ import { describe, expect, test } from "bun:test"; -import { DEFAULT_REPO_POLICY, emptyPack } from "@corbits/triage-contracts"; +import { DEFAULT_REPO_POLICY, emptyPack, isPatternsCheck, readCheckPack, readPolicyFile } from "@corbits/triage-contracts"; import type { IssueComment, Review } from "@corbits/github-reads"; -import { assembleItem, buildFacts } from "./facts.ts"; +import type { PrFileFacts } from "./checks.ts"; +import { addFilesPage, assembleItem, buildFacts, FIRST_FILES_PAGE, type FilesCursor, type FilesPage } from "./facts.ts"; +import { NO_FACTS } from "./item.ts"; +import { patternsResult } from "./patterns.ts"; +import { MAX_SYSTEM_ONE_STATE_BYTES, prepareQualityEvaluation } from "./quality.ts"; test("buildFacts carries the author's trust tier", () => { const facts = buildFacts("acme/widgets", 1, { author: "alice", authorAssociation: "MEMBER", branch: "a" }, [], [], [], DEFAULT_REPO_POLICY); @@ -81,3 +85,36 @@ describe("an answered review", () => { expect(answered([requested], [{ author: "ci[bot]", bot: true }, { author: "maya" }, { createdAt: "2026-10-01T09:00:00Z" }])).toBe(false); }); }); + +test("a diff past the judge's budget is read whole for the pattern rules while the judge reads a bounded part", () => { + const filler = Array.from({ length: 200 }, (_, line) => `const filler${line} = "${"x".repeat(30)}";`); + const fallback = "const late = value ?? 0;"; + const paths = Array.from({ length: 40 }, (_, index) => `src/file${String(index).padStart(2, "0")}.ts`); + const lines = (index: number) => (index === paths.length - 1 ? [...filler, fallback] : filler); + const patch = (index: number) => [`@@ -0,0 +1,${lines(index).length} @@`, ...lines(index).map((line) => `+${line}`)].join("\n"); + const hostFiles: PrFileFacts[] = paths.map((path, index) => ({ path, status: "added", additions: lines(index).length, deletions: 0, patch: patch(index) })); + const pages: FilesPage[] = hostFiles.flatMap((file, index): FilesPage[] => { + const half = Math.floor(file.patch!.length / 2); + const next = index + 1 < hostFiles.length ? { next: index + 1 } : {}; + return [{ files: [{ ...file, patch: file.patch!.slice(0, half) }], next: index, patchNext: half }, { files: [{ ...file, patch: file.patch!.slice(half) }], ...next }]; + }); + const listed: PrFileFacts[] = []; + let cursor: FilesCursor | undefined | string = FIRST_FILES_PAGE; + for (const page of pages) { + if (cursor === undefined || typeof cursor === "string") throw new Error(`stopped early: ${cursor}`); + cursor = addFilesPage(listed, cursor, page); + } + expect(cursor).toBeUndefined(); + expect(listed).toEqual(hostFiles); + expect(hostFiles.reduce((chars, file) => chars + file.patch!.length, 0)).toBeGreaterThan(3 * MAX_SYSTEM_ONE_STATE_BYTES); + + const pack = readCheckPack(readPolicyFile("rules:\n - id: conventions\n check: code-patterns\n text: Conventions.\n reason: Review time.\n evidence: [maintainer-stated]\n presets: [convention]\n", "acme/widgets")); + const contents = Object.fromEntries(paths.map((path, index) => [path, `${lines(index).join("\n")}\n`])); + expect(patternsResult(pack.custom.find(isPatternsCheck)!, listed, contents)).toEqual({ matches: [`${paths.at(-1)}:201 Invented fallback \`?? 0\`; handle the missing value instead.`], skipped: [], checked: paths.length }); + + const focused = { quality: [{ id: "focused" as const, group: "pull-request" as const }], custom: [], rules: [] }; + const { requests, focused: plan } = prepareQualityEvaluation({ ...NO_FACTS, repo: "acme/widgets", number: 8, title: "Add files", files: listed, paths }, focused); + const judged = requests.flatMap((request) => request.state.pullRequest.files.flatMap((file) => file.hunks)).join("").length; + expect(judged).toBeLessThanOrEqual(3 * MAX_SYSTEM_ONE_STATE_BYTES + 65_536); + expect(plan!.unread).toContain(paths.at(-1)!); +}); diff --git a/packages/triage-workflows/src/logic/facts.ts b/packages/triage-workflows/src/logic/facts.ts index 453b262bc..ce8a9b3b1 100644 --- a/packages/triage-workflows/src/logic/facts.ts +++ b/packages/triage-workflows/src/logic/facts.ts @@ -1,10 +1,8 @@ import { checkCancelled, checkFailed, checkPending, latestDecisive, listPrFilesPage, readPrTemplates, reviewedOnHead, summarizeChecks, type CheckRun, type GithubFetch, type IssueComment, type Review } from "@corbits/github-reads"; import { authorAssociation, authorKindOf, catalogCheckEnabled, policyTeams, shownBody, teamSlug, tierOf, visibleText, type CheckPack, type RepoPolicy, type RepoSource, type TriageEvent } from "@corbits/triage-contracts"; import { hiddenIn, hiddenMarkup, nothingRemoved, writtenDescription } from "./hidden-content.js"; -import { MAX_PATCH_INPUT_CHARS } from "./hunks.js"; import type { PrFacts, PrFileFacts } from "./checks.js"; import { isRecord } from "@corbits/common"; -import { MAX_SYSTEM_ONE_STATE_BYTES } from "./quality.js"; export type RepoDocs = Pick; @@ -95,48 +93,37 @@ export function buildFacts( }; } -// Patch text past three judge requests' worth is not read, so later pages leave patches out. -const MAX_PATCH_CHARS = 3 * MAX_SYSTEM_ONE_STATE_BYTES; - export type FilesPage = { files: PrFileFacts[]; next?: number; patchNext?: number }; -export type FilesCursor = { offset: number; patchOffset?: number; patches: boolean }; - -export type ListedFiles = { files: PrFileFacts[]; patchChars: number }; +export type FilesCursor = { offset: number; patchOffset?: number }; -export const FIRST_FILES_PAGE: FilesCursor = { offset: 0, patches: true }; +export const FIRST_FILES_PAGE: FilesCursor = { offset: 0 }; /** * Adds one page to `listed`, joining a patch continued from the previous page, and returns the next page's cursor, - * none when every file is read, or why the pages cannot be read. A patch past the budget is left cut and marked so. + * none when every file is read, or why the pages cannot be read. Every patch the host sends is read whole. */ -export function addFilesPage(listed: ListedFiles, cursor: FilesCursor, page: FilesPage): FilesCursor | undefined | string { +export function addFilesPage(listed: PrFileFacts[], cursor: FilesCursor, page: FilesPage): FilesCursor | undefined | string { let files = page.files; if (cursor.patchOffset !== undefined) { - const continued = listed.files.at(-1); + const continued = listed.at(-1); if (!continued || files[0]?.path !== continued.path) return `github_list_pr_files did not continue the patch at offset ${cursor.offset}`; continued.patch = (continued.patch ?? "") + (files[0].patch ?? ""); - listed.patchChars += files[0].patch?.length ?? 0; files = files.slice(1); } - listed.files.push(...files); - listed.patchChars += files.reduce((chars, file) => chars + (file.patch?.length ?? 0), 0); - const patches = listed.patchChars < MAX_PATCH_CHARS; + listed.push(...files); if (page.next === undefined) return undefined; if (page.patchNext !== undefined) { if (page.next < cursor.offset || (page.next === cursor.offset && page.patchNext <= (cursor.patchOffset ?? 0))) return `github_list_pr_files did not advance past offset ${cursor.offset}`; - const cut = listed.files.at(-1)!; - if (patches && (cut.patch?.length ?? 0) < MAX_PATCH_INPUT_CHARS) return { offset: page.next, patchOffset: page.patchNext, patches }; - cut.patchTruncated = true; - return { offset: page.next + 1, patches }; + return { offset: page.next, patchOffset: page.patchNext }; } if (page.next <= cursor.offset) return `github_list_pr_files did not advance past offset ${cursor.offset}`; - return { offset: page.next, patches }; + return { offset: page.next }; } /** Every changed file of a pull request, read page by page as the facts director reads them. */ export async function listFiles(gh: GithubFetch, repo: string, number: number): Promise { - const listed: ListedFiles = { files: [], patchChars: 0 }; + const listed: PrFileFacts[] = []; let cursor: FilesCursor | undefined = FIRST_FILES_PAGE; while (cursor !== undefined) { const page = await listPrFilesPage(gh, repo, number, cursor); @@ -144,7 +131,7 @@ export async function listFiles(gh: GithubFetch, repo: string, number: number): if (typeof next === "string") throw new Error(`${next} for #${number}`); cursor = next; } - return listed.files; + return listed; } function optionalString(value: unknown): string | undefined { @@ -171,7 +158,6 @@ export function fileFacts(value: unknown): PrFileFacts[] { ...(additions === undefined ? {} : { additions }), ...(deletions === undefined ? {} : { deletions }), ...(patch === undefined ? {} : { patch }), - ...(value.patchTruncated === true ? { patchTruncated: true as const } : {}), }]; } diff --git a/packages/triage-workflows/src/logic/hunks.ts b/packages/triage-workflows/src/logic/hunks.ts index e688d7238..5d780fffb 100644 --- a/packages/triage-workflows/src/logic/hunks.ts +++ b/packages/triage-workflows/src/logic/hunks.ts @@ -63,9 +63,9 @@ function hunkLines(hunk: Hunk): { added: number[]; removed: number[] } { return { added, removed }; } -/** The new file's line numbers, from 1, that the patch adds. */ +/** The new file's line numbers, from 1, that the whole patch adds. */ export function addedLines(patch: string): Set { - return new Set(hunksOf(readablePatch(patch)).flatMap((hunk) => hunkLines(hunk).added)); + return new Set(hunksOf(safeText(patch)).flatMap((hunk) => hunkLines(hunk).added)); } function citation(path: string, previousPath: string, hunk: Hunk): LineCitation { diff --git a/packages/triage-workflows/src/logic/patterns.ts b/packages/triage-workflows/src/logic/patterns.ts index b0e10efaf..a1c5ddfc0 100644 --- a/packages/triage-workflows/src/logic/patterns.ts +++ b/packages/triage-workflows/src/logic/patterns.ts @@ -1,7 +1,7 @@ import { parse } from "@ast-grep/napi"; import { isForeignPath, PATTERN_EXTENSIONS, PATTERN_PRESETS, type PatternExtension, patternRuleApplies, type PatternLanguage, type PatternRule, type PatternsCheck } from "@corbits/triage-contracts"; import type { PrFileFacts } from "./checks.js"; -import { addedLines, MAX_PATCH_INPUT_CHARS } from "./hunks.js"; +import { addedLines } from "./hunks.js"; type Grammar = { language: PatternLanguage; lang: "TypeScript" | "Tsx" | "JavaScript" }; @@ -18,7 +18,8 @@ const GRAMMARS: Record = { jsx: JAVASCRIPT, }; -export type PatternsResult = { matches: string[]; skipped: string[]; checked: number } | { unread: string }; +/** `unpatched` counts the files in scope left out because they came without a patch. */ +export type PatternsResult = { matches: string[]; skipped: string[]; checked: number; unpatched?: number } | { unread: string }; function presetRules(id: string): readonly PatternRule[] { const rules = PATTERN_PRESETS[id]; @@ -46,8 +47,14 @@ function inScope(file: PrFileFacts): boolean { return checkable(file) && !isForeignPath(file.path); } -function wholePatch(file: PrFileFacts): string | undefined { - return file.patch === undefined || file.patchTruncated || file.patch.length > MAX_PATCH_INPUT_CHARS ? undefined : file.patch; +/** A file in scope that came without a patch: GitHub sends none for a very large or binary file, Bitbucket Data Center for any. */ +function unpatched(file: PrFileFacts): boolean { + return inScope(file) && file.patch === undefined; +} + +function namedUnpatched(files: readonly PrFileFacts[]): string { + const more = files.length > 1 ? ` and ${files.length - 1} more` : ""; + return `the host sent no diff for ${files[0]!.path}${more}`; } function readPath(file: PrFileFacts): string { @@ -57,12 +64,14 @@ function readPath(file: PrFileFacts): string { /** The files one pull request reads the content of, GitHub's own diff limit, so one pull request's reads stay bounded. */ export const MAX_CONTENT_READS = 300; -/** The files whose content the rules need, or why they cannot be checked: a file in scope without its whole patch, or more files than are read. */ +/** + * The files whose content the rules need, or why they cannot be checked: files in scope came without a patch and none + * other adds code, or more files than are read. + */ export function filesToRead(files: readonly PrFileFacts[]): string[] | { unread: string } { - const scoped = files.filter(inScope); - const unpatched = scoped.find((file) => wholePatch(file) === undefined); - if (unpatched) return { unread: `${unpatched.path}: its patch was not read in full` }; - const toRead = scoped.filter((file) => addedLines(wholePatch(file)!).size > 0).map(readPath); + const toRead = files.filter((file) => inScope(file) && file.patch !== undefined && addedLines(file.patch).size > 0).map(readPath); + const missing = files.filter(unpatched); + if (toRead.length === 0 && missing.length > 0) return { unread: namedUnpatched(missing) }; if (toRead.length > MAX_CONTENT_READS) return { unread: `${toRead.length} files with added code, more than the ${MAX_CONTENT_READS} read per pull request` }; return toRead; } @@ -84,7 +93,10 @@ function fileMatches(file: PrFileFacts, patch: string, content: string, grammar: return [...out]; } -/** Matches on the lines a pull request adds; a file in scope whose patch or content was not read leaves the rule unchecked. */ +/** + * Matches on the lines a pull request adds; a file in scope whose content was not read leaves the rule unchecked, and + * one that came without a patch is named as not checked. + */ export function patternsResult(check: PatternsCheck, files: readonly PrFileFacts[], contents: Readonly>): PatternsResult { const toRead = filesToRead(files); if (!Array.isArray(toRead)) return toRead; @@ -99,8 +111,10 @@ export function patternsResult(check: PatternsCheck, files: readonly PrFileFacts } catch (error) { return { unread: error instanceof Error ? error.message : String(error) }; } - const skipped = files - .filter((file) => checkable(file) && isForeignPath(file.path)) - .map((file) => `${file.path}: skipped as vendored, built or installed code`); - return { matches, skipped, checked: files.filter(inScope).length }; + const missing = files.filter(unpatched); + const skipped = [ + ...missing.map((file) => `${file.path}: not checked, the host sent no diff for it`), + ...files.filter((file) => checkable(file) && isForeignPath(file.path)).map((file) => `${file.path}: skipped as vendored, built or installed code`), + ]; + return { matches, skipped, checked: files.filter(inScope).length - missing.length, ...(missing.length > 0 && { unpatched: missing.length }) }; } diff --git a/packages/triage-workflows/src/logic/quality.test.ts b/packages/triage-workflows/src/logic/quality.test.ts index 55954686d..926abe484 100644 --- a/packages/triage-workflows/src/logic/quality.test.ts +++ b/packages/triage-workflows/src/logic/quality.test.ts @@ -136,19 +136,19 @@ test("quality preflight refuses metadata that leaves too little room for files, expect(subject.requests[0]!.state.pullRequest.commits).toEqual(["x".repeat(200)]); const crowded = prepareQualityEvaluation({ ...facts([{ path: "src/a.ts", patch }]), docs: { "README.md": "d".repeat(85_000) } }, SOURCES); expect(crowded).toMatchObject({ requests: [], judgeError: expect.stringMatching(/^pull request too large for the decision model \(\d+ KB\)$/) }); - const wide = prepareQualityEvaluation(facts(Array.from({ length: 40 }, (_, index) => ({ path: `src/wide${index}.ts`, patch: widePatch() }))), SOURCES); + const wide = prepareQualityEvaluation({ ...facts(Array.from({ length: 40 }, (_, index) => ({ path: `src/wide${index}.ts`, patch: widePatch() }))), docs: { "README.md": "d".repeat(78_000) } }, SOURCES); expect(wide).toMatchObject({ requests: [], judgeError: expect.stringMatching(/^pull request too large for the decision model \(\d+ requests\)$/) }); }); test("quality preflight leaves a cut patch unread for focused even when its first hunk is read in pieces", () => { - const patch = ["@@ -0,0 +1,60 @@", ...Array.from({ length: 60 }, () => `+// ${"設".repeat(1_000)}`)].join("\n"); - const { focused } = prepareQualityEvaluation(facts([{ path: "src/big.ts", additions: 60, deletions: 0, patch, patchTruncated: true }]), SOURCES); + const patch = ["@@ -0,0 +1,70 @@", ...Array.from({ length: 70 }, () => `+// ${"設".repeat(1_000)}`)].join("\n"); + const { focused } = prepareQualityEvaluation(facts([{ path: "src/big.ts", additions: 70, deletions: 0, patch }]), SOURCES); expect(focused!.unread).toEqual(["src/big.ts"]); }); test("quality preflight leaves a cut patch unread for docs even when its read part fits", () => { const sources = { ...SOURCES, quality: [...SOURCES.quality, { id: "docs" as const, group: "code-vs-ci" as const }] }; - const { plan } = prepareQualityEvaluation(facts([{ path: "src/cut.ts", additions: 9_000, deletions: 0, patch: compactHunks(2), patchTruncated: true }]), sources); + const { plan } = prepareQualityEvaluation(facts([{ path: "src/cut.ts", additions: 1_400, deletions: 0, patch: compactHunks(1_400) }]), sources); expect(plan.docs!.unread).toEqual(["src/cut.ts"]); }); diff --git a/packages/triage-workflows/src/logic/quality.ts b/packages/triage-workflows/src/logic/quality.ts index c23e32899..1264c4d7f 100644 --- a/packages/triage-workflows/src/logic/quality.ts +++ b/packages/triage-workflows/src/logic/quality.ts @@ -1,6 +1,6 @@ import { isDocPath, isGeneratedPath, isTestPath, type CheckItem, type ModelCustomCheck, type QualityCheckId } from "@corbits/triage-contracts"; import { holdsOthersChanges, type DeterministicResult, type PrFacts, type PrFileFacts } from "./checks.js"; -import { boundedPath, hunkCitations, patchHunks } from "./hunks.js"; +import { boundedPath, hunkCitations, MAX_PATCH_INPUT_CHARS, patchHunks } from "./hunks.js"; import { globMatch } from "./glob.js"; import type { OpenPull } from "./duplicates.js"; import type { BooleanQuestion, ChoiceQuestion, ScoreQuestion } from "@corbits/system-one"; @@ -182,9 +182,21 @@ const MIN_FILE_ROOM = 8 * 1024; const MAX_JUDGE_REQUESTS = 64; export const QUALITY_EVALUATION_LIMIT_ERROR = "pull request too large for the decision model"; -/** Lockfiles say nothing the judge decides on, so it never reads them. */ -function judgedFiles(facts: PrFacts) { - return facts.files.filter((file) => !isGeneratedPath(file.path)); +// The judge reads patches up to three requests' worth; later files are sent without theirs. +const MAX_PATCH_CHARS = 3 * MAX_SYSTEM_ONE_STATE_BYTES; + +/** A changed file as the judge reads it: `patchTruncated` when its patch was cut at `MAX_PATCH_INPUT_CHARS`. */ +type JudgedFile = PrFileFacts & { patchTruncated?: true }; + +/** Lockfiles say nothing the judge decides on, so it never reads them; the rest share the judge's patch budget. */ +function judgedFiles(facts: PrFacts): JudgedFile[] { + let chars = 0; + return facts.files.filter((file) => !isGeneratedPath(file.path)).map(function judged({ patch, ...file }): JudgedFile { + if (patch === undefined || chars >= MAX_PATCH_CHARS) return file; + const read = patch.slice(0, MAX_PATCH_INPUT_CHARS); + chars += read.length; + return { ...file, patch: read, ...(read.length < patch.length && { patchTruncated: true as const }) }; + }); } // Custom checks are the repository's own rules, so their requests carry its contribution rules and say where they are. @@ -582,7 +594,7 @@ export function verdictRequest(facts: PrFacts, rules: PolicyRule[]): { request: return { request: { state: { ...empty, pullRequest: { ...empty.pullRequest, files } }, questions: [VERDICT_QUESTION, INSTRUCTED_QUESTION] } }; } -function stateFile(file: PrFileFacts, hunks: string[]): StateFile { +function stateFile(file: JudgedFile, hunks: string[]): StateFile { return { path: boundedPath(file.path), ...(file.status ? { status: file.status } : {}), hunks, ...(file.patchTruncated ? { truncated: true as const } : {}) }; } diff --git a/packages/triage-workflows/src/logic/render.ts b/packages/triage-workflows/src/logic/render.ts index 476725b7a..bf13ff4f2 100644 --- a/packages/triage-workflows/src/logic/render.ts +++ b/packages/triage-workflows/src/logic/render.ts @@ -679,7 +679,8 @@ function authorComment(ctx: RenderContext, checks: CheckResult[], verdict: Verdi } if (pending.length) { if (lines.length) lines.push(""); - lines.push("Waiting on:", ...pending.map((c) => `- ${c.reason}${failureEvidence(c, ctx)}`)); + // Rules held by the same unread input give the same line, so it is said once. + lines.push("Waiting on:", ...new Set(pending.map((c) => `- ${c.reason}${failureEvidence(c, ctx)}`))); } if (outageCount(checks) > 0) { if (lines.length) lines.push(""); @@ -705,7 +706,7 @@ function reasonOf(c: CheckResult, sources: Sources): string { function reasonsOf(det: DeterministicResult, model: CheckResult[]): string[] { const hard = det.checks.filter((c) => c.result === "fail" || (c.result === "unconfirmed" && c.check !== NOT_A_DRAFT)); const soft = model.filter((c) => c.result === "fail" || unsure(c)); - return [...hard.filter((c) => c.result === "fail"), ...hard.filter((c) => c.result !== "fail"), ...soft].map((c) => reasonOf(c, det.sources)); + return [...new Set([...hard.filter((c) => c.result === "fail"), ...hard.filter((c) => c.result !== "fail"), ...soft].map((c) => reasonOf(c, det.sources)))]; } type VerdictBase = Omit; From c1e38bccea3498071ad4e6a1fb25b366d71682af Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Tue, 27 Oct 2026 16:59:54 -0700 Subject: [PATCH 2/9] fix(triage-workflows): cap the patch text the code rules read at 2,000,000 characters One sidecar holds every deployment's step outputs, so an unbounded diff is a memory risk. Past the ceiling, later files are listed without patches and the pattern rules name them as not checked instead of refusing the pull request. --- docs/ARCHITECTURE.md | 12 ++-- .../Corbits Triage User Manual.md | 2 +- docs/DECISION_MODEL.md | 3 +- .../triage-workflows/src/actions/facts.ts | 18 ++--- .../triage-workflows/src/logic/checks.test.ts | 6 +- packages/triage-workflows/src/logic/checks.ts | 6 +- .../triage-workflows/src/logic/facts.test.ts | 68 +++++++++++++++---- packages/triage-workflows/src/logic/facts.ts | 36 +++++++--- .../triage-workflows/src/logic/patterns.ts | 17 +++-- .../triage-workflows/src/logic/quality.ts | 3 + 10 files changed, 121 insertions(+), 50 deletions(-) diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 8e9ae0529..940404083 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -549,10 +549,14 @@ label off that head, so it never stands on a head not yet triaged. (ast-grep's glob rules, checked when the policy is read), and keeps matches on lines the patch adds. The facts step reads every changed file's patch whole, across as many - tool pages as it takes; only the judge's view of the diff is bounded - (`judgedFiles` in `logic/quality.ts`). A file the host sent no patch for - (GitHub omits it for a very large or binary file) is listed in the - evidence as not checked and the rule runs on the rest; with nothing else + tool pages as it takes, up to 2,000,000 characters per pull request + (`MAX_CODE_RULES_PATCH_CHARS`), since one sidecar holds every + deployment's step outputs; the file that passes it partway and every + later one are listed without patches. The judge's view of the diff has + its own, smaller budget (`judgedFiles` in `logic/quality.ts`). A file + without a patch, because the host sent none (GitHub omits it for a very + large or binary file) or the diff passed that ceiling, is listed in the + evidence as not checked, saying which, and the rule runs on the rest; with nothing else to check, or a file whose content was not read, or a rule ast-grep refuses, the rule is unconfirmed; `vendor/`, `dist/`, `build/` and `generated/` at the repository root and `node_modules/` anywhere are diff --git a/docs/Corbits Triage User Manual/Corbits Triage User Manual.md b/docs/Corbits Triage User Manual/Corbits Triage User Manual.md index 231cd64b9..5148aaac5 100644 --- a/docs/Corbits Triage User Manual/Corbits Triage User Manual.md +++ b/docs/Corbits Triage User Manual/Corbits Triage User Manual.md @@ -258,7 +258,7 @@ A rule asks one question about a pull request and answers pass, fail or unconfir - **Flag changes to sensitive paths.** Checks changed files against the file's named groups of globs, each `high` or `medium`, written as `groups:`, such as `infra: { level: high, paths: ["infra/**", "**/*.tf"] }`. Without `groups` there are four: `schema` (`**/migrations/**`, `**/*.sql`) and `auth` (`**/auth/**`, `**/secrets/**`), which are high, and `ci` (`.github/**`, `**/Dockerfile`, `**/railway.json`, `**/railway.toml`) and `locks` (`**/bun.lock`, `**/package-lock.json`, `**/yarn.lock`, `**/pnpm-lock.yaml`), which are medium, since every dependency bump changes a lockfile. Globs read as in **Never accept changes to**. A change to a group at `failAt` or above, high unless the file says, fails the rule, which asks a maintainer unless the file gives it another `onFail`. Below that, from `labelAt`, medium unless the file says, it passes but labels the pull request `sensitive: medium` and notes it under **Notes:** (on Bitbucket Data Center, only the note). Its evidence names each group with its files, at most five, such as `schema: db/migrations/0004.sql`, then how many more. Regression risk keeps its own migration globs. - **Flag high-risk pull requests and label medium-risk ones.** Rates each pull request low, medium or high risk; see [Risk](#risk). - **Fail when …**, a text rule of your own, such as "Fail when any commit message contains "Co-Authored-By:"". It names where to look (the title, the description, any commit message, or several of them), whether the rule fails when the text **contains** or **does not contain** its phrases, and the phrases. Case does not matter. A **contains** rule on the description also looks at its hidden parts, such as HTML comments and the addresses behind links, while **does not contain** reads only what a reader sees. Only the first 100 commits are read. When a **contains** rule fails, Triage names each place and the phrases found there, asks for them to be removed, and quotes each matching line once. Text rules count toward the eight rules of your own. - - **Fail when added code matches …**, a code-pattern rule of your own, such as one that flags `?? 0`. It checks only the lines the pull request adds to TypeScript and JavaScript files (`.ts`, `.tsx`, `.mts`, `.cts`, `.js`, `.jsx`, `.mjs`, `.cjs`), so code already on the branch never fails it. On any other language it passes with **nothing checked: no TypeScript or JavaScript code added**, so don't add one to a repository written in Go, Python or similar. Turn on the shipped `convention` preset, which flags invented fallbacks (`?? 0`, `?? []`), nested ternaries, functions called where they are written, `.then`/`.catch` chains, empty `catch` blocks and lint or type suppression comments in both languages, or write your own patterns in [ast-grep](https://ast-grep.github.io/guide/rule-config.html)'s rule format. Your own pattern replaces a preset's pattern with the same `id` in both languages; to keep checking JavaScript, add a `language: javascript` pattern under the same id. A pattern takes ast-grep's own `files` and `ignores` lists to check only some paths or skip some. They follow ast-grep's glob rules: paths are from the repository root, `*` also matches across folders, and dotfiles are matched like any other file. Write each glob as a string; the `{ glob, caseInsensitive }` form is not read. Write a negated class as `[^a]`; a glob with `[!a]`, or one that isn't valid, refuses the policy. The preset's `?? 0` and `?? []` patterns ignore test files (`**/*.test.ts`, `**/*.test.tsx`, `**/*.test.js`, `**/*.test.jsx`, `**/*.test.mjs`, `**/*.test.cjs`). Each match is listed as evidence, as `path:line` and the pattern's message. Vendored and built code at the repository root (`vendor/`, `dist/`, `build/`, `generated/`) and installed code (`node_modules/` anywhere) is skipped, and each skipped file is listed in the evidence. The same folders deeper in the repository, such as `src/build/`, are checked. When the host sends no changed lines for a file, as GitHub does for a very large or binary one, that file is listed in the evidence as not checked and every other file is checked; if no other file adds code, the rule is unconfirmed. When a file's content can't be read or a pattern is not valid ast-grep, the rule is unconfirmed rather than passing. Triage reads at most 300 files per pull request, so on one that adds code to more, the rule is unconfirmed with a reason such as **code patterns could not be checked: 412 files with added code, more than the 300 read per pull request**. On Bitbucket Data Center, which does not give Triage each file's changed lines, it is unconfirmed whenever a TypeScript or JavaScript file of your own is added, edited, moved or copied. To give some patterns a different `onFail`, put them in a rule of their own. Code-pattern rules count toward the eight rules of your own, and the browser extension leaves them out. + - **Fail when added code matches …**, a code-pattern rule of your own, such as one that flags `?? 0`. It checks only the lines the pull request adds to TypeScript and JavaScript files (`.ts`, `.tsx`, `.mts`, `.cts`, `.js`, `.jsx`, `.mjs`, `.cjs`), so code already on the branch never fails it. On any other language it passes with **nothing checked: no TypeScript or JavaScript code added**, so don't add one to a repository written in Go, Python or similar. Turn on the shipped `convention` preset, which flags invented fallbacks (`?? 0`, `?? []`), nested ternaries, functions called where they are written, `.then`/`.catch` chains, empty `catch` blocks and lint or type suppression comments in both languages, or write your own patterns in [ast-grep](https://ast-grep.github.io/guide/rule-config.html)'s rule format. Your own pattern replaces a preset's pattern with the same `id` in both languages; to keep checking JavaScript, add a `language: javascript` pattern under the same id. A pattern takes ast-grep's own `files` and `ignores` lists to check only some paths or skip some. They follow ast-grep's glob rules: paths are from the repository root, `*` also matches across folders, and dotfiles are matched like any other file. Write each glob as a string; the `{ glob, caseInsensitive }` form is not read. Write a negated class as `[^a]`; a glob with `[!a]`, or one that isn't valid, refuses the policy. The preset's `?? 0` and `?? []` patterns ignore test files (`**/*.test.ts`, `**/*.test.tsx`, `**/*.test.js`, `**/*.test.jsx`, `**/*.test.mjs`, `**/*.test.cjs`). Each match is listed as evidence, as `path:line` and the pattern's message. Vendored and built code at the repository root (`vendor/`, `dist/`, `build/`, `generated/`) and installed code (`node_modules/` anywhere) is skipped, and each skipped file is listed in the evidence. The same folders deeper in the repository, such as `src/build/`, are checked. When the host sends no changed lines for a file, as GitHub does for a very large or binary one, or the pull request's diff is larger than the 2,000,000 characters Triage reads, those files are listed in the evidence as not checked, with the reason, and every other file is checked; if no other file adds code, the rule is unconfirmed. When a file's content can't be read or a pattern is not valid ast-grep, the rule is unconfirmed rather than passing. Triage reads at most 300 files per pull request, so on one that adds code to more, the rule is unconfirmed with a reason such as **code patterns could not be checked: 412 files with added code, more than the 300 read per pull request**. On Bitbucket Data Center, which does not give Triage each file's changed lines, it is unconfirmed whenever a TypeScript or JavaScript file of your own is added, edited, moved or copied. To give some patterns a different `onFail`, put them in a rule of their own. Code-pattern rules count toward the eight rules of your own, and the browser extension leaves them out. - **Judgement**, questions the decision model answers from the diff and the documentation: - **Check the change is focused on one thing.** The decision model is asked whether the pull request does one thing. When it fails, the verdict comment says it mixes unrelated changes and asks the author to split them out, unless the file sets another `onFail`. A pull request based on another open pull request's branch is checked on its own changes, and the comment says so, such as "Stacked on #1573: only this pull request's own changes were checked." On a branch another pull request was merged into, this rule is skipped, and the comment says why, such as "stacked branch, includes #1521". - **Check docs are updated when behavior changes.** Changing a documentation file passes it. Otherwise the decision model is asked, file by file, whether the docs stay correct, a statement in them is now wrong, or the change adds setup the docs must describe. When it fails, the verdict comment asks the author to update the documentation and names the files, unless the file sets another `onFail`. The default label, when `onFail` is `label`, is `docs: check`. diff --git a/docs/DECISION_MODEL.md b/docs/DECISION_MODEL.md index a492345b6..f2c94b447 100644 --- a/docs/DECISION_MODEL.md +++ b/docs/DECISION_MODEL.md @@ -100,12 +100,13 @@ Two different numbers come out of the answers: | Contribution rules | 8,000 bytes, default branch | `REPO_RULES_MAX_BYTES` | | Patch the judge reads per file | 65,536 chars | `MAX_PATCH_INPUT_CHARS` | | Patch the judge reads per pull request | 270,000 chars | `MAX_PATCH_CHARS` | +| Patch the rules that run in code read per pull request | 2,000,000 chars | `MAX_CODE_RULES_PATCH_CHARS` | | GitHub tool page | 9,000 chars | `PR_FILES_PAGE_CHARS` | - **Docs.** Docs are read from the default branch in this order: root `README`, then `docs/`, then other root markdown, then nested markdown, each in path order. Reading stops at 24,000 bytes; a doc that crosses the cap is cut and later docs are not seen. The same docs are in every request's state. - **Contribution rules.** Read under their own 8,000-byte budget, in this order: `AGENTS.md`, `CLAUDE.md`, `CONTRIBUTING` and `PULL_REQUEST_TEMPLATE` at the root, then `CONTRIBUTING` and `PULL_REQUEST_TEMPLATE` in `.github/` (including `PULL_REQUEST_TEMPLATE/`), then the same two in `docs/`. An `AGENTS.md` or `CLAUDE.md` inside a package stays an ordinary doc. Only requests that ask a custom rule carry them and give up room for them; requests that ask only built-in rules are unchanged. - **Request splitting.** Files fill each request's state in order up to the budget left after metadata and docs. A file too large for one request keeps its leading hunks there for every rule, and focused and docs read its remaining hunks in the next requests, docs asking about each part (any part needing docs fails it); a hunk too large for any request is cut at line ends across several. Each request asks only about the files it holds; whole-PR questions go in the first, and focused asks in each. If the title, body, commits, path list and docs leave less than 8 KB of the budget for files, nothing is asked and the model rules stay unconfirmed ("pull request too large for the decision model (N KB)", the size of that metadata). The same holds when the split needs more than 64 requests ("… (N requests)"). -- **Oversized patches.** A patch larger than one tool page is read across pages and joined, and every patch is read whole for the rules that run in code. The judge reads 65,536 chars of a file, which is then marked cut. Once the judge's 270,000-char budget is spent, later files are sent to it without patches. +- **Oversized patches.** A patch larger than one tool page is read across pages and joined, and every patch is read whole for the rules that run in code until 2,000,000 chars; the file that passes it partway and every later one are listed without patches. The judge reads 65,536 chars of a file, which is then marked cut. Once the judge's 270,000-char budget is spent, later files are sent to it without patches. - **Unread files.** A file with lines changed but no patch to read (GitHub left it out, the judge's budget was spent, or not even its first hunk fits a request) is unread, and a rule that would ask about it stays unconfirmed. Treatment differs by rule: - Docs reads a file too large for one request across requests as focused does. A file whose patch was cut while reading is asked about from the hunks that were read and is unread, so it can fail docs but never pass it. A changed doc file passes docs by rule regardless. - Scoped custom rules ask about a cut file (its patch cut while reading, or too large for one request) from the hunks that were read and treat it as unread. diff --git a/packages/triage-workflows/src/actions/facts.ts b/packages/triage-workflows/src/actions/facts.ts index 9be44ff18..d1942a0ff 100644 --- a/packages/triage-workflows/src/actions/facts.ts +++ b/packages/triage-workflows/src/actions/facts.ts @@ -4,9 +4,9 @@ import { type } from "arktype"; import { ACTION_KINDS, catalogCheckEnabled, classificationSources, customCheckEnabled, DEFAULT_RISK, emptyPack, isPatternsCheck, PolicySourceShape, repoPolicy, repoSourceOf, triageEventsOf, trustedRequestOf, visibleText, type ActionKind, type CheckPack, type PolicySource, type RepoSource, type TriageEvent } from "@corbits/triage-contracts"; import { repoDocs, type ClosedPr, type IssueComment, type RepoDocsPage, type Review, type RuleSpan } from "@corbits/github-reads"; import { isRecord } from "@corbits/common"; -import { DEPLOYMENT_UNREADABLE, hasModelChecks, NEEDS_SETUP_REASON, packFromInput, type PrFacts, type PrFileFacts } from "../logic/checks.js"; +import { DEPLOYMENT_UNREADABLE, hasModelChecks, NEEDS_SETUP_REASON, packFromInput, type PrFacts } from "../logic/checks.js"; import { asText } from "../logic/extract.js"; -import { addFilesPage, assembleItem, fileFacts, FIRST_FILES_PAGE, optionalCount, namedTeams, type CheckRun, type FilesCursor, type FilesPage, type PrData, type PullData, type RepoDocs } from "../logic/facts.js"; +import { addFilesPage, assembleItem, fileFacts, FIRST_FILES_PAGE, optionalCount, namedTeams, type CheckRun, type FilesCursor, type FilesPage, type ListedFiles, type PrData, type PullData, type RepoDocs } from "../logic/facts.js"; import { couldRepeat, MAX_OPEN_PULLS, overlapping, type OpenPull, type OpenPulls, type PullPrint } from "../logic/duplicates.js"; import type { History } from "../logic/history.js"; import { filesToRead, patternsResult, type PatternsResult } from "../logic/patterns.js"; @@ -239,7 +239,7 @@ async function fetchTargets({ host, stamp }: Host, repo: string, numbers: number const teams = namedTeams(pack, repo); const focused = catalogCheckEnabled(pack, "focused"); const risk = catalogCheckEnabled(pack, "regression-risk") || catalogCheckEnabled(pack, "risk"); - const listed = new Map(); + const listed = new Map(); const filesErrors = new Map(); function filesId(n: number, { offset, patchOffset }: FilesCursor): string { @@ -247,8 +247,8 @@ async function fetchTargets({ host, stamp }: Host, repo: string, numbers: number } function filesCall([n, cursor]: [number, FilesCursor]): ToolCall { - const { offset, patchOffset } = cursor; - return call(tool("list_pr_files"), { repo, number: n, offset, ...(patchOffset === undefined ? {} : { patchOffset }) }, filesId(n, cursor)); + const { offset, patchOffset, patches } = cursor; + return call(tool("list_pr_files"), { repo, number: n, offset, ...(patchOffset === undefined ? {} : { patchOffset }), ...(patches ? {} : { patches: false }) }, filesId(n, cursor)); } function prCalls(n: number): ToolCall[] { @@ -264,7 +264,7 @@ async function fetchTargets({ host, stamp }: Host, repo: string, numbers: number function addFiles(n: number, cursor: FilesCursor, result: ToolResult | undefined): FilesCursor | undefined { const page = filesPage(tool("list_pr_files"), result); - const read = listed.get(n) ?? []; + const read = listed.get(n) ?? { files: [], patchChars: 0 }; listed.set(n, read); const next = typeof page === "string" ? page : addFilesPage(read, cursor, page); if (typeof next !== "string") return next; @@ -297,7 +297,7 @@ async function fetchTargets({ host, stamp }: Host, repo: string, numbers: number } function historyPathsOf(n: number): string[] { - return historyPaths(listed.get(n)?.map((file) => file.path) ?? []); + return historyPaths(listed.get(n)?.files.map((file) => file.path) ?? []); } function historyCalls(): ToolCall[] { @@ -309,7 +309,7 @@ async function fetchTargets({ host, stamp }: Host, repo: string, numbers: number /** A pull request the host read whole, as the duplicate rules compare it. */ function ownPull(n: number): OwnPull[] { const pr = prs.get(n); - const files = listed.get(n); + const files = listed.get(n)?.files; if (!pr?.sha || !pr.base || pr.title === undefined || !files || filesErrors.has(n)) return []; return [{ number: n, title: visibleText(pr.title), headSha: pr.sha, base: pr.base, paths: files.map((file) => file.path) }]; } @@ -355,7 +355,7 @@ async function fetchTargets({ host, stamp }: Host, repo: string, numbers: number comments: data<{ comments: IssueComment[] }>(r1.get(`comments:${n}`))?.comments ?? [], commits: data<{ commits: Array<{ message?: string }> }>(r1.get(`commits:${n}`))?.commits ?? [], stack: data>(r1.get(`stack:${n}`)), - files: filesErrors.get(n) ?? listed.get(n) ?? [], + files: filesErrors.get(n) ?? listed.get(n)?.files ?? [], authorTeams: data>(r2.get(`teams:${prs.get(n)?.author}`)), fixes: fixesOf(n), }, { repo, policy, pack, events, docs, templates, source: stamp.repoSource }); diff --git a/packages/triage-workflows/src/logic/checks.test.ts b/packages/triage-workflows/src/logic/checks.test.ts index a1c365298..9825c8abd 100644 --- a/packages/triage-workflows/src/logic/checks.test.ts +++ b/packages/triage-workflows/src/logic/checks.test.ts @@ -447,7 +447,7 @@ rules: test("a file without its patch, as on Bitbucket Data Center, an unread content or a broken rule never passes", () => { const unpatched = { path: "src/a.ts", status: "modified", additions: 0, deletions: 0 }; - expect(filesToRead([unpatched])).toEqual({ unread: "the host sent no diff for src/a.ts" }); + expect(filesToRead([unpatched])).toEqual({ unread: "src/a.ts: the host sent no diff" }); expect(patterns([unpatched], {}).checks.find((check) => check.check === "conventions")).toMatchObject({ result: "unconfirmed", output: { kind: "could-not-check" } }); expect(patterns([file], {}).checks.find((check) => check.check === "conventions")).toMatchObject({ result: "unconfirmed" }); const nested = patterns([{ ...file, path: "src/build/a.ts" }], {}); @@ -460,12 +460,12 @@ rules: test("a file the host sent no diff for is named as not checked, and every other file is checked", () => { const large = { path: "src/large.ts", status: "modified", additions: 9_000, deletions: 0 }; const clean = { path: "src/b.ts", status: "added", additions: 1, deletions: 0, patch: "@@ -0,0 +1 @@\n+const b = 1;" }; - const left = "src/large.ts: not checked, the host sent no diff for it"; + const left = "src/large.ts: not checked, the host sent no diff"; expect(filesToRead([file, large, clean])).toEqual(["src/a.ts", "src/b.ts"]); const failed = patterns([file, large], { "src/a.ts": content }); expect(failed.checks.find((check) => check.check === "conventions")).toMatchObject({ result: "fail", evidence: expect.arrayContaining([left]), skipped: [left] }); const passed = patterns([clean, large], { "src/b.ts": "const b = 1;\n" }); - expect(passed.checks.find((check) => check.check === "conventions")).toMatchObject({ result: "pass", reason: "no code pattern matched; 1 file was not checked because the host sent no diff for it", evidence: [left] }); + expect(passed.checks.find((check) => check.check === "conventions")).toMatchObject({ result: "pass", reason: "no code pattern matched; 1 file was not checked", evidence: [left] }); }); test("an unknown preset is refused", () => { diff --git a/packages/triage-workflows/src/logic/checks.ts b/packages/triage-workflows/src/logic/checks.ts index 3c449dbfa..999d77ffe 100644 --- a/packages/triage-workflows/src/logic/checks.ts +++ b/packages/triage-workflows/src/logic/checks.ts @@ -55,6 +55,8 @@ export interface PrFileFacts { additions?: number; deletions?: number; patch?: string; + /** Listed past the code rules' patch ceiling, so its patch was not read. */ + patchUnread?: true; } export interface PrFacts { @@ -642,8 +644,8 @@ function riskResult(facts: PrFacts, rules: CheckRules): CheckResult { function patternsReason(failed: boolean, found: { skipped: string[]; checked: number; unpatched?: number }): string { if (failed) return "added code matches a code pattern"; - if (found.unpatched === 1) return "no code pattern matched; 1 file was not checked because the host sent no diff for it"; - if (found.unpatched) return `no code pattern matched; ${found.unpatched} files were not checked because the host sent no diff for them`; + if (found.unpatched === 1) return "no code pattern matched; 1 file was not checked"; + if (found.unpatched) return `no code pattern matched; ${found.unpatched} files were not checked`; if (found.checked === 0 && found.skipped.length > 0) return "nothing checked: the only TypeScript or JavaScript changed is vendored, built or installed code"; if (found.checked === 0) return "nothing checked: no TypeScript or JavaScript code added"; return "no code pattern matched"; diff --git a/packages/triage-workflows/src/logic/facts.test.ts b/packages/triage-workflows/src/logic/facts.test.ts index 29b96cb59..03df2ebf8 100644 --- a/packages/triage-workflows/src/logic/facts.test.ts +++ b/packages/triage-workflows/src/logic/facts.test.ts @@ -2,10 +2,10 @@ import { describe, expect, test } from "bun:test"; import { DEFAULT_REPO_POLICY, emptyPack, isPatternsCheck, readCheckPack, readPolicyFile } from "@corbits/triage-contracts"; import type { IssueComment, Review } from "@corbits/github-reads"; import type { PrFileFacts } from "./checks.ts"; -import { addFilesPage, assembleItem, buildFacts, FIRST_FILES_PAGE, type FilesCursor, type FilesPage } from "./facts.ts"; +import { addFilesPage, assembleItem, buildFacts, FIRST_FILES_PAGE, type FilesCursor, type FilesPage, type ListedFiles } from "./facts.ts"; import { NO_FACTS } from "./item.ts"; import { patternsResult } from "./patterns.ts"; -import { MAX_SYSTEM_ONE_STATE_BYTES, prepareQualityEvaluation } from "./quality.ts"; +import { MAX_CODE_RULES_PATCH_CHARS, MAX_SYSTEM_ONE_STATE_BYTES, prepareQualityEvaluation } from "./quality.ts"; test("buildFacts carries the author's trust tier", () => { const facts = buildFacts("acme/widgets", 1, { author: "alice", authorAssociation: "MEMBER", branch: "a" }, [], [], [], DEFAULT_REPO_POLICY); @@ -86,6 +86,23 @@ describe("an answered review", () => { }); }); +/** Reads `hostFiles` as the host pages them: one file a page, each patch split across two pages, none when the cursor asks for none. */ +function readPages(hostFiles: PrFileFacts[]): PrFileFacts[] { + const listed: ListedFiles = { files: [], patchChars: 0 }; + let cursor: FilesCursor | undefined | string = FIRST_FILES_PAGE; + while (cursor !== undefined) { + if (typeof cursor === "string") throw new Error(cursor); + const { patch, ...meta } = hostFiles[cursor.offset]!; + const next = cursor.offset + 1 < hostFiles.length ? { next: cursor.offset + 1 } : {}; + const half = Math.floor(patch!.length / 2); + let page: FilesPage = { files: [meta], ...next }; + if (cursor.patches && cursor.patchOffset === undefined) page = { files: [{ ...meta, patch: patch!.slice(0, half) }], next: cursor.offset, patchNext: half }; + else if (cursor.patches) page = { files: [{ ...meta, patch: patch!.slice(cursor.patchOffset) }], ...next }; + cursor = addFilesPage(listed, cursor, page); + } + return listed.files; +} + test("a diff past the judge's budget is read whole for the pattern rules while the judge reads a bounded part", () => { const filler = Array.from({ length: 200 }, (_, line) => `const filler${line} = "${"x".repeat(30)}";`); const fallback = "const late = value ?? 0;"; @@ -93,18 +110,7 @@ test("a diff past the judge's budget is read whole for the pattern rules while t const lines = (index: number) => (index === paths.length - 1 ? [...filler, fallback] : filler); const patch = (index: number) => [`@@ -0,0 +1,${lines(index).length} @@`, ...lines(index).map((line) => `+${line}`)].join("\n"); const hostFiles: PrFileFacts[] = paths.map((path, index) => ({ path, status: "added", additions: lines(index).length, deletions: 0, patch: patch(index) })); - const pages: FilesPage[] = hostFiles.flatMap((file, index): FilesPage[] => { - const half = Math.floor(file.patch!.length / 2); - const next = index + 1 < hostFiles.length ? { next: index + 1 } : {}; - return [{ files: [{ ...file, patch: file.patch!.slice(0, half) }], next: index, patchNext: half }, { files: [{ ...file, patch: file.patch!.slice(half) }], ...next }]; - }); - const listed: PrFileFacts[] = []; - let cursor: FilesCursor | undefined | string = FIRST_FILES_PAGE; - for (const page of pages) { - if (cursor === undefined || typeof cursor === "string") throw new Error(`stopped early: ${cursor}`); - cursor = addFilesPage(listed, cursor, page); - } - expect(cursor).toBeUndefined(); + const listed = readPages(hostFiles); expect(listed).toEqual(hostFiles); expect(hostFiles.reduce((chars, file) => chars + file.patch!.length, 0)).toBeGreaterThan(3 * MAX_SYSTEM_ONE_STATE_BYTES); @@ -118,3 +124,37 @@ test("a diff past the judge's budget is read whole for the pattern rules while t expect(judged).toBeLessThanOrEqual(3 * MAX_SYSTEM_ONE_STATE_BYTES + 65_536); expect(plan!.unread).toContain(paths.at(-1)!); }); + +describe("a diff past the code rules' ceiling", () => { + const pack = readCheckPack(readPolicyFile("rules:\n - id: conventions\n check: code-patterns\n text: Conventions.\n reason: Review time.\n evidence: [maintainer-stated]\n presets: [convention]\n", "acme/widgets")); + + function ceilingCase(filler: number, whole: number) { + const line = `const filler = "${"x".repeat(filler)}";`; + const patch = ["@@ -0,0 +1,200 @@", ...Array.from({ length: 200 }, () => `+${line}`)].join("\n"); + const hostFiles: PrFileFacts[] = Array.from({ length: 12 }, (_, index) => ({ path: `src/big${String(index).padStart(2, "0")}.ts`, status: "added", additions: 200, deletions: 0, patch })); + const listed = readPages(hostFiles); + expect(listed.slice(0, whole)).toEqual(hostFiles.slice(0, whole)); + const unread = listed.slice(whole); + expect(unread).toEqual(hostFiles.slice(whole).map(({ patch: _patch, ...meta }) => ({ ...meta, patchUnread: true }))); + const contents = Object.fromEntries(listed.slice(0, whole).map((file) => [file.path, `${Array.from({ length: 200 }, () => line).join("\n")}\n`])); + expect(patternsResult(pack.custom.find(isPatternsCheck)!, listed, contents)).toEqual({ + matches: [], + skipped: unread.map((file) => `${file.path}: not checked, the pull request's diff is larger than Triage reads`), + checked: whole, + unpatched: hostFiles.length - whole, + }); + return patch.length; + } + + test("keeps the file whose last page passes it, and lists the later files without patches for the pattern rules to name", () => { + const chars = ceilingCase(1_000, 10); + expect(chars * 9.5).toBeLessThan(MAX_CODE_RULES_PATCH_CHARS); + expect(chars * 10).toBeGreaterThan(MAX_CODE_RULES_PATCH_CHARS); + }); + + test("drops the patch of a file it passes partway through", () => { + const chars = ceilingCase(1_040, 9); + expect(chars * 9).toBeLessThan(MAX_CODE_RULES_PATCH_CHARS); + expect(chars * 9.5).toBeGreaterThan(MAX_CODE_RULES_PATCH_CHARS); + }); +}); diff --git a/packages/triage-workflows/src/logic/facts.ts b/packages/triage-workflows/src/logic/facts.ts index ce8a9b3b1..705e1b162 100644 --- a/packages/triage-workflows/src/logic/facts.ts +++ b/packages/triage-workflows/src/logic/facts.ts @@ -2,6 +2,7 @@ import { checkCancelled, checkFailed, checkPending, latestDecisive, listPrFilesP import { authorAssociation, authorKindOf, catalogCheckEnabled, policyTeams, shownBody, teamSlug, tierOf, visibleText, type CheckPack, type RepoPolicy, type RepoSource, type TriageEvent } from "@corbits/triage-contracts"; import { hiddenIn, hiddenMarkup, nothingRemoved, writtenDescription } from "./hidden-content.js"; import type { PrFacts, PrFileFacts } from "./checks.js"; +import { MAX_CODE_RULES_PATCH_CHARS } from "./quality.js"; import { isRecord } from "@corbits/common"; export type RepoDocs = Pick; @@ -95,35 +96,47 @@ export function buildFacts( export type FilesPage = { files: PrFileFacts[]; next?: number; patchNext?: number }; -export type FilesCursor = { offset: number; patchOffset?: number }; +export type FilesCursor = { offset: number; patchOffset?: number; patches: boolean }; -export const FIRST_FILES_PAGE: FilesCursor = { offset: 0 }; +export type ListedFiles = { files: PrFileFacts[]; patchChars: number }; + +export const FIRST_FILES_PAGE: FilesCursor = { offset: 0, patches: true }; + +function unreadPatch({ patch: _patch, ...file }: PrFileFacts): PrFileFacts { + return { ...file, patchUnread: true }; +} /** * Adds one page to `listed`, joining a patch continued from the previous page, and returns the next page's cursor, - * none when every file is read, or why the pages cannot be read. Every patch the host sends is read whole. + * none when every file is read, or why the pages cannot be read. Every patch is read whole until the code rules' ceiling; + * the file it falls in and every later one are listed without their patch and marked so. */ -export function addFilesPage(listed: PrFileFacts[], cursor: FilesCursor, page: FilesPage): FilesCursor | undefined | string { - let files = page.files; +export function addFilesPage(listed: ListedFiles, cursor: FilesCursor, page: FilesPage): FilesCursor | undefined | string { + let files = cursor.patches ? page.files : page.files.map(unreadPatch); if (cursor.patchOffset !== undefined) { - const continued = listed.at(-1); + const continued = listed.files.at(-1); if (!continued || files[0]?.path !== continued.path) return `github_list_pr_files did not continue the patch at offset ${cursor.offset}`; continued.patch = (continued.patch ?? "") + (files[0].patch ?? ""); + listed.patchChars += files[0].patch?.length ?? 0; files = files.slice(1); } - listed.push(...files); + listed.files.push(...files); + listed.patchChars += files.reduce((chars, file) => chars + (file.patch?.length ?? 0), 0); + const patches = cursor.patches && listed.patchChars < MAX_CODE_RULES_PATCH_CHARS; if (page.next === undefined) return undefined; if (page.patchNext !== undefined) { if (page.next < cursor.offset || (page.next === cursor.offset && page.patchNext <= (cursor.patchOffset ?? 0))) return `github_list_pr_files did not advance past offset ${cursor.offset}`; - return { offset: page.next, patchOffset: page.patchNext }; + if (patches) return { offset: page.next, patchOffset: page.patchNext, patches }; + listed.files.push(unreadPatch(listed.files.pop()!)); + return { offset: page.next + 1, patches }; } if (page.next <= cursor.offset) return `github_list_pr_files did not advance past offset ${cursor.offset}`; - return { offset: page.next }; + return { offset: page.next, patches }; } /** Every changed file of a pull request, read page by page as the facts director reads them. */ export async function listFiles(gh: GithubFetch, repo: string, number: number): Promise { - const listed: PrFileFacts[] = []; + const listed: ListedFiles = { files: [], patchChars: 0 }; let cursor: FilesCursor | undefined = FIRST_FILES_PAGE; while (cursor !== undefined) { const page = await listPrFilesPage(gh, repo, number, cursor); @@ -131,7 +144,7 @@ export async function listFiles(gh: GithubFetch, repo: string, number: number): if (typeof next === "string") throw new Error(`${next} for #${number}`); cursor = next; } - return listed; + return listed.files; } function optionalString(value: unknown): string | undefined { @@ -158,6 +171,7 @@ export function fileFacts(value: unknown): PrFileFacts[] { ...(additions === undefined ? {} : { additions }), ...(deletions === undefined ? {} : { deletions }), ...(patch === undefined ? {} : { patch }), + ...(value.patchUnread === true ? { patchUnread: true as const } : {}), }]; } diff --git a/packages/triage-workflows/src/logic/patterns.ts b/packages/triage-workflows/src/logic/patterns.ts index a1c5ddfc0..69ffbd93b 100644 --- a/packages/triage-workflows/src/logic/patterns.ts +++ b/packages/triage-workflows/src/logic/patterns.ts @@ -18,7 +18,7 @@ const GRAMMARS: Record = { jsx: JAVASCRIPT, }; -/** `unpatched` counts the files in scope left out because they came without a patch. */ +/** `unpatched` counts the files in scope left out because they have no patch. */ export type PatternsResult = { matches: string[]; skipped: string[]; checked: number; unpatched?: number } | { unread: string }; function presetRules(id: string): readonly PatternRule[] { @@ -47,14 +47,21 @@ function inScope(file: PrFileFacts): boolean { return checkable(file) && !isForeignPath(file.path); } -/** A file in scope that came without a patch: GitHub sends none for a very large or binary file, Bitbucket Data Center for any. */ +/** + * A file in scope without a patch: GitHub sends none for a very large or binary file, Bitbucket Data Center for any, + * and none is read past the code rules' ceiling. + */ function unpatched(file: PrFileFacts): boolean { return inScope(file) && file.patch === undefined; } +function whyUnpatched(file: PrFileFacts): string { + return file.patchUnread ? "the pull request's diff is larger than Triage reads" : "the host sent no diff"; +} + function namedUnpatched(files: readonly PrFileFacts[]): string { const more = files.length > 1 ? ` and ${files.length - 1} more` : ""; - return `the host sent no diff for ${files[0]!.path}${more}`; + return `${files[0]!.path}${more}: ${whyUnpatched(files[0]!)}`; } function readPath(file: PrFileFacts): string { @@ -95,7 +102,7 @@ function fileMatches(file: PrFileFacts, patch: string, content: string, grammar: /** * Matches on the lines a pull request adds; a file in scope whose content was not read leaves the rule unchecked, and - * one that came without a patch is named as not checked. + * one without a patch is named as not checked. */ export function patternsResult(check: PatternsCheck, files: readonly PrFileFacts[], contents: Readonly>): PatternsResult { const toRead = filesToRead(files); @@ -113,7 +120,7 @@ export function patternsResult(check: PatternsCheck, files: readonly PrFileFacts } const missing = files.filter(unpatched); const skipped = [ - ...missing.map((file) => `${file.path}: not checked, the host sent no diff for it`), + ...missing.map((file) => `${file.path}: not checked, ${whyUnpatched(file)}`), ...files.filter((file) => checkable(file) && isForeignPath(file.path)).map((file) => `${file.path}: skipped as vendored, built or installed code`), ]; return { matches, skipped, checked: files.filter(inScope).length - missing.length, ...(missing.length > 0 && { unpatched: missing.length }) }; diff --git a/packages/triage-workflows/src/logic/quality.ts b/packages/triage-workflows/src/logic/quality.ts index 1264c4d7f..a294fb458 100644 --- a/packages/triage-workflows/src/logic/quality.ts +++ b/packages/triage-workflows/src/logic/quality.ts @@ -184,6 +184,9 @@ export const QUALITY_EVALUATION_LIMIT_ERROR = "pull request too large for the de // The judge reads patches up to three requests' worth; later files are sent without theirs. const MAX_PATCH_CHARS = 3 * MAX_SYSTEM_ONE_STATE_BYTES; +// The rules that run in code read this much patch text, apart from the judge's budget; one sidecar holds every +// deployment's step outputs, so a diff past it is listed without its later patches. +export const MAX_CODE_RULES_PATCH_CHARS = 2_000_000; /** A changed file as the judge reads it: `patchTruncated` when its patch was cut at `MAX_PATCH_INPUT_CHARS`. */ type JudgedFile = PrFileFacts & { patchTruncated?: true }; From 9862114c72a3f8c32ce9e9852a66c2193dceb726 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Tue, 27 Oct 2026 17:08:10 -0700 Subject: [PATCH 3/9] fix(triage-workflows): share the code rules' patch ceiling across the facts step Lockfiles no longer count toward it, only a step that reached it marks files unread, unread files are grouped by reason, and a reason several rules share is tested to render once. --- docs/ARCHITECTURE.md | 27 +++++------ .../Corbits Triage User Manual.md | 2 +- docs/DECISION_MODEL.md | 4 +- .../triage-workflows/src/actions/facts.ts | 19 ++++---- .../triage-workflows/src/logic/checks.test.ts | 2 + .../triage-workflows/src/logic/facts.test.ts | 26 ++++++++--- packages/triage-workflows/src/logic/facts.ts | 46 +++++++++++-------- .../triage-workflows/src/logic/patterns.ts | 8 +++- .../triage-workflows/src/logic/quality.ts | 5 +- .../triage-workflows/src/logic/render.test.ts | 9 ++++ 10 files changed, 94 insertions(+), 54 deletions(-) diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 940404083..32ca0a29f 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -548,17 +548,18 @@ label off that head, so it never stands on a head not yet triaged. JavaScript file's head content its `files` and `ignores` globs admit (ast-grep's glob rules, checked when the policy is read), and keeps matches on lines the patch adds. - The facts step reads every changed file's patch whole, across as many - tool pages as it takes, up to 2,000,000 characters per pull request - (`MAX_CODE_RULES_PATCH_CHARS`), since one sidecar holds every - deployment's step outputs; the file that passes it partway and every - later one are listed without patches. The judge's view of the diff has - its own, smaller budget (`judgedFiles` in `logic/quality.ts`). A file - without a patch, because the host sent none (GitHub omits it for a very - large or binary file) or the diff passed that ceiling, is listed in the - evidence as not checked, saying which, and the rule runs on the rest; with nothing else - to check, or a file whose content was not read, or a rule ast-grep - refuses, the rule is unconfirmed; `vendor/`, `dist/`, `build/` and + The facts step reads every changed file's patch whole, across as many tool + pages as it takes, until the step has read 2,000,000 characters + (`MAX_CODE_RULES_PATCH_CHARS`), shared by every pull request a backlog + batch reads, since one sidecar holds every deployment's step outputs; + lockfiles and other generated files do not count. The file that passes it + partway and every later one are listed without patches. The judge's view of + the diff has its own, smaller budget (`judgedFiles` in `logic/quality.ts`). + A file without a patch, because the host sent none (GitHub omits it for a + very large or binary file) or the step passed that ceiling, is listed in + the evidence as not checked, saying which, and the rule runs on the rest; + with nothing else to check, or a file whose content was not read, or a rule + ast-grep refuses, the rule is unconfirmed; `vendor/`, `dist/`, `build/` and `generated/` at the repository root and `node_modules/` anywhere are skipped and listed as skipped in the evidence, and a `?? 0` inside arithmetic is a counter's real default. With such a rule on, the facts @@ -567,8 +568,8 @@ label off that head, so it never stands on a head not yet triaged. leaves the rule unconfirmed without reading any), runs the rules there and outputs only the matches. Bitbucket Data Center lists no patch, so there the rule is unconfirmed whenever an in-scope TypeScript or JavaScript file - is added, edited, moved or copied. The extension leaves code-pattern rules out and swaps the native module - for a stub. + is added, edited, moved or copied. The extension leaves code-pattern + rules out and swaps the native module for a stub. 3. **route**: skips the judge when no question needs asking. 4. **judge**: one System One call per request of questions. Each first-round request and the verdict request also asks whether the pull request's text, diff or docs try to direct the judge; a diff --git a/docs/Corbits Triage User Manual/Corbits Triage User Manual.md b/docs/Corbits Triage User Manual/Corbits Triage User Manual.md index 5148aaac5..929955bb0 100644 --- a/docs/Corbits Triage User Manual/Corbits Triage User Manual.md +++ b/docs/Corbits Triage User Manual/Corbits Triage User Manual.md @@ -258,7 +258,7 @@ A rule asks one question about a pull request and answers pass, fail or unconfir - **Flag changes to sensitive paths.** Checks changed files against the file's named groups of globs, each `high` or `medium`, written as `groups:`, such as `infra: { level: high, paths: ["infra/**", "**/*.tf"] }`. Without `groups` there are four: `schema` (`**/migrations/**`, `**/*.sql`) and `auth` (`**/auth/**`, `**/secrets/**`), which are high, and `ci` (`.github/**`, `**/Dockerfile`, `**/railway.json`, `**/railway.toml`) and `locks` (`**/bun.lock`, `**/package-lock.json`, `**/yarn.lock`, `**/pnpm-lock.yaml`), which are medium, since every dependency bump changes a lockfile. Globs read as in **Never accept changes to**. A change to a group at `failAt` or above, high unless the file says, fails the rule, which asks a maintainer unless the file gives it another `onFail`. Below that, from `labelAt`, medium unless the file says, it passes but labels the pull request `sensitive: medium` and notes it under **Notes:** (on Bitbucket Data Center, only the note). Its evidence names each group with its files, at most five, such as `schema: db/migrations/0004.sql`, then how many more. Regression risk keeps its own migration globs. - **Flag high-risk pull requests and label medium-risk ones.** Rates each pull request low, medium or high risk; see [Risk](#risk). - **Fail when …**, a text rule of your own, such as "Fail when any commit message contains "Co-Authored-By:"". It names where to look (the title, the description, any commit message, or several of them), whether the rule fails when the text **contains** or **does not contain** its phrases, and the phrases. Case does not matter. A **contains** rule on the description also looks at its hidden parts, such as HTML comments and the addresses behind links, while **does not contain** reads only what a reader sees. Only the first 100 commits are read. When a **contains** rule fails, Triage names each place and the phrases found there, asks for them to be removed, and quotes each matching line once. Text rules count toward the eight rules of your own. - - **Fail when added code matches …**, a code-pattern rule of your own, such as one that flags `?? 0`. It checks only the lines the pull request adds to TypeScript and JavaScript files (`.ts`, `.tsx`, `.mts`, `.cts`, `.js`, `.jsx`, `.mjs`, `.cjs`), so code already on the branch never fails it. On any other language it passes with **nothing checked: no TypeScript or JavaScript code added**, so don't add one to a repository written in Go, Python or similar. Turn on the shipped `convention` preset, which flags invented fallbacks (`?? 0`, `?? []`), nested ternaries, functions called where they are written, `.then`/`.catch` chains, empty `catch` blocks and lint or type suppression comments in both languages, or write your own patterns in [ast-grep](https://ast-grep.github.io/guide/rule-config.html)'s rule format. Your own pattern replaces a preset's pattern with the same `id` in both languages; to keep checking JavaScript, add a `language: javascript` pattern under the same id. A pattern takes ast-grep's own `files` and `ignores` lists to check only some paths or skip some. They follow ast-grep's glob rules: paths are from the repository root, `*` also matches across folders, and dotfiles are matched like any other file. Write each glob as a string; the `{ glob, caseInsensitive }` form is not read. Write a negated class as `[^a]`; a glob with `[!a]`, or one that isn't valid, refuses the policy. The preset's `?? 0` and `?? []` patterns ignore test files (`**/*.test.ts`, `**/*.test.tsx`, `**/*.test.js`, `**/*.test.jsx`, `**/*.test.mjs`, `**/*.test.cjs`). Each match is listed as evidence, as `path:line` and the pattern's message. Vendored and built code at the repository root (`vendor/`, `dist/`, `build/`, `generated/`) and installed code (`node_modules/` anywhere) is skipped, and each skipped file is listed in the evidence. The same folders deeper in the repository, such as `src/build/`, are checked. When the host sends no changed lines for a file, as GitHub does for a very large or binary one, or the pull request's diff is larger than the 2,000,000 characters Triage reads, those files are listed in the evidence as not checked, with the reason, and every other file is checked; if no other file adds code, the rule is unconfirmed. When a file's content can't be read or a pattern is not valid ast-grep, the rule is unconfirmed rather than passing. Triage reads at most 300 files per pull request, so on one that adds code to more, the rule is unconfirmed with a reason such as **code patterns could not be checked: 412 files with added code, more than the 300 read per pull request**. On Bitbucket Data Center, which does not give Triage each file's changed lines, it is unconfirmed whenever a TypeScript or JavaScript file of your own is added, edited, moved or copied. To give some patterns a different `onFail`, put them in a rule of their own. Code-pattern rules count toward the eight rules of your own, and the browser extension leaves them out. + - **Fail when added code matches …**, a code-pattern rule of your own, such as one that flags `?? 0`. It checks only the lines the pull request adds to TypeScript and JavaScript files (`.ts`, `.tsx`, `.mts`, `.cts`, `.js`, `.jsx`, `.mjs`, `.cjs`), so code already on the branch never fails it. On any other language it passes with **nothing checked: no TypeScript or JavaScript code added**, so don't add one to a repository written in Go, Python or similar. Turn on the shipped `convention` preset, which flags invented fallbacks (`?? 0`, `?? []`), nested ternaries, functions called where they are written, `.then`/`.catch` chains, empty `catch` blocks and lint or type suppression comments in both languages, or write your own patterns in [ast-grep](https://ast-grep.github.io/guide/rule-config.html)'s rule format. Your own pattern replaces a preset's pattern with the same `id` in both languages; to keep checking JavaScript, add a `language: javascript` pattern under the same id. A pattern takes ast-grep's own `files` and `ignores` lists to check only some paths or skip some. They follow ast-grep's glob rules: paths are from the repository root, `*` also matches across folders, and dotfiles are matched like any other file. Write each glob as a string; the `{ glob, caseInsensitive }` form is not read. Write a negated class as `[^a]`; a glob with `[!a]`, or one that isn't valid, refuses the policy. The preset's `?? 0` and `?? []` patterns ignore test files (`**/*.test.ts`, `**/*.test.tsx`, `**/*.test.js`, `**/*.test.jsx`, `**/*.test.mjs`, `**/*.test.cjs`). Each match is listed as evidence, as `path:line` and the pattern's message. Vendored and built code at the repository root (`vendor/`, `dist/`, `build/`, `generated/`) and installed code (`node_modules/` anywhere) is skipped, and each skipped file is listed in the evidence. The same folders deeper in the repository, such as `src/build/`, are checked. When the host sends no changed lines for a file, as GitHub does for a very large or binary one, or the diff is larger than the 2,000,000 characters Triage reads in one run (several pull requests checked together share them), those files are listed in the evidence as not checked, with the reason, and every other file is checked; if no other file adds code, the rule is unconfirmed. When a file's content can't be read or a pattern is not valid ast-grep, the rule is unconfirmed rather than passing. Triage reads at most 300 files per pull request, so on one that adds code to more, the rule is unconfirmed with a reason such as **code patterns could not be checked: 412 files with added code, more than the 300 read per pull request**. On Bitbucket Data Center, which does not give Triage each file's changed lines, it is unconfirmed whenever a TypeScript or JavaScript file of your own is added, edited, moved or copied. To give some patterns a different `onFail`, put them in a rule of their own. Code-pattern rules count toward the eight rules of your own, and the browser extension leaves them out. - **Judgement**, questions the decision model answers from the diff and the documentation: - **Check the change is focused on one thing.** The decision model is asked whether the pull request does one thing. When it fails, the verdict comment says it mixes unrelated changes and asks the author to split them out, unless the file sets another `onFail`. A pull request based on another open pull request's branch is checked on its own changes, and the comment says so, such as "Stacked on #1573: only this pull request's own changes were checked." On a branch another pull request was merged into, this rule is skipped, and the comment says why, such as "stacked branch, includes #1521". - **Check docs are updated when behavior changes.** Changing a documentation file passes it. Otherwise the decision model is asked, file by file, whether the docs stay correct, a statement in them is now wrong, or the change adds setup the docs must describe. When it fails, the verdict comment asks the author to update the documentation and names the files, unless the file sets another `onFail`. The default label, when `onFail` is `label`, is `docs: check`. diff --git a/docs/DECISION_MODEL.md b/docs/DECISION_MODEL.md index f2c94b447..75230d3e1 100644 --- a/docs/DECISION_MODEL.md +++ b/docs/DECISION_MODEL.md @@ -100,13 +100,13 @@ Two different numbers come out of the answers: | Contribution rules | 8,000 bytes, default branch | `REPO_RULES_MAX_BYTES` | | Patch the judge reads per file | 65,536 chars | `MAX_PATCH_INPUT_CHARS` | | Patch the judge reads per pull request | 270,000 chars | `MAX_PATCH_CHARS` | -| Patch the rules that run in code read per pull request | 2,000,000 chars | `MAX_CODE_RULES_PATCH_CHARS` | +| Patch the rules that run in code read per facts step, shared by a backlog batch | 2,000,000 chars | `MAX_CODE_RULES_PATCH_CHARS` | | GitHub tool page | 9,000 chars | `PR_FILES_PAGE_CHARS` | - **Docs.** Docs are read from the default branch in this order: root `README`, then `docs/`, then other root markdown, then nested markdown, each in path order. Reading stops at 24,000 bytes; a doc that crosses the cap is cut and later docs are not seen. The same docs are in every request's state. - **Contribution rules.** Read under their own 8,000-byte budget, in this order: `AGENTS.md`, `CLAUDE.md`, `CONTRIBUTING` and `PULL_REQUEST_TEMPLATE` at the root, then `CONTRIBUTING` and `PULL_REQUEST_TEMPLATE` in `.github/` (including `PULL_REQUEST_TEMPLATE/`), then the same two in `docs/`. An `AGENTS.md` or `CLAUDE.md` inside a package stays an ordinary doc. Only requests that ask a custom rule carry them and give up room for them; requests that ask only built-in rules are unchanged. - **Request splitting.** Files fill each request's state in order up to the budget left after metadata and docs. A file too large for one request keeps its leading hunks there for every rule, and focused and docs read its remaining hunks in the next requests, docs asking about each part (any part needing docs fails it); a hunk too large for any request is cut at line ends across several. Each request asks only about the files it holds; whole-PR questions go in the first, and focused asks in each. If the title, body, commits, path list and docs leave less than 8 KB of the budget for files, nothing is asked and the model rules stay unconfirmed ("pull request too large for the decision model (N KB)", the size of that metadata). The same holds when the split needs more than 64 requests ("… (N requests)"). -- **Oversized patches.** A patch larger than one tool page is read across pages and joined, and every patch is read whole for the rules that run in code until 2,000,000 chars; the file that passes it partway and every later one are listed without patches. The judge reads 65,536 chars of a file, which is then marked cut. Once the judge's 270,000-char budget is spent, later files are sent to it without patches. +- **Oversized patches.** A patch larger than one tool page is read across pages and joined, and every patch is read whole for the rules that run in code until one facts step has read 2,000,000 chars, a backlog batch sharing them; lockfiles and other generated files do not count. The file that passes it partway and every later one are listed without patches. The judge reads 65,536 chars of a file, which is then marked cut. Once the judge's 270,000-char budget is spent, later files are sent to it without patches. - **Unread files.** A file with lines changed but no patch to read (GitHub left it out, the judge's budget was spent, or not even its first hunk fits a request) is unread, and a rule that would ask about it stays unconfirmed. Treatment differs by rule: - Docs reads a file too large for one request across requests as focused does. A file whose patch was cut while reading is asked about from the hunks that were read and is unread, so it can fail docs but never pass it. A changed doc file passes docs by rule regardless. - Scoped custom rules ask about a cut file (its patch cut while reading, or too large for one request) from the hunks that were read and treat it as unread. diff --git a/packages/triage-workflows/src/actions/facts.ts b/packages/triage-workflows/src/actions/facts.ts index d1942a0ff..cf82413cd 100644 --- a/packages/triage-workflows/src/actions/facts.ts +++ b/packages/triage-workflows/src/actions/facts.ts @@ -4,9 +4,9 @@ import { type } from "arktype"; import { ACTION_KINDS, catalogCheckEnabled, classificationSources, customCheckEnabled, DEFAULT_RISK, emptyPack, isPatternsCheck, PolicySourceShape, repoPolicy, repoSourceOf, triageEventsOf, trustedRequestOf, visibleText, type ActionKind, type CheckPack, type PolicySource, type RepoSource, type TriageEvent } from "@corbits/triage-contracts"; import { repoDocs, type ClosedPr, type IssueComment, type RepoDocsPage, type Review, type RuleSpan } from "@corbits/github-reads"; import { isRecord } from "@corbits/common"; -import { DEPLOYMENT_UNREADABLE, hasModelChecks, NEEDS_SETUP_REASON, packFromInput, type PrFacts } from "../logic/checks.js"; +import { DEPLOYMENT_UNREADABLE, hasModelChecks, NEEDS_SETUP_REASON, packFromInput, type PrFacts, type PrFileFacts } from "../logic/checks.js"; import { asText } from "../logic/extract.js"; -import { addFilesPage, assembleItem, fileFacts, FIRST_FILES_PAGE, optionalCount, namedTeams, type CheckRun, type FilesCursor, type FilesPage, type ListedFiles, type PrData, type PullData, type RepoDocs } from "../logic/facts.js"; +import { addFilesPage, assembleItem, fileFacts, FIRST_FILES_PAGE, optionalCount, namedTeams, type CheckRun, type FilesCursor, type FilesPage, type PatchesRead, type PrData, type PullData, type RepoDocs } from "../logic/facts.js"; import { couldRepeat, MAX_OPEN_PULLS, overlapping, type OpenPull, type OpenPulls, type PullPrint } from "../logic/duplicates.js"; import type { History } from "../logic/history.js"; import { filesToRead, patternsResult, type PatternsResult } from "../logic/patterns.js"; @@ -239,7 +239,8 @@ async function fetchTargets({ host, stamp }: Host, repo: string, numbers: number const teams = namedTeams(pack, repo); const focused = catalogCheckEnabled(pack, "focused"); const risk = catalogCheckEnabled(pack, "regression-risk") || catalogCheckEnabled(pack, "risk"); - const listed = new Map(); + const listed = new Map(); + const patchesRead: PatchesRead = { chars: 0 }; const filesErrors = new Map(); function filesId(n: number, { offset, patchOffset }: FilesCursor): string { @@ -264,9 +265,9 @@ async function fetchTargets({ host, stamp }: Host, repo: string, numbers: number function addFiles(n: number, cursor: FilesCursor, result: ToolResult | undefined): FilesCursor | undefined { const page = filesPage(tool("list_pr_files"), result); - const read = listed.get(n) ?? { files: [], patchChars: 0 }; - listed.set(n, read); - const next = typeof page === "string" ? page : addFilesPage(read, cursor, page); + const files = listed.get(n) ?? []; + listed.set(n, files); + const next = typeof page === "string" ? page : addFilesPage(files, patchesRead, cursor, page); if (typeof next !== "string") return next; filesErrors.set(n, `${next} for #${n}`); return undefined; @@ -297,7 +298,7 @@ async function fetchTargets({ host, stamp }: Host, repo: string, numbers: number } function historyPathsOf(n: number): string[] { - return historyPaths(listed.get(n)?.files.map((file) => file.path) ?? []); + return historyPaths(listed.get(n)?.map((file) => file.path) ?? []); } function historyCalls(): ToolCall[] { @@ -309,7 +310,7 @@ async function fetchTargets({ host, stamp }: Host, repo: string, numbers: number /** A pull request the host read whole, as the duplicate rules compare it. */ function ownPull(n: number): OwnPull[] { const pr = prs.get(n); - const files = listed.get(n)?.files; + const files = listed.get(n); if (!pr?.sha || !pr.base || pr.title === undefined || !files || filesErrors.has(n)) return []; return [{ number: n, title: visibleText(pr.title), headSha: pr.sha, base: pr.base, paths: files.map((file) => file.path) }]; } @@ -355,7 +356,7 @@ async function fetchTargets({ host, stamp }: Host, repo: string, numbers: number comments: data<{ comments: IssueComment[] }>(r1.get(`comments:${n}`))?.comments ?? [], commits: data<{ commits: Array<{ message?: string }> }>(r1.get(`commits:${n}`))?.commits ?? [], stack: data>(r1.get(`stack:${n}`)), - files: filesErrors.get(n) ?? listed.get(n)?.files ?? [], + files: filesErrors.get(n) ?? listed.get(n) ?? [], authorTeams: data>(r2.get(`teams:${prs.get(n)?.author}`)), fixes: fixesOf(n), }, { repo, policy, pack, events, docs, templates, source: stamp.repoSource }); diff --git a/packages/triage-workflows/src/logic/checks.test.ts b/packages/triage-workflows/src/logic/checks.test.ts index 9825c8abd..79ed05118 100644 --- a/packages/triage-workflows/src/logic/checks.test.ts +++ b/packages/triage-workflows/src/logic/checks.test.ts @@ -448,6 +448,8 @@ rules: test("a file without its patch, as on Bitbucket Data Center, an unread content or a broken rule never passes", () => { const unpatched = { path: "src/a.ts", status: "modified", additions: 0, deletions: 0 }; expect(filesToRead([unpatched])).toEqual({ unread: "src/a.ts: the host sent no diff" }); + const past = (path: string) => ({ ...unpatched, path, patchUnread: true as const }); + expect(filesToRead([unpatched, past("src/b.ts"), past("src/c.ts")])).toEqual({ unread: "src/a.ts: the host sent no diff; src/b.ts and 1 more: the pull request's diff is larger than Triage reads" }); expect(patterns([unpatched], {}).checks.find((check) => check.check === "conventions")).toMatchObject({ result: "unconfirmed", output: { kind: "could-not-check" } }); expect(patterns([file], {}).checks.find((check) => check.check === "conventions")).toMatchObject({ result: "unconfirmed" }); const nested = patterns([{ ...file, path: "src/build/a.ts" }], {}); diff --git a/packages/triage-workflows/src/logic/facts.test.ts b/packages/triage-workflows/src/logic/facts.test.ts index 03df2ebf8..8e3cb9482 100644 --- a/packages/triage-workflows/src/logic/facts.test.ts +++ b/packages/triage-workflows/src/logic/facts.test.ts @@ -2,7 +2,7 @@ import { describe, expect, test } from "bun:test"; import { DEFAULT_REPO_POLICY, emptyPack, isPatternsCheck, readCheckPack, readPolicyFile } from "@corbits/triage-contracts"; import type { IssueComment, Review } from "@corbits/github-reads"; import type { PrFileFacts } from "./checks.ts"; -import { addFilesPage, assembleItem, buildFacts, FIRST_FILES_PAGE, type FilesCursor, type FilesPage, type ListedFiles } from "./facts.ts"; +import { addFilesPage, assembleItem, buildFacts, FIRST_FILES_PAGE, type FilesCursor, type FilesPage, type PatchesRead } from "./facts.ts"; import { NO_FACTS } from "./item.ts"; import { patternsResult } from "./patterns.ts"; import { MAX_CODE_RULES_PATCH_CHARS, MAX_SYSTEM_ONE_STATE_BYTES, prepareQualityEvaluation } from "./quality.ts"; @@ -87,9 +87,9 @@ describe("an answered review", () => { }); /** Reads `hostFiles` as the host pages them: one file a page, each patch split across two pages, none when the cursor asks for none. */ -function readPages(hostFiles: PrFileFacts[]): PrFileFacts[] { - const listed: ListedFiles = { files: [], patchChars: 0 }; - let cursor: FilesCursor | undefined | string = FIRST_FILES_PAGE; +function readPages(hostFiles: PrFileFacts[], first: FilesCursor = FIRST_FILES_PAGE, read: PatchesRead = { chars: 0 }): PrFileFacts[] { + const listed: PrFileFacts[] = []; + let cursor: FilesCursor | undefined | string = first; while (cursor !== undefined) { if (typeof cursor === "string") throw new Error(cursor); const { patch, ...meta } = hostFiles[cursor.offset]!; @@ -98,9 +98,9 @@ function readPages(hostFiles: PrFileFacts[]): PrFileFacts[] { let page: FilesPage = { files: [meta], ...next }; if (cursor.patches && cursor.patchOffset === undefined) page = { files: [{ ...meta, patch: patch!.slice(0, half) }], next: cursor.offset, patchNext: half }; else if (cursor.patches) page = { files: [{ ...meta, patch: patch!.slice(cursor.patchOffset) }], ...next }; - cursor = addFilesPage(listed, cursor, page); + cursor = addFilesPage(listed, read, cursor, page); } - return listed.files; + return listed; } test("a diff past the judge's budget is read whole for the pattern rules while the judge reads a bounded part", () => { @@ -152,6 +152,20 @@ describe("a diff past the code rules' ceiling", () => { expect(chars * 10).toBeGreaterThan(MAX_CODE_RULES_PATCH_CHARS); }); + test("is shared by every pull request the step reads, so one read after it is reached gets no patches", () => { + const read: PatchesRead = { chars: MAX_CODE_RULES_PATCH_CHARS }; + const file = { path: "src/a.ts", status: "added", additions: 1, deletions: 0, patch: "@@ -0,0 +1 @@\n+const a = 1;" }; + const listed = readPages([file, { ...file, path: "src/b.ts" }], FIRST_FILES_PAGE, read); + expect(listed).toEqual(["src/a.ts", "src/b.ts"].map((path) => ({ path, status: "added", additions: 1, deletions: 0, patchUnread: true }))); + }); + + test("leaves lockfiles out of the count, and marks nothing when the step asked for no patches", () => { + const lock = { path: "bun.lock", status: "modified", additions: 1, deletions: 0, patch: `@@ -1 +1 @@\n+${"x".repeat(MAX_CODE_RULES_PATCH_CHARS)}` }; + const file = { path: "src/a.ts", status: "added", additions: 1, deletions: 0, patch: "@@ -0,0 +1 @@\n+const a = 1;" }; + expect(readPages([lock, file, file])).toEqual([lock, file, file]); + expect(readPages([lock, file], { offset: 0, patches: false })).toEqual([lock, file].map(({ patch: _patch, ...meta }) => meta)); + }); + test("drops the patch of a file it passes partway through", () => { const chars = ceilingCase(1_040, 9); expect(chars * 9).toBeLessThan(MAX_CODE_RULES_PATCH_CHARS); diff --git a/packages/triage-workflows/src/logic/facts.ts b/packages/triage-workflows/src/logic/facts.ts index 705e1b162..1c7a4a514 100644 --- a/packages/triage-workflows/src/logic/facts.ts +++ b/packages/triage-workflows/src/logic/facts.ts @@ -1,5 +1,5 @@ import { checkCancelled, checkFailed, checkPending, latestDecisive, listPrFilesPage, readPrTemplates, reviewedOnHead, summarizeChecks, type CheckRun, type GithubFetch, type IssueComment, type Review } from "@corbits/github-reads"; -import { authorAssociation, authorKindOf, catalogCheckEnabled, policyTeams, shownBody, teamSlug, tierOf, visibleText, type CheckPack, type RepoPolicy, type RepoSource, type TriageEvent } from "@corbits/triage-contracts"; +import { authorAssociation, authorKindOf, catalogCheckEnabled, isGeneratedPath, policyTeams, shownBody, teamSlug, tierOf, visibleText, type CheckPack, type RepoPolicy, type RepoSource, type TriageEvent } from "@corbits/triage-contracts"; import { hiddenIn, hiddenMarkup, nothingRemoved, writtenDescription } from "./hidden-content.js"; import type { PrFacts, PrFileFacts } from "./checks.js"; import { MAX_CODE_RULES_PATCH_CHARS } from "./quality.js"; @@ -96,9 +96,11 @@ export function buildFacts( export type FilesPage = { files: PrFileFacts[]; next?: number; patchNext?: number }; -export type FilesCursor = { offset: number; patchOffset?: number; patches: boolean }; +/** `ceiling` is set once the step's patch ceiling is reached, so the files listed without patches are marked unread. */ +export type FilesCursor = { offset: number; patchOffset?: number; patches: boolean; ceiling?: true }; -export type ListedFiles = { files: PrFileFacts[]; patchChars: number }; +/** The patch text one facts step has read, shared by every pull request it reads. */ +export type PatchesRead = { chars: number }; export const FIRST_FILES_PAGE: FilesCursor = { offset: 0, patches: true }; @@ -106,45 +108,52 @@ function unreadPatch({ patch: _patch, ...file }: PrFileFacts): PrFileFacts { return { ...file, patchUnread: true }; } +/** Lockfiles and other generated files are read by no rule, so their patches do not count toward the ceiling. */ +function counted(file: PrFileFacts): number { + return isGeneratedPath(file.path) ? 0 : file.patch?.length ?? 0; +} + /** * Adds one page to `listed`, joining a patch continued from the previous page, and returns the next page's cursor, - * none when every file is read, or why the pages cannot be read. Every patch is read whole until the code rules' ceiling; + * none when every file is read, or why the pages cannot be read. Every patch is read whole until the step's ceiling; * the file it falls in and every later one are listed without their patch and marked so. */ -export function addFilesPage(listed: ListedFiles, cursor: FilesCursor, page: FilesPage): FilesCursor | undefined | string { - let files = cursor.patches ? page.files : page.files.map(unreadPatch); +export function addFilesPage(listed: PrFileFacts[], read: PatchesRead, cursor: FilesCursor, page: FilesPage): FilesCursor | undefined | string { + let files = cursor.ceiling ? page.files.map(unreadPatch) : page.files; if (cursor.patchOffset !== undefined) { - const continued = listed.files.at(-1); + const continued = listed.at(-1); if (!continued || files[0]?.path !== continued.path) return `github_list_pr_files did not continue the patch at offset ${cursor.offset}`; continued.patch = (continued.patch ?? "") + (files[0].patch ?? ""); - listed.patchChars += files[0].patch?.length ?? 0; + read.chars += counted(files[0]); files = files.slice(1); } - listed.files.push(...files); - listed.patchChars += files.reduce((chars, file) => chars + (file.patch?.length ?? 0), 0); - const patches = cursor.patches && listed.patchChars < MAX_CODE_RULES_PATCH_CHARS; + listed.push(...files); + read.chars += files.reduce((chars, file) => chars + counted(file), 0); + const patches = cursor.patches && read.chars < MAX_CODE_RULES_PATCH_CHARS; + const next = { patches, ...((cursor.ceiling || cursor.patches !== patches) && { ceiling: true as const }) }; if (page.next === undefined) return undefined; if (page.patchNext !== undefined) { if (page.next < cursor.offset || (page.next === cursor.offset && page.patchNext <= (cursor.patchOffset ?? 0))) return `github_list_pr_files did not advance past offset ${cursor.offset}`; - if (patches) return { offset: page.next, patchOffset: page.patchNext, patches }; - listed.files.push(unreadPatch(listed.files.pop()!)); - return { offset: page.next + 1, patches }; + if (patches) return { offset: page.next, patchOffset: page.patchNext, ...next }; + listed.push(unreadPatch(listed.pop()!)); + return { offset: page.next + 1, ...next }; } if (page.next <= cursor.offset) return `github_list_pr_files did not advance past offset ${cursor.offset}`; - return { offset: page.next, patches }; + return { offset: page.next, ...next }; } /** Every changed file of a pull request, read page by page as the facts director reads them. */ export async function listFiles(gh: GithubFetch, repo: string, number: number): Promise { - const listed: ListedFiles = { files: [], patchChars: 0 }; + const listed: PrFileFacts[] = []; + const read: PatchesRead = { chars: 0 }; let cursor: FilesCursor | undefined = FIRST_FILES_PAGE; while (cursor !== undefined) { const page = await listPrFilesPage(gh, repo, number, cursor); - const next = addFilesPage(listed, cursor, { ...page, files: page.files.flatMap(fileFacts) }); + const next = addFilesPage(listed, read, cursor, { ...page, files: page.files.flatMap(fileFacts) }); if (typeof next === "string") throw new Error(`${next} for #${number}`); cursor = next; } - return listed.files; + return listed; } function optionalString(value: unknown): string | undefined { @@ -171,7 +180,6 @@ export function fileFacts(value: unknown): PrFileFacts[] { ...(additions === undefined ? {} : { additions }), ...(deletions === undefined ? {} : { deletions }), ...(patch === undefined ? {} : { patch }), - ...(value.patchUnread === true ? { patchUnread: true as const } : {}), }]; } diff --git a/packages/triage-workflows/src/logic/patterns.ts b/packages/triage-workflows/src/logic/patterns.ts index 69ffbd93b..2f80652f8 100644 --- a/packages/triage-workflows/src/logic/patterns.ts +++ b/packages/triage-workflows/src/logic/patterns.ts @@ -59,9 +59,13 @@ function whyUnpatched(file: PrFileFacts): string { return file.patchUnread ? "the pull request's diff is larger than Triage reads" : "the host sent no diff"; } +/** The files without a patch, grouped by why: the first of each group named, the rest counted. */ function namedUnpatched(files: readonly PrFileFacts[]): string { - const more = files.length > 1 ? ` and ${files.length - 1} more` : ""; - return `${files[0]!.path}${more}: ${whyUnpatched(files[0]!)}`; + const groups = Map.groupBy(files, whyUnpatched); + return [...groups].map(function group([why, grouped]) { + const more = grouped.length > 1 ? ` and ${grouped.length - 1} more` : ""; + return `${grouped[0]!.path}${more}: ${why}`; + }).join("; "); } function readPath(file: PrFileFacts): string { diff --git a/packages/triage-workflows/src/logic/quality.ts b/packages/triage-workflows/src/logic/quality.ts index a294fb458..893ba8b16 100644 --- a/packages/triage-workflows/src/logic/quality.ts +++ b/packages/triage-workflows/src/logic/quality.ts @@ -184,8 +184,9 @@ export const QUALITY_EVALUATION_LIMIT_ERROR = "pull request too large for the de // The judge reads patches up to three requests' worth; later files are sent without theirs. const MAX_PATCH_CHARS = 3 * MAX_SYSTEM_ONE_STATE_BYTES; -// The rules that run in code read this much patch text, apart from the judge's budget; one sidecar holds every -// deployment's step outputs, so a diff past it is listed without its later patches. +// The rules that run in code read this much patch text per facts step, shared by every pull request a backlog batch +// reads, apart from the judge's budget; one sidecar holds every deployment's step outputs, so past it the step lists +// files without their patches. export const MAX_CODE_RULES_PATCH_CHARS = 2_000_000; /** A changed file as the judge reads it: `patchTruncated` when its patch was cut at `MAX_PATCH_INPUT_CHARS`. */ diff --git a/packages/triage-workflows/src/logic/render.test.ts b/packages/triage-workflows/src/logic/render.test.ts index b1a31a845..b1c511e05 100644 --- a/packages/triage-workflows/src/logic/render.test.ts +++ b/packages/triage-workflows/src/logic/render.test.ts @@ -173,6 +173,15 @@ describe("focused rendering", () => { expect(verdict.feedback).toBe(`${UNSURE}${UNSURE_FOCUSED}\n\nWaiting on:\n- checks are still running: \`build\``); }); + test("a reason several rules share is said once in the comment and among the reasons", () => { + const reason = "code patterns could not be checked: src/a.ts: the host sent no diff"; + const unread = (check: string) => ({ check, kind: "machine" as const, result: "unconfirmed" as const, reason, evidence: [], output: { kind: "could-not-check" as const, reason } }); + const det = { ...FOCUSED_DET, checks: [unread("conventions"), unread("no-ternaries")] }; + const verdict = renderVerdict({ repoSource: "github", author: "octocat", repo: "acme/widgets", headSha: "abc123", det, answers: parseAnswers(`${focusedAnswer(0.5, 0.4)}${NOT_INSTRUCTED}`), focused: focusedPlan({}).focused, choice: CHOICE.pass }); + expect(verdict.reasons).toEqual([`Not confirmed: ${reason}`, "Not confirmed: makes one focused change"]); + expect(verdict.feedback).toBe(`${UNSURE}${UNSURE_FOCUSED}\n\nWaiting on:\n- ${reason}`); + }); + test.each([ ["missing", ""], ["malformed", JSON.stringify({ id: "focused", type: "choice", choice: "one_thing", probabilities: { one_thing: 1 }, confidence: 2 })], From 55d04db93f2004880d034085ee2579337b2684ee Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Tue, 27 Oct 2026 17:09:49 -0700 Subject: [PATCH 4/9] fix(triage-workflows): treat a file GitHub only renamed as nothing to read --- docs/ARCHITECTURE.md | 7 ++++--- .../triage-workflows/src/actions/facts.ts | 8 +++---- .../triage-workflows/src/logic/checks.test.ts | 15 +++++++++---- .../triage-workflows/src/logic/facts.test.ts | 4 ++-- .../triage-workflows/src/logic/patterns.ts | 21 ++++++++++++------- 5 files changed, 34 insertions(+), 21 deletions(-) diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 32ca0a29f..a9d82148f 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -556,9 +556,10 @@ label off that head, so it never stands on a head not yet triaged. partway and every later one are listed without patches. The judge's view of the diff has its own, smaller budget (`judgedFiles` in `logic/quality.ts`). A file without a patch, because the host sent none (GitHub omits it for a - very large or binary file) or the step passed that ceiling, is listed in - the evidence as not checked, saying which, and the rule runs on the rest; - with nothing else to check, or a file whose content was not read, or a rule + very large or binary file; a file GitHub only renamed has nothing to read + and is not listed) or the step passed that ceiling, is listed in the + evidence as not checked, saying which, and the rule runs on the rest; with + nothing else to check, or a file whose content was not read, or a rule ast-grep refuses, the rule is unconfirmed; `vendor/`, `dist/`, `build/` and `generated/` at the repository root and `node_modules/` anywhere are skipped and listed as skipped in the evidence, and a `?? 0` inside diff --git a/packages/triage-workflows/src/actions/facts.ts b/packages/triage-workflows/src/actions/facts.ts index cf82413cd..d047da6d3 100644 --- a/packages/triage-workflows/src/actions/facts.ts +++ b/packages/triage-workflows/src/actions/facts.ts @@ -142,11 +142,11 @@ async function readContents(host: HostCall, tool: string, repo: string, sha: str } /** Each code-patterns rule's matches, run where the files are read so their text never leaves this step. */ -async function patternsOf(host: HostCall, tool: string, repo: string, item: { facts: PrFacts; pack: CheckPack }): Promise> { +async function patternsOf(host: HostCall, tool: string, repo: string, source: RepoSource, item: { facts: PrFacts; pack: CheckPack }): Promise> { const rules = item.pack.custom.filter(isPatternsCheck).filter(customCheckEnabled); - const toRead = filesToRead(item.facts.files); + const toRead = filesToRead(item.facts.files, source); const contents = Array.isArray(toRead) ? await readContents(host, tool, repo, item.facts.headSha, toRead) : {}; - return Object.fromEntries(rules.map((rule) => [rule.id, patternsResult(rule, item.facts.files, contents)])); + return Object.fromEntries(rules.map((rule) => [rule.id, patternsResult(rule, item.facts.files, contents, source)])); } /** An open pull request as the host lists it. */ @@ -370,7 +370,7 @@ async function fetchTargets({ host, stamp }: Host, repo: string, numbers: number if (!pack.custom.filter(isPatternsCheck).some(customCheckEnabled)) return items; const matched: RulesItem[] = []; for (const item of items) { - matched.push("facts" in item ? { ...item, facts: { ...item.facts, patterns: await patternsOf(host, tool("read_file"), repo, item) } } : item); + matched.push("facts" in item ? { ...item, facts: { ...item.facts, patterns: await patternsOf(host, tool("read_file"), repo, stamp.repoSource, item) } } : item); } return matched; } diff --git a/packages/triage-workflows/src/logic/checks.test.ts b/packages/triage-workflows/src/logic/checks.test.ts index 79ed05118..9153652d6 100644 --- a/packages/triage-workflows/src/logic/checks.test.ts +++ b/packages/triage-workflows/src/logic/checks.test.ts @@ -369,7 +369,7 @@ rules: `; function patterns(files: PrFileFacts[], contents: Record, pack = readCheckPack(readPolicyFile(FILE, "acme/widgets"))) { - const matched = Object.fromEntries(pack.custom.filter(isPatternsCheck).map((rule) => [rule.id, patternsResult(rule, files, contents)])); + const matched = Object.fromEntries(pack.custom.filter(isPatternsCheck).map((rule) => [rule.id, patternsResult(rule, files, contents, "github")])); return deriveState({ ...open, files, patterns: matched }, undefined, pack); } @@ -447,9 +447,9 @@ rules: test("a file without its patch, as on Bitbucket Data Center, an unread content or a broken rule never passes", () => { const unpatched = { path: "src/a.ts", status: "modified", additions: 0, deletions: 0 }; - expect(filesToRead([unpatched])).toEqual({ unread: "src/a.ts: the host sent no diff" }); + expect(filesToRead([unpatched], "github")).toEqual({ unread: "src/a.ts: the host sent no diff" }); const past = (path: string) => ({ ...unpatched, path, patchUnread: true as const }); - expect(filesToRead([unpatched, past("src/b.ts"), past("src/c.ts")])).toEqual({ unread: "src/a.ts: the host sent no diff; src/b.ts and 1 more: the pull request's diff is larger than Triage reads" }); + expect(filesToRead([unpatched, past("src/b.ts"), past("src/c.ts")], "github")).toEqual({ unread: "src/a.ts: the host sent no diff; src/b.ts and 1 more: the pull request's diff is larger than Triage reads" }); expect(patterns([unpatched], {}).checks.find((check) => check.check === "conventions")).toMatchObject({ result: "unconfirmed", output: { kind: "could-not-check" } }); expect(patterns([file], {}).checks.find((check) => check.check === "conventions")).toMatchObject({ result: "unconfirmed" }); const nested = patterns([{ ...file, path: "src/build/a.ts" }], {}); @@ -463,13 +463,20 @@ rules: const large = { path: "src/large.ts", status: "modified", additions: 9_000, deletions: 0 }; const clean = { path: "src/b.ts", status: "added", additions: 1, deletions: 0, patch: "@@ -0,0 +1 @@\n+const b = 1;" }; const left = "src/large.ts: not checked, the host sent no diff"; - expect(filesToRead([file, large, clean])).toEqual(["src/a.ts", "src/b.ts"]); + expect(filesToRead([file, large, clean], "github")).toEqual(["src/a.ts", "src/b.ts"]); const failed = patterns([file, large], { "src/a.ts": content }); expect(failed.checks.find((check) => check.check === "conventions")).toMatchObject({ result: "fail", evidence: expect.arrayContaining([left]), skipped: [left] }); const passed = patterns([clean, large], { "src/b.ts": "const b = 1;\n" }); expect(passed.checks.find((check) => check.check === "conventions")).toMatchObject({ result: "pass", reason: "no code pattern matched; 1 file was not checked", evidence: [left] }); }); + test("a file GitHub only renamed is nothing to read, while a Data Center move without its patch stays unread", () => { + const renamed = { path: "src/moved.ts", previousPath: "src/old.ts", status: "renamed", additions: 0, deletions: 0 }; + expect(filesToRead([renamed, file], "github")).toEqual(["src/a.ts"]); + expect(patternsResult(readCheckPack(readPolicyFile(FILE, "acme/widgets")).custom.find(isPatternsCheck)!, [renamed], {}, "github")).toEqual({ matches: [], skipped: [], checked: 1 }); + expect(filesToRead([renamed], "bitbucket-datacenter")).toEqual({ unread: "src/moved.ts: the host sent no diff" }); + }); + test("an unknown preset is refused", () => { expect(() => readPolicyFile(FILE.replace("[convention]", "[nope]"), "acme/widgets")).toThrow(); }); diff --git a/packages/triage-workflows/src/logic/facts.test.ts b/packages/triage-workflows/src/logic/facts.test.ts index 8e3cb9482..be23ca112 100644 --- a/packages/triage-workflows/src/logic/facts.test.ts +++ b/packages/triage-workflows/src/logic/facts.test.ts @@ -116,7 +116,7 @@ test("a diff past the judge's budget is read whole for the pattern rules while t const pack = readCheckPack(readPolicyFile("rules:\n - id: conventions\n check: code-patterns\n text: Conventions.\n reason: Review time.\n evidence: [maintainer-stated]\n presets: [convention]\n", "acme/widgets")); const contents = Object.fromEntries(paths.map((path, index) => [path, `${lines(index).join("\n")}\n`])); - expect(patternsResult(pack.custom.find(isPatternsCheck)!, listed, contents)).toEqual({ matches: [`${paths.at(-1)}:201 Invented fallback \`?? 0\`; handle the missing value instead.`], skipped: [], checked: paths.length }); + expect(patternsResult(pack.custom.find(isPatternsCheck)!, listed, contents, "github")).toEqual({ matches: [`${paths.at(-1)}:201 Invented fallback \`?? 0\`; handle the missing value instead.`], skipped: [], checked: paths.length }); const focused = { quality: [{ id: "focused" as const, group: "pull-request" as const }], custom: [], rules: [] }; const { requests, focused: plan } = prepareQualityEvaluation({ ...NO_FACTS, repo: "acme/widgets", number: 8, title: "Add files", files: listed, paths }, focused); @@ -137,7 +137,7 @@ describe("a diff past the code rules' ceiling", () => { const unread = listed.slice(whole); expect(unread).toEqual(hostFiles.slice(whole).map(({ patch: _patch, ...meta }) => ({ ...meta, patchUnread: true }))); const contents = Object.fromEntries(listed.slice(0, whole).map((file) => [file.path, `${Array.from({ length: 200 }, () => line).join("\n")}\n`])); - expect(patternsResult(pack.custom.find(isPatternsCheck)!, listed, contents)).toEqual({ + expect(patternsResult(pack.custom.find(isPatternsCheck)!, listed, contents, "github")).toEqual({ matches: [], skipped: unread.map((file) => `${file.path}: not checked, the pull request's diff is larger than Triage reads`), checked: whole, diff --git a/packages/triage-workflows/src/logic/patterns.ts b/packages/triage-workflows/src/logic/patterns.ts index 2f80652f8..b02c1c176 100644 --- a/packages/triage-workflows/src/logic/patterns.ts +++ b/packages/triage-workflows/src/logic/patterns.ts @@ -1,5 +1,5 @@ import { parse } from "@ast-grep/napi"; -import { isForeignPath, PATTERN_EXTENSIONS, PATTERN_PRESETS, type PatternExtension, patternRuleApplies, type PatternLanguage, type PatternRule, type PatternsCheck } from "@corbits/triage-contracts"; +import { isForeignPath, PATTERN_EXTENSIONS, PATTERN_PRESETS, type PatternExtension, patternRuleApplies, type PatternLanguage, type PatternRule, type PatternsCheck, type RepoSource } from "@corbits/triage-contracts"; import type { PrFileFacts } from "./checks.js"; import { addedLines } from "./hunks.js"; @@ -51,8 +51,13 @@ function inScope(file: PrFileFacts): boolean { * A file in scope without a patch: GitHub sends none for a very large or binary file, Bitbucket Data Center for any, * and none is read past the code rules' ceiling. */ -function unpatched(file: PrFileFacts): boolean { - return inScope(file) && file.patch === undefined; +function unpatched(file: PrFileFacts, source: RepoSource): boolean { + return inScope(file) && file.patch === undefined && !renamedOnly(file, source); +} + +/** GitHub sends no patch for a file it only renamed, so there is nothing to read; Data Center counts no lines to tell. */ +function renamedOnly(file: PrFileFacts, source: RepoSource): boolean { + return source === "github" && file.status === "renamed" && file.additions === 0 && file.deletions === 0; } function whyUnpatched(file: PrFileFacts): string { @@ -79,9 +84,9 @@ export const MAX_CONTENT_READS = 300; * The files whose content the rules need, or why they cannot be checked: files in scope came without a patch and none * other adds code, or more files than are read. */ -export function filesToRead(files: readonly PrFileFacts[]): string[] | { unread: string } { +export function filesToRead(files: readonly PrFileFacts[], source: RepoSource): string[] | { unread: string } { const toRead = files.filter((file) => inScope(file) && file.patch !== undefined && addedLines(file.patch).size > 0).map(readPath); - const missing = files.filter(unpatched); + const missing = files.filter((file) => unpatched(file, source)); if (toRead.length === 0 && missing.length > 0) return { unread: namedUnpatched(missing) }; if (toRead.length > MAX_CONTENT_READS) return { unread: `${toRead.length} files with added code, more than the ${MAX_CONTENT_READS} read per pull request` }; return toRead; @@ -108,8 +113,8 @@ function fileMatches(file: PrFileFacts, patch: string, content: string, grammar: * Matches on the lines a pull request adds; a file in scope whose content was not read leaves the rule unchecked, and * one without a patch is named as not checked. */ -export function patternsResult(check: PatternsCheck, files: readonly PrFileFacts[], contents: Readonly>): PatternsResult { - const toRead = filesToRead(files); +export function patternsResult(check: PatternsCheck, files: readonly PrFileFacts[], contents: Readonly>, source: RepoSource): PatternsResult { + const toRead = filesToRead(files, source); if (!Array.isArray(toRead)) return toRead; const matches: string[] = []; try { @@ -122,7 +127,7 @@ export function patternsResult(check: PatternsCheck, files: readonly PrFileFacts } catch (error) { return { unread: error instanceof Error ? error.message : String(error) }; } - const missing = files.filter(unpatched); + const missing = files.filter((file) => unpatched(file, source)); const skipped = [ ...missing.map((file) => `${file.path}: not checked, ${whyUnpatched(file)}`), ...files.filter((file) => checkable(file) && isForeignPath(file.path)).map((file) => `${file.path}: skipped as vendored, built or installed code`), From 7ae9f76f653da51ca424c69535a606746443ba3e Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Tue, 27 Oct 2026 17:13:47 -0700 Subject: [PATCH 5/9] fix(triage-workflows): give each pull request its own patch ceiling and keep no generated patch A pull request's result no longer depends on which others share its batch, and lockfile patches, which no rule reads, are dropped so they neither count nor ride in the step output. --- docs/ARCHITECTURE.md | 45 ++++++++++--------- .../Corbits Triage User Manual.md | 2 +- docs/DECISION_MODEL.md | 4 +- .../triage-workflows/src/actions/facts.ts | 19 ++++---- .../triage-workflows/src/logic/facts.test.ts | 26 ++++++----- packages/triage-workflows/src/logic/facts.ts | 43 +++++++++--------- .../triage-workflows/src/logic/quality.ts | 5 +-- 7 files changed, 74 insertions(+), 70 deletions(-) diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index a9d82148f..fe09f0b39 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -549,28 +549,29 @@ label off that head, so it never stands on a head not yet triaged. (ast-grep's glob rules, checked when the policy is read), and keeps matches on lines the patch adds. The facts step reads every changed file's patch whole, across as many tool - pages as it takes, until the step has read 2,000,000 characters - (`MAX_CODE_RULES_PATCH_CHARS`), shared by every pull request a backlog - batch reads, since one sidecar holds every deployment's step outputs; - lockfiles and other generated files do not count. The file that passes it - partway and every later one are listed without patches. The judge's view of - the diff has its own, smaller budget (`judgedFiles` in `logic/quality.ts`). - A file without a patch, because the host sent none (GitHub omits it for a - very large or binary file; a file GitHub only renamed has nothing to read - and is not listed) or the step passed that ceiling, is listed in the - evidence as not checked, saying which, and the rule runs on the rest; with - nothing else to check, or a file whose content was not read, or a rule - ast-grep refuses, the rule is unconfirmed; `vendor/`, `dist/`, `build/` and - `generated/` at the repository root and `node_modules/` anywhere are - skipped and listed as skipped in the evidence, and a `?? 0` inside - arithmetic is a counter's real default. With such a rule on, the facts - step reads each file whose patch adds lines at the head through the host's - `read_file` (`whole`), ten at a time and at most 300 per pull request (more - leaves the rule unconfirmed without reading any), runs the rules there and - outputs only the matches. Bitbucket Data Center lists no patch, so there - the rule is unconfirmed whenever an in-scope TypeScript or JavaScript file - is added, edited, moved or copied. The extension leaves code-pattern - rules out and swaps the native module for a stub. + pages as it takes, until it has read 2,000,000 characters of the pull + request (`MAX_CODE_RULES_PATCH_CHARS`), each pull request in a batch + counting its own, since one sidecar holds every deployment's step outputs; + lockfiles and other generated files are listed without their patch, which + no rule reads. The file that passes it partway and every later one are + listed without patches. The judge's view of the diff has its own, smaller + budget (`judgedFiles` in `logic/quality.ts`). A file without a patch, + because the host sent none (GitHub omits it for a very large or binary + file; a file GitHub only renamed has nothing to read and is not listed) or + the pull request passed that ceiling, is listed in the evidence as not + checked, saying which, and the rule runs on the rest; with nothing else to + check, or a file whose content was not read, or a rule ast-grep refuses, + the rule is unconfirmed; `vendor/`, `dist/`, `build/` and `generated/` at + the repository root and `node_modules/` anywhere are skipped and listed as + skipped in the evidence, and a `?? 0` inside arithmetic is a counter's real + default. With such a rule on, the facts step reads each file whose patch + adds lines at the head through the host's `read_file` (`whole`), ten at a + time and at most 300 per pull request (more leaves the rule unconfirmed + without reading any), runs the rules there and outputs only the matches. + Bitbucket Data Center lists no patch, so there the rule is unconfirmed + whenever an in-scope TypeScript or JavaScript file is added, edited, moved + or copied. The extension leaves code-pattern rules out and swaps the native + module for a stub. 3. **route**: skips the judge when no question needs asking. 4. **judge**: one System One call per request of questions. Each first-round request and the verdict request also asks whether the pull request's text, diff or docs try to direct the judge; a diff --git a/docs/Corbits Triage User Manual/Corbits Triage User Manual.md b/docs/Corbits Triage User Manual/Corbits Triage User Manual.md index 929955bb0..5148aaac5 100644 --- a/docs/Corbits Triage User Manual/Corbits Triage User Manual.md +++ b/docs/Corbits Triage User Manual/Corbits Triage User Manual.md @@ -258,7 +258,7 @@ A rule asks one question about a pull request and answers pass, fail or unconfir - **Flag changes to sensitive paths.** Checks changed files against the file's named groups of globs, each `high` or `medium`, written as `groups:`, such as `infra: { level: high, paths: ["infra/**", "**/*.tf"] }`. Without `groups` there are four: `schema` (`**/migrations/**`, `**/*.sql`) and `auth` (`**/auth/**`, `**/secrets/**`), which are high, and `ci` (`.github/**`, `**/Dockerfile`, `**/railway.json`, `**/railway.toml`) and `locks` (`**/bun.lock`, `**/package-lock.json`, `**/yarn.lock`, `**/pnpm-lock.yaml`), which are medium, since every dependency bump changes a lockfile. Globs read as in **Never accept changes to**. A change to a group at `failAt` or above, high unless the file says, fails the rule, which asks a maintainer unless the file gives it another `onFail`. Below that, from `labelAt`, medium unless the file says, it passes but labels the pull request `sensitive: medium` and notes it under **Notes:** (on Bitbucket Data Center, only the note). Its evidence names each group with its files, at most five, such as `schema: db/migrations/0004.sql`, then how many more. Regression risk keeps its own migration globs. - **Flag high-risk pull requests and label medium-risk ones.** Rates each pull request low, medium or high risk; see [Risk](#risk). - **Fail when …**, a text rule of your own, such as "Fail when any commit message contains "Co-Authored-By:"". It names where to look (the title, the description, any commit message, or several of them), whether the rule fails when the text **contains** or **does not contain** its phrases, and the phrases. Case does not matter. A **contains** rule on the description also looks at its hidden parts, such as HTML comments and the addresses behind links, while **does not contain** reads only what a reader sees. Only the first 100 commits are read. When a **contains** rule fails, Triage names each place and the phrases found there, asks for them to be removed, and quotes each matching line once. Text rules count toward the eight rules of your own. - - **Fail when added code matches …**, a code-pattern rule of your own, such as one that flags `?? 0`. It checks only the lines the pull request adds to TypeScript and JavaScript files (`.ts`, `.tsx`, `.mts`, `.cts`, `.js`, `.jsx`, `.mjs`, `.cjs`), so code already on the branch never fails it. On any other language it passes with **nothing checked: no TypeScript or JavaScript code added**, so don't add one to a repository written in Go, Python or similar. Turn on the shipped `convention` preset, which flags invented fallbacks (`?? 0`, `?? []`), nested ternaries, functions called where they are written, `.then`/`.catch` chains, empty `catch` blocks and lint or type suppression comments in both languages, or write your own patterns in [ast-grep](https://ast-grep.github.io/guide/rule-config.html)'s rule format. Your own pattern replaces a preset's pattern with the same `id` in both languages; to keep checking JavaScript, add a `language: javascript` pattern under the same id. A pattern takes ast-grep's own `files` and `ignores` lists to check only some paths or skip some. They follow ast-grep's glob rules: paths are from the repository root, `*` also matches across folders, and dotfiles are matched like any other file. Write each glob as a string; the `{ glob, caseInsensitive }` form is not read. Write a negated class as `[^a]`; a glob with `[!a]`, or one that isn't valid, refuses the policy. The preset's `?? 0` and `?? []` patterns ignore test files (`**/*.test.ts`, `**/*.test.tsx`, `**/*.test.js`, `**/*.test.jsx`, `**/*.test.mjs`, `**/*.test.cjs`). Each match is listed as evidence, as `path:line` and the pattern's message. Vendored and built code at the repository root (`vendor/`, `dist/`, `build/`, `generated/`) and installed code (`node_modules/` anywhere) is skipped, and each skipped file is listed in the evidence. The same folders deeper in the repository, such as `src/build/`, are checked. When the host sends no changed lines for a file, as GitHub does for a very large or binary one, or the diff is larger than the 2,000,000 characters Triage reads in one run (several pull requests checked together share them), those files are listed in the evidence as not checked, with the reason, and every other file is checked; if no other file adds code, the rule is unconfirmed. When a file's content can't be read or a pattern is not valid ast-grep, the rule is unconfirmed rather than passing. Triage reads at most 300 files per pull request, so on one that adds code to more, the rule is unconfirmed with a reason such as **code patterns could not be checked: 412 files with added code, more than the 300 read per pull request**. On Bitbucket Data Center, which does not give Triage each file's changed lines, it is unconfirmed whenever a TypeScript or JavaScript file of your own is added, edited, moved or copied. To give some patterns a different `onFail`, put them in a rule of their own. Code-pattern rules count toward the eight rules of your own, and the browser extension leaves them out. + - **Fail when added code matches …**, a code-pattern rule of your own, such as one that flags `?? 0`. It checks only the lines the pull request adds to TypeScript and JavaScript files (`.ts`, `.tsx`, `.mts`, `.cts`, `.js`, `.jsx`, `.mjs`, `.cjs`), so code already on the branch never fails it. On any other language it passes with **nothing checked: no TypeScript or JavaScript code added**, so don't add one to a repository written in Go, Python or similar. Turn on the shipped `convention` preset, which flags invented fallbacks (`?? 0`, `?? []`), nested ternaries, functions called where they are written, `.then`/`.catch` chains, empty `catch` blocks and lint or type suppression comments in both languages, or write your own patterns in [ast-grep](https://ast-grep.github.io/guide/rule-config.html)'s rule format. Your own pattern replaces a preset's pattern with the same `id` in both languages; to keep checking JavaScript, add a `language: javascript` pattern under the same id. A pattern takes ast-grep's own `files` and `ignores` lists to check only some paths or skip some. They follow ast-grep's glob rules: paths are from the repository root, `*` also matches across folders, and dotfiles are matched like any other file. Write each glob as a string; the `{ glob, caseInsensitive }` form is not read. Write a negated class as `[^a]`; a glob with `[!a]`, or one that isn't valid, refuses the policy. The preset's `?? 0` and `?? []` patterns ignore test files (`**/*.test.ts`, `**/*.test.tsx`, `**/*.test.js`, `**/*.test.jsx`, `**/*.test.mjs`, `**/*.test.cjs`). Each match is listed as evidence, as `path:line` and the pattern's message. Vendored and built code at the repository root (`vendor/`, `dist/`, `build/`, `generated/`) and installed code (`node_modules/` anywhere) is skipped, and each skipped file is listed in the evidence. The same folders deeper in the repository, such as `src/build/`, are checked. When the host sends no changed lines for a file, as GitHub does for a very large or binary one, or the pull request's diff is larger than the 2,000,000 characters Triage reads, those files are listed in the evidence as not checked, with the reason, and every other file is checked; if no other file adds code, the rule is unconfirmed. When a file's content can't be read or a pattern is not valid ast-grep, the rule is unconfirmed rather than passing. Triage reads at most 300 files per pull request, so on one that adds code to more, the rule is unconfirmed with a reason such as **code patterns could not be checked: 412 files with added code, more than the 300 read per pull request**. On Bitbucket Data Center, which does not give Triage each file's changed lines, it is unconfirmed whenever a TypeScript or JavaScript file of your own is added, edited, moved or copied. To give some patterns a different `onFail`, put them in a rule of their own. Code-pattern rules count toward the eight rules of your own, and the browser extension leaves them out. - **Judgement**, questions the decision model answers from the diff and the documentation: - **Check the change is focused on one thing.** The decision model is asked whether the pull request does one thing. When it fails, the verdict comment says it mixes unrelated changes and asks the author to split them out, unless the file sets another `onFail`. A pull request based on another open pull request's branch is checked on its own changes, and the comment says so, such as "Stacked on #1573: only this pull request's own changes were checked." On a branch another pull request was merged into, this rule is skipped, and the comment says why, such as "stacked branch, includes #1521". - **Check docs are updated when behavior changes.** Changing a documentation file passes it. Otherwise the decision model is asked, file by file, whether the docs stay correct, a statement in them is now wrong, or the change adds setup the docs must describe. When it fails, the verdict comment asks the author to update the documentation and names the files, unless the file sets another `onFail`. The default label, when `onFail` is `label`, is `docs: check`. diff --git a/docs/DECISION_MODEL.md b/docs/DECISION_MODEL.md index 75230d3e1..3601d2b3a 100644 --- a/docs/DECISION_MODEL.md +++ b/docs/DECISION_MODEL.md @@ -100,13 +100,13 @@ Two different numbers come out of the answers: | Contribution rules | 8,000 bytes, default branch | `REPO_RULES_MAX_BYTES` | | Patch the judge reads per file | 65,536 chars | `MAX_PATCH_INPUT_CHARS` | | Patch the judge reads per pull request | 270,000 chars | `MAX_PATCH_CHARS` | -| Patch the rules that run in code read per facts step, shared by a backlog batch | 2,000,000 chars | `MAX_CODE_RULES_PATCH_CHARS` | +| Patch the rules that run in code read per pull request | 2,000,000 chars | `MAX_CODE_RULES_PATCH_CHARS` | | GitHub tool page | 9,000 chars | `PR_FILES_PAGE_CHARS` | - **Docs.** Docs are read from the default branch in this order: root `README`, then `docs/`, then other root markdown, then nested markdown, each in path order. Reading stops at 24,000 bytes; a doc that crosses the cap is cut and later docs are not seen. The same docs are in every request's state. - **Contribution rules.** Read under their own 8,000-byte budget, in this order: `AGENTS.md`, `CLAUDE.md`, `CONTRIBUTING` and `PULL_REQUEST_TEMPLATE` at the root, then `CONTRIBUTING` and `PULL_REQUEST_TEMPLATE` in `.github/` (including `PULL_REQUEST_TEMPLATE/`), then the same two in `docs/`. An `AGENTS.md` or `CLAUDE.md` inside a package stays an ordinary doc. Only requests that ask a custom rule carry them and give up room for them; requests that ask only built-in rules are unchanged. - **Request splitting.** Files fill each request's state in order up to the budget left after metadata and docs. A file too large for one request keeps its leading hunks there for every rule, and focused and docs read its remaining hunks in the next requests, docs asking about each part (any part needing docs fails it); a hunk too large for any request is cut at line ends across several. Each request asks only about the files it holds; whole-PR questions go in the first, and focused asks in each. If the title, body, commits, path list and docs leave less than 8 KB of the budget for files, nothing is asked and the model rules stay unconfirmed ("pull request too large for the decision model (N KB)", the size of that metadata). The same holds when the split needs more than 64 requests ("… (N requests)"). -- **Oversized patches.** A patch larger than one tool page is read across pages and joined, and every patch is read whole for the rules that run in code until one facts step has read 2,000,000 chars, a backlog batch sharing them; lockfiles and other generated files do not count. The file that passes it partway and every later one are listed without patches. The judge reads 65,536 chars of a file, which is then marked cut. Once the judge's 270,000-char budget is spent, later files are sent to it without patches. +- **Oversized patches.** A patch larger than one tool page is read across pages and joined, and every patch is read whole for the rules that run in code until 2,000,000 chars of the pull request, each pull request in a backlog batch counting its own; lockfiles and other generated files are listed without their patch, so they do not count. The file that passes it partway and every later one are listed without patches. The judge reads 65,536 chars of a file, which is then marked cut. Once the judge's 270,000-char budget is spent, later files are sent to it without patches. - **Unread files.** A file with lines changed but no patch to read (GitHub left it out, the judge's budget was spent, or not even its first hunk fits a request) is unread, and a rule that would ask about it stays unconfirmed. Treatment differs by rule: - Docs reads a file too large for one request across requests as focused does. A file whose patch was cut while reading is asked about from the hunks that were read and is unread, so it can fail docs but never pass it. A changed doc file passes docs by rule regardless. - Scoped custom rules ask about a cut file (its patch cut while reading, or too large for one request) from the hunks that were read and treat it as unread. diff --git a/packages/triage-workflows/src/actions/facts.ts b/packages/triage-workflows/src/actions/facts.ts index d047da6d3..2526e1921 100644 --- a/packages/triage-workflows/src/actions/facts.ts +++ b/packages/triage-workflows/src/actions/facts.ts @@ -4,9 +4,9 @@ import { type } from "arktype"; import { ACTION_KINDS, catalogCheckEnabled, classificationSources, customCheckEnabled, DEFAULT_RISK, emptyPack, isPatternsCheck, PolicySourceShape, repoPolicy, repoSourceOf, triageEventsOf, trustedRequestOf, visibleText, type ActionKind, type CheckPack, type PolicySource, type RepoSource, type TriageEvent } from "@corbits/triage-contracts"; import { repoDocs, type ClosedPr, type IssueComment, type RepoDocsPage, type Review, type RuleSpan } from "@corbits/github-reads"; import { isRecord } from "@corbits/common"; -import { DEPLOYMENT_UNREADABLE, hasModelChecks, NEEDS_SETUP_REASON, packFromInput, type PrFacts, type PrFileFacts } from "../logic/checks.js"; +import { DEPLOYMENT_UNREADABLE, hasModelChecks, NEEDS_SETUP_REASON, packFromInput, type PrFacts } from "../logic/checks.js"; import { asText } from "../logic/extract.js"; -import { addFilesPage, assembleItem, fileFacts, FIRST_FILES_PAGE, optionalCount, namedTeams, type CheckRun, type FilesCursor, type FilesPage, type PatchesRead, type PrData, type PullData, type RepoDocs } from "../logic/facts.js"; +import { addFilesPage, assembleItem, fileFacts, FIRST_FILES_PAGE, optionalCount, namedTeams, type CheckRun, type FilesCursor, type FilesPage, type ListedFiles, type PrData, type PullData, type RepoDocs } from "../logic/facts.js"; import { couldRepeat, MAX_OPEN_PULLS, overlapping, type OpenPull, type OpenPulls, type PullPrint } from "../logic/duplicates.js"; import type { History } from "../logic/history.js"; import { filesToRead, patternsResult, type PatternsResult } from "../logic/patterns.js"; @@ -239,8 +239,7 @@ async function fetchTargets({ host, stamp }: Host, repo: string, numbers: number const teams = namedTeams(pack, repo); const focused = catalogCheckEnabled(pack, "focused"); const risk = catalogCheckEnabled(pack, "regression-risk") || catalogCheckEnabled(pack, "risk"); - const listed = new Map(); - const patchesRead: PatchesRead = { chars: 0 }; + const listed = new Map(); const filesErrors = new Map(); function filesId(n: number, { offset, patchOffset }: FilesCursor): string { @@ -265,9 +264,9 @@ async function fetchTargets({ host, stamp }: Host, repo: string, numbers: number function addFiles(n: number, cursor: FilesCursor, result: ToolResult | undefined): FilesCursor | undefined { const page = filesPage(tool("list_pr_files"), result); - const files = listed.get(n) ?? []; - listed.set(n, files); - const next = typeof page === "string" ? page : addFilesPage(files, patchesRead, cursor, page); + const read = listed.get(n) ?? { files: [], patchChars: 0 }; + listed.set(n, read); + const next = typeof page === "string" ? page : addFilesPage(read, cursor, page); if (typeof next !== "string") return next; filesErrors.set(n, `${next} for #${n}`); return undefined; @@ -298,7 +297,7 @@ async function fetchTargets({ host, stamp }: Host, repo: string, numbers: number } function historyPathsOf(n: number): string[] { - return historyPaths(listed.get(n)?.map((file) => file.path) ?? []); + return historyPaths(listed.get(n)?.files.map((file) => file.path) ?? []); } function historyCalls(): ToolCall[] { @@ -310,7 +309,7 @@ async function fetchTargets({ host, stamp }: Host, repo: string, numbers: number /** A pull request the host read whole, as the duplicate rules compare it. */ function ownPull(n: number): OwnPull[] { const pr = prs.get(n); - const files = listed.get(n); + const files = listed.get(n)?.files; if (!pr?.sha || !pr.base || pr.title === undefined || !files || filesErrors.has(n)) return []; return [{ number: n, title: visibleText(pr.title), headSha: pr.sha, base: pr.base, paths: files.map((file) => file.path) }]; } @@ -356,7 +355,7 @@ async function fetchTargets({ host, stamp }: Host, repo: string, numbers: number comments: data<{ comments: IssueComment[] }>(r1.get(`comments:${n}`))?.comments ?? [], commits: data<{ commits: Array<{ message?: string }> }>(r1.get(`commits:${n}`))?.commits ?? [], stack: data>(r1.get(`stack:${n}`)), - files: filesErrors.get(n) ?? listed.get(n) ?? [], + files: filesErrors.get(n) ?? listed.get(n)?.files ?? [], authorTeams: data>(r2.get(`teams:${prs.get(n)?.author}`)), fixes: fixesOf(n), }, { repo, policy, pack, events, docs, templates, source: stamp.repoSource }); diff --git a/packages/triage-workflows/src/logic/facts.test.ts b/packages/triage-workflows/src/logic/facts.test.ts index be23ca112..afbb791f7 100644 --- a/packages/triage-workflows/src/logic/facts.test.ts +++ b/packages/triage-workflows/src/logic/facts.test.ts @@ -2,7 +2,7 @@ import { describe, expect, test } from "bun:test"; import { DEFAULT_REPO_POLICY, emptyPack, isPatternsCheck, readCheckPack, readPolicyFile } from "@corbits/triage-contracts"; import type { IssueComment, Review } from "@corbits/github-reads"; import type { PrFileFacts } from "./checks.ts"; -import { addFilesPage, assembleItem, buildFacts, FIRST_FILES_PAGE, type FilesCursor, type FilesPage, type PatchesRead } from "./facts.ts"; +import { addFilesPage, assembleItem, buildFacts, FIRST_FILES_PAGE, type FilesCursor, type FilesPage, type ListedFiles } from "./facts.ts"; import { NO_FACTS } from "./item.ts"; import { patternsResult } from "./patterns.ts"; import { MAX_CODE_RULES_PATCH_CHARS, MAX_SYSTEM_ONE_STATE_BYTES, prepareQualityEvaluation } from "./quality.ts"; @@ -87,8 +87,8 @@ describe("an answered review", () => { }); /** Reads `hostFiles` as the host pages them: one file a page, each patch split across two pages, none when the cursor asks for none. */ -function readPages(hostFiles: PrFileFacts[], first: FilesCursor = FIRST_FILES_PAGE, read: PatchesRead = { chars: 0 }): PrFileFacts[] { - const listed: PrFileFacts[] = []; +function readPages(hostFiles: PrFileFacts[], first: FilesCursor = FIRST_FILES_PAGE): PrFileFacts[] { + const listed: ListedFiles = { files: [], patchChars: 0 }; let cursor: FilesCursor | undefined | string = first; while (cursor !== undefined) { if (typeof cursor === "string") throw new Error(cursor); @@ -98,9 +98,9 @@ function readPages(hostFiles: PrFileFacts[], first: FilesCursor = FIRST_FILES_PA let page: FilesPage = { files: [meta], ...next }; if (cursor.patches && cursor.patchOffset === undefined) page = { files: [{ ...meta, patch: patch!.slice(0, half) }], next: cursor.offset, patchNext: half }; else if (cursor.patches) page = { files: [{ ...meta, patch: patch!.slice(cursor.patchOffset) }], ...next }; - cursor = addFilesPage(listed, read, cursor, page); + cursor = addFilesPage(listed, cursor, page); } - return listed; + return listed.files; } test("a diff past the judge's budget is read whole for the pattern rules while the judge reads a bounded part", () => { @@ -152,18 +152,20 @@ describe("a diff past the code rules' ceiling", () => { expect(chars * 10).toBeGreaterThan(MAX_CODE_RULES_PATCH_CHARS); }); - test("is shared by every pull request the step reads, so one read after it is reached gets no patches", () => { - const read: PatchesRead = { chars: MAX_CODE_RULES_PATCH_CHARS }; + test("is each pull request's own, so one read after another passed it still gets its patches", () => { + const filler = `+const filler = "${"x".repeat(1_000)}";`; + const big: PrFileFacts = { path: "src/big.ts", status: "added", additions: 2_000, deletions: 0, patch: ["@@ -0,0 +1,2000 @@", ...Array.from({ length: 2_000 }, () => filler)].join("\n") }; const file = { path: "src/a.ts", status: "added", additions: 1, deletions: 0, patch: "@@ -0,0 +1 @@\n+const a = 1;" }; - const listed = readPages([file, { ...file, path: "src/b.ts" }], FIRST_FILES_PAGE, read); - expect(listed).toEqual(["src/a.ts", "src/b.ts"].map((path) => ({ path, status: "added", additions: 1, deletions: 0, patchUnread: true }))); + expect(readPages([big, file])).toEqual([big, { path: "src/a.ts", status: "added", additions: 1, deletions: 0, patchUnread: true }]); + expect(readPages([file])).toEqual([file]); }); - test("leaves lockfiles out of the count, and marks nothing when the step asked for no patches", () => { + test("keeps no lockfile patch, so a lockfile neither counts nor rides in the output, and marks nothing when no patches were asked for", () => { const lock = { path: "bun.lock", status: "modified", additions: 1, deletions: 0, patch: `@@ -1 +1 @@\n+${"x".repeat(MAX_CODE_RULES_PATCH_CHARS)}` }; + const { patch: _patch, ...lockMeta } = lock; const file = { path: "src/a.ts", status: "added", additions: 1, deletions: 0, patch: "@@ -0,0 +1 @@\n+const a = 1;" }; - expect(readPages([lock, file, file])).toEqual([lock, file, file]); - expect(readPages([lock, file], { offset: 0, patches: false })).toEqual([lock, file].map(({ patch: _patch, ...meta }) => meta)); + expect(readPages([lock, file, file])).toEqual([lockMeta, file, file]); + expect(readPages([lock, file], { offset: 0, patches: false })).toEqual([lock, file].map(({ patch: _unread, ...meta }) => meta)); }); test("drops the patch of a file it passes partway through", () => { diff --git a/packages/triage-workflows/src/logic/facts.ts b/packages/triage-workflows/src/logic/facts.ts index 1c7a4a514..eb6f122f3 100644 --- a/packages/triage-workflows/src/logic/facts.ts +++ b/packages/triage-workflows/src/logic/facts.ts @@ -96,11 +96,11 @@ export function buildFacts( export type FilesPage = { files: PrFileFacts[]; next?: number; patchNext?: number }; -/** `ceiling` is set once the step's patch ceiling is reached, so the files listed without patches are marked unread. */ +/** `ceiling` is set once the pull request's patch ceiling is reached, so the files listed without patches are marked unread. */ export type FilesCursor = { offset: number; patchOffset?: number; patches: boolean; ceiling?: true }; -/** The patch text one facts step has read, shared by every pull request it reads. */ -export type PatchesRead = { chars: number }; +/** One pull request's files so far and the patch text they hold. */ +export type ListedFiles = { files: PrFileFacts[]; patchChars: number }; export const FIRST_FILES_PAGE: FilesCursor = { offset: 0, patches: true }; @@ -108,34 +108,38 @@ function unreadPatch({ patch: _patch, ...file }: PrFileFacts): PrFileFacts { return { ...file, patchUnread: true }; } -/** Lockfiles and other generated files are read by no rule, so their patches do not count toward the ceiling. */ -function counted(file: PrFileFacts): number { - return isGeneratedPath(file.path) ? 0 : file.patch?.length ?? 0; +/** No rule reads a lockfile's or other generated file's patch, so it is never kept. */ +function kept(file: PrFileFacts): PrFileFacts { + if (!isGeneratedPath(file.path)) return file; + const { patch: _patch, ...meta } = file; + return meta; } /** * Adds one page to `listed`, joining a patch continued from the previous page, and returns the next page's cursor, - * none when every file is read, or why the pages cannot be read. Every patch is read whole until the step's ceiling; - * the file it falls in and every later one are listed without their patch and marked so. + * none when every file is read, or why the pages cannot be read. Every patch is read whole until the pull request's + * ceiling; the file it falls in and every later one are listed without their patch and marked so. */ -export function addFilesPage(listed: PrFileFacts[], read: PatchesRead, cursor: FilesCursor, page: FilesPage): FilesCursor | undefined | string { - let files = cursor.ceiling ? page.files.map(unreadPatch) : page.files; +export function addFilesPage(listed: ListedFiles, cursor: FilesCursor, page: FilesPage): FilesCursor | undefined | string { + let files = (cursor.ceiling ? page.files.map(unreadPatch) : page.files).map(kept); if (cursor.patchOffset !== undefined) { - const continued = listed.at(-1); + const continued = listed.files.at(-1); if (!continued || files[0]?.path !== continued.path) return `github_list_pr_files did not continue the patch at offset ${cursor.offset}`; continued.patch = (continued.patch ?? "") + (files[0].patch ?? ""); - read.chars += counted(files[0]); + listed.patchChars += files[0].patch?.length ?? 0; files = files.slice(1); } - listed.push(...files); - read.chars += files.reduce((chars, file) => chars + counted(file), 0); - const patches = cursor.patches && read.chars < MAX_CODE_RULES_PATCH_CHARS; + listed.files.push(...files); + listed.patchChars += files.reduce((chars, file) => chars + (file.patch?.length ?? 0), 0); + const patches = cursor.patches && listed.patchChars < MAX_CODE_RULES_PATCH_CHARS; const next = { patches, ...((cursor.ceiling || cursor.patches !== patches) && { ceiling: true as const }) }; if (page.next === undefined) return undefined; if (page.patchNext !== undefined) { if (page.next < cursor.offset || (page.next === cursor.offset && page.patchNext <= (cursor.patchOffset ?? 0))) return `github_list_pr_files did not advance past offset ${cursor.offset}`; + const cut = listed.files.at(-1)!; + if (isGeneratedPath(cut.path)) return { offset: page.next + 1, ...next }; if (patches) return { offset: page.next, patchOffset: page.patchNext, ...next }; - listed.push(unreadPatch(listed.pop()!)); + listed.files.push(unreadPatch(listed.files.pop()!)); return { offset: page.next + 1, ...next }; } if (page.next <= cursor.offset) return `github_list_pr_files did not advance past offset ${cursor.offset}`; @@ -144,16 +148,15 @@ export function addFilesPage(listed: PrFileFacts[], read: PatchesRead, cursor: F /** Every changed file of a pull request, read page by page as the facts director reads them. */ export async function listFiles(gh: GithubFetch, repo: string, number: number): Promise { - const listed: PrFileFacts[] = []; - const read: PatchesRead = { chars: 0 }; + const listed: ListedFiles = { files: [], patchChars: 0 }; let cursor: FilesCursor | undefined = FIRST_FILES_PAGE; while (cursor !== undefined) { const page = await listPrFilesPage(gh, repo, number, cursor); - const next = addFilesPage(listed, read, cursor, { ...page, files: page.files.flatMap(fileFacts) }); + const next = addFilesPage(listed, cursor, { ...page, files: page.files.flatMap(fileFacts) }); if (typeof next === "string") throw new Error(`${next} for #${number}`); cursor = next; } - return listed; + return listed.files; } function optionalString(value: unknown): string | undefined { diff --git a/packages/triage-workflows/src/logic/quality.ts b/packages/triage-workflows/src/logic/quality.ts index 893ba8b16..4fd02333d 100644 --- a/packages/triage-workflows/src/logic/quality.ts +++ b/packages/triage-workflows/src/logic/quality.ts @@ -184,9 +184,8 @@ export const QUALITY_EVALUATION_LIMIT_ERROR = "pull request too large for the de // The judge reads patches up to three requests' worth; later files are sent without theirs. const MAX_PATCH_CHARS = 3 * MAX_SYSTEM_ONE_STATE_BYTES; -// The rules that run in code read this much patch text per facts step, shared by every pull request a backlog batch -// reads, apart from the judge's budget; one sidecar holds every deployment's step outputs, so past it the step lists -// files without their patches. +// The rules that run in code read this much patch text per pull request, apart from the judge's budget; one sidecar +// holds every deployment's step outputs, so past it the pull request's later files are listed without their patches. export const MAX_CODE_RULES_PATCH_CHARS = 2_000_000; /** A changed file as the judge reads it: `patchTruncated` when its patch was cut at `MAX_PATCH_INPUT_CHARS`. */ From 2b88640477c078d7cd0664c0ffc9db2bc25a7aad Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Tue, 27 Oct 2026 17:55:37 -0700 Subject: [PATCH 6/9] fix(triage-workflows): treat any GitHub file with no lines changed as nothing to read --- docs/ARCHITECTURE.md | 13 +++++++------ .../triage-workflows/src/logic/checks.test.ts | 8 +++++--- .../triage-workflows/src/logic/facts.test.ts | 16 ++++++++++++++++ packages/triage-workflows/src/logic/patterns.ts | 8 ++++---- 4 files changed, 32 insertions(+), 13 deletions(-) diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index fe09f0b39..0960d4638 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -557,12 +557,13 @@ label off that head, so it never stands on a head not yet triaged. listed without patches. The judge's view of the diff has its own, smaller budget (`judgedFiles` in `logic/quality.ts`). A file without a patch, because the host sent none (GitHub omits it for a very large or binary - file; a file GitHub only renamed has nothing to read and is not listed) or - the pull request passed that ceiling, is listed in the evidence as not - checked, saying which, and the rule runs on the rest; with nothing else to - check, or a file whose content was not read, or a rule ast-grep refuses, - the rule is unconfirmed; `vendor/`, `dist/`, `build/` and `generated/` at - the repository root and `node_modules/` anywhere are skipped and listed as + file; a file whose lines GitHub counts as unchanged, such as a rename or a + mode change, has nothing to read and is not listed) or the pull request + passed that ceiling, is listed in the evidence as not checked, saying + which, and the rule runs on the rest; with nothing else to check, or a file + whose content was not read, or a rule ast-grep refuses, the rule is + unconfirmed; `vendor/`, `dist/`, `build/` and `generated/` at the + repository root and `node_modules/` anywhere are skipped and listed as skipped in the evidence, and a `?? 0` inside arithmetic is a counter's real default. With such a rule on, the facts step reads each file whose patch adds lines at the head through the host's `read_file` (`whole`), ten at a diff --git a/packages/triage-workflows/src/logic/checks.test.ts b/packages/triage-workflows/src/logic/checks.test.ts index 9153652d6..9d947cd5f 100644 --- a/packages/triage-workflows/src/logic/checks.test.ts +++ b/packages/triage-workflows/src/logic/checks.test.ts @@ -446,7 +446,7 @@ rules: }); test("a file without its patch, as on Bitbucket Data Center, an unread content or a broken rule never passes", () => { - const unpatched = { path: "src/a.ts", status: "modified", additions: 0, deletions: 0 }; + const unpatched = { path: "src/a.ts", status: "modified", additions: 3, deletions: 0 }; expect(filesToRead([unpatched], "github")).toEqual({ unread: "src/a.ts: the host sent no diff" }); const past = (path: string) => ({ ...unpatched, path, patchUnread: true as const }); expect(filesToRead([unpatched, past("src/b.ts"), past("src/c.ts")], "github")).toEqual({ unread: "src/a.ts: the host sent no diff; src/b.ts and 1 more: the pull request's diff is larger than Triage reads" }); @@ -470,9 +470,11 @@ rules: expect(passed.checks.find((check) => check.check === "conventions")).toMatchObject({ result: "pass", reason: "no code pattern matched; 1 file was not checked", evidence: [left] }); }); - test("a file GitHub only renamed is nothing to read, while a Data Center move without its patch stays unread", () => { + test("a file GitHub counts no lines of, renamed or with only its mode changed, is nothing to read, while a Data Center move without its patch stays unread", () => { const renamed = { path: "src/moved.ts", previousPath: "src/old.ts", status: "renamed", additions: 0, deletions: 0 }; - expect(filesToRead([renamed, file], "github")).toEqual(["src/a.ts"]); + const mode = { path: "src/run.ts", status: "modified", additions: 0, deletions: 0 }; + expect(filesToRead([renamed, mode, file], "github")).toEqual(["src/a.ts"]); + expect(filesToRead([mode], "github")).toEqual([]); expect(patternsResult(readCheckPack(readPolicyFile(FILE, "acme/widgets")).custom.find(isPatternsCheck)!, [renamed], {}, "github")).toEqual({ matches: [], skipped: [], checked: 1 }); expect(filesToRead([renamed], "bitbucket-datacenter")).toEqual({ unread: "src/moved.ts: the host sent no diff" }); }); diff --git a/packages/triage-workflows/src/logic/facts.test.ts b/packages/triage-workflows/src/logic/facts.test.ts index afbb791f7..b687b66d4 100644 --- a/packages/triage-workflows/src/logic/facts.test.ts +++ b/packages/triage-workflows/src/logic/facts.test.ts @@ -1,6 +1,8 @@ import { describe, expect, test } from "bun:test"; import { DEFAULT_REPO_POLICY, emptyPack, isPatternsCheck, readCheckPack, readPolicyFile } from "@corbits/triage-contracts"; import type { IssueComment, Review } from "@corbits/github-reads"; +import type { EffectContext } from "@intx/workflow"; +import { rules } from "../actions/rules.ts"; import type { PrFileFacts } from "./checks.ts"; import { addFilesPage, assembleItem, buildFacts, FIRST_FILES_PAGE, type FilesCursor, type FilesPage, type ListedFiles } from "./facts.ts"; import { NO_FACTS } from "./item.ts"; @@ -160,6 +162,20 @@ describe("a diff past the code rules' ceiling", () => { expect(readPages([file])).toEqual([file]); }); + test("keeps its marks through the step output, which the rules step reads as JSON rather than through fileFacts", async () => { + const filler = `+const filler = "${"x".repeat(1_000)}";`; + const big: PrFileFacts = { path: "src/big.ts", status: "added", additions: 2_000, deletions: 0, patch: ["@@ -0,0 +1,2000 @@", ...Array.from({ length: 2_000 }, () => filler)].join("\n") }; + const listed = readPages([big, { path: "src/a.ts", status: "added", additions: 1, deletions: 0, patch: "@@ -0,0 +1 @@\n+const a = 1;" }]); + const contents = { "src/big.ts": `${Array.from({ length: 2_000 }, () => filler.slice(1)).join("\n")}\n` }; + const facts = { ...NO_FACTS, repo: "acme/widgets", number: 8, state: "open" as const, files: listed, patterns: { conventions: patternsResult(pack.custom.find(isPatternsCheck)!, listed, contents, "github") } }; + const output = JSON.parse(JSON.stringify({ items: [{ facts, pack, automatic: [] }], batch: false })); + const ruled = await rules(output, {} as EffectContext, new AbortController().signal); + const item = ruled.items[0]!; + if (!("det" in item)) throw new Error("the item was not ruled"); + expect(item.facts.files[1]).toMatchObject({ path: "src/a.ts", patchUnread: true }); + expect(item.det.checks.find((check) => check.check === "conventions")).toMatchObject({ result: "pass", evidence: ["src/a.ts: not checked, the pull request's diff is larger than Triage reads"] }); + }); + test("keeps no lockfile patch, so a lockfile neither counts nor rides in the output, and marks nothing when no patches were asked for", () => { const lock = { path: "bun.lock", status: "modified", additions: 1, deletions: 0, patch: `@@ -1 +1 @@\n+${"x".repeat(MAX_CODE_RULES_PATCH_CHARS)}` }; const { patch: _patch, ...lockMeta } = lock; diff --git a/packages/triage-workflows/src/logic/patterns.ts b/packages/triage-workflows/src/logic/patterns.ts index b02c1c176..aa6c13cab 100644 --- a/packages/triage-workflows/src/logic/patterns.ts +++ b/packages/triage-workflows/src/logic/patterns.ts @@ -52,12 +52,12 @@ function inScope(file: PrFileFacts): boolean { * and none is read past the code rules' ceiling. */ function unpatched(file: PrFileFacts, source: RepoSource): boolean { - return inScope(file) && file.patch === undefined && !renamedOnly(file, source); + return inScope(file) && file.patch === undefined && !linesUnchanged(file, source); } -/** GitHub sends no patch for a file it only renamed, so there is nothing to read; Data Center counts no lines to tell. */ -function renamedOnly(file: PrFileFacts, source: RepoSource): boolean { - return source === "github" && file.status === "renamed" && file.additions === 0 && file.deletions === 0; +/** GitHub sends no patch for a file whose lines did not change, such as a rename or a mode change; Data Center counts no lines to tell. */ +function linesUnchanged(file: PrFileFacts, source: RepoSource): boolean { + return source === "github" && file.additions === 0 && file.deletions === 0; } function whyUnpatched(file: PrFileFacts): string { From a830127c8254e02ca42af398800c9f1608aba63a Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Tue, 27 Oct 2026 18:49:11 -0700 Subject: [PATCH 7/9] fix(triage-workflows): leave code patterns unconfirmed when a file that adds code was not checked A match in a checked file still fails the rule; otherwise a file in scope that adds code but has no patch leaves it unconfirmed, naming the files, so the rule passes only when every such file was checked. --- docs/ARCHITECTURE.md | 33 +++++++++---------- .../Corbits Triage User Manual.md | 2 +- .../triage-workflows/src/logic/checks.test.ts | 11 ++++--- packages/triage-workflows/src/logic/checks.ts | 4 +-- .../triage-workflows/src/logic/facts.test.ts | 11 ++----- .../triage-workflows/src/logic/patterns.ts | 26 +++++++-------- 6 files changed, 40 insertions(+), 47 deletions(-) diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 0960d4638..41fc44405 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -555,24 +555,23 @@ label off that head, so it never stands on a head not yet triaged. lockfiles and other generated files are listed without their patch, which no rule reads. The file that passes it partway and every later one are listed without patches. The judge's view of the diff has its own, smaller - budget (`judgedFiles` in `logic/quality.ts`). A file without a patch, + budget (`judgedFiles` in `logic/quality.ts`). A match in any checked file + fails the rule. Without one, a file that adds code but has no patch, because the host sent none (GitHub omits it for a very large or binary - file; a file whose lines GitHub counts as unchanged, such as a rename or a - mode change, has nothing to read and is not listed) or the pull request - passed that ceiling, is listed in the evidence as not checked, saying - which, and the rule runs on the rest; with nothing else to check, or a file - whose content was not read, or a rule ast-grep refuses, the rule is - unconfirmed; `vendor/`, `dist/`, `build/` and `generated/` at the - repository root and `node_modules/` anywhere are skipped and listed as - skipped in the evidence, and a `?? 0` inside arithmetic is a counter's real - default. With such a rule on, the facts step reads each file whose patch - adds lines at the head through the host's `read_file` (`whole`), ten at a - time and at most 300 per pull request (more leaves the rule unconfirmed - without reading any), runs the rules there and outputs only the matches. - Bitbucket Data Center lists no patch, so there the rule is unconfirmed - whenever an in-scope TypeScript or JavaScript file is added, edited, moved - or copied. The extension leaves code-pattern rules out and swaps the native - module for a stub. + file) or the pull request passed that ceiling, leaves the rule unconfirmed, + naming the files and why; a file GitHub counts no added lines in, such as a + rename or a mode change, has nothing to check. A file whose content was not + read, or a rule ast-grep refuses, leaves the rule unconfirmed. `vendor/`, + `dist/`, `build/` and `generated/` at the repository root and + `node_modules/` anywhere are skipped and listed as skipped in the evidence, + and a `?? 0` inside arithmetic is a counter's real default. With such a + rule on, the facts step reads each file whose patch adds lines at the head + through the host's `read_file` (`whole`), ten at a time and at most 300 per + pull request (more leaves the rule unconfirmed without reading any), runs + the rules there and outputs only the matches. Bitbucket Data Center lists + no patch, so there the rule is unconfirmed whenever an in-scope TypeScript + or JavaScript file is added, edited, moved or copied. The extension leaves + code-pattern rules out and swaps the native module for a stub. 3. **route**: skips the judge when no question needs asking. 4. **judge**: one System One call per request of questions. Each first-round request and the verdict request also asks whether the pull request's text, diff or docs try to direct the judge; a diff --git a/docs/Corbits Triage User Manual/Corbits Triage User Manual.md b/docs/Corbits Triage User Manual/Corbits Triage User Manual.md index 5148aaac5..df755299b 100644 --- a/docs/Corbits Triage User Manual/Corbits Triage User Manual.md +++ b/docs/Corbits Triage User Manual/Corbits Triage User Manual.md @@ -258,7 +258,7 @@ A rule asks one question about a pull request and answers pass, fail or unconfir - **Flag changes to sensitive paths.** Checks changed files against the file's named groups of globs, each `high` or `medium`, written as `groups:`, such as `infra: { level: high, paths: ["infra/**", "**/*.tf"] }`. Without `groups` there are four: `schema` (`**/migrations/**`, `**/*.sql`) and `auth` (`**/auth/**`, `**/secrets/**`), which are high, and `ci` (`.github/**`, `**/Dockerfile`, `**/railway.json`, `**/railway.toml`) and `locks` (`**/bun.lock`, `**/package-lock.json`, `**/yarn.lock`, `**/pnpm-lock.yaml`), which are medium, since every dependency bump changes a lockfile. Globs read as in **Never accept changes to**. A change to a group at `failAt` or above, high unless the file says, fails the rule, which asks a maintainer unless the file gives it another `onFail`. Below that, from `labelAt`, medium unless the file says, it passes but labels the pull request `sensitive: medium` and notes it under **Notes:** (on Bitbucket Data Center, only the note). Its evidence names each group with its files, at most five, such as `schema: db/migrations/0004.sql`, then how many more. Regression risk keeps its own migration globs. - **Flag high-risk pull requests and label medium-risk ones.** Rates each pull request low, medium or high risk; see [Risk](#risk). - **Fail when …**, a text rule of your own, such as "Fail when any commit message contains "Co-Authored-By:"". It names where to look (the title, the description, any commit message, or several of them), whether the rule fails when the text **contains** or **does not contain** its phrases, and the phrases. Case does not matter. A **contains** rule on the description also looks at its hidden parts, such as HTML comments and the addresses behind links, while **does not contain** reads only what a reader sees. Only the first 100 commits are read. When a **contains** rule fails, Triage names each place and the phrases found there, asks for them to be removed, and quotes each matching line once. Text rules count toward the eight rules of your own. - - **Fail when added code matches …**, a code-pattern rule of your own, such as one that flags `?? 0`. It checks only the lines the pull request adds to TypeScript and JavaScript files (`.ts`, `.tsx`, `.mts`, `.cts`, `.js`, `.jsx`, `.mjs`, `.cjs`), so code already on the branch never fails it. On any other language it passes with **nothing checked: no TypeScript or JavaScript code added**, so don't add one to a repository written in Go, Python or similar. Turn on the shipped `convention` preset, which flags invented fallbacks (`?? 0`, `?? []`), nested ternaries, functions called where they are written, `.then`/`.catch` chains, empty `catch` blocks and lint or type suppression comments in both languages, or write your own patterns in [ast-grep](https://ast-grep.github.io/guide/rule-config.html)'s rule format. Your own pattern replaces a preset's pattern with the same `id` in both languages; to keep checking JavaScript, add a `language: javascript` pattern under the same id. A pattern takes ast-grep's own `files` and `ignores` lists to check only some paths or skip some. They follow ast-grep's glob rules: paths are from the repository root, `*` also matches across folders, and dotfiles are matched like any other file. Write each glob as a string; the `{ glob, caseInsensitive }` form is not read. Write a negated class as `[^a]`; a glob with `[!a]`, or one that isn't valid, refuses the policy. The preset's `?? 0` and `?? []` patterns ignore test files (`**/*.test.ts`, `**/*.test.tsx`, `**/*.test.js`, `**/*.test.jsx`, `**/*.test.mjs`, `**/*.test.cjs`). Each match is listed as evidence, as `path:line` and the pattern's message. Vendored and built code at the repository root (`vendor/`, `dist/`, `build/`, `generated/`) and installed code (`node_modules/` anywhere) is skipped, and each skipped file is listed in the evidence. The same folders deeper in the repository, such as `src/build/`, are checked. When the host sends no changed lines for a file, as GitHub does for a very large or binary one, or the pull request's diff is larger than the 2,000,000 characters Triage reads, those files are listed in the evidence as not checked, with the reason, and every other file is checked; if no other file adds code, the rule is unconfirmed. When a file's content can't be read or a pattern is not valid ast-grep, the rule is unconfirmed rather than passing. Triage reads at most 300 files per pull request, so on one that adds code to more, the rule is unconfirmed with a reason such as **code patterns could not be checked: 412 files with added code, more than the 300 read per pull request**. On Bitbucket Data Center, which does not give Triage each file's changed lines, it is unconfirmed whenever a TypeScript or JavaScript file of your own is added, edited, moved or copied. To give some patterns a different `onFail`, put them in a rule of their own. Code-pattern rules count toward the eight rules of your own, and the browser extension leaves them out. + - **Fail when added code matches …**, a code-pattern rule of your own, such as one that flags `?? 0`. It checks only the lines the pull request adds to TypeScript and JavaScript files (`.ts`, `.tsx`, `.mts`, `.cts`, `.js`, `.jsx`, `.mjs`, `.cjs`), so code already on the branch never fails it. On any other language it passes with **nothing checked: no TypeScript or JavaScript code added**, so don't add one to a repository written in Go, Python or similar. Turn on the shipped `convention` preset, which flags invented fallbacks (`?? 0`, `?? []`), nested ternaries, functions called where they are written, `.then`/`.catch` chains, empty `catch` blocks and lint or type suppression comments in both languages, or write your own patterns in [ast-grep](https://ast-grep.github.io/guide/rule-config.html)'s rule format. Your own pattern replaces a preset's pattern with the same `id` in both languages; to keep checking JavaScript, add a `language: javascript` pattern under the same id. A pattern takes ast-grep's own `files` and `ignores` lists to check only some paths or skip some. They follow ast-grep's glob rules: paths are from the repository root, `*` also matches across folders, and dotfiles are matched like any other file. Write each glob as a string; the `{ glob, caseInsensitive }` form is not read. Write a negated class as `[^a]`; a glob with `[!a]`, or one that isn't valid, refuses the policy. The preset's `?? 0` and `?? []` patterns ignore test files (`**/*.test.ts`, `**/*.test.tsx`, `**/*.test.js`, `**/*.test.jsx`, `**/*.test.mjs`, `**/*.test.cjs`). Each match is listed as evidence, as `path:line` and the pattern's message. Vendored and built code at the repository root (`vendor/`, `dist/`, `build/`, `generated/`) and installed code (`node_modules/` anywhere) is skipped, and each skipped file is listed in the evidence. The same folders deeper in the repository, such as `src/build/`, are checked. When the host sends no changed lines for a file that adds code, as GitHub does for a very large or binary one, or the pull request's diff is larger than the 2,000,000 characters Triage reads, every other file is still checked: a match fails the rule, and otherwise it is unconfirmed, naming those files and why. When a file's content can't be read or a pattern is not valid ast-grep, the rule is unconfirmed rather than passing. Triage reads at most 300 files per pull request, so on one that adds code to more, the rule is unconfirmed with a reason such as **code patterns could not be checked: 412 files with added code, more than the 300 read per pull request**. On Bitbucket Data Center, which does not give Triage each file's changed lines, it is unconfirmed whenever a TypeScript or JavaScript file of your own is added, edited, moved or copied. To give some patterns a different `onFail`, put them in a rule of their own. Code-pattern rules count toward the eight rules of your own, and the browser extension leaves them out. - **Judgement**, questions the decision model answers from the diff and the documentation: - **Check the change is focused on one thing.** The decision model is asked whether the pull request does one thing. When it fails, the verdict comment says it mixes unrelated changes and asks the author to split them out, unless the file sets another `onFail`. A pull request based on another open pull request's branch is checked on its own changes, and the comment says so, such as "Stacked on #1573: only this pull request's own changes were checked." On a branch another pull request was merged into, this rule is skipped, and the comment says why, such as "stacked branch, includes #1521". - **Check docs are updated when behavior changes.** Changing a documentation file passes it. Otherwise the decision model is asked, file by file, whether the docs stay correct, a statement in them is now wrong, or the change adds setup the docs must describe. When it fails, the verdict comment asks the author to update the documentation and names the files, unless the file sets another `onFail`. The default label, when `onFail` is `label`, is `docs: check`. diff --git a/packages/triage-workflows/src/logic/checks.test.ts b/packages/triage-workflows/src/logic/checks.test.ts index 9d947cd5f..bda08afca 100644 --- a/packages/triage-workflows/src/logic/checks.test.ts +++ b/packages/triage-workflows/src/logic/checks.test.ts @@ -459,21 +459,22 @@ rules: expect(patterns([file], { "src/a.ts": content }, broken).checks.find((check) => check.check === "broken")).toMatchObject({ result: "unconfirmed" }); }); - test("a file the host sent no diff for is named as not checked, and every other file is checked", () => { + test("a match in a checked file fails, and without one a file that adds code the host sent no diff for leaves the rule unconfirmed", () => { const large = { path: "src/large.ts", status: "modified", additions: 9_000, deletions: 0 }; const clean = { path: "src/b.ts", status: "added", additions: 1, deletions: 0, patch: "@@ -0,0 +1 @@\n+const b = 1;" }; const left = "src/large.ts: not checked, the host sent no diff"; expect(filesToRead([file, large, clean], "github")).toEqual(["src/a.ts", "src/b.ts"]); const failed = patterns([file, large], { "src/a.ts": content }); expect(failed.checks.find((check) => check.check === "conventions")).toMatchObject({ result: "fail", evidence: expect.arrayContaining([left]), skipped: [left] }); - const passed = patterns([clean, large], { "src/b.ts": "const b = 1;\n" }); - expect(passed.checks.find((check) => check.check === "conventions")).toMatchObject({ result: "pass", reason: "no code pattern matched; 1 file was not checked", evidence: [left] }); + const unchecked = patterns([clean, large], { "src/b.ts": "const b = 1;\n" }); + expect(unchecked.checks.find((check) => check.check === "conventions")).toMatchObject({ result: "unconfirmed", reason: "code patterns could not be checked: src/large.ts: the host sent no diff" }); }); - test("a file GitHub counts no lines of, renamed or with only its mode changed, is nothing to read, while a Data Center move without its patch stays unread", () => { + test("a file GitHub counts no added lines in, renamed, with only its mode changed or only lines removed, is nothing to read, while a Data Center move without its patch stays unread", () => { const renamed = { path: "src/moved.ts", previousPath: "src/old.ts", status: "renamed", additions: 0, deletions: 0 }; const mode = { path: "src/run.ts", status: "modified", additions: 0, deletions: 0 }; - expect(filesToRead([renamed, mode, file], "github")).toEqual(["src/a.ts"]); + const removed = { path: "src/trimmed.ts", status: "modified", additions: 0, deletions: 40 }; + expect(filesToRead([renamed, mode, removed, file], "github")).toEqual(["src/a.ts"]); expect(filesToRead([mode], "github")).toEqual([]); expect(patternsResult(readCheckPack(readPolicyFile(FILE, "acme/widgets")).custom.find(isPatternsCheck)!, [renamed], {}, "github")).toEqual({ matches: [], skipped: [], checked: 1 }); expect(filesToRead([renamed], "bitbucket-datacenter")).toEqual({ unread: "src/moved.ts: the host sent no diff" }); diff --git a/packages/triage-workflows/src/logic/checks.ts b/packages/triage-workflows/src/logic/checks.ts index 999d77ffe..d6f6de7ec 100644 --- a/packages/triage-workflows/src/logic/checks.ts +++ b/packages/triage-workflows/src/logic/checks.ts @@ -642,10 +642,8 @@ function riskResult(facts: PrFacts, rules: CheckRules): CheckResult { return { ...base, result: "pass", ...(outcome === "label" && level !== "low" && { label: levelLabel("risk", level) }) }; } -function patternsReason(failed: boolean, found: { skipped: string[]; checked: number; unpatched?: number }): string { +function patternsReason(failed: boolean, found: { skipped: string[]; checked: number }): string { if (failed) return "added code matches a code pattern"; - if (found.unpatched === 1) return "no code pattern matched; 1 file was not checked"; - if (found.unpatched) return `no code pattern matched; ${found.unpatched} files were not checked`; if (found.checked === 0 && found.skipped.length > 0) return "nothing checked: the only TypeScript or JavaScript changed is vendored, built or installed code"; if (found.checked === 0) return "nothing checked: no TypeScript or JavaScript code added"; return "no code pattern matched"; diff --git a/packages/triage-workflows/src/logic/facts.test.ts b/packages/triage-workflows/src/logic/facts.test.ts index b687b66d4..8ddc6e53a 100644 --- a/packages/triage-workflows/src/logic/facts.test.ts +++ b/packages/triage-workflows/src/logic/facts.test.ts @@ -139,16 +139,11 @@ describe("a diff past the code rules' ceiling", () => { const unread = listed.slice(whole); expect(unread).toEqual(hostFiles.slice(whole).map(({ patch: _patch, ...meta }) => ({ ...meta, patchUnread: true }))); const contents = Object.fromEntries(listed.slice(0, whole).map((file) => [file.path, `${Array.from({ length: 200 }, () => line).join("\n")}\n`])); - expect(patternsResult(pack.custom.find(isPatternsCheck)!, listed, contents, "github")).toEqual({ - matches: [], - skipped: unread.map((file) => `${file.path}: not checked, the pull request's diff is larger than Triage reads`), - checked: whole, - unpatched: hostFiles.length - whole, - }); + expect(patternsResult(pack.custom.find(isPatternsCheck)!, listed, contents, "github")).toEqual({ unread: `${unread[0]!.path} and ${unread.length - 1} more: the pull request's diff is larger than Triage reads` }); return patch.length; } - test("keeps the file whose last page passes it, and lists the later files without patches for the pattern rules to name", () => { + test("keeps the file whose last page passes it, and lists the later files without patches, which leaves the pattern rules unconfirmed naming them", () => { const chars = ceilingCase(1_000, 10); expect(chars * 9.5).toBeLessThan(MAX_CODE_RULES_PATCH_CHARS); expect(chars * 10).toBeGreaterThan(MAX_CODE_RULES_PATCH_CHARS); @@ -173,7 +168,7 @@ describe("a diff past the code rules' ceiling", () => { const item = ruled.items[0]!; if (!("det" in item)) throw new Error("the item was not ruled"); expect(item.facts.files[1]).toMatchObject({ path: "src/a.ts", patchUnread: true }); - expect(item.det.checks.find((check) => check.check === "conventions")).toMatchObject({ result: "pass", evidence: ["src/a.ts: not checked, the pull request's diff is larger than Triage reads"] }); + expect(item.det.checks.find((check) => check.check === "conventions")).toMatchObject({ result: "unconfirmed", reason: "code patterns could not be checked: src/a.ts: the pull request's diff is larger than Triage reads" }); }); test("keeps no lockfile patch, so a lockfile neither counts nor rides in the output, and marks nothing when no patches were asked for", () => { diff --git a/packages/triage-workflows/src/logic/patterns.ts b/packages/triage-workflows/src/logic/patterns.ts index aa6c13cab..ae0ebc728 100644 --- a/packages/triage-workflows/src/logic/patterns.ts +++ b/packages/triage-workflows/src/logic/patterns.ts @@ -18,8 +18,7 @@ const GRAMMARS: Record = { jsx: JAVASCRIPT, }; -/** `unpatched` counts the files in scope left out because they have no patch. */ -export type PatternsResult = { matches: string[]; skipped: string[]; checked: number; unpatched?: number } | { unread: string }; +export type PatternsResult = { matches: string[]; skipped: string[]; checked: number } | { unread: string }; function presetRules(id: string): readonly PatternRule[] { const rules = PATTERN_PRESETS[id]; @@ -48,16 +47,16 @@ function inScope(file: PrFileFacts): boolean { } /** - * A file in scope without a patch: GitHub sends none for a very large or binary file, Bitbucket Data Center for any, - * and none is read past the code rules' ceiling. + * A file in scope that may add code but has no patch: GitHub sends none for a very large or binary file, Bitbucket + * Data Center for any, and none is read past the code rules' ceiling. */ function unpatched(file: PrFileFacts, source: RepoSource): boolean { - return inScope(file) && file.patch === undefined && !linesUnchanged(file, source); + return inScope(file) && file.patch === undefined && !addsNothing(file, source); } -/** GitHub sends no patch for a file whose lines did not change, such as a rename or a mode change; Data Center counts no lines to tell. */ -function linesUnchanged(file: PrFileFacts, source: RepoSource): boolean { - return source === "github" && file.additions === 0 && file.deletions === 0; +/** GitHub counts the lines a file adds, so one it adds none to, such as a rename or a mode change, has nothing to check; Data Center counts none to tell. */ +function addsNothing(file: PrFileFacts, source: RepoSource): boolean { + return source === "github" && file.additions === 0; } function whyUnpatched(file: PrFileFacts): string { @@ -81,8 +80,8 @@ function readPath(file: PrFileFacts): string { export const MAX_CONTENT_READS = 300; /** - * The files whose content the rules need, or why they cannot be checked: files in scope came without a patch and none - * other adds code, or more files than are read. + * The files whose content the rules need, or why they cannot be checked: files in scope have no patch and no other + * file adds code, or more files than are read. */ export function filesToRead(files: readonly PrFileFacts[], source: RepoSource): string[] | { unread: string } { const toRead = files.filter((file) => inScope(file) && file.patch !== undefined && addedLines(file.patch).size > 0).map(readPath); @@ -110,8 +109,8 @@ function fileMatches(file: PrFileFacts, patch: string, content: string, grammar: } /** - * Matches on the lines a pull request adds; a file in scope whose content was not read leaves the rule unchecked, and - * one without a patch is named as not checked. + * Matches on the lines a pull request adds. Without a match, a file in scope without its patch or content leaves the + * rule unchecked, naming the files, since a pass needs every file that adds code checked. */ export function patternsResult(check: PatternsCheck, files: readonly PrFileFacts[], contents: Readonly>, source: RepoSource): PatternsResult { const toRead = filesToRead(files, source); @@ -128,9 +127,10 @@ export function patternsResult(check: PatternsCheck, files: readonly PrFileFacts return { unread: error instanceof Error ? error.message : String(error) }; } const missing = files.filter((file) => unpatched(file, source)); + if (matches.length === 0 && missing.length > 0) return { unread: namedUnpatched(missing) }; const skipped = [ ...missing.map((file) => `${file.path}: not checked, ${whyUnpatched(file)}`), ...files.filter((file) => checkable(file) && isForeignPath(file.path)).map((file) => `${file.path}: skipped as vendored, built or installed code`), ]; - return { matches, skipped, checked: files.filter(inScope).length - missing.length, ...(missing.length > 0 && { unpatched: missing.length }) }; + return { matches, skipped, checked: files.filter(inScope).length - missing.length }; } From b6f2d8ed9c1b6941410deef8f964d92b73b79d58 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Tue, 27 Oct 2026 18:52:38 -0700 Subject: [PATCH 8/9] fix(triage-workflows): keep a code-pattern match when another file's content was not read --- docs/ARCHITECTURE.md | 8 ++--- .../Corbits Triage User Manual.md | 2 +- .../triage-workflows/src/logic/checks.test.ts | 9 ++++++ .../triage-workflows/src/logic/patterns.ts | 32 +++++++++++-------- 4 files changed, 32 insertions(+), 19 deletions(-) diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 41fc44405..87da68338 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -558,10 +558,10 @@ label off that head, so it never stands on a head not yet triaged. budget (`judgedFiles` in `logic/quality.ts`). A match in any checked file fails the rule. Without one, a file that adds code but has no patch, because the host sent none (GitHub omits it for a very large or binary - file) or the pull request passed that ceiling, leaves the rule unconfirmed, - naming the files and why; a file GitHub counts no added lines in, such as a - rename or a mode change, has nothing to check. A file whose content was not - read, or a rule ast-grep refuses, leaves the rule unconfirmed. `vendor/`, + file) or the pull request passed that ceiling, or whose content was not + read, leaves the rule unconfirmed, naming the files and why; a file GitHub + counts no added lines in, such as a rename or a mode change, has nothing to + check. A rule ast-grep refuses leaves the rule unconfirmed. `vendor/`, `dist/`, `build/` and `generated/` at the repository root and `node_modules/` anywhere are skipped and listed as skipped in the evidence, and a `?? 0` inside arithmetic is a counter's real default. With such a diff --git a/docs/Corbits Triage User Manual/Corbits Triage User Manual.md b/docs/Corbits Triage User Manual/Corbits Triage User Manual.md index df755299b..7d6f499b8 100644 --- a/docs/Corbits Triage User Manual/Corbits Triage User Manual.md +++ b/docs/Corbits Triage User Manual/Corbits Triage User Manual.md @@ -258,7 +258,7 @@ A rule asks one question about a pull request and answers pass, fail or unconfir - **Flag changes to sensitive paths.** Checks changed files against the file's named groups of globs, each `high` or `medium`, written as `groups:`, such as `infra: { level: high, paths: ["infra/**", "**/*.tf"] }`. Without `groups` there are four: `schema` (`**/migrations/**`, `**/*.sql`) and `auth` (`**/auth/**`, `**/secrets/**`), which are high, and `ci` (`.github/**`, `**/Dockerfile`, `**/railway.json`, `**/railway.toml`) and `locks` (`**/bun.lock`, `**/package-lock.json`, `**/yarn.lock`, `**/pnpm-lock.yaml`), which are medium, since every dependency bump changes a lockfile. Globs read as in **Never accept changes to**. A change to a group at `failAt` or above, high unless the file says, fails the rule, which asks a maintainer unless the file gives it another `onFail`. Below that, from `labelAt`, medium unless the file says, it passes but labels the pull request `sensitive: medium` and notes it under **Notes:** (on Bitbucket Data Center, only the note). Its evidence names each group with its files, at most five, such as `schema: db/migrations/0004.sql`, then how many more. Regression risk keeps its own migration globs. - **Flag high-risk pull requests and label medium-risk ones.** Rates each pull request low, medium or high risk; see [Risk](#risk). - **Fail when …**, a text rule of your own, such as "Fail when any commit message contains "Co-Authored-By:"". It names where to look (the title, the description, any commit message, or several of them), whether the rule fails when the text **contains** or **does not contain** its phrases, and the phrases. Case does not matter. A **contains** rule on the description also looks at its hidden parts, such as HTML comments and the addresses behind links, while **does not contain** reads only what a reader sees. Only the first 100 commits are read. When a **contains** rule fails, Triage names each place and the phrases found there, asks for them to be removed, and quotes each matching line once. Text rules count toward the eight rules of your own. - - **Fail when added code matches …**, a code-pattern rule of your own, such as one that flags `?? 0`. It checks only the lines the pull request adds to TypeScript and JavaScript files (`.ts`, `.tsx`, `.mts`, `.cts`, `.js`, `.jsx`, `.mjs`, `.cjs`), so code already on the branch never fails it. On any other language it passes with **nothing checked: no TypeScript or JavaScript code added**, so don't add one to a repository written in Go, Python or similar. Turn on the shipped `convention` preset, which flags invented fallbacks (`?? 0`, `?? []`), nested ternaries, functions called where they are written, `.then`/`.catch` chains, empty `catch` blocks and lint or type suppression comments in both languages, or write your own patterns in [ast-grep](https://ast-grep.github.io/guide/rule-config.html)'s rule format. Your own pattern replaces a preset's pattern with the same `id` in both languages; to keep checking JavaScript, add a `language: javascript` pattern under the same id. A pattern takes ast-grep's own `files` and `ignores` lists to check only some paths or skip some. They follow ast-grep's glob rules: paths are from the repository root, `*` also matches across folders, and dotfiles are matched like any other file. Write each glob as a string; the `{ glob, caseInsensitive }` form is not read. Write a negated class as `[^a]`; a glob with `[!a]`, or one that isn't valid, refuses the policy. The preset's `?? 0` and `?? []` patterns ignore test files (`**/*.test.ts`, `**/*.test.tsx`, `**/*.test.js`, `**/*.test.jsx`, `**/*.test.mjs`, `**/*.test.cjs`). Each match is listed as evidence, as `path:line` and the pattern's message. Vendored and built code at the repository root (`vendor/`, `dist/`, `build/`, `generated/`) and installed code (`node_modules/` anywhere) is skipped, and each skipped file is listed in the evidence. The same folders deeper in the repository, such as `src/build/`, are checked. When the host sends no changed lines for a file that adds code, as GitHub does for a very large or binary one, or the pull request's diff is larger than the 2,000,000 characters Triage reads, every other file is still checked: a match fails the rule, and otherwise it is unconfirmed, naming those files and why. When a file's content can't be read or a pattern is not valid ast-grep, the rule is unconfirmed rather than passing. Triage reads at most 300 files per pull request, so on one that adds code to more, the rule is unconfirmed with a reason such as **code patterns could not be checked: 412 files with added code, more than the 300 read per pull request**. On Bitbucket Data Center, which does not give Triage each file's changed lines, it is unconfirmed whenever a TypeScript or JavaScript file of your own is added, edited, moved or copied. To give some patterns a different `onFail`, put them in a rule of their own. Code-pattern rules count toward the eight rules of your own, and the browser extension leaves them out. + - **Fail when added code matches …**, a code-pattern rule of your own, such as one that flags `?? 0`. It checks only the lines the pull request adds to TypeScript and JavaScript files (`.ts`, `.tsx`, `.mts`, `.cts`, `.js`, `.jsx`, `.mjs`, `.cjs`), so code already on the branch never fails it. On any other language it passes with **nothing checked: no TypeScript or JavaScript code added**, so don't add one to a repository written in Go, Python or similar. Turn on the shipped `convention` preset, which flags invented fallbacks (`?? 0`, `?? []`), nested ternaries, functions called where they are written, `.then`/`.catch` chains, empty `catch` blocks and lint or type suppression comments in both languages, or write your own patterns in [ast-grep](https://ast-grep.github.io/guide/rule-config.html)'s rule format. Your own pattern replaces a preset's pattern with the same `id` in both languages; to keep checking JavaScript, add a `language: javascript` pattern under the same id. A pattern takes ast-grep's own `files` and `ignores` lists to check only some paths or skip some. They follow ast-grep's glob rules: paths are from the repository root, `*` also matches across folders, and dotfiles are matched like any other file. Write each glob as a string; the `{ glob, caseInsensitive }` form is not read. Write a negated class as `[^a]`; a glob with `[!a]`, or one that isn't valid, refuses the policy. The preset's `?? 0` and `?? []` patterns ignore test files (`**/*.test.ts`, `**/*.test.tsx`, `**/*.test.js`, `**/*.test.jsx`, `**/*.test.mjs`, `**/*.test.cjs`). Each match is listed as evidence, as `path:line` and the pattern's message. Vendored and built code at the repository root (`vendor/`, `dist/`, `build/`, `generated/`) and installed code (`node_modules/` anywhere) is skipped, and each skipped file is listed in the evidence. The same folders deeper in the repository, such as `src/build/`, are checked. When the host sends no changed lines for a file that adds code, as GitHub does for a very large or binary one, the pull request's diff is larger than the 2,000,000 characters Triage reads, or a file's content can't be read, every other file is still checked: a match fails the rule, and otherwise it is unconfirmed, naming those files and why. When a pattern is not valid ast-grep, the rule is unconfirmed rather than passing. Triage reads at most 300 files per pull request, so on one that adds code to more, the rule is unconfirmed with a reason such as **code patterns could not be checked: 412 files with added code, more than the 300 read per pull request**. On Bitbucket Data Center, which does not give Triage each file's changed lines, it is unconfirmed whenever a TypeScript or JavaScript file of your own is added, edited, moved or copied. To give some patterns a different `onFail`, put them in a rule of their own. Code-pattern rules count toward the eight rules of your own, and the browser extension leaves them out. - **Judgement**, questions the decision model answers from the diff and the documentation: - **Check the change is focused on one thing.** The decision model is asked whether the pull request does one thing. When it fails, the verdict comment says it mixes unrelated changes and asks the author to split them out, unless the file sets another `onFail`. A pull request based on another open pull request's branch is checked on its own changes, and the comment says so, such as "Stacked on #1573: only this pull request's own changes were checked." On a branch another pull request was merged into, this rule is skipped, and the comment says why, such as "stacked branch, includes #1521". - **Check docs are updated when behavior changes.** Changing a documentation file passes it. Otherwise the decision model is asked, file by file, whether the docs stay correct, a statement in them is now wrong, or the change adds setup the docs must describe. When it fails, the verdict comment asks the author to update the documentation and names the files, unless the file sets another `onFail`. The default label, when `onFail` is `label`, is `docs: check`. diff --git a/packages/triage-workflows/src/logic/checks.test.ts b/packages/triage-workflows/src/logic/checks.test.ts index bda08afca..7d2fa3ac5 100644 --- a/packages/triage-workflows/src/logic/checks.test.ts +++ b/packages/triage-workflows/src/logic/checks.test.ts @@ -480,6 +480,15 @@ rules: expect(filesToRead([renamed], "bitbucket-datacenter")).toEqual({ unread: "src/moved.ts: the host sent no diff" }); }); + test("a match fails the rule even when another file's content was not read, in either order", () => { + const added = { path: "src/b.ts", status: "added", additions: 1, deletions: 0, patch: "@@ -0,0 +1 @@\n+const b = x ?? 0;" }; + const left = "src/a.ts: not checked, its content was not read"; + for (const files of [[file, added], [added, file]]) { + const result = patterns(files, { "src/b.ts": "const b = x ?? 0;\n" }); + expect(result.checks.find((check) => check.check === "conventions")).toMatchObject({ result: "fail", evidence: expect.arrayContaining([left]) }); + } + }); + test("an unknown preset is refused", () => { expect(() => readPolicyFile(FILE.replace("[convention]", "[nope]"), "acme/widgets")).toThrow(); }); diff --git a/packages/triage-workflows/src/logic/patterns.ts b/packages/triage-workflows/src/logic/patterns.ts index ae0ebc728..417dfae08 100644 --- a/packages/triage-workflows/src/logic/patterns.ts +++ b/packages/triage-workflows/src/logic/patterns.ts @@ -59,13 +59,16 @@ function addsNothing(file: PrFileFacts, source: RepoSource): boolean { return source === "github" && file.additions === 0; } -function whyUnpatched(file: PrFileFacts): string { - return file.patchUnread ? "the pull request's diff is larger than Triage reads" : "the host sent no diff"; +/** A file in scope the rules could not check, and why. */ +type Unchecked = { path: string; why: string }; + +function withoutPatch(file: PrFileFacts): Unchecked { + return { path: file.path, why: file.patchUnread ? "the pull request's diff is larger than Triage reads" : "the host sent no diff" }; } -/** The files without a patch, grouped by why: the first of each group named, the rest counted. */ -function namedUnpatched(files: readonly PrFileFacts[]): string { - const groups = Map.groupBy(files, whyUnpatched); +/** The files not checked, grouped by why: the first of each group named, the rest counted. */ +function namedUnchecked(unchecked: readonly Unchecked[]): string { + const groups = Map.groupBy(unchecked, (file) => file.why); return [...groups].map(function group([why, grouped]) { const more = grouped.length > 1 ? ` and ${grouped.length - 1} more` : ""; return `${grouped[0]!.path}${more}: ${why}`; @@ -86,7 +89,7 @@ export const MAX_CONTENT_READS = 300; export function filesToRead(files: readonly PrFileFacts[], source: RepoSource): string[] | { unread: string } { const toRead = files.filter((file) => inScope(file) && file.patch !== undefined && addedLines(file.patch).size > 0).map(readPath); const missing = files.filter((file) => unpatched(file, source)); - if (toRead.length === 0 && missing.length > 0) return { unread: namedUnpatched(missing) }; + if (toRead.length === 0 && missing.length > 0) return { unread: namedUnchecked(missing.map(withoutPatch)) }; if (toRead.length > MAX_CONTENT_READS) return { unread: `${toRead.length} files with added code, more than the ${MAX_CONTENT_READS} read per pull request` }; return toRead; } @@ -109,28 +112,29 @@ function fileMatches(file: PrFileFacts, patch: string, content: string, grammar: } /** - * Matches on the lines a pull request adds. Without a match, a file in scope without its patch or content leaves the - * rule unchecked, naming the files, since a pass needs every file that adds code checked. + * Matches on the lines a pull request adds, in every file whose patch and content were read. Without a match, a file in + * scope without its patch or content leaves the rule unchecked, naming the files, since a pass needs every file that + * adds code checked. */ export function patternsResult(check: PatternsCheck, files: readonly PrFileFacts[], contents: Readonly>, source: RepoSource): PatternsResult { const toRead = filesToRead(files, source); if (!Array.isArray(toRead)) return toRead; const matches: string[] = []; + const unchecked = files.filter((file) => unpatched(file, source)).map(withoutPatch); try { const rules = patternRules(check); for (const file of files.filter((candidate) => toRead.includes(readPath(candidate)))) { const content = contents[readPath(file)]; - if (content === undefined) return { unread: `${file.path}: its content was not read` }; - matches.push(...fileMatches(file, file.patch!, content, grammarOf(file.path)!, rules)); + if (content === undefined) unchecked.push({ path: file.path, why: "its content was not read" }); + else matches.push(...fileMatches(file, file.patch!, content, grammarOf(file.path)!, rules)); } } catch (error) { return { unread: error instanceof Error ? error.message : String(error) }; } - const missing = files.filter((file) => unpatched(file, source)); - if (matches.length === 0 && missing.length > 0) return { unread: namedUnpatched(missing) }; + if (matches.length === 0 && unchecked.length > 0) return { unread: namedUnchecked(unchecked) }; const skipped = [ - ...missing.map((file) => `${file.path}: not checked, ${whyUnpatched(file)}`), + ...unchecked.map((file) => `${file.path}: not checked, ${file.why}`), ...files.filter((file) => checkable(file) && isForeignPath(file.path)).map((file) => `${file.path}: skipped as vendored, built or installed code`), ]; - return { matches, skipped, checked: files.filter(inScope).length - missing.length }; + return { matches, skipped, checked: files.filter(inScope).length - unchecked.length }; } From fef46daba1210d89ecf1ff89755180552c9d640b Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Tue, 27 Oct 2026 19:29:19 -0700 Subject: [PATCH 9/9] test(triage-workflows): pin why a file is unread past the patch ceiling Also states the worst-case patch text one facts step's output carries. --- docs/DECISION_MODEL.md | 1 + .../triage-workflows/src/logic/facts.test.ts | 16 +++++++++++++--- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/docs/DECISION_MODEL.md b/docs/DECISION_MODEL.md index 3601d2b3a..c6539f640 100644 --- a/docs/DECISION_MODEL.md +++ b/docs/DECISION_MODEL.md @@ -101,6 +101,7 @@ Two different numbers come out of the answers: | Patch the judge reads per file | 65,536 chars | `MAX_PATCH_INPUT_CHARS` | | Patch the judge reads per pull request | 270,000 chars | `MAX_PATCH_CHARS` | | Patch the rules that run in code read per pull request | 2,000,000 chars | `MAX_CODE_RULES_PATCH_CHARS` | +| Patch text in one facts step's output | 2,000,000 chars: one pull request per step, as the hub records backlog and catch-up mails per pull request and each manager reads its own | `MAX_CODE_RULES_PATCH_CHARS` × 1 | | GitHub tool page | 9,000 chars | `PR_FILES_PAGE_CHARS` | - **Docs.** Docs are read from the default branch in this order: root `README`, then `docs/`, then other root markdown, then nested markdown, each in path order. Reading stops at 24,000 bytes; a doc that crosses the cap is cut and later docs are not seen. The same docs are in every request's state. diff --git a/packages/triage-workflows/src/logic/facts.test.ts b/packages/triage-workflows/src/logic/facts.test.ts index 8ddc6e53a..876fbd40d 100644 --- a/packages/triage-workflows/src/logic/facts.test.ts +++ b/packages/triage-workflows/src/logic/facts.test.ts @@ -96,10 +96,9 @@ function readPages(hostFiles: PrFileFacts[], first: FilesCursor = FIRST_FILES_PA if (typeof cursor === "string") throw new Error(cursor); const { patch, ...meta } = hostFiles[cursor.offset]!; const next = cursor.offset + 1 < hostFiles.length ? { next: cursor.offset + 1 } : {}; - const half = Math.floor(patch!.length / 2); let page: FilesPage = { files: [meta], ...next }; - if (cursor.patches && cursor.patchOffset === undefined) page = { files: [{ ...meta, patch: patch!.slice(0, half) }], next: cursor.offset, patchNext: half }; - else if (cursor.patches) page = { files: [{ ...meta, patch: patch!.slice(cursor.patchOffset) }], ...next }; + if (cursor.patches && patch !== undefined && cursor.patchOffset === undefined) page = { files: [{ ...meta, patch: patch.slice(0, Math.floor(patch.length / 2)) }], next: cursor.offset, patchNext: Math.floor(patch.length / 2) }; + else if (cursor.patches && patch !== undefined) page = { files: [{ ...meta, patch: patch.slice(cursor.patchOffset) }], ...next }; cursor = addFilesPage(listed, cursor, page); } return listed.files; @@ -171,6 +170,17 @@ describe("a diff past the code rules' ceiling", () => { expect(item.det.checks.find((check) => check.check === "conventions")).toMatchObject({ result: "unconfirmed", reason: "code patterns could not be checked: src/a.ts: the pull request's diff is larger than Triage reads" }); }); + test("names a file the host sent no patch for by that cause, and only files it did not ask patches for by the ceiling", () => { + const filler = `+const filler = "${"x".repeat(1_000)}";`; + const omitted = { path: "src/omitted.ts", status: "modified", additions: 9_000, deletions: 0 }; + const big: PrFileFacts = { path: "src/big.ts", status: "added", additions: 2_000, deletions: 0, patch: ["@@ -0,0 +1,2000 @@", ...Array.from({ length: 2_000 }, () => filler)].join("\n") }; + const later = { path: "src/later.ts", status: "added", additions: 1, deletions: 0, patch: "@@ -0,0 +1 @@\n+const a = 1;" }; + const listed = readPages([omitted, big, later]); + expect(listed).toEqual([omitted, big, { path: "src/later.ts", status: "added", additions: 1, deletions: 0, patchUnread: true }]); + const contents = { "src/big.ts": `${Array.from({ length: 2_000 }, () => filler.slice(1)).join("\n")}\n` }; + expect(patternsResult(pack.custom.find(isPatternsCheck)!, listed, contents, "github")).toEqual({ unread: "src/omitted.ts: the host sent no diff; src/later.ts: the pull request's diff is larger than Triage reads" }); + }); + test("keeps no lockfile patch, so a lockfile neither counts nor rides in the output, and marks nothing when no patches were asked for", () => { const lock = { path: "bun.lock", status: "modified", additions: 1, deletions: 0, patch: `@@ -1 +1 @@\n+${"x".repeat(MAX_CODE_RULES_PATCH_CHARS)}` }; const { patch: _patch, ...lockMeta } = lock;