Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## samuv/skills-key-reanchor #6663 +/- ##
=============================================================
+ Coverage 79.10% 79.16% +0.05%
=============================================================
Files 786 787 +1
Lines 78848 79203 +355
=============================================================
+ Hits 62375 62699 +324
- Misses 16468 16499 +31
Partials 5 5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
3592c58 to
32904c3
Compare
32904c3 to
f155300
Compare
JAORMX
left a comment
There was a problem hiding this comment.
Reviewed the plugin-only stack against #6662: initially 32904c3 vs 560a05b, then verified the exact trees at f155300 vs d655077. The rebase changes only go.mod/go.sum relative to the initially reviewed plugin head; all implementation blobs are identical.
Spec: the plugin flow follows #6640's old-key-first, lock-first, explicit replacement and offline-adoption contract. No separate spec mismatch found.
Standards: the new projecttxn package exposes implementation-oriented Lock/LockPath helpers; reducing that API or documenting its canonical-root precondition would be worthwhile, but current production callers normalize roots, so this is nonblocking.
Domain: one newly added metadata-refresh rollback path can leave SQLite metadata inconsistent with the lock after cancellation (inline). Shared project transaction locking and the bounded trust-only rollback otherwise follow the intended design.
I excluded inherited SDK issues and concerns outside the documented contract, including crash-atomic journaling and treating offline adoption as authentication of already-materialized filesystem bytes. The keyless fallback for an unrecorded legacy entry is also consistent with the per-entry policy, not a replacement-key bypass.
Nonblocking: document public_key's base64-encoded DER SPKI representation and allowed option combinations in the API fields so generated SDK/OpenAPI docs explain the wire contract.
Core #297 has now been re-reviewed and approved at e5c9df0. The local cancellation issue on skills #6662 still needs its separate fix. All 46 checks passed on the originally reviewed plugin head; the new head's CI was still running at the last check. No local tests or linters were run.
| if opts.RefreshMetadata && !becomesManaged { | ||
| updated := buildInstalledPlugin(opts, scope, clientTypes, existing.Clients) | ||
| updated.Managed = existing.Managed | ||
| if err := s.store.Update(ctx, updated); err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
[P2] Make rollback of the new metadata-only update independent of request cancellation
An allowed reference change at the same digest now persists the refreshed Reference/signature bundle here before the expected-entry lock CAS. If the request is cancelled after this update and that CAS then conflicts (for example, an older process or external writer changed the lock), rollbackInstall tries to restore the previous database row using the cancelled context. SQLite can reject that transaction immediately. The conflict path correctly preserves the competing lock entry, but the failed upgrade can leave its own new metadata in SQLite, inconsistent with that lock.
Managed is preserved here, so this is distinct from the unmanaged-row symptom in the skills variant. Before this change the equal-digest no-op did not rewrite metadata. Please run the compensating store/file operations under a bounded context.WithoutCancel context, as persistTrustOnlyUpgrade already does, retaining rollback errors. Add a cancellation-plus-CAS-conflict regression asserting the old DB record is restored while the competing lock entry is not overwritten.
There was a problem hiding this comment.
Fixed in dbe24af. Plugin rollback now runs store/file compensation under a bounded five-second context detached from request cancellation while still preserving rollback errors. The regression cancels the request and forces the expected-entry CAS conflict, then asserts the old database record is restored and the competing lock entry is left untouched. The full pkg/plugins/pluginsvc suite passes with -race on the restacked head, inheriting toolhive-core v0.0.49 from #6662.
f155300 to
ab54e7c
Compare
ab54e7c to
dbe24af
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>
dbe24af to
bd94161
Compare
Summary
--public-keysupport to plugin upgrade and sync adoption, require--allow-signer-changefor replacement anchors, and retain the recorded key whenever it still verifies.Fixes #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 plugin key-trust E2E suite compiles and reaches setup, where it stops for the same missing runtime.Changes
pkg/projecttxnfor skills and plugins.Does this introduce a user-facing change?
Yes.
thv ai-plugin upgrade --allow-signer-change --public-key <PUBLIC_KEY_PATH>can re-anchor a key-pinned OCI plugin, andthv ai-plugin sync --adopt --public-key <PUBLIC_KEY_PATH>can adopt an installed key-signed plugin.Implementation plan
Approved implementation plan
Special notes for reviewers
This is the second PR in the stack and is based on #6662. The approved PR-size exception keeps plugin parity, shared transaction extraction, tests, and generated outputs in one reviewable change.