From ff3e6942ccabbf02866f642715125a9e635b3df6 Mon Sep 17 00:00:00 2001 From: xavix-yo Date: Wed, 22 Jul 2026 09:22:11 +0100 Subject: [PATCH] remove agent-callable restart tool (blast radius in multi-user model) --- CLAUDE.md | 16 ++++++++-------- Cargo.lock | 1 - Cargo.toml | 1 - crates/skald-core/src/approval/mod.rs | 1 - .../skald-core/src/session/handler/approval.rs | 7 ------- .../skald-core/src/session/handler/llm_loop.rs | 15 +-------------- crates/skald-core/src/session/handler/resume.rs | 11 ----------- crates/skald-core/src/skald/bundles.rs | 1 - crates/skald-core/src/tools/mod.rs | 1 - crates/skald-core/src/tools/tool_names.rs | 1 - run.sh | 9 +++++---- src/frontend/api/sessions.rs | 9 --------- 12 files changed, 14 insertions(+), 59 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 4aca404..3742427 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -55,7 +55,7 @@ The application core is the `skald-core` crate; the binaries are **shells** arou 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. +- **The core never learns about the process shell.** There is no in-core restart hook — the former `restart` tool and its `tools::restart::set_restart_handler` seam were removed. The only coupling to the supervisor is now the `run.sh` exit-code protocol (exit `255` ⇒ re-exec the same binary by path), a seam no code currently triggers (kept for a future admin-driven restart). The live expression of this principle is `skald_core::boot`, which emits startup lines each shell renders (`src/boot_format.rs` here). **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`. @@ -74,7 +74,7 @@ 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`, as the non-root host uid — `sudo` for system installs — with a robust /stop that reaps the command's process-group; see `container/`; the only live path is `run_with` (needs `ToolContext`) — the context-free `Tool::execute`/`execute_async` now **error** (`HOST_PATH_ERROR`) instead of the old host `sh -c`, so nothing can run a command outside the sandbox), `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/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/`; the only live path is `run_with` (needs `ToolContext`) — the context-free `Tool::execute`/`execute_async` now **error** (`HOST_PATH_ERROR`) instead of the old host `sh -c`, so nothing can run a command outside the sandbox), `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 | @@ -207,9 +207,9 @@ At context-build time (`MessageBuilder`), attachments of the **current turn** (t ## 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`, `restart`, `execute_task`, writes outside whitelisted paths). A `Require` registers a `oneshot` in the in-memory `pending` map keyed by `request_id` and emits an approval event over WS. +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). A `Require` registers a `oneshot` in the in-memory `pending` map keyed by `request_id` and emits an approval event over WS. -Resolution is **source-agnostic**: the WS + Inbox paths resolve by `request_id`; the inline chat card resolves by the durable `tool_call_id` via `POST /api/tools/:tool_call_id/resolve` (`resolve_tool` in `src/frontend/api/sessions.rs`), which derives the owning session from the tool call's own stack row — never a hardcoded source. Live pending cards fire the `oneshot`; post-restart a simple tool runs directly on the owning session via `ChatSessionHandler::execute_tool`, which now goes through the **same canonical path as the live loop** — `build_execution` (owner pool + per-user container `ToolContext`) driven by `drive_execution` — so a resolved `write_file`/`execute_cmd` acts on the user's workspace/container, never the server cwd/host (was a §6 escape; sub-agent + `restart` tools are still handled by their own branches earlier in `resolve_tool`). See `docs/approval/`. +Resolution is **source-agnostic**: the WS + Inbox paths resolve by `request_id`; the inline chat card resolves by the durable `tool_call_id` via `POST /api/tools/:tool_call_id/resolve` (`resolve_tool` in `src/frontend/api/sessions.rs`), which derives the owning session from the tool call's own stack row — never a hardcoded source. Live pending cards fire the `oneshot`; post-restart a simple tool runs directly on the owning session via `ChatSessionHandler::execute_tool`, which now goes through the **same canonical path as the live loop** — `build_execution` (owner pool + per-user container `ToolContext`) driven by `drive_execution` — so a resolved `write_file`/`execute_cmd` acts on the user's workspace/container, never the server cwd/host (was a §6 escape; sub-agent tools are still handled by their own branch earlier in `resolve_tool`). See `docs/approval/`. The **diff preview** in a `PendingWrite` event (`handler/approval.rs::read_current_content`) routes exactly like the fs-tools: `user-memory/`/`shared-memory/` → `memory_docs` on the right pool, every other agent path → the caller's host workspace via `resolve_host_path(&self.fs, …)`. It must never use the cwd-relative `fs::resolve` — that showed a bogus "new file" on overwrites (or the diff of a same-named cwd file), so the user would approve the wrong diff. @@ -217,11 +217,11 @@ The **diff preview** in a `PendingWrite` event (`handler/approval.rs::read_curre ## Restart -`restart` **no longer rebuilds anything** — it does not compile. +There is **no in-app restart** anymore. The agent-callable `restart` tool and its `set_restart_handler` seam were removed (blast radius = the whole box: it dropped every user's session and in-RAM DEK from one user's chat — a power-user leftover, out of place in the multi-user model). Nothing in the process now calls `libc::_exit(-1)`. -No restart handler is installed, so `restart` calls `libc::_exit(-1)` (= exit code 255); `run.sh` re-executes the same binary *by path*. (The `set_restart_handler` seam stays for a hypothetical shell without a supervisor, but nothing installs a handler today.) +The supervisor protocol survives but is currently **unreachable in-app**: `run.sh` still re-executes the binary *by path* when it exits `255`, but no code produces that exit code. Restarting is therefore a manual/admin operation. -Use it to pick up `config.yml` / `providers.yaml` / database changes, which are only read at startup. To load new **code**: `./build.sh`, then restart — the supervisor picks up the new binary on the next loop, since `build.sh` installs it with an atomic rename. +To pick up `config.yml` / `providers.yaml` / database changes (read only at startup), or to load new **code** (`./build.sh` installs the new binary via atomic rename): stop the server and let `run.sh` loop, or re-run `./run.sh`. A future admin-only restart action (endpoint/button gated by an admin capability) would re-use the `255 ⇒ re-exec` seam — it is intentionally kept for that. > `run.bat` is still stale (`cargo run`) and must be fixed. @@ -253,7 +253,7 @@ The `docs/` directory is **ignored** for now — do not read it, reference it, o Copy `default.config.yaml` → `config.yml`. Never commit `config.yml` (contains API keys). -`providers.yaml` (repo root, cwd-relative like `config.yml`) declares the **OpenAI-compatible LLM provider types** — endpoints, UI metadata, per-model JSON field mapping, id-glob enrichment rules, reasoning knobs. Loaded at boot by `llm::providers::declared`; edit + `restart`, no rebuild. An invalid entry is logged and skipped, never fatal; an `id` colliding with a native provider is skipped. Adding a new OpenAI-compatible provider is a YAML edit, not a Rust file. The shipped file is validated by a unit test (`declared::tests::shipped_providers_yaml_is_valid`). +`providers.yaml` (repo root, cwd-relative like `config.yml`) declares the **OpenAI-compatible LLM provider types** — endpoints, UI metadata, per-model JSON field mapping, id-glob enrichment rules, reasoning knobs. Loaded at boot by `llm::providers::declared`; edit + restart the process, no rebuild. An invalid entry is logged and skipped, never fatal; an `id` colliding with a native provider is skipped. Adding a new OpenAI-compatible provider is a YAML edit, not a Rust file. The shipped file is validated by a unit test (`declared::tests::shipped_providers_yaml_is_valid`). ## Python environment diff --git a/Cargo.lock b/Cargo.lock index 80d17fa..58f9f89 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4178,7 +4178,6 @@ dependencies = [ "futures", "honcho-client", "indexmap 2.14.0", - "libc", "llm-client", "mcp-client", "notify", diff --git a/Cargo.toml b/Cargo.toml index c6dabb9..c9d599a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -71,7 +71,6 @@ tracing = "0.1" tracing-subscriber = { version = "0.3", features = ["env-filter"] } tracing-appender = "0.2" chrono = { version = "0.4", default-features = false, features = ["clock", "std"] } -libc = "0.2" notify = "8" honcho-client = { path = "crates/honcho-client" } llm-client = { path = "crates/llm-client" } diff --git a/crates/skald-core/src/approval/mod.rs b/crates/skald-core/src/approval/mod.rs index af5243e..e62be71 100644 --- a/crates/skald-core/src/approval/mod.rs +++ b/crates/skald-core/src/approval/mod.rs @@ -231,7 +231,6 @@ impl ApprovalManager { let defaults: &[(&str, &str)] = &[ (tn::EXECUTE_CMD, "require"), - (tn::RESTART, "require"), // Opening a mobile pairing window emits a secret (the QR) into chat: // it must be a deliberate human action, not LLM-triggerable (plugin.md §11). ("mobile_start_pairing", "require"), diff --git a/crates/skald-core/src/session/handler/approval.rs b/crates/skald-core/src/session/handler/approval.rs index a931d2d..cb8bfee 100644 --- a/crates/skald-core/src/session/handler/approval.rs +++ b/crates/skald-core/src/session/handler/approval.rs @@ -41,13 +41,6 @@ impl ChatSessionHandler { } else if tool_name == tn::EXECUTE_CMD { let cmd = arguments["command"].as_str().unwrap_or(""); em.pending_write(request_id, tool_call_id, "$ execute_cmd".to_string(), None, format!("$ {cmd}")).await; - } else if tool_name == tn::RESTART { - em.pending_write( - request_id, tool_call_id, - "$ restart".to_string(), - None, - "Riavvia il processo (exit -1 → supervisor ricompila e rilancia)".to_string(), - ).await; } else { em.approval_required(request_id, tool_call_id, tool_name.to_string(), arguments.clone()).await; } diff --git a/crates/skald-core/src/session/handler/llm_loop.rs b/crates/skald-core/src/session/handler/llm_loop.rs index 6623688..e9dae04 100644 --- a/crates/skald-core/src/session/handler/llm_loop.rs +++ b/crates/skald-core/src/session/handler/llm_loop.rs @@ -1,9 +1,8 @@ use std::sync::Arc; use tokio::sync::mpsc; use tokio_util::sync::CancellationToken; -use tracing::{debug, info, trace}; +use tracing::{debug, trace}; -use crate::tools::tool_names as tn; use crate::chat_event_bus::ToolCallEvent; use crate::chatbot::{LlmTurn, ToolCall}; use crate::db::{chat_history, chat_llm_tools}; @@ -264,18 +263,6 @@ impl ChatSessionHandler { debug!(session_id = self.session_id, tool = %call.name, tool_call_id, "dispatching"); - // `restart` calls process::exit — mark the call done in the DB first so it - // doesn't reappear as `pending` after the supervisor relaunches. - if call.name == tn::RESTART { - info!(session_id = self.session_id, tool_call_id, "restart approved — marking done then exiting"); - chat_llm_tools::complete(pool, tool_call_id, "Riavvio avviato.", "string").await?; - em.tool_done(tool_call_id, "Riavvio avviato.".to_string(), "string".to_string(), None, None).await; - // Use _exit() to skip C atexit handlers (e.g. Metal GPU cleanup in - // whisper-rs/ggml, which aborts with SIGABRT and yields exit code 134 - // instead of 255 — breaking the run.sh restart supervisor). - unsafe { libc::_exit(-1) } - } - // Route the approved call to its executor. `AbortPending` means the // clarification WS channel closed — end the turn and leave the tool // `pending` for resume to re-ask. diff --git a/crates/skald-core/src/session/handler/resume.rs b/crates/skald-core/src/session/handler/resume.rs index 5c1e8fa..7a4f180 100644 --- a/crates/skald-core/src/session/handler/resume.rs +++ b/crates/skald-core/src/session/handler/resume.rs @@ -358,17 +358,6 @@ impl ChatSessionHandler { GateOutcome::ChannelClosed => return Ok(true), // pending still, WS disconnected } - // `restart` calls process::exit and never returns — mark done first. - if tc.name == tn::RESTART { - info!(session_id = self.session_id, tool_call_id = tc.id, "restart approved (resume) — marking done then exiting"); - chat_llm_tools::complete(pool, tc.id, "Riavvio avviato.", "string").await?; - em.tool_done(tc.id, "Riavvio avviato.".to_string(), "string".to_string(), None, None).await; - // Use _exit() to skip C atexit handlers (e.g. Metal GPU cleanup in - // whisper-rs/ggml, which aborts with SIGABRT and yields exit code 134 - // instead of 255 — breaking the run.sh restart supervisor). - unsafe { libc::_exit(-1) } - } - // Re-run the persisted intent through the SAME dispatcher as a live turn // (`execute_tool_call`), not the flat `build_execution`. This routes // sub-agent tools (`execute_task` mode=sync, `execute_subtask`, diff --git a/crates/skald-core/src/skald/bundles.rs b/crates/skald-core/src/skald/bundles.rs index 55fb3e6..90abddc 100644 --- a/crates/skald-core/src/skald/bundles.rs +++ b/crates/skald-core/src/skald/bundles.rs @@ -211,7 +211,6 @@ impl Tools { tool_registry.register(crate::tools::ast_outline::AstOutline::new()); tool_registry.register(crate::tools::exec::ExecuteCmd); tool_registry.register(crate::tools::read_notification::ReadNotification); - tool_registry.register(crate::tools::restart::Restart); // Unified listing / toggling across plugins, cron (+ agents for list). MCP // is no longer agent-managed (blueprint §14): connectors are curated by the // admin and activated by the user via the Connectors UI/API, not tools. diff --git a/crates/skald-core/src/tools/mod.rs b/crates/skald-core/src/tools/mod.rs index 4df59b8..cdc74bd 100644 --- a/crates/skald-core/src/tools/mod.rs +++ b/crates/skald-core/src/tools/mod.rs @@ -43,7 +43,6 @@ pub mod list_secrets; pub mod notify; pub mod set_secret; pub mod read_notification; -pub mod restart; pub mod show_file; pub mod toggle_item; diff --git a/crates/skald-core/src/tools/tool_names.rs b/crates/skald-core/src/tools/tool_names.rs index c7dc666..5e2ee99 100644 --- a/crates/skald-core/src/tools/tool_names.rs +++ b/crates/skald-core/src/tools/tool_names.rs @@ -1,6 +1,5 @@ pub const EXECUTE_TASK: &str = "execute_task"; pub const EXECUTE_SUBTASK: &str = "execute_subtask"; -pub const RESTART: &str = "restart"; pub const UPDATE_SCRATCHPAD: &str = "update_scratchpad"; pub const WRITE_TODOS: &str = "write_todos"; pub const ASK_USER_CLARIFICATION: &str = "ask_user_clarification"; diff --git a/run.sh b/run.sh index 70a9ada..960c703 100755 --- a/run.sh +++ b/run.sh @@ -7,12 +7,13 @@ # # App exit codes: # 0 graceful shutdown (SIGINT/SIGTERM) → stop the loop -# 255 restart requested (`restart` tool → libc::_exit(-1)) → re-exec +# 255 restart requested → re-exec the same binary by path # * error → propagate and stop # -# The loop re-executes the binary *by path*, so running ./build.sh while the -# supervisor is up and then asking the agent to restart loads the new build. -# Note that `restart` alone no longer rebuilds: edit source, ./build.sh, restart. +# The loop re-executes the binary *by path*, so after ./build.sh (atomic rename) +# a re-exec loads the new build. NOTE: the in-app `restart` tool was removed, so +# nothing currently produces 255 — this branch is kept for a future admin-only +# restart action. Today, restart manually: stop the server and re-run ./run.sh. set -u diff --git a/src/frontend/api/sessions.rs b/src/frontend/api/sessions.rs index e0be7ef..2881881 100644 --- a/src/frontend/api/sessions.rs +++ b/src/frontend/api/sessions.rs @@ -227,15 +227,6 @@ pub async fn resolve_tool( })); } - // `restart` calls process::exit — mark done in DB first. - if tc_name == tn::RESTART { - chat_llm_tools::complete(db, tc_id, "Riavvio avviato.", "string").await?; - // Use _exit() to skip C atexit handlers (e.g. Metal GPU cleanup in - // whisper-rs/ggml, which aborts with SIGABRT and yields exit code 134 - // instead of 255 — breaking the run.sh restart supervisor). - unsafe { libc::_exit(-1) } - } - // ── Live path: LLM loop is blocked waiting for approval ────────────────── if ctx.approval .resolve_for_tool_call(tc_id, ApprovalDecision::Approved)