diff --git a/.gitignore b/.gitignore index dca9f82..20ed111 100644 --- a/.gitignore +++ b/.gitignore @@ -9,6 +9,9 @@ blueprint/ /database/ # Per-user container home dirs ({WD}/homes/{userid}) — instance data, not source /homes/ +# Read-only memory signposts mounted into every container; regenerated at boot +# from the consts in crates/skald-core/src/container/mod.rs +/.memory-signpost/ # SQLite WAL-mode sidecar files (journal_mode=WAL) *.db-wal *.db-shm diff --git a/CLAUDE.md b/CLAUDE.md index 149c034..60e0e03 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -166,6 +166,8 @@ The agent sees **one namespace**, routed on the first path component. The choke 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. +**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`. 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. diff --git a/agents/common/memory.md b/agents/common/memory.md index 3684e40..1fe3d21 100644 --- a/agents/common/memory.md +++ b/agents/common/memory.md @@ -7,6 +7,12 @@ You have two persistent note stores, kept as Markdown and searchable. **Sessions When unsure where something belongs, prefer `user-memory/`. +## They are not folders on disk + +Both stores are **virtual**: they live in the database, not in the filesystem. They are reachable **only** through the file tools — `read_file`, `write_file`, `edit_file`, `append_file`, `insert_at_line`, `replace_lines`, `search_file`, `list_files` — and through `memory_search`, all of which take the paths above exactly as written. + +Never go through `execute_cmd`. A shell command cannot read a note (`cat user-memory/x.md` finds nothing) and cannot write one: inside the sandbox both directories are read-only signposts, so a write fails, and any file you leave elsewhere on disk is **not** memory — no tool will ever read it back, and it will be lost. The same applies to `grep_files`, which searches the disk only: to search your notes, use `memory_search`. + ## The indexes Each store has an `index.md` — one line per note with a brief summary — and **both are injected into your context automatically** at the start of each session (look for them below): diff --git a/crates/skald-core/src/container/mod.rs b/crates/skald-core/src/container/mod.rs index 08351bd..58a23b8 100644 --- a/crates/skald-core/src/container/mod.rs +++ b/crates/skald-core/src/container/mod.rs @@ -5,7 +5,9 @@ //! user is created and started at application boot; `execute_cmd` and — later — //! the user's stateful MCP servers run inside it, against the user's bind-mounted //! home (`{WD}/homes/{userid}` → `/root`) plus the shared folders they belong to, -//! plus the read-only `{WD}/docs` bundle mounted at `/root/docs` for every user. +//! plus the read-only `{WD}/docs` bundle mounted at `/root/docs` for every user and +//! the read-only memory **signposts** at `/root/{user,shared}-memory` (see +//! [`signpost_mounts`]). //! //! Docker is a **hard requirement**: [`ContainerManager::check_docker`] fails //! construction if the daemon is unreachable, and the shell exits at boot. @@ -16,7 +18,7 @@ //! a container can be recreated from the image at any time; boot reconciliation //! relies on that. -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use std::process::Stdio; use std::sync::Arc; use std::time::Duration; @@ -27,6 +29,7 @@ use sqlx::SqlitePool; use core_api::user_fs::{ProjectMount, SharedMount, UserFs}; use crate::db; +use crate::tools::fs as fs_tools; /// Our runtime image tag. Built once from the embedded [`Dockerfile`]. The version /// suffix is the image cache-buster: [`ContainerManager::ensure_image`] rebuilds only @@ -50,6 +53,9 @@ pub const PROJECTS_DIR: &str = "projects"; /// Subdirectory of the working directory holding the docs bundle, mounted /// read-only into every user's container at `{container_home}/docs`. pub const DOCS_DIR: &str = "docs"; +/// Subdirectory of the working directory holding the memory **signposts** — see +/// [`signpost_mounts`]. Dot-prefixed: it is internal plumbing, not a user folder. +pub const SIGNPOST_DIR: &str = ".memory-signpost"; /// Home mount point inside the container. pub const CONTAINER_HOME: &str = "/root"; /// Grace window `docker stop` gives in-container processes (SIGTERM → SIGKILL) @@ -126,6 +132,111 @@ pub async fn build_user_fs(system: &SqlitePool, user_id: &str) -> Result Ok(UserFs::new(user_id, home_host, container_name(user_id), container_home, shared, projects, docs_host)) } +// ── Memory signposts ────────────────────────────────────────────────────────── +// +// `user-memory/` and `shared-memory/` are **virtual**: the fs-tools classify those +// prefixes and route them to SQLite (`memory_docs`), so nothing of them exists on +// disk. Inside the container that used to mean bash saw nothing at all — and the +// nothing was worse than it sounds. `cat user-memory/x.md` returned a bare ENOENT, +// which tells a model that the note is missing rather than that it used the wrong +// door; and `mkdir -p user-memory && echo … > user-memory/x.md` *succeeded*, +// writing a real file into the home that no reader ever visits (every reader — +// `read_file`, `list_files`, `memory_search`, the lints, the viewer — goes to +// `memory_docs`), which the next `ls` then confirms as if it had worked. +// +// So each root gets a **read-only bind mount** carrying a README that names the +// tools to use instead. Two deliberate choices: +// +// - *Read-only as a mount, not as a mode.* The container user holds passwordless +// `sudo`, so a `chmod 0555` would be a suggestion; a `:ro` bind mount holds, +// because remounting it needs `CAP_SYS_ADMIN` and the container has none. Writes +// fail with EROFS. +// - *A README rather than an empty directory.* `Permission denied` is an error, not +// an instruction — models answer it by reaching for `sudo`. The README puts the +// correction in the same directory the failing command just named, which is the +// one feedback channel that lands in the turn where the mistake happened. +// +// These mounts are **not** part of [`UserFs`]: they back no agent path (the agent +// path `user-memory/…` is the note store) and the host-side fs-tools must never +// resolve into them. They exist only inside the sandbox, which is the only place +// the confusion happens. + +const SIGNPOST_README: &str = "README.md"; + +/// The signpost text for `user-memory/`. Addressed to the agent, in the vocabulary +/// its tools use. +const USER_MEMORY_SIGNPOST: &str = "\ +# This is not a folder + +`user-memory/` is a **virtual note store**, kept in the database, not on disk. This +directory is a signpost and is read-only: shell commands cannot read or write your +memory, and anything you manage to write near here is lost. + +Use the tools instead — they take the same paths: + + read_file path=\"user-memory/notes/x.md\" + write_file path=\"user-memory/notes/x.md\" content=\"…\" + edit_file path=\"user-memory/notes/x.md\" … + list_files path=\"user-memory/\" + memory_search query=\"\" + +`grep_files` does not reach the store either — use `memory_search`. +"; + +/// The signpost text for `shared-memory/`. Same rule; the extra line is the one +/// thing that differs about the shared store. +const SHARED_MEMORY_SIGNPOST: &str = "\ +# This is not a folder + +`shared-memory/` is a **virtual note store** shared with the whole group, kept in the +database, not on disk. This directory is a signpost and is read-only: shell commands +cannot read or write it, and anything you manage to write near here is lost. + +Use the tools instead — they take the same paths: + + read_file path=\"shared-memory/x.md\" + write_file path=\"shared-memory/x.md\" content=\"…\" + edit_file path=\"shared-memory/x.md\" … + list_files path=\"shared-memory/\" + memory_search query=\"\" + +Writing here asks the user to confirm first — that is expected, not an error. +`grep_files` does not reach the store either — use `memory_search`. +"; + +/// Where the two signposts live on the host and where they mount, read-only, in the +/// container. One pair of host directories for the whole instance: the content is +/// identical for every user, and the mount is a sign, not a workspace. +fn signpost_mounts(wd: &Path, container_home: &Path) -> [(PathBuf, PathBuf); 2] { + let root = wd.join(SIGNPOST_DIR); + [ + ( + root.join(fs_tools::USER_MEMORY_ROOT), + container_home.join(fs_tools::USER_MEMORY_ROOT), + ), + ( + root.join(fs_tools::SHARED_MEMORY_ROOT), + container_home.join(fs_tools::SHARED_MEMORY_ROOT), + ), + ] +} + +/// Creates the signpost directories and (re)writes their READMEs. The write is +/// unconditional so an edited text reaches existing installations at the next +/// container `ensure`, with no migration step — it is a few hundred bytes. +fn ensure_signposts(wd: &Path) -> Result<()> { + for ((host, _), body) in signpost_mounts(wd, Path::new(CONTAINER_HOME)) + .iter() + .zip([USER_MEMORY_SIGNPOST, SHARED_MEMORY_SIGNPOST]) + { + std::fs::create_dir_all(host) + .with_context(|| format!("failed to create signpost dir {}", host.display()))?; + std::fs::write(host.join(SIGNPOST_README), body) + .with_context(|| format!("failed to write signpost in {}", host.display()))?; + } + Ok(()) +} + /// Owns the container lifecycle: the docker availability check, the runtime image, /// and per-user create/start/stop/remove. Cheap to clone (holds an `Arc` pool). #[derive(Clone)] @@ -202,6 +313,7 @@ impl ContainerManager { /// container is already running. pub async fn ensure(&self, user_id: &str) -> Result<()> { let fs = build_user_fs(&self.system, user_id).await?; + let wd = std::env::current_dir().context("failed to read working directory")?; // Host directories must exist before the mount, or Docker creates them // root-owned with surprising modes. Created by the host process, so they are @@ -210,6 +322,7 @@ impl ContainerManager { std::fs::create_dir_all(&host) .with_context(|| format!("failed to create host dir {}", host.display()))?; } + ensure_signposts(&wd)?; let name = &fs.container_name; let want_user = host_uid_gid().map(|(uid, gid)| format!("{uid}:{gid}")); @@ -262,6 +375,12 @@ impl ContainerManager { args.push("-v".into()); args.push(spec); } + // The virtual memory roots, read-only, nested inside the home mount (Docker + // orders mounts by destination depth, as it already does for `shared/`). + for (host, container) in signpost_mounts(&wd, &fs.container_home) { + args.push("-v".into()); + args.push(format!("{}:{}:ro", host.display(), container.display())); + } args.push(IMAGE_TAG.into()); // Long-lived idle process; nothing runs until `docker exec` drives it. args.extend(["sleep".into(), "infinity".into()]); @@ -406,11 +525,32 @@ async fn image_matches(name: &str) -> bool { .unwrap_or(true) } +/// Whether a container carries the memory signpost mounts (see [`signpost_mounts`]). +/// Mounts are fixed at `docker create` time, so a container predating them keeps the +/// old, confusing view — bash silently writing into a `user-memory/` directory nobody +/// reads — until it is recreated. This is the fourth self-heal axis, and it is worth +/// its own check rather than an [`IMAGE_TAG`] bump: the image itself is unchanged, and +/// a bump would make every installation rebuild it to fix a mount. Unreadable inspect +/// ⇒ `true`, so a docker hiccup never churns a working container. +async fn signposts_mounted(name: &str) -> bool { + let Ok(out) = docker(&["inspect", "-f", "{{range .Mounts}}{{println .Destination}}{{end}}", name]).await + else { + return true; + }; + let dests: Vec<&str> = out.lines().map(str::trim).collect(); + signpost_mounts(Path::new(""), Path::new(CONTAINER_HOME)) + .iter() + .all(|(_, container)| dests.iter().any(|d| Path::new(d) == container)) +} + /// Whether an existing container can be reused as-is: right `--user` (§6 UID coherence), -/// `--init` (fast, clean `docker stop`) **and** the current image. A mismatch on any of -/// the three recreates it. +/// `--init` (fast, clean `docker stop`), the current image **and** the memory signpost +/// mounts. A mismatch on any of the four recreates it. async fn reusable(name: &str, want_user: &Option) -> bool { - user_matches(name, want_user).await && init_matches(name).await && image_matches(name).await + user_matches(name, want_user).await + && init_matches(name).await + && image_matches(name).await + && signposts_mounted(name).await } /// Gives the container's runtime `uid`/`gid` a passwd + shadow (+ group) entry, so diff --git a/crates/skald-core/src/tools/fs/mod.rs b/crates/skald-core/src/tools/fs/mod.rs index 62245a5..12ce6bd 100644 --- a/crates/skald-core/src/tools/fs/mod.rs +++ b/crates/skald-core/src/tools/fs/mod.rs @@ -68,17 +68,37 @@ pub struct MemRef { pub rel: String, } +/// Strips the ways an agent spells "in my home" — `./`, `~/`, the container-absolute +/// `{CONTAINER_HOME}/` — so the memory roots are recognised whichever spelling the +/// model reaches for. +/// +/// Without this, `~/user-memory/x.md` misses the match below and falls through to the +/// **disk** router, which resolves it against the caller's home: the note lands in a +/// physical `user-memory/` directory that no tool ever reads back, since every reader +/// (`read_file`, `list_files`, `memory_search`, the lints, the viewer) goes to +/// `memory_docs`. Silent data loss, and the kind an agent then re-confirms by `ls`. +fn strip_home_spelling(user_path: &str) -> &str { + let p = user_path.trim_start_matches("./"); + if let Some(rest) = p.strip_prefix("~/") { + return rest; + } + // `/root/user-memory/…`, but not `/rootless/…` — the separator is required. + p.strip_prefix(crate::container::CONTAINER_HOME) + .and_then(|rest| rest.strip_prefix('/')) + .unwrap_or(p) +} + /// Classifies a user-supplied path. Returns `Some` when it lands under one of the /// virtual memory roots — to be routed to SQLite — and `None` for an ordinary /// disk path. /// -/// The **first** component decides the store, taken raw *before* normalization, so -/// a `..` in the tail can never drop the memory root and silently fall back to a -/// disk path. The tail is then normalized (resolving `.`/`..`) and clamped at the -/// store root, so a memory path stays within its store and an absolute path is -/// always disk. +/// The **first** component decides the store, taken raw *before* normalization (bar +/// the home spelling, see [`strip_home_spelling`]), so a `..` in the tail can never +/// drop the memory root and silently fall back to a disk path. The tail is then +/// normalized (resolving `.`/`..`) and clamped at the store root, so a memory path +/// stays within its store. pub fn classify_memory(user_path: &str) -> Option { - let mut parts = user_path.trim_start_matches("./").splitn(2, ['/', '\\']); + let mut parts = strip_home_spelling(user_path).splitn(2, ['/', '\\']); let scope = match parts.next()? { USER_MEMORY_ROOT => MemScope::User, SHARED_MEMORY_ROOT => MemScope::Shared, @@ -417,6 +437,26 @@ mod tests { assert!(classify_memory("user-memoryish/x").is_none()); } + /// A memory path spelled as if it lived in the home must still reach the note + /// store — otherwise it would be written to a *physical* `user-memory/` directory + /// no reader ever looks at. + #[test] + fn classify_memory_accepts_home_spellings() { + for p in ["~/user-memory/x.md", "/root/user-memory/x.md", "./user-memory/x.md"] { + let m = classify_memory(p).unwrap_or_else(|| panic!("{p} must classify as memory")); + assert!(matches!(m.scope, MemScope::User)); + assert_eq!(m.rel, "x.md", "{p}"); + } + assert!(matches!( + classify_memory("~/shared-memory/casa.md").unwrap().scope, + MemScope::Shared + )); + + // the container-home strip needs a real separator, and stops at the home + assert!(classify_memory("/rootless/user-memory/x.md").is_none()); + assert!(classify_memory("/root/notes/user-memory/x.md").is_none()); + } + /// A throwaway owner-schema pool (as `Arc`, ready for a `ToolContext`), plus its /// dir for cleanup. `tag` + a counter keep parallel tests off the same file. async fn store(tag: &str) -> (Arc, PathBuf) {