Repository navigation
fix(proxy): address missed CodeRabbit outside-diff comments from PR #918 - #919
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis large release PR refactors core generation logic in Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms 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 |
There was a problem hiding this comment.
Pull request overview
Follow-up changes that address previously missed review feedback from PR #918, primarily improving proxy reliability/observability and tightening error handling around provider health checks, MCP tooling, task scheduling, and security tooling.
Changes:
- Hardened proxy observability/logging and fetch behavior (UTF-8 safe truncation, OTLP/log export setup refactors, improved fallback behavior when body artifact persistence fails).
- Improved robustness of MCP and task infrastructure (clearer tool/server disambiguation, safer task schedule rollback/cleanup, Redis TTL retry).
- Refactors/cleanup across provider selection/health checks, typed errors/timeouts, dependency overrides, and documentation site adjustments.
Reviewed changes
Copilot reviewed 41 out of 44 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/continuous-test-suite.ts | Updates cloaking runtime test to validate Claude Code-style session identity formatting. |
| src/lib/utils/providerUtils.ts | Uses ProviderHealthChecker for LiteLLM/Ollama availability and updates Ollama env handling. |
| src/lib/utils/providerHealth.ts | Propagates timeout into config checks; adds connectivity headers; adjusts model matching; refactors timeout usage. |
| src/lib/types/streamTypes.ts | Formatting-only changes to multi-imports and long union type formatting. |
| src/lib/types/proxyTypes.ts | Adds proxy mode/body capture related types and makes tool inputSchema required in ParsedClaudeRequest tools map type. |
| src/lib/telemetry/telemetryService.ts | Improves detection of externally-registered TracerProvider delegates. |
| src/lib/tasks/taskManager.ts | Adds better cleanup/rollback on scheduling failures; defers history clear with error handling. |
| src/lib/tasks/store/redisTaskStore.ts | Passes Redis client to TTL setter and adds retry/backoff for expire() calls. |
| src/lib/tasks/backends/bullmqBackend.ts | Ensures executor cache cleanup when schedule fails; minor logging fix. |
| src/lib/services/server/ai/observability/instrumentation.ts | Refactors OTEL/Langfuse initialization into helpers; adds OTLP metrics/logs initialization helper. |
| src/lib/proxy/sseInterceptor.ts | Makes truncation byte-accurate for UTF-8; updates accumulator byte counting. |
| src/lib/proxy/requestLogger.ts | Adds capped body capture, UTF-8 chunking for OTLP, and in-memory fallback when artifact persistence fails; improves cleanup to include body artifacts. |
| src/lib/proxy/proxyFetch.ts | Refactors header merging/trace injection; introduces proxied/direct handlers and request cloning for fallback safety. |
| src/lib/proxy/proxyConfig.ts | Tightens raw accounts object validation (rejects arrays). |
| src/lib/proxy/oauthFetch.ts | Refactors OAuth fetch into composable helpers; routes through createProxyFetch; rewrites MCP-prefixed streaming responses. |
| src/lib/proxy/claudeFormat.ts | Formatting-only changes; no behavioral changes intended. |
| src/lib/providers/openAI.ts | Resolves toolChoice once and guards against toolChoice referencing filtered-out tools; extracts stream transform helpers. |
| src/lib/providers/ollama.ts | Returns typed ProviderError for HTTP errors; improves 404 detection based on status/body. |
| src/lib/providers/litellm.ts | Formatting and stream transform refactor; minor error formatting improvements. |
| src/lib/providers/googleAiStudio.ts | Removes env-var aliasing block for GOOGLE_* API key. |
| src/lib/mcp/toolRegistry.ts | Extracts tool resolution/execution context creation; uses ErrorFactory for ambiguity cases. |
| src/lib/evaluation/scorers/scorerRegistry.ts | Moves built-in scorer definitions into tables; fixes initPromise reset on failure for retry. |
| src/lib/evaluation/pipeline/evaluationPipeline.ts | Uses ErrorFactory for invalid options configuration error. |
| src/lib/core/factory.ts | Uses typed timeout errors; refactors model resolution and provider creation into helpers. |
| src/lib/core/baseProvider.ts | Refactors generate() control flow into helper methods; maintains gen() alias; improves span-ending control. |
| src/lib/auth/anthropicOAuth.ts | Adds bounded cache eviction for Claude Code identity cache. |
| src/cli/commands/mcp.ts | Adds MCP status timeout; refactors annotate/list flows; improves ambiguous tool handling messaging. |
| scripts/security-check.ts | Adds lodash ignore placeholders; changes ignored vulnerability handling logic. |
| scripts/observability/manage-local-openobserve.sh | Replaces source-based env loading with safe line-by-line parsing; avoids printing password directly. |
| scripts/observability/check-proxy-telemetry.mjs | Treats missing summary file as retryable within loop (continue). |
| pnpm-lock.yaml | Adds override for xmldom and bumps OTLP trace exporter version. |
| package.json | Bumps @opentelemetry/exporter-trace-otlp-http and adds xmldom override. |
| docs/tutorials.md | Adds frontmatter (title/slug) for docs site routing. |
| docs/reference/provider-selection.md | Comment wording update about provider priority rationale. |
| docs/features/claude-proxy.md | Adds CLI install step before proxy quick start. |
| docs/features/claude-proxy-observability.md | Adds CLI install step before observability setup. |
| docs/cookbook/rate-limit-handling.md | Updates Redis v4 method names and zset call patterns. |
| docs/business-documentation.md | Updates quick start link to new tutorials slug route. |
| docs-site/scripts/build-llms-txt.ts | Iteratively reduces truncation to keep llms.txt summary under size target. |
| docs-site/config/redirects.ts | Removes one analytics redirect entry. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Comments suppressed due to low confidence (1)
scripts/security-check.ts:166
- The ignored-package check currently passes the audit as soon as any ignored package appears in the output. This can mask additional vulnerabilities in non-ignored packages (e.g., output contains both lodash and another vulnerable dependency) and incorrectly sets the scan to "passed".
Consider parsing the audit output to extract the set of vulnerable package names and only treating the scan as passed when that set is a subset of IGNORED_VULNERABLE_PACKAGES; otherwise continue with severity counting / failure logic.
// Check if vulnerabilities are from ignored packages
const isIgnoredPackage = IGNORED_VULNERABLE_PACKAGES.some(
(pkg) =>
output.includes(`│ Package │ ${pkg}`) ||
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;
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
1014
to
1027
| private static hasRequestedModel( | ||
| availableModels: string[], | ||
| requestedModel: string, | ||
| ): boolean { | ||
| const normalizedRequestedModel = requestedModel.trim(); | ||
| const requiresExactMatch = /@/.test(normalizedRequestedModel); | ||
|
|
||
| return availableModels.some( | ||
| (model) => | ||
| model === requestedModel || | ||
| model.startsWith(`${requestedModel}:`) || | ||
| requestedModel.startsWith(`${model}:`), | ||
| model === normalizedRequestedModel || | ||
| (!requiresExactMatch && | ||
| (model.startsWith(`${normalizedRequestedModel}:`) || | ||
| model.startsWith(`${normalizedRequestedModel}@`))), | ||
| ); |
There was a problem hiding this comment.
hasRequestedModel() no longer matches common Ollama tag variants (e.g. configured gemma3:27b vs installed gemma3:27b-fp16), because it only allows model.startsWith(<requested> + ':') or ... + '@'. This can cause false negatives in Ollama/LiteLLM availability checks and prevent selecting an otherwise working local fallback.
Consider restoring the previous bidirectional/prefix matching (or explicitly allowing - suffix variants) so requestedModel can match installed variants like *-fp16.
Comment on lines
+559
to
+564
| const cacheKey = createHash("sha256") | ||
| .update(maskProxyUrl(proxyUrl) ?? proxyUrl) | ||
| .digest("hex"); | ||
| const dispatcher = | ||
| agentCache.get(cacheKey) || (await createProxyAgent(proxyUrl)); | ||
| agentCache.set(cacheKey, dispatcher); |
There was a problem hiding this comment.
The ProxyAgent cache key is derived from maskProxyUrl(proxyUrl), which replaces username/password with ***. If HTTP_PROXY and HTTPS_PROXY (or other proxy envs) differ only by credentials, they will collide to the same cache key and may reuse a ProxyAgent configured with the wrong credentials.
To avoid collisions without storing plaintext credentials, consider hashing the original proxyUrl (or a canonical form that retains credential uniqueness) and only using maskProxyUrl for logging.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/lib/evaluation/scorers/scorerRegistry.ts (1)
467-482:⚠️ Potential issue | 🟠 MajorThe rejected promise blocks retry indefinitely.
When
ScorerRegistry.registerBuiltInLLMScorers()orScorerRegistry.registerBuiltInRuleScorers()throws, the async IIFE immediately rejects. The synchronous assignmentScorerRegistry.initPromise = (async () => { ... })()completes before the async body executes, so the rejected promise is already stored. The catch block then setsinitPromise = null, but the rejected promise object remains in the variable assignment—the next caller still receives the same permanently rejected initializer.The proposed fix is correct: separate promise creation from assignment, await the promise after assignment, and check
if (ScorerRegistry.initPromise === initPromise)before resetting to avoid races with concurrent retries. Also resetScorerRegistry.initialized = falseon error so the state machine is correct.🐛 Proposed fix
- ScorerRegistry.initPromise = (async () => { - try { - ScorerRegistry.registerBuiltInLLMScorers(); - ScorerRegistry.registerBuiltInRuleScorers(); - - ScorerRegistry.initialized = true; - logger.debug( - `Registered ${ScorerRegistry.scorers.size} built-in scorers (including aliases)`, - ); - } catch (err) { - ScorerRegistry.initPromise = null; // allow retry on next call - throw err; - } - })(); - - return ScorerRegistry.initPromise; + const initPromise = (async () => { + ScorerRegistry.registerBuiltInLLMScorers(); + ScorerRegistry.registerBuiltInRuleScorers(); + + ScorerRegistry.initialized = true; + logger.debug( + `Registered ${ScorerRegistry.scorers.size} built-in scorers (including aliases)`, + ); + })(); + + ScorerRegistry.initPromise = initPromise; + + try { + await initPromise; + } catch (err) { + if (ScorerRegistry.initPromise === initPromise) { + ScorerRegistry.initPromise = null; // allow retry on next call + } + ScorerRegistry.initialized = false; + throw err; + } + + return initPromise;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/evaluation/scorers/scorerRegistry.ts` around lines 467 - 482, The current async IIFE assigned to ScorerRegistry.initPromise stores a rejected promise on error and blocks retries; fix by creating a local promise (e.g., const initPromise = (async () => { ... })()), assign it to ScorerRegistry.initPromise before awaiting it, then await initPromise; in the catch handler check if (ScorerRegistry.initPromise === initPromise) before resetting ScorerRegistry.initPromise = null to avoid races, and also set ScorerRegistry.initialized = false on error; keep registerBuiltInLLMScorers and registerBuiltInRuleScorers calls inside the local async, and only set initialized = true after successful completion.src/lib/services/server/ai/observability/instrumentation.ts (1)
626-637:⚠️ Potential issue | 🟠 MajorAllow external-mode initialization to retry after an abort.
Both branches set
isInitialized = trueeven though no processor/provider was created. Once that flips, laterinitializeOpenTelemetry()calls only update config and never retry setup, so one missing-credentials startup or transient processor-construction failure can permanently disable observability for the rest of the process.Also applies to: 716-725
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/services/server/ai/observability/instrumentation.ts` around lines 626 - 637, The abort path in initializeOpenTelemetry() currently flips isInitialized = true when langfuseRequested && !hasLangfuseCreds (and the similar branch at the other location), preventing future retries; change these abort branches (the blocks that log the warning about missing credentials/OTLP endpoint) to set isCredentialsValid = false but do NOT set isInitialized = true so the next initializeOpenTelemetry() call can attempt setup again; keep the logging and return behavior but leave isInitialized false (and ensure no other code assumes initialization happened when isCredentialsValid is false).
🧹 Nitpick comments (7)
src/lib/providers/openAI.ts (1)
698-700: Consider usingErrorFactoryfor consistent typed errors.The error thrown here uses a raw
new Error(...)instead ofErrorFactory. As per coding guidelines, typed errors viaErrorFactoryshould be preferred for consistency across the codebase.♻️ Suggested refactor
- throw new Error( - `OpenAI streaming error with tools: ${errorMessage}. Try disabling tools with --disableTools`, - ); + throw this.formatProviderError( + new Error( + `OpenAI streaming error with tools: ${errorMessage}. Try disabling tools with --disableTools`, + ), + );This would wrap the error through
formatProviderError, which returns a typedProviderErrorfor consistency with other error paths in this provider.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/providers/openAI.ts` around lines 698 - 700, Replace the raw throw new Error(...) with the standardized typed error path: call formatProviderError (which uses ErrorFactory) to construct a ProviderError and throw that instead, preserving the original message ("OpenAI streaming error with tools: ${errorMessage}. Try disabling tools with --disableTools") and attaching any relevant context (errorMessage and provider/tool flags) so other provider error handlers can inspect it; update the throw site in openAI.ts to throw the result of formatProviderError(...) rather than creating a plain Error.src/lib/providers/ollama.ts (1)
2061-2062: Consider caching the type guard result to avoid duplicate checks.
isOllamaHttpError(error)is called twice on the same object. While not a bug, this can be simplified:♻️ Optional: Cache type guard result
- const httpStatus = isOllamaHttpError(error) ? error.statusCode : undefined; - const responseBody = isOllamaHttpError(error) ? error.responseBody : ""; + const ollamaErr = isOllamaHttpError(error) ? error : null; + const httpStatus = ollamaErr?.statusCode; + const responseBody = ollamaErr?.responseBody ?? "";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/providers/ollama.ts` around lines 2061 - 2062, Duplicate calls to the type guard isOllamaHttpError(error) should be avoided; call it once, store the boolean (e.g., const isHttpError = isOllamaHttpError(error)) and then use that variable to set httpStatus and responseBody (e.g., const httpStatus = isHttpError ? error.statusCode : undefined; const responseBody = isHttpError ? error.responseBody : "";), ensuring the variable name references isOllamaHttpError, error, httpStatus and responseBody so the type narrowing still applies.src/lib/tasks/store/redisTaskStore.ts (1)
248-283: Consider extracting duplicated retry logic into a helper.The retry loops for
runsKeyandhistKeyare nearly identical. Extracting this to a small helper would reduce duplication and make the retry parameters (attempts, backoff) easier to tune centrally.♻️ Suggested refactor
+ private async expireWithRetry( + client: RedisClient, + key: string, + ttlSeconds: number, + taskId: string, + keyType: string, + ): Promise<void> { + for (let attempt = 1; attempt <= 3; attempt++) { + try { + await client.expire(key, ttlSeconds); + return; + } catch (err) { + if (attempt === 3) { + logger.warn( + `[TaskStore:Redis] expire failed after 3 attempts on ${keyType} key — task data may outlive retention window`, + { taskId, key, ttlSeconds, err: String(err) }, + ); + } else { + await new Promise((r) => setTimeout(r, 100 * attempt)); + } + } + } + } private applyRetentionTTL(task: Task, client: RedisClient): void { // ... existing early-return logic ... const ttlSeconds = Math.ceil(ttlMs / 1000); - void (async () => { - const runsKey = taskRunsKey(task.id); - for (let attempt = 1; attempt <= 3; attempt++) { - try { - await client.expire(runsKey, ttlSeconds); - break; - } catch (err) { - if (attempt === 3) { - logger.warn( - "[TaskStore:Redis] expire failed after 3 attempts on task runs key — task data may outlive retention window", - { taskId: task.id, key: runsKey, ttlSeconds, err: String(err) }, - ); - } else { - await new Promise((r) => setTimeout(r, 100 * attempt)); - } - } - } - })(); - void (async () => { - const histKey = taskHistoryKey(task.id); - for (let attempt = 1; attempt <= 3; attempt++) { - try { - await client.expire(histKey, ttlSeconds); - break; - } catch (err) { - if (attempt === 3) { - logger.warn( - "[TaskStore:Redis] expire failed after 3 attempts on task history key — task data may outlive retention window", - { taskId: task.id, key: histKey, ttlSeconds, err: String(err) }, - ); - } else { - await new Promise((r) => setTimeout(r, 100 * attempt)); - } - } - } - })(); + void this.expireWithRetry(client, taskRunsKey(task.id), ttlSeconds, task.id, "task runs"); + void this.expireWithRetry(client, taskHistoryKey(task.id), ttlSeconds, task.id, "task history"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/tasks/store/redisTaskStore.ts` around lines 248 - 283, The duplicate retry loops around client.expire for runsKey and histKey should be extracted into a reusable async helper (e.g., retryExpire or withRetry) that accepts the key, ttlSeconds, maxAttempts, and a backoff function; replace the two IIFEs that call taskRunsKey(task.id) and taskHistoryKey(task.id) with calls to that helper, and ensure the helper logs the same logger.warn message (including taskId, key, ttlSeconds, and err) on the final failure and uses the same incremental backoff (100 * attempt) between attempts.src/cli/commands/mcp.ts (1)
3143-3145:argv.detailedisn't reachable from this command.
buildAnnotateOptions()doesn't define a--detailedflag, so this branch never runs. Either wire the option up or drop the condition to avoid dead CLI behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/commands/mcp.ts` around lines 3143 - 3145, The branch checking argv.detailed is dead because buildAnnotateOptions() doesn't define a --detailed flag; either add the flag to the CLI options or remove the unreachable branch. To fix, either (A) add a boolean detailed option in buildAnnotateOptions() so argv.detailed becomes available (ensure parsing sets argv.detailed and update any help/usage text), or (B) remove the if (argv.detailed) conditional and always call logger.always(` ${chalk.gray(tool.description)}`) in the code where logger.always, chalk.gray and tool.description are used; choose one approach and update tests/docs accordingly.src/lib/core/baseProvider.ts (1)
990-1013: Reuse the shared timeout path here.This reintroduces bespoke timeout orchestration inside
generate()instead of the shared helper already onBaseProvider, so timeout semantics and typed errors can drift again.As per coding guidelines "Use ErrorFactory for creating typed errors and wrap async operations with withTimeout utility for error handling".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/core/baseProvider.ts` around lines 990 - 1013, The generate() implementation is reintroducing bespoke timeout orchestration (createTimeoutController, composeAbortSignals, manual cleanup and direct await of executeGeneration) instead of using the shared BaseProvider timeout helper; replace the custom logic with the shared withTimeout-based wrapper (and produce typed errors via ErrorFactory) so timeout semantics and typed errors remain consistent: use the provider's withTimeout (or the BaseProvider helper) to wrap the call to this.executeGeneration(model, messages, tools, options) and ensure the wrapper passes/combines abortSignal correctly and maps timeout/rejection errors to the provider's ErrorFactory-created typed errors (remove createTimeoutController, composeAbortSignals, manual cleanup and direct await in generate()).src/cli/commands/proxy.ts (2)
827-891: Consider adding timeout protection for token refresh operations.The
refreshToken(account)calls (lines 848, 882) make external HTTP requests but have no timeout protection. If the Anthropic token endpoint becomes unresponsive, this background task could hang indefinitely, potentially blocking the refresh interval slot.As per coding guidelines: "wrap async operations with withTimeout utility for error handling."
♻️ Suggested improvement
+import { withTimeout } from "../../lib/utils/timeout.js"; + async function refreshProxyTokensInBackground(): Promise<void> { // ... if (needsRefresh(account)) { - const result = await refreshToken(account); + const result = await withTimeout( + refreshToken(account), + 30_000, + "Token refresh timed out" + ); if (result.success) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/commands/proxy.ts` around lines 827 - 891, The refreshProxyTokensInBackground function calls refreshToken(account) twice without timeout protection; wrap both refreshToken invocations (inside the anthropicKeys loop and the credPath block) with the project's withTimeout utility (e.g., await withTimeout(refreshToken(account), <reasonableMs>)) so the background job cannot hang; ensure you import or reference withTimeout, handle timeout/errors non-fatally the same way existing per-account try/catch does, and only call persistTokens when the timed call returns a successful result.
923-930: Consider adding timeout protection for OpenTelemetry flush during shutdown.The
flushOpenTelemetry()andshutdownOpenTelemetry()calls have no timeout. If the OTLP endpoint is unresponsive, shutdown could hang indefinitely, preventing graceful process termination.♻️ Suggested improvement with timeout
try { const { flushOpenTelemetry, shutdownOpenTelemetry } = await import("../../lib/services/server/ai/observability/instrumentation.js"); - await flushOpenTelemetry(); - await shutdownOpenTelemetry(); + await Promise.race([ + (async () => { + await flushOpenTelemetry(); + await shutdownOpenTelemetry(); + })(), + new Promise((_, reject) => + setTimeout(() => reject(new Error("OTEL shutdown timeout")), 5_000) + ), + ]).catch(() => { + // Timeout or error - proceed with shutdown + }); } catch { // non-fatal — proxy shutdown must not block on OTel }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/commands/proxy.ts` around lines 923 - 930, The shutdown path currently awaits flushOpenTelemetry() and shutdownOpenTelemetry() with no timeout; wrap each call (or the whole sequence) in a timeout-protected promise (e.g., Promise.race against a short timeout) so an unresponsive OTLP endpoint cannot hang shutdown, and on timeout log a warning/error and proceed; apply this around the imported functions flushOpenTelemetry and shutdownOpenTelemetry so the try/catch still prevents throws but also guarantees the process keeps shutting down within the timeout.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs-site/scripts/build-llms-txt.ts`:
- Around line 656-664: The log "Adjusted truncation length to ... to stay within
target size" is printed unconditionally when summaryBuild.truncateChars differs
from SUMMARY_CONTENT_TRUNCATE_CHARS even if the final summary still exceeds the
target; change this so the adjustment message is only logged when the final size
check passes. Specifically, update the code around summaryBuild.truncateChars /
SUMMARY_CONTENT_TRUNCATE_CHARS and summaryStats.size / SUMMARY_MAX_SIZE_KB:
either move the adjusted-truncation console.log to run after the size check or
wrap it in a condition that requires summaryStats.size <= SUMMARY_MAX_SIZE_KB *
1024 (and optionally that truncateChars changed and is >=
SUMMARY_MIN_TRUNCATE_CHARS) so you don't claim "to stay within target size"
unless the final byte size actually meets the limit.
In `@scripts/security-check.ts`:
- Around line 45-46: The check that uses .some() to decide a scan "passed" when
any ignored package appears is incorrect; update the logic in
scripts/security-check.ts (the block that inspects audit results, uses
ignoredPackages and sets status = "passed") so it only returns passed when every
detected vulnerable package is contained in ignoredPackages — e.g., collect the
list of vulnerable package names from the audit output and replace the .some()
test with a subset check (or .every()) that ensures all detected vuln packages
are ignored before setting status = "passed".
In `@src/lib/core/baseProvider.ts`:
- Around line 786-820: The generate method exits early on non-standard paths
(video, image, direct TTS, and early videoFrameResult) before recording success
metrics; update provider.generate to call SpanSerializer.endSpan(...) (and any
performance recording that uses startTime) just before each early return: i.e.,
after handleVideoGeneration return path, after the image flow that calls
executeImageGeneration/enhanceResult, before returning from
handleDirectTTSSynthesis, and when returning videoFrameResult from
handleVideoFrameGeneration; ensure you pass the same span context/startTime and
include success metadata so metrics are recorded consistently for these
branches.
In `@src/lib/core/factory.ts`:
- Around line 203-206: The call to dynamicModelProvider.resolveModel is passing
the literal "default" via modelName which prevents dynamic default resolution;
update the code around dynamicModelProvider.resolveModel to normalize modelName
(or the variable normalizedProvider’s model hint) so that when modelName ===
"default" you pass undefined instead (e.g., compute a lookupModel = modelName
=== "default" ? undefined : modelName) before calling
dynamicModelProvider.resolveModel(normalizedProvider, lookupModel) so
createProvider will trigger the provider's dynamic default path.
In `@src/lib/mcp/toolRegistry.ts`:
- Line 350: The executeTool flow currently swallows the ambiguity error thrown
by resolveToolExecutionTarget (e.g., "Ambiguous tool name") and converts it into
a ToolResult failure; update executeTool (and the similar block around the other
occurrence) to detect and rethrow ambiguity lookup errors instead of returning {
success: false } — for example, check the thrown error's message or type for
"Ambiguous tool name" (in addition to the existing "not found in registry" and
"not executable" checks) and rethrow the original error so callers relying on
thrown lookup failures (resolveToolExecutionTarget) receive the correct
exception.
In `@src/lib/providers/googleVertex.ts`:
- Line 1401: The current logger.info call emits raw tool payloads
(logger.info("Tool execution completed", { toolResults, toolCalls })), which may
contain prompts, secrets, or customer data; replace this with a sanitized
summary: implement a helper (e.g., sanitizeToolPayloads or
buildToolExecutionSummary) that strips/redacts sensitive fields from toolResults
and toolCalls and returns only safe metadata (counts, tool names/ids, status
codes, durations, and coarse error messages), then call logger.info with that
summary instead of the raw objects; ensure the helper is used wherever
toolResults/toolCalls are logged so no raw payloads are emitted.
- Around line 1357-1370: The current early-return only checks analysisSchema and
misses two other structured-output signals; update the guard so it returns when
none of the structured-output indicators are present: replace the existing check
that uses only analysisSchema with a combined condition that tests
(analysisSchema || options.output?.format === "json" || options.schema) so the
code becomes: if (!(analysisSchema || options.output?.format === "json" ||
options.schema)) { return streamOptions; }—this ensures the tool-disabling block
(where streamOptions.tools, streamOptions.toolChoice, and streamOptions.stopWhen
are deleted when !isAnthropic) runs whenever any structured-output signal is
present.
- Around line 1749-1751: The span in executeNativeGemini3StreamWithSpan is being
ended before runNativeGemini3StreamLoop finishes, so subsequent
span.addEvent()/span.setAttribute() calls are lost; fix this by awaiting the
loopPromise (returned by runNativeGemini3StreamLoop) before ending the span or
by moving the span lifecycle outside withClientSpan so the span remains open
until loopPromise resolves; specifically, update
executeNativeGemini3StreamWithSpan to await loopPromise (or ensure
withClientSpan doesn't call span.end() until loopPromise resolves) and keep
references to loopPromise, runNativeGemini3StreamLoop, withClientSpan,
span.addEvent, and span.setAttribute while making this change.
- Around line 1480-1521: The three independent Promise.resolve chains for
result.usage, result.finishReason, and result.text can end the span out of
order; replace them with a coordinated Promise.all (or async/await) that awaits
all three values before calling streamSpan.end(), then set the attributes
(gen_ai.usage.*, neurolink.cost using calculateCost with
getModelId/getDefaultVertexModel and options.model, and
gen_ai.response.finish_reason) after the values are resolved; ensure any error
in the combined await sets streamSpan.setStatus({ code: SpanStatusCode.ERROR,
message: ... }) and still calls streamSpan.end() so the span always closes with
correct attributes or error status.
In `@src/lib/proxy/oauthFetch.ts`:
- Around line 178-218: When enableMcpPrefix is true, also prefix
parsed.tool_choice.name with MCP_TOOL_PREFIX so a pinned tool reference matches
the renamed tools; inside the existing enableMcpPrefix block (near parsed.tools
and parsed.messages handling) add logic to check parsed.tool_choice?.name and,
if present and not already prefixed (use startsWith(MCP_TOOL_PREFIX)), set
parsed.tool_choice.name = `${MCP_TOOL_PREFIX}${parsed.tool_choice.name}`; keep
the existing deletion of parsed.thinking for parsed.tool_choice?.type === "any"
|| "tool".
In `@src/lib/proxy/proxyFetch.ts`:
- Around line 574-593: The proxy currently reconstructs Request into
fetchInput/fetchInit (variables fetchInput, fetchInit) which strips
Request-specific semantics like duplex; instead when input is an instance of
Request pass the original Request object through to undici.fetch (i.e., call
undici.fetch(input, { dispatcher }) or merge only allowed overrides) so the
duplex flag and streaming body are preserved; update the branch that handles
input instanceof Request to avoid creating a new RequestInit with input.body and
ensure undici.fetch receives the original Request instance (or a properly
constructed Request preserving duplex) along with the dispatcher.
In `@src/lib/proxy/sseInterceptor.ts`:
- Around line 199-206: The current truncateUtf8String returns "" when maxBytes
<= 0 or less than the TRUNCATION_MARKER size, which hides the truncation marker;
instead, when maxBytes > 0 but smaller than utf8ByteLength(TRUNCATION_MARKER)
return a byte-aware truncated slice of TRUNCATION_MARKER so at least part of the
marker is visible. Update truncateUtf8String to, in the branch that checks
maxBytes and markerBytes, if maxBytes <= 0 return "" but if 0 < maxBytes <
markerBytes produce a markerPrefix by iterating characters (using utf8ByteLength
to track bytes) until maxBytes is reached and return that markerPrefix; keep the
existing behavior for larger budgets and normal truncation logic.
In `@src/lib/tasks/taskManager.ts`:
- Around line 336-351: The rollback currently calls
restoreScheduledTask(existing, ...) before rollbackTaskUpdate(taskId, existing,
error) which can leave the store inconsistent if rollbackTaskUpdate fails; swap
the two calls so you call await this.rollbackTaskUpdate(taskId, existing, error)
first, then await this.restoreScheduledTask(existing, "update schedule
rollback"); keep the TaskError.create(...) construction and the same
error/details payload (taskId, previousSchedule: existing.schedule,
attemptedSchedule) so the thrown error behavior is unchanged.
- Around line 518-536: The rollbackTaskUpdate currently throws only
rollbackError which hides the original error and prevents callers like resume()
from creating the proper TaskError (e.g., SCHEDULE_FAILED); change
rollbackTaskUpdate to preserve both errors by constructing and throwing a
wrapped error that includes the original error (error) and the rollback error
(rollbackError) as structured context (e.g., message combining both and/or
properties like originalError and rollbackError or using the cause field) so
callers can still detect and rethrow TaskError.create with schedule details;
update references in rollbackTaskUpdate and ensure resume() and other callers
can inspect the wrapped error to generate the correct TaskError.
In `@src/lib/telemetry/telemetryService.ts`:
- Around line 106-111: The delegate-branch check that computes delegateName from
provider.getDelegate()/provider._delegate only excludes "NoopTracerProvider" but
must match the top-level exclusion of both "NoopTracerProvider" and
"ProxyTracerProvider"; update the boolean return to also exclude
"ProxyTracerProvider" (i.e., require delegateName && delegateName !==
"NoopTracerProvider" && delegateName !== "ProxyTracerProvider") so a proxy
wrapper returned by getDelegate() won't be misdetected as an external provider
(referencing provider.getDelegate, provider._delegate, delegateName, and the
provider names).
---
Outside diff comments:
In `@src/lib/evaluation/scorers/scorerRegistry.ts`:
- Around line 467-482: The current async IIFE assigned to
ScorerRegistry.initPromise stores a rejected promise on error and blocks
retries; fix by creating a local promise (e.g., const initPromise = (async () =>
{ ... })()), assign it to ScorerRegistry.initPromise before awaiting it, then
await initPromise; in the catch handler check if (ScorerRegistry.initPromise ===
initPromise) before resetting ScorerRegistry.initPromise = null to avoid races,
and also set ScorerRegistry.initialized = false on error; keep
registerBuiltInLLMScorers and registerBuiltInRuleScorers calls inside the local
async, and only set initialized = true after successful completion.
In `@src/lib/services/server/ai/observability/instrumentation.ts`:
- Around line 626-637: The abort path in initializeOpenTelemetry() currently
flips isInitialized = true when langfuseRequested && !hasLangfuseCreds (and the
similar branch at the other location), preventing future retries; change these
abort branches (the blocks that log the warning about missing credentials/OTLP
endpoint) to set isCredentialsValid = false but do NOT set isInitialized = true
so the next initializeOpenTelemetry() call can attempt setup again; keep the
logging and return behavior but leave isInitialized false (and ensure no other
code assumes initialization happened when isCredentialsValid is false).
---
Nitpick comments:
In `@src/cli/commands/mcp.ts`:
- Around line 3143-3145: The branch checking argv.detailed is dead because
buildAnnotateOptions() doesn't define a --detailed flag; either add the flag to
the CLI options or remove the unreachable branch. To fix, either (A) add a
boolean detailed option in buildAnnotateOptions() so argv.detailed becomes
available (ensure parsing sets argv.detailed and update any help/usage text), or
(B) remove the if (argv.detailed) conditional and always call logger.always(`
${chalk.gray(tool.description)}`) in the code where logger.always, chalk.gray
and tool.description are used; choose one approach and update tests/docs
accordingly.
In `@src/cli/commands/proxy.ts`:
- Around line 827-891: The refreshProxyTokensInBackground function calls
refreshToken(account) twice without timeout protection; wrap both refreshToken
invocations (inside the anthropicKeys loop and the credPath block) with the
project's withTimeout utility (e.g., await withTimeout(refreshToken(account),
<reasonableMs>)) so the background job cannot hang; ensure you import or
reference withTimeout, handle timeout/errors non-fatally the same way existing
per-account try/catch does, and only call persistTokens when the timed call
returns a successful result.
- Around line 923-930: The shutdown path currently awaits flushOpenTelemetry()
and shutdownOpenTelemetry() with no timeout; wrap each call (or the whole
sequence) in a timeout-protected promise (e.g., Promise.race against a short
timeout) so an unresponsive OTLP endpoint cannot hang shutdown, and on timeout
log a warning/error and proceed; apply this around the imported functions
flushOpenTelemetry and shutdownOpenTelemetry so the try/catch still prevents
throws but also guarantees the process keeps shutting down within the timeout.
In `@src/lib/core/baseProvider.ts`:
- Around line 990-1013: The generate() implementation is reintroducing bespoke
timeout orchestration (createTimeoutController, composeAbortSignals, manual
cleanup and direct await of executeGeneration) instead of using the shared
BaseProvider timeout helper; replace the custom logic with the shared
withTimeout-based wrapper (and produce typed errors via ErrorFactory) so timeout
semantics and typed errors remain consistent: use the provider's withTimeout (or
the BaseProvider helper) to wrap the call to this.executeGeneration(model,
messages, tools, options) and ensure the wrapper passes/combines abortSignal
correctly and maps timeout/rejection errors to the provider's
ErrorFactory-created typed errors (remove createTimeoutController,
composeAbortSignals, manual cleanup and direct await in generate()).
In `@src/lib/providers/ollama.ts`:
- Around line 2061-2062: Duplicate calls to the type guard
isOllamaHttpError(error) should be avoided; call it once, store the boolean
(e.g., const isHttpError = isOllamaHttpError(error)) and then use that variable
to set httpStatus and responseBody (e.g., const httpStatus = isHttpError ?
error.statusCode : undefined; const responseBody = isHttpError ?
error.responseBody : "";), ensuring the variable name references
isOllamaHttpError, error, httpStatus and responseBody so the type narrowing
still applies.
In `@src/lib/providers/openAI.ts`:
- Around line 698-700: Replace the raw throw new Error(...) with the
standardized typed error path: call formatProviderError (which uses
ErrorFactory) to construct a ProviderError and throw that instead, preserving
the original message ("OpenAI streaming error with tools: ${errorMessage}. Try
disabling tools with --disableTools") and attaching any relevant context
(errorMessage and provider/tool flags) so other provider error handlers can
inspect it; update the throw site in openAI.ts to throw the result of
formatProviderError(...) rather than creating a plain Error.
In `@src/lib/tasks/store/redisTaskStore.ts`:
- Around line 248-283: The duplicate retry loops around client.expire for
runsKey and histKey should be extracted into a reusable async helper (e.g.,
retryExpire or withRetry) that accepts the key, ttlSeconds, maxAttempts, and a
backoff function; replace the two IIFEs that call taskRunsKey(task.id) and
taskHistoryKey(task.id) with calls to that helper, and ensure the helper logs
the same logger.warn message (including taskId, key, ttlSeconds, and err) on the
final failure and uses the same incremental backoff (100 * attempt) between
attempts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bee57f95-96f8-41f2-b1d2-d00eaa0ea4eb
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (43)
docs-site/config/redirects.tsdocs-site/scripts/build-llms-txt.tsdocs/business-documentation.mddocs/cookbook/rate-limit-handling.mddocs/features/claude-proxy-observability.mddocs/features/claude-proxy.mddocs/reference/provider-selection.mddocs/tutorials.mdpackage.jsonscripts/observability/check-proxy-telemetry.mjsscripts/observability/manage-local-openobserve.shscripts/security-check.tssrc/cli/commands/mcp.tssrc/cli/commands/proxy.tssrc/lib/auth/anthropicOAuth.tssrc/lib/core/baseProvider.tssrc/lib/core/factory.tssrc/lib/evaluation/pipeline/evaluationPipeline.tssrc/lib/evaluation/scorers/scorerRegistry.tssrc/lib/mcp/toolRegistry.tssrc/lib/neurolink.tssrc/lib/providers/googleAiStudio.tssrc/lib/providers/googleVertex.tssrc/lib/providers/litellm.tssrc/lib/providers/ollama.tssrc/lib/providers/openAI.tssrc/lib/proxy/claudeFormat.tssrc/lib/proxy/oauthFetch.tssrc/lib/proxy/proxyConfig.tssrc/lib/proxy/proxyFetch.tssrc/lib/proxy/requestLogger.tssrc/lib/proxy/sseInterceptor.tssrc/lib/server/routes/claudeProxyRoutes.tssrc/lib/services/server/ai/observability/instrumentation.tssrc/lib/tasks/backends/bullmqBackend.tssrc/lib/tasks/store/redisTaskStore.tssrc/lib/tasks/taskManager.tssrc/lib/telemetry/telemetryService.tssrc/lib/types/proxyTypes.tssrc/lib/types/streamTypes.tssrc/lib/utils/providerHealth.tssrc/lib/utils/providerUtils.tstest/continuous-test-suite.ts
💤 Files with no reviewable changes (2)
- docs-site/config/redirects.ts
- src/lib/providers/googleAiStudio.ts
Comment on lines
+656
to
+664
| if (summaryBuild.truncateChars !== SUMMARY_CONTENT_TRUNCATE_CHARS) { | ||
| console.log( | ||
| ` Adjusted truncation length to ${summaryBuild.truncateChars} characters to stay within target size`, | ||
| ); | ||
| } | ||
| if (summaryStats.size > SUMMARY_MAX_SIZE_KB * 1024) { | ||
| console.log(` Warning: Summary exceeds target size of ${SUMMARY_MAX_SIZE_KB}KB`); | ||
| console.log( | ||
| ` Warning: Summary still exceeds target size of ${SUMMARY_MAX_SIZE_KB}KB`, | ||
| ); |
There was a problem hiding this comment.
Don't say the target was met before you verify the final byte size.
If the loop bottoms out at SUMMARY_MIN_TRUNCATE_CHARS and the summary is still oversized, Lines 656-659 say it was adjusted "to stay within target size" immediately before the warning on Lines 661-664. Make that log conditional on the final size check.
♻️ Suggested tweak
const summaryStats = fs.statSync(SUMMARY_OUTPUT);
const summarySizeKB = (summaryStats.size / 1024).toFixed(2);
+ const summaryWithinTarget = summaryStats.size <= SUMMARY_MAX_SIZE_KB * 1024;
console.log(` Output: ${SUMMARY_OUTPUT}`);
console.log(` Size: ${summarySizeKB} KB`);
if (summaryBuild.truncateChars !== SUMMARY_CONTENT_TRUNCATE_CHARS) {
console.log(
- ` Adjusted truncation length to ${summaryBuild.truncateChars} characters to stay within target size`,
+ summaryWithinTarget
+ ? ` Adjusted truncation length to ${summaryBuild.truncateChars} characters to stay within target size`
+ : ` Adjusted truncation length to ${summaryBuild.truncateChars} characters while trying to reach the target size`,
);
}
- if (summaryStats.size > SUMMARY_MAX_SIZE_KB * 1024) {
+ if (!summaryWithinTarget) {
console.log(
` Warning: Summary still exceeds target size of ${SUMMARY_MAX_SIZE_KB}KB`,
);
}📝 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.
Suggested change
| if (summaryBuild.truncateChars !== SUMMARY_CONTENT_TRUNCATE_CHARS) { | |
| console.log( | |
| ` Adjusted truncation length to ${summaryBuild.truncateChars} characters to stay within target size`, | |
| ); | |
| } | |
| if (summaryStats.size > SUMMARY_MAX_SIZE_KB * 1024) { | |
| console.log(` Warning: Summary exceeds target size of ${SUMMARY_MAX_SIZE_KB}KB`); | |
| console.log( | |
| ` Warning: Summary still exceeds target size of ${SUMMARY_MAX_SIZE_KB}KB`, | |
| ); | |
| const summaryStats = fs.statSync(SUMMARY_OUTPUT); | |
| const summarySizeKB = (summaryStats.size / 1024).toFixed(2); | |
| const summaryWithinTarget = summaryStats.size <= SUMMARY_MAX_SIZE_KB * 1024; | |
| console.log(` Output: ${SUMMARY_OUTPUT}`); | |
| console.log(` Size: ${summarySizeKB} KB`); | |
| if (summaryBuild.truncateChars !== SUMMARY_CONTENT_TRUNCATE_CHARS) { | |
| console.log( | |
| summaryWithinTarget | |
| ? ` Adjusted truncation length to ${summaryBuild.truncateChars} characters to stay within target size` | |
| : ` Adjusted truncation length to ${summaryBuild.truncateChars} characters while trying to reach the target size`, | |
| ); | |
| } | |
| if (!summaryWithinTarget) { | |
| console.log( | |
| ` Warning: Summary still exceeds target size of ${SUMMARY_MAX_SIZE_KB}KB`, | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs-site/scripts/build-llms-txt.ts` around lines 656 - 664, The log
"Adjusted truncation length to ... to stay within target size" is printed
unconditionally when summaryBuild.truncateChars differs from
SUMMARY_CONTENT_TRUNCATE_CHARS even if the final summary still exceeds the
target; change this so the adjustment message is only logged when the final size
check passes. Specifically, update the code around summaryBuild.truncateChars /
SUMMARY_CONTENT_TRUNCATE_CHARS and summaryStats.size / SUMMARY_MAX_SIZE_KB:
either move the adjusted-truncation console.log to run after the size check or
wrap it in a condition that requires summaryStats.size <= SUMMARY_MAX_SIZE_KB *
1024 (and optionally that truncateChars changed and is >=
SUMMARY_MIN_TRUNCATE_CHARS) so you don't claim "to stay within target size"
unless the final byte size actually meets the limit.
Comment on lines
+45
to
+46
| "lodash", // TODO: track in #xxx — no patch available upstream for @semantic-release dev dep | ||
| "lodash-es", // TODO: track in #xxx — no patch available upstream for @semantic-release dev dep |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the pass condition is currently "any ignored package present"
rg -n -C4 'isIgnoredPackage|some\(|results\.dependencies\.status = "passed"|return;' scripts/security-check.tsRepository: juspay/neurolink
Length of output: 2628
🏁 Script executed:
# Verify lines 45-46 show the IGNORED_VULNERABLE_PACKAGES constant
sed -n '40,55p' scripts/security-check.tsRepository: juspay/neurolink
Length of output: 691
🏁 Script executed:
# Check full vulnerability detection and filtering logic to ensure no other guards exist
sed -n '145,210p' scripts/security-check.tsRepository: juspay/neurolink
Length of output: 2525
Ignored-package expansion can incorrectly pass audits with real vulnerabilities
The current logic at line 148–166 uses .some() to check if ANY ignored package exists in the audit output. If true, the scan immediately returns with status = "passed" at line 165, skipping the severity check entirely. This means adding lodash and lodash-es (common packages) will cause any audit containing them to pass, even if non-ignored high/critical vulnerabilities are also present.
Suggested fix (only pass when all detected vulnerable packages are ignored)
- const isIgnoredPackage = IGNORED_VULNERABLE_PACKAGES.some(
- (pkg) =>
- output.includes(`│ Package │ ${pkg}`) ||
- output.includes(`Package: ${pkg}`),
- );
-
- if (isIgnoredPackage) {
+ const detectedPackages = Array.from(
+ new Set(
+ [...output.matchAll(/Package:\s+([^\s]+)/g)].map((m) => m[1]),
+ ),
+ );
+ const allDetectedAreIgnored =
+ detectedPackages.length > 0 &&
+ detectedPackages.every((pkg) =>
+ IGNORED_VULNERABLE_PACKAGES.includes(pkg),
+ );
+
+ if (allDetectedAreIgnored) {
const ignoredList = IGNORED_VULNERABLE_PACKAGES.join(", ");
this.log(
`Found vulnerabilities in temporarily ignored packages: ${ignoredList}`,
"cyan",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/security-check.ts` around lines 45 - 46, The check that uses .some()
to decide a scan "passed" when any ignored package appears is incorrect; update
the logic in scripts/security-check.ts (the block that inspects audit results,
uses ignoredPackages and sets status = "passed") so it only returns passed when
every detected vulnerable package is contained in ignoredPackages — e.g.,
collect the list of vulnerable package names from the audit output and replace
the .some() test with a subset check (or .every()) that ensures all detected
vuln packages are ignored before setting status = "passed".
Comment on lines
+786
to
+820
| try { | ||
| if (options.output?.mode === "video") { | ||
| return await this.handleVideoGeneration(options, startTime); | ||
| } | ||
|
|
||
| const isImageModel = IMAGE_GENERATION_MODELS.some((m) => | ||
| this.modelName.includes(m), | ||
| ); | ||
| if (isImageModel) { | ||
| logger.info( | ||
| `Image generation model detected, routing to executeImageGeneration`, | ||
| { | ||
| provider: this.providerName, | ||
| model: this.modelName, | ||
| }, | ||
| ); | ||
|
|
||
| if (isImageModel) { | ||
| logger.info( | ||
| `Image generation model detected, routing to executeImageGeneration`, | ||
| { | ||
| provider: this.providerName, | ||
| model: this.modelName, | ||
| }, | ||
| ); | ||
| const imageResult = await this.executeImageGeneration(options); | ||
| return await this.enhanceResult(imageResult, options, startTime); | ||
| } | ||
|
|
||
| const imageResult = await this.executeImageGeneration(options); | ||
| return await this.enhanceResult(imageResult, options, startTime); | ||
| } | ||
| if (options.tts?.enabled && !options.tts?.useAiResponse) { | ||
| return this.handleDirectTTSSynthesis(options, startTime); | ||
| } | ||
|
|
||
| // ===== TTS MODE 1: Direct Input Synthesis (useAiResponse=false) ===== | ||
| // Synthesize input text directly without AI generation | ||
| // This is optimal for simple read-aloud scenarios | ||
| if (options.tts?.enabled && !options.tts?.useAiResponse) { | ||
| const textToSynthesize = options.prompt ?? options.input?.text ?? ""; | ||
| const { tools, model } = await this.prepareGenerationContext(options); | ||
| const messages = await this.buildMessages(options); | ||
| const videoFrameResult = await this.handleVideoFrameGeneration( | ||
| options, | ||
| messages, | ||
| model, | ||
| startTime, | ||
| ); | ||
| if (videoFrameResult) { | ||
| return videoFrameResult; |
There was a problem hiding this comment.
End provider.generate metrics on the non-standard return paths.
The branches returning at Lines 788, 804, 808, and 820 exit before the standard flow records success metrics, so direct TTS / image / video requests now miss the success SpanSerializer.endSpan(...) and performance recording.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/core/baseProvider.ts` around lines 786 - 820, The generate method
exits early on non-standard paths (video, image, direct TTS, and early
videoFrameResult) before recording success metrics; update provider.generate to
call SpanSerializer.endSpan(...) (and any performance recording that uses
startTime) just before each early return: i.e., after handleVideoGeneration
return path, after the image flow that calls
executeImageGeneration/enhanceResult, before returning from
handleDirectTTSSynthesis, and when returning videoFrameResult from
handleVideoFrameGeneration; ensure you pass the same span context/startTime and
include success metadata so metrics are recorded consistently for these
branches.
Comment on lines
+203
to
+206
| const dynamicModel = dynamicModelProvider.resolveModel( | ||
| normalizedProvider, | ||
| modelName || undefined, | ||
| ); |
There was a problem hiding this comment.
Normalize "default" before dynamic model lookup.
dynamicModelProvider.resolveModel() only picks the provider default when modelHint is undefined. Passing the literal "default" here makes it do an exact/fuzzy lookup instead, so createProvider(..., "default") skips dynamic default resolution and falls back to the static provider default.
💡 Proposed fix
const dynamicModel = dynamicModelProvider.resolveModel(
normalizedProvider,
- modelName || undefined,
+ modelName && modelName !== "default" ? modelName : undefined,
);📝 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.
Suggested change
| const dynamicModel = dynamicModelProvider.resolveModel( | |
| normalizedProvider, | |
| modelName || undefined, | |
| ); | |
| const dynamicModel = dynamicModelProvider.resolveModel( | |
| normalizedProvider, | |
| modelName && modelName !== "default" ? modelName : undefined, | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/core/factory.ts` around lines 203 - 206, The call to
dynamicModelProvider.resolveModel is passing the literal "default" via modelName
which prevents dynamic default resolution; update the code around
dynamicModelProvider.resolveModel to normalize modelName (or the variable
normalizedProvider’s model hint) so that when modelName === "default" you pass
undefined instead (e.g., compute a lookupModel = modelName === "default" ?
undefined : modelName) before calling
dynamicModelProvider.resolveModel(normalizedProvider, lookupModel) so
createProvider will trigger the provider's dynamic default path.
| } | ||
| } | ||
|
|
||
| const { tool, toolId } = this.resolveToolExecutionTarget(toolName); |
There was a problem hiding this comment.
Ambiguous lookups are still falling through the normal ToolResult path.
resolveToolExecutionTarget() now throws on duplicate names, but executeTool() only rethrows "not found in registry" / "not executable" precondition failures. This new ambiguity error therefore gets converted into { success: false }, so callers that treat a resolved executeTool() value as success — e.g. the exec flow in src/cli/commands/mcp.ts — won't surface the lookup failure correctly.
💡 Suggested follow-up in executeTool()
if (
errMsg.includes("not found in registry") ||
errMsg.includes("not executable") ||
errMsg.includes("Ambiguous tool name")
) {
throw error;
}Also applies to: 668-689
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/mcp/toolRegistry.ts` at line 350, The executeTool flow currently
swallows the ambiguity error thrown by resolveToolExecutionTarget (e.g.,
"Ambiguous tool name") and converts it into a ToolResult failure; update
executeTool (and the similar block around the other occurrence) to detect and
rethrow ambiguity lookup errors instead of returning { success: false } — for
example, check the thrown error's message or type for "Ambiguous tool name" (in
addition to the existing "not found in registry" and "not executable" checks)
and rethrow the original error so callers relying on thrown lookup failures
(resolveToolExecutionTarget) receive the correct exception.
Comment on lines
+574
to
+593
| let fetchInput: string | URL; | ||
| let fetchInit = { ...init }; | ||
|
|
||
| if (input instanceof Request) { | ||
| fetchInput = input.url; | ||
| fetchInit = { | ||
| method: input.method, | ||
| headers: input.headers, | ||
| body: input.body, | ||
| ...init, | ||
| }; | ||
| } else { | ||
| fetchInput = input; | ||
| } | ||
|
|
||
| try { | ||
| const response = await fetchWithRetry(input, init); | ||
| const undici = await import("undici"); | ||
| const response = await undici.fetch(fetchInput, { | ||
| ...fetchInit, | ||
| dispatcher, | ||
| } as unknown as import("undici").RequestInit); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Undici / Node.js fetch, if I already have new Request(url, { method: "POST", body: someReadableStream, duplex: "half" }), is it safe to proxy it via fetch(req.url, { method: req.method, headers: req.headers, body: req.body }), or do I need to forward the original Requestobject (or re-setduplex: "half" explicitly)?
💡 Result:
No, it is not safe to proxy via fetch(req.url, { method: req.method, headers: req.headers, body: req.body }) without explicitly setting duplex: "half". You need to either forward the original Request object directly with fetch(req) or explicitly set duplex: "half" in the options when using individual properties. The duplex property is required by the Fetch spec (and enforced in Node.js/Undici) when the request body is a ReadableStream. The original req has duplex: "half" set during construction. However, when passing individual init properties to fetch, the duplex option is not automatically copied from req.duplex or preserved. Omitting it will throw "RequestInit: duplex option is required when sending a body". Evidence from sources: - Node.js issue #46221: Explicitly requires duplex: "half" for stream bodies, with proxy/pipe examples adding it manually. - Undici issue #1626: Proxy examples use new Request(newUrl, { ..., body: req.body, duplex: "half" }). - WHATWG Fetch spec via MDN: duplex must be set for ReadableStream bodies. - Multiple PRs/issues (e.g., Undici #1681) confirm duplex addition and enforcement. Safest options: 1. fetch(req) - Forwards the entire Request, preserving duplex. 2. fetch(newUrl, { ...req, duplex: "half" }) - Explicitly sets it. 3. fetch(newUrl, { method: req.method, headers: req.headers, body: req.body, duplex: "half" }) - Manual copy with duplex.
Citations:
- 1: RequestInit: duplex option is required when sending a body nodejs/node#46221
- 2: https://developer.mozilla.org/en-US/docs/Web/API/Request/duplex
- 3: Does fetch support piping body into request? nodejs/undici#1626
- 4: feat(fetch): add
Request{Init}.duplexand add WPTs nodejs/undici#1681
Preserve the original Request when proxying to avoid losing required duplex semantics.
When input is a Request, the current code reconstructs it as separate properties (body: input.body, etc.) passed to undici.fetch(). This loses the duplex property, which is required by the WHATWG Fetch spec and enforced in Node.js/Undici when the request body is a ReadableStream. Omitting it will throw RequestInit: duplex option is required when sending a body at runtime.
Pass the original Request object directly to preserve all streaming semantics:
Suggested direction
- let fetchInput: string | URL;
- let fetchInit = { ...init };
-
- if (input instanceof Request) {
- fetchInput = input.url;
- fetchInit = {
- method: input.method,
- headers: input.headers,
- body: input.body,
- ...init,
- };
- } else {
- fetchInput = input;
- }
+ const fetchInput = input;
+ const fetchInit = { ...init };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/proxy/proxyFetch.ts` around lines 574 - 593, The proxy currently
reconstructs Request into fetchInput/fetchInit (variables fetchInput, fetchInit)
which strips Request-specific semantics like duplex; instead when input is an
instance of Request pass the original Request object through to undici.fetch
(i.e., call undici.fetch(input, { dispatcher }) or merge only allowed overrides)
so the duplex flag and streaming body are preserved; update the branch that
handles input instanceof Request to avoid creating a new RequestInit with
input.body and ensure undici.fetch receives the original Request instance (or a
properly constructed Request preserving duplex) along with the dispatcher.
Comment on lines
+199
to
+206
| function truncateUtf8String(input: string, maxBytes: number): string { | ||
| if (utf8ByteLength(input) <= maxBytes) { | ||
| return input; | ||
| } | ||
| return `${input.slice(0, maxBytes)}${TRUNCATION_MARKER}`; | ||
|
|
||
| const markerBytes = utf8ByteLength(TRUNCATION_MARKER); | ||
| if (maxBytes <= 0 || maxBytes < markerBytes) { | ||
| return ""; |
There was a problem hiding this comment.
Keep the truncation marker visible when only a few bytes remain.
Returning "" here means the last overflow can silently cut data once the remaining budget is smaller than TRUNCATION_MARKER. Because both capped block content and raw-text capture reuse this helper, the final payload can look complete even though it was truncated.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/proxy/sseInterceptor.ts` around lines 199 - 206, The current
truncateUtf8String returns "" when maxBytes <= 0 or less than the
TRUNCATION_MARKER size, which hides the truncation marker; instead, when
maxBytes > 0 but smaller than utf8ByteLength(TRUNCATION_MARKER) return a
byte-aware truncated slice of TRUNCATION_MARKER so at least part of the marker
is visible. Update truncateUtf8String to, in the branch that checks maxBytes and
markerBytes, if maxBytes <= 0 return "" but if 0 < maxBytes < markerBytes
produce a markerPrefix by iterating characters (using utf8ByteLength to track
bytes) until maxBytes is reached and return that markerPrefix; keep the existing
behavior for larger budgets and normal truncation logic.
Comment on lines
+336
to
+351
| } catch (error) { | ||
| await this.restoreScheduledTask(existing, "update schedule rollback"); | ||
| await this.rollbackTaskUpdate(taskId, existing, error); | ||
| throw TaskError.create( | ||
| "SCHEDULE_FAILED", | ||
| `Failed to update schedule for task ${taskId}`, | ||
| { | ||
| cause: error instanceof Error ? error : undefined, | ||
| details: { | ||
| taskId, | ||
| previousSchedule: existing.schedule, | ||
| attemptedSchedule, | ||
| }, | ||
| }, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Consider reversing rollback order: store before schedule.
Currently, restoreScheduledTask is called before rollbackTaskUpdate. If restoreScheduledTask succeeds but rollbackTaskUpdate fails (and throws per line 534), the store will contain the new task state while the backend has the old schedule—causing a mismatch.
Rolling back the store first ensures the persisted state is consistent before attempting to restore the backend schedule.
🔧 Suggested fix
} catch (error) {
- await this.restoreScheduledTask(existing, "update schedule rollback");
await this.rollbackTaskUpdate(taskId, existing, error);
+ await this.restoreScheduledTask(existing, "update schedule rollback");
throw TaskError.create(📝 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.
Suggested change
| } catch (error) { | |
| await this.restoreScheduledTask(existing, "update schedule rollback"); | |
| await this.rollbackTaskUpdate(taskId, existing, error); | |
| throw TaskError.create( | |
| "SCHEDULE_FAILED", | |
| `Failed to update schedule for task ${taskId}`, | |
| { | |
| cause: error instanceof Error ? error : undefined, | |
| details: { | |
| taskId, | |
| previousSchedule: existing.schedule, | |
| attemptedSchedule, | |
| }, | |
| }, | |
| ); | |
| } | |
| } catch (error) { | |
| await this.rollbackTaskUpdate(taskId, existing, error); | |
| await this.restoreScheduledTask(existing, "update schedule rollback"); | |
| throw TaskError.create( | |
| "SCHEDULE_FAILED", | |
| `Failed to update schedule for task ${taskId}`, | |
| { | |
| cause: error instanceof Error ? error : undefined, | |
| details: { | |
| taskId, | |
| previousSchedule: existing.schedule, | |
| attemptedSchedule, | |
| }, | |
| }, | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/tasks/taskManager.ts` around lines 336 - 351, The rollback currently
calls restoreScheduledTask(existing, ...) before rollbackTaskUpdate(taskId,
existing, error) which can leave the store inconsistent if rollbackTaskUpdate
fails; swap the two calls so you call await this.rollbackTaskUpdate(taskId,
existing, error) first, then await this.restoreScheduledTask(existing, "update
schedule rollback"); keep the TaskError.create(...) construction and the same
error/details payload (taskId, previousSchedule: existing.schedule,
attemptedSchedule) so the thrown error behavior is unchanged.
Comment on lines
+518
to
+536
| private async rollbackTaskUpdate( | ||
| taskId: string, | ||
| previousTask: Task, | ||
| error: unknown, | ||
| ): Promise<Task> { | ||
| try { | ||
| return await this.getStore().update(taskId, previousTask); | ||
| } catch (rollbackError) { | ||
| logger.error( | ||
| "[TaskManager] Failed to roll back task update — store and in-memory state may be diverged; manual reconciliation required", | ||
| { | ||
| taskId, | ||
| originalError: String(error), | ||
| rollbackError: String(rollbackError), | ||
| }, | ||
| ); | ||
| throw rollbackError; | ||
| } | ||
| } |
There was a problem hiding this comment.
Rollback failure swallows the original error context.
When rollbackTaskUpdate throws rollbackError (line 534), callers like resume() never reach their throw TaskError.create(...) lines. The user receives a generic store error instead of the typed SCHEDULE_FAILED error with schedule details.
Consider wrapping both errors to preserve context:
🔧 Suggested fix
private async rollbackTaskUpdate(
taskId: string,
previousTask: Task,
error: unknown,
): Promise<Task> {
try {
return await this.getStore().update(taskId, previousTask);
} catch (rollbackError) {
logger.error(
"[TaskManager] Failed to roll back task update — store and in-memory state may be diverged; manual reconciliation required",
{
taskId,
originalError: String(error),
rollbackError: String(rollbackError),
},
);
- throw rollbackError;
+ throw TaskError.create(
+ "SCHEDULE_FAILED",
+ `Rollback failed after scheduling error for task ${taskId}`,
+ {
+ cause: error instanceof Error ? error : undefined,
+ details: { taskId, rollbackError: String(rollbackError) },
+ },
+ );
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/tasks/taskManager.ts` around lines 518 - 536, The rollbackTaskUpdate
currently throws only rollbackError which hides the original error and prevents
callers like resume() from creating the proper TaskError (e.g.,
SCHEDULE_FAILED); change rollbackTaskUpdate to preserve both errors by
constructing and throwing a wrapped error that includes the original error
(error) and the rollback error (rollbackError) as structured context (e.g.,
message combining both and/or properties like originalError and rollbackError or
using the cause field) so callers can still detect and rethrow TaskError.create
with schedule details; update references in rollbackTaskUpdate and ensure
resume() and other callers can inspect the wrapped error to generate the correct
TaskError.
Comment on lines
+106
to
+111
| const delegate = | ||
| typeof provider.getDelegate === "function" | ||
| ? provider.getDelegate() | ||
| : provider._delegate; | ||
| const delegateName = delegate?.constructor?.name || ""; | ||
| return Boolean(delegateName && delegateName !== "NoopTracerProvider"); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In OpenTelemetry JS, before any SDK tracer provider is registered, can trace.getTracerProvider().getDelegate?.()return anotherProxyTracerProvider, and should that still be treated as the default no-op path rather than an external provider?
💡 Result:
Yes, before any SDK TracerProvider is registered via trace.setGlobalTracerProvider, calling trace.getTracerProvider returns a ProxyTracerProvider. This ProxyTracerProvider has a getDelegate?. method that returns its internal delegate, which defaults to a no-op TracerProvider (such as NoopTracerProvider) until an SDK provider is set. This should be treated as the default no-op path, not an external provider, because the delegate is the built-in no-op implementation from @opentelemetry/api, ensuring no telemetry is emitted until a real SDK is registered. Once an SDK calls register or setGlobalTracerProvider, the proxy delegates to that real provider, activating early-acquired tracers without reinitialization.
Citations:
- 1: https://oneuptime.com/blog/post/2026-02-06-debug-opentelemetry-sdk-initialization-order/view
- 2: Instrumentation: JS: Consider Placing the registerInstrumentations Call after the provider.register Call open-telemetry/opentelemetry.io#2596
- 3: https://github.com/open-telemetry/opentelemetry-js/blob/main/api/src/trace/ProxyTracerProvider.ts
- 4: Add suport to flush via API open-telemetry/opentelemetry-js#3310
- 5: feat: tracers provided by the API become useable when provider registered open-telemetry/opentelemetry-js#1448
🏁 Script executed:
# First, locate the telemetry service file
fd -t f telemetryService.tsRepository: juspay/neurolink
Length of output: 97
🏁 Script executed:
# Read the relevant section of the file
cat -n src/lib/telemetry/telemetryService.ts | sed -n '95,125p'Repository: juspay/neurolink
Length of output: 1286
🏁 Script executed:
# Search for related delegate and provider checks in the codebase
rg -A 5 -B 5 "hasExternalTracerProvider|getDelegate" src/lib/telemetry/Repository: juspay/neurolink
Length of output: 3845
🏁 Script executed:
# Look for tests related to external provider detection
fd -t f -i "test|spec" | xargs rg -l "hasExternalTracerProvider|ProxyTracerProvider" 2>/dev/null | head -20Repository: juspay/neurolink
Length of output: 42
Align delegate-branch provider-name exclusions with top-level check.
The top-level branch excludes both ProxyTracerProvider and NoopTracerProvider, but the delegate branch only excludes NoopTracerProvider. If getDelegate() ever returns another proxy wrapper, this inconsistency causes a false-positive external-provider detection and unnecessary provider replacement.
🔧 Proposed fix
- return Boolean(delegateName && delegateName !== "NoopTracerProvider");
+ return Boolean(
+ delegateName &&
+ delegateName !== "NoopTracerProvider" &&
+ delegateName !== "ProxyTracerProvider",
+ );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/telemetry/telemetryService.ts` around lines 106 - 111, The
delegate-branch check that computes delegateName from
provider.getDelegate()/provider._delegate only excludes "NoopTracerProvider" but
must match the top-level exclusion of both "NoopTracerProvider" and
"ProxyTracerProvider"; update the boolean return to also exclude
"ProxyTracerProvider" (i.e., require delegateName && delegateName !==
"NoopTracerProvider" && delegateName !== "ProxyTracerProvider") so a proxy
wrapper returned by getDelegate() won't be misdetected as an external provider
(referencing provider.getDelegate, provider._delegate, delegateName, and the
provider names).
…corer definitions and unit tests - Decompose claudeProxyRoutes.ts: extract 20+ named handlers (handleTranslatedClaudeRequest, handleClaudePassthroughRequest, loadClaudeProxyAccounts, buildAnthropicTerminalErrorResponse, createAnthropicAttemptLogger, prepareAnthropicAccountAttempt, etc.) to resolve max-lines-per-function - Decompose neurolink.ts: extract executeGenerateWithMetricsContext, prepareGenerateRequest, buildGenerateTextOptions, maybeHandleEarlyGenerateResult, and 8+ more private methods - Decompose proxy CLI: extract ensureProxyStartAllowed, loadProxyStartEnv, createProxyNeurolinkRuntime - Decompose baseProvider: extract runGenerateInActiveContext private method, add gen() alias - Decompose GoogleVertex: extract maybeExecuteNativeGemini3ToolStream and executeAISDKStream - Add AIProviderFactory.resolveModelFromEnvironment for per-provider model env var overrides - Refactor MCPToolRegistry: extract resolveToolExecutionTarget and createExecutionContext helpers - Refactor oauthFetch: extract resolveOAuthRequestUrl helper, clean up request flow - Fix proxyFetch: add mergeTraceHeaders, pass input to injectTraceContext to preserve request headers - Fix OpenAI: resolve toolChoice once before stream creation to avoid duplicate computation - Fix TaskManager: cleanup callbacks before store delete, rollback on schedule failure, defer history clear - Add built-in LLM scorer definitions to ScorerRegistry (hallucination, toxicity, and more) - Add new proxy types: ProxyBodyCaptureInput, ClaudeFinalRequestLogger, ClaudeLoggedErrorBuilder - Update observability instrumentation, request logger, docs site, and dependencies - Ignore pre-existing lodash vulnerabilities in @semantic-release dev deps (no patch available)
murdore
force-pushed
the
refactor/decompose-proxy-routes-neurolink-core
branch
from
April 2, 2026 07:39
1098d44 to
8e88aa5
Compare
Contributor
✅ 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 |
Contributor
🤖 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 |
Contributor
|
🎉 This PR is included in version 9.43.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This was referenced Apr 8, 2026
8 of 10 tasks
7 tasks done
10 tasks done
5 tasks
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #918. Three CodeRabbit comments were posted outside the diff context (not as inline threads) and were missed in Cycle 1. This PR resolves them along with two nitpicks.
requestLogger.tsL642–654 — OTLP body log silently dropped whenwriteBodyArtifactthrows; populatedstoredfrompreparedBodyin the catch block so OTLP logging proceeds via memory fallbackmcp.tsL3033–3046 —findToolForAnnotation()silently picked first match in multi-server ambiguity; now returns"ambiguous"and caller exits with a clear--server <id>messagescorerRegistry.tsL467–479 —try-finallywithoutcatchleftinitPromisepermanently rejected on failure; addedcatchthat resetsinitPromise = nullbefore rethrowing to allow retryredisTaskStore.tsL257** —expire()` failure was only warn-logged; added 3-attempt retry with 100ms/200ms backoff before warn logfactory.ts(nitpick) —withTimeoutcalls used rawnew Error; replaced withErrorFactory.toolTimeout(...)for consistent typed errorsscripts/security-check.ts(nitpick) — added#xxxissue-tracking placeholder to lodash ignore entriesTest plan
Summary by CodeRabbit
Release Notes
Documentation
@juspay/neurolinkCLI in setup guides.New Features
Bug Fixes
Chores