diff --git a/crates/stella-cli/src/command_deck/session_clear.rs b/crates/stella-cli/src/command_deck/session_clear.rs index dd3f3fc8a..7178c6e23 100644 --- a/crates/stella-cli/src/command_deck/session_clear.rs +++ b/crates/stella-cli/src/command_deck/session_clear.rs @@ -64,7 +64,10 @@ //! it — but their closeout is *sealed out* of the new board by the spawn //! generation watermark ([`SubSessions::seal_task_board`]), and //! [`settle_worker_task`] quarantines the late report as a note on the -//! lead's lane instead of folding it silently back in. Without the seal +//! lead's lane instead of folding it silently back in. The seal holds +//! only because the driver is the persisted mirror's sole writer: a +//! worker's own closeout records no board at all (#1708 — see +//! `subsession::closeout`). Without the seal //! the drop would not even be safe: board ids restart at "1" after a //! clear, so a pre-clear worker for task "1" would complete whatever //! *new* task "1" the cleared session had since created. diff --git a/crates/stella-cli/src/subsession.rs b/crates/stella-cli/src/subsession.rs index e370a7579..acb11a8c2 100644 --- a/crates/stella-cli/src/subsession.rs +++ b/crates/stella-cli/src/subsession.rs @@ -22,12 +22,14 @@ //! skipped in favor of latency, and delegation is not recursive — a worker's //! own `task_assign` requests are reported on its lane instead of spawning. +mod closeout; + use std::collections::HashMap; use std::sync::Arc; use stella_core::Engine; use stella_core::tasks::SpawnRequest; -use stella_protocol::{AgentEvent, CompletionMessage, TaskItem}; +use stella_protocol::{AgentEvent, CompletionMessage}; use stella_tools::RegistryOptions; use stella_tui::{AgentMeta, AgentStatus, Inbound}; use tokio::sync::mpsc::{self, UnboundedSender}; @@ -797,28 +799,18 @@ async fn run_worker( WorkerEnd::Stopped, ), }; - if let Some((store, id)) = &execution { - let _ = agent::record_execution_end( - store, - *id, - ®istry, - files_before, - label, - cost, - persistence_complete, - ); - // Mirror the worker's final board (its own, session-scoped view) so - // `tasks` queries see sub-agent boards too. - let board = registry.task_board(); - let items: Vec = board - .lock() - .unwrap_or_else(|p| p.into_inner()) - .items() - .to_vec(); - if !items.is_empty() { - let _ = store.record_task_board(*id, Some(session_id), &items, now_ms()); - } - } + // Audit record only — deliberately NO task-board mirror. The worker's + // private board is scaffolding for this one run, and the session's + // `tasks` rows have exactly one writer: the driver, whose `/clear` seal + // this thread cannot consult (#1708 — see `closeout`'s module docs). + closeout::close_worker_execution( + execution.as_ref(), + ®istry, + files_before, + label, + cost, + persistence_complete, + ); (execution_id, cost, end) } diff --git a/crates/stella-cli/src/subsession/closeout.rs b/crates/stella-cli/src/subsession/closeout.rs new file mode 100644 index 000000000..10d69fafc --- /dev/null +++ b/crates/stella-cli/src/subsession/closeout.rs @@ -0,0 +1,194 @@ +//! A worker's store closeout — the execution's audit record, and deliberately +//! nothing keyed to the session's task board (#1708). +//! +//! Until #1708 the closeout also mirrored the worker's own board into the +//! `tasks` table under the LEAD's session id, "so `tasks` queries see +//! sub-agent boards too". At the table's `UNIQUE(session_id, task_id)` key +//! that goal is unachievable and the write was pure corruption, twice over: +//! +//! * **Ordinal collision.** A worker's private board numbers from "1" in its +//! own namespace, so its task "1" upserted over the lead's task "1" — a +//! different task, different subject, different owner — and no reader could +//! tell the surviving mixture apart. +//! * **It bypassed `/clear`'s seal** (#1692). The seal that quarantines a +//! pre-clear worker's board report lives on the driver +//! ([`SubSessions::seal_task_board`](super::SubSessions::seal_task_board), +//! consumed by `session_clear::settle_worker_task`), and this closeout runs +//! on the worker's own thread where no seal can reach — so a worker that +//! predated a `/clear` repopulated the persisted mirror the user destroyed. +//! +//! The fix is subtraction, not plumbing: a worker's private board is +//! scaffolding for its one delegated run — the same lifetime the pipeline +//! gives an authored witness — and is discarded with the run. The delegation +//! outcome the session cares about is the LEAD's board row, which the driver +//! already mirrors at both of its own write sites (the lead's turn end, and +//! worker settlement in `session_clear::settle_worker_task`). With this +//! module holding the only worker-side closeout, the session's `tasks` rows +//! have exactly one writer — the driver — which is what makes the `/clear` +//! seal airtight rather than advisory. +//! +//! The seam is a module of its own for two reasons: `subsession.rs` sits just +//! under the file-size gate's ceiling, and the invariant needs a witness — +//! the tests below drive this exact production path against a real store. + +use std::sync::Arc; + +use stella_store::Store; +use stella_tools::ToolRegistry; + +use crate::agent; + +/// Close out a worker's execution row: files touched, citations, agent and +/// MCP usage, and the outcome label ([`agent::record_execution_end`]) — +/// best-effort, exactly as the worker's closeout always was (the inbox +/// notification and the lane's own events are the user-facing signal). +/// +/// The signature is the contract (#1708): no session id comes in, so the +/// closeout **cannot** address the session-keyed `tasks` rows at all. See +/// the module docs for why that absence is deliberate. +pub(crate) fn close_worker_execution( + execution: Option<&(Arc, i64)>, + registry: &ToolRegistry, + files_before: usize, + outcome_label: &str, + cost_usd: f64, + persistence_complete: bool, +) { + let Some((store, id)) = execution else { + return; + }; + let _ = agent::record_execution_end( + store, + *id, + registry, + files_before, + outcome_label, + cost_usd, + persistence_complete, + ); +} + +#[cfg(test)] +mod tests { + use super::*; + use stella_protocol::{TaskItem, TaskStatus}; + + fn lead_row(id: &str, subject: &str) -> TaskItem { + TaskItem { + id: id.into(), + subject: subject.into(), + description: None, + status: TaskStatus::Pending, + owner: None, + } + } + + /// A store holding the LEAD session's board mirror, plus a worker whose + /// own private board is populated — the exact state `run_worker` closes + /// out from after a delegated run that used `task_create`. + fn delegation_fixture() -> (Arc, i64, ToolRegistry, tempfile::TempDir) { + let root = tempfile::tempdir().expect("root"); + let store = Arc::new(Store::in_memory().expect("store")); + let lead_exec = store + .begin_execution("deck", "the lead's goal", "anthropic", "claude") + .expect("lead execution"); + store + .record_task_board( + lead_exec, + Some("ses"), + &[ + lead_row("1", "ship the parser"), + lead_row("2", "write the release notes"), + ], + 1, + ) + .expect("lead mirror"); + let worker_exec = store + .begin_execution("deck-sub", "delegated work", "anthropic", "claude") + .expect("worker execution"); + let registry = ToolRegistry::with_issue_backend(root.path().to_path_buf(), None); + registry + .task_board() + .lock() + .unwrap() + .create("read the grammar", None); + (store, worker_exec, registry, root) + } + + /// **Witness for #1708, defect one.** A worker's private board numbers + /// from "1" in its own namespace, so persisting it under the lead's + /// session id upserted the worker's task "1" over the lead's unrelated + /// task "1" (`UNIQUE(session_id, task_id)`). The closeout must leave the + /// session's mirror byte-identical: same rows, same subjects, and no + /// appended strays (the row COUNT is asserted too, because a write that + /// switched to a NULL session id would pass the per-session read while + /// still growing the table). + #[test] + fn a_workers_private_board_never_lands_in_the_sessions_tasks_rows() { + let (store, worker_exec, registry, _root) = delegation_fixture(); + + close_worker_execution( + Some(&(store.clone(), worker_exec)), + ®istry, + 0, + "completed", + 0.0, + true, + ); + + let rows = store.list_session_tasks("ses").expect("read back"); + assert_eq!( + rows.iter() + .map(|t| (t.id.as_str(), t.subject.as_str())) + .collect::>(), + vec![("1", "ship the parser"), ("2", "write the release notes")], + "the lead's mirror survives a worker closeout untouched" + ); + assert_eq!( + store.count("tasks").expect("count"), + 2, + "no worker row lands anywhere in the table, session-keyed or not" + ); + assert!( + !store + .unfinished_executions() + .expect("unfinished") + .contains(&worker_exec), + "the closeout still closes: the worker's execution row is finished" + ); + } + + /// **Witness for #1708, defect two.** `/clear` deletes the session's + /// `tasks` rows (#1692), and its worker seal lives on the driver — a + /// thread this closeout runs nowhere near. The only way a pre-clear + /// worker cannot repopulate the destroyed mirror is for its closeout to + /// write no board at all: after the clear's delete, a worker closeout + /// must leave the session's mirror exactly as empty as the user made it. + #[test] + fn a_pre_clear_workers_closeout_cannot_repopulate_a_cleared_mirror() { + let (store, worker_exec, registry, _root) = delegation_fixture(); + store.clear_session_tasks("ses").expect("the /clear delete"); + + close_worker_execution( + Some(&(store.clone(), worker_exec)), + ®istry, + 0, + "completed", + 0.0, + true, + ); + + assert!( + store + .list_session_tasks("ses") + .expect("read back") + .is_empty(), + "a board the user destroyed stays destroyed" + ); + assert_eq!( + store.count("tasks").expect("count"), + 0, + "and no session-less stray survives either" + ); + } +}