Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (2)
src/core/webview/ClineProvider.ts (2)
1531-1601: 🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift
targetTask=nulldoesn't protect the parent's API config, only its_taskMode.The doc for the new
targetTaskparam promises that passingnullwill "skip the per-task mutation and only apply the global mode/API-config side effects... without stomping the still-running parent's own_taskMode." But thetask/targetTaskgate only wraps the_taskMode+ task-history mutation (theif (task) {...}block). The subsequentactivateProviderProfile({ name: profile.name })call (further down in this same function, when the new mode has a saved API config) is unconditional and internally rebuilds whichever taskthis.getCurrentTask()returns viaupdateTaskApiHandlerIfNeeded.In the fan-out delegation path (
delegateParentAndOpenChild),handleModeSwitch(requestedMode, null)runs before the child exists and before the parent is removed from the registry — sogetCurrentTask()at that point is still the parent. If the child's requested mode has a different saved/sticky API config than the parent's, this silently rebuilds and swaps the running parent'sapiConfiguration/API handler to the child's mode's provider settings mid-task, potentially rerouting the parent's next request to an unintended provider/model/key.Fix direction: thread an explicit target (or "skip current-task rebuild") flag through
activateProviderProfile/updateTaskApiHandlerIfNeededinstead of relying ongetCurrentTask().🐛 Sketch of one possible fix
- if (hasActualSettings) { - await this.activateProviderProfile({ name: profile.name }) - } else { + if (hasActualSettings) { + await this.activateProviderProfile({ name: profile.name }, { skipCurrentTaskApiRebuild: targetTask === null }) + } else { // The task will continue with the current/default configuration. }and in
activateProviderProfile/updateTaskApiHandlerIfNeeded, skip thegetCurrentTask()rebuild when that flag is set.🤖 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 `@src/core/webview/ClineProvider.ts` around lines 1531 - 1601, Update handleModeSwitch and the activateProviderProfile/updateTaskApiHandlerIfNeeded flow so passing targetTask=null also skips rebuilding the current task’s API configuration and handler. Thread an explicit skip-current-task-rebuild or target-task control through these methods, ensuring delegation fan-out applies global mode/config side effects without using getCurrentTask() to mutate the still-running parent.
3666-3703: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winReserved permit (and parent) can be lost if
createTask()throws.
childReservedReleaseis captured at line 3627, but the only cleanup that releases it (fan-out) or restores the parent (non-fan-out) lives in thecatchwrappingatomicReadAndUpdatestarting at line 3703 — which only runs for errors aftercreateTask()returns. ThecreateTask()call itself (3679-3683) is not wrapped in try/catch, yet it has real unguarded throw paths (OrganizationAllowListViolationError,Taskconstructor validation,addClineToStack()/getState()failures).If it throws:
- Fan-out case:
childReservedReleaseis never released — a permanent concurrency-permit leak for the life of thisTaskSchedulerinstance.- Non-fan-out case: the parent was already evicted in step 3 and is never restored — the user is left with no active task.
🐛 Proposed fix
- const child = await this.createTask(message, undefined, parent as any, { - initialTodos, - initialStatus: "active", - startTask: false, - }) + let child: Task + try { + child = await this.createTask(message, undefined, parent as any, { + initialTodos, + initialStatus: "active", + startTask: false, + }) + } catch (err) { + // createTask() can throw before any cleanup below runs — release the + // reserved permit (fan-out) so it doesn't leak, and let the caller see the error. + childReservedRelease?.() + throw err + }Note: the non-fan-out parent-restore case still needs equivalent handling here.
🤖 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 `@src/core/webview/ClineProvider.ts` around lines 3666 - 3703, Wrap the createTask call in the same failure-handling flow as atomicReadAndUpdate so any exception during child creation triggers cleanup. In the catch path, invoke childReservedRelease for fan-out and restore the evicted parent for non-fan-out, while preserving existing post-creation rollback behavior and avoiding duplicate cleanup.
🧹 Nitpick comments (1)
src/core/task/__tests__/delegation-concurrent.spec.ts (1)
136-163: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFragile microtask-count synchronization instead of polling a deterministic signal.
Both tests occupy scheduler permits with never-resolving
run()functions and then assumesem.acquire()has settled after a fixed number ofawait Promise.resolve()calls (2 ticks at Lines 149-150; 1 tick at Line 207, then 2 more at Lines 219-220). This hardcodes an implementation detail (exact microtask depth ofsem.acquire()); ifTaskSemaphore's internal promise chaining ever grows by one hop, these tests could start failing/flaking without a clear signal why. The atomic-reservation test at Lines 270-273 already demonstrates a more robust alternative: pollingscheduler.availableuntil it reflects the expected state.♻️ Suggested pattern for deterministic settling
- void scheduler.schedule(makeParent({ taskId: "occupant-1" }), () => new Promise<void>(() => {})) - void scheduler.schedule(makeParent({ taskId: "occupant-2" }), () => new Promise<void>(() => {})) - // Let both schedule() calls' internal sem.acquire() microtasks settle so - // both permits are actually held before we check availability. - await Promise.resolve() - await Promise.resolve() + void scheduler.schedule(makeParent({ taskId: "occupant-1" }), () => new Promise<void>(() => {})) + void scheduler.schedule(makeParent({ taskId: "occupant-2" }), () => new Promise<void>(() => {})) + // Poll the deterministic signal instead of assuming a fixed microtask depth. + while (scheduler.available > 0) { + await Promise.resolve() + }Also applies to: 185-227
🤖 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 `@src/core/task/__tests__/delegation-concurrent.spec.ts` around lines 136 - 163, Replace the fixed Promise.resolve() microtask waits in the permit-occupancy tests around callDelegate and the related concurrency cases with polling of scheduler.available until it reaches the expected exhausted state, matching the deterministic approach used by the atomic-reservation test. Keep the never-resolving occupancy tasks and assertions unchanged.
🤖 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 `@src/core/webview/ClineProvider.ts`:
- Around line 1531-1601: Update handleModeSwitch and the
activateProviderProfile/updateTaskApiHandlerIfNeeded flow so passing
targetTask=null also skips rebuilding the current task’s API configuration and
handler. Thread an explicit skip-current-task-rebuild or target-task control
through these methods, ensuring delegation fan-out applies global mode/config
side effects without using getCurrentTask() to mutate the still-running parent.
- Around line 3666-3703: Wrap the createTask call in the same failure-handling
flow as atomicReadAndUpdate so any exception during child creation triggers
cleanup. In the catch path, invoke childReservedRelease for fan-out and restore
the evicted parent for non-fan-out, while preserving existing post-creation
rollback behavior and avoiding duplicate cleanup.
---
Nitpick comments:
In `@src/core/task/__tests__/delegation-concurrent.spec.ts`:
- Around line 136-163: Replace the fixed Promise.resolve() microtask waits in
the permit-occupancy tests around callDelegate and the related concurrency cases
with polling of scheduler.available until it reaches the expected exhausted
state, matching the deterministic approach used by the atomic-reservation test.
Keep the never-resolving occupancy tasks and assertions unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 038f0437-f573-41dc-b617-b71c4911555b
📒 Files selected for processing (10)
apps/vscode-e2e/src/fixtures/subtasks.tsapps/vscode-e2e/src/suite/subtasks.test.tspackages/types/src/api.tssrc/__tests__/provider-delegation.spec.tssrc/core/task/TaskScheduler.tssrc/core/task/__tests__/delegation-concurrent.spec.tssrc/core/webview/ClineProvider.tssrc/eslint-suppressions.jsonsrc/extension/api.tssrc/utils/TaskSemaphore.ts
1 participant
Related GitHub Issue
Closes: #
Description
Test Procedure
Pre-Submission Checklist
*.visual.tsxsnapshot inwebview-ui/. Seewebview-ui/AGENTS.md→ "When a UI change needs a snapshot".Visual Snapshots
Videos (interaction / animation only)
Documentation Updates
Additional Notes
Get in Touch
Summary by CodeRabbit
New Features
Bug Fixes
Tests