diff --git a/AGENTS.md b/AGENTS.md index 54121fc7..407bf8be 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -280,7 +280,7 @@ editing Stella's own code should know what lives where: | `.stella/memories/*.md` | Durable lessons baked into the byte-stable system prompt prefix. Sorted by filename, loaded once per session. (Write side: the `save_memory` tool.) | | `.stella/skills//SKILL.md` | Auto-promoted skills from recurring reflection lessons. Never enforced — selected and injected as volatile context. | | `.stella/tools/*.toml` | Developer-defined custom script tools. Also scanned at `~/.stella/tools/`. | -| `.stella/settings.json` | Project-scope provider config (overrides built-ins or defines new providers) and tool switches (`tools.bash: "on"` opts the shell tool in — it is off by default in every scope). Merged per-field with org-managed and user scopes. | +| `.stella/settings.json` | Project-scope provider config (overrides built-ins or defines new providers) and tool switches (`tools.bash: "off"` withholds the shell tool — every built-in, the shell included, is registered by default since #710). Merged per-field with org-managed and user scopes. | | `.stella/mcp.toml` | MCP server config — extra tools merged into the registry at session start. | | `.stella/domains.toml` | Domain taxonomy for memory/reflection tagging, inferred by `stella init`. | | `.stella/workspace.json` | Durable per-workspace telemetry identity (`workspace_id`), written by `stella cloud register`. Deliberately **outside** `private/` and safe to commit — sharing it makes every clone/machine report under one `workspace_id` to a cloud org. | diff --git a/README.md b/README.md index 73161174..0b729848 100644 --- a/README.md +++ b/README.md @@ -437,7 +437,7 @@ are also accepted case-insensitively. | ---------------------------------------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | | `read_file` · `write_file` · `edit_file` · `delete_file` | File CRUD with surgical exact-substring edits | | `apply_edits` | One transactional batch of exact-substring edits across many files — every edit validates first, and if any fails nothing is written (`dry_run` validates without writing) | -| `bash` | Run a shell command (timeout kill; `trace: true` echoes each line) — **off by default**, registered only with `"tools": {"bash": "on"}` in settings (any scope) | +| `bash` | Run a shell command (timeout kill; `trace: true` echoes each line) — **registered by default**, withheld with `"tools": {"bash": "off"}` in settings (any scope) | | `grep` · `glob` | Regex content search (ripgrep) · glob file discovery (fd) | | `graph_query` | Query the indexed code graph: symbol definitions/references, file imports/importers/neighborhood — auto-built at session start, refreshed live | | `read_symbol` | Read a named symbol's exact source span, resolved through the code graph — no line-offset guessing; multiple definitions are listed, never silently picked | @@ -465,11 +465,17 @@ All file tools are workspace-root-pinned, and every read/write/edit/delete is recorded in the Files-Touched ledger (shown per turn as `[C·R·U·D] path`, also via `/files`). -**Bash is opt-in.** The default tool surface has no _free-form shell tool_: the -model works through enumerable-argv tools (build/test/lint/format, -`run_script`'s project-declared verbs, the process group, the `repo_*` tools). -Enable `bash` per user, org, or project by adding `"tools": {"bash": "on"}` to -the corresponding `settings.json` scope (normal per-field merge — project wins). +**Bash ships on, and switching it off bounds the shell tool rather than every +path to a shell.** `bash` is registered like every other built-in; withhold it +per user, org, or project by adding `"tools": {"bash": "off"}` to the +corresponding `settings.json` scope (normal per-field merge — project wins). +Prefer the enumerable-argv tools regardless (build/test/lint/format, +`run_script`'s project-declared verbs, the process group, the `repo_*` tools) — +they never interpret a shell string. Note that `"bash": "off"` removes the +free-form shell _tool_, not the shell _capability_: `build_project` and +`run_tests` take a `command` override, `verify_done` a `test_cmd`, and +`run_script` composes from the scripts index, so all four still reach `bash -c` +behind the `command.started` policy fence. Stated precisely, because a security claim that overreaches is worse than none: turning `bash` off removes the tool, not every route to a shell. diff --git a/docs/design/semantic-resolution-bridge.md b/docs/design/semantic-resolution-bridge.md index b34e7575..a3c3c2a9 100644 --- a/docs/design/semantic-resolution-bridge.md +++ b/docs/design/semantic-resolution-bridge.md @@ -75,10 +75,10 @@ strategy (`stella-mcp/tests/`). which spends the memory whether or not a resolution query ever arrives. - **Invariant fit:** clean on "ports, not concretions" (it *is* an adapter behind a trait; `stella-core` never sees it); hostile to dependency-light / - no-daemon unless opt-in gated exactly like the shell and web tools - (`tools.bash: "on"` — off by default in every scope, `AGENTS.md`; - `stella-tools/src/bash.rs:1-12` "Opt-in, never ambient"; - `stella-cli/src/settings.rs:552` for `web`) and registered conditionally + no-daemon unless gated behind a tool switch like the shell and web tools + (`tools.bash: "off"` withholds the shell — registered by default in every + scope since #710, `AGENTS.md`; `stella-tools/src/bash.rs:1-12`) and + registered conditionally like `graph_query` (no server, no schema burning tokens). - **Rough size:** LSP client core (initialize handshake, capability negotiation, document sync, request correlation) plus lifecycle supervision, diff --git a/docs/design/threat-model.md b/docs/design/threat-model.md index 4b1904a9..8bc6cdff 100644 --- a/docs/design/threat-model.md +++ b/docs/design/threat-model.md @@ -121,8 +121,10 @@ so managed denial survives explicit repository trust — the witness is The agent's tool surface is the blast radius of a successful A2. Two sub-boundaries matter and they are frequently conflated: -- **`bash` is opt-in** (`tools.bash: "on"`), and its module doc is explicit - that the default surface has no shell at all. +- **`bash` ships registered** and is withheld with `tools.bash: "off"`. #710 + moved every built-in to on-by-default with a switch, because the previous + posture covered built-ins only and most operators never found the switch. + Assume the default surface HAS a shell. - **The default surface is nonetheless not execution-free.** `run_script`, `start_process`, `repo_commit`/`repo_push`, and workspace custom tools all execute code without `bash` ever being enabled. `start_process` spawns argv @@ -245,10 +247,17 @@ mitigation therefore has a shape the threat does not. ### R4 — No SSRF guard on web tools, by design -An opted-in session can fetch any http(s) URL the host can reach, including -`localhost` and cloud metadata endpoints. This is a documented decision: it -matches the `bash` opt-in and is required for the "fetch my internal dev -server" use case. The gate is the settings opt-in, not a network allowlist. +Any session can fetch any http(s) URL the host can reach, including `localhost` +and cloud metadata endpoints. It is required for the "fetch my internal dev +server" use case, and there is no network allowlist. + +**The compensating control this decision rested on is gone.** The exposure was +accepted because the web family was opt-in, so reaching a metadata endpoint +took a deliberate host action. Since #710 the three key-free web tools are +registered by default and the only gate is an operator who knows to set +`"web": "off"`. Whether that remains acceptable, or whether the default surface +needs a loopback/metadata denylist, is an open ruling (#615) — R4 should not be +read as a still-current risk acceptance. One sharp edge is recorded in `web.rs`: reqwest strips `Cookie` and `Authorization` across a cross-host redirect, but a secret placed in a custom diff --git a/docs/why-stella.md b/docs/why-stella.md index eb81d278..31614289 100644 --- a/docs/why-stella.md +++ b/docs/why-stella.md @@ -61,7 +61,7 @@ plane. | **BYOK, model-agnostic** | Nine hosted providers (Anthropic, OpenAI, Gemini, xAI, DeepSeek, Z.ai, OpenRouter, Vertex, Bedrock) plus **any** OpenAI-compatible local server (Ollama, vLLM, LM Studio, llama.cpp). No account, no gateway. Pin per run with `--model provider/id`. | | **Zero telemetry egress by default** | Community/default Stella sends no telemetry anywhere. Executions, the full event stream, per-call token/cost telemetry, and a `[C·R·U·D] path` files-touched ledger land in a local `.stella/private/store.db` you can open with any SQLite client — and the store is never a dependency of a turn. The sole exception is an [explicitly enrolled Oxagen Enterprise managed deployment](https://stella.oxagen.sh/docs/telemetry#oxagen-enterprise-managed-export): a current signed policy may authorize one minimal content-free operational rollup to one exact allowlisted HTTPS sink. | | **Budget you can trust** | `--budget ` aborts cleanly **between** steps, never mid-tool, so a cap can't corrupt a half-written edit. | -| **Bounded blast radius** | File tools are workspace-root-pinned; the `bash` tool is **off by default** (settings `tools.bash: "on"` to opt in — the default surface is enumerable argv, no shell); an opt-in `bash` sandbox (Seatbelt / bubblewrap) contains prompt-injection damage and **fails closed**; a cloned repo's own hooks never auto-execute (`STELLA_PROJECT_HOOKS=1` to opt in). | +| **Bounded blast radius** | File tools are workspace-root-pinned; the `bash` tool is **registered by default** and withheld with settings `tools.bash: "off"` (and switching it off bounds the shell _tool_, not every path to a shell — `build_project`, `run_tests`, `verify_done` and `run_script` still compose `bash -c` behind the `command.started` policy fence); an opt-in `bash` sandbox (Seatbelt / bubblewrap) contains prompt-injection damage and **fails closed**; a cloned repo's own hooks never auto-execute (`STELLA_PROJECT_HOOKS=1` to opt in). | ## Also in the box diff --git a/scripts/file-size-baseline.txt b/scripts/file-size-baseline.txt index 9ff7b1e1..1e9b0c2a 100644 --- a/scripts/file-size-baseline.txt +++ b/scripts/file-size-baseline.txt @@ -12,7 +12,7 @@ 1623 stella-cli/src/candidate_ws.rs 4604 stella-cli/src/command_deck.rs 2095 stella-core/src/bus.rs -2087 stella-core/src/driver.rs +2098 stella-core/src/driver.rs 2819 stella-core/src/driver/tests.rs 1759 stella-fleet/src/fleet.rs 1601 stella-model/src/anthropic/tests.rs @@ -22,11 +22,11 @@ 2476 stella-pipeline/src/pipeline.rs 2022 stella-pipeline/src/pipeline/tests.rs 2399 stella-protocol/src/event.rs -2270 stella-store/src/lib.rs +2283 stella-store/src/lib.rs 2170 stella-store/src/tests.rs 1884 stella-store/src/usage.rs 1556 stella-tools/src/media.rs -3014 stella-tools/src/registry.rs +3019 stella-tools/src/registry.rs 1797 stella-tools/src/scripts.rs 1705 stella-tui/src/deck_render.rs 4049 stella-tui/src/deck_ui.rs diff --git a/stella-cli/src/agent/prompt.rs b/stella-cli/src/agent/prompt.rs index aa559dbe..84acccaa 100644 --- a/stella-cli/src/agent/prompt.rs +++ b/stella-cli/src/agent/prompt.rs @@ -38,7 +38,7 @@ macro_rules! tool_steering { - Read a definition by name with read_symbol; guessing read_file offsets after a graph_query is the round-trip it exists to remove. - A change touching several files is ONE apply_edits call, not a chain of edit_file calls. -- A tool you cannot see is not available in this session rather than nonexistent. There is no shell unless the workspace enables it ("tools": {"bash": "on"}); issue tracking, web, and media tools register only once their backend is configured (`stella connect github|linear`, an API key, or `gh auth`; ci_status needs the gh CLI). Reach for tool_search before concluding a capability is missing."# +- A tool you cannot see is not available in this session rather than nonexistent. The shell ships registered and a workspace withholds it with "tools": {"bash": "off"}; issue tracking, web, and media tools register only once their backend is configured (`stella connect github|linear`, an API key, or `gh auth`; ci_status needs the gh CLI). Reach for tool_search before concluding a capability is missing."# }; } @@ -469,7 +469,13 @@ mod tests { // A schema can only describe a tool that IS registered — it // can never explain an absence or how to lift it. "not available in this session", - "tools\": {\"bash\": \"on\"}", + // The switch's real polarity. Pinned as `off` on purpose: + // this sentence read `"bash": "on"` — "there is no shell + // unless the workspace enables it" — for every release after + // #710 shipped bash registered-by-default, so the prompt told + // the model the opposite of the tool surface it had, and this + // assertion pinned the false claim in place (#615). + "tools\": {\"bash\": \"off\"}", "tool_search", ] { assert!( diff --git a/stella-cli/src/doctor.rs b/stella-cli/src/doctor.rs index b1fc57a3..a265c9d2 100644 --- a/stella-cli/src/doctor.rs +++ b/stella-cli/src/doctor.rs @@ -5,18 +5,22 @@ //! one pass/fail line each, plus the opt-in repair for the one failure that //! previously had no way out. //! -//! Today there is exactly one check: the integrity of this workspace's -//! `.stella/private/store.db`. A corrupt store used to surface as an error at -//! session start and nothing else — no way to confirm the diagnosis, no way to -//! fix it — so the store's own [`stella_store::integrity`] answers the first -//! half and `--repair` the second. +//! Two checks today: +//! +//! - **store integrity** — this workspace's `.stella/private/store.db`. A +//! corrupt store used to surface as an error at session start and nothing +//! else (no way to confirm the diagnosis, no way to fix it), so the store's +//! own [`stella_store::integrity`] answers the first half and `--repair` the +//! second. +//! - **fleet ledger** — rows in `fleet.db` naming a run that is no longer +//! recorded. Report-only by design; see [`fleet_ledger_orphans`] for why +//! `--repair` must not touch them. //! //! The shape is a LIST of named checks rather than a single hard-coded probe //! because the next environment check (a provider reachable, a `.stella/` //! whose permissions leak transcripts, a codegraph index older than its //! workspace) should be one entry in [`checks`] and a renderer that already -//! knows how to print it — not a second command. It is deliberately still one -//! entry: the surface is only worth what it actually verifies. +//! knows how to print it — not a second command. //! //! # Exit codes //! @@ -111,7 +115,85 @@ pub(crate) fn run_doctor_at(workspace_root: &Path, repair: bool) -> Result<(), S /// Every check this build knows how to run, in the order they are reported. fn checks(workspace_root: &Path, repair: bool) -> Vec { - vec![store_integrity(workspace_root, repair)] + vec![ + store_integrity(workspace_root, repair), + fleet_ledger_orphans(workspace_root), + ] +} + +/// Report `fleet.db` rows whose run is gone (#617 item 5). +/// +/// This is a **report, not a repair**, and `--repair` deliberately does not act +/// on it. The rows could only be removed by deleting fleet history — which +/// tasks ran, what was attempted, what it cost — and nothing reads a row by +/// orphaned run today, so they are inert. A ledger created by a current build +/// constrains these columns and cannot acquire new orphans; an older file was +/// left unconstrained precisely because retrofitting the constraint requires +/// that deletion. So the honest posture is: show the operator what is there and +/// let them decide. +/// +/// A `Pass` either way — orphans are not a fault to fix, and failing the +/// command over inert rows on an old ledger would make `stella doctor` +/// unusable on exactly the workspaces that have history worth keeping. +fn fleet_ledger_orphans(workspace_root: &Path) -> Check { + const NAME: &str = "fleet ledger"; + + let db_path = workspace_root.join(".stella/private/fleet.db"); + if !db_path.exists() { + return Check::pass( + NAME, + "no fleet ledger yet — created by the first `stella fleet` run", + ); + } + let shown = display_path(workspace_root, &db_path); + + let ledger = match stella_fleet::Ledger::open(&db_path) { + Ok(ledger) => ledger, + Err(error) => { + return Check::fail(NAME, format!("{shown}: could not be opened: {error}")); + } + }; + let orphans = match ledger.orphan_rows() { + Ok(orphans) => orphans, + Err(error) => { + return Check::fail(NAME, format!("{shown}: could not be scanned: {error}")); + } + }; + let enforced = ledger.enforces_run_references().unwrap_or(false); + + if orphans.is_empty() { + return Check::pass( + NAME, + format!( + "{shown}: no rows reference a missing run{}", + if enforced { + " (and the schema enforces it)" + } else { + "" + } + ), + ); + } + + let total: i64 = orphans.iter().map(|o| o.count).sum(); + Check::pass( + NAME, + format!("{shown}: {total} row(s) reference a run that is no longer recorded"), + ) + .with_details( + orphans + .iter() + .map(|o| format!("{}.{}: {} row(s)", o.table, o.column, o.count)) + .collect(), + ) + .with_remedy(vec![ + "nothing to do — these rows are inert (no query resolves a row by its run) and \ + they are kept because removing them would delete fleet history" + .to_string(), + "this ledger predates the run-reference constraints; a fleet.db created by this \ + build cannot acquire new orphans" + .to_string(), + ]) } /// The exit-code contract: `Ok(())` (exit 0) when every check passed, `Err` @@ -351,14 +433,85 @@ mod tests { assert_eq!(run_doctor_at(dir.path(), false), Ok(())); let checks = checks(dir.path(), false); - assert_eq!(checks.len(), 1, "one shipped check: {checks:?}"); - assert_eq!(checks[0].status, CheckStatus::Pass); + assert_eq!( + checks.iter().map(|c| c.name).collect::>(), + vec!["store integrity", "fleet ledger"], + "the shipped checks, in report order: {checks:?}" + ); + assert!( + checks.iter().all(|c| c.status == CheckStatus::Pass), + "every check passes on a healthy workspace: {checks:?}" + ); assert!( checks[0].summary.contains("quick_check"), "the pass says what was run: {}", checks[0].summary ); assert!(checks[0].remedy.is_empty(), "a pass advises nothing"); + // No fleet run has happened in this workspace, so the ledger check has + // nothing to open and must say so rather than inventing a file. + assert!( + checks[1].summary.contains("no fleet ledger yet"), + "the ledger check names the absence: {}", + checks[1].summary + ); + } + + /// #617 item 5: an old ledger's orphan rows are surfaced, and `--repair` + /// leaves them alone — removing them would delete fleet history. + #[test] + fn doctor_reports_fleet_ledger_orphans_without_repairing_them() { + let dir = workspace_with_store(); + let db = dir.path().join(".stella/private/fleet.db"); + + // A ledger in the legacy (unconstrained) shape holding an orphan row. + { + let ledger = stella_fleet::Ledger::open(&db).expect("open ledger"); + let _ = ledger; + } + let conn = rusqlite::Connection::open(&db).expect("raw open"); + conn.execute_batch( + "DROP TABLE tasks; + CREATE TABLE tasks ( + run_id TEXT NOT NULL, + task_id TEXT NOT NULL, + title TEXT NOT NULL, + isolation TEXT NOT NULL, + PRIMARY KEY (run_id, task_id) + ); + INSERT INTO tasks (run_id, task_id, title, isolation) + VALUES ('deleted-run', 't1', 'orphaned', 'shared');", + ) + .expect("legacy shape with an orphan"); + drop(conn); + + for repair in [false, true] { + let checks = checks(dir.path(), repair); + let ledger_check = checks + .iter() + .find(|c| c.name == "fleet ledger") + .expect("the ledger check runs"); + assert_eq!( + ledger_check.status, + CheckStatus::Pass, + "inert orphans must not fail the command (repair={repair})" + ); + assert!( + ledger_check + .details + .iter() + .any(|d| d.contains("tasks.run_id")), + "the orphan class is named: {:?}", + ledger_check.details + ); + } + + // The row is still there after `--repair`. + let conn = rusqlite::Connection::open(&db).expect("raw reopen"); + let surviving: i64 = conn + .query_row("SELECT count(*) FROM tasks", [], |r| r.get(0)) + .expect("count"); + assert_eq!(surviving, 1, "--repair must not delete fleet history"); } #[test] diff --git a/stella-cli/tests/doctor_cli.rs b/stella-cli/tests/doctor_cli.rs index cb7cb2fa..39f5a3ee 100644 --- a/stella-cli/tests/doctor_cli.rs +++ b/stella-cli/tests/doctor_cli.rs @@ -73,7 +73,11 @@ fn doctor_reports_a_healthy_store_and_exits_zero() { stdout.contains("store integrity") && stdout.contains("quick_check"), "the check is named and says what it ran: {stdout}" ); - assert!(stdout.contains("1 ok, 0 failed"), "{stdout}"); + assert!( + stdout.contains("fleet ledger"), + "the fleet-ledger check is reported too: {stdout}" + ); + assert!(stdout.contains("2 ok, 0 failed"), "{stdout}"); } #[test] diff --git a/stella-context/src/store/schema.rs b/stella-context/src/store/schema.rs index d7d7cfe7..d108f9f2 100644 --- a/stella-context/src/store/schema.rs +++ b/stella-context/src/store/schema.rs @@ -483,16 +483,17 @@ fn restrict_to_owner(_path: &Path) {} /// message mirrors the one `stella_store::Store::migrate` already writes — the /// fault is an out-of-date binary, not a broken workspace. /// -/// **The version read is outside the transaction.** `unchecked_transaction` is -/// `DEFERRED`, so two processes opening the same fresh workspace at once (a -/// fleet run, or a `stella` session next to a `stella stats`) can both read -/// `user_version = 0` and both try to apply `MIGRATION_V1`; the loser reports +/// **The version is read inside a `BEGIN IMMEDIATE` transaction**, and that is +/// load-bearing rather than incidental. Read outside — or inside a `DEFERRED` +/// transaction, which takes its snapshot before it takes the write lock — two +/// processes opening the same fresh workspace at once (a fleet run, or a +/// `stella` session next to a `stella stats`) could both read +/// `user_version = 0` and both try to apply `MIGRATION_V1`; the loser reported /// SQLITE_BUSY or "table node already exists" instead of the "already migrated" -/// no-op it should. Re-reading `user_version` inside a `BEGIN IMMEDIATE` closes -/// the window — an audit note, not a fix, because the fix belongs with a test -/// that opens the same fresh path from two threads. +/// no-op it should (#617 item 8, matching `stella_store::migrations`). pub(crate) fn migrate(conn: &Connection) -> Result<(), ContextError> { - let version: i64 = conn.query_row("PRAGMA user_version", [], |r| r.get(0))?; + let tx = rusqlite::Transaction::new_unchecked(conn, rusqlite::TransactionBehavior::Immediate)?; + let version: i64 = tx.query_row("PRAGMA user_version", [], |r| r.get(0))?; if version > SCHEMA_VERSION { return Err(ContextError::SchemaTooNew(format!( "context.db is at schema version {version}, but this build only \ @@ -506,7 +507,6 @@ pub(crate) fn migrate(conn: &Connection) -> Result<(), ContextError> { if version == SCHEMA_VERSION { return Ok(()); } - let tx = conn.unchecked_transaction()?; if version < 1 { tx.execute_batch(MIGRATION_V1)?; } diff --git a/stella-context/src/store/tests.rs b/stella-context/src/store/tests.rs index 644b09ac..ab641f49 100644 --- a/stella-context/src/store/tests.rs +++ b/stella-context/src/store/tests.rs @@ -288,6 +288,9 @@ fn kill_mid_index_rolls_back_to_a_consistent_store() { struct CountingEmbedder { inner: crate::embed::HashEmbedder, embedded: std::sync::atomic::AtomicUsize, + /// The largest single `embed` request seen — proves the caller batches + /// rather than handing the backend a whole delta at once (#616 item 8). + max_request: std::sync::atomic::AtomicUsize, } impl CountingEmbedder { @@ -295,12 +298,17 @@ impl CountingEmbedder { Arc::new(Self { inner: crate::embed::HashEmbedder::with_revision(revision), embedded: std::sync::atomic::AtomicUsize::new(0), + max_request: std::sync::atomic::AtomicUsize::new(0), }) } fn count(&self) -> usize { self.embedded.load(Ordering::SeqCst) } + + fn max_request(&self) -> usize { + self.max_request.load(Ordering::SeqCst) + } } #[async_trait::async_trait] @@ -313,10 +321,53 @@ impl crate::embed::Embedder for CountingEmbedder { texts: &[String], ) -> Result, crate::embed::EmbedError> { self.embedded.fetch_add(texts.len(), Ordering::SeqCst); + self.max_request.fetch_max(texts.len(), Ordering::SeqCst); self.inner.embed(texts).await } } +/// #616 item 8: `upsert` embeds in `warm::BATCH`-sized requests. It used to +/// hand the embedder every missing body in one call, so a full-workspace first +/// index put the entire corpus in a single request — the exact shape a backend +/// with a request-size limit rejects, and the reason the warm indexer has +/// batched since it shipped. +#[tokio::test] +async fn upsert_embeds_in_bounded_batches_not_one_request_for_the_whole_delta() { + let dir = TempDir::new().unwrap(); + let path = dir.path().join("context.db"); + let embedder = CountingEmbedder::new("1"); + let store = ContextStore::open_with(&path, embedder.clone(), Arc::new(SystemClock)).unwrap(); + + // Comfortably more than one batch, with distinct content so nothing is + // deduped away before it reaches the embedder. + let nodes = crate::warm::BATCH * 2 + 5; + let mut delta = crate::writeback::ContextDelta::new(); + for i in 0..nodes { + delta = delta.with_node( + NodeInput::new(NodeKind::Concept, format!("node-{i}")) + .with_content(format!("distinct body number {i}")), + ); + } + store.upsert(delta).await.unwrap(); + + assert_eq!( + embedder.count(), + nodes, + "every distinct body is embedded exactly once" + ); + assert!( + embedder.max_request() <= crate::warm::BATCH, + "no single embed request may exceed warm::BATCH ({}), saw {}", + crate::warm::BATCH, + embedder.max_request() + ); + assert!( + embedder.max_request() > 1, + "batches should be filled, not sent one text at a time (saw {})", + embedder.max_request() + ); +} + #[tokio::test] async fn recall_never_embeds_stored_content_inline_only_the_query() { // L-C1: the first query does not pay indexing. Seed content (embedded diff --git a/stella-context/src/warm.rs b/stella-context/src/warm.rs index f453b009..fb8ba935 100644 --- a/stella-context/src/warm.rs +++ b/stella-context/src/warm.rs @@ -44,7 +44,11 @@ use crate::store::{nodes_missing_embedding, open_connection, store_embedding}; /// How many nodes are embedded and committed per transaction. Batching bounds /// what a kill mid-index loses (`L-L1`) and keeps a backend with a /// request-size limit from being handed the whole corpus at once. -const BATCH: usize = 64; +/// +/// `pub(crate)` because [`crate::writeback`]'s upsert embeds against the same +/// backend and had the same unbounded-request problem; one width, not two +/// (#616 item 8). +pub(crate) const BATCH: usize = 64; /// The stop flag shared by a [`crate::ContextStore`] and its background warm /// task. Cloneable and cheap; the store holds one, the task the other. diff --git a/stella-context/src/writeback.rs b/stella-context/src/writeback.rs index 111c6912..06b38576 100644 --- a/stella-context/src/writeback.rs +++ b/stella-context/src/writeback.rs @@ -18,6 +18,7 @@ use crate::store::{ insert_episode, insert_memory, node_by_id, sha256_hex, store_embedding, tag_edge_domains, tag_node_domains, to_hex, upsert_domain, upsert_node, }; +use crate::warm; /// How an episode turned out. Stored as its `as_str` form. #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] @@ -556,9 +557,20 @@ impl ContextStore { // of a large delta (a full-workspace first index is exactly the // case where the bodies are biggest). let (hashes, texts): (Vec, Vec) = missing.into_iter().unzip(); - let embeddings = self.embedder().embed(&texts).await?; - for (hash, emb) in hashes.into_iter().zip(embeddings) { - new_vectors.push((hash, emb.vector)); + // One request per `warm::BATCH` bodies rather than one request for + // the whole delta — a full-workspace first index handed a backend + // with a request-size limit the entire corpus in a single call. + // Same width as the warm indexer so there is one number to tune + // (#616 item 8). `chunks` borrows the bodies, so the peak-memory + // property the split above protects is preserved; only the short + // hash strings are copied per chunk. + for (hash_chunk, text_chunk) in + hashes.chunks(warm::BATCH).zip(texts.chunks(warm::BATCH)) + { + let embeddings = self.embedder().embed(text_chunk).await?; + for (hash, emb) in hash_chunk.iter().zip(embeddings) { + new_vectors.push((hash.clone(), emb.vector)); + } } } diff --git a/stella-core/src/driver.rs b/stella-core/src/driver.rs index b6fdede0..73c5cfa7 100644 --- a/stella-core/src/driver.rs +++ b/stella-core/src/driver.rs @@ -1152,11 +1152,22 @@ impl<'a> Engine<'a> { // than incidental: `complete` owns the gate (and with // it the channel's send half), so the pump can only // finish after `complete` has. Polling `complete` - // first means the `unreachable!` cannot be reached by - // an unlucky randomized poll order (#560). + // first means the pump arm cannot be taken by an + // unlucky randomized poll order (#560). + // + // That arm is believed unreachable, but it reports a + // typed terminal error rather than panicking: this is + // library code on a provider-driven path, where + // invariant 5 (no panics on runtime data) outranks + // asserting a structural claim (#618 item 17). biased; result = bounded_generation(self.config.model_timeout, &mut complete) => result, - _ = &mut pump => unreachable!("the gate keeps the speculation channel open"), + _ = &mut pump => Err(ProviderError::Terminal( + "speculation pump ended before the model call that feeds it; \ + the speculation gate holds the channel open for the whole call, \ + so this indicates the gate was dropped early" + .into(), + )), }; drop(complete); result.map(|result| (result, pump)) diff --git a/stella-fleet/src/ledger.rs b/stella-fleet/src/ledger.rs index 9e9b702d..786776c1 100644 --- a/stella-fleet/src/ledger.rs +++ b/stella-fleet/src/ledger.rs @@ -125,6 +125,60 @@ impl Ledger { Self::init(conn) } + /// Rows whose `run_id` names a run that is no longer in `runs`, per table. + /// + /// Only ever non-empty on a ledger created before `FRESH_SCHEMA`, which + /// constrains these four columns; existing files were deliberately left + /// unconstrained because retrofitting them can only be done by deleting + /// this history (#617 item 5). Reporting is therefore the whole remedy: + /// nothing reads a row by orphaned run today, so the rows are inert, but an + /// operator should be able to see them rather than have them silently + /// removed. Surfaced by `stella doctor`. + /// + /// Classes with a zero count are omitted, so an empty result means clean. + pub fn orphan_rows(&self) -> Result, LedgerError> { + let mut found = Vec::new(); + for (table, column) in RUN_REFERENCES { + // The table list is a compile-time constant, never user input. + let count: i64 = self.conn.query_row( + &format!( + "SELECT count(*) FROM {table} + WHERE {column} NOT IN (SELECT id FROM runs)" + ), + [], + |row| row.get(0), + )?; + if count > 0 { + found.push(OrphanRows { + table, + column, + count, + }); + } + } + Ok(found) + } + + /// Whether this ledger's `tasks.run_id` carries the `REFERENCES runs (id)` + /// constraint — i.e. whether it was created by `FRESH_SCHEMA` rather than the + /// legacy migration ladder. + /// + /// Read from `sqlite_master` rather than tracked in `user_version`, because + /// version alone cannot answer it: both variants are v2. + pub fn enforces_run_references(&self) -> Result { + let ddl: Option = self + .conn + .query_row( + "SELECT sql FROM sqlite_master WHERE type = 'table' AND name = 'tasks'", + [], + |row| row.get(0), + ) + .optional()?; + Ok(ddl + .map(|sql| sql.contains("REFERENCES runs")) + .unwrap_or(false)) + } + fn init(conn: Connection) -> Result { // execute_batch tolerates the row PRAGMA journal_mode returns (a // plain pragma_update errors on it). @@ -374,23 +428,35 @@ const SCHEMA_VERSION: i64 = 2; /// error for anyone who downgrades and belongs in a deliberate change with a /// migration story, not here. /// -/// **The version is read before the transaction opens**, which two processes -/// opening the same un-migrated `fleet.db` at once can both observe. The -/// transaction is DEFERRED, so the second one takes its read snapshot first and -/// then tries to write — in WAL that is `SQLITE_BUSY_SNAPSHOT`, which -/// `busy_timeout` does not retry, so one `open` can fail outright on that race. -/// It only bites on the first open of a file (an already-stamped ledger takes -/// the early return above and never opens a transaction), and today's steps -/// happen to be replay-safe. A step that is NOT replay-safe — an -/// `ALTER TABLE … ADD COLUMN`, say — must not be added without first moving the -/// version read inside an IMMEDIATE transaction, or the loser of the race -/// applies it twice. +/// **The version is read inside an IMMEDIATE transaction**, so two processes +/// opening the same un-migrated `fleet.db` at once cannot both observe the +/// pre-migration version and both apply the ladder. That race previously +/// surfaced as `SQLITE_BUSY_SNAPSHOT` on one of the two opens — which +/// `busy_timeout` does not retry — and it is also what made appending a +/// non-replay-safe step (an `ALTER TABLE … ADD COLUMN`) unsafe, because the +/// loser applied it twice. Both are closed (#617 item 8). fn migrate(conn: &Connection) -> Result<(), LedgerError> { - let version: i64 = conn.query_row("PRAGMA user_version", [], |r| r.get(0))?; + // IMMEDIATE, and the version is read inside it: a DEFERRED transaction + // snapshots before it locks, so two processes opening the same + // un-migrated `fleet.db` could both read the same version and both apply + // the ladder — in WAL the loser hits SQLITE_BUSY_SNAPSHOT, which + // `busy_timeout` does not retry, so one `open` failed outright. This is + // also what makes a non-replay-safe step (an `ALTER TABLE … ADD COLUMN`) + // safe to append below; before, it would have been applied twice + // (#617 item 8). + let tx = rusqlite::Transaction::new_unchecked(conn, rusqlite::TransactionBehavior::Immediate)?; + let version: i64 = tx.query_row("PRAGMA user_version", [], |r| r.get(0))?; if version >= SCHEMA_VERSION { return Ok(()); } - let tx = conn.unchecked_transaction()?; + // A genuinely fresh file gets the referential integrity an existing one + // cannot be given — see [`FRESH_SCHEMA`]. + if version == 0 && !any_fleet_table_exists(&tx)? { + tx.execute_batch(FRESH_SCHEMA)?; + tx.pragma_update(None, "user_version", SCHEMA_VERSION)?; + tx.commit()?; + return Ok(()); + } if version < 1 { tx.execute_batch(MIGRATION_V1)?; } @@ -402,6 +468,114 @@ fn migrate(conn: &Connection) -> Result<(), LedgerError> { Ok(()) } +/// Whether any ledger table is already present — the fresh-vs-legacy probe, +/// since `user_version = 0` means both "brand new file" and "created before +/// versioning existed". +fn any_fleet_table_exists(conn: &Connection) -> Result { + let count: i64 = conn.query_row( + "SELECT count(*) FROM sqlite_master WHERE type = 'table' + AND name IN ('runs', 'tasks', 'attempts', 'commits', 'lineage', 'spend')", + [], + |row| row.get(0), + )?; + Ok(count > 0) +} + +/// One orphan class: rows in `table` whose `column` names a run that is not in +/// `runs`. Produced by [`Ledger::orphan_rows`]. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct OrphanRows { + pub table: &'static str, + pub column: &'static str, + pub count: i64, +} + +/// The four `run_id` columns that a fresh file constrains and an existing one +/// does not. Kept beside `FRESH_SCHEMA` so the report and the constraints +/// cannot drift apart. +const RUN_REFERENCES: [(&str, &str); 4] = [ + ("tasks", "run_id"), + ("attempts", "run_id"), + ("lineage", "parent_run_id"), + ("spend", "run_id"), +]; + +/// The schema a **brand-new** `fleet.db` is created with: the current shape +/// (post-v2 `lineage`) plus the `REFERENCES runs (id)` constraints that +/// `tasks`, `attempts`, `lineage` and `spend` have always been missing. +/// +/// **Why fresh files only** (#617 item 5). Retrofitting these constraints onto +/// a deployed `fleet.db` is a table rebuild, and `apply_migration`-style +/// runners abort on `pragma_foreign_key_check` — so on any file that already +/// holds a row naming a deleted run, the migration fails and the ledger stops +/// opening. The only way through is deleting those rows, which is deleting a +/// user's fleet history: which tasks ran, what was attempted, what it cost. +/// The issue filed this as a routine schema tidy-up and did not say that. +/// So new files get enforcement, existing files are left exactly as they are, +/// and [`Ledger::orphan_rows`] reports what an unconstrained file holds +/// (surfaced by `stella doctor`). +/// +/// **Consequence a future migration author must know:** `SCHEMA_VERSION` no +/// longer determines shape by itself. Two files can both be at v2 — one with +/// these constraints and one without. A step that rebuilds any of these four +/// tables has to reproduce the right variant, or read the existing DDL from +/// `sqlite_master` rather than assuming. The alternative was worse: silently +/// deleting history, or leaving new databases unconstrained forever. +const FRESH_SCHEMA: &str = "\ +CREATE TABLE runs ( + id TEXT PRIMARY KEY, + root_task_count INTEGER NOT NULL, + created_at_ms INTEGER NOT NULL +); +CREATE TABLE tasks ( + run_id TEXT NOT NULL REFERENCES runs (id), + task_id TEXT NOT NULL, + title TEXT NOT NULL, + isolation TEXT NOT NULL, + PRIMARY KEY (run_id, task_id) +); +CREATE TABLE attempts ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + run_id TEXT NOT NULL REFERENCES runs (id), + task_id TEXT NOT NULL, + worktree_path TEXT NOT NULL, + branch TEXT NOT NULL, + started_at_ms INTEGER NOT NULL, + finished_at_ms INTEGER, + success INTEGER, + summary TEXT +); +CREATE INDEX attempts_by_task ON attempts (run_id, task_id); +CREATE TABLE commits ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + attempt_id INTEGER NOT NULL REFERENCES attempts (id), + run_id TEXT NOT NULL, + task_id TEXT NOT NULL, + sha TEXT NOT NULL, + branch TEXT NOT NULL, + message TEXT NOT NULL, + timestamp_ms INTEGER NOT NULL +); +CREATE INDEX commits_by_task ON commits (run_id, task_id); +CREATE TABLE lineage ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + parent_run_id TEXT NOT NULL REFERENCES runs (id), + child_task_id TEXT NOT NULL, + recorded_at_ms INTEGER NOT NULL, + UNIQUE (parent_run_id, child_task_id) +); +CREATE INDEX lineage_by_parent ON lineage (parent_run_id); +CREATE TABLE spend ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + run_id TEXT NOT NULL REFERENCES runs (id), + task_id TEXT NOT NULL, + attempt_id INTEGER NOT NULL REFERENCES attempts (id), + cost_usd REAL NOT NULL, + recorded_at_ms INTEGER NOT NULL +); +CREATE INDEX spend_by_run ON spend (run_id); +"; + /// v1 — the schema as it originally shipped (unversioned). Every statement is /// `IF NOT EXISTS`, so this doubles as the retrofit step for files created /// before `user_version` was stamped. @@ -489,6 +663,95 @@ CREATE INDEX IF NOT EXISTS lineage_by_parent ON lineage (parent_run_id); mod tests { use super::*; + /// #617 item 5: a brand-new ledger enforces the run references, so no new + /// orphan can ever be created. + #[test] + fn a_fresh_ledger_enforces_run_references_and_rejects_an_unknown_run() { + let ledger = Ledger::open_in_memory().expect("open"); + assert!(ledger.enforces_run_references().expect("ddl")); + assert!(ledger.orphan_rows().expect("scan").is_empty()); + + // Inserting a task for a run that does not exist must now fail. + let err = ledger.conn.execute( + "INSERT INTO tasks (run_id, task_id, title, isolation) + VALUES ('ghost', 't1', 'title', 'shared')", + [], + ); + assert!( + err.is_err(), + "a fresh ledger must reject a task naming an unknown run" + ); + } + + /// The other half of the ruling: a ledger that came up the legacy ladder is + /// left unconstrained — opening it must NOT fail, and must not delete the + /// orphan history it already holds. It is reported instead. + #[test] + fn a_legacy_ledger_keeps_its_orphans_and_reports_them() { + // Build a v0 file the old way, with an orphan row already in it. + let conn = Connection::open_in_memory().expect("conn"); + conn.execute_batch(MIGRATION_V1).expect("legacy schema"); + conn.execute( + "INSERT INTO tasks (run_id, task_id, title, isolation) + VALUES ('deleted-run', 't1', 'orphaned', 'shared')", + [], + ) + .expect("orphan row is accepted by the legacy shape"); + // `spend.attempt_id` has always been a real foreign key, so the attempt + // has to exist — its own `run_id` is the orphaned part. + conn.execute( + "INSERT INTO attempts (run_id, task_id, worktree_path, branch, started_at_ms) + VALUES ('deleted-run', 't1', '/w', 'fleet/t1', 0)", + [], + ) + .expect("orphan attempt row"); + let attempt_id: i64 = conn.last_insert_rowid(); + conn.execute( + "INSERT INTO spend (run_id, task_id, attempt_id, cost_usd, recorded_at_ms) + VALUES ('deleted-run', 't1', ?1, 1.0, 0)", + params![attempt_id], + ) + .expect("orphan spend row"); + + // Now let the ledger take it through migrate() as a deployed file. + let ledger = Ledger::init(conn).expect("a legacy file with orphans still opens"); + + assert!( + !ledger.enforces_run_references().expect("ddl"), + "an existing file is left unconstrained on purpose" + ); + + let orphans = ledger.orphan_rows().expect("scan"); + assert_eq!( + orphans, + vec![ + OrphanRows { + table: "tasks", + column: "run_id", + count: 1 + }, + OrphanRows { + table: "attempts", + column: "run_id", + count: 1 + }, + OrphanRows { + table: "spend", + column: "run_id", + count: 1 + }, + ], + "every orphan class is reported, none deleted" + ); + + // And the history is still there. + let surviving: i64 = ledger + .conn + .query_row("SELECT count(*) FROM tasks", [], |r| r.get(0)) + .expect("count"); + assert_eq!(surviving, 1, "the orphan row must not have been deleted"); + } + fn task(id: &str) -> Task { Task::new(id, format!("title {id}"), "prompt") } diff --git a/stella-media/src/error.rs b/stella-media/src/error.rs index 2fa09441..6660173e 100644 --- a/stella-media/src/error.rs +++ b/stella-media/src/error.rs @@ -93,6 +93,16 @@ pub enum MediaError { #[error("host journal sidecar race during concurrent first-open: {0}")] SidecarRace(String), + /// The host operation journal could not be opened, configured, or read + /// (I/O, a rejected path, a poisoned mutex, a failed statement). Terminal. + /// + /// Distinct from [`MediaError::Artifact`]: that one names the artifact + /// store under `.stella/artifacts/`, this one names the journal sidecar + /// database. Both used to surface as `Artifact`, which sent operators + /// looking in the wrong directory. + #[error("host journal error: {0}")] + Journal(String), + /// A non-retryable provider failure not covered above (4xx other than /// auth/rate-limit, 5xx that exhausted retries upstream). #[error("terminal media provider error: {0}")] @@ -140,6 +150,7 @@ mod tests { // A concurrent-first-open sidecar race is handled internally by the // journal's own retry, never by a caller's is_retryable loop. assert!(!MediaError::SidecarRace("racing".into()).is_retryable()); + assert!(!MediaError::Journal("cannot open journal".into()).is_retryable()); } #[test] diff --git a/stella-media/src/operation_journal.rs b/stella-media/src/operation_journal.rs index 32ee3af1..29c7898c 100644 --- a/stella-media/src/operation_journal.rs +++ b/stella-media/src/operation_journal.rs @@ -589,7 +589,7 @@ fn sql_error(error: rusqlite::Error) -> MediaError { } fn journal_error(message: impl Into) -> MediaError { - MediaError::Artifact(message.into()) + MediaError::Journal(message.into()) } struct PreparedPrivateSqlite { diff --git a/stella-store/src/enterprise_telemetry/migrations.rs b/stella-store/src/enterprise_telemetry/migrations.rs index 72653f42..e4caf347 100644 --- a/stella-store/src/enterprise_telemetry/migrations.rs +++ b/stella-store/src/enterprise_telemetry/migrations.rs @@ -104,10 +104,21 @@ pub(super) fn migrate_store_export_schema(conn: &mut Connection) -> Result<()> { [], |row| row.get(0), )?; + // `state_exists` is read outside a transaction, so it is an + // optimization and not a guarantee: two processes opening the same + // store concurrently both see no state row and both insert, and the + // loser used to fail the whole open on + // `UNIQUE constraint failed: enterprise_export_migration.singleton` + // — a fresh-workspace crash, since this runs on every `Store::open`. + // `DO NOTHING` rather than the sibling branch's `DO UPDATE`: both + // racers computed `is_complete` from the same ledger state, so the + // first writer's row is already the row this one would write + // (#617 item 8). conn.execute( "INSERT INTO enterprise_export_migration (singleton, version, last_rowid, migrated_rows, batches_completed, is_complete) - VALUES (1, 1, 0, 0, 0, ?1)", + VALUES (1, 1, 0, 0, 0, ?1) + ON CONFLICT(singleton) DO NOTHING", params![i64::from(!empty_nonce_exists)], )?; } diff --git a/stella-store/src/lib.rs b/stella-store/src/lib.rs index aa6c22a4..ae3347b1 100644 --- a/stella-store/src/lib.rs +++ b/stella-store/src/lib.rs @@ -87,7 +87,7 @@ use std::path::{Path, PathBuf}; use std::sync::Mutex; use base64::Engine as _; -use rusqlite::{Connection, OptionalExtension, params}; +use rusqlite::{Connection, OptionalExtension, TransactionBehavior, params}; use serde::Serialize; use stella_protocol::{AgentEvent, TaskItem, TaskStatus, ToolOutput}; @@ -164,6 +164,7 @@ use ddl::TABLES; use integrity::corrupt_store_error; use migrations::{ MIGRATIONS, SCHEMA_VERSION, any_store_table_exists, apply_migration, create_latest_schema, + initialize_store_pragmas, }; pub use cache_gaps::CacheCallGap; @@ -666,13 +667,9 @@ impl Store { // This batch is also the first statement to touch page 1, so it is // where an unreadable file announces itself — mapped here, before the // blanket `From` flattens the error code away. - conn.execute_batch( - "PRAGMA journal_mode=WAL; - PRAGMA synchronous=NORMAL; - PRAGMA busy_timeout=5000; - PRAGMA foreign_keys=ON;", - ) - .map_err(|error| corrupt_store_error(error, db_path))?; + // The batch absorbs a concurrent first-open's `SQLITE_BUSY`; see + // [`initialize_store_pragmas`]. + initialize_store_pragmas(&conn).map_err(|error| corrupt_store_error(error, db_path))?; let store = Self { conn: Mutex::new(conn), root, @@ -705,7 +702,16 @@ impl Store { /// (version stamped inside it — see [`apply_migration`]). fn migrate(&self) -> Result<()> { let mut conn = self.lock(); - let mut version: i64 = conn.query_row("PRAGMA user_version", [], |row| row.get(0))?; + // The fresh-file decision is made under a write lock, because + // "user_version is 0 AND no store table exists" is only true until + // someone else acts on it. Read outside a transaction — or inside a + // DEFERRED one, which snapshots before it locks — two processes + // opening the same fresh workspace (a fleet run beside a + // `stella stats`) both saw a fresh file and both ran + // `create_latest_schema`; the loser failed with "table … already + // exists" instead of no-opping (#617 item 8). + let bootstrap = conn.transaction_with_behavior(TransactionBehavior::Immediate)?; + let mut version: i64 = bootstrap.query_row("PRAGMA user_version", [], |row| row.get(0))?; if version < 0 { // `user_version` is whatever is in the file header, and the header // is not ours to trust: a negative stamp would index MIGRATIONS @@ -732,13 +738,17 @@ impl Store { this workspace." ))); } - if version == 0 && !any_store_table_exists(&conn)? { - let tx = conn.transaction()?; - create_latest_schema(&tx)?; - tx.pragma_update(None, "user_version", SCHEMA_VERSION)?; - tx.commit()?; + if version == 0 && !any_store_table_exists(&bootstrap)? { + create_latest_schema(&bootstrap)?; + bootstrap.pragma_update(None, "user_version", SCHEMA_VERSION)?; + bootstrap.commit()?; return Ok(()); } + // Not a fresh file: release the write lock before the step loop, which + // has to toggle `PRAGMA foreign_keys` between steps and so cannot run + // inside this transaction. Each step re-confirms the version under its + // own IMMEDIATE lock (see `apply_migration`). + drop(bootstrap); while version < SCHEMA_VERSION { let target = version + 1; // PRAGMA foreign_keys is silently ignored inside a transaction diff --git a/stella-store/src/migrations.rs b/stella-store/src/migrations.rs index 944fc4a4..b7d1379f 100644 --- a/stella-store/src/migrations.rs +++ b/stella-store/src/migrations.rs @@ -8,7 +8,7 @@ //! module live in `Store::migrate` (see the crate docs' "Schema //! versioning" section). -use rusqlite::{Connection, params}; +use rusqlite::{Connection, TransactionBehavior, params}; use crate::ddl::{ AGENT_USES_DDL, CONTEXT_BLOCKS_DDL, EXECUTION_REFLECTION_DDL, EXECUTIONS_DDL, FORGOTTEN_DDL, @@ -617,16 +617,81 @@ fn migrate_v11_to_v12(tx: &rusqlite::Transaction<'_>) -> Result<()> { Ok(()) } +/// Runs the connection pragmas, absorbing a concurrent first-open's +/// `SQLITE_BUSY`. +/// +/// `busy_timeout` is set first so every statement after it can wait, but that +/// is not enough on its own: converting a fresh rollback-journal database into +/// WAL needs an exclusive lock, and **SQLite skips the busy handler on that +/// upgrade** to avoid deadlock. So two processes opening the same new workspace +/// at once — a fleet run beside a `stella stats`, or four threads in a test — +/// gave one of them an immediate `database is locked` from `journal_mode=WAL` +/// itself, which no timeout could absorb, and `Store::open` failed outright. +/// Backing off lets the loser observe the settled WAL file instead. +/// +/// Only first opens are affected: once the file is WAL the pragma is a no-op +/// and takes no lock. `stella-media`'s journal already does exactly this +/// (`initialize_journal_database`) — the main store never got the same +/// treatment (#617 item 8). +/// +/// WAL also means a read-only caller (`stella stats`) is never blocked by a +/// live session's writes; `synchronous=NORMAL` is the standard WAL pairing, +/// durability to the last checkpoint rather than one fsync per event insert on +/// the hot render path. +pub(crate) fn initialize_store_pragmas( + conn: &Connection, +) -> std::result::Result<(), rusqlite::Error> { + const BUSY_ATTEMPTS: u32 = 40; + const BUSY_BACKOFF: std::time::Duration = std::time::Duration::from_millis(50); + + let mut attempts = 0; + loop { + // `execute_batch` tolerates the row `PRAGMA journal_mode` returns (a + // plain `pragma_update` errors on it). + let result = conn.execute_batch( + "PRAGMA busy_timeout=5000; + PRAGMA journal_mode=WAL; + PRAGMA synchronous=NORMAL; + PRAGMA foreign_keys=ON;", + ); + match result { + Err(error) + if attempts < BUSY_ATTEMPTS + && error.sqlite_error_code() == Some(rusqlite::ErrorCode::DatabaseBusy) => + { + attempts += 1; + std::thread::sleep(BUSY_BACKOFF); + } + other => return other, + } + } +} + /// Run one migration in its own transaction, stamping `user_version` before /// commit so version and shape can never disagree on disk. The caller has /// already suspended foreign-key enforcement (a no-op inside a /// transaction). +/// +/// The transaction is `IMMEDIATE` and re-reads `user_version` inside it, so +/// the version the caller decided on is re-confirmed under the write lock. +/// A `DEFERRED` transaction took its read snapshot before acquiring the +/// write lock, which let two processes migrating the same file concurrently +/// both apply the same step — the loser either failing on an already-applied +/// reshape or, for a non-idempotent step, applying it twice (#617 item 8). +/// Losing the race is now the no-op it should always have been. pub(crate) fn apply_migration( conn: &mut Connection, migration: Migration, target: i64, ) -> Result<()> { - let tx = conn.transaction()?; + let tx = conn.transaction_with_behavior(TransactionBehavior::Immediate)?; + // Re-read under the write lock: another process may have applied this + // exact step between the caller's read and this transaction. + let current: i64 = tx.query_row("PRAGMA user_version", [], |row| row.get(0))?; + if current >= target { + tx.commit()?; + return Ok(()); + } migration(&tx)?; // lang_altertable §7 requires a full FK audit before committing work // done with enforcement off. No store table declares foreign keys @@ -984,4 +1049,72 @@ mod tests { // Idempotent on a file already at the v16 shape. apply_migration(&mut conn, migrate_v15_to_v16, 16).expect("idempotent"); } + + /// #617 item 8: losing the migration race is a no-op, not an error. The + /// migration body must not run at all when the file is already stamped at + /// or past `target` — that is what makes a non-idempotent step (an + /// `ALTER TABLE … ADD COLUMN`) safe to add later. + #[test] + fn apply_migration_skips_a_step_another_process_already_stamped() { + let mut conn = Connection::open_in_memory().expect("db"); + conn.pragma_update(None, "user_version", 7).expect("stamp"); + + fn must_not_run(_: &rusqlite::Transaction<'_>) -> Result<()> { + Err(StoreError( + "the migration body ran even though the version was already stamped".into(), + )) + } + + // Target 7 against a file at 7: the step was applied by someone else. + apply_migration(&mut conn, must_not_run, 7).expect("a stamped step is skipped"); + // And a step already overtaken by a later version is skipped too. + apply_migration(&mut conn, must_not_run, 5).expect("an overtaken step is skipped"); + + let version: i64 = conn + .query_row("PRAGMA user_version", [], |row| row.get(0)) + .expect("read"); + assert_eq!(version, 7, "a skipped step must not move the version"); + } + + /// #617 item 8, the case the issue asked for by name: two processes opening + /// the same *fresh* workspace at once. Before the bootstrap read moved + /// inside `BEGIN IMMEDIATE`, both could observe `user_version = 0` with no + /// tables and both run `create_latest_schema`, and the loser failed with + /// "table … already exists" instead of no-opping. + #[test] + fn concurrent_first_open_of_a_fresh_workspace_both_succeed() { + let workspace = tempfile::tempdir().expect("tempdir"); + let root = workspace.path().to_path_buf(); + + let barrier = std::sync::Arc::new(std::sync::Barrier::new(4)); + let handles: Vec<_> = (0..4) + .map(|_| { + let root = root.clone(); + let barrier = std::sync::Arc::clone(&barrier); + std::thread::spawn(move || { + // Line the openers up so the version reads genuinely + // interleave rather than serializing by luck of spawn order. + barrier.wait(); + crate::Store::open(&root).map(|_| ()) + }) + }) + .collect(); + + for (i, handle) in handles.into_iter().enumerate() { + handle + .join() + .expect("opener thread panicked") + .unwrap_or_else(|error| { + panic!("opener {i} lost the fresh-open race and failed: {error}") + }); + } + + // The winner's schema is the one on disk, stamped at the current version. + let store = crate::Store::open(&root).expect("reopen"); + let version: i64 = store + .lock() + .query_row("PRAGMA user_version", [], |row| row.get(0)) + .expect("read"); + assert_eq!(version, crate::SCHEMA_VERSION); + } } diff --git a/stella-store/src/private.rs b/stella-store/src/private.rs index c93425b6..e7163a92 100644 --- a/stella-store/src/private.rs +++ b/stella-store/src/private.rs @@ -77,12 +77,78 @@ fn read_committable_file(path: &Path) -> Result<(Vec, u32)> { Ok((bytes, MODE_SHARED)) } +/// Create the generated ignore exclusively, or report that someone else won. +/// +/// `Ok(true)` means this caller created it; `Ok(false)` means it already +/// existed. The exclusivity is the point: the previous code took +/// `symlink_metadata` → `NotFound` → [`write_atomic`], so concurrent first +/// openers of one workspace all observed no file and all renamed over the +/// target. A rename unlinks the inode it replaces, so a racer that had just +/// opened the winner's file for validation saw `nlink() == 0` and failed +/// closed — `Store::open` reported the generated ignore "must be an +/// owner-controlled single-link regular file" for a file nothing had attacked +/// (#617 item 10). +/// +/// `O_NOFOLLOW` alongside `O_EXCL` so this cannot be aimed through a symlink +/// planted in the window, and [`MODE_SHARED`] because the file is meant to be +/// committed — the same mode [`write_atomic`] would have imposed. +#[cfg(unix)] +fn create_generated_ignore_exclusively(path: &Path) -> Result { + use std::io::Write as _; + use std::os::unix::fs::OpenOptionsExt; + + let mut options = std::fs::OpenOptions::new(); + options + .write(true) + .create_new(true) + .mode(MODE_SHARED) + .custom_flags(libc::O_NOFOLLOW | libc::O_CLOEXEC); + match options.open(path) { + Ok(mut file) => { + file.write_all(WORKSPACE_GENERATED_IGNORE) + .and_then(|_| file.sync_all()) + .map_err(|e| StoreError(format!("cannot write {}: {e}", path.display())))?; + Ok(true) + } + Err(error) if error.kind() == std::io::ErrorKind::AlreadyExists => Ok(false), + Err(error) => Err(StoreError(format!( + "cannot create generated ignore {}: {error}", + path.display() + ))), + } +} + +/// Non-unix has no ownership model to race over, so the pre-existing +/// check-then-write is kept as-is rather than pretending to be exclusive. +#[cfg(not(unix))] +fn create_generated_ignore_exclusively(path: &Path) -> Result { + match std::fs::symlink_metadata(path) { + Err(error) if error.kind() == std::io::ErrorKind::NotFound => { + write_atomic(path, WORKSPACE_GENERATED_IGNORE, MODE_SHARED)?; + Ok(true) + } + _ => Ok(false), + } +} + pub(crate) fn ensure_workspace_generated_ignore(dot: &Path) -> Result<()> { let path = dot.join(".gitignore"); + // Try to be the one that creates it. Whoever wins writes content that + // already contains `private/`, so every loser falls through to the read + // below, finds the entry, and returns without writing — no rename races a + // reader in the common case. + if create_generated_ignore_exclusively(&path)? { + return Ok(()); + } let (mut bytes, mode) = match std::fs::symlink_metadata(&path) { Ok(_) => read_committable_file(&path)?, Err(error) if error.kind() == std::io::ErrorKind::NotFound => { - return write_atomic(&path, WORKSPACE_GENERATED_IGNORE, MODE_SHARED); + // Created and then removed between the two calls. Rare enough to + // report rather than loop on. + return Err(StoreError(format!( + "generated ignore {} vanished while being inspected", + path.display() + ))); } Err(error) => { return Err(StoreError(format!( diff --git a/stella-store/src/private_state_tests.rs b/stella-store/src/private_state_tests.rs index ffb9b82f..e2734ba4 100644 --- a/stella-store/src/private_state_tests.rs +++ b/stella-store/src/private_state_tests.rs @@ -252,3 +252,57 @@ fn private_state_creation_ignores_an_ambient_zero_umask() { 0o600 ); } + +/// The generated `.gitignore` is created with `O_EXCL | O_NOFOLLOW` (#617 +/// item 10), so the exclusive-create path must not become a way to write +/// *through* a symlink someone planted at that name. `create_new` sees the +/// existing symlink and reports `AlreadyExists`, and the reader that follows +/// rejects it with `O_NOFOLLOW` — either way the target is never touched. +#[cfg(unix)] +#[test] +fn generated_ignore_creation_rejects_a_planted_symlink_without_writing_through_it() { + let root = tempfile::tempdir().unwrap(); + let dot = root.path().join(".stella"); + std::fs::create_dir_all(&dot).unwrap(); + + let outside = root.path().join("outside.txt"); + std::fs::write(&outside, b"untouched\n").unwrap(); + std::os::unix::fs::symlink(&outside, dot.join(".gitignore")).unwrap(); + + let error = match Store::open(root.path()) { + Err(error) => error, + Ok(_) => panic!("a symlinked generated ignore must be rejected"), + }; + assert!( + error.to_string().contains(".gitignore"), + "the error should name the offending path, got: {error}" + ); + assert_eq!( + std::fs::read(&outside).unwrap(), + b"untouched\n", + "the symlink target must not be written through" + ); +} + +/// The mode the exclusive create imposes is the committable one — the file is +/// meant to be checked in, so it must not land 0600 like private state. +#[cfg(unix)] +#[test] +fn generated_ignore_is_created_world_readable_not_owner_only() { + use std::os::unix::fs::PermissionsExt; + + let root = tempfile::tempdir().unwrap(); + Store::open(root.path()).expect("open"); + + let ignore = root.path().join(".stella/.gitignore"); + let mode = std::fs::metadata(&ignore).unwrap().permissions().mode() & 0o777; + assert_eq!( + mode, 0o644, + "the generated ignore is committable, not private" + ); + let bytes = std::fs::read(&ignore).unwrap(); + assert!( + bytes.split(|b| *b == b'\n').any(|line| line == b"private/"), + "the generated ignore must exclude the private dir" + ); +} diff --git a/stella-store/src/sessions.rs b/stella-store/src/sessions.rs index 02e25931..6feb9323 100644 --- a/stella-store/src/sessions.rs +++ b/stella-store/src/sessions.rs @@ -108,6 +108,43 @@ pub struct SessionRecord { pub exploring: Vec, } +/// A `.json` that is present but unusable — the state +/// [`SessionRegistry::list`] silently drops. +/// +/// It is reported rather than repaired because the record cannot be rebuilt: +/// `id`, `pid` and `started_at_ms` are recoverable from the filename (ids are +/// self-minted `ses--`), but `workspace` is not held anywhere else on +/// disk — not in `journal.jsonl`, not in `history.json` — and resuming needs +/// it. So the honest outcome is to surface the session as damaged, keep its +/// sidecar, and let a human decide, instead of silently hiding it (today) or +/// silently deleting it (what #617 item 12 proposed). +#[derive(Debug, Clone)] +pub struct DamagedRecord { + /// The record's id, taken from the filename. + pub id: String, + /// The unreadable record file. + pub path: PathBuf, + /// Why it could not be used — zero-length, or the parse error. + pub reason: String, + /// Whether a sidecar directory sits beside it. When true there is very + /// likely a recoverable `history.json` inside, which is what makes + /// deleting this session unacceptable. + pub has_sidecar: bool, +} + +/// What [`SessionRegistry::scan`] found, with healthy, damaged, and genuinely +/// orphaned state kept apart. See that method for why the distinction exists. +#[derive(Debug, Default)] +pub struct RegistryScan { + /// Records that parsed, newest-started first, liveness downgrade applied. + pub healthy: Vec, + /// Records present on disk but unusable. Their sidecars are intact. + pub damaged: Vec, + /// Sidecar directory names with no `.json` beside them at all — the + /// only state that is safe to reclaim. + pub orphan_sidecars: Vec, +} + impl SessionRecord { /// A fresh in-progress record for this process, timestamped now. pub fn new(workspace: impl Into, title: impl Into) -> Self { @@ -196,28 +233,125 @@ impl SessionRegistry { /// applied: a live-status record whose pid is gone is shown as `Error` /// (the session crashed without writing a terminal status). Unreadable /// files are skipped — one corrupt record never hides the rest. + /// + /// A skipped record is *invisible*, not *absent*: its `.json` is still + /// on disk. Anything deciding whether state may be deleted must use + /// [`Self::scan`], which keeps the two apart. pub fn list(&self) -> Vec { + self.scan().healthy + } + + /// The registry directory as it actually is on disk, with the three states + /// [`Self::list`] flattens into one kept apart. + /// + /// `list` reads `.json` and drops anything that fails to parse, so a + /// damaged record and a missing record are indistinguishable through it. + /// That conflation is load-bearing for anything destructive: `upsert`'s own + /// comment records that a power cut leaves a **zero-length `.json`**, + /// which `list` skips — so a sweep that deletes "sidecars `list` doesn't + /// account for" deletes the conversation of a session whose record was + /// merely truncated, and `history.json` is the only continuable copy + /// (#617 item 12). + /// + /// So the orphan test here is "is there a `.json` at all", answered + /// from the directory entry and never from a parse. + pub fn scan(&self) -> RegistryScan { let Ok(entries) = std::fs::read_dir(&self.dir) else { - return Vec::new(); + return RegistryScan::default(); }; - let mut records: Vec = entries - .filter_map(|entry| { - let entry = entry.ok()?; - if !entry.file_type().ok()?.is_file() { - return None; + + let mut scan = RegistryScan::default(); + let mut record_names: std::collections::HashSet = std::collections::HashSet::new(); + let mut sidecar_names: Vec = Vec::new(); + + for entry in entries.flatten() { + let Ok(file_type) = entry.file_type() else { + continue; + }; + let path = entry.path(); + if file_type.is_dir() { + if let Some(name) = path.file_name().and_then(|n| n.to_str()) { + sidecar_names.push(name.to_string()); } - let path = entry.path(); - if path.extension().and_then(|e| e.to_str()) != Some("json") { - return None; + continue; + } + if !file_type.is_file() || path.extension().and_then(|e| e.to_str()) != Some("json") { + continue; + } + let Some(stem) = path + .file_stem() + .and_then(|s| s.to_str()) + .map(str::to_string) + else { + continue; + }; + // Recorded BEFORE the parse: the file's existence is what makes a + // sidecar non-orphaned, whether or not its bytes are usable. + record_names.insert(stem.clone()); + + match crate::read_private_to_string(&path) + .map_err(|error| error.to_string()) + .and_then(|text| { + serde_json::from_str::(&text).map_err(|error| { + if text.is_empty() { + "record is zero-length (an interrupted write)".to_string() + } else { + error.to_string() + } + }) + }) { + Ok(mut record) => { + record.status = Self::presented_status(&record); + scan.healthy.push(record); } - let text = crate::read_private_to_string(&path).ok()?; - let mut record: SessionRecord = serde_json::from_str(&text).ok()?; - record.status = Self::presented_status(&record); - Some(record) - }) + Err(reason) => scan.damaged.push(DamagedRecord { + id: stem, + has_sidecar: path.with_extension("").is_dir(), + reason, + path, + }), + } + } + + scan.orphan_sidecars = sidecar_names + .into_iter() + .filter(|name| !record_names.contains(name)) .collect(); - records.sort_by_key(|r| std::cmp::Reverse(r.started_at_ms)); - records + + scan.healthy + .sort_by_key(|r| std::cmp::Reverse(r.started_at_ms)); + scan.damaged.sort_by(|a, b| a.id.cmp(&b.id)); + scan.orphan_sidecars.sort(); + scan + } + + /// Delete sidecar directories that have no `.json` beside them at all. + /// + /// The narrow definition is the safety property: a directory whose record + /// exists but cannot be parsed is **not** an orphan and is never touched + /// here (see [`Self::scan`]). Returns the ids removed. + /// + /// This reclaims the case [`Self::upsert`]'s comment calls "strands its + /// sidecar" — a record deleted while its directory survived — without + /// being able to reach a session whose record is merely damaged. + pub fn prune_orphan_sidecars(&self) -> Result> { + let mut removed = Vec::new(); + for id in self.scan().orphan_sidecars { + let dir = self.dir.join(&id); + // Re-check under the same name we are about to delete: a session + // that started between the scan and here has written its record. + if self.dir.join(format!("{id}.json")).exists() { + continue; + } + std::fs::remove_dir_all(&dir).map_err(|error| { + StoreError(format!( + "cannot remove orphan session sidecar {}: {error}", + dir.display() + )) + })?; + removed.push(id); + } + Ok(removed) } /// Read one record (no liveness downgrade — the raw stored state). @@ -507,6 +641,93 @@ mod tests { assert_eq!(std::fs::read_to_string(&target).unwrap(), "outside"); } + /// #617 item 12, the case that made the sweep unacceptable as filed. A + /// power cut leaves a zero-length `.json`; `list` skips it, so the + /// session looks accounted-for by nothing. The sweep must still not touch + /// it, because `history.json` beside it is the only continuable copy of the + /// conversation. + #[test] + fn a_power_cut_record_is_damaged_not_orphaned_and_survives_the_sweep() { + let (dir, reg) = temp_registry("powercut"); + let rec = SessionRecord::new("/w", "interrupted"); + reg.upsert(&rec).unwrap(); + + // The session had written a conversation snapshot. + let sidecar = reg.sidecar_dir(&rec.id); + std::fs::create_dir_all(&sidecar).unwrap(); + let history = sidecar.join(crate::journal::HISTORY_FILE); + std::fs::write(&history, b"[{\"role\":\"user\"}]").unwrap(); + + // Now the power cut: the record is published but zero-length. + std::fs::write(reg.path_for(&rec.id), b"").unwrap(); + + // `list` cannot see it — that is the trap. + assert!( + reg.list().is_empty(), + "a zero-length record is invisible to list; that is the premise" + ); + + // `scan` calls it damaged, NOT an orphan. + let scan = reg.scan(); + assert!(scan.healthy.is_empty()); + assert_eq!(scan.damaged.len(), 1, "the record is damaged"); + assert!( + scan.damaged[0].reason.contains("zero-length"), + "the reason should name the interrupted write, got: {}", + scan.damaged[0].reason + ); + assert!( + scan.damaged[0].has_sidecar, + "the damaged record must be reported as having recoverable state" + ); + assert!( + scan.orphan_sidecars.is_empty(), + "a sidecar whose record exists is never an orphan, got {:?}", + scan.orphan_sidecars + ); + + // And the sweep leaves the conversation alone. + assert!(reg.prune_orphan_sidecars().unwrap().is_empty()); + assert!( + history.exists(), + "the sweep deleted a resumable conversation" + ); + let _ = std::fs::remove_dir_all(&dir); + } + + /// The other half: a sidecar with genuinely no record beside it is the + /// "stranded sidecar" `upsert`'s comment names, and IS reclaimable. + #[test] + fn a_sidecar_with_no_record_at_all_is_swept() { + let (dir, reg) = temp_registry("orphan"); + crate::ensure_private_dir(&dir).unwrap(); + + let stranded = reg.sidecar_dir("ses-1-1"); + std::fs::create_dir_all(&stranded).unwrap(); + std::fs::write(stranded.join(crate::journal::HISTORY_FILE), b"[]").unwrap(); + + // A healthy session alongside it must be untouched. + let live = SessionRecord::new("/w", "live"); + reg.upsert(&live).unwrap(); + let live_sidecar = reg.sidecar_dir(&live.id); + std::fs::create_dir_all(&live_sidecar).unwrap(); + + let scan = reg.scan(); + assert_eq!(scan.orphan_sidecars, vec!["ses-1-1".to_string()]); + assert!(scan.damaged.is_empty()); + + assert_eq!( + reg.prune_orphan_sidecars().unwrap(), + vec!["ses-1-1".to_string()] + ); + assert!(!stranded.exists(), "the orphan should be reclaimed"); + assert!( + live_sidecar.exists(), + "a live session's sidecar is not an orphan" + ); + let _ = std::fs::remove_dir_all(&dir); + } + #[test] fn latest_resumable_wants_matching_workspace_state_and_no_live_owner() { let (dir, reg) = temp_registry("resumable"); diff --git a/stella-tools/README.md b/stella-tools/README.md index d18de20f..54c22cb1 100644 --- a/stella-tools/README.md +++ b/stella-tools/README.md @@ -43,7 +43,7 @@ the single credential deny-list rather than growing a second one that drifts. | [`src/project.rs`](src/project.rs), [`src/scripts.rs`](src/scripts.rs), [`src/diagnostics.rs`](src/diagnostics.rs), [`src/impact.rs`](src/impact.rs) | Build/test/lint/format as thin verbs over the scripts index, structured typecheck output, and the importer-edge blast radius behind `run_tests` `scope: "impacted"`. | | [`src/process.rs`](src/process.rs) | The long-running process group (`start_process`/`read_output`/`send_stdin`/`stop_process`) for servers, REPLs, watchers. | | [`src/repo.rs`](src/repo.rs), [`src/ci.rs`](src/ci.rs) | Vendor-neutral `repo_*` tools behind the `RepoBackend` port (`GitCli` is the only adapter), and `ci_status` via `gh`. | -| [`src/bash.rs`](src/bash.rs), [`src/sandbox.rs`](src/sandbox.rs) | The opt-in shell and its opt-in OS confinement (`sandbox-exec` / `bwrap`). | +| [`src/bash.rs`](src/bash.rs), [`src/sandbox.rs`](src/sandbox.rs) | The default-on shell (withheld with `"tools": {"bash": "off"}`) and its opt-in OS confinement (`sandbox-exec` / `bwrap`, `STELLA_BASH_SANDBOX`, which wraps `bash` only). | | [`src/web.rs`](src/web.rs), [`src/web_extract.rs`](src/web_extract.rs) | The opt-in web family, and the pure HTML/CSS extraction behind it (no I/O in `web_extract.rs`, so it unit-tests without a network). | | [`src/issues.rs`](src/issues.rs), [`src/issue_ops.rs`](src/issue_ops.rs), [`src/github_rest.rs`](src/github_rest.rs), [`src/tracker_auth.rs`](src/tracker_auth.rs) | Issue-tracker tools, the backend-dispatching operations shared with the Command Deck, a minimal GitHub REST client, and the `stella connect` OAuth store. | | [`src/media.rs`](src/media.rs), [`src/media/`](src/media) | `generate_image`/`generate_video`/`poll_video` over `stella-media`'s port, plus the always-on client-side `generate_svg` and the host-attested process-free authority marker. | @@ -118,11 +118,11 @@ got. of the prompt prefix and `HashMap` iteration order is per-process randomized. Prompt caching is a byte-level prefix match, so an unsorted list means every process writes a divergent cache entry. -- **`bash` is off by default in every scope** — user, org-managed, and project. It registers only - when settings say `tools.bash: "on"`. The `web` family has the same posture, because a fetched - page is untrusted input *and* an uncontrolled egress channel. Prefer the genuinely - shell-free executors (`run_lint`, `format_code`, `diagnostics`, `repo_*`, the process group), - which spawn enumerable argv and never interpret a shell string. +- **`bash` is registered by default in every scope** — user, org-managed, and project — and is + withheld with `tools.bash: "off"` (#710 moved every built-in to on-by-default with a switch; + the previous opt-in covered built-ins only, so most operators never found it). Prefer the + genuinely shell-free executors (`run_lint`, `format_code`, `diagnostics`, `repo_*`, the + process group), which spawn enumerable argv and never interpret a shell string. - **Turning `bash` off removes the shell *tool*, not the shell *capability*.** `build_project` and `run_tests` take a `command` override, `verify_done` a `test_cmd`, and `run_script` composes a line from the scripts index — all four are always-on and all four reach `bash -c` through diff --git a/stella-tools/src/registry.rs b/stella-tools/src/registry.rs index 132aba94..33972cb2 100644 --- a/stella-tools/src/registry.rs +++ b/stella-tools/src/registry.rs @@ -1076,11 +1076,16 @@ impl ToolRegistry { /// resolution failed — the tool returns the named error itself, /// ungated). /// - /// Every tool that reaches `bash -c` MUST ride the same fence as - /// `bash`: they sit in the default surface while `bash` is opt-in - /// (and `start_process`'s argv[0] may itself be a shell, - /// `["bash", "-c", …]`), so leaving any of them out hands ambient shell - /// execution to the very posture that turned `bash` off. + /// Every tool that reaches `bash -c` MUST ride the same fence as `bash`: + /// they stay in the surface when an operator sets `"bash": "off"` (and + /// `start_process`'s argv[0] may itself be a shell, `["bash", "-c", …]`), + /// so leaving any out hands ambient shell execution to the very posture + /// that turned `bash` off. **Known gap (#615):** `screenshot`, `ci_status` + /// and (via `checkout_branch`) `start_work_on_issue` compose `bash -c` + /// lines through [`crate::exec`] without reaching here, so a policy + /// denying command execution cannot see them; all three shell-quote, so it + /// is fence completeness, not an RCE. Closing it needs a + /// `resolve_command_for_gate` each and makes them newly deniable. /// /// That covers tools which *compose* a command line. `send_stdin` does /// not — it writes into an interpreter someone already started — but the diff --git a/stella-tools/src/web.rs b/stella-tools/src/web.rs index 5827297f..cd0fa979 100644 --- a/stella-tools/src/web.rs +++ b/stella-tools/src/web.rs @@ -1,12 +1,15 @@ -//! The opt-in web family: `web_search`, `web_fetch`, `web_extract_assets`, +//! The web family: `web_search`, `web_fetch`, `web_extract_assets`, //! `web_download`. //! -//! Network egress is never ambient — the whole family registers only when -//! settings opt in with `"tools": {"web": "on"}` (any scope), mirroring the -//! `bash` posture: a fetched page is untrusted input *and* an uncontrolled -//! egress channel, so the host turns it on deliberately. `web_search` -//! additionally needs a BYOK provider key (`BRAVE_API_KEY` or +//! **Registered by default, switchable off** with `"tools": {"web": "off"}` +//! (any scope), matching the `bash` posture since #710 — the three key-free +//! tools ship on, pinned by `registry`'s +//! `the_key_free_web_family_is_registered_with_no_options_at_all`. +//! `web_search` additionally needs a BYOK provider key (`BRAVE_API_KEY` or //! `TAVILY_API_KEY`) — no key, no dead schema, exactly like the media tools. +//! A fetched page is untrusted input *and* an uncontrolled egress channel, so +//! an operator who wants egress bounded has to switch the family off rather +//! than decline to switch it on. //! //! Logged-in fetches ride the user's own sessions via //! `~/.stella/web_auth.toml` (override with `STELLA_WEB_AUTH_FILE`): @@ -25,11 +28,19 @@ //! a confused deputy the SSRF note below does not cover, because the URL is //! chosen by the page rather than by the operator. //! -//! No SSRF guard: an opted-in session can fetch any http(s) URL the host can -//! reach — including `localhost` and cloud metadata endpoints. This is -//! deliberate (matching the `bash` opt-in, and required for the "fetch my -//! internal tool / dev server" use case); the gate is the settings opt-in, -//! not a network allowlist. +//! No SSRF guard: a session can fetch any http(s) URL the host can reach — +//! including `localhost` and cloud metadata endpoints. It is required for the +//! "fetch my internal tool / dev server" use case, and there is no network +//! allowlist. +//! +//! **The compensating control this used to name is gone.** The exposure was +//! justified here by the family being opt-in, so that reaching a metadata +//! endpoint took a deliberate host action; since #710 the family is +//! registered by default and the only gate is an operator who knows to set +//! `"web": "off"`. Whether that is acceptable, or whether the default surface +//! needs a metadata/loopback denylist, is an open ruling (#615) — this note +//! exists so the next reader does not re-derive the retired guarantee from a +//! stale comment. //! //! Fetch/extract are `read_only` (they observe the web, not the workspace); //! `web_download` writes through [`crate::resolve_within_root`] and is diff --git a/stella-tools/tests/doc_truth.rs b/stella-tools/tests/doc_truth.rs new file mode 100644 index 00000000..e4dc6fa3 --- /dev/null +++ b/stella-tools/tests/doc_truth.rs @@ -0,0 +1,84 @@ +//! Doc-truth guardrails: shipped prose must not contradict the tool surface +//! the crate actually registers. +//! +//! #710 moved `bash` and the key-free web family to registered-by-default, and +//! `registry`'s `bash_is_registered_with_no_options_at_all` / +//! `the_key_free_web_family_is_registered_with_no_options_at_all` pin that in +//! code. The prose was never swept, so for every release after #710 the README, +//! `AGENTS.md`, the threat model, `docs/why-stella.md` and the model's own +//! system prompt all still said the shell was opt-in and that the default +//! surface had no shell at all (#615). +//! +//! That is worth a gate rather than a one-time fix. A false claim in a threat +//! model is worse than no claim — a reader budgets their risk against it — and +//! a false claim in the system prompt is read by the model every turn, which is +//! how "there is no shell unless the workspace enables it" survived a release +//! in which the shell was always present. +//! +//! These assert the *enabling* spelling is absent. Any doc telling a reader to +//! switch the shell ON to get one has inverted the posture; `"off"` is the only +//! correct instruction. + +/// Every operator-facing statement of the shell's default posture. +const DOCS: &[(&str, &str)] = &[ + ("README.md", include_str!("../../README.md")), + ("AGENTS.md", include_str!("../../AGENTS.md")), + ("stella-tools/README.md", include_str!("../README.md")), + ( + "docs/why-stella.md", + include_str!("../../docs/why-stella.md"), + ), + ( + "docs/design/threat-model.md", + include_str!("../../docs/design/threat-model.md"), + ), + ( + "docs/design/semantic-resolution-bridge.md", + include_str!("../../docs/design/semantic-resolution-bridge.md"), + ), +]; + +/// The enabling spellings, in the JSON and dotted forms both, plus the two +/// prose phrasings that shipped. +const INVERTED: &[&str] = &[ + r#""bash": "on""#, + r#"bash": "on""#, + r#"tools.bash: "on""#, + "bash` is opt-in", + "Bash is opt-in", + "opt-in shell", +]; + +#[test] +fn shipped_docs_do_not_claim_the_shell_is_opt_in() { + for (label, text) in DOCS { + for claim in INVERTED { + assert!( + !text.contains(claim), + "{label} still tells the reader to enable the shell ({claim:?}), but `bash` \ + has been registered by default since #710 — see stella-tools' \ + `bash_is_registered_with_no_options_at_all`. The switch is `\"bash\": \"off\"`." + ); + } + } +} + +/// The same claim reached the model, not just the operator: the shared +/// `tool_steering!` block told every session the workspace had to enable the +/// shell. Asserted here rather than in `stella-cli` because this crate owns the +/// fact being described, and because the prompt's own test previously *pinned* +/// the false sentence. +#[test] +fn the_system_prompt_states_the_switch_in_its_real_polarity() { + let prompt = include_str!("../../stella-cli/src/agent/prompt.rs"); + assert!( + !prompt.contains("There is no shell unless the workspace enables it"), + "the system prompt still tells the model there is no shell by default; \ + `bash` is registered by default since #710" + ); + assert!( + prompt.contains(r#"tools": {"bash": "off"}"#), + "the system prompt should name the real switch (`\"bash\": \"off\"`) so a model \ + asked to bound its own surface gives the operator a working instruction" + ); +}