[AmethystLiang]

	Files	Added	Deleted	Net
Test	1	$\color{#1a7f37}{\Huge{\mathbf{+}}}$​203	0	$\color{#1a7f37}{\Huge{\mathbf{+}}}$​203
Prod	1	$\color{#1a7f37}{\Huge{\mathbf{+}}}$​4	$\color{#cf222e}{\Huge{\mathbf{−}}}$​1	$\color{#1a7f37}{\Huge{\mathbf{+}}}$​3

Problem

When persisting legacy worker terminal recovery state, snapshots were captured too early (inside the worker loop after each update). If the durable write failed, rollback could lose edits made during that write because it was using intermediate snapshots instead of the final state.

Solution

Move snapshot capture to after all workers are processed. This ensures rollback always restores the complete final state, preventing data loss when writes fail while edits are in flight.

Added comprehensive tests covering:

• Snapshot budgeting per execution host (local, SSH, runtime)
• Rollback behavior when writes fail mid-flight
• Edge cases (empty batches, unroutable workers, no-op persistence)

Testing

• Added 209 lines of tests to runtime-legacy-worker-terminal-recovery-persistence.test.ts
• Tests verify snapshot counts, rollback fidelity, and edge cases

Checklist

• This PR is small and focused
• I explained what changed and why (ELI5, the user-facing before/after, the mechanism, and why over the alternatives)
• Self-reviewed for correctness, security, and performance
• Cross-platform, SSH/remote, and path/shortcut impact considered (or N/A)

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Important

Review skipped

We couldn't safely recover the incremental review. No full review was started, and the last reviewed checkpoint was preserved. Retry later, or explicitly request a full review by commenting @coderabbitai full review.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

📝 Walkthrough

Walkthrough

persist now snapshots each affected host’s final stored session after processing all recovery resolutions. Tests cover batches of different sizes and host types, durable-write rollback, adopted resolutions without session edits, and empty or unroutable batches.

Priority: ⬇️ Low

Merge Risk: 🔵 Low · up to abe9c

The recovery persistence change appears sound. Two new tests check the returned dispatch-ID set with a length assertion, which a Set cannot satisfy, so the test suite fails until they check size instead. Fixing those assertions is a quick change before merge.

Security Architecture Review

Security architecture risk: ⚪ Minimal · up to abe9c

The change preserves existing access controls and host ownership while reducing redundant snapshots. The reviewed recovery and rollback paths show no material security risk introduced or worsened by this PR.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• observed — A recovery batch can affect multiple execution-host sessions, rather than just one worker. Original snapshots, final snapshots, and rollback remain keyed by the resolved execution-host identity.

Trust Boundaries and Controls

• inferred — The inspected change does not expand attacker-controlled reachability or persistence authority: production wiring remains unchanged, and the edit only relocates snapshot capture within the existing mutation callback.

Resilience and Maintainability Implications

• observed — The shared durable-write policy rolls back determinate failures but intentionally skips rollback for indeterminate outcomes. That policy is unchanged by this PR; the reviewed evidence does not establish how every external interruption is classified.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely identifies the main change: fixing legacy worker recovery snapshot timing to improve rollback safety.
Description check	✅ Passed	The description explains the problem, solution, and added tests. It is mostly complete, but it does not include a linked issue, the template’s ELI5/What Changed/Why sections, or a Visual Proof section…
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Commit to this branch
• Create a new PR

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

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

[coderabbitai]

Actionable comments posted: 1

---

ℹ️ Review info

⚙️ Run configuration

• Configuration used: Repository UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: fd031c58-a828-41c3-baa3-29366d524f7b

📥 Commits

Reviewing files that changed from the base of the PR and between f873aaa and abe9cbca22a1eabf045565c0f2302e9db2be1d42.

📒 Files selected for processing (2)

• src/main/runtime/runtime-legacy-worker-terminal-recovery-persistence.test.ts
• src/main/runtime/runtime-legacy-worker-terminal-recovery-persistence.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 6 remain after this review.

[pullfrog]

ℹ️ No critical issues — the code change is correct and reduces work; one documentation-accuracy note below.

Reviewed changes

• Per-host snapshot moved out of the candidate loop (src/main/runtime/runtime-legacy-worker-terminal-recovery-persistence.ts:71) — staged is now filled once per host after all candidates are processed, instead of deep-cloning the whole session after every worker. This drops structuredClone from O(candidates) to two per host.
• New snapshot-budget and rollback tests (src/main/runtime/runtime-legacy-worker-terminal-recovery-persistence.test.ts) — pin the clone count per host, exercise rollback while a durable write is in flight, and cover empty / unroutable / no-op-edit batches.

ℹ️ Nitpicks

• The PR title/body describe this as a rollback-safety fix, but the pre-change code was already safe: staged[host] was overwritten on every candidate for that host, so it always ended as that host's final state, giving rollback an identical input. I ran the new test file against the base (f873aaa) implementation and the rollback test passes there unchanged — only the structuredClone-count assertions fail. Consider reframing the change as the snapshot-budget optimization it actually is (and adjusting the "intermediate snapshots" language) so the history is accurate.

  ^{｜ Fix it ➔ ｜ View workflow run ｜ Using deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏}

[pullfrog]

✅ No new issues found.

Reviewed changes

Reviewed the two commits on the branch since the prior pullfrog review: the branch was rebased onto a newer main (e02adc5) and a follow-up commit corrected the returned-set assertions in the new snapshot-budget tests. The production change is unchanged from the prior review.

• Rebased onto main (e02adc5) — the .ts snapshot-timing change and test file carry over byte-for-byte from the previously reviewed revision.
• Corrected returned-set assertions in the snapshot-budget tests — persist returns a ReadonlySet<string>, so the previous toHaveLength(12) / toHaveLength(10) checks could never pass; they now assert .size is 12 / 10, consistent with the other assertions in the file. This addresses the CodeRabbit thread at test line 135. The file passes locally (7 tests).

  ^{｜ View workflow run ｜ Using deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏}