Fail closed on keyed skill upgrade verdicts

[samuv]

Summary

thv skill upgrade and thv ai-plugin upgrade are meant to be the same operation over two lock files, and the plugin side went through three review rounds on #6525 that fixed how a key-pinned entry is judged. None of those fixes reached pkg/skills/skillsvc/upgrade.go, so thv skill upgrade still has every one of the bugs the review found:

• The override drops a valid pin. When --allow-signer-change is passed, the signer guard is skipped entirely (planUpgrade gated it on !opts.AllowSignerChange). The project-wide flag then unpins every key-pinned skill, including ones whose candidate still verifies against the pinned key, and a transient verifier fault during planning is enough to re-anchor an artifact that still carries a good signature by that key. This is the fail-open the Honour a pinned cosign key on plugin sync and upgrade #6525 review called critical.
• An unsigned candidate is a dead end. The keyed guard reported it as signer-change-blocked, so the CLI told the user to pass --allow-signer-change. Upgrade has no unsigned-consent flag and that override is not one: it re-verifies from scratch, which an unsigned artifact still fails.
• A keyless move can be misdiagnosed. VerifyOCIWithKey returns ErrKeylessSigned only when every bundle is keyless, so an artifact mid-migration with a stale key-pair bundle beside a valid keyless one came back as ErrSignatureInvalid and was reported as a different key.
• The reinstall hint was not runnable. It named --public-key without the command, the skill argument, or --scope project, and thv skill install defaults to user scope where --public-key is rejected.

This PR mirrors the plugin verdict design into the skills service, function for function, so both halves are behaviourally identical and reviewable against code already approved:

• judgeKeyedCandidate measures the candidate against the pin once and returns one of three verdicts. Only a conclusive keyed mismatch (ErrKeylessSigned or ErrSignatureInvalid) followed by a keyless signature that verifies is a genuine move. An operational error stops before the keyless probe and is never read as evidence about the key.
• recordKeyedVerdict is the only mode-dependent step: a genuine move blocks without the override and proceeds with it. Undecided candidates fail in both modes, so the override can never turn a failed measurement into permission to re-anchor.
• resolveSignerPolicy narrows the project-wide --allow-signer-change to the entry it applies to, and applyUpgrade passes that narrowed flag to the install. A skill that needs the override no longer unpins the key-pinned skills beside it.
• Unsigned under a pinned key is an unsigned-rejected failure; the mixed-artifact case is a move; different key names the runnable thv skill uninstall … --scope project then thv skill install … --scope project --public-key <path> sequence.

The keyless guard's own unsigned arm (guardSignerChange, still signer-change-blocked for an unsigned candidate) has the same dead-end shape on both services and is deliberately left for #6629, which stacks on this one and needs projectReinstallCommand from here.

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)
• Generated API and CLI documentation (task docs) — no CLI or API surface changed

Coverage mirrors the plugin side: TestJudgeKeyedCandidate (seven verdict cases, each run through recordKeyedVerdict in both modes, asserting the probe is not consulted on an operational fault), TestJudgeKeyedCandidate_UndecodablePinnedKey, and four integration tests over a real install → upgrade against a real OCI store: pin verified on both the guard and the install, genuine key-to-keyless move re-anchors, same-key candidate keeps its pin under the override, and an operational fault under the override leaves the lock byte-identical. TestPrintUpgradeResultSignerRendering pins the three CLI renderings the design depends on.

Manual mutation check: reverting applyUpgrade to pass opts.AllowSignerChange instead of the narrowed flag makes TestUpgrade_AllowSignerChangeKeepsSameKeyPin fail, so the regression coverage is live rather than incidental.

Changes

File	Change
pkg/skills/skillsvc/upgrade.go	Verdict machinery (judgeKeyedCandidate, recordKeyedVerdict, resolveSignerPolicy, conclusiveKeyedMismatch, keyedFailureReason, keyedFailureMessage, projectReinstallCommand); upgradePlan.allowSignerChange; guardKeyedSignerChange removed
pkg/skills/skillsvc/upgrade_verify_test.go	TestGuardKeyedSignerChange replaced by TestJudgeKeyedCandidate; keyPinnedUpgradeFixture and four end-to-end keyed upgrade tests
cmd/thv/app/skill_upgrade_test.go	TestPrintUpgradeResultSignerRendering
docs/arch/12-skills-system.md	Keyed upgrade bullet rewritten to describe the verdict semantics and the per-entry override

Does this introduce a user-facing change?

Yes, for thv skill upgrade on key-pinned entries:

• --allow-signer-change no longer drops a key pin whose candidate still verifies against it, and no longer drops one on a verifier fault. Previously both silently re-anchored to keyless.
• An unsigned candidate under a pinned key is now failed [unsigned-rejected] with a runnable reinstall command, instead of signer change blocked pointing at a flag that cannot resolve it.
• A candidate signed by a different key prints the exact uninstall/reinstall sequence.

Special notes for reviewers

• This is a deliberate mirror of pkg/plugins/pluginsvc/upgrade.go after Honour a pinned cosign key on plugin sync and upgrade #6525, not a redesign. Reading the two files side by side should show only the skills/plugins type prefixes, the newRef, newDigest vs resolvedLatest parameter shape, and the thv skill vs thv ai-plugin command names differing.
• The shared-abstraction question (one implementation both services call) is real but intentionally deferred: mirroring first gets the two halves behaviourally identical against reviewed code, and an extraction is a cleaner follow-up once there is no behavioural diff left to reason about.
• guardSignerChange's keyless unsigned arm is unchanged here on purpose and is Reject unsigned upgrade candidates in both modes #6629. Writing that PR's override-mode test surfaced that on main thv skill upgrade --allow-signer-change silently converts a signer-pinned skill to unsigned: true when the candidate is unsigned; see Reject unsigned upgrade candidates in both modes #6629 for the mechanism.

🤖 Generated with Claude Code

[codecov]

Codecov Report

❌ Patch coverage is 96.25000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.90%. Comparing base (cfdd650) to head (217bc7d).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/skills/skillsvc/upgrade.go	96.25%	3 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #6628   +/-   ##
=======================================
  Coverage   78.89%   78.90%           
=======================================
  Files         778      778           
  Lines       77598    77657   +59     
=======================================
+ Hits        61223    61274   +51     
- Misses      16370    16378    +8     
  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]

Panel review completed against 217bc7dacc0336c377f2cec0b129a7b7f64545c1.

No merge blockers found. The keyed-candidate verdict separates conclusive key migration from unsigned, invalid, and operational-failure cases; --allow-signer-change permits only the verified key-to-keyless transition. The narrowed per-entry override preserves unrelated pins, and the regression coverage exercises the relevant verification and lock-preservation paths.

Exact-head CI is green.