fix(mcp): large-response externalization, retrieve_context tool, and exec cleanup - #942
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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 |
WalkthroughAdds an artifact-backed MCP output-normalization pipeline: large tool outputs can be externalized to a local temp artifact store, surrogates are returned in-tool outputs, retrieval tools support fetching/pagination, and CLI/tool execution paths are updated to use the normalizer and store. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client / CLI
participant ToolRegistry as Tool Registry / Tool
participant Normalizer as McpOutputNormalizer
participant ArtifactStore as LocalTempArtifactStore
participant MemoryTool as retrieve_context / Memory Tool
Client->>ToolRegistry: execute tool
ToolRegistry->>ToolRegistry: callTool() -> callResult
ToolRegistry->>Normalizer: normalize(callResult, {toolName, serverId})
Normalizer->>Normalizer: serialize & compute originalBytes
alt originalBytes <= warnBytes
Normalizer-->>ToolRegistry: return raw result (isExternalized=false)
else warnBytes < originalBytes <= maxBytes
Normalizer->>Normalizer: log warning
Normalizer-->>ToolRegistry: return raw result (isExternalized=false)
else originalBytes > maxBytes
alt strategy == "inline"
Normalizer-->>ToolRegistry: return raw result (isExternalized=false)
else strategy == "externalize"
Normalizer->>ArtifactStore: store(serializedPayload, meta)
ArtifactStore->>ArtifactStore: write temp file, generate preview
ArtifactStore-->>Normalizer: ArtifactRef(id,...)
Normalizer-->>ToolRegistry: return surrogate result with _meta.artifactId (isExternalized=true)
end
end
ToolRegistry-->>Client: final result (surrogate or raw)
%% retrieval flow
Client->>MemoryTool: retrieve_context(artifactId, offset, limit)
MemoryTool->>ArtifactStore: retrieve(artifactId)
ArtifactStore-->>MemoryTool: full payload
MemoryTool->>MemoryTool: apply offset/limit, compute pagination metadata
MemoryTool-->>Client: sliced content + pagination info
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (2)
src/lib/mcp/toolDiscoveryService.ts (1)
541-555: Wrap normalization call withwithTimeoutto align with repo async-error policy.The new async
normalize()path should be time-bounded the same way as other MCP operations.Suggested fix
- const normalized = await this.outputNormalizer.normalize( - callResult, - { toolName, serverId }, - ); + const normalized = await withTimeout( + this.outputNormalizer.normalize(callResult, { + toolName, + serverId, + }), + timeout, + new Error(`MCP output normalization timeout: ${toolName}`), + );As per coding guidelines: "Use
withTimeoututility to wrap async calls for error handling".🤖 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 541 - 555, Wrap the async call to this.outputNormalizer.normalize with the repository's withTimeout utility so normalization is time-bounded like other MCP ops: replace await this.outputNormalizer.normalize(callResult, { toolName, serverId }) with await withTimeout(this.outputNormalizer.normalize(callResult, { toolName, serverId }), /* appropriate timeout */) (or the pattern used elsewhere), ensure you import/use withTimeout, preserve assignment to normalized and the subsequent use of normalized.isExternalized, normalized.originalBytes and normalized.result, and keep the callSpan.setAttribute calls unchanged.src/lib/artifacts/artifactStore.ts (1)
46-47: MoveIndexEntryinto the canonical types module.This source-local type breaks the repo’s type-centralization rule. Keeping it beside
ArtifactMeta/ArtifactRefinsrc/lib/types/artifactTypes.tswill prevent type drift as more artifact backends get added.Based on learnings, "Project standard: Place reusable/shared types under src/lib/types/.ts; test-only helper types under test/types/.ts; avoid declaring local types inside source implementation files."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/artifacts/artifactStore.ts` around lines 46 - 47, The local type IndexEntry declared in artifactStore.ts should be moved into the canonical artifact types module where ArtifactMeta and ArtifactRef live: create and export type IndexEntry = ArtifactMeta & { path: string } in the artifactTypes module, replace the local declaration in artifactStore.ts with an import of IndexEntry, and update any references to use the imported type; ensure the export name matches exactly (IndexEntry) and run TS checks to confirm no remaining local declarations or import errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cli/commands/mcp.ts`:
- Around line 963-968: The call to toolRegistry.executeTool is unbounded and can
hang; wrap the promise returned by toolRegistry.executeTool(toolName, params,
{...}) with the existing withTimeout utility (use the same timeout value pattern
used elsewhere in this file/module), ensure you import/require withTimeout if
not already imported, and propagate or handle the timeout error the same way
other commands do (e.g., surface an error message/exit) so mcp exec cannot hang
indefinitely.
- Around line 932-945: Replace the raw synchronous process.exit calls and the
unbounded toolRegistry.executeTool invocation: wrap the call to
toolRegistry.executeTool(...) with the project's withTimeout helper to enforce a
max duration and handle timeout errors, and before exiting (both in the normal
path after await sdk.shutdown() and in the catch block) flush stdout/stderr
asynchronously and wait for those writes to complete (ensure any pending stdio
writes finish) before calling process.exit(exitCode); keep using
sdk.shutdown().catch(() => undefined) but only call process.exit after awaiting
stdio drains so output isn’t truncated.
In `@src/lib/artifacts/artifactStore.ts`:
- Around line 93-99: The artifact directory is created with default permissions
while files are 0o600; ensure the directory is hardened (e.g., 0o700) so it's
not listable by other users: when creating the directory in artifactStore (the
mkdir call that creates this.dir) pass a restrictive mode (0o700) and follow up
with an explicit fs.chmod(this.dir, 0o700) after mkdir to ensure the mode even
if umask interferes; leave file write (writeFile with mode 0o600) unchanged.
In `@src/lib/mcp/mcpOutputNormalizer.ts`:
- Around line 130-138: The call to this.artifactStore.store within normalize()
can hang and must be wrapped with the withTimeout utility; change the code that
awaits this.artifactStore.store(serialized, {...}) to instead call
withTimeout(this.artifactStore.store(serialized, {...}), timeoutMs) (choose the
repository's standard timeout constant or configuration) and handle timeout
errors consistently (reject/throw or log as other callers expect). Ensure you
import/require withTimeout if not already present and keep the rest of the ref
assignment (ref: ArtifactRef) and error propagation behavior the same.
- Around line 204-210: serialize currently uses JSON.stringify(value, null, 2)
which inflates byte counts and misleads originalBytes/warnBytes/maxBytes checks;
change serialize to produce the compact form (JSON.stringify(value) without
spacing) so size enforcement measures the true payload size, and if a
pretty-printed artifact is still desired create a separate serializer (e.g.,
serializePretty or persistPretty) used only when persisting human-readable
output; update places that set originalBytes or compare against
warnBytes/maxBytes to use serialize for measurement and use the pretty
serializer only for storage/logging.
In `@src/lib/memory/memoryRetrievalTools.ts`:
- Line 125: artifactStore.retrieve(...) and memoryManager.getSessionRaw(...) are
unbounded and must be wrapped with the withTimeout utility so retrievals fail
fast; update the calls in retrieve_context (where const content = await
artifactStore.retrieve(args.artifactId)) and the other occurrence around
memoryManager.getSessionRaw(...) to use withTimeout(promise, timeoutMs)
(introduce a clear timeout constant like RETRIEVE_TIMEOUT if not present) and
handle the timeout error path consistently (log/throw a descriptive error) so
stalled filesystem/Redis calls fail cleanly.
In `@src/lib/neurolink.ts`:
- Around line 1537-1555: The execute handler currently calls
tools.retrieve_context.execute directly (inside the execute async function and
after createMemoryRetrievalTools) which can block on Redis/artifact I/O; wrap
that call with the withTimeout utility so the retrieval promise is bounded.
Locate the call to tools.retrieve_context.execute and replace the direct await
with await withTimeout(() => (tools.retrieve_context.execute as (params:
unknown, ctx: unknown) => Promise<unknown>)(params, { toolCallId:
"memory-retrieval", messages: [] }), <sensible timeout ms>); ensure withTimeout
is imported/available and surface or rethrow the timeout error consistently from
the execute method.
In `@src/lib/session/globalSessionState.ts`:
- Around line 29-35: The numeric env parsing currently drops valid zero and
allows negatives; update the parse logic for maxBytes and warnBytes so you still
parse with parseInt(maxBytesRaw, 10)/parseInt(warnBytesRaw, 10) but then include
each value only if Number.isFinite(parsed) and parsed >= 0 (this preserves 0 and
rejects negatives/NaN). Replace the conditional spreads that use (maxBytes &&
!isNaN(maxBytes)) and (warnBytes && !isNaN(warnBytes)) with checks like
Number.isFinite(maxBytes) && maxBytes >= 0 and similarly for warnBytes so the
returned object only contains valid non-negative integers.
In `@test/continuous-test-suite-mcp-output-limits.ts`:
- Around line 63-64: The test creates a top-level testDir but other
LocalTempArtifactStore instances create nl-s* dirs directly under tmpdir(),
causing leaks; update every LocalTempArtifactStore construction to use the same
underTestDir(...) pattern (i.e., derive their base path from testDir) so their
artifacts live under testDir, and ensure the final rm(testDir, { recursive:
true, force: true }) call removes everything; look for uses of
LocalTempArtifactStore and the helper underTestDir and change the instances that
currently pass tmpdir() or bare names to instead nest under testDir.
---
Nitpick comments:
In `@src/lib/artifacts/artifactStore.ts`:
- Around line 46-47: The local type IndexEntry declared in artifactStore.ts
should be moved into the canonical artifact types module where ArtifactMeta and
ArtifactRef live: create and export type IndexEntry = ArtifactMeta & { path:
string } in the artifactTypes module, replace the local declaration in
artifactStore.ts with an import of IndexEntry, and update any references to use
the imported type; ensure the export name matches exactly (IndexEntry) and run
TS checks to confirm no remaining local declarations or import errors.
In `@src/lib/mcp/toolDiscoveryService.ts`:
- Around line 541-555: Wrap the async call to this.outputNormalizer.normalize
with the repository's withTimeout utility so normalization is time-bounded like
other MCP ops: replace await this.outputNormalizer.normalize(callResult, {
toolName, serverId }) with await
withTimeout(this.outputNormalizer.normalize(callResult, { toolName, serverId }),
/* appropriate timeout */) (or the pattern used elsewhere), ensure you
import/use withTimeout, preserve assignment to normalized and the subsequent use
of normalized.isExternalized, normalized.originalBytes and normalized.result,
and keep the callSpan.setAttribute calls unchanged.
🪄 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: 4daa2d08-a11f-4c2e-a3af-da4f2110831f
📒 Files selected for processing (15)
src/cli/commands/mcp.tssrc/lib/artifacts/artifactStore.tssrc/lib/core/redisConversationMemoryManager.tssrc/lib/mcp/externalServerManager.tssrc/lib/mcp/mcpOutputNormalizer.tssrc/lib/mcp/toolDiscoveryService.tssrc/lib/memory/memoryRetrievalTools.tssrc/lib/neurolink.tssrc/lib/session/globalSessionState.tssrc/lib/types/artifactTypes.tssrc/lib/types/configTypes.tssrc/lib/types/conversation.tssrc/lib/types/index.tssrc/lib/types/mcpOutputTypes.tstest/continuous-test-suite-mcp-output-limits.ts
pdogra1299
force-pushed
the
fix/mcp-output-limits-externalization
branch
from
April 11, 2026 17:28
042ee7f to
3d0dd70
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 |
…exec cleanup - Add McpOutputNormalizer to intercept MCP tool results above maxBytes threshold and write them to LocalTempArtifactStore (/tmp/neurolink-artifacts/) returning a surrogate with neurolinkArtifactId for LLM reference - Register retrieve_context as a user-defined tool (not built-in) so ToolsManager includes it in the LLM schema; fix gate to activate on artifact store presence, not only Redis - Wire NEUROLINK_MCP_OUTPUT_STRATEGY / NEUROLINK_MCP_MAX_OUTPUT_BYTES / NEUROLINK_MCP_WARN_OUTPUT_BYTES env vars into CLI session via buildMcpOutputLimitsFromEnv() in globalSessionState - Fix mcp exec --output: replace direct process.exit() calls with try/finally sdk.shutdown() + unconditional process.exit(exitCode) to close stdio child processes and prevent event-loop hang - Extract runExecTool() helper to stay within ESLint max-depth limit - Fix dead span attribute: mcp.output.strategy now set unconditionally (inline vs externalize) outside the isExternalized guard - Add memoryRetrievalTools optional memoryManager param so artifact retrieval works without Redis; add Section 6 unit tests (24 total) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
murdore
force-pushed
the
fix/mcp-output-limits-externalization
branch
from
April 12, 2026 02:38
3d0dd70 to
6166211
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 |
murdore
approved these changes
Apr 12, 2026
Contributor
|
@coderabbitai do full review evaluating all the changes and highlighting anything that has been missed. |
🧩 Analysis chain🏁 Script executed: Length of output: 42641 🏁 Script executed: Length of output: 26772 🏁 Script executed: Length of output: 13149
PR
|
| Missing test | Risk |
|---|---|
Multi-byte UTF-8 payload: verify totalSize matches bytes, not chars |
Medium — wrong pagination for Unicode-heavy payloads |
warnBytes >= maxBytes misconfiguration |
Medium — warn branch fires for everything, externalization never triggers |
maxBytes=0 in env |
Low — externalizes every tool call |
Concurrent store() calls on same dir |
Low — unlikely race on UUID collision |
cleanup() with very large index (100+ entries) |
Low — confirms parallelism fix |
Artifact not found after delete() then retrieve() |
Low — regression guard |
✅ What Is Correct
executeExeclifecycle: the earlyprocess.exit(1)calls forserverName/toolNameoccur beforenew NeuroLink(), so no SDK shutdown is needed for those branches. The innertry/finallycorrectly covers all SDK work.normalize()never-throws contract is solid — every error path returns raw result with a log.withTimeoutguards on bothstore()andretrieve()prevent hangs.retrieve_contextregistered as user-defined (not built-in) so it reaches the LLM tool schema — the fix is correct.- The regex ReDoS mitigation (200-char limit + no
gflag) is well-handled. - Per the retrieved learning: the intentional deferral of
sessionIdvalidation against artifact metadata is appropriate for the current single-process deployment model.
🧠 Learnings used
Learnt from: swaroopvarma1
Repo: juspay/neurolink PR: 913
File: src/lib/tasks/taskExecutor.ts:171-175
Timestamp: 2026-03-30T13:39:41.202Z
Learning: In `src/lib/neurolink.ts`, `generate()` returns `toolExecutions` normalized via `transformToolExecutions()` (from `src/lib/utils/transformationUtils.ts`), which produces objects with `{ name: string, input: StandardRecord, output: unknown, duration: number }`. A separate `transformToolExecutionsForMCP()` function returns `{ toolName, executionTime, success }` and is only used in the MCP code path (`tryMCPGeneration`). Do not confuse the two.
Learnt from: pdogra1299
Repo: juspay/neurolink PR: 942
File: src/lib/memory/memoryRetrievalTools.ts:113-152
Timestamp: 2026-04-11T17:32:35.237Z
Learning: In `src/lib/memory/memoryRetrievalTools.ts`, the `retrieve_context` tool's `artifactId` path does not validate `sessionId` against the artifact's stored metadata. This is intentional for the current scope: `LocalTempArtifactStore` (in `src/lib/artifacts/artifactStore.ts`) uses a process-local in-memory index, so artifact IDs from one process are not resolvable in another — cross-session leakage is not possible in the single-process CLI/SDK deployment. Session validation against artifact metadata is a deferred hardening item for future multi-tenant server deployments where a shared/persistent `ArtifactStore` backend (e.g., Redis, S3) might be used.
✅ Actions performed
Full review triggered.
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (4)
src/lib/types/configTypes.ts (1)
137-144: Reuse the canonicalMcpOutputStrategyhere.
src/lib/types/mcpOutputTypes.tsalready owns the public strategy union for this feature. Re-declaring it inMCPEnhancementsConfigmeans the config surface can drift from the normalizer contract later.♻️ Minimal refactor
+import type { McpOutputStrategy } from "./mcpOutputTypes.js"; ... outputLimits?: { /** What to do when output exceeds maxBytes. Default: "externalize". */ - strategy?: "inline" | "externalize"; + strategy?: McpOutputStrategy; /** Byte ceiling above which the strategy fires. Default: 102400 (100 KB). */ maxBytes?: number; /** Bytes at which a warning is emitted even when still inline. Default: 51200 (50 KB). */ warnBytes?: number; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/types/configTypes.ts` around lines 137 - 144, Replace the inline union type for outputLimits.strategy with the canonical McpOutputStrategy type from the mcp output types file: update the outputLimits interface to import and use McpOutputStrategy instead of re-declaring "inline" | "externalize", remove the local union declaration, and ensure an import for McpOutputStrategy is added at the top of the file so MCPEnhancementsConfig's strategy signature stays in sync with the normalizer contract.src/cli/commands/mcp.ts (1)
967-967: Consider importingWARN_BYTESfrom a shared constant.The
50 * 1024threshold is hardcoded here but may duplicate a constant defined elsewhere (e.g., in the MCP output limits module). Importing a shared constant would reduce drift if the threshold is adjusted.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/commands/mcp.ts` at line 967, Replace the hardcoded constant WARN_BYTES (currently set to 50 * 1024) with the shared constant exported by the MCP output limits module: remove the local const WARN_BYTES and import the canonical constant (named the same or as exported, e.g., WARN_BYTES/OUTPUT_WARN_BYTES) from the shared module used elsewhere in the codebase; update any references in this file to use that imported symbol so the threshold is centralized and won't drift.src/lib/memory/memoryRetrievalTools.ts (1)
109-153: Artifact retrieval path lacks observability span.The Redis-backed path (lines 171-180) creates a span for
memory.retrieve, but the artifact retrieval branch here has no span or metrics. This makes it harder to trace and monitor artifact retrievals in production.Proposed fix — add span for artifact retrieval
if (args.artifactId) { if (!artifactStore) { // ... existing error handling ... } + const span = SpanSerializer.createSpan( + SpanType.MEMORY, + "memory.retrieve.artifact", + { + "memory.operation": "retrieve", + "memory.store": "artifact", + "artifact.id": args.artifactId, + }, + ); + const startTime = Date.now(); const content = await withTimeout( artifactStore.retrieve(args.artifactId), 10_000, new Error( `ArtifactStore.retrieve() timed out for artifact "${args.artifactId}"`, ), ); if (content === null) { + span.durationMs = Date.now() - startTime; + const endedSpan = SpanSerializer.endSpan(span, SpanStatus.OK); + getMetricsAggregator().recordSpan(endedSpan); return { error: "Artifact not found or has expired", artifactId: args.artifactId, }; } // ... rest of logic ... + span.durationMs = Date.now() - startTime; + const endedSpan = SpanSerializer.endSpan(span, SpanStatus.OK); + getMetricsAggregator().recordSpan(endedSpan); return { /* ... */ }; }🤖 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 109 - 153, Add the same observability span used by the Redis path around the artifact retrieval branch: start a "memory.retrieve" span (or "memory.retrieve.artifact") before calling artifactStore.retrieve(args.artifactId), set attributes like retrieval.type="artifact" and artifact_id=args.artifactId, record errors if withTimeout or retrieve throws, and end the span in a finally block; update any metric/timing logic to mirror the Redis-backed path so artifact retrievals are traced consistently with the existing memory.retrieve span usage.src/lib/artifacts/artifactStore.ts (1)
82-87: Keep preview truncation byte-based.
generatePreview()trims by UTF-16 code units, while the rest of this feature enforces limits in UTF-8 bytes. Multibyte payloads can still produce previews larger than the intended budget. Reusing the byte-aware preview logic from the normalizer path would keep this consistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/artifacts/artifactStore.ts` around lines 82 - 87, generatePreview currently truncates by UTF-16 code units using payload.length and DEFAULT_PREVIEW_CHARS, which can exceed the UTF-8 byte budget; change it to truncate by UTF-8 bytes instead. Replace the simple slice logic in generatePreview with the byte-aware truncation used in the normalizer path (reuse the normalizer's byte-truncation algorithm or helper), ensuring the returned string is the longest prefix whose UTF-8 encoding is <= DEFAULT_PREVIEW_CHARS bytes and append the ellipsis when truncated. Keep the function signature generatePreview(payload: string): string and preserve the existing ellipsis semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/artifacts/artifactStore.ts`:
- Around line 75-80: The temp-directory usage leaks artifacts; change
ArtifactStore to isolate each process/session by creating a unique subdirectory
instead of writing directly to join(tmpdir(), "neurolink-artifacts") (modify the
constructor to append a per-process/session id such as process.pid + timestamp
or a UUID to that path), and/or implement startup pruning by scanning the base
"neurolink-artifacts" folder to rebuild the in-memory index or remove files not
belonging to any valid session; update the constructor and the cleanup() logic
(and any index-loading routine referenced around lines 156-168) so cleanup only
touches files in this instance's subdir and startup code either rebuilds index
entries from files in the subdir or prunes stale files in the base directory.
In `@src/lib/mcp/toolDiscoveryService.ts`:
- Around line 541-544: The output normalizer call in ToolDiscoveryService (the
call to this.outputNormalizer.normalize with { toolName, serverId }) omits
context.sessionId so artifacts lose session association; update the call to
include sessionId (e.g., { toolName, serverId, sessionId }) by pulling sessionId
from the surrounding execution context/options, and then propagate sessionId
through the shared execution options types and all callers of
ExternalServerManager.executeTool() so executeTool and outputNormalizer receive
and forward sessionId consistently.
In `@src/lib/memory/memoryRetrievalTools.ts`:
- Around line 145-152: The returned metadata uses character counts but
downstream logic (McpOutputNormalizer) expects bytes; compute and return
byte-based values instead: compute totalSize as Buffer.byteLength(content,
'utf8'), produce the sliced content using Buffer.from(content,
'utf8').slice(startByte, startByte + byteLimit).toString('utf8'), and make
hasMore compare startByte + byteLimit < totalSize; update offset and limit to
represent byte offsets/limits (use the same startByte and byteLimit variables
instead of start and charLimit). Update the block that returns artifactId,
content, totalSize, hasMore, offset, and limit to use these byte-based values so
pagination metadata is consistent with McpOutputNormalizer.
In `@src/lib/neurolink.ts`:
- Around line 1542-1548: The code narrows this.conversationMemory to
RedisConversationMemoryManager without verifying its runtime type, so when
initializeConversationMemoryForGeneration() falls back to
ConversationMemoryManager after Redis init fails, createMemoryRetrievalTools()
may receive the wrong instance and throw; fix by checking the concrete type
before casting (e.g., test that this.conversationMemory is an instance of
RedisConversationMemoryManager) and only pass a Redis-backed memoryManager into
createMemoryRetrievalTools(), otherwise pass undefined or skip creating
retrieval tools; adjust the code around createMemoryRetrievalTools,
conversationMemory, and initializeConversationMemoryForGeneration to use that
guard.
- Around line 1342-1346: Add artifact-store cleanup to NeuroLink.dispose by
detecting this.mcpArtifactStore (set when strategy === "externalize" and
assigned a LocalTempArtifactStore), calling its cleanup(olderThanMs) inside a
withTimeout wrapper (e.g., 24*60*60*1000 ms threshold, 5000 ms timeout), logging
start, success (including number of removed files), and catching/logging errors,
then setting this.mcpArtifactStore = undefined; follow the same try/catch and
logger patterns used for mcpToolResultCache/mcpToolRouter/mcpToolBatcher
cleanup.
In `@src/lib/session/globalSessionState.ts`:
- Around line 24-27: The current assignment silently coerces any invalid
NEUROLINK_MCP_OUTPUT_STRATEGY value into "externalize" via the
strategy/strategyRaw logic; change it so missing env and invalid env are handled
separately: read strategyRaw, if it's undefined/null keep strategy undefined (or
respect the existing maxBytes-only default path), but if strategyRaw is present
and not one of the allowed McpOutputStrategy values ("inline" | "externalize")
then surface the misconfiguration (throw or processLogger.error and exit)
instead of falling back to "externalize". Update the logic around the
strategy/strategyRaw variables in globalSessionState.ts and reference
NEUROLINK_MCP_OUTPUT_STRATEGY and the McpOutputStrategy type when implementing
the explicit validation.
In `@test/continuous-test-suite-mcp-output-limits.ts`:
- Around line 52-54: The tests currently measure sizes using string length
(payload.length) which assumes 1 byte per char; update the test assertions to
measure UTF-8 bytes instead by using Buffer.byteLength(payload, "utf8") wherever
totalSize or size assertions are made (references: makePayload, totalSize, and
the retrieve_context-related assertions), and modify makePayload or add at least
one test case that uses a non-ASCII payload (e.g., include "é" or an emoji) so
the suite will catch multibyte regressions; ensure all occurrences noted (around
the blocks 52-54, 394-421, 423-449) are changed from .length to
Buffer.byteLength(..., "utf8") and the expectations are adjusted accordingly.
---
Nitpick comments:
In `@src/cli/commands/mcp.ts`:
- Line 967: Replace the hardcoded constant WARN_BYTES (currently set to 50 *
1024) with the shared constant exported by the MCP output limits module: remove
the local const WARN_BYTES and import the canonical constant (named the same or
as exported, e.g., WARN_BYTES/OUTPUT_WARN_BYTES) from the shared module used
elsewhere in the codebase; update any references in this file to use that
imported symbol so the threshold is centralized and won't drift.
In `@src/lib/artifacts/artifactStore.ts`:
- Around line 82-87: generatePreview currently truncates by UTF-16 code units
using payload.length and DEFAULT_PREVIEW_CHARS, which can exceed the UTF-8 byte
budget; change it to truncate by UTF-8 bytes instead. Replace the simple slice
logic in generatePreview with the byte-aware truncation used in the normalizer
path (reuse the normalizer's byte-truncation algorithm or helper), ensuring the
returned string is the longest prefix whose UTF-8 encoding is <=
DEFAULT_PREVIEW_CHARS bytes and append the ellipsis when truncated. Keep the
function signature generatePreview(payload: string): string and preserve the
existing ellipsis semantics.
In `@src/lib/memory/memoryRetrievalTools.ts`:
- Around line 109-153: Add the same observability span used by the Redis path
around the artifact retrieval branch: start a "memory.retrieve" span (or
"memory.retrieve.artifact") before calling
artifactStore.retrieve(args.artifactId), set attributes like
retrieval.type="artifact" and artifact_id=args.artifactId, record errors if
withTimeout or retrieve throws, and end the span in a finally block; update any
metric/timing logic to mirror the Redis-backed path so artifact retrievals are
traced consistently with the existing memory.retrieve span usage.
In `@src/lib/types/configTypes.ts`:
- Around line 137-144: Replace the inline union type for outputLimits.strategy
with the canonical McpOutputStrategy type from the mcp output types file: update
the outputLimits interface to import and use McpOutputStrategy instead of
re-declaring "inline" | "externalize", remove the local union declaration, and
ensure an import for McpOutputStrategy is added at the top of the file so
MCPEnhancementsConfig's strategy signature stays in sync with the normalizer
contract.
🪄 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: ed64c8c6-6299-4681-9e4b-726d7d411243
📒 Files selected for processing (15)
src/cli/commands/mcp.tssrc/lib/artifacts/artifactStore.tssrc/lib/core/redisConversationMemoryManager.tssrc/lib/mcp/externalServerManager.tssrc/lib/mcp/mcpOutputNormalizer.tssrc/lib/mcp/toolDiscoveryService.tssrc/lib/memory/memoryRetrievalTools.tssrc/lib/neurolink.tssrc/lib/session/globalSessionState.tssrc/lib/types/artifactTypes.tssrc/lib/types/configTypes.tssrc/lib/types/conversation.tssrc/lib/types/index.tssrc/lib/types/mcpOutputTypes.tstest/continuous-test-suite-mcp-output-limits.ts
Comment on lines
+75
to
+80
| private readonly dir: string; | ||
| private readonly index: Map<string, IndexEntry> = new Map(); | ||
|
|
||
| constructor(dir?: string) { | ||
| this.dir = dir ?? join(tmpdir(), "neurolink-artifacts"); | ||
| } |
There was a problem hiding this comment.
The default shared temp directory will leak unreachable artifacts.
This implementation writes every process into the same temp folder, but cleanup() only sees the current in-memory index. After a restart, old files are neither retrievable nor removable here, so /tmp/neurolink-artifacts grows indefinitely. Please either isolate each process/session into its own subdirectory or rebuild/prune the directory on startup.
Also applies to: 156-168
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/artifacts/artifactStore.ts` around lines 75 - 80, The temp-directory
usage leaks artifacts; change ArtifactStore to isolate each process/session by
creating a unique subdirectory instead of writing directly to join(tmpdir(),
"neurolink-artifacts") (modify the constructor to append a per-process/session
id such as process.pid + timestamp or a UUID to that path), and/or implement
startup pruning by scanning the base "neurolink-artifacts" folder to rebuild the
in-memory index or remove files not belonging to any valid session; update the
constructor and the cleanup() logic (and any index-loading routine referenced
around lines 156-168) so cleanup only touches files in this instance's subdir
and startup code either rebuilds index entries from files in the subdir or
prunes stale files in the base directory.
Comment on lines
+541
to
+544
| const normalized = await this.outputNormalizer.normalize( | ||
| callResult, | ||
| { toolName, serverId }, | ||
| ); |
There was a problem hiding this comment.
Preserve sessionId when externalizing MCP output.
src/lib/mcp/mcpOutputNormalizer.ts stores context.sessionId in the artifact metadata it writes. Because this call site only passes { toolName, serverId }, every artifact created from this path loses its session association, which breaks session-scoped correlation now and makes later cleanup/access hardening impossible.
🩹 Minimal fix
async executeTool(
toolName: string,
serverId: string,
client: Client,
parameters: JsonObject,
- options: ExternalToolExecutionOptions = {},
+ options: ExternalToolExecutionOptions & { sessionId?: string } = {},
): Promise<ExternalMCPToolResult> {
...
const normalized = await this.outputNormalizer.normalize(
callResult,
- { toolName, serverId },
+ {
+ toolName,
+ serverId,
+ sessionId: options.sessionId,
+ },
);You'll still need to thread sessionId through the shared execution options and the ExternalServerManager.executeTool() callers.
🤖 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 541 - 544, The output
normalizer call in ToolDiscoveryService (the call to
this.outputNormalizer.normalize with { toolName, serverId }) omits
context.sessionId so artifacts lose session association; update the call to
include sessionId (e.g., { toolName, serverId, sessionId }) by pulling sessionId
from the surrounding execution context/options, and then propagate sessionId
through the shared execution options types and all callers of
ExternalServerManager.executeTool() so executeTool and outputNormalizer receive
and forward sessionId consistently.
Comment on lines
+145
to
+152
| return { | ||
| artifactId: args.artifactId, | ||
| content: slice, | ||
| totalSize: content.length, | ||
| hasMore: start + charLimit < content.length, | ||
| offset: start, | ||
| limit: charLimit, | ||
| }; |
There was a problem hiding this comment.
totalSize reports character count instead of byte count.
content.length returns the number of UTF-16 code units (characters), not bytes. Since McpOutputNormalizer uses byte-based thresholds (maxBytes, warnBytes), returning character count here creates inconsistent pagination metadata — clients cannot reliably calculate remaining bytes or compare against externalization thresholds.
Proposed fix
+ const sizeBytes = Buffer.byteLength(content, "utf-8");
const charLimit = Math.min(
args.limit ?? DEFAULT_RETRIEVAL_LIMIT,
MAX_RETRIEVAL_LIMIT,
);
const start = args.offset ?? 0;
const slice = content.slice(start, start + charLimit);
return {
artifactId: args.artifactId,
content: slice,
- totalSize: content.length,
+ totalSize: sizeBytes,
hasMore: start + charLimit < content.length,
offset: start,
limit: charLimit,
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| return { | |
| artifactId: args.artifactId, | |
| content: slice, | |
| totalSize: content.length, | |
| hasMore: start + charLimit < content.length, | |
| offset: start, | |
| limit: charLimit, | |
| }; | |
| const sizeBytes = Buffer.byteLength(content, "utf-8"); | |
| const charLimit = Math.min( | |
| args.limit ?? DEFAULT_RETRIEVAL_LIMIT, | |
| MAX_RETRIEVAL_LIMIT, | |
| ); | |
| const start = args.offset ?? 0; | |
| const slice = content.slice(start, start + charLimit); | |
| return { | |
| artifactId: args.artifactId, | |
| content: slice, | |
| totalSize: sizeBytes, | |
| hasMore: start + charLimit < content.length, | |
| offset: start, | |
| limit: charLimit, | |
| }; |
🤖 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 145 - 152, The returned
metadata uses character counts but downstream logic (McpOutputNormalizer)
expects bytes; compute and return byte-based values instead: compute totalSize
as Buffer.byteLength(content, 'utf8'), produce the sliced content using
Buffer.from(content, 'utf8').slice(startByte, startByte +
byteLimit).toString('utf8'), and make hasMore compare startByte + byteLimit <
totalSize; update offset and limit to represent byte offsets/limits (use the
same startByte and byteLimit variables instead of start and charLimit). Update
the block that returns artifactId, content, totalSize, hasMore, offset, and
limit to use these byte-based values so pagination metadata is consistent with
McpOutputNormalizer.
Comment on lines
+1342
to
+1346
| let artifactStore: ArtifactStore | undefined; | ||
| if (strategy === "externalize") { | ||
| artifactStore = new LocalTempArtifactStore(); | ||
| this.mcpArtifactStore = artifactStore; | ||
| logger.debug("[NeuroLink] MCP artifact store initialized (local-temp)"); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Artifact store API ==\n'
rg -n "interface ArtifactStore|class LocalTempArtifactStore|cleanup\\(|dispose\\(" src/lib/artifacts/artifactStore.ts
printf '\n== NeuroLink lifecycle references ==\n'
rg -n "mcpArtifactStore|async shutdown\\(|async dispose\\(" src/lib/neurolink.ts
printf '\n== MCP teardown delegation ==\n'
rg -n "cleanup\\(|dispose\\(|shutdown\\(" src/lib/mcp/mcpOutputNormalizer.ts src/lib/mcp/externalServerManager.tsRepository: juspay/neurolink
Length of output: 1130
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get line count of neurolink.ts to understand file size
wc -l src/lib/neurolink.ts
# Extract shutdown method (approximately lines 2949-2970)
echo "== NeuroLink.shutdown() (lines 2949-2980) =="
sed -n '2949,2980p' src/lib/neurolink.ts
# Extract dispose method (approximately lines 12027-12050)
echo "== NeuroLink.dispose() (lines 12027-12060) =="
sed -n '12027,12060p' src/lib/neurolink.ts
# Check if mcpArtifactStore.cleanup is called anywhere in the file
echo "== Search for mcpArtifactStore.cleanup calls =="
rg "mcpArtifactStore\\.cleanup" src/lib/neurolink.tsRepository: juspay/neurolink
Length of output: 2467
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get full dispose method - find where it ends
echo "== NeuroLink.dispose() full method (lines 12027-12150) =="
sed -n '12027,12150p' src/lib/neurolink.ts
# Check ExternalServerManager.shutdown() to see if it handles artifact cleanup
echo "== ExternalServerManager.shutdown() (lines 1596-1650) =="
sed -n '1596,1650p' src/lib/mcp/externalServerManager.ts
# Check if ExternalServerManager has any reference to mcpArtifactStore or artifact cleanup
echo "== Search for artifact references in ExternalServerManager =="
rg -n "artifact" src/lib/mcp/externalServerManager.ts | head -20Repository: juspay/neurolink
Length of output: 5906
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check McpOutputNormalizer for artifact cleanup
echo "== McpOutputNormalizer references to artifacts or cleanup =="
rg -n "artifact|cleanup|dispose" src/lib/mcp/mcpOutputNormalizer.ts | head -20
# Verify LocalTempArtifactStore cleanup implementation
echo "== LocalTempArtifactStore.cleanup() implementation =="
sed -n '156,180p' src/lib/artifacts/artifactStore.ts
# Check where mcpArtifactStore is used in NeuroLink
echo "== All usages of mcpArtifactStore in NeuroLink =="
rg -B2 -A2 "this\.mcpArtifactStore" src/lib/neurolink.ts | head -60Repository: juspay/neurolink
Length of output: 2461
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Continue reading dispose() method to check for artifact cleanup
echo "== NeuroLink.dispose() continuation (lines 12100-12200) =="
sed -n '12100,12200p' src/lib/neurolink.ts
# Search for any cleanup scheduling or background artifact cleanup
echo "== Search for automatic artifact cleanup scheduling =="
rg -n "cleanup.*artifact|artifact.*cleanup|setInterval.*cleanup" src/lib/neurolink.ts
# Verify if LocalTempArtifactStore has any automatic cleanup
echo "== LocalTempArtifactStore class definition and methods =="
sed -n '74,160p' src/lib/artifacts/artifactStore.tsRepository: juspay/neurolink
Length of output: 3375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if LocalTempArtifactStore actually writes to disk or is purely in-memory
echo "== LocalTempArtifactStore store() method =="
sed -n '110,155p' src/lib/artifacts/artifactStore.ts
# Check the constructor and index
echo "== LocalTempArtifactStore constructor =="
sed -n '74,110p' src/lib/artifacts/artifactStore.ts
# Search for any /tmp or disk references in artifactStore
echo "== Disk/tmp references in artifactStore =="
rg -n "tmp|disk|fs\.|mkdir|write|persist" src/lib/artifacts/artifactStore.ts | head -20Repository: juspay/neurolink
Length of output: 2793
Add artifact store cleanup to dispose lifecycle method.
LocalTempArtifactStore writes externalized payloads to /tmp/neurolink-artifacts/ on disk. It implements a cleanup(olderThanMs) method to delete expired artifacts, but this method is never called during NeuroLink shutdown or disposal. Repeated CLI/test lifecycles will orphan files in /tmp.
Add a cleanup call in NeuroLink.dispose() (similar to how mcpToolResultCache, mcpToolRouter, and mcpToolBatcher are destroyed in section 5). Wrap the cleanup with withTimeout per coding guidelines for async error handling:
Suggested fix pattern
// In NeuroLink.dispose(), add to section 5 (Maps and caches cleanup):
if (this.mcpArtifactStore) {
try {
logger.debug("[NeuroLink] Cleaning up artifact store...");
// Delete artifacts older than 24 hours
const cleaned = await withTimeout(
this.mcpArtifactStore.cleanup(24 * 60 * 60 * 1000),
5000,
new Error("Artifact store cleanup timed out"),
);
logger.debug(`[NeuroLink] Artifact cleanup: removed ${cleaned} expired file(s)`);
this.mcpArtifactStore = undefined;
} catch (error) {
logger.warn("[NeuroLink] Error cleaning up artifact store:", error);
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/neurolink.ts` around lines 1342 - 1346, Add artifact-store cleanup to
NeuroLink.dispose by detecting this.mcpArtifactStore (set when strategy ===
"externalize" and assigned a LocalTempArtifactStore), calling its
cleanup(olderThanMs) inside a withTimeout wrapper (e.g., 24*60*60*1000 ms
threshold, 5000 ms timeout), logging start, success (including number of removed
files), and catching/logging errors, then setting this.mcpArtifactStore =
undefined; follow the same try/catch and logger patterns used for
mcpToolResultCache/mcpToolRouter/mcpToolBatcher cleanup.
Comment on lines
+1542
to
+1548
| const memoryManager = this.conversationMemory as | ||
| | import("./core/redisConversationMemoryManager.js").RedisConversationMemoryManager | ||
| | undefined; | ||
| const tools = createMemoryRetrievalTools( | ||
| memoryManager, | ||
| this.mcpArtifactStore, | ||
| ); |
There was a problem hiding this comment.
Narrow conversationMemory before treating it as Redis-backed.
If Redis init fails, initializeConversationMemoryForGeneration() swaps this.conversationMemory to ConversationMemoryManager. This cast still passes that instance into createMemoryRetrievalTools(), so the session-history path can throw instead of returning the intended "Redis unavailable" error.
Suggested fix
- const memoryManager = this.conversationMemory as
- | import("./core/redisConversationMemoryManager.js").RedisConversationMemoryManager
- | undefined;
+ const memoryManager =
+ this.conversationMemory &&
+ "getSessionRaw" in this.conversationMemory &&
+ typeof this.conversationMemory.getSessionRaw === "function"
+ ? (this.conversationMemory as import("./core/redisConversationMemoryManager.js").RedisConversationMemoryManager)
+ : undefined;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/neurolink.ts` around lines 1542 - 1548, The code narrows
this.conversationMemory to RedisConversationMemoryManager without verifying its
runtime type, so when initializeConversationMemoryForGeneration() falls back to
ConversationMemoryManager after Redis init fails, createMemoryRetrievalTools()
may receive the wrong instance and throw; fix by checking the concrete type
before casting (e.g., test that this.conversationMemory is an instance of
RedisConversationMemoryManager) and only pass a Redis-backed memoryManager into
createMemoryRetrievalTools(), otherwise pass undefined or skip creating
retrieval tools; adjust the code around createMemoryRetrievalTools,
conversationMemory, and initializeConversationMemoryForGeneration to use that
guard.
Comment on lines
+24
to
+27
| const strategy: McpOutputStrategy = | ||
| strategyRaw === "inline" || strategyRaw === "externalize" | ||
| ? strategyRaw | ||
| : "externalize"; // safe default when only maxBytes is set |
There was a problem hiding this comment.
Don't silently coerce an invalid env strategy to externalize.
If NEUROLINK_MCP_OUTPUT_STRATEGY is present but misspelled, this branch still produces { strategy: "externalize" } and turns the feature on unexpectedly for CLI sessions. Missing and invalid values should be handled separately so misconfiguration is visible instead of silently changing behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/session/globalSessionState.ts` around lines 24 - 27, The current
assignment silently coerces any invalid NEUROLINK_MCP_OUTPUT_STRATEGY value into
"externalize" via the strategy/strategyRaw logic; change it so missing env and
invalid env are handled separately: read strategyRaw, if it's undefined/null
keep strategy undefined (or respect the existing maxBytes-only default path),
but if strategyRaw is present and not one of the allowed McpOutputStrategy
values ("inline" | "externalize") then surface the misconfiguration (throw or
processLogger.error and exit) instead of falling back to "externalize". Update
the logic around the strategy/strategyRaw variables in globalSessionState.ts and
reference NEUROLINK_MCP_OUTPUT_STRATEGY and the McpOutputStrategy type when
implementing the explicit validation.
Comment on lines
+52
to
+54
| function makePayload(sizeBytes: number): string { | ||
| return "x".repeat(sizeBytes); | ||
| } |
There was a problem hiding this comment.
These tests bake in the current char-count behavior.
The output-limit flow measures UTF-8 bytes, but this suite uses ASCII-only fixtures and asserts totalSize via payload.length. That will miss multibyte regressions and will block the Buffer.byteLength(..., "utf-8") fix in retrieve_context. Please switch these assertions to bytes and add at least one non-ASCII payload.
Also applies to: 394-421, 423-449
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/continuous-test-suite-mcp-output-limits.ts` around lines 52 - 54, The
tests currently measure sizes using string length (payload.length) which assumes
1 byte per char; update the test assertions to measure UTF-8 bytes instead by
using Buffer.byteLength(payload, "utf8") wherever totalSize or size assertions
are made (references: makePayload, totalSize, and the retrieve_context-related
assertions), and modify makePayload or add at least one test case that uses a
non-ASCII payload (e.g., include "é" or an emoji) so the suite will catch
multibyte regressions; ensure all occurrences noted (around the blocks 52-54,
394-421, 423-449) are changed from .length to Buffer.byteLength(..., "utf8") and
the expectations are adjusted accordingly.
Contributor
|
🎉 This PR is included in version 9.51.4 🎉 The release is available on: Your semantic-release bot 📦🚀 |
5 of 7 tasks
33 of 69 tasks
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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
McpOutputNormalizerthat intercepts tool results abovemaxBytes, writes them toLocalTempArtifactStore(/tmp/neurolink-artifacts/), and returns a surrogate with aneurolinkArtifactIdpointer so the LLM context stays leanretrieve_contexttool — Register it asuser-defined(notbuilt-in) soToolsManagerincludes it in the LLM's tool schema; gate now activates on artifact store presence alone, not only Redis;memoryManagerparam made optional so artifact retrieval works without RedisNEUROLINK_MCP_OUTPUT_STRATEGY/NEUROLINK_MCP_MAX_OUTPUT_BYTES/NEUROLINK_MCP_WARN_OUTPUT_BYTESread bybuildMcpOutputLimitsFromEnv()inglobalSessionStateso CLI sessions activate externalization without code changesmcp execlifecycle fix — Replace scatteredprocess.exit(1)calls withtry/finally sdk.shutdown()+ unconditionalprocess.exit(exitCode); closes stdio child processes and prevents event-loop hang after--outputwrite completesmcp.output.strategynow set unconditionally (inlinevsexternalize) outside theisExternalizedguard; previously the"preview"branch was dead codeTest plan
npx tsx test/continuous-test-suite-mcp-output-limits.ts— 24 tests, all pass (covers normalizer, artifact store, retrieve_context without Redis)new NeuroLink({ mcp: { outputLimits: { strategy: 'externalize', maxBytes: 2048 } } })→getAllAvailableTools()includesretrieve_context, large MCP response externalizes, artifact retrievable viaexecuteTool('retrieve_context', { artifactId })NEUROLINK_MCP_OUTPUT_STRATEGY=externalize NEUROLINK_MCP_MAX_OUTPUT_BYTES=2048 neurolink generate "..."→toolsUsedincludesretrieve_contextmcp exec --output result.json→ file written, process exits cleanly (no manual kill required)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Tests