fix(ai): stop retrying a response truncated at the length cap

[devops-thiago]

What type of PR is this?

• 🐛 Bug fix

Description

Fixes #492.

A model that stops on finish_reason: length returns a body cut mid-JSON. Nothing read the finish reason, so a truncation surfaced only as "Model response is not valid review JSON" — indistinguishable from a genuinely malformed response, and therefore routed into the transient-failure retry path.

It is not transient. The same prompt against the same cap is cut at the same point every time. So:

• AiReviewService.runWithRetries retried it max-ai-retries times (default 5) with exponential backoff
• FindingPipeline then treated the exhausted-retries failure as a batch failure and re-ran processBatch, entering that loop again

One over-cap batch therefore cost roughly ten full-price input calls, every one knowably futile, before the batch was dropped and its files disclosed as unreviewed. The only knob that bounds output spend multiplied it instead — and none of those retries counted against REVIEW_MAX_AI_CALLS, which caps only planned calls.

The change

Read finishReason off the ChatResponse and raise a distinct AiResponseTruncatedException. Both retry layers now decline it:

• the service rethrows immediately, without retrying and without broadcasting a retry event to the dashboard (a retry that never happens should not appear in the live feed)
• the pipeline goes straight to the existing disclosure path rather than paying for a second guaranteed truncation

The soft-fail contract is otherwise untouched: successful batches keep their findings, the truncated batch's files are still recorded as uncovered so the verdict holds and the summary discloses the gap.

Two details worth noting for review:

