fix(github): outlast the secondary-limit window, and say why a comment was refused - #723
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
🤖 ThrillhouseBot PR SummaryWhat this PR doesThe PR makes GitHubWriteRetry outlast the measured 72-second content-creation secondary-limit window by raising the attempt count to four (a 90s TOTAL_BUDGET) and flooring any derived backoff wait at 30s while GitHub reports a content-creation block; the GitHubWritePacer default ceiling rises to match. It also makes ReviewPublisher log GitHub's rejection status and message at WARN — keeping both reasons when the with-suggestion and without-suggestion attempts fail differently — instead of discarding every rejection reason at debug. Description vs. ImplementationNo mismatch found between the PR description and the change. Control-Flow Diagram🔀 Show diagramflowchart TD
A["Write POST refused: 403 content-creation block"] --> B{"Retry-After header present?"}
B -- "yes" --> C["Wait the stated Retry-After"]
B -- "no" --> D{"Body matches CONTENT_CREATION_BLOCK?"}
D -- "no" --> E["Use derived delay (reset instant or linear backoff)"]
D -- "yes" --> F{"Derived delay < 30s?"}
F -- "yes" --> G["Floor the wait at 30s"]
F -- "no" --> E
C --> I["GitHubWriteRetry: at most 4 attempts, each wait <= 30s"]
E --> I
G --> I
I -- "90s TOTAL_BUDGET outlasts the 72s block" --> J["Comment posts"]
I -- "still throttled after 4 attempts" --> K["Failure propagates; reason logged at WARN"]
Changes Overview
Changed Files
Risk Assessment
Previous Findings Status
|
| Check | Type | Status | Detail |
|---|---|---|---|
| dependency-review | check-run | ⏳ Pending | - |
| trivy | check-run | ⏳ Pending | - |
| test | check-run | ⏳ Pending | - |
| format | check-run | ⏳ Pending | - |
| frontend | check-run | ⏳ Pending | - |
Automated review by ThrillhouseBot. Reply with /review to re-run.
![thrillhousebot[bot]](https://avatars.githubusercontent.com/in/3996985?s=60&v=4){kind=small}
There was a problem hiding this comment.
ThrillhouseBot requested changes — see inline comments on the diff.
ThrillhouseBot noted 1 lower-confidence item(s) under Things to double-check in the PR summary (not posted as inline threads):
- LOW: File-level fallback drops the suggestion block silently by design and the body lead-in overstates that GitHub 'could not open a thread on' the line (
src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewPublisher.java:681)
fileLevelLeadIn says 'ThrillhouseBot could not open a thread on file:line'. When the file-level fallback is used because the line is genuinely outside the diff, GitHub was never asked; when it is used because the line-anchored POST was rejected, the POST was refused but the fallback then succeeded — so 'could not open a thread on' is accurate in the second case only. The first case (line outside diff) is now described as a failure to open a thread on a line that was never attempted — that is a wording that blames the diff again, which the PR explicitly says it no longer wants to do: the same sentence previously claimed a cause the code cannot know; now it claims another cause ('could not open a thread') for a case where the code never tried. This is a comment-contradicts-behavior issue: the javadoc for postFileLevelComment says it is 'the honest answer for a line that genuinely is outside the diff', but the lead-in tells the reader GitHub could not open a thread on that line, which is false for that genuine case. Suggest rewording to 'ThrillhouseBot filed this on the file: file:line could not be anchored' — but the PR deliberately removed 'could not be anchored'. The current wording is at least misleading for the outside-diff case. Medium because it misleads the reader about the cause, which is exactly the defect class this PR claims to fix.
thrillhousebot
Bot
added
bug
b87e053 to
dc89083
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
ThrillhouseBot found no issues in this PR, but some checks are still pending or failed:
- Check dependency-review is pending
- Check format is pending
- Check test is pending
- Check frontend is pending
- Check trivy is pending
Additionally, No new issues in this revision, but 4 previous finding(s) remain unresolved — fix them, or reply on their review thread with why they are deferred. A finding listed only under "Things to double-check" has no thread: clear it by commenting @thrillhousebot resolved path/to/File.java:42 — <the finding's title> on this PR.
🤖 ThrillhouseBot — changes since the last review
|
There was a problem hiding this comment.
ThrillhouseBot noted 1 lower-confidence item(s) under Things to double-check in the PR summary (not posted as inline threads):
- LOW: SUBJECT_TYPE_FILE without access modifier may not compile when accessed cross-package (
src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubReviewClient.java:362)
The added lineString SUBJECT_TYPE_FILE = "file";carries no access modifier. Inside a class that declaration is package-private, and ReviewOrchestratorTest (package dev.thiagogonzaga.thrillhousebot.review) accesses it from a different package, e.g.assertEquals(GitHubReviewClient.SUBJECT_TYPE_FILE, request.subjectType());andargThat(req -> GitHubReviewClient.SUBJECT_TYPE_FILE.equals(req.subjectType())). Per JLS 6.6.1 a package-private member is accessible only within its own package, so if GitHubReviewClient is a class this will fail compilation. If GitHubReviewClient is instead a @RegisterRestClient-style interface, interface fields are implicitly public static final and it compiles. The enclosing type declaration is not visible in the provided material (the hunk starts mid-file atrecord ReviewComment(), so verify whether GitHubReviewClient is declared as a class or an interface before merging; the PR description's claimed green run (3255 tests) contradicts this finding if it is a class, so CI is the decisive check. Fix: declare the constantpublic static final(valid and behavior-changing for a class; redundant but harmless for an interface).
c604f2b to
4d4eebb
Compare
There was a problem hiding this comment.
ThrillhouseBot found no issues in this PR, but some checks are still pending or failed:
- Check format is pending
- Check test is pending
- Check frontend is pending
- Check trivy is pending
- Check dependency-review is pending
Additionally, No new issues in this revision, but 2 previous finding(s) remain unresolved — fix them, or reply on their review thread with why they are deferred. A finding listed only under "Things to double-check" has no thread: clear it by commenting @thrillhousebot resolved path/to/File.java:42 — <the finding's title> on this PR.
|
Clearing the two items under Things to double-check, with reasons. @thrillhousebot resolved src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubReviewClient.java:362 — SUBJECT_TYPE_FILE without access modifier may not compile when accessed cross-package
@thrillhousebot resolved src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewPublisher.java:681 — File-level fallback drops the suggestion block silently by design and the body lead-in overstates that GitHub 'could not open a thread on' the line Half of this is factually wrong and the other half is accurate as written. The drop is not silent: the lead-in says so in the comment body — "so this finding has no line anchor and no applicable suggestion" — which is the disclosure the finding asks for, and it is there precisely because GitHub can only apply a On the wording, "could not open a thread on |
|
The next review will close every previous finding this comment names by its |
…t was refused Two defects behind #712's misdiagnosis, both measured on the round-7 build. The backoff budget was three attempts and its own javadoc asserted the resulting 60s was long enough to outlast the window GitHub's content-creation secondary limit uses. That was an assumption. GitHub answered content creation with 403 'You have exceeded a secondary rate limit and have been temporarily blocked from content creation' — x-ratelimit-remaining still at 4771, so primary quota was nowhere near exhausted — across 72 seconds, simultaneously on unrelated pull requests. The budget expired inside the window and the writes were given up on. A fourth attempt takes the budget to 90s, which outlasts the measured block with margin, and TOTAL_BUDGET is derived from the two bounds so it cannot drift from them. It is deliberately not sized to outlast an arbitrary block: a limit wider than this is telling the deployment it writes too fast, and the answer there is pacing, not a longer wait holding a PR's slot. The pacer's ceiling is documented as the same number and stays a literal, because reading it from the backoff makes the two static initializers circular — spotbugs rejects it, and whichever class loaded second would see a null. A test asserts the two are equal instead. The second defect is why none of this could be diagnosed. Every rejection reason was logged at debug, which is off in production, so a run recorded only that a comment 'could not be anchored to the current diff' — a claim about line numbers the code had no basis for. Twenty-nine rejections in that round are permanently undiagnosable, and the wording sent two scorers and then #712 hunting an off-by-N that did not exist. The status and body now reach the warning an operator sees. The first attempt stays at debug: a rejected suggestion block is ordinary and is how the retry without one is discovered.
…d keep both rejection reasons Raising the attempt count bounded one call's wait from above; it did not make the budget reach the measured 72-second block. Both ways of deriving a delay without a Retry-After undershoot it. The linear fallback gives 5s, 10s then 15s — thirty seconds spread over four attempts. An x-ratelimit-reset already in the past gives zero, so every attempt fires at once and the budget is gone in milliseconds, which is worse than not repeating at all: it spends the repeats while GitHub is still refusing. Neither number is GitHub speaking about this block. The reset belongs to the primary window, which a secondary limit leaves alone — the run behind #722 was blocked from content creation with remaining=4771. So a derived delay is floored at 30s for a content-creation block, which makes TOTAL_BUDGET a floor for this failure and not only a ceiling. An explicit Retry-After still wins outright, and a primary window that really is exhausted keeps its reset instant, since there the instant is a deadline for this failure rather than a number about another one. The rejection log kept only the second attempt's reason. The two attempts fail for different causes often enough to matter: a suggestion block GitHub will not take is a 422 about the payload, a content-creation block is a 403 about the moment. Reporting only the second names the payload for a finding a throttle refused, which is the wrong-diagnosis class this change exists to stop. Both are reported when they differ. Reported by the reviewer on this PR.
…nce is known SonarCloud's reliability gate flagged the Optional.get() at the end of postFindingComment (java:S3655). The value is in fact always present — the empty case returns early, and the combining step ends with .or(...) — but that is carried across a filter/map/or chain the analyzer cannot follow, and a rule about unchecked Optional access is one worth keeping strict. Presence is established once, at the early return, so the reason is a plain String from there on and the chain goes away. Same output, one less thing for a reader to prove. Reported by SonarCloud on this PR.
4d4eebb to
5309b48
Compare
There was a problem hiding this comment.
ThrillhouseBot noted 2 lower-confidence item(s) under Things to double-check in the PR summary (not posted as inline threads):
- LOW: Content-creation floor bypassed when the primary quota also reads 0 (
src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiError.java:220)
The new 30s floor for content-creation blocks is only reached by the final expression, but the earlier branchif (primaryWindowExhausted() && !derived.isZero()) { return derived; }returns the primary-window-derived delay first. When a content-creation 403 happens to carryx-ratelimit-remaining: 0and a short-futurex-ratelimit-reset(e.g. now+10s), retryDelay returns 10s instead of the 30s floor; three waits then total ~30s, still inside the 72s window this PR is sized against, so the #722 failure mode recurs. The floor's own javadoc promises that "a derived delay is floored" whenever GitHub "is blocking content creation and named no deadline of its own" — the carve-out violates that promise in this state. The PR's own measured evidence establishes the premise problem: "the run behind #722 is exactly that, a content-creation block carrying x-ratelimit-remaining=4771" — the headers describe the primary window, which the block leaves untouched, so remaining=0 does not make the reset "a deadline for this failure". None of the new tests cover a block body with remaining=0 (the new GitHubApiErrorTest cases use 4771), so the green suite does not exercise this path. Fix: apply the floor to content-creation blocks before honoring the primary reset. - LOW: Content-creation regex may miss a block wording without the two phrases (
src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiError.java:94)
Heuristic probe; input not in the diff: a 403 body such as {"message":"You have been temporarily blocked from content creation. Please try again later."} contains neither "secondary rate limit" nor "abuse detection", soPattern.compile("(?i)secondary rate limit|abuse detection").matcher(body).find()returns false. retryDelay then falls back to the linear 5s/10s/15s schedule and the three waits total 30s — well inside the 72s window this PR is sized against — so the floor and the 90s budget silently never engage for that wording, and the exact failure #722 describes recurs with no floor and no warning about it. The pattern matches the wording quoted in the PR's own logs and GitHub's known "abuse detection" variant, but the provided material does not establish whether GitHub also sends a content-creation block message that omits both phrases. Verify the real secondary-limit message variants GitHub emits for this block class; if a variant without either phrase exists, broaden the pattern (e.g. also match "content creation" or "temporarily blocked").
…mit headers say The floor carved out a case it should not have. A block carrying x-ratelimit-remaining=0 and a near reset returned the reset-derived delay instead, so a reset ten seconds out gave ten-second waits and spent the whole budget inside the 72-second window the floor exists to span. The carve-out also contradicted the floor's own javadoc, which promises a derived delay is floored whenever GitHub is blocking content creation and named no deadline. Those headers describe the primary window, which a content-creation block leaves untouched, so remaining=0 alongside it says nothing about when creation reopens. It is dropped, and the floor now applies to every such block. An explicit Retry-After still wins, since there GitHub is naming its own deadline. The test that motivated the carve-out asserted a primary-window reset is honoured while posting the secondary-limit body, which conflated the two failures. It now uses a plain rate-limit-exceeded body, so it tests the primary exhaustion it describes, and a new test covers the block-with-remaining-0 case that nothing exercised. The block wording is also matched from 'blocked from content creation' alone, in both the throttle test and the block test. Broadening only the latter would have been inert: a body naming the block without one of the other phrases would not read as a throttle at all, so the call would fail fast and never consult the floor. A 403 saying creation is blocked is a throttle by definition. Reported by the reviewer on this PR.
🤖 ThrillhouseBot — changes since the last review
|
There was a problem hiding this comment.
ThrillhouseBot noted 1 lower-confidence item(s) under Things to double-check in the PR summary (not posted as inline threads):
- LOW: Content-creation wording rules only match the exact noun phrase "blocked from content creation" (
src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiError.java:250)
Input not in the diff: {"message":"You have been temporarily blocked from creating content. Please retry your request again later."} — the same block named with the gerund "creating content" instead of the noun phrase "content creation". Both rules changed by this PR require the literal substring "blocked from content creation":return CONTENT_CREATION_BLOCK.matcher(body).find();(and the THROTTLE_WORDING pattern at the top of the class). A gerund variant contains neither "secondary rate limit" nor "abuse detection" nor the exact phrase, so isThrottled() returns false and the call fails fast with no retry; if the body also says "rate limit exceeded", blocksContentCreation() is false and retryDelay() falls back to the 5s/10s/15s linear schedule — three waits totalling 30s inside the measured 72s window, the exact undershoot this PR exists to remove. The pattern's own comment claims the rule covers "a body that names the block", which is a broader contract than the exact phrase implements. The provided material does not establish whether GitHub emits the gerund variant, so this is a verification request: check GitHub's actual content-creation secondary-limit/abuse-detection message variants; if any variant names the block without the exact phrase, broaden both patterns (e.g. also match "blocked from creating content" or simply the pair "content creation").
java:S4030 — the block-outlasting test recorded every wait into a list it never read; the elapsed counter is what the assertions use, so the list is gone. java:S3415 — the budget test passed the derived product as expected and the constant as actual, which reads as reversed. It now pins the literal 90s on both sides, matching this file's stated convention of writing the bounds out rather than restating whatever the production constants say. Reported by SonarCloud on this PR.
🤖 ThrillhouseBot — changes since the last review
|
There was a problem hiding this comment.
ThrillhouseBot noted 1 lower-confidence item(s) under Things to double-check in the PR summary (not posted as inline threads):
- LOW: CONTENT_CREATION_BLOCK over-matches the generic "secondary rate limit" wording (
src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiError.java:108)
The new patternPattern.compile("(?i)secondary rate limit|abuse detection|blocked from content creation")matches the bare phrase "secondary rate limit", so ANY 403 whose body contains that generic phrase is classified by blocksContentCreation() as the 72-second content-creation block, and every derived wait is floored at CONTENT_CREATION_BLOCK_MIN_DELAY (30s) even when the body never names content creation. Input not in the diff:{"message":"You have exceeded a secondary rate limit. Please wait a few minutes before you try again."}— GitHub's documented generic secondary-limit wording, which applies to any endpoint and is a milder throttle class than the content-creation block the floor and the 90s budget were sized against. For such a 403 the waits become 30s/30s/30s (up to 90s of per-PR dispatcher-slot hold) where the pre-change linear backoff gave 5s/10s/15s, and the test comment modified in this PR — "A throttle that is not a content-creation block keeps the linear backoff: it is the mild case, and slowing down a little is all it asks for" — describes intent the pattern does not implement for the "secondary rate limit" wording. The pattern's own javadoc claims it is "The wording of the block that stops content creation specifically, rather than any other throttle", yet the first alternative is GitHub's generic secondary-limit phrase, not the creation-block wording. Verification request: confirm whether GitHub emits the generic secondary-limit body above on the bot's write path; if it does, scope the first alternative (e.g. require creation-block wording, or keep "abuse detection" and "blocked from content creation" while dropping bare "secondary rate limit" from CONTENT_CREATION_BLOCK) or document the over-wait deliberately. Note the trade-off with the gerund-variant case already tracked as prior finding #2.
|
@thrillhousebot resolved src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiError.java:115 — Creation-block wording with an inserted word bypasses both throttle detection and the 30s floor Declined as deliberate. The pattern covers every observed wording of this block: the measured production body ("…secondary rate limit and have been temporarily blocked from content creation") and GitHub's older "abuse detection" variant; the inserted-word examples are hypothesized, not observed. If such a variant ever appeared, realistic bodies still carry "secondary rate limit", so THROTTLE_WORDING classifies it a throttle and the call degrades to the linear backoff — pre-#723 behavior, not fail-fast. The exact-phrase adjacency is a documented security choice: the error body is attacker-influenced text, and loosening it re-opens the over-match the previous round closed. Generated by Claude Code |
|
The next review will close every previous finding this comment names by its |
|
🤖 ThrillhouseBot — changes since the last review
|
There was a problem hiding this comment.
ThrillhouseBot found no issues in this PR, but some checks are still pending or failed:
- Check dependency-review is pending
- Check format is pending
- Check trivy is pending
- Check frontend is pending
- Check test is pending
Additionally, No new issues in this revision, but 1 previous finding(s) remain unresolved — fix them, or reply on their review thread with why they are deferred. A finding listed only under "Things to double-check" has no thread: clear it by commenting @thrillhousebot resolved path/to/File.java:42 — <the finding's title> on this PR.
|
@thrillhousebot resolved src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiError.java:220 — Content-creation floor bypassed when the primary quota also reads 0 Resolved by a later revision. The flagged branch — Generated by Claude Code |
|
The next review will close every previous finding this comment names by its |
|
/review Generated by Claude Code |
🤖 ThrillhouseBot — changes since the last review
|
There was a problem hiding this comment.
ThrillhouseBot noted 1 lower-confidence item(s) under Things to double-check in the PR summary (not posted as inline threads):
- LOW: 30s floor also covers "abuse detection" wording — verify it is the same long block (
src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiError.java:115)
The new floor silently changes the retry shape for every 403 body containing "abuse detection", not just the measured content-creation wording: CONTENT_CREATION_BLOCK isPattern.compile("(?i)abuse detection|blocked from (?:content creation|creating content)"), andCONTENT_CREATION_BLOCK_MIN_DELAY = Duration.ofSeconds(30)turns the previous linear 5s/10s/15s backoff ("abuse detection" was already in THROTTLE_WORDING) into a flat 30s per attempt. The floor's javadoc justifies this with a factual claim not verifiable from the provided material: "GitHub's older wording for the same block says 'abuse detection'". Input not in the diff:{"message":"You have triggered an abuse detection mechanism. Please wait a few minutes before you try again."}— GitHub's burst/abuse-detection throttle for rapid-fire requests. If that wording is a milder, shorter class (like the generic secondary-limit sentence this code deliberately does NOT floor), every such response now retries 3 times at 30s waits and holds the per-PR dispatcher slot for up to the whole 90s budget, where the linear 5/10/15s would have recovered within ~30s. No test in this diff exercises the floor with an "abuse detection" body — all floor tests use the blocked-from-content-creation wording. Verification request: check GitHub's documented/observed behavior for the abuse-detection message (window duration, Retry-After presence); if it is a milder class, keep "abuse detection" in THROTTLE_WORDING but remove it from CONTENT_CREATION_BLOCK.
|
@thrillhousebot resolved src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiError.java:115 — 30s floor also covers "abuse detection" wording — verify it is the same long block Verified — it is the same block. Before GitHub renamed these limits to "secondary rate limits" (2021), the content-creation block's wording was "You have triggered an abuse detection mechanism and have been temporarily blocked from content creation. Please retry your request again later." — "abuse detection" is the legacy name of this exact block, which is why the javadoc calls it the older wording. Modern api.github.com no longer emits "abuse detection" at all, so the phrase only matches legacy/GHES responses. Worst case if such a response were a milder throttle: the first retry waits 30s instead of 5s — bounded extra latency inside the same 90s budget, against missing a 72s block on the wording that historically named it. Keeping it under the floor is the deliberate trade. Generated by Claude Code |
|
A line number followed by a spaced separator and another number reads as a line range, and a range names no single finding. The one exception is the summary's own row: when an em dash (—) after the line number is followed by the finding's full title exactly as the summary prints it, the next review will still close that finding. Anything else — a real range, a shortened title, or a dotted spelling — clears nothing; name each finding by its |
|
/review |
🤖 ThrillhouseBot — changes since the last review
|
There was a problem hiding this comment.
ThrillhouseBot noted 1 lower-confidence item(s) under Things to double-check in the PR summary (not posted as inline threads):
- MEDIUM: Block-recognition regex is a fixed word-order whitelist; a reworded block message silently drops the 30s floor (
src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiError.java:118)
Anchor:Pattern.compile("(?i)abuse detection|blocked from (?:content creation|creating content)");— the rule that decides whether the 30s floor applies. Its own test states the contract:isRecognisedFromTheGerundWordingToosays "The rule's contract is that it recognises a body naming the block, so it must not turn on which word order GitHub happened to use." The implementation hardcodes exactly two word orders of the creation phrase ('blocked from content creation', 'blocked from creating content') plus 'abuse detection'. Input not in the diff:{"message":"You have exceeded a secondary rate limit and content creation is temporarily blocked. Please retry your request again later."}— the same two halves as the measured #722 body with the block clause reworded. It is still classified as throttled (THROTTLE_WORDING matches 'secondary rate limit'), so the retry does repeat — but blocksContentCreation() returns false, retryDelay falls back to the linear 5s/10s/15s derivation, and the retry spends only ~30s of waiting inside the 72-second window: the exact #722 undershoot this PR fixes, with no error or log change to reveal it. The floor is the only mechanism that makes TOTAL_BUDGET a floor for this failure class, so a reworded clause silently removes the fix. Verification request: enumerate GitHub's actual message variants for the content-creation secondary limit (e.g. 'content creation is temporarily blocked', 'blocked from creating pull request comments', hyphenated 'content-creation') and extend CONTENT_CREATION_BLOCK to cover the ones that exist, or match the block semantics (e.g. 'temporarily blocked' near a creation noun) instead of a fixed phrase list. Confidence is low because no third wording appears in the provided material; the stated contract is the evidence of intent.
|
@thrillhousebot resolved src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiError.java:118 — Block-recognition regex is a fixed word-order whitelist; a reworded block message silently drops the 30s floor Declined as deliberate, on the same grounds as the two prior wording findings, plus one correction: the miss would not be silent. The central premise — "no error or log change to reveal it" — is contradicted by this PR's other half: every rejection now logs the HTTP status and GitHub's verbatim message at warn level, so a reworded block that fell back to the linear delays would put the exact novel wording in front of the operator on the very round it happened. That logging is the designed safety net for unknown wordings: enumerate-and-extend is possible only for wording GitHub actually emits, and the log is what captures it. Matching "block semantics near a creation noun" over attacker-influenced error text is the over-match direction the earlier round deliberately closed. The test's quoted contract sentence covers the two observed word orders of the one observed phrase, not unbounded rewordings; the pattern covers every wording GitHub has been seen to emit for this block since the abuse-detection era. Generated by Claude Code |
|
The next review will close every previous finding this comment names by its |
…ery line terminator (#740) > **Stacked on #738 (`fix/730-retry-after-floor`).** Based against that branch so the diff reads clean; **re-target to `main` once #738 merges**. Only the last commit (`154d172`) belongs to this PR. Note that `ci.yml` triggers on pull requests into `main`/`develop`/`release/**` only, so the full CI run happens on re-target — the gates below were run locally on this exact tree. ## What type of PR is this? - [x] 🐛 Bug fix - [x] 🔒 Security ## Description Two defects in the same method, both in the path that exists to *explain* a failed write. ### 1. Redaction ran before the cap, and the JWT shape is quadratic — the primary fix `readBody` reads the entity with no size bound of its own, and `clean()` redacted the whole of it, capping at 512 characters only afterwards. So the cost of logging one 4xx was set by whatever the configured API host chose to send. `CREDENTIAL_SHAPED_VALUE`'s JWT alternative is `eyJ[\w-]{8,}\.[\w-]{8,}\.[\w-]{8,}`. `[\w-]` excludes `.`, so on a body of repeated `eyJ` each greedy run consumes to end-of-input, fails to find its separator and backtracks one character at a time — from one position in three. That is quadratic, and it was measured as such: | body size | time | |---|---| | 20 000 | 331 ms | | 40 000 | 1 213 ms | | 80 000 | 4 843 ms | | 160 000 | 19 404 ms | | 1.2 MB | ~1 078 623 ms (≈ 18 minutes) | Every doubling costs about 4×. That time is spent on the review's own carrier thread, inside `GitHubApiError.from`, before the retry decision the cleaned body feeds is even reached. The fix is the one the audit called for: **cut, then redact, then cap.** ```java var collapsed = WHITESPACE.matcher(raw).replaceAll(" ").strip(); var bounded = cutTo(collapsed, MAX_BODY_CHARS * 2); var redacted = redactCredentials(bounded); var capped = cutTo(redacted, MAX_BODY_CHARS); return capped.length() < redacted.length() || bounded.length() < collapsed.length() ? capped + "…" : capped; ``` Every regex pass is now bounded at a constant. The pre-cut is twice `MAX_BODY_CHARS` rather than exactly it because redaction only ever *shortens*, so the wider window leaves material to fill the 512-char cap with; past that the output was going to be truncated anyway, and what a wider window would pull into view is more of the token-shaped run being masked out. The ellipsis now marks **either** cut, so a body shortened before redaction is never mistaken for one GitHub sent whole. The surrogate-pair guard that protected the 512 cut is factored into `cutTo` and now protects both cuts. ### 2. `\s` is the ASCII six, so most line terminators survived the collapse `WHITESPACE` was `\\s+`, which java.util.regex reads as `[ \t\n\x0B\f\r]` unless the pattern asks for Unicode character classes. CR and LF being collapsed closes the classic forged-record vector, but NEL (U+0085), LINE SEPARATOR (U+2028), PARAGRAPH SEPARATOR (U+2029), NUL and the ANSI escape all reached the warn line intact — and a log viewer, a terminal, or a JSON/ECS shipper may treat any of them as a record boundary or a screen-control sequence. The class already documents a body as attacker-influenced text on its way to a log file and already pays for a collapse pass on that basis; the pass simply did not cover the class it claimed to. Now `[\\s\\p{IsCc}\\u2028\\u2029]+` — `\p{IsCc}` is the Unicode general category rather than POSIX `\p{Cntrl}`, so it reaches the C1 controls (U+0080–U+009F, NEL among them) as well as C0 and DEL. The strip moved after the collapse so a terminator at either end does not survive as a stray space. ### Not in this PR - **The `finding.file()` interpolation** (`ReviewPublisher.java:752` and `:814`) — the third item on the issue. `ReviewPublisher` is being changed by separate work in this round, so touching it here would conflict; it needs `MarkdownSafe.oneLine` at both warn sites and is left to that change. - **Bounding `readEntity` itself.** The audit lists it as optional. Every regex pass is bounded now, and the remaining cost of a huge body is one linear collapse pass plus the read that already happened. - **A5's "match on more than the log-shaped body".** That is a separate finding about *which* string the retry decision reads, not about the cost of producing it. ## Related Issues Fixes #731 ## How Has This Been Tested? - [x] Unit tests Four tests added to `GitHubApiErrorTest.BodyHandling`, all reusing the audit's probes: - `doesNotScanAWholeOversizedBodyLookingForCredentials` — 200 KB of `eyJ` (probe P2c), bounded at 2 000 ms. The bound is enormously slack against what this costs once the body is cut first; it is sized to fail only on the quadratic, never on a slow machine. - `collapsesTheLineTerminatorsAndControlsThatAreNotAsciiWhitespace` — probe P3b's body, asserted as one clean line. - `stripsALineTerminatorAtEitherEndRatherThanLeavingASpace` - `marksABodyCutBeforeRedactionAsTruncatedEvenWhenTheMaskFitsUnderTheCap` — a 4 000-char bearer value masks to `***`, and the result must still say it was cut. The existing `capsAnOverLongBodySoOneFailureCannotFloodTheLog` and `neverCutsAnOverLongBodyThroughASurrogatePair` are unchanged and still pass, which is what pins the cap and the surrogate guard through the restructuring. ### Verbatim red output on unfixed code ``` [ERROR] Tests run: 15, Failures: 4, Errors: 0, Skipped: 0, Time elapsed: 95.25 s <<< FAILURE! -- in dev.thiagogonzaga.thrillhousebot.github.GitHubApiErrorTest$BodyHandling [ERROR] GitHubApiErrorTest.doesNotScanAWholeOversizedBodyLookingForCredentials cleaning a 200 KB body took 92877ms ==> expected: <true> but was: <false> [ERROR] GitHubApiErrorTest.collapsesTheLineTerminatorsAndControlsThatAreNotAsciiWhitespace expected: <a WARN forged-by-NEL WARN forged-by-LS WARN forged-by-PS NUL [2J ansi> but was: <a?WARN forged-by-NEL ?WARN forged-by-LS ?WARN forged-by-PS NUL ansi> [ERROR] GitHubApiErrorTest.stripsALineTerminatorAtEitherEndRatherThanLeavingASpace expected: <boom> but was: <boom ?> [ERROR] GitHubApiErrorTest.marksABodyCutBeforeRedactionAsTruncatedEvenWhenTheMaskFitsUnderTheCap expected: <***?> but was: <***> ``` (The `?` are the terminal's rendering of the surviving U+0085 / U+2028 / U+2029 and of the ellipsis.) **92 877 ms for one 200 KB body** — this machine is slower than the one the audit measured 24 s on, which only sharpens the point. After the fix the whole 15-test `BodyHandling` nest runs in **3.4 s**, and all four go green: ``` [INFO] Tests run: 15, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 3.393 s -- in dev.thiagogonzaga.thrillhousebot.github.GitHubApiErrorTest$BodyHandling ``` ### Gates - `./mvnw -B spotless:apply` → clean - `./mvnw -B clean compile spotbugs:check spotless:check` → `BugInstance size is 0`, BUILD SUCCESS - `./mvnw -B clean test` → `Tests run: 3290, Failures: 0, Errors: 0, Skipped: 0` - jacoco ∩ `git diff -U0` on this commit's main code → 0 uncovered lines, 0 uncovered branches (both conditions of the ellipsis test and both arms of the surrogate guard are exercised) ## Checklist - [x] My code follows the project's coding standards - [x] I have performed a self-review of my own code - [x] I have added tests that prove my fix is effective or that my feature works - [x] New and existing unit tests pass locally with my changes - [x] I have updated the documentation accordingly - [x] My changes generate no new warnings or errors ## Screenshots / Logs See the verbatim red output above. ## Additional Notes Severity is latent, not live: GitHub's real error bodies are around 300 characters, and no path was found by which a PR author gets GitHub itself to echo hundreds of kilobytes into a 4xx body. It needs a large body from the configured API host — a GHES or reverse-proxy error page, a misconfigured base URL, a compromised endpoint. The redaction half is pre-existing from #704 (v0.6.2); #723 made the same `clean()` output load-bearing for a retry decision, and promoted a debug line to warn, which is what widened the exposure of the second half.
1 participant
What type of PR is this?
Description
Two defects behind the run that produced #712, both measured from the production logs on the round-7
build (
176371a).The window was a secondary rate limit, and the budget could not span it.
GitHubWriteRetryallowed three attempts, and its own javadoc asserted the resulting 60s was "long enough to outlast
the minute-long window GitHub's content-creation secondary limit uses". That was an assumption:
Primary quota nowhere near exhausted, and the block ran 72 seconds, simultaneously on unrelated
pull requests.
Raising the attempt count to four is only half of it, and the reviewer on this PR was right to push
back on the other half: more attempts bound one call's wait from above without making it reach
72s. Both ways of deriving a delay without a
Retry-Afterundershoot badly — the linear fallbackgives
5s + 10s + 15s, and anx-ratelimit-resetalready in the past gives zero, firing everyattempt at once and spending the budget in milliseconds while GitHub is still refusing.
Neither number is GitHub speaking about this block: the reset belongs to the primary window,
which a secondary limit leaves untouched. So a derived delay is floored at 30s for a
content-creation block, which makes
TOTAL_BUDGETa floor for this failure rather than only aceiling. An explicit
Retry-Afterstill wins outright, and a primary window that really is exhaustedkeeps its reset instant.
GitHubWritePacer.DEFAULT_MAX_WAITis documented as the same number and stays a literal: readingit from
TOTAL_BUDGETmakes the two static initializers circular (IC_INIT_CIRCULARITY, caught byspotbugs), since the retry already holds the pacer's
DEFAULT. A test asserts the two are equal.Why none of this was diagnosable. Every rejection reason was logged at debug, which is off in
production. A run recorded only:
and then, in the review body, "could not be anchored to the current diff" — a claim about line
numbers the code was in no position to make. The status that separates a throttle from a rejected
position was never written down, so the 29 rejections in that round are permanently undiagnosable,
and the wording sent two dogfood scorers and then #712 itself hunting an off-by-N that did not exist.
The status and GitHub's own message now reach the warning an operator sees, and when the two attempts
fail differently both reasons are kept: a suggestion block GitHub will not take is a 422 about
the payload, a content-creation block is a 403 about the moment, and reporting only the second names
the payload for a finding a throttle refused.
What this does not claim. The 5 logged throttles do not account for all 29 rejections, and the
rest cannot now be attributed, because their reason was discarded. A probe confirmed position was not
the cause: posting to
c/src/rollup.c:45— reported un-anchorable — succeeds today at the samecommit, path and line. This makes the next occurrence answerable rather than guessing at this one.
Related Issues
Closes #722. Split out of #712; the file-level fallback it logs against shipped in #721.
How Has This Been Tested?
Red proof, against the unfixed budget:
Red proof, against the unfixed delay floor:
Red proof, against the unfixed logging:
The budget test rides out a simulated 72-second block on a clock the recorded waits advance, rather
than asserting an attempt count.
./mvnw -B clean compile spotbugs:check spotless:check→BugInstance size is 0./mvnw -B clean test→Tests run: 3276, Failures: 0, Errors: 0, Skipped: 0Checklist