Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📜 Recent review details⏰ 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)
🧰 Additional context used🧠 Learnings (4)📚 Learning: 2026-02-06T15:52:42.315ZApplied to files:
📚 Learning: 2026-03-31T02:12:43.093ZApplied to files:
📚 Learning: 2026-06-10T18:18:08.545ZApplied to files:
📚 Learning: 2026-06-10T18:18:09.253ZApplied to files:
🔇 Additional comments (1)
Summary by CodeRabbitRelease NotesNew Features
Bug Fixes
Refactor
Tests
WalkthroughIntroduces an engine-owned ChangesEngine-owned AgenticLoop and CLI migration
Sequence Diagram(s)sequenceDiagram
participant useSubmitQuery
participant useGeminiStreamOrchestration
participant useAgenticLoop
participant AgenticLoop
participant AgentClient
participant CoreToolScheduler
useSubmitQuery->>AgenticLoop: runLoop(message, signal, promptId)
AgenticLoop->>AgentClient: sendMessageStream(message)
AgentClient-->>AgenticLoop: stream events
AgenticLoop->>AgenticLoop: streamAndCollect()
AgenticLoop-->>useAgenticLoop: yield stream_event
useAgenticLoop-->>useSubmitQuery: streamEventRouter(event)
AgenticLoop->>CoreToolScheduler: getOrCreateScheduler(schedulerSessionId)
CoreToolScheduler-->>AgenticLoop: tool_update / tool_output callbacks
AgenticLoop-->>useAgenticLoop: yield tool_update / tool_output
useAgenticLoop->>useGeminiStreamOrchestration: displayCallbacks.onToolCallsUpdate()
CoreToolScheduler-->>AgenticLoop: onAllToolCallsComplete(results)
AgenticLoop-->>useAgenticLoop: yield tools_complete
useAgenticLoop->>useAgenticLoop: handleToolsComplete() - history, display, memory
AgenticLoop->>AgentClient: sendMessageStream(functionResponse parts)
Note over AgenticLoop,AgentClient: continuation turn
AgenticLoop-->>useAgenticLoop: yield stream_event (continuation)
useAgenticLoop-->>useSubmitQuery: streamEventRouter(continuation event)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
📋 Issue PlannerBuilt with CodeRabbit's Coding Plans for faster development and fewer bugs. View plan used: ✨ Finishing Touches🧪 Generate unit tests (beta)
|
github-actions
Bot
added
the
maintainer:e2e:ok
LLxprt PR Review – PR #2050Title: Consolidate agentic turn loop into engine (Fixes #2034) Issue AlignmentEvidence the implementation satisfies #2034 requirements:
Side Effects
Code QualityStrengths:
Concerns:
Tests and CoverageCoverage impact: Increase (substantial)
VerdictReady PR #2050 is well-structured, thoroughly tested, and correctly implements the engine-owned agentic loop with policy + approval injection points as required by issue #2034. The CLI is rewired to consume the loop while preserving all user-facing behavior. The Minor notes for reviewers:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/agents/src/core/agenticLoop/AgenticLoop.ts`:
- Around line 107-130: The waitForNext method has a race condition where the
signal can be aborted between the initial signal.aborted check on line 108 and
the addEventListener call on line 124. If abort occurs in this window, the abort
event listener is never triggered and the promise never resolves, causing the
wait to hang. Fix this by adding another check for signal.aborted immediately
after the addEventListener call at line 124, and if the signal is already
aborted, invoke resolve() directly to ensure the promise resolves even if the
abort occurred in the race window.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f0fe9cf8-bc0c-486f-92a6-67e8ac9c4c37
⛔ Files ignored due to path filters (1)
project-plans/issue2034/PLAN.mdis excluded by!project-plans/**
📒 Files selected for processing (28)
packages/agents/src/core/agenticLoop/AgenticLoop.tspackages/agents/src/core/agenticLoop/__tests__/agenticLoop.integration.test.tspackages/agents/src/core/agenticLoop/index.tspackages/agents/src/core/agenticLoop/loopHelpers.tspackages/agents/src/core/agenticLoop/types.tspackages/agents/src/core/coreToolScheduler.tspackages/agents/src/index.tspackages/agents/src/scheduler/confirmation-coordinator.test.tspackages/agents/src/scheduler/confirmation-coordinator.tspackages/cli/src/ui/hooks/geminiStream/__tests__/streamUtils.test.tspackages/cli/src/ui/hooks/geminiStream/__tests__/toolCompletionHandler.test.tspackages/cli/src/ui/hooks/geminiStream/__tests__/useAgenticLoop.test.tsxpackages/cli/src/ui/hooks/geminiStream/__tests__/useStreamEventHandlers.watchdog.test.tspackages/cli/src/ui/hooks/geminiStream/streamEventDispatcher.test.tspackages/cli/src/ui/hooks/geminiStream/streamEventDispatcher.tspackages/cli/src/ui/hooks/geminiStream/streamUtils.tspackages/cli/src/ui/hooks/geminiStream/toolCompletionHandler.tspackages/cli/src/ui/hooks/geminiStream/useAgenticLoop.tspackages/cli/src/ui/hooks/geminiStream/useGeminiStreamLifecycle.tspackages/cli/src/ui/hooks/geminiStream/useGeminiStreamOrchestration.tspackages/cli/src/ui/hooks/geminiStream/useStreamEventHandlers.tspackages/cli/src/ui/hooks/geminiStream/useSubmitQuery.tspackages/cli/src/ui/hooks/useGeminiStream.dedup.test.tsxpackages/cli/src/ui/hooks/useGeminiStream.subagent.spec.tsxpackages/cli/src/ui/hooks/useGeminiStream.thinking.test.tsxpackages/cli/src/ui/hooks/useReactToolScheduler.tspackages/core/src/policy/policy-helpers.tspackages/core/src/scheduler/types.ts
💤 Files with no reviewable changes (2)
- packages/cli/src/ui/hooks/geminiStream/tests/useStreamEventHandlers.watchdog.test.ts
- packages/cli/src/ui/hooks/geminiStream/streamUtils.ts
📜 Review details
⏰ 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). (1)
- GitHub Check: Run LLxprt review
🧰 Additional context used
🧠 Learnings (8)
📚 Learning: 2026-02-06T15:52:42.315Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1305
File: scripts/generate-keybindings-doc.ts:1-5
Timestamp: 2026-02-06T15:52:42.315Z
Learning: In reviews of vybestack/llxprt-code, do not suggest changing existing copyright headers from 'Google LLC' to 'Vybestack LLC' for files that originated from upstream. Preserve upstream copyrights in files that came from upstream, and only apply 'Vybestack LLC' copyright on newly created, original LLxprt files. If a file is clearly LLxprt-original, it may carry the Vybestack header; if it is upstream-originated, keep the original sponsor header.
Applied to files:
packages/core/src/policy/policy-helpers.tspackages/agents/src/index.tspackages/core/src/scheduler/types.tspackages/agents/src/scheduler/confirmation-coordinator.test.tspackages/agents/src/core/agenticLoop/index.tspackages/agents/src/core/agenticLoop/loopHelpers.tspackages/agents/src/core/coreToolScheduler.tspackages/cli/src/ui/hooks/geminiStream/__tests__/streamUtils.test.tspackages/cli/src/ui/hooks/useReactToolScheduler.tspackages/cli/src/ui/hooks/geminiStream/__tests__/toolCompletionHandler.test.tspackages/agents/src/core/agenticLoop/types.tspackages/cli/src/ui/hooks/geminiStream/useAgenticLoop.tspackages/cli/src/ui/hooks/geminiStream/useGeminiStreamLifecycle.tspackages/agents/src/scheduler/confirmation-coordinator.tspackages/cli/src/ui/hooks/geminiStream/useStreamEventHandlers.tspackages/cli/src/ui/hooks/geminiStream/toolCompletionHandler.tspackages/cli/src/ui/hooks/geminiStream/useGeminiStreamOrchestration.tspackages/agents/src/core/agenticLoop/AgenticLoop.tspackages/cli/src/ui/hooks/geminiStream/streamEventDispatcher.test.tspackages/cli/src/ui/hooks/geminiStream/useSubmitQuery.tspackages/agents/src/core/agenticLoop/__tests__/agenticLoop.integration.test.tspackages/cli/src/ui/hooks/geminiStream/streamEventDispatcher.ts
📚 Learning: 2026-03-31T02:12:43.093Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1854
File: packages/core/src/core/subagentRuntimeSetup.test.ts:77-84
Timestamp: 2026-03-31T02:12:43.093Z
Learning: In this codebase, tool declarations should follow the single required contract `parametersJsonSchema`; do not ask to preserve or reintroduce the legacy `parameters` fallback field. Reviewers should not flag assertions/checks for missing `parameters` or suggest backward-compatibility behavior for `parameters`. Schema converters/providers are expected to error if `parametersJsonSchema` is absent instead of falling back to `parameters`.
Applied to files:
packages/core/src/policy/policy-helpers.tspackages/agents/src/index.tspackages/core/src/scheduler/types.tspackages/agents/src/scheduler/confirmation-coordinator.test.tspackages/agents/src/core/agenticLoop/index.tspackages/agents/src/core/agenticLoop/loopHelpers.tspackages/agents/src/core/coreToolScheduler.tspackages/cli/src/ui/hooks/geminiStream/__tests__/streamUtils.test.tspackages/cli/src/ui/hooks/useReactToolScheduler.tspackages/cli/src/ui/hooks/geminiStream/__tests__/toolCompletionHandler.test.tspackages/agents/src/core/agenticLoop/types.tspackages/cli/src/ui/hooks/geminiStream/useAgenticLoop.tspackages/cli/src/ui/hooks/geminiStream/useGeminiStreamLifecycle.tspackages/agents/src/scheduler/confirmation-coordinator.tspackages/cli/src/ui/hooks/geminiStream/useStreamEventHandlers.tspackages/cli/src/ui/hooks/geminiStream/toolCompletionHandler.tspackages/cli/src/ui/hooks/geminiStream/useGeminiStreamOrchestration.tspackages/agents/src/core/agenticLoop/AgenticLoop.tspackages/cli/src/ui/hooks/geminiStream/streamEventDispatcher.test.tspackages/cli/src/ui/hooks/geminiStream/useSubmitQuery.tspackages/agents/src/core/agenticLoop/__tests__/agenticLoop.integration.test.tspackages/cli/src/ui/hooks/geminiStream/streamEventDispatcher.ts
📚 Learning: 2026-06-10T18:18:08.545Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1983
File: packages/policy/src/policy-engine.ts:156-156
Timestamp: 2026-06-10T18:18:08.545Z
Learning: In this repo, ESLint rule `sonarjs/too-many-break-or-continue-in-loop` is set to fail loops that contain more than 1 `break`/`continue` total per loop (or both present). When a loop violates this (e.g., it contains a `break` and a `continue`, or has multiple `break`s/`continue`s), the code will not lint unless the violating line includes `// eslint-disable-next-line sonarjs/too-many-break-or-continue-in-loop`. In code reviews, do not suggest removing these `eslint-disable-next-line` directives (use refactoring only if it eliminates the underlying >1 break/continue pattern).
Applied to files:
packages/core/src/policy/policy-helpers.tspackages/cli/src/ui/hooks/useGeminiStream.subagent.spec.tsxpackages/agents/src/index.tspackages/cli/src/ui/hooks/useGeminiStream.dedup.test.tsxpackages/core/src/scheduler/types.tspackages/agents/src/scheduler/confirmation-coordinator.test.tspackages/agents/src/core/agenticLoop/index.tspackages/cli/src/ui/hooks/useGeminiStream.thinking.test.tsxpackages/agents/src/core/agenticLoop/loopHelpers.tspackages/agents/src/core/coreToolScheduler.tspackages/cli/src/ui/hooks/geminiStream/__tests__/streamUtils.test.tspackages/cli/src/ui/hooks/geminiStream/__tests__/useAgenticLoop.test.tsxpackages/cli/src/ui/hooks/useReactToolScheduler.tspackages/cli/src/ui/hooks/geminiStream/__tests__/toolCompletionHandler.test.tspackages/agents/src/core/agenticLoop/types.tspackages/cli/src/ui/hooks/geminiStream/useAgenticLoop.tspackages/cli/src/ui/hooks/geminiStream/useGeminiStreamLifecycle.tspackages/agents/src/scheduler/confirmation-coordinator.tspackages/cli/src/ui/hooks/geminiStream/useStreamEventHandlers.tspackages/cli/src/ui/hooks/geminiStream/toolCompletionHandler.tspackages/cli/src/ui/hooks/geminiStream/useGeminiStreamOrchestration.tspackages/agents/src/core/agenticLoop/AgenticLoop.tspackages/cli/src/ui/hooks/geminiStream/streamEventDispatcher.test.tspackages/cli/src/ui/hooks/geminiStream/useSubmitQuery.tspackages/agents/src/core/agenticLoop/__tests__/agenticLoop.integration.test.tspackages/cli/src/ui/hooks/geminiStream/streamEventDispatcher.ts
📚 Learning: 2026-06-10T18:18:09.253Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1983
File: packages/policy/src/policy-engine.ts:263-263
Timestamp: 2026-06-10T18:18:09.253Z
Learning: In this repository, the ESLint rule `sonarjs/too-many-break-or-continue-in-loop` is configured to allow at most 1 `break`/`continue` per loop (it is stricter than the SonarJS default). During code review, treat `// eslint-disable-next-line sonarjs/too-many-break-or-continue-in-loop` on loops with 2+ `break`/`continue` as intentional and do not suggest removing or changing those directives. Only consider a change if the rule is violated without an appropriate intentional disable.
Applied to files:
packages/core/src/policy/policy-helpers.tspackages/cli/src/ui/hooks/useGeminiStream.subagent.spec.tsxpackages/agents/src/index.tspackages/cli/src/ui/hooks/useGeminiStream.dedup.test.tsxpackages/core/src/scheduler/types.tspackages/agents/src/scheduler/confirmation-coordinator.test.tspackages/agents/src/core/agenticLoop/index.tspackages/cli/src/ui/hooks/useGeminiStream.thinking.test.tsxpackages/agents/src/core/agenticLoop/loopHelpers.tspackages/agents/src/core/coreToolScheduler.tspackages/cli/src/ui/hooks/geminiStream/__tests__/streamUtils.test.tspackages/cli/src/ui/hooks/geminiStream/__tests__/useAgenticLoop.test.tsxpackages/cli/src/ui/hooks/useReactToolScheduler.tspackages/cli/src/ui/hooks/geminiStream/__tests__/toolCompletionHandler.test.tspackages/agents/src/core/agenticLoop/types.tspackages/cli/src/ui/hooks/geminiStream/useAgenticLoop.tspackages/cli/src/ui/hooks/geminiStream/useGeminiStreamLifecycle.tspackages/agents/src/scheduler/confirmation-coordinator.tspackages/cli/src/ui/hooks/geminiStream/useStreamEventHandlers.tspackages/cli/src/ui/hooks/geminiStream/toolCompletionHandler.tspackages/cli/src/ui/hooks/geminiStream/useGeminiStreamOrchestration.tspackages/agents/src/core/agenticLoop/AgenticLoop.tspackages/cli/src/ui/hooks/geminiStream/streamEventDispatcher.test.tspackages/cli/src/ui/hooks/geminiStream/useSubmitQuery.tspackages/agents/src/core/agenticLoop/__tests__/agenticLoop.integration.test.tspackages/cli/src/ui/hooks/geminiStream/streamEventDispatcher.ts
📚 Learning: 2026-04-23T23:33:11.797Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1907
File: packages/cli/src/ui/App.test.tsx:1583-1586
Timestamp: 2026-04-23T23:33:11.797Z
Learning: In UI test files under packages/cli/src/ui, prefer test titles/descriptions that state the feature being tested (what behavior the user cares about), even if the assertions specifically exercise a guard/edge path. For example, if the test covers the “empty queue ⇒ no auto-send” branch, a title like “auto-send queued messages” is acceptable; aligning the title perfectly to the exact assertion is optional and not required for lint/enforcement batches.
Applied to files:
packages/cli/src/ui/hooks/useGeminiStream.subagent.spec.tsxpackages/cli/src/ui/hooks/useGeminiStream.dedup.test.tsxpackages/cli/src/ui/hooks/useGeminiStream.thinking.test.tsxpackages/cli/src/ui/hooks/geminiStream/__tests__/useAgenticLoop.test.tsx
📚 Learning: 2026-03-25T12:57:21.017Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1768
File: packages/cli/src/ui/hooks/useCoreEventHandlers.ts:71-78
Timestamp: 2026-03-25T12:57:21.017Z
Learning: In files under `packages/cli/src/ui/hooks/`, it is OK for a `useEffect` to register process-level cleanup via `registerCleanup(consolePatcher.cleanup)` without returning a React cleanup function (e.g., without `return () => consolePatcher.cleanup()`). Treat this as an intentional pattern: `registerCleanup()` is the project’s mechanism for cleanup that is meant to persist beyond React unmount/re-render, so do not flag the missing `return` as a leak/bug.
Applied to files:
packages/cli/src/ui/hooks/geminiStream/__tests__/streamUtils.test.tspackages/cli/src/ui/hooks/useReactToolScheduler.tspackages/cli/src/ui/hooks/geminiStream/__tests__/toolCompletionHandler.test.tspackages/cli/src/ui/hooks/geminiStream/useAgenticLoop.tspackages/cli/src/ui/hooks/geminiStream/useGeminiStreamLifecycle.tspackages/cli/src/ui/hooks/geminiStream/useStreamEventHandlers.tspackages/cli/src/ui/hooks/geminiStream/toolCompletionHandler.tspackages/cli/src/ui/hooks/geminiStream/useGeminiStreamOrchestration.tspackages/cli/src/ui/hooks/geminiStream/streamEventDispatcher.test.tspackages/cli/src/ui/hooks/geminiStream/useSubmitQuery.tspackages/cli/src/ui/hooks/geminiStream/streamEventDispatcher.ts
📚 Learning: 2026-03-26T00:49:43.150Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1778
File: packages/cli/src/auth/__tests__/auth-flow-orchestrator.spec.ts:309-324
Timestamp: 2026-03-26T00:49:43.150Z
Learning: In this repository’s Jest (or Jest-like) test files, it is acceptable to use `expect(promiseReturningFunction).resolves.not.toThrow()` when the function returns `Promise<void>`. Do not flag this as an incorrect or suboptimal matcher; for `Promise<void>` it is functionally equivalent to using `resolves.toBeUndefined()` to assert successful resolution.
Applied to files:
packages/cli/src/ui/hooks/geminiStream/__tests__/streamUtils.test.tspackages/cli/src/ui/hooks/geminiStream/__tests__/toolCompletionHandler.test.tspackages/agents/src/core/agenticLoop/__tests__/agenticLoop.integration.test.ts
📚 Learning: 2026-03-26T22:12:58.549Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1780
File: packages/cli/src/ui/hooks/geminiStream/useStreamEventHandlers.ts:1-5
Timestamp: 2026-03-26T22:12:58.549Z
Learning: When reviewing this repo, do not flag these files for missing "upstream" Google LLC copyright. For `useGeminiStream.ts` and the extracted geminiStream-hook modules (under `packages/cli/src/ui/hooks/geminiStream/`), ensure the copyright header is `Copyright 2025 Vybestack LLC`—these files were already updated to Vybestack LLC before PR `#1780` and the decomposition outputs correctly use Vybestack LLC.
Applied to files:
packages/cli/src/ui/hooks/geminiStream/__tests__/streamUtils.test.tspackages/cli/src/ui/hooks/geminiStream/__tests__/toolCompletionHandler.test.tspackages/cli/src/ui/hooks/geminiStream/useAgenticLoop.tspackages/cli/src/ui/hooks/geminiStream/useGeminiStreamLifecycle.tspackages/cli/src/ui/hooks/geminiStream/useStreamEventHandlers.tspackages/cli/src/ui/hooks/geminiStream/toolCompletionHandler.tspackages/cli/src/ui/hooks/geminiStream/useGeminiStreamOrchestration.tspackages/cli/src/ui/hooks/geminiStream/streamEventDispatcher.test.tspackages/cli/src/ui/hooks/geminiStream/useSubmitQuery.tspackages/cli/src/ui/hooks/geminiStream/streamEventDispatcher.ts
🔇 Additional comments (36)
packages/cli/src/ui/hooks/geminiStream/streamEventDispatcher.ts (3)
132-153: LGTM!
234-260: LGTM!
262-293: LGTM!Also applies to: 313-316
packages/cli/src/ui/hooks/geminiStream/useStreamEventHandlers.ts (2)
11-18: LGTM!Also applies to: 88-96
545-584: LGTM!packages/cli/src/ui/hooks/geminiStream/useGeminiStreamOrchestration.ts (8)
7-17: LGTM!
29-29: LGTM!
74-85: LGTM!
114-150: LGTM!
163-198: LGTM!
218-239: LGTM!
241-270: LGTM!
305-359: LGTM!packages/cli/src/ui/hooks/geminiStream/useSubmitQuery.ts (9)
38-38: LGTM!
89-102: LGTM!
115-141: LGTM!
143-177: LGTM!
268-308: LGTM!
310-354: LGTM!
356-365: LGTM!
367-381: LGTM!
414-437: LGTM!packages/agents/src/core/agenticLoop/index.ts (1)
11-24: LGTM!packages/agents/src/index.ts (1)
48-48: LGTM!packages/cli/src/ui/hooks/useReactToolScheduler.ts (1)
99-118: LGTM!Also applies to: 734-751, 760-772, 775-801, 864-867, 874-877, 886-887
packages/agents/src/core/agenticLoop/__tests__/agenticLoop.integration.test.ts (1)
1-1974: LGTM!packages/cli/src/ui/hooks/geminiStream/__tests__/useAgenticLoop.test.tsx (1)
1-543: LGTM!packages/cli/src/ui/hooks/geminiStream/streamEventDispatcher.test.ts (1)
8-8: LGTM!Also applies to: 96-96, 125-125, 139-139, 159-159, 185-185, 208-208, 212-212, 235-235, 258-258, 283-283, 308-308, 333-333, 357-357, 384-384, 391-505
packages/cli/src/ui/hooks/geminiStream/__tests__/streamUtils.test.ts (1)
36-36: LGTM!packages/cli/src/ui/hooks/geminiStream/__tests__/toolCompletionHandler.test.ts (1)
10-14: LGTM!Also applies to: 29-29
packages/cli/src/ui/hooks/useGeminiStream.dedup.test.tsx (1)
89-90: LGTM!Also applies to: 152-170
packages/cli/src/ui/hooks/useGeminiStream.subagent.spec.tsx (1)
234-235: LGTM!Also applies to: 266-267
packages/cli/src/ui/hooks/useGeminiStream.thinking.test.tsx (1)
299-300: LGTM!Also applies to: 336-337
packages/cli/src/ui/hooks/geminiStream/toolCompletionHandler.ts (1)
8-12: LGTM!Also applies to: 14-17, 19-21, 39-48, 53-57, 59-64, 72-72
packages/cli/src/ui/hooks/geminiStream/useAgenticLoop.ts (1)
1-293: LGTM!packages/cli/src/ui/hooks/geminiStream/useGeminiStreamLifecycle.ts (1)
30-31: LGTM!Also applies to: 86-93, 109-110, 112-118, 137-144, 225-230
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/agents/src/core/agenticLoop/AgenticLoop.ts (1)
486-494: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider applying the same abort-recheck pattern for consistency.
The abort promise setup doesn't include the post-listener
signal.abortedrecheck that was added toEventQueue.waitForNext. While this is less critical here because:
- The
drainWhileRunningloop exits viasignal.abortedcheck- The explicit abort handling block (lines 521-527) calls
cancelAll()triggering completion- Multiple fallback resolution paths exist
For defense-in-depth consistency, the same pattern could be applied:
Suggested improvement
const abortPromise = new Promise<null>((resolve) => { if (signal.aborted) { resolve(null); return; } const onAbort = () => resolve(null); signal.addEventListener('abort', onAbort, { once: true }); cleanupAbortListener = () => signal.removeEventListener('abort', onAbort); + // Close check→subscribe race window + if (signal.aborted) { + resolve(null); + } });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/agents/src/core/agenticLoop/AgenticLoop.ts` around lines 486 - 494, The abort promise setup in abortPromise at lines 486-494 is missing the post-listener signal.aborted recheck pattern that was applied to EventQueue.waitForNext for consistency. After registering the 'abort' event listener via signal.addEventListener, add another check for signal.aborted to catch the edge case where the signal is aborted between the initial check (line 488) and the listener registration completing. If signal.aborted is true at that point, resolve the promise immediately. This provides defense-in-depth consistency with the same pattern used elsewhere in the codebase.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/agents/src/core/agenticLoop/AgenticLoop.ts`:
- Around line 486-494: The abort promise setup in abortPromise at lines 486-494
is missing the post-listener signal.aborted recheck pattern that was applied to
EventQueue.waitForNext for consistency. After registering the 'abort' event
listener via signal.addEventListener, add another check for signal.aborted to
catch the edge case where the signal is aborted between the initial check (line
488) and the listener registration completing. If signal.aborted is true at that
point, resolve the promise immediately. This provides defense-in-depth
consistency with the same pattern used elsewhere in the codebase.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ff8a27ad-5c5a-451b-8e6f-55a5079d7845
📒 Files selected for processing (1)
packages/agents/src/core/agenticLoop/AgenticLoop.ts
📜 Review details
⏰ 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). (1)
- GitHub Check: Run LLxprt review
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2026-02-06T15:52:42.315Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1305
File: scripts/generate-keybindings-doc.ts:1-5
Timestamp: 2026-02-06T15:52:42.315Z
Learning: In reviews of vybestack/llxprt-code, do not suggest changing existing copyright headers from 'Google LLC' to 'Vybestack LLC' for files that originated from upstream. Preserve upstream copyrights in files that came from upstream, and only apply 'Vybestack LLC' copyright on newly created, original LLxprt files. If a file is clearly LLxprt-original, it may carry the Vybestack header; if it is upstream-originated, keep the original sponsor header.
Applied to files:
packages/agents/src/core/agenticLoop/AgenticLoop.ts
📚 Learning: 2026-03-31T02:12:43.093Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1854
File: packages/core/src/core/subagentRuntimeSetup.test.ts:77-84
Timestamp: 2026-03-31T02:12:43.093Z
Learning: In this codebase, tool declarations should follow the single required contract `parametersJsonSchema`; do not ask to preserve or reintroduce the legacy `parameters` fallback field. Reviewers should not flag assertions/checks for missing `parameters` or suggest backward-compatibility behavior for `parameters`. Schema converters/providers are expected to error if `parametersJsonSchema` is absent instead of falling back to `parameters`.
Applied to files:
packages/agents/src/core/agenticLoop/AgenticLoop.ts
📚 Learning: 2026-06-10T18:18:08.545Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1983
File: packages/policy/src/policy-engine.ts:156-156
Timestamp: 2026-06-10T18:18:08.545Z
Learning: In this repo, ESLint rule `sonarjs/too-many-break-or-continue-in-loop` is set to fail loops that contain more than 1 `break`/`continue` total per loop (or both present). When a loop violates this (e.g., it contains a `break` and a `continue`, or has multiple `break`s/`continue`s), the code will not lint unless the violating line includes `// eslint-disable-next-line sonarjs/too-many-break-or-continue-in-loop`. In code reviews, do not suggest removing these `eslint-disable-next-line` directives (use refactoring only if it eliminates the underlying >1 break/continue pattern).
Applied to files:
packages/agents/src/core/agenticLoop/AgenticLoop.ts
📚 Learning: 2026-06-10T18:18:09.253Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1983
File: packages/policy/src/policy-engine.ts:263-263
Timestamp: 2026-06-10T18:18:09.253Z
Learning: In this repository, the ESLint rule `sonarjs/too-many-break-or-continue-in-loop` is configured to allow at most 1 `break`/`continue` per loop (it is stricter than the SonarJS default). During code review, treat `// eslint-disable-next-line sonarjs/too-many-break-or-continue-in-loop` on loops with 2+ `break`/`continue` as intentional and do not suggest removing or changing those directives. Only consider a change if the rule is violated without an appropriate intentional disable.
Applied to files:
packages/agents/src/core/agenticLoop/AgenticLoop.ts
🔇 Additional comments (9)
packages/agents/src/core/agenticLoop/AgenticLoop.ts (9)
62-82: LGTM!
107-132: LGTM!
163-200: LGTM!
208-246: LGTM!
260-296: LGTM!
303-363: LGTM!
366-390: LGTM!
544-555: LGTM!
558-589: LGTM!
|
Deep Thinker's Assessment Overall Soundness: Directionally correct. The engine-owned AgenticLoop correctly owns send → stream → collect tool requests → schedule/execute → build functionResponse parts → feed back → repeat. The EventQueue bridge, scheduler isolation, and ConfirmationCoordinator changes are mostly correct. Issues Found (by severity): ┌───────┬─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ Acceptance Criteria Gaps: REQ-LOOP-001, 002, 004, 005 have strong coverage. Missing/under-covered: abort during pending ASK_USER approval; early async-generator return while scheduleAndAwait is mid-flight; explicit disposal assertions after early-return paths; approval handler subscription cleanup after abort. Architect's Assessment Verdict: APPROVE with non-blocking follow-ups. ┌───────────────────┬────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ Agreed Follow-ups (non-blocking, but recommended before merge)
|
|
@CodeRabbit assess this: Deep Thinker's Assessment Overall Soundness: Directionally correct. The engine-owned AgenticLoop correctly owns send → stream → collect tool requests → schedule/execute → build functionResponse parts → feed back → repeat. The EventQueue bridge, scheduler isolation, and ConfirmationCoordinator changes are mostly correct. Issues Found (by severity): ┌───────┬─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ Acceptance Criteria Gaps: REQ-LOOP-001, 002, 004, 005 have strong coverage. Missing/under-covered: abort during pending ASK_USER approval; early async-generator return while scheduleAndAwait is mid-flight; explicit disposal assertions after early-return paths; approval handler subscription cleanup after abort. Architect's Assessment Verdict: APPROVE with non-blocking follow-ups. ┌───────────────────┬────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ Agreed Follow-ups (non-blocking, but recommended before merge)
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Read the AgenticLoop implementation
fd -e ts 'AgenticLoop.ts' --exclude '__tests__' | xargs cat
echo "=== SEPARATOR ==="
# Read confirmation-coordinator.ts
fd -e ts 'confirmation-coordinator.ts' --exclude '.test.' | xargs cat
echo "=== SEPARATOR2 ==="
# Read useAgenticLoop.ts
fd -e ts 'useAgenticLoop.ts' --exclude '.test.' --exclude '.spec.' | xargs catLength of output: 71273
Findings Validation🔴 HIGH:
|
There was a problem hiding this comment.
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/geminiStream/useStreamEventHandlers.ts (1)
573-600: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider destructuring dependencies for consistency and robustness.
The dependency array
[deps, handlers]relies on the caller memoizing thedepsobject. Ifdepsis a new object reference each render, this callback will be recreated on every render, defeatinguseCallback's memoization.Other hooks in this file (e.g.,
useUserCancelledHandlerat lines 290-300,useErrorEventHandlerat lines 341-350) destructure the specific properties they use and list them individually in the dependency array. This pattern is more robust because it doesn't depend on object reference stability.♻️ Suggested refactor to match file's established pattern
function useProcessStreamEvent( deps: StreamEventHandlerDeps, handlers: HandlerMap, ) { + const { + config, + addItem, + sanitizeContent, + flushPendingHistoryItem, + pendingHistoryItemRef, + thinkingBlocksRef, + turnCancelledRef, + loopDetectedRef, + lastModelInfoRef, + lastModelIdentityRef, + setPendingHistoryItem, + setLastGeminiActivityTime, + setThought, + scheduleToolCalls, + } = deps; + const bufferRef = useRef(''); return useCallback( (event: GeminiEvent, userMessageTimestamp: number) => { const result = dispatchStreamEvent( event, { - config: deps.config, - addItem: deps.addItem, - sanitizeContent: deps.sanitizeContent, - flushPendingHistoryItem: deps.flushPendingHistoryItem, - pendingHistoryItemRef: deps.pendingHistoryItemRef, - thinkingBlocksRef: deps.thinkingBlocksRef, - turnCancelledRef: deps.turnCancelledRef, - loopDetectedRef: deps.loopDetectedRef, - lastModelInfoRef: deps.lastModelInfoRef, - lastModelIdentityRef: deps.lastModelIdentityRef, - setPendingHistoryItem: deps.setPendingHistoryItem, - setLastGeminiActivityTime: deps.setLastGeminiActivityTime, - setThought: deps.setThought, + config, + addItem, + sanitizeContent, + flushPendingHistoryItem, + pendingHistoryItemRef, + thinkingBlocksRef, + turnCancelledRef, + loopDetectedRef, + lastModelInfoRef, + lastModelIdentityRef, + setPendingHistoryItem, + setLastGeminiActivityTime, + setThought, ...handlers, - scheduleToolCalls: deps.scheduleToolCalls, + scheduleToolCalls, }, bufferRef.current, userMessageTimestamp, ); bufferRef.current = result.geminiMessageBuffer; }, - [deps, handlers], + [ + config, + addItem, + sanitizeContent, + flushPendingHistoryItem, + pendingHistoryItemRef, + thinkingBlocksRef, + turnCancelledRef, + loopDetectedRef, + lastModelInfoRef, + lastModelIdentityRef, + setPendingHistoryItem, + setLastGeminiActivityTime, + setThought, + scheduleToolCalls, + handlers, + ], ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/ui/hooks/geminiStream/useStreamEventHandlers.ts` around lines 573 - 600, The useCallback hook in this function relies on the entire deps object reference for its dependency array, which is fragile if the caller doesn't properly memoize it. Instead, follow the pattern used elsewhere in the file by destructuring the specific properties from deps that are actually used (such as config, addItem, sanitizeContent, flushPendingHistoryItem, pendingHistoryItemRef, thinkingBlocksRef, turnCancelledRef, loopDetectedRef, lastModelInfoRef, lastModelIdentityRef, setPendingHistoryItem, setLastGeminiActivityTime, setThought, and scheduleToolCalls), and then list each of these individual properties along with handlers in the dependency array instead of passing the entire deps object. This ensures the callback only recreates when the actual dependencies it uses change, regardless of whether deps itself is a new reference.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/cli/src/ui/hooks/geminiStream/useStreamEventHandlers.ts`:
- Around line 573-600: The useCallback hook in this function relies on the
entire deps object reference for its dependency array, which is fragile if the
caller doesn't properly memoize it. Instead, follow the pattern used elsewhere
in the file by destructuring the specific properties from deps that are actually
used (such as config, addItem, sanitizeContent, flushPendingHistoryItem,
pendingHistoryItemRef, thinkingBlocksRef, turnCancelledRef, loopDetectedRef,
lastModelInfoRef, lastModelIdentityRef, setPendingHistoryItem,
setLastGeminiActivityTime, setThought, and scheduleToolCalls), and then list
each of these individual properties along with handlers in the dependency array
instead of passing the entire deps object. This ensures the callback only
recreates when the actual dependencies it uses change, regardless of whether
deps itself is a new reference.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1069427c-9f22-4254-a89a-a400d0f17d00
📒 Files selected for processing (5)
packages/agents/src/core/agenticLoop/AgenticLoop.tspackages/agents/src/core/agenticLoop/__tests__/agenticLoop.integration.test.tspackages/agents/src/scheduler/confirmation-coordinator.test.tspackages/agents/src/scheduler/confirmation-coordinator.tspackages/cli/src/ui/hooks/geminiStream/useStreamEventHandlers.ts
📜 Review details
⏰ 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: E2E Test (Linux) - sandbox:docker
- GitHub Check: E2E Test (Linux) - sandbox:none
- GitHub Check: E2E Test (macOS)
- GitHub Check: Lint (Javascript)
- GitHub Check: CodeQL
- GitHub Check: Interactive UI (tmux)
- GitHub Check: Run LLxprt review
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2026-02-06T15:52:42.315Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1305
File: scripts/generate-keybindings-doc.ts:1-5
Timestamp: 2026-02-06T15:52:42.315Z
Learning: In reviews of vybestack/llxprt-code, do not suggest changing existing copyright headers from 'Google LLC' to 'Vybestack LLC' for files that originated from upstream. Preserve upstream copyrights in files that came from upstream, and only apply 'Vybestack LLC' copyright on newly created, original LLxprt files. If a file is clearly LLxprt-original, it may carry the Vybestack header; if it is upstream-originated, keep the original sponsor header.
Applied to files:
packages/agents/src/scheduler/confirmation-coordinator.test.tspackages/agents/src/core/agenticLoop/AgenticLoop.tspackages/agents/src/core/agenticLoop/__tests__/agenticLoop.integration.test.tspackages/cli/src/ui/hooks/geminiStream/useStreamEventHandlers.tspackages/agents/src/scheduler/confirmation-coordinator.ts
📚 Learning: 2026-03-31T02:12:43.093Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1854
File: packages/core/src/core/subagentRuntimeSetup.test.ts:77-84
Timestamp: 2026-03-31T02:12:43.093Z
Learning: In this codebase, tool declarations should follow the single required contract `parametersJsonSchema`; do not ask to preserve or reintroduce the legacy `parameters` fallback field. Reviewers should not flag assertions/checks for missing `parameters` or suggest backward-compatibility behavior for `parameters`. Schema converters/providers are expected to error if `parametersJsonSchema` is absent instead of falling back to `parameters`.
Applied to files:
packages/agents/src/scheduler/confirmation-coordinator.test.tspackages/agents/src/core/agenticLoop/AgenticLoop.tspackages/agents/src/core/agenticLoop/__tests__/agenticLoop.integration.test.tspackages/cli/src/ui/hooks/geminiStream/useStreamEventHandlers.tspackages/agents/src/scheduler/confirmation-coordinator.ts
📚 Learning: 2026-06-10T18:18:08.545Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1983
File: packages/policy/src/policy-engine.ts:156-156
Timestamp: 2026-06-10T18:18:08.545Z
Learning: In this repo, ESLint rule `sonarjs/too-many-break-or-continue-in-loop` is set to fail loops that contain more than 1 `break`/`continue` total per loop (or both present). When a loop violates this (e.g., it contains a `break` and a `continue`, or has multiple `break`s/`continue`s), the code will not lint unless the violating line includes `// eslint-disable-next-line sonarjs/too-many-break-or-continue-in-loop`. In code reviews, do not suggest removing these `eslint-disable-next-line` directives (use refactoring only if it eliminates the underlying >1 break/continue pattern).
Applied to files:
packages/agents/src/scheduler/confirmation-coordinator.test.tspackages/agents/src/core/agenticLoop/AgenticLoop.tspackages/agents/src/core/agenticLoop/__tests__/agenticLoop.integration.test.tspackages/cli/src/ui/hooks/geminiStream/useStreamEventHandlers.tspackages/agents/src/scheduler/confirmation-coordinator.ts
📚 Learning: 2026-06-10T18:18:09.253Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1983
File: packages/policy/src/policy-engine.ts:263-263
Timestamp: 2026-06-10T18:18:09.253Z
Learning: In this repository, the ESLint rule `sonarjs/too-many-break-or-continue-in-loop` is configured to allow at most 1 `break`/`continue` per loop (it is stricter than the SonarJS default). During code review, treat `// eslint-disable-next-line sonarjs/too-many-break-or-continue-in-loop` on loops with 2+ `break`/`continue` as intentional and do not suggest removing or changing those directives. Only consider a change if the rule is violated without an appropriate intentional disable.
Applied to files:
packages/agents/src/scheduler/confirmation-coordinator.test.tspackages/agents/src/core/agenticLoop/AgenticLoop.tspackages/agents/src/core/agenticLoop/__tests__/agenticLoop.integration.test.tspackages/cli/src/ui/hooks/geminiStream/useStreamEventHandlers.tspackages/agents/src/scheduler/confirmation-coordinator.ts
📚 Learning: 2026-03-26T00:49:43.150Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1778
File: packages/cli/src/auth/__tests__/auth-flow-orchestrator.spec.ts:309-324
Timestamp: 2026-03-26T00:49:43.150Z
Learning: In this repository’s Jest (or Jest-like) test files, it is acceptable to use `expect(promiseReturningFunction).resolves.not.toThrow()` when the function returns `Promise<void>`. Do not flag this as an incorrect or suboptimal matcher; for `Promise<void>` it is functionally equivalent to using `resolves.toBeUndefined()` to assert successful resolution.
Applied to files:
packages/agents/src/core/agenticLoop/__tests__/agenticLoop.integration.test.ts
📚 Learning: 2026-03-25T12:57:21.017Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1768
File: packages/cli/src/ui/hooks/useCoreEventHandlers.ts:71-78
Timestamp: 2026-03-25T12:57:21.017Z
Learning: In files under `packages/cli/src/ui/hooks/`, it is OK for a `useEffect` to register process-level cleanup via `registerCleanup(consolePatcher.cleanup)` without returning a React cleanup function (e.g., without `return () => consolePatcher.cleanup()`). Treat this as an intentional pattern: `registerCleanup()` is the project’s mechanism for cleanup that is meant to persist beyond React unmount/re-render, so do not flag the missing `return` as a leak/bug.
Applied to files:
packages/cli/src/ui/hooks/geminiStream/useStreamEventHandlers.ts
📚 Learning: 2026-03-26T22:12:58.549Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1780
File: packages/cli/src/ui/hooks/geminiStream/useStreamEventHandlers.ts:1-5
Timestamp: 2026-03-26T22:12:58.549Z
Learning: When reviewing this repo, do not flag these files for missing "upstream" Google LLC copyright. For `useGeminiStream.ts` and the extracted geminiStream-hook modules (under `packages/cli/src/ui/hooks/geminiStream/`), ensure the copyright header is `Copyright 2025 Vybestack LLC`—these files were already updated to Vybestack LLC before PR `#1780` and the decomposition outputs correctly use Vybestack LLC.
Applied to files:
packages/cli/src/ui/hooks/geminiStream/useStreamEventHandlers.ts
🔇 Additional comments (21)
packages/agents/src/core/agenticLoop/AgenticLoop.ts (7)
62-82: LGTM!
89-140: LGTM!
142-200: LGTM!
202-296: LGTM!
298-390: LGTM!
519-543: The try/finally pattern correctly ensures cleanup on early generator exit.The
normalExitflag combined with the finally block guarantees that:
- If the consumer abandons the generator early (e.g.,
breakfromfor await...of),scheduler.cancelAll()is called and resources are cleaned upforwardingActive,queue.close(),cleanupAbortListener(), anddisposeScheduler()always execute regardless of exit pathThis addresses the high-severity resource leak concern identified in the PR discussion.
546-598: LGTM!packages/agents/src/scheduler/confirmation-coordinator.ts (8)
54-114: LGTM!
126-199: LGTM!
200-300: LGTM!
346-378: Simplified onConfirm delegation removes redundant bus response path.The wrapped
onConfirmnow directly delegates tohandleConfirmationResponse(), which centralizes outcome handling and bus publishing. This eliminates the previous pattern where the callback would publish to the bus before calling the response handler.
380-472: LGTM!
520-522: Correctly defers originalOnConfirm invocation for ModifyWithEditor.The condition prevents calling the tool's original confirmation callback during
ModifyWithEditor, which is a transitional state. The callback is preserved viapendingOriginalConfirmHandlersand invoked only when the user makes a final decision (approve/cancel) after editing.
606-707: LGTM!
709-876: LGTM!packages/agents/src/core/agenticLoop/__tests__/agenticLoop.integration.test.ts (1)
1076-1123: LGTM!packages/agents/src/scheduler/confirmation-coordinator.test.ts (3)
83-118: LGTM!
269-304: LGTM!
932-1055: LGTM!packages/cli/src/ui/hooks/geminiStream/useStreamEventHandlers.ts (2)
112-127: LGTM!
446-446: LGTM!
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. |
|
@CodeRabbit evaluate this critique from another model: Summary of Most Important Issues| # | Severity | Issue | |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Read the AgenticLoop scheduleAndAwait and abort path
fd -e ts 'AgenticLoop.ts' --exclude '__tests__' | xargs grep -n "cancelAll\|forwardingActive\|drainWhileRunning\|scheduleAndAwait\|abort" | head -80
echo "=== FULL scheduleAndAwait ==="
fd -e ts 'AgenticLoop.ts' --exclude '__tests__' | xargs cat | grep -A 80 "scheduleAndAwait"
echo "=== SEPARATOR ==="
# handleConfirmationResponse and ModifyWithEditor
fd -e ts 'confirmation-coordinator.ts' --exclude '.test.' | xargs grep -n "ModifyWithEditor\|handleConfirmationResponse\|onConfirm\|skipBusPublish" | head -60
echo "=== FULL handleConfirmationResponse ==="
fd -e ts 'confirmation-coordinator.ts' --exclude '.test.' | xargs cat | grep -A 40 "handleConfirmationResponse"
echo "=== SEPARATOR2 ==="
# Profile-change-during-stream - check streamEventDispatcher
fd -e ts 'streamEventDispatcher.ts' --exclude '.test.' | xargs grep -n "profile\|model\|invalidat\|chat\|ModelInfo\|Finished" | head -40
echo "=== SEPARATOR3 ==="
# useAgenticLoop runLoop callback recreation
fd -e ts 'useAgenticLoop.ts' --exclude '.test.' --exclude '.spec.' | xargs grep -n "runLoop\|useCallback\|useMemo\|useRef\|args\." | head -50Length of output: 21774
Issue
|
| Issue | Verdict | Assessment quality |
|---|---|---|
#3 – Events lost after cancelAll() |
✅ Confirmed real | Accurate and well-targeted |
#5 – onConfirm skipped for ModifyWithEditor |
Overstates risk; misreads the callback contract | |
#7 – Deferred chat invalidation |
❌ Not supported by code | Speculative; no evidence found |
#1 – runLoop stale closures |
✅ Confirmed (same as prior Medium #4) |
Real issue, framing slightly imprecise |
Issue #3 is the only genuinely new finding here worth adding to the follow-up list. Issues #5 and #7 don't warrant action, and #1 is a duplicate of an already-identified concern.
|
@CodeRabbit evaluate these as well Issues I've Spotted###1. In useAgenticLoop.ts line ~249, the runLoop callback has [loop, args, processedMemoryTools] as deps. Since args is an object literal passed from the parent, it gets a new identity on every render, which means runLoop is recreated on every render. But runLoopRef.current = agenticLoop.runLoop is set during render in the orchestrator, so this is patched over by the ref pattern. However, it partially defeats the purpose of the useMemo on loop (which deliberately uses stable deps). The runLoop callback closes over args fields that change every render (addItem, onTodoPause, etc.), but since it's consumed via a ref, stale closures are avoided at call time. This is functional but fragile — a future maintainer who uses runLoop directly (not via ref) will get a stale closure. ###2. In AgenticLoop.ts ~line103-106, waitForNext checks this.buffered.length >0 synchronously, then returns a Promise. Between that check and the new Promise executor running (which sets this.resolveWait), the JS event loop won't interleave (single-threaded), so this is actually safe in practice. However, if push() is ever called from a different microtask context before the executor runs... Actually, in JS this is fine because the executor runs synchronously during new Promise. No issue. ###3. In AgenticLoop.ts, drainWhileRunning loops while (!state.completionSettled && !signal.aborted). On abort, it exits the loop, and the final yield* flushBuffered(queue) runs. Then control returns to scheduleAndAwait which does scheduler.cancelAll() and await Promise.race([scheduleTask, completionTask]). Events pushed between cancelAll() and queue.close() in the finally block are not yielded to the consumer. The forwardingActive = false flag prevents new pushes after the finally starts, but there's a window between drainWhileRunning returning and forwardingActive being set to false where cancelAll triggers callbacks that push into the queue but are never drained. The onAllToolCallsComplete callback explicitly pushes a final tool_update: [] if forwardingActive is still true. This is a potential display-state leak — the loop's consumer might miss the final tool cancellation display update. In practice the CLI display may not update cancelled tools correctly during abort. ###4. In confirmation-coordinator.ts, when handleModifyWithEditor re-publishes with a new correlationId, it sets pendingOriginalConfirmHandlers.set(newCorrelationId, originalOnConfirm) but the old correlationId's entry is cleaned up via registerStaleCorrelationId → cleanupCorrelationId. This cleanup path does this.pendingOriginalConfirmHandlers.delete(correlationId) (line ~835). So this is properly handled. No issue. ###5. In confirmation-coordinator.ts ~line519-520:
###6. The change from void (fire-and-forget) to await means schedule() now blocks until checkAndNotifyCompletion resolves. The comment says this ensures the scheduler notifies before schedule() resolves, which the loop relies on for its fallback logic. But if onAllToolCallsComplete (the notification callback) does heavy synchronous work, schedule() will now be slower. The loop's fallback (resolveCompletion([]) when !acceptedToolUpdateSeen) depends on schedule() resolving first, so this ordering is critical. This looks correct but is a subtle coupling — the loop assumes schedule() resolves only after tools are accepted or the scheduling is a no-op. ###7. In client.ts, invalidateChatForProfileChange sets this._previousHistory = undefined (discarding any previous history). If a profile change happens mid-stream, the deferred path saves the HistoryService via this._storedHistoryService = this.chat.getHistoryService(), but loses _previousHistory. The non-deferred (immediate) path does the same. This is consistent behavior but means any unflushed _previousHistory is lost on profile change — which appears to be the original behavior as well (the old handleModelProfileChanged just set currentSequenceModel = null and didn't touch chat at all). This is actually a new behavior — before this branch, a profile change mid-stream did NOT invalidate the chat. Now it defers invalidation until streams complete. This could cause subtle issues if the profile change should take effect immediately. ###8. resetBufferAfterTerminal resets geminiMessageBuffer to '' on terminal events. But the old code managed the buffer externally in consumeStreamEvents and didn't reset it on Finished (it just continued). With the new single-event processStreamEvent, the buffer is in a useRef, so it persists across events. The reset on Finished is correct (each turn should start fresh). However, Finished is now in BUFFER_RESET_EVENTS, and the Finished case in handleNotificationStreamEvent calls flushPendingGeminiContent before the buffer is reset by resetBufferAfterTerminal. Since resetBufferAfterTerminal wraps the return value, the flush happens first (good), then the buffer is cleared (good). This ordering is correct. ###9. The cancelOngoingRequest now also triggers on WaitingForConfirmation. This means pressing Ctrl+C during a tool confirmation will now abort the controller. But the AgenticLoop owns the abort signal — if the loop is waiting for an approval handler (which it manages via wireApprovalHandler), aborting the signal will cause the loop to exit its drainWhileRunning loop. The approval handler's pending promise will still be in-flight, and the wireApprovalHandler unsubscribe in the finally block will prevent the response from reaching the bus. This should be fine (the tool will be cancelled), but there's a question of whether the confirmation bus has a dangling unanswered request. ###10. At line ~501-505, processedConfirmations.has(callId) returns early. But in handleModifyWithEditor, processedConfirmations.delete(callId) is called to allow re-confirmation. If the editor flow races with a duplicate confirmation (e.g., user double-presses enter), the delete + re-add could allow a duplicate to slip through. The stale-correlation-id mechanism should guard this, but the timing window exists. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Issue `#1`: useAgenticLoop runLoop deps array
fd -e ts 'useAgenticLoop.ts' --exclude '__tests__' --exclude '.spec.' | xargs grep -n "useCallback\|useMemo\|deps\|runLoop\|processedMemoryTools\|\[loop" | head -40
echo "=== SEPARATOR ==="
# Issue `#6`: coreToolScheduler await change and coupling
fd -e ts 'coreToolScheduler.ts' | xargs grep -n "checkAndNotifyCompletion\|acceptedToolUpdateSeen\|resolveCompletion\|schedule\b" | head -50
echo "=== SEPARATOR2 ==="
# Issue `#7`: AgentClient handleModelProfileChanged
fd -e ts 'client.ts' --exclude '.test.' --exclude '.spec.' | xargs grep -n "handleModelProfileChanged\|invalidateChatForProfileChange\|_previousHistory\|_storedHistoryService\|deferred\|profileChange" | head -50
echo "=== SEPARATOR3 ==="
# Issue `#9`: useGeminiStreamLifecycle WaitingForConfirmation cancellation + bus dangling
fd -e ts 'useGeminiStreamLifecycle.ts' | xargs grep -n "WaitingForConfirmation\|cancelOngoingRequest\|abort\|Confirmation" | head -40
echo "=== SEPARATOR4 ==="
# Issue `#10`: processedConfirmations guard + handleModifyWithEditor race
fd -e ts 'confirmation-coordinator.ts' --exclude '.test.' | xargs grep -n "processedConfirmations\|handleModifyWithEditor\|delete\|has(callId\|add(callId" | head -60
echo "=== SEPARATOR5 ==="
# Issue `#5`: originalOnConfirm skip for ModifyWithEditor - get full handleConfirmationResponse
fd -e ts 'confirmation-coordinator.ts' --exclude '.test.' | xargs cat | grep -A 60 "async handleConfirmationResponse"Length of output: 8691 🏁 Script executed: #!/bin/bash
# Get the full scheduleAndAwait and drainWhileRunning to validate `#3` more precisely
fd -e ts 'AgenticLoop.ts' --exclude '__tests__' | xargs cat | grep -n "" | head -300Length of output: 11562 🏁 Script executed: #!/bin/bash
# Get scheduleAndAwait and drainWhileRunning from AgenticLoop.ts
fd -e ts 'AgenticLoop.ts' --exclude '__tests__' | xargs cat | grep -n "" | sed -n '300,598p'
echo "=== CLIENT ==="
# Get handleModelProfileChanged and invalidateChatForProfileChange
fd -e ts 'client.ts' --exclude '.test.' --exclude '.spec.' | xargs cat | grep -n "" | sed -n '230,320p'Length of output: 15256
Issue
|
| # | Verdict | Net assessment |
|---|---|---|
#1 |
✅ Confirmed | Duplicate of prior Medium — fragile ref pattern, stale non-display closures |
#2 |
✅ Self-resolved | No issue |
#3 |
✅ Confirmed | Real gap — cancelAll() events lost after abort drain; same as prior Medium #3 |
#4 |
✅ Self-resolved | No issue |
#5 |
Design-correct skip; same conclusion as prior | |
#6 |
Not a deadlock; coupling is intentional and safe | |
#7 |
Real behavior change deserving a note; not a correctness regression | |
#8 |
✅ Self-resolved | Correct ordering |
#9 |
✅ New finding | Concretizes the fire-and-forget risk via WaitingForConfirmation abort path |
#10 |
Defense in depth guards it; theoretical only |
The only genuinely new actionable finding beyond the prior rounds is #9: the WaitingForConfirmation cancellation change creates a concrete path that triggers the dangling approval-handler-promise scenario. It strengthens the case for the abort-aware approval handler recommendation (prior Nice-to-have #3) to be elevated to a Should-fix.
|
Follow-up for the two linked CodeRabbit comments: Addressed the actionable findings in commit eae12b1. Actions taken:
Evaluated as intentional/no action:
Verification after the follow-up:
Note: local npm run test was twice terminated by the shell tool with signal 15 before completion, but progressed through multiple packages with passing output; the impacted targeted suites above passed and CI will run the full matrix on this pushed commit. |
|
@CodeRabbit consider these from another agent: Potential Issues I Spotted###1. Potential Race: scheduleTask Fallback for Empty Completion (AgenticLoop.ts) In scheduleAndAwait(), the code resolves completion with [] if scheduler.schedule() finishes without any acceptedToolUpdateSeen: `typescriptconst scheduleTask = scheduler .schedule(requests, signal) However, coreToolScheduler.ts was changed to await this.checkAndNotifyCompletion(). If schedule() now resolves after onAllToolCallsComplete fires (which it should, since the await was added), then acceptedToolUpdateSeen will be true and the fallback won't trigger. But if there's a path where onAllToolCallsComplete fires but the tool update callback hasn't set acceptedToolUpdateSeen yet (e.g. a tool that validates-and-deny-s immediately without ever entering the update path), the fallback resolveCompletion([]) is the safety net — which seems intentional and correct. ###2. completionPromise vs completionTask Confusion (AgenticLoop.ts) There are two promises:
The drainWhileRunning watches completionTask for the state.completionSettled flag, but resolveCompletion resolves completionPromise (not completionTask). Since completionTask is chained from completionPromise, the resolution propagates. This looks correct but is subtle — the .finally() on completionTask is what sets completionSettled, and since completionPromise never rejects, the .catch() is dead code (defensive). ###3. EventQueue.waitForNext — No Spurious Wakeup Guard After AwaitIn drainWhileRunning: `typescriptawait Promise.race([completionTask, queue.waitForNext(signal)]); After waitForNext resolves (due to a push), flushBuffered drains. But after completionTask resolves, state.completionSettled is set in the .finally() microtask. There's a theoretical window where Promise.race resolves on completionTask but state.completionSettled hasn't been set yet by the .finally() handler (finally runs in a separate microtask). This would cause the while (!state.completionSettled) check to loop one more iteration — but flushBuffered would be a no-op (queue empty), and waitForNext would return immediately (since closed or buffered.length >0 might not be true yet). Actually, the loop condition checks state.completionSettled at the top, and the .finally() runs before .then() handlers, so by the time Promise.race resolves, the finally should have run. This is likely fine but is subtle. ###4. buildNextMessage is async but Has No await (AgenticLoop.ts) `typescriptprivate async buildNextMessage(completed: CompletedToolCall[]): Promise<{...}> It calls recordCancelledToolHistory which is async and awaited — wait, actually the await is inside the if branch for all-cancelled. The function is async for that reason. This is fine. ###5. Double Tool Group Display in CLILooking at the flow: the engine loop emits tools_complete → useAgenticLoop.routeLoopEvent → handleToolsComplete → addItem(mapTrackedToolCallsToDisplay(completed), ...).But also: the CLI's useReactToolScheduler has its own onAllToolCallsComplete callback (via createMainSchedulerCallbacks) that calls processPrimaryCompletion → addItem(mapTrackedToolCallsToDisplay(...)). However, the AgenticLoop uses its own scheduler session ID (schedulerSessionId), not the CLI main scheduler's session ID. So the loop's scheduler callbacks fire on the loop's scheduler, and the CLI main scheduler's onAllToolCallsComplete is separate. The display callbacks (onToolCallsUpdate, outputUpdateHandler) are forwarded to the CLI's replaceToolCalls/updateToolOutput which use the main scheduler ID's symbol.So: tool updates flow to the correct display state via the forwarded callbacks, and tools_complete events trigger addItem in the loop's event handler. The CLI main scheduler's own onAllToolCallsComplete should NOT fire for loop-scheduled tools because they use a different scheduler instance. This seems correct but the dual-path is fragile — if someone accidentally uses the same session ID, you'd get double display items. ###6. responseSubmittedToGemini Flag is Now Dead on the Loop PathThe TrackedToolCall types in useReactToolScheduler.ts still carry responseSubmittedToGemini, but with the engine loop owning continuation, this flag is no longer used for the primary flow. The markToolsAsSubmitted function still sets it for external/subagent tools (display-only clearing), which seems correct. But it's vestigial for the main continuation path. Not a bug, but technical debt. ###7. useSubmitQuery.ts Still Passes scheduleToolCalls Through DepsuseStreamEventHandlers still receives scheduleToolCalls in its deps and passes it to dispatchStreamEvent → StreamEventDeps.scheduleToolCalls. But looking at streamEventDispatcher.ts, the ToolCallRequest case just returns { geminiMessageBuffer } — it doesn't call scheduleToolCalls. The scheduler callback is in the deps but appears unused in the dispatcher itself. It may still be used by the queryPreparer.ts path. This is not a bug but the scheduleToolCalls wiring through StreamEventDeps is potentially unnecessary now. ###8. Deleted Watchdog Test (863 lines) The file useStreamEventHandlers.watchdog.test.ts was deleted entirely (863 lines). This presumably tested the old CLI-owned watchdog/streaming flow that was replaced by the engine loop. If any watchdog behavior was supposed to be preserved, its test coverage needs to exist somewhere in the new test files. The new agenticLoop.integration.test.ts (2094 lines) and useAgenticLoop.test.tsx (584 lines) provide substantial coverage, but I can't verify all863 lines of watchdog scenarios are covered without comparing test cases. ###9. testBoundaryConfig in Integration Tests Uses as Config CastThe integration test creates a partial Config fixture and casts it via fixture as Config. The comment justifies this (Config is a large class), but this means if Config gains new methods that the loop calls, the tests won't catch the mismatch at compile time — they'd fail at runtime instead. ###10. Minor: void result.events in runTurn `typescriptconst result = yield* this.scheduleAndAwait(dedupedRequests, signal); The events field of TurnToolResult is the EventQueue, which has already been drained and closed by the time scheduleAndAwait returns. The void result.events is explicitly discarding it, which is fine but slightly odd — it suggests the return type could just omit events. SummaryThe branch is a well-structured refactoring with solid architectural decisions. The agentic loop consolidation is the centerpiece — moving multi-turn logic from CLI hooks into the engine with proper injection points. The bug fixes (#2035 OAuth loop, #2045 compression retry, #2040 context limit) are focused and well-documented. The cancellation hardening commits show iterative attention to race conditions. The main risk areas are the subtle async coordination in scheduleAndAwait/EventQueue/drainWhileRunning, the dual-scheduler display path, and ensuring the deleted watchdog test coverage is fully replaced. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Issue `#1`, `#2`, `#3`: Get current scheduleAndAwait, drainWhileRunning, completionPromise/completionTask from AgenticLoop.ts
fd -e ts 'AgenticLoop.ts' --exclude '__tests__' | xargs cat | grep -n "" | sed -n '1,650p'
echo "=== TurnToolResult type ==="
fd -e ts 'AgenticLoop.ts' --exclude '__tests__' | xargs grep -n "TurnToolResult\|result\.events\|void result"
echo "=== SEPARATOR - queryPreparer ==="
fd -e ts 'queryPreparer.ts' --exclude '.test.' | xargs grep -n "scheduleToolCalls" | head -30
echo "=== SEPARATOR - StreamEventDeps scheduleToolCalls ==="
fd -e ts 'streamEventDispatcher.ts' --exclude '.test.' | xargs grep -n "scheduleToolCalls\|ToolCallRequest" | head -30
echo "=== SEPARATOR - useSubmitQuery scheduleToolCalls wiring ==="
fd -e ts 'useSubmitQuery.ts' --exclude '.test.' | xargs grep -n "scheduleToolCalls" | head -20
echo "=== SEPARATOR - watchdog test deleted, check if behavior covered elsewhere ==="
# Check if idle timeout / watchdog is covered in new test files
fd -e ts -e tsx '__tests__' | xargs grep -l "watchdog\|idle.*timeout\|LLXPRT_STREAM_IDLE_TIMEOUT\|stall\|AgentExecutionStopped\|AgentExecutionBlocked" 2>/dev/null | head -10
echo "=== SEPARATOR - scheduleToolCalls in StreamEventDeps type ==="
fd -e ts 'streamEventDispatcher.ts' --exclude '.test.' | xargs cat | head -80Length of output: 30527 🏁 Script executed: #!/bin/bash
# Check scheduleAndAwait current code after the follow-up commit fixes
fd -e ts 'AgenticLoop.ts' --exclude '__tests__' | xargs cat | grep -n "" | sed -n '380,600p'
echo "=== SEPARATOR - check TurnToolResult and void result.events ==="
fd -e ts 'AgenticLoop.ts' --exclude '__tests__' | xargs grep -n -A3 -B3 "TurnToolResult\|void result\|result\.events"Length of output: 11629
Issue
|
| # | Verdict |
|---|---|
#1 |
✅ Self-corrected — intentional safety net |
#2 |
✅ Self-corrected — defensive dead code, harmless |
#3 |
✅ Already fixed by eae12b1ed — drain step present at line 538 |
#4 |
✅ Self-corrected — async needed for inner await |
#5 |
|
#6 |
📝 Tech debt — responseSubmittedToGemini vestigial on primary path |
#7 |
✅ Not dead — queryPreparer.ts still consumes scheduleToolCalls |
#8 |
🔴 Real gap — idle-timeout/watchdog test coverage not replaced in new suite |
#9 |
as Config cast is a known tradeoff |
#10 |
📝 Trivial — events field in TurnToolResult can be removed |
The only genuinely new actionable finding is #8: the deleted watchdog test suite's StreamIdleTimeout and environment-config scenarios have no coverage in the new integration suite. Would you like me to open a tracking issue for that gap, and/or for the #10 TurnToolResult.events cleanup?
|
Follow-up for #2050 (comment): Actions taken:
Evaluated and not changed here:
Verification for the new cleanup:
|
1 participant
Summary
fixes #2034
Verification
Note: workspace-level npm run test repeatedly terminated the providers workspace under npm orchestration with exit code 143, while the direct packages/providers suite passed. I found no implementation-related failure in that direct run.