Fix repository regression findings#911
Conversation
…260719 # Conflicts: # docs/branch-review-ledger.md
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 34939609 | Triggered | PostgreSQL Credentials | ff34393 | tests/offline-release-profile.test.ts | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🔥 Files not summarized due to errors (1)
📝 WalkthroughWalkthroughThis PR hardens request cancellation, semantic reranking, PDF extraction, authentication, and API validation. It also adds Therapy Compass dataset projections, improves artifact availability routing, persists factsheet saves, and expands CI, offline verification, and regression tests. ChangesPlatform, data, and verification
Request cancellation and validation
Ranking, auth, and extraction
Factsheet and Therapy Compass UI
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: c53386dda5
ℹ️ 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".
| if (userResult.error && !isDefinitiveAuthValidationError(userResult.error)) { | ||
| setSession(null); | ||
| setStatus("error"); | ||
| setNotice(null); | ||
| setError("Session could not be verified. Check your connection and retry."); |
There was a problem hiding this comment.
When getUser() fails because Supabase auth is temporarily unreachable while getSession() still returns a stored session, this early return makes the later verificationUnavailable path unreachable. That regresses the documented offline/flaky-load behavior: the UI enters an error state with session cleared instead of preserving the signed-in view until server-side API calls can validate or reject the token. A focused AuthProvider test with getUser() returning a retryable fetch error and getSession() returning a session would catch this.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Summary
- Added
shouldFailInitialSessionVerificationso retryable Supabase auth fetch errors are not treated as initial-load verification failures, while non-retryable unknown errors still enter the error path. src/lib/supabase/client.tsxL131-L135 - Updated
AuthProviderinitialization to compute retryable verification unavailability before the generic verification-error branch, allowing stored sessions to reach the existing offline/flaky-load preservation path. src/lib/supabase/client.tsxL186-L203 - Added focused regression coverage for retryable auth fetch outages, non-retryable verification failures, and definitive token rejection. tests/private-client-auth.test.tsL53-L57
Commit / PR
- Committed changes on the current branch:
c8a1e86 fix: preserve sessions during auth fetch outages. - Created PR metadata with title:
Preserve stored sessions during retryable auth outages.
Testing
- ✅
git diff --check ⚠️ npm run test:focused -- --files src/lib/supabase/client.tsx,tests/private-client-auth.test.ts— repository guard refused focused selection because a test file changed and instructed running the full unit suite.⚠️ npx vitest run tests/private-client-auth.test.ts— environment dependency limitation: local dependencies were incomplete, andnpxattempted to use an ad-hoc Vitest install without the repo’s installed Supabase packages.⚠️ npm run test -- --runInBand— environment dependency limitation:node_modules/vitest/vitest.mjsis missing in this checkout.
|
@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. |
|
Superseded by clean-history PR #912 so GitGuardian can evaluate the final tree without the removed false-positive signature in an earlier commit. No force push or scanner bypass was used. |
Pull request was closed
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d581e0af15
ℹ️ 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".
| if (userResult.error && !isDefinitiveAuthValidationError(userResult.error)) { | ||
| setSession(null); | ||
| setStatus("error"); | ||
| setNotice(null); | ||
| setError("Session could not be verified. Check your connection and retry."); | ||
| return; |
There was a problem hiding this comment.
When a browser has a stored Supabase session and getUser() fails because the auth server is temporarily unreachable, this new early return runs before the existing isAuthRetryableFetchError/resolveInitialAuthState path below. That drops session to null and leaves authorizationHeader empty, so a valid signed-in user loses private-document access on a flaky/offline load instead of retaining the stored session until the server can validate it; an AuthProvider test with getUser() returning a fetch error and getSession() returning a session would catch this.
Useful? React with 👍 / 👎.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/supabase/client.tsx (1)
182-197: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRetryable auth outages skip the session-preservation fallback.
isDefinitiveAuthValidationError()returns false for transient fetch failures, so this early return fires beforeverificationUnavailableis computed. In the auth-server-unreachable case,setSession(null)+status: "error"runs and the stored session is never preserved. Move the retryable-fetch case out of this guard, or remove the now-dead fallback path/comment.🤖 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/supabase/client.tsx` around lines 182 - 197, Update the initial auth verification flow around isDefinitiveAuthValidationError and isAuthRetryableFetchError so retryable fetch failures do not enter the early setSession(null)/error return. Compute or otherwise account for verificationUnavailable before the guard, preserve the stored session through resolveInitialAuthState when the auth server is unreachable, and retain the existing early-return behavior for definitive validation errors.
🧹 Nitpick comments (4)
src/lib/extractors/document.ts (1)
239-269: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd focused tests for the new per-page fallback image extraction.
This loop's behavior (per-page
partialextraction, budget assertion ordering, hardcoded PNG output,needsOcrinterplay) changed materially but isn't directly exercised in the provided test diffs. Based on path instructions, ingestion changes should "add focused tests when needed to prove migrated or changed behavior" — e.g., a fixture PDF with an embedded image validating written bytes/mimeType/dimensions, and a case simulating missing/malformed image data.🤖 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 239 - 269, Add focused tests covering the per-page fallback extraction loop around parser.getImage: verify partial page requests, PNG output bytes and metadata, dimensions, and budget assertion ordering; also cover missing or malformed image data and the needsOcr interaction. Use an embedded-image fixture or targeted parser mock, and assert that written artifacts and failure behavior match the intended limits.Source: Path instructions
src/lib/document-enrichment.ts (1)
806-809: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor inconsistency: fallback queries don't use the new
withOptionalAbortSignalhelper.The new helper introduced above at Lines 733-736 was applied to the RPC calls but not to the
labelsQuery/summariesQueryfallback path here, which still uses the old inlineif (args.signal) { ... }pattern. Purely cosmetic — behavior is identical — but leaves two idioms for the same thing in one file.♻️ Optional consistency fix
- if (args.signal) { - labelsQuery = labelsQuery.abortSignal(args.signal); - summariesQuery = summariesQuery.abortSignal(args.signal); - } + labelsQuery = withOptionalAbortSignal(labelsQuery, args.signal); + summariesQuery = withOptionalAbortSignal(summariesQuery, args.signal);🤖 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/document-enrichment.ts` around lines 806 - 809, Update the fallback query handling for labelsQuery and summariesQuery to use the existing withOptionalAbortSignal helper, replacing the inline args.signal conditional while preserving the current abort-signal behavior.scripts/verify-release-offline.mjs (1)
11-18: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the stale comment at line 23
The note still sayseval-qualitydeletes these keys again; update it toeval:rag:offlineto match the current step name.🤖 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 `@scripts/verify-release-offline.mjs` around lines 11 - 18, Update the stale comment near the release verification step list to reference eval:rag:offline instead of eval-quality, keeping the listed commands unchanged.src/components/therapy-compass/data/use-therapy-data.ts (1)
33-39: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winNarrow the index-catalogue type in
src/components/therapy-compass/data/use-therapy-data.ts:33-39
therapies-index.jsonis still loaded asTherapy[], even though it omits several requiredTherapyfields. Model the compact payload separately (Pick<Therapy, ...>orTherapyIndexEntry) so index-route code can’t accidentally read fields that aren’t in the JSON.🤖 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/components/therapy-compass/data/use-therapy-data.ts` around lines 33 - 39, Update loadDataset and the index-catalogue loading path to use a compact type such as TherapyIndexEntry or Pick<Therapy, ...> for therapies-index.json instead of Therapy[]. Preserve the full Therapy[] type for therapies.json, and ensure downstream index-route code only accesses fields present in the compact payload.
🤖 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/extractors/document.ts`:
- Around line 239-269: In the fallback image loop around parser.getImage,
validate that each image has finite positive width and height and binary
image.data with a nonzero byteLength before calling
budget.assertRenderDimensions, budget.assertArtifact, or Buffer.from. Skip
malformed images with continue so one corrupt image does not propagate to the
outer fallbackError handler; preserve normal processing for valid images.
- Around line 253-266: Update the fallback image handling near the image
extraction loop to detect the MIME type from the extracted image bytes before
deriving the file extension. Replace the hardcoded image/png value used by the
extension and images.push metadata, ensuring native formats such as JPEG receive
matching type and extension.
---
Outside diff comments:
In `@src/lib/supabase/client.tsx`:
- Around line 182-197: Update the initial auth verification flow around
isDefinitiveAuthValidationError and isAuthRetryableFetchError so retryable fetch
failures do not enter the early setSession(null)/error return. Compute or
otherwise account for verificationUnavailable before the guard, preserve the
stored session through resolveInitialAuthState when the auth server is
unreachable, and retain the existing early-return behavior for definitive
validation errors.
---
Nitpick comments:
In `@scripts/verify-release-offline.mjs`:
- Around line 11-18: Update the stale comment near the release verification step
list to reference eval:rag:offline instead of eval-quality, keeping the listed
commands unchanged.
In `@src/components/therapy-compass/data/use-therapy-data.ts`:
- Around line 33-39: Update loadDataset and the index-catalogue loading path to
use a compact type such as TherapyIndexEntry or Pick<Therapy, ...> for
therapies-index.json instead of Therapy[]. Preserve the full Therapy[] type for
therapies.json, and ensure downstream index-route code only accesses fields
present in the compact payload.
In `@src/lib/document-enrichment.ts`:
- Around line 806-809: Update the fallback query handling for labelsQuery and
summariesQuery to use the existing withOptionalAbortSignal helper, replacing the
inline args.signal conditional while preserving the current abort-signal
behavior.
In `@src/lib/extractors/document.ts`:
- Around line 239-269: Add focused tests covering the per-page fallback
extraction loop around parser.getImage: verify partial page requests, PNG output
bytes and metadata, dimensions, and budget assertion ordering; also cover
missing or malformed image data and the needsOcr interaction. Use an
embedded-image fixture or targeted parser mock, and assert that written
artifacts and failure behavior match the intended limits.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6c2959e5-7b8d-489f-8b49-d5c4d2a206bb
📒 Files selected for processing (49)
.github/workflows/ci.ymldocs/branch-review-ledger.mdpackage.jsonpublic/therapy-compass-data/therapies-index.jsonscripts/build-therapies-index.mjsscripts/ci-change-scope.mjsscripts/verify-release-offline.mjssrc/app/api/documents/[id]/route.tssrc/app/api/documents/[id]/summarize/route.tssrc/app/api/search/route.tssrc/components/factsheets/factsheet-detail-page.tsxsrc/components/factsheets/factsheets-data.tssrc/components/therapy-compass/bindings.tsxsrc/components/therapy-compass/data/use-therapy-data.tssrc/components/therapy-compass/screens/brief-screen.tsxsrc/components/therapy-compass/screens/pathways-screen.tsxsrc/components/therapy-compass/screens/recommend-screen.tsxsrc/components/therapy-compass/therapy-card.tsxsrc/lib/answer-ranking.tssrc/lib/document-detail.tssrc/lib/document-enrichment.tssrc/lib/extractors/document.tssrc/lib/extractors/pdf-extraction-budget.tssrc/lib/openai.tssrc/lib/rag-versioning.tssrc/lib/rag.tssrc/lib/saved-registry-storage.tssrc/lib/search-scope.tssrc/lib/semantic-rerank.tssrc/lib/supabase/client.tsxsrc/lib/types.tstests/answer-ranking.test.tstests/api-validation-contract.test.tstests/embed-texts-integrity.test.tstests/factsheet-save.dom.test.tsxtests/factsheets-data.test.tstests/offline-release-profile.test.tstests/openai-cache.test.tstests/pdf-extraction-budget.test.tstests/pdf-extractor.test.tstests/private-access-routes.test.tstests/private-client-auth.test.tstests/rag-answer-fallback.test.tstests/rag-document-summary.test.tstests/semantic-rerank.test.tstests/therapy-compass-artifact-navigation.dom.test.tsxtests/therapy-compass-data-recovery.dom.test.tsxtests/therapy-compass-mode-wiring.test.tstests/ui-route-coverage.spec.ts
| const images: ExtractedDocument["images"] = []; | ||
| for (const page of imageResult.pages) { | ||
| for (const [index, image] of page.images.entries()) { | ||
| const dataUrlMatch = image.dataUrl?.match(/^data:(.*?);base64,(.*)$/); | ||
| const mimeType = dataUrlMatch?.[1] ?? "image/png"; | ||
| const extension = mimeType.includes("jpeg") ? "jpg" : "png"; | ||
| const outputPath = path.join(imageDir, `fallback-page-${page.pageNumber}-image-${index + 1}.${extension}`); | ||
| const bytes = dataUrlMatch ? safeBufferFrom(dataUrlMatch[2], "base64") : Buffer.from(image.data); | ||
| if (!bytes) continue; | ||
| budget.addArtifact(bytes.byteLength); | ||
| await writeFile(outputPath, bytes); | ||
| images.push({ | ||
| pageNumber: page.pageNumber, | ||
| path: outputPath, | ||
| mimeType, | ||
| bbox: null, | ||
| width: null, | ||
| height: null, | ||
| sourceKind: "fallback", | ||
| metadata: { source_kind: "fallback" }, | ||
| }); | ||
| // Extract one page at a time so aggregate limits can stop subsequent decoding, and | ||
| // request only binary data to avoid holding a duplicate base64 representation. | ||
| for (const rawPage of rawPages) { | ||
| const imageResult = await parser.getImage({ | ||
| partial: [rawPage.pageNumber], | ||
| imageBuffer: true, | ||
| imageDataUrl: false, | ||
| imageThreshold: 20, | ||
| }); | ||
| for (const page of imageResult.pages) { | ||
| for (const [index, image] of page.images.entries()) { | ||
| budget.assertRenderDimensions(image.width, image.height); | ||
| budget.assertArtifact(image.data.byteLength); | ||
| const mimeType = "image/png"; | ||
| const extension = mimeType.includes("jpeg") ? "jpg" : "png"; | ||
| const outputPath = path.join(imageDir, `fallback-page-${page.pageNumber}-image-${index + 1}.${extension}`); | ||
| const bytes = Buffer.from(image.data); | ||
| budget.addArtifact(bytes.byteLength); | ||
| await writeFile(outputPath, bytes); | ||
| images.push({ | ||
| pageNumber: page.pageNumber, | ||
| path: outputPath, | ||
| mimeType, | ||
| bbox: null, | ||
| width: image.width, | ||
| height: image.height, | ||
| sourceKind: "fallback", | ||
| metadata: { source_kind: "fallback" }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Missing-image-data guard removed — a single malformed image now aborts the whole fallback extraction.
Previously the JS fallback had a "bytes is falsy" guard; it's gone. If image.data (or width/height) is ever missing/malformed for one image on one page, image.data.byteLength (Line 252) throws, and that exception propagates straight out of this loop into the outer catch (fallbackError) (Line 302), which tears down the whole extraction instead of just skipping the bad image. For adversarial/corrupt uploaded PDFs this turns a partial-degradation case into a full ingestion failure.
🛡️ Proposed fix to skip malformed images instead of crashing
for (const page of imageResult.pages) {
for (const [index, image] of page.images.entries()) {
+ if (!image.data || image.data.byteLength === 0) continue;
budget.assertRenderDimensions(image.width, image.height);
budget.assertArtifact(image.data.byteLength);Based on path instructions, "For clinical, retrieval, answer-generation, ingestion, ... changes, verify realistic failure paths and preserve conservative fallback behavior".
📝 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.
| const images: ExtractedDocument["images"] = []; | |
| for (const page of imageResult.pages) { | |
| for (const [index, image] of page.images.entries()) { | |
| const dataUrlMatch = image.dataUrl?.match(/^data:(.*?);base64,(.*)$/); | |
| const mimeType = dataUrlMatch?.[1] ?? "image/png"; | |
| const extension = mimeType.includes("jpeg") ? "jpg" : "png"; | |
| const outputPath = path.join(imageDir, `fallback-page-${page.pageNumber}-image-${index + 1}.${extension}`); | |
| const bytes = dataUrlMatch ? safeBufferFrom(dataUrlMatch[2], "base64") : Buffer.from(image.data); | |
| if (!bytes) continue; | |
| budget.addArtifact(bytes.byteLength); | |
| await writeFile(outputPath, bytes); | |
| images.push({ | |
| pageNumber: page.pageNumber, | |
| path: outputPath, | |
| mimeType, | |
| bbox: null, | |
| width: null, | |
| height: null, | |
| sourceKind: "fallback", | |
| metadata: { source_kind: "fallback" }, | |
| }); | |
| // Extract one page at a time so aggregate limits can stop subsequent decoding, and | |
| // request only binary data to avoid holding a duplicate base64 representation. | |
| for (const rawPage of rawPages) { | |
| const imageResult = await parser.getImage({ | |
| partial: [rawPage.pageNumber], | |
| imageBuffer: true, | |
| imageDataUrl: false, | |
| imageThreshold: 20, | |
| }); | |
| for (const page of imageResult.pages) { | |
| for (const [index, image] of page.images.entries()) { | |
| budget.assertRenderDimensions(image.width, image.height); | |
| budget.assertArtifact(image.data.byteLength); | |
| const mimeType = "image/png"; | |
| const extension = mimeType.includes("jpeg") ? "jpg" : "png"; | |
| const outputPath = path.join(imageDir, `fallback-page-${page.pageNumber}-image-${index + 1}.${extension}`); | |
| const bytes = Buffer.from(image.data); | |
| budget.addArtifact(bytes.byteLength); | |
| await writeFile(outputPath, bytes); | |
| images.push({ | |
| pageNumber: page.pageNumber, | |
| path: outputPath, | |
| mimeType, | |
| bbox: null, | |
| width: image.width, | |
| height: image.height, | |
| sourceKind: "fallback", | |
| metadata: { source_kind: "fallback" }, | |
| }); | |
| } | |
| const images: ExtractedDocument["images"] = []; | |
| // Extract one page at a time so aggregate limits can stop subsequent decoding, and | |
| // request only binary data to avoid holding a duplicate base64 representation. | |
| for (const rawPage of rawPages) { | |
| const imageResult = await parser.getImage({ | |
| partial: [rawPage.pageNumber], | |
| imageBuffer: true, | |
| imageDataUrl: false, | |
| imageThreshold: 20, | |
| }); | |
| for (const page of imageResult.pages) { | |
| for (const [index, image] of page.images.entries()) { | |
| if (!image.data || image.data.byteLength === 0) continue; | |
| budget.assertRenderDimensions(image.width, image.height); | |
| budget.assertArtifact(image.data.byteLength); | |
| const mimeType = "image/png"; | |
| const extension = mimeType.includes("jpeg") ? "jpg" : "png"; | |
| const outputPath = path.join(imageDir, `fallback-page-${page.pageNumber}-image-${index + 1}.${extension}`); | |
| const bytes = Buffer.from(image.data); | |
| budget.addArtifact(bytes.byteLength); | |
| await writeFile(outputPath, bytes); | |
| images.push({ | |
| pageNumber: page.pageNumber, | |
| path: outputPath, | |
| mimeType, | |
| bbox: null, | |
| width: image.width, | |
| height: image.height, | |
| sourceKind: "fallback", | |
| metadata: { source_kind: "fallback" }, | |
| }); | |
| } |
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 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 239 - 269, In the fallback image
loop around parser.getImage, validate that each image has finite positive width
and height and binary image.data with a nonzero byteLength before calling
budget.assertRenderDimensions, budget.assertArtifact, or Buffer.from. Skip
malformed images with continue so one corrupt image does not propagate to the
outer fallbackError handler; preserve normal processing for valid images.
Source: Path instructions
| const mimeType = "image/png"; | ||
| const extension = mimeType.includes("jpeg") ? "jpg" : "png"; | ||
| const outputPath = path.join(imageDir, `fallback-page-${page.pageNumber}-image-${index + 1}.${extension}`); | ||
| const bytes = Buffer.from(image.data); | ||
| budget.addArtifact(bytes.byteLength); | ||
| await writeFile(outputPath, bytes); | ||
| images.push({ | ||
| pageNumber: page.pageNumber, | ||
| path: outputPath, | ||
| mimeType, | ||
| bbox: null, | ||
| width: image.width, | ||
| height: image.height, | ||
| sourceKind: "fallback", |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does pdf-parse npm package getImage() always return PNG-encoded image buffers regardless of the source image encoding?
💡 Result:
No, the getImage() method in the pdf-parse npm package does not inherently convert all extracted images to PNG-encoded buffers. The method is designed to extract embedded images from a PDF file in their native format as they exist within the PDF structure [1][2]. While the documentation and examples provided by the package author often use .png file extensions when saving the output [1][3], this is a convention for naming the output files rather than a technical transformation performed by the package itself. The binary buffers returned by getImage() contain the image data exactly as it is stored (e.g., JPEG, PNG, or other formats supported by the PDF specification) within the source PDF [1]. Therefore, if a PDF contains a JPEG-encoded image, the buffer returned by getImage() will be a JPEG-encoded buffer, not a PNG-encoded one.
Citations:
- 1: https://github.com/mehmet-kozan/pdf-parse
- 2: https://github.com/mehmet-kozan/pdf-parse/blob/main/README.md
- 3: https://github.com/mehmet-kozan/pdf-parse/blob/902a64c7b26ff9f1ad58f0a11727a6280f51226c/README.md
Derive the image MIME type from the extracted bytes instead of hardcoding image/png. pdf-parse can return embedded images in their native format, so this currently writes the wrong extension and metadata for non-PNG images.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 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 253 - 266, Update the fallback
image handling near the image extraction loop to detect the MIME type from the
extracted image bytes before deriving the file extension. Replace the hardcoded
image/png value used by the extension and images.push metadata, ensuring native
formats such as JPEG receive matching type and extension.
1 participant
Summary
Verification
Verification not run: provider-backed OpenAI retrieval/answer evaluations were not run because live provider access was outside this remediation workflow.
Risk and rollout
Clinical Governance Preflight
Notes
Summary by CodeRabbit