Add project-level skills lock file (POC)#5715
Open
samuv wants to merge 1 commit into
Open
Conversation
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>
samuv
requested review from
ChrisJBurns,
JAORMX,
amirejaz,
aponcedeleonch,
jhrozek,
lujunsan,
rdimitrov,
reyortiz3 and
tgrunnagle
as code owners
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
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.
Collaborator
|
Love it! Let's talk on Monday about this. Shall we have an RFC too? Just for docs |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| // Load reads and parses the lock file for projectRoot. A missing lock file | ||
| // is not an error — it returns an empty Lockfile ready to be populated. | ||
| func Load(projectRoot string) (*Lockfile, error) { | ||
| data, err := os.ReadFile(Path(projectRoot)) // #nosec G304 -- projectRoot is validated by callers before reaching this package |
|
|
||
| path := Path(projectRoot) | ||
| tmp := path + ".tmp" | ||
| if err := os.WriteFile(tmp, data, 0o644); err != nil { // #nosec G306 -- lock file is committed to git, not sensitive |
| if err := os.WriteFile(tmp, data, 0o644); err != nil { // #nosec G306 -- lock file is committed to git, not sensitive | ||
| return fmt.Errorf("writing lock file: %w", err) | ||
| } | ||
| if err := os.Rename(tmp, path); err != nil { |
| if err := os.WriteFile(tmp, data, 0o644); err != nil { // #nosec G306 -- lock file is committed to git, not sensitive | ||
| return fmt.Errorf("writing lock file: %w", err) | ||
| } | ||
| if err := os.Rename(tmp, path); err != nil { |
| return fmt.Errorf("writing lock file: %w", err) | ||
| } | ||
| if err := os.Rename(tmp, path); err != nil { | ||
| _ = os.Remove(tmp) |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #5715 +/- ##
========================================
Coverage 70.62% 70.63%
========================================
Files 681 686 +5
Lines 68809 69184 +375
========================================
+ Hits 48596 48865 +269
- Misses 16664 16739 +75
- Partials 3549 3580 +31 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
Author
Yeah 🚀 , I opened this RFC: stacklok/toolhive-rfcs#80. I just refined it a bit. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
--scope projecthave no reproducible pin. Team members can't restore an identical set of skills, and there's no controlled path to pull newer content when the catalog moves. Every other package manager (npm, pnpm, Cargo, go) solves this with a committed lock file; skills didn't have one.toolhive.lock.yamlat the project root that pinsname,version,source,resolvedReference, anddigestfor every project-scoped skill install. Addsthv skill sync(restore pinned state) andthv skill upgrade(re-resolve sources, install newer content). Lock entries are client-agnostic;syncinstalls for all detected clients by default.Fixes #
Type of change
Test plan
task test)task test-e2e)task lint-fix)Unit tests cover the
lockfilepackage, the install/uninstall lock hooks,Sync,Upgrade, the new API routes, and CLI helpers (resolveProjectRoot,shortDigest).pkg/skills/...,pkg/api/v1, andcmd/thv/appall passtask test. Manual end-to-end verification ofthv skill install --scope project→sync→upgradeagainst real GHCR artifacts was done locally.API Compatibility
v1beta1API, OR theapi-break-allowedlabel is applied and the migration guidance is described above.New read-only-ish endpoints (
POST /skills/sync,POST /skills/upgrade) are additive; no existing operator CRD surface is touched.Changes
pkg/skills/lockfile/lockfile.gotoolhive.lock.yamlpkg/skills/skillsvc/install.gosource)pkg/skills/skillsvc/uninstall.gopkg/skills/skillsvc/sync.goSync— install each entry at pinnedresolvedReference@digest; report/prune unmanagedpkg/skills/skillsvc/upgrade.goUpgrade— re-resolvesource, compare digests, install + rewrite on change;--dry-run; immutable-source detectionpkg/skills/skillsvc/pin.gopinOCIReference,pinGitReference,isImmutableSource)pkg/skills/service.go,options.goSync/Upgradeon theSkillServiceinterface + option/result typespkg/skills/client/{client,dto}.gopkg/api/v1/skills.go,skills_types.goPOST /skills/sync,POST /skills/upgraderoutes + DTOscmd/thv/app/skill_sync.go,skill_upgrade.go--prune/--dry-run/--clients/--project-rootcmd/thv/app/skill_helpers.goresolveProjectRoothelper (git-root auto-detection)docs/arch/12-skills-system.mddocs/cli/*,docs/server/*Does this introduce a user-facing change?
Yes. New
thv skill syncandthv skill upgradecommands, and project-scoped installs now write atoolhive.lock.yamlat the project root (intended to be committed to git). User-scope installs are unchanged.Implementation plan
Approved implementation plan
Agreed design (from interview):
toolhive.lock.yamlat the project root (committed to git).name,version,source(what the user typed),resolvedReference(concrete OCI ref / git URL),digest(OCIsha256:...or git commit). Top-levelversion: 1; entries sorted by name for stable diffs. (AninstalledAtfield was originally proposed and later dropped to match the convention every other package manager follows — lock files store identity + source + integrity, not chronology.)syncinstalls for all detected clients (override with--clients).--scope projectoperations touch the lock; user-scope installs never do.resolvedReference@digestfor missing/drifted entries; reports unmanaged project-scope skills;--pruneuninstalls them.source, installs and rewrites the entry if the digest changed. Immutable sources (git commit,@sha256) reported as not upgradable.--dry-runprints what would change.skillsvcwith new API endpoints; CLI commands are thin wrappers. CLI auto-detects the git root from cwd for sync/upgrade,--project-rootoverrides.Flow:
flowchart LR installCmd["thv skill install --scope project"] --> svc[skillsvc install] svc --> files["client skill dirs (.claude/skills/...)"] svc --> db[(SQLite state)] svc --> lock["toolhive.lock.yaml (upsert entry)"] syncCmd["thv skill sync"] --> lock2["read lock"] --> pinned["install resolvedReference@digest"] upgradeCmd["thv skill upgrade"] --> reresolve["re-resolve source vs catalog"] --> compare{"digest changed?"} -->|yes| rewrite["install + rewrite lock entry"]Risks / notes for the POC:
gitresolversupports checkout of arbitrary commit hashes (full clone when needed).ociskills.RegistryClient.Pull()acceptsrepo@sha256:...refs.Special notes for reviewers
installedAtfield was deliberately omitted from the lock entry schema. Every major lock file (npm/pnpm/yarn/Cargo/go/poetry) stores identity + source + integrity but not chronology, for good reasons: reproducibility is guaranteed by the digest not the timestamp; timestamps are environment-local and produce meaningless merge churn on no-op regenerations; and "when was this pinned" is already answered better bygit blame.git log -p -- toolhive.lock.yamlserves any recency/audit need.sync/upgradecan repair the lock file. This mirrors the existing best-effort file-cleanup pattern inuninstall.go.SyncandUpgradecallinstallInternal(notInstall) so they don't overwrite the entry'sSourcewith an already-resolved reference —Sourcemust stay as the user's original input soupgradecan re-resolve it.TOOLHIVE_DEV=trueblindly enabled plain HTTP for all OCI pulls, breaking real-registry auth via oras-go's cross-origin redirect guard). That fix is not in this PR; it's stashed locally and will be a separate PR.Made with Cursor