Verify skill signatures at install time

[samuv]

Note

Stack 2 of RFC THV-0080 (tracking issue #5899) — stacked on #6121 (gitsign verification), which stacks on #6091 (verifier wrapper). This is the highest-risk PR of the stack (it changes install behavior for project scope) — flagged for extra review scrutiny per the plan.

Summary

This PR turns the verification machinery built by the previous PRs into enforced install-time policy. Project-scoped installs (with the lock file feature enabled) now verify artifact signatures before anything is extracted or recorded:

• OCI installs verify through the Sigstore keyless flow; git installs verify the gitsign commit signature over the resolved commit's payload. Both happen at the hook point right after digest resolution, before extraction, DB writes, or lock writes — a rejected install leaves no trace.
• Trust on first use: with no lock entry, the observed signer identity is recorded as provenance: in the lock entry and the Sigstore bundle is persisted with the DB record (for sync's offline re-verification). Subsequent installs feed the recorded identity into the verifier's Sigstore policy — mismatch is a 403 with the prior lock state untouched.
• Unsigned artifacts are rejected by default (403). --allow-unsigned (CLI + existing API field) records an explicit unsigned: true exception. An entry already locked to a signer identity refuses unsigned or local-build replacements outright — even with the flag — since that substitution is exactly what the lock exists to catch.
• Local-store installs (local builds, raw layer data) carry no registry signature, so project-scoped installs of them are an explicit unsigned trust decision.
• Sync restores honor the recorded unsigned exception (the lock file is the policy being restored); fresh user-driven installs must repeat the flag.
• Typed failure classification: signature-invalid / signer-mismatch / unsigned-rejected failure reasons, classified via errors.Is on the verifier's sentinels; classifySyncFailure consumes them.

User-scoped installs and gate-off environments are completely unaffected (verified by test — the verifier mock has zero expectations there).

Type of change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Documentation update
• Refactoring (no functional changes)
• Test improvements
• CI/CD or build system changes

Test plan

• Unit tests pass locally (task test, full suite with -race)
• Linting passes (task lint-fix, 0 issues)
• New unit tests: TOFU records provenance + bundle and the second install receives the recorded identity as expected (asserted inside the mock); unsigned rejected without flag (403, no DB record, no lock entry); unsigned accepted with flag (unsigned: true recorded); signer mismatch rejected with prior lock state intact; locked-unsigned requires the flag again on reinstall (verifier not consulted); user scope skips verification entirely; local-install matrix (no flag / flag / locked identity refuses local replacement / locked unsigned honored); failure-reason classification.
• task docs regenerated (--allow-unsigned flag docs).

Does this introduce a user-facing change?

Yes, behind the experimental TOOLHIVE_SKILLS_LOCK_ENABLED gate: project-scoped installs of unsigned skills are now rejected unless --allow-unsigned is passed, and signed skills are verified against the lock file's recorded signer identity. User-scoped installs are unchanged.

Special notes for reviewers

• The verification decision is made inside the per-source installers (whose opts is a by-value copy), so it travels back on InstallResult (json:"-" fields) and is folded into the lock-recording step in installAndRegister — the one construction point both OCI and git flows share.
• Existing lock-mechanics tests now run with a permissive mock verifier injected by the shared test constructor; verification-behavior tests override it. No test-only hooks in production types — the seam is the WithVerifier constructor option.
• The signed round-trip E2E (push with the signer's test key → install verifies) needs a key-verification path through install options, which the RFC defers to catalog-supplied trust; the current E2E-able surface is covered by the unsigned±flag flows and will land with the sync re-verification PR's E2E.

Generated with Claude Code

[codecov]

Codecov Report

❌ Patch coverage is 73.97959% with 51 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.40%. Comparing base (d8c01d5) to head (961b9c2).

Files with missing lines	Patch %	Lines
pkg/skills/skillsvc/verify.go	73.38%	31 Missing and 6 partials ⚠️
pkg/skills/skillsvc/install_oci.go	36.36%	5 Missing and 2 partials ⚠️
pkg/skills/skillsvc/install.go	37.50%	4 Missing and 1 partial ⚠️
pkg/skills/skillsvc/sync.go	0.00%	2 Missing ⚠️

Additional details and impacted files

@@                        Coverage Diff                        @@
##           skills-sig/10b-gitsign-verify    #6129      +/-   ##
=================================================================
- Coverage                          72.41%   72.40%   -0.02%     
=================================================================
  Files                                738      739       +1     
  Lines                              76054    76234     +180     
=================================================================
+ Hits                               55074    55194     +120     
- Misses                             17069    17116      +47     
- Partials                            3911     3924      +13     

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

[JAORMX]

Panel review posted as inline comments (one critical correctness bug, plus verification-ordering and UX items). Context: reviewed this PR together with its stack (#6091, #6121) against RFC THV-0080 and tracking issue #5899; cross-cutting items are noted in-line where they live.

The headline finding: --allow-unsigned is non-functional for CLI users — the flag is added here but the HTTP client DTO (pkg/skills/client/dto.go) never carries it to the server, so every CLI --allow-unsigned install is rejected with a 403 telling the user to pass the flag they already passed. Details inline on cmd/thv/app/skill_install.go.

[JAORMX]

Inline comments from the panel review. Recap, by severity:

• Critical: --allow-unsigned never reaches the server — pkg/skills/client DTO drops the field (see cmd/thv/app/skill_install.go). Every CLI user of the headline flag gets a 403 telling them to pass the flag they passed. Two-line fix + a client round-trip test.
• Medium: TOFU race — lock read before per-skill mutex in both installFromGit and installFromOCI (installByName has the correct order to copy).
• Medium: sync trusts unsigned: true without re-verifying — the provenance: → unsigned: true lock-diff downgrade path should at least be documented as security-relevant review surface.
• Medium (UX): API error strings name the CLI flag; signer-mismatch is a dead-end error; TOFU pins the identity silently (RFC says "displayed prominently").
• Medium (design): the result.Provenance/Unsigned fold-back defeats opts-by-value in a way that silently drops the next decision field — consider threading *InstallOptions before PR12 grows the decision shape.

Also noted for housekeeping: this PR is ~437 lines of code changes across 12 non-test/doc/generated files (over the 400/10 guideline in .claude/rules/pr-creation.md — a natural split is the API/CLI plumbing vs the service-layer verification), and two commit subjects exceed the 50-char limit ("Update lock E2E for install-time verification", "Pass allow_unsigned through the skills install API").