diff --git a/CLAUDE.md b/CLAUDE.md index 60e0e03..e9a2511 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -163,12 +163,17 @@ The agent sees **one namespace**, routed on the first path component. The choke | `shared/{X}/…` | host `{WD}/shared/{X}` (if a member) | `UserFs::host_base_and_tail` | | `projects/{O}/{S}/…` | host `{WD}/projects/{owner_userid}/{S}` (if a member) | `UserFs::host_base_and_tail` | | `~/…`, relative | host `{WD}/homes/{userid}` | `UserFs::host_base_and_tail` | +| any other absolute path (`/tmp/…`, `/etc/…`) | the **container's own** filesystem | `resolve_target` → `container::exec_fs` | -Two views, **one storage**: the fs-tools run **host-side** in the Skald process on `{WD}/homes/{userid}` + `{WD}/shared/{X}`; `execute_cmd` runs **inside the container** (`docker exec -w skald-{userid} sh -c …`, via `ExecuteCmd::run_with`) on the same paths bind-mounted (`homes/{userid}`→`/root`, `shared/{X}`→`/root/shared/{X}`, read-only when `can_write=0`). A file written in the container appears to the host fs-tools and vice versa. +Two views, **one storage**: for the mounted subtree the fs-tools run **host-side** in the Skald process on `{WD}/homes/{userid}` + `{WD}/shared/{X}`; `execute_cmd` runs **inside the container** (`docker exec -w skald-{userid} sh -c …`, via `ExecuteCmd::run_with`) on the same paths bind-mounted (`homes/{userid}`→`/root`, `shared/{X}`→`/root/shared/{X}`, read-only when `can_write=0`). A file written in the container appears to the host fs-tools and vice versa. + +**The security boundary is the container, not the mounted subtree — the mount is the *fast* path, not the only one.** An agent already reaches every corner of its container through `execute_cmd`, which runs there with passwordless `sudo`; fs-tools that stopped at the mounts were not protecting anything, they were offering a poorer view of the same sandbox, and the model answered that by shelling out (the observed failure: `read_file /tmp/cv.txt` → *"path escapes your workspace"* → the agent re-read it with `cat`). So `resolve_target` routes a physical path to one of two backings. An **absolute** path is container vocabulary — it is what `execute_cmd` prints — so it is reverse-mapped through `UserFs::container_to_agent` first: landing on a mount takes the host path (**`/root/x` *is* `~/x`**, which the tools used to reject outright, since `PathBuf::join` with an absolute tail silently discards the base and the result then failed the prefix check); landing nowhere means it exists only in the container, and `container::exec_fs` acts there over `docker exec` (paths passed **positionally** as `$1`, so a path containing `$(…)` is data, not syntax). Membership is not bypassed: `/root/shared/{X}` for a non-member still resolves to the same error as `shared/{X}`. + +**One implementation per tool, not two.** Every single-file fs-tool already funnels through the same shape — resolve, then run a sync `execute` over one absolute host path — so the container branch is a **shuttle** (`fs::Shuttle`, behind `fs::run_physical`): pull the file out of the container, run the *unchanged* tool on the copy, push it back if the content changed (compared by bytes, not mtime, whose one-second resolution would miss a fast edit). Nothing about a tool's messages, diffs or pure transforms is duplicated. A missing remote file is deliberately **not** pre-created — `write_file` reports "Created" vs "Overwrote" from whether the path existed, and a placeholder would make every creation lie. Three tools opt out of the shuttle because a single file is the wrong unit: `list_files` lists in place via `exec_fs::list` (`find -printf`; `line_count` is omitted, since counting lines would turn a listing into a `docker exec` per file), `read_file` reads container paths as text (a shuttled copy is gone by the time the projection would inline a `MediaRef`, so media stays a mount-only feature), and `grep_files` **refuses** container paths with a pointer to `execute_cmd` + `rg` — its regex flavour, glob, windowing and offset would all have to be re-derived from ripgrep's flags, and a grep that answers *almost* the same is worse than one that says where to go. The viewer follows the same routing through `resolve_view_target` (`GET /api/file` and `show_file_to_user` open container paths; served without an ETag, so the editor stays read-only there). **The memory roots are signposted inside the container, not merely absent.** `user-memory/`/`shared-memory/` are virtual, so nothing of them existed on disk — and the nothing was worse than it sounds: `cat user-memory/x.md` returned a bare ENOENT (which reads as *the note is missing*, not *wrong door*), while `mkdir -p user-memory && echo … > user-memory/x.md` **succeeded**, writing a real file into the home that no reader ever visits and that the next `ls` then confirms as if it had worked. Each root is therefore a **read-only bind mount** (`{WD}/.memory-signpost/{root}` → `{container_home}/{root}:ro`, gitignored, rewritten from consts on every `ensure`) holding a README that names the tools. Read-only *as a mount*, not as a mode: the container user has passwordless `sudo`, so a `chmod` would be a suggestion, whereas `:ro` holds — remounting needs `CAP_SYS_ADMIN` (verified: write, `sudo` write, `sudo chmod`, `sudo mount -o remount,rw` and `sudo rm` all fail). A README rather than an empty dir because `Permission denied` is an error, not an instruction — models answer it by reaching for `sudo`; the README puts the correction in the directory the failing command just named. These mounts are deliberately **not** in `UserFs`: they back no agent path and the host-side fs-tools must never resolve into them. They are the **fourth self-heal axis** in `reusable()` (`signposts_mounted`) rather than an `IMAGE_TAG` bump, since the image is unchanged and a bump would make every box rebuild it to fix a mount. The matching half is in `classify_memory`, which now strips the home spellings (`./`, `~/`, `/root/`) before matching the root — without it `~/user-memory/x.md` missed the match, fell through to the disk router, and became exactly the invisible physical file the signpost exists to prevent. -**Containment** (`resolve_host_path`): every physical fs-tool op canonicalizes the resolved path (following symlinks) and prefix-checks it against its mount base, **fail-closed**. Since the same tree is writable from inside the container, a symlink planted there that points outside the home/shared root is caught here — the host-side tool never escapes the user's workspace. `grep_files` stays disk-only (regex ≠ FTS; memory → `memory_search`) but resolves its root the same way. `execute_cmd`'s `workdir` is an agent path mapped to its container path via `UserFs::to_container`. +**Containment** (`resolve_host_path`) is unchanged and still guards **the host branch**: every path that lands on a mount is canonicalized (following symlinks) and prefix-checked against its mount base, **fail-closed**. That check is what it always was — the defence against a symlink planted from inside the container pointing at the **host's** `/etc`, which the host-side tool would otherwise follow off the box. Opening the container branch does not weaken it: that branch never touches the host filesystem, so there is no host to escape from, and the check keeps applying to everything mounted. `grep_files` stays disk-only (regex ≠ FTS; memory → `memory_search`) but resolves its root the same way. `execute_cmd`'s `workdir` is an agent path mapped to its container path via `UserFs::to_container`. The threading: `UserContext.fs` (built by `container::build_user_fs` at login, snapshotting shared memberships) → `ChatSessionManager` → `ChatSessionHandler.fs` → `ToolContext.fs`. **Admin CRUD is wired** (`src/frontend/api/shared_folders.rs` — `GET/POST /api/shared-folders`, `PATCH/DELETE /api/shared-folders/{id}`, `POST`/`DELETE .../members[/{user_id}]`; UI `shared-folders.js`): a create/describe/delete + per-member `can_write` surface, and each mutation emits `SystemEvent::UserMountsChanged`, on which the lifecycle reconciler runs `Skald::refresh_user_mounts` — rebuilding the affected user's fs + container mounts **in place**, so a membership change lands without a re-login (blueprint §6's "admin CRUD" + "membership refresh without re-login" TODOs, now closed; it still settles at next login/boot if the live remount fails). `execute_cmd` /stop is robust: the command runs under `setsid -w` in its own process-group (leader pid recorded in a container pidfile), and a `KillReaper` drop-guard reaps that group on /stop **or** timeout via a detached `docker exec` that walks `/proc` and kills members by **positive pid** (the container's dash mishandles `kill -`); the pidfile is passed positionally (`$1`), and the container's `--init` (tini) reaps the killed processes so no zombies accumulate. **Per-user MCP connectors now run inside this container** (§7) — the container infra enabled it; see the MCP connectors section. @@ -366,6 +371,14 @@ Three consequences worth not re-deriving: The future automatic pass should trigger off the **resolved model's own context window**, not a hand-tuned `threshold_tokens` that has no idea which model is answering. +### The system prefix is frozen per conversation + +Same economics, other end of the request. `AgentSystemContext::system_context` is called **once per round**, and it reassembled `base` from disk and SQLite every time — so an agent writing `user-memory/index.md` in round 3 made round 4, seconds later and with the cache certainly warm, a full miss. Since `base` is the head of every provider's cache key, that is the most expensive string in the request to touch. `loop_adapters/prefix_cache.rs::PrefixCache` builds it once per `(conversation, agent)` — the agent is in the key because a sub-agent shares its parent's conversation but has a prompt of its own — and holds it on `UserLoopRuntime`, so it outlives the turn. + +The refresh rule is the only one that is free: **rebuild once the conversation has been idle longer than a provider's cache could survive** (`PREFIX_TTL`, 20 min). The clock is therefore *idle time of this conversation*, not time since a file changed, and reading restarts it — every `get` is a request about to go out. The asymmetry that sets the constant: below a provider's window you pay misses that buy nothing, above it you only pay freshness. + +**Writes are deliberately not reacted to, and there is no bus variant for this.** When the agent itself edits an injected file the content is already in the context — its tool call and result sit two messages downstream — so refreshing would repeat what the model just said. A write from *elsewhere* (the same user's Telegram session, a cron job, another member editing `shared-memory/`) is genuinely invisible until the TTL: that is the case where an immediate rebuild costs the most, since a conversation that would notice is by definition a warm one, and the cheaper freshness path already exists — the agent can `read_file`, and a tool result *appends*, which invalidates nothing. The injection header says so in words. Cross-user invalidation would need a `SystemEventBus` variant plus a subscriber per user (the writer lives in a different `UserContext`); it is future work, and this type's key is the seam for it. Note `base` is frozen **whole**: freezing the memory files while letting `__USER_PROFILE__` move would invalidate just as much. The cost is that an `AGENT.md` edit lands at the next rebuild rather than the next round. + ## Approval gate The rule engine `ApprovalManager::check` returns `Allow`/`Deny`/`Require` per tool call (default rules seeded on first boot; the catch-all `* require @999999` gates anything not explicitly allowed — e.g. `execute_cmd`, `execute_task`, writes outside whitelisted paths). It is wired to the loop as `loop_adapters/gate.rs::ApprovalGate` (`agent_loop::gate::Gate`). A `Require` registers a `oneshot` in the in-memory `pending` map keyed by `request_id` and emits an approval event over WS. diff --git a/crates/skald-core/src/container/exec_fs.rs b/crates/skald-core/src/container/exec_fs.rs new file mode 100644 index 0000000..2e7fa2b --- /dev/null +++ b/crates/skald-core/src/container/exec_fs.rs @@ -0,0 +1,141 @@ +//! Filesystem primitives that act **inside** a user's container, for the paths +//! their bind mounts do not cover (`/tmp`, `/etc`, an installed package's files…). +//! +//! The security boundary is the container, not the bind-mounted subtree: an agent +//! already has unrestricted reach in there through `execute_cmd`, which runs with +//! passwordless `sudo`. Tools that stopped at the mounts were therefore not +//! protecting anything — they offered a poorer view of the same sandbox, and the +//! model routinely worked around them by shelling out. These primitives close +//! that gap so the fs-tools see what the shell sees. +//! +//! What does *not* change is host containment. A path that lands on a mount keeps +//! the host fast path and its canonicalize-and-prefix-check, which is what stops a +//! symlink planted in the container from resolving against the **host's** `/etc`. +//! Nothing here ever touches the host filesystem, so there is no host to escape +//! from on this side. +//! +//! Paths are passed to `sh` **positionally** (`$1`), never interpolated into the +//! script, so a path containing quotes or `$(…)` is data and not shell syntax — +//! the same rule `execute_cmd` already follows for its pidfile. + +use std::path::Path; +use std::process::Stdio; + +use anyhow::{Context, Result, bail}; +use tokio::io::AsyncWriteExt; + +/// Runs a shell snippet inside `container` with `args` bound to `$1`, `$2`, … +/// Returns raw stdout — callers that expect text decode it themselves, so a +/// binary `cat` is not mangled on the way through. +async fn sh(container: &str, script: &str, args: &[&str]) -> Result> { + let mut argv: Vec<&str> = vec!["exec", container, "sh", "-c", script, "_"]; + argv.extend_from_slice(args); + + let out = tokio::process::Command::new("docker") + .args(&argv) + .stdin(Stdio::null()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .output() + .await + .context("failed to spawn `docker` (is the Docker CLI installed?)")?; + + if out.status.success() { + Ok(out.stdout) + } else { + let err = String::from_utf8_lossy(&out.stderr); + bail!("{}", err.trim()); + } +} + +/// True when the snippet exits 0 — for the `test`-style probes, where a non-zero +/// exit is the answer rather than a failure. +async fn sh_ok(container: &str, script: &str, args: &[&str]) -> bool { + sh(container, script, args).await.is_ok() +} + +/// Reads a file from inside the container. +pub async fn read(container: &str, path: &Path) -> Result> { + let p = path.to_string_lossy(); + sh(container, r#"cat -- "$1""#, &[&p]) + .await + .with_context(|| format!("Cannot read file: {p}")) +} + +/// Writes a file inside the container, creating its parent directories. The +/// bytes travel on stdin rather than inside the script, so content is never +/// shell-parsed and size is bounded by the pipe, not by `ARG_MAX`. +pub async fn write(container: &str, path: &Path, bytes: &[u8]) -> Result<()> { + let p = path.to_string_lossy(); + let mut child = tokio::process::Command::new("docker") + .args([ + "exec", "-i", container, "sh", "-c", + r#"mkdir -p -- "$(dirname -- "$1")" && cat > "$1""#, "_", &p, + ]) + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .spawn() + .context("failed to spawn `docker`")?; + + child + .stdin + .take() + .context("docker exec produced no stdin")? + .write_all(bytes) + .await + .with_context(|| format!("Failed to write: {p}"))?; + + let out = child.wait_with_output().await.context("docker exec failed")?; + if !out.status.success() { + bail!("Failed to write {p}: {}", String::from_utf8_lossy(&out.stderr).trim()); + } + Ok(()) +} + +pub async fn exists(container: &str, path: &Path) -> bool { + sh_ok(container, r#"test -e "$1""#, &[&path.to_string_lossy()]).await +} + +pub async fn is_dir(container: &str, path: &Path) -> bool { + sh_ok(container, r#"test -d "$1""#, &[&path.to_string_lossy()]).await +} + +/// One entry of a container directory listing. +pub struct Entry { + pub name: String, + pub is_dir: bool, + pub size: u64, +} + +/// Lists a directory inside the container, `depth` levels deep (1 = immediate +/// children). Emits `type\tsize\tpath` per line via `find`, which is in the image +/// and needs no parsing of `ls`'s locale-dependent output. +pub async fn list(container: &str, path: &Path, depth: usize) -> Result> { + let p = path.to_string_lossy(); + let d = depth.max(1).to_string(); + let raw = sh( + container, + r#"find "$1" -mindepth 1 -maxdepth "$2" -printf '%y\t%s\t%p\n' 2>/dev/null || true"#, + &[&p, &d], + ) + .await + .with_context(|| format!("Cannot list directory: {p}"))?; + + let text = String::from_utf8_lossy(&raw); + let prefix = format!("{}/", p.trim_end_matches('/')); + let mut out = Vec::new(); + for line in text.lines() { + let mut f = line.splitn(3, '\t'); + let (Some(kind), Some(size), Some(full)) = (f.next(), f.next(), f.next()) else { + continue; + }; + out.push(Entry { + name: full.strip_prefix(&prefix).unwrap_or(full).to_string(), + is_dir: kind == "d", + size: size.parse().unwrap_or(0), + }); + } + out.sort_by(|a, b| b.is_dir.cmp(&a.is_dir).then_with(|| a.name.cmp(&b.name))); + Ok(out) +} diff --git a/crates/skald-core/src/container/mod.rs b/crates/skald-core/src/container/mod.rs index 58a23b8..8fce4fa 100644 --- a/crates/skald-core/src/container/mod.rs +++ b/crates/skald-core/src/container/mod.rs @@ -18,6 +18,8 @@ //! a container can be recreated from the image at any time; boot reconciliation //! relies on that. +pub mod exec_fs; + use std::path::{Path, PathBuf}; use std::process::Stdio; use std::sync::Arc; diff --git a/crates/skald-core/src/loop_adapters/catalog.rs b/crates/skald-core/src/loop_adapters/catalog.rs index 7be3b81..d4b110e 100644 --- a/crates/skald-core/src/loop_adapters/catalog.rs +++ b/crates/skald-core/src/loop_adapters/catalog.rs @@ -28,6 +28,7 @@ use crate::llm::logging::RequestLogTarget; use crate::loop_adapters::activation::SkaldToolActivator; use crate::loop_adapters::builtins::{SkaldAskUserTool, SkaldHumanChannel}; use crate::loop_adapters::history::SqliteHistory; +use crate::loop_adapters::prefix_cache::PrefixCache; use crate::loop_adapters::runtime::LoopConfig; use crate::loop_adapters::scope::TurnScope; use crate::loop_adapters::selector::SkaldSelector; @@ -51,6 +52,9 @@ pub struct SkaldAgentCatalog { /// The swappable fs cell, so a §6 remount reaches sub-agents too. fs: SharedFs, config: LoopConfig, + /// Shared with the parent runtime: a child's prefix is keyed by its own + /// agent id, so it never collides with the conversation's root frame. + prefix_cache: Arc, /// The delegate tool, injected post-construction. **Weak** on purpose: the /// delegate holds the catalog, so an `Arc` here would be a cycle that never /// frees (and this graph lives as long as the user). @@ -70,6 +74,7 @@ impl SkaldAgentCatalog { registry: Arc, fs: SharedFs, config: LoopConfig, + prefix_cache: Arc, ) -> Self { let core_tools = registry.all_tools(); Self { @@ -84,6 +89,7 @@ impl SkaldAgentCatalog { core_tools, fs, config, + prefix_cache, delegate: RwLock::new(Weak::new()), } } @@ -134,6 +140,7 @@ impl AgentCatalog for SkaldAgentCatalog { // writes the SAME one as its parent. scratchpad_sid: scope.scratchpad_sid, datetime: self.config.datetime.clone(), + prefix_cache: self.prefix_cache.clone(), }); // The child's def list: parent's base minus root-only minus the diff --git a/crates/skald-core/src/loop_adapters/mod.rs b/crates/skald-core/src/loop_adapters/mod.rs index 161320e..4661a14 100644 --- a/crates/skald-core/src/loop_adapters/mod.rs +++ b/crates/skald-core/src/loop_adapters/mod.rs @@ -21,6 +21,9 @@ //! tool result — the library does the shaping. //! - [`async_task`] — `execute_task mode=async` as a durable cron job, and the //! delivery of its result back into the parent conversation (§7.2). +//! - [`prefix_cache`] — the cacheable half of the system prompt, frozen per +//! conversation so a mid-turn memory write does not invalidate the provider's +//! prompt cache. //! - [`runtime::UserLoopRuntime`] — the one `LoopManager` per user (D12) these //! are all assembled into, plus the per-turn parameters. @@ -33,6 +36,7 @@ pub mod history; pub mod hooks; pub mod live_input; pub mod media_source; +pub mod prefix_cache; pub mod preview; #[cfg(test)] mod projection_snapshots; diff --git a/crates/skald-core/src/loop_adapters/prefix_cache.rs b/crates/skald-core/src/loop_adapters/prefix_cache.rs new file mode 100644 index 0000000..c14c306 --- /dev/null +++ b/crates/skald-core/src/loop_adapters/prefix_cache.rs @@ -0,0 +1,167 @@ +//! `PrefixCache` — the system prefix, frozen for as long as a provider's prompt +//! cache could still be holding it. +//! +//! Every provider that caches keys on the longest common *prefix*, and the +//! system prompt is the first thing in it — so rebuilding it changes the whole +//! request. That is what used to happen on every round: +//! [`AgentSystemContext`](super::system::AgentSystemContext) reassembles `base` +//! from disk and SQLite each time it is asked, so an agent writing to +//! `user-memory/index.md` in round 3 turned round 4, seconds later and with the +//! cache certainly warm, into a full miss. +//! +//! So the prefix is built once and kept. The refresh rule is the one that costs +//! nothing: **rebuild only once the conversation has been idle long enough that +//! the provider's cache is gone anyway.** Below that window a rebuild buys +//! freshness at the price of a guaranteed miss; above it, it is free. Hence the +//! clock is *idle time of this conversation*, not time since some file changed +//! — and every call to [`PrefixCache::get`] is a request about to go out, which +//! is why reading restarts the window. +//! +//! **Writes are deliberately not reacted to.** When the agent itself edits an +//! injected file the new content is already in the context — the tool call and +//! its result sit two messages downstream — so refreshing the prefix would only +//! repeat what the model just said. A write from *elsewhere* (the same user's +//! Telegram session, a cron job, another member editing `shared-memory/`) is +//! genuinely invisible until the TTL, and that is the trade taken knowingly: it +//! is precisely the case where an immediate rebuild costs the most, since a +//! conversation that would notice is by definition a warm one. The freshness +//! path already exists and is cheaper — the agent can `read_file`, and a tool +//! result *appends*, which never invalidates anything. The injection header in +//! `system.rs` tells it so. +//! +//! Reacting to another user's write would need a `SystemEventBus` variant and a +//! subscriber per user, since the writer lives in a different `UserContext`. +//! That is future work; the seam for it is this type's key. + +use std::collections::HashMap; +use std::sync::Mutex; +use std::time::{Duration, Instant}; + +use agent_loop::ids::ConversationId; + +/// How long a prefix survives without its conversation calling a model. +/// +/// The asymmetry that sets it: going *below* a provider's cache window pays +/// misses that buy nothing, while going above only costs freshness we have +/// already decided we do not need. Anthropic's `ephemeral` blocks live 5 +/// minutes; OpenAI's automatic prefix cache is fuzzier and can last longer. +pub const PREFIX_TTL: Duration = Duration::from_secs(20 * 60); + +/// A conversation plus the agent running in it. Both are needed: a sub-agent +/// shares its parent's conversation but has its own prompt, and therefore its +/// own cache prefix. +type Key = (ConversationId, String); + +struct Entry { + base: String, + last_used: Instant, +} + +/// One user's frozen prefixes. Lives on `UserLoopRuntime`, so it spans every +/// turn of every conversation that user has open. +pub struct PrefixCache { + ttl: Duration, + entries: Mutex>, +} + +impl PrefixCache { + pub fn new() -> Self { + Self::with_ttl(PREFIX_TTL) + } + + /// A cache with a custom idle window — tests, and the knob a config key + /// would turn if one is ever wanted. + pub fn with_ttl(ttl: Duration) -> Self { + Self { ttl, entries: Mutex::new(HashMap::new()) } + } + + /// The prefix for this turn, if one was built recently enough. Restarts the + /// idle window on a hit. + pub fn get(&self, key: &Key) -> Option { + let mut entries = self.entries.lock().unwrap(); + let entry = entries.get_mut(key)?; + if entry.last_used.elapsed() >= self.ttl { + entries.remove(key); + return None; + } + entry.last_used = Instant::now(); + Some(entry.base.clone()) + } + + /// Stores a freshly built prefix, dropping whatever has gone idle — which is + /// what keeps the map bounded without an eviction policy to remember. It is + /// also what collects the one-shot conversations (system-agent passes, + /// ephemeral turns) that would otherwise each leave an entry behind. + /// + /// Two rounds racing on the same key build twice and the last one wins. That + /// is why the build happens *outside* this type: holding the lock across it + /// would serialise every turn of every conversation behind one mutex, to + /// save a duplicated string. + pub fn put(&self, key: Key, base: String) { + let mut entries = self.entries.lock().unwrap(); + entries.retain(|_, e| e.last_used.elapsed() < self.ttl); + entries.insert(key, Entry { base, last_used: Instant::now() }); + } +} + +impl Default for PrefixCache { + fn default() -> Self { + Self::new() + } +} + +#[cfg(test)] +mod tests { + use super::*; + + fn key(conv: &str, agent: &str) -> Key { + (ConversationId::new(conv), agent.to_string()) + } + + #[test] + fn a_stored_prefix_is_served_back() { + let cache = PrefixCache::new(); + cache.put(key("session:1", "assistant"), "PROMPT".into()); + assert_eq!(cache.get(&key("session:1", "assistant")).as_deref(), Some("PROMPT")); + } + + #[test] + fn an_idle_prefix_is_a_miss() { + let cache = PrefixCache::with_ttl(Duration::from_millis(20)); + cache.put(key("session:1", "assistant"), "PROMPT".into()); + std::thread::sleep(Duration::from_millis(40)); + assert_eq!(cache.get(&key("session:1", "assistant")), None); + } + + /// The whole point of the idle clock: a conversation that keeps talking + /// keeps its prefix, however long it runs. + #[test] + fn using_a_prefix_restarts_the_idle_window() { + let cache = PrefixCache::with_ttl(Duration::from_millis(60)); + cache.put(key("session:1", "assistant"), "PROMPT".into()); + for _ in 0..4 { + std::thread::sleep(Duration::from_millis(20)); + assert!(cache.get(&key("session:1", "assistant")).is_some()); + } + } + + /// A sub-agent shares the conversation and must not be served its parent's + /// prompt. + #[test] + fn the_agent_is_part_of_the_key() { + let cache = PrefixCache::new(); + cache.put(key("session:1", "assistant"), "PARENT".into()); + cache.put(key("session:1", "researcher"), "CHILD".into()); + assert_eq!(cache.get(&key("session:1", "assistant")).as_deref(), Some("PARENT")); + assert_eq!(cache.get(&key("session:1", "researcher")).as_deref(), Some("CHILD")); + } + + #[test] + fn storing_drops_the_entries_that_went_idle() { + let cache = PrefixCache::with_ttl(Duration::from_millis(20)); + cache.put(key("session:1", "assistant"), "OLD".into()); + std::thread::sleep(Duration::from_millis(40)); + cache.put(key("session:2", "assistant"), "NEW".into()); + assert_eq!(cache.entries.lock().unwrap().len(), 1); + } +} diff --git a/crates/skald-core/src/loop_adapters/runtime.rs b/crates/skald-core/src/loop_adapters/runtime.rs index a430207..d6951c3 100644 --- a/crates/skald-core/src/loop_adapters/runtime.rs +++ b/crates/skald-core/src/loop_adapters/runtime.rs @@ -42,6 +42,7 @@ use crate::loop_adapters::gate::ApprovalGate; use crate::loop_adapters::history::SqliteHistory; use crate::loop_adapters::hooks::{DtlReanchorHook, SkaldWritePreviewHook}; use crate::loop_adapters::live_input::PendingLiveInput; +use crate::loop_adapters::prefix_cache::PrefixCache; use crate::loop_adapters::preview::PreviewContext; use crate::loop_adapters::projection_cfg::skald_assembler; use crate::loop_adapters::scope::TurnScope; @@ -92,6 +93,9 @@ pub struct UserLoopRuntime { clarification: Arc, tool_discovery: Arc, config: LoopConfig, + /// The user's frozen system prefixes, shared with the agent catalog so a + /// sub-agent's own prefix is cached alongside its parent's. + prefix_cache: Arc, } /// What a turn contributes on top of the runtime. @@ -154,6 +158,10 @@ impl UserLoopRuntime { .build()?, ); + // One per user, living as long as this runtime: a conversation's system + // prefix must outlast its turns for the provider's cache to hold. + let prefix_cache = Arc::new(PrefixCache::new()); + let catalog = Arc::new(SkaldAgentCatalog::new( pool.clone(), shared_pool.clone(), @@ -165,6 +173,7 @@ impl UserLoopRuntime { tools.clone(), fs.clone(), config.clone(), + prefix_cache.clone(), )); // `mode: "async"` runs as a durable cron job; the manager behind it is // set at wiring time (see `CronExecutor`). @@ -197,6 +206,7 @@ impl UserLoopRuntime { clarification, tool_discovery, config, + prefix_cache, })) } @@ -244,6 +254,7 @@ impl UserLoopRuntime { project_root: scope.project_root.clone(), scratchpad_sid: scope.scratchpad_sid, datetime: self.config.datetime.clone(), + prefix_cache: self.prefix_cache.clone(), }); // The agent's own declarations. Loaded once here and used twice below — diff --git a/crates/skald-core/src/loop_adapters/system.rs b/crates/skald-core/src/loop_adapters/system.rs index 3ac20df..0380a65 100644 --- a/crates/skald-core/src/loop_adapters/system.rs +++ b/crates/skald-core/src/loop_adapters/system.rs @@ -16,6 +16,7 @@ use agent_loop::context::{SystemContext, SystemContextSource, TurnInfo}; use sqlx::SqlitePool; use crate::config::DatetimeConfig; +use crate::loop_adapters::prefix_cache::PrefixCache; use crate::mcp::McpProvider; /// Registry of installed skills, relative to Skald's process cwd. Injected @@ -44,18 +45,92 @@ pub struct AgentSystemContext { /// sub-task (the blackboard is shared by every agent of a session). pub scratchpad_sid: i64, pub datetime: DatetimeConfig, + /// The user's frozen prefixes — `base` is assembled once per conversation + /// and reused while its provider cache could still be warm. + pub prefix_cache: Arc, } #[agent_loop::async_trait] impl SystemContextSource for AgentSystemContext { - async fn system_context(&self, _turn: &TurnInfo) -> agent_loop::Result { + async fn system_context(&self, turn: &TurnInfo) -> agent_loop::Result { + // `base` is the head of every provider's cache key, so reassembling it + // between rounds — which is what an agent editing an injected memory + // file used to cause — invalidates the entire request. It is therefore + // built once per conversation and held; see [`super::prefix_cache`]. + let key = (turn.conversation.clone(), self.agent_id.clone()); + let static_content = match self.prefix_cache.get(&key) { + Some(base) => base, + None => { + let base = self.build_base().await?; + self.prefix_cache.put(key, base.clone()); + base + } + }; + + // The scratchpad sits before the conversation: shared by every agent of + // the session, and re-read every turn (it changes, so it is its own + // message rather than part of the cached prefix). + let extra_static = self.scratchpad_block().await?.into_iter().collect(); + + // The fresh layers, in the order the model reads them. + let mut dynamic_tail: Vec = Vec::new(); + dynamic_tail.extend(self.extra_dynamic.clone()); + dynamic_tail.extend(self.datetime_block()); + + Ok(SystemContext { + base: static_content, + extra_static, + dynamic_tail, + tail_reminder: self.tail_reminder.clone(), + }) + } +} + +/// OS description (type + version), computed once. +fn os_description() -> &'static str { + static OS: std::sync::OnceLock = std::sync::OnceLock::new(); + OS.get_or_init(|| os_info::get().to_string()) +} + +/// Formats an instant to hour precision: `Sunday 2026-08-02 17:00 +02:00`. +/// +/// Minutes and seconds are dropped by the format string itself, so the +/// truncation always happens in the zone being displayed. The weekday is part +/// of the format on purpose — see [`AgentSystemContext::datetime_block`]. +fn render_hour(dt: chrono::DateTime) -> String +where + Tz::Offset: std::fmt::Display, +{ + dt.format("%A %Y-%m-%d %H:00 %:z").to_string() +} + +/// System IANA timezone name, computed once. +fn system_timezone() -> Option<&'static str> { + static TZ: std::sync::OnceLock> = std::sync::OnceLock::new(); + TZ.get_or_init(|| iana_time_zone::get_timezone().ok()).as_deref() +} + +impl AgentSystemContext { + /// Assembles the cacheable prefix: the agent's prompt, its injected memory, + /// the skills index, the interface extras and every substitution. + /// + /// Every layer here is frozen together, because the unit a provider caches + /// is the finished string — freezing the memory files while letting + /// `__USER_PROFILE__` move would invalidate just as much. The cost is that + /// an `AGENT.md` edit is picked up at the next rebuild rather than the next + /// round, which matters only while writing prompts. + async fn build_base(&self) -> agent_loop::Result { let mut static_content = crate::agents::load_prompt(&self.agent_id)?; let meta = crate::agents::load_meta(&self.agent_id)?; if !meta.inject_memory.is_empty() { static_content.push_str( "\n\n---\nThe following memory files have been loaded automatically. \ - You can edit them with `edit_file` or `write_file` using the path shown.\n" + You can edit them with `edit_file` or `write_file` using the path shown.\n\ + Their contents are a snapshot taken earlier in this conversation. Your own \ + edits are already reflected in what you have seen since; but if it matters \ + that a file is current — a shared note another member may have changed in \ + the meantime — read it again before relying on it.\n" ); for mem_path in &meta.inject_memory { let (content, display) = self.load_inject_memory(mem_path).await; @@ -116,52 +191,9 @@ impl SystemContextSource for AgentSystemContext { } } - static_content = resolve_harness_tag(static_content); - - // The scratchpad sits before the conversation: shared by every agent of - // the session, and re-read every turn (it changes, so it is its own - // message rather than part of the cached prefix). - let extra_static = self.scratchpad_block().await?.into_iter().collect(); - - // The fresh layers, in the order the model reads them. - let mut dynamic_tail: Vec = Vec::new(); - dynamic_tail.extend(self.extra_dynamic.clone()); - dynamic_tail.extend(self.datetime_block()); - - Ok(SystemContext { - base: static_content, - extra_static, - dynamic_tail, - tail_reminder: self.tail_reminder.clone(), - }) + Ok(resolve_harness_tag(static_content)) } -} -/// OS description (type + version), computed once. -fn os_description() -> &'static str { - static OS: std::sync::OnceLock = std::sync::OnceLock::new(); - OS.get_or_init(|| os_info::get().to_string()) -} - -/// Formats an instant to hour precision: `Sunday 2026-08-02 17:00 +02:00`. -/// -/// Minutes and seconds are dropped by the format string itself, so the -/// truncation always happens in the zone being displayed. The weekday is part -/// of the format on purpose — see [`AgentSystemContext::datetime_block`]. -fn render_hour(dt: chrono::DateTime) -> String -where - Tz::Offset: std::fmt::Display, -{ - dt.format("%A %Y-%m-%d %H:00 %:z").to_string() -} - -/// System IANA timezone name, computed once. -fn system_timezone() -> Option<&'static str> { - static TZ: std::sync::OnceLock> = std::sync::OnceLock::new(); - TZ.get_or_init(|| iana_time_zone::get_timezone().ok()).as_deref() -} - -impl AgentSystemContext { /// The session scratchpad as an XML block, or `None` when empty. async fn scratchpad_block(&self) -> agent_loop::Result> { let notes = crate::db::scratchpad::for_session(&self.pool, self.scratchpad_sid).await?; diff --git a/crates/skald-core/src/loop_adapters/testkit.rs b/crates/skald-core/src/loop_adapters/testkit.rs index 7f25969..829728b 100644 --- a/crates/skald-core/src/loop_adapters/testkit.rs +++ b/crates/skald-core/src/loop_adapters/testkit.rs @@ -224,6 +224,9 @@ pub async fn project(db: &Db, agent: &AgentFixture, case: &Case) -> Vec { project_root: None, scratchpad_sid: 1, datetime: datetime(), + // A cache of its own per projection: each case must see a freshly + // assembled prefix, never one another case left behind. + prefix_cache: Arc::new(crate::loop_adapters::prefix_cache::PrefixCache::new()), }; let system = system_source .system_context(&TurnInfo { diff --git a/crates/skald-core/src/tools/fs/append_file.rs b/crates/skald-core/src/tools/fs/append_file.rs index f34063b..ec65141 100644 --- a/crates/skald-core/src/tools/fs/append_file.rs +++ b/crates/skald-core/src/tools/fs/append_file.rs @@ -82,10 +82,7 @@ impl Tool for AppendFile { fn run_with<'a>(&'a self, ctx: &ToolContext, args: Value) -> Box { let path = super::path_arg(&args).unwrap_or_default(); let Some(m) = classify_memory(&path) else { - return match super::rewrite_to_host(&ctx.fs, &path, args) { - Ok(args) => self.run(args), - Err(e) => super::error_exec(e.to_string()), - }; + return super::run_physical(self, &ctx.fs, &path, args); }; let pool = match m.scope { MemScope::User => Arc::clone(&ctx.pool), diff --git a/crates/skald-core/src/tools/fs/edit_file.rs b/crates/skald-core/src/tools/fs/edit_file.rs index b680e6b..bb83a63 100644 --- a/crates/skald-core/src/tools/fs/edit_file.rs +++ b/crates/skald-core/src/tools/fs/edit_file.rs @@ -159,10 +159,7 @@ impl Tool for EditFile { fn run_with<'a>(&'a self, ctx: &ToolContext, args: Value) -> Box { let path = super::path_arg(&args).unwrap_or_default(); let Some(m) = classify_memory(&path) else { - return match super::rewrite_to_host(&ctx.fs, &path, args) { - Ok(args) => self.run(args), - Err(e) => super::error_exec(e.to_string()), - }; + return super::run_physical(self, &ctx.fs, &path, args); }; let pool = match m.scope { MemScope::User => Arc::clone(&ctx.pool), diff --git a/crates/skald-core/src/tools/fs/grep_files.rs b/crates/skald-core/src/tools/fs/grep_files.rs index bce565d..ea1c0b6 100644 --- a/crates/skald-core/src/tools/fs/grep_files.rs +++ b/crates/skald-core/src/tools/fs/grep_files.rs @@ -100,9 +100,19 @@ impl Tool for GrepFiles { user-memory/ or shared-memory/".to_string(), ); } - match super::rewrite_to_host(&ctx.fs, &path, args) { - Ok(args) => self.run(args), - Err(e) => super::error_exec(e.to_string()), + // Searching a *tree* is the one thing neither the shuttle (one file) nor a + // faithful `rg` translation can serve: this tool's regex flavour, glob, + // windowing and offset would all have to be re-derived from ripgrep's + // flags and output, and a grep that answers *almost* the same is worse + // than one that says where to go. + match super::resolve_target(&ctx.fs, &path) { + Ok(super::FsTarget::Host(host)) => self.run(super::point_at(&path, &host, args)), + Ok(super::FsTarget::Container { .. }) => super::error_exec(format!( + "grep_files only searches your mounted folders (~, shared/, projects/, docs/); \ + {path} lives only inside your container. Search it with execute_cmd, e.g. \ + `rg -n 'pattern' {path}` (ripgrep is installed)." + )), + Err(e) => super::error_exec(e.to_string()), } } diff --git a/crates/skald-core/src/tools/fs/insert_at_line.rs b/crates/skald-core/src/tools/fs/insert_at_line.rs index 77d76d7..4d17c5f 100644 --- a/crates/skald-core/src/tools/fs/insert_at_line.rs +++ b/crates/skald-core/src/tools/fs/insert_at_line.rs @@ -96,10 +96,7 @@ impl Tool for InsertAtLine { fn run_with<'a>(&'a self, ctx: &ToolContext, args: Value) -> Box { let path = super::path_arg(&args).unwrap_or_default(); let Some(m) = classify_memory(&path) else { - return match super::rewrite_to_host(&ctx.fs, &path, args) { - Ok(args) => self.run(args), - Err(e) => super::error_exec(e.to_string()), - }; + return super::run_physical(self, &ctx.fs, &path, args); }; let pool = match m.scope { MemScope::User => Arc::clone(&ctx.pool), diff --git a/crates/skald-core/src/tools/fs/list_files.rs b/crates/skald-core/src/tools/fs/list_files.rs index 80b654a..07878f6 100644 --- a/crates/skald-core/src/tools/fs/list_files.rs +++ b/crates/skald-core/src/tools/fs/list_files.rs @@ -79,10 +79,24 @@ impl Tool for ListFiles { let path = args["path"].as_str().unwrap_or("").to_string(); let with_metadata = args["with_metadata"].as_bool().unwrap_or(false); let Some(m) = classify_memory(&path) else { - return match super::rewrite_to_host(&ctx.fs, &path, args) { - Ok(args) => self.run(args), - Err(e) => super::error_exec(e.to_string()), + // A directory is the one shape the shuttle cannot serve — it moves a + // single file — so a container-only path is listed in place. + let host = match super::resolve_target(&ctx.fs, &path) { + Ok(super::FsTarget::Host(h)) => h, + Ok(super::FsTarget::Container { container, path: dir }) => { + let depth = args["depth"].as_u64().unwrap_or(3) as usize; + let dirs_only = args["dirs_only"].as_bool().unwrap_or(false); + return Box::new(SimpleExecution::new(Box::pin(async move { + let entries = + crate::container::exec_fs::list(&container, &dir, depth).await?; + Ok(ToolResult::Text(render_container_listing( + entries, dirs_only, with_metadata, + )?)) + }))); + } + Err(e) => return super::error_exec(e.to_string()), }; + return self.run(super::point_at(&path, &host, args)); }; let pool = match m.scope { MemScope::User => Arc::clone(&ctx.pool), @@ -139,6 +153,35 @@ impl Tool for ListFiles { } } +/// Renders a container listing into the same JSON the on-disk walk emits: a bare +/// array of relative paths, or `FileEntry` rows under `with_metadata`. +/// +/// `line_count` is always absent here. Counting lines means reading the file, and +/// reading a container file means one `docker exec` each — a listing must not +/// quietly become a full read of the tree. +fn render_container_listing( + entries: Vec, + dirs_only: bool, + with_metadata: bool, +) -> Result { + let mut rows: Vec = entries + .into_iter() + .filter(|e| if dirs_only { e.is_dir } else { !e.is_dir }) + .filter(|e| !e.name.split('/').any(|c| SKIP_DIRS.contains(&c))) + .collect(); + rows.sort_by(|a, b| a.name.cmp(&b.name)); + + if !with_metadata { + let paths: Vec = rows.into_iter().map(|e| e.name).collect(); + return Ok(serde_json::to_string(&paths)?); + } + let entries: Vec = rows + .into_iter() + .map(|e| FileEntry { path: e.name, line_count: None, size: Some(human_size(e.size)) }) + .collect(); + Ok(serde_json::to_string(&entries)?) +} + /// A `with_metadata` listing row. Field order (declaration order) is the wire /// order; `line_count` and `size` are omitted when unavailable. #[derive(serde::Serialize)] diff --git a/crates/skald-core/src/tools/fs/mod.rs b/crates/skald-core/src/tools/fs/mod.rs index 12ce6bd..0df0e11 100644 --- a/crates/skald-core/src/tools/fs/mod.rs +++ b/crates/skald-core/src/tools/fs/mod.rs @@ -248,28 +248,194 @@ pub(crate) fn resolve_host_path(fs: &UserFs, agent_path: &str) -> Result Result<(PathBuf, String)> { + match resolve_view_target(fs, input)? { + (FsTarget::Host(host), agent) => Ok((host, agent)), + (FsTarget::Container { .. }, agent) => anyhow::bail!( + "{agent} lives only inside your container; this action needs a file in your \ + mounted folders (~, shared/, projects/)" + ), + } +} + +/// The view-surface twin of [`resolve_target`]: normalizes an incoming path to the +/// agent vocabulary and says where it lives, so the viewer can open a +/// container-only path (`/tmp/report.pdf`) the same way the agent reads it. +/// +/// A container-only path has no agent-vocabulary spelling — it *is* its own +/// display form, which is also what `show_file_to_user` echoes back. +pub fn resolve_view_target(fs: &UserFs, input: &str) -> Result<(FsTarget, String)> { if classify_memory(input).is_some() { anyhow::bail!("memory notes can't be opened in the file viewer: {input}"); } + let raw = Path::new(input); + if raw.is_absolute() && fs.container_to_agent(raw).is_none() { + let path = lexical_normalize(raw); + let display = path.to_string_lossy().into_owned(); + return Ok(( + FsTarget::Container { container: fs.container_name.clone(), path }, + display, + )); + } let agent = fs.to_agent_display(input) .ok_or_else(|| anyhow::anyhow!("path is outside your workspace: {input}"))?; let host = resolve_host_path(fs, &agent)?; - Ok((host, agent)) + Ok((FsTarget::Host(host), agent)) } -/// Rewrites the `path` argument of a physical fs-tool call to the resolved absolute -/// host path, so the on-disk `execute` (which takes absolute paths as-is) acts on -/// the caller's per-user workspace rather than the process working directory. +/// Points the `path` argument of a physical fs-tool call at the absolute path the +/// on-disk `execute` should act on — the caller's host workspace, or the shuttled +/// copy of a container file — instead of the process working directory. /// /// The caller's agent-visible path is stashed under [`DISPLAY_PATH_KEY`] so `execute` /// can show it in its messages — the model must never see the host path. This key is /// never persisted: tool args are logged from `call.arguments` *before* `run_with` /// rewrites them, and tool results are plain strings. -pub(crate) fn rewrite_to_host(fs: &UserFs, agent_path: &str, mut args: Value) -> Result { - let host = resolve_host_path(fs, agent_path)?; +pub(crate) fn point_at(agent_path: &str, abs: &Path, mut args: Value) -> Value { args[DISPLAY_PATH_KEY] = Value::String(agent_path.to_string()); - args["path"] = Value::String(host.to_string_lossy().into_owned()); - Ok(args) + args["path"] = Value::String(abs.to_string_lossy().into_owned()); + args +} + +// ── Container routing ───────────────────────────────────────────────────────── +// +// The security boundary is the **container**, not the bind-mounted subtree. An +// agent already reaches every corner of its container through `execute_cmd`, +// which runs there with passwordless `sudo`; fs-tools that stopped at the mounts +// were not protecting anything, they were showing a poorer view of the same +// sandbox — and the model routinely answered that by shelling out instead. +// +// So a physical path resolves to one of two backings, and the mount is the *fast* +// one rather than the only one. Host containment is untouched: it is what stops a +// symlink planted in the container from resolving against the **host's** `/etc`, +// and it still guards every path that lands on a mount. The container branch +// never touches the host filesystem, so it has no host to escape from. + +/// Where a physical (non-memory) agent path actually lives. +pub enum FsTarget { + /// A bind-mounted path: host and container see the same bytes, so the tool + /// acts on the host directly — no `docker exec`, and full media support. + Host(PathBuf), + /// A container-only path (`/tmp`, `/etc`, a package's files…), reachable + /// solely through the container's own filesystem. + Container { container: String, path: PathBuf }, +} + +/// Resolves a physical agent path to its backing. +/// +/// An absolute path is **container vocabulary** — it is what `execute_cmd` prints +/// and what the agent's shell sees — so it is reverse-mapped first. Landing on a +/// mount takes the host path (`/root/x` *is* `~/x`, which the tools used to +/// reject); landing nowhere means the path exists only inside the container. +pub(crate) fn resolve_target(fs: &UserFs, agent_path: &str) -> Result { + if Path::new(agent_path).is_absolute() { + return match fs.container_to_agent(Path::new(agent_path)) { + Some(mapped) => Ok(FsTarget::Host(resolve_host_path(fs, &mapped)?)), + None => Ok(FsTarget::Container { + container: fs.container_name.clone(), + path: lexical_normalize(Path::new(agent_path)), + }), + }; + } + Ok(FsTarget::Host(resolve_host_path(fs, agent_path)?)) +} + +/// A container file materialised host-side for the duration of one tool call. +/// +/// Every single-file fs-tool funnels through the same shape — resolve, then run a +/// sync `execute` that reads and writes one absolute host path. Rather than give +/// each of them a second implementation, with a second set of messages, diffs and +/// edge cases to keep in step, the file is pulled out of the container, the +/// **unchanged** tool runs on the copy, and the copy goes back if it changed. +/// +/// A missing remote file is deliberately not pre-created: `write_file` says +/// "Created" or "Overwrote" based on whether the path existed, and a placeholder +/// would make every creation report the wrong one. +pub(crate) struct Shuttle { + dir: PathBuf, + local: PathBuf, + container: String, + remote: PathBuf, + /// The bytes as pulled, or `None` when the remote file did not exist. + /// Compared by content rather than mtime, whose one-second resolution on some + /// filesystems would miss a fast edit. + before: Option>, +} + +impl Shuttle { + async fn pull(container: &str, remote: &Path) -> Result { + let dir = std::env::temp_dir().join(format!("skald-fs-{}", uuid::Uuid::new_v4())); + tokio::fs::create_dir_all(&dir).await + .with_context(|| format!("Failed to create temporary directory: {}", dir.display()))?; + // Keep the basename: tools and media sniffing key off the extension. + let name = remote.file_name().unwrap_or_else(|| std::ffi::OsStr::new("file")); + let local = dir.join(name); + + let before = if crate::container::exec_fs::exists(container, remote).await { + let bytes = crate::container::exec_fs::read(container, remote).await?; + tokio::fs::write(&local, &bytes).await + .with_context(|| format!("Failed to stage {}", remote.display()))?; + Some(bytes) + } else { + None + }; + + Ok(Self { + dir, + local, + container: container.to_string(), + remote: remote.to_path_buf(), + before, + }) + } + + /// Pushes the copy back when the tool created or changed it, then cleans up. + async fn finish(self) -> Result<()> { + let after = tokio::fs::read(&self.local).await.ok(); + let changed = match (&self.before, &after) { + (before, Some(a)) => before.as_ref() != Some(a), + (_, None) => false, + }; + let pushed = if changed { + crate::container::exec_fs::write(&self.container, &self.remote, after.as_deref().unwrap_or(&[])).await + } else { + Ok(()) + }; + let _ = tokio::fs::remove_dir_all(&self.dir).await; + pushed + } +} + +/// The single entry point a single-file fs-tool uses for a physical path: resolve +/// the backing, then run the tool's own `execute` against it — directly on the +/// host, or on a shuttled copy for a container-only path. +pub(crate) fn run_physical<'a, T>( + tool: &'a T, + fs: &UserFs, + agent_path: &str, + args: Value, +) -> Box +where + T: crate::tools::Tool + ?Sized, +{ + match resolve_target(fs, agent_path) { + Err(e) => error_exec(e.to_string()), + Ok(FsTarget::Host(host)) => tool.run(point_at(agent_path, &host, args)), + Ok(FsTarget::Container { container, path }) => { + let display = agent_path.to_string(); + Box::new(SimpleExecution::new(Box::pin(async move { + let shuttle = Shuttle::pull(&container, &path).await?; + let args = point_at(&display, &shuttle.local, args); + // The tool's own error wins over a push failure: the push is + // bookkeeping, the tool's message is what the model must read. + let out = tool.execute_typed(args).await; + let pushed = shuttle.finish().await; + match out { + Ok(v) => pushed.map(|()| v), + Err(e) => Err(e), + } + }))) + } + } } /// Private stash key for the agent-visible path, set by [`rewrite_to_host`] alongside @@ -414,6 +580,73 @@ mod tests { let _ = std::fs::remove_dir_all(&root); } + /// Container routing: a container-absolute path that names a **mount** takes + /// the host fast path (`/root/x` *is* `~/x` — it used to be rejected as an + /// escape, because the absolute tail replaced the home base on `join`), while + /// one that names nothing mounted resolves inside the container. + #[test] + fn absolute_paths_route_to_the_mount_or_to_the_container() { + use core_api::user_fs::SharedMount; + + let root = std::env::temp_dir().join(format!("skald-fstgt-{}", std::process::id())); + let home = root.join("homes").join("u1"); + let shared = root.join("shared").join("family"); + let _ = std::fs::remove_dir_all(&root); + std::fs::create_dir_all(&home).unwrap(); + std::fs::create_dir_all(&shared).unwrap(); + + let fs = UserFs::new( + "u1", + home.clone(), + "skald-u1", + PathBuf::from("/root"), + vec![SharedMount { + name: "family".into(), + host: shared.clone(), + container: PathBuf::from("/root/shared/family"), + can_write: true, + }], + vec![], + None, + ); + + let home_canon = canonicalize_for_policy(&home.to_string_lossy(), Path::new("/")); + let shared_canon = canonicalize_for_policy(&shared.to_string_lossy(), Path::new("/")); + + let host = |p: &str| match resolve_target(&fs, p).unwrap() { + FsTarget::Host(h) => h, + FsTarget::Container { path, .. } => panic!("{p} routed to the container as {path:?}"), + }; + let container = |p: &str| match resolve_target(&fs, p).unwrap() { + FsTarget::Container { container, path } => (container, path), + FsTarget::Host(h) => panic!("{p} routed to the host as {h:?}"), + }; + + // The container spelling of the home and of a shared mount reach the same + // host files as the agent vocabulary does. + assert_eq!(host("/root/notes.md"), host("~/notes.md")); + assert!(path_under(&host("/root/notes.md"), &home_canon)); + assert_eq!( + host("/root/shared/family/list.md"), + host("shared/family/list.md") + ); + assert!(path_under(&host("/root/shared/family/list.md"), &shared_canon)); + + // Nothing mounted there → the container's own filesystem. + let (name, path) = container("/tmp/cv.txt"); + assert_eq!(name, "skald-u1"); + assert_eq!(path, PathBuf::from("/tmp/cv.txt")); + assert_eq!(container("/etc/os-release").1, PathBuf::from("/etc/os-release")); + // `..` is collapsed before it can name a parent of anything. + assert_eq!(container("/tmp/../tmp/x").1, PathBuf::from("/tmp/x")); + + // A shared folder the user does not belong to stays an error — the + // container spelling must not become a way around membership. + assert!(resolve_target(&fs, "/root/shared/secret/x.md").is_err()); + + let _ = std::fs::remove_dir_all(&root); + } + #[test] fn classify_memory_splits_root_from_key() { let u = classify_memory("user-memory/notes/x.md").unwrap(); diff --git a/crates/skald-core/src/tools/fs/read_file.rs b/crates/skald-core/src/tools/fs/read_file.rs index e0a8caa..135bc2d 100644 --- a/crates/skald-core/src/tools/fs/read_file.rs +++ b/crates/skald-core/src/tools/fs/read_file.rs @@ -141,8 +141,15 @@ impl Tool for ReadFile { let Some(m) = classify_memory(&path) else { // Physical path: resolve + containment-check up front (so an escape // fails immediately), then read inside the work future. - let host = match super::resolve_host_path(&ctx.fs, &path) { - Ok(h) => h, + let host = match super::resolve_target(&ctx.fs, &path) { + Ok(super::FsTarget::Host(h)) => h, + // A container-only path has no host file to sniff or to hand on + // as a `MediaRef` (the shuttled copy is gone by the time the + // projection would inline it), so it is read as text — same + // windowing, same line numbers, via the shared `execute`. + Ok(super::FsTarget::Container { .. }) => { + return super::run_physical(self, &ctx.fs, &path, args); + } Err(e) => return super::error_exec(e.to_string()), }; let start = args["start_line"].as_u64().map(|n| (n as usize).saturating_sub(1)).unwrap_or(0); diff --git a/crates/skald-core/src/tools/fs/replace_lines.rs b/crates/skald-core/src/tools/fs/replace_lines.rs index 5377afe..3968dce 100644 --- a/crates/skald-core/src/tools/fs/replace_lines.rs +++ b/crates/skald-core/src/tools/fs/replace_lines.rs @@ -101,10 +101,7 @@ impl Tool for ReplaceLines { fn run_with<'a>(&'a self, ctx: &ToolContext, args: Value) -> Box { let path = super::path_arg(&args).unwrap_or_default(); let Some(m) = classify_memory(&path) else { - return match super::rewrite_to_host(&ctx.fs, &path, args) { - Ok(args) => self.run(args), - Err(e) => super::error_exec(e.to_string()), - }; + return super::run_physical(self, &ctx.fs, &path, args); }; let pool = match m.scope { MemScope::User => Arc::clone(&ctx.pool), diff --git a/crates/skald-core/src/tools/fs/search_file.rs b/crates/skald-core/src/tools/fs/search_file.rs index 4a58a93..428b719 100644 --- a/crates/skald-core/src/tools/fs/search_file.rs +++ b/crates/skald-core/src/tools/fs/search_file.rs @@ -114,10 +114,7 @@ impl Tool for SearchFile { fn run_with<'a>(&'a self, ctx: &ToolContext, args: Value) -> Box { let path = super::path_arg(&args).unwrap_or_default(); let Some(m) = classify_memory(&path) else { - return match super::rewrite_to_host(&ctx.fs, &path, args) { - Ok(args) => self.run(args), - Err(e) => super::error_exec(e.to_string()), - }; + return super::run_physical(self, &ctx.fs, &path, args); }; let pool = match m.scope { MemScope::User => Arc::clone(&ctx.pool), diff --git a/crates/skald-core/src/tools/fs/write_file.rs b/crates/skald-core/src/tools/fs/write_file.rs index 3edf35c..0dd7f7e 100644 --- a/crates/skald-core/src/tools/fs/write_file.rs +++ b/crates/skald-core/src/tools/fs/write_file.rs @@ -65,10 +65,7 @@ impl Tool for WriteFile { fn run_with<'a>(&'a self, ctx: &ToolContext, args: Value) -> Box { let path = super::path_arg(&args).unwrap_or_default(); let Some(m) = classify_memory(&path) else { - return match super::rewrite_to_host(&ctx.fs, &path, args) { - Ok(args) => self.run(args), - Err(e) => super::error_exec(e.to_string()), - }; + return super::run_physical(self, &ctx.fs, &path, args); }; let pool = match m.scope { MemScope::User => Arc::clone(&ctx.pool), diff --git a/crates/skald-core/src/tools/show_file.rs b/crates/skald-core/src/tools/show_file.rs index 1822fdc..1313f51 100644 --- a/crates/skald-core/src/tools/show_file.rs +++ b/crates/skald-core/src/tools/show_file.rs @@ -112,12 +112,21 @@ pub fn make_tool( // Resolve against the caller's workspace snapshot: gives the host path to // stat and the canonical agent path the viewer will fetch back. let user_fs = fs.load(); - let (abs, display) = fs::resolve_view_path(user_fs.as_ref(), path) + let (target, display) = fs::resolve_view_target(user_fs.as_ref(), path) .map_err(|e| anyhow::anyhow!("show_file_to_user: {e}"))?; - if !abs.exists() { + // A container-only path is statted through the container, the same way + // the viewer will fetch it back. + let (exists, is_dir) = match &target { + fs::FsTarget::Host(abs) => (abs.exists(), abs.is_dir()), + fs::FsTarget::Container { container, path } => ( + crate::container::exec_fs::exists(container, path).await, + crate::container::exec_fs::is_dir(container, path).await, + ), + }; + if !exists { anyhow::bail!("show_file_to_user: file not found: {display}"); } - if abs.is_dir() { + if is_dir { anyhow::bail!("show_file_to_user: '{display}' is a directory, not a file"); } diff --git a/src/frontend/api/files.rs b/src/frontend/api/files.rs index 3094f9c..219ae44 100644 --- a/src/frontend/api/files.rs +++ b/src/frontend/api/files.rs @@ -198,12 +198,38 @@ pub async fn get_file( } let user_fs = ctx.fs.load(); - let (abs, agent) = match fs_tools::resolve_view_path(user_fs.as_ref(), &q.path) { - Ok((abs, agent)) => (abs, agent), - Err(e) => return (StatusCode::BAD_REQUEST, format!("Invalid path: {e}")).into_response(), + let (target, agent) = match fs_tools::resolve_view_target(user_fs.as_ref(), &q.path) { + Ok(resolved) => resolved, + Err(e) => return (StatusCode::BAD_REQUEST, format!("Invalid path: {e}")).into_response(), }; let writable = user_fs.can_write_to(&agent); + // A container-only path (`/tmp/…`) has no host file behind it: the bytes come + // out through the container, so the user sees what the agent read. Served + // read-only — the editor's optimistic locking is an on-disk `mtime`+`len`, + // which has no counterpart here, and without an ETag the frontend keeps the + // file in view mode rather than risking a blind overwrite. + let abs = match target { + fs_tools::FsTarget::Host(abs) => abs, + fs_tools::FsTarget::Container { container, path } => { + return match skald_core::container::exec_fs::read(&container, &path).await { + Ok(bytes) => { + let mut response = bytes.into_response(); + response.headers_mut().insert( + header::CONTENT_TYPE, + HeaderValue::from_static(content_type_for(&q.path)), + ); + if q.force_download { + set_attachment(&mut response, &basename(&q.path)); + } + response + } + Err(_) => (StatusCode::NOT_FOUND, format!("File not found: {}", q.path)) + .into_response(), + }; + } + }; + if q.compile_latex && is_latex(&q.path) { return match state.latex_compiler().compile(&abs).await { Ok(pdf) => {