fix(task-lifecycle): preserve parent-child link when delegated subtask is interrupted (#560)#787
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds ChangesInterrupted status lifecycle
React.useContext migration
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 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 |
edelauna
changed the title
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
p12tic
approved these changes
Jul 3, 2026
…child cancellation
edelauna
requested review from
JamesRobert20,
hannesrudolph,
navedmerchant and
taltas
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/webview/ClineProvider.ts (1)
3283-3309: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winKeep rehydrate on cancel-persistence failure
throw historyErrorexitscancelTaskInternal()beforecreateTaskWithHistoryItem(...), so a double failure can leave the task stack empty until reload. Log the error and fall through with the in-memory severedhistoryIteminstead.🤖 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 3283 - 3309, The cancel persistence flow in ClineProvider’s cancelTaskInternal is rethrowing historyError from updateTaskHistory, which aborts the rest of the recovery path and can leave the task stack empty; change the catch around updateTaskHistory so it logs the failure and continues using the already severed in-memory historyItem instead of throwing. Keep the parentTask/rootTask clearing and this.cancelledDelegationChildIds update, then allow the later createTaskWithHistoryItem path to run even when persistence fails.
🧹 Nitpick comments (5)
webview-ui/src/context/__tests__/ExtensionStateContext.spec.tsx (1)
250-258: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSpying on
React.useContextcouples the test to an implementation detail.Mocking
React.useContextdirectly works here becauseuseExtensionStatenow calls it via the namespace import, but this ties the test to that specific call style — if the hook reverts to a destructureduseContextimport, this mock silently stops exercising the intended path (the spy would still fire without error, sinceReact.useContextis still a valid reference, but coupling remains implicit). Consider retaining/adding a render-based test (component rendered outside the provider) alongside or instead of this spy-based approach for a more implementation-agnostic guarantee.🤖 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 `@webview-ui/src/context/__tests__/ExtensionStateContext.spec.tsx` around lines 250 - 258, The `useExtensionState` test is too tightly coupled to the hook’s current `React.useContext` call style; instead of relying only on a direct spy, add or keep a render-based test that renders a component using `useExtensionState` outside `ExtensionStateContextProvider` and asserts the thrown error, so the coverage stays valid even if the hook’s import style changes.apps/vscode-e2e/src/suite/subtasks.test.ts (1)
75-75: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider replacing the fixed
sleep(1_500)with a condition-basedwaitFor.The suite already uses
waitFor-style polling elsewhere (e.g. line 372/564 waiting ongetTaskApiConversationHistoryLength); a hard-coded sleep here is more prone to flakiness under slow CI and adds fixed latency regardless of actual readiness.🤖 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` at line 75, Replace the fixed sleep in the test flow with condition-based polling using waitFor. In subtasks.test.ts, locate the await sleep(1_500) call and switch it to a waitFor pattern consistent with the existing getTaskApiConversationHistoryLength checks so the test proceeds only when the expected state is ready. Keep the same surrounding test logic, but poll for the specific readiness condition instead of relying on a hard-coded delay.apps/vscode-e2e/src/fixtures/subtasks.ts (1)
39-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated 3-tier fallback matching logic between
completionAfterAnswerand the new interrupt fixture.The new fixture at lines 316-340 re-implements the same "structured tool-result / call-id+answer / bare user-message" fallback chain already encapsulated in
completionAfterAnswer(39-63), just against different marker/answer constants. GeneralizingcompletionAfterAnswerto accept a marker/answer/exclusion list would let this new scenario reuse it instead of duplicating the logic.♻️ Suggested generalization
-const completionAfterAnswer = (followupId: string, completionId: string) => ({ - match: { - predicate: (req: ChatCompletionRequest) => - !requestContains(req, [SUBTASK_INTERRUPT_CHILD_MARKER]) && - !requestContains(req, [SUBTASK_INTERRUPT_PARENT_MARKER]) && - (toolResultContains(req, followupId, [SUBTASK_CHILD_FOLLOWUP_ANSWER]) || - requestContains(req, [followupId, SUBTASK_CHILD_FOLLOWUP_ANSWER]) || - requestContains(req, [ - SUBTASK_CHILD_MARKER, - `<user_message>\\n${SUBTASK_CHILD_FOLLOWUP_ANSWER}\\n</user_message>`, - ])), - }, - response: { - toolCalls: [{ name: "attempt_completion", arguments: JSON.stringify({ result: "9" }), id: completionId }], - }, -}) +const completionAfterAnswer = ( + followupId: string, + completionId: string, + opts?: { marker?: string; answer?: string; excludeMarkers?: string[] }, +) => { + const marker = opts?.marker ?? SUBTASK_CHILD_MARKER + const answer = opts?.answer ?? SUBTASK_CHILD_FOLLOWUP_ANSWER + const excludeMarkers = opts?.excludeMarkers ?? [SUBTASK_INTERRUPT_CHILD_MARKER, SUBTASK_INTERRUPT_PARENT_MARKER] + return { + match: { + predicate: (req: ChatCompletionRequest) => + excludeMarkers.every((m) => !requestContains(req, [m])) && + (toolResultContains(req, followupId, [answer]) || + requestContains(req, [followupId, answer]) || + requestContains(req, [marker, `<user_message>\\n${answer}\\n</user_message>`])), + }, + response: { + toolCalls: [{ name: "attempt_completion", arguments: JSON.stringify({ result: answer }), id: completionId }], + }, + } +}Also applies to: 316-340
🤖 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/fixtures/subtasks.ts` around lines 39 - 63, The fallback matching logic for completion responses is duplicated between completionAfterAnswer and the new interrupt fixture, so refactor completionAfterAnswer to accept the marker, answer token, and exclusion markers as parameters. Update the predicate in completionAfterAnswer to work with the caller-provided constants while preserving the same three-tier match order, then reuse it in the interrupt fixture instead of re-implementing the structured tool-result, call-id+answer, and bare user-message checks.apps/cli/src/ui/components/autocomplete/triggers/HistoryTrigger.tsx (1)
136-151: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a status→{icon,color} lookup instead of nested ternaries.
Functionally correct, but the duplicated
item.status === Xchains for icon and color are getting hard to scan with 4 branches. A small lookup map would be more maintainable for future status additions.♻️ Suggested refactor
+const STATUS_DISPLAY: Record<string, { icon: string; color: string }> = { + completed: { icon: "✓", color: "green" }, + active: { icon: "●", color: "yellow" }, + interrupted: { icon: "⏸", color: "cyan" }, +} + renderItem: (item: HistoryResult, isSelected: boolean) => { - // Status indicator - const statusIcon = - item.status === "completed" - ? "✓" - : item.status === "active" - ? "●" - : item.status === "interrupted" - ? "⏸" - : "○" - const statusColor = - item.status === "completed" - ? "green" - : item.status === "active" - ? "yellow" - : item.status === "interrupted" - ? "cyan" - : "gray" + // Status indicator + const { icon: statusIcon, color: statusColor } = STATUS_DISPLAY[item.status ?? ""] ?? { + icon: "○", + color: "gray", + }🤖 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/cli/src/ui/components/autocomplete/triggers/HistoryTrigger.tsx` around lines 136 - 151, The `HistoryTrigger` status rendering logic uses duplicated nested ternaries for `item.status` to derive `statusIcon` and `statusColor`, which is hard to extend. Refactor this block into a single status-to-presentation lookup in `HistoryTrigger.tsx` so both values come from one mapping keyed by status, and keep the fallback behavior for unknown statuses.src/core/webview/ClineProvider.ts (1)
3271-3285: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider guarding the "interrupted" write with
assertValidTransition.Every other status-changing write in this file (line 567, 3583, 3834, 3839) calls
assertValidTransitionbefore persisting. This newhistoryItem.status = "interrupted"write skips that check. TheawaitingChildId === task.taskIdguard mitigates most invalid-transition risk (e.g., a completed child already clearsawaitingChildId), but adding the explicit guard would keep this write consistent with the rest of the file's defensive pattern and catch any future edge case.♻️ Suggested addition
if (parentHistory?.status === "delegated" && parentHistory?.awaitingChildId === task.taskId) { + assertValidTransition(historyItem!.status, "interrupted") historyItem = { ...historyItem!, status: "interrupted" } await this.updateTaskHistory(historyItem)🤖 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 3271 - 3285, The new interrupted status write in ClineProvider’s cancel flow should be guarded the same way as the other status updates in this file. Before setting historyItem.status to "interrupted" and calling updateTaskHistory, add an assertValidTransition check using the current historyItem status and the target interrupted state so this path matches the defensive pattern used elsewhere in ClineProvider and catches any future invalid transition edge cases.
🤖 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 316-356: The fixtures in subtasks.ts are hardcoding result strings
that already exist as exported constants, which can drift from the test
expectations. Update the two attempt_completion responses in the mock.addFixture
blocks to reuse SUBTASK_INTERRUPT_CHILD_FOLLOWUP_ANSWER and
SUBTASK_INTERRUPT_PARENT_RESULT instead of literal values, keeping the fixture
responses aligned with subtasks.test.ts. Use the existing constants already
referenced in this file to locate the affected toolCalls payloads.
In `@packages/types/src/api.ts`:
- Around line 48-53: The contract for getTaskApiConversationHistoryLength is not
fully honored because the implementation in ApiService does not fall back to 0
when the history is unavailable. Update the getTaskApiConversationHistoryLength
path in src/extension/api.ts so it returns the persisted count when present and
explicitly returns 0 for missing/undefined/unavailable task history, matching
the JSDoc in the API interface.
In `@src/extension/api.ts`:
- Around line 243-247: getTaskApiConversationHistoryLength currently calls
getTaskWithId directly, so missing tasks throw instead of honoring the
documented 0 fallback. Update the getTaskApiConversationHistoryLength method in
src/extension/api.ts to match the same try/catch pattern used by
isTaskInHistory, and return 0 when getTaskWithId fails or the task is
unavailable. Keep the existing apiConversationHistory.length path for valid
tasks so the method matches the contract in packages/types/src/api.ts.
---
Outside diff comments:
In `@src/core/webview/ClineProvider.ts`:
- Around line 3283-3309: The cancel persistence flow in ClineProvider’s
cancelTaskInternal is rethrowing historyError from updateTaskHistory, which
aborts the rest of the recovery path and can leave the task stack empty; change
the catch around updateTaskHistory so it logs the failure and continues using
the already severed in-memory historyItem instead of throwing. Keep the
parentTask/rootTask clearing and this.cancelledDelegationChildIds update, then
allow the later createTaskWithHistoryItem path to run even when persistence
fails.
---
Nitpick comments:
In `@apps/cli/src/ui/components/autocomplete/triggers/HistoryTrigger.tsx`:
- Around line 136-151: The `HistoryTrigger` status rendering logic uses
duplicated nested ternaries for `item.status` to derive `statusIcon` and
`statusColor`, which is hard to extend. Refactor this block into a single
status-to-presentation lookup in `HistoryTrigger.tsx` so both values come from
one mapping keyed by status, and keep the fallback behavior for unknown
statuses.
In `@apps/vscode-e2e/src/fixtures/subtasks.ts`:
- Around line 39-63: The fallback matching logic for completion responses is
duplicated between completionAfterAnswer and the new interrupt fixture, so
refactor completionAfterAnswer to accept the marker, answer token, and exclusion
markers as parameters. Update the predicate in completionAfterAnswer to work
with the caller-provided constants while preserving the same three-tier match
order, then reuse it in the interrupt fixture instead of re-implementing the
structured tool-result, call-id+answer, and bare user-message checks.
In `@apps/vscode-e2e/src/suite/subtasks.test.ts`:
- Line 75: Replace the fixed sleep in the test flow with condition-based polling
using waitFor. In subtasks.test.ts, locate the await sleep(1_500) call and
switch it to a waitFor pattern consistent with the existing
getTaskApiConversationHistoryLength checks so the test proceeds only when the
expected state is ready. Keep the same surrounding test logic, but poll for the
specific readiness condition instead of relying on a hard-coded delay.
In `@src/core/webview/ClineProvider.ts`:
- Around line 3271-3285: The new interrupted status write in ClineProvider’s
cancel flow should be guarded the same way as the other status updates in this
file. Before setting historyItem.status to "interrupted" and calling
updateTaskHistory, add an assertValidTransition check using the current
historyItem status and the target interrupted state so this path matches the
defensive pattern used elsewhere in ClineProvider and catches any future invalid
transition edge cases.
In `@webview-ui/src/context/__tests__/ExtensionStateContext.spec.tsx`:
- Around line 250-258: The `useExtensionState` test is too tightly coupled to
the hook’s current `React.useContext` call style; instead of relying only on a
direct spy, add or keep a render-based test that renders a component using
`useExtensionState` outside `ExtensionStateContextProvider` and asserts the
thrown error, so the coverage stays valid even if the hook’s import style
changes.
🪄 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: 47b46968-67b0-4887-89e5-481bf53a8f18
📒 Files selected for processing (24)
apps/cli/src/ui/components/autocomplete/triggers/HistoryTrigger.tsxapps/cli/src/ui/components/autocomplete/triggers/__tests__/HistoryTrigger.test.tsxapps/cli/src/ui/types.tsapps/vscode-e2e/src/fixtures/subtasks.tsapps/vscode-e2e/src/suite/subtasks.test.tspackages/core/src/task-history/index.tspackages/types/src/api.tspackages/types/src/history.tspackages/types/src/task.tssrc/__tests__/helpers/provider-stub.tssrc/__tests__/removeClineFromStack-delegation.spec.tssrc/core/task-persistence/TaskHistoryStore.tssrc/core/task-persistence/__tests__/TaskHistoryStore.reconciliation.spec.tssrc/core/task-persistence/taskMetadata.tssrc/core/task/Task.tssrc/core/task/__tests__/Task.persistence.spec.tssrc/core/tools/AttemptCompletionTool.tssrc/core/tools/__tests__/attemptCompletionTool.spec.tssrc/core/webview/ClineProvider.tssrc/core/webview/__tests__/ClineProvider.flicker-free-cancel.spec.tssrc/core/webview/__tests__/ClineProvider.sticky-profile.spec.tssrc/extension/api.tswebview-ui/src/context/ExtensionStateContext.tsxwebview-ui/src/context/__tests__/ExtensionStateContext.spec.tsx
github-actions
Bot
added
the
awaiting-review
github-actions
Bot
removed
the
awaiting-review
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/extension/__tests__/api-task-conversation-history-length.spec.ts (1)
32-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a test for missing/empty
apiConversationHistory.Current coverage handles the resolved-with-history and rejected cases, but not a resolved task lacking
apiConversationHistory(e.g.undefinedor[]). Adding this would tighten coverage of the error/edge-handling path per the coding guideline for spec files.As per coding guidelines,
**/*.{test,spec}.{ts,tsx,js}should "Use package-local unit tests for pure logic, parsing, state transitions, validation, serialization, request construction, retry decisions, and error handling."🤖 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/extension/__tests__/api-task-conversation-history-length.spec.ts` around lines 32 - 44, Add a spec in api-task-conversation-history-length.spec.ts for the missing-edge case in getTaskApiConversationHistoryLength: when getTaskWithId resolves to a task whose apiConversationHistory is undefined or an empty array, the method should return 0 instead of throwing or miscounting. Reuse the existing mockGetTaskWithId and api.getTaskApiConversationHistoryLength setup to cover this resolved-but-empty path alongside the current history and rejection tests.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.
Nitpick comments:
In `@src/extension/__tests__/api-task-conversation-history-length.spec.ts`:
- Around line 32-44: Add a spec in api-task-conversation-history-length.spec.ts
for the missing-edge case in getTaskApiConversationHistoryLength: when
getTaskWithId resolves to a task whose apiConversationHistory is undefined or an
empty array, the method should return 0 instead of throwing or miscounting.
Reuse the existing mockGetTaskWithId and api.getTaskApiConversationHistoryLength
setup to cover this resolved-but-empty path alongside the current history and
rejection tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3e5da1eb-a36a-4ac3-9d51-98089f111e3a
📒 Files selected for processing (3)
apps/vscode-e2e/src/fixtures/subtasks.tssrc/extension/__tests__/api-task-conversation-history-length.spec.tssrc/extension/api.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/extension/api.ts
- apps/vscode-e2e/src/fixtures/subtasks.ts
github-actions
Bot
added
the
awaiting-review
navedmerchant
previously approved these changes
Jul 5, 2026
github-actions
Bot
removed
the
awaiting-review
navedmerchant
approved these changes
Jul 6, 2026
github-actions
Bot
added
the
awaiting-review
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related GitHub Issue
Closes: #560
Description
When a delegated subtask was cancelled mid-execution,
cancelTask()immediately severed the parent–child link — parent transitioned from"delegated"→"active"withawaitingChildIdcleared. When the user resumed the child and it calledattempt_completion, the parent no longer awaited it, so it fell through to the standalone "Start New Task" flow instead of reporting back.How it works
Changes
Core fix:
packages/types/src/history.ts— Add"interrupted"toHistoryItemstatus enumpackages/types/src/task.ts,src/core/task/Task.ts,src/core/task-persistence/taskMetadata.ts— Propagate"interrupted"throughinitialStatustypessrc/core/webview/ClineProvider.ts#cancelTask— Mark cancelled delegated child as"interrupted"instead of severing parent link; awaitabortTask()promise before writing"interrupted"to prevent a latesaveClineMessageswrite from downgrading the status back to"active"src/core/webview/ClineProvider.ts#removeClineFromStack— Skip parent auto-repair when child history shows"interrupted"(parent must stay"delegated"for resume)src/core/tools/AttemptCompletionTool.ts— Accept"interrupted"alongside"active"so resumed children can delegate back to parentCLI/session type sync:
packages/core/src/task-history/index.ts— Add"interrupted"to session reader allow-list so persisted interrupted sessions are not silently droppedapps/cli/src/ui/types.ts,apps/cli/src/ui/components/autocomplete/triggers/HistoryTrigger.tsx— Add"interrupted"to status unions; display interrupted tasks with ⏸ icon and cyan colorTest Procedure
Unit tests (3 files updated, 3 new tests added):
src/core/webview/__tests__/ClineProvider.flicker-free-cancel.spec.ts— Updated existing cancel tests; added tests asserting child is marked"interrupted"and parent stays"delegated"src/__tests__/removeClineFromStack-delegation.spec.ts— Added test asserting parent is not auto-repaired when child is"interrupted"src/core/tools/__tests__/attemptCompletionTool.spec.ts— Added test for interrupted child delegating back to parentapps/cli/src/ui/components/autocomplete/triggers/__tests__/HistoryTrigger.test.tsx— Added test asserting ⏸ icon renders for interrupted statusE2E test:
apps/vscode-e2e/src/suite/subtasks.test.ts— New test: interrupt child mid-execution → resume → asserts child reports back to parent and parent completes with expected resultAll unit tests pass:
pnpm vitest run(408/408 files).Pre-Submission Checklist
Summary by CodeRabbit