Add project-level skills lock file (POC)#5715
Conversation
There was a problem hiding this comment.
Large PR Detected
This PR exceeds 1000 lines of changes and requires justification before it can be reviewed.
How to unblock this PR:
Add a section to your PR description with the following format:
## Large PR Justification
[Explain why this PR must be large, such as:]
- Generated code that cannot be split
- Large refactoring that must be atomic
- Multiple related changes that would break if separated
- Migration or data transformationAlternative:
Consider splitting this PR into smaller, focused changes (< 1000 lines each) for easier review and reduced risk.
See our Contributing Guidelines for more details.
This review will be automatically dismissed once you add the justification section.
|
Love it! Let's talk on Monday about this. Shall we have an RFC too? Just for docs |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #5715 +/- ##
==========================================
- Coverage 70.74% 69.75% -1.00%
==========================================
Files 683 704 +21
Lines 69194 70884 +1690
==========================================
+ Hits 48950 49443 +493
- Misses 16663 17741 +1078
- Partials 3581 3700 +119 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Yeah 🚀 , I opened this RFC: stacklok/toolhive-rfcs#80. I just refined it a bit. |
Large PR justification has been provided. Thank you!
|
✅ Large PR justification has been provided. The size review has been dismissed and this PR can now proceed with normal review. |
dafb744 to
ae2f41a
Compare
Introduce toolhive.lock.yaml, committed at the project root, that pins the
name, version, source, resolved reference, and digest of every project-scoped
skill install. This gives teams reproducible skill installs (restorable via
"thv skill sync") and a path to refresh pinned content when the catalog moves
("thv skill upgrade"), mirroring the role package-lock.json plays for npm.
- New pkg/skills/lockfile package: schema, load/save, and file-locked
upsert/remove using pkg/fileutils.WithFileLock; entries sorted by name for
stable diffs.
- skillsvc install/uninstall hooks: project-scope installs upsert a lock
entry (recording the original source string for later re-resolution);
uninstalls remove it. User-scope installs never touch the lock.
- Sync: installs each entry strictly at its pinned resolvedReference@digest,
reports unmanaged project-scope skills, and prunes them with --prune.
- Upgrade: re-resolves each entry's original source, compares digests, and
installs + rewrites the entry on change; immutable sources (OCI digest,
git commit) reported as not-upgradable; --dry-run supported.
- API: POST /skills/sync and POST /skills/upgrade plus skills client methods.
- CLI: thv skill sync and thv skill upgrade with git-root auto-detection and
--project-root override; JSON/text output.
- Docs: regenerated CLI/swagger docs; updated docs/arch/12-skills-system.md
with a Project Lock File section, diagram, and endpoint/CLI tables.
Co-authored-by: Cursor <cursoragent@cursor.com>
Add requiredBy and explicit fields, load-time validation, and a deterministic dirhash helper for on-disk integrity verification. Co-authored-by: Cursor <cursoragent@cursor.com>
Move sync and upgrade behind a dedicated lock service interface with typed options, results, and regenerated mocks. Co-authored-by: Cursor <cursoragent@cursor.com>
Track whether a skill was installed by the lock file so sync and uninstall can distinguish managed from ad-hoc installations. Co-authored-by: Cursor <cursoragent@cursor.com>
Fail on lock write errors, record content digests, recursively install transitive dependencies, and cascade uninstall orphaned lock entries. Co-authored-by: Cursor <cursoragent@cursor.com>
Re-hash content digests, reinstall drifted skills, harden pruning, and support dry-run check plus divergent client directory adoption. Co-authored-by: Cursor <cursoragent@cursor.com>
Add preview mode, fail-on-changes, allow-ref-change, and typed failure reasons for blocked reference changes and content drift. Co-authored-by: Cursor <cursoragent@cursor.com>
Expose sync and upgrade on the lock service with typed request bodies, pre-install confirmation gates, and exit codes 2/3/4 for partial failure. Co-authored-by: Cursor <cursoragent@cursor.com>
Update architecture notes and regenerate CLI and swagger docs for new flags, exit codes, and lock file integrity semantics. Co-authored-by: Cursor <cursoragent@cursor.com>
Codespell flags UpToDate as a misspelling; rename the Go field to AlreadyCurrent while keeping the JSON key up_to_date unchanged. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
661d209 to
1f1db15
Compare
CI has no cosign key or keyless OIDC configured, so the push-then- install E2E test failed once push started signing by default. Co-authored-by: Cursor <cursoragent@cursor.com>
Proposed split for easier reviewOf course this PR is not something mergeable as-is — it bundles two logically separate efforts (~6.6k lines / 74 files) into one diff: the project-scope skills lock file (RFC THV-0080 POC) and Sigstore signing/verification v1 on top of it. The idea is to split the work into 14 smaller PRs, stacked across two dependency chains, so each one is independently reviewable, buildable, and testable ( Stack 1 — Lock file POC (6 PRs, ~4.5k LOC)
Stack 2 — Sigstore v1 (8 PRs, ~2.1k LOC, based on
|
| PR | Scope | ~Size |
|---|---|---|
| 7 | Add provenance/unsigned fields to lock schema |
~100 LOC |
| 8 | pkg/skills/verifier package (OCI referrer, gitsign, offline bundle verify) + go.mod |
~860 LOC (~150 generated mock) |
| 9 | pkg/skills/signer package (key-based signing, Cosign referrer attach) |
~330 LOC |
| 10 | Persist Sigstore bundle (migration 004) + expose git commit signature | ~90 LOC |
| 11 | Verify signatures on project-scope install, TOFU identity recording | ~480 LOC |
| 12 | Offline bundle re-verification in sync --check |
~50 LOC |
| 13 | Block upgrades on signer identity change, --allow-signer-change |
~100 LOC |
| 14 | Sign on push (--key/--no-sign), CLI/API flags, trust-model docs |
~260 LOC |
Notes
- This is a rewrite-by-file, not a cherry-pick of the existing 19 commits — a couple of the original commits (the initial lock POC commit and the install-verification commit) mix concerns that need to be separated across PRs, and one test fix needs to move earlier so each PR is green on its own.
go.mod/go.sum(newsigstore/sigstore-godeps) land in PR8; generated docs land in PR6 and PR14 (the two PRs that change API/CLI surface).- PR11 is the largest and highest-risk PR in Stack 2 (core signature-enforcement logic) — flagging it now for extra reviewer attention.
If this split makes sense, I'll open the branch chain (skills-lock/01-lockfile → … → skills-lock/14-sigstore-cli) and close this PR in favor of the stack, linking back here for context. Let me know if you'd prefer a different grouping (e.g. fewer, larger PRs) before I start splitting.
Summary
sync/upgradewould restore or advance skills safely.toolhive.lock.yamlfor project scope withthv skill syncandthv skill upgrade, then fold RFC THV-0080 v1 Sigstore signing/verification into the same POC: verify on consume, sign on push, pin publisher identity (provenance) or recordunsigned: true, and re-verify stored bundles offline insync --check.POC scope (RFC THV-0080)
This PR is a feasibility vertical slice, not production-hardening split across many small PRs yet. It proves:
source,resolvedReference,digest,contentDigestcontentDigestdirhash +sync --checksync --adoptwrites lock entries (with provenance orunsigned)toolhive.requiresmaterialized;requiredBy+ cascade uninstall--allow-ref-change;--allow-signer-changethv skill pushsigns with--key(cosign);--no-signescape hatch--yes, exit codes 2/3/4Trust model (v1)
Three layers (see
docs/arch/12-skills-system.md):sync --checkprovenance.signerIdentity+provenance.certIssuerpinned at first verified install;signer-mismatchon divergencetoolhive.lock.yaml;unsigned: trueis an explicit reviewed exceptionDeferred: keyless OIDC push (E2E against Sigstore staging); catalog-supplied expected identity (needs toolhive-core
ProvenanceonSkilltypes).Lock entry schema (v1)
CLI surface
thv skill sync—--check,--adopt,--prune,--yes,--clients,--project-rootthv skill upgrade [name...]—--preview,--fail-on-changes,--allow-ref-change,--allow-signer-change,--yesthv skill install—--allow-unsigned(project scope)thv skill push—--key,--no-signExit codes:
0success ·2check/freshness failure ·3partial failure ·4policy rejectionCommit structure (19 commits, atomic by layer)
Lock file core
Sigstore v1
Type of change
Test plan
task test—pkg/skills/...passes)task lint-fix)sync --checkAPI compatibility
Additive:
POST /skills/sync,POST /skills/upgrade; new request fields (allowUnsigned,allowSignerChange,key,skipSigning).SkillLockServiceis separate fromSkillService. No operator CRD changes.Large PR justification
~6.6k lines / 74 files. Kept unified for POC feasibility: lock integrity, sync/upgrade guardrails, and Sigstore trust are coupled — splitting mid-stack leaves non-functional intermediate states. If validated, split into the stack below for production merge.
Review focus
contentDigestthe right integrity primitive?requiredBy/explicit) correct?--checksufficient?SkillLockServicethe right API boundary?Does this introduce a user-facing change?
Yes — new
sync/upgradecommands, lock file on project install, Sigstore flags, push signing.