fix(core): consolidate multiple CoreToolScheduler instances into single shared instance #1060
Conversation
…le shared instance (closes #1055) This fix addresses the issue of multiple CoreToolScheduler instances being created across the application, which led to: - Multiple MessageBus subscribers generating unknown correlationId spam logs - Memory leaks from schedulers that never get disposed() - Inconsistent lifecycle management across different components Changes: - Added Config-based singleton pattern with getOrCreateScheduler() and disposeScheduler() methods - Created dedicated schedulerSingleton.ts module to avoid circular dependencies - Updated all 5 instantiation points to use the singleton: - CLI Interactive Mode (useReactToolScheduler.ts) - UI Web (useToolScheduler.ts) - A2A Server (task.ts) - Subagents (subagent.ts) - Non-Interactive Executor kept as-is (temporary properly-disposed instances) - Added proper disposal logic on component unmount and session end - Exported PolicyEngine, PolicyDecision, PolicyRule types from core index - Added comprehensive tests for singleton behavior Benefits: - Only ONE CoreToolScheduler instance per session - All agents/subagents share the same scheduler instance - No unknown correlationId log spam - All schedulers properly dispose() on session end - No memory leaks from abandoned schedulers
WalkthroughCentralizes CoreToolScheduler into a session-scoped, lazily-initialized singleton (getOrCreateScheduler), migrates callers (Task, subagents, CLI/UI hooks) to await the shared scheduler, adds lifecycle APIs (dispose/disposeScheduler/clearAllSchedulers), and updates tests and mocks to the async/shared scheduler flow. Changes
Sequence Diagram(s)sequenceDiagram
participant Component as UI / CLI / Task / Subagent
participant Config as Config.getOrCreateScheduler
participant Singleton as SchedulerSingleton
participant CoreSched as CoreToolScheduler
Component->>Config: getOrCreateScheduler(sessionId, callbacks)
activate Config
Config->>Singleton: request scheduler for sessionId
activate Singleton
alt cached instance exists
Singleton-->>Config: resolves cached CoreSched (refCount++)
else create new instance
Singleton->>CoreSched: dynamic import & new CoreToolScheduler(options)
activate CoreSched
CoreSched-->>Singleton: instantiated
Singleton->>Singleton: cache instance (refCount=1)
end
Singleton-->>Config: CoreSched
Config-->>Component: Promise resolves -> CoreSched
deactivate Singleton
deactivate Config
Component->>CoreSched: schedule(toolCall)
CoreSched->>CoreSched: handle scheduling, publish/subscribe (single MessageBus sub)
CoreSched-->>Component: onToolCallsUpdate / output updates / completion
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
github-actions
bot
added
the
maintainer:e2e:ok
|
I'll analyze this pull request that consolidates multiple CoreToolScheduler instances into a single shared instance. Let me check which files were actually added in this PR: Let me check the actual structure of the files: Let me check for the new singleton implementation that was mentioned in the PR: Let me examine the key instantiation points that were updated: > LLxprt PR Review – PR #1060Issue Alignment
Side Effects
Code Quality
Tests & Coverage
Verdict |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
packages/a2a-server/src/agent/task.ts (1)
61-121: Avoidnull as anyforscheduler; use definite assignment (or nullable type) instead.
This currently bypasses type safety and the repo TS guideline. (Line 90-92).Proposed fix (type-safe initialization)
export class Task { id: string; contextId: string; - scheduler: CoreToolScheduler; + scheduler!: CoreToolScheduler; config: Config; geminiClient: GeminiClient; pendingToolConfirmationDetails: Map<string, ToolCallConfirmationDetails>; taskState: TaskState; eventBus?: ExecutionEventBus; completedToolCalls: CompletedToolCall[]; skipFinalTrueAfterInlineEdit = false; @@ private constructor( id: string, contextId: string, config: Config, eventBus?: ExecutionEventBus, ) { @@ this.contextId = contextId; this.config = config; - // eslint-disable-next-line @typescript-eslint/no-explicit-any - this.scheduler = null as any; const contentConfig = this.config.getContentGeneratorConfig();packages/ui/src/hooks/useToolScheduler.ts (1)
287-373: Major: callbacks can outlive the component (no mounted guard + no deregistration).
Because callbacks are registered into the session singleton (Line 341-353) and aren’t removed on cleanup (Line 369-373), the scheduler can keep calling intosetToolCallsafter unmount and also retain closures indefinitely.Local mitigation (guard handlers + remove unused isInitializing)
- let mounted = true; - let isInitializing = true; + let mounted = true; @@ const handleOutputUpdate = ( toolCallId: string, outputChunk: string, ): void => { + if (!mounted) return; setToolCalls((prev) => applyOutputUpdate(prev, toolCallId, outputChunk)); }; @@ const handleToolCallsUpdate = (updatedCalls: CoreToolCall[]): void => { + if (!mounted) return; setToolCalls((prevCalls) => { if (updatedCalls.length === 0) { return []; } @@ const handleAllComplete = async ( completedToolCalls: CompletedToolCall[], ): Promise<void> => { @@ await onCompleteRef.current(completedToolCalls); @@ handleToolCallsUpdate([]); }; @@ - } finally { - isInitializing = false; - } + } finally { + // no-op + } };If
Config.getOrCreateScheduler()stores callbacks per session (as the PR summary indicates), the real fix is to add a core-level “unregister callbacks” mechanism (token-based) and call it from this cleanup. Based on learnings, this is important to prevent long-running session leaks.packages/cli/src/ui/hooks/useReactToolScheduler.ts (3)
191-238: Handle errors in async scheduler initialization (avoid unhandled rejection).
initializeScheduler()awaitsconfig.getOrCreateScheduler(...)with no try/catch; failures will surface as unhandled promise rejections.Proposed fix
useEffect(() => { let mounted = true; const initializeScheduler = async () => { - const instance = await config.getOrCreateScheduler(sessionId, { - outputUpdateHandler: (toolCallId, chunk) => - updateToolCallOutput(mainSchedulerId, toolCallId, chunk), - onAllToolCallsComplete: async (completedToolCalls) => { - if (completedToolCalls.length > 0) { - await onComplete(mainSchedulerId, completedToolCalls, { - isPrimary: true, - }); - } - replaceToolCallsForScheduler(mainSchedulerId, []); - }, - onToolCallsUpdate: (calls) => { - replaceToolCallsForScheduler(mainSchedulerId, calls); - }, - getPreferredEditor, - onEditorClose, - onEditorOpen, - }); - - if (mounted) { - setScheduler(instance); - } + try { + const instance = await config.getOrCreateScheduler(sessionId, { + outputUpdateHandler: (toolCallId, chunk) => + updateToolCallOutput(mainSchedulerId, toolCallId, chunk), + onAllToolCallsComplete: async (completedToolCalls) => { + if (completedToolCalls.length > 0) { + await onComplete(mainSchedulerId, completedToolCalls, { + isPrimary: true, + }); + } + replaceToolCallsForScheduler(mainSchedulerId, []); + }, + onToolCallsUpdate: (calls) => { + replaceToolCallsForScheduler(mainSchedulerId, calls); + }, + getPreferredEditor, + onEditorClose, + onEditorOpen, + }); + + if (mounted) setScheduler(instance); + } catch { + if (mounted) setScheduler(null); + } };
251-276: UseschedulerConfig.getSessionId()insidecreateExternalScheduler(don’t capture outer sessionId).
Right now a subagent could be created with a different config/session but still bind to the outer session’s scheduler.Proposed fix
- const instance = await schedulerConfig.getOrCreateScheduler(sessionId, { + const externalSessionId = schedulerConfig.getSessionId(); + const instance = await schedulerConfig.getOrCreateScheduler(externalSessionId, { // Only update the local UI state - don't call outputUpdateHandler as well, // since that would cause duplicate output (the subagent's outputUpdateHandler // calls onMessage which goes to task.updateOutput, creating a second display). // The local updateToolCallOutput handles the UI rendering for subagent tools.
239-294: Fix type signature:ExternalSchedulerFactoryshould be async to match implementation.
createExternalScheduleris an async function (line 240) that returnsPromise<{ schedule(...) }>, but is cast toExternalSchedulerFactorywhich is typed as a synchronous function returning{ schedule(...) }immediately (line 25). While the current invocation inpackages/core/src/core/subagent.ts:688wraps the factory call withPromise.resolve()and correctly awaits the result, the deceptive type signature creates a footgun: future callers may assume the factory is synchronous and call it withoutawait, causing runtime failures. Update theExternalSchedulerFactorytype to be(args) => Promise<{ schedule(...) }>to match the actual async implementation.
🤖 Fix all issues with AI agents
In @packages/core/src/config/config.scheduler.test.ts:
- Around line 22-43: The test uses a cast to any for mockSettingsService in the
Config constructor (configParams.settingsService) which violates the TypeScript
rule; replace the any cast with a proper structural type or unknown that matches
the methods used (get, set, getAllGlobalSettings, getProviderSettings,
setProviderSetting, clear) and update other occurrences noted (around lines
where mockSettingsService is created and the other instances at 215-238) so the
mock object is typed as that interface or as unknown cast to the expected
SettingsService type accepted by Config, ensuring you keep method signatures
compatible with vi.fn().
- Around line 7-16: The test file is calling beforeAll but it isn't imported
from vitest; update the top import where describe, it, expect, beforeEach, vi
are imported to also include beforeAll so the beforeAll(async () => { ... })
hook works correctly (i.e., add beforeAll to the import list from 'vitest'
alongside the existing symbols).
In @packages/core/src/config/config.scheduler.test.ts.bak:
- Around line 1-300: Remove the accidental backup file named
config.scheduler.test.ts.bak from the PR (delete
packages/core/src/config/config.scheduler.test.ts.bak) so only the real test
file remains; ensure it is unstaged/removed from the commit and not re-added
(git rm --cached or delete and amend the commit) and update .gitignore if your
editor keeps creating such backups.
In @packages/core/src/config/config.ts:
- Around line 1864-1892: getOrCreateScheduler currently stores multiple callback
objects in schedulerCallbacks but the CoreToolScheduler instance created only
uses the first callback set at construction, so subsequent registrants never
receive updates; fix by constructing CoreToolScheduler with "fanout" handler
functions (e.g., outputUpdateHandler, onAllToolCallsComplete, onToolCallsUpdate,
getPreferredEditor, onEditorClose, onEditorOpen) that iterate over
this.schedulerCallbacks.get(sessionId) and invoke each registered callback,
update the existing-scheduler branch to push new callbacks as now but ensure
those callbacks are used by the fanout handlers, and add methods to
register/unregister callbacks (or expose an unregister function on registration)
so callers can remove their callback to prevent memory leaks and session growth
while keeping schedulerInstances and schedulerCallbacks in sync.
- Around line 1841-1862: The getters schedulerInstances and schedulerCallbacks
currently mutate untyped runtime state via (this as any) and ESLint
suppressions; replace this pattern by delegating to the scheduler singleton
module (schedulerSingleton.ts) or by introducing properly typed private fields
on Config (e.g., private _schedulerInstances: Map<string, CoreToolScheduler> and
private _schedulerCallbacks: Map<string, SchedulerCallbacks[]>) using type-only
imports for CoreToolScheduler and SchedulerCallbacks so no runtime import cycle
occurs; update the schedulerInstances and schedulerCallbacks getters to return
the singleton maps (or the new typed private fields) and remove all (this as
any) casts and eslint-disable comments.
- Around line 1837-1901: getOrCreateScheduler in config.ts contains a race where
concurrent calls can each construct a scheduler (via schedulerInstances/
schedulerCallbacks) and the file duplicates singleton logic instead of
delegating to schedulerSingleton._getOrCreateScheduler; fix by removing the
local getOrCreateScheduler implementation and delegating calls to the shared
_getOrCreateScheduler exported from schedulerSingleton.ts, and update
disposeScheduler to forward to the singleton's dispose method; in
schedulerSingleton.ts add a per-session in‑flight Promise map or mutex to ensure
only one CoreToolScheduler is constructed for a session (serialize creation and
return the same Promise for concurrent calls), and replace the unchecked (this
as any)._schedulerInstances/_schedulerCallbacks usages with properly typed
private fields or at minimum unknown + type guards to eliminate the any
suppressions.
In @packages/core/src/config/schedulerSingleton.ts:
- Around line 26-29: The code currently allows async TOCTOU duplicate scheduler
creation because schedulerInstances is a plain Map and schedulerCallbacks are
never invoked; fix by introducing an in-flight creation map (e.g.,
inFlightSchedulers: Map<string, Promise<Scheduler>>) and use it in the same
factory function that creates schedulers so concurrent callers await the same
Promise instead of creating new instances, ensure schedulerInstances stores the
final Scheduler (typed, not any), and on fulfillment fan out/invoke any stored
callbacks in schedulerCallbacks for that sessionId; also remove the
inFlightSchedulers entry on both success and failure (and propagate errors) so
retries work correctly.
- Around line 14-24: The SchedulerCallbacks interface is using `any`; replace
those with safer types: change onAllToolCallsComplete and onToolCallsUpdate to
accept `unknown[]` instead of `any[]`, and change getPreferredEditor to return
`unknown` (or a concrete Editor type if available), then add runtime/typing
guards where these callbacks are invoked to narrow the `unknown` before using
(update call sites that rely on specific properties to perform type checks or
casts). Update the interface members `onAllToolCallsComplete`,
`onToolCallsUpdate`, and `getPreferredEditor` accordingly and ensure any code
calling them performs appropriate type guards or mappings.
- Around line 71-74: getSchedulerInstance currently returns any; change its
signature to return the proper scheduler type instead of any (e.g., Scheduler |
undefined or ConcreteSchedulerClass | undefined) and remove the eslint-disable
comment. Update the function signature from getSchedulerInstance(sessionId:
string): any to use the imported Scheduler type (or the concrete class/interface
used in schedulerInstances) and ensure schedulerInstances.get(sessionId) is
typed accordingly so the function returns the correct typed value rather than
any.
- Around line 62-69: The disposeScheduler function currently calls
scheduler.dispose() and then deletes maps, which means if dispose() throws the
maps won't be cleaned; change disposeScheduler to retrieve the scheduler from
schedulerInstances, call scheduler.dispose() inside a try block and perform
schedulerInstances.delete(sessionId) and schedulerCallbacks.delete(sessionId)
inside a finally block so cleanup always runs; if you want to preserve original
error behavior rethrow the caught error after the finally.
🧹 Nitpick comments (10)
packages/core/src/core/subagent.ts (1)
686-706: Verify error handling for async scheduler creation.The shift from synchronous to asynchronous scheduler provisioning introduces new failure modes. While the current try-catch in
runInteractive(line 712) will catch errors fromawait schedulerPromise, ensure that:
- Scheduler creation failures are logged appropriately
- The error messages distinguish between scheduler factory failures and
getOrCreateSchedulerfailures- Subagent cleanup is performed even if scheduler creation fails
Consider adding explicit error context around the scheduler creation:
🛡️ Suggested error handling enhancement
- const schedulerPromise = options?.schedulerFactory - ? Promise.resolve( - options.schedulerFactory({ - schedulerConfig, - onAllToolCallsComplete: handleCompletion, - outputUpdateHandler, - onToolCallsUpdate: undefined, - }), - ) - : (async () => { - const sessionId = schedulerConfig.getSessionId(); - return await schedulerConfig.getOrCreateScheduler(sessionId, { - outputUpdateHandler, - onAllToolCallsComplete: handleCompletion, - onToolCallsUpdate: undefined, - getPreferredEditor: () => undefined, - onEditorClose: () => {}, - }); - })(); - - const scheduler = await schedulerPromise; + let scheduler; + try { + const schedulerPromise = options?.schedulerFactory + ? Promise.resolve( + options.schedulerFactory({ + schedulerConfig, + onAllToolCallsComplete: handleCompletion, + outputUpdateHandler, + onToolCallsUpdate: undefined, + }), + ) + : (async () => { + const sessionId = schedulerConfig.getSessionId(); + return await schedulerConfig.getOrCreateScheduler(sessionId, { + outputUpdateHandler, + onAllToolCallsComplete: handleCompletion, + onToolCallsUpdate: undefined, + getPreferredEditor: () => undefined, + onEditorClose: () => {}, + }); + })(); + + scheduler = await schedulerPromise; + } catch (error) { + this.logger.warn( + () => + `Subagent ${this.subagentId} failed to create scheduler: ${error instanceof Error ? error.message : String(error)}`, + ); + throw error; + }Based on learnings, this change supports the consolidation to a single CoreToolScheduler instance per interactive session.
packages/a2a-server/src/agent/task.ts (1)
431-440: GuardsessionIdbefore callinggetOrCreateScheduler().
IfgetSessionId()can be empty/undefined for any config mode, fail fast with a clear error.Proposed fix (fail fast)
private async createScheduler(): Promise<CoreToolScheduler> { const sessionId = this.config.getSessionId(); + if (!sessionId) { + throw new Error('[Task] Missing sessionId; cannot create/get CoreToolScheduler'); + } return await this.config.getOrCreateScheduler(sessionId, { outputUpdateHandler: this._schedulerOutputUpdate.bind(this), onAllToolCallsComplete: this._schedulerAllToolCallsComplete.bind(this), onToolCallsUpdate: this._schedulerToolCallsUpdate.bind(this), getPreferredEditor: () => 'vscode', onEditorClose: () => {}, }); }packages/core/src/config/config.scheduler.test.ts (1)
128-149: Strengthen the disposal test by asserting “new instance” after dispose.
Right now it only asserts “defined”, not “different from the disposed instance”.Proposed fix
it('should dispose and remove the scheduler for a given sessionId', async () => { @@ - await config.getOrCreateScheduler(testSessionId, callbacks); + const oldScheduler = await config.getOrCreateScheduler(testSessionId, callbacks); @@ const newScheduler = await config.getOrCreateScheduler( testSessionId, callbacks, ); expect(newScheduler).toBeDefined(); + expect(newScheduler).not.toBe(oldScheduler); });packages/ui/src/hooks/useToolScheduler.ts (1)
399-406: Prefer exposingcancelAll()in the scheduler type instead of casting.
The double-cast suggests the public type surface is out of sync with runtime (Line 403-405).packages/cli/src/ui/hooks/useToolScheduler.test.ts (1)
48-75: MockgetOrCreateSchedulershould behave like a singleton (and avoidany).
Right now it creates a newCoreToolSchedulerevery call (Line 66-73), which can hide “duplicate subscription / callback accumulation” bugs the PR is meant to prevent.Proposed fix (per-session cache)
const mockConfig = { @@ - getOrCreateScheduler: vi.fn(async (_sessionId: string, callbacks: any) => { + getOrCreateScheduler: vi.fn(async (_sessionId: string, callbacks: unknown) => { const { CoreToolScheduler: CoreToolSchedulerClass } = await import('@vybestack/llxprt-code-core'); - return new CoreToolSchedulerClass({ - ...callbacks, - config: mockConfig as unknown as Config, - }); + // Minimal singleton behavior for tests + const key = _sessionId ?? 'default'; + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const self = mockConfig as any; + self.__schedulerBySession ??= new Map<string, unknown>(); + if (!self.__schedulerBySession.has(key)) { + self.__schedulerBySession.set( + key, + new CoreToolSchedulerClass({ + ...(callbacks as Record<string, unknown>), + config: mockConfig as unknown as Config, + }), + ); + } + return self.__schedulerBySession.get(key); }),packages/cli/src/ui/hooks/useReactToolScheduler.ts (1)
324-327: Replaceconsole.warnwith the repo logger.
Even ifconsole.warnisn’t explicitly banned, it bypasses your structured logging pipeline and makes tests noisier.Also applies to: 409-412
packages/core/src/config/config.ts (2)
85-89: Avoid two competing “singleton” implementations; delegate fromConfigtoschedulerSingleton.Right now
ConfigimportsgetOrCreateScheduler/disposeSchedulerbut then re-implements its own per-instance maps later. That risks reintroducing multiple schedulers if some codepaths use the module singleton and others use theConfig-local one, and it duplicates the code you’ll need to harden (locking, callback fanout, error handling).Proposed change (delegate, remove duplication)
import { - getOrCreateScheduler as _getOrCreateScheduler, - disposeScheduler as _disposeScheduler, + getOrCreateScheduler as _getOrCreateScheduler, + disposeScheduler as _disposeScheduler, type SchedulerCallbacks, } from './schedulerSingleton.js';- /** - * CoreToolScheduler singleton methods - * Implemented as getters to avoid circular dependencies with CoreToolScheduler - */ - private get schedulerInstances(): Map< - string, - import('../core/coreToolScheduler.js').CoreToolScheduler - > { - // eslint-disable-next-line @typescript-eslint/no-explicit-any - if (!(this as any)._schedulerInstances) { - // eslint-disable-next-line @typescript-eslint/no-explicit-any - (this as any)._schedulerInstances = new Map(); - } - // eslint-disable-next-line @typescript-eslint/no-explicit-any - return (this as any)._schedulerInstances; - } - - private get schedulerCallbacks(): Map<string, SchedulerCallbacks[]> { - // eslint-disable-next-line @typescript-eslint/no-explicit-any - if (!(this as any)._schedulerCallbacks) { - // eslint-disable-next-line @typescript-eslint/no-explicit-any - (this as any)._schedulerCallbacks = new Map(); - } - // eslint-disable-next-line @typescript-eslint/no-explicit-any - return (this as any)._schedulerCallbacks; - } - async getOrCreateScheduler( sessionId: string, callbacks: SchedulerCallbacks, ): Promise<import('../core/coreToolScheduler.js').CoreToolScheduler> { - let scheduler = this.schedulerInstances.get(sessionId); - - if (!scheduler) { - // Use dynamic import to avoid circular dependency issues - const { CoreToolScheduler: CoreToolSchedulerClass } = - await import('../core/coreToolScheduler.js'); - scheduler = new CoreToolSchedulerClass({ - config: this, - outputUpdateHandler: callbacks.outputUpdateHandler, - onAllToolCallsComplete: callbacks.onAllToolCallsComplete, - onToolCallsUpdate: callbacks.onToolCallsUpdate, - getPreferredEditor: callbacks.getPreferredEditor, - onEditorClose: callbacks.onEditorClose, - onEditorOpen: callbacks.onEditorOpen, - }); - this.schedulerInstances.set(sessionId, scheduler); - this.schedulerCallbacks.set(sessionId, [callbacks]); - } else { - const existingCallbacks = this.schedulerCallbacks.get(sessionId) || []; - existingCallbacks.push(callbacks); - this.schedulerCallbacks.set(sessionId, existingCallbacks); - } - - return scheduler; + return _getOrCreateScheduler(this, sessionId, callbacks); } disposeScheduler(sessionId: string): void { - const scheduler = this.schedulerInstances.get(sessionId); - if (scheduler) { - scheduler.dispose(); - this.schedulerInstances.delete(sessionId); - this.schedulerCallbacks.delete(sessionId); - } + _disposeScheduler(sessionId); }
1894-1901: Make disposal robust:dispose()throwing should not prevent map cleanup.If
scheduler.dispose()throws, you’ll leak the map entry and keep the scheduler reachable. Prefertry/finallyand delete entries regardless.packages/core/src/config/schedulerSingleton.ts (2)
76-86:clearAllSchedulers()is fine for tests, but consider shrinking public surface.If this is primarily for tests, consider not re-exporting it from your public index (or mark it internal) to avoid “global teardown” being called in production codepaths by mistake.
1-86: Follow-up risk: CoreToolScheduler still needs reentrancy safety for parallel tool completions.Based on learnings,
packages/core/src/core/coreToolScheduler.tshas a known race inpublishBufferedResults()under parallel tool completion. With a single shared scheduler per session, this can become more visible; worth addressing in a follow-up PR with a mutex/serialization around publish. Based on learnings, this is a known hang class.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
packages/a2a-server/src/agent/task.test.tspackages/a2a-server/src/agent/task.tspackages/a2a-server/src/utils/testing_utils.tspackages/cli/src/ui/hooks/useReactToolScheduler.tspackages/cli/src/ui/hooks/useToolScheduler.test.tspackages/cli/vitest.config.tspackages/core/src/config/config.scheduler.test.tspackages/core/src/config/config.scheduler.test.ts.bakpackages/core/src/config/config.tspackages/core/src/config/schedulerSingleton.tspackages/core/src/core/coreToolScheduler.tspackages/core/src/core/subagent.tspackages/core/src/index.tspackages/ui/src/hooks/useToolScheduler.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Don't useany- Always specify proper types. Useunknownif the type is truly unknown and add proper type guards.
Do not useconsole.logorconsole.debug- Use the sophisticated logging system instead. Log files are written to ~/.llxprt/debug/
Fix all linting errors, including warnings aboutanytypes
Files:
packages/core/src/config/config.scheduler.test.tspackages/a2a-server/src/agent/task.test.tspackages/cli/vitest.config.tspackages/core/src/core/subagent.tspackages/ui/src/hooks/useToolScheduler.tspackages/core/src/config/schedulerSingleton.tspackages/core/src/core/coreToolScheduler.tspackages/a2a-server/src/utils/testing_utils.tspackages/core/src/index.tspackages/core/src/config/config.tspackages/cli/src/ui/hooks/useReactToolScheduler.tspackages/a2a-server/src/agent/task.tspackages/cli/src/ui/hooks/useToolScheduler.test.ts
🧠 Learnings (5)
📓 Common learnings
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2026-01-09T02:26:33.951Z
Learning: Preference: Consolidate to a single CoreToolScheduler instance per interactive session to avoid duplicated handlers and unknown correlationId noise.
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2025-12-26T14:17:41.983Z
Learning: In packages/core/src/core/coreToolScheduler.ts, the `publishBufferedResults()` method (line 1355) is called concurrently by multiple tool completion handlers (line 1510 in each tool's `.then()` callback) when parallel tools finish simultaneously. This causes a race condition where `nextPublishIndex` can be corrupted and tools can hang in 'executing' state forever, blocking the scheduler and causing subsequent tool calls to queue indefinitely. The method must be made reentrant-safe using a mutex/lock pattern to serialize concurrent calls.
📚 Learning: 2025-12-26T14:17:41.983Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2025-12-26T14:17:41.983Z
Learning: In packages/core/src/core/coreToolScheduler.ts, the `publishBufferedResults()` method (line 1355) is called concurrently by multiple tool completion handlers (line 1510 in each tool's `.then()` callback) when parallel tools finish simultaneously. This causes a race condition where `nextPublishIndex` can be corrupted and tools can hang in 'executing' state forever, blocking the scheduler and causing subsequent tool calls to queue indefinitely. The method must be made reentrant-safe using a mutex/lock pattern to serialize concurrent calls.
Applied to files:
packages/core/src/config/config.scheduler.test.tspackages/core/src/core/subagent.tspackages/core/src/config/config.scheduler.test.ts.bakpackages/ui/src/hooks/useToolScheduler.tspackages/core/src/config/schedulerSingleton.tspackages/core/src/core/coreToolScheduler.tspackages/core/src/config/config.tspackages/cli/src/ui/hooks/useReactToolScheduler.tspackages/a2a-server/src/agent/task.tspackages/cli/src/ui/hooks/useToolScheduler.test.ts
📚 Learning: 2026-01-09T02:26:33.951Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2026-01-09T02:26:33.951Z
Learning: Preference: Consolidate to a single CoreToolScheduler instance per interactive session to avoid duplicated handlers and unknown correlationId noise.
Applied to files:
packages/core/src/config/config.scheduler.test.tspackages/core/src/core/subagent.tspackages/core/src/config/config.scheduler.test.ts.bakpackages/ui/src/hooks/useToolScheduler.tspackages/core/src/config/schedulerSingleton.tspackages/core/src/core/coreToolScheduler.tspackages/core/src/config/config.tspackages/cli/src/ui/hooks/useReactToolScheduler.tspackages/a2a-server/src/agent/task.ts
📚 Learning: 2025-12-05T14:59:34.368Z
Learnt from: CR
Repo: vybestack/llxprt-code PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-05T14:59:34.368Z
Learning: Applies to **/*.{ts,tsx} : Don't use `any` - Always specify proper types. Use `unknown` if the type is truly unknown and add proper type guards.
Applied to files:
packages/cli/vitest.config.ts
📚 Learning: 2025-12-05T14:59:34.368Z
Learnt from: CR
Repo: vybestack/llxprt-code PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-05T14:59:34.368Z
Learning: Use subagents (general-purpose, typescript-code-reviewer, or typescript-coder) for complex multi-step tasks, research, complex implementations, code review, and extensive file searching
Applied to files:
packages/core/src/core/subagent.ts
🧬 Code graph analysis (4)
packages/core/src/config/config.scheduler.test.ts (3)
packages/core/src/core/coreToolScheduler.ts (1)
CoreToolScheduler(387-1927)packages/core/src/config/config.ts (1)
Config(370-1902)packages/core/src/config/schedulerSingleton.ts (1)
clearAllSchedulers(76-86)
packages/ui/src/hooks/useToolScheduler.ts (2)
packages/core/src/index.ts (1)
sessionId(184-184)scripts/lint.js (1)
config(290-290)
packages/core/src/config/schedulerSingleton.ts (1)
packages/core/src/config/config.ts (5)
SchedulerCallbacks(1905-1905)schedulerInstances(1841-1852)schedulerCallbacks(1854-1862)getOrCreateScheduler(1864-1892)disposeScheduler(1894-1901)
packages/cli/src/ui/hooks/useToolScheduler.test.ts (1)
packages/core/src/config/config.ts (1)
Config(370-1902)
⏰ Context from checks skipped due to timeout of 270000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Slow E2E - Win
- GitHub Check: E2E Test (macOS)
- GitHub Check: E2E Test (Linux) - sandbox:none
- GitHub Check: E2E Test (Linux) - sandbox:docker
🔇 Additional comments (8)
packages/core/src/core/subagent.ts (1)
44-45: LGTM! Imports are necessary for async scheduler pattern.The
SubagentSchedulerFactorytype import andToolErrorTypeimport support the new asynchronous scheduler provisioning flow introduced in this PR.packages/core/src/core/coreToolScheduler.ts (1)
376-376: LGTM! Export enables external scheduler creation.Exporting
CoreToolSchedulerOptionsenables the new scheduler singleton pattern introduced inschedulerSingleton.tswhile maintaining type safety. This change supports the PR's goal of consolidating scheduler instances per session.packages/a2a-server/src/agent/task.test.ts (1)
35-42: LGTM! Mock scheduler provides complete test coverage.The mock scheduler implementation correctly provides all necessary methods (
schedule,cancelAll,dispose,toolCalls) required by the Task class. Theanytype assertion is acceptable in test context and is properly documented with the eslint-disable comment.packages/core/src/index.ts (2)
11-11: LGTM! Scheduler singleton export enables per-session scheduler management.This export provides external access to the scheduler singleton pattern, supporting the PR's objective of consolidating multiple CoreToolScheduler instances into a single shared instance per session.
Based on learnings, this enables the single scheduler instance per interactive session pattern.
13-19: LGTM! Policy exports reorganized using barrel pattern.The consolidation of policy exports through a barrel export (
./policy/index.js) with explicit re-exports of key types maintains the public API while improving code organization. This change doesn't introduce breaking changes.packages/a2a-server/src/utils/testing_utils.ts (1)
54-69: LGTM! Complete mock implementation supports async scheduler pattern.The
getOrCreateSchedulermock correctly implements the new asynchronous scheduler provisioning interface with a complete mock scheduler object. The mock includes all necessary methods (schedule,cancelAll,dispose,toolCalls,getPreferredEditor) and properly wires theconfigreference back tomockConfigfor test consistency.The use of
anyfor callbacks is acceptable in test utilities and is properly documented with the eslint-disable comment.Based on learnings, this mock supports testing the single scheduler instance per session pattern.
packages/cli/vitest.config.ts (1)
19-73: Targeted Vitest include/exclude change looks consistent (no unintended self-exclusion).
The new explicit include forsrc/ui/hooks/useToolScheduler.test.ts(Line 104-106) should still run despite the remaining hook.spec.tsexclusions (Line 70).Also applies to: 94-106
packages/core/src/config/config.ts (1)
1904-1906: Export is fine; ensure only one source of truth forSchedulerCallbacks.
| async getOrCreateScheduler( | ||
| sessionId: string, | ||
| callbacks: SchedulerCallbacks, | ||
| ): Promise<import('../core/coreToolScheduler.js').CoreToolScheduler> { | ||
| let scheduler = this.schedulerInstances.get(sessionId); | ||
|
|
||
| if (!scheduler) { | ||
| // Use dynamic import to avoid circular dependency issues | ||
| const { CoreToolScheduler: CoreToolSchedulerClass } = | ||
| await import('../core/coreToolScheduler.js'); | ||
| scheduler = new CoreToolSchedulerClass({ | ||
| config: this, | ||
| outputUpdateHandler: callbacks.outputUpdateHandler, | ||
| onAllToolCallsComplete: callbacks.onAllToolCallsComplete, | ||
| onToolCallsUpdate: callbacks.onToolCallsUpdate, | ||
| getPreferredEditor: callbacks.getPreferredEditor, | ||
| onEditorClose: callbacks.onEditorClose, | ||
| onEditorOpen: callbacks.onEditorOpen, | ||
| }); | ||
| this.schedulerInstances.set(sessionId, scheduler); | ||
| this.schedulerCallbacks.set(sessionId, [callbacks]); | ||
| } else { | ||
| const existingCallbacks = this.schedulerCallbacks.get(sessionId) || []; | ||
| existingCallbacks.push(callbacks); | ||
| this.schedulerCallbacks.set(sessionId, existingCallbacks); | ||
| } | ||
|
|
||
| return scheduler; | ||
| } |
There was a problem hiding this comment.
Callback registration is misleading: callbacks beyond the first aren’t wired into the scheduler.
In the “existing scheduler” branch you append to schedulerCallbacks, but the scheduler keeps using only the initial callbacks passed at construction. If multiple consumers expect updates (CLI UI + subagents + tasks), only the first registrant will receive them.
Suggested direction: create the scheduler with “fanout” handlers that iterate over the current callback list, and add a way to unregister callbacks to avoid leaks when React hooks remount/unmount. Based on learnings, this is especially important to avoid long-running session growth.
🤖 Prompt for AI Agents
In @packages/core/src/config/config.ts around lines 1864 - 1892,
getOrCreateScheduler currently stores multiple callback objects in
schedulerCallbacks but the CoreToolScheduler instance created only uses the
first callback set at construction, so subsequent registrants never receive
updates; fix by constructing CoreToolScheduler with "fanout" handler functions
(e.g., outputUpdateHandler, onAllToolCallsComplete, onToolCallsUpdate,
getPreferredEditor, onEditorClose, onEditorOpen) that iterate over
this.schedulerCallbacks.get(sessionId) and invoke each registered callback,
update the existing-scheduler branch to push new callbacks as now but ensure
those callbacks are used by the fanout handlers, and add methods to
register/unregister callbacks (or expose an unregister function on registration)
so callers can remove their callback to prevent memory leaks and session growth
while keeping schedulerInstances and schedulerCallbacks in sync.
- Add clearAllSchedulers() cleanup to CLI test afterEach hooks to prevent memory leaks - Update a2a-server mock scheduler to properly simulate tool-update events - Fix import statements and remove unused eslint-disable directives - All lint and typecheck warnings now resolved
- Import from @vybestack/llxprt-code-core instead of dist path - Fixes TypeScript compilation errors
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/a2a-server/src/utils/testing_utils.ts (2)
54-93: Replaceanytypes with proper type definitions orunknownwith type guards.The mock extensively uses
anytypes (lines 56, 60, 64) with eslint-disable comments, which violates the coding guidelines. As per coding guidelines, proper types should be specified, orunknownshould be used with type guards if the type is truly unknown.Consider:
- Defining a proper interface for the
_callbacksparameter instead ofany- Typing
mockSchedulerwith the expected scheduler interface or usingunknownwith proper type assertions- Replacing
any[]in the request parameter with a proper array type (e.g.,Array<{ callId: string; name: string }>)♻️ Suggested type improvements
+ interface SchedulerCallbacks { + getPreferredEditor?: (...args: unknown[]) => unknown; + } + + interface ToolRequest { + callId: string; + name: string; + } + getOrCreateScheduler: vi.fn().mockImplementation( - // eslint-disable-next-line @typescript-eslint/no-explicit-any - async (_sessionId: string, _callbacks: any) => { + async (_sessionId: string, _callbacks: SchedulerCallbacks) => { // Mock getOrCreateScheduler for tests // Return a complete mock scheduler with all necessary methods - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const mockScheduler: any = { + const mockScheduler = { schedule: vi.fn().mockImplementation( async ( - // eslint-disable-next-line @typescript-eslint/no-explicit-any - request: { callId: string; name: string } | any[], + request: ToolRequest | ToolRequest[], _signal: unknown, ) => {
68-68: Fix misleading comment.The comment states "Publish 'state-change' event with 'state-change' kind" but the code (line 71) publishes
'tool-update'with kind'tool-call-update'. Update the comment to accurately reflect the actual behavior.📝 Suggested correction
- // Simulate the real scheduler behavior: - // 1. Publish 'state-change' event with 'state-change' kind + // Simulate the real scheduler behavior by publishing a tool-update event const messageBus = mockConfig.getMessageBus();
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/a2a-server/src/utils/testing_utils.tspackages/cli/src/ui/hooks/useToolScheduler.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/cli/src/ui/hooks/useToolScheduler.test.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Don't useany- Always specify proper types. Useunknownif the type is truly unknown and add proper type guards.
Do not useconsole.logorconsole.debug- Use the sophisticated logging system instead. Log files are written to ~/.llxprt/debug/
Fix all linting errors, including warnings aboutanytypes
Files:
packages/a2a-server/src/utils/testing_utils.ts
🧠 Learnings (1)
📓 Common learnings
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2026-01-09T02:26:33.951Z
Learning: Preference: Consolidate to a single CoreToolScheduler instance per interactive session to avoid duplicated handlers and unknown correlationId noise.
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2025-12-26T14:17:41.983Z
Learning: In packages/core/src/core/coreToolScheduler.ts, the `publishBufferedResults()` method (line 1355) is called concurrently by multiple tool completion handlers (line 1510 in each tool's `.then()` callback) when parallel tools finish simultaneously. This causes a race condition where `nextPublishIndex` can be corrupted and tools can hang in 'executing' state forever, blocking the scheduler and causing subsequent tool calls to queue indefinitely. The method must be made reentrant-safe using a mutex/lock pattern to serialize concurrent calls.
- Delete all .bak files from repository - Install UI dependencies to fix linting issues - All CI checks now passing
- Move ToolConfirmationOutcome to type-only import - Remove unused isInitializing variable - Avoid floating promises with void schedulerRef.current?.?.dispose() - Properly handle optional dispose call
All imports from @vybestack/llxprt-code-core are only used as types. Move them to the type-only import to satisfy @typescript-eslint/consistent-type-imports
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/ui/src/hooks/useToolScheduler.ts (1)
7-15: Fix ESLint error: useimport typefor type-only imports.
CoreToolScheduleris no longer instantiated directly (the singleton is obtained viaconfig.getOrCreateScheduler()), so it's only used as a type annotation. This triggers the pipeline failure.Proposed fix
-import { - CoreToolScheduler, - type ToolCall as CoreToolCall, - type CompletedToolCall, - type WaitingToolCall, - type ExecutingToolCall, - type CancelledToolCall, - type ToolCallConfirmationDetails, -} from '@vybestack/llxprt-code-core'; +import type { + CoreToolScheduler, + ToolCall as CoreToolCall, + CompletedToolCall, + WaitingToolCall, + ExecutingToolCall, + CancelledToolCall, + ToolCallConfirmationDetails, +} from '@vybestack/llxprt-code-core';
🤖 Fix all issues with AI agents
In @packages/ui/src/hooks/useToolScheduler.ts:
- Around line 369-375: The cleanup currently calls
schedulerRef.current.dispose() which bypasses the singleton registry; instead
capture or store the sessionId (e.g., in a ref like sessionIdRef) when creating
the scheduler and on unmount call config.disposeScheduler(sessionIdRef.current)
to remove it from the registry, then set mounted = false and
schedulerRef.current = null; replace the direct dispose() call in the return
cleanup with this registry-based dispose and ensure sessionId is initialized
where the scheduler is created so the cleanup has access to it.
🧹 Nitpick comments (1)
packages/ui/src/hooks/useToolScheduler.ts (1)
403-409: Consider updatingCoreToolSchedulertype definition to includecancelAll.The type cast suggests the interface is incomplete. If
cancelAllis a stable API onCoreToolScheduler, consider updating the type definition in@vybestack/llxprt-code-coreto avoid this runtime cast.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/ui/bun.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
integration-tests/token-tracking-property.test.ts.bakpackages/cli/src/providers/providerManagerInstance.ts.bakpackages/cli/src/ui/hooks/useGeminiStream.test.tsx.bakpackages/ui/src/hooks/useToolScheduler.ts
💤 Files with no reviewable changes (3)
- packages/cli/src/ui/hooks/useGeminiStream.test.tsx.bak
- integration-tests/token-tracking-property.test.ts.bak
- packages/cli/src/providers/providerManagerInstance.ts.bak
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Don't useany- Always specify proper types. Useunknownif the type is truly unknown and add proper type guards.
Do not useconsole.logorconsole.debug- Use the sophisticated logging system instead. Log files are written to ~/.llxprt/debug/
Fix all linting errors, including warnings aboutanytypes
Files:
packages/ui/src/hooks/useToolScheduler.ts
🧠 Learnings (5)
📓 Common learnings
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2026-01-09T02:26:33.951Z
Learning: Preference: Consolidate to a single CoreToolScheduler instance per interactive session to avoid duplicated handlers and unknown correlationId noise.
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2025-12-26T14:17:41.983Z
Learning: In packages/core/src/core/coreToolScheduler.ts, the `publishBufferedResults()` method (line 1355) is called concurrently by multiple tool completion handlers (line 1510 in each tool's `.then()` callback) when parallel tools finish simultaneously. This causes a race condition where `nextPublishIndex` can be corrupted and tools can hang in 'executing' state forever, blocking the scheduler and causing subsequent tool calls to queue indefinitely. The method must be made reentrant-safe using a mutex/lock pattern to serialize concurrent calls.
📚 Learning: 2025-12-26T14:17:41.983Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2025-12-26T14:17:41.983Z
Learning: In packages/core/src/core/coreToolScheduler.ts, the `publishBufferedResults()` method (line 1355) is called concurrently by multiple tool completion handlers (line 1510 in each tool's `.then()` callback) when parallel tools finish simultaneously. This causes a race condition where `nextPublishIndex` can be corrupted and tools can hang in 'executing' state forever, blocking the scheduler and causing subsequent tool calls to queue indefinitely. The method must be made reentrant-safe using a mutex/lock pattern to serialize concurrent calls.
Applied to files:
packages/ui/src/hooks/useToolScheduler.ts
📚 Learning: 2026-01-09T02:26:33.951Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2026-01-09T02:26:33.951Z
Learning: Preference: Consolidate to a single CoreToolScheduler instance per interactive session to avoid duplicated handlers and unknown correlationId noise.
Applied to files:
packages/ui/src/hooks/useToolScheduler.ts
📚 Learning: 2025-11-17T08:33:42.962Z
Learnt from: MinWooPark-dotcom
Repo: vybestack/llxprt-code PR: 577
File: packages/cli/src/ui/components/messages/ToolMessage.tsx:89-167
Timestamp: 2025-11-17T08:33:42.962Z
Learning: In the vybestack/llxprt-code repository, single-use helper functions in React components can be kept inline rather than extracted to separate utilities, especially when the extraction doesn't provide immediate value (no reuse, no isolated testing planned) and keeps the PR scope focused on feature delivery.
Applied to files:
packages/ui/src/hooks/useToolScheduler.ts
📚 Learning: 2025-11-16T22:51:26.374Z
Learnt from: e2720pjk
Repo: vybestack/llxprt-code PR: 583
File: packages/core/src/providers/openai/OpenAIProvider.ts:935-959
Timestamp: 2025-11-16T22:51:26.374Z
Learning: In the llxprt-code codebase (packages/core/src/providers/openai/OpenAIProvider.ts), tools like `run_shell_command` use internal streaming only for real-time UI updates during execution, but each tool execution produces exactly ONE final `ToolResponseBlock` containing the full result. The streaming chunks are never sent to the LLM and are not converted into multiple tool messages. The OpenAI Chat Completions API requires that each tool call (tool_call_id) corresponds to exactly one message with role 'tool', so duplicate tool response detection that removes subsequent tool messages with the same tool_call_id is correct and necessary for API compliance.
Applied to files:
packages/ui/src/hooks/useToolScheduler.ts
🪛 GitHub Actions: LLxprt Code CI
packages/ui/src/hooks/useToolScheduler.ts
[error] 7-7: ESLint: All imports in the declaration are only used as types. Use 'import type' to fix.
[error] 1-1: Lint step 'bun run lint' failed due to ESLint error(s).
⏰ Context from checks skipped due to timeout of 270000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: E2E Test (Linux) - sandbox:none
- GitHub Check: E2E Test (Linux) - sandbox:docker
- GitHub Check: E2E Test (macOS)
- GitHub Check: Slow E2E - Win
🔇 Additional comments (1)
packages/ui/src/hooks/useToolScheduler.ts (1)
340-367: Async initialization pattern looks correct with proper mounted guards.The
initializeSchedulerfunction correctly:
- Uses the singleton via
config.getOrCreateScheduler(sessionId, callbacks)- Guards
schedulerRef.currentassignment withmountedcheck- Handles errors gracefully with logging
One minor consideration: the handler callbacks (
handleOutputUpdate,handleToolCallsUpdate) don't checkmountedbefore callingsetToolCalls. React 19 handles unmounted state updates gracefully, so this is acceptable, but for extra robustness you could add mounted guards to the handlers as well.
|
@CodeRabbit we're borked on this test -- any idea why it leaks : ✓ Shell npx vitest run packages/cli/src/ui/hooks/useToolScheduler.test.ts --root packages/cli --coverage false -t "tool call" (Run the first tool scheduler test that exercises a tool call.) <--- Last few GCs ---> [28555:0x138008000] 39089 ms: Mark-Compact (reduce) 4092.6 (4098.9) -> 4072.6 (4078.5) MB, pooled: 0.0 MB, 557.49 / 0.00 ms (+ 0.7 ms in 1 steps since start of marking, 1: 0x108918d5c node::OOMErrorHandler(char const*, v8::OOMDetails const&) [/opt/homebrew/Cellar/node/25.2.1/lib/libnode.141.dylib] ⎯⎯⎯⎯ Unhandled Rejection ⎯⎯⎯⎯⎯ |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@acoliver Let me examine the test file to understand the memory leak. 🏁 Script executed: Length of output: 35094 @acoliver I found the memory leak! The issue is in the test mock setup at line 67-73 of getOrCreateScheduler: vi.fn(async (_sessionId: string, callbacks: any) => {
const { CoreToolScheduler: CoreToolSchedulerClass } =
await import('@vybestack/llxprt-code-core');
return new CoreToolSchedulerClass({
...callbacks,
config: mockConfig as unknown as Config,
});
}),The problem: The mock is creating real
When tests run with fake timers and multiple The fix: Replace the real scheduler with a lightweight mock: getOrCreateScheduler: vi.fn(async (_sessionId: string, callbacks: any) => {
return {
schedule: vi.fn().mockResolvedValue(undefined),
cancelAll: vi.fn(),
dispose: vi.fn(),
toolCalls: [],
getPreferredEditor: vi.fn(),
config: mockConfig,
toolRegistry: mockToolRegistry,
// Add any other methods/properties needed by tests
};
}),This matches the pattern already used in Would you like me to generate the complete fix for the test file? 🧠 Learnings used |
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/core/src/core/coreToolScheduler.ts (1)
505-526: Avoid leakingpendingConfirmationson “missing signal” pathIf you decide to ignore late confirmations, also delete the correlationId mapping (otherwise it can linger until disposal).
Proposed fix
const originalSignal = this.callIdToSignal.get(callId); if (!originalSignal) { if (toolSchedulerLogger.enabled) { toolSchedulerLogger.debug( () => `Skipping TOOL_CONFIRMATION_RESPONSE for callId=${callId} because signal is missing (call already finalized).`, ); } + this.pendingConfirmations.delete(response.correlationId); return; }packages/cli/src/ui/hooks/useReactToolScheduler.ts (1)
200-252: Critical: Memory leak if scheduler initialization throws before completion.If
initializeSchedulerthrows an exception afterconfig.getOrCreateSchedulersucceeds (line 205) but beforeresolved = true(line 224), the cleanup function (lines 247-252) won't dispose the scheduler because it only disposes whenresolvedis true. This creates a memory leak with lingering subscriptions and timers.🔧 Proposed fix
Move
resolved = trueimmediately after scheduler creation, or restructure to track the scheduler instance directly:useEffect(() => { let mounted = true; - let resolved = false; + let createdScheduler: CoreToolScheduler | null = null; const initializeScheduler = async () => { const instance = await config.getOrCreateScheduler(sessionId, { outputUpdateHandler: (toolCallId, chunk) => updateToolCallOutput(mainSchedulerId, toolCallId, chunk), onAllToolCallsComplete: async (completedToolCalls) => { if (completedToolCalls.length > 0) { await onComplete(mainSchedulerId, completedToolCalls, { isPrimary: true, }); } replaceToolCallsForScheduler(mainSchedulerId, []); }, onToolCallsUpdate: (calls) => { replaceToolCallsForScheduler(mainSchedulerId, calls); }, getPreferredEditor, onEditorClose, onEditorOpen, }); + createdScheduler = instance; - resolved = true; if (!mounted) { config.disposeScheduler?.(sessionId); return; } // ... rest of function }; void initializeScheduler(); return () => { mounted = false; - if (resolved) { + if (createdScheduler) { config.disposeScheduler?.(sessionId); } }; }, [ // ... dependencies ]);
🤖 Fix all issues with AI agents
In @packages/cli/src/ui/hooks/useCommandCompletion.test.ts:
- Around line 385-389: The test description is inconsistent with its
expectations: update the test case description string (the object with
expectedSuggestions and expectedShowSuggestions in useCommandCompletion.test) so
it reflects that suggestions should be shown when shellModeActive is true (e.g.,
change "should not show slash command suggestions when shellModeActive is true"
to "should show slash command suggestions when shellModeActive is true").
In @packages/cli/src/ui/hooks/useReactToolScheduler.ts:
- Around line 253-264: The effect that calls getOrCreateScheduler is re-running
because callback props (onComplete, replaceToolCallsForScheduler,
updateToolCallOutput, getPreferredEditor, onEditorClose, onEditorOpen) are in
its dependency array; remove those callbacks from that lifecycle effect so the
scheduler is only created/disposed by stable keys (config, sessionId,
mainSchedulerId), and add a second effect that watches the callback functions
and calls scheduler?.setCallbacks({...}) (or passes them into
getOrCreateScheduler if it updates callbacks internally) whenever they change;
specifically update the useEffect in useReactToolScheduler to only depend on
config/sessionId/mainSchedulerId for creation/disposal and introduce a separate
useEffect that depends on scheduler plus the listed callbacks to call
setCallbacks (using the scheduler variable and getOrCreateScheduler/setCallbacks
symbols).
In @packages/cli/src/ui/hooks/useSlashCompletion.test.ts:
- Line 1253: The test assertion is wrong: update the expectation in
useSlashCompletion.test so that after selecting the 'add' sub-command (index 1)
from the '/memory' parent the buffer contains '/memory add ' instead of '/add ';
change the assertion on result.current.textBuffer.text to expect '/memory add '
and then run tests, and also add a follow-up ticket to inspect
handleAutocomplete / useSlashCompletion logic to ensure sub-commands are
appended (not replacing the parent command).
In @packages/cli/src/ui/hooks/useToolScheduler.test.ts:
- Around line 85-91: The test uses the real scheduler via getOrCreateScheduler
delegating to actualGetOrCreateScheduler which creates CoreToolScheduler
instances and causes OOM; replace that vi.fn delegation with a lightweight mock
object implementing the minimal scheduler API used in tests (methods/properties:
schedule, cancelAll, dispose, toolCalls, getPreferredEditor, config,
toolRegistry) and make schedule a jest/vitest spy (e.g., schedule.mock) so
existing assertions can be adapted; remove any creation paths that call
actualGetOrCreateScheduler or CoreToolScheduler to avoid registering MessageBus
subscribers/timers and update assertions to reference the mock's spy methods
instead of real scheduler behavior.
In @packages/core/src/config/config.scheduler.test.ts:
- Around line 22-54: Tests leak scheduler resources because many create
schedulers but don't dispose them; add an afterEach that disposes any schedulers
created by the test and still clears singleton state: add an afterEach(async ()
=> { if (config && typeof config.disposeScheduler === 'function') await
config.disposeScheduler(); const { clearAllSchedulers } = await
import('./schedulerSingleton.js'); clearAllSchedulers(); }); to the test file so
Config.disposeScheduler() is always called and the
schedulerSingleton.clearAllSchedulers() fallback runs, guarding for undefined
config.
In @packages/core/src/config/config.ts:
- Around line 1896-1909: In disposeScheduler, make cleanup resilient to
exceptions from entry.scheduler.dispose(): after decrementing entry.refCount and
returning early if >0, call entry.scheduler.dispose() inside a try/catch/finally
so that this.schedulerEntries.delete(sessionId) always runs (in finally);
optionally log the caught error (e.g., via this.logger or console.error) and
rethrow only if you want callers to observe the failure — but do not allow a
thrown error to prevent deleting the schedulerEntries entry or leave refCount
stranded.
🧹 Nitpick comments (8)
packages/core/src/telemetry/loggers.ts (1)
146-157: Consider moving uiEvent construction after the SDK initialization check.The
uiEventobject is constructed before verifying SDK initialization. If the SDK is not initialized, the object is created but never used, resulting in minor wasted work. For consistency withlogApiErrorandlogApiResponse(which use an early guard before any event construction), consider moving lines 146-150 after line 156.♻️ Proposed refactor
if (process.env.VERBOSE === 'true') { console.error(`[TELEMETRY] logToolCall: ${event.function_name}`); } + if (!isTelemetrySdkInitialized()) { + if (process.env.VERBOSE === 'true') { + console.error(`[TELEMETRY] SDK not initialized, skipping log`); + } + return; + } + const uiEvent = { ...event, 'event.name': EVENT_TOOL_CALL, 'event.timestamp': new Date().toISOString(), } as UiEvent; - if (!isTelemetrySdkInitialized()) { - if (process.env.VERBOSE === 'true') { - console.error(`[TELEMETRY] SDK not initialized, skipping log`); - } - return; - } uiTelemetryService.addEvent(uiEvent);packages/core/src/tools/read-many-files.ts (1)
259-277: Per-file ignore filtering refactor looks correct.The shift from pre-filtered sets to on-the-fly
shouldIgnoreFilechecks is clean and maintains the correct precedence (git ignore takes priority over llxprt ignore). The separate counting for each ignore type is informative.Optional micro-optimization: If profiling shows
shouldIgnoreFileis expensive, consider calling it once per file with both flags enabled, then inspecting the result to determine which ignore list matched. However, given that file I/O later in the pipeline is likely the bottleneck, the current approach (two calls for clarity and separate counts) is probably fine.packages/cli/src/ui/hooks/useCommandCompletion.test.ts (1)
422-427: Clarify the purpose of conditional cleanup.The test conditionally calls
resetCompletionState()only whenshellModeActiveis true. If this is test cleanup, it should be done consistently for both test cases, or the reasoning should be documented.Is this cleanup necessary only for the shellModeActive case due to state leakage, or should both parameterized cases perform cleanup?
packages/cli/src/ui/hooks/atCommandProcessor.ts (1)
8-8: Good UX improvement with proper path resolution.The logic to display relative paths instead of absolute paths when showing file content is a clear UX improvement. The implementation properly:
- Resolves both target directory and content path via
realpathSyncwith fallback to normalization- Computes relative path and validates it's within the workspace (no
..prefix)- Gracefully falls back to the original absolute path if resolution fails
Note: The synchronous filesystem operations (
fsSync.realpathSync) are used within an async function. While acceptable for path resolution (typically fast), be aware this could briefly block the event loop. If this becomes a performance concern in large workspaces, consider async alternatives.Also applies to: 468-490, 492-492
packages/cli/src/ui/hooks/atCommandProcessor.test.ts (1)
81-105: TightenisPathWithinWorkspaceprefix check to avoid false positives
resolved.startsWith(workspaceRoot)will treat paths like/tmp/ws-2/...as inside/tmp/ws. Consider a separator-aware check.Proposed tweak
- return resolved.startsWith(workspaceRoot); + return ( + resolved === workspaceRoot || + resolved.startsWith(workspaceRoot + path.sep) + );packages/core/src/core/subagent.ts (1)
934-939: Ifscheduler.dispose()can be async, don’t silently drop rejectionsToday
schedulerDispose()is called withoutawait; if a factory ever returnsPromise<void>, errors won’t be caught by the try/catch.packages/a2a-server/src/utils/testing_utils.ts (1)
31-196: AdddisposeSchedulerto the mock Config if the code under test calls itRight now only
getOrCreateScheduleris mocked; if A2A task lifecycle now disposes schedulers, tests may start failing or leaking.packages/core/src/config/config.ts (1)
85-90: Avoid “two singleton implementations” drifting (Config-local map vsschedulerSingleton.ts)Right now Config has its own
_schedulerEntries, but the file also imports singleton helpers. I’d strongly prefer one source of truth to prevent mismatched cleanup/refCounts across packages.Also applies to: 1837-1910
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
packages/a2a-server/src/http/app.test.tspackages/a2a-server/src/utils/testing_utils.tspackages/cli/src/ui/hooks/atCommandProcessor.test.tspackages/cli/src/ui/hooks/atCommandProcessor.tspackages/cli/src/ui/hooks/slashCommandProcessor.test.tspackages/cli/src/ui/hooks/useCommandCompletion.test.tspackages/cli/src/ui/hooks/useFolderTrust.test.tspackages/cli/src/ui/hooks/useFolderTrust.tspackages/cli/src/ui/hooks/useReactToolScheduler.tspackages/cli/src/ui/hooks/useSlashCompletion.test.tspackages/cli/src/ui/hooks/useToolScheduler.test.tspackages/core/src/config/config.scheduler.test.tspackages/core/src/config/config.tspackages/core/src/config/schedulerSingleton.tspackages/core/src/core/coreToolScheduler.tspackages/core/src/core/subagent.tspackages/core/src/core/subagentScheduler.tspackages/core/src/debug/FileOutput.tspackages/core/src/telemetry/loggers.tspackages/core/src/test-utils/mock-tool.tspackages/core/src/tools/read-many-files.tspackages/ui/src/hooks/useToolScheduler.ts
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2026-01-09T02:26:33.951Z
Learning: Preference: Consolidate to a single CoreToolScheduler instance per interactive session to avoid duplicated handlers and unknown correlationId noise.
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2025-12-26T14:17:41.983Z
Learning: In packages/core/src/core/coreToolScheduler.ts, the `publishBufferedResults()` method (line 1355) is called concurrently by multiple tool completion handlers (line 1510 in each tool's `.then()` callback) when parallel tools finish simultaneously. This causes a race condition where `nextPublishIndex` can be corrupted and tools can hang in 'executing' state forever, blocking the scheduler and causing subsequent tool calls to queue indefinitely. The method must be made reentrant-safe using a mutex/lock pattern to serialize concurrent calls.
📚 Learning: 2025-11-16T22:51:26.374Z
Learnt from: e2720pjk
Repo: vybestack/llxprt-code PR: 583
File: packages/core/src/providers/openai/OpenAIProvider.ts:935-959
Timestamp: 2025-11-16T22:51:26.374Z
Learning: In the llxprt-code codebase (packages/core/src/providers/openai/OpenAIProvider.ts), tools like `run_shell_command` use internal streaming only for real-time UI updates during execution, but each tool execution produces exactly ONE final `ToolResponseBlock` containing the full result. The streaming chunks are never sent to the LLM and are not converted into multiple tool messages. The OpenAI Chat Completions API requires that each tool call (tool_call_id) corresponds to exactly one message with role 'tool', so duplicate tool response detection that removes subsequent tool messages with the same tool_call_id is correct and necessary for API compliance.
Applied to files:
packages/a2a-server/src/http/app.test.tspackages/a2a-server/src/utils/testing_utils.tspackages/core/src/core/coreToolScheduler.tspackages/cli/src/ui/hooks/useToolScheduler.test.ts
📚 Learning: 2025-12-26T14:17:41.983Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2025-12-26T14:17:41.983Z
Learning: In packages/core/src/core/coreToolScheduler.ts, the `publishBufferedResults()` method (line 1355) is called concurrently by multiple tool completion handlers (line 1510 in each tool's `.then()` callback) when parallel tools finish simultaneously. This causes a race condition where `nextPublishIndex` can be corrupted and tools can hang in 'executing' state forever, blocking the scheduler and causing subsequent tool calls to queue indefinitely. The method must be made reentrant-safe using a mutex/lock pattern to serialize concurrent calls.
Applied to files:
packages/core/src/core/subagent.tspackages/core/src/config/config.tspackages/a2a-server/src/utils/testing_utils.tspackages/core/src/config/config.scheduler.test.tspackages/core/src/core/coreToolScheduler.tspackages/cli/src/ui/hooks/useReactToolScheduler.tspackages/cli/src/ui/hooks/useToolScheduler.test.tspackages/core/src/config/schedulerSingleton.tspackages/ui/src/hooks/useToolScheduler.ts
📚 Learning: 2026-01-09T02:26:33.951Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2026-01-09T02:26:33.951Z
Learning: Preference: Consolidate to a single CoreToolScheduler instance per interactive session to avoid duplicated handlers and unknown correlationId noise.
Applied to files:
packages/core/src/core/subagent.tspackages/core/src/config/config.tspackages/core/src/config/config.scheduler.test.tspackages/core/src/core/coreToolScheduler.tspackages/cli/src/ui/hooks/useReactToolScheduler.tspackages/cli/src/ui/hooks/useToolScheduler.test.tspackages/core/src/config/schedulerSingleton.tspackages/ui/src/hooks/useToolScheduler.ts
📚 Learning: 2025-11-23T00:04:44.027Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2025-11-23T00:04:44.027Z
Learning: In TypeScript/JavaScript, readonly fields don't need to be nullified in dispose() methods for garbage collection. Once the parent object has no external references, the GC will collect the entire object including all readonly fields. Focus disposal cleanup on actual leak vectors like event listeners and active operations, not on nullifying all object references.
Applied to files:
packages/core/src/core/subagent.tspackages/ui/src/hooks/useToolScheduler.ts
📚 Learning: 2025-11-17T08:33:42.962Z
Learnt from: MinWooPark-dotcom
Repo: vybestack/llxprt-code PR: 577
File: packages/cli/src/ui/components/messages/ToolMessage.tsx:89-167
Timestamp: 2025-11-17T08:33:42.962Z
Learning: In the vybestack/llxprt-code repository, single-use helper functions in React components can be kept inline rather than extracted to separate utilities, especially when the extraction doesn't provide immediate value (no reuse, no isolated testing planned) and keeps the PR scope focused on feature delivery.
Applied to files:
packages/ui/src/hooks/useToolScheduler.ts
📚 Learning: 2025-11-25T16:56:18.980Z
Learnt from: CR
Repo: vybestack/llxprt-code PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T16:56:18.980Z
Learning: Before reporting a task as finished, run `npm run test` from the repository root and ensure it succeeds (exit code 0)
Applied to files:
packages/cli/src/ui/hooks/atCommandProcessor.test.ts
🧬 Code graph analysis (8)
packages/a2a-server/src/utils/testing_utils.ts (3)
packages/core/src/providers/openai/test-types.ts (1)
MockToolCall(32-41)packages/core/src/tools/tools.ts (1)
ToolCallConfirmationDetails(682-686)packages/core/src/config/index.ts (1)
ApprovalMode(11-11)
packages/core/src/config/config.scheduler.test.ts (1)
packages/core/src/config/schedulerSingleton.ts (1)
clearAllSchedulers(111-120)
packages/core/src/core/coreToolScheduler.ts (2)
packages/core/src/utils/editor.ts (1)
EditorType(9-17)packages/core/src/config/config.ts (1)
Config(370-1910)
packages/cli/src/ui/hooks/useReactToolScheduler.ts (2)
packages/core/src/core/coreToolScheduler.ts (1)
CoreToolScheduler(387-1932)packages/core/src/core/turn.ts (1)
ToolCallRequestInfo(101-108)
packages/cli/src/ui/hooks/slashCommandProcessor.test.ts (1)
packages/cli/src/config/settings.ts (1)
LoadedSettings(339-467)
packages/core/src/config/schedulerSingleton.ts (1)
packages/core/src/config/config.ts (4)
SchedulerCallbacks(1913-1913)schedulerEntries(1841-1855)getOrCreateScheduler(1857-1894)disposeScheduler(1896-1909)
packages/ui/src/hooks/useToolScheduler.ts (1)
packages/core/src/index.ts (1)
sessionId(184-184)
packages/cli/src/ui/hooks/atCommandProcessor.test.ts (1)
packages/cli/src/ui/hooks/atCommandProcessor.ts (1)
handleAtCommand(148-539)
🪛 GitHub Actions: LLxprt Code CI
packages/ui/src/hooks/useToolScheduler.ts
[warning] 267-267: Function 'useToolScheduler' has too many lines (187). Maximum allowed is 80. max-lines-per-function
[warning] 289-289: Arrow function has too many lines (84). Maximum allowed is 80. max-lines-per-function
[warning] 289-289: Arrow function has a complexity of 16. Maximum allowed is 15. complexity
[error] 369-369: Unnecessary optional chain on a non-nullish value. @typescript-eslint/no-unnecessary-condition
🪛 GitHub Check: Lint (Javascript)
packages/ui/src/hooks/useToolScheduler.ts
[failure] 369-369:
Unnecessary optional chain on a non-nullish value
⏰ Context from checks skipped due to timeout of 270000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: E2E Test (Linux) - sandbox:docker
- GitHub Check: E2E Test (Linux) - sandbox:none
- GitHub Check: Slow E2E - Win
- GitHub Check: E2E Test (macOS)
🔇 Additional comments (38)
packages/cli/src/ui/hooks/useFolderTrust.ts (1)
17-17: LGTM: Modern Node.js import pattern.The change from
import * as process from 'process'toimport process from 'node:process'modernizes the import to use thenode:protocol, which is the recommended pattern for Node.js built-in modules in ESM. This is functionally equivalent and aligns with current best practices.Note: This appears to be an incidental linting/style cleanup, unrelated to the PR's core scheduler consolidation objectives.
packages/core/src/telemetry/loggers.ts (1)
281-287: LGTM! Early guards prevent wasted work.Both
logApiErrorandlogApiResponsenow correctly check SDK initialization before constructing UI events or performing any logging work. This pattern efficiently prevents unnecessary object allocations and method calls when telemetry is not available.Also applies to: 322-328
packages/cli/src/ui/hooks/slashCommandProcessor.test.ts (2)
188-190: LGTM: More complete mock structure.Adding the
merged: {}property alignsmockSettingswith the actualLoadedSettingsstructure (which exposes amergedgetter per the relevant snippet at packages/cli/src/config/settings.ts:338-466). This reduces reliance on type assertions and makes the mock more robust.
117-128: These mocks are necessary and correctly structured.The
DebugLoggerandIdeClientmocks incoreMocksare essential. The implementation atslashCommandProcessor.tsdirectly uses both:
- Lines 47–48: instantiates two
DebugLoggerinstances for logging- Lines 319, 325: calls
IdeClient.getInstance()to retrieve the IDE clientThe dual
mockIdeClientpatterns are not duplication. They serve distinct access patterns:
coreMocks.IdeClient.getInstance()handles static method calls in the implementationmockConfig.getIdeClient()(lines 235–238) handles config-based access used elsewhere (e.g.,useIdeTrustListener.ts)packages/a2a-server/src/http/app.test.ts (4)
218-218: Status transition updated to 'scheduled'.The change from 'validating' to 'scheduled' aligns with the new scheduler singleton lifecycle introduced in this PR. The tool call is now explicitly scheduled before requesting approval, which is consistent with the centralized CoreToolScheduler behavior.
312-336: Consistent status and naming updates for multiple tool calls.The variable renaming from
toolCallValidateEventtotoolCallScheduledEventimproves code clarity, and the status update to 'scheduled' is consistent with the single-tool test case. Both tools correctly receive the 'scheduled' status before transitioning to 'awaiting_approval'.
540-596: Consistent event adjustments in YOLO mode test.The changes mirror those in the no-approval test: event indices adjusted and total count reduced from 10 to 9. The YOLO mode bypasses approval but maintains the same
scheduled→executing→successlifecycle.The consistency across both tests is good, but the same verification applies—confirm the event count reduction matches actual runtime behavior.
Based on learnings, consolidating to a single CoreToolScheduler instance per session eliminates duplicate event emissions. The event count reduction from 10 to 9 aligns with this consolidation, but runtime verification is recommended to ensure no events are inadvertently dropped.
423-479: Event count and indices require runtime validation.The test expects 9 events with the sequence: task creation, working (initial), working, scheduled, executing, success, working (before LLM), text-content, and final event. The event indices are logically consistent with this flow, but runtime verification is needed to confirm the actual event count matches the assertion of 9. The test environment does not have test dependencies available to execute the test and validate the expectations.
packages/core/src/debug/FileOutput.ts (3)
50-52: LGTM — Lazy timer initialization improves resource efficiency.Starting the flush timer only on the first write (rather than in the constructor) prevents unnecessary timer overhead when the logger is inactive. This works correctly with the guard in
startFlushTimer()to ensure only one timer runs at a time.
88-90: LGTM — Guard prevents multiple concurrent timers.This guard is essential for preventing duplicate timers when
startFlushTimer()is called from multiple paths (e.g., bothwrite()and the timer callback conditionally restarting). Works correctly with the lazy initialization inwrite()and the timer callback's cleanup.
94-97: LGTM — Self-scheduling timer stops when idle.The timer now conditionally restarts only when entries remain in the queue, improving efficiency by stopping when the logger is idle. The pattern is safe from race conditions:
- The timeout remains set during
await flushQueue(), preventing concurrent timer starts fromwrite().- After clearing (line 94), if both
write()and the timer callback attempt to restart, the guard at lines 88-90 ensures only one timer is created.packages/core/src/core/subagentScheduler.ts (1)
25-25: LGTM - appropriate lifecycle extension.Adding an optional
disposemethod to the factory return type aligns with the PR's scheduler lifecycle management goals and maintains backward compatibility.packages/core/src/test-utils/mock-tool.ts (1)
118-118: LGTM - cleaner assignment.Direct assignment is appropriate since
options.shouldConfirmExecutealready matches the expected signature. The refactor eliminates an unnecessary wrapper function.packages/cli/src/ui/hooks/useFolderTrust.test.ts (1)
129-131: The test expectations are correctly written. Since the mock setup at line 58 globally replacesprocess.cwd()viavi.mock('node:process'), both the implementation and the test expectations call the same mocked function, which returns'/test/path'. Usingprocess.cwd()in the expectations (rather than hardcoding'/test/path') is actually a better practice because it ensures the expectations automatically sync with the implementation and eliminates duplication if the mock value changes.Likely an incorrect or invalid review comment.
packages/cli/src/ui/hooks/atCommandProcessor.test.ts (1)
38-55: Solid test isolation via cwd management + realpathThe chdir/restore pattern should reduce path brittleness and keep expectations stable across environments.
Also applies to: 129-133
packages/core/src/core/coreToolScheduler.ts (2)
376-444:CoreToolSchedulerOptionsexport +setCallbacksis a clean API improvementThis makes the singleton wiring much easier without constructor churn.
1438-1571: publishBufferedResults reentrancy guard addresses the parallel-completion raceThe “single publisher” + “pendingPublishRequest” pattern matches the needed serialization. Based on learnings, this is the right direction for preventing
nextPublishIndexcorruption when tools finish concurrently.packages/core/src/config/config.scheduler.test.ts (1)
65-101: Good coverage of identity + refCount semanticsThe suite hits “same session => same instance”, “different sessions => different instance”, and “dispose then recreate” cases.
Also applies to: 155-170, 189-208
packages/ui/src/hooks/useToolScheduler.ts (1)
267-372: Consider queuingschedule()calls until the scheduler is readyRight now, early
schedule()calls are dropped whenschedulerRef.currentis still null (async init). If that can happen in the UI flow, add a small in-hook queue (similar to the CLI hook approach).packages/a2a-server/src/utils/testing_utils.ts (1)
12-193: Good: lightweight scheduler mock avoids real-scheduler subscriptions in testsThis should prevent the prior “real scheduler per test” leak/OOM pattern while still exercising the expected lifecycle callbacks.
packages/core/src/config/schedulerSingleton.ts (5)
14-24: LGTM: SchedulerCallbacks interface is well-defined.The interface appropriately defines all callback types needed by CoreToolScheduler, with proper optional markers and eslint-disable comments for the
anytypes.
26-34: LGTM: SchedulerEntry structure supports ref-counting lifecycle.The entry structure appropriately tracks the scheduler, reference count, and callbacks for change detection.
91-104: LGTM: Ref-counting disposal logic is correct.The function properly decrements the reference count and only disposes the scheduler when no more references remain, preventing premature cleanup of shared schedulers.
111-120: LGTM: Cleanup functions are correctly implemented.
clearAllSchedulersappropriately handles disposal errors during cleanup, andgetSchedulerInstanceprovides simple access to the cached scheduler.
64-87: The concerns in this review comment are unfounded.
CoreToolScheduler.setCallbacks()exists as a public method and is called unconditionally whenshouldRefreshCallbacksis true. The optional chaining on line 77 is used solely because the scheduler is typed asany, not because the method is optional. Additionally,setCallbacks()is a trivial method that only assigns properties—it has no side effects or performance implications, so detecting callback changes via reference equality and updating them frequently is not a concern.The suggested solutions (memoization documentation and explicit type checking) are unnecessary and misdiagnose the actual code behavior.
Likely an incorrect or invalid review comment.
packages/cli/src/ui/hooks/useReactToolScheduler.ts (5)
36-36: LGTM: useRef import added for pending request queue.The addition of
useRefsupports the pending request queue mechanism needed for async scheduler initialization.
188-195: LGTM: State initialization supports async scheduler pattern.The new state variables correctly support the async scheduler initialization pattern:
sessionIdmemoized from configschedulernullable state for async loadingpendingScheduleRequestsref to queue requests before scheduler is ready
266-321: LGTM: Sub-agent scheduler factory correctly reuses session singleton.The
createExternalSchedulercallback appropriately:
- Calls
getOrCreateSchedulerto share the session-scoped scheduler- Returns schedule and dispose functions that delegate to the shared instance
- Includes proper dependencies in the
useCallbackarrayThis achieves the PR goal of eliminating duplicate schedulers and MessageBus subscriptions.
Based on learnings: Consolidates to a single CoreToolScheduler instance per interactive session to avoid duplicated handlers and unknown correlationId noise.
347-383: LGTM: Schedule function correctly queues requests during async initialization.The function properly handles the async scheduler initialization by:
- Queuing requests in
pendingScheduleRequestswhen scheduler is null (lines 363-369)- Processing queued requests after initialization (lines 229-240)
- Silently catching cancellation errors, which is expected behavior (lines 377-380)
413-415: LGTM: Safe cancellation with optional chaining.Using optional chaining for
scheduler?.cancelAll()correctly handles the case where the scheduler hasn't been initialized yet.packages/cli/src/ui/hooks/useToolScheduler.test.ts (8)
9-9: LGTM: cleanup import added for proper test lifecycle.Adding
cleanupfrom@testing-library/reactensures proper unmounting between tests.
29-31: LGTM: PolicyDecision import supports updated mock.The import is needed for the mock policy engine at lines 82-84.
33-37: LGTM: flushAllTimers helper improves test timing control.The helper function provides a convenient way to flush multiple rounds of pending timers in tests.
52-68: LGTM: Actual core imports and MessageBus mock added.The imports of actual singleton functions and the mock MessageBus support the test infrastructure changes.
136-143: LGTM: Proper test cleanup added.The additions correctly clean up React state, timers, schedulers, and debug loggers between tests to prevent memory leaks and test pollution.
255-261: LGTM: Cleanup duplicated for second describe block.Proper cleanup ensures each test suite starts with a clean state.
156-188: Test updated to use real timers and waitFor polling.The test now uses real timers and
vi.waitForinstead of manual timer advancement. This is likely a workaround for async scheduler initialization, though it makes tests slower.Note: Once the mock scheduler is implemented (per comment on lines 85-91), consider whether fake timers can be used consistently across all tests for faster execution.
279-343: Test refactored with real timers and polling.Similar to the previous test, this uses real timers and
vi.waitForto handle async scheduler behavior. The test logic correctly validates successful tool execution.
| }, [ | ||
| config, | ||
| sessionId, | ||
| mainSchedulerId, | ||
| onComplete, | ||
| replaceToolCallsForScheduler, | ||
| updateToolCallOutput, | ||
|
|
||
| getPreferredEditor, | ||
| onEditorClose, | ||
| onEditorOpen, | ||
| ]); |
There was a problem hiding this comment.
Major: Excessive useEffect re-runs may recreate scheduler unnecessarily.
The dependency array includes multiple callback functions (onComplete, replaceToolCallsForScheduler, updateToolCallOutput, getPreferredEditor, onEditorClose, onEditorOpen). If these aren't memoized, the effect will re-run on every render, disposing and recreating the scheduler.
This conflicts with the singleton pattern goal of maintaining one scheduler per session. The singleton's setCallbacks mechanism (in schedulerSingleton.ts) was designed to update callbacks without recreating the scheduler, but this dependency array bypasses that optimization.
Potential solutions
Option 1: Remove callback dependencies
Since getOrCreateScheduler calls setCallbacks when callbacks change, you may not need all callbacks in the dependency array:
}, [
config,
sessionId,
mainSchedulerId,
- onComplete,
- replaceToolCallsForScheduler,
- updateToolCallOutput,
- getPreferredEditor,
- onEditorClose,
- onEditorOpen,
]);Then ensure these callbacks are passed fresh to getOrCreateScheduler each time they change, relying on its internal change detection.
Option 2: Memoize all callbacks
Ensure all callback dependencies are properly memoized with useCallback:
const handleComplete = useCallback(async (completedToolCalls) => {
// ... implementation
}, [/* appropriate deps */]);
const handleToolCallsUpdate = useCallback((calls) => {
// ... implementation
}, [/* appropriate deps */]);
// Use memoized callbacks in getOrCreateSchedulerOption 3: Separate callback updates from scheduler lifecycle
Use a separate effect to update callbacks:
// Effect 1: Create/dispose scheduler (runs only when sessionId changes)
useEffect(() => {
// create scheduler
return () => { /* dispose */ };
}, [config, sessionId, mainSchedulerId]);
// Effect 2: Update callbacks (runs when callbacks change)
useEffect(() => {
if (scheduler) {
scheduler.setCallbacks?.({ /* callbacks */ });
}
}, [scheduler, onComplete, replaceToolCallsForScheduler, ...]);🤖 Prompt for AI Agents
In @packages/cli/src/ui/hooks/useReactToolScheduler.ts around lines 253 - 264,
The effect that calls getOrCreateScheduler is re-running because callback props
(onComplete, replaceToolCallsForScheduler, updateToolCallOutput,
getPreferredEditor, onEditorClose, onEditorOpen) are in its dependency array;
remove those callbacks from that lifecycle effect so the scheduler is only
created/disposed by stable keys (config, sessionId, mainSchedulerId), and add a
second effect that watches the callback functions and calls
scheduler?.setCallbacks({...}) (or passes them into getOrCreateScheduler if it
updates callbacks internally) whenever they change; specifically update the
useEffect in useReactToolScheduler to only depend on
config/sessionId/mainSchedulerId for creation/disposal and introduce a separate
useEffect that depends on scheduler plus the listed callbacks to call
setCallbacks (using the scheduler variable and getOrCreateScheduler/setCallbacks
symbols).
| }); | ||
|
|
||
| expect(result.current.textBuffer.text).toBe('/memory add '); | ||
| expect(result.current.textBuffer.text).toBe('/add '); |
There was a problem hiding this comment.
Critical: Test assertion validates incorrect sub-command completion behavior.
This test expects /add after selecting the 'add' sub-command from a '/memory' parent command, but the correct behavior should be /memory add . The current assertion validates a regression where the parent command is being replaced instead of preserved.
Expected flow:
- User types:
/memory - Suggestions show:
['show', 'add'] - User selects:
'add'(index 1) - Result should be:
/memory add✓ - Actual result:
/add✗
The test name "should append a sub-command when the parent is complete" confirms the expected behavior is to append, not replace. The standalone /add would be an invalid command in the CLI context.
🐛 Proposed fix
- expect(result.current.textBuffer.text).toBe('/add ');
+ expect(result.current.textBuffer.text).toBe('/memory add ');Note: This test change likely masks an underlying bug in the handleAutocomplete or useSlashCompletion implementation. After fixing this assertion, investigate and fix the root cause in the hook's completion logic to ensure sub-commands are properly appended to their parent commands.
📝 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.
| expect(result.current.textBuffer.text).toBe('/add '); | |
| expect(result.current.textBuffer.text).toBe('/memory add '); |
🤖 Prompt for AI Agents
In @packages/cli/src/ui/hooks/useSlashCompletion.test.ts at line 1253, The test
assertion is wrong: update the expectation in useSlashCompletion.test so that
after selecting the 'add' sub-command (index 1) from the '/memory' parent the
buffer contains '/memory add ' instead of '/add '; change the assertion on
result.current.textBuffer.text to expect '/memory add ' and then run tests, and
also add a follow-up ticket to inspect handleAutocomplete / useSlashCompletion
logic to ensure sub-commands are appended (not replacing the parent command).
| beforeEach(async () => { | ||
| // Create a minimal Config instance for testing | ||
| const mockSettingsService = { | ||
| get: vi.fn(), | ||
| set: vi.fn(), | ||
| getAllGlobalSettings: vi.fn(() => ({})), | ||
| getProviderSettings: vi.fn(() => ({})), | ||
| setProviderSetting: vi.fn(), | ||
| clear: vi.fn(), | ||
| }; | ||
|
|
||
| const configParams = { | ||
| sessionId: testSessionId, | ||
| targetDir: process.cwd(), | ||
| debugMode: false, | ||
| cwd: process.cwd(), | ||
| model: 'gemini-pro', | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| settingsService: mockSettingsService as any, | ||
| eventEmitter: undefined, | ||
| }; | ||
|
|
||
| config = new Config(configParams); | ||
| await config.initialize(); | ||
|
|
||
| // Clear any existing scheduler instances from previous tests | ||
| const { clearAllSchedulers } = await import('./schedulerSingleton.js'); | ||
| clearAllSchedulers(); | ||
| }); | ||
|
|
||
| // Note: afterEach cleanup is skipped because require() doesn't work in ESM mode. | ||
| // beforeEach already clears schedulers before each test. | ||
|
|
There was a problem hiding this comment.
Add afterEach cleanup; “require doesn’t work in ESM” shouldn’t prevent disposing
Several tests create schedulers and never call config.disposeScheduler(). That’s a test leak risk (subscriptions/timeouts).
Suggested afterEach
import { describe, it, expect, beforeAll, beforeEach, vi } from 'vitest';
+import { afterEach } from 'vitest';
import { Config } from './config.js';
@@
beforeEach(async () => {
@@
const { clearAllSchedulers } = await import('./schedulerSingleton.js');
clearAllSchedulers();
});
+
+ afterEach(() => {
+ // Best-effort: dispose any schedulers created via this Config instance.
+ const entries = (
+ config as unknown as { _schedulerEntries?: Map<string, { refCount: number }> }
+ )._schedulerEntries;
+ if (!entries) return;
+ for (const [sessionId, entry] of entries.entries()) {
+ // drain refCount
+ for (let i = 0; i < Math.max(1, entry.refCount); i++) {
+ config.disposeScheduler(sessionId);
+ }
+ }
+ });📝 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.
| beforeEach(async () => { | |
| // Create a minimal Config instance for testing | |
| const mockSettingsService = { | |
| get: vi.fn(), | |
| set: vi.fn(), | |
| getAllGlobalSettings: vi.fn(() => ({})), | |
| getProviderSettings: vi.fn(() => ({})), | |
| setProviderSetting: vi.fn(), | |
| clear: vi.fn(), | |
| }; | |
| const configParams = { | |
| sessionId: testSessionId, | |
| targetDir: process.cwd(), | |
| debugMode: false, | |
| cwd: process.cwd(), | |
| model: 'gemini-pro', | |
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | |
| settingsService: mockSettingsService as any, | |
| eventEmitter: undefined, | |
| }; | |
| config = new Config(configParams); | |
| await config.initialize(); | |
| // Clear any existing scheduler instances from previous tests | |
| const { clearAllSchedulers } = await import('./schedulerSingleton.js'); | |
| clearAllSchedulers(); | |
| }); | |
| // Note: afterEach cleanup is skipped because require() doesn't work in ESM mode. | |
| // beforeEach already clears schedulers before each test. | |
| beforeEach(async () => { | |
| // Create a minimal Config instance for testing | |
| const mockSettingsService = { | |
| get: vi.fn(), | |
| set: vi.fn(), | |
| getAllGlobalSettings: vi.fn(() => ({})), | |
| getProviderSettings: vi.fn(() => ({})), | |
| setProviderSetting: vi.fn(), | |
| clear: vi.fn(), | |
| }; | |
| const configParams = { | |
| sessionId: testSessionId, | |
| targetDir: process.cwd(), | |
| debugMode: false, | |
| cwd: process.cwd(), | |
| model: 'gemini-pro', | |
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | |
| settingsService: mockSettingsService as any, | |
| eventEmitter: undefined, | |
| }; | |
| config = new Config(configParams); | |
| await config.initialize(); | |
| // Clear any existing scheduler instances from previous tests | |
| const { clearAllSchedulers } = await import('./schedulerSingleton.js'); | |
| clearAllSchedulers(); | |
| }); | |
| afterEach(() => { | |
| // Best-effort: dispose any schedulers created via this Config instance. | |
| const entries = ( | |
| config as unknown as { _schedulerEntries?: Map<string, { refCount: number }> } | |
| )._schedulerEntries; | |
| if (!entries) return; | |
| for (const [sessionId, entry] of entries.entries()) { | |
| // drain refCount | |
| for (let i = 0; i < Math.max(1, entry.refCount); i++) { | |
| config.disposeScheduler(sessionId); | |
| } | |
| } | |
| }); | |
| // Note: afterEach cleanup is skipped because require() doesn't work in ESM mode. | |
| // beforeEach already clears schedulers before each test. |
🤖 Prompt for AI Agents
In @packages/core/src/config/config.scheduler.test.ts around lines 22 - 54,
Tests leak scheduler resources because many create schedulers but don't dispose
them; add an afterEach that disposes any schedulers created by the test and
still clears singleton state: add an afterEach(async () => { if (config &&
typeof config.disposeScheduler === 'function') await config.disposeScheduler();
const { clearAllSchedulers } = await import('./schedulerSingleton.js');
clearAllSchedulers(); }); to the test file so Config.disposeScheduler() is
always called and the schedulerSingleton.clearAllSchedulers() fallback runs,
guarding for undefined config.
| disposeScheduler(sessionId: string): void { | ||
| const entry = this.schedulerEntries.get(sessionId); | ||
| if (!entry) { | ||
| return; | ||
| } | ||
|
|
||
| entry.refCount -= 1; | ||
| if (entry.refCount > 0) { | ||
| return; | ||
| } | ||
|
|
||
| entry.scheduler.dispose(); | ||
| this.schedulerEntries.delete(sessionId); | ||
| } |
There was a problem hiding this comment.
Make disposeScheduler robust if scheduler.dispose() throws
A throw will currently prevent deleting the entry (and can strand refCounts).
Proposed fix
entry.refCount -= 1;
if (entry.refCount > 0) {
return;
}
- entry.scheduler.dispose();
- this.schedulerEntries.delete(sessionId);
+ try {
+ entry.scheduler.dispose();
+ } finally {
+ this.schedulerEntries.delete(sessionId);
+ }🤖 Prompt for AI Agents
In @packages/core/src/config/config.ts around lines 1896 - 1909, In
disposeScheduler, make cleanup resilient to exceptions from
entry.scheduler.dispose(): after decrementing entry.refCount and returning early
if >0, call entry.scheduler.dispose() inside a try/catch/finally so that
this.schedulerEntries.delete(sessionId) always runs (in finally); optionally log
the caught error (e.g., via this.logger or console.error) and rethrow only if
you want callers to observe the failure — but do not allow a thrown error to
prevent deleting the schedulerEntries entry or leave refCount stranded.
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/ui/src/hooks/useToolScheduler.ts (1)
341-386: Major: async init can drop tool requests (schedule becomes a no-op until scheduler resolves).Previously the scheduler was available synchronously; now
schedule()silently does nothing untilinitializeScheduler()finishes. If the UI callsschedule()during that window, you’ll lose tool calls.A small queue (buffer requests in a ref and flush after
schedulerRef.currentis set) would preserve old behavior without changing the public API.packages/cli/src/ui/hooks/useReactToolScheduler.ts (2)
187-264: Effect dependency churn can repeatedly dispose/recreate the “singleton” scheduler.Because the effect depends on callback props (
onComplete,getPreferredEditor,onEditorClose,onEditorOpen, etc.), any non-memoized caller will retrigger it, causingdisposeScheduler/getOrCreateSchedulercycles and potentially tearing down the scheduler mid-session.Given you added
CoreToolScheduler.setCallbacks(), a safer pattern is:
- create/get scheduler once per
(config, sessionId)- keep handlers in refs, and pass stable wrapper callbacks (or call
scheduler.setCallbacks()when they change) without disposing the scheduler.
266-321: Type mismatch:createExternalScheduleris async but cast to syncExternalSchedulerFactorytype, masking thedisposeproperty.
ExternalSchedulerFactoryis defined as a sync function returning{ schedule }with nodisposeproperty. However,createExternalScheduleris async and returns{ schedule, dispose }. The cast on line 339 (as ExternalSchedulerFactory) hides this incompatibility. If any code calls the factory withoutawait, it will receive a Promise instead of the expected object, causing runtime failures on.schedule(...).The correct type is
SubagentSchedulerFactory(which includes optionaldispose), notExternalSchedulerFactory. Update the function signature and type parameter at line 324-325 to reflect the async behavior and disposal pattern, then remove the cast.packages/cli/src/ui/hooks/useToolScheduler.test.ts (1)
52-96: High OOM/leak risk: tests still instantiate real schedulers viaactualGetOrCreateScheduler.This recreates the “real scheduler in tests” problem (subscriptions/timers/maps) and can still OOM under vitest concurrency even with
clearAllSchedulers().Suggestion: replace
mockConfig.getOrCreateSchedulerwith a minimal in-memory mock scheduler (similar topackages/a2a-server/src/utils/testing_utils.ts) that only implements the surfaces used by these tests.Also:
mockMessageBus.subscribeshould return an unsubscribe function to match the real API.Proposed fix (lightweight scheduler + proper subscribe return)
const mockMessageBus = { - subscribe: vi.fn(), + subscribe: vi.fn().mockReturnValue(() => {}), unsubscribe: vi.fn(), publish: vi.fn(), }; +const makeMockScheduler = (callbacks: any) => { + return { + schedule: vi.fn(async (reqOrReqs: any, _signal: AbortSignal) => { + const reqs = Array.isArray(reqOrReqs) ? reqOrReqs : [reqOrReqs]; + callbacks?.onToolCallsUpdate?.( + reqs.map((r: any) => ({ + status: 'success', + request: r, + tool: mockToolRegistry.getTool(r.name), + invocation: mockToolRegistry.getTool(r.name)?.build?.(r.args), + response: { + callId: r.callId, + responseParts: [], + resultDisplay: '', + error: undefined, + errorType: undefined, + agentId: r.agentId ?? 'primary', + }, + })), + ); + callbacks?.onAllToolCallsComplete?.([]); + }), + cancelAll: vi.fn(), + dispose: vi.fn(), + toolCalls: [], + }; +}; const mockConfig = { @@ - getOrCreateScheduler: vi.fn((sessionId: string, callbacks: any) => - actualGetOrCreateScheduler( - mockConfig as unknown as Config, - sessionId, - callbacks, - ), - ), - disposeScheduler: vi.fn((sessionId: string) => - actualDisposeScheduler(sessionId), - ), + getOrCreateScheduler: vi.fn(async (_sessionId: string, callbacks: any) => + makeMockScheduler(callbacks), + ), + disposeScheduler: vi.fn(), setInteractiveSubagentSchedulerFactory: vi.fn(), } as unknown as Config;
🤖 Fix all issues with AI agents
In @packages/cli/src/ui/hooks/useCommandCompletion.test.ts:
- Around line 385-388: The test case's expectations contradict its description
and the implementation: when shellModeActive maps to reverseSearchActive (see
useCommandCompletion's handling around the reverseSearchActive logic),
suggestions should be hidden. Update the test entry that currently sets
expectedSuggestions: 1 and expectedShowSuggestions: true to expectedSuggestions:
0 and expectedShowSuggestions: false so the assertions match the description
"should not show slash command suggestions when shellModeActive is true" and the
component behavior.
- Around line 423-427: Add an explicit cleanup() call from
@testing-library/react in the afterEach hook of useCommandCompletion.test to
ensure renderHook auto-unmounts and isolates tests instead of relying on the
manual conditional reset; remove the conditional block that calls
result.current.resetCompletionState() when shellModeActive and rely on cleanup()
for teardown, and if tests still fail after this change, investigate and fix
resource leaks in the hook implementation referenced by resetCompletionState /
shellModeActive so the hook cleans up on unmount.
In @packages/core/src/config/config.ts:
- Around line 85-90: Remove the unused imports and duplicate scheduler map by
deleting the local _schedulerEntries-based implementation and delegating to the
singleton helpers: remove the imports' unused variants or, better, keep and use
_getOrCreateScheduler and _disposeScheduler; change Config.getOrCreateScheduler
to call _getOrCreateScheduler(...) instead of the inline implementation that
references _schedulerEntries, and change Config.disposeScheduler (or related
disposal method) to call _disposeScheduler(...) so all scheduler lifecycle is
handled by schedulerSingleton rather than duplicated in config.ts.
In @packages/core/src/config/schedulerSingleton.ts:
- Around line 35-89: getOrCreateScheduler can race and create duplicate
schedulers; make it concurrency-safe by storing an in-flight promise for the
scheduler in schedulerEntries (or a separate inFlight map) when no entry exists
so concurrent callers await the same creation promise. Specifically, when
getOrCreateScheduler sees no entry, immediately create and store an object with
a schedulerPromise (the dynamic import + instantiation of
CoreToolSchedulerClass) and refCount=1, then await that promise and replace
schedulerPromise with the real scheduler; when an entry exists and has a
schedulerPromise, increment refCount and await entry.schedulerPromise before
using entry.scheduler; ensure you still call entry.scheduler.setCallbacks (and
update entry.callbacks) after the awaited scheduler is ready, referencing
getOrCreateScheduler, schedulerEntries, CoreToolSchedulerClass,
entry.scheduler.setCallbacks, and entry.callbacks.
In @packages/core/src/core/coreToolScheduler.ts:
- Around line 505-516: The handler for TOOL_CONFIRMATION_RESPONSE currently
early-returns when originalSignal (from callIdToSignal.get(callId)) is missing,
but it fails to clean up pendingConfirmations[correlationId]; update the code in
the same handler so that when originalSignal is falsy you also delete
pendingConfirmations[correlationId] (or call the appropriate cleanup method for
the pendingConfirmations map) before returning, preserving the existing debug
log behavior using toolSchedulerLogger and ensuring correlationId is available
in scope for the deletion.
In @packages/core/src/core/subagent.ts:
- Around line 686-711: The code is calling getOrCreateScheduler and
disposeScheduler on schedulerConfig (the shim) which lacks those methods; update
the async branch to call this.config.getOrCreateScheduler(sessionId, {...})
instead of schedulerConfig.getOrCreateScheduler, and change the fallback
schedulerDispose to call
this.config.disposeScheduler(this.config.getSessionId()) (keep the existing
options?.schedulerFactory conditional that binds scheduler.dispose when present
or no-ops otherwise). Ensure you reference the existing symbols:
schedulerConfig.getSessionId(), this.config.getOrCreateScheduler(...),
schedulerDispose conditional, and this.config.disposeScheduler(...).
In @packages/ui/src/hooks/useToolScheduler.ts:
- Around line 338-372: The linter flags the unnecessary optional chaining on
config.disposeScheduler; replace the conditional call
config.disposeScheduler?.(config.getSessionId()) with a direct call to
config.disposeScheduler(config.getSessionId()) (or otherwise ensure
disposeScheduler is always defined on the config type), and keep the surrounding
teardown behavior that sets mounted = false and schedulerRef.current = null in
the return cleanup of the effect so the scheduler is disposed deterministically.
🧹 Nitpick comments (7)
packages/core/src/core/subagent.ts (1)
935-939: Consider logging disposal errors at debug level for diagnosability.While swallowing disposal errors is acceptable cleanup behavior, silently ignoring them reduces observability for long-running session debugging.
💡 Optional: log disposal errors
try { schedulerDispose(); } catch (_error) { - // ignore scheduler disposal errors + this.logger.debug( + () => `Subagent ${this.subagentId} scheduler disposal failed: ${_error instanceof Error ? _error.message : String(_error)}` + ); }packages/core/src/telemetry/loggers.ts (1)
146-158: MoveuiEventconstruction after the SDK init guard (currently still built even when skipping).Right now
uiEventis created beforeisTelemetrySdkInitialized()is checked, so we still allocate/spreadeventwhen telemetry is disabled/uninitialized.Proposed tweak
- const uiEvent = { - ...event, - 'event.name': EVENT_TOOL_CALL, - 'event.timestamp': new Date().toISOString(), - } as UiEvent; if (!isTelemetrySdkInitialized()) { if (process.env.VERBOSE === 'true') { console.error(`[TELEMETRY] SDK not initialized, skipping log`); } return; } + const uiEvent = { + ...event, + 'event.name': EVENT_TOOL_CALL, + 'event.timestamp': new Date().toISOString(), + } as UiEvent; uiTelemetryService.addEvent(uiEvent);packages/core/src/config/config.scheduler.test.ts (1)
22-50: These tests may be heavier than needed (risk of leak/OOM); consider mocking CoreToolScheduler construction.Right now the suite initializes a real
Configand imports/constructs the realCoreToolScheduler, which can register subscribers/timers. If this suite grows (or runs alongside other scheduler-heavy tests), it’s a plausible path back to Node heap pressure.If you want, I can sketch a
vi.mock('../core/coreToolScheduler.js', ...)that returns a minimal fake scheduler (setCallbacks,dispose,schedule) while still validating singleton/refCount semantics.packages/a2a-server/src/http/app.test.ts (1)
208-337: Looks consistent with the new “scheduled” state; consider reducing reliance on exact event indexes if this flakes.packages/ui/src/hooks/useToolScheduler.ts (1)
267-373: Follow-up: linter “max-lines-per-function” warnings—worth splitting effect/init into helpers if you want CI clean.packages/a2a-server/src/utils/testing_utils.ts (1)
57-191: Mock scheduler API doesn’t match CoreToolScheduler (risk of tests diverging from production).Key mismatches:
schedulesignature isschedule(requests: unknown[])but real usage isschedule(request | request[], signal).confirmationResultsis treated asboolean[]but you later cast entries toToolCallConfirmationDetails.toolCalls: []is never updated, even though you emit updates via callbacks.If this file is used broadly as a shared testing util, it’s worth keeping it structurally compatible with the real scheduler.
Proposed fix (make mock schedule signature + confirmation typing match)
- schedule: vi.fn().mockImplementation(async (requests: unknown[]) => { - const scheduledCalls = requests.map((request, index) => { + schedule: vi + .fn() + .mockImplementation( + async ( + requestOrRequests: unknown, + _signal?: AbortSignal, + ) => { + const requests = Array.isArray(requestOrRequests) + ? requestOrRequests + : [requestOrRequests]; + const scheduledCalls = requests.map((request, index) => { const callId = (request as { callId?: string }).callId ?? `call-${index}`; const name = (request as { name?: string }).name ?? 'mock-tool'; const args = (request as { args?: Record<string, unknown> }).args ?? {}; return makeCall(callId, name, 'scheduled', args); - }); - callbacks?.onToolCallsUpdate?.(scheduledCalls); + }); + + // Keep the mock’s observable state consistent with emitted updates. + // (If you need it mutable, switch this to a closure var.) + callbacks?.onToolCallsUpdate?.(scheduledCalls); const approvalMode = mockConfig.getApprovalMode(); const shouldBypassApproval = approvalMode === ApprovalMode.YOLO; - const confirmationResults = await Promise.all( + const confirmationDetailsByCall = await Promise.all( scheduledCalls.map(async (call) => { const tool = mockConfig .getToolRegistry?.() ?.getTool?.(call.request.name); if (!tool || typeof tool.build !== 'function') { - return false; + return null; } const invocation = tool.build(call.request.args); if ( !invocation || typeof invocation.shouldConfirmExecute !== 'function' ) { - return false; + return null; } - return invocation.shouldConfirmExecute( - new AbortController().signal, - ); + return (await invocation.shouldConfirmExecute( + new AbortController().signal, + )) as ToolCallConfirmationDetails | null; }), ); const requiresApproval = !shouldBypassApproval && - confirmationResults.some((result) => result); + confirmationDetailsByCall.some(Boolean); if (requiresApproval) { const awaitingCalls = scheduledCalls.map((call, index) => makeCall( call.request.callId, call.request.name, 'awaiting_approval', call.request.args, - (confirmationResults[index] as ToolCallConfirmationDetails) || - confirmationDetails, + confirmationDetailsByCall[index] ?? confirmationDetails, ), ); callbacks?.onToolCallsUpdate?.(awaitingCalls); return; } const executingCalls = scheduledCalls.map((call) => makeCall( call.request.callId, call.request.name, 'executing', call.request.args, ), ); callbacks?.onToolCallsUpdate?.(executingCalls); const successCalls = scheduledCalls.map((call) => ({ ...makeCall( call.request.callId, call.request.name, 'success', call.request.args, ), response: makeResponse(call.request.callId, call.request.name), })); callbacks?.onToolCallsUpdate?.(successCalls); callbacks?.onAllToolCallsComplete?.(successCalls as never[]); - }), + }, + ),packages/cli/src/ui/hooks/useReactToolScheduler.ts (1)
363-380:ScheduleFnreturnsPromisebut is typed asvoid.Either:
- change
ScheduleFnto(...): Promise<void>(and keep returning), or- don’t return anything (fire-and-forget) to preserve the current public hook contract.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
packages/a2a-server/src/http/app.test.tspackages/a2a-server/src/utils/testing_utils.tspackages/cli/src/ui/hooks/atCommandProcessor.test.tspackages/cli/src/ui/hooks/atCommandProcessor.tspackages/cli/src/ui/hooks/slashCommandProcessor.test.tspackages/cli/src/ui/hooks/useCommandCompletion.test.tspackages/cli/src/ui/hooks/useFolderTrust.test.tspackages/cli/src/ui/hooks/useFolderTrust.tspackages/cli/src/ui/hooks/useReactToolScheduler.tspackages/cli/src/ui/hooks/useSlashCompletion.test.tspackages/cli/src/ui/hooks/useToolScheduler.test.tspackages/core/src/config/config.scheduler.test.tspackages/core/src/config/config.tspackages/core/src/config/schedulerSingleton.tspackages/core/src/core/coreToolScheduler.tspackages/core/src/core/subagent.tspackages/core/src/core/subagentScheduler.tspackages/core/src/debug/FileOutput.tspackages/core/src/telemetry/loggers.tspackages/core/src/test-utils/mock-tool.tspackages/core/src/tools/read-many-files.tspackages/ui/src/hooks/useToolScheduler.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/cli/src/ui/hooks/useSlashCompletion.test.ts
- packages/core/src/test-utils/mock-tool.ts
- packages/cli/src/ui/hooks/atCommandProcessor.ts
- packages/cli/src/ui/hooks/useFolderTrust.test.ts
- packages/cli/src/ui/hooks/useFolderTrust.ts
- packages/cli/src/ui/hooks/slashCommandProcessor.test.ts
- packages/core/src/tools/read-many-files.ts
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2026-01-09T02:26:33.951Z
Learning: Preference: Consolidate to a single CoreToolScheduler instance per interactive session to avoid duplicated handlers and unknown correlationId noise.
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2025-12-26T14:17:41.983Z
Learning: In packages/core/src/core/coreToolScheduler.ts, the `publishBufferedResults()` method (line 1355) is called concurrently by multiple tool completion handlers (line 1510 in each tool's `.then()` callback) when parallel tools finish simultaneously. This causes a race condition where `nextPublishIndex` can be corrupted and tools can hang in 'executing' state forever, blocking the scheduler and causing subsequent tool calls to queue indefinitely. The method must be made reentrant-safe using a mutex/lock pattern to serialize concurrent calls.
📚 Learning: 2025-11-16T22:51:26.374Z
Learnt from: e2720pjk
Repo: vybestack/llxprt-code PR: 583
File: packages/core/src/providers/openai/OpenAIProvider.ts:935-959
Timestamp: 2025-11-16T22:51:26.374Z
Learning: In the llxprt-code codebase (packages/core/src/providers/openai/OpenAIProvider.ts), tools like `run_shell_command` use internal streaming only for real-time UI updates during execution, but each tool execution produces exactly ONE final `ToolResponseBlock` containing the full result. The streaming chunks are never sent to the LLM and are not converted into multiple tool messages. The OpenAI Chat Completions API requires that each tool call (tool_call_id) corresponds to exactly one message with role 'tool', so duplicate tool response detection that removes subsequent tool messages with the same tool_call_id is correct and necessary for API compliance.
Applied to files:
packages/a2a-server/src/http/app.test.tspackages/core/src/core/coreToolScheduler.tspackages/a2a-server/src/utils/testing_utils.tspackages/cli/src/ui/hooks/useToolScheduler.test.ts
📚 Learning: 2025-12-26T14:17:41.983Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2025-12-26T14:17:41.983Z
Learning: In packages/core/src/core/coreToolScheduler.ts, the `publishBufferedResults()` method (line 1355) is called concurrently by multiple tool completion handlers (line 1510 in each tool's `.then()` callback) when parallel tools finish simultaneously. This causes a race condition where `nextPublishIndex` can be corrupted and tools can hang in 'executing' state forever, blocking the scheduler and causing subsequent tool calls to queue indefinitely. The method must be made reentrant-safe using a mutex/lock pattern to serialize concurrent calls.
Applied to files:
packages/core/src/core/subagent.tspackages/core/src/core/coreToolScheduler.tspackages/core/src/config/config.scheduler.test.tspackages/core/src/config/schedulerSingleton.tspackages/cli/src/ui/hooks/useReactToolScheduler.tspackages/cli/src/ui/hooks/useToolScheduler.test.tspackages/core/src/config/config.tspackages/ui/src/hooks/useToolScheduler.ts
📚 Learning: 2026-01-09T02:26:33.951Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2026-01-09T02:26:33.951Z
Learning: Preference: Consolidate to a single CoreToolScheduler instance per interactive session to avoid duplicated handlers and unknown correlationId noise.
Applied to files:
packages/core/src/core/subagent.tspackages/core/src/core/coreToolScheduler.tspackages/core/src/config/config.scheduler.test.tspackages/core/src/config/schedulerSingleton.tspackages/cli/src/ui/hooks/useReactToolScheduler.tspackages/cli/src/ui/hooks/useToolScheduler.test.tspackages/core/src/config/config.tspackages/ui/src/hooks/useToolScheduler.ts
📚 Learning: 2025-11-23T00:04:44.027Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2025-11-23T00:04:44.027Z
Learning: In TypeScript/JavaScript, readonly fields don't need to be nullified in dispose() methods for garbage collection. Once the parent object has no external references, the GC will collect the entire object including all readonly fields. Focus disposal cleanup on actual leak vectors like event listeners and active operations, not on nullifying all object references.
Applied to files:
packages/core/src/core/subagent.ts
📚 Learning: 2025-11-25T16:56:18.980Z
Learnt from: CR
Repo: vybestack/llxprt-code PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T16:56:18.980Z
Learning: Before reporting a task as finished, run `npm run test` from the repository root and ensure it succeeds (exit code 0)
Applied to files:
packages/cli/src/ui/hooks/atCommandProcessor.test.ts
📚 Learning: 2025-11-17T08:33:42.962Z
Learnt from: MinWooPark-dotcom
Repo: vybestack/llxprt-code PR: 577
File: packages/cli/src/ui/components/messages/ToolMessage.tsx:89-167
Timestamp: 2025-11-17T08:33:42.962Z
Learning: In the vybestack/llxprt-code repository, single-use helper functions in React components can be kept inline rather than extracted to separate utilities, especially when the extraction doesn't provide immediate value (no reuse, no isolated testing planned) and keeps the PR scope focused on feature delivery.
Applied to files:
packages/ui/src/hooks/useToolScheduler.ts
🧬 Code graph analysis (5)
packages/core/src/config/config.scheduler.test.ts (1)
packages/core/src/config/schedulerSingleton.ts (1)
clearAllSchedulers(111-120)
packages/core/src/telemetry/loggers.ts (3)
packages/core/src/telemetry/uiTelemetry.ts (1)
uiTelemetryService(283-283)packages/core/src/telemetry/sdk.ts (1)
isTelemetrySdkInitialized(40-42)packages/core/src/telemetry/index.ts (1)
isTelemetrySdkInitialized(19-19)
packages/core/src/config/schedulerSingleton.ts (1)
packages/core/src/config/config.ts (4)
SchedulerCallbacks(1913-1913)schedulerEntries(1841-1855)getOrCreateScheduler(1857-1894)disposeScheduler(1896-1909)
packages/cli/src/ui/hooks/useReactToolScheduler.ts (2)
packages/core/src/core/coreToolScheduler.ts (1)
CoreToolScheduler(387-1932)packages/core/src/core/turn.ts (1)
ToolCallRequestInfo(101-108)
packages/cli/src/ui/hooks/useToolScheduler.test.ts (3)
packages/core/src/index.ts (3)
PolicyDecision(16-16)sessionId(184-184)DebugLogger(339-339)packages/core/src/config/config.ts (1)
Config(370-1910)packages/core/src/core/coreToolScheduler.ts (1)
CompletedToolCall(148-151)
🪛 GitHub Actions: LLxprt Code CI
packages/ui/src/hooks/useToolScheduler.ts
[warning] 267-267: Function 'useToolScheduler' has too many lines (187). Maximum allowed is 80 max-lines-per-function
[warning] 289-289: Arrow function has too many lines (84). Maximum allowed is 80 max-lines-per-function
[error] 369-369: Unnecessary optional chain on a non-nullish value @typescript-eslint/no-unnecessary-condition
🪛 GitHub Check: Lint (Javascript)
packages/ui/src/hooks/useToolScheduler.ts
[failure] 369-369:
Unnecessary optional chain on a non-nullish value
⏰ Context from checks skipped due to timeout of 270000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: E2E Test (Linux) - sandbox:docker
- GitHub Check: Slow E2E - Win
- GitHub Check: E2E Test (macOS)
- GitHub Check: E2E Test (Linux) - sandbox:none
🔇 Additional comments (21)
packages/core/src/debug/FileOutput.ts (3)
50-52: LGTM! Lazy timer initialization prevents unnecessary timers.Starting the flush timer on-demand (when the first entry is written) rather than eagerly in the constructor ensures the timer exists only when needed, reducing resource consumption and potential leaks.
88-90: LGTM! Guard prevents re-entrancy.The early return when a timer already exists ensures only one flush timer is active at a time, preventing timer accumulation and race conditions.
94-97: LGTM! Clean self-scheduling pattern with proper lifecycle management.Clearing the timer reference before conditionally restarting ensures the timer stops when the queue is empty and resumes only when needed. This pattern prevents timer leaks and aligns well with the PR's focus on proper resource lifecycle management.
packages/core/src/core/subagent.ts (1)
44-45: LGTM!The imports are correctly added and used -
SubagentSchedulerFactoryis properly imported as a type for the options parameter, andToolErrorTypeis used in the error handling logic.packages/cli/src/ui/hooks/atCommandProcessor.test.ts (5)
23-23: LGTM: Using real filesystem path resolution improves test reliability.The addition of
fs.realpathSyncensures test files are referenced by their canonical paths, which prevents issues with symlinks and path normalization. This aligns with the broader PR goal of using more realistic test environments.Also applies to: 41-41
63-71: LGTM: MessageBus mock is appropriately lightweight for this test context.The mock provides all required MessageBus methods without creating real subscriptions or schedulers, avoiding the memory leak issue mentioned in the PR comments (which affected a different test file). The auto-resolved
requestConfirmationis reasonable for these tests sincehandleAtCommandis the focus, not confirmation flow logic.
200-203: LGTM: Test expectations consistently updated for relative path handling.All test cases have been systematically updated to expect relative paths in query strings and file reference outputs, which aligns perfectly with the new working-directory-based test setup. The changes are internally consistent across:
- Single file references
- Multiple file references
- Git-ignored and llxprt-ignored files
- Punctuation-terminated paths
- Files with special characters
- Edge cases
This improves test maintainability and portability.
Also applies to: 237-240, 269-272, 295-300, 334-341, 368-376, 404-418, 465-486, 511-518, 543-557, 566-586, 626-646, 670-678, 708-723, 821-844, 848-875, 878-905, 908-931, 934-957, 960-983, 986-1012, 1015-1038, 1041-1067, 1070-1096, 1103-1107
44-45: Working directory manipulation is appropriately isolated for this test file.Changing
process.cwd()globally is typically risky, but here it's mitigated by:
- Vitest's
singleThread: trueconfiguration in the project, which ensures tests run sequentially, not in parallel- The
afterEachhook that reliably restores the original working directory, even if tests fail- Temporary directory creation via
mkdtemplimits scope of side effectsThis pattern is sound for isolated test suites.
81-105: The mock implementation is correct and appropriately handles test scenarios. ThetestRootDiris a hardcoded test constant ("/test/root"), not a real filesystem location, sofs.realpathSyncwill necessarily fail and trigger the fallback logic—which is intentional and necessary for the mock to function properly. The try-catch block explicitly handles non-existent paths by constructing the expected resolved path, not silently suppressing errors.The path resolution logic correctly handles relative paths, absolute paths, and paths outside the workspace. For test purposes with controlled, non-existent paths, this implementation is appropriate and requires no changes.
packages/core/src/telemetry/loggers.ts (2)
280-288: Good:logApiErrornow bails out before creating/sending UI telemetry when SDK isn’t ready.
321-329: Good:logApiResponsenow bails out before creating/sending UI telemetry when SDK isn’t ready.packages/core/src/config/config.ts (1)
1912-1913: Good: re-exportingSchedulerCallbacksmakes it easier for consumers to type callback wiring.packages/core/src/config/schedulerSingleton.ts (1)
91-120:disposeScheduler/clearAllSchedulersbehavior looks sane—nice to see best-effort cleanup.packages/core/src/core/subagentScheduler.ts (1)
15-26: Disposal hook is properly integrated—scheduler cleanup occurs in finally block.The
dispose?: () => voidaddition is backward-compatible and correctly invoked in therunInteractivemethod's finally block (line 936 of subagent.ts). The implementation properly binds the factory-provided dispose method when available, otherwise falls back to config-based disposal, and errors during disposal are safely caught.packages/core/src/config/config.scheduler.test.ts (1)
103-120: Callbacks assertion is correctly aligned with Config's implementation.The test properly verifies that
config._schedulerEntriesdoesn't retain callbacks.Config.getOrCreateScheduler()(line 1857 inconfig.ts) uses its instance-level_schedulerEntriescache to store scheduler entries. The assertion is meaningful because it ensures Config's internal cache doesn't hold callback references that could prevent garbage collection. The separate module-levelschedulerSingletonuses a different, independent cache; this test correctly checks the cache corresponding to the instance method being exercised.packages/a2a-server/src/utils/testing_utils.ts (1)
12-15: Keep test-utility imports consistent with the public core API surface.This import from
@vybestack/llxprt-code-coreis good (avoids reaching into internal paths), and matches the PR direction.packages/core/src/core/coreToolScheduler.ts (3)
376-444: Callback wiring viasetCallbacks()matches the singleton direction.Exporting
CoreToolSchedulerOptionsand centralizing callback assignment makes it much easier/safer to reuse a scheduler and “retarget” handlers per session.
1557-1569: Good guard against uselesssetImmediatechurn.Only rescheduling when there’s a buffered result for
nextPublishIndexavoids the “spin” behavior when results are sparse/out-of-order.
1788-1792:isFinalizingToolCallsreset infinallyis the right hardening.This prevents the scheduler from being stuck “running” if
onAllToolCallsCompletethrows/rejects.packages/cli/src/ui/hooks/useReactToolScheduler.ts (1)
413-415: Safe optional chaining oncancelAll()is a good defensive tweak.packages/cli/src/ui/hooks/useToolScheduler.test.ts (1)
135-143: Teardown improvements are good, but they won’t save you if real schedulers are created.
cleanup(),clearAllSchedulers(), andDebugLogger.disposeAll()are all directionally right; the key is preventing heavy scheduler instantiation in the first place (see above).Also applies to: 254-261
| expectedSuggestions: 1, | ||
| expectedShowSuggestions: true, | ||
| description: | ||
| 'should not show slash command suggestions when shellModeActive is true', |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check if the file exists and read the relevant section
if [ -f "packages/cli/src/ui/hooks/useCommandCompletion.test.ts" ]; then
echo "=== Lines 375-435 of useCommandCompletion.test.ts ==="
sed -n '375,435p' packages/cli/src/ui/hooks/useCommandCompletion.test.ts
else
echo "File not found at packages/cli/src/ui/hooks/useCommandCompletion.test.ts"
# Try to find it
find . -name "useCommandCompletion.test.ts" 2>/dev/null
fiRepository: vybestack/llxprt-code
Length of output: 1910
🏁 Script executed:
# Find and examine the useCommandCompletion hook implementation
find . -name "useCommandCompletion.ts" ! -path "*/node_modules/*" ! -path "*/.git/*" 2>/dev/null | head -5Repository: vybestack/llxprt-code
Length of output: 47
🏁 Script executed:
# Try a broader search
fd -e ts -e tsx "useCommandCompletion" ! -path "*/node_modules/*" 2>/dev/null | head -20Repository: vybestack/llxprt-code
Length of output: 47
🏁 Script executed:
# Let's search from the packages/cli/src directory
ls -la packages/cli/src/ui/hooks/ 2>/dev/null || ls -la packages/cli/src/ 2>/dev/null || find packages -type d -name hooks 2>/dev/nullRepository: vybestack/llxprt-code
Length of output: 7665
🏁 Script executed:
# Read the useCommandCompletion.tsx hook implementation
cat -n packages/cli/src/ui/hooks/useCommandCompletion.tsxRepository: vybestack/llxprt-code
Length of output: 10854
🏁 Script executed:
# Check the useSlashCompletion hook for how it uses reverseSearchActive
sed -n '1,100p' packages/cli/src/ui/hooks/useSlashCompletion.tsxRepository: vybestack/llxprt-code
Length of output: 3494
🏁 Script executed:
# Continue reading useSlashCompletion to see how reverseSearchActive is used
sed -n '100,200p' packages/cli/src/ui/hooks/useSlashCompletion.tsxRepository: vybestack/llxprt-code
Length of output: 3068
🏁 Script executed:
# Let me check if there are other related tests or comments in the test file to understand the intent
sed -n '370,450p' packages/cli/src/ui/hooks/useCommandCompletion.test.tsRepository: vybestack/llxprt-code
Length of output: 2366
🏁 Script executed:
# Check for recent changes or comments in the test file around this area
git log --oneline -n 20 packages/cli/src/ui/hooks/useCommandCompletion.test.ts 2>/dev/null | head -10Repository: vybestack/llxprt-code
Length of output: 118
🏁 Script executed:
# Check the commit details
git show ff7d0c43 --stat | head -30Repository: vybestack/llxprt-code
Length of output: 1728
🏁 Script executed:
# Show the diff for the useCommandCompletion.test.ts file in that commit
git show ff7d0c43 -- packages/cli/src/ui/hooks/useCommandCompletion.test.ts | grep -A 30 "shellModeActive" | head -60Repository: vybestack/llxprt-code
Length of output: 1932
🏁 Script executed:
# Check what the test looked like before this commit
git show ff7d0c43^:packages/cli/src/ui/hooks/useCommandCompletion.test.ts 2>/dev/null | sed -n '370,450p'Repository: vybestack/llxprt-code
Length of output: 47
🏁 Script executed:
# Try to see the full diff for this section
git show ff7d0c43 -- packages/cli/src/ui/hooks/useCommandCompletion.test.ts | grep -B 20 "shellModeActive: true" | head -50Repository: vybestack/llxprt-code
Length of output: 622
🏁 Script executed:
# Check if this is a newly added test by looking at the entire diff for this area
git show ff7d0c43 -- packages/cli/src/ui/hooks/useCommandCompletion.test.ts 2>/dev/null | grep -A 5 -B 5 "should not show slash command" | head -30Repository: vybestack/llxprt-code
Length of output: 391
Critical: Test expectations contradict description and implementation behavior.
The test description "should not show slash command suggestions when shellModeActive is true" is correct based on the implementation (which treats shellModeActive as reverseSearchActive on line 175 of useCommandCompletion.tsx, causing completion state to reset).
However, the expectations are incorrect:
expectedSuggestions: 1andexpectedShowSuggestions: trueverify that suggestions ARE shown- These should be
expectedSuggestions: 0andexpectedShowSuggestions: falseto align with the intended behavior
The test will currently pass even though it's checking the opposite of what the description claims.
🤖 Prompt for AI Agents
In @packages/cli/src/ui/hooks/useCommandCompletion.test.ts around lines 385 -
388, The test case's expectations contradict its description and the
implementation: when shellModeActive maps to reverseSearchActive (see
useCommandCompletion's handling around the reverseSearchActive logic),
suggestions should be hidden. Update the test entry that currently sets
expectedSuggestions: 1 and expectedShowSuggestions: true to expectedSuggestions:
0 and expectedShowSuggestions: false so the assertions match the description
"should not show slash command suggestions when shellModeActive is true" and the
component behavior.
| if (shellModeActive) { | ||
| act(() => { | ||
| result.current.resetCompletionState(); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the test file around lines 423-427 and understand the context
head -450 packages/cli/src/ui/hooks/useCommandCompletion.test.ts | tail -50Repository: vybestack/llxprt-code
Length of output: 1558
🏁 Script executed:
# Let's also check for afterEach hooks and general test structure
grep -n "afterEach\|beforeEach\|describe\|it(" packages/cli/src/ui/hooks/useCommandCompletion.test.ts | head -40Repository: vybestack/llxprt-code
Length of output: 2079
🏁 Script executed:
# Check if there's a cleanup pattern documented or if this is part of a larger pattern
rg "resetCompletionState" packages/cli/src/ui/hooks/useCommandCompletion.test.ts -B 3 -A 3Repository: vybestack/llxprt-code
Length of output: 1491
🏁 Script executed:
# Look for the shellModeActive variable and understand what it controls in this test
rg "shellModeActive" packages/cli/src/ui/hooks/useCommandCompletion.test.ts -B 5 -A 5 | head -60Repository: vybestack/llxprt-code
Length of output: 1403
🏁 Script executed:
# Check what the afterEach hook does
sed -n '229,232p' packages/cli/src/ui/hooks/useCommandCompletion.test.tsRepository: vybestack/llxprt-code
Length of output: 117
🏁 Script executed:
# Check the full context of the test to understand if this is an isolated test or part of a describe block
sed -n '203,430p' packages/cli/src/ui/hooks/useCommandCompletion.test.ts | grep -n "describe\|it\|it.each\|afterEach" | tail -30Repository: vybestack/llxprt-code
Length of output: 1101
🏁 Script executed:
# Let's see if there are any similar patterns in other test files within the hooks directory
find packages/cli/src/ui/hooks -name "*.test.ts" -exec grep -l "if.*shellModeActive" {} \;Repository: vybestack/llxprt-code
Length of output: 119
🏁 Script executed:
# Check if renderHook from vitest has any special considerations
# Let's look at imports to see what test utilities are being used
head -20 packages/cli/src/ui/hooks/useCommandCompletion.test.tsRepository: vybestack/llxprt-code
Length of output: 863
🏁 Script executed:
# Check if renderHook is being unmounted or cleanup explicitly in the test
sed -n '173,430p' packages/cli/src/ui/hooks/useCommandCompletion.test.tsRepository: vybestack/llxprt-code
Length of output: 7210
🏁 Script executed:
# Look for the full test to see if there's any explicit cleanup or unmount
sed -n '170,430p' packages/cli/src/ui/hooks/useCommandCompletion.test.ts | cat -nRepository: vybestack/llxprt-code
Length of output: 9131
🏁 Script executed:
# Check if renderHook is being explicitly unmounted in any tests
rg "renderHook" packages/cli/src/ui/hooks/useCommandCompletion.test.ts | grep -E "unmount|cleanup" | head -10Repository: vybestack/llxprt-code
Length of output: 47
🏁 Script executed:
# Let's check the actual implementation of useCommandCompletion to understand what shellModeActive does
find packages/cli/src/ui/hooks -name "useCommandCompletion.ts" -o -name "useCommandCompletion.js"Repository: vybestack/llxprt-code
Length of output: 47
🏁 Script executed:
# Let's read the actual hook implementation to understand the shellModeActive parameter
head -100 packages/cli/src/ui/hooks/useCommandCompletion.tsRepository: vybestack/llxprt-code
Length of output: 173
🏁 Script executed:
# Check if there are any effects or cleanup functions that might be specific to shellModeActive
rg "shellModeActive" packages/cli/src/ui/hooks/useCommandCompletion.ts -B 3 -A 3Repository: vybestack/llxprt-code
Length of output: 154
🏁 Script executed:
# Find the useCommandCompletion hook file
find packages/cli/src -name "*useCommandCompletion*"Repository: vybestack/llxprt-code
Length of output: 170
🏁 Script executed:
# List the actual files in the hooks directory
ls -la packages/cli/src/ui/hooks/Repository: vybestack/llxprt-code
Length of output: 7665
🏁 Script executed:
# Read the useCommandCompletion.tsx implementation
cat packages/cli/src/ui/hooks/useCommandCompletion.tsxRepository: vybestack/llxprt-code
Length of output: 8768
🏁 Script executed:
# Check if there's any difference in behavior between the two shellModeActive cases
# Look at the test descriptions and expectations more carefully
sed -n '206,260p' packages/cli/src/ui/hooks/useCommandCompletion.test.tsRepository: vybestack/llxprt-code
Length of output: 1681
🏁 Script executed:
# Check if renderHook requires explicit cleanup/unmount in vitest
# Look for any documentation patterns in other test files
rg "renderHook" packages/cli/src/ui/hooks --glob="*.test.ts" -A 5 | head -80Repository: vybestack/llxprt-code
Length of output: 6297
🏁 Script executed:
# Look for any comment or documentation about why manual reset is needed when shellModeActive
sed -n '200,265p' packages/cli/src/ui/hooks/useCommandCompletion.test.tsRepository: vybestack/llxprt-code
Length of output: 2009
🏁 Script executed:
# Check how useToolScheduler.test.ts uses cleanup (since it imports it)
rg "cleanup" packages/cli/src/ui/hooks/useToolScheduler.test.ts -B 2 -A 2 | head -30Repository: vybestack/llxprt-code
Length of output: 477
🏁 Script executed:
# Check if there are any memory leak issues or warnings in the test output
# Look for comments about memory leaks or state issues in this test file
rg "memory\|leak\|cleanup\|dispose\|unmount" packages/cli/src/ui/hooks/useCommandCompletion.test.ts -iRepository: vybestack/llxprt-code
Length of output: 47
🏁 Script executed:
# Let's check what useSlashCompletion returns and if it has state that might need cleaning
grep -n "return {" packages/cli/src/ui/hooks/useSlashCompletion.tsx | tail -5Repository: vybestack/llxprt-code
Length of output: 146
🏁 Script executed:
# Check if useSlashCompletion has any useEffect or subscriptions that might leak
rg "useEffect|useRef|setTimeout|setInterval|subscribe|addEventListener" packages/cli/src/ui/hooks/useSlashCompletion.tsx | head -20Repository: vybestack/llxprt-code
Length of output: 532
🏁 Script executed:
# Check the return statement of useSlashCompletion to see what it returns
sed -n '875,920p' packages/cli/src/ui/hooks/useSlashCompletion.tsxRepository: vybestack/llxprt-code
Length of output: 370
🏁 Script executed:
# Let's check if the test file has any comments or TODOs about cleanup
head -30 packages/cli/src/ui/hooks/useCommandCompletion.test.tsRepository: vybestack/llxprt-code
Length of output: 1110
🏁 Script executed:
# Compare how useToolScheduler.test.ts handles cleanup vs useCommandCompletion.test.ts
rg "afterEach|cleanup" packages/cli/src/ui/hooks/useToolScheduler.test.tsRepository: vybestack/llxprt-code
Length of output: 281
Add explicit cleanup() to afterEach hook to properly isolate tests.
The conditional manual call to resetCompletionState() when shellModeActive is true indicates a test isolation issue. Similar test files like useToolScheduler.test.ts explicitly call cleanup() from @testing-library/react in their afterEach hooks, but this test file does not. This missing cleanup allows resources and state from the shellModeActive code path to leak between tests.
The manual reset is a workaround that masks the underlying problem. Proper test isolation requires:
import { renderHook, act, waitFor } from '@testing-library/react';
+ import { cleanup } from '@testing-library/react';
afterEach(() => {
+ cleanup();
vi.restoreAllMocks();
});Then remove the conditional reset block (lines 254-258) and ensure the hook properly cleans up via renderHook's auto-unmounting. If the test still fails without the manual reset, that indicates a resource leak in the hook implementation that needs investigation.
🤖 Prompt for AI Agents
In @packages/cli/src/ui/hooks/useCommandCompletion.test.ts around lines 423 -
427, Add an explicit cleanup() call from @testing-library/react in the afterEach
hook of useCommandCompletion.test to ensure renderHook auto-unmounts and
isolates tests instead of relying on the manual conditional reset; remove the
conditional block that calls result.current.resetCompletionState() when
shellModeActive and rely on cleanup() for teardown, and if tests still fail
after this change, investigate and fix resource leaks in the hook implementation
referenced by resetCompletionState / shellModeActive so the hook cleans up on
unmount.
packages/core/src/core/subagent.ts
Outdated
| const schedulerPromise = options?.schedulerFactory | ||
| ? Promise.resolve( | ||
| options.schedulerFactory({ | ||
| schedulerConfig, | ||
| onAllToolCallsComplete: handleCompletion, | ||
| outputUpdateHandler, | ||
| onToolCallsUpdate: undefined, | ||
| }), | ||
| ) | ||
| : (async () => { | ||
| const sessionId = schedulerConfig.getSessionId(); | ||
| return await schedulerConfig.getOrCreateScheduler(sessionId, { | ||
| outputUpdateHandler, | ||
| onAllToolCallsComplete: handleCompletion, | ||
| onToolCallsUpdate: undefined, | ||
| getPreferredEditor: () => undefined, | ||
| onEditorClose: () => {}, | ||
| }); | ||
| })(); | ||
|
|
||
| const scheduler = await schedulerPromise; | ||
| const schedulerDispose = options?.schedulerFactory | ||
| ? typeof scheduler.dispose === 'function' | ||
| ? scheduler.dispose.bind(scheduler) | ||
| : () => {} | ||
| : () => schedulerConfig.disposeScheduler(schedulerConfig.getSessionId()); |
There was a problem hiding this comment.
Critical: schedulerConfig does not have getOrCreateScheduler or disposeScheduler methods.
The createSchedulerConfig() method (lines 1405-1463) creates a shim that does not include getOrCreateScheduler or disposeScheduler. These methods exist on this.config (the foreground config), not on the shim. This will cause a TypeError at runtime when no factory is provided.
🐛 Proposed fix: use `this.config` for scheduler lifecycle operations
const schedulerPromise = options?.schedulerFactory
? Promise.resolve(
options.schedulerFactory({
schedulerConfig,
onAllToolCallsComplete: handleCompletion,
outputUpdateHandler,
onToolCallsUpdate: undefined,
}),
)
: (async () => {
const sessionId = schedulerConfig.getSessionId();
- return await schedulerConfig.getOrCreateScheduler(sessionId, {
+ return await this.config.getOrCreateScheduler(sessionId, {
outputUpdateHandler,
onAllToolCallsComplete: handleCompletion,
onToolCallsUpdate: undefined,
getPreferredEditor: () => undefined,
onEditorClose: () => {},
});
})();
const scheduler = await schedulerPromise;
const schedulerDispose = options?.schedulerFactory
? typeof scheduler.dispose === 'function'
? scheduler.dispose.bind(scheduler)
: () => {}
- : () => schedulerConfig.disposeScheduler(schedulerConfig.getSessionId());
+ : () => this.config.disposeScheduler(schedulerConfig.getSessionId());📝 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.
| const schedulerPromise = options?.schedulerFactory | |
| ? Promise.resolve( | |
| options.schedulerFactory({ | |
| schedulerConfig, | |
| onAllToolCallsComplete: handleCompletion, | |
| outputUpdateHandler, | |
| onToolCallsUpdate: undefined, | |
| }), | |
| ) | |
| : (async () => { | |
| const sessionId = schedulerConfig.getSessionId(); | |
| return await schedulerConfig.getOrCreateScheduler(sessionId, { | |
| outputUpdateHandler, | |
| onAllToolCallsComplete: handleCompletion, | |
| onToolCallsUpdate: undefined, | |
| getPreferredEditor: () => undefined, | |
| onEditorClose: () => {}, | |
| }); | |
| })(); | |
| const scheduler = await schedulerPromise; | |
| const schedulerDispose = options?.schedulerFactory | |
| ? typeof scheduler.dispose === 'function' | |
| ? scheduler.dispose.bind(scheduler) | |
| : () => {} | |
| : () => schedulerConfig.disposeScheduler(schedulerConfig.getSessionId()); | |
| const schedulerPromise = options?.schedulerFactory | |
| ? Promise.resolve( | |
| options.schedulerFactory({ | |
| schedulerConfig, | |
| onAllToolCallsComplete: handleCompletion, | |
| outputUpdateHandler, | |
| onToolCallsUpdate: undefined, | |
| }), | |
| ) | |
| : (async () => { | |
| const sessionId = schedulerConfig.getSessionId(); | |
| return await this.config.getOrCreateScheduler(sessionId, { | |
| outputUpdateHandler, | |
| onAllToolCallsComplete: handleCompletion, | |
| onToolCallsUpdate: undefined, | |
| getPreferredEditor: () => undefined, | |
| onEditorClose: () => {}, | |
| }); | |
| })(); | |
| const scheduler = await schedulerPromise; | |
| const schedulerDispose = options?.schedulerFactory | |
| ? typeof scheduler.dispose === 'function' | |
| ? scheduler.dispose.bind(scheduler) | |
| : () => {} | |
| : () => this.config.disposeScheduler(schedulerConfig.getSessionId()); |
🤖 Prompt for AI Agents
In @packages/core/src/core/subagent.ts around lines 686 - 711, The code is
calling getOrCreateScheduler and disposeScheduler on schedulerConfig (the shim)
which lacks those methods; update the async branch to call
this.config.getOrCreateScheduler(sessionId, {...}) instead of
schedulerConfig.getOrCreateScheduler, and change the fallback schedulerDispose
to call this.config.disposeScheduler(this.config.getSessionId()) (keep the
existing options?.schedulerFactory conditional that binds scheduler.dispose when
present or no-ops otherwise). Ensure you reference the existing symbols:
schedulerConfig.getSessionId(), this.config.getOrCreateScheduler(...),
schedulerDispose conditional, and this.config.disposeScheduler(...).
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cli/src/ui/hooks/useToolScheduler.test.ts (1)
52-96: CRITICAL: Test mock creates real CoreToolScheduler instances causing OOM.The mock at lines 85-91 calls
actualGetOrCreateScheduler, which creates realCoreToolSchedulerinstances in tests. According to the PR comments, this is the root cause of the Node OOM error:
- Real schedulers register MessageBus subscribers, hold maps/sets/timers, and run async work
- When tests repeatedly advance timers, these real instances accumulate memory faster than GC can reclaim
- This causes heap exhaustion and "Channel closed" errors
The recommended fix is to replace the real scheduler with a lightweight mock object implementing the minimal scheduler API.
🐛 Proposed fix: Replace real scheduler with lightweight mock
-const actualCore = await vi.importActual< - typeof import('@vybestack/llxprt-code-core') ->('@vybestack/llxprt-code-core'); -const actualGetOrCreateScheduler = actualCore.getOrCreateScheduler; -const actualDisposeScheduler = actualCore.disposeScheduler; -const actualClearAllSchedulers = actualCore.clearAllSchedulers; - const mockToolRegistry = { getTool: vi.fn(), getAllToolNames: vi.fn(() => ['mockTool', 'anotherTool']), }; const mockMessageBus = { subscribe: vi.fn(), unsubscribe: vi.fn(), publish: vi.fn(), }; +// Lightweight mock scheduler to avoid OOM from real CoreToolScheduler instances +const createMockScheduler = (callbacks: any) => ({ + schedule: vi.fn(async () => { + // Simulate async schedule behavior for tests + await Promise.resolve(); + }), + cancelAll: vi.fn(), + dispose: vi.fn(), + toolCalls: [], + getPreferredEditor: vi.fn(() => undefined), + config: mockConfig, + toolRegistry: mockToolRegistry, +}); + +const mockSchedulers = new Map<string, any>(); + const mockConfig = { getToolRegistry: vi.fn(() => mockToolRegistry as unknown as ToolRegistry), getApprovalMode: vi.fn(() => ApprovalMode.DEFAULT), getSessionId: () => 'test-session-id', getUsageStatisticsEnabled: () => true, getDebugMode: () => false, getAllowedTools: vi.fn(() => []), getContentGeneratorConfig: () => ({ model: 'test-model', authType: 'oauth-personal', }), getMessageBus: () => mockMessageBus, getPolicyEngine: vi.fn(() => ({ evaluate: vi.fn(() => PolicyDecision.ASK_USER), })), - getOrCreateScheduler: vi.fn((sessionId: string, callbacks: any) => - actualGetOrCreateScheduler( - mockConfig as unknown as Config, - sessionId, - callbacks, - ), - ), - disposeScheduler: vi.fn((sessionId: string) => - actualDisposeScheduler(sessionId), - ), + getOrCreateScheduler: vi.fn(async (sessionId: string, callbacks: any) => { + if (!mockSchedulers.has(sessionId)) { + mockSchedulers.set(sessionId, createMockScheduler(callbacks)); + } + return mockSchedulers.get(sessionId); + }), + disposeScheduler: vi.fn((sessionId: string) => { + const scheduler = mockSchedulers.get(sessionId); + if (scheduler) { + scheduler.dispose(); + mockSchedulers.delete(sessionId); + } + }), setInteractiveSubagentSchedulerFactory: vi.fn(), } as unknown as Config;Then remove the
actualClearAllSchedulers()calls at lines 140 and 259 since they're no longer needed:afterEach(() => { cleanup(); vi.clearAllTimers(); vi.useRealTimers(); - // Clean up scheduler instances to avoid memory leaks - void actualClearAllSchedulers(); - + mockSchedulers.clear(); DebugLogger.disposeAll(); });
🧹 Nitpick comments (1)
packages/cli/src/ui/hooks/useToolScheduler.test.ts (1)
157-157: Timer control improvements appropriate after scheduler mock fix.The switch to
vi.useRealTimers()for certain tests and the use ofvi.waitFor()patterns for async assertions are good improvements that make tests more reliable. However, these patterns will work correctly only after the real CoreToolScheduler instances are replaced with lightweight mocks (as noted in the critical issue at lines 85-91).Also applies to: 280-280, 346-346, 387-387, 424-424, 669-669
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/cli/src/ui/hooks/useToolScheduler.test.tspackages/ui/src/hooks/useToolScheduler.ts
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2026-01-09T02:26:33.951Z
Learning: Preference: Consolidate to a single CoreToolScheduler instance per interactive session to avoid duplicated handlers and unknown correlationId noise.
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2025-12-26T14:17:41.983Z
Learning: In packages/core/src/core/coreToolScheduler.ts, the `publishBufferedResults()` method (line 1355) is called concurrently by multiple tool completion handlers (line 1510 in each tool's `.then()` callback) when parallel tools finish simultaneously. This causes a race condition where `nextPublishIndex` can be corrupted and tools can hang in 'executing' state forever, blocking the scheduler and causing subsequent tool calls to queue indefinitely. The method must be made reentrant-safe using a mutex/lock pattern to serialize concurrent calls.
📚 Learning: 2026-01-09T02:26:33.951Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2026-01-09T02:26:33.951Z
Learning: Preference: Consolidate to a single CoreToolScheduler instance per interactive session to avoid duplicated handlers and unknown correlationId noise.
Applied to files:
packages/ui/src/hooks/useToolScheduler.tspackages/cli/src/ui/hooks/useToolScheduler.test.ts
📚 Learning: 2025-12-26T14:17:41.983Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2025-12-26T14:17:41.983Z
Learning: In packages/core/src/core/coreToolScheduler.ts, the `publishBufferedResults()` method (line 1355) is called concurrently by multiple tool completion handlers (line 1510 in each tool's `.then()` callback) when parallel tools finish simultaneously. This causes a race condition where `nextPublishIndex` can be corrupted and tools can hang in 'executing' state forever, blocking the scheduler and causing subsequent tool calls to queue indefinitely. The method must be made reentrant-safe using a mutex/lock pattern to serialize concurrent calls.
Applied to files:
packages/ui/src/hooks/useToolScheduler.tspackages/cli/src/ui/hooks/useToolScheduler.test.ts
📚 Learning: 2025-11-17T08:33:42.962Z
Learnt from: MinWooPark-dotcom
Repo: vybestack/llxprt-code PR: 577
File: packages/cli/src/ui/components/messages/ToolMessage.tsx:89-167
Timestamp: 2025-11-17T08:33:42.962Z
Learning: In the vybestack/llxprt-code repository, single-use helper functions in React components can be kept inline rather than extracted to separate utilities, especially when the extraction doesn't provide immediate value (no reuse, no isolated testing planned) and keeps the PR scope focused on feature delivery.
Applied to files:
packages/ui/src/hooks/useToolScheduler.ts
📚 Learning: 2025-11-16T22:51:26.374Z
Learnt from: e2720pjk
Repo: vybestack/llxprt-code PR: 583
File: packages/core/src/providers/openai/OpenAIProvider.ts:935-959
Timestamp: 2025-11-16T22:51:26.374Z
Learning: In the llxprt-code codebase (packages/core/src/providers/openai/OpenAIProvider.ts), tools like `run_shell_command` use internal streaming only for real-time UI updates during execution, but each tool execution produces exactly ONE final `ToolResponseBlock` containing the full result. The streaming chunks are never sent to the LLM and are not converted into multiple tool messages. The OpenAI Chat Completions API requires that each tool call (tool_call_id) corresponds to exactly one message with role 'tool', so duplicate tool response detection that removes subsequent tool messages with the same tool_call_id is correct and necessary for API compliance.
Applied to files:
packages/cli/src/ui/hooks/useToolScheduler.test.ts
🧬 Code graph analysis (2)
packages/ui/src/hooks/useToolScheduler.ts (2)
packages/core/src/index.ts (1)
sessionId(184-184)scripts/lint.js (1)
config(290-290)
packages/cli/src/ui/hooks/useToolScheduler.test.ts (2)
packages/core/src/index.ts (3)
PolicyDecision(16-16)sessionId(184-184)DebugLogger(339-339)packages/core/src/core/coreToolScheduler.ts (1)
CompletedToolCall(148-151)
⏰ Context from checks skipped due to timeout of 270000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Test (macos-latest, 24.x)
- GitHub Check: Test (ubuntu-latest, 24.x)
- GitHub Check: Test (windows-latest, 24.x)
- GitHub Check: E2E Test (Linux) - sandbox:none
- GitHub Check: Slow E2E - Win
- GitHub Check: E2E Test (Linux) - sandbox:docker
- GitHub Check: E2E Test (macOS)
🔇 Additional comments (6)
packages/ui/src/hooks/useToolScheduler.ts (3)
2-13: LGTM: Import changes support singleton scheduler pattern.The addition of
ToolConfirmationOutcomeand reorganization of type imports correctly support the new scheduler lifecycle and confirmation handling.
295-296: LGTM: Async singleton initialization with proper mount guard.The async initialization pattern correctly prevents state updates after unmount. The scheduler operations (
schedule,cancelAll) safely handle the case where the scheduler hasn't initialized yet by checking for null.Note: The cleanup at line 369 calls
disposeSchedulereven if initialization hasn't completed. This is safe assumingdisposeScheduleris idempotent, which is consistent with singleton patterns.Based on learnings, consolidating to a single CoreToolScheduler instance per session avoids duplicated handlers and unknown correlationId noise.
Also applies to: 338-365
367-372: LGTM: Cleanup properly disposes scheduler singleton.The cleanup sequence correctly:
- Sets
mounted = falseto prevent pending async operations from updating state- Disposes the scheduler via
config.disposeScheduler()to clean up subscriptions/timers- Clears the local ref
This ensures proper lifecycle management and prevents memory leaks.
packages/cli/src/ui/hooks/useToolScheduler.test.ts (3)
33-37: LGTM: Helper for flushing multiple rounds of pending timers.The
flushAllTimershelper correctly iterates multiple times to ensure all pending async timers are flushed, which is useful for complex async test scenarios.
136-136: Test cleanup additions are appropriate but will change with scheduler mock fix.The additions of
cleanup()andDebugLogger.disposeAll()are good test hygiene. TheactualClearAllSchedulers()calls will be replaced withmockSchedulers.clear()once the lightweight mock scheduler is implemented (per the fix for the OOM issue at lines 85-91).Also applies to: 140-140, 142-142, 255-255, 259-259, 260-260
176-184: LGTM: Async test assertions improved with waitFor patterns.The migration from direct assertions to
vi.waitFor()patterns with explicit timeouts and intervals makes tests more robust and less flaky for async operations. These patterns will work reliably once the scheduler mock fix is applied.Also applies to: 300-318, 360-368, 404-412, 442-450, 705-708
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-24.x-ubuntu-latest' artifact from the main CI run. |
Prevents 'getOrCreateScheduler is not a function' runtime error when subagents run interactively. The createSchedulerConfig POJO needed to delegate getOrCreateScheduler/disposeScheduler back to the real Config instance. - Add SchedulerCallbacks type import to subagent.ts - Delegate getOrCreateScheduler(sessionId, callbacks) to this.config - Delegate disposeScheduler(sessionId) to this.config - Await schedulerDispose with error logging in finally block - Add scheduler creation error logging to aid debugging
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)
packages/core/src/core/coreToolScheduler.ts (1)
533-547:dispose()likely needs to clear more internal state to meet the “no leaks” objective.
If a scheduler is disposed mid-session/test, it still retains references viathis.toolCalls,this.requestQueue,this.pendingResults, andthis.callIdToSignal. Also,setImmediatecallbacks can still fire after dispose.Proposed diff (minimal state clearing + disposed guard)
export class CoreToolScheduler { + private disposed = false; // ... dispose(): void { + this.disposed = true; if (this.messageBusUnsubscribe) { this.messageBusUnsubscribe(); this.messageBusUnsubscribe = undefined; } this.pendingConfirmations.clear(); this.processedConfirmations.clear(); this.seenCallIds.clear(); + this.callIdToSignal.clear(); + this.pendingResults.clear(); + this.requestQueue = []; + this.toolCalls = []; // Clean up any pending stale correlation ID timeouts for (const timeout of this.staleCorrelationIds.values()) { clearTimeout(timeout); } this.staleCorrelationIds.clear(); }private async publishBufferedResults(signal: AbortSignal): Promise<void> { + if (this.disposed) return; // ... }
🤖 Fix all issues with AI agents
In @packages/a2a-server/src/agent/task.ts:
- Around line 430-443: The scheduler created by createScheduler() via
this.config.getOrCreateScheduler(sessionId) is never disposed, causing leaks;
update the Task cleanup/termination path (where tasks are removed from
executor.tasks or the task ends) to call await
this.config.disposeScheduler(sessionId) (retrieve sessionId with
this.config.getSessionId()) and then remove the task from executor.tasks; ensure
you await the disposeScheduler call (or handle its promise), catch/log any
errors to avoid throwing during cleanup, and only call disposeScheduler when
sessionId is present and this task actually owns/created the scheduler.
In @packages/a2a-server/src/utils/testing_utils.ts:
- Around line 30-37: The mock for defaultMessageBus.subscribe currently returns
a function that returns an unsubscribe function, making unsubscribe assertions
awkward; change subscribe to return the unsubscribe mock directly (e.g., create
a single unsubscribe mock like unsubscribe = vi.fn() and set subscribe:
vi.fn().mockReturnValue(unsubscribe) or mockImplementation(() => unsubscribe))
so callers get an unsubscribe mock when they call defaultMessageBus.subscribe
and you can assert unsubscribe was called or leaked.
In @packages/ui/src/hooks/useToolScheduler.ts:
- Around line 274-279: ESLint flags the generic Array<...> usage in the
pendingScheduleRequests ref type; change the type declaration for
pendingScheduleRequests (the useRef call) to use the shorthand tuple/array
syntax instead — replace Array<{ request: ToolCallRequestInfo |
ToolCallRequestInfo[]; signal: AbortSignal; }> with { request:
ToolCallRequestInfo | ToolCallRequestInfo[]; signal: AbortSignal; }[] in the
useRef type, and make the same replacement at the other occurrence around the
second declaration referenced (the lines near 414-415) so both refs use ...[]
instead of Array<...>.
🧹 Nitpick comments (5)
packages/cli/src/ui/hooks/useToolScheduler.test.ts (1)
427-433: Acknowledged: Skipped tests documented.The comment adequately explains why confirmation, live output, and cancellation tests are skipped due to testing complexity. Consider creating a follow-up issue to track resolving these testing challenges.
Would you like me to open an issue to track investigating robust testing approaches for the skipped scenarios?
packages/ui/src/hooks/useToolScheduler.ts (1)
296-299: ClearpendingScheduleRequestswhen config/session changes to avoid retaining aborted signals/requests.
Right now, switchingconfigto null only nullsschedulerRef.current; queued requests remain referenced indefinitely.Also applies to: 376-384, 402-417
packages/a2a-server/src/utils/testing_utils.ts (1)
65-219: Mock scheduler looks appropriately “lightweight” (good), but consider per-session caching if tests need singleton semantics.
Right now everygetOrCreateScheduler()call returns a fresh scheduler; that’s fine for many tests, but it won’t catch regressions in “shared instance per session” behavior.packages/core/src/core/coreToolScheduler.ts (1)
1439-1571: Reentrancy guard inpublishBufferedResults()matches the previously reported race.
One small concern: scanningpendingResultsfornextPublishIndexin multiple places is O(n) and repeated; acceptable for small batches, but consider trackingexecutionIndex -> callIdif this grows. (Based on learnings.)packages/cli/src/ui/hooks/useReactToolScheduler.ts (1)
193-198: (Optional) Align with ESLint: prefer...[]overArray<...>.
Even though CI didn’t flag this file, the same rule likely applies repo-wide.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
packages/a2a-server/src/agent/task.tspackages/a2a-server/src/utils/testing_utils.tspackages/cli/src/ui/hooks/atCommandProcessor.test.tspackages/cli/src/ui/hooks/useCommandCompletion.test.tspackages/cli/src/ui/hooks/useReactToolScheduler.tspackages/cli/src/ui/hooks/useToolScheduler.test.tspackages/core/src/config/config.scheduler.test.tspackages/core/src/config/config.tspackages/core/src/config/schedulerSingleton.tspackages/core/src/core/coreToolScheduler.tspackages/core/src/core/subagent.tspackages/core/src/telemetry/loggers.tspackages/ui/src/hooks/useToolScheduler.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/core/src/telemetry/loggers.ts
- packages/core/src/config/config.scheduler.test.ts
- packages/cli/src/ui/hooks/useCommandCompletion.test.ts
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2026-01-09T02:26:33.951Z
Learning: Preference: Consolidate to a single CoreToolScheduler instance per interactive session to avoid duplicated handlers and unknown correlationId noise.
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2025-12-26T14:17:41.983Z
Learning: In packages/core/src/core/coreToolScheduler.ts, the `publishBufferedResults()` method (line 1355) is called concurrently by multiple tool completion handlers (line 1510 in each tool's `.then()` callback) when parallel tools finish simultaneously. This causes a race condition where `nextPublishIndex` can be corrupted and tools can hang in 'executing' state forever, blocking the scheduler and causing subsequent tool calls to queue indefinitely. The method must be made reentrant-safe using a mutex/lock pattern to serialize concurrent calls.
📚 Learning: 2025-12-26T14:17:41.983Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2025-12-26T14:17:41.983Z
Learning: In packages/core/src/core/coreToolScheduler.ts, the `publishBufferedResults()` method (line 1355) is called concurrently by multiple tool completion handlers (line 1510 in each tool's `.then()` callback) when parallel tools finish simultaneously. This causes a race condition where `nextPublishIndex` can be corrupted and tools can hang in 'executing' state forever, blocking the scheduler and causing subsequent tool calls to queue indefinitely. The method must be made reentrant-safe using a mutex/lock pattern to serialize concurrent calls.
Applied to files:
packages/a2a-server/src/utils/testing_utils.tspackages/core/src/core/coreToolScheduler.tspackages/core/src/core/subagent.tspackages/cli/src/ui/hooks/useReactToolScheduler.tspackages/cli/src/ui/hooks/useToolScheduler.test.tspackages/a2a-server/src/agent/task.tspackages/core/src/config/schedulerSingleton.tspackages/ui/src/hooks/useToolScheduler.tspackages/core/src/config/config.ts
📚 Learning: 2025-11-16T22:51:26.374Z
Learnt from: e2720pjk
Repo: vybestack/llxprt-code PR: 583
File: packages/core/src/providers/openai/OpenAIProvider.ts:935-959
Timestamp: 2025-11-16T22:51:26.374Z
Learning: In the llxprt-code codebase (packages/core/src/providers/openai/OpenAIProvider.ts), tools like `run_shell_command` use internal streaming only for real-time UI updates during execution, but each tool execution produces exactly ONE final `ToolResponseBlock` containing the full result. The streaming chunks are never sent to the LLM and are not converted into multiple tool messages. The OpenAI Chat Completions API requires that each tool call (tool_call_id) corresponds to exactly one message with role 'tool', so duplicate tool response detection that removes subsequent tool messages with the same tool_call_id is correct and necessary for API compliance.
Applied to files:
packages/a2a-server/src/utils/testing_utils.tspackages/core/src/core/coreToolScheduler.ts
📚 Learning: 2025-11-25T16:56:18.980Z
Learnt from: CR
Repo: vybestack/llxprt-code PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T16:56:18.980Z
Learning: Before reporting a task as finished, run `npm run test` from the repository root and ensure it succeeds (exit code 0)
Applied to files:
packages/cli/src/ui/hooks/atCommandProcessor.test.ts
📚 Learning: 2026-01-09T02:26:33.951Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2026-01-09T02:26:33.951Z
Learning: Preference: Consolidate to a single CoreToolScheduler instance per interactive session to avoid duplicated handlers and unknown correlationId noise.
Applied to files:
packages/core/src/core/coreToolScheduler.tspackages/core/src/core/subagent.tspackages/cli/src/ui/hooks/useReactToolScheduler.tspackages/cli/src/ui/hooks/useToolScheduler.test.tspackages/a2a-server/src/agent/task.tspackages/core/src/config/schedulerSingleton.tspackages/ui/src/hooks/useToolScheduler.tspackages/core/src/config/config.ts
📚 Learning: 2025-11-17T08:33:42.962Z
Learnt from: MinWooPark-dotcom
Repo: vybestack/llxprt-code PR: 577
File: packages/cli/src/ui/components/messages/ToolMessage.tsx:89-167
Timestamp: 2025-11-17T08:33:42.962Z
Learning: In the vybestack/llxprt-code repository, single-use helper functions in React components can be kept inline rather than extracted to separate utilities, especially when the extraction doesn't provide immediate value (no reuse, no isolated testing planned) and keeps the PR scope focused on feature delivery.
Applied to files:
packages/ui/src/hooks/useToolScheduler.ts
🧬 Code graph analysis (6)
packages/a2a-server/src/utils/testing_utils.ts (4)
packages/core/src/config/schedulerSingleton.ts (1)
SchedulerCallbacks(20-29)packages/core/src/core/turn.ts (1)
ToolCallRequestInfo(101-108)packages/core/src/tools/tools.ts (1)
ToolCallConfirmationDetails(682-686)packages/core/src/core/coreToolScheduler.ts (3)
schedule(833-867)ToolCall(139-146)CompletedToolCall(148-151)
packages/cli/src/ui/hooks/atCommandProcessor.test.ts (1)
packages/cli/src/ui/hooks/atCommandProcessor.ts (1)
handleAtCommand(148-539)
packages/core/src/core/subagent.ts (3)
packages/core/src/index.ts (1)
sessionId(184-184)packages/core/src/config/config.ts (1)
SchedulerCallbacks(1850-1850)packages/core/src/config/schedulerSingleton.ts (1)
SchedulerCallbacks(20-29)
packages/cli/src/ui/hooks/useReactToolScheduler.ts (2)
packages/core/src/index.ts (2)
DebugLogger(339-339)sessionId(184-184)packages/core/src/core/coreToolScheduler.ts (1)
CoreToolScheduler(387-1933)
packages/a2a-server/src/agent/task.ts (2)
packages/core/src/core/coreToolScheduler.ts (1)
CoreToolScheduler(387-1933)packages/core/src/index.ts (1)
sessionId(184-184)
packages/core/src/config/schedulerSingleton.ts (3)
packages/core/src/config/config.ts (3)
SchedulerCallbacks(1850-1850)getOrCreateScheduler(1837-1842)disposeScheduler(1844-1846)packages/core/src/core/coreToolScheduler.ts (3)
CompletedToolCall(148-151)ToolCall(139-146)CoreToolScheduler(387-1933)packages/core/src/utils/editor.ts (1)
EditorType(9-17)
🪛 GitHub Actions: LLxprt Code CI
packages/ui/src/hooks/useToolScheduler.ts
[warning] 267-267: Function 'useToolScheduler' has too many lines (218). Maximum allowed is 80
[error] 275-275: ESLint: Array type using 'Array' is forbidden. Use 'T[]' instead
🪛 GitHub Check: Lint (Javascript)
packages/ui/src/hooks/useToolScheduler.ts
[failure] 275-275:
Array type using 'Array' is forbidden. Use 'T[]' instead
⏰ Context from checks skipped due to timeout of 270000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: E2E Test (Linux) - sandbox:docker
- GitHub Check: E2E Test (macOS)
- GitHub Check: Slow E2E - Win
- GitHub Check: E2E Test (Linux) - sandbox:none
🔇 Additional comments (25)
packages/cli/src/ui/hooks/atCommandProcessor.test.ts (6)
23-23: LGTM!Adding the synchronous
fsimport is appropriate forrealpathSyncoperations used in path resolution throughout the tests.
38-42: LGTM!Returning
fs.realpathSync(fullPath)ensures canonical paths are used, which improves consistency in path comparisons across test assertions.
44-53: LGTM!The CWD management pattern is sound—saving and restoring in
beforeEach/afterEachensures relative path resolution works correctly. TheafterEachhook runs even on test failures, guaranteeing cleanup.Also applies to: 132-136
63-71: LGTM!The message bus mock provides all required methods for the scheduler integration. The mock is appropriately lightweight for the
@command processor tests which don't directly exercise complex message bus interactions.
81-108: LGTM!The
isPathWithinWorkspaceimplementation is well-designed:
- Handles both absolute and relative paths correctly.
- Uses
realpathSyncto resolve symlinks for canonical comparison.- Includes a sensible fallback for non-existent paths.
- Prevents false positives from partial directory name matches by appending
path.sepin the containment check.
184-219: LGTM!The test expectation updates consistently reflect the shift to relative paths. The
Content from @${relativePath}pattern is used uniformly, and the assertions correctly match the expected behavior of thehandleAtCommandfunction with the new workspace context implementation.packages/core/src/config/schedulerSingleton.ts (5)
20-29: LGTM on the SchedulerCallbacks interface.Clean type definitions with appropriate optional/required distinctions. The required
getPreferredEditorandonEditorClosecallbacks align with scheduler lifecycle needs.
31-44: LGTM on internal state management types.The two-map approach (
schedulerEntriesfor ready schedulers,schedulerInitStatesfor in-flight creations) correctly handles concurrent requests and in-flight deduplication.
46-113: LGTM on callback aggregation.The
createCombinedCallbacksfunction correctly aggregates multiple callback sets. The sequentialawaitforonAllToolCallsCompleteensures ordered execution, and the first-defined-wins semantics forgetPreferredEditoris appropriate.
217-244: LGTM on disposal logic.The reference counting and cleanup are correctly implemented. The defensive deletion at line 229 is harmless. The in-flight disposal path (lines 234-243) correctly prevents entry creation when all references are released during initialization.
246-262: LGTM on utility functions.
getSchedulerInstanceprovides direct access for introspection, andclearAllSchedulersproperly cleans up all state with error tolerance—appropriate for test teardown and shutdown scenarios. Based on learnings, this consolidation to a single scheduler per session aligns with the preference to avoid duplicated handlers and unknown correlationId noise.packages/cli/src/ui/hooks/useToolScheduler.test.ts (5)
37-52: LGTM on test helper functions.
buildRequestprovides sensible defaults for creating test requests, andflushAllTimerswith iteration support ensures nested async work completes reliably.
67-291: Excellent mock scheduler implementation.This lightweight mock addresses the OOM issue by avoiding real
CoreToolSchedulerinstances with theirMessageBussubscriptions and internal state. The mock faithfully simulates scheduling, execution, and error paths while maintaining callback wiring—sufficient for testing the hook's behavior without memory overhead.
308-329: LGTM on mock singleton implementation.The
getOrCreateScheduleranddisposeSchedulermocks correctly simulate the session-scoped singleton pattern with thecreatedSchedulersmap, providing proper per-session isolation in tests.
369-378: LGTM on test cleanup.Thorough cleanup disposing all schedulers, clearing timers, and cleaning up
DebugLogger. Deleting from aMapduring iteration is safe in JavaScript.
1046-1309: LGTM on mapToDisplay tests.Comprehensive test coverage for all
ToolCallstatus types including validating, awaiting_approval, scheduled, executing (with/without live output), success, error, and cancelled states. The test structure with parameterized cases is clean and maintainable.packages/core/src/core/coreToolScheduler.ts (3)
376-445:setCallbacks()+ exportedCoreToolSchedulerOptionsis the right direction for singleton reuse.
This enables “same scheduler, updated handlers” without reconstructing MessageBus subscriptions.
450-527: Good: ignore TOOL_CONFIRMATION_RESPONSE when signal is missing (prevents post-finalization noise).
Also good to deletependingConfirmationsentry on that path.
1769-1797: Good:isFinalizingToolCallsreset is now exception-safe.packages/a2a-server/src/agent/task.ts (1)
61-120: Async scheduler initialization inTask.create()is a solid safety improvement.
It prevents “scheduler not initialized” races for normal call paths.Also applies to: 430-443
packages/core/src/core/subagent.ts (2)
1484-1491: Good:createSchedulerConfig()forwardsgetOrCreateScheduler/disposeSchedulerso subagent configs don’t instantiate schedulers directly.
690-729: No disposal hazard: the singleton uses ref-counting to safely handle multiple callers.The
disposeScheduler()method decrementsrefCountand returns early ifrefCount > 0, ensuring the scheduler stays alive until all references are released. Multiple subagents can safely dispose without tearing down the shared session scheduler.packages/ui/src/hooks/useToolScheduler.ts (1)
295-400: No lifecycle hazard—ref-counting prevents premature disposal.
The implementation properly handles multiple UI surfaces sharing the samesessionIdthrough ref-counted disposal (only disposes when refCount reaches 0) and callback updates viasetCallbacks(). Each unmount safely decrements the ref-count without breaking in-flight tool calls from other consumers.Likely an incorrect or invalid review comment.
packages/cli/src/ui/hooks/useReactToolScheduler.ts (1)
191-293: This concern is not valid—disposeScheduleris properly ref-counted and safe.The scheduler entry maintains a
refCountfield that is incremented by eachgetOrCreateSchedulercall and decremented by eachdisposeSchedulercall. The scheduler is only deleted whenrefCountreaches 0. Both the primary scheduler and subagent schedulers call these methods on the samesessionId, so their reference counts are properly tracked together. When a subagent disposes, it only decrements the count; the shared scheduler remains active until all owners (primary + all subagents) have disposed.Likely an incorrect or invalid review comment.
packages/core/src/config/config.ts (1)
85-90: Callback rebinding and disposal safety are properly implemented.Verification confirms:
shouldRefreshCallbacks()(lines 115-125) detects callback changes and triggerssetCallbacks()updates (lines 137-145), preventing stale UI handlers.disposeScheduler()uses ref-counting (lines 220, 239) to prevent premature teardown; disposal only occurs whenrefCountreaches 0.- Multi-consumer support is handled via
createCombinedCallbacks()(lines 46-113) which safely merges handlers from concurrent consumers.- Disposal is idempotent—silently returns if sessionId not found (lines 235-236).
Test coverage includes multi-consumer callback merging, ref-counted disposal ordering, and concurrent scheduler creation.
| private async createScheduler(): Promise<CoreToolScheduler> { | ||
| const sessionId = this.config.getSessionId(); | ||
| if (!sessionId) { | ||
| throw new Error('Scheduler sessionId is required'); | ||
| } | ||
| return await this.config.getOrCreateScheduler(sessionId, { | ||
| outputUpdateHandler: this._schedulerOutputUpdate.bind(this), | ||
| onAllToolCallsComplete: this._schedulerAllToolCallsComplete.bind(this), | ||
| onToolCallsUpdate: this._schedulerToolCallsUpdate.bind(this), | ||
| getPreferredEditor: () => 'vscode', | ||
| config: this.config, | ||
| onEditorClose: () => {}, | ||
| }); | ||
| return scheduler; | ||
| } | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find where Task instances are disposed / request handling finishes, and whether disposeScheduler is invoked.
rg -n "class Task\b|new Task\b|Task\.create\b|disposeScheduler\(|\\.dispose\(\)" packages/a2a-server/src -SRepository: vybestack/llxprt-code
Length of output: 612
🏁 Script executed:
#!/bin/bash
# Read the full Task class definition
cat -n packages/a2a-server/src/agent/task.ts | head -150Repository: vybestack/llxprt-code
Length of output: 5642
🏁 Script executed:
#!/bin/bash
# Get file length and search for cleanup-related methods
wc -l packages/a2a-server/src/agent/task.ts
echo "=== Searching for cleanup/dispose methods ==="
rg -n "dispose|cleanup|destroy|close\(" packages/a2a-server/src/agent/task.tsRepository: vybestack/llxprt-code
Length of output: 152
🏁 Script executed:
#!/bin/bash
# Search entire codebase for disposeScheduler
echo "=== Searching for disposeScheduler in entire codebase ==="
rg -n "disposeScheduler" packages/a2a-server/src -S
echo ""
echo "=== Reading around createScheduler method (lines 420-460) ==="
sed -n '420,460p' packages/a2a-server/src/agent/task.tsRepository: vybestack/llxprt-code
Length of output: 1437
🏁 Script executed:
#!/bin/bash
# Check how Task instances are used and lifecycle in executor
echo "=== Task.create calls and surrounding context in executor.ts ==="
rg -n -B 5 -A 10 "Task.create" packages/a2a-server/src/agent/executor.ts
echo ""
echo "=== Check if tasks are stored and when they're cleaned up ==="
rg -n "completedTasks|activeTasks|taskMap" packages/a2a-server/src/agent/executor.tsRepository: vybestack/llxprt-code
Length of output: 1605
🏁 Script executed:
#!/bin/bash
# Find TaskWrapper class and task removal logic
echo "=== TaskWrapper class definition ==="
rg -n "class TaskWrapper" packages/a2a-server/src -S
echo ""
echo "=== Where tasks are removed/deleted ==="
rg -n "tasks\.delete\|tasks\.clear\|removeTask\|cleanupTask" packages/a2a-server/srcRepository: vybestack/llxprt-code
Length of output: 207
🏁 Script executed:
#!/bin/bash
# Read TaskWrapper class
echo "=== TaskWrapper class (lines 44-100) ==="
sed -n '44,100p' packages/a2a-server/src/agent/executor.ts
echo ""
echo "=== Search for request completion/cleanup in endpoints ==="
rg -n "finalize|complete|cleanup\(" packages/a2a-server/src/http/endpoints.ts | head -20Repository: vybestack/llxprt-code
Length of output: 1818
🏁 Script executed:
#!/bin/bash
# Find HTTP endpoint files
echo "=== HTTP endpoint files ==="
find packages/a2a-server/src -name "*endpoint*" -o -name "*handler*" -o -name "*route*" | head -10
echo ""
echo "=== Check for task finalization anywhere ==="
rg -n "task.*end\|task.*complet\|task.*finali" packages/a2a-server/src --ignore-case | head -20Repository: vybestack/llxprt-code
Length of output: 185
🏁 Script executed:
#!/bin/bash
# Look for HTTP handlers or entry points
echo "=== HTTP directory structure ==="
find packages/a2a-server/src/http -type f | head -20
echo ""
echo "=== Check CoderAgentExecutor for any cleanup methods ==="
rg -n "disposeScheduler|dispose\(|cleanup" packages/a2a-server/src/agent/executor.tsRepository: vybestack/llxprt-code
Length of output: 365
🏁 Script executed:
#!/bin/bash
# Check app.ts for task handling and cleanup
echo "=== app.ts content ==="
cat -n packages/a2a-server/src/http/app.tsRepository: vybestack/llxprt-code
Length of output: 8092
🏁 Script executed:
#!/bin/bash
# Check Config interface for disposeScheduler method
echo "=== Search for Config interface definition ==="
rg -n "interface Config|disposeScheduler" packages/a2a-server/src -A 3 | head -50
echo ""
echo "=== Check if disposeScheduler is defined in llxprt-code-core ==="
rg -n "disposeScheduler" --type ts -g "*.d.ts" | head -20Repository: vybestack/llxprt-code
Length of output: 456
🏁 Script executed:
#!/bin/bash
# Check the testing_utils mock to understand Config interface
echo "=== Context around disposeScheduler mock in testing_utils.ts (lines 200-230) ==="
sed -n '200,230p' packages/a2a-server/src/utils/testing_utils.ts
echo ""
echo "=== Search for Config type usage to understand if disposeScheduler is required ==="
rg -n "config\.disposeScheduler|getOrCreateScheduler" packages/a2a-server/src -A 2Repository: vybestack/llxprt-code
Length of output: 1487
Add config.disposeScheduler(sessionId) to the Task cleanup logic to prevent scheduler leaks.
The createScheduler() method creates a scheduler via config.getOrCreateScheduler() but the scheduler is never disposed. Task instances are stored in executor.tasks indefinitely with no cleanup path. When a session/task ends, call config.disposeScheduler(sessionId) to release the scheduler resource and avoid accumulating leaked scheduler instances.
🤖 Prompt for AI Agents
In @packages/a2a-server/src/agent/task.ts around lines 430 - 443, The scheduler
created by createScheduler() via this.config.getOrCreateScheduler(sessionId) is
never disposed, causing leaks; update the Task cleanup/termination path (where
tasks are removed from executor.tasks or the task ends) to call await
this.config.disposeScheduler(sessionId) (retrieve sessionId with
this.config.getSessionId()) and then remove the task from executor.tasks; ensure
you await the disposeScheduler call (or handle its promise), catch/log any
errors to avoid throwing during cleanup, and only call disposeScheduler when
sessionId is present and this task actually owns/created the scheduler.
| const defaultMessageBus = { | ||
| subscribe: vi.fn().mockReturnValue(() => {}), | ||
| subscribe: vi.fn().mockReturnValue(() => vi.fn()), | ||
| publish: vi.fn(), | ||
| respondToConfirmation: vi.fn(), | ||
| requestConfirmation: vi.fn().mockResolvedValue(true), | ||
| removeAllListeners: vi.fn(), | ||
| listenerCount: vi.fn().mockReturnValue(0), | ||
| }; |
There was a problem hiding this comment.
Mock subscribe should return an unsubscribe mock function, not a function that returns a function.
Current shape makes “unsubscribe called” assertions awkward and can hide leaks.
Proposed diff
- const defaultMessageBus = {
- subscribe: vi.fn().mockReturnValue(() => vi.fn()),
+ const defaultMessageBus = {
+ subscribe: vi.fn().mockReturnValue(vi.fn()),
publish: vi.fn(),
respondToConfirmation: vi.fn(),
requestConfirmation: vi.fn().mockResolvedValue(true),
removeAllListeners: vi.fn(),
listenerCount: vi.fn().mockReturnValue(0),
};📝 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.
| const defaultMessageBus = { | |
| subscribe: vi.fn().mockReturnValue(() => {}), | |
| subscribe: vi.fn().mockReturnValue(() => vi.fn()), | |
| publish: vi.fn(), | |
| respondToConfirmation: vi.fn(), | |
| requestConfirmation: vi.fn().mockResolvedValue(true), | |
| removeAllListeners: vi.fn(), | |
| listenerCount: vi.fn().mockReturnValue(0), | |
| }; | |
| const defaultMessageBus = { | |
| subscribe: vi.fn().mockReturnValue(vi.fn()), | |
| publish: vi.fn(), | |
| respondToConfirmation: vi.fn(), | |
| requestConfirmation: vi.fn().mockResolvedValue(true), | |
| removeAllListeners: vi.fn(), | |
| listenerCount: vi.fn().mockReturnValue(0), | |
| }; |
🤖 Prompt for AI Agents
In @packages/a2a-server/src/utils/testing_utils.ts around lines 30 - 37, The
mock for defaultMessageBus.subscribe currently returns a function that returns
an unsubscribe function, making unsubscribe assertions awkward; change subscribe
to return the unsubscribe mock directly (e.g., create a single unsubscribe mock
like unsubscribe = vi.fn() and set subscribe:
vi.fn().mockReturnValue(unsubscribe) or mockImplementation(() => unsubscribe))
so callers get an unsubscribe mock when they call defaultMessageBus.subscribe
and you can assert unsubscribe was called or leaked.
| const existingEntry = schedulerEntries.get(sessionId); | ||
| if (existingEntry) { | ||
| existingEntry.refCount += 1; | ||
| existingEntry.callbacks = combinedCallbacks; | ||
| return existingEntry.scheduler; | ||
| } | ||
| schedulerEntries.set(sessionId, { | ||
| scheduler, | ||
| refCount: 1, | ||
| callbacks: combinedCallbacks, | ||
| }); | ||
| return scheduler; |
There was a problem hiding this comment.
Reference counting bug causes scheduler leak.
When multiple callers join an in-flight creation, reference counts are double-counted. The joiner's reference is already included when inFlight.refCount is incremented (line 153), and this count is transferred to the entry at line 208. However, the joiner then increments refCount again at line 171, inflating the count.
Trace:
- Caller A starts creation →
initState.refCount = 1 - Caller B joins →
initState.refCount = 2(line 153) - Caller A completes →
entry.refCount = 2(line 208) - Caller B's await resolves →
entry.refCount++at line 171 → refCount = 3 (wrong, should be 2)
This prevents proper disposal since disposeScheduler will never reach zero.
🐛 Proposed fix
const existingEntry = schedulerEntries.get(sessionId);
if (existingEntry) {
- existingEntry.refCount += 1;
existingEntry.callbacks = combinedCallbacks;
return existingEntry.scheduler;
}
- schedulerEntries.set(sessionId, {
- scheduler,
- refCount: 1,
- callbacks: combinedCallbacks,
- });
+ // This path should not normally be reached since the first creator
+ // sets schedulerEntries before any awaiter continues. If it is reached
+ // (e.g., due to disposal during await), create entry with proper refCount.
+ schedulerEntries.set(sessionId, {
+ scheduler,
+ refCount: inFlight.refCount,
+ callbacks: combinedCallbacks,
+ });
return scheduler;📝 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.
| const existingEntry = schedulerEntries.get(sessionId); | |
| if (existingEntry) { | |
| existingEntry.refCount += 1; | |
| existingEntry.callbacks = combinedCallbacks; | |
| return existingEntry.scheduler; | |
| } | |
| schedulerEntries.set(sessionId, { | |
| scheduler, | |
| refCount: 1, | |
| callbacks: combinedCallbacks, | |
| }); | |
| return scheduler; | |
| const existingEntry = schedulerEntries.get(sessionId); | |
| if (existingEntry) { | |
| existingEntry.callbacks = combinedCallbacks; | |
| return existingEntry.scheduler; | |
| } | |
| // This path should not normally be reached since the first creator | |
| // sets schedulerEntries before any awaiter continues. If it is reached | |
| // (e.g., due to disposal during await), create entry with proper refCount. | |
| schedulerEntries.set(sessionId, { | |
| scheduler, | |
| refCount: inFlight.refCount, | |
| callbacks: combinedCallbacks, | |
| }); | |
| return scheduler; |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/ui/src/hooks/useToolScheduler.ts (1)
430-436: Consider clearing pending requests on cancelAll.When
cancelAll()is called while the scheduler is still initializing, pending requests in the queue won't be affected. If the intent is to cancel all pending work, consider also clearingpendingScheduleRequests.current:const cancelAll: CancelAllFn = useCallback(() => { + pendingScheduleRequests.current = []; const scheduler = schedulerRef.current; if (scheduler !== null) { (scheduler as unknown as { cancelAll(): void }).cancelAll(); } }, []);Alternatively, if callers are expected to abort their own signals (which are checked during flush at line 378), the current behavior is acceptable.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/ui/src/hooks/useToolScheduler.ts
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2026-01-09T02:26:33.951Z
Learning: Preference: Consolidate to a single CoreToolScheduler instance per interactive session to avoid duplicated handlers and unknown correlationId noise.
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2025-12-26T14:17:41.983Z
Learning: In packages/core/src/core/coreToolScheduler.ts, the `publishBufferedResults()` method (line 1355) is called concurrently by multiple tool completion handlers (line 1510 in each tool's `.then()` callback) when parallel tools finish simultaneously. This causes a race condition where `nextPublishIndex` can be corrupted and tools can hang in 'executing' state forever, blocking the scheduler and causing subsequent tool calls to queue indefinitely. The method must be made reentrant-safe using a mutex/lock pattern to serialize concurrent calls.
📚 Learning: 2025-12-26T14:17:41.983Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2025-12-26T14:17:41.983Z
Learning: In packages/core/src/core/coreToolScheduler.ts, the `publishBufferedResults()` method (line 1355) is called concurrently by multiple tool completion handlers (line 1510 in each tool's `.then()` callback) when parallel tools finish simultaneously. This causes a race condition where `nextPublishIndex` can be corrupted and tools can hang in 'executing' state forever, blocking the scheduler and causing subsequent tool calls to queue indefinitely. The method must be made reentrant-safe using a mutex/lock pattern to serialize concurrent calls.
Applied to files:
packages/ui/src/hooks/useToolScheduler.ts
📚 Learning: 2026-01-09T02:26:33.951Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 0
File: :0-0
Timestamp: 2026-01-09T02:26:33.951Z
Learning: Preference: Consolidate to a single CoreToolScheduler instance per interactive session to avoid duplicated handlers and unknown correlationId noise.
Applied to files:
packages/ui/src/hooks/useToolScheduler.ts
📚 Learning: 2025-11-17T08:33:42.962Z
Learnt from: MinWooPark-dotcom
Repo: vybestack/llxprt-code PR: 577
File: packages/cli/src/ui/components/messages/ToolMessage.tsx:89-167
Timestamp: 2025-11-17T08:33:42.962Z
Learning: In the vybestack/llxprt-code repository, single-use helper functions in React components can be kept inline rather than extracted to separate utilities, especially when the extraction doesn't provide immediate value (no reuse, no isolated testing planned) and keeps the PR scope focused on feature delivery.
Applied to files:
packages/ui/src/hooks/useToolScheduler.ts
🧬 Code graph analysis (1)
packages/ui/src/hooks/useToolScheduler.ts (2)
packages/core/src/core/turn.ts (1)
ToolCallRequestInfo(101-108)packages/core/src/index.ts (1)
sessionId(184-184)
⏰ Context from checks skipped due to timeout of 270000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Test (windows-latest, 24.x)
- GitHub Check: Test (macos-latest, 24.x)
- GitHub Check: Test (ubuntu-latest, 24.x)
- GitHub Check: Slow E2E - Win
- GitHub Check: E2E Test (Linux) - sandbox:docker
- GitHub Check: E2E Test (Linux) - sandbox:none
- GitHub Check: E2E Test (macOS)
🔇 Additional comments (6)
packages/ui/src/hooks/useToolScheduler.ts (6)
2-13: LGTM!Import additions are appropriate for the singleton scheduler pattern. The
Configtype is needed forgetOrCreateScheduler/disposeSchedulercalls, andToolConfirmationOutcomeis correctly added for the confirmation response typing.
274-279: LGTM!The pending requests queue is a solid pattern for handling the async scheduler initialization. Storing both the request and signal allows proper abort handling when flushing.
301-352: LGTM!The
mountedflag guards are correctly applied to all handlers, preventing React state updates after unmount. This is essential for the async initialization pattern and prevents the common "Can't perform a React state update on an unmounted component" warning.
354-391: LGTM!The async initialization pattern is correctly implemented:
- The singleton is properly created via
config.getOrCreateScheduler()with session scoping- The unmount check after await (lines 369-372) prevents leaks if the component unmounts during initialization
- Setting
schedulerRef.currentbefore flushing pending requests (line 374) ensures newschedule()calls during the flush go directly to the scheduler rather than the queue- Error handling appropriately resets state
This aligns with the PR objective to consolidate schedulers per session and avoid duplicate MessageBus subscriptions. Based on learnings, this addresses the unknown correlationId noise issue.
393-400: LGTM!Cleanup is correctly ordered:
mounted = falseprevents in-flight callbacks from updating statedisposeScheduler(sessionId)ensures proper singleton cleanupschedulerRef.current = nullclears the referenceThe
sessionIdis correctly captured at effect start (line 301) and used consistently in both initialization and cleanup.
403-417: LGTM!The schedule function correctly handles both states:
- When scheduler is ready: schedules directly (lines 408-411)
- When scheduler is initializing: queues the request (line 414)
The early
returnon line 411 is crucial to prevent double-scheduling. The empty dependency array is correct sinceschedulerRefis a stable ref identity.
…ToolScheduler nonInteractive mode (issue #1057) Add test file nonInteractiveToolExecutor.simplified.test.ts that: - Validates executeToolCall passes nonInteractive: true to CoreToolScheduler - Verifies tools needing approval are rejected with POLICY_VIOLATION - Tests emoji filtering is still applied for executor.ts compatibility - Confirms agentId handling and proper scheduler disposal This test file validates the existing behavior implemented in #1055/#1060 where CoreToolScheduler already handles nonInteractive mode with auto-rejection of awaiting_approval status. The test confirms the consolidation is complete - no code changes needed as the nonInteractive flag and auto-rejection logic were already added.
…t expectations This commit fixes 30 test failures caused by a cherry-pick regression. ## Root Cause Cherry-pick 9f0d3a2 (upstream #11671) replaced the entire useToolScheduler.test.ts file with an older version that didn't include the mock scheduler infrastructure from PR #1060 (scheduler consolidation). ## Changes ### useToolScheduler.test.ts (12 tests fixed) - Restored PR 1060's mock scheduler infrastructure: - buildMockScheduler() function - createdSchedulers Map for tracking scheduler instances - mockMessageBus, mockConfig with getOrCreateScheduler/disposeScheduler - Proper cleanup in afterEach with DebugLogger.disposeAll() - Tests now properly mock the async scheduler initialization ### config.test.ts (3 tests fixed) - Fixed loadCliConfig calls to include ExtensionEnablementManager parameter - Updated hierarchical memory test to expect extensions array instead of flat file paths (matches refactored signature) - Fixed getFileFilteringRespectGeminiIgnore -> getFileFilteringRespectLlxprtIgnore ### settings.test.ts (11 tests fixed) - Updated extensionManagement expectation from false to true (new default) - Added disableYoloMode: false to security expectations - Added saveSettings import that was missing - Fixed saveSettings test to not use non-existent updateSettingsFilePreservingFormat ### memoryCommand.test.ts (3 tests fixed) - Added getExtensions mock to config object ### KeypressContext.test.tsx (2 tests fixed) - Updated test expectations for kitty sequence parsing: - name: undefined -> name: 'undefined' (string) - sequence: '\x1b[m' -> code: '[m'
…cate code (issue #1057) (#1087) * test(core): add tests validating nonInteractiveToolExecutor uses CoreToolScheduler nonInteractive mode (issue #1057) Add test file nonInteractiveToolExecutor.simplified.test.ts that: - Validates executeToolCall passes nonInteractive: true to CoreToolScheduler - Verifies tools needing approval are rejected with POLICY_VIOLATION - Tests emoji filtering is still applied for executor.ts compatibility - Confirms agentId handling and proper scheduler disposal This test file validates the existing behavior implemented in #1055/#1060 where CoreToolScheduler already handles nonInteractive mode with auto-rejection of awaiting_approval status. The test confirms the consolidation is complete - no code changes needed as the nonInteractive flag and auto-rejection logic were already added. * refactor(core): simplify nonInteractiveToolExecutor by removing duplicate code (issue #1057) This commit simplifies nonInteractiveToolExecutor.ts from 477 lines to 228 lines by: 1. Removing emoji filtering code (~150 lines) - Emoji filtering is already handled by individual tools (edit.ts, write-file.ts) - Each tool has its own getEmojiFilter() that applies filtering during execution - Removing duplicates this responsibility 2. Removing the awaiting_approval race condition code (~30 lines) - PolicyEngine with nonInteractive: true already converts ASK_USER to DENY - Tools that need approval fail before reaching awaiting_approval state - No need to race between completion and awaiting_approval 3. Deleting the simplified test file - Was added as part of investigation, now redundant 4. Updated test file to reflect simplified implementation - Removed emoji filtering tests (now tested in write-file.test.ts, edit.test.ts) - Added comment explaining the change The remaining ~230 lines handle: - Config adapter for partial Config types - PolicyEngine wrapping with nonInteractive: true - Error response creation - Basic scheduler lifecycle (create, schedule, dispose) Closes #1057
…t expectations This commit fixes 30 test failures caused by a cherry-pick regression. ## Root Cause Cherry-pick 9f0d3a2 (upstream #11671) replaced the entire useToolScheduler.test.ts file with an older version that didn't include the mock scheduler infrastructure from PR #1060 (scheduler consolidation). ## Changes ### useToolScheduler.test.ts (12 tests fixed) - Restored PR 1060's mock scheduler infrastructure: - buildMockScheduler() function - createdSchedulers Map for tracking scheduler instances - mockMessageBus, mockConfig with getOrCreateScheduler/disposeScheduler - Proper cleanup in afterEach with DebugLogger.disposeAll() - Tests now properly mock the async scheduler initialization ### config.test.ts (3 tests fixed) - Fixed loadCliConfig calls to include ExtensionEnablementManager parameter - Updated hierarchical memory test to expect extensions array instead of flat file paths (matches refactored signature) - Fixed getFileFilteringRespectGeminiIgnore -> getFileFilteringRespectLlxprtIgnore ### settings.test.ts (11 tests fixed) - Updated extensionManagement expectation from false to true (new default) - Added disableYoloMode: false to security expectations - Added saveSettings import that was missing - Fixed saveSettings test to not use non-existent updateSettingsFilePreservingFormat ### memoryCommand.test.ts (3 tests fixed) - Added getExtensions mock to config object ### KeypressContext.test.tsx (2 tests fixed) - Updated test expectations for kitty sequence parsing: - name: undefined -> name: 'undefined' (string) - sequence: '\x1b[m' -> code: '[m'
* docs: create cherry-pick plan for v0.11.3..v0.12.0 sync Tracking issue: #709 Range: v0.11.3 → v0.12.0 (135 commits) - PICK: 55 commits (11 batches of 5) - SKIP: 67 commits (release mgmt, GitHub workflows, telemetry, debugLogger migrations) - REIMPLEMENT: 13 commits (deferred - extension manager, policy engine, etc.) Plan uses subagents: - cherrypicker: executes cherry-picks and resolves conflicts - deepthinker: verifies each batch, triggers remediation if needed All batches require full verification: - npm run lint/typecheck/test/format/build - node scripts/start.js --profile-load synthetic smoke test - No conflict markers, all tests must pass * fix(test): unskip and fix useToolScheduler tests (#11671) * Add extension alias for extensions command (#11622) * Add setting to disable YOLO mode (#11609) Co-authored-by: Shreya Keshive <[email protected]> * fix(ui): Fix and unskip InputPrompt tests (#11700) * fix(a2a-server): Fix and unskip GCS persistence test (#11755) * feat(preflight): Use venv for yamllint installation (#11694) * fix(cli): fix race condition and unskip tests in useGitBranchName (#11759) * refactor: simplify FilterReport and remove unused code (#11681) Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Co-authored-by: Jacob Richman <[email protected]> * refactor(core): Clean up exclude description (#11678) Co-authored-by: Jacob Richman <[email protected]> * fix: resolve typecheck errors after cherry-pick batch 02 * fix(ui): resolve race condition in double-escape handler (#8913) Co-authored-by: Megha Bansal <[email protected]> * refactor(cli): Parameterize tests in InputPrompt (#11776) * Docs: Fix broken link in docs/cli/configuration.md (#11655) * Fix broken links in documentation (#11789) Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com> * feat(ux): Surface internal errors via unified event system (#11803) * fix: align shell allowlist handling (#11510) (#11813) * Use raw writes to stdin where possible in tests (#11837) * feat(core) Bump get-ripgrep version. (#11698) * Use raw writes to stdin in test (#11871) * fix(cli): re-throw errors in non-interactive mode (#11849) * Adding Parameterised tests (#11930) * chore(core): add token caching in google auth provider (#11946) * run bom test on windows (#11828) * Fix(cli): Use the correct extensionPath (#11896) * fix(security) - Use emitFeedback (#11961) Co-authored-by: gemini-cli-robot <[email protected]> * Add regression tests for shell command parsing (#11962) * Fix(cli): Use cross-platform path separators in extension tests (#11970) * fix linked extension test on windows (#11973) * fix(cli): Use correct defaults for file filtering (#11426) * use coreEvents.emitFeedback in extension enablement (#11985) * Fix tests (#11998) * fix(tools): ReadFile no longer shows confirmation when message bus is off (#12003) * refactor: Switch over to unified shouldIgnoreFile (#11815) * Fix typo in: packages/cli/src/utils/handleAutoUpdate.ts (#11809) * docs(contributing): update project structure section with missing packages (#11599) * fix: Import FilterFilesOptions in zedIntegration.ts * Fix the shortenPath function to correctly insert ellipsis. (#12004) Co-authored-by: Greg Shikhman <[email protected]> * add absolute file path description for windows (#12007) * fix: user configured oauth scopes should take precedence over discovered scopes (#12088) * docs: update cherry-pick progress for v0.11.3..v0.12.0 sync Summary: - 35 commits successfully cherry-picked - 23 commits skipped (architectural incompatibility) - All verification passes (lint, typecheck, format, build) - Pre-existing test failures unrelated to this sync Tracking: #709 * feat: add ExtensionsList.tsx component for /extensions list command Manually adds the ExtensionsList component from upstream commits: - e89012e (Make a stateful extensions list component, with update statuses (#8301)) - cc7e147 (Pass whole extensions rather than just context files (#10910)) Full cherry-pick was skipped due to extensive conflicts across many files (extension.ts, UIStateContext.tsx, slashCommandProcessor.ts, etc). LLxprt already has the extension infrastructure from previous cherry-picks; this commit adds only the missing UI component. Changes: - Add ExtensionsList.tsx component that displays installed extensions - Add ExtensionsList.test.tsx with tests for extension display states - Wire up ExtensionsList in HistoryItemDisplay for extensions_list type - Use ext.isActive directly instead of checking disabled settings list Adapted for LLxprt: - Uses Colors from LLxprt's color system - Gets extensionsUpdateState from commandContext.ui (LLxprt structure) * Adds executeCommand endpoint with support for /extensions list (#11515) * refactor: pass whole extensions to memoryDiscovery instead of file paths (upstream cc7e147) Changed loadServerHierarchicalMemory to accept GeminiCLIExtension[] objects instead of extensionContextFilePaths: string[], with filtering by isActive at point of use. Changes: - packages/core/src/utils/memoryDiscovery.ts: Updated loadServerHierarchicalMemory signature and internal helper functions to accept extensions array; filtering moved to main function after getting base filePaths - packages/core/src/config/config.ts: Updated refreshMemory to pass extensions instead of extensionContextFilePaths - packages/cli/src/config/config.ts: Updated loadHierarchicalLlxprtMemory and loadCliConfig to pass allExtensions/full extensions array - packages/cli/src/ui/*: Updated callers to pass config.getExtensions() - packages/a2a-server/src/config/config.ts: Updated loadConfig to pass extensions * feat: Propagate traceId from code assist to response metadata (Fixes … (#11360) Co-authored-by: owenofbrien <[email protected]> * feat: enable extension management by default and support gemini-extension.json fallback - Set extensionManagement default to true (was false) - Add fallback to gemini-extension.json when llxprt-extension.json not found - Support upstream extensions that use gemini-extension.json format - Update error messages to mention both config file options * fix(cli): exit cleanly after extensions subcommand completes When running 'extensions install' or other extensions subcommands, the CLI would complete the command but then continue to launch the TUI. This caused dual input handling where both readline (from the consent prompt) and TUI keypress handlers were consuming input, resulting in doubled characters. Add a check similar to the existing 'mcp' command handling to exit after an extensions subcommand runs. * fix: use effectiveExtensionPath for linked extensions and update tests - Fix extensionPath hydration for linked extensions to use the source path instead of the installed symlink path (port of upstream #11896) - Update extension tests to expect coreEvents.emitFeedback instead of console.error - Update error message expectations to include gemini-extension.json fallback * fix(tests): restore PR 1060 scheduler mock infrastructure and fix test expectations This commit fixes 30 test failures caused by a cherry-pick regression. ## Root Cause Cherry-pick 9f0d3a2 (upstream #11671) replaced the entire useToolScheduler.test.ts file with an older version that didn't include the mock scheduler infrastructure from PR #1060 (scheduler consolidation). ## Changes ### useToolScheduler.test.ts (12 tests fixed) - Restored PR 1060's mock scheduler infrastructure: - buildMockScheduler() function - createdSchedulers Map for tracking scheduler instances - mockMessageBus, mockConfig with getOrCreateScheduler/disposeScheduler - Proper cleanup in afterEach with DebugLogger.disposeAll() - Tests now properly mock the async scheduler initialization ### config.test.ts (3 tests fixed) - Fixed loadCliConfig calls to include ExtensionEnablementManager parameter - Updated hierarchical memory test to expect extensions array instead of flat file paths (matches refactored signature) - Fixed getFileFilteringRespectGeminiIgnore -> getFileFilteringRespectLlxprtIgnore ### settings.test.ts (11 tests fixed) - Updated extensionManagement expectation from false to true (new default) - Added disableYoloMode: false to security expectations - Added saveSettings import that was missing - Fixed saveSettings test to not use non-existent updateSettingsFilePreservingFormat ### memoryCommand.test.ts (3 tests fixed) - Added getExtensions mock to config object ### KeypressContext.test.tsx (2 tests fixed) - Updated test expectations for kitty sequence parsing: - name: undefined -> name: 'undefined' (string) - sequence: '\x1b[m' -> code: '[m' * fix: address CodeRabbit review issues - config.ts: Fix disableYoloMode to only affect YOLO mode, not AUTO_EDIT The setting was incorrectly overriding all approval modes to DEFAULT. Now it only blocks YOLO mode as the setting name implies. - gcs.test.ts: Fix mock once() return value that was accidentally commented out The return statement was on the same line as a comment, making it part of the comment instead of actual code. - CONTRIBUTING.md: Add missing colon after test-utils for consistency - utf-bom-encoding.test.ts: Fix typo 'integraion' -> 'integration' * fix: resolve 5 test failures from cherry-pick regressions ## turn.test.ts - thought events with traceId - Mock getResponseText didn't filter thought parts like the real implementation - Added thought filtering to match actual behavior in generateContentResponseUtilities.ts - Also fixed mock data to use thought: true (boolean) not thought: 'thinking' (string) ## memoryDiscovery.test.ts - extension context file paths - Function signature changed from string[] to GeminiCLIExtension[] (commit f6fea31) - Updated test to pass proper extension objects with isActive and contextFiles ## keychain-token-storage.ts - emitFeedback not called - Incomplete cherry-pick: commit 74f688d added tests expecting coreEvents.emitFeedback but the implementation changes weren't included - Added coreEvents import and replaced console.error with coreEvents.emitFeedback in listServers() and getAllCredentials() error handlers * feat: implement shell-replacement allowlist mode with deep AST traversal - Add ShellReplacementMode type with 'allowlist' | 'all' | 'none' values - Change default from false to 'allowlist' to match upstream behavior - Add normalizeShellReplacement() for legacy boolean compatibility - Port upstream collectCommandDetails for deep AST walking: - Extracts commands from command substitution ($(), backticks) - Extracts commands from process substitution (<(), >()) - Extracts commands from function bodies - Walks entire tree to find ALL commands that will execute - Update parseCommandDetails to use collectCommandDetails - Add tests for collectCommandDetails in shell-parser.test.ts - Update shell-utils tests to skip deep-extraction tests when tree-sitter not available in test environment - Update settings.test.ts expectations for new default - Update setCommand.ts with shell-replacement options Security: This ensures nested malicious commands in substitutions and function definitions are validated against the allowlist. * fix(tests): update shell replacement tests for new allowlist default - Update shell.test.ts to set mode to 'none' when testing substitution blocking - Rewrite shell-utils.shellReplacement.test.ts for three-mode behavior: - 'none': blocks all command substitution - 'allowlist' (default): allows substitution, validates inner commands - 'all': allows all substitution (legacy true behavior) - Add tests for legacy boolean value handling (true -> 'all', false -> 'none') - Fix test that expected default to block (now default is allowlist) * fix: address CodeRabbit review feedback from review 3653721722 - Fix turn.test.ts: remove extra 'test-model' argument from turn.run() calls (lines 603-607 and 636-640) - Turn.run only accepts 2 args (req, signal) - Fix memoryDiscovery.test.ts: correct import path for GeminiCLIExtension from '../extensions/types.js' to '../config/config.js' - Fix shell-utils.ts: 'all' mode now skips deep AST validation (legacy behavior) while 'allowlist' mode validates nested commands via tree-sitter * fix(tests): resolve @testing-library/react resolution in scripts/tests Add path alias in scripts/tests/vitest.config.ts to resolve @testing-library/react from packages/cli/node_modules where it gets hoisted, fixing the macOS CI failure for virtualized-list-scrolltop.test.js * fix: restore @lvce-editor/ripgrep and update branding references - Keep @lvce-editor/ripgrep at ^1.6.0 (not the @joshua.litt fork and not v4.x which requires Node 22+). The cherry-pick erroneously switched to a temporary fork package. - Restore package-lock.json from main to fix temp-dir ESM top-level await issue that breaks vscode-ide-companion build on Node 20 - Update .lycheeignore to use vybestack/llxprt-code URLs instead of google-gemini/gemini-cli - Update CONTRIBUTING.md to use LLxprt Code branding and correct URLs throughout * fix(tests): correct @testing-library/react path in scripts vitest config The package is hoisted to root node_modules, not packages/cli/node_modules. Restoring package-lock.json from main changed the hoisting location. --------- Co-authored-by: Sandy Tao <[email protected]> Co-authored-by: kevinjwang1 <[email protected]> Co-authored-by: Adib234 <[email protected]> Co-authored-by: Shreya Keshive <[email protected]> Co-authored-by: JAYADITYA <[email protected]> Co-authored-by: Eric Rahm <[email protected]> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Co-authored-by: Jacob Richman <[email protected]> Co-authored-by: Eric Rahm <[email protected]> Co-authored-by: Keith Lyons <[email protected]> Co-authored-by: Megha Bansal <[email protected]> Co-authored-by: Jainam M <[email protected]> Co-authored-by: Smetalo <[email protected]> Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com> Co-authored-by: Abhi <[email protected]> Co-authored-by: cornmander <[email protected]> Co-authored-by: Tommaso Sciortino <[email protected]> Co-authored-by: joshualitt <[email protected]> Co-authored-by: Riddhi Dutta <[email protected]> Co-authored-by: Adam Weidman <[email protected]> Co-authored-by: Sehoon Shon <[email protected]> Co-authored-by: shishu314 <[email protected]> Co-authored-by: gemini-cli-robot <[email protected]> Co-authored-by: Jacob MacDonald <[email protected]> Co-authored-by: Allen Hutchison <[email protected]> Co-authored-by: Qiyu-Wei <[email protected]> Co-authored-by: Lakshan Perera <[email protected]> Co-authored-by: ph-sp <[email protected]> Co-authored-by: Gaurav Sehgal <[email protected]> Co-authored-by: Jack Wotherspoon <[email protected]> Co-authored-by: jdgarrido1105 <[email protected]> Co-authored-by: Paweł Dec <[email protected]> Co-authored-by: owenofbrien <[email protected]>
This pull request implements a fix for Issue #1055: Consolidate multiple CoreToolScheduler instances into single shared instance per session.
Problem
Multiple CoreToolScheduler instances were being created across the application, leading to:
Solution
Implemented a Config-based singleton pattern with
getOrCreateScheduler()anddisposeScheduler()methods.Changes
Core Infrastructure
packages/core/src/config/schedulerSingleton.ts- Dedicated module avoiding circular dependenciespackages/core/src/config/config.ts- Added singleton methodsconfig.scheduler.test.ts)Updated All Instantiation Points (5 locations)
CLI Interactive Mode (
packages/cli/src/ui/hooks/useReactToolScheduler.ts)config.getOrCreateScheduler()UI Web (
packages/ui/src/hooks/useToolScheduler.ts)config.getOrCreateScheduler()disposeScheduler()A2A Server (
packages/a2a-server/src/agent/task.ts)config.getOrCreateScheduler()Subagents (
packages/core/src/core/subagent.ts)config.getOrCreateScheduler()when no factory providedNon-Interactive Executor (
packages/core/src/core/nonInteractiveToolExecutor.ts)Benefits
Verification
Note: Some pre-existing test failures in CLI and A2A-server packages exist (unrelated to this change - mock setup issues). Core scheduler functionality and all affected code paths are tested and passing.
closes #1055
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
UX
✏️ Tip: You can customize this high-level summary in your review settings.