fix(runtime-host): recover failed workspace Skill deletion

[garvit-arora]

Summary

• Safely cancel an unapplied workspace Skill deletion during recovery when the live target remains unchanged and no tombstone was created
• Prevent a failed Windows rename from permanently blocking Skill catalog access and Runtime Host startup
• Add a regression test for recovery after a failed pre-rename operation

Fixes #5987

Verification

• npm exec -- biome check packages/runtime-host/src/server/skill-catalog-transaction.ts packages/runtime-host/src/__tests__/skill-catalog-transaction.test.ts
• Focused Skill transaction tests: new regression and all related deletion tests pass
• Existing Windows symlink test remains unavailable without symlink privileges
• Full runtime-host build is currently blocked by unrelated current-main type errors in Agent Graph/archive-retention modules

AI use

Generative tooling made a substantive contribution.

Tool(s) and scope: GitHub Copilot assisted with investigation, implementation, and regression-test drafting; the final behavior and safety condition were reviewed against the transaction state machine.

[garvit-arora]

The Windows recovery job showed all 94 Skill catalog tests passing, but the gate still expected 93. Updated the workflow assertion to 94 in the latest push. Product tests remain green; please rerun CI.

[Astro-Han]

Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.

Reviewed range: merge-base 74094c6 .. head b6d8402 (2 commits: eb312b0 fix, b6d8402 Windows test-count bump). First review of this PR; no prior findings.

The fix is narrow and fits the transaction state machine. During recover() only, if the delete rename into tombstone throws, the tombstone is still absent, and the live target is a plain non-symlink directory that still matches intent.expected, the transaction is moved to GC and the deletion is cancelled instead of failing every catalog query and Host startup (#5987). Path containment is kept: the target is join(root.skills, skillId) under the canonical, already-verified skills root. The content check is kept too, because the manifest is re-verified before cancelling. If the rename actually took effect, the tombstone exists and the original error is rethrown. A first-pass (non-recovery) failure still reports commit_outcome_unknown, which is correct because the outcome is resolved by the next recovery. The new regression test passes in the windows_recovery job (94/94).

Findings

P2: PR-caused CI failure. The required test job fails in scripts/ci-workflow-policy.test.mjs ("Windows recovery executes the complete Skill catalog suite", around line 938). Lines 946-947 still assert /# tests 93/u and /# pass 93/u against .github/workflows/windows-recovery.yml, which this PR changes to 94. Update both assertions to 94 (and the expected Write-Error message, if the test checks it).

P3: Recovery now rolls back a user-requested deletion on any rename error, including transient ones such as EBUSY or an Explorer handle, and it does so silently. The Skill simply reappears after the delete returned commit_outcome_unknown. This is a reasonable resolution of #5987, but a diagnostic or log entry when an intent is cancelled would make the outcome traceable. Optionally, the first-pass delete could cancel immediately and return a definite failure instead of commit_outcome_unknown, since the outcome is actually known there.

P3: The regression test simulates the rename failure with the new before_delete_rename failpoint, which throws before rename is called. No real filesystem error is exercised. A variant that makes the rename fail for real (for example the POSIX chmod reproduction described in #5987) would cover the actual error path.

P3 (minor): If the re-verification inside the catch fails, its persistence_failed replaces the original rename error. The resulting behavior is the same as before (the transaction is retained), but the root-cause errno is lost from the error chain. Consider attaching it as cause.

No protocol or epoch files are touched. Mergeability: MERGEABLE, state BLOCKED. CI on b6d8402: windows_recovery passes, test fails (P2 above, PR-caused).

[Astro-Han]

P2: scripts/ci-workflow-policy.test.mjs lines 946-947 still assert # tests 93 and # pass 93 against this workflow, so the required test job fails. Bump those assertions to 94 together with this change.

[Astro-Han]

P3: This cancels the user's deletion on any rename error during recovery, including transient ones, and nothing records that it happened. Consider emitting a diagnostic when an intent is cancelled, and keeping the rename errno as cause if the re-verification below throws.

[Astro-Han]

Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.

Incremental review: b6d8402 to 49d4f53. This adds one commit, 49d4f53 ("test(windows): align Skill suite count policy"). The merge-base is unchanged (74094c6) and there was no rebase.

Prior findings

• P2 (CI failure in scripts/ci-workflow-policy.test.mjs): fixed. Lines 946-947 now assert # tests 94 and # pass 94, matching windows-recovery.yml. The new commit changes nothing else.
• P3 (recovery silently cancels a requested deletion on any rename error, including transient ones): still open. This is a non-blocking design note.
• P3 (the regression test uses the simulated before_delete_rename failpoint, not a real filesystem error): still open. Non-blocking.
• P3 (the original rename errno is lost when re-verification fails inside the catch): still open. Non-blocking.

New findings in this range: none.

No protocol or epoch files are touched.

Mergeability/CI: On 49d4f53, test and windows_recovery both pass. GitHub reports the PR as MERGEABLE, and the merge state is BLOCKED only because there is no approving review. No P0-P2 issues remain.