Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6662 +/- ##
==========================================
+ Coverage 79.14% 79.20% +0.05%
==========================================
Files 789 790 +1
Lines 79021 79411 +390
==========================================
+ Hits 62545 62894 +349
- Misses 16471 16512 +41
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.
Reviewed 560a05b, with separate passes over trust decisions/adoption, project transactions and rollback, CLI/API/SDK integration, standards, and reuse.
Spec: the local re-anchor flow follows #6640: old-key-first, lock-first policy selection, explicit replacement authorization, shared strict evidence, and offline adoption against the installed digest. No separate spec mismatch found.
Standards: no separate merge-blocking convention findings.
Domain: one cancellation/rollback correctness issue in the newly added same-digest metadata refresh path needs fixing (inline). A candidate finding about transitive entries becoming explicit was compared with the base and discarded as pre-existing.
Dependency: strict evidence safety remains blocked by the two findings posted on stacklok/toolhive-core#297; please consume the corrected core revision before merging this integration.
Nonblocking API documentation: please describe public_key as base64-encoded DER SPKI in the sync/upgrade request fields, including adopt/check and allow_signer_change constraints, so generated SDK/OpenAPI consumers do not have to infer the wire format from internal options.
All 46 current CI checks passed. No local tests or linters were run.
| updated := buildInstalledSkill(opts, scope, clientTypes, existing.Clients) | ||
| if err := s.store.Update(ctx, updated); err != nil { |
There was a problem hiding this comment.
[P2] Preserve managed state and make metadata-refresh compensation cancellation-safe
This new same-digest/reference-change path builds a fresh record without copying existing.Managed, so the first store update writes Managed=false. If cancellation lands after that update and the lock CAS but before recordLockState writes Managed=true, the latter update fails. rollbackInstall then tries to restore the previous row using the same cancelled context; the SQLite store cannot start that transaction, while the context-free lock rollback can still restore the old pin. The failed upgrade leaves an unmanaged installed row paired with a live lock entry. A subsequent uninstall skips lock removal (existing.Managed is false), so sync can reinstall the supposedly removed skill.
Before this PR, equal digests returned UpToDate without this metadata mutation. Please preserve the existing managed state in both this branch and applyGitInstallExisting, and run rollback compensation with a bounded cancellation-detached context, as the trust-only rollback already does. A cancellation regression should assert the failed refresh restores both row and lock, then verify uninstall removes the lock entry.
There was a problem hiding this comment.
Fixed in 2a73313 and updated again in 987021e for the released core dependency. The same-digest and Git refresh paths now preserve Managed, and rollback uses a bounded five-second context detached from request cancellation. The regression cancels after the metadata write and lock CAS, verifies both the prior database row and lock entry are restored, then uninstalls and confirms the managed lock entry is removed. The full pkg/skills/skillsvc suite passes with -race against toolhive-core v0.0.49.
JAORMX
left a comment
There was a problem hiding this comment.
Checked the update at d655077. The exact tree delta from 560a05b is only go.mod/go.sum, moving to core e5c9df0. I re-reviewed and approved stacklok/toolhive-core#297, so the upstream completeness concerns in my previous review are cleared.
The local metadata-refresh cancellation/rollback finding remains unchanged: the implementation blobs are identical to the reviewed head. My request-changes disposition therefore remains for that P2 only; the public_key wire-format documentation suggestion is nonblocking. No local tests or linters were run.
987021e to
db62269
Compare
Signed-off-by: Samuele Verzi <samu@stacklok.com>
Signed-off-by: Samuele Verzi <samu@stacklok.com>
Signed-off-by: Samuele Verzi <samu@stacklok.com>
Signed-off-by: Samuele Verzi <samu@stacklok.com>
Signed-off-by: Samuele Verzi <samu@stacklok.com>
Signed-off-by: Samuele Verzi <samu@stacklok.com>
Signed-off-by: Samuele Verzi <samu@stacklok.com>
db62269 to
95adeb3
Compare
Summary
--public-keysupport to skill upgrade and sync adoption, require--allow-signer-changefor replacement anchors, and try the recorded key before a proposed replacement.Part of #6640
Type of change
Test plan
task test)task test-e2e)task lint-fix)task gen,task docs, andtask buildpass. Changed-package unit tests pass. The fulltask testrun reaches five unrelatedpkg/apifailures because no container runtime is available. The focused key-trust E2E suite compiles and reaches setup, where it stops for the same missing runtime.Changes
Does this introduce a user-facing change?
Yes.
thv skill upgrade --allow-signer-change --public-key <PUBLIC_KEY_PATH>can re-anchor a key-pinned OCI skill, andthv skill sync --adopt --public-key <PUBLIC_KEY_PATH>can adopt an installed key-signed skill.Implementation plan
Approved implementation plan
Special notes for reviewers
This is the first PR in a two-PR stack. The plugin parity change follows in #6663. Strict bundle retrieval depends on stacklok/toolhive-core#297. The approved PR-size exception keeps the complete skills workflow and its generated outputs together while splitting plugin parity into the second PR.