feat(cli): VIDEO-022 - Process video arguments with validation#895
Conversation
|
@harshitarupani-wq is attempting to deploy a commit to the Sachin Sharma's projects Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThis change adds local video file validation to the CLI, including file existence checks, supported extension validation, and permission verification. It incorporates URL detection to bypass validation for remote inputs and provides resolved-path error messages. The changes include implementation logic, comprehensive test coverage, and progress documentation. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Harshita-Rupani29
force-pushed
the
feature/video-022-cli-video-argument-processing
branch
3 times, most recently
from
2ff186a to
1ab8b6f
Compare
| * Comprehensive tests for video file processing in CLI commands: | ||
| * - Video flag definitions | ||
| * - Video file validation (existence, extensions) | ||
| * - Video options passed to SDK |
There was a problem hiding this comment.
remove the test file. testing has been moved to continous test case only
|
|
||
| // Helper method to process CLI video files | ||
| // Supported video file extensions for CLI validation | ||
| private static readonly SUPPORTED_VIDEO_EXTENSIONS = [ |
There was a problem hiding this comment.
The extension list is too small.
- Add validateVideoFiles() with existence, extension, permission checks - Support 8 video formats: .mp4, .webm, .mov, .avi, .mkv, .m4v, .wmv, .flv - Skip validation for URLs (http, https, file, data URIs) - Add descriptive error messages with resolved paths
murdore
force-pushed
the
feature/video-022-cli-video-argument-processing
branch
from
1ab8b6f to
d478c53
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/cli/factories/commandFactory.ts (1)
125-129:⚠️ Potential issue | 🟠 MajorExpand CLI extension allowlist to match SDK-supported video formats.
Line 551-Line 560 currently rejects formats that the SDK accepts (e.g.,
.3gp,.3g2,.ts,.mts,.m2ts,.ogv,.vob). This creates user-facing false validation failures beforegenerate()/stream()even reach SDK processing.🔧 Proposed fix
private static readonly SUPPORTED_VIDEO_EXTENSIONS = [ ".mp4", ".webm", ".mov", ".avi", ".mkv", ".m4v", ".wmv", ".flv", + ".3gp", + ".3g2", + ".ts", + ".mts", + ".m2ts", + ".ogv", + ".vob", ];- "Add video file for analysis (can be used multiple times) (MP4, WebM, MOV, AVI, MKV)", + "Add video file for analysis (can be used multiple times) (MP4, WebM, MOV, AVI, MKV, M4V, WMV, FLV, 3GP, 3G2, TS, MTS, M2TS, OGV, VOB)",Also applies to: 551-560
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/factories/commandFactory.ts` around lines 125 - 129, The CLI currently rejects several SDK-supported video formats; update the video option validation so the allowlist used by the commandFactory's "video" CLI option (and the validator referenced around the block that rejects extensions at the code that checks extensions near lines 551-560) includes the missing extensions: .3gp, .3g2, .ts, .mts, .m2ts, .ogv, .vob (handle case-insensitive matches and both dotted and non-dotted input if applicable). Locate the extension-checking constant or function (e.g., the allowedVideoExtensions array or validateFileExtension used by the "video" option and the generate()/stream() pre-checks) and add these extensions so the CLI no longer rejects formats the SDK accepts.
🧹 Nitpick comments (1)
src/cli/factories/commandFactory.ts (1)
565-573: Deduplicate URL detection helpers to avoid behavior drift.
isURL()andisNonLocalFileReference()implement the same protocol checks in two places. Keep one source of truth and delegate to it.♻️ Proposed refactor
private static isNonLocalFileReference(filePath: string): boolean { - const lower = filePath.toLowerCase(); - return ( - lower.startsWith("http://") || - lower.startsWith("https://") || - lower.startsWith("file://") || - lower.startsWith("data:") - ); + return CLICommandFactory.isURL(filePath); }Also applies to: 640-648
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/factories/commandFactory.ts` around lines 565 - 573, The URL/protocol checks are duplicated between isURL and isNonLocalFileReference; consolidate them by keeping one canonical helper (e.g., isURL) and have isNonLocalFileReference call/delegate to it instead of repeating the startsWith checks. Update the implementation so isNonLocalFileReference references isURL (or move the checks to a shared private helper used by both) and remove the duplicated startsWith logic in the other method to ensure a single source of truth for protocol detection.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@memory-bank/progress.md`:
- Around line 19-27: The fenced TypeScript and Bash code blocks (e.g., the
"videoOptions" block and the later example block around lines 39-49) lack
surrounding blank lines and violate MD031; insert a single blank line
immediately before and after each fenced code block so there is an empty line
above the opening ``` and an empty line below the closing ``` for both the
videoOptions block and the example commands block.
In `@test/cli/video-cli.test.ts`:
- Around line 77-119: The tests currently assert locally-defined constants
instead of the real CLI behavior; update each spec to use the actual
CLICommandFactory command builders/handlers (e.g., construct the command via
CLICommandFactory.create... or the relevant builder) and invoke the
parser/handler to obtain the defined flags/options, then assert those parsed
option definitions and the payloads passed to generate()/stream() by mocking the
SDK/session calls (spy on or stub generate() and stream() to capture arguments)
so you validate the real parser output and the payload sent to the SDK rather
than self-asserted constants.
- Around line 548-551: Replace the constant expressions inside the test "should
handle quality at boundary values" with named variables for clarity: define
minQuality = 0 and maxQuality = 100, then compute boolean checks like
isMinInRange = (minQuality >= 0 && minQuality <= 100) and isMaxInRange =
(maxQuality >= 0 && maxQuality <= 100) and assert
expect(isMinInRange).toBe(true) and expect(isMaxInRange).toBe(true); update the
test that currently uses the raw expressions so it matches the variable-based
pattern used by adjacent tests.
---
Duplicate comments:
In `@src/cli/factories/commandFactory.ts`:
- Around line 125-129: The CLI currently rejects several SDK-supported video
formats; update the video option validation so the allowlist used by the
commandFactory's "video" CLI option (and the validator referenced around the
block that rejects extensions at the code that checks extensions near lines
551-560) includes the missing extensions: .3gp, .3g2, .ts, .mts, .m2ts, .ogv,
.vob (handle case-insensitive matches and both dotted and non-dotted input if
applicable). Locate the extension-checking constant or function (e.g., the
allowedVideoExtensions array or validateFileExtension used by the "video" option
and the generate()/stream() pre-checks) and add these extensions so the CLI no
longer rejects formats the SDK accepts.
---
Nitpick comments:
In `@src/cli/factories/commandFactory.ts`:
- Around line 565-573: The URL/protocol checks are duplicated between isURL and
isNonLocalFileReference; consolidate them by keeping one canonical helper (e.g.,
isURL) and have isNonLocalFileReference call/delegate to it instead of repeating
the startsWith checks. Update the implementation so isNonLocalFileReference
references isURL (or move the checks to a shared private helper used by both)
and remove the duplicated startsWith logic in the other method to ensure a
single source of truth for protocol detection.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 10fbe9ee-883b-4c7b-8bef-9e78d1a728b3
📒 Files selected for processing (3)
memory-bank/progress.mdsrc/cli/factories/commandFactory.tstest/cli/video-cli.test.ts
| **Video Options Passed to SDK**: | ||
| ```typescript | ||
| videoOptions: { | ||
| frames: argv.videoFrames, // Number of frames to extract (default: 8) | ||
| quality: argv.videoQuality, // Frame quality 0-100 (default: 85) | ||
| format: argv.videoFormat, // jpeg or png (default: jpeg) | ||
| transcribeAudio: argv.transcribeAudio // Audio transcription (default: false) | ||
| } | ||
| ``` |
There was a problem hiding this comment.
Add blank lines around fenced code blocks to satisfy MD031.
The new section has fenced blocks without surrounding blank lines, which markdownlint flags.
📝 Proposed fix
**Video Options Passed to SDK**:
+
```typescript
videoOptions: {
frames: argv.videoFrames, // Number of frames to extract (default: 8)
quality: argv.videoQuality, // Frame quality 0-100 (default: 85)
format: argv.videoFormat, // jpeg or png (default: jpeg)
transcribeAudio: argv.transcribeAudio // Audio transcription (default: false)
}Error Handling:
@@
Example Commands:
+
# Basic video analysis
neurolink generate "Describe this video" --video path/to/video.mp4
@@
# With audio transcription
neurolink generate "Transcribe" --video video.mp4 --transcribe-audio</details>
Also applies to: 39-49
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.22.0)</summary>
[warning] 20-20: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against the current code and only fix it if needed.
In @memory-bank/progress.md around lines 19 - 27, The fenced TypeScript and Bash
code blocks (e.g., the "videoOptions" block and the later example block around
lines 39-49) lack surrounding blank lines and violate MD031; insert a single
blank line immediately before and after each fenced code block so there is an
empty line above the opening and an empty line below the closing for
both the videoOptions block and the example commands block.
</details>
<!-- fingerprinting:phantom:poseidon:hawk:7bfad6d8-1e59-4b6e-8437-60b2321fa157 -->
<!-- This is an auto-generated comment by CodeRabbit -->
| it("should define video flag with correct type", () => { | ||
| // Test the expected flag definition structure | ||
| const videoFlag = { | ||
| type: "string" as const, | ||
| description: | ||
| "Add video file for analysis (can be used multiple times) (MP4, WebM, MOV, AVI, MKV)", | ||
| }; | ||
|
|
||
| expect(videoFlag.type).toBe("string"); | ||
| expect(videoFlag.description).toContain("video file"); | ||
| expect(videoFlag.description).toContain("MP4"); | ||
| }); | ||
|
|
||
| it("should define correct video-related flags", () => { | ||
| // These are the expected video flags from VIDEO-021 | ||
| const expectedFlags = [ | ||
| "video", | ||
| "video-frames", | ||
| "video-quality", | ||
| "video-format", | ||
| "transcribe-audio", | ||
| ]; | ||
|
|
||
| // Verify all expected flags exist in documentation/implementation | ||
| expectedFlags.forEach((flag) => { | ||
| expect(flag).toBeDefined(); | ||
| }); | ||
| }); | ||
|
|
||
| it("should have correct default values for video options", () => { | ||
| // Default values as specified in VIDEO-021 | ||
| const defaults = { | ||
| "video-frames": 8, | ||
| "video-quality": 85, | ||
| "video-format": "jpeg", | ||
| "transcribe-audio": false, | ||
| }; | ||
|
|
||
| expect(defaults["video-frames"]).toBe(8); | ||
| expect(defaults["video-quality"]).toBe(85); | ||
| expect(defaults["video-format"]).toBe("jpeg"); | ||
| expect(defaults["transcribe-audio"]).toBe(false); | ||
| }); |
There was a problem hiding this comment.
These tests are mostly self-asserting and do not validate the real CLI implementation.
The suite recreates expected values/logic locally instead of invoking CLICommandFactory command builders/handlers, so implementation regressions can still pass (false confidence). Please pivot these cases to assert actual parser option definitions and payloads sent through generate()/stream() with mocked SDK/session dependencies.
Also applies to: 122-146, 200-219, 224-247, 299-337, 340-380
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/cli/video-cli.test.ts` around lines 77 - 119, The tests currently assert
locally-defined constants instead of the real CLI behavior; update each spec to
use the actual CLICommandFactory command builders/handlers (e.g., construct the
command via CLICommandFactory.create... or the relevant builder) and invoke the
parser/handler to obtain the defined flags/options, then assert those parsed
option definitions and the payloads passed to generate()/stream() by mocking the
SDK/session calls (spy on or stub generate() and stream() to capture arguments)
so you validate the real parser output and the payload sent to the SDK rather
than self-asserted constants.
| it("should handle quality at boundary values", () => { | ||
| expect(0 >= 0 && 0 <= 100).toBe(true); // Min boundary | ||
| expect(100 >= 0 && 100 <= 100).toBe(true); // Max boundary | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify constant boundary self-comparisons in the test suite.
rg -nP 'expect\(\s*(0|100)\s*>=\s*(0|100)\s*&&\s*(0|100)\s*<=\s*(0|100)\s*\)\.toBe\(true\)' test/cli/video-cli.test.tsRepository: juspay/neurolink
Length of output: 193
🏁 Script executed:
# Check for linting configuration that would flag constant expressions
fd -t f -e json -e js -e ts '(eslint|lint|tsconfig)' | head -20Repository: juspay/neurolink
Length of output: 367
🏁 Script executed:
# Read the full test context around lines 548-551
sed -n '545,555p' test/cli/video-cli.test.tsRepository: juspay/neurolink
Length of output: 419
🏁 Script executed:
# Read the eslint configuration to check for rules about constant expressions
cat eslint.config.jsRepository: juspay/neurolink
Length of output: 7944
🏁 Script executed:
# Check tsconfig.json for any compiler options that might flag constant expressions
cat tsconfig.json | head -100Repository: juspay/neurolink
Length of output: 701
Use variables instead of constant expressions for clarity in boundary tests.
Lines 549-550 test boundary values using constant expressions (0 >= 0 && 0 <= 100, 100 >= 0 && 100 <= 100). Extract these to variables for improved readability and maintainability, consistent with the pattern used in adjacent tests.
🔧 Proposed fix
it("should handle quality at boundary values", () => {
- expect(0 >= 0 && 0 <= 100).toBe(true); // Min boundary
- expect(100 >= 0 && 100 <= 100).toBe(true); // Max boundary
+ const minQuality = 0;
+ const maxQuality = 100;
+ expect(minQuality >= 0 && minQuality <= 100).toBe(true);
+ expect(maxQuality >= 0 && maxQuality <= 100).toBe(true);
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it("should handle quality at boundary values", () => { | |
| expect(0 >= 0 && 0 <= 100).toBe(true); // Min boundary | |
| expect(100 >= 0 && 100 <= 100).toBe(true); // Max boundary | |
| }); | |
| it("should handle quality at boundary values", () => { | |
| const minQuality = 0; | |
| const maxQuality = 100; | |
| expect(minQuality >= 0 && minQuality <= 100).toBe(true); | |
| expect(maxQuality >= 0 && maxQuality <= 100).toBe(true); | |
| }); |
🧰 Tools
🪛 Biome (2.4.10)
[error] 549-549: Comparing to itself is potentially pointless.
(lint/suspicious/noSelfCompare)
[error] 550-550: Comparing to itself is potentially pointless.
(lint/suspicious/noSelfCompare)
🪛 ESLint
[error] 549-549: Unexpected constant truthiness on the left-hand side of a && expression.
(no-constant-binary-expression)
[error] 550-550: Unexpected constant truthiness on the left-hand side of a && expression.
(no-constant-binary-expression)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/cli/video-cli.test.ts` around lines 548 - 551, Replace the constant
expressions inside the test "should handle quality at boundary values" with
named variables for clarity: define minQuality = 0 and maxQuality = 100, then
compute boolean checks like isMinInRange = (minQuality >= 0 && minQuality <=
100) and isMaxInRange = (maxQuality >= 0 && maxQuality <= 100) and assert
expect(isMinInRange).toBe(true) and expect(isMaxInRange).toBe(true); update the
test that currently uses the raw expressions so it matches the variable-based
pattern used by adjacent tests.
3 participants
Video CLI Flags Implementation Summary
Overview
Added comprehensive video file support to the NeuroLink CLI, enabling users to analyze videos from the command line with configurable frame extraction and audio transcription options.
Changes Made
1. Command Factory Updates (
src/cli/factories/commandFactory.ts)Added Video Flags to Common Options
Added Video Processing Helper
Integrated Video Processing in Generate Command
processCliVideoFilesIntegrated Video Processing in Stream Command
processCliVideoFilesAdded Example Commands
neurolink generate "Describe this video" --video path/to/video.mp4neurolink stream "Narrate this video" --video path/to/video.mp42. Test Suite (
test/unit/cli/video-flags.test.ts)Created comprehensive unit tests validating:
3. Verification Script (
test/unit/cli/verify-video-flags.sh)Created bash script demonstrating:
Supported Commands
Generate Command
Stream Command
Loop Command
# Interactive mode with video support (inherits all commonOptions) neurolink loop --video path/to/video.mp4Video Options
--video--video-frames--video-quality--video-format--transcribe-audioSupported Video Formats
Acceptance Criteria Status
✅ All acceptance criteria met:
Technical Implementation
Pattern Consistency
The implementation follows the existing patterns for multimodal input:
--image,--pdf, and--csvflagsArchitecture
Dependencies
This implementation depends on:
This implementation blocks:
Files Modified
src/cli/factories/commandFactory.ts- Added video flags and processingtest/unit/cli/video-flags.test.ts- Added unit teststest/unit/cli/verify-video-flags.sh- Added verification scriptTesting
Run tests with:
pnpm test test/unit/cli/video-flags.test.tsRun verification script:
Notes
Summary by CodeRabbit
Release Notes
New Features
Tests