fix(context): pre-dispatch compaction for inline conversationMessages - #993
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 33 minutes and 21 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ 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 |
🤖 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.
Pull request overview
Fixes a bug in directProviderGeneration where pre-dispatch context compaction was skipped when callers provided conversationMessages inline (without enabling conversation memory), preventing oversized payloads from being dispatched to providers and adding an audit signal for insufficient compaction.
Changes:
- Update pre-dispatch compaction gating to run for inline
conversationMessagesas well asconversationMemory. - Emit a new
compaction.insufficientevent when compaction doesn’t get within budget (soft) and when emergency truncation also fails (hard). - Add a real-provider continuous test script plus helpers and documentation for Curator Issue #2 reproduction/verification.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
src/lib/neurolink.ts |
Makes pre-dispatch compaction reachable for inline conversationMessages and emits compaction.insufficient. |
test/continuous-test-suite-issue-02-overflow-retry.ts |
Adds a standalone reproduction/verification runner using real LiteLLM and span counting. |
test/helpers/spanCapture.ts |
Adds an OTel in-memory span capture installer for the standalone test runner. |
test/helpers/largeConversation.ts |
Adds helpers to generate oversized conversationMessages payloads for the test runner. |
test/helpers/envGuard.ts |
Adds env gating + provider-error classification for skipping real-provider tests. |
docs/curator-feedback-fixes/issue-02-overflow-retry.md |
Documents root cause, fix, and verification evidence for Curator Issue #2. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| spans.reset(); | ||
| const sdk = new NeuroLink(); | ||
| const conversation = buildLargeConversationMessages({ | ||
| targetTokens: TARGET_TOKENS, | ||
| perTurnTokens: 5_000, | ||
| }); |
There was a problem hiding this comment.
test_2_1_pre_dispatch_hard_cap_absent creates a NeuroLink instance but never calls shutdown(), unlike the other tests in this file. This can leave open handles (HTTP agents, telemetry processors, etc.) and make the script timing-dependent unless process.exit() is used. Consider wrapping the test body in a try/finally and calling await sdk.shutdown?.().catch(() => {}).
| console.log( | ||
| `\n${colors.bright}Results:${colors.reset} ${passed} passed, ${failed} failed, ${skipped} skipped`, | ||
| ); | ||
| process.exit(0); // bug repro |
There was a problem hiding this comment.
The script always exits with code 0 (process.exit(0)), even when one or more tests record FAIL. Other continuous-test-suite runners in this repo exit non-zero on failures, which makes it easier to automate regression detection. Consider exiting with process.exit(failed > 0 ? 1 : 0) (and optionally a distinct code if everything is skipped).
| process.exit(0); // bug repro | |
| process.exit(failed > 0 ? 1 : 0); |
| * rather than "the test reproduced the bug". Mirrors the convention used | ||
| * by continuous-test-suite-credentials.ts. |
There was a problem hiding this comment.
The docstring says this helper “Mirrors the convention used by continuous-test-suite-credentials.ts”, but the pattern list here is missing several strings that credentials treats as provider/credential errors (e.g. "permission denied", "failed to", "403", "not found"). Either update the list to match (to avoid misclassifying expected provider failures as test failures) or adjust the comment to reflect that it’s only a subset.
| * rather than "the test reproduced the bug". Mirrors the convention used | |
| * by continuous-test-suite-credentials.ts. | |
| * rather than "the test reproduced the bug". Uses a subset of the | |
| * provider/credential-error conventions from | |
| * continuous-test-suite-credentials.ts. |
| let i = 0; | ||
| while (out.length < wordCount) { | ||
| out.push(SENTENCES[i++ % SENTENCES.length]); |
There was a problem hiding this comment.
generateLargeText intends to generate ~wordCount words, but the loop condition uses out.length where out is an array of sentences. This will overshoot dramatically (by ~words-per-sentence factor), generating far more text/tokens than targetTokens suggests and potentially causing unexpectedly expensive real-provider calls. Consider tracking the accumulated word count (or approximate token count) instead of sentence count when deciding when to stop appending.
| let i = 0; | |
| while (out.length < wordCount) { | |
| out.push(SENTENCES[i++ % SENTENCES.length]); | |
| let generatedWordCount = 0; | |
| let i = 0; | |
| while (generatedWordCount < wordCount) { | |
| const sentence = SENTENCES[i++ % SENTENCES.length]; | |
| out.push(sentence); | |
| generatedWordCount += sentence.trim().split(/\s+/).length; |
| const dpgMessageCount = conversationMessages?.length || 0; | ||
| const dpgCompactionSessionId = this.getCompactionSessionId(options); | ||
| // Curator P1-2: pre-dispatch compaction must run for inline | ||
| // `conversationMessages` too (not just conversationMemory). Without | ||
| // this, a 1.3M-token caller-supplied conversation against a 128K | ||
| // window dispatches anyway and the provider returns | ||
| // "prompt is too long" — the bug Curator's report cited. | ||
| const dpgHasInlineMessages = | ||
| !!optionsWithMessages.conversationMessages?.length; | ||
| if ( | ||
| budgetCheck.shouldCompact && | ||
| this.conversationMemory && | ||
| (this.conversationMemory || dpgHasInlineMessages) && | ||
| dpgMessageCount > | ||
| (this.lastCompactionMessageCount.get(dpgCompactionSessionId) ?? 0) |
There was a problem hiding this comment.
The compaction watermark is keyed by getCompactionSessionId(options) which falls back to "__default__" when options.context.sessionId is absent. With the new inline-message compaction path, this can cause cross-request pollution: after one inline call sets lastCompactionMessageCount for __default__, subsequent inline calls with the same (or fewer) message count will skip compaction even if they are oversized. Consider either (a) not applying the watermark for inline conversationMessages, or (b) keying the watermark by a per-request identifier (e.g. context.requestId / random UUID) when conversationMemory is off or sessionId is missing.
| // Curator P1-2: emit `compaction.insufficient` so cost / audit | ||
| // listeners can record the specific failure mode (separate | ||
| // from a generic provider error). | ||
| try { | ||
| this.emitter.emit("compaction.insufficient", { | ||
| stagesAttempted: compactionResult.stagesUsed, | ||
| finalTokens: finalBudget.estimatedInputTokens, | ||
| budget: finalBudget.availableInputTokens, | ||
| provider: providerName, | ||
| model: options.model, | ||
| timestamp: Date.now(), | ||
| }); | ||
| } catch { | ||
| /* listener errors are non-fatal */ | ||
| } |
There was a problem hiding this comment.
In the hard-failure path (finalBudget.withinBudget === false), this code emits compaction.insufficient after emergency truncation, but an earlier compaction.insufficient was already emitted when postCompactBudget was over budget. That means hard failures will produce two compaction.insufficient events for one request, which can lead to double-counting in cost/audit listeners. Consider emitting only once per request (e.g. only the final emission), or adding a required discriminator field (like phase: "post_compaction" | "post_truncation") so listeners can interpret multiple emissions safely.
| // Curator P1-2: emit `compaction.insufficient` so cost / audit | |
| // listeners can record the specific failure mode (separate | |
| // from a generic provider error). | |
| try { | |
| this.emitter.emit("compaction.insufficient", { | |
| stagesAttempted: compactionResult.stagesUsed, | |
| finalTokens: finalBudget.estimatedInputTokens, | |
| budget: finalBudget.availableInputTokens, | |
| provider: providerName, | |
| model: options.model, | |
| timestamp: Date.now(), | |
| }); | |
| } catch { | |
| /* listener errors are non-fatal */ | |
| } | |
| // `compaction.insufficient` is emitted earlier in the | |
| // compaction flow when the post-compaction budget check fails. | |
| // Do not emit it again here after emergency truncation, or | |
| // listeners may double-count a single request failure. |
| * conversation. Capture every `ai.generateText` / `ai.streamText` | ||
| * span (real OTel) — that count equals the number of dispatched | ||
| * provider calls. |
There was a problem hiding this comment.
This file header says it captures ai.generateText / ai.streamText spans, but the actual dispatch counting logic uses neurolink.http.fetchWithRetry spans (and comments later state AI SDK spans aren’t captured by the local exporter). Update the header comment to match what’s actually being captured so the test’s measurement strategy is unambiguous.
| * conversation. Capture every `ai.generateText` / `ai.streamText` | |
| * span (real OTel) — that count equals the number of dispatched | |
| * provider calls. | |
| * conversation. Capture every `neurolink.http.fetchWithRetry` | |
| * span (real OTel) — that count is used as the number of | |
| * dispatched provider calls. |
| const spans = installSpanCapture(); | ||
|
|
||
| import { NeuroLink } from "../dist/index.js"; | ||
| import { buildLargeConversationMessages } from "./helpers/largeConversation.js"; | ||
| import { | ||
| isExpectedProviderError, | ||
| skipIfEnvMissing, | ||
| } from "./helpers/envGuard.js"; | ||
|
|
There was a problem hiding this comment.
installSpanCapture() needs to run before NeuroLink is imported, but static ESM imports are resolved/evaluated before top-level code runs. In a type: module repo this means ../dist/index.js will be loaded before installSpanCapture() executes, so spans may not be captured as intended. Consider switching the NeuroLink import to a dynamic await import() that happens after installing span capture (or otherwise restructuring to guarantee ordering).
| const spans = installSpanCapture(); | |
| import { NeuroLink } from "../dist/index.js"; | |
| import { buildLargeConversationMessages } from "./helpers/largeConversation.js"; | |
| import { | |
| isExpectedProviderError, | |
| skipIfEnvMissing, | |
| } from "./helpers/envGuard.js"; | |
| import { buildLargeConversationMessages } from "./helpers/largeConversation.js"; | |
| import { | |
| isExpectedProviderError, | |
| skipIfEnvMissing, | |
| } from "./helpers/envGuard.js"; | |
| const spans = installSpanCapture(); | |
| const { NeuroLink } = await import("../dist/index.js"); |
|
Force-pushed addressing reviewer Finding #3 + applying same recipe as the rest:
Suite re-verified: 3/3 PASS (pre-dispatch hard cap, no wasted retries, compaction.insufficient event fires). @coderabbitai full review |
04f8314 to
7300b50
Compare
|
🧠 Learnings used✅ Actions performedFull review triggered. |
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/neurolink.ts (1)
5915-5923:⚠️ Potential issue | 🟠 MajorInline history detection is still incorrect for
[], and the new watermark can now leak across stateless inline requests.This path still uses
conversationMessages?.lengthto decide whether inline history exists, soconversationMessages: []is treated as “absent” and falls back toconversationMemory. On top of that, once inline callers enter the compaction path, requests withoutcontext.sessionIdall share the"__default__"watermark, so one compacted request can suppress pre-dispatch compaction for an unrelated request on the same instance.💡 Suggested direction
- let conversationMessages = optionsWithMessages.conversationMessages - ?.length - ? optionsWithMessages.conversationMessages - : await getConversationMessages(this.conversationMemory, options); + const hasInlineConversationMessages = + optionsWithMessages.conversationMessages !== undefined; + let conversationMessages = hasInlineConversationMessages + ? optionsWithMessages.conversationMessages + : await getConversationMessages(this.conversationMemory, options); const dpgMessageCount = conversationMessages?.length || 0; - const dpgCompactionSessionId = this.getCompactionSessionId(options); - const dpgHasInlineMessages = - !!optionsWithMessages.conversationMessages?.length; + const dpgHasInlineMessages = hasInlineConversationMessages; + const requestContext = options.context as + | Record<string, unknown> + | undefined; + const dpgCompactionSessionId = + dpgHasInlineMessages && !requestContext?.sessionId + ? ((requestContext?.requestId as string | undefined) ?? + "__inline_request__") + : this.getCompactionSessionId(options);Also applies to: 5941-5953
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/neurolink.ts` around lines 5915 - 5923, The current inline-history detection uses conversationMessages?.length which treats an explicit empty array as "absent" and falls back to conversationMemory, and the fallback watermark "__default__" is shared across stateless requests causing watermark leakage; change the check to detect presence of the conversationMessages property itself (e.g., check using Object.prototype.hasOwnProperty.call(optionsWithMessages, 'conversationMessages') or a null/undefined check) so an explicit [] is honored, and replace the shared "__default__" watermark used in stateless flows with a per-request unique watermark (e.g., derive from context.sessionId when present, otherwise generate a short unique id) used by the compaction logic (affecting the code around optionsWithMessages, conversationMessages, getConversationMessages, and any watermark variable handling) so compacted state cannot leak between unrelated stateless inline requests.
🧹 Nitpick comments (3)
test/helpers/largeConversation.ts (1)
44-57: Optional: extract the message shape into a namedtypefor reuse.The inline
Array<{ role: "user" | "assistant"; content: string }>is repeated on lines 46 and 49 and is also the public return shape. Extracting it would tighten the public API and avoid drift if asystemrole is later added.♻️ Proposed extraction
+export type LargeConvMessage = { + role: "user" | "assistant"; + content: string; +}; + export function buildLargeConversationMessages( o: LargeConvOptions, -): Array<{ role: "user" | "assistant"; content: string }> { +): LargeConvMessage[] { const perTurn = o.perTurnTokens ?? 5_000; const turns = Math.ceil(o.targetTokens / perTurn); - const msgs: Array<{ role: "user" | "assistant"; content: string }> = []; + const msgs: LargeConvMessage[] = []; for (let i = 0; i < turns; i++) { msgs.push({ role: i % 2 === 0 ? "user" : "assistant", content: `Turn ${i + 1}: ${generateLargeText(perTurn)}`, }); } return msgs; }As per coding guidelines: "Always use
typefor type definitions, never useinterface."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/helpers/largeConversation.ts` around lines 44 - 57, Define a reusable type for the message shape (e.g., LargeConvMessage) and replace the inline annotations in buildLargeConversationMessages: change the function return type Array<{ role: "user" | "assistant"; content: string }> to Array<LargeConvMessage>, and update the msgs variable declaration to use LargeConvMessage as well; ensure the pushed object (role/content) conforms to LargeConvMessage and keep generateLargeText usage unchanged.test/continuous-test-suite-issue-02-overflow-retry.ts (2)
241-243: Listener registered without removal.
compaction.insufficientis added in test 2.5 with no matchingoff/removeListener. Sincesdk.shutdown()happens in thefinallyand the process exits shortly after, this is benign today, but if future tests are appended after 2.5 they'd inherit a stale counter on a shared emitter style. Tightening cleanup avoids that footgun.🧹 Suggested cleanup
- const sdk = new NeuroLink(); - let events = 0; - sdk.getEventEmitter().on("compaction.insufficient", () => events++); + const sdk = new NeuroLink(); + let events = 0; + const onInsufficient = () => events++; + sdk.getEventEmitter().on("compaction.insufficient", onInsufficient); @@ } finally { + sdk.getEventEmitter().off("compaction.insufficient", onInsufficient); await sdk.shutdown?.().catch(() => {}); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/continuous-test-suite-issue-02-overflow-retry.ts` around lines 241 - 243, A listener for "compaction.insufficient" is attached to sdk.getEventEmitter() using NeuroLink and never removed; update the test to unregister the handler after use (or use a one-time listener) to avoid leaking state across tests: capture the handler you pass to sdk.getEventEmitter().on("compaction.insufficient", handler) (or replace with a once-style registration) and call getEventEmitter().off/removeListener(handler) in the finally block alongside sdk.shutdown() so the events counter won't persist for subsequent tests.
64-66: Stale function names reflect pre-fix expectations.The function names
test_2_1_pre_dispatch_hard_cap_absentandtest_2_5_compaction_insufficient_event_missingdescribe the bug being absent/missing (the pre-fix state). The test bodies now assert the opposite — that the hard cap is enforced and the event does fire. This will be confusing to future readers.🧹 Suggested rename
-async function test_2_1_pre_dispatch_hard_cap_absent(): Promise<void> { +async function test_2_1_pre_dispatch_hard_cap_enforced(): Promise<void> {-async function test_2_5_compaction_insufficient_event_missing(): Promise<void> { +async function test_2_5_compaction_insufficient_event_fires(): Promise<void> {Update the corresponding
awaitcalls inmain()accordingly.Also applies to: 233-235
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/continuous-test-suite-issue-02-overflow-retry.ts` around lines 64 - 66, The test function names are stale: rename test_2_1_pre_dispatch_hard_cap_absent to something like test_2_1_pre_dispatch_hard_cap_enforced and rename test_2_5_compaction_insufficient_event_missing to something like test_2_5_compaction_insufficient_event_fires (or similar positive names) so they reflect the asserted behavior; update the corresponding await calls in main() to use the new function names and adjust any references to these symbols (test_2_1_pre_dispatch_hard_cap_absent, test_2_5_compaction_insufficient_event_missing, and their await invocations) to avoid dangling references.
🤖 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/neurolink.ts`:
- Around line 4890-4940: The code returns null when lastCompactionResult is
null, which silently falls back to the original provider error instead of
signaling compaction failure; update the post-loop logic to handle the case
where no compaction was attempted/succeeded by emitting/returning a
compaction.insufficient signal (or throwing ContextBudgetExceededError as
appropriate) so callers get a clear overflow response; modify the block that
checks lastCompactionResult (and any caller expectations of the function that
uses ContextCompactor.compact, escalationFractions, verifiedBudget) to either
set compaction.insufficient on the result object or throw
ContextBudgetExceededError with context (requestId, provider/model) instead of
returning null.
In `@test/continuous-test-suite-issue-02-overflow-retry.ts`:
- Line 285: The test script currently forces a successful exit via
process.exit(0) which hides any failures; update the exit logic to propagate
non-zero when there are failing tests by replacing the unconditional
process.exit(0) with a conditional exit using the test failure counter (variable
name: failed) so the process exits with 1 if failed > 0 and 0 otherwise (i.e.,
use process.exit(failed > 0 ? 1 : 0) or equivalent logic tied to failed).
- Line 23: Replace the compiled-artifact import with a source import so the test
uses the live TypeScript code: change the import of NeuroLink from
"../dist/index.js" to the project's source entry (e.g., "../src/index.ts" or the
TS-native resolution used by tsx) in the test file to avoid requiring a prior
build and to prevent stale-dist false passes; update any surrounding header
comment if needed to reflect that the test runs against source rather than the
built artifact.
In `@test/helpers/largeConversation.ts`:
- Around line 26-35: generateLargeText currently uses out.length (number of
sentences) to reach wordCount, causing a ~7–9x overshoot because SENTENCES
contains multiword sentences; change the loop in generateLargeText to count
actual words instead of sentences: compute wordCount as before, keep a running
totalWords, append full SENTENCES entries to out and increment totalWords by the
number of words in the appended sentence (split on whitespace), and when a
sentence would overshoot, append only the needed words from that SENTENCES entry
(trimmed) so totalWords equals wordCount; ensure the final return still does
out.join(" ").
---
Outside diff comments:
In `@src/lib/neurolink.ts`:
- Around line 5915-5923: The current inline-history detection uses
conversationMessages?.length which treats an explicit empty array as "absent"
and falls back to conversationMemory, and the fallback watermark "__default__"
is shared across stateless requests causing watermark leakage; change the check
to detect presence of the conversationMessages property itself (e.g., check
using Object.prototype.hasOwnProperty.call(optionsWithMessages,
'conversationMessages') or a null/undefined check) so an explicit [] is honored,
and replace the shared "__default__" watermark used in stateless flows with a
per-request unique watermark (e.g., derive from context.sessionId when present,
otherwise generate a short unique id) used by the compaction logic (affecting
the code around optionsWithMessages, conversationMessages,
getConversationMessages, and any watermark variable handling) so compacted state
cannot leak between unrelated stateless inline requests.
---
Nitpick comments:
In `@test/continuous-test-suite-issue-02-overflow-retry.ts`:
- Around line 241-243: A listener for "compaction.insufficient" is attached to
sdk.getEventEmitter() using NeuroLink and never removed; update the test to
unregister the handler after use (or use a one-time listener) to avoid leaking
state across tests: capture the handler you pass to
sdk.getEventEmitter().on("compaction.insufficient", handler) (or replace with a
once-style registration) and call getEventEmitter().off/removeListener(handler)
in the finally block alongside sdk.shutdown() so the events counter won't
persist for subsequent tests.
- Around line 64-66: The test function names are stale: rename
test_2_1_pre_dispatch_hard_cap_absent to something like
test_2_1_pre_dispatch_hard_cap_enforced and rename
test_2_5_compaction_insufficient_event_missing to something like
test_2_5_compaction_insufficient_event_fires (or similar positive names) so they
reflect the asserted behavior; update the corresponding await calls in main() to
use the new function names and adjust any references to these symbols
(test_2_1_pre_dispatch_hard_cap_absent,
test_2_5_compaction_insufficient_event_missing, and their await invocations) to
avoid dangling references.
In `@test/helpers/largeConversation.ts`:
- Around line 44-57: Define a reusable type for the message shape (e.g.,
LargeConvMessage) and replace the inline annotations in
buildLargeConversationMessages: change the function return type Array<{ role:
"user" | "assistant"; content: string }> to Array<LargeConvMessage>, and update
the msgs variable declaration to use LargeConvMessage as well; ensure the pushed
object (role/content) conforms to LargeConvMessage and keep generateLargeText
usage 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: e9fbcf62-c075-4bfb-aa73-67dd513638fe
📒 Files selected for processing (4)
src/lib/neurolink.tstest/continuous-test-suite-issue-02-overflow-retry.tstest/helpers/envGuard.tstest/helpers/largeConversation.ts
| import { installSpanCapture } from "./helpers/spanCapture.js"; | ||
| const spans = installSpanCapture(); | ||
|
|
||
| import { NeuroLink } from "../dist/index.js"; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
tsx import path conventions for in-repo TypeScript test scripts vs built dist
💡 Result:
Recommended convention: in in-repo TypeScript tests, import your in-repo modules from source paths (e.g., src/... or your tsconfig aliases pointing at src/...), and import built/dist entrypoints only when you are testing the published artifact (or when running code against the package as an external dependency). 1) Why you should not “always” import from dist in tests - TypeScript’s compilerOptions.paths is explicitly a compile/type-resolution mapping and does not automatically change what gets emitted by tsc (and it does not automatically handle runtime resolution). It’s meant to tell TypeScript where to find code; other tools must apply the mapping at runtime/bundling time. - Therefore, when your tests run directly against the repo (via tsx, vitest, Jest+ts-jest, Vite, etc.), you generally want the runtime resolver to point at the same source tree you’re executing/type-checking. 2) How to do it cleanly: separate tsconfigs for “tests+dev” vs “build” - Pattern: keep one tsconfig for editor/typechecking (includes tests), and a separate build tsconfig that only compiles src. - This avoids compiling tests into dist, while still letting your test runner/IDE resolve imports from src. Practical wiring (common) - tsconfig.json (dev/test): includes src//* and tests; defines compilerOptions.paths so aliases resolve to src. - tsconfig.build.json (build): extends the base but includes only src and emits to dist. 3) Use path aliases that target src for tests (and teach the test runner those aliases) - In tsconfig.json, configure baseUrl + paths so aliases like @/* map to src/*. - Example shape (per TS docs): paths remaps imports relative to baseUrl. - Your test runner/bundler needs to apply these aliases at runtime. - For Vitest/Vite, Vite does not inherently use TS paths the way TypeScript does; projects typically need a plugin like vite-tsconfig-paths (or Vite’s resolve.tsconfigPaths depending on versions/config). Vitest issues commonly show failures when relying on tsconfig paths without the proper runtime alias resolution. - If you don’t set up alias resolution correctly, imports in test files may fail at runtime even if TypeScript type-checking succeeds. 4) Only point tests at dist when testing the built artifact Use dist imports in two cases: - You are intentionally performing “consumer-like” tests against what a user would import (e.g., after tsc/bundler output, verify runtime entrypoints, package exports, etc.). - You are running tests in a way that executes compiled JS from dist (or runs against an installed package rather than repo source). 5) What about file extensions like .test.tsx? - It’s fine to use .tsx for test files when the test code contains JSX; the convention is about the file’s contents (JSX vs non-JSX), not about src-vs-dist. Bottom line (rule of thumb) - Default: tests in the repo should import from source (via src/... or aliases mapping to src/...). - For artifact/package verification: have a separate set of tests (or a separate step) that imports from dist/package entrypoints.
Citations:
- 1: https://www.typescriptlang.org/tsconfig/paths.html
- 2: https://stackoverflow.com/questions/35470511/setting-up-tsconfig-with-spec-test-folder
- 3: https://stackoverflow.com/questions/69567870/use-tsconfig-flags-for-test-files-but-exclude-them-from-build
- 4: paths (typescript config) don't get resolved vitest-dev/vitest#4171
- 5: Unable to both compile and run Vitest: won't resolve excluded files aleclarson/vite-tsconfig-paths#170
- 6: globalSetup file cannot use tsconfig import paths in vitest 4 vitest-dev/vitest#9411
- 7: https://stackoverflow.com/questions/56122961/can-i-use-the-tsx-extension-for-test-files-if-using-react-with-typescript
- 8: http://jestjs.io/docs/next/configuration
🏁 Script executed:
# First, find the test file
fd "continuous-test-suite-issue-02-overflow-retry" test/
# Read the file to see the import and contextRepository: juspay/neurolink
Length of output: 114
🏁 Script executed:
# Check the specific test file and its imports
if [ -f "test/continuous-test-suite-issue-02-overflow-retry.ts" ]; then
echo "=== File content (first 40 lines) ==="
head -40 "test/continuous-test-suite-issue-02-overflow-retry.ts"
echo ""
echo "=== File size ==="
wc -l "test/continuous-test-suite-issue-02-overflow-retry.ts"
else
echo "File not found"
fiRepository: juspay/neurolink
Length of output: 1493
🏁 Script executed:
# Also check if there's a tsconfig for tests and what the project structure looks like
ls -la tsconfig*.json 2>/dev/null || echo "No tsconfig files found"
echo ""
echo "=== Project structure ==="
ls -la src/ test/ 2>/dev/null | head -20Repository: juspay/neurolink
Length of output: 1272
🏁 Script executed:
# Check imports in other continuous test suites
echo "=== continuous-test-suite-auth.ts ==="
head -30 test/continuous-test-suite-auth.ts | grep -A 2 "import { NeuroLink"
echo ""
echo "=== continuous-test-suite-autoresearch-e2e.ts ==="
head -30 test/continuous-test-suite-autoresearch-e2e.ts | grep -A 2 "import { NeuroLink"
echo ""
echo "=== continuous-test-suite-autoresearch-live.ts ==="
head -30 test/continuous-test-suite-autoresearch-live.ts | grep -A 2 "import { NeuroLink"Repository: juspay/neurolink
Length of output: 201
🏁 Script executed:
# Check tsconfig.json to see what the default setup is
cat tsconfig.jsonRepository: juspay/neurolink
Length of output: 737
🏁 Script executed:
# Check if there's documentation about how to run tests
cat test/TESTING_SCRIPTS.md | head -100Repository: juspay/neurolink
Length of output: 2670
🏁 Script executed:
# Let's check the actual import statements in other test files more directly
echo "=== continuous-test-suite-auth.ts first 50 lines ==="
head -50 test/continuous-test-suite-auth.ts | cat -n
echo ""
echo "=== continuous-test-suite-autoresearch-e2e.ts first 50 lines ==="
head -50 test/continuous-test-suite-autoresearch-e2e.ts | cat -nRepository: juspay/neurolink
Length of output: 3875
🏁 Script executed:
# Check if there are any test files that import from src instead of dist
rg "import.*from.*['\"].*src/" test/ --type ts --type tsxRepository: juspay/neurolink
Length of output: 88
🏁 Script executed:
# Also check the package.json to understand the build setup and entry points
cat package.json | head -100Repository: juspay/neurolink
Length of output: 5962
🏁 Script executed:
# Check continuous-test-suite-autoresearch-e2e.ts for imports
grep -n "import.*NeuroLink\|from.*['\"]" test/continuous-test-suite-autoresearch-e2e.ts | head -20Repository: juspay/neurolink
Length of output: 236
🏁 Script executed:
# Check what autoresearch-live imports
grep -n "import.*NeuroLink\|from.*['\"]" test/continuous-test-suite-autoresearch-live.ts | head -20Repository: juspay/neurolink
Length of output: 484
🏁 Script executed:
# Check if there's any documentation about when to use dist vs src for tests
find . -name "*.md" -type f -not -path "./node_modules/*" -not -path "./.git/*" | xargs grep -l "dist.*test\|test.*src" 2>/dev/null | head -5Repository: juspay/neurolink
Length of output: 214
🏁 Script executed:
# Let's check what dist/index.js actually exports
if [ -f "dist/index.js" ]; then
echo "=== dist/index.js exists ==="
head -30 dist/index.js
else
echo "dist/index.js does not exist - this suggests dist is not built"
fiRepository: juspay/neurolink
Length of output: 123
🏁 Script executed:
# Check the test file more completely to understand its purpose
cat -n test/continuous-test-suite-issue-02-overflow-retry.ts | head -100Repository: juspay/neurolink
Length of output: 3836
🏁 Script executed:
# Check autoresearch-live to see why it imports from src
grep -B 5 -A 5 "from.*src/lib" test/continuous-test-suite-autoresearch-live.ts | head -30Repository: juspay/neurolink
Length of output: 493
🏁 Script executed:
# Check if there's a vite config or build config that explains dist structure
cat vite.config.ts | head -50Repository: juspay/neurolink
Length of output: 1852
Use source import to avoid unnecessary build coupling for this behavior test.
The test's purpose is validating token overflow retry handling with real LiteLLM—a behavioral concern—not verifying the published artifact. Importing from ../src/index.ts (or via tsx's native TypeScript resolution) eliminates the required pnpm run build step and prevents stale-code false passes during iteration. The file header documents the build requirement, but since other similar continuous test suites import from source, align this test with that pattern.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/continuous-test-suite-issue-02-overflow-retry.ts` at line 23, Replace
the compiled-artifact import with a source import so the test uses the live
TypeScript code: change the import of NeuroLink from "../dist/index.js" to the
project's source entry (e.g., "../src/index.ts" or the TS-native resolution used
by tsx) in the test file to avoid requiring a prior build and to prevent
stale-dist false passes; update any surrounding header comment if needed to
reflect that the test runs against source rather than the built artifact.
7300b50 to
9d2e754
Compare
|
Force-pushed addressing reviewer Finding #4 (standalone oversized prompt/tool payload): The pre-dispatch compaction block was gated on Fix: added a pre-dispatch hard cap branch BEFORE the existing compaction block:
Suite re-verified: 3/3 PASS. @coderabbitai full review |
|
✅ Actions performedFull review triggered. |
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
9d2e754 to
df7f70e
Compare
|
Force-pushed addressing the reviewer's CI-enforcement follow-up:
Same correction applied to #990 in the parallel push. @coderabbitai full review |
|
✅ Actions performedFull review triggered. |
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
df7f70e to
7daf5b4
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 |
7daf5b4 to
aa4af7e
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 |
Round-2 update — strict end-to-end verification + stream-path fix gaps closedPushed The previous test was too lenient — Gaps the strict tests caught (now fixed in this commit)
Test rewrite (
|
|
🧠 Learnings used✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
src/lib/neurolink.ts (1)
5187-5188:⚠️ Potential issue | 🟠 MajorDon't fall back to the opaque provider overflow when compaction never engages.
If every escalation attempt returns
compacted: false, thisreturn nullbypasses bothcompaction.insufficientand the typedContextBudgetExceededError, so callers still see the old provider 400 path.Suggested direction
- if (!lastCompactionResult) { - return null; - } + if (!lastCompactionResult) { + const exhaustedBudget = checkContextBudget({ + provider: options.provider || "openai", + model: options.model, + maxTokens: options.maxTokens, + systemPrompt: options.systemPrompt, + currentPrompt: options.prompt, + conversationMessages: originalMessages as Array<{ + role: string; + content: string; + }>, + }); + + try { + this.emitter.emit("compaction.insufficient", { + stagesAttempted: [], + finalTokens: exhaustedBudget.estimatedInputTokens, + budget: exhaustedBudget.availableInputTokens, + provider: options.provider || "openai", + model: options.model, + phase: "post-provider-recovery", + fractionsTried: escalationFractions, + timestamp: Date.now(), + }); + } catch { + /* listener errors are non-fatal */ + } + + throw new ContextBudgetExceededError( + "Context overflow recovery could not reduce the conversation enough to retry safely.", + { + estimatedTokens: exhaustedBudget.estimatedInputTokens, + availableTokens: exhaustedBudget.availableInputTokens, + breakdown: exhaustedBudget.breakdown, + }, + ); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/neurolink.ts` around lines 5187 - 5188, The early "return null" when !lastCompactionResult lets callers fall back to the opaque provider overflow path instead of surfacing compaction failures; change the behavior so that when no compaction ever engaged (i.e., lastCompactionResult is falsy or every escalation record has compacted: false) you either return the compaction.insufficient sentinel or throw the typed ContextBudgetExceededError instead of null. Locate uses of lastCompactionResult in this routine and replace the null return with logic that inspects escalation results for any compacted:true; if none exist, return compaction.insufficient (or throw new ContextBudgetExceededError with the same message used elsewhere) so callers see the intended compaction/limit outcome rather than the provider 400 path.
🤖 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/neurolink.ts`:
- Around line 6203-6207: The condition currently uses this.conversationMemory to
decide whether to bypass the hard cap, but that can be true while there are zero
actually compactable messages; update the if so it checks the real compactable
history (e.g., compactableMessages.length or a computed
compactableMessageCount/hasCompactableMessages) instead of
this.conversationMemory so the hard cap is enforced when there are no
compactable messages; keep the other checks (budgetCheck.withinBudget and
dpgHasInlineMessages) and ensure this change stays consistent with the later
compaction logic that examines dpgMessageCount and streamMessageCount.
- Around line 6302-6310: The compaction.insufficient emissions (the emitter.emit
calls that build the payload with stagesAttempted, finalTokens, budget,
provider, model, willEmergencyTruncate, timestamp) are missing a phase
discriminator; update each such call (e.g., the
emitter.emit("compaction.insufficient", ...) instances referenced around the
blocks in neurolink.ts, including the occurrences near the shown diff and the
ones at the other ranges mentioned) to include a phase field (string) that
identifies this as the mid-compaction signal (for example phase: "mid" or
"during"); ensure the same phase key/value is added consistently to all
compaction.insufficient payloads so listeners can distinguish mid-compaction
from terminal failure emissions.
- Around line 6208-6233: The ContextBudgetExceededError thrown inside the
per-provider try block (e.g., after emitter.emit("compaction.insufficient") and
the throw new ContextBudgetExceededError(...)) is being swallowed by the
surrounding provider catch and treated as an ordinary provider failure; update
the provider-level catch to detect if (err instanceof
ContextBudgetExceededError) and rethrow it immediately so budget-exceeded errors
propagate out of the direct-provider loop (do the same for the similar throw
site around the other block at the noted location). Ensure you reference
ContextBudgetExceededError in the catch path and do not wrap or convert it into
a generic provider failure.
In `@test/continuous-test-suite-issue-02-overflow-retry.ts`:
- Around line 515-535: The test currently only checks events and oversized
arrays but never verifies the thrown error type; update the PASS/FAIL logic so
it asserts the outcome actually indicates a ContextBudgetExceededError: ensure
out.ok is false (or equivalent failure flag) and that the error payload (e.g.,
out.error, out.err, or out.throwable) is an instance of or has name
"ContextBudgetExceededError" before recording PASS; if that check fails record a
FAIL with a clear message including out (similar to existing messages). Locate
and modify the block using testName, oversized, events, dispatches, out and
record to add this explicit typed-error assertion and adjust the PASS/FAIL
branches accordingly.
- Around line 166-187: The test is matching the identifier dpgHasInlineMessages
(inlineGate regex) which will be mangled by minification; update the test and
code so the marker is a preserved string literal instead of an identifier: in
the source around the inline-gate logic (where dpgHasInlineMessages is used in
src/lib/neurolink.ts) emit a stable marker string or audit/event literal (for
example a dedicated "pre-dispatch-inline-gate" or similar) and then change the
test's markers.inlineGate from /dpgHasInlineMessages/ to a regex or exact match
for that new literal; ensure the new literal is actually present in the shipped
artifact and adjust the record() checks that reference markers and missing
accordingly.
In `@test/helpers/fetchCapture.ts`:
- Around line 66-71: The method and body extraction mis-handle Request inputs:
change the method expression to prefer init.method, then fall back to
input.method when input is a Request, and finally default to "GET" (e.g.,
replace current computation used to produce method with something like:
init?.method ?? (input instanceof Request ? input.method : "GET")). Also
populate body from init.body if provided, otherwise if input is a Request
extract the request's body (e.g., use input.body or clone the Request to read
its body) before calling bytesOf so bodyBytes reflects Request bodies; update
references around bytesOf and the forBodyOver/MAX_COMPACTED_BODY_BYTES check
accordingly.
---
Duplicate comments:
In `@src/lib/neurolink.ts`:
- Around line 5187-5188: The early "return null" when !lastCompactionResult lets
callers fall back to the opaque provider overflow path instead of surfacing
compaction failures; change the behavior so that when no compaction ever engaged
(i.e., lastCompactionResult is falsy or every escalation record has compacted:
false) you either return the compaction.insufficient sentinel or throw the typed
ContextBudgetExceededError instead of null. Locate uses of lastCompactionResult
in this routine and replace the null return with logic that inspects escalation
results for any compacted:true; if none exist, return compaction.insufficient
(or throw new ContextBudgetExceededError with the same message used elsewhere)
so callers see the intended compaction/limit outcome rather than the provider
400 path.
🪄 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: caaba832-c40d-4d98-a0fc-37fd471d00a8
📒 Files selected for processing (4)
src/lib/neurolink.tstest/continuous-test-suite-issue-02-overflow-retry.tstest/helpers/fetchCapture.tstest/helpers/largeConversation.ts
aa4af7e to
1514ee9
Compare
Round-3 update — addressed 7 reviewer findings (
|
|
I'll kick off the full review of 🧠 Learnings used✅ Actions performedFull review triggered. |
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
test/helpers/largeConversation.ts (1)
26-35:⚠️ Potential issue | 🟠 MajorWord/sentence accounting still overshoots
targetTokensby ~7–9×.
out.lengthmeasures sentence count, but it is being compared againstwordCount, so each sentence (avg ~7–9 words) effectively contributes one unit toward the word budget. WithperTurnTokens = 5_000, this emits ~35k+ tokens per turn rather than ~5k, which skews fraction-truncation thresholds, "single-round insufficient" calibration, and real-provider cost/latency by an order of magnitude. The previously-suggested fix (track awordscounter and split on whitespace) still applies.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/helpers/largeConversation.ts` around lines 26 - 35, The generateLargeText function is overshooting because it compares out.length (sentence count) to wordCount; change the loop to track an explicit word counter (e.g., words) and append sentences split on whitespace from SENTENCES, adding only as many words as needed to reach wordCount and slicing the last sentence if necessary; keep wordsPerToken and wordCount calculation but replace the out.length check with words < wordCount and build out as an array of words (or join after collecting), ensuring generateLargeText, SENTENCES, wordsPerToken, wordCount, and out are the only symbols touched.
🧹 Nitpick comments (1)
test/helpers/fetchCapture.ts (1)
75-101: Request-body-as-stream case inforBodyOveris a defensive concern, not current practice.When
input instanceof Request, the body is aReadableStream, whichbytesOfreturns−1for. The filterforBodyOver(bytes)then silently excludes entries wherebodyBytes > bytesfails. While no code in the SDK currently dispatches viafetch(new Request(...)), this creates a theoretical correctness gap: if any code path were added that does so with a large body,forBodyOver(MAX_COMPACTED_BODY_BYTES)would miss it.This is defensive future-proofing; the actual code paths use plain URL +
initdispatch, which work as intended. If desired, consider one of:
- Clone the
Requestand read its body to text before re-dispatch for accurate byte measurement- Or include
−1entries inforBodyOverresults with a note they are unmeasurable🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/helpers/fetchCapture.ts` around lines 75 - 101, The forBodyOver filter currently excludes records where bytesOf returned -1 for streaming Request bodies, causing large-stream requests to be missed; update the implementation referenced by forBodyOver (and consider bytesOf and the fetch wrapper that records into records) so it treats bodyBytes === -1 as "unmeasurable" and includes those records in the result — i.e., change the predicate to return records where r.bodyBytes === -1 || r.bodyBytes > bytes (or alternatively clone and read Request bodies in the fetch wrapper to compute actual bytes if you prefer precise measurement).
🤖 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/neurolink.ts`:
- Around line 367-372: The code currently marks ContextBudgetExceededError as
globally non-retryable which prevents directProviderGeneration() from trying
alternate providers; change this so ContextBudgetExceededError remains retryable
when an "auto" provider fallback is possible: in the error handling branch for
ContextBudgetExceededError, either remove the unconditional "return true" or
conditionally return true only when the provider is fixed (e.g., provider !==
"auto" or similar flag available), so that directProviderGeneration() can
continue to attempt other provider candidates; reference
ContextBudgetExceededError and directProviderGeneration() when locating the
change.
In `@test/continuous-test-suite-issue-02-overflow-retry.ts`:
- Line 405: The test title "2.3 — no wasted retries: 145K-token conversation
produces ≤1 compacted dispatch" is inconsistent with the implemented pass gate
which checks dispatches.length <= 2 and reports "≤ 2"; update either the
human-readable title string to state "≤2" (change the literal test name) or
change the enforcement to dispatches.length <= 1 and its report text so the
title, check, and message all match; ensure you update all occurrences (the
title at the quoted string and the related assertion/reporting logic around the
dispatches.length check seen also in the block around lines 461-473).
- Line 230: The optional-call pattern `await sdk.shutdown?.().catch(() => {})`
can produce a TypeError when `sdk.shutdown` is undefined because the `?.` only
short-circuits the call, not the subsequent `.catch`; update all occurrences of
this pattern (the `sdk.shutdown` calls) to either drop the `?.` and assert the
method exists (e.g., `if (sdk.shutdown) await sdk.shutdown().catch(...)`) or
propagate the optional chaining to the catch (e.g., use optional chaining on the
catch call), ensuring you safely handle the case where `sdk.shutdown` is
undefined and avoid `undefined.catch` errors.
---
Duplicate comments:
In `@test/helpers/largeConversation.ts`:
- Around line 26-35: The generateLargeText function is overshooting because it
compares out.length (sentence count) to wordCount; change the loop to track an
explicit word counter (e.g., words) and append sentences split on whitespace
from SENTENCES, adding only as many words as needed to reach wordCount and
slicing the last sentence if necessary; keep wordsPerToken and wordCount
calculation but replace the out.length check with words < wordCount and build
out as an array of words (or join after collecting), ensuring generateLargeText,
SENTENCES, wordsPerToken, wordCount, and out are the only symbols touched.
---
Nitpick comments:
In `@test/helpers/fetchCapture.ts`:
- Around line 75-101: The forBodyOver filter currently excludes records where
bytesOf returned -1 for streaming Request bodies, causing large-stream requests
to be missed; update the implementation referenced by forBodyOver (and consider
bytesOf and the fetch wrapper that records into records) so it treats bodyBytes
=== -1 as "unmeasurable" and includes those records in the result — i.e., change
the predicate to return records where r.bodyBytes === -1 || r.bodyBytes > bytes
(or alternatively clone and read Request bodies in the fetch wrapper to compute
actual bytes if you prefer precise measurement).
🪄 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: 99977907-db08-44d6-847d-f88be6a2a4c3
📒 Files selected for processing (4)
src/lib/neurolink.tstest/continuous-test-suite-issue-02-overflow-retry.tstest/helpers/fetchCapture.tstest/helpers/largeConversation.ts
| maxTokens: 64, | ||
| disableTools: true, | ||
| }); | ||
| await sdk.shutdown?.().catch(() => {}); |
There was a problem hiding this comment.
Broken optional-chain swallows a TypeError when shutdown is absent.
await sdk.shutdown?.().catch(() => {}) only short-circuits the call, not the .catch. If sdk.shutdown is ever undefined, the expression evaluates to undefined and then undefined.catch(...) throws a TypeError, aborting the test run before subsequent assertions/cleanup. Either drop the ?. (if shutdown is part of the public API) or chain the ?. through .catch.
🛠️ Proposed fix
- await sdk.shutdown?.().catch(() => {});
+ await sdk.shutdown?.()?.catch(() => {});Also applies to: 319-319, 427-427, 511-511
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/continuous-test-suite-issue-02-overflow-retry.ts` at line 230, The
optional-call pattern `await sdk.shutdown?.().catch(() => {})` can produce a
TypeError when `sdk.shutdown` is undefined because the `?.` only short-circuits
the call, not the subsequent `.catch`; update all occurrences of this pattern
(the `sdk.shutdown` calls) to either drop the `?.` and assert the method exists
(e.g., `if (sdk.shutdown) await sdk.shutdown().catch(...)`) or propagate the
optional chaining to the catch (e.g., use optional chaining on the catch call),
ensuring you safely handle the case where `sdk.shutdown` is undefined and avoid
`undefined.catch` errors.
…ionMessages on both generate and stream paths + compaction.insufficient event
Curator P1-2: when callers passed `conversationMessages` directly (without
enabling conversationMemory), the pre-dispatch compaction block in
directProviderGeneration was bypassed and the SDK dispatched the raw
oversized payload to the provider. Reproduction showed a real outbound
HTTP call carrying 1,331,024 input tokens to a 128K-window LiteLLM model.
Root cause: gating conditions in both `directProviderGeneration` (generate)
and `createMCPStream` (stream) required `&& this.conversationMemory`. The
inline-conversationMessages path skipped compaction entirely on both paths,
and `handleStreamError` would then re-dispatch the raw oversized payload as
a fallback.
Fix:
1. Compaction runs whenever EITHER conversationMemory is set OR
caller-provided conversationMessages exist. Applied symmetrically to
`directProviderGeneration` AND `createMCPStream`.
2. Pre-dispatch hard cap: when budget check fails AND there is nothing
to compact (no memory + no inline messages — only a huge prompt or
huge tool definitions), throw ContextBudgetExceededError before any
HTTP roundtrip. Applied to both paths.
3. `handleStreamError` rethrows ContextBudgetExceededError instead of
falling back to a same-payload provider.stream() call — that fallback
was undoing the hard cap on the stream path.
4. New `compaction.insufficient` event emitted from three sites on each
path:
- pre-dispatch hard cap (phase: pre-dispatch-no-recovery)
- post-compaction over budget (willEmergencyTruncate: true)
- post-emergency-truncation still over budget (terminal failure)
Lets cost / audit listeners record the specific failure mode.
Test (`test/continuous-test-suite-issue-02-overflow-retry.ts`):
• Wraps `globalThis.fetch` BEFORE NeuroLink is imported (real fetch
runs; we just observe body bytes — no mocking).
• Drives every behavior twice — once via `sdk.generate()` and once via
`sdk.stream()`.
• Asserts on dispatched body size (compacted payload < 1.5MB threshold
vs bug-state 5MB), event emission, dispatch count, typed error
propagation.
Reproduction (real LiteLLM, 200K / 145K / 500K conversations against
128K-window model):
before: stream-path huge prompt dispatched 1.3M tokens; 0 events on
stream overflow; handleStreamError re-dispatched on overflow.
after: 9/9 PASS across generate + stream
- hard cap throws before any dispatch (0 chat-completion POSTs)
- compaction reduces 200K conv to 443KB / 428KB body
- exactly 1 dispatch for 145K conv (no retry storm)
- compaction.insufficient event fires on overflow recoveries
1514ee9 to
76f00a8
Compare
Round-4 update — addressed 5 reviewer findings (
|
|
Kicking off the full review of 🧠 Learnings used✅ Actions performedFull review triggered. |
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
|
🎉 This PR is included in version 9.59.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Summary
Curator P1-2: when callers passed
conversationMessagesdirectly (without enabling conversationMemory), the pre-dispatch compaction block indirectProviderGenerationwas bypassed and the SDK dispatched the raw oversized payload to the provider. Real reproduction showed a 1,331,024-token outbound HTTP call to a 128K-window model.Root cause
The compaction-gate condition required
&& this.conversationMemory. Inline-conversationMessagescallers skipped compaction entirely:Fix
conversationMemoryis enabled OR caller-providedconversationMessagesexist. Same compactor + same emergency-truncation safety net.compaction.insufficientevent. Fires when (a) post-compaction budget is still over (single round insufficient — emergency truncation will save), or (b) emergency truncation also fails (hard failure,ContextBudgetExceededErrorthrown). Lets cost / audit listeners record the specific signal separately from the eventual outcome.Reproduction (real LiteLLM, 1.06M-word conversation, 128K model)
The
neurolink.context.compactspan proves pre-dispatch compaction ran. The singleneurolink.http.fetchWithRetryproves no wasted retries.Backward compatibility
Compaction running for inline-conversationMessages callers is the fix, not a regression. Callers that previously relied on the SDK not compacting and surfacing a provider 400 directly must catch the existing
ContextBudgetExceededError. Thecompaction.insufficientevent is purely additive.Verification
Test plan
neurolink.context.compactspan)compaction.insufficientevent emitted at expected boundarySummary by CodeRabbit
Bug Fixes
New Features
Tests