Repository navigation
fix(pdf): harden image conversion — accurate pages, per-page resilience, scale, streaming, aggregate & URL limits - #1201
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
Next review available in: 12 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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 Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (7)
✨ Finishing Touches🧪 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 |
murdore
force-pushed
the
fix/pdf-page-memory-and-password
branch
from
July 18, 2026 10:26
c163187 to
5651eab
Compare
murdore
force-pushed
the
fix/pdf-hardening
branch
from
July 18, 2026 10:35
de1c188 to
957cda0
Compare
murdore
force-pushed
the
fix/pdf-page-memory-and-password
branch
from
July 18, 2026 12:27
5651eab to
895d9ad
Compare
murdore
force-pushed
the
fix/pdf-hardening
branch
from
July 18, 2026 12:32
957cda0 to
1d21b23
Compare
murdore
force-pushed
the
fix/pdf-page-memory-and-password
branch
from
July 19, 2026 17:13
895d9ad to
fdf62b7
Compare
…ce, scale, streaming, aggregate & URL limits Consolidated hardening of the PDF image-fallback path (providers without native PDF support), stacked on the password/canvas work in #1195. New getAccuratePageCount() parses the PDF via pdf-parse (pdfjs), time-bounded by PAGE_COUNT_TIMEOUT_MS, and process() prefers it over the unreliable /Type /Page header regex for both limit enforcement and reported metadata; falls back to the regex estimate on timeout/failure. convertToImages drives an indexed getPage() loop with per-page try/catch: a single failing page is recorded in result.errors ({ page, error }) instead of discarding every already-converted page. Default scale 2 → 1.5 (DEFAULT_SCALE) to ~halve per-page canvas memory; the full MIN_SCALE(0.1)..MAX_SCALE(10) range is enforced; a memory estimate is logged before conversion. New async generator convertToImagesStream() yields each page as it renders (PDFImagePage) with an onProgress callback; convertToImages shares its validation. Per-page failures are yielded with `error` set. processExplicitPdfFiles now checks combined page count + size across all PDFs against the provider ceiling — N files each under the single-file limit can no longer exceed it together. loadFromURL issues a HEAD (DEFAULT_HEAD_TIMEOUT) and rejects an oversized Content-Length before downloading any body; a missing header / HEAD-refusing server falls through to the existing streaming byte guard. The previously-dead provider config field is now exposed on FileProcessingResult.metadata.requiresCitations (and documented) so provider adapters can act on it. Tests: 8 deterministic cases in continuous-test-suite-bugfixes.ts (+ new test/fixtures/pdf-page3-corrupt.pdf), plus the scale-validation test extended for MIN_SCALE. Docs: pdf-support.md conversion-resilience + streaming sections.
murdore
force-pushed
the
fix/pdf-hardening
branch
from
July 19, 2026 17:37
1d21b23 to
7efbe94
Compare
Contributor
✅ 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 |
Contributor
🤖 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 |
Contributor
Documentation Validation Results🚀 Documentation validation passed!
📦 Build artifact uploaded successfully. Ready for deployment preview. Commit: |
There was a problem hiding this comment.
Pull request overview
This PR hardens the PDF image-fallback pipeline (used when a provider lacks native PDF support) by improving page counting, conversion resilience, streaming conversion, and enforcing limits earlier and more consistently across multi-PDF and URL-based inputs.
Changes:
- Add an accurate, time-bounded PDF page-count probe (pdfjs via
pdf-parse) and surfacerequiresCitationsin processed PDF metadata. - Improve PDF→image conversion resilience: default scale lowered to 1.5 with min/max bounds, per-page error isolation, and a new streaming async-generator API with progress callbacks.
- Enforce aggregate limits across multiple PDFs in one request and add URL pre-flight oversize rejection via
HEADbefore downloading.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| test/continuous-test-suite-bugfixes.ts | Adds regression tests for accurate page counts, per-page isolation, streaming conversion, aggregate enforcement, URL pre-flight, and citations surfacing. |
| src/lib/utils/pdfProcessor.ts | Implements accurate page count probing, per-page conversion isolation, scale limits/defaults, memory-estimate logging, and convertToImagesStream(). |
| src/lib/utils/messageBuilder.ts | Adds aggregate (combined) page/size enforcement for multiple attached PDFs. |
| src/lib/utils/fileDetector.ts | Adds HEAD pre-flight content-length check before downloading URLs (with caching integration). |
| src/lib/types/file.ts | Extends types for citations requirement metadata and streaming conversion progress/page results. |
| docs/features/pdf-support.md | Documents conversion resilience/limits and streaming conversion API. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+301
to
+309
| const info = await Promise.race([ | ||
| pdf.getInfo(), | ||
| new Promise<never>((_, reject) => | ||
| setTimeout( | ||
| () => reject(new Error("page-count timeout")), | ||
| PDF_LIMITS.PAGE_COUNT_TIMEOUT_MS, | ||
| ), | ||
| ), | ||
| ]); |
Comment on lines
+313
to
+316
| await ( | ||
| pdf as unknown as { destroy?: () => Promise<void> | void } | ||
| ).destroy?.(); | ||
| } |
Comment on lines
+1329
to
+1331
| const totalPages = pdfFiles.reduce((sum, f) => sum + (f.pageCount ?? 0), 0); | ||
| const totalMB = | ||
| pdfFiles.reduce((sum, f) => sum + f.buffer.length, 0) / (1024 * 1024); |
Comment on lines
+1908
to
+1919
| const head = await request(url, { | ||
| dispatcher: getGlobalDispatcher().compose( | ||
| interceptors.redirect({ maxRedirections: 5 }), | ||
| ), | ||
| method: "HEAD", | ||
| headersTimeout: FileDetector.DEFAULT_HEAD_TIMEOUT, | ||
| bodyTimeout: FileDetector.DEFAULT_HEAD_TIMEOUT, | ||
| }); | ||
| // Drain/close the (empty) HEAD body so the connection can be reused. | ||
| await head.body.dump(); | ||
| const declaredLength = Number(head.headers["content-length"]); | ||
| if (Number.isFinite(declaredLength) && declaredLength > maxSize) { |
Comment on lines
+652
to
+658
| /** | ||
| * Streaming variant of {@link convertToImages} (#302): yields each page's | ||
| * base64 PNG as soon as it renders instead of buffering the whole document, | ||
| * and reports progress via `options.onProgress`. A page that fails to render | ||
| * is yielded with `error` set (per-page isolation, #294) rather than aborting | ||
| * the stream. `convertToImages` is the batch wrapper over this contract. | ||
| */ |
Comment on lines
+261
to
+262
| `convertToImages()` is the batch wrapper over this stream and returns the same | ||
| per-page `errors` contract. |
Tara-ag
requested changes
Jul 19, 2026
Tara-ag
left a comment
Tara-ag
left a comment
Contributor
There was a problem hiding this comment.
Review summary
Reviewed the 8 changed files in this PDF hardening PR. The changes are well-scoped and the test coverage is good, but there are several correctness issues that need to be addressed before merging.
New issues raised
| Severity | Count | Files |
|---|---|---|
| 🔒 CRITICAL | 1 | src/lib/utils/pdfProcessor.ts |
| 3 | src/lib/utils/pdfProcessor.ts, src/lib/utils/messageBuilder.ts, src/lib/utils/fileDetector.ts |
|
| 💡 MINOR | 2 | src/lib/utils/pdfProcessor.ts, docs/features/pdf-support.md |
Blocking concerns
- Dangling timeout in
getAccuratePageCount()(pdfProcessor.ts:309) — thesetTimeoutis never cleared whenpdf.getInfo()wins, which can keep the process alive and waste work. pdf.destroy()failure discards a valid page count (pdfProcessor.ts:316) — cleanup errors should not null out an already-computed accurate count.- Aggregate page guard silently passes when page counts are unknown (
messageBuilder.ts:1331) — summingpageCount ?? 0lets corrupted/odd PDFs bypass the aggregate limit. - HEAD pre-flight doesn't check status code (
fileDetector.ts:1908) — a 405/4xx HEAD can still be read as a size rejection instead of falling through to the GET streaming guard.
These meet the ≥3 MAJOR threshold for requesting changes.
Minor notes
- The JSDoc/docs wording implying
convertToImages()wrapsconvertToImagesStream()is misleading because the implementations are separate; reword to clarify they share validation and per-page semantics.
Next steps
Please address the four blocking items above (each comment includes a suggested fix). Once those are resolved and CI is green, this should be good to merge. The new tests and fixtures are a solid addition.
| PDF_LIMITS.PAGE_COUNT_TIMEOUT_MS, | ||
| ), | ||
| ), | ||
| ]); |
Contributor
There was a problem hiding this comment.
🔒 CRITICAL: getAccuratePageCount() starts a setTimeout inside Promise.race but never clears it when pdf.getInfo() wins. The dangling timer can keep the process alive and does unnecessary work.
Suggested fix:
const timeout = setTimeout(
() => reject(new Error("page-count timeout")),
PDF_LIMITS.PAGE_COUNT_TIMEOUT_MS,
);
try {
const info = await Promise.race([pdf.getInfo(), timeoutPromise]);
// ...
return total;
} finally {
clearTimeout(timeout);
await (pdf as unknown as { destroy?: () => Promise<void> | void }).destroy?.();
}Also consider timeout.unref() so it never delays process exit even if the race is still pending when the process wants to shut down.
| await ( | ||
| pdf as unknown as { destroy?: () => Promise<void> | void } | ||
| ).destroy?.(); | ||
| } |
Contributor
There was a problem hiding this comment.
pdf.destroy() throws, the outer try in getAccuratePageCount() treats the whole accurate page count as failed and returns null, even when getInfo() succeeded. Cleanup failures should not discard a valid page count.
Suggested fix (matching the pattern already used in fileReferenceRegistry.ts):
} finally {
await (
pdf as unknown as { destroy?: () => Promise<void> | void }
).destroy?.().catch(() => {
/* cleanup - ignore destroy errors */
});
}| if (aggregateConfig && pdfFiles.length > 1) { | ||
| const totalPages = pdfFiles.reduce((sum, f) => sum + (f.pageCount ?? 0), 0); | ||
| const totalMB = | ||
| pdfFiles.reduce((sum, f) => sum + f.buffer.length, 0) / (1024 * 1024); |
Contributor
There was a problem hiding this comment.
estimatedPages === null (e.g. corrupted/odd PDFs), because totalPages sums pageCount ?? 0. For limit enforcement, treating unknown page counts as 0 undercounts and defeats the aggregate guard.
Suggested fix: when any PDF has an unknown page count, either (a) reject the aggregate check until all page counts are known, or (b) fall back to a conservative upper-bound estimate. For example:
const unknownPageCount = pdfFiles.some((f) => f.pageCount === null || f.pageCount === undefined);
const totalPages = pdfFiles.reduce((sum, f) => sum + (f.pageCount ?? 0), 0);
if (!unknownPageCount && totalPages > aggregateConfig.maxPages) {
throw new Error(...);
}
// Optionally warn/log when unknown so operators know the guard was partial.At minimum, the aggregate guard should not silently pass when page counts are missing.
| // Drain/close the (empty) HEAD body so the connection can be reused. | ||
| await head.body.dump(); | ||
| const declaredLength = Number(head.headers["content-length"]); | ||
| if (Number.isFinite(declaredLength) && declaredLength > maxSize) { |
Contributor
There was a problem hiding this comment.
head.statusCode. If a server returns 405/4xx, this code still reads content-length and may reject as “too large” based on the error response size, even though the intent is to fall through to the GET streaming guard when HEAD is refused.
Suggested fix: only enforce the size when the HEAD succeeds (2xx). For example:
if (head.statusCode >= 200 && head.statusCode < 300) {
const declaredLength = Number(head.headers["content-length"]);
if (Number.isFinite(declaredLength) && declaredLength > maxSize) {
throw new Error(`File too large: ...`);
}
}Non-2xx HEAD responses should be treated the same as a missing header and fall through to the GET streaming guard.
| * and reports progress via `options.onProgress`. A page that fails to render | ||
| * is yielded with `error` set (per-page isolation, #294) rather than aborting | ||
| * the stream. `convertToImages` is the batch wrapper over this contract. | ||
| */ |
Contributor
There was a problem hiding this comment.
💡 MINOR: The JSDoc says “convertToImages is the batch wrapper over this contract”, but convertToImages() doesn't call convertToImagesStream() (it has its own implementation). This is misleading for maintainers and readers. Suggest rewording to avoid implying an implementation relationship, e.g. “Batch variant of the same conversion; both share validateImageConversionInput and per-page isolation semantics.”
| if (aggregateConfig && pdfFiles.length > 1) { | ||
| const totalPages = pdfFiles.reduce((sum, f) => sum + (f.pageCount ?? 0), 0); | ||
| const totalMB = | ||
| pdfFiles.reduce((sum, f) => sum + f.buffer.length, 0) / (1024 * 1024); |
Contributor
There was a problem hiding this comment.
estimatedPages === null (e.g. corrupted/odd PDFs), because totalPages sums pageCount ?? 0. For limit enforcement, treating unknown page counts as 0 undercounts and defeats the aggregate guard.
Suggested fix: when any PDF has an unknown page count, either (a) reject the aggregate check until all page counts are known, or (b) fall back to a conservative upper-bound estimate. For example:
const unknownPageCount = pdfFiles.some((f) => f.pageCount === null || f.pageCount === undefined);
const totalPages = pdfFiles.reduce((sum, f) => sum + (f.pageCount ?? 0), 0);
if (!unknownPageCount && totalPages > aggregateConfig.maxPages) {
throw new Error(...);
}
// Optionally warn/log when unknown so operators know the guard was partial.At minimum, the aggregate guard should not silently pass when page counts are missing.
| PDF_LIMITS.PAGE_COUNT_TIMEOUT_MS, | ||
| ), | ||
| ), | ||
| ]); |
Contributor
There was a problem hiding this comment.
🔒 CRITICAL: getAccuratePageCount() starts a setTimeout inside Promise.race but never clears it when pdf.getInfo() wins. The dangling timer can keep the process alive and does unnecessary work.
Suggested fix:
const timeout = setTimeout(
() => reject(new Error("page-count timeout")),
PDF_LIMITS.PAGE_COUNT_TIMEOUT_MS,
);
try {
const info = await Promise.race([pdf.getInfo(), timeoutPromise]);
// ...
return total;
} finally {
clearTimeout(timeout);
await (pdf as unknown as { destroy?: () => Promise<void> | void }).destroy?.();
}Also consider timeout.unref() so it never delays process exit even if the race is still pending when the process wants to shut down.
| await ( | ||
| pdf as unknown as { destroy?: () => Promise<void> | void } | ||
| ).destroy?.(); | ||
| } |
Contributor
There was a problem hiding this comment.
pdf.destroy() throws, the outer try in getAccuratePageCount() treats the whole accurate page count as failed and returns null, even when getInfo() succeeded. Cleanup failures should not discard a valid page count.
Suggested fix (matching the pattern already used in fileReferenceRegistry.ts):
} finally {
await (
pdf as unknown as { destroy?: () => Promise<void> | void }
).destroy?.().catch(() => {
/* cleanup - ignore destroy errors */
});
}| * and reports progress via `options.onProgress`. A page that fails to render | ||
| * is yielded with `error` set (per-page isolation, #294) rather than aborting | ||
| * the stream. `convertToImages` is the batch wrapper over this contract. | ||
| */ |
Contributor
There was a problem hiding this comment.
💡 MINOR: The JSDoc says “convertToImages is the batch wrapper over this contract”, but convertToImages() doesn't call convertToImagesStream() (it has its own implementation). This is misleading for maintainers and readers.
Suggest rewording to avoid implying an implementation relationship, e.g. “Batch variant of the same conversion; both share validateImageConversionInput and per-page isolation semantics.”
| ``` | ||
|
|
||
| `convertToImages()` is the batch wrapper over this stream and returns the same | ||
| per-page `errors` contract. |
Contributor
There was a problem hiding this comment.
💡 MINOR: Docs say convertToImages() is “the batch wrapper over this stream”, but the implementation isn't a wrapper over convertToImagesStream() (it’s a separate batch implementation). Suggest rewording to avoid implying an implementation relationship, e.g. “Batch variant with the same per-page errors contract.”
Contributor
|
🎉 This PR is included in version 9.95.3 🎉 The release is available on: Your semantic-release bot 📦🚀 |
murdore
added a commit
that referenced
this pull request
Jul 19, 2026
…ts, url-secret redaction, supervisor identity) Closes 12 post-merge review gaps found across PRs #1191/#1195/#1201/#1202/#1203: - imageProcessor.ts: redact URLs in cache-hit debug logs; document MIN_VALID_IMAGE_SIZE threshold derivation. - logSanitize.ts: shared SENSITIVE_URL_QUERY_PARAM_DENYLIST + stripSensitiveUrlParams(), reused by imageCache.ts's normalizeUrl so cache keys (not just logs) don't retain signed-URL secrets. - fileDetector.ts: dedicated cacheKeyForUrl() helper for the URL content-type cache; loadFromURL no longer trusts a non-2xx HEAD's Content-Length for the size gate, falling through to the GET guard instead of over-rejecting. - commandFactory.ts: executeBatch now validates --csv-max-rows and wires csvOptions/pdfOptions.password into runBatchGenerate via the same buildCsvOptionsFromArgv/resolvePdfPassword helpers generate/stream use (previously silently dropped on batch). - inputValidation.ts: added PDF_SOFT_MAX_MB soft-limit warning, separate from the existing PDF_MAX_MB hard limit. - pdfProcessor.ts: getAccuratePageCount now clears its race-timer when getInfo() wins (was left dangling for PAGE_COUNT_TIMEOUT_MS every call), and a throwing pdf.destroy() no longer discards an already-obtained page count; convertToImagesStream's JSDoc no longer falsely claims to be a wrapper over convertToImages. - messageBuilder.ts: the aggregate PDF page-limit check now warns when any attached file's page count is unknown instead of silently undercounting the combined total. - proxy.ts: processLooksLikeProxySupervisor (exported for testability) now requires both 'neurolink' and 'proxy' in the process args, not 'neurolink' alone, to identify the proxy supervisor process. - docs/features/csv-support.md, docs/features/pdf-support.md: corrected encoding-detection ordering and the convertToImages batch-wrapper claim. - docs/MIGRATION.md: new — documents the three breaking changes shipped in 9.94.x-9.95.x with before/after examples and adaptation guidance. Adds 8 regression tests to test/continuous-test-suite-bugfixes.ts covering all of the above (225/225 passing, up from 217). BREAKING CHANGE: three breaking changes shipped earlier in 9.94.x-9.95.x are now documented in docs/MIGRATION.md: (1) multimodal file/CSV processing is fail-loud since v9.94.6 (throws ErrorFactory.fileProcessingFailed /csvProcessingFailed instead of log-and-skip), (2) MessageContent (src/lib/types/multimodal.ts) dropped its open index signature since v9.94.6 (closed shape, TS2353 on unknown fields), (3) BatchCommandArgs.file was renamed to .promptsFile since v9.95.1. No further behavior changes to these three in this commit — this commit only adds the migration doc.
murdore
added a commit
that referenced
this pull request
Jul 19, 2026
…ts, url-secret redaction, supervisor identity) Closes 12 post-merge review gaps found across PRs #1191/#1195/#1201/#1202/#1203: - imageProcessor.ts: redact URLs in cache-hit debug logs; document MIN_VALID_IMAGE_SIZE threshold derivation. - logSanitize.ts: shared SENSITIVE_URL_QUERY_PARAM_DENYLIST + stripSensitiveUrlParams(), reused by imageCache.ts's normalizeUrl so cache keys (not just logs) don't retain signed-URL secrets. - fileDetector.ts: dedicated cacheKeyForUrl() helper for the URL content-type cache; loadFromURL no longer trusts a non-2xx HEAD's Content-Length for the size gate, falling through to the GET guard instead of over-rejecting. - commandFactory.ts: executeBatch now validates --csv-max-rows and wires csvOptions/pdfOptions.password into runBatchGenerate via the same buildCsvOptionsFromArgv/resolvePdfPassword helpers generate/stream use (previously silently dropped on batch). - inputValidation.ts: added PDF_SOFT_MAX_MB soft-limit warning, separate from the existing PDF_MAX_MB hard limit. - pdfProcessor.ts: getAccuratePageCount now clears its race-timer when getInfo() wins (was left dangling for PAGE_COUNT_TIMEOUT_MS every call), and a throwing pdf.destroy() no longer discards an already-obtained page count; convertToImagesStream's JSDoc no longer falsely claims to be a wrapper over convertToImages. - messageBuilder.ts: the aggregate PDF page-limit check now warns when any attached file's page count is unknown instead of silently undercounting the combined total. - proxy.ts: processLooksLikeProxySupervisor (exported for testability) now requires both 'neurolink' and 'proxy' in the process args, not 'neurolink' alone, to identify the proxy supervisor process. - docs/features/csv-support.md, docs/features/pdf-support.md: corrected encoding-detection ordering and the convertToImages batch-wrapper claim. - docs/MIGRATION.md: new — documents the three breaking changes shipped in 9.94.x-9.95.x with before/after examples and adaptation guidance. Adds 8 regression tests to test/continuous-test-suite-bugfixes.ts covering all of the above (225/225 passing, up from 217). BREAKING CHANGE: three breaking changes shipped earlier in 9.94.x-9.95.x are now documented in docs/MIGRATION.md: (1) multimodal file/CSV processing is fail-loud since v9.94.6 (throws ErrorFactory.fileProcessingFailed /csvProcessingFailed instead of log-and-skip), (2) MessageContent (src/lib/types/multimodal.ts) dropped its open index signature since v9.94.6 (closed shape, TS2353 on unknown fields), (3) BatchCommandArgs.file was renamed to .promptsFile since v9.95.1. No further behavior changes to these three in this commit — this commit only adds the migration doc.
murdore
added a commit
that referenced
this pull request
Jul 19, 2026
…ts, url-secret redaction, supervisor identity) Closes 12 post-merge review gaps found across PRs #1191/#1195/#1201/#1202/#1203: - imageProcessor.ts: redact URLs in cache-hit debug logs; document MIN_VALID_IMAGE_SIZE threshold derivation. - logSanitize.ts: shared SENSITIVE_URL_QUERY_PARAM_DENYLIST + stripSensitiveUrlParams(), reused by imageCache.ts's normalizeUrl so cache keys (not just logs) don't retain signed-URL secrets. - fileDetector.ts: dedicated cacheKeyForUrl() helper for the URL content-type cache; loadFromURL no longer trusts a non-2xx HEAD's Content-Length for the size gate, falling through to the GET guard instead of over-rejecting. - commandFactory.ts: executeBatch now validates --csv-max-rows and wires csvOptions/pdfOptions.password into runBatchGenerate via the same buildCsvOptionsFromArgv/resolvePdfPassword helpers generate/stream use (previously silently dropped on batch). - inputValidation.ts: added PDF_SOFT_MAX_MB soft-limit warning, separate from the existing PDF_MAX_MB hard limit. - pdfProcessor.ts: getAccuratePageCount now clears its race-timer when getInfo() wins (was left dangling for PAGE_COUNT_TIMEOUT_MS every call), and a throwing pdf.destroy() no longer discards an already-obtained page count; convertToImagesStream's JSDoc no longer falsely claims to be a wrapper over convertToImages. - messageBuilder.ts: the aggregate PDF page-limit check now warns when any attached file's page count is unknown instead of silently undercounting the combined total. - proxy.ts: processLooksLikeProxySupervisor (exported for testability) now requires both 'neurolink' and 'proxy' in the process args, not 'neurolink' alone, to identify the proxy supervisor process. - docs/features/csv-support.md, docs/features/pdf-support.md: corrected encoding-detection ordering and the convertToImages batch-wrapper claim. - docs/MIGRATION.md: new — documents the three breaking changes shipped in 9.94.x-9.95.x with before/after examples and adaptation guidance. Adds 8 regression tests to test/continuous-test-suite-bugfixes.ts covering all of the above (225/225 passing, up from 217). BREAKING CHANGE: three breaking changes shipped earlier in 9.94.x-9.95.x are now documented in docs/MIGRATION.md: (1) multimodal file/CSV processing is fail-loud since v9.94.6 (throws ErrorFactory.fileProcessingFailed /csvProcessingFailed instead of log-and-skip), (2) MessageContent (src/lib/types/multimodal.ts) dropped its open index signature since v9.94.6 (closed shape, TS2353 on unknown fields), (3) BatchCommandArgs.file was renamed to .promptsFile since v9.95.1. No further behavior changes to these three in this commit — this commit only adds the migration doc.
murdore
added a commit
that referenced
this pull request
Jul 19, 2026
…ts, url-secret redaction, supervisor identity) Closes 12 post-merge review gaps found across PRs #1191/#1195/#1201/#1202/#1203: - imageProcessor.ts: redact URLs in cache-hit debug logs; document MIN_VALID_IMAGE_SIZE threshold derivation. - logSanitize.ts: shared SENSITIVE_URL_QUERY_PARAM_DENYLIST + stripSensitiveUrlParams(), reused by imageCache.ts's normalizeUrl so cache keys (not just logs) don't retain signed-URL secrets. - fileDetector.ts: dedicated cacheKeyForUrl() helper for the URL content-type cache; loadFromURL no longer trusts a non-2xx HEAD's Content-Length for the size gate, falling through to the GET guard instead of over-rejecting. - commandFactory.ts: executeBatch now validates --csv-max-rows and wires csvOptions/pdfOptions.password into runBatchGenerate via the same buildCsvOptionsFromArgv/resolvePdfPassword helpers generate/stream use (previously silently dropped on batch). - inputValidation.ts: added PDF_SOFT_MAX_MB soft-limit warning, separate from the existing PDF_MAX_MB hard limit. - pdfProcessor.ts: getAccuratePageCount now clears its race-timer when getInfo() wins (was left dangling for PAGE_COUNT_TIMEOUT_MS every call), and a throwing pdf.destroy() no longer discards an already-obtained page count; convertToImagesStream's JSDoc no longer falsely claims to be a wrapper over convertToImages. - messageBuilder.ts: the aggregate PDF page-limit check now warns when any attached file's page count is unknown instead of silently undercounting the combined total. - proxy.ts: processLooksLikeProxySupervisor (exported for testability) now requires both 'neurolink' and 'proxy' in the process args, not 'neurolink' alone, to identify the proxy supervisor process. - docs/features/csv-support.md, docs/features/pdf-support.md: corrected encoding-detection ordering and the convertToImages batch-wrapper claim. - docs/MIGRATION.md: new — documents the three breaking changes shipped in 9.94.x-9.95.x with before/after examples and adaptation guidance. Adds 8 regression tests to test/continuous-test-suite-bugfixes.ts covering all of the above (225/225 passing, up from 217). BREAKING CHANGE: three breaking changes shipped earlier in 9.94.x-9.95.x are now documented in docs/MIGRATION.md: (1) multimodal file/CSV processing is fail-loud since v9.94.6 (throws ErrorFactory.fileProcessingFailed /csvProcessingFailed instead of log-and-skip), (2) MessageContent (src/lib/types/multimodal.ts) dropped its open index signature since v9.94.6 (closed shape, TS2353 on unknown fields), (3) BatchCommandArgs.file was renamed to .promptsFile since v9.95.1. No further behavior changes to these three in this commit — this commit only adds the migration doc.
murdore
added a commit
that referenced
this pull request
Jul 19, 2026
…ts, url-secret redaction, supervisor identity) Closes 12 post-merge review gaps found across PRs #1191/#1195/#1201/#1202/#1203: - imageProcessor.ts: redact URLs in cache-hit debug logs; document MIN_VALID_IMAGE_SIZE threshold derivation. - logSanitize.ts: shared SENSITIVE_URL_QUERY_PARAM_DENYLIST + stripSensitiveUrlParams(), reused by imageCache.ts's normalizeUrl so cache keys (not just logs) don't retain signed-URL secrets. - fileDetector.ts: dedicated cacheKeyForUrl() helper for the URL content-type cache; loadFromURL no longer trusts a non-2xx HEAD's Content-Length for the size gate, falling through to the GET guard instead of over-rejecting. - commandFactory.ts: executeBatch now validates --csv-max-rows and wires csvOptions/pdfOptions.password into runBatchGenerate via the same buildCsvOptionsFromArgv/resolvePdfPassword helpers generate/stream use (previously silently dropped on batch). - inputValidation.ts: added PDF_SOFT_MAX_MB soft-limit warning, separate from the existing PDF_MAX_MB hard limit. - pdfProcessor.ts: getAccuratePageCount now clears its race-timer when getInfo() wins (was left dangling for PAGE_COUNT_TIMEOUT_MS every call), and a throwing pdf.destroy() no longer discards an already-obtained page count; convertToImagesStream's JSDoc no longer falsely claims to be a wrapper over convertToImages. - messageBuilder.ts: the aggregate PDF page-limit check now warns when any attached file's page count is unknown instead of silently undercounting the combined total. - proxy.ts: processLooksLikeProxySupervisor (exported for testability) now requires both 'neurolink' and 'proxy' in the process args, not 'neurolink' alone, to identify the proxy supervisor process. - docs/features/csv-support.md, docs/features/pdf-support.md: corrected encoding-detection ordering and the convertToImages batch-wrapper claim. - docs/MIGRATION.md: new — documents the three breaking changes shipped in 9.94.x-9.95.x with before/after examples and adaptation guidance. Adds 8 regression tests to test/continuous-test-suite-bugfixes.ts covering all of the above (225/225 passing, up from 217). BREAKING CHANGE: three breaking changes shipped earlier in 9.94.x-9.95.x are now documented in docs/MIGRATION.md: (1) multimodal file/CSV processing is fail-loud since v9.94.6 (throws ErrorFactory.fileProcessingFailed /csvProcessingFailed instead of log-and-skip), (2) MessageContent (src/lib/types/multimodal.ts) dropped its open index signature since v9.94.6 (closed shape, TS2353 on unknown fields), (3) BatchCommandArgs.file was renamed to .promptsFile since v9.95.1. No further behavior changes to these three in this commit — this commit only adds the migration doc.
murdore
added a commit
that referenced
this pull request
Jul 19, 2026
…ts, url-secret redaction, supervisor identity) Closes 12 post-merge review gaps found across PRs #1191/#1195/#1201/#1202/#1203: - imageProcessor.ts: redact URLs in cache-hit debug logs; document MIN_VALID_IMAGE_SIZE threshold derivation. - logSanitize.ts: shared SENSITIVE_URL_QUERY_PARAM_DENYLIST + stripSensitiveUrlParams(), reused by imageCache.ts's normalizeUrl so cache keys (not just logs) don't retain signed-URL secrets. - fileDetector.ts: dedicated cacheKeyForUrl() helper for the URL content-type cache; loadFromURL no longer trusts a non-2xx HEAD's Content-Length for the size gate, falling through to the GET guard instead of over-rejecting. - commandFactory.ts: executeBatch now validates --csv-max-rows and wires csvOptions/pdfOptions.password into runBatchGenerate via the same buildCsvOptionsFromArgv/resolvePdfPassword helpers generate/stream use (previously silently dropped on batch). - inputValidation.ts: added PDF_SOFT_MAX_MB soft-limit warning, separate from the existing PDF_MAX_MB hard limit. - pdfProcessor.ts: getAccuratePageCount now clears its race-timer when getInfo() wins (was left dangling for PAGE_COUNT_TIMEOUT_MS every call), and a throwing pdf.destroy() no longer discards an already-obtained page count; convertToImagesStream's JSDoc no longer falsely claims to be a wrapper over convertToImages. - messageBuilder.ts: the aggregate PDF page-limit check now warns when any attached file's page count is unknown instead of silently undercounting the combined total. - proxy.ts: processLooksLikeProxySupervisor (exported for testability) now requires both 'neurolink' and 'proxy' in the process args, not 'neurolink' alone, to identify the proxy supervisor process. - docs/features/csv-support.md, docs/features/pdf-support.md: corrected encoding-detection ordering and the convertToImages batch-wrapper claim. - docs/MIGRATION.md: new — documents the three breaking changes shipped in 9.94.x-9.95.x with before/after examples and adaptation guidance. Adds 8 regression tests to test/continuous-test-suite-bugfixes.ts covering all of the above (225/225 passing, up from 217). BREAKING CHANGE: three breaking changes shipped earlier in 9.94.x-9.95.x are now documented in docs/MIGRATION.md: (1) multimodal file/CSV processing is fail-loud since v9.94.6 (throws ErrorFactory.fileProcessingFailed /csvProcessingFailed instead of log-and-skip), (2) MessageContent (src/lib/types/multimodal.ts) dropped its open index signature since v9.94.6 (closed shape, TS2353 on unknown fields), (3) BatchCommandArgs.file was renamed to .promptsFile since v9.95.1. No further behavior changes to these three in this commit — this commit only adds the migration doc.
murdore
added a commit
that referenced
this pull request
Jul 19, 2026
…ts, url-secret redaction, supervisor identity) Closes 12 post-merge review gaps found across PRs #1191/#1195/#1201/#1202/#1203: - imageProcessor.ts: redact URLs in cache-hit debug logs; document MIN_VALID_IMAGE_SIZE threshold derivation. - logSanitize.ts: shared SENSITIVE_URL_QUERY_PARAM_DENYLIST + stripSensitiveUrlParams(), reused by imageCache.ts's normalizeUrl so cache keys (not just logs) don't retain signed-URL secrets. - fileDetector.ts: dedicated cacheKeyForUrl() helper for the URL content-type cache; loadFromURL no longer trusts a non-2xx HEAD's Content-Length for the size gate, falling through to the GET guard instead of over-rejecting. - commandFactory.ts: executeBatch now validates --csv-max-rows and wires csvOptions/pdfOptions.password into runBatchGenerate via the same buildCsvOptionsFromArgv/resolvePdfPassword helpers generate/stream use (previously silently dropped on batch). - inputValidation.ts: added PDF_SOFT_MAX_MB soft-limit warning, separate from the existing PDF_MAX_MB hard limit. - pdfProcessor.ts: getAccuratePageCount now clears its race-timer when getInfo() wins (was left dangling for PAGE_COUNT_TIMEOUT_MS every call), and a throwing pdf.destroy() no longer discards an already-obtained page count; convertToImagesStream's JSDoc no longer falsely claims to be a wrapper over convertToImages. - messageBuilder.ts: the aggregate PDF page-limit check now warns when any attached file's page count is unknown instead of silently undercounting the combined total. - proxy.ts: processLooksLikeProxySupervisor (exported for testability) now requires both 'neurolink' and 'proxy' in the process args, not 'neurolink' alone, to identify the proxy supervisor process. - docs/features/csv-support.md, docs/features/pdf-support.md: corrected encoding-detection ordering and the convertToImages batch-wrapper claim. - docs/MIGRATION.md: new — documents the three breaking changes shipped in 9.94.x-9.95.x with before/after examples and adaptation guidance. Adds 8 regression tests to test/continuous-test-suite-bugfixes.ts covering all of the above (225/225 passing, up from 217). BREAKING CHANGE: three breaking changes shipped earlier in 9.94.x-9.95.x are now documented in docs/MIGRATION.md: (1) multimodal file/CSV processing is fail-loud since v9.94.6 (throws ErrorFactory.fileProcessingFailed /csvProcessingFailed instead of log-and-skip), (2) MessageContent (src/lib/types/multimodal.ts) dropped its open index signature since v9.94.6 (closed shape, TS2353 on unknown fields), (3) BatchCommandArgs.file was renamed to .promptsFile since v9.95.1. No further behavior changes to these three in this commit — this commit only adds the migration doc.
murdore
added a commit
that referenced
this pull request
Jul 20, 2026
…ts, url-secret redaction, supervisor identity) Closes 12 post-merge review gaps found across PRs #1191/#1195/#1201/#1202/#1203: - imageProcessor.ts: redact URLs in cache-hit debug logs; document MIN_VALID_IMAGE_SIZE threshold derivation. - logSanitize.ts: shared SENSITIVE_URL_QUERY_PARAM_DENYLIST + stripSensitiveUrlParams(), reused by imageCache.ts's normalizeUrl so cache keys (not just logs) don't retain signed-URL secrets. - fileDetector.ts: dedicated cacheKeyForUrl() helper for the URL content-type cache; loadFromURL no longer trusts a non-2xx HEAD's Content-Length for the size gate, falling through to the GET guard instead of over-rejecting. - commandFactory.ts: executeBatch now validates --csv-max-rows and wires csvOptions/pdfOptions.password into runBatchGenerate via the same buildCsvOptionsFromArgv/resolvePdfPassword helpers generate/stream use (previously silently dropped on batch). - inputValidation.ts: added PDF_SOFT_MAX_MB soft-limit warning, separate from the existing PDF_MAX_MB hard limit. - pdfProcessor.ts: getAccuratePageCount now clears its race-timer when getInfo() wins (was left dangling for PAGE_COUNT_TIMEOUT_MS every call), and a throwing pdf.destroy() no longer discards an already-obtained page count; convertToImagesStream's JSDoc no longer falsely claims to be a wrapper over convertToImages. - messageBuilder.ts: the aggregate PDF page-limit check now warns when any attached file's page count is unknown instead of silently undercounting the combined total. - proxy.ts: processLooksLikeProxySupervisor (exported for testability) now requires both 'neurolink' and 'proxy' in the process args, not 'neurolink' alone, to identify the proxy supervisor process. - docs/features/csv-support.md, docs/features/pdf-support.md: corrected encoding-detection ordering and the convertToImages batch-wrapper claim. - docs/MIGRATION.md: new — documents the three breaking changes shipped in 9.94.x-9.95.x with before/after examples and adaptation guidance. Adds 8 regression tests to test/continuous-test-suite-bugfixes.ts covering all of the above (225/225 passing, up from 217). BREAKING CHANGE: three breaking changes shipped earlier in 9.94.x-9.95.x are now documented in docs/MIGRATION.md: (1) multimodal file/CSV processing is fail-loud since v9.94.6 (throws ErrorFactory.fileProcessingFailed /csvProcessingFailed instead of log-and-skip), (2) MessageContent (src/lib/types/multimodal.ts) dropped its open index signature since v9.94.6 (closed shape, TS2353 on unknown fields), (3) BatchCommandArgs.file was renamed to .promptsFile since v9.95.1. No further behavior changes to these three in this commit — this commit only adds the migration doc.
This was referenced Aug 7, 2026
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Consolidated hardening of the PDF image-fallback path (providers without native PDF support).
getAccuratePageCount()parses via pdf-parse/pdfjs (time-bounded byPAGE_COUNT_TIMEOUT_MS);process()prefers it for limit enforcement + reportedestimatedPages, falling back to the regex on timeout/failure.getPage()loop with per-pagetry/catch: a failing page is recorded inresult.errors({ page, error }) instead of discarding all converted pages.2 → 1.5; fullMIN_SCALE(0.1)…MAX_SCALE(10)range enforced; memory estimate logged before conversion.convertToImagesStream()async generator yields each page as it renders with anonProgresscallback;convertToImagesshares its validation.processExplicitPdfFileschecks combined page count + size across all PDFs against the provider ceiling.loadFromURLissues aHEADand rejects an oversizedContent-Lengthbefore downloading; missing header / HEAD-refusing server falls through to the streaming guard.requiresCitationsmetadata.requiresCitations(+ documented) so provider adapters can act on it.Verification
pnpm run check→ 0 errors ·pnpm run lint→ clean ·pnpm run build→ All good · prettier clean.updater:tests fail locally (pass in CI).test/fixtures/pdf-page3-corrupt.pdf, a real 3-page PDF with page 3's stream corrupted so only that page fails): accurate/graceful page count; per-page isolation (images.length===2,errors=[{page:3}], no throw); scale 1.5 default + memory-estimate log; streaming order + progress + early-termination; aggregate 120-page rejection; URL pre-flight (oversized rejected before GET via a localhttpserver); citations surfaced. The existing scale-validation test is extended to prove the newMIN_SCALE.Docs
docs/features/pdf-support.md: Conversion Resilience & Limits and Streaming Conversion sections.Fixes #287, #294, #297, #302, #309, #317, #349.