fix(delegation): atomically serialize reopenParentFromDelegation

[edelauna]

Related GitHub Issue

Closes: #365

Description

Replaces the sentinel-based multi-step write in reopenParentFromDelegation (parent active → close child → child completed → clear sentinel) with a single atomicUpdatePair call that writes child=completed and parent=active in one lock acquisition — no intermediate state is ever persisted.

Key changes:

• TaskHistoryStore.atomicUpdatePair — new public method that updates two HistoryItems within a single withLock acquisition. Both updaters run before any I/O; both files are written before the lock releases. Extracted upsertCore private helper to share the write+cache logic without triggering onWrite/index-write per record.
• reopenParentFromDelegation steps 3–5 — removeClineFromStack (step 4) moved before the atomic write so any stale "active" status written by abortTask is immediately overwritten. Steps 3, 5, and 5B collapsed into one atomicUpdatePair(childTaskId, parentTaskId, ...) call. Falls back to updateTaskHistory for the parent if atomicUpdatePair fails.
• Tests — atomicUpdatePair unit tests added to TaskHistoryStore.spec.ts (happy path, concurrent upsert ordering, missing-ID throws, partial-failure/known-limitation, onWrite called once). history-resume-delegation.spec.ts updated to stub taskHistoryStore.atomicUpdatePair instead of updateTaskHistory; handoff-correctness and partial-failure tests added. nested-delegation-resume.spec.ts updated with taskHistoryStore stub.

Why child first: a crash after the child write but before the parent write leaves child=completed, parent=delegated — which reconcileDelegationState already handles (resumable). A crash after the parent write but before the child write would leave parent=active with child=active and no way to report back.

Test Procedure

cd src
npx vitest run __tests__/history-resume-delegation.spec.ts \
  __tests__/nested-delegation-resume.spec.ts \
  core/task-persistence/__tests__/TaskHistoryStore.spec.ts

All 6447 tests pass (npx vitest run).

Pre-Submission Checklist

• Issue Linked: This PR is linked to an approved GitHub Issue (see "Related GitHub Issue" above).
• Scope: My changes are focused on the linked issue (one major feature/fix per PR).
• Self-Review: I have performed a thorough self-review of my code.
• Testing: New and/or updated tests have been added to cover my changes (if applicable).
• Documentation Impact: No documentation updates required (internal persistence layer change).
• Contribution Guidelines: I have read and agree to the Contributor Guidelines.

Additional Notes

assertValidTransition referenced in the issue pseudocode is not implemented — the guard at the top of reopenParentFromDelegation (checking status === "delegated" || status === "active" and awaitingChildId === childTaskId) already enforces valid state before the updaters run, and no transition table exists in the codebase to validate against.

Summary by CodeRabbit

• Bug Fixes
  • Improved delegation resume so the related child and parent task history changes are saved together, reducing inconsistent intermediate states after restarts.
  • Enhanced parent reopening behavior to preserve completion details and correctly maintain child/parent relationships.
  • Added stronger safeguards for cancellation, stale handoffs, and cases where reopening should be skipped, including safer abort behavior.
  • Updated webview notifications to reliably reflect the completed child and reopened parent when the view is active.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: ac6b60f4-a73e-41b5-b99e-34f24eba1b56

📥 Commits

Reviewing files that changed from the base of the PR and between b5fb006 and 19346d2.

📒 Files selected for processing (4)

• src/__tests__/history-resume-delegation.spec.ts
• src/core/task-persistence/TaskHistoryStore.ts
• src/core/task-persistence/__tests__/TaskHistoryStore.spec.ts
• src/core/webview/ClineProvider.ts

🚧 Files skipped from review as they are similar to previous changes (1)

• src/core/task-persistence/TaskHistoryStore.ts

---

📝 Walkthrough

Walkthrough

Adds atomicUpdatePair to TaskHistoryStore, rewrites reopenParentFromDelegation to use it, and updates delegation-resume tests to validate atomic child/parent history transitions and related webview behavior.

Atomic delegation handoff

Layer / File(s)	Summary
TaskHistoryStore atomic pair update src/core/task-persistence/TaskHistoryStore.ts	Extracts per-item upsert logic into upsertCore and adds atomicUpdatePair to update two cached history items under one lock.
TaskHistoryStore atomicUpdatePair tests src/core/task-persistence/__tests__/TaskHistoryStore.spec.ts	Adds tests for the paired update path, including concurrency, validation, missing-cache rejection, partial failure, and onWrite behavior.
ClineProvider reopenParentFromDelegation src/core/webview/ClineProvider.ts	Closes the child instance when needed, then uses taskHistoryStore.atomicUpdatePair to complete the child and activate the parent before clearing caches and posting webview updates.
history-resume-delegation coverage src/__tests__/history-resume-delegation.spec.ts	Reworks delegation-resume tests to use taskHistoryStore, checks atomic updater outputs and event ordering, models rejection and guard paths, and adds webview-update assertions plus handoff correctness checks.
nested-delegation-resume taskHistoryStore stub src/__tests__/nested-delegation-resume.spec.ts	Adds a taskHistoryStore test double with atomicUpdatePair and get, and passes it into the nested delegation provider stub.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related issues

