Reject unsigned upgrade candidates in both modes - #6629
Conversation
96bc6df to
ac8994f
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6629 +/- ##
==========================================
- Coverage 78.94% 78.90% -0.05%
==========================================
Files 781 781
Lines 77784 77800 +16
==========================================
- Hits 61410 61387 -23
- Misses 16369 16408 +39
Partials 5 5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
JAORMX
left a comment
There was a problem hiding this comment.
Panel review completed against ac8994fa9b74a7fcc8926ccea313da6a3d258e93.
No merge blockers found. The guard now remains active under --allow-signer-change, while only the actual signer/provenance-change arm is waived. An unsigned candidate consequently remains rejected without mutating the recorded signer state. The paired skills install-time gate supplies appropriate defence in depth, and coverage includes both override modes plus already-unsigned entries.
Exact-head CI is green.
Pull Request is not mergeable
ac8994f to
2b48bc0
Compare
The keyless signer guard reported an unsigned candidate as a signer change, so the CLI pointed at --allow-signer-change. That flag cannot resolve it: upgrade has no unsigned-consent flag, and the override re-verifies from scratch, which an unsigned artifact fails just the same. On plugins that was a dead end with a misleading remedy. On skills it was worse: the override cleared the lock expectation, and isAllowedUnsigned granted any lock-driven install with no expectation, so `thv skill upgrade --allow-signer-change` silently turned a signer-pinned skill into an unsigned entry with no --allow-unsigned ever given. Run the guard in both modes and waive only the identity-change arm under the override; unsigned and unverifiable candidates fail either way, naming the project-scoped reinstall that records the exception. Narrow the override to entries that record a signer, since an anchorless entry has nothing to change, and stop the skills install grant from applying when the override cleared the expectation. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Samuele Verzi <samu@stacklok.com>
2b48bc0 to
8bc9582
Compare
Summary
Stacked on #6628. Closes the last divergence between
thv skill upgradeandthv ai-plugin upgradeon how a signer-pinned entry treats a candidate that has lost its signature, and in doing so fixes a real hole on the skills side.Why. The keyless signer guard on both services reported an unsigned candidate as
signer-change-blocked, so the CLI told the user to pass--allow-signer-change. That flag cannot resolve it: upgrade has no unsigned-consent flag, and the override re-verifies from scratch, which an unsigned artifact fails just the same. Writing the test for the override mode showed the two services then diverge:thv ai-plugin sync --allow-unsigned. Both claims are wrong for an entry that records a signer. A dead end with a misleading remedy.isAllowedUnsignedgrants an unsigned artifact to any lock-driven install with no expectation. Upgrade's own install is lock-driven. Sothv skill upgrade --allow-signer-changeagainst an unsigned candidate succeeded, dropped the recorded signer identity, and recorded the entry asunsigned: true, with no--allow-unsignedever given. A project-wide flag meant for "the publisher changed identity" silently converted a signer-pinned skill into an unsigned exception.What changed, mirrored on both services:
guardSignerChangenow runs in both modes and takes the override as a parameter. Only the identity-or-provenance-field-changed arm is waived by it; an unsigned candidate isfailed [unsigned-rejected]either way, and an unverifiable one fails either way. The unsigned failure names the runnableuninstall … --scope projecttheninstall … --scope project --allow-unsignedsequence, reusingprojectReinstallCommandfrom Fail closed on keyed skill upgrade verdicts #6628.resolveSignerPolicynarrows the override tofalsefor an entry with no recorded signer. There is no signer change to authorize on such an entry, and forwarding the flag only clearedexpectUnsignedfor an entry the lock already accepts as unsigned.isAllowedUnsignedno longer honours the lock-driven grant whenAllowSignerChangeis set. A nil expectation under the override is not evidence of what the entry recorded. This is defence in depth behind the plan-time guard, since the guard is the only caller that sets the flag today.Type of change
Test plan
task test)task test-e2e)task lint-fix)task docs) — no CLI or API surface changedTestUpgrade_UnsignedCandidateRejectedAgainstSignedEntryon both services now runs in both modes and assertsfailed,unsigned-rejected, the reinstall remedy, and an unchanged lock withUnsignedstill false. Before the fix this test failed in override mode on both sides with exactly the two behaviours described above (skills:upgradedwithProvenancenil; plugins: the misleading install-time message).TestUpgrade_UnsignedEntryUpgradesUnderSignerChangeOverrideon both services proves anunsigned: trueentry still upgrades under the project-wide override with no probe.TestIsAllowedUnsignedNotGrantedUnderSignerChangeOverridepins the skills install-time gate.Changes
pkg/skills/skillsvc/upgrade.go,pkg/plugins/pluginsvc/upgrade.gopkg/skills/skillsvc/verify.goisAllowedUnsignedlock-driven grant gated on!AllowSignerChange*_test.goon both servicesdocs/arch/12-skills-system.md,docs/arch/14-plugins-system.md--allow-signer-changecovers an unsigned candidateDoes this introduce a user-facing change?
Yes.
thv skill upgrade --allow-signer-changeno longer converts a signer-pinned skill into an unsigned entry when the candidate is unsigned. It fails withunsigned-rejectedand the reinstall command. Anyone relying on the old behaviour was relying on the override doubling as--allow-unsigned, which it was never documented to do.thv skill upgradeandthv ai-plugin upgrade, an unsigned candidate under a signer-pinned entry reportsfailed [unsigned-rejected]with a runnable remedy instead ofsigner change blockedpointing at a flag that cannot resolve it.Special notes for reviewers
maintoday. It needs a project-scoped skill with a recorded signer, a newer unsigned artifact at the same source, and--allow-signer-change. That is a plausible sequence for someone told by the previous message to pass that flag.resolveSignerPolicynarrowing for anchorless entries is what makes the skills install-time gate safe to add: a pre-verification orunsigned: trueentry never seesAllowSignerChangeat install, so the lock-driven grant still applies to it as before.🤖 Generated with Claude Code