Repository navigation
fix(triage): let the judge read every patch the facts step read - #700
Conversation
|
Independent review of 54e65fb against origin/main. Checked: The code change is right. On the DECISION_MODEL row "Patch text in one facts step's output ... one pull request per step": it is correct. The hub's manager intake splits backlog and Findings
Notes, not blocking
|
54e65fb to
37ada0c
Compare
|
Fixed in 37ada0c: docs state the true worst case (64 first-round + verdict + at most 30 focused re-sends, since re-sends stop at 30 named files = 95 requests, about $0.26, not ~129), the stale facts.test.ts test is dropped, and quality.test.ts pins 64 requests sent / 65 refused with the request-count reason. |
|
Re-review at 37ada0c. No findings. Both earlier findings are fixed. The cost line in docs/DECISION_MODEL.md, its limits row and docs/ARCHITECTURE.md now name 64 first-round requests, plus the verdict request and at most 30 focused re-sends. The leftover facts.test.ts test is removed, and its pattern-rule coverage is still there in the code-rules ceiling tests. I checked the bound against quality.ts and the evaluate and parts actions. The 64 cap applies only to the first round. The new cap test is load-bearing. I changed the comparison to
|
37ada0c to
82e6866
Compare
|
Triage: In progress Triage is waiting for CI to finish before it decides. This comment shows the verdict once it has. |
There was a problem hiding this comment.
Removes the judge's own 270,000-char patch budget so the judge reads every patch the facts step reads, capped by 64 first-round calls instead of by character count. Two concerns stand out: large but legitimate diffs can exceed the request cap and get no judgment at all, and the cap can bind before the 2,000,000-char ceiling the PR describes as the only limit.
P2 · The 64-request cap refuses diffs the facts step read in full, so the judge does not read every patch it was given · packages/triage-workflows/src/logic/quality.ts:562
The PR says the facts step's 2,000,000-char ceiling is the one limit and that the judge reads every patch the facts step read. In practice the 64-call cap binds first. Each request has about 90,000 bytes of file room, minus metadata, docs and the path list. The PR describes up to about 24 KB of docs and up to 8 KB each for paths and directories. A file larger than about half that room gets its own request. A diff of many files in the 25–35 KB range, all under the ceiling, therefore needs more than 64 first-round requests. The judge then sends nothing and every judged rule goes Unsure, even though the facts step read those patches in full.
Trigger: a PR of about 1.5–2.0 MB of patch made of many files around 25–35 KB each, with docs near 24 KB and long paths. Its first-round split needs more than 64 requests, so the code returns the 'pull request too large' refusal even though the facts step read every patch.
Fix: either bound the request count by the same measure the facts step uses, or pack files into requests more tightly, or document the mismatch and correct the PR's claim that the 2,000,000-char ceiling is the only limit.
if (requests.length > MAX_JUDGE_REQUESTS) return { plan, ...(focused && { focused }), requests: [], judgeError: ${QUALITY_EVALUATION_LIMIT_ERROR} (${requests.length} requests) };
| 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; | ||
| if (patch === undefined) return file; |
There was a problem hiding this comment.
P2 · Large diffs that were partly judged now get no judgment at all
Without the 270,000-char budget, every patch up to the facts step's 2,000,000-char ceiling enters the request plan. Per-request room is about 90 KB, and docs (up to 24 KB), metadata and the path list take a share of that. For a wide PR the path list alone can be tens of KB, so a legitimate diff can need more than 64 first-round requests. Before this change, later files were sent without patches and focused/docs still got answered. Now no request is sent and every judged rule, including docs and custom rules, is Unsure, with no partial degradation.
Trigger: a PR with many files whose path list leaves about 25–30 KB of file room per request, and roughly 1.5–2 MB of patch, for example about 1,000 files with sizeable diffs.
Fix: when the request count exceeds the cap, degrade gracefully by sending the first 64 requests' worth of patches and marking the remaining files unread, so only the affected rules become Unsure and the others still get answered. Alternatively, reduce per-request overhead (path list, docs) so the room per request is larger.
if (patch === undefined) return file;
There was a problem hiding this comment.
Reproduced (71 files under the 2,000,000-char ceiling needed 71 calls and got none); fixed in 1592b62: the first 64 calls are sent and only files past them are unread, pinned by a test in quality.test.ts.
TheGreatAxios
left a comment
There was a problem hiding this comment.
Review at 1592b62.
Findings
packages/triage-workflows/src/logic/quality.ts:582: on any diff whose first round needs 64 or more requests, duplicates is dropped and its reason always says "(65 requests)". The first round is now cut to 64 before duplicates is appended, sorequests.length + 1is always 65, whatever the split needed. I confirmed this with a 100-file diff that needed about 100 requests: the reason was "pull request too large for the decision model (65 requests)". Duplicates' request carries no diff, so now that later file requests can be left unread, it could keep its slot: cut the first round at 63 when duplicates has a request, and the one extra file request becomes unread like the rest. If dropping it stays, the reason should not give a request count that the code itself produced.
Notes, not blocking
leaveUnreadis correct for every rule kind. I checked it with a probe: 100 files, plus focused, docs, duplicates and custom rules scoped to the pull request, a file and a hunk. 64 requests went out. Focused, docs and both scoped rules each listed the same 36 later files as unread. Hunk citations were reduced to their paths and listed once. No file was both asked and unread for the file-scoped rule. The whole-PR custom question stayed in request 1.render.tsnever passes a rule that has an unread file: focused andfileCheckgo unconfirmed unless an answered item fails, and docs passes only on a changed doc file, by rule. A file split across the cut keeps its docs part question and is also unread, so it can fail docs but cannot pass it.- Determinism: the plan is a pure function of
facts.files, which keeps GitHub's file order. Two runs on the same input gave the same JSON, so the same files are unread on every run of a head. The files left unread are the tail of that order, which is about path order and not risk. Putting files that docs or scoped rules ask about first, or the smallest files first, would leave fewer files unread. That is optional. - A whole-PR custom rule is still answered only from request 1. Before, a diff past the cap got no answer at all. Now it gets one that did not see the later files. The semantics are the same as for any multi-request diff, so this needs no change.
- The new cap test is load-bearing. With the rest of the branch in place, swapping in main's
quality.tsfails it attoHaveLength(10 requests), and swapping in 82e6866's fails it atjudgeError("71 requests"). Neither deleted test is the only cover for a behaviour that still exists: refusal past the cap is gone, and the KB refusal is still tested at :138. The new test does not cover scoped custom rules or the hunk-citation branch ofleaveUnread, which only the probe above exercised. - The docs match the code. The DECISION_MODEL.md line "Hunk-scoped custom rules add one question per hunk, uncapped, so wide diffs can need more requests" could add that past 64 requests the later files are unread.
bun run lintexits 0 with no warnings in the touched files.bun run checkexits 0.bun --conditions=intx-src test packages/triage-workflows: 297 pass, 0 fail. Both commits are Conventional, with no attribution lines.
1592b62 to
aae86b7
Compare
|
Fixed in aae86b7: with a duplicates request the first round is cut at 63, so duplicates keeps its slot and no reason quotes a request count; the 71-file test now asserts it is sent, and the hunk-scoped docs line notes files past the 64th request are unread. |
TheGreatAxios
left a comment
There was a problem hiding this comment.
Re-review at aae86b7.
The duplicates finding is fixed. When duplicates has a request, the first round is cut at 63 and the duplicates request is always sent, and the "(65 requests)" reason is gone. Compared with 1592b62, only quality.ts, quality.test.ts and docs/DECISION_MODEL.md changed. The updated cap test is load-bearing: with 1592b62's quality.ts swapped in, it fails at the openPullRequests assertion. The cost bound is still right, since duplicates now counts within the 64 first-round requests.
Nits (wording only)
- In
docs/DECISION_MODEL.md, two places still say 64 where it is 63 when duplicates has a request: the "Unread files" bullet ("only requests past the 64th hold it"), and the sentence about files past the 64th request at the end of the hunk-scoped line in the cost section. The "Request splitting" bullet already says "(63 when duplicates has a request)", and these two could say the same. - The title of the cap test in
quality.test.tssays "reading the first 64", but the test sends 63 file requests and the duplicates request.
bun --conditions=intx-src test packages/triage-workflows in a fresh worktree: 297 pass, 0 fail. bun run lint exits 0. bun run check exits 0.
aae86b7 to
950e733
Compare
TheGreatAxios
left a comment
There was a problem hiding this comment.
Both nits applied: DECISION_MODEL.md says 63 where duplicates has a request; the test title matches what it sends.
TheGreatAxios
left a comment
There was a problem hiding this comment.
Re-review at 950e733.
Compared with aae86b7, only the two nit lines and the test title changed, and all three are right.
One nit of the same kind, which I missed last round: the Focused bullet under "Unread files" in docs/DECISION_MODEL.md still says "part of it was only in requests past the 64th". When duplicates has a request, that is the 63rd. This is wording only and is not blocking.
950e733 to
fe93915
Compare
TheGreatAxios
left a comment
There was a problem hiding this comment.
No findings.
The judge read patches only up to 270,000 chars, three requests' worth, so on a larger pull request the files past it reached the judge without lines and focused and docs stayed unconfirmed. The facts step's 2,000,000-char ceiling is below what 64 requests hold, so the judge keeps no patch budget of its own: it reads every patch the facts step read, each cut at 65,536 chars, across up to 64 first-round requests, now its only bound and pinned by a test. DECISION_MODEL.md states the worst case, 64 first-round requests, the verdict request and at most 30 focused re-sends, about $0.26, and says a diff needing more than 64 in the first round leaves every judged rule Unsure as too large for the decision model.
…d leave the rest unread
|
Rebased onto main after #711; the only conflict was the facts paragraph in docs/ARCHITECTURE.md, resolved by keeping this PR's paragraph and main's route and judge lines. |
fe93915 to
c36a049
Compare
There was a problem hiding this comment.
Removes the judge's separate 270,000-char patch budget so it reads every patch the facts step read, split across up to 64 first-round calls. Files past the cap are listed as unread and the affected rules return Unsure, naming them. No review findings were supplied for this change.
Supersedes #692.
Builds on #690 (merged), which moved the judge's limits into
quality.tsand caps the patch the facts step reads at 2,000,000 chars per pull request.The judge already splits a large diff by file across several calls, up to 64 per pull request. It combines focused and docs in code. Focused fails when any slice is a separate change, passes when every slice serves the purpose, and is Unsure otherwise. Docs fails when any file part needs docs. What left #688 Unsure was the judge's own patch budget: it read only 270,000 chars, three calls' worth, so later files reached it without lines and focused listed them as unread.
The judge no longer has a patch budget of its own. It reads every patch the facts step read, each cut at 65,536 chars as before, split across as many first-round calls as it takes up to 64. Files past the facts step's 2,000,000-char ceiling are listed as unread and the rule is Unsure, naming them. A diff that needs more than 64 first-round calls is still judged: the first 64 are sent (63 when duplicates has a request, which keeps its slot), and the files only later calls would hold are unread, so the rules that ask about them are Unsure, naming them. Before, such a diff sent nothing and left every judged rule Unsure. The worst case is 95 calls: the 64 first-round calls, the verdict call, and at most 30 focused re-sends, since re-sends stop once 30 files are named. That is about $0.26 at $0.0027 per full call. Nothing is trimmed silently, and small pull requests are unchanged. Bitbucket Data Center gives no patches, so it is unaffected.
quality.ts:judgedFilesdrops the 270,000-char total. Past 64 first-round calls, the plan drops the later calls and marks their files unread for focused, docs and scoped custom rules, instead of refusing the whole pull request.pr-triage.test.ts: the multi-call test, which runs the facts step against a fake GitHub, now uses more than 270,000 chars of patch. It checks that every file reaches the judge across at least 4 calls.facts.test.ts: drops fix(triage-workflows): read every patch for the rules that run in code #690's test that the judge reads at most three calls' worth. Its pattern-rule half is already covered by the ceiling test.quality.test.ts: a new test builds a diff under the 2,000,000-char ceiling (71 files of 28,000 chars, 24 KB of docs, an 8 KB body, long paths) that needs 71 calls. With a duplicates shortlist on, it checks that 63 file calls and the duplicates call are sent, the 8 files past them are unread for focused and docs, and every other file is asked about.docs/DECISION_MODEL.mdanddocs/ARCHITECTURE.md: they now say the judge reads as much as the facts step read, up to 64 first-round calls, state the 95-call worst case and its cost, and say that files past the 64th call are unread.Verification
listFilesandprepareQualityEvaluation, using its files from the GitHub API. Before this change: 4 calls and 77 files unread by focused. With this change: 9 calls and 0 unread.quality.tswith 10 files missing and passes here.bun --conditions=intx-src test packages/triage-workflows: 297 pass, 0 fail.bun run lint: exit 0, no new warnings.bun run check: exit 0.bun run test:chainhung locally with no output, so CI runs it. Its fixture is a one-line patch.Closes CL-11613