From e6ff824af241324c9f16e629cf3b1f100e6bbb61 Mon Sep 17 00:00:00 2001 From: Will Killian Date: Wed, 8 Jul 2026 19:29:44 -0400 Subject: [PATCH 1/4] feat: emit automatic skill-load marks Signed-off-by: Will Killian --- crates/cli/src/adapters/mod.rs | 41 ++ crates/cli/src/installer.rs | 1 + crates/cli/src/session.rs | 14 +- crates/cli/tests/coverage/adapters_tests.rs | 107 +++++ crates/cli/tests/coverage/installer_tests.rs | 7 + crates/cli/tests/coverage/session_tests.rs | 66 ++++ crates/core/src/api/mod.rs | 1 + crates/core/src/api/skill_load.rs | 374 ++++++++++++++++++ crates/core/src/api/tool.rs | 45 ++- .../tests/integration/api_surface_tests.rs | 231 +++++++++++ crates/core/tests/unit/atif_tests.rs | 30 ++ crates/core/tests/unit/skill_load_tests.rs | 294 ++++++++++++++ .../claude-code/hooks/hooks.json | 11 + .../coding-agents/codex/hooks/hooks.json | 11 + 14 files changed, 1230 insertions(+), 3 deletions(-) create mode 100644 crates/core/src/api/skill_load.rs create mode 100644 crates/core/tests/unit/skill_load_tests.rs diff --git a/crates/cli/src/adapters/mod.rs b/crates/cli/src/adapters/mod.rs index ce60db69d..f3a121664 100644 --- a/crates/cli/src/adapters/mod.rs +++ b/crates/cli/src/adapters/mod.rs @@ -781,6 +781,17 @@ fn classify( )), ]; } + if normalized == "userpromptexpansion" + && let Some(event) = inferred_skill_load_event( + payload, + headers, + rules.kind, + extractor, + &fallback_session_id, + ) + { + return vec![NormalizedEvent::HookMark(event)]; + } let primary = classify_primary(payload, headers, extractor, rules, &fallback_session_id); if normalized == "stop" && !primary.is_terminal() { return vec![ @@ -797,6 +808,36 @@ fn classify( vec![primary] } +fn inferred_skill_load_event( + payload: &Value, + headers: &HeaderMap, + kind: AgentKind, + extractor: &dyn AgentPayloadExtractor, + fallback_session_id: &str, +) -> Option { + let expansion_type = string_at(payload, &["expansion_type"])?; + if normalize_name(&expansion_type) != "slashcommand" { + return None; + } + let skill_name = string_at(payload, &["command_name"])?; + let skill_name = skill_name.trim(); + if skill_name.is_empty() { + return None; + } + + let mut event = + common_session_event_with_fallback(payload, headers, kind, extractor, fallback_session_id); + event.payload = json!({"skill_name": skill_name}); + if let Some(metadata) = event.metadata.as_object_mut() { + metadata.insert("skill_load_source".into(), json!("prompt_expansion")); + metadata.insert("inferred".into(), json!(true)); + if let Some(command_source) = string_at(payload, &["command_source"]) { + metadata.insert("command_source".into(), json!(command_source)); + } + } + Some(event) +} + /// Classify a raw hook event using adapter-specific names before generic names. /// /// Unknown events are intentionally converted to hook marks, not errors, so new diff --git a/crates/cli/src/installer.rs b/crates/cli/src/installer.rs index 364f229e9..3bd955bf5 100644 --- a/crates/cli/src/installer.rs +++ b/crates/cli/src/installer.rs @@ -18,6 +18,7 @@ use crate::error::CliError; const HOOK_EVENTS: &[&str] = &[ "SessionStart", "UserPromptSubmit", + "UserPromptExpansion", "PreToolUse", "PostToolUse", "PostToolUseFailure", diff --git a/crates/cli/src/session.rs b/crates/cli/src/session.rs index 839b470e0..388582751 100644 --- a/crates/cli/src/session.rs +++ b/crates/cli/src/session.rs @@ -950,7 +950,19 @@ impl Session { NormalizedEvent::PromptSubmitted(event) => self.start_turn(event).await, NormalizedEvent::Compaction(event) => self.mark("compaction", event), NormalizedEvent::Notification(event) => self.mark("notification", event), - NormalizedEvent::HookMark(event) => self.mark("hook_mark", event), + NormalizedEvent::HookMark(event) => { + let name = if event + .metadata + .get("skill_load_source") + .and_then(Value::as_str) + == Some("prompt_expansion") + { + "skill.load.inferred" + } else { + "hook_mark" + }; + self.mark(name, event) + } } }) .await diff --git a/crates/cli/tests/coverage/adapters_tests.rs b/crates/cli/tests/coverage/adapters_tests.rs index 685f3c23f..517df3a18 100644 --- a/crates/cli/tests/coverage/adapters_tests.rs +++ b/crates/cli/tests/coverage/adapters_tests.rs @@ -48,6 +48,113 @@ fn maps_claude_canonical_tool_payload() { ); } +#[test] +fn preserves_supported_coding_agent_skill_load_tool_arguments() { + let cases = [ + claude_code::adapt( + json!({ + "session_id": "claude-session", + "hook_event_name": "PreToolUse", + "tool_use_id": "claude-skill", + "tool_name": "Skill", + "tool_input": {"skill": "review"} + }), + &HeaderMap::new(), + ), + codex::adapt( + json!({ + "session_id": "codex-session", + "hook_event_name": "PreToolUse", + "tool_use_id": "codex-skill", + "tool_name": "Bash", + "tool_input": {"command": "cat /workspace/skills/review/SKILL.md"} + }), + &HeaderMap::new(), + ), + hermes::adapt( + json!({ + "session_id": "hermes-session", + "hook_event_name": "pre_tool_call", + "tool_name": "skill_view", + "tool_input": {"name": "review"}, + "extra": {"tool_call_id": "hermes-skill"} + }), + &HeaderMap::new(), + ), + ]; + + let expected = [ + ("Skill", json!({"skill": "review"})), + ( + "Bash", + json!({"command": "cat /workspace/skills/review/SKILL.md"}), + ), + ("skill_view", json!({"name": "review"})), + ]; + for (outcome, (tool_name, arguments)) in cases.into_iter().zip(expected) { + match &outcome.events[0] { + NormalizedEvent::ToolStarted(event) => { + assert_eq!(event.tool_name, tool_name); + assert_eq!(event.arguments, arguments); + } + event => panic!("unexpected event: {event:?}"), + } + } +} + +#[test] +fn maps_slash_command_expansion_to_minimal_inferred_skill_mark() { + let outcome = claude_code::adapt( + json!({ + "session_id": "claude-session", + "hook_event_name": "UserPromptExpansion", + "expansion_type": "slash_command", + "command_name": "review", + "command_args": "123", + "command_source": "plugin", + "prompt": "/review 123" + }), + &HeaderMap::new(), + ); + + match &outcome.events[0] { + NormalizedEvent::HookMark(event) => { + assert_eq!(event.payload, json!({"skill_name": "review"})); + assert_eq!(event.metadata["skill_load_source"], "prompt_expansion"); + assert_eq!(event.metadata["inferred"], true); + assert_eq!(event.metadata["command_source"], "plugin"); + assert!(event.metadata.get("prompt").is_none()); + } + event => panic!("unexpected event: {event:?}"), + } +} + +#[test] +fn does_not_infer_non_slash_or_empty_prompt_expansions() { + for payload in [ + json!({ + "session_id": "claude-session", + "hook_event_name": "UserPromptExpansion", + "expansion_type": "mcp_prompt", + "command_name": "review" + }), + json!({ + "session_id": "claude-session", + "hook_event_name": "UserPromptExpansion", + "expansion_type": "slash_command", + "command_name": "" + }), + ] { + let outcome = claude_code::adapt(payload, &HeaderMap::new()); + match &outcome.events[0] { + NormalizedEvent::HookMark(event) => { + assert_ne!(event.metadata["skill_load_source"], "prompt_expansion"); + } + event => panic!("unexpected event: {event:?}"), + } + } +} + #[test] fn maps_claude_post_tool_failure_with_canonical_fields() { let headers = HeaderMap::new(); diff --git a/crates/cli/tests/coverage/installer_tests.rs b/crates/cli/tests/coverage/installer_tests.rs index 9a5b96073..5d3020176 100644 --- a/crates/cli/tests/coverage/installer_tests.rs +++ b/crates/cli/tests/coverage/installer_tests.rs @@ -78,6 +78,13 @@ fn merge_hooks_is_idempotent_and_preserves_existing_entries() { let twice = merge_hooks(once.clone(), generated).unwrap(); assert_eq!(once, twice); assert_eq!(twice["hooks"]["Stop"].as_array().unwrap().len(), 2); + assert_eq!( + twice["hooks"]["UserPromptExpansion"] + .as_array() + .unwrap() + .len(), + 1 + ); } #[test] diff --git a/crates/cli/tests/coverage/session_tests.rs b/crates/cli/tests/coverage/session_tests.rs index aa92a95e2..117831a3c 100644 --- a/crates/cli/tests/coverage/session_tests.rs +++ b/crates/cli/tests/coverage/session_tests.rs @@ -3385,6 +3385,72 @@ async fn empty_hook_marks_do_not_create_empty_atif_steps() { assert!(atif["subagent_trajectories"].is_null()); } +#[tokio::test] +async fn inferred_skill_load_hook_marks_use_the_stable_event_contract() { + let _guard = PLUGIN_CONFIG_TEST_LOCK.lock().await; + let temp = tempfile::tempdir().unwrap(); + let _ = clear_plugin_configuration(); + let atof_exporter = make_atof_test_exporter(&temp.path().join("atof"), "events.jsonl"); + let subscriber_name = "cli-inferred-skill-load-atof-test"; + let _ = deregister_subscriber(subscriber_name); + register_subscriber(subscriber_name, atof_exporter.subscriber()).unwrap(); + let manager = SessionManager::new(session_test_config()); + + manager + .apply_events( + &HeaderMap::new(), + vec![ + NormalizedEvent::AgentStarted(SessionEvent { + session_id: "inferred-skill-load".into(), + agent_kind: AgentKind::ClaudeCode, + event_name: "SessionStart".into(), + payload: json!({}), + metadata: json!({}), + }), + NormalizedEvent::HookMark(SessionEvent { + session_id: "inferred-skill-load".into(), + agent_kind: AgentKind::ClaudeCode, + event_name: "UserPromptExpansion".into(), + payload: json!({"skill_name": "review"}), + metadata: json!({ + "agent_kind": "claude-code", + "skill_load_source": "prompt_expansion", + "inferred": true + }), + }), + NormalizedEvent::AgentEnded(SessionEvent { + session_id: "inferred-skill-load".into(), + agent_kind: AgentKind::ClaudeCode, + event_name: "SessionEnd".into(), + payload: json!({}), + metadata: json!({}), + }), + ], + ) + .await + .unwrap(); + + atof_exporter.force_flush().unwrap(); + assert!(deregister_subscriber(subscriber_name).unwrap()); + let events = read_atof_events(atof_exporter.path()); + let marks = events + .iter() + .filter(|event| event["name"] == "skill.load.inferred") + .collect::>(); + assert_eq!( + marks.len(), + 1, + "expected one inferred skill mark: {events:#?}" + ); + assert_eq!(marks[0]["data"], json!({"skill_name": "review"})); + assert_eq!(marks[0]["metadata"]["agent_kind"], "claude-code"); + assert_eq!( + marks[0]["metadata"]["skill_load_source"], + "prompt_expansion" + ); + assert_eq!(marks[0]["metadata"]["inferred"], true); +} + #[tokio::test] async fn handles_out_of_order_subagent_and_tool_end_events() { let config = GatewayConfig { diff --git a/crates/core/src/api/mod.rs b/crates/core/src/api/mod.rs index f8a914e45..21b5b83b3 100644 --- a/crates/core/src/api/mod.rs +++ b/crates/core/src/api/mod.rs @@ -21,3 +21,4 @@ pub mod subscriber; pub mod tool; pub(crate) mod shared; +pub(crate) mod skill_load; diff --git a/crates/core/src/api/skill_load.rs b/crates/core/src/api/skill_load.rs new file mode 100644 index 000000000..196140c69 --- /dev/null +++ b/crates/core/src/api/skill_load.rs @@ -0,0 +1,374 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026, NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +//! Private detection helpers for eager skill-load observability marks. + +use std::collections::HashSet; + +use serde_json::Value; + +pub(crate) const HANDLED_METADATA_KEY: &str = "nemo_relay.skill_load_handled"; +pub(crate) const PRECOMPUTED_METADATA_KEY: &str = "nemo_relay.skill_loads"; + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum SkillLoadSource { + SkillTool, + StructuredRead, + ShellRead, +} + +impl SkillLoadSource { + pub(crate) const fn as_str(self) -> &'static str { + match self { + Self::SkillTool => "skill_tool", + Self::StructuredRead => "structured_read", + Self::ShellRead => "shell_read", + } + } + + fn from_str(value: &str) -> Option { + match value { + "skill_tool" => Some(Self::SkillTool), + "structured_read" => Some(Self::StructuredRead), + "shell_read" => Some(Self::ShellRead), + _ => None, + } + } +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) struct SkillLoad { + pub(crate) name: String, + pub(crate) source: SkillLoadSource, +} + +pub(crate) fn detect(tool_name: &str, args: &Value) -> Vec { + let normalized_tool = normalize_identifier(tool_name); + let source_and_names = if matches!(normalized_tool.as_str(), "skill" | "skillview") { + (SkillLoadSource::SkillTool, skill_tool_names(args)) + } else if is_structured_reader(&normalized_tool) && !has_partial_read_controls(args) { + ( + SkillLoadSource::StructuredRead, + structured_skill_names(args), + ) + } else if is_shell_tool(&normalized_tool) { + (SkillLoadSource::ShellRead, shell_skill_names(args)) + } else { + return Vec::new(); + }; + + deduplicate(source_and_names.1) + .into_iter() + .map(|name| SkillLoad { + name, + source: source_and_names.0, + }) + .collect() +} + +pub(crate) fn precomputed(metadata: Option<&Value>) -> Option> { + let entries = metadata? + .as_object()? + .get(PRECOMPUTED_METADATA_KEY)? + .as_array()?; + let mut seen = HashSet::new(); + Some( + entries + .iter() + .filter_map(|entry| { + let entry = entry.as_object()?; + let name = entry.get("skill_name")?.as_str()?.trim(); + let source = SkillLoadSource::from_str(entry.get("source")?.as_str()?)?; + (!name.is_empty() && seen.insert(name.to_string())).then(|| SkillLoad { + name: name.to_string(), + source, + }) + }) + .collect(), + ) +} + +fn skill_tool_names(args: &Value) -> Vec { + let mut names = Vec::new(); + collect_named_strings(args, &mut |key, value| { + if matches!(key.as_str(), "skill" | "skillname" | "name") { + let value = value.trim(); + if !value.is_empty() { + names.push(value.to_string()); + } + } + }); + names +} + +fn structured_skill_names(args: &Value) -> Vec { + let mut names = Vec::new(); + collect_named_values(args, &mut |key, value| { + if matches!( + key.as_str(), + "path" | "filepath" | "filename" | "file" | "paths" + ) { + collect_path_skill_names(value, &mut names); + } + }); + names +} + +fn shell_skill_names(args: &Value) -> Vec { + let mut commands = Vec::new(); + collect_named_strings(args, &mut |key, value| { + if matches!(key.as_str(), "command" | "cmd") { + commands.push(value.to_string()); + } + }); + commands + .into_iter() + .flat_map(|command| complete_reader_paths(&command)) + .filter_map(|path| skill_name_from_path(&path)) + .collect() +} + +fn is_structured_reader(tool_name: &str) -> bool { + [ + "read", + "readfile", + "readtextfile", + "readmultiplefiles", + "fileread", + ] + .iter() + .any(|reader| tool_name == *reader || tool_name.ends_with(reader)) +} + +fn is_shell_tool(tool_name: &str) -> bool { + matches!( + tool_name, + "bash" + | "shell" + | "shellcommand" + | "exec" + | "execcommand" + | "execute" + | "terminal" + | "runcommand" + | "runshellcommand" + | "shellexec" + | "powershell" + ) +} + +fn has_partial_read_controls(value: &Value) -> bool { + match value { + Value::Object(object) => object.iter().any(|(key, value)| { + let key = normalize_identifier(key); + let partial = match key.as_str() { + "offset" => value.as_i64().is_some_and(|offset| offset != 0), + "limit" | "range" | "head" | "tail" | "startline" | "endline" | "linestart" + | "lineend" => !value.is_null(), + _ => false, + }; + partial || has_partial_read_controls(value) + }), + Value::Array(values) => values.iter().any(has_partial_read_controls), + _ => false, + } +} + +fn collect_path_skill_names(value: &Value, names: &mut Vec) { + match value { + Value::String(path) => { + if let Some(name) = skill_name_from_path(path) { + names.push(name); + } + } + Value::Array(values) => { + for value in values { + collect_path_skill_names(value, names); + } + } + _ => {} + } +} + +fn collect_named_strings(value: &Value, visit: &mut impl FnMut(String, &str)) { + collect_named_values(value, &mut |key, value| { + if let Some(value) = value.as_str() { + visit(key, value); + } + }); +} + +fn collect_named_values(value: &Value, visit: &mut impl FnMut(String, &Value)) { + match value { + Value::Object(object) => { + for (key, value) in object { + visit(normalize_identifier(key), value); + collect_named_values(value, visit); + } + } + Value::Array(values) => { + for value in values { + collect_named_values(value, visit); + } + } + _ => {} + } +} + +fn skill_name_from_path(path: &str) -> Option { + let path = path.trim().trim_matches(['\'', '"']); + let components = path + .split(['/', '\\']) + .filter(|component| !component.is_empty()) + .collect::>(); + let [.., parent, file] = components.as_slice() else { + return None; + }; + if !file.eq_ignore_ascii_case("SKILL.md") + || matches!(*parent, "." | "..") + || parent.ends_with(':') + { + return None; + } + Some((*parent).to_string()) +} + +fn complete_reader_paths(command: &str) -> Vec { + let Some(words) = tokenize_simple_command(command) else { + return Vec::new(); + }; + let Some(executable) = words.first().and_then(|word| executable_name(word)) else { + return Vec::new(); + }; + match executable.as_str() { + "cat" => positional_paths(&words[1..], &[]), + "bat" | "batcat" => positional_paths(&words[1..], &["-r", "--line-range"]), + "get-content" => powershell_content_paths(&words[1..]), + _ => Vec::new(), + } +} + +fn positional_paths(words: &[String], rejected_flags: &[&str]) -> Vec { + if words.iter().any(|word| { + rejected_flags + .iter() + .any(|flag| word.eq_ignore_ascii_case(flag) || word.starts_with(&format!("{flag}="))) + }) { + return Vec::new(); + } + words + .iter() + .filter(|word| !word.starts_with('-')) + .cloned() + .collect() +} + +fn powershell_content_paths(words: &[String]) -> Vec { + if words.iter().any(|word| { + ["-totalcount", "-tail", "-head", "-first", "-last"] + .iter() + .any(|flag| word.eq_ignore_ascii_case(flag) || word.starts_with(&format!("{flag}:"))) + }) { + return Vec::new(); + } + + let mut paths = Vec::new(); + let mut index = 0; + while index < words.len() { + let word = &words[index]; + if word.eq_ignore_ascii_case("-path") || word.eq_ignore_ascii_case("-literalpath") { + if let Some(path) = words.get(index + 1) { + paths.push(path.clone()); + } + index += 2; + continue; + } + if !word.starts_with('-') { + paths.push(word.clone()); + } + index += 1; + } + paths +} + +fn tokenize_simple_command(command: &str) -> Option> { + let mut words = Vec::new(); + let mut current = String::new(); + let mut quote = None; + let mut characters = command.chars().peekable(); + + while let Some(character) = characters.next() { + match quote { + Some(active_quote) if character == active_quote => quote = None, + Some('\'') => current.push(character), + Some('"') if character == '\\' => { + if characters + .peek() + .is_some_and(|next| matches!(next, '\\' | '"' | '$' | '`' | '\n')) + { + current.push(characters.next()?); + } else { + current.push(character); + } + } + Some(_) => current.push(character), + None if matches!(character, '\'' | '"') => quote = Some(character), + None if matches!(character, '|' | '&' | ';' | '<' | '>' | '`' | '\n' | '\r') => { + return None; + } + None if character.is_whitespace() => { + if !current.is_empty() { + words.push(std::mem::take(&mut current)); + } + } + None if character == '$' && characters.peek() == Some(&'(') => return None, + None if character == '\\' => { + if characters + .peek() + .is_some_and(|next| next.is_whitespace() || matches!(next, '\\' | '\'' | '"')) + { + current.push(characters.next()?); + } else { + current.push(character); + } + } + None => current.push(character), + } + } + if quote.is_some() { + return None; + } + if !current.is_empty() { + words.push(current); + } + (!words.is_empty()).then_some(words) +} + +fn executable_name(executable: &str) -> Option { + executable + .rsplit(['/', '\\']) + .next() + .map(|name| name.to_ascii_lowercase()) + .map(|name| name.strip_suffix(".exe").unwrap_or(&name).to_string()) + .filter(|name| !name.is_empty()) +} + +fn normalize_identifier(value: &str) -> String { + value + .chars() + .filter(|character| character.is_ascii_alphanumeric()) + .flat_map(char::to_lowercase) + .collect() +} + +fn deduplicate(names: Vec) -> Vec { + let mut seen = HashSet::new(); + names + .into_iter() + .filter(|name| seen.insert(name.clone())) + .collect() +} + +#[cfg(test)] +#[path = "../../tests/unit/skill_load_tests.rs"] +mod tests; diff --git a/crates/core/src/api/tool.rs b/crates/core/src/api/tool.rs index 3d60c7f80..564e31352 100644 --- a/crates/core/src/api/tool.rs +++ b/crates/core/src/api/tool.rs @@ -14,6 +14,7 @@ use crate::api::shared::{ ensure_runtime_owner, metadata_with_otel_status, resolve_parent_uuid, sanitize_event, snapshot_event_subscribers, }; +use crate::api::skill_load; use crate::error::{FlowError, Result}; use crate::json::Json; use chrono::{DateTime, TimeDelta, Utc}; @@ -229,12 +230,26 @@ fn tool_call_with_subscriber_snapshot( let entries = state.tool_sanitize_request_entries(&scope_locals); (entries, subscribers) }; + let handled_skill_loads = params + .metadata + .as_ref() + .and_then(Json::as_object) + .and_then(|metadata| metadata.get(skill_load::HANDLED_METADATA_KEY)) + .and_then(Json::as_bool) + .is_some_and(|handled| handled); + let skill_loads = if handled_skill_loads { + Vec::new() + } else if let Some(skill_loads) = skill_load::precomputed(params.metadata.as_ref()) { + skill_loads + } else { + skill_load::detect(params.name, ¶ms.args) + }; let sanitized_args = NemoRelayContextState::tool_sanitize_request_snapshot_chain( params.name, params.args, &entries, ); - let (handle, event) = { + let (handle, event, marks) = { let context = global_context(); let state = context .read() @@ -250,11 +265,37 @@ fn tool_call_with_subscriber_snapshot( .build(); let handle = state.create_tool_handle(handle_params); let event = state.build_tool_start_event(&handle, Some(sanitized_args)); - (handle, event) + let marks = skill_loads + .into_iter() + .map(|skill_load| { + state.create_event(MarkEvent::new( + BaseEvent::builder() + .name("skill.load") + .parent_uuid(handle.uuid) + .timestamp(handle.started_at) + .data(json!({"skill_name": skill_load.name})) + .metadata(json!({ + "skill_load_source": skill_load.source.as_str(), + "tool_name": handle.name, + })) + .build(), + None, + None, + )) + }) + .collect::>(); + (handle, event, marks) }; + let marks = marks + .into_iter() + .filter_map(sanitize_event) + .collect::>(); if let Some(event) = sanitize_event(event) { NemoRelayContextState::emit_event(&event, &subscribers); } + for mark in marks { + NemoRelayContextState::emit_event(&mark, &subscribers); + } Ok((handle, subscribers)) } diff --git a/crates/core/tests/integration/api_surface_tests.rs b/crates/core/tests/integration/api_surface_tests.rs index fdac6e00e..c508cc484 100644 --- a/crates/core/tests/integration/api_surface_tests.rs +++ b/crates/core/tests/integration/api_surface_tests.rs @@ -338,6 +338,237 @@ fn shared_type_reexports_keep_existing_core_paths() { assert!(attributes.contains(ToolAttributes::REMOTE)); } +#[test] +fn tool_start_eagerly_emits_deduplicated_skill_load_marks() { + let _lock = TEST_MUTEX.lock().unwrap(); + reset_global(); + setup_isolated_thread(); + + let events = capture_events("skill-load-api-events"); + let tool_handle = tool_call( + nemo_relay::api::tool::ToolCallParams::builder() + .name("read_multiple_files") + .args(json!({ + "paths": [ + "/skills/review/SKILL.md", + "/skills/testing/SKILL.md", + "/skills/review/SKILL.md" + ] + })) + .build(), + ) + .unwrap(); + tool_call_end( + nemo_relay::api::tool::ToolCallEndParams::builder() + .handle(&tool_handle) + .result(json!({"ok": true})) + .build(), + ) + .unwrap(); + + let captured = captured_events_snapshot(&events); + assert_eq!(captured.len(), 4); + assert_eq!(captured[0].name(), "read_multiple_files"); + assert_eq!(captured[0].scope_category(), Some(ScopeCategory::Start)); + assert_eq!(captured[1].name(), "skill.load"); + assert_eq!(captured[1].parent_uuid(), Some(tool_handle.uuid)); + assert_eq!(captured[1].data(), Some(&json!({"skill_name": "review"}))); + assert_eq!( + captured[1].metadata(), + Some(&json!({ + "skill_load_source": "structured_read", + "tool_name": "read_multiple_files" + })) + ); + assert_eq!(captured[2].name(), "skill.load"); + assert_eq!(captured[2].parent_uuid(), Some(tool_handle.uuid)); + assert_eq!(captured[2].data(), Some(&json!({"skill_name": "testing"}))); + assert_eq!(captured[3].scope_category(), Some(ScopeCategory::End)); + + deregister_subscriber("skill-load-api-events").unwrap(); +} + +#[test] +fn integration_owned_skill_load_metadata_suppresses_core_detection() { + let _lock = TEST_MUTEX.lock().unwrap(); + reset_global(); + setup_isolated_thread(); + + let events = capture_events("handled-skill-load-api-events"); + let tool_handle = tool_call( + nemo_relay::api::tool::ToolCallParams::builder() + .name("read_file") + .args(json!({"path": "/skills/review/SKILL.md"})) + .metadata(json!({"nemo_relay.skill_load_handled": true})) + .build(), + ) + .unwrap(); + tool_call_end( + nemo_relay::api::tool::ToolCallEndParams::builder() + .handle(&tool_handle) + .result(json!({"ok": true})) + .build(), + ) + .unwrap(); + + let captured = captured_events_snapshot(&events); + assert_eq!(captured.len(), 2); + assert!(captured.iter().all(|event| event.name() != "skill.load")); + + deregister_subscriber("handled-skill-load-api-events").unwrap(); +} + +#[test] +fn integration_precomputed_skill_load_survives_stripped_tool_arguments() { + let _lock = TEST_MUTEX.lock().unwrap(); + reset_global(); + setup_isolated_thread(); + + let events = capture_events("precomputed-skill-load-api-events"); + let tool_handle = tool_call( + nemo_relay::api::tool::ToolCallParams::builder() + .name("read_file") + .args(json!({"stripped": true, "argKeys": ["path"]})) + .metadata(json!({ + "nemo_relay.skill_loads": [{ + "skill_name": "review", + "source": "structured_read" + }] + })) + .build(), + ) + .unwrap(); + tool_call_end( + nemo_relay::api::tool::ToolCallEndParams::builder() + .handle(&tool_handle) + .result(json!({"ok": true})) + .build(), + ) + .unwrap(); + + let captured = captured_events_snapshot(&events); + assert_eq!(captured.len(), 3); + assert_eq!(captured[1].name(), "skill.load"); + assert_eq!(captured[1].parent_uuid(), Some(tool_handle.uuid)); + assert_eq!(captured[1].data(), Some(&json!({"skill_name": "review"}))); + assert_eq!( + captured[1].metadata(), + Some(&json!({ + "skill_load_source": "structured_read", + "tool_name": "read_file" + })) + ); + + deregister_subscriber("precomputed-skill-load-api-events").unwrap(); +} + +#[test] +fn skill_load_detection_uses_original_arguments_before_observability_sanitization() { + let _lock = TEST_MUTEX.lock().unwrap(); + reset_global(); + setup_isolated_thread(); + + register_tool_sanitize_request_guardrail( + "strip-skill-path", + 1, + Arc::new(|_name, _args| json!({"path": "[redacted]"})), + ) + .unwrap(); + let events = capture_events("sanitized-skill-load-api-events"); + let tool_handle = tool_call( + nemo_relay::api::tool::ToolCallParams::builder() + .name("read_file") + .args(json!({"path": "/skills/review/SKILL.md"})) + .build(), + ) + .unwrap(); + tool_call_end( + nemo_relay::api::tool::ToolCallEndParams::builder() + .handle(&tool_handle) + .result(json!({"ok": true})) + .build(), + ) + .unwrap(); + + let captured = captured_events_snapshot(&events); + assert_eq!(captured[0].data(), Some(&json!({"path": "[redacted]"}))); + assert_eq!(captured[1].name(), "skill.load"); + assert_eq!(captured[1].data(), Some(&json!({"skill_name": "review"}))); + + deregister_subscriber("sanitized-skill-load-api-events").unwrap(); + deregister_tool_sanitize_request_guardrail("strip-skill-path").unwrap(); +} + +#[tokio::test] +async fn managed_skill_load_marks_survive_failures_repeat_per_call_and_skip_blocked_calls() { + let _lock = TEST_MUTEX.lock().unwrap(); + reset_global(); + setup_isolated_thread(); + + let events = capture_events("managed-skill-load-api-events"); + let skill_args = json!({"path": "/skills/review/SKILL.md"}); + + let error = tool_call_execute( + nemo_relay::api::tool::ToolCallExecuteParams::builder() + .name("read_file") + .args(skill_args.clone()) + .func(failing_tool_exec()) + .build(), + ) + .await + .unwrap_err(); + assert!(matches!(error, FlowError::Internal(message) if message == "tool execution failed")); + + for _ in 0..2 { + tool_call_execute( + nemo_relay::api::tool::ToolCallExecuteParams::builder() + .name("read_file") + .args(skill_args.clone()) + .func(noop_tool_exec()) + .build(), + ) + .await + .unwrap(); + } + + register_tool_conditional_execution_guardrail( + "block-skill-load", + 1, + Arc::new(|_name, _args| Ok(Some("blocked before start".into()))), + ) + .unwrap(); + let blocked = tool_call_execute( + nemo_relay::api::tool::ToolCallExecuteParams::builder() + .name("read_file") + .args(skill_args) + .func(noop_tool_exec()) + .build(), + ) + .await; + assert!(matches!(blocked, Err(FlowError::GuardrailRejected(_)))); + deregister_tool_conditional_execution_guardrail("block-skill-load").unwrap(); + + let captured = captured_events_snapshot(&events); + let skill_marks = captured + .iter() + .filter(|event| event.name() == "skill.load") + .collect::>(); + assert_eq!(skill_marks.len(), 3); + for mark in skill_marks { + let position = captured + .iter() + .position(|event| event.uuid() == mark.uuid()) + .unwrap(); + assert_eq!( + captured[position - 1].scope_category(), + Some(ScopeCategory::Start) + ); + assert_eq!(mark.parent_uuid(), Some(captured[position - 1].uuid())); + } + + deregister_subscriber("managed-skill-load-api-events").unwrap(); +} + #[test] fn test_manual_lifecycle_timestamp_overrides() { let _lock = TEST_MUTEX.lock().unwrap(); diff --git a/crates/core/tests/unit/atif_tests.rs b/crates/core/tests/unit/atif_tests.rs index 701e6fd21..caad6209e 100644 --- a/crates/core/tests/unit/atif_tests.rs +++ b/crates/core/tests/unit/atif_tests.rs @@ -2780,6 +2780,36 @@ fn test_exporter_ignores_marks_with_hook_metadata() { assert!(trajectory.steps.is_empty()); } +#[test] +fn test_exporter_omits_skill_load_mark_from_atif_steps() { + let exporter = AtifExporter::new("session-1".to_string(), make_agent_info()); + let tool_uuid = Uuid::now_v7(); + let mark_uuid = Uuid::now_v7(); + + let tool_start = event_builder(tool_uuid, EventType::Start) + .name("read_file") + .scope_type(ScopeType::Tool) + .build(); + let mark = event_builder(mark_uuid, EventType::Mark) + .name("skill.load") + .parent_uuid(tool_uuid) + .data(json!({"skill_name": "review"})) + .metadata(json!({ + "skill_load_source": "structured_read", + "tool_name": "read_file" + })) + .build(); + + { + let mut state = exporter.state.lock().unwrap(); + state.events.push(tool_start); + state.events.push(mark); + } + + let trajectory = exporter.export().unwrap(); + assert!(trajectory.steps.is_empty()); +} + #[test] fn test_exporter_embeds_nested_subagent_trajectory() { let root_uuid = Uuid::now_v7(); diff --git a/crates/core/tests/unit/skill_load_tests.rs b/crates/core/tests/unit/skill_load_tests.rs new file mode 100644 index 000000000..120b7628f --- /dev/null +++ b/crates/core/tests/unit/skill_load_tests.rs @@ -0,0 +1,294 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026, NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +use serde_json::{Value, json}; + +use super::{SkillLoad, SkillLoadSource, detect, precomputed}; + +fn expected(names: &[&str], source: SkillLoadSource) -> Vec { + names + .iter() + .map(|name| SkillLoad { + name: (*name).to_string(), + source, + }) + .collect() +} + +fn assert_detects(tool: &str, args: Value, names: &[&str], source: SkillLoadSource) { + assert_eq!( + detect(tool, &args), + expected(names, source), + "tool={tool}, args={args}" + ); +} + +fn assert_rejected(tool: &str, args: Value) { + assert!(detect(tool, &args).is_empty(), "tool={tool}, args={args}"); +} + +#[test] +fn first_class_tools_accept_every_supported_name_field() { + for (tool, args, name) in [ + ("Skill", json!({"skill": "review"}), "review"), + ("skill_view", json!({"skill_name": "testing"}), "testing"), + ( + "skill-view", + json!({"name": "mlops/axolotl"}), + "mlops/axolotl", + ), + ( + "skill_view", + json!({"request": {"name": "nested-skill"}}), + "nested-skill", + ), + ] { + assert_detects(tool, args, &[name], SkillLoadSource::SkillTool); + } +} + +#[test] +fn first_class_tools_reject_missing_empty_and_non_string_names() { + for (tool, args) in [ + ("Skill", json!({})), + ("Skill", json!({"skill": " "})), + ("skill_view", json!({"name": 7})), + ("skill_catalog", json!({"skill": "review"})), + ] { + assert_rejected(tool, args); + } +} + +#[test] +fn structured_readers_accept_every_supported_tool_and_path_field() { + for (tool, args) in [ + ("Read", json!({"path": "/skills/review/SKILL.md"})), + ("read_file", json!({"file_path": "/skills/review/SKILL.md"})), + ( + "read_text_file", + json!({"filepath": "/skills/review/SKILL.md"}), + ), + ("file_read", json!({"filename": "/skills/review/SKILL.md"})), + ( + "mcp__filesystem__read_file", + json!({"file": "/skills/review/SKILL.md"}), + ), + ( + "mcp__filesystem__read_multiple_files", + json!({"paths": ["/skills/review/SKILL.md"]}), + ), + ] { + assert_detects(tool, args, &["review"], SkillLoadSource::StructuredRead); + } +} + +#[test] +fn structured_readers_support_posix_windows_relative_nested_and_duplicate_paths() { + assert_detects( + "read_multiple_files", + json!({ + "request": { + "paths": [ + "/workspace/.agents/skills/review/SKILL.md", + "C:\\Users\\me\\.codex\\skills\\test-runner\\skill.MD", + "relative/skills/authoring/SKILL.md", + "/workspace/.agents/skills/review/SKILL.md" + ] + } + }), + &["review", "test-runner", "authoring"], + SkillLoadSource::StructuredRead, + ); +} + +#[test] +fn structured_readers_allow_zero_offset_but_reject_every_partial_read_control() { + assert_detects( + "Read", + json!({"file_path": "/skills/review/SKILL.md", "offset": 0}), + &["review"], + SkillLoadSource::StructuredRead, + ); + + for args in [ + json!({"file_path": "/skills/review/SKILL.md", "offset": 1}), + json!({"file_path": "/skills/review/SKILL.md", "offset": -1}), + json!({"file_path": "/skills/review/SKILL.md", "limit": 2000}), + json!({"file_path": "/skills/review/SKILL.md", "range": "1:20"}), + json!({"file_path": "/skills/review/SKILL.md", "head": true}), + json!({"file_path": "/skills/review/SKILL.md", "tail": 20}), + json!({"file_path": "/skills/review/SKILL.md", "start_line": 1}), + json!({"file_path": "/skills/review/SKILL.md", "end_line": 20}), + json!({"file_path": "/skills/review/SKILL.md", "line_start": 1}), + json!({"file_path": "/skills/review/SKILL.md", "line_end": 20}), + json!({ + "file_path": "/skills/review/SKILL.md", + "options": {"limit": 20} + }), + ] { + assert_rejected("Read", args); + } +} + +#[test] +fn structured_readers_reject_non_skill_paths_missing_parents_and_non_read_tools() { + for (tool, args) in [ + ("Read", json!({"file_path": "/skills/review/README.md"})), + ("Read", json!({"file_path": "SKILL.md"})), + ("Read", json!({"file_path": "/SKILL.md"})), + ("Read", json!({"file_path": "C:\\SKILL.md"})), + ("Read", json!({"file_path": "/skills/./SKILL.md"})), + ("Read", json!({"file_path": "/skills/../SKILL.md"})), + ( + "write_file", + json!({"file_path": "/skills/review/SKILL.md"}), + ), + ("edit_file", json!({"file_path": "/skills/review/SKILL.md"})), + ("list_directory", json!({"path": "/skills/review/SKILL.md"})), + ] { + assert_rejected(tool, args); + } +} + +#[test] +fn shell_detection_accepts_complete_cat_bat_and_batcat_commands() { + for (tool, command, names) in [ + ( + "Bash", + "cat -n '/workspace/skills/review/SKILL.md'", + &["review"][..], + ), + ( + "exec_command", + "/usr/bin/bat --plain C:\\skills\\testing\\SKILL.md", + &["testing"][..], + ), + ( + "terminal", + "\"C:\\Tools\\bat.exe\" --plain C:\\skills\\review\\SKILL.md", + &["review"][..], + ), + ( + "run_shell_command", + "batcat /skills/review/SKILL.md /skills/testing/SKILL.md /skills/review/SKILL.md", + &["review", "testing"][..], + ), + ] { + assert_detects( + tool, + json!({"command": command}), + names, + SkillLoadSource::ShellRead, + ); + } +} + +#[test] +fn shell_detection_accepts_complete_powershell_get_content_forms() { + for command in [ + "Get-Content -Raw -LiteralPath 'C:\\skills\\review\\SKILL.md'", + "Get-Content -Encoding utf8 -Path C:\\skills\\review\\SKILL.md", + "C:\\Windows\\System32\\Get-Content.exe C:\\skills\\review\\SKILL.md", + ] { + assert_detects( + "powershell", + json!({"cmd": command}), + &["review"], + SkillLoadSource::ShellRead, + ); + } +} + +#[test] +fn shell_detection_rejects_partial_transformed_and_compound_commands() { + for command in [ + "sed -n '1,200p' /skills/review/SKILL.md", + "head /skills/review/SKILL.md", + "tail /skills/review/SKILL.md", + "bat -r 1:20 /skills/review/SKILL.md", + "bat --line-range 1:20 /skills/review/SKILL.md", + "bat --line-range=1:20 /skills/review/SKILL.md", + "Get-Content -TotalCount 20 /skills/review/SKILL.md", + "Get-Content -Tail 20 /skills/review/SKILL.md", + "Get-Content -Head 20 /skills/review/SKILL.md", + "Get-Content -First 20 /skills/review/SKILL.md", + "Get-Content -Last 20 /skills/review/SKILL.md", + "cat /skills/review/SKILL.md | head", + "cat /skills/review/SKILL.md > /tmp/copy", + "cat /skills/review/SKILL.md < /tmp/input", + "cat /skills/review/SKILL.md && echo done", + "cat /skills/review/SKILL.md || echo failed", + "cat /skills/review/SKILL.md; echo done", + "cat /skills/review/SKILL.md\necho done", + "cat $(find /skills -name SKILL.md)", + "cat `find /skills -name SKILL.md`", + ] { + assert_rejected("shell", json!({"command": command})); + } +} + +#[test] +fn shell_detection_rejects_malformed_unknown_and_non_command_inputs() { + for (tool, args) in [ + ("shell", json!({"command": "cat '/skills/review/SKILL.md"})), + ( + "shell", + json!({"command": "cp /skills/review/SKILL.md /tmp"}), + ), + ("shell", json!({"command": ""})), + ("shell", json!({"command": 7})), + ("shell", json!({"script": "cat /skills/review/SKILL.md"})), + ("python", json!({"command": "cat /skills/review/SKILL.md"})), + ] { + assert_rejected(tool, args); + } +} + +#[test] +fn precomputed_detections_validate_sources_names_and_deduplicate() { + assert_eq!( + precomputed(Some(&json!({ + "nemo_relay.skill_loads": [ + {"skill_name": "review", "source": "structured_read"}, + {"skill_name": "testing", "source": "skill_tool"}, + {"skill_name": "authoring", "source": "shell_read"}, + {"skill_name": "review", "source": "structured_read"}, + {"skill_name": "", "source": "shell_read"}, + {"skill_name": "ignored", "source": "unknown"}, + {"source": "shell_read"}, + "malformed" + ] + }))), + Some(vec![ + SkillLoad { + name: "review".into(), + source: SkillLoadSource::StructuredRead, + }, + SkillLoad { + name: "testing".into(), + source: SkillLoadSource::SkillTool, + }, + SkillLoad { + name: "authoring".into(), + source: SkillLoadSource::ShellRead, + }, + ]) + ); +} + +#[test] +fn precomputed_detections_reject_missing_and_malformed_envelopes() { + for metadata in [ + None, + Some(json!(null)), + Some(json!([])), + Some(json!({})), + Some(json!({"nemo_relay.skill_loads": {}})), + ] { + assert_eq!(precomputed(metadata.as_ref()), None); + } + assert_eq!( + precomputed(Some(&json!({"nemo_relay.skill_loads": []}))), + Some(Vec::new()) + ); +} diff --git a/integrations/coding-agents/claude-code/hooks/hooks.json b/integrations/coding-agents/claude-code/hooks/hooks.json index 02df67346..b43125111 100644 --- a/integrations/coding-agents/claude-code/hooks/hooks.json +++ b/integrations/coding-agents/claude-code/hooks/hooks.json @@ -22,6 +22,17 @@ ] } ], + "UserPromptExpansion": [ + { + "hooks": [ + { + "type": "command", + "command": "nemo-relay plugin-shim hook claude", + "timeout": 30 + } + ] + } + ], "PreToolUse": [ { "matcher": "*", diff --git a/integrations/coding-agents/codex/hooks/hooks.json b/integrations/coding-agents/codex/hooks/hooks.json index 43a79c2be..8dcfc8906 100644 --- a/integrations/coding-agents/codex/hooks/hooks.json +++ b/integrations/coding-agents/codex/hooks/hooks.json @@ -23,6 +23,17 @@ ] } ], + "UserPromptExpansion": [ + { + "hooks": [ + { + "type": "command", + "command": "nemo-relay plugin-shim hook codex", + "timeout": 30 + } + ] + } + ], "PreToolUse": [ { "matcher": "*", From 1b019f3ff5f9977c2255e57b1d817837a690c8f0 Mon Sep 17 00:00:00 2001 From: Zhongxuan Wang Date: Thu, 9 Jul 2026 17:38:39 -0700 Subject: [PATCH 2/4] fix: declare Claude Code 2.1.116 hook-event floor UserPromptExpansion is only present in Claude Code's plugin hook whitelist from 2.1.116. Older hosts reject the entire plugin hooks file on the unknown event name and silently load no relay hooks, for both the transparent-run temp plugin and the marketplace plugin. Document the verified floor on HOOK_EVENTS, make doctor warn when the probed Claude Code predates it, and state the requirement in the plugin README. Signed-off-by: Zhongxuan Wang (cherry picked from commit d3b895910daef0951993bc28d53f0b5664987c19) --- crates/cli/src/doctor.rs | 36 +++++++++++++++++++ crates/cli/src/installer.rs | 13 ++++--- crates/cli/tests/coverage/doctor_tests.rs | 23 ++++++++++++ .../coding-agents/claude-code/README.md | 12 +++++-- 4 files changed, 76 insertions(+), 8 deletions(-) diff --git a/crates/cli/src/doctor.rs b/crates/cli/src/doctor.rs index 4b47d17f9..60edb4f45 100644 --- a/crates/cli/src/doctor.rs +++ b/crates/cli/src/doctor.rs @@ -430,6 +430,12 @@ async fn collect_agents( if !hook_details.is_empty() { details.push(hook_details); } + if agent == CodingAgent::ClaudeCode + && let Some(warning) = version.as_deref().and_then(claude_hook_floor_warning) + { + status = combine_status(status, Status::Warn, true); + details.push(warning); + } out.push(AgentInfo { name: display_name, status, @@ -577,6 +583,36 @@ fn hook_file_status( } } +// Claude Code validates plugin hooks.json against a strict event-name whitelist and rejects the +// entire plugin's hooks on one unknown name. 2.1.116 is the oldest release that accepts every +// event in the generated hook config (`UserPromptExpansion` was added to the whitelist there), +// so older hosts silently load no relay hooks at all. Keep in sync with `HOOK_EVENTS` in +// installer.rs. +const CLAUDE_HOOK_EVENT_FLOOR: (u64, u64, u64) = (2, 1, 116); + +// Returns a doctor warning when a probed Claude Code version predates the hook-event floor. +// Unparseable version strings return None: a missing warning is recoverable, a false one is not. +fn claude_hook_floor_warning(version: &str) -> Option { + let parsed = parse_leading_semver(version)?; + (parsed < CLAUDE_HOOK_EVENT_FLOOR).then(|| { + let (major, minor, patch) = CLAUDE_HOOK_EVENT_FLOOR; + format!( + "version predates {major}.{minor}.{patch}; this Claude Code rejects \ + UserPromptExpansion and will silently load no relay hooks" + ) + }) +} + +// Parses the leading `major.minor.patch` token from a probed version line such as +// "2.1.206 (Claude Code)". Suffixes like prerelease tags fail the numeric parse and yield None. +fn parse_leading_semver(version: &str) -> Option<(u64, u64, u64)> { + let mut parts = version.split_whitespace().next()?.splitn(3, '.'); + let major = parts.next()?.parse().ok()?; + let minor = parts.next()?.parse().ok()?; + let patch = parts.next()?.parse().ok()?; + Some((major, minor, patch)) +} + async fn probe_version(binary: &Path) -> Option { // Spawn ` --version` and read the first line of stdout. Bounded by the network // timeout (re-used as a generic short timeout) so a misbehaving binary doesn't hang doctor. diff --git a/crates/cli/src/installer.rs b/crates/cli/src/installer.rs index 3bd955bf5..23462d0d9 100644 --- a/crates/cli/src/installer.rs +++ b/crates/cli/src/installer.rs @@ -10,11 +10,14 @@ use serde_json::{Value, json}; use crate::config::{CodingAgent, GatewayMode, HookForwardCommand}; use crate::error::CliError; -// Claude Code's hook loader strictly whitelists event names — any unknown event causes the -// entire hooks file to be rejected (no hooks register). Only events present in Claude Code's -// whitelist as of 2.1.x belong here. Codex 0.129 has a smaller subset (SessionStart, -// UserPromptSubmit, PreToolUse, PostToolUse, Stop, PreCompact, PostCompact, PermissionRequest) -// and silently ignores events it doesn't recognize, so the union list is safe for both agents. +// Claude Code validates plugin hooks.json against a strict event-name whitelist — one unknown +// event rejects the entire plugin's hooks (no hooks register, silently). Both Claude vectors +// (the transparent-run temp plugin and the marketplace plugin) are plugin hooks.json, so every +// event here must exist in the oldest supported Claude Code. UserPromptExpansion sets that +// floor: 2.1.116 (verified empirically; 2.1.114 rejects it — see `claude_hook_floor_warning` +// in doctor.rs). Codex 0.129 has a smaller subset (SessionStart, UserPromptSubmit, PreToolUse, +// PostToolUse, Stop, PreCompact, PostCompact, PermissionRequest) and silently ignores events +// it doesn't recognize, so the union list is safe for both agents. const HOOK_EVENTS: &[&str] = &[ "SessionStart", "UserPromptSubmit", diff --git a/crates/cli/tests/coverage/doctor_tests.rs b/crates/cli/tests/coverage/doctor_tests.rs index 02d089519..a93cab3c5 100644 --- a/crates/cli/tests/coverage/doctor_tests.rs +++ b/crates/cli/tests/coverage/doctor_tests.rs @@ -1625,3 +1625,26 @@ fn format_agents_json_matches_doctor_agents_shape() { assert_eq!(parsed[0]["version"], "2.1.4"); assert_eq!(parsed[0]["path"], "/opt/homebrew/bin/claude"); } + +#[test] +fn claude_hook_floor_warning_pins_version_boundary() { + // 2.1.116 is the first Claude Code whose plugin hook whitelist accepts UserPromptExpansion; + // 2.1.114 is the newest published version that rejects it (2.1.115 was never published). + let cases = [ + ("2.1.114 (Claude Code)", true), + ("2.1.116 (Claude Code)", false), + ("2.1.206 (Claude Code)", false), + ("2.0.999 (Claude Code)", true), + ("3.0.0 (Claude Code)", false), + ("2.1.116-beta (Claude Code)", false), + ("not a version", false), + ("", false), + ]; + for (version, expect_warning) in cases { + assert_eq!( + claude_hook_floor_warning(version).is_some(), + expect_warning, + "unexpected floor verdict for {version:?}" + ); + } +} diff --git a/integrations/coding-agents/claude-code/README.md b/integrations/coding-agents/claude-code/README.md index e14f0d869..1fef347de 100644 --- a/integrations/coding-agents/claude-code/README.md +++ b/integrations/coding-agents/claude-code/README.md @@ -21,9 +21,15 @@ same local hook and gateway controls as Claude Code. ## Captured Events The bundle forwards `SessionStart`, `SessionEnd`, `UserPromptSubmit`, -`PreToolUse`, `PostToolUse`, `PostToolUseFailure`, `PermissionRequest`, -`SubagentStart`, `SubagentStop`, `Notification`, `Stop`, `PreCompact`, and -`PostCompact` as scope, tool, mark, or private LLM correlation events. +`UserPromptExpansion`, `PreToolUse`, `PostToolUse`, `PostToolUseFailure`, +`PermissionRequest`, `SubagentStart`, `SubagentStop`, `Notification`, `Stop`, +`PreCompact`, and `PostCompact` as scope, tool, mark, or private LLM +correlation events. + +The bundle requires Claude Code 2.1.116 or newer. Older versions do not have +`UserPromptExpansion` in their hook-event whitelist and reject the entire +plugin hook configuration, so no relay hooks load. `nemo-relay doctor` reports +this condition. Claude Code observability is turn-oriented. A multi-turn session can produce one root `claude-code-turn` span or ATIF trajectory per user turn. That is expected From cbc7c9eeb818f683d1e86a5d680fa444002eee2c Mon Sep 17 00:00:00 2001 From: Will Killian Date: Fri, 10 Jul 2026 09:40:00 -0400 Subject: [PATCH 3/4] fix: streamline skill-load detection Signed-off-by: Will Killian --- Cargo.lock | 1 + crates/cli/src/adapters/mod.rs | 8 +- crates/cli/src/session.rs | 5 +- crates/cli/tests/coverage/adapters_tests.rs | 14 +- crates/core/Cargo.toml | 1 + crates/core/src/api/skill_load.rs | 180 ++++++------------ crates/core/src/api/tool.rs | 14 +- crates/core/tests/unit/skill_load_tests.rs | 3 + .../coding-agents/codex/hooks/hooks.json | 11 -- 9 files changed, 96 insertions(+), 141 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 5927aa621..ca7125f5c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1494,6 +1494,7 @@ dependencies = [ "serde", "serde_json", "sha2", + "shell-words", "strum", "tempfile", "thiserror 2.0.18", diff --git a/crates/cli/src/adapters/mod.rs b/crates/cli/src/adapters/mod.rs index f3a121664..f117a4830 100644 --- a/crates/cli/src/adapters/mod.rs +++ b/crates/cli/src/adapters/mod.rs @@ -5,6 +5,9 @@ pub(crate) mod claude_code; pub(crate) mod codex; pub(crate) mod hermes; +pub(crate) const SKILL_LOAD_SOURCE_KEY: &str = "skill_load_source"; +pub(crate) const SKILL_LOAD_SOURCE_PROMPT_EXPANSION: &str = "prompt_expansion"; + use axum::http::HeaderMap; use serde_json::{Map, Value, json}; use uuid::Uuid; @@ -829,7 +832,10 @@ fn inferred_skill_load_event( common_session_event_with_fallback(payload, headers, kind, extractor, fallback_session_id); event.payload = json!({"skill_name": skill_name}); if let Some(metadata) = event.metadata.as_object_mut() { - metadata.insert("skill_load_source".into(), json!("prompt_expansion")); + metadata.insert( + SKILL_LOAD_SOURCE_KEY.into(), + json!(SKILL_LOAD_SOURCE_PROMPT_EXPANSION), + ); metadata.insert("inferred".into(), json!(true)); if let Some(command_source) = string_at(payload, &["command_source"]) { metadata.insert("command_source".into(), json!(command_source)); diff --git a/crates/cli/src/session.rs b/crates/cli/src/session.rs index 388582751..a479a5c0f 100644 --- a/crates/cli/src/session.rs +++ b/crates/cli/src/session.rs @@ -23,6 +23,7 @@ use nemo_relay::api::tool::{ use serde_json::{Map, Value, json}; use tokio::sync::Mutex; +use crate::adapters::{SKILL_LOAD_SOURCE_KEY, SKILL_LOAD_SOURCE_PROMPT_EXPANSION}; use crate::alignment::{ self, GatewayManagementPolicy, PendingSubagentStart, SessionAlias, SessionAlignmentState, insert_optional, json_string_at, json_value_at, merge_metadata, @@ -953,9 +954,9 @@ impl Session { NormalizedEvent::HookMark(event) => { let name = if event .metadata - .get("skill_load_source") + .get(SKILL_LOAD_SOURCE_KEY) .and_then(Value::as_str) - == Some("prompt_expansion") + == Some(SKILL_LOAD_SOURCE_PROMPT_EXPANSION) { "skill.load.inferred" } else { diff --git a/crates/cli/tests/coverage/adapters_tests.rs b/crates/cli/tests/coverage/adapters_tests.rs index 517df3a18..97614a6b5 100644 --- a/crates/cli/tests/coverage/adapters_tests.rs +++ b/crates/cli/tests/coverage/adapters_tests.rs @@ -120,7 +120,10 @@ fn maps_slash_command_expansion_to_minimal_inferred_skill_mark() { match &outcome.events[0] { NormalizedEvent::HookMark(event) => { assert_eq!(event.payload, json!({"skill_name": "review"})); - assert_eq!(event.metadata["skill_load_source"], "prompt_expansion"); + assert_eq!( + event.metadata[SKILL_LOAD_SOURCE_KEY], + SKILL_LOAD_SOURCE_PROMPT_EXPANSION + ); assert_eq!(event.metadata["inferred"], true); assert_eq!(event.metadata["command_source"], "plugin"); assert!(event.metadata.get("prompt").is_none()); @@ -148,7 +151,14 @@ fn does_not_infer_non_slash_or_empty_prompt_expansions() { let outcome = claude_code::adapt(payload, &HeaderMap::new()); match &outcome.events[0] { NormalizedEvent::HookMark(event) => { - assert_ne!(event.metadata["skill_load_source"], "prompt_expansion"); + assert_ne!( + event + .metadata + .get(SKILL_LOAD_SOURCE_KEY) + .and_then(Value::as_str), + Some(SKILL_LOAD_SOURCE_PROMPT_EXPANSION) + ); + assert_eq!(event.payload["hook_event_name"], "UserPromptExpansion"); } event => panic!("unexpected event: {event:?}"), } diff --git a/crates/core/Cargo.toml b/crates/core/Cargo.toml index 13c2096ad..ccc0c2b41 100644 --- a/crates/core/Cargo.toml +++ b/crates/core/Cargo.toml @@ -99,6 +99,7 @@ object_store = { version = "0.13", default-features = false, features = ["aws"], tokio-tungstenite = { version = "0.27", default-features = false, features = ["connect", "rustls-tls-native-roots"], optional = true } tower = { version = "0.5", features = ["util"], optional = true } hyper-util = { version = "0.1", features = ["tokio"], optional = true } +shell-words = "1" [dev-dependencies] tokio = { version = "1", features = ["rt", "macros", "sync", "test-util", "rt-multi-thread", "time"] } diff --git a/crates/core/src/api/skill_load.rs b/crates/core/src/api/skill_load.rs index 196140c69..88788c752 100644 --- a/crates/core/src/api/skill_load.rs +++ b/crates/core/src/api/skill_load.rs @@ -6,36 +6,19 @@ use std::collections::HashSet; use serde_json::Value; +use strum::{EnumString, IntoStaticStr}; pub(crate) const HANDLED_METADATA_KEY: &str = "nemo_relay.skill_load_handled"; pub(crate) const PRECOMPUTED_METADATA_KEY: &str = "nemo_relay.skill_loads"; -#[derive(Debug, Clone, Copy, PartialEq, Eq)] +#[derive(Debug, Clone, Copy, PartialEq, Eq, EnumString, IntoStaticStr)] +#[strum(serialize_all = "snake_case")] pub(crate) enum SkillLoadSource { SkillTool, StructuredRead, ShellRead, } -impl SkillLoadSource { - pub(crate) const fn as_str(self) -> &'static str { - match self { - Self::SkillTool => "skill_tool", - Self::StructuredRead => "structured_read", - Self::ShellRead => "shell_read", - } - } - - fn from_str(value: &str) -> Option { - match value { - "skill_tool" => Some(Self::SkillTool), - "structured_read" => Some(Self::StructuredRead), - "shell_read" => Some(Self::ShellRead), - _ => None, - } - } -} - #[derive(Debug, Clone, PartialEq, Eq)] pub(crate) struct SkillLoad { pub(crate) name: String, @@ -46,7 +29,7 @@ pub(crate) fn detect(tool_name: &str, args: &Value) -> Vec { let normalized_tool = normalize_identifier(tool_name); let source_and_names = if matches!(normalized_tool.as_str(), "skill" | "skillview") { (SkillLoadSource::SkillTool, skill_tool_names(args)) - } else if is_structured_reader(&normalized_tool) && !has_partial_read_controls(args) { + } else if is_structured_reader(tool_name) && !has_partial_read_controls(args) { ( SkillLoadSource::StructuredRead, structured_skill_names(args), @@ -78,7 +61,7 @@ pub(crate) fn precomputed(metadata: Option<&Value>) -> Option> { .filter_map(|entry| { let entry = entry.as_object()?; let name = entry.get("skill_name")?.as_str()?.trim(); - let source = SkillLoadSource::from_str(entry.get("source")?.as_str()?)?; + let source = entry.get("source")?.as_str()?.parse().ok()?; (!name.is_empty() && seen.insert(name.to_string())).then(|| SkillLoad { name: name.to_string(), source, @@ -90,7 +73,10 @@ pub(crate) fn precomputed(metadata: Option<&Value>) -> Option> { fn skill_tool_names(args: &Value) -> Vec { let mut names = Vec::new(); - collect_named_strings(args, &mut |key, value| { + visit_named_values(args, |key, value| { + let Some(value) = value.as_str() else { + return; + }; if matches!(key.as_str(), "skill" | "skillname" | "name") { let value = value.trim(); if !value.is_empty() { @@ -103,7 +89,7 @@ fn skill_tool_names(args: &Value) -> Vec { fn structured_skill_names(args: &Value) -> Vec { let mut names = Vec::new(); - collect_named_values(args, &mut |key, value| { + visit_named_values(args, |key, value| { if matches!( key.as_str(), "path" | "filepath" | "filename" | "file" | "paths" @@ -116,8 +102,10 @@ fn structured_skill_names(args: &Value) -> Vec { fn shell_skill_names(args: &Value) -> Vec { let mut commands = Vec::new(); - collect_named_strings(args, &mut |key, value| { - if matches!(key.as_str(), "command" | "cmd") { + visit_named_values(args, |key, value| { + if matches!(key.as_str(), "command" | "cmd") + && let Some(value) = value.as_str() + { commands.push(value.to_string()); } }); @@ -129,15 +117,20 @@ fn shell_skill_names(args: &Value) -> Vec { } fn is_structured_reader(tool_name: &str) -> bool { - [ + const READERS: [&str; 5] = [ "read", "readfile", "readtextfile", "readmultiplefiles", "fileread", - ] - .iter() - .any(|reader| tool_name == *reader || tool_name.ends_with(reader)) + ]; + let segments = tool_name + .split(|character: char| !character.is_ascii_alphanumeric()) + .filter(|segment| !segment.is_empty()) + .map(normalize_identifier) + .collect::>(); + (1..=segments.len()) + .any(|length| READERS.contains(&segments[segments.len() - length..].concat().as_str())) } fn is_shell_tool(tool_name: &str) -> bool { @@ -158,20 +151,19 @@ fn is_shell_tool(tool_name: &str) -> bool { } fn has_partial_read_controls(value: &Value) -> bool { - match value { - Value::Object(object) => object.iter().any(|(key, value)| { - let key = normalize_identifier(key); - let partial = match key.as_str() { + let mut partial = false; + visit_named_values(value, |key, value| { + if !partial { + let key = normalize_identifier(&key); + partial = match key.as_str() { "offset" => value.as_i64().is_some_and(|offset| offset != 0), "limit" | "range" | "head" | "tail" | "startline" | "endline" | "linestart" | "lineend" => !value.is_null(), _ => false, }; - partial || has_partial_read_controls(value) - }), - Value::Array(values) => values.iter().any(has_partial_read_controls), - _ => false, - } + } + }); + partial } fn collect_path_skill_names(value: &Value, names: &mut Vec) { @@ -181,37 +173,26 @@ fn collect_path_skill_names(value: &Value, names: &mut Vec) { names.push(name); } } - Value::Array(values) => { - for value in values { - collect_path_skill_names(value, names); - } - } + Value::Array(values) => values + .iter() + .for_each(|value| collect_path_skill_names(value, names)), _ => {} } } -fn collect_named_strings(value: &Value, visit: &mut impl FnMut(String, &str)) { - collect_named_values(value, &mut |key, value| { - if let Some(value) = value.as_str() { - visit(key, value); - } - }); -} - -fn collect_named_values(value: &Value, visit: &mut impl FnMut(String, &Value)) { - match value { - Value::Object(object) => { - for (key, value) in object { - visit(normalize_identifier(key), value); - collect_named_values(value, visit); - } - } - Value::Array(values) => { - for value in values { - collect_named_values(value, visit); +fn visit_named_values(value: &Value, mut visit: impl FnMut(String, &Value)) { + let mut stack = vec![value]; + while let Some(value) = stack.pop() { + match value { + Value::Object(object) => { + for (key, value) in object { + visit(normalize_identifier(key), value); + stack.push(value); + } } + Value::Array(values) => stack.extend(values.iter()), + _ => {} } - _ => {} } } @@ -234,9 +215,25 @@ fn skill_name_from_path(path: &str) -> Option { } fn complete_reader_paths(command: &str) -> Vec { - let Some(words) = tokenize_simple_command(command) else { + if command.contains(['\n', '\r']) { + return Vec::new(); + } + // Preserve Windows separators: shell-words treats a lone backslash as an escape. + let escaped_windows_paths = command.replace('\\', "\\\\"); + let Ok(words) = shell_words::split(&escaped_windows_paths) else { return Vec::new(); }; + if words.is_empty() + || words.iter().any(|word| { + matches!( + word.as_str(), + "|" | "||" | "&" | "&&" | ";" | "<" | ">" | "<<" | ">>" + ) || word.contains("$(") + || word.contains('`') + }) + { + return Vec::new(); + } let Some(executable) = words.first().and_then(|word| executable_name(word)) else { return Vec::new(); }; @@ -291,59 +288,6 @@ fn powershell_content_paths(words: &[String]) -> Vec { paths } -fn tokenize_simple_command(command: &str) -> Option> { - let mut words = Vec::new(); - let mut current = String::new(); - let mut quote = None; - let mut characters = command.chars().peekable(); - - while let Some(character) = characters.next() { - match quote { - Some(active_quote) if character == active_quote => quote = None, - Some('\'') => current.push(character), - Some('"') if character == '\\' => { - if characters - .peek() - .is_some_and(|next| matches!(next, '\\' | '"' | '$' | '`' | '\n')) - { - current.push(characters.next()?); - } else { - current.push(character); - } - } - Some(_) => current.push(character), - None if matches!(character, '\'' | '"') => quote = Some(character), - None if matches!(character, '|' | '&' | ';' | '<' | '>' | '`' | '\n' | '\r') => { - return None; - } - None if character.is_whitespace() => { - if !current.is_empty() { - words.push(std::mem::take(&mut current)); - } - } - None if character == '$' && characters.peek() == Some(&'(') => return None, - None if character == '\\' => { - if characters - .peek() - .is_some_and(|next| next.is_whitespace() || matches!(next, '\\' | '\'' | '"')) - { - current.push(characters.next()?); - } else { - current.push(character); - } - } - None => current.push(character), - } - } - if quote.is_some() { - return None; - } - if !current.is_empty() { - words.push(current); - } - (!words.is_empty()).then_some(words) -} - fn executable_name(executable: &str) -> Option { executable .rsplit(['/', '\\']) diff --git a/crates/core/src/api/tool.rs b/crates/core/src/api/tool.rs index 564e31352..9ad5abbe4 100644 --- a/crates/core/src/api/tool.rs +++ b/crates/core/src/api/tool.rs @@ -237,18 +237,18 @@ fn tool_call_with_subscriber_snapshot( .and_then(|metadata| metadata.get(skill_load::HANDLED_METADATA_KEY)) .and_then(Json::as_bool) .is_some_and(|handled| handled); + let sanitized_args = NemoRelayContextState::tool_sanitize_request_snapshot_chain( + params.name, + params.args, + &entries, + ); let skill_loads = if handled_skill_loads { Vec::new() } else if let Some(skill_loads) = skill_load::precomputed(params.metadata.as_ref()) { skill_loads } else { - skill_load::detect(params.name, ¶ms.args) + skill_load::detect(params.name, &sanitized_args) }; - let sanitized_args = NemoRelayContextState::tool_sanitize_request_snapshot_chain( - params.name, - params.args, - &entries, - ); let (handle, event, marks) = { let context = global_context(); let state = context @@ -275,7 +275,7 @@ fn tool_call_with_subscriber_snapshot( .timestamp(handle.started_at) .data(json!({"skill_name": skill_load.name})) .metadata(json!({ - "skill_load_source": skill_load.source.as_str(), + "skill_load_source": <&str>::from(skill_load.source), "tool_name": handle.name, })) .build(), diff --git a/crates/core/tests/unit/skill_load_tests.rs b/crates/core/tests/unit/skill_load_tests.rs index 120b7628f..10ce0f9b2 100644 --- a/crates/core/tests/unit/skill_load_tests.rs +++ b/crates/core/tests/unit/skill_load_tests.rs @@ -145,6 +145,9 @@ fn structured_readers_reject_non_skill_paths_missing_parents_and_non_read_tools( ), ("edit_file", json!({"file_path": "/skills/review/SKILL.md"})), ("list_directory", json!({"path": "/skills/review/SKILL.md"})), + ("thread", json!({"path": "/skills/review/SKILL.md"})), + ("spread", json!({"path": "/skills/review/SKILL.md"})), + ("unread", json!({"path": "/skills/review/SKILL.md"})), ] { assert_rejected(tool, args); } diff --git a/integrations/coding-agents/codex/hooks/hooks.json b/integrations/coding-agents/codex/hooks/hooks.json index 8dcfc8906..43a79c2be 100644 --- a/integrations/coding-agents/codex/hooks/hooks.json +++ b/integrations/coding-agents/codex/hooks/hooks.json @@ -23,17 +23,6 @@ ] } ], - "UserPromptExpansion": [ - { - "hooks": [ - { - "type": "command", - "command": "nemo-relay plugin-shim hook codex", - "timeout": 30 - } - ] - } - ], "PreToolUse": [ { "matcher": "*", From 4425d55eb057353affc826202ee6b1e020ecda8a Mon Sep 17 00:00:00 2001 From: Will Killian Date: Fri, 10 Jul 2026 18:00:59 -0400 Subject: [PATCH 4/4] fix: detect skill loads before sanitizing tool args Signed-off-by: Will Killian --- crates/core/src/api/tool.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/crates/core/src/api/tool.rs b/crates/core/src/api/tool.rs index 9ad5abbe4..2655fc857 100644 --- a/crates/core/src/api/tool.rs +++ b/crates/core/src/api/tool.rs @@ -237,18 +237,18 @@ fn tool_call_with_subscriber_snapshot( .and_then(|metadata| metadata.get(skill_load::HANDLED_METADATA_KEY)) .and_then(Json::as_bool) .is_some_and(|handled| handled); - let sanitized_args = NemoRelayContextState::tool_sanitize_request_snapshot_chain( - params.name, - params.args, - &entries, - ); let skill_loads = if handled_skill_loads { Vec::new() } else if let Some(skill_loads) = skill_load::precomputed(params.metadata.as_ref()) { skill_loads } else { - skill_load::detect(params.name, &sanitized_args) + skill_load::detect(params.name, ¶ms.args) }; + let sanitized_args = NemoRelayContextState::tool_sanitize_request_snapshot_chain( + params.name, + params.args, + &entries, + ); let (handle, event, marks) = { let context = global_context(); let state = context