Repository navigation
fix(multimodal): close post-merge review gaps (batch parity, pdf limits, url-secret redaction, supervisor identity) - #1211
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
✅ 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 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR documents breaking API and CLI changes, strengthens batch and proxy validation, sanitizes URL-based cache keys, adjusts URL size preflight behavior, hardens PDF page counting, and adds regression tests covering these changes. ChangesNeuroLink hardening and migration updates
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
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 addresses a set of post-merge audit gaps across multimodal handling, CLI batch parity, PDF hardening, and URL secret hygiene, and adds targeted regression tests plus documentation of recently shipped breaking changes.
Changes:
- Aligns
neurolink batchwithgenerate/streamfor CSV validation/options and PDF password wiring; adjusts PDF warning threshold to match docs. - Hardens PDF page counting (timer cleanup, destroy-guard) and adds an aggregate page-limit WARN when page counts are unknown.
- Improves URL handling (redacted cached-image log; strips sensitive query params from URL cache keys) and tightens proxy supervisor process identification; adds migration/docs updates and regression tests.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/continuous-test-suite-bugfixes.ts | Adds regression tests covering batch parity, URL cache key redaction, PDF page-limit/page-count edge cases, and proxy supervisor identity. |
| src/lib/utils/pdfProcessor.ts | Fixes timer cleanup and makes destroy() cleanup non-fatal for accurate page counting; clarifies streaming conversion docs. |
| src/lib/utils/messageBuilder.ts | WARNs when aggregate PDF page-limit verification is partial due to unknown page counts. |
| src/lib/utils/logSanitize.ts | Introduces shared sensitive query-param denylist + helper to strip them from URL objects for long-lived cache keys. |
| src/lib/utils/imageProcessor.ts | Redacts URL in cached-image debug log; expands minimum-valid-image-size documentation. |
| src/lib/utils/imageCache.ts | Strips sensitive query params from URL-based cache keys to avoid persisting secrets in-memory. |
| src/lib/utils/fileDetector.ts | Strips sensitive query params from URL content-type cache keys; avoids trusting content-length from non-2xx HEAD responses. |
| src/cli/utils/inputValidation.ts | Adds PDF_SOFT_MAX_MB=50 and uses it for --pdf warning threshold. |
| src/cli/factories/commandFactory.ts | Wires batch CSV max-rows validation, shared CSV options builder, and PDF password options into batch runs. |
| src/cli/commands/proxy.ts | Tightens supervisor PID verification (requires neurolink and proxy in args) and exports helper for testing. |
| docs/MIGRATION.md | Documents breaking changes shipped in 9.94.x–9.95.x with before/after and migration guidance. |
| docs/features/pdf-support.md | Clarifies that convertToImages() is not a wrapper over convertToImagesStream(). |
| docs/features/csv-support.md | Clarifies encoding detection order and performance characteristics. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Tara-ag
left a comment
Tara-ag
left a comment
There was a problem hiding this comment.
Review Summary
Files reviewed: 13
New issues raised: 3 (1 MAJOR, 2 MINOR)
Blocking issues: 0
Findings
-
MAJOR —
docs/MIGRATION.md: The doc retroactively documents three breaking changes, which is helpful. However, CLAUDE.md Critical Rule 5 states the public SDK API must not break existing callers. Consider adding a policy note that future breaking changes require pre-shipping migration docs, and evaluate whetherMessageContentshould retain a deprecated index signature for one minor cycle to ease adoption. -
MINOR —
src/cli/factories/commandFactory.ts:executeBatchstill constructsvideoOptionsinline rather than using a shared helper likebuildCsvOptionsFromArgv/resolvePdfPassword. ExtractingbuildVideoOptionsFromArgv(argv)would reduce drift across generate/stream/batch. -
MINOR —
src/cli/commands/proxy.ts:processLooksLikeProxySupervisoris a meaningful hardening step, but remains regex-based overpsoutput. Consider a follow-up with a stronger identity signal (startTime cross-check againstProxySupervisorState.startTime, or a nonce verified via/proc/<pid>/environ/ps -o lstart=).
What looks good
- Secret redaction is now applied to long-lived cache keys (
ImageCache,FileDetectorURL cache), not just logs. - URL pre-flight HEAD correctly ignores non-2xx
content-lengthand falls through to the streaming GET guard. getAccuratePageCountclears its race timer and guards a throwingpdf.destroy().- Batch parity fixes wire
pdfOptions.passwordand the fullcsvOptionsshape. - Eight focused regression tests cover the claimed gaps.
No hardcoded secrets, injection vectors, or other critical security issues were introduced. Approving with inline comments for the author to address at their discretion.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/lib/utils/fileDetector.ts (1)
1959-1969: 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick winRedact the URL and error message before logging.
The
HEADpre-flight error log currently surfaces the full, unredactedurl, as well as a raw error message which may also embed the URL if thrown by the HTTP client. Based on learnings, full URL strings must never be logged or surfaced; they should be sanitized withredactUrlForError()to prevent leaking presigned authentication tokens. Additionally, you can usesanitizeErrorCause(error).messageto safely extract the error text without leaking URLs.🔒️ Proposed fix
} catch (error) { if (error instanceof Error && /File too large/.test(error.message)) { throw error; } logger.debug( - `[FileDetector] HEAD pre-flight skipped for ${url}: ${ - error instanceof Error ? error.message : String(error) - }`, + `[FileDetector] HEAD pre-flight skipped for ${redactUrlForError(url)}: ${ + sanitizeErrorCause(error).message + }`, ); }🤖 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/fileDetector.ts` around lines 1959 - 1969, Update the HEAD pre-flight catch block in file detection to redact both logged values: pass url through redactUrlForError() and derive the error text from sanitizeErrorCause(error).message instead of directly using the raw error message or String(error). Keep the existing File too large rethrow behavior unchanged.Source: Learnings
src/cli/factories/commandFactory.ts (1)
4711-4716: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winBatch resolves
--pdf-passwordonce per prompt, not once per run.
batchPdfFiles(and images/csv/video) are resolved once before the loop, butresolvePdfPassword(argv)is called fresh insiderunBatchGenerateon every iteration — so a multi-prompt batch with--pdf-passwordprints the "visible in shell history" stderr warning once per prompt instead of once for the whole run.🔇 Proposed fix: hoist password resolution alongside the other batch file variables
const batchVideoFiles = CLICommandFactory.processCliVideoFiles( argv.video as string | string[] | undefined, ); + const batchPdfPassword = CLICommandFactory.resolvePdfPassword(argv);...(batchPdfFiles?.length && { pdfOptions: { - password: CLICommandFactory.resolvePdfPassword(argv), + password: batchPdfPassword, }, }),Also applies to: 4866-4870
🤖 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/cli/factories/commandFactory.ts` around lines 4711 - 4716, Resolve the PDF password once when initializing batch inputs alongside batchPdfFiles, batchVideoFiles, and the other processed files. Update runBatchGenerate to accept and reuse that resolved password for every prompt, removing the per-iteration resolvePdfPassword(argv) call while preserving existing password behavior.
🧹 Nitpick comments (1)
src/lib/utils/pdfProcessor.ts (1)
294-340: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the shared
withTimeouthelper here.
The manualPromise.raceis fine, butsrc/lib/utils/async/withTimeout.tsalready handles the timeout cleanup. Using it here would keep this pattern consistent and avoid reintroducing the same timer-leak bug later.🤖 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/pdfProcessor.ts` around lines 294 - 340, The manual Promise.race timeout in getAccuratePageCount should be replaced with the shared withTimeout helper from the async utilities. Reuse that helper around pdf.getInfo(), remove the local timeoutHandle and related timer setup/cleanup, and preserve the existing PDF destroy cleanup and page-count fallback behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/cli/factories/commandFactory.ts`:
- Around line 4711-4716: Resolve the PDF password once when initializing batch
inputs alongside batchPdfFiles, batchVideoFiles, and the other processed files.
Update runBatchGenerate to accept and reuse that resolved password for every
prompt, removing the per-iteration resolvePdfPassword(argv) call while
preserving existing password behavior.
In `@src/lib/utils/fileDetector.ts`:
- Around line 1959-1969: Update the HEAD pre-flight catch block in file
detection to redact both logged values: pass url through redactUrlForError() and
derive the error text from sanitizeErrorCause(error).message instead of directly
using the raw error message or String(error). Keep the existing File too large
rethrow behavior unchanged.
---
Nitpick comments:
In `@src/lib/utils/pdfProcessor.ts`:
- Around line 294-340: The manual Promise.race timeout in getAccuratePageCount
should be replaced with the shared withTimeout helper from the async utilities.
Reuse that helper around pdf.getInfo(), remove the local timeoutHandle and
related timer setup/cleanup, and preserve the existing PDF destroy cleanup and
page-count fallback behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 222c1419-96e6-449b-aad4-fec753fd29b9
📒 Files selected for processing (13)
docs/MIGRATION.mddocs/features/csv-support.mddocs/features/pdf-support.mdsrc/cli/commands/proxy.tssrc/cli/factories/commandFactory.tssrc/cli/utils/inputValidation.tssrc/lib/utils/fileDetector.tssrc/lib/utils/imageCache.tssrc/lib/utils/imageProcessor.tssrc/lib/utils/logSanitize.tssrc/lib/utils/messageBuilder.tssrc/lib/utils/pdfProcessor.tstest/continuous-test-suite-bugfixes.ts
506fc3c to
da03092
Compare
🤖 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: 1
🤖 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/cli/factories/commandFactory.ts`:
- Around line 4876-4885: Hoist CLICommandFactory.resolvePdfPassword(argv) out of
the per-prompt loop in runBatchGenerate, storing its result in a batch-scoped
value before iteration. Update the batch pdfOptions construction to reuse that
cached password, ensuring the warning is emitted only once while preserving the
existing PDF option behavior.
🪄 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
Run ID: b3da4697-c08e-4667-b54a-870a38c46061
📒 Files selected for processing (13)
docs/MIGRATION.mddocs/features/csv-support.mddocs/features/pdf-support.mdsrc/cli/commands/proxy.tssrc/cli/factories/commandFactory.tssrc/cli/utils/inputValidation.tssrc/lib/utils/fileDetector.tssrc/lib/utils/imageCache.tssrc/lib/utils/imageProcessor.tssrc/lib/utils/logSanitize.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 (9)
- docs/features/pdf-support.md
- src/cli/utils/inputValidation.ts
- src/lib/utils/messageBuilder.ts
- docs/MIGRATION.md
- src/lib/utils/pdfProcessor.ts
- docs/features/csv-support.md
- src/lib/utils/imageCache.ts
- src/lib/utils/fileDetector.ts
- src/lib/utils/imageProcessor.ts
da03092 to
d2236bb
Compare
🤖 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 |
d2236bb to
599050f
Compare
🤖 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 |
Tara-ag
left a comment
Tara-ag
left a comment
There was a problem hiding this comment.
Review Summary
Files reviewed: 13
New issues raised this run: 0 (one existing unresolved CRITICAL comment remains)
Blocking issues: 1
Blocking issue
- 🔒 CRITICAL —
src/lib/utils/fileDetector.ts:1961: The HEAD pre-flight catch block still logs the rawurl, including presigned-URL query tokens/signatures. This contradicts the URL-secret hardening goal of this PR and violates the secret-exposure blocking criterion. The GET error path below already redacts correctly; the HEAD catch should use the sameredactUrlForError(url)+sanitizeErrorCause(error).messagepattern. This issue is already tracked in the pending inline comment and must be fixed before merge.
Previously raised issues
All other points from earlier review rounds ( brittle source-scan windows, GCS denylist, cache-key collision, retroactive migration docs, shared buildVideoOptionsFromArgv, proxy start-time cross-check, and per-prompt PDF-password resolution) have been addressed by the author.
What looks good
- Batch parity fixes wire the full
csvOptionsshape, validate--csv-max-rows, and resolve the PDF password once per run. - URL cache keys now strip sensitive query params while keeping distinct auth contexts distinct via the hash suffix.
- Non-2xx HEAD responses no longer trust misleading
content-length. - PDF page-count race timer is cleared and
destroy()failures are guarded. - Proxy supervisor identity is hardened with both arg matching and start-time cross-check.
- Regression tests cover the claimed gaps.
Please redact the HEAD pre-flight debug log, then this is good to approve.
599050f to
3a4d17b
Compare
🤖 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.
🧹 Nitpick comments (2)
src/lib/utils/logSanitize.ts (1)
260-279: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider adding missing AWS SigV4 parameters to the denylist.
The denylist includes all auxiliary parameters for GCP V4 signatures (e.g.,
x-goog-algorithm,x-goog-date,x-goog-expires), but omits the corresponding AWS SigV4 auxiliary parameters. While these are not strictly sensitive secrets like the signature itself, adding them ensures consistency and prevents these variable, auth-related parameters from persisting in plaintext within the cache key.💡 Proposed consistency improvement
export const SENSITIVE_URL_QUERY_PARAM_DENYLIST: readonly string[] = [ "token", "signature", "x-amz-signature", "x-amz-credential", "x-amz-security-token", + "x-amz-algorithm", + "x-amz-date", + "x-amz-expires", + "x-amz-signedheaders", "awsaccesskeyid", "expires", "sig",🤖 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/logSanitize.ts` around lines 260 - 279, Update SENSITIVE_URL_QUERY_PARAM_DENYLIST to include the missing AWS SigV4 auxiliary query parameters, specifically the X-Amz algorithm, date, expires, and signed headers keys, matching the existing lowercase naming convention and the corresponding GCP entries.src/lib/utils/pdfProcessor.ts (1)
300-321: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
unrefsupport towithTimeout, then use it here. This path needs the timer detached from process liveness; the shared helper currently clears it, but notunref().🤖 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/pdfProcessor.ts` around lines 300 - 321, Update the shared withTimeout helper to call unref() on its timer when supported, then refactor the page-count timeout flow around pdf.getInfo() to use withTimeout instead of creating and clearing a local timer. Preserve the existing timeout duration, error behavior, and total-value validation while removing the duplicated timeoutHandle management.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/lib/utils/logSanitize.ts`:
- Around line 260-279: Update SENSITIVE_URL_QUERY_PARAM_DENYLIST to include the
missing AWS SigV4 auxiliary query parameters, specifically the X-Amz algorithm,
date, expires, and signed headers keys, matching the existing lowercase naming
convention and the corresponding GCP entries.
In `@src/lib/utils/pdfProcessor.ts`:
- Around line 300-321: Update the shared withTimeout helper to call unref() on
its timer when supported, then refactor the page-count timeout flow around
pdf.getInfo() to use withTimeout instead of creating and clearing a local timer.
Preserve the existing timeout duration, error behavior, and total-value
validation while removing the duplicated timeoutHandle management.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 16e82ffa-376f-4256-b76b-c0f56720f15e
📒 Files selected for processing (13)
docs/MIGRATION.mddocs/features/csv-support.mddocs/features/pdf-support.mdsrc/cli/commands/proxy.tssrc/cli/factories/commandFactory.tssrc/cli/utils/inputValidation.tssrc/lib/utils/fileDetector.tssrc/lib/utils/imageCache.tssrc/lib/utils/imageProcessor.tssrc/lib/utils/logSanitize.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 (10)
- docs/features/pdf-support.md
- docs/MIGRATION.md
- src/lib/utils/imageCache.ts
- src/cli/utils/inputValidation.ts
- src/lib/utils/imageProcessor.ts
- src/lib/utils/fileDetector.ts
- src/lib/utils/messageBuilder.ts
- src/cli/commands/proxy.ts
- docs/features/csv-support.md
- src/cli/factories/commandFactory.ts
Tara-ag
left a comment
Tara-ag
left a comment
There was a problem hiding this comment.
Review Summary
Reviewed the 13 changed files in PR #1211. Loaded existing review comments first; no duplicate points were raised.
New issues found
Raised 6 inline comments across 6 files:
- src/cli/commands/proxy.ts —
getProcessStartTimeusesexecFileSyncinside anasyncfunction, blocking the event loop during uninstall and lacking a timeout bound. Suggested switching to asyncexecFilewrapped withwithTimeout. - src/cli/factories/commandFactory.ts — Batch
pdfOptionsis constructed with a possibly-undefinedpassword, creating a subtle inconsistency withgenerate/stream. Suggested aligning the conditional construction. - src/lib/utils/logSanitize.ts — The URL param denylist includes overly broad generic names (
token,signature,expires) that are also common non-secret query params, risking incorrect cache-key normalization. Suggested narrowing to provider-specific/auth-shaped params. - src/lib/utils/fileDetector.ts —
cacheKeyForUrlstrips sensitive params but does not strip tracking params, so URLs differing only by tracking params get distinct cache keys unlikeImageCache. - src/lib/utils/imageCache.ts — The content-hash dedup index is overwritten to the newest URL, making the older mapping unreachable; suggested keeping the oldest/stablest mapping.
- src/lib/utils/messageBuilder.ts — Aggregate PDF page-limit warning does not name which file(s) had unknown page counts, making the log less actionable.
Also left a confirmation note on src/lib/utils/pdfProcessor.ts that the race-timer/destroy fix is logically sound.
Blocking assessment
None of the newly raised issues meet the formal blocking criteria in the project standards: no hardcoded secrets, no critical security vulnerability, no new backward-compatibility break, no non-negotiable CLAUDE.md rule violation, and fewer than 3 MAJOR issues. The PR consolidates legitimate post-merge fixes and adds regression tests.
Recommendation
Approve. The two highest-impact items to address in a follow-up are the async ps call in proxy.ts and the breadth of the URL param denylist in logSanitize.ts.
3a4d17b to
dba96e3
Compare
🤖 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.
🧹 Nitpick comments (1)
test/continuous-test-suite-bugfixes.ts (1)
8338-8374: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReduce flakiness of the
ps-based proxy-supervisor tests. Both tests spawn children, wait a fixed 300 ms, then rely onps(readPsField/getProcessStartTime) to observe args and start time; under CI load the child args may not be visible within 300 ms, and on platforms lackingpsthe assertions fail rather than skip.
test/continuous-test-suite-bugfixes.ts#L8338-L8374: replace the fixed 300 ms sleep with a bounded poll untilreadPsField(pid, "args")returns the expected args (or a timeout), and gate the test onpsavailability.test/continuous-test-suite-bugfixes.ts#L8375-L8427: apply the same readiness poll/psgate, and confirmPROXY_SUPERVISOR_START_TIME_TOLERANCE_MScomfortably exceeds the drift between millisecondnew Date().toISOString()and the second-granularityps -o lstart=value socloseMatchcannot flake.🤖 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 `@test/continuous-test-suite-bugfixes.ts` around lines 8338 - 8374, In test/continuous-test-suite-bugfixes.ts:8338-8374, replace the fixed delay in the processLooksLikeProxySupervisor test with a bounded readiness poll using readPsField(pid, "args"), and skip or otherwise gate the test when ps is unavailable. Apply the same poll and ps-availability gate to the test at test/continuous-test-suite-bugfixes.ts:8375-8427, and ensure PROXY_SUPERVISOR_START_TIME_TOLERANCE_MS is comfortably larger than the millisecond-to-second timestamp precision drift so getProcessStartTime/closeMatch remains stable.
🤖 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.
Nitpick comments:
In `@test/continuous-test-suite-bugfixes.ts`:
- Around line 8338-8374: In test/continuous-test-suite-bugfixes.ts:8338-8374,
replace the fixed delay in the processLooksLikeProxySupervisor test with a
bounded readiness poll using readPsField(pid, "args"), and skip or otherwise
gate the test when ps is unavailable. Apply the same poll and ps-availability
gate to the test at test/continuous-test-suite-bugfixes.ts:8375-8427, and ensure
PROXY_SUPERVISOR_START_TIME_TOLERANCE_MS is comfortably larger than the
millisecond-to-second timestamp precision drift so
getProcessStartTime/closeMatch remains stable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 51391caf-0cad-421e-88d1-f81ba4fd8d34
📒 Files selected for processing (13)
docs/MIGRATION.mddocs/features/csv-support.mddocs/features/pdf-support.mdsrc/cli/commands/proxy.tssrc/cli/factories/commandFactory.tssrc/cli/utils/inputValidation.tssrc/lib/utils/fileDetector.tssrc/lib/utils/imageCache.tssrc/lib/utils/imageProcessor.tssrc/lib/utils/logSanitize.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 (12)
- src/lib/utils/messageBuilder.ts
- docs/MIGRATION.md
- src/lib/utils/imageCache.ts
- src/lib/utils/imageProcessor.ts
- src/lib/utils/logSanitize.ts
- docs/features/csv-support.md
- docs/features/pdf-support.md
- src/cli/utils/inputValidation.ts
- src/lib/utils/pdfProcessor.ts
- src/lib/utils/fileDetector.ts
- src/cli/factories/commandFactory.ts
- src/cli/commands/proxy.ts
Tara-ag
left a comment
Tara-ag
left a comment
There was a problem hiding this comment.
Review Summary
Files reviewed: 13
New issues raised this run: 0
Existing unresolved MAJOR issue: 1
Blocking issue
-
⚠️ MAJOR —src/cli/commands/proxy.ts:256:getProcessStartTimeparsesps -o lstart=output withnew Date(out)without accounting for the system's local timezone. Thelstartvalue is emitted in local time, while the persistedexpectedStartTimeIsois an ISO 8601 UTC string. On any machine not running UTC, the twoDateobjects will differ by the timezone offset (e.g., 5.5 hours for IST), causingdriftMsto exceed the 10 s tolerance andprocessLooksLikeProxySupervisorto returnfalsefor a legitimate supervisor. During uninstall this leaves the real supervisor running and orphans its socket.This issue is already tracked in the pending inline comment at
src/cli/commands/proxy.ts:256. The fix is to append the current local timezone offset to thelstartoutput before parsing, so both timestamps resolve to the same absolute instant.
Previously addressed issues
All other review points from earlier rounds have been resolved by the author:
- Batch parity:
--pdf-password/NEUROLINK_PDF_PASSWORDis now resolved once per run and wired intopdfOptions;--csv-max-rowsis validated; fullcsvOptionsandvideoOptionsare built via shared helpers. - PDF hardening: aggregate page-limit warns when page counts are unknown;
getAccuratePageCountclears its race timer and guards a throwingpdf.destroy(). - URL/secret handling: cache-hit debug log is redacted;
ImageCacheandFileDetectorURL caches strip sensitive query params and fold a hash of the stripped values into the key to avoid cross-auth-context collisions; non-2xx HEAD responses no longer trust misleadingcontent-length. - Proxy supervisor identity now requires both
"neurolink"and"proxy"in args and cross-checks OS-reported start time. - Breaking changes are documented in
docs/MIGRATION.mdwith a policy note for future changes. - Eight focused regression tests cover the claimed gaps.
Recommendation
Please fix the getProcessStartTime timezone parsing issue, then this is good to approve.
dba96e3 to
11be8f6
Compare
🤖 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 |
Tara-ag
left a comment
Tara-ag
left a comment
There was a problem hiding this comment.
Review Summary
Reviewed all 13 changed files. The PR addresses real post-merge gaps and the test coverage is welcome, but two critical issues and three major issues remain that must be fixed before merge.
New blocking issues raised
CRITICAL (2)
src/cli/commands/proxy.ts:256—getProcessStartTimebuilds a local-timeDateand compares it directly against a UTC ISO timestamp. On non-UTC hosts the drift will exceed the tolerance and uninstall will fail to stop a legitimate supervisor, orphaning its socket.src/lib/utils/fileDetector.ts:1966— The HEAD pre-flightcatchswallows all non-size errors atdebuglevel, including TLS/DNS/network anomalies, allowing a malicious or misconfigured endpoint to force a full streaming GET and bypass the size guard.
MAJOR (3)
src/cli/commands/proxy.ts:210—readPsFieldre-importsnode:child_process/node:utiland re-promisifiesexecFileon every call; should be module-level and useErrorFactorytyped errors consistently.src/cli/commands/proxy.ts:310—processLooksLikeProxySupervisoris exported solely for the regression test, exposing internal uninstall-path logic as public API.src/lib/utils/logSanitize.ts:278— The sensitive-param denylist includes overly broad generic names (token,signature,expires) that are commonly ordinary content-varying query params, creating a semantic mismatch and unnecessary cache-key mutation.
MINOR (5)
src/lib/utils/logSanitize.ts:340— case-insensitive stripping mutates original param casing without documenting it.src/lib/utils/fileDetector.ts:86—cacheKeyForUrlandImageCache.normalizeUrlshould share a singlenormalizeUrlForCachehelper to prevent future drift.src/cli/factories/commandFactory.ts:4885— batchpdfOptionsconstruction is inconsistent with generate/stream; consider a sharedbuildPdfOptionsFromArgvhelper.src/lib/utils/messageBuilder.ts:1326— aggregate page-limit warning logs raw filenames (caller-controlled data); consider basenaming or documenting.test/continuous-test-suite-bugfixes.ts:7780— several new tests rely on source-code string matching, which is fragile; prefer behavioral assertions where feasible.
Decision
REQUEST_CHANGES — the two CRITICAL issues (supervisor identity timezone bug and silent HEAD failure bypass) and three MAJOR issues meet the blocking criteria. Once these are addressed, the remaining MINOR items can be handled at the author's discretion.
…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.
11be8f6 to
7272535
Compare
🤖 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.
🧹 Nitpick comments (3)
src/lib/utils/logSanitize.ts (3)
309-313: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMake tracking parameter removal case-insensitive.
URLSearchParams.delete()is strictly case-sensitive. Since tracking parameters in the wild can occasionally be mixed-case (e.g.,?UTM_SOURCE=...), consider making this removal case-insensitive to match the robust behavior ofstripSensitiveUrlParamsForCacheKey.💡 Proposed refactor
export function stripTrackingParams(parsed: URL): void { - TRACKING_URL_QUERY_PARAMS.forEach((param) => - parsed.searchParams.delete(param), - ); + const keysToDelete: string[] = []; + for (const key of parsed.searchParams.keys()) { + if (TRACKING_URL_QUERY_PARAMS.includes(key.toLowerCase())) { + keysToDelete.push(key); + } + } + keysToDelete.forEach((key) => parsed.searchParams.delete(key)); }🤖 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/logSanitize.ts` around lines 309 - 313, Update stripTrackingParams to remove tracking query parameters case-insensitively, rather than relying on URLSearchParams.delete with exact casing. Compare each parsed parameter name against TRACKING_URL_QUERY_PARAMS using a case-insensitive match, while preserving non-tracking parameters.
260-279: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider adding the remaining AWS SigV4 parameters for symmetry.
The list comprehensively covers GCS metadata parameters (e.g.,
x-goog-date,x-goog-algorithm,x-goog-expires), but omits the AWS equivalents. Consider adding them so that all non-content-varying AWS auth metadata is consistently stripped from the plaintext portion of the cache key.💡 Proposed additions
"x-amz-signature", "x-amz-credential", "x-amz-security-token", + "x-amz-date", + "x-amz-algorithm", + "x-amz-expires", + "x-amz-signedheaders", "awsaccesskeyid", "expires",🤖 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/logSanitize.ts` around lines 260 - 279, Extend SENSITIVE_URL_QUERY_PARAM_DENYLIST with the remaining AWS SigV4 metadata query parameters, matching the existing x-goog entries and excluding content-varying parameters, so AWS authentication metadata is consistently removed from cache keys.
373-376: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
JSON.stringifyfor mathematically collision-free hashing.Using
.join("&")could theoretically result in a hash collision if a decoded parameter value contains an&that perfectly mimics another parameter boundary (e.g.,token=Aandsig=Bvstoken=A&sig=Bas a single parameter). While astronomically unlikely to occur naturally, usingJSON.stringifyguarantees boundary safety.💡 Proposed tweak
const hash = createHash("sha256") - .update(strippedPairs.join("&")) + .update(JSON.stringify(strippedPairs)) .digest("hex") .slice(0, 16);🤖 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/logSanitize.ts` around lines 373 - 376, Update the hash input construction in the log sanitization flow to serialize strippedPairs with JSON.stringify instead of joining with "&". Keep the existing SHA-256 hashing and 16-character hex truncation unchanged, ensuring parameter boundaries remain unambiguous.
🤖 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.
Nitpick comments:
In `@src/lib/utils/logSanitize.ts`:
- Around line 309-313: Update stripTrackingParams to remove tracking query
parameters case-insensitively, rather than relying on URLSearchParams.delete
with exact casing. Compare each parsed parameter name against
TRACKING_URL_QUERY_PARAMS using a case-insensitive match, while preserving
non-tracking parameters.
- Around line 260-279: Extend SENSITIVE_URL_QUERY_PARAM_DENYLIST with the
remaining AWS SigV4 metadata query parameters, matching the existing x-goog
entries and excluding content-varying parameters, so AWS authentication metadata
is consistently removed from cache keys.
- Around line 373-376: Update the hash input construction in the log
sanitization flow to serialize strippedPairs with JSON.stringify instead of
joining with "&". Keep the existing SHA-256 hashing and 16-character hex
truncation unchanged, ensuring parameter boundaries remain unambiguous.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5ca0ec91-2122-4f6a-9ed5-d1ab048b4ebd
📒 Files selected for processing (13)
docs/MIGRATION.mddocs/features/csv-support.mddocs/features/pdf-support.mdsrc/cli/commands/proxy.tssrc/cli/factories/commandFactory.tssrc/cli/utils/inputValidation.tssrc/lib/utils/fileDetector.tssrc/lib/utils/imageCache.tssrc/lib/utils/imageProcessor.tssrc/lib/utils/logSanitize.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 (10)
- src/cli/utils/inputValidation.ts
- docs/features/pdf-support.md
- src/lib/utils/fileDetector.ts
- src/lib/utils/imageProcessor.ts
- src/lib/utils/pdfProcessor.ts
- docs/features/csv-support.md
- src/lib/utils/messageBuilder.ts
- docs/MIGRATION.md
- src/cli/commands/proxy.ts
- src/cli/factories/commandFactory.ts
Tara-ag
left a comment
Tara-ag
left a comment
There was a problem hiding this comment.
Review Summary
Files reviewed: 13 changed files
New issues raised this run: 0 (none)
Existing review comments: Audited all 27 existing threads; each has been addressed or justified by the author.
I walked the diff file-by-file against the project standards (CLAUDE.md Critical Rules 1–6, focus areas, and blocking criteria) and against the existing review threads. No new blocking issues remain:
- Security: Secrets are redacted before logging (
imageProcessor.ts,fileDetector.ts); presigned-URL auth params are stripped and hash-suffixed for cache keys (logSanitize.ts,imageCache.ts,fileDetector.ts); HEAD pre-flight no longer trusts non-2xxcontent-length. - Architecture: CLI-only proxy hardening stays in
src/cli/commands/proxy.ts; no SDK/CLI boundary leak. Dynamic imports in the CLI command are outside the provider-registry scope of CLAUDE.md Critical Rule 1. - Backward compatibility: The PR documents already-shipped breaking changes retroactively; it does not introduce new public-API breaks.
- Correctness: Batch now wires
pdfOptions,csvOptions, andvideoOptionsthrough the same shared helpers asgenerate/stream; PDF aggregate page-limit warns on unknown counts;getAccuratePageCountclears its race timer and guards a throwingdestroy(). - Tests: 8 new regression tests cover the claimed gaps.
All previously raised concerns (broad denylist entries, exported test seam, readPsField dynamic imports, source-string assertions, pdfOptions: { password: undefined } parity, timezone/start-time parsing) have either been fixed in subsequent commits or answered with a coherent justification in the thread. I am not re-raising them.
Decision: Approve.
|
🎉 This PR is included in version 10.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Summary
After the 13-PR multimodal/CLI/PDF/proxy batch merged into
release, an audit of the review threads that were still unresolved at merge (re-review comments from Yama/CodeRabbit/Copilot on the rebased commits) surfaced 12 verified gaps still present in released code — adversarially verified againstrelease, not just taken at face value. This PR closes all of them, plus the docs/nits, in one consolidated fix.Fixes
Backward-compatibility (documented, not reverted) — 3 breaking changes shipped in 9.94.x–9.95.x are now documented in
docs/MIGRATION.mdwith before/after + migration steps (seeBREAKING CHANGE:footer):MessageContentdropped its open index signature (closed shape);BatchCommandArgs.file→.promptsFile.CLI
batchparity —neurolink batchpreviously dropped several inputs thegenerate/streampaths honor:--pdf-password/NEUROLINK_PDF_PASSWORDwas silently ignored → encrypted PDFs failed. Now wired viaresolvePdfPassword.--csv-max-rowswasn't fail-fast validated;--csv-encoding/--csv-sanitize-names/--csv-name-case/--csv-parse-timeout-mswere ignored → now built via the sharedbuildCsvOptionsFromArgv.--pdfsoft-warning fired at 100 MB while docs promise 50 MB → dedicatedPDF_SOFT_MAX_MB = 50.PDF hardening
pageCount ?? 0, silently undercounting when a PDF's page count is unknown → now WARNs (enforcement still runs on the known sum; unknown-count PDFs aren't failed outright).getAccuratePageCountnever cleared itsPromise.racetimeout (dangling 5 s handle) and a throwingpdf.destroy()infinallydiscarded a valid count → timer is cleared/unref'd and destroy is guarded.URL/secret handling
redactUrlForError.imageCacheandfileDetectorURL caches keyed on the full URL (secret query params persisted in in-memory Map keys) → a shared sensitive-param denylist strips them from cache keys.maxSizewithout checkingstatusCode→ non-2xx HEAD now falls through to the streaming GET guard.Proxy —
processLooksLikeProxySupervisormatched any process whose args merely containedneurolink→ now requires bothneurolinkand theproxysubcommand before a stale/recycled pid could be signalled during uninstall.Tests & validation
tsc --strict0 errors ·lint0 errors ·build:cligreen · pre-commit gate (validate:all + full build) green.Follow-up (out of scope, flagged)
pdf-parse(pinnedpdfjs-dist@5.4.296) andpdf-to-img(pdfjs-dist@5.4.624) are separate installs; oncePDFProcessor.convertToImages()runs in a process, subsequent realgetAccuratePageCount()calls hit a pdfjs "API version does not match Worker version" error and degrade tonullfor the rest of the process. Pre-existing, not caused by these fixes — worth a follow-up ticket.Summary by CodeRabbit
Bug Fixes
content-lengthfrom non-2xx HEAD responses.Documentation