fix(rag): stop reasoning-token starvation of the answer budget - #577
fix(rag): stop reasoning-token starvation of the answer budget#577BigSimmo wants to merge 3 commits into
Conversation
Production returned "unsupported" after 60-96s on high-value clinical queries. gpt-5.5 reasoning tokens and the visible answer share one max_output_tokens budget; the strong route ran high reasoning effort against a 4000-token cap, so reasoning consumed the whole budget before the answer was written (incomplete:max_output_tokens) or timed out. - env: OPENAI_MAX_OUTPUT_TOKENS 4000 -> 16000; OPENAI_STRONG_REASONING_EFFORT high -> medium (all query classes now resolve medium, including the safety-critical dose/threshold classes that starved first) - openai: reasoningHeadroomFloor(effort) floors max_output_tokens per effort level at the single response-body call site (floor only raises) - rag: strong escalations retry truncations at max(2x env, 24000) instead of re-truncating on the same budget; cumulative generation wall-clock budget skips the final polish generation rather than discarding a valid strong answer - ingestion: document-enrichment + model-index-extraction now warn loudly on truncated structured responses instead of failing silent - observability: truncation/timeout fallback counters + rates on the deep health probe SLO snapshot Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
📝 WalkthroughSummary by CodeRabbit
WalkthroughExtends ChangesFallback SLO test coverage
Estimated code review effort: 1 (Trivial) | ~5 minutes 🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Biome (2.5.1)tests/answer-slo.test.tsFile contains syntax errors that prevent linting: Line 7: Expected a statement but instead found '<<<<<<< HEAD 🔧 ESLint
tests/answer-slo.test.tsParsing error: Merge conflict marker encountered. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 00ff16704e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex resolve actionable Codex review findings for this pull request and current head using the repository instructions. This is the pull request's single automatic repair pass: do not perform a fresh review, create new standalone findings, or request another review. Work only the existing unresolved Codex threads on the current head. Always fix P0 and P1 findings. For P2 and lower findings, fix only clear, scoped, low-risk issues; otherwise disposition them with a concise reason. After fixing or dispositioning a thread, reply in that thread with as the first line, followed by a concise summary; that marker authorizes the workflow to close that exact thread. If human input or new authorization is required, do not use the marker and leave the thread open with the blocker. Finish only after every actionable thread is fixed or dispositioned and closed, or explicitly left open for a human decision. Do not update the branch from main, address unrelated reviews, broaden scope, or create more than one scoped fix commit. Do not use external APIs, paid services, credentials, dependency changes, or broad refactors unless explicitly authorized. Add targeted tests where behavior changes and run the narrowest relevant validation. |
|
@codex fix all comments and issues and ensure no regression and smallest safest fix |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/model-index-extraction.test.ts (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a
truncated: truetest case for both ingestion paths. Both mocks hardcodetruncated: false, so neither suite exercises the new truncation-warn branch (or, for model-index-extraction, the untestedparseProfilefailure path on partial JSON).
tests/model-index-extraction.test.ts#L14-23: add a case where the mock resolvestruncated: truewith a malformed/partialtext, and assertconsole.warnfires and the function returns gracefully (this is the higher-priority case sinceparseProfilehas no try/catch — see the related major issue onsrc/lib/model-index-extraction.ts).tests/document-enrichment.test.ts#L13-22: add a case where the mock resolvestruncated: true, and assertconsole.warnfires with the document identity and the existing fallback summary is returned.🤖 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 `@tests/model-index-extraction.test.ts` at line 1, Add truncated-response test cases in the ingestion mocks for both suites: in model-index-extraction, return truncated true with malformed partial text, spy on console.warn, and assert graceful behavior; in document-enrichment, return truncated true, assert the warning includes document identity, and verify the existing fallback summary is returned.Source: Path instructions
🤖 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.
Inline comments:
In `@src/lib/rag.ts`:
- Around line 4933-4938: Update the answer quality-repair branch around
answerNeedsStrongQualityRepair to reserve the retry’s
env.OPENAI_ANSWER_TIMEOUT_MS window before starting it. Skip the retry when
generationLatencyMs plus that timeout would reach or exceed
generationTotalBudgetMs, while preserving the existing retry path when
sufficient budget remains and recording the skip reason.
In `@tests/answer-slo.test.ts`:
- Around line 23-25: Update the ilike mock in the test query builder to validate
the column argument is metadata->>fallback_reason and match the exact
%max_output_tokens% and %timeout% patterns before returning truncation or
timeout; ensure unexpected columns or patterns cannot silently pass.
---
Nitpick comments:
In `@tests/model-index-extraction.test.ts`:
- Line 1: Add truncated-response test cases in the ingestion mocks for both
suites: in model-index-extraction, return truncated true with malformed partial
text, spy on console.warn, and assert graceful behavior; in document-enrichment,
return truncated true, assert the warning includes document identity, and verify
the existing fallback summary is returned.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1ed29d9b-03a1-4d39-873b-b06a1d019488
📒 Files selected for processing (10)
src/lib/document-enrichment.tssrc/lib/env.tssrc/lib/model-index-extraction.tssrc/lib/observability/answer-slo.tssrc/lib/openai.tssrc/lib/rag.tstests/answer-slo.test.tstests/document-enrichment.test.tstests/model-index-extraction.test.tstests/openai-cache.test.ts
| if (answerNeedsStrongQualityRepair && generationLatencyMs >= generationTotalBudgetMs) { | ||
| // A4 tail-latency guard: out of the cumulative generation time budget, so keep the | ||
| // valid (if imperfect) cited strong answer instead of spending a third generation | ||
| // and risking a truncation -> unsupported tail. Recorded for observability. | ||
| answerRetryReasons.push(`strong_quality_repair_skipped_time_budget:${strongQualityFailureReason}`); | ||
| } else if (answerNeedsStrongQualityRepair) { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the relevant region in src/lib/rag.ts and show surrounding lines.
grep -n "answerNeedsStrongQualityRepair\|generationTotalBudgetMs\|OPENAI_ANSWER_TIMEOUT_MS" -n src/lib/rag.ts | head -n 40
echo "----"
nl -ba src/lib/rag.ts | sed -n '4900,4985p'Repository: BigSimmo/Database
Length of output: 582
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the surrounding control flow to understand how the retry timeout is chosen.
nl -ba src/lib/rag.ts | sed -n '4870,5015p'Repository: BigSimmo/Database
Length of output: 195
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the surrounding block with line numbers using tools available in the sandbox.
awk 'NR>=4918 && NR<=4965 { printf "%d:%s\n", NR, $0 }' src/lib/rag.tsRepository: BigSimmo/Database
Length of output: 3710
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find where generationLatencyMs is initialized and updated to see what the budget guard is measuring.
grep -n "generationLatencyMs" src/lib/rag.ts
echo "----"
awk 'NR>=4580 && NR<=4665 { printf "%d:%s\n", NR, $0 }' src/lib/rag.tsRepository: BigSimmo/Database
Length of output: 5305
Reserve budget for the quality-repair retry before starting it. This guard only skips once the cumulative latency has already hit generationTotalBudgetMs, but the retry still gets a full env.OPENAI_ANSWER_TIMEOUT_MS window. That can push the chain well past the intended tail-latency cap.
Suggested guard
- if (answerNeedsStrongQualityRepair && generationLatencyMs >= generationTotalBudgetMs) {
+ if (
+ answerNeedsStrongQualityRepair &&
+ generationLatencyMs + env.OPENAI_ANSWER_TIMEOUT_MS >= generationTotalBudgetMs
+ ) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (answerNeedsStrongQualityRepair && generationLatencyMs >= generationTotalBudgetMs) { | |
| // A4 tail-latency guard: out of the cumulative generation time budget, so keep the | |
| // valid (if imperfect) cited strong answer instead of spending a third generation | |
| // and risking a truncation -> unsupported tail. Recorded for observability. | |
| answerRetryReasons.push(`strong_quality_repair_skipped_time_budget:${strongQualityFailureReason}`); | |
| } else if (answerNeedsStrongQualityRepair) { | |
| if ( | |
| answerNeedsStrongQualityRepair && | |
| generationLatencyMs + env.OPENAI_ANSWER_TIMEOUT_MS >= generationTotalBudgetMs | |
| ) { | |
| // A4 tail-latency guard: out of the cumulative generation time budget, so keep the | |
| // valid (if imperfect) cited strong answer instead of spending a third generation | |
| // and risking a truncation -> unsupported tail. Recorded for observability. | |
| answerRetryReasons.push(`strong_quality_repair_skipped_time_budget:${strongQualityFailureReason}`); | |
| } else if (answerNeedsStrongQualityRepair) { |
🤖 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 `@src/lib/rag.ts` around lines 4933 - 4938, Update the answer quality-repair
branch around answerNeedsStrongQualityRepair to reserve the retry’s
env.OPENAI_ANSWER_TIMEOUT_MS window before starting it. Skip the retry when
generationLatencyMs plus that timeout would reach or exceed
generationTotalBudgetMs, while preserving the existing retry path when
sufficient budget remains and recording the skip reason.
Source: Coding guidelines
| not: (column: string) => build(column.includes("hybrid_rpc_errors") ? "hybrid" : "degraded"), | ||
| ilike: (_column: string, pattern: string) => | ||
| build(pattern.includes("max_output_tokens") ? "truncation" : "timeout"), |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Assert the ilike column as well as the pattern.
The mock ignores _column, so this test still passes if production queries the wrong JSON path and reports incorrect SLO counts. Record or assert metadata->>fallback_reason together with the exact %max_output_tokens% and %timeout% patterns.
Proposed test hardening
- ilike: (_column: string, pattern: string) =>
- build(pattern.includes("max_output_tokens") ? "truncation" : "timeout"),
+ ilike: (column: string, pattern: string) => {
+ expect(column).toBe("metadata->>fallback_reason");
+ expect(["%max_output_tokens%", "%timeout%"]).toContain(pattern);
+ return build(pattern.includes("max_output_tokens") ? "truncation" : "timeout");
+ },📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| not: (column: string) => build(column.includes("hybrid_rpc_errors") ? "hybrid" : "degraded"), | |
| ilike: (_column: string, pattern: string) => | |
| build(pattern.includes("max_output_tokens") ? "truncation" : "timeout"), | |
| ilike: (column: string, pattern: string) => { | |
| expect(column).toBe("metadata->>fallback_reason"); | |
| expect(["%max_output_tokens%", "%timeout%"]).toContain(pattern); | |
| return build(pattern.includes("max_output_tokens") ? "truncation" : "timeout"); | |
| }, |
🤖 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 `@tests/answer-slo.test.ts` around lines 23 - 25, Update the ilike mock in the
test query builder to validate the column argument is metadata->>fallback_reason
and match the exact %max_output_tokens% and %timeout% patterns before returning
truncation or timeout; ensure unexpected columns or patterns cannot silently
pass.
Summary
Testing
|
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. An unexpected error occurred while generating fixes: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) |
|
@copilot resolve the merge conflicts on this branch. |
|
@codex fix all issues identified with smallest fix without regression. |
Head branch was pushed to by a user without write access
|
Caution Docstrings generation - FAILED No docstrings were generated. |
Merge conflicts resolved. There was a single conflict in |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@tests/answer-slo.test.ts`:
- Around line 7-12: Resolve the merge conflict in tests/answer-slo.test.ts by
removing the conflict markers and retaining the intended comment text, including
the third-argument event-type scoping clarification. Ensure no `<<<<<<<`,
`=======`, or `>>>>>>>` markers remain so Biome can parse the SLO tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8b162e64-1d39-4bc7-b7ef-58e2a12f373d
📒 Files selected for processing (1)
tests/answer-slo.test.ts
|
Summary
Testing
|
|
Closing as a duplicate: the same token-starvation fix landed on main as #580 (merged 12:23Z) from the session that originally authored it — all five non-rag files are byte-identical to this branch, and the rag.ts differences are main-side dose-gate refinements from #574, not fix content. The verification evidence gathered here still stands and applies to #580: verify:cheap 1935 ✓; golden retrieval eval 36/36, content_mrr@10=0.9111; answer-quality A/B vs main baseline — identical failing-case sets, zero max_output_tokens/provider_timeout fallbacks, grounded rate no drop, danger failure 0. See #459 for the canary triage. |
Production returned "unsupported" after 60-96s on high-value clinical queries. gpt-5.5 reasoning tokens and the visible answer share one max_output_tokens budget; the strong route ran high reasoning effort against a 4000-token cap, so reasoning consumed the whole budget before the answer was written (incomplete:max_output_tokens) or timed out.
Summary
Verification
npm run verify:pr-localDuring development, use
npm run verify:cheapas the faster iteration gate before the final PR-local preflight.npm run verify:uiwhen UI, routing, styling, browser behavior, reduced-motion, or forced-colors behavior changednpm run verify:releasebefore release or handoff confidence claimsFor retrieval, ranking, selection, chunking, source/citation rendering, or answer-contract changes,
verify:pr-localrunseval:rag:offlineautomatically. Run the offline command directly during iteration before spending a live eval.npm run eval:retrieval:quality(must stay 36/36) when retrieval, ranking, selection, chunking, or scoring behavior changed — CI cannot run it (needs live keys), so run it locally and paste the summary. A metadata/governance-weighting change once buried correct docs (recall 1.0→0.76) and only this eval caught it.npm run eval:rag -- --limit 15+npm run eval:quality -- --rag-onlywhen answer generation, the synthesis prompt, or answer post-processing changed (grounded-supported must not drop; citation-failure 0)npm run check:production-readinesswhen clinical workflow, privacy, environment, Supabase, source governance, or deployment behavior changednpm run check:deployment-readinesswhen deployment startup, hosting, or rollout behavior changedClinical Governance Preflight
Complete this section when the change touches ingestion, answer generation, search/ranking, source rendering, document access, privacy, production env, or clinical output.
Clinical KB Database(sjrfecxgysukkwxsowpy)Notes