Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ All notable changes to ThrillhouseBot.

### Fixed

- **A claim the verifier rejected is not published by a later round on the same commit** (#711): the second-pass audit reached opposite verdicts on one claim and one unchanged commit within a single dogfood round. It rejected a SQL-injection candidate with correct reasoning β€” the route value is gated by a catalog allow-list before it reaches the query β€” and forty minutes later, on the same commit, published that claim as CRITICAL on a public route. A second audit of the same claim on the same code is not a second opinion; it is the same question put again to a sampled model, and nothing about being later makes the second answer the right one. Nor is a second audit guaranteed to happen: verification fails open, so an empty response body, a timeout or the review's spend ceiling posts the candidate exactly as the reviewer raised it, and production has logged rounds that published a finding no second stage had screened. Rejections are now remembered for the head commit they were reached on, and a later round on that head drops the claim before the verification call instead of putting it to the model again β€” so it also stays dropped on the rounds where no verdict comes back at all. Only rejections are remembered, never confirmations, which means the store can only ever publish fewer findings than before. A push clears the entry: a rejection is an answer about code, and the code changed.
- **The three recurring container defects are graded the same way in every pull request** (#773): a round of dogfood scoring read 65 findings over twelve pull requests against the code β€” 62 of them true β€” and found the grading, not the findings, to be the problem. The same unpinned base image drew medium on four of those pull requests and low on three, with nothing in the changes to tell them apart. A root-owned volume under a non-root `USER` drew low on the pull request where the container dies on its first write and medium on another of the same shape. The same omission drew a confident inline finding in one half of a paired-language change and a low-confidence collapsed item in the other, on evidence that was equally provable from the diff in both. A severity that moves with the review rather than with the defect teaches a reader that the field carries no information. Three classes now carry a stated grade, decided by what the defect costs and who it reaches rather than by the review's impression of it: a mutable external reference (a base image, action or chart named by a tag instead of a digest) is medium, a path the running user cannot write is high, and a container that never drops privilege is medium, each at medium confidence. The review prompt states them, and a calibration stage applies them after the finding verifier, so a downgrade there cannot re-spread the class. It regrades only a finding anchored in a Dockerfile, manifest, compose file, workflow or Terraform file that states the class in its own words, and it leaves alone any finding that also asserts something the class does not cover β€” a privileged container, a host mount or namespace, an added capability, a credential, a named CVE. Nothing is dropped, added or reworded, so the finding set is exactly the one the review produced. Medium confidence puts every anchored finding on the diff instead of in the collapsed "Things to double-check" block, and leaves the merge verdict to the rest of the review: under the default `REVIEW_BLOCKING_STRICTNESS=balanced`, which needs high confidence, no anchored finding requests changes on its own, and one the review over-graded at critical with high confidence stops doing so, which is the calibration working rather than a side effect of it. The prompt also states the mirror of the severity rule for confidence, which is the half no deterministic stage can decide: equally provable defects get equal confidence, and an unequal one must name the fact it could not check
- **Repeated timeouts on one AI call stop spending the whole retry budget** (#862): a streaming attempt waits `THRILLHOUSEBOT_REVIEW_AI_TIMEOUT_SECONDS`, 900 seconds in production, and a timeout was then retried like any other transient failure up to `THRILLHOUSEBOT_REVIEW_MAX_AI_RETRIES`, so one call could spend 75 minutes of wall clock while its review held the pull request's dispatcher slot. Production saw 20 timed-out attempts in 24 hours, all on one 503-file pull request, and the failed reviews of that day cost more than the completed ones. At most two attempts of one call may now end at the deadline: the second one fails the call instead of spending the attempts that are left, and the decision is logged at WARN with the session id, the attempt and the wait. Every other transient failure keeps the whole budget, a timeout followed by a successful attempt still succeeds, and the reasoning step-down's repeat (#839) shares the bound rather than getting a second pair of waits. The repeat keeps the full deadline, since it is there for the attempt whose first token never arrived because the provider queued the request, and the bound already brings the ceiling down from 75 minutes to 30. The setting still means one attempt's wait. The final summary call shares the loop and behaves the same way, and a review whose batches time out still discloses the files it did not read
- **A review interrupted by a restart no longer stays `in_progress` for good** (#863): a session row is written `in_progress` when the review starts and updated when it ends, so a review killed between those two writes β€” a deploy restart, a crash, a `docker kill` β€” never got the terminal one and stayed `in_progress` for the life of the database. Production had 10 such rows, the oldest from 2026-06-09 and the newest from the 2026-09-09 restart, each counting as a running review and hiding the genuinely in-flight ones among them. Startup now moves every `in_progress` row to `failed` with "Review interrupted before it finished (bot restart or crash)" as the reason, which is what tells it apart on the dashboard from a review that failed on its own. Nothing carries a review across a restart, so a row still in progress at boot belongs to a review that is over: the sweep needs no age threshold and reconciles the rows stranded before it existed, with no manual SQL. Only the status and the reason are written, so the tokens and the cost the review had already paid for stay on the row. Sweeping every row at boot is safe because the bot is a single process; running more than one replica would need the rows to carry an owner first
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,7 @@ private static AiReviewService.PromptInputs withDiff(
private final FrameworkFalsePositiveFilter frameworkFilter;
private final FindingDeduplicator deduplicator;
private final FindingVerificationService findingVerificationService;
private final VerifierRejectionMemory rejectionMemory;
private final FollowUpAnalyzer followUpAnalyzer;
private final ObjectMapper mapper;
private final BotIdentity botIdentity;
Expand All @@ -209,6 +210,7 @@ public FindingPipeline(
FrameworkFalsePositiveFilter frameworkFilter,
FindingDeduplicator deduplicator,
FindingVerificationService findingVerificationService,
VerifierRejectionMemory rejectionMemory,
FollowUpAnalyzer followUpAnalyzer,
ObjectMapper mapper,
BotIdentity botIdentity,
Expand All @@ -221,6 +223,7 @@ public FindingPipeline(
this.frameworkFilter = frameworkFilter;
this.deduplicator = deduplicator;
this.findingVerificationService = findingVerificationService;
this.rejectionMemory = rejectionMemory;
this.followUpAnalyzer = followUpAnalyzer;
this.mapper = mapper;
this.botIdentity = botIdentity;
Expand Down Expand Up @@ -767,19 +770,23 @@ private BatchOutcome refineBatchOutcome(
var attached = run.evidence().forFindings(batchResponse.findings());
var validated = quoteValidator.validate(batchResponse, batch.text());
validated = frameworkFilter.filter(validated, batch.text());
// #711: as in the single-call lane β€” a claim the audit already rejected on this head is not
// put to the verifier a second time, and stays dropped on a round that never gets a verdict.
var candidates = rejectionMemory.withoutRejectionsOnThisHead(run.session(), validated);
// #736: the verification call is the one review-path call that does no budget arithmetic of
// its own, so the section the author alone sizes is bounded here before it is sent.
var verified =
findingVerificationService.verify(
ledgerSessionId(run.session()),
validated,
candidates,
PrContextBudget.bound(
batchInputs.prContext(), budgetPlanner.perCallInputBudget(), tokenCounter),
batchInputs.diff(),
batchInputs.projectStack(),
batchInputs.previousFindings(),
attached,
run.plan()::recordVerificationCoverage);
rejectionMemory.remember(run.session(), candidates.findings(), verified.findings());
return new BatchOutcome(
index,
verified.findings(),
Expand Down Expand Up @@ -1435,19 +1442,24 @@ private ReviewResponse refine(
aiResponse = quoteValidator.validate(aiResponse, diff);
aiResponse = frameworkFilter.filter(aiResponse, diff);
aiResponse = deduplicator.dedupe(aiResponse);
// #711: a claim the audit already rejected on this head is dropped before the call rather
// than put to it again. A second verdict on unchanged code is a re-roll of the same question,
// and on the fail-open paths there is no second verdict at all.
var candidates = rejectionMemory.withoutRejectionsOnThisHead(session, aiResponse);
// #736: the verification call is the one review-path call that does no budget arithmetic of
// its own, so the section the author alone sizes is bounded here before it is sent.
aiResponse =
findingVerificationService.verify(
ledgerSessionId(session),
aiResponse,
candidates,
PrContextBudget.bound(
promptInputs.prContext(), budgetPlanner.perCallInputBudget(), tokenCounter),
promptInputs.diff(),
promptInputs.projectStack(),
promptInputs.previousFindings(),
attached,
plan::recordVerificationCoverage);
rejectionMemory.remember(session, candidates.findings(), aiResponse.findings());
// #773: the last word on the two graded fields, so the anchored infrastructure classes cannot
// be re-spread by the verifier's own lowering, and the grade the publisher routes on is the
// one persisted below for the next round to compare against.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -838,8 +838,16 @@ private static boolean sameAnchor(ReviewResponse.Finding finding, ReviewResponse
&& Math.abs(finding.line() - prior.line()) <= DUPLICATE_LINE_TOLERANCE;
}

private static boolean isSameFinding(
ReviewResponse.Finding finding, ReviewResponse.Finding prior) {
/**
* Whether two findings raised in different rounds argue the same defect: the same file, and
* either the same anchor with a similar title or enough shared content to be one claim reworded.
*
* <p>Package-private so {@link VerifierRejectionMemory} recalls a rejection by the rule the
* follow-up passes already recognize a re-raise by (#711). A claim that comes back only because
* the model worded it differently is the same claim, and a second recognizer would let the two
* passes disagree about that.
*/
static boolean isSameFinding(ReviewResponse.Finding finding, ReviewResponse.Finding prior) {
if (finding.file() == null || !FilePaths.same(finding.file(), prior.file())) {
return false;
}
Expand Down
Loading
Loading