From 32d6dcc423337c8bb546b70ffe2b0e352297845a Mon Sep 17 00:00:00 2001 From: xavix-yo Date: Tue, 4 Aug 2026 22:33:37 +0100 Subject: [PATCH] fix: route get_ast_outline through the caller's workspace, not the server cwd The tool was a single-user leftover: it only implemented the context-free execute, so a relative path resolved against the server process cwd and projects/{owner}/{slug}/... failed with "Cannot read file" while every other fs tool worked. It now overrides run_with like its siblings: memory paths outline the note from the right pool, physical paths go through the shared UserFs shuttle (home, shared, projects, container), and the agent-visible path is what headers and errors show. Also gains target_path and a workspace-aware path description. --- crates/skald-core/src/skald/bundles.rs | 2 +- crates/skald-core/src/tools/ast_outline.rs | 304 ++++++++++++++++----- 2 files changed, 236 insertions(+), 70 deletions(-) diff --git a/crates/skald-core/src/skald/bundles.rs b/crates/skald-core/src/skald/bundles.rs index 1cd1c2c..68c4226 100644 --- a/crates/skald-core/src/skald/bundles.rs +++ b/crates/skald-core/src/skald/bundles.rs @@ -215,7 +215,7 @@ impl Tools { pub(super) fn build(rt: &Runtime, integrations: &Integrations, tasks: &Tasks, models: &Models) -> Self { let mut tool_registry = ToolRegistry::new(); crate::tools::fs::register_all(&mut tool_registry, Arc::clone(&rt.db)); - tool_registry.register(crate::tools::ast_outline::AstOutline::new()); + tool_registry.register(crate::tools::ast_outline::AstOutline::new(Arc::clone(&rt.db))); tool_registry.register(crate::tools::exec::ExecuteCmd); tool_registry.register(crate::tools::read_notification::ReadNotification); // Unified listing / toggling across plugins, cron (+ agents and MCP for diff --git a/crates/skald-core/src/tools/ast_outline.rs b/crates/skald-core/src/tools/ast_outline.rs index e0800b0..495b5a2 100644 --- a/crates/skald-core/src/tools/ast_outline.rs +++ b/crates/skald-core/src/tools/ast_outline.rs @@ -1,13 +1,23 @@ -use anyhow::Result; +use std::sync::Arc; + +use anyhow::{Context, Result}; use serde_json::{Value, json}; +use sqlx::SqlitePool; -use crate::tools::{Tool, ToolDescriptionLength, truncate_label, MAX_LABEL_SHORT}; -use crate::tools::fs::read_to_string; +use crate::tools::{ + SimpleExecution, Tool, ToolContext, ToolDescriptionLength, ToolExecution, ToolResult, + truncate_label, MAX_LABEL_SHORT, +}; +use crate::tools::fs::{self, MemScope}; -pub struct AstOutline; +pub struct AstOutline { + /// The `shared-memory` (system) pool. `user-memory` resolves per call from the + /// `ToolContext`; only the shared store is a global singleton captured here. + shared_pool: Arc, +} impl AstOutline { - pub fn new() -> Self { Self } + pub fn new(shared_pool: Arc) -> Self { Self { shared_pool } } } impl Tool for AstOutline { @@ -23,6 +33,7 @@ impl Tool for AstOutline { of the full definition — same column format as read_file, so you pass START/END straight to \ read_file's start_line/end_line to read just the definition you care about. \ Typical flow: outline first, then read only the ranges you need — far cheaper than reading the whole file. \ + Paths under user-memory/ (private) or shared-memory/ (shared) outline a note from your memory instead of disk. \ Supported: .rs .py .js .mjs .ts .tsx .go .java .c .h .cpp .cc .hpp .swift .lua .rb .sh .ex .exs \ .kt .json .toml .yaml .yml .html .css .md .sql" } @@ -33,57 +44,93 @@ impl Tool for AstOutline { "properties": { "path": { "type": "string", - "description": "Path to the source file. Relative to project root or absolute." + "description": "Path to the source file. Relative to `~` (your home) — `shared/{name}/…` and `projects/{owner}/{slug}/…` mounts included — or a container-absolute path (e.g. /tmp/x.py)." } }, "required": ["path"] }) } + fn target_path(&self, args: &Value) -> Option { + fs::path_arg(args) + } + fn describe(&self, args: &Value, _length: ToolDescriptionLength) -> String { let path = args["path"].as_str().unwrap_or("?"); truncate_label(&format!("outline `{path}`"), MAX_LABEL_SHORT) } + /// Routes `user-memory/…` / `shared-memory/…` to the note store; every other + /// path is physical and resolves against the caller's workspace (home, + /// shared folders, projects) or, for a container-only absolute path, their + /// container — via the shared fs shuttle. + fn run_with<'a>(&'a self, ctx: &ToolContext, args: Value) -> Box { + let path = fs::path_arg(&args).unwrap_or_default(); + let Some(m) = fs::classify_memory(&path) else { + return fs::run_physical(self, &ctx.fs, &path, args); + }; + let pool = match m.scope { + MemScope::User => Arc::clone(&ctx.pool), + MemScope::Shared => Arc::clone(&self.shared_pool), + }; + let rel = m.rel; + + Box::new(SimpleExecution::new(Box::pin(async move { + let Some(doc) = crate::db::memory_docs::get(&pool, &rel).await? else { + anyhow::bail!("No note at {path}"); + }; + Ok(ToolResult::Text(outline_source(&path, &doc.content)?)) + }))) + } + fn execute(&self, args: Value) -> Result { let path = args["path"].as_str() .ok_or_else(|| anyhow::anyhow!("Missing required argument: path"))?; + let display = fs::display_path_arg(&args); + let abs = fs::resolve(path)?; + let source = std::fs::read_to_string(&abs) + .with_context(|| format!("Cannot read file: {display}"))?; + outline_source(display, &source) + } +} - let ext = std::path::Path::new(path) - .extension() - .and_then(|e| e.to_str()) - .unwrap_or(""); +/// Outlines `source`, dispatching on `display`'s extension. `display` is the +/// agent-visible path used in the header and in errors — never a host path. +fn outline_source(display: &str, source: &str) -> Result { + let ext = std::path::Path::new(display) + .extension() + .and_then(|e| e.to_str()) + .unwrap_or(""); - match ext { - "rs" => outline_rust(path), - "py" => outline_ts(path, ts_python(), "Python"), - "js" | "mjs" => outline_ts(path, ts_javascript(), "JavaScript"), - "ts" => outline_ts(path, ts_typescript(false), "TypeScript"), - "tsx" => outline_ts(path, ts_typescript(true), "TypeScript/TSX"), - "go" => outline_ts(path, ts_go(), "Go"), - "java" => outline_ts(path, ts_java(), "Java"), - "c" | "h" => outline_ts(path, ts_c(), "C"), - "cpp" | "cc" | "hpp" | "cxx"=> outline_ts(path, ts_cpp(), "C++"), - "swift" => outline_ts(path, ts_swift(), "Swift"), - "lua" => outline_ts(path, ts_lua(), "Lua"), - "rb" => outline_ts(path, ts_ruby(), "Ruby"), - "sh" | "bash" => outline_ts(path, ts_bash(), "Bash"), - "ex" | "exs" => outline_ts(path, ts_elixir(), "Elixir"), - "json" => outline_json(path), - "yaml" | "yml" => outline_ts(path, ts_yaml(), "YAML"), - "html" => outline_ts(path, ts_html(), "HTML"), - "css" => outline_ts(path, ts_css(), "CSS"), - // text-based fallbacks for crates incompatible with tree-sitter 0.26 - "kt" | "kts" => outline_kotlin(path), - "toml" => outline_toml(path), - "sql" => outline_sql(path), - "md" | "markdown" => outline_markdown(path), - other => Ok(format!( - "Language not supported for AST outline: .{other}\n\ - Supported: .rs .py .js .ts .tsx .go .java .c .cpp .swift .lua .rb .sh .ex \ - .kt .json .toml .yaml .html .css .md .sql" - )), - } + match ext { + "rs" => outline_rust(display, source), + "py" => outline_ts(display, source, ts_python(), "Python"), + "js" | "mjs" => outline_ts(display, source, ts_javascript(), "JavaScript"), + "ts" => outline_ts(display, source, ts_typescript(false), "TypeScript"), + "tsx" => outline_ts(display, source, ts_typescript(true), "TypeScript/TSX"), + "go" => outline_ts(display, source, ts_go(), "Go"), + "java" => outline_ts(display, source, ts_java(), "Java"), + "c" | "h" => outline_ts(display, source, ts_c(), "C"), + "cpp" | "cc" | "hpp" | "cxx"=> outline_ts(display, source, ts_cpp(), "C++"), + "swift" => outline_ts(display, source, ts_swift(), "Swift"), + "lua" => outline_ts(display, source, ts_lua(), "Lua"), + "rb" => outline_ts(display, source, ts_ruby(), "Ruby"), + "sh" | "bash" => outline_ts(display, source, ts_bash(), "Bash"), + "ex" | "exs" => outline_ts(display, source, ts_elixir(), "Elixir"), + "json" => outline_json(display, source), + "yaml" | "yml" => outline_ts(display, source, ts_yaml(), "YAML"), + "html" => outline_ts(display, source, ts_html(), "HTML"), + "css" => outline_ts(display, source, ts_css(), "CSS"), + // text-based fallbacks for crates incompatible with tree-sitter 0.26 + "kt" | "kts" => outline_kotlin(display, source), + "toml" => outline_toml(display, source), + "sql" => outline_sql(display, source), + "md" | "markdown" => outline_markdown(display, source), + other => Ok(format!( + "Language not supported for AST outline: .{other}\n\ + Supported: .rs .py .js .ts .tsx .go .java .c .cpp .swift .lua .rb .sh .ex \ + .kt .json .toml .yaml .html .css .md .sql" + )), } } @@ -96,17 +143,16 @@ struct LangConfig { container_kinds: &'static [&'static str], } -fn outline_ts(path: &str, cfg: LangConfig, lang_label: &str) -> Result { - let source = read_to_string(path)?; +fn outline_ts(display: &str, source: &str, cfg: LangConfig, lang_label: &str) -> Result { let mut parser = tree_sitter::Parser::new(); parser.set_language(&cfg.language) .map_err(|e| anyhow::anyhow!("tree-sitter language load error: {e}"))?; let tree = parser.parse(source.as_bytes(), None) - .ok_or_else(|| anyhow::anyhow!("tree-sitter parse returned None for {path}"))?; + .ok_or_else(|| anyhow::anyhow!("tree-sitter parse returned None for {display}"))?; - let mut out = format!("--- {lang_label} outline: {path} ---\n\n"); - collect_nodes(tree.root_node(), &source, &cfg, 0, &mut out); + let mut out = format!("--- {lang_label} outline: {display} ---\n\n"); + collect_nodes(tree.root_node(), source, &cfg, 0, &mut out); Ok(out) } @@ -335,19 +381,18 @@ fn ts_css() -> LangConfig { const JSON_VALUE_KINDS: &[&str] = &["object", "array", "string", "number", "true", "false", "null"]; -fn outline_json(path: &str) -> Result { - let source = read_to_string(path)?; +fn outline_json(display: &str, source: &str) -> Result { let mut parser = tree_sitter::Parser::new(); let language: tree_sitter::Language = tree_sitter_json::LANGUAGE.into(); parser.set_language(&language) .map_err(|e| anyhow::anyhow!("tree-sitter language load error: {e}"))?; let tree = parser.parse(source.as_bytes(), None) - .ok_or_else(|| anyhow::anyhow!("tree-sitter parse returned None for {path}"))?; + .ok_or_else(|| anyhow::anyhow!("tree-sitter parse returned None for {display}"))?; - let mut out = format!("--- JSON outline: {path} ---\n\n"); + let mut out = format!("--- JSON outline: {display} ---\n\n"); // document → single top-level value (object or array). if let Some(top) = json_first_value(tree.root_node()) { - json_walk(top, &source, 0, &mut out); + json_walk(top, source, 0, &mut out); } Ok(out) } @@ -467,13 +512,12 @@ fn json_key_text(key: tree_sitter::Node, source: &str) -> String { // ── text-based fallbacks (crates incompatible with tree-sitter 0.26) ─────── -fn outline_kotlin(path: &str) -> Result { - let source = read_to_string(path)?; - let mut out = format!("--- Kotlin outline: {path} ---\n\n"); +fn outline_kotlin(display: &str, source: &str) -> Result { + let mut out = format!("--- Kotlin outline: {display} ---\n\n"); let re = regex::Regex::new( r"(?m)^\s*((?:(?:public|private|protected|internal|open|abstract|override|suspend|inline|data|sealed|companion|object)\s+)*(?:fun|class|object|interface|enum\s+class|data\s+class|sealed\s+class)\s+[\w<>?]+)" ).unwrap(); - for cap in re.captures_iter(&source) { + for cap in re.captures_iter(source) { let start = 1 + source[..cap.get(0).unwrap().start()].matches('\n').count(); let end = 1 + source[..cap.get(0).unwrap().end()].matches('\n').count(); out.push_str(&format!("{start:>4}-{end:>4} | {}\n", cap[1].trim())); @@ -481,9 +525,8 @@ fn outline_kotlin(path: &str) -> Result { Ok(out) } -fn outline_toml(path: &str) -> Result { - let source = read_to_string(path)?; - let mut out = format!("--- TOML outline: {path} ---\n\n"); +fn outline_toml(display: &str, source: &str) -> Result { + let mut out = format!("--- TOML outline: {display} ---\n\n"); for (i, line) in source.lines().enumerate() { let t = line.trim(); if (t.starts_with("[[") && t.ends_with("]]")) @@ -496,13 +539,12 @@ fn outline_toml(path: &str) -> Result { Ok(out) } -fn outline_sql(path: &str) -> Result { - let source = read_to_string(path)?; - let mut out = format!("--- SQL outline: {path} ---\n\n"); +fn outline_sql(display: &str, source: &str) -> Result { + let mut out = format!("--- SQL outline: {display} ---\n\n"); let re = regex::Regex::new( r#"(?im)^\s*(CREATE\s+(?:OR\s+REPLACE\s+)?(?:TABLE|VIEW|INDEX|UNIQUE\s+INDEX|FUNCTION|PROCEDURE|TRIGGER|SCHEMA|SEQUENCE|TYPE)\s+(?:IF\s+NOT\s+EXISTS\s+)?[\w."]+)"# ).unwrap(); - for cap in re.captures_iter(&source) { + for cap in re.captures_iter(source) { let start = 1 + source[..cap.get(0).unwrap().start()].matches('\n').count(); let end = 1 + source[..cap.get(0).unwrap().end()].matches('\n').count(); out.push_str(&format!("{start:>4}-{end:>4} | {}\n", cap[1].trim())); @@ -510,9 +552,8 @@ fn outline_sql(path: &str) -> Result { Ok(out) } -fn outline_markdown(path: &str) -> Result { - let source = read_to_string(path)?; - let mut out = format!("--- Markdown outline: {path} ---\n\n"); +fn outline_markdown(display: &str, source: &str) -> Result { + let mut out = format!("--- Markdown outline: {display} ---\n\n"); for (i, line) in source.lines().enumerate() { if line.starts_with('#') { let n = i + 1; @@ -524,15 +565,14 @@ fn outline_markdown(path: &str) -> Result { // ── Rust outline (syn-based) ─────────────────────────────────────────────── -fn outline_rust(path: &str) -> Result { +fn outline_rust(display: &str, source: &str) -> Result { use syn::{File, Item, ImplItem, TraitItem}; use syn::spanned::Spanned; - let content = read_to_string(path)?; - let file: File = syn::parse_file(&content) - .map_err(|e| anyhow::anyhow!("Parse error in {path}: {e}"))?; + let file: File = syn::parse_file(source) + .map_err(|e| anyhow::anyhow!("Parse error in {display}: {e}"))?; - let mut out = format!("--- Rust outline: {path} ---\n\n"); + let mut out = format!("--- Rust outline: {display} ---\n\n"); for item in &file.items { match item { @@ -647,3 +687,129 @@ fn fmt_line(start: usize, end: usize, s: &str, indent: usize) -> String { let prefix = " ".repeat(indent); format!("{start:>4}-{end:>4} | {prefix}{}\n", s.trim()) } + +#[cfg(test)] +mod tests { + use super::*; + use std::path::PathBuf; + + use serde_json::json; + + use core_api::user_fs::{ProjectMount, UserFs}; + + use crate::tools::ExecutionOutcome; + + /// A throwaway owner-schema pool (as `Arc`, ready for a `ToolContext`), plus + /// its dir for cleanup. `tag` + a counter keep parallel tests off the same file. + async fn store(tag: &str) -> (Arc, PathBuf) { + use std::sync::atomic::{AtomicU64, Ordering}; + static SEQ: AtomicU64 = AtomicU64::new(0); + let n = SEQ.fetch_add(1, Ordering::Relaxed); + let dir = std::env::temp_dir().join(format!("skald-ast-{}-{tag}-{n}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + let pool = crate::db::create_user_pool(&dir.join("owner.db"), None).await.unwrap(); + (Arc::new(pool), dir) + } + + /// Drives the tool through the context-aware path and returns its text result. + async fn drive(tool: &AstOutline, ctx: &ToolContext, args: Value) -> Result { + match tool.run_with(ctx, args).wait().await { + ExecutionOutcome::Completed(r) => Ok(r.to_wire()), + ExecutionOutcome::Failed(e) => Err(e), + ExecutionOutcome::Cancelled => Err("cancelled".into()), + } + } + + /// Physical paths resolve against the caller's `UserFs` — home-relative for + /// `~/…` and bare paths, the project mount for `projects/{owner}/{slug}/…` — + /// never against the server process cwd. Regression for the single-user + /// leftover that made `get_ast_outline projects/…/x.py` fail with + /// "Cannot read file" while every other fs tool worked. + #[tokio::test] + async fn outline_routes_home_and_project_paths_through_user_fs() { + let (shared, sdir) = store("phys-shared").await; + let (user, udir) = store("phys-user").await; + + let root = std::env::temp_dir().join(format!("skald-astphys-{}", uuid::Uuid::new_v4())); + let home = root.join("homes").join("u1"); + let project = root.join("projects").join("owner-id").join("budget"); + std::fs::create_dir_all(&home).unwrap(); + std::fs::create_dir_all(&project).unwrap(); + let py = "def hello(name):\n return f\"hi {name}\"\n"; + std::fs::write(home.join("x.py"), py).unwrap(); + std::fs::write(project.join("y.py"), py).unwrap(); + + let fs = Arc::new(UserFs::new( + "u1", home.clone(), "skald-u1", PathBuf::from("/root"), vec![], + vec![ProjectMount { + owner_username: "alice".into(), + slug: "budget".into(), + host: project.clone(), + container: PathBuf::from("/root/projects/alice/budget"), + can_write: false, + }], + None, + )); + let ctx = ToolContext { session_id: 1, user_id: "u1".into(), pool: Arc::clone(&user), fs, mcp: None }; + let tool = AstOutline::new(Arc::clone(&shared)); + + // `/homes/u1` only ever appears in the resolved host path, never in the + // agent namespace — a robust, OS-independent leak detector. + let leak_marker = "/homes/u1"; + + // Home: both the `~/` spelling and a bare relative path. + for p in ["~/x.py", "x.py"] { + let out = drive(&tool, &ctx, json!({"path": p})).await.unwrap(); + assert!(out.contains("function_definition: hello"), "{p}: {out}"); + assert!(out.contains("outline: ~/x.py") || out.contains(&format!("outline: {p}")), "{p}: {out}"); + assert!(!out.contains(leak_marker), "host path leaked for {p}: {out}"); + } + + // A project mount the caller belongs to. + let out = drive(&tool, &ctx, json!({"path": "projects/alice/budget/y.py"})).await.unwrap(); + assert!(out.contains("Python outline: projects/alice/budget/y.py"), "{out}"); + assert!(out.contains("function_definition: hello"), "{out}"); + assert!(!out.contains(leak_marker), "host path leaked: {out}"); + + // A project the caller cannot reach is an error, and a missing file + // names the agent path — never the host path. + assert!(drive(&tool, &ctx, json!({"path": "projects/bob/budget/y.py"})).await.is_err()); + let err = drive(&tool, &ctx, json!({"path": "~/nope.py"})).await.unwrap_err(); + assert!(err.contains("~/nope.py"), "{err}"); + assert!(!err.contains(leak_marker), "host path leaked in error: {err}"); + + let _ = std::fs::remove_dir_all(&root); + let _ = std::fs::remove_dir_all(&udir); + let _ = std::fs::remove_dir_all(&sdir); + } + + /// Memory paths outline the note from the right store (user vs shared), and + /// a missing note errors instead of falling through to the disk router. + #[tokio::test] + async fn outline_reads_memory_notes_from_the_right_store() { + let (shared, sdir) = store("mem-shared").await; + let (user, udir) = store("mem-user").await; + + crate::db::memory_docs::upsert(&user, "notes.md", "# Private\n\ntext\n## Sub\n").await.unwrap(); + crate::db::memory_docs::upsert(&shared, "house.md", "# Shared\n").await.unwrap(); + + let fs = Arc::new(UserFs::new( + "u1", PathBuf::from("/tmp"), "skald-u1", PathBuf::from("/root"), vec![], vec![], None, + )); + let ctx = ToolContext { session_id: 1, user_id: "u1".into(), pool: Arc::clone(&user), fs, mcp: None }; + let tool = AstOutline::new(Arc::clone(&shared)); + + let out = drive(&tool, &ctx, json!({"path": "user-memory/notes.md"})).await.unwrap(); + assert!(out.contains("Markdown outline: user-memory/notes.md"), "{out}"); + assert!(out.contains("# Private") && out.contains("## Sub"), "{out}"); + + let out = drive(&tool, &ctx, json!({"path": "shared-memory/house.md"})).await.unwrap(); + assert!(out.contains("# Shared"), "{out}"); + + assert!(drive(&tool, &ctx, json!({"path": "user-memory/ghost.md"})).await.is_err()); + + let _ = std::fs::remove_dir_all(&udir); + let _ = std::fs::remove_dir_all(&sdir); + } +}