Sign plugin pushes and remove the lock feature gate

[samuv]

Summary

Stack 2 of #6300 gave plugins a lock file, install-time verification, offline sync re-verification, and an upgrade signer guard — but left two ends open. Publishing still had no way to sign, so the only artifact a project-scoped install could accept was one a human had signed by hand out of band; and the whole feature stayed behind TOOLHIVE_PLUGINS_LOCK_ENABLED, which existed precisely because the consume half had no publish half to trust.

This closes both, and completes the tracker's Definition of Done.

• Publishing is signed by default, and keyless only. thv ai-plugin push signs the pushed artifact through toolhive-core's container/signer via --identity-token — acquired automatically from an ambient CI token or an interactive browser sign-in when not given — and attaches the signature next to the artifact where PR7's install verification finds it. --no-sign is the explicit opt-out. A failed signing fails the push: an artifact published as if it were signed, which consumers then have to install with --allow-unsigned, is worse than a push that visibly did not complete.
• No --key. Key signing has no consume-time path — install verification is keyless-only, and a key-signed artifact fails as ErrSignatureInvalid rather than ErrUnsigned, so --allow-unsigned cannot override it. The flag could only produce plugins nobody can install, so it is not offered; pluginsvc.Push also rejects a key set by an in-process caller, since plugins.PushOptions aliases skills.PushOptions and the field still exists. Filed as --key push signing has no install-time verification path (skills and plugins) #6442 (which affects shipped thv skill push --key too); the flag returns for plugins in the change that makes it verifiable.
• The signing fields survive the transport boundary. The plugins push DTO carried only reference, so every flag would have died between the CLI and the service. identity_token and no_sign now thread through pkg/api/v1 and the Go HTTP client, each pinned by a round-trip test. The install response gains the recorded provenance for the mirror-image reason: the CLI is a pure HTTP client, so a dropped provenance block would print every signed install as if it were untracked.
• Trust state is visible. thv ai-plugin info renders the lock file's recorded signer ("Signed by / Cert issuer", with the (provisional) marker for git signatures and (unsigned — explicit exception) for recorded exceptions), and thv ai-plugin install prints the trust outcome on completion. RFC THV-0080 wants the pinned identity shown at decision time, not discovered weeks later inside a signer-mismatch error.
• The feature gate is gone. pkg/plugins/feature_gate.go and every LockFileFeatureEnabled check are removed. Lock recording, sync, upgrade, and signature verification are now standard behavior for project-scoped plugins.
• Docs graduate. docs/arch/14-plugins-system.md gains the trust-model section it never had — TOFU semantics, the allow_unsigned / allow_signer_change escape hatches, offline re-verification, and what deliberately remains trusted on faith — with a pointer from the skills document's lock section, since both systems write the same toolhive.lock.yaml.

Part of #6300. Skills counterpart: #6139. Stacked on #6401, which merged while this was in progress — so this targets main directly.

Type of change

• Bug fix
• New feature
• Refactoring (no behavior change)
• Dependency update
• Documentation
• Other (describe):

Test plan

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

New coverage:

• Signing-method validation: zero methods, both methods, and --no-sign combined with either are each rejected with 400 before anything is pushed.
• The identity-token path reaches the signer with the digest that was pushed; the TOOLHIVE_SIGSTORE_FULCIO_URL / _REKOR_URL overrides reach core's signer.Options; --no-sign never calls the signer at all. A key is refused before the registry or the signer is touched, --key is asserted absent from the command, and a key in the API request body is asserted not to reach the service.
• A failed signing fails the push.
• DTO round-trips for identity_token and no_sign at both the API handler and the Go client; install-response round-trips for signed / provisional / unsigned.
• Info reports each of the four trust states (signed, provisional, unsigned exception, no lock entry), and InstallResult carries the decision install recorded.
• CLI rendering for every trust state on both info and install.
• Gate-removal fallout: the formerly-gated lock, sync, upgrade, and verification tests now run unconditionally; the four tests that existed only to assert gated-off behavior are removed.

E2E: pkg/transport/proxy/streamable's TestMCPGoClientInitializeAndPing fails on this branch, but it also fails on a clean upstream/main worktree — pre-existing and unrelated. Every other unit test passes. task test-e2e was not run locally; test/e2e/cli_plugins_lock_test.go is updated (gate env removed, pushes carry explicit no_sign) and compiles.

