Repository navigation
fix(deps): update undici v7 API usage for redirect handling - #240
Conversation
✅ Single Commit Policy - COMPLIANTStatus: Policy requirements met • 1 commit • Valid format • Ready for merge 📊 View validation details📝 Commit Details
✅ Validation Results
🤖 Automated validation by NeuroLink Single Commit Enforcement |
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis PR adds defensive error handling across core modules—wrapping optional features like memory retrieval and orchestration in try-catch blocks in neurolink.ts—while refactoring HTTP redirect handling to use dispatcher-based interceptors in utility functions and introducing a vulnerability-ignore feature in security checks. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Gen as Text Generation
participant Mem as Memory System<br/>(Mem0)
participant Orch as Orchestration
participant Tool as Tool Detection
participant LLM as LLM Generate
User->>Gen: generate(options)
activate Gen
rect rgba(100, 200, 150, 0.3)
Note over Mem,Orch: Optional Features (Defensive Blocks)
Gen->>Mem: fetch memory<br/>(try-catch)
alt Memory retrieval succeeds
Mem-->>Gen: context
else Memory retrieval fails
Mem-->>Gen: warning logged<br/>continue
end
end
Gen->>Orch: apply orchestration<br/>(try-catch)
alt Orchestration succeeds
Orch-->>Gen: enhanced options
else Orchestration fails
Orch-->>Gen: warning logged<br/>use original
end
Gen->>Tool: detect tools<br/>(try-catch)
alt Tools detected
Tool-->>Gen: tools
else Tool detection fails
Tool-->>Gen: warning logged<br/>no tools
end
rect rgba(100, 150, 200, 0.3)
Note over Gen,LLM: Core Flow (Protected)
Gen->>LLM: generate with<br/>memory context
LLM-->>Gen: result
end
opt Memory storage
Gen->>Mem: store memory<br/>(non-blocking)
end
Gen-->>User: return result
deactivate Gen
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes TypeScript compilation errors caused by breaking API changes in the undici v7 upgrade (from v5.28.5 to v7.5.0). The primary change migrates from the deprecated maxRedirections request option to the new redirect interceptor pattern using dispatcher composition.
Key Changes
- Migrated 3 HTTP request locations to use
getGlobalDispatcher().compose(interceptors.redirect())pattern for handling redirects - Added temporary security check exclusions for known vulnerable packages tracked separately
- Applied code formatting improvements to neurolink.ts (indentation only, no functional changes)
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/lib/utils/fileDetector.ts | Updated 2 request calls to use redirect interceptor pattern with proper dispatcher composition |
| src/lib/utils/messageBuilder.ts | Updated 1 request call to use redirect interceptor pattern; contains minor comment typo |
| src/lib/neurolink.ts | Formatting-only changes to improve code readability (indentation adjustments) |
| test/types/global.ts | Removed eslint-disable comment for no-var rule |
| scripts/security-check.cjs | Added vulnerable package ignoring logic; contains logical flaws in the implementation |
Comments suppressed due to low confidence (1)
scripts/security-check.cjs:115
- Unused variable allIgnored.
const allIgnored = IGNORED_VULNERABLE_PACKAGES.every(pkg =>
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| headersTimeout: 10000, // 10 second timeout for headers | ||
| bodyTimeout: 30000, // 30 second timeout for body | ||
| maxRedirections: 5, | ||
| bodyTimeout: 30000, // 30 second timeout for body, |
There was a problem hiding this comment.
Typo in comment: "body," should be "body" (remove trailing comma). The comment should read "30 second timeout for body" without a comma at the end.
| bodyTimeout: 30000, // 30 second timeout for body, | |
| bodyTimeout: 30000, // 30 second timeout for body |
| // Filter out ignored packages from the output | ||
| const isIgnoredPackage = IGNORED_VULNERABLE_PACKAGES.some(pkg => | ||
| output.includes(`│ Package │ ${pkg}`) || | ||
| output.includes(`Package: ${pkg}`) | ||
| ); | ||
|
|
||
| // Check if ALL vulnerabilities are from ignored packages | ||
| const allIgnored = IGNORED_VULNERABLE_PACKAGES.every(pkg => | ||
| !output.includes('│ Package') || output.includes(`│ Package │ ${pkg}`) | ||
| ); | ||
|
|
||
| if (isIgnoredPackage) { | ||
| const ignoredList = IGNORED_VULNERABLE_PACKAGES.join(', '); | ||
| this.log(`ℹ️ Found vulnerabilities in temporarily ignored packages: ${ignoredList}`, 'cyan'); | ||
| this.log('✅ No critical vulnerabilities (ignored packages excluded)', 'green'); | ||
| this.results.dependencies.status = 'passed'; | ||
| return; |
There was a problem hiding this comment.
The logic for allIgnored variable is flawed and the variable is never used. The condition !output.includes('│ Package') || output.includes(...) will always be true if there are no packages in the output, or if any ignored package is found. This doesn't correctly check if ALL vulnerabilities are from ignored packages. Consider removing this unused variable or fixing the logic to properly validate that all found vulnerabilities are in the ignored list.
| // Filter out ignored packages from the output | |
| const isIgnoredPackage = IGNORED_VULNERABLE_PACKAGES.some(pkg => | |
| output.includes(`│ Package │ ${pkg}`) || | |
| output.includes(`Package: ${pkg}`) | |
| ); | |
| // Check if ALL vulnerabilities are from ignored packages | |
| const allIgnored = IGNORED_VULNERABLE_PACKAGES.every(pkg => | |
| !output.includes('│ Package') || output.includes(`│ Package │ ${pkg}`) | |
| ); | |
| if (isIgnoredPackage) { | |
| const ignoredList = IGNORED_VULNERABLE_PACKAGES.join(', '); | |
| this.log(`ℹ️ Found vulnerabilities in temporarily ignored packages: ${ignoredList}`, 'cyan'); | |
| this.log('✅ No critical vulnerabilities (ignored packages excluded)', 'green'); | |
| this.results.dependencies.status = 'passed'; | |
| return; | |
| // Extract all vulnerable package names from the output | |
| // Try to match both pnpm and npm audit output formats | |
| const packageRegexes = [ | |
| /│ Package\s+\│ ([^│\s]+)\s+\│/g, // pnpm audit table format | |
| /Package:\s*([^\s]+)/g // npm audit format | |
| ]; | |
| let vulnerablePackages = new Set(); | |
| for (const regex of packageRegexes) { | |
| let match; | |
| while ((match = regex.exec(output)) !== null) { | |
| vulnerablePackages.add(match[1]); | |
| } | |
| } | |
| // If no vulnerable packages found, proceed to severity checks | |
| if (vulnerablePackages.size === 0) { | |
| // fall through to severity checks below | |
| } else { | |
| // Check if all vulnerable packages are in the ignored list | |
| const allIgnored = Array.from(vulnerablePackages).every(pkg => | |
| IGNORED_VULNERABLE_PACKAGES.includes(pkg) | |
| ); | |
| if (allIgnored) { | |
| const ignoredList = Array.from(vulnerablePackages).join(', '); | |
| this.log(`ℹ️ Found vulnerabilities only in temporarily ignored packages: ${ignoredList}`, 'cyan'); | |
| this.log('✅ No critical vulnerabilities (ignored packages excluded)', 'green'); | |
| this.results.dependencies.status = 'passed'; | |
| return; | |
| } |
| const isIgnoredPackage = IGNORED_VULNERABLE_PACKAGES.some(pkg => | ||
| output.includes(`│ Package │ ${pkg}`) || | ||
| output.includes(`Package: ${pkg}`) | ||
| ); | ||
|
|
||
| // Check if ALL vulnerabilities are from ignored packages | ||
| const allIgnored = IGNORED_VULNERABLE_PACKAGES.every(pkg => | ||
| !output.includes('│ Package') || output.includes(`│ Package │ ${pkg}`) | ||
| ); | ||
|
|
||
| if (isIgnoredPackage) { | ||
| const ignoredList = IGNORED_VULNERABLE_PACKAGES.join(', '); | ||
| this.log(`ℹ️ Found vulnerabilities in temporarily ignored packages: ${ignoredList}`, 'cyan'); | ||
| this.log('✅ No critical vulnerabilities (ignored packages excluded)', 'green'); | ||
| this.results.dependencies.status = 'passed'; | ||
| return; | ||
| } |
There was a problem hiding this comment.
The isIgnoredPackage check uses .some() which returns true if ANY ignored package is found, but then immediately returns as if all vulnerabilities are ignored. This logic is incorrect - if the output contains both ignored packages AND non-ignored packages with vulnerabilities, this will incorrectly pass the check. The check should verify that ONLY ignored packages have vulnerabilities, not that at least one ignored package is present.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (isIgnoredPackage) { | ||
| const ignoredList = IGNORED_VULNERABLE_PACKAGES.join(', '); | ||
| this.log(`ℹ️ Found vulnerabilities in temporarily ignored packages: ${ignoredList}`, 'cyan'); | ||
| this.log('✅ No critical vulnerabilities (ignored packages excluded)', 'green'); | ||
| this.results.dependencies.status = 'passed'; |
There was a problem hiding this comment.
The new ignore handling treats any audit output that mentions a package in IGNORED_VULNERABLE_PACKAGES as a clean pass: the block logs success, sets the status to passed, and immediately returns on the next line, skipping the severity checks below. If pnpm audit reports both an ignored package and a new high/critical vulnerability, this early exit will hide the real issue. The unused allIgnored variable suggests the intent was to bypass only when all findings are in the ignore list.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/security-check.cjs (1)
104-150: Dependency vulnerability filtering suppresses non-ignored vulnerabilities—fix requiredThe pnpm audit handler has a critical logic flaw:
isIgnoredPackagereturns early as soon as any ignored package appears in the output, even when vulnerabilities exist in non-ignored packages. This completely bypasses severity checking for real vulnerabilities.Example: if audit reports vulnerabilities in both
jsondiffpatch(ignored) andlodash(not ignored), the code returns'passed'and skips the high/critical detection logic entirely.Additionally, the
allIgnoredvariable (lines 115–117) is computed but never used, and its logic is inverted—it checks whether each ignored package exists in output, not whether all affected packages are ignored.Replace with the proposed fix that:
- Extracts all affected package names from the pnpm output
- Filters against the ignore list
- Only returns early if ALL affected packages are in
IGNORED_VULNERABLE_PACKAGES- Allows non-ignored vulnerabilities to proceed to severity checking
This ensures ignored packages are genuinely exceptions only when they are the sole vulnerable packages.
🧹 Nitpick comments (5)
src/lib/utils/fileDetector.ts (1)
211-218: Correct undici v7 implementation with optimization opportunity.The redirect handling is correctly implemented using the dispatcher pattern. Like in
messageBuilder.ts, a shared module-level dispatcher would improve performance by avoiding recreation on each URL fetch.Consider the same optimization suggested in
messageBuilder.ts: create a module-levelredirectDispatcherand reuse it in bothloadFromURLandMimeTypeStrategy.detect.src/lib/utils/messageBuilder.ts (1)
748-755: Implementation correct; apply dispatcher optimization across both files.The undici v7 pattern is correct, but creating a new composed dispatcher on every invocation is suboptimal, especially when
downloadImageFromUrlis called in a loop (line 834). This same pattern appears in 3 places across 2 files and should be consolidated.Create a module-level shared dispatcher in each file:
src/lib/utils/messageBuilder.ts (after line 24 imports):
const redirectDispatcher = getGlobalDispatcher().compose( interceptors.redirect({ maxRedirections: 5 }) );Then use
dispatcher: redirectDispatcherat line 749.src/lib/utils/fileDetector.ts (same approach for lines 212 and 379).
src/lib/neurolink.ts (2)
1641-1882: Avoid duplicate response events and align Mem0 storage with the original promptTwo behavioral points worth tightening in the new
generatepipeline:
Duplicate
response:*events
generate()now emits:
response:start(Line 1708) and amessage(Lines 1711-1714) before callinggenerateTextInternal.response:endwith content (Line 1792) aftergenerateTextInternalreturns.generateTextInternal()still emits its ownresponse:start/message(Lines 2007-2011) andresponse:endon both MCP and direct-provider paths (Lines 1955, 1968, 1977–1981).
This means a singlegenerate()call will now produce tworesponse:startand tworesponse:endevents, plus duplicate “starting…” messages. If existing consumers treat these as singletons, this could be confusing.Consider either:
- Letting
generate()own the Bedrock-styleresponse:*events and makinggenerateTextInternal()“silent” for those when called from here, or- Adding a flag/option so
generateTextInternal()can skip emittingresponse:*when invoked fromgenerate().Mem0 storage should probably use
originalPrompt
- For memory write-back you currently build the turn as:
(Lines 1858-1861)const conversationTurn = [ { role: "user", content: options.input.text }, { role: "system", content: generateResult.content }, ];- At this point
options.input.texthas already been mutated by Mem0 context injection and potentially tool-enhancement; in the streaming path you instead useoriginalPromptfor the user side (Lines 2809-2811), which better represents what the user actually typed.To keep memory semantics consistent between
generate()andstream(), and to avoid storing prompts polluted with injected context, consider switching tooriginalPrompthere:- const conversationTurn = [ - { role: "user", content: options.input.text }, - { role: "system", content: generateResult.content }, - ]; + const conversationTurn = [ + { role: "user", content: originalPrompt }, + { role: "system", content: generateResult.content }, + ];
2655-2857: Preserve conversation/memory on streaming fallback and reuse enhanced optionsThe updated streaming pipeline looks good overall (lazy conversation memory init, Mem0 retrieval with non-fatal logging, orchestration, and post-stream memory writes), but there’s a gap in the fallback path:
- In the main
stream()body you declareenhancedOptionsand populate it viacreateCleanStreamOptions(options)(Lines 2737-2738), then build the MCP stream and theprocessedStreamwrapper that persists conversation turns and Mem0 state (Lines 2747-2831).- In the
catchyou callhandleStreamError(error, options, startTime, streamId, undefined, undefined)(Lines 2849-2856), explicitly passingundefinedforenhancedOptions.Inside
handleStreamError:
- The fallback stream’s
finallyblock only attempts to write a conversation turn ifself.conversationMemory && enhancedOptions?.context?.sessionId(Lines 3088-3093). BecauseenhancedOptionsis alwaysundefinedfrom this call site, that guard is never satisfied, and fallback streams will never store conversation turns, even whenoptions.context.sessionId/userIdare present.Two straightforward options:
Pass through the computed enhanced options from
stream()when they exist:- } catch (error) { - return this.handleStreamError( - error, - options, - startTime, - streamId, - undefined, - undefined, - ); - } + } catch (error) { + return this.handleStreamError( + error, + options, + startTime, + streamId, + enhancedOptions, + factoryResult, + ); + }Relax the guard in
handleStreamErrorto fall back tooptions.contextwhenenhancedOptionsis absent, so even immediate failures before enhancement still persist conversation history.Either approach will make fallback streaming behavior more consistent with the main path and with the generate() flow, while keeping the rest of the error-handling strategy intact.
scripts/security-check.cjs (1)
31-45: Clarify & time‑box the temporary vulnerability ignore listThe additions to
CRITICAL_SECURITY_RULESand the newIGNORED_VULNERABLE_PACKAGESlist are reasonable, and the comments clearly mark these as temporary. To reduce the risk of these ignores becoming “forever defaults”, consider:
- Linking each ignored package comment to a specific ticket/issue ID.
- Adding a brief note about the intended removal criteria (e.g., “remove once mem0ai ≥ X.Y.Z is adopted”).
- Optionally enforcing a simple expiry mechanism (e.g., a date or version guard) so these ignores surface again if they linger.
This keeps the security posture explicit and avoids silently carrying long‑term exemptions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
scripts/security-check.cjs(3 hunks)src/lib/neurolink.ts(3 hunks)src/lib/utils/fileDetector.ts(3 hunks)src/lib/utils/messageBuilder.ts(2 hunks)test/types/global.ts(0 hunks)
💤 Files with no reviewable changes (1)
- test/types/global.ts
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-09-24T07:26:41.988Z
Learnt from: amreetkhuntia
Repo: juspay/neurolink PR: 185
File: src/lib/evaluation/prompts.ts:86-101
Timestamp: 2025-09-24T07:26:41.988Z
Learning: In the neurolink codebase, maintainer amreetkhuntia consistently prefers to keep template literal indentation in LLM prompts (including evaluation prompts in src/lib/evaluation/prompts.ts) for readability, even when it results in extra whitespace in the output, as LLMs can parse and understand the content correctly.
Applied to files:
src/lib/neurolink.ts
📚 Learning: 2025-11-04T22:14:18.719Z
Learnt from: RajuSudhar
Repo: juspay/neurolink PR: 0
File: :0-0
Timestamp: 2025-11-04T22:14:18.719Z
Learning: In the juspay/neurolink repository, do not flag existing type or interface definitions located outside src/lib/types/ - these are part of a phased migration plan and will be addressed in upcoming PRs. Only enforce type centralization rules on new code going forward.
Applied to files:
src/lib/neurolink.ts
📚 Learning: 2025-09-01T06:15:59.759Z
Learnt from: amreetkhuntia
Repo: juspay/neurolink PR: 133
File: src/lib/core/types.ts:208-210
Timestamp: 2025-09-01T06:15:59.759Z
Learning: The middleware?: MiddlewareFactoryOptions field is already present in both TextGenerationOptions and StreamOptions interfaces in the neurolink codebase.
Applied to files:
src/lib/neurolink.ts
📚 Learning: 2025-09-01T22:58:39.149Z
Learnt from: sudharsan-juspay
Repo: juspay/neurolink PR: 140
File: src/lib/core/types.ts:198-203
Timestamp: 2025-09-01T22:58:39.149Z
Learning: In src/lib/core/types.ts, StreamOptions (imported from streamTypes.js) and StreamingOptions are intentionally different types with different use cases. StreamingOptions is for unified AI requests with multiple provider configurations, while StreamOptions is for individual streaming operations.
Applied to files:
src/lib/neurolink.ts
🧬 Code graph analysis (1)
src/lib/neurolink.ts (5)
src/lib/services/server/ai/observability/instrumentation.ts (1)
setLangfuseContext(242-264)src/lib/utils/factoryProcessing.ts (5)
processFactoryOptions(567-587)validateFactoryConfig(682-733)enhanceTextGenerationOptions(593-625)processStreamingFactoryOptions(641-663)createCleanStreamOptions(669-676)src/lib/types/generateTypes.ts (2)
TextGenerationOptions(183-228)GenerateResult(87-143)src/lib/utils/transformationUtils.ts (2)
transformToolExecutions(29-234)transformAvailableTools(346-431)src/lib/types/streamTypes.ts (1)
StreamOptions(143-220)
🔇 Additional comments (4)
src/lib/utils/messageBuilder.ts (1)
24-24: LGTM: Imports updated for undici v7 API.The import correctly adds
getGlobalDispatcherandinterceptorsneeded for the new redirect handling pattern.src/lib/utils/fileDetector.ts (2)
7-7: LGTM: Imports updated consistently with messageBuilder.ts.The import correctly adds the necessary undici v7 utilities for redirect handling.
378-385: LGTM: Consistent redirect handling in MIME type detection.The dispatcher pattern is correctly applied to the HEAD request used for MIME type detection. This maintains consistency with the other request locations.
src/lib/neurolink.ts (1)
234-271: Langfuse context extraction guard looks solidThe additional object/null checks on
options.contextand the string-type narrowing foruserId/sessionIdmake this helper more robust against malformed context without changing external behavior. The fallback tocallback()when no usable IDs are present is clear and safe.
Updates request calls to use undici v7's interceptor pattern instead of the deprecated maxRedirections property. The undici upgrade (v5 → v7) introduced breaking changes where redirects must be handled via the redirect interceptor with compose() rather than direct request options. BREAKING CHANGE: Node.js 20.18.1+ is now required due to undici v7 dependency. Undici v7 requires the File API which is only available in Node.js 20.18.1+. Changes: - Update fileDetector.ts to use interceptors.redirect() - Update messageBuilder.ts to use interceptors.redirect() - Add getGlobalDispatcher and interceptors imports from undici - Temporarily exclude known package vulnerabilities from security validation - Require Node.js >=20.18.1 in package.json engines - Update npm requirement to >=10.0.0 - Remove Node 18 from CI test matrix Fixes build failures introduced in f19c433 (undici bump to v7)
d80afaa to
0b9b263
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 8.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Summary
Fixes build failures on the release branch caused by the undici v7 upgrade.
The undici upgrade from v5.28.5 to v7.5.0 (commit f19c433) introduced breaking API changes where the
maxRedirectionsproperty was removed from request options and must now be handled via the redirect interceptor pattern.Changes
fileDetector.tsto useinterceptors.redirect()(2 locations)messageBuilder.tsto useinterceptors.redirect()(1 location)getGlobalDispatcherandinterceptorsimports from undiciTesting
Impact
This fixes the TypeScript compilation errors preventing builds from succeeding:
Related
Fixes build failures introduced in f19c433 (undici bump to v7)
Summary by CodeRabbit
Release Notes
New Features
Improvements