From 6cb4ea0ce871e7f8d315a0b5e21db23aa9230ee4 Mon Sep 17 00:00:00 2001 From: xavix-yo Date: Tue, 4 Aug 2026 12:27:08 +0100 Subject: [PATCH] feat: let the fs-tools reach the whole container, and stop rebuilding the system prefix every round MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two changes to what a turn costs and what it can see. ## The system prefix is frozen per conversation `AgentSystemContext::system_context` is called once per round and reassembled `base` from disk and SQLite each time, so an agent writing `user-memory/index.md` in round 3 turned round 4 — seconds later, with the provider cache certainly warm — into a full miss. `base` is the head of every provider's cache key, so it is the most expensive string in the request to touch. `PrefixCache` builds it once per (conversation, agent) and holds it on `UserLoopRuntime`. The refresh rule is the only free one: rebuild once the conversation has been idle longer than a provider's cache could survive (20 min). The clock is idle time of the conversation, not time since a file changed, and reading restarts it — every get is a request about to go out. Writes are deliberately not reacted to. The agent's own edits are already in the context, two messages downstream. A write from elsewhere is invisible until the TTL: that is precisely where an immediate rebuild costs the most, and the cheaper freshness path already exists — a `read_file` result appends, and appending invalidates nothing. The injection header now says so. Also removes 4 DB queries and 2 file reads per round. ## The security boundary is the container, not the mounted subtree `read_file /tmp/cv.txt` answered "path escapes your workspace" and the agent re-read the file with `cat`. It was right to refuse — /tmp exists only inside the container — but the refusal protected nothing: `execute_cmd` already runs there with passwordless sudo. The mount is the fast path, not the perimeter. `resolve_target` now routes an absolute path through `container_to_agent` first. Landing on a mount takes the host path, which also fixes a real bug: `/root/x` IS `~/x`, yet every tool rejected it, because `PathBuf::join` with an absolute tail discards the base and the result then failed the prefix check (`/root/shared/{X}/…` too). Landing nowhere means container-only, served by the new `container::exec_fs` over `docker exec`, with paths passed positionally so a path containing `$(…)` stays data. Membership still holds: `/root/shared/{X}` for a non-member fails exactly as `shared/{X}` does. Single-file tools get this without a second implementation: `fs::Shuttle` pulls the file out, runs the unchanged tool on the copy, and pushes it back if the bytes changed. `list_files` lists in place, `read_file` reads container paths as text (a shuttled copy cannot back a MediaRef), and `grep_files` refuses them with a pointer to `rg` rather than approximating its own semantics. The viewer follows the same routing, so the user can open what the agent read. Host containment is untouched and still guards every mounted path — it is the defence against a symlink planted in the container pointing at the host's /etc, and the container branch never touches the host filesystem at all. Verified end-to-end against a live skald-runtime:v3 container: write/read/edit on /tmp round-trip, /etc/os-release reads, binary and shell-metacharacter paths survive, and /root/notes.md lands in the host home. --- CLAUDE.md | 17 +- crates/skald-core/src/container/exec_fs.rs | 141 ++++++++++ crates/skald-core/src/container/mod.rs | 2 + .../skald-core/src/loop_adapters/catalog.rs | 7 + crates/skald-core/src/loop_adapters/mod.rs | 4 + .../src/loop_adapters/prefix_cache.rs | 167 ++++++++++++ .../skald-core/src/loop_adapters/runtime.rs | 11 + crates/skald-core/src/loop_adapters/system.rs | 124 +++++---- .../skald-core/src/loop_adapters/testkit.rs | 3 + crates/skald-core/src/tools/fs/append_file.rs | 5 +- crates/skald-core/src/tools/fs/edit_file.rs | 5 +- crates/skald-core/src/tools/fs/grep_files.rs | 16 +- .../skald-core/src/tools/fs/insert_at_line.rs | 5 +- crates/skald-core/src/tools/fs/list_files.rs | 49 +++- crates/skald-core/src/tools/fs/mod.rs | 249 +++++++++++++++++- crates/skald-core/src/tools/fs/read_file.rs | 11 +- .../skald-core/src/tools/fs/replace_lines.rs | 5 +- crates/skald-core/src/tools/fs/search_file.rs | 5 +- crates/skald-core/src/tools/fs/write_file.rs | 5 +- crates/skald-core/src/tools/show_file.rs | 15 +- src/frontend/api/files.rs | 32 ++- 21 files changed, 784 insertions(+), 94 deletions(-) create mode 100644 crates/skald-core/src/container/exec_fs.rs create mode 100644 crates/skald-core/src/loop_adapters/prefix_cache.rs 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) => {