feat(proxy): OTLP observability, passthrough mode, env-file, and bug fixes - #915
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
✅ 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 |
Documentation Validation Results🚀 Documentation validation passed!
📦 Build artifact uploaded successfully. Ready for deployment preview. Commit: |
There was a problem hiding this comment.
Pull request overview
Adds first-class observability and new operating modes to the Claude-compatible proxy, alongside provider fallback/health improvements and broad docs updates.
Changes:
- Add OTLP “triple-signal” (traces/metrics/logs) export for proxy requests, including SSE stream telemetry extraction and request span lifecycle tracing.
- Introduce proxy
--passthroughand--env-fileflags, plus improvements to fallback ordering and provider health checks (LiteLLM/Ollama). - Apply targeted bug fixes (lifecycle callback clobbering, MCP init order, redis client import) and update documentation accordingly.
Reviewed changes
Copilot reviewed 45 out of 46 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/lib/server/routes/claudeProxyRoutes.ts | Adds passthrough mode, tracing hooks, SSE interception, and fallback availability checks. |
| src/lib/proxy/proxyTracer.ts | New request-lifecycle tracer (spans/metrics/correlation helpers). |
| src/lib/proxy/sseInterceptor.ts | New TransformStream to parse Anthropic SSE for usage/model telemetry without altering bytes. |
| src/lib/proxy/requestLogger.ts | Adds OTLP log record emission + trace correlation fields in structured logs. |
| src/lib/services/server/ai/observability/instrumentation.ts | Adds OTLP HTTP exporters for traces/metrics/logs; registers global MeterProvider; exposes LoggerProvider. |
| src/lib/telemetry/telemetryService.ts | Reuses existing global TracerProvider when present; avoids duplicate registration; adjusts shutdown behavior. |
| src/lib/telemetry/index.ts | Updates telemetry init description to reflect external provider reuse + exporter-based enablement. |
| src/cli/commands/proxy.ts | Adds --env-file and --passthrough; loads env file; initializes OTel; persists envFile/passthrough in state; flushes OTel on shutdown. |
| src/lib/proxy/proxyEnv.ts | New env-file resolution/loading helper (CLI/env/default path). |
| src/lib/proxy/proxyConfig.ts | Allows routing-only configs; guards accounts parsing when accounts are omitted. |
| src/lib/proxy/oauthFetch.ts | Makes injected Anthropic billing block deterministic and preserves prompt caching prefix chain. |
| src/lib/utils/providerHealth.ts | Adds LiteLLM health checks; improves Ollama base URL resolution and model validation helpers; adds fallback availability helper. |
| src/lib/utils/providerUtils.ts | Reorders default provider priority to prefer self-hosted (LiteLLM/Ollama) and improves Ollama model matching. |
| src/lib/providers/litellm.ts | Fixes LiteLLM model construction to use chat-capable OpenAI-compatible model. |
| src/lib/providers/ollama.ts | Makes default model env-configurable; improves usage accounting and error classification for 404s. |
| src/lib/types/proxyTypes.ts | Extends request log schema with cache token fields + traceId/spanId correlation fields. |
| src/lib/types/cli.ts | Extends CLI types/state to include envFile and passthrough mode metadata. |
| src/lib/neurolink.ts | Fixes lifecycle callback clobbering by conditionally spreading provided callbacks only. |
| src/lib/auth/sessionManager.ts | Migrates Redis session storage import/initialization from ioredis to node-redis. |
| src/cli/commands/mcp.ts | Ensures MCP is initialized before listing/testing/exec/remove flows. |
| scripts/build-browser.mjs | Updates browser build stubs for added OTel logs/metrics packages. |
| package.json | Adds OTel logs/metrics dependencies needed for OTLP triple-signal export. |
| pnpm-lock.yaml | Lockfile updates for newly added OTel dependencies (and checksum change). |
| docs/telemetry-guide.md | Documents proxy OTLP triple-signal export and trace correlation in logs. |
| docs/features/observability.md | Adds proxy observability section; notes TracerProvider reuse behavior. |
| docs/features/claude-proxy.md | Documents new proxy CLI flags (--passthrough, --env-file). |
| docs/features/claude-proxy-config-reference.md | Documents new proxy CLI flags in config reference. |
| docs/features/claude-proxy-architecture.md | Documents SSE interceptor + tracer wiring and OTel lifecycle. |
| docs/features/claude-proxy-troubleshooting.md | Adds guidance for correlating JSONL logs with traces (traceId/spanId). |
| docs/features/provider-orchestration.md | Updates guidance to “self-hosted first” fallback ordering. |
| docs/reference/provider-selection.md | Updates sample provider priority order to include LiteLLM/Ollama earlier. |
| docs/guides/enterprise/multi-provider-failover.md | Updates failover recommendations to prioritize self-hosted providers. |
| docs/cookbook/multi-provider-fallback.md | Updates fallback examples and explains self-hosted-first default ordering. |
| docs/getting-started/providers/ollama.md | Documents prefix model matching behavior and env override usage. |
| docs/real-time-services.md | Notes lifecycle callback preservation through middleware. |
| docs/advanced/mcp-integration.md | Documents calling getMCPStatus() prior to listMCPServers(). |
| docs/advanced/auth-architecture.md | Updates Redis storage docs from ioredis to node-redis. |
| docs/sdk/nestjs-integration.md | Updates redis cache package name (ioredis → redis variant). |
| docs/guides/server-adapters/middleware.md | Updates Redis rate limit store example to node-redis. |
| docs/guides/server-adapters/koa.md | Updates Redis usage example to node-redis connection flow. |
| docs/guides/frameworks/fastify.md | Updates Redis-related install and code examples to node-redis. |
| docs/guides/examples/code-patterns.md | Updates Redis cache example to node-redis. |
| docs/cookbook/rate-limit-handling.md | Updates Redis rate limiter example to node-redis. |
| docs/custom-middleware-guide.md | Updates Redis-backed middleware example types to node-redis. |
| docs/plans/2026-03-07-observability-api-wiring.md | Notes updated TelemetryService behavior re: external TracerProvider reuse. |
| CLAUDE.md | Updates feature status table text (evaluation/testing note, TTS scope note). |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try { | ||
| // Set OTLP defaults before init (env vars take precedence) | ||
| if (!process.env.OTEL_EXPORTER_OTLP_ENDPOINT) { | ||
| process.env.OTEL_EXPORTER_OTLP_ENDPOINT = "http://localhost:4318"; | ||
| } |
There was a problem hiding this comment.
proxy start sets OTEL_EXPORTER_OTLP_ENDPOINT to http://localhost:4318 when it’s not already set, which implicitly enables OTLP exporting even when the user didn’t opt in. This can cause repeated export attempts/noise in environments without a collector, and it contradicts docs that imply OTLP is enabled only when the env var is configured. Consider only defaulting when a dedicated flag is set, or leaving it unset and logging a hint instead.
| const ollamaBase = this.getOllamaBaseUrl(); | ||
| if (!ollamaBase.startsWith("http")) { | ||
| healthStatus.isConfigured = false; | ||
| healthStatus.configurationIssues.push("Invalid OLLAMA_API_BASE format"); | ||
| healthStatus.recommendations.push( | ||
| "Set OLLAMA_API_BASE to a valid URL (e.g., http://localhost:11434)", | ||
| ); |
There was a problem hiding this comment.
The Ollama config validation now uses getOllamaBaseUrl() (which prefers OLLAMA_BASE_URL and falls back to OLLAMA_API_BASE), but the user-facing issue/recommendation strings still refer only to OLLAMA_API_BASE. Update these messages to reference OLLAMA_BASE_URL (and optionally mention OLLAMA_API_BASE as a legacy alias) so users are guided to the right env var.
| getTraceHeaders(): Record<string, string> { | ||
| return this.bridge.injectContext({}); | ||
| } |
There was a problem hiding this comment.
getTraceHeaders() injects from context.active(), but ProxyTracer never makes rootSpan the active span. As a result, traceparent/tracestate may not be injected into upstream headers (breaking distributed trace propagation). Inject using a context that explicitly sets rootSpan active (e.g., context.with(trace.setSpan(context.active(), this.rootSpan), ...)) or update OtelBridge.injectContext to accept an explicit context/span.
| | Metrics | `$OTEL_EXPORTER_OTLP_ENDPOINT/v1/metrics` | Request counters, latency histograms, token usage gauges | | ||
| | Logs | `$OTEL_EXPORTER_OTLP_ENDPOINT/v1/logs` | Structured request log records with traceId/spanId correlation | | ||
|
|
||
| **Configuration:** Set `OTEL_EXPORTER_OTLP_ENDPOINT` (e.g., `http://localhost:4318`) before starting the proxy. The proxy defaults to `service.name=neurolink-proxy`. |
There was a problem hiding this comment.
This doc says users must set OTEL_EXPORTER_OTLP_ENDPOINT before starting the proxy, but proxy start currently defaults it to http://localhost:4318 when unset. Please align the docs with actual behavior (either document the default, or change the CLI to only enable OTLP when explicitly configured).
|
|
||
| if (isClaudeTarget) { | ||
| // ─── PASSTHROUGH MODE (Claude → Claude) ─────────────── | ||
| // ─── PASSTHROUGH MODE (Claude → Claude) ────────���────── |
There was a problem hiding this comment.
The section header comment contains garbled/unprintable characters ("───���──────"), which makes logs/searching and file readability harder. Please replace with plain ASCII (or consistent box-drawing) characters.
1c2912e to
f2db69f
Compare
|
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:
WalkthroughAdds a Claude proxy observability stack: OTLP traces/metrics/logs, OpenObserve local compose/import tooling and CLI, new proxy telemetry commands, extensive proxy telemetry instrumentation (spans, SSE/raw-stream capture, body artifacts, OTLP emission), proxy CLI/env persistence, and widespread docs/examples and safety/type fixes. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (14)
src/lib/evaluation/pipeline/evaluationPipeline.ts (1)
263-298:⚠️ Potential issue | 🟡 MinorClarify or validate the behavior when both
onlyScorersandskipScorersare provided.Currently, when both options are specified,
onlyScorerssilently takes precedence andskipScorersis ignored. This behavior is neither validated nor documented, which could confuse users who provide both options expecting either an error or different semantics (e.g., intersection).Consider one of the following approaches:
- Add validation to reject both being specified simultaneously
- Document the precedence in JSDoc for
PipelineExecutionOptions- Implement meaningful semantics when both are provided (e.g., run only scorers in
onlyScorersthat are NOT inskipScorers)Option 1: Add validation at the start of `execute()`
async execute( input: ScorerInput, options?: PipelineExecutionOptions, ): Promise<PipelineResult> { if (!this._initialized) { await this.initialize(); } + + if (options?.onlyScorers && options?.skipScorers) { + throw new Error( + "Cannot specify both 'onlyScorers' and 'skipScorers' options" + ); + } const startTime = Date.now();Option 2: Document the precedence in type definition
Add JSDoc to
PipelineExecutionOptionsat line 22:/** * Pipeline execution options */ export type PipelineExecutionOptions = { /** Correlation ID for tracing */ correlationId?: string; /** Custom timeout override */ timeout?: number; - /** Skip specific scorers */ + /** Skip specific scorers. Ignored if onlyScorers is also specified. */ skipScorers?: string[]; - /** Only run specific scorers */ + /** Only run specific scorers. Takes precedence over skipScorers. */ onlyScorers?: string[]; /** Additional metadata to attach */ metadata?: JsonObject; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/evaluation/pipeline/evaluationPipeline.ts` around lines 263 - 298, The code silently ignores skipScorers when onlyScorers is present; add explicit validation so callers get a clear error instead of surprising precedence: in the entry point method execute() validate PipelineExecutionOptions and throw a user-friendly error if both onlyScorers and skipScorers are provided; update the JSDoc for PipelineExecutionOptions to document that they are mutually exclusive; leave _getScorersToRun and _getSkippedScorers unchanged (they can assume validated input) and ensure any tests or callers are updated to avoid passing both options simultaneously.src/lib/neurolink.ts (2)
8884-8894:⚠️ Potential issue | 🔴 CriticalScope tool-result caching by auth/request context.
This cache key is still only
toolName + params, but the actual execution also depends onoptions.authContextandthis.toolExecutionContext, which are merged later in the method. Two callers with identical params but different user/session/auth state can therefore read or overwrite each other’s cached results. At minimum, bypass caching for context-sensitive executions; ideally include the relevant auth/session scope in the cache key.Also applies to: 9083-9084, 9112-9113
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/neurolink.ts` around lines 8884 - 8894, The cache check using toolResultCache.getCachedResult(toolName, params) is too broad because it ignores request/auth scope; update the cache key (or skip caching) to include options.authContext and this.toolExecutionContext (or any derived session/user id) when computing cached results and when writing to the cache, and ensure the isCacheEnabled logic still respects toolAnnotations.destructiveHint; locate the cache-read/write calls around isCacheEnabled / toolResultCache.getCachedResult and the corresponding cache set calls and modify them to incorporate options.authContext and this.toolExecutionContext (or bypass caching when those contexts exist) so results are scoped per auth/request context.
7819-7868: 🛠️ Refactor suggestion | 🟠 MajorUse
ErrorFactory.toolTimeout()in this wrapper.The registration-time timeout path still rejects with a raw
Error, so callers lose the typed timeout metadata that the rest ofsrc/**/*.tsexpects for classification and logging. Keep theDOMExceptionbranch for caller-initiated aborts, but useErrorFactoryfor the timeout branch.Suggested change
- reject( - new Error( - `Tool '${toolName}' timed out after ${toolTimeout}ms (configured at registration)`, - ), - ); + reject(ErrorFactory.toolTimeout(toolName, toolTimeout));As per coding guidelines,
src/**/*.ts: Use ErrorFactory for creating typed errors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/neurolink.ts` around lines 7819 - 7868, The timeout rejection inside the execution wrapper should use ErrorFactory.toolTimeout instead of constructing a raw Error: in the convertedTool.execute wrapper (where originalExecute, toolTimeout and toolName are defined and composedSignal is used) replace the branch that rejects when timeoutSignal.aborted to call ErrorFactory.toolTimeout(...) producing a typed error (include toolName and toolTimeout as metadata/arguments per ErrorFactory signature) while keeping the DOMException branch for non-timeout aborts; ensure the Promise.race rejection uses that ErrorFactory-produced error so callers receive the expected typed timeout error.src/lib/auth/anthropicOAuth.ts (1)
254-270:⚠️ Potential issue | 🟠 MajorRemove the Claude 4-breaking beta from the exported OAuth defaults.
src/lib/providers/anthropic.tsnow documents thatinterleaved-thinking-2025-05-14triggersinvalid_request_erroron Claude 4, but both exported OAuth beta sets still include it here.src/cli/commands/auth.tsusesOAUTH_BETA_HEADERSdirectly, andsrc/lib/proxy/oauthFetch.tsconsumesCLAUDE_CODE_OAUTH_BETAS, so OAuth requests can still reintroduce the same 400 path by default.Suggested patch
-export const OAUTH_BETA_HEADERS = - "oauth-2025-04-20,interleaved-thinking-2025-05-14"; +export const OAUTH_BETA_HEADERS = "oauth-2025-04-20"; export const CLAUDE_CODE_OAUTH_BETAS = [ "oauth-2025-04-20", "claude-code-20250219", - "interleaved-thinking-2025-05-14", "context-management-2025-06-27", "prompt-caching-scope-2026-01-05", "advanced-tool-use-2025-11-20",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/anthropicOAuth.ts` around lines 254 - 270, The exported beta header defaults include the Claude-4-breaking flag "interleaved-thinking-2025-05-14"; remove that token from both OAUTH_BETA_HEADERS and the CLAUDE_CODE_OAUTH_BETAS array so OAuth requests no longer send the problematic header by default. Update the OAUTH_BETA_HEADERS string to exclude "interleaved-thinking-2025-05-14" and remove that entry from the CLAUDE_CODE_OAUTH_BETAS const while keeping the other betas intact so src/cli/commands/auth.ts and src/lib/proxy/oauthFetch.ts consume the corrected defaults.docs/guides/server-adapters/middleware.md (1)
639-672:⚠️ Potential issue | 🔴 CriticalInconsistency between import and usage.
Line 639 imports
createClientfrom theredispackage, but line 672 still usesnew Redis()which is the oldioredissyntax. This would cause a runtime error.🐛 Fix the Redis client instantiation
-const redisStore = new RedisRateLimitStore(new Redis()); +const redisClient = createClient(); +await redisClient.connect(); +const redisStore = new RedisRateLimitStore(redisClient);Note: The
redispackage requires calling.connect()before use, unlikeiorediswhich auto-connects.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/guides/server-adapters/middleware.md` around lines 639 - 672, The code imports createClient but still constructs the client with the old ioredis style (new Redis()); replace the instantiation at the end so you call createClient() and await its connect() before passing it into RedisRateLimitStore (e.g., const client = createClient(); await client.connect(); const redisStore = new RedisRateLimitStore(client)); also ensure the client variable uses the imported RedisClientType and that any Redis method names in RedisRateLimitStore (get, setex/setEx, pexpire/pExpire, incr, del) match the API of the createClient() client.docs/custom-middleware-guide.md (1)
830-857:⚠️ Potential issue | 🟡 MinorUse
set()with options instead of deprecatedsetex.The example imports from the
redisnpm package but usesredisClient.setex(), which is a legacy Redis command. In redis v4+, the recommended approach is to useset()with an options object specifying the expiration time.Proposed fix
- await redisClient.setex(cacheKey, 3600, JSON.stringify(result)); + await redisClient.set(cacheKey, JSON.stringify(result), { EX: 3600 });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/custom-middleware-guide.md` around lines 830 - 857, The example middleware (createRedisCachingMiddleware, specifically its wrapGenerate implementation) uses the deprecated redisClient.setex; change it to use redisClient.set(cacheKey, JSON.stringify(result), { EX: 3600 }) so the TTL is supplied via options (or { PX: ms } if you prefer milliseconds), and ensure you await redisClient.set(...) like the existing call so the value is persisted with expiration; keep the cacheKey generation and JSON.stringify/parse behavior unchanged.docs/cookbook/rate-limit-handling.md (1)
349-362:⚠️ Potential issue | 🟡 MinorConstructor parameter type inconsistency with the redis package migration.
The import and field type were updated to use
RedisClientTypefrom theredispackage, but the constructor parameter on line 357 still referencesRedis(the oldioredistype), which is no longer imported. This would cause a TypeScript error if used.📝 Proposed fix
- constructor(redis: Redis, key: string, limit: number, window: number = 60) { + constructor(redis: RedisClientType, key: string, limit: number, window: number = 60) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/cookbook/rate-limit-handling.md` around lines 349 - 362, The constructor parameter type is wrong: change the RedisRateLimiter constructor to accept the same RedisClientType used for the private field (replace the obsolete "Redis" type with RedisClientType) so the constructor signature and the private redis: RedisClientType field match the imported type from "redis"; update the constructor parameter in the RedisRateLimiter class accordingly.docs/guides/frameworks/fastify.md (1)
397-446:⚠️ Potential issue | 🟠 MajorCache plugin uses ioredis API with node-redis imports—code will not work as written.
The code imports from node-redis but still uses ioredis patterns:
new Redis()constructor,setex()method, andRedistype annotation. This requires three corrections:
- Change type annotation from
cache: Redistocache: RedisClientType- Replace
new Redis(url)withcreateClient({ url: ... })and addawait redis.connect()- Replace
setex()withsetEx()(camelCase)Suggested fix
import { createClient, type RedisClientType } from "redis"; declare module "fastify" { interface FastifyInstance { - cache: Redis; + cache: RedisClientType; async function cachePlugin(fastify: FastifyInstance) { - const redis = new Redis(process.env.REDIS_URL || "redis://localhost:6379"); + const redis = createClient({ + url: process.env.REDIS_URL || "redis://localhost:6379", + }); + await redis.connect(); - await redis.setex(request.cacheKey, ttl, payload); + await redis.setEx(request.cacheKey, ttl, payload);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/guides/frameworks/fastify.md` around lines 397 - 446, The cache plugin currently mixes ioredis usage with node-redis imports: update the FastifyInstance cache type from Redis to RedisClientType, replace the ioredis-style instantiation in cachePlugin (remove new Redis(...)) with createClient({ url: process.env.REDIS_URL || "redis://localhost:6379" }) and call await redis.connect() after creation, and change the ioredis method setex(...) to the node-redis camelCase setEx(...) when storing the payload; keep existing uses of redis.get(...) and the cacheResponse, onRequest, onSend logic intact but ensure types and method names match node-redis APIs.docs/guides/examples/code-patterns.md (1)
641-668:⚠️ Potential issue | 🟠 MajorUse correct node-redis v4+ API for this migration snippet.
The example is incomplete: the Redis client is never connected, and
setexdoesn't exist in node-redis. Useawait client.connect()before accessing the client and replace the TTL command with the.set()method with an options object.📌 Corrected doc fix
class RedisCachedAIService { private redis: RedisClientType; + private redisReady: Promise<void>; private ai: NeuroLink; constructor() { this.redis = createClient({ url: `redis://${process.env.REDIS_HOST}:${process.env.REDIS_PORT}`, password: process.env.REDIS_PASSWORD, }); + this.redisReady = this.redis.connect(); this.ai = new NeuroLink({ providers: [ { name: "openai", config: { apiKey: process.env.OPENAI_API_KEY } }, ], }); } async generate(prompt: string, ttlSeconds: number = 3600): Promise<string> { + await this.redisReady; const cacheKey = `ai:${this.hash(prompt)}`; const cached = await this.redis.get(cacheKey); if (cached) { console.log("Redis cache hit"); return cached; } console.log("Redis cache miss"); const result = await this.ai.generate({ input: { text: prompt }, provider: "openai", }); - await this.redis.setex(cacheKey, ttlSeconds, result.content); + await this.redis.set(cacheKey, result.content, { EX: ttlSeconds });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/guides/examples/code-patterns.md` around lines 641 - 668, The Redis usage in the generate method is using node-redis v4 incorrectly: the client is never connected and setex is not part of v4; update the constructor where this.redis = createClient(...) to call await this.redis.connect() (or ensure connect is awaited after instantiation), and in generate replace await this.redis.setex(cacheKey, ttlSeconds, result.content) with the v4 call await this.redis.set(cacheKey, result.content, { EX: ttlSeconds }); keep using this.redis.get(cacheKey) as-is and ensure any top-level async initialization handles the connect promise.src/lib/telemetry/telemetryService.ts (1)
127-175:⚠️ Potential issue | 🟠 MajorAvoid an order-dependent double-bootstrap that silently skips OTLP metrics and logs initialization.
Both
TelemetryServiceandinstrumentation.tsindependently try to register a globalTracerProviderwithout coordination. WhenTelemetryServiceauto-initializes first (via singleton), it registers aBasicTracerProvider. WheninitializeOpenTelemetry()then runs in standalone mode and attempts to register its ownNodeTracerProvider, it hits a duplicate registration error. The graceful error handler switches to external mode and returns early—before creating theMeterProviderandLoggerProvider(lines 798–870). This means OTLP metrics and logs never initialize even though the application expects them.The comment at line 812 ("Register globally so TelemetryService's metrics.getMeter() picks it up") shows that
instrumentation.tsintends to provide the globalMeterProviderforTelemetryServiceto consume. Establishing clear ownership of global provider registration—either by having one path own it or by explicit coordination—will prevent silent initialization failures.🤖 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 127 - 175, TelemetryService.initializeTelemetry() currently bails out on duplicate TracerProvider registration and returns early, preventing MeterProvider and LoggerProvider from being initialized when instrumentation.initializeOpenTelemetry() intends to own global provider registration; change the flow so that when hasExternalTracerProvider() or a duplicate-registration error is detected (use hasExternalTracerProvider(), adoptExternalTracerProvider()), you adopt the external tracer but continue and ensure metrics.getMeter(), MeterProvider and LoggerProvider initialization still run (i.e., do not return early), or alternatively avoid registering a BasicTracerProvider inside initializeTelemetry() and defer tracer registration to initializeOpenTelemetry() so ownership is explicit; update initializeTelemetry(), hasExternalTracerProvider(), and the duplicate-registration catch branch to either skip tracer registration but still initialize metrics/loggers, or to detect and adopt external providers while continuing full metrics/logging setup.src/cli/commands/proxy.ts (3)
1918-1948:⚠️ Potential issue | 🟠 MajorXML-escape persisted CLI paths before writing the plist.
Lines 1918-1948 inject raw
--env-file/--configvalues into the plist XML. A valid path containing&,<,>, or quotes will produce invalid XML and makelaunchctl loadfail.🤖 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 1918 - 1948, The plist builder injects raw paths (envFile/configFile) into XML which will break on characters like &, <, > or quotes; add an XML-escape helper (e.g., escapeXml) and apply it to envFile and configFile when constructing envFileArgs and configArgs (and optionally to other dynamic values like nodeExec, entryScript, host) so the strings replace &, <, >, " and ' with their XML entity equivalents before interpolation into the PLIST string.
1200-1228:⚠️ Potential issue | 🟠 Major
proxy status --format jsonreturns before stats are fetched.Lines 1226-1228 serialize the persisted state and exit before the live
/statuscall below runs. The machine-readable path therefore never includes the newstats,totalAttempts, or per-account counters.🤖 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 1200 - 1228, The JSON path currently serializes and returns the persisted status early (the status object created near the top, checked with isProcessRunning(state.pid)) before the subsequent live /status call can populate stats, totalAttempts, and per-account counters; remove or defer the early return guarded by argv.format === "json" and instead perform the live status fetch, merge the returned stats into the existing status object (including fields like stats, totalAttempts, per-account counters and any updated uptime/url), then call logger.always(JSON.stringify(status, null, 2)); ensure the isProcessRunning/state logic remains but the JSON output happens only after the live fetch/merge so machine-readable output includes the new metrics.
647-694:⚠️ Potential issue | 🟠 MajorPassthrough loses the original request payload before routing.
Line 663 eagerly consumes
c.req.json(), and the context only carries the parsed object. That means the new--passthroughmode can no longer be transparent/byte-preserving, which is exactly the class of request-shape drift this PR is trying to avoid.🤖 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 647 - 694, The code eagerly calls c.req.json() into body for every request which discards the original bytes and breaks --passthrough transparency; change the request intake to preserve the raw payload when passthrough is enabled: detect argv.passthrough (passthrough) and, instead of always doing await c.req.json(), read and store the raw request bytes/text (e.g., await c.req.arrayBuffer() or text()) into ctx.rawBody (or ctx.bodyRaw) and only attempt JSON parsing into ctx.body when passthrough is false (or when parsing succeeds and is needed). Ensure the ServerContext you build (ctx) contains both the raw payload (rawBody/rawText) and a parsed body fallback, and update any use-sites/route.handler expectations (route.handler, createClaudeProxyRoutes) to use ctx.rawBody when forwarding in passthrough mode so the original byte-preserving request can be proxied.src/lib/proxy/requestLogger.ts (1)
692-719:⚠️ Potential issue | 🟠 MajorThe cleanup size cap never counts
logs/bodies/**.Lines 692-719 only size-trim the JSONL files. The new gzip body artifacts are age-pruned, but they can still grow without bound inside the 7-day window and exhaust disk on busy proxies.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/proxy/requestLogger.ts` around lines 692 - 719, The size-trimming pass only accounts for JSONL entries (remaining/totalSize) and ignores the gzipped body artifacts under bodiesDir, so large body files can blow past maxSizeMb; update the cleanup to include bodiesDir contents in the size calculation and trimming: when building the initial file list (used to compute totalSize and populate remaining) also iterate readdirSync(bodiesDir) and statSync each bodyPath to push entries with path and size into remaining (or add their sizes to totalSize), and when deleting oldest files handle these body files the same way (decrement totalSize, increment deletedCount and freedBytes, and unlinkSync the body file). Ensure you still apply the age-based cutoff for bodies (mtimeMs < cutoff) before considering them for size-based deletion so both passes cooperate.
🟠 Major comments (20)
src/lib/core/redisConversationMemoryManager.ts-224-224 (1)
224-224: 🛠️ Refactor suggestion | 🟠 MajorWrap touched Redis calls with
withTimeout()for bounded async behavior.At Line 224, Line 939, and Line 1552, Redis operations are still unbounded awaits. Please wrap these with
withTimeout(...)(as already done inupdateAgenticLoopReport) to avoid hanging spans/requests when Redis stalls.Proposed patch
- const conversationData = await redisClient.get(redisKey); + const conversationData = await withTimeout(redisClient.get(redisKey), 5000); ... - const conversationData = await redisClient.get(redisKey); + const conversationData = await withTimeout(redisClient.get(redisKey), 5000); ... - const result = await redisClient.del(redisKey); + const result = await withTimeout(redisClient.del(redisKey), 5000);As per coding guidelines, "Wrap async operations with withTimeout utility".
Also applies to: 939-939, 1552-1552
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/core/redisConversationMemoryManager.ts` at line 224, The Redis get call that may hang needs to be wrapped with the withTimeout utility: replace the direct await redisClient.get(redisKey) in redisConversationMemoryManager (and the other unbounded Redis awaits at the spots noted) with await withTimeout(redisClient.get(redisKey), REDIS_TIMEOUT_MS) (use the same timeout value/pattern used in updateAgenticLoopReport) so all Redis calls are bounded; update any surrounding error handling to handle the timeout rejection consistently.src/lib/proxy/modelRouter.ts-24-26 (1)
24-26:⚠️ Potential issue | 🟠 MajorGive passthrough precedence over implicit Gemini routing.
With this order, any
gemini-*model bypassespassthroughModelshandling. That can break explicit passthrough intent.💡 Proposed ordering fix
- if (requestedModel.startsWith("gemini-")) { - return { provider: "vertex", model: requestedModel }; - } if (this.passthrough.has(requestedModel)) { return { provider: "anthropic", model: requestedModel }; } + if (requestedModel.startsWith("gemini-")) { + return { provider: "vertex", model: requestedModel }; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/proxy/modelRouter.ts` around lines 24 - 26, The current routing gives implicit gemini handling precedence and skips explicit passthrough intent; update the logic in modelRouter.ts so the passthroughModels check runs before the requestedModel.startsWith("gemini-") branch: first see if requestedModel matches/configures passthrough (using the passthroughModels collection or matching function), return the passthrough { provider, model } result when present, and only then fall back to the gemini startsWith("gemini-") return; ensure you reference the existing identifiers requestedModel and passthroughModels so the change integrates with the surrounding routing logic.src/lib/proxy/proxyConfig.ts-262-274 (1)
262-274:⚠️ Potential issue | 🟠 MajorValidate malformed
routingsections before the presence check.If
routingis provided as an array/string,hasRoutingbecomes false. With validaccounts, validation still passes andparseRoutingConfig()later collapses the bad section into{}; withoutaccounts, users get the misleading “must contain at least one” error instead of a shape error.Fail fast on invalid
routing+ if (cfg.routing !== undefined && !hasRouting) { + errors.push('"routing" must be an object'); + return errors; + } + if (!hasAccounts && !hasRouting) { errors.push('Config must contain at least one of "accounts" or "routing"'); return errors; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/proxy/proxyConfig.ts` around lines 262 - 274, The presence check incorrectly treats malformed cfg.routing (e.g., array/string) as absent; before computing hasRouting and before returning the generic "must contain" error, add an explicit shape validation for cfg.routing: if cfg.routing is defined but not an object or is an array, push a clear shape error into errors (e.g., 'routing must be an object') and return early so parseRoutingConfig() never sees a bad value; use the existing cfg.routing, hasRouting, errors and parseRoutingConfig identifiers to locate where to insert this guard.src/lib/proxy/proxyConfig.ts-583-589 (1)
583-589:⚠️ Potential issue | 🟠 MajorKeep
parseProxyConfigString()in sync with the new routing-only shape.This guard only fixes
loadProxyConfig(). Lines 679-684 inparseProxyConfigString()still doObject.entries(raw.accounts)unconditionally, so a routing-only config now validates and then crashes when it comes through the string-based entrypoint.Mirror the optional-accounts guard in the string parser
- const rawAccounts = raw.accounts as Record<string, unknown[]>; - for (const [provider, list] of Object.entries(rawAccounts)) { - accounts[provider] = list.map((item) => - applyAccountDefaults(item as Partial<ProxyAccountConfig>), - ); - } + const rawAccounts = raw.accounts as Record<string, unknown[]> | undefined; + if (rawAccounts && typeof rawAccounts === "object" && !Array.isArray(rawAccounts)) { + for (const [provider, list] of Object.entries(rawAccounts)) { + accounts[provider] = list.map((item) => + applyAccountDefaults(item as Partial<ProxyAccountConfig>), + ); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/proxy/proxyConfig.ts` around lines 583 - 589, The string-based parser parseProxyConfigString() still assumes raw.accounts exists and does Object.entries unconditionally; mirror the guard used elsewhere: check raw.accounts is defined and typeof raw.accounts === "object" before iterating, and only then populate accounts by mapping each (provider, list) to applyAccountDefaults(item as Partial<ProxyAccountConfig>); if raw.accounts is missing leave accounts as an empty object so routing-only configs don't crash.src/lib/neurolink.ts-12126-12129 (1)
12126-12129:⚠️ Potential issue | 🟠 Major
ensureAuthProvider()can still initialize the provider twice under concurrency.This lazy-init path still delegates to
setAuthProvider(), and the public setter clearsthis.authInitPromisebefore the async provider-factory branch finishes. A second concurrent authenticated request can then observeauthInitPromise === undefinedand start a second initialization. Please keep lazy init on a private path that does not mutate the dedupe promise until the original attempt settles.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/neurolink.ts` around lines 12126 - 12129, The lazy-init path using authInitPromise can race because ensureAuthProvider delegates to setAuthProvider which clears authInitPromise mid-init; fix by creating a private initialization path (e.g., a new private method like initAuthProviderInternal or call setAuthProvider with an internal flag) that performs the async provider-factory work without mutating this.authInitPromise until the operation settles, and have ensureAuthProvider assign this.authInitPromise to that private-init promise; keep the public setAuthProvider behavior unchanged for external callers but ensure it does not clear/overwrite authInitPromise during an in-flight private init.src/lib/providers/ollama.ts-2039-2043 (1)
2039-2043:⚠️ Potential issue | 🟠 MajorDon't classify every 404 as
InvalidModelError.The new message already says the same 404 can come from a bad base URL or the wrong API mode, not just a missing model. Returning
InvalidModelErrorfor all of them will steer fallback/retry handling and operator guidance down the wrong path. Only emit model-not-found when the response body confirms it; otherwise return a configuration/network error, preferably throughErrorFactory. As per coding guidelines:src/**/*.ts: Use ErrorFactory for creating typed errors.🤖 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 2039 - 2043, The code currently maps any 404 to InvalidModelError; change this so you inspect the actual response body/response error details (from the caught error in the Ollama request) and only return InvalidModelError when the body explicitly indicates the model is missing for this.modelName; for all other 404s (bad base URL, wrong API mode, network issues) create and return a configuration/network error using ErrorFactory (include this.providerName and this.baseUrl in the message) instead of InvalidModelError so callers can retry/fallback correctly.src/lib/proxy/sseInterceptor.ts-67-75 (1)
67-75:⚠️ Potential issue | 🟠 MajorCap raw transcripts and event logs before recording them.
contentBlocksare capped, buteventsandrawTextChunkscurrently grow with the full stream. All current proxy call sites enablecaptureRawText, andProxyTracer.logStreamEvents()serializes the fulleventsarray into span-event payloads, so long responses can blow up both heap usage and trace storage. Add byte/event ceilings plus a truncation marker here before storing them.Also applies to: 159-160, 340-343, 444-447
🤖 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 67 - 75, The events array and rawText/rawTextChunks need caps and truncation markers to prevent unbounded growth: in the SSE interceptor (symbols: events, rawText, rawTextChunks, contentBlocks) enforce a max number of events and/or max cumulative byte size when pushing new entries and drop or truncate excess content while appending a clear truncation marker string (e.g., "...[TRUNCATED]") so downstream serializers like ProxyTracer.logStreamEvents() see trimmed data; also apply the same byte/entry ceiling when building rawText from rawTextChunks and when aggregating contentBlocks so that any concatenation respects the caps and sets the truncation marker instead of growing the heap/trace payload unboundedly.src/lib/proxy/cloaking/plugins/sessionIdentity.ts-13-14 (1)
13-14:⚠️ Potential issue | 🟠 MajorKeeping
purgeExpiredSessions()as a no-op leaks the shared identity cache.
getOrCreateClaudeCodeIdentity()now stores TTL'd identities in the module-levelclaudeCodeIdentityCache, but expired entries are only replaced when the same seed is looked up again. Existing maintenance callers invokingpurgeExpiredSessions()will no longer reclaim stale keys, so a long-lived proxy keeps one dead entry per distinct account/token seed. Wire this wrapper to a real purge inanthropicOAuth.tsor remove the compatibility hook so callers don't assume cleanup still happens.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/proxy/cloaking/plugins/sessionIdentity.ts` around lines 13 - 14, The deprecated no-op purgeExpiredSessions() leaks entries from the module-level claudeCodeIdentityCache because getOrCreateClaudeCodeIdentity() only replaces TTL'd identities on same-seed lookups; replace the no-op by wiring it to the real cleanup routine in anthropicOAuth.ts (or remove the exported wrapper and update callers) so expired cache entries are actively purged: locate purgeExpiredSessions and either implement it to call the purge/cleanup function in anthropicOAuth.ts that iterates claudeCodeIdentityCache and removes expired entries, or delete the export and update any callers to call the proper cleanup API in anthropicOAuth.ts; ensure references to claudeCodeIdentityCache and getOrCreateClaudeCodeIdentity remain consistent.src/lib/proxy/rawStreamCapture.ts-13-14 (1)
13-14:⚠️ Potential issue | 🟠 MajorBound client-body capture before logging it.
This helper retains every decoded chunk until end-of-stream, and the proxy now awaits it on streamed responses. A long SSE response therefore pins the entire client body in memory — on top of the upstream capture — so one large stream can scale heap use linearly with output size. Please truncate after a fixed byte budget or make full-body capture opt-in.
Also applies to: 31-34, 37-42
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/proxy/rawStreamCapture.ts` around lines 13 - 14, The helper currently accumulates every decoded chunk into chunks and totalBytes (symbols: chunks, totalBytes in rawStreamCapture.ts), which can pin large streamed responses in memory; change this to a bounded capture by introducing a MAX_CAPTURE_BYTES constant and stop appending decoded chunks once totalBytes exceeds that budget (still increment totalBytes to reflect real size), mark a truncated flag so logs indicate truncation, and apply the same truncation logic to the other capture points noted (lines 31-34 and 37-42) so full-body capture is either limited or made opt-in via a config flag.src/lib/proxy/oauthFetch.ts-317-329 (1)
317-329:⚠️ Potential issue | 🟠 MajorUse a refresh-stable seed for synthetic Claude Code identities.
When
metadata.user_idis missing, this seedsgetOrCreateClaudeCodeIdentity()from the access-token prefix. Any token refresh changes that seed, so the helper generates a differentdevice_id/account_uuidpair for the same authenticated user. That undermines the stable Claude Code fingerprinting this wrapper is trying to emulate and can fragment session/rate-limit continuity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/proxy/oauthFetch.ts` around lines 317 - 329, The code currently seeds getOrCreateClaudeCodeIdentity with an access-token prefix (tokenPrefix), which changes on token refresh and breaks identity stability; instead seed the identity using a refresh-stable identifier: pass parsed.metadata?.user_id (or another persistent account id already present in parsed.metadata) as the primary seed to getOrCreateClaudeCodeIdentity (use tokenPrefix only as a last-resort fallback), and persist the resolved identity back into parsed.metadata.user_id and requestHeaders ("x-claude-code-session-id") so future calls use the stable seed rather than the transient token prefix; update the call sites around getToken(), tokenPrefix, getOrCreateClaudeCodeIdentity, parsed.metadata and requestHeaders.set accordingly.src/lib/providers/ollama.ts-174-193 (1)
174-193:⚠️ Potential issue | 🟠 MajorPreserve the upstream finish reason and total-token fallback.
Both branches now normalize every non-streaming completion to
finishReason: "stop", and the OpenAI-compatible path leavesusage.totalTokensundefined when the backend omitsusage.total_tokenseven thoughpromptTokensandcompletionTokenswere already recovered. That hides truncation/tool-call terminations and breaks callers that depend onusage.totalTokens.Also applies to: 256-283
🤖 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 174 - 193, The code currently forces finishReason to "stop" and leaves usage.totalTokens undefined when upstream omits usage.total_tokens; change the returned finishReason to preserve the upstream value (e.g. use data.choices?.[0]?.finish_reason ?? undefined or leave as-is instead of hardcoding "stop") and set usage.totalTokens to usage.total_tokens ?? (promptTokens + completionTokens) so callers still get a sensible fallback; apply the same fixes in the other branch handling OpenAI-compatible responses (the block that computes text, usage, promptTokens, completionTokens and returns finishReason/usage) and keep references to the same symbols (text, usage, promptTokens, completionTokens, finishReason, estimateTokenCount) when making the edits.src/lib/utils/providerUtils.ts-155-157 (1)
155-157:⚠️ Potential issue | 🟠 MajorDefault Ollama model drift can cause false “unavailable” results.
This check now defaults to
llama3.2:latest, but other runtime paths default tollama3.1:8b(src/lib/providers/ollama.tsandsrc/lib/utils/providerHealth.ts). WhenOLLAMA_MODELis unset, provider selection may incorrectly reject a healthy Ollama instance.📌 Suggested fix
- const defaultOllamaModel = - process.env.OLLAMA_MODEL || "llama3.2:latest"; + const defaultOllamaModel = + process.env.OLLAMA_MODEL || "llama3.1:8b";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/utils/providerUtils.ts` around lines 155 - 157, The defaultOllamaModel constant currently falls back to "llama3.2:latest" which is inconsistent with other code paths that default to "llama3.1:8b" (see src/lib/providers/ollama.ts and src/lib/utils/providerHealth.ts); update the fallback in defaultOllamaModel (in providerUtils.ts) to "llama3.1:8b" or centralize the default into a shared constant used by providerHealth, ollama provider, and providerUtils so provider selection and health checks use the exact same default model string and avoid false "unavailable" results.src/lib/proxy/proxyFetch.ts-25-37 (1)
25-37:⚠️ Potential issue | 🟠 MajorFix incorrect require path for instrumentation module.
The path
"../../services/server/ai/observability/instrumentation.js"will fail at runtime withMODULE_NOT_FOUND. Fromsrc/lib/proxy/proxyFetch.ts, the correct relative path should be"../services/server/ai/observability/instrumentation.ts"(one../instead of two, and.tsextension for TypeScript).Additionally, the comment states "Dynamic import to avoid hard dependency" but the code uses synchronous
require(). Consider using asyncimport()instead for consistency with the pattern used inoauthFetch.ts(line 343).🐛 Proposed fix
// Dynamic import to avoid hard dependency — getLangfuseContext is only // available when the observability module is loaded. - const { getLangfuseContext } = - require("../../services/server/ai/observability/instrumentation.js") as { - getLangfuseContext: () => - | { - sessionId?: string; - userId?: string; - conversationId?: string; - } - | undefined; - }; + // eslint-disable-next-line `@typescript-eslint/no-require-imports` + const mod = require("../services/server/ai/observability/instrumentation.js") as { + getLangfuseContext?: () => + | { + sessionId?: string; + userId?: string; + conversationId?: string; + } + | undefined; + }; + const getLangfuseContext = mod.getLangfuseContext; + if (!getLangfuseContext) return 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 25 - 37, The dynamic import in proxyFetch.ts is using a wrong require path and synchronous require; replace the require("../../services/server/ai/observability/instrumentation.js") usage with an awaited dynamic import("../services/server/ai/observability/instrumentation.ts") and extract getLangfuseContext from the imported module (getLangfuseContext) to match the TypeScript file and module resolution; update the surrounding code in the function that calls getLangfuseContext to be async/await-aware, keep the same return shape (sessionId, userId, conversationId | undefined), and guard for the possibility the module or getLangfuseContext is undefined before invoking it.src/lib/proxy/usageStats.ts-39-64 (1)
39-64:⚠️ Potential issue | 🟠 MajorKeep per-account
errorCountattempt-level for the failing account.After this change, any 401/403/5xx that gets retried away only updates
lastErrorAt. The bad account’serrorCountstays flat, so per-account health/failure-share stats will look clean even while one account is flaking and requests are succeeding elsewhere.Proposed fix
export function recordAttemptError( accountLabel: string, accountType: string, status: number, ): void { const acct = ensureAccount(accountLabel, accountType); acct.lastErrorAt = Date.now(); + acct.errorCount++; if (status === 429) { stats.totalRateLimits++; acct.rateLimitCount++; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/proxy/usageStats.ts` around lines 39 - 64, recordAttemptError currently only updates acct.lastErrorAt and rate-limit counters for interim failures, so transient 401/403/5xx retries never increment the per-account acct.errorCount; update recordAttemptError to increment acct.errorCount (and set lastErrorAt) for non-2xx attempt failures so the failing account’s error metrics reflect attempt-level failures. Locate recordAttemptError and use ensureAccount(accountLabel, accountType) to get acct, then increment acct.errorCount and set acct.lastErrorAt; preserve the existing rate-limit handling (stats.totalRateLimits and acct.rateLimitCount) and leave recordFinalError behavior intact.src/lib/types/proxyTypes.ts-522-530 (1)
522-530:⚠️ Potential issue | 🟠 MajorRename consumers in the same change.
AccountStats.requestCount/lastRequestAtwere renamed here, butsrc/cli/commands/telemetry.ts:700-708,722-730andsrc/cli/commands/observability.ts:539-547,566-574still readrequestCount. Those commands will now printundefinedor fail type-checking until the remaining callers are updated.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/types/proxyTypes.ts` around lines 522 - 530, You renamed AccountStats.requestCount -> attemptCount and lastRequestAt -> lastAttemptAt but left callers unchanged; update every consumer that reads requestCount or lastRequestAt to use attemptCount and lastAttemptAt instead (for example, the CLI telemetry and observability command code that prints or type-checks AccountStats), adjusting any formatting/variable names and types accordingly so the telemetry/observability command handlers compile and display the correct values.scripts/observability/check-proxy-telemetry.mjs-92-125 (1)
92-125:⚠️ Potential issue | 🟠 MajorTreat a missing local log directory as “no summary” instead of aborting.
On a fresh setup,
fs.readdir("~/.neurolink/logs")throwsENOENT, and the top-level catch exits before the stream freshness report is printed. The doctor command should still check OpenObserve and then mark the local summary as missing.Proposed fix
async function readLatestLocalSummary() { const logsDir = join(homedir(), ".neurolink", "logs"); - const entries = await fs.readdir(logsDir); + let entries; + try { + entries = await fs.readdir(logsDir); + } catch (error) { + if ( + error && + typeof error === "object" && + "code" in error && + error.code === "ENOENT" + ) { + return null; + } + throw error; + } const files = entries🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/observability/check-proxy-telemetry.mjs` around lines 92 - 125, readLatestLocalSummary currently lets fs.readdir throw ENOENT on fresh setups which aborts the doctor flow; modify readLatestLocalSummary so that when reading logsDir (constructed as join(homedir(), ".neurolink", "logs")) if fs.readdir or subsequent open fails with ENOENT it is caught and the function returns null (treat missing local log directory as “no summary”) while other errors still propagate; ensure the function still iterates files, opens, reads tail chunks and closes handles as before, but explicitly handle ENOENT from fs.readdir and fs.open to return null so the rest of the checks (e.g., stream freshness) continue.src/lib/services/server/ai/observability/instrumentation.ts-943-979 (1)
943-979:⚠️ Potential issue | 🟠 Major
flushOpenTelemetry()still skips the OTLP trace pipeline.The tracerProvider with attached BatchSpanProcessor (for OTLP export) is not flushed, allowing the function to return while recent spans remain buffered. This is inconsistent with the flushing of metrics and logs, and mirrors the same provider type that is properly shut down in
shutdownOpenTelemetry().Proposed fix
+ if (tracerProvider && !usingExternalProvider) { + try { + logger.info(`${LOG_PREFIX} Flushing OTLP traces...`); + await tracerProvider.forceFlush(); + } catch (error) { + failures.push({ signal: "traces", error }); + logger.error(`${LOG_PREFIX} Trace flush failed`, { + error: error instanceof Error ? error.message : String(error), + stack: error instanceof Error ? error.stack : undefined, + }); + } + } else { + logger.debug(`${LOG_PREFIX} No TracerProvider to flush`); + } + if (meterProvider) { try { logger.info(`${LOG_PREFIX} Flushing OTLP metrics...`); await meterProvider.forceFlush();🤖 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 943 - 979, flushOpenTelemetry() currently flushes meterProvider and loggerProvider but omits the tracer pipeline; update the function to also flush the tracerProvider used for OTLP traces by locating the tracerProvider variable (and any BatchSpanProcessor-attached provider) and adding an await tracerProvider.forceFlush() inside a try/catch like the meterProvider/loggerProvider blocks, pushing failures.push({ signal: "traces", error }) on error and logging the error and stack with logger.error(`${LOG_PREFIX} Trace flush failed`, ...), so traces are awaited before the function returns.src/lib/proxy/requestLogger.ts-454-455 (1)
454-455:⚠️ Potential issue | 🟠 MajorDon't ship absolute artifact paths in OTLP body logs.
Line 455 exports the full local
bodyPathto the remote backend. That leaks the operator's home directory / username into observability data and turns a local-only path into externally visible metadata.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/proxy/requestLogger.ts` around lines 454 - 455, The code currently sends the full local absolute stored.bodyPath in OTLP logs which leaks user/home paths; update the logic that builds the logged object (around the spread using stored.bodyPath) to never include an absolute path — instead include a safe identifier such as the file basename or a redacted/hashed value (e.g., use path.basename(stored.bodyPath) or a stable hash of the path) or omit the field entirely; ensure you import/require path if using basename and apply this change where stored.bodyPath is referenced in requestLogger.ts so only non-sensitive data is emitted.src/cli/commands/proxy.ts-2034-2042 (1)
2034-2042:⚠️ Potential issue | 🟠 MajorFail fast when an explicit
--configpath is missing.Lines 2034-2037 silently drop a missing custom config and install the service without it. That makes
neurolink proxy install --config ...look successful while launching a proxy with different routing than the user asked for.Suggested fix
const configPath = (argv as { config?: string }).config ?? join(homedir(), ".neurolink", "proxy-config.yaml"); + if ((argv as { config?: string }).config && !existsSync(configPath)) { + console.info(chalk.red(`Proxy config file not found: ${configPath}`)); + process.exit(1); + } const configFile = existsSync(configPath) ? configPath : undefined;🤖 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 2034 - 2042, The code currently silently ignores a missing custom config by setting configFile = undefined when the given configPath doesn't exist; change this so if the caller passed an explicit --config (check (argv as { config?: string }).config !== undefined) and the file at configPath does not exist, log a clear error and exit (similar to the envFile check) instead of falling back to the default. Update the logic around configPath/configFile to detect an explicitly provided config value and call console.info(chalk.red(...)) and process.exit(1) when that explicit path is missing; leave the existing fallback behavior only when no --config was provided.src/lib/proxy/proxyTracer.ts-549-605 (1)
549-605:⚠️ Potential issue | 🟠 MajorRequest/response bodies logged without redaction may expose sensitive data.
The body logging methods (
logRequestBody,logUpstreamRequestBody,logUpstreamResponseBody,logStreamEvents) capture full payloads as span events. These bodies may contain:
- PII in conversation content (user messages, assistant responses)
- Sensitive business data in prompts
- Potentially embedded credentials
Consider either:
- Adding a configurable flag to disable body logging in production
- Applying content redaction similar to
transformParamsForLogging()- Truncating bodies to reasonable limits
Suggested: Add opt-in flag and size limit
+const MAX_BODY_LOG_SIZE = 4096; // Limit logged body size + /** Log the incoming client request body. */ -logRequestBody(body: string): void { +logRequestBody(body: string, enableBodyLogging = false): void { + if (!enableBodyLogging) return; + const truncatedBody = body.length > MAX_BODY_LOG_SIZE + ? body.slice(0, MAX_BODY_LOG_SIZE) + "...[truncated]" + : body; this.rootSpan.addEvent("proxy.client.request_body", { - "proxy.body": body, + "proxy.body": truncatedBody, "proxy.body.size": body.length, }); }As per coding guidelines: "Use safe parameter logging with transformParamsForLogging() utility when logging sensitive data".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/proxy/proxyTracer.ts` around lines 549 - 605, The span events currently log full payloads in logRequestBody, logUpstreamRequestBody, logUpstreamResponseBody, and logStreamEvents which can leak sensitive data; update these methods to (1) consult a configurable boolean (e.g., enableBodyLogging) and only log full bodies when true, (2) apply safe redaction via the existing transformParamsForLogging() utility (or a new redactContent helper) before adding bodies to events, and (3) enforce a hard truncation limit (e.g., maxBodyLogSize) so stored strings are clipped and include a size field/flag indicating truncation; make sure the option and limit are plumbed into the class (constructor or config) and used in these functions so production defaults to disabled or truncated/redacted logging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7c9f7b09-41db-4fa8-96a5-6e35bdeb8a41
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (92)
CLAUDE.mdREADME.mddocs/advanced/auth-architecture.mddocs/advanced/mcp-integration.mddocs/assets/dashboards/neurolink-proxy-observability-dashboard.jsondocs/cli/commands.mddocs/cli/index.mddocs/cookbook/multi-provider-fallback.mddocs/cookbook/rate-limit-handling.mddocs/custom-middleware-guide.mddocs/features/claude-proxy-architecture.mddocs/features/claude-proxy-config-reference.mddocs/features/claude-proxy-observability.mddocs/features/claude-proxy-troubleshooting.mddocs/features/claude-proxy.mddocs/features/index.mddocs/features/observability.mddocs/features/provider-orchestration.mddocs/getting-started/index.mddocs/getting-started/providers/ollama.mddocs/guides/enterprise/multi-provider-failover.mddocs/guides/examples/code-patterns.mddocs/guides/frameworks/fastify.mddocs/guides/server-adapters/koa.mddocs/guides/server-adapters/middleware.mddocs/plans/2026-03-07-observability-api-wiring.mddocs/real-time-services.mddocs/reference/index.mddocs/reference/provider-selection.mddocs/sdk/nestjs-integration.mddocs/telemetry-guide.mdpackage.jsonscripts/build-browser.mjsscripts/observability/check-proxy-telemetry.mjsscripts/observability/docker-compose.proxy-observability.yamlscripts/observability/import-openobserve-dashboard.mjsscripts/observability/manage-local-openobserve.shscripts/observability/otel-collector.proxy-observability.yamlscripts/observability/proxy-observability.env.examplesrc/cli/commands/mcp.tssrc/cli/commands/proxy.tssrc/cli/commands/task.tssrc/cli/factories/commandFactory.tssrc/cli/parser.tssrc/lib/auth/anthropicOAuth.tssrc/lib/auth/providers/firebase.tssrc/lib/auth/providers/jwt.tssrc/lib/auth/providers/workos.tssrc/lib/auth/sessionManager.tssrc/lib/auth/tokenStore.tssrc/lib/client/aiSdkAdapter.tssrc/lib/client/streamingClient.tssrc/lib/core/modules/GenerationHandler.tssrc/lib/core/redisConversationMemoryManager.tssrc/lib/evaluation/BatchEvaluator.tssrc/lib/evaluation/hooks/observabilityHooks.tssrc/lib/evaluation/pipeline/evaluationPipeline.tssrc/lib/evaluation/pipeline/strategies/batchStrategy.tssrc/lib/evaluation/pipeline/strategies/samplingStrategy.tssrc/lib/neurolink.tssrc/lib/observability/otelBridge.tssrc/lib/providers/amazonBedrock.tssrc/lib/providers/anthropic.tssrc/lib/providers/litellm.tssrc/lib/providers/ollama.tssrc/lib/proxy/cloaking/plugins/sessionIdentity.tssrc/lib/proxy/modelRouter.tssrc/lib/proxy/oauthFetch.tssrc/lib/proxy/proxyConfig.tssrc/lib/proxy/proxyEnv.tssrc/lib/proxy/proxyFetch.tssrc/lib/proxy/proxyTracer.tssrc/lib/proxy/rawStreamCapture.tssrc/lib/proxy/requestLogger.tssrc/lib/proxy/sseInterceptor.tssrc/lib/proxy/usageStats.tssrc/lib/rag/chunkers/MarkdownChunker.tssrc/lib/rag/chunking/markdownChunker.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/index.tssrc/lib/telemetry/telemetryService.tssrc/lib/types/cli.tssrc/lib/types/proxyTypes.tssrc/lib/utils/messageBuilder.tssrc/lib/utils/providerHealth.tssrc/lib/utils/providerUtils.tstest/continuous-test-suite-workflow.tstest/continuous-test-suite.ts
f2db69f to
caafcbc
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 |
caafcbc to
f96f84d
Compare
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
|
@coderabbitai I am going to show you the full review and make sure to check all the boundary check of what is all or not, also verify if all the things are correctly resolved or not. |
|
✅ Actions performedFull review triggered. |
|
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
src/lib/core/redisConversationMemoryManager.ts (1)
1558-1567:⚠️ Potential issue | 🟠 Major
clearSessionstill has an unbounded async cleanup path after delete.Line 1566 calls
removeUserSession(...)without timeout. A slow/hung Redis call there can still stallclearSessioneven thoughdelis now bounded.⚙️ Proposed fix
if (Number(result) > 0) { // Remove session from user's session set if (userId) { - await this.removeUserSession(userId, sessionId); + await withTimeout( + this.removeUserSession(userId, sessionId), + REDIS_TIMEOUT_MS, + ); }As per coding guidelines, "Wrap async operations with withTimeout utility".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/core/redisConversationMemoryManager.ts` around lines 1558 - 1567, The clearSession flow calls removeUserSession(userId, sessionId) without a timeout, leaving an unbounded async cleanup; wrap that call in the same withTimeout utility (using REDIS_TIMEOUT_MS) so the removeUserSession promise is bounded and handle a timeout/error (e.g., catch and log via processLogger or this.logger) without blocking clearSession; update the block where redisClient.del(redisKey) is followed by removeUserSession to call withTimeout(this.removeUserSession(userId, sessionId), REDIS_TIMEOUT_MS) and handle rejection/timeouts gracefully.src/lib/client/streamingClient.ts (1)
439-476:⚠️ Potential issue | 🟠 MajorGuarding
shift()helps, butevents()still leaves the SSE open on early iterator exit.This generator starts
connect()itself, yet thefinallyblock only unregisters callbacks. If the caller breaks out of thefor awaitloop, or the server emits[DONE]but does not close immediately, the HTTP stream keeps running in the background.♻️ Proposed fix
async *events( requestOptions: { body?: unknown; headers?: Record<string, string>; } = {}, ): AsyncGenerator<StreamEvent, void, unknown> { const events: StreamEvent[] = []; let done = false; let error: Error | null = null; let resolver: (() => void) | null = null; + const ownsConnection = + this.state === "disconnected" || this.state === "error"; const onMessage = (event: StreamEvent) => { events.push(event); resolver?.(); }; @@ } finally { this.off("message", onMessage as (...args: unknown[]) => void); this.off("done", onDone as (...args: unknown[]) => void); this.off("error", onError as (...args: unknown[]) => void); + if ( + ownsConnection && + this.state !== "disconnected" && + this.state !== "error" + ) { + this.disconnect(); + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/client/streamingClient.ts` around lines 439 - 476, The generator currently only unregisters event handlers in the finally block, leaving the underlying SSE/HTTP stream started by connect(requestOptions) open when the iterator is exited early; fix this by also actively closing the connection there: set done = true, call resolver?.() to unblock any pending await, and invoke the client's stream-close method (e.g., this.disconnect() or this.close()—use the actual method your class exposes to terminate the connection) so the background connect promise and SSE/HTTP stream are aborted; keep the existing removal of handlers (this.off("message", onMessage), this.off("done", onDone), this.off("error", onError)) as well.src/lib/utils/providerUtils.ts (1)
147-167:⚠️ Potential issue | 🟠 MajorOllama availability check hardcodes localhost URL, ignoring
OLLAMA_BASE_URL.
isProviderAvailable("ollama")fetches fromhttp://localhost:11434/api/tagsdirectly, butproviderHealth.tsusesgetOllamaBaseUrl()which respectsOLLAMA_BASE_URLandOLLAMA_API_BASEenvironment variables. This inconsistency means remote Ollama instances will be detected as unavailable by this function even when properly configured.Proposed fix to use the configured Ollama URL
if (providerName === "ollama") { try { - const response = await fetch("http://localhost:11434/api/tags", { + const ollamaBase = process.env.OLLAMA_BASE_URL || process.env.OLLAMA_API_BASE || "http://localhost:11434"; + const response = await fetch(`${ollamaBase}/api/tags`, { method: "GET", signal: AbortSignal.timeout(2000), });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/utils/providerUtils.ts` around lines 147 - 167, The Ollama check in isProviderAvailable (providerName === "ollama") uses a hardcoded "http://localhost:11434/api/tags"; change it to build the URL from the existing helper used elsewhere (e.g., call getOllamaBaseUrl() or getOllamaApiBase() and append "/api/tags"), falling back to the localhost default if the helper returns empty, then use that URL for the fetch (preserving the GET method and AbortSignal.timeout). Update any variable names accordingly so the function consistently detects remote Ollama instances the same way providerHealth.ts does.src/lib/proxy/usageStats.ts (1)
39-65:⚠️ Potential issue | 🟠 MajorBoth
recordAttemptError()andrecordFinalError()increment the same account'serrorCountin the same request path, causing double-counting.The call chain in
claudeProxyRoutes.tsconfirms this: at line 2248,recordAttemptError(account.label, account.type, retryStatus)increments the account's error count, then at line 2298,recordFinalError(retryStatus, account.label, account.type)increments the same account again for the same failure. Similar sequences occur elsewhere (e.g., lines 2576 and 2603). This meanserrorCountno longer reflects the actual number of failed attempts when both functions are called.Either consolidate error tracking into a single function or ensure only one increments
acct.errorCount.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/proxy/usageStats.ts` around lines 39 - 65, The account error is being double-counted because recordAttemptError(...) increments acct.errorCount and later recordFinalError(...) increments it again for the same failure; pick one place to update per-attempt error counts. Fix by removing the per-account increment in recordFinalError (leave stats.totalErrors/stats.totalRequests updates) and instead only update acct.errorCount and acct.lastErrorAt inside recordAttemptError (which calls ensureAccount(...)); alternatively, if you prefer to count only final failures, move acct.errorCount++ and acct.lastErrorAt assignment out of recordAttemptError into recordFinalError and ensure callers only call the chosen function for counting. Ensure references to ensureAccount, acct.errorCount, acct.lastErrorAt, recordAttemptError, and recordFinalError are updated accordingly.src/lib/proxy/requestLogger.ts (1)
708-734:⚠️ Potential issue | 🟠 MajorThe 500 MB cap will not reclaim
logs/bodies/growth.This totals one
statper date directory underlogs/bodies/and then only deletes JSONL files in the size pass. Once the nested.json.gzartifacts become the dominant footprint, cleanup can stay over the cap indefinitely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/proxy/requestLogger.ts` around lines 708 - 734, The current size calculation only stats top-level entries in bodiesDirForSize and never removes nested artifacts, so logs/bodies/ can keep growing; update the logic in requestLogger.ts (symbols: bodiesDirForSize, bodiesSize, readdirSync, statSync) to recursively walk bodiesDirForSize and sum sizes of all nested files (including .json.gz) when computing bodiesSize, and also include bodies files in the eviction candidate set so the deletion loop (symbols: remaining, oldest, unlinkSync, deletedCount, freedBytes) can remove oldest body files as well as the top-level JSONL files until totalSize <= maxBytes. Ensure traversal handles errors non-fatally and uses file mtime to rank oldest items consistently across both sets.src/cli/commands/proxy.ts (1)
466-1125: 🛠️ Refactor suggestion | 🟠 MajorSplit
proxyStartCommand.handlerbefore CI will pass.GitHub Actions is already failing
max-lines-per-functionhere. The new env-file, passthrough, OTel init, background refresh, and shutdown branches need to move into helpers so this handler stays orchestration-only.🤖 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 466 - 1125, The handler function is too large and should be split into smaller helper functions so it only orchestrates startup; extract the OpenTelemetry init block (references: initializeOpenTelemetry, buildObservabilityConfigFromEnv, isOpenTelemetryInitialized), the background token refresh logic (references: needsRefresh, refreshToken, persistTokens, tokenStore, refreshInterval), the server startup/state persistence (references: serve, spawnFailOpenGuard, saveProxyState, setClaudeProxySettings, printProxyBanner), and the graceful shutdown logic (references: shutdown, flushOpenTelemetry, shutdownOpenTelemetry, clearClaudeProxySettings) into dedicated helper functions (e.g., initOpenTelemetry(), startBackgroundRefreshLoop(), startServerAndPersistState(), configureClaudeAuto(), setupShutdownHandlers()) and replace the inlined blocks in handler with calls to those helpers so handler becomes orchestration-only.
🟠 Major comments (21)
src/lib/evaluation/pipeline/evaluationPipeline.ts-261-271 (1)
261-271:⚠️ Potential issue | 🟠 MajorUse ErrorFactory instead of generic
Errorfor option validation.The new mutual-exclusivity branch throws a plain
Error; in this codebase it should be a typed error fromErrorFactoryso callers can handle it reliably.As per coding guidelines,
src/**/*.ts: Use ErrorFactory for creating typed errors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/evaluation/pipeline/evaluationPipeline.ts` around lines 261 - 271, Replace the plain throw new Error in _validateExecutionOptions (method validating PipelineExecutionOptions) with a typed error produced by the project's ErrorFactory: import ErrorFactory if needed and call the appropriate factory method to create a validation/invalid-argument error with the message "Cannot specify both 'onlyScorers' and 'skipScorers' options" (e.g., ErrorFactory.create(...)), then throw that typed error so callers can handle it reliably.src/cli/factories/commandFactory.ts-610-612 (1)
610-612:⚠️ Potential issue | 🟠 MajorUse
ErrorFactoryinstead ofnew Errorfor CLI validation failures.This should emit a typed error per repository standards, not a generic
Error.As per coding guidelines:
src/**/*.ts: Use ErrorFactory for creating typed errors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/factories/commandFactory.ts` around lines 610 - 612, Replace the plain throw new Error(...) in commandFactory.ts with a typed error from ErrorFactory: import ErrorFactory and throw ErrorFactory.validation(`One or more input files do not exist:\n${missingPaths.join("\n")}`) (or the project’s standard ErrorFactory method for validation errors) so the CLI emits a typed validation error instead of a generic Error; update the import to include ErrorFactory and ensure the thrown error preserves the same message content.src/cli/factories/commandFactory.ts-563-603 (1)
563-603:⚠️ Potential issue | 🟠 Major
file://inputs currently bypass local existence checks.
file://is a local file reference, but it is treated as non-local and skipped. Missing local files can pass pre-validation and fail later in less actionable paths.Suggested fix
+import { fileURLToPath } from "node:url"; ... private static isNonLocalFileReference(filePath: string): boolean { const lower = filePath.toLowerCase(); return ( lower.startsWith("http://") || lower.startsWith("https://") || - lower.startsWith("file://") || lower.startsWith("data:") ); } ... for (let i = 0; i < resolvedPaths.length; i++) { - const resolvedPath = resolvedPaths[i]; + const resolvedPath = resolvedPaths[i]; + const pathToCheck = resolvedPath.toLowerCase().startsWith("file://") + ? fileURLToPath(resolvedPath) + : resolvedPath; if (CLICommandFactory.isNonLocalFileReference(resolvedPath)) { continue; } - if (!fs.existsSync(resolvedPath)) { + if (!fs.existsSync(pathToCheck)) { missingPaths.push( - `${option} path not found: ${rawPaths[i]} (resolved to ${resolvedPath})`, + `${option} path not found: ${rawPaths[i]} (resolved to ${pathToCheck})`, ); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/factories/commandFactory.ts` around lines 563 - 603, isNonLocalFileReference currently treats "file://" as non-local which lets local file:// references skip existence checks in validateCliInputFiles; update the isNonLocalFileReference method to only treat network/data URIs (e.g., "http://", "https://", "data:") as non-local and remove "file://" from that list, and ensure validateCliInputFiles still resolves any "file://" inputs to a filesystem path (using resolveFilePaths or by stripping the file:// scheme) before calling fs.existsSync so local file:// paths are validated like plain paths.src/lib/auth/providers/firebase.ts-95-103 (1)
95-103:⚠️ Potential issue | 🟠 MajorPreserve
PROVIDER_INIT_FAILEDinstead of routing it to API fallback.
PROVIDER_INIT_FAILEDis thrown in the new guard but the broad catch still falls back tovalidateViaApi()whenapiKeyexists. That masks provider initialization failures and weakens the typed error path.Suggested fix
- } catch (error) { - // If local validation fails and API key is available, try REST API - if (this.apiKey) { - return this.validateViaApi(token); - } + } catch (error) { + if ( + error instanceof Error && + "code" in error && + (error as { code?: string }).code === AuthError.codes.PROVIDER_INIT_FAILED + ) { + return { + valid: false, + error: error.message, + }; + } + + // If local validation fails and API key is available, try REST API + if (this.apiKey) { + return this.validateViaApi(token); + }Also applies to: 120-124
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/providers/firebase.ts` around lines 95 - 103, When catching errors in this provider, do not treat an AuthError with code "PROVIDER_INIT_FAILED" as recoverable by validateViaApi(); instead detect and rethrow the provider init error so the typed failure path is preserved. Update the catch logic around where jwks is checked (and the other similar block at lines ~120-124) to inspect the thrown error (e.g., error.code === "PROVIDER_INIT_FAILED" or instanceof AuthError) and rethrow it immediately; only call validateViaApi() for other error types or when apiKey logic truly applies. Ensure you reference the existing AuthError.create(..., "PROVIDER_INIT_FAILED") usage and the validateViaApi() call when making this change.src/lib/auth/providers/workos.ts-96-103 (1)
96-103:⚠️ Potential issue | 🟠 MajorDo not swallow init failures in unconditional API fallback.
The new
PROVIDER_INIT_FAILEDcan never be handled distinctly because the method catches everything and always falls back tovalidateSessionViaAPI(). This can hide initialization problems behind a different auth path.Suggested fix
- } catch { - // If JWT validation fails, try session validation via API - return this.validateSessionViaAPI(token); + } catch (error) { + if ( + error instanceof Error && + "code" in error && + (error as { code?: string }).code === AuthError.codes.PROVIDER_INIT_FAILED + ) { + return { + valid: false, + error: error.message, + }; + } + // If JWT validation fails, try session validation via API + return this.validateSessionViaAPI(token); }Also applies to: 146-149
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/providers/workos.ts` around lines 96 - 103, The provider init error (AuthError with code "PROVIDER_INIT_FAILED" raised when jwks is missing in workos.ts) is being masked by the unconditional fallback to validateSessionViaAPI(); update the catch paths surrounding the jwks checks and the other occurrence (around the block at lines similar to 146-149) to rethrow AuthError instances that have code "PROVIDER_INIT_FAILED" instead of falling back—i.e., detect errors created by AuthError.create("PROVIDER_INIT_FAILED", ...) and throw them again, and only use validateSessionViaAPI() for other error types.src/cli/commands/task.ts-481-485 (1)
481-485:⚠️ Potential issue | 🟠 MajorValidate
--atformat, not just presence.This path accepts any non-empty string for
at, so malformed timestamps can be persisted as one-time schedules and fail later at execution time. Reject invalid timestamps here to fail fast.Proposed fix
} else { if (!argv.at) { throw new Error("One-time tasks require --at"); } - schedule = { type: "once", at: argv.at }; + const parsedAt = new Date(argv.at); + if (Number.isNaN(parsedAt.getTime())) { + throw new Error( + "Invalid --at value. Use an ISO 8601 timestamp, e.g. 2026-04-01T14:00:00Z", + ); + } + schedule = { type: "once", at: parsedAt.toISOString() }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/commands/task.ts` around lines 481 - 485, The current branch accepts any non-empty argv.at and persists schedule = { type: "once", at: argv.at } — change this to validate the timestamp format before creating the schedule: parse argv.at (e.g. with Date.parse or a strict ISO-8601 regex) and ensure it yields a valid Date (and optionally that it is in the future); if parsing fails, throw a clear error like "Invalid --at timestamp" instead of accepting the raw string. Update the code path that builds the one-time schedule (the block referencing argv.at and schedule = { type: "once", at: argv.at }) to perform this validation and only assign the at value once validated.src/lib/proxy/rawStreamCapture.ts-34-44 (1)
34-44:⚠️ Potential issue | 🟠 MajorCap the capture by raw bytes, not decoded text length.
capturedBytesis advanced withdecoded.length/finalChunk.length, so the limiter is counting UTF-16 code units whileMAX_CAPTURE_BYTESandtotalBytesare bytes. Multibyte UTF-8 content can overshoot the 1 MB ceiling, and the flush path can drop overflow without reliably markingtruncated.Also applies to: 56-63
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/proxy/rawStreamCapture.ts` around lines 34 - 44, The capture limiter is counting UTF-16 code units (finalChunk.length) instead of actual bytes, so multibyte UTF-8 can overflow MAX_CAPTURE_BYTES and truncation may not be flagged; in rawStreamCapture update all places where capturedBytes is advanced (including the decoder.flush path and the other block at lines 56-63) to measure and cap by byte length using a byte-aware measure (e.g., TextEncoder().encode(str).length or Buffer.byteLength(str, 'utf8')), trim the string by bytes (iteratively or by encoding and slicing the Uint8Array) so you only push content that fits within MAX_CAPTURE_BYTES, update capturedBytes by the actual byte count, and set truncated = true when any overflow is trimmed; reference variables/functions: capturedBytes, MAX_CAPTURE_BYTES, decoder.decode()/decoder.decode(null), finalChunk, TRUNCATION_MARKER, and ensure the same change is applied to both flush and normal-finalization paths.src/lib/proxy/rawStreamCapture.ts-76-88 (1)
76-88:⚠️ Potential issue | 🟠 MajorResolve
captureon writer failures too.The promise only settles from
flush()or this wrapper sink’sabort(). IfinnerWriter.write()orinnerWriter.close()rejects,capturestays pending and any caller awaiting the artifact can hang indefinitely.🔧 Minimal fix
const innerWriter = transform.writable.getWriter(); + void innerWriter.closed.catch(() => undefined).finally(settle); const writable = new WritableStream<Uint8Array>({🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/proxy/rawStreamCapture.ts` around lines 76 - 88, The wrapper WritableStream currently calls settle() only from abort() and flush(), so if innerWriter.write(...) or innerWriter.close() rejects the capture promise never resolves; wrap the calls to innerWriter.write and innerWriter.close to catch rejections, call settle() inside the catch, then rethrow the error so upstream still sees the failure. Update the write(...) and close(...) handlers on the writable stream (referencing innerWriter, writable, write, close, abort, capture and settle()) to perform this try/catch (or promise.catch) behavior so capture is resolved on writer failures as well.src/lib/providers/ollama.ts-72-84 (1)
72-84:⚠️ Potential issue | 🟠 MajorAvoid classifying 404s from a formatted string.
Please keep
statusand response body structured instead of baking them intoerror.message. Because Line 82 always includes404 Not Found, the laterincludes("not found")branch also matches plain endpoint 404s, so a bad base URL or API-mode mismatch can now surface asInvalidModelErrorinstead of the endpoint-mismatchProviderError.As per coding guidelines, `src/**/*.ts`: Use ErrorFactory for creating typed errors.🩹 Minimal guard while switching to a typed error
- (errMsg.toLowerCase().includes("model") || - errMsg.toLowerCase().includes("not found")) + errMsg.toLowerCase().includes("model") && + errMsg.toLowerCase().includes("not found")Also applies to: 2039-2049
🤖 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 72 - 84, The current createOllamaHttpError builds a plain Error whose message embeds status and body, which causes downstream string-matching (e.g., includes("not found")) to misclassify 404s; change createOllamaHttpError to return a typed error via ErrorFactory (use the project's ErrorFactory API) and attach structured properties like status (response.status) and body (responseBody) on the error object instead of baking them only into message, keep a concise message but ensure consumers check error.status/error.body (not error.message) for logic that distinguishes 404 vs provider/endpoint errors; update references to createOllamaHttpError to expect the typed Error.src/cli/commands/mcp.ts-750-751 (1)
750-751:⚠️ Potential issue | 🟠 MajorHandle MCP init failures explicitly before continuing command flow.
getMCPStatus()can return{ error: ... }without throwing. These call sites ignore that and continue, which can surface misleading downstream errors. Also, these readiness calls should be timeout-bounded in CLI flow.As per coding guidelines `src/**/*.ts`: Wrap async operations with withTimeout utility.💡 Suggested fix pattern
- await sdk.getMCPStatus(); + const mcpStatus = await withTimeout( + sdk.getMCPStatus(), + 10_000, + ErrorFactory.toolTimeout("mcpStatus", 10_000), + ); + if (mcpStatus.error) { + if (spinner) spinner.fail(); + logger.error(chalk.red(`❌ MCP initialization failed: ${mcpStatus.error}`)); + process.exit(1); + }Apply this in
MCPCommandFactory.executeTest,MCPCommandFactory.executeExec, andMCPCommandFactory.executeRemove.Also applies to: 864-865, 992-993
🤖 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 750 - 751, The call sites in MCPCommandFactory (executeTest, executeExec, executeRemove) call sdk.getMCPStatus() and sdk.listMCPServers() but don’t handle the non-throwing error case or apply timeouts; update each location to call these ops via the withTimeout utility and then check the returned value for an { error } field before continuing (if error present, log/return a clear failure). Specifically, replace direct awaits of getMCPStatus and listMCPServers with withTimeout(sdk.getMCPStatus(), timeoutMs) and withTimeout(sdk.listMCPServers(), timeoutMs), then branch on result.error to short-circuit the command flow with a helpful message instead of proceeding to downstream logic.scripts/observability/manage-local-openobserve.sh-148-157 (1)
148-157:⚠️ Potential issue | 🟠 MajorDon't print the configured OpenObserve password.
The
setupbanner echoes${NEUROLINK_OPENOBSERVE_PASSWORD}to stdout. If someone overrides the default, the real secret ends up in terminal scrollback and CI logs. Print only the username and point users to the env file for the password.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/observability/manage-local-openobserve.sh` around lines 148 - 157, The banner currently prints the OpenObserve password via the heredoc using ${NEUROLINK_OPENOBSERVE_PASSWORD}; remove that variable from the printed output so secrets are not sent to stdout/CI logs, leaving only the username (${NEUROLINK_OPENOBSERVE_USER}) and a note telling users to consult the env file for the password (or how to set it), updating the heredoc in manage-local-openobserve.sh accordingly.src/lib/proxy/proxyFetch.ts-42-52 (1)
42-52:⚠️ Potential issue | 🟠 MajorKeep
x-neurolink-*context headers off third-party provider calls.These headers are injected for every outbound fetch, not just requests aimed at the local Neurolink proxy. That forwards
sessionId,userId, andconversationIdto Anthropic/OpenAI/etc. and to any intermediary forward proxy, which is a privacy leak. Gate them on a trusted Neurolink destination and keep only W3C trace context for generic upstreams.🤖 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 42 - 52, Currently the NeuroLink session headers (x-neurolink-session-id, x-neurolink-user-id, x-neurolink-conversation-id) are always injected into the carrier (from getLangfuseContext()), leaking PII to third-party providers; change the logic in proxyFetch.ts so you only add those headers when the outbound request target is the trusted Neurolink proxy (e.g., check the request URL/host against a trusted destination or an env var like NEUROLINK_PROXY_HOST or an isNeurolinkHost helper) and otherwise do not set them (keep only W3C trace context for generic upstreams). Locate the injection site around getLangfuseContext() and carrier and gate the three x-neurolink-* assignments behind that host/trust check. Ensure existing W3C trace headers remain intact for all requests.scripts/observability/otel-collector.proxy-observability.yaml-34-40 (1)
34-40:⚠️ Potential issue | 🟠 MajorMake TLS insecurity opt-in instead of hardcoded.
The endpoint is environment-configurable, allowing deployment against remote HTTPS backends. Hardcoding
tls.insecure: truewhile also using environment-driven Basic auth creates a footgun: anyone pointing this config at a remote HTTPS server without explicitly enabling cert validation will skip TLS checks while still transmitting credentials, leaving the connection vulnerable to MITM attacks. Move theinsecureflag to an environment variable with a safe default (falseor omitted), or explicitly document the security implications and local-only intent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/observability/otel-collector.proxy-observability.yaml` around lines 34 - 40, The tls.insecure setting under otlphttp/openobserve is hardcoded to true; change it to be opt-in via an environment variable (e.g., NEUROLINK_OPENOBSERVE_TLS_INSECURE) with a safe default of false/omitted, and use that env var to set tls.insecure instead of the literal true so remote HTTPS backends do certificate validation by default; update otlphttp/openobserve and the tls.insecure reference accordingly and document that enabling the env var disables cert validation for local/testing only.src/lib/proxy/requestLogger.ts-338-343 (1)
338-343:⚠️ Potential issue | 🟠 MajorUse the shared log-sanitization utility before persisting full bodies.
redactBody()only strips a hard-coded set of exact JSON string keys, but this path now writes whole payloads to disk and OTLP. Please route headers/body throughtransformParamsForLogging()so secret handling stays aligned with the centralized policy.As per coding guidelines, "src/**/*.ts: Use safe parameter logging with transformParamsForLogging() utility when logging sensitive data".
Also applies to: 476-495
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/proxy/requestLogger.ts` around lines 338 - 343, redactBody currently serializes and regex-replaces keys (serializeBody and SENSITIVE_BODY_KEYS) but you must route headers/bodies through the shared transformParamsForLogging() sanitizer before persisting full payloads; update redactBody to call transformParamsForLogging(body) (or transformParamsForLogging({ body }) as appropriate), then serialize that sanitized result instead of the raw body, and remove direct use of SENSITIVE_BODY_KEYS; apply the same change to the other code path that writes full bodies (the similar body-logging block referenced in the review) so both use transformParamsForLogging for consistent centralized redaction.src/lib/proxy/proxyTracer.ts-380-387 (1)
380-387:⚠️ Potential issue | 🟠 MajorDon't replace the caller's Langfuse user with the proxy account.
startRequest()seedsuserIdfromx-neurolink-user-id, butsetAccountSelection()overwrites it withselectedAccount. After routing, the trace is attributed to the Anthropic account email instead of the actual caller. Keep the account on span attributes/metadata and leaveuserIdunchanged.Also applies to: 491-494
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/proxy/proxyTracer.ts` around lines 380 - 387, The code is overwriting the original caller userId (seeded by startRequest from x-neurolink-user-id) with the proxy's selectedAccount when calling setLangfuseContext; change the calls around setLangfuseContext (the block using nlUserId/nlSessionId/nlConversationId/selectedAccount and the similar block at the other occurrence) to leave userId as nlUserId (or ctx.session-derived value) and instead store selectedAccount on the span metadata or an explicit account field (e.g., metadata.account or metadata.selectedAccount) so that tracing attribution keeps the caller as userId while still recording which proxy account was used. Ensure setAccountSelection can still set selectedAccount for metadata but does not overwrite nlUserId before calling setLangfuseContext.src/cli/commands/proxy.ts-1233-1248 (1)
1233-1248:⚠️ Potential issue | 🟠 MajorTime-box the live
/statusfetch.This new prefetch runs on every
proxy statusinvocation, including--format json, but it has no timeout. If the proxy accepts the connection and then stalls, the CLI hangs indefinitely instead of falling back to persisted state.As per coding guidelines,
src/**/*.ts: Wrap async operations with withTimeout utility.🤖 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 1233 - 1248, The fetch to `${status.url}/status` that populates liveStats can hang because it lacks a timeout; wrap the fetch call (or the whole await of statusResp.json()) with the project's withTimeout utility (import if missing) and handle timeout errors by falling back to the existing behavior (leave liveStats null). In practice, replace the direct await fetch/await statusResp.json() sequence inside the try block in the proxy status flow (where status.running && status.url is checked) with a withTimeout-wrapped Promise so a timeout rejects fast and is caught by the existing catch block, preserving the non-fatal fallback behavior.src/lib/proxy/proxyTracer.ts-225-246 (1)
225-246:⚠️ Potential issue | 🟠 MajorTruncation currently bypasses JSON redaction.
For oversized JSON payloads, the code truncates first,
JSON.parse(truncated)then fails, and the fallback returns the raw truncated body. Any secret that appears before the cutoff is therefore logged unredacted. Redact the parsed body first, then truncate the redacted serialization; the non-JSON fallback should also get a last-pass scrub instead of returning raw text.As per coding guidelines,
src/**/*.ts: Use safe parameter logging with transformParamsForLogging() utility when logging sensitive data.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/proxy/proxyTracer.ts` around lines 225 - 246, redactBodyForLogging currently truncates before redaction which lets secrets in oversized JSON slip through; change it to parse the full body first (if JSON), walk and redact keys using SENSITIVE_BODY_KEYS (the existing walk logic), then JSON.stringify the redacted result and only then apply truncation to maxLen (append "…[truncated]" when necessary). For non-JSON bodies, run the raw text through the project's safe-logging helper transformParamsForLogging() (or apply the same SENSITIVE_BODY_KEYS-based scrub) as a last-pass scrub before truncating and returning. Update references inside redactBodyForLogging to use transformParamsForLogging for consistency with other code paths.src/lib/proxy/proxyTracer.ts-184-212 (1)
184-212:⚠️ Potential issue | 🟠 MajorRedact the NeuroLink identity headers before exporting trace events.
x-neurolink-user-id,x-neurolink-session-id, andx-neurolink-conversation-idare propagated specifically to carry caller identity, butredactHeaders()leaves them untouched. Any call tologRequestHeaders()/logUpstreamRequestHeaders()will emit those identifiers verbatim to OTLP/Langfuse.🔒 Suggested fix
const SENSITIVE_HEADER_NAMES = new Set([ "authorization", "proxy-authorization", "x-api-key", "cookie", "set-cookie", + "x-neurolink-user-id", + "x-neurolink-session-id", + "x-neurolink-conversation-id", ]);As per coding guidelines,
src/**/*.ts: Use safe parameter logging with transformParamsForLogging() utility when logging sensitive data.Also applies to: 611-642
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/proxy/proxyTracer.ts` around lines 184 - 212, The current redactHeaders function doesn’t handle NeuroLink identity headers (x-neurolink-user-id, x-neurolink-session-id, x-neurolink-conversation-id) so logRequestHeaders/logUpstreamRequestHeaders can emit PII; update redactHeaders (and the SENSITIVE_HEADER_NAMES set) to treat those header names as sensitive OR ensure headers passed to logRequestHeaders/logUpstreamRequestHeaders are run through the transformParamsForLogging() utility before exporting traces; modify the code paths that call redactHeaders/logRequestHeaders/logUpstreamRequestHeaders to use transformParamsForLogging() when preparing OTLP/Langfuse payloads so NeuroLink identity headers are redacted consistently.src/cli/commands/proxy.ts-663-669 (1)
663-669:⚠️ Potential issue | 🟠 MajorDon't coerce malformed JSON into an empty request.
This turns an invalid client payload into
{}and then calls the route handler anyway. In full mode that changes a parse failure into a different request/response path, and can send nonsense upstream instead of returning a clean 400.💡 Suggested fix
if (method === "post") { rawBody = await c.req.text().catch(() => undefined); - try { - body = rawBody ? JSON.parse(rawBody) : emptyBody; - } catch { - body = emptyBody; - } + if (!rawBody) { + body = emptyBody; + } else { + try { + body = JSON.parse(rawBody); + } catch { + if (!passthrough) { + return c.json( + { + type: "error", + error: { + type: "invalid_request_error", + message: "Request body must be valid JSON", + }, + }, + 400, + ); + } + body = emptyBody; + } + } }🤖 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 663 - 669, The code in the POST branch (where variables rawBody, body and emptyBody are used) silently coerces malformed JSON into emptyBody; instead detect JSON.parse failures and return a 400 error response to the client rather than calling the route handler with `{}`. Update the POST handling in the proxy handler to catch JSON.parse errors on rawBody and send an HTTP 400 (with a concise error message) when parse fails, leaving valid parsed objects as body and only using emptyBody for genuinely empty request bodies.src/lib/proxy/proxyTracer.ts-529-535 (1)
529-535:⚠️ Potential issue | 🟠 MajorSubstituted models are still costed as the requested model.
setModelSubstitution()only annotates the span. All pricing and most metric labels still readthis.model, which never changes from the original request, so a fallback/remap to a different pricing tier will report the wrong cost and telemetry. Track aneffectiveModeland switch pricing/label generation to it once known.Also applies to: 578-589, 772-788, 807-813, 855-861
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/proxy/proxyTracer.ts` around lines 529 - 535, The span annotation via setModelSubstitution() doesn't change pricing/metrics because calculateCost and label generation still use this.model; introduce an effectiveModel value (e.g., a private effectiveModel property or local variable in ProxyTracer methods) that defaults to this.model but is overwritten when setModelSubstitution() is applied, then update all calls to calculateCost(...) and any telemetry/label creation (locations around calculateCost usage and the methods that build metric/span names) to use effectiveModel instead of this.model so costs and telemetry reflect the actual used model; ensure setModelSubstitution() sets the effectiveModel and any span annotation logic remains intact so both span and cost/metric code are consistent.src/cli/commands/proxy.ts-1070-1078 (1)
1070-1078:⚠️ Potential issue | 🟠 MajorWrap the OTel shutdown calls with a timeout to prevent signal handler hangs.
The
flushOpenTelemetry()andshutdownOpenTelemetry()helpers call external provider methods (.forceFlush(),.shutdown()) without internal timeout protection. In a signal handler context, any hang blocks process termination (SIGTERM/SIGINT).Wrap the async calls at lines 1074–1075 with
withTimeout()fromsrc/lib/utils/async/withTimeout.tsusing a reasonable deadline (e.g., 5–10 seconds for OTel shutdown):Example fix
- await flushOpenTelemetry(); - await shutdownOpenTelemetry(); + await withTimeout(flushOpenTelemetry(), 5000, "OTel flush timeout"); + await withTimeout(shutdownOpenTelemetry(), 5000, "OTel shutdown timeout");This follows the coding guideline for
src/**/*.ts: wrap async operations with thewithTimeoututility.🤖 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 1070 - 1078, The OpenTelemetry shutdown calls (flushOpenTelemetry and shutdownOpenTelemetry) can hang in a signal handler; wrap both await calls with the withTimeout(...) helper to enforce a 5–10 second deadline (for example 5000 ms) and import/use withTimeout where the OTel helpers are invoked, so you call await withTimeout(flushOpenTelemetry(), 5000) and await withTimeout(shutdownOpenTelemetry(), 5000) inside the try block, keeping the catch to swallow non-fatal errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8c9cbb32-c53c-4906-a299-db9334a63120
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (114)
.github/workflows/ci.yml.github/workflows/copilot-review.yml.github/workflows/docs-deploy.yml.github/workflows/docs-pr-validation.yml.github/workflows/docs-version.yml.github/workflows/release.ymlCLAUDE.mdREADME.mddocs-site/package.jsondocs/advanced/auth-architecture.mddocs/advanced/mcp-integration.mddocs/assets/dashboards/neurolink-proxy-observability-dashboard.jsondocs/cli/commands.mddocs/cli/index.mddocs/cookbook/multi-provider-fallback.mddocs/cookbook/rate-limit-handling.mddocs/custom-middleware-guide.mddocs/features/claude-proxy-architecture.mddocs/features/claude-proxy-config-reference.mddocs/features/claude-proxy-observability.mddocs/features/claude-proxy-troubleshooting.mddocs/features/claude-proxy.mddocs/features/index.mddocs/features/observability.mddocs/features/provider-orchestration.mddocs/getting-started/index.mddocs/getting-started/providers/ollama.mddocs/guides/enterprise/multi-provider-failover.mddocs/guides/examples/code-patterns.mddocs/guides/frameworks/fastify.mddocs/guides/server-adapters/koa.mddocs/guides/server-adapters/middleware.mddocs/plans/2026-03-07-observability-api-wiring.mddocs/real-time-services.mddocs/reference/index.mddocs/reference/provider-selection.mddocs/sdk/nestjs-integration.mddocs/telemetry-guide.mdpackage.jsonscripts/build-browser.mjsscripts/observability/check-proxy-telemetry.mjsscripts/observability/docker-compose.proxy-observability.yamlscripts/observability/import-openobserve-dashboard.mjsscripts/observability/manage-local-openobserve.shscripts/observability/otel-collector.proxy-observability.yamlscripts/observability/proxy-observability.env.examplesrc/cli/commands/mcp.tssrc/cli/commands/proxy.tssrc/cli/commands/task.tssrc/cli/factories/commandFactory.tssrc/cli/parser.tssrc/lib/auth/anthropicOAuth.tssrc/lib/auth/providers/firebase.tssrc/lib/auth/providers/jwt.tssrc/lib/auth/providers/workos.tssrc/lib/auth/sessionManager.tssrc/lib/auth/tokenStore.tssrc/lib/client/aiSdkAdapter.tssrc/lib/client/streamingClient.tssrc/lib/core/modules/GenerationHandler.tssrc/lib/core/redisConversationMemoryManager.tssrc/lib/evaluation/BatchEvaluator.tssrc/lib/evaluation/hooks/observabilityHooks.tssrc/lib/evaluation/pipeline/evaluationPipeline.tssrc/lib/evaluation/pipeline/strategies/batchStrategy.tssrc/lib/evaluation/pipeline/strategies/samplingStrategy.tssrc/lib/neurolink.tssrc/lib/observability/otelBridge.tssrc/lib/providers/amazonBedrock.tssrc/lib/providers/anthropic.tssrc/lib/providers/anthropicBaseProvider.tssrc/lib/providers/azureOpenai.tssrc/lib/providers/googleAiStudio.tssrc/lib/providers/googleVertex.tssrc/lib/providers/huggingFace.tssrc/lib/providers/litellm.tssrc/lib/providers/mistral.tssrc/lib/providers/ollama.tssrc/lib/providers/openAI.tssrc/lib/providers/openRouter.tssrc/lib/providers/openaiCompatible.tssrc/lib/proxy/claudeFormat.tssrc/lib/proxy/cloaking/plugins/sessionIdentity.tssrc/lib/proxy/modelRouter.tssrc/lib/proxy/oauthFetch.tssrc/lib/proxy/proxyConfig.tssrc/lib/proxy/proxyEnv.tssrc/lib/proxy/proxyFetch.tssrc/lib/proxy/proxyTracer.tssrc/lib/proxy/rawStreamCapture.tssrc/lib/proxy/requestLogger.tssrc/lib/proxy/sseInterceptor.tssrc/lib/proxy/usageStats.tssrc/lib/rag/chunkers/MarkdownChunker.tssrc/lib/rag/chunking/markdownChunker.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/index.tssrc/lib/telemetry/telemetryService.tssrc/lib/types/cli.tssrc/lib/types/proxyTypes.tssrc/lib/types/streamTypes.tssrc/lib/utils/messageBuilder.tssrc/lib/utils/providerHealth.tssrc/lib/utils/providerUtils.tssrc/lib/utils/toolChoice.tstest/continuous-test-suite-workflow.tstest/continuous-test-suite.tstest/unit/quietDetector.test.tstest/unit/updateChecker.test.tstest/unit/updateState.test.ts
💤 Files with no reviewable changes (3)
- test/unit/updateState.test.ts
- test/unit/updateChecker.test.ts
- test/unit/quietDetector.test.ts
| async function injectTraceContext(init?: RequestInit): Promise<RequestInit> { | ||
| const carrier: Record<string, string> = {}; | ||
| propagation.inject(context.active(), carrier); | ||
|
|
||
| // Also inject NeuroLink session context from Langfuse AsyncLocalStorage | ||
| const langfuseContext = await getLangfuseContext(); | ||
| if (langfuseContext?.sessionId) { | ||
| carrier["x-neurolink-session-id"] = langfuseContext.sessionId; | ||
| } | ||
| if (langfuseContext?.userId) { | ||
| carrier["x-neurolink-user-id"] = langfuseContext.userId; | ||
| } | ||
| if (langfuseContext?.conversationId) { | ||
| carrier["x-neurolink-conversation-id"] = langfuseContext.conversationId; | ||
| } | ||
|
|
||
| if (Object.keys(carrier).length === 0) { | ||
| return init ?? {}; | ||
| } | ||
|
|
||
| const existingHeaders = new Headers(init?.headers); | ||
| for (const [key, value] of Object.entries(carrier)) { | ||
| if (!existingHeaders.has(key)) { | ||
| existingHeaders.set(key, value); | ||
| } | ||
| } | ||
|
|
||
| return { ...init, headers: existingHeaders }; |
There was a problem hiding this comment.
Preserve original Request headers when injecting trace context.
When createProxyFetch() is called with a Request object and optional init, injectTraceContext() only receives the init parameter and constructs a new RequestInit with the injected headers. If init does not include a headers property, the returned enrichedInit contains only the injected headers (e.g., x-trace-id, x-neurolink-*). When this enrichedInit is passed to fetch(originalRequest, enrichedInit), the original Request headers (such as authorization, content-type, and provider-specific headers) may be lost if the Fetch API does not properly merge headers between the Request object and RequestInit parameter.
🐛 Proposed fix
-async function injectTraceContext(init?: RequestInit): Promise<RequestInit> {
+async function injectTraceContext(
+ input: RequestInfo | URL,
+ init?: RequestInit,
+): Promise<RequestInit> {
const carrier: Record<string, string> = {};
propagation.inject(context.active(), carrier);
@@
- const existingHeaders = new Headers(init?.headers);
+ const existingHeaders = new Headers(
+ init?.headers ?? (input instanceof Request ? input.headers : undefined),
+ );
@@
- const enrichedInit = await injectTraceContext(init);
+ const enrichedInit = await injectTraceContext(input, init);
@@
- init = await injectTraceContext(init);
+ init = await injectTraceContext(input, init);Also applies to: 459, 515–516
🤖 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 38 - 65, injectTraceContext
currently only considers the provided RequestInit and injected carrier headers,
which can cause loss of headers when createProxyFetch is called with an original
Request (e.g., authorization, content-type). Update injectTraceContext (and the
createProxyFetch flow that calls it) to merge headers from the original Request
as well as init: obtain headers from init, then if a Request object was passed
into createProxyFetch extract its headers and copy any missing entries into the
Headers instance before adding carrier entries; ensure injectTraceContext
returns a RequestInit whose headers include original Request headers, init
headers, and the injected carrier headers so fetch(originalRequest,
enrichedInit) does not drop existing Request headers.
There was a problem hiding this comment.
Actionable comments posted: 16
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
src/lib/tasks/backends/bullmqBackend.ts (1)
89-118:⚠️ Potential issue | 🟠 MajorUndo executor registration when scheduling fails.
The executor is stored before any BullMQ write. If
upsertJobScheduler()oradd()rejects, the backend keeps a live executor for a task that never got scheduled, so failed creates/reschedules leak closures and can service stale jobs.🛠️ Suggested fix
async schedule(task: Task, executor: TaskExecutorFn): Promise<void> { const queue = this.getQueue(); this.executors.set(task.id, executor); const jobData = { taskId: task.id, task }; const schedule = task.schedule; - if (schedule.type === "cron") { - await queue.upsertJobScheduler( - task.id, - { - pattern: schedule.expression, - ...(schedule.timezone ? { tz: schedule.timezone } : {}), - }, - { name: task.name, data: jobData }, - ); - } else if (schedule.type === "interval") { - await queue.upsertJobScheduler( - task.id, - { every: schedule.every }, - { name: task.name, data: jobData }, - ); - } else if (schedule.type === "once") { - const at = - typeof schedule.at === "string" ? new Date(schedule.at) : schedule.at; - const delay = Math.max(0, at.getTime() - Date.now()); - await queue.add(task.name, jobData, { - jobId: task.id, - delay, - }); - } + try { + if (schedule.type === "cron") { + await queue.upsertJobScheduler( + task.id, + { + pattern: schedule.expression, + ...(schedule.timezone ? { tz: schedule.timezone } : {}), + }, + { name: task.name, data: jobData }, + ); + } else if (schedule.type === "interval") { + await queue.upsertJobScheduler( + task.id, + { every: schedule.every }, + { name: task.name, data: jobData }, + ); + } else if (schedule.type === "once") { + const at = + typeof schedule.at === "string" ? new Date(schedule.at) : schedule.at; + const delay = Math.max(0, at.getTime() - Date.now()); + await queue.add(task.name, jobData, { + jobId: task.id, + delay, + }); + } + } catch (err) { + this.executors.delete(task.id); + throw err; + } logger.info("[BullMQ] Task scheduled", { taskId: task.id, type: schedule.type,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/tasks/backends/bullmqBackend.ts` around lines 89 - 118, The schedule function registers the executor into this.executors before performing BullMQ writes (queue.upsertJobScheduler / queue.add), which can leak executors if those calls fail; fix by either moving this.executors.set(task.id, executor) to after the successful scheduling operation or keep the current placement but wrap the scheduling calls in try/catch and on any rejection call this.executors.delete(task.id) before rethrowing the error; reference the schedule method and the calls to queue.upsertJobScheduler and queue.add (and the executors map) to locate where to add the try/catch or relocation of the set.src/lib/providers/openAI.ts (1)
399-407:⚠️ Potential issue | 🟡 MinorUse one resolved
toolChoicevalue for both logging and request payload.Line 405 derives
toolChoicewith inline logic, while Line 445 now usesresolveToolChoice(...). If callers passoptions.toolChoice, logs can report a different value than what is actually sent.🔧 Suggested alignment
+ const resolvedToolChoice = resolveToolChoice(options, tools, shouldUseTools); + logger.debug(`OpenAI: streamText request parameters:`, { modelName: this.modelName, messagesCount: messages.length, temperature: options.temperature, maxTokens: options.maxTokens, toolsCount: Object.keys(tools).length, - toolChoice: - shouldUseTools && Object.keys(tools).length > 0 ? "auto" : "none", + toolChoice: resolvedToolChoice, maxSteps: options.maxSteps || DEFAULT_MAX_STEPS, firstToolExample: Object.keys(tools).length > 0 ? { name: Object.keys(tools)[0], @@ - toolChoice: resolveToolChoice(options, tools, shouldUseTools), + toolChoice: resolvedToolChoice,Also applies to: 445-445
🤖 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 399 - 407, Compute the toolChoice once using the existing resolveToolChoice(...) call and reuse that single resolved value for both logging and the request payload to avoid discrepancies; specifically, in the streamText implementation call resolveToolChoice(shouldUseTools, tools, options.toolChoice) (or the existing signature) and store the result in a local variable (e.g., resolvedToolChoice), then replace the inline logic in the logger.debug call and the later payload construction to use that variable; update references to options.toolChoice/inline checks so logger.debug, the request payload, and any subsequent logic all read the same resolvedToolChoice.src/lib/utils/providerUtils.ts (1)
147-162:⚠️ Potential issue | 🟠 MajorUse
OLLAMA_BASE_URLand match the full requested model, not just the family.This probe still hits
http://localhost:11434even whenOLLAMA_BASE_URLis configured, and the new fallback only compares the prefix before:, sollama3.1:70blooks healthy if onlyllama3.1:8bis installed. That can makegetBestProvider()prefer Ollama and then fail the real request.🔧 Suggested fix
if (providerName === "ollama") { try { - const response = await fetch("http://localhost:11434/api/tags", { + const ollamaBaseUrl = + process.env.OLLAMA_BASE_URL || "http://localhost:11434"; + const tagsUrl = new URL( + "api/tags", + ollamaBaseUrl.endsWith("/") ? ollamaBaseUrl : `${ollamaBaseUrl}/`, + ); + const response = await fetch(tagsUrl, { method: "GET", signal: AbortSignal.timeout(2000), }); if (response.ok) { const { models } = await response.json(); const defaultOllamaModel = process.env.OLLAMA_MODEL || "llama3.1:8b"; - // Check for exact match first, then prefix match (e.g. "gemma3:27b" matches "gemma3:27b-fp16") + const variantPrefix = `${defaultOllamaModel}-`; return models.some( (m: UnknownRecord) => - m.name === defaultOllamaModel || - (typeof m.name === "string" && - m.name.startsWith(defaultOllamaModel.split(":")[0] + ":")), + typeof m.name === "string" && + (m.name === defaultOllamaModel || + m.name.startsWith(variantPrefix)), ); } return false;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/utils/providerUtils.ts` around lines 147 - 162, The ollama probe is hardcoded to "http://localhost:11434" and only compares the model family prefix; update the fetch in the provider probe (the branch where providerName === "ollama") to use process.env.OLLAMA_BASE_URL (defaulting to "http://localhost:11434") instead of the literal URL, and change the model check for defaultOllamaModel (from process.env.OLLAMA_MODEL || "llama3.1:8b") to verify the full requested model string is present in the returned models (match m.name === defaultOllamaModel), allowing a sensible suffix tolerance only if Ollama returns variants with suffixes (e.g., treat m.name that starts with defaultOllamaModel + "-" as acceptable); update the models.some logic accordingly so getBestProvider() only prefers Ollama when the exact requested model is available.src/lib/proxy/proxyFetch.ts (1)
590-593:⚠️ Potential issue | 🟠 MajorKey the proxy-agent cache with a credential-aware value.
maskProxyUrl()makeshttp://user-a@proxy:8080andhttp://user-b@proxy:8080collide, so oneProxyAgentcan be reused with the wrong proxy credentials. That breaks mixed HTTP/HTTPS proxy auth setups and can leak the wrong auth to later requests.🔐 Proposed fix
- const cacheKey = maskProxyUrl(proxyUrl) ?? proxyUrl; // mask credentials in cache key + // Use the full proxy URL (or a hash of it) so agents with different + // credentials do not collide in the cache. + const cacheKey = proxyUrl;🤖 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 590 - 593, The cache key currently uses maskProxyUrl(proxyUrl) which strips credentials and causes different credentialed URLs (e.g., user-a@proxy and user-b@proxy) to collide; update the agentCache key logic to include credential-aware information so agents are not reused across different proxy credentials: use the original proxyUrl or a derived key that preserves credentials when calling agentCache.get/set (referencing maskProxyUrl, proxyUrl, agentCache, createProxyAgent, and the ProxyAgent creation flow) so each unique credential set gets its own cached ProxyAgent.src/lib/services/server/ai/observability/instrumentation.ts (1)
556-573:⚠️ Potential issue | 🟠 MajorDon't short-circuit OTLP-only telemetry in external-provider mode.
This branch returns early whenever Langfuse credentials are missing. If a host follows the documented
useExternalTracerProvider: truepath and only setsOTEL_EXPORTER_OTLP_ENDPOINT, we never reach the meter/logger setup below, so logs and metrics are silently disabled. Because this path also latchesisInitialized = true, a later re-init cannot recover either.Based on learnings: External TracerProvider mode requires useExternalTracerProvider: true and disables internal TracerProvider creation/registration.
src/lib/proxy/requestLogger.ts (1)
708-734:⚠️ Potential issue | 🟠 MajorThe 500 MB cleanup pass still won't control body-artifact growth.
bodiesSizehere is calculated from the date directories themselves, not recursively from the.json.gzfiles, and the eviction loop only deletes entries fromremaining. Once body captures dominate disk usage, cleanup can neither measure nor reclaim the actual space.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/proxy/requestLogger.ts` around lines 708 - 734, The cleanup currently measures bodiesSize by summing sizes of date directories (bodiesDirForSize) and never includes the individual body artifact files in the eviction loop (remaining), so body captures can grow unbounded; fix by walking bodiesDirForSize recursively (or iterate each date subdirectory) to sum sizes of the actual body files (e.g., .json.gz) and either (A) add each body file as an object into the same remaining list (with .path and .size) so the existing eviction loop can delete them, or (B) run a parallel eviction that deletes the oldest body files (using unlinkSync) while decrementing totalSize, deletedCount, and freedBytes until totalSize <= maxBytes; ensure you reference bodiesDirForSize, bodiesSize, remaining, totalSize, maxBytes, unlinkSync and update totalSize/freedBytes when removing body files.
♻️ Duplicate comments (1)
src/lib/proxy/proxyFetch.ts (1)
38-65:⚠️ Potential issue | 🔴 CriticalStill preserve
Requestheaders when injecting trace context.When
inputis aRequest, this helper only clonesinit?.headers. As soon as it returns aheadersfield, the laterfetchWithRetry(input, enrichedInit)and proxiedundici.fetch(fetchInput, fetchInit)paths can replace the original request headers with just the injected carrier values, dropping auth/content-type on traced requests.🐛 Proposed fix
-async function injectTraceContext(init?: RequestInit): Promise<RequestInit> { +async function injectTraceContext( + input: RequestInfo | URL, + init?: RequestInit, +): Promise<RequestInit> { const carrier: Record<string, string> = {}; propagation.inject(context.active(), carrier); @@ - const existingHeaders = new Headers(init?.headers); + const existingHeaders = new Headers( + init?.headers ?? (input instanceof Request ? input.headers : undefined), + ); @@ - const enrichedInit = await injectTraceContext(init); + const enrichedInit = await injectTraceContext(input, init); @@ - init = await injectTraceContext(init); + init = await injectTraceContext(input, init);Also applies to: 441-443, 515-516
🤖 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 38 - 65, injectTraceContext currently only copies init?.headers so when the caller passes a Request object its original headers (auth/content-type) can be lost; update injectTraceContext to build the merged Headers from three sources in order of precedence: start with headers from the Request input (if fetch input is a Request — locate where injectTraceContext is called and ensure it receives the Request or expose the Request to this function), then apply init?.headers, and finally apply the carrier entries but do not overwrite any existing header keys (use Headers.has to guard). Ensure the function returns the merged Headers instance as headers in the returned RequestInit so both fetchWithRetry and undici.fetch keep original request headers while adding trace fields (also apply the same merge logic at the other occurrences around lines referenced in the comment).
🟡 Minor comments (13)
docs/real-time-services.md-238-239 (1)
238-239:⚠️ Potential issue | 🟡 MinorFix callback name inconsistency in documentation comment.
The comment refers to
onFinish, but the actual code example below usesonComplete(line 250). This inconsistency may confuse readers.📝 Proposed fix to align comment with code example
-// Note: onChunk, onFinish, and onError callbacks are preserved through middleware — +// Note: onChunk, onComplete, and onError callbacks are preserved through middleware — // middleware will not override callbacks you've explicitly set.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/real-time-services.md` around lines 238 - 239, The documentation comment references onFinish but the example code uses onComplete; update the comment so callback names match the example by replacing onFinish with onComplete (or alternatively change the example to onFinish) to keep consistency between the comment and the code example; ensure you update the line mentioning callbacks preserved through middleware to read "onChunk, onComplete, and onError" and verify no other occurrences of onFinish remain in the surrounding text.src/lib/tasks/taskManager.ts-225-229 (1)
225-229:⚠️ Potential issue | 🟡 MinorRoll back callback registration when create fails.
If
backend.schedule()rejects, the task is deleted from storage but itsonSuccess/onError/onCompleteclosures remain inthis.callbacks.🛠️ Suggested fix
try { await backend.schedule(task, (t) => this.onTaskTick(t)); } catch (err) { await store.delete(task.id); + this.callbacks.delete(task.id); throw err; }🤖 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 225 - 229, When backend.schedule(task, (t) => this.onTaskTick(t)) rejects, the task record is deleted but the registered closures remain in this.callbacks; update the catch block to also roll back the callback registration for that task (e.g., remove this.callbacks entry for task.id or call an existing unregister/clear method) before rethrowing so onSuccess/onError/onComplete references are not retained for a failed create; ensure you target the callback storage used by TaskManager (this.callbacks / onTaskTick / onSuccess/onError/onComplete) to fully clear those closures.docs/reference/provider-selection.md-492-492 (1)
492-492:⚠️ Potential issue | 🟡 MinorAvoid the blanket “no rate limits” claim in fallback docs.
Line 492 can mislead users: self-hosted LiteLLM often still inherits upstream model/provider rate limits. Consider wording this as “prioritize local/self-hosted to reduce cloud rate-limit exposure.”
📝 Suggested wording
-// Default priority: self-hosted first (no rate limits), then cloud providers +// Default priority: local/self-hosted first to reduce cloud limits/cost, then cloud providers🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/reference/provider-selection.md` at line 492, Replace the misleading phrase in the docs entry that currently reads "Default priority: self-hosted first (no rate limits), then cloud providers" with wording that avoids an absolute "no rate limits" claim—e.g., "Default priority: prioritize local/self-hosted to reduce cloud rate-limit exposure; note self-hosted LiteLLM may still inherit upstream provider limits." Locate and edit the line containing that exact string to preserve context and ensure the new text explicitly mentions that local deployments can still be subject to upstream/model rate limits.CLAUDE.md-1032-1032 (1)
1032-1032:⚠️ Potential issue | 🟡 MinorResolve test-coverage wording contradiction in the Evaluation row.
Line 1032 says “0 tests” but the note says there are CLI integration tests. Please make this consistent (e.g., “0 unit tests” if that is the intent).
📝 Suggested wording
-| **Evaluation/Scoring** | ⚠️ Code complete, 0 tests | RAGAS-based evaluator, 14 scorers, pipelines, CLI — integration tests only, no unit tests | +| **Evaluation/Scoring** | ⚠️ Code complete, 0 unit tests | RAGAS-based evaluator, 14 scorers, pipelines, CLI integration tests only |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLAUDE.md` at line 1032, The "Evaluation/Scoring" table cell currently reads "0 tests" but the note mentions CLI integration tests; update the wording in that table row (the "Evaluation/Scoring" entry) to be consistent — e.g., change "0 tests" to "0 unit tests" or to "0 unit tests, integration tests present" so it accurately reflects that only integration tests exist (adjust the same cell text to match whichever phrasing you choose).docs/features/claude-proxy-troubleshooting.md-235-240 (1)
235-240:⚠️ Potential issue | 🟡 MinorCondition the
traceId/spanIdclaim to avoid contradictory guidance.Line 235 reads as unconditional, but Line 239 says these fields appear when
OTEL_EXPORTER_OTLP_ENDPOINTis set.✏️ Suggested wording fix
-Each log entry includes: timestamp, request ID, method, path, model, account label, response status, response time (ms), token usage, and OTel correlation fields (`traceId`, `spanId`). +Each log entry includes: timestamp, request ID, method, path, model, account label, response status, response time (ms), and token usage. +When `OTEL_EXPORTER_OTLP_ENDPOINT` is set, entries also include OTel correlation fields (`traceId`, `spanId`).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/features/claude-proxy-troubleshooting.md` around lines 235 - 240, The doc currently states that every log entry includes `traceId` and `spanId` unconditionally but later says those fields only appear when `OTEL_EXPORTER_OTLP_ENDPOINT` is set; update the wording so it is consistent by conditioning the presence of the OTel fields on the environment variable (OTEL_EXPORTER_OTLP_ENDPOINT) and clarifying that logs will include `traceId` and `spanId` only when that exporter is configured; modify the paragraph that lists log entry fields (mentioning `traceId`, `spanId`) and the "Correlate logs with traces" section so both explicitly reference OTEL_EXPORTER_OTLP_ENDPOINT and use the same phrasing for `traceId`/`spanId`.docs/cookbook/multi-provider-fallback.md-294-303 (1)
294-303:⚠️ Potential issue | 🟡 MinorUpdate nearby fallback-flow text to match the new local-first order.
After this change, the later walkthrough still describes the old OpenAI→Anthropic→Google sequence, which now conflicts with this section’s default order and can mislead users.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/cookbook/multi-provider-fallback.md` around lines 294 - 303, Update the fallback-flow narrative to match the new local-first provider order defined by the providers array and getBestProvider(): change any walkthrough text that lists the old OpenAI→Anthropic→Google sequence to the new sequence starting with self-hosted and local providers (litellm, ollama) followed by cloud fallbacks (openai, anthropic, google-ai), and ensure examples, diagrams, and any conditional logic descriptions referencing provider priority reflect getBestProvider()'s priority ordering.docs/features/observability.md-371-373 (1)
371-373:⚠️ Potential issue | 🟡 MinorReconcile the new auto-reuse note with the troubleshooting guidance.
Line 373 says
TelemetryServicecan reuse an existing globalTracerProvider, but the troubleshooting section later still says the fix for duplicate registration is to setuseExternalTracerProvider: true. Please spell out when auto-reuse is enough and when explicit external mode is still required. Based on learnings: External TracerProvider mode requires useExternalTracerProvider: true and disables internal TracerProvider creation/registration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/features/observability.md` around lines 371 - 373, Clarify that TelemetryService will automatically detect and reuse a global TracerProvider in most cases, but when you need to fully disable the library’s internal TracerProvider creation/registration you must enable explicit external provider mode by setting useExternalTracerProvider: true; update the docs to state that auto-reuse covers simple coexistence (it reuses an already-registered global TracerProvider to avoid duplicates), whereas more complex scenarios where you need to prevent any internal registration or lifecycle management (for example when your application supplies and controls the TracerProvider entirely) require setting useExternalTracerProvider: true to disable internal creation/registration and opt into external TracerProvider mode.scripts/observability/docker-compose.proxy-observability.yaml-39-40 (1)
39-40:⚠️ Potential issue | 🟡 MinorDefault credentials are visible in the config file.
The default
ZO_ROOT_USER_EMAILandZO_ROOT_USER_PASSWORDare embedded in the compose file. While acceptable for local dev tooling, add a comment noting these should be overridden via the.envfile in any shared or persistent setup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/observability/docker-compose.proxy-observability.yaml` around lines 39 - 40, The compose file currently exposes default credentials for ZO_ROOT_USER_EMAIL and ZO_ROOT_USER_PASSWORD; update the docker-compose section containing ZO_ROOT_USER_EMAIL and ZO_ROOT_USER_PASSWORD to add a clear inline comment instructing developers to override these values via the .env file for any shared or persistent environment (and not to commit real credentials), and ensure the variables reference the environment fallbacks as currently written so local dev keeps defaults while deployments use .env-provided values.src/lib/proxy/rawStreamCapture.ts-56-65 (1)
56-65:⚠️ Potential issue | 🟡 MinorVariable naming mismatch:
capturedBytestracks character count, not bytes.
capturedBytesis incremented bydecoded.length(character count after UTF-8 decoding), but the name suggests byte counting. SinceMAX_CAPTURE_BYTESis named as bytes, this could cause confusion or unexpected truncation points for multi-byte UTF-8 characters.If the intent is to limit captured text to approximately 1MB of characters, consider renaming to
capturedCharsor adding a comment. If the intent is to limit actual bytes, usechunk.byteLengthinstead ofdecoded.length.🔧 Suggested clarification
-/** Maximum bytes to capture before stopping accumulation (1 MB). */ -const MAX_CAPTURE_BYTES = 1024 * 1024; +/** Maximum characters to capture before stopping accumulation (~1M chars). */ +const MAX_CAPTURE_CHARS = 1024 * 1024; const TRUNCATION_MARKER = "\n...[TRUNCATED]"; export function createRawStreamCapture(): RawStreamCaptureResult { const decoder = new TextDecoder(); const chunks: string[] = []; let totalBytes = 0; - let capturedBytes = 0; + let capturedChars = 0;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/proxy/rawStreamCapture.ts` around lines 56 - 65, The variable capturedBytes is misleading because the code measures characters via decoded.length but compares to MAX_CAPTURE_BYTES (bytes); rename capturedBytes to capturedChars (and update all references, including the comparisons and the capturedBytes += ... line) and add a short comment near MAX_CAPTURE_BYTES (or next to the new capturedChars) clarifying that the limit is a character-count limit not a byte limit; alternatively if you intended to limit raw bytes, replace uses of decoded.length with chunk.byteLength and ensure slicing/decoding logic handles byte-level truncation—locate symbols: capturedBytes, MAX_CAPTURE_BYTES, decoded, chunk.byteLength, chunks, TRUNCATION_MARKER, and truncated.scripts/observability/import-openobserve-dashboard.mjs-45-64 (1)
45-64:⚠️ Potential issue | 🟡 MinorValidate option values before consuming the next argv slot.
If one of these flags is last or followed by another flag,
argv[index + 1]becomesundefinedor--next-flag, and the script fails much later with a misleading path/auth error. Please fail fast inparseArgs()for every value-taking option.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/observability/import-openobserve-dashboard.mjs` around lines 45 - 64, The parseArgs() switch currently grabs argv[index + 1] for flags like "--dashboard", "--openobserve-url", "--org", "--user", and "--password" without validating it; update parseArgs() to check that argv[index + 1] exists and is not another flag (e.g., does not start with "--") before assigning to options.dashboard, options.openobserveUrl, options.org, options.user, options.password, and if invalid, throw or exit with a clear message indicating the missing value for the specific flag; ensure you increment index only after a successful validation so the parser fails fast on missing/invalid values.docs/features/claude-proxy-config-reference.md-28-29 (1)
28-29:⚠️ Potential issue | 🟡 MinorMention
NEUROLINK_ENV_FILEalongside--env-file.This file is positioned as the authoritative config reference, but this new flag description only shows the CLI path. Call out the env-based alternative here too so CI/service users can discover it without digging into implementation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/features/claude-proxy-config-reference.md` around lines 28 - 29, Update the CLI flag docs row for `--env-file` to also call out the environment variable alternative `NEUROLINK_ENV_FILE` so readers can discover the env-based config path; modify the description for the `--env-file` entry (the table row referencing `--env-file`) to mention that `NEUROLINK_ENV_FILE` can be used as an equivalent way to specify the path to the .env file (and note precedence if applicable, e.g., CLI flag overrides env var).src/lib/proxy/proxyTracer.ts-219-222 (1)
219-222:⚠️ Potential issue | 🟡 MinorHandle non-numeric environment variable values to avoid NaN.
If
NEUROLINK_PROXY_TRACE_BODY_LOG_BYTESis set to a non-numeric value,parseIntreturnsNaN. The??operator only handlesnull/undefined, notNaN. This would causebody.length > NaNto always befalse, disabling truncation.🛠️ Proposed fix
-const MAX_BODY_LOG_SIZE = Number.parseInt( - process.env.NEUROLINK_PROXY_TRACE_BODY_LOG_BYTES ?? "8192", - 10, -); +const MAX_BODY_LOG_SIZE = (() => { + const parsed = Number.parseInt( + process.env.NEUROLINK_PROXY_TRACE_BODY_LOG_BYTES ?? "8192", + 10, + ); + return Number.isNaN(parsed) ? 8192 : parsed; +})();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/proxy/proxyTracer.ts` around lines 219 - 222, The MAX_BODY_LOG_SIZE initialization should validate the parsed environment value to avoid NaN (which breaks checks like body.length > MAX_BODY_LOG_SIZE); update the code that sets MAX_BODY_LOG_SIZE (currently using Number.parseInt and NEUROLINK_PROXY_TRACE_BODY_LOG_BYTES) to parse the env var, ensure it is a finite positive integer (e.g., via Number.parseInt(...) then Number.isFinite/Number.isInteger and > 0), and fall back to 8192 if validation fails so subsequent uses (like the body.length > MAX_BODY_LOG_SIZE check) behave correctly.src/lib/observability/otelBridge.ts-33-38 (1)
33-38:⚠️ Potential issue | 🟡 MinorHandle comma-only
traceparentconcatenation as well.On Line 34, normalization only detects
", "; some intermediaries emit"value1,value2"(no space). In that case extraction can still fail and parent trace linkage is lost.Suggested fix
- if ( - typeof normalizedHeaders["traceparent"] === "string" && - normalizedHeaders["traceparent"].includes(", ") - ) { - normalizedHeaders["traceparent"] = - normalizedHeaders["traceparent"].split(", ")[0]; - } + if (typeof normalizedHeaders["traceparent"] === "string") { + const traceparent = normalizedHeaders["traceparent"]; + if (traceparent.includes(",")) { + normalizedHeaders["traceparent"] = traceparent.split(",")[0].trim(); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/observability/otelBridge.ts` around lines 33 - 38, The traceparent normalization only strips values when the header contains ", " and misses comma-only concatenations; update the logic around normalizedHeaders["traceparent"] so after confirming typeof normalizedHeaders["traceparent"] === "string" you split on a comma with optional whitespace (e.g., split using a regex like /,\s*/ or check includes(",") and then take the first segment and trim it) and assign that first segment back to normalizedHeaders["traceparent"] to handle both "value1, value2" and "value1,value2" cases.
🧹 Nitpick comments (13)
src/cli/commands/task.ts (1)
481-484: Good defensive validation; consider usingErrorFactoryfor consistency.The explicit runtime check is a solid improvement over the previous non-null assertion. While yargs
.check()at lines 150-165 already guaranteesargv.atis present when reaching this branch, this defensive guard provides an extra safety net against potential bypasses.For alignment with project conventions, consider using
ErrorFactoryinstead of a plainError.,
♻️ Suggested refactor using ErrorFactory
} else { if (!argv.at) { - throw new Error("One-time tasks require --at"); + throw ErrorFactory.validation("One-time tasks require --at"); } schedule = { type: "once", at: argv.at }; }This requires importing
ErrorFactoryat the top of the file:import { ErrorFactory } from "../../lib/errors/errorFactory.js";As per coding guidelines:
src/**/*.ts: Use ErrorFactory for creating typed errors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/commands/task.ts` around lines 481 - 484, Replace the plain Error thrown when argv.at is missing with a typed error created via ErrorFactory: import ErrorFactory (e.g., import { ErrorFactory } from "../../lib/errors/errorFactory.js";) at the top of the file and change the throw in the branch that checks argv.at (the block that sets schedule = { type: "once", at: argv.at }) to use ErrorFactory to construct and throw an appropriate error (e.g., ErrorFactory.create(...) with a clear code/message) so it follows project conventions and preserves the existing defensive guard.src/cli/factories/commandFactory.ts (1)
593-606: Consider making the path correlation more explicit.The loop assumes
resolvedPaths[i]directly corresponds torawPaths[i], which depends onresolveFilePathsmaintaining array length and order. If that function ever filters invalid entries, the error messages would associate the wrong original paths.♻️ Optional: Use zip-style iteration for robustness
const rawPaths = Array.isArray(value) ? value : [value]; const resolvedPaths = resolveFilePaths(rawPaths); - for (let i = 0; i < resolvedPaths.length; i++) { - const resolvedPath = resolvedPaths[i]; + for (const [rawPath, resolvedPath] of rawPaths.map((raw, i) => [raw, resolvedPaths[i]] as const)) { if (CLICommandFactory.isNonLocalFileReference(resolvedPath)) { continue; } if (!fs.existsSync(resolvedPath)) { missingPaths.push( - `${option} path not found: ${rawPaths[i]} (resolved to ${resolvedPath})` + `${option} path not found: ${rawPath} (resolved to ${resolvedPath})` ); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/factories/commandFactory.ts` around lines 593 - 606, The error messages assume index alignment between rawPaths and resolvedPaths, which can break if resolveFilePaths filters or reorders entries; update the loop in the validation that uses resolveFilePaths/rawPaths to explicitly correlate each resolvedPath with its original rawPath (e.g., produce a zipped array of { rawPath, resolvedPath } or change resolveFilePaths to return mappings), then use that mapping when calling CLICommandFactory.isNonLocalFileReference and when pushing to missingPaths so the pushed message references the correct original rawPath and includes option and resolvedPath.src/cli/commands/mcp.ts (1)
2995-3324: SplitexecuteAnnotateto address function-size lint warning and reduce complexity.The pipeline warning shows
executeAnnotateexceeds max-lines-per-function. Extracting list-mode handling, tool lookup, annotation assembly, and rendering into helpers will make this path easier to maintain and test.🤖 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 2995 - 3324, The executeAnnotate function is too large and should be split into smaller helpers to satisfy the lint rule; extract the list-mode code into a new function (e.g., handleAnnotateListMode) that takes argv, servers, and logger and uses inferAnnotations/getAnnotationSummary, extract tool lookup into findTool(servers, toolName, serverId) returning the foundTool shape, extract annotation assembly into buildAnnotations(argv, foundTool) which uses JSON.parse, inferAnnotations, mergeAnnotations and validateAnnotations, and extract the final rendering into renderAnnotationPreview(foundTool, annotations, argv) which prints the same outputs; replace the big bodies in executeAnnotate with calls to these helpers, preserving error handling and process.exit behavior and keeping references to inferAnnotations, mergeAnnotations, validateAnnotations, and getAnnotationSummary so existing behavior and tests remain unchanged.src/lib/evaluation/pipeline/evaluationPipeline.ts (1)
261-272: Consider using ErrorFactory for typed errors.The validation throws a plain
Error. Per coding guidelines,ErrorFactoryshould be used for creating typed errors insrc/**/*.tsfiles. This would provide consistent error handling and better error categorization.♻️ Suggested refactor using ErrorFactory
+import { ErrorFactory } from "../../utils/errorHandling.js"; + private _validateExecutionOptions(options?: PipelineExecutionOptions): void { const hasOnlyScorers = !!options?.onlyScorers && options.onlyScorers.length > 0; const hasSkipScorers = !!options?.skipScorers && options.skipScorers.length > 0; if (hasOnlyScorers && hasSkipScorers) { - throw new Error( - "Cannot specify both 'onlyScorers' and 'skipScorers' options", - ); + throw ErrorFactory.validation( + "Cannot specify both 'onlyScorers' and 'skipScorers' options", + { onlyScorers: options.onlyScorers, skipScorers: options.skipScorers }, + ); } }As per coding guidelines: "Use ErrorFactory for creating typed errors".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/evaluation/pipeline/evaluationPipeline.ts` around lines 261 - 272, The _validateExecutionOptions method currently throws a plain Error when both onlyScorers and skipScorers are specified; replace that throw with a typed error created via ErrorFactory (e.g., ErrorFactory.create / appropriate factory method) so callers get a consistent, typed error; keep the same descriptive message ("Cannot specify both 'onlyScorers' and 'skipScorers' options") and throw the ErrorFactory-produced error from _validateExecutionOptions (which accepts PipelineExecutionOptions) instead of new Error.docs/guides/server-adapters/koa.md (1)
325-327: Consider using ES module import for consistency.The rest of this documentation file uses ES module
importsyntax, but this snippet usesrequire(). For consistency:📝 Suggested fix
-const { createClient } = require("redis"); +import { createClient } from "redis";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/guides/server-adapters/koa.md` around lines 325 - 327, The snippet uses CommonJS require to get createClient and construct redis (const { createClient } = require("redis"); const redis = createClient(); await redis.connect();) but the rest of the doc uses ES module imports; replace the require with an ES import by importing createClient via import { createClient } from "redis" and then instantiate and connect using the same redis variable (redis = createClient(); await redis.connect()); update any surrounding wording to reflect the ES module form and ensure createClient and redis are referenced consistently.src/lib/types/proxyTypes.ts (1)
264-271: KeepinputSchemarequired on fallback tools.Claude
tools[]always includeinput_schema, so making the translatedinputSchemaoptional only drops a useful compile-time guarantee and hides malformed parser output from provider code.executecan stay optional.🧩 Suggested tightening
tools: Record< string, { description?: string; - inputSchema?: unknown; + inputSchema: unknown; execute?: (...args: unknown[]) => unknown; } >;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/types/proxyTypes.ts` around lines 264 - 271, The tools record in proxyTypes.ts currently makes inputSchema optional; change the tools type so that inputSchema is required (i.e., remove the optional marker on inputSchema) while leaving execute optional, so provider fallback code and callers (the tools property) always have an inputSchema compile-time guarantee; update the inline comment if needed and ensure the type change targets the tools: Record< string, { description?: string; inputSchema: unknown; execute?: (...args: unknown[]) => unknown; } > declaration (the tools field/type) so malformed parser output is caught at compile time.src/lib/proxy/proxyConfig.ts (1)
588-595: Inconsistent guard inloadProxyConfigvsparseProxyConfigString.
loadProxyConfigat line 589 checksrawAccounts && typeof rawAccounts === "object"but omits the!Array.isArray(rawAccounts)guard thatparseProxyConfigStringcorrectly includes at lines 686-688. While validation should catch array-shaped accounts earlier, adding the same guard here maintains consistency and defensive safety.♻️ Suggested fix for consistency
const rawAccounts = raw.accounts as Record<string, unknown[]> | undefined; - if (rawAccounts && typeof rawAccounts === "object") { + if (rawAccounts && typeof rawAccounts === "object" && !Array.isArray(rawAccounts)) { for (const [provider, list] of Object.entries(rawAccounts)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/proxy/proxyConfig.ts` around lines 588 - 595, The guard in loadProxyConfig currently allows array-shaped values for rawAccounts; update the conditional that defines rawAccounts in loadProxyConfig to mirror parseProxyConfigString by adding a "!Array.isArray(rawAccounts)" check so you only iterate when rawAccounts is an object (not an array), leaving the mapping to applyAccountDefaults(item as Partial<ProxyAccountConfig>) unchanged; reference loadProxyConfig, rawAccounts, parseProxyConfigString, and applyAccountDefaults when making this small defensive change.scripts/observability/docker-compose.proxy-observability.yaml (1)
12-13: Consider pinning image versions for reproducibility.Using
:latesttags forotel/opentelemetry-collector-contribandpublic.ecr.aws/zinclabs/openobservecan lead to inconsistent behavior across environments. For local development tooling, consider pinning to specific versions or at least documenting the tested versions.♻️ Example version pinning
services: otel-collector: - image: otel/opentelemetry-collector-contrib:latest + image: otel/opentelemetry-collector-contrib:0.96.0 restart: unless-stoppedopenobserve: - image: public.ecr.aws/zinclabs/openobserve:latest + image: public.ecr.aws/zinclabs/openobserve:v0.10.0 restart: unless-stoppedAlso applies to: 33-33
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/observability/docker-compose.proxy-observability.yaml` around lines 12 - 13, The docker-compose uses floating image tags "otel/opentelemetry-collector-contrib:latest" and "public.ecr.aws/zinclabs/openobserve:latest"; replace those :latest tags with a pinned version (e.g., a tested semver or digest) for reproducibility, e.g., update the image fields for the OpenTelemetry collector and OpenObserve service referenced by those image strings, or parameterize them via env vars (e.g., OTEL_COLLECTOR_IMAGE and OPENOBSERVE_IMAGE) and document the tested versions in README so CI/local dev use the same pinned images.src/lib/telemetry/telemetryService.ts (1)
85-112: Reliance on internal_delegateproperty is fragile.The
hasExternalTracerProvider()method accessesprovider._delegate?.constructor?.namewhich is an internal implementation detail of OpenTelemetry'sProxyTracerProvider. This could break with future OpenTelemetry updates.Consider adding a comment documenting this dependency, and/or wrapping the check in a try-catch with a fallback strategy (which is already partially done).
📝 Suggested documentation
private hasExternalTracerProvider(): boolean { try { + // NOTE: We inspect _delegate which is an internal OpenTelemetry API detail. + // ProxyTracerProvider wraps the actual provider in _delegate. This check + // may need updating if OpenTelemetry changes its internal structure. const provider = trace.getTracerProvider() as { constructor?: { name?: string }; _delegate?: { constructor?: { name?: string } }; } | null;🤖 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 85 - 112, The method hasExternalTracerProvider() is fragile because it directly reads the internal provider._delegate property; update it to avoid relying on that internal field: remove or stop using provider._delegate?.constructor?.name and instead prefer safe, public checks (e.g., inspect provider.constructor?.name for "ProxyTracerProvider" / "NoopTracerProvider" and/or feature-detect public APIs like typeof provider.getTracer === "function"), and keep the existing try/catch; also add an inline comment in hasExternalTracerProvider() documenting that inspecting _delegate is an implementation detail of some OpenTelemetry providers and that we fall back to constructor-name and feature-detection to remain resilient to future OpenTelemetry changes.src/lib/providers/googleAiStudio.ts (1)
61-63: Side effect at module load time.Setting
process.env.GOOGLE_GENERATIVE_AI_API_KEYat module load time (top-level scope) could cause unexpected behavior ifGOOGLE_AI_API_KEYis modified after the module is first imported. Consider moving this togetApiKey()or constructor for more predictable behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/providers/googleAiStudio.ts` around lines 61 - 63, The module currently assigns process.env.GOOGLE_GENERATIVE_AI_API_KEY at import time; remove that top-level side-effect and instead implement the fallback logic inside the provider's runtime path (e.g., inside getApiKey() or the provider constructor). Specifically, delete the top-level block that sets process.env.GOOGLE_GENERATIVE_AI_API_KEY and update getApiKey() (or the class constructor responsible for configuration) to return process.env.GOOGLE_GENERATIVE_AI_API_KEY || process.env.GOOGLE_AI_API_KEY (or set the env var there if you prefer), so the fallback is evaluated when the key is actually requested rather than at module load.src/cli/commands/proxy.ts (1)
466-1125: Extract startup stages out ofproxyStartCommand.handler.This handler now bundles env loading, config resolution, route wiring, OTel init, state persistence, and shutdown in one 600+ line function. Splitting those stages would clear the current
max-lines-per-functionCI warning and make the failure paths much easier to audit.🤖 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 466 - 1125, The handler function is too large and should be decomposed: extract discrete startup stages into smaller named functions (e.g., loadProxyEnvFileStage, loadProxyConfigStage, createModelRouterStage, buildClaudeAppStage, initObservabilityStage, startServerStage, and setupShutdownHandlers) and have proxyStartCommand.handler orchestrate them sequentially; each new function should encapsulate the logic currently in the handler (env loading/error handling around loadProxyEnvFile, config loading/loadProxyConfig, modelRouter creation/ModelRouter, route wiring/createClaudeProxyRoutes and app.onError, OTEL initialization/initializeOpenTelemetry, starting the server/serve and spawnFailOpenGuard, state persistence/saveProxyState, and shutdown logic/shutdown), return any necessary artifacts (neurolink instance, app, server, port/host, refresh timers, guardPid, loadedEnvFile, proxyConfig) and surface errors so the top-level handler can log/spinner/fail consistently, preserving behavior such as launchd guards, signal handling, and Claude auto-configure calls (setClaudeProxySettings/clearClaudeProxySettings).src/lib/auth/anthropicOAuth.ts (1)
108-113: Consider adding a cache size limit to prevent unbounded memory growth.The
claudeCodeIdentityCachehas TTL-based expiration but no maximum size. If many unique seeds are used (e.g., per-request seeds), the cache could grow unboundedly untilpurgeExpiredClaudeCodeIdentities()is called. Consider adding an LRU eviction policy or size cap.💡 Example: Add size limit check
const CLAUDE_CODE_IDENTITY_TTL_MS = 3_600_000; const CLAUDE_CODE_IDENTITY_NAMESPACE = "neurolink-claude-code-identity-v1"; +const CLAUDE_CODE_IDENTITY_CACHE_MAX_SIZE = 10_000; const claudeCodeIdentityCache = new Map< string, ClaudeCodeIdentity & { expiresAt: number } >();Then in
getOrCreateClaudeCodeIdentity, before inserting:if (claudeCodeIdentityCache.size >= CLAUDE_CODE_IDENTITY_CACHE_MAX_SIZE) { purgeExpiredClaudeCodeIdentities(now); // If still at limit, evict oldest entry if (claudeCodeIdentityCache.size >= CLAUDE_CODE_IDENTITY_CACHE_MAX_SIZE) { const firstKey = claudeCodeIdentityCache.keys().next().value; if (firstKey) claudeCodeIdentityCache.delete(firstKey); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/anthropicOAuth.ts` around lines 108 - 113, The cache claudeCodeIdentityCache currently has only TTL eviction; add a size cap constant (e.g., CLAUDE_CODE_IDENTITY_CACHE_MAX_SIZE) and enforce it inside getOrCreateClaudeCodeIdentity: before inserting a new entry call purgeExpiredClaudeCodeIdentities(now), and if the cache is still at or above the max, remove the oldest Map entry (use claudeCodeIdentityCache.keys().next().value) to bound memory; update any related tests/docs and keep TTL logic (CLAUDE_CODE_IDENTITY_TTL_MS) unchanged.src/lib/proxy/proxyTracer.ts (1)
840-870: Cost calculation is performed multiple times - consider caching.The cost is calculated in
setUsage()(line 529),end()(line 807), andrecordMetrics()(line 855). While the results are deterministic, caching the computed cost would improve efficiency.💡 Cache cost in setUsage
private usage?: UsageContext; +private computedCost?: number; private mode: "full" | "passthrough" | "passthrough-cli" = "full"; // In setUsage(): const cost = calculateCost("anthropic", this.model, { input: ctx.inputTokens, output: ctx.outputTokens, total: totalTokens, cacheCreationTokens: ctx.cacheCreationTokens, cacheReadTokens: ctx.cacheReadTokens, }); +this.computedCost = cost; // In end() and recordMetrics(), use this.computedCost instead of recalculating🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/proxy/proxyTracer.ts` around lines 840 - 870, The cost is being recomputed in setUsage(), end(), and recordMetrics(); compute and cache the cost once when usage is set (inside setUsage()) by calling calculateCost(...) and store it on the tracer instance (e.g., this.cachedCost or this.computedCost), then update end() and recordMetrics() to read and reuse that cached value (falling back to recalculation only if cached value is missing), making sure the stored value can be undefined or a number and preserving the same input token fields used by calculateCost.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4241a7a4-f120-4b5e-98fa-29e0e56bfa71
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (114)
.github/workflows/ci.yml.github/workflows/copilot-review.yml.github/workflows/docs-deploy.yml.github/workflows/docs-pr-validation.yml.github/workflows/docs-version.yml.github/workflows/release.ymlCLAUDE.mdREADME.mddocs-site/package.jsondocs/advanced/auth-architecture.mddocs/advanced/mcp-integration.mddocs/assets/dashboards/neurolink-proxy-observability-dashboard.jsondocs/cli/commands.mddocs/cli/index.mddocs/cookbook/multi-provider-fallback.mddocs/cookbook/rate-limit-handling.mddocs/custom-middleware-guide.mddocs/features/claude-proxy-architecture.mddocs/features/claude-proxy-config-reference.mddocs/features/claude-proxy-observability.mddocs/features/claude-proxy-troubleshooting.mddocs/features/claude-proxy.mddocs/features/index.mddocs/features/observability.mddocs/features/provider-orchestration.mddocs/getting-started/index.mddocs/getting-started/providers/ollama.mddocs/guides/enterprise/multi-provider-failover.mddocs/guides/examples/code-patterns.mddocs/guides/frameworks/fastify.mddocs/guides/server-adapters/koa.mddocs/guides/server-adapters/middleware.mddocs/plans/2026-03-07-observability-api-wiring.mddocs/real-time-services.mddocs/reference/index.mddocs/reference/provider-selection.mddocs/sdk/nestjs-integration.mddocs/telemetry-guide.mdpackage.jsonscripts/build-browser.mjsscripts/observability/check-proxy-telemetry.mjsscripts/observability/docker-compose.proxy-observability.yamlscripts/observability/import-openobserve-dashboard.mjsscripts/observability/manage-local-openobserve.shscripts/observability/otel-collector.proxy-observability.yamlscripts/observability/proxy-observability.env.examplesrc/cli/commands/mcp.tssrc/cli/commands/proxy.tssrc/cli/commands/task.tssrc/cli/factories/commandFactory.tssrc/cli/parser.tssrc/lib/auth/anthropicOAuth.tssrc/lib/auth/providers/firebase.tssrc/lib/auth/providers/jwt.tssrc/lib/auth/providers/workos.tssrc/lib/auth/sessionManager.tssrc/lib/auth/tokenStore.tssrc/lib/client/aiSdkAdapter.tssrc/lib/client/streamingClient.tssrc/lib/core/modules/GenerationHandler.tssrc/lib/core/redisConversationMemoryManager.tssrc/lib/evaluation/BatchEvaluator.tssrc/lib/evaluation/hooks/observabilityHooks.tssrc/lib/evaluation/pipeline/evaluationPipeline.tssrc/lib/evaluation/pipeline/strategies/batchStrategy.tssrc/lib/evaluation/pipeline/strategies/samplingStrategy.tssrc/lib/neurolink.tssrc/lib/observability/otelBridge.tssrc/lib/providers/amazonBedrock.tssrc/lib/providers/anthropic.tssrc/lib/providers/anthropicBaseProvider.tssrc/lib/providers/azureOpenai.tssrc/lib/providers/googleAiStudio.tssrc/lib/providers/googleVertex.tssrc/lib/providers/huggingFace.tssrc/lib/providers/litellm.tssrc/lib/providers/mistral.tssrc/lib/providers/ollama.tssrc/lib/providers/openAI.tssrc/lib/providers/openRouter.tssrc/lib/providers/openaiCompatible.tssrc/lib/proxy/claudeFormat.tssrc/lib/proxy/cloaking/plugins/sessionIdentity.tssrc/lib/proxy/modelRouter.tssrc/lib/proxy/oauthFetch.tssrc/lib/proxy/proxyConfig.tssrc/lib/proxy/proxyEnv.tssrc/lib/proxy/proxyFetch.tssrc/lib/proxy/proxyTracer.tssrc/lib/proxy/rawStreamCapture.tssrc/lib/proxy/requestLogger.tssrc/lib/proxy/sseInterceptor.tssrc/lib/proxy/usageStats.tssrc/lib/rag/chunkers/MarkdownChunker.tssrc/lib/rag/chunking/markdownChunker.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/index.tssrc/lib/telemetry/telemetryService.tssrc/lib/types/cli.tssrc/lib/types/proxyTypes.tssrc/lib/types/streamTypes.tssrc/lib/utils/messageBuilder.tssrc/lib/utils/providerHealth.tssrc/lib/utils/providerUtils.tssrc/lib/utils/toolChoice.tstest/continuous-test-suite-workflow.tstest/continuous-test-suite.tstest/unit/quietDetector.test.tstest/unit/updateChecker.test.tstest/unit/updateState.test.ts
💤 Files with no reviewable changes (3)
- test/unit/updateChecker.test.ts
- test/unit/quietDetector.test.ts
- test/unit/updateState.test.ts
| import { createClient, type RedisClientType } from "redis"; | ||
|
|
||
| class RedisRateLimiter { | ||
| private redis: Redis; | ||
| private redis: RedisClientType; | ||
| private key: string; | ||
| private limit: number; | ||
| private window: number; // seconds | ||
|
|
||
| constructor(redis: Redis, key: string, limit: number, window: number = 60) { | ||
| constructor( | ||
| redis: RedisClientType, | ||
| key: string, | ||
| limit: number, | ||
| window: number = 60, | ||
| ) { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the Redis distributed limiter snippet
sed -n '349,397p' docs/cookbook/rate-limit-handling.md
# Detect ioredis-style calls that should not remain after RedisClientType migration
rg -n 'zremrangebyscore|zrange\(.*WITHSCORES|zadd\(' docs/cookbook/rate-limit-handling.md
# Check whether node-redis camelCase alternatives are present
rg -n 'zRemRangeByScore|zRangeWithScores|zAdd' docs/cookbook/rate-limit-handling.mdRepository: juspay/neurolink
Length of output: 1574
Node-redis migration is incomplete; ioredis method signatures remain in working example.
The code imports RedisClientType from "redis" (node-redis) but calls ioredis-style methods that don't exist on that type. Lines 374, 380, and 391 use zremrangebyscore(), zrange(..., "WITHSCORES"), and zadd() which will fail at runtime. Update to node-redis equivalents:
Required updates
- await this.redis.zremrangebyscore(this.key, 0, windowStart);
+ await this.redis.zRemRangeByScore(this.key, 0, windowStart);
- const oldestEntry = await this.redis.zrange(this.key, 0, 0, "WITHSCORES");
- const waitTime = oldestEntry[1]
- ? parseInt(oldestEntry[1]) + this.window * 1000 - now
+ const oldestEntry = await this.redis.zRangeWithScores(this.key, 0, 0);
+ const waitTime = oldestEntry[0]
+ ? oldestEntry[0].score + this.window * 1000 - now
: 1000;
- await this.redis.zadd(this.key, now, `${now}-${Math.random()}`);
+ await this.redis.zAdd(this.key, {
+ score: now,
+ value: `${now}-${Math.random()}`,
+ });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/cookbook/rate-limit-handling.md` around lines 349 - 362, The
RedisRateLimiter class is calling ioredis methods that don't exist on
node-redis; replace zremrangebyscore, zrange(..., "WITHSCORES"), and zadd with
the node-redis equivalents: use client.zRemRangeByScore(key, min, max) for
removals, use client.zRange(key, min, max, { WITHSCORES: true }) or
client.zRangeWithScores(key, start, stop) (depending on your installed
node-redis helper) to retrieve members with scores, and use client.zAdd(key, [{
score: Number(score), value: member }]) to add entries; update the calls in
RedisRateLimiter methods that currently call zremrangebyscore, zrange, and zadd
accordingly and adapt any handling of returned value shapes to the node-redis
return types.
| "@opentelemetry/api-logs": "^0.214.0", | ||
| "@opentelemetry/context-async-hooks": "^2.6.1", | ||
| "@opentelemetry/core": "^2.6.0", | ||
| "@opentelemetry/exporter-logs-otlp-http": "^0.214.0", | ||
| "@opentelemetry/exporter-metrics-otlp-http": "^0.214.0", | ||
| "@opentelemetry/exporter-trace-otlp-http": "^0.213.0", | ||
| "@opentelemetry/resources": "^2.6.0", | ||
| "@opentelemetry/sdk-logs": "^0.214.0", | ||
| "@opentelemetry/sdk-metrics": "^2.6.1", |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For OpenTelemetry JS, do experimental packages such as @opentelemetry/exporter-trace-otlp-http, @opentelemetry/exporter-logs-otlp-http, @opentelemetry/exporter-metrics-otlp-http, @opentelemetry/api-logs, and @opentelemetry/sdk-logs need to stay on the same 0.x minor version? Is 0.213.x supported with 0.214.x in one application?
💡 Result:
Yes—within OpenTelemetry JS, the “experimental / development” packages are intended to be kept on the same 0.x minor version (i.e., the same 0.213.x, or the same 0.214.x, etc.). The repo explicitly states that “all experimental packages are released with the same version” and provides a compatibility matrix for sets of versions expected to work together. [1]
So mixing 0.213.x and 0.214.x in the same application is not supported / not guaranteed to work—a 0.x minor bump can include breaking changes for in-development signals. [2]
Recommendation: align @opentelemetry/exporter-*-otlp-http, @opentelemetry/api-logs, @opentelemetry/sdk-logs, etc. to the same 0.214.x (or all to 0.213.x). [1][2]
Sources
[1] OpenTelemetry JS “Package Version Compatibility” (repo README) (gitea.cncfstack.com)
[2] OpenTelemetry “Versioning and stability” (minor bumps may include breaking changes for in-development signals) (opentelemetry.io)
Citations:
- 1: https://gitea.cncfstack.com/open-telemetry/opentelemetry-js/src/commit/7fde94081ed141c7d61db269b77d5765887a9665
- 2: https://opentelemetry.io/docs/specs/otel/versioning-and-stability/
Align OpenTelemetry experimental packages to the same 0.x minor version.
The logs/metrics additions are pinned to 0.214.x, but @opentelemetry/exporter-trace-otlp-http remains on 0.213.0. OpenTelemetry JS requires all experimental packages to share the same 0.x minor version—mixing minors like 0.213.x with 0.214.x is not supported and can cause duplicate exporter stacks and flaky telemetry initialization.
📦 Fix: update trace exporter to 0.214.x
- "@opentelemetry/exporter-trace-otlp-http": "^0.213.0",
+ "@opentelemetry/exporter-trace-otlp-http": "^0.214.0",📝 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.
| "@opentelemetry/api-logs": "^0.214.0", | |
| "@opentelemetry/context-async-hooks": "^2.6.1", | |
| "@opentelemetry/core": "^2.6.0", | |
| "@opentelemetry/exporter-logs-otlp-http": "^0.214.0", | |
| "@opentelemetry/exporter-metrics-otlp-http": "^0.214.0", | |
| "@opentelemetry/exporter-trace-otlp-http": "^0.213.0", | |
| "@opentelemetry/resources": "^2.6.0", | |
| "@opentelemetry/sdk-logs": "^0.214.0", | |
| "@opentelemetry/sdk-metrics": "^2.6.1", | |
| "@opentelemetry/api-logs": "^0.214.0", | |
| "@opentelemetry/context-async-hooks": "^2.6.1", | |
| "@opentelemetry/core": "^2.6.0", | |
| "@opentelemetry/exporter-logs-otlp-http": "^0.214.0", | |
| "@opentelemetry/exporter-metrics-otlp-http": "^0.214.0", | |
| "@opentelemetry/exporter-trace-otlp-http": "^0.214.0", | |
| "@opentelemetry/resources": "^2.6.0", | |
| "@opentelemetry/sdk-logs": "^0.214.0", | |
| "@opentelemetry/sdk-metrics": "^2.6.1", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package.json` around lines 218 - 226, The dependency list mixes OpenTelemetry
experimental minors—specifically `@opentelemetry/exporter-trace-otlp-http` is at
0.213.0 while other OTEL experimental packages are 0.214.x; update the version
of `@opentelemetry/exporter-trace-otlp-http` in package.json to ^0.214.0 to align
minors with `@opentelemetry/api-logs`, exporter-logs-otlp-http,
exporter-metrics-otlp-http, sdk-logs, etc., then reinstall (npm/yarn) and update
the lockfile so all OTEL experimental packages share the same 0.214.x minor.
| load_env() { | ||
| if [[ -f "${ENV_FILE}" ]]; then | ||
| set -a | ||
| # shellcheck source=/dev/null | ||
| source "${ENV_FILE}" | ||
| set +a | ||
| fi | ||
| } |
There was a problem hiding this comment.
Don't source a dotenv file.
source "${ENV_FILE}" executes shell, not just KEY=VALUE assignments. Any command substitution or stray shell syntax inside proxy-observability.env runs with the caller's privileges. Please parse it as dotenv data instead of evaluating it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/observability/manage-local-openobserve.sh` around lines 11 - 18, The
load_env function currently sources ${ENV_FILE} which executes arbitrary shell;
instead, read and parse the dotenv file safely: open ENV_FILE line-by-line, skip
empty lines and comments, match lines with a KEY=VALUE pattern (reference:
load_env and ENV_FILE), extract the key and raw value without using eval, strip
surrounding single or double quotes from the value, unescape any escaped
characters as needed, and export each variable with export KEY="value"; ensure
you do not call source or allow command substitution when handling
proxy-observability.env.
| cat <<EOF | ||
| Local proxy observability is ready. | ||
|
|
||
| OpenObserve UI: ${NEUROLINK_OPENOBSERVE_URL} | ||
| OpenObserve login: ${NEUROLINK_OPENOBSERVE_USER} / ${NEUROLINK_OPENOBSERVE_PASSWORD} | ||
| OTLP HTTP endpoint: http://localhost:${NEUROLINK_OTLP_HTTP_PORT} |
There was a problem hiding this comment.
Avoid printing the configured OpenObserve password.
setup echoes NEUROLINK_OPENOBSERVE_PASSWORD verbatim. That leaks non-default credentials into terminal scrollback, CI logs, and screenshots. Please mask it or print only the username plus reset instructions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/observability/manage-local-openobserve.sh` around lines 148 - 153,
The script prints NEUROLINK_OPENOBSERVE_PASSWORD in the heredoc (cat <<EOF)
which leaks secrets; update the output in manage-local-openobserve.sh (the setup
output block that references NEUROLINK_OPENOBSERVE_URL,
NEUROLINK_OPENOBSERVE_USER, NEUROLINK_OPENOBSERVE_PASSWORD and
NEUROLINK_OTLP_HTTP_PORT) to avoid revealing the password—replace the raw
password with a masked value (e.g., ******), or omit it and print the username
plus a one-line instruction on how to view or reset the password (for example
“password hidden — run ./setup --show-password or reset via ...”); ensure you
only change the heredoc content and keep variable references for URL, user, and
port intact.
| : ora("Testing MCP server connections...").start(); | ||
|
|
||
| const sdk = new NeuroLink(); | ||
| await sdk.getMCPStatus(); |
There was a problem hiding this comment.
Bound MCP initialization with timeout to avoid hanging CLI commands.
Line 750, Line 864, and Line 992 call sdk.getMCPStatus() with no timeout. If MCP init stalls, test, exec, and remove can block indefinitely.
⏱️ Proposed fix
+const MCP_INIT_TIMEOUT_MS = 10_000;
+
+async function ensureMCPInitialized(sdk: NeuroLink): Promise<void> {
+ await withTimeout(
+ sdk.getMCPStatus(),
+ MCP_INIT_TIMEOUT_MS,
+ ErrorFactory.toolTimeout("mcpInitialization", MCP_INIT_TIMEOUT_MS),
+ );
+}
...
- await sdk.getMCPStatus();
+ await ensureMCPInitialized(sdk);
...
- await sdk.getMCPStatus();
+ await ensureMCPInitialized(sdk);
...
- await sdk.getMCPStatus();
+ await ensureMCPInitialized(sdk);As per coding guidelines src/**/*.ts: "Wrap async operations with withTimeout utility" and "Use ErrorFactory for creating typed errors".
Also applies to: 864-864, 992-992
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli/commands/mcp.ts` at line 750, Wrap each direct call to
sdk.getMCPStatus() (used in the test, exec, and remove command flows) with the
withTimeout utility and handle timeout by throwing a typed error via
ErrorFactory; specifically, replace await sdk.getMCPStatus() with await
withTimeout(sdk.getMCPStatus(), <appropriateTimeoutMs>) and when withTimeout
rejects due to timeout, throw an ErrorFactory-created error (e.g.,
ErrorFactory.create('MCP_INIT_TIMEOUT', 'MCP initialization timed out')) after
importing withTimeout and ErrorFactory and using a shared timeout constant name
(e.g., MCP_INIT_TIMEOUT) so the CLI does not hang indefinitely.
| const client = this.getClient(); | ||
| await client.hSet(TASKS_HASH, task.id, JSON.stringify(task)); | ||
| this.applyRetentionTTL(task); |
There was a problem hiding this comment.
Keep retention TTL best-effort.
applyRetentionTTL() now re-enters getClient(). If the connection drops after hSet() succeeds, save()/update() will reject even though the task write is already persisted, because TTL setup throws synchronously before the .catch() handlers run.
🛠️ Suggested fix
private applyRetentionTTL(task: Task): void {
@@
const ttlMs = ttlMap[task.status];
if (ttlMs) {
- const client = this.getClient();
+ const client = this.client;
+ if (!client?.isOpen) {
+ logger.debug("[TaskStore:Redis] Skipping TTL; client unavailable", {
+ taskId: task.id,
+ });
+ return;
+ }
const ttlSeconds = Math.ceil(ttlMs / 1000);
// Set TTL on associated keys
- client.expire(taskRunsKey(task.id), ttlSeconds).catch((err) => {
+ void client.expire(taskRunsKey(task.id), ttlSeconds).catch((err) => {
logger.debug("[TaskStore:Redis] Failed to set TTL", {
error: String(err),
});
});
- client.expire(taskHistoryKey(task.id), ttlSeconds).catch((err) => {
+ void client.expire(taskHistoryKey(task.id), ttlSeconds).catch((err) => {
logger.debug("[TaskStore:Redis] Failed to set TTL", {
error: String(err),
});
});
}
}Also applies to: 115-129, 242-250
🤖 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 85 - 87, The save/update
flow currently calls applyRetentionTTL() which re-enters getClient() and can
throw after hSet() has already persisted the task, causing save()/update() to
reject incorrectly; change applyRetentionTTL usage to be best-effort by either
(1) refactoring applyRetentionTTL to accept the existing Redis client (pass the
client returned by getClient() used for hSet so it doesn't call getClient()
again) or (2) invoke applyRetentionTTL without letting its errors surface (wrap
the call in a try/catch or call it and attach .catch(...) to swallow/log errors)
so TTL failures do not reject save()/update(); update calls in save/update and
the other locations (around lines 115-129, 242-250) accordingly and ensure any
logging uses process-safe logging rather than throwing.
| const updated = await store.update(taskId, taskUpdates); | ||
|
|
||
| // Re-schedule if schedule changed and task is active | ||
| if (updates.schedule && updated.status === "active") { | ||
| await this.backend!.cancel(taskId); | ||
| await this.backend!.schedule(updated, (t) => this.onTaskTick(t)); | ||
| await backend.cancel(taskId); | ||
| await backend.schedule(updated, (t) => this.onTaskTick(t)); | ||
| } |
There was a problem hiding this comment.
Keep store state and backend scheduling in sync on failure.
These flows commit one side before the other. If the second step throws, the task can end up active-but-unscheduled, paused in BullMQ but still active in the store, or updated in the store with no matching scheduler.
🛠️ Suggested pattern
- const updated = await store.update(taskId, { status: "active" });
- await backend.schedule(updated, (t) => this.onTaskTick(t));
+ const updated = await store.update(taskId, { status: "active" });
+ try {
+ await backend.schedule(updated, (t) => this.onTaskTick(t));
+ } catch (err) {
+ await store.update(taskId, { status: "paused" });
+ throw err;
+ }Apply the same compensation idea in pause() and in the schedule-update branch by restoring task/existing when the second operation fails.
Also applies to: 340-341, 363-364
🤖 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 301 - 307, The update flow
currently commits to store via store.update(taskId, taskUpdates) then calls
backend.cancel and backend.schedule, which can leave store and scheduler out of
sync if backend throws; change to a compensation pattern: after calling
store.update (in the schedule-update branch and in pause()), call
backend.cancel/backend.schedule as now but catch any errors and on failure
immediately restore the previous task state by calling store.update with the
original task/existing object, then rethrow the error so callers see the
failure; reference the functions store.update, backend.cancel, backend.schedule
and the pause() method for where to add the try/catch + restore logic.
| case AIProviderName.LITELLM: | ||
| return this.getLiteLLMModelsUrl(); |
There was a problem hiding this comment.
The LiteLLM connectivity probe will false-fail behind auth.
/v1/models is now the generic health endpoint, but checkConnectivity() probes it without the Authorization header. On any LiteLLM deployment that requires LITELLM_API_KEY, includeConnectivityTest will report a 401 even when the proxy itself is healthy.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/utils/providerHealth.ts` around lines 603 - 604, The connectivity
probe for AIProviderName.LITELLM uses getLiteLLMModelsUrl() but calls
checkConnectivity() (used by includeConnectivityTest) without supplying the
Authorization header, causing false 401s when LITELLM_API_KEY is required;
update the LiteLLM branch of the connectivity check to include the
Authorization: Bearer <LITELLM_API_KEY> (or the configured header value) when
making the request so the /v1/models health endpoint is queried with credentials
present and returns correct health status.
| private static async checkLiteLLMConfig( | ||
| healthStatus: ProviderHealthStatusOptions, | ||
| ): Promise<void> { | ||
| const liteLLMBase = this.getLiteLLMBaseUrl(); | ||
| if (!liteLLMBase.startsWith("http")) { | ||
| healthStatus.isConfigured = false; | ||
| healthStatus.configurationIssues.push("Invalid LITELLM_BASE_URL format"); | ||
| healthStatus.recommendations.push( | ||
| "Set LITELLM_BASE_URL to a valid URL (e.g., http://localhost:4000)", | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
| const availability = await this.checkLiteLLMAvailability({ | ||
| model: this.getConfiguredLiteLLMModel(), | ||
| timeout: 2000, | ||
| }); | ||
|
|
||
| if (!availability.available) { | ||
| healthStatus.isConfigured = false; | ||
| healthStatus.configurationIssues.push( | ||
| `LiteLLM runtime check failed: ${availability.reason ?? "unknown error"}`, | ||
| ); | ||
| healthStatus.recommendations.push( | ||
| "Start the LiteLLM proxy and ensure the configured model is available from /v1/models", | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
| healthStatus.isConfigured = true; | ||
| } |
There was a problem hiding this comment.
Honor the caller timeout in these runtime probes.
Fast paths such as getBestHealthyProvider() pass a smaller timeout, but these checks hard-code 2000. Missing local LiteLLM/Ollama instances can still stall provider selection longer than the requested budget.
Also applies to: 1121-1152
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/utils/providerHealth.ts` around lines 1086 - 1116, checkLiteLLMConfig
currently hard-codes a 2000ms probe timeout which ignores caller time budgets
(e.g., getBestHealthyProvider). Change checkLiteLLMConfig to accept a timeout
parameter (e.g., timeoutMs: number) and pass that timeout into the call to
checkLiteLLMAvailability instead of 2000; update callers (like
getBestHealthyProvider) to forward their timeout, and do the same for the
sibling probe function referenced around lines 1121-1152 so all runtime probes
honor the caller-supplied timeout. Ensure parameter names (checkLiteLLMConfig,
checkLiteLLMAvailability, getConfiguredLiteLLMModel) are used to locate and
update calls.
| const providers = [ | ||
| "vertex", // Prioritize Google Cloud AI (Vertex) first | ||
| "litellm", // Prioritize self-hosted/proxy (no rate limits) | ||
| "ollama", // Local models (no rate limits) | ||
| "vertex", // Google Cloud AI (enterprise) |
There was a problem hiding this comment.
Don't front-load LiteLLM in the legacy fallback until its probe is bounded.
hasProviderEnvVars("litellm") is unconditional, so this order change makes the legacy path try LiteLLM before any configured cloud provider whenever the health checker falls back. Because that probe is a live generate() call, a dead/local-only LiteLLM can now stall provider resolution instead of failing through quickly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/utils/providerUtils.ts` around lines 111 - 114, The legacy fallback
currently unconditionally prioritizes "litellm" in the providers array which
causes hasProviderEnvVars("litellm") to be checked and a live generate() probe
to run before cloud providers; change the logic so LiteLLM is not front-loaded
in the legacy fallback (either move "litellm" later in the providers array or
gate its inclusion with a bounded probe check) to avoid stalling provider
resolution — update the providers array and the code paths that call
hasProviderEnvVars("litellm") in providerUtils.ts (referencing the providers
constant and the health-check/legacy fallback code) so cloud providers are tried
first unless LiteLLM is explicitly safe to probe.
…upport - Add triple-signal OTLP export (traces, metrics, logs) to proxy via instrumentation.ts - Add SSE stream interceptor for zero-overhead telemetry extraction from Anthropic responses - Add proxy request tracer with full span lifecycle (receive → upstream → stream → end) - Add --passthrough flag for transparent forwarding without retry/rotation/polyfill - Add --env-file flag to load provider keys from a dedicated env file - Enhance request logger with OTel traceId/spanId correlation - Make TelemetryService reuse existing global TracerProvider (avoid duplicate registration) - Improve LiteLLM and Ollama health checks with model list validation - Prioritize self-hosted providers (LiteLLM, Ollama) in fallback order - Fix lifecycle callback clobber in generate/stream (onFinish/onError/onChunk) - Fix sessionManager to use 'redis' package instead of undeclared 'ioredis' - Fix MCP commands to initialize MCP before listing servers - Update CLAUDE.md: TTS→core-path-only, Eval→integration-tests-only
f96f84d to
573d7a5
Compare
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
|
🎉 This PR is included in version 9.42.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Summary
--passthroughflag: Transparent forwarding mode — no retry, rotation, or polyfill--env-fileflag: Load provider API keys from a dedicated env fileChanged files (46)
New files
src/lib/proxy/proxyTracer.ts— OTel request tracersrc/lib/proxy/sseInterceptor.ts— SSE telemetry extractionsrc/lib/proxy/proxyEnv.ts— Env file loading for proxyCode changes
src/lib/server/routes/claudeProxyRoutes.ts— Passthrough mode, OTel integrationsrc/lib/services/server/ai/observability/instrumentation.ts— OTLP metrics + logs exportsrc/lib/telemetry/telemetryService.ts— Global TracerProvider reusesrc/lib/neurolink.ts— Fix lifecycle callback clobbersrc/lib/auth/sessionManager.ts— Fix ioredis→redissrc/cli/commands/mcp.ts— Fix MCP init ordersrc/lib/utils/providerHealth.ts— LiteLLM + Ollama health checkssrc/lib/utils/providerUtils.ts— Fallback order: self-hosted firstDocumentation (22 files)
Test plan
pnpm run check— TypeScript compilation passespnpm run lint— Zero new errorspnpm run build— Full build succeedsneurolink proxy start— Proxy starts and serves requestsneurolink proxy start --passthrough— Transparent forwarding worksneurolink proxy start --env-file ./test.env— Env file loadedOTEL_EXPORTER_OTLP_ENDPOINT=http://localhost:4318neurolink mcp testworks without errors (MCP init fix)Summary by CodeRabbit
New Features
Improvements
Documentation