From 54ced966a1c9e0f264cd30e12f9d1e6e3015d828 Mon Sep 17 00:00:00 2001 From: Thiago Gonzaga Date: Mon, 10 Aug 2026 23:54:57 +0000 Subject: [PATCH] fix(review): meter and ceiling-gate the finding-verifier AI call (#514) The token spend ceiling (REVIEW_MAX_TOKENS_PER_REVIEW) is documented as covering every AI call a review makes, but the finding-verifier call - a billed concise-model call made once per batch and once on the single-call path, on by default - was neither metered nor gated: its blocking call never runs under the streaming path's ReviewSessionContext bind, so the observability listener dropped its usage, and nothing on the verifier path consulted the ledger before calling. FindingVerificationService now takes the review's ledger key: it records the call's Result-reported usage straight into the ReviewTokenLedger (before unwrapping, so a truncated-but-billed response still counts), and once the ceiling is reached it skips the call fail-open - the unverified findings are kept and the skip is logged naming the ceiling, so a review at its ceiling degrades in quality but is never lost and never bills another verifier call. The deterministic hedged-finding demotion still runs on the skip path; it costs no tokens. --- .../review/FindingPipeline.java | 20 ++- .../review/ai/FindingVerificationService.java | 72 ++++++-- .../review/ai/ReviewTokenLedger.java | 4 +- .../review/FindingPipelineTest.java | 62 ++++++- .../review/ReviewOrchestratorTest.java | 4 +- .../ai/FindingVerificationServiceTest.java | 161 +++++++++++++++--- .../review/eval/PromptEvalTest.java | 4 +- 7 files changed, 278 insertions(+), 49 deletions(-) diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/FindingPipeline.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/FindingPipeline.java index 1a87c3d4..569cc21a 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/FindingPipeline.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/FindingPipeline.java @@ -405,7 +405,7 @@ private void retryBatch( // salvage-or-disclose step as a parallel-pass truncation, and no further retry (#495). outcomesByIndex[index] = salvageTruncatedBatch( - index, batches, promptInputs, plan, previousFilesById, truncation.get()); + session, index, batches, promptInputs, plan, previousFilesById, truncation.get()); return; } // Soft-fail like the on-request generators (DocGenerationService / PrImprovementService): @@ -451,7 +451,7 @@ private void joinBatchOutcomes( // the complete leading findings out of the buffered partial body first (#500). outcomesByIndex[i] = salvageTruncatedBatch( - i, batches, promptInputs, plan, previousFilesById, truncation.get()); + session, i, batches, promptInputs, plan, previousFilesById, truncation.get()); } else if (isSpendCeilingBlocked(e)) { // Deterministic like a truncation: the ledger is monotonic within a review, so a retry // would be refused identically. Degrade like the budgeter — disclose, with the ceiling @@ -487,16 +487,19 @@ private BatchOutcome processBatch( var batchInputs = withDiff(promptInputs, PromptTemplateEscaper.fence(batch.text()), ""); var batchResponse = aiReviewService.reviewBatch(session, batchInputs, index + 1, batches.size()); - return refineBatchOutcome(index, batch, batchInputs, batchResponse, plan, previousFilesById); + return refineBatchOutcome( + session, index, batch, batchInputs, batchResponse, plan, previousFilesById); } /** * Runs one batch's raw response through the per-batch chain — quote validation and framework * filtering against the batch's own in-budget text, verification, and status scoping. Shared by * the parsed path and the salvage path, so salvaged findings face exactly the checks a normally - * parsed batch's findings do. + * parsed batch's findings do. The session keys the spend ledger for the verification call, which + * is metered and ceiling-gated inside {@link FindingVerificationService}. */ private BatchOutcome refineBatchOutcome( + ReviewSession session, int index, DiffBudgetPlanner.DiffBatch batch, AiReviewService.PromptInputs batchInputs, @@ -507,6 +510,7 @@ private BatchOutcome refineBatchOutcome( validated = frameworkFilter.filter(validated, batch.text()); var verified = findingVerificationService.verify( + ledgerSessionId(session), validated, batchInputs.diff(), batchInputs.projectStack(), @@ -526,9 +530,11 @@ private BatchOutcome refineBatchOutcome( * When nothing salvages (the cut landed before the first element closed, or the lane carried no * body), it falls back to the pre-#500 behaviour: the files are disclosed as not reviewed. * Replaces disclosure only — the truncation was already refused a retry (#495), and salvage makes - * no AI call of its own, so #509's ceiling accounting is untouched. + * no AI call of its own; the verification call it funnels salvaged findings into is metered and + * ceiling-gated like any other, so #509's ceiling accounting is untouched. */ private BatchOutcome salvageTruncatedBatch( + ReviewSession session, int index, List batches, AiReviewService.PromptInputs promptInputs, @@ -557,7 +563,8 @@ private BatchOutcome salvageTruncatedBatch( var batchInputs = withDiff(promptInputs, PromptTemplateEscaper.fence(batch.text()), ""); var partialResponse = new ReviewResponse(salvaged.findings(), salvaged.previousFindingsStatus(), null); - return refineBatchOutcome(index, batch, batchInputs, partialResponse, plan, previousFilesById); + return refineBatchOutcome( + session, index, batch, batchInputs, partialResponse, plan, previousFilesById); } private static List filenamesOf(List files) { @@ -1028,6 +1035,7 @@ private ReviewResponse refine( aiResponse = deduplicator.dedupe(aiResponse); aiResponse = findingVerificationService.verify( + ledgerSessionId(session), aiResponse, promptInputs.diff(), promptInputs.projectStack(), diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/FindingVerificationService.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/FindingVerificationService.java index 147c8168..2feb1918 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/FindingVerificationService.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/FindingVerificationService.java @@ -17,6 +17,7 @@ import com.fasterxml.jackson.annotation.JsonProperty; import com.fasterxml.jackson.databind.ObjectMapper; +import dev.langchain4j.service.Result; import dev.thiagogonzaga.thrillhousebot.config.ThrillhouseConfig; import dev.thiagogonzaga.thrillhousebot.review.Confidence; import dev.thiagogonzaga.thrillhousebot.review.PromptTemplateEscaper; @@ -38,6 +39,14 @@ * *

Fails open by design — any verifier error keeps the original findings, so a broken or slow * verification call can degrade quality but never lose a review. + * + *

The verifier is a billed review-path call, so it participates in the {@code + * REVIEW_MAX_TOKENS_PER_REVIEW} spend ceiling like every other call the review makes: its {@link + * Result#tokenUsage() provider-reported usage} is recorded straight into the {@link + * ReviewTokenLedger} (the blocking call never passes through the streaming path's {@link + * ReviewSessionContext} bind, so the observability listener cannot correlate it), and once the + * ceiling is reached the call is skipped fail-open — the unverified findings are kept, which is + * exactly this service's error contract. */ @ApplicationScoped public class FindingVerificationService { @@ -45,13 +54,18 @@ public class FindingVerificationService { private final FindingVerifier verifier; private final ThrillhouseConfig config; private final ObjectMapper mapper; + private final ReviewTokenLedger tokenLedger; @Inject public FindingVerificationService( - FindingVerifier verifier, ThrillhouseConfig config, ObjectMapper mapper) { + FindingVerifier verifier, + ThrillhouseConfig config, + ObjectMapper mapper, + ReviewTokenLedger tokenLedger) { this.verifier = verifier; this.config = config; this.mapper = mapper; + this.tokenLedger = tokenLedger; } private static final Pattern HEDGING = @@ -62,23 +76,44 @@ public FindingVerificationService( /** * Audits the response's findings; {@code diff}, {@code projectStack} and {@code previousFindings} * must already be escaped for prompt templating, the same values handed to the review call. - * Previous findings let the verifier reject re-raises of answered findings. + * Previous findings let the verifier reject re-raises of answered findings. {@code + * ledgerSessionId} is the review's {@link ReviewTokenLedger} key ({@link + * ReviewTokenLedger#keyFor}): the call's usage is recorded against it, and once the review's + * spend ceiling is reached the call is skipped fail-open, keeping the unverified findings. */ public ReviewResponse verify( - ReviewResponse response, String diff, String projectStack, String previousFindings) { + long ledgerSessionId, + ReviewResponse response, + String diff, + String projectStack, + String previousFindings) { ReviewResponse screened = demoteHedgedBlockingFindings(response); if (!config.review().verifierEnabled() || screened.findings().isEmpty()) { return screened; } + if (tokenLedger.ceilingReached(ledgerSessionId)) { + // The verifier is a fresh billed call; once the ceiling is reached it is not made. Skipping + // fail-open keeps the unverified findings — degraded quality, never a lost review. + Log.warnf( + "Finding verification for review session %d skipped at the review's token spend ceiling" + + " (%d tokens spent, ceiling %d — REVIEW_MAX_TOKENS_PER_REVIEW); keeping the %d" + + " unverified finding(s)", + ledgerSessionId, + tokenLedger.tokensSpent(ledgerSessionId), + tokenLedger.ceiling(), + screened.findings().size()); + return screened; + } try { - var raw = - AiResponses.textOrThrowOnTruncation( - verifier.verify( - PromptTemplateEscaper.escape(renderCandidates(screened.findings())), - diff, - projectStack, - previousFindings == null ? "" : previousFindings), - "Finding verification"); + var result = + verifier.verify( + PromptTemplateEscaper.escape(renderCandidates(screened.findings())), + diff, + projectStack, + previousFindings == null ? "" : previousFindings); + // Meter before unwrapping: a truncated response was still billed, so its spend counts. + recordVerifierUsage(ledgerSessionId, result); + var raw = AiResponses.textOrThrowOnTruncation(result, "Finding verification"); var verdicts = mapper.readValue(ReviewResponseParser.extractJson(raw), VerificationResponse.class); return apply(screened, verdicts); @@ -92,6 +127,21 @@ public ReviewResponse verify( } } + /** + * Records the verifier call's provider-reported usage in the review's ledger. The blocking AI + * service returns usage on its {@link Result}, so it is recorded here directly — the + * observability listener only correlates calls made under the streaming path's session bind, + * which this call never is. A missing result or usage (a provider that omits it) records nothing; + * the ledger itself tolerates a null side of the count. + */ + private void recordVerifierUsage(long ledgerSessionId, Result result) { + var usage = result == null ? null : result.tokenUsage(); + if (usage == null) { + return; + } + tokenLedger.recordUsage(ledgerSessionId, usage.inputTokenCount(), usage.outputTokenCount()); + } + /** * Deterministic guard that runs even when the AI verifier is disabled or fails: a * blocking-eligible finding whose own wording hedges ("may", "might", "could"...) is by diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/ReviewTokenLedger.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/ReviewTokenLedger.java index 2f2da981..e2f95208 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/ReviewTokenLedger.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/ReviewTokenLedger.java @@ -33,7 +33,9 @@ * pinned by {@code StreamingChatModelListenerOrderingTest} guarantees the listener's {@code * onResponse} runs before the call's own completion handler resolves — so by the time the review * path decides whether to make its next call, the previous call's spend is already in the - * ledger. + * ledger. The one review-path call that never runs under that bind — the blocking finding-verifier + * call — records its {@code Result}-reported usage directly from {@link + * FindingVerificationService}, which also gates it on {@link #ceilingReached}. * *

Entries follow an open/record/clear lifecycle keyed by session id: {@link * dev.thiagogonzaga.thrillhousebot.review.FindingPipeline} opens the entry before its first call diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/FindingPipelineTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/FindingPipelineTest.java index d6f4b128..92ce0651 100644 --- a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/FindingPipelineTest.java +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/FindingPipelineTest.java @@ -25,6 +25,8 @@ import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyInt; +import static org.mockito.ArgumentMatchers.anyLong; +import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.lenient; import static org.mockito.Mockito.mock; @@ -93,8 +95,8 @@ void setUp() { when(quoteValidator.validate(any(), any())).thenAnswer(inv -> inv.getArgument(0)); when(frameworkFilter.filter(any(), any())).thenAnswer(inv -> inv.getArgument(0)); when(deduplicator.dedupe(any())).thenAnswer(inv -> inv.getArgument(0)); - when(findingVerificationService.verify(any(), any(), any(), any())) - .thenAnswer(inv -> inv.getArgument(0)); + when(findingVerificationService.verify(anyLong(), any(), any(), any(), any())) + .thenAnswer(inv -> inv.getArgument(1)); when(followUpAnalyzer.dropRepliedDuplicates(any(), any(), any(), any())) .thenAnswer(inv -> inv.getArgument(0)); lenient() @@ -200,7 +202,7 @@ void multiCallReviewsEachBatchAggregatesAndSummarizes() { verify(aiReviewService).reviewBatch(eq(session), any(), eq(1), eq(2)); verify(aiReviewService).reviewBatch(eq(session), any(), eq(2), eq(2)); verify(aiReviewService).summarize(eq(session), any()); - verify(findingVerificationService, times(2)).verify(any(), any(), any(), any()); + verify(findingVerificationService, times(2)).verify(anyLong(), any(), any(), any(), any()); assertEquals(2, result.findings().size()); assertSame(summary, result.summary()); @@ -283,7 +285,7 @@ void multiCallSalvagesTheCompleteFindingsFromATruncatedBatchResponse() { // #495's no-retry stands: salvage replaces the disclose step, never re-enters the retry lane. verify(aiReviewService, times(1)).reviewBatch(eq(session), any(), eq(1), anyInt()); // The salvaged findings run the same validate/verify chain as any batch's. - verify(findingVerificationService, times(2)).verify(any(), any(), any(), any()); + verify(findingVerificationService, times(2)).verify(anyLong(), any(), any(), any(), any()); assertEquals(4, result.findings().size()); assertEquals("S1", result.findings().get(0).title()); @@ -1254,6 +1256,58 @@ void billedThenRefusedBatchesDiscloseInsteadOfClaimingNoCallsWereMade() { verify(aiReviewService, never()).summarize(eq(session), any()); } + @Test + void verifierAiCallsAreSkippedOnceTheSpendCeilingIsReached() { + // #514: the finding verifier is a billed AI call the review makes once per batch, so the + // ceiling's "once reached no further call is made" contract covers it too. Wires a real + // FindingVerificationService over a mock FindingVerifier so the assertion sits on the actual + // AI seam: past the ceiling the verifier must never fire, while the skip fails open — each + // batch keeps its unverified findings and the review completes with the counts-only summary. + var thrillhouseConfig = mock(dev.thiagogonzaga.thrillhousebot.config.ThrillhouseConfig.class); + var reviewConfig = + mock(dev.thiagogonzaga.thrillhousebot.config.ThrillhouseConfig.ReviewConfig.class); + when(thrillhouseConfig.review()).thenReturn(reviewConfig); + when(reviewConfig.verifierEnabled()).thenReturn(true); + var findingVerifier = mock(dev.thiagogonzaga.thrillhousebot.review.ai.FindingVerifier.class); + var realVerificationService = + new FindingVerificationService( + findingVerifier, thrillhouseConfig, new ObjectMapper(), tokenLedger); + var p = + new FindingPipeline( + aiReviewService, + quoteValidator, + frameworkFilter, + deduplicator, + realVerificationService, + followUpAnalyzer, + new ObjectMapper(), + BotIdentity.from(List.of("thrillhousebot[bot]")), + budgetPlanner, + new TokenCounter(), + tokenLedger, + new dev.thiagogonzaga.thrillhousebot.review.ai.TruncatedResponseSalvager( + new ObjectMapper())); + var session = persistedSession(); + var ctx = reviewContext(); + var template = new AiReviewService.PromptInputs("d", "ctx", "base", "stack", "tests", "", ""); + when(aiReviewService.reviewBatch(eq(session), any(), eq(1), anyInt())) + .thenReturn(new ReviewResponse(List.of(finding("a.java", "A")), List.of(), null)); + when(aiReviewService.reviewBatch(eq(session), any(), eq(2), anyInt())) + .thenReturn(new ReviewResponse(List.of(finding("b.java", "B")), List.of(), null)); + when(tokenLedger.ceilingReached(42L)).thenReturn(true); + // ceilingReached=true implies spent >= a positive ceiling — stub the state consistently. + when(tokenLedger.tokensSpent(42L)).thenReturn(106_000L); + when(tokenLedger.ceiling()).thenReturn(100_000L); + + var result = p.run(session, template, ctx, multiBatchPlan(), new DiffLineResolver(Map.of())); + + // No billed verifier call is made past the ceiling (the summary call stays refused too)... + verify(findingVerifier, never()).verify(anyString(), anyString(), anyString(), anyString()); + verify(aiReviewService, never()).summarize(any(), any()); + // ...and the skip fails open: both batches' unverified findings survive. + assertEquals(2, result.findings().size()); + } + @Test void aDisabledCeilingLeavesTheMultiCallPathUntouched() { // #499(d) characterization: with the default REVIEW_MAX_TOKENS_PER_REVIEW=0 a review behaves diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewOrchestratorTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewOrchestratorTest.java index 58407e62..7421a984 100644 --- a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewOrchestratorTest.java +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ReviewOrchestratorTest.java @@ -197,8 +197,8 @@ diffFormatter, new TokenCounter(), config, new ActiveModelSettings(config, "m")) when(projectStackResolver.resolve(any(), any(), any(), anyLong())).thenReturn(""); when(summaryGenerator.generate(anyInt(), anyInt(), anyInt(), any(), any(), any())) .thenReturn(""); - when(findingVerificationService.verify(any(), any(), any(), any())) - .thenAnswer(invocation -> invocation.getArgument(0)); + when(findingVerificationService.verify(anyLong(), any(), any(), any(), any())) + .thenAnswer(invocation -> invocation.getArgument(1)); when(followUpAnalyzer.dropRepliedDuplicates(any(), any(), any(), any())) .thenAnswer(invocation -> invocation.getArgument(0)); lenient().when(followUpAnalyzer.parsePreviousResponses(any())).thenReturn(List.of()); diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ai/FindingVerificationServiceTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ai/FindingVerificationServiceTest.java index 182b895b..f83f0184 100644 --- a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ai/FindingVerificationServiceTest.java +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ai/FindingVerificationServiceTest.java @@ -22,6 +22,9 @@ import static org.mockito.Mockito.*; import com.fasterxml.jackson.databind.ObjectMapper; +import dev.langchain4j.model.output.FinishReason; +import dev.langchain4j.model.output.TokenUsage; +import dev.langchain4j.service.Result; import dev.thiagogonzaga.thrillhousebot.config.ThrillhouseConfig; import java.util.List; import org.junit.jupiter.api.BeforeEach; @@ -32,12 +35,17 @@ class FindingVerificationServiceTest { + /** The review's ledger key, as {@link ReviewTokenLedger#keyFor} would produce it. */ + private static final long SESSION = 42L; + @Mock private FindingVerifier verifier; @Mock private ThrillhouseConfig config; @Mock private ThrillhouseConfig.ReviewConfig reviewConfig; + @Mock private ReviewTokenLedger tokenLedger; + private FindingVerificationService service; @BeforeEach @@ -45,7 +53,33 @@ void setUp() { MockitoAnnotations.openMocks(this); when(config.review()).thenReturn(reviewConfig); when(reviewConfig.verifierEnabled()).thenReturn(true); - service = new FindingVerificationService(verifier, config, new ObjectMapper()); + service = new FindingVerificationService(verifier, config, new ObjectMapper(), tokenLedger); + } + + /** Swaps in a real, opened ledger so a test can observe the spend the service records. */ + private ReviewTokenLedger realLedger(long ceiling) { + when(reviewConfig.maxTokensPerReview()).thenReturn(ceiling); + var ledger = new ReviewTokenLedger(config); + ledger.open(SESSION); + service = new FindingVerificationService(verifier, config, new ObjectMapper(), ledger); + return ledger; + } + + private static Result aiOkWithUsage(String text, int inputTokens, int outputTokens) { + return Result.builder() + .content(text) + .finishReason(FinishReason.STOP) + .tokenUsage(new TokenUsage(inputTokens, outputTokens)) + .build(); + } + + private static Result aiTruncatedWithUsage( + String partialText, int inputTokens, int outputTokens) { + return Result.builder() + .content(partialText) + .finishReason(FinishReason.LENGTH) + .tokenUsage(new TokenUsage(inputTokens, outputTokens)) + .build(); } private static ReviewResponse.Finding finding(String risk, String confidence, String title) { @@ -76,7 +110,7 @@ void shouldDemoteHedgedBlockingFindingsToMediumConfidence() { null, null)); - var result = service.verify(original, "diff", "stack", ""); + var result = service.verify(SESSION, original, "diff", "stack", ""); assertEquals("medium", result.findings().get(0).confidence()); assertEquals("high", result.findings().get(0).risk()); @@ -100,7 +134,7 @@ void shouldDemoteWhenOnlyDescriptionHedgesOrAFieldIsNull() { new ReviewResponse.Finding("high", "high", "h", 3, "May break", null, null, null), new ReviewResponse.Finding("high", "high", "i", 4, "Breaks startup", null, null, null)); - var result = service.verify(original, "diff", "stack", ""); + var result = service.verify(SESSION, original, "diff", "stack", ""); assertEquals("medium", result.findings().get(0).confidence()); assertEquals("medium", result.findings().get(1).confidence()); @@ -128,7 +162,7 @@ void shouldNotDemoteAssertiveBlockingFindingsOrHedgedNonBlockingOnes() { new ReviewResponse.Finding( "critical", "medium", "h", 3, "May break", "Possibly wrong.", null, null)); - var result = service.verify(original, "diff", "stack", ""); + var result = service.verify(SESSION, original, "diff", "stack", ""); assertSame(original, result); } @@ -138,7 +172,7 @@ void shouldSkipVerificationWhenDisabled() { when(reviewConfig.verifierEnabled()).thenReturn(false); ReviewResponse original = response(finding("critical", "high", "Bug")); - var result = service.verify(original, "diff", "stack", ""); + var result = service.verify(SESSION, original, "diff", "stack", ""); assertSame(original, result); verifyNoInteractions(verifier); @@ -148,12 +182,91 @@ void shouldSkipVerificationWhenDisabled() { void shouldSkipVerificationWhenNoFindings() { var original = new ReviewResponse(List.of(), List.of(), null); - var result = service.verify(original, "diff", "stack", ""); + var result = service.verify(SESSION, original, "diff", "stack", ""); + + assertSame(original, result); + verifyNoInteractions(verifier); + } + + @Test + void skipsTheVerifierAiCallOnceTheSpendCeilingIsReached() { + // #514: the verifier is a billed review-path call, so "once reached no further call is made" + // applies to it too. The skip fails open — the unverified findings are kept, never lost. + when(tokenLedger.ceilingReached(SESSION)).thenReturn(true); + ReviewResponse original = response(finding("critical", "high", "Bug")); + + var result = service.verify(SESSION, original, "diff", "stack", ""); assertSame(original, result); verifyNoInteractions(verifier); } + @Test + void hedgedDemotionStillRunsWhenTheCeilingSkipsTheVerifier() { + // The deterministic hedging guard costs no tokens, so the ceiling must not switch it off. + when(tokenLedger.ceilingReached(SESSION)).thenReturn(true); + ReviewResponse original = + response( + new ReviewResponse.Finding( + "high", "high", "f", 1, "May break", "This could fail.", null, null)); + + var result = service.verify(SESSION, original, "diff", "stack", ""); + + assertEquals("medium", result.findings().get(0).confidence()); + verifyNoInteractions(verifier); + } + + @Test + void recordsTheVerifierCallsUsageInTheReviewsLedger() { + // #514: the verifier's spend is real billed usage; the ledger must increase by the + // provider-reported input+output of the call. + var ledger = realLedger(100_000L); + ReviewResponse original = response(finding("critical", "high", "Bug")); + when(verifier.verify(anyString(), anyString(), anyString(), anyString())) + .thenReturn(aiOkWithUsage("{\"verdicts\": []}", 1200, 34)); + + service.verify(SESSION, original, "diff", "stack", ""); + + assertEquals(1234L, ledger.tokensSpent(SESSION)); + } + + @Test + void recordsUsageEvenWhenTheVerifierResponseIsCutShort() { + // A truncated verifier response was still billed: its usage lands in the ledger before the + // truncation is turned into the fail-open path. + var ledger = realLedger(100_000L); + ReviewResponse original = response(finding("critical", "high", "Bug")); + when(verifier.verify(anyString(), anyString(), anyString(), anyString())) + .thenReturn(aiTruncatedWithUsage("{\"verdicts\": [{\"id", 900, 100)); + + var result = service.verify(SESSION, original, "diff", "stack", ""); + + assertSame(original, result); // fails open, findings kept + assertEquals(1000L, ledger.tokensSpent(SESSION)); + } + + @Test + void recordsNothingWhenTheProviderReportsNoUsage() { + ReviewResponse original = response(finding("critical", "high", "Bug")); + when(verifier.verify(anyString(), anyString(), anyString(), anyString())) + .thenReturn(aiOk("{\"verdicts\": []}")); + + service.verify(SESSION, original, "diff", "stack", ""); + + verify(tokenLedger, never()).recordUsage(anyLong(), any(), any()); + } + + @Test + void failsOpenAndRecordsNothingWhenTheVerifierReturnsNoResult() { + ReviewResponse original = response(finding("critical", "high", "Bug")); + when(verifier.verify(anyString(), anyString(), anyString(), anyString())).thenReturn(null); + + var result = service.verify(SESSION, original, "diff", "stack", ""); + + assertSame(original, result); + verify(tokenLedger, never()).recordUsage(anyLong(), any(), any()); + } + @Test void shouldKeepResponseUntouchedWhenAllFindingsConfirmed() { ReviewResponse original = response(finding("critical", "high", "Bug")); @@ -165,7 +278,7 @@ void shouldKeepResponseUntouchedWhenAllFindingsConfirmed() { "confidence": "high", "reason": "verified"}]} """)); - var result = service.verify(original, "diff", "stack", ""); + var result = service.verify(SESSION, original, "diff", "stack", ""); assertSame(original, result); } @@ -188,7 +301,7 @@ void shouldDropRejectedFindingsAndRecountSummary() { ]} """)); - var result = service.verify(original, "diff", "stack", ""); + var result = service.verify(SESSION, original, "diff", "stack", ""); assertEquals(2, result.findings().size()); assertEquals("Real nit", result.findings().get(0).title()); @@ -213,7 +326,7 @@ void shouldDowngradeRiskAndConfidence() { "confidence": "low", "reason": "not verifiable from the diff"}]} """)); - var result = service.verify(original, "diff", "stack", ""); + var result = service.verify(SESSION, original, "diff", "stack", ""); assertEquals(1, result.findings().size()); var downgraded = result.findings().get(0); @@ -234,7 +347,7 @@ void keepsFindingsUnchangedWhenTheVerifierResponseIsCutShort() { when(verifier.verify(anyString(), anyString(), anyString(), anyString())) .thenReturn(aiTruncated("{\"verdicts\": [{\"id\": 1, \"verdict\": \"downgr")); - var result = service.verify(original, "diff", "stack", ""); + var result = service.verify(SESSION, original, "diff", "stack", ""); // Fails open: the unverified finding survives at its original risk/confidence. assertEquals(1, result.findings().size()); @@ -253,7 +366,7 @@ void shouldTolerateRawControlCharsInVerifierResponse() { "{\"verdicts\": [{\"id\": 1, \"verdict\": \"downgraded\", \"risk\": \"medium\"," + " \"confidence\": \"low\", \"reason\": \"guard\tmissing\nhere\"}]}")); - var result = service.verify(original, "diff", "stack", ""); + var result = service.verify(SESSION, original, "diff", "stack", ""); assertEquals("medium", result.findings().get(0).risk()); assertEquals("low", result.findings().get(0).confidence()); @@ -270,7 +383,7 @@ void downgradeShouldNeverRaiseRiskOrConfidence() { "confidence": "high", "reason": "tries to escalate"}]} """)); - var result = service.verify(original, "diff", "stack", ""); + var result = service.verify(SESSION, original, "diff", "stack", ""); var kept = result.findings().get(0); assertEquals("medium", kept.risk()); @@ -287,7 +400,7 @@ void downgradeWithoutRatingsShouldKeepOriginalValues() { {"verdicts": [{"id": 1, "verdict": "downgraded", "reason": "no ratings given"}]} """)); - var result = service.verify(original, "diff", "stack", ""); + var result = service.verify(SESSION, original, "diff", "stack", ""); var kept = result.findings().get(0); assertEquals("high", kept.risk()); @@ -312,7 +425,7 @@ void downgradeWithGarbledRatingsShouldKeepOriginalValues() { ]} """)); - var result = service.verify(original, "diff", "stack", ""); + var result = service.verify(SESSION, original, "diff", "stack", ""); // Garbled labels must not collapse the rating to the lenient-parse default var garbled = result.findings().get(0); @@ -336,7 +449,7 @@ void shouldKeepFindingWhenVerdictDecisionFieldIsMissing() { {"verdicts": [{"id": 1, "reason": "verdict field omitted"}]} """)); - var result = service.verify(original, "diff", "stack", ""); + var result = service.verify(SESSION, original, "diff", "stack", ""); assertSame(original, result); } @@ -355,7 +468,7 @@ void downgradeWithBlankRatingsShouldKeepOriginalValues() { ]} """)); - var result = service.verify(original, "diff", "stack", ""); + var result = service.verify(SESSION, original, "diff", "stack", ""); var blankRisk = result.findings().get(0); assertEquals("critical", blankRisk.risk()); @@ -376,7 +489,7 @@ void shouldKeepFindingsWithoutVerdictOrWithUnknownVerdict() { {"verdicts": [{"id": 2, "verdict": "shrug", "reason": "?"}]} """)); - var result = service.verify(original, "diff", "stack", ""); + var result = service.verify(SESSION, original, "diff", "stack", ""); assertSame(original, result); } @@ -394,7 +507,7 @@ void shouldUseFirstVerdictWhenIdsAreDuplicated() { ]} """)); - var result = service.verify(original, "diff", "stack", ""); + var result = service.verify(SESSION, original, "diff", "stack", ""); assertTrue(result.findings().isEmpty()); } @@ -411,7 +524,7 @@ void shouldParseFencedVerifierOutput() { ``` """)); - var result = service.verify(original, "diff", "stack", ""); + var result = service.verify(SESSION, original, "diff", "stack", ""); assertTrue(result.findings().isEmpty()); } @@ -422,7 +535,7 @@ void shouldFailOpenWhenVerifierThrows() { when(verifier.verify(anyString(), anyString(), anyString(), anyString())) .thenThrow(new RuntimeException("model unavailable")); - var result = service.verify(original, "diff", "stack", ""); + var result = service.verify(SESSION, original, "diff", "stack", ""); assertSame(original, result); } @@ -433,7 +546,7 @@ void shouldFailOpenWhenVerifierReturnsInvalidJson() { when(verifier.verify(anyString(), anyString(), anyString(), anyString())) .thenReturn(aiOk("not json at all")); - var result = service.verify(original, "diff", "stack", ""); + var result = service.verify(SESSION, original, "diff", "stack", ""); assertSame(original, result); } @@ -448,7 +561,7 @@ void shouldHandleNullSummaryWhenRecounting() { {"verdicts": [{"id": 1, "verdict": "rejected", "reason": "fp"}]} """)); - var result = service.verify(original, "diff", "stack", ""); + var result = service.verify(SESSION, original, "diff", "stack", ""); assertTrue(result.findings().isEmpty()); assertNull(result.summary()); @@ -461,7 +574,7 @@ void shouldSendEscapedCandidatesWithIdsAndPassThroughContext() { when(verifier.verify(anyString(), anyString(), anyString(), anyString())) .thenReturn(aiOk("{\"verdicts\": []}")); - service.verify(original, "the-diff", "the-stack", "prior context"); + service.verify(SESSION, original, "the-diff", "the-stack", "prior context"); var candidates = ArgumentCaptor.forClass(String.class); verify(verifier) @@ -482,7 +595,7 @@ void shouldPassEmptyPreviousFindingsWhenNull() { when(verifier.verify(anyString(), anyString(), anyString(), anyString())) .thenReturn(aiOk("{\"verdicts\": []}")); - service.verify(original, "the-diff", "the-stack", null); + service.verify(SESSION, original, "the-diff", "the-stack", null); verify(verifier).verify(anyString(), eq("the-diff"), eq("the-stack"), eq("")); } diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/eval/PromptEvalTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/eval/PromptEvalTest.java index eabc063d..7514a565 100644 --- a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/eval/PromptEvalTest.java +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/eval/PromptEvalTest.java @@ -145,9 +145,11 @@ private String verifierOutcome(EvalCase evalCase) { f.suggestionOld(), f.suggestionNew()); var response = new ReviewResponse(List.of(candidate), List.of(), null); + // No review session exists here; the unopened sentinel key means no ceiling ever gates the + // call and its usage is dropped by the ledger's no-open-entry guard. var verified = findingVerificationService.verify( - response, PromptTemplateEscaper.fence(evalCase.diff()), "", ""); + Long.MIN_VALUE, response, PromptTemplateEscaper.fence(evalCase.diff()), "", ""); if (verified.findings().isEmpty()) { return "rejected"; }