Honour a pinned cosign key on plugin sync and upgrade

[samuv]

Summary

PR 1 of this stack lets a plugin be installed project-scoped against a cosign public key, pinned in lockfile.Provenance.PublicKey. But pluginsvc's lock-driven operations were still keyless-only, so that pin was unusable in practice the moment you ran anything other than install:

• sync never settled. verifyStoredSignature handed the entry to VerifyBundleOffline, which cannot check a key-pair bundle. Sync reads a verification refusal as drift it can heal by reinstalling — so a key-pinned plugin was reported modified on every run, and sync --check failed permanently on a project that was in fact intact.
• upgrade could not run at all. guardSignerChange → probeCandidateSigner → VerifyOCI(..., nil) returns ErrKeySigned for a key-signed candidate, which fell into the generic failure arm — so the entry was reported as a broken signature where a policy decision was intended.
• sync --adopt refused opaquely, with a generic "verifying stored bundle for adoption" wrapper around ErrKeySigned.

This transliterates the skills fix (#6478) onto pkg/plugins:

• sync re-verifies against the pinned key. verifyStoredKeySignature decodes entry.Provenance.PublicKey and calls VerifyBundleOfflineWithKey. The signature is checked against the lock entry's digest, and the simple-signing payload is recovered from the stored bundle rather than rebuilt from a reference — a payload reconstructed from a reference verifies against whatever that reference claims, which is exactly the check a signature lifted from another artifact passes. The fail-closed missing-bundle branch is unchanged in behaviour but now reports the anchor via lockedAnchorDescription, so a key pin no longer prints signer "". The plugins-specific errLockTrustUnrecorded drift handling is untouched.
• Adoption refuses a key-signed install with something actionable. Adoption back-fills trust from what the bundle reveals, and a key-pair bundle reveals no identity and does not carry the key; recording it as unsigned instead would file a false trust decision about an artifact that is signed. It is now a 403 wrapping ErrKeySigned (so classifySyncFailure reports FailureReasonKeySigned) naming thv ai-plugin install --public-key, and stating that --allow-unsigned is not a substitute.
• upgrade applies the pinned key to the candidate. guardKeyedSignerChange verifies against it, which is the evidence the signer has not changed — there is no certificate identity to compare. The arms are split by whether the remedy the CLI prints actually works. Only a candidate that moved to keyless signing is SignerChangeBlocked — --allow-signer-change genuinely resolves that by dropping the key and taking the keyless path — and it carries the identity it moved to, because the renderer infers "unsigned" from an absent NewSignerIdentity. A candidate that lost its signature is Failed/FailureReasonUnsignedRejected: the override bypasses this guard but not verification, and upgrade has no unsigned-consent flag to pair with it, so it would dead-end in unsigned-rejected one step later. A candidate signed by a different key is Failed/FailureReasonSignatureInvalid, because re-anchoring in place is not offered. Both failures name uninstall-and-reinstall with the flag that applies.

Scope stays install-only for --public-key, exactly as skills v1: sync and upgrade take no key of their own, and no in-place re-anchor.

Part of #6442

Type of change

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

Test plan

• Unit tests (task test)
• E2E tests (task test-e2e)
• Linting (task lint-fix)
• Manual testing (describe below)

Unit tests only, mirroring what skills shipped (there is no E2E for the key path on either surface). Every new test was confirmed to fail with the two non-test files reverted, so each one pins behaviour this PR adds rather than behaviour that already worked — the one deliberate exception is the --allow-signer-change test, which pins a PR 1 invariant this PR must not break.

Pinned explicitly: a key-pinned entry with a stored bundle verifies via VerifyBundleOfflineWithKey and VerifyBundleOffline is asserted never called (reaching the keyless path is the bug, and it fails closed in a way that looks like drift); sync --check and an apply both report an intact key-pinned project as AlreadyCurrent; a missing bundle names "a cosign public key" and not signer ""; an undecodable pinned key fails closed rather than verifying nothing; adopting a key-signed install is a 403 wrapping ErrKeySigned classified FailureReasonKeySigned, writes no lock entry, and is unchanged by --allow-unsigned; the four keyed upgrade transitions (same key proceeds, keyless blocks, unsigned blocks, different key fails with the re-anchor remedy) with the keyless probe asserted never called and no invented NewSignerIdentity on the blocked arms; an undecodable pinned key fails the plan rather than reporting a signer change.

Two end-to-end upgrade tests run against a real OCI store with a mocked registry client, driving install → publish → upgrade: one proves the candidate guard and the install applyUpgrade performs both verify against the pinned key (exactly two VerifyOCIWithKey calls, VerifyOCI never), rather than assuming that from resolveKeyAnchor; the other proves --allow-signer-change drops the key and re-anchors to the observed identity.

Changes

File	Change
pkg/plugins/pluginsvc/sync.go	key branch in verifyStoredSignature, new verifyStoredKeySignature, anchor-aware missing-bundle message, ErrKeySigned refusal in adoptionTrust
pkg/plugins/pluginsvc/upgrade.go	key dispatch in guardSignerChange, new guardKeyedSignerChange and blockKeyToKeylessChange
pkg/plugins/pluginsvc/sync_verify_test.go, upgrade_verify_test.go	tests
cmd/thv/app/ai_plugin_upgrade_test.go	CLI-output assertions for the blocked/failed renderings

Does this introduce a user-facing change?

Yes. A plugin installed with thv ai-plugin install --public-key now behaves like any other pinned plugin: thv ai-plugin sync reports it as current instead of as drift on every run, sync --check passes on an intact project, and thv ai-plugin upgrade evaluates candidates against the pinned key and reports a signer change or a failure with the remedy that actually applies. thv ai-plugin sync --adopt on a key-signed install now explains that it must be installed with --public-key, instead of failing with a generic stored-bundle error. Nothing changes for keyless or unsigned plugins.

Special notes for reviewers

• This does not close --key push signing has no install-time verification path (plugins) #6442. PR 3 restores --key on thv ai-plugin push and is the one that closes it — restoring the publish flag any earlier would recreate exactly the publish-only dead end the issue was opened about. docs/arch/14-plugins-system.md still documents plugin signing as keyless-only for the same reason; the trust-model rewrite belongs with PR 3, so the doc flips when the feature is complete rather than describing a half-built path.
• Two deliberate deviations from Honour a pinned cosign key on sync and upgrade #6478, both in the direction of current main:
  • Honour a pinned cosign key on sync and upgrade #6478 added describeLockAnchor to skillsvc/sync.go, which is byte-identical to lockedAnchorDescription in skillsvc/verify.go — two names for the same function in the same package. PR 1 already gave pluginsvc lockedAnchorDescription, so this reuses it rather than landing the duplicate.
  • VerifyBundleOfflineWithKey has since lost the imageRef parameter Honour a pinned cosign key on sync and upgrade #6478 passed it: the payload is recovered from the stored bundle instead of reconstructed from a reference, which is the signature-transplant fix. verifyStoredKeySignature therefore takes no reference, and the "entry pins no reference" arm Honour a pinned cosign key on sync and upgrade #6478's diff shows no longer exists.
• No git arm in guardKeyedSignerChange. The lock schema refuses publicKey on a git entry (lockfile/validation.go — a commit signature is verified against a certificate, never a key), so a key-pinned entry is always OCI and latest.commitPayload is always empty there. The comment states the assumption at the call site.
• Review round (8875c6a). Two arms of guardKeyedSignerChange were wrong and are fixed: an unsigned candidate was reported as a signer change, whose remedy --allow-signer-change cannot deliver (it drops the key, re-verifies keyless, and fails unsigned-rejected because UpgradeOptions has no unsigned-consent field and upgrade has no --allow-unsigned); and a keyless candidate arrived with no NewSignerIdentity, which the CLI renders as "unsigned". The renderer is unchanged — the invariant it relies on is now upheld at the source, and TestPrintPluginUpgradeResultSignerRendering pins it from the CLI side.
• Two related sites left out of scope, both pre-existing. upgrade.go:262-264 (the keyless guard's unsigned arm) has the identical dead end, and skillsvc/upgrade.go:287-288 is byte-equivalent to the pre-fix keyed code, so the two halves now diverge. Fixing either changes behaviour on a path this PR is not about; happy to take both in a follow-up.
• task docs was run and produced no changes, as expected — there is no CLI or API surface change in this PR.

Generated with Claude Code

[codecov]

Codecov Report

❌ Patch coverage is 97.27273% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.72%. Comparing base (2533388) to head (5b1fc32).
⚠️ Report is 7 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/plugins/pluginsvc/upgrade.go	96.66%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6525      +/-   ##
==========================================
+ Coverage   78.69%   78.72%   +0.03%     
==========================================
  Files         777      777              
  Lines       76797    77213     +416     
==========================================
+ Hits        60432    60785     +353     
- Misses      16360    16423      +63     
  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]

Reviewed fca0e69 against stacked base 9ceccbb. Two blockers:

1. pkg/plugins/pluginsvc/upgrade.go:320-322 classifies both ErrKeylessSigned and ErrUnsigned as SignerChangeBlocked, whose remediation is --allow-signer-change. That override cannot authorize an unsigned candidate: it bypasses the guard but applies the upgrade without AllowUnsigned, so verification still fails unsigned-rejected. Do not describe unsigned as remediable by that flag; report the actual unsigned rejection unless an explicit unsigned-consent path is added.

2. cmd/thv/app/ai_plugin_upgrade.go:169-174 renders every signer-change outcome with no NewSignerIdentity as unsigned. A keyless-signed candidate therefore gets reported as unsigned, although guardKeyedSignerChange intentionally treats it as a key-to-keyless signer transition. Preserve that distinction in the rendered result and cover it with a CLI-output assertion.

The key-aware offline-sync and keyed candidate verification itself looks sound. CI is currently green and is reported separately from this review decision.

[samuv]

Both blockers confirmed against the code and fixed in 8875c6a. Thanks — the unsigned arm was a genuine dead end.

1. Unsigned candidate no longer advertised as remediable

Traced it: with --allow-signer-change, resolveKeyAnchor drops the recorded key and returns the keyless path, verifyOCIInstall sets expected, expectUnsigned = nil, false, and isAllowedUnsigned then returns opts.AllowUnsigned — which applyUpgrade never sets, and which UpgradeOptions has no field for. So the override reliably lands on unsigned-rejected. Confirmed there is no --allow-unsigned on thv ai-plugin upgrade; it exists only on install.

ErrUnsigned now reports Failed / unsigned-rejected and names the route that works (uninstall, then thv ai-plugin install --allow-unsigned).

2. Keyless candidate now carries its identity

Fixed at the source rather than in the renderer. NewSignerIdentity empty ⇒ "unsigned" is a sound inference given the invariant that a blocked outcome names its signer; the bug was that guardKeyedSignerChange produced an outcome violating it. VerifyOCIWithKey can't report an identity — it was asked about a key — so the keyless arm now probes and fills the field. The alternative (a parallel signal on the outcome) would mean widening the shared skills.UpgradeOutcome to work around a value we can just populate correctly.

Falling out of that: a candidate whose keyless signature does not verify is now Failed rather than blocked, on the same principle as #1 — --allow-signer-change re-verifies rather than skipping verification, so it can't get that candidate installed either.

cmd/thv/app/ai_plugin_upgrade.go is therefore unchanged. The CLI-output assertion you asked for is in TestPrintPluginUpgradeResultSignerRendering, pinning all three renderings (keyless names the identity / absent identity means unsigned / unsigned-under-a-pinned-key reports the rejection). It passes against the current renderer by design — it locks the invariant so a regression on either side trips it.

Two related sites I did not touch

Both pre-date this PR; flagging rather than folding in, since each is a behaviour change on a path this PR isn't about. Happy to take them as a follow-up covering both halves:

• pkg/plugins/pluginsvc/upgrade.go:262-264 — the keyless guard's unsigned arm has the identical dead end (SignerChangeBlocked → --allow-signer-change → unsigned-rejected). Fixing it changes the CLI exit code for an existing non-key path.
• pkg/skills/skillsvc/upgrade.go:287-288 — the merged skills keyed guard is byte-equivalent to what I just fixed, so plugins and skills now diverge here. Correct-over-consistent seemed the right way round, but it should be squared up.

task lint-fix clean, task test green under -race, task docs a no-op. #6528 restacked.

[JAORMX]

Fresh review at 8875c6a against stacked base 1e80f9d.

The two prior blockers are resolved: unsigned keyed candidates now fail as unsigned-rejected rather than advertising --allow-signer-change, and a keyless candidate is probed so its identity is available to the renderer.

Blocking — mixed key-pair and keyless signatures receive a remediation that cannot work

VerifyOCIWithKey returns ErrKeylessSigned only when every discovered bundle has a certificate (pkg/skills/verifier/oci.go:75-82, 222-236). For a realistic migration artifact carrying both a valid keyless signature and a stale/different-key signature, it instead returns ErrSignatureInvalid. guardKeyedSignerChange therefore takes the default failure arm (pkg/plugins/pluginsvc/upgrade.go:320-341) and tells the user to reinstall with --public-key.

That result is wrong: --allow-signer-change drops the pinned key and the later normal keyless verification accepts the valid keyless bundle. This is a supported key-to-keyless transition, but the mixed-bundle case is misclassified and its actionable remedy is withheld. After a failed keyed verification, probe the keyless path before assigning the different-key/damaged-signature failure; when that probe succeeds, return SignerChangeBlocked with the observed identity. Add a mixed key-pair plus valid keyless bundle regression case.

CI is green at this head and is separate from the review decision.

[JAORMX]

Review of exact head 6f288f8 against stacked base #6524 (9ceccbb). Prior blockers are still present; this pass also found two additional correctness defects.

1. Mixed valid keyless + stale/different-key signature incorrectly fails instead of exposing the supported key-to-keyless transition. VerifyOCIWithKey returns ErrKeylessSigned only when every bundle is keyless; a mixed artifact returns ErrSignatureInvalid. guardKeyedSignerChange then takes the default failure arm and tells the user to reinstall with --public-key, even though --allow-signer-change drops the pin and the ordinary keyless verifier accepts the valid keyless bundle. On a keyed-verification failure, probe the keyless path before classifying a permanent different-key/damaged-signature error; if it verifies, return SignerChangeBlocked with the observed identity. Add the mixed-bundle regression.

2. --allow-signer-change breaks a same-key upgrade. planUpgrade skips guardSignerChange whenever the flag is set (pkg/plugins/pluginsvc/upgrade.go:210-214), then applyUpgrade forwards the flag to installAlreadyLocked (:443-455). resolveKeyAnchor consequently drops the pinned public key (pkg/plugins/pluginsvc/verify.go:199-215), so a candidate still signed by that very key goes through keyless verification and fails ErrKeySigned. This is reachable when a multi-plugin upgrade needs the override for another plugin, and even an individual same-key upgrade regresses when the flag is supplied. Preserve verification against the recorded key unless keyed verification establishes a keyless transition that the override authorizes; add an upgrade regression that asserts same-key success and retained Provenance.PublicKey with the flag.

3. The default keyed-verification arm classifies operational verifier errors (registry/transport/context failures) as signature-invalid and advises a destructive uninstall/reinstall with a key. Only errors matching ErrSignatureInvalid should receive that classification/remediation; propagate/classify other failures as operational failures.

The pinned-key offline sync and adoption refusal paths look sound. CI is currently in progress and is reported separately; it is not part of this review decision.

[samuv]

All three verified against the code and fixed in aed8c3a.

1. Mixed keyless + stale-key artifact

Confirmed: onlyKeylessSigned (oci.go:285-295) returns false the moment any bundle lacks a certificate, so wrapInvalid(lastErr) wins and the caller sees ErrSignatureInvalid. The keyed guard now probes the keyless path before committing to a diagnosis, and a probe that verifies is reported as SignerChangeBlocked with the observed identity — regardless of whether the keyed error was ErrKeylessSigned or ErrSignatureInvalid. blockKeyToKeylessChange is replaced by classifyKeyedCandidate, which folds both cases into one path. Regression case: "a mixed keyless and stale-key artifact is the supported transition".

2. --allow-signer-change breaking a same-key upgrade

Confirmed empirically before fixing — the install fails with:

plugin "my-plugin": artifact is signed with a cosign key pair, not keylessly; re-run 'thv ai-plugin install --public-key' ...

The framing that made this click is that the override is project-wide but applied per entry, so needing it for one plugin unpins every key-pinned plugin beside it. The override authorizes dropping an anchor the artifact has moved off, not ignoring one it still satisfies. upgradePlan now carries allowSignerChange narrowed per entry: for a key-pinned entry under the flag, candidateMatchesPinnedKey verifies against the pin, and a candidate that still passes keeps it. An undecodable pin reports false, since the override is exactly the escape hatch for an anchor that can no longer be applied.

Regression: TestUpgrade_AllowSignerChangeKeepsSameKeyPin asserts Upgraded and a retained Provenance.PublicKey with the flag set.

This also exposed that TestUpgrade_AllowSignerChangeMovesKeyPinnedEntryToKeyless was self-contradictory — it mocked keyed verification as succeeding while asserting a key-to-keyless move. It now models a candidate that genuinely moved.

3. Operational errors classified as signature-invalid

Fixed: only errors.Is(verifyErr, ErrSignatureInvalid) gets the signature-invalid reason and the uninstall/reinstall remedy. Everything else — registry, transport, context — is FailureReasonUnknown with the wrapped error and no destructive advice. Case "an operational verifier failure is not a signature verdict" asserts the reason and that the message contains no uninstall.

---

Every new expectation was re-run with upgrade.go stashed; all three fail without the change. task lint-fix clean, task test green under -race, task docs a no-op. Stack re-synced with gh stack, #6528 rebased.

Still deliberately out of scope, unchanged from my last reply: the keyless guard's unsigned arm at upgrade.go:262-264, and skillsvc/upgrade.go — which now diverges from plugins on all three of these. Happy to take both halves in a follow-up.

[JAORMX]

Fresh review of exact head aed8c3a against stacked base #6524 (9ceccbb).

All three prior blockers are resolved:

1. A failed pinned-key verification now probes the keyless path, so a valid mixed keyless plus stale-key artifact is reported as an overridable signer transition (pkg/plugins/pluginsvc/upgrade.go:384-413).
2. --allow-signer-change is narrowed per entry: candidates that still verify against their pinned key retain it and use keyed verification (pkg/plugins/pluginsvc/upgrade.go:217-225, 357-381).
3. Only ErrSignatureInvalid gets signature-invalid/reinstall remediation; operational verifier errors remain unknown failures without destructive advice (pkg/plugins/pluginsvc/upgrade.go:427-441).

No blocking or non-blocking findings in the current range. Exact-head CI is green; that status is separate from this approval.

[JAORMX]

Fresh review of exact head aed8c3a against stacked base #6524 as merged (1e80f9d). The prior three blockers are resolved, but two merge blockers remain.

1. pkg/plugins/pluginsvc/upgrade.go:217-225,373-381 makes candidateMatchesPinnedKey return false for every keyed-verifier error. With project-wide --allow-signer-change, a registry/transport/context failure is therefore treated as proof that this entry moved off its pinned key; applyUpgrade receives AllowSignerChange: true (:509-520), drops the key, and may keylessly verify/rewrite the provenance. A candidate carrying both the still-valid pinned-key signature and a valid keyless signature can consequently be re-anchored to keyless merely because the keyed planning probe had a transient failure. Return/propagate an operational error distinctly; only permit the per-entry anchor drop after a conclusive mismatch plus successful keyless verification. Add the regression asserting no install or lock rewrite for an operational keyed-verifier error.

2. Several failure messages offer an incomplete reinstall command: upgrade.go:347-351,430-433 and sync.go:578-582. thv ai-plugin install --public-key and --allow-unsigned require [plugin-name]; more importantly, these recoveries must be project-scoped to create/replace the project lock entry, whereas install defaults to user scope and a user-scope --public-key is rejected. Provide a complete project reinstall sequence (plugin reference/name plus --scope project, and applicable project-root guidance) so the advertised remediation works.

CI is green at this head (including unit, lint, docs, codegen, security, lifecycle, operator, and E2E matrices) and is reported separately from this review decision.

[JAORMX]

Fresh review of exact head 5b1fc32 against the PR-owned fixup from aed8c3a (the base is #6524 as merged; unrelated main commits are not attributed to this PR).

The prior merge blockers are resolved:

1. Keyed candidates are measured against their recorded key even with --allow-signer-change. Operational verifier errors now produce a fail-closed keyedUndecided outcome, so they cannot drop the anchor or rewrite lock provenance (pkg/plugins/pluginsvc/upgrade.go:258-267, 415-453). The regression asserts a deadline failure preserves both pin and digest (upgrade_verify_test.go:931-980).
2. Key-signed-adoption remediation now gives a complete project-scoped install command with the plugin name and project-root guidance (pkg/plugins/pluginsvc/sync.go:578-582).

No blocking or non-blocking findings in the current range. Panel review included an independent security check of the anchor-downgrade path with no findings.

Exact-head CI: all 45 checks are successful. CI status is reported separately from this approval.

[JAORMX]

Fresh review of exact head 5b1fc32 against stacked base #6524 as merged (2533388). The earlier trust-transition blockers remain resolved, but three actionable recovery paths are still incorrect:

1. pkg/plugins/pluginsvc/upgrade.go:523-525 generates project-scoped uninstall/install commands without --project-root, while validateProjectRootForScope unconditionally requires it for --scope project (cmd/thv/app/skill_helpers.go:67-73). The stated conditional is false; the command is rejected even in the project directory. Include --project-root <path> in both commands.

2. pkg/plugins/pluginsvc/upgrade.go:507 and pkg/plugins/pluginsvc/sync.go:581-582 advertise --public-key <path-or-base64>, but the CLI always treats this option as a file path and passes it to os.ReadFile (cmd/thv/app/ai_plugin_install.go:67-72). A base64 value cannot repair the install. Advertise a public-key file path only.

3. pkg/plugins/pluginsvc/sync.go:581-585 tells a rejected adoption to reinstall pl.Metadata.Name, despite pl.Reference being the actual existing source. For a direct OCI reference, the bare name can resolve elsewhere or not at all. Use the original reference and include the mandatory project root.

Please add regression coverage for complete recovery commands, not only their flag substrings. CI is green at this head; that status is separate from this review decision.