fix(review): meter and ceiling-gate the finding-verifier AI call

[devops-thiago]

What type of PR is this?

• 🐛 Bug fix

Description

REVIEW_MAX_TOKENS_PER_REVIEW is documented as a ceiling "on the tokens one review may consume across every AI call it makes", with "Once reached no further call is made". The finding-verifier call — a billed concise-model AI call made once per batch (and once on the single-call path), on by default via REVIEW_VERIFIER_ENABLED — escaped both halves of that contract:

• Not metered: the verifier is a blocking AI-service call that never passes through AiReviewService.streamOnce's ReviewSessionContext bind, so OtelObservabilityListener.onResponse saw no session id in the request attributes and dropped its usage. The ceiling undercounted real spend by the whole verification cost — per-call input on the order of the batch review call itself (the prompt repeats the batch diff, findings JSON, project stack and previous findings).
• Not gated: ensureCallAllowed's only call site is AiReviewService.runWithRetries; nothing on the verifier path consulted the ledger, so a review at its ceiling kept making billed verifier calls (one per batch outcome — parsed and salvaged — plus the single-call path).

Fix (in FindingVerificationService, the seam both call sites share):

• verify now takes the review's ledger key (ReviewTokenLedger.keyFor), threaded from FindingPipeline.refine / refineBatchOutcome.
• Metering: the call's Result#tokenUsage() is recorded straight into the ReviewTokenLedger, before unwrapping — so a truncated-but-billed response still counts. Recording directly (instead of via the listener) is deliberate: the blocking call never runs under the streaming session bind, so there is no double count.
• Gate: once ceilingReached, the verifier call is skipped fail-open — the unverified findings are kept (the service's existing error contract) and the skip is logged at WARN naming REVIEW_MAX_TOKENS_PER_REVIEW. The review is never failed and the deterministic hedged-finding demotion (no AI call) still runs.

No doc changes: the fix makes the existing README / .env.example / application.properties wording ("across every AI call", "once reached no further call is made") true, which is the direction the audit prescribed.

Related Issues

Fixes #514

How Has This Been Tested?

• Unit tests

Red → green proofs (each new behavioral test fails on unfixed code exactly as claimed, then passes with the fix). Mockito's literal angle-bracket matchers are shown as ⟨any⟩ so they survive markdown sanitization.

Gate (pipeline level) — the audit's proof test (Audit3VerifierGateProofTest, 2-batch multi-call plan, ledger at its ceiling for the whole run) with the expectation set to the documented behavior (never()), run on unfixed 5c04600:

org.mockito.exceptions.verification.NeverWantedButInvoked:
findingVerificationService.verify(⟨any⟩, ⟨any⟩, ⟨any⟩, ⟨any⟩);
Never wanted here:
-⟩ at dev.thiagogonzaga.thrillhousebot.review.ai.FindingVerificationService.verify(FindingVerificationService.java:69)
But invoked here:
-⟩ at dev.thiagogonzaga.thrillhousebot.review.FindingPipeline.refineBatchOutcome(FindingPipeline.java:509)  (twice, once per batch)

That test is adapted into FindingPipelineTest.verifierAiCallsAreSkippedOnceTheSpendCeilingIsReached, which wires a real FindingVerificationService over a mock FindingVerifier so the assertion sits on the actual AI seam; against the plumbing without the gate it fails the same way:

FindingPipelineTest.verifierAiCallsAreSkippedOnceTheSpendCeilingIsReached:1305
findingVerifier.verify(⟨any string⟩, ⟨any string⟩, ⟨any string⟩, ⟨any string⟩);
Never wanted here: ...
But invoked here:
-⟩ at dev.thiagogonzaga.thrillhousebot.review.ai.FindingVerificationService.verify(FindingVerificationService.java:96)  (twice)

Gate + metering (service level) — new FindingVerificationServiceTest tests against the unfixed behavior:

skipsTheVerifierAiCallOnceTheSpendCeilingIsReached
org.mockito.exceptions.verification.NoInteractionsWanted:
But found these interactions on mock 'verifier':
-⟩ at dev.thiagogonzaga.thrillhousebot.review.ai.FindingVerificationService.verify(FindingVerificationService.java:96)

recordsTheVerifierCallsUsageInTheReviewsLedger
org.opentest4j.AssertionFailedError: expected: ⟨1234⟩ but was: ⟨0⟩

recordsUsageEvenWhenTheVerifierResponseIsCutShort
org.opentest4j.AssertionFailedError: expected: ⟨1000⟩ but was: ⟨0⟩

All of the above pass with the fix; the metering tests use a real ReviewTokenLedger and assert the ledger increases by the provider-reported input+output. Additional tests cover the ceiling skip preserving the hedged-finding demotion, a Result with no usage, and a null Result (fail-open, nothing recorded).

Gates: spotless:apply clean; clean compile spotbugs:check spotless:check — BugInstance size is 0; clean test — 2573 tests, 0 failures, 0 errors, 0 skipped. Jacoco ∩ git diff -U0 5c04600...HEAD over changed main code: zero uncovered lines, zero uncovered branches.

Checklist

• My code follows the project's coding standards
• I have performed a self-review of my own code
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes
• I have updated the documentation accordingly
• My changes generate no new warnings or errors

Screenshots / Logs

New WARN on the skip path:

Finding verification for review session 42 skipped at the review's token spend ceiling (106000 tokens spent, ceiling 100000 — REVIEW_MAX_TOKENS_PER_REVIEW); keeping the 1 unverified finding(s)

Additional Notes

Scope kept to the verifier seams: FindingVerificationService (gate + metering), FindingPipeline only in refineBatchOutcome/verification call sites (threading the session key through processBatch/salvageTruncatedBatch callers), and the ReviewTokenLedger/salvageTruncatedBatch javadocs whose "fed by the listener" / "ceiling accounting untouched" claims the fix completes. ReviewResult, VerdictBuilder, DiffBudgetPlanner, countsOnlySummary and the disclosure seams are untouched (owned by #518/#515).

[github-actions]

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Scanned Files

None

[thrillhousebot]

🤖 ThrillhouseBot PR Summary

What this PR does

Meters and ceiling-gates the finding-verifier AI call. FindingVerificationService.verify now takes the review's ledger key, records the verifier Result's tokenUsage into ReviewTokenLedger before unwrapping the response, and skips the billed call fail-open (keeping unverified findings and logging a WARN) once REVIEW_MAX_TOKENS_PER_REVIEW is reached. FindingPipeline threads the session ledger key through the batch, salvage, and single-call verification paths so both halves of the documented ceiling contract apply to the verifier.

Control-Flow Diagram

🔀 Show diagram

flowchart TD
  A["FindingPipeline refine / refineBatchOutcome / salvage"] --> B["FindingVerificationService.verify(session, ...)"]
  B --> C["demoteHedgedBlockingFindings"]
  C --> D{"verifierEnabled and findings non-empty?"}
  D -- "no" --> E["return screened findings"]
  D -- "yes" --> F{"ceilingReached(session)?"}
  F -- "yes" --> G["WARN: skip verifier, keep unverified findings"]
  G --> E
  F -- "no" --> H["verifier.verify(...) blocking AI call"]
  H --> I["recordUsage(session, input, output)"]
  I --> J{"response truncated?"}
  J -- "yes" --> K["textOrThrowOnTruncation throws -> catch -> fail-open"]
  K --> E
  J -- "no" --> L["parse verdicts and apply"]
  L --> E

Loading

Changes Overview

• Files changed: 7
• Lines added: +278
• Lines removed: -49

Changed Files

File	Change	Summary
src/main/java/dev/thiagogonzaga/thrillhousebot/review/FindingPipeline.java	Modified	Threads the ReviewSession through processBatch/refineBatchOutcome/salvageTruncatedBatch and passes ledgerSessionId(session) to both verify call sites.
src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/FindingVerificationService.java	Modified	Adds ledger injection, session-key param, ceiling gate (fail-open + WARN), and direct token-usage recording before response unwrapping.
src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/ReviewTokenLedger.java	Modified	Javadoc updated: the blocking verifier call records Result usage directly and is ceiling-gated via ceilingReached.
src/test/java/dev/thiagogonzaga/thrillhousebot/review/FindingPipelineTest.java	Modified	Adds pipeline-level proof that verifier calls are skipped once the ceiling is reached while both batches' findings are kept; updates verify stub arity.
src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewOrchestratorTest.java	Modified	Updates the verify mock stub to the new five-argument signature with pass-through of the response argument.
src/test/java/dev/thiagogonzaga/thrillhousebot/review/ai/FindingVerificationServiceTest.java	Modified	Adds tests: ceiling skip keeps findings, hedged demotion survives skip, ledger records input+output (incl. truncated call), null/absent usage records nothing.
src/test/java/dev/thiagogonzaga/thrillhousebot/review/eval/PromptEvalTest.java	Modified	Passes the Long.MIN_VALUE unopened sentinel ledger key so eval-only verification is never ceiling-gated.

Risk Assessment

Risk	Count
🔴 Critical	0
🟠 High	0
🟡 Medium	0
🔵 Low	0

No new issues found in this PR, but the review cannot be approved until CI is confirmed green.

⚠️ CI Checks Status

Some checks are still pending or have failed:

Check	Type	Status	Detail
changes	check-run	⏳ Pending	-
test	check-run	⏳ Pending	-
actionlint	check-run	⏳ Pending	-
format	check-run	⏳ Pending	-
trivy	check-run	⏳ Pending	-
frontend	check-run	⏳ Pending	-
dependency-review	check-run	⏳ Pending	-

---

Automated review by ThrillhouseBot. Reply with /review to re-run.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud