fix(skills): keep a newer skills ref and a removed tool when a refresh overlaps

[dg-coreylweathers]

Stacked on #125 until it merges.

What

Fixes S5 from the #111 review: a concurrent refresh could overwrite a newer explicit skills ref. It also covers the related case where a refresh that started before dg skills remove puts the removed tool's skills back.

dg skills update and the plugin refresh choose each tool's ref from skills.json, then download the bundle with no lock held. While that download runs, another command can install a tool from a different ref or remove it. Before this change, the refresh then wrote over that newer state.

How

• A refresh passes the skills.json it planned from (since) to install_for, which passes it to install_tool.
• install_tool already holds the skills.json lock for the whole install. Right after it reads the records under that lock, and before it stages, marks or moves anything, it compares the tool's whole record with the planned one (_recorded: the tool's full skill_folders record, including skills_ref, version, installed_at and every folder's fingerprint, plus its installed_skills entry, or nothing when the tool is not listed):
  • The tool is no longer listed: skip it (E31), always.
  • The record changed in any way: skip it, unless the user passed --ref to this dg skills update. The warning is E30 when the record now names a different ref, and E32 otherwise (the same ref, which may be a moving branch, or no ref). An explicit --ref still wins, and the last explicit command wins, but it never brings back a removed tool. So dg skills update --ref <moving ref> applies its ref even if its download is older than one another command installed meanwhile.
• A skip raises SkillSkipped (a SkillInstallError). install_for catches it per tool, prints one warning on stderr, does not yield that tool and carries on with the next one. Exit codes and the yielded tuples are unchanged.
• Fresh installs (dg skills install, dg skills setup, the first-time prompt in dg login) pass no snapshot and keep their current behavior. DEEPCTL_SKILLS_REF counts as an implicit ref.
• A tool that only has 0.3.x records is listed with no ref on both sides, so a refresh still installs its folders.

Messages

• E30: Another deepctl command installed {display}'s skills from a different ref while this one was running, so deepctl kept that ref and did not update {display}.
• E31: Another deepctl command removed {display}'s skills while this one was running, so deepctl did not install them again.
• E32: Another deepctl command changed {display}'s skills while this one was running, so deepctl left them as that command left them and did not update {display}; run 'dg skills update' to refresh them. It fires on any change to the tool's record, including one left by another command that failed partway.

Interleavings covered

• A refresh overlaps dg skills remove (during the download or between tools): the tool is skipped with E31 and nothing is written for it.
• dg skills remove waits on the lock while the refresh installs that tool: the remove then removes the fresh copy.
• A refresh overlaps dg skills install --ref X: the tool is skipped with E30 and X stays.
• dg skills update --ref Y overlaps dg skills install --ref X: Y is applied (both are explicit); a tool removed meanwhile is still skipped.
• Two refreshes of one moving ref (for example main) that downloaded different commits: whichever installs first stays, and the other is skipped with E32.
• Two refreshes from the same snapshot of a blank-ref or 0.3.x-only record: the first installs and the second is skipped with E32, never E30. Ctrl-C at the check: nothing is written and the lock is released.
• Known edge, left as is: if a tool is removed during the download and an unowned folder appears at its destination, the preflight reports E1 for the whole run and changes nothing.

Tests

