fix(skills): stop quarantined skill trust from escalating on relocation - #6712
Merged
Conversation
update_trust_for_reloaded_skills's source_kind-mismatch branch adopted the new source kind's default trust level whenever the stored level was "active" (not Blocked). Since Quarantined is active and [skills.trust] local_level defaults to Trusted, relocating a Quarantined skill's unchanged-content directory out of managed_dir (Hub -> Local) silently promoted it to Trusted on the next reload, with no review step. The branch now takes SkillTrustLevel::min_trust of the stored level and the new source kind's default, so a source_kind change can never move a skill's effective trust higher than it already had. Symmetrically, moving a Trusted skill into a source kind with a stricter default now correctly demotes it too - an intentional fail-closed side effect of the same fix. The adjacent hash-mismatch branch had the identical defect one line above: a hash mismatch unconditionally assigned hash_mismatch_level (default Quarantined) even when the existing row was an explicit operator Blocked, silently un-blocking it on a one-byte edit. Also clamped through min_trust. skill_catalog_items (feeds the TUI mention picker) filtered out only Blocked skills, so a Quarantined skill still appeared there as a normal, invocable name. New SkillTrustLevel::is_hidden_from_catalog closes this - SkillCatalogItem carries no trust field, so hiding is the only option on this surface. The other two catalog sites originally in scope for this fix turned out to already be correctly handled by the concurrently-merged #6710/#6701 fix, which takes an annotate-not-hide strategy instead (format_skills_catalog's trust_levels parameter renders a trust="..." attribute, and #6710's own activation filter already excludes Quarantined/Blocked from matching); this change does not alter that design. Closes #6707 Closes #6706
bug-ops
enabled auto-merge (squash)
July 28, 2026 20:04
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
update_trust_for_reloaded_skills'ssource_kind-mismatch branch previously adopted the new source kind's default trust level whenever the stored level was "active" (notBlocked). SinceQuarantinedis active and[skills.trust] local_leveldefaults toTrusted, relocating aQuarantinedskill's unchanged-content directory out ofmanaged_dir(Hub -> Local reclassification) silently promoted it toTrustedon the next reload, with no review step. The branch now takesSkillTrustLevel::min_trustof the stored level and the new source kind's default, so asource_kindchange can never move a skill's effective trust higher than it already had.hash_mismatch_level(defaultQuarantined), even when the existing row was an explicit operatorBlocked— silently un-blocking it on a one-byte content edit. Also clamped throughmin_trust.skill_catalog_items(feeds the TUI mention picker) filtered out onlyBlockedskills, so aQuarantinedskill still appeared there as a normal, invocable name, indistinguishable from a reviewed skill. NewSkillTrustLevel::is_hidden_from_catalogcloses this —SkillCatalogItemcarries no trust field, so hiding is the only option on this surface.Note on scope vs #6710
This branch's base went stale mid-session: PR #6710 (closing #6701) merged concurrently and made the canonical design decision for the other two catalog sites originally in scope for #6706 (
reload_skills's system-prompt catalog andapply_skill_trust_and_gating's per-turn catalog) — it takes an annotate, not hide, strategy there:Quarantined/Blockedskills stay visible with atrust="..."XML attribute (format_skills_catalog'strust_levelsparameter) so the model/operator can still name and promote them, while #6710's own activation filter already excludes them fromactive_skill_names/matching. This PR was rebased onto that decision and does not alter it — the mention-picker exclusion above is the only remaining hide-site, and closes the gap #6710 didn't touch.Test plan
cargo +nightly fmt --checkcargo clippy --profile ci --workspace --all-targets --features "desktop,ide,server,chat,pdf,scheduler,testing" -- -D warningscargo nextest run --config-file .github/nextest.toml -p zeph-core -p zeph-common --lib --bins(also verified full workspace suite green during review, 2553 passed / 0 failed)cargo test --doc -p zeph-common(81 passed, incl. newis_hidden_from_catalogdoctest)RUSTDOCFLAGS="--deny rustdoc::broken_intra_doc_links" cargo doc --no-deps --all-features -p zeph-commongitleaks protect --stagedupdate_trust_hash_mismatch_never_unblocks_explicit_block) manually verified load-bearing by reverting the fix and confirming the test fails, then restoring itCloses #6707
Closes #6706