fix(rag): restore live-validated retrieval order#915
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
📝 WalkthroughWalkthroughAdds deterministic released search-result stabilization before telemetry processing and uses ChangesRanking consistency
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
CI triageCI failed on this PR. Automated classification of the 2 failed job(s):
Compared with main CI run #3531 (success). Classification is evidence routing, not permission to ignore a failure. Exact quarantined Playwright identities remain governed by the flake ledger. |
![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: 8dedbfa060
ℹ️ 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/rag-second-stage-ranking.test.ts (1)
140-180: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the changed telemetry path, not only the helper.
This test calls
stabilizeReleasedSearchOrderdirectly, so it will still pass if the production call insrc/lib/rag.ts, Line 590 is removed or moved after deduplication. Add duplicate-ID fixtures and drive the public telemetry path, asserting the retained result andtop_score/second_top_score.As per coding guidelines, retrieval changes require realistic failure-path verification and focused tests proving the changed behavior.
🤖 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/rag-second-stage-ranking.test.ts` around lines 140 - 180, The test currently validates stabilizeReleasedSearchOrder directly instead of the production telemetry flow. Extend the test around applySecondStageRerankIfNeeded with duplicate-ID fixtures, exercise the public path that invokes stabilization and deduplication, and assert the retained result plus telemetry top_score and second_top_score values. Ensure the test would fail if the production stabilization call is removed or moved after deduplication.Source: Coding guidelines
🤖 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`:
- Line 569: Replace the locale-dependent ID comparison in the result comparator
with deterministic plain string ordering using < and >, while preserving the
existing equal-score tie-breaker behavior and return ordering.
---
Nitpick comments:
In `@tests/rag-second-stage-ranking.test.ts`:
- Around line 140-180: The test currently validates stabilizeReleasedSearchOrder
directly instead of the production telemetry flow. Extend the test around
applySecondStageRerankIfNeeded with duplicate-ID fixtures, exercise the public
path that invokes stabilization and deduplication, and assert the retained
result plus telemetry top_score and second_top_score values. Ensure the test
would fail if the production stabilization call is removed or moved after
deduplication.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 82f24383-9a14-4ded-be2c-616119c7155c
📒 Files selected for processing (2)
src/lib/rag.tstests/rag-second-stage-ranking.test.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2f8e6416f1
ℹ️ 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".
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. The branch was updated while autofix was in progress. Please try again. |
Head branch was pushed to by a user without write access
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/rag-second-stage-ranking.test.ts (1)
140-187: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winCover the production telemetry path, not only the helper sequence.
This test calls
stabilizeReleasedSearchOrderdirectly at Line 183, so it can pass even if the real telemetry pipeline omits stabilization or runs it after deduplication—the behavior this PR is meant to protect. Add an assertion around the production telemetry-processing entry point, preserving the stronger duplicate through that path.As per coding guidelines, retrieval changes must verify realistic failure paths and add focused tests for changed behavior.
🤖 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/rag-second-stage-ranking.test.ts` around lines 140 - 187, Extend the test to exercise the production telemetry-processing entry point that normally handles the results, rather than relying only on direct calls to applySecondStageRerankIfNeeded and stabilizeReleasedSearchOrder. Assert that this pipeline preserves the stronger released-hybrid duplicate and produces the expected distinct order, while keeping the existing helper-level assertions for focused coverage.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@tests/rag-second-stage-ranking.test.ts`:
- Around line 140-187: Extend the test to exercise the production
telemetry-processing entry point that normally handles the results, rather than
relying only on direct calls to applySecondStageRerankIfNeeded and
stabilizeReleasedSearchOrder. Assert that this pipeline preserves the stronger
released-hybrid duplicate and produces the expected distinct order, while
keeping the existing helper-level assertions for focused coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d445d8f5-86c6-4575-8aaf-bfc93a4ec6eb
📒 Files selected for processing (2)
src/lib/rag.tstests/rag-second-stage-ranking.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/rag.ts
Summary
Verification
npm run eval:rag:offline— 36 golden cases, 21 suites, 294 tests passed.npm run lint— passed.git diff --check— passed.npm run verify:cheap— all guards plus 319 files / 2,894 tests passed before this two-file patch; the focused patch checks above passed afterward.npm run verify:ui— production build plus 239/239 Playwright tests passed before this retrieval-only patch; UI verification not repeated because no UI, route, styling, or browser behavior changed.npm run typecheck— pending the repository-wide heavyweight lock owned by an unrelatedverify:pr-localrun; hosted Build/CI also covers type safety.npm run eval:retrieval:quality -- --fail-on-threshold— branch-scoped live canary 29678081433 is in progress on8dedbfa06.npm run eval:rag -- --limit 15+npm run eval:quality -- --rag-only— the workflow runs its answer-quality subset after retrieval passes.npm run check:production-readinessequivalent provider checks — Supabase project identity, migration history, live ACL/title-scope postconditions, security advisors, and performance advisors checked through the authorized Supabase MCP deployment workflow.Risk and rollout
8dedbfa06; no schema or production-data rollback is required.Clinical Governance Preflight
Clinical KB Database(sjrfecxgysukkwxsowpy)Notes
maincanary 29677244606 reproduced four deterministic failures: clozapine ANC threshold, patient safety plan, lithium monitoring, and alcohol CIWA threshold.Summary by CodeRabbit
rankScorefrom score explanations as a final fallback.