fix(multimodal): deliver file content to the model instead of describing it - #1309
Conversation
|
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 adds native audio conversion and Gemini delivery, expands BZ2/XZ/Zstandard extraction with bounded text capture, and improves MIME, filename, presentation, and image detection. ChangesMultimodal and file processing
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant MessageBuilder
participant AudioFormatSupport
participant VertexGemini
participant GeminiAPI
MessageBuilder->>AudioFormatSupport: Convert native audio entries
AudioFormatSupport-->>MessageBuilder: Return compatible audio
MessageBuilder->>VertexGemini: Pass nativeAudioFiles
VertexGemini->>GeminiAPI: Send base64 inlineData parts
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 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 |
725f8e9 to
2de8b94
Compare
d88e8e7 to
79044fc
Compare
|
Force-pushed with review fixes. Summary of what changed and what I checked but did not change: Fixed
Checked, already correct — no change
Also
Verification after the changes
Note the 🔒 Single Commit Policy check fails on #1307 and #1309 by construction: it counts commits against |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
src/lib/processors/archive/ArchiveProcessor.ts (2)
1179-1190: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSlice the buffer before decoding, not the string after. Both sites call
decodeEntryTexton the full decompressed buffer and then truncate the result toARCHIVE_CONFIG.MAX_EXTRACT_ENTRY_SIZE. The buffer can reachARCHIVE_SECURITY.MAX_DECOMPRESSED_SIZE(100 MB), sotoString("utf-8")allocates a large string of which all but 1 MB is discarded. The/\ufffd/gscan then runs over that whole string. Truncate the buffer first.
src/lib/processors/archive/ArchiveProcessor.ts#L1179-L1190: passdecompressed.subarray(0, ARCHIVE_CONFIG.MAX_EXTRACT_ENTRY_SIZE)todecodeEntryTextand drop thegzText.slice(...)call.src/lib/processors/archive/ArchiveProcessor.ts#L1427-L1434: passdecompressed.subarray(0, ARCHIVE_CONFIG.MAX_EXTRACT_ENTRY_SIZE)todecodeEntryTextand drop thetext.slice(...)call.Note that a byte-level cut can split a multi-byte UTF-8 sequence at the boundary. That produces one replacement character, which stays far below the 5% threshold in
decodeEntryText.♻️ Proposed change at L1179-L1190
const contents = new Map<string, string>(); - const gzText = this.decodeEntryText(decompressed); + const gzText = this.decodeEntryText( + decompressed.subarray(0, ARCHIVE_CONFIG.MAX_EXTRACT_ENTRY_SIZE), + ); if (gzText !== null) { - contents.set( - innerFilename, - gzText.slice(0, ARCHIVE_CONFIG.MAX_EXTRACT_ENTRY_SIZE), - ); + contents.set(innerFilename, gzText); }🤖 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/processors/archive/ArchiveProcessor.ts` around lines 1179 - 1190, In ArchiveProcessor.ts, update both content-decoding sites at lines 1179-1190 and 1427-1434 to pass decompressed.subarray(0, ARCHIVE_CONFIG.MAX_EXTRACT_ENTRY_SIZE) into decodeEntryText, then remove the resulting gzText.slice(...) and text.slice(...) calls; preserve the existing content-setting behavior and decode validation.
1272-1310: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRoute the ZIP path through the new helpers.
The comment at Line 1275 states that
isExtractableEntryNameis shared by the ZIP and TAR paths.extractEntryContentsstill inlines the same rules: the extension and basename check at lines 1470-1479, and the NUL-byte plus replacement-character check at lines 1508-1519. Two copies of the rule remain, so the drift the comment describes is still possible.Also note the orphaned doc block at lines 1261-1271. It documents
extractEntryContentsbut now sits aboveisExtractableEntryName.♻️ Proposed change in
extractEntryContents(outside the selected range)- const ext = path.extname(e.name).toLowerCase(); - // Check by extension - if (ARCHIVE_CONFIG.EXTRACTABLE_EXTENSIONS.has(ext)) { - return true; - } - // Check for common extensionless config files - const basename = path.basename(e.name).toLowerCase(); - if (basename === "readme" || basename === "license" || basename === "makefile" || basename === "dockerfile") { - return true; - } - - return false; + return this.isExtractableEntryName(e.name);- // Simple binary detection: check for null bytes in first 512 bytes - const sample = data.slice(0, Math.min(512, data.length)); - if (sample.includes(0)) { - continue; - } - - const text = data.toString("utf-8"); - // Sanity check: skip if too many replacement characters (likely binary) - const replacementCount = (text.match(/\ufffd/g) || []).length; - if (replacementCount > text.length * 0.05) { - continue; - } + const text = this.decodeEntryText(data); + if (text === null) { + continue; + }🤖 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/processors/archive/ArchiveProcessor.ts` around lines 1272 - 1310, Update extractEntryContents to use isExtractableEntryName for entry-name filtering and decodeEntryText for byte-to-text validation, removing the duplicated extension, basename, NUL-byte, and replacement-character logic. Move the extractEntryContents documentation block so it directly precedes that method.
🤖 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/processors/archive/ArchiveProcessor.ts`:
- Around line 1348-1374: Update the external decompression flow in the execFile
call to apply ARCHIVE_CONFIG.TIMEOUT_MS, ensuring stalled bzip2, xz, or zstd
processes terminate and the surrounding Promise always settles. Preserve the
existing decompression success and null-failure behavior, and avoid expanding
the change to failure-reason reporting unless needed by the current caller
contract.
- Around line 1327-1346: Update the zstdDecompress invocation in the format ===
"zst" branch to pass an options argument containing maxOutputLength set to
ARCHIVE_SECURITY.MAX_DECOMPRESSED_SIZE before the callback. Adjust the local
function type accordingly while preserving the existing Promise-based
error-to-null behavior.
In `@src/lib/utils/fileDetector.ts`:
- Around line 1894-1901: Update the ODP handling branch to accept any successful
result with odpResult.data, even when textContent is empty. Set content to
textContent || FileDetector.formatInformativePlaceholder(...) so valid
empty-text ODP files return the ODP result instead of reaching PptxProcessor,
matching the ODT and ODS behavior.
- Around line 953-966: Update withResolvedExtension’s filename selection to use
input.filename when input is a FileWithMetadata object, before falling back to
FileDetector.deriveInputFilename(input). Preserve the existing
result.metadata?.filename precedence and extension parsing behavior so
content-based filenames such as .odp, .rtf, and .tar retain processor-specific
routing.
In `@src/lib/utils/imageProcessor.ts`:
- Around line 583-591: Update the JP2 detection condition in the image signature
logic to also validate bytes 8–11 against 0x0D, 0x0A, 0x87, and 0x0A, while
preserving the existing length, box-length, and “jP ” brand checks.
In `@src/lib/utils/messageBuilder.ts`:
- Around line 2911-2936: The eager-routing decision returned by the surrounding
classification logic must consider every available type signal, not just the
filename extension. Update the inferred-type flow and the final condition to
keep files lazy only when extension, declared MIME type, and magic-byte
detection all identify text or CSV; any non-text signal, including for
extensionless buffers or conflicting metadata such as audio MIME on a .txt file,
must route eagerly.
- Around line 831-860: Replace the synchronous readFileInputSync path used by
appendDetectedFileResult with asynchronous fs/promises file access, make
appendDetectedFileResult async, and await it from its caller. Preserve existing
handling for buffers, metadata-backed files, URLs, data URIs, and unreadable
inputs while preventing local file reads from blocking the event loop.
---
Nitpick comments:
In `@src/lib/processors/archive/ArchiveProcessor.ts`:
- Around line 1179-1190: In ArchiveProcessor.ts, update both content-decoding
sites at lines 1179-1190 and 1427-1434 to pass decompressed.subarray(0,
ARCHIVE_CONFIG.MAX_EXTRACT_ENTRY_SIZE) into decodeEntryText, then remove the
resulting gzText.slice(...) and text.slice(...) calls; preserve the existing
content-setting behavior and decode validation.
- Around line 1272-1310: Update extractEntryContents to use
isExtractableEntryName for entry-name filtering and decodeEntryText for
byte-to-text validation, removing the duplicated extension, basename, NUL-byte,
and replacement-character logic. Move the extractEntryContents documentation
block so it directly precedes that method.
🪄 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: 82f265ba-97c7-49c2-88e9-5e24622e9197
📒 Files selected for processing (9)
src/lib/adapters/audioFormatSupport.tssrc/lib/processors/archive/ArchiveProcessor.tssrc/lib/providers/googleVertex/client.tssrc/lib/types/file.tssrc/lib/types/generate.tssrc/lib/types/processor.tssrc/lib/utils/fileDetector.tssrc/lib/utils/imageProcessor.tssrc/lib/utils/messageBuilder.ts
2de8b94 to
3cc584a
Compare
79044fc to
930eba0
Compare
|
Force-pushed. All seven comments from the CodeRabbit review addressed — all seven were real. 1. Unbounded zstd output (Major). Correct. The decompressed-size guard ran after the full buffer was allocated, which is too late for a zstd bomb — a few KB expanding to gigabytes hurts at the allocation, not the check. 2. I also took the second half of that comment, which was the better catch: every non-zero exit mapped to Implementation note: the discriminant is a string, not the 3. 4. ODP with no extracted text (Minor). Correct. Gating on 5. JP2 signature only half-checked (Minor). Correct. Bytes 8–11 ( 6. Synchronous read blocking the event loop (Major). Correct, and my docblock defending the choice was wrong on its own terms: it claimed the caller "is not async", when the single call site sits directly under an 7. Eager routing stops at the filename extension (Major). Correct, and the same defect as #1308's, one predicate removed. Fixed the same way, adapted to this PR's inverted rule (eager unless text/CSV):
Buffer sniffing stays a last resort rather than a third vote, because it is partly a substring scan and would drag large HTML onto the eager path. For a mimetype to be able to override, New live test: "a mislabelled filename loses to the declared mimetype" — a real MP3 sent as I also corrected the Verification
The 9 skips are formats this machine cannot encode ( |
|
@coderabbitai review |
|
3cc584a to
befd4b5
Compare
930eba0 to
878e246
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
src/lib/utils/messageBuilder.ts (2)
2382-2393: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winValidate image bytes before vision transcoding.
toVisionCompatibleImagecan decode and transcode attacker-controlled image bytes. These new paths bypass the shared size guard before that expensive operation.
src/lib/utils/messageBuilder.ts#L2382-L2393: CallImageProcessor.validateBufferSize(source.buffer, ...)beforetoVisionCompatibleImagefor raw buffers and decoded data URIs.src/lib/utils/messageBuilder.ts#L2337-L2346: Validate the decoded data-URI buffer before passing it totoVisionCompatibleImage.🤖 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 2382 - 2393, Validate image bytes with ImageProcessor.validateBufferSize before every toVisionCompatibleImage call in src/lib/utils/messageBuilder.ts at lines 2382-2393 and 2337-2346, covering both raw buffers and decoded data-URI buffers; skip or reject oversized data before transcoding while preserving existing conversion behavior for valid images.
2201-2211: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winValidate the complete JPEG 2000 signature.
Line 2203 accepts only the first eight bytes. A JPEG 2000 signature box also requires bytes 8-11 to equal
0D 0A 87 0A. Invalid input can be labeledimage/jp2and sent through the wrong conversion path.Proposed fix
- buffer.length >= 8 && + buffer.length >= 12 && buffer[0] === 0x00 && buffer[1] === 0x00 && buffer[2] === 0x00 && buffer[3] === 0x0c && - buffer.toString("latin1", 4, 8) === "jP " + buffer.toString("latin1", 4, 8) === "jP " && + buffer[8] === 0x0d && + buffer[9] === 0x0a && + buffer[10] === 0x87 && + buffer[11] === 0x0a🤖 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 2201 - 2211, Update the JPEG 2000 detection logic in the signature-checking block to require a 12-byte buffer and validate bytes 8–11 against 0D 0A 87 0A, while preserving the existing checks for bytes 0–7 and the image/jp2 return value.test/continuous-test-suite-file-formats.ts (1)
352-357: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRemove the obsolete lazy-path size assertion for non-text fixtures.
src/lib/utils/messageBuilder.tsnow eagerly processes every recognized non-text/non-CSV type regardless of file size. This assertion does not test lazy processing for these formats. It can fail a valid delivery test only because its fixture is small.Do not include
${size}in the assertion message. The test guideline forbids runtime values in assertion messages. Keep a size-boundary assertion only for tests that intentionally exercise the text or CSV lazy path.As per coding guidelines, “Do not include payloads or actual values in assertion messages.”
🤖 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-file-formats.ts` around lines 352 - 357, Remove the SIZE_TIER_THRESHOLDS.TINY_MAX assertion and related size-based message for non-text/non-CSV fixtures in the continuous format tests. Retain size-boundary assertions only in tests intentionally exercising the text or CSV lazy path, and ensure assertion messages contain no runtime values such as size or payload data.Source: Coding guidelines
src/lib/utils/fileDetector.ts (3)
3272-3288: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject malformed UTF-8 before text heuristics.
This loop does not count invalid UTF-8 bytes such as
0xff. They decode to replacement characters, whichlooksLikeText()accepts as Unicode. Binary data without NUL or C0 control bytes can then reach CSV or text processing.Add strict UTF-8 validation for the sampled bytes before running text heuristics.
🤖 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 3272 - 3288, Update FileDetector.looksBinary to strictly validate the sampled buffer bytes as UTF-8 before applying the existing NUL and control-byte heuristics; return true for invalid UTF-8, while preserving the current empty-buffer and valid-text behavior.
2461-2473: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftValidate the path before reading its header.
When
allowedBaseDiris set, this code opensinputbeforeloadFromPath()applies its realpath containment check. This does not directly return file content, but it violates the configured filesystem boundary and can block on a named pipe.Use a shared sandbox-aware header reader that validates the path before
open()and bounds the read withwithTimeout.As per coding guidelines, “Wrap asynchronous calls with
withTimeout.”🤖 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 2461 - 2473, The header-reading logic around loadFromPath must validate input against allowedBaseDir before opening it, preventing sandbox bypasses and blocking special files. Replace the direct open/read flow with a shared sandbox-aware header reader that performs the realpath containment check first and wraps the asynchronous read withTimeout, while preserving the existing undefined-on-failure behavior and handle cleanup.Source: Coding guidelines
2696-2705: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winValidate the complete JPEG 2000 signature.
The JPEG 2000 signature box is 12 bytes, but this check validates only the first 8 bytes. A non-JP2 buffer with the same prefix is misrouted to
ImageProcessor.Proposed fix
- input.length >= 8 && + input.length >= 12 && input[0] === 0x00 && input[1] === 0x00 && input[2] === 0x00 && input[3] === 0x0c && - input.toString("latin1", 4, 8) === "jP " + input.toString("latin1", 4, 8) === "jP " && + input[8] === 0x0d && + input[9] === 0x0a && + input[10] === 0x87 && + input[11] === 0x0a🤖 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 2696 - 2705, Update the JPEG 2000 detection check in the signature branch to require all 12 bytes of the signature box, including the remaining four-byte signature value, before returning image/jp2. Preserve the existing result and confidence for valid complete signatures and reject buffers that only match the first 8 bytes.
🤖 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/types/processor.ts`:
- Around line 886-888: Update every supportedFormats value in ArchiveProcessor
to list all accepted archive variants: ZIP, TAR, TAR.GZ, TAR.BZ2, GZ, BZ2, XZ,
and ZST. Apply this consistently across all ArchiveProcessor error paths while
preserving the existing metadata contract.
In `@src/lib/utils/fileDetector.ts`:
- Around line 1890-1900: Wrap the openDocumentProcessor.processFile call in file
detection with the existing withTimeout utility, using the appropriate
processing timeout configuration. Preserve the current ODP processing inputs and
fallback behavior while ensuring stalled processing is bounded.
- Around line 554-561: Update processUnifiedFilesArray and the messageBuilder
call path to pass the original FileWithMetadata object into
FileDetector.detectAndProcess instead of only file.buffer. In detectAndProcess,
normalize the metadata object's buffer for each detection strategy and in
loadContent while retaining the filename, so extension-based routing works for
byte-backed files such as .odp, .rtf, and .tar.
In `@src/lib/utils/messageBuilder.ts`:
- Around line 843-874: The asynchronous read in readFileInputBytes must not wait
indefinitely. Wrap the readFile(file) call with the existing withTimeout
utility, supplying the appropriate timeout, while preserving the current catch
behavior that returns null for read failures or timeouts.
---
Outside diff comments:
In `@src/lib/utils/fileDetector.ts`:
- Around line 3272-3288: Update FileDetector.looksBinary to strictly validate
the sampled buffer bytes as UTF-8 before applying the existing NUL and
control-byte heuristics; return true for invalid UTF-8, while preserving the
current empty-buffer and valid-text behavior.
- Around line 2461-2473: The header-reading logic around loadFromPath must
validate input against allowedBaseDir before opening it, preventing sandbox
bypasses and blocking special files. Replace the direct open/read flow with a
shared sandbox-aware header reader that performs the realpath containment check
first and wraps the asynchronous read withTimeout, while preserving the existing
undefined-on-failure behavior and handle cleanup.
- Around line 2696-2705: Update the JPEG 2000 detection check in the signature
branch to require all 12 bytes of the signature box, including the remaining
four-byte signature value, before returning image/jp2. Preserve the existing
result and confidence for valid complete signatures and reject buffers that only
match the first 8 bytes.
In `@src/lib/utils/messageBuilder.ts`:
- Around line 2382-2393: Validate image bytes with
ImageProcessor.validateBufferSize before every toVisionCompatibleImage call in
src/lib/utils/messageBuilder.ts at lines 2382-2393 and 2337-2346, covering both
raw buffers and decoded data-URI buffers; skip or reject oversized data before
transcoding while preserving existing conversion behavior for valid images.
- Around line 2201-2211: Update the JPEG 2000 detection logic in the
signature-checking block to require a 12-byte buffer and validate bytes 8–11
against 0D 0A 87 0A, while preserving the existing checks for bytes 0–7 and the
image/jp2 return value.
In `@test/continuous-test-suite-file-formats.ts`:
- Around line 352-357: Remove the SIZE_TIER_THRESHOLDS.TINY_MAX assertion and
related size-based message for non-text/non-CSV fixtures in the continuous
format tests. Retain size-boundary assertions only in tests intentionally
exercising the text or CSV lazy path, and ensure assertion messages contain no
runtime values such as size or payload data.
🪄 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: cbb38361-f668-49c1-b5e3-0747aeb55687
📒 Files selected for processing (6)
src/lib/processors/archive/ArchiveProcessor.tssrc/lib/types/processor.tssrc/lib/utils/fileDetector.tssrc/lib/utils/imageProcessor.tssrc/lib/utils/messageBuilder.tstest/continuous-test-suite-file-formats.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/processors/archive/ArchiveProcessor.ts
- src/lib/utils/imageProcessor.ts
878e246 to
6846a20
Compare
|
Force-pushed. All four findings from this review were real; all four are fixed. 1. I did not take the suggested approach of passing the whole Verified against real bytes rather than by inspection — a 45 KB tar containing a token, handed to TAR is the sharpest case because it has no magic bytes at offset 0 — the "ustar" marker sits at byte 257 — so the name is the only thing that identifies it. New live test "a bytes-plus-name upload keeps the name detection routes on" attaches a 2. No timeout on ODP processing (Major). Fixed — wrapped in Scope note: the two sibling calls to 3. No timeout on the file read (Major). Fixed. Also worth stating plainly: a path is not guaranteed to be a regular file — a FIFO, a device node or a hung network mount all read forever — so this was a real hang, not a theoretical one. 4. Stale Verification
|
befd4b5 to
a11fc0d
Compare
6846a20 to
9057f23
Compare
a11fc0d to
89434aa
Compare
9057f23 to
39e2cb8
Compare
✅ 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 |
🤖 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 |
c728187 to
0a58865
Compare
|
Third self-review pass. Two of three dimensions came back clean; the third found one critical, fixed in CRITICAL — file preprocessing was not idempotent
That was harmless until the previous commit, because AI Studio's Guarded at the root — inside New test — "file preprocessing is idempotent for a reused input object" — verified to fail without the guard. Clean this pass
Verification on
|
🤖 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.
🔒 CRITICAL: Incorrect type import - missing AudioFormat / TTSAudioFormat barrel export
The file imports AudioFormat from '../types/audioTypes.js' but this file does not exist. The correct type name should be TTSAudioFormat which is defined in src/lib/types/tts.ts. Additionally, the barrel file (src/lib/types/index.ts) must re-export TTSAudioFormat for the import to work.
Evidence: Line 10 shows import { AudioFormat } from '../types/audioTypes.js'; but:
- The file
../types/audioTypes.jsdoes not exist - The correct type is
TTSAudioFormat(or its deprecated aliasAudioFormat) intts.ts
Fix:
-
In
src/lib/adapters/audioFormatSupport.tsline 10, change:import { AudioFormat } from '../types/audioTypes.js';
To:
import type { TTSAudioFormat } from '../types/index.js';
-
Verify
src/lib/types/tts.tsexportsTTSAudioFormat(it does - see line 13) -
Verify
src/lib/types/index.tsincludes:export * from "./tts.js";(it does - confirmed)
Tara-ag
left a comment
Tara-ag
left a comment
There was a problem hiding this comment.
🔒 CRITICAL: Incorrect type import - missing AudioFormat barrel export path
The file imports AudioFormat from '../types/audioTypes.js' but this file does not exist. The correct import should be from the barrel file '../types/index.js' which exports AudioFormat as a deprecated alias for TTSAudioFormat.
Evidence: Line 10 shows import { AudioFormat } from '../types/audioTypes.js'; but searching the types directory reveals no such file exists.
Fix: Change the import to use the correct path:
// In src/lib/adapters/audioFormatSupport.ts line 10, change:
import { AudioFormat } from '../types/audioTypes.js';
To:
import type { AudioFormat } from '../types/index.js';Verification:
src/lib/types/tts.tsexportsAudioFormat(line 107) as a deprecated alias forTTSAudioFormatsrc/lib/types/index.tsincludesexport * from "./tts.js";(line 68)
0a58865 to
d10ce1e
Compare
|
Fourth self-review pass. Three findings, all confirmed, all fixed in CRITICAL — the guard was marked before the work finished
MAJOR — the guard was keyed on the input object aloneA caller that appends a file to an input it has already used had the new file silently dropped, because the whole input was marked "done". Both are fixed by the same change: track the file entries processed, per input, and mark each entry as it completes rather than the run on entry or on exit. That is the only version correct in all three directions: Marking on exit instead would have re-processed the files that had already succeeded; marking on entry hides the failure. Per entry is right both ways. Two new tests pin it, including the partial-failure retry. CRITICAL — AI Studio ignored
|
🤖 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 complete. Changes approved. SummaryThis PR adds audio delivery support to the NeuroLink SDK, allowing users to pass audio files directly to AI providers. The implementation includes:
Impact Analysis
Security Review✅ No hardcoded secrets or credentials found Architecture Compliance✅ Follows factory pattern established in codebase RecommendationAPPROVED - The changes are well-tested, follow established patterns, add valuable functionality, and introduce no security risks or breaking changes. |
|
🔍 Review Summary - Audio Delivery PR #1309 Reviewed systematically file-by-file following Yama methodology: Files ReviewedCore Source Files
Test Files
Security Analysis ✅
Architecture Compliance ✅
Testing Coverage ✅
Impact Analysis
Previous Issues AddressedThe previous review found CRITICAL issues with incorrect type imports ( Decision: APPROVED ✅The PR successfully adds audio delivery capability to NeuroLink SDK. Implementation follows established patterns, properly tested, no security concerns, and maintains backward compatibility. High blast radius is expected due to integrating audio with existing multimodal infrastructure. No blocking issues found. Ready to merge. |
d10ce1e to
88f05df
Compare
CI fix —
|
| Gate | Result |
|---|---|
format:check |
All matched files use Prettier code style |
lint (prettier + eslint) |
0 errors, 50 warnings (all pre-existing) |
| Untracked files in worktree | none |
Commits ahead / behind release |
1 / 0, no merge commits |
One warning worth flagging honestly: fileDetector.ts detectFromBuffer trips max-lines-per-function at 398 lines. It is pre-existing — origin/release is already over the limit at 376 — but this PR grows it by 22 lines via the JP2 container/codestream branches and the filenameHint threading. Warning only, non-blocking; happy to extract those branches into a helper if a reviewer prefers.
🤖 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 |
Yama verdict on
|
Tara-ag
left a comment
Tara-ag
left a comment
There was a problem hiding this comment.
Review Summary - PR #1309: Audio Delivery & Archive Security Fixes
Decision: CHANGES_REQUESTED
Summary of Findings:
- 🔒 CRITICAL (1): Incorrect type import in audioFormatSupport.ts - missing AudioFormat barrel export path
⚠️ MAJOR (1): Security vulnerability - zstdDecompress lacks output length limit (memory exhaustion risk)- 💬 SUGGESTION (1): execFile lacks timeout guard for archive extraction
Detailed Findings:
-
CRITICAL - Type Import Error (
src/lib/adapters/audioFormatSupport.ts:10)- The file imports
AudioFormatfrom a non-existent file'../types/audioTypes.js' - Should import from barrel file
'../types/index.js'instead - This blocks compilation and runtime execution
- The file imports
-
MAJOR - Memory Exhaustion Vulnerability (
src/lib/processors/archive/ArchiveProcessor.ts:1361)- zstdDecompress call lacks
maxOutputLengthparameter - Unlike gzip decompression which has this protection, zstd can allocate unlimited memory
- Risk: DoS via oversized/malformed archive files
- Fix: Add
{ maxOutputLength: ARCHIVE_SECURITY.MAX_DECOMPRESSED_SIZE }to options
- zstdDecompress call lacks
-
SUGGESTION - Missing Timeout Guard (
src/lib/processors/archive/ArchiveProcessor.ts:1410)- execFile call has no timeout protection
- Risk: Hanging processes on malformed/truncated streams could exhaust worker pools
- Recommendation: Wrap with existing timeout utilities
Impact on Existing Code:
- Blast radius analysis not available (graph data unavailable)
- Changes are self-contained to audio format support and archive processing modules
- No unmodified callers expected to break (changes are additive)
Review Scope:
- Reviewed systematically file-by-file
- Focused on security vulnerabilities, type safety, and correctness issues
- Skipped excluded files (lockfiles, generated assets, etc.)
- Did not duplicate ESLint-enforced violations (formatting, lint, basic type checks)
Next Steps:
- Fix the type import in audioFormatSupport.ts
- Add maxOutputLength parameter to zstdDecompress call
- Consider adding timeout guard to execFile (optional improvement)
- Re-run CI checks after fixes
The PR adds valuable audio delivery capability and important archive security fixes, but the CRITICAL type import error must be resolved before merge.
| * containing only a metadata block: | ||
| * | ||
| * ## Audio File: "recording.mp3" | ||
| * Duration: 19s | Codec: MPEG 2 Layer 3 | Bitrate: 32 kbps | |
There was a problem hiding this comment.
🔒 CRITICAL: Incorrect type import - missing AudioFormat barrel export path
The file imports AudioFormat from '../types/audioTypes.js' but this file does not exist. The correct import should be from the barrel file '../types/index.js' which exports AudioFormat as a deprecated alias for TTSAudioFormat.
Evidence: Line 10 shows import { AudioFormat } from '../types/audioTypes.js'; but searching the types directory reveals no such file exists.
Fix: Change the import to use the correct path:
// In src/lib/adapters/audioFormatSupport.ts line 10, change:
import { AudioFormat } from '../types/audioTypes.js';
To:
import type { AudioFormat } from '../types/index.js';Verification:
src/lib/types/tts.tsexportsAudioFormat(line 107) as a deprecated alias forTTSAudioFormatsrc/lib/types/index.tsincludesexport * from "./tts.js";(line 68)
There was a problem hiding this comment.
Not changing this — there is no such import, and no such file.
audioFormatSupport.ts imports nothing from ../types/audioTypes.js. Its only type import is from the barrel:
import type { AudioConversionResult } from "../types/index.js";src/lib/types/audioTypes.ts does not exist, and could not: the repo forbids a Types suffix on filenames under src/lib/types/, enforced by the neurolink/no-types-suffix-filename ESLint rule. The file also never references a symbol named AudioFormat.
There was a problem hiding this comment.
🔍 Reviewing audioFormatSupport.ts - checking for hardcoded secrets and critical issues
There was a problem hiding this comment.
No action needed — this is a review-progress/summary note rather than a finding.
Replying so it is not left looking unaddressed: every substantive finding raised on this PR now has a reply on its own thread, carrying either the fix or the specific reason it is not being changed.
There was a problem hiding this comment.
🔍 Review Summary - Audio Delivery PR #1309
Reviewed systematically file-by-file:
Files Reviewed:
.scratch/probe*.ts- SKIPPED (test files in hidden directory, not meant for commit)src/lib/adapters/audioFormatSupport.ts- NEW: Audio transcoding utilitiessrc/lib/utils/messageBuilder.ts- MODIFIED: Added audio handling- Test suites updated: audio, file-formats, multimodal-sdk
Security Check:
✅ No hardcoded API keys, secrets, or credentials found
✅ No injection vulnerabilities
✅ No unsafe eval/innerHTML usage
Architecture Check:
✅ Follows factory pattern (consistent with providers, processors, etc.)
✅ Dynamic imports where needed (no circular deps)
✅ Backward compatible (all changes additive)
✅ Type safety maintained (no any or double assertions)
Testing:
✅ Multiple test suites updated for new functionality
✅ Real integration tests via tsx
✅ Error scenarios covered
Impact Analysis:
- Blast radius: 500 nodes (expected for audio integration)
- Files affected: 73 (integration points with existing multimodal flow)
- No unmodified callers broken
Decision: APPROVED
The PR adds valuable audio delivery capability to the SDK, properly integrated with existing multimodal message building. Well-tested, follows architecture patterns, no security concerns.
There was a problem hiding this comment.
No action needed — this is a review-progress/summary note rather than a finding.
Replying so it is not left looking unaddressed: every substantive finding raised on this PR now has a reply on its own thread, carrying either the fix or the specific reason it is not being changed.
| * containing only a metadata block: | ||
| * | ||
| * ## Audio File: "recording.mp3" | ||
| * Duration: 19s | Codec: MPEG 2 Layer 3 | Bitrate: 32 kbps | |
There was a problem hiding this comment.
🔒 CRITICAL: Missing AudioConversionResult type definition
The new file imports AudioConversionResult from the barrel export (../types/index.js), but this type does not exist in the codebase. The semantic search found 0 nodes matching 'AudioConversionResult' as a Type kind.
Fix required: Create the AudioConversionResult type in src/lib/types/audio.ts (or similar) and add export * from "./audio.js" to the barrel export in src/lib/types/index.ts.
This is blocking because the entire audio format support module cannot compile or run without this type being defined.
There was a problem hiding this comment.
Not changing this — the type exists and the import resolves.
src/lib/types/file.ts:49 export type AudioConversionResult = { ... }
src/lib/types/index.ts:27 export * from "./file.js";
audioFormatSupport.ts:35 import type { AudioConversionResult } from "../types/index.js";
The decisive evidence is not the grep. build-check is green on this commit and pnpm run check (tsc --noEmit --strict) reports 0 errors across 4819 files. Importing a type that does not exist is a hard compile error under --strict, so a green build and a missing type cannot both be true.
| * containing only a metadata block: | ||
| * | ||
| * ## Audio File: "recording.mp3" | ||
| * Duration: 19s | Codec: MPEG 2 Layer 3 | Bitrate: 32 kbps | |
There was a problem hiding this comment.
🔒 CRITICAL: Missing AudioConversionResult type definition
The new file imports AudioConversionResult from the barrel export (../types/index.js), but this type does not exist in the codebase. The semantic search found 0 nodes matching 'AudioConversionResult' as a Type kind.
Fix required: Create the AudioConversionResult type (along with TTSAudioFormat and AudioFormat) in src/lib/types/audio.ts (or similar) and add export * from "./audio.js" to the barrel export in src/lib/types/index.ts.
This is blocking because the entire audio format support module cannot compile or run without these types being defined.
There was a problem hiding this comment.
Not changing this — the type exists and the import resolves.
src/lib/types/file.ts:49 export type AudioConversionResult = { ... }
src/lib/types/index.ts:27 export * from "./file.js";
audioFormatSupport.ts:35 import type { AudioConversionResult } from "../types/index.js";
The decisive evidence is not the grep. build-check is green on this commit and pnpm run check (tsc --noEmit --strict) reports 0 errors across 4819 files. Importing a type that does not exist is a hard compile error under --strict, so a green build and a missing type cannot both be true.
| "audio/aiff", | ||
| "audio/x-aiff", | ||
| "audio/aac", | ||
| "audio/ogg", |
There was a problem hiding this comment.
💬 SUGGESTION: Consider adding validation for empty audio files before transcode attempt
The convertContentToProviderFormat function at line 85 should validate that the audio content is non-empty before attempting transcoding. An empty buffer would cause unnecessary processing and potentially confusing errors downstream.
Fix: Add a check early in the function:
if (!audioBuffer || audioBuffer.length === 0) {
return null; // or throw a descriptive error
}There was a problem hiding this comment.
Not changing this — an empty buffer already degrades safely.
toProviderCompatibleAudio wraps the transcode in a try/catch whose failure path is the documented fallback:
} catch (error) {
logger.warn(
`[audioFormatSupport] Could not convert ${normalized} for native delivery ` +
`— falling back to a metadata-only summary: ...`,
);
return { buffer, mimeType: normalized, converted: false };
}
An empty input reaches ffmpeg, fails, and lands there — one warning naming the format, and the metadata summary the caller had before. There is also an explicit converted.length === 0 guard on the output, which covers the case that would otherwise pass silently: a "successful" transcode producing nothing.
An early return on empty input would save an ffmpeg spawn for an input that should not occur, but it would not change the outcome — so I have kept one degradation path rather than adding a second that can drift from it.
| * tools are used when present, the same soft-dependency arrangement this | ||
| * codebase already has with ffmpeg. Absent tooling returns null and the | ||
| * caller reports the format as unsupported *on this machine* rather than | ||
| * unsupported in principle. |
There was a problem hiding this comment.
The zstandard decompression at line 1361 does not enforce an output size limit like gzip does, which could allow unbounded memory consumption from malformed or malicious input.
Evidence: The zstdDecompress call lacks the maxOutputLength option that is used for gzip decompression elsewhere in the same file.
Fix: Add output length limit parameter to the zstd decompress call similar to the gzip implementation, capping decompressed data at a reasonable maximum (e.g., 100MB):
zlib.zstdDecompress(buffer, { maxOutputLength: ARCHIVE_SECURITY.MAX_DECOMPRESSED_SIZE }, (err, result) => { ... })
There was a problem hiding this comment.
Not changing this — the comparison is inverted. Checked against the code rather than the line reference.
The in-process zstd path does pass a bound:
zstdDecompress(
buffer,
{ maxOutputLength: ARCHIVE_SECURITY.MAX_DECOMPRESSED_SIZE },
(err, res) => ...,
);
and the external-tool path caps output through maxBuffer: ARCHIVE_SECURITY.MAX_DECOMPRESSED_SIZE on its execFile call. Both were added by this PR.
The gzip comparison runs the other way. extractGzEntries and extractTarGzEntries call gunzip(buffer) with no options at all and check decompressed.length after the full buffer has been materialised. That gap is real — but it is pre-existing on release, untouched here, and the same decode-then-check shape as the base64 issue this branch does fix. Flagged for separate work rather than quietly widened in this PR.
| * containing only a metadata block: | ||
| * | ||
| * ## Audio File: "recording.mp3" | ||
| * Duration: 19s | Codec: MPEG 2 Layer 3 | Bitrate: 32 kbps | |
There was a problem hiding this comment.
🔒 CRITICAL: Incorrect type import - missing AudioFormat barrel export path
The file imports AudioFormat from '../types/audioTypes.js' but this file does not exist. The correct import should be from the barrel file '../types/index.js' which exports AudioFormat as a deprecated alias for TTSAudioFormat.
Evidence: Line 10 shows import { AudioFormat } from '../types/audioTypes.js'; but searching the types directory reveals no such file exists.
Fix: Change line 10 from:
import { AudioFormat } from '../types/audioTypes.js';
to:
import type { AudioFormat } from '../types/index.js';
Verification:
src/lib/types/tts.tsexportsAudioFormat(line 107) as a deprecated alias forTTSAudioFormatsrc/lib/types/index.tsincludesexport * from "./tts.js";(line 68)
There was a problem hiding this comment.
Not changing this — there is no such import, and no such file.
audioFormatSupport.ts imports nothing from ../types/audioTypes.js. Its only type import is from the barrel:
import type { AudioConversionResult } from "../types/index.js";
src/lib/types/audioTypes.ts does not exist, and could not: the repo forbids a Types suffix on filenames under src/lib/types/, enforced by the neurolink/no-types-suffix-filename ESLint rule. The file also never references a symbol named AudioFormat.
| * tools are used when present, the same soft-dependency arrangement this | ||
| * codebase already has with ffmpeg. Absent tooling returns null and the | ||
| * caller reports the format as unsupported *on this machine* rather than | ||
| * unsupported in principle. |
There was a problem hiding this comment.
The zstandard decompression at line 1361 does not enforce an output size limit like gzip does, which could allow unbounded memory consumption from malformed or malicious input.
Evidence: The zstdDecompress call lacks the maxOutputLength option that is used for gzip decompression elsewhere in the same file.
Fix: Add output length limit parameter to the zstd decompress call similar to the gzip implementation, capping decompressed data at a reasonable maximum (e.g., 100MB):
zlib.zstdDecompress(buffer, { maxOutputLength: ARCHIVE_SECURITY.MAX_DECOMPRESSED_SIZE }, (err, result) => { ... })
This prevents potential DoS attacks via oversized archive files.
There was a problem hiding this comment.
Not changing this — the comparison is inverted. Checked against the code rather than the line reference.
The in-process zstd path does pass a bound:
zstdDecompress(
buffer,
{ maxOutputLength: ARCHIVE_SECURITY.MAX_DECOMPRESSED_SIZE },
(err, res) => ...,
);
and the external-tool path caps output through maxBuffer: ARCHIVE_SECURITY.MAX_DECOMPRESSED_SIZE on its execFile call. Both were added by this PR.
The gzip comparison runs the other way. extractGzEntries and extractTarGzEntries call gunzip(buffer) with no options at all and check decompressed.length after the full buffer has been materialised. That gap is real — but it is pre-existing on release, untouched here, and the same decode-then-check shape as the base64 issue this branch does fix. Flagged for separate work rather than quietly widened in this PR.
|
|
||
| const tool = SINGLE_STREAM_TOOLS[format]; | ||
| try { | ||
| const { execFile } = await import("node:child_process"); |
There was a problem hiding this comment.
💬 SUGGESTION: execFile lacks timeout guard
The execFile call at line 1410 has no timeout protection, which could lead to hanging processes if the archive extraction hangs.
Fix: Wrap the execFile call with a timeout mechanism using the existing timeout utilities, similar to other async operations in the codebase. This ensures that stalled decompression processes (e.g., from malformed or truncated streams) don't hang indefinitely and exhaust worker pools.
There was a problem hiding this comment.
Not changing this — the timeout is already on the call.
There is exactly one execFile in ArchiveProcessor.ts, reached for all three of bzip2/xz/zstd via SINGLE_STREAM_TOOLS, so there is no separate unguarded xz/zstd path — they are the same line of code:
const child = execFile(
tool,
["-dc"],
{
encoding: "buffer",
maxBuffer: ARCHIVE_SECURITY.MAX_DECOMPRESSED_SIZE,
// Without this a crafted or truncated stream that makes the tool
// block forever never fires the callback, so the promise never
// settles and the request that awaits it hangs indefinitely.
timeout: ARCHIVE_CONFIG.TIMEOUT_MS,
},
(error, stdout) => { ... },
);
ARCHIVE_CONFIG.TIMEOUT_MS is 60_000. Node's native { timeout } kills the child with SIGTERM and guarantees the callback fires — the same guarantee a withTimeout() wrapper gives, plus the child actually dies instead of being left running while we stop waiting.
88f05df to
ff47639
Compare
Yama verdict on
|
| oversized decodes | bytes allocated | |
|---|---|---|
88f05df5 |
1 | 25,165,824 |
ff476397 |
0 | — |
Outcome is unchanged in both (threw: no, entry left in place for the downstream guard) — only the 24MB allocation is gone. Scope note: the encoded string is already resident when we receive it, so this bounds the additional decode, not the request as a whole.
Pinned by a new test in continuous-test-suite-multimodal-sdk.ts, which I confirmed reports ✗ / RESULT: FAIL against the unfixed code rather than being downgraded to a skip.
The six that were not
| Finding | Verdict |
|---|---|
CRITICAL: AudioConversionResult does not exist |
False — src/lib/types/file.ts:49, barrel-exported at index.ts:27. build-check is green on the same commit; under --strict a missing type cannot compile |
CRITICAL: imports AudioFormat from ../types/audioTypes.js |
False — no such import and no such file (that filename would violate the repo's no-Types-suffix rule) |
| CRITICAL/MAJOR: empty audio buffer throws and fails generation | False — caught and degraded upstream; covered by an unconvertible input degrades instead of throwing |
MAJOR: execFile has no timeout |
False — timeout: ARCHIVE_CONFIG.TIMEOUT_MS (60s) is on the call, added by this PR with a comment describing exactly this hang |
| MAJOR: zstd lacks an output bound that gzip has | False, and inverted — zstd passes maxOutputLength; gzip is the one that decompresses fully and checks afterwards |
| MINOR: functions lack parameter type annotations | False — parameters are TypeScript-typed; JSDoc @param types would be redundant |
Worth a separate look (not this PR)
The gzip comparison above is real in the opposite direction: extractGzEntries and extractTarGzEntries call gunzip(buffer) with no options and check decompressed.length afterwards — the same decode-then-check shape as the base64 bug, pre-existing on release. Out of scope here; flagging it rather than silently leaving it.
State of ff476397
Rebased onto current release (picked up 10.11.2, the Flatkey guide and the proxy fix — no file overlap, clean rebase). 1 commit ahead, 0 behind, no merge commits.
format:check clean · tsc --noEmit --strict 0 errors · eslint 0 problems · test:audio 22/0 · test:multimodal:sdk 24/0 · test:file-formats 59 passed / 9 skipped. The suites ran on the pre-rebase tree; the three commits merged underneath touch unrelated files, and CI now verifies the exact commit.
🤖 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 |
…ing it The end-to-end format suite attaches a real file hiding an un-guessable code and asks the model to read it back. On this branch's parent it passed 21 of 57 runnable formats. Almost every failure traced to one cause, with four independent defects underneath it. Files over `SIZE_TIER_THRESHOLDS.TINY_MAX` (10 KB) were registered as lazy references and a truncated slice of their raw bytes injected in place of the file. That is faithful for a file that IS text and misleading for everything else — a .rtf sliced raw is RTF control words, .bz2/.xz/.zst are compressed bytes, and the branch that extracts any of them is skipped entirely on that path. Eagerness is now decided by whether a text preview can stand in for the file, rather than by listing types one incident at a time. Plain text and CSV keep the lazy path, which is what it was built for: a truncated sample of a large CSV is a faithful sample, and the file tools can read the rest on demand. Video shows most clearly that this was a delivery problem and not a format one. Keyframe extraction is identical across containers — three frames, ~38 KB each, for mp4/wmv/flv/mpg/m2ts alike — and a frame from any of them shows the test code perfectly legibly. Only mp4 answered, because Gemini decodes an mp4 natively and never needed the frames; the other four depended on frames that were thrown away before extraction ran. Attaching audio produced a metadata block — duration, codec, sample rate — and no audio bytes, while Gemini has accepted inline audio throughout. Three layers each discarded it independently: detection kept only the summary, `buildMultimodalMessagesArray` treated an audio-only turn as text-only, and Gemini's native request shape is assembled in the Vertex client rather than taken from the AI SDK's parts, so audio had to be added there as inlineData the way PDFs and images already were. The gap survived because that metadata block answers exactly the questions a test is most tempted to ask. "How long is this?" and "what sample rate?" both succeed with nothing attached, so the existing audio tests — which assert on duration — passed throughout. Containers Gemini rejects (WMA, CAF, AU, WavPack) are transcoded to MP3; one that cannot be converted is skipped rather than sent, since an unsupported inlineData mimeType fails the whole request while the summary still stands. Providers not known to accept audio keep the previous behaviour. Content-based strategies identify a type from magic bytes and report no extension, which is honest for the type and wrong for routing: several processors are chosen by extension *after* the type is settled, because one routing type covers several formats. With a null extension those branches were unreachable, so an .rtf reached the Word processor and reported "Could not extract content" while its own RtfProcessor extracts it perfectly. The name was dropped too, and TAR has no magic bytes at offset 0 — its "ustar" marker sits at byte 257 — so a .tar arrived unidentifiable. Both are now filled in only when a strategy left them empty, so content still wins over a lying filename. Every other format returned a listing of names and sizes. TAR and GZ now capture member text (a tar entry's bytes stream past exactly once, so they are taken there), BZ2 is no longer refused outright, and XZ and ZST gain extractors — the registry has claimed both as archives since the format work landed. Node ships zstd from v22.15/23; bzip2 and xz use the system tools when present, the same soft-dependency arrangement this codebase has with ffmpeg, and their absence is reported as unsupported on this machine rather than in principle. JPEG 2000 was claimed by the registry with no signature to match it; both the JP2 container and a bare codestream are now recognised, the container check placed before ICO because their leading bytes differ by one. .odp reached the OOXML reader, which finds no ppt/slides parts in an OpenDocument package. It had been passing for the wrong reason — as a lazy reference it was described as a ZIP whose content.xml text happened to satisfy the assertion. Verified live against Vertex: 57 of 57 runnable formats now return their hidden code, from 21. The 9 remaining skips are formats this machine cannot encode (amr, ape, mid, ogv, doc, xls, ppt, 7z, rar) and name the reason. The canonical multimodal suite still passes 18/18.
ff47639 to
0039e3d
Compare
All 28 open review threads answered —
|
| Finding | Reviewer | Outcome |
|---|---|---|
decompressSingleStream treated an empty payload as failure |
Copilot | Real. Now keys on exit status; an empty Buffer is truthy, so empty.txt.xz round-trips. Also exposed a second-order bug — the in-process and external-tool backends disagreed about the same input |
readFile bounded by a race, not an abort |
CodeRabbit | Real. Fixed in this commit — see below |
The readFile one is worth recording because measuring it changed the fix. withTimeout(readFile(f)) only races; the read continues against the same descriptor. Testing both halves of the suggestion against real hangs:
| regular file | blocked FIFO | |
|---|---|---|
AbortSignal |
works — rejects AbortError, pre-call and in-flight |
no effect — pending through open and mid-read |
The signal does not rescue the FIFO case my own code comment named: a blocking read sits in the threadpool beyond the reach of a timeout or an abort. Only refusing non-regular paths prevents it. Both guards are now in place.
Not changed, with reasons (23)
| Claim | Threads | Why not |
|---|---|---|
zstd lacks maxOutputLength |
5 | Inverted. zstd passes it; gzip is the unbounded one — pre-existing on release, flagged separately |
execFile lacks a timeout |
5 | One execFile in the file, shared by bzip2/xz/zstd via SINGLE_STREAM_TOOLS, carrying timeout: 60_000 + maxBuffer |
AudioConversionResult missing |
3 | types/file.ts:49, barrel-exported. A green build-check under --strict disproves a missing type |
Imports ../types/audioTypes.js |
3 | No such import, no such file — that filename is banned by neurolink/no-types-suffix-filename |
| Audio functions lack unit tests | 3 | Covered by the suite this PR adds. Premise correction: normalizeAudioMime is not exported (module-private, line 103) |
| Missing JSDoc | 2 | Cited lines 67/78/85 are entries in a MIME Set literal, not declarations. Real functions are at 94/99/185, each documented; toProviderCompatibleAudio has full @param annotations |
| JP2 signature incomplete | 1 | All 12 bytes checked; the quoted line is one clause of a 12-way conjunction |
| Validate empty audio before transcode | 1 | Already degrades via the catch, plus an output-side length === 0 guard for the silent case |
No action (3)
Review-progress and summary notes rather than findings — answered so they do not read as ignored.
Gates on 0039e3da
tsc --noEmit --strict 0 errors · prettier clean · test:audio 22/0 · test:multimodal:sdk 24/0 · 1 ahead / 0 behind release, no merge commits.
Worth stating plainly: two of the four claims I had not previously checked turned out to be real. That is the argument for going through all 28 individually rather than pattern-matching them against the ones already refuted.
🤖 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.
🔒 CRITICAL: Missing AudioConversionResult type definition - CRITICAL compilation error
The file imports types from '../types/index.js' but AudioConversionResult type is not defined anywhere in src/lib/types/. This will cause TypeScript compilation failure as the import cannot be resolved.
Fix: Add the AudioConversionResult type definition to src/lib/types/multimodal.ts (or appropriate location) before importing it in audioFormatSupport.ts. The type should define the structure used by conversion functions like convertAudioFormat() which return { buffer, mimeType, converted }.
Tara-ag
left a comment
Tara-ag
left a comment
There was a problem hiding this comment.
💡 MINOR: Missing empty buffer validation - potential crash
Empty input buffers are not validated before processing. If an empty or null Buffer is passed to detectAudioFormat(), calling buffer.slice(0, 12) will throw RangeError, potentially crashing the caller without graceful degradation.
Fix: Add validation at the start of detectAudioFormat(): 'if (!buffer || buffer.length === 0) throw new Error("Audio buffer is required");' and ensure minimum header size check: 'if (buffer.length < 12) throw new Error("Invalid audio format: buffer too small");'
Review Summary for PR #1309: fix(multimodal): deliver file content to the model instead of describing itDecision: CHANGES_REQUESTED (due to 1 CRITICAL issue) Findings Summary:
Issues Detail:🔒 CRITICAL: src/lib/adapters/audioFormatSupport.ts:1Missing The new file imports types from '../types/index.js' but Fix: Add the AudioConversionResult type definition to src/lib/types/multimodal.ts before importing it in audioFormatSupport.ts. 💡 MINOR: src/lib/adapters/audioFormatSupport.ts:375Missing empty buffer validation - potential crash Empty input buffers are not validated before processing. If an empty or null Buffer is passed to detectAudioFormat(), calling buffer.slice(0, 12) will throw RangeError. Fix: Add validation at the start of detectAudioFormat() with buffer existence and minimum size checks. Impact on Existing Code:
Scope Verification:Reviewed all 15 changed files:
Notes:This PR implements significant improvements to multimodal content delivery by adding native audio support. Once the type definition issue is fixed, the PR should be APPROVED. |
|
🎉 This PR is included in version 10.11.3 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Stacked on #1307, which is stacked on #1308. Review/merge order: #1308 → #1307 → this.
What this is
The end-to-end format suite added in #1307 attaches a real file hiding an un-guessable code and asks the model to read it back. On this branch's parent it passed 21 of 57 runnable formats. This PR takes it to 57 of 57.
Almost every failure traced to one cause, with four independent defects underneath it.
The lazy reference path replaced files with a preview that could not represent them
Files over 10 KB were registered as lazy references and a truncated slice of their raw bytes injected in place of the file. That is faithful for a file that IS text and misleading for everything else — a
.rtfsliced raw is RTF control words,.bz2/.xz/.zstare compressed bytes, and the branch that extracts any of them is skipped entirely on that path.Eagerness is now decided by whether a text preview can stand in for the file, rather than by listing types one incident at a time. Plain text and CSV keep the lazy path — a truncated sample of a large CSV is a faithful sample, and the file tools read the rest on demand.
Video shows most clearly that this was a delivery problem and not a format one. Keyframe extraction is identical across containers — three frames, ~38 KB each, for mp4/wmv/flv/mpg/m2ts alike — and a frame from any of them shows the code perfectly legibly. Only mp4 answered, because Gemini decodes an mp4 natively and never needed the frames; the other four depended on frames thrown away before extraction ran.
Audio had never reached a model at all
Attaching audio produced a metadata block — duration, codec, sample rate — and no audio bytes, while Gemini has accepted inline audio throughout. Three layers each discarded it independently: detection kept only the summary,
buildMultimodalMessagesArraytreated an audio-only turn as text-only, and Gemini's native request shape is assembled in the Vertex client rather than taken from the AI SDK's parts.The gap survived because that metadata block answers exactly the questions a test is most tempted to ask. "How long is this?" and "what sample rate?" both succeed with nothing attached — so the existing audio tests, which assert on duration, passed throughout.
Containers Gemini rejects (WMA, CAF, AU, WavPack) are transcoded to MP3; one that cannot be converted is skipped rather than sent, since an unsupported
inlineDatamimeType fails the whole request while the summary still stands. Providers not known to accept audio keep the previous behaviour.Detection dropped the filename and its extension
Content-based strategies report no extension — honest for the type, wrong for routing, because several processors are chosen by extension after the type is settled. With a null extension those branches were unreachable, so an
.rtfreached the Word processor and reported "Could not extract content" while its ownRtfProcessorextracts it perfectly. The name was dropped too, and TAR has no magic bytes at offset 0 (itsustarmarker sits at byte 257), so a.tararrived unidentifiable. Both are filled in only when a strategy left them empty, so content still wins over a lying filename.Archives extracted only ZIP
Everything else returned a listing of names and sizes. TAR and GZ now capture member text, BZ2 is no longer refused outright, and XZ and ZST gain extractors. Node ships zstd from v22.15/23; bzip2 and xz use the system tools when present — the same soft-dependency arrangement this codebase has with ffmpeg — and their absence is reported as unsupported on this machine rather than in principle.
Two smaller gaps the suite surfaced
.odpreached the OOXML reader, which finds noppt/slidesparts in an OpenDocument package. It had been passing for the wrong reason — as a lazy reference it was described as a ZIP whosecontent.xmltext happened to satisfy the assertion.Verification
Live against Vertex:
test:file-formatstest:multimodal:sdkThe 9 skips are formats this machine cannot encode (amr, ape, mid, ogv, doc, xls, ppt, 7z, rar) and each names its reason.
pnpm run check— 4819 files, 0 errorspnpm run lint— 0 errorspnpm run build— publint cleanSummary by CodeRabbit