fix: curator production fixes — compaction, timeout, MCP events, Langfuse - #937
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR adds a rollout plan document and implements six production fixes: context compaction/truncation tuning, provider-aware and context‑scaled timeouts, extended External MCP event payloads with Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
Contributor
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
Contributor
Documentation Validation Results🚀 Documentation validation passed!
📦 Build artifact uploaded successfully. Ready for deployment preview. Commit: |
There was a problem hiding this comment.
Pull request overview
This PR applies a set of NeuroLink SDK production fixes observed via Curator monitoring, spanning context compaction behavior, provider timeouts, external MCP event payloads, and Langfuse/OpenTelemetry instrumentation.
Changes:
- Adjusts context compaction behavior (lower prune threshold; finer truncation escalation; clears compaction watermark on failure).
- Updates timeout selection to use per-provider defaults and adds context-size-based timeout scaling.
- Adds
serverNameto external MCP event payloads and avoids duplicate Langfuse span processor registration; preserves ALS context across background memory writes.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/lib/context/contextCompactor.ts |
Lowers pruneMinimumSavings and increases truncation maxIterations to reduce near-miss over-truncation. |
src/lib/context/stages/slidingWindowTruncator.ts |
Changes iterative truncation escalation step from +25% to +10%. |
src/lib/neurolink.ts |
Clears compaction watermark on final-budget failure; wraps background memory write to preserve Langfuse ALS; adds serverName to SDK-emitted MCP events. |
src/lib/core/modules/Utilities.ts |
Uses per-provider default timeouts (via getDefaultTimeout) and adds getContextAwareTimeout(). |
src/lib/types/externalMcp.ts |
Extends exported MCP server event payload types with serverName. |
src/lib/mcp/externalServerManager.ts |
Populates serverName for emitted server/tool lifecycle events. |
src/lib/services/server/ai/observability/instrumentation.ts |
Skips registering Langfuse span processor if one is already present on the global tracer provider. |
docs/superpowers/plans/2026-04-09-curator-production-fixes.md |
Adds an implementation/verification plan documenting the fixes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+234
to
+238
| if (options.timeout) { | ||
| const parsed = parseTimeout(options.timeout); | ||
| if (parsed !== undefined) { | ||
| return parsed; | ||
| } |
There was a problem hiding this comment.
getTimeout() calls parseTimeout() without guarding for exceptions. parseTimeout() throws for values like "0s"/"0" (<= 0), but validateTextGenerationOptions() currently allows those formats, so a caller can pass validation and still crash at runtime. Consider either tightening validation to reject non-positive timeouts or wrapping parseTimeout in try/catch and falling back to defaults with a warning/error.
Comment on lines
305
to
+309
| export type ExternalMCPServerEvents = { | ||
| /** Server status changed */ | ||
| statusChanged: { | ||
| serverId: string; | ||
| serverName: string; |
There was a problem hiding this comment.
ExternalMCPServerEvents is exported as part of the public SDK types. Making serverName a required field is a TypeScript-breaking change for any downstream code that constructs these event payloads (e.g., tests/mocks). If you want to keep this backwards compatible, consider making serverName optional in the type (while still always emitting it at runtime), or introduce a new versioned event type.
Comment on lines
960
to
964
| this.emit("disconnected", { | ||
| serverId, | ||
| serverName: this.getServerName(serverId), | ||
| reason: "Manually removed", | ||
| timestamp: new Date(), |
There was a problem hiding this comment.
In removeServer(), the server is deleted from this.servers before emitting the disconnected event, so getServerName(serverId) will always fall back to serverId (the config name is no longer available). Capture serverName (or the instance/config) before deleting, and use that captured value in the emitted payload.
Comment on lines
10801
to
10804
| this.emitter.emit("externalMCP:serverRemoved", { | ||
| serverId, | ||
| serverName: serverId, | ||
| timestamp: Date.now(), |
There was a problem hiding this comment.
externalMCP:serverRemoved emits serverName: serverId, which defeats the goal of providing a human-readable server name (and differs from serverAdded, which uses config.name || serverId). Consider resolving the name before removal (e.g., via externalServerManager.getServer(serverId)?.config.name) and emitting that value (fall back to serverId if unavailable).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/mcp/externalServerManager.ts (1)
956-965:⚠️ Potential issue | 🟡 MinorCapture
serverNamebefore removing the server entry.
this.servers.delete(serverId)runs immediately before this emit, sogetServerName(serverId)falls back toserverIdon manual removals. That leavesdisconnected.serverNamewrong for named servers in the one path this PR is meant to normalize.🛠️ Minimal fix
+ const serverName = instance.config.name || serverId; + // Remove from registry this.servers.delete(serverId); // Emit event this.emit("disconnected", { serverId, - serverName: this.getServerName(serverId), + serverName, reason: "Manually removed", timestamp: new Date(), } satisfies ExternalMCPServerEvents["disconnected"]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/mcp/externalServerManager.ts` around lines 956 - 965, Before calling this.servers.delete(serverId) capture the server name into a local variable so the emitted "disconnected" event includes the correct name; i.e., call const serverName = this.getServerName(serverId) (or similar) before this.servers.delete(serverId) and then pass serverName into this.emit("disconnected", { serverId, serverName, reason: "Manually removed", timestamp: new Date() } as ExternalMCPServerEvents["disconnected"]). Ensure you reference the same symbol names (getServerName, servers.delete, emit, ExternalMCPServerEvents["disconnected"]) used in the diff.
🧹 Nitpick comments (3)
src/lib/neurolink.ts (1)
1719-1750: Wrap background memory writes with a timeout guard.The new background async path still runs unbounded (
Promise.all(writeOps)), which can hang indefinitely under downstream latency spikes. Please wrap it withwithTimeout.As per coding guidelines `**/*.ts`: Use `withTimeout` utility to wrap async calls for error handling.♻️ Suggested patch
- await Promise.all(writeOps); + await withTimeout( + Promise.all(writeOps), + 10000, + new Error("Background memory write timed out"), + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/neurolink.ts` around lines 1719 - 1750, The background memory write block created in runWithCurrentLangfuseContext (wrappedMemoryWrite) currently awaits Promise.all(writeOps) without a timeout; replace that await with a call to withTimeout(Promise.all(writeOps), <reasonable ms>) and handle the potential timeout/rejection in the catch so the operation cannot hang indefinitely; keep the existing logger.warn in the catch and include timeout context in the message. Ensure you import/use withTimeout and apply this change where ensureMemoryReady(), client.add(...), and Promise.all(writeOps) are used before setImmediate(wrappedMemoryWrite).docs/superpowers/plans/2026-04-09-curator-production-fixes.md (2)
628-646: Enhance verification checklist to cover all changes.The verification commands are generally good, but consider adding:
TypeScript compilation check for Task 3 providerName changes:
# Verify Utilities constructor accepts providerName parameter grep -A 3 "constructor.*defaultTimeout.*providerName" src/lib/core/modules/Utilities.ts # Verify BaseProvider passes providerName grep -A 2 "new Utilities.*this.name" src/lib/core/baseProvider.tsVerify Task 4 serverRemoved uses getServerName (not hardcoded serverId):
# Check serverRemoved doesn't hardcode serverId as serverName rg "serverRemoved.*serverName.*serverId[^,]*(,|\})" src/lib/neurolink.tsAdd a runtime verification step for staging deployment to confirm:
- Langfuse duplicate span prevention
- Timeout scaling for large contexts
- MCP event serverName population
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/superpowers/plans/2026-04-09-curator-production-fixes.md` around lines 628 - 646, Update the Verification Checklist to include static checks that confirm the Utilities constructor now accepts providerName and that BaseProvider forwards its name: add grep/rg checks for "constructor.*defaultTimeout.*providerName" in Utilities (src/lib/core/modules/Utilities.ts) and for "new Utilities.*this.name" in BaseProvider (src/lib/core/baseProvider.ts); add a pattern check ensuring serverRemoved uses getServerName instead of hardcoded serverId by searching for serverRemoved and instances of serverId used as serverName in neurolink (src/lib/neurolink.ts); and append runtime staging verification steps to exercise Langfuse duplicate-span prevention, timeout scaling for large contexts, and that MCP events include populated serverName (verify via logs or a small staged scenario).
15-26: Clarify file structure table entries.The table lists
src/lib/neurolink.tsthree times (lines 19, 24, 20) and mentionssrc/lib/core/baseProvider.ts(line 20), but:
- The multiple
neurolink.tsentries could be consolidated or differentiated more clearly (e.g., "neurolink.ts (compaction)", "neurolink.ts (MCP events)", "neurolink.ts (AsyncLocalStorage)").baseProvider.tsis listed in the table but not referenced in any task steps. Task 3 modifies onlyUtilities.tsand doesn't touchbaseProvider.ts. Either remove it from the table or add the corresponding implementation steps.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/superpowers/plans/2026-04-09-curator-production-fixes.md` around lines 15 - 26, The table has duplicate neurolink.ts rows and a dangling baseProvider.ts entry; consolidate the three neurolink rows into clearly labeled entries (e.g., "src/lib/neurolink.ts (compaction)", "src/lib/neurolink.ts (MCP events)", "src/lib/neurolink.ts (AsyncLocalStorage/ALS)") so each task maps to the specific change (clear compaction watermark on budget failure, include serverName in serverAdded/serverRemoved events, wrap setImmediate with ALS context/auto-detect Langfuse processor). Also either remove the `src/lib/core/baseProvider.ts` row if no change is required, or add a corresponding implementation step that updates baseProvider to use per-provider default timeouts (pointing reviewers to the Utilities.getDefaultTimeout change and the baseProvider methods that currently hardcode 30000ms).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/superpowers/plans/2026-04-09-curator-production-fixes.md`:
- Around line 417-424: The serverRemoved emitter currently hardcodes serverName
to serverId; update the emit to resolve the canonical name like other events by
calling the same helper or lookup used elsewhere (e.g., use
getServerName(serverId) or retrieve the server instance from
this.mcpServerManager and use instance.config.name || serverId) before emitting
externalMCP:serverRemoved so add serverName: getServerName(serverId) (or
serverName: instance.config.name || serverId) and ensure this.mcpServerManager
is available in the current scope or fetch the instance first.
In `@src/lib/core/modules/Utilities.ts`:
- Around line 232-245: In getTimeout, don't gate parseTimeout on truthiness (it
should accept 0 or empty-string), so call parseTimeout(options.timeout) directly
and return the parsed value if it's not undefined; also pick the provider
default based on whether options is streaming or generation (use
getDefaultTimeout(this.providerName, ..."stream" or "generate")—detect
StreamOptions via a streaming discriminator present on StreamOptions such as an
onData/onChunk/onStream callback or a boolean stream flag) instead of hardcoding
"generate"; reference getTimeout, parseTimeout, getDefaultTimeout,
TextGenerationOptions, StreamOptions, providerName, and defaultTimeout when
making the change.
In `@src/lib/neurolink.ts`:
- Around line 10801-10804: When emitting the "externalMCP:serverRemoved" event,
preserve the configured human-readable name instead of setting serverName to
serverId: before you remove the server from the internal store, read the
server's configured name (e.g., from the server instance or config object held
in the class) into a local variable and then call
this.emitter.emit("externalMCP:serverRemoved", { serverId, serverName:
<capturedName>, timestamp: Date.now() }) so the event includes the real name;
update the code around the removal logic that currently calls this.emitter.emit
to use that capturedName (refer to the this.emitter.emit invocation and the
surrounding removal function).
---
Outside diff comments:
In `@src/lib/mcp/externalServerManager.ts`:
- Around line 956-965: Before calling this.servers.delete(serverId) capture the
server name into a local variable so the emitted "disconnected" event includes
the correct name; i.e., call const serverName = this.getServerName(serverId) (or
similar) before this.servers.delete(serverId) and then pass serverName into
this.emit("disconnected", { serverId, serverName, reason: "Manually removed",
timestamp: new Date() } as ExternalMCPServerEvents["disconnected"]). Ensure you
reference the same symbol names (getServerName, servers.delete, emit,
ExternalMCPServerEvents["disconnected"]) used in the diff.
---
Nitpick comments:
In `@docs/superpowers/plans/2026-04-09-curator-production-fixes.md`:
- Around line 628-646: Update the Verification Checklist to include static
checks that confirm the Utilities constructor now accepts providerName and that
BaseProvider forwards its name: add grep/rg checks for
"constructor.*defaultTimeout.*providerName" in Utilities
(src/lib/core/modules/Utilities.ts) and for "new Utilities.*this.name" in
BaseProvider (src/lib/core/baseProvider.ts); add a pattern check ensuring
serverRemoved uses getServerName instead of hardcoded serverId by searching for
serverRemoved and instances of serverId used as serverName in neurolink
(src/lib/neurolink.ts); and append runtime staging verification steps to
exercise Langfuse duplicate-span prevention, timeout scaling for large contexts,
and that MCP events include populated serverName (verify via logs or a small
staged scenario).
- Around line 15-26: The table has duplicate neurolink.ts rows and a dangling
baseProvider.ts entry; consolidate the three neurolink rows into clearly labeled
entries (e.g., "src/lib/neurolink.ts (compaction)", "src/lib/neurolink.ts (MCP
events)", "src/lib/neurolink.ts (AsyncLocalStorage/ALS)") so each task maps to
the specific change (clear compaction watermark on budget failure, include
serverName in serverAdded/serverRemoved events, wrap setImmediate with ALS
context/auto-detect Langfuse processor). Also either remove the
`src/lib/core/baseProvider.ts` row if no change is required, or add a
corresponding implementation step that updates baseProvider to use per-provider
default timeouts (pointing reviewers to the Utilities.getDefaultTimeout change
and the baseProvider methods that currently hardcode 30000ms).
In `@src/lib/neurolink.ts`:
- Around line 1719-1750: The background memory write block created in
runWithCurrentLangfuseContext (wrappedMemoryWrite) currently awaits
Promise.all(writeOps) without a timeout; replace that await with a call to
withTimeout(Promise.all(writeOps), <reasonable ms>) and handle the potential
timeout/rejection in the catch so the operation cannot hang indefinitely; keep
the existing logger.warn in the catch and include timeout context in the
message. Ensure you import/use withTimeout and apply this change where
ensureMemoryReady(), client.add(...), and Promise.all(writeOps) are used before
setImmediate(wrappedMemoryWrite).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 55a01c34-9937-4f26-b0de-fcf83f4d0261
📒 Files selected for processing (8)
docs/superpowers/plans/2026-04-09-curator-production-fixes.mdsrc/lib/context/contextCompactor.tssrc/lib/context/stages/slidingWindowTruncator.tssrc/lib/core/modules/Utilities.tssrc/lib/mcp/externalServerManager.tssrc/lib/neurolink.tssrc/lib/services/server/ai/observability/instrumentation.tssrc/lib/types/externalMcp.ts
murdore
force-pushed
the
fix/curator-production-fixes
branch
from
April 9, 2026 09:19
02b6136 to
88c6bf1
Compare
Contributor
✅ Single Commit Policy - COMPLIANTStatus: Policy requirements met • 1 commit • Valid format • Ready for merge 📊 View validation details📝 Commit Details
✅ Validation Results
🤖 Automated validation by NeuroLink Single Commit Enforcement |
Contributor
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
murdore
force-pushed
the
fix/curator-production-fixes
branch
from
April 9, 2026 10:11
88c6bf1 to
0d7e87b
Compare
Contributor
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
Contributor
Author
|
@coderabbitai do full review and verify if all the comments are resolved. |
|
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/context/stages/slidingWindowTruncator.ts (1)
9-10:⚠️ Potential issue | 🟡 MinorStale comment: "up to 3 passes" should be "up to 6 passes".
The doc comment references the old iteration count. With
maxIterationsincreased to 6 incontextCompactor.ts, this should be updated for consistency.- * instead of fixed 50%, with iterative refinement up to 3 passes. + * instead of fixed 50%, with iterative refinement up to 6 passes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/context/stages/slidingWindowTruncator.ts` around lines 9 - 10, The doc comment in slidingWindowTruncator.ts incorrectly states "up to 3 passes" for the adaptive truncation loop; update that comment to "up to 6 passes" to match the increased maxIterations used in contextCompactor.ts (and search for any other stale mentions of "3 passes" related to slidingWindowTruncator or adaptive truncation and update them to 6). Ensure the comment near the slidingWindowTruncator adaptive truncation description is changed accordingly.
♻️ Duplicate comments (1)
src/lib/core/modules/Utilities.ts (1)
241-249:⚠️ Potential issue | 🟠 Major
getTimeout()misclassifies multimodal generate calls as stream.Line 244 infers
"stream"fromoptions.input, butTextGenerationOptionsin this file also usesinput(see Line 173-Line 179). That routes generate requests to streaming defaults (2m) and bypasses provider-specific generate defaults.💡 Suggested fix
- getTimeout(options: TextGenerationOptions | StreamOptions): number { + getTimeout( + options: TextGenerationOptions | StreamOptions, + operation: "generate" | "stream" = "generate", + ): number { @@ - // Detect streaming vs generation for per-provider default lookup. - // StreamOptions has an `input` property; TextGenerationOptions does not. - const operation = - "input" in options && options.input ? "stream" : "generate"; - // Use per-provider default (e.g., vertex=60s, streaming=2m) instead of global 30s const providerDefault = parseTimeout( getDefaultTimeout(this.providerName, operation), );#!/bin/bash # Verify whether `input` exists on both option types and inspect getTimeout call sites. rg -n -C3 'type\s+TextGenerationOptions|interface\s+TextGenerationOptions|input\??\s*:' src/lib/types rg -n -C3 'type\s+StreamOptions|interface\s+StreamOptions|input\??\s*:' src/lib/types src/lib/types/streamTypes.ts rg -n -C2 '\.getTimeout\s*\(' src/lib🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/core/modules/Utilities.ts` around lines 241 - 249, getTimeout incorrectly classifies calls as "stream" by checking "input" in options; since TextGenerationOptions also defines input, change the detection in Utilities.getTimeout (the block that sets operation and calls getDefaultTimeout/parseTimeout) to use a reliable discriminator between StreamOptions and TextGenerationOptions (e.g., check for a StreamOptions-only property or an explicit options.mode/stream flag) instead of "input", so generate requests use provider generate defaults; update the logic around operation, getDefaultTimeout(this.providerName, operation), and parseTimeout accordingly and add/adjust any type guards for StreamOptions/TextGenerationOptions to ensure correct routing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/superpowers/plans/2026-04-09-curator-production-fixes.md`:
- Around line 214-229: The getTimeout method's snippet is stale: replace the
truthy check "if (options.timeout)" with an explicit undefined/null check (e.g.,
options.timeout !== undefined && options.timeout !== null) so zero or falsy
timeouts are honored, and remove the hardcoded "generate" operation when calling
getDefaultTimeout — instead pass the actual operation context used by this class
(e.g., use this.operation or an operation parameter) so
getDefaultTimeout(this.providerName, <operation>) is called; update references
in getTimeout and any callers to align with the current implementation.
In `@src/lib/core/modules/Utilities.ts`:
- Around line 267-269: The scaling currently uses Math.floor(estimatedTokens /
100_000) which makes exact multiples (100_000, 200_000, 300_000) count as the
next tier; change the divisor logic to use Math.floor((estimatedTokens - 1) /
100_000) so that thresholds are strictly ">" (e.g., 100_000 stays 1x, 100_001 →
1.5x), keeping the rest of the computation (scale = 1 + ... * 0.5,
Math.min(scale, 4), and Math.round(baseTimeout * ...)) unchanged and referencing
the existing estimatedTokens, scale, and baseTimeout variables.
In `@src/lib/neurolink.ts`:
- Around line 5411-5412: The code currently deletes lastCompactionMessageCount
for compactionSessionId in one failure path but not in other exits; update the
other compaction failure paths (specifically inside directProviderGeneration and
createMCPStream where they set lastCompactionMessageCount then throw
ContextBudgetExceededError) to also remove the watermark before throwing so
handleContextOverflow can later re-compact; locate the places in
directProviderGeneration and createMCPStream that call
this.lastCompactionMessageCount.set(...) then throw ContextBudgetExceededError
and insert this.lastCompactionMessageCount.delete(compactionSessionId) (or
ensure a finally/cleanup block runs) immediately before the throw.
In `@src/lib/services/server/ai/observability/instrumentation.ts`:
- Around line 671-682: The code reads the internal OpenTelemetry field
_registeredSpanProcessors on provider and compares constructor?.name to
"LangfuseSpanProcessor" (in hasExistingLangfuse), which is fragile and may break
on minified builds or OpenTelemetry changes; update the instrumentation.ts
around the provider/_registeredSpanProcessors check to (1) add a clear comment
explaining this is using an internal API and the constructor-name check is
brittle, (2) document the intended fallback behavior if the field or name check
fails (e.g., treat as no existing Langfuse processor and proceed or provide an
explicit opt-out/config flag), and (3) optionally recommend a more robust
detection approach in the comment (such as exposing a public registration flag,
using instanceof when available, or relying on explicit configuration) so future
maintainers know why this is fragile and what to change (referencing variables:
provider, _registeredSpanProcessors, hasExistingLangfuse, and
LangfuseSpanProcessor).
---
Outside diff comments:
In `@src/lib/context/stages/slidingWindowTruncator.ts`:
- Around line 9-10: The doc comment in slidingWindowTruncator.ts incorrectly
states "up to 3 passes" for the adaptive truncation loop; update that comment to
"up to 6 passes" to match the increased maxIterations used in
contextCompactor.ts (and search for any other stale mentions of "3 passes"
related to slidingWindowTruncator or adaptive truncation and update them to 6).
Ensure the comment near the slidingWindowTruncator adaptive truncation
description is changed accordingly.
---
Duplicate comments:
In `@src/lib/core/modules/Utilities.ts`:
- Around line 241-249: getTimeout incorrectly classifies calls as "stream" by
checking "input" in options; since TextGenerationOptions also defines input,
change the detection in Utilities.getTimeout (the block that sets operation and
calls getDefaultTimeout/parseTimeout) to use a reliable discriminator between
StreamOptions and TextGenerationOptions (e.g., check for a StreamOptions-only
property or an explicit options.mode/stream flag) instead of "input", so
generate requests use provider generate defaults; update the logic around
operation, getDefaultTimeout(this.providerName, operation), and parseTimeout
accordingly and add/adjust any type guards for
StreamOptions/TextGenerationOptions to ensure correct routing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3f22fd21-00c7-43f4-aaf9-e7b63bbed2dc
📒 Files selected for processing (8)
docs/superpowers/plans/2026-04-09-curator-production-fixes.mdsrc/lib/context/contextCompactor.tssrc/lib/context/stages/slidingWindowTruncator.tssrc/lib/core/modules/Utilities.tssrc/lib/mcp/externalServerManager.tssrc/lib/neurolink.tssrc/lib/services/server/ai/observability/instrumentation.tssrc/lib/types/externalMcp.ts
Comment on lines
+214
to
+229
| getTimeout(options: TextGenerationOptions | StreamOptions): number { | ||
| // If caller specified a timeout, use it (supports number ms and string formats) | ||
| if (options.timeout) { | ||
| const parsed = parseTimeout(options.timeout); | ||
| if (parsed !== undefined) { | ||
| return parsed; | ||
| } | ||
| } | ||
|
|
||
| // Use per-provider default (e.g., vertex=60s, ollama=5m) instead of global 30s | ||
| const providerDefault = parseTimeout( | ||
| getDefaultTimeout(this.providerName, "generate"), | ||
| ); | ||
| return providerDefault ?? this.defaultTimeout; | ||
| } | ||
| ``` |
There was a problem hiding this comment.
Timeout code snippet is stale vs current fix.
This snippet still shows if (options.timeout) and hardcoded "generate". If reused, it reintroduces the timeout regression that was just fixed. Please align the snippet with the current implementation behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/superpowers/plans/2026-04-09-curator-production-fixes.md` around lines
214 - 229, The getTimeout method's snippet is stale: replace the truthy check
"if (options.timeout)" with an explicit undefined/null check (e.g.,
options.timeout !== undefined && options.timeout !== null) so zero or falsy
timeouts are honored, and remove the hardcoded "generate" operation when calling
getDefaultTimeout — instead pass the actual operation context used by this class
(e.g., use this.operation or an operation parameter) so
getDefaultTimeout(this.providerName, <operation>) is called; update references
in getTimeout and any callers to align with the current implementation.
murdore
force-pushed
the
fix/curator-production-fixes
branch
from
April 9, 2026 14:56
0d7e87b to
042e341
Compare
Contributor
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
docs/superpowers/plans/2026-04-09-curator-production-fixes.md (1)
214-233:⚠️ Potential issue | 🟠 MajorHardcoded "generate" operation causes streaming operations to use incorrect timeout values.
The code calls
getTimeout(options)from multiple streaming methods (anthropic.ts:980, openAI.ts:348, openRouter.ts:304, mistral.ts:79, etc.), butgetTimeout()hardcodes the "generate" operation instead of "stream".This means:
- OpenAI streaming gets 30s timeout instead of 2m
- Vertex streaming gets 60s timeout instead of 2m
- Anthropic streaming gets 60s timeout instead of 2m
The comment claiming "BaseProvider.stream() applies its own longer timeout" is incorrect—no such override exists.
BaseProvider.stream()simply callsexecuteStream(), which uses the timeout fromgetTimeout()directly.Fix: Pass the operation type through to
getTimeout(), or detect it dynamically from the options parameter.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/superpowers/plans/2026-04-09-curator-production-fixes.md` around lines 214 - 233, getTimeout currently hardcodes the "generate" operation causing streaming paths to get wrong timeouts; change getTimeout signature to accept an operation parameter (e.g., operation: "generate" | "stream") or infer it from whether options is StreamOptions, then use that operation when calling getDefaultTimeout(this.providerName, operation). Update all callers (streaming methods like those in anthropic.ts, openAI.ts, openRouter.ts, mistral.ts and any BaseProvider.stream/executeStream invocations) to pass "stream" for streaming paths (and "generate" for non-streaming calls or keep default behavior), ensuring TextGenerationOptions/StreamOptions callers are adjusted accordingly.
🧹 Nitpick comments (1)
docs/superpowers/plans/2026-04-09-curator-production-fixes.md (1)
482-514: Optional: Consider more robust processor detection.The auto-detection at lines 487-498 relies on:
- Internal property
_registeredSpanProcessors(underscore-prefixed, suggesting it's private/internal to the OpenTelemetry SDK).- Constructor name comparison (
p.constructor?.name === "LangfuseSpanProcessor"), which can break with minification or class wrapping.While this works for the immediate use case, consider:
Alternative detection approaches
- Check for a unique public method or property specific to
LangfuseSpanProcessor(e.g.,typeof p.flush === 'function' && p.constructor.name.includes('Langfuse')).- If the Langfuse SDK exposes a way to query registered processors, use that instead.
- Document the fragility in a code comment so future maintainers are aware.
Example:
const hasExistingLangfuse = existingProcessors.some( (p) => p && typeof p === "object" && // Fragile: relies on constructor name (breaks with minification) (p.constructor?.name === "LangfuseSpanProcessor" || // Fallback: check for Langfuse-specific methods (typeof (p as any).flush === "function" && "langfuseClient" in (p as any))) );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/superpowers/plans/2026-04-09-curator-production-fixes.md` around lines 482 - 514, The current auto-detection uses the internal _registeredSpanProcessors array and a fragile constructor name check (hasExistingLangfuse) which can break under minification; update the detection in the provider registration block to prefer a public SDK query if available, otherwise broaden the predicate to check for Langfuse-specific public members (e.g., presence of a flush method or a langfuseClient property) on each processor instead of relying solely on p.constructor?.name, and add a short comment next to _registeredSpanProcessors/hasExistingLangfuse explaining the fragility and why the fallback checks are needed; keep the existing skipLangfuse and logging behavior unchanged and still gate adding langfuseProcessor via provider.addSpanProcessor when appropriate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/superpowers/plans/2026-04-09-curator-production-fixes.md`:
- Around line 534-607: The saveToMemory background op using setImmediate in
neurolink.ts must both preserve AsyncLocalStorage and be bounded by a 30s
timeout: import runWithCurrentLangfuseContext and withTimeout (if missing), wrap
the async memory write callback (the block starting at setImmediate(async () =>
{ ... } that calls this.ensureMemoryReady() and performs memory writes) by first
wrapping the body with withTimeout(..., 30_000) and then wrap that with
runWithCurrentLangfuseContext, assign to a symbol like wrappedMemoryWrite, and
call setImmediate(wrappedMemoryWrite) instead of setImmediate(async () => ...);
ensure error handling inside remains unchanged and run pnpm run check to verify
zero errors.
- Around line 236-259: The new getContextAwareTimeout method is never invoked;
update call sites that currently use getTimeout to pass estimated token counts
into getContextAwareTimeout instead. Specifically, in provider entry points such
as BaseProvider.generate and BaseProvider.stream (and any provider-specific
overrides) replace calls like this.getTimeout(options) with
this.getContextAwareTimeout(options, estimatedTokens) where estimatedTokens is
the precomputed token count (e.g., from your context compaction/token estimation
flow or ContextCompactor/MessageTokenCounter results); also update any context
compaction flows that call getTimeout to use getContextAwareTimeout and thread
the estimatedTokens value through the call chain. Ensure all call signatures
supply the correct estimated token variable.
- Around line 29-88: The PR only clears the compaction watermark in
compactMCPConversationForBudget but misses two other failure paths; locate the
catch/throw sites in directProviderGeneration and createMCPStream where
ContextBudgetExceededError is thrown or re-thrown and delete the compaction
watermark there using the same symbol pattern (call
this.lastCompactionMessageCount.delete(compactionSessionId)) before re-throwing
so handleContextOverflow can retry compaction; ensure you reference
compactionSessionId and ContextBudgetExceededError in each fix and run pnpm run
check to verify no errors.
In `@src/lib/neurolink.ts`:
- Around line 1745-1749: withTimeout currently only races Promise.all(writeOps)
so client.add(...) calls keep running after the timeout; to fix this, make the
underlying write ops cancellable or use transport-level timeouts: update the
write flow that builds writeOps to use a cancellable pattern (e.g., an
AbortController/AbortSignal passed into each client.add call or a client.add
timeout option) and ensure when withTimeout triggers you call abort() on that
controller (or close/reset the Redis connection) so the in-flight client.add
operations are actually stopped; change the code that creates writeOps and the
client.add invocation to accept and honor the abort/timeout signal so the
background memory writes are truly bounded.
---
Duplicate comments:
In `@docs/superpowers/plans/2026-04-09-curator-production-fixes.md`:
- Around line 214-233: getTimeout currently hardcodes the "generate" operation
causing streaming paths to get wrong timeouts; change getTimeout signature to
accept an operation parameter (e.g., operation: "generate" | "stream") or infer
it from whether options is StreamOptions, then use that operation when calling
getDefaultTimeout(this.providerName, operation). Update all callers (streaming
methods like those in anthropic.ts, openAI.ts, openRouter.ts, mistral.ts and any
BaseProvider.stream/executeStream invocations) to pass "stream" for streaming
paths (and "generate" for non-streaming calls or keep default behavior),
ensuring TextGenerationOptions/StreamOptions callers are adjusted accordingly.
---
Nitpick comments:
In `@docs/superpowers/plans/2026-04-09-curator-production-fixes.md`:
- Around line 482-514: The current auto-detection uses the internal
_registeredSpanProcessors array and a fragile constructor name check
(hasExistingLangfuse) which can break under minification; update the detection
in the provider registration block to prefer a public SDK query if available,
otherwise broaden the predicate to check for Langfuse-specific public members
(e.g., presence of a flush method or a langfuseClient property) on each
processor instead of relying solely on p.constructor?.name, and add a short
comment next to _registeredSpanProcessors/hasExistingLangfuse explaining the
fragility and why the fallback checks are needed; keep the existing skipLangfuse
and logging behavior unchanged and still gate adding langfuseProcessor via
provider.addSpanProcessor when appropriate.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f397c672-7e40-4fc4-b24c-fab72829a63b
📒 Files selected for processing (8)
docs/superpowers/plans/2026-04-09-curator-production-fixes.mdsrc/lib/context/contextCompactor.tssrc/lib/context/stages/slidingWindowTruncator.tssrc/lib/core/modules/Utilities.tssrc/lib/mcp/externalServerManager.tssrc/lib/neurolink.tssrc/lib/services/server/ai/observability/instrumentation.tssrc/lib/types/externalMcp.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/lib/context/stages/slidingWindowTruncator.ts
- src/lib/types/externalMcp.ts
- src/lib/services/server/ai/observability/instrumentation.ts
Comment on lines
+29
to
+88
| ### Task 1: Context Compaction — Lower Prune Threshold and Clear Watermark on Failure [CRITICAL] | ||
|
|
||
| **Problem:** `pruneMinimumSavings: 20_000` means Stage 1 skips pruning unless it can save 20K+ tokens. In typical Curator conversations (no tool outputs, no file duplicates), stages 1-2 do nothing, stage 3 summarization fails silently, and stage 4 truncation may not be aggressive enough. The watermark check also prevents re-compaction on the `handleContextOverflow` recovery path since message count hasn't grown. | ||
|
|
||
| **Files:** | ||
|
|
||
| - Modify: `src/lib/context/contextCompactor.ts:47` (pruneMinimumSavings default) | ||
| - Modify: `src/lib/neurolink.ts:5342` (watermark set) and `~5401` (throw path) | ||
|
|
||
| - [ ] **Step 1: Lower pruneMinimumSavings from 20_000 to 500** | ||
|
|
||
| In `src/lib/context/contextCompactor.ts`, change line 47: | ||
|
|
||
| ```typescript | ||
| // Before | ||
| pruneMinimumSavings: 20_000, | ||
|
|
||
| // After | ||
| pruneMinimumSavings: 500, | ||
| ``` | ||
|
|
||
| This allows Stage 1 to prune tool outputs even when savings are modest (500+ tokens). The 20K threshold was too conservative — in production, even small savings compound across the pipeline. | ||
|
|
||
| - [ ] **Step 2: Clear watermark when compaction fails to bring context under budget** | ||
|
|
||
| In `src/lib/neurolink.ts`, in `compactMCPConversationForBudget`, the watermark is set at line ~5342 regardless of whether compaction succeeds. When the final budget check fails and throws `ContextBudgetExceededError`, the watermark is stale — subsequent `handleContextOverflow` recovery calls see "already compacted this message count" and skip compaction. | ||
|
|
||
| Find the throw at line ~5402 (the `ContextBudgetExceededError` inside `compactMCPConversationForBudget`): | ||
|
|
||
| ```typescript | ||
| if (!finalBudget.withinBudget) { | ||
| throw new ContextBudgetExceededError( | ||
| ``` | ||
|
|
||
| Add a watermark clear before the throw: | ||
|
|
||
| ```typescript | ||
| if (!finalBudget.withinBudget) { | ||
| // Clear watermark so handleContextOverflow recovery can re-compact | ||
| this.lastCompactionMessageCount.delete(compactionSessionId); | ||
|
|
||
| throw new ContextBudgetExceededError( | ||
| ``` | ||
|
|
||
| - [ ] **Step 3: Verify build** | ||
|
|
||
| Run: `pnpm run check` | ||
| Expected: 0 errors | ||
|
|
||
| - [ ] **Step 4: Commit** | ||
|
|
||
| ```bash | ||
| git add src/lib/context/contextCompactor.ts src/lib/neurolink.ts | ||
| git commit -m "fix(context): lower prune threshold and clear watermark on compaction failure | ||
|
|
||
| pruneMinimumSavings 20K→500 so Stage 1 prunes tool outputs with modest savings. | ||
| Clear compaction watermark when budget check fails, allowing handleContextOverflow | ||
| recovery to re-compact instead of skipping (stale watermark)." | ||
| ``` | ||
|
|
There was a problem hiding this comment.
Critical: Task 1 incomplete — missing watermark clears in two other code paths.
The PR summary states the watermark must be cleared "in compactMCPConversationForBudget, directProviderGeneration, createMCPStream" (three locations), but Task 1 only shows the fix for compactMCPConversationForBudget. The other two failure paths (directProviderGeneration and createMCPStream) will still exhibit stale watermark behavior, leaving the production issue partially unresolved.
Add substeps to Task 1 covering:
- directProviderGeneration: Find where it calls compaction and catches
ContextBudgetExceededError, then clear the watermark before re-throwing. - createMCPStream: Similarly locate the compaction failure path and clear the watermark.
Each location should follow the same pattern as line 68:
this.lastCompactionMessageCount.delete(compactionSessionId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/superpowers/plans/2026-04-09-curator-production-fixes.md` around lines
29 - 88, The PR only clears the compaction watermark in
compactMCPConversationForBudget but misses two other failure paths; locate the
catch/throw sites in directProviderGeneration and createMCPStream where
ContextBudgetExceededError is thrown or re-thrown and delete the compaction
watermark there using the same symbol pattern (call
this.lastCompactionMessageCount.delete(compactionSessionId)) before re-throwing
so handleContextOverflow can retry compaction; ensure you reference
compactionSessionId and ContextBudgetExceededError in each fix and run pnpm run
check to verify no errors.
Comment on lines
+236
to
+259
| - [ ] **Step 3: Add dynamic timeout scaling for large contexts** | ||
|
|
||
| After the `getTimeout` method, add a new method for context-aware timeout: | ||
|
|
||
| ```typescript | ||
| /** | ||
| * Get timeout scaled by estimated input token count. | ||
| * For large contexts (>100K tokens), increase timeout proportionally. | ||
| */ | ||
| getContextAwareTimeout( | ||
| options: TextGenerationOptions | StreamOptions, | ||
| estimatedTokens?: number, | ||
| ): number { | ||
| const baseTimeout = this.getTimeout(options); | ||
|
|
||
| if (!estimatedTokens || estimatedTokens <= 100_000) { | ||
| return baseTimeout; | ||
| } | ||
|
|
||
| // Scale: >100K → 1.5x, >200K → 2x, >300K → 2.5x | ||
| const scale = 1 + Math.floor(estimatedTokens / 100_000) * 0.5; | ||
| return Math.round(baseTimeout * Math.min(scale, 4)); | ||
| } | ||
| ``` |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Integration step missing: getContextAwareTimeout() is defined but never called.
Task 3 adds the getContextAwareTimeout() method, but the plan doesn't show where to integrate it. The method won't be used unless call sites are updated to invoke it instead of getTimeout().
Add a substep to Task 3 showing where to call getContextAwareTimeout(). Likely candidates:
- Provider generate/stream methods where token counts are already estimated
- Context compaction flows where message token counts are computed
For example, if BaseProvider has token estimates before calling the underlying API, replace:
-const timeout = this.utilities.getTimeout(options);
+const timeout = this.utilities.getContextAwareTimeout(options, estimatedTokens);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/superpowers/plans/2026-04-09-curator-production-fixes.md` around lines
236 - 259, The new getContextAwareTimeout method is never invoked; update call
sites that currently use getTimeout to pass estimated token counts into
getContextAwareTimeout instead. Specifically, in provider entry points such as
BaseProvider.generate and BaseProvider.stream (and any provider-specific
overrides) replace calls like this.getTimeout(options) with
this.getContextAwareTimeout(options, estimatedTokens) where estimatedTokens is
the precomputed token count (e.g., from your context compaction/token estimation
flow or ContextCompactor/MessageTokenCounter results); also update any context
compaction flows that call getTimeout to use getContextAwareTimeout and thread
the estimatedTokens value through the call chain. Ensure all call signatures
supply the correct estimated token variable.
Comment on lines
+534
to
+607
| ### Task 6: AsyncLocalStorage Context Propagation [LOW] | ||
|
|
||
| **Problem:** `setImmediate()` at `neurolink.ts:1718` (memory write background op) loses AsyncLocalStorage context. The `runWithCurrentLangfuseContext` helper exists and is used in Redis/conversation memory managers but not here. Spans created inside this callback become orphaned Langfuse traces. | ||
|
|
||
| **Files:** | ||
|
|
||
| - Modify: `src/lib/neurolink.ts:1718` (setImmediate in saveToMemory) | ||
|
|
||
| - [ ] **Step 1: Import runWithCurrentLangfuseContext if not already imported** | ||
|
|
||
| In `src/lib/neurolink.ts`, check imports. If `runWithCurrentLangfuseContext` is not imported, add it to the existing observability import: | ||
|
|
||
| ```typescript | ||
| import { runWithCurrentLangfuseContext } from "./services/server/ai/observability/instrumentation.js"; | ||
| ``` | ||
|
|
||
| - [ ] **Step 2: Wrap setImmediate callback with context propagation** | ||
|
|
||
| In `src/lib/neurolink.ts`, find line 1718: | ||
|
|
||
| ```typescript | ||
| setImmediate(async () => { | ||
| try { | ||
| const client = this.ensureMemoryReady(); | ||
| ``` | ||
|
|
||
| Replace with: | ||
|
|
||
| ```typescript | ||
| const wrappedMemoryWrite = runWithCurrentLangfuseContext(async () => { | ||
| try { | ||
| const client = this.ensureMemoryReady(); | ||
| ``` | ||
|
|
||
| Then find the closing of this `setImmediate` callback (the matching `});`) and replace: | ||
|
|
||
| ```typescript | ||
| // Before | ||
| }); | ||
|
|
||
| // After | ||
| }); | ||
| setImmediate(wrappedMemoryWrite); | ||
| ``` | ||
|
|
||
| The full pattern becomes: | ||
|
|
||
| ```typescript | ||
| const wrappedMemoryWrite = runWithCurrentLangfuseContext(async () => { | ||
| try { | ||
| const client = this.ensureMemoryReady(); | ||
| // ... rest of the async body unchanged ... | ||
| } catch (error) { | ||
| // ... error handling unchanged ... | ||
| } | ||
| }); | ||
| setImmediate(wrappedMemoryWrite); | ||
| ``` | ||
|
|
||
| - [ ] **Step 3: Verify build** | ||
|
|
||
| Run: `pnpm run check` | ||
| Expected: 0 errors | ||
|
|
||
| - [ ] **Step 4: Commit** | ||
|
|
||
| ```bash | ||
| git add src/lib/neurolink.ts | ||
| git commit -m "fix(langfuse): preserve AsyncLocalStorage context in memory write background op | ||
|
|
||
| Wrap setImmediate callback in runWithCurrentLangfuseContext to prevent orphaned | ||
| Langfuse traces from background memory operations." | ||
| ``` | ||
|
|
There was a problem hiding this comment.
Major: Task 6 missing withTimeout wrapper for background memory writes.
The PR summary states "bounded background memory writes with ... withTimeout(..., 30_000)", the AI summary mentions "bounded background writes with a 30s timeout", and the verification checklist (line 658) checks for withTimeout.*writeOps, but Task 6's implementation steps (lines 550-591) only add runWithCurrentLangfuseContext() without the timeout wrapper.
Add missing step for withTimeout wrapper
After Step 2, add:
Step 2b: Add timeout wrapper to bound background writes
Import withTimeout if not already imported:
import { withTimeout } from "./utils/async.js"; // adjust path as neededWrap the async memory operation with a 30-second timeout:
const wrappedMemoryWrite = runWithCurrentLangfuseContext(async () => {
return withTimeout(async () => {
try {
const client = this.ensureMemoryReady();
// ... rest of memory write logic ...
} catch (error) {
// ... error handling ...
}
}, 30_000); // 30s timeout
});
setImmediate(wrappedMemoryWrite);This prevents hung Redis writes from blocking indefinitely.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/superpowers/plans/2026-04-09-curator-production-fixes.md` around lines
534 - 607, The saveToMemory background op using setImmediate in neurolink.ts
must both preserve AsyncLocalStorage and be bounded by a 30s timeout: import
runWithCurrentLangfuseContext and withTimeout (if missing), wrap the async
memory write callback (the block starting at setImmediate(async () => { ... }
that calls this.ensureMemoryReady() and performs memory writes) by first
wrapping the body with withTimeout(..., 30_000) and then wrap that with
runWithCurrentLangfuseContext, assign to a symbol like wrappedMemoryWrite, and
call setImmediate(wrappedMemoryWrite) instead of setImmediate(async () => ...);
ensure error handling inside remains unchanged and run pnpm run check to verify
zero errors.
Comment on lines
+1745
to
+1749
| await withTimeout( | ||
| Promise.all(writeOps), | ||
| 30_000, | ||
| new Error("Background memory write timed out after 30s"), | ||
| ); |
There was a problem hiding this comment.
withTimeout here does not actually stop hung memory writes.
withTimeout() only races Promise.all(writeOps); the underlying client.add(...) calls keep running after 30s. That means this still logs a timeout without actually bounding the stuck Redis/I/O work the PR is trying to fix. The underlying write path needs cancellation or transport-level timeouts, not just a raced wrapper.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/neurolink.ts` around lines 1745 - 1749, withTimeout currently only
races Promise.all(writeOps) so client.add(...) calls keep running after the
timeout; to fix this, make the underlying write ops cancellable or use
transport-level timeouts: update the write flow that builds writeOps to use a
cancellable pattern (e.g., an AbortController/AbortSignal passed into each
client.add call or a client.add timeout option) and ensure when withTimeout
triggers you call abort() on that controller (or close/reset the Redis
connection) so the in-flight client.add operations are actually stopped; change
the code that creates writeOps and the client.add invocation to accept and honor
the abort/timeout signal so the background memory writes are truly bounded.
murdore
force-pushed
the
fix/curator-production-fixes
branch
from
April 9, 2026 19:17
042e341 to
69bdbfe
Compare
Contributor
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
Contributor
Author
|
@coderabbitai review Please verify each change freshly against the current code. Specifically check: Context Compaction:
Timeout: MCP Events: Langfuse: |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 12991
✅ Context Compaction (5/5)
✅ Timeout (4/4)
✅ MCP Events (4/4)
✅ Langfuse (4/4)
All 17 checks pass. The code matches every specification exactly as described. 🎉 🧠 Learnings used |
… Langfuse Fixes 6 production issues from Curator monitoring whose root cause is in the NeuroLink SDK: token overflow compaction (prune threshold, watermark, graduated truncation), Vertex timeout per-provider defaults, MCP event serverName field, Langfuse duplicate span auto-detection, and AsyncLocalStorage context propagation.
murdore
force-pushed
the
fix/curator-production-fixes
branch
from
April 9, 2026 19:47
69bdbfe to
1fce851
Compare
Contributor
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
Contributor
|
🎉 This PR is included in version 9.50.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This was referenced Apr 17, 2026
This was referenced Apr 25, 2026
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes 6 production issues identified across 4 Curator monitoring cycles (2.27M telemetry rows, 10.3 GB, Mar 18 – Apr 8 2026) where the root cause lives in the NeuroLink SDK, not in Curator.
Root Cause
Production telemetry from Curator (AI Slack assistant using NeuroLink as core engine) revealed six compounding issues:
Token overflow — compaction pipeline ineffective — `pruneMinimumSavings: 20_000` blocked Stage 1 from pruning tool outputs unless savings exceeded 20K tokens. In typical Curator conversations, stages 1-2 found nothing, stage 3 summarization failed silently, and stage 4 truncation used coarse 25%/iteration jumps (3 max). Evidence: 207K tokens vs 200K limit, compaction removed ~5 tokens, 3 wasted retry attempts per overflow.
Compaction watermark prevented recovery — After compaction failed and threw `ContextBudgetExceededError`, the `lastCompactionMessageCount` watermark was stale in all three paths (`compactMCPConversationForBudget`, `directProviderGeneration`, `createMCPStream`). Subsequent `handleContextOverflow` recovery calls saw "already compacted" and skipped re-compaction.
Vertex AI timeout too short — `BaseProvider.defaultTimeout = 30000` (30s) was hardcoded. `DEFAULT_TIMEOUTS.providers.vertex = "60s"` existed in `timeout.ts` as dead code — `Utilities.getTimeout()` never called `getDefaultTimeout()`. Gemini p90 latency for large contexts (69K-353K tokens) is 54.5s. Evidence: 21 timeout errors in 5-day period. Additionally, no dynamic scaling existed for large contexts.
MCP events missing serverName — All `externalMCP:*` events carried `serverId` but not `serverName`. The `removeServer` path was especially broken — it called `getServerName()` after `servers.delete()` had already destroyed the instance.
Duplicate Langfuse spans — When both NeuroLink and the consumer registered a `LangfuseSpanProcessor`, duplicate spans were exported. No auto-detection existed.
AsyncLocalStorage context loss — `setImmediate()` in `storeMemoryInBackground` lost AsyncLocalStorage context. Background memory writes appeared as orphaned root spans in Langfuse. Also lacked a timeout, so hung Redis connections could block indefinitely.
Changes
Context Compaction (FIX 1-2):
Timeout (FIX 3):
MCP Events (FIX 4):
Langfuse Observability (FIX 5-6):
Verified — No Change Needed
Files Changed
Test Plan