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" + ); + } }