• AiResponseTruncatedException extends AiReviewException deliberately. asAiReviewException returns an AiReviewException cause as-is, so the subclass survives the CompletableFuture unwrapping with its identity intact. Extending RuntimeException instead would have been silently re-wrapped and the guard would never fire.
• isResponseTruncated walks the cause chain (the failure arrives as CompletionException over the service's exception) with a self-reference guard so a malformed chain cannot loop.

The message names max-output-tokens explicitly, so an operator reading the log can see which knob caused it and that leaving it unset falls back to the provider default.

Related Issues

Fixes #492

Companions, not included here: #493 (output-buffer-tokens subtracted from input on separate-budget models) and #494 (the startup rule built on the same shared-window assumption). This one is independent of both — it bites on any model, shared-window or not, for anyone who sets max-output-tokens at all.

How Has This Been Tested?

• Unit tests

New fake TruncatedTokenStream emits a body cut mid-JSON with FinishReason.LENGTH — what a provider actually returns — and counts its own start() calls so a test can assert what a truncation cost.

Service layer, AiReviewServiceTest. Red phase with only the no-retry guard reverted (detection left in place), so the failure isolates the guard:

[ERROR] AiReviewServiceTest.shouldNotRetryAResponseTruncatedAtTheLengthCap:949 Unexpected exception type thrown,
expected: <...AiResponseTruncatedException> but was: <...AiReviewException>
[ERROR] AiReviewServiceTest.shouldNotBroadcastRetryEventsForATruncatedResponse:974 Unexpected exception type thrown,
expected: <...AiResponseTruncatedException> but was: <...AiReviewException>

That generic AiReviewException is the defect visible in one line: the truncation was retried to exhaustion and reported as "AI review failed after N attempts". Green with the guard restored. The first test asserts starts.get() == 1 — exactly one call — and that the parser is never reached.

Batch layer, FindingPipelineTest.multiCallDoesNotRetryABatchTruncatedAtTheModelsLengthCap. Red with the branch disabled:

org.mockito.exceptions.verification.TooManyActualInvocations:
aiReviewService.reviewBatch(
    ReviewSession<null>,
    <any>,
    1,
    <any integer>
);

The second, futile call caught directly. The test also pins the parts that must not change — the successful batch keeps its finding, plan.runtimeUncoveredFiles() still lists the truncated batch's file, and the summary still says a.java (not reviewed.

Gates on the final tree:

• ./mvnw -B spotless:apply / spotless:check — clean
• ./mvnw -B clean compile spotbugs:check — BugInstance size is 0, BUILD SUCCESS
• ./mvnw -B clean test — Tests run: 2429, Failures: 0, Errors: 0, Skipped: 0

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

Additional Notes

Scope, stated plainly. This makes a truncation cheap and legible instead of expensive and mute. It does not yet recover the findings — a truncated batch is still disclosed as unreviewed rather than re-run split smaller or salvaged for whatever parsed. That is a deliberate follow-up: batches are planned up front by DiffBudgetPlanner with token accounting, so re-splitting one mid-flight duplicates that logic and deserves its own change. The cost amplification was the urgent half and it is self-contained.

No behaviour changes for any deployment that does not set max-output-tokens, since no max_tokens is sent and no length stop can occur.

[github-actions]

Dependency Review

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

Scanned Files

None

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[thrillhousebot]

⚠️ ThrillhouseBot review could not be completed.

The review service encountered an error. Please reply with /review or @Thrillhousebot review to retry.

[thrillhousebot]

🤖 ThrillhouseBot PR Summary

What this PR does

Detects responses truncated at the model's max-output-tokens cap via finish_reason=length, raises a distinct AiResponseTruncatedException, and makes both retry layers (AiReviewService and FindingPipeline) decline to re-run the deterministic failing call, disclosing the affected batch's files as not reviewed once instead of paying roughly ten futile billed calls.

Control-Flow Diagram

🔀 Show diagram

flowchart TD
  A["AiReviewService: streaming onCompleteResponse fires"] --> B{"finishReason == LENGTH?"}
  B -- "no" --> C["parse JSON and complete normally"]
  B -- "yes" --> D["completeExceptionally(AiResponseTruncatedException)"]
  D --> E["runWithRetries catches truncation; logs; rethrows, no retry, no broadcast"]
  E --> F["reviewBatch future fails with truncation in cause chain"]
  F --> G{"FindingPipeline: isResponseTruncated walks cause chain?"}
  G -- "yes" --> H["recordUncoveredFiles; skip batch retry; disclose files as not reviewed"]
  G -- "no" --> I["failedIndices; batch retry path"]

Loading

Changes Overview

• Files changed: 6
• Lines added: +324
• Lines removed: -6

Changed Files

File	Change	Summary
src/main/java/dev/thiagogonzaga/thrillhousebot/review/FindingPipeline.java	Modified	Skips the batch retry for truncation-capped failures, records the batch's files as uncovered, and adds a bounded cause-chain walk (MAX_CAUSE_DEPTH=16).
src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/AiResponseTruncatedException.java	Added	New exception type for deterministic length-cap truncations, extending AiReviewException so it survives CompletableFuture unwrapping.
src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/AiReviewService.java	Modified	Detects finish_reason=length in the streaming completion handler and rethrows the new truncation exception without retrying or broadcasting retry events.
src/test/java/dev/thiagogonzaga/thrillhousebot/review/FindingPipelineTest.java	Modified	Adds tests: truncated batch is not retried and files are disclosed; the cyclic cause-chain test fixture is actually acyclic (see finding).
src/test/java/dev/thiagogonzaga/thrillhousebot/review/ai/AiReviewServiceTest.java	Modified	Adds tests: a truncated response is attempted exactly once and no retry/stream-failed events are broadcast.
src/test/java/dev/thiagogonzaga/thrillhousebot/review/ai/TruncatedTokenStream.java	Added	Fake TokenStream emitting a ChatResponse cut mid-JSON with finishReason LENGTH; counts start() calls so tests can assert call cost.

Risk Assessment

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

Things to double-check

3 lower-confidence findings

• MEDIUM: Cycle-guard test fixture is not actually cyclic (src/test/java/dev/thiagogonzaga/thrillhousebot/review/FindingPipelineTest.java:264) (low confidence — verify before acting)
• MEDIUM: Verify OpenAI streaming populates finishReason on ChatResponse (src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/AiReviewService.java:267) (low confidence — verify before acting)
• LOW: Verify AiResponseTruncatedException survives reviewBatch propagation (src/main/java/dev/thiagogonzaga/thrillhousebot/review/FindingPipeline.java:202) (low confidence — verify before acting)

---

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

[thrillhousebot]

ThrillhouseBot noted 3 lower-confidence item(s) under Things to double-check in the PR summary (not posted as inline threads):

• MEDIUM: Cycle-guard test fixture is not actually cyclic (src/test/java/dev/thiagogonzaga/thrillhousebot/review/FindingPipelineTest.java:264)
  multiCallSurvivesACyclicCauseChainOnAFailedBatch claims to test the MAX_CAUSE_DEPTH protection with a cycle 'A caused-by B caused-by A', but the fixture builds a linear chain. The anonymous 'outer' returns tail; the anonymous tail returns head; head is a plain new RuntimeException("head") with no cause, so head.getCause() is null. The chain outer -> tail -> head -> null terminates in 3 steps, so isResponseTruncated returns false without ever reaching the depth bound. The test would pass identically if MAX_CAUSE_DEPTH and the whole bound were deleted, so it provides no protection for the cycle guard this PR highlights as a design detail. Make the fixture genuinely cyclic (e.g. tail.getCause() returns this) so the guard is actually exercised.
• MEDIUM: Verify OpenAI streaming populates finishReason on ChatResponse (src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/AiReviewService.java:267)
  The service-level detection depends entirely on response.finishReason() == FinishReason.LENGTH firing when the real model hits the cap. The new tests prove only the handling code: TruncatedTokenStream fabricates the trigger with ChatResponse.builder().finishReason(FinishReason.LENGTH), so the production path is exercised only against a fake. If the langchain4j/OpenAI streaming integration does not populate finishReason on the ChatResponse delivered to onCompleteResponse (e.g. it arrives null), this changed line never executes under the real trigger: the cut body still fails to parse and still lands in the transient-retry path, making the fix a silent no-op. This is a verification request: run the real OpenAiStreamingChatModel with a capped max_tokens and confirm the completion handler receives finishReason == LENGTH, or add an integration test that reproduces the provider response without fabricating it.
• LOW: Verify AiResponseTruncatedException survives reviewBatch propagation (src/main/java/dev/thiagogonzaga/thrillhousebot/review/FindingPipeline.java:202)
  The pipeline guard only changes behavior when the CompletionException cause chain contains AiResponseTruncatedException. The batch-layer test fabricates this by stubbing aiReviewService.reviewBatch(...) with .thenThrow(new AiResponseTruncatedException(...)), so the real propagation path — service.review up through AiReviewService.reviewBatch and into the CompletableFuture/join() — is never exercised. The PR's design note covers asAiReviewException preserving an AiReviewException cause inside the service, but not whether reviewBatch itself rethrows the exception with its cause chain intact; if reviewBatch replaces it with a new exception that discards the original cause, isResponseTruncated returns false and the truncated batch still receives a futile batch-level retry. Verification request: confirm reviewBatch propagates the truncation with its cause chain intact, or add a test that goes through the real reviewBatch instead of mocking it.

[thrillhousebot]

No new issues in this revision, but 2 previous finding(s) remain unresolved — fix them, or reply on their review thread (where one exists) with why they are deferred.

[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