feat(core): production reliability fixes, bash tool, and LiteLLM vision tests (NL-001–NL-007) - #877
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
✅ Single Commit Policy - COMPLIANTStatus: Policy requirements met • 1 commit • Valid format • Ready for merge 📊 View validation details📝 Commit Details
✅ Validation Results
🤖 Automated validation by NeuroLink Single Commit Enforcement |
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR introduces model alias resolution with runtime actions (block/warn/redirect), token-based summarization splitting, a bash command execution tool with output truncation, circuit-breaker-aware tool filtering, enhanced observability with retry metadata and error classification, and refactored static method calls across adapters and utilities to improve code clarity. Changes
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 unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
Documentation Validation Results🚀 Documentation validation passed!
📦 Build artifact uploaded successfully. Ready for deployment preview. Commit: |
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (5)
src/lib/context/prompts/summarizationPrompt.ts (1)
25-28: Consider renaming the backward compatibility alias.The
NINE_SECTIONSalias now points to an array with 10 sections, which is semantically misleading. While the deprecation notice helps, consumers checkingNINE_SECTIONS.lengthwill get 10, potentially breaking their expectations.Consider:
- Exporting
NINE_SECTIONSas a frozen copy of the original 9 sections, or- Renaming to
LEGACY_SECTIONSto avoid the numeric implication.🔧 Alternative: Use a semantic name for the alias
-/** - * `@deprecated` Use SUMMARY_SECTIONS instead. Kept for backward compatibility. - */ -const NINE_SECTIONS = SUMMARY_SECTIONS; +/** + * `@deprecated` Use SUMMARY_SECTIONS instead. Kept for backward compatibility. + * Note: Despite the name, this now contains 10 sections after the v3.1 update. + */ +const NINE_SECTIONS = SUMMARY_SECTIONS;Or for strict backward compatibility:
+const LEGACY_NINE_SECTIONS = [ + "Primary Request and Intent", + "Key Technical Concepts", + "Files and Code Sections", + "Problem Solving", + "Pending Tasks", + "Task Evolution", + "Current Work", + "Next Step", + "Required Files", +] as const; + /** * `@deprecated` Use SUMMARY_SECTIONS instead. Kept for backward compatibility. */ -const NINE_SECTIONS = SUMMARY_SECTIONS; +const NINE_SECTIONS = LEGACY_NINE_SECTIONS;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/context/prompts/summarizationPrompt.ts` around lines 25 - 28, The alias NINE_SECTIONS currently points to SUMMARY_SECTIONS (10 items) which is misleading; either preserve true backward compatibility by creating a frozen copy containing only the original 9 entries and assign it to NINE_SECTIONS (use Object.freeze on the new array) or rename the alias to a semantic non-numeric name like LEGACY_SECTIONS and update all exports/usages accordingly; update the deprecation comment to reflect the chosen approach and adjust any imports referencing NINE_SECTIONS to the new name if renamed.src/lib/agent/directTools.ts (1)
695-711: Security check may have false negatives with symlinks or..traversal within the allowed tree.The check
!resolvedCwd.startsWith(currentCwd)correctly prevents obvious escapes like/tmp, but:
- Symlinks inside
process.cwd()could point outside the allowed directory after resolution.- The check happens after
path.resolve(), which should normalize.., but edge cases with trailing slashes or unusual path constructions could theoretically bypass this.Consider using
fs.realpathSyncto resolve symlinks before comparison for stronger sandboxing.🛡️ Stronger symlink-aware validation
const resolvedCwd = cwd ? path.resolve(cwd) : process.cwd(); const currentCwd = process.cwd(); + +// Resolve symlinks for accurate containment check +let realResolvedCwd: string; +let realCurrentCwd: string; +try { + realResolvedCwd = fs.realpathSync(resolvedCwd); + realCurrentCwd = fs.realpathSync(currentCwd); +} catch { + return { + success: false, + code: -1, + stdout: "", + stderr: "", + error: `Cannot resolve path: ${resolvedCwd}`, + }; +} // Security: prevent execution outside current directory -if (!resolvedCwd.startsWith(currentCwd)) { +if (!realResolvedCwd.startsWith(realCurrentCwd)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/agent/directTools.ts` around lines 695 - 711, The current directory check in execute (symbols: execute, resolvedCwd, currentCwd) can be bypassed by symlinks; replace the simple path.resolve comparison with symlink-aware real path comparison: call fs.realpathSync on both resolvedCwd and process.cwd() (handle errors with try/catch), then compare that realResolved startsWith realCurrent + path.sep or equals realCurrent to avoid partial matches; keep the existing timeout and return/error behavior unchanged.test/unit/tools/executeBashCommand.test.ts (2)
106-121: Test usesddcommand with/dev/zerowhich is Unix-specific.This test will fail on Windows. Additionally, the assertion only checks that the result is defined and has a boolean
successfield, which doesn't verify truncation actually occurred.💡 More explicit truncation verification
it("should truncate large output", async () => { - // Generate output larger than 100KB using printf const result = await executeBashCommand.execute( { - command: "dd", - args: ["if=/dev/zero", "bs=1024", "count=200"], + command: "node", + args: ["-e", "console.log('x'.repeat(150000))"], timeout: 30000, }, { toolCallId: "test-10", messages: [], abortSignal: undefined as never }, ); - // dd with 200KB of zeros should either be truncated or fail due to maxBuffer - // Either way, the tool should not throw expect(result).toBeDefined(); expect(typeof result.success).toBe("boolean"); + // Verify truncation indicator is present when output exceeds limit + if (result.success && result.stdout.length > 100000) { + expect(result.stdout).toContain("[output truncated"); + } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/tools/executeBashCommand.test.ts` around lines 106 - 121, The test "should truncate large output" uses a Unix-only `dd` call; replace the command in the test that calls executeBashCommand.execute with a cross-platform generator (e.g., run Node itself via command "node" and args ["-e", "process.stdout.write('0'.repeat(200*1024))"]) so it works on Windows and CI, and strengthen assertions to verify truncation actually happened by checking the returned output length is <= the truncation threshold (e.g., 100*1024) and that the original requested size (200*1024) is larger than the returned length or that a truncation marker is present; reference the test and executeBashCommand.execute in the change.
86-94: Test relies onsleepcommand which may not exist on Windows.The timeout test uses
sleep 60which is a Unix command. On Windows CI environments, this test would fail. Consider using a cross-platform approach or marking platform-specific tests.💡 Cross-platform alternative using Node
it("should enforce timeout", async () => { + // Use a cross-platform command that blocks const result = await executeBashCommand.execute( - { command: "sleep", args: ["60"], timeout: 1000 }, + { command: "node", args: ["-e", "setTimeout(() => {}, 60000)"], timeout: 1000 }, { toolCallId: "test-8", messages: [], abortSignal: undefined as never }, ); expect(result.success).toBe(false); - // Should complete within reasonable time due to timeout + expect(result.error).toContain("timed out"); }, 10000);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/tools/executeBashCommand.test.ts` around lines 86 - 94, The test uses the Unix-only "sleep 60" which fails on Windows; update the timeout test in executeBashCommand.test.ts to use a cross-platform long-running process (or skip on Windows). Replace the command invocation in the call to executeBashCommand.execute (where command is "sleep" and args ["60"]) with a Node-based cross-platform runner (e.g., command "node" with args ["-e", "setTimeout(()=>{},60000)"]) or add a platform check to skip the test on Windows, ensuring you reference the executeBashCommand.execute call and the specific test block when making the change.src/lib/types/contextTypes.ts (1)
6-6: MoveCompactionStageinto the shared types module.
contextTypes.tsnow depends oncontextCompactor.ts, whilecontextCompactor.tsalready depends on this file forCompactionConfig/CompactionResult. That weakens the type/implementation boundary and creates an avoidable cycle.♻️ Suggested direction
-import type { CompactionStage } from "../context/contextCompactor.js"; +export type CompactionStage = + | "prune" + | "deduplicate" + | "summarize" + | "truncate";Then import
CompactionStagefromsrc/lib/types/contextTypes.tsinsidesrc/lib/context/contextCompactor.tsinstead of declaring it in the implementation file.As per coding guidelines: Maintain strict TypeScript across all modules with no circular dependencies between type definition files. Organize types by domain (providers, generation, streaming, MCP, etc.) to prevent circular imports.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/types/contextTypes.ts` at line 6, contextTypes.ts currently imports CompactionStage from contextCompactor.ts creating a type-cycle with contextCompactor.ts (which needs CompactionConfig/CompactionResult); move the CompactionStage declaration into the shared types module (export it from src/lib/types/contextTypes.ts), remove the import of CompactionStage from contextCompactor in contextTypes.ts, and update src/lib/context/contextCompactor.ts to import CompactionStage from the shared types module instead; ensure the exported name is exactly CompactionStage and that CompactionConfig and CompactionResult remain defined where they are to avoid reintroducing a circular dependency.
🤖 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/agent/directTools.ts`:
- Around line 736-744: The callback in the execFile handler currently assigns
error.code directly to the result object causing type inconsistency when
error.code is a string (e.g., 'ENOENT'); update the handler that builds the
resolve payload (the anonymous callback passed to execFile which uses resolve,
truncateOutput, and error.killed) to normalize code into a number by checking
typeof error.code === 'number' and using it directly, otherwise map common
string codes (e.g., 'ENOENT' -> 127) or fall back to a safe numeric value (e.g.,
1) so the returned result.code is always a number.
In `@src/lib/context/stages/structuredSummarizer.ts`:
- Around line 69-78: The token-splitting uses the summarization provider
(config.provider) but must use the budget provider; change the
findSplitIndexByTokens call(s) that pass config.provider to pass
config.budgetProvider (or effectiveMemoryConfig.budgetProvider) for the
targetRecentTokens split (same change for the other occurrence around lines
101-106). Also ensure before any LLM invocation in this stage you invoke the
BudgetChecker validation (e.g., BudgetChecker.validateContextFits or the
project’s ensureWithinWindow/checkAndCompact method) to verify context fits the
model window and trigger auto-compaction when usage exceeds the 80% threshold.
- Around line 31-54: findSplitIndexByTokens can return messages.length which
makes summarizeMessages include the newest turn in the summary (dropping the
live tail). Clamp the computed splitIndex so it never equals messages.length and
always leaves the latest turn unsummarized: in findSplitIndexByTokens adjust the
final return to ensure splitIndex is at most messages.length - 1 (and still at
least 1). This guarantees the live tail (latest message) is preserved for the
next model call; refer to the function name findSplitIndexByTokens and the
consumer summarizeMessages when making the change.
In `@src/lib/mcp/toolRegistry.ts`:
- Around line 781-805: In getAvailableTools, the circuit breaker lookup uses
tool.name causing collisions across servers; change the lookup to use the
fully-qualified tool id (the same key used to store tools, e.g.,
`${tool.serverId}.${tool.name}` or tool.toolId) when calling
circuitBreakers.get(...) and when building unavailableTools so breakers are
isolated per server; also deduplicate tools here the same way listTools() does
(use the fully-qualified key to skip duplicates) so getAvailableTools returns
the same deduplicated set and unavailable list as listTools.
In `@src/lib/neurolink.ts`:
- Around line 6459-6484: After compaction (after compactor.compact and after
repairing messages and updating this.lastCompactionMessageCount), recalc and
re-check the stream budget and follow the same guarded flow used in
tryMCPGeneration()/directProviderGeneration(): recompute streamBudget based on
the new conversationMessages and streamBudget.availableInputTokens, then if
postCompactBudget indicates insufficient tokens or
streamBudget.shouldAbort/!postCompactBudget trigger emergencyContentTruncation
(or the same fallback used elsewhere) instead of immediately calling
provider.stream; ensure you reference streamBudget, postCompactBudget,
emergencyContentTruncation, provider.stream, compactor.compact,
conversationMessages and this.lastCompactionMessageCount when making this
change.
- Around line 7766-7824: The current logic records isError:true after the
circuit breaker operation has already reported success; move the isToolError
handling into the original breaker-wrapped operation so failures are recorded
before onSuccess/metrics/events run: inside the function passed to
circuitBreaker.execute(...) (the same operation that currently returns tool
results) detect isToolError and throw an Error (or reject) to let the breaker
record a real failure, and only after circuitBreaker.execute resolves
successfully (i.e., no isToolError) emit the success metrics/event (the code
that increments metrics.successfulExecutions and emits tool:end); also remove or
skip the post-hoc synthetic failure block (the current NL-001) to avoid
replaying failures that occur after success has been recorded. Ensure toolSpan
attributes and setStatus for tool.error are still set when throwing/failing so
telemetry is captured.
- Around line 2880-2884: The code currently calls resolveModel on the
caller-supplied options.model early, but later Object.assign(options,
orchestratedOptions) can overwrite options.model with a routed value
(route.model) that skips your alias/deprecation mapping; update the flow so that
after orchestration merges in orchestratedOptions (the Object.assign that
injects route.model) you call resolveModel again to normalize the final
options.model (i.e., run resolveModel(options.model, this.modelAliasConfig)
immediately after the Object.assign), ensuring both generate and stream paths
use the alias-resolved model and any ModelRouter redirects are normalized.
- Around line 4378-4380: The generate() function rebuilds a fresh GenerateResult
and never copies mcpResult.retries into it, so callers and the
generate.retry_count span attribute lose MCP retry history; fix by propagating
the MCP retry metadata into the GenerateResult instance created in generate()
(copy mcpResult.retries into the new GenerateResult or merge it before any
reads/return), and ensure generate.retry_count uses that propagated retries
object (fall back to {count:0, errors:[]} if absent). Reference symbols:
generate(), mcpResult.retries, GenerateResult, generate.retry_count.
- Around line 8845-8848: setModelAliasConfig updates modelAliasConfig but
generateText() still calls generateTextInternal() directly, bypassing
resolveModel() so deprecated aliases aren't enforced for the legacy path; update
generateText() to resolve the model alias before delegating by calling
resolveModel(...) (the same logic used in generate()/stream()) and pass the
resolved model to generateTextInternal(), ensuring generate(), stream(),
generateText(), and generateTextInternal() all use resolved model names and
respect ModelAliasConfig.
- Around line 475-479: The shared numeric watermark lastCompactionMessageCount
in the NeuroLink class causes compaction decisions to be global; change it to a
per-conversation watermark map (e.g., lastCompactionMessageCountByConversation:
Map<string, number>) and update all compaction logic that reads/writes
lastCompactionMessageCount (references in NeuroLink where compacting is
performed — lines around the current declaration and the usages at the other
noted spots) to use the conversation/session identifier as the key so each
conversation tracks its own last compaction message count; ensure
initialization, lookup (default 0), update after compaction, and any
serialization/cleanup are handled consistently.
---
Nitpick comments:
In `@src/lib/agent/directTools.ts`:
- Around line 695-711: The current directory check in execute (symbols: execute,
resolvedCwd, currentCwd) can be bypassed by symlinks; replace the simple
path.resolve comparison with symlink-aware real path comparison: call
fs.realpathSync on both resolvedCwd and process.cwd() (handle errors with
try/catch), then compare that realResolved startsWith realCurrent + path.sep or
equals realCurrent to avoid partial matches; keep the existing timeout and
return/error behavior unchanged.
In `@src/lib/context/prompts/summarizationPrompt.ts`:
- Around line 25-28: The alias NINE_SECTIONS currently points to
SUMMARY_SECTIONS (10 items) which is misleading; either preserve true backward
compatibility by creating a frozen copy containing only the original 9 entries
and assign it to NINE_SECTIONS (use Object.freeze on the new array) or rename
the alias to a semantic non-numeric name like LEGACY_SECTIONS and update all
exports/usages accordingly; update the deprecation comment to reflect the chosen
approach and adjust any imports referencing NINE_SECTIONS to the new name if
renamed.
In `@src/lib/types/contextTypes.ts`:
- Line 6: contextTypes.ts currently imports CompactionStage from
contextCompactor.ts creating a type-cycle with contextCompactor.ts (which needs
CompactionConfig/CompactionResult); move the CompactionStage declaration into
the shared types module (export it from src/lib/types/contextTypes.ts), remove
the import of CompactionStage from contextCompactor in contextTypes.ts, and
update src/lib/context/contextCompactor.ts to import CompactionStage from the
shared types module instead; ensure the exported name is exactly CompactionStage
and that CompactionConfig and CompactionResult remain defined where they are to
avoid reintroducing a circular dependency.
In `@test/unit/tools/executeBashCommand.test.ts`:
- Around line 106-121: The test "should truncate large output" uses a Unix-only
`dd` call; replace the command in the test that calls executeBashCommand.execute
with a cross-platform generator (e.g., run Node itself via command "node" and
args ["-e", "process.stdout.write('0'.repeat(200*1024))"]) so it works on
Windows and CI, and strengthen assertions to verify truncation actually happened
by checking the returned output length is <= the truncation threshold (e.g.,
100*1024) and that the original requested size (200*1024) is larger than the
returned length or that a truncation marker is present; reference the test and
executeBashCommand.execute in the change.
- Around line 86-94: The test uses the Unix-only "sleep 60" which fails on
Windows; update the timeout test in executeBashCommand.test.ts to use a
cross-platform long-running process (or skip on Windows). Replace the command
invocation in the call to executeBashCommand.execute (where command is "sleep"
and args ["60"]) with a Node-based cross-platform runner (e.g., command "node"
with args ["-e", "setTimeout(()=>{},60000)"]) or add a platform check to skip
the test on Windows, ensuring you reference the executeBashCommand.execute call
and the specific test block when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 72c5261f-39d5-4b7b-a7a2-74ef9e19fa68
📒 Files selected for processing (20)
.gitignoredocs/analysis/issue-triage-2026-03-14.mdsrc/lib/adapters/providerImageAdapter.tssrc/lib/agent/directTools.tssrc/lib/context/contextCompactor.tssrc/lib/context/prompts/summarizationPrompt.tssrc/lib/context/stages/structuredSummarizer.tssrc/lib/mcp/servers/agent/directToolsServer.tssrc/lib/mcp/toolRegistry.tssrc/lib/neurolink.tssrc/lib/providers/googleAiStudio.tssrc/lib/providers/googleVertex.tssrc/lib/types/configTypes.tssrc/lib/types/contextTypes.tssrc/lib/types/generateTypes.tssrc/lib/utils/messageBuilder.tssrc/lib/utils/pdfProcessor.tstest/unit/context/prompts/summarizationPrompt.test.tstest/unit/multimodal/litellm-vision.test.tstest/unit/tools/executeBashCommand.test.ts
| function findSplitIndexByTokens( | ||
| messages: ChatMessage[], | ||
| targetRecentTokens: number, | ||
| provider?: string, | ||
| ): number { | ||
| let recentTokens = 0; | ||
| let splitIndex = messages.length; | ||
|
|
||
| for (let i = messages.length - 1; i >= 0; i--) { | ||
| const content = | ||
| typeof messages[i].content === "string" | ||
| ? messages[i].content | ||
| : JSON.stringify(messages[i].content); | ||
| const msgTokens = estimateTokens(content, provider); | ||
| if (recentTokens + msgTokens > targetRecentTokens) { | ||
| splitIndex = i + 1; | ||
| break; | ||
| } | ||
| recentTokens += msgTokens; | ||
| } | ||
|
|
||
| // Ensure at least one message is summarized | ||
| return Math.max(1, splitIndex); | ||
| } |
There was a problem hiding this comment.
Keep the live tail out of the summary.
findSplitIndexByTokens() can return messages.length when the newest turn alone is larger than targetRecentTokens. That makes summarizeMessages() summarize the entire transcript and drop the latest prompt instead of preserving it for the next model call.
💡 Suggested fix
function findSplitIndexByTokens(
messages: ChatMessage[],
targetRecentTokens: number,
provider?: string,
): number {
let recentTokens = 0;
- let splitIndex = messages.length;
+ let splitIndex = 0;
+ const minRecentCount = Math.min(4, messages.length - 1);
for (let i = messages.length - 1; i >= 0; i--) {
const content =
typeof messages[i].content === "string"
? messages[i].content
: JSON.stringify(messages[i].content);
const msgTokens = estimateTokens(content, provider);
- if (recentTokens + msgTokens > targetRecentTokens) {
+ if (
+ recentTokens + msgTokens > targetRecentTokens &&
+ messages.length - (i + 1) >= minRecentCount
+ ) {
splitIndex = i + 1;
break;
}
recentTokens += msgTokens;
}
- // Ensure at least one message is summarized
- return Math.max(1, splitIndex);
+ return Math.min(splitIndex, messages.length - minRecentCount);
}📝 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.
| function findSplitIndexByTokens( | |
| messages: ChatMessage[], | |
| targetRecentTokens: number, | |
| provider?: string, | |
| ): number { | |
| let recentTokens = 0; | |
| let splitIndex = messages.length; | |
| for (let i = messages.length - 1; i >= 0; i--) { | |
| const content = | |
| typeof messages[i].content === "string" | |
| ? messages[i].content | |
| : JSON.stringify(messages[i].content); | |
| const msgTokens = estimateTokens(content, provider); | |
| if (recentTokens + msgTokens > targetRecentTokens) { | |
| splitIndex = i + 1; | |
| break; | |
| } | |
| recentTokens += msgTokens; | |
| } | |
| // Ensure at least one message is summarized | |
| return Math.max(1, splitIndex); | |
| } | |
| function findSplitIndexByTokens( | |
| messages: ChatMessage[], | |
| targetRecentTokens: number, | |
| provider?: string, | |
| ): number { | |
| let recentTokens = 0; | |
| let splitIndex = 0; | |
| const minRecentCount = Math.min(4, messages.length - 1); | |
| for (let i = messages.length - 1; i >= 0; i--) { | |
| const content = | |
| typeof messages[i].content === "string" | |
| ? messages[i].content | |
| : JSON.stringify(messages[i].content); | |
| const msgTokens = estimateTokens(content, provider); | |
| if ( | |
| recentTokens + msgTokens > targetRecentTokens && | |
| messages.length - (i + 1) >= minRecentCount | |
| ) { | |
| splitIndex = i + 1; | |
| break; | |
| } | |
| recentTokens += msgTokens; | |
| } | |
| return Math.min(splitIndex, messages.length - minRecentCount); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/context/stages/structuredSummarizer.ts` around lines 31 - 54,
findSplitIndexByTokens can return messages.length which makes summarizeMessages
include the newest turn in the summary (dropping the live tail). Clamp the
computed splitIndex so it never equals messages.length and always leaves the
latest turn unsummarized: in findSplitIndexByTokens adjust the final return to
ensure splitIndex is at most messages.length - 1 (and still at least 1). This
guarantees the live tail (latest message) is preserved for the next model call;
refer to the function name findSplitIndexByTokens and the consumer
summarizeMessages when making the change.
| if (config?.targetTokens && config.targetTokens > 0) { | ||
| // Keep `keepRecentRatio` fraction of the target budget as recent context | ||
| const targetRecentTokens = Math.floor( | ||
| config.targetTokens * keepRecentRatio, | ||
| ); | ||
| splitIndex = findSplitIndexByTokens( | ||
| messages, | ||
| targetRecentTokens, | ||
| config.provider, | ||
| ); |
There was a problem hiding this comment.
Use the budget provider for the token split, not the summarizer provider.
config.provider is later copied into effectiveMemoryConfig.summarizationProvider, so this new split path is sizing the keep/reduce boundary with the summary model's tokenizer. In mixed-provider setups that can over/under-summarize Stage 3 and force unnecessary Stage 4 truncation.
As per coding guidelines: BudgetChecker must validate context fits within model's window before every LLM call, triggering auto-compaction when usage exceeds 80%.
Also applies to: 101-106
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/context/stages/structuredSummarizer.ts` around lines 69 - 78, The
token-splitting uses the summarization provider (config.provider) but must use
the budget provider; change the findSplitIndexByTokens call(s) that pass
config.provider to pass config.budgetProvider (or
effectiveMemoryConfig.budgetProvider) for the targetRecentTokens split (same
change for the other occurrence around lines 101-106). Also ensure before any
LLM invocation in this stage you invoke the BudgetChecker validation (e.g.,
BudgetChecker.validateContextFits or the project’s
ensureWithinWindow/checkAndCompact method) to verify context fits the model
window and trigger auto-compaction when usage exceeds the 80% threshold.
| /** | ||
| * NL-001: Get available tools, filtering out those with OPEN circuit breakers. | ||
| * Returns both the filtered tools and the list of unavailable tool names. | ||
| */ | ||
| getAvailableTools( | ||
| circuitBreakers: Map< | ||
| string, | ||
| import("../utils/errorHandling.js").CircuitBreaker | ||
| >, | ||
| ): { tools: ToolInfo[]; unavailableTools: string[] } { | ||
| const allTools = Array.from(this.tools.values()); | ||
| const unavailableTools: string[] = []; | ||
| const tools: ToolInfo[] = []; | ||
|
|
||
| for (const tool of allTools) { | ||
| const breaker = circuitBreakers.get(tool.name); | ||
| if (breaker && breaker.getState() === "open") { | ||
| unavailableTools.push(tool.name); | ||
| } else { | ||
| tools.push(tool); | ||
| } | ||
| } | ||
|
|
||
| return { tools, unavailableTools }; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check how circuit breakers are keyed in the codebase.
# Search for circuitBreakers.get( patterns to see what keys are used
echo "=== Circuit breaker key patterns ==="
rg -n -C3 'circuitBreakers\.get\(' --type=ts
echo ""
echo "=== Circuit breaker key assignment ==="
rg -n -C3 'circuitBreakers\.set\(' --type=tsRepository: juspay/neurolink
Length of output: 3395
🏁 Script executed:
# Find the ToolInfo interface and tool registration logic
echo "=== ToolInfo interface definition ==="
rg -n -C5 'interface ToolInfo|type ToolInfo' --type=ts src/lib/mcp/
echo ""
echo "=== Tool registration in toolRegistry ==="
rg -n -C5 'registerTool\|tools\.set\|this\.tools\.set' --type=ts src/lib/mcp/toolRegistry.ts | head -100
echo ""
echo "=== listTools() method implementation ==="
rg -n -A20 'listTools\(' --type=ts src/lib/mcp/toolRegistry.ts | head -60Repository: juspay/neurolink
Length of output: 1207
🏁 Script executed:
# Search for where tools are registered/constructed
echo "=== Where tools are registered ==="
rg -n -B5 -A10 'tools\.set\(' --type=ts src/lib/mcp/toolRegistry.ts | head -80
echo ""
echo "=== ToolInfo type/interface definition ==="
rg -n -B2 -A8 'type ToolInfo|interface ToolInfo' --type=ts
echo ""
echo "=== Where getAvailableTools is called ==="
rg -n -B3 -A3 'getAvailableTools\(' --type=tsRepository: juspay/neurolink
Length of output: 6041
🏁 Script executed:
# Search for toolCircuitBreakers management
echo "=== toolCircuitBreakers initialization and usage ==="
rg -n -B3 -A3 'toolCircuitBreakers' --type=ts src/lib/neurolink.ts | head -100
echo ""
echo "=== How circuit breakers are set for tools ==="
rg -n -B5 -A5 'toolCircuitBreakers\.set\|toolCircuitBreakers\.get' --type=tsRepository: juspay/neurolink
Length of output: 3313
🏁 Script executed:
# Get more context around line 7654 where circuit breakers are created
echo "=== Context around circuit breaker creation (line 7654) ==="
sed -n '7640,7680p' src/lib/neurolink.ts
echo ""
echo "=== Search for where toolName is defined in tool execution ==="
rg -n 'const toolName = |let toolName = |toolName:' --type=ts src/lib/neurolink.ts | head -20Repository: juspay/neurolink
Length of output: 2871
🏁 Script executed:
# Find the executeTool method signature
echo "=== executeTool method signature ==="
rg -n -B5 'executeTool\(' --type=ts src/lib/neurolink.ts | grep -A10 'async executeTool\|private.*executeTool' | head -40
echo ""
echo "=== Find where executeTool is called to see what toolName is passed ==="
rg -n -B2 'this\.executeTool\|await.*executeTool' --type=ts src/lib/neurolink.ts | head -60Repository: juspay/neurolink
Length of output: 169
🏁 Script executed:
# Find method definition more carefully
echo "=== Find method that contains circuit breaker creation (around line 7654) ==="
sed -n '7500,7670p' src/lib/neurolink.ts | head -100
echo ""
echo "=== Get the method name that contains the circuit breaker logic ==="
sed -n '7400,7660p' src/lib/neurolink.ts | grep -E '^\s*(async\s+)?[a-zA-Z_][a-zA-Z0-9_]*\s*\(' | tail -5Repository: juspay/neurolink
Length of output: 3559
🏁 Script executed:
# Find where executeTool is called to see what values are passed
echo "=== Calls to executeTool method ==="
rg -n '\.executeTool\(' --type=ts -A2 src/lib/neurolink.ts | head -40
echo ""
echo "=== Tool execution in generate method context ==="
rg -n -B5 -A10 'toolName.*=.*tool\.(name|id)' --type=ts src/lib/neurolink.ts | head -60Repository: juspay/neurolink
Length of output: 781
🏁 Script executed:
# Get full listTools implementation to understand deduplication
echo "=== Full listTools implementation ==="
sed -n '603,680p' src/lib/mcp/toolRegistry.tsRepository: juspay/neurolink
Length of output: 2449
Fix circuit breaker keying to prevent tool collision across servers.
The method uses tool.name to look up circuit breakers (line 796), but tools are stored by fully-qualified toolId (e.g., serverId.toolName). This causes critical collisions: if multiple servers register tools with the same name (e.g., both server1 and server2 have a "search" tool), they would incorrectly share a single circuit breaker state. The circuit breaker key must be fully-qualified to maintain isolation per server.
Additionally, this method does not deduplicate tools like listTools() does (lines 615-622 use ${tool.serverId}.${tool.name} for deduplication), creating inconsistent behavior between methods.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/mcp/toolRegistry.ts` around lines 781 - 805, In getAvailableTools,
the circuit breaker lookup uses tool.name causing collisions across servers;
change the lookup to use the fully-qualified tool id (the same key used to store
tools, e.g., `${tool.serverId}.${tool.name}` or tool.toolId) when calling
circuitBreakers.get(...) and when building unavailableTools so breakers are
isolated per server; also deduplicate tools here the same way listTools() does
(use the fully-qualified key to skip duplicates) so getAvailableTools returns
the same deduplicated set and unavailable list as listTools.
| // NL-004: Resolve model aliases/deprecations before processing | ||
| options.model = resolveModel( | ||
| options.model, | ||
| this.modelAliasConfig, | ||
| ); |
There was a problem hiding this comment.
Resolve aliases again after orchestration picks a model.
This only normalizes the caller-supplied options.model. Later Object.assign(options, orchestratedOptions) in Line 3021 and Line 5873 can inject a fresh route.model, so a blocked or redirected model from ModelRouter bypasses the alias map on both generate and stream.
Minimal fix
// Use orchestrated options
Object.assign(options, orchestratedOptions);
+ options.model = resolveModel(
+ options.model,
+ this.modelAliasConfig,
+ );Also applies to: 5443-5445
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/neurolink.ts` around lines 2880 - 2884, The code currently calls
resolveModel on the caller-supplied options.model early, but later
Object.assign(options, orchestratedOptions) can overwrite options.model with a
routed value (route.model) that skips your alias/deprecation mapping; update the
flow so that after orchestration merges in orchestratedOptions (the
Object.assign that injects route.model) you call resolveModel again to normalize
the final options.model (i.e., run resolveModel(options.model,
this.modelAliasConfig) immediately after the Object.assign), ensuring both
generate and stream paths use the alias-resolved model and any ModelRouter
redirects are normalized.
| // NL-007: Attach retry metadata to result | ||
| if (retryCount > 0) { | ||
| mcpResult.retries = { count: retryCount, errors: retryErrors }; |
There was a problem hiding this comment.
generate.retry_count never sees the MCP retry data.
The MCP path stores mcpResult.retries here, but generate() rebuilds a fresh GenerateResult above Line 3223 without copying that field before Line 3321 reads it. Callers lose the retry history and the span attribute stays 0 even after MCP retries.
Also applies to: 3321-3325
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/neurolink.ts` around lines 4378 - 4380, The generate() function
rebuilds a fresh GenerateResult and never copies mcpResult.retries into it, so
callers and the generate.retry_count span attribute lose MCP retry history; fix
by propagating the MCP retry metadata into the GenerateResult instance created
in generate() (copy mcpResult.retries into the new GenerateResult or merge it
before any reads/return), and ensure generate.retry_count uses that propagated
retries object (fall back to {count:0, errors:[]} if absent). Reference symbols:
generate(), mcpResult.retries, GenerateResult, generate.retry_count.
| // NL-001: Count isError:true results as circuit breaker failures | ||
| // This ensures tools that return error results (not just thrown errors) are tracked | ||
| if (isToolError && circuitBreaker) { | ||
| // Record a failure by executing a rejected promise through the breaker | ||
| try { | ||
| await circuitBreaker.execute(async () => { | ||
| throw new Error(`Tool ${toolName} returned isError:true`); | ||
| }); | ||
| } catch { | ||
| // Expected — we intentionally triggered the failure recording | ||
| } | ||
| mcpLogger.debug( | ||
| `[${functionTag}] Circuit breaker failure recorded for isError result`, | ||
| { | ||
| toolName, | ||
| circuitBreakerState: circuitBreaker.getState(), | ||
| circuitBreakerFailures: circuitBreaker.getFailureCount(), | ||
| }, | ||
| ); | ||
| } | ||
|
|
||
| // NL-002 + NL-003: Format and capture MCP error results | ||
| if (isToolError) { | ||
| const resultObj = result as Record<string, unknown>; | ||
| const contentArr = resultObj.content as | ||
| | Array<{ type?: string; text?: string }> | ||
| | undefined; | ||
| const errorText = | ||
| contentArr | ||
| ?.filter((c) => c.type === "text" && c.text) | ||
| .map((c) => c.text) | ||
| .join(" ") || | ||
| (typeof resultObj.error === "string" | ||
| ? resultObj.error | ||
| : "Unknown error"); | ||
| const errorCategory = classifyMcpErrorMessage(errorText); | ||
| const prefix = `[TOOL_ERROR: ${toolName} failed (${errorCategory})] `; | ||
|
|
||
| // NL-002: Modify the text content in-place to include the error prefix | ||
| if (contentArr && Array.isArray(contentArr)) { | ||
| for (const content of contentArr) { | ||
| if (content.type === "text" && content.text) { | ||
| content.text = prefix + content.text; | ||
| break; // Only prefix the first text content | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // NL-003: Capture error details in span attributes for telemetry | ||
| toolSpan.setAttribute( | ||
| "tool.error.message", | ||
| errorText.substring(0, 500), | ||
| ); | ||
| toolSpan.setAttribute("tool.error.category", errorCategory); | ||
| toolSpan.setStatus({ | ||
| code: SpanStatusCode.ERROR, | ||
| message: `MCP tool returned isError: ${errorText.substring(0, 200)}`, | ||
| }); | ||
| } |
There was a problem hiding this comment.
isError:true is still being recorded as a successful tool call.
By the time this block runs, the outer circuitBreaker.execute() has already called onSuccess(), metrics.successfulExecutions++ has already run, and Line 7756 emitted tool:end with success: true. Replaying a synthetic failure here never accumulates past 1 breaker failure and leaves health metrics and listeners reporting a successful execution. Handle isError inside the original breaker-wrapped operation, then emit success metrics/events only after the final status is known.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/neurolink.ts` around lines 7766 - 7824, The current logic records
isError:true after the circuit breaker operation has already reported success;
move the isToolError handling into the original breaker-wrapped operation so
failures are recorded before onSuccess/metrics/events run: inside the function
passed to circuitBreaker.execute(...) (the same operation that currently returns
tool results) detect isToolError and throw an Error (or reject) to let the
breaker record a real failure, and only after circuitBreaker.execute resolves
successfully (i.e., no isToolError) emit the success metrics/event (the code
that increments metrics.successfulExecutions and emits tool:end); also remove or
skip the post-hoc synthetic failure block (the current NL-001) to avoid
replaying failures that occur after success has been recorded. Ensure toolSpan
attributes and setStatus for tool.error are still set when throwing/failing so
telemetry is captured.
| setModelAliasConfig( | ||
| config: import("./types/generateTypes.js").ModelAliasConfig, | ||
| ): void { | ||
| this.modelAliasConfig = config; |
There was a problem hiding this comment.
setModelAliasConfig() doesn't cover generateText() yet.
Line 3746 still forwards straight to generateTextInternal() without resolveModel(), so the same deprecated model can be blocked in generate()/stream() but silently accepted through the legacy API.
Minimal fix
async generateText(
options: TextGenerationOptions,
): Promise<TextGenerationResult> {
// Validate required parameters for backward compatibility
if (
!options.prompt ||
typeof options.prompt !== "string" ||
options.prompt.trim() === ""
) {
throw new Error(
"GenerateText options must include prompt as a non-empty string",
);
}
+ options.model = resolveModel(options.model, this.modelAliasConfig);
+
// Use internal generation method directly
return await this.generateTextInternal(options);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/neurolink.ts` around lines 8845 - 8848, setModelAliasConfig updates
modelAliasConfig but generateText() still calls generateTextInternal() directly,
bypassing resolveModel() so deprecated aliases aren't enforced for the legacy
path; update generateText() to resolve the model alias before delegating by
calling resolveModel(...) (the same logic used in generate()/stream()) and pass
the resolved model to generateTextInternal(), ensuring generate(), stream(),
generateText(), and generateTextInternal() all use resolved model names and
respect ModelAliasConfig.
0161ab1 to
b03ad40
Compare
b03ad40 to
5b77ee2
Compare
5b77ee2 to
c206c5a
Compare
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
c206c5a to
1b89ce5
Compare
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
1b89ce5 to
e0b4673
Compare
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
e0b4673 to
430dfe5
Compare
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
430dfe5 to
f8236eb
Compare
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
f8236eb to
85fc736
Compare
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
85fc736 to
6405b5e
Compare
Review Feedback Addressed (Cycle 1)Changes MadeCritical:
Major:
Minor:
Nitpicks:
Already fixed (prior round):
Files Modified
Validation
Resolution Status
@coderabbitai — All feedback addressed. Please re-review. |
🤖 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 |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 35185
Cycle 1 Verification ✅
All feedback from Cycle 1 is verified. The [approve] 🧠 Learnings used |
…on tests (NL-001–NL-007) Context compaction: - Fix silent summarization skip when memoryConfig is undefined (Bug 1) by building effectiveMemoryConfig from provider/model defaults - Add lastCompactionMessageCount watermark to prevent re-triggering (Bug 2) - Forward summarizationProvider/Model to all 4 compaction call sites - Pass targetTokens to summarizer for token-based split (Bug 4) - Use checkContextBudget() in compactSession() instead of arbitrary messages.length * 100 heuristic (Bug 5) - Add Section 10 "Constraints and Established Rules" to summarization prompt with explicit preservation instructions (Bug 3) - Add adaptive truncation params to sliding window stage MCP reliability (NL-001–NL-003): - Filter circuit-breaker-open tools from LLM tool list (NL-001) - Prefix isError tool results with category for AI disambiguation (NL-002) - Capture MCP error messages and categories in OTel spans (NL-003) Provider improvements (NL-004, NL-005, NL-007): - Add model deprecation/alias map with warn/redirect/block (NL-004) - Add typed error classes for provider errors (NL-005) - Expose provider retry count and errors in analytics (NL-007) New feature: - Add executeBashCommand built-in tool with execFile, cwd validation, timeout cap (120s), output truncation (100KB), and 12 unit tests LiteLLM: - Add 20 unit tests for vision on production code path (proxy bypass, image count validation, image formatting) Verified: 2706 vitest + 341 continuous tests (npx tsx), 0 code regressions
6405b5e to
f4df8dc
Compare
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
|
🎉 This PR is included in version 9.26.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Summary
executeBashCommandbuilt-in tool with execFile, cwd validation, timeout cap, output truncationFiles Changed (20)
structuredSummarizer.ts,contextCompactor.ts,summarizationPrompt.ts,contextTypes.tsneurolink.ts(watermark, config forwarding, NL-001–NL-007)toolRegistry.ts(circuit breaker filtering),directToolsServer.ts(bash category)googleAiStudio.ts,googleVertex.ts(typed errors)configTypes.ts,generateTypes.ts(retries, ModelAliasConfig)directTools.ts(executeBashCommand)providerImageAdapter.ts,messageBuilder.ts,pdfProcessor.tsexecuteBashCommand.test.ts(NEW),litellm-vision.test.ts(NEW),summarizationPrompt.test.tsissue-triage-2026-03-14.md(NEW)Test Plan
pnpm run check— 0 type errorspnpm test— 2706 vitest tests passing (91 files)npx tsx test/continuous-test-suite.ts— 29/29 passednpx tsx test/continuous-test-suite-context.ts— 19/19 passednpx tsx test/continuous-test-suite-memory.ts— 15/15 passednpx tsx test/continuous-test-suite-providers.ts— 22/25 (2 env failures: OpenRouter free model 404)npx tsx test/continuous-test-suite-observability.ts— 14/14 passednpx tsx test/continuous-test-suite-mcp-http.ts— 16/16 passednpx tsx test/continuous-test-suite-rag.ts— 100/100 passednpx tsx test/continuous-test-suite-evaluation.ts— 12/12 passednpx tsx test/continuous-test-suite-workflow.ts— 15/15 passednpx tsx test/continuous-test-suite-servers.ts— 40/40 passednpx tsx test/continuous-test-suite-tracing.ts— 10/10 passednpx tsx test/continuous-test-suite-tts.ts— 15/15 passednpx tsx test/continuous-test-suite-ppt.ts— 16/16 passednpx tsx test/continuous-test-suite-media-gen.ts— 18/18 passedTotal: 3,047 tests, 0 code regressions
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests