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"; }