Repository navigation
fix(multimodal): enforce the aggregate PDF limit on the input.content path - #1294
Conversation
✅ Single Commit Policy - COMPLIANTStatus: Policy requirements met • 1 commit • Valid format • Ready for merge 📊 View validation details📝 Commit Details
✅ Validation Results
🤖 Automated validation by NeuroLink Single Commit Enforcement |
|
Warning Review limit reached
Next review available in: 47 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughChangesPDF aggregate limit enforcement
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant MessageBuilder
participant PDFProcessor
participant ErrorFactory
MessageBuilder->>MessageBuilder: Collect PDF buffers and metadata
MessageBuilder->>PDFProcessor: Resolve page counts for input.content PDFs
PDFProcessor-->>MessageBuilder: Return page counts or null
MessageBuilder->>ErrorFactory: Create validation error when limits fail
ErrorFactory-->>MessageBuilder: Return non-retriable PDF validation error
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
There was a problem hiding this comment.
Pull request overview
This PR fixes a multimodal PDF limit bypass by enforcing provider aggregate PDF limits (pages and size) for PDFs submitted via both input.pdfFiles and the documented input.content: [{ type: "pdf" }] path in the message builder.
Changes:
- Extracts an aggregate PDF page/size guard into a shared
enforceAggregatePdfLimits()and applies it to both submission surfaces. - Adds
PDFProcessor.resolvePageCount()to resolve missingmetadata.pagesfrom PDF buffers so aggregate page enforcement can’t be bypassed via omitted metadata. - Introduces a shared
MultimodalPdfEntrytype to unify the PDF entry shape across both code paths, and adds a regression test for the page-limit bypass on theinput.contentsurface.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| test/continuous-test-suite-bugfixes.ts | Adds a regression test covering aggregate page-limit enforcement for input.content PDFs (with/without metadata.pages). |
| src/lib/utils/pdfProcessor.ts | Adds resolvePageCount() to derive best-effort page counts from buffers when metadata is missing. |
| src/lib/utils/messageBuilder.ts | Implements shared aggregate PDF guard and applies it to both input.pdfFiles and input.content PDF handling. |
| src/lib/types/file.ts | Adds MultimodalPdfEntry type to centralize the PDF entry shape used by the guard. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Under the limit in aggregate still resolves — the guard must not be | ||
| // rejecting every multi-PDF content request. | ||
| await build([synth(20), synth(20), synth(20)], false); // 60 total | ||
| return true; |
![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
🤖 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/utils/messageBuilder.ts`:
- Around line 1358-1369: Replace the untyped Error throws in the aggregate page
and size limit checks with dedicated ErrorFactory constructors. Add or reuse
typed ErrorFactory symbols for aggregate page-limit and aggregate size-limit
failures, preserving the existing provider, counts, limits, and guidance in
their messages.
- Around line 1316-1319: Update the aggregate PDF validation flow around
PDFProcessor.getProviderConfig so it returns early only when pdfFiles.length ===
0, allowing a single input.content PDF to undergo page and byte limit checks.
Preserve the provider-config guard and add regression coverage for single-PDF
page and size limit violations.
- Around line 1325-1356: Update the aggregate PDF page-count logic around
pageCounts and totalPages to always resolve each file’s authoritative page count
from its buffer, ignoring caller-supplied metadata.pages/pageCount for
enforcement. Treat any unresolved or failed PDFProcessor.resolvePageCount result
as a request error and reject before the aggregate limit check instead of
converting it to zero or only warning; add regressions covering spoofed metadata
and unresolved counts.
- Around line 1325-1348: Move the aggregate totalMB calculation and its
over-limit enforcement before the pageCounts Promise.all block and any
PDFProcessor.resolvePageCount calls. Reject oversized requests immediately using
the existing limit behavior, while preserving page-count resolution for requests
within the aggregate size limit.
🪄 Autofix
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: d831b299-9055-4961-acfd-234b8d1591ae
📒 Files selected for processing (4)
src/lib/types/file.tssrc/lib/utils/messageBuilder.tssrc/lib/utils/pdfProcessor.tstest/continuous-test-suite-bugfixes.ts
🛡️ Yama Review Verdict: CHANGES_REQUESTEDSeverity counts — 🔒 CRITICAL: 0 · This PR adds PDF aggregation validation (#309) with new error types and comprehensive test coverage. Overall good work with excellent test scenarios, but one MAJOR type safety issue needs to be fixed before merge. Findings: 1. MAJOR - processPdfFile returns wrong type (src/lib/utils/pdfProcessor.ts:70): The function declares Promise<Uint8Array> but returns data.text which is a string. This will cause type errors and potential runtime failures when callers expect Uint8Array. Fix: Either change the return type to Promise<string> or cast the return value: return data.text as Uint8Array; Impact on existing code: - Changed files: 5 (messageBuilder.ts, pdfProcessor.ts, errorHandling.ts, file.ts, continuous-test-suite-bugfixes.ts) - New error types added: PdfPageCountUnverifiableError, PdfAggregatePageLimitExceededError, PdfAggregateSizeLimitExceededError - Test coverage: Excellent - 4 new regression tests covering page limits, size limits, unresolved counts, and the content-path scenario - No breaking changes to public API Recommendation: CHANGES_REQUESTED - The type safety issue in pdfProcessor.ts must be fixed before this PR can be merged. All other aspects of the PR are solid. Findings behind this verdict
|
a2f4981 to
a9203f6
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/utils/messageBuilder.ts`:
- Around line 1364-1380: Reject every PDF with an unresolved page count: update
the aggregate check in messageBuilder.ts (lines 1364-1380) to throw
pdfPageCountUnverifiable() regardless of trustSuppliedPageCounts, remove the
trusted warning path, and update errorHandling.ts (lines 712-731) so
pdfPageCountUnverifiable() supports both explicit and content-path PDFs. Add the
input.pdfFiles regression and rejection assertion in
test/continuous-test-suite-bugfixes.ts (lines 1937-2015).
- Around line 1354-1358: Update the unknownFileNames mapping to normalize
Windows backslashes to forward slashes before passing each filename to
basename(), while preserving the existing "<unnamed file>" fallback and redacted
basename-only logging behavior.
🪄 Autofix
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: 9ba9a36f-66c6-4b6c-9c2c-adbd123330b9
📒 Files selected for processing (5)
src/lib/types/file.tssrc/lib/utils/errorHandling.tssrc/lib/utils/messageBuilder.tssrc/lib/utils/pdfProcessor.tstest/continuous-test-suite-bugfixes.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/utils/pdfProcessor.ts
- src/lib/types/file.ts
| if (unknownFileNames.length > 0) { | ||
| if (!trustSuppliedPageCounts) { | ||
| // Untrusted surface: an unreadable count is indistinguishable from an | ||
| // evasion attempt, and counting it as zero is precisely the hole. Fail | ||
| // closed rather than admit an unverifiable document. | ||
| throw ErrorFactory.pdfPageCountUnverifiable(unknownFileNames, provider); | ||
| } | ||
| // Trusted surface: detection already vetted these, so one unreadable | ||
| // count must not fail an otherwise valid request — but it must not be | ||
| // silent either, since the known sum may undercount the true total. | ||
| logger.warn( | ||
| `[PDF] Aggregate page-limit check across ${pdfFiles.length} PDFs could only be ` + | ||
| `partially verified: ${unknownFileNames.length} file(s) have an unknown ` + | ||
| `page count (${unknownFileNames.join(", ")}), so the known total (${totalPages}) may ` + | ||
| `undercount the true combined page count.`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Reject every PDF with an unresolved page count.
PDFProcessor.process() can return estimatedPages: null. The trusted branch logs this condition and counts the file as zero pages. That permits aggregate page-limit bypasses through input.pdfFiles.
src/lib/utils/messageBuilder.ts#L1364-L1380: throw a typed validation error when any page count is unresolved, regardless of submission path.src/lib/utils/errorHandling.ts#L712-L731: makepdfPageCountUnverifiable()valid for both explicit and content-path PDFs.test/continuous-test-suite-bugfixes.ts#L1937-L2015: add aninput.pdfFilesregression with an unresolved page count and assert rejection.
📍 Affects 3 files
src/lib/utils/messageBuilder.ts#L1364-L1380(this comment)src/lib/utils/errorHandling.ts#L712-L731test/continuous-test-suite-bugfixes.ts#L1937-L2015
🤖 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/utils/messageBuilder.ts` around lines 1364 - 1380, Reject every PDF
with an unresolved page count: update the aggregate check in messageBuilder.ts
(lines 1364-1380) to throw pdfPageCountUnverifiable() regardless of
trustSuppliedPageCounts, remove the trusted warning path, and update
errorHandling.ts (lines 712-731) so pdfPageCountUnverifiable() supports both
explicit and content-path PDFs. Add the input.pdfFiles regression and rejection
assertion in test/continuous-test-suite-bugfixes.ts (lines 1937-2015).
|
Addressed the review findings — pushed. Details in the updated PR body and commit message. |
… path
The provider page/size ceiling was only applied by processExplicitPdfFiles,
which serves `input.pdfFiles`. PDFs submitted through the documented
`input.content: [{ type: "pdf" }]` surface built their own pdfFiles array in
convertContentToProviderFormat and went straight to the provider, so moving an
over-limit payload from one field to the other bypassed the check entirely
(120 pages against openai's 100-page limit was accepted).
Extract the guard into a shared enforceAggregatePdfLimits() and call it from
both paths, with three properties the input.content surface needs:
- It runs for a single PDF, not just for two or more. input.content never goes
through FileDetector.detectAndProcess, so nothing else validated it and one
200-page document passed untouched.
- Caller-supplied `metadata.pages` is not trusted there. It is plain request
input, and believing it let three 40-page PDFs each declare `pages: 1` and
clear the ceiling. Counts are re-derived from the bytes via the new
PDFProcessor.resolvePageCount(); an unreadable PDF now fails closed rather
than contributing zero pages. The input.pdfFiles path keeps trusting its
counts, which come from detection, and keeps warning instead of failing.
- The byte total is checked before any parsing, so an oversized request is
rejected without first spending parser CPU on every document in it.
Aggregate failures now raise typed NeuroLinkErrors
(pdfAggregatePageLimitExceeded / pdfAggregateSizeLimitExceeded /
pdfPageCountUnverifiable) rather than bare Errors.
The repeated inline PDF-entry shape becomes MultimodalPdfEntry in
src/lib/types/file.ts per Critical Rule 2.
Closes #309
a9203f6 to
68342d0
Compare
|
Second review round addressed. Fixed — Windows separators in logged filenames. Correct and worth having: Declined — hard-failing unresolved page counts on the The asymmetry is deliberate. The two surfaces have different threat models:
Turning that warning into a throw would reject requests that work today, for users whose PDFs merely fail to parse cleanly — a regression well outside the scope of #309, and one that would land silently in a patch release. The residual gap is real but narrow: it needs a PDF that defeats both pdfjs and the header-regex estimate, and such a file also fails image conversion downstream for every non-native-PDF provider. I'd rather it be fixed deliberately, with its own issue and its own release note, than smuggled in here. Happy to open that issue if maintainers agree it's worth tightening. |
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
Review SummaryDecision: APPROVED This PR implements issue #309 by adding aggregate PDF limit enforcement across both submission surfaces ( Changes Reviewed (6 files):
Key Implementation Highlights:
Impact on Existing Code:The change adds a pre-enforcement guard that was previously missing for the No blocking issues found. All changes are safe to merge. |
Tara-ag
left a comment
Tara-ag
left a comment
There was a problem hiding this comment.
Review Summary for PR #1294
Decision: CHANGES_REQUESTED
This PR adds PDF aggregation validation (#309) with new error types and comprehensive tests. Overall good work, but one blocking issue needs to be fixed.
Findings (1 total):
⚠️ MAJOR (1):
- File:
src/lib/utils/pdfProcessor.ts, line 70 - Issue:
processPdfFilehas return type mismatch - declaresPromise<Uint8Array>but returns string - Impact: Type errors and runtime failures when callers expect Uint8Array
- Fix: Change return type to
Promise<string>or cast return value
Impact on existing code:
- Changed files: 6 (package.json, src/lib/types/file.ts, src/lib/utils/errorHandling.ts, src/lib/utils/messageBuilder.ts, src/lib/utils/pdfProcessor.ts, test/continuous-test-suite-bugfixes.ts)
- Impact radius: ~9,835 nodes affected (moderate impact)
- Execution flows touched: handleAuth, testExternalMCPConnection, useWorkflow
- No breaking changes to public SDK API (CLAUDE.md Rule 5 satisfied)
Positive observations:
- ✅ Comprehensive test coverage for all scenarios
- ✅ Proper error handling via ErrorFactory typed errors
- ✅ Tests cover both input.pdfFiles and input.content paths
- ✅ Handles spoofed metadata page count scenarios
- ✅ Handles unresolved page count scenarios
- ✅ Test assertions avoid leaking sensitive data
- ✅ Backward compatibility maintained
Recommendation:
Fix the type safety issue in processPdfFile before this PR can be merged.
Tara-ag
left a comment
Tara-ag
left a comment
There was a problem hiding this comment.
Review Summary
This PR adds PDF aggregation validation (#309) with new error types and comprehensive test coverage. Overall good work with excellent test scenarios, but one MAJOR type safety issue needs to be fixed before merge.
Findings
-
MAJOR -
processPdfFilereturns wrong type (src/lib/utils/pdfProcessor.ts:70): The function declaresPromise<Uint8Array>but returnsdata.textwhich is a string. This will cause type errors and potential runtime failures when callers expect Uint8Array.Fix: Either change the return type to
Promise<string>or cast the return value:return data.text as Uint8Array;
Impact on existing code
- Changed files: 5 (messageBuilder.ts, pdfProcessor.ts, errorHandling.ts, file.ts, continuous-test-suite-bugfixes.ts)
- New error types added:
PdfPageCountUnverifiableError,PdfAggregatePageLimitExceededError,PdfAggregateSizeLimitExceededError - Test coverage: Excellent - 4 new regression tests covering page limits, size limits, unresolved counts, and the content-path scenario
- No breaking changes to public API
Recommendation
CHANGES_REQUESTED - The type safety issue in pdfProcessor.ts must be fixed before this PR can be merged. All other aspects of the PR are solid.
Tara-ag
left a comment
Tara-ag
left a comment
There was a problem hiding this comment.
Review Summary
This PR adds PDF aggregation validation (#309) with new error types and comprehensive test coverage. Overall good work with excellent test scenarios, but one MAJOR type safety issue needs to be fixed before merge.
Findings
-
MAJOR -
processPdfFilereturns wrong type (src/lib/utils/pdfProcessor.ts:70): The function declaresPromise<Uint8Array>but returnsdata.textwhich is a string. This will cause type errors and potential runtime failures when callers expect Uint8Array.Fix: Either change the return type to
Promise<string>or cast the return value:return data.text as Uint8Array;
Impact on existing code
- Changed files: 5 (messageBuilder.ts, pdfProcessor.ts, errorHandling.ts, file.ts, continuous-test-suite-bugfixes.ts)
- New error types added:
PdfPageCountUnverifiableError,PdfAggregatePageLimitExceededError,PdfAggregateSizeLimitExceededError - Test coverage: Excellent - 4 new regression tests covering page limits, size limits, unresolved counts, and the content-path scenario
- No breaking changes to public API
Recommendation
CHANGES_REQUESTED - The type safety issue in pdfProcessor.ts must be fixed before this PR can be merged. All other aspects of the PR are solid.
|
🎉 This PR is included in version 10.10.8 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Closes #309
The bug
The provider page/size ceiling was only applied by
processExplicitPdfFiles, which servesinput.pdfFiles. PDFs submitted through the documentedinput.content: [{ type: "pdf" }]surface built their ownpdfFilesarray inconvertContentToProviderFormatand went straight to the provider.Moving an over-limit payload from one field to the other bypassed the check entirely:
input.pdfFilesinput.contentThe fix
enforceAggregatePdfLimits(), now called from both paths.metadata.pagesis optional and routinely omitted, and an absent count previously summed as zero — so even after wiring the guard in, the ceiling would not have tripped.PDFProcessor.resolvePageCount()resolves a missing count from the buffer (accurate pdfjs count, falling back to the header estimate — mirroring what the file path already derives).MultimodalPdfEntryinsrc/lib/types/file.tsper Critical Rule 2.Verification
New test
MessageBuilder #309: input.content PDFs hit the same aggregate page limit as input.pdfFilescovers both with- and without-metadata.pages, plus an under-limit case so the guard isn't just rejecting everything.Confirmed it genuinely catches the regression — with the new call removed it reports
✗and exits non-zero (not⊘ skipped):With the fix: 269/269 pass.
pnpm run check0 errors;pnpm run lint0 errors.Summary by CodeRabbit
New Features
Bug Fixes