fix(telemetry): close all OTel/Langfuse gaps, add full proxy error logging, increase retry limit - #962
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 |
|
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 widespread OpenTelemetry instrumentation, trace-context propagation, tool-end/generation event emission utilities, telemetry verification tests, and comprehensive telemetry documentation/plans across providers, server routes, workflow, RAG/memory, clients, auth, and core runtime. (32 words) Changes
Sequence Diagram(s)sequenceDiagram
participant Provider as Provider (e.g., OpenAI/Bedrock)
participant Neurolink as Neurolink (event emitter)
participant SpanUtil as SpanSerializer / withSpan
participant Backend as Langfuse / Metrics
Provider->>Neurolink: onStepFinish (toolResults)
Neurolink->>SpanUtil: emitToolEndFromStepFinish -> create "tool:end" span (with traceId/parentSpanId)
SpanUtil->>Backend: serialize & send span (status WARNING/ERROR/OK)
Backend-->>SpanUtil: ack
SpanUtil-->>Neurolink: span recorded / instrumentation complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
Documentation Validation Results
🚧 Please fix the failing checks before merging. Commit: |
There was a problem hiding this comment.
Pull request overview
This PR substantially expands and aligns observability across NeuroLink by closing telemetry gaps between OTel/Langfuse (Pipeline A) and the internal MetricsAggregator/Langfuse API flow (Pipeline B), while also improving Claude proxy diagnostics (full upstream error capture) and increasing 429 retry limits.
Changes:
- Add widespread
withSpan/tracer instrumentation across workflow, HTTP routes, RAG, memory, MCP, auth, processors, evaluation, clients, and CLI turns. - Link Pipeline B spans to the active OTel context via
getActiveTraceContext()and enrich Langfuse attributes/levels (ERROR/WARNING, usage details, model params). - Enhance Claude proxy error diagnostics (headers/body capture for non-200s incl. 429/auth-retry) and increase per-account 429 retries from 5 → 10.
Reviewed changes
Copilot reviewed 69 out of 69 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/lib/workflow/core/responseConditioner.ts | Wrap response conditioning with workflow tracer spans and conditioning attributes. |
| src/lib/workflow/core/judgeScorer.ts | Add workflow spans and judge timing attributes. |
| src/lib/workflow/core/ensembleExecutor.ts | Add workflow spans + success/failure/time attributes for ensemble execution. |
| src/lib/utils/toolEndEmitter.ts | New helper to emit tool:end from AI-SDK onStepFinish tool results. |
| src/lib/types/span.ts | Add WARNING to SpanStatus. |
| src/lib/telemetry/tracers.ts | Add auth and workflow tracers. |
| src/lib/telemetry/traceContext.ts | New helper to read active OTel trace/parent span IDs. |
| src/lib/services/server/ai/observability/instrumentation.ts | Enrich spans for Langfuse (usage_details, model params, WARNING/ERROR level/status message). |
| src/lib/server/routes/toolRoutes.ts | Add HTTP spans for tool list/search/execute endpoints. |
| src/lib/server/routes/openApiRoutes.ts | Add HTTP spans for OpenAPI JSON/YAML/docs endpoints. |
| src/lib/server/routes/memoryRoutes.ts | Add helper wrapper for HTTP spans around memory routes. |
| src/lib/server/routes/mcpRoutes.ts | Add helper wrapper for HTTP spans around MCP routes. |
| src/lib/server/routes/healthRoutes.ts | Add helper wrapper + health attributes for HTTP spans around health/version endpoints. |
| src/lib/server/routes/claudeProxyRoutes.ts | Increase 429 retry limit; add full upstream error capture for 429/auth-retry paths; add HTTP spans for proxy utility endpoints. |
| src/lib/server/routes/agentRoutes.ts | Add HTTP spans around agent execute/stream/embed endpoints. |
| src/lib/server/middleware/common.ts | Add OTel span + ERROR status propagation for timing middleware. |
| src/lib/rag/retrieval/vectorQueryTool.ts | Add RAG spans and attributes for vector query tool execution. |
| src/lib/rag/reranker/reranker.ts | Add RAG spans around reranking and batch reranking; emit output counts. |
| src/lib/rag/ragIntegration.ts | Add RAG search span wrapping in-memory RAG tool execute. |
| src/lib/rag/chunkers/BaseChunker.ts | Add RAG chunking span with strategy/content length/chunk count attributes. |
| src/lib/providers/openaiCompatible.ts | Emit tool:end events from AI-SDK onStepFinish. |
| src/lib/providers/openRouter.ts | Emit tool:end events from AI-SDK onStepFinish. |
| src/lib/providers/openAI.ts | Emit tool:end events from onStepFinish; set OTel ERROR status/exception on stream failures. |
| src/lib/providers/ollama.ts | Include usage in streaming; aggregate usage; emit generation:end; emit tool:end for tool results. |
| src/lib/providers/mistral.ts | Emit tool:end events from AI-SDK onStepFinish. |
| src/lib/providers/litellm.ts | Emit tool:end events from AI-SDK onStepFinish. |
| src/lib/providers/huggingFace.ts | Emit tool:end events from AI-SDK onStepFinish. |
| src/lib/providers/googleVertex.ts | Emit tool:end; emit generation:end for native paths that bypass AI SDK telemetry. |
| src/lib/providers/googleAiStudio.ts | Emit tool:end; emit generation:end for native paths; set OTel ERROR status/exception on stream failures. |
| src/lib/providers/azureOpenai.ts | Emit tool:end events from AI-SDK onStepFinish. |
| src/lib/providers/anthropic.ts | Emit tool:end; set OTel ERROR status/exception on stream failures. |
| src/lib/providers/amazonSagemaker.ts | Add span around stream() and mark not implemented via attributes. |
| src/lib/providers/amazonBedrock.ts | Emit generation:end (generate + stream); aggregate usage; map AWS throttling to RateLimitError; emit tool:end for tool results. |
| src/lib/processors/registry/ProcessorRegistry.ts | Add processor span for processFile dispatch. |
| src/lib/processors/base/BaseFileProcessor.ts | Add file processing span with file metadata attributes. |
| src/lib/observability/utils/spanSerializer.ts | Inherit active OTel trace context for Pipeline B spans; map WARNING to Langfuse level. |
| src/lib/neurolink.ts | Multiple telemetry fixes: reuse active OTel context, avoid duplicate Pipeline B generation spans when Pipeline A handled, WARNING on finish reasons, stream events/errors, abort classification, tool retry count, MCP error extraction. |
| src/lib/memory/memoryRetrievalTools.ts | Add memory retrieval span; improve span status handling for not found; record counts/sizes. |
| src/lib/mcp/toolRegistry.ts | Rename span name to avoid duplication; explicitly set OTel ERROR when returning an error result (no throw). |
| src/lib/mcp/toolDiscoveryService.ts | Add span around tool discovery and record discovered count. |
| src/lib/mcp/mcpClientFactory.ts | Link MCP transport spans to active OTel trace context. |
| src/lib/mcp/httpRetryHandler.ts | Link MCP retry spans to active OTel trace context. |
| src/lib/mcp/httpRateLimiter.ts | Link MCP rate limiter spans to active OTel trace context. |
| src/lib/mcp/batching/requestBatcher.ts | Add MCP batch execution span with batch metrics. |
| src/lib/evaluation/ragasEvaluator.ts | Add evaluation span + expose scores on OTel span. |
| src/lib/core/modules/ToolsManager.ts | Set OTel span status ERROR when tool result indicates isError:true. |
| src/lib/core/modules/StreamHandler.ts | Emit sentinel chunk on NoOutputGeneratedError to enable WARNING classification downstream. |
| src/lib/core/modules/GenerationHandler.ts | Emit tool:end for AI-SDK-driven tool calls via shared helper. |
| src/lib/context/summarizationEngine.ts | Add memory summarization span + summarized/not attributes. |
| src/lib/context/contextCompactor.ts | Add context compaction span; link Pipeline B span to active OTel; mark WARNING when compaction insufficient. |
| src/lib/context/budgetChecker.ts | Link budget check span to active OTel trace context. |
| src/lib/client/wsClient.ts | Track WebSocket connection lifecycle with an OTel span, ending on close/error/disconnect. |
| src/lib/client/streamingClient.ts | Add client spans around SSE/WS connect calls. |
| src/lib/client/sseClient.ts | Add client span around SSE stream initiation. |
| src/lib/client/httpClient.ts | Add client span around HTTP requests. |
| src/lib/auth/tokenStore.ts | Add auth spans around token save/load/clear/get-valid and include useful attributes. |
| src/lib/auth/sessionManager.ts | Add auth spans around session create/get/refresh/validate with masked IDs. |
| src/lib/auth/anthropicOAuth.ts | Add auth spans around OAuth exchange/refresh/revoke operations. |
| src/cli/loop/session.ts | Add per-turn CLI span for tracing interactive loop commands. |
| docs/telemetry-fix-diff.txt | Record before/after verification output diff. |
| docs/telemetry-fix-completion-report.md | Summary report of resolved gaps and remaining known gap. |
| docs/telemetry-baseline-proof.txt | Baseline proof output for gap verification suite. |
| docs/telemetry-after-proof.txt | Post-fix proof output for gap verification suite. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return withSpan( | ||
| { | ||
| name: "neurolink.http.stream", | ||
| tracer: tracers.http, | ||
| attributes: { | ||
| "http.route": "/api/agent/stream", | ||
| "ai.provider": request.provider || "default", | ||
| "ai.model": request.model || "default", | ||
| }, | ||
| }, | ||
| }); | ||
| async () => { | ||
| // Normalize input | ||
| const input = | ||
| typeof request.input === "string" | ||
| ? { text: request.input } | ||
| : request.input; | ||
|
|
||
| // Create redactor (no-op if redaction is not enabled) | ||
| const redactor = createStreamRedactor(ctx.redaction); | ||
| const result = await ctx.neurolink.stream({ | ||
| input, | ||
| provider: request.provider, | ||
| model: request.model, | ||
| systemPrompt: request.systemPrompt, | ||
| temperature: request.temperature, | ||
| maxTokens: request.maxTokens, | ||
| context: { | ||
| // When an authenticated user context exists (set by auth middleware), | ||
| // always use its IDs to prevent caller-supplied impersonation. | ||
| sessionId: ctx.user | ||
| ? ctx.session?.id | ||
| : (ctx.session?.id ?? request.sessionId), | ||
| userId: ctx.user ? ctx.user.id : request.userId, | ||
| userEmail: ctx.user?.email, | ||
| userRoles: ctx.user?.roles, | ||
| requestId: ctx.requestId, | ||
| }, | ||
| }); | ||
|
|
||
| // Wrap stream to apply redaction to each chunk | ||
| async function* redactedStream(): AsyncIterable<unknown> { | ||
| for await (const chunk of result.stream) { | ||
| // Apply redaction to chunk (returns unchanged if redaction disabled) | ||
| yield redactor(chunk); | ||
| } | ||
| } | ||
| // Create redactor (no-op if redaction is not enabled) | ||
| const redactor = createStreamRedactor(ctx.redaction); | ||
|
|
||
| return redactedStream(); | ||
| // Wrap stream to apply redaction to each chunk | ||
| async function* redactedStream(): AsyncIterable<unknown> { | ||
| for await (const chunk of result.stream) { | ||
| // Apply redaction to chunk (returns unchanged if redaction disabled) | ||
| yield redactor(chunk); | ||
| } | ||
| } | ||
|
|
||
| return redactedStream(); | ||
| }, | ||
| ); // end withSpan |
There was a problem hiding this comment.
This streaming handler returns an AsyncIterable from inside withSpan. Since withSpan ends the span as soon as the callback resolves, the span will end immediately after the generator is created (before any chunks are sent, and before stream errors can be observed). To accurately trace streaming duration/outcome, wrap the returned async iterator so the span is ended in a finally after iteration completes (or use a streaming-aware span helper).
| try { | ||
| // Prevent concurrent discovery for same server | ||
| if (this.discoveryInProgress.has(serverId)) { | ||
| return { | ||
| success: false, | ||
| error: `Discovery already in progress for server: ${serverId}`, | ||
| toolCount: 0, | ||
| tools: [], | ||
| duration: Date.now() - startTime, | ||
| serverId, | ||
| }; | ||
| } | ||
|
|
||
| this.discoveryInProgress.add(serverId); | ||
|
|
||
| mcpLogger.info( | ||
| `[ToolDiscoveryService] Starting tool discovery for server: ${serverId}`, | ||
| ); | ||
|
|
||
| // Create circuit breaker for tool discovery | ||
| const circuitBreaker = globalCircuitBreakerManager.getBreaker( | ||
| `tool-discovery-${serverId}`, | ||
| { | ||
| failureThreshold: 2, | ||
| resetTimeout: 60000, | ||
| operationTimeout: timeout, | ||
| }, | ||
| ); | ||
| // Create circuit breaker for tool discovery | ||
| const circuitBreaker = globalCircuitBreakerManager.getBreaker( | ||
| `tool-discovery-${serverId}`, | ||
| { | ||
| failureThreshold: 2, | ||
| resetTimeout: 60000, | ||
| operationTimeout: timeout, | ||
| }, | ||
| ); | ||
|
|
||
| // Discover tools with circuit breaker protection | ||
| const tools = await circuitBreaker.execute(async () => { | ||
| return await this.performToolDiscovery(serverId, client, timeout); | ||
| }); | ||
| // Discover tools with circuit breaker protection | ||
| const tools = await circuitBreaker.execute(async () => { | ||
| return await this.performToolDiscovery(serverId, client, timeout); | ||
| }); | ||
|
|
||
| // Register discovered tools | ||
| const registeredTools = await this.registerDiscoveredTools( | ||
| serverId, | ||
| tools, | ||
| ); | ||
| // Register discovered tools | ||
| const registeredTools = await this.registerDiscoveredTools( | ||
| serverId, | ||
| tools, | ||
| ); | ||
|
|
||
| const result: ToolDiscoveryResult = { | ||
| success: true, | ||
| toolCount: registeredTools.length, | ||
| tools: registeredTools, | ||
| duration: Date.now() - startTime, | ||
| serverId, | ||
| }; | ||
| span.setAttribute("mcp.tools_discovered", registeredTools.length); | ||
|
|
||
| // Emit discovery completed event | ||
| this.emit("discoveryCompleted", { | ||
| serverId, | ||
| toolCount: registeredTools.length, | ||
| duration: result.duration, | ||
| timestamp: new Date(), | ||
| } satisfies ToolRegistryEvents["discoveryCompleted"]); | ||
| const result: ToolDiscoveryResult = { | ||
| success: true, | ||
| toolCount: registeredTools.length, | ||
| tools: registeredTools, | ||
| duration: Date.now() - startTime, | ||
| serverId, | ||
| }; | ||
|
|
||
| mcpLogger.info( | ||
| `[ToolDiscoveryService] Discovery completed for ${serverId}: ${registeredTools.length} tools`, | ||
| ); | ||
| // Emit discovery completed event | ||
| this.emit("discoveryCompleted", { | ||
| serverId, | ||
| toolCount: registeredTools.length, | ||
| duration: result.duration, | ||
| timestamp: new Date(), | ||
| } satisfies ToolRegistryEvents["discoveryCompleted"]); | ||
|
|
||
| return result; | ||
| } catch (error) { | ||
| const errorMessage = | ||
| error instanceof Error ? error.message : String(error); | ||
| mcpLogger.info( | ||
| `[ToolDiscoveryService] Discovery completed for ${serverId}: ${registeredTools.length} tools`, | ||
| ); | ||
|
|
||
| mcpLogger.error( | ||
| `[ToolDiscoveryService] Discovery failed for ${serverId}:`, | ||
| error, | ||
| ); | ||
| return result; | ||
| } catch (error) { | ||
| const errorMessage = | ||
| error instanceof Error ? error.message : String(error); | ||
|
|
||
| // Emit discovery failed event | ||
| this.emit("discoveryFailed", { | ||
| serverId, | ||
| error: errorMessage, | ||
| timestamp: new Date(), | ||
| } satisfies ToolRegistryEvents["discoveryFailed"]); | ||
| mcpLogger.error( | ||
| `[ToolDiscoveryService] Discovery failed for ${serverId}:`, | ||
| error, | ||
| ); | ||
|
|
||
| return { | ||
| success: false, | ||
| error: errorMessage, | ||
| toolCount: 0, | ||
| tools: [], | ||
| duration: Date.now() - startTime, | ||
| serverId, | ||
| }; | ||
| } finally { | ||
| this.discoveryInProgress.delete(serverId); | ||
| } | ||
| // Emit discovery failed event | ||
| this.emit("discoveryFailed", { | ||
| serverId, | ||
| error: errorMessage, | ||
| timestamp: new Date(), | ||
| } satisfies ToolRegistryEvents["discoveryFailed"]); | ||
|
|
||
| return { | ||
| success: false, | ||
| error: errorMessage, | ||
| toolCount: 0, | ||
| tools: [], | ||
| duration: Date.now() - startTime, | ||
| serverId, | ||
| }; |
There was a problem hiding this comment.
discoverTools() catches errors and returns {success:false,...} (and also returns early when discovery is already in progress). Because withSpan marks the span OK unless an exception is thrown, these failure cases will be recorded as OK spans. Please set the span status to ERROR (and record the exception / error message) before returning success:false so MCP discovery failures are visible in traces.
| try { | ||
| // Get tool from registry | ||
| const tools = await ctx.toolRegistry.listTools(); | ||
| const tool = tools.find((t) => t.name === request.name); | ||
|
|
||
| // Execute the tool | ||
| const result = await ctx.toolRegistry.executeTool( | ||
| request.name, | ||
| request.arguments, | ||
| ); | ||
| if (!tool) { | ||
| span.setAttribute("tool.found", false); | ||
| return { | ||
| success: false, | ||
| error: `Tool '${request.name}' not found`, | ||
| duration: Date.now() - startTime, | ||
| }; | ||
| } | ||
|
|
||
| return { | ||
| success: true, | ||
| data: result, | ||
| duration: Date.now() - startTime, | ||
| metadata: { | ||
| toolName: request.name, | ||
| sessionId: request.sessionId || null, | ||
| }, | ||
| }; | ||
| } catch (error) { | ||
| const errorMessage = | ||
| error instanceof Error ? error.message : String(error); | ||
| return { | ||
| success: false, | ||
| error: errorMessage, | ||
| duration: Date.now() - startTime, | ||
| }; | ||
| } | ||
| // Execute the tool | ||
| const result = await ctx.toolRegistry.executeTool( | ||
| request.name, | ||
| request.arguments, | ||
| ); | ||
|
|
||
| span.setAttribute("tool.success", true); | ||
| return { | ||
| success: true, | ||
| data: result, | ||
| duration: Date.now() - startTime, | ||
| metadata: { | ||
| toolName: request.name, | ||
| sessionId: request.sessionId || null, | ||
| }, | ||
| }; | ||
| } catch (error) { | ||
| const errorMessage = | ||
| error instanceof Error ? error.message : String(error); | ||
| span.setAttribute("tool.success", false); | ||
| return { | ||
| success: false, | ||
| error: errorMessage, | ||
| duration: Date.now() - startTime, | ||
| }; | ||
| } |
There was a problem hiding this comment.
withSpan always sets the span status to OK unless the callback throws. In this handler you return error payloads (e.g., tool not found / execution failure) instead of throwing, so the OTel span will still be marked OK and won’t reflect the failure. Consider setting span status to ERROR (and recording the exception) before returning success:false, including the tool-not-found branch.
| try { | ||
| // Get tool from registry | ||
| const tools = await ctx.toolRegistry.listTools(); | ||
| const tool = tools.find((t) => t.name === name); | ||
|
|
||
| // Execute the tool | ||
| const result = await ctx.toolRegistry.executeTool(name, args); | ||
| if (!tool) { | ||
| span.setAttribute("tool.found", false); | ||
| return { | ||
| success: false, | ||
| error: `Tool '${name}' not found`, | ||
| duration: Date.now() - startTime, | ||
| }; | ||
| } | ||
|
|
||
| return { | ||
| success: true, | ||
| data: result, | ||
| duration: Date.now() - startTime, | ||
| metadata: { | ||
| toolName: name, | ||
| }, | ||
| }; | ||
| } catch (error) { | ||
| const errorMessage = | ||
| error instanceof Error ? error.message : String(error); | ||
| return { | ||
| success: false, | ||
| error: errorMessage, | ||
| duration: Date.now() - startTime, | ||
| }; | ||
| } | ||
| // Execute the tool | ||
| const result = await ctx.toolRegistry.executeTool(name, args); | ||
|
|
||
| span.setAttribute("tool.success", true); | ||
| return { | ||
| success: true, | ||
| data: result, | ||
| duration: Date.now() - startTime, | ||
| metadata: { | ||
| toolName: name, | ||
| }, | ||
| }; | ||
| } catch (error) { | ||
| const errorMessage = | ||
| error instanceof Error ? error.message : String(error); | ||
| span.setAttribute("tool.success", false); | ||
| return { | ||
| success: false, | ||
| error: errorMessage, | ||
| duration: Date.now() - startTime, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Same issue as the /tools/execute route: errors are converted into {success:false} return values, so withSpan will still set the span status to OK. Please set span status to ERROR (and optionally recordException) before returning error responses like tool-not-found and execution errors so traces accurately reflect failures.
| return withSpan( | ||
| { | ||
| name: "neurolink.http.execute", | ||
| tracer: tracers.http, | ||
| attributes: { | ||
| "http.route": "/api/agent/execute", | ||
| "ai.provider": request.provider || "default", | ||
| "ai.model": request.model || "default", | ||
| }, |
There was a problem hiding this comment.
The http.route attribute is hard-coded to /api/..., but this router supports a configurable basePath. If createAgentRoutes() is used with a non-default basePath, traces will be attributed to the wrong route. Use the ${basePath}/agent/... path string (or the same path value used in the route definition) for http.route here (and in the other agent handlers in this file).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
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/auth/tokenStore.ts (1)
420-431:⚠️ Potential issue | 🟡 Minor
auth.refreshedattribute not set on the no-refresher fallback path.When no refresher/refresh token is available, the method returns at line 430 without setting
auth.refreshed. Downstream queries filtering onauth.refreshed=falseto count "served expired-but-still-valid-under-strict-check" tokens will miss this path. Considerspan.setAttribute("auth.refreshed", false)before the return to keep the attribute present on everyget_validspan outcome.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/tokenStore.ts` around lines 420 - 431, In the Phase 2 refresher branch inside get_valid (where tokenRefreshers.get(provider) and snapshot.refreshToken are checked), add span.setAttribute("auth.refreshed", false) before the early return so the no-refresher fallback path records auth.refreshed; keep the existing logger and then call span.setAttribute("auth.refreshed", false) immediately prior to returning the result of isTokenExpired(snapshot, 0) ? null : snapshot.accessToken.src/lib/workflow/core/ensembleExecutor.ts (1)
140-158:⚠️ Potential issue | 🟡 MinorOuter OTel span is not marked ERROR when the ensemble fails without throwing.
When
successCount === 0, the innerSpanSerializerspan is ended withSpanStatus.ERROR, but thewithSpan-wrapped OTel span (the one emitted to the OTel pipeline) returns normally and will recordOK/UNSETstatus — so Langfuse/OTel sees a "successful" ensemble span while the internal metrics aggregator sees a failure. Consider callingotelSpan.setStatus({ code: SpanStatusCode.ERROR, message: "No successful model responses" })on the zero-success path to keep the two pipelines consistent.The same applies to
executeModelGroupsInnerat lines 499–501 whentotalSuccessCount === 0.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/workflow/core/ensembleExecutor.ts` around lines 140 - 158, The outer OpenTelemetry span (otelSpan) isn't marked ERROR when all model responses fail, causing a mismatch between the internal SpanSerializer status and OTel; update ensembleExecutor.ts to set the OTel span status when successCount === 0 by calling otelSpan.setStatus({ code: SpanStatusCode.ERROR, message: "No successful model responses" }) right after computing spanStatus (and before returning), and apply the same change in executeModelGroupsInner where totalSuccessCount === 0 (use the same otelSpan.setStatus call with a similar message) so the OTel span and the internal metrics aggregator remain consistent; reference otelSpan, SpanStatusCode, successCount, and totalSuccessCount to locate the fixes.src/lib/client/wsClient.ts (2)
355-370:⚠️ Potential issue | 🟡 Minor
onerrorwithoutonclosecan leak the span.The comment says "onclose will fire next and end the span," which is true for transport-level errors in browser/
ws. However, there are paths whereonerrorfires but the socket is discarded beforeoncloseruns (e.g., the process/tab exits, or the socket is GC'd after the caller drops the reference). In those cases the span stays open forever.A safer pattern is to record the error here, set ERROR status, and leave a timer/flag so that if
onclosedoesn't arrive within a small window the span is force-ended. Alternatively, just end the span here and letoncloseno-op ifconnectionSpan === null. The latter is simpler and reliable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/client/wsClient.ts` around lines 355 - 370, The onerror handler can leak connectionSpan if onclose never fires; modify the onerror block in wsClient.ts (the this.ws.onerror handler) to not only recordException and setStatus on connectionSpan but also call connectionSpan.end() and then clear connectionSpan (set it to null) so the span is finalized immediately; also ensure the this.ws.onclose handler (the onclose logic) is already guarded to no-op when connectionSpan is null (or add that guard) so calling end() here won’t double-end the span.
129-192:⚠️ Potential issue | 🟠 MajorSpan leak on exceptions and re-entrant
connect().Two leak paths were introduced here:
- The span is started at line 143 before
new URL(this.config.baseUrl)(line 157) and theWebSocketconstructor (lines 182–187). If either throws (invalid URL,wspackage not installed, browser CSP blocking WS),connectionSpanis populated but never ended — the exception propagates out ofconnect()without anyspan.end().- The early return at lines 130–132 only guards the fully
OPENcase. Ifconnect()is called while a previous attempt is still inconnecting,error, orclosingstate, this code overwritesthis.connectionSpanwith a fresh one, orphaning the previous span (it'll only be ended if the previous ws eventually reachesonclose/onerror, which is not guaranteed if the app is throwing away the client).Wrap the constructor calls in try/catch and also end any pre-existing span before starting a new one:
🔒 Suggested fix
- this.connectionSpan = tracers.http.startSpan( - "neurolink.client.ws.connect", - { - kind: SpanKind.CLIENT, - attributes: { - "http.url": this.config.baseUrl, - "ws.auto_reconnect": this.config.autoReconnect, - "ws.reconnect_attempt": this.reconnectAttempts, - }, - }, - ); + // End any stale span from a prior failed/interrupted connect attempt. + if (this.connectionSpan) { + this.connectionSpan.setStatus({ + code: SpanStatusCode.ERROR, + message: "Superseded by new connect() call", + }); + this.connectionSpan.end(); + this.connectionSpan = null; + } + this.connectionSpan = tracers.http.startSpan( + "neurolink.client.ws.connect", + { + kind: SpanKind.CLIENT, + attributes: { + "http.url": this.config.baseUrl, + "ws.auto_reconnect": this.config.autoReconnect, + "ws.reconnect_attempt": this.reconnectAttempts, + }, + }, + ); + + try { + // ...URL + WebSocket construction... + } catch (error) { + const err = error instanceof Error ? error : new Error(String(error)); + this.connectionSpan.recordException(err); + this.connectionSpan.setStatus({ + code: SpanStatusCode.ERROR, + message: err.message, + }); + this.connectionSpan.end(); + this.connectionSpan = null; + throw error; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/client/wsClient.ts` around lines 129 - 192, connect() can leak connectionSpan on exceptions and when connect() is re-entered; before creating a new span ensure any existing this.connectionSpan is ended (call this.connectionSpan.end()) and avoid stomping an in-progress connect by returning early if state is "connecting" (or other non-OPEN transient states), then wrap the URL construction and WebSocket instantiation (the new URL(...) and new WebSocket(...) / WebSocket as ws constructor block that assigns this.ws) in a try/catch so that if either throws you call this.connectionSpan.end() (and clear this.connectionSpan) before rethrowing the error; ensure setupEventListeners() and this.pendingAuth are only set after successful socket creation so the span lifecycle is correctly terminated in onclose/onerror handlers.src/lib/neurolink.ts (2)
3899-3912:⚠️ Potential issue | 🟠 MajorOnly set
pipelineAHandledwhen Pipeline A actually emitted a generation observation.
finalizeGenerateRequestResult()runs for every successfulgenerate()path, so this unconditionally disables Pipeline B ininitializeMetricsListeners(). Native-provider executions that do not create Pipeline A generation spans will now lose their only generation observation entirely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/neurolink.ts` around lines 3899 - 3912, finalizeGenerateRequestResult() is unconditionally setting pipelineAHandled which prevents Pipeline B spans even when Pipeline A didn't emit one; change it to set pipelineAHandled only when there is a definite signal that Pipeline A emitted a generation observation (e.g., check a marker on the result like textResult.pipelineAGenerationEmitted or a metadata flag added by the Pipeline A emission code), and ensure Pipeline A's emission path sets that marker when it emits; update initializeMetricsListeners() to rely on that marker instead of assuming true and only skip creating the Pipeline B span when the marker is present.
9004-9035:⚠️ Potential issue | 🟡 MinorFailed tool spans still miss
tool.retry_count.The attribute is only set after
withRetry()returns successfully. If the last retry still fails and control goes tocatch, the span is ended without the retry count, which hides the retry history on the failure cases you most need to inspect.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/neurolink.ts` around lines 9004 - 9035, The tool retry count is only recorded after withRetry() succeeds, so failed executions never get tool.retry_count; update the control flow around prepared.circuitBreaker.execute / withRetry so toolSpan.setAttribute("tool.retry_count", toolRetryCount) is always called even on failure — e.g., ensure the attribute is set in a finally block that wraps the execute() call (or set it inside the onRetry handler and also in the catch before rethrow) so that toolRetryCount (tracked by the onRetry callback) is written to the span regardless of success or error.
🟡 Minor comments (10)
src/cli/loop/session.ts-171-199 (1)
171-199:⚠️ Potential issue | 🟡 MinorSet ERROR status alongside
recordExceptionfor per-turn spans.
recordExceptionadds an exception event but does not change the span's status; the span will still be exported with the default UNSET/OK status, which is inconsistent with the explicitsetStatus({ code: ERROR })pattern used throughout this PR (e.g.,toolRegistry.tsT3 fix,StreamHandler.validateStreamOptions). This undercuts the Pipeline A Langfuse level/status enrichment the PR claims to close.Also consider using
withSpanfor consistency — it handles status/exception/end uniformly and matches the rest of the PR.🛠️ Minimal fix
} catch (e) { - turnSpan.recordException( - e instanceof Error ? e : new Error(String(e)), - ); + const err = e instanceof Error ? e : new Error(String(e)); + turnSpan.recordException(err); + turnSpan.setStatus({ + code: SpanStatusCode.ERROR, + message: err.message, + }); throw e; } finally { turnSpan.end(); }Add import:
-import { tracers } from "../../lib/telemetry/tracers.js"; +import { tracers } from "../../lib/telemetry/tracers.js"; +import { SpanStatusCode } from "@opentelemetry/api";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/loop/session.ts` around lines 171 - 199, The span for each CLI turn currently only calls turnSpan.recordException but doesn't mark the span status as ERROR; update the error handling inside tracers.sdk.startActiveSpan (the async callback that receives turnSpan) to call turnSpan.setStatus({ code: tracers.sdk.SpanStatusCode.ERROR }) (or the equivalent enum/value exported by tracers.sdk) whenever an exception is recorded, or replace the startActiveSpan usage with tracers.sdk.withSpan/withActiveSpan helper so status, exception and end are handled uniformly; ensure you reference the existing turnSpan.recordException call and add the corresponding turnSpan.setStatus call in the catch block to match the pattern used in toolRegistry.ts and StreamHandler.validateStreamOptions.src/lib/utils/toolEndEmitter.ts-54-80 (1)
54-80:⚠️ Potential issue | 🟡 MinorEdge case: array-typed
outputwill pass the object check.
typeof output === "object"is true for arrays, so if an SDK version returnsoutputas a raw content array (rather than{ content: [...] }), the"isError" in outputcheck runs against array indices/properties and the subsequentoutput.contentaccess yieldsundefined, falling through to the generic"Tool returned isError: true"message. Minor, but consider an explicit!Array.isArray(output)guard if you want the array-as-output shape to be handled (extract text parts directly from it).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/utils/toolEndEmitter.ts` around lines 54 - 80, The object-type checks treat arrays as objects, causing array-typed output to be misinterpreted; update the isError detection and handling in the loop over toolResults (variables tr, output, isError) to explicitly exclude arrays (e.g., require !Array.isArray(output) when checking "isError" in output) and add handling for when output itself is an array (extract text parts from the array entries similar to the content-array branch) so that array-shaped outputs produce useful error messages instead of falling back to "Tool returned isError: true".src/lib/client/httpClient.ts-256-268 (1)
256-268:⚠️ Potential issue | 🟡 MinorSpan omits response status and error details — reduces observability value.
The span records
http.method/http.route/http.urlon entry but never recordshttp.response.status_code, retry counts, or exceptions (HttpError, ClientTimeoutError, ClientNetworkError). For a telemetry-focused PR, this is a notable gap: failed HTTP calls (4xx/5xx/timeouts) will appear as successful spans. Consider capturing the outcome in the callback:♻️ Suggested enrichment
- async () => this._doRequest<T>(method, path, body, options), + async (span) => { + try { + const res = await this._doRequest<T>(method, path, body, options); + span.setAttribute("http.response.status_code", res.status); + if (context && typeof context === "object") { + // optionally attach retryCount from context if exposed + } + return res; + } catch (err) { + if (err instanceof HttpError) { + span.setAttribute("http.response.status_code", err.status); + } + if (err instanceof Error) { + span.recordException(err); + } + span.setStatus({ + code: SpanStatusCode.ERROR, + message: err instanceof Error ? err.message : String(err), + }); + throw err; + } + },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/client/httpClient.ts` around lines 256 - 268, The span created in withClientSpan (name "neurolink.client.http.request", tracer tracers.http) currently only records request attributes; update the callback around this._doRequest<T>(...) to capture and set additional span attributes and status: after the request completes, add "http.response.status_code" and retry count (if available) to the span and mark span status based on status code; on thrown errors (HttpError, ClientTimeoutError, ClientNetworkError or generic exceptions) record the exception details and error attributes on the span and set an error status; ensure you use the same span object returned by withClientSpan to call setAttribute/setStatus/recordException so failures/timeouts are reflected in telemetry for _doRequest and related retry logic.docs/telemetry-after-proof.txt-98-103 (1)
98-103:⚠️ Potential issue | 🟡 MinorThe typecheck evidence is internally inconsistent.
This section includes an
ERR_MODULE_NOT_FOUNDstack trace and then immediately reports0 ERRORS. Please regenerate or annotate the artifact so the actual typecheck outcome is unambiguous.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/telemetry-after-proof.txt` around lines 98 - 103, The TYPECHECK section is inconsistent: it shows an ERR_MODULE_NOT_FOUND stack trace (the node internal ModuleJob syncLink error) but the summary line reports "0 ERRORS"; regenerate the artifact or edit this section so the actual typecheck result is unambiguous by either (a) re-running the typecheck and replacing the block so the stack trace and the final summary match, or (b) annotating the section to explicitly state that the stack trace is a transient module resolution warning and that the final "COMPLETED ... 0 ERRORS" is the authoritative result; ensure you address the stack trace containing 'ERR_MODULE_NOT_FOUND' and the summary line that currently reports zero errors so readers can see a consistent outcome.docs/telemetry-gaps-audit.md-460-462 (1)
460-462:⚠️ Potential issue | 🟡 MinorThe verification-suite path is stale.
This points to
test/telemetry-gaps-verification.test.ts, but the PR addstest/telemetry-gaps-verification.ts. The current reference sends readers to the wrong file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/telemetry-gaps-audit.md` around lines 460 - 462, Update the "8. Verification Plan" reference in docs/telemetry-gaps-audit.md so it points to the new test file name `test/telemetry-gaps-verification.ts` (not the stale `test/telemetry-gaps-verification.test.ts`); edit the sentence that currently mentions `test/telemetry-gaps-verification.test.ts` to use the correct backticked path `test/telemetry-gaps-verification.ts` so readers are directed to the actual verification suite added in the PR.src/lib/rag/chunkers/BaseChunker.ts-101-107 (1)
101-107:⚠️ Potential issue | 🟡 MinorEmpty-content validation is recorded as a span exception.
Because the
content.trim().length === 0check lives insidewithSpan(...), throwingChunkingErrorhere will causewithSpanto callspan.recordException(...)andsetStatus({ code: ERROR }). Empty input is an input-validation failure, not a chunker runtime error — marking the span ERROR will pollute error metrics/alerts in Langfuse with caller-side mistakes.Consider validating before entering the span (or setting a distinct attribute and still returning
[]/OKfor empty input), mirroring how other "invalid input" paths are typically handled.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/rag/chunkers/BaseChunker.ts` around lines 101 - 107, The empty-content validation currently throws ChunkingError inside the withSpan block causing withSpan to record an exception; move the content.trim().length === 0 check out of the withSpan(...) scope in the BaseChunker (the method around the shown diff) and handle it as a caller/input validation: either return an empty array immediately or throw the ChunkingError before entering withSpan so the span is not marked ERROR; ensure references to this.strategy and RAGErrorCodes.CHUNKING_EMPTY_CONTENT are preserved in the returned/raised path so callers still get the same semantics.src/lib/core/modules/GenerationHandler.ts-226-234 (1)
226-234:⚠️ Potential issue | 🟡 MinorAd‑hoc cast can silently drop tool errors.
toolResultsis cast toArray<{ toolName; output?; result?; error? }>without any runtime shape check. If the AI SDK changes field names (e.g. usestoolResult.error.messageor nests underoutput),emitToolEndFromStepFinishwill receiveundefinedforerror/outputand every Pipeline B tool span will appear successful — exactly the S2/G5 gap this PR is trying to close.Consider defensively normalizing inside
emitToolEndFromStepFinish(or here) rather than relying on a bare cast. At minimum, add a test that feeds a realistic AI‑SDK v5 tool‑error step result and asserts the resultingtool:endevent carries the error.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/core/modules/GenerationHandler.ts` around lines 226 - 234, The code unsafely casts toolResults before calling emitToolEndFromStepFinish, which can drop nested SDK v5 error shapes; update GenerationHandler to defensively normalize each entry in toolResults (use the symbol toolResults and call site emitToolEndFromStepFinish/getEmitterFn) into the exact shape expected by emitToolEndFromStepFinish by extracting error message from common variants (e.g. result.error, error.message, output.error, output?.result?.error) and mapping output/result into the expected fields, then pass the normalized array; also add a unit test that constructs a realistic AI‑SDK v5 step result (with nested error.message/output.*) and asserts the emitted tool:end event contains the error.docs/telemetry-fix-diff.txt-1-100 (1)
1-100:⚠️ Potential issue | 🟡 MinorNo code — verification diff snapshot.
Generated artifact; no behavior implications. Separately,
docs/telemetry-baseline-proof.txtshows the lint run failed withELIFECYCLE exit 1due to Prettier formatting warnings ondocs/telemetry-fix-execution-plan.md,docs/telemetry-fix-plan.md,docs/telemetry-gaps-audit.md, andtest/telemetry-gaps-verification.ts. Please runprettier --writeon those files so CI can go green.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/telemetry-fix-diff.txt` around lines 1 - 100, The diff shows CI failing due to Prettier formatting warnings; run prettier --write on the four affected docs/test files (docs/telemetry-fix-execution-plan.md, docs/telemetry-fix-plan.md, docs/telemetry-gaps-audit.md, test/telemetry-gaps-verification.ts), re-stage the updated files, and commit/push the changes so the lint step passes and CI can succeed.test/telemetry-gaps-verification.ts-1001-1013 (1)
1001-1013:⚠️ Potential issue | 🟡 MinorT8's fallback anchor never updates
executeIdx.When
execute:is absent butexecute(exists,executeIdxstays-1, so the latersubstring(executeIdx, ...)starts from the top of the file. That can turn the RAG gap check into a false PASS/FAIL depending on unrelated content earlier in the file.🛠️ Suggested fix
- const executeIdx = source.indexOf("execute:"); + let executeIdx = source.indexOf("execute:"); if (executeIdx === -1) { // Try alternate patterns - const altIdx = - source.indexOf("execute(") || source.indexOf("execute ="); + const altIdx = Math.max( + source.indexOf("execute("), + source.indexOf("execute ="), + ); if (altIdx === -1) { return null; } + executeIdx = altIdx; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/telemetry-gaps-verification.ts` around lines 1001 - 1013, The fallback search for other "execute" anchors computes altIdx but never assigns it back to executeIdx, so executeIdx remains -1 and executeSection/substring uses the wrong start; fix by performing an explicit search for alternative anchors (e.g., search for "execute(" and "execute =" individually, pick the first index that is >= 0) and assign that value to executeIdx (update the code around the current source.indexOf("execute:") / altIdx logic), ensuring executeIdx is the correct start before using executeSection and substring.src/lib/neurolink.ts-3983-3991 (1)
3983-3991:⚠️ Potential issue | 🟡 Minor
step_countwill always be1here.
generateResultis assembled a few lines above and never includes astepsarray, so these attributes can't reflect the actual multi-step/tool loop. Read the count from the pre-normalized internal result before the step data is dropped, or plumb the field through first.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/neurolink.ts` around lines 3983 - 3991, generateSpan's neurolink.step_count and neurolink.max_steps_reached are computed from generateResult which was normalized and no longer contains the original steps, so step_count is always 1; instead locate the pre-normalized internal result that contains the steps array (the variable/structure built a few lines above where generateResult is assembled—the pre-normalized result used to construct generateResult) and read its steps.length there (or pass the steps count through into generateResult when normalizing). Update the code that sets generateSpan attributes (currently using generateResult and options.maxSteps) to use that pre-normalized steps count (or the newly plumbed field) so neurolink.step_count reflects the actual tool/step loop and neurolink.max_steps_reached compares that count to options.maxSteps.
🧹 Nitpick comments (10)
src/lib/providers/amazonSagemaker.ts (1)
126-153: Minor:sagemaker.not_implemented: trueis fine, but the innertry/catcharound a singlethrowis redundant.
handleProviderErroris invoked unconditionally on the only throw inside the callback, so the try/catch just forwards. SincewithSpanalready records exceptions and sets ERROR status from thrown errors, you can simplify:♻️ Simplification
async () => { - try { - // For now, throw an error indicating this is not yet implemented - throw new SageMakerError( - "SageMaker streaming not yet fully implemented. Coming in next phase.", - { - code: "MODEL_ERROR", - statusCode: 501, - endpoint: this.modelConfig.endpointName, - }, - ); - } catch (error) { - throw this.handleProviderError(error); - } + throw this.handleProviderError( + new SageMakerError( + "SageMaker streaming not yet fully implemented. Coming in next phase.", + { + code: "MODEL_ERROR", + statusCode: 501, + endpoint: this.modelConfig.endpointName, + }, + ), + ); },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/providers/amazonSagemaker.ts` around lines 126 - 153, The async callback passed to withSpan contains a redundant try/catch that only rethrows via handleProviderError; remove the try/catch and directly throw the SageMakerError from inside the callback so withSpan can record the exception and set ERROR status; keep the span attributes (including "sagemaker.not_implemented": true) and ensure you no longer call handleProviderError from this callback (handleProviderError can remain used elsewhere).src/lib/mcp/batching/requestBatcher.ts (1)
287-415: Terminal.catch(...)changes error semantics ofexecuteBatch()— it now silently swallows batch-level failures.Previously
executeBatch()would reject, letting the callers at lines 244, 267, and 421 log via their own.catch. With the new.catch((error) => logger.error(...))at line 413,executeBatch()always resolves, so:
- The caller
.catchhandlers at lines 244/267/421 become dead code.drain()only pollsisIdleso this is benign for it, but any future caller expecting rejection will be surprised.Individual request
reject()calls happen before the rethrow, so per-request contracts are preserved — this is purely about observability/API shape. Recommend either removing the terminal.catch(rely on caller-side handling) or removing the redundant caller-side handlers.♻️ Option: drop the terminal catch, keep caller handlers
- ).catch((error) => { - logger.error("Batch span execution failed:", error); - }); + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/mcp/batching/requestBatcher.ts` around lines 287 - 415, The terminal .catch on the withSpan promise swallows batch-level errors and prevents executeBatch from rejecting; remove the final .catch((error) => logger.error(...)) so the promise returned by the withSpan call (inside executeBatch) can propagate rejections to callers (preserving the existing caller-side .catch handlers at executeBatch call sites), or alternatively replace it with logging followed by rethrow if you want logging here; locate the withSpan invocation in executeBatch and delete the trailing .catch handler (or change it to rethrow after logging) so batch-level failures are not silently swallowed.src/lib/providers/litellm.ts (1)
310-320: Tool-end emission looks correct, but note the inline type-cast is duplicated across providers.The exact
Array<{ toolName; output?; result?; error? }>cast is repeated in litellm, mistral, azureOpenai, huggingFace, and anthropic. Consider havingemitToolEndFromStepFinishacceptunknown[](or an exportedToolStepResult[]alias fromtoolEndEmitter.ts) and perform the narrowing internally, so callers don't each maintain a parallel structural shape. Pure refactor — no behavior change.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/providers/litellm.ts` around lines 310 - 320, The repeated inline cast of tool result shape should be removed by changing emitToolEndFromStepFinish to accept a broader input (e.g., unknown[] or an exported ToolStepResult[] type from toolEndEmitter.ts) and perform the type-narrowing inside that function; export a ToolStepResult type alias from toolEndEmitter.ts, update emitToolEndFromStepFinish signature to use that alias or unknown[], and then remove the Array<{ toolName: string; output?: unknown; result?: unknown; error?: string; }> casts at each call site (e.g., in litellm’s onStepFinish where emitToolEndFromStepFinish(this.neurolink?.getEventEmitter(), toolResults) is called); update other providers (mistral, azureOpenai, huggingFace, anthropic) to pass the raw toolResults and rely on the centralized narrowing inside emitToolEndFromStepFinish.src/lib/server/routes/mcpRoutes.ts (1)
22-42: Consider consolidating duplicatedtracedXxxHandlerwrappers.Per the AI summary, the same wrapper pattern (http.route + http.request.id +
tracers.http) is now duplicated acrossmcpRoutes,memoryRoutes,healthRoutes,openApiRoutes,toolRoutes, andagentRoutes. Extracting a singletracedRouteHandlerhelper into a shared module (e.g.server/utils/tracedHandler.ts) would remove copy/paste and keep span attribute conventions centralized.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/server/routes/mcpRoutes.ts` around lines 22 - 42, Multiple route wrappers (e.g., tracedMcpHandler) repeat the same span setup; create a single reusable helper (e.g., tracedRouteHandler) in a shared module (server/utils/tracedHandler.ts) that accepts name, route, and the handler fn and internally calls withSpan using tracers.http and the attributes {"http.route": route, "http.request.id": ctx.requestId}; then update tracedMcpHandler, memoryRoutes, healthRoutes, openApiRoutes, toolRoutes, and agentRoutes to delegate to this new tracedRouteHandler (preserve the original function signatures like tracedMcpHandler and calls to withSpan/tracers.http so callers don’t change).src/lib/processors/registry/ProcessorRegistry.ts (1)
457-475: Consider recording outcome attributes on the span.The span is created with input attributes only; consider adding
processor.matched(boolean) /processor.name(frommatch.name) and recording exceptions viaspan.recordExceptionon failure paths to make the trace useful for debugging unsupported-type/processing-failure scenarios. Otherwise looks clean.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/processors/registry/ProcessorRegistry.ts` around lines 457 - 475, The span created by withSpan in ProcessorRegistry.processFile should record outcome attributes and exceptions: after calling this.findProcessor(fileInfo.mimetype, fileInfo.name) set span attributes "processor.matched" (true/false) and if matched set "processor.name" to match.name; when no match is found keep matched=false and return null; wrap the processor.processFile(fileInfo, options) call in try/catch and on error call span.recordException(error) and set an attribute like "processor.error" or "processor.status" before rethrowing or returning a failure; reference the withSpan call, findProcessor, the returned match, and the processor (BaseFileProcessor.processFile) to locate where to add these span attribute and exception-recording updates.src/lib/rag/chunkers/BaseChunker.ts (1)
92-96: Duplicate span attributes.
rag.chunker.content_lengthandrag.chunker.content_charsare set to the same value (content.length) and convey identical information. Drop one (likelycontent_chars) to avoid schema noise in Langfuse/OTel.♻️ Suggested change
attributes: { "rag.chunker.strategy": this.strategy, "rag.chunker.content_length": content.length, - "rag.chunker.content_chars": content.length, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/rag/chunkers/BaseChunker.ts` around lines 92 - 96, In BaseChunker where the span attributes are set (attributes: { "rag.chunker.strategy": this.strategy, "rag.chunker.content_length": content.length, "rag.chunker.content_chars": content.length }), remove the duplicate attribute "rag.chunker.content_chars" so only "rag.chunker.content_length" remains; update any related comments/tests that reference content_chars and ensure any telemetry/schema consumers use the single "rag.chunker.content_length" attribute (class/method: BaseChunker attribute assignment).src/lib/client/sseClient.ts (1)
697-699: Reconnect path produces deeply nested spans.
attemptReconnectcallsthis.stream(path, options, callbacks), which re-enterswithClientSpan(...). Because the outer_streamInternalis stillawaiting inside its span, each reconnect becomes a child of the previous attempt's span. WithmaxReconnectAttempts = 5(default) this is tolerable, but for a long-lived flaky connection you'll get a 6-level-deepneurolink.client.sse.streamchain per logical stream.Consider either:
- calling
this._streamInternal(...)directly fromattemptReconnect(keeps a single span per logical stream), or- tagging the reconnect with
sse.reconnect_attemptand letting each attempt be a sibling span instead of a child.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/client/sseClient.ts` around lines 697 - 699, The reconnect logic in attemptReconnect calls this.stream(path, options, callbacks) which re-enters withClientSpan and nests spans under the original _streamInternal span; change attemptReconnect to call this._streamInternal(...) directly (preserving the same span) or alter reconnection to create sibling spans by invoking withClientSpan with a tag like "sse.reconnect_attempt" so reconnect attempts are not children of the original span; update the code in the attemptReconnect function to use either this._streamInternal(...) instead of this.stream(...) or wrap the reconnect call in withClientSpan and add the "sse.reconnect_attempt" attribute, keeping maxReconnectAttempts behavior intact.src/lib/providers/amazonBedrock.ts (3)
2227-2238: Usethis.providerNamefor consistency.The new
RateLimitErrorbranch hardcodes the provider string to"bedrock", while the siblingAuthenticationError/ProviderErrorbranches in the same method usethis.providerName. Prefer the same pattern here so downstream consumers see a single canonical provider identifier.🛠️ Suggested fix
return new RateLimitError( `Bedrock rate limit (throttled): ${error instanceof Error ? error.message : String(error)}`, - "bedrock", + this.providerName, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/providers/amazonBedrock.ts` around lines 2227 - 2238, The RateLimitError branch in the Bedrock error mapping currently hardcodes the provider string "bedrock"; update it to use the instance property this.providerName for consistency with the AuthenticationError/ProviderError branches—locate the block that checks errName/errCode for "ThrottlingException" and change the provider argument passed to new RateLimitError to this.providerName so downstream consumers get the canonical provider identifier.
284-304: Consider emittinggeneration:endon failure too.
generate()only emitsgeneration:endon the success path. IfconversationLoop()throws (Bedrock error, max-iterations, tool mapping mismatch, etc.), no event is emitted and Pipeline B never sees the failed generation — arguably the most important case for observability. A try/catch that emitssuccess: falsewith the error classification before rethrowing would close that gap symmetrically with the new streaming path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/providers/amazonBedrock.ts` around lines 284 - 304, The generate() flow currently only emits generation:end after a successful conversationLoop() call; wrap the call to this.conversationLoop(options) in a try/catch inside generate(), and in the catch emit a generation:end via this.neurolink?.getEventEmitter() with success: false, include responseTime (Date.now() - generateStartTime), timestamp, result metadata (model: this.modelName || this.getDefaultModel(), provider: this.providerName), and an error classification/message, then rethrow the error so upstream behavior is unchanged; ensure the same emitter path used in the success branch (generateEmitter = this.neurolink?.getEventEmitter()) is used for failures for symmetric observability.
1394-1483: Duplication: first-turn and continuation stream parsers are ~90% identical.The inline first-iteration chunk parser (lines ~1396–1483) and
processStreamResponse(lines ~1821–1925) now both maintain the_inputBuffer/toolUsereconstruction logic, themessageStop→ continue pattern, and themetadata.usagebreak. Any future bug fix to stream-chunk handling has to be made in two places, and this PR already had to update both in parallel. Extracting a shared private helper (e.g.,private async drainConverseStream(stream, controller): Promise<{ content, stopReason, usage }>) would collapse both call sites.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/providers/amazonBedrock.ts` around lines 1394 - 1483, Duplicate stream-parsing logic exists between the inline first-iteration loop and processStreamResponse; extract the common behavior into a private helper (suggested name: drainConverseStream) that accepts the stream and controller and returns an object like { contentBlocks: firstMessageContent, stopReason, usage: { inputTokens, outputTokens } }. Move shared handling for contentBlockStart/contentBlockDelta/contentBlockStop, _inputBuffer/toolUse reconstruction, messageStop → continue behavior, and metadata.usage break into that helper, then replace the inline loop and the body of processStreamResponse to call drainConverseStream(stream, controller) and consume its returned contentBlocks, stopReason, and usage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 569b1d2e-3229-47df-bd69-b13fbbfb669a
📒 Files selected for processing (69)
docs/telemetry-after-proof.txtdocs/telemetry-baseline-proof.txtdocs/telemetry-fix-completion-report.mddocs/telemetry-fix-diff.txtdocs/telemetry-fix-execution-plan.mddocs/telemetry-fix-plan.mddocs/telemetry-gaps-audit.mdsrc/cli/loop/session.tssrc/lib/auth/anthropicOAuth.tssrc/lib/auth/sessionManager.tssrc/lib/auth/tokenStore.tssrc/lib/client/httpClient.tssrc/lib/client/sseClient.tssrc/lib/client/streamingClient.tssrc/lib/client/wsClient.tssrc/lib/context/budgetChecker.tssrc/lib/context/contextCompactor.tssrc/lib/context/summarizationEngine.tssrc/lib/core/baseProvider.tssrc/lib/core/modules/GenerationHandler.tssrc/lib/core/modules/StreamHandler.tssrc/lib/core/modules/ToolsManager.tssrc/lib/evaluation/ragasEvaluator.tssrc/lib/mcp/batching/requestBatcher.tssrc/lib/mcp/httpRateLimiter.tssrc/lib/mcp/httpRetryHandler.tssrc/lib/mcp/mcpClientFactory.tssrc/lib/mcp/toolDiscoveryService.tssrc/lib/mcp/toolRegistry.tssrc/lib/memory/memoryRetrievalTools.tssrc/lib/neurolink.tssrc/lib/observability/utils/spanSerializer.tssrc/lib/processors/base/BaseFileProcessor.tssrc/lib/processors/registry/ProcessorRegistry.tssrc/lib/providers/amazonBedrock.tssrc/lib/providers/amazonSagemaker.tssrc/lib/providers/anthropic.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/rag/chunkers/BaseChunker.tssrc/lib/rag/ragIntegration.tssrc/lib/rag/reranker/reranker.tssrc/lib/rag/retrieval/vectorQueryTool.tssrc/lib/server/middleware/common.tssrc/lib/server/routes/agentRoutes.tssrc/lib/server/routes/claudeProxyRoutes.tssrc/lib/server/routes/healthRoutes.tssrc/lib/server/routes/mcpRoutes.tssrc/lib/server/routes/memoryRoutes.tssrc/lib/server/routes/openApiRoutes.tssrc/lib/server/routes/toolRoutes.tssrc/lib/services/server/ai/observability/instrumentation.tssrc/lib/telemetry/traceContext.tssrc/lib/telemetry/tracers.tssrc/lib/types/span.tssrc/lib/utils/toolEndEmitter.tssrc/lib/workflow/core/ensembleExecutor.tssrc/lib/workflow/core/judgeScorer.tssrc/lib/workflow/core/responseConditioner.tssrc/lib/workflow/core/workflowRunner.tstest/telemetry-gaps-verification.ts
| } catch (error) { | ||
| const errorMessage = | ||
| error instanceof Error ? error.message : String(error); | ||
|
|
||
| // Emit discovery failed event | ||
| this.emit("discoveryFailed", { | ||
| serverId, | ||
| error: errorMessage, | ||
| timestamp: new Date(), | ||
| } satisfies ToolRegistryEvents["discoveryFailed"]); | ||
| mcpLogger.error( | ||
| `[ToolDiscoveryService] Discovery failed for ${serverId}:`, | ||
| error, | ||
| ); | ||
|
|
||
| return { | ||
| success: false, | ||
| error: errorMessage, | ||
| toolCount: 0, | ||
| tools: [], | ||
| duration: Date.now() - startTime, | ||
| serverId, | ||
| }; | ||
| } finally { | ||
| this.discoveryInProgress.delete(serverId); | ||
| } | ||
| // Emit discovery failed event | ||
| this.emit("discoveryFailed", { | ||
| serverId, | ||
| error: errorMessage, | ||
| timestamp: new Date(), | ||
| } satisfies ToolRegistryEvents["discoveryFailed"]); | ||
|
|
||
| return { | ||
| success: false, | ||
| error: errorMessage, | ||
| toolCount: 0, | ||
| tools: [], | ||
| duration: Date.now() - startTime, | ||
| serverId, | ||
| }; | ||
| } finally { | ||
| this.discoveryInProgress.delete(serverId); | ||
| } | ||
| }, | ||
| ); |
There was a problem hiding this comment.
Discovery failures won't mark the span as ERROR — same gap the T3 fix addresses in toolRegistry.ts.
The catch at line 155 converts exceptions into a returned { success: false, ... } result instead of rethrowing. Because withSpan only auto-detects errors via thrown exceptions, the neurolink.mcp.discoverTools span will be recorded with OK status for every failed discovery, hiding failures in Langfuse/OTel and diverging from the explicit setStatus(ERROR) + recordException pattern you just applied in src/lib/mcp/toolRegistry.ts (lines 554–562).
🛠️ Proposed fix
} catch (error) {
const errorMessage =
error instanceof Error ? error.message : String(error);
+ span.setStatus({ code: SpanStatusCode.ERROR, message: errorMessage });
+ if (error instanceof Error) {
+ span.recordException(error);
+ }
+
mcpLogger.error(
`[ToolDiscoveryService] Discovery failed for ${serverId}:`,
error,
);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/mcp/toolDiscoveryService.ts` around lines 155 - 183, The catch in
ToolDiscoveryService's discovery handler converts exceptions into a returned
failure result so withSpan (neurolink.mcp.discoverTools) never sees a thrown
error and the span stays OK; update the catch block inside the discovery
function to call span.setStatus({ code: SpanStatusCode.ERROR }) and
span.recordException(error) (same pattern used in toolRegistry.ts) and then
rethrow the error (instead of just returning { success: false, ... }), while
still emitting the "discoveryFailed" event and ensuring
discoveryInProgress.delete(serverId) runs in finally; this ensures the withSpan
wrapper records the error status and exception for neurolink.mcp.discoverTools.
| // Validate weights sum to 1.0 | ||
| const totalWeight = | ||
| (weights.semantic || DEFAULT_WEIGHTS.semantic) + | ||
| (weights.vector || DEFAULT_WEIGHTS.vector) + | ||
| (weights.position || DEFAULT_WEIGHTS.position); | ||
|
|
||
| if (Math.abs(totalWeight - 1.0) > 0.01) { | ||
| logger.warn("[Reranker] Weights do not sum to 1.0, normalizing", { | ||
| original: weights, | ||
| total: totalWeight, | ||
| }); | ||
| } | ||
|
|
||
| const normalizedWeights = { | ||
| semantic: (weights.semantic || DEFAULT_WEIGHTS.semantic) / totalWeight, | ||
| vector: (weights.vector || DEFAULT_WEIGHTS.vector) / totalWeight, | ||
| position: (weights.position || DEFAULT_WEIGHTS.position) / totalWeight, | ||
| }; | ||
|
|
||
| const rerankedResults: RerankResult[] = []; | ||
|
|
||
| // Process results in parallel batches for efficiency | ||
| const batchSize = 5; | ||
| for (let i = 0; i < results.length; i += batchSize) { | ||
| const batch = results.slice(i, i + batchSize); | ||
| const batchPromises = batch.map(async (result, batchIndex) => { | ||
| const globalIndex = i + batchIndex; | ||
|
|
||
| // Calculate vector score (use existing score or 0) | ||
| const vectorScore = result.score ?? 0; | ||
|
|
||
| // Calculate position score (inverse of position) | ||
| const positionScore = 1 - globalIndex / results.length; | ||
|
|
||
| // Calculate semantic score using LLM | ||
| const semanticResult = await calculateSemanticScore( | ||
| query, | ||
| result.text || (result.metadata?.text as string) || "", | ||
| model, | ||
| ); | ||
|
|
||
| // Combine scores | ||
| const combinedScore = | ||
| normalizedWeights.semantic * semanticResult.score + | ||
| normalizedWeights.vector * vectorScore + | ||
| normalizedWeights.position * positionScore; | ||
|
|
||
| return { | ||
| result, | ||
| score: combinedScore, | ||
| details: { | ||
| semantic: semanticResult.score, | ||
| vector: vectorScore, | ||
| position: positionScore, | ||
| queryAnalysis: semanticResult.analysis, | ||
| }, | ||
| const normalizedWeights = { | ||
| semantic: (weights.semantic || DEFAULT_WEIGHTS.semantic) / totalWeight, | ||
| vector: (weights.vector || DEFAULT_WEIGHTS.vector) / totalWeight, | ||
| position: (weights.position || DEFAULT_WEIGHTS.position) / totalWeight, |
There was a problem hiding this comment.
Honor explicit zero weights.
These blocks use || to apply defaults, so a caller cannot intentionally set semantic, vector, or position to 0. For example, { semantic: 0, vector: 1, position: 0 } gets rewritten to the defaults instead of disabling semantic/position scoring. Use ?? for defaulting and reject totalWeight <= 0.
Also applies to: 227-237
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/rag/reranker/reranker.ts` around lines 69 - 85, The weight
normalization uses || which treats explicit zeros as absent; change the
defaulting to use nullish coalescing (weights.semantic ??
DEFAULT_WEIGHTS.semantic, weights.vector ?? DEFAULT_WEIGHTS.vector,
weights.position ?? DEFAULT_WEIGHTS.position) when computing totalWeight and
when building normalizedWeights, and add a guard that rejects or throws if
totalWeight <= 0 (instead of dividing by zero) with a clear error/log message
via logger.warn or throw; apply the same change to the analogous block around
the code referenced at lines ~227-237 in reranker.ts.
| return withSpan( | ||
| { | ||
| name: "neurolink.http.execute", | ||
| tracer: tracers.http, | ||
| attributes: { | ||
| "http.route": "/api/agent/execute", | ||
| "ai.provider": request.provider || "default", | ||
| "ai.model": request.model || "default", | ||
| }, |
There was a problem hiding this comment.
Use basePath in the new http.route attributes.
These handlers are mounted with a configurable basePath, but the spans are hardcoded to /api/.... Any deployment that mounts the routes elsewhere will emit the wrong route label and split telemetry across otherwise identical endpoints.
Suggested fix
- "http.route": "/api/agent/execute",
+ "http.route": `${basePath}/agent/execute`,
...
- "http.route": "/api/agent/stream",
+ "http.route": `${basePath}/agent/stream`,
...
- "http.route": "/api/agent/embed",
+ "http.route": `${basePath}/agent/embed`,
...
- "http.route": "/api/agent/embed-many",
+ "http.route": `${basePath}/agent/embed-many`,Also applies to: 139-147, 229-237, 292-301
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/server/routes/agentRoutes.ts` around lines 57 - 65, The spans in
agentRoutes.ts set the "http.route" attribute to hardcoded "/api/..." values
(see the withSpan calls using tracers.http and attributes with "http.route"),
which breaks telemetry when routes are mounted under a configurable basePath;
update each withSpan that currently sets "http.route" (including the occurrences
for execute, and the other two blocks in this file) to build the route using the
configured basePath plus the route suffix (e.g., `${basePath}/agent/execute` or
similar) rather than a hardcoded string; locate the withSpan invocations and
modify the attributes object to reference the basePath variable used for
mounting the handlers and concatenate the route path so the span uses the actual
mounted route.
| attributes: { | ||
| "http.route": `${basePath}/tools/search`, | ||
| "tools.search.query": (ctx.query.q as string) ?? "", | ||
| }, | ||
| }, | ||
| async () => { | ||
| const { q, source, limit } = ctx.query; | ||
| const tools = await ctx.toolRegistry.listTools(); | ||
|
|
||
| let filtered = tools; | ||
| let filtered = tools; | ||
|
|
||
| // Filter by search query | ||
| if (q) { | ||
| const query = q.toLowerCase(); | ||
| filtered = filtered.filter( | ||
| (tool) => | ||
| tool.name.toLowerCase().includes(query) || | ||
| (tool.description && | ||
| tool.description.toLowerCase().includes(query)), | ||
| ); | ||
| } | ||
| // Filter by search query | ||
| if (q) { | ||
| const query = q.toLowerCase(); | ||
| filtered = filtered.filter( | ||
| (tool) => | ||
| tool.name.toLowerCase().includes(query) || | ||
| (tool.description && | ||
| tool.description.toLowerCase().includes(query)), | ||
| ); | ||
| } | ||
|
|
||
| // Filter by source | ||
| if (source) { | ||
| filtered = filtered.filter( | ||
| (tool) => (tool.source || "built-in") === source, | ||
| ); | ||
| } | ||
| // Filter by source | ||
| if (source) { | ||
| filtered = filtered.filter( | ||
| (tool) => (tool.source || "built-in") === source, | ||
| ); | ||
| } | ||
|
|
||
| // Apply limit | ||
| const maxResults = limit ? parseInt(limit, 10) : 50; | ||
| filtered = filtered.slice(0, maxResults); | ||
| // Apply limit | ||
| const maxResults = limit ? parseInt(limit, 10) : 50; | ||
| filtered = filtered.slice(0, maxResults); |
There was a problem hiding this comment.
Normalize query params before using them as strings.
Line 86 assumes q is a string, and Line 103 assumes limit is a parseable scalar. In most query parsers those values can also be arrays, so a request like ?q=a&q=b will throw on toLowerCase(), and malformed limit values can collapse the result set unexpectedly. Validate or normalize the query params before filtering.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/server/routes/toolRoutes.ts` around lines 73 - 104, Normalize and
validate query params before using them: extract q, source, and limit from
ctx.query as strings/numbers (e.g., if Array.isArray(q) use q[0] or default to
"", likewise for source), convert the normalized q toLowerCase() only after
ensuring it's a string, and parse limit with parseInt then validate it (fallback
to a safe default like 50 if parseInt returns NaN or a non-positive value)
before slicing filtered; update the search handler that uses ctx.query, q,
source, limit and ctx.toolRegistry.listTools to perform these checks so
multi-valued or malformed query params do not throw or unexpectedly collapse
results.
769aedc to
ce38e9b
Compare
Review Feedback Addressed (Cycle 1)Fixes Applied (14 of 25 comments addressed)Span error status on returned errors (Comments #4, #5, #7, #8, #10)
Don't fall back to fake streaming on terminal errors (Comment #9 — Critical)
Stream generation:end success flag accuracy (Comments #13, #15)
Dead stream finishReason code (Comment #11)
Proxy response redaction (Comment #17)
Verification test exit code (Comment #19)
Proof artifact inconsistency (Comment #18)
Tool retry count on failed retries (Review body — RB-6)
WebSocket span leak (Review body — RB-4)
Intentionally Deferred (5 items)
Dismissed (6 items)
Verification
Requesting Re-review@coderabbitai full review |
|
A few notes on the dismissed items I'll record for future reviews:
Triggering the full re-review now. ✏️ Learnings added
✅ Actions performedFull review triggered. |
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
|
@coderabbitai have you completed the full comprehensive review or not? |
|
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 13
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 (5)
src/lib/client/httpClient.ts (1)
506-547:⚠️ Potential issue | 🟡 Minor
stream()path is not wrapped withwithClientSpan.The PR instruments
request()(and per the summary,sseClient/streamingClient/wsClient), butNeuroLinkClient.stream()here bypassesrequest()and issues a directfetchagainst/api/agent/stream. As a result, client-side SSE streaming requests don't produce anhttp.*span and won't show up in the Pipeline-A trace alongside other client transports. Consider wrapping the fetch+read loop inwithClientSpanusingtracers.httpfor consistency with the rest of the telemetry coverage added in this PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/client/httpClient.ts` around lines 506 - 547, The stream() implementation issues a direct fetch and should be wrapped in withClientSpan using tracers.http (like other transports) so SSE calls produce an http.* span; modify the async body of stream() (the fetch + response-read/error handling logic that uses this.config.fetch, requestId, and response.ok) to run inside withClientSpan(tracers.http, { attributes: { "http.method": "POST", "http.url": `${this.config.baseUrl}/api/agent/stream`, "request.id": requestId }}, async (span) => { ... }), ensure you propagate the original signal, capture and record error attributes on span before rethrowing, and close/end the span in a finally block so spans are created for NeuroLinkClient.stream() similarly to request()/sseClient/streamingClient/wsClient.src/lib/providers/openAI.ts (1)
451-526:⚠️ Potential issue | 🟡 MinorPotential span leak if post-
streamTextsetup throws.
streamSpanis only ended synchronously in the inner catch (lines 513–526) or asynchronously whenresult.textsettles (lines 562–572). If an exception is thrown betweenstreamText()returning and the function returning (e.g., insidecreateOpenAITransformedStreamconstruction,streamAnalyticsCollector.createAnalytics, or any future addition), the outer catch at line 613 rethrows without endingstreamSpan, and since the caller never receivesresultthe stream may never be consumed — leaving the span open.Consider ending
streamSpanin the outer catch as well (guarded by a "not yet ended" flag), or move the span.end() into afinallywired to a consumed/error signal.🤖 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 451 - 526, The streamSpan created before calling streamText can remain open if an exception is thrown after streamText returns but before the returned stream is consumed; update the error handling so streamSpan is always ended: add a guarded "ended" flag (or similar) that you set when you call streamSpan.end() (currently done inside the inner catch and when result.text settles), then in the outer catch (the one that currently rethrows without ending the span) call streamSpan.end() if not already ended; alternatively move the span.end() into a finally block tied to the stream consumption lifecycle (e.g., in the completion/rejection handlers created by createOpenAITransformedStream, streamAnalyticsCollector.createAnalytics, or when awaiting result.text) so streamSpan is reliably closed even if post-streamText setup throws. Ensure references: streamSpan, streamText, result.text, createOpenAITransformedStream, streamAnalyticsCollector.createAnalytics.src/lib/providers/googleAiStudio.ts (1)
1285-1327:⚠️ Potential issue | 🟡 Minor
finishReasonmisreportsmax_stepsat the boundary inexecuteNativeGemini3Generate.
step >= maxSteps ? "max_steps" : "stop"(lines 1302, 1323) is wrong when the loop completes normally on the final allowed iteration:stepis incremented at the top of each iteration, so breaking successfully on the last iteration leavesstep === maxStepsand gets reported asmax_stepseven though the model stopped on its own. The stream path correctly tracks this viacompletedWithFinalAnswer; please mirror that here.🛠️ Proposed fix
let finalText = ""; let lastStepText = ""; let totalInputTokens = 0; let totalOutputTokens = 0; + let completedWithFinalAnswer = false; ... // If no function calls, we're done if (chunkResult.stepFunctionCalls.length === 0) { finalText = stepText; + completedWithFinalAnswer = true; break; } ... - finalText = handleMaxStepsTermination( + const hitStepLimitWithoutFinalAnswer = + step >= maxSteps && !completedWithFinalAnswer; + finalText = handleMaxStepsTermination( "[GoogleAIStudio]", step, maxSteps, finalText, lastStepText, ); ... - span.setAttribute( - ATTR.GEN_AI_FINISH_REASON, - step >= maxSteps ? "max_steps" : "stop", - ); + span.setAttribute( + ATTR.GEN_AI_FINISH_REASON, + hitStepLimitWithoutFinalAnswer ? "max_steps" : "stop", + ); ... - finishReason: step >= maxSteps ? "max_steps" : "stop", + finishReason: hitStepLimitWithoutFinalAnswer + ? "max_steps" + : "stop",🤖 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 1285 - 1327, In executeNativeGememi3Generate the finishReason is computed using step >= maxSteps which misreports normal completion on the final allowed iteration; change the logic to use the same completedWithFinalAnswer flag as the stream path (or a boolean that’s set when the model returns a final answer) to decide finishReason ("stop" when completedWithFinalAnswer is true, otherwise "max_steps"), and update both the span.setAttribute(ATTR.GEN_AI_FINISH_REASON, ...) call and the nativeGenerateEmitter.emit payload (result.finishReason) to use that flag instead of comparing step and maxSteps so the finish reason matches the stream path.src/lib/providers/ollama.ts (1)
959-1138:⚠️ Potential issue | 🟠 MajorMirror this manual
generation:endemission in the non-tool stream path too.This closes the gap for
executeStreamWithTools(), butexecuteStreamWithoutTools()still bypasses the AI SDK without emitting an equivalentgeneration:end. Plain Ollama streams will therefore keep missing the Pipeline B generation observation and usage/accounting that tool-enabled streams now get.🤖 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 959 - 1138, The non-tool path (executeStreamWithoutTools) must mirror the manual generation:end emission and analytics resolution you added for the tool path: in executeStreamWithoutTools (the stream's finally/cleanup block) capture the same instance refs (ollamaNeurolink, this.providerName, this.modelName || FALLBACK_OLLAMA_MODEL), build aggregatedUsage from totalInputTokens/totalOutputTokens, call resolveAnalytics(createAnalytics(...)) with the same metadata (requestId like `ollama-stream-${Date.now()}`, streamingMode: true, iterations: iteration, response time from startTime), then if ollamaNeurolink?.getEventEmitter() exists emit "generation:end" with the same payload shape (provider, responseTime, timestamp, result: { content: "", usage: aggregatedUsage, model: modelName, provider, finishReason: ollamaStreamErrored ? "error" : (lastFinishReason ?? "stop") }, success: !ollamaStreamErrored); ensure you reference the same local variables (totalInputTokens, totalOutputTokens, lastFinishReason, ollamaStreamErrored, iteration, startTime) so Pipeline B receives the generation observation for non-tool streams too.src/lib/neurolink.ts (1)
6518-6528:⚠️ Potential issue | 🟠 MajorFallback stream completions still emit stale telemetry.
These payloads can still misreport successful fallback streams: in the zero-chunk fallback path,
chunkCountnever includes fallback chunks, and in the error-fallback path the emitted completion hard-codeschunkCount: 0,finishReason: "stop", andmodel: options.model. The resulting Pipeline B span can look empty or be attributed to the wrong termination reason/model even when fallback content streamed successfully. Feed the actual fallback chunk count, finish reason, andfallbackStreamResult.modelinto the emittedstream:completeevent.Also applies to: 7692-7703
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/neurolink.ts` around lines 6518 - 6528, The emitted stream completion payload currently uses stale values (chunkCount, finishReason, model) when a fallback stream path is used; update the stream:complete emission logic (the block that builds the completion object using streamState, chunkCount, accumulatedContent, sessionId, resolvedUsage) to prefer values from the fallback result when present—specifically replace hard-coded or pre-fallback values with fallbackStreamResult.chunkCount (or computed fallbackChunkCount), fallbackStreamResult.finishReason (or streamState.finishReason if fallback absent), and fallbackStreamResult.model (instead of options.model) so zero-chunk and error-fallback paths report the actual fallbackChunkCount, finishReason, and model; apply the same fix in the equivalent emission at the other occurrence around the 7692-7703 region.
♻️ Duplicate comments (8)
src/lib/providers/googleVertex.ts (1)
2200-2225:⚠️ Potential issue | 🟠 MajorStream
generation:endstill drops the generated text.This event always sends
result.content: "", so Pipeline B/Langfuse records a successful generation with no output even when the stream produced text. Capture the terminalstepText(or accumulate streamed text) and emit that instead.Suggested fix
- if (chunkResult.stepFunctionCalls.length === 0) { - completedWithFinalAnswer = true; - break; - } + if (chunkResult.stepFunctionCalls.length === 0) { + lastStepText = stepText; + completedWithFinalAnswer = true; + break; + } ... - content: "", + content: lastStepText,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/providers/googleVertex.ts` around lines 2200 - 2225, The emitted "generation:end" currently sends result.content: "" which discards the streamed output; change the emit to include the final generated text by using the terminal stepText (or the accumulator that collects streamed tokens, e.g., streamedText/accumulatedText) instead of an empty string, so in the block where vertexStreamEmitter.emit("generation:end", {...}) set result.content to the final assembled text (trim/join if you accumulate per-step) and keep the rest of the payload (provider, usage, model, finishReason) unchanged; reference the existing vertexStreamEmitter, params.maxSteps, step, completedWithFinalAnswer and the variable that holds per-step text (stepText or your streaming accumulator) when implementing the fix.docs/telemetry-after-proof.txt (1)
69-73:⚠️ Potential issue | 🟠 MajorA4 is still summarized incorrectly.
The detailed section shows one confirmed A4 test and one not-confirmed A4 test, but the per-gap summary collapses A4 to
NOT CONFIRMED. That makes the proof artifact misleading again.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/telemetry-after-proof.txt` around lines 69 - 73, The per-gap summary under the "Per-gap summary" block incorrectly collapses mixed-status gaps (like A4) to NOT CONFIRMED; update the summary-generation logic (e.g., the function generatePerGapSummary or renderPerGapSummary and the gapSummaries/gaps aggregation code) to aggregate test results per gap and choose the summary status by checking counts in this order: if any CONFIRMED exists mark CONFIRMED, else if any SKIP and no CONFIRMED mark SKIP, else if any NOT CONFIRMED mark NOT CONFIRMED; ensure the A4 aggregation uses these counts rather than collapsing mixed results to NOT CONFIRMED.src/lib/server/routes/toolRoutes.ts (1)
80-116:⚠️ Potential issue | 🟡 MinorQuery params still treated as
string/parseable without normalization.Same observation from the prior cycle:
ctx.query.q,source, andlimitcan be arrays in most query parsers, andparseInton NaN silently yields an unexpected 0-result slice. Cast at line 76 ((ctx.query.q as string) ?? "") also crashestoLowerCase()ifqarrives as an array. Worth normalizing in one place up front.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/server/routes/toolRoutes.ts` around lines 80 - 116, Normalize and validate query params from ctx.query up front: coerce q, source and limit to single string values before using them (e.g., ensure q is a single string before calling toLowerCase, ensure source is a string before comparing, and parseInt limit safely). In the handler that calls ctx.toolRegistry.listTools() use a short normalization block that handles array inputs (take first element), defaults (q -> "", source -> "built-in"), and safe integer parsing for limit (fallback to 50 when parseInt yields NaN or non-positive), then use the normalized q, source, and maxResults variables in the filtering and slicing logic to avoid crashes and unexpected 0-length slices.src/lib/server/routes/agentRoutes.ts (1)
62-65:⚠️ Potential issue | 🟠 Major
http.routestill hardcoded to/api/...across all four endpoints.Same finding as the prior cycle: the prefix is computed from the caller-provided
basePath, but the span attribute hardcodes/api/agent/.... Any mount under a differentbasePathwill split telemetry. Since the PR scope is full OTel coverage, worth fixing now rather than re-deferring.🛠️ Proposed fix
- "http.route": "/api/agent/execute", + "http.route": `${basePath}/agent/execute`, ... - "http.route": "/api/agent/stream", + "http.route": `${basePath}/agent/stream`, ... - "http.route": "/api/agent/embed", + "http.route": `${basePath}/agent/embed`, ... - "http.route": "/api/agent/embed-many", + "http.route": `${basePath}/agent/embed-many`,Also applies to: 144-147, 234-237, 297-301
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/server/routes/agentRoutes.ts` around lines 62 - 65, The telemetry span attribute "http.route" is hardcoded to "/api/agent/..." causing split traces when the router is mounted under a different basePath; update the span attribute to use the computed mount prefix instead of the literal string. Locate the places in agentRoutes.ts where "http.route" is set (inside the execute handler and the three other endpoints referenced) and replace the literal "/api/agent/..." with the dynamic prefix (the same value used to build the route, e.g., the computed basePath/prefix or the existing variable used to mount routes) concatenated with the endpoint path (for example `${prefix}/agent/execute`), or alternatively derive it from request.baseUrl/request.originalUrl consistently; apply this change to all four occurrences (the blocks around the execute handler and the three other endpoint handlers).src/lib/rag/reranker/reranker.ts (1)
69-85:⚠️ Potential issue | 🟠 MajorHonor explicit zero weights and reject zero-total configs.
Lines 71-85 and Line 229-Line 237 still use
||for defaulting, so{ semantic: 0, vector: 1, position: 0 }gets rewritten to the defaults, and{ semantic: 0, vector: 0, position: 0 }will divide by zero during normalization.Suggested fix
- const totalWeight = - (weights.semantic || DEFAULT_WEIGHTS.semantic) + - (weights.vector || DEFAULT_WEIGHTS.vector) + - (weights.position || DEFAULT_WEIGHTS.position); + const semanticWeight = weights.semantic ?? DEFAULT_WEIGHTS.semantic; + const vectorWeight = weights.vector ?? DEFAULT_WEIGHTS.vector; + const positionWeight = weights.position ?? DEFAULT_WEIGHTS.position; + const totalWeight = + semanticWeight + vectorWeight + positionWeight; + + if (totalWeight <= 0) { + throw new Error("Reranker weights must sum to a value greater than 0"); + } if (Math.abs(totalWeight - 1.0) > 0.01) { logger.warn("[Reranker] Weights do not sum to 1.0, normalizing", { original: weights, total: totalWeight, }); } const normalizedWeights = { - semantic: (weights.semantic || DEFAULT_WEIGHTS.semantic) / totalWeight, - vector: (weights.vector || DEFAULT_WEIGHTS.vector) / totalWeight, - position: (weights.position || DEFAULT_WEIGHTS.position) / totalWeight, + semantic: semanticWeight / totalWeight, + vector: vectorWeight / totalWeight, + position: positionWeight / totalWeight, };#!/bin/bash node - <<'NODE' const defaults = { semantic: 0.4, vector: 0.4, position: 0.2 }; const weights = { semantic: 0, vector: 1, position: 0 }; const withOr = { semantic: weights.semantic || defaults.semantic, vector: weights.vector || defaults.vector, position: weights.position || defaults.position, }; const withNullish = { semantic: weights.semantic ?? defaults.semantic, vector: weights.vector ?? defaults.vector, position: weights.position ?? defaults.position, }; console.log({ withOr, withNullish, zeroTotal: 0 / 0 }); NODEAlso applies to: 227-237
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/rag/reranker/reranker.ts` around lines 69 - 85, The weights normalization code incorrectly uses || which treats explicit zero as missing and can produce divide-by-zero; update all defaulting of weights (where weights.semantic/vector/position are combined with DEFAULT_WEIGHTS and where normalizedWeights is computed — e.g. in reranker.ts around the weights/normalizedWeights logic and the similar block at lines ~227-237) to use the nullish coalescing operator (??) so explicit zeros are honored, then validate totalWeight after summing and throw or return an error if totalWeight === 0 (or very near zero) instead of dividing; ensure logger.warn still runs when totalWeight deviates from 1.0 but reject zero-total configs explicitly.src/lib/memory/memoryRetrievalTools.ts (1)
243-255:⚠️ Potential issue | 🟠 MajorThese not-found returns still bypass the outer span error status.
The new wrapper only sees a normal return in the session-not-found and message-not-found branches, so
neurolink.memory.retrieve_contextwill still be exported as successful unless you also markotelSpanas failed here.Also applies to: 261-270
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/memory/memoryRetrievalTools.ts` around lines 243 - 255, The not-found early returns (when memoryManager.getSessionRaw(sessionId) or message lookup fails) currently end and record an internal span via SpanSerializer.endSpan but do not mark the outer otelSpan as failed, so neurolink.memory.retrieve_context is exported as successful; update both the session-not-found branch (around getSessionRaw() handling) and the message-not-found branch (lines ~261-270) to set the otelSpan status to error (include a descriptive message like `Session not found: ${sessionId}` or `Message not found: ${messageId}`) before ending/recording spans, using the same SpanStatus.ERROR semantics you already use with SpanSerializer.endSpan and getMetricsAggregator().recordSpan to ensure the outer span is exported as failed.src/lib/core/baseProvider.ts (1)
261-300:⚠️ Potential issue | 🔴 CriticalThe real-stream fallback is still too broad for terminal failures.
This still falls back to
executeFakeStreaming()for any stream error that doesn't match these exact lowercase substrings. A typedRateLimitError,AuthenticationError,TimeoutError, or even"Rate Limit"/"Authentication"message will slip through and trigger a second upstream request. The fallback needs to be gated by an explicit “streaming-with-tools unsupported” classification, not by negative string matching.Based on learnings,
src/lib/core/baseProvider.tsis the base class all providers extend. Centralstream()method merges tools before calling provider-specificexecuteStream().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/core/baseProvider.ts` around lines 261 - 300, The current catch in the stream flow (in BaseProvider.stream that calls executeStream) must stop using negative substring matching and only fall back to executeFakeStreaming when the error is explicitly a “streaming-with-tools unsupported” signal; update the catch to: treat AbortError/TimeoutError/RateLimitError/AuthenticationError (or any error with a dedicated type/property like instanceof RateLimitError, AuthenticationError, TimeoutError, or a boolean flag such as error.isStreamingNotSupported or error.code === 'STREAMING_UNSUPPORTED') as terminal and rethrow via handleProviderError, and only call executeFakeStreaming(options, analysisSchema) when the provider explicitly indicates streaming-with-tools is unsupported (e.g., instanceof StreamingNotSupportedError or error.isStreamingNotSupported === true); keep supportsTools() gating in place and ensure executeStream implementations can surface the explicit streaming-unsupported signal.src/lib/server/routes/claudeProxyRoutes.ts (1)
3296-3322:⚠️ Potential issue | 🟠 MajorSanitize upstream error bodies before persisting these diagnostics.
These paths still write raw provider response bodies into tracer/log snapshots. Stripping
authorization/x-api-keyheaders and truncating at 4 KB does not prevent echoed secrets, prompts, or other PII from being stored. Please run the body through the logging sanitizer beforetracer.logUpstreamResponseBody()/logProxyBody(), and derivebodySizefrom the sanitized payload instead of the original body.As per coding guidelines "Use
transformParamsForLogging()to safely strip secrets before logging."Also applies to: 4245-4273
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/server/routes/claudeProxyRoutes.ts` around lines 3296 - 3322, The retry path is persisting raw upstream response bodies; run the response body through the existing logging sanitizer (transformParamsForLogging) before calling tracer.logUpstreamResponseBody and before passing body to logProxyBody, replace cappedRetryBody with the sanitized+truncated payload, and compute bodySize from the sanitized string (not retryBody); keep the header redaction intact, and apply the same change for the duplicate block around the other occurrence (lines ~4245-4273) so all upstream response logging uses transformParamsForLogging and derived size from the sanitized output.
🟡 Minor comments (12)
src/lib/processors/base/BaseFileProcessor.ts-151-235 (1)
151-235:⚠️ Potential issue | 🟡 MinorSpan is not marked as errored on failure return paths.
When validation/download/post-validation returns
{ success: false, error }, the callback returns normally, so the span is left with OK/unset status even though the operation failed. Same for the outercatchwhich returns an error object rather than rethrowing. Consumers looking at failed file processing in Langfuse/OTel won't seeERRORstatus here. Consider_span.setStatus({ code: SpanStatusCode.ERROR, message: ... })(and optionally_span.recordException) on the failure branches, matching the error-propagation fixes applied to tool discovery / server routes / memory retrieval in cycle 1.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/processors/base/BaseFileProcessor.ts` around lines 151 - 235, The span created in the anonymous async handler is never marked errored on failure paths; update the failure branches in the handler (where validateFileWithResult, downloadFileWithRetry, validateDownloadedFileWithResult return { success: false, error } and where you return createError(FileErrorCode.*)) to call _span.setStatus({ code: SpanStatusCode.ERROR, message: error?.message || String(error) }) and optionally _span.recordException(error) before returning, and in the catch block set the span status/recordException and then rethrow the error (or re-throw after wrapping) instead of returning the error object so the span accurately reflects failures for validateFileWithResult, downloadFileWithRetry, validateDownloadedFileWithResult, and buildProcessedResultWithResult paths.src/lib/server/routes/memoryRoutes.ts-19-36 (1)
19-36:⚠️ Potential issue | 🟡 MinorSpan stays OK for server-side error responses — propagate error codes to set ERROR status.
Handlers return
createErrorResponse(...)envelopes instead of throwing (e.g.MEMORY_ERRORfrom caught exceptions at lines 155-162, 255-262).tracedMemoryHandlerpasses a callback that doesn't use the span parameter, so the wrapping span is recorded with OK status despite the route returning a 5xx-equivalent error payload. Only thrown exceptions trigger ERROR status inwithSpan(line 21); returned error objects are treated as successful results (line 19).🛠️ Proposed fix
function tracedMemoryHandler<T>( name: string, route: string, fn: (ctx: ServerContext) => Promise<T>, ): (ctx: ServerContext) => Promise<T> { return (ctx: ServerContext) => withSpan( { name, tracer: tracers.http, attributes: { "http.route": route, "http.request.id": ctx.requestId, }, }, - () => fn(ctx), + async (span) => { + const result = await fn(ctx); + // createErrorResponse shape: { error: { code, message, ... } } + const errCode = (result as { error?: { code?: string } } | undefined) + ?.error?.code; + if (errCode && errCode !== "SESSION_NOT_FOUND") { + span.setStatus({ code: SpanStatusCode.ERROR, message: errCode }); + } + return result; + }, ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/server/routes/memoryRoutes.ts` around lines 19 - 36, tracedMemoryHandler currently calls withSpan with a callback that ignores the span so returned error-response envelopes (e.g. createErrorResponse/MEMORY_ERROR) are recorded as OK; modify tracedMemoryHandler to receive the active span from withSpan (or accept a callback signature fn(ctx, span)) and after awaiting fn(ctx) inspect the returned value for your error envelope shape and, when it represents an error, call span.setStatus({ code: SpanStatusCode.ERROR, message: <short message or error.code> }) (import SpanStatusCode from the tracing library) before returning the envelope so non-thrown server errors propagate ERROR status to tracing.src/lib/rag/retrieval/vectorQueryTool.ts-124-131 (1)
124-131:⚠️ Potential issue | 🟡 Minor
topKuses two different fallbacks here.The span records
params.topK ?? topK, but the actual query usesparams.topK || topK. A request withtopK: 0is traced as0and executed with the default, which is both misleading and behaviorally inconsistent.Suggested fix
- topK: params.topK || topK, + topK: params.topK ?? topK,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/rag/retrieval/vectorQueryTool.ts` around lines 124 - 131, The tracing and execution use different fallbacks for topK: the span records params.topK ?? topK but the call to store.query uses params.topK || topK, causing params.topK === 0 to be misrepresented; change the store.query invocation to use the same nullish-coalescing fallback (params.topK ?? topK) so both tracing and execution match (update the topK argument in the store.query call that currently uses params.topK || topK), leaving other parameters (indexName, queryVector/queryEmbedding, filter, includeVectors, providerOptions) unchanged.src/lib/server/routes/openApiRoutes.ts-58-90 (1)
58-90:⚠️ Potential issue | 🟡 MinorThese OpenAPI spans still drop request correlation.
All three handlers ignore
ServerContext, so unlike the other traced routes they cannot attachhttp.request.id./openapi.json,/openapi.yaml, and/docswill therefore show up as uncorrelated spans when you're debugging a specific request.Also applies to: 97-133, 140-177
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/server/routes/openApiRoutes.ts` around lines 58 - 90, The OpenAPI route handlers (the async handler passed to withSpan) ignore ServerContext so they don't attach per-request correlation (http.request.id); update each handler (the handler functions that call withSpan in this file for /openapi.json, /openapi.yaml, and /docs) to accept a ServerContext parameter (e.g., ctx or serverCtx), extract the request correlation id or trace info from it (e.g., ctx.request.id or the request trace header) and either pass it into withSpan as an attribute or set it on the created span (span.setAttribute("http.request.id", ...)); ensure you still call getRoutes, construct OpenAPIGenerator, and return generator.generate() but now include the request id on the span (and if your tracing helper supports a parent context, propagate ctx's trace context into withSpan using tracers.http).src/lib/providers/googleAiStudio.ts-1045-1064 (1)
1045-1064:⚠️ Potential issue | 🟡 MinorStream
generation:endsendscontent: "".Because tokens are pushed through
channelas they arrive, nothing accumulates an aggregate string here, so Langfuse'sGENERATIONobservation will always have empty output on the native Gemini 3 stream path. Consider accumulating the streamed text (or at least the laststepText) and passing it ascontentso Pipeline B has a meaningful response body.🤖 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 1045 - 1064, The stream emitter currently sends an empty content in nativeStreamEmitter.emit("generation:end"); fix by accumulating the streamed pieces into a variable (e.g., accumulatedOutput or finalText) as tokens are pushed to channel (the same place where stepText is produced) and update that variable with each chunk (or at least set it to the latest stepText). Then use that accumulatedOutput when emitting the "generation:end" result.content (alongside existing fields like modelName, providerName, hitStepLimitWithoutFinalAnswer) so the final payload contains the aggregated/last streamed text instead of "".docs/telemetry-gaps-audit.md-460-462 (1)
460-462:⚠️ Potential issue | 🟡 MinorUpdate the verifier path.
Line 462 points to
test/telemetry-gaps-verification.test.ts, but this PR addstest/telemetry-gaps-verification.ts. The audit currently sends readers to a non-existent file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/telemetry-gaps-audit.md` around lines 460 - 462, The documentation points to the wrong verifier filename; update the reference in the "Verification Plan" section from "test/telemetry-gaps-verification.test.ts" to the actual file added "test/telemetry-gaps-verification.ts" so readers are directed to the existing verifier, ensuring the link/text matches the new test filename.docs/telemetry-fix-execution-plan.md-5-5 (1)
5-5:⚠️ Potential issue | 🟡 MinorFix the scheduling math in the plan.
Line 5 says the peak parallelism is 6 agents in Phase 5, but Line 181 caps Phase 5 at 5 and Line 195 puts the 6-agent peak in Phase 6. Line 232 also says there are 9 sequential phases, while the sequence listed there contains substantially more stage hops. This makes the execution plan internally inconsistent.
Also applies to: 181-181, 195-195, 231-232
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/telemetry-fix-execution-plan.md` at line 5, The document's execution plan has inconsistent scheduling numbers: update the "Estimated parallelism" statement and all phase descriptions so Phase 5 peak concurrency matches the actual cap (ensure Phase 5 is consistently capped at 5 agents if that is the intended limit, or change the cap elsewhere to 6 if the intent is a 6-agent peak), move the 6-agent peak to the correct phase (Phase 6) if that is the true peak, and reconcile the total count of phases so the line that currently states "9 sequential phases" matches the actual listed sequence; search for the strings "Estimated parallelism", "Phase 5", "Phase 6", and "9 sequential phases" and make the numbers consistent across the whole document.docs/telemetry-fix-completion-report.md-95-120 (1)
95-120:⚠️ Potential issue | 🟡 MinorCorrect the modified-file count.
Line 95 says there are 17 modified source files, but the list below contains 23 entries. The report should either update the count or trim the list so the summary stays trustworthy.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/telemetry-fix-completion-report.md` around lines 95 - 120, The summary header "### Source files modified (17)" is incorrect compared to the following list of 23 entries; update that header to the correct count "23" (or remove six entries if you intended to keep 17) so the summary matches the list—look for the header line "### Source files modified (17)" in docs/telemetry-fix-completion-report.md and change the numeral to "23" (or trim the excess entries such as any of the listed files like src/lib/providers/anthropic.ts, src/lib/providers/openAI.ts, src/lib/providers/googleAiStudio.ts, src/lib/providers/googleVertex.ts, src/lib/providers/amazonBedrock.ts, src/lib/observability/utils/spanSerializer.ts if you choose to reduce to 17).test/telemetry-gaps-verification.ts-1004-1005 (1)
1004-1005:⚠️ Potential issue | 🟡 MinorUse an explicit fallback for
indexOf()here.Line 1005 is broken because
indexOf()returns-1, which is truthy in JavaScript. When the firstindexOf("execute(")returns-1, the||operator doesn't evaluate the second condition—it just passes through-1. This breaks the intended fallback logic, causing the T8 telemetry gap check to incorrectly returnnulleven whenexecute =patterns exist in the source.Suggested fix
- const altIdx = - source.indexOf("execute(") || source.indexOf("execute ="); + const executeCallIdx = source.indexOf("execute("); + const executeAssignIdx = source.indexOf("execute ="); + const altIdx = + executeCallIdx !== -1 ? executeCallIdx : executeAssignIdx;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/telemetry-gaps-verification.ts` around lines 1004 - 1005, The fallback for finding the "execute" location is wrong because using || with indexOf can propagate -1; update the logic around altIdx so you first call source.indexOf("execute(") into a variable (altIdx) and if altIdx === -1 then set altIdx = source.indexOf("execute ="); ensure you treat -1 as "not found" rather than relying on truthiness so altIdx correctly falls back to the second search when the first returns -1.src/lib/workflow/core/ensembleExecutor.ts-148-150 (1)
148-150:⚠️ Potential issue | 🟡 MinorPropagate degraded ensemble outcomes to the OTel span, not just counts.
Both wrappers now always complete as successful traces unless an exception is thrown. That means
minResponsesfailures and early-stop group failures still look green in OTel/Langfuse even though these functions returnerrors/partial-failure results. Set the span status before returning when the execution outcome is degraded.Also applies to: 499-501
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/workflow/core/ensembleExecutor.ts` around lines 148 - 150, The span currently only records counts (otelSpan.setAttribute("workflow.success_count"..., "workflow.failure_count", "workflow.total_time_ms") ) but never updates span status for degraded outcomes; before returning from the ensemble execution (where successCount, failureCount, minResponses and any returned errors/partial results are known) set otelSpan.setStatus({ code: SpanStatusCode.ERROR, message: "degraded: <reason>" }) for degraded outcomes (e.g., successCount < minResponses, early-stop group failures, or non-empty errors) and otelSpan.setStatus({ code: SpanStatusCode.OK }) for fully successful runs; apply the same fix to the other otelSpan.setAttribute block later in the file (the second occurrence) so traces reflect partial failures instead of always appearing successful.src/lib/auth/tokenStore.ts-375-382 (1)
375-382:⚠️ Potential issue | 🟡 MinorDeduped refresh waiters still miss the new auth outcome attributes.
The new span fields are only written on the fast-path and on the caller that performs the refresh. Callers that hit the existing
inFlightRefreshesbranch return without ever settingauth.expired/auth.refreshed, so the hot concurrent path still produces incomplete telemetry.Suggested fix
const existing = this.inFlightRefreshes.get(provider); if (existing) { logger.debug("Awaiting in-flight refresh for provider", { provider }); + span.setAttribute("auth.expired", true); const result = await existing; + span.setAttribute("auth.refreshed", true); return result.accessToken; }Also applies to: 399-413, 512-513
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/tokenStore.ts` around lines 375 - 382, The issue is that callers who hit the inFlightRefreshes branch return without setting span attributes like "auth.expired" and "auth.refreshed", so add setting of those attributes for all code paths: in _getValidTokenImpl (the function invoked by withSpan) ensure that whenever a token result is resolved—whether from the fast-path, the caller performing refresh (refreshToken / performRefresh logic), or a waiter that awaited an existing inFlightRefreshes promise—you call span.setAttribute for the same auth attributes before returning; update the branch that reads from inFlightRefreshes to extract the refresh outcome and set span.setAttribute("auth.expired", ...)/setAttribute("auth.refreshed", ...) (and any other auth.* attributes used elsewhere) so telemetry is populated for deduped waiters as well, and apply the same change to the equivalent paths referenced around the other ranges (the refresh handling in the token refresh functions at the other locations).src/lib/auth/sessionManager.ts-418-427 (1)
418-427:⚠️ Potential issue | 🟡 MinorRecord the resolved storage backend here, not just the requested config.
createStorage()can fall back from"redis"toMemorySessionStorage, but this span still reportsauth.storagefromthis.config.storage. That makes the new auth telemetry wrong on the fallback path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/sessionManager.ts` around lines 418 - 427, The span currently logs "auth.storage" using this.config.storage which may differ from the actual backend returned by createStorage(); before creating the span for neurolink.auth.session.create, resolve the real storage backend (e.g., call or access the existing resolved storage from createStorage()/this.storage or the method that performs fallback) and set the span attribute "auth.storage" to that resolved value instead of this.config.storage; ensure the change is applied around the withSpan invocation that wraps this._createSession so the span reflects the actual backend used.
🧹 Nitpick comments (10)
src/lib/providers/amazonSagemaker.ts (1)
139-152: Redundant try/catch wrapper.The inner
try { throw ... } catch (error) { throw this.handleProviderError(error); }can be simplified to a directthrow this.handleProviderError(new SageMakerError(...)), since the only thing thrown inside thetryis theSageMakerErrorconstructed on the same line. This also makes it clearer thathandleProviderErroris always invoked on this sentinel error.♻️ Proposed simplification
- async () => { - try { - // For now, throw an error indicating this is not yet implemented - throw new SageMakerError( - "SageMaker streaming not yet fully implemented. Coming in next phase.", - { - code: "MODEL_ERROR", - statusCode: 501, - endpoint: this.modelConfig.endpointName, - }, - ); - } catch (error) { - throw this.handleProviderError(error); - } - }, + async () => { + // Not yet implemented — surface a typed provider error through the span. + throw this.handleProviderError( + new SageMakerError( + "SageMaker streaming not yet fully implemented. Coming in next phase.", + { + code: "MODEL_ERROR", + statusCode: 501, + endpoint: this.modelConfig.endpointName, + }, + ), + ); + },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/providers/amazonSagemaker.ts` around lines 139 - 152, Remove the redundant try/catch and directly throw the handled SageMakerError: replace the try { throw new SageMakerError(...) } catch (error) { throw this.handleProviderError(error) } pattern with a single throw this.handleProviderError(new SageMakerError(...)); ensure you construct the SageMakerError with the same message, code/statusCode and endpoint (this.modelConfig.endpointName) so handleProviderError is invoked deterministically on that sentinel error.src/lib/rag/chunkers/BaseChunker.ts (1)
92-96: Redundant span attributes —content_lengthandcontent_charsare identical.Both are set to
content.length. Drop one (or rename one to represent a distinct dimension, e.g., byte length vs char length) to avoid duplicated cardinality in traces.♻️ Suggested diff
attributes: { "rag.chunker.strategy": this.strategy, "rag.chunker.content_length": content.length, - "rag.chunker.content_chars": content.length, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/rag/chunkers/BaseChunker.ts` around lines 92 - 96, In BaseChunker.ts update the span attributes to avoid duplicate cardinality: remove either "rag.chunker.content_length" or "rag.chunker.content_chars" (whichever is unused elsewhere) or change one to represent a distinct metric (e.g., byte length) by computing Buffer.byteLength(content) and setting that attribute instead; ensure the updated attributes object still includes "rag.chunker.strategy" and a single clear content-size attribute so traces no longer contain identical values for both keys.src/lib/core/modules/StreamHandler.ts (1)
122-124: Sentinel chunk carriesmetadatabut the generator return type doesn't declare it.
createTextStreamis typed asAsyncGenerator<{ content: string }>, so downstream consumers in Pipeline B that need to detectnoOutput/errorTypeto setSpanStatus.WARNINGwill have to cast or rely onas anyto see those fields. Consider widening the return type to include the optional metadata so the sentinel contract is part of the public type surface:♻️ Suggested type widening
- createTextStream(result: { - textStream: AsyncIterable<string>; - }): AsyncGenerator<{ content: string }> { + createTextStream(result: { + textStream: AsyncIterable<string>; + }): AsyncGenerator<{ + content: string; + metadata?: { noOutput?: boolean; errorType?: string }; + }> {Also applies to: 159-167
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/core/modules/StreamHandler.ts` around lines 122 - 124, The generator return type for createTextStream is too narrow (AsyncGenerator<{ content: string }>) and omits the sentinel metadata, so widen the return type to include an optional metadata object (e.g., metadata?: { noOutput?: boolean; errorType?: string; [key: string]: any }) so downstream consumers can read noOutput/errorType without casting; update createTextStream’s AsyncGenerator generic accordingly and apply the same widening to the other generator at the 159-167 block (the related stream factory) so the sentinel contract is part of the public type surface.src/lib/client/sseClient.ts (1)
109-128: Recursive reconnect produces nested spans.
attemptReconnect()(line 698) callsthis.stream(...)again, which executes from inside the parent span's active context. Each reconnect therefore nests a newneurolink.client.sse.streamspan under the previous one. With long-lived sessions and multiple reconnects this yields a deep span tree in Langfuse.Consider either invoking
_streamInternal(...)directly fromattemptReconnect, or starting the reconnect span as a sibling (e.g.,context.with(ROOT_CONTEXT, ...)) so reconnects are peers rather than children.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/client/sseClient.ts` around lines 109 - 128, The current implementation of stream (wrapping _streamInternal in withClientSpan) causes attemptReconnect() to call this.stream(...) and create nested "neurolink.client.sse.stream" spans; update the reconnection path so reconnects are siblings instead of children by either having attemptReconnect call this._streamInternal(path, options, callbacks) directly (bypassing withClientSpan) or by wrapping the reconnection call in a root context (e.g., use context.with(ROOT_CONTEXT, () => this.stream(...))) so each reconnect starts a peer span; locate attemptReconnect, stream, and _streamInternal in sseClient.ts and apply one of these fixes consistently.src/lib/telemetry/traceContext.ts (1)
16-21: Prefer the canonicalisSpanContextValidover a hardcoded zero-trace string.
@opentelemetry/apiexposesisSpanContextValid(spanContext)which checks bothtraceIdandspanIdvalidity per the OTel spec. The current check misses the case wheretraceIdis non-zero butspanIdis the invalid all-zero value — Pipeline B spans would then be linked withparentSpanId: "0000000000000000".♻️ Suggested change
-import { trace, context } from "@opentelemetry/api"; +import { trace, context, isSpanContextValid } from "@opentelemetry/api"; @@ const activeSpan = trace.getSpan(context.active()); if (!activeSpan) { return {}; } const ctx = activeSpan.spanContext(); - // Invalid trace IDs are all zeros — don't use those - if (!ctx.traceId || ctx.traceId === "00000000000000000000000000000000") { + if (!isSpanContextValid(ctx)) { return {}; } return { traceId: ctx.traceId, parentSpanId: ctx.spanId };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/telemetry/traceContext.ts` around lines 16 - 21, Replace the manual zero-check on activeSpan.spanContext() with the canonical isSpanContextValid check from `@opentelemetry/api`: import isSpanContextValid, call isSpanContextValid(ctx) on the ctx returned by activeSpan.spanContext(), and only return { traceId: ctx.traceId, parentSpanId: ctx.spanId } when isSpanContextValid returns true; otherwise return {} — this ensures both traceId and spanId are validated (preventing parentSpanId from being the all-zero span).src/lib/providers/azureOpenai.ts (1)
182-190: Duplicated inline type cast fortoolResultsacross providers.The same
as Array<{ toolName: string; output?: unknown; result?: unknown; error?: string }>cast is repeated in 7 provideronStepFinishhandlers in this PR. Consider moving the type handling intoemitToolEndFromStepFinish's parameter type (e.g., accept the AI SDK's native step-finishtoolResultstype, orunknown[]and narrow internally) so each call site reduces toemitToolEndFromStepFinish(this.neurolink?.getEventEmitter(), event.toolResults).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/providers/azureOpenai.ts` around lines 182 - 190, The repeated inline cast of event.toolResults should be removed by changing emitToolEndFromStepFinish’s parameter type to accept the provider SDK’s native toolResults type (or simply unknown[]), then perform proper internal narrowing/validation inside emitToolEndFromStepFinish; update calls like emitToolEndFromStepFinish(this.neurolink?.getEventEmitter(), event.toolResults) (remove the as Array<...> cast) and keep any runtime guards inside emitToolEndFromStepFinish to handle optional neurolink and shape-check tool result entries (referencing emitToolEndFromStepFinish, neurolink?.getEventEmitter(), and event.toolResults).src/lib/server/routes/toolRoutes.ts (1)
339-358: Minor: GET/tools/:namedoesn't mark span ERROR when tool is missing.The execute handlers flip
tool.found=false+setStatus(ERROR)for the same condition, but the GET handler returnscreateErrorResponse(...)silently. Consider mirroring the ERROR status for trace-level parity.- async () => { + async (span) => { const tools = await ctx.toolRegistry.listTools(); const tool = tools.find((t) => t.name === name); if (!tool) { + span.setAttribute("tool.found", false); + span.setStatus({ + code: SpanStatusCode.ERROR, + message: `Tool '${name}' not found`, + }); return createErrorResponse( "TOOL_NOT_FOUND", `Tool '${name}' not found`, undefined, ctx.requestId, ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/server/routes/toolRoutes.ts` around lines 339 - 358, The GET /tools/:name handler returns createErrorResponse when a tool isn't found but does not mark the trace span as error; update the async handler (the function that calls ctx.toolRegistry.listTools and returns createErrorResponse) to mirror the execute handlers by calling ctx.span.setAttribute('tool.found', false) and ctx.span.setStatus({ code: SpanStatusCode.ERROR }) (or your project's equivalent) immediately before returning createErrorResponse so the span/error state matches other handlers.docs/telemetry-baseline-proof.txt (1)
98-114: Optional: label baseline artifact explicitly to avoid confusion with the post-fix report.The TYPECHECK/LINT sections intentionally capture a pre-fix failure state (
ERR_MODULE_NOT_FOUND, prettier warnings,ELIFECYCLEexit 1), which can be mistaken for a regression when diffed against the completion report. A one-line header like# Pre-fix baseline — captured before telemetry remediation. Failures below are expected.at the top would make the intent unambiguous.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/telemetry-baseline-proof.txt` around lines 98 - 114, Add an explicit one-line header at the top of the document clarifying this is the pre-fix baseline (for example: "Pre-fix baseline — captured before telemetry remediation. Failures below are expected.") so readers won't confuse the TYPECHECK/LINT block (the "TYPECHECK/LINT" sections showing ERR_MODULE_NOT_FOUND, prettier warnings, ELIFECYCLE) with a post-fix regression; update the existing docs/telemetry-baseline-proof.txt content to prepend that single clarifying line.src/lib/utils/toolEndEmitter.ts (1)
82-91: Optional: allow callers to pass a per-resultresponseTimeinstead of hardcoding 0.Pipeline B will currently report zero duration for every AI-SDK-driven tool call emitted through this helper, which loses a useful metric. The AI SDK doesn't expose per-tool timing in
onStepFinish, but callers that measure elapsed time around the SDK call could thread it through.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/utils/toolEndEmitter.ts` around lines 82 - 91, The emitter currently hardcodes responseTime: 0 when calling createToolEventPayload in toolEndEmitter.ts; change the relevant function (the helper that calls emitter.emit with tr.toolName and createToolEventPayload) to accept an optional responseTime parameter (defaulting to 0) and pass that value instead of 0 into createToolEventPayload; update any callers (e.g., places using the helper from onStepFinish or other SDK wrappers) to forward measured durations when available so per-call timing is preserved.docs/telemetry-fix-plan.md (1)
1-6: Add a “status” banner so this plan isn’t mistaken for pending work.Given this PR appears to have implemented the phases, add a short header note like “Historical execution plan for PR
#962; see completion report for final state.” This will prevent operational confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/telemetry-fix-plan.md` around lines 1 - 6, Add a short status banner at the top of the NeuroLink Telemetry Fix Plan document to mark it as historical/complete (for example: "Historical execution plan for PR `#962`; see completion report for final state.") so readers don’t treat it as pending work; insert this one-line header directly under the existing title "NeuroLink Telemetry Fix Plan" (or at the very top) and ensure it is clearly labeled as a historical/completed plan.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 81bd77e5-328e-4a61-ab9c-013057e2c90e
📒 Files selected for processing (69)
docs/telemetry-after-proof.txtdocs/telemetry-baseline-proof.txtdocs/telemetry-fix-completion-report.mddocs/telemetry-fix-diff.txtdocs/telemetry-fix-execution-plan.mddocs/telemetry-fix-plan.mddocs/telemetry-gaps-audit.mdsrc/cli/loop/session.tssrc/lib/auth/anthropicOAuth.tssrc/lib/auth/sessionManager.tssrc/lib/auth/tokenStore.tssrc/lib/client/httpClient.tssrc/lib/client/sseClient.tssrc/lib/client/streamingClient.tssrc/lib/client/wsClient.tssrc/lib/context/budgetChecker.tssrc/lib/context/contextCompactor.tssrc/lib/context/summarizationEngine.tssrc/lib/core/baseProvider.tssrc/lib/core/modules/GenerationHandler.tssrc/lib/core/modules/StreamHandler.tssrc/lib/core/modules/ToolsManager.tssrc/lib/evaluation/ragasEvaluator.tssrc/lib/mcp/batching/requestBatcher.tssrc/lib/mcp/httpRateLimiter.tssrc/lib/mcp/httpRetryHandler.tssrc/lib/mcp/mcpClientFactory.tssrc/lib/mcp/toolDiscoveryService.tssrc/lib/mcp/toolRegistry.tssrc/lib/memory/memoryRetrievalTools.tssrc/lib/neurolink.tssrc/lib/observability/utils/spanSerializer.tssrc/lib/processors/base/BaseFileProcessor.tssrc/lib/processors/registry/ProcessorRegistry.tssrc/lib/providers/amazonBedrock.tssrc/lib/providers/amazonSagemaker.tssrc/lib/providers/anthropic.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/rag/chunkers/BaseChunker.tssrc/lib/rag/ragIntegration.tssrc/lib/rag/reranker/reranker.tssrc/lib/rag/retrieval/vectorQueryTool.tssrc/lib/server/middleware/common.tssrc/lib/server/routes/agentRoutes.tssrc/lib/server/routes/claudeProxyRoutes.tssrc/lib/server/routes/healthRoutes.tssrc/lib/server/routes/mcpRoutes.tssrc/lib/server/routes/memoryRoutes.tssrc/lib/server/routes/openApiRoutes.tssrc/lib/server/routes/toolRoutes.tssrc/lib/services/server/ai/observability/instrumentation.tssrc/lib/telemetry/traceContext.tssrc/lib/telemetry/tracers.tssrc/lib/types/span.tssrc/lib/utils/toolEndEmitter.tssrc/lib/workflow/core/ensembleExecutor.tssrc/lib/workflow/core/judgeScorer.tssrc/lib/workflow/core/responseConditioner.tssrc/lib/workflow/core/workflowRunner.tstest/telemetry-gaps-verification.ts
| onEnd(span: ReadableSpan): void { | ||
| // ... existing logic ... | ||
|
|
||
| // Propagate error info to Langfuse-consumable attributes | ||
| if (span.status.code === SpanStatusCode.ERROR) { | ||
| span.attributes["langfuse.level"] = "ERROR"; | ||
| if (span.status.message) { | ||
| span.attributes["langfuse.status_message"] = span.status.message; | ||
| } | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| **Note**: `ReadableSpan` is read-only by spec. `LangfuseSpanProcessor` may or may not read these attributes. Test whether `@langfuse/otel` respects custom attributes for level/statusMessage. If not, this fix has no effect and should be replaced with a custom span processor that wraps `LangfuseSpanProcessor`. | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In OpenTelemetry JS, can a SpanProcessor safely mutate ReadableSpan.attributesinsideonEnd? Please cite the official API/docs for ReadableSpanandSpanProcessor.
💡 Result:
No, a SpanProcessor cannot safely mutate ReadableSpan.attributes inside onEnd in OpenTelemetry JS. The official API documentation shows that SpanProcessor.onEnd(span: ReadableSpan): void receives a ReadableSpan [1,20]. The ReadableSpan interface defines attributes as a readonly property (Readonly attributes: Attributes) with no mutation methods like setAttribute [7]. In contrast, onStart receives a mutable Span [1,20], and the experimental onEnding(span: Span): void also receives a mutable Span for modifications before finalization [20,22]. The OpenTelemetry specification confirms onEnd receives a readable span where modification is not allowed, while recent updates (PR #4024 merged 2024) introduced onEnding specifically for safe mutations on end [5,16,22]. Attempting to mutate ReadableSpan.attributes directly (e.g., span.attributes['key'] = value) would violate the readonly contract and is unsafe/undefined behavior, as attributes provides read access only.
Citations:
- 1: https://open-telemetry.github.io/opentelemetry-js/interfaces/_opentelemetry_sdk-trace-node.SpanProcessor.html
- 2: https://opentelemetry.io/docs/specs/otel/trace/sdk/
- 3: https://open-telemetry.github.io/opentelemetry-js/interfaces/_opentelemetry_sdk_trace_base.ReadableSpan.html
- 4: Add ability for SpanProcessor to mutate spans on end open-telemetry/opentelemetry-specification#4024
- 5: https://open-telemetry.github.io/opentelemetry-js/interfaces/_opentelemetry_sdk-trace-base.SpanProcessor.html
- 6: [sdk-trace-*] implement experimental
SpanProcessor#onEnding(Span)open-telemetry/opentelemetry-js#5836
ReadableSpan mutation in onEnd violates the readonly contract—use onEnding instead.
Per OpenTelemetry JS API documentation, SpanProcessor.onEnd(span: ReadableSpan) receives a read-only span with a readonly attributes property that cannot be safely mutated. The attempt to set span.attributes["..."] = ... would violate this contract and result in undefined behavior.
Instead, use the onEnding(span: Span): void method (experimental, available after PR #4024), which receives a mutable Span and permits attribute mutation before span finalization. Revise the plan to clarify this distinction and update the implementation snippet accordingly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/telemetry-fix-plan.md` around lines 861 - 875, The plan mutates a
ReadableSpan in onEnd which violates the readonly contract; update to use the
mutable onEnding(span: Span) hook instead. Change any logic in
LangfuseSpanProcessor that currently reads/writes attributes in onEnd to
implement onEnding(span: Span): void and set span.setAttribute or mutate
span.attributes there (referencing the Span type and onEnding method), and keep
onEnd only for read-only post-finalization handling; if onEnding is unavailable,
implement a custom wrapper span processor that intercepts Span before
finalization and augments attributes for Langfuse prior to delegating to
LangfuseSpanProcessor.
ce38e9b to
d3ac082
Compare
Review Feedback Addressed (Cycle 2)Changes MadeAll 13 comments from CodeRabbit's second review have been analyzed. 11 fixed, 2 addressed as doc-only.
Files Modified
Validation
Requesting Re-review@coderabbitai — All 13 comments from the second review have been addressed. Please re-review. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
Triggering the full re-review now. ✅ Actions performedFull review triggered. |
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
src/lib/auth/tokenStore.ts (1)
435-440:⚠️ Potential issue | 🟡 MinorSpan loses
auth.refreshedattribute on the in-flight dedup path.When a concurrent caller awaits an existing in-flight refresh, the token is effectively refreshed for this call, but the span never records
auth.refreshed. Only the leading caller sets it on line 512. Set the attribute here too so Langfuse shows consistent refresh accounting across concurrent requests.🛠️ Suggested change
const existing = this.inFlightRefreshes.get(provider); if (existing) { logger.debug("Awaiting in-flight refresh for provider", { provider }); const result = await existing; + span.setAttribute("auth.refreshed", true); return result.accessToken; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auth/tokenStore.ts` around lines 435 - 440, Concurrent callers that await an existing in-flight refresh via this.inFlightRefreshes.get(provider) never set the tracing attribute auth.refreshed, so spans for those callers miss the refresh marker; after awaiting the existing Promise (the variable existing -> result), set the same span attribute used by the leading caller (e.g., span.setAttribute('auth.refreshed', true) or the equivalent tracing helper used elsewhere in this file) before returning result.accessToken so concurrent paths record the refresh consistently.src/lib/core/modules/StreamHandler.ts (1)
122-184:⚠️ Potential issue | 🟠 MajorReturn type hides the sentinel
metadata; Pipeline B cannot access it type-safely.
createTextStreamdeclaresAsyncGenerator<{ content: string }>, but lines 161–167 emit{ content: "", metadata: { noOutput, errorType } }when the stream produces no output. Downstream code iterates the stream viafor await (const chunk of result.stream)and sees onlychunk.content, making the sentinel metadata inaccessible despite being emitted. The type contract must be widened so Pipeline B can read thenoOutputflag to set appropriate span status.Widen the generator chunk type to expose optional metadata
- createTextStream(result: { - textStream: AsyncIterable<string>; - }): AsyncGenerator<{ content: string }> { + createTextStream(result: { + textStream: AsyncIterable<string>; + }): AsyncGenerator<{ + content: string; + metadata?: { noOutput?: boolean; errorType?: string }; + }> {If a shared stream-chunk type already exists in
src/lib/types/, prefer reusing or extending it instead of the inline shape.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/core/modules/StreamHandler.ts` around lines 122 - 184, createTextStream currently types its return as AsyncGenerator<{ content: string }>, but it yields a sentinel chunk with metadata (noOutput, errorType) which downstream Pipeline B cannot access type-safely; widen the generator item type (or reuse/extend a shared stream-chunk type in src/lib/types) to include an optional metadata property (e.g., metadata?: { noOutput?: boolean; errorType?: string }) so consumers iterating the stream (for await ... of result.stream) can read chunk.metadata.noOutput; update the createTextStream signature and any affected callers/types to the new shared/extended chunk type.src/lib/types/span.ts (1)
50-55:⚠️ Potential issue | 🟠 MajorFix WARNING status handling across observability consumers.
The
SpanStatus.WARNINGenum extension introduces real bugs: otelBridge maps WARNING to OTel OK status (line 149), metricsAggregator counts WARNING as success (line 102), posthogExporter and laminarExporter default WARNING to "unset" (no case in switch), datadogExporter loses the signal (maps to "info"), and continuous-test-suite-workflow assertions exclude WARNING entirely. While spanSerializer correctly maps WARNING → "WARNING" in Langfuse format, downstream consumers must handle all three statuses:
- otelBridge.ts:149 — Update ternary to handle WARNING (map to OTel ERROR or introduce new OTel code)
- metricsAggregator.ts:102, 493 — Use enum comparisons instead of hardcoded
2or1; define WARNING aggregation logic- posthogExporter.ts, laminarExporter.ts — Add
case SpanStatus.WARNING:handlers- datadogExporter.ts:186 — Map WARNING to appropriate Datadog status
- samplers.ts:160 — Consider if WARNING should be sampled with error-priority
- continuous-test-suite-workflow.ts:1874 — Include
SpanStatus.WARNINGin final status assertions🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/types/span.ts` around lines 50 - 55, The new SpanStatus.WARNING enum value is not handled consistently across consumers; update each consumer to explicitly handle SpanStatus.WARNING rather than relying on numeric literals or default branches: in otelBridge (function handling OTel mapping) change the ternary at otelBridge.ts:149 to explicitly check for SpanStatus.WARNING and map it to the correct OTel status (or introduce a new mapping), in metricsAggregator (symbols referencing aggregation logic at metricsAggregator.ts:102 and :493) replace hardcoded numeric checks (e.g., `=== 1`/`=== 2`) with enum comparisons against SpanStatus.OK/ERROR/WARNING and implement aggregation rules for WARNING, add a `case SpanStatus.WARNING:` branch in both posthogExporter and laminarExporter switch handlers, update datadogExporter mapping (around the handler at datadogExporter.ts:186) to map WARNING to the appropriate Datadog status, review samplers (samplers.ts around line 160) to decide whether WARNING should be sampled with error-priority and adjust logic accordingly, and include SpanStatus.WARNING in the final status assertions in continuous-test-suite-workflow (around 1874) so tests account for WARNING outcomes.src/lib/workflow/core/ensembleExecutor.ts (1)
139-158:⚠️ Potential issue | 🟡 MinorOuter OTel span stays OK when all ensemble models fail.
The Pipeline B
SpanSerializerspan is set toSpanStatus.ERRORwhensuccessCount === 0, but the outer OTel span fromwithSpan(...)only receives success/failure count attributes and nosetStatus({ code: ERROR }). Pipeline A/B will disagree in Langfuse for fully failed ensembles. Consider mirroring the ERROR status ontootelSpanhere (and similarly at the tail ofexecuteModelGroupsInneron lines 499–501 whentotalSuccessCount === 0).🛠️ Suggested fix
+ if (successCount === 0) { + otelSpan.setStatus({ + code: (await import("@opentelemetry/api")).SpanStatusCode.ERROR, + message: "No successful model responses", + }); + } otelSpan.setAttribute("workflow.success_count", successCount); otelSpan.setAttribute("workflow.failure_count", failureCount); otelSpan.setAttribute("workflow.total_time_ms", totalTime);(Prefer a static top-level
import { SpanStatusCode } from "@opentelemetry/api"instead of the inline dynamic import.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/workflow/core/ensembleExecutor.ts` around lines 139 - 158, The outer OpenTelemetry span (otelSpan) is not marked ERROR when all ensemble models fail, causing mismatch with the SpanSerializer span; update the end-of-ensemble logic (around SpanSerializer.endSpan and where successCount is computed) to call otelSpan.setStatus({ code: SpanStatusCode.ERROR }) when successCount === 0 (and mirror the same change at the end of executeModelGroupsInner when totalSuccessCount === 0), and add a top-level import for SpanStatusCode from "@opentelemetry/api".src/lib/providers/amazonBedrock.ts (2)
1627-1697:⚠️ Potential issue | 🟡 Minor
analyticsPromisecan hang forever if the returned stream is never consumed.
resolveAnalyticsonly fires from the async iterator'sfinally. If a caller destructuresanalyticsbut abandonsstream(early error handling, cancelled request, conditional flow), awaitinganalyticswill never settle — and nogeneration:endis emitted either.Consider either (a) resolving
analyticsPromisefrom the outerstreamingConversationLoopscope once the ReadableStream closes/errors (independent of iteration), or (b) attaching a safetyqueueMicrotask/timeout that resolves with a best-effort analytics snapshot if the iterable is garbage-collected without being consumed. Low-impact for happy-path consumers, but worth tightening becauseanalyticsPromiseis exposed onStreamResult.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/providers/amazonBedrock.ts` around lines 1627 - 1697, analyticsPromise can hang because resolveAnalytics is only called inside wrappedStreamIterable's finally which never runs if the caller never iterates stream; update the code so resolveAnalytics is guaranteed to settle regardless of iteration by adding an outer safety resolver: after creating analyticsPromise/resolveAnalytics, schedule a fallback resolution via queueMicrotask (or a short setTimeout) that checks a consumed flag and, if still false, calls resolveAnalytics(createAnalytics(...)) with best-effort aggregatedUsage and streamingMode true; also mark the flag as true inside wrappedStreamIterable's async iterator before yielding (or in its finally) so the fallback no-ops when the stream is consumed, and keep emitting generation:end via streamEmitter from both the iterator finally and the fallback path to preserve telemetry.
1229-1347:⚠️ Potential issue | 🟠 Major
executeStreamrethrows without emitting a failuregeneration:endon non-permission errors (and on the "null result" fallback throw).Two paths in this catch block skip Pipeline B entirely:
- Non-permission error (Line 1338-1347): when
streamingConversationLoopthrows synchronously (e.g.prepareStreamCommandfailure,bedrockClient.sendtimeout, non-AccessDeniedExceptionerror), thewrappedStreamIterableis never created, itsfinallynever runs, and the catch just ends the span and rethrows. Nogeneration:endis emitted, so Langfuse has no terminal event for the failed stream.- Fallback null result (Line 1275-1283): when
generate()returnsnull, we throw without emitting either —generate()did not emit a failure event in this "success-but-null" case, and nothing downstream will.Mirror the stream-path
finallyhere (and the generate path you already added) so Pipeline B observes failed streams.🛠️ Suggested fix
+ const startTime = Date.now(); logger.debug( "🟢 [TRACE] executeStream TRY block - about to call streamingConversationLoop", ); @@ if (!generateResult) { + this.neurolink?.getEventEmitter()?.emit("generation:end", { + provider: this.providerName, + responseTime: Date.now() - startTime, + timestamp: Date.now(), + result: { + content: "", + usage: { input: 0, output: 0, total: 0 }, + model: this.modelName || this.getDefaultModel(), + provider: this.providerName, + finishReason: "error", + }, + success: false, + error: "Generate method returned null result", + }); streamSpan.setStatus({ code: SpanStatusCode.ERROR, message: "Generate method returned null result", }); @@ } catch (error: unknown) { @@ // Re-throw non-permission errors + this.neurolink?.getEventEmitter()?.emit("generation:end", { + provider: this.providerName, + responseTime: Date.now() - startTime, + timestamp: Date.now(), + result: { + content: "", + usage: { input: 0, output: 0, total: 0 }, + model: this.modelName || this.getDefaultModel(), + provider: this.providerName, + finishReason: "error", + }, + success: false, + error: errorObj instanceof Error ? errorObj.message : String(errorObj), + }); streamSpan.setStatus({Based on learnings "Native providers (amazonBedrock.ts, …) emit their own
generation:endevents WITHOUT this flag. … allowing native provider events through to Pipeline B."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/providers/amazonBedrock.ts` around lines 1229 - 1347, The catch in executeStream currently rethrows non-permission errors and the "generate returned null" error without emitting the Pipeline B terminal event; update executeStream (the catch block around streamingConversationLoop and the null-result branch after calling this.generate) to mirror the streaming path's wrappedStreamIterable finally: before rethrowing, emit a generation:end (or equivalent) event including failure metadata/reason and ensure streamSpan records the error and ends, so Pipeline B receives a terminal event for both non-permission errors and the fallback-null case (refer to the wrappedStreamIterable finally logic, streamingConversationLoop, streamSpan.setStatus/recordException/end, and the generate null-result branch).src/lib/neurolink.ts (2)
3568-3584:⚠️ Potential issue | 🟠 MajorRecord the generate exception on the OTel span.
This catch block sets
ERRORstatus but never callsgenerateSpan.recordException(error). Failedgenerate()traces will miss the exception event even though the stream path records it.Suggested fix
} catch (error) { generateSpan.setStatus({ code: SpanStatusCode.ERROR, message: error instanceof Error ? error.message : String(error), }); + if (error instanceof Error) { + generateSpan.recordException(error); + } // G7 fix: Distinguish context overflow errors with dedicated attributes if (error instanceof ContextBudgetExceededError) { generateSpan.setAttribute("neurolink.error.type", "context_overflow");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/neurolink.ts` around lines 3568 - 3584, The catch block that handles failures from generate() currently sets SpanStatusCode.ERROR but doesn't record the exception; add a call to generateSpan.recordException(error) inside that catch block (near where generateSpan.setStatus is called) so the exception event is attached to the OTel span; keep the existing ContextBudgetExceededError attribute logic (generateSpan.setAttribute(...)) and call recordException before or immediately after setStatus to ensure the thrown error is captured on the span.
7665-7705:⚠️ Potential issue | 🟠 MajorEmit the fallback stream’s real completion metadata.
This completion event hard-codes
finishReason: "stop"andchunkCount: 0, and it omitsusage. If the fallback stream ends withlength/content-filteror produces tokens/chunks, Pipeline B will record the wrong finish reason, chunk count, and cost data.Suggested fix
- let fallbackAccumulatedContent = ""; + let fallbackAccumulatedContent = ""; + let fallbackChunkCount = 0; const fallbackProcessedStream = (async function* (self: NeuroLink) { try { for await (const chunk of fallbackStreamResult.stream) { + fallbackChunkCount++; if ( chunk && "content" in chunk && typeof chunk.content === "string" ) { @@ logger.info( `[NeuroLink.handleStreamError] stream() - COMPLETE SUCCESS (fallback)`, { provider: providerName, model: options.model, responseTimeMs: Date.now() - startTime, contentLength: fallbackAccumulatedContent.length, }, ); // S6 fix: Emit stream:complete after successful fallback so Pipeline B records it try { + const fallbackFinishReason = + fallbackStreamResult.finishReason ?? "stop"; self.emitter.emit("stream:complete", { content: fallbackAccumulatedContent, provider: providerName, model: options.model || "unknown", - finishReason: "stop", + finishReason: fallbackFinishReason, metadata: { durationMs: Date.now() - startTime, - chunkCount: 0, + chunkCount: fallbackChunkCount, totalLength: fallbackAccumulatedContent.length, isFallback: true, - finishReason: "stop", + finishReason: fallbackFinishReason, + usage: fallbackStreamResult.usage, }, }); } catch { /* non-blocking */ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/neurolink.ts` around lines 7665 - 7705, The stream-complete event inside the fallbackProcessedStream generator is emitting hard-coded metadata (finishReason: "stop", chunkCount: 0, no usage) which misreports fallback results; update the logic in fallbackProcessedStream (used on NeuroLink) to compute real completion metadata from fallbackStreamResult and the accumulated chunks: derive finishReason from fallbackStreamResult (or the final chunk/response object), count emitted chunks as chunkCount, include any usage/cost info present on fallbackStreamResult or its final chunk, set totalLength from fallbackAccumulatedContent.length, and include isFallback: true and accurate durationMs using startTime before emitting self.emitter.emit("stream:complete") so Pipeline B receives correct finishReason, chunkCount, and usage data.
♻️ Duplicate comments (11)
src/lib/providers/googleVertex.ts (1)
2200-2225:⚠️ Potential issue | 🟠 MajorEmit the streamed text in the native Vertex
generation:endevent.This manual completion event still sends
result.content: "", so downstream telemetry records a successful generation with no output text. Capture the final streamed text buffer and emit that here instead of an empty string.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/providers/googleVertex.ts` around lines 2200 - 2225, The manual Vertex event emits result.content as an empty string causing telemetry to miss the actual output; locate where the stream is accumulated (the final streamed text buffer used during streaming) and replace result.content: "" in the vertexStreamEmitter.emit("generation:end", ...) call with the accumulated final text (e.g., the concatenated/trimmed stream buffer variable used elsewhere in this module). Ensure you reference vertexStreamEmitter and the same final-stream variable so the emitted payload includes the real output text and still preserves usage, model, provider, finishReason, and success fields.docs/telemetry-baseline-proof.txt (1)
98-114:⚠️ Potential issue | 🟡 MinorSanitize the captured proof output before committing it.
This block still contains a machine-specific absolute path and a local module-resolution trace. That makes the artifact non-reproducible and leaks workstation details into the repo.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/telemetry-baseline-proof.txt` around lines 98 - 114, The captured proof in docs/telemetry-baseline-proof.txt includes a machine-specific absolute path (e.g. "/Users/sachinsharma/Developer/temp/neurolink-fork/fix/proxy-bug-fixes") and an internal module-resolution trace ("node:internal/modules/esm/module_job") which must be sanitized; edit docs/telemetry-baseline-proof.txt to remove or replace any absolute paths and internal stack traces with nondisclosing placeholders (e.g. <REPO_ROOT_PATH> and <MODULE_STACK_TRACE>), ensure the output remains representative, then run Prettier (--write) and re-run the linter to confirm no formatting or lint warnings remain.src/lib/server/routes/agentRoutes.ts (1)
57-65:⚠️ Potential issue | 🟠 MajorUse
basePathwhen settinghttp.route.All four spans still hardcode
/api/..., so deployments mounted under a different base path will emit the wrong route label and split telemetry. Build the route attribute frombasePathinstead of a literal.Also applies to: 139-147, 229-237, 292-301
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/server/routes/agentRoutes.ts` around lines 57 - 65, The spans set a hardcoded "http.route" (e.g., in the withSpan call for name "neurolink.http.execute") which breaks telemetry for deployments with non-root mounts; change the attribute to build the route using the existing basePath variable plus the endpoint path (e.g., `${basePath}/api/agent/execute`), normalizing slashes so you don't end up with double or missing slashes; apply the same change to the other withSpan usages that set "http.route" (the ones for the other API endpoints in this file) so all spans use basePath-derived routes instead of literals.src/lib/rag/reranker/reranker.ts (1)
69-85:⚠️ Potential issue | 🟠 MajorDon't default reranker weights with
||.Using
||rewrites explicit0weights back to the defaults, so callers cannot intentionally disable a factor, and an all-zero config still leaves you normalizing an invalid total. Use??here and rejecttotalWeight <= 0before dividing.Also applies to: 227-237
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/rag/reranker/reranker.ts` around lines 69 - 85, The code uses || when falling back to DEFAULT_WEIGHTS which overrides explicit 0 values and allows an all-zero sum; change the fallback for weights.semantic, weights.vector, and weights.position to use the nullish coalescing operator (??) instead of || in the totalWeight calculation and when building normalizedWeights (references: weights, DEFAULT_WEIGHTS, normalizedWeights), and add a guard that rejects or throws (or returns an error) if totalWeight <= 0 before performing the division; apply the same ?? + validation change to the other occurrence around the reranker (the block referenced at 227-237).src/lib/server/routes/toolRoutes.ts (1)
74-76:⚠️ Potential issue | 🟠 MajorNormalize
q,source, andlimitbefore using them.These values can still arrive as arrays.
q.toLowerCase()will throw for requests like?q=a&q=b, andparseInt(limit, 10)on a non-scalar can silently skew the slice. Thetools.search.queryattribute has the same problem because it castsctx.query.qtostringbefore normalization.Suggested normalization
handler: async (ctx: ServerContext) => withSpan( { name: "neurolink.http.tools.search", tracer: tracers.http, attributes: { "http.route": `${basePath}/tools/search`, - "tools.search.query": (ctx.query.q as string) ?? "", }, }, - async () => { - const { q, source, limit } = ctx.query; + async (span) => { + const rawQ = Array.isArray(ctx.query.q) + ? ctx.query.q[0] + : ctx.query.q; + const rawSource = Array.isArray(ctx.query.source) + ? ctx.query.source[0] + : ctx.query.source; + const rawLimit = Array.isArray(ctx.query.limit) + ? ctx.query.limit[0] + : ctx.query.limit; + const q = typeof rawQ === "string" ? rawQ : ""; + const source = + typeof rawSource === "string" ? rawSource : undefined; + const parsedLimit = + typeof rawLimit === "string" + ? parseInt(rawLimit, 10) + : Number(rawLimit); + const maxResults = + Number.isFinite(parsedLimit) && parsedLimit > 0 + ? parsedLimit + : 50; + span.setAttribute("tools.search.query", q); const tools = await ctx.toolRegistry.listTools(); let filtered = tools; @@ - const maxResults = limit ? parseInt(limit, 10) : 50; filtered = filtered.slice(0, maxResults);Also applies to: 80-105
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/server/routes/toolRoutes.ts` around lines 74 - 76, Normalize request query params before using or logging them: read ctx.query into local scalars (e.g., const qRaw = ctx.query.q, sourceRaw = ctx.query.source, limitRaw = ctx.query.limit), coerce arrays to their first element (if Array.isArray(...)) and then set q = String(qRaw || "").toLowerCase(), source = String(sourceRaw || "").toLowerCase() (or default), and limit = Number.isNaN(parseInt(String(limitRaw || "0"), 10)) ? defaultLimit : parseInt(String(limitRaw), 10). Replace direct uses of (ctx.query.q as string), ctx.query.source, and ctx.query.limit in the attributes block and in the handlers (references around tools.search.query and the logic between lines 80-105) with these normalized variables so toLowerCase and parseInt are applied to scalar strings only.src/lib/core/baseProvider.ts (1)
261-293:⚠️ Potential issue | 🔴 CriticalFallback is still too broad for real-stream failures.
This still retries with fake streaming for any non-terminal
executeStream()error when tools are enabled. A 404/500 or provider-side validation failure will now issue a second upstream request, hide the original failure mode, and potentially double spend. The fallback needs a positive “streaming-with-tools unsupported” signal instead of a negative allowlist of terminal errors.Based on learnings,
src/lib/core/baseProvider.tsis the base class all providers extend. Centralstream()method merges tools before calling provider-specificexecuteStream().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/core/baseProvider.ts` around lines 261 - 293, The current catch in stream() indiscriminately falls back to executeFakeStreaming() for any non-terminal error; change it to only fallback when the provider explicitly signals "streaming-with-tools unsupported" (e.g., by throwing or returning a well-known sentinel error/object), and otherwise propagate via handleProviderError(realStreamError). Concretely, update the catch in baseProvider.stream/executeStream handling to detect a positive signal (unique symbol/class/name such as StreamingWithToolsUnsupportedError or error.code === "STREAMING_TOOLS_UNSUPPORTED") before calling executeFakeStreaming(options, analysisSchema); for all other errors (including 404/500/validation), call throw this.handleProviderError(realStreamError) and log the original error and providerName. Ensure providers are updated to throw that sentinel when they cannot stream with tools so the fallback is only used when explicitly supported.src/lib/memory/memoryRetrievalTools.ts (1)
248-255:⚠️ Potential issue | 🟠 MajorMark these not-found returns as failed on the outer OTel span.
Session not foundandMessage not foundstill return normally here. Because the surroundingwithSpan(...)only sees a fulfilled callback,neurolink.memory.retrieve_contextis recorded as success even though the tool returned an error payload. SetotelSpantoERRORbefore each early return, or throw a typed error and let the wrapper classify it.Also applies to: 264-270
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/memory/memoryRetrievalTools.ts` around lines 248 - 255, The early-return paths in memoryRetrievalTools.ts (e.g., the "Session not found" branch that calls SpanSerializer.endSpan and getMetricsAggregator().recordSpan) are currently leaving the surrounding withSpan/otel span as successful; update both the "Session not found" and the "Message not found" branches (the blocks around the SpanSerializer.endSpan calls) to mark the outer OTel span as ERROR before returning—either by setting the otelSpan status to ERROR or by throwing a typed error that the withSpan wrapper will catch—so neurolink.memory.retrieve_context is recorded as failed when these not-found cases occur. Ensure you reference and update the SpanSerializer.endSpan/SpanStatus usage and the getMetricsAggregator().recordSpan calls in those branches.src/lib/server/routes/claudeProxyRoutes.ts (2)
4864-4881:⚠️ Potential issue | 🟠 MajorReject malformed message entries before calling
.map().
Array.isArray(body.messages)still accepts[null],["x"], and other non-object items. The subsequentm.contentaccess will throw and turn a bad request into a 500 instead of the intended 400. Validate each element shape up front, or coerce/skip non-conforming items before buildingtext.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/server/routes/claudeProxyRoutes.ts` around lines 4864 - 4881, The code currently assumes each entry in body.messages is an object with a content field before mapping, which can throw; update the validation around body/messages to reject or sanitize malformed entries: after confirming Array.isArray(body?.messages) use a check like body.messages.every(m => m && typeof m === "object" && ("content" in m)) and if false return buildClaudeError(400, "Invalid message entries"); alternatively filter out nonconforming items before constructing text (e.g., messages.filter(m => m && typeof m === "object").map(...)). Ensure you reference the same variables (body, messages, buildClaudeError, text) and apply this check/filtration immediately before the mapping that builds text.
3296-3322:⚠️ Potential issue | 🟠 MajorSanitize these persisted upstream diagnostics with the shared helper.
These branches still snapshot capped raw response bodies after only removing two header names. If Anthropic echoes prompt text, account metadata, or tokens in an error payload, that data still lands in tracer logs and disk snapshots. Run both headers and body through
transformParamsForLogging()before calling the tracer andlogProxyBody().As per coding guidelines
Use transformParamsForLogging() to safely strip secrets before logging.Also applies to: 4245-4273
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/server/routes/claudeProxyRoutes.ts` around lines 3296 - 3322, The code currently builds retryRespHeaders/safeRetryHeaders and cappedRetryBody then logs them directly; instead pass both headers and the capped body through the shared sanitizer transformParamsForLogging() and use the returned sanitized headers/body for tracer?.logUpstreamResponseHeaders, tracer?.logUpstreamResponseBody and logProxyBody. Specifically, replace uses of safeRetryHeaders and cappedRetryBody with the outputs of transformParamsForLogging (apply it to retryRespHeaders and retryBody/cappedRetryBody), and ensure contentType and bodySize in logProxyBody reflect the sanitized payloads where applicable; do the same fix for the other identical branch referenced.src/lib/workflow/core/workflowRunner.ts (2)
740-742:⚠️ Potential issue | 🟠 MajorEarly stream cancellation still reports success and drops the serialized workflow span.
If the consumer stops after the preliminary chunk, execution falls through to
finallywithout ever ending or recording theSpanSerializerworkflow span. That same path also marks the outer OTel spanOK, even though the workflow was aborted before completion. End/record both spans in this path, and use a cancelled/aborted status instead ofOK.Also applies to: 869-905
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/workflow/core/workflowRunner.ts` around lines 740 - 742, The finally block currently leaves the SpanSerializer workflow span un-ended and sets the outer OpenTelemetry span to OK when execution is aborted early; update the finally block in workflowRunner where spanEnded is declared so that if spanEnded is false you explicitly end/record the SpanSerializer workflow span and set the outer OTel span status to a cancelled/aborted status (not OK). Concretely: use the existing spanEnded flag to guard a call to the SpanSerializer end/record method and call the OTel span setStatus(...) with a CANCELLED/ABORTED code before closing the outer span, and replicate the same fix for the other finally region covering the 869-905 area to ensure both spans are always ended and marked cancelled on early stream cancellation.
689-700:⚠️ Potential issue | 🟠 Major
startActiveSpan()still won't keep this workflow span active during generator iteration.The callback only runs while the generator is created. Later
next()calls onrunWorkflowStreamingInner()execute outside that active context, so child spans created during streaming can still miss this parent unless the iterator methods are bound to the captured OTel context before returning.Does OpenTelemetry JavaScript `tracer.startActiveSpan()` preserve the active context across later async generator iterations returned from the callback, or should async iterator methods be wrapped with `context.bind()`?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/workflow/core/workflowRunner.ts` around lines 689 - 700, The span started with tracers.workflow.startActiveSpan around runWorkflowStreamingInner only covers generator creation, not subsequent iterations; ensure the async generator's next/return/throw methods run in the span's context by binding the returned iterator to the captured OpenTelemetry context (use context.bind or equivalent) before yielding it; locate the startActiveSpan call that creates generator and wrap/bind the generator's iterator methods (the variable generator returned by tracers.workflow.startActiveSpan and the runWorkflowStreamingInner callback/otelSpan) so child spans created during streaming remain children of the workflow span.
🧹 Nitpick comments (7)
src/lib/server/routes/memoryRoutes.ts (1)
19-36: Consider setting span ERROR status for error responses.Since the handlers return
createErrorResponse(...)objects (e.g.,MEMORY_ERROR,SESSION_NOT_FOUND) rather than throwing,withSpanwill never observe a failure and the span will always close as OK. If you want failed requests to be visible in Langfuse/OTel, inspect the handler's return for an error-shaped payload and mark the span accordingly. Non-blocking — flagging for future observability fidelity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/server/routes/memoryRoutes.ts` around lines 19 - 36, tracedMemoryHandler currently wraps the handler with withSpan but since handlers return error-shaped responses rather than throw, spans always close OK; update tracedMemoryHandler to await the result of fn(ctx) inside the withSpan callback, detect error-shaped payloads (e.g., the shape produced by createErrorResponse and constants like MEMORY_ERROR or SESSION_NOT_FOUND) and, when detected, set the span status to ERROR and/or add an error attribute (use the span API used by withSpan/tracers.http, e.g., span.setStatus({ code: SpanStatusCode.ERROR, message: ... }) and span.setAttribute('error', true)) before returning the result so failed responses are visible in tracing.src/lib/telemetry/traceContext.ts (1)
12-22: ConsiderisSpanContextValidinstead of a hard-coded all-zero check.
@opentelemetry/apiexportsisSpanContextValid(spanContext)which validates bothtraceIdandspanIdagainst the OTel spec requirements. The current check only validatestraceIdfor all-zeros but doesn't validatespanId, which could result in an invalid all-zerosspanIdbeing propagated asparentSpanId.♻️ Suggested refactor
-import { trace, context } from "@opentelemetry/api"; +import { trace, context, isSpanContextValid } from "@opentelemetry/api"; @@ - const activeSpan = trace.getSpan(context.active()); - if (!activeSpan) { - return {}; - } - const ctx = activeSpan.spanContext(); - // Invalid trace IDs are all zeros — don't use those - if (!ctx.traceId || ctx.traceId === "00000000000000000000000000000000") { - return {}; - } - return { traceId: ctx.traceId, parentSpanId: ctx.spanId }; + const activeSpan = trace.getSpan(context.active()); + if (!activeSpan) { + return {}; + } + const ctx = activeSpan.spanContext(); + if (!isSpanContextValid(ctx)) { + return {}; + } + return { traceId: ctx.traceId, parentSpanId: ctx.spanId };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/telemetry/traceContext.ts` around lines 12 - 22, Replace the manual all-zero traceId check with the OpenTelemetry helper: after getting ctx = activeSpan.spanContext() call isSpanContextValid(ctx) (imported from '@opentelemetry/api') and return {} when it returns false; keep returning { traceId: ctx.traceId, parentSpanId: ctx.spanId } when valid. Ensure you add the isSpanContextValid import and remove the hard-coded traceId all-zero string check so both traceId and spanId are validated.src/lib/observability/utils/spanSerializer.ts (1)
37-47: Confirm intentional coupling ofparentSpanIdinheritance totraceIdinheritance.
resolvedParentSpanIdis only populated fromgetActiveTraceContext()inside theif (!resolvedTraceId)branch. So a caller that passes an explicittraceIdbut omitsparentSpanIdwill end up withparentSpanId === undefinedeven when an active OTel span exists — breaking parent linkage for that Pipeline B span in Langfuse/OTel.If callers always pair traceId+parentSpanId when they override, this is fine. Otherwise, consider lifting the parent inheritance out:
♻️ Optional refactor
- let resolvedTraceId = traceId; - let resolvedParentSpanId = parentSpanId; - if (!resolvedTraceId) { - const otelCtx = getActiveTraceContext(); - resolvedTraceId = otelCtx.traceId ?? randomBytes(16).toString("hex"); - if (!resolvedParentSpanId && otelCtx.parentSpanId) { - resolvedParentSpanId = otelCtx.parentSpanId; - } - } + let resolvedTraceId = traceId; + let resolvedParentSpanId = parentSpanId; + if (!resolvedTraceId || !resolvedParentSpanId) { + const otelCtx = getActiveTraceContext(); + if (!resolvedTraceId) { + resolvedTraceId = otelCtx.traceId ?? randomBytes(16).toString("hex"); + } + if (!resolvedParentSpanId && otelCtx.parentSpanId) { + resolvedParentSpanId = otelCtx.parentSpanId; + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/observability/utils/spanSerializer.ts` around lines 37 - 47, The current logic only inherits parentSpanId from getActiveTraceContext() when traceId is not provided (see resolvedTraceId and resolvedParentSpanId handling), causing callers that supply traceId but omit parentSpanId to lose parent linkage; change the flow so you always call getActiveTraceContext() when parentSpanId is missing (regardless of whether traceId was provided) and set resolvedParentSpanId = otelCtx.parentSpanId if undefined, while preserving any explicit parentSpanId; keep the existing resolvedTraceId fallback behavior (randomBytes when otelCtx.traceId missing) but do not couple parentSpanId inheritance to the traceId branch.src/lib/server/routes/healthRoutes.ts (1)
230-292: Optional: memoizeprocess.memoryUsage()in/health/detailed.
process.memoryUsage()is called five times within a single request handler. It's not free (it iterates all V8 heap spaces). Call it once into a local and reuse.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/server/routes/healthRoutes.ts` around lines 230 - 292, The handler calls process.memoryUsage() multiple times; capture it once into a local (e.g., const mem = process.memoryUsage()) and replace all subsequent process.memoryUsage() uses in this block (the heapUsed/heapTotal/rss/external calculations and the span.setAttribute("health.memory.heap_mb") call) with values derived from that cached mem object so the code uses the single sampled memory snapshot instead of calling process.memoryUsage() repeatedly.src/lib/server/routes/openApiRoutes.ts (1)
58-133: Optional: extract the duplicated JSON/YAML spec-building logic.The JSON and YAML handlers both do
routes = getRoutes?.() ?? [], the identical warn/debug log block, set the sameopenapi.route_countattribute, and constructnew OpenAPIGenerator({ basePath, routes }). Consider a small helper (e.g.,buildOpenApiSpec(span)) to avoid drift when one side is edited in isolation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/server/routes/openApiRoutes.ts` around lines 58 - 133, The JSON and YAML route handlers duplicate the same logic (calling getRoutes?.(), the warn/debug logging, span.setAttribute("openapi.route_count", ...), and instantiating new OpenAPIGenerator({ basePath, routes })), which risks drift; extract that shared logic into a small helper (e.g., buildOpenApiSpec or buildOpenApiContext) that accepts the span and returns { routes, generator } or the final spec for each handler, then update the JSON handler to call generator.generate() and the YAML handler to call generator.toYAML() while keeping response wrapping (like _raw/contentType) in the YAML branch; reference symbols: getRoutes, withSpan, OpenAPIGenerator, span.setAttribute("openapi.route_count", ...), generator.generate, generator.toYAML.src/lib/providers/amazonBedrock.ts (2)
2256-2272: Throttling detection looks correct; consider also catching HTTP 429 as a fallback.
error.name === "ThrottlingException"is the canonical AWS SDK v3 surface for Bedrock rate limiting, so this will catch the common case. For defense in depth (e.g., gateway-level throttling or SDK variants that surface the throttle only via$metadata.httpStatusCode), you could additionally check(error as any)?.$metadata?.httpStatusCode === 429. Non-blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/providers/amazonBedrock.ts` around lines 2256 - 2272, Add a fallback check for HTTP 429 on top of the existing ThrottlingException checks: inside the error handling where errName / errCode are inspected (the block that returns new RateLimitError and new ProviderError), also inspect (error as any)?.$metadata?.httpStatusCode === 429 and treat that as a rate-limit case; if true, return the same RateLimitError (using the same message pattern and "bedrock" provider) instead of falling through to ProviderError. Ensure you reference the existing RateLimitError and ProviderError constructors and preserve providerName in the ProviderError path.
1407-1508: Recommended refactor: the first-iteration inline chunk loop duplicatesprocessStreamResponse.The block that walks Bedrock stream chunks (
contentBlockStart/contentBlockDeltatext & toolUse /contentBlockStopwith_inputBufferJSON parsing /messageStop/metadata.usage) is now implemented twice — once inline inside theReadableStream.startcallback and again inprocessStreamResponse. The two copies must stay in lockstep (you already had to patch themessageStop→continue+ metadata handling in both places for this PR), which is a maintenance hazard and an easy source of future drift.Consider extracting a single
consumeBedrockStream(response.stream, controller)helper that returns{ stopReason, messageContent, usage }and call it from both call sites. No behavior change intended, just dedup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/providers/amazonBedrock.ts` around lines 1407 - 1508, The inline loop in the ReadableStream.start callback duplicates logic already in processStreamResponse (handling contentBlockStart/contentBlockDelta text and toolUse, contentBlockStop JSON _inputBuffer parsing, messageStop handling, and metadata.usage accumulation), which risks drift; extract that chunk-processing logic into a single helper (e.g., consumeBedrockStream) that accepts the Bedrock async-iterable (response.stream) and the controller and returns the aggregated result ({ stopReason, messageContent, usage }), then replace the inline loop in ReadableStream.start and the existing call site to processStreamResponse to use this new helper so both places share the same implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f37b65b5-b3b4-48f9-bf61-8a3612b50281
📒 Files selected for processing (69)
docs/telemetry-after-proof.txtdocs/telemetry-baseline-proof.txtdocs/telemetry-fix-completion-report.mddocs/telemetry-fix-diff.txtdocs/telemetry-fix-execution-plan.mddocs/telemetry-fix-plan.mddocs/telemetry-gaps-audit.mdsrc/cli/loop/session.tssrc/lib/auth/anthropicOAuth.tssrc/lib/auth/sessionManager.tssrc/lib/auth/tokenStore.tssrc/lib/client/httpClient.tssrc/lib/client/sseClient.tssrc/lib/client/streamingClient.tssrc/lib/client/wsClient.tssrc/lib/context/budgetChecker.tssrc/lib/context/contextCompactor.tssrc/lib/context/summarizationEngine.tssrc/lib/core/baseProvider.tssrc/lib/core/modules/GenerationHandler.tssrc/lib/core/modules/StreamHandler.tssrc/lib/core/modules/ToolsManager.tssrc/lib/evaluation/ragasEvaluator.tssrc/lib/mcp/batching/requestBatcher.tssrc/lib/mcp/httpRateLimiter.tssrc/lib/mcp/httpRetryHandler.tssrc/lib/mcp/mcpClientFactory.tssrc/lib/mcp/toolDiscoveryService.tssrc/lib/mcp/toolRegistry.tssrc/lib/memory/memoryRetrievalTools.tssrc/lib/neurolink.tssrc/lib/observability/utils/spanSerializer.tssrc/lib/processors/base/BaseFileProcessor.tssrc/lib/processors/registry/ProcessorRegistry.tssrc/lib/providers/amazonBedrock.tssrc/lib/providers/amazonSagemaker.tssrc/lib/providers/anthropic.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/rag/chunkers/BaseChunker.tssrc/lib/rag/ragIntegration.tssrc/lib/rag/reranker/reranker.tssrc/lib/rag/retrieval/vectorQueryTool.tssrc/lib/server/middleware/common.tssrc/lib/server/routes/agentRoutes.tssrc/lib/server/routes/claudeProxyRoutes.tssrc/lib/server/routes/healthRoutes.tssrc/lib/server/routes/mcpRoutes.tssrc/lib/server/routes/memoryRoutes.tssrc/lib/server/routes/openApiRoutes.tssrc/lib/server/routes/toolRoutes.tssrc/lib/services/server/ai/observability/instrumentation.tssrc/lib/telemetry/traceContext.tssrc/lib/telemetry/tracers.tssrc/lib/types/span.tssrc/lib/utils/toolEndEmitter.tssrc/lib/workflow/core/ensembleExecutor.tssrc/lib/workflow/core/judgeScorer.tssrc/lib/workflow/core/responseConditioner.tssrc/lib/workflow/core/workflowRunner.tstest/telemetry-gaps-verification.ts
| **Date**: 2026-04-16 | ||
| **Status**: ✅ COMPLETE — 26/28 verification tests resolved | ||
| **Regressions**: 0 | ||
|
|
||
| --- | ||
|
|
||
| ## Summary | ||
|
|
||
| | Metric | Before | After | Delta | | ||
| | ---------------------------------- | ------ | ----- | ------- | | ||
| | Verification tests confirming gaps | 28 | 2 | **-26** | | ||
| | Type errors | 0 | 0 | 0 | | ||
| | Bugfix regression tests | 48/48 | 48/48 | 0 | | ||
| | Total fixes applied | — | 22 | — | | ||
|
|
||
| ## Gaps Resolved (25 FAIL + 1 SKIP = 26 of 28) |
There was a problem hiding this comment.
Update this report to match the current verification state.
This artifact still says 26/28 resolved and keeps A2 open, but the current PR objectives for this branch say telemetry verification is at 0/28 unresolved. Merging the stale report will confuse future debugging and release signoff.
Also applies to: 72-86
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/telemetry-fix-completion-report.md` around lines 3 - 18, Update the
stale status and summary table text in the report: replace the "**Status**: ✅
COMPLETE — 26/28 verification tests resolved" phrase and the "Gaps Resolved (25
FAIL + 1 SKIP = 26 of 28)" line with the current verification state (telemetry
at 0/28 unresolved, i.e., 28/28 resolved), adjust the summary table row
"Verification tests confirming gaps" and the "After"/"Delta" numbers to reflect
0 unresolved (or 28/28 resolved) and update "Total fixes applied" if needed;
also make the same textual fixes in the duplicated section later in the document
that contains the same status/table content.
d3ac082 to
cf3fd35
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 |
cf3fd35 to
54fe3fa
Compare
Review Feedback Addressed (Cycle 3) + Proxy Cooling FixesProxy Fixes (from user-reported bugs)
Cycle 3 CodeRabbit Comments (15 total)
13/15 fixed, 2 deferred (style nit + architectural pattern requiring larger refactor) Validation
@coderabbitai — All actionable comments addressed. Please re-review. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
Proxy Cooling Fixes (beyond CodeRabbit scope — good proactive improvements):
Notable Cycle 3 Fixes:
Deferred items acknowledged:
Triggering the full re-review now. ✏️ Learnings added
✅ Actions performedFull review triggered. |
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
54fe3fa to
1c13cf0
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 |
Additional scope: type-consolidation refactor (22 commits,
|
🤖 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 |
a3c0983 to
c30ff94
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 |
c30ff94 to
407bee6
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 |
…rden proxy retry - Close 28/28 OTel/Langfuse telemetry gaps across all providers - Relocate 262 local type declarations into canonical src/lib/types/ - Add no-local-type-alias ESLint rule to prevent future violations - Add per-account coolingUntil + maybeResetPrimaryToHome for proxy 429 handling - Increase MAX_RATE_LIMIT_SAME_ACCOUNT_RETRIES to 10 - Fix ReDoS in memoryRetrievalTools, Array.isArray validation in proxy - Add failure generation:end emits for Bedrock, Google AI Studio, Ollama - Fix Pipeline B spanId uniqueness, step_count source, stream span lifecycle
407bee6 to
d7713e9
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.54.7 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Summary
Telemetry gaps resolved (28/28)
Foundation — Pipeline B → OTel linkage
createMetricsTraceContextreads active OTel span instead of random UUIDmetricsSpanremoved from BaseProviderSpanSerializer.createSpaninherits traceId viagetActiveTraceContext()Tool telemetry (T1–T10)
isError:true, error arg propagation inemitToolEndEvent, circuit breaker error propagation, retry count attributediscoverTools, RAG search, memory retrieval, request batcher — all wrapped withwithSpanGenerate pipeline (G2–G7)
content-filter/lengthfinish reasons → WARNING level, step count attributes, retry count,ContextBudgetExceededErrorclassificationStream pipeline (S1–S8)
stream:errorevent emission,tool:endfromonStepFinishfor all 13 providers,finishReasonrecording,NoOutputGeneratedErrorsentinel chunk, abort distinction, duplicate span removalProvider fixes (P1–P8)
ThrottlingException→RateLimitError, Gemini 3 + Bedrockgeneration:endwith real token counts, Langfuse level/status enrichmentFull subsystem OTel coverage
All 13 providers, CLI turns, context/summarization, evaluation, file processing, RAG pipeline (chunker/reranker/vector), all server routes, middleware, workflow (sync + streaming), auth token store, HTTP/SSE/WS clients, memory retrieval, MCP subsystems
Langfuse enrichment (ContextEnricher.onEnd)
langfuse.usage_details,gen_ai.response.model,gen_ai.request.model_parameters,langfuse.levelERROR/WARNINGProxy improvements
Full raw error logging for all non-200 responses
429 path in
fetchAnthropicAccountResponseand all auth-retry error paths now capture:tracer.logUpstreamResponseHeaders()tracer.logUpstreamResponseBody()logProxyBody({ phase: "upstream_response", ... })Retry limit
MAX_RATE_LIMIT_SAME_ACCOUNT_RETRIES: 5 → 10New files
src/lib/telemetry/traceContext.ts—getActiveTraceContext()helpersrc/lib/utils/toolEndEmitter.ts— sharedemitToolEndFromStepFinish()helpertest/telemetry-gaps-verification.ts— 28-test verification suitedocs/telemetry-gaps-audit.md— full audit documentdocs/telemetry-fix-plan.md— detailed fix specificationsdocs/telemetry-fix-completion-report.md— executive summaryTest plan
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests