Add SkillLockService interface and managed install flag

[samuv]

Summary

• Why: RFC THV-0080's sync and upgrade commands operate over a project's whole lock file, not a single named skill — a different shape than SkillService's per-skill operations. Lock-managed installs also need a durable marker so sync --prune can tell lock-managed skills apart from ones a user installed/manages by hand outside the lock file.
• What:
  • SkillLockService interface (Sync, Upgrade) with generated mock, kept separate from SkillService per the RFC's API design.
  • SyncOptions/SyncResult/SyncFailure, UpgradeOptions/UpgradeResult/UpgradeOutcome, and the RFC's typed FailureReason enum (registry-unreachable, digest-missing, validation-rejected, lock-write-failed, ref-change-blocked, signature-invalid, signer-mismatch, unsigned-rejected, unknown).
  • InstalledSkill.Managed + migration 003 (installed_skills.managed), wired through the SQLite store.

Part of the production stack for #5715 (RFC THV-0080). Stack: 2/6 — lockfile → lock-service → install-hooks → sync → upgrade → cli-exitcodes. Based on #5892 (PR1); no new consumers yet — skillsvc wiring lands in PR3.

Type of change

• New feature

Test plan

• Unit tests (task test)
• Linting (task lint-fix)

Added: migration test proving 003 defaults existing rows to unmanaged and its Down cleanly drops the column without disturbing 001/002 tables; store-level round-trip test for Managed (create, then flip via Update).

Does this introduce a user-facing change?

Yes — InstalledSkill gains a managed field, visible in the existing GET/list skills API responses (swagger updated accordingly). No new endpoints yet.

Special notes for reviewers

• UpgradeOptions/UpgradeStatus intentionally omit AllowSignerChange/signer-change-blocked — those are Sigstore-stack concerns (PR13). Entry-level provenance/unsigned fields land in the Sigstore stack too (PR7).
• No deprecated fields carried over from the POC (SyncResult.Unmanaged, UpgradeOptions.DryRun) — these are new APIs, shipped clean.

🤖 Generated with Claude Code

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 71.84%. Comparing base (97a9dfa) to head (bae6917).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5893      +/-   ##
==========================================
- Coverage   71.84%   71.84%   -0.01%     
==========================================
  Files         708      708              
  Lines       72811    72823      +12     
==========================================
+ Hits        52311    52317       +6     
- Misses      16765    16768       +3     
- Partials     3735     3738       +3     

☔ 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]

Tracked in #5899 (RFC THV-0080 stack).

[JAORMX]

🤖 Automated panel review — PR 2/6 (SkillLockService + managed flag)

Multi-agent review, each finding adversarially verified against the diff. Advisory. Whole stack gated behind TOOLHIVE_SKILLS_LOCK_ENABLED (off by default).

API boundary verdict: splitting SkillLockService from SkillService is the right call and conforms to the RFC — no objection there.

🟡 Minor

• Three Sigstore FailureReason values ship with no producer (options.go:~160): signature-invalid, signer-mismatch, unsigned-rejected. Nothing in this lock-only stack can emit them (classifySyncFailure only ever produces registry-unreachable/digest-missing/validation-rejected/lock-write-failed/unknown). They're dead exported JSON-API constants pinned before the Sigstore design exists — if that stack refines the taxonomy (e.g. renames signer-mismatch), it's now a breaking change. It's also internally inconsistent with this PR deliberately deferring the sibling signer-change-blocked value. Gate them into the stack that produces them, or comment them reserved.
• FailureReasonRegistryUnreachable is OCI-registry-specific (options.go:~154) yet is the only transport-failure reason — git-source fetch failures have no accurate typed reason for the CI/automation consumers the enum exists for.
• InstalledSkill.Managed uses json:",omitempty" (types.go:~170), so the newly-added user-facing field is omitted whenever false — i.e. for every user-scope install and every unmanaged project install (the common case the PR says it exposes). Contradicts the swagger contract and hides the false state from list/get consumers. Drop omitempty for a bool that's meant to be observable.
• UpgradeResult.Outcomes lacks omitempty and is a slice (options.go:~258), so an empty result marshals to {"outcomes":null} rather than [] — mildly awkward for typed clients.

🟢 Verified praise

• Migration 003 is well covered: a test proves rows created pre-003 default to unmanaged, and Down cleanly drops only the managed column while leaving 001/002 intact — plus an app-level errManagedRequiresProjectScope guard enforcing managed-implies-project-scope with round-trip tests. These are the security-correct defaults.

[samuv]

All findings addressed in the latest push:

• Unproduced FailureReason values: removed — the three Sigstore reasons and ref-change-blocked (which nothing produced either; the ref-change guard surfaces as an UpgradeStatus, not a failure reason). They land in the stack that ships their producers, so refining the taxonomy there won't be a breaking change. The type's doc comment states the policy.
• Managed omitempty: dropped — false (unmanaged) is an observable state list/get consumers need to see; swagger regenerated.
• registry-unreachable OCI-specific wording: doc comment now covers any remote source (OCI registry or git host), which is what classifySyncFailure actually maps to it. The RFC-pinned string itself is unchanged.
• UpgradeResult.Outcomes marshaling null: fixed on the upgrade PR (Add thv skill upgrade: re-resolve pinned skills to newer content #5896) where the implementation lives — Upgrade now always returns a non-nil slice.

[JAORMX]

🤖 Automated panel re-review on 611f480 → approving

All findings from the earlier review are resolved against the actual code:

• ✅ Premature Sigstore FailureReason constants removed (and ref-change-blocked demoted to a UpgradeStatus, not a reason); the type doc now states the "only reasons the current feature set can produce are defined" policy.
• ✅ FailureReasonRegistryUnreachable doc broadened to cover OCI and git remotes while keeping the RFC-pinned wire string.
• ✅ InstalledSkill.Managed dropped omitempty (with regenerated swagger) — false now always serializes.
• ↩️ UpgradeResult.Outcomes null-vs-[] nit reasonably deferred to producer PR #5896 (adding omitempty here wouldn't help; only a guaranteed non-nil slice at the producer yields []).

Bonus: the new errManagedRequiresProjectScope invariant guard (rejecting managed on non-project installs, tested in Create + Update) is a net improvement. No regressions. LGTM.

[JAORMX]

Panel review (spec / standards / security / library-reuse / duplication / golang-conventions / UX) — clean across the board.

Solid foundation: SkillLockService is cleanly separated from SkillService, the managed SQLite flag + migration 003 are correct (store enforces the project-scope invariant on Create and Update, migration test verifies the default), and the diff is genuinely lean.

Non-blocking notes for follow-up layers in the stack:

• Spec gap (deferral?): RFC THV-0080 lists a public PreserveSource install option alongside SkillLockService, but InstallOptions is untouched here. Assume it's coming in a later PR — flagging so it doesn't fall through.
• errManagedRequiresProjectScope (skill_store.go) is unexported, so the API layer can't errors.Is-match it to a 400 vs 500. Fine if intentionally internal.
• Migration test hardcodes entry_id = 1 — safe on a fresh DB, brittle if a pre-seeded fixture ever lands.

Approving.