From 73ed2ed671d9f85d1ac47347b4b8e3aeba284d16 Mon Sep 17 00:00:00 2001 From: "Andrei G." Date: Tue, 28 Jul 2026 22:00:40 +0200 Subject: [PATCH] fix(skills): stop quarantined skill trust from escalating on relocation 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 --- CHANGELOG.md | 29 ++ crates/zeph-common/src/trust_level.rs | 36 +++ crates/zeph-core/src/agent/skill_reload.rs | 358 +++++++++++++++++++-- 3 files changed, 405 insertions(+), 18 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7f350170f..7c795be53 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -208,6 +208,35 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). (`parent_effective_trust_level`), which previously duplicated the same logic. - RC-4 (AutoSkill draft-name collisions with native tool IDs) is tracked separately in #6702 and intentionally out of scope here. +- `zeph-core`, `zeph-common`: closed the remaining trust-quarantine gaps left open by #6702's + own fix, split out as #6707 and #6706. (1) `update_trust_for_reloaded_skills`'s + `source_kind`-mismatch branch (e.g. a skill's directory relocated from inside `managed_dir` + to outside it, flipping its classified kind from `Hub` to `Local`) previously 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`, moving a + `Quarantined` skill's unchanged-content directory out of `managed_dir` 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 to something higher than it + already had — symmetrically, relocating 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-mismatched skill was 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 — now also `min_trust`-clamped against the stored level. + (2) `skill_catalog_items` (feeds `Channel::send_skill_catalog` / the TUI mention picker) + filtered out only `Blocked` skills, so a `Quarantined` skill still appeared there as a + normal, invocable skill name; new `SkillTrustLevel::is_hidden_from_catalog` (`zeph-common`, + next to `is_active`/`min_trust`) closes this — `SkillCatalogItem` carries no trust field, so + hiding is the only option on this surface. #6706's other two originally-identified sites + (`reload_skills`'s system-prompt catalog listing and `apply_skill_trust_and_gating`'s + per-turn `remaining_skills` catalog filter) turned out to already be correctly handled by the + concurrently-merged #6710/#6701 fix, which takes the *annotate* strategy instead of *hide* at + both: `Quarantined`/`Blocked` skills stay visible in the XML catalog with a `trust="..."` + attribute (`format_skills_catalog`'s `trust_levels` parameter) so the model/operator can still + name and promote them, while #6710's own `filter_active_skills_by_trust` (RC-1) already + excludes them from `active_skill_names`/matching. This PR does not alter that design — the + mention-picker exclusion above is the only remaining hide-site. - `crates/zeph-core`/`src/serve/agent_factory.rs`: `cargo test --features full` could crash with a genuine stack overflow (`SIGABRT`) on diff --git a/crates/zeph-common/src/trust_level.rs b/crates/zeph-common/src/trust_level.rs index 7f0401224..a95184f93 100644 --- a/crates/zeph-common/src/trust_level.rs +++ b/crates/zeph-common/src/trust_level.rs @@ -152,6 +152,34 @@ impl SkillTrustLevel { pub const fn is_active(self) -> bool { !matches!(self, Self::Blocked) } + + /// Returns `true` for trust levels that must never appear on a listing surface with no + /// trust-annotation mechanism — [`Blocked`](Self::Blocked) (explicitly disabled) and + /// [`Quarantined`](Self::Quarantined) (unreviewed / hash-mismatched) alike. + /// + /// This is the *hide* strategy for surfaces that cannot show a trust level inline, e.g. + /// the mention-picker catalog (`SkillCatalogItem` carries only `name`/`description`, no + /// trust field). Contrast with the *annotate* strategy used by the XML skill-prompt + /// catalog (`zeph_skills::prompt::format_skills_catalog`'s `trust_levels` parameter), + /// which keeps a `Quarantined`/`Blocked` skill visible with a `trust="..."` attribute so + /// the model/operator can still name it and promote it — that surface intentionally does + /// *not* use this method. This method says nothing about matching-candidate selection or + /// per-turn dispatch gating (`TurnTrustFloor`, weakest-link trust fold) either — those are + /// separate concerns with their own filtering logic. + /// + /// # Examples + /// + /// ```rust + /// use zeph_common::SkillTrustLevel; + /// + /// assert!(SkillTrustLevel::Blocked.is_hidden_from_catalog()); + /// assert!(SkillTrustLevel::Quarantined.is_hidden_from_catalog()); + /// assert!(!SkillTrustLevel::Trusted.is_hidden_from_catalog()); + /// ``` + #[must_use] + pub const fn is_hidden_from_catalog(self) -> bool { + matches!(self, Self::Blocked | Self::Quarantined) + } } impl FromStr for SkillTrustLevel { @@ -212,6 +240,14 @@ mod tests { assert!(!SkillTrustLevel::Blocked.is_active()); } + #[test] + fn is_hidden_from_catalog_excludes_blocked_and_quarantined_only() { + assert!(SkillTrustLevel::Blocked.is_hidden_from_catalog()); + assert!(SkillTrustLevel::Quarantined.is_hidden_from_catalog()); + assert!(!SkillTrustLevel::Trusted.is_hidden_from_catalog()); + assert!(!SkillTrustLevel::Verified.is_hidden_from_catalog()); + } + #[test] fn default_is_quarantined() { assert_eq!(SkillTrustLevel::default(), SkillTrustLevel::Quarantined); diff --git a/crates/zeph-core/src/agent/skill_reload.rs b/crates/zeph-core/src/agent/skill_reload.rs index 42ae88641..a9df26f53 100644 --- a/crates/zeph-core/src/agent/skill_reload.rs +++ b/crates/zeph-core/src/agent/skill_reload.rs @@ -20,12 +20,12 @@ use zeph_tools::registry::ToolDef; impl Agent { /// Builds the current skill catalog (name + description) for - /// [`Channel::send_skill_catalog`], excluding blocked skills. + /// [`Channel::send_skill_catalog`], excluding blocked and quarantined skills. /// /// Shared by the startup emit (`Agent::run`) and the hot-reload emit below so both - /// apply the same [`zeph_common::SkillTrustLevel::Blocked`] filter — reading - /// `registry.read().all_meta()` raw at only one of the two sites would let blocked - /// skills appear in the mention picker until the first hot-reload, then silently + /// apply the same [`zeph_common::SkillTrustLevel::is_hidden_from_catalog`] filter — + /// reading `registry.read().all_meta()` raw at only one of the two sites would let + /// hidden skills appear in the mention picker until the first hot-reload, then silently /// vanish (M2). Also runs [`Self::warn_on_tool_id_collisions`] here for the same reason: /// it is the one call site that fires on both startup and every hot-reload, independent /// of trust-DB/`memory` availability (#6702 S4). @@ -49,10 +49,9 @@ impl Agent { all_meta .into_iter() .filter(|m| { - !matches!( - trust_map.get(&m.name), - Some(snap) if snap.trust_level == zeph_common::SkillTrustLevel::Blocked - ) + !trust_map + .get(&m.name) + .is_some_and(|snap| snap.trust_level.is_hidden_from_catalog()) }) .map(|m| crate::channel::SkillCatalogItem { name: m.name, @@ -143,17 +142,21 @@ impl Agent { .flatten(); let trust_level = if let Some(ref row) = existing { if row.blake3_hash != current_hash { - trust_cfg.hash_mismatch_level + // A hash mismatch must never un-block an explicit operator Blocked + // decision either: take the more restrictive of the stored level and + // `hash_mismatch_level` (default Quarantined, severity 2). Without this, + // a single-byte edit to a Blocked skill (severity 3) would silently + // downgrade it to Quarantined (is_active() flips true, matchable again) — + // same defect class as the source_kind branch below (#6707). + row.trust_level.min_trust(trust_cfg.hash_mismatch_level) } else if row.source_kind != source_kind { - // source_kind changed (e.g., hub → bundled on upgrade). - // Never override an explicit operator block. For active trust levels, - // adopt the source-kind initial level when it grants more trust. - let stored = row.trust_level; - if !stored.is_active() || stored.severity() <= initial_level.severity() { - stored - } else { - *initial_level - } + // source_kind changed (e.g., hub -> local on relocation) but content is + // unchanged (hash still matches). A relocation must never silently + // escalate trust: take the more restrictive (higher-severity) of the + // stored level and the new source kind's default. Otherwise a Quarantined + // skill moved out of `managed_dir` (Hub -> Local, whose default is + // Trusted) would be promoted straight to Trusted with no review (#6707). + row.trust_level.min_trust(*initial_level) } else { row.trust_level } @@ -528,6 +531,277 @@ mod tests { ); } + /// Regression test for #6707: relocating a `Quarantined` skill's directory out of + /// `managed_dir` (content unchanged, hash still matches) flips its classified + /// `source_kind` from `Hub` to `Local`. Since `[skills.trust] local_level` defaults to + /// `Trusted`, the source_kind-mismatch branch must not silently promote the skill — + /// the stored `Quarantined` level must survive. + #[tokio::test] + async fn update_trust_source_kind_change_never_escalates_active_trust() { + let managed = tempfile::tempdir().unwrap(); + let relocated = tempfile::tempdir().unwrap(); + let skill_dir = relocated.path().join("my-skill"); + tokio::fs::create_dir_all(&skill_dir).await.unwrap(); + let skill_md = "---\nname: my-skill\ndescription: A test skill.\nversion: 0\nsource: trace_extraction\n---\n\n## How to use\nDo the thing.\n"; + tokio::fs::write(skill_dir.join("SKILL.md"), skill_md) + .await + .unwrap(); + let hash = zeph_skills::compute_skill_hash(&skill_dir).unwrap(); + + let memory = test_memory().await; + memory + .sqlite() + .upsert_skill_trust( + "my-skill", + SkillTrustLevel::Quarantined, + SourceKind::Hub, + None, + // Original (pre-relocation) path, still inside managed_dir. + managed.path().join("my-skill").to_str(), + &hash, + ) + .await + .unwrap(); + + let mut agent = + agent_with_memory_and_executor(memory.clone(), MockToolExecutor::no_tools()); + agent.services.skill.trust_config.local_level = SkillTrustLevel::Trusted; + agent.services.skill.managed_dir = Some(managed.path().to_path_buf()); + + let meta = zeph_skills::loader::SkillMeta { + name: "my-skill".into(), + description: "A test skill.".into(), + version: 0, + source: "trace_extraction".into(), + // Relocated outside managed_dir -> classify_source_kind now returns `Local`. + skill_dir: skill_dir.clone(), + ..Default::default() + }; + agent.update_trust_for_reloaded_skills(&[meta]).await; + + let row = memory + .sqlite() + .load_skill_trust("my-skill") + .await + .unwrap() + .unwrap(); + assert_eq!( + row.trust_level, + SkillTrustLevel::Quarantined, + "relocating a Quarantined skill out of managed_dir must not promote it to the \ + new source kind's (Trusted) default" + ); + assert_eq!( + row.source_kind, + SourceKind::Local, + "source_kind must still be updated to reflect the new location" + ); + } + + /// Regression test for #6707 (S4): a hash mismatch must never un-block an explicit + /// operator `Blocked` decision either — same defect class as the source_kind-mismatch + /// branch above, one `if` earlier in `update_trust_for_reloaded_skills`. Before the fix, + /// any hash mismatch unconditionally assigned `hash_mismatch_level` (default + /// `Quarantined`, severity 2), silently downgrading a `Blocked` (severity 3) skill and + /// making it `is_active()` again on a single-byte content edit. + #[tokio::test] + async fn update_trust_hash_mismatch_never_unblocks_explicit_block() { + let managed = tempfile::tempdir().unwrap(); + let skill_dir = managed.path().join("my-skill"); + tokio::fs::create_dir_all(&skill_dir).await.unwrap(); + let skill_md = "---\nname: my-skill\ndescription: A test skill.\nversion: 0\nsource: trace_extraction\n---\n\n## How to use\nDo the thing.\n"; + tokio::fs::write(skill_dir.join("SKILL.md"), skill_md) + .await + .unwrap(); + + let memory = test_memory().await; + memory + .sqlite() + .upsert_skill_trust( + "my-skill", + SkillTrustLevel::Blocked, + // Same source_kind the reload will compute (skill_dir is inside managed_dir, + // no .bundled marker), so only the hash-mismatch branch fires, not the + // source_kind-mismatch one — isolates the behavior under test. + SourceKind::Hub, + None, + skill_dir.to_str(), + // Deliberately stale hash, unrelated to the real file content below, so the + // freshly computed hash never matches and the hash-mismatch branch fires. + "stale-hash-does-not-match-actual-content", + ) + .await + .unwrap(); + + let mut agent = + agent_with_memory_and_executor(memory.clone(), MockToolExecutor::no_tools()); + agent.services.skill.trust_config.hash_mismatch_level = SkillTrustLevel::Quarantined; + agent.services.skill.managed_dir = Some(managed.path().to_path_buf()); + + let meta = zeph_skills::loader::SkillMeta { + name: "my-skill".into(), + description: "A test skill.".into(), + version: 0, + source: "trace_extraction".into(), + skill_dir: skill_dir.clone(), + ..Default::default() + }; + agent.update_trust_for_reloaded_skills(&[meta]).await; + + let row = memory + .sqlite() + .load_skill_trust("my-skill") + .await + .unwrap() + .unwrap(); + assert_eq!( + row.trust_level, + SkillTrustLevel::Blocked, + "a hash mismatch on an explicitly Blocked skill must not demote it to \ + hash_mismatch_level (Quarantined) — the operator block must survive" + ); + } + + /// Reverse-direction sibling of `update_trust_source_kind_change_never_escalates_active_trust`: + /// proves the `row.trust_level.min_trust(*initial_level)` call site passes its arguments + /// in a way that correctly picks the *worse* (higher-severity) level when the new source + /// kind's default is stricter than the stored level — not just that `min_trust` itself + /// works (that's covered by `zeph-common`'s own unit tests), but that this specific call + /// site wires it with the right operands in both directions. + #[tokio::test] + async fn update_trust_source_kind_change_demotes_to_worse_new_default() { + let managed = tempfile::tempdir().unwrap(); + let skill_dir = managed.path().join("my-skill"); + tokio::fs::create_dir_all(&skill_dir).await.unwrap(); + let skill_md = "---\nname: my-skill\ndescription: A test skill.\nversion: 0\nsource: trace_extraction\n---\n\n## How to use\nDo the thing.\n"; + tokio::fs::write(skill_dir.join("SKILL.md"), skill_md) + .await + .unwrap(); + let hash = zeph_skills::compute_skill_hash(&skill_dir).unwrap(); + + let memory = test_memory().await; + memory + .sqlite() + .upsert_skill_trust( + "my-skill", + SkillTrustLevel::Trusted, + SourceKind::Local, + None, + skill_dir.to_str(), + &hash, + ) + .await + .unwrap(); + + let mut agent = + agent_with_memory_and_executor(memory.clone(), MockToolExecutor::no_tools()); + // An extreme, unambiguous target level (Blocked, severity 3) rules out any + // coincidental pass — this specifically regression-locks against the old "adopt + // initial_level for any active stored level" logic being reintroduced without a + // severity comparison (`min_trust` itself is commutative, so this test's value is + // in exercising the real call site end-to-end, not in catching an argument swap). + agent.services.skill.trust_config.default_level = SkillTrustLevel::Blocked; + agent.services.skill.managed_dir = Some(managed.path().to_path_buf()); + + let meta = zeph_skills::loader::SkillMeta { + name: "my-skill".into(), + description: "A test skill.".into(), + version: 0, + source: "trace_extraction".into(), + // Still inside managed_dir but with no `.bundled` marker -> classify_source_kind + // returns Hub (a genuine source_kind change from the stored Local). + skill_dir: skill_dir.clone(), + ..Default::default() + }; + agent.update_trust_for_reloaded_skills(&[meta]).await; + + let row = memory + .sqlite() + .load_skill_trust("my-skill") + .await + .unwrap() + .unwrap(); + assert_eq!( + row.trust_level, + SkillTrustLevel::Blocked, + "a source_kind change to a stricter default must demote the stored Trusted level, \ + proving min_trust's arguments are wired in the right direction at this call site" + ); + assert_eq!(row.source_kind, SourceKind::Hub); + } + + /// #6706 (M3): documents the intentional fail-closed side effect of the symmetric + /// `min_trust` enforcement — moving a `Trusted` skill's directory *into* `managed_dir` + /// (Local -> Hub) under the realistic default `[skills.trust] default_level = + /// "quarantined"` demotes it to `Quarantined`, the same as any other newly-discovered + /// Hub skill. This is not a new decision introduced by #6707/#6706, it falls out of + /// enforcing "never escalate" symmetrically: a relocation can also never *implicitly* + /// inherit a higher trust level than the destination's default allows either. + #[tokio::test] + async fn update_trust_relocation_into_managed_dir_demotes_to_stricter_hub_default() { + let managed = tempfile::tempdir().unwrap(); + let outside = tempfile::tempdir().unwrap(); + let skill_dir = outside.path().join("my-skill"); + tokio::fs::create_dir_all(&skill_dir).await.unwrap(); + let skill_md = "---\nname: my-skill\ndescription: A test skill.\nversion: 0\nsource: trace_extraction\n---\n\n## How to use\nDo the thing.\n"; + tokio::fs::write(skill_dir.join("SKILL.md"), skill_md) + .await + .unwrap(); + let hash = zeph_skills::compute_skill_hash(&skill_dir).unwrap(); + + let memory = test_memory().await; + memory + .sqlite() + .upsert_skill_trust( + "my-skill", + SkillTrustLevel::Trusted, + SourceKind::Local, + None, + skill_dir.to_str(), + &hash, + ) + .await + .unwrap(); + + let mut agent = + agent_with_memory_and_executor(memory.clone(), MockToolExecutor::no_tools()); + // Realistic operator config: unset .bundled marker/allowlist -> plain Hub skill under + // the default `default_level` (Quarantined per `TrustConfig::default()`). + agent.services.skill.trust_config.default_level = SkillTrustLevel::Quarantined; + agent.services.skill.managed_dir = Some(managed.path().to_path_buf()); + + let relocated_dir = managed.path().join("my-skill"); + tokio::fs::create_dir_all(&relocated_dir).await.unwrap(); + tokio::fs::write(relocated_dir.join("SKILL.md"), skill_md) + .await + .unwrap(); + + let meta = zeph_skills::loader::SkillMeta { + name: "my-skill".into(), + description: "A test skill.".into(), + version: 0, + source: "trace_extraction".into(), + // Relocated inside managed_dir -> classify_source_kind now returns `Hub`. + skill_dir: relocated_dir, + ..Default::default() + }; + agent.update_trust_for_reloaded_skills(&[meta]).await; + + let row = memory + .sqlite() + .load_skill_trust("my-skill") + .await + .unwrap() + .unwrap(); + assert_eq!( + row.trust_level, + SkillTrustLevel::Quarantined, + "relocating a Trusted skill into managed_dir under a stricter Hub default must \ + demote it, not silently keep the previous, more permissive Trusted level" + ); + assert_eq!(row.source_kind, SourceKind::Hub); + } + struct MessageCaptureLayer { messages: Arc>>, } @@ -659,4 +933,52 @@ mod tests { let _ = agent.skill_catalog_items().await; assert_collision_warn_fired(&messages.lock().unwrap()); } + + /// Regression test for #6706: a skill with a persisted `Quarantined` trust row must not + /// appear in the catalog fed to the mention picker (`skill_catalog_items` / + /// `Channel::send_skill_catalog`), the same way `Blocked` skills are already excluded. + #[tokio::test] + async fn skill_catalog_items_excludes_quarantined_skill() { + let temp_dir = tempfile::tempdir().unwrap(); + let skill_dir = temp_dir.path().join("quarantined-skill"); + std::fs::create_dir(&skill_dir).unwrap(); + std::fs::write( + skill_dir.join("SKILL.md"), + "---\nname: quarantined-skill\ndescription: A quarantined skill.\n---\nBody", + ) + .unwrap(); + let registry = SkillRegistry::load(&[temp_dir.path().to_path_buf()]); + + let memory = test_memory().await; + memory + .sqlite() + .upsert_skill_trust( + "quarantined-skill", + SkillTrustLevel::Quarantined, + SourceKind::Local, + None, + skill_dir.to_str(), + "irrelevant-hash", + ) + .await + .unwrap(); + + let provider = mock_provider(vec![]); + let channel = MockChannel::new(vec![]); + let mut agent = Agent::new( + provider, + channel, + registry, + None, + 5, + MockToolExecutor::no_tools(), + ) + .with_memory(memory, zeph_memory::ConversationId(1), 50, 5, 50); + + let items = agent.skill_catalog_items().await; + assert!( + items.iter().all(|i| i.name != "quarantined-skill"), + "a Quarantined skill must not appear in the catalog/mention-picker listing" + ); + } }