From 0ceea768633ba6350c50b47a52da0bfb5c66afe9 Mon Sep 17 00:00:00 2001 From: Samuele Verzi Date: Fri, 3 Jul 2026 16:40:18 +0200 Subject: [PATCH 1/6] Add RFC for project-level skills lock file Co-authored-by: Cursor --- rfcs/0080-skills-lock-file.md | 228 ++++++++++++++++++++++++++++++++++ 1 file changed, 228 insertions(+) create mode 100644 rfcs/0080-skills-lock-file.md diff --git a/rfcs/0080-skills-lock-file.md b/rfcs/0080-skills-lock-file.md new file mode 100644 index 0000000..4068417 --- /dev/null +++ b/rfcs/0080-skills-lock-file.md @@ -0,0 +1,228 @@ +# RFC-0080: Project-Level Skills Lock File + +- **Status**: Draft +- **Author(s)**: Samu Vannier (@samuv) +- **Created**: 2026-07-03 +- **Last Updated**: 2026-07-03 +- **Target Repository**: toolhive +- **Related Issues**: [toolhive#5715](https://github.com/stacklok/toolhive/pull/5715) + +## Summary + +Add a project-level skills lock file (`toolhive.lock.yaml`) that pins the name, version, source, resolved reference, and digest of every project-scoped skill install, plus `thv skill sync` and `thv skill upgrade` commands to restore pinned state and pull newer content from the catalog. This brings to skills the reproducibility guarantees that `package-lock.json`, `Cargo.lock`, and `go.sum` provide for other ecosystems. + +## Problem Statement + +- **Current behavior:** `thv skill install --scope project` writes files to client skill dirs and a SQLite record, but nothing pins *which* content was installed in a shareable, version-controlled form. Two teammates cloning the same repo get whatever the catalog currently serves, not what the original installer intended. +- **Who is affected:** any team using project-scoped skills for shared workflows (code-review conventions, testing skills, org-specific instructions). Reproducibility and controlled upgrades are both missing. +- **Why worth solving:** skills are supply-chain artifacts (instructions an AI assistant follows). Unpinned installs mean silent drift across machines and no auditable path from "what we agreed to use" to "what's actually installed". Every comparable package manager solved this with a committed lock file; skills currently have no equivalent. + +## Goals + +- Pin the exact content digest of every project-scoped skill install in a file committed to the project repo. +- Restore an identical skill set on any machine via `thv skill sync`. +- Provide a controlled, digest-based upgrade path via `thv skill upgrade`. +- Keep the lock client-agnostic by pinning content, not which client apps installed it. +- Stay out of user-scope installs entirely. + +## Non-Goals + +- Version-constraint resolution or manifest-driven installs. There is no `toolhive.yaml` with `^1.0.0` ranges in this proposal. The lock file is the declaration of intent; sync installs exactly what's pinned. +- Per-client pinning. The lock pins content; sync installs for all detected clients, overridable with `--clients`. +- A dependency resolver or transitive dependency graph for skill-on-skill dependencies. +- Lock-file entries for user-scope installs. + +## Proposed Solution + +Use a single lock file written by install commands. `--scope project` installs upsert an entry; uninstalls remove it. `sync` reinstalls at the pinned `resolvedReference@digest`; `upgrade` re-resolves the original `source` and rewrites the entry if the digest changed. + +### High-Level Design + +```mermaid +flowchart LR + installCmd["thv skill install --scope project"] --> svc[skillsvc install] + svc --> files["client skill dirs"] + svc --> db[(SQLite state)] + svc --> lock["toolhive.lock.yaml (upsert)"] + syncCmd["thv skill sync"] --> lock2["read lock"] --> pinned["install resolvedReference@digest"] + upgradeCmd["thv skill upgrade"] --> reresolve["re-resolve source"] --> compare{"digest changed?"} -->|yes| rewrite["install + rewrite entry"] +``` + +### Detailed Design + +The lock file has a top-level `version: 1` field and a deterministic list of skill entries sorted by name for stable diffs: + +```yaml +version: 1 +skills: + - name: code-review + version: 1.0.0 + source: code-review + resolvedReference: ghcr.io/org/code-review:1.0.0 + digest: sha256:9f2b1e... +``` + +Each entry stores `name`, optional `version` from `SKILL.md` frontmatter, `source`, `resolvedReference`, and `digest`. `source` is the original user input, such as a plain registry name, OCI reference, or `git://` reference, preserved verbatim so upgrade can re-resolve it. `resolvedReference` is the concrete OCI reference or git URL that source resolved to. `digest` is either an OCI `sha256:...` digest or a git commit hash. + +The schema deliberately omits `installedAt`. Every major lock file, including npm, pnpm, yarn, Cargo, Go, and Poetry, stores identity, source, and integrity, not chronology. Reproducibility is guaranteed by the digest, not a timestamp; timestamps are environment-local and produce meaningless merge churn on no-op regenerations. "When was this pinned" is answered better by `git blame`, and `git log -p -- toolhive.lock.yaml` serves any recency or audit need. + +#### Component Changes + +- Add a new `pkg/skills/lockfile` package for schema handling, load/save, and file-locked upsert/remove using `pkg/fileutils.WithFileLock` in the same pattern as config writes. Marshalling is deterministic, with sorted entries. +- Update `pkg/skills/skillsvc` install and uninstall hooks so project-scope installs upsert a lock entry recording the original `opts.Name` as `Source` before any internal resolution, and uninstalls remove it. Lock write errors are logged but never fail the install or uninstall because the skill's files and DB record are already correct, and a subsequent sync or upgrade can repair the lock. User-scope installs never touch the lock. +- Add `Sync`, which reads the lock, compares each entry against `SkillStore` state, installs strictly by pinned `resolvedReference@digest` using OCI pull-by-digest or git clone plus checkout of the pinned commit, and reports unmanaged project-scope skills. With `--prune`, it uninstalls unmanaged skills. It never rewrites the lock; it only makes FS/DB match what's pinned. +- Add `Upgrade`, which re-resolves each entry's `source` exactly as a fresh `thv skill install ` would. Registry names resolve through the catalog, git branches/tags resolve to current heads, and OCI tags resolve to current digests. If the digest changed, upgrade installs and rewrites the entry. Immutable sources such as OCI `@sha256:` digests or full 40-character git commit hashes are reported as `not-upgradable` without contacting the network. `--dry-run` prints what would change. `source` is never rewritten, so future upgrades keep re-resolving the same input. +- Have `Sync` and `Upgrade` call `installInternal`, not `Install`, so they do not overwrite the entry's `Source` with an already-resolved reference. + +#### API Changes + +This proposal is additive and introduces no breaking changes: + +- `POST /skills/sync` with body `{projectRoot, clients, prune}` returns a report containing installed, up-to-date, unmanaged, pruned, and failed skills. +- `POST /skills/upgrade` with body `{projectRoot, names, dryRun, clients}` returns per-skill outcomes of upgraded, up-to-date, not-upgradable, or failed, including old and new digests where relevant. +- `SkillService` gains `Sync` and `Upgrade` methods, and `pkg/skills/client` gains corresponding HTTP methods. + +#### Configuration Changes + +None. The lock file is project-level, discovered by auto-detecting the git root from the current working directory, using the nearest enclosing `.git` directory. `--project-root` provides an override. No global config is added. + +#### Data Model Changes + +No SQLite schema changes are required. The lock file is a new committed YAML artifact at the project root, while the SQLite `installed_skills` table continues to hold runtime install records. The lock is the shareable pin; SQLite is the local install state. `sync` reconciles the two. + +## Security Considerations + +### Threat Model + +A malicious or compromised lock file, such as one introduced through a tampered PR, could pin a skill to a known-bad digest. The threat is an attacker committing a lock entry pointing to malicious skill content that teammates then sync. The relevant attacker capabilities are write access to the project repo or the ability to submit a PR that is merged. + +### Authentication and Authorization + +Lock-file writes happen server-side in `skillsvc`, gated by the same API auth as existing skill install/uninstall operations. This proposal introduces no new auth surface. `sync` and `upgrade` reuse the existing OCI Docker credential chain and git token auth paths, with no new credential handling. + +### Data Security + +The lock file contains no secrets, only public references and content digests. It is committed to git by design. No sensitive data is stored or transmitted beyond what `thv skill install` already transmits. + +### Input Validation + +Lock entries are validated on load via the same `ValidateSkillName` used for installs. `resolvedReference` and `digest` are passed through the existing OCI and git resolution paths, which already enforce SSRF guards such as no localhost/private IPs in git refs except dev mode, shell-injection reference validation, and supply-chain checks such as requiring artifact skill names to match OCI repo paths. A tampered lock can only pin references that would also pass a manual `thv skill install`. + +### Secrets Management + +None. The lock stores no tokens. Registry and git authentication use the existing per-process credential chain. + +### Audit and Logging + +Lock upsert/remove errors are logged via `slog.Warn` with skill name and project root. `sync` and `upgrade` return structured reports covering installed, up-to-date, unmanaged, pruned, failed, upgraded, and not-upgradable states suitable for audit. The lock file itself, being git-committed, gives a full history of what was pinned when, which is stronger cross-machine provenance than DB-only audit. + +### Mitigations + +- Pinned-digest installs mean `sync` reproduces exact bytes, and a merged lock entry is auditable in `git blame`. +- `upgrade` refuses to upgrade entries already pinned to a digest or commit, preventing accidental re-resolution of intentionally locked content. +- Best-effort lock writes mean a failed lock write cannot corrupt an install because files and DB state are already correct, and `sync` can repair drift. + +## Alternatives Considered + +### Alternative 1: Separate manifest and lock file + +- **Description:** A hand-edited `toolhive.yaml` declares desired skills with version constraints, and the lock resolves them. +- **Pros:** Supports `^1.0.0` ranges; upgrade is "re-resolve constraints". +- **Cons:** Requires building a dependency resolver; doubles the surface area with two files to keep in sync; skills do not have a rich enough versioning or constraint ecosystem to justify it yet. +- **Why not chosen:** The POC scope is narrower. The single-lock model delivers reproducibility now and leaves the door open to a manifest layer later without rework. + +### Alternative 2: Store pin data only in the SQLite store + +- **Description:** No committed file; sync reads from a shared or replicated DB. +- **Pros:** No new file format. +- **Cons:** Not portable across machines; cannot be reviewed in a PR; no `git blame` provenance; defeats the committed, auditable pin goal entirely. +- **Why not chosen:** This approach fundamentally does not meet the reproducibility and audit goals. + +### Alternative 3: Per-client lock entries + +- **Description:** Pin which client apps each skill installs into. +- **Pros:** Precise per-client control. +- **Cons:** Couples the lock to the local client set; bloats entries; one teammate without Cursor installed cannot sync cleanly. +- **Why not chosen:** Content pinning is the real need; client targeting is a local concern handled at sync time by `--clients` or detected clients. + +## Compatibility + +### Backward Compatibility + +This change is fully backward compatible. Existing project-scope installs simply start writing a lock file. Pre-existing installs without a lock entry still work; sync reports them as unmanaged and does not touch them unless `--prune` is set. No migration is required because the lock file is additive. User-scope installs are entirely unchanged. + +### Forward Compatibility + +The top-level `version: 1` schema field allows future schema evolution. The `source` field is the extensibility hook for a future manifest/resolver layer; a v2 schema could add constraint expressions alongside `source` without breaking v1 readers. New entry fields can be added with `omitempty`. + +## Implementation Plan + +A POC implementation already exists in [toolhive#5715](https://github.com/stacklok/toolhive/pull/5715). Post-RFC acceptance, split it into reviewable PRs. + +### Phase 1: Lock file package + +- Add the `pkg/skills/lockfile` schema and file-locked operations. + +### Phase 2: Install/uninstall hooks + +- Add project-scope install upsert and uninstall remove behavior. + +### Phase 3: Sync + +- Add the `Sync` service method, API, and `thv skill sync` CLI. + +### Phase 4: Upgrade + +- Add the `Upgrade` service method, API, and `thv skill upgrade` CLI. + +### Phase 5: Docs + +- Update `docs/arch/12-skills-system.md`, CLI docs, and swagger. + +### Dependencies + +None blocking. This proposal relies on existing `gitresolver` commit checkout and `ociskills.RegistryClient` digest pulls, both of which are already capable of supporting the design. + +## Testing Strategy + +- **Unit tests:** `lockfile` package round-trip, upsert/remove, and missing-file behavior; skillsvc hooks for project versus user scope; non-fatal lock write failures; `Sync` drift detection, unmanaged reports, and prune behavior; `Upgrade` digest changes, immutable sources, and dry-run behavior; API routes; CLI helpers. +- **E2E tests:** `thv skill install --scope project`, then `sync` on a clean tree, then `upgrade` after a catalog bump, against real GHCR artifacts. +- **Security tests:** confirm a tampered lock can only pin to references that pass existing supply-chain validation. + +## Documentation + +- Update `docs/arch/12-skills-system.md` with a new "Project Lock File" section, as done in the POC PR. +- Regenerate CLI docs via `task docs`. +- Add a short user-facing guide on committing `toolhive.lock.yaml` and running `sync` after pull. + +## Open Questions + +1. Should `sync` warn or refuse when the local SQLite state has a different digest for a skill that is in the lock, or silently reinstall to the pinned digest? The POC currently reinstalls silently. +2. Should the lock support an optional `clients` field per entry for teams that genuinely want per-client pinning, or stay strictly client-agnostic? +3. Should `upgrade` without args default to all entries, as in the current POC, or require explicit selection to avoid accidental mass upgrades in CI? +4. Where should a future manifest layer live, `toolhive.yaml` or `toolhive.skills.yaml`, if and when constraints are added, and should the lock file name change to match? + +## References + +- POC PR: +- Research note that motivated this: +- Prior art: npm `package-lock.json`, pnpm `pnpm-lock.yaml`, `Cargo.lock`, `go.sum`, `poetry.lock` +- oras-go cross-origin redirect auth guard, relevant to pinned-digest pulls: GHSA-vh4v-2xq2-g5cg + +--- + +## RFC Lifecycle + + + +### Review History + +| Date | Reviewer | Decision | Notes | +|------|----------|----------|-------| +| YYYY-MM-DD | @reviewer | Under Review | Initial submission | + +### Implementation Tracking + +| Repository | PR | Status | +|------------|----|--------| +| toolhive | [#5715](https://github.com/stacklok/toolhive/pull/5715) | POC | From dd017f672a7daedba82d153bd46b35e76942503d Mon Sep 17 00:00:00 2001 From: Samuele Verzi Date: Fri, 3 Jul 2026 16:42:20 +0200 Subject: [PATCH 2/6] Rename skills lock RFC file Co-authored-by: Cursor --- rfcs/{0080-skills-lock-file.md => THV-0080-skills-lock-file.md} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename rfcs/{0080-skills-lock-file.md => THV-0080-skills-lock-file.md} (100%) diff --git a/rfcs/0080-skills-lock-file.md b/rfcs/THV-0080-skills-lock-file.md similarity index 100% rename from rfcs/0080-skills-lock-file.md rename to rfcs/THV-0080-skills-lock-file.md From b606b909d13166471a27cc3bb2f69e521d27925d Mon Sep 17 00:00:00 2001 From: Samuele Verzi Date: Fri, 3 Jul 2026 16:44:59 +0200 Subject: [PATCH 3/6] Fix author name in skills lock RFC Co-authored-by: Cursor --- rfcs/THV-0080-skills-lock-file.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rfcs/THV-0080-skills-lock-file.md b/rfcs/THV-0080-skills-lock-file.md index 4068417..f651d7a 100644 --- a/rfcs/THV-0080-skills-lock-file.md +++ b/rfcs/THV-0080-skills-lock-file.md @@ -1,7 +1,7 @@ # RFC-0080: Project-Level Skills Lock File - **Status**: Draft -- **Author(s)**: Samu Vannier (@samuv) +- **Author(s)**: Samuele Verzi (@samuv) - **Created**: 2026-07-03 - **Last Updated**: 2026-07-03 - **Target Repository**: toolhive From bea4c3731c1be9b38d1fa38bfce6ee42e1ade426 Mon Sep 17 00:00:00 2001 From: Samuele Verzi Date: Fri, 3 Jul 2026 16:57:30 +0200 Subject: [PATCH 4/6] Refine skills lock RFC with design decisions Co-authored-by: Cursor --- rfcs/THV-0080-skills-lock-file.md | 75 +++++++++++++++++++++---------- 1 file changed, 52 insertions(+), 23 deletions(-) diff --git a/rfcs/THV-0080-skills-lock-file.md b/rfcs/THV-0080-skills-lock-file.md index f651d7a..f23ab3b 100644 --- a/rfcs/THV-0080-skills-lock-file.md +++ b/rfcs/THV-0080-skills-lock-file.md @@ -20,8 +20,9 @@ Add a project-level skills lock file (`toolhive.lock.yaml`) that pins the name, ## Goals - Pin the exact content digest of every project-scoped skill install in a file committed to the project repo. -- Restore an identical skill set on any machine via `thv skill sync`. +- Restore the pinned skill set on any machine via `thv skill sync`. - Provide a controlled, digest-based upgrade path via `thv skill upgrade`. +- Let CI verify that installed state matches the lock via `thv skill sync --check`. - Keep the lock client-agnostic by pinning content, not which client apps installed it. - Stay out of user-scope installs entirely. @@ -50,7 +51,9 @@ flowchart LR ### Detailed Design -The lock file has a top-level `version: 1` field and a deterministic list of skill entries sorted by name for stable diffs: +`toolhive.lock.yaml` is the general ToolHive project lock file, not a skills-only artifact. Schema version 1 defines only the `skills:` key, but the top-level structure deliberately leaves room for future artifact types (for example a `plugins:` key for the plugins system) to join as sibling keys without a schema break. Loading a lock file with an unknown or newer top-level `version` is a hard error with a clear "upgrade thv" message; it is never silently ignored or partially parsed. + +The lock file has a top-level `version: 1` field and a deterministic list of skill entries sorted by name for stable diffs. Entries pin either OCI or git content: ```yaml version: 1 @@ -60,31 +63,45 @@ skills: source: code-review resolvedReference: ghcr.io/org/code-review:1.0.0 digest: sha256:9f2b1e... + - name: testing-conventions + source: git://github.com/org/skills.git#main/testing-conventions + resolvedReference: git://github.com/org/skills.git + digest: 4f0c9a1d2e8b7c6a5f4e3d2c1b0a9f8e7d6c5b4a ``` -Each entry stores `name`, optional `version` from `SKILL.md` frontmatter, `source`, `resolvedReference`, and `digest`. `source` is the original user input, such as a plain registry name, OCI reference, or `git://` reference, preserved verbatim so upgrade can re-resolve it. `resolvedReference` is the concrete OCI reference or git URL that source resolved to. `digest` is either an OCI `sha256:...` digest or a git commit hash. +Each entry stores `name`, optional `version` from `SKILL.md` frontmatter, `source`, `resolvedReference`, and `digest`. `source` is the original user input, such as a plain registry name, OCI reference, or `git://` reference, preserved verbatim so upgrade can re-resolve it. `resolvedReference` is the concrete OCI reference or git URL that source resolved to. `digest` is either an OCI `sha256:...` digest or a git commit hash, as in the two entries above. The schema deliberately omits `installedAt`. Every major lock file, including npm, pnpm, yarn, Cargo, Go, and Poetry, stores identity, source, and integrity, not chronology. Reproducibility is guaranteed by the digest, not a timestamp; timestamps are environment-local and produce meaningless merge churn on no-op regenerations. "When was this pinned" is answered better by `git blame`, and `git log -p -- toolhive.lock.yaml` serves any recency or audit need. +The lock stays strictly client-agnostic: there is no per-entry `clients` field. Client sets are machine-local (one teammate may not have a given client installed at all), so client targeting is a sync-time concern handled by `--clients` or client auto-detection. If genuine demand for per-client pinning appears, an optional field can be added later with `omitempty` without a schema break. + +Skill dependencies (`toolhive.requires` in `SKILL.md` frontmatter, recorded in the SQLite `skill_dependencies` table) are not locked in v1. Dependencies materialized by an install are recorded in SQLite but get no lock entry of their own, so they surface as *unmanaged* in sync reports. Proper dependency locking (entries with a machine-generated provenance marker, or a nested structure) is future work; until then, teams that want dependencies pinned should install them explicitly at project scope. + #### Component Changes -- Add a new `pkg/skills/lockfile` package for schema handling, load/save, and file-locked upsert/remove using `pkg/fileutils.WithFileLock` in the same pattern as config writes. Marshalling is deterministic, with sorted entries. -- Update `pkg/skills/skillsvc` install and uninstall hooks so project-scope installs upsert a lock entry recording the original `opts.Name` as `Source` before any internal resolution, and uninstalls remove it. Lock write errors are logged but never fail the install or uninstall because the skill's files and DB record are already correct, and a subsequent sync or upgrade can repair the lock. User-scope installs never touch the lock. -- Add `Sync`, which reads the lock, compares each entry against `SkillStore` state, installs strictly by pinned `resolvedReference@digest` using OCI pull-by-digest or git clone plus checkout of the pinned commit, and reports unmanaged project-scope skills. With `--prune`, it uninstalls unmanaged skills. It never rewrites the lock; it only makes FS/DB match what's pinned. -- Add `Upgrade`, which re-resolves each entry's `source` exactly as a fresh `thv skill install ` would. Registry names resolve through the catalog, git branches/tags resolve to current heads, and OCI tags resolve to current digests. If the digest changed, upgrade installs and rewrites the entry. Immutable sources such as OCI `@sha256:` digests or full 40-character git commit hashes are reported as `not-upgradable` without contacting the network. `--dry-run` prints what would change. `source` is never rewritten, so future upgrades keep re-resolving the same input. +- Add a new `pkg/skills/lockfile` package for schema handling, load/save, and file-locked upsert/remove using `pkg/fileutils.WithFileLock` in the same pattern as config writes. Marshalling is deterministic, with sorted entries. Entry fields are validated at load time (see Input Validation), since the lock file is the one hand-editable input this feature introduces. +- Update `pkg/skills/skillsvc` install and uninstall hooks so project-scope installs upsert a lock entry recording the original `opts.Name` as `Source` before any internal resolution, and uninstalls remove it. Lock write errors never fail the install or uninstall, because the skill's files and DB record are already correct and a subsequent sync or upgrade can repair the lock; however, the failure is not merely logged server-side. It is surfaced as a warning field in the API response and printed by the CLI, so the user knows the committed lock is now stale. User-scope installs never touch the lock. +- Add `Sync`, which reads the lock, compares each entry against `SkillStore` state, installs strictly by pinned `resolvedReference@digest` using OCI pull-by-digest or git clone plus checkout of the pinned commit, and reports unmanaged project-scope skills. A locked skill whose local record has a *different* digest is reinstalled to the pinned digest and reported distinctly as `drifted`, not lumped into `installed`. With `--prune`, it uninstalls unmanaged skills. It never rewrites the lock; it only makes FS/DB match what's pinned. +- Drift detection compares the lock against SQLite install records, not re-hashed file content. If a user hand-edits or deletes installed skill files without going through `thv`, the DB still reports the pinned digest and sync will report `upToDate` without repairing the files. Similarly, a digest match says nothing about which clients hold the files: a client installed after the original install will have an empty skill directory yet the entry still reports `upToDate`. Content-hash verification and per-client file presence checks are noted as future work; the pragmatic remedy today is `thv skill install` of the affected skill. +- Add `--check` to `thv skill sync`: it computes the same diff sync would apply but changes nothing, exiting non-zero if any entry is missing or drifted. This gives CI a cheap gate analogous to `npm ci` or `cargo --locked`. +- Add `Upgrade`, which re-resolves each entry's `source` exactly as a fresh `thv skill install ` would. Registry names resolve through the catalog, git branches/tags resolve to current heads, and OCI tags resolve to current digests. If the digest changed, upgrade installs and rewrites the entry. Immutable sources such as OCI `@sha256:` digests or full 40-character git commit hashes are reported as `not-upgradable` without contacting the network. `source` is never rewritten, so future upgrades keep re-resolving the same input. +- Bare `thv skill upgrade` upgrades all lock entries, matching `npm update` and `cargo update` conventions. The safety net is `--dry-run` plus the fact that an upgrade only propagates to teammates once the lock diff is reviewed and committed. +- `--dry-run` prints what would change but is not fully side-effect-free: there is no "peek" API for OCI or git, so dry-run still pulls the artifact into the local cache or clones the repo, skipping only extraction and FS/DB writes. +- `Sync` and `Upgrade` process every entry even when some fail (registry down, digest garbage-collected): failures are reported per skill in the structured report, successful installs stay in place, and the CLI exits non-zero if any entry failed. During upgrade, the lock is rewritten per entry as each upgrade lands; a failure after install but before the lock rewrite leaves lock and reality briefly inconsistent, which the report flags and the next sync or upgrade repairs. - Have `Sync` and `Upgrade` call `installInternal`, not `Install`, so they do not overwrite the entry's `Source` with an already-resolved reference. #### API Changes -This proposal is additive and introduces no breaking changes: +This proposal is additive, with no breaking HTTP API changes (adding `Sync` and `Upgrade` to the `SkillService` Go interface does break external implementers of that interface, if any exist): -- `POST /skills/sync` with body `{projectRoot, clients, prune}` returns a report containing installed, up-to-date, unmanaged, pruned, and failed skills. +- `POST /skills/sync` with body `{projectRoot, clients, prune, check}` returns a report containing installed, drifted, up-to-date, unmanaged, pruned, and failed skills. With `check: true` the report is computed but nothing is installed or pruned. - `POST /skills/upgrade` with body `{projectRoot, names, dryRun, clients}` returns per-skill outcomes of upgraded, up-to-date, not-upgradable, or failed, including old and new digests where relevant. +- Install responses gain an optional warning field used when the lock write fails after a successful project-scope install. - `SkillService` gains `Sync` and `Upgrade` methods, and `pkg/skills/client` gains corresponding HTTP methods. #### Configuration Changes -None. The lock file is project-level, discovered by auto-detecting the git root from the current working directory, using the nearest enclosing `.git` directory. `--project-root` provides an override. No global config is added. +None. The lock file is project-level, discovered by auto-detecting the git root from the current working directory, using the nearest enclosing `.git` directory. `--project-root` provides an override. If there is no enclosing git repository and no `--project-root`, the command errors rather than guessing; monorepo sub-projects that want their own lock are supported only via an explicit `--project-root ` (the lock lives wherever it points). No global config is added. #### Data Model Changes @@ -94,7 +111,12 @@ No SQLite schema changes are required. The lock file is a new committed YAML art ### Threat Model -A malicious or compromised lock file, such as one introduced through a tampered PR, could pin a skill to a known-bad digest. The threat is an attacker committing a lock entry pointing to malicious skill content that teammates then sync. The relevant attacker capabilities are write access to the project repo or the ability to submit a PR that is merged. +A malicious or compromised lock file, such as one introduced through a tampered PR, could pin a skill to a known-bad digest. The threat is an attacker committing a lock entry pointing to malicious skill content that teammates then sync. The relevant attacker capabilities are write access to the project repo or the ability to submit a PR that is merged. `sync` amplifies this threat compared to manual installs: it turns "review what you install" into "run one command after `git pull`", silently installing instructions an AI assistant will follow, which is why lock diffs deserve the same review scrutiny as code. + +Two related considerations: + +- The two pin types do not carry equal integrity guarantees. OCI `sha256:` digests are content-addressed and collision-resistant; git commit hashes are SHA-1 for most repositories, where collisions have been demonstrated. This is an accepted trade-off inherited from git itself; future skill signing (e.g. Sigstore, as floated in THV-0030) would mitigate it. +- `sync --prune` is a destructive operation driven by a committed file: a tampered lock that *removes* entries, combined with a habitual `sync --prune`, deletes legitimate skills. Prune therefore stays opt-in and pruned skills are itemized in the report. ### Authentication and Authorization @@ -106,7 +128,9 @@ The lock file contains no secrets, only public references and content digests. I ### Input Validation -Lock entries are validated on load via the same `ValidateSkillName` used for installs. `resolvedReference` and `digest` are passed through the existing OCI and git resolution paths, which already enforce SSRF guards such as no localhost/private IPs in git refs except dev mode, shell-injection reference validation, and supply-chain checks such as requiring artifact skill names to match OCI repo paths. A tampered lock can only pin references that would also pass a manual `thv skill install`. +The lock file is the one hand-editable input this feature introduces, so entries are validated at the trust boundary: `lockfile.Load` validates entry names via the same `ValidateSkillName` used for installs and rejects malformed digests before any entry is acted on. (The POC instead relies on downstream parsing — `ParseGitReference` and `nameref.ParseReference` during `buildPinnedReference`, plus the full install validation path — which is effective but harder to reason about; load-time validation is the specified design.) Beyond load, `resolvedReference` and `digest` still flow through the existing OCI and git resolution paths, which enforce SSRF guards such as no localhost/private IPs in git refs except dev mode, shell-injection reference validation, and supply-chain checks such as requiring artifact skill names to match OCI repo paths. A tampered lock can only pin references that would also pass a manual `thv skill install`. + +The `projectRoot` accepted by the sync/upgrade API bodies is validated the same way existing project-scope install paths are, so the daemon cannot be directed to read or write lock files in arbitrary unrelated locations. ### Secrets Management @@ -114,13 +138,14 @@ None. The lock stores no tokens. Registry and git authentication use the existin ### Audit and Logging -Lock upsert/remove errors are logged via `slog.Warn` with skill name and project root. `sync` and `upgrade` return structured reports covering installed, up-to-date, unmanaged, pruned, failed, upgraded, and not-upgradable states suitable for audit. The lock file itself, being git-committed, gives a full history of what was pinned when, which is stronger cross-machine provenance than DB-only audit. +Lock upsert/remove errors are logged via `slog.Warn` with skill name and project root, and additionally surfaced to the caller as a warning in the API response and CLI output. `sync` and `upgrade` return structured reports covering installed, drifted, up-to-date, unmanaged, pruned, failed, upgraded, and not-upgradable states suitable for audit. The lock file itself, being git-committed, gives a full history of what was pinned when, which is stronger cross-machine provenance than DB-only audit. ### Mitigations -- Pinned-digest installs mean `sync` reproduces exact bytes, and a merged lock entry is auditable in `git blame`. +- Pinned-digest installs mean `sync` reproduces the exact pinned content, and a merged lock entry is auditable in `git blame`. - `upgrade` refuses to upgrade entries already pinned to a digest or commit, preventing accidental re-resolution of intentionally locked content. -- Best-effort lock writes mean a failed lock write cannot corrupt an install because files and DB state are already correct, and `sync` can repair drift. +- Best-effort lock writes mean a failed lock write cannot corrupt an install because files and DB state are already correct; the failure is surfaced to the user, and `sync` can repair drift recorded in the DB (it does not detect manual edits to installed files; see Detailed Design). +- `sync` reports new, drifted, and pruned entries per skill, keeping the "one command after `git pull`" flow observable rather than silent. ## Alternatives Considered @@ -153,7 +178,7 @@ This change is fully backward compatible. Existing project-scope installs simply ### Forward Compatibility -The top-level `version: 1` schema field allows future schema evolution. The `source` field is the extensibility hook for a future manifest/resolver layer; a v2 schema could add constraint expressions alongside `source` without breaking v1 readers. New entry fields can be added with `omitempty`. +The top-level `version: 1` schema field allows future schema evolution; readers hard-error on unknown versions rather than misinterpreting them. Because `toolhive.lock.yaml` is the general project lock, future artifact types such as plugins (THV-0077) can add sibling top-level keys (e.g. `plugins:`) next to `skills:` without a version bump. The `source` field is the extensibility hook for a future manifest/resolver layer; a v2 schema could add constraint expressions alongside `source` without breaking v1 readers. New entry fields can be added with `omitempty`. ## Implementation Plan @@ -185,9 +210,9 @@ None blocking. This proposal relies on existing `gitresolver` commit checkout an ## Testing Strategy -- **Unit tests:** `lockfile` package round-trip, upsert/remove, and missing-file behavior; skillsvc hooks for project versus user scope; non-fatal lock write failures; `Sync` drift detection, unmanaged reports, and prune behavior; `Upgrade` digest changes, immutable sources, and dry-run behavior; API routes; CLI helpers. -- **E2E tests:** `thv skill install --scope project`, then `sync` on a clean tree, then `upgrade` after a catalog bump, against real GHCR artifacts. -- **Security tests:** confirm a tampered lock can only pin to references that pass existing supply-chain validation. +- **Unit tests:** `lockfile` package round-trip, upsert/remove, missing-file behavior, load-time validation of names and digests, and unknown-version rejection; skillsvc hooks for project versus user scope; non-fatal lock write failures surfacing warnings; `Sync` drift detection (including the `drifted` report category), `--check` exit codes, unmanaged reports, prune behavior, and continue-on-partial-failure semantics; `Upgrade` digest changes, immutable sources, upgrade-all default, and dry-run behavior; API routes; CLI helpers. +- **E2E tests:** `thv skill install --scope project`, then `sync` on a clean tree, then `sync --check` (clean and after inducing drift), then `upgrade` after a catalog bump, against real GHCR artifacts. +- **Security tests:** confirm a tampered lock can only pin to references that pass existing supply-chain validation, and that malformed lock entries are rejected at load time. ## Documentation @@ -197,15 +222,19 @@ None blocking. This proposal relies on existing `gitresolver` commit checkout an ## Open Questions -1. Should `sync` warn or refuse when the local SQLite state has a different digest for a skill that is in the lock, or silently reinstall to the pinned digest? The POC currently reinstalls silently. -2. Should the lock support an optional `clients` field per entry for teams that genuinely want per-client pinning, or stay strictly client-agnostic? -3. Should `upgrade` without args default to all entries, as in the current POC, or require explicit selection to avoid accidental mass upgrades in CI? -4. Where should a future manifest layer live, `toolhive.yaml` or `toolhive.skills.yaml`, if and when constraints are added, and should the lock file name change to match? +All questions raised in earlier drafts have been resolved during design review; the outcomes live in the relevant design sections above. + +1. ~~Sync behavior on digest drift~~ → **Resolved: reinstall to the pinned digest, reported distinctly as `drifted`** (see Detailed Design). +2. ~~Optional per-entry `clients` field~~ → **Resolved: the lock stays strictly client-agnostic; targeting is a sync-time local concern** (see Detailed Design). +3. ~~`upgrade` default target selection~~ → **Resolved: bare `upgrade` upgrades all entries, matching npm/cargo conventions, with `--dry-run` and the committed lock diff as safety nets** (see Detailed Design). +4. ~~Future manifest naming and lock file name~~ → **Resolved: the lock name stays `toolhive.lock.yaml` since it is the general project lock; manifest naming is deferred to a future manifest RFC** (see Forward Compatibility). ## References - POC PR: - Research note that motivated this: +- [THV-0030: Skills Lifecycle Management in ToolHive CLI](./THV-0030-skills-lifecycle-management.md) +- [THV-0041: SQLite-Based State Management](./THV-0041-sqlite-state-management.md) - Prior art: npm `package-lock.json`, pnpm `pnpm-lock.yaml`, `Cargo.lock`, `go.sum`, `poetry.lock` - oras-go cross-origin redirect auth guard, relevant to pinned-digest pulls: GHSA-vh4v-2xq2-g5cg From 0ed7a651439bd94dc1f3c4f31fef645fd4bea4b1 Mon Sep 17 00:00:00 2001 From: Samuele Verzi Date: Tue, 7 Jul 2026 11:22:05 +0200 Subject: [PATCH 5/6] Address panel review feedback on skills lock RFC Co-authored-by: Cursor --- rfcs/THV-0080-skills-lock-file.md | 199 +++++++++++++++++++----------- 1 file changed, 126 insertions(+), 73 deletions(-) diff --git a/rfcs/THV-0080-skills-lock-file.md b/rfcs/THV-0080-skills-lock-file.md index f23ab3b..ba020c5 100644 --- a/rfcs/THV-0080-skills-lock-file.md +++ b/rfcs/THV-0080-skills-lock-file.md @@ -3,13 +3,13 @@ - **Status**: Draft - **Author(s)**: Samuele Verzi (@samuv) - **Created**: 2026-07-03 -- **Last Updated**: 2026-07-03 +- **Last Updated**: 2026-07-07 - **Target Repository**: toolhive - **Related Issues**: [toolhive#5715](https://github.com/stacklok/toolhive/pull/5715) ## Summary -Add a project-level skills lock file (`toolhive.lock.yaml`) that pins the name, version, source, resolved reference, and digest of every project-scoped skill install, plus `thv skill sync` and `thv skill upgrade` commands to restore pinned state and pull newer content from the catalog. This brings to skills the reproducibility guarantees that `package-lock.json`, `Cargo.lock`, and `go.sum` provide for other ecosystems. +Add a project-level lock file (`toolhive.lock.yaml`) that pins the name, version, source, resolved reference, checkout digest, and content digest of every project-scoped skill install (including transitively materialized dependencies), plus `thv skill sync` and `thv skill upgrade` commands to restore pinned state and pull newer content from the catalog. The lock borrows the *shape* of `package-lock.json`, `Cargo.lock`, and `go.sum` (identity, source, integrity in a committed file) but does **not** claim their external trust roots in v1: the trust root for a committed pin is PR review of the lock diff, with a v2 milestone for Sigstore signing (per THV-0030) and transparency-log-style source-to-digest binding. ## Problem Statement @@ -19,10 +19,10 @@ Add a project-level skills lock file (`toolhive.lock.yaml`) that pins the name, ## Goals -- Pin the exact content digest of every project-scoped skill install in a file committed to the project repo. -- Restore the pinned skill set on any machine via `thv skill sync`. +- Pin the exact content of every project-scoped skill install (including transitive `toolhive.requires` dependencies) in a file committed to the project repo, with a deterministic `contentDigest` for integrity verification. +- Restore the pinned skill set on any machine via `thv skill sync`, with a pre-install confirmation gate on interactive terminals. - Provide a controlled, digest-based upgrade path via `thv skill upgrade`. -- Let CI verify that installed state matches the lock via `thv skill sync --check`. +- Let CI verify that installed on-disk content matches the lock via `thv skill sync --check` (content re-hash, not SQLite records alone). - Keep the lock client-agnostic by pinning content, not which client apps installed it. - Stay out of user-scope installs entirely. @@ -30,12 +30,13 @@ Add a project-level skills lock file (`toolhive.lock.yaml`) that pins the name, - Version-constraint resolution or manifest-driven installs. There is no `toolhive.yaml` with `^1.0.0` ranges in this proposal. The lock file is the declaration of intent; sync installs exactly what's pinned. - Per-client pinning. The lock pins content; sync installs for all detected clients, overridable with `--clients`. -- A dependency resolver or transitive dependency graph for skill-on-skill dependencies. +- A dependency *resolver* or version-constraint graph for skill-on-skill dependencies. Transitive deps are *recorded* in the lock (Cargo-style), not re-resolved from constraints. - Lock-file entries for user-scope installs. +- External trust roots in v1 (GOSUMDB-style transparency logs, mandatory Sigstore verification). These are named as a v2 milestone. ## Proposed Solution -Use a single lock file written by install commands. `--scope project` installs upsert an entry; uninstalls remove it. `sync` reinstalls at the pinned `resolvedReference@digest`; `upgrade` re-resolves the original `source` and rewrites the entry if the digest changed. +Use a single lock file written by install commands. `--scope project` installs upsert an entry (and entries for transitively materialized dependencies); uninstalls remove them. `sync` reinstalls at the pinned `resolvedReference@digest` and verifies `contentDigest` on disk; `upgrade` re-resolves the original `source` and rewrites the entry if the digest changed. ### High-Level Design @@ -45,13 +46,14 @@ flowchart LR svc --> files["client skill dirs"] svc --> db[(SQLite state)] svc --> lock["toolhive.lock.yaml (upsert)"] - syncCmd["thv skill sync"] --> lock2["read lock"] --> pinned["install resolvedReference@digest"] + syncCmd["thv skill sync"] --> prompt["pre-install prompt"] + prompt --> lock2["read lock"] --> pinned["install + verify contentDigest"] upgradeCmd["thv skill upgrade"] --> reresolve["re-resolve source"] --> compare{"digest changed?"} -->|yes| rewrite["install + rewrite entry"] ``` ### Detailed Design -`toolhive.lock.yaml` is the general ToolHive project lock file, not a skills-only artifact. Schema version 1 defines only the `skills:` key, but the top-level structure deliberately leaves room for future artifact types (for example a `plugins:` key for the plugins system) to join as sibling keys without a schema break. Loading a lock file with an unknown or newer top-level `version` is a hard error with a clear "upgrade thv" message; it is never silently ignored or partially parsed. +`toolhive.lock.yaml` is proposed as the ToolHive project lock file name. **This RFC owns only the `version` and `skills:` keys.** Reserving sibling top-level keys (for example `plugins:` for the plugins system in THV-0077) is a *contract proposal* that a future plugins or artifacts RFC must ratify; THV-0077 does not currently specify a lock schema. Loading a lock file with an unknown or newer top-level `version` is a hard error with a clear "upgrade thv" message; it is never silently ignored or partially parsed. The lock file has a top-level `version: 1` field and a deterministic list of skill entries sorted by name for stable diffs. Entries pin either OCI or git content: @@ -63,89 +65,123 @@ skills: source: code-review resolvedReference: ghcr.io/org/code-review:1.0.0 digest: sha256:9f2b1e... + contentDigest: sha256:a1b2c3d4... - name: testing-conventions source: git://github.com/org/skills.git#main/testing-conventions resolvedReference: git://github.com/org/skills.git digest: 4f0c9a1d2e8b7c6a5f4e3d2c1b0a9f8e7d6c5b4a + contentDigest: sha256:e5f6a7b8... + requiredBy: + - code-review ``` -Each entry stores `name`, optional `version` from `SKILL.md` frontmatter, `source`, `resolvedReference`, and `digest`. `source` is the original user input, such as a plain registry name, OCI reference, or `git://` reference, preserved verbatim so upgrade can re-resolve it. `resolvedReference` is the concrete OCI reference or git URL that source resolved to. `digest` is either an OCI `sha256:...` digest or a git commit hash, as in the two entries above. +Each entry stores: -The schema deliberately omits `installedAt`. Every major lock file, including npm, pnpm, yarn, Cargo, Go, and Poetry, stores identity, source, and integrity, not chronology. Reproducibility is guaranteed by the digest, not a timestamp; timestamps are environment-local and produce meaningless merge churn on no-op regenerations. "When was this pinned" is answered better by `git blame`, and `git log -p -- toolhive.lock.yaml` serves any recency or audit need. +- `name`, optional `version` from `SKILL.md` frontmatter +- `source`: the original user input (registry name, OCI reference, or `git://` reference), preserved verbatim so upgrade can re-resolve it +- `resolvedReference`: the concrete OCI reference or git URL that source resolved to +- `digest`: the checkout pin — OCI manifest digest or git commit hash used to fetch the artifact +- `contentDigest`: a deterministic SHA-256 dirhash of the materialized skill file set (the integrity primitive for `--check` and always-on verification). This closes the gap where a git commit hash pins history, not the skill subdirectory's content, and avoids relying on SHA-1 as the content-integrity anchor. +- `requiredBy` (optional): parent skill names for transitively materialized dependencies -The lock stays strictly client-agnostic: there is no per-entry `clients` field. Client sets are machine-local (one teammate may not have a given client installed at all), so client targeting is a sync-time concern handled by `--clients` or client auto-detection. If genuine demand for per-client pinning appears, an optional field can be added later with `omitempty` without a schema break. +`source` is never rewritten, so future upgrades keep re-resolving the same input. -Skill dependencies (`toolhive.requires` in `SKILL.md` frontmatter, recorded in the SQLite `skill_dependencies` table) are not locked in v1. Dependencies materialized by an install are recorded in SQLite but get no lock entry of their own, so they surface as *unmanaged* in sync reports. Proper dependency locking (entries with a machine-generated provenance marker, or a nested structure) is future work; until then, teams that want dependencies pinned should install them explicitly at project scope. +The schema deliberately omits `installedAt`. Every major lock file, including npm, pnpm, yarn, Cargo, Go, and Poetry, stores identity, source, and integrity, not chronology. Reproducibility is guaranteed by the digest, not a timestamp; timestamps are environment-local and produce meaningless merge churn on no-op regenerations. "When was this pinned" is answered better by `git blame`; `git log -p -- toolhive.lock.yaml` serves any recency or audit need. The CLI `--help` for sync points operators to that command for pin history. + +The lock stays strictly client-agnostic: there is no per-entry `clients` field. Client sets are machine-local, so client targeting is a sync-time concern handled by `--clients` or client auto-detection. + +#### Trust model (v1 vs v2) + +In v1, a committed lock entry is an **assertion**, not an externally verified fact. The `digest` and `contentDigest` are verified against themselves at install and sync time (reproducing the pinned bytes and re-hashing on disk), but there is no GOSUMDB, transparency log, or signature that binds `source` to `digest` independently of the lock author. The trust root is **PR review of the lock diff**, the same model teams use for any committed dependency pin before external attestation exists. Re-deriving digests from `source` at sync time would defeat pinning (two machines would diverge whenever the catalog moves); sync therefore installs exactly what the lock says. + +v2 (out of scope for this RFC, tracked as a follow-on milestone) adds an external trust layer: Sigstore verification per THV-0030, and/or transparency-log-style source-to-digest binding so a tampered lock entry can be checked against a record outside the repo. + +#### Transitive dependencies + +Skills declaring `toolhive.requires` in frontmatter materialize dependencies at install time. Project-scope installs **record** transitively materialized skills in the lock (Cargo.lock model: record what was installed, do not build a resolver). Each dependency entry includes `requiredBy: [parent]`. `sync` installs them at their pinned digest and `contentDigest`; they are never `--prune` candidates while a parent in the lock still requires them. Constraint-based resolution (`^1.0.0` ranges, dependency graphs) remains a non-goal. #### Component Changes -- Add a new `pkg/skills/lockfile` package for schema handling, load/save, and file-locked upsert/remove using `pkg/fileutils.WithFileLock` in the same pattern as config writes. Marshalling is deterministic, with sorted entries. Entry fields are validated at load time (see Input Validation), since the lock file is the one hand-editable input this feature introduces. -- Update `pkg/skills/skillsvc` install and uninstall hooks so project-scope installs upsert a lock entry recording the original `opts.Name` as `Source` before any internal resolution, and uninstalls remove it. Lock write errors never fail the install or uninstall, because the skill's files and DB record are already correct and a subsequent sync or upgrade can repair the lock; however, the failure is not merely logged server-side. It is surfaced as a warning field in the API response and printed by the CLI, so the user knows the committed lock is now stale. User-scope installs never touch the lock. -- Add `Sync`, which reads the lock, compares each entry against `SkillStore` state, installs strictly by pinned `resolvedReference@digest` using OCI pull-by-digest or git clone plus checkout of the pinned commit, and reports unmanaged project-scope skills. A locked skill whose local record has a *different* digest is reinstalled to the pinned digest and reported distinctly as `drifted`, not lumped into `installed`. With `--prune`, it uninstalls unmanaged skills. It never rewrites the lock; it only makes FS/DB match what's pinned. -- Drift detection compares the lock against SQLite install records, not re-hashed file content. If a user hand-edits or deletes installed skill files without going through `thv`, the DB still reports the pinned digest and sync will report `upToDate` without repairing the files. Similarly, a digest match says nothing about which clients hold the files: a client installed after the original install will have an empty skill directory yet the entry still reports `upToDate`. Content-hash verification and per-client file presence checks are noted as future work; the pragmatic remedy today is `thv skill install` of the affected skill. -- Add `--check` to `thv skill sync`: it computes the same diff sync would apply but changes nothing, exiting non-zero if any entry is missing or drifted. This gives CI a cheap gate analogous to `npm ci` or `cargo --locked`. -- Add `Upgrade`, which re-resolves each entry's `source` exactly as a fresh `thv skill install ` would. Registry names resolve through the catalog, git branches/tags resolve to current heads, and OCI tags resolve to current digests. If the digest changed, upgrade installs and rewrites the entry. Immutable sources such as OCI `@sha256:` digests or full 40-character git commit hashes are reported as `not-upgradable` without contacting the network. `source` is never rewritten, so future upgrades keep re-resolving the same input. -- Bare `thv skill upgrade` upgrades all lock entries, matching `npm update` and `cargo update` conventions. The safety net is `--dry-run` plus the fact that an upgrade only propagates to teammates once the lock diff is reviewed and committed. -- `--dry-run` prints what would change but is not fully side-effect-free: there is no "peek" API for OCI or git, so dry-run still pulls the artifact into the local cache or clones the repo, skipping only extraction and FS/DB writes. -- `Sync` and `Upgrade` process every entry even when some fail (registry down, digest garbage-collected): failures are reported per skill in the structured report, successful installs stay in place, and the CLI exits non-zero if any entry failed. During upgrade, the lock is rewritten per entry as each upgrade lands; a failure after install but before the lock rewrite leaves lock and reality briefly inconsistent, which the report flags and the next sync or upgrade repairs. -- Have `Sync` and `Upgrade` call `installInternal`, not `Install`, so they do not overwrite the entry's `Source` with an already-resolved reference. +- Add a new `pkg/skills/lockfile` package for schema handling, load/save, and file-locked upsert/remove using `pkg/fileutils.WithFileLock` in the same pattern as config writes. Marshalling is deterministic, with sorted entries. Entry fields are validated at load time (see Input Validation). +- Update `pkg/skills/skillsvc` install and uninstall hooks so project-scope installs upsert lock entries (including transitive deps with `requiredBy`), recording the original `opts.Name` as `Source` before any internal resolution, and uninstalls remove them. **A failed lock write on project-scope install exits non-zero** with a clear message ("skill installed but lock NOT updated — do not commit; re-run or fix permissions"). Files and DB records stay in place (no rollback), but the command fails so CI and humans cannot miss a stale lock. User-scope installs never touch the lock. +- Set a `managed: true` marker on SQLite install records created by project-scope locked installs. `--prune` removes only skills previously lock-managed that are no longer in the lock (`removed-from-lock`); out-of-band installs (`never-managed`) are reported but never pruned. +- Add `Sync`, which reads the lock, compares each entry against on-disk content (via `contentDigest`) and `SkillStore` state, installs strictly by pinned `resolvedReference@digest`, and reports unmanaged project-scope skills split into `never-managed` vs `removed-from-lock`. A locked skill whose local `contentDigest` differs is reinstalled and reported as `drifted`. Digest verification is **always-on on every install path**, including OCI cache hits. With `--prune`, it uninstalls only `removed-from-lock` skills (listed in the pre-flight prompt). It never rewrites the lock. +- **Pre-install gate:** on an interactive TTY, `sync` prints a pre-flight summary (name, source, digest, and contentDigest for entries to install, drift, or prune) and asks `Install? [y/N]` defaulting to **No**. `--yes` skips the prompt for scripts; non-interactive without `--yes` fails closed. This gates the "one command after `git pull`" flow before AI-followed instructions land on disk. +- Add `--check` to `thv skill sync`: computes whether installed on-disk content matches the lock by **re-hashing skill files against `contentDigest`** in every detected client directory, plus file-presence checks. Changes nothing. Exits non-zero if any entry is missing, drifted, or has absent client files. This verifies *installed state matches the lock*; it is not a stale-lock freshness gate (see Upgrade below). +- Add `--adopt` to `thv skill sync`: writes lock entries for existing project-scope installs using their current digests and contentDigests. On first run with no lock entries but existing installs, the CLI prints: *"No lock entries yet, but N skills are installed locally. They are unmanaged. To pin them, run `thv skill sync --adopt` or `thv skill install --scope project` each."* +- Add `Upgrade`, which re-resolves each entry's `source` exactly as a fresh `thv skill install ` would. If the digest changed, upgrade installs and rewrites the entry. If re-resolution yields a **different `resolvedReference`** (not merely a new digest for the same ref), upgrade refuses without `--allow-ref-change` and highlights the change in the report — mitigating catalog-redirect and typosquat TOCTOU. Teams should prefer fully-qualified OCI refs in `source` for security-sensitive skills. Immutable sources (OCI `@sha256:` digests, full commit hashes) are `not-upgradable` without network contact. `source` is never rewritten. +- Bare `thv skill upgrade` upgrades all lock entries. Use `thv skill upgrade --preview --fail-on-changes` as an optional CI **freshness** gate: exits non-zero if any mutable source would re-resolve to a new digest (distinct from `--check`'s integrity gate). +- `--preview` (formerly `--dry-run`) prints what would change. **It is not side-effect-free:** there is no peek API for OCI or git, so preview still fetches artifacts into the local cache or clones repos, skipping only extraction and FS/DB writes. `--help` and startup banner state this explicitly. Fetches are bounded (max body size, context timeout). `--check` does not populate the cache. +- `Sync` and `Upgrade` process every entry even when some fail: failures carry a typed `reason` (see API Changes); successes stay; CLI exits non-zero on any failure. During upgrade, per-entry lock rewrite means a failure after install but before lock write leaves brief inconsistency, flagged in the report. +- Sync and upgrade installs use a public `PreserveSource` install option so the entry's `Source` is not overwritten with an already-resolved reference (replacing the unexported `installInternal` pattern). #### API Changes -This proposal is additive, with no breaking HTTP API changes (adding `Sync` and `Upgrade` to the `SkillService` Go interface does break external implementers of that interface, if any exist): +HTTP API changes are additive. Go changes introduce a **new interface** without breaking existing implementers: -- `POST /skills/sync` with body `{projectRoot, clients, prune, check}` returns a report containing installed, drifted, up-to-date, unmanaged, pruned, and failed skills. With `check: true` the report is computed but nothing is installed or pruned. -- `POST /skills/upgrade` with body `{projectRoot, names, dryRun, clients}` returns per-skill outcomes of upgraded, up-to-date, not-upgradable, or failed, including old and new digests where relevant. -- Install responses gain an optional warning field used when the lock write fails after a successful project-scope install. -- `SkillService` gains `Sync` and `Upgrade` methods, and `pkg/skills/client` gains corresponding HTTP methods. +- `POST /skills/sync` with body `{projectRoot, clients, prune, check, adopt, yes}` returns a report with installed, drifted, upToDate, neverManaged, removedFromLock, pruned, and failed skills. With `check: true`, nothing is installed or pruned. +- `POST /skills/upgrade` with body `{projectRoot, names, preview, failOnChanges, allowRefChange, clients}` returns per-skill outcomes (upgraded, upToDate, notUpgradable, refChangeBlocked, failed) with old/new digests where relevant. +- Project-scope install responses exit non-zero (HTTP 500 or equivalent) when the lock write fails after a successful install. +- Define a new **`SkillLockService`** interface with `Sync` and `Upgrade` methods; `skillsvc` satisfies both `SkillService` and `SkillLockService`. The existing `SkillService` interface is **unchanged**, preserving compile compatibility for external implementers in `toolhive-git-skills` and `stacklok-enterprise-platform`. `pkg/skills/client` gains HTTP methods for the new endpoints. +- Per-entry failures include a typed `reason` enum: `registry-unreachable`, `digest-missing`, `validation-rejected`, `lock-write-failed`, `ref-change-blocked`, `unknown`, plus a human-readable `error` string. +- **Exit codes:** `0` clean; `2` drift or check failure; `3` partial failure (see report); `4` validation or policy rejection. #### Configuration Changes -None. The lock file is project-level, discovered by auto-detecting the git root from the current working directory, using the nearest enclosing `.git` directory. `--project-root` provides an override. If there is no enclosing git repository and no `--project-root`, the command errors rather than guessing; monorepo sub-projects that want their own lock are supported only via an explicit `--project-root ` (the lock lives wherever it points). No global config is added. +None beyond lock discovery. The lock file is project-level, discovered using **`git rev-parse --show-toplevel` semantics** (handles worktrees where `.git` is a file, not a directory). `--project-root` provides an override. If there is no enclosing git repository and no `--project-root`, the command errors with: *"no git repository found (or not inside one); pass --project-root to specify the project root"*. Monorepo sub-projects use explicit `--project-root `. No global config is added. #### Data Model Changes -No SQLite schema changes are required. The lock file is a new committed YAML artifact at the project root, while the SQLite `installed_skills` table continues to hold runtime install records. The lock is the shareable pin; SQLite is the local install state. `sync` reconciles the two. +The SQLite `installed_skills` table gains a boolean `managed` column (or equivalent flag) set `true` for project-scope locked installs. No other schema changes. The lock file is a committed YAML artifact; SQLite holds runtime install state. `sync` reconciles lock, DB, and on-disk content. This coexists with [THV-0041](./THV-0041-sqlite-state-management.md): SQLite remains the unified *local runtime* store; the lock is the shareable, reviewable *committed pin*. Drift between them is expected transiently and is what `sync` repairs. ## Security Considerations ### Threat Model -A malicious or compromised lock file, such as one introduced through a tampered PR, could pin a skill to a known-bad digest. The threat is an attacker committing a lock entry pointing to malicious skill content that teammates then sync. The relevant attacker capabilities are write access to the project repo or the ability to submit a PR that is merged. `sync` amplifies this threat compared to manual installs: it turns "review what you install" into "run one command after `git pull`", silently installing instructions an AI assistant will follow, which is why lock diffs deserve the same review scrutiny as code. +A malicious or compromised lock file, such as one introduced through a tampered PR, could pin a skill to a known-bad digest. The threat is an attacker committing a lock entry pointing to malicious skill content that teammates then sync. Attacker capabilities: write access to the project repo or ability to merge a PR. + +**v1 trust boundary:** a committed digest is an assertion verified against itself at install time, not against an external record. PR review of the lock diff is the primary control until v2 adds Sigstore (THV-0030) and/or transparency-log binding. -Two related considerations: +`sync` amplifies this threat: it can turn "review what you install" into "run one command after `git pull`". The pre-install confirmation gate (default `[y/N]`) mitigates this for interactive use; `--yes` is required for scripted/CI sync. -- The two pin types do not carry equal integrity guarantees. OCI `sha256:` digests are content-addressed and collision-resistant; git commit hashes are SHA-1 for most repositories, where collisions have been demonstrated. This is an accepted trade-off inherited from git itself; future skill signing (e.g. Sigstore, as floated in THV-0030) would mitigate it. -- `sync --prune` is a destructive operation driven by a committed file: a tampered lock that *removes* entries, combined with a habitual `sync --prune`, deletes legitimate skills. Prune therefore stays opt-in and pruned skills are itemized in the report. +Additional considerations: + +- OCI `sha256:` digests and `contentDigest` dirhashes are collision-resistant; git commit hashes remain SHA-1 on most hosts and pin history, not content — `contentDigest` is the content-integrity anchor. +- `sync --prune` is destructive: a tampered lock removing entries combined with `--prune --yes` deletes previously lock-managed skills. Prune is opt-in, gated by the pre-flight prompt, and limited to `removed-from-lock` (never out-of-band installs). +- Upgrade re-resolution of bare registry names is a TOCTOU vector; `--allow-ref-change` gates reference changes. ### Authentication and Authorization -Lock-file writes happen server-side in `skillsvc`, gated by the same API auth as existing skill install/uninstall operations. This proposal introduces no new auth surface. `sync` and `upgrade` reuse the existing OCI Docker credential chain and git token auth paths, with no new credential handling. +Lock-file writes happen server-side in `skillsvc`, gated by the same API auth as existing skill install/uninstall. `sync` and `upgrade` reuse the existing OCI Docker credential chain and git token auth paths (transport auth, not catalog mapping auth). ### Data Security -The lock file contains no secrets, only public references and content digests. It is committed to git by design. No sensitive data is stored or transmitted beyond what `thv skill install` already transmits. +The lock file contains no secrets, only public references and content digests. It is committed to git by design. ### Input Validation -The lock file is the one hand-editable input this feature introduces, so entries are validated at the trust boundary: `lockfile.Load` validates entry names via the same `ValidateSkillName` used for installs and rejects malformed digests before any entry is acted on. (The POC instead relies on downstream parsing — `ParseGitReference` and `nameref.ParseReference` during `buildPinnedReference`, plus the full install validation path — which is effective but harder to reason about; load-time validation is the specified design.) Beyond load, `resolvedReference` and `digest` still flow through the existing OCI and git resolution paths, which enforce SSRF guards such as no localhost/private IPs in git refs except dev mode, shell-injection reference validation, and supply-chain checks such as requiring artifact skill names to match OCI repo paths. A tampered lock can only pin references that would also pass a manual `thv skill install`. +The lock file is the one hand-editable input this feature introduces. `lockfile.Load` validates entry names via `ValidateSkillName`, rejects malformed `digest` and `contentDigest` formats, and validates `requiredBy` references before any entry is acted on. + +Beyond load, `resolvedReference` and `digest` flow through existing OCI and git resolution paths (SSRF guards, shell-injection ref validation, supply-chain name checks). A tampered lock can only pin references that would pass a manual `thv skill install`; it cannot be independently proven correct without v2 attestation. -The `projectRoot` accepted by the sync/upgrade API bodies is validated the same way existing project-scope install paths are, so the daemon cannot be directed to read or write lock files in arbitrary unrelated locations. +**`projectRoot` validation (CWE-22):** the API body field must be (1) absolute, (2) canonicalized with symlinks resolved, (3) within a git-rooted tree matching auto-detection rules. For the HTTP API, `projectRoot` must fall under a daemon-configured `--serve-root`; callers cannot direct the daemon to arbitrary filesystem locations. ### Secrets Management -None. The lock stores no tokens. Registry and git authentication use the existing per-process credential chain. +None. The lock stores no tokens. ### Audit and Logging -Lock upsert/remove errors are logged via `slog.Warn` with skill name and project root, and additionally surfaced to the caller as a warning in the API response and CLI output. `sync` and `upgrade` return structured reports covering installed, drifted, up-to-date, unmanaged, pruned, failed, upgraded, and not-upgradable states suitable for audit. The lock file itself, being git-committed, gives a full history of what was pinned when, which is stronger cross-machine provenance than DB-only audit. +Lock upsert/remove errors are logged via `slog.Warn`. Failed project-scope lock writes fail the command (non-zero exit). `sync` and `upgrade` return structured reports with typed failure reasons suitable for audit. The git-committed lock provides cross-machine provenance via `git blame` and `git log -p -- toolhive.lock.yaml`. ### Mitigations -- Pinned-digest installs mean `sync` reproduces the exact pinned content, and a merged lock entry is auditable in `git blame`. -- `upgrade` refuses to upgrade entries already pinned to a digest or commit, preventing accidental re-resolution of intentionally locked content. -- Best-effort lock writes mean a failed lock write cannot corrupt an install because files and DB state are already correct; the failure is surfaced to the user, and `sync` can repair drift recorded in the DB (it does not detect manual edits to installed files; see Detailed Design). -- `sync` reports new, drifted, and pruned entries per skill, keeping the "one command after `git pull`" flow observable rather than silent. +- `contentDigest` plus always-on verification and `--check` re-hashing detect on-disk tamper and cache substitution. +- Pre-install confirmation gate (interactive default No) before sync installs AI-followed instructions. +- `upgrade` refuses reference changes without `--allow-ref-change`; immutable pins cannot be re-resolved. +- Failed lock writes fail the install command for project scope. +- Transitive deps are locked with `requiredBy` provenance; prune cannot delete out-of-band or required skills. +- v2 milestone: Sigstore verification and source-to-digest transparency binding. ## Alternatives Considered @@ -153,90 +189,106 @@ Lock upsert/remove errors are logged via `slog.Warn` with skill name and project - **Description:** A hand-edited `toolhive.yaml` declares desired skills with version constraints, and the lock resolves them. - **Pros:** Supports `^1.0.0` ranges; upgrade is "re-resolve constraints". -- **Cons:** Requires building a dependency resolver; doubles the surface area with two files to keep in sync; skills do not have a rich enough versioning or constraint ecosystem to justify it yet. -- **Why not chosen:** The POC scope is narrower. The single-lock model delivers reproducibility now and leaves the door open to a manifest layer later without rework. +- **Cons:** Requires building a dependency resolver; doubles the surface area with two files to keep in sync. +- **Why not chosen:** The single-lock model delivers reproducibility now and leaves the door open to a manifest layer later. ### Alternative 2: Store pin data only in the SQLite store - **Description:** No committed file; sync reads from a shared or replicated DB. - **Pros:** No new file format. -- **Cons:** Not portable across machines; cannot be reviewed in a PR; no `git blame` provenance; defeats the committed, auditable pin goal entirely. -- **Why not chosen:** This approach fundamentally does not meet the reproducibility and audit goals. +- **Cons:** Not portable; cannot be reviewed in a PR; defeats the committed pin goal. +- **Why not chosen:** Fundamentally does not meet reproducibility and audit goals. ### Alternative 3: Per-client lock entries - **Description:** Pin which client apps each skill installs into. - **Pros:** Precise per-client control. -- **Cons:** Couples the lock to the local client set; bloats entries; one teammate without Cursor installed cannot sync cleanly. -- **Why not chosen:** Content pinning is the real need; client targeting is a local concern handled at sync time by `--clients` or detected clients. +- **Cons:** Couples the lock to the local client set; bloats entries. +- **Why not chosen:** Content pinning is the real need; client targeting is a sync-time local concern. + +### Alternative 4: Re-derive digests from source at sync time + +- **Description:** On sync, re-resolve each entry's `source` and verify the committed digest matches the currently-resolved digest. +- **Pros:** Binds source to digest without an external trust root. +- **Cons:** Defeats pinning — sync would install whatever the catalog serves today, not what the lock author intended. Two machines diverge whenever the catalog moves. +- **Why not chosen:** Contradicts the core reproducibility goal. External attestation (v2) is the correct fix for unauthenticated digests. ## Compatibility ### Backward Compatibility -This change is fully backward compatible. Existing project-scope installs simply start writing a lock file. Pre-existing installs without a lock entry still work; sync reports them as unmanaged and does not touch them unless `--prune` is set. No migration is required because the lock file is additive. User-scope installs are entirely unchanged. +HTTP API changes are additive. The existing `SkillService` Go interface is unchanged (new `SkillLockService` carries sync/upgrade). Existing project-scope installs start writing a lock file on next install. Pre-existing installs without lock entries work; sync reports them as `never-managed` and offers `--adopt`. User-scope installs are unchanged. + +**Not fully backward compatible:** project-scope installs now fail non-zero when the lock write fails (previously best-effort warn). POC consumers must update to handle the new exit semantics. ### Forward Compatibility -The top-level `version: 1` schema field allows future schema evolution; readers hard-error on unknown versions rather than misinterpreting them. Because `toolhive.lock.yaml` is the general project lock, future artifact types such as plugins (THV-0077) can add sibling top-level keys (e.g. `plugins:`) next to `skills:` without a version bump. The `source` field is the extensibility hook for a future manifest/resolver layer; a v2 schema could add constraint expressions alongside `source` without breaking v1 readers. New entry fields can be added with `omitempty`. +Readers hard-error on unknown lock `version`. This RFC proposes the `toolhive.lock.yaml` filename and owns `version` + `skills:` only; sibling keys such as `plugins:` are a contract proposal for THV-0077 or a dedicated project-lock RFC to ratify. The `source` field is the extensibility hook for a future manifest layer. New entry fields use `omitempty`. ## Implementation Plan -A POC implementation already exists in [toolhive#5715](https://github.com/stacklok/toolhive/pull/5715). Post-RFC acceptance, split it into reviewable PRs. +A POC implementation exists in [toolhive#5715](https://github.com/stacklok/toolhive/pull/5715). Post-RFC acceptance, split and extend it into reviewable PRs. ### Phase 1: Lock file package -- Add the `pkg/skills/lockfile` schema and file-locked operations. +- `pkg/skills/lockfile` schema (`contentDigest`, `requiredBy`), load-time validation, file-locked ops. ### Phase 2: Install/uninstall hooks -- Add project-scope install upsert and uninstall remove behavior. +- Project-scope upsert (including transitive deps), uninstall remove, fail-on-lock-write, `managed` SQLite flag. ### Phase 3: Sync -- Add the `Sync` service method, API, and `thv skill sync` CLI. +- `SkillLockService.Sync`, content re-hash, pre-install gate, `--check`, `--adopt`, prune hardening, API + CLI. ### Phase 4: Upgrade -- Add the `Upgrade` service method, API, and `thv skill upgrade` CLI. +- `SkillLockService.Upgrade`, `--preview`, `--fail-on-changes`, `--allow-ref-change`, API + CLI. ### Phase 5: Docs -- Update `docs/arch/12-skills-system.md`, CLI docs, and swagger. +- `docs/arch/12-skills-system.md`, CLI docs, swagger, exit-code table. + +### Phase 6: v2 trust layer (follow-on, out of scope here) + +- Sigstore verification per THV-0030; optional transparency-log source-to-digest binding. ### Dependencies -None blocking. This proposal relies on existing `gitresolver` commit checkout and `ociskills.RegistryClient` digest pulls, both of which are already capable of supporting the design. +None blocking. Relies on existing `gitresolver`, `ociskills.RegistryClient`, and THV-0041 SQLite store. ## Testing Strategy -- **Unit tests:** `lockfile` package round-trip, upsert/remove, missing-file behavior, load-time validation of names and digests, and unknown-version rejection; skillsvc hooks for project versus user scope; non-fatal lock write failures surfacing warnings; `Sync` drift detection (including the `drifted` report category), `--check` exit codes, unmanaged reports, prune behavior, and continue-on-partial-failure semantics; `Upgrade` digest changes, immutable sources, upgrade-all default, and dry-run behavior; API routes; CLI helpers. -- **E2E tests:** `thv skill install --scope project`, then `sync` on a clean tree, then `sync --check` (clean and after inducing drift), then `upgrade` after a catalog bump, against real GHCR artifacts. -- **Security tests:** confirm a tampered lock can only pin to references that pass existing supply-chain validation, and that malformed lock entries are rejected at load time. +- **Unit tests:** lockfile round-trip, validation, `contentDigest` computation, transitive `requiredBy` entries; fail-on-lock-write; managed marker and prune semantics (`never-managed` vs `removed-from-lock`); sync pre-install gate, content re-hash in `--check`, `--adopt`; upgrade ref-change blocking, `--preview`, `--fail-on-changes`; typed failure reasons and exit codes; `SkillLockService` separate from `SkillService`. +- **E2E tests:** install → sync → `--check` (clean, after on-disk tamper, after missing client files) → `--adopt` first-run → upgrade with ref change blocked → `--preview --fail-on-changes` against real GHCR artifacts. +- **Security tests:** malformed lock rejected at load; `projectRoot` path traversal rejected; tampered lock installs only pass existing supply-chain validation; sync gate requires confirmation on TTY. ## Documentation -- Update `docs/arch/12-skills-system.md` with a new "Project Lock File" section, as done in the POC PR. -- Regenerate CLI docs via `task docs`. -- Add a short user-facing guide on committing `toolhive.lock.yaml` and running `sync` after pull. +- Update `docs/arch/12-skills-system.md` with "Project Lock File" section (trust model, contentDigest, transitive deps). +- Regenerate CLI docs via `task docs` (`--preview`, `--check`, `--adopt`, exit codes, pin history pointer). +- User guide: committing `toolhive.lock.yaml`, running `sync` after pull, CI patterns for `--check` vs `upgrade --preview --fail-on-changes`. ## Open Questions -All questions raised in earlier drafts have been resolved during design review; the outcomes live in the relevant design sections above. +Resolved during design and panel review; outcomes live in the design sections above. -1. ~~Sync behavior on digest drift~~ → **Resolved: reinstall to the pinned digest, reported distinctly as `drifted`** (see Detailed Design). -2. ~~Optional per-entry `clients` field~~ → **Resolved: the lock stays strictly client-agnostic; targeting is a sync-time local concern** (see Detailed Design). -3. ~~`upgrade` default target selection~~ → **Resolved: bare `upgrade` upgrades all entries, matching npm/cargo conventions, with `--dry-run` and the committed lock diff as safety nets** (see Detailed Design). -4. ~~Future manifest naming and lock file name~~ → **Resolved: the lock name stays `toolhive.lock.yaml` since it is the general project lock; manifest naming is deferred to a future manifest RFC** (see Forward Compatibility). +1. ~~Sync drift behavior~~ → reinstall, report `drifted`, verify via `contentDigest` re-hash. +2. ~~Per-entry `clients` field~~ → lock stays client-agnostic. +3. ~~Upgrade default target~~ → upgrade all; `--preview --fail-on-changes` for CI freshness. +4. ~~Lock file naming / general lock~~ → `toolhive.lock.yaml` proposed; this RFC owns `skills:` only; sibling keys are a contract proposal. +5. ~~v1 trust root~~ → PR review of lock diff; v2 adds Sigstore / transparency binding. +6. ~~Transitive dependencies~~ → record in lock with `requiredBy`; no resolver. ## References - POC PR: -- Research note that motivated this: +- Research note: - [THV-0030: Skills Lifecycle Management in ToolHive CLI](./THV-0030-skills-lifecycle-management.md) - [THV-0041: SQLite-Based State Management](./THV-0041-sqlite-state-management.md) -- Prior art: npm `package-lock.json`, pnpm `pnpm-lock.yaml`, `Cargo.lock`, `go.sum`, `poetry.lock` -- oras-go cross-origin redirect auth guard, relevant to pinned-digest pulls: GHSA-vh4v-2xq2-g5cg +- [THV-0077: Plugin lifecycle management](./THV-0077-plugins-lifecycle-management.md) +- Prior art (shape, not v1 trust parity): npm `package-lock.json`, pnpm `pnpm-lock.yaml`, `Cargo.lock`, `go.sum`, `poetry.lock` +- oras-go cross-origin redirect auth guard: GHSA-vh4v-2xq2-g5cg --- @@ -249,6 +301,7 @@ All questions raised in earlier drafts have been resolved during design review; | Date | Reviewer | Decision | Notes | |------|----------|----------|-------| | YYYY-MM-DD | @reviewer | Under Review | Initial submission | +| 2026-07-07 | @JAORMX | Commented | Panel review; RFC revised to address findings | ### Implementation Tracking From cd35106e6b92d59a9c8e02273a3b889fb9671aff Mon Sep 17 00:00:00 2001 From: Samuele Verzi Date: Wed, 8 Jul 2026 15:13:13 +0200 Subject: [PATCH 6/6] Fold Sigstore signing into v1 scope Co-authored-by: Cursor --- rfcs/THV-0080-skills-lock-file.md | 264 +++++++++++++++++------------- 1 file changed, 154 insertions(+), 110 deletions(-) diff --git a/rfcs/THV-0080-skills-lock-file.md b/rfcs/THV-0080-skills-lock-file.md index ba020c5..aaec519 100644 --- a/rfcs/THV-0080-skills-lock-file.md +++ b/rfcs/THV-0080-skills-lock-file.md @@ -3,26 +3,27 @@ - **Status**: Draft - **Author(s)**: Samuele Verzi (@samuv) - **Created**: 2026-07-03 -- **Last Updated**: 2026-07-07 +- **Last Updated**: 2026-07-08 - **Target Repository**: toolhive - **Related Issues**: [toolhive#5715](https://github.com/stacklok/toolhive/pull/5715) ## Summary -Add a project-level lock file (`toolhive.lock.yaml`) that pins the name, version, source, resolved reference, checkout digest, and content digest of every project-scoped skill install (including transitively materialized dependencies), plus `thv skill sync` and `thv skill upgrade` commands to restore pinned state and pull newer content from the catalog. The lock borrows the *shape* of `package-lock.json`, `Cargo.lock`, and `go.sum` (identity, source, integrity in a committed file) but does **not** claim their external trust roots in v1: the trust root for a committed pin is PR review of the lock diff, with a v2 milestone for Sigstore signing (per THV-0030) and transparency-log-style source-to-digest binding. +Add a project-level lock file (`toolhive.lock.yaml`) that pins the name, version, source, resolved reference, checkout digest, content digest, and **Sigstore signer identity** of every project-scoped skill install (including transitively materialized dependencies), plus `thv skill sync` and `thv skill upgrade` commands to restore pinned state and pull newer content from the catalog. **v1 ships Sigstore signing and verification end-to-end**: `thv skill push` signs OCI artifacts (keyless by default), install/sync/upgrade verify signatures and pinned publisher identity, and `sync --check` re-verifies offline from stored Sigstore bundles. This delivers the Sigstore integration [THV-0030](./THV-0030-skills-lifecycle-management.md) deferred as "future work" and closes the panel-review finding that a committed digest alone is an unauthenticated trust root. ## Problem Statement -- **Current behavior:** `thv skill install --scope project` writes files to client skill dirs and a SQLite record, but nothing pins *which* content was installed in a shareable, version-controlled form. Two teammates cloning the same repo get whatever the catalog currently serves, not what the original installer intended. -- **Who is affected:** any team using project-scoped skills for shared workflows (code-review conventions, testing skills, org-specific instructions). Reproducibility and controlled upgrades are both missing. +- **Current behavior:** `thv skill install --scope project` writes files to client skill dirs and a SQLite record, but nothing pins *which* content was installed in a shareable, version-controlled form. Two teammates cloning the same repo get whatever the catalog currently serves, not what the original installer intended. No signature verification exists for skill artifacts. +- **Who is affected:** any team using project-scoped skills for shared workflows (code-review conventions, testing skills, org-specific instructions). Reproducibility, controlled upgrades, and supply-chain attestation are all missing. - **Why worth solving:** skills are supply-chain artifacts (instructions an AI assistant follows). Unpinned installs mean silent drift across machines and no auditable path from "what we agreed to use" to "what's actually installed". Every comparable package manager solved this with a committed lock file; skills currently have no equivalent. ## Goals -- Pin the exact content of every project-scoped skill install (including transitive `toolhive.requires` dependencies) in a file committed to the project repo, with a deterministic `contentDigest` for integrity verification. +- Pin the exact content and **publisher identity** of every project-scoped skill install (including transitive `toolhive.requires` dependencies) in a file committed to the project repo, with a deterministic `contentDigest` for integrity verification. +- **Sign on publish, verify on consume:** `thv skill push` produces keyless Sigstore signatures (OCI referrers); install/sync/upgrade verify signatures and locked signer identity by default. - Restore the pinned skill set on any machine via `thv skill sync`, with a pre-install confirmation gate on interactive terminals. - Provide a controlled, digest-based upgrade path via `thv skill upgrade`. -- Let CI verify that installed on-disk content matches the lock via `thv skill sync --check` (content re-hash, not SQLite records alone). +- Let CI verify that installed on-disk content **and signatures** match the lock via `thv skill sync --check` (content re-hash + offline Sigstore bundle re-verification). - Keep the lock client-agnostic by pinning content, not which client apps installed it. - Stay out of user-scope installs entirely. @@ -32,23 +33,25 @@ Add a project-level lock file (`toolhive.lock.yaml`) that pins the name, version - Per-client pinning. The lock pins content; sync installs for all detected clients, overridable with `--clients`. - A dependency *resolver* or version-constraint graph for skill-on-skill dependencies. Transitive deps are *recorded* in the lock (Cargo-style), not re-resolved from constraints. - Lock-file entries for user-scope installs. -- External trust roots in v1 (GOSUMDB-style transparency logs, mandatory Sigstore verification). These are named as a v2 milestone. +- Org-wide signer policy files (allow/deny lists of publisher identities). Deferred to future work. ## Proposed Solution -Use a single lock file written by install commands. `--scope project` installs upsert an entry (and entries for transitively materialized dependencies); uninstalls remove them. `sync` reinstalls at the pinned `resolvedReference@digest` and verifies `contentDigest` on disk; `upgrade` re-resolves the original `source` and rewrites the entry if the digest changed. +Use a single lock file written by install commands. `--scope project` installs upsert an entry (and entries for transitively materialized dependencies), recording verified Sigstore provenance; uninstalls remove them. `sync` reinstalls at the pinned `resolvedReference@digest`, verifies `contentDigest` and signature offline; `upgrade` re-resolves the original `source`, verifies the new artifact's signature, and rewrites the entry if the digest changed. ### High-Level Design ```mermaid flowchart LR - installCmd["thv skill install --scope project"] --> svc[skillsvc install] + pushCmd["thv skill push"] --> sign["Sigstore sign OCI referrer"] + installCmd["thv skill install --scope project"] --> verify["verify signature + identity"] + verify --> svc[skillsvc install] svc --> files["client skill dirs"] - svc --> db[(SQLite state)] - svc --> lock["toolhive.lock.yaml (upsert)"] + svc --> db[(SQLite + Sigstore bundle)] + svc --> lock["toolhive.lock.yaml upsert"] syncCmd["thv skill sync"] --> prompt["pre-install prompt"] - prompt --> lock2["read lock"] --> pinned["install + verify contentDigest"] - upgradeCmd["thv skill upgrade"] --> reresolve["re-resolve source"] --> compare{"digest changed?"} -->|yes| rewrite["install + rewrite entry"] + prompt --> lock2["read lock"] --> pinned["install + verify contentDigest + signature"] + upgradeCmd["thv skill upgrade"] --> reresolve["re-resolve source"] --> compare{"digest changed?"} -->|yes| rewrite["verify + install + rewrite entry"] ``` ### Detailed Design @@ -66,13 +69,27 @@ skills: resolvedReference: ghcr.io/org/code-review:1.0.0 digest: sha256:9f2b1e... contentDigest: sha256:a1b2c3d4... + provenance: + signerIdentity: "https://github.com/stacklok/toolhive/.github/workflows/release.yaml@refs/heads/main" + certIssuer: "https://token.actions.githubusercontent.com" + repositoryUri: "https://github.com/stacklok/toolhive-catalog" + sigstoreUrl: "https://rekor.sigstore.dev" - name: testing-conventions source: git://github.com/org/skills.git#main/testing-conventions resolvedReference: git://github.com/org/skills.git digest: 4f0c9a1d2e8b7c6a5f4e3d2c1b0a9f8e7d6c5b4a contentDigest: sha256:e5f6a7b8... + provenance: + signerIdentity: "https://github.com/org/skills/.github/workflows/sign.yaml@refs/heads/main" + certIssuer: "https://token.actions.githubusercontent.com" requiredBy: - code-review + - name: legacy-skill + source: ghcr.io/example/legacy:1.0.0 + resolvedReference: ghcr.io/example/legacy:1.0.0 + digest: sha256:deadbeef... + contentDigest: sha256:cafebabe... + unsigned: true ``` Each entry stores: @@ -81,213 +98,239 @@ Each entry stores: - `source`: the original user input (registry name, OCI reference, or `git://` reference), preserved verbatim so upgrade can re-resolve it - `resolvedReference`: the concrete OCI reference or git URL that source resolved to - `digest`: the checkout pin — OCI manifest digest or git commit hash used to fetch the artifact -- `contentDigest`: a deterministic SHA-256 dirhash of the materialized skill file set (the integrity primitive for `--check` and always-on verification). This closes the gap where a git commit hash pins history, not the skill subdirectory's content, and avoids relying on SHA-1 as the content-integrity anchor. +- `contentDigest`: a deterministic SHA-256 dirhash of the materialized skill file set (the integrity primitive for `--check` and always-on verification) +- `provenance` (optional): pinned Sigstore publisher identity — `signerIdentity`, `certIssuer`, and optionally `repositoryUri`, `sigstoreUrl` (mirrors the existing registry provenance shape in `docs/arch/06-registry-system.md`) +- `unsigned: true` (optional): explicit marker when install used `--allow-unsigned`; visible in lock diffs so reviewers see the exception - `requiredBy` (optional): parent skill names for transitively materialized dependencies `source` is never rewritten, so future upgrades keep re-resolving the same input. -The schema deliberately omits `installedAt`. Every major lock file, including npm, pnpm, yarn, Cargo, Go, and Poetry, stores identity, source, and integrity, not chronology. Reproducibility is guaranteed by the digest, not a timestamp; timestamps are environment-local and produce meaningless merge churn on no-op regenerations. "When was this pinned" is answered better by `git blame`; `git log -p -- toolhive.lock.yaml` serves any recency or audit need. The CLI `--help` for sync points operators to that command for pin history. +The schema deliberately omits `installedAt`. Every major lock file stores identity, source, and integrity, not chronology. "When was this pinned" is answered by `git blame`; the CLI `--help` for sync points operators to `git log -p -- toolhive.lock.yaml`. -The lock stays strictly client-agnostic: there is no per-entry `clients` field. Client sets are machine-local, so client targeting is a sync-time concern handled by `--clients` or client auto-detection. +The lock stays strictly client-agnostic: there is no per-entry `clients` field. -#### Trust model (v1 vs v2) +#### Trust model -In v1, a committed lock entry is an **assertion**, not an externally verified fact. The `digest` and `contentDigest` are verified against themselves at install and sync time (reproducing the pinned bytes and re-hashing on disk), but there is no GOSUMDB, transparency log, or signature that binds `source` to `digest` independently of the lock author. The trust root is **PR review of the lock diff**, the same model teams use for any committed dependency pin before external attestation exists. Re-deriving digests from `source` at sync time would defeat pinning (two machines would diverge whenever the catalog moves); sync therefore installs exactly what the lock says. +v1 combines **three layers**: -v2 (out of scope for this RFC, tracked as a follow-on milestone) adds an external trust layer: Sigstore verification per THV-0030, and/or transparency-log-style source-to-digest binding so a tampered lock entry can be checked against a record outside the repo. +1. **Sigstore signature verification** — every install/sync/upgrade verifies that the artifact at the pinned digest carries a valid Sigstore signature (Fulcio certificate + Rekor transparency log entry for OCI; gitsign for git commits). Unsigned artifacts are rejected unless `--allow-unsigned` is passed at install time and recorded in the lock. +2. **Pinned publisher identity** — the lock records `provenance.signerIdentity` and `provenance.certIssuer` observed at first install. Subsequent sync/upgrade verifies the signature matches the locked identity; upgrade refuses signer changes without `--allow-signer-change`. +3. **PR review of the lock diff** — teammates review committed lock changes (including `provenance` blocks and any `unsigned: true` markers) before merge, the same way teams review dependency updates. + +**First-trust establishment:** + +- **Catalog-first:** when a skill resolves through a catalog entry that carries provenance metadata (`signer_identity`, `cert_issuer` in the registry schema), install verifies the artifact signature against that expected identity before writing the lock entry. +- **TOFU fallback:** for direct OCI or git refs with no catalog provenance, the verified signer identity from the first install is recorded in the lock and displayed prominently; sync and upgrade enforce it thereafter. + +Re-deriving digests from `source` at sync time would defeat pinning; sync installs exactly what the lock says and verifies signatures against pinned identity. + +**Offline verification:** Sigstore bundles (certificate chain + transparency log entry) are stored alongside local install state (SQLite or adjacent state dir). `sync --check` re-hashes on-disk content against `contentDigest` **and** re-verifies the stored bundle + locked identity offline — no network, suitable for hermetic CI. Rekor is consulted online only at install/upgrade time when fetching new artifacts. + +#### Signing on publish + +`thv skill push` signs the pushed OCI artifact after upload: + +- **Keyless by default:** Fulcio short-lived certificate + Rekor transparency log entry, using ambient OIDC in CI (GitHub Actions) or browser-based flow for local pushes. Signature stored as an OCI referrer attached to the artifact digest. +- **`--key`:** optional key-based signing (cosign key pair) for air-gapped or key-managed setups. +- Implementation extends toolhive-core's existing `container/verifier` package (`sigstore-go`, already used for MCP image verification in `thv run --image-verification`) for skill OCI referrers. + +Stacklok catalog skills are signed in CI as a coordinated workstream (not a blocker for this RFC's acceptance). + +#### Verification on consume + +Install, sync, and upgrade verify signatures **by default**: + +- **OCI skills:** fetch Sigstore bundle from OCI referrer (or Rekor by digest); verify certificate chain and transparency log entry; check signer identity against catalog provenance (first install) or locked `provenance` block (subsequent operations). +- **Git skills:** verify gitsign signature on the pinned commit; check signer identity the same way. Unsigned git commits require `--allow-unsigned`. +- **Enforcement:** signed skills that fail verification are rejected (`reason: signature-invalid` or `signer-mismatch`). Unsigned skills are rejected unless `--allow-unsigned` at install (recorded as `unsigned: true` in lock; sync inherits the exception). #### Transitive dependencies -Skills declaring `toolhive.requires` in frontmatter materialize dependencies at install time. Project-scope installs **record** transitively materialized skills in the lock (Cargo.lock model: record what was installed, do not build a resolver). Each dependency entry includes `requiredBy: [parent]`. `sync` installs them at their pinned digest and `contentDigest`; they are never `--prune` candidates while a parent in the lock still requires them. Constraint-based resolution (`^1.0.0` ranges, dependency graphs) remains a non-goal. +Skills declaring `toolhive.requires` in frontmatter materialize dependencies at install time. Project-scope installs **record** transitively materialized skills in the lock (Cargo.lock model) with `requiredBy: [parent]` and full provenance. `sync` installs them at their pinned digest, `contentDigest`, and verified identity; they are never `--prune` candidates while a parent requires them. #### Component Changes -- Add a new `pkg/skills/lockfile` package for schema handling, load/save, and file-locked upsert/remove using `pkg/fileutils.WithFileLock` in the same pattern as config writes. Marshalling is deterministic, with sorted entries. Entry fields are validated at load time (see Input Validation). -- Update `pkg/skills/skillsvc` install and uninstall hooks so project-scope installs upsert lock entries (including transitive deps with `requiredBy`), recording the original `opts.Name` as `Source` before any internal resolution, and uninstalls remove them. **A failed lock write on project-scope install exits non-zero** with a clear message ("skill installed but lock NOT updated — do not commit; re-run or fix permissions"). Files and DB records stay in place (no rollback), but the command fails so CI and humans cannot miss a stale lock. User-scope installs never touch the lock. -- Set a `managed: true` marker on SQLite install records created by project-scope locked installs. `--prune` removes only skills previously lock-managed that are no longer in the lock (`removed-from-lock`); out-of-band installs (`never-managed`) are reported but never pruned. -- Add `Sync`, which reads the lock, compares each entry against on-disk content (via `contentDigest`) and `SkillStore` state, installs strictly by pinned `resolvedReference@digest`, and reports unmanaged project-scope skills split into `never-managed` vs `removed-from-lock`. A locked skill whose local `contentDigest` differs is reinstalled and reported as `drifted`. Digest verification is **always-on on every install path**, including OCI cache hits. With `--prune`, it uninstalls only `removed-from-lock` skills (listed in the pre-flight prompt). It never rewrites the lock. -- **Pre-install gate:** on an interactive TTY, `sync` prints a pre-flight summary (name, source, digest, and contentDigest for entries to install, drift, or prune) and asks `Install? [y/N]` defaulting to **No**. `--yes` skips the prompt for scripts; non-interactive without `--yes` fails closed. This gates the "one command after `git pull`" flow before AI-followed instructions land on disk. -- Add `--check` to `thv skill sync`: computes whether installed on-disk content matches the lock by **re-hashing skill files against `contentDigest`** in every detected client directory, plus file-presence checks. Changes nothing. Exits non-zero if any entry is missing, drifted, or has absent client files. This verifies *installed state matches the lock*; it is not a stale-lock freshness gate (see Upgrade below). -- Add `--adopt` to `thv skill sync`: writes lock entries for existing project-scope installs using their current digests and contentDigests. On first run with no lock entries but existing installs, the CLI prints: *"No lock entries yet, but N skills are installed locally. They are unmanaged. To pin them, run `thv skill sync --adopt` or `thv skill install --scope project` each."* -- Add `Upgrade`, which re-resolves each entry's `source` exactly as a fresh `thv skill install ` would. If the digest changed, upgrade installs and rewrites the entry. If re-resolution yields a **different `resolvedReference`** (not merely a new digest for the same ref), upgrade refuses without `--allow-ref-change` and highlights the change in the report — mitigating catalog-redirect and typosquat TOCTOU. Teams should prefer fully-qualified OCI refs in `source` for security-sensitive skills. Immutable sources (OCI `@sha256:` digests, full commit hashes) are `not-upgradable` without network contact. `source` is never rewritten. -- Bare `thv skill upgrade` upgrades all lock entries. Use `thv skill upgrade --preview --fail-on-changes` as an optional CI **freshness** gate: exits non-zero if any mutable source would re-resolve to a new digest (distinct from `--check`'s integrity gate). -- `--preview` (formerly `--dry-run`) prints what would change. **It is not side-effect-free:** there is no peek API for OCI or git, so preview still fetches artifacts into the local cache or clones repos, skipping only extraction and FS/DB writes. `--help` and startup banner state this explicitly. Fetches are bounded (max body size, context timeout). `--check` does not populate the cache. -- `Sync` and `Upgrade` process every entry even when some fail: failures carry a typed `reason` (see API Changes); successes stay; CLI exits non-zero on any failure. During upgrade, per-entry lock rewrite means a failure after install but before lock write leaves brief inconsistency, flagged in the report. -- Sync and upgrade installs use a public `PreserveSource` install option so the entry's `Source` is not overwritten with an already-resolved reference (replacing the unexported `installInternal` pattern). +- Add `pkg/skills/lockfile` for schema handling (`provenance`, `unsigned`, `contentDigest`, `requiredBy`), load/save, file-locked upsert/remove. Entry fields validated at load time. +- Extend toolhive-core `container/verifier` (or a thin `pkg/skills/verifier` wrapper) for OCI referrer verification and gitsign commit verification, reusing `sigstore-go`. +- **`thv skill push`:** after OCI upload, sign artifact (keyless default; `--key` optional); store signature as OCI referrer. +- Project-scope install/uninstall hooks upsert lock entries (including transitive deps), record verified `provenance`, fail non-zero on lock write failure. Store Sigstore bundle with install state for offline re-verification. +- **`managed: true`** SQLite marker for lock-managed installs; `--prune` only removes `removed-from-lock` skills. +- **`Sync`:** content re-hash, offline signature re-verification, pre-install gate (interactive `[y/N]`), `--check`, `--adopt`, prune hardening. +- **`Upgrade`:** re-resolve `source`, verify new artifact signature, refuse ref changes without `--allow-ref-change`, refuse signer changes without `--allow-signer-change`. +- **`--preview`** (not side-effect-free; fetches to cache). **`--preview --fail-on-changes`** as optional CI freshness gate. +- Typed failure `reason` enum includes `signature-invalid`, `signer-mismatch`, `unsigned-rejected`. +- Public `PreserveSource` install option; separate `SkillLockService` interface. #### API Changes -HTTP API changes are additive. Go changes introduce a **new interface** without breaking existing implementers: +HTTP API changes are additive. Go: new `SkillLockService` interface; `SkillService` unchanged. -- `POST /skills/sync` with body `{projectRoot, clients, prune, check, adopt, yes}` returns a report with installed, drifted, upToDate, neverManaged, removedFromLock, pruned, and failed skills. With `check: true`, nothing is installed or pruned. -- `POST /skills/upgrade` with body `{projectRoot, names, preview, failOnChanges, allowRefChange, clients}` returns per-skill outcomes (upgraded, upToDate, notUpgradable, refChangeBlocked, failed) with old/new digests where relevant. -- Project-scope install responses exit non-zero (HTTP 500 or equivalent) when the lock write fails after a successful install. -- Define a new **`SkillLockService`** interface with `Sync` and `Upgrade` methods; `skillsvc` satisfies both `SkillService` and `SkillLockService`. The existing `SkillService` interface is **unchanged**, preserving compile compatibility for external implementers in `toolhive-git-skills` and `stacklok-enterprise-platform`. `pkg/skills/client` gains HTTP methods for the new endpoints. -- Per-entry failures include a typed `reason` enum: `registry-unreachable`, `digest-missing`, `validation-rejected`, `lock-write-failed`, `ref-change-blocked`, `unknown`, plus a human-readable `error` string. -- **Exit codes:** `0` clean; `2` drift or check failure; `3` partial failure (see report); `4` validation or policy rejection. +- `POST /skills/sync` body `{projectRoot, clients, prune, check, adopt, yes}`. +- `POST /skills/upgrade` body `{projectRoot, names, preview, failOnChanges, allowRefChange, allowSignerChange, clients}`. +- `POST /skills/push` gains signing options `{keyless, key}` (or CLI-only if push stays CLI-direct). +- Project-scope install: non-zero on lock write failure; body gains `allowUnsigned`. +- Failure `reason` enum: `registry-unreachable`, `digest-missing`, `validation-rejected`, `lock-write-failed`, `ref-change-blocked`, `signature-invalid`, `signer-mismatch`, `unsigned-rejected`, `unknown`. +- **Exit codes:** `0` clean; `2` drift or check failure; `3` partial failure; `4` validation or policy rejection (includes signature failures). #### Configuration Changes -None beyond lock discovery. The lock file is project-level, discovered using **`git rev-parse --show-toplevel` semantics** (handles worktrees where `.git` is a file, not a directory). `--project-root` provides an override. If there is no enclosing git repository and no `--project-root`, the command errors with: *"no git repository found (or not inside one); pass --project-root to specify the project root"*. Monorepo sub-projects use explicit `--project-root `. No global config is added. +Lock discovery via `git rev-parse --show-toplevel` semantics. `--project-root` override. No global config added. #### Data Model Changes -The SQLite `installed_skills` table gains a boolean `managed` column (or equivalent flag) set `true` for project-scope locked installs. No other schema changes. The lock file is a committed YAML artifact; SQLite holds runtime install state. `sync` reconciles lock, DB, and on-disk content. This coexists with [THV-0041](./THV-0041-sqlite-state-management.md): SQLite remains the unified *local runtime* store; the lock is the shareable, reviewable *committed pin*. Drift between them is expected transiently and is what `sync` repairs. +SQLite `installed_skills` gains `managed` flag. Sigstore bundles stored per install (adjacent blob or dedicated column/table). Lock file is committed YAML; SQLite is local runtime state. Reconciled with [THV-0041](./THV-0041-sqlite-state-management.md). ## Security Considerations ### Threat Model -A malicious or compromised lock file, such as one introduced through a tampered PR, could pin a skill to a known-bad digest. The threat is an attacker committing a lock entry pointing to malicious skill content that teammates then sync. Attacker capabilities: write access to the project repo or ability to merge a PR. - -**v1 trust boundary:** a committed digest is an assertion verified against itself at install time, not against an external record. PR review of the lock diff is the primary control until v2 adds Sigstore (THV-0030) and/or transparency-log binding. +Primary threat: a tampered lock entry (via merged PR) pins malicious skill content or a malicious publisher identity that teammates then sync. -`sync` amplifies this threat: it can turn "review what you install" into "run one command after `git pull`". The pre-install confirmation gate (default `[y/N]`) mitigates this for interactive use; `--yes` is required for scripted/CI sync. +**v1 controls:** -Additional considerations: +- Sigstore signature verification binds artifact bytes to a publisher identity independently of the lock author. +- Locked `provenance` block prevents identity substitution on sync/upgrade without `--allow-signer-change`. +- `unsigned: true` markers are visible in lock diffs for reviewer scrutiny. +- Pre-install confirmation gate (interactive default No) before AI-followed instructions land on disk. +- PR review of lock diff remains the human gate for accepting new pins and identity changes. -- OCI `sha256:` digests and `contentDigest` dirhashes are collision-resistant; git commit hashes remain SHA-1 on most hosts and pin history, not content — `contentDigest` is the content-integrity anchor. -- `sync --prune` is destructive: a tampered lock removing entries combined with `--prune --yes` deletes previously lock-managed skills. Prune is opt-in, gated by the pre-flight prompt, and limited to `removed-from-lock` (never out-of-band installs). -- Upgrade re-resolution of bare registry names is a TOCTOU vector; `--allow-ref-change` gates reference changes. +Residual risks: `--allow-unsigned` and `--allow-signer-change` are explicit escape hatches; catalog compromise could supply wrong expected identity on first install (mitigated by catalog curation). ### Authentication and Authorization -Lock-file writes happen server-side in `skillsvc`, gated by the same API auth as existing skill install/uninstall. `sync` and `upgrade` reuse the existing OCI Docker credential chain and git token auth paths (transport auth, not catalog mapping auth). +Lock-file writes gated by existing API auth. Sigstore verification uses Fulcio/Rekor public infrastructure (no new credential handling for verification). Signing uses ambient OIDC (CI) or browser flow (local). ### Data Security -The lock file contains no secrets, only public references and content digests. It is committed to git by design. +Lock file contains no secrets — public references, digests, and publisher identities only. Sigstore bundles stored locally contain public certificate chains. ### Input Validation -The lock file is the one hand-editable input this feature introduces. `lockfile.Load` validates entry names via `ValidateSkillName`, rejects malformed `digest` and `contentDigest` formats, and validates `requiredBy` references before any entry is acted on. - -Beyond load, `resolvedReference` and `digest` flow through existing OCI and git resolution paths (SSRF guards, shell-injection ref validation, supply-chain name checks). A tampered lock can only pin references that would pass a manual `thv skill install`; it cannot be independently proven correct without v2 attestation. - -**`projectRoot` validation (CWE-22):** the API body field must be (1) absolute, (2) canonicalized with symlinks resolved, (3) within a git-rooted tree matching auto-detection rules. For the HTTP API, `projectRoot` must fall under a daemon-configured `--serve-root`; callers cannot direct the daemon to arbitrary filesystem locations. +`lockfile.Load` validates names, digest formats, `provenance` fields, `requiredBy` references. `projectRoot` validation (CWE-22): absolute, symlink-canonicalized, git-rooted; HTTP API constrained under daemon `--serve-root`. ### Secrets Management -None. The lock stores no tokens. +None in the lock. Signing keys managed by Sigstore keyless flow or user-supplied `--key`. ### Audit and Logging -Lock upsert/remove errors are logged via `slog.Warn`. Failed project-scope lock writes fail the command (non-zero exit). `sync` and `upgrade` return structured reports with typed failure reasons suitable for audit. The git-committed lock provides cross-machine provenance via `git blame` and `git log -p -- toolhive.lock.yaml`. +Structured reports with typed failure reasons including signature outcomes. Git-committed lock provides provenance history. Rekor provides public transparency log for signatures. ### Mitigations -- `contentDigest` plus always-on verification and `--check` re-hashing detect on-disk tamper and cache substitution. -- Pre-install confirmation gate (interactive default No) before sync installs AI-followed instructions. -- `upgrade` refuses reference changes without `--allow-ref-change`; immutable pins cannot be re-resolved. -- Failed lock writes fail the install command for project scope. -- Transitive deps are locked with `requiredBy` provenance; prune cannot delete out-of-band or required skills. -- v2 milestone: Sigstore verification and source-to-digest transparency binding. +- Sigstore verification + pinned identity closes the unauthenticated-digest finding. +- `contentDigest` re-hash + offline bundle re-verification in `--check`. +- Pre-install gate, ref-change and signer-change gates on upgrade. +- Transitive deps locked with `requiredBy`; prune limited to `removed-from-lock`. +- gitsign for git sources; `--allow-unsigned` recorded explicitly in lock. ## Alternatives Considered ### Alternative 1: Separate manifest and lock file -- **Description:** A hand-edited `toolhive.yaml` declares desired skills with version constraints, and the lock resolves them. -- **Pros:** Supports `^1.0.0` ranges; upgrade is "re-resolve constraints". -- **Cons:** Requires building a dependency resolver; doubles the surface area with two files to keep in sync. -- **Why not chosen:** The single-lock model delivers reproducibility now and leaves the door open to a manifest layer later. +- **Why not chosen:** Single-lock model delivers reproducibility now; manifest layer can be added later. -### Alternative 2: Store pin data only in the SQLite store +### Alternative 2: Store pin data only in SQLite -- **Description:** No committed file; sync reads from a shared or replicated DB. -- **Pros:** No new file format. -- **Cons:** Not portable; cannot be reviewed in a PR; defeats the committed pin goal. -- **Why not chosen:** Fundamentally does not meet reproducibility and audit goals. +- **Why not chosen:** Not portable or reviewable in PRs. ### Alternative 3: Per-client lock entries -- **Description:** Pin which client apps each skill installs into. -- **Pros:** Precise per-client control. -- **Cons:** Couples the lock to the local client set; bloats entries. -- **Why not chosen:** Content pinning is the real need; client targeting is a sync-time local concern. +- **Why not chosen:** Content pinning is the need; client targeting is sync-time local. ### Alternative 4: Re-derive digests from source at sync time -- **Description:** On sync, re-resolve each entry's `source` and verify the committed digest matches the currently-resolved digest. -- **Pros:** Binds source to digest without an external trust root. -- **Cons:** Defeats pinning — sync would install whatever the catalog serves today, not what the lock author intended. Two machines diverge whenever the catalog moves. -- **Why not chosen:** Contradicts the core reproducibility goal. External attestation (v2) is the correct fix for unauthenticated digests. +- **Why not chosen:** Defeats pinning. Sigstore identity pinning (v1) is the correct binding mechanism. + +### Alternative 5: Defer signing to v2 + +- **Why not chosen:** Skills are AI-executed instructions; shipping without signature verification repeats the panel review's core finding. toolhive-core already has sigstore-go verification for MCP images; extending to skills is incremental. ## Compatibility ### Backward Compatibility -HTTP API changes are additive. The existing `SkillService` Go interface is unchanged (new `SkillLockService` carries sync/upgrade). Existing project-scope installs start writing a lock file on next install. Pre-existing installs without lock entries work; sync reports them as `never-managed` and offers `--adopt`. User-scope installs are unchanged. +HTTP API additive. `SkillService` unchanged (`SkillLockService` is new). Existing unsigned skills in the wild require `--allow-unsigned` at first project-scope install (recorded in lock). Stacklok catalog signing rollout is coordinated separately. -**Not fully backward compatible:** project-scope installs now fail non-zero when the lock write fails (previously best-effort warn). POC consumers must update to handle the new exit semantics. +**Behavior changes from POC:** lock write failure exits non-zero; unsigned skills rejected by default. ### Forward Compatibility -Readers hard-error on unknown lock `version`. This RFC proposes the `toolhive.lock.yaml` filename and owns `version` + `skills:` only; sibling keys such as `plugins:` are a contract proposal for THV-0077 or a dedicated project-lock RFC to ratify. The `source` field is the extensibility hook for a future manifest layer. New entry fields use `omitempty`. +Unknown lock `version` hard-errors. This RFC owns `skills:` only; sibling keys are a contract proposal. New entry fields use `omitempty`. ## Implementation Plan -A POC implementation exists in [toolhive#5715](https://github.com/stacklok/toolhive/pull/5715). Post-RFC acceptance, split and extend it into reviewable PRs. +POC in [toolhive#5715](https://github.com/stacklok/toolhive/pull/5715); extend and split post-acceptance. ### Phase 1: Lock file package -- `pkg/skills/lockfile` schema (`contentDigest`, `requiredBy`), load-time validation, file-locked ops. +- Schema (`provenance`, `unsigned`, `contentDigest`, `requiredBy`), validation, file-locked ops. ### Phase 2: Install/uninstall hooks -- Project-scope upsert (including transitive deps), uninstall remove, fail-on-lock-write, `managed` SQLite flag. +- Upsert with provenance, fail-on-lock-write, `managed` flag, Sigstore bundle storage. ### Phase 3: Sync -- `SkillLockService.Sync`, content re-hash, pre-install gate, `--check`, `--adopt`, prune hardening, API + CLI. +- Content re-hash, offline signature verify, pre-install gate, `--check`, `--adopt`, prune hardening. ### Phase 4: Upgrade -- `SkillLockService.Upgrade`, `--preview`, `--fail-on-changes`, `--allow-ref-change`, API + CLI. +- `--preview`, `--fail-on-changes`, `--allow-ref-change`, `--allow-signer-change`. -### Phase 5: Docs +### Phase 5: Signing and verification -- `docs/arch/12-skills-system.md`, CLI docs, swagger, exit-code table. +- `thv skill push` keyless signing; OCI referrer + gitsign verification on consume; extend `container/verifier`; catalog CI signing (coordinated). -### Phase 6: v2 trust layer (follow-on, out of scope here) +### Phase 6: Docs -- Sigstore verification per THV-0030; optional transparency-log source-to-digest binding. +- `docs/arch/12-skills-system.md`, CLI docs, swagger, signing/verification guide. ### Dependencies -None blocking. Relies on existing `gitresolver`, `ociskills.RegistryClient`, and THV-0041 SQLite store. +- toolhive-core `container/verifier` (`sigstore-go`) +- Existing `gitresolver`, `ociskills.RegistryClient`, THV-0041 SQLite store +- Sigstore staging infrastructure for E2E tests ## Testing Strategy -- **Unit tests:** lockfile round-trip, validation, `contentDigest` computation, transitive `requiredBy` entries; fail-on-lock-write; managed marker and prune semantics (`never-managed` vs `removed-from-lock`); sync pre-install gate, content re-hash in `--check`, `--adopt`; upgrade ref-change blocking, `--preview`, `--fail-on-changes`; typed failure reasons and exit codes; `SkillLockService` separate from `SkillService`. -- **E2E tests:** install → sync → `--check` (clean, after on-disk tamper, after missing client files) → `--adopt` first-run → upgrade with ref change blocked → `--preview --fail-on-changes` against real GHCR artifacts. -- **Security tests:** malformed lock rejected at load; `projectRoot` path traversal rejected; tampered lock installs only pass existing supply-chain validation; sync gate requires confirmation on TTY. +- **Unit tests:** lockfile schema including `provenance`/`unsigned`; signature verification (valid, invalid, wrong identity); gitsign commit verification; offline bundle re-verification in `--check`; push signing (keyless mock, `--key`); all existing sync/upgrade/prune tests. +- **E2E tests:** signed catalog skill install → sync → `--check` (content + signature offline) → upgrade with signer-change blocked → `--preview --fail-on-changes`; unsigned skill rejected then `--allow-unsigned` recorded; gitsign-signed git skill path. Fixtures against Sigstore staging. +- **Security tests:** tampered lock with wrong identity rejected; `unsigned: true` visible in diff; sync gate on TTY; `projectRoot` traversal rejected. ## Documentation -- Update `docs/arch/12-skills-system.md` with "Project Lock File" section (trust model, contentDigest, transitive deps). -- Regenerate CLI docs via `task docs` (`--preview`, `--check`, `--adopt`, exit codes, pin history pointer). -- User guide: committing `toolhive.lock.yaml`, running `sync` after pull, CI patterns for `--check` vs `upgrade --preview --fail-on-changes`. +- `docs/arch/12-skills-system.md`: lock file, trust model, signing, verification, gitsign. +- CLI docs: `--allow-unsigned`, `--allow-signer-change`, push signing, `--check` offline semantics. +- User guide: committing lock, CI patterns (`--check` integrity vs `upgrade --preview --fail-on-changes` freshness). +- Note in THV-0030 tracking: this RFC delivers the Sigstore integration 0030 listed as future work. + +## Future Work + +- Org-wide signer policy files (allow/deny publisher identity lists). +- Catalog-level trust roots binding registry name to expected signer without per-lock TOFU. ## Open Questions -Resolved during design and panel review; outcomes live in the design sections above. +Resolved during design and review: -1. ~~Sync drift behavior~~ → reinstall, report `drifted`, verify via `contentDigest` re-hash. -2. ~~Per-entry `clients` field~~ → lock stays client-agnostic. -3. ~~Upgrade default target~~ → upgrade all; `--preview --fail-on-changes` for CI freshness. -4. ~~Lock file naming / general lock~~ → `toolhive.lock.yaml` proposed; this RFC owns `skills:` only; sibling keys are a contract proposal. -5. ~~v1 trust root~~ → PR review of lock diff; v2 adds Sigstore / transparency binding. -6. ~~Transitive dependencies~~ → record in lock with `requiredBy`; no resolver. +1. ~~Sync drift~~ → reinstall, report `drifted`, verify via `contentDigest` re-hash. +2. ~~Per-entry `clients`~~ → client-agnostic lock. +3. ~~Upgrade default~~ → upgrade all; `--preview --fail-on-changes` for CI freshness. +4. ~~Lock naming~~ → `toolhive.lock.yaml`; this RFC owns `skills:` only. +5. ~~Trust root / signing timing~~ → **Sigstore signing and verification ship in v1**; identity pinned in lock; PR review of lock diff remains human gate. +6. ~~Transitive deps~~ → record in lock with `requiredBy`. +7. ~~v1 vs v2 trust layer~~ → collapsed; v2 milestone removed. ## References - POC PR: - Research note: -- [THV-0030: Skills Lifecycle Management in ToolHive CLI](./THV-0030-skills-lifecycle-management.md) +- [THV-0030: Skills Lifecycle Management in ToolHive CLI](./THV-0030-skills-lifecycle-management.md) — Sigstore integration deferred there; delivered by this RFC - [THV-0041: SQLite-Based State Management](./THV-0041-sqlite-state-management.md) - [THV-0077: Plugin lifecycle management](./THV-0077-plugins-lifecycle-management.md) -- Prior art (shape, not v1 trust parity): npm `package-lock.json`, pnpm `pnpm-lock.yaml`, `Cargo.lock`, `go.sum`, `poetry.lock` +- toolhive-core `container/verifier` (sigstore-go): MCP image verification precedent +- Prior art (shape): npm `package-lock.json`, `Cargo.lock`, `go.sum` - oras-go cross-origin redirect auth guard: GHSA-vh4v-2xq2-g5cg --- @@ -301,7 +344,8 @@ Resolved during design and panel review; outcomes live in the design sections ab | Date | Reviewer | Decision | Notes | |------|----------|----------|-------| | YYYY-MM-DD | @reviewer | Under Review | Initial submission | -| 2026-07-07 | @JAORMX | Commented | Panel review; RFC revised to address findings | +| 2026-07-07 | @JAORMX | Commented | Panel review; RFC revised | +| 2026-07-08 | @JAORMX | Commented | Requested v1 signing; RFC updated | ### Implementation Tracking