• [Story 2.2] Serialize reopenParentFromDelegation steps 4–5 #365 — Directly matches the atomic pair persistence rewrite for reopenParentFromDelegation and the new TaskHistoryStore API.
• [Epic 2] Task Lifecycle Fixes #357 — Covers the task lifecycle fixes this change is part of.
• [Story 2.3] Task status transition guard and startup reconciliation #366 — Related delegation-state persistence and reconciliation work.

Possibly related PRs

• Zoo-Code-Org/Zoo-Code#691: Adds atomicReadAndUpdate to TaskHistoryStore, which uses the same lock-held persistence pattern.
• Zoo-Code-Org/Zoo-Code#510: Also changes ClineProvider.reopenParentFromDelegation guard behavior and surrounding tests.

Suggested reviewers

• taltas
• JamesRobert20
• hannesrudolph
• navedmerchant

A bunny hopped by with a ledger and grin,
“Two history writes now land as one win!”
No halfway state lurks in the night,
Just child and parent set right.
Hoppy, tidy, atomic—done in a spin 🐇

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Linked Issues check	⚠️ Warning	The atomic pair and tests address #365, but the sentinel recovery cleanup in reconcileDelegationState is not shown as removed.	Remove the sentinel-specific recovery case from reconcileDelegationState and confirm the delegation flow no longer depends on it.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title is concise and accurately summarizes the main change: atomicizing reopenParentFromDelegation.
Description check	✅ Passed	The PR description covers the issue link, implementation summary, test procedure, checklist, and notes; only optional sections are omitted.
Out of Scope Changes check	✅ Passed	All changes are focused on delegation persistence, TaskHistoryStore atomic writes, and the related tests.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch issue/365

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[codecov]

Codecov Report

❌ Patch coverage is 92.50000% with 3 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/core/webview/ClineProvider.ts	76.92%	0 Missing and 3 partials ⚠️

📢 Thoughts on this report? Let us know!

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (1)

src/core/task-persistence/__tests__/TaskHistoryStore.spec.ts (1)

445-605: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add validation coverage for updater ID changes.

Alongside the source guard, add cases where the first and second updater return a different id, and assert no unintended task file/cache entry is written. As per coding guidelines, "**/*.{test,spec}.{ts,tsx,js}: 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/core/task-persistence/__tests__/TaskHistoryStore.spec.ts` around lines
445 - 605, The atomicUpdatePair test coverage is missing validation for
updater-generated ID changes, so add cases in TaskHistoryStore.atomicUpdatePair
where the first and second updater functions return a different id. Assert that
the source guard rejects or prevents the change and that no unintended task file
or cache entry is created or modified for the new id. Keep the assertions
aligned with the existing TaskHistoryStore.spec patterns for on-disk persistence
and in-memory cache state.

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-persistence/TaskHistoryStore.ts`:
- Around line 579-583: The atomicUpdatePair flow currently writes whatever IDs
the two updaters return, so validate both updated IDs before calling upsertCore.
In TaskHistoryStore.atomicUpdatePair, compare updatedFirst.id and
updatedSecond.id against the original first.id and second.id, and reject the
operation if either updater changed its task ID. Keep the existing behavior
aligned with atomicReadAndUpdate by preserving pair IDs before any writes and
only proceeding to upsertCore when both IDs still match.

In `@src/core/webview/ClineProvider.ts`:
- Around line 3771-3778: The success path in ClineProvider’s atomic history
transition updates storage via atomicUpdatePair() but never notifies the
webview, so the in-memory task history stays stale. After the paired update for
childTaskId and parentTaskId, explicitly broadcast taskHistoryItemUpdated for
both records using the same payload shape as updateTaskHistory() does, or route
this path through updateTaskHistory() so the write-through event is emitted
consistently.

---

Nitpick comments:
In `@src/core/task-persistence/__tests__/TaskHistoryStore.spec.ts`:
- Around line 445-605: The atomicUpdatePair test coverage is missing validation
for updater-generated ID changes, so add cases in
TaskHistoryStore.atomicUpdatePair where the first and second updater functions
return a different id. Assert that the source guard rejects or prevents the
change and that no unintended task file or cache entry is created or modified
for the new id. Keep the assertions aligned with the existing
TaskHistoryStore.spec patterns for on-disk persistence and in-memory cache
state.

🪄 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: 1dedc00c-be40-44dc-a008-c7c14f393884

📥 Commits

Reviewing files that changed from the base of the PR and between f75b64e and b5fb006.

📒 Files selected for processing (5)

• src/__tests__/history-resume-delegation.spec.ts
• src/__tests__/nested-delegation-resume.spec.ts
• src/core/task-persistence/TaskHistoryStore.ts
• src/core/task-persistence/__tests__/TaskHistoryStore.spec.ts
• src/core/webview/ClineProvider.ts