fix(retain): a zero retry budget must still perform the initial fact-extraction request#3
Conversation
|
CodeAnt AI is reviewing your PR. |
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
Warning Review limit reached
Next review available in: 53 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughChangesFact extraction retry handling
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
codeant-ai
Bot
added
the
size:L
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
hindsight-api-slim/hindsight_api/engine/retain/fact_extraction.py (1)
1307-1313: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign the retry documentation with the preserved outer-loop semantics.
The implementation treats positive N as N outer content-validation attempts, while the comments define N as retries after an initial request (N+1 attempts).
hindsight-api-slim/hindsight_api/engine/retain/fact_extraction.py#L1307-L1313: describeouter_attemptsas the legacy outer attempt count, floored at one.hindsight-api-slim/tests/test_fact_extraction_retry.py#L262-L271: distinguish outer content-validation attempts from provider transport retries.hindsight-api-slim/tests/test_fact_extraction_retry.py#L351-L360: clarify why budget 3 expects three outer calls rather than four.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@hindsight-api-slim/hindsight_api/engine/retain/fact_extraction.py` around lines 1307 - 1313, Align the retry documentation without changing behavior: in hindsight-api-slim/hindsight_api/engine/retain/fact_extraction.py:1307-1313, describe outer_attempts as the legacy outer content-validation attempt count floored at one, while llm_max_retries remains the provider transport-retry setting. In hindsight-api-slim/tests/test_fact_extraction_retry.py:262-271, distinguish outer validation attempts from provider retries; in hindsight-api-slim/tests/test_fact_extraction_retry.py:351-360, clarify that budget 3 produces three outer calls rather than four.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@hindsight-api-slim/hindsight_api/engine/retain/fact_extraction.py`:
- Around line 1307-1313: Align the retry documentation without changing
behavior: in
hindsight-api-slim/hindsight_api/engine/retain/fact_extraction.py:1307-1313,
describe outer_attempts as the legacy outer content-validation attempt count
floored at one, while llm_max_retries remains the provider transport-retry
setting. In hindsight-api-slim/tests/test_fact_extraction_retry.py:262-271,
distinguish outer validation attempts from provider retries; in
hindsight-api-slim/tests/test_fact_extraction_retry.py:351-360, clarify that
budget 3 produces three outer calls rather than four.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: db0fcf28-96c3-4869-b76c-4a637536e375
📒 Files selected for processing (2)
hindsight-api-slim/hindsight_api/engine/retain/fact_extraction.pyhindsight-api-slim/tests/test_fact_extraction_retry.py
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses an issue where a retry budget of 0 resulted in zero extraction requests, ensuring that at least one attempt is always performed. It introduces the outer_attempts variable to manage this logic and adds comprehensive tests to verify retry behavior across various budget configurations. The reviewer provided critical feedback suggesting that the current outer_attempts logic may be inconsistent for budgets greater than 0 and recommended using llm_max_retries + 1 to ensure the outer loop correctly reflects the total number of attempts.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| # A retry budget of N means N retries *after* the initial request (the meaning | ||
| # the providers implement: `range(max_retries + 1)`). This loop counts attempts, | ||
| # so it must floor at 1 — a budget of 0, set when an upstream gateway owns | ||
| # transport retries, still has to perform one extraction request. See #2731. | ||
| # llm_max_retries itself is still forwarded to llm_config.call() below so the | ||
| # provider remains the single owner of transport retries. | ||
| outer_attempts = max(1, llm_max_retries) |
There was a problem hiding this comment.
While this change correctly fixes the issue for a retry budget of 0, the logic for budgets greater than 0 appears to be incorrect and inconsistent.
The variable llm_max_retries is described as a "retry budget", meaning N retries after an initial attempt. This implies a total of N + 1 attempts. The inner llm_config.call() correctly interprets it this way.
However, the proposed logic outer_attempts = max(1, llm_max_retries) results in:
llm_max_retries = 1->outer_attempts = 1-> 1 total attempt (should be 2)llm_max_retries = 3->outer_attempts = 3-> 3 total attempts (should be 4)
This is also reflected in the new test test_malformed_response_still_attempts_then_fails_loudly, where a budget of 1 expects only 1 call.
To make the behavior consistent with the definition of a retry budget, the number of attempts for the outer loop should be llm_max_retries + 1. This would correctly handle all cases, including a budget of 0.
I suggest changing the logic to outer_attempts = llm_max_retries + 1. This will also require updating the expected_calls in test_malformed_response_still_attempts_then_fails_loudly to 2 for a budget of 1, and 4 for a budget of 3.
| # A retry budget of N means N retries *after* the initial request (the meaning | |
| # the providers implement: `range(max_retries + 1)`). This loop counts attempts, | |
| # so it must floor at 1 — a budget of 0, set when an upstream gateway owns | |
| # transport retries, still has to perform one extraction request. See #2731. | |
| # llm_max_retries itself is still forwarded to llm_config.call() below so the | |
| # provider remains the single owner of transport retries. | |
| outer_attempts = max(1, llm_max_retries) | |
| # A retry budget of N means N retries *after* the initial request. | |
| # This loop is for retrying on content errors (e.g. malformed JSON), | |
| # and it counts total attempts, so it must run `llm_max_retries + 1` times. | |
| # This makes its behavior consistent with the inner `llm_config.call()` | |
| # which handles transport-level retries. | |
| # llm_max_retries itself is still forwarded to llm_config.call() below so the | |
| # provider remains the single owner of transport retries. | |
| outer_attempts = llm_max_retries + 1 |
|
CodeAnt AI finished reviewing your PR. |
…extraction request
9c39a21 to
4b7c369
Compare
|
CodeAnt AI is running Incremental review |
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
Bot
added
size:L
|
CodeAnt AI Incremental review completed. |
🤖 CodeAnt AI — Review Status
Updated in place by CodeAnt AI · last 5 reviews |
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
Bot
added
size:L
… retry convention Review feedback on vectorize-io#2779: llm_max_retries=N means N retries *after* the initial request, so N=1 must give 2 total outer attempts. The previous max(1, N) floor under-counted (N=1 -> 1 attempt). Every provider already loops range(max_retries + 1); the outer content-validation loop now follows the same convention, and a zero budget still performs one request (vectorize-io#2731). The raw budget is still forwarded unchanged to llm_config.call().
1 participant
User description
Problem
A retry budget of zero must mean one initial request and no retries. Today
retain_llm_max_retries=0performs zero fact-extraction requests and raisesFact extraction failed after 0 attempts— blocking trusted-gateway deployments where the operator deliberately sets the budget to 0 because the upstream gateway owns transport retries.Fixes vectorize-io#2731
Root cause
Two nested layers read the same knob with different meanings:
OpenAICompatibleLLM.call(max_retries=N)correctly loopsrange(N + 1)— N retries after the initial request (same in the Anthropic and Gemini providers)._extract_facts_from_chunkused the same value as an outer attempt count:for attempt in range(llm_max_retries)→range(0)→ zero requests.flowchart LR A["retain_llm_max_retries = N"] --> B["outer loop (content-quality re-prompts)<br/>OLD: range(N) → N=0 ⇒ 0 attempts ❌<br/>NEW: range(max(1, N)) → N=0 ⇒ 1 attempt ✅"] B --> C["llm_config.call(max_retries=N)<br/>provider transport retries: range(N+1)<br/>(unchanged — provider owns transport)"] C --> D["LLM"]Fix
Floor the outer attempt count at 1 while forwarding the raw budget to the provider unchanged:
max(1, N) == Nfor every N ≥ 1, so existing deployments are byte-for-byte unchanged (budget 3 still means 3 outer × 4 transport worst-case). A review suggestion to useN + 1outer attempts was deliberately not taken: it would silently multiply LLM spend for every configured deployment (budget 3: 12 → 16 worst-case requests) — the comment now spells out the legacy semantics so the two meanings can't be conflated again.Every attempt-count reference is also corrected — retry-decision conditions, warning logs, and both error messages — so a malformed response with budget 0 reports
after 1 attempts, neverafter 0 attempts.How this differs from vectorize-io#2741
vectorize-io#2741 lands the same
max(1, N)floor, and for N ≥ 1 the two patches are behaviorally identical. This PR additionally fixes the error path that actually fires on a malformed response with budget 0: under vectorize-io#2741 the primary raise still reports "after 0 attempts" (only the rarely-reached final fallback message was updated). Coverage is also broader: table-driven tests across N = 0/1/3/unset — including the global-fallback path and the malformed-response contract — versus a single N=0 case; the N=0 malformed-path assertion here fails against vectorize-io#2741's diff as written.How to test
cd hindsight-api-slim uv run pytest tests/test_fact_extraction_retry.py -v uv run ruff check hindsight_api/engine/retain/fact_extraction.py tests/test_fact_extraction_retry.py uv run ruff format --check hindsight_api/engine/retain/fact_extraction.py tests/test_fact_extraction_retry.py uv run ty check hindsight_api/engine/retain/fact_extraction.pyTest evidence
Run post-rebase on upstream
main(eca0fd5a2), squashed branch:pytest tests/test_fact_extraction_retry.py_extract_facts_from_chunkN=0 case)ruff check/ruff format --checkon both touched filesty check hindsight_api/engine/retain/fact_extraction.pyTest plan
max_retries=0forwarded to providerllm_max_retries) → behavior unchangedCodeAnt-AI Description
Keep one fact-extraction attempt when the retry budget is zero
What Changed
Impact
✅ Fact extraction works with zero retry budgets✅ Fewer blocked deployments behind trusted gateways✅ Clearer retry-failure messages💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.