From 12a1423834c4fcf40eafe08cdcfb196e2ab318cd Mon Sep 17 00:00:00 2001 From: nonoqing Date: Tue, 29 Sep 2026 17:49:49 +0800 Subject: [PATCH] feat(skills): execute session-scoped command hooks --- .../external-ai-work-sources-design.md | 7 +- docs/features/agent-hooks.md | 42 ++ docs/features/agent-hooks.zh-CN.md | 32 ++ .../cargo-dependency-boundaries.mjs | 2 +- .../core-boundaries/rules/feature-rules.mjs | 7 +- src/apps/desktop/src/api/skill_api.rs | 1 + src/crates/assembly/core/AGENTS.md | 7 + .../src/agentic/session/session_manager.rs | 1 + .../core/src/agentic/tools/framework.rs | 6 + .../tools/implementations/skill_tool.rs | 6 + .../tools/implementations/skills/registry.rs | 68 +++ .../skills/registry/discovery.rs | 25 +- .../skills/registry/imports.rs | 4 + .../agentic/tools/pipeline/tool_pipeline.rs | 424 +++++++++++++++++- src/crates/assembly/core/src/native_hooks.rs | 26 +- .../core/src/native_hooks/skill_hooks.rs | 204 +++++++++ src/crates/execution/agent-runtime/AGENTS.md | 3 + src/crates/execution/agent-runtime/Cargo.toml | 3 + .../agent-runtime/src/native_hooks/engine.rs | 173 ++++++- .../agent-runtime/src/native_hooks/handler.rs | 58 +++ .../agent-runtime/src/native_hooks/kind.rs | 3 + .../agent-runtime/src/native_hooks/mod.rs | 5 +- .../agent-runtime/src/native_hooks/output.rs | 34 +- .../src/native_hooks/registry.rs | 123 +++++ .../agent-runtime/src/skills/hooks.rs | 248 ++++++++++ .../execution/agent-runtime/src/skills/mod.rs | 2 + .../agent-runtime/src/skills/types.rs | 40 +- .../skill_contracts.rs | 92 +++- .../tests/native_hook_execution_contracts.rs | 274 +++++++++++ .../skills/hooks/useInstalledSkills.test.tsx | 43 +- .../scenes/skills/hooks/useInstalledSkills.ts | 21 +- .../components/ChatContextPicker.tsx | 22 +- .../ChatContextPickerOverlay.test.tsx | 34 +- .../api/service-api/ConfigAPI.test.ts | 5 +- .../src/infrastructure/config/types/index.ts | 2 + src/web-ui/src/locales/en-US/flow-chat.json | 2 + .../src/locales/en-US/scenes/skills.json | 2 + src/web-ui/src/locales/zh-CN/flow-chat.json | 2 + .../src/locales/zh-CN/scenes/skills.json | 2 + src/web-ui/src/locales/zh-TW/flow-chat.json | 2 + .../src/locales/zh-TW/scenes/skills.json | 2 + 41 files changed, 1973 insertions(+), 86 deletions(-) create mode 100644 src/crates/assembly/core/src/native_hooks/skill_hooks.rs create mode 100644 src/crates/execution/agent-runtime/src/skills/hooks.rs diff --git a/docs/architecture/extensions/external-ai-work-sources-design.md b/docs/architecture/extensions/external-ai-work-sources-design.md index 30601b727e..a18b83dd06 100644 --- a/docs/architecture/extensions/external-ai-work-sources-design.md +++ b/docs/architecture/extensions/external-ai-work-sources-design.md @@ -479,9 +479,14 @@ Plugin Host Runtime、已退役的 LSP Runtime，以及通用动态模型路由 Claude runtime 变量与动态 shell 表达式保留为未展开、未执行的文本，加载说明明确它们不是实际值或命令结果； 如任务需要这些数据，Agent 必须通过当前工作区的正常工具与权限流程获取。行内 shell 表达式的识别要求行首或空白边界及闭合 反引号；普通 Markdown 中的感叹号、Excel `Sheet1!A1` 和错误值不会触发兼容性警告。 - `context`（包括 `fork`）、`agent`、`hooks`、`paths`、`shell`、`runtime`、`background`、`disallowed-tools` + `context`（包括 `fork`）、`agent`、`paths`、`shell`、`runtime`、`background`、`disallowed-tools` 涉及未实现的执行方式或约束，仍阻止加载；格式损坏及无效调用控制字段也仍返回错误。本地与 Remote、名称与稳定键加载共享 同一兼容性判断和模型说明。此切片不增加插件 Skill、祖先活动目录、文件 watcher、URL 来源或另一条 reload 命令。 +- Claude Skill 的同步 command `hooks` 在显式 Skill 调用时注册到会话的共享 HookRegistry，扫描与导入不执行。 + Skill 调用形成工具预检边界，同轮后续工具也接受新增 Hook；Bash/ExecCommand 与 Write 参数由方言适配转换， + ask 进入现有权限邮箱，deny/exit 2 阻断，updatedInput 继续接受原参数约束和最终校验。 + once、会话隔离、幂等注册与退出取消由现有 portable Hook 引擎持有；配置 gating 与生命周期清理在 Core owner。 + Remote 工作区激活明确不支持，其他控制面复用目标 runtime；详见 [Agent Hooks](../../features/agent-hooks.zh-CN.md)。 - Claude Subagent 扫描用户与逐层项目 `.claude/agents/**/*.md`，近工作目录定义整项覆盖；Claude MCP 保留 `local > project > user` 的整项覆盖，local 只读取与规范化当前工作区严格匹配的项目项。 - Codex Subagent 从用户与逐层项目 `[agents]`、角色文件合并，缺失字段按 Codex 层级继承；`enabled`、默认模型、角色级 diff --git a/docs/features/agent-hooks.md b/docs/features/agent-hooks.md index c499eb64ab..2da1702b2b 100644 --- a/docs/features/agent-hooks.md +++ b/docs/features/agent-hooks.md @@ -179,6 +179,48 @@ the same backend and keeps `/hooks_external` and `/hooks-external` as aliases for the unified `/hooks` management view. `reset` is available only as explicit recovery for a corrupt OpenBitFun-managed index and never changes source files. +## Hooks declared by skills + +Invoking a Claude-format skill through `Skill` registers its validated synchronous +`type: "command"` handlers in the existing Hook engine for that session. Discovery, +listing, and importing do not register or execute them. Imported Claude skills +retain their source dialect. The external Hook catalog remains read-only. + +Skill hooks use the supported lifecycle events listed above, regular-expression +matchers, stdin JSON, exit-code blocking, and `updatedInput`. They run after the +configured command layers. Registration is idempotent; invoking a changed hook +declaration in the same session returns an error instead of replacing active rules. +`once: true` is consumed after exit code 0, atomically across concurrent dispatches; +exit 2, other failures, and timeouts leave it eligible. A skill loaded mid-batch is +a preflight barrier: later calls see its hooks even within the same model response. + +The Claude adapter maps `Bash` to `ExecCommand` and `command` to `cmd`. For `Write`, +it translates the path-first `payload` into `file_path`/`content` and converts +`updatedInput` back before normal input validation. An ambiguous Write destination +is blocked while a matching skill hook is active. `Edit` keeps its existing fields. +The command receives `CLAUDE_SKILL_DIR`, `CLAUDE_SESSION_ID`, and +`CLAUDE_PROJECT_DIR`; this does not expand variables in the skill's prose. + +Skill `PreToolUse` hooks also support Claude's `permissionDecision: "ask"` through +the existing session permission mailbox. An ask requires a fresh reply even in +bypass mode; a policy deny still wins. The hook reason is included in the approval +metadata. Native `hooks.json` keeps its Codex decision contract. + +The master `app.hooks.enabled` gate applies. Project skills additionally require +`app.hooks.project_hooks_enabled`, including on subsequent dispatch. Activation +without a session/local workspace, or in an SSH/remote workspace, returns an +explicit error; no controller-local fallback executes. Remote control, Peer Device, +and Detached Dispatch reuse their target runtime's session and permission owners; +this change does not introduce a separate client-side hook runner. + +Registrations survive ordinary turns and idle in-process session unloading. Session +end/delete/discard removes them and cancels running handlers; cancelled SessionEnd +dispatch also clears them. They are process-local and are not restored after a +runtime restart: invoke the skill again. Unknown events, asynchronous handlers, +`prompt`/`agent` handlers, and unknown execution fields reject the entire skill +rather than silently dropping constraints. Script files remain live dependencies, +as for native command hooks; the declaration fingerprint does not snapshot scripts. + ## Quick start Create `/config/hooks.json`: diff --git a/docs/features/agent-hooks.zh-CN.md b/docs/features/agent-hooks.zh-CN.md index 94a90860ec..3bbe0fa99e 100644 --- a/docs/features/agent-hooks.zh-CN.md +++ b/docs/features/agent-hooks.zh-CN.md @@ -144,6 +144,38 @@ openbitfun hooks reset --confirm `/hooks_external`、`/hooks-external` 作为统一 `/hooks` 管理视图的兼容别名。 `reset` 只用于显式恢复损坏的 OpenBitFun 托管索引，绝不会修改来源文件。 +## 技能中的 Hooks + +通过 `Skill` 调用 Claude 格式技能时，经过完整校验的同步 `type: command` +处理器会注册到现有 Hook 引擎，归属于当前会话。扫描、列表和导入不注册、不执行； +导入后的 Claude 技能保留来源方言，外部 Hook 目录仍然只是只读发现数据。 + +技能使用上文已支持的生命周期事件、正则 matcher、stdin JSON、退出码阻断和 +`updatedInput`。执行顺序在配置的 command 层之后。同一会话重复加载不重复注册； +声明改变时明确报错，不静默替换已激活规则。`once: true` 仅在退出码为 0 后消费， +并发派发也只成功执行一次；退出码 2、其他失败和超时不消费。 +同一模型响应先调用技能、再调用工具时，后续工具会等技能完成后再执行 Hook 与权限预检。 + +Claude 适配将 `Bash` 匹配到 `ExecCommand`，并双向转换 `command/cmd`。 +`Write` 的路径前缀 `payload` 会转换成 `file_path/content`，返回的 `updatedInput` +再转换回本地格式并接受正常校验；有匹配的技能 Hook 时，无法确定写入目标的调用会被阻断。 +`Edit` 沿用原参数。命令环境包含 `CLAUDE_SKILL_DIR`、`CLAUDE_SESSION_ID`、 +`CLAUDE_PROJECT_DIR`；技能正文中的变量仍不会因此展开。 + +技能 `PreToolUse` 的 `permissionDecision: ask` 进入现有会话权限邮箱，携带 Hook +原因；即使启用了自动批准，也需要用户本次答复，已有权限拒绝仍然优先。 +原生 `hooks.json` 保持 Codex 决策契约。 + +激活受 `app.hooks.enabled` 控制；项目技能还要求 `app.hooks.project_hooks_enabled`， +后续派发也遵守该开关。缺少所属会话、本地工作区或处于 SSH/远程工作区时明确拒绝激活， +不会回退到控制端本地执行。远程控制、Peer Device、Detached Dispatch 复用目标运行时 +已有的会话和权限 owner，不增加客户端执行器。 + +注册跨普通回合及进程内空闲卸载保留；会话结束、删除或临时会话丢弃时清除并取消正在运行的 +处理器，SessionEnd 派发被取消时也会清理。状态只存在于运行时内存，进程重启后需要重新调用技能。 +未知事件、异步、`prompt/agent` 类型和未知执行字段会使整份技能加载失败，不会只丢弃部分约束。 +脚本文件与原生命令 Hook 一样是实时依赖，声明指纹不代表脚本快照。 + ## 快速开始 创建 `<用户配置目录>/config/hooks.json`： diff --git a/scripts/core-boundaries/cargo-dependency-boundaries.mjs b/scripts/core-boundaries/cargo-dependency-boundaries.mjs index 879a1598b1..7e9162a579 100644 --- a/scripts/core-boundaries/cargo-dependency-boundaries.mjs +++ b/scripts/core-boundaries/cargo-dependency-boundaries.mjs @@ -199,7 +199,7 @@ const CORE_TOKIO_AGGREGATES = new Set([ 'tools-mcp', ]); const AGENT_RUNTIME_TOKIO_FEATURES = new Map([ - ['native-hook-runtime', ['io-util', 'macros', 'process', 'rt', 'time']], + ['native-hook-runtime', ['io-util', 'macros', 'process', 'rt', 'sync', 'time']], ['agent-runtime', ['io-util', 'macros', 'process', 'rt', 'sync', 'time']], ]); diff --git a/scripts/core-boundaries/rules/feature-rules.mjs b/scripts/core-boundaries/rules/feature-rules.mjs index 30deecdb25..55500819a5 100644 --- a/scripts/core-boundaries/rules/feature-rules.mjs +++ b/scripts/core-boundaries/rules/feature-rules.mjs @@ -146,7 +146,7 @@ export const optionalDependencyFeatureOwnerRules = [ { depName: 'log', ownerFeatures: ['agent-runtime', 'native-hook-runtime'] }, { depName: 'regex', ownerFeatures: ['agent-runtime', 'definition-contracts', 'native-hook-settings'] }, { depName: 'serde', ownerFeatures: ['agent-runtime', 'definition-contracts', 'native-hook-runtime'] }, - { depName: 'serde_json', ownerFeatures: ['agent-runtime', 'native-hook-runtime', 'native-hook-settings'] }, + { depName: 'serde_json', ownerFeatures: ['agent-runtime', 'definition-contracts', 'native-hook-runtime', 'native-hook-settings'] }, { depName: 'serde_yaml', ownerFeatures: ['agent-runtime', 'definition-contracts'] }, { depName: 'sha2', ownerFeatures: ['agent-runtime'] }, { depName: 'thiserror', ownerFeatures: ['agent-runtime', 'definition-contracts', 'native-hook-runtime'] }, @@ -604,9 +604,12 @@ export const capabilityContractDependencyRules = [ featureProfiles: { default: [], 'definition-contracts': [ + // Skill declarations reuse pure hook validation without process support. + 'native-hook-settings', 'dep:openbitfun-core-types', 'dep:regex', 'dep:serde', + 'dep:serde_json', 'dep:serde_yaml', 'dep:thiserror', ], @@ -623,6 +626,8 @@ export const capabilityContractDependencyRules = [ 'tokio/macros', 'tokio/process', 'tokio/rt', + // Session hook once guards and cancellation watchers. + 'tokio/sync', 'tokio/time', ], 'agent-runtime': [ diff --git a/src/apps/desktop/src/api/skill_api.rs b/src/apps/desktop/src/api/skill_api.rs index 45e8e6c91a..3276924345 100644 --- a/src/apps/desktop/src/api/skill_api.rs +++ b/src/apps/desktop/src/api/skill_api.rs @@ -1604,6 +1604,7 @@ mod tests { path: "/remote/denied".into(), source_id: "codex".into(), message: "permission denied".into(), + unsupported_field: None, }, ], }; diff --git a/src/crates/assembly/core/AGENTS.md b/src/crates/assembly/core/AGENTS.md index 9db8a7dac0..de579e388e 100644 --- a/src/crates/assembly/core/AGENTS.md +++ b/src/crates/assembly/core/AGENTS.md @@ -283,6 +283,13 @@ Skill discovery, installation provenance, and local/remote registry regressions: cargo test --locked -p openbitfun-core --no-default-features --features agent-runtime,git --lib agentic::tools::implementations::skills:: ``` +Skill hook activation, session cleanup, and tool preflight/permission ordering: + +```bash +cargo test --locked -p openbitfun-core --no-default-features --features agent-runtime,git --lib native_hooks +cargo test --locked -p openbitfun-core --no-default-features --features agent-runtime,git --lib agentic::tools::pipeline::tool_pipeline::tests +``` + For configured OpenCode discovery and explicit skill loading, include their owner feature and tool tests: ```bash diff --git a/src/crates/assembly/core/src/agentic/session/session_manager.rs b/src/crates/assembly/core/src/agentic/session/session_manager.rs index 9fb4d7df91..57cf05ea25 100644 --- a/src/crates/assembly/core/src/agentic/session/session_manager.rs +++ b/src/crates/assembly/core/src/agentic/session/session_manager.rs @@ -5016,6 +5016,7 @@ impl SessionManager { } } + crate::native_hooks::clear_session_hook_state(session_id); self.active_turn_permission_modes.remove(session_id); clear_session_runtime_stores( session_id, diff --git a/src/crates/assembly/core/src/agentic/tools/framework.rs b/src/crates/assembly/core/src/agentic/tools/framework.rs index 5fb7bed2f2..4e33f76477 100644 --- a/src/crates/assembly/core/src/agentic/tools/framework.rs +++ b/src/crates/assembly/core/src/agentic/tools/framework.rs @@ -111,6 +111,12 @@ pub trait Tool: Send + Sync { self.is_readonly() } + /// Subsequent calls must be preflighted after this tool finishes because + /// it can change session execution policy (for example, activate hooks). + fn invalidates_tool_preflight(&self) -> bool { + false + } + /// Describe permission actions and resources without performing side effects. fn permission_intents( &self, diff --git a/src/crates/assembly/core/src/agentic/tools/implementations/skill_tool.rs b/src/crates/assembly/core/src/agentic/tools/implementations/skill_tool.rs index 2f3fb10b4f..d8aa8512b3 100644 --- a/src/crates/assembly/core/src/agentic/tools/implementations/skill_tool.rs +++ b/src/crates/assembly/core/src/agentic/tools/implementations/skill_tool.rs @@ -153,6 +153,10 @@ impl Tool for SkillTool { } fn is_concurrency_safe(&self, _input: Option<&Value>) -> bool { + false + } + + fn invalidates_tool_preflight(&self) -> bool { true } @@ -299,6 +303,8 @@ impl Tool for SkillTool { } }; + crate::native_hooks::activate_skill_hooks(&skill_data, context).await?; + if let Some(arguments) = input.get("arguments").and_then(Value::as_str) { skill_data.content = expand_prompt_template_arguments_with_names( &skill_data.content, diff --git a/src/crates/assembly/core/src/agentic/tools/implementations/skills/registry.rs b/src/crates/assembly/core/src/agentic/tools/implementations/skills/registry.rs index b03f91bdfe..502ba37783 100644 --- a/src/crates/assembly/core/src/agentic/tools/implementations/skills/registry.rs +++ b/src/crates/assembly/core/src/agentic/tools/implementations/skills/registry.rs @@ -308,6 +308,44 @@ mod local_skill_scan_tests { ); } + #[tokio::test] + async fn claude_scan_distinguishes_unsupported_fields_from_invalid_markdown() { + let temp = tempfile::tempdir().unwrap(); + let root_path = temp.path().join("skills"); + write_skill(&root_path.join("good")); + for (name, markdown) in [ + ( + "guard", + "---\nname: guard\ndescription: Guard tools.\ncontext: fork\n---\n", + ), + ("broken", "---\nname: broken\n---\n"), + ] { + let directory = root_path.join(name); + fs::create_dir_all(&directory).unwrap(); + fs::write(directory.join("SKILL.md"), markdown).unwrap(); + } + let mut root = test_root(root_path); + root.slot = "home.claude"; + root.source_id = "claude-code"; + let scan = SkillRegistry::scan_skills_in_dir(&root).await; + assert_eq!(scan.candidates.len(), 1); + assert_eq!(scan.diagnostics.len(), 2); + let unsupported = scan + .diagnostics + .iter() + .find(|item| item.unsupported_field.is_some()) + .unwrap(); + assert_eq!(unsupported.unsupported_field.as_deref(), Some("context")); + assert!(unsupported.path.ends_with("guard/SKILL.md")); + let failure = scan + .diagnostics + .iter() + .find(|item| item.unsupported_field.is_none()) + .unwrap(); + assert!(failure.path.ends_with("broken/SKILL.md")); + assert!(failure.message.contains("description")); + } + #[tokio::test] async fn claude_config_root_keeps_source_identity_and_rejects_relative_roots() { let temp = tempfile::tempdir().unwrap(); @@ -1116,6 +1154,7 @@ impl SkillRegistry { path, source_id: "pi".into(), message, + unsupported_field: None, } })); let pi_scan = @@ -1133,6 +1172,7 @@ impl SkillRegistry { path, source_id: "opencode".to_string(), message, + unsupported_field: None, } })); let configured_scan = @@ -2501,6 +2541,7 @@ mod remote_scan_tests { calls: AtomicUsize, installation_lock: Option, pi_settings: Option, + claude_hooks: bool, } impl DelayedFs { @@ -2537,6 +2578,11 @@ mod remote_scan_tests { if path.ends_with("openai.yaml") { return Ok("policy:\n allow_implicit_invocation: false\n".into()); } + if self.claude_hooks && path.ends_with("/.claude/skills/skill-00/SKILL.md") { + return Ok( + "---\nname: skill-00\ndescription: Guard tools.\ncontext: fork\n---\n".into(), + ); + } let name = path.rsplit('/').nth(1).unwrap(); Ok(format!( "---\nname: {name}\ndescription: {path}\n---\nBody\n" @@ -2562,6 +2608,7 @@ mod remote_scan_tests { self.round_trip().await; Ok(path.contains("/.openbitfun/") || path.contains("/.codex/") + || (self.claude_hooks && path.contains("/.claude/")) || (self.installation_lock.is_some() && path.contains("/.agents/"))) } async fn read_dir(&self, path: &str) -> anyhow::Result> { @@ -2579,6 +2626,27 @@ mod remote_scan_tests { } } + #[tokio::test] + async fn remote_claude_scan_reports_unsupported_fields_without_loading_the_skill() { + let fs = DelayedFs { + claude_hooks: true, + ..Default::default() + }; + let scan = SkillRegistry::scan_remote_project_skills(&fs, "/remote/project").await; + assert_eq!(scan.candidates.len(), 38); + assert_eq!(scan.diagnostics.len(), 1); + let diagnostic = &scan.diagnostics[0]; + assert_eq!( + diagnostic.path, + "/remote/project/.claude/skills/skill-00/SKILL.md" + ); + assert_eq!(diagnostic.unsupported_field.as_deref(), Some("context")); + assert!(!scan + .candidates + .iter() + .any(|candidate| candidate.info.path == "/remote/project/.claude/skills/skill-00")); + } + #[tokio::test] async fn remote_pi_settings_paths_report_unsupported_without_local_substitution() { let fs = DelayedFs { diff --git a/src/crates/assembly/core/src/agentic/tools/implementations/skills/registry/discovery.rs b/src/crates/assembly/core/src/agentic/tools/implementations/skills/registry/discovery.rs index e7cdc8dd1a..ad44bbc927 100644 --- a/src/crates/assembly/core/src/agentic/tools/implementations/skills/registry/discovery.rs +++ b/src/crates/assembly/core/src/agentic/tools/implementations/skills/registry/discovery.rs @@ -18,6 +18,7 @@ pub(super) fn diagnostic( path: path.into(), source_id: source_id.into(), message: message.to_string(), + unsupported_field: None, } } @@ -482,11 +483,13 @@ impl SkillRegistry { candidate.info.import_origin = import_origin; scan.candidates.push(candidate); } - Err(error) => scan.diagnostics.push(diagnostic( - &skill_md, - entry.source_id, - error, - )), + Err(error) => scan.diagnostics.push( + SkillScanDiagnostic::from_parse_error( + &skill_md, + entry.source_id, + &error, + ), + ), } } Ok(None) => scan.diagnostics.push(diagnostic( @@ -811,11 +814,13 @@ impl SkillRegistry { candidate.info.import_origin = import_origin; scan.candidates.push(candidate); } - Err(error) => scan.diagnostics.push(diagnostic( - skill_md.to_string_lossy(), - entry.source_id, - error, - )), + Err(error) => { + scan.diagnostics.push(SkillScanDiagnostic::from_parse_error( + skill_md.to_string_lossy(), + entry.source_id, + &error, + )) + } } } // A skill is a package boundary; its reference examples diff --git a/src/crates/assembly/core/src/agentic/tools/implementations/skills/registry/imports.rs b/src/crates/assembly/core/src/agentic/tools/implementations/skills/registry/imports.rs index 38ab93ae08..8ae7ceffb5 100644 --- a/src/crates/assembly/core/src/agentic/tools/implementations/skills/registry/imports.rs +++ b/src/crates/assembly/core/src/agentic/tools/implementations/skills/registry/imports.rs @@ -576,6 +576,9 @@ mod tests { async fn imported_package_preserves_dialect_assets_identity_and_runtime_selection() { let temp = tempfile::tempdir().unwrap(); let source = source(temp.path()).await; + let skill_file = Path::new(&source.path).join("SKILL.md"); + let markdown = fs::read_to_string(&skill_file).await.unwrap(); + fs::write(&skill_file, markdown.replacen("---", "---\nhooks:\n PreToolUse:\n - matcher: Bash\n hooks:\n - type: command\n command: exit 2", 1)).await.unwrap(); let target = temp.path().join("native"); let origin = import_copy(source.clone(), target.clone()).await.unwrap(); let native = SkillRegistry::scan_skills_in_dir(&root( @@ -601,6 +604,7 @@ mod tests { native.info.parser_source_slot(), ) .unwrap(); + assert!(loaded.hooks.as_ref().is_some_and(|hooks| !hooks.is_empty())); assert_eq!(loaded.name, "demo"); assert!(content.contains("scripts/tool.py")); assert!(target.join("demo/scripts/tool.py").is_file()); diff --git a/src/crates/assembly/core/src/agentic/tools/pipeline/tool_pipeline.rs b/src/crates/assembly/core/src/agentic/tools/pipeline/tool_pipeline.rs index d82123993e..646bf67878 100644 --- a/src/crates/assembly/core/src/agentic/tools/pipeline/tool_pipeline.rs +++ b/src/crates/assembly/core/src/agentic/tools/pipeline/tool_pipeline.rs @@ -664,6 +664,7 @@ pub struct ToolPipeline { /// Tool task ids a PreToolUse hook approved. The approval waives the /// interactive permission prompt only; policy denials still apply. hook_preapprovals: Arc>>, + hook_asks: Arc>>, } impl ToolPipeline { @@ -680,6 +681,7 @@ impl ToolPipeline { permission_request_manager: None, permission_plans: Arc::new(TokioMutex::new(HashMap::new())), hook_preapprovals: Arc::new(TokioMutex::new(HashSet::new())), + hook_asks: Arc::new(TokioMutex::new(HashMap::new())), } } @@ -702,9 +704,23 @@ impl ToolPipeline { intents: Vec, context: ToolUseContext, ) -> OpenBitFunResult { + let hook_ask = self + .hook_asks + .lock() + .await + .get(&task.tool_call.tool_id) + .cloned(); + let mut intents = intents; if intents.is_empty() { - return Ok(PermissionPlanDraft::Allowed); + if hook_ask.is_none() { + return Ok(PermissionPlanDraft::Allowed); + } + intents.push(PermissionIntent::new( + "custom_tool", + vec![tool_name.clone()], + )); } + let forced_intents = hook_ask.as_ref().map(|_| intents.clone()); let (project_id, project_path) = permission_scope(&context, &intents)?; let permission_policy = task.options.permission_policy.clone(); @@ -729,7 +745,10 @@ impl ToolPipeline { }; let asks = match plan_permission_intents(intents, &permission_policy, &grants, case_sensitivity) { - PermissionIntentPlan::Allowed => return Ok(PermissionPlanDraft::Allowed), + PermissionIntentPlan::Allowed => match forced_intents { + Some(intents) => intents, + None => return Ok(PermissionPlanDraft::Allowed), + }, PermissionIntentPlan::Denied(intent) => { return Ok(PermissionPlanDraft::Rejected { reason: format!( @@ -745,13 +764,12 @@ impl ToolPipeline { // A PreToolUse hook already approved this call. The approval reaches // here — after policy evaluation — precisely so that it waives only // the interactive prompt: a policy Deny above has already returned. - if self.hook_preapprovals.lock().await.contains(&tool_call_id) { + if hook_ask.is_none() && self.hook_preapprovals.lock().await.contains(&tool_call_id) { return Ok(PermissionPlanDraft::Allowed); } - // The tool call would prompt the user: give PermissionRequest hooks - // a chance to decide first. An explicit hook decision replaces the - // interactive prompt for this invocation. + // PermissionRequest denials remain authoritative even when a skill + // requires a fresh user reply. An allow cannot waive that explicit ask. if let Some(hook_decision) = native_hooks::dispatch_permission_request( native_hook_session_facts(&task.context, &task.options), &tool_name, @@ -760,20 +778,15 @@ impl ToolPipeline { .await { if hook_decision.allow { - info!( - "PermissionRequest hook allowed tool call without prompting: tool_name={}", - tool_name - ); - return Ok(PermissionPlanDraft::Allowed); + if hook_ask.is_none() { + return Ok(PermissionPlanDraft::Allowed); + } + } else { + let reason = hook_decision.message.unwrap_or_else(|| { + format!("A PermissionRequest hook denied the '{tool_name}' tool call.") + }); + return Ok(PermissionPlanDraft::Rejected { reason }); } - let reason = hook_decision.message.unwrap_or_else(|| { - format!("A PermissionRequest hook denied the '{tool_name}' tool call.") - }); - info!( - "PermissionRequest hook denied tool call: tool_name={}", - tool_name - ); - return Ok(PermissionPlanDraft::Rejected { reason }); } if manager.is_none() { @@ -801,7 +814,20 @@ impl ToolPipeline { identity: tool_name.clone(), }, delegation: permission_delegation.clone(), - display_metadata: intent.display_metadata, + display_metadata: { + let mut metadata = intent.display_metadata; + if let Some(reason) = &hook_ask { + metadata.insert( + "riskDescription".into(), + serde_json::Value::String(reason.clone()), + ); + metadata.insert( + "requiresFreshApproval".into(), + serde_json::Value::Bool(true), + ); + } + metadata + }, }) .collect(); @@ -1000,6 +1026,8 @@ impl ToolPipeline { task_id.clone(), PermissionExecutionPlan::Rejected { reason }, ); + } else if let Some(reason) = decision.ask_reason { + self.hook_asks.lock().await.insert(task_id.clone(), reason); } else if decision.allow { // A hook approval only waives the interactive prompt. It is // recorded for the planner rather than short-circuiting it, @@ -1250,9 +1278,47 @@ impl ToolPipeline { "Permission batch lost its owning Dialog Turn".to_string(), ) })?; - let receivers = self - .register_permission_requests(batch_requests, &dialog_turn_id, auto_approve) - .await?; + // A skill's explicit ask must survive bypass mode. Keep other + // requests' existing auto-approval policy and original ordering. + let forced = self.hook_asks.lock().await.clone(); + let mut receivers = Vec::with_capacity(batch_requests.len()); + let mut groups: Vec<(bool, Vec)> = Vec::new(); + for request in batch_requests { + let approve = auto_approve + && !request + .tool_call_id + .as_ref() + .is_some_and(|id| forced.contains_key(id)); + if let Some((_, requests)) = groups + .last_mut() + .filter(|(previous, _)| *previous == approve) + { + requests.push(request); + } else { + groups.push((approve, vec![request])); + } + } + for (approve, requests) in groups { + match self + .register_permission_requests(requests, &dialog_turn_id, approve) + .await + { + Ok(group) => receivers.extend(group), + Err(error) => { + self.cancel_permission_request_ids( + receivers + .into_iter() + .map(|pending: PendingPermissionReceiver| { + pending.request_id().to_string() + }) + .collect(), + "Permission registration failed".into(), + ) + .await; + return Err(error); + } + } + } let mut receivers_by_task = HashMap::>::new(); for ((task_id, _), receiver) in ordered_requests.into_iter().zip(receivers) { @@ -1424,6 +1490,12 @@ impl ToolPipeline { preapprovals.remove(task_id); } } + { + let mut asks = self.hook_asks.lock().await; + for task_id in task_ids { + asks.remove(task_id); + } + } for task_id in task_ids { let Some(plan) = self.permission_plans.lock().await.remove(task_id) else { continue; @@ -1466,7 +1538,12 @@ impl ToolPipeline { self.register_permission_requests( requests, &task.context.dialog_turn_id, - task.options.auto_approve_ask, + task.options.auto_approve_ask + && !self + .hook_asks + .lock() + .await + .contains_key(&task.tool_call.tool_id), ) .await?, ), @@ -1643,6 +1720,63 @@ impl ToolPipeline { task_ids.push(tool_id); } + // A policy-changing call closes the preflight segment. Later tools + // are validated only after its new session hooks become visible. + let mut segments = Vec::new(); + let mut segment = Vec::new(); + { + let registry = self.tool_registry.read().await; + for (task_id, name) in task_ids.iter().zip(&tool_names) { + segment.push(task_id.clone()); + if registry + .get_tool(name) + .is_some_and(|tool| tool.invalidates_tool_preflight()) + { + segments.push(std::mem::take(&mut segment)); + } + } + } + if !segment.is_empty() { + segments.push(segment); + } + let mut results = Vec::with_capacity(task_ids.len()); + let mut segments = segments.into_iter(); + while let Some(segment) = segments.next() { + if self.should_interrupt_for_round_injection(&context) { + results.extend( + self.build_steering_interrupted_results( + segment.into_iter().chain(segments.flatten()), + ) + .await, + ); + break; + } + match self + .execute_preflight_segment(segment, &options, subagent_call_count) + .await + { + Ok(segment_results) => results.extend(segment_results), + Err(error) => { + self.cleanup_permission_plans(&task_ids, "Tool execution failed".into()) + .await; + return Err(error); + } + } + } + Ok(results) + } + + async fn execute_preflight_segment( + &self, + task_ids: Vec, + options: &ToolExecutionOptions, + subagent_call_count: usize, + ) -> OpenBitFunResult> { + let tool_names = task_ids + .iter() + .filter_map(|id| self.state_manager.get_task(id)) + .map(|task| task.invocation.effective_tool_name) + .collect::>(); // PreToolUse hooks run before permission planning so a hook decision // (deny / pre-approve / rewritten input) is visible to the planner // and no permission prompt is raised for calls a hook already decided. @@ -3046,6 +3180,248 @@ mod tests { round_injection_yieldable: bool, } + #[cfg(unix)] + struct HookActivatingTestTool { + skill: openbitfun_agent_runtime::skills::SkillData, + } + + #[cfg(unix)] + #[async_trait] + impl Tool for HookActivatingTestTool { + fn name(&self) -> &str { + "ActivateSkillHooks" + } + async fn description(&self) -> OpenBitFunResult { + Ok("Activate test skill".into()) + } + fn short_description(&self) -> String { + "Activate test skill".into() + } + fn input_schema(&self) -> serde_json::Value { + json!({"type":"object"}) + } + fn is_readonly(&self) -> bool { + true + } + fn is_concurrency_safe(&self, _: Option<&serde_json::Value>) -> bool { + false + } + fn invalidates_tool_preflight(&self) -> bool { + true + } + async fn call_impl( + &self, + _: &serde_json::Value, + context: &ToolUseContext, + ) -> OpenBitFunResult> { + native_hooks::activate_skill_hooks(&self.skill, context).await?; + Ok(vec![ToolResult::Result { + data: json!({"loaded":true}), + result_for_assistant: None, + image_attachments: None, + }]) + } + } + + #[cfg(unix)] + fn test_skill_hooks(command: &str) -> openbitfun_agent_runtime::skills::SkillData { + use openbitfun_agent_runtime::skills::{SkillData, SkillLocation}; + let mut skill = SkillData::from_markdown_for_source_slot( + "/skills/test".into(), + "---\nname: test\ndescription: Test skill\n---\nTest", + SkillLocation::User, + true, + "claude", + ) + .unwrap(); + skill.key = "user::claude::test".into(); + skill.hooks=Some(openbitfun_agent_runtime::skills::SkillHooks::from_yaml(&serde_yaml::to_value(json!({"PreToolUse":[{"matcher":"Capture","hooks":[{"type":"command","command":command}]}]})).unwrap()).unwrap()); + skill + } + + #[cfg(unix)] + struct ClearTestHooks(String); + #[cfg(unix)] + impl Drop for ClearTestHooks { + fn drop(&mut self) { + native_hooks::clear_session_hook_state(&self.0); + } + } + + #[cfg(unix)] + #[tokio::test] + async fn skill_hooks_apply_to_later_tools_in_the_same_round_and_persist() { + for parallel in [false, true] { + let temp = tempfile::tempdir().unwrap(); + let mut context = permission_test_context(); + context.workspace = Some(WorkspaceBinding::new(None, temp.path().into())); + context.session_id = uuid::Uuid::new_v4().to_string(); + let _clear = ClearTestHooks(context.session_id.clone()); + let pipeline = test_tool_pipeline(); + let captured = Arc::new(Mutex::new(None)); + register_capturing_test_tool(&pipeline, "Capture", captured.clone()).await; + pipeline + .tool_registry + .write() + .await + .register_tool(Arc::new(HookActivatingTestTool { + skill: test_skill_hooks("echo skill-blocked >&2; exit 2"), + })); + let mut capture = test_tool_call("after", "Capture"); + capture.arguments = json!({"city":"safe"}); + let mut before = capture.clone(); + before.tool_id = "before".into(); + let mut options = ToolExecutionOptions::default(); + options.allow_parallel = parallel; + let result = pipeline + .execute_tools( + vec![ + before, + test_tool_call("activate", "ActivateSkillHooks"), + capture.clone(), + ], + context.clone(), + options.clone(), + ) + .await + .unwrap(); + assert!(!result[0].result.is_error); + assert!(!result[1].result.is_error, "{:?}", result[1]); + assert_eq!(result[2].result.result["category"], "permission_denied"); + assert!(result[2] + .result + .result + .to_string() + .contains("skill-blocked")); + *captured.lock().unwrap() = None; + capture.tool_id = "next-round".into(); + assert_eq!( + pipeline + .execute_tools(vec![capture.clone()], context.clone(), options.clone()) + .await + .unwrap()[0] + .result + .result["category"], + "permission_denied" + ); + assert!(captured.lock().unwrap().is_none()); + native_hooks::clear_session_hook_state(&context.session_id); + capture.tool_id = "after-close".into(); + assert!( + !pipeline + .execute_tools(vec![capture], context, options) + .await + .unwrap()[0] + .result + .is_error + ); + } + } + + #[cfg(unix)] + #[tokio::test] + async fn skill_hook_ask_requires_user_reply_even_with_allow_and_bypass() { + let temp = tempfile::tempdir().unwrap(); + let mut context = permission_test_context(); + context.workspace = Some(WorkspaceBinding::new(None, temp.path().into())); + context.session_id = uuid::Uuid::new_v4().to_string(); + let _clear = ClearTestHooks(context.session_id.clone()); + let store = Arc::new(MemoryPermissionStore::default()); + let manager = permission_test_manager(store); + let pipeline = test_tool_pipeline().with_permission_request_manager(manager.clone()); + let captured = Arc::new(Mutex::new(None)); + register_capturing_test_tool(&pipeline, "Capture", captured.clone()).await; + let skill = test_skill_hooks( + r#"printf '%s' '{"hookSpecificOutput":{"permissionDecision":"ask","permissionDecisionReason":"review this call"}}'"#, + ); + let task = ToolTask::new( + test_tool_call("activation", "Capture"), + context.clone(), + ToolExecutionOptions::default(), + ); + native_hooks::activate_skill_hooks( + &skill, + &pipeline.build_tool_use_context(&task, CancellationToken::new()), + ) + .await + .unwrap(); + let mut call = test_tool_call("asked", "Capture"); + call.arguments = json!({"city":"safe"}); + let mut options = ToolExecutionOptions::default(); + options.auto_approve_ask = true; + options.permission_policy = ResolvedPermissionPolicy::new( + vec![PermissionRule::new( + "custom_tool", + "*", + PermissionEffect::Allow, + )], + Vec::new(), + ); + let running = pipeline.clone(); + let run_context = context.clone(); + let execution = tokio::spawn(async move { + running + .execute_tools(vec![call], run_context, options) + .await + }); + let request = wait_for_permission_request(&manager).await; + assert_eq!(request.session_id, context.session_id); + assert_eq!( + request.display_metadata["riskDescription"], + "review this call" + ); + assert!(captured.lock().unwrap().is_none()); + pipeline + .reply_to_tool("asked", PermissionReply::Reject { feedback: None }) + .await + .unwrap(); + assert_eq!( + execution.await.unwrap().unwrap()[0].result.result["category"], + "user_rejected" + ); + assert!(captured.lock().unwrap().is_none()); + assert!(pipeline.hook_asks.lock().await.is_empty()); + // A deny remains stronger than a hook's request for approval. + let mut denied = test_tool_call("policy-denied", "Capture"); + denied.arguments = json!({"city":"safe"}); + let mut options = ToolExecutionOptions::default(); + options.permission_policy = ResolvedPermissionPolicy::new( + vec![PermissionRule::new( + "custom_tool", + "*", + PermissionEffect::Deny, + )], + Vec::new(), + ); + assert_eq!( + pipeline + .execute_tools(vec![denied], context.clone(), options) + .await + .unwrap()[0] + .result + .result["category"], + "permission_denied" + ); + assert!(manager.pending_requests().is_empty()); + let veto = openbitfun_agent_runtime::skills::SkillHooks::from_yaml(&serde_yaml::from_str("PermissionRequest: [{matcher: Capture, hooks: [{type: command, command: 'echo approval-veto >&2; exit 2'}]}]").unwrap()).unwrap(); + native_hooks::runtime_hook_registry() + .register_session_skill( + &context.session_id, + "veto", + veto.fingerprint(), + veto.registrations(&context.session_id, "veto", "/", None, "/"), + ) + .unwrap(); + let mut vetoed = test_tool_call("vetoed", "Capture"); + vetoed.arguments = json!({"city":"safe"}); + let result = pipeline + .execute_tools(vec![vetoed], context, ToolExecutionOptions::default()) + .await + .unwrap(); + assert_eq!(result[0].result.result["reason"], "approval-veto"); + assert!(manager.pending_requests().is_empty()); + } + struct CapturingTestTool { name: String, received_arguments: Arc>>, diff --git a/src/crates/assembly/core/src/native_hooks.rs b/src/crates/assembly/core/src/native_hooks.rs index 5672b6bb3d..ea3ff71ea2 100644 --- a/src/crates/assembly/core/src/native_hooks.rs +++ b/src/crates/assembly/core/src/native_hooks.rs @@ -44,6 +44,9 @@ use std::sync::{Arc, OnceLock}; const MAX_CACHED_WORKSPACE_HOOK_SOURCES: usize = 32; const MAX_PENDING_CONTEXT_SESSIONS: usize = 1024; +mod skill_hooks; +pub(crate) use skill_hooks::activate_skill_hooks; + pub(crate) fn new_runtime_hook_registry() -> RuntimeHookRegistry { runtime_hook_registry() } @@ -341,6 +344,8 @@ pub struct UserPromptSubmitHookDecision { pub struct PreToolUseHookDecision { /// The tool call must not run; the reason is fed back to the model. pub deny_reason: Option, + /// Skill hook requests an explicit decision through the permission mailbox. + pub ask_reason: Option, /// The tool call bypasses the permission prompt for this invocation. pub allow: bool, /// Replacement tool arguments (`hookSpecificOutput.updatedInput`). @@ -445,6 +450,11 @@ pub async fn dispatch_pre_tool_use( Some(AgentHookPermissionOutcome::Allow { .. }) => { decision.allow = true; } + Some(AgentHookPermissionOutcome::Ask { reason }) => { + decision.ask_reason = Some(reason.clone().unwrap_or_else(|| { + format!("A skill hook requires approval for the '{tool_name}' tool call.") + })); + } None => {} } if decision.deny_reason.is_none() { @@ -496,7 +506,7 @@ pub async fn dispatch_permission_request( allow: true, message: reason, }), - None => None, + None | Some(AgentHookPermissionOutcome::Ask { .. }) => None, } } @@ -609,6 +619,14 @@ pub async fn dispatch_stop( /// SessionEnd hooks run when a session is deleted (`reason: "other"`). /// Timeouts are capped tightly so deletion never hangs. pub async fn dispatch_session_end(facts: NativeHookSessionFacts<'_>, reason: &str) { + // Also clear on cancellation while waiting for SessionEnd commands. + struct ClearOnExit<'a>(&'a str); + impl Drop for ClearOnExit<'_> { + fn drop(&mut self) { + clear_session_hook_state(self.0); + } + } + let _clear = ClearOnExit(facts.session_id); pending_session_context().remove(facts.session_id); if let Some(dispatch) = prepare(facts, AgentHookEvent::SessionEnd).await { dispatch @@ -622,6 +640,7 @@ pub async fn dispatch_session_end(facts: NativeHookSessionFacts<'_>, reason: &st /// Drop per-session hook state without dispatching anything. pub fn clear_session_hook_state(session_id: &str) { pending_session_context().remove(session_id); + runtime_hook_registry().clear_session(session_id); } /// Built-in DeepReview shared-context measurement hook. @@ -876,9 +895,10 @@ async fn prepare<'a>( facts.workspace_root, config.project_hooks_enabled, ) - .await?; + .await? + .with_project_hooks_enabled(config.project_hooks_enabled); let workspace_scope = facts.workspace_id.map(str::to_owned); - if !engine.has_rules_for_workspace(event, workspace_scope.as_deref()) { + if !engine.has_rules_for_session(event, workspace_scope.as_deref(), facts.session_id) { return None; } let cwd = facts diff --git a/src/crates/assembly/core/src/native_hooks/skill_hooks.rs b/src/crates/assembly/core/src/native_hooks/skill_hooks.rs new file mode 100644 index 0000000000..09b2849a88 --- /dev/null +++ b/src/crates/assembly/core/src/native_hooks/skill_hooks.rs @@ -0,0 +1,204 @@ +//! Skill invocation only activates validated commands in the owning session. + +use super::{hooks_config, runtime_hook_registry, AgentHooksConfig}; +use crate::agentic::tools::framework::ToolUseContext; +use crate::util::errors::{OpenBitFunError, OpenBitFunResult}; +use openbitfun_agent_runtime::native_hooks::RuntimeHookRegistry; +use openbitfun_agent_runtime::skills::{SkillData, SkillLocation}; + +pub(crate) async fn activate_skill_hooks( + skill: &SkillData, + context: &ToolUseContext, +) -> OpenBitFunResult<()> { + activate( + &runtime_hook_registry(), + &hooks_config().await, + skill, + context, + ) +} + +fn activate( + registry: &RuntimeHookRegistry, + config: &AgentHooksConfig, + skill: &SkillData, + context: &ToolUseContext, +) -> OpenBitFunResult<()> { + let Some(hooks) = skill.hooks.as_ref().filter(|hooks| !hooks.is_empty()) else { + return Ok(()); + }; + let reject = |reason: &str| { + OpenBitFunError::tool(format!( + "Cannot activate skill '{}' hooks: {reason}", + skill.name + )) + }; + if context.is_remote() { + return Err(reject( + "command hooks are unsupported in remote workspaces; no local command was executed", + )); + } + if !config.enabled { + return Err(reject("app.hooks.enabled is disabled")); + } + if skill.location == SkillLocation::Project && !config.project_hooks_enabled { + return Err(reject( + "project skill commands require app.hooks.project_hooks_enabled", + )); + } + let session_id = context + .session_id + .as_deref() + .filter(|id| !id.is_empty()) + .ok_or_else(|| reject("an owning session is required"))?; + let root = context + .workspace_root() + .ok_or_else(|| reject("an owning local workspace is required"))?; + let root = root.to_string_lossy(); + let skill_path = skill.path.as_str(); + let fingerprint = serde_json::json!([ + hooks.fingerprint(), + skill_path, + context.workspace_id(), + root + ]) + .to_string(); + let mut registrations = hooks.registrations( + session_id, + &skill.key, + skill_path, + context.workspace_id(), + &root, + ); + for registration in &mut registrations { + registration.requires_project_trust = skill.location == SkillLocation::Project; + } + registry + .register_session_skill(session_id, &skill.key, &fingerprint, registrations) + .map_err(|error| reject(&error.to_string()))?; + Ok(()) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::agentic::workspace::WorkspaceBinding; + use openbitfun_agent_runtime::native_hooks::{AgentHookEngine, AgentHookEvent}; + + fn guarded_skill() -> SkillData { + let mut skill = SkillData::from_markdown_for_source_slot("/skills/guard".into(), + "---\nname: guard\ndescription: Guard tools\nhooks:\n PreToolUse:\n - matcher: Edit\n hooks:\n - type: command\n command: exit 2\n---\nGuard edits.\n", SkillLocation::User, true, "claude").unwrap(); + skill.key = "user::claude::guard".into(); + skill + } + + #[test] + fn activation_gates_and_idempotence_are_session_owned() { + let root = tempfile::tempdir().unwrap(); + let registry = RuntimeHookRegistry::default(); + let engine = AgentHookEngine::with_registry(registry.clone()); + let mut context = ToolUseContext::for_tool_listing( + Some(WorkspaceBinding::new(Some("w".into()), root.path().into())), + None, + ); + let mut skill = guarded_skill(); + assert!( + activate(®istry, &AgentHooksConfig::default(), &skill, &context) + .unwrap_err() + .to_string() + .contains("session") + ); + context.session_id = Some("skill-session".into()); + assert!(!engine.has_rules_for_session( + AgentHookEvent::PreToolUse, + Some("w"), + "skill-session" + )); + let mut config = AgentHooksConfig::default(); + config.enabled = false; + assert!(activate(®istry, &config, &skill, &context).is_err()); + config.enabled = true; + skill.location = SkillLocation::Project; + assert!(activate(®istry, &config, &skill, &context).is_err()); + config.project_hooks_enabled = true; + activate(®istry, &config, &skill, &context).unwrap(); + activate(®istry, &config, &skill, &context).unwrap(); + assert!(engine.has_rules_for_session( + AgentHookEvent::PreToolUse, + Some("w"), + "skill-session" + )); + assert!(!engine.has_rules_for_session( + AgentHookEvent::PreToolUse, + Some("w"), + "other-session" + )); + registry.clear_session("skill-session"); + assert!(!engine.has_rules_for_session( + AgentHookEvent::PreToolUse, + Some("w"), + "skill-session" + )); + let identity = crate::service::remote_ssh::workspace_state::workspace_session_identity( + "/remote/project", + Some("connection"), + Some("host"), + ) + .unwrap(); + context.workspace = Some(WorkspaceBinding::new_remote( + None, + "/remote/project".into(), + "connection".into(), + "Remote".into(), + identity, + )); + assert!(activate(®istry, &config, &skill, &context) + .unwrap_err() + .to_string() + .contains("remote workspaces")); + } + + #[cfg(unix)] + #[tokio::test] + async fn session_end_runs_registered_hooks_then_clears_state() { + let temp = tempfile::tempdir().unwrap(); + let session = uuid::Uuid::new_v4().to_string(); + let hooks = openbitfun_agent_runtime::skills::SkillHooks::from_yaml( + &serde_yaml::from_str( + "SessionEnd: [{hooks: [{type: command, command: 'touch ended'}]}]", + ) + .unwrap(), + ) + .unwrap(); + let registry = runtime_hook_registry(); + registry + .register_session_skill( + &session, + "end", + hooks.fingerprint(), + hooks.registrations(&session, "end", "/", None, "/"), + ) + .unwrap(); + super::super::dispatch_session_end( + super::super::NativeHookSessionFacts { + workspace_id: None, + session_id: &session, + turn_id: None, + workspace_root: Some(temp.path()), + is_remote_workspace: false, + model: "test", + bypass_permissions: false, + }, + "other", + ) + .await; + assert!(temp.path().join("ended").exists()); + assert!( + !AgentHookEngine::with_registry(registry).has_rules_for_session( + AgentHookEvent::SessionEnd, + None, + &session + ) + ); + } +} diff --git a/src/crates/execution/agent-runtime/AGENTS.md b/src/crates/execution/agent-runtime/AGENTS.md index 2023269b29..77d7f382d2 100644 --- a/src/crates/execution/agent-runtime/AGENTS.md +++ b/src/crates/execution/agent-runtime/AGENTS.md @@ -9,6 +9,9 @@ port-backed `sdk` / `AgentRuntime` facade that can be built and tested without ## Feature Boundaries +- `definition-contracts` includes pure `native-hook-settings` validation for + skill frontmatter. Discovery does not select process execution or Tokio. + - `native-hook-settings` exposes Codex-compatible hook settings parsing and validation without process execution. - `native-hook-runtime` extends settings with payload, output, and managed diff --git a/src/crates/execution/agent-runtime/Cargo.toml b/src/crates/execution/agent-runtime/Cargo.toml index 655f77b920..594e774392 100644 --- a/src/crates/execution/agent-runtime/Cargo.toml +++ b/src/crates/execution/agent-runtime/Cargo.toml @@ -18,9 +18,11 @@ required-features = ["agent-runtime"] [features] default = [] definition-contracts = [ + "native-hook-settings", "dep:openbitfun-core-types", "dep:regex", "dep:serde", + "dep:serde_json", "dep:serde_yaml", "dep:thiserror", ] @@ -37,6 +39,7 @@ native-hook-runtime = [ "tokio/macros", "tokio/process", "tokio/rt", + "tokio/sync", "tokio/time", ] agent-runtime = [ diff --git a/src/crates/execution/agent-runtime/src/native_hooks/engine.rs b/src/crates/execution/agent-runtime/src/native_hooks/engine.rs index 1726c93096..ec1b533553 100644 --- a/src/crates/execution/agent-runtime/src/native_hooks/engine.rs +++ b/src/crates/execution/agent-runtime/src/native_hooks/engine.rs @@ -10,7 +10,9 @@ //! - Any other exit code, spawn failure, or timeout: a non-blocking warning. use super::call::{HookCall, HookCallPayload}; -use super::handler::{HookHandler, HookHandlerResult, PluginHookCall}; +use super::handler::{ + CommandHookOptions, HookHandler, HookHandlerResult, HookToolMapping, PluginHookCall, +}; use super::kind::RuntimeHookKind; use super::output::{non_empty, AgentHookOutcome, RawHookOutput}; use super::payload::AgentHookPayload; @@ -37,6 +39,7 @@ const MAX_CAPTURED_OUTPUT_BYTES: usize = 1024 * 1024; pub struct AgentHookEngine { registry: RuntimeHookRegistry, settings: Option>, + project_hooks_disabled: bool, } impl AgentHookEngine { @@ -48,6 +51,7 @@ impl AgentHookEngine { Self { registry, settings: Some(Arc::new(settings)), + project_hooks_disabled: false, } } @@ -55,9 +59,15 @@ impl AgentHookEngine { Self { registry, settings: None, + project_hooks_disabled: false, } } + pub fn with_project_hooks_enabled(mut self, enabled: bool) -> Self { + self.project_hooks_disabled = !enabled; + self + } + pub fn is_empty(&self) -> bool { self.registry.plans().is_empty() } @@ -77,6 +87,22 @@ impl AgentHookEngine { .is_empty() } + pub fn has_rules_for_session( + &self, + event: AgentHookEvent, + workspace_scope: Option<&str>, + session_id: &str, + ) -> bool { + !self + .registry + .registrations_for_session( + RuntimeHookKind::Lifecycle(event), + workspace_scope, + session_id, + ) + .is_empty() + } + pub fn settings(&self) -> &AgentHookSettings { self.settings .as_deref() @@ -102,24 +128,75 @@ impl AgentHookEngine { ) -> AgentHookOutcome { let event = payload.event(); let mut outcome = AgentHookOutcome::default(); - let registrations = self - .registry - .registrations_for_workspace(RuntimeHookKind::Lifecycle(event), workspace_scope); + let registrations = self.registry.registrations_for_session( + RuntimeHookKind::Lifecycle(event), + workspace_scope, + &payload.common.session_id, + ); if registrations.is_empty() { return outcome; } let matcher_value = payload.event.matcher_value(); - let payload_json = payload.to_json().to_string(); + let payload_value = payload.to_json(); let call = lifecycle_call(payload, cwd); for registration in registrations.iter() { - if !registration.matcher.matches(matcher_value) { + if self.project_hooks_disabled && registration.requires_project_trust { + continue; + } + let options = ®istration.command_options; + let mapping = options + .tool_mappings + .iter() + .find(|mapping| Some(mapping.runtime_name.as_str()) == matcher_value); + if !registration.matcher.matches(matcher_value) + && !mapping + .is_some_and(|mapping| registration.matcher.matches(Some(&mapping.hook_name))) + { + continue; + } + let mut once = match &options.once { + Some(state) => Some(state.lock().await), + None => None, + }; + if once.as_deref().copied().unwrap_or(false) || !registration.is_active() { continue; } outcome.executed_handlers += 1; let finalized = match ®istration.handler { HookHandler::Command(handler) => { - self.run_and_apply(event, handler, &payload_json, cwd, &mut outcome) - .await + let mut input = payload_value.clone(); + if let Some(mapping) = mapping { + input["tool_name"] = Value::String(mapping.hook_name.clone()); + if let Err(reason) = + translate_input_fields(&mut input["tool_input"], mapping, false) + { + outcome.block_reason = Some(reason); + break; + } + } + let input_json = input.to_string(); + let (finalized, succeeded) = tokio::select! { + biased; + _ = registration.cancelled() => { + outcome.block_reason = Some("Session skill hooks were cleared during this event".into()); + break; + }, + result = self.run_and_apply( + event, + handler, + &input_json, + cwd, + options, + mapping, + &mut outcome, + ) => result, + }; + if succeeded { + if let Some(state) = once.as_deref_mut() { + *state = true; + } + } + finalized } HookHandler::Builtin { executor } => { apply_handler_result(executor.execute(&call).await, &mut outcome) @@ -313,17 +390,18 @@ impl AgentHookEngine { result } - /// Run one handler and fold its result into `outcome`. Returns `true` - /// when the dispatch is finalized (blocked or denied) and remaining - /// handlers must not run. + /// Returns (dispatch finalized, command exited successfully). A failed or + /// cancelled once handler stays eligible for a later matching event. async fn run_and_apply( &self, event: AgentHookEvent, handler: &AgentHookHandler, payload_json: &str, cwd: &Path, + options: &CommandHookOptions, + mapping: Option<&HookToolMapping>, outcome: &mut AgentHookOutcome, - ) -> bool { + ) -> (bool, bool) { let command = handler.effective_command(); let timeout = handler.effective_timeout(event); debug!( @@ -332,8 +410,9 @@ impl AgentHookEngine { command, timeout.as_millis() ); - let run = run_hook_command(command, payload_json, cwd, timeout).await; - match run { + let run = run_hook_command(command, payload_json, cwd, timeout, &options.environment).await; + let mut succeeded = false; + let finalized = match run { HookCommandRun::SpawnFailed(error) => { outcome.warnings.push(format!( "Hook '{command}' for {event} could not be started: {error}" @@ -352,16 +431,35 @@ impl AgentHookEngine { stdout, stderr, } => match exit_code { - Some(0) => match serde_json::from_str::(stdout.trim()) { - Ok(output) => outcome.apply_output(output), - Err(_) => { - let text = stdout.trim(); - if !text.is_empty() && event.plain_stdout_is_context() { - outcome.additional_context.push(truncate_model_output(text)); + Some(0) => { + succeeded = true; + match serde_json::from_str::(stdout.trim()) { + Ok(mut output) => { + if let Some(updated) = output + .hook_specific_output + .as_mut() + .and_then(|specific| specific.updated_input.as_mut()) + { + if let Some(mapping) = mapping { + if let Err(reason) = + translate_input_fields(updated, mapping, true) + { + outcome.block_reason = Some(reason); + return (true, succeeded); + } + } + } + outcome.apply_output_with_ask(output, options.supports_ask) + } + Err(_) => { + let text = stdout.trim(); + if !text.is_empty() && event.plain_stdout_is_context() { + outcome.additional_context.push(truncate_model_output(text)); + } + false } - false } - }, + } Some(2) => { let reason = non_empty(Some(stderr)).unwrap_or_else(|| { format!("Hook '{command}' blocked this {event} event (exit code 2).") @@ -384,8 +482,37 @@ impl AgentHookEngine { false } }, + }; + (finalized, succeeded) + } +} + +fn translate_input_fields( + input: &mut Value, + mapping: &HookToolMapping, + reverse: bool, +) -> Result<(), String> { + if let Some(adapter) = &mapping.input_adapter { + if reverse { + adapter.to_runtime(input)?; + } else { + adapter.to_hook(input)?; + } + } + let Some(fields) = input.as_object_mut() else { + return Ok(()); + }; + for (runtime, hook) in &mapping.input_fields { + let (from, to) = if reverse { + (hook, runtime) + } else { + (runtime, hook) + }; + if let Some(value) = fields.remove(from) { + fields.insert(to.clone(), value); } } + Ok(()) } #[derive(Debug, Clone, PartialEq)] @@ -465,6 +592,7 @@ async fn run_hook_command( payload_json: &str, cwd: &Path, timeout: Duration, + environment: &std::collections::BTreeMap, ) -> HookCommandRun { let mut process = if cfg!(windows) { let mut process = Command::new("cmd"); @@ -475,6 +603,7 @@ async fn run_hook_command( process.arg("-c").arg(command); process }; + process.envs(environment); if let Some(cwd) = existing_dir(cwd) { process.current_dir(cwd); } diff --git a/src/crates/execution/agent-runtime/src/native_hooks/handler.rs b/src/crates/execution/agent-runtime/src/native_hooks/handler.rs index 5253c57079..177eed26e2 100644 --- a/src/crates/execution/agent-runtime/src/native_hooks/handler.rs +++ b/src/crates/execution/agent-runtime/src/native_hooks/handler.rs @@ -6,8 +6,37 @@ use super::registry::RuntimeHookPlan; use super::settings::{AgentHookHandler, AgentHookMatcher}; use async_trait::async_trait; use serde_json::Value; +use std::collections::BTreeMap; +use std::sync::atomic::{AtomicBool, Ordering}; use std::sync::Arc; +/// A host-supplied translation for a command hook's tool vocabulary. Native +/// command registrations have no mapping and retain their existing contract. +#[derive(Debug, Clone)] +pub struct HookToolMapping { + pub runtime_name: String, + pub hook_name: String, + /// Runtime argument name -> hook argument name; reversed for updatedInput. + pub input_fields: BTreeMap, + pub input_adapter: Option>, +} + +/// Provider-owned transformations for tools with different input encodings. +pub trait HookInputAdapter: Send + Sync + std::fmt::Debug { + fn to_hook(&self, input: &mut Value) -> Result<(), String>; + fn to_runtime(&self, input: &mut Value) -> Result<(), String>; +} + +#[derive(Debug, Default)] +pub struct CommandHookOptions { + pub environment: BTreeMap, + pub tool_mappings: Vec, + /// Opt-in for sources whose contract supports a PreToolUse ask decision. + pub supports_ask: bool, + /// Shared across dispatch snapshots; failures leave this false for retry. + pub once: Option>, +} + #[derive(Clone)] pub enum HookHandler { Command(AgentHookHandler), @@ -96,6 +125,10 @@ pub struct RuntimeHookRegistration { pub handler: HookHandler, pub matcher: AgentHookMatcher, pub workspace_scope: Option, + pub command_options: Arc, + pub requires_project_trust: bool, + pub(crate) active: Option>, + pub(crate) cancellation: Option>, } impl RuntimeHookRegistration { @@ -105,7 +138,32 @@ impl RuntimeHookRegistration { handler, matcher, workspace_scope: None, + command_options: Arc::default(), + requires_project_trust: false, + active: None, + cancellation: None, + } + } + + pub fn with_command_options(mut self, options: CommandHookOptions) -> Self { + self.command_options = Arc::new(options); + self + } + + pub(crate) fn is_active(&self) -> bool { + self.active + .as_ref() + .is_none_or(|active| active.load(Ordering::Acquire)) + } + + pub(crate) async fn cancelled(&self) { + let Some(mut cancellation) = self.cancellation.clone() else { + return std::future::pending().await; + }; + if *cancellation.borrow() { + return; } + let _ = cancellation.changed().await; } pub fn with_workspace_scope(mut self, workspace_scope: impl Into) -> Self { diff --git a/src/crates/execution/agent-runtime/src/native_hooks/kind.rs b/src/crates/execution/agent-runtime/src/native_hooks/kind.rs index c0fc8f5c7b..7ea17a6294 100644 --- a/src/crates/execution/agent-runtime/src/native_hooks/kind.rs +++ b/src/crates/execution/agent-runtime/src/native_hooks/kind.rs @@ -23,6 +23,8 @@ pub enum RuntimeHookSource { UserCommand, ProjectCommand, ImportedCommand, + /// Command handlers activated by a skill invocation in one session. + SkillCommand, /// Executable plugin contribution. The concrete ecosystem is owned by an /// adapter; the portable runtime only models trust/source precedence. Plugin, @@ -41,6 +43,7 @@ impl fmt::Display for RuntimeHookSource { Self::UserCommand => f.write_str("user-command"), Self::ProjectCommand => f.write_str("project-command"), Self::ImportedCommand => f.write_str("imported-command"), + Self::SkillCommand => f.write_str("skill-command"), Self::Plugin => f.write_str("plugin"), } } diff --git a/src/crates/execution/agent-runtime/src/native_hooks/mod.rs b/src/crates/execution/agent-runtime/src/native_hooks/mod.rs index 74d9ac1235..f823d16ac4 100644 --- a/src/crates/execution/agent-runtime/src/native_hooks/mod.rs +++ b/src/crates/execution/agent-runtime/src/native_hooks/mod.rs @@ -38,8 +38,9 @@ pub use call::{HookCall, HookCallPayload}; pub use engine::{AgentHookEngine, PluginHookDispatchResult, MAX_HOOK_MODEL_OUTPUT_BYTES}; #[cfg(feature = "native-hook-runtime")] pub use handler::{ - BuiltinHookExecutor, HookHandler, HookHandlerResult, PluginHookCall, PluginHookExecutor, - PluginHookGenerationIdentity, PluginHookResult, RuntimeHookRegistration, + BuiltinHookExecutor, CommandHookOptions, HookHandler, HookHandlerResult, HookInputAdapter, + HookToolMapping, PluginHookCall, PluginHookExecutor, PluginHookGenerationIdentity, + PluginHookResult, RuntimeHookRegistration, }; #[cfg(feature = "native-hook-runtime")] pub use kind::{RuntimeHookKind, RuntimeHookSource}; diff --git a/src/crates/execution/agent-runtime/src/native_hooks/output.rs b/src/crates/execution/agent-runtime/src/native_hooks/output.rs index a78a47d091..213715b1df 100644 --- a/src/crates/execution/agent-runtime/src/native_hooks/output.rs +++ b/src/crates/execution/agent-runtime/src/native_hooks/output.rs @@ -56,8 +56,16 @@ pub(crate) struct RawPermissionRequestDecision { /// or PermissionRequest `decision.behavior`. #[derive(Debug, Clone, PartialEq, Eq)] pub enum AgentHookPermissionOutcome { - Allow { reason: Option }, - Deny { reason: Option }, + Allow { + reason: Option, + }, + Deny { + reason: Option, + }, + /// Only enabled by registrations whose source contract supports ask. + Ask { + reason: Option, + }, } /// Aggregated result of dispatching one event across all matching handlers. @@ -103,7 +111,11 @@ impl AgentHookOutcome { /// Fold one parsed stdout document into the aggregate. Returns `true` /// when dispatch should stop running further handlers (a final blocking /// or denying decision was made). - pub(crate) fn apply_output(&mut self, output: RawHookOutput) -> bool { + pub(crate) fn apply_output_with_ask( + &mut self, + output: RawHookOutput, + supports_ask: bool, + ) -> bool { let mut finalized = false; if let Some(message) = non_empty(output.system_message) { self.system_messages.push(message); @@ -125,6 +137,14 @@ impl AgentHookOutcome { finalized = true; } if let Some(specific) = output.hook_specific_output { + if supports_ask + && specific.permission_decision.as_deref() == Some("ask") + && !self.permission_denied() + { + self.permission = Some(AgentHookPermissionOutcome::Ask { + reason: non_empty(specific.permission_decision_reason.clone()), + }); + } if let Some(context) = non_empty(specific.additional_context) { self.additional_context.push(context); } @@ -159,7 +179,13 @@ impl AgentHookOutcome { self.permission = Some(AgentHookPermissionOutcome::Deny { reason }); true } - Some("allow") if !self.permission_denied() => { + Some("allow") + if !self.permission_denied() + && !matches!( + self.permission, + Some(AgentHookPermissionOutcome::Ask { .. }) + ) => + { self.permission = Some(AgentHookPermissionOutcome::Allow { reason }); false } diff --git a/src/crates/execution/agent-runtime/src/native_hooks/registry.rs b/src/crates/execution/agent-runtime/src/native_hooks/registry.rs index 0fcdb26d3d..3a1354a8c2 100644 --- a/src/crates/execution/agent-runtime/src/native_hooks/registry.rs +++ b/src/crates/execution/agent-runtime/src/native_hooks/registry.rs @@ -4,6 +4,7 @@ use super::handler::{HookHandler, RuntimeHookRegistration}; use super::kind::{RuntimeHookKind, RuntimeHookSource}; use std::collections::{BTreeMap, HashSet}; use std::fmt; +use std::sync::atomic::{AtomicBool, Ordering}; use std::sync::{Arc, RwLock}; #[derive(Debug, Clone, Copy, PartialEq, Eq)] @@ -98,6 +99,12 @@ pub enum RuntimeHookRegistryBuildError { InvalidTimeoutMillis { hook_id: String }, #[error("duplicate runtime hook id {hook_id}")] DuplicateHookId { hook_id: String }, + #[error("invalid session skill hook registration")] + InvalidSessionSkill, + #[error("skill hooks changed after activation; start a new session to use the changed skill")] + SkillChanged, + #[error("session skill hook handler limit exceeded")] + SessionHandlerLimit, } #[derive(Debug, Clone, PartialEq, Eq, thiserror::Error)] @@ -177,6 +184,14 @@ struct RuntimeHookRegistryState { entries: BTreeMap>, source_activation: BTreeMap<(RuntimeHookSource, Option), RuntimeHookActivation>, active_plugin_generations: BTreeMap, + session_skills: BTreeMap>, +} + +struct SessionSkillHooks { + fingerprint: String, + active: Arc, + cancellation: tokio::sync::watch::Sender, + entries: Arc<[RuntimeHookRegistration]>, } impl Default for RuntimeHookRegistryState { @@ -185,6 +200,7 @@ impl Default for RuntimeHookRegistryState { entries: BTreeMap::new(), source_activation: BTreeMap::new(), active_plugin_generations: BTreeMap::new(), + session_skills: BTreeMap::new(), } } } @@ -209,6 +225,113 @@ impl RuntimeHookRegistry { RuntimeHookRegistryBuilder::default() } + /// Publish a skill atomically and at most once per session. The fingerprint + /// pins the invoked definition, including consumed once handlers, until + /// session teardown; refresh cannot silently replace executable rules. + pub fn register_session_skill( + &self, + session_id: &str, + skill_key: &str, + fingerprint: &str, + mut entries: Vec, + ) -> Result { + if session_id.trim().is_empty() + || skill_key.trim().is_empty() + || entries.iter().any(|entry| { + entry.plan.source() != RuntimeHookSource::SkillCommand + || !matches!(&entry.handler, HookHandler::Command(_)) + }) + { + return Err(RuntimeHookRegistryBuildError::InvalidSessionSkill.into()); + } + validate_entries(&entries)?; + let mut state = self.inner.write().expect("hook registry lock poisoned"); + let skills = state + .session_skills + .entry(session_id.to_string()) + .or_default(); + if let Some(existing) = skills.get(skill_key) { + return if existing.fingerprint == fingerprint { + Ok(existing.entries.len()) + } else { + Err(RuntimeHookRegistryBuildError::SkillChanged.into()) + }; + } + let count = entries.len(); + if skills + .values() + .map(|skill| skill.entries.len()) + .sum::() + + count + > super::settings::MAX_HOOK_HANDLERS + { + return Err(RuntimeHookRegistryBuildError::SessionHandlerLimit.into()); + } + let active = Arc::new(AtomicBool::new(true)); + let (cancellation, receiver) = tokio::sync::watch::channel(false); + for entry in &mut entries { + entry.active = Some(active.clone()); + entry.cancellation = Some(receiver.clone()); + } + skills.insert( + skill_key.to_string(), + SessionSkillHooks { + fingerprint: fingerprint.to_string(), + active, + cancellation, + entries: Arc::from(entries), + }, + ); + Ok(count) + } + + /// Invalidate snapshots as well as removing the registry's ownership. + pub fn clear_session(&self, session_id: &str) { + let mut state = self.inner.write().expect("hook registry lock poisoned"); + if let Some(skills) = state.session_skills.remove(session_id) { + for skill in skills.values() { + skill.active.store(false, Ordering::Release); + skill.cancellation.send_replace(true); + } + } + } + + pub fn registrations_for_session( + &self, + kind: RuntimeHookKind, + workspace_scope: Option<&str>, + session_id: &str, + ) -> Arc<[RuntimeHookRegistration]> { + let mut entries = self + .registrations_for_workspace(kind.clone(), workspace_scope) + .to_vec(); + let state = self.inner.read().expect("hook registry lock poisoned"); + if let Some(skills) = state.session_skills.get(session_id) { + entries.extend( + skills + .values() + .flat_map(|skill| skill.entries.iter()) + .filter(|entry| { + entry.plan.kind() == &kind + && entry.is_active() + && entry + .workspace_scope + .as_deref() + .is_none_or(|scope| Some(scope) == workspace_scope) + }) + .cloned(), + ); + } + entries.sort_by(|left, right| { + left.plan + .source() + .cmp(&right.plan.source()) + .then_with(|| left.plan.order().cmp(&right.plan.order())) + .then_with(|| left.plan.id().cmp(right.plan.id())) + }); + Arc::from(entries) + } + pub fn register_batch( &self, entries: Vec, diff --git a/src/crates/execution/agent-runtime/src/skills/hooks.rs b/src/crates/execution/agent-runtime/src/skills/hooks.rs new file mode 100644 index 0000000000..0f8e01b59b --- /dev/null +++ b/src/crates/execution/agent-runtime/src/skills/hooks.rs @@ -0,0 +1,248 @@ +//! Executable skill declarations. Discovery validates the complete declaration; +//! only invocation publishes its handlers into a session's existing registry. + +use super::SkillParseError; +use crate::native_hooks::{ + AgentHookEvent, AgentHookHandler, AgentHookMatcher, AgentHookScope, AgentHookSettings, + AgentHookSettingsLayer, +}; +use serde_json::{json, Value}; + +#[derive(Debug, Clone)] +// Discovery-only consumers validate and retain these fields without executing them. +#[cfg_attr(not(feature = "native-hook-runtime"), allow(dead_code))] +struct SkillHook { + event: AgentHookEvent, + matcher: AgentHookMatcher, + handler: AgentHookHandler, + once: bool, +} + +#[derive(Debug, Clone)] +pub struct SkillHooks { + hooks: Vec, + fingerprint: String, +} + +impl SkillHooks { + pub fn from_yaml(value: &serde_yaml::Value) -> Result { + let value = serde_json::to_value(value).map_err(|error| { + SkillParseError::InvalidFormat(format!("Invalid skill hooks: {error}")) + })?; + let invalid = || { + SkillParseError::InvalidFormat( + "Skill hooks must contain valid event matcher groups and command handlers".into(), + ) + }; + let events = value.as_object().ok_or_else(invalid)?; + for (event, groups) in events { + if AgentHookEvent::parse(event).is_none() { + return Err(SkillParseError::UnsupportedClaudeField(format!( + "hooks.{event}" + ))); + } + for group in groups.as_array().ok_or_else(invalid)? { + let group = group.as_object().ok_or_else(invalid)?; + if let Some(field) = group + .keys() + .find(|field| !matches!(field.as_str(), "matcher" | "hooks")) + { + return Err(SkillParseError::UnsupportedClaudeField(format!( + "hooks.{field}" + ))); + } + for handler in group + .get("hooks") + .and_then(Value::as_array) + .ok_or_else(invalid)? + { + let handler = handler.as_object().ok_or_else(invalid)?; + if let Some(kind) = handler.get("type").and_then(Value::as_str) { + if kind != "command" { + return Err(SkillParseError::UnsupportedClaudeField(format!( + "hooks.type={kind}" + ))); + } + } + if let Some(field) = handler.keys().find(|field| { + !matches!( + field.as_str(), + "type" + | "command" + | "commandWindows" + | "timeout" + | "statusMessage" + | "once" + ) + }) { + return Err(SkillParseError::UnsupportedClaudeField(format!( + "hooks.{field}" + ))); + } + if handler.get("once").is_some_and(|once| !once.is_boolean()) { + return Err(invalid()); + } + } + } + } + let (settings, issues) = AgentHookSettings::from_layers(&[AgentHookSettingsLayer { + scope: AgentHookScope::User, + source: "skill hooks".into(), + bytes: serde_json::to_vec(&json!({ "hooks": value })).map_err(|_| invalid())?, + }]); + // Unlike optional user configuration, dropping any skill declaration + // would silently remove part of that skill's behavior or constraints. + if !issues.is_empty() { + return Err(SkillParseError::InvalidFormat( + issues + .iter() + .map(ToString::to_string) + .collect::>() + .join("; "), + )); + } + let mut hooks = Vec::new(); + for event in AgentHookEvent::ALL { + let Some(groups) = events.get(event.as_str()).and_then(Value::as_array) else { + continue; + }; + let mut rules = settings.rules_for(event).iter(); + for group in groups { + let raw_handlers = group["hooks"].as_array().ok_or_else(invalid)?; + if raw_handlers.is_empty() { + continue; + } + let rule = rules.next().ok_or_else(invalid)?; + for (handler, raw) in rule.handlers.iter().zip(raw_handlers) { + hooks.push(SkillHook { + event, + matcher: rule.matcher.clone(), + handler: handler.clone(), + once: raw.get("once").and_then(Value::as_bool).unwrap_or(false), + }); + } + } + } + Ok(Self { + hooks, + fingerprint: value.to_string(), + }) + } + + pub fn is_empty(&self) -> bool { + self.hooks.is_empty() + } + + pub fn fingerprint(&self) -> &str { + &self.fingerprint + } + + #[cfg(feature = "native-hook-runtime")] + pub fn registrations( + &self, + session_id: &str, + skill_key: &str, + skill_path: &str, + workspace_scope: Option<&str>, + workspace_root: &str, + ) -> Vec { + use crate::native_hooks::{ + CommandHookOptions, HookToolMapping, RuntimeHookKind, RuntimeHookRegistration, + RuntimeHookSource, + }; + self.hooks + .iter() + .enumerate() + .map(|(index, hook)| { + let id = json!(["skill", session_id, skill_key, index]).to_string(); + let mut registration = RuntimeHookRegistration::command( + id, + RuntimeHookKind::Lifecycle(hook.event), + RuntimeHookSource::SkillCommand, + hook.handler.clone(), + hook.matcher.clone(), + ) + .with_command_options(CommandHookOptions { + environment: [ + ("CLAUDE_SESSION_ID".into(), session_id.into()), + ("CLAUDE_SKILL_DIR".into(), skill_path.into()), + ("CLAUDE_PROJECT_DIR".into(), workspace_root.into()), + ] + .into(), + // The Skill parser already owns Claude dialect projection. + // Keep the engine's mapping primitive provider-neutral. + tool_mappings: vec![ + HookToolMapping { + runtime_name: "ExecCommand".into(), + hook_name: "Bash".into(), + input_fields: [("cmd".into(), "command".into())].into(), + input_adapter: None, + }, + HookToolMapping { + runtime_name: "Write".into(), + hook_name: "Write".into(), + input_fields: Default::default(), + input_adapter: Some(std::sync::Arc::new(ClaudeWriteInput)), + }, + ], + supports_ask: true, + once: hook.once.then(|| tokio::sync::Mutex::new(false)), + }); + registration.plan = registration.plan.with_order(index as u16); + if let Some(workspace) = workspace_scope { + registration = registration.with_workspace_scope(workspace); + } + registration + }) + .collect() + } +} + +#[cfg(feature = "native-hook-runtime")] +#[derive(Debug)] +struct ClaudeWriteInput; + +#[cfg(feature = "native-hook-runtime")] +impl crate::native_hooks::HookInputAdapter for ClaudeWriteInput { + fn to_hook(&self, input: &mut Value) -> Result<(), String> { + let payload = input + .get("payload") + .and_then(Value::as_str) + .ok_or("Cannot inspect Write with skill hooks: missing path-first payload")?; + let (header, content) = payload.split_once('\n').unwrap_or((payload, "")); + let path = header + .trim_end_matches('\r') + .strip_prefix("+++ ") + .filter(|path| !path.trim().is_empty()) + .ok_or("Cannot inspect Write with skill hooks: an explicit destination is required")?; + let path = path.to_owned(); + let content = content.to_owned(); + let fields = input + .as_object_mut() + .ok_or("Write input must be an object")?; + fields.remove("payload"); + fields.insert("file_path".into(), Value::String(path)); + fields.insert("content".into(), Value::String(content)); + Ok(()) + } + + fn to_runtime(&self, input: &mut Value) -> Result<(), String> { + let path = input + .get("file_path") + .and_then(Value::as_str) + .filter(|path| !path.trim().is_empty() && !path.contains(['\n', '\r'])) + .ok_or("Invalid Write hook updatedInput: file_path is required")?; + let content = input + .get("content") + .and_then(Value::as_str) + .ok_or("Invalid Write hook updatedInput: content is required")?; + let payload = format!("+++ {path}\n{content}"); + let fields = input + .as_object_mut() + .ok_or("Write input must be an object")?; + fields.remove("file_path"); + fields.remove("content"); + fields.insert("payload".into(), Value::String(payload)); + Ok(()) + } +} diff --git a/src/crates/execution/agent-runtime/src/skills/mod.rs b/src/crates/execution/agent-runtime/src/skills/mod.rs index 24784481a7..572e697edd 100644 --- a/src/crates/execution/agent-runtime/src/skills/mod.rs +++ b/src/crates/execution/agent-runtime/src/skills/mod.rs @@ -7,6 +7,7 @@ #[cfg(feature = "agent-runtime")] mod catalog; +mod hooks; #[cfg(feature = "agent-runtime")] mod keys; #[cfg(feature = "agent-runtime")] @@ -14,6 +15,7 @@ mod policy; #[cfg(feature = "agent-runtime")] mod resolver; mod roots; +pub use hooks::SkillHooks; #[cfg(feature = "agent-runtime")] mod selection; mod types; diff --git a/src/crates/execution/agent-runtime/src/skills/types.rs b/src/crates/execution/agent-runtime/src/skills/types.rs index 63fd945840..686ac6675d 100644 --- a/src/crates/execution/agent-runtime/src/skills/types.rs +++ b/src/crates/execution/agent-runtime/src/skills/types.rs @@ -14,9 +14,29 @@ pub struct SkillScanDiagnostic { pub path: String, pub source_id: String, pub message: String, + /// A required declaration this runtime cannot honor. The skill stays unloaded. + /// Older hosts omit this field and retain ordinary scan-error presentation. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub unsupported_field: Option, } impl SkillScanDiagnostic { + pub fn from_parse_error( + path: impl Into, + source_id: impl Into, + error: &SkillParseError, + ) -> Self { + Self { + path: path.into(), + source_id: source_id.into(), + message: error.to_string(), + unsupported_field: match error { + SkillParseError::UnsupportedClaudeField(field) => Some(field.clone()), + _ => None, + }, + } + } + pub fn to_xml(&self) -> String { let text = format!( "Skill discovery notice at {} ({}): {}", @@ -52,6 +72,8 @@ pub(crate) enum SkillSourceDialect { pub enum SkillParseError { #[error("Invalid SKILL.md format: {0}")] InvalidFormat(String), + #[error("Claude skill requires unsupported field '{0}'; this skill was not loaded")] + UnsupportedClaudeField(String), #[error("Missing required field '{0}' in SKILL.md")] MissingField(&'static str), #[error("Invalid skill path: {0}")] @@ -219,6 +241,8 @@ pub struct SkillData { pub argument_names: Vec, /// Compatibility notices accompany both discovery and explicit loading. pub compatibility_warnings: Vec, + /// Validated declarations, activated only when this skill is invoked. + pub hooks: Option, } fn default_allow_implicit_invocation() -> bool { @@ -406,7 +430,6 @@ fn claude_compatibility_warnings( const UNSUPPORTED_FIELDS: &[&str] = &[ "context", "agent", - "hooks", "paths", "shell", "runtime", @@ -417,9 +440,9 @@ fn claude_compatibility_warnings( .iter() .find(|field| metadata.get(**field).is_some()) { - return Err(SkillParseError::InvalidFormat(format!( - "Claude field '{field}' is not supported" - ))); + return Err(SkillParseError::UnsupportedClaudeField( + (*field).to_string(), + )); } let mut warnings = Vec::new(); @@ -580,6 +603,14 @@ impl SkillData { let argument_hint = optional_string(&metadata, "argument-hint")?; let skill_content = if with_content { body } else { String::new() }; + let hooks = if dialect == SkillSourceDialect::ClaudeCode { + metadata + .get("hooks") + .map(super::SkillHooks::from_yaml) + .transpose()? + } else { + None + }; Ok(SkillData { key: String::new(), name, @@ -597,6 +628,7 @@ impl SkillData { argument_hint, argument_names, compatibility_warnings, + hooks, }) } diff --git a/src/crates/execution/agent-runtime/tests/agent_definition_contracts/skill_contracts.rs b/src/crates/execution/agent-runtime/tests/agent_definition_contracts/skill_contracts.rs index 9055c0332c..e89d0a855b 100644 --- a/src/crates/execution/agent-runtime/tests/agent_definition_contracts/skill_contracts.rs +++ b/src/crates/execution/agent-runtime/tests/agent_definition_contracts/skill_contracts.rs @@ -238,7 +238,6 @@ fn claude_skill_rejects_unavailable_runtime_semantics() { for field in [ "context: fork", "agent: Explore", - "hooks: {}", "paths: src/**", "shell: bash", "runtime: node", @@ -255,7 +254,21 @@ fn claude_skill_rejects_unavailable_runtime_semantics() { "claude", ) .expect_err("unsupported Claude behavior must fail closed"); - assert!(matches!(error, SkillParseError::InvalidFormat(_))); + assert_eq!( + error, + SkillParseError::UnsupportedClaudeField(field.split_once(':').unwrap().0.into()) + ); + let diagnostic = openbitfun_agent_runtime::skills::SkillScanDiagnostic::from_parse_error( + "/workspace/.claude/skills/unsafe/SKILL.md", + "claude-code", + &error, + ); + assert_eq!( + diagnostic.unsupported_field.as_deref(), + Some(field.split_once(':').unwrap().0) + ); + assert!(diagnostic.message.contains("this skill was not loaded")); + assert!(!diagnostic.message.contains("Invalid SKILL.md format")); } } @@ -1380,9 +1393,36 @@ fn skill_scan_reports_tolerate_older_shapes_and_escape_diagnostics() { path: "/remote/".into(), source_id: "codex".into(), message: "read & parse failed".into(), + unsupported_field: None, }; assert!(diagnostic.to_xml().contains("<path>")); assert!(diagnostic.to_xml().contains("read & parse failed")); + let legacy_diagnostic = serde_json::json!({ + "path": "/remote/", "sourceId": "codex", "message": "read & parse failed" + }); + let decoded: SkillScanDiagnostic = serde_json::from_value(legacy_diagnostic.clone()).unwrap(); + assert_eq!(decoded.unsupported_field, None); + assert_eq!(serde_json::to_value(decoded).unwrap(), legacy_diagnostic); + + let unsupported = SkillScanDiagnostic::from_parse_error( + "/remote/guard/SKILL.md", + "claude-code", + &SkillParseError::UnsupportedClaudeField("hooks".into()), + ); + let encoded = serde_json::to_value(&unsupported).unwrap(); + assert_eq!(encoded["unsupportedField"], "hooks"); + assert_eq!( + serde_json::from_value::(encoded.clone()).unwrap(), + unsupported + ); + #[derive(serde::Deserialize)] + struct LegacyDiagnostic { + path: String, + message: String, + } + let legacy: LegacyDiagnostic = serde_json::from_value(encoded).unwrap(); + assert_eq!(legacy.path, unsupported.path); + assert_eq!(legacy.message, unsupported.message); } #[test] @@ -1410,3 +1450,51 @@ fn workspace_skill_disable_blocks_all_modes_without_reclassifying_the_source() { } assert!(is_skill_globally_enabled(&skill, &HashSet::new())); } + +#[test] +fn claude_skill_hooks_validate_whole_declaration_without_execution() { + use openbitfun_agent_runtime::skills::SkillHooks; + let valid = r#"PreToolUse: + - matcher: "Bash|Edit|Write" + hooks: + - type: command + command: echo guard + once: true + timeout: 5 +Stop: + - hooks: + - type: command + command: echo finished +"#; + let hooks = SkillHooks::from_yaml(&serde_yaml::from_str(valid).unwrap()).unwrap(); + assert!(!hooks.is_empty()); + for invalid in [ + valid.replace("type: command", "type: prompt"), + valid.replace("once: true", "once: maybe"), + valid.replace("timeout: 5", "timeout: invalid"), + valid.replace("Bash|Edit|Write", "["), + valid.replace("once: true", "async: true"), + valid.replace("PreToolUse:", "UnknownEvent:"), + ] { + assert!( + SkillHooks::from_yaml(&serde_yaml::from_str(&invalid).unwrap()).is_err(), + "{invalid}" + ); + } + let markdown = format!( + "---\nname: guarded\ndescription: guarded work\nhooks:\n{}---\nGuard tools.\n", + valid + .lines() + .map(|line| format!(" {line}\n")) + .collect::() + ); + let skill = SkillData::from_markdown_for_source_slot( + "/skills/guarded".into(), + &markdown, + SkillLocation::User, + true, + "claude", + ) + .unwrap(); + assert!(skill.hooks.is_some()); +} diff --git a/src/crates/execution/agent-runtime/tests/native_hook_execution_contracts.rs b/src/crates/execution/agent-runtime/tests/native_hook_execution_contracts.rs index 2072ee008f..f31fb056cc 100644 --- a/src/crates/execution/agent-runtime/tests/native_hook_execution_contracts.rs +++ b/src/crates/execution/agent-runtime/tests/native_hook_execution_contracts.rs @@ -496,3 +496,277 @@ async fn events_without_configured_rules_execute_nothing() { assert_eq!(outcome.executed_handlers, 0); assert!(outcome.additional_context.is_empty()); } + +#[cfg(feature = "agent-runtime")] +mod skill_hooks { + use super::*; + use openbitfun_agent_runtime::native_hooks::{ + AgentHookEvent, RuntimeHookKind, RuntimeHookRegistry, + }; + use openbitfun_agent_runtime::skills::SkillHooks; + + fn install( + registry: &RuntimeHookRegistry, + command: &str, + once: bool, + matcher: &str, + workspace: Option<&str>, + ) -> SkillHooks { + let hooks = SkillHooks::from_yaml( + &serde_yaml::to_value(json!({"PreToolUse":[{ + "matcher": matcher, "hooks":[{"type":"command", "command":command, "once":once}] + }]})) + .unwrap(), + ) + .unwrap(); + registry + .register_session_skill( + "session-1", + "guard", + hooks.fingerprint(), + hooks.registrations("session-1", "guard", "/skills/guard", workspace, "/"), + ) + .unwrap(); + hooks + } + + #[tokio::test] + async fn invocation_is_idempotent_isolated_and_cleanup_invalidates_snapshots() { + let registry = RuntimeHookRegistry::default(); + let hooks = install( + ®istry, + "echo blocked >&2; exit 2", + false, + "Bash", + Some("w1"), + ); + assert_eq!( + registry + .register_session_skill( + "session-1", + "guard", + hooks.fingerprint(), + hooks.registrations("session-1", "guard", "/skills/guard", Some("w1"), "/") + ) + .unwrap(), + 1 + ); + assert!(registry + .register_session_skill( + "session-1", + "guard", + "changed", + hooks.registrations("session-1", "guard", "/skills/guard", Some("w1"), "/") + ) + .is_err()); + let engine = AgentHookEngine::with_registry(registry.clone()); + let payload = pre_tool_use_payload("ExecCommand"); + assert_eq!( + engine + .dispatch_for_workspace(&payload, Path::new("."), Some("w1")) + .await + .block_reason + .as_deref(), + Some("blocked") + ); + assert_eq!( + engine + .dispatch_for_workspace(&payload, Path::new("."), Some("w2")) + .await + .executed_handlers, + 0 + ); + let mut other = payload.clone(); + other.common.session_id = "session-2".into(); + assert_eq!( + engine + .dispatch_for_workspace(&other, Path::new("."), Some("w1")) + .await + .executed_handlers, + 0 + ); + let snapshot = registry.registrations_for_session( + RuntimeHookKind::Lifecycle(AgentHookEvent::PreToolUse), + Some("w1"), + "session-1", + ); + registry.clear_session("session-1"); + let detached = RuntimeHookRegistry::default(); + detached.register_batch(snapshot.to_vec()).unwrap(); + assert_eq!( + AgentHookEngine::with_registry(detached) + .dispatch_for_workspace(&payload, Path::new("."), Some("w1")) + .await + .executed_handlers, + 0 + ); + assert!(!engine.has_rules_for_session(AgentHookEvent::PreToolUse, Some("w1"), "session-1")); + } + + #[tokio::test] + async fn bash_input_environment_and_updated_input_round_trip() { + let registry = RuntimeHookRegistry::default(); + install( + ®istry, + r#"python3 -c 'import json,sys,os; d=json.load(sys.stdin); assert d["tool_name"]=="Bash"; assert d["tool_input"]["command"]=="echo before"; assert "cmd" not in d["tool_input"]; assert os.environ["CLAUDE_SESSION_ID"]=="session-1"; assert os.environ["CLAUDE_SKILL_DIR"]=="/skills/guard"; print(json.dumps({"hookSpecificOutput":{"permissionDecision":"ask","permissionDecisionReason":"review command","updatedInput":{"command":"echo after","yield_time_ms":1000}}}))'"#, + false, + "Bash", + None, + ); + let engine = AgentHookEngine::with_registry(registry); + let mut payload = pre_tool_use_payload("ExecCommand"); + if let AgentHookEventPayload::PreToolUse { tool_input, .. } = &mut payload.event { + *tool_input = json!({"cmd":"echo before"}); + } + let result = dispatch(&engine, &payload).await; + assert!(result.warnings.is_empty(), "{:?}", result.warnings); + assert_eq!( + result.permission, + Some(AgentHookPermissionOutcome::Ask { + reason: Some("review command".into()) + }) + ); + assert_eq!( + result.updated_input, + Some(json!({"cmd":"echo after","yield_time_ms":1000})) + ); + } + + #[tokio::test] + async fn write_input_round_trip_and_ambiguous_destination_blocks() { + let registry = RuntimeHookRegistry::default(); + install( + ®istry, + r#"python3 -c 'import json,sys; d=json.load(sys.stdin); assert d["tool_input"]=={"file_path":"/tmp/a","content":"before"}; print(json.dumps({"hookSpecificOutput":{"updatedInput":{"file_path":"/tmp/b","content":"after"}}}))'"#, + false, + "Write", + None, + ); + let engine = AgentHookEngine::with_registry(registry); + let mut payload = pre_tool_use_payload("Write"); + if let AgentHookEventPayload::PreToolUse { tool_input, .. } = &mut payload.event { + *tool_input = json!({"payload":"+++ /tmp/a\r\nbefore"}); + } + let result = dispatch(&engine, &payload).await; + assert!(result.warnings.is_empty(), "{:?}", result.warnings); + assert_eq!( + result.updated_input, + Some(json!({"payload":"+++ /tmp/b\nafter"})) + ); + if let AgentHookEventPayload::PreToolUse { tool_input, .. } = &mut payload.event { + *tool_input = json!({"payload":"no path"}); + } + assert!(dispatch(&engine, &payload).await.is_blocked()); + } + + #[tokio::test] + async fn once_is_atomic_and_failed_commands_remain_eligible() { + for command in ["exit 2", "exit 7"] { + let registry = RuntimeHookRegistry::default(); + install(®istry, command, true, "Read", None); + let engine = AgentHookEngine::with_registry(registry); + let payload = pre_tool_use_payload("Read"); + assert_eq!(dispatch(&engine, &payload).await.executed_handlers, 1); + assert_eq!(dispatch(&engine, &payload).await.executed_handlers, 1); + } + let registry = RuntimeHookRegistry::default(); + let hooks = install(®istry, "sleep 0.02; exit 0", true, "Read", None); + let engine = AgentHookEngine::with_registry(registry.clone()); + let payload = pre_tool_use_payload("Read"); + let (a, b) = tokio::join!(dispatch(&engine, &payload), dispatch(&engine, &payload)); + assert_eq!(a.executed_handlers + b.executed_handlers, 1); + registry + .register_session_skill( + "session-1", + "guard", + hooks.fingerprint(), + hooks.registrations("session-1", "guard", "/skills/guard", None, "/"), + ) + .unwrap(); + assert_eq!(dispatch(&engine, &payload).await.executed_handlers, 0); + } + + #[tokio::test] + async fn project_gate_applies_to_already_registered_skills() { + let registry = RuntimeHookRegistry::default(); + let hooks = SkillHooks::from_yaml( + &serde_yaml::from_str("PreToolUse: [{hooks: [{type: command, command: 'exit 2'}]}]") + .unwrap(), + ) + .unwrap(); + let mut entries = hooks.registrations("session-1", "project", "/", None, "/"); + entries[0].requires_project_trust = true; + registry + .register_session_skill("session-1", "project", hooks.fingerprint(), entries) + .unwrap(); + let engine = AgentHookEngine::with_registry(registry); + assert!(dispatch(&engine, &pre_tool_use_payload("Read")) + .await + .is_blocked()); + assert_eq!( + dispatch( + &engine.with_project_hooks_enabled(false), + &pre_tool_use_payload("Read") + ) + .await + .executed_handlers, + 0 + ); + } + + #[tokio::test] + async fn clearing_a_session_cancels_an_in_flight_handler() { + struct Scratch(std::path::PathBuf); + impl Drop for Scratch { + fn drop(&mut self) { + let _ = std::fs::remove_dir_all(&self.0); + } + } + let root = + Scratch(std::env::temp_dir().join(format!("skill-hook-{}", uuid::Uuid::new_v4()))); + std::fs::create_dir(&root.0).unwrap(); + let registry = RuntimeHookRegistry::default(); + install( + ®istry, + "touch started; exec sleep 10", + false, + "Read", + None, + ); + let engine = AgentHookEngine::with_registry(registry.clone()); + let cwd = root.0.clone(); + let running = + tokio::spawn(async move { engine.dispatch(&pre_tool_use_payload("Read"), &cwd).await }); + tokio::time::timeout(std::time::Duration::from_secs(2), async { + while !root.0.join("started").exists() { + tokio::time::sleep(std::time::Duration::from_millis(5)).await; + } + }) + .await + .unwrap(); + registry.clear_session("session-1"); + let result = tokio::time::timeout(std::time::Duration::from_secs(1), running) + .await + .unwrap() + .unwrap(); + assert!(result.is_blocked()); + assert!( + !AgentHookEngine::with_registry(registry).has_rules_for_session( + AgentHookEvent::PreToolUse, + None, + "session-1" + ) + ); + } + + #[tokio::test] + async fn native_settings_do_not_acquire_skill_ask_semantics() { + let native = engine( + r#"{"hooks":{"PreToolUse":[{"hooks":[{"type":"command","command":"printf '%s' '{\"hookSpecificOutput\":{\"permissionDecision\":\"ask\"}}'"}]}]}}"#, + ); + assert!(dispatch(&native, &pre_tool_use_payload("Read")) + .await + .permission + .is_none()); + } +} diff --git a/src/web-ui/src/app/scenes/skills/hooks/useInstalledSkills.test.tsx b/src/web-ui/src/app/scenes/skills/hooks/useInstalledSkills.test.tsx index 89bb53d351..e3acdf636e 100644 --- a/src/web-ui/src/app/scenes/skills/hooks/useInstalledSkills.test.tsx +++ b/src/web-ui/src/app/scenes/skills/hooks/useInstalledSkills.test.tsx @@ -3,15 +3,15 @@ import React, { act } from 'react'; import { createRoot, type Root } from 'react-dom/client'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; -import type { SkillInfo } from '@/infrastructure/config/types'; +import type { SkillInfo, SkillScanDiagnostic } from '@/infrastructure/config/types'; import { globalEventBus } from '@/infrastructure/event-bus'; import { useInstalledSkills } from './useInstalledSkills'; import type { InstalledFilter } from '../skillsSceneStore'; const getSkillConfigsMock = vi.hoisted(() => vi.fn()); -const translateMock = vi.hoisted(() => (key: string) => key); +const translateMock = vi.hoisted(() => (key: string, options?: { field?: string }) => options?.field ? `${key}:${options.field}` : key); const diagnosticsMock = vi.hoisted(() => ({ - items: [] as Array<{path: string; sourceId: string; message: string}>, + items: [] as SkillScanDiagnostic[], available: true, })); const getGlobalSkillSettingsMock = vi.hoisted(() => vi.fn()); @@ -113,6 +113,43 @@ describe('useInstalledSkills', () => { }); }); + it('reports unsupported skill capabilities without claiming a scan failure', async () => { + diagnosticsMock.items = [{ + path: '/skills/guard/SKILL.md', sourceId: 'claude-code', + message: 'unsupported hooks', unsupportedField: 'hooks', + }]; + await act(async () => root.render()); + expect(currentInstalled?.skills).toEqual([]); + expect(currentInstalled?.diagnostics).toEqual(diagnosticsMock.items); + expect(notificationMocks.warning).not.toHaveBeenCalled(); + expect(notificationMocks.info).toHaveBeenCalledExactlyOnceWith('list.unsupportedSkills', { + title: 'nav.title', + metadata: { diagnostics: '/skills/guard/SKILL.md: list.unsupportedField:hooks' }, + }); + await act(async () => { await currentInstalled?.loadSkills(true); }); + expect(notificationMocks.info).toHaveBeenCalledTimes(1); + }); + + it('keeps read failures separate from unsupported skill capabilities', async () => { + const skill: SkillInfo = { + key: 'user::openbitfun::good', name: 'good', description: '', path: '/skills/good', + level: 'user', sourceId: 'openbitfun', sourceSlot: 'openbitfun', dirName: 'good', isBuiltin: false, + }; + getSkillConfigsMock.mockResolvedValue([skill]); + diagnosticsMock.items = [ + { path: '/skills/missing', sourceId: 'claude-code', message: 'missing target' }, + { path: '/skills/guard/SKILL.md', sourceId: 'claude-code', message: 'unsupported hooks', unsupportedField: 'hooks' }, + ]; + await act(async () => root.render()); + expect(notificationMocks.warning).toHaveBeenCalledExactlyOnceWith('list.scanIncomplete', { + title: 'nav.title', metadata: { diagnostics: '/skills/missing: missing target' }, + }); + expect(notificationMocks.info).toHaveBeenCalledExactlyOnceWith('list.unsupportedSkills', { + title: 'nav.title', metadata: { diagnostics: '/skills/guard/SKILL.md: list.unsupportedField:hooks' }, + }); + expect(currentInstalled?.skills).toEqual([skill]); + }); + it('does not repeat unchanged scan warnings on refresh, but reports changes and recurrence', async () => { diagnosticsMock.items = [ { path: '/skills/first', sourceId: 'openbitfun', message: 'missing target' }, diff --git a/src/web-ui/src/app/scenes/skills/hooks/useInstalledSkills.ts b/src/web-ui/src/app/scenes/skills/hooks/useInstalledSkills.ts index d4dab658ed..8d3b49e734 100644 --- a/src/web-ui/src/app/scenes/skills/hooks/useInstalledSkills.ts +++ b/src/web-ui/src/app/scenes/skills/hooks/useInstalledSkills.ts @@ -106,7 +106,7 @@ export function useInstalledSkills({ ])); const diagnosticKeys = list.diagnostics - .map(({ sourceId, path, message }) => JSON.stringify([sourceId, path, message])) + .map(({ sourceId, path, message, unsupportedField }) => JSON.stringify([sourceId, path, message, unsupportedField])) .sort(); const feedbackKey = JSON.stringify([ capabilityRef.current.key, list.diagnosticsAvailable, diagnosticKeys, @@ -114,17 +114,30 @@ export function useInstalledSkills({ // Gallery focus and tab re-entry refresh the scan; only changed results notify. if (lastScanFeedbackKeyRef.current !== feedbackKey) { lastScanFeedbackKeyRef.current = feedbackKey; - if (list.diagnostics.length > 0) { + const failures = list.diagnostics.filter(item => !item.unsupportedField); + const unsupported = list.diagnostics.filter(item => item.unsupportedField); + if (failures.length > 0) { const message = list.skills.length > 0 ? t('list.scanIncomplete') : t('list.loadFailed'); notifyScanWarning(message, { title: t('nav.title'), metadata: { - diagnostics: list.diagnostics + diagnostics: failures .map(({ path, message }) => `${path}: ${message}`) .join('\n'), }, }); - } else if (!list.diagnosticsAvailable) { + } + if (unsupported.length > 0) { + notifyScanInfo(t('list.unsupportedSkills'), { + title: t('nav.title'), + metadata: { + diagnostics: unsupported + .map(item => `${item.path}: ${t('list.unsupportedField', { field: item.unsupportedField })}`) + .join('\n'), + }, + }); + } + if (list.diagnostics.length === 0 && !list.diagnosticsAvailable) { notifyScanInfo(t('list.diagnosticsUnavailable'), { title: t('nav.title') }); } } diff --git a/src/web-ui/src/flow_chat/components/ChatContextPicker.tsx b/src/web-ui/src/flow_chat/components/ChatContextPicker.tsx index 03c93e0ddd..f47e6e1da8 100644 --- a/src/web-ui/src/flow_chat/components/ChatContextPicker.tsx +++ b/src/web-ui/src/flow_chat/components/ChatContextPicker.tsx @@ -154,6 +154,8 @@ export const ChatContextPicker: React.FC = ({ onAddImage, }) => { const { t } = useTranslation('flow-chat'); + const skillScanFailures = skillDiagnostics.filter(item => !item.unsupportedField); + const unsupportedSkills = skillDiagnostics.filter(item => item.unsupportedField); const [results, setResults] = useState([]); const [sessionResults, setSessionResults] = useState([]); const [workspaceReferences, setWorkspaceReferences] = useState([]); @@ -975,9 +977,23 @@ export const ChatContextPicker: React.FC = ({ {(view === 'skills' || isSearchMode) && !skillsLoading && !skillsLoadFailed && ( - skillDiagnostics.length > 0 ? - {skillDiagnostics.map((item, index) =>

