feat: let the fs-tools reach the whole container, and stop rebuilding the system prefix every round
Nightly Build / build (push) Successful in 7m33s
Nightly Build / build (push) Successful in 7m33s
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.
This commit is contained in:
@@ -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<Vec<u8>> {
|
||||
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<Vec<u8>> {
|
||||
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<Vec<Entry>> {
|
||||
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)
|
||||
}
|
||||
Reference in New Issue
Block a user