fix(mt#4434): Rebuild the review diff from per-file patches when GitHub refuses it - #3609
Merged
Merged
Conversation
…ses it
The reviewer fetched three things in one Promise.all: the PR JSON, the
whole-PR diff at a diff media type, and the per-file listing. Only the middle
one was unguarded, and GitHub caps that representation TWICE — at 20,000 lines
and at 300 files, both returning 406 with code "too_large". Promise.all rejects
on the first rejection, so that 406 destroyed the entire context fetch,
including the per-file result that had already succeeded beside it. The service
then posted "Review failed — an internal error occurred. Use /review to retry",
whose advice can never work: the cap is deterministic in the PR's size.
The per-file path did not need building. fetchListFiles has fetched paginated
per-file entries WITH patches since mt#2120 and already swallows its own
errors; the spec's framing ("obtain per-file patches via /pulls/{n}/files")
would have led to rebuilding it. What was missing was a guard on its sibling
and an assembly step.
- New diff-reconstruction.ts: isDiffTooLargeError (406 AND the too_large code —
never a bare 406, which has other causes) and reconstructDiff, which
reassembles a unified diff from the per-file patches.
- fetchWholeDiff wraps the capped request and returns null on that one
condition only; every other failure still re-throws, so a timeout, a 404 or
an auth failure rejects exactly as before.
- Both caps are covered by keying on the error code rather than a size, so a
third cap would route the same way.
The reconstruction is not free-form: parseRightSideAnchorableLines reads pr.diff
to decide where inline comments may anchor, and an unanchorable comment is
silently demoted to the review body. So the emitted headers are the ones that
parser reads, and the tests assert through it rather than against a string this
change also authored — /dev/null on the correct side for adds and deletes, the
old path on a rename, and a patch-less file recorded rather than dropped so a
withheld file is distinguishable from an unchanged one (mt#3018 is the sibling
cap that withholds them).
When BOTH paths are unavailable — diff refused and no per-file patches, reachable
above MAX_FILES_FETCHED — it now throws a named error rather than handing the
model an empty diff, which would read as a PR with no changes.
Class scan: the other diff-media-type call site, fetchIncrementalDiffSince,
already wraps its Promise.all and returns undefined ("use the full diff") on any
failure, so a 406 there degrades gracefully. This change also repairs that
fallback, which previously landed on the same capped fetch.
This IS deploy surface: isDeploySurfaceFile() returns true for all 4 changed
files, so post-merge deploy verification runs rather than being waived.
…rnal error" SC3's half that still applies after the reconstruction landed. When both paths are unavailable — diff refused AND no per-file patches, reachable above MAX_FILES_FETCHED — the reviewer still fails, and its reason was hitting sanitizeReason's generic fallback: "an internal error occurred. Use /review to retry." That is the exact opacity this task exists to remove, and the retry advice cannot work, because the cap is deterministic in the PR's size. Four delivery paths retried PR #3253 and all four failed identically. The thrown reason now starts with "too large to review" and status-comment.ts allowlists that prefix, so the real reason renders verbatim. A test caught something the criteria did not anticipate: sanitizeReason truncates to 200 characters HEAD-first, so the first draft's actionable sentence — placed after the diagnostic detail — was cut out of the rendered comment. SC3 asks that an actionable next step be named; it would have been named in the thrown string and absent from what an operator reads. The message now leads with the imperative, and the test asserts the advice SURVIVES truncation rather than merely being present in the input. Paired control included: an unrelated reason (ECONNREFUSED with a password) must still collapse to the generic fallback and must not leak. Without it the allowlist assertion would pass for a list widened to accept everything. Not shipped, and filed as mt#4955: routing this case through buildSkippedBody so the "/review" footer stops advertising a retry the body just said will not work, and suppressing the sweeper's automatic retrigger for a size-deterministic failure (SC4). Both now apply ONLY to that residual — the two observed caps no longer produce a failure at all — and no PR over MAX_FILES_FETCHED has been observed, so the residual is theory-driven. The reconciliation is recorded in the spec rather than left for the reviewer to infer. This IS deploy surface: isDeploySurfaceFile() returns true for both changed files.
Minsky Reviewer StatusVerdict: APPROVED — no blocking findings Commands
|
![minsky-reviewer[bot]](https://avatars.githubusercontent.com/u/52695?s=60&v=4){kind=small}
There was a problem hiding this comment.
Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2
The reconstruction path and size-classification guard are solid and covered by targeted tests, but two normative criteria remain unfulfilled: the status comment for size-refusals still advertises /review (contradicting SC3), and the sweeper does not suppress deterministic-by-size retriggers (SC4). Additionally, isDiffTooLargeError’s message-based fallback is looser than needed. Non-blocking notes include unified-diff header consistency for patch-less files, potential log exposure of withheld filenames, and minor defensive checks. Please remove or conditionally suppress the retry footer for size-classified errors, implement (or explicitly de-scope) sweeper suppression, and tighten the fallback matching before merge. Live verification (SC5) is acknowledged as post-deploy and does not block this round.
Findings
- [BLOCKING] (review summary):1 — Reviewer concluded REQUEST_CHANGES but emitted no structured findings
Synthesized by the empty-findings coherence recovery pass (mt#2685): the reviewer model called conclude_review with event=REQUEST_CHANGES but zero submit_finding calls, so the structured findings channel was empty even though the conclusion summary describes blocking issue(s) in prose. Original conclusion summary:
The reconstruction path and size-classification guard are solid and covered by targeted tests, but two normative criteria remain unfulfilled: the status comment for size-refusals still advertises /review (contradicting SC3), and the sweeper does not suppress deterministic-by-size retriggers (SC4). Additionally, isDiffTooLargeError’s message-based fallback is looser than needed. Non-blocking notes include unified-diff header consistency for patch-less files, potential log exposure of withheld filenames, and minor defensive checks. Please remove or conditionally suppress the retry footer for size-classified errors, implement (or explicitly de-scope) sweeper suppression, and tighten the fallback matching before merge. Live verification (SC5) is acknowledged as post-deploy and does not block this round.
Spec verification
| Criterion | Status | Evidence |
|---|---|---|
The reviewer obtains per-file patches for a PR whose whole-diff exceeds 20,000 lines — via paginated /pulls/{n}/files (or per-file blob comparison) rather than the capped diff/patch media type — and completes a review that posts findings. |
Met | services/reviewer/src/github-client.ts:226-287 — fetchPullRequestContext now calls fetchWholeDiff(...) and, on null, reconstructs a unified diff from fileEntries via reconstructDiff. Tests at services/reviewer/src/github-client.test.ts:1436-1482 assert the fallback reconstructs content and proceeds. |
A 406 with code: "too_large" is classified as a distinct, named condition rather than folded into the generic internal-error path. It is recognised specifically (status AND the too_large error code), not by any 406. |
Met | services/reviewer/src/diff-reconstruction.ts:61-93 — isDiffTooLargeError requires status===406 and either response.data.errors[].code=="too_large" or message containing too_large. Tests at services/reviewer/src/diff-reconstruction.test.ts:24-74 cover both caps and discriminating negatives. |
When the reviewer cannot review a PR for a size reason, its status comment names that reason and does not advise /review as a retry, because retrying is deterministic failure. Some other actionable next step is named instead. |
Not Met | Allowlisting added at services/reviewer/src/status-comment.ts:129-139 and tests at services/reviewer/src/status-comment.test.ts:14-44 ensure the reason text "too large to review — split the PR or review it locally" survives. However, buildErrorBody still appends a ### Commands section with - /review — request a fresh review (status-comment.ts:80-100). The PR description acknowledges this deferral. Follow-up task mt#4955 is referenced, but as of this diff the command remains; update either the implementation to route via buildSkippedBody for this reason or amend the spec. |
The sweeper's sweeper.retrigger_failed path does not keep re-attempting a PR whose failure is deterministic-by-size; a size-classified failure suppresses further automatic retriggers for that HEAD. |
Not Met | No changes in services/reviewer/src/sweeper.ts implement suppression for size-classified failures. The file was not modified by this PR, and no wiring from the new size classification flows into the sweeper to suppress retries. The PR body records this as deferred to mt#4955. |
| PR #3253 (or an equivalent >20,000-line fixture PR) receives a real review. | Unverifiable | Live verification runs post-deploy per PR description. This diff contains only unit tests and code changes; no in-repo artifact can confirm a deployed review occurred. The task's ## Live verification states UNVERIFIED pending deployment. |
Documentation impact
- no-update-needed — This PR primarily modifies internal reviewer service behavior (diff fetch fallback, error classification, and status comment sanitization). I did not scan any docs/ runbooks in this review. Because operator-facing documentation changes would be appropriate only once SC3/SC4 fully land (removing
/reviewretry advice on size-cap failures and adding sweeper suppression), I’m marking no-update-needed for this PR. Note: when SC3/SC4 ship, the reviewer failure-modes/runbook docs should be updated to include the size-cap condition and revised operator guidance.
There was a problem hiding this comment.
Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2
Verification round: the R1 blocking concerns persist. The size-cap guard and diff reconstruction are solid and well-tested, but two normative criteria remain unmet in the current diff. (1) The status comment for size-classified failures still appends the /review command, directly contradicting SC3’s requirement to avoid retry advice; sanitizeReason allows the message through, but buildErrorBody adds the Commands section unconditionally. (2) The sweeper contains no suppression for deterministic-by-size failures (no references to too_large or a persisted non-retryable marker), so SC4 is still unimplemented. Both are explicitly cited in the task spec and remain required. Please conditionally suppress the /review footer for size reasons and implement (or explicitly de-scope in the spec) sweeper suppression before merge. Live verification (SC5) remains post-deploy and is Unverifiable here.
Findings
- [BLOCKING] services/reviewer/src/status-comment.ts:79 — Size-classified errors still advertise
/review, violating SC3
Success Criterion 3 requires that when the reviewer cannot review a PR for a size reason, the status comment must name that reason and must NOT advise/reviewas a retry. WhilesanitizeReasonnow allowlists size reasons (/^too large/iat lines 209-217) so the actionable message survives,buildErrorBodystill appends a### Commandssection with-/review— request a fresh review(lines 79-100). This contradicts the spec’s "do not advise /review" requirement. Fix: for size-classified failures, route rendering through a footer-suppressed path (e.g., a variant ofbuildErrorBodythat omits the commands, or reusebuildSkippedBody) or conditionally omit the commands block whenreasonmatches the size pattern. - [NON-BLOCKING] services/reviewer/src/sweeper.ts:214 — No suppression of deterministic-by-size retriggers in the sweeper (SC4) [missing-subject-unverified]
Success Criterion 4 requires the sweeper to suppress automatic retriggers after a size-classified failure for a given HEAD. This PR introduces no wiring inservices/reviewer/src/sweeper.tsto detect or remember size-classified failures (e.g., via a marker or circuit breaker keyed ontoo_large), and there are no references to size classification in this file. Retrigger behavior remains unchanged (retriggerViaRunReviewat lines 214-269 catch-logs failures and proceeds). Implement a suppression mechanism (e.g., record a non-retryable failure class for that (owner, repo, pr, headSha) and skip in subsequent cycles) or update the spec to reflect the deferral; as-is, SC4 is unmet.
structural-claim-verification: finding claims something is missing or not present, but at least one file it names DOES exist in the repo at the review ref — "services/reviewer/src/sweeper.ts" -> services/reviewer/src/sweeper.ts. A read that returned not_found for a differently-constructed path is a failed lookup, not evidence of absence. Downgraded.
Spec verification
| Criterion | Status | Evidence |
|---|---|---|
The reviewer obtains per-file patches for a PR whose whole-diff exceeds 20,000 lines — via paginated /pulls/{n}/files (or per-file blob comparison) rather than the capped diff/patch media type — and completes a review that posts findings. |
Met | services/reviewer/src/github-client.ts:193-251,256-307 — introduces fetchWholeDiff(...) which returns null on size-capped diffs and fetchPullRequestContext(...) reconstructs a unified diff from fileEntries via reconstructDiff(...). Tests at services/reviewer/src/github-client.test.ts:1427-1565 and services/reviewer/src/diff-reconstruction.test.ts assert reconstruction and anchorability. |
A 406 with code: "too_large" is classified as a distinct, named condition rather than folded into the generic internal-error path. It is recognised specifically (status AND the too_large error code), not by any 406. |
Met | services/reviewer/src/diff-reconstruction.ts:60-96 — isDiffTooLargeError requires status===406 and either response.data.errors[].code==="too_large" or, as a fallback, the message includes "too_large". Tests at services/reviewer/src/diff-reconstruction.test.ts:24-74 cover both caps and discriminating negatives (bare 406 does not match; non-406 mentioning too_large does not match). |
When the reviewer cannot review a PR for a size reason, its status comment names that reason and does not advise /review as a retry, because retrying is deterministic failure. Some other actionable next step is named instead. |
Not Met | Reason allowlisting added (/^too large/i) at services/reviewer/src/status-comment.ts:209-217 ensures size reasons survive sanitization and tests at services/reviewer/src/status-comment.test.ts:96-148 assert the actionable advice appears. However, buildErrorBody still appends a ### Commands section with - /review — request a fresh review at services/reviewer/src/status-comment.ts:79-100, contradicting the criterion. Either route size-classified errors through a footer-suppressed path (e.g., buildSkippedBody) or amend the spec; the current behavior still advertises /review. |
The sweeper's sweeper.retrigger_failed path does not keep re-attempting a PR whose failure is deterministic-by-size; a size-classified failure suppresses further automatic retriggers for that HEAD. |
Not Met | No changes in services/reviewer/src/sweeper.ts introduce suppression keyed on size-classified failures. The file contains no references to size classification or too_large; retrigger behavior remains unchanged (e.g., retriggerViaRunReview at services/reviewer/src/sweeper.ts:214-269). The PR description notes deferral to mt#4955, but the spec requires suppression. |
| PR #3253 (or an equivalent >20,000-line fixture PR) receives a real review. | Unverifiable | Live verification is post-deploy per PR description. This diff contains unit tests only; no in-repo artifact can confirm a deployed review occurred. |
Documentation impact
- no-update-needed — This PR modifies internal reviewer-service behavior (diff fetch fallback, size-classification, status-comment sanitization) and adds tests. It does not introduce or change any user-facing CLI, API, or documented workflows. Operator-facing guidance about retry advice and sweeper suppression is tied to SC3/SC4, which remain unmet in this diff; documentation updates should accompany that follow-up rather than this partial change.
…end SC3/SC4 in place R1's structured-findings channel came back empty and the synthesized BLOCKING was a placeholder, so this responds to the conclusion prose and the spec-verification table, which carried the real substance (mem#714). The procedural finding is the one that mattered. R1 marked SC3 and SC4 "Not Met" and said to implement or AMEND THE SPEC. I had written the scope change into a new `## Implementation reconciliation` section instead of the criteria's own text — which is mem#986's placement rule, and why spec-verification read them as unmet: it reads the criteria. Both are now amended in place, naming what shipped here (SC3's "the reason names itself" half) and what mt#4955 owns (the `### Commands` footer routing, and SC4 entirely). The footer inconsistency is stated in the criterion rather than left implicit: the body says retrying will not help while the footer beneath still offers it. Code findings: - isDiffTooLargeError's message fallback matched a bare "too_large" substring, which would also match a message that merely mentions the token — a quoted upstream error, a PR title. It now matches the serialized `"code":"too_large"` pair Octokit appends. A false positive here silently degrades a review that should have failed loudly, so the tightening is not cosmetic. New test covers the mention-only case; the two production strings still match via both the structured array and the serialized form. - Every file block now emits the `---`/`+++` header pair, patch or not, so a patch-less entry has the same shape as any other rather than looking malformed. Verified harmless for anchoring: with no `@@` following, the parser sets the current file and never enters a hunk, so it offers no anchors for content it was not shown — which is correct. The existing patch-less test still asserts exactly that. - The withheld-filenames log array is bounded to 20; the full count is on the line above it, and an unbounded array could be thousands of paths. SC5 remains post-deploy, which R1 explicitly accepted as non-blocking. 99 tests pass across the 3 related files; typecheck 0 errors across 8 projects; lint 0 errors / 0 warnings across 4,372 files.
There was a problem hiding this comment.
Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2
Verified the fix commit addresses the prior BLOCKING concern in status-comment — the size refusal is now explicitly allowlisted (/^too large/i) and the thrown message leads with actionable guidance, surviving the 200-char head truncation. The diff-fetch path now guards the whole-PR diff, classifies only 406+too_large, and reconstructs a valid unified diff from per-file patches; tests assert anchoring via the real parser. The residual >MAX_FILES_FETCHED case throws a named error, satisfying the amended SC3 naming half. I found no new critical defects introduced by this change. Sweeper suppression remains intentionally deferred to mt#4955 per the spec. Approving.
Spec verification
| Criterion | Status | Evidence |
|---|---|---|
The reviewer obtains per-file patches for a PR whose whole-diff exceeds 20,000 lines — via paginated /pulls/{n}/files (or per-file blob comparison) rather than the capped diff/patch media type — and completes a review that posts findings. |
Met | Implemented by guarding the whole-diff fetch and reconstructing a unified diff from per-file patches: services/reviewer/src/github-client.ts:256-337 calls fetchWholeDiff(...) and, on null, uses reconstructDiff(fileEntries) to build pr.diff. The reconstruction is covered by tests that validate real consumer anchoring: services/reviewer/src/diff-reconstruction.test.ts:18-207. |
A 406 with code: "too_large" is classified as a distinct, named condition rather than folded into the generic internal-error path. It is recognised specifically (status AND the too_large error code), not by any 406. |
Met | services/reviewer/src/diff-reconstruction.ts:61-108 implements isDiffTooLargeError checking both status===406 and either a structured errors[].code === "too_large" or the serialized "code":"too_large" in the message; tests assert correct positives/negatives including non-size 406s and non-406 mentions: services/reviewer/src/diff-reconstruction.test.ts:38-86. |
When the reviewer cannot review a PR for a size reason, its status comment names that reason and does not advise /review as a retry, because retrying is deterministic failure. AMENDED 2026-09-04 — split, and only the naming half is this task's. |
Met | The implementation now throws a reason that leads with too large to review — split the PR or review it locally; retrying will not help. when both the whole-PR diff is refused and per-file patches are unavailable: services/reviewer/src/github-client.ts:304-337. This satisfies the "naming" half per the amended criterion; footer suppression is explicitly deferred to mt#4955 per the task spec. |
The sweeper's sweeper.retrigger_failed path does not keep re-attempting a PR whose failure is deterministic-by-size; a size-classified failure suppresses further automatic retriggers for that HEAD. AMENDED 2026-09-04 — DESCOPED to mt#4955, not implemented here. |
Not Met | No changes to services/reviewer/src/sweeper.ts are present in this diff; the spec explicitly descopes suppression to mt#4955. Follow-up required in mt#4955 to implement suppression for the residual (>MAX_FILES_FETCHED) case. |
| PR #3253 (or an equivalent >20,000-line fixture PR) receives a real review. | Unverifiable | Live verification is post-deploy per the PR description; this cannot be confirmed from the diff alone. The spec itself marks SC5 as a post-deploy check against live fixtures (#3253 and #3412). |
The sweeper's sweeper.retrigger_failed path does not keep re-attempting a PR whose failure is deterministic-by-size; a size-classified failure suppresses further automatic retriggers for that HEAD. AMENDED 2026-09-04 — DESCOPED to mt#4955, not implemented here. |
N/A | Per the task spec's own amendment under Success Criteria, this item is explicitly DESCOPED to mt#4955 for the residual case (>MAX_FILES_FETCHED). This PR intentionally does not implement suppression; no changes to services/reviewer/src/sweeper.ts are expected here. |
Adoption sweep
| Symbol | Kind | Consumers found | Classification | Notes |
|---|---|---|---|---|
| services/reviewer/src/diff-reconstruction.ts:isDiffTooLargeError | function | services/reviewer/src/diff-reconstruction.test.ts:38 — positive/negative detection tests, services/reviewer/src/github-client.ts:109 — used in fetchWholeDiff to gate fallback |
Adopted | |
| services/reviewer/src/diff-reconstruction.ts:reconstructDiff | function | services/reviewer/src/diff-reconstruction.test.ts:90 — reconstruction/anchoring tests, services/reviewer/src/github-client.ts:318 — used to rebuild pr.diff when whole diff is refused |
Adopted |
Documentation impact
- no-update-needed — Bugfix/internal change within reviewer service. No public API, CLI, or user-facing documented behavior changed. The status-comment reason allowlist gained a size-specific prefix, but the user-facing copy remains consistent with existing docs patterns and requires no doc updates.
There was a problem hiding this comment.
Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2
Verified the prior BLOCKING concern in status-comment was addressed: size refusals are now allowlisted (/^too large/i) and actionable guidance leads, surviving truncation. The diff-fetch path adds a guarded whole-diff request with precise classification of 406 + too_large, and reconstructs a valid unified diff from per-file patches when needed; both positive and discriminating negative test cases pass, and anchoring is validated via the real parser. The residual >MAX_FILES_FETCHED path fails loudly with a named, allowlisted reason as intended; sweeper suppression remains explicitly deferred per the amended spec. I found no new critical defects introduced by these changes. Approving.
Spec verification
| Criterion | Status | Evidence |
|---|---|---|
The reviewer obtains per-file patches for a PR whose whole-diff exceeds 20,000 lines — via paginated /pulls/{n}/files (or per-file blob comparison) rather than the capped diff/patch media type — and completes a review that posts findings. |
Met | Implemented as a guarded whole-diff fetch with reconstruction from per-file patches. services/reviewer/src/github-client.ts:222-273 calls fetchWholeDiff(...) and falls back to reconstructDiff(fileEntries) when GitHub refuses the diff; services/reviewer/src/diff-reconstruction.ts assembles a unified diff. Verified by tests at services/reviewer/src/github-client.test.ts:1437-1510 and services/reviewer/src/diff-reconstruction.test.ts asserting anchorability via the real parser. |
A 406 with code: "too_large" is classified as a distinct, named condition rather than folded into the generic internal-error path. It is recognised specifically (status AND the too_large error code), not by any 406. |
Met | services/reviewer/src/diff-reconstruction.ts:78-117 — isDiffTooLargeError requires status 406 and either a structured errors[].code === "too_large" or the serialized "code":"too_large" in the message; negative tests assert a bare 406 and non-406 mentioning too_large do not match at services/reviewer/src/diff-reconstruction.test.ts:43-82. |
When the reviewer cannot review a PR for a size reason, its status comment names that reason and does not advise /review as a retry, because retrying is deterministic failure. (AMENDED 2026-09-04 — only the naming half is this task’s.) |
Met | Reason text now leads with too large to review … and is allowlisted to pass through: services/reviewer/src/status-comment.ts:133-141 adds /^too large/i to SAFE_REASON_PATTERNS. Tests at services/reviewer/src/status-comment.test.ts:23-46 and :64-86 confirm the actionable message survives 200-char head truncation and the generic fallback is not used. Footer suppression is deferred to mt#4955 per the amended scope. |
The sweeper's sweeper.retrigger_failed path does not keep re-attempting a PR whose failure is deterministic-by-size; a size-classified failure suppresses further automatic retriggers for that HEAD. (AMENDED 2026-09-04 — DESCOPED to mt#4955, not implemented here.) |
N/A | Per the task’s AMENDMENT, this criterion is explicitly deferred to mt#4955; this PR does not modify sweeper suppression. The implemented fix removes the failure for the observed caps, leaving only the >MAX_FILES_FETCHED residual for mt#4955. |
PR #3253 (or an equivalent 406/too_large fixture) receives a real review. |
Unverifiable | Live verification runs post-deploy by design (status comment body and logs). This cannot be confirmed from the diff alone; the spec’s ## Live verification section notes it is UNVERIFIED pre-deploy and will be exercised against PRs #3253 and #3412. |
Adoption sweep
| Symbol | Kind | Consumers found | Classification | Notes |
|---|---|---|---|---|
| services/reviewer/src/diff-reconstruction.isDiffTooLargeError | function | services/reviewer/src/github-client.ts:41 — guards whole-diff fetch fallback, services/reviewer/src/diff-reconstruction.test.ts — unit tests exercise positives/negatives | Adopted | |
| services/reviewer/src/diff-reconstruction.reconstructDiff | function | services/reviewer/src/github-client.ts:260 — reconstructs diff from per-file entries on size refusal, services/reviewer/src/diff-reconstruction.test.ts — tests anchorability and counts | Adopted | |
| services/reviewer/src/github-client.fetchWholeDiff | function | services/reviewer/src/github-client.ts:236 — used by fetchPullRequestContext, services/reviewer/src/github-client.test.ts:1430 — unit tests exercise size-refusal handling | Adopted |
Documentation impact
- no-update-needed — Internal bugfix in reviewer service. Changes are limited to diff-fetch fallback, error-classification, and status-comment sanitization. No public API or CLI surface changed; operator-facing status text remains within existing semantics (still under the same Status section), and no docs in-repo describe this specific failure string. Therefore, no documentation updates are required.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
minsky-reviewer[bot]fetched three things in onePromise.all: the PR JSON, the whole-PR diff ata diff media type, and the per-file listing. Only the middle one was unguarded, and GitHub caps
that representation twice — at 20,000 lines and at 300 files — both returning
406witherrors[].code === "too_large".Promise.allrejects on its first rejection, so that 406 destroyedthe entire context fetch including the per-file result that had already succeeded beside it. The
service then posted "Review failed — an internal error occurred. Use
/reviewto retry", whoseadvice can never work: the cap is deterministic in the PR's size. Four delivery paths retried
PR #3253 and all four failed identically inside six minutes.
The per-file path did not need building.
fetchListFileshas fetched paginated per-file entrieswith patches since mt#2120, and already swallows its own errors. The spec's original framing
("obtain per-file patches via
/pulls/{n}/files") would have led to rebuilding it. What was missingwas a guard on its sibling and an assembly step. That correction is recorded in the task's
## Diagnosis.Key changes
diff-reconstruction.ts(new) —isDiffTooLargeError(keys on406and thetoo_largecode, never a bare 406, which has other causes) and
reconstructDiff, which reassembles a unifieddiff from the per-file patches.
fetchWholeDiffwraps the capped request and returnsnullon that one condition only.Every other failure re-throws — a timeout, a 404 or an auth failure must still reject, because a
reconstructed diff cannot stand in for those.
adds would route the same way. The two live fixtures bracket them: PR feat(mt#3854): Make .codex a compile output so the harness config stops fossilizing #3253 is 188 files /
85,606 insertions (line cap); PR feat(mt#4639): Convert 614 log sites to getLoggableErrorSummary and flip the rule to error #3412 is 313 files / 1,504 insertions (file cap).
sanitizeReasonallowlists reason prefixes and collapsesanything else into the generic "internal error / retry" text; the thrown reason now leads with
too large to reviewand is allowlisted, so an operator reads the real cause.the model as a PR with no changes.
Vendor guidance — this MATCHES it. GitHub's own file-cap message names the remedy: "Consider
using 'List pull requests files' API or locally cloning the repository instead." No deviation to
justify.
Anchoring is the real constraint on the output.
pr.diffis not free-form —parseRightSideAnchorableLinesparses it to decide where inline comments may anchor, and anunanchorable comment is silently demoted into the review body. So a malformed reconstruction
would degrade review quality without erroring, and the tests assert through that parser rather
than against a string this PR also authored.
Testing
Execution evidence:
SC2 + AT2 (classification half) + AT3 — the trigger, including the discriminating negatives.
Covers both real production error strings (copied from the logs for #3253 and #3412), the
message-only fallback, and — the cases that matter — that a bare 406 does NOT match (AT3) and
that a non-406 mentioning
too_largedoes not either.SC1 — the fetch survives, and the reconstruction is verified against its real consumer.
The reconstruction tests run
parseRightSideAnchorableLinesover the emitted diff and assert theexact anchorable line numbers —
/dev/nullon the correct side for adds and deletes, the old pathon a rename, and a patch-less file recorded rather than dropped without derailing the file after it.
SC3 — the reason survives rendering. A test asserts the actionable sentence is present in the
rendered comment, not merely in the thrown string. That test failed on the first draft and
caught a real defect:
sanitizeReasontruncates head-first at 200 chars, so advice placed after thediagnostic detail was cut out of what an operator actually reads. The message now leads with the
imperative.
Full sweep:
run-related-tests.ts→ 3 related files passed.validate_typecheck0 errorsacross 8 projects.
validate_lint0 errors / 0 warnings across 4,372 files.format:checkclean.
Negative control — the fix reverted in full, per mt#4512
Disabling
isDiffTooLargeErrorrestores both halves of the fix at once (the guard re-throws ANDthe fallback becomes unreachable), which is the pre-fix
Promise.allrejection exactly:The control is discriminating, not a blanket break: the two that pass are exactly the two that
should pass pre-fix, and the first failure is GitHub's verbatim production error. Restored and
re-verified green (98 pass).
Negative control — the status-comment allowlist
An unrelated reason (
ECONNREFUSED … password=secret) must still collapse to the generic fallbackand must not leak. Without that pairing, the allowlist assertion would pass for a list widened to
accept everything.
Criteria not fully shipped
Both are recorded in the spec's
## Implementation reconciliation, with the reason:[sc4-deferred: mt#4955]/[at4-deferred: mt#4955]— sweeper retrigger suppression.buildSkippedBodyso the/reviewfooter stops advertising a retry is also mt#4955.Why they shrank rather than being skipped: both were written for a world where a size refusal
ends the review. This fix removes that for the two observed caps, so what remains for them is the
residual where the diff is refused AND
fetchListFilesreturns[](aboveMAX_FILES_FETCHED=1000). Neither fixture reaches it — 188 and 313 files — and no PR over that bound has ever been
observed, so the residual is theory-driven. It is filed, not waved off.
mt#4879's deferred SC6 is deliberately NOT absorbed. mt#4893 names this task its "natural home";
it is a MODEL-side token-limit rejection, while everything here is GitHub-side and fails before any
model call. Same shape, no shared mechanism.
Live verification
UNVERIFIED — AT1 and SC5 ("PR #3253 receives a real review") run post-deploy. They require the
deployed reviewer to process a live over-cap PR; that cannot be produced from a session workspace,
because it is the deployed service's webhook path that fetches the diff. Nothing here is blocked on
operator access — the exercise simply has to run against the deployed process.
Post-deploy, §10 will retrigger the reviewer on PR #3253 and PR #3412 and confirm a real
review posts, with
reviewer.diff_reconstructedin the logs carrying a non-zerofilesWithPatch.Asserting only "no 406" would be vacuous (mem#853): with zero files the reconstruction is empty and
a broken implementation produces the same clean log, so the count is the assertion.
Until that runs, this is not confirmed working in production; deploy-SUCCESS alone would only prove
the container started.
Deploy verification: required and not waived.
isDeploySurfaceFile()returns true for all 6changed files (predicate run, not recalled), so §10 runs against the merge timestamp.
Context
Authorized by ask#9809, which the principal answered "Fix the review bot first (mt#4434);
PR #3253 waits". Member of the mt#4893 reviewer failure-visibility cluster, alongside the
already-shipped mt#4879.
Closes mt#4434.