Repository navigation
fix(triage-workflows): read every patch for the rules that run in code - #690
TheGreatAxios merged 9 commits into
Conversation
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.
…0,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.
TheGreatAxios
left a comment
There was a problem hiding this comment.
This branch has the facts step read every changed file's patch, up to a separate 2,000,000-character ceiling, for the code-pattern rules. The judge keeps its own limits in judgedFiles, a file without a patch is checked around and named in the evidence, and a repeated reason is shown once. Lint is clean and the triage-workflows tests pass (289). GitHub paging and the 300 content reads are unchanged. Bitbucket Data Center still comes out unconfirmed.
packages/triage-workflows/src/logic/patterns.ts:54:unpatched()treats a pure rename as a file without a diff. GitHub sends nopatchfor a rename with no changes (checked on #647's files API). So a pull request that editsa.tsand renamesb.tspasses with "no code pattern matched; 1 file was not checked" and the evidence says "not checked, the host sent no diff". A pull request whose only code change is renames comes out unconfirmed. Both reproduced withpatternsResult.quality.ts:538already treatsadditions === 0 && deletions === 0as nothing to read. Using the same test here (oradditions === 0) leaves Data Center unconfirmed, because it reports no counts.packages/triage-workflows/src/logic/quality.ts:189: the ceiling is per pull request, but one facts step output holds every pull request of a backlog or catch-up batch (fetchListed→fetchTargets(numbers)inactions/facts.ts). A single step output can therefore hold N × 2,000,000 chars, which doesn't match the reason given for the constant. Also, with 9,000-char tool pages, reaching the ceiling takes about 225 or more sequentiallist_pr_filescalls for one pull request, each fetching a 100-file GitHub page again (about 30 at 270,000).packages/triage-workflows/src/logic/quality.ts:186: the judge only sees what the facts step read, so its real budget is the smaller ofMAX_PATCH_CHARSandMAX_CODE_RULES_PATCH_CHARS. #691 sets the judge's budget to 64 × 90,000 = 5,760,000, which is above 2,000,000. Either it also raises the code-rules ceiling, which ties the two limits together again, or the judge stays capped at about 22 requests' worth while the docs say 64. This needs deciding before #691 rebases onto this branch.packages/triage-workflows/src/logic/patterns.ts:62:namedUnpatchedgives the first file's reason for "and N more", so a mixed list (a file with no diff from the host plus files past the ceiling) shows the wrong reason for the rest.packages/triage-workflows/src/logic/facts.ts:115: withpatches: false, every file is markedpatchUnread. That includes every file of the history scan (HISTORY_FILES_PAGE), where no ceiling was passed. This contradicts the field's doc atchecks.ts:58. No pattern rules run on the scan, but its step output now carries a wrong flag on every file.packages/triage-workflows/src/logic/facts.ts:174:fileFactsonly parses host tool results, and the tool never sendspatchUnread(onlyaddFilesPagesets it). This branch is dead code, and it lets a tool result set the flag.packages/triage-workflows/src/logic/render.ts:683: no test covers showing a reason once (here and at:709). NeitherWaiting on:norreasonsis tested with two pattern rules held by the same unread input.packages/triage-workflows/src/logic/facts.ts:124: patches neither reader uses still count toward the code-rules ceiling, for example lockfiles, which pattern rules skip and the judge drops as generated. A large lockfile bump can push real.tsfiles past the ceiling, and they are then shown as not checked.docs/ARCHITECTURE.md:559: this line is longer than the surrounding wrap width.
… 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.
TheGreatAxios
left a comment
There was a problem hiding this comment.
Re-review at 55d04db. All nine earlier findings are closed. The source parameter is threaded through patternsOf, filesToRead and patternsResult correctly, and it is needed: Data Center sets additions: 0, deletions: 0 on every file (bitbucket-datacenter-reads/src/changes.ts:33). patchUnread is set only once the ceiling is reached (history scans with patches: false are not marked), the dead parse is gone, reasons are grouped, and the render test covers showing a reason once. Lint passes and the triage-workflows tests pass (293).
packages/triage-workflows/src/actions/facts.ts:243: becausepatchesReadis shared across a batch, a pull request's result now depends on which other pull requests are in the batch. On a catch-up or backlog run whose pull requests add up to more than 2,000,000 chars, the large pull requests have their later files listed aspatchUnread. Pages are read in rounds, so they are cut first. Those files have additions but no patch, so the judge treats them as unread (quality.ts:541), and focused and docs come out Unsure. The same pull request checked on its own (or before this branch, with a 270,000 budget per pull request) would get an answer. For example, eight 270,000-char pull requests in one backlog already cross the ceiling. The owner should choose between a per-pull-request floor of at least the judge's budget before the shared ceiling applies, and a smaller backlog batch.packages/triage-workflows/src/logic/facts.ts:112: lockfile patches no longer count toward the ceiling, but they are still read page by page and stored in the step output, where nothing reads them (the judge drops them, pattern rules skip them). So the ceiling no longer bounds what the step output holds. Dropping the patch of a file that matchesisGeneratedPathinaddFilesPagewould keep both the count and the output bounded.
…nd 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.
TheGreatAxios
left a comment
There was a problem hiding this comment.
Re-review at 7ae9f76: each pull request has its own 2,000,000-char ceiling again, and generated-file patches are dropped in addFilesPage. Both remaining findings are closed. A generated patch that runs over a page is cut only when it is alone on its page (listPrFilesPage breaks before an oversized file on a page that already holds files). So skipping to next + 1 only skips the rest of that file, and the file after it starts the next read; the lockfile test covers this split. actions/facts.ts matches main apart from the host parameter. Lint passes and the triage-workflows tests pass (293). No findings.
|
Triage: Unsure Waiting on:
Notes:
Changed since the last verdict (Fail at |
There was a problem hiding this comment.
Changes the facts step to read every changed file's patch whole up to a 2,000,000-character per-pull-request ceiling, so the code-pattern rules no longer refuse large pull requests, while the judge keeps its own budgets and files past the ceiling are marked unread. Three findings: a dropped passthrough for the new patchUnread flag, a batch-level step output that can grow beyond the stated bound, and mode-only changes being reported as unpatched.
| ...(patch === undefined ? {} : { patch }), | ||
| ...(value.patchTruncated === true ? { patchTruncated: true as const } : {}), | ||
| }]; |
There was a problem hiding this comment.
P2 · fileFacts drops the new patchUnread flag when it reads files back
The patchTruncated passthrough was removed, but no patchUnread passthrough replaces it. When the step output is parsed back through fileFacts, a file past the 2,000,000-character ceiling loses its marker, so whyUnpatched reports "the host sent no diff" instead of "the pull request's diff is larger than Triage reads".
Trigger: a pull request whose diff passes MAX_CODE_RULES_PATCH_CHARS, with its files carried through the step output and re-parsed by fileFacts before patternsResult runs.
Fix: carry patchUnread through the fileFacts mapping alongside the patch, for example ...(file.patchUnread ? { patchUnread: true } : {}), and add a test that round-trips an unread file through the step output.
There was a problem hiding this comment.
Not a real bug: fileFacts only parses host tool results, and the rules step reads facts from the step output as JSON. A new test in 2b88640 sends a past-ceiling file through the step output into rules, and it keeps patchUnread and the "larger than Triage reads" evidence.
| 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; |
There was a problem hiding this comment.
P2 · Per-pull-request 2,000,000-char ceiling does not bound the step output across a batch
The ceiling is counted per pull request, since addFilesPage gets a fresh ListedFiles for each one, and fetchTargets keeps each item's full patches in the step output. A batch of N pull requests can therefore carry about N × 2,000,000 characters of patch text in one output. The description and code comment say the sidecar's diff "stays bounded", which is only true per pull request.
Trigger: a backlog or multi-PR step where several large pull requests, each under the ceiling, are read together.
Fix: either apply a shared batch-level budget across items, or correct the description and comment to say the bound is per pull request and state the worst-case batch size.
There was a problem hiding this comment.
Accepted trade-off: the ceiling is per pull request on purpose, so a result never depends on its batch, and on the manager path each facts step reads one pull request.
| * 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); |
There was a problem hiding this comment.
P3 · A GitHub file with no lines changed and no patch is reported as 'host sent no diff'
renamedOnly exempts only status 'renamed'. A file with zero additions and zero deletions and no patch, such as a permission-only change, is still counted as unpatched. It is listed as "not checked, the host sent no diff", and if it is the only in-scope file the rule becomes unconfirmed. The change's own rule is that a file with no lines added or removed has nothing to read, so it should not be listed.
Trigger: a pull request that only changes the executable bit of a .ts file. GitHub sends no patch and additions and deletions are both 0.
Fix: exempt files with additions === 0 && deletions === 0 as well as renamed-only files in the unpatched check, for example return inScope(file) && file.patch === undefined && !renamedOnly(file, source) && (file.additions ?? 1) + (file.deletions ?? 1) > 0;, and add a test for a mode-only change.
There was a problem hiding this comment.
Fixed in 2b88640: any GitHub file with no lines added or removed and no patch, whatever its status, is now nothing to read. Bitbucket Data Center is unchanged, and the test covers a mode-only change.
There was a problem hiding this comment.
Changes the facts step to read every changed file's patch whole, up to a 2,000,000-character per-pull-request ceiling, so code-pattern rules no longer refuse large pull requests like #688. The judge's budgets now apply inside judgedFiles, and the pattern rules list files they could not check with a reason. Two findings: a code-pattern rule can pass while an unchecked file that adds code is ignored, and the step output now carries up to about 7.4 times more patch text per pull request.
| 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` }; |
There was a problem hiding this comment.
P1 · Code-pattern rule can pass while an in-scope file that adds code was never checked
Before this change, any in-scope file without a patch left the rule unconfirmed. Now, as long as at least one other file has a patch, the rule runs on the rest and passes. The unchecked file is only listed as evidence, so the rule can report pass without having inspected that file's added lines. This conflicts with the manual's principle that a rule which cannot check its input is unconfirmed rather than passing.
Trigger: a GitHub pull request with a very large TypeScript file (GitHub sends no patch) that adds ?? 0, plus one small clean TypeScript file with a patch. filesToRead returns ['src/clean.ts'], so the rule passes with "no code pattern matched; 1 file was not checked".
Fix: when any in-scope file that adds code is missing a patch, return unconfirmed instead of pass, and keep the "not checked" list as supporting evidence. For example, change the early return in patterns.ts so that missing.length > 0 yields { unread: namedUnpatched(missing) } regardless of toRead.length.
There was a problem hiding this comment.
Fixed in a830127: a match in a checked file fails, otherwise a file in scope that adds code but has no patch leaves the rule unconfirmed with the files named, and it passes only when every such file was checked.
| 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; |
There was a problem hiding this comment.
P2 · Step output now carries up to 2,000,000 chars of patch text per pull request, up from 270,000
The change raises the per-pull-request patch ceiling from 270,000 to 2,000,000 characters and keeps those patches in the facts output that goes through the sidecar. The description says the diff the sidecar holds stays bounded, but that bound is now about 7.4 times larger per pull request. In a batch, every pull request carries its own 2 MB of patch text.
Trigger: a batch of many large pull requests, each with a multi-megabyte diff. Each item's facts output carries up to 2 MB of patch text, so the step output grows with batch size.
Fix: bound the total across the batch, not just per pull request, or keep patches out of the step output and pass only the evidence the rules need. At minimum, state the batch-level worst case in the docs and the description, and confirm the sidecar can hold it.
There was a problem hiding this comment.
Accepted trade-off: the 2,000,000-character ceiling bounds each pull request's step output, and on the manager path each facts step reads one pull request.
…at 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.
TheGreatAxios
left a comment
There was a problem hiding this comment.
Re-review at a830127: a GitHub file with no added lines is now nothing to check, and the code-pattern rule fails closed. A match in a checked file fails the rule. With no match, any in-scope file that adds code but has no patch leaves the rule unconfirmed, naming the files. Bitbucket stays unconfirmed. Fail, unconfirmed and nothing-to-read are each covered by a test in checks.test.ts, the ceiling cases now expect unconfirmed, and a JSON round trip through the rules step keeps the marks. The ARCHITECTURE.md and user manual sentences match. Lint passes and the triage-workflows tests pass (294).
packages/triage-workflows/src/logic/patterns.ts:122: a file whose content was not read still returnsunreadfrom inside the loop, which throws away matches already found in other files. Fail closed is kept (it never passes), but a pull request with a real match comes out unconfirmed instead of failing. This contradicts "A match in any checked file fails the rule" (docs/ARCHITECTURE.md:558). Reproduced: withsrc/b.tsaddingx ?? 0andsrc/a.ts's content unread, both file orders return{ unread: "src/a.ts: its content was not read" }. Collecting the unread files and applying the same match-first rule as for files without a patch would make the two cases agree.
…content was not read
|
Re review 5676809786 (patterns.ts:122): fixed in b6f2d8e. A file whose content was not read is now handled like a file with no patch: every other file is checked, a match anywhere fails the rule, and otherwise the rule is unconfirmed naming the unread files. A new test covers b.ts adding |
TheGreatAxios
left a comment
There was a problem hiding this comment.
Re-review at b6f2d8e: a match now fails the rule even when another file's content was not read, in either file order. The new test covers both orders, and the unread file is listed as not checked. Lint passes, triage-workflows tests pass (295). No findings.
There was a problem hiding this comment.
Changes the facts step to read every changed file's patch whole up to a new 2,000,000-character per-pull-request ceiling, so code-pattern rules no longer refuse large pull requests like #688, and the judge applies its own limits to what it sends. Code-pattern rules now check every patch and report unread files with a reason. Two defects need attention: the ceiling is per pull request, so batch step output can grow without bound, and files the host sent no patch for are mislabelled as larger than the ceiling.
| 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; |
There was a problem hiding this comment.
P2 · Patch ceiling is per pull request, so a batch's step output can grow without bound
Each pull request may now carry up to 2,000,000 patch characters in the facts step output, up from 270,000, and every pull request in a backlog batch gets its own ceiling. A batch's serialized output therefore grows with batch size times 2 MB. All deployments share one sidecar for step outputs, so this conflicts with the claim that the diff they carry stays bounded.
Trigger: a backlog batch of many large pull requests, for example 50 pull requests each near 2 MB of patches, produces roughly 100 MB of step output JSON passed to the rules step.
Fix: bound the total across the batch as well as per pull request, or lower the per-pull-request ceiling so the worst-case batch output has a stated maximum, and document that maximum in DECISION_MODEL.md's limits table.
There was a problem hiding this comment.
Accepted trade-off: the worst case is 1 × 2,000,000 = 2,000,000 characters per facts step output. The hub records backlog and catch-up mails one pull request at a time (manager-intake pullsOf/itemsOf), each manager's facts step reads only its own, and DECISION_MODEL.md's limits table now says so (fef46da).
| */ | ||
| 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); |
There was a problem hiding this comment.
P3 · Files the host sent no patch for are reported as larger than the ceiling
Once the ceiling is reached, unreadPatch is applied to every file on the page, including GitHub files that never had a patch because they are very large or binary. Those files then carry patchUnread, and withoutPatch in patterns.ts reports them as "the pull request's diff is larger than Triage reads" instead of "the host sent no diff". The rule still ends unconfirmed, but the stated cause is wrong for those files.
Trigger: a pull request past the 2,000,000-character ceiling that also contains a GitHub file whose patch the host omitted. The code-pattern rule's reason names the wrong cause for that file.
Fix: only apply unreadPatch to files that actually had a patch when the ceiling was hit, and leave patchless files marked with the host-sent-no-diff reason.
There was a problem hiding this comment.
Checked in fef46da: a file the host was asked about and sent no patch for keeps "the host sent no diff" (new test). Only files on pages requested without patches after the ceiling, and the file cut at it, are marked, and for those the ceiling is the actual reason they were not read.
Also states the worst-case patch text one facts step's output carries.
Nothing triage asked to change still fails, so this request no longer stands.
The facts step now reads every changed file's patch whole, up to its own ceiling of 2,000,000 characters per pull request. Before, it stopped reading patches once the diff passed the budget sized for the judge, and the code-pattern rules then refused the whole pull request. On #688 (141 files) both pattern rules said a six-line file's patch "was not read in full".
addFilesPagedrops the judge's 270,000-character budget and its 65,536-character per-file cut.MAX_CODE_RULES_PATCH_CHARS(2,000,000), which sits next to the judge's budgets inquality.ts. Each pull request in a batch counts its own, so its result never depends on the others. One sidecar holds every deployment's step outputs, so the diff they carry stays bounded.patchUnread.judgedFilesinquality.tsnow applies the judge's two limits to what it sends. It marks a cut file and sends files past its budget without a patch, so the judge's requests stay the same size as before.A GitHub file that adds no lines, such as a rename or a mode change, has nothing to check. Bitbucket Data Center stays unconfirmed as before. The 300-file content read limit stays.
reasons, is now shown once.Verified:
bun run lintis clean.bun --conditions=intx-src test ./packages/triage-workflows: 295 pass, 0 fail.reasons.bun run checkloops on local Bun 1.4.2, so CI runs the typecheck.Closes CL-11612