test(multimodal): add processor-level audio, video, office and TTS suites - #1257
Conversation
✅ Single Commit Policy - COMPLIANTStatus: Policy requirements met • 1 commit • Valid format • Ready for merge 📊 View validation details📝 Commit Details
✅ Validation Results
🤖 Automated validation by NeuroLink Single Commit Enforcement |
|
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:
📝 WalkthroughWalkthroughAdded continuous test suites for audio, video, Office documents, TTS, and the multimodal SDK. Added runtime media and document fixture helpers. Added package scripts for individual suites and combined multimodal testing. ChangesMultimodal testing
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant TestSuite
participant NeuroLinkSDK
participant Provider
TestSuite->>NeuroLinkSDK: Submit audio, video, or document input
NeuroLinkSDK->>Provider: Build multimodal request
Provider-->>NeuroLinkSDK: Return generated or streamed content
NeuroLinkSDK-->>TestSuite: Expose response content
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
There was a problem hiding this comment.
Pull request overview
This PR adds previously-missing multimodal test coverage by introducing new continuous-test suites for audio, video, office documents, and TTS (unit/no-API), along with helper utilities to synthesize media/office fixtures at test runtime. It also exposes these suites via new package.json scripts so they can be run individually or as a single multimodal group.
Changes:
- Added runtime-generated fixture helpers for office formats (DOCX/XLSX) and media formats (audio/video via ffmpeg).
- Added four new continuous test suites: audio, video, office, and TTS unit (no API).
- Added
test:*scripts for the new suites plus atest:multimodalaggregate script.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/helpers/officeFixtures.ts | Adds in-memory DOCX/XLSX fixture generation and optional dependency detection. |
| test/helpers/mediaFixtures.ts | Adds ffmpeg-based audio/video fixture generation and corrupt fixture creation. |
| test/continuous-test-suite-audio.ts | Adds no-API audio detection + AudioProcessor coverage using generated fixtures. |
| test/continuous-test-suite-video.ts | Adds no-API video detection + VideoProcessor probing/keyframe coverage using generated fixtures. |
| test/continuous-test-suite-office.ts | Adds no-API Word/Excel processing coverage and ZIP/error-path guards using generated fixtures. |
| test/continuous-test-suite-tts-unit.ts | Adds no-API/unit coverage for TTSProcessor registry/validation/dispatch behavior. |
| package.json | Adds new scripts to run the added multimodal suites individually and together. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Review SummaryDecision: APPROVED ✅ This PR adds comprehensive test coverage for multimodal processing features (audio, video, office documents, TTS unit tests) that previously had no dedicated test suites. Changes Overview
Test Suites Added
Key Strengths
VerificationPer PR description:
Impact on Existing CodeNone - this is a pure test addition PR with zero modifications to production code or public APIs. The PR is ready to merge. It addresses all 15 open test issues in the multimodal backlog (#477, #483, #485, #487, #491, #493, #495, #496, #498, #499, #502, #510, #518, #527, #528) as documented in the PR description. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
test/continuous-test-suite-tts-unit.ts (1)
77-85: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert that
TTSOptionsreaches the handler.The test checks the text argument but does not check
options. IfTTSProcessor.synthesize()drops or changes provider options, this test still passes. Use a non-empty validTTSOptionsvalue and assert that the stub receives its expected fields.🤖 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-tts-unit.ts` around lines 77 - 85, Update the “synthesize dispatches to the registered handler” test to pass a non-empty valid TTSOptions object to TTSProcessor.synthesize and assert the registered handler receives the expected option fields unchanged, while preserving the existing text, invocation count, and buffer assertions.test/continuous-test-suite-audio.ts (1)
1-15: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftClose the remaining gaps from issue
#477.This suite covers metadata extraction, MIME/extension detection, and degraded-input handling. Issue
#477also requests provider selection across scenarios, size validation, supported-format checks, and transcription tests using a mocked OpenAI client, plus a stated 85% AudioProcessor coverage target. None of these appear in this file.Do you want me to draft the provider-selection, size-validation, and mocked-OpenAI-transcription tests as a follow-up addition to this suite?
Also applies to: 104-130
🤖 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-audio.ts` around lines 1 - 15, Extend the audio test suite beyond metadata and detection to cover issue `#477`’s remaining requirements: provider selection across scenarios, size-limit validation, supported-format checks, and transcription using a mocked OpenAI client. Add assertions for AudioProcessor behavior and ensure the resulting tests target the stated 85% AudioProcessor coverage goal.
🤖 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 `@test/continuous-test-suite-tts-unit.ts`:
- Around line 177-184: Add a TTSProcessor.getVoices method that accepts a
provider and locale, resolves the registered handler through the processor
dispatch path, and delegates voice retrieval to that handler. Update the
“getVoices reaches the handler” test to call TTSProcessor.getVoices(PROVIDER,
"en-US") instead of invoking getHandler and the handler directly, while
preserving the existing non-empty voice assertion.
In `@test/continuous-test-suite-video.ts`:
- Around line 91-118: Relax the hardcoded "aac" assertion in the real MP4
processing test around videoProcessor.processFile so it only verifies that
metadata.audioCodec is present, matching the existing audio-track coverage
pattern. Leave the fixture-specific AAC literal assertion in the separate
audio-track test unchanged.
---
Nitpick comments:
In `@test/continuous-test-suite-audio.ts`:
- Around line 1-15: Extend the audio test suite beyond metadata and detection to
cover issue `#477`’s remaining requirements: provider selection across scenarios,
size-limit validation, supported-format checks, and transcription using a mocked
OpenAI client. Add assertions for AudioProcessor behavior and ensure the
resulting tests target the stated 85% AudioProcessor coverage goal.
In `@test/continuous-test-suite-tts-unit.ts`:
- Around line 77-85: Update the “synthesize dispatches to the registered
handler” test to pass a non-empty valid TTSOptions object to
TTSProcessor.synthesize and assert the registered handler receives the expected
option fields unchanged, while preserving the existing text, invocation count,
and buffer assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 696a0689-35b9-4647-8369-f4c5d6a0f8a2
📒 Files selected for processing (7)
package.jsontest/continuous-test-suite-audio.tstest/continuous-test-suite-office.tstest/continuous-test-suite-tts-unit.tstest/continuous-test-suite-video.tstest/helpers/mediaFixtures.tstest/helpers/officeFixtures.ts
9ee52f3 to
f95f5d4
Compare
murdore
changed the title
🤖 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 |
|
💬 MINOR: Duration format inconsistency between video and audio processors VideoProcessor renders duration as "2s" while AudioProcessor uses "0:00" (m:ss) format. Both feed the same model so the inconsistency may confuse LLM prompts. Suggestion: Align the duration formatting across sibling processors. Either adopt m:ss for both or 2s style consistently. See line 139 in |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/continuous-test-suite-video.ts (1)
91-118: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRelax the hardcoded
"aac"audio-codec assertion.
makeVideoFile()does not pass-c:afor this MP4 fixture, so ffmpeg selects the MP4 muxer's default audio codec. That default depends on the ffmpeg build, so line 117 can fail on setups that do not default to native AAC.Use the same pattern already applied at line 132:
assert(Boolean(result.data.metadata.audioCodec), ...).🐛 Proposed fix
- assertEqual(metadata.audioCodec, "aac", "muxed audio codec identified"); + assert(Boolean(metadata.audioCodec), "muxed audio codec identified");🤖 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-video.ts` around lines 91 - 118, In the real MP4 probe test, replace the hardcoded audioCodec equality assertion with the existing truthiness-check pattern used near line 132, asserting that result.data.metadata.audioCodec is present without requiring a specific codec.
🧹 Nitpick comments (1)
test/continuous-test-suite-audio.ts (1)
3-15: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAdd the missing AUDIO-029 audio coverage.
This suite covers metadata extraction, FileDetector routing, degraded input, and text content, but no tests cover provider selection, file-size validation, or mocked OpenAI transcription. Add those cases to
test/continuous-test-suite-audio.tsand update the AUDIO-029/#477 header so the listed coverage matches the suite contents.🤖 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-audio.ts` around lines 3 - 15, Extend the audio suite’s existing tests to cover provider selection, file-size validation, and mocked OpenAI transcription, using the same fixtures and test structure already present in test/continuous-test-suite-audio.ts. Update the AUDIO-029/#477 header description so its listed coverage accurately includes these new cases.
🤖 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.
Duplicate comments:
In `@test/continuous-test-suite-video.ts`:
- Around line 91-118: In the real MP4 probe test, replace the hardcoded
audioCodec equality assertion with the existing truthiness-check pattern used
near line 132, asserting that result.data.metadata.audioCodec is present without
requiring a specific codec.
---
Nitpick comments:
In `@test/continuous-test-suite-audio.ts`:
- Around line 3-15: Extend the audio suite’s existing tests to cover provider
selection, file-size validation, and mocked OpenAI transcription, using the same
fixtures and test structure already present in
test/continuous-test-suite-audio.ts. Update the AUDIO-029/#477 header
description so its listed coverage accurately includes these new cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e07343ba-38eb-49f9-8523-7e86fbebcd2c
📒 Files selected for processing (7)
package.jsontest/continuous-test-suite-audio.tstest/continuous-test-suite-office.tstest/continuous-test-suite-tts-unit.tstest/continuous-test-suite-video.tstest/helpers/mediaFixtures.tstest/helpers/officeFixtures.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- package.json
- test/continuous-test-suite-office.ts
- test/continuous-test-suite-tts-unit.ts
f95f5d4 to
37e69c3
Compare
🛡️ Yama Review Verdict: CHANGES_REQUESTEDSeverity counts — 🔒 CRITICAL: 0 · 🤖 Yama Review Summary
Verified findings (3):
Findings behind this verdict
|
a0407da to
d65d6af
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 |
AudioProcessor and VideoProcessor each carried their own private formatDuration and disagreed on the result: a two-second file rendered as "0:02" from audio and "2s" from video, and a zero duration as "0:00" versus "0s". Both strings land in the textContent handed to the model, frequently in the same request when a video's muxed audio track is described alongside it, so the mismatch reads as two different facts about one file. Both now delegate to a shared formatMediaDuration(). The explicit-unit form wins over the clock form because these strings are read by a language model, not rendered in a player scrubber: "1m 30s" has one reading, while "1:30" is ambiguous between 1m30s and 1h30m and needs context that may not be present. Video previously truncated where audio rounded, so the two also disagreed on fractional durations; the shared helper rounds, and a 2.6s clip now reads "3s" on both sides. Non-finite and negative inputs render "0s" rather than a fabricated number — callers reach this on a failed probe. Raised in review on #1257. Regression test pins the format and the edge cases across both processors. Full bugfixes suite: 235 passed, 0 failed.
AudioProcessor and VideoProcessor each carried their own private formatDuration and disagreed on the result: a two-second file rendered as "0:02" from audio and "2s" from video, and a zero duration as "0:00" versus "0s". Both strings land in the textContent handed to the model, frequently in the same request when a video's muxed audio track is described alongside it, so the mismatch reads as two different facts about one file. Both now delegate to a shared formatMediaDuration(). The explicit-unit form wins over the clock form because these strings are read by a language model, not rendered in a player scrubber: "1m 30s" has one reading, while "1:30" is ambiguous between 1m30s and 1h30m and needs context that may not be present. Video previously truncated where audio rounded, so the two also disagreed on fractional durations; the shared helper rounds, and a 2.6s clip now reads "3s" on both sides. Non-finite and negative inputs render "0s" rather than a fabricated number — callers reach this on a failed probe. Raised in review on #1257. Regression test pins the format and the edge cases across both processors. Full bugfixes suite: 235 passed, 0 failed.
d65d6af to
3979392
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 |
Review SummaryThis PR adds comprehensive processor-level test suites for multimodal functionality (audio, video, office documents, TTS) that previously had no dedicated test coverage. The implementation shipped; the tests didn't. Files Changed (8 total)
Review FindingsNo issues found. All changes are test-only and follow NeuroLink's established patterns:
Impact on Existing Code
Architectural Compliance✅ All CLAUDE.md rules respected (this is a test-only PR) DecisionAPPROVED - This PR adds essential test coverage for multimodal processors that was missing. The tests are well-structured, cover edge cases, and follow NeuroLink's testing conventions. No action required beyond merging. |
…d TTS suites Audio, video, Office document and TTS processing shipped without dedicated suites. These add processor-level coverage plus an SDK-level suite that proves a file handed to generate()/stream() actually reaches the model — the seam where multimodal support really breaks, and one the processor tests cannot reach. Every live assertion is written so a refusal cannot satisfy it. That constraint came from evidence, not caution: an earlier revision asserted the response contained "VIDEO", which passes on the refusal "No video is attached.", and a third asserted "RECEIVED" after a prompt that instructed the model to say it. Assertions now key on values obtainable only from the file — a filename, a fixture's real 320x240 resolution, a cell value, a duration — each confirmed against a live negative control that receives no file. Review round 2 hardened two more of the same shape: - The Buffer-input audio test asserted only the absence of a sentinel. A live negative control answers "I apologize for the confusion…" with no file at all, which satisfies that. It now asks for the duration of a deliberately odd 7s fixture. Sample rate was tried first and rejected: the model returns "44100" with no file attached, so a guessable fact is not evidence. - The mixed-multimodal test asserted "AUDIO", which the refusal "no audio file is attached" contains. It now asserts the filename. generateNonEmpty() also retries NeuroLink's turn-ended messages, which are non-empty plausible prose carrying no answer and so pass an emptiness check. Fixtures are minted at run time with ffmpeg and adm-zip/exceljs rather than committed. Suites skip when a tool or optional dependency is absent; a dependency that is present but broken fails loudly instead. Local run: 49 passed, 4 documented skips, 0 failures. The skips are open product bugs (#1258, #1259), not missing coverage. Closes #483 Closes #485 Closes #487 Closes #495 Closes #499 Closes #518 Closes #526 Closes #530
3979392 to
bfcf5cb
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
Decision: APPROVED ✅
This pull request adds comprehensive test coverage for multimodal file processing (audio, video, office documents, and TTS). All 8 changed files are test files only with zero production code changes.
Findings Summary
- Total findings: 0
- Critical/Major issues: None
- Security concerns: None
- Breaking changes: None
Review Scope
- ✅ Reviewed all 8 changed files systematically
- ✅ Verified no hardcoded secrets or credentials in test files
- ✅ Confirmed proper error handling and graceful degradation (Skip when dependencies unavailable)
- ✅ Validated TypeScript types and imports follow project conventions
- ✅ No breaking changes to SDK API (purely additive tests)
Impact on existing code
- Changed files: 8 (all test files)
- Affected flows: None (test-only changes)
- Production impact: None
- Risk level: Low - these are test additions that don't affect runtime behavior
Files Reviewed
package.json- Added new test scripts (harmless configuration change)test/continuous-test-suite-audio.ts- Audio processor teststest/continuous-test-suite-multimodal-sdk.ts- End-to-end SDK teststest/continuous-test-suite-office.ts- Word/Excel document teststest/continuous-test-suite-tts-unit.ts- TTS unit teststest/continuous-test-suite-video.ts- Video processor teststest/helpers/mediaFixtures.ts- Media fixture generation helperstest/helpers/officeFixtures.ts- Office document fixture generation helpers
Notes
- The
VideoProcessorduration format note ("2s" vs "0:00") is a documentation comment acknowledging future work - not a code issue - All tests properly skip when ffmpeg or optional dependencies are unavailable
- Following established patterns from CONTRIBUTING.md (no vitest runner, tsx-based suites)
No inline comments required - this is a clean addition of test coverage with no code quality or security issues found.
Review Summary for PR #1257Decision: APPROVED ✅ This pull request adds comprehensive test suites for multimodal features (audio, video, office documents, TTS). All changes are confined to test files and configuration; no production code is modified. Findings
Files Changed
Impact on Existing Code
Verification Against Project Standards✅ No hardcoded secrets or credentials Notes
Review completed following Yama methodology: file-by-file analysis, evidence-based findings, and impact assessment via code knowledge graph. |
|
🎉 This PR is included in version 10.8.13 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Closes #477, #483, #485, #487, #491, #493, #495, #496, #498, #499, #502, #518, #527.
13 of the 15 filed test issues. #528 is left open deliberately (see below).
Why these were open
The implementation shipped; the tests didn't.
…while
AudioProcessor,VideoProcessor,WordProcessorandExcelProcessorare all live inrelease.Two levels — 50 passing tests
test:audiotest:videotest:officetest:tts:unittest:multimodal:sdkgenerate()/stream()The SDK suite is the one that matters for #491, #493, #498, #502 and #510: it drives real files through
generate(),stream(),FileDetector.detectAndProcess()andbuildMultimodalMessagesArray(), and asserts on content the model could only know by reading the file —FALCONinside a.docx,4242in a spreadsheet cell,PELICANin a mixed audio+spreadsheet call. "The call succeeded" cannot be mistaken for "the document arrived".Full run output and a per-issue evidence map are attached as comments below.
What the SDK level found
#1258 —
stream()drops file content on the Vertex path. Identical input, same provider:generate()"ORCHID"stream()"there are no documents attached to this conversation"Confirmed at
maxTokens: 1024. Anthropic is correct on both paths. The file is detected on the stream path, so the loss is between detection and the Vertex request. The parity test runs against Anthropic; a skipped marker keeps the Vertex gap visible until #1258 lands.This is precisely the class of defect processor-level tests cannot see, and it was invisible until these tests existed.
#528 left open
It requires real OpenAI/Google/Azure TTS calls across 6 voices with
synthesizeStream(). This key set has no OpenAI credits, so I could not verify it — leaving it open rather than claiming it.Fixtures are generated, not committed
ffmpeg mints the audio/video; a
.docxis a ZIP of XML parts built withadm-zip(a direct dependency), and an.xlsxis written by the sameexceljsthe processor reads back. Real parsers read real container headers, so synthetic bytes prove nothing — and a generated fixture cannot drift out of sync with the format the way a checked-in binary can. Missing tools or optional dependencies SKIP rather than fail.Assertions corrected against the code
successwith zeroed metadata andcodec: "unknown". Two tests asserted rejection and failed — the contract was right.TTSProcessor.synthesizeis(text, provider, options); handlers exposeisConfigured()and return{ buffer, format, size }.buildMultimodalMessagesArraytakes(options, provider, model)with a nestedinput.Two apparent bugs were investigated and withdrawn: an
audioFilespath-vs-Buffer discrepancy that turned out to be Vertex intermittently returning an empty completion (the live helper now retries once), and amaxTokenstheory disproved by the same call succeeding at 60 and 512.Findings recorded, not buried
Duration: 0:00 | Codec: unknownwith nothing marking it degraded. Audio analogue of IMG-010: No Empty Image Handling #293.durationFormattedis inconsistent —"0:00"from audio,"2s"from video.Verification
pnpm run checkpnpm run lintNot wired into CI: no workflow currently runs any continuous suite (
test (20)is formatting, linting and build). Separate gap, separate change.Summary by CodeRabbit