feat(multimodal): add comprehensive PDF file support with native docu… - #211
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 |
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds end-to-end PDF support and multimodal plumbing: new PDF types, PDFProcessor, CLI Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User (CLI/SDK)
participant CLI as CommandFactory
participant NL as NeuroLink
participant MB as MessageBuilder
participant PP as PDFProcessor
participant P as Provider
participant M as Model
U->>CLI: invoke with --pdf ./file.pdf or SDK pdfFiles
CLI->>NL: generate/stream({ input, pdfFiles })
NL->>MB: buildMultimodalOptions + buildMultimodalMessagesArray
MB->>PP: PDFProcessor.process(buffer, { provider })
PP-->>MB: FileProcessingResult (application/pdf, pages, metadata)
MB-->>NL: Multimodal messages (text + file parts)
NL->>P: execute(messages, tools?, opts)
P->>M: send multimodal request
M-->>P: response (text / tool_calls / stream)
P-->>NL: stream/chunks + tool call info
NL-->>U: aggregated stream / final result
sequenceDiagram
autonumber
participant T as Test Runner
participant NL as NeuroLink
T->>NL: create instance & run PDF tests
T->>NL: invoke dispose()
NL-->>T: confirm cleanup (MCP, listeners, caches)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
3460039 to
49b5990
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR adds comprehensive PDF file support to NeuroLink's multimodal pipeline, enabling AI-powered document analysis across multiple providers with native PDF processing capabilities. The implementation preserves visual elements like charts, tables, and images through binary document passing rather than text conversion.
- Native PDF processing with provider-specific validation and compatibility checks
- Auto-detection integration with existing multimodal file system (PDFs, CSVs, images)
- Comprehensive provider support matrix with clear error messages for unsupported providers
Reviewed Changes
Copilot reviewed 33 out of 38 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/continuous-test-suite.ts | Added 8 new PDF test cases and resource cleanup helpers for comprehensive testing |
| src/lib/utils/pdfProcessor.ts | Core PDF validation and processing logic with provider compatibility system |
| src/lib/utils/messageBuilder.ts | Enhanced multimodal message builder with PDF support and file type detection |
| src/lib/utils/fileDetector.ts | Updated file detection to include PDF magic byte validation |
| src/lib/types/*.ts | Added PDF-specific type definitions and provider configurations |
| src/lib/providers/*.ts | Updated 11 providers with PDF multimodal support and message conversion |
| src/lib/neurolink.ts | Added dispose() method for proper resource cleanup |
| src/cli/factories/commandFactory.ts | Added CLI --pdf flag support for command-line PDF processing |
| examples/pdf-analysis.ts | Comprehensive PDF usage examples and best practices |
| docs/*.md | Updated documentation with PDF support guide and feature integration |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
🤖 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/providers/amazonBedrock.ts (1)
537-567: Image/PDF blocks stripped from Converse payloadWhen we map
BedrockMessageback to AWS’sContentBlock(Line 537 onwards) we only copytext,toolUse, andtoolResult. Newly-addedimage/documententries fall through to the default branch, which emits{ text: "" }, so every image or PDF block we collected is dropped before the Converse/ConverseStream call. Please add explicit branches foritem.imageanditem.documentthat preserve theformat,name, andsource.bytesfields so the native multimodal payload actually reaches Bedrock.if (item.toolResult) { return { toolResult: { toolUseId: item.toolResult.toolUseId, content: item.toolResult.content, status: item.toolResult.status, }, } as ContentBlock; } - return { text: "" } as ContentBlock; + if (item.image) { + return { + image: { + format: item.image.format, + source: { + bytes: item.image.source?.bytes, + }, + }, + } as ContentBlock; + } + if (item.document) { + return { + document: { + format: item.document.format, + name: item.document.name, + source: { + bytes: item.document.source?.bytes, + }, + }, + } as ContentBlock; + } + return { text: "" } as ContentBlock;
🧹 Nitpick comments (2)
src/lib/providers/mistral.ts (2)
117-136: Consider extracting multimodal options builder to reduce duplication.The multimodalOptions construction is duplicated across providers (mistral.ts, litellm.ts). This creates a maintenance burden when new fields are added.
Consider creating a shared utility function:
// In src/lib/utils/messageBuilder.ts export function buildMultimodalOptionsFromStream( options: StreamOptions, providerName: string, modelName: string ) { return { input: { text: options.input?.text || "", images: options.input?.images, content: options.input?.content, files: options.input?.files, csvFiles: options.input?.csvFiles, pdfFiles: options.input?.pdfFiles, }, csvOptions: options.csvOptions, systemPrompt: options.systemPrompt, conversationHistory: options.conversationMessages, provider: providerName, model: modelName, temperature: options.temperature, maxTokens: options.maxTokens, enableAnalytics: options.enableAnalytics, enableEvaluation: options.enableEvaluation, context: options.context, }; }Then use it in providers:
- const multimodalOptions = { - input: { - text: options.input?.text || "", - images: options.input?.images, - content: options.input?.content, - files: options.input?.files, - csvFiles: options.input?.csvFiles, - pdfFiles: options.input?.pdfFiles, - }, - csvOptions: options.csvOptions, - systemPrompt: options.systemPrompt, - conversationHistory: options.conversationMessages, - provider: this.providerName, - model: this.modelName, - temperature: options.temperature, - maxTokens: options.maxTokens, - enableAnalytics: options.enableAnalytics, - enableEvaluation: options.enableEvaluation, - context: options.context, - }; + const multimodalOptions = buildMultimodalOptionsFromStream( + options, + this.providerName, + this.modelName + );
100-114: Consider reducing log verbosity for production.The multimodal input logging includes 12 fields per request, which may be excessive for production environments and could impact performance at scale.
Consider condensing to essential fields or making verbose logging conditional:
logger.debug( `Mistral: Detected multimodal input, using multimodal message builder`, { - hasImages: !!options.input?.images?.length, imageCount: options.input?.images?.length || 0, - hasContent: !!options.input?.content?.length, contentCount: options.input?.content?.length || 0, - hasFiles: !!options.input?.files?.length, fileCount: options.input?.files?.length || 0, - hasCSVFiles: !!options.input?.csvFiles?.length, csvFileCount: options.input?.csvFiles?.length || 0, - hasPDFFiles: !!options.input?.pdfFiles?.length, pdfFileCount: options.input?.pdfFiles?.length || 0, }, );This removes redundant
has*boolean flags (already implied by counts > 0).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
examples/data/invoice.pdfis excluded by!**/*.pdfexamples/data/report.pdfis excluded by!**/*.pdftest/fixtures/invalid.pdfis excluded by!**/*.pdftest/fixtures/multi-page.pdfis excluded by!**/*.pdftest/fixtures/valid-sample.pdfis excluded by!**/*.pdf
📒 Files selected for processing (33)
README.md(3 hunks)TEST_RESULTS.md(1 hunks)docs/features/index.md(2 hunks)docs/features/multimodal-chat.md(1 hunks)docs/features/pdf-support.md(1 hunks)docs/index.md(3 hunks)examples/data/README.md(3 hunks)examples/pdf-analysis.ts(1 hunks)src/cli/factories/commandFactory.ts(5 hunks)src/lib/adapters/providerImageAdapter.ts(3 hunks)src/lib/agent/directTools.ts(3 hunks)src/lib/core/baseProvider.ts(10 hunks)src/lib/neurolink.ts(3 hunks)src/lib/providers/amazonBedrock.ts(14 hunks)src/lib/providers/anthropic.ts(3 hunks)src/lib/providers/azureOpenai.ts(2 hunks)src/lib/providers/googleAiStudio.ts(2 hunks)src/lib/providers/googleVertex.ts(5 hunks)src/lib/providers/huggingFace.ts(2 hunks)src/lib/providers/litellm.ts(2 hunks)src/lib/providers/mistral.ts(2 hunks)src/lib/providers/ollama.ts(6 hunks)src/lib/providers/openAI.ts(2 hunks)src/lib/providers/openaiCompatible.ts(2 hunks)src/lib/types/content.ts(1 hunks)src/lib/types/fileTypes.ts(3 hunks)src/lib/types/generateTypes.ts(1 hunks)src/lib/types/providers.ts(2 hunks)src/lib/types/streamTypes.ts(1 hunks)src/lib/utils/fileDetector.ts(4 hunks)src/lib/utils/messageBuilder.ts(9 hunks)src/lib/utils/pdfProcessor.ts(1 hunks)test/continuous-test-suite.ts(40 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-01T22:58:39.149Z
Learnt from: sudharsan-juspay
PR: juspay/neurolink#140
File: src/lib/core/types.ts:198-203
Timestamp: 2025-09-01T22:58:39.149Z
Learning: In src/lib/core/types.ts, StreamOptions (imported from streamTypes.js) and StreamingOptions are intentionally different types with different use cases. StreamingOptions is for unified AI requests with multiple provider configurations, while StreamOptions is for individual streaming operations.
Applied to files:
src/lib/types/streamTypes.ts
📚 Learning: 2025-09-17T17:55:15.261Z
Learnt from: RajuSudhar
PR: juspay/neurolink#173
File: src/lib/index.ts:16-16
Timestamp: 2025-09-17T17:55:15.261Z
Learning: In src/lib/types/providers.ts, ProviderConfig was renamed to AIModelProviderConfig to deduplicate type names, as there was an existing ProviderConfig type that better suited the "ProviderConfig" name. This was an intentional breaking change for better type organization.
Applied to files:
src/lib/adapters/providerImageAdapter.ts
🧬 Code graph analysis (13)
src/lib/providers/litellm.ts (2)
src/lib/utils/messageBuilder.ts (3)
buildMultimodalMessagesArray(434-693)convertToCoreMessages(134-231)buildMessagesArray(288-428)src/lib/core/constants.ts (1)
DEFAULT_MAX_STEPS(10-10)
src/lib/providers/openaiCompatible.ts (2)
src/lib/utils/messageBuilder.ts (3)
buildMultimodalMessagesArray(434-693)convertToCoreMessages(134-231)buildMessagesArray(288-428)src/lib/core/constants.ts (1)
DEFAULT_MAX_STEPS(10-10)
examples/pdf-analysis.ts (2)
src/lib/neurolink.ts (1)
neurolink(5927-5927)src/lib/utils/pdfProcessor.ts (1)
process(114-184)
test/continuous-test-suite.ts (3)
neurolink-demo/test-providers.js (2)
TEST_CONFIG(37-56)args(8-8)src/lib/neurolink.ts (1)
NeuroLink(184-5924)src/lib/utils/pdfProcessor.ts (1)
process(114-184)
src/lib/providers/ollama.ts (7)
src/lib/types/providers.ts (3)
OllamaMessage(761-768)OllamaToolCall(741-748)OllamaToolResult(753-756)src/lib/types/streamTypes.ts (2)
StreamOptions(143-220)StreamResult(226-267)src/lib/types/tools.ts (1)
ToolArgs(33-38)src/lib/core/constants.ts (1)
DEFAULT_MAX_STEPS(10-10)src/lib/utils/messageBuilder.ts (1)
buildMultimodalMessagesArray(434-693)src/lib/core/analytics.ts (1)
createAnalytics(16-64)src/lib/types/common.ts (1)
JsonValue(23-29)
src/lib/neurolink.ts (1)
src/lib/utils/logger.ts (2)
logger(341-380)error(223-225)
src/lib/providers/huggingFace.ts (2)
src/lib/utils/messageBuilder.ts (3)
buildMultimodalMessagesArray(434-693)convertToCoreMessages(134-231)buildMessagesArray(288-428)src/lib/core/constants.ts (1)
DEFAULT_MAX_STEPS(10-10)
src/lib/providers/amazonBedrock.ts (5)
src/lib/types/providers.ts (1)
BedrockMessage(621-624)src/lib/utils/messageBuilder.ts (1)
buildMultimodalMessagesArray(434-693)src/lib/core/constants.ts (1)
DEFAULT_MAX_STEPS(10-10)src/lib/core/analytics.ts (1)
createAnalytics(16-64)src/lib/types/streamTypes.ts (1)
StreamOptions(143-220)
src/lib/utils/pdfProcessor.ts (2)
src/lib/types/fileTypes.ts (2)
PDFProviderConfig(77-83)FileProcessingResult(38-58)src/lib/utils/logger.ts (1)
logger(341-380)
src/lib/providers/mistral.ts (2)
src/lib/utils/logger.ts (1)
logger(341-380)src/lib/utils/messageBuilder.ts (2)
convertToCoreMessages(134-231)buildMessagesArray(288-428)
src/lib/core/baseProvider.ts (2)
src/lib/types/content.ts (2)
TextContent(9-12)ImageContent(17-33)src/lib/utils/schemaConversion.ts (1)
convertJsonSchemaToZod(74-174)
src/lib/utils/fileDetector.ts (4)
src/lib/types/fileTypes.ts (1)
FileProcessingResult(38-58)src/lib/utils/csvProcessor.ts (1)
CSVProcessor(68-369)src/lib/utils/imageProcessor.ts (1)
ImageProcessor(13-411)src/lib/utils/pdfProcessor.ts (1)
PDFProcessor(109-229)
src/lib/utils/messageBuilder.ts (2)
src/lib/utils/fileDetector.ts (3)
result(340-352)result(567-579)FileDetector(56-271)src/lib/utils/logger.ts (2)
logger(341-380)error(223-225)
🔇 Additional comments (38)
src/lib/agent/directTools.ts (3)
114-115: LGTM!The default value of
falseforincludeHiddenis sensible and aligns well with typical user expectations (hidden files should be opt-in). The updated description is also clearer.
439-442: LGTM!The default empty string is handled correctly by the execute function, which validates column presence for operations that require it (e.g., lines 499-504).
446-447: LGTM!The default value of
1000aligns with the description and matches the default parameter value in the execute function (line 449). This change improves consistency between schema and implementation.src/lib/providers/openAI.ts (2)
349-355: LGTM! Consistent multimodal input handling.The addition of
pdfFilesto the multimodal input detection follows the same pattern ascsvFiles, ensuring consistent handling across all multimodal content types in the OpenAI provider.
374-382: LGTM! PDF options properly propagated.The
pdfFilesfield is correctly included in themultimodalOptionsobject, ensuring PDF content flows through the multimodal message builder alongside other content types.src/lib/providers/googleAiStudio.ts (1)
157-163: LGTM! PDF support integrated consistently.The multimodal input detection now includes
pdfFiles, maintaining consistency with the OpenAI and Anthropic providers. The implementation correctly extends the existing pattern for handling mixed content types.src/lib/providers/anthropic.ts (1)
166-189: LGTM! Enhanced diagnostics for PDF processing.The Anthropic provider includes comprehensive debug logging for PDF inputs (
hasPDFFiles,pdfFileCount), which is excellent for troubleshooting multimodal workflows. This follows the same pattern established for CSV files.docs/features/index.md (2)
32-32: LGTM! PDF feature properly indexed.The PDF File Support feature is correctly added to the Core Features table with appropriate description and documentation link.
43-50: LGTM! Platform capabilities accurately reflect PDF support.The updated table correctly shows PDF document support in the Multimodal pipeline row, with proper cross-references to all three related documentation pages (multimodal, CSV, PDF).
src/lib/utils/fileDetector.ts (3)
90-96: LGTM! Provider parameter properly threaded.The
detectAndProcessmethod correctly passes theprovideroption through toprocessFile, enabling provider-specific PDF processing logic (e.g., validation of file size and page limits per provider).
176-189: LGTM! PDF processing integrated cleanly.The PDF case follows the same pattern as CSV and image processing, delegating to
PDFProcessor.process()with the provider option. This maintains consistency across all file type handlers.
471-471: Clarify confidence increase justification.The ExtensionStrategy confidence increased from 70 to 85 (15-point increase). While file extensions are generally reliable indicators, this change affects the detection strategy priority for all file types.
Please clarify the reasoning for this increase. If it's based on empirical testing or to prioritize extension detection over content heuristics (75% confidence), please document this in the code comments or commit message.
Based on learnings, this could be related to improving detection accuracy, but the justification should be explicit.
examples/data/README.md (2)
42-76: LGTM! Clear PDF sample documentation.The documentation for
invoice.pdfandreport.pdfprovides clear descriptions of contents, use cases, and expected values. This makes it easy for users to understand what to expect when testing PDF features.
93-142: LGTM! Comprehensive multimodal examples.The usage examples cover a wide range of scenarios:
- Single PDF analysis
- Multi-page PDF processing
- PDF comparison
- Multimodal (CSV + PDF) analysis
All examples correctly specify the required provider (vertex or anthropic), which aligns with the provider compatibility documented elsewhere in the PR.
docs/features/multimodal-chat.md (1)
196-279: Approve PDF File Support section
The new PDF File Support section is thorough—clear quick-start examples, SDK usage, provider compatibility, best practices, and token guidance—and appears only once in the document.src/lib/providers/mistral.ts (1)
89-96: LGTM on multimodal detection logic.The hasMultimodalInput check correctly covers all multimodal input types (images, content, files, csvFiles, pdfFiles). This pattern is consistent with the litellm.ts changes.
README.md (3)
29-29: LGTM on PDF support documentation.The What's New section clearly highlights PDF support with a link to comprehensive documentation. The positioning in Q4 2025 features is appropriate.
265-281: Clear SDK usage example for PDF support.The example effectively demonstrates:
- Auto-detection via unified
filesarray- Explicit
pdfFilesfor mixed file types- Provider requirement (vertex) for PDF support
- Integration with existing CSV and image handling
287-295: Platform capabilities table accurately reflects PDF support.The updated table correctly notes that PDFs are handled in the multimodal pipeline with auto-detection for mixed file types, aligning with the implementation.
src/lib/types/fileTypes.ts (3)
69-83: Well-designed PDF provider configuration types.The PDFProviderConfig interface captures essential provider constraints:
- Size and page limits for validation
- Native support detection
- Citation requirements (with "auto" option for conditional behavior)
- API type discrimination (document vs files-api vs unsupported)
These types enable robust provider-specific PDF handling.
52-57: PDF metadata fields are comprehensive.The metadata additions cover:
- PDF version for format validation
- Estimated page count for limit checking
- Provider and API type for routing decisions
These fields align well with the validation and processing requirements described in the PDF support documentation.
107-122: Google Files API types match official API structure.The GoogleFilesAPIUploadResult interface correctly mirrors Google's Files API response format, including all required fields (name, mimeType, sizeBytes, timestamps, hashes, uri).
docs/features/pdf-support.md (5)
1-24: Excellent documentation structure and overview.The PDF support documentation provides:
- Clear overview of native multimodal processing
- 5-step workflow explanation
- Key distinction from CSV processing (binary vs text)
- Practical examples in Quick Start section
This gives users a strong foundation for understanding PDF support.
144-157: Provider support matrix is clear and accurate.The table effectively communicates:
- Size and page limits per provider
- API type used (Document vs Files API)
- Recommended use cases
- Google AI Studio's unique 2GB limit
This helps users make informed provider selections.
159-178: Excellent error messaging for unsupported providers.The documentation includes the exact error message users will see, along with clear resolution options. This reduces support burden and improves user experience.
340-409: Comprehensive best practices section.The best practices cover:
- Provider selection based on file size and use case
- File size validation code examples
- Error handling patterns with specific error types
- Streaming recommendations for large documents
- Prompt specificity guidance
This section provides actionable guidance for production use.
501-598: Thorough troubleshooting guide.Each common error includes:
- Problem description
- Specific solutions (with code examples)
- Debug techniques for verification
The "PDF Content Not Being Analyzed" section is particularly helpful, walking users through verification steps with actual code.
src/lib/types/content.ts (2)
49-61: PDFContent type is well-designed and consistent.The PDFContent type:
- Follows the established pattern (type discriminator, data field, optional metadata)
- Includes appropriate PDF-specific metadata (pages, version)
- Uses Buffer | string for data flexibility (like ImageContent)
- Supports optional description field for context
66-66: Content union correctly extended for PDF support.Adding PDFContent to the Content union enables PDFs as a first-class multimodal content type throughout the codebase, maintaining type safety across the pipeline.
TEST_RESULTS.md (3)
1-62: Excellent test documentation with clear results.The test results document:
- Overall pass rate and duration (100%, 242s)
- Breakdown by category (CSV: 6, PDF: 6, Core: 4, etc.)
- Provider used (Vertex AI)
- Clear status indicators (✅)
This provides confidence in the PDF support implementation.
65-111: HITL test fix explanation is valuable.The before/after comparison clearly demonstrates:
- Problem: Non-deterministic AI behavior made tests flaky
- Solution: Direct tool execution ensures 100% reliability
- Benefits: Faster, provider-agnostic, tests the real feature
This explanation helps reviewers understand the architectural improvement and could be useful for other developers facing similar testing challenges.
113-125: Test coverage table shows comprehensive validation.The breakdown shows:
- 6 PDF tests covering CLI/SDK, streaming, and mixed modalities
- 6 CSV tests for comparison
- HITL, business tools, and enterprise features tested
This demonstrates thorough validation of the multimodal PDF functionality.
src/lib/providers/litellm.ts (2)
182-244: Multimodal implementation matches mistral.ts pattern.The LiteLLM provider correctly implements the same multimodal detection and branching logic as Mistral, ensuring consistency across providers. The hasMultimodalInput check is comprehensive (images, content, files, csvFiles, pdfFiles).
Note: Same refactor opportunities apply here as in mistral.ts:
- Extract multimodalOptions builder to shared utility (Lines 210-229 duplicate construction)
- Reduce log verbosity (Lines 193-207 have 12 log fields)
See mistral.ts review comments for detailed suggestions.
253-253: Good addition of maxSteps with default.Adding
maxSteps: options.maxSteps || DEFAULT_MAX_STEPSensures agentic workflows have a safety limit while allowing user override. This aligns with other providers.examples/pdf-analysis.ts (4)
1-21: Clear example setup with helpful prerequisites.The file header includes:
- Execution instructions (
npx tsx examples/pdf-analysis.ts)- Prerequisites checklist (credentials, PDF files)
- Workaround for tool schema errors (NEUROLINK_DISABLE_TOOLS)
This reduces friction for users trying the examples.
27-47: Basic example demonstrates core PDF functionality.Example 1 shows:
- Simple pdfFiles array usage
- Specific prompt for information extraction
- Appropriate maxTokens setting (500)
- Error handling with console output
This provides a clear starting point for users.
168-196: Error handling example is instructive.Example 6 intentionally triggers an error with an unsupported provider (openai instead of vertex), then demonstrates:
- Error message inspection
- User-friendly error messaging
- Guidance to use supported providers
This teaches users proper error handling patterns.
34-34: All referenced example files exist in examples/data/.
49b5990 to
dffc96f
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 |
dffc96f to
fd66942
Compare
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 35 out of 40 changed files in this pull request and generated 6 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
fd66942 to
539d5a6
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 |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 35 out of 40 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
test/continuous-test-suite.ts:137
- Removing
as constfromTEST_CONFIGallows the object to be mutated, which could lead to unexpected behavior in tests. Since the configuration is modified later via CLI arguments, consider using a readonly type for the base configuration and creating a mutable copy for runtime modifications.
} as const;
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| ); | ||
| const fallbackStreamResult = await provider.stream({ |
There was a problem hiding this comment.
The removed boolean parameter from createProvider() call is part of a pattern seen throughout the file. Ensure that all calls to AIProviderFactory.createProvider() have been updated consistently to match the new method signature.
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/providers/litellm.ts (1)
233-246: AwaitstreamTextto avoid runtime failureHere, too,
streamText()is asynchronous. Using the unresolved promise immediately (fortextStream, analytics, etc.) will throw. Add anawaitlike in the other providers.- const result = streamText({ + const result = await streamText({
♻️ Duplicate comments (3)
vite.config.ts (1)
2-2: The import change is correct; disregard the Copilot comment.The change to import
defineConfigfrom"vitest/config"instead of"vite"is the recommended pattern when your Vite config includes Vitest test configuration. This import ensures proper TypeScript type inference for thetestproperty (lines 8-43). The Copilot comment suggesting to revert is incorrect.test/continuous-test-suite.ts (1)
57-74: Consider restoringas constassertion.A past review comment noted that removing the
as constassertion reduces type safety and prevents accidental mutations. While the configuration is now more flexible with optional model override, you could still benefit from the immutability guarantees.Apply this diff to restore type safety while maintaining flexibility:
expectedFileData: { "package.json": [ packageData.version || "unknown", packageData.main || "dist/index.js", ], "README.md": ["NeuroLink", "MCP", "SDK"], "tsconfig.json": ["ES2022", "CommonJS", "strict"], ".mcp-config.json": ["filesystem", "github", "stdio"], }, - }; + } as const;Note: This assumes
modelandmaxTokenscan remain mutable for runtime configuration.src/lib/providers/ollama.ts (1)
488-499: PDF content is still being silently dropped.This is the same issue flagged in previous reviews. After PDFs are detected and queued through
buildMultimodalOptions→buildMultimodalMessagesArray→ file-type content items, they reach this conversion method and are silently discarded with only a warning log. This breaks the advertised PDF support for Ollama.Either:
- Forward PDFs to Ollama if the model supports them, or
- Throw an explicit error early (in
buildMultimodalMessagesArrayor provider validation) so users know Ollama doesn't support PDFs yet.The current behavior—accepting PDFs through the SDK/CLI, processing them, then silently dropping them here—constitutes data loss and violates user expectations.
🧹 Nitpick comments (7)
src/lib/neurolink.ts (1)
5818-5844: Ensure external MCP tools are unregistered during dispose().
dispose()callsexternalServerManager.shutdown()directly, but that path skips the existingshutdownExternalMCPServers()helper, so the tool registry keeps stale references to external MCP tools. After disposal,getAllAvailableTools()still returns these entries even though the backing servers are gone. Please invokeawait this.shutdownExternalMCPServers()(or at least callthis.unregisterAllExternalMCPToolsFromRegistry()before shutting the manager down) so the registry stays consistent once resources are torn down.- if (this.externalServerManager) { - try { - logger.debug("[NeuroLink] Shutting down external MCP servers..."); - await this.externalServerManager.shutdown(); + if (this.externalServerManager) { + try { + logger.debug("[NeuroLink] Shutting down external MCP servers..."); + await this.shutdownExternalMCPServers();test/continuous-test-suite.ts (1)
2986-3006: Consider making inter-test delay configurable.The 60-second delay for OpenAI rate limits (30,000 TPM) is hardcoded and may be unnecessarily long for other scenarios or future rate limit changes. Consider making this configurable.
Option 1: Add to TEST_CONFIG:
const TEST_CONFIG = { provider: "vertex", model: undefined as string | undefined, maxTokens: undefined as number | undefined, timeout: 60000, + interTestDelay: { + openai: 60000, // OpenAI: 30,000 TPM limit + default: 5000, // Others: resource cleanup + },Option 2: Calculate dynamically based on provider limits:
const getInterTestDelay = (provider: string): number => { const rateLimitResetTime = { openai: 60000, // 30,000 TPM per minute anthropic: 10000, // Higher limits default: 5000, }; return rateLimitResetTime[provider as keyof typeof rateLimitResetTime] ?? rateLimitResetTime.default; };src/lib/providers/googleVertex.ts (1)
843-845: Consider using buildMultimodalOptions for consistency.While the multimodal PDF integration is correct, this provider constructs the
multimodalOptionsobject inline (lines 862-881), whereas other providers (OpenAI, GoogleAIStudio, HuggingFace) use thebuildMultimodalOptionsutility.For maintainability and consistency, consider refactoring to use the centralized builder:
- const multimodalOptions = { - input: { - text: options.input?.text || "", - images: options.input?.images, - content: options.input?.content, - files: options.input?.files, - csvFiles: options.input?.csvFiles, - pdfFiles: options.input?.pdfFiles, - }, - csvOptions: options.csvOptions, - systemPrompt: options.systemPrompt, - conversationHistory: options.conversationMessages, - provider: this.providerName, - model: this.modelName, - temperature: options.temperature, - maxTokens: options.maxTokens, - enableAnalytics: options.enableAnalytics, - enableEvaluation: options.enableEvaluation, - context: options.context, - }; + const multimodalOptions = buildMultimodalOptions( + options, + this.providerName, + this.modelName, + );You'll need to add the import:
+import { buildMultimodalOptions } from "../utils/multimodalOptionsBuilder.js";Also applies to: 856-858, 869-869
src/lib/providers/ollama.ts (1)
1229-1242: Silent argument dropping in tool execution.The type conversion loop filters out non-JsonValue arguments without logging which fields were dropped. If a tool receives unexpected argument types (e.g., functions, symbols, undefined), they're silently removed, potentially causing tool execution to fail with cryptic "missing required argument" errors.
Apply this diff to add logging for dropped arguments:
const toolArgs: ToolArgs = {}; + const droppedKeys: string[] = []; for (const [key, value] of Object.entries(toolInput)) { // Only include values that are JsonValue compatible if ( value === null || typeof value === "string" || typeof value === "number" || typeof value === "boolean" || (typeof value === "object" && value !== null) ) { toolArgs[key] = value as JsonValue; + } else { + droppedKeys.push(key); } } + if (droppedKeys.length > 0) { + logger.warn(`[OllamaProvider] Dropped non-JsonValue tool arguments for ${toolName}:`, droppedKeys); + }src/lib/utils/messageBuilder.ts (2)
448-451: Validate provider PDF support before processing.The code calculates
maxSizefrompdfConfigbut doesn't verify whether the provider actually supports PDFs. A provider could have a config entry but still not support PDF processing (or support could be model-specific like Ollama).Additionally, the fallback logic
pdfConfig ? pdfConfig.maxSizeMB * 1024 * 1024 : 10 * 1024 * 1024doesn't handle the case wherepdfConfigexists butmaxSizeMBis undefined or zero.Apply this diff to add validation and safer defaults:
const pdfConfig = PDFProcessor.getProviderConfig(provider); - const maxSize = pdfConfig - ? pdfConfig.maxSizeMB * 1024 * 1024 - : 10 * 1024 * 1024; + if (!pdfConfig) { + logger.debug(`[FileDetector] Provider ${provider} does not support PDFs, skipping PDF detection`); + } + const maxSize = pdfConfig?.maxSizeMB + ? pdfConfig.maxSizeMB * 1024 * 1024 + : 10 * 1024 * 1024;
486-491: PDF files queued without provider validation.When a PDF is auto-detected from the
filesarray, it's immediately queued tooptions.input.pdfFileswithout checking:
- Whether the provider supports PDF processing
- Whether the PDF exceeds provider-specific page or size limits
This defers validation until later in the pipeline, leading to confusing errors or silent failures (as seen with Ollama). Consider adding early validation here using
PDFProcessor.validateProviderSupport()or similar.Apply this diff to add early validation:
} else if (result.type === "pdf") { + // Validate provider supports PDFs before queuing + const pdfConfig = PDFProcessor.getProviderConfig(provider); + if (!pdfConfig) { + throw new Error( + `Provider ${provider} does not support PDF processing. ` + + `Supported providers: ${PDFProcessor.getSupportedProviders().join(", ")}` + ); + } options.input.pdfFiles = [ ...(options.input.pdfFiles || []), result.content, ]; logger.info(`[FileDetector] ✅ PDF: ${extractFilename(file)}`); }src/lib/adapters/providerImageAdapter.ts (1)
346-348: Consider more precise model matching to avoid false positives.The current validation uses substring matching (
includes()), which could match unintended models. For example, "my-custom-gpt-4o-model" would match "gpt-4o" even if it's not a vision-capable model.If this flexibility is intentional (to support fine-tuned or custom variants), consider documenting this behavior. Otherwise, you might want more precise matching:
const isSupported = supportedModels.some((supportedModel) => - model.toLowerCase().includes(supportedModel.toLowerCase()), + model.toLowerCase() === supportedModel.toLowerCase() || + model.toLowerCase().startsWith(supportedModel.toLowerCase() + "-") || + model.toLowerCase().startsWith(supportedModel.toLowerCase() + ":"), );This alternative matches exact names, hyphenated variants (e.g., "gpt-4o-mini"), and colon variants (e.g., "llama4:latest") while avoiding false positives from arbitrary substrings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
examples/data/invoice.pdfis excluded by!**/*.pdfexamples/data/report.pdfis excluded by!**/*.pdftest/fixtures/invalid.pdfis excluded by!**/*.pdftest/fixtures/multi-page.pdfis excluded by!**/*.pdftest/fixtures/valid-sample.pdfis excluded by!**/*.pdf
📒 Files selected for processing (35)
README.md(3 hunks)TEST_RESULTS.md(1 hunks)docs/features/index.md(2 hunks)docs/features/multimodal-chat.md(1 hunks)docs/features/pdf-support.md(1 hunks)docs/index.md(3 hunks)examples/data/README.md(3 hunks)examples/pdf-analysis.ts(1 hunks)src/cli/factories/commandFactory.ts(5 hunks)src/lib/adapters/providerImageAdapter.ts(3 hunks)src/lib/agent/directTools.ts(3 hunks)src/lib/core/baseProvider.ts(10 hunks)src/lib/neurolink.ts(3 hunks)src/lib/providers/amazonBedrock.ts(15 hunks)src/lib/providers/anthropic.ts(3 hunks)src/lib/providers/azureOpenai.ts(3 hunks)src/lib/providers/googleAiStudio.ts(3 hunks)src/lib/providers/googleVertex.ts(5 hunks)src/lib/providers/huggingFace.ts(2 hunks)src/lib/providers/litellm.ts(2 hunks)src/lib/providers/mistral.ts(2 hunks)src/lib/providers/ollama.ts(6 hunks)src/lib/providers/openAI.ts(3 hunks)src/lib/providers/openaiCompatible.ts(2 hunks)src/lib/types/content.ts(1 hunks)src/lib/types/fileTypes.ts(3 hunks)src/lib/types/generateTypes.ts(1 hunks)src/lib/types/providers.ts(2 hunks)src/lib/types/streamTypes.ts(1 hunks)src/lib/utils/fileDetector.ts(4 hunks)src/lib/utils/messageBuilder.ts(11 hunks)src/lib/utils/multimodalOptionsBuilder.ts(1 hunks)src/lib/utils/pdfProcessor.ts(1 hunks)test/continuous-test-suite.ts(40 hunks)vite.config.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-17T17:55:15.261Z
Learnt from: RajuSudhar
PR: juspay/neurolink#173
File: src/lib/index.ts:16-16
Timestamp: 2025-09-17T17:55:15.261Z
Learning: In src/lib/types/providers.ts, ProviderConfig was renamed to AIModelProviderConfig to deduplicate type names, as there was an existing ProviderConfig type that better suited the "ProviderConfig" name. This was an intentional breaking change for better type organization.
Applied to files:
src/lib/providers/azureOpenai.tssrc/lib/providers/openAI.ts
🔇 Additional comments (23)
vite.config.ts (2)
10-10: LGTM!The addition of the
excludepattern fornode_modulesis a standard best practice that prevents Vitest from scanning dependency directories for test files.
44-44: LGTM!The removal of the
as const satisfies UserConfigtype assertion simplifies the code without any functional impact. ThedefineConfigfunction from"vitest/config"already provides proper typing.test/continuous-test-suite.ts (2)
46-54: LGTM! Provider-specific token limits.The introduction of
PROVIDER_MAX_TOKENSproperly addresses the previous issue where a global limit of 8192 was applied to all providers. Now Anthropic and Bedrock use 8192, while Vertex, AI Studio, and OpenAI can use higher limits appropriate for their models.
2410-2840: LGTM! PDF test coverage is comprehensive.The 8 new PDF test functions (
testCLIGeneratePDF,testCLIStreamPDF,testSDKGeneratePDF,testSDKStreamPDF,testCLIStreamTwoPDFComparison,testCLIStreamPDFAndCSV) mirror the existing CSV test patterns and provide thorough coverage of CLI, SDK, single-file, multi-file, and mixed-modality scenarios.The test fixtures (
test/fixtures/valid-sample.pdf,test/fixtures/multi-page.pdf) are referenced and validation logic checks for expected content (revenue values, quarters). This aligns well with the PR's goal of comprehensive PDF support.src/lib/types/generateTypes.ts (1)
24-24: LGTM! PDF support added to GenerateOptions.The addition of
pdfFiles?: Array<Buffer | string>mirrors the existingcsvFilespattern and maintains consistency with the multimodal input design. This enables explicit PDF file inputs alongside the auto-detectfilesarray.docs/features/index.md (1)
32-32: LGTM! PDF feature properly documented.The PDF File Support entry has been added to the Core Features table (line 32) and integrated into the Platform Capabilities multimodal pipeline description (line 45) with appropriate links to
pdf-support.md. This ensures the feature is discoverable and well-documented.Also applies to: 45-45
src/lib/providers/azureOpenai.ts (1)
151-171: LGTM! Consistent multimodal refactoring.The changes align Azure OpenAI with the centralized multimodal options construction pattern:
- Line 151-152: PDF detection added to
hasMultimodalInputcheck alongside CSV files- Lines 167-171: Replaced inline
multimodalOptionsobject withbuildMultimodalOptions()callThis refactoring improves maintainability by centralizing the construction logic in
src/lib/utils/multimodalOptionsBuilder.ts, ensuring consistent behavior across all providers (OpenAI, Anthropic, Vertex, Azure, etc.).src/lib/types/streamTypes.ts (1)
149-149: LGTM! PDF support added to StreamOptions.The addition of
pdfFiles?: Array<Buffer | string>toStreamOptions.inputmirrors the change inGenerateOptions(src/lib/types/generateTypes.ts:24) and maintains consistency across the streaming and generation APIs.src/lib/providers/anthropic.ts (1)
171-197: LGTM! Anthropic provider aligned with multimodal refactoring.The changes follow the same centralized pattern applied to other providers:
- Lines 171-172: PDF detection added to multimodal input check
- Lines 188-189: Diagnostic logging expanded to include
hasPDFFilesandpdfFileCount- Lines 193-197: Replaced inline
multimodalOptionsconstruction withbuildMultimodalOptions()callThis ensures consistent multimodal handling across OpenAI, Azure OpenAI, Anthropic, Vertex, and other providers, improving maintainability and reducing code duplication.
src/lib/providers/googleAiStudio.ts (1)
162-164: LGTM! Consistent multimodal PDF support integration.The integration of PDF file support follows the established pattern for other modalities (CSV, images). The use of the centralized
buildMultimodalOptionsutility promotes consistency across providers.Also applies to: 178-182
src/lib/providers/openAI.ts (1)
354-356: LGTM! Multimodal PDF support implemented consistently.The implementation mirrors the pattern used in other providers, maintaining consistency across the codebase.
Also applies to: 374-378
examples/data/README.md (1)
1-143: Excellent documentation of PDF support!The documentation clearly explains:
- PDF file contents and structure
- Use cases for single vs. multi-page PDFs
- CLI usage examples
- Multimodal CSV+PDF scenarios
This will help users understand the new PDF capabilities.
src/lib/providers/googleVertex.ts (1)
1627-1628: LGTM! Claude model validation patterns updated appropriately.The addition of
claude-opus-4-1@andclaude-3-7-sonnet@patterns correctly extends support for newer Anthropic model versions while maintaining the date-based version format validation.src/lib/types/content.ts (1)
49-61: LGTM! PDFContent type properly integrated.The
PDFContenttype definition:
- Follows the same structure as
CSVContentandImageContent- Includes appropriate metadata fields (filename, pages, version, description)
- Correctly added to the
Contentunion typeThis enables type-safe PDF handling across the multimodal pipeline.
Also applies to: 66-66
src/lib/providers/huggingFace.ts (1)
172-217: LGTM! Comprehensive multimodal support with proper logging.The implementation:
- Correctly detects all multimodal input types including PDFs
- Uses the centralized
buildMultimodalOptionsutility for consistency- Includes detailed logging for debugging multimodal workflows
- Adds
maxStepsparameter for better streaming controlWell-integrated with the existing HuggingFace provider architecture.
Also applies to: 224-224
src/lib/utils/fileDetector.ts (2)
471-471: Verify extension confidence bump applies only to PDFs
ExtensionStrategy now returnsconfidence=85(>80% threshold) unconditionally at src/lib/utils/fileDetector.ts:471, causing detection to stop early for all file types. Was this intended only for PDFs (e.g.ext==='pdf')? Otherwise revert to 70% for non-PDFs and update the docs.
21-21: PDF provider validations are in place
PDFProcessor.process enforces provider-specific checks (file signature, maxSizeMB, maxPages, supportsNative, bedrock mode); no changes needed.src/lib/utils/multimodalOptionsBuilder.ts (1)
10-10: Empty string fallback safe Downstream consumers like buildMultimodalMessagesArray use truthiness checks oninput.text, so""andundefinedare treated identically.src/lib/utils/messageBuilder.ts (1)
941-994: LGTM with integration note.The implementation correctly formats PDFs as
{ type: "file", data: Buffer, mimeType: "application/pdf" }for the Vercel AI SDK standard. However, note that individual providers must handle this format in their message conversion logic.Currently,
OllamaProvider.convertToOllamaMessages()(lines 488-499 in ollama.ts) explicitly drops file-type items with only a warning, breaking the PDF support chain. When fixing the Ollama provider, ensure it either:
- Forwards these file items to the Ollama API if supported, or
- Throws an error early in the pipeline before reaching this point
Based on learnings about the Vercel AI SDK and multimodal support patterns.
src/lib/adapters/providerImageAdapter.ts (4)
205-207: Confirm and test Ollama image support
Ollama’s experimental OpenAI-compatibility layer partially supports multimodal messages with image_url (data URI or base64), but behavior varies by model/version. Add integration tests against your target Ollama model and implement error handling or fallbacks for unsupported cases.
87-112: Document @ timestamp suffix and confirm Claude 4.x GA
Add inline comments explaining that the trailing@in model IDs pins to a specific build date (e.g.,@YYYYMMDD), and note that Claude Opus 4 and Claude Sonnet 4 have been GA on Vertex AI since May 22, 2025.
58-58: Confirm existence and vision support for claude-3-7-sonnet
Anthropic’s docs listclaude-3-7-sonnet(API id:claude-3-7-sonnet-20250219/ alias:claude-3-7-sonnet-latest). Verify that this variant supports vision inputs insrc/lib/adapters/providerImageAdapter.ts:58.
27-49: OpenAI vision model list accurate — GPT-5, GPT-4.1, o3, and o4 variants exist, support vision/multimodal inputs, and the listed model names (e.g., “gpt-5”, “gpt-4.1-mini”, “o4-mini”) match OpenAI’s API specifications.
539d5a6 to
978f72e
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 |
978f72e to
f8ab247
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
🤖 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
Copilot reviewed 35 out of 40 changed files in this pull request and generated 7 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| @@ -1,12 +1,13 @@ | |||
| import { sveltekit } from "@sveltejs/kit/vite"; | |||
| import { defineConfig, type UserConfig } from "vite"; | |||
| import { defineConfig } from "vitest/config"; | |||
There was a problem hiding this comment.
The import has been changed from 'vite' to 'vitest/config', but the function is still being used for Vite configuration. Consider using the proper import path or ensure this change is intentional for test configuration compatibility.
| import { defineConfig } from "vitest/config"; | |
| import { defineConfig } from "vite"; |
| const PROVIDER_MAX_TOKENS: Record<string, number> = { | ||
| anthropic: 8192, // Claude 3.5 Sonnet output limit | ||
| vertex: 10000, // Gemini 1.5 Pro can handle more | ||
| "google-ai-studio": 10000, // Same as Vertex | ||
| openai: 16384, // GPT-4o can handle more | ||
| bedrock: 8192, // Conservative default for various models | ||
| ollama: 4096, // Local models typically lower | ||
| }; |
There was a problem hiding this comment.
The token limits are hardcoded and may become outdated as model capabilities change. Consider moving these to a configuration file or making them dynamically configurable.
| /* | ||
| * ======================================================================================== | ||
| * TODO: FIX HITL TESTS - CURRENT APPROACH IS NON-DETERMINISTIC | ||
| * ======================================================================================== |
There was a problem hiding this comment.
[nitpick] This extensive comment block (97 lines) documents important technical decisions and troubleshooting history, but it's quite verbose for inline code. Consider moving this detailed analysis to a separate documentation file or issue tracker.
|
|
||
| export class PDFProcessor { | ||
| // PDF magic bytes: %PDF- | ||
| private static readonly PDF_SIGNATURE = Buffer.from("%PDF-", "ascii"); |
There was a problem hiding this comment.
The PDF signature validation is good, but consider adding validation for common PDF corruption patterns or encrypted PDFs that might cause processing issues downstream.
| function convertContentItem( | ||
| item: unknown, | ||
| ): | ||
| | TextPart | ||
| | ImagePart | ||
| | { type: "file"; data: Buffer; mimeType: string } | ||
| | null { |
There was a problem hiding this comment.
The return type includes an inline object type for file parts. Consider defining a proper FilePart type interface for consistency with TextPart and ImagePart.
| logger.debug("🟦 [TRACE] streamingConversationLoop ENTRY"); | ||
| const maxIterations = 10; | ||
| const startTime = Date.now(); | ||
| const maxIterations = options.maxSteps || DEFAULT_MAX_STEPS; |
There was a problem hiding this comment.
The maxSteps logic is duplicated across multiple providers. Consider centralizing this logic in a base class or utility function to maintain consistency.
| // 1. Flush and shutdown OpenTelemetry | ||
| try { | ||
| logger.debug("[NeuroLink] Flushing and shutting down OpenTelemetry..."); | ||
| await flushOpenTelemetry(); | ||
| await shutdownOpenTelemetry(); | ||
| logger.debug("[NeuroLink] OpenTelemetry shutdown successfully"); | ||
| } catch (error) { | ||
| const err = | ||
| error instanceof Error | ||
| ? error | ||
| : new Error(`OpenTelemetry shutdown error: ${String(error)}`); | ||
| cleanupErrors.push(err); | ||
| logger.warn("[NeuroLink] Error shutting down OpenTelemetry:", error); | ||
| } | ||
|
|
||
| // 2. Shutdown external MCP server connections | ||
| if (this.externalServerManager) { | ||
| try { | ||
| logger.debug("[NeuroLink] Shutting down external MCP servers..."); | ||
| await this.externalServerManager.shutdown(); | ||
| logger.debug( | ||
| "[NeuroLink] External MCP servers shutdown successfully", | ||
| ); | ||
| } catch (error) { | ||
| const err = | ||
| error instanceof Error | ||
| ? error | ||
| : new Error(`External server shutdown error: ${String(error)}`); | ||
| cleanupErrors.push(err); | ||
| logger.warn( | ||
| "[NeuroLink] Error shutting down external MCP servers:", | ||
| error, | ||
| ); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
The dispose method is comprehensive but performs sequential cleanup operations. For better performance in scenarios with many resources, consider running independent cleanup operations in parallel using Promise.allSettled().
| // 1. Flush and shutdown OpenTelemetry | |
| try { | |
| logger.debug("[NeuroLink] Flushing and shutting down OpenTelemetry..."); | |
| await flushOpenTelemetry(); | |
| await shutdownOpenTelemetry(); | |
| logger.debug("[NeuroLink] OpenTelemetry shutdown successfully"); | |
| } catch (error) { | |
| const err = | |
| error instanceof Error | |
| ? error | |
| : new Error(`OpenTelemetry shutdown error: ${String(error)}`); | |
| cleanupErrors.push(err); | |
| logger.warn("[NeuroLink] Error shutting down OpenTelemetry:", error); | |
| } | |
| // 2. Shutdown external MCP server connections | |
| if (this.externalServerManager) { | |
| try { | |
| logger.debug("[NeuroLink] Shutting down external MCP servers..."); | |
| await this.externalServerManager.shutdown(); | |
| logger.debug( | |
| "[NeuroLink] External MCP servers shutdown successfully", | |
| ); | |
| } catch (error) { | |
| const err = | |
| error instanceof Error | |
| ? error | |
| : new Error(`External server shutdown error: ${String(error)}`); | |
| cleanupErrors.push(err); | |
| logger.warn( | |
| "[NeuroLink] Error shutting down external MCP servers:", | |
| error, | |
| ); | |
| } | |
| } | |
| // Run independent cleanup operations in parallel | |
| const cleanupTasks: Promise<void>[] = []; | |
| // 1. Flush and shutdown OpenTelemetry | |
| cleanupTasks.push( | |
| (async () => { | |
| try { | |
| logger.debug("[NeuroLink] Flushing and shutting down OpenTelemetry..."); | |
| await flushOpenTelemetry(); | |
| await shutdownOpenTelemetry(); | |
| logger.debug("[NeuroLink] OpenTelemetry shutdown successfully"); | |
| } catch (error) { | |
| const err = | |
| error instanceof Error | |
| ? error | |
| : new Error(`OpenTelemetry shutdown error: ${String(error)}`); | |
| cleanupErrors.push(err); | |
| logger.warn("[NeuroLink] Error shutting down OpenTelemetry:", error); | |
| } | |
| })() | |
| ); | |
| // 2. Shutdown external MCP server connections | |
| if (this.externalServerManager) { | |
| cleanupTasks.push( | |
| (async () => { | |
| try { | |
| logger.debug("[NeuroLink] Shutting down external MCP servers..."); | |
| await this.externalServerManager.shutdown(); | |
| logger.debug( | |
| "[NeuroLink] External MCP servers shutdown successfully", | |
| ); | |
| } catch (error) { | |
| const err = | |
| error instanceof Error | |
| ? error | |
| : new Error(`External server shutdown error: ${String(error)}`); | |
| cleanupErrors.push(err); | |
| logger.warn( | |
| "[NeuroLink] Error shutting down external MCP servers:", | |
| error, | |
| ); | |
| } | |
| })() | |
| ); | |
| } | |
| // Wait for all cleanup tasks to settle | |
| await Promise.allSettled(cleanupTasks); |
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
vite.config.ts (1)
19-30: Contradictory concurrency configuration.The configuration sets
maxThreads: 4(allowing up to 4 parallel threads) butmaxConcurrency: 1(forcing sequential test execution). This means only 1 test runs at a time, rendering the 4 threads wasteful. If sequential execution is required for stability as noted in the comment, consider settingmaxThreads: 1to match. If parallel execution is desired, increasemaxConcurrencyto leverage the available threads.Apply this diff if sequential execution is the intent:
poolOptions: { threads: { singleThread: false, // Allow some parallelism minThreads: 1, - maxThreads: 4, // Increased parallelism for faster test execution + maxThreads: 1, // Match sequential execution intent }, },Or if parallel execution is desired:
// Enable isolation with proper cleanup to prevent interference isolate: true, // Ensure test isolation for reliability - maxConcurrency: 1, // Sequential execution for stability + maxConcurrency: 4, // Allow parallel execution to match thread poolsrc/lib/adapters/providerImageAdapter.ts (3)
174-178: Missing support for “google-ai-studio” routing.“google-ai-studio” is used elsewhere in the repo/docs. Add a case to ensure proper formatting.
- case "google-ai": - case "google": + case "google-ai": + case "google": + case "google-ai-studio": adaptedPayload = this.formatForGoogleAI(text, images); break;
50-56: Add “google-ai-studio” to capability map.Without it, validateVisionSupport() will reject that provider.
- "google-ai": [ + "google-ai": [ "gemini-2.5-pro", "gemini-2.5-flash", "gemini-1.5-pro", "gemini-1.5-flash", "gemini-pro-vision", ], + "google-ai-studio": [ + "gemini-2.5-pro", + "gemini-2.5-flash", + "gemini-1.5-pro", + "gemini-1.5-flash", + "gemini-pro-vision", + ],
311-335: Azure model validation will false‑fail with custom deployment names.Azure uses deployment names (e.g., “prod-vision”), not raw model IDs. substring includes() will reject valid deployments.
private static validateVisionSupport(provider: string, model: string): void { const normalizedProvider = provider.toLowerCase(); const supportedModels = VISION_CAPABILITIES[ normalizedProvider as keyof typeof VISION_CAPABILITIES ]; if (!supportedModels) { throw new Error( `Provider ${provider} does not support vision processing. ` + `Supported providers: ${Object.keys(VISION_CAPABILITIES).join(", ")}`, ); } - const isSupported = supportedModels.some((supportedModel) => - model.toLowerCase().includes(supportedModel.toLowerCase()), - ); + // Azure/Azure OpenAI use deployment names that may not include model ids + const isAzure = normalizedProvider === "azure" || normalizedProvider === "azure-openai"; + const isSupported = isAzure + ? true // trust caller for Azure; deployments map to vision-capable models externally + : supportedModels.some((supportedModel) => + model.toLowerCase().includes(supportedModel.toLowerCase()), + ); if (!isSupported) { throw new Error( `Provider ${provider} with model ${model} does not support vision processing. ` + `Supported models for ${provider}: ${supportedModels.join(", ")}`, ); } }
♻️ Duplicate comments (9)
src/lib/agent/directTools.ts (1)
37-37: Good fix: description now matches actual behavior (no hardcoded default).Aligns with prior feedback; optional timezone now truly defaults to system local time.
docs/index.md (1)
274-274: Clarify multi-provider PDF support.The comment incorrectly implies Vertex is the only provider supporting PDF. According to docs/features/pdf-support.md, multiple providers support PDF including Anthropic, AWS Bedrock, Google AI Studio, OpenAI, LiteLLM, and Mistral.
Update the comment:
- provider: "vertex", // Vertex is one of several providers supporting PDF (see docs/features/pdf-support.md) + provider: "vertex", // PDF-capable provider (see docs/features/pdf-support.md for complete list)Or more concisely:
- provider: "vertex", // Vertex is one of several providers supporting PDF (see docs/features/pdf-support.md) + provider: "vertex", // Supports PDF along with anthropic, bedrock, google-ai, openai, litellm, mistraldocs/features/multimodal-chat.md (1)
251-265: Align provider table with actual PDF support.
PDF_PROVIDER_CONFIGSalready enables Ollama at 10 MB / 100 pages, yet the table still claims it’s unsupported. Please add Ollama to the supported list (matching the configured limits) and drop it from the “Not supported” note so the docs reflect reality.-| **Hugging Face** | 10 MB | 100 | Native PDF support | - -**Not supported:** Ollama +| **Hugging Face** | 10 MB | 100 | Native PDF support | +| **Ollama** | 10 MB | 100 | Native PDF support |src/lib/providers/openaiCompatible.ts (1)
285-316: AwaitstreamTextbefore using its result
streamText()returns a promise. Without awaiting it,resultremains unresolved andresult.textStreamaccess blows up at runtime (the same issue flagged earlier). Await the call before consuming the stream.- const result = streamText({ + const result = await streamText({src/lib/core/baseProvider.ts (2)
1097-1119: Remove unusedoriginalInputSchemaDeclared but not used after assignment. Drop it to avoid lint/no-unused-vars.
1400-1442: Strict-mode JSON Schema fix preservesrequiredcorrectlyNo longer forces all props to required; only enforces
additionalProperties: falserecursively. This addresses prior concern.src/lib/providers/ollama.ts (1)
552-558: PDF handling for Ollama fixed (reject early, exclude from detection)No more silent drops; clear error path. Good alignment with provider capabilities.
Also applies to: 563-569, 754-760
src/lib/utils/messageBuilder.ts (2)
86-92: Extract the inline union type for maintainability.The return type includes an inline union
{ type: "file"; data: Buffer; mimeType: string }that makes the signature harder to read. This was previously flagged by Copilot.Apply this diff to extract the type:
+type ConvertedContentItem = TextPart | ImagePart | { type: 'file'; data: Buffer; mimeType: string } | null; + -function convertContentItem( - item: unknown, -): - | TextPart - | ImagePart - | { type: "file"; data: Buffer; mimeType: string } - | null { +function convertContentItem(item: unknown): ConvertedContentItem {
387-387: Hardcoded file size limit doesn't match provider-specific limits.The
maxSizewas increased to 50MB, but this unified processing path uses a single hardcoded limit that could:
- Allow files too large for some providers (e.g., 50MB → fails for 5MB-limited providers)
- Block files some providers accept (e.g., 100MB → rejected but Google AI Studio supports 2GB)
This echoes concerns from previous review comments by Copilot and coderabbitai[bot].
Consider passing the
providerparameter to this function and using provider-specific limits viaPDFProcessor.getProviderConfig(provider).
🧹 Nitpick comments (16)
src/lib/providers/amazonBedrock.ts (3)
806-811: Make MIME type check case-insensitive and handle variations.The previous critical issue (PDFs dropped when emitted as
type: "file") is now fixed. However, the MIME type comparison at line 810 is case-sensitive and won't match variations like"application/PDF"or"application/pdf;charset=utf-8".Apply this diff to handle MIME type variations:
} else if ( contentItem.type === "document" || contentItem.type === "pdf" || (contentItem.type === "file" && - contentItem.mimeType === "application/pdf") + contentItem.mimeType?.toLowerCase().startsWith("application/pdf")) ) {
813-825: Use regex for data URI stripping for consistency and robustness.The previous critical issue (corrupted PDF bytes from data URIs) is now fixed with the stripping logic. However, the current approach using
indexOfandsubstringis less robust than the regex pattern used for images at line 781.Apply this diff to align with the image handling approach:
let docData: Buffer; if (typeof contentItem.data === "string") { - // Strip data URI prefix if present (e.g., "data:application/pdf;base64,...") - const dataStr = contentItem.data; - if (dataStr.startsWith("data:")) { - const commaIndex = dataStr.indexOf(","); - const base64Payload = - commaIndex !== -1 - ? dataStr.substring(commaIndex + 1) - : dataStr; - docData = Buffer.from(base64Payload, "base64"); - } else { - docData = Buffer.from(dataStr, "base64"); - } + const pdfString = contentItem.data.replace( + /^data:application\/pdf;base64,/i, + "", + ); + docData = Buffer.from(pdfString, "base64"); } else {
833-833: Unsafe type assertion for document name.The type assertion
(contentItem.name as string)assumesnameis always a string, but it could beundefinedornull. While the fallback to"document.pdf"handles falsy values, the assertion bypasses TypeScript's type safety.Apply this diff to use proper type checking:
- name: (contentItem.name as string) || "document.pdf", + name: typeof contentItem.name === "string" && contentItem.name + ? contentItem.name + : "document.pdf",src/lib/agent/directTools.ts (2)
113-115: Zod default positioning is fine.default(false) + optional is acceptable; no behavior change. Consider leaving out optional() since default already handles undefined.
- includeHidden: z - .boolean() - .optional() - .default(false) + includeHidden: z + .boolean() + .default(false)
438-446: Defaulting column to empty string is unnecessary and mildly confusing.Since operations check if (!column), an empty string behaves the same as undefined. Prefer omitting default to reduce ambiguity.
- column: z - .string() - .optional() - .default("") + column: z + .string() + .optional()src/lib/adapters/providerImageAdapter.ts (2)
184-187: Ollama formatting path likely incorrect for images.Most Ollama OpenAI‑compat endpoints don’t accept OpenAI’s image_url format; they often require “images” arrays or provider‑specific fields. Verify before routing via formatForOpenAI.
If the Ollama client you target implements OpenAI chat completions with image_url, keep as-is; otherwise, add an Ollama‑specific formatter (e.g., using base64 inline or the “images” array expected by Ollama).
27-49: Model lists drift risk.Large hardcoded lists will rot. Consider centralizing to provider docs or env-configurable allowlists; fall back to provider capability checks where available.
Also applies to: 65-79, 81-112, 113-144
src/lib/providers/litellm.ts (1)
238-239: Default maxSteps applied.Sensible defaulting. Consider documenting DEFAULT_MAX_STEPS in provider README for predictable tool behavior.
test/continuous-test-suite.ts (2)
2890-2909: Streaming skip check too strict; only matches exact “gpt-5” or “o3”.Models like “gpt-5-mini” or “o3-mini” won’t be skipped. Use substring/startsWith.
- if (provider === "openai" && (model === "gpt-5" || model === "o3")) { + if ( + provider === "openai" && + (model?.startsWith("gpt-5") || model?.startsWith("o3")) + ) { return true; }
2411-2455: CLI PDF generate test is fine; ensure fixtures exist.If test/fixtures/valid-sample.pdf isn’t present in published tarball/CI, add guard like README screenshot test to skip gracefully.
README.md (1)
274-274: Clarify multi-provider PDF support.The comment suggests Vertex is required for PDF support, but according to the PR summary and docs/features/pdf-support.md, multiple providers support PDF (Anthropic, AWS Bedrock, Google AI Studio, OpenAI, LiteLLM, Mistral).
Update the comment to clarify:
- provider: "vertex", // Required for PDF + provider: "vertex", // PDF-capable provider (also: anthropic, openai, bedrock, google-ai, litellm, mistral)Or simply:
- provider: "vertex", // Required for PDF + provider: "vertex", // Supports PDF (see docs/features/pdf-support.md for all PDF-capable providers)src/cli/factories/commandFactory.ts (1)
79-82: NewFlag parsed correctly. Recommend updating:
- completion script suggestions
- command examples (generate/stream) to include
src/lib/utils/pdfProcessor.ts (3)
133-146: Supported providers list may include duplicates/aliasesDeduplicate/sort to avoid confusing output (e.g., huggingface vs hugging-face).
Apply this diff:
- const supportedProviders = Object.keys(PDF_PROVIDER_CONFIGS) - .filter((p) => PDF_PROVIDER_CONFIGS[p].supportsNative) - .join(", "); + const supportedProviders = Array.from( + new Set( + Object.keys(PDF_PROVIDER_CONFIGS).filter( + (p) => PDF_PROVIDER_CONFIGS[p].supportsNative, + ), + ), + ) + .sort() + .join(", ");
195-202: Normalize provider key in helpers
supportsNativePDF()andgetProviderConfig()should lowercase provider likeprocess()to avoid missed matches.Apply this diff:
- static supportsNativePDF(provider: string): boolean { - const config = PDF_PROVIDER_CONFIGS[provider]; + static supportsNativePDF(provider: string): boolean { + const key = (provider || "").toLowerCase(); + const config = PDF_PROVIDER_CONFIGS[key] ?? PDF_PROVIDER_CONFIGS[provider]; return config?.supportsNative || false; } static getProviderConfig(provider: string): PDFProviderConfig | null { - return PDF_PROVIDER_CONFIGS[provider] || null; + const key = (provider || "").toLowerCase(); + return PDF_PROVIDER_CONFIGS[key] ?? PDF_PROVIDER_CONFIGS[provider] ?? null; }
218-219: Tighten page-count heuristic regexUse a word boundary to avoid matching
/Pages.Apply this diff:
- const pageMatches = header.match(/\/Type\s*\/Page[^s]/g); + const pageMatches = header.match(/\/Type\s*\/Page\b/g);src/lib/providers/ollama.ts (1)
847-878: Ensure toolparametersare JSON-serializable
parametersmay be Zod/Schema objects from the AI SDK. BeforeJSON.stringify, unwrap to plain JSON (e.g., if{ type: 'json_schema', schema }useschema; if Zod, convert via zod-to-json-schema or fallback to{ type:'object', properties:{}, required:[] }).Example adjustment:
- private convertToolsToOllamaFormat(tools: unknown): unknown[] { + private convertToolsToOllamaFormat(tools: unknown): unknown[] { if (!tools || typeof tools !== "object") { return []; } const toolsArray = Array.isArray(tools) ? tools : Object.values(tools); - return toolsArray.map((tool: { name?: string; description?: string; parameters?: unknown; function?: { name?: string; description?: string; parameters?: unknown; }; }) => ({ + const serialize = (p: unknown): Record<string, unknown> => { + const rec = p as Record<string, unknown>; + if (rec && rec.type === "json_schema" && rec.schema) return rec.schema as Record<string, unknown>; + if (rec && typeof (rec as any).parse === "function") { + // TODO: convert Zod -> JSON Schema; fallback minimal object to avoid invalid payloads + return { type: "object", properties: {}, required: [] }; + } + return (rec as Record<string, unknown>) || { type: "object", properties: {}, required: [] }; + }; + return toolsArray.map((tool: { name?: string; description?: string; parameters?: unknown; function?: { name?: string; description?: string; parameters?: unknown; }; }) => ({ type: "function", function: { name: tool.name || tool.function?.name, description: tool.description || tool.function?.description, - parameters: tool.parameters || - tool.function?.parameters || { - type: "object", - properties: {}, - required: [], - }, + parameters: serialize(tool.parameters || tool.function?.parameters), }, })); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
examples/data/invoice.pdfis excluded by!**/*.pdfexamples/data/report.pdfis excluded by!**/*.pdftest/fixtures/invalid.pdfis excluded by!**/*.pdftest/fixtures/multi-page.pdfis excluded by!**/*.pdftest/fixtures/valid-sample.pdfis excluded by!**/*.pdf
📒 Files selected for processing (35)
README.md(3 hunks)TEST_RESULTS.md(1 hunks)docs/features/index.md(2 hunks)docs/features/multimodal-chat.md(1 hunks)docs/features/pdf-support.md(1 hunks)docs/index.md(3 hunks)examples/data/README.md(3 hunks)examples/pdf-analysis.ts(1 hunks)src/cli/factories/commandFactory.ts(5 hunks)src/lib/adapters/providerImageAdapter.ts(3 hunks)src/lib/agent/directTools.ts(3 hunks)src/lib/core/baseProvider.ts(10 hunks)src/lib/neurolink.ts(3 hunks)src/lib/providers/amazonBedrock.ts(15 hunks)src/lib/providers/anthropic.ts(3 hunks)src/lib/providers/azureOpenai.ts(3 hunks)src/lib/providers/googleAiStudio.ts(3 hunks)src/lib/providers/googleVertex.ts(5 hunks)src/lib/providers/huggingFace.ts(2 hunks)src/lib/providers/litellm.ts(2 hunks)src/lib/providers/mistral.ts(2 hunks)src/lib/providers/ollama.ts(6 hunks)src/lib/providers/openAI.ts(3 hunks)src/lib/providers/openaiCompatible.ts(2 hunks)src/lib/types/content.ts(1 hunks)src/lib/types/fileTypes.ts(3 hunks)src/lib/types/generateTypes.ts(1 hunks)src/lib/types/providers.ts(2 hunks)src/lib/types/streamTypes.ts(1 hunks)src/lib/utils/fileDetector.ts(4 hunks)src/lib/utils/messageBuilder.ts(15 hunks)src/lib/utils/multimodalOptionsBuilder.ts(1 hunks)src/lib/utils/pdfProcessor.ts(1 hunks)test/continuous-test-suite.ts(40 hunks)vite.config.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-17T17:55:15.261Z
Learnt from: RajuSudhar
PR: juspay/neurolink#173
File: src/lib/index.ts:16-16
Timestamp: 2025-09-17T17:55:15.261Z
Learning: In src/lib/types/providers.ts, ProviderConfig was renamed to AIModelProviderConfig to deduplicate type names, as there was an existing ProviderConfig type that better suited the "ProviderConfig" name. This was an intentional breaking change for better type organization.
Applied to files:
src/lib/providers/openAI.tssrc/lib/providers/azureOpenai.tssrc/lib/adapters/providerImageAdapter.tssrc/lib/neurolink.ts
📚 Learning: 2025-09-01T22:58:39.149Z
Learnt from: sudharsan-juspay
PR: juspay/neurolink#140
File: src/lib/core/types.ts:198-203
Timestamp: 2025-09-01T22:58:39.149Z
Learning: In src/lib/core/types.ts, StreamOptions (imported from streamTypes.js) and StreamingOptions are intentionally different types with different use cases. StreamingOptions is for unified AI requests with multiple provider configurations, while StreamOptions is for individual streaming operations.
Applied to files:
src/lib/types/streamTypes.ts
🔇 Additional comments (36)
vite.config.ts (2)
2-2: Import fromvitest/configis correct for Vitest configuration.The previous Copilot comment suggesting to import from
viteis incorrect. When using Vitest with test configuration, importingdefineConfigfromvitest/configis the recommended approach per Vitest documentation. This import re-exports Vite'sdefineConfigwith Vitest-specific type augmentation, providing proper type inference for thetestconfiguration block without requiring explicit type assertions.
19-26: Verify thread pool compatibility with process-spawning tests. continuous-test-suite.ts imports child_process.spawn, reads process.env and relies on process.exit; running it in worker threads may share state or propagate exits—consider using forks or isolating this suite.src/lib/providers/amazonBedrock.ts (2)
865-917: LGTM: Multimodal streaming support.The multimodal input detection properly checks for all file types (images, PDFs, CSVs, files, content), and the message building flow correctly uses
buildMultimodalOptions→buildMultimodalMessagesArray→convertToBedrockMessages. The fallback to text-only messaging for simple cases is appropriate.
1371-1507: LGTM: Tool execution tracking.The tool execution storage implementation properly tracks both tool calls (lines 1402-1408) and results (lines 1421-1427, 1446-1452), validates 1:1 mapping between tool uses and results (lines 1473-1481), and safely handles storage failures (lines 1501-1506) without disrupting the streaming flow.
docs/features/pdf-support.md (1)
148-157: Verify OpenAI limits and wording.Table shows OpenAI supported (10 MB/100 pages). Ensure this remains consistent across the doc (unsupported list and troubleshooting should not name OpenAI).
src/lib/providers/litellm.ts (1)
183-229: Multimodal path selection and conversion look solid.Clean split between multimodal vs text flow, proper CoreMessage conversion, and useful debug logs.
Please confirm buildMultimodalMessagesArray handles PDFs for LiteLLM’s OpenAI‑compat path (Files API vs inline). If needed, we can add adapter guards per upstream model family.
src/lib/types/generateTypes.ts (1)
24-24: LGTM!The
pdfFilesfield addition is consistent with the existing multimodal input pattern and properly typed.docs/features/index.md (2)
32-32: LGTM!The PDF File Support documentation entry is well-structured and consistent with other feature entries in the table.
42-50: LGTM!The Platform Capabilities table accurately reflects the expanded multimodal support including PDFs, with appropriate documentation links.
src/lib/providers/googleAiStudio.ts (3)
34-34: LGTM!Importing the centralized multimodal options builder improves code maintainability across providers.
162-164: LGTM!The multimodal input detection correctly includes all file types (images, content, files, csvFiles, pdfFiles) for comprehensive multimodal handling.
178-182: LGTM!Refactoring to use
buildMultimodalOptionscentralizes the options construction logic, improving consistency across providers and reducing code duplication.src/lib/types/streamTypes.ts (1)
148-149: LGTM!The type definitions for
csvFilesandpdfFilesare properly typed, and the comments clearly distinguish between text-converted CSV files and binary-processed PDF documents.README.md (2)
29-29: LGTM!The PDF File Support entry in "What's New" is concise and includes an appropriate link to the PDF Guide.
288-295: LGTM!The Platform Capabilities table accurately reflects the expanded multimodal pipeline supporting CSV and PDF documents alongside images.
src/lib/providers/azureOpenai.ts (3)
20-20: LGTM!Importing the centralized multimodal options builder ensures consistency with other providers.
151-153: LGTM!The multimodal input detection comprehensively checks all file types including the newly added
pdfFiles.
167-171: LGTM!Refactoring to use
buildMultimodalOptionsreduces code duplication and centralizes multimodal options construction across providers.docs/index.md (3)
29-29: LGTM!The PDF File Support entry is properly documented in "What's New" with an appropriate link to the comprehensive PDF guide.
271-271: LGTM!The example now correctly references
examples/data/invoice.pdf, which is an actual file added in this PR.
286-294: LGTM!The Platform Capabilities table comprehensively reflects the expanded multimodal support for images, CSV, and PDF documents.
examples/data/README.md (4)
1-3: LGTM!The updated header and description accurately reflect the expanded scope to include both CSV and PDF sample files.
42-76: LGTM!The documentation for the new PDF files (
invoice.pdfandreport.pdf) is comprehensive, clearly describing contents, structure, and intended use cases.
79-102: LGTM!The usage section provides clear examples for both CSV and PDF analysis, including multimodal scenarios combining both file types.
122-143: LGTM!The example queries comprehensively demonstrate various PDF analysis scenarios (single-page, multi-page, comparison, and multimodal) with practical prompts.
src/cli/factories/commandFactory.ts (3)
262-271: PDF CLI normalization mirrors CSV/imagesReturns array consistently. LGTM.
1430-1444: ThreadingpdfFilesintogenerate()input is correctPayload assembly is consistent with images/CSV/files.
Also applies to: 1446-1447
1685-1687: ThreadingpdfFilesintostream()input is correctConsistent with non-stream path. LGTM.
Also applies to: 1693-1699
src/lib/core/baseProvider.ts (2)
56-65: MultimodalInput extended with PDFsType addition is minimal and safe.
357-360: PDF included in multimodal detection/build pathsWiring into
buildMultimodalMessagesArraylooks correct.Also applies to: 370-379
src/lib/utils/pdfProcessor.ts (1)
116-123: Magic-byte check is clear and correctHeader validation via
%PDF-signature is good.src/lib/types/providers.ts (2)
590-616: Bedrock content blocks extended (image/document)Matches multimodal/document work. LGTM.
738-769: Ollama tool/message types addedTypes align with provider implementation.
src/lib/utils/messageBuilder.ts (3)
809-809: LGTM! Type annotations are accurate.The return type and content array typing correctly reflect the image-only processing without PDFs.
Also applies to: 845-845, 920-924
937-983: LGTM! Multimodal content assembly is well-structured.The new function correctly combines text, images, and PDFs into a unified content array. The use of
map()for PDFs (lines 969-980) addresses the efficiency concern raised in previous review comments.Minor suggestion: The image content addition (lines 959-965) could be simplified using spread:
// Add images if present if (images.length > 0) { const imageContent = await convertSimpleImagesToProviderFormat( "", images, provider, model, ); - if (Array.isArray(imageContent)) { - imageContent.forEach((item) => { - if (item.type !== "text") { - content.push(item); - } - }); - } + content.push(...imageContent.filter(item => item.type !== "text")); }
23-23: Approve PDF integration and FilePart import
FilePartis available inaiv4.3.16 (dist/index.d.ts,rsc/dist/index.d.ts). No further action required.
…ment processing
Implement complete PDF file processing capabilities for the multimodal pipeline,
enabling AI-powered document analysis across providers with native PDF support.
This feature adds automatic file type detection, provider compatibility checks,
and seamless binary document passing to AI vision models.
**Core Features:**
* **Native PDF Processing**
- Direct binary document passing (no text conversion)
- Preserves visual elements (charts, tables, images, formatting)
- Provider-specific validation (file size, page limits)
- Magic byte detection (%PDF- signature verification)
- Support for single and multi-page documents
* **Provider Compatibility System**
- Vertex AI: 5MB, 100 pages (document API)
- Anthropic: 5MB, 100 pages (document API)
- AWS Bedrock: 5MB, 100 pages with Converse API support
- Google AI Studio: 2000MB, 100 pages (Files API)
- OpenAI: 10MB, 100 pages (Files API)
- LiteLLM: 10MB, 100 pages (Files API)
- OpenAI Compatible: 10MB, 100 pages (Files API)
- Clear error messages for unsupported providers
* **Auto-Detection Integration**
- Works with unified `files` array (PDFs, CSVs, images)
- Explicit PDF processing via `pdfFiles` array
- Supports file paths, URLs, Buffers, and data URIs
- Seamless integration with existing FileDetector system
* **CLI Integration**
- `--pdf <path>` for explicit PDF files (can be used multiple times)
- `--file <path>` for auto-detection (PDF, CSV, images)
- Full provider compatibility validation at CLI level
- Helpful error messages with provider suggestions
* **SDK Integration**
- `pdfFiles` array for explicit PDF processing
- `files` array for auto-detected file types
- Full TypeScript type safety
- Backward compatible with existing `images` and `csvFiles` arrays
- Streaming support via `stream()` method
**Type System Enhancements:**
* **Provider Type Organization**
- Moved Ollama type definitions to src/lib/types/providers.ts:734-768
- Added OllamaMessage, OllamaToolCall, OllamaToolResult types
- Enhanced BedrockContentBlock with image and document support
- Centralized provider-specific types for better maintainability
**Vision Model Support Expansion:**
* **OpenAI Provider**
- GPT-5 family: gpt-5, gpt-5-2025-08-07, gpt-5-pro, gpt-5-mini, gpt-5-nano
- GPT-4.1 family: gpt-4.1, gpt-4.1-mini, gpt-4.1-nano
- o-series reasoning: o3, o3-mini, o4, o4-mini, o4-mini-deep-research
- Existing GPT-4 models maintained
* **Anthropic Provider**
- Claude 3.7 Sonnet support
- Existing Claude 3.x models
* **Vertex AI Provider**
- Claude 4.x models: claude-sonnet-4-5@, claude-sonnet-4@, claude-opus-4-1@
- Claude 3.7 models: claude-3-7-sonnet@
- Updated versioned model patterns
- Gemini 2.0 Flash support
* **Ollama Provider**
- Llama 4 family: llama4, llama4:scout, llama4:maverick
- Gemma 3 family: gemma3, gemma3n, gemma3-it
- Qwen 3 family: qwen3, qwen2.5vl, qwen2.5-vl
- Mistral Small 3: mistral-small3, mistral-small3.1, mistral-small3.2
- DeepSeek R1: deepseek-r1, deepseek-r1-qwen
- LLaVA family: llava, llava:7b, llava:13b, llava:34b, llava-phi3
- Other models: moondream, bakllava
**Multimodal Message Handling:**
* **Ollama Provider**
- extractImagesFromMessages() method for base64 image extraction
- convertToOllamaMessages() for multimodal format conversion
- Handles text + image combinations in chat format
* **Amazon Bedrock Provider**
- convertToBedrockMessages() with image and document support
- BedrockContentBlock enhancements for multimodal content
- Proper handling of system messages
* **HuggingFace Provider**
- Multimodal message builder integration
- Support for images, PDFs, CSVs, and files array
- Enhanced logging for multimodal input detection
* **Other Providers**
- Mistral, LiteLLM, OpenAI Compatible: multimodal enhancements
- Google Vertex: improved message conversion
- Azure OpenAI: updated for multimodal support
**Resource Management:**
* **NeuroLink Cleanup**
- Added dispose() method (140 lines) for proper resource cleanup
- MCP server connection shutdown
- Event listener cleanup to prevent memory leaks
- Circuit breaker cleanup
- Prevents resource leaks in test environments
- Error aggregation for cleanup failures
**Code Quality:**
* **Lint Fixes**
- Removed unused imports from amazonBedrock.ts
- Fixed parameter naming in ollama.ts (_analysisSchema)
- Reduced nesting depth in ollama.ts (7→6 levels)
- Added eslint suppressions where needed
**Implementation Details:**
- Created `PDFProcessor` utility with provider config system (201 lines)
- Enhanced `FileDetector` with PDF magic byte detection (11 lines)
- Updated `messageBuilder` for PDF multimodal processing (148 lines)
- Added provider-specific PDF support in 11 provider files
- Comprehensive type system in `fileTypes.ts` for PDF configs
- Provider capability matrix with size/page limits
- Updated providerImageAdapter with 113 lines of vision model definitions
**Testing:**
- 8 new PDF-specific tests in continuous-test-suite.ts
- Test coverage: CLI generate, CLI stream, SDK generate, SDK stream
- Test fixtures: valid-sample.pdf, multi-page.pdf, invalid.pdf
- Multimodal tests: PDF + CSV, PDF + image combinations
- Provider compatibility validation tests
- File format validation tests
- Enhanced test suite with 510 lines of updates
**Documentation:**
- Comprehensive `pdf-support.md` guide (832 lines)
- Updated `multimodal-chat.md` with PDF section
- Updated `features/index.md` with PDF entry
- Created `examples/pdf-analysis.ts` with 7 examples
- Sample PDF documents in `examples/data/`
- Provider compatibility matrix documentation
**Files Changed:**
- Modified: 17 files (2,166 insertions, 268 deletions)
- Types: providers.ts (+58 lines - Ollama types + Bedrock enhancements)
- Providers:
- ollama.ts (+862 lines - multimodal support)
- amazonBedrock.ts (+237 lines - message conversion)
- huggingFace.ts (+89 lines - multimodal integration)
- mistral.ts (+88 lines - multimodal support)
- litellm.ts (+74 lines - multimodal support)
- openaiCompatible.ts (+82 lines - multimodal support)
- azureOpenai.ts (+20 lines - updates)
- googleVertex.ts (+2 lines - updates)
- openAI.ts (+1 line - eslint fix)
- Core:
- neurolink.ts (+143 lines - dispose method + cleanup)
- baseProvider.ts (+108 lines - multimodal support)
- Adapters: providerImageAdapter.ts (+113 lines - vision models)
- Utils:
- fileDetector.ts (+2 lines - PDF detection)
- pdfProcessor.ts (+38 lines - enhancements)
- Tools: directTools.ts (+7 lines - updates)
- Tests: continuous-test-suite.ts (+510 lines - comprehensive tests)
**Provider Support Matrix:**
- ✅ Vertex AI (5MB, 100 pages)
- ✅ Anthropic (5MB, 100 pages)
- ✅ AWS Bedrock (5MB, 100 pages)
- ✅ Google AI Studio (2000MB, 100 pages)
- ✅ OpenAI (10MB, 100 pages)
- ✅ LiteLLM (10MB, 100 pages)
- ✅ OpenAI Compatible (10MB, 100 pages)
- ✅ Mistral (enabled with multimodal)
- ✅ Ollama (enabled with multimodal)
- ✅ HuggingFace (enabled with multimodal)
- ❌ Azure OpenAI (not yet supported)
**Key Design Decisions:**
- PDFs passed as binary documents (not converted to text)
- Provider-specific size and page limits enforced
- No `pdfOptions` needed (unlike CSV) - binary format
- Visual analysis via AI vision models (charts, images, tables)
- Automatic format validation with clear error messages
- Centralized type definitions in types/providers.ts
- Vision model list expansion for future-proofing
- Resource cleanup patterns for production use
**Examples:**
```bash
npx @juspay/neurolink generate "Summarize this invoice" --pdf invoice.pdf --provider vertex
npx @juspay/neurolink generate "Compare Q1 and Q2" --pdf q1.pdf --pdf q2.pdf --provider anthropic
npx @juspay/neurolink generate "Analyze report, data, and chart" --file report.pdf --file data.csv --file chart.png --provider vertex
npx @juspay/neurolink stream "Explain this contract" --pdf contract.pdf --provider openai
npx @juspay/neurolink generate "Describe this image" --image photo.jpg --provider ollama --model llama4
```
```typescript
// SDK usage
await neurolink.generate({
input: {
text: "What is the total revenue in this financial report?",
pdfFiles: ["financial-report.pdf"]
},
provider: "vertex"
});
// Multiple PDFs
await neurolink.generate({
input: {
text: "Compare revenue figures between quarters",
pdfFiles: ["q1-report.pdf", "q2-report.pdf"]
},
provider: "anthropic"
});
// Auto-detection
await neurolink.generate({
input: {
text: "Analyze all provided documents",
files: ["report.pdf", "data.csv", "chart.png"]
},
provider: "vertex"
});
// Cleanup resources
await neurolink.dispose();
```
**Related:**
- Enhances multimodal pipeline capabilities
- Complements CSV support (374b375) with document analysis
- Works with 10+ AI providers (native support for 7)
- Follows repository coding standards
- Enterprise-ready with comprehensive error handling
- Foundation for future document processing features
- Supports latest AI models (GPT-5, Claude 4, Llama 4, Gemini 2.0)
f8ab247 to
9b3f0a1
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 |
…ment processing
Implement complete PDF file processing capabilities for the multimodal pipeline, enabling AI-powered document analysis across providers with native PDF support. This feature adds automatic file type detection, provider compatibility checks, and seamless binary document passing to AI vision models.
Core Features:
Native PDF Processing
Provider Compatibility System
Auto-Detection Integration
filesarray (PDFs, CSVs, images)pdfFilesarrayCLI Integration
--pdf <path>for explicit PDF files (can be used multiple times)--file <path>for auto-detection (PDF, CSV, images)SDK Integration
pdfFilesarray for explicit PDF processingfilesarray for auto-detected file typesimagesandcsvFilesarraysstream()methodType System Enhancements:
Vision Model Support Expansion:
OpenAI Provider
Anthropic Provider
Vertex AI Provider
Ollama Provider
Multimodal Message Handling:
Ollama Provider
Amazon Bedrock Provider
HuggingFace Provider
Other Providers
Resource Management:
Code Quality:
Implementation Details:
PDFProcessorutility with provider config system (201 lines)FileDetectorwith PDF magic byte detection (11 lines)messageBuilderfor PDF multimodal processing (148 lines)fileTypes.tsfor PDF configsTesting:
Documentation:
pdf-support.mdguide (832 lines)multimodal-chat.mdwith PDF sectionfeatures/index.mdwith PDF entryexamples/pdf-analysis.tswith 7 examplesexamples/data/Files Changed:
Provider Support Matrix:
Key Design Decisions:
pdfOptionsneeded (unlike CSV) - binary formatExamples:
Related:
Pull Request
Description
Type of Change
Related Issues
Changes Made
AI Provider Impact
Component Impact
Testing
Test Environment
Performance Impact
Breaking Changes
Screenshots/Demo
Checklist
Additional Notes
Summary by CodeRabbit
New Features
Documentation
Tests
Chores