diff --git a/CLAUDE.md b/CLAUDE.md index 4306b84..5160957 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -57,7 +57,7 @@ Two rules keep the boundary real, and both are enforced by the compiler: - **The core never names a plugin.** A plugin contributes tools through `Plugin::tools(self: Arc)` — the sibling of `http_router()` — so nothing in the core has to downcast to a concrete type. Naming one would drag every plugin in the tree into the core, including a C build via `plugin-transcribe-whisper-local`. - **The core never learns about the process shell.** The `restart` tool defaults to the supervisor protocol (`exit(-1)`); a shell with different needs (e.g. one with no supervisor) can install its own `tools::restart::set_restart_handler` at startup. The default server shell installs none and relies on `run.sh`. The seam stays even though nothing installs a handler today. -**Plugin visibility & per-user config.** Plugins are managed from the `#plugins` page, not only by the agent. Enable/disable + instance config + access grants are gated by the `plugin.manage` capability (admin-only by construction). Visibility is **opt-in**: a row in `plugin_access(plugin_id, user_id)` grants a user sight of an enabled plugin (`plugin_id` is bare TEXT, never a FK — a `plugins` row exists only after the first toggle). A plugin with a non-empty `Plugin::user_config_schema()` exposes per-user settings, stored in `plugin_user_configs` (**admin-readable system.db — never secrets**) and applied through the `Plugin::update_user_config` hook, whose default just stores the blob via the `PluginUserConfigApi` on `PluginContext.user_config`. Telegram is the reference impl: the user pastes the bot's pairing code in their Plugins page, the override turns it into a `chat_id → user_id` binding (same write path as the `telegram_pairing` tool) and stores a `{linked, chat_id}` status blob for the UI. Endpoints: admin `GET/PUT /api/plugins[/{id}]` + `GET/PUT /api/plugins/{id}/access`; user `GET /api/plugins/mine` + `PUT /api/plugins/{id}/my-config`. +**Plugin visibility & per-user config.** The admin surface is split in two: `#plugin-catalog` (`plugin-catalog.js`) is a status board — one card per plugin with an enable toggle + health dot + a Configure button — and `#plugin-detail?id=` (`plugin-detail.js`) holds the instance-config form + per-user access checklist for one plugin (the plugin counterpart of `connector-detail.js`). The user-facing half is `#plugins` (`plugins-page.js`): granted plugins + their per-user config forms. Enable/disable + instance config + access grants are gated by the `plugin.manage` capability (admin-only by construction). Visibility is **opt-in**: a row in `plugin_access(plugin_id, user_id)` grants a user sight of an enabled plugin (`plugin_id` is bare TEXT, never a FK — a `plugins` row exists only after the first toggle). A plugin with a non-empty `Plugin::user_config_schema()` exposes per-user settings, stored in `plugin_user_configs` (**admin-readable system.db — never secrets**) and applied through the `Plugin::update_user_config` hook, whose default just stores the blob via the `PluginUserConfigApi` on `PluginContext.user_config`. Telegram is the reference impl: the user pastes the bot's pairing code in their Plugins page, the override turns it into a `chat_id → user_id` binding (same write path as the `telegram_pairing` tool) and stores a `{linked, chat_id}` status blob for the UI. Endpoints: admin `GET/PUT /api/plugins[/{id}]` + `GET/PUT /api/plugins/{id}/access`; user `GET /api/plugins/mine` + `PUT /api/plugins/{id}/my-config`. **Plugin HTTP routes & web pages.** Every plugin's `http_router()` mounts at boot under `/api/plugin//` — **enabled or not**: two shared gates wrap each router (`require_auth`, then `guard::plugin_enabled_gate`, which re-checks the DB flag per request and answers 404 while disabled), so enable/disable serves/stops routes immediately with no restart, and plugin responses carry `Cache-Control: no-cache`. The router contract: cheap and safe to build pre-start, handlers tolerant of the not-running state (resolve runtime state per request through a shared cell, as mobile-connector does). A plugin may also contribute **frontend pages** via `Plugin::web_pages()` (`PluginPage { page_id, title, icon, entry, admin_only, priority }`): `GET /api/plugins/pages` returns the caller's visible pages (admin: all; others: non-`admin_only` pages of granted, enabled plugins) with `entry_url` resolved, and the sidebar renders them as menu entries routed `#plugin//`. A single `` (`web/components/plugin-page-host.js`) dynamic-imports the fragment ES module the plugin serves from its own router, registers its default-exported HTMLElement class, and mounts it with the `plugin-id` attribute — the fragment talks to its backend only through `/api/plugin//…` and runs with full session privileges (plugins are trusted: they ship in the binary). The frontend knows nothing about plugin page contents or behavior. @@ -74,8 +74,8 @@ Two rules keep the boundary real, and both are enforced by the compiler: | `crates/skald-core/src/chat_hub/` | `ChatHub`: broadcast events to all connected WS clients | | `crates/skald-core/src/chat_event_bus.rs` | Global async bus for cross-session events | | `crates/skald-core/src/agents.rs` | Discovers agents from `agents/*/`, loads meta + system prompt | -| `crates/skald-core/src/tools/` | Built-in tools: `exec` (**runs inside the caller's per-user Docker container** via `docker exec` — see `container/`), `restart`, `list_agents`, `fs/*` (route `user-memory/`/`shared-memory/` to `memory_docs`, and every other **physical** path through `ctx.fs` to the caller's per-user host workspace — see DB tables + container), `notify`, `ast_outline`, `image_generate`, MCP tools, plugin tools, cron tools | -| `crates/skald-core/src/container/` | `ContainerManager` (§6): per-user Docker containers (the execution sandbox). Docker is a **hard requirement** — `check_docker()` fails `Skald::new` (→ shell exits) if the daemon is unreachable. Builds our own `skald-runtime` image (python+node) once from the embedded `Dockerfile`, then `reconcile_all()` at boot ensures one running container `skald-{userid}` per active user. `build_user_fs()` assembles a user's `UserFs` (home `{WD}/homes/{userid}` → `/root`, plus each `shared/{name}` they belong to). Shells the `docker` CLI (no client crate) | +| `crates/skald-core/src/tools/` | Built-in tools: `exec` (**runs inside the caller's per-user Docker container** via `docker exec`, as the non-root host uid — `sudo` for system installs — with a robust /stop that reaps the command's process-group; see `container/`), `restart`, `list_agents`, `fs/*` (route `user-memory/`/`shared-memory/` to `memory_docs`, and every other **physical** path through `ctx.fs` to the caller's per-user host workspace — see DB tables + container), `notify`, `ast_outline`, `image_generate`, MCP tools, plugin tools, cron tools | +| `crates/skald-core/src/container/` | `ContainerManager` (§6): per-user Docker containers (the execution sandbox). Docker is a **hard requirement** — `check_docker()` fails `Skald::new` (→ shell exits) if the daemon is unreachable. Builds our own `skald-runtime` image (python+node+**sudo**; tag is **versioned** `skald-runtime:v2` so a `Dockerfile` change forces a rebuild) once from the embedded `Dockerfile`, then `reconcile_all()` at boot ensures one running container `skald-{userid}` per active user. Each container runs as the **host `uid:gid`** (`--user`, §6 UID coherence) with `--init` (tini reaps zombies); `ensure()` **self-heals** a container whose `--user` is stale (e.g. an old root one) by recreating it, and injects a passwd/shadow entry post-create so `sudo` (NOPASSWD, in the image) resolves the arbitrary uid. `build_user_fs()` assembles a user's `UserFs` (home `{WD}/homes/{userid}` → `/root`, plus each `shared/{name}` they belong to). Shells the `docker` CLI (no client crate) | | `crates/skald-core/src/tool_catalog.rs` | `ToolCatalog`: unified tool listing façade (wraps ToolRegistry + McpManager) | | `crates/skald-core/src/events.rs` | `ServerEvent` enum streamed over WebSocket to the frontend | | `crates/skald-core/src/db/` | sqlx SQLite — see below | @@ -124,7 +124,7 @@ Schema is greenfield (no migrations, §0), but a purely **additive** column land ## Filesystem & containers (blueprint §6) -Each user has one **permanent Docker container** (`skald-{userid}`, our own `skald-runtime` image with python+node), created on user creation and started at boot (`ContainerManager`, `crates/skald-core/src/container/`). Docker is **required**: a missing daemon fails `Skald::new` and the process exits. +Each user has one **permanent Docker container** (`skald-{userid}`, our own `skald-runtime` image with python+node), created on user creation and started at boot (`ContainerManager`, `crates/skald-core/src/container/`). Docker is **required**: a missing daemon fails `Skald::new` and the process exits. The container runs as the **host `uid:gid`** (not root) so files created in-container and by the host-side fs-tools share ownership on the bind mounts (matters on native Linux; masked on macOS Docker Desktop). Because that user isn't root, the image ships passwordless `sudo` (a passwd/shadow entry is injected at create) so an agent can still `sudo apt-get install …`; `--init` runs tini as pid 1 to reap zombies. The agent sees **one namespace**, routed on the first path component. The choke point is `UserFs` (`core-api/src/user_fs.rs`, a pure value type carried in `ToolContext.fs`), plus `resolve_host_path()` in `tools/fs/mod.rs`: @@ -139,7 +139,7 @@ Two views, **one storage**: the fs-tools run **host-side** in the Skald process **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`. `execute_cmd` cancellation caveat: dropping the `docker exec` client on /stop may not kill the in-container process (a robust stop tracking the PID + `docker exec … kill` is a follow-up). **Per-user MCP connectors now run inside this container** (§7) — the container infra enabled it; see the MCP connectors section. +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 calls a best-effort `remount(user)` that rebuilds 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. ## MCP connectors (blueprint §7/§14/§15) @@ -292,8 +292,11 @@ All extend `LightElement` from `web/lib/base.js` (Lit). `ChatSession` (`web/lib/ | `approval-rules.js` | `` | Approval rule management | | `cron-jobs.js` | `` | Scheduled job management | | `connectors.js` | `` | MCP Connectors list (one row per connector): user activate/deactivate + granted globals; admin gets a **Sign-in providers** modal (OAuth client creds) + Catalog/Marketplace nav (§7/§14/§15) | -| `plugins-page.js` | `` | `#plugins` — user: granted plugins + schema-driven per-user config form; admin: enable toggle, instance config, per-user access checklist | +| `plugins-page.js` | `` | `#plugins` — user half: granted plugins + schema-driven per-user config form | +| `plugin-catalog.js` | `` | `#plugin-catalog` — admin status board: one card per plugin (enable toggle + health dot + Configure → `#plugin-detail`) | +| `plugin-detail.js` | `` | `#plugin-detail?id=` — one plugin's admin page: instance-config form (`config_schema`) + per-user access checklist (plugin twin of `connector-detail.js`) | | `plugin-page-host.js` | `` | Host for plugin-contributed pages (`#plugin//`): dynamic-imports the fragment module, registers its element, mounts it with `plugin-id` | +| `shared-folders.js` | `` | `#shared-folders` — admin-only CRUD for on-disk shared folders (§6): create/describe/delete + per-member read-only/read-write grants; description feeds the assistant's `__SHARED_FOLDERS__` context | | `connector-detail.js` | `` | A connector's own page (`#connector?name=X`): env/secret form + Test, the **OAuth login panel** (sign in → paste code → complete, §15), global enable + per-user access grants | | `shared/connector-common.js` | (helpers) | Shared Connectors vocabulary: `statusOf` (incl. `needs_login` for a pending OAuth row), `STATUS_LABEL`, schema normalization, `jf` fetch | | `llm-providers.js` | `` | LLM provider management | @@ -302,3 +305,4 @@ All extend `LightElement` from `web/lib/base.js` (Lit). `ChatSession` (`web/lib/ | `models-transcribe.js` | `` | Transcription model CRUD | | `models-image.js` | `` | Image generation model CRUD | | `mobile-app.js` | `` | Mobile app shell | +| `shared/settings-page.js` | `` | Mobile settings: per-user avatar, locale picker (`I18nMixin`), profile/preferences | diff --git a/agents/project-coordinator/AGENT.md b/agents/project-coordinator/AGENT.md index 8a52dad..85f34c4 100644 --- a/agents/project-coordinator/AGENT.md +++ b/agents/project-coordinator/AGENT.md @@ -28,15 +28,17 @@ Delegate work to these task specialists via `execute_task` / `execute_subtask`: Your system prompt already contains, without you asking: -- The project's **name**, **description**, and **working directory** (the project root — all relative file paths resolve there). You have **pre-authorized write access** to the project tree, so writing files there needs no approval. A project may be **shared** with other members (read-only or read-write): anything you write into the project folder is visible to everyone it is shared with, so keep private, user-specific notes in `user-memory/` rather than in a shared project. +- The project's **name**, **description**, **folder path** (`projects/{owner_username}/{slug}`), and **sharing** (which members it's shared with, if any). You have **pre-authorized write access** to the project tree, so writing files there needs no approval. A project may be **shared** with other members (read-only or read-write): anything you write into the project folder is visible to everyone it is shared with, so keep private, user-specific notes in `user-memory/` rather than in a shared project. - **`user-memory/index.md`** and **`shared-memory/index.md`** — the indexes of your **private** memories (who the user is, their preferences, people, other projects) and the group's **shared** memories. Both are injected automatically. Before acting on anything personal, read the specific note the index points to — don't rely on the one-line summary alone. - **`SKALD.md`** at the project root — this project's **living diary** (see below). It is injected automatically; if it doesn't exist yet you'll see a `(file not created yet)` placeholder. Treat all of this as ground truth. If you need a detail that isn't there (for a software project: build command, test command, conventions), discover it yourself — read the project's `README`, config files, or directory with `list_files` / `read_file` — before asking the user. -### Use relative paths inside the project +### Reference project files by their full path -Every filesystem tool (`read_file`, `write_file`, `edit_file`, `list_files`, …) and `execute_cmd` already run with the project root as their working directory. For files **inside the project, always use paths relative to the project root** — e.g. `notes/itinerary.md`, `drafts/chapter-1.md`, or `src/main.rs` — not the full absolute path. Do not prepend the working directory yourself, and do not `cd` into it in `execute_cmd`. Use an absolute path only for files that live **outside** the project tree. +The session working directory is your home directory (`~`), not the project folder. A relative path like `notes/itinerary.md` resolves to `~/notes/itinerary.md` — your private home, not the project. To reference a file **inside the project**, always use the full agent path under the project folder shown above — e.g. `projects/alice/trip-planning/notes/itinerary.md`, `projects/alice/trip-planning/drafts/chapter-1.md`, or `projects/alice/trip-planning/src/main.rs`. This applies to every filesystem tool (`read_file`, `write_file`, `edit_file`, `list_files`, …). + +For `execute_cmd`, either pass the project folder as `workdir` (preferred — e.g. `{"workdir": "projects/alice/trip-planning", "command": "make test"}`) or `cd` into it at the start of the command. Use a relative path (or `~/…`) only for files that live in your private home, outside the project tree. --- @@ -64,7 +66,7 @@ Do **not** push code-oriented agents (software-architect, software-engineer, spe ``` ## PROJECT CONTEXT Project: -Project root: +Project folder: Description: # (software tasks only:) Build/check command: diff --git a/agents/project-coordinator/meta.json b/agents/project-coordinator/meta.json index 2be024b..c17ebd7 100644 --- a/agents/project-coordinator/meta.json +++ b/agents/project-coordinator/meta.json @@ -15,6 +15,6 @@ "type": "chat", "scope": "reasoning", "strength": "average", - "inject_memory": ["user-memory/index.md", "shared-memory/index.md", "$WD/SKALD.md"], + "inject_memory": ["user-memory/index.md", "shared-memory/index.md", "__PROJECT_ROOT__/SKALD.md"], "icon": "icon.png" } diff --git a/crates/skald-core/src/container/Dockerfile b/crates/skald-core/src/container/Dockerfile index 3a70541..beaa569 100644 --- a/crates/skald-core/src/container/Dockerfile +++ b/crates/skald-core/src/container/Dockerfile @@ -21,8 +21,20 @@ RUN apt-get update && apt-get install -y --no-install-recommends \ ca-certificates \ curl \ git \ + sudo \ + util-linux \ && rm -rf /var/lib/apt/lists/* +# The container runs as the host process's uid:gid (blueprint §6 UID coherence), so +# in-container work and the host fs-tools share ownership on the bind mounts. That +# user is not root, so a blanket passwordless sudo restores install capability +# (`sudo apt-get install …`, `sudo npm i -g …`) inside the user's own sandbox — no +# security boundary is crossed (the isolation is the mount set, not the uid; the +# container was already full-root before). `util-linux` provides `setsid`, used to +# make `execute_cmd` killable as a process group. +RUN echo 'ALL ALL=(ALL) NOPASSWD:ALL' > /etc/sudoers.d/skald-nopasswd \ + && chmod 0440 /etc/sudoers.d/skald-nopasswd + WORKDIR /root # The container is long-lived: created once, started at boot, exec'd into per diff --git a/crates/skald-core/src/container/mod.rs b/crates/skald-core/src/container/mod.rs index af9ba02..386507a 100644 --- a/crates/skald-core/src/container/mod.rs +++ b/crates/skald-core/src/container/mod.rs @@ -27,8 +27,12 @@ use core_api::user_fs::{ProjectMount, SharedMount, UserFs}; use crate::db; -/// Our runtime image tag. Built once from the embedded [`Dockerfile`]. -const IMAGE_TAG: &str = "skald-runtime"; +/// Our runtime image tag. Built once from the embedded [`Dockerfile`]. The version +/// suffix is the image cache-buster: [`ContainerManager::ensure_image`] rebuilds only +/// when the tag is absent, so **bump it whenever the [`Dockerfile`] changes** (e.g. +/// `v2` added `sudo` + a NOPASSWD sudoers for the non-root container user). Old tags +/// linger as orphaned images (harmless). +const IMAGE_TAG: &str = "skald-runtime:v2"; /// The embedded Dockerfile — the source of truth, so the image can be built with /// no files shipped alongside the binary (binary-first). @@ -53,6 +57,19 @@ pub fn container_name(user_id: &str) -> String { format!("skald-{user_id}") } +/// The host process's own `(uid, gid)`, or `None` on non-unix. We run each container +/// as this uid:gid (blueprint §6 UID coherence) so files created inside the container +/// and by the host-side fs-tools share ownership on the bind mounts. On non-unix we +/// fall back to the image default (root) and skip `--user`. +#[cfg(unix)] +fn host_uid_gid() -> Option<(u32, u32)> { + Some((unsafe { libc::getuid() }, unsafe { libc::getgid() })) +} +#[cfg(not(unix))] +fn host_uid_gid() -> Option<(u32, u32)> { + None +} + /// Builds the [`UserFs`] view for a user: private home + the shared folders they /// belong to, plus the container those mount into. Host paths are absolute /// (anchored at the process working directory), as Docker bind mounts require. @@ -159,36 +176,62 @@ impl ContainerManager { Ok(()) } - /// Ensures the user's container exists and is running. Creates the host - /// directories, the container (if missing) with the right bind mounts, and - /// starts it (if stopped). Idempotent — a no-op when already running. + /// Ensures the user's container exists, runs as the host uid:gid, and is started. + /// Creates the host directories, the container (if missing) with the right bind + /// mounts + `--user`, and starts it (if stopped). Self-healing: a container whose + /// `--user` no longer matches the host uid:gid (e.g. an old root container from a + /// previous binary) is torn down and recreated. Idempotent — a no-op when a + /// matching container is already running. pub async fn ensure(&self, user_id: &str) -> Result<()> { let fs = build_user_fs(&self.system, user_id).await?; // Host directories must exist before the mount, or Docker creates them - // root-owned with surprising modes. + // root-owned with surprising modes. Created by the host process, so they are + // owned by the host uid:gid the container runs as — the mounts are writable. for (host, _container, _w) in fs.mounts() { std::fs::create_dir_all(&host) .with_context(|| format!("failed to create host dir {}", host.display()))?; } let name = &fs.container_name; + let want_user = host_uid_gid().map(|(uid, gid)| format!("{uid}:{gid}")); + match container_state(name).await { - ContainerState::Running => return Ok(()), - ContainerState::Stopped => { + // Reuse only if it runs as the expected user; otherwise recreate below. + ContainerState::Running if user_matches(name, &want_user).await => return Ok(()), + ContainerState::Stopped if user_matches(name, &want_user).await => { docker(&["start", name]).await.context("docker start failed")?; return Ok(()); } ContainerState::Absent => {} + // Present but with a stale `--user` (e.g. an old root container): tear it + // down. The container holds no durable state — everything is in the bind + // mounts — so a recreate is safe. + _ => { + let _ = docker(&["rm", "-f", name]).await; + } } let mut args: Vec = vec![ "create".into(), + // `--init` runs tini as pid 1 so orphaned/killed processes are reaped — + // otherwise `execute_cmd`'s /stop reaper (and any command that leaves + // orphans) would accumulate zombies under the idle `sleep infinity`. + "--init".into(), "--name".into(), name.clone(), "--workdir".into(), fs.container_home.to_string_lossy().into_owned(), ]; + // Run as the host uid:gid for bind-mount ownership coherence (§6). HOME is set + // explicitly because the passwd entry that resolves this uid is injected only + // *after* create (see below), so Docker would otherwise default HOME to "/". + if let Some(user) = &want_user { + args.push("--user".into()); + args.push(user.clone()); + args.push("-e".into()); + args.push(format!("HOME={}", fs.container_home.to_string_lossy())); + } for (host, container, writable) in fs.mounts() { let mut spec = format!("{}:{}", host.display(), container.display()); if !writable { @@ -204,6 +247,14 @@ impl ContainerManager { let argv: Vec<&str> = args.iter().map(String::as_str).collect(); docker(&argv).await.context("docker create failed")?; docker(&["start", name]).await.context("docker start failed")?; + + // Give the non-root container user a passwd/group entry so `sudo` (NOPASSWD, + // baked into the image) can resolve it. Persists in the container's writable + // layer for its lifetime; re-done on recreate. Best-effort. + if let Some((uid, gid)) = host_uid_gid() { + ensure_container_user(name, uid, gid).await; + } + tracing::info!(user = %user_id, container = %name, "user container created and started"); Ok(()) } @@ -279,6 +330,44 @@ async fn container_state(name: &str) -> ContainerState { } } +/// Reads a container's configured `--user` (`docker inspect .Config.User`). Empty for a +/// container created without `--user` (i.e. root). +async fn container_user(name: &str) -> String { + docker(&["inspect", "-f", "{{.Config.User}}", name]) + .await + .map(|s| s.trim().to_string()) + .unwrap_or_default() +} + +/// Whether a container's `--user` matches what we want. `want == None` (non-unix, no +/// `--user` requested) matches anything so we never churn a container needlessly. +async fn user_matches(name: &str, want: &Option) -> bool { + match want { + None => true, + Some(w) => &container_user(name).await == w, + } +} + +/// Gives the container's runtime `uid`/`gid` a passwd + shadow (+ group) entry, so +/// tools that resolve the invoking user work despite the arbitrary numeric uid — and +/// so `sudo` succeeds (without a shadow entry PAM's account phase fails with "account +/// validation failure" even under NOPASSWD). The shadow password is `*` (login +/// disabled, account valid); the group is added only when its gid is otherwise unused. +/// Runs as root inside the container (`-u 0`, which overrides the container's `--user`), +/// idempotent (keyed on the passwd entry), best-effort. +async fn ensure_container_user(name: &str, uid: u32, gid: u32) { + let script = format!( + "if ! getent passwd {uid} >/dev/null 2>&1; then \ + getent group {gid} >/dev/null 2>&1 || echo 'skald:x:{gid}:' >> /etc/group; \ + echo 'skald:x:{uid}:{gid}:skald:/root:/bin/sh' >> /etc/passwd; \ + echo 'skald:*:19000:0:99999:7:::' >> /etc/shadow; \ + fi" + ); + if let Err(e) = docker(&["exec", "-u", "0", name, "sh", "-c", &script]).await { + tracing::warn!(container = %name, error = %e, "failed to inject container passwd entry (sudo may not resolve the user)"); + } +} + /// Runs `docker `, returning trimmed stdout on success or an error carrying /// stderr. `stdin` is closed so a build never blocks waiting for input. async fn docker(args: &[&str]) -> Result { diff --git a/crates/skald-core/src/projects/mod.rs b/crates/skald-core/src/projects/mod.rs index d416dfd..b969c61 100644 --- a/crates/skald-core/src/projects/mod.rs +++ b/crates/skald-core/src/projects/mod.rs @@ -1,34 +1,70 @@ use crate::db::projects::Project; use crate::run_context::RunContext; -/// Builds the runtime `RunContext` for working on `project`, layering the project's -/// working directory + a context header over an optional pre-resolved `base` RC (which -/// carries static config set at creation time, e.g. `security_group`). +/// A project member's display info for the system-prompt block. /// -/// `working_directory` is the **agent path** `projects/{owner_username}/{slug}` — the -/// same namespace the fs-tools and `execute_cmd` route through (the host/container -/// mapping is handled by `UserFs`). Writes there are auto-allowed by the seeded -/// `projects/*` approval rule and physically gated by the per-member read-only mount, -/// so no host-path `allow_fs_writes` grant is needed (that was the old single-user -/// model, which predated per-user containers). -pub fn build_runtime_run_context( +/// The display name is what the user usually goes by (fallback to the username); +/// the username is the unique handle. Both are shown so the agent can refer to a +/// member either way the user does in conversation. +pub struct ProjectMemberView { + pub display_name: String, + pub username: String, +} + +/// Builds the runtime `RunContext` for working on `project`, layering a project +/// context block over an optional pre-resolved `base` RC (which carries static +/// config set at creation time, e.g. `security_group`). +/// +/// The session working directory is **always** the user's home (`~`); project +/// files are referenced by their absolute agent path `projects/{owner}/{slug}`, +/// which `UserFs` routes to the per-member bind mount. This keeps the working +/// directory stable across sessions (so MCP servers running in the container see +/// a consistent cwd) and avoids silent path rewriting inside tool calls. +/// +/// Writes under `projects/*` are auto-allowed by the seeded approval rule and +/// physically gated by the per-member read-only mount, so no host-path +/// `allow_fs_writes` grant is needed. +pub fn build_project_run_context( project: &Project, owner_username: &str, + members: &[ProjectMemberView], base: Option, ) -> RunContext { let mut rc = base.unwrap_or_default(); - rc.working_directory = Some(format!("projects/{owner_username}/{}", project.slug)); + let project_path = format!("projects/{owner_username}/{}", project.slug); + rc.project_root = Some(project_path.clone()); - let project_header = if project.description.is_empty() { - format!("You are working on project \"{}\".", project.name) + let mut block = vec![ + format!("You are working on project \"{}\".", project.name), + format!("Project folder: {project_path}"), + ]; + if !project.description.is_empty() { + block.insert(1, format!("Description: {}", project.description)); + } + // Sharing line: list members other than the owner, or note the project is private. + // The owner is implicit (they are the user the agent is talking to), so they are + // excluded from the list. Display name first, username in parentheses. + let others: Vec = members + .iter() + .filter(|m| m.username != owner_username) + .map(|m| { + if m.display_name.is_empty() || m.display_name == m.username { + m.username.clone() + } else { + format!("{} ({})", m.display_name, m.username) + } + }) + .collect(); + let sharing = if others.is_empty() { + "Shared with: not shared with anyone yet.".to_string() } else { - format!( - "You are working on project \"{}\". Description: {}", - project.name, project.description - ) + format!("Shared with: {}.", others.join(", ")) }; - let mut injected = vec![project_header]; + block.push(sharing); + + // Prepend the project block to any existing system_prompt fragments. + let mut injected = block; injected.extend(std::mem::take(&mut rc.system_prompt)); rc.system_prompt = injected; diff --git a/crates/skald-core/src/run_context/mod.rs b/crates/skald-core/src/run_context/mod.rs index f75ef01..0aaf6c3 100644 --- a/crates/skald-core/src/run_context/mod.rs +++ b/crates/skald-core/src/run_context/mod.rs @@ -1,4 +1,3 @@ -use std::path::PathBuf; use std::sync::Arc; use anyhow::{Result, bail}; @@ -21,9 +20,13 @@ pub struct RunContext { /// `docs/`, `skills/`, and everything in `allow_fs_writes`, which is readable too). #[serde(default)] pub allow_fs_reads: Vec, - /// Working directory for tool calls. None means Skald's own process cwd. + /// Project root (agent path `projects/{owner}/{slug}`) when this is a project + /// session, `None` otherwise. The session working directory is always the user's + /// home (`~`); the agent references project files via this absolute agent path, + /// which `UserFs` routes to the per-member bind mount. Used to resolve + /// `__PROJECT_ROOT__` placeholders in an agent's `inject_memory` paths. #[serde(default)] - pub working_directory: Option, + pub project_root: Option, } impl RunContext { @@ -51,24 +54,14 @@ impl RunContext { Some(self.system_prompt.join("\n\n")) } - /// Effective working directory for this session. - /// Returns the configured path if set and non-empty, otherwise Skald's process cwd. - pub fn effective_working_dir(&self) -> PathBuf { - self.working_directory - .as_deref() - .filter(|d| !d.is_empty()) - .map(PathBuf::from) - .unwrap_or_else(|| std::env::current_dir().unwrap_or_default()) - } - /// True if writing to `path` is pre-authorized by this RunContext. - /// Entries in `allow_fs_writes` are resolved against `effective_working_dir`, - /// so relative entries like `"data"` are treated as relative to the session WD. + /// Entries in `allow_fs_writes` are resolved against Skald's process cwd, + /// so relative entries like `"data"` are treated as relative to the process cwd. /// Paths are canonicalized first (resolving `..`/symlinks), then matched as /// exact file OR recursive directory prefix. pub fn is_write_allowed(&self, path: &str) -> bool { if self.allow_fs_writes.is_empty() { return false; } - let wd = self.effective_working_dir(); + let wd = std::env::current_dir().unwrap_or_default(); let canon = canonicalize_for_policy(path, &wd); self.allow_fs_writes.iter().any(|entry| { path_under(&canon, &canonicalize_for_policy(entry, &wd)) @@ -76,20 +69,20 @@ impl RunContext { } /// True if reading `path` is pre-authorized by this RunContext. - /// Read access is granted (no approval prompt) for: the working directory itself, - /// its `docs/` and `skills/` subtrees (always-safe baseline), any `allow_fs_reads` - /// entry, and anything writable (write implies read). All paths are canonicalized - /// first so `..`/symlink escapes cannot widen the grant. + /// Read access is granted (no approval prompt) for: the process working directory + /// itself, its `docs/` and `skills/` subtrees (always-safe baseline), any + /// `allow_fs_reads` entry, and anything writable (write implies read). All paths + /// are canonicalized first so `..`/symlink escapes cannot widen the grant. /// /// Note: this only relaxes a `Require` decision to `Allow` — an explicit `Deny` /// rule (e.g. on `secrets/`) still wins, because the approval engine is consulted /// first and `Deny` is never overridden by this fast-path. pub fn is_read_allowed(&self, path: &str) -> bool { - let wd = self.effective_working_dir(); + let wd = std::env::current_dir().unwrap_or_default(); let canon = canonicalize_for_policy(path, &wd); let mut roots: Vec = vec![ - canonicalize_for_policy(".", &wd), // working directory itself + canonicalize_for_policy(".", &wd), // process working directory canonicalize_for_policy("docs", &wd), canonicalize_for_policy("skills", &wd), ]; @@ -116,7 +109,7 @@ pub enum RunContextDecision { /// role's effective set ([`crate::db::roles::role_allows_group`]); anything else /// is [`RunContextDecision::Forbidden`]. /// - **fs escalation**: for a non-admin every other `RunContext` field -/// (`system_prompt`, `allow_fs_writes`/`allow_fs_reads`, `working_directory`) is +/// (`system_prompt`, `allow_fs_writes`/`allow_fs_reads`, `project_root`) is /// **discarded** — the client can set the permission group, nothing more. A rich /// run-context (a project's) is resolved server-side, never through this path. /// @@ -303,57 +296,14 @@ mod tests { dir } - fn rc_with_wd(wd: &PathBuf) -> RunContext { - RunContext { - working_directory: Some(wd.to_string_lossy().into_owned()), - ..Default::default() - } - } - - #[test] - fn read_allows_working_dir_docs_skills() { - let wd = unique_tmp(); - for sub in ["docs", "skills", "sub", "secrets"] { - std::fs::create_dir_all(wd.join(sub)).unwrap(); - std::fs::write(wd.join(sub).join("f.txt"), "x").unwrap(); - } - std::fs::write(wd.join("root.txt"), "x").unwrap(); - - let rc = rc_with_wd(&wd); - assert!(rc.is_read_allowed("root.txt")); - assert!(rc.is_read_allowed("docs/f.txt")); - assert!(rc.is_read_allowed("skills/f.txt")); - assert!(rc.is_read_allowed("sub/f.txt")); - // secrets/ is under the WD, so the fast-path allows it — the `secrets/` *deny rule* - // (consulted before this fast-path in the gate) is what actually blocks it. - assert!(rc.is_read_allowed("secrets/f.txt")); - - std::fs::remove_dir_all(&wd).ok(); - } - - #[test] - fn read_denies_outside_working_dir() { - let wd = unique_tmp(); - let outside = unique_tmp(); // sibling temp dir, not under wd - std::fs::write(outside.join("f.txt"), "x").unwrap(); - - let rc = rc_with_wd(&wd); - assert!(!rc.is_read_allowed(outside.join("f.txt").to_str().unwrap())); - - std::fs::remove_dir_all(&wd).ok(); - std::fs::remove_dir_all(&outside).ok(); - } - #[test] fn read_allows_write_paths_and_extra_reads() { - let wd = unique_tmp(); let writable = unique_tmp(); let readable = unique_tmp(); std::fs::write(writable.join("w.txt"), "x").unwrap(); std::fs::write(readable.join("r.txt"), "x").unwrap(); let rc = RunContext { - working_directory: Some(wd.to_string_lossy().into_owned()), allow_fs_writes: vec![writable.to_string_lossy().into_owned()], allow_fs_reads: vec![readable.to_string_lossy().into_owned()], ..Default::default() @@ -365,7 +315,6 @@ mod tests { assert!(rc.is_read_allowed(readable.join("r.txt").to_str().unwrap())); assert!(!rc.is_write_allowed(readable.join("r.txt").to_str().unwrap())); - std::fs::remove_dir_all(&wd).ok(); std::fs::remove_dir_all(&writable).ok(); std::fs::remove_dir_all(&readable).ok(); } @@ -408,15 +357,15 @@ mod tests { std::fs::create_dir_all(wd.join("data")).unwrap(); std::fs::create_dir_all(wd.join("secrets")).unwrap(); + let data_dir = wd.join("data").to_string_lossy().into_owned(); let rc = RunContext { - working_directory: Some(wd.to_string_lossy().into_owned()), - allow_fs_writes: vec!["data".to_string()], + allow_fs_writes: vec![data_dir], ..Default::default() }; // Writing into data/ is allowed... - assert!(rc.is_write_allowed("data/new.txt")); + assert!(rc.is_write_allowed(wd.join("data").join("new.txt").to_str().unwrap())); // ...but data/../secrets/x escapes the grant and must NOT be allowed. - assert!(!rc.is_write_allowed("data/../secrets/x.txt")); + assert!(!rc.is_write_allowed(wd.join("data").join("..").join("secrets").join("x.txt").to_str().unwrap())); std::fs::remove_dir_all(&wd).ok(); } diff --git a/crates/skald-core/src/session/handler/dispatch.rs b/crates/skald-core/src/session/handler/dispatch.rs index a182aab..3adbab6 100644 --- a/crates/skald-core/src/session/handler/dispatch.rs +++ b/crates/skald-core/src/session/handler/dispatch.rs @@ -1,9 +1,10 @@ -//! Working-directory argument rewriting and the per-tool-call dispatch router. +//! Per-tool-call dispatch router. //! -//! Extracted from `run_agent_turn`: `effective_args` applies the RunContext working -//! directory to a call's arguments, and `execute_tool_call` routes an approved call -//! to the right executor (special non-cancellable paths + the unified cancellable -//! `ToolExecution` path). +//! Extracted from `run_agent_turn`: `execute_tool_call` routes an approved call to +//! the right executor (special non-cancellable paths + the unified cancellable +//! `ToolExecution` path). The session working directory is always the user's home +//! (`~`); tool calls receive their arguments unchanged, and the agent references +//! project files via the absolute agent path `projects/{owner}/{slug}/…`. use serde_json::Value; use tokio::sync::mpsc; @@ -39,28 +40,6 @@ pub(super) enum DispatchResult { } impl ChatSessionHandler { - /// Applies the RunContext working directory to a tool call's arguments: - /// resolves a relative `path` against the effective WD and injects `workdir` - /// for `execute_cmd`. The caller keeps the original `arguments` for the - /// `ToolStart` event / DB logging; this returns the copy used for execution. - pub(super) async fn effective_args(&self, tool_name: &str, args: &Value) -> Value { - let mut effective = args.clone(); - let wd = self.run_context.read().await - .as_ref() - .map(|rc| rc.effective_working_dir()); - if let Some(wd) = wd { - if let Some(path) = effective["path"].as_str() - && !std::path::Path::new(path).is_absolute() - { - effective["path"] = Value::String(wd.join(path).to_string_lossy().into_owned()); - } - if tool_name == tn::EXECUTE_CMD && effective.get("workdir").is_none() { - effective["workdir"] = Value::String(wd.to_string_lossy().into_owned()); - } - } - effective - } - /// Routes one already-approved tool call to the right executor. Covers the /// special, non-cancellable paths (sub-agent, scratchpad, todos, clarification, /// the `task_completed` stub) and the unified cancellable `ToolExecution` path diff --git a/crates/skald-core/src/session/handler/llm_loop.rs b/crates/skald-core/src/session/handler/llm_loop.rs index 9072625..7a226b3 100644 --- a/crates/skald-core/src/session/handler/llm_loop.rs +++ b/crates/skald-core/src/session/handler/llm_loop.rs @@ -31,9 +31,9 @@ enum CallFlow { /// Outcome of gating + dispatching one call inside a concurrent sub-agent batch, /// carried from the concurrent phase to the ordered recording phase. enum GatedExec { - /// Gate passed; the sub-agent produced an outcome to record. `effective` is the - /// working-dir-resolved args used for recording (FileChanged / logging). - Done { effective: serde_json::Value, outcome: ExecutionOutcome }, + /// Gate passed; the sub-agent produced an outcome to record. `arguments` is + /// the call's args (used for FileChanged / logging). + Done { arguments: serde_json::Value, outcome: ExecutionOutcome }, /// Approval gate rejected the call — already marked/emitted by the gate; skip it. Rejected, /// The turn must end now: the clarification WS channel closed (dispatch returned @@ -234,12 +234,12 @@ impl ChatSessionHandler { self.tools.target_path(&call.name, &call.arguments), ).await; - // Resolve relative paths / inject workdir from the RunContext. - // `call.arguments` (originals) were used for the ToolStart event and DB - // logging above; `effective_args` is used from here on. - let effective_args = self.effective_args(&call.name, &call.arguments).await; + // Tool calls receive their arguments unchanged — the session working + // directory is always the user's home (`~`), and the agent references + // project files via their absolute agent path. `call.arguments` is both + // logged and executed. - match self.run_approval_gate(tool_call_id, &call.name, &effective_args, &config.agent_id, em).await? { + match self.run_approval_gate(tool_call_id, &call.name, &call.arguments, &config.agent_id, em).await? { GateOutcome::Proceed => {} GateOutcome::Rejected => return Ok(CallFlow::Continue), GateOutcome::ChannelClosed => return Ok(CallFlow::End(TurnOutcome::Cancelled)), @@ -263,14 +263,14 @@ impl ChatSessionHandler { // clarification WS channel closed — end the turn and leave the tool // `pending` for resume to re-ask. let outcome = match self.execute_tool_call( - stack_id, config, tool_call_id, &call.name, &effective_args, token, tx, + stack_id, config, tool_call_id, &call.name, &call.arguments, token, tx, ).await { DispatchResult::Outcome(o) => o, DispatchResult::AbortPending => return Ok(CallFlow::End(TurnOutcome::Cancelled)), }; match self.record_tool_outcome( - tool_call_id, &call.name, &effective_args, outcome, em, Some(all_tool_calls), + tool_call_id, &call.name, &call.arguments, outcome, em, Some(all_tool_calls), ).await? { RecordFlow::Continue => Ok(CallFlow::Continue), RecordFlow::Abort => Ok(CallFlow::End(TurnOutcome::Cancelled)), @@ -340,14 +340,13 @@ impl ChatSessionHandler { { let mut stream = stream::iter(jobs) .map(|(idx, tool_call_id, name, arguments)| async move { - let effective = self.effective_args(&name, &arguments).await; let gated = match self.run_approval_gate( - tool_call_id, &name, &effective, &config.agent_id, em, + tool_call_id, &name, &arguments, &config.agent_id, em, ).await { Ok(GateOutcome::Proceed) => match self.execute_tool_call( - stack_id, config, tool_call_id, &name, &effective, token, tx, + stack_id, config, tool_call_id, &name, &arguments, token, tx, ).await { - DispatchResult::Outcome(outcome) => Ok(GatedExec::Done { effective, outcome }), + DispatchResult::Outcome(outcome) => Ok(GatedExec::Done { arguments, outcome }), DispatchResult::AbortPending => Ok(GatedExec::AbortTurn), }, Ok(GateOutcome::Rejected) => Ok(GatedExec::Rejected), @@ -370,9 +369,9 @@ impl ChatSessionHandler { // The gate already marked the row rejected and emitted the event. GatedExec::Rejected => {} GatedExec::AbortTurn => abort = true, - GatedExec::Done { effective, outcome } => { + GatedExec::Done { arguments, outcome } => { match self.record_tool_outcome( - *tool_call_id, &call.name, &effective, outcome, em, Some(all_tool_calls), + *tool_call_id, &call.name, &arguments, outcome, em, Some(all_tool_calls), ).await? { RecordFlow::Continue => {} RecordFlow::Abort => abort = true, diff --git a/crates/skald-core/src/session/handler/message_builder.rs b/crates/skald-core/src/session/handler/message_builder.rs index e3fcfa7..9a05fb1 100644 --- a/crates/skald-core/src/session/handler/message_builder.rs +++ b/crates/skald-core/src/session/handler/message_builder.rs @@ -47,9 +47,11 @@ pub struct MessageBuilder { pub max_history_messages: usize, pub max_tool_result_chars: Option, pub compactor: Option>, - /// Effective working directory for this session. When set (e.g. from a project - /// RunContext), it overrides the process cwd in the date/time/OS/WD tail block. - pub working_directory: Option, + /// Project root (agent path `projects/{owner}/{slug}`) when this is a project + /// session — used to resolve `__PROJECT_ROOT__` placeholders in `inject_memory` + /// paths. `None` for non-project sessions, in which case an `inject_memory` + /// entry that references `__PROJECT_ROOT__` is skipped (with a warning). + pub project_root: Option, } impl MessageBuilder { @@ -117,9 +119,7 @@ impl MessageBuilder { // ── Skills index ────────────────────────────────────────────────────── // Injected for every agent unless it opts out (`inject_skills: false`). - // Reuses the memory-path resolution so the shown path is relative when the - // index is under the session WD, absolute otherwise (it lives under Skald's - // own cwd, so it shows as absolute inside project sessions). Skipped silently + // Reuses the memory-path resolver for display consistency. Skipped silently // when no skills are installed. if meta.inject_skills { let (abs, display) = self.resolve_memory_path(SKILLS_INDEX_PATH); @@ -398,14 +398,11 @@ impl MessageBuilder { None => format!("Current date and time: {formatted}"), }; - let cwd = self.working_directory.clone() - .unwrap_or_else(|| std::env::current_dir().unwrap_or_default()) - .display() - .to_string(); + let cwd = "~"; + Some(format!( "{date_line}\nOperating system: {}\nWorking directory: {cwd}\n\ - Filesystem tools and execute_cmd use this working directory for relative paths — \ - no need to `cd` into it first.", + Filesystem tools and execute_cmd resolve relative paths against your home directory.", os_description() )) } else { @@ -455,17 +452,18 @@ impl MessageBuilder { /// Builds the MCP list section that replaces the `__MCP_LIST__` sentinel. /// Resolves an `inject_memory` entry to `(absolute path to read, path to show)`. /// - /// `$WD` expands to the session's effective working directory (RunContext WD, or the - /// process cwd when unset). The shown path is **relative to that working directory - /// when the file lives under it, absolute otherwise** — so when the agent references - /// it back via `edit_file`/`write_file`, the loop's working-directory injection - /// (which rewrites relative paths against the WD) resolves to the very same file. + /// `__PROJECT_ROOT__` expands to the session's project root (the agent path + /// `projects/{owner}/{slug}`, set on the RunContext for project sessions) — + /// e.g. `"__PROJECT_ROOT__/SKALD.md"` loads a project-local diary. The shown + /// path is the agent path itself, which the loop's filesystem routing + /// resolves back to the same file when the agent references it via + /// `edit_file`/`write_file`. /// Loads an `inject_memory` entry, returning `(content, display_path)`. /// /// Virtual memory paths are read from SQLite: `user-memory/…` from the owner /// `pool`, `shared-memory/…` from the `shared_pool` (`system.db`). Everything - /// else (`data/…`, `$WD/…`) is an ordinary disk read. A missing note / file - /// yields `None`, rendered as "(file not created yet)". + /// else (`data/…`, `__PROJECT_ROOT__/…`, an absolute path) is an ordinary disk + /// read. A missing note / file yields `None`, rendered as "(file not created yet)". async fn load_inject_memory(&self, mem_path: &str) -> (Option, String) { use crate::tools::fs::{classify_memory, MemScope}; if let Some(m) = classify_memory(mem_path) { @@ -482,15 +480,22 @@ impl MessageBuilder { } fn resolve_memory_path(&self, mem_path: &str) -> (std::path::PathBuf, String) { - let wd = self.working_directory.clone() - .unwrap_or_else(|| std::env::current_dir().unwrap_or_default()); - let expanded = mem_path.replace("$WD", &wd.display().to_string()); - let abs = crate::tools::fs::resolve(&expanded) - .unwrap_or_else(|_| std::path::PathBuf::from(&expanded)); - let display = match abs.strip_prefix(&wd) { - Ok(rel) => rel.to_string_lossy().into_owned(), - Err(_) => abs.to_string_lossy().into_owned(), + let display = if mem_path.contains("__PROJECT_ROOT__") { + match &self.project_root { + Some(root) => mem_path.replace("__PROJECT_ROOT__", root), + None => { + tracing::warn!( + mem_path, + "inject_memory entry references __PROJECT_ROOT__ but this session has no project root; skipping" + ); + return (std::path::PathBuf::from(mem_path), mem_path.to_string()); + } + } + } else { + mem_path.to_string() }; + let abs = crate::tools::fs::resolve(&display) + .unwrap_or_else(|_| std::path::PathBuf::from(&display)); (abs, display) } diff --git a/crates/skald-core/src/session/handler/messages.rs b/crates/skald-core/src/session/handler/messages.rs index 1bd0090..ee3a01d 100644 --- a/crates/skald-core/src/session/handler/messages.rs +++ b/crates/skald-core/src/session/handler/messages.rs @@ -24,9 +24,9 @@ impl ChatSessionHandler { cache_hints: bool, capabilities: &[String], ) -> anyhow::Result> { - let effective_wd = self.run_context.read().await + let project_root = self.run_context.read().await .as_ref() - .map(|rc| rc.effective_working_dir()); + .and_then(|rc| rc.project_root.clone()); let builder = MessageBuilder { pool: Arc::clone(&self.db), shared_pool: Arc::clone(&self.shared_pool), @@ -37,7 +37,7 @@ impl ChatSessionHandler { max_history_messages: self.max_history_messages, max_tool_result_chars: self.max_tool_result_chars, compactor: self.compactor.clone(), - working_directory: effective_wd, + project_root, }; // `pool` is passed in from the caller (always `&self.db`) but we take // ownership via Arc::clone above so the signature stays backward-compatible. diff --git a/crates/skald-core/src/session/handler/resume.rs b/crates/skald-core/src/session/handler/resume.rs index d1dcbc7..0e0f7f0 100644 --- a/crates/skald-core/src/session/handler/resume.rs +++ b/crates/skald-core/src/session/handler/resume.rs @@ -326,11 +326,9 @@ impl ChatSessionHandler { // sub-agent tools (`execute_task` mode=sync, `execute_subtask`, // `run_subtask`) through the recursive interception in `dispatch.rs`; // `build_execution` alone does not know them and would fail with - // "Unknown tool: execute_task". Apply the RunContext working dir exactly - // like the live loop. - let effective_args = self.effective_args(&tc.name, &args).await; + // "Unknown tool: execute_task". Args are passed through unchanged. let outcome = match self.execute_tool_call( - stack_id, config, tc.id, &tc.name, &effective_args, token, tx, + stack_id, config, tc.id, &tc.name, &args, token, tx, ).await { super::dispatch::DispatchResult::Outcome(o) => o, // Clarification WS channel closed mid-resume — leave the tool pending @@ -339,7 +337,7 @@ impl ChatSessionHandler { }; // resume passes `None`: it does not accumulate ToolCallEvents nor re-emit // FileChanged (only a live turn does). A /stop mid-resume returns Abort. - match self.record_tool_outcome(tc.id, &tc.name, &effective_args, outcome, &em, None).await? { + match self.record_tool_outcome(tc.id, &tc.name, &args, outcome, &em, None).await? { RecordFlow::Continue => {} RecordFlow::Abort => return Ok(true), } diff --git a/crates/skald-core/src/tools/exec.rs b/crates/skald-core/src/tools/exec.rs index 290bc67..640432e 100644 --- a/crates/skald-core/src/tools/exec.rs +++ b/crates/skald-core/src/tools/exec.rs @@ -24,6 +24,7 @@ impl Tool for ExecuteCmd { fn description(&self) -> &str { "Execute a shell command (sh -c) inside your sandbox container (python + node available). \ Reserve this for: builds, installs, git, tests, scripts, processes, network, package managers. \ + Runs as a non-root user; prefix system-package or global installs with `sudo` (e.g. `sudo apt-get install …`). \ Do NOT use cat/head/tail to read files — use read_file instead. \ Do NOT use grep/rg/find to search — use grep_files instead. \ Do NOT use ls to list directories — use list_files instead. \ @@ -33,10 +34,6 @@ impl Tool for ExecuteCmd { } fn parameters_schema(&self) -> Value { - let cwd = std::env::current_dir() - .map(|p| p.to_string_lossy().to_string()) - .unwrap_or_else(|_| ".".to_string()); - json!({ "type": "object", "properties": { @@ -46,10 +43,8 @@ impl Tool for ExecuteCmd { }, "workdir": { "type": "string", - "description": format!( - "Working directory for the command (absolute path). \ - Omit to use the project root (currently: {cwd})." - ) + "description": "Working directory for the command (an agent path like `projects/{owner}/{slug}` or `~`). \ + Omit to use your home directory (`~`)." }, "timeout": { "type": "integer", @@ -115,29 +110,46 @@ impl Tool for ExecuteCmd { .unwrap_or(DEFAULT_TIMEOUT_SECS) .clamp(1, MAX_TIMEOUT_SECS); + // Robust /stop: run the command in its own session/process-group whose + // leader pid is recorded in a container-side pidfile. On /stop (the work + // future dropped) or on a timeout, the `KillReaper` drop-guard reaps that + // group via a second `docker exec … kill` — `kill_on_drop` alone only kills + // the local `docker exec` client, not the tree Docker started *inside* the + // container. + let pidfile = format!("/tmp/skald-exec-{}.pgid", uuid::Uuid::new_v4()); + let wrapper = format!("echo $$ > {pidfile}; trap 'rm -f {pidfile}' EXIT; {command}"); + Box::new(SimpleExecution::new(Box::pin(async move { - Ok(ToolResult::Text(run_in_container(&container, &workdir, &command, timeout_secs).await?)) + let guard = KillReaper::new(container.clone(), pidfile.clone()); + let out = run_in_container(&container, &workdir, &wrapper, &command, timeout_secs).await?; + guard.disarm(); + Ok(ToolResult::Text(out)) }))) } } -/// Runs a command inside a user's container: `docker exec -w sh -c `. -/// Shares the capture/timeout machinery with the host path. +/// Runs a wrapped command inside a user's container: +/// `docker exec -w setsid -w sh -c