API Compatibility

• This PR does not break the v1beta1 API, OR the api-break-allowed label is applied and the migration guidance is described above.

Does this introduce a user-facing change?

Yes, two.

TOOLHIVE_PLUGINS_LOCK_ENABLED is removed. Project-scoped plugin installs now always record into toolhive.lock.yaml and are always signature-verified; thv ai-plugin sync and thv ai-plugin upgrade work without any environment variable. Anyone who was setting the variable can stop. The corresponding effect for users who were not setting it: a project-scoped install of an unsigned plugin now requires --allow-unsigned, which is recorded in the lock file as an explicit exception. User-scoped installs are unaffected.

thv ai-plugin push now signs by default, keylessly. A push with neither --identity-token nor --no-sign is rejected rather than publishing unsigned; when signing is wanted, an OIDC token is acquired automatically (ambient CI token, else browser sign-in). There is no --key flag and no key field on the push API — see above and #6442.

Special notes for reviewers

Scope. 20 non-test, non-generated files, ~270 insertions. Over the 10-file guideline, under the 400-line one. The pieces are one logical change and resist splitting: removing the gate without signed publishing would leave verification with nothing to verify, and the push DTO, the install response, and the CLI renderers each touch the same four files. This mirrors #6139's shape on the skills side.

Deliberate reuse over duplication. plugins.ProvenanceInfo is a type alias for skills.ProvenanceInfo rather than a parallel struct — the THV-0080 provenance contract is shared, and a second struct would only be somewhere for the two to drift. The signer mock is likewise reused from pkg/skills/skillsvc/mocks (it mocks toolhive-core's signer.Signer, not anything skills-specific), matching how pluginsvc already reuses pkg/skills/verifier/mocks. validateSigningInputs is duplicated into pluginsvc — skills' copy is unexported, and this is the established mirror pattern for that package pair.

One drive-by comment fix. Removing the gate made ~45 //nolint:paralleltest // uses t.Setenv via newLockTestService reasons untrue, since the helper no longer calls t.Setenv. I updated the reason text rather than converting the tests to parallel — that would be a much larger, riskier change than this PR should carry, and the suppressions are still load-bearing (paralleltest is enabled). Converting them is reasonable follow-up work.

🤖 Generated with Claude Code

[codecov]

Codecov Report

❌ Patch coverage is 93.97590% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.10%. Comparing base (d1d76c3) to head (cba9ed4).

Files with missing lines	Patch %	Lines
pkg/plugins/pluginsvc/build.go	88.37%	5 Missing ⚠️
pkg/plugins/pluginsvc/verify.go	94.11%	2 Missing ⚠️
pkg/skills/client/client.go	87.50%	1 Missing ⚠️
pkg/skills/identitytoken/acquire.go	75.00%	1 Missing ⚠️
pkg/skills/identitytoken/transport.go	95.65%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6438      +/-   ##
==========================================
- Coverage   78.11%   78.10%   -0.01%     
==========================================
  Files         767      767              
  Lines       74395    74513     +118     
==========================================
+ Hits        58110    58202      +92     
- Misses      16280    16306      +26     
  Partials        5        5              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[JAORMX]

Two merge-blocking gaps remain in the completed plugin trust flow: key-signed pushes have no corresponding consume-time trust path, and gate removal can promote a pre-gate same-digest install to managed/signed without rematerializing or persisting its bundle. I also flagged the silent loss of trust state when info cannot read the lock. CI is green, but these trust-model issues need resolution before merge.

[JAORMX]

Re-reviewed commits after 542b46a. The key-signing dead end was removed, same-digest pre-gate installs now rematerialize and persist their bundle, and lock trust-read errors are propagated. No new blocking findings. CI is green.

[samuv]

Round 3

06581e007..1c765ed1c, on top of the rebase. Both blocking findings fixed, plus the info UX item; each has a threaded reply with the detail. Two things worth pulling up to the top.

The migration finding was a real miss on my part, not a partial fix. Classifying unrecorded entries as drift made sync reinstall them, and lock-driven installs were granted an implicit unsigned exception — so the repair rewrote "no trust decision" into unsigned: true by itself. I made the state worse, not better: an entry that was visibly ambiguous became a standing exception that looks deliberate. Fixed by removing the allowance so only the explicit flag can record an exception, which also means a legacy entry is repaired automatically only when a signature actually verifies, as you specified.

Fixing it surfaced something the finding did not cover: SyncOptions.AllowUnsigned was never forwarded into either reinstall path, so failing closed on its own would have left the migration with no remedy at all — sync --allow-unsigned would have failed identically to bare sync. Both paths now forward it. Flagging it explicitly because a fail-closed path with no exit would have been worse than the bug.

On the info derivation, Managed && Provenance == nil && !Unsigned also matches a deleted lock entry whose DB record still says managed — a state sync treats as RemovedFromLock, not drift. Keyed on the lock entry existing instead, which needed expectedLockTrust split so entry presence is available without a second lock read.

Still open

The artifact-digest binding thread is unresolved and cannot be fixed here — it is entirely in toolhive-core. The trace, the two aggravating factors (the correctly-bound referrers path is only a fallback, and .sig is the path our own signer writes), and a related install/sync digest asymmetry are all in that thread. It still needs a routing decision from you on how to track it; I have not filed anything.

Verification

task test exit 0 · golangci-lint 0 issues across pkg/..., cmd/thv/..., test/e2e/... · new commits build individually · task docs regenerated (trust_unrecorded in the schema).

Size

Unchanged in kind but larger again — this round adds ~4 code files and ~130 lines. The split I proposed last round still applies, and the drift-migration commits now form an even cleaner seam than they did before, since they are self-contained and carry their own tests. Happy to split before you spend another full pass on it.

[JAORMX]

This still lets a same-digest Git reinstall promote a pre-verification project install to a trusted, managed lock entry without rematerializing it. mustPersistTrust only fires for OCI because Git verification has no bundle. If the legacy on-disk tree was modified, the fresh git signature authenticates the commit while the no-op path leaves the modified files active; only a later sync notices the contentDigest mismatch. Treat newly established provenance on an unmanaged existing record as requiring rematerialization even when SigstoreBundle is empty, and cover a drifted same-commit Git migration. (CWE-345)

[samuv]

Confirmed and fixed in c4cba193d. You are right, and the reason it was reachable is worse than "Git verification has no bundle" suggests — it could never have fired for git, because VerifyGit returns resultFromCore(observed, nil). There is no bundle to differ from the stored one, so mustPersistTrust is structurally unreachable on that path rather than merely usually false.

Tracing the rest of it: lockContentDigest hashes opts.LayerData, never the disk. So the entry that gets written asserts the pristine content digest and the freshly verified signer, while dispatchExtraction returned existing verbatim and left the modified tree active. The install reports success and prints the identity. Exactly as you say, only a later sync --check catches it.

I keyed the fix on the managed transition rather than on newly established provenance. Your phrasing would have fixed the case you found and left one beside it: an --allow-unsigned same-digest reinstall of the same legacy record records a contentDigest with no provenance at all and has the identical gap — the entry claims content nobody rematerialized. The invariant that actually holds is narrower to state and wider in coverage:

a lock entry only ever describes content the install that wrote it materialized.

So the condition is storeErr == nil && !existing.Managed && scope == plugins.ScopeProject, and both reasons route to installExtractionUpgradeDigest, which already rematerializes for every client and persists. I checked it does not over-trigger: an already-managed record is unaffected, user scope is excluded, and a fresh install has no record — so it fires only on unmanaged→managed, which is one extra materialization the first time a legacy record gains an entry, and never again.

TestInstallVerification_SameCommitGitMigrationRematerializes covers the drifted same-commit git migration you asked for, and asserts three things: the tampered file is restored to fixture content, the entry records the verified signer, and a following sync --check reports AlreadyCurrent rather than drift — i.e. the entry is self-consistent when written, not only after a repair pass. I verified it fails without the fix, on both the file content and the drift assertion.

One trap worth flagging, because I nearly shipped the test green and useless. alwaysSignedVerifier returns signedResult(), which carries Bundle: []byte(...). Building the test on it would have made mustPersistTrust fire and route the reinstall to the rematerializing path for a reason no git install ever has — passing while exercising nothing. The test uses a new gitSignedVerifier that returns a nil bundle, mirroring what VerifyGit actually does, and the helper's doc comment says why so nobody swaps it back.

docs/arch/14-plugins-system.md:202 states the invariant and why the condition is not "a bundle was freshly verified".

[JAORMX]

This forwarding makes --allow-unsigned authorize repairing a legacy lock entry with unrecorded trust into a standing unsigned: true exception, but the flag help and pluginsSyncRequest.AllowUnsigned still say it is only for --adopt. That can make an operator approve a different trust transition than the one described, and the new info remediation is undiscoverable from sync --help. Please describe both uses and regenerate the CLI/server docs.

[samuv]

Fixed in d32980e85. Agreed on both halves — the flag authorizes a second, materially different trust transition, and the operator reading --help sees only the first.

--allow-unsigned now reads:

Record plugins as unsigned in the lock file: when adopting installs whose signature state cannot be established (--adopt), and when repairing an entry that records no trust decision and whose content is unsigned

and pluginsSyncRequest.AllowUnsigned names both cases, including that the repair fails closed without it — which is the part that makes the flag necessary rather than merely permissive. docs/cli/thv_ai-plugin_sync.md and the server docs are regenerated.

That also closes the discoverability gap you point at: info says run 'thv ai-plugin sync', and sync --help now names "an entry that records no trust decision" as a thing the flag exists for, so the two ends of the remedy meet.

I left thv skill sync --allow-unsigned alone deliberately — skills has no unrecorded-trust refusal (errLockTrustUnrecorded and unrecordedTrustError are plugins-only), so its help text is still accurate and changing it would describe behaviour that package does not have.

The same regeneration picks up the stale TrustUnrecorded schema description that was failing Docs / Verify Swagger Documentation — the committed text was an earlier draft of the doc comment, from before I dropped Managed from the condition. That check should be green now.

[JAORMX]

Re-reviewed commits after 34a17a7 against the current main diff with Spec, Standards, Security, Architecture, UX, DevEx, and Reuse lenses. The redirect and implicit-unsigned migration blockers are fixed, and the unrecorded trust state is now visible. The artifact-digest binding blocker remains unresolved (discussion_r3894939583), and a same-digest Git migration can still record verified provenance without rematerializing drifted files. I also flagged the stale --allow-unsigned contract. CI currently fails Docs / Verify Swagger Documentation because the generated TrustUnrecorded schema description is stale.

[samuv]

Round 4 pushed: c4cba193d, d32980e85.

Finding	Status
Same-digest Git migration records provenance without rematerializing (CWE-345)	Fixed — c4cba193d
Stale --allow-unsigned contract	Fixed — d32980e85
Docs / Verify Swagger Documentation failing	Fixed — same regeneration
Artifact-digest binding (r3894939583)	Fixed upstream in stacklok/toolhive-core#263; needs a bump here

Two things worth your attention rather than just the diff.

I widened the Git fix past what you asked for. Keying on newly established provenance would have fixed the case you found and left one beside it: an --allow-unsigned same-digest reinstall of the same legacy record writes a contentDigest with no provenance and the identical gap. So the condition is the unmanaged-to-managed transition, which covers the OCI, git, and unsigned shapes under one invariant — a lock entry only ever describes content the install that wrote it materialized. Details and the over-triggering analysis in the thread.

The core fix is not a free bump. #263 settles a second inconsistency I hit while writing it: online verification bound .sig-derived bundles to the payload digest, offline bound them to the artifact digest the caller passes, and sync.go passes the artifact digest — so offline re-verification of a .sig-derived bundle could not have worked. Settling that means bundles persisted by v0.0.42 from a cosign signature carry no payload and now fail closed instead of appearing to verify. The bump therefore needs a companion decision here (re-fetch and re-store, or treat payload-less stored bundles as "must re-verify online"), which I would rather land as its own PR. Your call on which behaviour.

Standing offer, third time: this PR is well past the 400-line / 10-file budget and I think it splits cleanly. Say the word and I will propose the split rather than keep growing it.