Serialize skill sync and upgrade under the lock

[samuv]

Summary

The plugin lock-file review found that sync/upgrade can mutate from a stale lock/DB snapshot (resurrecting a concurrent uninstall, or overwriting a newer install) and that default sync never expands to newly detected clients. Skills had the same shapes.

• Re-read each skill under the per-skill lock before classifying or mutating; nested Install/Uninstall skip a second acquire of the same key.
• Default thv skill sync (no --clients) keeps Install’s all-detected default, and --check is not current unless every detected client is present.
• Each upgrade reloads, plans, and applies under the same lock so a removed entry is not resurrected. Upgrade still preserves an existing skill’s client list.

Type of change

• Bug fix
• New feature
• Refactoring (no behavior change)
• Dependency update
• Documentation
• Other (describe):

Test plan

• Unit tests (task test) — ran go test for ./pkg/skills/skillsvc/ (TestSync_, TestUpgrade_, TestInstall)
• E2E tests (task test-e2e)
• Linting (task lint-fix)
• Manual testing (describe below)

Changes

File	Change
pkg/skills/skillsvc/service.go	Nested-safe lockSkill
pkg/skills/skillsvc/install.go / _git.go / _oci.go / uninstall.go	Use lockSkill so sync/upgrade can call them while holding the key
pkg/skills/skillsvc/sync.go	Per-skill fresh read; default clients = all detected
pkg/skills/skillsvc/upgrade.go	Per-skill reload/plan/apply
pkg/skills/skillsvc/*_test.go	Stale-list, new-client, and removed-entry coverage

Does this introduce a user-facing change?

Yes. thv skill sync without --clients now materializes newly detected skill-supporting clients instead of keeping only the clients recorded on the previous install. thv skill upgrade is unchanged: it still preserves the existing client list.

Special notes for reviewers

This is the skills counterpart of the plugin lock-file review on #6316 / #6317. Plugin-only issues (lock-restore callback errors, Materialize-after-extract cleanup) do not apply here: skills uninstall still treats installed-but-unlocked as the safe inconsistency, and skills extract without a post-extract marketplace/settings step.

[JAORMX]

Reviewed the full PR against main. The single-key context marker does not safely model nested dependency locks, and direct Git/OCI installs still release the lock before lock-file/group bookkeeping, so the intended serialization is incomplete and can deadlock. Requesting changes for the two inline blockers.

[samuv]

Update

f01ead3dd replaces heldSkillLock with a project-scoped transaction lock held through install bookkeeping and deps. Git/OCI no longer unlock before installAndRegister. Nested deps use locked helpers under the held tx.

env -u TOOLHIVE_DEV go test ./pkg/skills/skillsvc/ ./pkg/groups/ -count=1 passes.

[samuv]

Rebased onto current main (8343851e8). All previously unanswered review threads have replies and are marked resolved — please take another look when convenient.

[JAORMX]

Re-reviewed commits after 4769eb2 against main. The transaction refactor still has four merge-blocking rollback/resource-lifecycle gaps.

[samuv]

Aug-18 review round addressed

0a9d7b615 + 79a7cdb9e (rebased onto current main):

• All extraction/write paths snapshot targets first and join every compensation error (removeSkillDirs is gone)
• rollbackInstall results are joined at every call site
• RestoreFiles now removes freshly created trees and restores pre-existing ones (tracked via dirBackup.existed)
• projectTx is a fixed 64-stripe mutex set (no unbounded map)
• Snapshot/restore run through os.Root handles for OS-enforced containment (CodeQL path-injection alerts)

env -u TOOLHIVE_DEV go test ./pkg/skills/... ./pkg/groups/ and task lint-fix pass. The full task test run shows two pre-existing local-env failures (SSRF test under TOOLHIVE_DEV=true, streamable proxy test that also fails on clean main) — neither is introduced by this PR.

[codecov]

Codecov Report

❌ Patch coverage is 72.86996% with 121 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.06%. Comparing base (0b8acf2) to head (a0c937a).

Files with missing lines	Patch %	Lines
pkg/skills/skillsvc/install.go	68.06%	25 Missing and 13 partials ⚠️
pkg/skills/skillsvc/file_snapshot.go	64.70%	17 Missing and 13 partials ⚠️
pkg/skills/skillsvc/sync.go	68.00%	18 Missing and 6 partials ⚠️
pkg/skills/skillsvc/install_oci.go	33.33%	7 Missing and 3 partials ⚠️
pkg/skills/skillsvc/install_extraction.go	82.35%	3 Missing and 3 partials ⚠️
pkg/skills/skillsvc/install_git.go	77.77%	4 Missing and 2 partials ⚠️
pkg/skills/skillsvc/upgrade.go	83.33%	3 Missing and 2 partials ⚠️
pkg/skills/skillsvc/lock.go	60.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6352      +/-   ##
==========================================
+ Coverage   73.04%   73.06%   +0.01%     
==========================================
  Files         745      746       +1     
  Lines       79208    79505     +297     
==========================================
+ Hits        57857    58088     +231     
- Misses      17300    17337      +37     
- Partials     4051     4080      +29     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[samuv]

Patch coverage improved

c776a3dd8 adds failure-path tests for the areas codecov flagged:

• file_snapshot.go: unit tests for snapshot/restore (missing parent, non-directory target, nested trees, executable-mode preservation, fresh-tree removal, dedupe)
• Rollback: storage failure during pre-existing-record restore, lock-snapshot corruption after DB create (fresh tree removed, errors joined), force-reinstall group failure restoring the prior lock entry
• Sync/upgrade: adopt compensation when marking managed fails (entry removed), corrupt-lock failures at both the entry points and per-name reconcile
• depState: nil-receiver, cycle, and completion semantics
• groups.RemoveSkillFromGroup: full unit matrix

Also rebased onto current main (7a0c0a51b), which fixes the zizmor static-analysis failure. The remaining uncovered patch lines are deep I/O error branches (e.g. os.Root open/walk failures) that are impractical to inject portably.

[samuv]

Resolved the merge conflict with main via a merge commit (a0c937a20) — no history rewrite, no code changes beyond the conflict itself: a single import-block hunk in pkg/skills/skillsvc/service.go where #6383's toolhive-core/container/signer import collided with this branch's httperr import (kept both).

The repo dismisses stale approvals on push, so the earlier approval was auto-dismissed; re-requested review. Tests and lint pass on the merged head.