From fef6e0cc95b77c2f374e5de373f2e00700653096 Mon Sep 17 00:00:00 2001 From: Indra Gunanda Date: Tue, 24 Mar 2026 21:35:31 +0700 Subject: [PATCH 01/15] feat(tui): add question modal state fields, helpers, and event hooks (#32) --- crates/cli/src/tui/state.rs | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/crates/cli/src/tui/state.rs b/crates/cli/src/tui/state.rs index db1eadb..0a96b67 100644 --- a/crates/cli/src/tui/state.rs +++ b/crates/cli/src/tui/state.rs @@ -155,6 +155,10 @@ pub struct TuiSessionState { /// Agent profile picker popup. pub agent_picker_open: bool, pub agent_picker_index: usize, + /// Question modal popup (arrow-key option picker). + pub question_modal_open: bool, + pub question_modal_index: usize, + pub question_modal_scroll: usize, /// Command palette selection index (separate from slash_menu_index). pub palette_index: usize, /// Session picker popup (interactive list with resume). @@ -254,6 +258,9 @@ impl TuiSessionState { permission_picker_index: 0, agent_picker_open: false, agent_picker_index: 0, + question_modal_open: false, + question_modal_index: 0, + question_modal_scroll: 0, palette_index: 0, session_picker_open: false, session_picker_search: String::new(), @@ -352,6 +359,18 @@ impl TuiSessionState { self.agent_picker_index = 0; } + pub fn open_question_modal(&mut self) { + self.question_modal_open = true; + self.question_modal_index = 0; + self.question_modal_scroll = 0; + } + + pub fn close_question_modal(&mut self) { + self.question_modal_open = false; + self.question_modal_index = 0; + self.question_modal_scroll = 0; + } + pub fn open_session_picker(&mut self, entries: Vec, current: &str) { self.session_picker_open = true; self.session_picker_search.clear(); @@ -588,12 +607,14 @@ impl TuiSessionState { self.blocks.push(DisplayBlock::Question(question.clone())); // Bring the prompt into view when follow-tail is on (default). self.transcript_follow_tail = true; + self.open_question_modal(); } AgentEvent::QuestionResolved { question_id, selection, } => { self.active_question = None; + self.close_question_modal(); self.blocks.push(DisplayBlock::System(format!( "Answered question {question_id}: {selection:?}" ))); From 93fea2e351da6d5a38d807ffa14a40bd61570e57 Mon Sep 17 00:00:00 2001 From: Indra Gunanda Date: Tue, 24 Mar 2026 21:38:45 +0700 Subject: [PATCH 02/15] fix(tui): close question modal in clear_replayed_interaction_state (#32) Prevents stale modal-open state when resuming a session that was interrupted mid-question. --- crates/cli/src/tui/state.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/crates/cli/src/tui/state.rs b/crates/cli/src/tui/state.rs index 0a96b67..8bfe49c 100644 --- a/crates/cli/src/tui/state.rs +++ b/crates/cli/src/tui/state.rs @@ -422,6 +422,7 @@ impl TuiSessionState { pub fn clear_replayed_interaction_state(&mut self) { self.active_approval = None; self.active_question = None; + self.close_question_modal(); } pub fn clear_active_approval_if_matches(&mut self, call_id: &str) { From 4ac6177fbcc53d56129238bdfe9b5c93a1163ee7 Mon Sep 17 00:00:00 2001 From: Indra Gunanda Date: Tue, 24 Mar 2026 21:40:35 +0700 Subject: [PATCH 03/15] feat(tui): add question modal key event handling (#32) --- crates/cli/src/tui/app.rs | 58 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 58 insertions(+) diff --git a/crates/cli/src/tui/app.rs b/crates/cli/src/tui/app.rs index 85c235f..24eb6bb 100644 --- a/crates/cli/src/tui/app.rs +++ b/crates/cli/src/tui/app.rs @@ -2597,6 +2597,7 @@ pub fn run_blocking( Event::Mouse(_) if g.permission_picker_open => continue, Event::Mouse(_) if g.agent_picker_open => continue, Event::Mouse(_) if g.session_picker_open => continue, + Event::Mouse(_) if g.question_modal_open => continue, Event::Mouse(m) => { let sz = terminal.size()?; let area = Rect::new(0, 0, sz.width, sz.height); @@ -3060,6 +3061,63 @@ pub fn run_blocking( continue; } + // Question modal keyboard handling. + if g.question_modal_open { + if let Some(ref q) = g.active_question.clone() { + // Total items: 1 (suggested) + options.len() + (1 if allow_custom for "Chat about this") + let total = 1 + q.options.len() + if q.allow_custom { 1 } else { 0 }; + match (key.code, key.modifiers) { + (KeyCode::Esc, _) => { + if q.allow_custom { + // Fall back to inline text input + g.close_question_modal(); + } + // If !allow_custom, Esc is a no-op + } + (KeyCode::Up, _) | (KeyCode::Char('k'), KeyModifiers::NONE) => { + g.question_modal_index = + g.question_modal_index.saturating_sub(1); + } + (KeyCode::Down, _) | (KeyCode::Char('j'), KeyModifiers::NONE) => { + g.question_modal_index = + (g.question_modal_index + 1).min(total - 1); + } + (KeyCode::Enter, _) => { + let idx = g.question_modal_index; + let sel = if idx == 0 { + // Suggested answer + Some(QuestionSelection::Suggested) + } else if idx <= q.options.len() { + // Regular option (1-based → 0-based) + Some(QuestionSelection::Option { + option_id: q.options[idx - 1].id.clone(), + }) + } else { + // "Chat about this" — fall back to inline text input + None + }; + + if let Some(sel) = sel { + let qid = q.question_id.clone(); + g.close_question_modal(); + g.active_question = None; + drop(g); + if let Some(ref tx) = question_answer_tx { + let _ = tx.send((qid, sel)); + } else { + let _ = cmd_tx.send(TuiCmd::QuestionAnswer(sel)); + } + } else { + // "Chat about this" — close modal, keep active_question + g.close_question_modal(); + } + } + _ => {} + } + } + continue; + } + // Permission picker keyboard handling. if g.permission_picker_open { const PERM_COUNT: usize = 5; From d49c76581e289daabf5d330502f8b6e58b46066d Mon Sep 17 00:00:00 2001 From: Indra Gunanda Date: Tue, 24 Mar 2026 21:44:33 +0700 Subject: [PATCH 04/15] feat(tui): render question modal popup and suppress composer hint (#32) --- crates/cli/src/tui/app.rs | 109 +++++++++++++++++++++++++++++++++++++- 1 file changed, 108 insertions(+), 1 deletion(-) diff --git a/crates/cli/src/tui/app.rs b/crates/cli/src/tui/app.rs index 24eb6bb..397bb95 100644 --- a/crates/cli/src/tui/app.rs +++ b/crates/cli/src/tui/app.rs @@ -1798,7 +1798,7 @@ pub fn run_blocking( "Approval: y/n · Ctrl+Y approve · Ctrl+N deny · Ctrl+U always allow · /approve · /deny · other /commands still work", Style::default().fg(theme::ERROR), )) - } else if g.active_question.is_some() { + } else if g.active_question.is_some() && !g.question_modal_open { Line::from(Span::styled( "Enter / 0 = suggested · 1–n = option · click underlined line · /auto-answer · End = transcript bottom (empty input)", Style::default().fg(theme::WARN), @@ -2135,6 +2135,113 @@ pub fn run_blocking( frame.render_widget(popup, popup_area); } + // Question modal popup (arrow-key option picker). + if g.question_modal_open + && let Some(ref q) = g.active_question + { + let has_chat_option = q.allow_custom; + let total_items = 1 + q.options.len() + if has_chat_option { 1 } else { 0 }; + // +4 for: title line, blank, blank before footer, footer + let rows = (total_items as u16).saturating_add(6).max(8); + let popup_w = 60u16.min(area.width.saturating_sub(4)); + let popup_area = centered_rect(area, popup_w, rows); + + let mut lines: Vec = vec![ + Line::from(Span::styled( + format!(" {} ", q.prompt), + Style::default() + .fg(theme::ASSISTANT) + .add_modifier(Modifier::BOLD), + )), + Line::default(), + ]; + + // Suggested answer (index 0) + let suggested_label = format!(" Suggested: {} ", q.suggested_answer); + if g.question_modal_index == 0 { + lines.push(Line::from(Span::styled( + format!(" ► {}", suggested_label.trim()), + Style::default() + .fg(Color::Black) + .bg(theme::USER) + .add_modifier(Modifier::BOLD), + ))); + } else { + lines.push(Line::from(Span::styled( + format!(" {}", suggested_label.trim()), + Style::default().fg(theme::TEXT), + ))); + } + + // Options (index 1..n) + for (i, o) in q.options.iter().enumerate() { + let item_idx = i + 1; + let label = format!("{} ", o.label); + if g.question_modal_index == item_idx { + lines.push(Line::from(Span::styled( + format!(" ► {}", label.trim()), + Style::default() + .fg(Color::Black) + .bg(theme::USER) + .add_modifier(Modifier::BOLD), + ))); + } else { + lines.push(Line::from(Span::styled( + format!(" {}", label.trim()), + Style::default().fg(theme::TEXT), + ))); + } + } + + // "Chat about this" (last item, only if allow_custom) + if has_chat_option { + let chat_idx = 1 + q.options.len(); + if g.question_modal_index == chat_idx { + lines.push(Line::from(Span::styled( + " ► Chat about this", + Style::default() + .fg(Color::Black) + .bg(theme::USER) + .add_modifier(Modifier::BOLD), + ))); + } else { + lines.push(Line::from(Span::styled( + " Chat about this", + Style::default() + .fg(theme::MUTED) + .add_modifier(Modifier::ITALIC), + ))); + } + } + + // Footer + lines.push(Line::default()); + let footer_text = if has_chat_option { + " ↑↓ select · Enter confirm · Esc chat " + } else { + " ↑↓ select · Enter confirm " + }; + lines.push(Line::from(Span::styled( + footer_text, + Style::default().fg(theme::MUTED), + ))); + + frame.render_widget(ClearWidget, popup_area); + let popup = Paragraph::new(Text::from(lines)) + .block( + Block::default() + .borders(Borders::ALL) + .border_style(Style::default().fg(theme::BORDER)) + .title(Span::styled( + " question ", + Style::default().fg(theme::WARN), + )), + ) + .style(Style::default().bg(theme::SURFACE)) + .wrap(Wrap { trim: false }); + frame.render_widget(popup, popup_area); + } + if g.session_picker_open { let filter = g.session_picker_search.to_ascii_lowercase(); let filtered_indices: Vec = g.session_picker_entries.iter().enumerate() From a0ad081ab324406a511a61ee3766ac87bbc17401 Mon Sep 17 00:00:00 2001 From: Indra Gunanda Date: Tue, 24 Mar 2026 21:47:02 +0700 Subject: [PATCH 05/15] test(tui): add question modal state tests (#32) --- crates/cli/src/tui/state.rs | 72 +++++++++++++++++++++++++++++++++++++ 1 file changed, 72 insertions(+) diff --git a/crates/cli/src/tui/state.rs b/crates/cli/src/tui/state.rs index 8bfe49c..492f7ae 100644 --- a/crates/cli/src/tui/state.rs +++ b/crates/cli/src/tui/state.rs @@ -897,4 +897,76 @@ mod tests { assert!(st.active_approval.is_none()); assert!(st.active_question.is_none()); } + + #[test] + fn open_close_question_modal() { + let mut st = TuiSessionState::new( + "s".into(), + "m".into(), + "@build".into(), + "default".into(), + PathBuf::from("/tmp"), + ); + assert!(!st.question_modal_open); + assert_eq!(st.question_modal_index, 0); + + st.open_question_modal(); + assert!(st.question_modal_open); + assert_eq!(st.question_modal_index, 0); + assert_eq!(st.question_modal_scroll, 0); + + st.question_modal_index = 3; + st.close_question_modal(); + assert!(!st.question_modal_open); + assert_eq!(st.question_modal_index, 0); + assert_eq!(st.question_modal_scroll, 0); + } + + #[test] + fn question_requested_opens_modal() { + let mut st = TuiSessionState::new( + "s".into(), + "m".into(), + "@build".into(), + "default".into(), + PathBuf::from("/tmp"), + ); + let q = InteractiveQuestionPayload { + question_id: "q-1".into(), + call_id: "c1".into(), + prompt: "Pick".into(), + options: vec![QuestionOption { + id: "a".into(), + label: "A".into(), + }], + allow_custom: true, + suggested_answer: "A".into(), + }; + st.apply_event(&AgentEvent::QuestionRequested { + question: q.clone(), + }); + assert!(st.question_modal_open); + assert_eq!(st.question_modal_index, 0); + assert!(st.active_question.is_some()); + } + + #[test] + fn question_resolved_closes_modal() { + let mut st = TuiSessionState::new( + "s".into(), + "m".into(), + "@build".into(), + "default".into(), + PathBuf::from("/tmp"), + ); + st.question_modal_open = true; + st.question_modal_index = 2; + st.apply_event(&AgentEvent::QuestionResolved { + question_id: "q-1".into(), + selection: QuestionSelection::Suggested, + }); + assert!(st.active_question.is_none()); + assert!(!st.question_modal_open); + assert_eq!(st.question_modal_index, 0); + } } From 99cfd4e0c95705cb487bdaadf405ce6bc0985f77 Mon Sep 17 00:00:00 2001 From: Indra Gunanda Date: Tue, 24 Mar 2026 21:55:04 +0700 Subject: [PATCH 06/15] feat(tui): hide inline question options when modal is open (#32) Keep the question header and prompt visible in the transcript but skip the numbered options, tips, and click targets while the modal popup is handling selection. --- crates/cli/src/tui/app.rs | 67 ++++++++++++++++++++------------------- 1 file changed, 35 insertions(+), 32 deletions(-) diff --git a/crates/cli/src/tui/app.rs b/crates/cli/src/tui/app.rs index 397bb95..34e8eb0 100644 --- a/crates/cli/src/tui/app.rs +++ b/crates/cli/src/tui/app.rs @@ -1001,58 +1001,61 @@ fn transcript_lines_and_hits( None, ); } - push_transcript_line( - &mut lines, - &mut hits, - Line::from(vec![ - Span::styled( - format!(" [0] suggested: {} ", q.suggested_answer), - Style::default() - .fg(theme::SUCCESS) - .add_modifier(Modifier::UNDERLINED), - ), - Span::styled("(click)", Style::default().fg(theme::MUTED)), - ]), - Some(QuestionSelection::Suggested), - ); - for (i, o) in q.options.iter().enumerate() { + // When the modal is open, skip inline options — the popup handles selection. + if !state.question_modal_open { push_transcript_line( &mut lines, &mut hits, Line::from(vec![ Span::styled( - format!(" [{}] ({}) {} ", i + 1, o.id, o.label), + format!(" [0] suggested: {} ", q.suggested_answer), Style::default() - .fg(theme::TEXT) + .fg(theme::SUCCESS) .add_modifier(Modifier::UNDERLINED), ), Span::styled("(click)", Style::default().fg(theme::MUTED)), ]), - Some(QuestionSelection::Option { - option_id: o.id.clone(), - }), + Some(QuestionSelection::Suggested), ); - } - if q.allow_custom { + for (i, o) in q.options.iter().enumerate() { + push_transcript_line( + &mut lines, + &mut hits, + Line::from(vec![ + Span::styled( + format!(" [{}] ({}) {} ", i + 1, o.id, o.label), + Style::default() + .fg(theme::TEXT) + .add_modifier(Modifier::UNDERLINED), + ), + Span::styled("(click)", Style::default().fg(theme::MUTED)), + ]), + Some(QuestionSelection::Option { + option_id: o.id.clone(), + }), + ); + } + if q.allow_custom { + push_transcript_line( + &mut lines, + &mut hits, + Line::from(Span::styled( + " [c] type your own answer below, then Enter", + Style::default().fg(theme::MUTED), + )), + None, + ); + } push_transcript_line( &mut lines, &mut hits, Line::from(Span::styled( - " [c] type your own answer below, then Enter", + " Tip: /auto-answer or Enter on empty = suggested · click an option above", Style::default().fg(theme::MUTED), )), None, ); } - push_transcript_line( - &mut lines, - &mut hits, - Line::from(Span::styled( - " Tip: /auto-answer or Enter on empty = suggested · click an option above", - Style::default().fg(theme::MUTED), - )), - None, - ); push_transcript_line(&mut lines, &mut hits, Line::default(), None); } DisplayBlock::ErrorLine(s) => { From 57b0e82d44a6c4013aeb73115ce435e344637ad0 Mon Sep 17 00:00:00 2001 From: Indra Gunanda Date: Tue, 24 Mar 2026 23:00:09 +0700 Subject: [PATCH 07/15] chore: add regex dependency to nca-core (#34) --- Cargo.lock | 1 + crates/core/Cargo.toml | 1 + 2 files changed, 2 insertions(+) diff --git a/Cargo.lock b/Cargo.lock index 079c33f..351935a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2151,6 +2151,7 @@ dependencies = [ "globset", "mcpr", "nca-common", + "regex", "reqwest 0.12.28", "scraper", "serde", diff --git a/crates/core/Cargo.toml b/crates/core/Cargo.toml index 9b0dafe..d1bf639 100644 --- a/crates/core/Cargo.toml +++ b/crates/core/Cargo.toml @@ -22,6 +22,7 @@ scraper = "0.23" futures-util = "0.3" serde_yaml = "0.9.34" mcpr = "0.2.3" +regex = "1" [dev-dependencies] tempfile = "3" From 518411199b6cd6ea50e61aca0485b265770a356a Mon Sep 17 00:00:00 2001 From: Indra Gunanda Date: Tue, 24 Mar 2026 23:05:13 +0700 Subject: [PATCH 08/15] feat: add skill supporting file inlining (#34) --- crates/core/src/skills.rs | 390 +++++++++++++++++++++++++++++++++++++- 1 file changed, 389 insertions(+), 1 deletion(-) diff --git a/crates/core/src/skills.rs b/crates/core/src/skills.rs index 2b92718..a7f689d 100644 --- a/crates/core/src/skills.rs +++ b/crates/core/src/skills.rs @@ -108,7 +108,7 @@ impl Skill { let mut prompt = format!( "Use the skill `{}`.\n\nSkill instructions:\n{}\n", self.command, - self.body.trim() + self.expanded_body().trim() ); if !task.trim().is_empty() { prompt.push_str(&format!("\nTask:\n{}\n", task.trim())); @@ -142,6 +142,44 @@ impl Skill { SkillSource::FileSystem => "filesystem", } } + + /// Return the skill body with referenced supporting files inlined. + /// + /// Scans the body for file references (./path, @path, backtick-wrapped paths), + /// resolves them against the skill's directory, and appends their contents. + /// Bare filenames (no `/` or `./` prefix) resolve only against skill directory (Level 1). + pub fn expanded_body(&self) -> String { + let refs = extract_file_references(&self.body); + if refs.is_empty() { + return self.body.clone(); + } + + let mut expanded = self.body.clone(); + for ref_path in &refs { + let clean = ref_path.trim_start_matches("./"); + let is_bare_filename = !clean.contains('/'); + + let resolved = if is_bare_filename { + // Pattern 4: bare filenames resolve only against skill directory (Level 1) + let candidate = self.directory.join(clean); + if candidate.is_file() { + Some(candidate) + } else { + None + } + } else { + resolve_skill_reference(ref_path, &self.directory) + }; + + if let Some(resolved) = resolved + && let Ok(content) = std::fs::read_to_string(&resolved) + { + expanded.push_str(&format!("\n\n===== {} =====\n\n{}", clean, content.trim())); + } + } + + expanded + } } fn parse_skill_file(path: &Path) -> Result { @@ -394,6 +432,115 @@ fn parse_permission_mode_str(raw: &str) -> Option { } } +/// Resolve a file reference to an absolute path using three-level strategy: +/// 1. Relative to skill directory +/// 2. Relative to catalog root (parent of skill directory) +/// 3. Strip `skills/` prefix and retry against catalog root +/// +/// Returns `None` if the file doesn't exist at any level. +fn resolve_skill_reference(reference: &str, skill_directory: &Path) -> Option { + let clean = reference.trim_start_matches("./"); + + // Level 1: relative to skill directory + let level1 = skill_directory.join(clean); + if level1.is_file() { + return Some(level1); + } + + // Level 2: relative to catalog root (parent of skill directory) + if let Some(catalog_root) = skill_directory.parent() { + let level2 = catalog_root.join(clean); + if level2.is_file() { + return Some(level2); + } + + // Level 3: strip "skills/" prefix and retry against catalog root + if let Some(stripped) = clean.strip_prefix("skills/") { + let level3 = catalog_root.join(stripped); + if level3.is_file() { + return Some(level3); + } + } + } + + None +} + +/// Extract file references from a skill body. +/// +/// Detects: `./path`, `@path`, backtick-wrapped paths with supported extensions. +/// Returns deduplicated list in first-occurrence order, with `@` stripped and `./` preserved. +fn extract_file_references(body: &str) -> Vec { + use regex::Regex; + use std::collections::HashSet; + use std::sync::LazyLock; + + static EXCLUDED_NAMES: &[&str] = &[ + "CLAUDE.md", + "AGENTS.md", + "GEMINI.md", + "SKILL.md", + "README.md", + "package.json", + ]; + + static DOT_SLASH_RE: LazyLock = LazyLock::new(|| { + Regex::new(r"\./[a-zA-Z0-9_][a-zA-Z0-9_./-]*\.(md|sh|ts|js|dot|txt|html|cjs)").unwrap() + }); + + // Require @ at start of line or after whitespace to avoid matching emails + static AT_RE: LazyLock = LazyLock::new(|| { + Regex::new(r"(?:^|\s)@([a-zA-Z0-9_][a-zA-Z0-9_./-]*\.(md|sh|ts|js|dot|txt|html|cjs))") + .unwrap() + }); + + static BACKTICK_RE: LazyLock = LazyLock::new(|| { + Regex::new(r"`([a-zA-Z0-9_][a-zA-Z0-9_./-]*\.(md|sh|ts|js|dot|txt|html|cjs))`").unwrap() + }); + + fn is_excluded(path: &str) -> bool { + EXCLUDED_NAMES.contains(&path) + || path.contains("path/to/") + || path.starts_with("http://") + || path.starts_with("https://") + } + + let mut seen = HashSet::new(); + let mut refs = Vec::new(); + + // Pattern 1: ./path references + for m in DOT_SLASH_RE.find_iter(body) { + let path = m.as_str().to_string(); + let key = path.trim_start_matches("./").to_string(); + if !is_excluded(&key) && !seen.contains(&key) { + seen.insert(key); + refs.push(path); + } + } + + // Pattern 2: @path references (strip @, require word boundary) + for cap in AT_RE.captures_iter(body) { + let path = cap[1].to_string(); + let key = path.clone(); + if !is_excluded(&key) && !seen.contains(&key) { + seen.insert(key); + refs.push(path); + } + } + + // Pattern 3+4: backtick-wrapped paths + for cap in BACKTICK_RE.captures_iter(body) { + let path = cap[1].to_string(); + let key = path.trim_start_matches("./").to_string(); + if !is_excluded(&key) && !EXCLUDED_NAMES.contains(&path.as_str()) && !seen.contains(&key) { + seen.insert(key); + refs.push(path); + } + } + + refs +} + #[cfg(test)] mod tests { use super::*; @@ -468,4 +615,245 @@ Component management and styling... assert_eq!(shad.context, SkillContextMode::Fork); assert_eq!(shad.permission_mode, Some(PermissionMode::Plan)); } + + // === extract_file_references tests === + + #[test] + fn extracts_dot_slash_references() { + let body = "Read ./implementer-prompt.md for details.\nAlso see ./subdir/helper.sh here."; + let refs = extract_file_references(body); + assert_eq!(refs, vec!["./implementer-prompt.md", "./subdir/helper.sh"]); + } + + #[test] + fn extracts_at_prefixed_references() { + let body = + "Use @testing-anti-patterns.md to avoid pitfalls.\nSee @graphviz-conventions.dot too."; + let refs = extract_file_references(body); + assert_eq!( + refs, + vec!["testing-anti-patterns.md", "graphviz-conventions.dot"] + ); + } + + #[test] + fn extracts_backtick_paths_with_directory() { + let body = "Check `skills/brainstorming/visual-companion.md` for guidance.\nAlso `subdir/file.ts`."; + let refs = extract_file_references(body); + assert_eq!( + refs, + vec!["skills/brainstorming/visual-companion.md", "subdir/file.ts"] + ); + } + + #[test] + fn extracts_backtick_bare_filenames() { + let body = "See `code-reviewer.md` for the template.\nUse `helper.sh` too."; + let refs = extract_file_references(body); + assert_eq!(refs, vec!["code-reviewer.md", "helper.sh"]); + } + + #[test] + fn excludes_well_known_files_in_backticks() { + let body = "Check `CLAUDE.md` and `AGENTS.md` and `GEMINI.md` and `SKILL.md` and `README.md` and `package.json`."; + let refs = extract_file_references(body); + assert!(refs.is_empty()); + } + + #[test] + fn excludes_urls_and_template_paths() { + let body = "See https://example.com/file.md and path/to/test.md for examples."; + let refs = extract_file_references(body); + assert!(refs.is_empty()); + } + + #[test] + fn deduplicates_preserving_order() { + let body = "Use ./helper.md first.\nThen `helper.md` again.\nAnd @helper.md once more."; + let refs = extract_file_references(body); + assert_eq!(refs, vec!["./helper.md"]); + } + + // === resolve_skill_reference tests === + + #[test] + fn resolves_reference_in_skill_directory() { + let dir = tempfile::tempdir().unwrap(); + let skill_dir = dir.path().join("my-skill"); + std::fs::create_dir_all(&skill_dir).unwrap(); + std::fs::write(skill_dir.join("helper.md"), "helper content").unwrap(); + + let result = resolve_skill_reference("./helper.md", &skill_dir); + assert!(result.is_some()); + assert_eq!(result.unwrap(), skill_dir.join("helper.md")); + } + + #[test] + fn resolves_reference_via_catalog_root_fallback() { + let dir = tempfile::tempdir().unwrap(); + let catalog = dir.path().join("skills"); + let skill_dir = catalog.join("sdd"); + let other_skill = catalog.join("brainstorming"); + std::fs::create_dir_all(&skill_dir).unwrap(); + std::fs::create_dir_all(&other_skill).unwrap(); + std::fs::write(other_skill.join("visual.md"), "visual content").unwrap(); + + let result = resolve_skill_reference("brainstorming/visual.md", &skill_dir); + assert!(result.is_some()); + assert_eq!(result.unwrap(), other_skill.join("visual.md")); + } + + #[test] + fn resolves_skills_prefix_by_stripping() { + let dir = tempfile::tempdir().unwrap(); + let catalog = dir.path().join("skills"); + let brainstorming = catalog.join("brainstorming"); + std::fs::create_dir_all(&brainstorming).unwrap(); + std::fs::write(brainstorming.join("visual.md"), "content").unwrap(); + + let skill_dir = catalog.join("sdd"); + std::fs::create_dir_all(&skill_dir).unwrap(); + + let result = resolve_skill_reference("skills/brainstorming/visual.md", &skill_dir); + assert!(result.is_some()); + assert_eq!(result.unwrap(), brainstorming.join("visual.md")); + } + + #[test] + fn returns_none_for_missing_file() { + let dir = tempfile::tempdir().unwrap(); + let skill_dir = dir.path().join("my-skill"); + std::fs::create_dir_all(&skill_dir).unwrap(); + + let result = resolve_skill_reference("./nonexistent.md", &skill_dir); + assert!(result.is_none()); + } + + // === expanded_body tests === + + #[test] + fn expanded_body_inlines_referenced_files() { + let dir = tempfile::tempdir().unwrap(); + let skill_dir = dir.path().join("my-skill"); + std::fs::create_dir_all(&skill_dir).unwrap(); + std::fs::write(skill_dir.join("helper.md"), "Helper content here.").unwrap(); + + let skill = Skill { + name: "test".into(), + description: None, + command: "test".into(), + model: None, + permission_mode: None, + context: SkillContextMode::Inline, + directory: skill_dir, + body: "Main body.\n\nSee ./helper.md for details.".into(), + source: SkillSource::FileSystem, + }; + + let expanded = skill.expanded_body(); + assert!(expanded.contains("Main body.")); + assert!(expanded.contains("===== helper.md =====")); + assert!(expanded.contains("Helper content here.")); + } + + #[test] + fn expanded_body_skips_missing_files() { + let dir = tempfile::tempdir().unwrap(); + let skill_dir = dir.path().join("my-skill"); + std::fs::create_dir_all(&skill_dir).unwrap(); + + let skill = Skill { + name: "test".into(), + description: None, + command: "test".into(), + model: None, + permission_mode: None, + context: SkillContextMode::Inline, + directory: skill_dir, + body: "Main body.\n\nSee ./nonexistent.md for details.".into(), + source: SkillSource::FileSystem, + }; + + let expanded = skill.expanded_body(); + assert_eq!(expanded, "Main body.\n\nSee ./nonexistent.md for details."); + } + + #[test] + fn expanded_body_inlines_at_prefixed_reference() { + let dir = tempfile::tempdir().unwrap(); + let skill_dir = dir.path().join("tdd"); + std::fs::create_dir_all(&skill_dir).unwrap(); + std::fs::write( + skill_dir.join("testing-anti-patterns.md"), + "Anti-pattern content.", + ) + .unwrap(); + + let skill = Skill { + name: "test".into(), + description: None, + command: "test".into(), + model: None, + permission_mode: None, + context: SkillContextMode::Inline, + directory: skill_dir, + body: "Main body.\n\nRead @testing-anti-patterns.md to avoid pitfalls.".into(), + source: SkillSource::FileSystem, + }; + + let expanded = skill.expanded_body(); + assert!(expanded.contains("===== testing-anti-patterns.md =====")); + assert!(expanded.contains("Anti-pattern content.")); + } + + #[test] + fn expanded_body_bare_filename_resolves_only_in_skill_dir() { + let dir = tempfile::tempdir().unwrap(); + let catalog = dir.path().join("skills"); + let skill_dir = catalog.join("review"); + let other_dir = catalog.join("other"); + std::fs::create_dir_all(&skill_dir).unwrap(); + std::fs::create_dir_all(&other_dir).unwrap(); + std::fs::write(other_dir.join("template.md"), "Other content.").unwrap(); + + let skill = Skill { + name: "test".into(), + description: None, + command: "test".into(), + model: None, + permission_mode: None, + context: SkillContextMode::Inline, + directory: skill_dir, + body: "See `template.md` for the template.".into(), + source: SkillSource::FileSystem, + }; + + let expanded = skill.expanded_body(); + assert!(!expanded.contains("=====")); + assert!(!expanded.contains("Other content.")); + } + + #[test] + fn expanded_body_deduplicates() { + let dir = tempfile::tempdir().unwrap(); + let skill_dir = dir.path().join("my-skill"); + std::fs::create_dir_all(&skill_dir).unwrap(); + std::fs::write(skill_dir.join("helper.md"), "Helper.").unwrap(); + + let skill = Skill { + name: "test".into(), + description: None, + command: "test".into(), + model: None, + permission_mode: None, + context: SkillContextMode::Inline, + directory: skill_dir, + body: "See ./helper.md and `helper.md` again.".into(), + source: SkillSource::FileSystem, + }; + + let expanded = skill.expanded_body(); + let count = expanded.matches("===== helper.md =====").count(); + assert_eq!(count, 1); + } } From af4dbad29ea85d3f6c4ad8c18131945dc934d2ca Mon Sep 17 00:00:00 2001 From: Indra Gunanda Date: Tue, 24 Mar 2026 23:19:10 +0700 Subject: [PATCH 09/15] feat: add invoke_skill tool for dynamic skill loading (#34) --- crates/core/src/harness.rs | 2 +- crates/core/src/tools/invoke_skill.rs | 186 ++++++++++++++++++++++++++ crates/core/src/tools/mod.rs | 2 + crates/runtime/src/supervisor.rs | 5 + 4 files changed, 194 insertions(+), 1 deletion(-) create mode 100644 crates/core/src/tools/invoke_skill.rs diff --git a/crates/core/src/harness.rs b/crates/core/src/harness.rs index 6d9426f..52f27bd 100644 --- a/crates/core/src/harness.rs +++ b/crates/core/src/harness.rs @@ -142,7 +142,7 @@ fn skills_section( section.push('\n'); } section.push_str( - "\nUse these skill summaries when relevant. Full skill instructions are loaded only when explicitly invoked by the user or REPL.", + "\nUse the invoke_skill tool to load full instructions when a task matches a skill.", ); Some(section) } diff --git a/crates/core/src/tools/invoke_skill.rs b/crates/core/src/tools/invoke_skill.rs new file mode 100644 index 0000000..d6019f8 --- /dev/null +++ b/crates/core/src/tools/invoke_skill.rs @@ -0,0 +1,186 @@ +//! Tool that lets the LLM load a skill's full instructions by name. + +use crate::skills::SkillCatalog; +use crate::tools::ToolExecutor; +use nca_common::tool::{ToolCall, ToolDefinition, ToolResult}; +use std::path::PathBuf; + +pub struct InvokeSkillTool { + workspace_root: PathBuf, + skill_directories: Vec, +} + +impl InvokeSkillTool { + pub fn new(workspace_root: PathBuf, skill_directories: Vec) -> Self { + Self { + workspace_root, + skill_directories, + } + } +} + +#[async_trait::async_trait] +impl ToolExecutor for InvokeSkillTool { + fn definition(&self) -> ToolDefinition { + ToolDefinition { + name: "invoke_skill".into(), + description: "Load a skill's full instructions by name. Use this when a task matches \ + an available skill from the skills manifest. Returns the complete skill \ + instructions to follow." + .into(), + parameters: serde_json::json!({ + "type": "object", + "properties": { + "skill_name": { + "type": "string", + "description": "The command name from the skills manifest (e.g., 'brainstorming', 'test-driven-development')" + } + }, + "required": ["skill_name"] + }), + } + } + + async fn execute(&self, call: &ToolCall) -> ToolResult { + let skill_name = call.input["skill_name"] + .as_str() + .unwrap_or("") + .trim() + .to_string(); + + if skill_name.is_empty() { + return ToolResult { + call_id: call.id.clone(), + success: false, + output: String::new(), + error: Some("skill_name is required".into()), + }; + } + + let skills = match SkillCatalog::discover(&self.workspace_root, &self.skill_directories) { + Ok(s) => s, + Err(e) => { + return ToolResult { + call_id: call.id.clone(), + success: false, + output: String::new(), + error: Some(format!("Failed to discover skills: {e}")), + }; + } + }; + + if let Some(skill) = skills.iter().find(|s| s.command == skill_name) { + let body = skill.expanded_body(); + ToolResult { + call_id: call.id.clone(), + success: true, + output: format!( + "Skill `{}` loaded. Follow these instructions:\n\n{}", + skill.command, + body.trim() + ), + error: None, + } + } else { + let available: Vec<&str> = skills.iter().map(|s| s.command.as_str()).collect(); + ToolResult { + call_id: call.id.clone(), + success: false, + output: String::new(), + error: Some(format!( + "Skill '{}' not found. Available skills: {}", + skill_name, + available.join(", ") + )), + } + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + fn make_tool(workspace: &std::path::Path) -> InvokeSkillTool { + InvokeSkillTool::new( + workspace.to_path_buf(), + vec![std::path::PathBuf::from(".nca/skills")], + ) + } + + fn make_call(skill_name: &str) -> ToolCall { + ToolCall { + id: "call-1".into(), + name: "invoke_skill".into(), + input: serde_json::json!({ "skill_name": skill_name }), + } + } + + #[test] + fn definition_has_correct_name_and_parameters() { + let dir = tempfile::tempdir().unwrap(); + let tool = make_tool(dir.path()); + let def = tool.definition(); + assert_eq!(def.name, "invoke_skill"); + assert!(def.description.contains("Load a skill")); + assert!(def.parameters["properties"]["skill_name"].is_object()); + assert!( + def.parameters["required"] + .as_array() + .unwrap() + .contains(&serde_json::json!("skill_name")) + ); + } + + #[tokio::test] + async fn returns_expanded_body_for_valid_skill() { + let dir = tempfile::tempdir().unwrap(); + let skill_dir = dir.path().join(".nca/skills/my-skill"); + std::fs::create_dir_all(&skill_dir).unwrap(); + std::fs::write( + skill_dir.join("SKILL.md"), + "---\nname: My Skill\ncommand: my-skill\ndescription: A test skill\n---\nDo the thing.\n\nSee ./helper.md for details.\n", + ) + .unwrap(); + std::fs::write(skill_dir.join("helper.md"), "Helper content.").unwrap(); + + let tool = make_tool(dir.path()); + let result = tool.execute(&make_call("my-skill")).await; + + assert!(result.success); + assert!(result.output.contains("Skill `my-skill` loaded")); + assert!(result.output.contains("Do the thing.")); + assert!(result.output.contains("===== helper.md =====")); + assert!(result.output.contains("Helper content.")); + } + + #[tokio::test] + async fn returns_error_for_unknown_skill() { + let dir = tempfile::tempdir().unwrap(); + let skill_dir = dir.path().join(".nca/skills/real-skill"); + std::fs::create_dir_all(&skill_dir).unwrap(); + std::fs::write( + skill_dir.join("SKILL.md"), + "---\nname: Real\ncommand: real-skill\n---\nReal body.\n", + ) + .unwrap(); + + let tool = make_tool(dir.path()); + let result = tool.execute(&make_call("nonexistent")).await; + + assert!(!result.success); + let err = result.error.unwrap(); + assert!(err.contains("not found")); + assert!(err.contains("real-skill")); + } + + #[tokio::test] + async fn returns_error_for_empty_skill_name() { + let dir = tempfile::tempdir().unwrap(); + let tool = make_tool(dir.path()); + let result = tool.execute(&make_call("")).await; + + assert!(!result.success); + assert!(result.error.unwrap().contains("skill_name is required")); + } +} diff --git a/crates/core/src/tools/mod.rs b/crates/core/src/tools/mod.rs index 1928e9b..4624cae 100644 --- a/crates/core/src/tools/mod.rs +++ b/crates/core/src/tools/mod.rs @@ -9,6 +9,7 @@ pub mod edit_file; pub mod fetch_url; pub mod filesystem; pub mod git; +pub mod invoke_skill; pub mod list_directory; pub mod mcp; pub mod move_path; @@ -21,6 +22,7 @@ pub mod web_search; pub mod write_file; pub use ask_question::AskQuestionTool; +pub use invoke_skill::InvokeSkillTool; use nca_common::config::WebConfig; use nca_common::tool::{ToolCall, ToolDefinition, ToolResult}; diff --git a/crates/runtime/src/supervisor.rs b/crates/runtime/src/supervisor.rs index 0d956ed..67ff8b0 100644 --- a/crates/runtime/src/supervisor.rs +++ b/crates/runtime/src/supervisor.rs @@ -18,6 +18,7 @@ use nca_core::hooks::{HookEventKind, HookRunner}; use nca_core::provider::ProviderError; use nca_core::provider::factory::build_provider; use nca_core::tools::AskQuestionTool; +use nca_core::tools::InvokeSkillTool; use nca_core::tools::ToolRegistry; use nca_core::tools::mcp::load_mcp_tools; use nca_core::tools::spawn_subagent::{SpawnRequest, SpawnSubagentTool}; @@ -181,6 +182,10 @@ impl Supervisor { event_tx.clone(), question_pending.clone(), ))); + tools.register(Box::new(InvokeSkillTool::new( + workspace_root.clone(), + config.harness.skill_directories.clone(), + ))); let session_id = cfg.session_id.unwrap_or_else(generate_session_id); let session_store = SessionStore::new(workspace_root.join(&config.session.history_dir)); From 7f4ed24c147bf444cb2040206ca55ad056879174 Mon Sep 17 00:00:00 2001 From: Indra Gunanda Date: Tue, 24 Mar 2026 23:25:05 +0700 Subject: [PATCH 10/15] feat: add subagent skill indicator in sidebar and transcript (#34) --- crates/cli/src/tui/app.rs | 6 ++++++ crates/cli/src/tui/state.rs | 10 ++++++++++ crates/core/src/tools/invoke_skill.rs | 24 +++++++++++++++++++++++- crates/runtime/src/supervisor.rs | 5 +++-- 4 files changed, 42 insertions(+), 3 deletions(-) diff --git a/crates/cli/src/tui/app.rs b/crates/cli/src/tui/app.rs index 34e8eb0..660404a 100644 --- a/crates/cli/src/tui/app.rs +++ b/crates/cli/src/tui/app.rs @@ -1555,6 +1555,12 @@ pub fn run_blocking( Style::default().fg(theme::MUTED), ))); } + if let Some(ref skill_name) = row.skill { + todo_lines.push(Line::from(Span::styled( + format!(" [{}]", sidebar_fit(skill_name, 24)), + Style::default().fg(theme::WARN), + ))); + } if !row.task.is_empty() && row.task != "(sub-agent)" { todo_lines.push(Line::from(Span::styled( format!(" {}", sidebar_fit(&row.task, 26)), diff --git a/crates/cli/src/tui/state.rs b/crates/cli/src/tui/state.rs index 492f7ae..8325f18 100644 --- a/crates/cli/src/tui/state.rs +++ b/crates/cli/src/tui/state.rs @@ -48,6 +48,7 @@ pub struct SubagentRow { pub phase: String, pub detail: String, pub running: bool, + pub skill: Option, } /// Status of an API key validation during onboarding. @@ -655,6 +656,7 @@ impl TuiSessionState { phase: String::new(), detail: String::new(), running: true, + skill: None, }); } self.blocks.push(DisplayBlock::System(format!( @@ -677,6 +679,9 @@ impl TuiSessionState { row.phase = phase.clone(); row.detail = d.clone(); row.running = true; + if phase == "skill" { + row.skill = Some(detail.clone()); + } } else { self.subagents.push(SubagentRow { id: child_session_id.clone(), @@ -684,6 +689,11 @@ impl TuiSessionState { phase: phase.clone(), detail: d.clone(), running: true, + skill: if phase == "skill" { + Some(detail.clone()) + } else { + None + }, }); } self.blocks diff --git a/crates/core/src/tools/invoke_skill.rs b/crates/core/src/tools/invoke_skill.rs index d6019f8..fb641f3 100644 --- a/crates/core/src/tools/invoke_skill.rs +++ b/crates/core/src/tools/invoke_skill.rs @@ -2,19 +2,30 @@ use crate::skills::SkillCatalog; use crate::tools::ToolExecutor; +use nca_common::event::AgentEvent; use nca_common::tool::{ToolCall, ToolDefinition, ToolResult}; use std::path::PathBuf; +use tokio::sync::mpsc; pub struct InvokeSkillTool { workspace_root: PathBuf, skill_directories: Vec, + event_tx: mpsc::Sender, + session_id: String, } impl InvokeSkillTool { - pub fn new(workspace_root: PathBuf, skill_directories: Vec) -> Self { + pub fn new( + workspace_root: PathBuf, + skill_directories: Vec, + event_tx: mpsc::Sender, + session_id: String, + ) -> Self { Self { workspace_root, skill_directories, + event_tx, + session_id, } } } @@ -71,6 +82,14 @@ impl ToolExecutor for InvokeSkillTool { if let Some(skill) = skills.iter().find(|s| s.command == skill_name) { let body = skill.expanded_body(); + let _ = self + .event_tx + .send(AgentEvent::ChildSessionActivity { + child_session_id: self.session_id.clone(), + phase: "skill".into(), + detail: skill.command.clone(), + }) + .await; ToolResult { call_id: call.id.clone(), success: true, @@ -102,9 +121,12 @@ mod tests { use super::*; fn make_tool(workspace: &std::path::Path) -> InvokeSkillTool { + let (tx, _rx) = tokio::sync::mpsc::channel(16); InvokeSkillTool::new( workspace.to_path_buf(), vec![std::path::PathBuf::from(".nca/skills")], + tx, + "test-session".into(), ) } diff --git a/crates/runtime/src/supervisor.rs b/crates/runtime/src/supervisor.rs index 67ff8b0..d7bb022 100644 --- a/crates/runtime/src/supervisor.rs +++ b/crates/runtime/src/supervisor.rs @@ -182,12 +182,13 @@ impl Supervisor { event_tx.clone(), question_pending.clone(), ))); + let session_id = cfg.session_id.unwrap_or_else(generate_session_id); tools.register(Box::new(InvokeSkillTool::new( workspace_root.clone(), config.harness.skill_directories.clone(), + event_tx.clone(), + session_id.clone(), ))); - - let session_id = cfg.session_id.unwrap_or_else(generate_session_id); let session_store = SessionStore::new(workspace_root.join(&config.session.history_dir)); let ipc_server = IpcServer::new(&session_id); From 3c488e3e8772804c93e989f1ffa6d7f9c3abbc77 Mon Sep 17 00:00:00 2001 From: Indra Gunanda Date: Wed, 25 Mar 2026 07:33:30 +0700 Subject: [PATCH 11/15] fix: security and stability fixes for skill invocation (#34) - Remove stale session_id from InvokeSkillTool (was not updated on session reset, causing events tagged to wrong session) - Block path traversal (../) in skill file reference resolution - Prevent AGENTS.md skills from leaking parent directory files via Level-2 fallback (only filesystem skills use catalog root fallback) - Detect invoke_skill phase in subagent activity for sidebar indicator - Add path traversal rejection test --- crates/cli/src/tui/state.rs | 4 +- crates/core/src/skills.rs | 60 +- crates/core/src/tools/invoke_skill.rs | 24 +- crates/runtime/src/supervisor.rs | 4 +- .../plans/2026-03-24-invoke-skill-tool.md | 365 +++++++++++ .../plans/2026-03-24-question-modal-picker.md | 532 +++++++++++++++ .../2026-03-24-skill-supporting-files.md | 603 ++++++++++++++++++ .../2026-03-24-invoke-skill-tool-design.md | 111 ++++ ...2026-03-24-question-modal-picker-design.md | 131 ++++ ...026-03-24-skill-supporting-files-design.md | 122 ++++ ...6-03-24-subagent-skill-indicator-design.md | 66 ++ 11 files changed, 1985 insertions(+), 37 deletions(-) create mode 100644 docs/superpowers/plans/2026-03-24-invoke-skill-tool.md create mode 100644 docs/superpowers/plans/2026-03-24-question-modal-picker.md create mode 100644 docs/superpowers/plans/2026-03-24-skill-supporting-files.md create mode 100644 docs/superpowers/specs/2026-03-24-invoke-skill-tool-design.md create mode 100644 docs/superpowers/specs/2026-03-24-question-modal-picker-design.md create mode 100644 docs/superpowers/specs/2026-03-24-skill-supporting-files-design.md create mode 100644 docs/superpowers/specs/2026-03-24-subagent-skill-indicator-design.md diff --git a/crates/cli/src/tui/state.rs b/crates/cli/src/tui/state.rs index 8325f18..542a85b 100644 --- a/crates/cli/src/tui/state.rs +++ b/crates/cli/src/tui/state.rs @@ -679,7 +679,7 @@ impl TuiSessionState { row.phase = phase.clone(); row.detail = d.clone(); row.running = true; - if phase == "skill" { + if phase == "skill" || phase == "invoke_skill" { row.skill = Some(detail.clone()); } } else { @@ -689,7 +689,7 @@ impl TuiSessionState { phase: phase.clone(), detail: d.clone(), running: true, - skill: if phase == "skill" { + skill: if phase == "skill" || phase == "invoke_skill" { Some(detail.clone()) } else { None diff --git a/crates/core/src/skills.rs b/crates/core/src/skills.rs index a7f689d..0378f24 100644 --- a/crates/core/src/skills.rs +++ b/crates/core/src/skills.rs @@ -157,6 +157,12 @@ impl Skill { let mut expanded = self.body.clone(); for ref_path in &refs { let clean = ref_path.trim_start_matches("./"); + + // Block directory traversal + if clean.contains("..") { + continue; + } + let is_bare_filename = !clean.contains('/'); let resolved = if is_bare_filename { @@ -438,9 +444,15 @@ fn parse_permission_mode_str(raw: &str) -> Option { /// 3. Strip `skills/` prefix and retry against catalog root /// /// Returns `None` if the file doesn't exist at any level. +/// Rejects paths containing `..` to prevent directory traversal. fn resolve_skill_reference(reference: &str, skill_directory: &Path) -> Option { let clean = reference.trim_start_matches("./"); + // Block directory traversal + if clean.contains("..") { + return None; + } + // Level 1: relative to skill directory let level1 = skill_directory.join(clean); if level1.is_file() { @@ -448,17 +460,35 @@ fn resolve_skill_reference(reference: &str, skill_directory: &Path) -> Option//SKILL.md + // AGENTS.md skills have directory == workspace_root, so parent is workspace parent. + let skill_dir_name = skill_directory + .file_name() + .and_then(|n| n.to_str()) + .unwrap_or(""); + let is_likely_skill_subdir = + !skill_dir_name.is_empty() && skill_dir_name != "." && skill_directory != catalog_root; + + if is_likely_skill_subdir { + let level2 = catalog_root.join(clean); + if level2.is_file() { + return Some(level2); + } - // Level 3: strip "skills/" prefix and retry against catalog root - if let Some(stripped) = clean.strip_prefix("skills/") { - let level3 = catalog_root.join(stripped); - if level3.is_file() { - return Some(level3); + // Level 3: strip "skills/" prefix and retry against catalog root + if let Some(stripped) = clean.strip_prefix("skills/") + && !stripped.contains("..") + { + let level3 = catalog_root.join(stripped); + if level3.is_file() { + return Some(level3); + } } } } @@ -729,6 +759,18 @@ Component management and styling... assert!(result.is_none()); } + #[test] + fn rejects_path_traversal() { + let dir = tempfile::tempdir().unwrap(); + let skill_dir = dir.path().join("my-skill"); + std::fs::create_dir_all(&skill_dir).unwrap(); + // Create a file in the parent that should NOT be reachable + std::fs::write(dir.path().join("secret.md"), "secret content").unwrap(); + + let result = resolve_skill_reference("../secret.md", &skill_dir); + assert!(result.is_none()); + } + // === expanded_body tests === #[test] diff --git a/crates/core/src/tools/invoke_skill.rs b/crates/core/src/tools/invoke_skill.rs index fb641f3..d6019f8 100644 --- a/crates/core/src/tools/invoke_skill.rs +++ b/crates/core/src/tools/invoke_skill.rs @@ -2,30 +2,19 @@ use crate::skills::SkillCatalog; use crate::tools::ToolExecutor; -use nca_common::event::AgentEvent; use nca_common::tool::{ToolCall, ToolDefinition, ToolResult}; use std::path::PathBuf; -use tokio::sync::mpsc; pub struct InvokeSkillTool { workspace_root: PathBuf, skill_directories: Vec, - event_tx: mpsc::Sender, - session_id: String, } impl InvokeSkillTool { - pub fn new( - workspace_root: PathBuf, - skill_directories: Vec, - event_tx: mpsc::Sender, - session_id: String, - ) -> Self { + pub fn new(workspace_root: PathBuf, skill_directories: Vec) -> Self { Self { workspace_root, skill_directories, - event_tx, - session_id, } } } @@ -82,14 +71,6 @@ impl ToolExecutor for InvokeSkillTool { if let Some(skill) = skills.iter().find(|s| s.command == skill_name) { let body = skill.expanded_body(); - let _ = self - .event_tx - .send(AgentEvent::ChildSessionActivity { - child_session_id: self.session_id.clone(), - phase: "skill".into(), - detail: skill.command.clone(), - }) - .await; ToolResult { call_id: call.id.clone(), success: true, @@ -121,12 +102,9 @@ mod tests { use super::*; fn make_tool(workspace: &std::path::Path) -> InvokeSkillTool { - let (tx, _rx) = tokio::sync::mpsc::channel(16); InvokeSkillTool::new( workspace.to_path_buf(), vec![std::path::PathBuf::from(".nca/skills")], - tx, - "test-session".into(), ) } diff --git a/crates/runtime/src/supervisor.rs b/crates/runtime/src/supervisor.rs index d7bb022..6d5eb25 100644 --- a/crates/runtime/src/supervisor.rs +++ b/crates/runtime/src/supervisor.rs @@ -182,13 +182,11 @@ impl Supervisor { event_tx.clone(), question_pending.clone(), ))); - let session_id = cfg.session_id.unwrap_or_else(generate_session_id); tools.register(Box::new(InvokeSkillTool::new( workspace_root.clone(), config.harness.skill_directories.clone(), - event_tx.clone(), - session_id.clone(), ))); + let session_id = cfg.session_id.unwrap_or_else(generate_session_id); let session_store = SessionStore::new(workspace_root.join(&config.session.history_dir)); let ipc_server = IpcServer::new(&session_id); diff --git a/docs/superpowers/plans/2026-03-24-invoke-skill-tool.md b/docs/superpowers/plans/2026-03-24-invoke-skill-tool.md new file mode 100644 index 0000000..e14fcb6 --- /dev/null +++ b/docs/superpowers/plans/2026-03-24-invoke-skill-tool.md @@ -0,0 +1,365 @@ +# Invoke Skill Tool Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Add an `invoke_skill` tool that the LLM can call to dynamically load a skill's full instructions. + +**Architecture:** Create a new `InvokeSkillTool` struct implementing `ToolExecutor`, register it post-construction in `supervisor.rs` (for both safe and full modes), and update the system prompt footer to tell the LLM to use the tool. + +**Tech Stack:** Rust, async-trait + +**Spec:** `docs/superpowers/specs/2026-03-24-invoke-skill-tool-design.md` + +--- + +### Task 1: Create `invoke_skill.rs` tool implementation + +**Files:** +- Create: `crates/core/src/tools/invoke_skill.rs` + +- [ ] **Step 1: Create the tool file with struct and constructor** + +Create `crates/core/src/tools/invoke_skill.rs`: + +```rust +//! Tool that lets the LLM load a skill's full instructions by name. + +use crate::skills::SkillCatalog; +use crate::tools::ToolExecutor; +use nca_common::tool::{ToolCall, ToolDefinition, ToolResult}; +use std::path::PathBuf; + +pub struct InvokeSkillTool { + workspace_root: PathBuf, + skill_directories: Vec, +} + +impl InvokeSkillTool { + pub fn new(workspace_root: PathBuf, skill_directories: Vec) -> Self { + Self { + workspace_root, + skill_directories, + } + } +} + +#[async_trait::async_trait] +impl ToolExecutor for InvokeSkillTool { + fn definition(&self) -> ToolDefinition { + ToolDefinition { + name: "invoke_skill".into(), + description: "Load a skill's full instructions by name. Use this when a task matches \ + an available skill from the skills manifest. Returns the complete skill \ + instructions to follow." + .into(), + parameters: serde_json::json!({ + "type": "object", + "properties": { + "skill_name": { + "type": "string", + "description": "The command name from the skills manifest (e.g., 'brainstorming', 'test-driven-development')" + } + }, + "required": ["skill_name"] + }), + } + } + + async fn execute(&self, call: &ToolCall) -> ToolResult { + let skill_name = call.input["skill_name"] + .as_str() + .unwrap_or("") + .trim() + .to_string(); + + if skill_name.is_empty() { + return ToolResult { + call_id: call.id.clone(), + success: false, + output: String::new(), + error: Some("skill_name is required".into()), + }; + } + + let skills = match SkillCatalog::discover(&self.workspace_root, &self.skill_directories) { + Ok(s) => s, + Err(e) => { + return ToolResult { + call_id: call.id.clone(), + success: false, + output: String::new(), + error: Some(format!("Failed to discover skills: {e}")), + }; + } + }; + + if let Some(skill) = skills.iter().find(|s| s.command == skill_name) { + let body = skill.expanded_body(); + ToolResult { + call_id: call.id.clone(), + success: true, + output: format!( + "Skill `{}` loaded. Follow these instructions:\n\n{}", + skill.command, + body.trim() + ), + error: None, + } + } else { + let available: Vec<&str> = skills.iter().map(|s| s.command.as_str()).collect(); + ToolResult { + call_id: call.id.clone(), + success: false, + output: String::new(), + error: Some(format!( + "Skill '{}' not found. Available skills: {}", + skill_name, + available.join(", ") + )), + } + } + } +} +``` + +- [ ] **Step 2: Verify it compiles** + +Run: `cargo check -p nca-core 2>&1 | tail -5` +Expected: may warn about dead code (not registered yet), but no errors + +- [ ] **Step 3: Commit** + +```bash +git add crates/core/src/tools/invoke_skill.rs +git commit -m "feat: add invoke_skill tool implementation (#34)" +``` + +--- + +### Task 2: Export module and register the tool + +**Files:** +- Modify: `crates/core/src/tools/mod.rs:1-23` (add module and re-export) +- Modify: `crates/runtime/src/supervisor.rs:148-155` (register tool) + +- [ ] **Step 1: Add module declaration and re-export in `mod.rs`** + +In `crates/core/src/tools/mod.rs`, add after `pub mod ask_question;` (line 2): + +```rust +pub mod invoke_skill; +``` + +And after `pub use ask_question::AskQuestionTool;` (line 23), add: + +```rust +pub use invoke_skill::InvokeSkillTool; +``` + +- [ ] **Step 2: Register `InvokeSkillTool` in `supervisor.rs`** + +In `crates/runtime/src/supervisor.rs`, add the import at the top (after the existing `use nca_core::tools::AskQuestionTool;` on line 20): + +```rust +use nca_core::tools::InvokeSkillTool; +``` + +Then add the registration after the `AskQuestionTool` block (after line 183), following the same post-construction pattern: + +```rust +tools.register(Box::new(InvokeSkillTool::new( + workspace_root.clone(), + config.harness.skill_directories.clone(), +))); +``` + +Note: `config.harness.skill_directories` is a `Vec` field on `NcaConfig.harness` — the same field used by `build_system_prompt` in `harness.rs`. This registers in both safe and full modes since the tool is read-only. + +- [ ] **Step 3: Verify it compiles** + +Run: `cargo check -p nca-runtime 2>&1 | tail -5` +Expected: no errors + +- [ ] **Step 4: Commit** + +```bash +git add crates/core/src/tools/mod.rs crates/runtime/src/supervisor.rs +git commit -m "feat: register invoke_skill tool in supervisor (#34)" +``` + +--- + +### Task 3: Update system prompt skills footer + +**Files:** +- Modify: `crates/core/src/harness.rs:144-146` + +- [ ] **Step 1: Update the footer text** + +In `crates/core/src/harness.rs`, change line 144-146 from: + +```rust + section.push_str( + "\nUse these skill summaries when relevant. Full skill instructions are loaded only when explicitly invoked by the user or REPL.", + ); +``` + +To: + +```rust + section.push_str( + "\nUse the invoke_skill tool to load full instructions when a task matches a skill.", + ); +``` + +- [ ] **Step 2: Search for any test asserting the old footer string** + +Run: `grep -n "explicitly invoked\|Full skill instructions" crates/core/src/harness.rs` + +If any test asserts the old footer text, update it to match the new string. The `layers_sections_in_stable_order` test only asserts `"Available Skills:"` (section header), which is unchanged. But verify no other assertion references the old footer. + +Then run: `cargo test -p nca-core layers_sections 2>&1 | tail -10` +Expected: PASS + +- [ ] **Step 3: Verify full build** + +Run: `cargo check --workspace 2>&1 | tail -5` +Expected: no errors + +- [ ] **Step 4: Commit** + +```bash +git add crates/core/src/harness.rs +git commit -m "feat: update skills manifest footer to reference invoke_skill tool (#34)" +``` + +--- + +### Task 4: Add tests for invoke_skill tool + +**Files:** +- Modify: `crates/core/src/tools/invoke_skill.rs` (add test module) + +- [ ] **Step 1: Add test module to `invoke_skill.rs`** + +Append to the end of `crates/core/src/tools/invoke_skill.rs`: + +```rust +#[cfg(test)] +mod tests { + use super::*; + + fn make_tool(workspace: &std::path::Path) -> InvokeSkillTool { + InvokeSkillTool::new( + workspace.to_path_buf(), + vec![std::path::PathBuf::from(".nca/skills")], + ) + } + + fn make_call(skill_name: &str) -> ToolCall { + ToolCall { + id: "call-1".into(), + name: "invoke_skill".into(), + input: serde_json::json!({ "skill_name": skill_name }), + } + } + + #[test] + fn definition_has_correct_name_and_parameters() { + let dir = tempfile::tempdir().unwrap(); + let tool = make_tool(dir.path()); + let def = tool.definition(); + assert_eq!(def.name, "invoke_skill"); + assert!(def.description.contains("Load a skill")); + assert!(def.parameters["properties"]["skill_name"].is_object()); + assert!(def.parameters["required"] + .as_array() + .unwrap() + .contains(&serde_json::json!("skill_name"))); + } + + #[tokio::test] + async fn returns_expanded_body_for_valid_skill() { + let dir = tempfile::tempdir().unwrap(); + let skill_dir = dir.path().join(".nca/skills/my-skill"); + std::fs::create_dir_all(&skill_dir).unwrap(); + std::fs::write( + skill_dir.join("SKILL.md"), + "---\nname: My Skill\ncommand: my-skill\ndescription: A test skill\n---\nDo the thing.\n\nSee ./helper.md for details.\n", + ) + .unwrap(); + std::fs::write(skill_dir.join("helper.md"), "Helper content.").unwrap(); + + let tool = make_tool(dir.path()); + let result = tool.execute(&make_call("my-skill")).await; + + assert!(result.success); + assert!(result.output.contains("Skill `my-skill` loaded")); + assert!(result.output.contains("Do the thing.")); + assert!(result.output.contains("===== helper.md =====")); + assert!(result.output.contains("Helper content.")); + } + + #[tokio::test] + async fn returns_error_for_unknown_skill() { + let dir = tempfile::tempdir().unwrap(); + let skill_dir = dir.path().join(".nca/skills/real-skill"); + std::fs::create_dir_all(&skill_dir).unwrap(); + std::fs::write( + skill_dir.join("SKILL.md"), + "---\nname: Real\ncommand: real-skill\n---\nReal body.\n", + ) + .unwrap(); + + let tool = make_tool(dir.path()); + let result = tool.execute(&make_call("nonexistent")).await; + + assert!(!result.success); + let err = result.error.unwrap(); + assert!(err.contains("not found")); + assert!(err.contains("real-skill")); + } + + #[tokio::test] + async fn returns_error_for_empty_skill_name() { + let dir = tempfile::tempdir().unwrap(); + let tool = make_tool(dir.path()); + let result = tool.execute(&make_call("")).await; + + assert!(!result.success); + assert!(result.error.unwrap().contains("skill_name is required")); + } +} +``` + +- [ ] **Step 2: Run tests** + +Run: `cargo test -p nca-core invoke_skill 2>&1 | tail -20` +Expected: all 4 tests pass + +- [ ] **Step 3: Commit** + +```bash +git add crates/core/src/tools/invoke_skill.rs +git commit -m "test: add invoke_skill tool tests (#34)" +``` + +--- + +### Task 5: Full build and test verification + +- [ ] **Step 1: Run all workspace tests** + +Run: `cargo test --workspace 2>&1 | tail -30` +Expected: all tests pass + +- [ ] **Step 2: Run clippy** + +Run: `cargo clippy --workspace -- -D warnings 2>&1 | tail -10` +Expected: no warnings + +- [ ] **Step 3: Commit if any cleanup needed** + +```bash +git add -A && git commit -m "chore: invoke_skill tool - final verification (#34)" +``` diff --git a/docs/superpowers/plans/2026-03-24-question-modal-picker.md b/docs/superpowers/plans/2026-03-24-question-modal-picker.md new file mode 100644 index 0000000..a85d7e6 --- /dev/null +++ b/docs/superpowers/plans/2026-03-24-question-modal-picker.md @@ -0,0 +1,532 @@ +# Question Modal Picker Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Replace the number-key-based Ask Question option picker with a modal popup using arrow key navigation. + +**Architecture:** Add modal state fields and open/close helpers to `TuiSessionState`, render the modal as a centered popup (same pattern as permission/agent pickers), intercept key events when modal is open, and trigger the modal automatically when a `QuestionRequested` event arrives. + +**Tech Stack:** Rust, ratatui, crossterm + +**Spec:** `docs/superpowers/specs/2026-03-24-question-modal-picker-design.md` + +--- + +### Task 1: Add modal state fields and open/close helpers + +**Files:** +- Modify: `crates/cli/src/tui/state.rs:61-171` (add fields to `TuiSessionState`) +- Modify: `crates/cli/src/tui/state.rs:187-266` (add defaults in `new()`) +- Modify: `crates/cli/src/tui/state.rs:268-383` (add helper methods) + +- [ ] **Step 1: Add state fields to `TuiSessionState` struct** + +Add after `agent_picker_index` (line 157): + +```rust +/// Question modal popup (arrow-key option picker). +pub question_modal_open: bool, +pub question_modal_index: usize, +pub question_modal_scroll: usize, +``` + +- [ ] **Step 2: Add defaults in `new()` constructor** + +Add after `agent_picker_index: 0,` (line 256): + +```rust +question_modal_open: false, +question_modal_index: 0, +question_modal_scroll: 0, +``` + +- [ ] **Step 3: Add `open_question_modal` / `close_question_modal` helpers** + +Add after `close_agent_picker` (line 353): + +```rust +pub fn open_question_modal(&mut self) { + self.question_modal_open = true; + self.question_modal_index = 0; + self.question_modal_scroll = 0; +} + +pub fn close_question_modal(&mut self) { + self.question_modal_open = false; + self.question_modal_index = 0; + self.question_modal_scroll = 0; +} +``` + +- [ ] **Step 4: Verify it compiles** + +Run: `cargo check -p nca-cli 2>&1 | tail -5` +Expected: no errors (new fields are unused warnings are fine) + +- [ ] **Step 5: Commit** + +```bash +git add crates/cli/src/tui/state.rs +git commit -m "feat(tui): add question modal state fields and helpers (#32)" +``` + +--- + +### Task 2: Auto-open modal on QuestionRequested event + +**Files:** +- Modify: `crates/cli/src/tui/state.rs:586-591` (`apply_event` for `QuestionRequested`) + +- [ ] **Step 1: Modify `QuestionRequested` handler to open the modal** + +Change the existing handler at line 586-591 from: + +```rust +AgentEvent::QuestionRequested { question } => { + self.active_question = Some(question.clone()); + self.blocks.push(DisplayBlock::Question(question.clone())); + self.transcript_follow_tail = true; +} +``` + +To: + +```rust +AgentEvent::QuestionRequested { question } => { + self.active_question = Some(question.clone()); + self.blocks.push(DisplayBlock::Question(question.clone())); + self.transcript_follow_tail = true; + self.open_question_modal(); +} +``` + +- [ ] **Step 2b: Add defensive `close_question_modal()` to `QuestionResolved` handler** + +In the `QuestionResolved` handler (line 592-600), add `self.close_question_modal();` after `self.active_question = None;`: + +```rust +AgentEvent::QuestionResolved { + question_id, + selection, +} => { + self.active_question = None; + self.close_question_modal(); + self.blocks.push(DisplayBlock::System(format!( + "Answered question {question_id}: {selection:?}" + ))); +} +``` + +- [ ] **Step 2: Verify it compiles** + +Run: `cargo check -p nca-cli 2>&1 | tail -5` +Expected: no errors + +- [ ] **Step 3: Commit** + +```bash +git add crates/cli/src/tui/state.rs +git commit -m "feat(tui): auto-open question modal on QuestionRequested (#32)" +``` + +--- + +### Task 3: Add key event handling for the question modal + +**Files:** +- Modify: `crates/cli/src/tui/app.rs:3062-3111` (add question modal handler before permission picker handler) + +- [ ] **Step 1: Add question modal keyboard handler** + +Insert before the permission picker handler (before line 3063 `// Permission picker keyboard handling.`): + +```rust +// Question modal keyboard handling. +if g.question_modal_open { + if let Some(ref q) = g.active_question.clone() { + // Total items: 1 (suggested) + options.len() + (1 if allow_custom for "Chat about this") + let total = 1 + q.options.len() + if q.allow_custom { 1 } else { 0 }; + match (key.code, key.modifiers) { + (KeyCode::Esc, _) => { + if q.allow_custom { + // Fall back to inline text input + g.close_question_modal(); + } + // If !allow_custom, Esc is a no-op + } + (KeyCode::Up, _) | (KeyCode::Char('k'), KeyModifiers::NONE) => { + g.question_modal_index = g.question_modal_index.saturating_sub(1); + } + (KeyCode::Down, _) | (KeyCode::Char('j'), KeyModifiers::NONE) => { + g.question_modal_index = (g.question_modal_index + 1).min(total - 1); + } + (KeyCode::Enter, _) => { + let idx = g.question_modal_index; + let sel = if idx == 0 { + // Suggested answer + Some(QuestionSelection::Suggested) + } else if idx <= q.options.len() { + // Regular option (1-based → 0-based) + Some(QuestionSelection::Option { + option_id: q.options[idx - 1].id.clone(), + }) + } else { + // "Chat about this" — fall back to inline text input + None + }; + + if let Some(sel) = sel { + let qid = q.question_id.clone(); + g.close_question_modal(); + g.active_question = None; + drop(g); + if let Some(ref tx) = question_answer_tx { + let _ = tx.send((qid, sel)); + } else { + let _ = cmd_tx.send(TuiCmd::QuestionAnswer(sel)); + } + } else { + // "Chat about this" — close modal, keep active_question + g.close_question_modal(); + } + } + _ => {} + } + } + continue; +} +``` + +- [ ] **Step 2: Add mouse event swallowing for question modal** + +In the mouse event swallowing section (around line 2590-2599), add after `Event::Mouse(_) if g.session_picker_open => continue,`: + +```rust +Event::Mouse(_) if g.question_modal_open => continue, +``` + +- [ ] **Step 3: Verify it compiles** + +Run: `cargo check -p nca-cli 2>&1 | tail -5` +Expected: no errors + +- [ ] **Step 4: Commit** + +```bash +git add crates/cli/src/tui/app.rs +git commit -m "feat(tui): add question modal key event handling (#32)" +``` + +--- + +### Task 4: Render the question modal popup + +**Files:** +- Modify: `crates/cli/src/tui/app.rs:2085-2086` (add rendering after permission picker rendering block) + +- [ ] **Step 1: Add question modal rendering** + +Insert after the agent picker rendering block (after the closing `}` of the `if g.agent_picker_open` block, around line 2140): + +```rust +// Question modal popup (arrow-key option picker). +if g.question_modal_open { + if let Some(ref q) = g.active_question { + let has_chat_option = q.allow_custom; + let total_items = 1 + q.options.len() + if has_chat_option { 1 } else { 0 }; + // +4 for: title line, blank, blank before footer, footer + let rows = (total_items as u16).saturating_add(6).max(8); + let popup_w = 60u16.min(area.width.saturating_sub(4)); + let popup_area = centered_rect(area, popup_w, rows); + + let mut lines: Vec = vec![ + Line::from(Span::styled( + format!(" {} ", q.prompt), + Style::default() + .fg(theme::ASSISTANT) + .add_modifier(Modifier::BOLD), + )), + Line::default(), + ]; + + // Suggested answer (index 0) + let suggested_label = format!(" Suggested: {} ", q.suggested_answer); + if g.question_modal_index == 0 { + lines.push(Line::from(Span::styled( + format!(" ► {}", suggested_label.trim()), + Style::default() + .fg(Color::Black) + .bg(theme::USER) + .add_modifier(Modifier::BOLD), + ))); + } else { + lines.push(Line::from(Span::styled( + format!(" {}", suggested_label.trim()), + Style::default().fg(theme::TEXT), + ))); + } + + // Options (index 1..n) + for (i, o) in q.options.iter().enumerate() { + let item_idx = i + 1; + let label = format!("{} ", o.label); + if g.question_modal_index == item_idx { + lines.push(Line::from(Span::styled( + format!(" ► {}", label.trim()), + Style::default() + .fg(Color::Black) + .bg(theme::USER) + .add_modifier(Modifier::BOLD), + ))); + } else { + lines.push(Line::from(Span::styled( + format!(" {}", label.trim()), + Style::default().fg(theme::TEXT), + ))); + } + } + + // "Chat about this" (last item, only if allow_custom) + if has_chat_option { + let chat_idx = 1 + q.options.len(); + if g.question_modal_index == chat_idx { + lines.push(Line::from(Span::styled( + " ► Chat about this", + Style::default() + .fg(Color::Black) + .bg(theme::USER) + .add_modifier(Modifier::BOLD), + ))); + } else { + lines.push(Line::from(Span::styled( + " Chat about this", + Style::default() + .fg(theme::MUTED) + .add_modifier(Modifier::ITALIC), + ))); + } + } + + // Footer + lines.push(Line::default()); + let footer_text = if has_chat_option { + " ↑↓ select · Enter confirm · Esc chat " + } else { + " ↑↓ select · Enter confirm " + }; + lines.push(Line::from(Span::styled( + footer_text, + Style::default().fg(theme::MUTED), + ))); + + frame.render_widget(ClearWidget, popup_area); + let popup = Paragraph::new(Text::from(lines)) + .block( + Block::default() + .borders(Borders::ALL) + .border_style(Style::default().fg(theme::BORDER)) + .title(Span::styled( + " question ", + Style::default().fg(theme::WARN), + )), + ) + .style(Style::default().bg(theme::SURFACE)) + .wrap(Wrap { trim: false }); + frame.render_widget(popup, popup_area); + } +} +``` + +- [ ] **Step 2: Verify it compiles** + +Run: `cargo check -p nca-cli 2>&1 | tail -5` +Expected: no errors + +- [ ] **Step 3: Commit** + +```bash +git add crates/cli/src/tui/app.rs +git commit -m "feat(tui): render question modal popup (#32)" +``` + +--- + +### Task 5: Suppress composer hint when modal is open + +**Files:** +- Modify: `crates/cli/src/tui/app.rs:1801-1804` (composer hint line) + +- [ ] **Step 1: Guard the question hint with `!question_modal_open`** + +Change the hint at line 1801-1804 from: + +```rust +} else if g.active_question.is_some() { + Line::from(Span::styled( + "Enter / 0 = suggested · 1–n = option · click underlined line · /auto-answer · End = transcript bottom (empty input)", + Style::default().fg(theme::WARN), + )) +``` + +To: + +```rust +} else if g.active_question.is_some() && !g.question_modal_open { + Line::from(Span::styled( + "Enter / 0 = suggested · 1–n = option · click underlined line · /auto-answer · End = transcript bottom (empty input)", + Style::default().fg(theme::WARN), + )) +``` + +- [ ] **Step 2: Verify it compiles** + +Run: `cargo check -p nca-cli 2>&1 | tail -5` +Expected: no errors + +- [ ] **Step 3: Commit** + +```bash +git add crates/cli/src/tui/app.rs +git commit -m "feat(tui): suppress composer hint when question modal is open (#32)" +``` + +--- + +### Task 6: Update existing tests and add new tests + +**Files:** +- Modify: `crates/cli/src/tui/state.rs:749-878` (test module) + +- [ ] **Step 1: Add test for `open_question_modal` / `close_question_modal` helpers** + +Add to the test module: + +```rust +#[test] +fn open_close_question_modal() { + let mut st = TuiSessionState::new( + "s".into(), + "m".into(), + "@build".into(), + "default".into(), + PathBuf::from("/tmp"), + ); + assert!(!st.question_modal_open); + assert_eq!(st.question_modal_index, 0); + + st.open_question_modal(); + assert!(st.question_modal_open); + assert_eq!(st.question_modal_index, 0); + assert_eq!(st.question_modal_scroll, 0); + + st.question_modal_index = 3; + st.close_question_modal(); + assert!(!st.question_modal_open); + assert_eq!(st.question_modal_index, 0); + assert_eq!(st.question_modal_scroll, 0); +} +``` + +- [ ] **Step 2: Add test verifying `QuestionRequested` event opens the modal** + +```rust +#[test] +fn question_requested_opens_modal() { + let mut st = TuiSessionState::new( + "s".into(), + "m".into(), + "@build".into(), + "default".into(), + PathBuf::from("/tmp"), + ); + let q = InteractiveQuestionPayload { + question_id: "q-1".into(), + call_id: "c1".into(), + prompt: "Pick".into(), + options: vec![QuestionOption { + id: "a".into(), + label: "A".into(), + }], + allow_custom: true, + suggested_answer: "A".into(), + }; + st.apply_event(&AgentEvent::QuestionRequested { + question: q.clone(), + }); + assert!(st.question_modal_open); + assert_eq!(st.question_modal_index, 0); + assert!(st.active_question.is_some()); +} +``` + +- [ ] **Step 3: Add test for `QuestionResolved` clears modal state defensively** + +```rust +#[test] +fn question_resolved_closes_modal() { + let mut st = TuiSessionState::new( + "s".into(), + "m".into(), + "@build".into(), + "default".into(), + PathBuf::from("/tmp"), + ); + st.question_modal_open = true; + st.question_modal_index = 2; + st.apply_event(&AgentEvent::QuestionResolved { + question_id: "q-1".into(), + selection: QuestionSelection::Suggested, + }); + assert!(st.active_question.is_none()); + assert!(!st.question_modal_open); + assert_eq!(st.question_modal_index, 0); +} +``` + +- [ ] **Step 4: Run all tests** + +Run: `cargo test -p nca-cli 2>&1 | tail -20` +Expected: all tests pass + +- [ ] **Step 5: Commit** + +```bash +git add crates/cli/src/tui/state.rs +git commit -m "test(tui): add question modal state tests (#32)" +``` + +--- + +### Task 7: Manual verification and final cleanup + +- [ ] **Step 1: Build the full project** + +Run: `cargo build 2>&1 | tail -10` +Expected: builds successfully + +- [ ] **Step 2: Run all tests across workspace** + +Run: `cargo test --workspace 2>&1 | tail -20` +Expected: all tests pass + +- [ ] **Step 3: Manual verification checklist** + +Run the application and trigger a question (use the ask_question tool). Verify: +- Modal appears centered with the question prompt as title +- Suggested answer is highlighted at index 0 +- Arrow up/down moves the highlight correctly +- Enter on suggested → sends `QuestionSelection::Suggested` +- Enter on an option → sends `QuestionSelection::Option` +- Enter on "Chat about this" → closes modal, inline text input appears +- Esc with `allow_custom=true` → closes modal, inline text input appears +- Esc with `allow_custom=false` → no-op, modal stays open +- Mouse clicks are swallowed while modal is open +- Composer hint is hidden while modal is open +- `/auto-answer` still works (bypasses modal via existing path) + +- [ ] **Step 4: Commit any cleanup** + +```bash +git add -A +git commit -m "feat(tui): question modal picker - final cleanup (#32)" +``` diff --git a/docs/superpowers/plans/2026-03-24-skill-supporting-files.md b/docs/superpowers/plans/2026-03-24-skill-supporting-files.md new file mode 100644 index 0000000..52fcf35 --- /dev/null +++ b/docs/superpowers/plans/2026-03-24-skill-supporting-files.md @@ -0,0 +1,603 @@ +# Skill Supporting Files Inlining Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** When a skill is invoked, automatically inline referenced supporting files from the skill's directory into the prompt. + +**Architecture:** Add a `extract_file_references()` function to detect file paths in the SKILL.md body using pattern matching (./paths, @paths, backtick-wrapped paths), add a three-level path resolution strategy, and add an `expanded_body()` method on `Skill` that inlines resolved files. Update `prompt_for_task()` to use the expanded body. + +**Tech Stack:** Rust, regex crate + +**Spec:** `docs/superpowers/specs/2026-03-24-skill-supporting-files-design.md` + +--- + +### Task 1: Add `regex` dependency to `nca-core` + +**Files:** +- Modify: `crates/core/Cargo.toml` + +- [ ] **Step 1: Add regex dependency** + +Add to `[dependencies]` section in `crates/core/Cargo.toml`: + +```toml +regex = "1" +``` + +- [ ] **Step 2: Verify it compiles** + +Run: `cargo check -p nca-core 2>&1 | tail -5` +Expected: no errors + +- [ ] **Step 3: Commit** + +```bash +git add crates/core/Cargo.toml Cargo.lock +git commit -m "chore: add regex dependency to nca-core (#34)" +``` + +--- + +### Task 2: Implement `extract_file_references()` with tests (TDD) + +**Files:** +- Modify: `crates/core/src/skills.rs:397-471` (add to test module and add function before tests) + +- [ ] **Step 1: Write failing tests for reference extraction** + +Add these tests to the `#[cfg(test)] mod tests` block in `crates/core/src/skills.rs`: + +```rust +#[test] +fn extracts_dot_slash_references() { + let body = "Read ./implementer-prompt.md for details.\nAlso see ./subdir/helper.sh here."; + let refs = extract_file_references(body); + assert_eq!(refs, vec!["./implementer-prompt.md", "./subdir/helper.sh"]); +} + +#[test] +fn extracts_at_prefixed_references() { + let body = "Use @testing-anti-patterns.md to avoid pitfalls.\nSee @graphviz-conventions.dot too."; + let refs = extract_file_references(body); + assert_eq!(refs, vec!["testing-anti-patterns.md", "graphviz-conventions.dot"]); +} + +#[test] +fn extracts_backtick_paths_with_directory() { + let body = "Check `skills/brainstorming/visual-companion.md` for guidance.\nAlso `subdir/file.ts`."; + let refs = extract_file_references(body); + assert_eq!(refs, vec!["skills/brainstorming/visual-companion.md", "subdir/file.ts"]); +} + +#[test] +fn extracts_backtick_bare_filenames() { + let body = "See `code-reviewer.md` for the template.\nUse `helper.sh` too."; + let refs = extract_file_references(body); + assert_eq!(refs, vec!["code-reviewer.md", "helper.sh"]); +} + +#[test] +fn excludes_well_known_files_in_backticks() { + let body = "Check `CLAUDE.md` and `AGENTS.md` and `GEMINI.md` and `SKILL.md` and `README.md` and `package.json`."; + let refs = extract_file_references(body); + assert!(refs.is_empty()); +} + +#[test] +fn excludes_urls_and_template_paths() { + let body = "See https://example.com/file.md and path/to/test.md for examples."; + let refs = extract_file_references(body); + assert!(refs.is_empty()); +} + +#[test] +fn deduplicates_preserving_order() { + let body = "Use ./helper.md first.\nThen `helper.md` again.\nAnd @helper.md once more."; + let refs = extract_file_references(body); + assert_eq!(refs, vec!["./helper.md"]); +} +``` + +- [ ] **Step 2: Run tests to verify they fail** + +Run: `cargo test -p nca-core extract_file_references 2>&1 | tail -10` +Expected: FAIL — function not found + +- [ ] **Step 3: Implement `extract_file_references()`** + +Add this function before the `#[cfg(test)]` block in `crates/core/src/skills.rs` (after `parse_permission_mode_str`, around line 395): + +```rust +/// Extract file references from a skill body. +/// +/// Detects: `./path`, `@path`, backtick-wrapped paths with supported extensions. +/// Returns deduplicated list in first-occurrence order, with `@` stripped and `./` preserved. +fn extract_file_references(body: &str) -> Vec { + use regex::Regex; + use std::collections::HashSet; + use std::sync::LazyLock; + + static EXCLUDED_NAMES: &[&str] = &[ + "CLAUDE.md", "AGENTS.md", "GEMINI.md", "SKILL.md", "README.md", "package.json", + ]; + + static DOT_SLASH_RE: LazyLock = LazyLock::new(|| { + Regex::new(r"\./[a-zA-Z0-9_][a-zA-Z0-9_./-]*\.(md|sh|ts|js|dot|txt|html|cjs)").unwrap() + }); + + // Require @ at start of line or after whitespace to avoid matching emails + static AT_RE: LazyLock = LazyLock::new(|| { + Regex::new(r"(?:^|\s)@([a-zA-Z0-9_][a-zA-Z0-9_./-]*\.(md|sh|ts|js|dot|txt|html|cjs))") + .unwrap() + }); + + static BACKTICK_RE: LazyLock = LazyLock::new(|| { + Regex::new(r"`([a-zA-Z0-9_][a-zA-Z0-9_./-]*\.(md|sh|ts|js|dot|txt|html|cjs))`").unwrap() + }); + + fn is_excluded(path: &str) -> bool { + EXCLUDED_NAMES.contains(&path) + || path.contains("path/to/") + || path.starts_with("http://") + || path.starts_with("https://") + } + + let mut seen = HashSet::new(); + let mut refs = Vec::new(); + + // Pattern 1: ./path references + for m in DOT_SLASH_RE.find_iter(body) { + let path = m.as_str().to_string(); + let key = path.trim_start_matches("./").to_string(); + if !is_excluded(&key) && !seen.contains(&key) { + seen.insert(key); + refs.push(path); + } + } + + // Pattern 2: @path references (strip @, require word boundary) + for cap in AT_RE.captures_iter(body) { + let path = cap[1].to_string(); + let key = path.clone(); + if !is_excluded(&key) && !seen.contains(&key) { + seen.insert(key); + refs.push(path); + } + } + + // Pattern 3+4: backtick-wrapped paths + for cap in BACKTICK_RE.captures_iter(body) { + let path = cap[1].to_string(); + let key = path.trim_start_matches("./").to_string(); + if !is_excluded(&key) && !EXCLUDED_NAMES.contains(&path.as_str()) && !seen.contains(&key) { + seen.insert(key); + refs.push(path); + } + } + + refs +} +``` + +Keep the `use` statements as local imports inside the function — no top-level import needed. + +- [ ] **Step 5: Run tests to verify they pass** + +Run: `cargo test -p nca-core extract_file_references 2>&1 | tail -20` +Expected: all 7 tests pass + +- [ ] **Step 6: Commit** + +```bash +git add crates/core/src/skills.rs +git commit -m "feat: add extract_file_references for skill supporting files (#34)" +``` + +--- + +### Task 3: Implement `resolve_skill_reference()` with tests (TDD) + +**Files:** +- Modify: `crates/core/src/skills.rs` + +- [ ] **Step 1: Write failing tests for path resolution** + +Add to the test module: + +```rust +#[test] +fn resolves_reference_in_skill_directory() { + let dir = tempfile::tempdir().unwrap(); + let skill_dir = dir.path().join("my-skill"); + std::fs::create_dir_all(&skill_dir).unwrap(); + std::fs::write(skill_dir.join("helper.md"), "helper content").unwrap(); + + let result = resolve_skill_reference("./helper.md", &skill_dir); + assert!(result.is_some()); + assert_eq!(result.unwrap(), skill_dir.join("helper.md")); +} + +#[test] +fn resolves_reference_via_catalog_root_fallback() { + let dir = tempfile::tempdir().unwrap(); + let catalog = dir.path().join("skills"); + let skill_dir = catalog.join("sdd"); + let other_skill = catalog.join("brainstorming"); + std::fs::create_dir_all(&skill_dir).unwrap(); + std::fs::create_dir_all(&other_skill).unwrap(); + std::fs::write(other_skill.join("visual.md"), "visual content").unwrap(); + + // Level 2: resolve against catalog root (parent of skill_dir) + let result = resolve_skill_reference("brainstorming/visual.md", &skill_dir); + assert!(result.is_some()); + assert_eq!(result.unwrap(), other_skill.join("visual.md")); +} + +#[test] +fn resolves_skills_prefix_by_stripping() { + let dir = tempfile::tempdir().unwrap(); + let catalog = dir.path().join("skills"); + let brainstorming = catalog.join("brainstorming"); + std::fs::create_dir_all(&brainstorming).unwrap(); + std::fs::write(brainstorming.join("visual.md"), "content").unwrap(); + + let skill_dir = catalog.join("sdd"); + std::fs::create_dir_all(&skill_dir).unwrap(); + + // Level 3: strip skills/ prefix, resolve against catalog root + let result = resolve_skill_reference("skills/brainstorming/visual.md", &skill_dir); + assert!(result.is_some()); + assert_eq!(result.unwrap(), brainstorming.join("visual.md")); +} + +#[test] +fn returns_none_for_missing_file() { + let dir = tempfile::tempdir().unwrap(); + let skill_dir = dir.path().join("my-skill"); + std::fs::create_dir_all(&skill_dir).unwrap(); + + let result = resolve_skill_reference("./nonexistent.md", &skill_dir); + assert!(result.is_none()); +} +``` + +- [ ] **Step 2: Run tests to verify they fail** + +Run: `cargo test -p nca-core resolve_skill_reference 2>&1 | tail -10` +Expected: FAIL — function not found + +- [ ] **Step 3: Implement `resolve_skill_reference()`** + +Add before `extract_file_references` in `crates/core/src/skills.rs`: + +```rust +/// Resolve a file reference to an absolute path using three-level strategy: +/// 1. Relative to skill directory +/// 2. Relative to catalog root (parent of skill directory) +/// 3. Strip `skills/` prefix and retry against catalog root +/// +/// Returns `None` if the file doesn't exist at any level. +fn resolve_skill_reference(reference: &str, skill_directory: &Path) -> Option { + let clean = reference.trim_start_matches("./"); + + // Level 1: relative to skill directory + let level1 = skill_directory.join(clean); + if level1.is_file() { + return Some(level1); + } + + // Level 2: relative to catalog root (parent of skill directory) + if let Some(catalog_root) = skill_directory.parent() { + let level2 = catalog_root.join(clean); + if level2.is_file() { + return Some(level2); + } + + // Level 3: strip "skills/" prefix and retry against catalog root + if let Some(stripped) = clean.strip_prefix("skills/") { + let level3 = catalog_root.join(stripped); + if level3.is_file() { + return Some(level3); + } + } + } + + None +} +``` + +- [ ] **Step 4: Run tests to verify they pass** + +Run: `cargo test -p nca-core resolve_skill_reference 2>&1 | tail -20` +Expected: all 4 tests pass + +- [ ] **Step 5: Commit** + +```bash +git add crates/core/src/skills.rs +git commit -m "feat: add resolve_skill_reference with three-level strategy (#34)" +``` + +--- + +### Task 4: Implement `expanded_body()` with tests (TDD) + +**Files:** +- Modify: `crates/core/src/skills.rs:95-145` (add method to `impl Skill`) + +- [ ] **Step 1: Write failing test for expanded_body** + +Add to the test module: + +```rust +#[test] +fn expanded_body_inlines_referenced_files() { + let dir = tempfile::tempdir().unwrap(); + let skill_dir = dir.path().join("my-skill"); + std::fs::create_dir_all(&skill_dir).unwrap(); + std::fs::write(skill_dir.join("helper.md"), "Helper content here.").unwrap(); + + let skill = Skill { + name: "test".into(), + description: None, + command: "test".into(), + model: None, + permission_mode: None, + context: SkillContextMode::Inline, + directory: skill_dir, + body: "Main body.\n\nSee ./helper.md for details.".into(), + source: SkillSource::FileSystem, + }; + + let expanded = skill.expanded_body(); + assert!(expanded.contains("Main body.")); + assert!(expanded.contains("===== helper.md =====")); + assert!(expanded.contains("Helper content here.")); +} + +#[test] +fn expanded_body_skips_missing_files() { + let dir = tempfile::tempdir().unwrap(); + let skill_dir = dir.path().join("my-skill"); + std::fs::create_dir_all(&skill_dir).unwrap(); + + let skill = Skill { + name: "test".into(), + description: None, + command: "test".into(), + model: None, + permission_mode: None, + context: SkillContextMode::Inline, + directory: skill_dir, + body: "Main body.\n\nSee ./nonexistent.md for details.".into(), + source: SkillSource::FileSystem, + }; + + let expanded = skill.expanded_body(); + assert_eq!(expanded.trim(), "Main body.\n\nSee ./nonexistent.md for details."); +} + +#[test] +fn expanded_body_inlines_at_prefixed_reference() { + let dir = tempfile::tempdir().unwrap(); + let skill_dir = dir.path().join("tdd"); + std::fs::create_dir_all(&skill_dir).unwrap(); + std::fs::write(skill_dir.join("testing-anti-patterns.md"), "Anti-pattern content.").unwrap(); + + let skill = Skill { + name: "test".into(), + description: None, + command: "test".into(), + model: None, + permission_mode: None, + context: SkillContextMode::Inline, + directory: skill_dir, + body: "Main body.\n\nRead @testing-anti-patterns.md to avoid pitfalls.".into(), + source: SkillSource::FileSystem, + }; + + let expanded = skill.expanded_body(); + assert!(expanded.contains("===== testing-anti-patterns.md =====")); + assert!(expanded.contains("Anti-pattern content.")); +} + +#[test] +fn expanded_body_bare_filename_resolves_only_in_skill_dir() { + let dir = tempfile::tempdir().unwrap(); + let catalog = dir.path().join("skills"); + let skill_dir = catalog.join("review"); + let other_dir = catalog.join("other"); + std::fs::create_dir_all(&skill_dir).unwrap(); + std::fs::create_dir_all(&other_dir).unwrap(); + // File exists in sibling skill dir but NOT in the skill's own dir + std::fs::write(other_dir.join("template.md"), "Other content.").unwrap(); + + let skill = Skill { + name: "test".into(), + description: None, + command: "test".into(), + model: None, + permission_mode: None, + context: SkillContextMode::Inline, + directory: skill_dir, + body: "See `template.md` for the template.".into(), + source: SkillSource::FileSystem, + }; + + let expanded = skill.expanded_body(); + // Should NOT inline — bare filename only resolves against skill dir (Level 1) + assert!(!expanded.contains("=====")); + assert!(!expanded.contains("Other content.")); +} + +#[test] +fn expanded_body_deduplicates() { + let dir = tempfile::tempdir().unwrap(); + let skill_dir = dir.path().join("my-skill"); + std::fs::create_dir_all(&skill_dir).unwrap(); + std::fs::write(skill_dir.join("helper.md"), "Helper.").unwrap(); + + let skill = Skill { + name: "test".into(), + description: None, + command: "test".into(), + model: None, + permission_mode: None, + context: SkillContextMode::Inline, + directory: skill_dir, + body: "See ./helper.md and `helper.md` again.".into(), + source: SkillSource::FileSystem, + }; + + let expanded = skill.expanded_body(); + let count = expanded.matches("===== helper.md =====").count(); + assert_eq!(count, 1); +} +``` + +- [ ] **Step 2: Run tests to verify they fail** + +Run: `cargo test -p nca-core expanded_body 2>&1 | tail -10` +Expected: FAIL — method not found + +- [ ] **Step 3: Implement `expanded_body()`** + +Add to the `impl Skill` block (after `source_label()`, around line 144): + +```rust +/// Return the skill body with referenced supporting files inlined. +/// +/// Scans the body for file references (./path, @path, backtick-wrapped paths), +/// resolves them against the skill's directory, and appends their contents. +/// Bare filenames (no `/` or `./` prefix) resolve only against skill directory (Level 1). +pub fn expanded_body(&self) -> String { + let refs = extract_file_references(&self.body); + if refs.is_empty() { + return self.body.clone(); + } + + let mut expanded = self.body.clone(); + for ref_path in &refs { + let clean = ref_path.trim_start_matches("./"); + let is_bare_filename = !clean.contains('/'); + + let resolved = if is_bare_filename { + // Pattern 4: bare filenames resolve only against skill directory (Level 1) + let candidate = self.directory.join(clean); + if candidate.is_file() { Some(candidate) } else { None } + } else { + resolve_skill_reference(ref_path, &self.directory) + }; + + if let Some(resolved) = resolved { + if let Ok(content) = std::fs::read_to_string(&resolved) { + expanded.push_str(&format!("\n\n===== {} =====\n\n{}", clean, content.trim())); + } + } + } + + expanded +} +``` + +- [ ] **Step 4: Run tests to verify they pass** + +Run: `cargo test -p nca-core expanded_body 2>&1 | tail -20` +Expected: all 4 tests pass + +- [ ] **Step 5: Commit** + +```bash +git add crates/core/src/skills.rs +git commit -m "feat: add expanded_body() for skill supporting file inlining (#34)" +``` + +--- + +### Task 5: Update `prompt_for_task()` to use expanded body + +**Files:** +- Modify: `crates/core/src/skills.rs:107-117` + +- [ ] **Step 1: Update `prompt_for_task()`** + +Change line 111 in `prompt_for_task()` from: + +```rust + self.body.trim() +``` + +To: + +```rust + self.expanded_body().trim() +``` + +The full method becomes: + +```rust +pub fn prompt_for_task(&self, task: &str) -> String { + let mut prompt = format!( + "Use the skill `{}`.\n\nSkill instructions:\n{}\n", + self.command, + self.expanded_body().trim() + ); + if !task.trim().is_empty() { + prompt.push_str(&format!("\nTask:\n{}\n", task.trim())); + } + prompt +} +``` + +- [ ] **Step 2: Verify it compiles** + +Run: `cargo check -p nca-core 2>&1 | tail -5` +Expected: no errors + +- [ ] **Step 3: Commit** + +```bash +git add crates/core/src/skills.rs +git commit -m "feat: use expanded_body in prompt_for_task for supporting files (#34)" +``` + +--- + +### Task 6: Full build and test verification + +- [ ] **Step 1: Run all workspace tests** + +Run: `cargo test --workspace 2>&1 | tail -30` +Expected: all tests pass + +- [ ] **Step 2: Run clippy** + +Run: `cargo clippy --workspace -- -D warnings 2>&1 | tail -10` +Expected: no warnings + +- [ ] **Step 3: Verify with real superpower skills** + +Copy a superpower skill to `.nca/skills/` and verify expansion works: + +```bash +mkdir -p .nca/skills/test-skill +cp /home/nst/Documents/superpower/skills/subagent-driven-development/SKILL.md .nca/skills/test-skill/ +cp /home/nst/Documents/superpower/skills/subagent-driven-development/implementer-prompt.md .nca/skills/test-skill/ +cp /home/nst/Documents/superpower/skills/subagent-driven-development/spec-reviewer-prompt.md .nca/skills/test-skill/ +cp /home/nst/Documents/superpower/skills/subagent-driven-development/code-quality-reviewer-prompt.md .nca/skills/test-skill/ +``` + +Then invoke the skill and verify the prompt includes the supporting file contents. + +- [ ] **Step 4: Clean up test files** + +```bash +rm -rf .nca/skills/test-skill +``` + +- [ ] **Step 5: Commit if any cleanup needed** + +```bash +git add -A && git commit -m "chore: skill supporting files - final verification (#34)" +``` diff --git a/docs/superpowers/specs/2026-03-24-invoke-skill-tool-design.md b/docs/superpowers/specs/2026-03-24-invoke-skill-tool-design.md new file mode 100644 index 0000000..7790c94 --- /dev/null +++ b/docs/superpowers/specs/2026-03-24-invoke-skill-tool-design.md @@ -0,0 +1,111 @@ +# Invoke Skill Tool Design + +**Issue:** [#34](https://github.com/madebyaris/native-cli-ai/issues/34) (sub-project 2 of 4) +**Date:** 2026-03-24 +**Status:** Approved + +## Summary + +Add an `invoke_skill` tool that the LLM can call to dynamically load a skill's full instructions. This enables the LLM to auto-invoke skills when a task matches, and enables cross-skill chaining (skills telling the LLM to invoke other skills). + +## Motivation + +Currently, skills can only be invoked by the user typing `/command`. The system prompt includes a skills manifest (name + one-line description) but the LLM has no way to load full skill instructions on its own. This means skills like superpowers' `using-superpowers` — which instructs the LLM to invoke other skills — cannot work. + +## Design + +### Tool Definition + +``` +name: "invoke_skill" +description: "Load a skill's full instructions by name. Use this when a task matches + an available skill from the skills manifest. Returns the complete skill + instructions to follow." +parameters: + type: object + properties: + skill_name: + type: string + description: "The command name from the skills manifest (e.g., 'brainstorming', 'test-driven-development')" + required: ["skill_name"] +``` + +### Tool Behavior + +**On success:** Returns the expanded skill body (with inlined supporting files from sub-project 1): +``` +Skill `{command}` loaded. Follow these instructions: + +{expanded_body} +``` + +**On failure:** Returns error with available skill names: +``` +Skill '{name}' not found. Available skills: brainstorming, test-driven-development, writing-plans, ... +``` + +**No side effects:** The tool only returns text. It does NOT change model, permission mode, or any session state. Model/permission overrides only apply via explicit user `/command` invocation. + +### Implementation Structure + +**New file:** `crates/core/src/tools/invoke_skill.rs` + +Follows the `ToolExecutor` trait pattern (same as `ask_question.rs`): + +```rust +pub struct InvokeSkillTool { + workspace_root: PathBuf, + skill_directories: Vec, +} +``` + +The `execute()` method: +1. Parse `skill_name` from `call.input` +2. Call `SkillCatalog::discover()` with stored workspace root and skill directories +3. Find matching skill by `command` field +4. Return `skill.expanded_body()` as the tool result +5. On no match, return error listing available skill commands + +### Registration + +Register `InvokeSkillTool` **post-construction** in `crates/runtime/src/supervisor.rs`, following the same pattern used for `AskQuestionTool` (lines ~180-183). Do NOT modify the `with_default_readonly_tools` / `with_default_full_tools` factory signatures. + +Since `invoke_skill` is a pure read operation (no side effects), register it in **both** readonly and full tool modes — it should be available even in plan/safe mode. + +The tool needs `workspace_root` (already available at the registration site) and `skill_directories` (from `config.harness.skill_directories`, same field already used by `build_system_prompt`). + +### System Prompt Update + +Change the skills manifest footer in `crates/core/src/harness.rs` (line ~144-146) from: + +``` +"Use these skill summaries when relevant. Full skill instructions are loaded only when explicitly invoked by the user or REPL." +``` + +To: + +``` +"Use the invoke_skill tool to load full instructions when a task matches a skill." +``` + +### What stays the same + +- `SkillCatalog::discover()` — unchanged +- `expanded_body()` — reused from sub-project 1 +- `/command` slash invocation — still works via REPL with model/permission overrides +- All other tools — unchanged +- Skills manifest format in system prompt — only footer text changes + +### Testing + +- Unit test: `invoke_skill` with valid skill name returns expanded body containing skill instructions +- Unit test: `invoke_skill` with unknown skill name returns error listing available skills +- Unit test: tool definition has correct name, description, and parameter schema +- Existing tests: `harness.rs` tests that assert system prompt content **will** need the footer text updated to match the new string + +## Files Affected + +- Create: `crates/core/src/tools/invoke_skill.rs` — new tool implementation +- Modify: `crates/core/src/tools/mod.rs` — export new module +- Modify: tool registration site — register `InvokeSkillTool` +- Modify: `crates/core/src/harness.rs:144-146` — update skills manifest footer diff --git a/docs/superpowers/specs/2026-03-24-question-modal-picker-design.md b/docs/superpowers/specs/2026-03-24-question-modal-picker-design.md new file mode 100644 index 0000000..74930df --- /dev/null +++ b/docs/superpowers/specs/2026-03-24-question-modal-picker-design.md @@ -0,0 +1,131 @@ +# Question Modal Picker Design + +**Issue:** [#32](https://github.com/madebyaris/native-cli-ai/issues/32) +**Date:** 2026-03-24 +**Status:** Approved + +## Summary + +Replace the current number-key-based Ask Question option selection with a modal popup using arrow key navigation, matching the UX of existing pickers (model, branch, permission, agent). Include a "Chat about this" option (when custom answers are allowed) that falls back to inline free-text input. + +## Motivation + +The current Ask Question tool displays options as `[0] suggested`, `[1] option`, `[2] option` and requires the user to type a number. This is inconsistent with the rest of the TUI, which uses arrow-key navigation in modal pickers. The goal is a unified, readable, Claude Code-like selection experience. + +## Design + +### State & Data Model + +New fields in `TuiSessionState`: + +```rust +question_modal_open: bool, +question_modal_index: usize, // currently highlighted option (0 = suggested) +question_modal_scroll: usize, // viewport scroll offset for long option lists +``` + +The modal reads from the existing `active_question: Option` — no separate payload copy. This is consistent with how `active_approval` works. + +The options list is built as: +1. **Suggested answer** (index 0, pre-selected, visually distinguished) +2. **Options from payload** (index 1..n) +3. **"Chat about this"** (last item, **only shown when `allow_custom` is true**) + +Helper methods `open_question_modal()` / `close_question_modal()` follow the existing pattern (see `open_model_picker` / `close_model_picker` in `state.rs`): +- `open_question_modal()`: sets `question_modal_open = true`, `question_modal_index = 0`, `question_modal_scroll = 0` +- `close_question_modal()`: sets `question_modal_open = false`, resets index and scroll + +No changes to `InteractiveQuestionPayload` or `QuestionSelection` enums. + +### `allow_custom` Edge Case + +When `allow_custom` is `false`: +- **"Chat about this"** option is **not shown** in the modal +- **Esc** is a **no-op** (modal stays open, user must pick an option) +- The user can only select the suggested answer or one of the listed options + +When `allow_custom` is `true`: +- **"Chat about this"** is the last item in the list +- **Esc** closes the modal and falls back to inline text input + +### Key Event Handling + +When `question_modal_open` is true, key events are intercepted before any other handler (same priority as existing pickers): + +| Key | Action | +|-----|--------| +| `↑` / `k` | Move highlight up, saturate at 0 | +| `↓` / `j` | Move highlight down, saturate at last item | +| `Enter` | Confirm selection | +| `Esc` | If `allow_custom`: close modal, fall back to inline text input. If `!allow_custom`: no-op | + +No number keys, no search, no other modifiers. + +The event handling slots into `handle_key_event` alongside the model/permission/agent picker blocks, checked early so it captures input before the composer. Mouse events on the transcript are swallowed while the modal is open (consistent with existing picker behavior at `app.rs:2593-2598`). + +### Rendering + +Modal uses `centered_rect()` (existing helper) to create a popup overlay: + +``` +┌─── Question ─────────────────────────┐ +│ │ +│ What would you like to do? │ +│ │ +│ ► Suggested: refactor the module │ ← highlighted +│ Option 1: rewrite from scratch │ +│ Option 2: add tests first │ +│ Chat about this │ ← only if allow_custom +│ │ +│ ↑↓ select · Enter confirm · Esc │ +└──────────────────────────────────────┘ +``` + +Styling (existing theme conventions): +- **Title**: question prompt text, `fg(theme::ASSISTANT)`, bold +- **Selected item**: `bg(theme::USER)`, `fg(Color::Black)`, bold, `►` prefix +- **Unselected items**: `fg(theme::TEXT)`, indented with spaces +- **"Chat about this"**: `fg(theme::MUTED)`, italic +- **Footer**: keybinding hints, `fg(theme::MUTED)`. When `!allow_custom`, omit "Esc" from hints +- **Border**: `Borders::ALL`, standard block + +Modal width adapts to content (capped at ~60% terminal width). If options exceed visible area, viewport scrolling using `question_modal_scroll` (same logic as model picker's `MODEL_PICKER_MAX_ROWS`). + +**Composer hint suppression:** While the question modal is open, the composer hint line (currently shows `"Enter / 0 = suggested..."`) is suppressed or replaced since the user interacts via the modal, not the composer. + +**Empty options list:** When `options` is empty, the modal shows only "Suggested: ..." and (if `allow_custom`) "Chat about this". This is valid and works as expected. + +### Integration & Flow + +**Opening the modal:** +- When `active_question` is set (question payload arrives from core), call `open_question_modal()`. +- The existing inline rendering code for questions (~lines 977-1056 in `app.rs`) is bypassed when the modal is active. + +**Closing the modal — selection paths:** + +| Selection | Action | +|-----------|--------| +| Suggested answer | Send `QuestionSelection::Suggested` via existing channel, call `close_question_modal()`, clear `active_question` | +| Regular option | Send `QuestionSelection::Option { option_id }` via existing channel, call `close_question_modal()`, clear `active_question` | +| "Chat about this" | Call `close_question_modal()`, keep `active_question` alive, switch to inline text input mode (reuse existing `allow_custom` / `[c]` flow) | +| Esc (allow_custom) | Same as "Chat about this" | +| Esc (!allow_custom) | No-op, modal stays open | + +**What stays the same:** +- `InteractiveQuestionPayload` struct — unchanged +- `QuestionSelection` enum — unchanged +- `parse_tui_question_answer` — kept as fallback for the inline text input path +- `/auto-answer` slash command — still works, bypasses modal entirely + +### Testing + +- Unit tests for index bounds (up at 0, down at last) +- Unit tests for selection mapping (index → correct `QuestionSelection` variant) +- Unit tests for `allow_custom: false` behavior (no "Chat about this", Esc is no-op) +- Manual verification of modal rendering and keyboard flow + +## Files Affected + +- `crates/cli/src/tui/state.rs` — add modal state fields, `open_question_modal()` / `close_question_modal()` methods +- `crates/cli/src/tui/app.rs` — rendering (new modal draw function), key event handling, question activation logic, composer hint suppression, mouse event swallowing +- `crates/core/src/tools/ask_question.rs` — no changes expected diff --git a/docs/superpowers/specs/2026-03-24-skill-supporting-files-design.md b/docs/superpowers/specs/2026-03-24-skill-supporting-files-design.md new file mode 100644 index 0000000..93e8cdb --- /dev/null +++ b/docs/superpowers/specs/2026-03-24-skill-supporting-files-design.md @@ -0,0 +1,122 @@ +# Skill Supporting Files Inlining Design + +**Issue:** [#34](https://github.com/madebyaris/native-cli-ai/issues/34) (sub-project 1 of 4) +**Date:** 2026-03-24 +**Status:** Approved + +## Summary + +When a skill is invoked via `/command`, automatically detect file references in the SKILL.md body, read the referenced files from the skill's directory, and inline their contents into the prompt. This ensures skills that reference supporting files (like superpower's `./implementer-prompt.md`, `@testing-anti-patterns.md`, etc.) work correctly. + +## Motivation + +Complex skills like superpowers reference sibling files in their SKILL.md body (e.g., `./implementer-prompt.md`, `@spec-reviewer-prompt.md`, `skills/brainstorming/visual-companion.md`). Currently NCA only loads the SKILL.md body — the LLM has no way to access these referenced files, causing skill invocation to fail. + +## Design + +### Reference Detection + +Scan the SKILL.md body for file references matching these **intentional reference patterns only**: + +1. **`./` prefixed paths**: `./filename.md`, `./subdir/file.md` — strongest signal of intentional reference +2. **`@` prefixed paths**: `@filename.md`, `@testing-anti-patterns.md` — Claude Code convention for "include this file". Strip `@` during path resolution. +3. **Backtick-wrapped paths with directory component**: `` `subdir/file.md` ``, `` `skills/brainstorming/visual-companion.md` `` — when the path contains a `/` +4. **Backtick-wrapped filenames with supported extension**: `` `code-reviewer.md` ``, `` `helper.sh` `` — bare filenames in backticks, resolved only against `skill.directory`. This catches same-directory references like `requesting-code-review`'s `` `code-reviewer.md` ``. + +**Explicitly excluded** (not matched): +- Bare filenames outside backticks without `./` or `@` prefix (e.g., prose mentions of `CLAUDE.md`) +- Well-known config files in backticks: `CLAUDE.md`, `AGENTS.md`, `GEMINI.md`, `SKILL.md`, `package.json`, `README.md` +- URLs (anything starting with `http://` or `https://`) +- Paths containing `path/to/` (template examples) +- The file `SKILL.md` itself + +**Supported extensions**: `.md`, `.sh`, `.ts`, `.js`, `.dot`, `.txt`, `.html`, `.cjs` + +Detection produces a deduplicated list of reference paths, preserving first-occurrence order. + +### Path Resolution + +All references are resolved using a **three-level strategy**: + +1. **Primary**: resolve relative to `skill.directory` (the SKILL.md parent folder) +2. **Catalog root fallback**: if not found, resolve relative to the parent of `skill.directory` (the catalog root, e.g., `~/.nca/skills/` for a skill at `~/.nca/skills/brainstorming/SKILL.md`) +3. **Strip `skills/` prefix**: if still not found and the reference starts with `skills/`, strip the `skills/` prefix and retry against the catalog root. This handles references like `skills/brainstorming/visual-companion.md` where the author's mental model treats `skills/` as a top-level namespace. + +Pattern 4 (bare backtick filenames) only uses level 1 — resolved against `skill.directory` only. This prevents bare filenames from accidentally matching files in other skill directories. + +This handles same-directory references (`./helper.md`), cross-skill references (`brainstorming/visual-companion.md`), and namespaced references (`skills/brainstorming/visual-companion.md`). + +If a file doesn't exist at any resolution level, it's silently skipped. + +### Inlining Logic + +New method on `Skill`: + +```rust +pub fn expanded_body(&self) -> String +``` + +Steps: +1. Extract file references from `self.body` using the patterns above +2. For each reference, resolve path using the three-level strategy +3. Read file contents (skip if read fails or file doesn't exist) +4. Build expanded body: + - Original `self.body` first + - Each referenced file appended as: + ``` + \n\n===== referenced-file.md =====\n\n[file contents] + ``` + +The `=====` separator avoids conflict with markdown `---` (horizontal rules / YAML frontmatter). + +### Integration + +`prompt_for_task()` changes from using `self.body.trim()` to `self.expanded_body().trim()`: + +```rust +pub fn prompt_for_task(&self, task: &str) -> String { + let mut prompt = format!( + "Use the skill `{}`.\n\nSkill instructions:\n{}\n", + self.command, + self.expanded_body().trim() + ); + if !task.trim().is_empty() { + prompt.push_str(&format!("\nTask:\n{}\n", task.trim())); + } + prompt +} +``` + +### What stays the same + +- `parse_skill_file()` — no changes +- `SkillCatalog::discover()` — no changes +- `Skill.body` field — stores raw SKILL.md content, unexpanded +- `manifest_summary()` — uses description, not expanded body +- Slash menu / system prompt skill listing — unchanged +- AGENTS.md parsing — unchanged (AGENTS.md skills don't have supporting files) + +### Known Limitations + +- **No size limit** on inlined content (by design — user preference) +- **Extension list is fixed** — `.md`, `.sh`, `.ts`, `.js`, `.dot`, `.txt`, `.html`, `.cjs`. Can be extended later if needed. +- **References inside code fences are matched** — references inside fenced code blocks (e.g., bad-example illustrations) are scanned. Non-existent files are silently skipped, so this is benign but slightly surprising. +- **No viewport scrolling** for large expanded prompts — relies on existing context window management + +### Testing + +- **Reference detection with `./` paths**: Body with `./foo.md` and `./subdir/bar.md`, verify correct paths extracted in order. +- **Reference detection with `@` paths**: Body with `@helper.md` and `@testing-anti-patterns.md`, verify `@` stripped and paths resolved. +- **Reference detection with backtick directory paths**: Body with `` `skills/brainstorming/visual-companion.md` ``, verify matched. +- **Reference detection with bare backtick filenames**: Body with `` `code-reviewer.md` ``, verify matched. Body with `` `CLAUDE.md` ``, verify NOT matched (excluded). +- **Exclusions**: Body with `AGENTS.md`, `https://example.com/file.md`, `path/to/test.md`, verify none matched. +- **`expanded_body()`**: Temp skill directory with SKILL.md referencing `./helper.md`, verify expanded output contains both with `===== helper.md =====` separator. +- **Missing files**: Reference non-existent file, verify silently skipped, rest expands correctly. +- **Three-level resolution**: Skill in `skills/sdd/SKILL.md` referencing `skills/brainstorming/file.md`, verify `skills/` prefix is stripped and file resolves against catalog root. +- **Bare backtick resolves only against skill directory**: `` `code-reviewer.md` `` in a skill at `skills/review/` resolves only to `skills/review/code-reviewer.md`, NOT to files in other directories. +- **Deduplication**: Same file referenced by `./helper.md` and `` `helper.md` ``, verify inlined only once. +- **No regressions**: Existing `parse_skill_file` and `parse_agents_md` tests pass unchanged. + +## Files Affected + +- `crates/core/src/skills.rs` — add `expanded_body()` method, reference extraction function, update `prompt_for_task()` diff --git a/docs/superpowers/specs/2026-03-24-subagent-skill-indicator-design.md b/docs/superpowers/specs/2026-03-24-subagent-skill-indicator-design.md new file mode 100644 index 0000000..fb9d13f --- /dev/null +++ b/docs/superpowers/specs/2026-03-24-subagent-skill-indicator-design.md @@ -0,0 +1,66 @@ +# Subagent Skill Indicator Design + +**Issue:** [#34](https://github.com/madebyaris/native-cli-ai/issues/34) (sub-project 4 of 4) +**Date:** 2026-03-24 +**Status:** Approved + +## Summary + +Show which skill a subagent is running — in both the TUI sidebar and the transcript. Uses existing `ChildSessionActivity` events with `phase: "skill"` emitted from `InvokeSkillTool`. + +## Motivation + +When subagents invoke skills, there's no visible indicator of which skill is active. Users need to see this for debugging and understanding what their agents are doing. + +## Design + +### Event Emission + +When `InvokeSkillTool::execute()` successfully loads a skill, emit a `ChildSessionActivity` event: +- `child_session_id`: current session ID +- `phase`: `"skill"` +- `detail`: the skill command name (e.g., `"brainstorming"`) + +This requires adding two fields to `InvokeSkillTool`: +- `event_tx: mpsc::Sender` — the event channel (same one used by `AskQuestionTool`) +- `session_id: String` — current session ID (needed for `child_session_id` field) + +### Sidebar Display + +Add `skill: Option` field to `SubagentRow` in `crates/cli/src/tui/state.rs`. + +In `apply_event` for `ChildSessionActivity`: when `phase == "skill"`, set `row.skill = Some(detail.clone())`. + +In the sidebar rendering: when `row.skill` is `Some(name)`, append `[name]` after the task text, styled with `theme::WARN` or similar. + +### Transcript Display + +Already handled — `ChildSessionActivity` events are rendered as `"↳ {short_id}… · {phase} · {detail}"` in `state.rs:667-668`. A skill invocation will appear as: +``` +↳ sub-abc… · skill · brainstorming +``` + +No transcript rendering changes needed. + +### Registration Update + +`InvokeSkillTool::new()` signature changes to accept `event_tx` and `session_id`. The registration in `supervisor.rs` passes these (both are already available at the registration site — `event_tx` is used by `AskQuestionTool`, `session_id` is generated at line ~185). + +### What stays the same + +- Event pipeline — uses existing `ChildSessionActivity`, no new event types +- `ChildSessionSpawned` / `ChildSessionCompleted` — unchanged +- Transcript rendering — existing `System` block formatting handles it +- All other subagent fields — unchanged + +### Testing + +- Unit test: `SubagentRow.skill` populated when `ChildSessionActivity` arrives with `phase == "skill"` +- Manual: invoke skill in subagent, verify sidebar shows skill label and transcript shows activity line + +## Files Affected + +- Modify: `crates/core/src/tools/invoke_skill.rs` — add `event_tx` and `session_id` fields, emit activity event on success +- Modify: `crates/runtime/src/supervisor.rs` — pass `event_tx` and `session_id` to `InvokeSkillTool::new()` +- Modify: `crates/cli/src/tui/state.rs` — add `skill` field to `SubagentRow`, populate on activity event +- Modify: `crates/cli/src/tui/app.rs` — render skill label in sidebar (if sidebar renders subagent rows) From 202bf66ba88e98ede2002cbf861b04c29c53f80c Mon Sep 17 00:00:00 2001 From: Indra Gunanda Date: Wed, 25 Mar 2026 07:38:23 +0700 Subject: [PATCH 12/15] fix: stop infinite tool retry loops after 3 consecutive failures (#34) When the same tool fails 3 times in a row (e.g., ask_question called with malformed inputs by a weaker model), the agent loop now stops and surfaces an error instead of retrying indefinitely. --- crates/core/src/agent.rs | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/crates/core/src/agent.rs b/crates/core/src/agent.rs index 8999832..85f5904 100644 --- a/crates/core/src/agent.rs +++ b/crates/core/src/agent.rs @@ -112,6 +112,10 @@ impl AgentLoop { let mut empty_retries = 0_u32; let mut attachments_cleaned = attachments.is_empty(); const MAX_EMPTY_RETRIES: u32 = 2; + // Consecutive failures of the same tool — stops infinite retry loops. + let mut consecutive_tool_failures: u32 = 0; + let mut last_failed_tool: String = String::new(); + const MAX_CONSECUTIVE_TOOL_FAILURES: u32 = 3; let final_text = loop { if self.is_cancelled() { @@ -475,6 +479,23 @@ impl AgentLoop { .await; } + // Track consecutive failures of the same tool to detect infinite retry loops. + let all_failed_same_tool = !final_results.is_empty() + && final_results.iter().all(|r| !r.success) + && tool_calls.len() == 1; + if all_failed_same_tool { + let tool_name = &tool_calls[0].name; + if *tool_name == last_failed_tool { + consecutive_tool_failures += 1; + } else { + last_failed_tool = tool_name.clone(); + consecutive_tool_failures = 1; + } + } else { + consecutive_tool_failures = 0; + last_failed_tool.clear(); + } + for result in final_results { self.messages.push(Message::tool( result.call_id.clone(), @@ -486,6 +507,18 @@ impl AgentLoop { }) .await; } + + if consecutive_tool_failures >= MAX_CONSECUTIVE_TOOL_FAILURES { + let msg = format!( + "Tool `{}` failed {} times consecutively — stopping to avoid infinite loop.", + last_failed_tool, consecutive_tool_failures + ); + self.emit(AgentEvent::Error { + message: msg.clone(), + }) + .await; + break msg; + } }; if self.cost_tracker.input_tokens == 0 && self.cost_tracker.output_tokens == 0 { From 02bc192d8eab93d9b81f1d5369695760423ab1c5 Mon Sep 17 00:00:00 2001 From: Indra Gunanda Date: Wed, 25 Mar 2026 09:01:04 +0700 Subject: [PATCH 13/15] feat: add skill_installer module with source parsing, lock file, install/remove (#36) --- Cargo.lock | 1 + crates/core/Cargo.toml | 3 +- crates/core/src/lib.rs | 1 + crates/core/src/skill_installer.rs | 588 +++++++++++++++++++++++++++++ 4 files changed, 592 insertions(+), 1 deletion(-) create mode 100644 crates/core/src/skill_installer.rs diff --git a/Cargo.lock b/Cargo.lock index 351935a..5f50a86 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2146,6 +2146,7 @@ version = "0.2.0" dependencies = [ "async-trait", "base64", + "chrono", "futures-util", "genai", "globset", diff --git a/crates/core/Cargo.toml b/crates/core/Cargo.toml index d1bf639..3d8895f 100644 --- a/crates/core/Cargo.toml +++ b/crates/core/Cargo.toml @@ -23,8 +23,9 @@ futures-util = "0.3" serde_yaml = "0.9.34" mcpr = "0.2.3" regex = "1" +chrono = { version = "0.4", features = ["serde", "clock"] } +tempfile = "3" [dev-dependencies] -tempfile = "3" tiny_http = "0.12.0" tokio = { workspace = true, features = ["test-util"] } diff --git a/crates/core/src/lib.rs b/crates/core/src/lib.rs index 38ddd6d..f5153aa 100644 --- a/crates/core/src/lib.rs +++ b/crates/core/src/lib.rs @@ -5,5 +5,6 @@ pub mod cost; pub mod harness; pub mod hooks; pub mod provider; +pub mod skill_installer; pub mod skills; pub mod tools; diff --git a/crates/core/src/skill_installer.rs b/crates/core/src/skill_installer.rs new file mode 100644 index 0000000..9bcf083 --- /dev/null +++ b/crates/core/src/skill_installer.rs @@ -0,0 +1,588 @@ +//! Skill installation: source parsing, git clone, file copy, lock file management. + +use serde::{Deserialize, Serialize}; +use std::collections::BTreeMap; +use std::path::{Path, PathBuf}; + +/// Parsed source for skill installation. +#[derive(Debug, Clone)] +pub enum SkillSource { + /// GitHub repo: clone URL derived from owner/repo or full URL. + GitHub { clone_url: String }, + /// Local directory path. + Local { path: PathBuf }, +} + +/// Parse a source string into a SkillSource. +/// +/// Supports: +/// - `owner/repo` → GitHub clone URL +/// - `https://github.com/owner/repo` → GitHub clone URL +/// - `./path` or `/absolute/path` → Local path +pub fn parse_source(source: &str) -> Result { + let trimmed = source.trim(); + + // Local paths: starts with ./ or / or ~ + if trimmed.starts_with('/') + || trimmed.starts_with("./") + || trimmed.starts_with("~/") + || trimmed.starts_with("..") + { + let path = if trimmed.starts_with("~/") { + let home = std::env::var("HOME").unwrap_or_else(|_| "/tmp".into()); + PathBuf::from(home).join(trimmed.strip_prefix("~/").unwrap()) + } else { + PathBuf::from(trimmed) + }; + return Ok(SkillSource::Local { path }); + } + + // GitHub URL: https://github.com/owner/repo + if trimmed.starts_with("https://github.com/") { + let url = trimmed.trim_end_matches('/'); + let clone_url = if url.ends_with(".git") { + url.to_string() + } else { + format!("{url}.git") + }; + return Ok(SkillSource::GitHub { clone_url }); + } + + // GitHub shorthand: owner/repo (exactly one slash, no dots or colons) + if trimmed.contains('/') + && trimmed.matches('/').count() == 1 + && !trimmed.contains(':') + && !trimmed.contains('.') + { + let clone_url = format!("https://github.com/{trimmed}.git"); + return Ok(SkillSource::GitHub { clone_url }); + } + + Err(format!( + "Cannot parse source '{trimmed}'. Use owner/repo, a GitHub URL, or a local path." + )) +} + +/// Sanitize a skill name for use as a directory name. +/// Rejects names containing `..` (path traversal). +/// Converts to kebab-case: lowercase, non-alphanumeric → `-`, strip leading/trailing `-`. +pub fn sanitize_skill_name(name: &str) -> Result { + if name.contains("..") { + return Err(format!("Skill name '{name}' contains path traversal (..)")); + } + + let mut out = String::new(); + let mut last_dash = false; + for ch in name.chars() { + if ch.is_ascii_alphanumeric() { + out.push(ch.to_ascii_lowercase()); + last_dash = false; + } else if !last_dash && !out.is_empty() { + out.push('-'); + last_dash = true; + } + } + let sanitized = out.trim_matches('-').to_string(); + if sanitized.is_empty() { + return Err(format!("Skill name '{name}' produces empty sanitized name")); + } + Ok(sanitized) +} + +/// Lock file tracking installed skills. +#[derive(Debug, Clone, Serialize, Deserialize, Default)] +pub struct SkillLock { + pub skills: BTreeMap, +} + +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct SkillLockEntry { + pub source: String, + pub commit: Option, + pub installed_at: String, +} + +impl SkillLock { + /// Read lock file from path. Returns empty lock if file doesn't exist. + pub fn load(path: &Path) -> Result { + if !path.exists() { + return Ok(Self::default()); + } + let content = + std::fs::read_to_string(path).map_err(|e| format!("failed to read lock file: {e}"))?; + serde_json::from_str(&content).map_err(|e| format!("failed to parse lock file: {e}")) + } + + /// Write lock file to path. Creates parent directories if needed. + pub fn save(&self, path: &Path) -> Result<(), String> { + if let Some(parent) = path.parent() { + std::fs::create_dir_all(parent) + .map_err(|e| format!("failed to create lock file directory: {e}"))?; + } + let content = serde_json::to_string_pretty(self) + .map_err(|e| format!("failed to serialize lock file: {e}"))?; + std::fs::write(path, content).map_err(|e| format!("failed to write lock file: {e}")) + } + + /// Add or update a skill entry. + pub fn upsert(&mut self, name: &str, entry: SkillLockEntry) { + self.skills.insert(name.to_string(), entry); + } + + /// Remove a skill entry. Returns true if it existed. + pub fn remove(&mut self, name: &str) -> bool { + self.skills.remove(name).is_some() + } +} + +/// Get the lock file path for the given scope. +pub fn lock_file_path(global: bool, workspace_root: &Path) -> PathBuf { + if global { + let home = std::env::var("HOME").unwrap_or_else(|_| "/tmp".into()); + PathBuf::from(home).join(".nca/skills.lock") + } else { + workspace_root.join(".nca/skills.lock") + } +} + +/// Get the skills directory for the given scope. +pub fn skills_dir(global: bool, workspace_root: &Path) -> PathBuf { + if global { + let home = std::env::var("HOME").unwrap_or_else(|_| "/tmp".into()); + PathBuf::from(home).join(".nca/skills") + } else { + workspace_root.join(".nca/skills") + } +} + +/// Clone a git repo to a temp directory (shallow, depth=1). +/// Returns the temp directory path. +pub fn git_clone_to_temp(clone_url: &str) -> Result { + let tmp = tempfile::tempdir().map_err(|e| format!("failed to create temp dir: {e}"))?; + let status = std::process::Command::new("git") + .args([ + "clone", + "--depth", + "1", + clone_url, + &tmp.path().display().to_string(), + ]) + .stdout(std::process::Stdio::piped()) + .stderr(std::process::Stdio::piped()) + .status() + .map_err(|e| format!("failed to run git clone: {e}"))?; + if !status.success() { + return Err(format!( + "git clone failed for '{clone_url}' (exit code: {status})" + )); + } + Ok(tmp) +} + +/// Get the HEAD commit hash of a git repo. +pub fn git_head_commit(repo_path: &Path) -> Result { + let output = std::process::Command::new("git") + .args(["-C", &repo_path.display().to_string(), "rev-parse", "HEAD"]) + .output() + .map_err(|e| format!("failed to run git rev-parse: {e}"))?; + if !output.status.success() { + return Err("git rev-parse HEAD failed".into()); + } + Ok(String::from_utf8_lossy(&output.stdout).trim().to_string()) +} + +/// Discover SKILL.md files in a directory tree. +/// Returns (skill_name, skill_directory) pairs. +pub fn discover_skills_in_dir(dir: &Path) -> Result, String> { + let mut found = Vec::new(); + let entries = + std::fs::read_dir(dir).map_err(|e| format!("failed to read dir {}: {e}", dir.display()))?; + + for entry in entries { + let entry = entry.map_err(|e| e.to_string())?; + let path = entry.path(); + if path.is_dir() { + let skill_file = path.join("SKILL.md"); + if skill_file.exists() + && let Some(name) = path.file_name().and_then(|n| n.to_str()) + { + found.push((name.to_string(), path)); + } + } + } + + Ok(found) +} + +/// Copy a skill directory (including all files) to the target location. +/// Creates the target directory if it doesn't exist. +pub fn copy_skill_dir(source: &Path, target: &Path) -> Result<(), String> { + if target.exists() { + std::fs::remove_dir_all(target) + .map_err(|e| format!("failed to remove existing skill dir: {e}"))?; + } + copy_dir_recursive(source, target) +} + +fn copy_dir_recursive(src: &Path, dst: &Path) -> Result<(), String> { + std::fs::create_dir_all(dst) + .map_err(|e| format!("failed to create dir {}: {e}", dst.display()))?; + for entry in std::fs::read_dir(src).map_err(|e| format!("read dir: {e}"))? { + let entry = entry.map_err(|e| e.to_string())?; + let src_path = entry.path(); + let dst_path = dst.join(entry.file_name()); + if src_path.is_dir() { + copy_dir_recursive(&src_path, &dst_path)?; + } else { + std::fs::copy(&src_path, &dst_path) + .map_err(|e| format!("copy {}: {e}", src_path.display()))?; + } + } + Ok(()) +} + +/// Install skills from a parsed source. +/// Returns list of installed skill names. +pub fn install_skills( + source: &SkillSource, + skill_filter: &[String], + global: bool, + workspace_root: &Path, +) -> Result, String> { + let (skills_root, commit, _tmp) = match source { + SkillSource::GitHub { clone_url } => { + let tmp = git_clone_to_temp(clone_url)?; + let commit = git_head_commit(tmp.path()).ok(); + // Look for skills in root or in a "skills" subdirectory + let skills_path = if tmp.path().join("skills").is_dir() { + tmp.path().join("skills") + } else { + tmp.path().to_path_buf() + }; + (skills_path, commit, Some(tmp)) + } + SkillSource::Local { path } => { + let resolved = if path.is_relative() { + workspace_root.join(path) + } else { + path.clone() + }; + if !resolved.exists() { + return Err(format!( + "Local path '{}' does not exist", + resolved.display() + )); + } + // Look for skills in root or in a "skills" subdirectory + let skills_path = if resolved.join("skills").is_dir() { + resolved.join("skills") + } else { + resolved + }; + (skills_path, None, None) + } + }; + + let discovered = discover_skills_in_dir(&skills_root)?; + if discovered.is_empty() { + return Err(format!( + "No SKILL.md files found in '{}'", + skills_root.display() + )); + } + + let filtered: Vec<_> = if skill_filter.is_empty() { + discovered + } else { + discovered + .into_iter() + .filter(|(name, _)| skill_filter.iter().any(|f| f == name)) + .collect() + }; + + if filtered.is_empty() { + let available: Vec<_> = discover_skills_in_dir(&skills_root)? + .into_iter() + .map(|(n, _)| n) + .collect(); + return Err(format!( + "No matching skills found. Available: {}", + available.join(", ") + )); + } + + let target_dir = skills_dir(global, workspace_root); + let lock_path = lock_file_path(global, workspace_root); + let mut lock = SkillLock::load(&lock_path)?; + let now = chrono::Utc::now().to_rfc3339(); + + let source_label = match source { + SkillSource::GitHub { clone_url } => format!( + "github:{}", + clone_url + .trim_end_matches(".git") + .trim_start_matches("https://github.com/") + ), + SkillSource::Local { path } => format!("local:{}", path.display()), + }; + + let mut installed = Vec::new(); + for (name, src_dir) in &filtered { + let safe_name = sanitize_skill_name(name)?; + let dest = target_dir.join(&safe_name); + + if dest.exists() { + eprintln!("Warning: overwriting existing skill '{safe_name}'"); + } + + copy_skill_dir(src_dir, &dest)?; + + lock.upsert( + &safe_name, + SkillLockEntry { + source: source_label.clone(), + commit: commit.clone(), + installed_at: now.clone(), + }, + ); + + installed.push(safe_name); + } + + lock.save(&lock_path)?; + Ok(installed) +} + +/// Remove an installed skill by name. +pub fn remove_skill(name: &str, global: bool, workspace_root: &Path) -> Result<(), String> { + let lock_path = lock_file_path(global, workspace_root); + let mut lock = SkillLock::load(&lock_path)?; + + if !lock.remove(name) { + let available: Vec<_> = lock.skills.keys().cloned().collect(); + let scope = if global { "global" } else { "local" }; + return Err(if available.is_empty() { + format!("No {scope} skills installed") + } else { + format!( + "Skill '{name}' not found in {scope} lock file. Available: {}", + available.join(", ") + ) + }); + } + + let dir = skills_dir(global, workspace_root).join(name); + if dir.exists() { + std::fs::remove_dir_all(&dir).map_err(|e| format!("failed to remove skill dir: {e}"))?; + } + + lock.save(&lock_path)?; + Ok(()) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn parses_github_shorthand() { + let result = parse_source("vercel-labs/agent-skills").unwrap(); + match result { + SkillSource::GitHub { clone_url } => { + assert_eq!(clone_url, "https://github.com/vercel-labs/agent-skills.git"); + } + _ => panic!("expected GitHub source"), + } + } + + #[test] + fn parses_github_url() { + let result = parse_source("https://github.com/owner/repo").unwrap(); + match result { + SkillSource::GitHub { clone_url } => { + assert_eq!(clone_url, "https://github.com/owner/repo.git"); + } + _ => panic!("expected GitHub source"), + } + } + + #[test] + fn parses_github_url_with_git_suffix() { + let result = parse_source("https://github.com/owner/repo.git").unwrap(); + match result { + SkillSource::GitHub { clone_url } => { + assert_eq!(clone_url, "https://github.com/owner/repo.git"); + } + _ => panic!("expected GitHub source"), + } + } + + #[test] + fn parses_local_relative_path() { + let result = parse_source("./my-skills").unwrap(); + match result { + SkillSource::Local { path } => assert_eq!(path, PathBuf::from("./my-skills")), + _ => panic!("expected Local source"), + } + } + + #[test] + fn parses_local_absolute_path() { + let result = parse_source("/home/user/skills").unwrap(); + match result { + SkillSource::Local { path } => assert_eq!(path, PathBuf::from("/home/user/skills")), + _ => panic!("expected Local source"), + } + } + + #[test] + fn rejects_invalid_source() { + assert!(parse_source("just-a-word").is_err()); + } + + #[test] + fn sanitizes_skill_name() { + assert_eq!(sanitize_skill_name("My Skill").unwrap(), "my-skill"); + assert_eq!( + sanitize_skill_name("test-driven-development").unwrap(), + "test-driven-development" + ); + assert_eq!( + sanitize_skill_name(" Spaces & Symbols! ").unwrap(), + "spaces-symbols" + ); + } + + #[test] + fn rejects_path_traversal_in_name() { + assert!(sanitize_skill_name("../evil").is_err()); + assert!(sanitize_skill_name("foo/../bar").is_err()); + } + + #[test] + fn rejects_empty_sanitized_name() { + assert!(sanitize_skill_name("!!!").is_err()); + } + + #[test] + fn lock_file_roundtrip() { + let dir = tempfile::tempdir().unwrap(); + let lock_path = dir.path().join(".nca/skills.lock"); + + let mut lock = SkillLock::default(); + lock.upsert( + "brainstorming", + SkillLockEntry { + source: "github:owner/repo".into(), + commit: Some("abc123".into()), + installed_at: "2026-03-25T00:00:00Z".into(), + }, + ); + + lock.save(&lock_path).unwrap(); + let loaded = SkillLock::load(&lock_path).unwrap(); + assert_eq!(loaded.skills.len(), 1); + assert_eq!( + loaded.skills["brainstorming"].commit.as_deref(), + Some("abc123") + ); + } + + #[test] + fn lock_file_load_missing_returns_empty() { + let dir = tempfile::tempdir().unwrap(); + let lock = SkillLock::load(&dir.path().join("nonexistent.lock")).unwrap(); + assert!(lock.skills.is_empty()); + } + + #[test] + fn lock_file_remove_entry() { + let mut lock = SkillLock::default(); + lock.upsert( + "test", + SkillLockEntry { + source: "github:a/b".into(), + commit: None, + installed_at: "2026-03-25T00:00:00Z".into(), + }, + ); + assert!(lock.remove("test")); + assert!(!lock.remove("test")); + assert!(lock.skills.is_empty()); + } + + #[test] + fn install_skills_from_local_path() { + let src = tempfile::tempdir().unwrap(); + let ws = tempfile::tempdir().unwrap(); + + // Create a skill in the source + let skill_dir = src.path().join("my-skill"); + std::fs::create_dir_all(&skill_dir).unwrap(); + std::fs::write( + skill_dir.join("SKILL.md"), + "---\nname: My Skill\ncommand: my-skill\n---\nBody.\n", + ) + .unwrap(); + std::fs::write(skill_dir.join("helper.md"), "Helper.").unwrap(); + + let source = SkillSource::Local { + path: src.path().to_path_buf(), + }; + let installed = install_skills(&source, &[], false, ws.path()).unwrap(); + + assert_eq!(installed, vec!["my-skill"]); + + // Verify files were copied + let dest = ws.path().join(".nca/skills/my-skill/SKILL.md"); + assert!(dest.exists()); + let helper = ws.path().join(".nca/skills/my-skill/helper.md"); + assert!(helper.exists()); + + // Verify lock file + let lock = SkillLock::load(&ws.path().join(".nca/skills.lock")).unwrap(); + assert!(lock.skills.contains_key("my-skill")); + assert!(lock.skills["my-skill"].commit.is_none()); // local install + } + + #[test] + fn install_skills_no_skills_found_errors() { + let src = tempfile::tempdir().unwrap(); + let ws = tempfile::tempdir().unwrap(); + + // Empty directory — no SKILL.md + let source = SkillSource::Local { + path: src.path().to_path_buf(), + }; + let result = install_skills(&source, &[], false, ws.path()); + assert!(result.is_err()); + assert!(result.unwrap_err().contains("No SKILL.md")); + } + + #[test] + fn remove_skill_deletes_dir_and_lock_entry() { + let ws = tempfile::tempdir().unwrap(); + let skill_dir = ws.path().join(".nca/skills/test-skill"); + std::fs::create_dir_all(&skill_dir).unwrap(); + std::fs::write(skill_dir.join("SKILL.md"), "test").unwrap(); + + // Create lock entry + let lock_path = ws.path().join(".nca/skills.lock"); + let mut lock = SkillLock::default(); + lock.upsert( + "test-skill", + SkillLockEntry { + source: "github:a/b".into(), + commit: Some("abc".into()), + installed_at: "2026-03-25T00:00:00Z".into(), + }, + ); + lock.save(&lock_path).unwrap(); + + remove_skill("test-skill", false, ws.path()).unwrap(); + + assert!(!skill_dir.exists()); + let lock = SkillLock::load(&lock_path).unwrap(); + assert!(!lock.skills.contains_key("test-skill")); + } +} From 037da0e96a8d203ff603935586212c04e6aa4201 Mon Sep 17 00:00:00 2001 From: Indra Gunanda Date: Wed, 25 Mar 2026 09:04:55 +0700 Subject: [PATCH 14/15] feat: expand nca skills to add/remove/update subcommands (#36) --- crates/cli/src/main.rs | 175 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 172 insertions(+), 3 deletions(-) diff --git a/crates/cli/src/main.rs b/crates/cli/src/main.rs index f028d39..4b10680 100644 --- a/crates/cli/src/main.rs +++ b/crates/cli/src/main.rs @@ -200,9 +200,13 @@ enum Command { #[arg(long)] json: bool, }, + /// Manage skills: list, add, remove, update Skills { + /// Output as JSON (shorthand for `nca skills list --json`) #[arg(long)] json: bool, + #[command(subcommand)] + command: Option, }, Mcp { #[arg(long)] @@ -291,6 +295,39 @@ enum MemoryCommand { }, } +#[derive(clap::Subcommand, Debug)] +enum SkillsCommand { + /// List installed skills + List { + #[arg(long)] + json: bool, + }, + /// Install skills from a GitHub repo or local path + Add { + /// Source: owner/repo, GitHub URL, or local path + source: String, + /// Install specific skills by name (default: all) + #[arg(short, long)] + skill: Vec, + /// Install to ~/.nca/skills/ instead of .nca/skills/ + #[arg(short, long)] + global: bool, + }, + /// Remove an installed skill + Remove { + /// Skill command name to remove + name: String, + /// Remove from ~/.nca/skills/ instead of .nca/skills/ + #[arg(short, long)] + global: bool, + }, + /// Update installed skills from their source + Update { + /// Specific skill to update (default: all) + name: Option, + }, +} + #[derive(clap::ValueEnum, Clone, Copy, Debug)] enum CliPermissionMode { Default, @@ -505,9 +542,27 @@ async fn try_main() -> anyhow::Result<()> { Some(Command::Cancel { session_id, json }) => { cancel_session(&config, &workspace_root, &session_id, json).await?; } - Some(Command::Skills { json }) => { - list_skills(&config, &workspace_root, json)?; - } + Some(Command::Skills { json, command }) => match command { + None => { + list_skills(&config, &workspace_root, json)?; + } + Some(SkillsCommand::List { json: j }) => { + list_skills(&config, &workspace_root, j || json)?; + } + Some(SkillsCommand::Add { + source, + skill, + global, + }) => { + handle_skills_add(&source, &skill, global, &workspace_root)?; + } + Some(SkillsCommand::Remove { name, global }) => { + handle_skills_remove(&name, global, &workspace_root)?; + } + Some(SkillsCommand::Update { name }) => { + handle_skills_update(name.as_deref(), &workspace_root)?; + } + }, Some(Command::Mcp { json }) => { list_mcp_servers(&config, json)?; } @@ -1274,6 +1329,120 @@ fn list_skills(config: &NcaConfig, workspace_root: &Path, json: bool) -> anyhow: Ok(()) } +fn handle_skills_add( + source: &str, + skill_filter: &[String], + global: bool, + workspace_root: &Path, +) -> anyhow::Result<()> { + use nca_core::skill_installer::{install_skills, parse_source}; + + let parsed = parse_source(source).map_err(anyhow::Error::msg)?; + let installed = install_skills(&parsed, skill_filter, global, workspace_root) + .map_err(anyhow::Error::msg)?; + + let scope = if global { "(global)" } else { "(local)" }; + println!( + "Installed {} skill(s) {scope}: {}", + installed.len(), + installed.join(", ") + ); + Ok(()) +} + +fn handle_skills_remove(name: &str, global: bool, workspace_root: &Path) -> anyhow::Result<()> { + use nca_core::skill_installer::remove_skill; + + remove_skill(name, global, workspace_root).map_err(anyhow::Error::msg)?; + println!("Removed skill: {name}"); + Ok(()) +} + +fn handle_skills_update(name: Option<&str>, workspace_root: &Path) -> anyhow::Result<()> { + use nca_core::skill_installer::{ + SkillLock, SkillLockEntry, copy_skill_dir, discover_skills_in_dir, git_clone_to_temp, + git_head_commit, lock_file_path, skills_dir, + }; + + let mut updated = 0u32; + let mut up_to_date = 0u32; + let mut skipped = 0u32; + + for global in [false, true] { + let lock_path = lock_file_path(global, workspace_root); + let mut lock = SkillLock::load(&lock_path).map_err(anyhow::Error::msg)?; + let target = skills_dir(global, workspace_root); + let mut changed = false; + + let entries: Vec<_> = lock + .skills + .iter() + .filter(|(k, _)| name.is_none() || name == Some(k.as_str())) + .map(|(k, v)| (k.clone(), v.clone())) + .collect(); + + for (skill_name, entry) in entries { + if entry.commit.is_none() { + eprintln!("Skipping '{skill_name}' (installed from local path)"); + skipped += 1; + continue; + } + + let clone_url = entry + .source + .strip_prefix("github:") + .map(|repo| format!("https://github.com/{repo}.git")); + + let Some(url) = clone_url else { + eprintln!("Skipping '{skill_name}' (unknown source format)"); + skipped += 1; + continue; + }; + + let tmp = git_clone_to_temp(&url).map_err(anyhow::Error::msg)?; + let new_commit = git_head_commit(tmp.path()).ok(); + + if new_commit.as_deref() == entry.commit.as_deref() { + up_to_date += 1; + continue; + } + + let skills_path = if tmp.path().join("skills").is_dir() { + tmp.path().join("skills") + } else { + tmp.path().to_path_buf() + }; + + let discovered = discover_skills_in_dir(&skills_path).map_err(anyhow::Error::msg)?; + + if let Some((_, src_dir)) = discovered.iter().find(|(n, _)| n == &skill_name) { + let dest = target.join(&skill_name); + copy_skill_dir(src_dir, &dest).map_err(anyhow::Error::msg)?; + lock.upsert( + &skill_name, + SkillLockEntry { + source: entry.source.clone(), + commit: new_commit, + installed_at: chrono::Utc::now().to_rfc3339(), + }, + ); + changed = true; + updated += 1; + } else { + eprintln!("Warning: skill '{skill_name}' no longer found in source repo"); + skipped += 1; + } + } + + if changed { + lock.save(&lock_path).map_err(anyhow::Error::msg)?; + } + } + + println!("Updated {updated}, already up-to-date {up_to_date}, skipped {skipped}"); + Ok(()) +} + fn list_mcp_servers(config: &NcaConfig, json: bool) -> anyhow::Result<()> { if json { print_json(&config.mcp, false)?; From 7d4b5d3992044bbd09fd923f6ac51dd0a54ef7f4 Mon Sep 17 00:00:00 2001 From: Test Date: Sat, 28 Mar 2026 13:12:12 +0700 Subject: [PATCH 15/15] fix(tui): prioritize approval shortcuts over question modal input Ensure active approvals always receive Ctrl+Y/Ctrl+N/Ctrl+U handling even when the question modal is open. Also harden git integration tests against inherited git environment and host hooks, and make doctor JSON CLI test deterministic against host env overrides. --- crates/autoresearch/src/git_integration.rs | 50 +++++- crates/cli/src/tui/app.rs | 191 +++++++++++++++------ crates/cli/tests/cli_commands.rs | 3 + 3 files changed, 186 insertions(+), 58 deletions(-) diff --git a/crates/autoresearch/src/git_integration.rs b/crates/autoresearch/src/git_integration.rs index 92d39f7..5ab81cb 100644 --- a/crates/autoresearch/src/git_integration.rs +++ b/crates/autoresearch/src/git_integration.rs @@ -7,6 +7,26 @@ use std::path::{Path, PathBuf}; use anyhow::{Context, Result}; use tokio::process::Command; +fn sanitize_git_env(cmd: &mut Command) -> &mut Command { + for key in [ + "GIT_DIR", + "GIT_WORK_TREE", + "GIT_INDEX_FILE", + "GIT_PREFIX", + "GIT_OBJECT_DIRECTORY", + "GIT_ALTERNATE_OBJECT_DIRECTORIES", + "GIT_COMMON_DIR", + "GIT_CONFIG", + "GIT_CONFIG_GLOBAL", + "GIT_CONFIG_SYSTEM", + "GIT_CEILING_DIRECTORIES", + "GIT_DISCOVERY_ACROSS_FILESYSTEM", + ] { + cmd.env_remove(key); + } + cmd +} + /// Git manager for autonomous research workflows pub struct GitManager { repo_path: PathBuf, @@ -22,7 +42,9 @@ impl GitManager { /// Run a git command and return the output async fn run(&self, args: &[&str]) -> Result { - let output = Command::new("git") + let mut cmd = Command::new("git"); + sanitize_git_env(&mut cmd); + let output = cmd .current_dir(&self.repo_path) .args(args) .output() @@ -294,21 +316,35 @@ mod tests { let temp = TempDir::new()?; // Initialize git repo - Command::new("git") - .current_dir(temp.path()) + let mut init = Command::new("git"); + sanitize_git_env(&mut init); + init.current_dir(temp.path()) .args(["init"]) .output() .await .expect("git init failed"); - Command::new("git") + let mut config_hooks = Command::new("git"); + sanitize_git_env(&mut config_hooks); + config_hooks + .current_dir(temp.path()) + .args(["config", "core.hooksPath", "/dev/null"]) + .output() + .await + .expect("git config failed"); + + let mut config_email = Command::new("git"); + sanitize_git_env(&mut config_email); + config_email .current_dir(temp.path()) .args(["config", "user.email", "test@test.com"]) .output() .await .expect("git config failed"); - Command::new("git") + let mut config_name = Command::new("git"); + sanitize_git_env(&mut config_name); + config_name .current_dir(temp.path()) .args(["config", "user.name", "Test"]) .output() @@ -316,7 +352,9 @@ mod tests { .expect("git config failed"); // Add an initial commit so HEAD is valid - Command::new("git") + let mut initial_commit = Command::new("git"); + sanitize_git_env(&mut initial_commit); + initial_commit .current_dir(temp.path()) .args(["commit", "--allow-empty", "-m", "initial"]) .output() diff --git a/crates/cli/src/tui/app.rs b/crates/cli/src/tui/app.rs index 660404a..0eb6a54 100644 --- a/crates/cli/src/tui/app.rs +++ b/crates/cli/src/tui/app.rs @@ -1301,6 +1301,42 @@ fn parse_approval_verdict(line: &str) -> Option { } } +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum PrimaryInputMode { + Approval, + QuestionModal, + Normal, +} + +fn primary_input_mode(active_approval: bool, question_modal_open: bool) -> PrimaryInputMode { + if active_approval { + PrimaryInputMode::Approval + } else if question_modal_open { + PrimaryInputMode::QuestionModal + } else { + PrimaryInputMode::Normal + } +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum ApprovalShortcutAction { + Approve, + Deny, + AllowPattern, +} + +fn approval_shortcut_action( + code: KeyCode, + modifiers: KeyModifiers, +) -> Option { + match (code, modifiers) { + (KeyCode::Char('y'), KeyModifiers::CONTROL) => Some(ApprovalShortcutAction::Approve), + (KeyCode::Char('n'), KeyModifiers::CONTROL) => Some(ApprovalShortcutAction::Deny), + (KeyCode::Char('u'), KeyModifiers::CONTROL) => Some(ApprovalShortcutAction::AllowPattern), + _ => None, + } +} + fn parse_md_line(line: &str) -> Line<'static> { if line.starts_with("```") { return Line::from(Span::styled( @@ -3177,8 +3213,55 @@ pub fn run_blocking( continue; } + let primary_mode = + primary_input_mode(g.active_approval.is_some(), g.question_modal_open); + + // Approval shortcuts take precedence over question-modal handling. + if matches!(primary_mode, PrimaryInputMode::Approval) + && let Some(req) = g.active_approval.clone() + && let Some(action) = approval_shortcut_action(key.code, key.modifiers) + { + let call_id = req.call_id.clone(); + g.input_buffer.clear(); + g.cursor_char_idx = 0; + match action { + ApprovalShortcutAction::Approve => { + drop(g); + if let Some(ref tx) = approval_answer_tx { + let _ = tx.send(ApprovalAnswer::Verdict { + call_id, + approved: true, + }); + } + } + ApprovalShortcutAction::Deny => { + drop(g); + if let Some(ref tx) = approval_answer_tx { + let _ = tx.send(ApprovalAnswer::Verdict { + call_id, + approved: false, + }); + } + } + ApprovalShortcutAction::AllowPattern => { + let input_json: serde_json::Value = + serde_json::from_str(&req.input).unwrap_or_default(); + let pattern = suggest_allow_pattern(&req.tool, &input_json); + g.blocks.push(DisplayBlock::System(format!( + "Always allowing: {pattern}" + ))); + drop(g); + if let Some(ref tx) = approval_answer_tx { + let _ = + tx.send(ApprovalAnswer::AllowPattern { call_id, pattern }); + } + } + } + continue; + } + // Question modal keyboard handling. - if g.question_modal_open { + if matches!(primary_mode, PrimaryInputMode::QuestionModal) { if let Some(ref q) = g.active_question.clone() { // Total items: 1 (suggested) + options.len() + (1 if allow_custom for "Chat about this") let total = 1 + q.options.len() + if q.allow_custom { 1 } else { 0 }; @@ -3460,55 +3543,6 @@ pub fn run_blocking( drop(g); let _ = cmd_tx.send(TuiCmd::CycleModel(false)); } - (KeyCode::Char('y'), KeyModifiers::CONTROL) => { - if let Some(req) = g.active_approval.clone() { - let call_id = req.call_id.clone(); - g.input_buffer.clear(); - g.cursor_char_idx = 0; - drop(g); - if let Some(ref tx) = approval_answer_tx { - let _ = tx.send(ApprovalAnswer::Verdict { - call_id, - approved: true, - }); - } - continue; - } - } - (KeyCode::Char('n'), KeyModifiers::CONTROL) => { - if let Some(req) = g.active_approval.clone() { - let call_id = req.call_id.clone(); - g.input_buffer.clear(); - g.cursor_char_idx = 0; - drop(g); - if let Some(ref tx) = approval_answer_tx { - let _ = tx.send(ApprovalAnswer::Verdict { - call_id, - approved: false, - }); - } - continue; - } - } - (KeyCode::Char('u'), KeyModifiers::CONTROL) => { - if let Some(req) = g.active_approval.clone() { - let input_json: serde_json::Value = - serde_json::from_str(&req.input).unwrap_or_default(); - let pattern = suggest_allow_pattern(&req.tool, &input_json); - let call_id = req.call_id.clone(); - g.input_buffer.clear(); - g.cursor_char_idx = 0; - g.blocks.push(DisplayBlock::System(format!( - "Always allowing: {pattern}" - ))); - drop(g); - if let Some(ref tx) = approval_answer_tx { - let _ = - tx.send(ApprovalAnswer::AllowPattern { call_id, pattern }); - } - continue; - } - } (KeyCode::Enter, _) => { if let Some((buf, cidx)) = apply_selected_at_completion( &workspace_files, @@ -3775,10 +3809,12 @@ pub fn run_blocking( #[cfg(test)] mod approval_parse_tests { use super::{ - TuiCmd, apply_selected_at_completion, branch_picker_enter_command, + ApprovalShortcutAction, PrimaryInputMode, TuiCmd, apply_selected_at_completion, + approval_shortcut_action, branch_picker_enter_command, completed_at_mention_range_before_cursor, composer_line, delete_completed_at_mention, - filtered_branch_indices, parse_approval_verdict, + filtered_branch_indices, parse_approval_verdict, primary_input_mode, }; + use crossterm::event::{KeyCode, KeyModifiers}; #[test] fn parses_yes_with_punctuation_and_synonyms() { @@ -3889,4 +3925,55 @@ mod approval_parse_tests { assert_eq!(mention_span.style.bg, Some(super::theme::MENTION_BG)); } + + #[test] + fn primary_input_mode_prefers_approval_when_both_active() { + assert_eq!(primary_input_mode(true, true), PrimaryInputMode::Approval); + } + + #[test] + fn primary_input_mode_uses_question_modal_without_approval() { + assert_eq!( + primary_input_mode(false, true), + PrimaryInputMode::QuestionModal + ); + } + + #[test] + fn approval_hotkeys_map_ctrl_y_and_ctrl_n() { + assert_eq!( + approval_shortcut_action(KeyCode::Char('y'), KeyModifiers::CONTROL), + Some(ApprovalShortcutAction::Approve) + ); + assert_eq!( + approval_shortcut_action(KeyCode::Char('n'), KeyModifiers::CONTROL), + Some(ApprovalShortcutAction::Deny) + ); + } + + #[test] + fn approval_hotkeys_map_ctrl_u() { + assert_eq!( + approval_shortcut_action(KeyCode::Char('u'), KeyModifiers::CONTROL), + Some(ApprovalShortcutAction::AllowPattern) + ); + } + + #[test] + fn approval_shortcuts_stay_active_when_question_modal_open() { + let mode = primary_input_mode(true, true); + assert_eq!(mode, PrimaryInputMode::Approval); + assert_eq!( + approval_shortcut_action(KeyCode::Char('y'), KeyModifiers::CONTROL), + Some(ApprovalShortcutAction::Approve) + ); + assert_eq!( + approval_shortcut_action(KeyCode::Char('n'), KeyModifiers::CONTROL), + Some(ApprovalShortcutAction::Deny) + ); + assert_eq!( + approval_shortcut_action(KeyCode::Char('u'), KeyModifiers::CONTROL), + Some(ApprovalShortcutAction::AllowPattern) + ); + } } diff --git a/crates/cli/tests/cli_commands.rs b/crates/cli/tests/cli_commands.rs index 383f865..74715e1 100644 --- a/crates/cli/tests/cli_commands.rs +++ b/crates/cli/tests/cli_commands.rs @@ -400,6 +400,9 @@ model = "claude-3-7-sonnet-latest" .expect("binary") .current_dir(temp.path()) .env("HOME", temp.path()) + .env_remove("NCA_MODEL") + .env_remove("NCA_DEFAULT_PROVIDER") + .env_remove("ANTHROPIC_MODEL") .arg("doctor") .arg("--json") .assert()