• S5 regression: test_refresh_never_overwrites_a_ref_changed_during_its_download
• Moving ref (review B1): test_refresh_never_overwrites_a_newer_copy_of_the_same_ref (core), test_refresh_keeps_a_newer_copy_of_the_same_moving_ref (plugin) and test_update_keeps_a_newer_copy_of_the_same_moving_ref (dg skills update). Each gives two refreshes one ref but different bundle bodies, lands the newer one first, and asserts the newer body and record stay with one E32 and no E30.
• Removed during a download: test_refresh_never_reinstalls_a_tool_removed_during_its_download
• Core, in TestRecheckUnderLock: the skip is per tool; an explicit ref still wins but never brings back a removed tool; fresh installs ignore the records; 0.3.x-only tools still get folders (and are skipped with E31 when removed); two refreshes of a blank-ref or 0.3.x-only record keep the first install and warn with E32, never E30 (test_two_refreshes_of_a_blank_ref_record_keep_the_first); Ctrl-C at the check writes nothing. test_fetch_runs_without_the_lock now runs with and without a snapshot.
• dg skills update: test_update_keeps_a_ref_installed_during_its_download (exit 0, E30 on stderr), test_update_with_ref_overrides_but_skips_a_removed_tool, and test_update_summary_counts_only_tools_it_installed (the closing summary lists only the refs of tools it installed, and the /setup-mcp hint appears only when Claude Code was installed).
• Plugin refresh: test_refresh_skips_a_tool_removed_during_its_download (stdout empty, one E31 warning) and test_refresh_keeps_an_explicit_ref_set_during_its_download (exit 0, one E30 warning).
• Targeted mutants of the new check (no E31 branch, no E30 branch, ignoring --ref, comparing the planned ref instead of the record, treating a skip as a failure) each fail at least two tests.
• The four B1 tests above fail against the previous ref-only check.
• Outside the suite, on Linux in Docker as uid 1000, rerun on the current head: the same-ref probe (18 of 18 pass: an overlapping install of the pinned ref or of the same DEEPCTL_SKILLS_REF, and a second refresh of a blank-ref record, are each skipped with exactly one E32 and leave the other command's files and record byte for byte; a different env ref gets E30; update --ref Y over install --ref Y goes ahead with no warning); remove and install --ref overlapping a plugin refresh and dg skills update, during the download and between tools (8 of 8 pass); the remove/refresh orderings for plugin, update, login and install, plus a second plugin refresh in place of the remove, including a remove that finishes during the download (30 of 30 with no orphans or leftover staging); and a HOME written by main's dg skills install --all, refreshed by the plugin and by dg skills update in both orders, which installs every folder tool with no E30 or E31.

Not in this PR

A pending-removal state, keep-going past a failed tool, and pruning of retired skills.

Budget and gate

• Source lines (excluding tests): 69 added of the 120 budget. skill_generator.py +56/-6, deepctl_cmd_skills/command.py +11/-8, deepctl_cmd_plugin/command.py +1/-1, README.md +1/-1.
• Docker gate (python3.12-bookworm-slim): ruff format, ruff check, mypy and the full suite: 1991 passed, 9 skipped. Core tests as uid 1000: 728 passed, 2 skipped. Core tests on Python 3.10: 727 passed, 3 skipped.

Review response

• B1 (Greg, moving ref): fixed. The recheck now compares the tool's whole record, not only skills_ref, so a non-explicit refresh skips a tool whose record changed at all, including under the same ref spelling, and warns with the new E32. E31 for a removed tool and the --ref override are unchanged. Race tests for core, the plugin refresh and dg skills update cover it, and the earlier blank-ref case now prints the true E32 instead of installing silently.

🤖 Generated with Claude Code

[GregHolmes]

Title: [B1] packages/deepctl-core/src/deepctl_core/skill_generator.py:632-636 A moving ref can still be overwritten by an older refresh

Summary: The new recheck compares only the ref string. User refs are deliberately mutable and always re-downloaded (packages/deepctl-core/src/deepctl_core/skill_bundle.py:171-176), so two overlapping refreshes of the same branch name can fetch different commits. If the newer download installs first, the older download sees the same string in skills_ref, passes the guard, and replaces the newer folders with stale content. This is the stale-refresh class this PR is intended to eliminate, just without a changed ref spelling.

Expected: A non-explicit refresh that fetched an older resolution of a moving ref must never replace content installed by a later refresh of that same ref.

Observed: For both runs using a ref such as main, _recorded(state, cli) == _recorded(since, cli) == ref; the guard treats the record as unchanged and the older download proceeds.

Recommended fix: Recheck a stable snapshot of the full tool record, not only skills_ref, before staging or moving. A non-explicit refresh should skip when that record changed, including when the ref spelling stayed the same; preserve the existing E31 behavior for a removed record and the --ref override policy. Add a race test where two refreshes use one moving ref but receive different bundle bodies, assert the newer body remains installed, and cover plugin refresh plus dg skills update.

[dg-coreylweathers]

On B1 (moving refs): fixed in 1f86cd0 and b0b1595. Under the lock, install_tool now compares a snapshot of the tool's whole record (skill_folders.<tool> with its ref, version, installed_at and folder fingerprints, plus installed_skills.<tool>) with the one the refresh planned from. A refresh without --ref skips the tool on any change, including the same ref spelling: E30 when the record now names a different ref, E32 otherwise. E31 for a removed tool is unchanged, and an explicit dg skills update --ref still applies its ref but never reinstalls a removed tool. Your race is a test for the core path, the plugin refresh and dg skills update: two refreshes of one moving ref with different bundle bodies, newer first; the newer body stays. All four fail against the old ref-only check.