Verification spike: issue #26 skill refresh data-loss path

[jon-devlapaz]

Verdict

Partially mitigated — the original silent-drop failure mode from #26 is not present on current main. On publish+rollback double failure, rollback_or_retain_backup calls TempDir::keep() and the error names the recovery backup path. #26 can be closed as mitigated for the refresh/replace path. #29 should stay open for the shared durable helper, temp-ish recovery naming, and other call sites.

Background checked

Issue #26 (opened @ 7209ca3) described: live skill moved to staging backup; if publish and restore both fail, dropping TempDir deletes the only copy.

Current main (4c86577) uses a different path.

Current behavior (characterization)

Skill refresh flows through:

1. refresh::refresh_skill → skills::replace_verified (src/refresh.rs:177)
2. replace_verified_inner stages beside destination, copies to new/, then calls publish_staged_tree (src/skills.rs:791-820)
3. publish_staged_tree (src/skills.rs:778-788):
   • rename(target → staging/old)
   • rename(staged → target); on failure → rollback_or_retain_backup
4. rollback_or_retain_backup (src/skills.rs:752-773):
   • On successful rollback: return mapped publish error
   • On rollback failure: staging.keep(), build recovery path kept/old, return error naming recovery backup: <path>

Same helper is used by library promotion (src/library.rs:163) and skillset refresh publish (src/skillsets.rs:527, 1252).

Evidence

Commands run

cargo test rollback_failure_retains_recovery_backup -- --nocapture
cargo test publish_staged_tree_restores_target_when_publish_fails -- --nocapture
cargo test --workspace --all-targets --locked
cargo fmt --all -- --check
cargo clippy --workspace --all-targets --locked -- -D warnings

All passed locally on Linux x86_64.

Key assertions

Test	What it proves
skills::tests::rollback_failure_retains_recovery_backup	Double failure at rollback_or_retain_backup: error contains recovery backup, original bytes remain at kept/old/
skills::tests::publish_staged_tree_restores_target_when_publish_fails (new)	Real publish_staged_tree seam: missing new/ → publish fails → rollback restores original target bytes; no recovery backup message
k10_skillset_refresh_updates_clean_tree_and_refuses_local_edits	End-to-end refresh happy path (existing acceptance)

What is not proven

• Deterministic end-to-end double-failure injection through the full rename window (would need concurrent target recreation or test hooks). The helper test + direct code path (publish_staged_tree line 786 → rollback_or_retain_backup) is the strongest honest proof available without Refactor: durable atomic skill-tree swap (shared helper) #29 refactors.
• Windows (explicitly out of v1 scope).

Residual risks vs #29

Gap	Notes
Recovery path is temp-named	Kept dir remains .tink-update-* / .tink-promote-* beside destination — operator-visible but not a durable orphan name (#29 design target)
install_local (first install)	Stages and single renames without backup/rollback (src/skills.rs:735-746). Different failure shape; not #26's replace path
Manifest pair write	Separate rollback + keep() in manifest::write_atomic (src/manifest.rs:346-364) — #29 scope
Binary update	update::replace_binary has its own keep-on-rollback-failure path (src/update.rs:442-451)
Concurrent target recreation	Documented failure mode; no lock; recovery depends on keep() winning the race
Cross-filesystem rename	Explicitly out of v1 (ACCEPTANCE.md)

Recommendation

• Close skill refresh can drop skill tree if post-swap restore fails #26 as mitigated: silent backup drop on refresh replace is fixed; error names recoverable path.
• Keep Refactor: durable atomic skill-tree swap (shared helper) #29 open for shared durable helper, consistent orphan naming, and consolidating manifest/update/install paths.
• This PR adds characterization test + docs only; no full Refactor: durable atomic skill-tree swap (shared helper) #29 refactor.

Changes

• Add publish_staged_tree_restores_target_when_publish_fails unit test
• Document recovery behavior in docs/RELIABILITY.md, proof gaps in docs/TESTING.md
• Clarify K10 acceptance row to reference double-failure unit coverage

Relates to #26, #29