fix(webhook): bound the manual-review authorization GitHub API call on the ACK path (#92) - #164
Merged
Merged
Conversation
…n the ACK path (#92) ManualReviewAuthorizer.hasWriteAccess makes a blocking GitHub call (collaboratorPermission, plus a possible installation-token mint on a cold cache) on the synchronous webhook request thread before the 200 ACK. Under a degraded GitHub this could occupy a webhook worker past GitHub's ~10s delivery SLA. Both calls route through the shared github-api REST client that also fetches large PR diffs on the review path, so a config-level read-timeout would clamp those fetches too. Instead, bound the work in code, scoped to exactly the ACK-path authorization: run the write-access check on a virtual-thread executor and apply a configurable timeout (manual-trigger-auth-timeout, default 5s), failing closed on timeout/interrupt/error as before. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
🤖 ThrillhouseBot PR SummaryWhat this PR doesAdds a configurable timeout for the GitHub write-access permission check during manual-review authorization, executing the check asynchronously on virtual threads so that the webhook acknowledgement thread is not blocked past GitHub's SLA. If the check exceeds the timeout, authorization is denied (fail‑closed). Changes Overview
Risk Assessment
No new issues found in this PR, but the review cannot be approved until the required checks are passing.
|
| Check | Type | Status | Detail |
|---|---|---|---|
| docker-pr | check-run | ⏳ Pending | - |
| test | check-run | ⏳ Pending | - |
| Analyze (java-kotlin) | check-run | ⏳ Pending | - |
Automated review by ThrillhouseBot. Reply with /review to re-run.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #164 +/- ##
============================================
- Coverage 98.99% 98.92% -0.08%
- Complexity 1437 1440 +3
============================================
Files 51 51
Lines 3791 3810 +19
Branches 541 542 +1
============================================
+ Hits 3753 3769 +16
- Misses 8 10 +2
- Partials 30 31 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…erage gate SonarCloud new-code coverage was 78.8% (< 80% gate): the @PreDestroy shutdown, the InterruptedException handler, and the getCause() null branch were uncovered. - Simplify the ExecutionException log to pass the cause as the SLF4J throwable arg, dropping the null-cause ternary (a task ExecutionException always carries a non-null cause) — removes the uncovered condition. - Add tests for the interrupted-while-waiting path and for shutdown(). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![thrillhousebot[bot]](https://avatars.githubusercontent.com/in/3996985?s=60&v=4){kind=small}
…check Moving the write-access check onto a Callable meant any Throwable it threw (including Errors such as OutOfMemoryError) was wrapped in ExecutionException and swallowed as a denied review — the original synchronous code caught only RuntimeException and let Errors propagate. Rethrow the cause when it is an Error to preserve that fail-fast behavior; add a test asserting propagation. Addresses review feedback on #164. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…n check - Use unnamed catch variables (`_`) for the unused TimeoutException and InterruptedException bindings (java:S7467), matching the codebase idiom. - Replace Thread.sleep in the timeout/interrupt tests with a CountDownLatch that keeps the mocked call in flight deterministically (java:S2925); also drops ~4s of wall-clock wait from the suite. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
devops-thiago
added a commit
that referenced
this pull request
Jun 19, 2026
…dit (#165) Follow-ups from the `release/v0.2.0` review of the eight merged feature PRs (#157–#164). The quality gate passed on all of them, but the audit surfaced 7 open Sonar code smells and 7 behavior findings. This PR addresses every one, with tests, and targets `release/v0.2.0`. ## Sonar code smells (now resolved) | Rule | Where | Fix | |------|-------|-----| | S3776 | `WebhookController.handleIssueComment` | Extracted `handleCommentCommand` + `maybeDispatchConversationalMention` to drop cognitive complexity from 17 | | S135 | `MaintainerReplyService.renderThread`, `CommentCommandService.botRootCommentIds` | Collapsed the double `break`/`continue` (do-while paging) | | S2629 | `CommentCommandService` `/summary` logs | Removed the eager `repository(ctx)` string concat from the log args | | S7467 | `CommentCommandService.notifyPaused` | Log the previously-unused `catch` exception | | S5976 | `WebhookControllerTest` | Parameterized the three "ignore review comment" tests into one `@ParameterizedTest` | ## Review findings (fixed) 1. **Reply context pagination** — `MaintainerReplyService` listed PR review comments with the non-paged client (GitHub's 30/page default), so on a busy thread the root comment and prior replies past page one were missed. It now walks pages (100/page, bounded). 2. **Commands in quoted text** — `TriggerDetector` matched `/pause`, `/resolve`, etc. inside fenced code blocks and blockquotes. It now strips fenced code, blockquotes, and inline code before detection, so quoting or documenting a command no longer executes it. 3. **`/resolve` thread cap** — `ReviewThreadService` queried `reviewThreads(first: 100)` with no cursor, so `/resolve` silently skipped bot threads beyond the first 100. It now paginates via `pageInfo`/`after` (bounded). 4. **`/pause` race** — the check-then-insert could throw on a concurrent `/pause` (unique-constraint loser), dropping the confirmation. `/pause` is now idempotent: a row that already exists counts as success and still posts the confirmation; only a genuine failure propagates. 5. **Hard-coded bot logins** — the infinite-loop guard hard-coded two logins, so a different App slug would let the bot answer its own replies. Bot logins are now configurable (`thrillhousebot.github.bot-logins`, env `GITHUB_BOT_LOGINS`), falling back to the built-in logins if empty. 6. **Null head/base NPE** — a `pull_request` event missing `head`/`base` would NPE and silently drop the delivery; `WebhookController` now guards and ignores it. 7. **Non-numeric `GITHUB_APP_ID`** — passed the fail-fast validator and only failed on the first webhook; `StartupConfigValidator` now rejects it at boot. ## Notes - The bot's own inline findings on the original PRs were already validated by the maintainer (5 fixed, 5 declined). The `/pause` race here is the one decline ("premise doesn't hold") that was valid — the async `/pause` path runs on the unbounded virtual-thread executor, so concurrent commands can race. Fixed with bounded impact. - The four "declined false positives" in #159 were independently re-verified as correct (GitHub review threads are flat; arrow-case yields the boolean; non-PR mentions are guarded) and are **not** changed here. ## Testing - Full suite: **1160 tests, 0 failures**, `spotless:check` clean (run with `-Djacoco.skip=true -Dquarkus.jacoco.enabled=false` per the JDK 25 JaCoCo issue). - New/updated tests cover: app-id numeric validation, fenced/quoted command stripping, configurable + empty-fallback bot logins, GraphQL thread pagination, reply-comment pagination, the `/pause` race (both branches), and the null head/base guard.
This was referenced Jun 19, 2026
Open
devops-thiago
added a commit
that referenced
this pull request
Jun 19, 2026
…ance (#168) Adds `.github/thrillhousebot.md` — the per-repo review-instructions file the bot's own `instructions-file` config reads first, which this repository was not dogfooding. The seed set comes directly from the v0.2.0 review retrospective (PRs #157–#165), targeting the bot's observed failure modes on its own code: - **Platform facts** that caused false positives — *GitHub review threads are flat* (the bot raised a now-declined "nested replies" finding twice on #159); *list endpoints default to 30/page*. - **Review heuristics** for recurring misses — paginate GitHub collection fetches (missed on #159/#160, see #166); command parsers must ignore fenced/quoted text (missed on #160); validators must check format not just presence (missed on #157); new config keys must be documented (missed on #164). - **House conventions** — identify the bot's own comments via `TriggerDetector.isBotComment(...)` not a hardcoded login (#165); guard nullable payload fields like `head`/`base` consistently (#158). This is the cheap, immediate mitigation that pays off on the next review, ahead of the deeper prompt/verifier work tracked in #112 / #117 / #123 / #166 and the automated learnings in #38. Closes #167.
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.
What type of PR is this?
Description
ManualReviewAuthorizer.hasWriteAccessmakes a blocking GitHub call —collaboratorPermission, plus a possible installation-token mint on a cold cache — on the synchronous webhook request thread before the200ACK is returned (WebhookController.handleIssueComment→isAuthorized). GitHub expects the webhook ACK within ~10s; under a degraded GitHub a single authorized/reviewcould occupy a webhook worker thread past that SLA, risk a redelivery, and tie up the request pool.This PR adds an explicit, short, configurable timeout to that authorization work so it fails fast on the ACK path.
Why an in-code timeout rather than a REST-client
read-timeoutBoth blocking calls route through the
github-apiREST client — the same client that fetches large PR diffs (getPullRequestFiles/compareCommits) on the review path. Settingquarkus.rest-client."github-api".read-timeoutto a few seconds would also clamp those large-diff fetches, risking spurious review failures. So the bound is applied in code, scoped to exactly the ACK-path authorization and covering both the token mint and the permission call.What changed
ManualReviewAuthorizerruns the write-access check on a dedicated virtual-thread executor (matching the existingReviewExecutorProduceridiom) and appliesFuture.get(timeout). It still fails closed on missing permission / any API error — same behavior as before — and now also on timeout or interrupt. The executor is torn down via@PreDestroy.ThrillhouseConfig.ReviewConfiggainsmanual-trigger-auth-timeout(Duration, default 5s).application.propertiessurfaces it as theMANUAL_TRIGGER_AUTH_TIMEOUTenv override.The negative
author_associationpre-filter (zero-API rejection of non-write associations) and the allowlist short-circuit are untouched, so the common abuse path still makes no API call. Automaticpull_requestreviews are unaffected.Related Issues
Fixes #92
Refs #85, #70
How Has This Been Tested?
Unit tests
Integration tests
Manual testing
New
ManualReviewAuthorizerTest.shouldFailClosedWhenPermissionCheckExceedsTimeout: with a 100ms timeout and a permission call that would otherwise answer "admin" after 2s, the check abandons it and denies.ManualReviewAuthorizerTest+WebhookControllerTest: 45 tests, 0 failures (run with-Djacoco.skip=true -Dquarkus.jacoco.enabled=falseon JDK 25)../mvnw spotless:checkpasses.Checklist
Additional Notes
The timeout is operator-tunable (
MANUAL_TRIGGER_AUTH_TIMEOUT, default5s) and bounds the entire authorization round-trip on the webhook acknowledgement thread, including a cold-cache installation-token mint.