Repository navigation
feat(multimodal): add file processor system with 17+ file types and S… - #809
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 |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Documentation Validation Results🚀 Documentation validation passed!
📦 Build artifact uploaded successfully. Ready for deployment preview. Commit: |
85ab808 to
89f230f
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🤖 Fix all issues with AI agents
In `@src/lib/image-gen/ImageGenService.ts`:
- Around line 179-230: The generateParams object in ImageGenService is missing
image count controls and the neurolink.generate call lacks timeout protection:
include options.numberOfImages (capped by this.config.maxImages) into
generateParams (e.g., numberOfImages or maxImages as the provider expects) so
callers can request multiple images and the config limit is enforced, and wrap
the neurolink.generate(generateParams) call with the withTimeout utility to
enforce this.config.timeout (or options.timeout) for graceful cancellation;
after generation continue using extractImageFromResult as before. Also replace
the deprecated substr() usage elsewhere in the same file with substring() to
avoid runtime deprecation issues.
In `@src/lib/processors/base/BaseFileProcessor.ts`:
- Around line 382-433: The downloadFile method implements its own
AbortController timeout; replace that manual timeout logic by wrapping the
fetch+response handling (including content-type check, arrayBuffer -> Buffer
conversion and optional gunzipAsync decompression) inside the shared withTimeout
helper so timeouts are consistent and provider fallback applied; remove the
explicit setTimeout/clearTimeout and AbortController handling (or use
withTimeout to provide/forward a signal if required), ensure you call
withTimeout(...) with this.config.timeoutMs (or the passed timeout) and preserve
the same error messages thrown for non-ok responses, HTML responses, and gzip
decompression failures so downloadFile's behavior and diagnostics stay
unchanged.
In `@src/lib/processors/config/fileTypes.ts`:
- Around line 238-246: The listed extension constants (e.g., R_EXTENSIONS,
ASSEMBLY_EXTENSIONS, JULIA_EXTENSIONS, etc.) contain uppercase entries that will
never match because isSupportedExtension lowercases filenames; update these
constants to use only lowercase extensions (e.g., ".r", ".rmd", ".s") or change
the matching logic in isSupportedExtension to perform a case-insensitive
compare, but prefer normalizing the arrays to lowercase for simplicity—modify
R_EXTENSIONS and ASSEMBLY_EXTENSIONS (and the other extension arrays around
lines 360-372) to contain only lowercase strings so they match the lowercased
filename check.
In `@src/lib/processors/data/XmlProcessor.ts`:
- Around line 158-170: The parseXmlSecurely function uses require() in ESM which
will break at runtime; replace the dynamic require with Node's createRequire by
importing createRequire from "module" and instantiating it with import.meta.url,
then use that require to load "fast-xml-parser" (so keep const { XMLParser } =
require("fast-xml-parser") but via createRequire). Ensure the import/typing is
added at the top of the module and that parseXmlSecurely continues to
instantiate XMLParser with the same options; apply the same createRequire fix in
OpenDocumentProcessor and YamlProcessor for their require() calls.
In `@src/lib/processors/document/OpenDocumentProcessor.ts`:
- Around line 80-83: The code in OpenDocumentProcessor uses a bare
require("adm-zip") inside the try block which fails in ESM; replace it with
Node's createRequire interoperability: import createRequire from "node:module"
(or get it via named import), construct a require function with
createRequire(import.meta.url), then call that require to load "adm-zip" (assign
to AdmZip) before creating zip from buffer; update the try block where AdmZip
and zip are created so it uses the createRequire-based require instead of the
bare require.
In `@src/lib/processors/errors/errorSerializer.ts`:
- Around line 271-303: The code uses createHash("md5") in
generateErrorFingerprint and generateFingerprintFromString which triggers
weak-crypto warnings; change the algorithm string to "sha256" for both uses
(keep the existing .digest(...).substring(0,16) truncation to preserve
fingerprint length) and update any inline comment if present to reflect SHA-256
is used; ensure the symbols touched are generateErrorFingerprint and
generateFingerprintFromString and replace both createHash("md5") calls with
createHash("sha256").
In `@src/lib/processors/integration/FileProcessorIntegration.ts`:
- Around line 165-190: processFileWithRegistry currently awaits
processor.processor.processFile and match.processor.processFile directly, which
can hang; wrap each per-file async call with the project's withTimeout helper
and treat a timeout as a failed/skipped processing (return null result or fall
back when options.allowFallback is set). Specifically: when calling
registry.getProcessor(...).processor.processFile(...) and when calling
match.processor.processFile(...), invoke withTimeout(process.call, timeoutMs)
(use the project standard timeout value) and catch timeout/errors to return {
processorName: null, result: null } or trigger fallback logic honoring
options.allowFallback; ensure any thrown timeouts are handled and do not block
batch processing. Also apply the same withTimeout wrapping to the other awaited
processor call referenced (lines ~242-245).
- Around line 174-259: The batch processor processBatchWithRegistry incorrectly
treats options.allowFallback (it’s unused for processing and the skipped reason
is inverted); update processBatchWithRegistry to either route files with no
matched processor to a default/fallback processor via processFileWithRegistry
(or registry.getProcessor('default') / registry.findProcessor fallback) when
options.allowFallback is true, or if you prefer to keep no fallback simply
invert the message and remove the unused flag; specifically, modify the
no-processor branch where result.skipped is pushed (and any earlier code that
ignores options.allowFallback) so that when options.allowFallback is true you
call the fallback processor and push to successful/failed based on its result,
otherwise push a skipped entry with the corrected reason string referencing
fileInfo.mimetype.
In `@src/lib/processors/markup/SvgProcessor.ts`:
- Around line 169-179: When sanitizeSvgContent(rawContent) throws inside
SvgProcessor.ts, do not use the partial regex fallback; instead "fail closed" by
returning or assigning a safe empty output (e.g., set textContent = "" or return
an empty safe SVG) so no executable content can leak. Locate the try/catch
around sanitizeSvgContent in SvgProcessor (variable textContent and rawContent)
and replace the catch block with logic that strips all content or returns a
known-safe sanitized string immediately rather than applying the current regex
replacements.
In `@src/lib/processors/registry/ProcessorRegistry.ts`:
- Around line 484-517: The current processWithResult block treats any processor
failure as NO_PROCESSOR_FOUND and returns type "unsupported"; update both the
non-success branch (where result.success is false) and the catch block so that
if a match and match.processor exist but processing failed you return type:
match.name (so callers know which processor was used) and set error.code to
"PROCESSOR_FAILED" (instead of "NO_PROCESSOR_FOUND"), preserving the existing
error.message, filename, mimetype, suggestion and supportedTypes; also consider
adding a small field like error.processor = match.name to both error objects to
make the failing processor explicit.
🟡 Minor comments (14)
src/lib/processors/code/ConfigProcessor.ts-179-200 (1)
179-200:⚠️ Potential issue | 🟡 MinorNormalize filenames for cross-platform
.envdetection.
split("/")misses Windows paths and.env.*files in subdirectories, which can cause mis-detection. Use a separator-agnostic basename for bothisFileSupportedandgetExtension.🔧 Suggested fix
- const basename = filename.split("/").pop() || filename; + const basename = filename.split(/[\\/]/).pop() ?? filename; @@ - if (filename.startsWith(".env")) { + const base = filename.split(/[\\/]/).pop() ?? filename; + if (base.startsWith(".env")) { return ".env"; } - const match = filename.toLowerCase().match(/\.[^.]+$/); + const match = base.toLowerCase().match(/\.[^.]+$/);Also applies to: 382-389
src/lib/processors/code/ConfigProcessor.ts-236-243 (1)
236-243:⚠️ Potential issue | 🟡 MinorPrettier: expand inline returns.
CI flagged formatting; these single-line
ifblocks are likely the culprit.🎨 Suggested formatting
- if (lowerExt === ".env") {return "env";} - if (lowerExt === ".ini" || lowerExt === ".cfg" || lowerExt === ".conf") {return "ini";} - if (lowerExt === ".toml") {return "toml";} - if (lowerExt === ".properties") {return "properties";} + if (lowerExt === ".env") { + return "env"; + } + if (lowerExt === ".ini" || lowerExt === ".cfg" || lowerExt === ".conf") { + return "ini"; + } + if (lowerExt === ".toml") { + return "toml"; + } + if (lowerExt === ".properties") { + return "properties"; + }src/lib/utils/json/extract.ts-29-69 (1)
29-69:⚠️ Potential issue | 🟡 MinorGreedy JSON regex can skip valid snippets.
The
{[\s\S]*}/[\s\S]*matches can swallow multiple JSON blocks (or braces in prose), causingJSON.parseto fail even when a valid snippet exists. Consider using the bracket-balancing approach fromextractAllJsonFromTextto locate the first complete object/array instead of a greedy regex.docs/migration/CURATOR_MIGRATION_ANALYSIS_REPORT.md-363-370 (1)
363-370:⚠️ Potential issue | 🟡 MinorPipeline failure: MDX compilation error due to unescaped generic type syntax.
The CI pipeline reports an MDX compilation failure at line 377: "End-tag-mismatch: Expected a closing tag for
<T>". This occurs because MDX interprets angle brackets in TypeScript generic syntax (likeToolExecutionResult<T>) as HTML tags.Escape the generic type or wrap it in backticks to prevent MDX parsing issues.
🔧 Proposed fix
-| **Tool Execution** | ToolResult | ToolExecutionResult<T> | ⚠️ Wrapper needed | +| **Tool Execution** | ToolResult | `ToolExecutionResult<T>` | ⚠️ Wrapper needed |docs/features/file-processors.md-31-31 (1)
31-31:⚠️ Potential issue | 🟡 MinorDocumentation omits
.docextension support for Word processor.The
WordProcessorimplementation supports both.docxand.docextensions (viaSUPPORTED_WORD_EXTENSIONS), but this table only lists.docx.📝 Suggested fix
-| **Word** | `.docx` | `WordProcessor` | Text extraction, paragraph preservation | +| **Word** | `.docx`, `.doc` | `WordProcessor` | Text extraction, paragraph preservation |src/lib/processors/markup/MarkdownProcessor.ts-111-111 (1)
111-111:⚠️ Potential issue | 🟡 MinorPriority comment is inconsistent with PROCESSOR_PRIORITIES constant.
The comment states "Priority: 25 (after HTML at priority 20)" but
PROCESSOR_PRIORITIES.MARKDOWNis defined as40inregistry/types.ts, andPROCESSOR_PRIORITIES.HTMLis80.📝 Suggested fix
- * Priority: 25 (after HTML at priority 20, before generic text) + * Priority: 40 (before JSON at priority 50, before generic text at 110)src/lib/processors/document/WordProcessor.ts-241-241 (1)
241-241:⚠️ Potential issue | 🟡 MinorNon-null assertion on
downloadResult.datacould mask edge cases.If
downloadResult.successistruebutdatais somehowundefined, this assertion would cause a runtime error. Consider adding an explicit check.🛡️ Proposed defensive check
buffer = downloadResult.data!; + if (!buffer) { + return { + success: false, + error: this.createError(FileErrorCode.DOWNLOAD_FAILED, { + reason: "Download succeeded but no data returned", + }), + }; + }src/lib/processors/document/RtfProcessor.ts-369-369 (1)
369-369:⚠️ Potential issue | 🟡 MinorFormatting inconsistency flagged by pipeline.
The pipeline indicates a Prettier formatting issue. Line 369 has
if (!filename) {return "Unknown";}which should likely have spaces around the braces.This appears to be in the wrong file context - the formatting issue is on RtfProcessor but this line is in languageMap. The RtfProcessor likely has similar formatting issues causing the pipeline warning.
src/lib/processors/config/languageMap.ts-369-369 (1)
369-369:⚠️ Potential issue | 🟡 MinorFormatting issue causing pipeline failure.
Line 369 has a formatting inconsistency that's causing the Prettier check to fail:
🔧 Fix formatting
export function detectLanguageFromFilename(filename: string): string { - if (!filename) {return "Unknown";} + if (!filename) { + return "Unknown"; + }src/lib/processors/document/RtfProcessor.ts-219-226 (1)
219-226:⚠️ Potential issue | 🟡 MinorPotential bug in skipGroup reset logic.
The
skipGroupflag is only reset whendepth <= 0, but it should reset when exiting the specific group that was marked for skipping. If a skippable group contains nested groups, the flag will remain true after exiting the inner groups but before exiting the skippable group itself, which is correct. However, ifdepthbecomes 0 or negative due to malformed RTF with unbalanced braces, content after the unbalanced close brace may be incorrectly skipped or included.Consider tracking the depth at which
skipGroupwas set:🔧 Suggested fix for tracking skip depth
let depth = 0; let skipGroup = false; + let skipGroupDepth = 0; let i = 0; // ... if (char === "{") { depth++; // Check if this is a group we should skip const nextChars = text.substring(i + 1, i + 20); const groupMatch = nextChars.match(/^\\([a-z]+)/); - if (groupMatch && skipGroupNames.includes(groupMatch[1])) { + if (groupMatch && skipGroupNames.includes(groupMatch[1]) && !skipGroup) { skipGroup = true; + skipGroupDepth = depth; } i++; continue; } if (char === "}") { depth--; - if (depth <= 0) { + if (skipGroup && depth < skipGroupDepth) { skipGroup = false; + skipGroupDepth = 0; } i++; continue; }src/lib/processors/document/ExcelProcessor.ts-366-371 (1)
366-371:⚠️ Potential issue | 🟡 MinorRemove unnecessary Buffer to ArrayBuffer cast.
The cast
buffer as unknown as ArrayBufferis redundant. ExcelJS v4.4.0 directly accepts Node.jsBufferin theload()method—pass the buffer without casting:await workbook.xlsx.load(buffer);The double-cast workaround
as unknown as ArrayBuffermasks a type mismatch that shouldn't exist and reduces code clarity.src/lib/processors/code/SourceCodeProcessor.ts-215-215 (1)
215-215:⚠️ Potential issue | 🟡 MinorFix Prettier warning for inline if.
Line 215 doesn’t match Prettier’s formatting and CI reports a formatting warning.
🛠️ Proposed fix
- if (!filename) {return false;} + if (!filename) { + return false; + }src/lib/processors/code/SourceCodeProcessor.ts-223-224 (1)
223-224:⚠️ Potential issue | 🟡 MinorUse
path.basename()to handle Windows path separators for exact filename matches.Line 223 only splits on
/, so Windows paths likeC:\path\Dockerfilewon't extract the basename correctly. Usenode:pathto normalize across platforms and align with camelCase conventions.Proposed fix
+import { basename as pathBasename } from "node:path"; import { BaseFileProcessor } from "../base/BaseFileProcessor.js"; @@ - const basename = filename.split("/").pop() || filename; - if (EXACT_FILENAME_MAP[basename]) { + const baseName = pathBasename(filename); + if (EXACT_FILENAME_MAP[baseName]) {src/lib/processors/config/sizeLimits.ts-210-212 (1)
210-212:⚠️ Potential issue | 🟡 MinorFix Prettier violation in
formatBytes.
CI already reports formatting issues; this inlineifis one of them.🧹 Suggested fix
- if (bytes === 0) {return "0 Bytes";} + if (bytes === 0) { + return "0 Bytes"; + }
🧹 Nitpick comments (29)
src/lib/utils/json/extract.ts (1)
105-108: Move exportedJsonTypeGuardto shared types.This exported type is shared API surface; centralize it under
src/lib/typesto match project standards.Based on learnings: Project standard: Place reusable/shared types under src/lib/types/.ts; test-only helper types under test/types/.ts; avoid declaring local types inside source implementation files.
src/lib/processors/code/ConfigProcessor.ts (1)
66-78: MoveProcessedConfigto shared types module.This exported, reusable type should live under
src/lib/typesfor consistency and discoverability.Based on learnings: Project standard: Place reusable/shared types under src/lib/types/.ts; test-only helper types under test/types/.ts; avoid declaring local types inside source implementation files.
docs/migration/CURATOR_MIGRATION_VERIFICATION.md (1)
36-39: Consider noting that paths are developer-specific.The document contains hardcoded local paths (e.g.,
/Users/sachinsharma/Developer/...) which are specific to the original author's environment. This is acceptable for internal migration documentation, but consider adding a note at the top indicating readers should substitute their own paths.src/lib/processors/errors/FileErrorCode.ts (1)
339-350: Add a safe fallback for unknown error codes.If a code is deserialized/cast from external input,
ERROR_MESSAGES[code]can beundefined, which can cascade into runtime errors. A defensive fallback toUNKNOWN_ERRORkeeps the API robust.♻️ Suggested fix
export function getErrorTemplate(code: FileErrorCode): ErrorMessageTemplate { - return ERROR_MESSAGES[code]; + return ERROR_MESSAGES[code] ?? ERROR_MESSAGES[FileErrorCode.UNKNOWN_ERROR]; } export function isRetryableErrorCode(code: FileErrorCode): boolean { - return ERROR_MESSAGES[code].retryable; + return (ERROR_MESSAGES[code] ?? ERROR_MESSAGES[FileErrorCode.UNKNOWN_ERROR]).retryable; }src/lib/processors/markup/SvgProcessor.ts (1)
66-78: MoveProcessedSvgto the shared types module.This is an exported, reusable type and should live under
src/lib/types/*with re-exports fromsrc/lib/types/index.tsto match the project’s type-centralization standard.
Based on learnings: Project standard: Place reusable/shared types under src/lib/types/.ts; test-only helper types under test/types/.ts; avoid declaring local types inside source implementation files.src/lib/utils/async/retry.ts (1)
192-195: ThrowRetryExhaustedErrorto preserve context.
RetryExhaustedErroris defined but unused. Throwing it on exhaustion keeps the original error and attempts count, improving diagnostics without changing successful behavior.♻️ Suggested fix
- if (attempt >= totalAttempts) { - throw err; - } + if (attempt >= totalAttempts) { + throw new RetryExhaustedError( + "Retry attempts exhausted", + totalAttempts, + err, + ); + }src/lib/image-gen/types.ts (1)
46-69: Consider using union types instead ofstringfor constrained fields.
ImageGenOptionsusesstringforprovider,aspectRatio, andstyle, but specific union types (ImageGenProvider,AspectRatio,StylePreset) are defined later in this file. Using the union types would provide better type safety and IDE autocompletion.♻️ Proposed refactor for type consistency
/** * Override default provider * e.g., "vertex", "openai" */ - provider?: string; + provider?: ImageGenProvider; ... /** * Aspect ratio for the generated image * e.g., "16:9", "1:1", "4:3", "9:16" */ - aspectRatio?: string; + aspectRatio?: AspectRatio; ... /** * Style preset for the image * e.g., "realistic", "artistic", "cartoon", "watercolor", "photorealistic" */ - style?: string; + style?: StylePreset;Note: This would require moving the type definitions before
ImageGenOptionsor using forward references.src/lib/processors/markup/TextProcessor.ts (1)
154-160: Consider using a text-specific truncation limit.The code uses
SIZE_LIMITS.MAX_SOURCE_CODE_LINESto truncate text files, but this constant name suggests it's intended for source code. Plain text files (logs, documents) may have different truncation requirements than source code.💡 Suggested improvement
Consider adding a dedicated
MAX_TEXT_LINESconstant in the size limits config, or rename the existing constant to be more generic (e.g.,MAX_TEXT_FILE_LINES) if it's intended to be shared across text-based processors.- if (lines.length > SIZE_LIMITS.MAX_SOURCE_CODE_LINES) { + if (lines.length > SIZE_LIMITS.MAX_TEXT_LINES) {src/lib/utils/fileDetector.ts (1)
654-687: Security consideration: Fallback returns unsanitized SVG content.When the SvgProcessor fails or is unavailable, the fallback returns raw SVG content without sanitization. If this content is later rendered in a browser context, it could pose an XSS risk.
The current implementation is acceptable since:
- The primary path uses sanitization
- The warning is logged when fallback occurs
- Consumers should be aware of the fallback behavior
However, consider documenting this behavior explicitly or adding a flag to indicate when content is unsanitized.
return { type: "svg", content: content.toString("utf-8"), mimeType: "image/svg+xml", metadata: { confidence: detection.metadata.confidence, size: content.length, filename: detection.metadata.filename, extension: detection.extension, + unsanitized: true, // Flag to indicate raw content }, };src/lib/processors/document/RtfProcessor.ts (1)
254-261: Hex escape handling for extended characters may produce incorrect results.Using
String.fromCharCodewith values from\'xxhex escapes assumes the character code is in the range 0-255, which works for Latin-1 but may not correctly handle characters from other code pages that RTF documents can use (via\ansicpgor\macfont declarations).This is a known limitation of lightweight RTF parsers. Consider documenting this limitation.
src/lib/processors/config/languageMap.ts (1)
437-458: Consider adding "GitHub CODEOWNERS" to code-related categories.
CODEOWNERSfiles define code ownership rules and are arguably code/config-related rather than plain documentation. The current implementation excludes it from source code files, which may be intentional but worth verifying.src/lib/processors/document/ExcelProcessor.ts (1)
404-412: Row iteration continues after limit is reached.The
returnstatement inside theeachRowcallback only exits the current callback invocation, not the entire iteration. ExcelJS will continue to call the callback for subsequent rows, though they won't be added to therowsarray.For very large sheets, this means all rows are still traversed even after hitting the limit. Consider using a flag to skip processing or breaking out earlier if performance is a concern.
💡 Early termination approach
ExcelJS doesn't support breaking out of
eachRow, but you could track a flag and return immediately:let hitLimit = false; worksheet.eachRow((row, rowNumber) => { if (hitLimit) return; // Skip processing but still called if (rowIndex >= maxRows) { hitLimit = true; if (!truncatedSheets.includes(worksheet.name)) { truncatedSheets.push(worksheet.name); } truncated = true; return; } // ... rest of processing });Alternatively, for large files, consider using streaming with
worksheet.getRows()which allows more control.src/lib/processors/data/YamlProcessor.ts (2)
184-192: Use dynamic import instead of require() for ESM consistency.Using
require()in an ESM module can cause issues and is inconsistent with the rest of the codebase which uses ES module imports. Consider using a dynamic import or moving the import to the top of the file.♻️ Suggested refactor using dynamic import
+import * as yaml from "js-yaml"; + // ... private parseYamlSecurely(content: string): unknown { - // Dynamically import js-yaml to parse YAML securely - const yaml = require("js-yaml"); return yaml.load(content, { schema: yaml.CORE_SCHEMA, // Only allow standard YAML types, no custom tags // Prevent billion laughs attack via alias expansion // Note: js-yaml doesn't have maxAliasCount, but using CORE_SCHEMA + size limits provides protection }); }Or if lazy loading is intended:
private async parseYamlSecurely(content: string): Promise<unknown> { - // Dynamically import js-yaml to parse YAML securely - const yaml = require("js-yaml"); + const yaml = await import("js-yaml"); return yaml.load(content, { schema: yaml.CORE_SCHEMA, }); }Note: If using async, you'll need to update the callers accordingly.
203-248: YAML is parsed twice - once for validation, once for result building.The YAML content is parsed in
validateDownloadedFileWithResultand again inbuildProcessedResult. For large files, this doubles the parsing overhead.Consider caching the parsed result during validation and reusing it, or restructuring to parse only once.
💡 Caching approach
One option is to store the parsed result as an instance property during validation:
private cachedParsedResult: unknown = null; protected override async validateDownloadedFileWithResult(...) { // ... dangerous tags check ... this.cachedParsedResult = this.parseYamlSecurely(content); return { success: true, data: undefined }; } protected override buildProcessedResult(...) { const content = buffer.toString("utf-8"); const parsed = this.cachedParsedResult; this.cachedParsedResult = null; // Clear cache // ... rest of method }However, this adds statefulness to the processor. Alternatively, consider if the architecture allows passing the parsed result through the processing pipeline.
Also applies to: 258-286
src/lib/processors/config/mimeTypes.ts (1)
263-296: Consider relocating MIME union types to the shared types module.Line 263 onward defines exported MIME union types inside this implementation file. To align with the project standard, move these reusable types (ImageMimeType, MimeType, etc.) into src/lib/types and re-export them here (and from src/lib/types/index.ts). Based on learnings: Project standard: Place reusable/shared types under src/lib/types/.ts; test-only helper types under test/types/.ts; avoid declaring local types inside source implementation files.
src/lib/processors/document/OpenDocumentProcessor.ts (1)
17-26: MoveProcessedOpenDocumentto the shared types module.Lines 17-26 export an interface from this implementation file; please relocate it to src/lib/types and re-export to keep shared types centralized. Based on learnings: Project standard: Place reusable/shared types under src/lib/types/.ts; test-only helper types under test/types/.ts; avoid declaring local types inside source implementation files.
src/lib/processors/code/SourceCodeProcessor.ts (1)
76-91: MoveProcessedSourceCodeto the shared types module.Lines 76-91 export an interface from this implementation file; please relocate it to src/lib/types and re-export from the processor barrel to keep shared types centralized. Based on learnings: Project standard: Place reusable/shared types under src/lib/types/.ts; test-only helper types under test/types/.ts; avoid declaring local types inside source implementation files.
src/lib/processors/integration/FileProcessorIntegration.ts (2)
70-124: Move integration types to the shared types module.Lines 70-124 declare
FileProcessingOptionsandBatchFileProcessingResultin this implementation file. Please relocate them to src/lib/types and re-export, keeping shared types centralized. Based on learnings: Project standard: Place reusable/shared types under src/lib/types/.ts; test-only helper types under test/types/.ts; avoid declaring local types inside source implementation files.
327-337: Avoid unsafe casts when exposing processor config.Lines 327-337 cast
processorthroughunknownto access config fields, which bypasses strict typing and can hide mismatches. Consider adding a typed accessor onProcessorRegistry/ProcessorEntry(or exposing config in the registry types) to preserve type safety. As per coding guidelines: Maintain strict TypeScript type safety across all modules with no implicit any and proper type inference.src/lib/processors/data/XmlProcessor.ts (1)
59-70: MoveProcessedXmlto the shared types module.Lines 59-70 export an interface from this implementation file; please relocate it to src/lib/types and re-export to keep shared types centralized. Based on learnings: Project standard: Place reusable/shared types under src/lib/types/.ts; test-only helper types under test/types/.ts; avoid declaring local types inside source implementation files.
src/lib/processors/registry/ProcessorRegistry.ts (2)
431-486: Add timeout/fallback handling around processor execution.
processFile/processWithResultcallprocessor.processFiledirectly; if a processor doesn't enforce its own timeout, the registry can hang and never try alternates. Consider wrapping the call withwithTimeoutand optionally falling back to the next match on timeout.
As per coding guidelines: Implement graceful provider fallback with withTimeout utility for async operations.
100-555: Consider extending the shared BaseRegistry.
This class re-implements common registry concerns (singleton, register/unregister, list, clear). IfBaseRegistryexists, aligning with it improves consistency and reduces duplication.
Based on learnings: Use BaseFactory and BaseRegistry in core/infrastructure for consistent factory and registry pattern implementation across all features.src/lib/processors/config/sizeLimits.ts (1)
306-316: Move exported size-limit types to a shared types module.
These are exported API types and should live undersrc/lib/typesper project standard to keep implementation files focused on logic.
Based on learnings: Project standard: Place reusable/shared types under src/lib/types/.ts; test-only helper types under test/types/.ts; avoid declaring local types inside source implementation files.src/lib/image-gen/ImageGenService.ts (2)
128-206: Wrapneurolink.generatewithwithTimeoutand fallback.
Currently the only timeout is passed through to the provider; a localwithTimeoutwrapper (and optional fallback provider) would guard against hangs and aligns with the async guideline.
As per coding guidelines: Implement graceful provider fallback with withTimeout utility for async operations.
79-85: Avoid deprecatedsubstrfor instanceId generation.
substris deprecated in modern JS/TS; usesliceinstead.🔧 Suggested fix
- this.instanceId = `ImageGenService-${Date.now()}-${Math.random().toString(36).substr(2, 9)}`; + this.instanceId = `ImageGenService-${Date.now()}-${Math.random().toString(36).slice(2, 11)}`;src/lib/processors/cli/fileProcessorCli.ts (2)
88-176: Avoid duplicating MIME mappings; reuse processors/config to prevent drift.The CLI hardcodes
EXTENSION_TO_MIME_TYPEwhile the processors/config layer already defines MIME types/extensions. This risks silent divergence as new types are added. Consider exposing a shared helper in processors/config (e.g.,getMimeTypeForExtension) and using that here to keep detection consistent.
424-445: Expose processor config via a public accessor instead of casting.
getSupportedFileTypesreaches intoproc.processorwith an unsafe cast to accessconfig. This bypasses theprotectedboundary and will break ifBaseFileProcessorchanges. Please add a public accessor (e.g.,getConfig()orgetSupportedTypes()) and consume that here.♻️ Suggested change (uses a public accessor)
- const config = ( - proc.processor as unknown as { - config?: { - supportedMimeTypes?: string[]; - supportedExtensions?: string[]; - }; - } - ).config; + const config = ( + proc.processor as { + getConfig?: () => { + supportedMimeTypes?: string[]; + supportedExtensions?: string[]; + }; + } + ).getConfig?.();src/lib/processors/base/BaseFileProcessor.ts (1)
127-163: Re-check size after download using the actual buffer length.
validateFileWithResultrelies onfileInfo.size, which can be missing or stale for URL downloads. Enforce the size limit after the buffer is available to prevent oversized files slipping through.🔧 Suggested patch
} else { // No buffer or URL provided return { success: false, error: this.createError(FileErrorCode.DOWNLOAD_FAILED, { reason: "No buffer or URL provided for file", }), }; } + // Enforce size limit against actual buffer length + if (!this.validateFileSize(buffer.length)) { + const sizeMB = this.formatSizeMB(buffer.length); + return { + success: false, + error: this.createError(FileErrorCode.FILE_TOO_LARGE, { + sizeMB, + maxMB: this.config.maxSizeMB, + type: this.config.fileTypeName, + }), + }; + } + // Step 3: Post-download validation (subclasses can override) const postValidationResult = await this.validateDownloadedFileWithResult(buffer, fileInfo);src/lib/processors/config/fileTypes.ts (1)
634-662: Move reusable extension types intosrc/lib/types.These exported types are reusable across the codebase, but the project standard is to place shared types under
src/lib/types/*.ts(and re-export fromsrc/lib/types/index.ts) instead of defining them locally in implementation files.Based on learnings: Project standard: Place reusable/shared types under src/lib/types/.ts; test-only helper types under test/types/.ts; avoid declaring local types inside source implementation files.
| // Determine provider and model | ||
| const provider = options.provider ?? this.config.defaultProvider; | ||
| const model = options.model ?? this.config.defaultModel; | ||
| const region = options.region ?? this.config.defaultRegion; | ||
|
|
||
| // Build generation parameters | ||
| const generateParams: Record<string, unknown> = { | ||
| input, | ||
| provider, | ||
| model, | ||
| disableTools: true, | ||
| temperature: | ||
| options.temperature ?? this.config.defaultTemperature ?? 0.75, | ||
| timeout: this.config.timeout, | ||
| }; | ||
|
|
||
| // Add region if specified (for Vertex AI) | ||
| if (region) { | ||
| generateParams.region = region; | ||
| } | ||
|
|
||
| // Add aspect ratio if specified | ||
| if (options.aspectRatio) { | ||
| generateParams.aspectRatio = options.aspectRatio; | ||
| } | ||
|
|
||
| // Call NeuroLink generate | ||
| const result = await neurolink.generate(generateParams); | ||
|
|
||
| // Extract image from result | ||
| const imageOutput = this.extractImageFromResult(result); | ||
| const generationTimeMs = Date.now() - startTime; | ||
|
|
||
| if (imageOutput) { | ||
| return { | ||
| success: true, | ||
| imageBuffer: imageOutput.imageBuffer, | ||
| base64: imageOutput.base64, | ||
| mimeType: imageOutput.mimeType, | ||
| model, | ||
| provider, | ||
| generationTimeMs, | ||
| }; | ||
| } | ||
|
|
||
| return { | ||
| success: false, | ||
| error: "No image generated", | ||
| model, | ||
| provider, | ||
| generationTimeMs, | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "ImageGenService.ts" | head -20Repository: juspay/neurolink
Length of output: 98
🏁 Script executed:
cat -n ./src/lib/image-gen/ImageGenService.tsRepository: juspay/neurolink
Length of output: 13200
🏁 Script executed:
find . -type f -name "types.ts" | grep -i imageRepository: juspay/neurolink
Length of output: 88
🏁 Script executed:
cat -n ./src/lib/image-gen/types.tsRepository: juspay/neurolink
Length of output: 7839
🏁 Script executed:
find . -type f -name "*.ts" | xargs grep -l "withTimeout" | head -10Repository: juspay/neurolink
Length of output: 423
🏁 Script executed:
grep -n "withTimeout" ./src/lib/core/baseProvider.ts | head -20Repository: juspay/neurolink
Length of output: 166
🏁 Script executed:
sed -n '1350,1430p' ./src/lib/core/baseProvider.tsRepository: juspay/neurolink
Length of output: 2662
🏁 Script executed:
find . -type f -name "errorHandling.ts"Repository: juspay/neurolink
Length of output: 92
🏁 Script executed:
grep -n "export.*withTimeout\|function withTimeout" ./src/lib/utils/errorHandling.ts -A 15Repository: juspay/neurolink
Length of output: 510
🏁 Script executed:
rg "numberOfImages" --type ts -B 2 -A 2Repository: juspay/neurolink
Length of output: 281
🏁 Script executed:
grep -n "numberOfImages\|maxImages" ./src/lib/image-gen/ImageGenService.tsRepository: juspay/neurolink
Length of output: 194
🏁 Script executed:
find . -type f -name "*.ts" -path "*/neurolink*" | xargs grep -l "generate.*numberOfImages\|numberOfImages.*generate" 2>/dev/null | head -5Repository: juspay/neurolink
Length of output: 42
🏁 Script executed:
grep -rn "neurolink.generate\|\.generate(" ./src/lib --include="*.ts" | grep -v "imageGen\|ImageGen" | head -20Repository: juspay/neurolink
Length of output: 1895
🏁 Script executed:
grep -n "generate.*Record<string, unknown>\|async generate(" ./src/lib/neurolink.ts | head -20Repository: juspay/neurolink
Length of output: 82
🏁 Script executed:
sed -n '1837,1950p' ./src/lib/neurolink.tsRepository: juspay/neurolink
Length of output: 4324
🏁 Script executed:
find . -type f -name "*.ts" -path "*image*" | xargs ls -laRepository: juspay/neurolink
Length of output: 922
numberOfImages / maxImages are ignored, and generate() lacks timeout protection.
ImageGenOptions.numberOfImages and ImageGenConfig.maxImages never flow into generateParams, so callers can't request multiple images and limits are unenforced. Additionally, the NeuroLink generate() call at line 206 should be wrapped with the withTimeout utility to enforce graceful timeout handling per coding guidelines.
🛠️ Suggested fixes
+ const maxImages = this.config.maxImages ?? 1;
+ const numberOfImages = Math.min(options.numberOfImages ?? 1, maxImages);
+
const generateParams: Record<string, unknown> = {
input,
provider,
model,
disableTools: true,
+ numberOfImages,
temperature:
options.temperature ?? this.config.defaultTemperature ?? 0.75,
timeout: this.config.timeout,
};
// Call NeuroLink generate
+ const { withTimeout } = await import("../utils/errorHandling.js");
- const result = await neurolink.generate(generateParams);
+ const result = await withTimeout(
+ neurolink.generate(generateParams),
+ this.config.timeout,
+ );Also replace the deprecated substr() at line 84 with substring():
- this.instanceId = `ImageGenService-${Date.now()}-${Math.random().toString(36).substr(2, 9)}`;
+ this.instanceId = `ImageGenService-${Date.now()}-${Math.random().toString(36).substring(2, 11)}`;📝 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.
| // Determine provider and model | |
| const provider = options.provider ?? this.config.defaultProvider; | |
| const model = options.model ?? this.config.defaultModel; | |
| const region = options.region ?? this.config.defaultRegion; | |
| // Build generation parameters | |
| const generateParams: Record<string, unknown> = { | |
| input, | |
| provider, | |
| model, | |
| disableTools: true, | |
| temperature: | |
| options.temperature ?? this.config.defaultTemperature ?? 0.75, | |
| timeout: this.config.timeout, | |
| }; | |
| // Add region if specified (for Vertex AI) | |
| if (region) { | |
| generateParams.region = region; | |
| } | |
| // Add aspect ratio if specified | |
| if (options.aspectRatio) { | |
| generateParams.aspectRatio = options.aspectRatio; | |
| } | |
| // Call NeuroLink generate | |
| const result = await neurolink.generate(generateParams); | |
| // Extract image from result | |
| const imageOutput = this.extractImageFromResult(result); | |
| const generationTimeMs = Date.now() - startTime; | |
| if (imageOutput) { | |
| return { | |
| success: true, | |
| imageBuffer: imageOutput.imageBuffer, | |
| base64: imageOutput.base64, | |
| mimeType: imageOutput.mimeType, | |
| model, | |
| provider, | |
| generationTimeMs, | |
| }; | |
| } | |
| return { | |
| success: false, | |
| error: "No image generated", | |
| model, | |
| provider, | |
| generationTimeMs, | |
| }; | |
| // Determine provider and model | |
| const provider = options.provider ?? this.config.defaultProvider; | |
| const model = options.model ?? this.config.defaultModel; | |
| const region = options.region ?? this.config.defaultRegion; | |
| const maxImages = this.config.maxImages ?? 1; | |
| const numberOfImages = Math.min(options.numberOfImages ?? 1, maxImages); | |
| // Build generation parameters | |
| const generateParams: Record<string, unknown> = { | |
| input, | |
| provider, | |
| model, | |
| disableTools: true, | |
| numberOfImages, | |
| temperature: | |
| options.temperature ?? this.config.defaultTemperature ?? 0.75, | |
| timeout: this.config.timeout, | |
| }; | |
| // Add region if specified (for Vertex AI) | |
| if (region) { | |
| generateParams.region = region; | |
| } | |
| // Add aspect ratio if specified | |
| if (options.aspectRatio) { | |
| generateParams.aspectRatio = options.aspectRatio; | |
| } | |
| // Call NeuroLink generate | |
| const { withTimeout } = await import("../utils/errorHandling.js"); | |
| const result = await withTimeout( | |
| neurolink.generate(generateParams), | |
| this.config.timeout, | |
| ); | |
| // Extract image from result | |
| const imageOutput = this.extractImageFromResult(result); | |
| const generationTimeMs = Date.now() - startTime; | |
| if (imageOutput) { | |
| return { | |
| success: true, | |
| imageBuffer: imageOutput.imageBuffer, | |
| base64: imageOutput.base64, | |
| mimeType: imageOutput.mimeType, | |
| model, | |
| provider, | |
| generationTimeMs, | |
| }; | |
| } | |
| return { | |
| success: false, | |
| error: "No image generated", | |
| model, | |
| provider, | |
| generationTimeMs, | |
| }; |
🤖 Prompt for AI Agents
In `@src/lib/image-gen/ImageGenService.ts` around lines 179 - 230, The
generateParams object in ImageGenService is missing image count controls and the
neurolink.generate call lacks timeout protection: include options.numberOfImages
(capped by this.config.maxImages) into generateParams (e.g., numberOfImages or
maxImages as the provider expects) so callers can request multiple images and
the config limit is enforced, and wrap the neurolink.generate(generateParams)
call with the withTimeout utility to enforce this.config.timeout (or
options.timeout) for graceful cancellation; after generation continue using
extractImageFromResult as before. Also replace the deprecated substr() usage
elsewhere in the same file with substring() to avoid runtime deprecation issues.
| private parseXmlSecurely(content: string): unknown { | ||
| // Dynamically import fast-xml-parser | ||
| const { XMLParser } = require("fast-xml-parser"); | ||
|
|
||
| // Initialize XML parser with sensible defaults | ||
| // XXE Protection: Disable entity processing to prevent XML External Entity attacks | ||
| const parser = new XMLParser({ | ||
| ignoreAttributes: false, | ||
| attributeNamePrefix: "@_", | ||
| textNodeName: "#text", | ||
| parseAttributeValue: true, | ||
| parseTagValue: true, | ||
| trimValues: true, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check the module system configuration
cat -n tsconfig.json | head -30Repository: juspay/neurolink
Length of output: 645
🏁 Script executed:
# Check package.json for module type declaration
cat -n package.json | head -30Repository: juspay/neurolink
Length of output: 1370
🏁 Script executed:
# Read the XmlProcessor.ts file to see current imports and module type
cat -n src/lib/processors/data/XmlProcessor.ts | head -180Repository: juspay/neurolink
Length of output: 6711
🏁 Script executed:
# Check if there are any existing createRequire uses in the codebase
rg "createRequire" -t tsRepository: juspay/neurolink
Length of output: 42
🏁 Script executed:
# Check for other require() calls in TypeScript files
rg 'require\(' src/lib -t ts | head -20Repository: juspay/neurolink
Length of output: 325
🏁 Script executed:
# Check if .svelte-kit/tsconfig.json exists
ls -la .svelte-kit/tsconfig.json 2>/dev/null || echo "File not found"
cat -n .svelte-kit/tsconfig.json 2>/dev/null | head -50Repository: juspay/neurolink
Length of output: 74
🏁 Script executed:
# Check for type: "module" in package.json
grep -n '"type"' package.jsonRepository: juspay/neurolink
Length of output: 134
🏁 Script executed:
# Look for build configuration in vite.config
find . -name "vite.config.*" -type f | head -5Repository: juspay/neurolink
Length of output: 76
🏁 Script executed:
# Check if there's a svelte.config.js
cat -n svelte.config.js 2>/dev/null | head -50Repository: juspay/neurolink
Length of output: 928
🏁 Script executed:
# Verify the fast-xml-parser package exports structure
cat -n node_modules/fast-xml-parser/package.json 2>/dev/null | head -30Repository: juspay/neurolink
Length of output: 42
🏁 Script executed:
# Check vite.config.ts for any special handling
cat -n vite.config.tsRepository: juspay/neurolink
Length of output: 3075
🏁 Script executed:
# Look for node_modules/fast-xml-parser to see what it exports
fd "fast-xml-parser" node_modules -type d | head -3Repository: juspay/neurolink
Length of output: 230
🏁 Script executed:
# Check if fast-xml-parser has ESM exports
cat node_modules/fast-xml-parser/package.json | jq '.exports' 2>/dev/null || cat node_modules/fast-xml-parser/package.json | grep -A 5 '"main"'Repository: juspay/neurolink
Length of output: 133
🏁 Script executed:
# Search for any dynamic imports in the codebase to see the pattern
rg "import\(" src/ -t ts | head -10Repository: juspay/neurolink
Length of output: 903
🏁 Script executed:
# Confirm the imports in other files also have the same pattern
cat -n src/lib/processors/document/OpenDocumentProcessor.ts | grep -A 2 -B 2 "require("Repository: juspay/neurolink
Length of output: 253
🏁 Script executed:
# Check if there are any existing patterns we should follow for dynamic imports
rg "from.*import" src/lib/processors -A 2 | head -30Repository: juspay/neurolink
Length of output: 42
Use createRequire for CommonJS modules in ESM context.
Line 160 uses require() which is undefined in ESM modules ("type": "module" in package.json). This will throw at runtime. Replace with createRequire() and add proper typing:
Fix
+import { createRequire } from "node:module";
import { BaseFileProcessor } from "../base/BaseFileProcessor.js";
+
+const require = createRequire(import.meta.url);Then at line 160:
- const { XMLParser } = require("fast-xml-parser");
+ const { XMLParser } = require("fast-xml-parser") as typeof import("fast-xml-parser");Note: Same issue exists in src/lib/processors/document/OpenDocumentProcessor.ts and src/lib/processors/data/YamlProcessor.ts.
🤖 Prompt for AI Agents
In `@src/lib/processors/data/XmlProcessor.ts` around lines 158 - 170, The
parseXmlSecurely function uses require() in ESM which will break at runtime;
replace the dynamic require with Node's createRequire by importing createRequire
from "module" and instantiating it with import.meta.url, then use that require
to load "fast-xml-parser" (so keep const { XMLParser } =
require("fast-xml-parser") but via createRequire). Ensure the import/typing is
added at the top of the module and that parseXmlSecurely continues to
instantiate XMLParser with the same options; apply the same createRequire fix in
OpenDocumentProcessor and YamlProcessor for their require() calls.
| try { | ||
| // Dynamically import adm-zip to avoid issues if not available | ||
| const AdmZip = require("adm-zip"); | ||
| const zip = new AdmZip(buffer); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check the file structure and content
cat -n src/lib/processors/document/OpenDocumentProcessor.ts | head -100Repository: juspay/neurolink
Length of output: 4066
🏁 Script executed:
# Check the package.json for module configuration
cat package.json | head -50Repository: juspay/neurolink
Length of output: 2499
🏁 Script executed:
# Check tsconfig.json for module resolution settings
cat tsconfig.jsonRepository: juspay/neurolink
Length of output: 526
🏁 Script executed:
# Check for "type": "module" in package.json
grep -n '"type"' package.jsonRepository: juspay/neurolink
Length of output: 134
🏁 Script executed:
# Check dependencies for adm-zip
grep -A 30 '"dependencies"' package.jsonRepository: juspay/neurolink
Length of output: 1276
🏁 Script executed:
# Check the .svelte-kit/tsconfig.json if it exists
cat .svelte-kit/tsconfig.json 2>/dev/null | head -50Repository: juspay/neurolink
Length of output: 42
🏁 Script executed:
# Also check if there's a tsconfig.cli.json for build configuration
cat tsconfig.cli.json 2>/dev/nullRepository: juspay/neurolink
Length of output: 563
Use createRequire for CommonJS interoperability in ESM context.
Line 82 uses bare require("adm-zip") in a file configured for ESM (package.json has "type": "module" and the project builds with "module": "NodeNext"). In ESM runtime, require is undefined and will throw a ReferenceError. Use createRequire from "node:module" to interoperate with the CommonJS module.
Fix
+import { createRequire } from "node:module";
import { BaseFileProcessor } from "../base/BaseFileProcessor.js";
import type { FileInfo, FileProcessingResult, ProcessedFileBase, ProcessOptions } from "../base/types.js";
import { SIZE_LIMITS } from "../config/index.js";
+
+const require = createRequire(import.meta.url);
@@
- const AdmZip = require("adm-zip");
+ const AdmZip = require("adm-zip") as typeof import("adm-zip");🤖 Prompt for AI Agents
In `@src/lib/processors/document/OpenDocumentProcessor.ts` around lines 80 - 83,
The code in OpenDocumentProcessor uses a bare require("adm-zip") inside the try
block which fails in ESM; replace it with Node's createRequire interoperability:
import createRequire from "node:module" (or get it via named import), construct
a require function with createRequire(import.meta.url), then call that require
to load "adm-zip" (assign to AdmZip) before creating zip from buffer; update the
try block where AdmZip and zip are created so it uses the createRequire-based
require instead of the bare require.
| export function generateErrorFingerprint( | ||
| error: Error, | ||
| context?: { operation?: string }, | ||
| ): string { | ||
| // Normalize message by replacing dynamic values | ||
| const normalizedMessage = normalizeErrorMessage(error.message); | ||
|
|
||
| // Get first relevant stack frame (most relevant location) | ||
| const firstFrame = extractFirstRelevantFrame(error.stack); | ||
|
|
||
| const components = [ | ||
| error.name, | ||
| normalizedMessage, | ||
| firstFrame, | ||
| context?.operation || "", | ||
| ]; | ||
|
|
||
| return createHash("md5") | ||
| .update(components.join("|")) | ||
| .digest("hex") | ||
| .substring(0, 16); | ||
| } | ||
|
|
||
| /** | ||
| * Generate a fingerprint from a plain string (for non-Error values). | ||
| * | ||
| * @param value - String value to fingerprint | ||
| * @returns 16-character hex fingerprint hash | ||
| */ | ||
| function generateFingerprintFromString(value: string): string { | ||
| const normalized = normalizeErrorMessage(value); | ||
| return createHash("md5").update(normalized).digest("hex").substring(0, 16); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
MD5 vs SHA-256 security weak crypto fingerprinting Node.js
💡 Result:
MD5 vs SHA-256 (security + “crypto fingerprinting”)
MD5 (128-bit) is cryptographically broken for collision resistance. Practical collision and chosen-prefix collision attacks exist, meaning an attacker can craft two different inputs that share the same MD5—making MD5 unsafe for “fingerprints” used as security identifiers (e.g., signing, tamper-detection against an active attacker, deduplication where an attacker can choose inputs). (rfc-editor.org)
SHA-256 (256-bit) is the modern baseline and is still considered secure in practice for collision resistance and general fingerprinting/integrity use (subject to normal engineering caveats). NIST recommends SHA-2 (including SHA-256) broadly and encourages at least SHA-256 for interoperability. (csrc.nist.gov)
What to use in Node.js
Simple fingerprint / content ID (non-secret):
import { createHash } from "node:crypto";
const fp = createHash("sha256").update(data).digest("hex");Node supports createHash() and SHA-256 as a digest algorithm. (nodejs.org)
Tamper-evident fingerprint (you share a secret): use HMAC-SHA-256
import { createHmac } from "node:crypto";
const mac = createHmac("sha256", secretKey).update(data).digest("hex");(Plain hashes don’t provide authenticity; HMAC does.) (nodejs.org)
Certificates / keys: prefer SHA-256 fingerprints (Node even exposes x509.fingerprint256). (nodejs.org)
Rule of thumb
- Avoid MD5 for anything security-relevant; collisions are the core problem. (rfc-editor.org)
- Use SHA-256 for general-purpose fingerprinting; use HMAC-SHA-256 when you need to detect malicious changes. (csrc.nist.gov)
Citations:
- 1: https://www.rfc-editor.org/rfc/rfc6151?utm_source=openai
- 2: https://csrc.nist.gov/Projects/Hash-Functions/NIST-Policy-on-Hash-Functions?utm_source=openai
- 3: https://nodejs.org/api/crypto.html?utm_source=openai
- 4: https://nodejs.org/api/crypto.html?utm_source=openai
- 5: https://nodejs.org/api/crypto.html?utm_source=openai
- 6: https://www.rfc-editor.org/rfc/rfc6151?utm_source=openai
- 7: https://csrc.nist.gov/Projects/Hash-Functions/NIST-Policy-on-Hash-Functions?utm_source=openai
🏁 Script executed:
# Locate the file and verify the code segment
find . -type f -name "errorSerializer.ts" | head -5Repository: juspay/neurolink
Length of output: 106
🏁 Script executed:
# Read the specific lines in question
sed -n '271,303p' ./src/lib/processors/errors/errorSerializer.ts
# Get broader context around those lines
sed -n '250,320p' ./src/lib/processors/errors/errorSerializer.tsRepository: juspay/neurolink
Length of output: 3203
🏁 Script executed:
# Search for MD5 usage in the file to see all instances
rg "md5" ./src/lib/processors/errors/errorSerializer.ts -i -A 2 -B 2Repository: juspay/neurolink
Length of output: 344
🏁 Script executed:
# Check for TypeScript type annotations and naming conventions in the functions
sed -n '271,303p' ./src/lib/processors/errors/errorSerializer.ts | cat -nRepository: juspay/neurolink
Length of output: 1225
Replace MD5 with SHA-256 to satisfy CodeQL and avoid weak crypto flagging.
MD5 is cryptographically broken with practical collision attacks documented in RFC 6151. Even for non-security fingerprints, NIST recommends SHA-256 as the modern baseline. Swap the algorithm while keeping the 16-character truncation to maintain fingerprint size consistency.
🔐 Proposed fix
- return createHash("md5")
+ return createHash("sha256")
.update(components.join("|"))
.digest("hex")
.substring(0, 16);- return createHash("md5").update(normalized).digest("hex").substring(0, 16);
+ return createHash("sha256").update(normalized).digest("hex").substring(0, 16);📝 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.
| export function generateErrorFingerprint( | |
| error: Error, | |
| context?: { operation?: string }, | |
| ): string { | |
| // Normalize message by replacing dynamic values | |
| const normalizedMessage = normalizeErrorMessage(error.message); | |
| // Get first relevant stack frame (most relevant location) | |
| const firstFrame = extractFirstRelevantFrame(error.stack); | |
| const components = [ | |
| error.name, | |
| normalizedMessage, | |
| firstFrame, | |
| context?.operation || "", | |
| ]; | |
| return createHash("md5") | |
| .update(components.join("|")) | |
| .digest("hex") | |
| .substring(0, 16); | |
| } | |
| /** | |
| * Generate a fingerprint from a plain string (for non-Error values). | |
| * | |
| * @param value - String value to fingerprint | |
| * @returns 16-character hex fingerprint hash | |
| */ | |
| function generateFingerprintFromString(value: string): string { | |
| const normalized = normalizeErrorMessage(value); | |
| return createHash("md5").update(normalized).digest("hex").substring(0, 16); | |
| } | |
| export function generateErrorFingerprint( | |
| error: Error, | |
| context?: { operation?: string }, | |
| ): string { | |
| // Normalize message by replacing dynamic values | |
| const normalizedMessage = normalizeErrorMessage(error.message); | |
| // Get first relevant stack frame (most relevant location) | |
| const firstFrame = extractFirstRelevantFrame(error.stack); | |
| const components = [ | |
| error.name, | |
| normalizedMessage, | |
| firstFrame, | |
| context?.operation || "", | |
| ]; | |
| return createHash("sha256") | |
| .update(components.join("|")) | |
| .digest("hex") | |
| .substring(0, 16); | |
| } | |
| /** | |
| * Generate a fingerprint from a plain string (for non-Error values). | |
| * | |
| * `@param` value - String value to fingerprint | |
| * `@returns` 16-character hex fingerprint hash | |
| */ | |
| function generateFingerprintFromString(value: string): string { | |
| const normalized = normalizeErrorMessage(value); | |
| return createHash("sha256").update(normalized).digest("hex").substring(0, 16); | |
| } |
🧰 Tools
🪛 GitHub Check: CodeQL
[failure] 289-289: Use of a broken or weak cryptographic algorithm
A broken or weak cryptographic algorithm depends on sensitive data from a call to replace.
[failure] 302-302: Use of a broken or weak cryptographic algorithm
A broken or weak cryptographic algorithm depends on sensitive data from a call to replace.
🤖 Prompt for AI Agents
In `@src/lib/processors/errors/errorSerializer.ts` around lines 271 - 303, The
code uses createHash("md5") in generateErrorFingerprint and
generateFingerprintFromString which triggers weak-crypto warnings; change the
algorithm string to "sha256" for both uses (keep the existing
.digest(...).substring(0,16) truncation to preserve fingerprint length) and
update any inline comment if present to reflect SHA-256 is used; ensure the
symbols touched are generateErrorFingerprint and generateFingerprintFromString
and replace both createHash("md5") calls with createHash("sha256").
| export async function processFileWithRegistry( | ||
| fileInfo: FileInfo, | ||
| options?: FileProcessingOptions, | ||
| ): Promise<{ | ||
| processorName: string | null; | ||
| result: FileProcessingResult<ProcessedFileBase> | null; | ||
| }> { | ||
| const registry = getProcessorRegistry(); | ||
|
|
||
| // Use preferred processor if specified | ||
| if (options?.preferredProcessor) { | ||
| const processor = registry.getProcessor(options.preferredProcessor); | ||
| if (processor) { | ||
| const result = await processor.processor.processFile(fileInfo, options); | ||
| return { processorName: options.preferredProcessor, result }; | ||
| } | ||
| } | ||
|
|
||
| // Auto-detect processor based on MIME type and filename | ||
| const match = registry.findProcessor(fileInfo.mimetype, fileInfo.name); | ||
| if (!match) { | ||
| return { processorName: null, result: null }; | ||
| } | ||
|
|
||
| const result = await match.processor.processFile(fileInfo, options); | ||
| return { processorName: match.name, result }; |
There was a problem hiding this comment.
Wrap registry processing in withTimeout for hung processors.
Lines 165-190 and 242-245 await processor work without any guard. This can stall batch processing if a processor hangs; please wrap per-file processing with the project’s withTimeout helper and treat timeouts as failures/skips (optionally honoring allowFallback). As per coding guidelines: Implement graceful provider fallback with withTimeout utility for async operations.
Also applies to: 242-245
🤖 Prompt for AI Agents
In `@src/lib/processors/integration/FileProcessorIntegration.ts` around lines 165
- 190, processFileWithRegistry currently awaits processor.processor.processFile
and match.processor.processFile directly, which can hang; wrap each per-file
async call with the project's withTimeout helper and treat a timeout as a
failed/skipped processing (return null result or fall back when
options.allowFallback is set). Specifically: when calling
registry.getProcessor(...).processor.processFile(...) and when calling
match.processor.processFile(...), invoke withTimeout(process.call, timeoutMs)
(use the project standard timeout value) and catch timeout/errors to return {
processorName: null, result: null } or trigger fallback logic honoring
options.allowFallback; ensure any thrown timeouts are handled and do not block
batch processing. Also apply the same withTimeout wrapping to the other awaited
processor call referenced (lines ~242-245).
| // Use preferred processor if specified | ||
| if (options?.preferredProcessor) { | ||
| const processor = registry.getProcessor(options.preferredProcessor); | ||
| if (processor) { | ||
| const result = await processor.processor.processFile(fileInfo, options); | ||
| return { processorName: options.preferredProcessor, result }; | ||
| } | ||
| } | ||
|
|
||
| // Auto-detect processor based on MIME type and filename | ||
| const match = registry.findProcessor(fileInfo.mimetype, fileInfo.name); | ||
| if (!match) { | ||
| return { processorName: null, result: null }; | ||
| } | ||
|
|
||
| const result = await match.processor.processFile(fileInfo, options); | ||
| return { processorName: match.name, result }; | ||
| } | ||
|
|
||
| // ============================================================================= | ||
| // BATCH FILE PROCESSING | ||
| // ============================================================================= | ||
|
|
||
| /** | ||
| * Process multiple files using the ProcessorRegistry. | ||
| * Files are processed sequentially and categorized by outcome. | ||
| * | ||
| * @param files - Array of file information objects | ||
| * @param options - Processing options (max files, auth headers, timeout) | ||
| * @returns Batch result with successful, failed, and skipped files | ||
| * | ||
| * @example | ||
| * ```typescript | ||
| * const files: FileInfo[] = [ | ||
| * { id: "1", name: "image.jpg", mimetype: "image/jpeg", size: 512000 }, | ||
| * { id: "2", name: "doc.pdf", mimetype: "application/pdf", size: 1024000 }, | ||
| * { id: "3", name: "unknown.xyz", mimetype: "application/octet-stream", size: 100 }, | ||
| * ]; | ||
| * | ||
| * const result = await processBatchWithRegistry(files, { | ||
| * maxFiles: 50, | ||
| * timeout: 60000, | ||
| * }); | ||
| * | ||
| * console.log(`Processed ${result.successful.length} files successfully`); | ||
| * console.log(`Failed: ${result.failed.length}`); | ||
| * console.log(`Skipped: ${result.skipped.length}`); | ||
| * | ||
| * // Access individual results | ||
| * for (const { fileInfo, processorName, result } of result.successful) { | ||
| * console.log(`${fileInfo.name}: ${result.data?.size} bytes`); | ||
| * } | ||
| * ``` | ||
| */ | ||
| export async function processBatchWithRegistry( | ||
| files: FileInfo[], | ||
| options?: FileProcessingOptions, | ||
| ): Promise<BatchFileProcessingResult> { | ||
| const result: BatchFileProcessingResult = { | ||
| successful: [], | ||
| failed: [], | ||
| skipped: [], | ||
| }; | ||
|
|
||
| const maxFiles = options?.maxFiles ?? 100; | ||
| const filesToProcess = files.slice(0, maxFiles); | ||
|
|
||
| // Process files sequentially | ||
| for (const fileInfo of filesToProcess) { | ||
| try { | ||
| const { processorName, result: processResult } = | ||
| await processFileWithRegistry(fileInfo, options); | ||
|
|
||
| if (!processorName || !processResult) { | ||
| // No processor found for this file type | ||
| if (options?.allowFallback) { | ||
| result.skipped.push({ | ||
| fileInfo, | ||
| reason: "No processor found and fallback disabled", | ||
| }); | ||
| } else { | ||
| result.skipped.push({ | ||
| fileInfo, | ||
| reason: `No processor found for MIME type: ${fileInfo.mimetype}`, | ||
| }); | ||
| } |
There was a problem hiding this comment.
allowFallback is documented but not implemented (and the reason is inverted).
Lines 175-186 ignore allowFallback, and Lines 247-259 emit “fallback disabled” when allowFallback is true. Either implement real fallback behavior (e.g., route to a default processor) or remove the option/docs; at minimum fix the inverted message.
🛠️ Proposed fix (message correction)
- if (options?.allowFallback) {
- result.skipped.push({
- fileInfo,
- reason: "No processor found and fallback disabled",
- });
- } else {
- result.skipped.push({
- fileInfo,
- reason: `No processor found for MIME type: ${fileInfo.mimetype}`,
- });
- }
+ if (options?.allowFallback) {
+ result.skipped.push({
+ fileInfo,
+ reason: "No processor found; fallback enabled",
+ });
+ } else {
+ result.skipped.push({
+ fileInfo,
+ reason: `No processor found for MIME type: ${fileInfo.mimetype}`,
+ });
+ }📝 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.
| // Use preferred processor if specified | |
| if (options?.preferredProcessor) { | |
| const processor = registry.getProcessor(options.preferredProcessor); | |
| if (processor) { | |
| const result = await processor.processor.processFile(fileInfo, options); | |
| return { processorName: options.preferredProcessor, result }; | |
| } | |
| } | |
| // Auto-detect processor based on MIME type and filename | |
| const match = registry.findProcessor(fileInfo.mimetype, fileInfo.name); | |
| if (!match) { | |
| return { processorName: null, result: null }; | |
| } | |
| const result = await match.processor.processFile(fileInfo, options); | |
| return { processorName: match.name, result }; | |
| } | |
| // ============================================================================= | |
| // BATCH FILE PROCESSING | |
| // ============================================================================= | |
| /** | |
| * Process multiple files using the ProcessorRegistry. | |
| * Files are processed sequentially and categorized by outcome. | |
| * | |
| * @param files - Array of file information objects | |
| * @param options - Processing options (max files, auth headers, timeout) | |
| * @returns Batch result with successful, failed, and skipped files | |
| * | |
| * @example | |
| * ```typescript | |
| * const files: FileInfo[] = [ | |
| * { id: "1", name: "image.jpg", mimetype: "image/jpeg", size: 512000 }, | |
| * { id: "2", name: "doc.pdf", mimetype: "application/pdf", size: 1024000 }, | |
| * { id: "3", name: "unknown.xyz", mimetype: "application/octet-stream", size: 100 }, | |
| * ]; | |
| * | |
| * const result = await processBatchWithRegistry(files, { | |
| * maxFiles: 50, | |
| * timeout: 60000, | |
| * }); | |
| * | |
| * console.log(`Processed ${result.successful.length} files successfully`); | |
| * console.log(`Failed: ${result.failed.length}`); | |
| * console.log(`Skipped: ${result.skipped.length}`); | |
| * | |
| * // Access individual results | |
| * for (const { fileInfo, processorName, result } of result.successful) { | |
| * console.log(`${fileInfo.name}: ${result.data?.size} bytes`); | |
| * } | |
| * ``` | |
| */ | |
| export async function processBatchWithRegistry( | |
| files: FileInfo[], | |
| options?: FileProcessingOptions, | |
| ): Promise<BatchFileProcessingResult> { | |
| const result: BatchFileProcessingResult = { | |
| successful: [], | |
| failed: [], | |
| skipped: [], | |
| }; | |
| const maxFiles = options?.maxFiles ?? 100; | |
| const filesToProcess = files.slice(0, maxFiles); | |
| // Process files sequentially | |
| for (const fileInfo of filesToProcess) { | |
| try { | |
| const { processorName, result: processResult } = | |
| await processFileWithRegistry(fileInfo, options); | |
| if (!processorName || !processResult) { | |
| // No processor found for this file type | |
| if (options?.allowFallback) { | |
| result.skipped.push({ | |
| fileInfo, | |
| reason: "No processor found and fallback disabled", | |
| }); | |
| } else { | |
| result.skipped.push({ | |
| fileInfo, | |
| reason: `No processor found for MIME type: ${fileInfo.mimetype}`, | |
| }); | |
| } | |
| if (options?.allowFallback) { | |
| result.skipped.push({ | |
| fileInfo, | |
| reason: "No processor found; fallback enabled", | |
| }); | |
| } else { | |
| result.skipped.push({ | |
| fileInfo, | |
| reason: `No processor found for MIME type: ${fileInfo.mimetype}`, | |
| }); | |
| } |
🤖 Prompt for AI Agents
In `@src/lib/processors/integration/FileProcessorIntegration.ts` around lines 174
- 259, The batch processor processBatchWithRegistry incorrectly treats
options.allowFallback (it’s unused for processing and the skipped reason is
inverted); update processBatchWithRegistry to either route files with no matched
processor to a default/fallback processor via processFileWithRegistry (or
registry.getProcessor('default') / registry.findProcessor fallback) when
options.allowFallback is true, or if you prefer to keep no fallback simply
invert the message and remove the unused flag; specifically, modify the
no-processor branch where result.skipped is pushed (and any earlier code that
ignores options.allowFallback) so that when options.allowFallback is true you
call the fallback processor and push to successful/failed based on its result,
otherwise push a skipped entry with the corrected reason string referencing
fileInfo.mimetype.
| // Apply security sanitization using allowlist-based approach | ||
| let textContent: string; | ||
| try { | ||
| textContent = sanitizeSvgContent(rawContent); | ||
| } catch { | ||
| // If sanitization fails (e.g., malformed XML with XXE), use fallback | ||
| // Basic fallback: strip script tags and event handlers | ||
| textContent = rawContent | ||
| .replace(/<script[\s\S]*?<\/script\s*>/gi, "") | ||
| .replace(/\s+on[a-z]+\s*=\s*(?:"[^"]*"|'[^']*'|[^\s>]+)/gi, ""); | ||
| } |
There was a problem hiding this comment.
Fail closed when SVG sanitization throws.
The regex fallback is incomplete and can leave executable content in the output. It’s safer to fail closed (or strip all content) when sanitizeSvgContent throws, rather than partially sanitizing.
🔒 Safer fallback (fail closed)
- // Apply security sanitization using allowlist-based approach
- let textContent: string;
- try {
- textContent = sanitizeSvgContent(rawContent);
- } catch {
- // If sanitization fails (e.g., malformed XML with XXE), use fallback
- // Basic fallback: strip script tags and event handlers
- textContent = rawContent
- .replace(/<script[\s\S]*?<\/script\s*>/gi, "")
- .replace(/\s+on[a-z]+\s*=\s*(?:"[^"]*"|'[^']*'|[^\s>]+)/gi, "");
- }
-
- // Build security warnings
- const securityWarnings: string[] = [];
+ // Build security warnings
+ const securityWarnings: string[] = [];
+
+ // Apply security sanitization using allowlist-based approach
+ let textContent: string;
+ try {
+ textContent = sanitizeSvgContent(rawContent);
+ } catch {
+ // Fail closed: drop content on sanitizer failure to avoid partial sanitization
+ securityWarnings.push("SVG sanitization failed; content was removed.");
+ textContent = '<svg xmlns="http://www.w3.org/2000/svg"></svg>';
+ }📝 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.
| // Apply security sanitization using allowlist-based approach | |
| let textContent: string; | |
| try { | |
| textContent = sanitizeSvgContent(rawContent); | |
| } catch { | |
| // If sanitization fails (e.g., malformed XML with XXE), use fallback | |
| // Basic fallback: strip script tags and event handlers | |
| textContent = rawContent | |
| .replace(/<script[\s\S]*?<\/script\s*>/gi, "") | |
| .replace(/\s+on[a-z]+\s*=\s*(?:"[^"]*"|'[^']*'|[^\s>]+)/gi, ""); | |
| } | |
| // Build security warnings | |
| const securityWarnings: string[] = []; | |
| // Apply security sanitization using allowlist-based approach | |
| let textContent: string; | |
| try { | |
| textContent = sanitizeSvgContent(rawContent); | |
| } catch { | |
| // Fail closed: drop content on sanitizer failure to avoid partial sanitization | |
| securityWarnings.push("SVG sanitization failed; content was removed."); | |
| textContent = '<svg xmlns="http://www.w3.org/2000/svg"></svg>'; | |
| } |
🧰 Tools
🪛 GitHub Check: CodeQL
[failure] 176-178: Incomplete multi-character sanitization
This string may still contain on, which may cause an HTML attribute injection vulnerability.
[failure] 176-177: Incomplete multi-character sanitization
This string may still contain <script, which may cause an HTML element injection vulnerability.
[failure] 177-177: Bad HTML filtering regexp
This regular expression does not match script end tags like </script\t\n bar>.
🤖 Prompt for AI Agents
In `@src/lib/processors/markup/SvgProcessor.ts` around lines 169 - 179, When
sanitizeSvgContent(rawContent) throws inside SvgProcessor.ts, do not use the
partial regex fallback; instead "fail closed" by returning or assigning a safe
empty output (e.g., set textContent = "" or return an empty safe SVG) so no
executable content can leak. Locate the try/catch around sanitizeSvgContent in
SvgProcessor (variable textContent and rawContent) and replace the catch block
with logic that strips all content or returns a known-safe sanitized string
immediately rather than applying the current regex replacements.
| try { | ||
| const result = await match.processor.processFile(fileInfo, options); | ||
|
|
||
| if (result.success && result.data) { | ||
| return { type: match.name, data: result.data }; | ||
| } else { | ||
| return { | ||
| type: "unsupported", | ||
| data: null, | ||
| error: { | ||
| code: "NO_PROCESSOR_FOUND", | ||
| message: `Failed to process "${fileInfo.name || "file"}": ${result.error?.message || "Processor returned no data."}`, | ||
| filename: fileInfo.name || "unknown", | ||
| mimetype: fileInfo.mimetype || "unknown", | ||
| suggestion: | ||
| "The file may be corrupted or in an unexpected format. Try re-uploading or converting to a standard format.", | ||
| supportedTypes: this.getSupportedTypes(), | ||
| }, | ||
| }; | ||
| } | ||
| } catch (error) { | ||
| return { | ||
| type: "unsupported", | ||
| data: null, | ||
| error: { | ||
| code: "NO_PROCESSOR_FOUND", | ||
| message: `Failed to process "${fileInfo.name || "file"}": ${error instanceof Error ? error.message : "Unknown error"}`, | ||
| filename: fileInfo.name || "unknown", | ||
| mimetype: fileInfo.mimetype || "unknown", | ||
| suggestion: "Please check if the file is valid and not corrupted.", | ||
| supportedTypes: this.getSupportedTypes(), | ||
| }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Return a processing error code when a processor exists but fails.
processWithResult reports NO_PROCESSOR_FOUND and type: "unsupported" even when a processor was found but failed, which misleads callers and hides real processing failures.
🛠️ Suggested fix
- return {
- type: "unsupported",
+ return {
+ type: match.name,
data: null,
error: {
- code: "NO_PROCESSOR_FOUND",
+ code: "PROCESSING_FAILED",
message: `Failed to process "${fileInfo.name || "file"}": ${result.error?.message || "Processor returned no data."}`,
filename: fileInfo.name || "unknown",
mimetype: fileInfo.mimetype || "unknown",
suggestion:
"The file may be corrupted or in an unexpected format. Try re-uploading or converting to a standard format.",
supportedTypes: this.getSupportedTypes(),
},
};
}
} catch (error) {
return {
- type: "unsupported",
+ type: match.name,
data: null,
error: {
- code: "NO_PROCESSOR_FOUND",
+ code: "PROCESSING_FAILED",
message: `Failed to process "${fileInfo.name || "file"}": ${error instanceof Error ? error.message : "Unknown error"}`,
filename: fileInfo.name || "unknown",
mimetype: fileInfo.mimetype || "unknown",
suggestion: "Please check if the file is valid and not corrupted.",
supportedTypes: this.getSupportedTypes(),
},
};
}🤖 Prompt for AI Agents
In `@src/lib/processors/registry/ProcessorRegistry.ts` around lines 484 - 517, The
current processWithResult block treats any processor failure as
NO_PROCESSOR_FOUND and returns type "unsupported"; update both the non-success
branch (where result.success is false) and the catch block so that if a match
and match.processor exist but processing failed you return type: match.name (so
callers know which processor was used) and set error.code to "PROCESSOR_FAILED"
(instead of "NO_PROCESSOR_FOUND"), preserving the existing error.message,
filename, mimetype, suggestion and supportedTypes; also consider adding a small
field like error.processor = match.name to both error objects to make the
failing processor explicit.
There was a problem hiding this comment.
Actionable comments posted: 10
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🤖 Fix all issues with AI agents
In `@src/lib/image-gen/ImageGenService.ts`:
- Around line 205-209: Replace the direct await
neurolink.generate(generateParams) call in ImageGenService with the withTimeout
utility so the provider call is bounded; specifically, wrap
neurolink.generate(generateParams) with withTimeout using this.config.timeout
and a clear timeout message (e.g., "Image generation timed out after
${this.config.timeout}ms"), then pass the awaited result into the existing
extractImageFromResult(result) flow to preserve behavior and enable graceful
fallback.
In `@src/lib/processors/config/fileTypes.ts`:
- Around line 283-401: SOURCE_CODE_EXTENSIONS is missing several declared
language variants so some source files (e.g., PHP3/4/5, PHP secure, Perl
pods/tests, Common Lisp, Fortran 2003, .stylus, and others) won’t be detected;
update the SOURCE_CODE_EXTENSIONS constant to include the missing extensions
such as .php3, .php4, .php5, .phps, .pod, .t, .cl, .f03, .stylus (and any other
per-language variants noted in your language arrays), or refactor to derive
SOURCE_CODE_EXTENSIONS programmatically from the per-language extension arrays
to keep them in sync (make changes where SOURCE_CODE_EXTENSIONS is declared to
either add the entries or replace the literal array with a computed union).
In `@src/lib/processors/data/XmlProcessor.ts`:
- Around line 142-149: The checkXxeVectors method currently uses case-sensitive
string.includes checks and misses lowercased XXE markers; update checkXxeVectors
to perform case-insensitive detection by normalizing the input (e.g.,
content.toLowerCase()) or using case-insensitive regex (e.g., /<!doctype/i,
/<!entity/i) and return hasDOCTYPE/hasENTITY based on those checks so variations
like "<!doctype>" or mixed case are caught by the function.
In `@src/lib/processors/document/OpenDocumentProcessor.ts`:
- Around line 72-117: The buildProcessedResult method currently swallows
extraction errors and writes error text into textContent; instead, modify
buildProcessedResult (used by buildProcessedResultWithResult) to throw on any
extraction failure or when content.xml is missing (e.g., replace setting
textContent = "[Error...]" with throwing a descriptive Error) and rethrow the
caught error in the catch block rather than returning an error string; also
ensure the returned filename uses this.getFilename(fileInfo) consistently
(replace any direct filename usage with this.getFilename(fileInfo)) so callers
receive proper exceptions and consistent metadata.
In `@src/lib/processors/document/RtfProcessor.ts`:
- Around line 219-226: The bug is that skipGroup is only cleared when depth <=
0, which lets nested groups keep skipGroup true and incorrectly skip subsequent
content; modify the RtfProcessor parsing logic (where skipGroup and depth are
managed) to record the depth at which a skip group was entered (e.g., set
skipGroupStartDepth when you set skipGroup = true) and then clear skipGroup when
the current depth drops below that recorded skipGroupStartDepth in the '}'
handling code; update the relevant symbols (skipGroup, depth and introduce
skipGroupStartDepth or skipDepth) so the parser only skips until the matching
closing brace for the group that triggered skipping.
In `@src/lib/processors/markup/SvgProcessor.ts`:
- Around line 162-189: The catch block in SvgProcessor.buildProcessedResult that
falls back to regex-based stripping after sanitizeSvgContent fails is
unsafe—replace the fallback with a fail-closed behavior: when sanitizeSvgContent
throws, propagate a processing error (throw a specific Error or return a
failure/unsafe flag in the ProcessedSvg) so the caller can reject the file
instead of returning partially cleaned markup; remove the iterative
regex/script-attribute cleanup in the catch, and ensure callers of
buildProcessedResult (and any code that consumes ProcessedSvg) handle the new
error/unsafe result path appropriately; keep references to isSvgContentSafe for
pre-check logging but do not use it to bypass failure.
In `@src/lib/processors/registry/ProcessorRegistry.ts`:
- Around line 178-219: The register method currently lets allowDuplicates
silently replace the existing entry because a Map cannot have duplicate keys; to
fix this, perform the existing-registration check before running registration
validation and if this.processors.has(normalizedName) and
options?.allowDuplicates is true, simply return early (do not set the map or
touch aliases) so duplicates are ignored; keep the existing logic that calls
removeAliasesForProcessor(normalizedName) only when options?.overwriteExisting
is true, and only proceed to validate and set
this.processors.set(normalizedName, ...) and register registration.aliases when
you are actually going to overwrite or insert.
- Around line 463-517: processWithResult currently returns error.code
"NO_PROCESSOR_FOUND" for both missing processors and processor failures; update
it so missing-processor returns "NO_PROCESSOR_FOUND" but failures from
match.processor.processFile (both when result.success is false and in the catch
block) return a distinct code like "PROCESSING_ERROR" and include the original
processor error details (use result.error?.message and result.error?.code where
available, and error.message/error.stack for thrown errors) in the returned
error object (e.g., message, originalError or cause) while preserving filename,
mimetype, suggestion and supportedTypes; change the branches that build the
error object after calling match.processor.processFile and in the catch to use
"PROCESSING_ERROR" and attach the processor's error info so downstream code can
differentiate root causes.
In `@src/lib/utils/fileDetector.ts`:
- Around line 621-687: The SVG handler processSvgAsText currently falls back to
returning raw SVG on processor failure, which reintroduces XSS risk; update
processSvgAsText (and its result-handling branches) to fail closed by surfacing
an error instead of returning unsanitized markup: when processSvg returns
success=false or when the dynamic import/processing throws, log the error with
context (use detection.metadata.filename, detection.extension) and then throw a
descriptive error (or return a standardized failure FileProcessingResult/error
object) so callers cannot receive raw SVG content; adjust callers of
processSvgAsText accordingly to handle the thrown error/path.
🟡 Minor comments (14)
docs/migration/CURATOR_MIGRATION_IMPLEMENTATION_PLANS.md-5-6 (1)
5-6:⚠️ Potential issue | 🟡 MinorAvoid absolute local paths in docs.
Use repo-relative or placeholder paths so the doc is portable and not tied to a single machine.
docs/migration/CURATOR_MIGRATION_IMPLEMENTATION_PLANS.md-279-279 (1)
279-279:⚠️ Potential issue | 🟡 MinorFix MD031 fenced block spacing.
markdownlint reports missing blank lines around fenced code blocks at these locations; add a blank line before and after each fence.
Also applies to: 311-311, 726-726, 1275-1275, 1422-1422, 1604-1604
src/lib/utils/json/extract.ts-50-67 (1)
50-67:⚠️ Potential issue | 🟡 MinorGreedy JSON match can miss valid JSON.
/\{[\s\S]*\}/and/\[[\s\S]*\]/will swallow multiple blocks (e.g., two JSON objects), causing parse failure even when valid JSON exists. Consider a non-greedy or iterative scan.Suggested patch
- // Try to find JSON object pattern - const objectMatch = text.match(/\{[\s\S]*\}/); - if (objectMatch) { - try { - JSON.parse(objectMatch[0]); - return objectMatch[0]; - } catch { - // Continue to array pattern - } - } - - // Try to find JSON array pattern - const arrayMatch = text.match(/\[[\s\S]*\]/); - if (arrayMatch) { - try { - JSON.parse(arrayMatch[0]); - return arrayMatch[0]; - } catch { - // No valid JSON found - } - } + // Try to find JSON object/array patterns (non-greedy, first parseable wins) + const candidateRegex = /(\{[\s\S]*?\}|\[[\s\S]*?\])/g; + let candidate: RegExpExecArray | null; + while ((candidate = candidateRegex.exec(text)) !== null) { + const snippet = candidate[1]; + try { + JSON.parse(snippet); + return snippet; + } catch { + // Try next candidate + } + }src/lib/processors/errors/errorHelpers.ts-485-506 (1)
485-506:⚠️ Potential issue | 🟡 MinorHandle string
Retry-Afterand clamp non‑positive attempts.
Some HTTP clients surfaceretryAfteras a string (orretry-after), andattempt <= 0currently yields sub‑base delays. Parsing the header and clamping attempts avoids overly aggressive retries.🛠️ Suggested fix
export function getRetryDelay( error: unknown, attempt: number, baseDelayMs: number = 1000, ): number { // Check for rate limit with Retry-After header if (typeof error === "object" && error !== null) { const errorObj = error as Record<string, unknown>; - if (typeof errorObj.retryAfter === "number") { - return errorObj.retryAfter * 1000; - } + const retryAfter = (errorObj.retryAfter ?? errorObj["retry-after"]) as unknown; + if (typeof retryAfter === "number") { + return retryAfter * 1000; + } + if (typeof retryAfter === "string") { + const parsed = Number(retryAfter); + if (!Number.isNaN(parsed)) { + return parsed * 1000; + } + } } // Exponential backoff: base * 2^(attempt-1) - const exponentialDelay = baseDelayMs * 2 ** (attempt - 1); + const safeAttempt = Math.max(1, attempt); + const exponentialDelay = baseDelayMs * 2 ** (safeAttempt - 1);docs/features/file-processors.md-293-317 (1)
293-317:⚠️ Potential issue | 🟡 MinorFix error-handling example (
instanceof+ enum values).
FileProcessingErroris a TypeScript interface, soinstanceofwon't work. Use theisFileProcessingError()type guard instead. Additionally, the sample enum members are incorrect:
SIZE_EXCEEDED→FILE_TOO_LARGECORRUPTED→CORRUPTED_FILEPERMISSION_DENIED→DOWNLOAD_AUTH_FAILED🛠️ Suggested doc update
-import { FileErrorCode, FileProcessingError } from "@juspay/neurolink"; +import { FileErrorCode, isFileProcessingError } from "@juspay/neurolink"; try { const result = await neurolink.generate({ input: { files: ["./corrupted.xlsx"] }, }); } catch (error) { - if (error instanceof FileProcessingError) { + if (isFileProcessingError(error)) { switch (error.code) { case FileErrorCode.UNSUPPORTED_TYPE: console.log("File type not supported"); break; - case FileErrorCode.SIZE_EXCEEDED: + case FileErrorCode.FILE_TOO_LARGE: console.log("File too large"); break; - case FileErrorCode.CORRUPTED: + case FileErrorCode.CORRUPTED_FILE: console.log("File is corrupted"); break; - case FileErrorCode.PERMISSION_DENIED: + case FileErrorCode.DOWNLOAD_AUTH_FAILED: console.log("Cannot read file"); break; } } }docs/migration/CURATOR_MIGRATION_VERIFICATION.md-700-719 (1)
700-719:⚠️ Potential issue | 🟡 MinorHyphenate compound modifiers in risk headings.
Headings like “High Risk Items” read as a compound modifier and should be hyphenated.
✍️ Suggested edit
-### 8.1 High Risk Items +### 8.1 High-Risk Items @@ -### 8.2 Medium Risk Items +### 8.2 Medium-Risk Items @@ -### 8.3 Low Risk Items +### 8.3 Low-Risk ItemsREADME.md-248-255 (1)
248-255:⚠️ Potential issue | 🟡 MinorAlign the file-type count with the earlier “50+” callout.
Line 248 says “17+ file types,” while the “What’s New” section highlights “50+ file types.” Consider clarifying that 17+ is the number of categories and 50+ is the total count (including code languages).
✏️ Proposed wording tweak
-**17+ file types supported** with intelligent content extraction and provider-agnostic processing: +**17+ file categories supported** (50+ total file types including code languages) with intelligent content extraction and provider-agnostic processing:docs/migration/CURATOR_MIGRATION_ANALYSIS_REPORT.md-367-367 (1)
367-367:⚠️ Potential issue | 🟡 MinorPipeline failure: Unescaped
<T>generic causes MDX compilation error.The
<T>inToolExecutionResult<T>is being parsed as a JSX/HTML tag by MDX, causing the build to fail. In Markdown tables (outside code blocks), angle brackets need escaping.🔧 Proposed fix
-| **Tool Execution** | ToolResult | ToolExecutionResult<T> | ⚠️ Wrapper needed | +| **Tool Execution** | ToolResult | `ToolExecutionResult<T>` | ⚠️ Wrapper needed |src/lib/processors/data/YamlProcessor.ts-184-192 (1)
184-192:⚠️ Potential issue | 🟡 MinorUpdate misleading security documentation to reflect actual implementation.
The file's header (line 18) and class documentation (line 122) claim that
maxAliasCountis limited to 100, butjs-yamldoesn't support this option and the code never passes it toyaml.load(). The inline comment at line 190 correctly notes this limitation. Update the header and class docs to accurately reflect that protection against billion laughs attacks relies onCORE_SCHEMA+ file size limits, notmaxAliasCount.Additionally, the method uses
require("js-yaml")while the rest of the module uses ESM imports with.jsextensions. For consistency, use dynamicimport("js-yaml")instead. SinceparseYamlSecurely()is called synchronously at lines 230 and 267, keep the method synchronous but update the internal import approach.src/lib/processors/code/SourceCodeProcessor.ts-214-239 (1)
214-239:⚠️ Potential issue | 🟡 MinorHandle Windows path separators when checking exact filenames.
split("/")misses\, soC:\path\Dockerfilewon’t match. Split on both separators to keep cross-platform support.🔧 Suggested fix
- const basename = filename.split("/").pop() || filename; + const basename = filename.split(/[/\\]/).pop() || filename;src/lib/processors/config/languageMap.ts-368-406 (1)
368-406:⚠️ Potential issue | 🟡 MinorHandle Windows-style paths when resolving basenames.
split("/")misses\, soC:\path\Dockerfilewon’t match exact filename entries. Split on both separators to keep cross-platform support.🔧 Suggested fix
- const basename = filename.split("/").pop() || filename; + const basename = filename.split(/[/\\]/).pop() || filename;src/lib/processors/document/OpenDocumentProcessor.ts-122-185 (1)
122-185:⚠️ Potential issue | 🟡 MinorAvoid double-unescaping XML entities.
Decoding
&first collapses double-escaped sequences (e.g.,&lt;becomes<) and can lose literal<text. Decode&last (or use a single-pass decoder) in both paths.🔧 Suggested fix (ordering)
- const text = stripped - .replace(/&/g, "&") - .replace(/</g, "<") - .replace(/>/g, ">") - .replace(/"/g, '"') - .replace(/'/g, "'") + const text = stripped + .replace(/</g, "<") + .replace(/>/g, ">") + .replace(/"/g, '"') + .replace(/'/g, "'") + .replace(/&/g, "&") ... - const simpleText = stripped - .replace(/\s+/g, " ") - .replace(/&/g, "&") - .replace(/</g, "<") - .replace(/>/g, ">") - .replace(/"/g, '"') - .replace(/'/g, "'") + const simpleText = stripped + .replace(/\s+/g, " ") + .replace(/</g, "<") + .replace(/>/g, ">") + .replace(/"/g, '"') + .replace(/'/g, "'") + .replace(/&/g, "&")src/lib/processors/cli/fileProcessorCli.ts-267-269 (1)
267-269:⚠️ Potential issue | 🟡 MinorReplace
console.infowithlogger.infoper pipeline requirements.The CI pipeline flagged these lines for using
console.infoin production code. Use the project's logger utility instead.🐛 Proposed fix
Add import at the top:
import { logger } from "../../utils/logger.js";Then replace console.info calls:
if (options?.verbose) { - console.info(`Processing: ${fileInfo.name}`); - console.info(` Size: ${fileInfo.size} bytes`); - console.info(` MIME: ${fileInfo.mimetype}`); + logger.info(`Processing: ${fileInfo.name}`); + logger.info(` Size: ${fileInfo.size} bytes`); + logger.info(` MIME: ${fileInfo.mimetype}`); }if (options?.verbose) { - console.info(` Processor: ${options.processor}`); + logger.info(` Processor: ${options.processor}`); }if (options?.verbose) { - console.info(` Processor: ${match.name}`); - console.info(` Confidence: ${match.confidence}%`); + logger.info(` Processor: ${match.name}`); + logger.info(` Confidence: ${match.confidence}%`); }Also applies to: 288-290, 321-324
src/lib/processors/integration/FileProcessorIntegration.ts-247-260 (1)
247-260:⚠️ Potential issue | 🟡 MinorLogic appears inverted for
allowFallbackhandling.When
allowFallbackistrue, the message says "fallback disabled", which is contradictory. The condition logic seems reversed.🐛 Proposed fix
if (!processorName || !processResult) { // No processor found for this file type - if (options?.allowFallback) { - result.skipped.push({ - fileInfo, - reason: "No processor found and fallback disabled", - }); - } else { + if (!options?.allowFallback) { result.skipped.push({ fileInfo, reason: `No processor found for MIME type: ${fileInfo.mimetype}`, }); + } else { + // allowFallback is true - could implement fallback processing here + result.skipped.push({ + fileInfo, + reason: "No processor found (fallback not yet implemented)", + }); } continue; }
🧹 Nitpick comments (26)
src/lib/utils/async/retry.ts (1)
12-215: Add optional timeout wrapping to prevent hung retries.If
fn()stalls indefinitely, retries never progress. Consider an optionaltimeoutMsinRetryOptionsand wrap attempts withwithTimeoutto keep retries and provider fallback responsive.Suggested patch
-import { delay } from "./delay.js"; +import { delay } from "./delay.js"; +import { withTimeout } from "./withTimeout.js"; @@ export interface RetryOptions { @@ onRetry?: (error: Error, attempt: number, delayMs: number) => void; + /** Optional timeout per attempt (ms). */ + timeoutMs?: number; } @@ const { maxRetries, baseDelayMs, maxDelayMs, backoffMultiplier = 2, shouldRetry = () => true, onRetry, + timeoutMs, } = config; @@ try { - return await fn(); + const attemptPromise = fn(); + return timeoutMs + ? await withTimeout(attemptPromise, timeoutMs, `Retry attempt ${attempt} timed out`) + : await attemptPromise; } catch (error) {Based on learnings: Applies to src/lib/**/*.ts : Implement graceful provider fallback with withTimeout utility for async operations.
src/lib/utils/json/extract.ts (1)
105-108: Move exported JsonTypeGuard to shared types.Since this is a reusable exported type, consider placing it under
src/lib/types/and re-exporting throughsrc/lib/types/index.tsto align with the project’s type organization standard.Based on learnings: Project standard: Place reusable/shared types under src/lib/types/.ts; test-only helper types under test/types/.ts; avoid declaring local types inside source implementation files. Also: Export types and interfaces from src/lib/types/index.ts as the main type definitions file.
src/lib/processors/config/sizeLimits.ts (1)
306-316: Relocate exported type aliases to shared types.
SizeLimitMBKey,SizeLimitBytesKey,ProcessingLimitKey, andSizeLimitsare exported and likely reusable; consider moving them tosrc/lib/types/and re-export viasrc/lib/types/index.ts.Based on learnings: Project standard: Place reusable/shared types under src/lib/types/.ts; test-only helper types under test/types/.ts; avoid declaring local types inside source implementation files.
src/lib/types/processorTypes.ts (1)
87-137: UseFileProcessorErrorCodeenum and discriminated unions to enforce type safety.
ProcessorFileError.codeis currently astring, and the result interfaces allow invalid states like{ success: true, error }or{ success: false, data }. UsingFileProcessorErrorCodeand discriminated unions prevents these invalid combinations and matches the type-safe pattern already in use elsewhere in the codebase (e.g.,src/lib/server/utils/validation.ts).♻️ Suggested refactor
export interface ProcessorFileError { /** Error code for programmatic handling */ - code: string; + code: FileProcessorErrorCode; /** Technical error message */ message: string; /** User-friendly error message */ userMessage: string; /** Additional context/details about the error */ details?: Record<string, unknown>; } -/** - * Generic result type for internal operations. - * Used for validation and download operations that don't return ProcessedFileBase. - */ -export interface ProcessorOperationResult<T = void> { - /** Whether the operation was successful */ - success: boolean; - /** Operation result data (present when success is true) */ - data?: T; - /** Error information (present when success is false) */ - error?: ProcessorFileError; -} +export type ProcessorOperationResult<T = void> = + | { success: true; data: T } + | { success: false; error: ProcessorFileError }; /** * Result of a file processing operation. * Uses discriminated union pattern for type-safe error handling. */ -export interface ProcessorFileResult<T extends ProcessedFileBase = ProcessedFileBase> { - /** Whether the processing was successful */ - success: boolean; - /** Processed file data (present when success is true) */ - data?: T; - /** Error information (present when success is false) */ - error?: ProcessorFileError; -} +export type ProcessorFileResult<T extends ProcessedFileBase = ProcessedFileBase> = + | { success: true; data: T } + | { success: false; error: ProcessorFileError };src/lib/image-gen/types.ts (1)
40-81: Use the existing union types for provider/aspect/style in public options.
provider,aspectRatio, andstyleare currentlystring, despite havingImageGenProvider,AspectRatio, andStylePresetunions. This weakens type safety and IntelliSense for consumers.♻️ Proposed update
export interface ImageGenOptions { @@ - provider?: string; + provider?: ImageGenProvider; @@ - aspectRatio?: string; + aspectRatio?: AspectRatio; @@ - style?: string; + style?: StylePreset; } @@ export interface ImageGenResult { @@ - provider?: string; + provider?: ImageGenProvider; } @@ export interface ImageGenConfig { @@ - defaultProvider: string; + defaultProvider: ImageGenProvider; @@ } @@ export interface ImageGenToolParams { @@ - aspectRatio?: string; + aspectRatio?: AspectRatio; @@ - style?: string; + style?: StylePreset; }As per coding guidelines: Maintain strict TypeScript type safety across all modules with no implicit any and proper type inference.
Also applies to: 146-182, 214-222, 283-309
docs/migration/CURATOR_MIGRATION_ANALYSIS_REPORT.md (1)
5-6: Local file paths in documentation.Lines 5-6 contain developer-specific local paths that may be confusing for other contributors and could inadvertently expose system structure details.
📝 Suggested fix
-> **Source Repository**: `/Users/sachinsharma/Developer/Official/curator-fork/curator` -> **Target Repository**: `/Users/sachinsharma/Developer/temp/neurolink-fork/feat/multimodality-support` +> **Source Repository**: `curator` (Curator production repository) +> **Target Repository**: `neurolink` (branch: `feat/multimodality-support`)src/lib/processors/data/YamlProcessor.ts (1)
160-172:containsDangerousTagsmethod appears unused.The
containsDangerousTagsmethod (line 170-172) is defined but not called anywhere - onlygetDetectedDangerousTagsis used. Consider removing the unused method to reduce code surface.src/lib/processors/config/mimeTypes.ts (1)
263-296: Move exported MIME type unions intosrc/lib/types.These are reusable public types; keeping them in a config implementation file conflicts with the project’s type placement standard. Consider relocating them under
src/lib/typesand re-exporting fromsrc/lib/types/index.ts.
Based on learnings: Project standard: Place reusable/shared types under src/lib/types/.ts; test-only helper types under test/types/.ts; avoid declaring local types inside source implementation files.src/lib/processors/markup/MarkdownProcessor.ts (2)
46-55: Prefer centralized Markdown MIME/extension lists.To prevent drift across processors, consider sourcing these lists from the shared config modules instead of local arrays.
83-98: MoveProcessedMarkdownto the shared types area.This exported type is part of the public surface and should live under
src/lib/typeswith re-exports fromsrc/lib/types/index.ts.
Based on learnings: Project standard: Place reusable/shared types under src/lib/types/.ts; test-only helper types under test/types/.ts; avoid declaring local types inside source implementation files.src/lib/processors/base/types.ts (1)
40-90: Consider relocating these reusable processor types tosrc/lib/types.These interfaces are widely shared; keeping them under the types domain (with re-export via
src/lib/types/index.ts) aligns with project standards.
Based on learnings: Project standard: Place reusable/shared types under src/lib/types/.ts; test-only helper types under test/types/.ts; avoid declaring local types inside source implementation files.src/lib/processors/markup/SvgProcessor.ts (1)
66-78: MoveProcessedSvgto the shared types area.This exported type is part of the public surface and should live under
src/lib/typeswith re-exports fromsrc/lib/types/index.ts.
Based on learnings: Project standard: Place reusable/shared types under src/lib/types/.ts; test-only helper types under test/types/.ts; avoid declaring local types inside source implementation files.src/lib/processors/data/XmlProcessor.ts (1)
59-70: MoveProcessedXmlto the shared types area.This exported type is part of the public surface and should live under
src/lib/typeswith re-exports fromsrc/lib/types/index.ts.
Based on learnings: Project standard: Place reusable/shared types under src/lib/types/.ts; test-only helper types under test/types/.ts; avoid declaring local types inside source implementation files.src/lib/processors/registry/ProcessorRegistry.ts (3)
100-117: Consider aligning this registry with the shared BaseRegistry.If a core/infrastructure BaseRegistry exists, extending it here would keep lifecycle behavior and cross-registry conventions consistent.
Based on learnings: Applies to src/lib/{factories,*Registry}.ts : Use BaseFactory and BaseRegistry in core/infrastructure for consistent factory and registry pattern implementation across all features.
431-440: Consider routing registry processing through ProcessorPipeline.Directly invoking
processor.processFileskips any pipeline hooks or standardized I/O handling that ProcessorPipeline may provide.
Based on learnings: Applies to src/lib/processors/{pipeline,registry}.ts : Use ProcessorPipeline for input/output processing with ProcessorRegistry for processor management and dynamic registration.
565-590: Avoid reaching into protectedconfig.
reg.processor["config"]bypasses encapsulation and is brittle. Consider exposing supported MIME types/extensions viaProcessorRegistrationor a public getter onBaseFileProcessor.src/lib/processors/base/BaseFileProcessor.ts (1)
382-433: Consider usingwithTimeoutfor async download handling.The manual AbortController works, but wrapping the fetch/decompression in the shared
withTimeoututility would align timeout handling across providers and enable consistent fallback behavior. As per coding guidelines:src/lib/**/*.ts: Implement graceful provider fallback with withTimeout utility for async operations.src/lib/processors/document/OpenDocumentProcessor.ts (1)
81-83: Type the dynamicadm-zipimport to avoidany.
require("adm-zip")becomesanyunder strict TS. Add a type assertion (or typed dynamic import) to preserve type safety. As per coding guidelines:src/**/*.ts: Maintain strict TypeScript type safety across all modules with no implicit any and proper type inference.🔧 Suggested fix
- const AdmZip = require("adm-zip"); + const AdmZip = require("adm-zip") as typeof import("adm-zip");src/lib/processors/document/ExcelProcessor.ts (2)
287-289: Avoid non-null assertion after success check.Line 289 uses
downloadResult.data!but the success check on Line 283 already validates the result. However, TypeScript's type narrowing doesn't automatically infer thatdatais defined whensuccessis true. Consider a safer pattern.🔧 Proposed fix using explicit check
if (!downloadResult.success) { return { success: false, error: downloadResult.error, }; } - buffer = downloadResult.data!; + if (!downloadResult.data) { + return { + success: false, + error: this.createError(FileErrorCode.DOWNLOAD_FAILED, { + reason: "Download succeeded but returned no data", + }), + }; + } + buffer = downloadResult.data;
404-412: Row iteration continues after limit is reached.The
eachRowcallback setstruncated = trueand returns early whenrowIndex >= maxRows, buteachRowwill continue invoking the callback for remaining rows. This is inefficient for large sheets.Consider tracking truncation state outside and breaking early if ExcelJS supports it, or document that this is expected behavior. For very large files, this could impact performance.
src/lib/processors/integration/FileProcessorIntegration.ts (1)
327-340: Type assertions for accessing processor config are brittle.The nested type assertions to access
processor.configcould break silently if the processor implementation changes. Consider defining a shared interface for processors that expose their configuration.♻️ Suggested approach
Consider adding a
getConfig()method to theProcessorBaseinterface or using a type guard:// In base/types.ts or registry/types.ts export interface ProcessorWithConfig { config?: { supportedMimeTypes?: string[]; supportedExtensions?: string[]; }; } // Then use type guard function hasConfig(processor: unknown): processor is ProcessorWithConfig { return typeof processor === 'object' && processor !== null && 'config' in processor; }src/lib/processors/errors/errorSerializer.ts (2)
457-462: Redundant ternary expression.Both branches of the ternary return the same value (
value.byteLength), making it unnecessary.🔧 Proposed fix
if (ArrayBuffer.isView(value) || value instanceof ArrayBuffer) { - const length = ArrayBuffer.isView(value) - ? value.byteLength - : value.byteLength; + const length = value.byteLength; return `[Buffer: ${length} bytes]`; }
20-48: Consider adding common sensitive field patterns.The SENSITIVE_FIELDS list is comprehensive but could include additional common patterns like
jwt,key(standalone), andx-api-key.🔧 Suggested additions
const SENSITIVE_FIELDS = [ "password", "token", "secret", "apiKey", "api_key", "authorization", "cookie", "session", "credentials", "privateKey", "private_key", "accessToken", "access_token", "refreshToken", "refresh_token", "apiSecret", "api_secret", "clientSecret", "client_secret", "bearer", "auth", "ssn", "socialSecurity", "creditCard", "credit_card", "cvv", "pin", + "jwt", + "x-api-key", + "passphrase", + "connectionString", + "connection_string", ] as const;src/lib/image-gen/imageGenTools.ts (1)
325-331: Each call creates a new ImageGenService instance.
getImageGenToolsandgetBasicImageGenToolcreate a newImageGenServiceinstance on every call. If called multiple times in an application, this creates multiple service instances which may not be the intended behavior.Consider either documenting this behavior or providing a way to pass an existing service instance.
♻️ Suggested approach - allow reusing service
export function getImageGenTools( - config?: Partial<ImageGenConfig>, + configOrService?: Partial<ImageGenConfig> | ImageGenService, ): ImageGenToolDefinition[] { - const service = new ImageGenService(config); + const service = configOrService instanceof ImageGenService + ? configOrService + : new ImageGenService(configOrService); return [createImageGenTool(service), createImageVariationTool(service)]; }Alternatively, add a note in the JSDoc that a new service is created per call.
Also applies to: 339-344
src/lib/processors/cli/fileProcessorCli.ts (1)
206-232: Consider using async fs operations for consistency.The function is declared
asyncbut uses synchronous fs operations (existsSync,statSync,readFileSync). For CLI usage this is typically fine, but for consistency with the async signature and to avoid blocking the event loop on large files, consider using async versions.♻️ Suggested async version
+import { promises as fsPromises } from "fs"; export async function loadFileFromPath(filePath: string): Promise<FileInfo> { const absolutePath = path.resolve(filePath); - if (!fs.existsSync(absolutePath)) { - throw new Error(`File not found: ${absolutePath}`); - } - - const stats = fs.statSync(absolutePath); + let stats; + try { + stats = await fsPromises.stat(absolutePath); + } catch { + throw new Error(`File not found: ${absolutePath}`); + } + if (!stats.isFile()) { throw new Error(`Not a file: ${absolutePath}`); } - const buffer = fs.readFileSync(absolutePath); + const buffer = await fsPromises.readFile(absolutePath); const filename = path.basename(absolutePath); const ext = path.extname(filename).toLowerCase();src/lib/processors/config/fileTypes.ts (1)
634-662: Consider relocating shared type aliases to src/lib/types.These exported types look reusable across modules; placing them under src/lib/types and re-exporting via src/lib/types/index.ts keeps shared type ownership consistent.
Based on learnings: Project standard: Place reusable/shared types under src/lib/types/.ts; test-only helper types under test/types/.ts; avoid declaring local types inside source implementation files; Export types and interfaces from src/lib/types/index.ts as the main type definitions file.
| export const SOURCE_CODE_EXTENSIONS = [ | ||
| // JavaScript/TypeScript | ||
| ".js", | ||
| ".jsx", | ||
| ".mjs", | ||
| ".cjs", | ||
| ".ts", | ||
| ".tsx", | ||
| // Python | ||
| ".py", | ||
| ".pyw", | ||
| ".pyi", | ||
| // Java/Kotlin | ||
| ".java", | ||
| ".kt", | ||
| ".kts", | ||
| // Systems languages | ||
| ".go", | ||
| ".rs", | ||
| ".c", | ||
| ".h", | ||
| ".cpp", | ||
| ".hpp", | ||
| ".cc", | ||
| ".cxx", | ||
| ".hxx", | ||
| ".cs", | ||
| // Scripting languages | ||
| ".rb", | ||
| ".rake", | ||
| ".php", | ||
| ".phtml", | ||
| ".sh", | ||
| ".bash", | ||
| ".zsh", | ||
| ".fish", | ||
| ".ksh", | ||
| ".pl", | ||
| ".pm", | ||
| ".lua", | ||
| // Database | ||
| ".sql", | ||
| // Mobile | ||
| ".swift", | ||
| ".dart", | ||
| ".m", | ||
| ".mm", | ||
| // Functional | ||
| ".scala", | ||
| ".sc", | ||
| ".hs", | ||
| ".lhs", | ||
| ".ex", | ||
| ".exs", | ||
| ".erl", | ||
| ".hrl", | ||
| ".clj", | ||
| ".cljs", | ||
| ".cljc", | ||
| ".edn", | ||
| ".fs", | ||
| ".fsx", | ||
| ".fsi", | ||
| ".ml", | ||
| ".mli", | ||
| ".lisp", | ||
| ".lsp", | ||
| ".scm", | ||
| ".ss", | ||
| // Other languages | ||
| ".groovy", | ||
| ".gvy", | ||
| ".gy", | ||
| ".gsh", | ||
| ".ps1", | ||
| ".psm1", | ||
| ".psd1", | ||
| ".r", | ||
| ".R", | ||
| ".rmd", | ||
| ".jl", | ||
| ".nim", | ||
| ".nims", | ||
| ".zig", | ||
| ".v", | ||
| ".cr", | ||
| ".d", | ||
| ".asm", | ||
| ".s", | ||
| ".S", | ||
| ".f", | ||
| ".f90", | ||
| ".f95", | ||
| ".for", | ||
| ".cob", | ||
| ".cbl", | ||
| ".pas", | ||
| ".pp", | ||
| ".ada", | ||
| ".adb", | ||
| ".ads", | ||
| // Web/templates | ||
| ".vue", | ||
| ".svelte", | ||
| ".hbs", | ||
| ".handlebars", | ||
| ".ejs", | ||
| ".pug", | ||
| ".jade", | ||
| // Stylesheets | ||
| ".css", | ||
| ".scss", | ||
| ".sass", | ||
| ".less", | ||
| ".styl", | ||
| // Build/Config | ||
| ".dockerfile", | ||
| ".mk", | ||
| ] as const; |
There was a problem hiding this comment.
SOURCE_CODE_EXTENSIONS omits declared language variants.
The combined list claims to include “all source code extensions” but misses entries already defined above (e.g., .php3/.php4/.php5/.phps, .pod/.t, .cl, .Rmd, .f03, .cobol, .p, .stylus). Those files won’t be recognized as source code. Consider adding the missing entries (or deriving this list from the per-language arrays to prevent drift).
🧩 Suggested fix (add missing extensions)
".rb",
".rake",
".php",
".phtml",
+ ".php3",
+ ".php4",
+ ".php5",
+ ".phps",
".sh",
".bash",
".zsh",
".fish",
".ksh",
".pl",
".pm",
+ ".pod",
+ ".t",
".lua",
// Database
".sql",
@@
".ml",
".mli",
".lisp",
".lsp",
+ ".cl",
".scm",
".ss",
@@
".r",
".R",
".rmd",
+ ".Rmd",
".jl",
@@
".f",
".f90",
".f95",
+ ".f03",
".for",
".cob",
".cbl",
+ ".cobol",
".pas",
".pp",
+ ".p",
@@
".less",
".styl",
+ ".stylus",🤖 Prompt for AI Agents
In `@src/lib/processors/config/fileTypes.ts` around lines 283 - 401,
SOURCE_CODE_EXTENSIONS is missing several declared language variants so some
source files (e.g., PHP3/4/5, PHP secure, Perl pods/tests, Common Lisp, Fortran
2003, .stylus, and others) won’t be detected; update the SOURCE_CODE_EXTENSIONS
constant to include the missing extensions such as .php3, .php4, .php5, .phps,
.pod, .t, .cl, .f03, .stylus (and any other per-language variants noted in your
language arrays), or refactor to derive SOURCE_CODE_EXTENSIONS programmatically
from the per-language extension arrays to keep them in sync (make changes where
SOURCE_CODE_EXTENSIONS is declared to either add the entries or replace the
literal array with a computed union).
| private checkXxeVectors(content: string): { | ||
| hasDOCTYPE: boolean; | ||
| hasENTITY: boolean; | ||
| } { | ||
| return { | ||
| hasDOCTYPE: content.includes("<!DOCTYPE"), | ||
| hasENTITY: content.includes("<!ENTITY"), | ||
| }; |
There was a problem hiding this comment.
XXE checks should be case-insensitive.
Lowercase <!doctype> or <!entity> would bypass the current includes checks and still enable XXE vectors.
🛡️ Suggested fix (case-insensitive XXE detection)
- hasDOCTYPE: content.includes("<!DOCTYPE"),
- hasENTITY: content.includes("<!ENTITY"),
+ hasDOCTYPE: /<!doctype/i.test(content),
+ hasENTITY: /<!entity/i.test(content),📝 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.
| private checkXxeVectors(content: string): { | |
| hasDOCTYPE: boolean; | |
| hasENTITY: boolean; | |
| } { | |
| return { | |
| hasDOCTYPE: content.includes("<!DOCTYPE"), | |
| hasENTITY: content.includes("<!ENTITY"), | |
| }; | |
| private checkXxeVectors(content: string): { | |
| hasDOCTYPE: boolean; | |
| hasENTITY: boolean; | |
| } { | |
| return { | |
| hasDOCTYPE: /<!doctype/i.test(content), | |
| hasENTITY: /<!entity/i.test(content), | |
| }; | |
| } |
🤖 Prompt for AI Agents
In `@src/lib/processors/data/XmlProcessor.ts` around lines 142 - 149, The
checkXxeVectors method currently uses case-sensitive string.includes checks and
misses lowercased XXE markers; update checkXxeVectors to perform
case-insensitive detection by normalizing the input (e.g.,
content.toLowerCase()) or using case-insensitive regex (e.g., /<!doctype/i,
/<!entity/i) and return hasDOCTYPE/hasENTITY based on those checks so variations
like "<!doctype>" or mixed case are caught by the function.
| if (char === "}") { | ||
| depth--; | ||
| if (depth <= 0) { | ||
| skipGroup = false; | ||
| } | ||
| i++; | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Bug: skipGroup reset logic may skip valid content in nested structures.
The skipGroup flag only resets when depth <= 0, but skip groups (like fonttbl) may be nested inside other groups. When exiting a skip group that's not at the root level, skipGroup remains true and subsequent content is incorrectly skipped.
Example: {\\rtf1{\\outer{\\fonttbl...}valid text}} - "valid text" would be skipped because depth is 2 when exiting fonttbl.
🐛 Proposed fix
+ let skipGroupDepth = 0; // Track the depth at which skip started
+
while (i < text.length) {
const char = text[i];
if (char === "{") {
depth++;
// Check if this is a group we should skip
const nextChars = text.substring(i + 1, i + 20);
const groupMatch = nextChars.match(/^\\([a-z]+)/);
- if (groupMatch && skipGroupNames.includes(groupMatch[1])) {
+ if (groupMatch && skipGroupNames.includes(groupMatch[1]) && !skipGroup) {
skipGroup = true;
+ skipGroupDepth = depth;
}
i++;
continue;
}
if (char === "}") {
depth--;
- if (depth <= 0) {
+ if (skipGroup && depth < skipGroupDepth) {
skipGroup = false;
+ skipGroupDepth = 0;
}
i++;
continue;
}🤖 Prompt for AI Agents
In `@src/lib/processors/document/RtfProcessor.ts` around lines 219 - 226, The bug
is that skipGroup is only cleared when depth <= 0, which lets nested groups keep
skipGroup true and incorrectly skip subsequent content; modify the RtfProcessor
parsing logic (where skipGroup and depth are managed) to record the depth at
which a skip group was entered (e.g., set skipGroupStartDepth when you set
skipGroup = true) and then clear skipGroup when the current depth drops below
that recorded skipGroupStartDepth in the '}' handling code; update the relevant
symbols (skipGroup, depth and introduce skipGroupStartDepth or skipDepth) so the
parser only skips until the matching closing brace for the group that triggered
skipping.
| private static async processSvgAsText( | ||
| content: Buffer, | ||
| detection: FileDetectionResult, | ||
| ): Promise<FileProcessingResult> { | ||
| try { | ||
| // Dynamic import to avoid circular dependencies | ||
| const { processSvg } = await import( | ||
| "../processors/markup/SvgProcessor.js" | ||
| ); | ||
|
|
||
| const result = await processSvg({ | ||
| id: "svg-file", | ||
| name: detection.metadata.filename || "image.svg", | ||
| mimetype: "image/svg+xml", | ||
| size: content.length, | ||
| buffer: content, | ||
| }); | ||
|
|
||
| if (result.success && result.data) { | ||
| logger.info( | ||
| `[FileDetector] SVG processed as text: ${detection.metadata.filename || "image.svg"}`, | ||
| ); | ||
| return { | ||
| type: "svg", | ||
| content: result.data.textContent, // Sanitized SVG content | ||
| mimeType: "image/svg+xml", | ||
| metadata: { | ||
| confidence: detection.metadata.confidence, | ||
| size: content.length, | ||
| filename: detection.metadata.filename, | ||
| extension: detection.extension, | ||
| }, | ||
| }; | ||
| } else { | ||
| // Fallback: return raw content if processor fails | ||
| logger.warn( | ||
| `[FileDetector] SVG processor failed, using raw content: ${result.error?.userMessage}`, | ||
| ); | ||
| return { | ||
| type: "svg", | ||
| content: content.toString("utf-8"), | ||
| mimeType: "image/svg+xml", | ||
| metadata: { | ||
| confidence: detection.metadata.confidence, | ||
| size: content.length, | ||
| filename: detection.metadata.filename, | ||
| extension: detection.extension, | ||
| }, | ||
| }; | ||
| } | ||
| } catch (error) { | ||
| // Fallback: if SvgProcessor is not available, return raw content | ||
| logger.warn( | ||
| `[FileDetector] SVG processor not available, using raw content: ${error instanceof Error ? error.message : String(error)}`, | ||
| ); | ||
| return { | ||
| type: "svg", | ||
| content: content.toString("utf-8"), | ||
| mimeType: "image/svg+xml", | ||
| metadata: { | ||
| confidence: detection.metadata.confidence, | ||
| size: content.length, | ||
| filename: detection.metadata.filename, | ||
| extension: detection.extension, | ||
| }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Fail closed when SVG sanitization is unavailable or fails.
Returning raw SVG in the failure paths reintroduces XSS vectors. It’s safer to surface an error than to emit unsanitized markup.
🛡️ Suggested fix (fail closed on sanitization failure)
- } else {
- // Fallback: return raw content if processor fails
- logger.warn(
- `[FileDetector] SVG processor failed, using raw content: ${result.error?.userMessage}`,
- );
- return {
- type: "svg",
- content: content.toString("utf-8"),
- mimeType: "image/svg+xml",
- metadata: {
- confidence: detection.metadata.confidence,
- size: content.length,
- filename: detection.metadata.filename,
- extension: detection.extension,
- },
- };
- }
+ }
+ // Fail closed instead of returning unsanitized SVG
+ throw new Error(
+ `SVG sanitization failed: ${result.error?.userMessage ?? "unknown error"}`,
+ );
...
- } catch (error) {
- // Fallback: if SvgProcessor is not available, return raw content
- logger.warn(
- `[FileDetector] SVG processor not available, using raw content: ${error instanceof Error ? error.message : String(error)}`,
- );
- return {
- type: "svg",
- content: content.toString("utf-8"),
- mimeType: "image/svg+xml",
- metadata: {
- confidence: detection.metadata.confidence,
- size: content.length,
- filename: detection.metadata.filename,
- extension: detection.extension,
- },
- };
- }
+ } catch (error) {
+ logger.warn(
+ `[FileDetector] SVG processor failed or unavailable: ${error instanceof Error ? error.message : String(error)}`,
+ );
+ throw error;
+ }🤖 Prompt for AI Agents
In `@src/lib/utils/fileDetector.ts` around lines 621 - 687, The SVG handler
processSvgAsText currently falls back to returning raw SVG on processor failure,
which reintroduces XSS risk; update processSvgAsText (and its result-handling
branches) to fail closed by surfacing an error instead of returning unsanitized
markup: when processSvg returns success=false or when the dynamic
import/processing throws, log the error with context (use
detection.metadata.filename, detection.extension) and then throw a descriptive
error (or return a standardized failure FileProcessingResult/error object) so
callers cannot receive raw SVG content; adjust callers of processSvgAsText
accordingly to handle the thrown error/path.
89f230f to
c9af737
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 |
c9af737 to
340ee1d
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 and make sure to validate all the requirements are getting completed |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainI'll conduct a comprehensive review of this PR to validate all requirements are being completed. Let me examine the implementation systematically. 🏁 Script executed: Length of output: 1848 🏁 Script executed: Length of output: 621 🏁 Script executed: Length of output: 4592 🏁 Script executed: Length of output: 1404 🏁 Script executed: Length of output: 1276 🏁 Script executed: Length of output: 714 🏁 Script executed: Length of output: 635 🏁 Script executed: Length of output: 671 🏁 Script executed: Length of output: 393 🏁 Script executed: Length of output: 1059 🏁 Script executed: Length of output: 391 Let me verify a few more critical aspects: 🏁 Script executed: Length of output: 385 🏁 Script executed: Length of output: 231 🏁 Script executed: Length of output: 1339 🏁 Script executed: Length of output: 560 🏁 Script executed: Length of output: 861 🏁 Script executed: Length of output: 127 🏁 Script executed: Length of output: 183 🏁 Script executed: Length of output: 1113 🏁 Script executed: Length of output: 116 🏁 Script executed: Length of output: 56 Let me verify the test structure and a couple of final details: 🏁 Script executed: Length of output: 2621 🏁 Script executed: Length of output: 493 🏁 Script executed: Length of output: 956 🏁 Script executed: Length of output: 1602 🏁 Script executed: Length of output: 128 🏁 Script executed: Length of output: 1407
✅ All Requirements Successfully CompletedArchitecture & File Coverage (37/37 files ✅)Processor Categories:
File Type Coverage: 17+ Categories ✅Security Features ✅✓ OWASP HTML sanitization - context-aware escaping in html.ts
✓ SVG XSS prevention - allowlist approach, blocks scripts/events in svg.ts
✓ XXE protection - rejects DOCTYPE/ENTITY declarations in XmlProcessor
✓ YAML security - CORE_SCHEMA, dangerous tag detection in YamlProcessor
✓ Config security - SECRET_KEY_PATTERNS, redaction in ConfigProcessor
✓ Size limits - comprehensive SIZE_LIMITS_MB, SIZE_LIMITS_BYTES, PROCESSING_LIMITSCore Infrastructure ✅
SVG Text Processing ✅✓ FileType union includes "svg"
✓ processSvgAsText() in fileDetector.ts with dynamic import
✓ messageBuilder injects SVG as ```xml code block
✓ SvgProcessor sanitizes and treats SVG as text contentUtilities ✅
ImageGen Service ✅✓ ImageGenService: 393 lines, dynamic NeuroLink import, config management
✓ imageGenTools: 372 lines, createImageGenTool, createImageVariationTool
✓ Types: comprehensive ImageGenConfig, ImageGenOptions, ImageGenResultTesting & Documentation ✅Configuration Coverage ✅
📊 Code Quality Metrics🎯 Architecture Patterns
✨ Notable Implementation Highlights
📝 Minor Notes
Final Verdict: ✅ ALL REQUIREMENTS COMPLETEDThis PR successfully delivers on all stated objectives:
The implementation is thorough, well-architected, follows TypeScript best practices, and is ready for merge. Outstanding work! 🚀 |
340ee1d to
9ea84bb
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 |
9ea84bb to
0f44846
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 |
0f44846 to
b61bcfc
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 |
…VG text injection Migrate file processing capabilities from Curator to NeuroLink, adding a provider-agnostic ProcessorRegistry architecture that supports documents, data files, markup, source code, and configuration files. Fix SVG files being sent as binary images (which providers reject) by processing them as sanitized text instead. New file processors (37 files in src/lib/processors/): - Document: Excel (.xlsx/.xls), Word (.docx), RTF, OpenDocument (.odt/.ods/.odp) - Data: JSON, YAML, XML with validation and formatting - Markup: HTML (OWASP sanitization), SVG (XSS prevention), Markdown, Text - Code: 50+ languages with syntax detection, config files (.env/.ini/.toml) - Registry: Priority-based ProcessorRegistry with BaseFileProcessor abstract class - Errors: FileErrorCode enum with user-friendly error messages - Config: MIME types, file extensions, language maps, size limits - Integration: FileProcessorIntegration and CLI helpers SVG fix (3 files modified): - src/lib/types/fileTypes.ts: Add "svg" to FileType union - src/lib/utils/fileDetector.ts: Map SVG to "svg" type (not "image"), add processSvgAsText() method, check SVG before generic image/ MIME - src/lib/utils/messageBuilder.ts: Inject SVG as xml code block in prompt text instead of sending as binary image part Supporting additions: - src/lib/utils/async/: delay, withTimeout, retry utilities - src/lib/utils/json/: safeParse, JSON extraction utilities - src/lib/utils/sanitizers/: SVG, HTML, filename sanitizers (OWASP) - src/lib/image-gen/: ImageGenService, tools, and types - src/lib/types/processorTypes.ts: Centralized processor type definitions - src/lib/types/index.ts: Add processorTypes barrel export + import reordering - test/file-processor-test-suite.ts: 30 tests covering CLI and SDK paths - test/fixtures/: Test files for audio, code, documents, ebooks, fonts, images, and video - docs/migration/: Curator migration analysis, plans, and verification Documentation updates: - CLAUDE.md: Expand multimodal support description, add processor architecture to Message Building section, add Document/Data/Markup/Code subsections, update I/O Processors feature count - README.md: Add file types to What's New, expand multimodal in GitHub Action table, add files to SDK example, add Multimodal & File Processing section - docs/features/file-processors.md: New comprehensive guide (supported types, architecture, security, provider compatibility, extension guide) - docs/features/multimodal.md: Add new file types to overview and related features - docs/cli/commands.md: Add --file, --pdf, --csv flags with examples
b61bcfc to
83b11b2
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 |
|
🎉 This PR is included in version 9.2.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
2 participants
…VG text injection
Migrate file processing capabilities from Curator to NeuroLink, adding a provider-agnostic ProcessorRegistry architecture that supports documents, data files, markup, source code, and configuration files. Fix SVG files being sent as binary images (which providers reject) by processing them as sanitized text instead.
New file processors (37 files in src/lib/processors/):
SVG fix (3 files modified):
Supporting additions:
Documentation updates:
Pull Request
Description
What does this PR do?
A clear and concise description of the changes in this pull request.
Related Issues
Does this PR close any issues?
Fixes #(issue number)
Closes #(issue number)
Relates to #(issue number)
Type of Change
Please select the type of change:
Motivation and Context
Why is this change needed? What problem does it solve?
Provide context for reviewers:
Changes Made
What specific changes were made?
Provide a bullet-point list of the key changes:
Breaking Changes
Does this PR introduce breaking changes?
If yes, describe:
Testing
How has this been tested?
Please describe the tests you ran and their results:
Test Coverage
Manual Testing Steps
Provide steps for manual testing:
Code Quality
Have you followed code quality standards?
Documentation
Have you updated documentation?
Commit Message Format
Does your commit follow semantic commit conventions?
type(scope): descriptionExample:
feat(providers): add support for LiteLLM proxyDependencies
Does this PR add, update, or remove dependencies?
If yes, list dependencies and justification:
Performance Impact
Does this change affect performance?
If applicable, provide benchmark results:
Security Considerations
Are there any security implications?
If applicable, describe:
Deployment Notes
Special deployment instructions?
Screenshots / Videos
If applicable, add screenshots or videos to demonstrate changes:
[Add screenshots or videos here]
Reviewer Checklist
For reviewers:
Additional Notes
Any additional information for reviewers:
[Add any extra context, concerns, or questions here]
Pre-submission Checklist
Before submitting, ensure you have:
pnpm testpnpm buildpnpm run validate:alland all checks passThank you for contributing to NeuroLink!
Summary by CodeRabbit
New Features
Documentation
CLI