diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 5646746da..87da68338 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -548,18 +548,30 @@ 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 - `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. + The facts step reads every changed file's patch whole, across as many tool + 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 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, 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 + 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 f8188ccc5..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 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 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/docs/DECISION_MODEL.md b/docs/DECISION_MODEL.md index 9b4181729..c6539f640 100644 --- a/docs/DECISION_MODEL.md +++ b/docs/DECISION_MODEL.md @@ -98,18 +98,20 @@ 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` | +| 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. - **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 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. - - 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..2526e1921 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. */ @@ -369,7 +369,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 9d216ea9b..7d2fa3ac5 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); } @@ -446,8 +446,10 @@ 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" }); + 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" }); 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 +459,36 @@ rules: expect(patterns([file], { "src/a.ts": content }, broken).checks.find((check) => check.check === "broken")).toMatchObject({ result: "unconfirmed" }); }); + 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 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 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 }; + 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" }); + }); + + 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/checks.ts b/packages/triage-workflows/src/logic/checks.ts index fd61e697c..d6f6de7ec 100644 --- a/packages/triage-workflows/src/logic/checks.ts +++ b/packages/triage-workflows/src/logic/checks.ts @@ -55,8 +55,8 @@ export interface PrFileFacts { additions?: number; deletions?: number; patch?: string; - /** Past the patch budget. */ - patchTruncated?: true; + /** Listed past the code rules' patch ceiling, so its patch was not read. */ + patchUnread?: true; } export interface PrFacts { diff --git a/packages/triage-workflows/src/logic/facts.test.ts b/packages/triage-workflows/src/logic/facts.test.ts index ac9c6b41e..876fbd40d 100644 --- a/packages/triage-workflows/src/logic/facts.test.ts +++ b/packages/triage-workflows/src/logic/facts.test.ts @@ -1,7 +1,13 @@ 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 { 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"; +import { patternsResult } from "./patterns.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); @@ -81,3 +87,111 @@ describe("an answered review", () => { expect(answered([requested], [{ author: "ci[bot]", bot: true }, { author: "maya" }, { createdAt: "2026-10-01T09:00:00Z" }])).toBe(false); }); }); + +/** 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): PrFileFacts[] { + const listed: ListedFiles = { files: [], patchChars: 0 }; + let cursor: FilesCursor | undefined | string = first; + 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 } : {}; + let page: FilesPage = { files: [meta], ...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; +} + +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 listed = readPages(hostFiles); + 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, "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); + 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)!); +}); + +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, "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, 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); + }); + + 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;" }; + expect(readPages([big, file])).toEqual([big, { path: "src/a.ts", status: "added", additions: 1, deletions: 0, patchUnread: true }]); + 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: "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; + 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([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", () => { + 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 453b262bc..eb6f122f3 100644 --- a/packages/triage-workflows/src/logic/facts.ts +++ b/packages/triage-workflows/src/logic/facts.ts @@ -1,10 +1,9 @@ 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 { MAX_PATCH_INPUT_CHARS } from "./hunks.js"; import type { PrFacts, PrFileFacts } from "./checks.js"; +import { MAX_CODE_RULES_PATCH_CHARS } from "./quality.js"; import { isRecord } from "@corbits/common"; -import { MAX_SYSTEM_ONE_STATE_BYTES } from "./quality.js"; export type RepoDocs = Pick; @@ -95,23 +94,34 @@ 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 }; +/** `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 }; +/** 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 }; +function unreadPatch({ patch: _patch, ...file }: PrFileFacts): PrFileFacts { + return { ...file, patchUnread: true }; +} + +/** 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. 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 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: ListedFiles, cursor: FilesCursor, page: FilesPage): FilesCursor | undefined | string { - let files = page.files; + let files = (cursor.ceiling ? page.files.map(unreadPatch) : page.files).map(kept); if (cursor.patchOffset !== undefined) { 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}`; @@ -121,17 +131,19 @@ export function addFilesPage(listed: ListedFiles, cursor: FilesCursor, page: Fil } listed.files.push(...files); listed.patchChars += files.reduce((chars, file) => chars + (file.patch?.length ?? 0), 0); - const patches = listed.patchChars < MAX_PATCH_CHARS; + 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 (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 }; + if (isGeneratedPath(cut.path)) return { offset: page.next + 1, ...next }; + if (patches) return { offset: page.next, patchOffset: page.patchNext, ...next }; + 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}`; - 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. */ @@ -171,7 +183,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..417dfae08 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 { 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, MAX_PATCH_INPUT_CHARS } from "./hunks.js"; +import { addedLines } from "./hunks.js"; type Grammar = { language: PatternLanguage; lang: "TypeScript" | "Tsx" | "JavaScript" }; @@ -46,8 +46,33 @@ 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 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 && !addsNothing(file, source); +} + +/** 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; +} + +/** 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 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}`; + }).join("; "); } function readPath(file: PrFileFacts): string { @@ -57,12 +82,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. */ -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); +/** + * 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); + const missing = files.filter((file) => unpatched(file, source)); + 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; } @@ -84,23 +111,30 @@ 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. */ -export function patternsResult(check: PatternsCheck, files: readonly PrFileFacts[], contents: Readonly>): PatternsResult { - const toRead = filesToRead(files); +/** + * 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 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 }; + if (matches.length === 0 && unchecked.length > 0) return { unread: namedUnchecked(unchecked) }; + const skipped = [ + ...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 - unchecked.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..4fd02333d 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,24 @@ 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; +// 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`. */ +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 +597,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.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 })], 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;