Fix residual audit findings#926
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
📝 WalkthroughWalkthroughThe changes normalize Therapy Compass slugs, enforce PDF image budgets, propagate RAG cancellation, centralize released-result ordering, preserve semantic rerank metadata, refine initial auth failure handling, and document verification results. ChangesTherapy catalogue normalization
Document processing safeguards
Retrieval pipeline safeguards
Initial auth verification handling
Review ledger record
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
tests/pdf-extraction-budget.test.ts (1)
166-182: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the regression test prove pre-decode rejection.
With
maxImageSize: 1andstopAtErrors: true, removingassertDeclaredPdfImageDimensionscould still produce the same error later during parser extraction. Spy on the fallback parser’sgetText/getImagepath, or use a fixture that distinguishes preflight rejection from decode-time rejection.🤖 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/pdf-extraction-budget.test.ts` around lines 166 - 182, The regression test around extractPdf must verify rejection occurs before fallback image decoding, not merely that it eventually returns PDF_EXTRACTION_BUDGET_EXCEEDED. Add a spy or equivalent assertion on the fallback parser’s getText/getImage path, or replace the fixture with one that distinguishes preflight from decode-time failure, while preserving the oversized-image budget setup and expected error.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 `@docs/branch-review-ledger.md`:
- Line 633: Replace the `see PR head` placeholder in the ledger entry with the
exact commit SHA returned by `git rev-parse codex/close-913-review-findings`,
preserving the existing date, branch/ref, scope, outcome, and checks fields.
In `@src/lib/extractors/document.ts`:
- Around line 222-229: Update the preflight loop around subtypeMarker to avoid
repeated unbounded lastIndexOf and indexOf scans across the buffer. Replace the
per-match backward and forward searches with a single-pass dictionary traversal
or bounded candidate windows, while preserving the 64 KiB dictionary limit and
existing candidate-processing behavior.
- Around line 259-268: Scope stopAtErrors: true to the image-dimension
validation path instead of applying it to the parser used by getText(),
preserving best-effort extraction for unrelated recoverable PDF errors. Update
the PDFParse flow around assertDeclaredPdfImageDimensions and add coverage with
a malformed-but-readable PDF fixture that still extracts text despite non-image
parse errors.
In `@tests/therapy-compass-pathways.test.ts`:
- Around line 203-208: Update the duplicate-slug test around
LEGACY_DUPLICATE_SLUGS to assert against the canonical index used by
src/lib/therapies.ts, sourced from therapies-index.json rather than only
therapies.json. Ensure each legacy slug is absent from the application’s
canonical map, or explicitly validate both representations.
---
Nitpick comments:
In `@tests/pdf-extraction-budget.test.ts`:
- Around line 166-182: The regression test around extractPdf must verify
rejection occurs before fallback image decoding, not merely that it eventually
returns PDF_EXTRACTION_BUDGET_EXCEEDED. Add a spy or equivalent assertion on the
fallback parser’s getText/getImage path, or replace the fixture with one that
distinguishes preflight from decode-time failure, while preserving the
oversized-image budget setup and expected error.
🪄 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: e9f7b45f-8be3-4a8f-a196-b04ed3362d75
📒 Files selected for processing (14)
docs/branch-review-ledger.mddocs/site-map.mdpublic/therapy-compass-data/therapies-index.jsonpublic/therapy-compass-data/therapies.jsonsrc/data/therapies-index.jsonsrc/lib/extractors/document.tssrc/lib/rag.tssrc/lib/semantic-rerank.tssrc/lib/supabase/client.tsxtests/pdf-extraction-budget.test.tstests/private-client-auth.test.tstests/rag-document-summary.test.tstests/semantic-rerank.test.tstests/therapy-compass-pathways.test.ts
💤 Files with no reviewable changes (3)
- docs/site-map.md
- src/data/therapies-index.json
- public/therapy-compass-data/therapies-index.json
CI triageCI failed on this PR. Automated classification of the 2 failed job(s):
Compared with main CI run #3588 (failure). Classification is evidence routing, not permission to ignore a failure. Exact quarantined Playwright identities remain governed by the flake ledger. |
|
@cursoragent Please can you resolve all comments for me and fix all CI failing issues to ensure this PR can be merged to main. Ensure no regression and safe and smallest logical fix. |
|
Resolved all five CodeRabbit review comments and fixed the CI failure. Pushed to CI fix (Static PR checks)
CodeRabbit comments addressed
Verification run locally
The PR required job was failing only because Static PR checks failed on the maintainability budget; that root cause is fixed. Hosted CI should go green on the next run.   |
|
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. |
|
@CodeRabbit please fix failing CI testing to ensure this merges to main |
|
✅ CI fix PR created: Follow
6 failing check(s)
1 file(s) modified
View agent analysis |
- Extract stabilizeReleasedSearchOrder into released-search-order.ts to keep rag.ts within the no-growth maintainability budget (fixes Static PR checks). - Use single-pass PDF dictionary scanning for declared image dimension preflight. - Scope stopAtErrors to the image parser only so recoverable text errors still extract in the JavaScript fallback path. - Assert legacy duplicate therapy slugs against therapies-index.json as well as therapies.json. - Prove oversized-image rejection happens before pdf.js getText/getImage decode. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
…n replay, and static PR checks (#927) Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/lib/extractors/document.ts (2)
221-232: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftMake the PDF image preflight syntax-aware.
This raw scan also inspects literal strings, comments, and stream bytes, so a valid PDF containing
<< /Subtype /Image /Width ... /Height ... >>as content can be rejected before text extraction despite having no oversized image. Parse actual PDF dictionaries—or skip non-dictionary lexical regions and handle nesting—before enforcing the budget.🤖 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/extractors/document.ts` around lines 221 - 232, Update the dictionary scanning logic around the visible extraction loop so it parses only actual PDF dictionary objects, skipping comments, literal strings, and stream bytes while correctly handling nested delimiters. Apply budget.assertRenderDimensions only after extracting Width and Height from a syntactically valid image dictionary, preserving the existing size-limit and traversal behavior.
288-293: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftDo not let optional image extraction discard successful text extraction.
After
textParser.getText()succeeds, anyimageParser.getImage()error other than the exact size message is rethrown, so malformed or unsupported image data can make the whole readable document fail. Return the extracted text with a warning and no images for non-budget image errors; reserve fatal propagation for confirmed budget violations.As per coding guidelines, for clinical and document-processing changes, verify realistic failure paths and preserve conservative fallback behavior; add focused tests when needed.
Also applies to: 349-357
🤖 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/extractors/document.ts` around lines 288 - 293, Update the image extraction flow around imageParser.getImage so that, after textParser.getText succeeds, non-budget image errors are caught, logged as warnings, and result in returning the extracted text with no images. Preserve rethrowing only for the exact confirmed image-size budget violation, including the corresponding handling at the alternate image extraction path around the referenced branch.Source: Coding guidelines
🧹 Nitpick comments (2)
src/lib/released-search-order.ts (2)
29-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate tie-break logic between the two comparators.
compareReleasedHybridStrengthandcompareReleasedSearchOrderrepeat the same similarity → relevance-score → id tie-break chain, differing only in the primary score. Consider extracting the shared tail into a helper to avoid the two copies drifting out of sync.♻️ Suggested extraction
+const compareReleasedTieBreak = (left: SearchResult, right: SearchResult) => { + const leftSimilarity = left.similarity ?? 0; + const rightSimilarity = right.similarity ?? 0; + if (rightSimilarity !== leftSimilarity) return rightSimilarity - leftSimilarity; + if (right.relevance?.score !== left.relevance?.score) + return (right.relevance?.score ?? 0) - (left.relevance?.score ?? 0); + return left.id.localeCompare(right.id); +}; const compareReleasedHybridStrength = (left: SearchResult, right: SearchResult) => { const leftHybrid = left.hybrid_score ?? left.similarity ?? 0; const rightHybrid = right.hybrid_score ?? right.similarity ?? 0; if (rightHybrid !== leftHybrid) return rightHybrid - leftHybrid; - const leftSimilarity = left.similarity ?? 0; - const rightSimilarity = right.similarity ?? 0; - if (rightSimilarity !== leftSimilarity) return rightSimilarity - leftSimilarity; - if (right.relevance?.score !== left.relevance?.score) - return (right.relevance?.score ?? 0) - (left.relevance?.score ?? 0); - return left.id.localeCompare(right.id); + return compareReleasedTieBreak(left, right); };🤖 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/released-search-order.ts` around lines 29 - 56, Extract the shared similarity → relevance-score → id tie-break chain from compareReleasedHybridStrength and compareReleasedSearchOrder into a helper, then have both comparators delegate to it after comparing their respective primary scores. Preserve the existing descending ordering and id-based final tie-break behavior.
13-28: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocstring doesn't mention the
preserveIncomingOrderparameter.The JSDoc explains
preferSecondStageScore/second-stage-score fallback behavior in detail but never documentspreserveIncomingOrder, even though it unconditionally forcespreserveCurrentOrder = true(line 28), bypassing sorting entirely — including bypassing valid, bounded release scores. Given this is now an exported, shared utility (per the stack outline, called fromrag.tsaspreserveSemanticRerankOrder), documenting when/why callers should set it would help future maintainers reason about precedence between the two flags.🤖 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/released-search-order.ts` around lines 13 - 28, Update the JSDoc for stabilizeReleasedSearchOrder to document preserveIncomingOrder, including that it forces the incoming order to be retained and takes precedence over release-score sorting, including valid bounded second-stage scores. Clarify how it interacts with preferSecondStageScore so callers understand when to enable it.
🤖 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/released-search-order.ts`:
- Around line 40-48: Update compareReleasedSearchOrder by hoisting each optional
releaseRankScore value from left.score_explanation and right.score_explanation
into local variables before the conditional checks, then use those locals in
isBoundedReleaseRankScore and the selected score expression so strict
nullability narrowing is preserved.
---
Outside diff comments:
In `@src/lib/extractors/document.ts`:
- Around line 221-232: Update the dictionary scanning logic around the visible
extraction loop so it parses only actual PDF dictionary objects, skipping
comments, literal strings, and stream bytes while correctly handling nested
delimiters. Apply budget.assertRenderDimensions only after extracting Width and
Height from a syntactically valid image dictionary, preserving the existing
size-limit and traversal behavior.
- Around line 288-293: Update the image extraction flow around
imageParser.getImage so that, after textParser.getText succeeds, non-budget
image errors are caught, logged as warnings, and result in returning the
extracted text with no images. Preserve rethrowing only for the exact confirmed
image-size budget violation, including the corresponding handling at the
alternate image extraction path around the referenced branch.
---
Nitpick comments:
In `@src/lib/released-search-order.ts`:
- Around line 29-56: Extract the shared similarity → relevance-score → id
tie-break chain from compareReleasedHybridStrength and
compareReleasedSearchOrder into a helper, then have both comparators delegate to
it after comparing their respective primary scores. Preserve the existing
descending ordering and id-based final tie-break behavior.
- Around line 13-28: Update the JSDoc for stabilizeReleasedSearchOrder to
document preserveIncomingOrder, including that it forces the incoming order to
be retained and takes precedence over release-score sorting, including valid
bounded second-stage scores. Clarify how it interacts with
preferSecondStageScore so callers understand when to enable it.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 07fbd0db-7e40-4a7e-9ff5-da2ad94bc2e6
📒 Files selected for processing (6)
docs/branch-review-ledger.mdsrc/lib/extractors/document.tssrc/lib/rag.tssrc/lib/released-search-order.tstests/pdf-extraction-budget.test.tstests/therapy-compass-pathways.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/therapy-compass-pathways.test.ts
- tests/pdf-extraction-budget.test.ts
- docs/branch-review-ledger.md
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 1 file(s) based on 1 unresolved review comment. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 1 file(s) based on 1 unresolved review comment. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
… into codex/close-913-review-findings # Conflicts: # docs/branch-review-ledger.md # src/lib/extractors/document.ts # tests/pdf-extraction-budget.test.ts # tests/therapy-compass-pathways.test.ts
… into codex/close-913-review-findings
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)
src/lib/extractors/document.ts (1)
223-239: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftParse PDF tokens before enforcing image dimensions.
The
<</>>searches and/Subtype /Imageregex operate on raw bytes, so markers inside a string, comment, or stream can be mistaken for a real image dictionary. A crafted or valid PDF could therefore triggerassertRenderDimensionsand fail extraction even when no oversized image exists. Tokenize actual dictionaries (including nesting and stream boundaries) and add a regression fixture containing image-like markers in non-dictionary content.As per coding guidelines, document-access changes must verify realistic failure paths, preserve conservative fallback behavior, and add focused tests when needed.
🤖 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/extractors/document.ts` around lines 223 - 239, Replace the raw marker and regex scanning in the extraction loop with PDF tokenization that identifies actual dictionaries, tracks nested << / >> structures, and respects stream boundaries before inspecting /Subtype, /Width, or /Height. Preserve conservative fallback behavior by skipping malformed or incomplete token sequences, and add a focused regression fixture/test containing image-like markers inside strings, comments, and streams to confirm assertRenderDimensions is not triggered for non-dictionary content.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 `@src/lib/extractors/document.ts`:
- Around line 223-239: Replace the raw marker and regex scanning in the
extraction loop with PDF tokenization that identifies actual dictionaries,
tracks nested << / >> structures, and respects stream boundaries before
inspecting /Subtype, /Width, or /Height. Preserve conservative fallback behavior
by skipping malformed or incomplete token sequences, and add a focused
regression fixture/test containing image-like markers inside strings, comments,
and streams to confirm assertRenderDimensions is not triggered for
non-dictionary content.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: cc2ddec0-de8d-4540-a896-46aea5a53710
📒 Files selected for processing (6)
docs/branch-review-ledger.mdsrc/lib/extractors/document.tssrc/lib/rag-document-summary-context.tssrc/lib/rag.tssrc/lib/released-search-order.tstests/pdf-extraction-budget.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/released-search-order.ts
- docs/branch-review-ledger.md
2 participants
Summary - Close the five unresolved current review findings left on merged PR #913. - Remove six legacy duplicate Therapy Compass records and regenerate both indexes plus the tracked sitemap. - Propagate summary request cancellation through both Supabase reads. - Enforce the PDF image pixel ceiling before fallback decoding and classify over-budget failures. - Preserve semantic rerank scores without an existing score explanation and preserve stored auth sessions during native retryable fetch failures. ## Verification - [x]
npm run verify:pr-local— format, lint, typecheck, 322 test files / 2,938 passed / 1 skipped, Next 16.2.10 production build with 1,676 generated pages, client-bundle secret scan, and 36 RAG fixtures across 21 suites. - [x] Focused changed-surface suite — 51 files / 635 tests passed. - [x]npm run sitemap:checkandtests/site-map.test.ts— 6/6 passed. - [x] Earlier full localnpm run verify:uiin this release task — Chromium 239/239 passed; this cleanup does not change routes or styling. Exact-head hosted UI remains required. - [ ]npm run verify:release— provider-backed portions will run after the PR has an exact hosted head. - [ ]npm run eval:retrieval:quality— approved provider canary will run on the final protected-main head. - [ ]npm run eval:rag -- --limit 15+npm run eval:quality -- --rag-only— approved provider canary will run on the final protected-main head. - [ ]npm run check:production-readiness— local worktree has no provider secrets; project identity and boot guard passed, while secret-backed checks are delegated to hosted CI using repository secrets. - [x]git diff --check, docs index, docs links, therapy-index integrity, and client-bundle secret scan passed. ## Risk and rollout - Risk: Medium. This changes auth outage fallback, PDF ingestion resource enforcement, summary cancellation, semantic answer-evidence metadata, and public therapy catalogue routes. - Rollback: Revert the remediation commit; the earlier Supabase DDL deployment is independent and does not need rollback for this PR. - Provider or production effects: The explicitly authorized Supabase deployment toClinical KB Database(sjrfecxgysukkwxsowpy) was completed earlier in this task. This PR performs no additional production-data writes or Supabase mutations. Hosted CI and the release canary will use repository provider secrets after push. ## Clinical Governance Preflight - [x] Source-backed claims still require linked source verification before clinical use - [x] No patient-identifiable document workflow was introduced or expanded without explicit governance approval - [x] Supabase target remainsClinical KB Database(sjrfecxgysukkwxsowpy) - [x] Service-role keys and private document access remain server-only - [x] Demo/synthetic content remains clearly separated from real clinical sources - [x] Source metadata, review status, and outdated/unknown-source behavior remain conservative - [x] Deployment classification/TGA SaMD impact was checked when clinical decision-support behavior changed ## Notes - Standalone Stylelint remains not applicable because the repository has no Stylelint configuration. - All current review threads on #913 and #926 were resolved after exact-head verification. ## Summary by CodeRabbit - New Features - Updated the Therapy Compass catalog with richer, standardized therapy information and clearer naming. - Improved search result ordering and ranking consistency, including results without detailed score information. - Bug Fixes - Added safeguards to reject oversized embedded PDF images before processing. - Improved document summarization cancellation handling. - Preserved stored sessions during temporary authentication or network failures. - Removed legacy duplicate therapy entries and routes. - Documentation - Updated the site map and review ledger to reflect current therapy routes and verification status.