Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds concurrent subtask fan-out with scheduler reservations, task-local startup configuration, provider rollback handling, configurable scheduler concurrency, and VS Code E2E coverage for concurrent and cross-profile execution. ChangesConcurrent delegation infrastructure
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
src/core/task/__tests__/delegation-concurrent.spec.ts (1)
251-272: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRestore the
vscode.window.showErrorMessagespy.Unless a global
restoreMocks/beforeEachresets it, this spy leaks into later tests in the file.♻️ Restore after use
expect(showErrorMessage).toHaveBeenCalledWith( "Failed to restore the parent task after subtask creation failed. Reopen the task from history to continue.", ) + showErrorMessage.mockRestore() })🤖 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 251 - 272, Restore the vscode.window.showErrorMessage spy created in the test after its assertions complete, ensuring cleanup runs even when the test fails. Update the test around showErrorMessage and preserve the existing error and call-count assertions.src/core/webview/ClineProvider.ts (2)
213-236: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winA hung profile mutation silently blocks every later mode/profile switch.
The timeout only rejects the caller;
providerProfileMutationQueuestill chains onrun, so if a mutation never settles the queue stalls forever with no signal. Consider logging when the timeout fires (and when the underlying op eventually settles) so this state is diagnosable.♻️ Log on timeout
private withProviderProfileMutationTimeout<T>(operation: Promise<T>): Promise<T> { let timeoutId: ReturnType<typeof setTimeout> | undefined const timeout = new Promise<never>((_, reject) => { timeoutId = setTimeout(() => { + this.log( + `[providerProfileMutation] mutation exceeded ${ClineProvider.PENDING_OPERATION_TIMEOUT_MS}ms; queued mutations remain blocked until it settles`, + ) reject(new Error("Provider profile mutation timed out")) }, ClineProvider.PENDING_OPERATION_TIMEOUT_MS) })🤖 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 213 - 236, Update enqueueProviderProfileMutation and withProviderProfileMutationTimeout so a timeout emits a diagnostic log identifying the hung provider profile mutation, and also log when the underlying operation eventually settles after timing out. Preserve the existing caller rejection, queue chaining, and timeout cleanup behavior.
3645-3679: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated restore block; a small loop reads better.
The two
getTaskWithId+createTaskWithHistoryItemblocks are identical apart from log wording.♻️ Retry loop
- if (!fanOut) { - try { - const { historyItem: parentHistory } = await this.getTaskWithId(parentTaskId) - await this.createTaskWithHistoryItem(parentHistory) - } catch (firstRollbackError) { - ... - } - } else { + if (!fanOut) { + for (let attempt = 1; attempt <= 2; attempt++) { + try { + const { historyItem: parentHistory } = await this.getTaskWithId(parentTaskId) + await this.createTaskWithHistoryItem(parentHistory) + return + } catch (error) { + this.log( + `[delegateParentAndOpenChild] Failed to restore parent ${parentTaskId} during rollback (attempt ${attempt}/2): ${ + (error as Error)?.message ?? String(error) + }`, + ) + } + } + vscode.window.showErrorMessage( + "Failed to restore the parent task after subtask creation failed. Reopen the task from history to continue.", + ) + } else { childReservedRelease?.() }🤖 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 3645 - 3679, Refactor the non-fan-out branch of restoreParentOrReleasePermit into a small bounded retry loop around getTaskWithId and createTaskWithHistoryItem, preserving the initial-attempt and single-retry behavior. Keep distinct logging for the first failure and retry failure, and retain the existing showErrorMessage handling after both attempts fail; leave the fan-out permit release path unchanged.src/__tests__/provider-delegation.spec.ts (1)
9-26: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate private-method binding logic.
bindRestoreParentOrReleasePermitre-implements the sameproto.restoreParentOrReleasePermit.bind(s)pattern already centralized insrc/__tests__/helpers/provider-stub.ts(makeProviderStub). Two copies of this binding logic now need to stay in sync if the private method's signature changes.Consider exporting a shared binder from
provider-stub.ts(or having this spec build its providers throughmakeProviderStub) instead of re-declaring the cast/bind locally.♻️ Possible consolidation
-function bindRestoreParentOrReleasePermit(provider: ClineProvider): void { - type WithRestore = { - restoreParentOrReleasePermit: ( - parentTaskId: string, - fanOut: boolean, - childReservedRelease: (() => void) | undefined, - ) => Promise<void> - } - ;(provider as unknown as WithRestore).restoreParentOrReleasePermit = ( - ClineProvider.prototype as unknown as WithRestore - ).restoreParentOrReleasePermit.bind(provider) -} +// import { bindRestoreParentOrReleasePermit } from "./helpers/provider-stub"Run this to confirm whether the binder already exists/could be exported from
provider-stub.ts:#!/bin/bash rg -n 'restoreParentOrReleasePermit' src/__tests__/helpers/provider-stub.ts src/__tests__/provider-delegation.spec.ts🤖 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/__tests__/provider-delegation.spec.ts` around lines 9 - 26, Remove the local bindRestoreParentOrReleasePermit implementation from provider-delegation.spec.ts and reuse a shared binder exported from makeProviderStub’s provider-stub helper, or construct these providers through makeProviderStub. Ensure restoreParentOrReleasePermit binding and its signature have a single centralized implementation.apps/vscode-e2e/src/suite/subtasks.test.ts (1)
78-97: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAimock journal is read/matched without per-test scoping.
readAimockJournalfetches the full server-side journal on every poll andwaitForAimockRequest/assertAimockRequestmatch against it without filtering to the current test's requests. Since the aimock server likely persists its journal across the whole suite run, this risks matching stale entries from an earlier test if predicates aren't sufficiently unique (here they happen to rely on the newly-addedentry.body.model, which is unique to this test, so the risk is currently mitigated in practice, but the helper itself provides no structural guarantee against pulling in older requests).
As per coding guidelines, "For fetch-interceptor test suites, reset in-memory request/event capture insetup()or allocate a fresh per-test buffer instead of reusing shared mutable state; scope request-shape assertions to the current probe or test tag only; do not pull in older requests."🤖 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 `@apps/vscode-e2e/src/suite/subtasks.test.ts` around lines 78 - 97, Scope Aimock journal assertions to the current test in waitForAimockRequest and assertAimockRequest rather than matching the full persisted journal. Reset or establish a fresh per-test request buffer during setup, and update readAimockJournal or the matching flow to exclude entries from earlier tests while preserving polling behavior.Source: Coding guidelines
🤖 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 `@apps/vscode-e2e/src/suite/subtasks.test.ts`:
- Around line 255-256: Validate the results of both upsertProfile calls for the
parent and child profiles before using them to populate modeApiConfigs. Fail
immediately with a clear error if either result is undefined, and remove the
non-null assertions while preserving the existing successful-profile flow.
In `@src/core/task/__tests__/delegation-concurrent.spec.ts`:
- Around line 148-152: Bound both scheduler polling loops in
delegation-concurrent.spec.ts: lines 148-152 waiting for scheduler.available > 0
and lines 316-318 waiting for scheduler.available > 1. Use the bounded polling
pattern demonstrated by the childStarted loop, preserving the existing await
behavior while ensuring a regression exits the loop and allows the test
assertion to fail.
In `@src/core/task/Task.ts`:
- Around line 3786-3795: The task-local values are only introduced in the shown
scope but shared-state mode and apiConfiguration are still used elsewhere in
attemptApiRequest(). Replace every provider-state read passed to
buildNativeToolsArrayWithRestrictions, metadata.mode, and the Gemini
allowedFunctionNames path with the local await this.getTaskMode() and
this.apiConfiguration values, ensuring delegated requests consistently use the
current Task instance’s configuration.
---
Nitpick comments:
In `@apps/vscode-e2e/src/suite/subtasks.test.ts`:
- Around line 78-97: Scope Aimock journal assertions to the current test in
waitForAimockRequest and assertAimockRequest rather than matching the full
persisted journal. Reset or establish a fresh per-test request buffer during
setup, and update readAimockJournal or the matching flow to exclude entries from
earlier tests while preserving polling behavior.
In `@src/__tests__/provider-delegation.spec.ts`:
- Around line 9-26: Remove the local bindRestoreParentOrReleasePermit
implementation from provider-delegation.spec.ts and reuse a shared binder
exported from makeProviderStub’s provider-stub helper, or construct these
providers through makeProviderStub. Ensure restoreParentOrReleasePermit binding
and its signature have a single centralized implementation.
In `@src/core/task/__tests__/delegation-concurrent.spec.ts`:
- Around line 251-272: Restore the vscode.window.showErrorMessage spy created in
the test after its assertions complete, ensuring cleanup runs even when the test
fails. Update the test around showErrorMessage and preserve the existing error
and call-count assertions.
In `@src/core/webview/ClineProvider.ts`:
- Around line 213-236: Update enqueueProviderProfileMutation and
withProviderProfileMutationTimeout so a timeout emits a diagnostic log
identifying the hung provider profile mutation, and also log when the underlying
operation eventually settles after timing out. Preserve the existing caller
rejection, queue chaining, and timeout cleanup behavior.
- Around line 3645-3679: Refactor the non-fan-out branch of
restoreParentOrReleasePermit into a small bounded retry loop around
getTaskWithId and createTaskWithHistoryItem, preserving the initial-attempt and
single-retry behavior. Keep distinct logging for the first failure and retry
failure, and retain the existing showErrorMessage handling after both attempts
fail; leave the fan-out permit release path unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 844dfb75-f9db-47b2-b90f-4708230169b0
📒 Files selected for processing (15)
apps/vscode-e2e/src/fixtures/subtasks.tsapps/vscode-e2e/src/suite/subtasks.test.tsdocs/formal/task-delegation/README.mddocs/formal/task-delegation/TaskDelegation.cfgdocs/formal/task-delegation/TaskDelegation.tlasrc/__tests__/helpers/provider-stub.tssrc/__tests__/provider-delegation.spec.tssrc/core/task/Task.tssrc/core/task/__tests__/Task.spec.tssrc/core/task/__tests__/delegation-concurrent.spec.tssrc/core/task/__tests__/delegation-state-machine.spec.tssrc/core/webview/ClineProvider.tssrc/core/webview/__tests__/ClineProvider.apiHandlerRebuild.spec.tssrc/eslint-suppressions.jsonsrc/extension/api.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/eslint-suppressions.json
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)
src/core/task/__tests__/Task.spec.ts (1)
462-481: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAssert task-local API configuration in this prompt test.
This test only distinguishes and asserts
mode; it still passes ifgetSystemPrompt()regresses to provider-stateapiConfiguration. Make a prompt option such astodoListEnableddiffer and assert theSYSTEM_PROMPToptions argument uses the task value.As per coding guidelines,
**/*.{test,spec}.{ts,tsx,js}must use package-local unit tests for state transitions and request construction.🤖 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__/Task.spec.ts` around lines 462 - 481, The getSystemPrompt test currently verifies only task-local mode and does not protect task-local apiConfiguration. Update the test around getTaskTestAccess(cline).getSystemPrompt() to give the task a distinct apiConfiguration option such as todoListEnabled, then assert the SYSTEM_PROMPT options argument uses that task value rather than mockProvider state; keep the existing mode assertions intact.Source: Coding guidelines
🤖 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 `@src/core/task/Task.ts`:
- Around line 4030-4033: Scope ProviderProfileChanged handling in
Task.setupProviderProfileChangeListener to the task affected by the profile
change, preventing updates from overwriting unrelated live tasks’
configurations; the request construction around getTaskMode and
this.apiConfiguration should then retain each task’s original metadata. In
src/core/task/Task.ts lines 4030-4033, ensure the request uses the task-isolated
configuration. In src/core/task/__tests__/Task.spec.ts lines 602-651, add a
package-local unit test that creates tasks with distinct configurations, emits a
profile-change event, and verifies the parent request metadata remains
unchanged.
---
Outside diff comments:
In `@src/core/task/__tests__/Task.spec.ts`:
- Around line 462-481: The getSystemPrompt test currently verifies only
task-local mode and does not protect task-local apiConfiguration. Update the
test around getTaskTestAccess(cline).getSystemPrompt() to give the task a
distinct apiConfiguration option such as todoListEnabled, then assert the
SYSTEM_PROMPT options argument uses that task value rather than mockProvider
state; keep the existing mode assertions intact.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fae2acc2-f12d-465b-9003-e1e5bfd14434
📒 Files selected for processing (7)
apps/vscode-e2e/src/suite/subtasks.test.tssrc/__tests__/helpers/provider-stub.tssrc/__tests__/provider-delegation.spec.tssrc/core/task/Task.tssrc/core/task/__tests__/Task.spec.tssrc/core/task/__tests__/delegation-concurrent.spec.tssrc/core/webview/__tests__/ClineProvider.lockApiConfig.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/vscode-e2e/src/suite/subtasks.test.ts
9a810ed to
7cde55d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/core/webview/ClineProvider.ts (1)
3714-3748: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCollapse the duplicated restore attempt into a loop.
The restore body is written twice, once for the first attempt and once for the retry. A short loop removes the duplication and keeps the log messages distinct by attempt index.
♻️ Proposed refactor
if (!fanOut) { - try { - const { historyItem: parentHistory } = await this.getTaskWithId(parentTaskId) - await this.createTaskWithHistoryItem(parentHistory) - } catch (firstRollbackError) { - this.log( - `[delegateParentAndOpenChild] Failed to restore parent ${parentTaskId} during rollback, retrying once: ${ - (firstRollbackError as Error)?.message ?? String(firstRollbackError) - }`, - ) - try { - const { historyItem: parentHistory } = await this.getTaskWithId(parentTaskId) - await this.createTaskWithHistoryItem(parentHistory) - } catch (rollbackError) { - this.log( - `[delegateParentAndOpenChild] Failed to restore parent ${parentTaskId} during rollback retry: ${ - (rollbackError as Error)?.message ?? String(rollbackError) - }`, - ) - vscode.window.showErrorMessage( - "Failed to restore the parent task after subtask creation failed. Reopen the task from history to continue.", - ) - } - } + const maxAttempts = 2 + for (let attempt = 1; attempt <= maxAttempts; attempt++) { + try { + const { historyItem: parentHistory } = await this.getTaskWithId(parentTaskId) + await this.createTaskWithHistoryItem(parentHistory) + return + } catch (rollbackError) { + this.log( + `[delegateParentAndOpenChild] Failed to restore parent ${parentTaskId} during rollback (attempt ${attempt}/${maxAttempts}): ${ + (rollbackError as Error)?.message ?? String(rollbackError) + }`, + ) + } + } + vscode.window.showErrorMessage( + "Failed to restore the parent task after subtask creation failed. Reopen the task from history to continue.", + ) } else {🤖 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 3714 - 3748, Refactor the non-fan-out branch of restoreParentOrReleasePermit to perform the parent restoration in a short two-attempt loop instead of duplicating the getTaskWithId/createTaskWithHistoryItem body. Keep attempt-specific failure logging, retry only once, and preserve the final error message and showErrorMessage behavior after both attempts fail.
🤖 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 `@apps/vscode-e2e/src/fixtures/subtasks.ts`:
- Around line 177-193: Both fan-out child fixtures incorrectly use flat latency,
delaying every response chunk; export a shared SUBTASK_FANOUT_CHILD_DELAY_MS
constant next to SUBTASK_API_HANG_RESPONSE_LATENCY_MS, then replace latency:
15_000 with streamingProfile.ttft using that constant in
apps/vscode-e2e/src/fixtures/subtasks.ts lines 177-193 and 234-250.
In `@src/core/task/Task.ts`:
- Around line 567-572: Update the startupSnapshot branch in Task initialization
so an undefined startupSnapshot.apiConfigName falls back to the existing default
API configuration name before resolving taskApiConfigReady. Preserve explicitly
provided API configuration names and keep behavior aligned with the non-snapshot
initialization path.
---
Nitpick comments:
In `@src/core/webview/ClineProvider.ts`:
- Around line 3714-3748: Refactor the non-fan-out branch of
restoreParentOrReleasePermit to perform the parent restoration in a short
two-attempt loop instead of duplicating the
getTaskWithId/createTaskWithHistoryItem body. Keep attempt-specific failure
logging, retry only once, and preserve the final error message and
showErrorMessage behavior after both attempts fail.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4d95e62e-7390-451b-b293-27aea6ce92ff
📒 Files selected for processing (14)
apps/vscode-e2e/src/fixtures/subtasks.tsapps/vscode-e2e/src/suite/subtasks.test.tssrc/__tests__/helpers/provider-stub.tssrc/__tests__/provider-delegation.spec.tssrc/core/task/Task.tssrc/core/task/TaskScheduler.tssrc/core/task/__tests__/Task.spec.tssrc/core/task/__tests__/TaskScheduler.spec.tssrc/core/task/__tests__/delegation-concurrent.spec.tssrc/core/webview/ClineProvider.tssrc/core/webview/__tests__/ClineProvider.spec.tssrc/eslint-suppressions.jsonsrc/extension/__tests__/api-task-conversation-history-length.spec.tssrc/utils/__tests__/TaskSemaphore.spec.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/vscode-e2e/src/suite/subtasks.test.ts
- src/core/task/TaskScheduler.ts
- src/core/task/tests/delegation-concurrent.spec.ts
github-actions
Bot
added
the
awaiting-review
github-actions
Bot
removed
the
awaiting-review
edelauna
changed the title
github-actions
Bot
added
the
awaiting-review
github-actions
Bot
added
awaiting-review
1 participant
PR 3 of 3 for #369; follows #1085 and #1087. Fan-out is disabled by default and scheduler concurrency is exposed only for extension-host test coverage.
Summary
Testing
Scope
modeApiConfigspersistence work from fix(provider): isolate profile mutations from focused tasks #1087.