From 9d1ca08be777a9f0d2927347c9223340ed9871db Mon Sep 17 00:00:00 2001 From: local Date: Tue, 2 Jun 2026 02:28:54 +0800 Subject: [PATCH 1/2] Implement advanced Claude Code skill runtime Co-authored-by: multica-agent --- ...ithub-issue-advanced-claude-code-skills.md | 44 ++ docs/prd-advanced-claude-code-skills.md | 70 +++ .../.openspec.yaml | 2 + .../design.md | 60 +++ .../proposal.md | 28 ++ .../specs/claude-code-skills/spec.md | 73 +++ .../implement-advanced-claude-skills/tasks.md | 11 + src/a2a_server.rs | 4 + src/acp_server.rs | 4 + src/hooks.rs | 15 + src/session.rs | 156 ++++++- src/session_runner.rs | 229 +++++++-- src/skills.rs | 102 +++- tests/session_runner_slash.rs | 438 +++++++++++++++++- tests/skills_discover.rs | 72 ++- 15 files changed, 1257 insertions(+), 51 deletions(-) create mode 100644 docs/github-issue-advanced-claude-code-skills.md create mode 100644 docs/prd-advanced-claude-code-skills.md create mode 100644 openspec/changes/implement-advanced-claude-skills/.openspec.yaml create mode 100644 openspec/changes/implement-advanced-claude-skills/design.md create mode 100644 openspec/changes/implement-advanced-claude-skills/proposal.md create mode 100644 openspec/changes/implement-advanced-claude-skills/specs/claude-code-skills/spec.md create mode 100644 openspec/changes/implement-advanced-claude-skills/tasks.md diff --git a/docs/github-issue-advanced-claude-code-skills.md b/docs/github-issue-advanced-claude-code-skills.md new file mode 100644 index 0000000..7d22f6e --- /dev/null +++ b/docs/github-issue-advanced-claude-code-skills.md @@ -0,0 +1,44 @@ +# Implement Advanced Claude Code Skills Runtime Semantics + +## Problem + +The first Claude Code skills alignment PR covered discovery, frontmatter tolerance, model/user visibility, direct slash invocation, argument substitution, and cwd-to-root project discovery. The remaining advanced semantics are now required: dynamic shell context injection, forked subagent-style execution, and skill-scoped model/tool/hook enforcement. + +## Proposed Scope + +- Parse and retain advanced skill frontmatter fields: + - `model` + - `effort` + - `allowed-tools` + - `disallowed-tools` + - `hooks` + - `context` + - `agent` + - `shell` +- Render dynamic shell context before direct skill invocation reaches the model: + - inline `` !`command` `` + - fenced ` ```! ` command blocks +- Run shell context commands from the session cwd using a deterministic shell choice. +- Support `agent: fork` by running the skill in an isolated child transcript and appending only the final assistant result to the parent transcript. +- Enforce `allowed-tools` and `disallowed-tools` for the invoked skill only. +- Run skill-scoped hooks for the invoked skill only, in addition to normal session hooks. +- Override the model for the invoked skill only when the named model is available from the host model factory. +- Add focused tests for all required runtime behaviors. + +## Out Of Scope + +- Filesystem watching or live rediscovery after startup. +- Adding new model-effort provider APIs. +- Expanding the global config format beyond what is needed to run skill-scoped frontmatter. + +## Acceptance Criteria + +- A directly invoked skill containing `` !`printf context` `` sends `context` to the model in place of the inline expression. +- A directly invoked skill containing a fenced ` ```! ` block sends the command output to the model in place of the command block. +- Shell context command failures are visible in the rendered prompt as an error marker. +- `agent: fork` leaves the parent transcript free of the skill's internal user prompt while preserving the final assistant result. +- `allowed-tools` limits the tool specs advertised to the model and blocks out-of-scope tool execution. +- `disallowed-tools` removes denied tools even when the allow list would include them. +- Skill-scoped hooks can block a tool call during that skill invocation and do not remain active afterward. +- `model` selects the requested model for that skill invocation and restores the previous model afterward. +- `cargo fmt --check`, focused tests, full `cargo test`, and `openspec validate implement-advanced-claude-skills --strict` pass. diff --git a/docs/prd-advanced-claude-code-skills.md b/docs/prd-advanced-claude-code-skills.md new file mode 100644 index 0000000..c8678ae --- /dev/null +++ b/docs/prd-advanced-claude-code-skills.md @@ -0,0 +1,70 @@ +# PRD: Advanced Claude Code Skills Runtime Alignment + +## Overview / Problem Statement + +Ra now supports the basic Claude Code skill shape, but advanced Claude Code skills still lose important runtime semantics. Users with existing skills expect dynamic shell context injection, isolated subagent-style execution, and skill-scoped model, tool, and hook constraints to affect the actual invocation rather than being silently ignored. + +## Goals & Success Metrics + +- Direct `/skill-name` invocation renders dynamic shell context before the model sees the skill prompt. +- Skills that request forked execution run against an isolated copy of the conversation and do not mutate the parent session transcript with internal subagent turns. +- Skills that declare `model`, `allowed-tools`, `disallowed-tools`, or `hooks` apply those constraints for that skill invocation only. +- Existing prompt-template behavior and previously aligned skill discovery/frontmatter behavior remain backward-compatible. +- Focused Rust tests cover dynamic shell rendering, scoped tool filtering, scoped hooks, scoped model selection, and fork transcript isolation. + +## User Personas & Stories + +- As a Claude Code skill author, I want `` !`command` `` and fenced ` ```! ` blocks to inject command output so that skills can include live project state. +- As an agent operator, I want high-risk skills to restrict tools and hooks at invocation time so that local policy travels with the skill. +- As a runtime integrator, I want forked skill execution so that exploratory skill work can produce a result without polluting the parent session history. + +## Functional Requirements + +| Priority | Requirement | +| --- | --- | +| Must | Parse and persist skill frontmatter for `model`, `allowed-tools`, `disallowed-tools`, `hooks`, `context`, `agent`, and `shell`. | +| Must | Replace inline `` !`command` `` expressions with captured stdout before the skill prompt is submitted. | +| Must | Replace fenced ` ```! ` command blocks with captured stdout before the skill prompt is submitted. | +| Must | Run shell context commands from the current session cwd, using `/bin/sh -c` by default and `bash -lc` when `shell: bash` is declared. | +| Must | Insert a readable error marker when a shell context command exits unsuccessfully instead of aborting the entire skill invocation. | +| Must | Support `agent: fork` as an isolated skill execution mode that runs on a child transcript snapshot and appends only the final assistant result to the parent transcript. | +| Must | Apply `allowed-tools` as an invocation-scoped allow list for tool specs and execution. | +| Must | Apply `disallowed-tools` as an invocation-scoped deny list on top of the allow list. | +| Must | Apply skill-scoped `hooks` in addition to session hooks for that invocation. | +| Must | Apply `model` as an invocation-scoped model override when the host can build the named model. | +| Should | Treat unsupported shell names as the default shell and include the declaration in tests/docs as best-effort compatibility. | +| Could | Parse `effort` for future model parameters without changing the model trait in this change. | +| Won't | Implement live filesystem watching or automatic nested skill discovery during an already-running session. | + +## Non-Functional Requirements + +- Scope changes to skill rendering and the shared session runner/session path. +- Preserve deterministic behavior in tests without network calls. +- Avoid weakening existing session-level hooks and tool filters. +- Keep failed shell context commands visible to the model for debugging. + +## Design Considerations + +The user-facing behavior should be compatible where Ra has the necessary runtime surfaces today. Skill-scoped behavior should be temporary and should restore the parent session model, hooks, tool visibility, and transcript after the invocation completes. + +## Technical Considerations + +The implementation will extend `Skill`/`SlashTemplate`, `SessionRunner`, and `Session`. `RunnerHost` will expose model construction so the runner can honor `model` overrides without making protocol-specific code leak into skill rendering. Session-level scoped runtime state will filter advertised and executable tools and combine hooks for a single invocation. + +## Timeline & Milestones + +| Milestone | Owner | Target | +| --- | --- | --- | +| Updated PRD, issue draft, and OpenSpec change | Agent | Before implementation | +| Runtime implementation and focused tests | Agent | Same PR | +| New GitHub issue and PR | Agent | After validation | + +## Open Questions & Risks + +- Claude Code's exact internal fork/subagent transcript behavior is not public API. Ra will implement a practical equivalent: cloned parent context for the skill and parent transcript isolation except for the final skill result. +- Skill-scoped model overrides depend on configured model IDs. Unknown model IDs should fail visibly rather than silently using the wrong model. +- Shell context injection executes local commands and therefore inherits Ra's existing local execution risk profile. + +## Appendix + +Reference: current Claude Code skills documentation at `https://code.claude.com/docs/en/skills.md` and `https://code.claude.com/docs/zh-CN/skills.md`, checked during this change. diff --git a/openspec/changes/implement-advanced-claude-skills/.openspec.yaml b/openspec/changes/implement-advanced-claude-skills/.openspec.yaml new file mode 100644 index 0000000..a2168c3 --- /dev/null +++ b/openspec/changes/implement-advanced-claude-skills/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-06-01 diff --git a/openspec/changes/implement-advanced-claude-skills/design.md b/openspec/changes/implement-advanced-claude-skills/design.md new file mode 100644 index 0000000..1b589df --- /dev/null +++ b/openspec/changes/implement-advanced-claude-skills/design.md @@ -0,0 +1,60 @@ +## Context + +Ra's current `Skill` model already tolerates advanced Claude Code fields during YAML parsing, but it discards them. `ResourceBundle::prompt_map` converts user-invocable skills into `SlashTemplate` values consumed by `SessionRunner`, and the runner sends the rendered template through `Session::prompt`. `Session` owns the model, tool catalog, message log, cwd, and hook engine. + +## Goals / Non-Goals + +**Goals:** + +- Preserve advanced skill frontmatter in the runtime template. +- Render dynamic shell context deterministically from the session cwd. +- Apply model/tool/hook settings only to the directly invoked skill turn. +- Implement a practical forked skill execution mode that isolates parent transcript history from internal skill turns. +- Keep existing non-skill prompt-template behavior unchanged. + +**Non-Goals:** + +- Do not implement provider-specific model effort controls. +- Do not add live skill rediscovery while a session is already running. +- Do not reinterpret Claude Code permissions beyond tool-name and simple `Tool(pattern)` declarations. + +## Decisions + +### Store Runtime Options On Skill Templates + +`Skill` and `SlashTemplate` will carry a `SkillRuntimeOptions` struct containing optional model, effort, shell, agent mode, allowed/disallowed tool declarations, and hooks. Prompt templates keep `None`, so legacy prompt commands remain unaffected. + +### Render Shell Context In The Runner + +Dynamic context syntax is prompt rendering, not model behavior. The runner already owns direct slash invocation and has access to session cwd, so it will replace inline `` !`command` `` and fenced ` ```! ` blocks after argument substitution and before calling the session. + +Unsuccessful shell commands should become readable error markers in the rendered prompt. This preserves debuggability and avoids unexpectedly aborting the entire skill invocation. + +### Use Scoped Session Runtime State + +`Session` will expose a scoped invocation method that takes an optional runtime override. During that invocation, it will: + +- temporarily select an override model when provided, +- filter advertised tool specs and executable tool lookup using an allow/deny policy, +- merge skill hooks with session hooks, +- restore the previous runtime state after completion. + +The scope is held in the async prompt path and is not persisted into the message log. + +### Extend RunnerHost For Model Resolution + +`RunnerHost` already abstracts host behavior needed by `SessionRunner`. Add a `build_model_for_id` method with a default `None` implementation so ACP can resolve model IDs via its existing factory, while tests and other hosts can opt in without changing protocol code. + +Unknown model IDs should fail the skill invocation visibly. Silent fallback to the default model would violate the skill author's explicit runtime policy. + +### Forked Skill Execution Uses Transcript Snapshot Isolation + +For `agent: fork`, the runner will run the rendered skill prompt against a child session state initialized from the parent transcript snapshot. After the child finishes, the parent transcript is restored to its pre-skill state and receives only the final assistant text produced by the child. Tool calls and intermediate skill messages remain isolated from the parent transcript. + +This matches the operational need for fork/subagent behavior with Ra's current single-session architecture and avoids protocol-specific session creation in the shared runner. + +## Risks / Trade-offs + +- Tool declarations in Claude Code can include richer permission patterns than Ra tool names. The first implementation will enforce by normalized tool name and `Tool(pattern)` prefix, which covers the current Ra tool-spec surface. +- Forked execution cannot perfectly emulate Claude Code internals without a public transcript contract. Tests will lock Ra's defined behavior: parent snapshot in, final assistant result out. +- Shell context injection executes local commands before the model call. That is expected for Claude Code-compatible skills and is contained to direct skill invocation. diff --git a/openspec/changes/implement-advanced-claude-skills/proposal.md b/openspec/changes/implement-advanced-claude-skills/proposal.md new file mode 100644 index 0000000..de740af --- /dev/null +++ b/openspec/changes/implement-advanced-claude-skills/proposal.md @@ -0,0 +1,28 @@ +## Why + +The merged Claude Code skills alignment made existing skills discoverable and directly invocable, but it still ignores advanced runtime semantics that users now require. Advanced Claude Code skills often depend on live shell context, isolated/forked execution, and per-skill model/tool/hook policy; treating those fields as inert metadata causes behavior drift and can bypass author intent. + +## What Changes + +- Parse and retain advanced Claude Code skill frontmatter fields used at runtime. +- Render inline and fenced dynamic shell context blocks before a skill prompt reaches the model. +- Add invocation-scoped runtime options for direct skill slash commands. +- Support `agent: fork` by running a skill against an isolated child transcript and returning only the final result to the parent session. +- Enforce skill-scoped model overrides, tool allow/deny lists, and hooks for the duration of a skill invocation. +- Add PRD, GitHub issue draft, OpenSpec requirements, focused tests, and documentation comments for the new runtime behavior. + +## Capabilities + +### New Capabilities + +- None. + +### Modified Capabilities + +- `claude-code-skills`: Extend previously aligned skill discovery/invocation behavior with advanced runtime semantics for shell context, forked execution, and scoped model/tool/hook policy. + +## Impact + +- Affects `src/skills.rs`, `src/session_runner.rs`, `src/session.rs`, `src/hooks.rs`, protocol runner host implementations, docs, and tests. +- Adds no new external runtime dependency. +- Keeps existing prompt-template and basic skill invocation behavior backward-compatible. diff --git a/openspec/changes/implement-advanced-claude-skills/specs/claude-code-skills/spec.md b/openspec/changes/implement-advanced-claude-skills/specs/claude-code-skills/spec.md new file mode 100644 index 0000000..2cc7346 --- /dev/null +++ b/openspec/changes/implement-advanced-claude-skills/specs/claude-code-skills/spec.md @@ -0,0 +1,73 @@ +## MODIFIED Requirements + +### Requirement: Claude Code Frontmatter Compatibility +Ra SHALL parse Claude Code skills with optional `name` and `description` frontmatter fields and retain advanced runtime frontmatter fields for direct invocation. + +#### Scenario: Missing name falls back to directory command +- **WHEN** a skill at `.claude/skills/release/SKILL.md` omits frontmatter `name` +- **THEN** Ra uses `release` as the skill command name and display fallback + +#### Scenario: Missing description falls back to markdown body +- **WHEN** a skill omits frontmatter `description` +- **THEN** Ra uses the first non-empty markdown paragraph as the model-facing description + +#### Scenario: Additional Claude Code fields are retained +- **WHEN** a skill contains Claude Code frontmatter fields such as `model`, `allowed-tools`, `disallowed-tools`, `hooks`, `context`, `agent`, or `shell` +- **THEN** Ra loads the skill and carries those fields into the slash invocation runtime template + +### Requirement: Direct Skill Invocation +Ra SHALL expose user-invocable skills as slash commands that expand to the skill body through the existing prompt-template dispatch path. + +#### Scenario: Skill slash command expands to body +- **WHEN** the user enters `/deploy staging` +- **THEN** Ra sends the `deploy` skill body to the model and includes `ARGUMENTS: staging` when the skill body has no argument placeholders + +#### Scenario: Non-user-invocable skill is hidden from slash map +- **WHEN** a skill has `user-invocable: false` +- **THEN** Ra does not expose it as a slash-command template + +#### Scenario: Skill arguments replace placeholders +- **WHEN** the user enters `/migrate SearchBar React Vue` and the skill body contains `$ARGUMENTS`, `$ARGUMENTS[1]`, `$0`, or named argument placeholders such as `$component` +- **THEN** Ra replaces those placeholders before sending the rendered prompt to the model + +#### Scenario: Inline dynamic shell context is rendered +- **WHEN** a directly invoked skill body contains `` !`printf context` `` +- **THEN** Ra replaces the expression with the captured command output before sending the prompt to the model + +#### Scenario: Fenced dynamic shell context is rendered +- **WHEN** a directly invoked skill body contains a fenced ` ```! ` command block +- **THEN** Ra replaces the block with the captured command output before sending the prompt to the model + +#### Scenario: Failed dynamic shell command remains visible +- **WHEN** a dynamic shell context command exits unsuccessfully +- **THEN** Ra includes a readable command failure marker in the rendered prompt + +### Requirement: Skill Scoped Runtime Enforcement +Ra SHALL enforce supported skill-scoped runtime fields for a direct skill invocation only. + +#### Scenario: Skill scoped allowed tools limit model tool specs +- **WHEN** a directly invoked skill declares `allowed-tools: [read]` +- **THEN** the model receives only the `read` tool spec for that invocation + +#### Scenario: Skill scoped disallowed tools deny execution +- **WHEN** a directly invoked skill declares `disallowed-tools: [bash]` +- **THEN** a model-requested `bash` tool call is returned as an error instead of executing + +#### Scenario: Skill scoped hooks are temporary +- **WHEN** a directly invoked skill declares a PreToolUse hook that denies `bash` +- **THEN** the `bash` call is denied during that skill invocation and the hook does not affect later non-skill prompts + +#### Scenario: Skill scoped model override is temporary +- **WHEN** a directly invoked skill declares `model: review-model` and the host can build that model +- **THEN** Ra uses `review-model` for the skill invocation and restores the prior model after it completes + +### Requirement: Forked Skill Invocation +Ra SHALL support direct skill invocation with isolated fork/subagent-style transcript behavior. + +#### Scenario: Forked skill isolates internal transcript +- **WHEN** a directly invoked skill declares `agent: fork` +- **THEN** Ra runs the skill against a child transcript initialized from the parent snapshot and restores the parent transcript after the child run + +#### Scenario: Forked skill returns final result to parent +- **WHEN** a forked skill completes with assistant text +- **THEN** Ra appends the final assistant result to the parent transcript without appending the skill's internal user prompt diff --git a/openspec/changes/implement-advanced-claude-skills/tasks.md b/openspec/changes/implement-advanced-claude-skills/tasks.md new file mode 100644 index 0000000..c09152e --- /dev/null +++ b/openspec/changes/implement-advanced-claude-skills/tasks.md @@ -0,0 +1,11 @@ +# Tasks + +- [x] Add updated PRD and GitHub issue draft artifacts. +- [x] Add OpenSpec proposal, design, requirement delta, and task artifacts. +- [x] Extend skill parsing/templates with advanced runtime frontmatter. +- [x] Render dynamic shell context for inline and fenced command syntax. +- [x] Add invocation-scoped model, tool, and hook enforcement in the shared session path. +- [x] Implement `agent: fork` transcript isolation for direct skill invocation. +- [x] Add focused tests for shell context, scoped enforcement, scoped model selection, and fork behavior. +- [x] Run formatting, focused tests, full tests, and strict OpenSpec validation. +- [ ] Open a new GitHub issue and PR for review. diff --git a/src/a2a_server.rs b/src/a2a_server.rs index 08d65de..b01fe17 100644 --- a/src/a2a_server.rs +++ b/src/a2a_server.rs @@ -184,6 +184,10 @@ impl RunnerHost for A2aState { // typically don't type slash commands, so we stay terse. vec![(self.model_factory.default_model_id(), "Default".into())] } + + fn build_model_for_id(&self, model_id: &str) -> Option> { + self.model_factory.build(model_id) + } } /// The A2A executor. Each incoming `SendMessage` / `SendStreamingMessage` diff --git a/src/acp_server.rs b/src/acp_server.rs index c86bed8..af561ba 100644 --- a/src/acp_server.rs +++ b/src/acp_server.rs @@ -303,6 +303,10 @@ impl RunnerHost for SharedState { .map(|m| (m.model_id.to_string(), m.name.clone())) .collect() } + + fn build_model_for_id(&self, model_id: &str) -> Option> { + self.model_factory.build(model_id) + } } /// Bridge between Ra's `ClientHandle` trait and the ACP `ConnectionTo`. diff --git a/src/hooks.rs b/src/hooks.rs index e103656..6dc2b44 100644 --- a/src/hooks.rs +++ b/src/hooks.rs @@ -211,6 +211,21 @@ impl HookEngine { && self.stop.is_empty() } + pub fn merged_with(&self, other: &HookEngine) -> HookEngine { + HookEngine { + pre: Arc::new(self.pre.iter().chain(other.pre.iter()).cloned().collect()), + post: Arc::new(self.post.iter().chain(other.post.iter()).cloned().collect()), + submit: Arc::new( + self.submit + .iter() + .chain(other.submit.iter()) + .cloned() + .collect(), + ), + stop: Arc::new(self.stop.iter().chain(other.stop.iter()).cloned().collect()), + } + } + /// Fire every PreToolUse hook whose matcher hits `tool_name`. Returns /// the merged decision (block / additional_context / stop). pub async fn pre_tool_use( diff --git a/src/session.rs b/src/session.rs index 361d88e..e9c418a 100644 --- a/src/session.rs +++ b/src/session.rs @@ -54,6 +54,19 @@ pub struct Session { /// can pre-route through `rtk rewrite`. Default /// (`RtkRewriter::default()`) is a no-op pass-through. rtk: crate::tools::RtkRewriter, + /// Invocation-scoped runtime constraints for direct skill execution. + runtime_scope: Arc>>, + /// Serializes prompt invocations so scoped overrides cannot overlap. + prompt_lock: Arc>, +} + +/// Temporary runtime controls used for a single `prompt()` invocation. +#[derive(Clone)] +pub struct SessionRuntimeScope { + pub model: Option>, + pub allowed_tools: Vec, + pub disallowed_tools: Vec, + pub hooks: Option>, } /// Result of one `prompt()` call. @@ -87,6 +100,8 @@ impl Session { hooks: None, file_approver: None, rtk: crate::tools::RtkRewriter::default(), + runtime_scope: Arc::new(Mutex::new(None)), + prompt_lock: Arc::new(Mutex::new(())), } } @@ -127,6 +142,17 @@ impl Session { self.hooks.as_ref() } + pub fn effective_hooks_for_scope( + &self, + scope: Option<&SessionRuntimeScope>, + ) -> Option> { + effective_hooks(self.hooks.as_ref(), scope) + } + + pub fn cwd(&self) -> &std::path::Path { + &self.cwd + } + /// Set or replace the system-style preamble injected at the head of /// every turn's history. Pass an empty string to clear. pub async fn set_system_prompt(&self, sp: impl Into) { @@ -254,6 +280,56 @@ impl Session { /// Send a user message and run the turn loop to completion or cancellation. pub async fn prompt(&self, user_text: impl Into) -> Result { + let _prompt_guard = self.prompt_lock.clone().lock_owned().await; + self.prompt_unlocked(user_text.into()).await + } + + /// Send a user message with temporary runtime controls. The controls apply + /// only to this invocation and are cleared even if the turn returns an + /// error. + pub async fn prompt_scoped( + &self, + user_text: impl Into, + scope: SessionRuntimeScope, + ) -> Result { + let prompt_guard = self.prompt_lock.clone().lock_owned().await; + *self.runtime_scope.lock().await = Some(scope); + let result = self.prompt_unlocked(user_text.into()).await; + *self.runtime_scope.lock().await = None; + drop(prompt_guard); + result + } + + /// Run a prompt against a child transcript initialized from the current + /// parent transcript, then restore the parent and append only the child's + /// final assistant text. Used by `agent: fork` skill invocation. + pub async fn prompt_forked( + &self, + user_text: impl Into, + scope: Option, + ) -> Result { + let _prompt_guard = self.prompt_lock.clone().lock_owned().await; + let parent_snapshot = self.snapshot_messages().await; + if let Some(scope) = scope { + *self.runtime_scope.lock().await = Some(scope); + } + let result = self.prompt_unlocked(user_text.into()).await; + *self.runtime_scope.lock().await = None; + let child_messages = self.snapshot_messages().await; + self.restore_messages(parent_snapshot).await; + let outcome = result?; + if let Some(final_text) = final_assistant_text(&child_messages) { + let mut restored = self.snapshot_messages().await; + restored.push(Message::Assistant { + content: final_text, + tool_calls: Vec::new(), + }); + self.restore_messages(restored).await; + } + Ok(outcome) + } + + async fn prompt_unlocked(&self, user_text: String) -> Result { // Re-arm the cancel token for this prompt invocation. let token = { let mut guard = self.cancel.lock().await; @@ -261,9 +337,10 @@ impl Session { guard.clone() }; - self.messages.lock().await.push(Message::User { - content: user_text.into(), - }); + self.messages + .lock() + .await + .push(Message::User { content: user_text }); let _ = self.tx.send(Event::AgentStart); let outcome = tokio::select! { @@ -309,16 +386,22 @@ impl Session { } else { history }; + let scope = self.runtime_scope.lock().await.clone(); let specs: Vec = self .tools .values() + .filter(|t| tool_allowed(t.name(), scope.as_ref())) .map(|t| ToolSpec { name: t.name().to_string(), description: t.description().to_string(), parameters: t.schema(), }) .collect(); - let model = self.model.read().await.clone(); + let model = if let Some(model) = scope.as_ref().and_then(|s| s.model.clone()) { + model + } else { + self.model.read().await.clone() + }; // ATOF: scope the whole streamed response from the model layer. // Drops at the end of the stream-consumption loop, before tool // execution starts, so each tool gets its own sibling Tool scope @@ -357,6 +440,16 @@ impl Session { }); for call in pending_calls { + if !tool_allowed(&call.name, scope.as_ref()) { + let result = ToolResult { + call_id: call.id.clone(), + is_error: true, + content: format!("tool `{}` denied by skill-scoped tool policy", call.name), + }; + let _ = self.tx.send(Event::ToolCallEnd(result.clone())); + self.messages.lock().await.push(Message::ToolResult(result)); + continue; + } let tool = self .tools .get(&call.name) @@ -364,7 +457,8 @@ impl Session { .clone(); // PreToolUse hook: any deny short-circuits the tool entirely. - let pre_decision = if let Some(h) = &self.hooks { + let scoped_hooks = self.effective_hooks_for_scope(scope.as_ref()); + let pre_decision = if let Some(h) = &scoped_hooks { h.pre_tool_use(self.session_id.as_deref(), &call.name, &call.input) .await } else { @@ -410,7 +504,7 @@ impl Session { } // PostToolUse hook: may block, kill, or append context. - if let Some(h) = &self.hooks { + if let Some(h) = &scoped_hooks { let post = h .post_tool_use( self.session_id.as_deref(), @@ -439,3 +533,53 @@ impl Session { Ok(stop) } } + +fn effective_hooks( + session_hooks: Option<&Arc>, + scope: Option<&SessionRuntimeScope>, +) -> Option> { + match (session_hooks, scope.and_then(|s| s.hooks.as_ref())) { + (Some(base), Some(extra)) => Some(Arc::new(base.merged_with(extra))), + (Some(base), None) => Some(base.clone()), + (None, Some(extra)) => Some(extra.clone()), + (None, None) => None, + } +} + +fn tool_allowed(name: &str, scope: Option<&SessionRuntimeScope>) -> bool { + let Some(scope) = scope else { + return true; + }; + if !scope.allowed_tools.is_empty() + && !scope + .allowed_tools + .iter() + .any(|decl| tool_decl_matches(decl, name)) + { + return false; + } + !scope + .disallowed_tools + .iter() + .any(|decl| tool_decl_matches(decl, name)) +} + +fn tool_decl_matches(decl: &str, name: &str) -> bool { + let decl = decl.trim(); + if decl == "*" || decl == name { + return true; + } + let head = decl + .split_once('(') + .map(|(tool, _)| tool) + .unwrap_or(decl) + .trim(); + head == name +} + +fn final_assistant_text(messages: &[Message]) -> Option { + messages.iter().rev().find_map(|msg| match msg { + Message::Assistant { content, .. } if !content.is_empty() => Some(content.clone()), + _ => None, + }) +} diff --git a/src/session_runner.rs b/src/session_runner.rs index c27adb4..b9b004c 100644 --- a/src/session_runner.rs +++ b/src/session_runner.rs @@ -15,14 +15,17 @@ use crate::events::Event as RaEvent; use crate::model::Message as RaMessage; +use crate::model::Model; use crate::nemo_obs; -use crate::session::{PromptOutcome, Session}; -use crate::skills::SlashTemplate; -use anyhow::Result; +use crate::session::{PromptOutcome, Session, SessionRuntimeScope}; +use crate::skills::{SkillRuntimeOptions, SlashTemplate}; +use anyhow::{Context, Result}; use async_trait::async_trait; use futures::future::BoxFuture; use std::collections::HashMap; +use std::path::Path; use std::sync::Arc; +use tokio::process::Command; /// Hint to clients about the kind of work a tool does. Maps to ACP `ToolKind` /// and A2A artifact roles, but stays protocol-neutral here. @@ -94,6 +97,11 @@ pub trait RunnerHost: Send + Sync { /// Names + ids of advertised models, used by `/models` slash command. fn list_models_for_display(&self) -> Vec<(String, String)>; + + /// Resolve a model id for invocation-scoped skill overrides. + fn build_model_for_id(&self, _model_id: &str) -> Option> { + None + } } /// Slash command parsed from a user message. @@ -206,14 +214,46 @@ impl SessionRunner { { on_event(RunnerEvent::Started); - // UserPromptSubmit hook (if any). A block short-circuits the - // whole turn loop and surfaces the reason as agent text. - // `additionalContext` is prepended as a system-style preamble to - // the user prompt so the model sees it on the same turn. - let mut user_text = user_text; - if let Some(hooks) = self.session.hooks() { + let template_names: Vec<&str> = self.prompt_templates.keys().map(|s| s.as_str()).collect(); + let mut effective_text = user_text; + let mut skill_runtime = None; + if let Some(cmd) = parse_slash_command(&effective_text, &template_names) { + // Built-in slash commands run server-side; user-defined prompt + // templates expand into a fresh prompt that *does* hit the LLM. + if SLASH_COMMANDS.contains(&cmd.name.as_str()) { + self.run_slash(&cmd, on_event).await; + return RunOutcome::Completed; + } + if let Some(template) = self.prompt_templates.get(&cmd.name).cloned() { + effective_text = render_slash_template(&template, &cmd.args); + skill_runtime = template.runtime.clone(); + if let Some(runtime) = &skill_runtime { + match render_dynamic_shell_context(effective_text, runtime, self.session.cwd()) + .await + { + Ok(rendered) => effective_text = rendered, + Err(e) => return RunOutcome::Failed(format!("{e:#}")), + } + if let Some(context) = runtime.context.as_ref().filter(|s| !s.is_empty()) { + effective_text = format!("[skill context]\n{context}\n\n{effective_text}"); + } + } + } + } + + let scope = match skill_runtime.as_ref() { + Some(runtime) => match self.runtime_scope_for(runtime).await { + Ok(scope) => Some(scope), + Err(e) => return RunOutcome::Failed(format!("{e:#}")), + }, + None => None, + }; + + // UserPromptSubmit hooks run after slash expansion so skill-scoped + // hooks observe the same prompt that will reach the model. + if let Some(hooks) = self.session.effective_hooks_for_scope(scope.as_ref()) { let decision = hooks - .user_prompt_submit(Some(&self.session_id), &user_text) + .user_prompt_submit(Some(&self.session_id), &effective_text) .await; if let Some(reason) = decision.stop.clone() { on_event(RunnerEvent::TextDelta(format!( @@ -230,29 +270,26 @@ impl SessionRunner { return RunOutcome::Failed(reason); } if let Some(extra) = decision.additional_context { - user_text = format!("[hook context]\n{extra}\n\n{user_text}"); - } - } - - let template_names: Vec<&str> = self.prompt_templates.keys().map(|s| s.as_str()).collect(); - let mut effective_text = user_text; - if let Some(cmd) = parse_slash_command(&effective_text, &template_names) { - // Built-in slash commands run server-side; user-defined prompt - // templates expand into a fresh prompt that *does* hit the LLM. - if SLASH_COMMANDS.contains(&cmd.name.as_str()) { - self.run_slash(&cmd, on_event).await; - return RunOutcome::Completed; - } - if let Some(template) = self.prompt_templates.get(&cmd.name).cloned() { - effective_text = render_slash_template(&template, &cmd.args); + effective_text = format!("[hook context]\n{extra}\n\n{effective_text}"); } } // Subscribe BEFORE prompt() so we don't miss the first events. let mut rx = self.session.subscribe(); let session = self.session.clone(); - let prompt_fut: BoxFuture<'_, Result> = - Box::pin(async move { session.prompt(effective_text).await }); + let forked = skill_runtime + .as_ref() + .map(|runtime| runtime.is_fork()) + .unwrap_or(false); + let prompt_fut: BoxFuture<'_, Result> = Box::pin(async move { + if forked { + session.prompt_forked(effective_text, scope).await + } else if let Some(scope) = scope { + session.prompt_scoped(effective_text, scope).await + } else { + session.prompt(effective_text).await + } + }); // Pump events from the broadcast channel into the callback while // the prompt future runs concurrently. Stops when AgentEnd is @@ -332,6 +369,32 @@ impl SessionRunner { outcome.unwrap_or(RunOutcome::Completed) } + async fn runtime_scope_for( + &self, + runtime: &SkillRuntimeOptions, + ) -> Result { + let model = match runtime.model.as_deref() { + Some(model_id) => Some( + self.host + .build_model_for_id(model_id) + .with_context(|| format!("skill requested unknown model `{model_id}`"))?, + ), + None => None, + }; + let skill_hooks = crate::hooks::HookEngine::from_config(&runtime.hooks); + let hooks = if skill_hooks.is_empty() { + None + } else { + Some(Arc::new(skill_hooks)) + }; + Ok(SessionRuntimeScope { + model, + allowed_tools: runtime.allowed_tools.clone(), + disallowed_tools: runtime.disallowed_tools.clone(), + hooks, + }) + } + async fn run_slash(&self, cmd: &SlashCommand, on_event: &mut F) where F: FnMut(RunnerEvent) + Send, @@ -434,6 +497,118 @@ fn split_slash_args(args: &str) -> Vec { args.split_whitespace().map(ToOwned::to_owned).collect() } +async fn render_dynamic_shell_context( + text: String, + runtime: &SkillRuntimeOptions, + cwd: &Path, +) -> Result { + let text = render_fenced_shell_context(text, runtime, cwd).await?; + render_inline_shell_context(text, runtime, cwd).await +} + +async fn render_fenced_shell_context( + text: String, + runtime: &SkillRuntimeOptions, + cwd: &Path, +) -> Result { + let mut out = String::new(); + let mut rest = text.as_str(); + while let Some(start) = rest.find("```!") { + out.push_str(&rest[..start]); + let after_marker = &rest[start + 4..]; + let command_start = after_marker + .strip_prefix("\r\n") + .map(|s| (s, 2usize)) + .or_else(|| after_marker.strip_prefix('\n').map(|s| (s, 1usize))); + let (after_newline, consumed_newline) = match command_start { + Some(pair) => pair, + None => { + out.push_str("```!"); + rest = after_marker; + continue; + } + }; + if let Some(end) = after_newline.find("\n```") { + let command = &after_newline[..end]; + out.push_str(&run_shell_context(command, runtime, cwd).await); + let after_end = &after_newline[end + 4..]; + rest = if let Some(stripped) = after_end.strip_prefix("\r\n") { + stripped + } else if let Some(stripped) = after_end.strip_prefix('\n') { + stripped + } else { + after_end + }; + } else { + out.push_str("```!"); + out.push_str(&after_marker[..consumed_newline]); + rest = after_newline; + } + } + out.push_str(rest); + Ok(out) +} + +async fn render_inline_shell_context( + text: String, + runtime: &SkillRuntimeOptions, + cwd: &Path, +) -> Result { + let mut out = String::new(); + let mut rest = text.as_str(); + while let Some(start) = rest.find("!`") { + out.push_str(&rest[..start]); + let after_marker = &rest[start + 2..]; + if let Some(end) = after_marker.find('`') { + let command = &after_marker[..end]; + out.push_str(&run_shell_context(command, runtime, cwd).await); + rest = &after_marker[end + 1..]; + } else { + out.push_str("!`"); + rest = after_marker; + break; + } + } + out.push_str(rest); + Ok(out) +} + +async fn run_shell_context(command: &str, runtime: &SkillRuntimeOptions, cwd: &Path) -> String { + let command = command.trim(); + if command.is_empty() { + return String::new(); + } + let shell = runtime.shell.as_deref().unwrap_or(""); + let mut cmd = if shell.eq_ignore_ascii_case("bash") { + let mut c = Command::new("bash"); + c.arg("-lc").arg(command); + c + } else { + let mut c = Command::new("/bin/sh"); + c.arg("-c").arg(command); + c + }; + let output = cmd.current_dir(cwd).output().await; + match output { + Ok(output) => { + let mut combined = String::from_utf8_lossy(&output.stdout).into_owned(); + if !output.stderr.is_empty() { + combined.push_str(&String::from_utf8_lossy(&output.stderr)); + } + if output.status.success() { + combined + } else { + format!( + "[shell context command failed: `{command}` exited with {}]\n{}", + output.status.code().unwrap_or(-1), + combined + ) + } + } + Err(e) => format!("[shell context command failed: `{command}`: {e}]"), + } +} + fn tool_kind(name: &str) -> ToolKindHint { match name { "read" | "grep" | "glob" | "ls" | "fuzzy" => ToolKindHint::Read, diff --git a/src/skills.rs b/src/skills.rs index 16e0a76..1b7c765 100644 --- a/src/skills.rs +++ b/src/skills.rs @@ -19,6 +19,7 @@ //! //! Plus prompt templates from `[prompts]` for slash commands. +use crate::config::HooksSection; use anyhow::{Context, Result}; use globset::{Glob, GlobSetBuilder}; use serde::Deserialize; @@ -44,6 +45,8 @@ pub struct Skill { pub compatibility: Option, /// Optional `license:` field. pub license: Option, + /// Runtime-only Claude Code fields that apply to direct skill invocation. + pub runtime: SkillRuntimeOptions, /// If true, omit this skill from the model-facing catalog. pub disable_model_invocation: bool, /// If false, do not expose this skill as a direct slash command. @@ -64,12 +67,53 @@ pub struct PromptTemplate { pub body: String, } +/// Runtime controls carried by Claude Code skill frontmatter and applied +/// only when the skill is directly invoked as `/skill-name`. +#[derive(Debug, Default, Clone)] +pub struct SkillRuntimeOptions { + pub model: Option, + pub effort: Option, + pub context: Option, + pub agent: Option, + pub shell: Option, + pub allowed_tools: Vec, + pub disallowed_tools: Vec, + pub hooks: HooksSection, +} + +impl SkillRuntimeOptions { + pub fn is_empty(&self) -> bool { + self.model.is_none() + && self.effort.is_none() + && self.context.is_none() + && self.agent.is_none() + && self.shell.is_none() + && self.allowed_tools.is_empty() + && self.disallowed_tools.is_empty() + && self.hooks.pre_tool_use.is_empty() + && self.hooks.post_tool_use.is_empty() + && self.hooks.user_prompt_submit.is_empty() + && self.hooks.stop.is_empty() + } + + pub fn is_fork(&self) -> bool { + matches!(self.agent, Some(SkillAgentMode::Fork)) + } +} + +/// Direct-invocation execution mode declared by skill `agent:` frontmatter. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum SkillAgentMode { + Fork, +} + /// Slash-command template handed to [`crate::session_runner::SessionRunner`]. #[derive(Debug, Clone)] pub struct SlashTemplate { pub body: String, pub arguments: Vec, pub append_arguments_fallback: bool, + pub runtime: Option, } impl SlashTemplate { @@ -78,14 +122,16 @@ impl SlashTemplate { body, arguments: Vec::new(), append_arguments_fallback: false, + runtime: None, } } - pub fn skill(body: String, arguments: Vec) -> Self { + pub fn skill(body: String, arguments: Vec, runtime: SkillRuntimeOptions) -> Self { Self { body, arguments, append_arguments_fallback: true, + runtime: Some(runtime), } } } @@ -228,7 +274,7 @@ impl ResourceBundle { .map(|s| { ( s.command_name.clone(), - SlashTemplate::skill(s.body.clone(), s.arguments.clone()), + SlashTemplate::skill(s.body.clone(), s.arguments.clone(), s.runtime.clone()), ) }) .collect(); @@ -368,24 +414,16 @@ struct Frontmatter { #[serde(rename = "user-invocable")] user_invocable: Option, #[serde(rename = "allowed-tools")] - #[allow(dead_code)] allowed_tools: Option, #[serde(rename = "disallowed-tools")] - #[allow(dead_code)] disallowed_tools: Option, - #[allow(dead_code)] model: Option, - #[allow(dead_code)] effort: Option, - #[allow(dead_code)] context: Option, - #[allow(dead_code)] agent: Option, - #[allow(dead_code)] - hooks: Option, + hooks: Option, #[allow(dead_code)] paths: Option, - #[allow(dead_code)] shell: Option, } @@ -398,6 +436,16 @@ fn parse_skill(p: &Path) -> Result { let name = fm.name.clone().unwrap_or_else(|| command_name.clone()); let description = skill_description(&fm, body); let arguments = parse_arguments(fm.arguments.as_ref()); + let runtime = SkillRuntimeOptions { + model: normalize_opt_string(fm.model), + effort: normalize_opt_string(fm.effort), + context: normalize_opt_string(fm.context), + agent: parse_agent_mode(fm.agent.as_deref()), + shell: normalize_opt_string(fm.shell), + allowed_tools: parse_tool_list(fm.allowed_tools.as_ref()), + disallowed_tools: parse_tool_list(fm.disallowed_tools.as_ref()), + hooks: fm.hooks.unwrap_or_default(), + }; Ok(Skill { command_name, name, @@ -406,6 +454,7 @@ fn parse_skill(p: &Path) -> Result { arguments, compatibility: fm.compatibility, license: fm.license, + runtime, disable_model_invocation: fm.disable_model_invocation.unwrap_or(false), user_invocable: fm.user_invocable.unwrap_or(true), path: p.to_path_buf(), @@ -413,6 +462,37 @@ fn parse_skill(p: &Path) -> Result { }) } +fn normalize_opt_string(raw: Option) -> Option { + raw.map(|s| s.trim().to_string()).filter(|s| !s.is_empty()) +} + +fn parse_agent_mode(raw: Option<&str>) -> Option { + match raw.map(str::trim).filter(|s| !s.is_empty()) { + Some("fork") | Some("subagent") => Some(SkillAgentMode::Fork), + _ => None, + } +} + +fn parse_tool_list(raw: Option<&serde_yaml::Value>) -> Vec { + match raw { + Some(serde_yaml::Value::String(s)) => split_tool_list(s), + Some(serde_yaml::Value::Sequence(items)) => items + .iter() + .filter_map(|item| item.as_str()) + .flat_map(split_tool_list) + .collect(), + _ => Vec::new(), + } +} + +fn split_tool_list(s: &str) -> Vec { + s.split(',') + .map(str::trim) + .filter(|tool| !tool.is_empty()) + .map(ToOwned::to_owned) + .collect() +} + fn parse_arguments(raw: Option<&serde_yaml::Value>) -> Vec { match raw { Some(serde_yaml::Value::String(s)) => s diff --git a/tests/session_runner_slash.rs b/tests/session_runner_slash.rs index a93d551..ea549d8 100644 --- a/tests/session_runner_slash.rs +++ b/tests/session_runner_slash.rs @@ -4,14 +4,21 @@ use std::sync::{Arc, Mutex}; use async_trait::async_trait; use futures::stream::{self, BoxStream, StreamExt}; use ra::{ + config::{Hook, HooksSection}, + events::ToolCall, model::{Message, Model, ModelChunk, StopReason, ToolSpec}, session_runner::{RunOutcome, RunnerHost, SessionRunner}, - skills::SlashTemplate, - Session, + skills::{SkillAgentMode, SkillRuntimeOptions, SlashTemplate}, + tool_ctx::ToolCtx, + Session, Tool, }; +use schemars::{schema_for, JsonSchema}; +use serde::Deserialize; struct ScriptedModel { seen: Arc>>>, + seen_tools: Arc>>>, + chunks: Vec, } #[async_trait] @@ -19,9 +26,42 @@ impl Model for ScriptedModel { async fn stream( &self, messages: &[Message], - _tools: &[ToolSpec], + tools: &[ToolSpec], ) -> anyhow::Result> { self.seen.lock().unwrap().push(messages.to_vec()); + self.seen_tools + .lock() + .unwrap() + .push(tools.iter().map(|t| t.name.clone()).collect()); + Ok(stream::iter(self.chunks.clone()).boxed()) + } +} + +impl ScriptedModel { + fn end_turn(seen: Arc>>>) -> Self { + Self { + seen, + seen_tools: Arc::new(Mutex::new(Vec::new())), + chunks: vec![ModelChunk::End { + stop_reason: StopReason::EndTurn, + }], + } + } +} + +struct NamedModel { + name: &'static str, + seen: Arc>>, +} + +#[async_trait] +impl Model for NamedModel { + async fn stream( + &self, + _messages: &[Message], + _tools: &[ToolSpec], + ) -> anyhow::Result> { + self.seen.lock().unwrap().push(self.name); Ok(stream::iter(vec![ModelChunk::End { stop_reason: StopReason::EndTurn, }]) @@ -44,10 +84,75 @@ impl RunnerHost for NullHost { } } +struct ModelHost { + override_model: Arc, +} + +#[async_trait] +impl RunnerHost for ModelHost { + async fn save_session(&self, _session_id: &str) {} + + fn default_ctx_window(&self) -> u64 { + 200_000 + } + + fn list_models_for_display(&self) -> Vec<(String, String)> { + vec![] + } + + fn build_model_for_id(&self, model_id: &str) -> Option> { + (model_id == "review-model").then(|| self.override_model.clone()) + } +} + +#[derive(Debug, Deserialize, JsonSchema)] +struct EchoParams { + text: String, +} + +#[derive(Clone)] +struct EchoTool { + name: &'static str, + log: Arc>>, +} + +#[async_trait] +impl Tool for EchoTool { + fn name(&self) -> &str { + self.name + } + + fn description(&self) -> &str { + "test echo tool" + } + + fn schema(&self) -> serde_json::Value { + serde_json::to_value(schema_for!(EchoParams)).unwrap() + } + + async fn execute( + &self, + _call_id: &str, + input: serde_json::Value, + _ctx: &ToolCtx, + ) -> anyhow::Result { + let params: EchoParams = serde_json::from_value(input)?; + self.log + .lock() + .unwrap() + .push(format!("{}:{}", self.name, params.text)); + Ok(params.text) + } +} + +fn skill_template(body: &str, runtime: SkillRuntimeOptions) -> SlashTemplate { + SlashTemplate::skill(body.to_string(), Vec::new(), runtime) +} + #[tokio::test] async fn skill_slash_template_with_args_expands_before_model() { let seen = Arc::new(Mutex::new(Vec::new())); - let model = ScriptedModel { seen: seen.clone() }; + let model = ScriptedModel::end_turn(seen.clone()); let session = Arc::new(Session::new(Arc::new(model), vec![])); let mut templates = HashMap::new(); @@ -56,6 +161,7 @@ async fn skill_slash_template_with_args_expands_before_model() { SlashTemplate::skill( "Deploy using the project release checklist.".to_string(), Vec::new(), + SkillRuntimeOptions::default(), ), ); @@ -78,7 +184,7 @@ async fn skill_slash_template_with_args_expands_before_model() { #[tokio::test] async fn skill_slash_template_replaces_arguments_placeholders() { let seen = Arc::new(Mutex::new(Vec::new())); - let model = ScriptedModel { seen: seen.clone() }; + let model = ScriptedModel::end_turn(seen.clone()); let session = Arc::new(Session::new(Arc::new(model), vec![])); let mut templates = HashMap::new(); @@ -87,6 +193,7 @@ async fn skill_slash_template_replaces_arguments_placeholders() { SlashTemplate::skill( "Migrate $component from $0 to $ARGUMENTS[1]. Raw: $ARGUMENTS.".to_string(), vec!["component".to_string()], + SkillRuntimeOptions::default(), ), ); @@ -109,7 +216,7 @@ async fn skill_slash_template_replaces_arguments_placeholders() { #[tokio::test] async fn prompt_template_with_args_keeps_plain_append_behavior() { let seen = Arc::new(Mutex::new(Vec::new())); - let model = ScriptedModel { seen: seen.clone() }; + let model = ScriptedModel::end_turn(seen.clone()); let session = Arc::new(Session::new(Arc::new(model), vec![])); let mut templates = HashMap::new(); @@ -131,3 +238,322 @@ async fn prompt_template_with_args_keeps_plain_append_behavior() { "prompt templates should keep legacy append behavior, got: {user_msg:?}" ); } + +#[tokio::test] +async fn skill_slash_template_renders_dynamic_shell_context() { + let seen = Arc::new(Mutex::new(Vec::new())); + let model = ScriptedModel::end_turn(seen.clone()); + let session = Arc::new(Session::new(Arc::new(model), vec![])); + + let mut templates = HashMap::new(); + templates.insert( + "inspect".to_string(), + skill_template( + "Inline: !`printf inline-context`\nBlock:\n```!\nprintf fenced-context\n```", + SkillRuntimeOptions::default(), + ), + ); + + let runner = SessionRunner::new(session, "test-session".into(), Arc::new(NullHost)) + .with_prompt_templates(Arc::new(templates)); + + let outcome = runner.run_input("/inspect".to_string(), |_| {}).await; + assert_eq!(outcome, RunOutcome::Completed); + + let calls = seen.lock().unwrap().clone(); + let user_msg = calls[0].iter().find(|m| matches!(m, Message::User { .. })); + assert!( + matches!(user_msg, Some(Message::User { content }) if content.contains("Inline: inline-context") && content.contains("fenced-context")), + "dynamic shell context should be rendered, got: {user_msg:?}" + ); +} + +#[tokio::test] +async fn skill_slash_template_marks_failed_dynamic_shell_context() { + let seen = Arc::new(Mutex::new(Vec::new())); + let model = ScriptedModel::end_turn(seen.clone()); + let session = Arc::new(Session::new(Arc::new(model), vec![])); + + let mut templates = HashMap::new(); + templates.insert( + "inspect".to_string(), + skill_template("Before !`exit 7` after", SkillRuntimeOptions::default()), + ); + + let runner = SessionRunner::new(session, "test-session".into(), Arc::new(NullHost)) + .with_prompt_templates(Arc::new(templates)); + + let outcome = runner.run_input("/inspect".to_string(), |_| {}).await; + assert_eq!(outcome, RunOutcome::Completed); + + let calls = seen.lock().unwrap().clone(); + let user_msg = calls[0].iter().find(|m| matches!(m, Message::User { .. })); + assert!( + matches!(user_msg, Some(Message::User { content }) if content.contains("shell context command failed") && content.contains("exited with 7")), + "failed shell context should be visible, got: {user_msg:?}" + ); +} + +#[tokio::test] +async fn skill_scoped_allowed_tools_limit_model_specs() { + let seen = Arc::new(Mutex::new(Vec::new())); + let seen_tools = Arc::new(Mutex::new(Vec::new())); + let model = ScriptedModel { + seen: seen.clone(), + seen_tools: seen_tools.clone(), + chunks: vec![ModelChunk::End { + stop_reason: StopReason::EndTurn, + }], + }; + let session = Arc::new(Session::new( + Arc::new(model), + vec![ + Arc::new(EchoTool { + name: "read", + log: Arc::new(Mutex::new(Vec::new())), + }), + Arc::new(EchoTool { + name: "bash", + log: Arc::new(Mutex::new(Vec::new())), + }), + ], + )); + + let mut templates = HashMap::new(); + templates.insert( + "review".to_string(), + skill_template( + "Review.", + SkillRuntimeOptions { + allowed_tools: vec!["read".to_string()], + ..SkillRuntimeOptions::default() + }, + ), + ); + + let runner = SessionRunner::new(session, "test-session".into(), Arc::new(NullHost)) + .with_prompt_templates(Arc::new(templates)); + + let outcome = runner.run_input("/review".to_string(), |_| {}).await; + assert_eq!(outcome, RunOutcome::Completed); + + assert_eq!(seen_tools.lock().unwrap()[0], vec!["read".to_string()]); +} + +#[tokio::test] +async fn skill_scoped_disallowed_tools_block_execution() { + let seen = Arc::new(Mutex::new(Vec::new())); + let log = Arc::new(Mutex::new(Vec::new())); + let model = ScriptedModel { + seen, + seen_tools: Arc::new(Mutex::new(Vec::new())), + chunks: vec![ + ModelChunk::ToolCall(ToolCall { + id: "call-1".to_string(), + name: "bash".to_string(), + input: serde_json::json!({ "text": "blocked" }), + }), + ModelChunk::End { + stop_reason: StopReason::EndTurn, + }, + ], + }; + let session = Arc::new(Session::new( + Arc::new(model), + vec![Arc::new(EchoTool { + name: "bash", + log: log.clone(), + })], + )); + + let mut templates = HashMap::new(); + templates.insert( + "audit".to_string(), + skill_template( + "Audit.", + SkillRuntimeOptions { + disallowed_tools: vec!["bash".to_string()], + ..SkillRuntimeOptions::default() + }, + ), + ); + + let mut events = Vec::new(); + let runner = SessionRunner::new(session, "test-session".into(), Arc::new(NullHost)) + .with_prompt_templates(Arc::new(templates)); + + let outcome = runner + .run_input("/audit".to_string(), |ev| events.push(ev)) + .await; + assert_eq!(outcome, RunOutcome::Completed); + assert!(log.lock().unwrap().is_empty(), "denied tool must not run"); + assert!( + events.iter().any(|ev| matches!( + ev, + ra::session_runner::RunnerEvent::ToolCallEnd { + is_error: true, + content, + .. + } if content.contains("denied by skill-scoped tool policy") + )), + "denied tool should produce an error ToolCallEnd: {events:?}" + ); +} + +#[tokio::test] +async fn skill_scoped_hooks_are_temporary() { + let seen = Arc::new(Mutex::new(Vec::new())); + let log = Arc::new(Mutex::new(Vec::new())); + let model = ScriptedModel { + seen, + seen_tools: Arc::new(Mutex::new(Vec::new())), + chunks: vec![ + ModelChunk::ToolCall(ToolCall { + id: "call-1".to_string(), + name: "bash".to_string(), + input: serde_json::json!({ "text": "maybe" }), + }), + ModelChunk::End { + stop_reason: StopReason::EndTurn, + }, + ], + }; + let session = Arc::new(Session::new( + Arc::new(model), + vec![Arc::new(EchoTool { + name: "bash", + log: log.clone(), + })], + )); + + let mut hooks = HooksSection::default(); + hooks.pre_tool_use.push(Hook { + matcher: "bash".to_string(), + command: "printf '{\"hookSpecificOutput\":{\"hookEventName\":\"PreToolUse\",\"permissionDecision\":\"deny\",\"permissionDecisionReason\":\"skill hook\"}}'".to_string(), + timeout: 5.0, + run_async: false, + }); + let mut templates = HashMap::new(); + templates.insert( + "guarded".to_string(), + skill_template( + "Guarded.", + SkillRuntimeOptions { + hooks, + ..SkillRuntimeOptions::default() + }, + ), + ); + + let mut events = Vec::new(); + let runner = SessionRunner::new(session, "test-session".into(), Arc::new(NullHost)) + .with_prompt_templates(Arc::new(templates)); + + let outcome = runner + .run_input("/guarded".to_string(), |ev| events.push(ev)) + .await; + assert_eq!(outcome, RunOutcome::Completed); + assert!( + log.lock().unwrap().is_empty(), + "hook-denied tool must not run" + ); + assert!( + events.iter().any(|ev| matches!( + ev, + ra::session_runner::RunnerEvent::ToolCallEnd { + is_error: true, + content, + .. + } if content.contains("skill hook") + )), + "skill hook denial should surface in tool result: {events:?}" + ); +} + +#[tokio::test] +async fn skill_scoped_model_override_is_temporary() { + let seen_models = Arc::new(Mutex::new(Vec::new())); + let default_model = Arc::new(NamedModel { + name: "default", + seen: seen_models.clone(), + }); + let override_model = Arc::new(NamedModel { + name: "review", + seen: seen_models.clone(), + }); + let session = Arc::new(Session::new(default_model, vec![])); + + let mut templates = HashMap::new(); + templates.insert( + "review".to_string(), + skill_template( + "Review.", + SkillRuntimeOptions { + model: Some("review-model".to_string()), + ..SkillRuntimeOptions::default() + }, + ), + ); + let host = Arc::new(ModelHost { override_model }); + let runner = SessionRunner::new(session.clone(), "test-session".into(), host) + .with_prompt_templates(Arc::new(templates)); + + let outcome = runner.run_input("/review".to_string(), |_| {}).await; + assert_eq!(outcome, RunOutcome::Completed); + session.prompt("plain".to_string()).await.unwrap(); + + assert_eq!(&*seen_models.lock().unwrap(), &["review", "default"]); +} + +#[tokio::test] +async fn forked_skill_does_not_keep_internal_user_prompt() { + let seen = Arc::new(Mutex::new(Vec::new())); + let model = ScriptedModel { + seen: seen.clone(), + seen_tools: Arc::new(Mutex::new(Vec::new())), + chunks: vec![ + ModelChunk::TextDelta("fork result".to_string()), + ModelChunk::End { + stop_reason: StopReason::EndTurn, + }, + ], + }; + let session = Arc::new(Session::new(Arc::new(model), vec![])); + session + .prompt("parent prompt".to_string()) + .await + .expect("seed parent"); + + let mut templates = HashMap::new(); + templates.insert( + "fork-review".to_string(), + skill_template( + "Internal fork prompt.", + SkillRuntimeOptions { + agent: Some(SkillAgentMode::Fork), + ..SkillRuntimeOptions::default() + }, + ), + ); + let runner = SessionRunner::new(session.clone(), "test-session".into(), Arc::new(NullHost)) + .with_prompt_templates(Arc::new(templates)); + + let outcome = runner.run_input("/fork-review".to_string(), |_| {}).await; + assert_eq!(outcome, RunOutcome::Completed); + + let messages = session.snapshot_messages().await; + assert!( + !messages.iter().any(|msg| matches!( + msg, + Message::User { content } if content.contains("Internal fork prompt") + )), + "fork prompt should not remain in parent transcript: {messages:?}" + ); + assert!( + messages.iter().any(|msg| matches!( + msg, + Message::Assistant { content, .. } if content.contains("fork result") + )), + "fork result should be appended to parent transcript: {messages:?}" + ); +} diff --git a/tests/skills_discover.rs b/tests/skills_discover.rs index 99e4c80..dfe9bbe 100644 --- a/tests/skills_discover.rs +++ b/tests/skills_discover.rs @@ -4,7 +4,9 @@ use ra::{ config::RaConfig, - skills::{build_resource_bundle, default_discover_globs, load_skills, ResourceBundle}, + skills::{ + build_resource_bundle, default_discover_globs, load_skills, ResourceBundle, SkillAgentMode, + }, }; use std::fs; use std::sync::{Mutex, OnceLock}; @@ -209,3 +211,71 @@ fn prompt_map_preserves_skill_arguments_metadata() { ); assert!(template.append_arguments_fallback); } + +#[test] +fn skill_parser_preserves_advanced_runtime_frontmatter() { + let _guard = cwd_lock(); + let tmp = TempDir::new().unwrap(); + let cwd = tmp.path(); + let skill_dir = cwd.join(".claude/skills/advanced"); + fs::create_dir_all(&skill_dir).unwrap(); + fs::write( + skill_dir.join("SKILL.md"), + r#"--- +description: Advanced skill +model: review-model +effort: high +context: Keep output concise. +agent: fork +shell: bash +allowed-tools: [read, "Bash(git status:*)"] +disallowed-tools: "write, edit" +hooks: + PreToolUse: + - matcher: bash + command: "echo hook" + timeout: 1 +--- + +Advanced body. +"#, + ) + .unwrap(); + + std::env::set_current_dir(cwd).unwrap(); + + let skills = load_skills(&default_discover_globs()); + let skill = skills + .iter() + .find(|s| s.command_name == "advanced") + .expect("advanced skill should load"); + + assert_eq!(skill.runtime.model.as_deref(), Some("review-model")); + assert_eq!(skill.runtime.effort.as_deref(), Some("high")); + assert_eq!( + skill.runtime.context.as_deref(), + Some("Keep output concise.") + ); + assert_eq!(skill.runtime.agent, Some(SkillAgentMode::Fork)); + assert_eq!(skill.runtime.shell.as_deref(), Some("bash")); + assert_eq!( + skill.runtime.allowed_tools, + vec!["read".to_string(), "Bash(git status:*)".to_string()] + ); + assert_eq!( + skill.runtime.disallowed_tools, + vec!["write".to_string(), "edit".to_string()] + ); + assert_eq!(skill.runtime.hooks.pre_tool_use.len(), 1); + + let bundle = ResourceBundle { + skills, + ..ResourceBundle::default() + }; + let slash = bundle.prompt_map(); + let template = slash.get("advanced").expect("advanced slash template"); + assert!( + template.runtime.is_some(), + "advanced runtime metadata must flow into slash templates" + ); +} From 546a561c264fb7eaf1d03c732385033a13958f88 Mon Sep 17 00:00:00 2001 From: local Date: Tue, 2 Jun 2026 03:00:23 +0800 Subject: [PATCH 2/2] Fix advanced skill runtime review blockers Co-authored-by: multica-agent --- ...ithub-issue-advanced-claude-code-skills.md | 5 +- docs/prd-advanced-claude-code-skills.md | 9 +- .../design.md | 12 +- .../proposal.md | 2 +- .../specs/claude-code-skills/spec.md | 22 +- .../implement-advanced-claude-skills/tasks.md | 6 +- src/session.rs | 15 +- src/session_runner.rs | 64 ++++-- src/skills.rs | 37 +-- tests/session_runner_slash.rs | 210 +++++++++++++++++- tests/skills_discover.rs | 16 +- 11 files changed, 331 insertions(+), 67 deletions(-) diff --git a/docs/github-issue-advanced-claude-code-skills.md b/docs/github-issue-advanced-claude-code-skills.md index 7d22f6e..9dd4742 100644 --- a/docs/github-issue-advanced-claude-code-skills.md +++ b/docs/github-issue-advanced-claude-code-skills.md @@ -18,8 +18,9 @@ The first Claude Code skills alignment PR covered discovery, frontmatter toleran - Render dynamic shell context before direct skill invocation reaches the model: - inline `` !`command` `` - fenced ` ```! ` command blocks +- Preserve one-pass semantics: dynamic shell context is rendered from the original skill template before argument substitution, and inserted arguments are not rescanned. - Run shell context commands from the session cwd using a deterministic shell choice. -- Support `agent: fork` by running the skill in an isolated child transcript and appending only the final assistant result to the parent transcript. +- Support `context: fork` by running the skill in an isolated child transcript and appending only a new final assistant result to the parent transcript. - Enforce `allowed-tools` and `disallowed-tools` for the invoked skill only. - Run skill-scoped hooks for the invoked skill only, in addition to normal session hooks. - Override the model for the invoked skill only when the named model is available from the host model factory. @@ -36,7 +37,7 @@ The first Claude Code skills alignment PR covered discovery, frontmatter toleran - A directly invoked skill containing `` !`printf context` `` sends `context` to the model in place of the inline expression. - A directly invoked skill containing a fenced ` ```! ` block sends the command output to the model in place of the command block. - Shell context command failures are visible in the rendered prompt as an error marker. -- `agent: fork` leaves the parent transcript free of the skill's internal user prompt while preserving the final assistant result. +- `context: fork` leaves the parent transcript free of the skill's internal user prompt while preserving only a new final assistant result. - `allowed-tools` limits the tool specs advertised to the model and blocks out-of-scope tool execution. - `disallowed-tools` removes denied tools even when the allow list would include them. - Skill-scoped hooks can block a tool call during that skill invocation and do not remain active afterward. diff --git a/docs/prd-advanced-claude-code-skills.md b/docs/prd-advanced-claude-code-skills.md index c8678ae..ece5ba7 100644 --- a/docs/prd-advanced-claude-code-skills.md +++ b/docs/prd-advanced-claude-code-skills.md @@ -6,7 +6,7 @@ Ra now supports the basic Claude Code skill shape, but advanced Claude Code skil ## Goals & Success Metrics -- Direct `/skill-name` invocation renders dynamic shell context before the model sees the skill prompt. +- Direct `/skill-name` invocation renders dynamic shell context from the original skill template before arguments are inserted. - Skills that request forked execution run against an isolated copy of the conversation and do not mutate the parent session transcript with internal subagent turns. - Skills that declare `model`, `allowed-tools`, `disallowed-tools`, or `hooks` apply those constraints for that skill invocation only. - Existing prompt-template behavior and previously aligned skill discovery/frontmatter behavior remain backward-compatible. @@ -23,11 +23,11 @@ Ra now supports the basic Claude Code skill shape, but advanced Claude Code skil | Priority | Requirement | | --- | --- | | Must | Parse and persist skill frontmatter for `model`, `allowed-tools`, `disallowed-tools`, `hooks`, `context`, `agent`, and `shell`. | -| Must | Replace inline `` !`command` `` expressions with captured stdout before the skill prompt is submitted. | -| Must | Replace fenced ` ```! ` command blocks with captured stdout before the skill prompt is submitted. | +| Must | Replace inline `` !`command` `` expressions with captured stdout before the skill prompt is submitted, using one pass over the original skill template. | +| Must | Replace fenced ` ```! ` command blocks with captured stdout before the skill prompt is submitted, using one pass over the original skill template. | | Must | Run shell context commands from the current session cwd, using `/bin/sh -c` by default and `bash -lc` when `shell: bash` is declared. | | Must | Insert a readable error marker when a shell context command exits unsuccessfully instead of aborting the entire skill invocation. | -| Must | Support `agent: fork` as an isolated skill execution mode that runs on a child transcript snapshot and appends only the final assistant result to the parent transcript. | +| Must | Support `context: fork` as an isolated skill execution mode that runs on a child transcript snapshot and appends only a new final assistant result to the parent transcript. | | Must | Apply `allowed-tools` as an invocation-scoped allow list for tool specs and execution. | | Must | Apply `disallowed-tools` as an invocation-scoped deny list on top of the allow list. | | Must | Apply skill-scoped `hooks` in addition to session hooks for that invocation. | @@ -41,6 +41,7 @@ Ra now supports the basic Claude Code skill shape, but advanced Claude Code skil - Scope changes to skill rendering and the shared session runner/session path. - Preserve deterministic behavior in tests without network calls. - Avoid weakening existing session-level hooks and tool filters. +- Do not silently broaden constrained tool declarations such as `bash(...)` to the whole tool. - Keep failed shell context commands visible to the model for debugging. ## Design Considerations diff --git a/openspec/changes/implement-advanced-claude-skills/design.md b/openspec/changes/implement-advanced-claude-skills/design.md index 1b589df..cad6dad 100644 --- a/openspec/changes/implement-advanced-claude-skills/design.md +++ b/openspec/changes/implement-advanced-claude-skills/design.md @@ -22,11 +22,11 @@ Ra's current `Skill` model already tolerates advanced Claude Code fields during ### Store Runtime Options On Skill Templates -`Skill` and `SlashTemplate` will carry a `SkillRuntimeOptions` struct containing optional model, effort, shell, agent mode, allowed/disallowed tool declarations, and hooks. Prompt templates keep `None`, so legacy prompt commands remain unaffected. +`Skill` and `SlashTemplate` will carry a `SkillRuntimeOptions` struct containing optional model, effort, shell, context, agent, allowed/disallowed tool declarations, and hooks. Prompt templates keep `None`, so legacy prompt commands remain unaffected. `context: fork` is the fork trigger; `agent` is retained as the optional subagent type/compatibility alias. ### Render Shell Context In The Runner -Dynamic context syntax is prompt rendering, not model behavior. The runner already owns direct slash invocation and has access to session cwd, so it will replace inline `` !`command` `` and fenced ` ```! ` blocks after argument substitution and before calling the session. +Dynamic context syntax is prompt rendering, not model behavior. The runner already owns direct slash invocation and has access to session cwd, so it will replace inline `` !`command` `` and fenced ` ```! ` blocks on the original skill body before argument substitution. This is intentionally one-pass: user-provided arguments inserted through `$ARGUMENTS`, `$N`, named placeholders, or no-placeholder fallback are never scanned as dynamic shell context. Unsuccessful shell commands should become readable error markers in the rendered prompt. This preserves debuggability and avoids unexpectedly aborting the entire skill invocation. @@ -39,7 +39,9 @@ Unsuccessful shell commands should become readable error markers in the rendered - merge skill hooks with session hooks, - restore the previous runtime state after completion. -The scope is held in the async prompt path and is not persisted into the message log. +Tool names are matched case-insensitively. Constrained declarations such as `bash(git status:*)` are not expanded to the whole `bash` tool; they fail closed until Ra has command-level policy enforcement. + +The scope is held in the async prompt path and is not persisted into the message log. Skill-scoped Stop hooks run on normal successful completion as well as early block/stop paths. ### Extend RunnerHost For Model Resolution @@ -49,12 +51,12 @@ Unknown model IDs should fail the skill invocation visibly. Silent fallback to t ### Forked Skill Execution Uses Transcript Snapshot Isolation -For `agent: fork`, the runner will run the rendered skill prompt against a child session state initialized from the parent transcript snapshot. After the child finishes, the parent transcript is restored to its pre-skill state and receives only the final assistant text produced by the child. Tool calls and intermediate skill messages remain isolated from the parent transcript. +For `context: fork`, the runner will run the rendered skill prompt against a child session state initialized from the parent transcript snapshot. After the child finishes, the parent transcript is restored to its pre-skill state and receives only the final assistant text produced after the fork snapshot. Tool calls and intermediate skill messages remain isolated from the parent transcript. If the fork produces no new assistant text, nothing is appended to the parent transcript. This matches the operational need for fork/subagent behavior with Ra's current single-session architecture and avoids protocol-specific session creation in the shared runner. ## Risks / Trade-offs -- Tool declarations in Claude Code can include richer permission patterns than Ra tool names. The first implementation will enforce by normalized tool name and `Tool(pattern)` prefix, which covers the current Ra tool-spec surface. +- Tool declarations in Claude Code can include richer permission patterns than Ra tool names. This implementation enforces whole-tool names only and fails closed on constrained declarations so it does not silently broaden permissions. - Forked execution cannot perfectly emulate Claude Code internals without a public transcript contract. Tests will lock Ra's defined behavior: parent snapshot in, final assistant result out. - Shell context injection executes local commands before the model call. That is expected for Claude Code-compatible skills and is contained to direct skill invocation. diff --git a/openspec/changes/implement-advanced-claude-skills/proposal.md b/openspec/changes/implement-advanced-claude-skills/proposal.md index de740af..41564e6 100644 --- a/openspec/changes/implement-advanced-claude-skills/proposal.md +++ b/openspec/changes/implement-advanced-claude-skills/proposal.md @@ -7,7 +7,7 @@ The merged Claude Code skills alignment made existing skills discoverable and di - Parse and retain advanced Claude Code skill frontmatter fields used at runtime. - Render inline and fenced dynamic shell context blocks before a skill prompt reaches the model. - Add invocation-scoped runtime options for direct skill slash commands. -- Support `agent: fork` by running a skill against an isolated child transcript and returning only the final result to the parent session. +- Support `context: fork` by running a skill against an isolated child transcript and returning only the final result to the parent session. - Enforce skill-scoped model overrides, tool allow/deny lists, and hooks for the duration of a skill invocation. - Add PRD, GitHub issue draft, OpenSpec requirements, focused tests, and documentation comments for the new runtime behavior. diff --git a/openspec/changes/implement-advanced-claude-skills/specs/claude-code-skills/spec.md b/openspec/changes/implement-advanced-claude-skills/specs/claude-code-skills/spec.md index 2cc7346..f12e4fe 100644 --- a/openspec/changes/implement-advanced-claude-skills/specs/claude-code-skills/spec.md +++ b/openspec/changes/implement-advanced-claude-skills/specs/claude-code-skills/spec.md @@ -30,6 +30,10 @@ Ra SHALL expose user-invocable skills as slash commands that expand to the skill - **WHEN** the user enters `/migrate SearchBar React Vue` and the skill body contains `$ARGUMENTS`, `$ARGUMENTS[1]`, `$0`, or named argument placeholders such as `$component` - **THEN** Ra replaces those placeholders before sending the rendered prompt to the model +#### Scenario: Argument substitution is not rescanned as shell context +- **WHEN** a skill body contains `$ARGUMENTS` and the user argument text contains dynamic shell syntax +- **THEN** Ra preserves the argument text literally and does not execute it as shell context + #### Scenario: Inline dynamic shell context is rendered - **WHEN** a directly invoked skill body contains `` !`printf context` `` - **THEN** Ra replaces the expression with the captured command output before sending the prompt to the model @@ -49,6 +53,14 @@ Ra SHALL enforce supported skill-scoped runtime fields for a direct skill invoca - **WHEN** a directly invoked skill declares `allowed-tools: [read]` - **THEN** the model receives only the `read` tool spec for that invocation +#### Scenario: Skill scoped tool names match case-insensitively +- **WHEN** a directly invoked skill declares tool names with Claude-style casing such as `Read` or `Bash` +- **THEN** Ra matches them against its lowercase built-in tool names + +#### Scenario: Constrained tool declarations fail closed +- **WHEN** a directly invoked skill declares a constrained tool pattern such as `bash(git status:*)` +- **THEN** Ra does not expose the whole `bash` tool for that declaration + #### Scenario: Skill scoped disallowed tools deny execution - **WHEN** a directly invoked skill declares `disallowed-tools: [bash]` - **THEN** a model-requested `bash` tool call is returned as an error instead of executing @@ -57,6 +69,10 @@ Ra SHALL enforce supported skill-scoped runtime fields for a direct skill invoca - **WHEN** a directly invoked skill declares a PreToolUse hook that denies `bash` - **THEN** the `bash` call is denied during that skill invocation and the hook does not affect later non-skill prompts +#### Scenario: Skill scoped Stop hook runs on success +- **WHEN** a directly invoked skill declares a Stop hook and the skill invocation completes normally +- **THEN** Ra runs the skill-scoped Stop hook before finishing the runner turn + #### Scenario: Skill scoped model override is temporary - **WHEN** a directly invoked skill declares `model: review-model` and the host can build that model - **THEN** Ra uses `review-model` for the skill invocation and restores the prior model after it completes @@ -65,9 +81,13 @@ Ra SHALL enforce supported skill-scoped runtime fields for a direct skill invoca Ra SHALL support direct skill invocation with isolated fork/subagent-style transcript behavior. #### Scenario: Forked skill isolates internal transcript -- **WHEN** a directly invoked skill declares `agent: fork` +- **WHEN** a directly invoked skill declares `context: fork` - **THEN** Ra runs the skill against a child transcript initialized from the parent snapshot and restores the parent transcript after the child run #### Scenario: Forked skill returns final result to parent - **WHEN** a forked skill completes with assistant text - **THEN** Ra appends the final assistant result to the parent transcript without appending the skill's internal user prompt + +#### Scenario: Forked skill without new result leaves parent unchanged +- **WHEN** a forked skill completes without producing new assistant text after the fork snapshot +- **THEN** Ra does not append any prior parent assistant text back into the parent transcript diff --git a/openspec/changes/implement-advanced-claude-skills/tasks.md b/openspec/changes/implement-advanced-claude-skills/tasks.md index c09152e..2bb8aca 100644 --- a/openspec/changes/implement-advanced-claude-skills/tasks.md +++ b/openspec/changes/implement-advanced-claude-skills/tasks.md @@ -5,7 +5,9 @@ - [x] Extend skill parsing/templates with advanced runtime frontmatter. - [x] Render dynamic shell context for inline and fenced command syntax. - [x] Add invocation-scoped model, tool, and hook enforcement in the shared session path. -- [x] Implement `agent: fork` transcript isolation for direct skill invocation. +- [x] Implement `context: fork` transcript isolation for direct skill invocation. - [x] Add focused tests for shell context, scoped enforcement, scoped model selection, and fork behavior. - [x] Run formatting, focused tests, full tests, and strict OpenSpec validation. -- [ ] Open a new GitHub issue and PR for review. +- [x] Open a new GitHub issue and PR for review. +- [x] Address review blockers: one-pass shell rendering before argument substitution, `context: fork`, tool-policy casing/constrained declarations, fork no-result behavior, and skill Stop hooks. +- [x] Add review regression tests and rerun validation. diff --git a/src/session.rs b/src/session.rs index e9c418a..885a497 100644 --- a/src/session.rs +++ b/src/session.rs @@ -316,9 +316,10 @@ impl Session { let result = self.prompt_unlocked(user_text.into()).await; *self.runtime_scope.lock().await = None; let child_messages = self.snapshot_messages().await; + let parent_len = parent_snapshot.len(); self.restore_messages(parent_snapshot).await; let outcome = result?; - if let Some(final_text) = final_assistant_text(&child_messages) { + if let Some(final_text) = final_assistant_text(&child_messages[parent_len..]) { let mut restored = self.snapshot_messages().await; restored.push(Message::Assistant { content: final_text, @@ -566,15 +567,13 @@ fn tool_allowed(name: &str, scope: Option<&SessionRuntimeScope>) -> bool { fn tool_decl_matches(decl: &str, name: &str) -> bool { let decl = decl.trim(); - if decl == "*" || decl == name { + if decl == "*" || decl.eq_ignore_ascii_case(name) { return true; } - let head = decl - .split_once('(') - .map(|(tool, _)| tool) - .unwrap_or(decl) - .trim(); - head == name + if decl.contains('(') || decl.contains(')') { + return false; + } + decl.eq_ignore_ascii_case(name) } fn final_assistant_text(messages: &[Message]) -> Option { diff --git a/src/session_runner.rs b/src/session_runner.rs index b9b004c..45fe338 100644 --- a/src/session_runner.rs +++ b/src/session_runner.rs @@ -82,6 +82,13 @@ pub enum RunnerEvent { Finished(RunOutcome), } +#[derive(Clone)] +struct PreparedSkillRuntime { + scope: SessionRuntimeScope, + stop_hooks: Option>, + forked: bool, +} + /// Narrow capability surface a `SessionRunner` needs from whatever holds /// long-lived agent state. Lets the protocol-specific server (ACP, /// A2A, …) own the actual `SharedState` without leaking ACP types @@ -225,29 +232,44 @@ impl SessionRunner { return RunOutcome::Completed; } if let Some(template) = self.prompt_templates.get(&cmd.name).cloned() { - effective_text = render_slash_template(&template, &cmd.args); skill_runtime = template.runtime.clone(); if let Some(runtime) = &skill_runtime { - match render_dynamic_shell_context(effective_text, runtime, self.session.cwd()) - .await + match render_dynamic_shell_context( + template.body.clone(), + runtime, + self.session.cwd(), + ) + .await { - Ok(rendered) => effective_text = rendered, + Ok(rendered) => { + let shell_rendered_template = SlashTemplate { + body: rendered, + ..template + }; + effective_text = + render_slash_template(&shell_rendered_template, &cmd.args); + } Err(e) => return RunOutcome::Failed(format!("{e:#}")), } - if let Some(context) = runtime.context.as_ref().filter(|s| !s.is_empty()) { + if let Some(context) = runtime.prompt_context() { effective_text = format!("[skill context]\n{context}\n\n{effective_text}"); } + } else { + effective_text = render_slash_template(&template, &cmd.args); } } } - let scope = match skill_runtime.as_ref() { + let prepared_runtime = match skill_runtime.as_ref() { Some(runtime) => match self.runtime_scope_for(runtime).await { - Ok(scope) => Some(scope), + Ok(prepared) => Some(prepared), Err(e) => return RunOutcome::Failed(format!("{e:#}")), }, None => None, }; + let scope = prepared_runtime + .as_ref() + .map(|prepared| prepared.scope.clone()); // UserPromptSubmit hooks run after slash expansion so skill-scoped // hooks observe the same prompt that will reach the model. @@ -277,9 +299,9 @@ impl SessionRunner { // Subscribe BEFORE prompt() so we don't miss the first events. let mut rx = self.session.subscribe(); let session = self.session.clone(); - let forked = skill_runtime + let forked = prepared_runtime .as_ref() - .map(|runtime| runtime.is_fork()) + .map(|prepared| prepared.forked) .unwrap_or(false); let prompt_fut: BoxFuture<'_, Result> = Box::pin(async move { if forked { @@ -366,13 +388,19 @@ impl SessionRunner { } } } - outcome.unwrap_or(RunOutcome::Completed) + let outcome = outcome.unwrap_or(RunOutcome::Completed); + if matches!(outcome, RunOutcome::Completed) { + if let Some(hooks) = prepared_runtime.and_then(|prepared| prepared.stop_hooks) { + hooks.stop(Some(&self.session_id)).await; + } + } + outcome } async fn runtime_scope_for( &self, runtime: &SkillRuntimeOptions, - ) -> Result { + ) -> Result { let model = match runtime.model.as_deref() { Some(model_id) => Some( self.host @@ -387,11 +415,15 @@ impl SessionRunner { } else { Some(Arc::new(skill_hooks)) }; - Ok(SessionRuntimeScope { - model, - allowed_tools: runtime.allowed_tools.clone(), - disallowed_tools: runtime.disallowed_tools.clone(), - hooks, + Ok(PreparedSkillRuntime { + scope: SessionRuntimeScope { + model, + allowed_tools: runtime.allowed_tools.clone(), + disallowed_tools: runtime.disallowed_tools.clone(), + hooks: hooks.clone(), + }, + stop_hooks: hooks, + forked: runtime.is_fork(), }) } diff --git a/src/skills.rs b/src/skills.rs index 1b7c765..6701dc2 100644 --- a/src/skills.rs +++ b/src/skills.rs @@ -74,7 +74,7 @@ pub struct SkillRuntimeOptions { pub model: Option, pub effort: Option, pub context: Option, - pub agent: Option, + pub agent: Option, pub shell: Option, pub allowed_tools: Vec, pub disallowed_tools: Vec, @@ -97,16 +97,28 @@ impl SkillRuntimeOptions { } pub fn is_fork(&self) -> bool { - matches!(self.agent, Some(SkillAgentMode::Fork)) + self.context + .as_deref() + .map(|s| s.eq_ignore_ascii_case("fork")) + .unwrap_or(false) + || self + .agent + .as_deref() + .map(|s| { + let s = s.trim(); + s.eq_ignore_ascii_case("fork") || s.eq_ignore_ascii_case("subagent") + }) + .unwrap_or(false) + } + + pub fn prompt_context(&self) -> Option<&str> { + self.context + .as_deref() + .map(str::trim) + .filter(|s| !s.is_empty() && !s.eq_ignore_ascii_case("fork")) } } -/// Direct-invocation execution mode declared by skill `agent:` frontmatter. -#[derive(Debug, Clone, Copy, PartialEq, Eq)] -pub enum SkillAgentMode { - Fork, -} - /// Slash-command template handed to [`crate::session_runner::SessionRunner`]. #[derive(Debug, Clone)] pub struct SlashTemplate { @@ -440,7 +452,7 @@ fn parse_skill(p: &Path) -> Result { model: normalize_opt_string(fm.model), effort: normalize_opt_string(fm.effort), context: normalize_opt_string(fm.context), - agent: parse_agent_mode(fm.agent.as_deref()), + agent: normalize_opt_string(fm.agent), shell: normalize_opt_string(fm.shell), allowed_tools: parse_tool_list(fm.allowed_tools.as_ref()), disallowed_tools: parse_tool_list(fm.disallowed_tools.as_ref()), @@ -466,13 +478,6 @@ fn normalize_opt_string(raw: Option) -> Option { raw.map(|s| s.trim().to_string()).filter(|s| !s.is_empty()) } -fn parse_agent_mode(raw: Option<&str>) -> Option { - match raw.map(str::trim).filter(|s| !s.is_empty()) { - Some("fork") | Some("subagent") => Some(SkillAgentMode::Fork), - _ => None, - } -} - fn parse_tool_list(raw: Option<&serde_yaml::Value>) -> Vec { match raw { Some(serde_yaml::Value::String(s)) => split_tool_list(s), diff --git a/tests/session_runner_slash.rs b/tests/session_runner_slash.rs index ea549d8..162861a 100644 --- a/tests/session_runner_slash.rs +++ b/tests/session_runner_slash.rs @@ -8,7 +8,7 @@ use ra::{ events::ToolCall, model::{Message, Model, ModelChunk, StopReason, ToolSpec}, session_runner::{RunOutcome, RunnerHost, SessionRunner}, - skills::{SkillAgentMode, SkillRuntimeOptions, SlashTemplate}, + skills::{SkillRuntimeOptions, SlashTemplate}, tool_ctx::ToolCtx, Session, Tool, }; @@ -294,6 +294,34 @@ async fn skill_slash_template_marks_failed_dynamic_shell_context() { ); } +#[tokio::test] +async fn skill_arguments_are_not_rescanned_for_dynamic_shell_context() { + let seen = Arc::new(Mutex::new(Vec::new())); + let model = ScriptedModel::end_turn(seen.clone()); + let session = Arc::new(Session::new(Arc::new(model), vec![])); + + let mut templates = HashMap::new(); + templates.insert( + "review".to_string(), + skill_template("Review $ARGUMENTS", SkillRuntimeOptions::default()), + ); + + let runner = SessionRunner::new(session, "test-session".into(), Arc::new(NullHost)) + .with_prompt_templates(Arc::new(templates)); + + let outcome = runner + .run_input("/review !`printf should-not-run`".to_string(), |_| {}) + .await; + assert_eq!(outcome, RunOutcome::Completed); + + let calls = seen.lock().unwrap().clone(); + let user_msg = calls[0].iter().find(|m| matches!(m, Message::User { .. })); + assert!( + matches!(user_msg, Some(Message::User { content }) if content.contains("Review !`printf should-not-run`")), + "argument-inserted shell syntax must remain literal, got: {user_msg:?}" + ); +} + #[tokio::test] async fn skill_scoped_allowed_tools_limit_model_specs() { let seen = Arc::new(Mutex::new(Vec::new())); @@ -340,6 +368,94 @@ async fn skill_scoped_allowed_tools_limit_model_specs() { assert_eq!(seen_tools.lock().unwrap()[0], vec!["read".to_string()]); } +#[tokio::test] +async fn skill_tool_policy_matches_case_insensitively() { + let seen = Arc::new(Mutex::new(Vec::new())); + let seen_tools = Arc::new(Mutex::new(Vec::new())); + let model = ScriptedModel { + seen, + seen_tools: seen_tools.clone(), + chunks: vec![ModelChunk::End { + stop_reason: StopReason::EndTurn, + }], + }; + let session = Arc::new(Session::new( + Arc::new(model), + vec![ + Arc::new(EchoTool { + name: "read", + log: Arc::new(Mutex::new(Vec::new())), + }), + Arc::new(EchoTool { + name: "bash", + log: Arc::new(Mutex::new(Vec::new())), + }), + ], + )); + + let mut templates = HashMap::new(); + templates.insert( + "review".to_string(), + skill_template( + "Review.", + SkillRuntimeOptions { + allowed_tools: vec!["Read".to_string()], + disallowed_tools: vec!["BASH".to_string()], + ..SkillRuntimeOptions::default() + }, + ), + ); + + let runner = SessionRunner::new(session, "test-session".into(), Arc::new(NullHost)) + .with_prompt_templates(Arc::new(templates)); + + let outcome = runner.run_input("/review".to_string(), |_| {}).await; + assert_eq!(outcome, RunOutcome::Completed); + assert_eq!(seen_tools.lock().unwrap()[0], vec!["read".to_string()]); +} + +#[tokio::test] +async fn constrained_tool_policy_declarations_do_not_expand_to_whole_tool() { + let seen = Arc::new(Mutex::new(Vec::new())); + let seen_tools = Arc::new(Mutex::new(Vec::new())); + let model = ScriptedModel { + seen, + seen_tools: seen_tools.clone(), + chunks: vec![ModelChunk::End { + stop_reason: StopReason::EndTurn, + }], + }; + let session = Arc::new(Session::new( + Arc::new(model), + vec![Arc::new(EchoTool { + name: "bash", + log: Arc::new(Mutex::new(Vec::new())), + })], + )); + + let mut templates = HashMap::new(); + templates.insert( + "review".to_string(), + skill_template( + "Review.", + SkillRuntimeOptions { + allowed_tools: vec!["bash(git status:*)".to_string()], + ..SkillRuntimeOptions::default() + }, + ), + ); + + let runner = SessionRunner::new(session, "test-session".into(), Arc::new(NullHost)) + .with_prompt_templates(Arc::new(templates)); + + let outcome = runner.run_input("/review".to_string(), |_| {}).await; + assert_eq!(outcome, RunOutcome::Completed); + assert!( + seen_tools.lock().unwrap()[0].is_empty(), + "constrained declaration should not silently expose entire bash tool" + ); +} + #[tokio::test] async fn skill_scoped_disallowed_tools_block_execution() { let seen = Arc::new(Mutex::new(Vec::new())); @@ -470,6 +586,41 @@ async fn skill_scoped_hooks_are_temporary() { ); } +#[tokio::test] +async fn skill_stop_hook_runs_on_successful_completion() { + let seen = Arc::new(Mutex::new(Vec::new())); + let model = ScriptedModel::end_turn(seen.clone()); + let session = Arc::new(Session::new(Arc::new(model), vec![])); + + let marker = tempfile::NamedTempFile::new().unwrap(); + let marker_path = marker.path().to_string_lossy().into_owned(); + let mut hooks = HooksSection::default(); + hooks.stop.push(Hook { + matcher: ".*".to_string(), + command: format!("printf stop > {}", shell_quote(&marker_path)), + timeout: 5.0, + run_async: false, + }); + let mut templates = HashMap::new(); + templates.insert( + "guarded".to_string(), + skill_template( + "Guarded.", + SkillRuntimeOptions { + hooks, + ..SkillRuntimeOptions::default() + }, + ), + ); + + let runner = SessionRunner::new(session, "test-session".into(), Arc::new(NullHost)) + .with_prompt_templates(Arc::new(templates)); + + let outcome = runner.run_input("/guarded".to_string(), |_| {}).await; + assert_eq!(outcome, RunOutcome::Completed); + assert_eq!(std::fs::read_to_string(marker.path()).unwrap(), "stop"); +} + #[tokio::test] async fn skill_scoped_model_override_is_temporary() { let seen_models = Arc::new(Mutex::new(Vec::new())); @@ -530,7 +681,7 @@ async fn forked_skill_does_not_keep_internal_user_prompt() { skill_template( "Internal fork prompt.", SkillRuntimeOptions { - agent: Some(SkillAgentMode::Fork), + context: Some("fork".to_string()), ..SkillRuntimeOptions::default() }, ), @@ -557,3 +708,58 @@ async fn forked_skill_does_not_keep_internal_user_prompt() { "fork result should be appended to parent transcript: {messages:?}" ); } + +#[tokio::test] +async fn forked_skill_without_new_assistant_result_does_not_duplicate_parent_result() { + let seen = Arc::new(Mutex::new(Vec::new())); + let model = ScriptedModel { + seen, + seen_tools: Arc::new(Mutex::new(Vec::new())), + chunks: vec![ModelChunk::End { + stop_reason: StopReason::EndTurn, + }], + }; + let session = Arc::new(Session::new(Arc::new(model), vec![])); + session + .restore_messages(vec![Message::Assistant { + content: "parent answer".to_string(), + tool_calls: Vec::new(), + }]) + .await; + + let mut templates = HashMap::new(); + templates.insert( + "fork-review".to_string(), + skill_template( + "Internal fork prompt.", + SkillRuntimeOptions { + context: Some("fork".to_string()), + ..SkillRuntimeOptions::default() + }, + ), + ); + let runner = SessionRunner::new(session.clone(), "test-session".into(), Arc::new(NullHost)) + .with_prompt_templates(Arc::new(templates)); + + let outcome = runner.run_input("/fork-review".to_string(), |_| {}).await; + assert_eq!(outcome, RunOutcome::Completed); + + let messages = session.snapshot_messages().await; + let parent_answer_count = messages + .iter() + .filter(|msg| { + matches!( + msg, + Message::Assistant { content, .. } if content == "parent answer" + ) + }) + .count(); + assert_eq!( + parent_answer_count, 1, + "fork should not duplicate old result" + ); +} + +fn shell_quote(s: &str) -> String { + format!("'{}'", s.replace('\'', "'\\''")) +} diff --git a/tests/skills_discover.rs b/tests/skills_discover.rs index dfe9bbe..909dc5b 100644 --- a/tests/skills_discover.rs +++ b/tests/skills_discover.rs @@ -4,9 +4,7 @@ use ra::{ config::RaConfig, - skills::{ - build_resource_bundle, default_discover_globs, load_skills, ResourceBundle, SkillAgentMode, - }, + skills::{build_resource_bundle, default_discover_globs, load_skills, ResourceBundle}, }; use std::fs; use std::sync::{Mutex, OnceLock}; @@ -225,8 +223,8 @@ fn skill_parser_preserves_advanced_runtime_frontmatter() { description: Advanced skill model: review-model effort: high -context: Keep output concise. -agent: fork +context: fork +agent: Explore shell: bash allowed-tools: [read, "Bash(git status:*)"] disallowed-tools: "write, edit" @@ -252,11 +250,9 @@ Advanced body. assert_eq!(skill.runtime.model.as_deref(), Some("review-model")); assert_eq!(skill.runtime.effort.as_deref(), Some("high")); - assert_eq!( - skill.runtime.context.as_deref(), - Some("Keep output concise.") - ); - assert_eq!(skill.runtime.agent, Some(SkillAgentMode::Fork)); + assert_eq!(skill.runtime.context.as_deref(), Some("fork")); + assert_eq!(skill.runtime.agent.as_deref(), Some("Explore")); + assert!(skill.runtime.is_fork()); assert_eq!(skill.runtime.shell.as_deref(), Some("bash")); assert_eq!( skill.runtime.allowed_tools,