{item.path}: {item.message}

)} - : !skillDiagnosticsAvailable &&

{t('contextPicker.skillsDiagnosticsUnavailable')}

+ <> + {skillScanFailures.length > 0 && ( + + {skillScanFailures.map((item, index) =>

{item.path}: {item.message}

)} + + )} + {unsupportedSkills.length > 0 && ( + + {unsupportedSkills.map((item, index) => ( +

{item.path}: {t('contextPicker.skillsUnsupportedField', { field: item.unsupportedField })}

+ ))} + + )} + {skillDiagnostics.length === 0 && !skillDiagnosticsAvailable && ( +

{t('contextPicker.skillsDiagnosticsUnavailable')}

+ )} + )}
↑↓ {t('contextPicker.navHint')} diff --git a/src/web-ui/src/flow_chat/components/ChatContextPickerOverlay.test.tsx b/src/web-ui/src/flow_chat/components/ChatContextPickerOverlay.test.tsx index 4758a10b4f..dc06b53ce8 100644 --- a/src/web-ui/src/flow_chat/components/ChatContextPickerOverlay.test.tsx +++ b/src/web-ui/src/flow_chat/components/ChatContextPickerOverlay.test.tsx @@ -14,7 +14,7 @@ import { globalThis.IS_REACT_ACT_ENVIRONMENT = true; vi.mock('react-i18next', () => ({ - useTranslation: () => ({ t: (key: string) => key }), + useTranslation: () => ({ t: (key: string, options?: { field?: string }) => options?.field ? `${key}:${options.field}` : key }), })); vi.mock('@/infrastructure/api', () => ({ @@ -53,6 +53,7 @@ interface HarnessProps { skills?: readonly ContextPickerSkill[]; skillsLoading?: boolean; skillsLoadFailed?: boolean; + skillDiagnostics?: ChatContextPickerProps['skillDiagnostics']; onRetrySkills?: ChatContextPickerProps['onRetrySkills']; onSelectSkill?: ChatContextPickerProps['onSelectSkill']; onAddImage?: ChatContextPickerProps['onAddImage']; @@ -69,6 +70,7 @@ const Harness: React.FC = ({ skills, skillsLoading, skillsLoadFailed, + skillDiagnostics, onRetrySkills, onSelectSkill, onAddImage, @@ -96,6 +98,7 @@ const Harness: React.FC = ({ skills={skills} skillsLoading={skillsLoading} skillsLoadFailed={skillsLoadFailed} + skillDiagnostics={skillDiagnostics} onRetrySkills={onRetrySkills} onSelectSkill={onSelectSkill} onAddImage={onAddImage} @@ -111,6 +114,35 @@ const option = (kind: string) => document.querySelector( ); describe('ChatContextPicker overlay', () => { + it('separates unavailable skill capabilities from scan errors in the picker', async () => { + const unsupported = { + path: '/remote/guard/SKILL.md', sourceId: 'claude-code', + message: 'unsupported hooks', unsupportedField: 'hooks', + }; + const failure = { path: '/remote/missing', sourceId: 'claude-code', message: 'missing target' }; + const render = (diagnostics: ChatContextPickerProps['skillDiagnostics']) => act(async () => root.render( + , + )); + await render([unsupported]); + expect(document.body.textContent).toContain('contextPicker.skillsUnsupported'); + expect(document.body.textContent).not.toContain('contextPicker.skillsIncomplete'); + const details = document.querySelector('details'); + await act(async () => details?.querySelector('summary')?.click()); + expect(document.body.textContent).toContain('/remote/guard/SKILL.md'); + expect(document.body.textContent).toContain('contextPicker.skillsUnsupportedField:hooks'); + + await render([unsupported, failure]); + const disclosures = Array.from(document.querySelectorAll('details')); + expect(disclosures).toHaveLength(2); + for (const disclosure of disclosures) { + if (!disclosure.open) await act(async () => disclosure.querySelector('summary')?.click()); + } + expect(disclosures[0].textContent).toContain('missing target'); + expect(disclosures[0].textContent).not.toContain('/remote/guard'); + expect(disclosures[1].textContent).toContain('contextPicker.skillsUnsupportedField:hooks'); + expect(disclosures[1].textContent).not.toContain('missing target'); + }); + const mcpCatalog = { modeRestricted: false, tools: [{ name: 'mcp__docs__search', serverId: 'docs', serverName: 'Docs', toolName: 'search', description: 'Find manuals' }], diff --git a/src/web-ui/src/infrastructure/api/service-api/ConfigAPI.test.ts b/src/web-ui/src/infrastructure/api/service-api/ConfigAPI.test.ts index 26cb0add36..f9ba3c883b 100644 --- a/src/web-ui/src/infrastructure/api/service-api/ConfigAPI.test.ts +++ b/src/web-ui/src/infrastructure/api/service-api/ConfigAPI.test.ts @@ -197,7 +197,10 @@ describe('skill scan response compatibility', () => { }); it('preserves partial inventories and diagnostics from new hosts', async () => { - const value = { skills: [{ key: 'project::codex::pdf' }], diagnostics: [{ path: '/remote/denied', sourceId: 'codex', message: 'permission denied' }] }; + const value = { skills: [{ key: 'project::codex::pdf' }], diagnostics: [ + { path: '/remote/denied', sourceId: 'codex', message: 'permission denied' }, + { path: '/remote/guard/SKILL.md', sourceId: 'claude-code', message: 'unsupported hooks', unsupportedField: 'hooks' }, + ] }; invokeMock.mockResolvedValueOnce(value); expect(await new ConfigAPI().getModeSkillScanReport({ modeId: 'agent', workspaceId: 'remote-workspace-id' })) .toEqual({ ...value, diagnosticsAvailable: true }); diff --git a/src/web-ui/src/infrastructure/config/types/index.ts b/src/web-ui/src/infrastructure/config/types/index.ts index fbac67601b..d7fd233f92 100644 --- a/src/web-ui/src/infrastructure/config/types/index.ts +++ b/src/web-ui/src/infrastructure/config/types/index.ts @@ -468,6 +468,8 @@ export interface SkillScanDiagnostic { path: string; sourceId: string; message: string; + /** Required declaration this host cannot honor; absent on older hosts. */ + unsupportedField?: string | null; } export interface SkillScanReport { diff --git a/src/web-ui/src/locales/en-US/flow-chat.json b/src/web-ui/src/locales/en-US/flow-chat.json index 47376b0c3c..8c3b2a7cfe 100644 --- a/src/web-ui/src/locales/en-US/flow-chat.json +++ b/src/web-ui/src/locales/en-US/flow-chat.json @@ -1089,6 +1089,8 @@ "unsupportedHost": "This host does not support MCP selection. Update the host to use it." }, "skillsIncomplete": "Some skills could not be scanned. Available skills are still listed.", + "skillsUnsupported": "Some skills require unsupported capabilities and were not loaded.", + "skillsUnsupportedField": "This host does not support the skill's {{field}} declaration. The skill was not loaded.", "skillsDiagnosticsUnavailable": "This host does not report skill scan diagnostics.", "menuTitle": "Choose context", "searchResults": "Search Results", diff --git a/src/web-ui/src/locales/en-US/scenes/skills.json b/src/web-ui/src/locales/en-US/scenes/skills.json index f15292d46f..8ef319cd8d 100644 --- a/src/web-ui/src/locales/en-US/scenes/skills.json +++ b/src/web-ui/src/locales/en-US/scenes/skills.json @@ -231,6 +231,8 @@ "globalToggleLabel": "Toggle global availability for {{name}}" }, "scanIncomplete": "Some skills could not be scanned. Available skills are still listed.", + "unsupportedSkills": "Some skills require unsupported capabilities and were not loaded.", + "unsupportedField": "This host does not support the skill's {{field}} declaration. The skill was not loaded.", "diagnosticsUnavailable": "This host does not report skill scan diagnostics." }, "deleteModal": { diff --git a/src/web-ui/src/locales/zh-CN/flow-chat.json b/src/web-ui/src/locales/zh-CN/flow-chat.json index 49799f052c..6f88900f2d 100644 --- a/src/web-ui/src/locales/zh-CN/flow-chat.json +++ b/src/web-ui/src/locales/zh-CN/flow-chat.json @@ -1089,6 +1089,8 @@ "unsupportedHost": "当前主机不支持选择 MCP，请更新主机版本。" }, "skillsIncomplete": "部分技能扫描失败，已成功读取的技能仍可使用。", + "skillsUnsupported": "部分技能依赖暂不支持的能力，未加载。", + "skillsUnsupportedField": "当前主机暂不支持技能内的 {{field}} 声明，该技能未加载。", "skillsDiagnosticsUnavailable": "当前主机不提供技能扫描诊断。", "menuTitle": "选择上下文", "searchResults": "搜索结果", diff --git a/src/web-ui/src/locales/zh-CN/scenes/skills.json b/src/web-ui/src/locales/zh-CN/scenes/skills.json index 9ea6893716..dcdc8cf7e7 100644 --- a/src/web-ui/src/locales/zh-CN/scenes/skills.json +++ b/src/web-ui/src/locales/zh-CN/scenes/skills.json @@ -231,6 +231,8 @@ "globalToggleLabel": "切换 {{name}} 的全局可用状态" }, "scanIncomplete": "部分技能扫描失败，已成功读取的技能仍可使用。", + "unsupportedSkills": "部分技能依赖暂不支持的能力，未加载。", + "unsupportedField": "当前主机暂不支持技能内的 {{field}} 声明，该技能未加载。", "diagnosticsUnavailable": "当前主机不提供技能扫描诊断。" }, "deleteModal": { diff --git a/src/web-ui/src/locales/zh-TW/flow-chat.json b/src/web-ui/src/locales/zh-TW/flow-chat.json index 213a1e9a04..8765f63ae3 100644 --- a/src/web-ui/src/locales/zh-TW/flow-chat.json +++ b/src/web-ui/src/locales/zh-TW/flow-chat.json @@ -1089,6 +1089,8 @@ "unsupportedHost": "目前主機不支援選擇 MCP，請更新主機版本。" }, "skillsIncomplete": "部分技能掃描失敗，已成功讀取的技能仍可使用。", + "skillsUnsupported": "部分技能依賴暫不支援的能力，未載入。", + "skillsUnsupportedField": "目前主機暫不支援技能內的 {{field}} 宣告，該技能未載入。", "skillsDiagnosticsUnavailable": "目前主機不提供技能掃描診斷。", "menuTitle": "選擇上下文", "searchResults": "搜尋結果", diff --git a/src/web-ui/src/locales/zh-TW/scenes/skills.json b/src/web-ui/src/locales/zh-TW/scenes/skills.json index 6038699146..98b910876f 100644 --- a/src/web-ui/src/locales/zh-TW/scenes/skills.json +++ b/src/web-ui/src/locales/zh-TW/scenes/skills.json @@ -231,6 +231,8 @@ "globalToggleLabel": "切換 {{name}} 的全域可用狀態" }, "scanIncomplete": "部分技能掃描失敗，已成功讀取的技能仍可使用。", + "unsupportedSkills": "部分技能依賴暫不支援的能力，未載入。", + "unsupportedField": "目前主機暫不支援技能內的 {{field}} 宣告，該技能未載入。", "diagnosticsUnavailable": "目前主機不提供技能掃描診斷。" }, "deleteModal": {