From 9bdb7e5d3ec2a2c472ee090dcd42a1030b53d63d Mon Sep 17 00:00:00 2001 From: macanderson Date: Sun, 26 Jul 2026 20:51:05 -0700 Subject: [PATCH 1/2] P1 subset: close the store-open races and stop the docs lying about the shell MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Lands the rows across the five P1 umbrella issues that need no maintainer ruling. The other ~79 rows are decision-gated by construction and are left alone; see the PR body for the audit. **#617 item 8 + 10 — concurrent first open of a workspace.** The version read that decides which migration to run sat outside the transaction that applies it, so two processes opening one fresh workspace could both act on the same observation. Closing that surfaced three further real races on the same path, each found by the test the issue asked for (four threads, one fresh path): - `apply_migration` re-applied a step another process had already stamped. Now IMMEDIATE and re-reads `user_version` under the write lock, so losing the race is the no-op it should always have been — which is also what makes a future non-replay-safe step safe to append. - `enterprise_export_migration`'s state row was inserted after an unguarded existence check, so the loser failed `Store::open` outright on `UNIQUE constraint failed`. Now `ON CONFLICT DO NOTHING`, matching what its sibling branch already did. - `PRAGMA journal_mode=WAL` ran before `busy_timeout` was set, and SQLite skips the busy handler on a WAL upgrade, so the one statement needing an exclusive lock had no retry budget. Now behind a bounded retry, mirroring `stella-media`'s `initialize_journal_database`, which already documented this exact hazard. - The generated `.stella/.gitignore` was written with check-then-rename, so concurrent openers all renamed over the target and a racer validating the winner's file saw `nlink() == 0` and failed closed. Now an `O_EXCL | O_NOFOLLOW` exclusive create; losers read the settled file. Verified end-to-end, not just in tests: five concurrent `stella init` runs on one fresh workspace report "database is locked" three times on `origin/main` and zero times here. Each fix was negative-controlled by reverting it and watching the specific failure return. The same version-read-outside-the-transaction shape was an unfixed audit note in `stella-context` and `stella-fleet`; both now use IMMEDIATE too, and the notes are replaced with what the code does. **#615 — the shipped docs inverted a security claim.** #710 moved `bash` and the key-free web family to registered-by-default and never swept the prose, so every release since told operators the opposite of the surface they had: `README.md`, `AGENTS.md`, `stella-tools/README.md`, `docs/why-stella.md`, `docs/design/threat-model.md` and the semantic-resolution ADR all still said the shell was opt-in. The model was told too — `tool_steering!` carried "There is no shell unless the workspace enables it", and the prompt's own test pinned that false sentence. `web.rs` additionally justified having no SSRF guard (localhost, cloud metadata) on the grounds that the family was opt-in, a compensating control that no longer exists; that is now recorded as an open ruling rather than a retired guarantee presented as current. `stella-tools/tests/doc_truth.rs` gates all of it so the claim cannot invert again silently. Item 1 (the `run_tests` shell injection) was already fixed in #624 — verified, including its negative control. The `command.started` fence gap on `screenshot` / `ci_status` / `start_work_on_issue` is recorded at `command_line_for` rather than fixed: closing it makes those tools newly deniable by an existing policy, which is an operator-visible change. **#618 item 5, 17 · #616 item 8 — three small ones.** - Journal failures were reported as `MediaError::Artifact`, sending operators to `.stella/artifacts/` for a fault in the journal sidecar. New `Journal` variant. - The speculation `select!` had an `unreachable!` on a provider-driven path; it now reports a typed terminal error (invariant 5 outranks asserting a structural claim). - `upsert` handed the embedder every missing body in one request, so a full-workspace first index put the whole corpus in a single call. Batched at `warm::BATCH`, the width the warm indexer has always used. The file-size baseline moves three lines: the growth in `driver.rs`, `stella-store/src/lib.rs` and `registry.rs` is in-place code plus its rationale and cannot be extracted. The 50-line doc-truth test was moved to its own file rather than raising a baseline for it. --- AGENTS.md | 2 +- README.md | 18 ++- docs/design/semantic-resolution-bridge.md | 8 +- docs/design/threat-model.md | 21 ++- docs/why-stella.md | 2 +- scripts/file-size-baseline.txt | 6 +- stella-cli/src/agent/prompt.rs | 10 +- stella-context/src/store/schema.rs | 18 +-- stella-context/src/store/tests.rs | 51 +++++++ stella-context/src/warm.rs | 6 +- stella-context/src/writeback.rs | 18 ++- stella-core/src/driver.rs | 17 ++- stella-fleet/src/ledger.rs | 30 ++-- stella-media/src/error.rs | 11 ++ stella-media/src/operation_journal.rs | 2 +- .../src/enterprise_telemetry/migrations.rs | 13 +- stella-store/src/lib.rs | 38 +++-- stella-store/src/migrations.rs | 137 +++++++++++++++++- stella-store/src/private.rs | 68 ++++++++- stella-store/src/private_state_tests.rs | 54 +++++++ stella-tools/README.md | 12 +- stella-tools/src/registry.rs | 15 +- stella-tools/src/web.rs | 33 +++-- stella-tools/tests/doc_truth.rs | 84 +++++++++++ 24 files changed, 581 insertions(+), 93 deletions(-) create mode 100644 stella-tools/tests/doc_truth.rs 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-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..523defce 100644 --- a/stella-fleet/src/ledger.rs +++ b/stella-fleet/src/ledger.rs @@ -374,23 +374,27 @@ 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()?; if version < 1 { tx.execute_batch(MIGRATION_V1)?; } 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-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" + ); +} From 669976f5b254b39bab46c7801148b3dff1fb16c0 Mon Sep 17 00:00:00 2001 From: macanderson Date: Mon, 27 Jul 2026 00:38:17 -0700 Subject: [PATCH 2/2] #617 items 12 + 5: reclaim orphan state without ever deleting history MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both rows were left out of the previous commit because both destroy data. The owner ruled: fix the root cause before sweeping, and constrain new fleet ledgers only. **Item 12 — the orphan session-sidecar sweep.** The blocker was that `SessionRegistry::list` cannot tell "no record" from "unreadable record": it drops anything that fails to parse (`.ok()?`), and `upsert`'s own comment records that a power cut leaves a zero-length `.json`. A sweep keyed on what `list` accounts for therefore deletes the sidecar of a session whose record was merely truncated — and `history.json` in there is the only continuable copy of the conversation. So the conflation is fixed first. `scan` reports three states instead of one: healthy records, `damaged` records (present but unusable, sidecar intact), and `orphan_sidecars` (no `.json` beside them at any readability). The orphan test is answered from the directory entry, never from a parse. `list` now delegates to `scan().healthy`, so there is one implementation. `prune_orphan_sidecars` then deletes only genuine orphans, re-checking for a record under the same name immediately before removing — a session that started since the scan has written one. Damaged records are reported rather than repaired, and the type says why: `id`, `pid` and `started_at_ms` are recoverable from the filename (ids are self-minted `ses--`), but `workspace` is held nowhere else on disk — not in `journal.jsonl`, not in `history.json` — and resuming needs it. Silently hiding the session (today) and silently deleting it (as filed) are both wrong; surfacing it is what is actually available. The power-cut test asserts the whole chain: `list` cannot see the record, `scan` calls it damaged with its sidecar noted, the sweep is a no-op, and `history.json` survives. Negative-controlled against the filed definition (orphan = "not in `list`"), which classifies that session as an orphan and would delete it. **Item 5 — the fleet run references.** Retrofitting `REFERENCES runs (id)` onto a deployed `fleet.db` cannot succeed while an orphan row exists: the migration runner aborts on `pragma_foreign_key_check`, so the file stops opening, and the only way through is deleting 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 so. Fresh files now get the constraints via a new `FRESH_SCHEMA` (the current post-v2 shape plus the four `run_id` references), so a ledger created by this build cannot acquire an orphan. Existing files take the unchanged ladder and are not touched. The tradeoff is stated at `FRESH_SCHEMA`: `SCHEMA_VERSION` no longer determines shape alone, so a future step rebuilding one of these tables must reproduce the right variant or read the DDL from `sqlite_master`. `Ledger::orphan_rows` reports what an unconstrained file holds, and `enforces_run_references` answers which variant a file is — read from `sqlite_master`, because both are v2. That report is surfaced by a second `stella doctor` check rather than landing as uncalled API. It passes either way: inert rows on an old ledger are not a fault, and failing the command over them would make `doctor` unusable on exactly the workspaces with history worth keeping. `--repair` deliberately does not act on it, asserted by a test that checks the rows are still there afterwards. Verified against the real binary: a legacy ledger holding 4 orphan rows across 3 tables reports each class with its count and an explanation, exits 0, and `stella doctor --repair` leaves every row and the $4.20 of recorded spend intact. `doctor`'s module doc said "today there is exactly one check"; it now describes both, and the count assertion in its tests checks names rather than a number. --- stella-cli/src/doctor.rs | 173 ++++++++++++++++++++-- stella-cli/tests/doctor_cli.rs | 6 +- stella-fleet/src/ledger.rs | 259 +++++++++++++++++++++++++++++++++ stella-store/src/sessions.rs | 253 ++++++++++++++++++++++++++++++-- 4 files changed, 664 insertions(+), 27 deletions(-) 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-fleet/src/ledger.rs b/stella-fleet/src/ledger.rs index 523defce..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). @@ -395,6 +449,14 @@ fn migrate(conn: &Connection) -> Result<(), LedgerError> { if version >= SCHEMA_VERSION { return Ok(()); } + // 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)?; } @@ -406,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. @@ -493,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-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");