From 445a7fb49c779e75ceca4c2e310c20b9bfa60cab Mon Sep 17 00:00:00 2001 From: Wanjun Gu Date: Sun, 13 Sep 2026 16:15:23 -0700 Subject: [PATCH 1/4] fix(privacy): a busy store says so, and declassifying keeps History's order Two items from the 1.90.4 hold, both in the one writer that lowers a chat's classification (`privacy::declassify`). Privacy-critical code: needs a human reviewer. The write-first lock ordering from #294 and who may declassify are unchanged. Item 8 - a lock timeout answered a bodyless 500. `declassify` now names a waited-out store (SQLITE_BUSY 5/261/773 or a pool timeout; NOT the 517 snapshot conflict #294 removed) with a `StoreBusy` context whose Display is `DECLASSIFY_STORE_BUSY`. The route answers it 503 + Retry-After with that sentence, and every other failure 500 with `DECLASSIFY_FAILED`. An Err still changes nothing (the transaction rolls back on drop); both sentences claim only that. No retry inside the call: SQLite's busy handler already waited 5 s, and an immediate second attempt under the load that causes this failed again 3 times in 7. The desktop dialog reads the status off the Response instead of throwing the body (which toasted "[object Object]"), shows the daemon's sentence, and escalates to the typed phrase only on the 400 that means the grade moved - a busy store used to swap the single click for the phrase under a false "record has changed". Item 11 - declassifying reordered History. The UPDATE no longer stamps `updated_at`; a classification change is not use. Nothing learned of a declassification through that stamp (measured: a second window kept the private badge for 30 s), so the change now announces itself: `utils/sessionRowSync.ts` re-reads the row once per window and the session list cache, the sidebar's rows and the session page patch it in place. --- Cargo.lock | 1 + crates/biorouter-server/Cargo.toml | 5 + crates/biorouter-server/src/routes/session.rs | 64 ++- .../tests/declassify_store_busy.rs | 270 +++++++++++++ crates/biorouter/src/privacy/declassify.rs | 379 +++++++++++++++++- docs/security/privacy-tiers.md | 11 + ui/desktop/openapi.json | 5 +- ui/desktop/src/api/types.gen.ts | 6 +- .../useSidebarSessions.test.ts | 61 +++ .../BioRouterSidebar/useSidebarSessions.ts | 22 + .../sessions/DeclassifySessionDialog.test.tsx | 136 ++++++- .../sessions/DeclassifySessionDialog.tsx | 101 ++++- ...SessionHistoryView.browserSurface.test.tsx | 8 +- .../sessions/SessionHistoryView.test.tsx | 50 ++- .../sessions/SessionHistoryView.tsx | 12 + .../SessionListView.browserSurface.test.tsx | 8 +- .../SessionListView.declassify.test.tsx | 8 +- ui/desktop/src/utils/sessionListCache.test.ts | 33 ++ ui/desktop/src/utils/sessionListCache.ts | 31 ++ ui/desktop/src/utils/sessionRowSync.test.ts | 156 +++++++ ui/desktop/src/utils/sessionRowSync.ts | 141 +++++++ 21 files changed, 1472 insertions(+), 36 deletions(-) create mode 100644 crates/biorouter-server/tests/declassify_store_busy.rs create mode 100644 ui/desktop/src/utils/sessionRowSync.test.ts create mode 100644 ui/desktop/src/utils/sessionRowSync.ts diff --git a/Cargo.lock b/Cargo.lock index 5057d3e1c..3e6d6afa6 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1329,6 +1329,7 @@ dependencies = [ "serial_test", "sha2 0.10.9", "socket2 0.6.1", + "sqlx", "tempfile", "thiserror 1.0.69", "tikv-jemalloc-ctl", diff --git a/crates/biorouter-server/Cargo.toml b/crates/biorouter-server/Cargo.toml index 254853142..c5140f084 100644 --- a/crates/biorouter-server/Cargo.toml +++ b/crates/biorouter-server/Cargo.toml @@ -95,6 +95,11 @@ env-lock = { workspace = true } wiremock = { workspace = true } zip = "8.6.0" serial_test = { workspace = true } +# `tests/declassify_store_busy.rs` holds `sessions.db`'s write lock from a +# connection outside the daemon's pool, which no public API of `biorouter` can +# do. The same version and features `biorouter` already compiles, so this adds +# no crate to the build. +sqlx = { version = "0.8", features = ["runtime-tokio-rustls", "sqlite"] } # Issue #56 DR-20 / Task 55. This crate's routes raise the OS authentication # prompt — `POST /sessions/{id}/declassify` and `/config/upsert`'s master-switch # arm — so its TESTS would type a real password on every run without a stand-in. diff --git a/crates/biorouter-server/src/routes/session.rs b/crates/biorouter-server/src/routes/session.rs index 1391a31e7..e72d92003 100644 --- a/crates/biorouter-server/src/routes/session.rs +++ b/crates/biorouter-server/src/routes/session.rs @@ -15,7 +15,8 @@ use axum::{ use biorouter::agents::ExtensionConfig; use biorouter::conversation::message::Message; use biorouter::privacy::declassify::{ - authenticate_declassification, declassify, DeclassifyOutcome, UserConfirmation, + authenticate_declassification, declassify, is_store_busy, DeclassifyOutcome, UserConfirmation, + DECLASSIFY_STORE_BUSY, }; use biorouter::privacy::SessionClassification; use biorouter::session::extension_data::ExtensionState; @@ -1558,6 +1559,19 @@ const DECLASSIFY_SYSTEM_AUTH_REFUSED: &str = so marking it public needs your operating system to confirm it is you. That did not happen, \ and nothing was changed."; +/// What `POST /sessions/{id}/declassify` says when it failed for a reason that +/// is not a busy store — which it answers with +/// `biorouter::privacy::declassify::DECLASSIFY_STORE_BUSY` and a 503 instead. +/// +/// It used to say nothing at all: every `Err` was a bodyless 500. The sentence +/// claims only what an `Err` from the writer guarantees — this request changed +/// nothing — and does not invite a retry, because a genuine fault is not cleared +/// by waiting. The cause goes to the daemon log, not the body: a database error +/// can name paths, and a person cannot act on it anyway. +pub const DECLASSIFY_FAILED: &str = + "Nothing was changed and this chat was not marked public, because Biorouter hit an error. The \ + daemon log records it."; + #[derive(Debug, Default, Deserialize, ToSchema)] #[serde(rename_all = "camelCase")] pub struct DeclassifySessionRequest { @@ -1611,7 +1625,12 @@ pub struct DeclassifySessionResponse { request carried no proof it came from them (body = plain \ text)"), (status = 404, description = "Session not found"), - (status = 500, description = "Internal server error") + (status = 500, description = "Internal server error. Nothing was changed (body = plain \ + text)"), + (status = 503, description = "The session store stayed busy with other writes for longer \ + than the daemon waits. Nothing was changed, and the same \ + call a moment later can succeed; `Retry-After` is set \ + (body = plain text)") ), security( ("api_key" = []) @@ -1706,9 +1725,33 @@ async fn declassify_session( privacy_tier: SessionClassification::Public, })) } + // Item 8 of the 1.90.4 hold (2026-09-13). Both arms used to be one: a + // bodyless 500, which the desktop toasted as `[object Object]` and — on + // the single-click path — read as a stale grade and escalated to the + // typed phrase with a sentence claiming the chat's record had changed. + // Measured under a saturating external writer: 2 of 30 at 5.40 s and + // 5.46 s, daemon log `(code: 5) database is locked`. + // + // Either way nothing was written: `declassify` changes nothing on an + // `Err` (its transaction rolls back on drop), and a probe that answered + // before it never writes. So neither body claims more than that. + Err(e) if is_store_busy(&e) => { + // WARN, not ERROR: nothing is broken, other work held the lock. + tracing::warn!( + "Declassifying session {} gave up waiting for the session store: {:#}", + session_id, + e + ); + Err(( + StatusCode::SERVICE_UNAVAILABLE, + [(axum::http::header::RETRY_AFTER, "1")], + DECLASSIFY_STORE_BUSY, + ) + .into_response()) + } Err(e) => { - tracing::error!("Failed to declassify session {}: {}", session_id, e); - Err(StatusCode::INTERNAL_SERVER_ERROR.into_response()) + tracing::error!("Failed to declassify session {}: {:#}", session_id, e); + Err((StatusCode::INTERNAL_SERVER_ERROR, DECLASSIFY_FAILED).into_response()) } } } @@ -3895,6 +3938,11 @@ mod declassify_tests { DECLASSIFY_NO_USER_KEY, DECLASSIFY_CONFIRMATION_MISMATCH, DECLASSIFY_SYSTEM_AUTH_REFUSED, + // Not refusals, but the same route's plain-text bodies, so they are + // held to the same two rules: distinct, and never wearing a marker + // that sends the renderer's toast somewhere that cannot help. + DECLASSIFY_STORE_BUSY, + DECLASSIFY_FAILED, ]; for (i, one) in all.iter().enumerate() { for other in &all[i + 1..] { @@ -3915,6 +3963,14 @@ mod declassify_tests { // have no model audience and do not need to. assert!(DECLASSIFY_NEEDS_USER.contains("Do not retry")); assert!(DECLASSIFY_NO_USER_KEY.contains("Do not retry")); + // A busy store is the one failure a retry DOES clear, and a genuine + // fault the one it does not — so exactly one of the two says so. + assert!(DECLASSIFY_STORE_BUSY.contains("Try again")); + assert!(!DECLASSIFY_FAILED.to_lowercase().contains("try again")); + // Neither may say the chat is public, and both must say nothing changed. + for body in [DECLASSIFY_STORE_BUSY, DECLASSIFY_FAILED] { + assert!(body.contains("not marked public"), "{body}"); + } } /// SD-8. A daemon that holds no key may not answer a person with advice only diff --git a/crates/biorouter-server/tests/declassify_store_busy.rs b/crates/biorouter-server/tests/declassify_store_busy.rs new file mode 100644 index 000000000..30355010d --- /dev/null +++ b/crates/biorouter-server/tests/declassify_store_busy.rs @@ -0,0 +1,270 @@ +//! Item 8 of the 1.90.4 release hold, at `POST /sessions/{id}/declassify`: a +//! declassification that loses the chat store's write lock must SAY so, and must +//! say something different from a genuine failure. +//! +//! **Measured before this was written** (2026-09-13). Under a saturating +//! external writer on the same `sessions.db`, 2 of 30 declassifications came +//! back as a bodyless 500 at 5.40 s and 5.46 s, daemon log `(code: 5) database +//! is locked` — SQLite's busy timeout running out. Both rolled back, correctly, +//! and both succeeded on retry. Reproduced in the desktop app by holding the +//! write lock across one click: the toast read `Could not mark this chat public +//! [object Object]`, and the single-click dialog escalated to the typed phrase +//! with *"This chat's record has changed since this list was loaded"* — a claim +//! about the chat that nothing established. +//! +//! What is pinned here, over the real route and the real `check_token` layer: +//! +//! * a store held past the busy timeout answers **503** with `Retry-After` and +//! `DECLASSIFY_STORE_BUSY`, writes no ledger row, leaves the chat private, and +//! the same call succeeds once the store is free; +//! * any other failure answers **500** with `DECLASSIFY_FAILED`, not the busy +//! sentence, and changes nothing either. +//! +//! ⚠ **Its own binary on purpose.** The busy test holds `sessions.db`'s write +//! lock for more than five seconds. In the lib's test binary, where the route's +//! other tests live, every test that touches the shared store in parallel would +//! wait out the same timeout and fail — a flake manufactured by the test. Here +//! the store is this binary's alone (`test_sandbox`), and `#[serial]` keeps the +//! two tests off each other. + +// Redirects this binary's Biorouter data/config/state dirs at a throwaway root +// before `main`, so the lock below can never be taken on the developer's real +// `sessions.db`. +#[path = "../src/test_sandbox.rs"] +mod test_sandbox; + +use std::sync::Arc; +use std::time::{Duration, Instant}; + +use axum::body::Body; +use axum::http::{HeaderMap, Request, StatusCode}; +use biorouter::conversation::message::Message; +use biorouter::model::ModelConfig; +use biorouter::privacy::declassify::DECLASSIFY_STORE_BUSY; +use biorouter::privacy::SessionClassification; +use biorouter::session::session_manager::{SessionManager, SessionType, DB_NAME, SESSIONS_FOLDER}; +use biorouter_server::routes::session::DECLASSIFY_FAILED; +use biorouter_server::state::AppState; +use serial_test::serial; +use sqlx::{ConnectOptions, Connection}; +use tower::ServiceExt; + +const TEST_SECRET: &str = "declassify-store-busy-secret"; +const TEST_USER_ACTION_KEY: &str = "declassify-store-busy-user-action-key"; + +/// The desktop's daemon holds a user-action key, and the refusal for a caller +/// without one comes BEFORE the store is touched — so without this every +/// request here would be a 403 and measure nothing about the store. +fn install_user_action_key() { + let digest: [u8; 32] = + ::digest(TEST_USER_ACTION_KEY.as_bytes()).into(); + biorouter_server::auth::install_user_action_digest(Some(digest)); + let mut headers = HeaderMap::new(); + headers.insert("X-User-Action", TEST_USER_ACTION_KEY.parse().unwrap()); + assert!( + biorouter_server::auth::is_user_action(&headers), + "the user-action digest did not take, so every request below would stop at the proof \ + check and never reach the store" + ); +} + +/// A private chat that merely ran a turn on a private model: §12.4's single +/// click, so neither a phrase nor an operating-system prompt stands between the +/// request and the store. +async fn seed_turn_private(state: &Arc) -> String { + let manager = state.session_manager(); + let session = manager + .create_session( + std::env::temp_dir().join("declassify_store_busy"), + "Store busy fixture".to_string(), + SessionType::User, + ) + .await + .unwrap(); + manager + .add_message(&session.id, &Message::user().with_text("patient MRN 12345")) + .await + .unwrap(); + manager + .update(&session.id) + .provider_name("versa_azure") + .model_config(ModelConfig::new("gpt-4o").unwrap()) + .raise_privacy(SessionClassification::Private, "turn:versa_azure") + .apply() + .await + .unwrap(); + session.id +} + +/// The request the desktop sends — secret, user-action proof, no confirmation — +/// through the same `check_token` layer `commands::agent::run` installs. +async fn post_declassify( + state: Arc, + session_id: &str, +) -> (StatusCode, HeaderMap, String) { + let app = biorouter_server::routes::session::routes(state).layer( + axum::middleware::from_fn_with_state( + TEST_SECRET.to_string(), + biorouter_server::auth::check_token, + ), + ); + let request = Request::builder() + .method("POST") + .uri(format!("/sessions/{session_id}/declassify")) + .header("content-type", "application/json") + .header("X-Secret-Key", TEST_SECRET) + .header("X-User-Action", TEST_USER_ACTION_KEY) + .body(Body::from(r#"{"confirmation":null}"#)) + .unwrap(); + let response = app.oneshot(request).await.unwrap(); + let status = response.status(); + let headers = response.headers().clone(); + let bytes = axum::body::to_bytes(response.into_body(), usize::MAX) + .await + .unwrap(); + ( + status, + headers, + String::from_utf8_lossy(&bytes).into_owned(), + ) +} + +/// A connection of our own to THIS binary's `sessions.db`, outside the daemon's +/// pool — the shape of the external writer the measurement used. +async fn external_connection() -> sqlx::SqliteConnection { + let path = SessionManager::shared_store_root() + .join(SESSIONS_FOLDER) + .join(DB_NAME); + assert!( + path.is_file(), + "{} does not exist, so a lock taken on it would not be the daemon's store", + path.display() + ); + sqlx::sqlite::SqliteConnectOptions::new() + .filename(&path) + .connect() + .await + .unwrap() +} + +/// What the store actually holds for this chat, read around the daemon. +async fn stored_state(session_id: &str) -> (String, i64) { + let mut conn = external_connection().await; + let tier: String = sqlx::query_scalar("SELECT privacy_tier FROM sessions WHERE id = ?1") + .bind(session_id) + .fetch_one(&mut conn) + .await + .unwrap(); + let ledger: i64 = + sqlx::query_scalar("SELECT COUNT(*) FROM classification_audit WHERE session_id = ?1") + .bind(session_id) + .fetch_one(&mut conn) + .await + .unwrap(); + conn.close().await.unwrap(); + (tier, ledger) +} + +/// ⚠ **Fails on `origin/main`**, where this request answers a bodyless 500. +#[tokio::test(flavor = "multi_thread")] +#[serial] +async fn a_store_held_past_the_busy_timeout_answers_503_in_words_and_changes_nothing() { + install_user_action_key(); + let state = AppState::new().await.unwrap(); + let id = seed_turn_private(&state).await; + + // Hold the write lock for longer than the pool's five-second busy timeout. + let mut holder = external_connection().await; + sqlx::query("BEGIN IMMEDIATE") + .execute(&mut holder) + .await + .unwrap(); + + let started = Instant::now(); + let (status, headers, body) = post_declassify(state.clone(), &id).await; + let waited = started.elapsed(); + + sqlx::query("ROLLBACK").execute(&mut holder).await.unwrap(); + holder.close().await.unwrap(); + + assert_eq!( + status, + StatusCode::SERVICE_UNAVAILABLE, + "a declassification that only waited out the store's lock must not read as a daemon \ + fault (body: {body:?})" + ); + assert_eq!( + body, DECLASSIFY_STORE_BUSY, + "the 503 does not carry the sentence a person can act on" + ); + assert_eq!( + headers + .get(axum::http::header::RETRY_AFTER) + .and_then(|v| v.to_str().ok()), + Some("1") + ); + assert!( + waited >= Duration::from_secs(4), + "answered after {waited:?}: it did not wait for the lock, so this did not measure a \ + lock timeout" + ); + + // Nothing landed. The one outcome this must never have is a private chat + // lowered, or a ledger row claiming it was, by a request that failed. + assert_eq!( + stored_state(&id).await, + ("private".to_string(), 0), + "a declassification that answered 503 changed the store" + ); + + // And it is transient: the same request, with the store free, succeeds. + let (status, _, body) = post_declassify(state.clone(), &id).await; + assert_eq!( + status, + StatusCode::OK, + "retry after the lock cleared: {body}" + ); + assert_eq!(stored_state(&id).await, ("public".to_string(), 1)); +} + +/// The busy sentence is for a busy store only. A fault that waiting cannot clear +/// — a trigger aborting the ledger insert stands in for one — is a 500 in its own +/// words, and must never tell the person to try again. +/// +/// ⚠ **Fails on `origin/main`**, where it answers a bodyless 500. +#[tokio::test(flavor = "multi_thread")] +#[serial] +async fn a_genuine_fault_answers_500_in_its_own_words_and_changes_nothing() { + install_user_action_key(); + let state = AppState::new().await.unwrap(); + let id = seed_turn_private(&state).await; + + let trigger = format!( + "fail_ledger_insert_{}", + id.replace(|c: char| !c.is_alphanumeric(), "_") + ); + let mut conn = external_connection().await; + sqlx::query(&format!( + "CREATE TRIGGER {trigger} BEFORE INSERT ON classification_audit \ + WHEN NEW.session_id = '{id}' BEGIN SELECT RAISE(ABORT, 'injected fault'); END" + )) + .execute(&mut conn) + .await + .unwrap(); + + let (status, _, body) = post_declassify(state.clone(), &id).await; + + sqlx::query(&format!("DROP TRIGGER {trigger}")) + .execute(&mut conn) + .await + .unwrap(); + conn.close().await.unwrap(); + + assert_eq!(status, StatusCode::INTERNAL_SERVER_ERROR, "body: {body:?}"); + assert_eq!(body, DECLASSIFY_FAILED); + assert_ne!( + body, DECLASSIFY_STORE_BUSY, + "a genuine fault was told to try again" + ); + assert_eq!(stored_state(&id).await, ("private".to_string(), 0)); +} diff --git a/crates/biorouter/src/privacy/declassify.rs b/crates/biorouter/src/privacy/declassify.rs index e5f8282e6..c32ddb80c 100644 --- a/crates/biorouter/src/privacy/declassify.rs +++ b/crates/biorouter/src/privacy/declassify.rs @@ -364,6 +364,98 @@ pub enum DeclassifyOutcome { SessionNotFound, } +/// What a person is told when a declassification gave up waiting for the chat +/// store. Shared by both doors: the route answers it as a 503's body, and the +/// CLI prints it as the head of the error chain [`declassify`] returns. +/// +/// ⚠ **Every clause is a claim this module can stand behind, and no more.** +/// "Changed nothing" holds because the only statements that can fail this way +/// run inside the writing transaction (or are its commit), and a +/// `sqlx::Transaction` that is dropped without committing rolls back — so no +/// ledger row and no lowered tier survive it. It says the chat was not marked +/// public *by this request*, rather than "the chat is still private", on +/// purpose: another request may land while this one is being answered, and a +/// sentence about the row's present state would be a read this code never made. +/// +/// ⚠ **Short on purpose.** The desktop shows it in a toast whose message is +/// clamped to three lines, and a first draft that opened with the cause lost +/// "Try again in a moment" behind the ellipsis — measured in the running app. +/// So it leads with what happened to the chat, then the remedy. +/// +/// It invites the retry, unlike the proof-of-user refusals beside it in +/// `routes/session.rs`. Those are refusals a retry cannot change; this is a +/// wait that ran out, and the same call a moment later is the remedy. Both +/// doors sit behind a person — the user-action header, or a terminal — so no +/// model is being taught to loop on it. +/// +/// ⚠ **The retry is the person's, not this module's.** SQLite's busy handler is +/// already a bounded retry loop: every statement here polls the write lock for +/// the pool's full five-second `busy_timeout` before this error exists. A +/// second in-process attempt was measured (2026-09-13, two external writers on +/// the same `sessions.db`) and it mostly bought a longer spinner: 7 of 30 +/// declassifications gave up, an immediate retry rescued 4 of the 7 and the +/// other 3 failed again, at up to 9.5 s each — under the load that causes this +/// error the busy periods are correlated, not independent. +pub const DECLASSIFY_STORE_BUSY: &str = + "Nothing was changed and this chat was not marked public, because other writes kept the chat \ + store busy. Try again in a moment."; + +/// The error context [`declassify`] attaches when it failed only because the +/// store stayed busy. +/// +/// A type rather than a string, so a door asks [`is_store_busy`] — a downcast — +/// and a reworded sentence cannot quietly turn a 503 back into an empty 500. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct StoreBusy; + +impl std::fmt::Display for StoreBusy { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str(DECLASSIFY_STORE_BUSY) + } +} + +/// Did this declassification fail only because the chat store stayed busy? +/// +/// `true` exactly for an error [`declassify`] returned with [`StoreBusy`] +/// attached. It is the one distinction a door should draw from an `Err`: this +/// one says "try again", and every other failure is a genuine fault and says so. +pub fn is_store_busy(error: &anyhow::Error) -> bool { + error.downcast_ref::().is_some() +} + +/// Is this the store's busy wait running out, as opposed to any other fault? +/// +/// Two shapes, both meaning "other work held the write lock, or every pooled +/// connection, for longer than this process waits", and both cleared by the +/// same call a moment later: +/// +/// * SQLite answered `SQLITE_BUSY` once the pool's `busy_timeout` ran out — +/// extended code 5 — or one of its two other waited-out forms, +/// `SQLITE_BUSY_RECOVERY` (261) and `SQLITE_BUSY_TIMEOUT` (773). sqlx reports +/// `sqlite3_extended_errcode`, so they arrive as exactly those strings. The +/// daemon log of the report this was written for reads `(code: 5) database +/// is locked`. +/// * sqlx gave up acquiring a pooled connection (`PoolTimedOut`). +/// +/// ⚠ **`SQLITE_BUSY_SNAPSHOT` (517) is deliberately NOT one of them.** It is the +/// instant refusal of a transaction that READ before it wrote — no busy handler +/// is consulted for it — and the write-first statement at the top of +/// [`declassify_in_one_transaction`] exists so that it cannot happen here. If it +/// ever does, the lock ordering has regressed, and answering it as "busy, try +/// again" would dress that regression up as load. It stays a genuine failure: a +/// 500 and an `ERROR` in the log. +fn waited_out_the_store(error: &anyhow::Error) -> bool { + error + .chain() + .any(|cause| match cause.downcast_ref::() { + Some(sqlx::Error::PoolTimedOut) => true, + Some(sqlx::Error::Database(db)) => { + matches!(db.code().as_deref(), Some("5") | Some("261") | Some("773")) + } + _ => false, + }) +} + /// The ONLY writer in the tree permitted to lower `privacy_tier`. /// /// Every other write goes through the session update builder, whose emission is @@ -437,13 +529,48 @@ pub enum DeclassifyOutcome { /// honest and the direction is fail-safe — private is the protected state — but /// the user can watch their action undo itself. Preventing it would mean /// refusing to declassify a busy session, which §12.4 does not ask for. -/// ⚠ **`_ok` is borrowed rather than consumed**, and that is what keeps the two +/// ⚠ **`ok` is borrowed rather than consumed**, and that is what keeps the two /// doors at ONE construction site each. Both call this twice for a chat on the /// strong control — once to probe, once to write — and /// [`the_proof_of_user_is_constructed_in_exactly_two_places`] counts /// constructions per file, not calls. One human action, one proof, however many /// times the writer is asked. +/// +/// # Errors +/// +/// An `Err` means this call changed nothing: every write is inside one +/// transaction, and a transaction that does not reach its commit rolls back on +/// drop. One kind of `Err` is not a fault at all — the store stayed busy for +/// longer than the pool waits — and it carries [`StoreBusy`], so a door can tell +/// a person to try again rather than answer an empty 500 (which is what the +/// route did until 2026-09-13: two of thirty declassifications under a +/// saturating external writer came back bodyless at 5.4 s, and the desktop +/// showed `[object Object]`). Ask [`is_store_busy`]; everything else is genuine. pub async fn declassify( + sm: &SessionManager, + session_id: &str, + confirmation: Option<&str>, + authorization: Option<&SystemAuthorization>, + ok: &UserConfirmation, +) -> Result { + declassify_in_one_transaction(sm, session_id, confirmation, authorization, ok) + .await + .map_err(|error| { + if waited_out_the_store(&error) { + error.context(StoreBusy) + } else { + error + } + }) +} + +/// [`declassify`]'s body: one transaction, and everything its doc comment says +/// about lock ordering, grading and the ledger lives here. +/// +/// Split out only so the error it returns can be named in ONE place. The `?`s +/// below are many and each can fail on a busy store; wrapping them one by one +/// is how a new statement would come to be the one that answers an empty 500. +async fn declassify_in_one_transaction( sm: &SessionManager, session_id: &str, confirmation: Option<&str>, @@ -570,9 +697,24 @@ pub async fn declassify( .execute(&mut *tx) .await?; + // ⚠ **`updated_at` is not touched, and that is a decision, not an omission.** + // `updated_at` orders History, the sidebar's keyset pages and `biorouter + // session list`, and buckets History's date groups — it answers "when was + // this chat last USED". A classification change is not use. This statement + // stamped it until 2026-09-13, and the measured result was 796 + // declassifications moving months-old chats (20260224_11, created + // 2026-02-24) into History's "Today". + // + // Nothing needed the stamp to learn of the change. The session-row feed + // (`GET /sessions/changes`, `SessionStorage::session_meta_rows`) compares + // `privacy_tier`/`privacy_reason` and deliberately never `updated_at`; and a + // second window had no live signal at all before — it learned only when some + // unrelated refresh re-read a list that this stamp had re-sorted. The + // desktop now announces the change itself (`utils/sessionRowSync.ts`), and + // every list surface re-reads the row in place. sqlx::query( "UPDATE sessions \ - SET privacy_tier = 'public', privacy_reason = ?2, updated_at = datetime('now') \ + SET privacy_tier = 'public', privacy_reason = ?2 \ WHERE id = ?1", ) .bind(session_id) @@ -1733,4 +1875,237 @@ mod tests { let row = sm.get_session(&id, false).await.unwrap(); assert_eq!(row.privacy_tier, SessionClassification::Public); } + + /// Take `sessions.db`'s write lock the way a real transaction does — a + /// statement that writes nothing but takes the lock at its prologue, inside a + /// real `Transaction` — on a SECOND store over the same file, so nothing in + /// process memory orders it against the declassification. Dropping the + /// returned transaction releases it. + async fn hold_the_write_lock( + other: &SessionManager, + ) -> sqlx::Transaction<'static, sqlx::Sqlite> { + let pool = other.storage().pool().await.unwrap(); + let mut tx = pool.begin().await.unwrap(); + sqlx::query("UPDATE sessions SET id = id WHERE 1 = 0") + .execute(&mut *tx) + .await + .unwrap(); + tx + } + + /// Item 8 of the 1.90.4 hold (2026-09-13). Measured under a saturating + /// external writer: two of thirty declassifications failed at 5.40 s and + /// 5.46 s with `(code: 5) database is locked`, the route answered a + /// bodyless 500, and the desktop toasted `[object Object]`. + /// + /// Three things are pinned, and the second is the one that matters most: + /// + /// 1. The error is NAMED — [`is_store_busy`] says so, and its head is the + /// sentence a person can act on. On `origin/main` the error is sqlx's + /// alone. + /// 2. It changed nothing. The chat is still private and the ledger holds no + /// row claiming a transition. A lock timeout must never be reported, or + /// recorded, as a declassification. + /// 3. It is transient: once the lock is released, the same call succeeds. + /// + /// The lock is held for real, past the pool's five-second `busy_timeout`, so + /// this test takes about that long. `turn:*` provenance, so neither a phrase + /// nor a password is in the way and the store is the only variable. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn a_store_that_stays_busy_is_named_and_changes_nothing() { + let temp = tempfile::TempDir::new().unwrap(); + let sm = SessionManager::new(temp.path().to_path_buf()); + let id = private_session_with_reason(&sm, "turn:versa_azure").await; + let ok = UserConfirmation::for_test(); + + let other = SessionManager::new(temp.path().to_path_buf()); + let lock = hold_the_write_lock(&other).await; + let started = std::time::Instant::now(); + let error = declassify(&sm, &id, None, None, &ok) + .await + .expect_err("a store write-locked past the busy timeout cannot be declassified"); + let waited = started.elapsed(); + + assert!( + is_store_busy(&error), + "a declassification that only lost the write lock is not named as one: {error:#}" + ); + assert_eq!( + error.to_string(), + DECLASSIFY_STORE_BUSY, + "the error's head is not the sentence a person is shown" + ); + assert!( + format!("{error:#}").contains("database is locked"), + "the cause was swallowed rather than kept under the sentence: {error:#}" + ); + // It really waited: an instant failure here would be a snapshot conflict + // (a lock-ordering regression), which must not be classified as busy. + assert!( + waited >= std::time::Duration::from_secs(4), + "gave up after {waited:?}, well inside the five-second busy timeout" + ); + + drop(lock); + let row = sm.get_session(&id, false).await.unwrap(); + assert_eq!( + row.privacy_tier, + SessionClassification::Private, + "a declassification that timed out lowered the tier anyway" + ); + assert_eq!(row.privacy_reason.as_deref(), Some("turn:versa_azure")); + assert!( + audit_rows(&sm, &id).await.is_empty(), + "the ledger claims a transition that did not happen" + ); + + assert_eq!( + declassify(&sm, &id, None, None, &ok).await.unwrap(), + DeclassifyOutcome::Declassified, + "the same call did not succeed once the store was free" + ); + } + + /// The other half of item 8: only a busy store is called one. A fault that is + /// not contention — here a trigger that aborts the ledger insert — must stay a + /// genuine failure, or a broken daemon would keep telling people to "try + /// again in a moment" forever. It changes nothing either. + #[tokio::test] + async fn a_fault_that_is_not_a_busy_store_is_not_called_one() { + let temp = tempfile::TempDir::new().unwrap(); + let sm = SessionManager::new(temp.path().to_path_buf()); + let id = private_session_with_reason(&sm, "turn:versa_azure").await; + { + let pool = sm.storage().pool().await.unwrap(); + sqlx::query(&format!( + "CREATE TRIGGER fail_this_ledger_insert BEFORE INSERT ON classification_audit \ + WHEN NEW.session_id = '{id}' BEGIN SELECT RAISE(ABORT, 'injected fault'); END" + )) + .execute(pool) + .await + .unwrap(); + } + + let error = declassify(&sm, &id, None, None, &UserConfirmation::for_test()) + .await + .expect_err("the trigger aborts the ledger insert"); + assert!( + !is_store_busy(&error), + "a genuine fault was named a busy store: {error:#}" + ); + assert!(format!("{error:#}").contains("injected fault")); + + let row = sm.get_session(&id, false).await.unwrap(); + assert_eq!(row.privacy_tier, SessionClassification::Private); + assert!(audit_rows(&sm, &id).await.is_empty()); + } + + /// `SQLITE_BUSY_SNAPSHOT` (517) is not a busy store, and this is measured on + /// a real WAL file rather than asserted of a hand-built error: a transaction + /// that READ, then lost the write to a commit made after its snapshot, is + /// refused instantly with no busy wait. That is the shape PR #294's + /// write-first statement removed from `declassify`, so if it ever reaches a + /// door again it must read as the regression it is. + #[tokio::test] + async fn a_snapshot_conflict_is_not_classified_as_a_busy_store() { + let temp = tempfile::TempDir::new().unwrap(); + let reader_store = SessionManager::new(temp.path().to_path_buf()); + let s = reader_store + .create_session(std::env::temp_dir(), "snap".to_string(), SessionType::User) + .await + .unwrap(); + let writer_store = SessionManager::new(temp.path().to_path_buf()); + + let pool = reader_store.storage().pool().await.unwrap(); + let mut tx = pool.begin().await.unwrap(); + // Pin a read snapshot. + let _: i64 = sqlx::query_scalar("SELECT COUNT(*) FROM sessions") + .fetch_one(&mut *tx) + .await + .unwrap(); + // Another store commits after that snapshot. + writer_store + .add_message( + &s.id, + &crate::conversation::message::Message::user().with_text("later"), + ) + .await + .unwrap(); + // The upgrade is refused. + let started = std::time::Instant::now(); + let refused = sqlx::query("UPDATE sessions SET name = 'x' WHERE id = ?1") + .bind(&s.id) + .execute(&mut *tx) + .await + .expect_err("a stale snapshot cannot upgrade to a writer"); + assert!( + started.elapsed() < std::time::Duration::from_secs(2), + "the snapshot conflict waited, so this test is not measuring 517" + ); + let code = match &refused { + sqlx::Error::Database(db) => db.code().map(|c| c.into_owned()), + other => panic!("expected a database error, got {other:?}"), + }; + assert_eq!( + code.as_deref(), + Some("517"), + "not SQLITE_BUSY_SNAPSHOT: {refused}" + ); + + assert!( + !waited_out_the_store(&anyhow::Error::from(refused)), + "a snapshot conflict was classified as a busy store" + ); + } + + /// Item 11 of the 1.90.4 hold (2026-09-13): declassifying a chat must not + /// move it in History. + /// + /// The writer stamped `updated_at = datetime('now')`, so a chat created + /// months ago jumped into History's "Today" (measured across 796 + /// declassifications; 20260224_11 was one). `updated_at` orders every chat + /// list and buckets History's dates, and a classification change is not use + /// of the chat. Fails on `origin/main`. + #[tokio::test] + async fn declassifying_a_chat_does_not_move_it_in_history() { + let temp = tempfile::TempDir::new().unwrap(); + let sm = SessionManager::new(temp.path().to_path_buf()); + const LAST_USED: &str = "2026-02-24 11:01:03"; + let mut ids = vec![]; + for reason in ["turn:versa_azure", "backfill:ollama"] { + let id = private_session_with_reason(&sm, reason).await; + let pool = sm.storage().pool().await.unwrap(); + sqlx::query("UPDATE sessions SET updated_at = ?1 WHERE id = ?2") + .bind(LAST_USED) + .bind(&id) + .execute(pool) + .await + .unwrap(); + ids.push(id); + } + let mut before = vec![]; + for id in &ids { + before.push(sm.get_session(id, false).await.unwrap().updated_at); + } + + // Both graded paths — the single click, and the phrase plus password — + // go through the same statement. + for id in &ids { + assert_eq!( + declassify_for_test(&sm, id).await.unwrap(), + DeclassifyOutcome::Declassified + ); + } + + for (id, was) in ids.iter().zip(before) { + let row = sm.get_session(id, false).await.unwrap(); + assert_eq!(row.privacy_tier, SessionClassification::Public); + assert_eq!( + row.updated_at, was, + "declassifying {id} moved its last-used time from {was} to {}, which re-sorts \ + History and puts a months-old chat under Today", + row.updated_at + ); + } + } } diff --git a/docs/security/privacy-tiers.md b/docs/security/privacy-tiers.md index 102580c0f..c6b64bda6 100644 --- a/docs/security/privacy-tiers.md +++ b/docs/security/privacy-tiers.md @@ -128,6 +128,17 @@ this section is the ledger. `biorouter session declassify ` in the CLI, which is the only surface that reaches a private chat no listing shows. + Two properties added on 2026-09-13 (the 1.90.4 hold, items 8 and 11). **A declassification does + not move the chat's `updated_at`**: a classification change is not use of the chat, and stamping + it put months-old chats under History's "Today". The desktop announces the change on its own + channel instead (`ui/desktop/src/utils/sessionRowSync.ts`), so a second window's History row, + sidebar row and session page re-read the row in place. **A declassification that only waited out + the store's busy timeout answers `503` with `Retry-After` and a sentence** + (`privacy::declassify::DECLASSIFY_STORE_BUSY`), distinct from a genuine failure's `500` + (`routes::session::DECLASSIFY_FAILED`); both mean nothing landed, and neither retries inside + the call — SQLite's busy handler already waited five seconds, and a second + attempt under the load that causes this was measured to fail again three times in seven. + ⚠ **R18's "no cached grant" is honoured on Linux and NOT guaranteed on macOS (F-13).** R18 asks for a prompt "raised once per operation (no session, no cached grant)". The Linux prompter implements that literally — it declines `pkexec`'s `org.freedesktop.policykit.exec` *because* diff --git a/ui/desktop/openapi.json b/ui/desktop/openapi.json index 1893b3fa3..278beabe4 100644 --- a/ui/desktop/openapi.json +++ b/ui/desktop/openapi.json @@ -4316,7 +4316,10 @@ "description": "Session not found" }, "500": { - "description": "Internal server error" + "description": "Internal server error. Nothing was changed (body = plain text)" + }, + "503": { + "description": "The session store stayed busy with other writes for longer than the daemon waits. Nothing was changed, and the same call a moment later can succeed; `Retry-After` is set (body = plain text)" } }, "security": [ diff --git a/ui/desktop/src/api/types.gen.ts b/ui/desktop/src/api/types.gen.ts index 4ec4346a9..d3147ae28 100644 --- a/ui/desktop/src/api/types.gen.ts +++ b/ui/desktop/src/api/types.gen.ts @@ -7572,9 +7572,13 @@ export type DeclassifySessionErrors = { */ 404: unknown; /** - * Internal server error + * Internal server error. Nothing was changed (body = plain text) */ 500: unknown; + /** + * The session store stayed busy with other writes for longer than the daemon waits. Nothing was changed, and the same call a moment later can succeed; `Retry-After` is set (body = plain text) + */ + 503: unknown; }; export type DeclassifySessionResponses = { diff --git a/ui/desktop/src/components/BioRouterSidebar/useSidebarSessions.test.ts b/ui/desktop/src/components/BioRouterSidebar/useSidebarSessions.test.ts index f8698c699..59ad1f427 100644 --- a/ui/desktop/src/components/BioRouterSidebar/useSidebarSessions.test.ts +++ b/ui/desktop/src/components/BioRouterSidebar/useSidebarSessions.test.ts @@ -9,10 +9,12 @@ import { notifySessionListChanged } from '../../utils/sessionListCache'; const mocks = vi.hoisted(() => ({ listSidebarSessions: vi.fn(), + getSession: vi.fn(), })); vi.mock('../../api', () => ({ listSidebarSessions: mocks.listSidebarSessions, + getSession: mocks.getSession, })); // The proof the desktop sends. Since issue #56's QA sweep (2026-09-10) the @@ -197,3 +199,62 @@ describe('a deleted chat leaves Recents without a reload', () => { expect(result.current.sessions.map((session) => session.id)).toContain('session-19'); }); }); + +/** + * Item 11 of the 1.90.4 hold (2026-09-13). The daemon used to stamp + * `updated_at` on a declassification, so the next head refresh carried the chat + * — moved to the top, which was the bug. With the chat left where it belongs, a + * refresh re-reads only the HEAD of the keyset and never reaches a months-old + * row, so the row is re-marked by id from ANOTHER window's announcement. + */ +describe('a declassified chat is re-marked in Recents without a reload', () => { + it('patches a scrolled-in row where it sits, from a sibling window', async () => { + const firstPage = Array.from({ length: 10 }, (_, index) => ({ + ...makeSummary(index), + privacy_tier: 'private' as const, + })); + const secondPage = Array.from({ length: 10 }, (_, index) => ({ + ...makeSummary(index + 10), + privacy_tier: 'private' as const, + })); + mocks.listSidebarSessions + .mockResolvedValueOnce({ + data: { sessions: firstPage, has_more: true, next_cursor: 'cursor-page-2' }, + }) + .mockResolvedValueOnce({ + data: { sessions: secondPage, has_more: false, next_cursor: null }, + }); + + const { result } = renderHook(() => useSidebarSessions()); + await waitFor(() => expect(result.current.sessions).toHaveLength(10)); + act(() => result.current.loadMore()); + await waitFor(() => expect(result.current.sessions).toHaveLength(20)); + const order = result.current.sessions.map((session) => session.id); + mocks.listSidebarSessions.mockClear(); + mocks.getSession.mockResolvedValue({ + data: { id: 'session-17', privacy_tier: 'public', privacy_reason: 'declassified_by_user' }, + }); + + // Another window declassified it. + const sibling = new BroadcastChannel('biorouter:session-row'); + try { + sibling.postMessage({ sessionId: 'session-17' }); + await waitFor(() => + expect(result.current.sessions.find((s) => s.id === 'session-17')?.privacy_tier).toBe( + 'public' + ) + ); + } finally { + sibling.close(); + } + + expect(result.current.sessions.map((session) => session.id)).toEqual(order); + expect( + result.current.sessions + .filter((s) => s.id !== 'session-17') + .every((s) => s.privacy_tier === 'private') + ).toBe(true); + // No head refresh was needed — and one could not have reached this row. + expect(mocks.listSidebarSessions).not.toHaveBeenCalled(); + }); +}); diff --git a/ui/desktop/src/components/BioRouterSidebar/useSidebarSessions.ts b/ui/desktop/src/components/BioRouterSidebar/useSidebarSessions.ts index a8c1e59c3..a7acc176d 100644 --- a/ui/desktop/src/components/BioRouterSidebar/useSidebarSessions.ts +++ b/ui/desktop/src/components/BioRouterSidebar/useSidebarSessions.ts @@ -3,6 +3,7 @@ import { listSidebarSessions, type SessionSummary } from '../../api'; import { userActionHeaders } from '../../utils/userAction'; import { subscribeSessionNameChanges } from '../../utils/sessionNameSync'; import { subscribeSessionListChanges, subscribeSessionRemoved } from '../../utils/sessionListCache'; +import { subscribeSessionRowChanges } from '../../utils/sessionRowSync'; export const SIDEBAR_SESSION_PAGE_SIZE = 10; @@ -135,6 +136,26 @@ export default function useSidebarSessions(): SidebarSessionsState { setSessions(remaining); }); + // A chat's classification changed in place — a declassification, here or + // in another window (`sessionRowSync`). Patched by id, for the same reason + // the removal above is: `loadPage(true)` re-reads only the HEAD of the + // keyset, and a declassified chat is usually months old and nowhere near + // it. Until 2026-09-13 the daemon stamped `updated_at` on a declassification, + // so the next head refresh happened to carry the chat (moved to the top, + // which was the bug); with the chat left where it belongs, only this + // reaches its row. + const unsubscribeRows = subscribeSessionRowChanges(({ sessionId, privacy_tier }) => { + let changed = false; + const next = sessionsRef.current.map((session) => { + if (session.id !== sessionId || session.privacy_tier === privacy_tier) return session; + changed = true; + return { ...session, privacy_tier }; + }); + if (!changed) return; + sessionsRef.current = next; + setSessions(next); + }); + window.addEventListener('session-created', scheduleRefresh); window.addEventListener('message-stream-finished', scheduleRefresh); @@ -142,6 +163,7 @@ export default function useSidebarSessions(): SidebarSessionsState { unsubscribeNames(); unsubscribeList(); unsubscribeRemoved(); + unsubscribeRows(); if (refreshTimer !== undefined) window.clearTimeout(refreshTimer); window.removeEventListener('session-created', scheduleRefresh); window.removeEventListener('message-stream-finished', scheduleRefresh); diff --git a/ui/desktop/src/components/sessions/DeclassifySessionDialog.test.tsx b/ui/desktop/src/components/sessions/DeclassifySessionDialog.test.tsx index e58ad0328..6fc3d62b9 100644 --- a/ui/desktop/src/components/sessions/DeclassifySessionDialog.test.tsx +++ b/ui/desktop/src/components/sessions/DeclassifySessionDialog.test.tsx @@ -7,6 +7,8 @@ import type { Session } from '../../api'; const mocks = vi.hoisted(() => ({ declassifySession: vi.fn(), toastError: vi.fn(), + toastSuccess: vi.fn(), + announceSessionRowChanged: vi.fn(), })); vi.mock('../../api', () => ({ @@ -15,13 +17,44 @@ vi.mock('../../api', () => ({ vi.mock('../../toasts', () => ({ toastError: mocks.toastError, - toastSuccess: vi.fn(), + toastSuccess: mocks.toastSuccess, })); vi.mock('../../utils/userAction', () => ({ userActionHeaders: async () => ({ 'X-User-Action': 'test-key' }), })); +vi.mock('../../utils/sessionRowSync', () => ({ + announceSessionRowChanged: mocks.announceSessionRowChanged, +})); + +/** + * Answer the way the generated client (`api/client/client.gen.ts`) really does, + * for BOTH ways of calling it — which is what lets a test here fail on the code + * it replaced rather than on a mock that only fits the new call. + * + * With `throwOnError` the client throws the parsed BODY and the Response is + * gone; a plain-text body stays a string and an empty one becomes `{}` + * (`finalError || {}`). Without it the same body comes back as `error`, beside + * the Response. + */ +function answerLikeTheClient(status: number, body: string) { + return async (options: { throwOnError?: boolean }) => { + if (status === 200) { + const data = { sessionId: s.id, privacyTier: 'public' }; + return { data, request: {}, response: { status } }; + } + const error = body || {}; + if (options.throwOnError) throw error; + return { data: undefined, error, request: {}, response: { status } }; + }; +} + +/** The daemon's 503 body, `privacy::declassify::DECLASSIFY_STORE_BUSY`. */ +const STORE_BUSY = + 'Nothing was changed and this chat was not marked public, because other writes kept the chat ' + + 'store busy. Try again in a moment.'; + const s = { id: 'abc123def456', name: 'Cohort of 4,102 patients', @@ -36,7 +69,7 @@ const s = { beforeEach(() => { vi.clearAllMocks(); - mocks.declassifySession.mockResolvedValue({}); + mocks.declassifySession.mockImplementation(answerLikeTheClient(200, '')); }); afterEach(() => { @@ -159,10 +192,12 @@ describe('DeclassifySessionDialog', () => { path: { session_id: 'abc123def456' }, body: { confirmation: 'def456' }, headers: { 'X-User-Action': 'test-key' }, - throwOnError: true, }) ); await waitFor(() => expect(onDeclassified).toHaveBeenCalledWith('abc123def456')); + // Item 11: the change announces itself, so a second window's lists re-read + // the row instead of waiting for a re-sort that no longer happens. + expect(mocks.announceSessionRowChanged).toHaveBeenCalledWith('abc123def456'); }); it('the single-click path holds the request open for the undo window', async () => { @@ -210,7 +245,6 @@ describe('DeclassifySessionDialog', () => { path: { session_id: 'abc123def456' }, body: { confirmation: null }, headers: { 'X-User-Action': 'test-key' }, - throwOnError: true, }) ); await waitFor(() => expect(onDeclassified).toHaveBeenCalledWith('abc123def456')); @@ -289,8 +323,11 @@ describe('DeclassifySessionDialog', () => { // stale prop, so clicking again fails identically — an unrecoverable // dialog. The strong control is always an acceptable answer to a refusal, // and it is the only one that can recover from a stale grade. - mocks.declassifySession.mockRejectedValue( - new Error("The confirmation did not match the last six characters of this chat's id.") + mocks.declassifySession.mockImplementation( + answerLikeTheClient( + 400, + "The confirmation did not match the last six characters of this chat's id. Nothing was changed." + ) ); render( @@ -313,7 +350,33 @@ describe('DeclassifySessionDialog', () => { it('surfaces a refusal instead of claiming the chat is now public', async () => { const user = userEvent.setup(); const onDeclassified = vi.fn(); - mocks.declassifySession.mockRejectedValue('Nothing was changed.'); + const refusal = + 'This chat does not record an observed turn on a private model as the reason it is private, ' + + 'so marking it public needs your operating system to confirm it is you. That did not happen, ' + + 'and nothing was changed.'; + mocks.declassifySession.mockImplementation(answerLikeTheClient(403, refusal)); + + render( + + ); + await user.type(screen.getByLabelText(/last 6 characters/i), 'def456'); + await user.click(screen.getByRole('button', { name: /Make public/ })); + + await waitFor(() => + expect(mocks.toastError).toHaveBeenCalledWith({ + title: 'Could not mark this chat public', + msg: refusal, + }) + ); + expect(onDeclassified).not.toHaveBeenCalled(); + expect(mocks.toastSuccess).not.toHaveBeenCalled(); + expect(mocks.announceSessionRowChanged).not.toHaveBeenCalled(); + }); + + it('a thrown request (the daemon unreachable) is a failure, not a success', async () => { + const user = userEvent.setup(); + const onDeclassified = vi.fn(); + mocks.declassifySession.mockRejectedValue(new TypeError('Failed to fetch')); render( @@ -323,5 +386,64 @@ describe('DeclassifySessionDialog', () => { await waitFor(() => expect(mocks.toastError).toHaveBeenCalled()); expect(onDeclassified).not.toHaveBeenCalled(); + expect(mocks.toastSuccess).not.toHaveBeenCalled(); + }); + + // Item 8 of the 1.90.4 hold (2026-09-13). Reproduced in the running app by + // holding `sessions.db`'s write lock across one single-click declassification: + // the daemon answered a bodyless 500 after its five-second busy wait, the toast + // read "Could not mark this chat public / [object Object]", and the dialog + // swapped the single click for the typed phrase under "That request was + // refused. This chat's record has changed since this list was loaded" — none + // of which was true. + it('a busy store says so in the daemon’s words and keeps the single click', async () => { + mocks.declassifySession.mockImplementation(answerLikeTheClient(503, STORE_BUSY)); + const onDeclassified = vi.fn(); + + render( + + ); + fireEvent.click(screen.getByRole('button', { name: /Make public/ })); + + await waitFor(() => + expect(mocks.toastError).toHaveBeenCalledWith({ + title: 'The chat store was busy', + msg: STORE_BUSY, + }) + ); + // Not escalated: a busy store says nothing about this chat's grade, so the + // control it was offered is still the right one to try again with. + await waitFor(() => expect(screen.getByRole('button', { name: /Make public/ })).toBeEnabled()); + expect(screen.queryByRole('textbox')).toBeNull(); + expect(screen.queryByText(/record has changed/i)).toBeNull(); + // And never reported public. + expect(onDeclassified).not.toHaveBeenCalled(); + expect(mocks.toastSuccess).not.toHaveBeenCalled(); + expect(mocks.announceSessionRowChanged).not.toHaveBeenCalled(); + }); + + it('a failure with no body is described, never shown as [object Object]', async () => { + mocks.declassifySession.mockImplementation(answerLikeTheClient(500, '')); + + render( + + ); + fireEvent.click(screen.getByRole('button', { name: /Make public/ })); + + await waitFor(() => expect(mocks.toastError).toHaveBeenCalled()); + const { title, msg } = mocks.toastError.mock.calls[0][0] as { title: string; msg: string }; + expect(msg).not.toContain('[object Object]'); + expect(msg).toMatch(/not marked public/); + expect(title).toBe('Could not mark this chat public'); + expect(screen.queryByRole('textbox')).toBeNull(); }); }); diff --git a/ui/desktop/src/components/sessions/DeclassifySessionDialog.tsx b/ui/desktop/src/components/sessions/DeclassifySessionDialog.tsx index 6fe1478ec..daa02e704 100644 --- a/ui/desktop/src/components/sessions/DeclassifySessionDialog.tsx +++ b/ui/desktop/src/components/sessions/DeclassifySessionDialog.tsx @@ -2,6 +2,7 @@ import { useCallback, useEffect, useLayoutEffect, useRef, useState } from 'react import { declassifySession, type Session } from '../../api'; import { toastError, toastSuccess } from '../../toasts'; import { userActionHeaders } from '../../utils/userAction'; +import { announceSessionRowChanged } from '../../utils/sessionRowSync'; import { DangerousConfirmDialog } from '../ui/DangerousConfirmDialog'; import { Dialog, @@ -83,6 +84,53 @@ export function confirmationPhrase(sessionId: string): string { return [...sessionId].slice(-6).join(''); } +/** + * What went wrong, in the words the person is shown — item 8 of the 1.90.4 + * hold (2026-09-13). + * + * The daemon answers every failure of `POST /sessions/{id}/declassify` with a + * plain-text sentence, and that sentence is the message. It did not always: a + * lock timeout was a bodyless 500, the generated client throws the parsed body + * rather than the Response, and `String({})` put **`[object Object]`** in the + * toast. Measured in the running app with the store's write lock held across + * one click. + * + * So the status is read off the Response, never inferred from the body, and a + * body that is not a sentence gets one written here — still claiming only what + * a failed request guarantees. No Response at all (`status` undefined) means + * the request never reached the daemon; a raw `TypeError: Failed to fetch` is + * not something a person can act on, so it is not shown either. + * + * A 503 is the daemon saying the chat store stayed write-locked by other work + * past its wait: the one failure a retry clears, titled so it cannot be read + * as a refusal. + */ +export interface DeclassifyFailure { + status: number | undefined; + title: string; + message: string; +} + +export function describeDeclassifyFailure( + status: number | undefined, + body: unknown +): DeclassifyFailure { + const sentence = typeof body === 'string' && body.trim().length > 0 ? body.trim() : undefined; + let message: string; + if (status === undefined) { + message = 'Biorouter could not be reached, so this chat was not marked public.'; + } else { + message = + sentence ?? + `Biorouter answered ${status} without saying why, and this chat was not marked public.`; + } + return { + status, + title: status === 503 ? 'The chat store was busy' : 'Could not mark this chat public', + message, + }; +} + type Phase = 'confirm' | 'undo' | 'sending'; export interface DeclassifySessionDialogProps { @@ -163,36 +211,59 @@ export function DeclassifySessionDialog({ const send = useCallback( async (confirmation: string | null) => { setPhase('sending'); + let failure: DeclassifyFailure | null = null; try { - await declassifySession({ + // NOT `throwOnError`: the generated client then throws the parsed BODY + // and drops the Response, and the status is what separates a refusal + // from a busy store. See `describeDeclassifyFailure`. + const result = await declassifySession({ path: { session_id: session.id }, body: { confirmation }, // DR-16's proof-of-user. Without it the daemon refuses, correctly: // the server secret alone is reachable from any developer-enabled // agent shell (§9.3 A1) and is not evidence of a human. headers: await userActionHeaders(), - throwOnError: true, }); + // Success is a 200 and nothing else. A missing Response (the fetch + // failed) or any other status is a failure, whatever `data` holds — a + // private chat must never be reported public when the write did not + // land. + if (result.response?.status !== 200) { + failure = describeDeclassifyFailure(result.response?.status, result.error); + } + } catch (error) { + failure = describeDeclassifyFailure(undefined, error); + } + + if (failure === null) { toastSuccess({ title: 'Chat marked public', msg: 'It no longer carries a private marker. The change is recorded.', }); + // Every list surface in every window re-reads this row in place (item + // 11: the daemon no longer re-sorts it to say something happened). + announceSessionRowChanged(session.id); latest.current.onDeclassified?.(session.id); latest.current.onClose?.(); - } catch (error) { - toastError({ - title: 'Could not mark this chat public', - msg: error instanceof Error ? error.message : String(error), - }); - // A request that carried no confirmation was refused, so the weak - // control this dialog rendered was the wrong one — most likely because - // the cached row's `turn:*` provenance has since been displaced by an - // `mcp:*` one. Returning to the same control would re-render it from - // the same stale prop and fail identically, forever. Escalating is the - // only recovery, and it is never the wrong answer to a refusal. - if (confirmation === null) setEscalated(true); - setPhase('confirm'); + return; } + + toastError({ title: failure.title, msg: failure.message }); + // A request that carried no confirmation was answered "the confirmation + // did not match" (400), so the weak control this dialog rendered was the + // wrong one — most likely because the cached row's `turn:*` provenance + // has since been displaced by an `mcp:*` one. Returning to the same + // control would re-render it from the same stale prop and fail + // identically, forever, so it escalates. + // + // ⚠ **Only on that answer.** This used to escalate on ANY failure, and a + // lock timeout then swapped the single click for the typed phrase under a + // sentence claiming "this chat's record has changed since this list was + // loaded" — a claim about the chat that a busy store does not make. A + // busy store, a network failure or a daemon fault leaves the grade + // exactly as it was, so the same control is the right one to try again. + if (confirmation === null && failure.status === 400) setEscalated(true); + setPhase('confirm'); }, [session.id] ); diff --git a/ui/desktop/src/components/sessions/SessionHistoryView.browserSurface.test.tsx b/ui/desktop/src/components/sessions/SessionHistoryView.browserSurface.test.tsx index 5103af2b2..2ac6b134f 100644 --- a/ui/desktop/src/components/sessions/SessionHistoryView.browserSurface.test.tsx +++ b/ui/desktop/src/components/sessions/SessionHistoryView.browserSurface.test.tsx @@ -63,7 +63,13 @@ function renderPrivatePage() { beforeEach(() => { vi.clearAllMocks(); - mocks.declassifySession.mockResolvedValue({}); + // The shape the generated client resolves a 200 with. The dialog reads the + // status off the Response, so a bare `{}` — no Response at all — is a failed + // request, exactly as it is at runtime. + mocks.declassifySession.mockResolvedValue({ + data: { sessionId: 'x', privacyTier: 'public' }, + response: { status: 200 }, + }); }); afterEach(() => { diff --git a/ui/desktop/src/components/sessions/SessionHistoryView.test.tsx b/ui/desktop/src/components/sessions/SessionHistoryView.test.tsx index 78aec9abd..7687af3fa 100644 --- a/ui/desktop/src/components/sessions/SessionHistoryView.test.tsx +++ b/ui/desktop/src/components/sessions/SessionHistoryView.test.tsx @@ -5,9 +5,12 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'; import SessionHistoryView from './SessionHistoryView'; import type { Session } from '../../api'; -const mocks = vi.hoisted(() => ({ declassifySession: vi.fn() })); +const mocks = vi.hoisted(() => ({ declassifySession: vi.fn(), getSession: vi.fn() })); -vi.mock('../../api', () => ({ declassifySession: mocks.declassifySession })); +vi.mock('../../api', () => ({ + declassifySession: mocks.declassifySession, + getSession: mocks.getSession, +})); vi.mock('../../utils/userAction', () => ({ userActionHeaders: async () => ({ 'X-User-Action': 'test-key' }), @@ -60,7 +63,13 @@ function renderView(over: Partial = {}, showActionButtons = false) { beforeEach(() => { vi.clearAllMocks(); - mocks.declassifySession.mockResolvedValue({}); + // The shape the generated client resolves a 200 with. The dialog reads the + // status off the Response, so a bare `{}` — no Response at all — is a failed + // request, exactly as it is at runtime. + mocks.declassifySession.mockResolvedValue({ + data: { sessionId: 'x', privacyTier: 'public' }, + response: { status: 200 }, + }); }); describe('SessionHistoryView — the privacy marker', () => { @@ -118,6 +127,41 @@ describe('SessionHistoryView — declassification', () => { expect(screen.getByTestId('privacy-badge')).toHaveAttribute('data-privacy', 'public') ); }); + + // Item 11 of the 1.90.4 hold (2026-09-13): a declassification made in ANOTHER + // window reaches this page too. The page reads its `session` prop once, and + // the daemon no longer re-sorts the chat to say something happened, so the + // badge follows the row announcement — and states the daemon's read of it. + it('follows a declassification made in another window', async () => { + mocks.getSession.mockResolvedValue({ + data: { + id: '20260714_130000', + privacy_tier: 'public', + privacy_reason: 'declassified_by_user', + }, + }); + renderView({ + privacy_tier: 'private', + privacy_reason: 'turn:versa_azure', + id: '20260714_130000', + }); + expect(screen.getByTestId('privacy-badge')).toHaveAttribute('data-privacy', 'private'); + + const otherWindow = new BroadcastChannel('biorouter:session-row'); + try { + // A different chat first: this page must not move for it. + otherWindow.postMessage({ sessionId: 'someone-else' }); + otherWindow.postMessage({ sessionId: '20260714_130000' }); + await waitFor(() => + expect(screen.getByTestId('privacy-badge')).toHaveAttribute('data-privacy', 'public') + ); + } finally { + otherWindow.close(); + } + expect(mocks.getSession).toHaveBeenCalledWith( + expect.objectContaining({ path: { session_id: '20260714_130000' } }) + ); + }); }); /** diff --git a/ui/desktop/src/components/sessions/SessionHistoryView.tsx b/ui/desktop/src/components/sessions/SessionHistoryView.tsx index 7de042438..665329ce4 100644 --- a/ui/desktop/src/components/sessions/SessionHistoryView.tsx +++ b/ui/desktop/src/components/sessions/SessionHistoryView.tsx @@ -39,6 +39,7 @@ import { Message, Session } from '../../api'; import { PrivacyBadge } from '../ui/PrivacyBadge'; import { DeclassifySessionDialog } from './DeclassifySessionDialog'; import { DECLASSIFY_NEEDS_HOST_SHORT, declassifyBrowserReason } from './declassifyOnBrowser'; +import { subscribeSessionRowChanges } from '../../utils/sessionRowSync'; import { useNavigation } from '../../hooks/useNavigation'; import { ReadableContent } from '../Layout/ReadableContent'; import { MODAL_SIZE } from '../ModalShell'; @@ -217,6 +218,17 @@ const SessionHistoryView: React.FC = ({ const artifactPanel = useArtifactPanel({ isMobile: useIsMobile(), allowWindowResize: false }); const { splitPaneRef, artifact: presentedArtifact, openArtifact } = artifactPanel; useEffect(() => setTier(session.privacy_tier), [session.privacy_tier]); + // …and follow a declassification made anywhere else — another window's + // History row, or this chat's own page open twice. The `session` prop is read + // once when the page opens, so without this the badge above a chat that is + // no longer private would stay private until the page was reopened. + useEffect( + () => + subscribeSessionRowChanges(({ sessionId, privacy_tier }) => { + if (sessionId === session.id) setTier(privacy_tier); + }), + [session.id] + ); const messages = session.conversation || []; const billedTokenEstimate = billedSessionTokenEstimate(session); diff --git a/ui/desktop/src/components/sessions/SessionListView.browserSurface.test.tsx b/ui/desktop/src/components/sessions/SessionListView.browserSurface.test.tsx index 8c095c352..b186a8dd0 100644 --- a/ui/desktop/src/components/sessions/SessionListView.browserSurface.test.tsx +++ b/ui/desktop/src/components/sessions/SessionListView.browserSurface.test.tsx @@ -83,7 +83,13 @@ function openRowMenu() { beforeEach(() => { vi.clearAllMocks(); clearSessionListCache(); - mocks.declassifySession.mockResolvedValue({}); + // The shape the generated client resolves a 200 with. The dialog reads the + // status off the Response, so a bare `{}` — no Response at all — is a failed + // request, exactly as it is at runtime. + mocks.declassifySession.mockResolvedValue({ + data: { sessionId: 'x', privacyTier: 'public' }, + response: { status: 200 }, + }); mocks.listSessions.mockResolvedValue({ data: { sessions: [ diff --git a/ui/desktop/src/components/sessions/SessionListView.declassify.test.tsx b/ui/desktop/src/components/sessions/SessionListView.declassify.test.tsx index 1696d5468..ba8121310 100644 --- a/ui/desktop/src/components/sessions/SessionListView.declassify.test.tsx +++ b/ui/desktop/src/components/sessions/SessionListView.declassify.test.tsx @@ -58,7 +58,13 @@ beforeEach(() => { vi.clearAllMocks(); clearSessionListCache(); mocks.listSessions.mockResolvedValue({ data: { sessions: [] } }); - mocks.declassifySession.mockResolvedValue({}); + // The shape the generated client resolves a 200 with. The dialog reads the + // status off the Response, so a bare `{}` — no Response at all — is a failed + // request, exactly as it is at runtime. + mocks.declassifySession.mockResolvedValue({ + data: { sessionId: 'x', privacyTier: 'public' }, + response: { status: 200 }, + }); }); function row(overrides: Partial & { id: string; name: string }): Session { diff --git a/ui/desktop/src/utils/sessionListCache.test.ts b/ui/desktop/src/utils/sessionListCache.test.ts index a6e5c6f34..b1b43a01e 100644 --- a/ui/desktop/src/utils/sessionListCache.test.ts +++ b/ui/desktop/src/utils/sessionListCache.test.ts @@ -8,15 +8,18 @@ import { subscribeSessionListChanges, } from './sessionListCache'; import { announceSessionName } from './sessionNameSync'; +import { announceSessionRowChanged } from './sessionRowSync'; const mocks = vi.hoisted(() => ({ listSessions: vi.fn(), updateSessionName: vi.fn(), + getSession: vi.fn(), })); vi.mock('../api', () => ({ listSessions: mocks.listSessions, updateSessionName: mocks.updateSessionName, + getSession: mocks.getSession, })); // The proof the desktop sends. Since issue #56's QA sweep (2026-09-10) a list @@ -250,4 +253,34 @@ describe('sessionListCache', () => { await vi.waitFor(() => expect(mocks.listSessions).toHaveBeenCalled()); unsub(); }); + + // Item 11 of the 1.90.4 hold (2026-09-13). A declassification no longer + // re-sorts the chat, so nothing about the LIST changes and no refetch would + // notice. History's badge and its row menu read this entry, so it is patched + // where it sits — from the daemon's read, not from the announcement. + it('re-marks a declassified chat in place, without refetching or reordering', async () => { + mocks.listSessions.mockResolvedValue({ + data: { + sessions: [ + { id: 'recent', privacy_tier: 'private', privacy_reason: 'turn:versa_azure' }, + { id: 'old', privacy_tier: 'private', privacy_reason: 'backfill:ollama' }, + ], + }, + }); + await refreshSessionList(); + mocks.listSessions.mockClear(); + mocks.getSession.mockResolvedValue({ + data: { id: 'old', privacy_tier: 'public', privacy_reason: 'declassified_by_user' }, + }); + + announceSessionRowChanged('old'); + + await vi.waitFor(() => + expect(getCachedSessionList()).toEqual([ + { id: 'recent', privacy_tier: 'private', privacy_reason: 'turn:versa_azure' }, + { id: 'old', privacy_tier: 'public', privacy_reason: 'declassified_by_user' }, + ]) + ); + expect(mocks.listSessions).not.toHaveBeenCalled(); + }); }); diff --git a/ui/desktop/src/utils/sessionListCache.ts b/ui/desktop/src/utils/sessionListCache.ts index 44312fbb2..71719729a 100644 --- a/ui/desktop/src/utils/sessionListCache.ts +++ b/ui/desktop/src/utils/sessionListCache.ts @@ -1,6 +1,7 @@ import { listSessions, type Session } from '../api'; import { userActionHeaders } from './userAction'; import { subscribeSessionNameChanges } from './sessionNameSync'; +import { subscribeSessionRowChanges } from './sessionRowSync'; let cachedSessions: Session[] | null = null; let inFlightRequest: Promise | null = null; @@ -58,6 +59,36 @@ subscribeSessionNameChanges(({ sessionId, name, userSetName }) => { emitChange(); }); +// A row whose classification changed in place — a declassification, in this +// window or another (`sessionRowSync`). History's badge, its row menu ("Make +// this chat public" is offered on private rows only), Home recents and the tab +// strip's cached tiers all read this cache, so the entry is patched where it +// sits. Nothing is re-sorted: the daemon no longer moves `updated_at` for a +// classification change, so the row's place in the list is still right. +// +// ⚠ Unlike the name channel above, a fact that lands while a list request is in +// flight is NOT re-applied over that request's answer. The row read may have +// been ISSUED before the list was, so replaying it could write an older +// `public` over a newer `private` — the wrong direction for a privacy badge. A +// list that snaps a row back to private is the safe miss, and the next +// announcement or refresh corrects it. +subscribeSessionRowChanges(({ sessionId, privacy_tier, privacy_reason }) => { + if (!cachedSessions) return; + const idx = cachedSessions.findIndex((s) => s.id === sessionId); + if (idx === -1) return; + const current = cachedSessions[idx]; + if ( + current.privacy_tier === privacy_tier && + (current.privacy_reason ?? null) === privacy_reason + ) { + return; + } + const next = cachedSessions.slice(); + next[idx] = { ...current, privacy_tier, privacy_reason }; + cachedSessions = next; + emitChange(); +}); + // ── Cross-window "the set of sessions changed" signal ────────────────────── // A sibling to sessionNameSync's name channel, for list MEMBERSHIP: a session // created, diverged, deleted or imported. Every list surface — sidebar Recents, diff --git a/ui/desktop/src/utils/sessionRowSync.test.ts b/ui/desktop/src/utils/sessionRowSync.test.ts new file mode 100644 index 000000000..15fcec9f5 --- /dev/null +++ b/ui/desktop/src/utils/sessionRowSync.test.ts @@ -0,0 +1,156 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { + announceSessionRowChanged, + subscribeSessionRowChanges, + type SessionRowFacts, +} from './sessionRowSync'; + +/** + * Item 11 of the 1.90.4 hold (2026-09-13). Declassifying a chat no longer moves + * its `updated_at`, and before that change a second window never learned of a + * declassification at all (measured: History open in two windows, window B kept + * the private badge for the full 30 seconds watched). This channel is what + * tells it. + * + * Under vitest's jsdom environment `BroadcastChannel` is Node's, so a second + * `new BroadcastChannel(name)` in the test stands in for another window. The + * receive path is asynchronous twice over — the delivery, then the re-read — so + * every assertion WAITS for what it expects rather than ticking the loop once; + * see `sessionBindingSync.test.ts` for how a single `setTimeout(0)` loses that + * race. + */ +const CHANNEL_NAME = 'biorouter:session-row'; + +const mocks = vi.hoisted(() => ({ + getSession: vi.fn(), +})); + +vi.mock('../api', () => ({ + getSession: mocks.getSession, +})); + +vi.mock('./userAction', () => ({ + userActionHeaders: async () => ({ 'X-User-Action': 'test-proof' }), +})); + +function row(id: string, privacy_tier: 'public' | 'private', privacy_reason: string | null) { + return { data: { id, privacy_tier, privacy_reason } }; +} + +describe('sessionRowSync', () => { + const unsubscribes: Array<() => void> = []; + let other: BroadcastChannel; + + beforeEach(() => { + vi.clearAllMocks(); + other = new BroadcastChannel(CHANNEL_NAME); + }); + + afterEach(() => { + // The listener set is process-global: a failed assertion must not leave a + // listener behind for the next test to receive. + while (unsubscribes.length > 0) unsubscribes.pop()!(); + other.close(); + }); + + function listen(): SessionRowFacts[] { + const seen: SessionRowFacts[] = []; + unsubscribes.push(subscribeSessionRowChanges((facts) => seen.push(facts))); + return seen; + } + + it('re-reads a row another window announced and hands the read to every subscriber', async () => { + mocks.getSession.mockResolvedValue(row('s1', 'public', 'declassified_by_user')); + const first = listen(); + const second = listen(); + + other.postMessage({ sessionId: 's1' }); + + const expected = { + sessionId: 's1', + privacy_tier: 'public', + privacy_reason: 'declassified_by_user', + }; + await vi.waitFor(() => expect(second).toEqual([expected])); + expect(first).toEqual([expected]); + // ONE read for the window, shared by both subscribers, carrying the proof a + // private chat's row needs. + expect(mocks.getSession).toHaveBeenCalledTimes(1); + expect(mocks.getSession).toHaveBeenCalledWith({ + path: { session_id: 's1' }, + query: { metadata_only: true }, + headers: { 'X-User-Action': 'test-proof' }, + throwOnError: true, + }); + }); + + it('delivers what the daemon read, never what the message claimed', async () => { + // A chat declassified and raised straight back by a turn: the message says + // nothing about the tier, and if it did it would be the stale half. + mocks.getSession.mockResolvedValue(row('s2', 'private', 'turn:versa_azure')); + const seen = listen(); + + other.postMessage({ sessionId: 's2', privacy_tier: 'public' }); + + await vi.waitFor(() => expect(seen).toHaveLength(1)); + expect(seen[0].privacy_tier).toBe('private'); + }); + + it('reads nothing for a malformed message', async () => { + mocks.getSession.mockImplementation(async ({ path }: { path: { session_id: string } }) => + row(path.session_id, 'public', null) + ); + const seen = listen(); + + other.postMessage({}); + other.postMessage(null); + other.postMessage({ sessionId: 7 }); + other.postMessage({ sessionId: '' }); + // Posted last on the same port, so its arrival proves the four above were + // already processed. + other.postMessage({ sessionId: 'barrier' }); + + await vi.waitFor(() => expect(seen.map((f) => f.sessionId)).toEqual(['barrier'])); + expect(mocks.getSession).toHaveBeenCalledTimes(1); + }); + + it('does not deliver a read that a newer read of the same chat overtook', async () => { + let finishOlder: ((value: unknown) => void) | undefined; + mocks.getSession + .mockImplementationOnce( + () => + new Promise((resolve) => { + finishOlder = resolve; + }) + ) + .mockResolvedValueOnce(row('s3', 'public', 'declassified_by_user')); + const seen = listen(); + + announceSessionRowChanged('s3'); + await vi.waitFor(() => expect(mocks.getSession).toHaveBeenCalledTimes(1)); + announceSessionRowChanged('s3'); + await vi.waitFor(() => expect(seen).toHaveLength(1)); + + // The older answer lands last, holding the older fact. + finishOlder!(row('s3', 'private', 'turn:versa_azure')); + await new Promise((resolve) => setTimeout(resolve, 20)); + expect(seen).toEqual([ + { sessionId: 's3', privacy_tier: 'public', privacy_reason: 'declassified_by_user' }, + ]); + }); + + it('re-reads in the announcing window too, and a failed read changes nothing', async () => { + mocks.getSession + .mockRejectedValueOnce(new Error('403')) + .mockResolvedValueOnce(row('s4', 'public', 'declassified_by_user')); + const seen = listen(); + + announceSessionRowChanged('s4'); + await vi.waitFor(() => expect(mocks.getSession).toHaveBeenCalledTimes(1)); + await new Promise((resolve) => setTimeout(resolve, 20)); + expect(seen).toEqual([]); + + announceSessionRowChanged('s4'); + await vi.waitFor(() => expect(seen).toHaveLength(1)); + }); +}); diff --git a/ui/desktop/src/utils/sessionRowSync.ts b/ui/desktop/src/utils/sessionRowSync.ts new file mode 100644 index 000000000..f0721a2fc --- /dev/null +++ b/ui/desktop/src/utils/sessionRowSync.ts @@ -0,0 +1,141 @@ +import { getSession, type SessionClassification } from '../api'; +import { userActionHeaders } from './userAction'; + +/** + * "A chat's ROW changed in place" — for a change the list surfaces render but + * that moves nothing in the list. Today exactly one writer needs it: a + * declassification (issue #56 §12.4). + * + * # Why this exists — item 11 of the 1.90.4 hold (2026-09-13) + * + * `privacy::declassify` used to stamp `updated_at = datetime('now')`, which put + * a chat created months ago into History's "Today" (measured across 796 + * declassifications). The stamp is gone, and the obvious worry was that + * something learned about a declassification *because* `updated_at` moved. + * Measured before removing it, with History open in two windows: window A + * declassified `20260803_1550`, and window B went on badging it private for the + * full 30 seconds watched. Nothing crossed. B learned only when some unrelated + * refresh re-read a list — which the stamp had re-sorted, so the chat came back + * at the TOP. Without the stamp that incidental path is gone for the sidebar + * outright: `useSidebarSessions` re-reads only the head of its keyset, and a + * months-old row is never in the head. + * + * So the change announces itself, and every list surface re-reads the row in + * place: + * + * - `sessionListCache` (History's rows, Home recents, the tab strip's cached + * tiers) patches the entry it holds; + * - `useSidebarSessions` patches the row it holds, wherever it sits; + * - `SessionHistoryView` moves its page badge. + * + * An open chat needs none of this: `GET /sessions/changes` compares + * `privacy_tier` and `privacy_reason` (never `updated_at`), and its poll tells + * the chat's controller to re-read within about two seconds. + * + * # The announcement is a nudge, never a payload + * + * The same rule `sessionBindingSync`'s app-model nudge and `sessionMetaSubscription` + * state: a receiver is handed an id and goes to look. A declassified chat can be + * raised straight back by a turn a moment later, and a window that applied + * "public" from a message would show a public badge over a private chat — the + * one direction a privacy badge must never be wrong in. Re-reading the daemon + * ends on whichever WRITE landed last. + * + * ⚠ **One read per announcement per window, shared by every subscriber.** The + * row is read here and handed to the subscribers, rather than each subscriber + * fetching for itself, so a declassification costs one `GET` per window. + * + * ⚠ **Announce only what the daemon accepted.** Call it after the write + * resolved 200; a nudge that outran its write would re-read the value it was + * sent to replace. + */ + +export interface SessionRowFacts { + sessionId: string; + privacy_tier: SessionClassification; + privacy_reason: string | null; +} + +type Listener = (facts: SessionRowFacts) => void; + +const listeners = new Set(); + +/** + * The newest read issued for each chat. Two announcements for one chat can + * overlap, and the answer to the first may arrive second; only the newest read + * is delivered. Generations come from one counter that never resets, so a read + * issued after an entry was cleared can never share a number with an older one + * still in flight. + */ +const newestRead = new Map(); +let readCounter = 0; + +let channel: BroadcastChannel | null = null; + +function getChannel(): BroadcastChannel | null { + if (channel) return channel; + // Lazy, and absent-tolerant, so a test environment without BroadcastChannel + // still loads the module — as `sessionNameSync` and `sessionBindingSync` do. + if (typeof BroadcastChannel === 'undefined') return null; + channel = new BroadcastChannel('biorouter:session-row'); + channel.onmessage = (event: MessageEvent) => { + // Shape-checked, not trusted: this arrives from another window. + const sessionId = (event.data as { sessionId?: unknown } | null | undefined)?.sessionId; + if (typeof sessionId !== 'string' || sessionId.length === 0) return; + void reread(sessionId); + }; + return channel; +} + +async function reread(sessionId: string): Promise { + if (listeners.size === 0) return; + const generation = ++readCounter; + newestRead.set(sessionId, generation); + try { + const response = await getSession({ + path: { session_id: sessionId }, + // The row, not the transcript: this wants two strings. + query: { metadata_only: true }, + // A private chat's row is refused without the proof-of-user, and the + // chat this exists for may still be private by the time it is read. + headers: await userActionHeaders(), + throwOnError: true, + }); + const row = response.data; + if (newestRead.get(sessionId) !== generation) return; + if (!row || row.id !== sessionId || !row.privacy_tier) return; + const facts: SessionRowFacts = { + sessionId, + privacy_tier: row.privacy_tier, + privacy_reason: row.privacy_reason ?? null, + }; + for (const listener of [...listeners]) listener(facts); + } catch { + // Silent, as `refreshSessionBinding` is: a read that failed leaves every + // surface exactly as stale as it was, which is not worth a toast. + } finally { + if (newestRead.get(sessionId) === generation) newestRead.delete(sessionId); + } +} + +/** + * Announce that `sessionId`'s row changed in place. Re-reads it for this window + * and tells every other window to do the same. + */ +export function announceSessionRowChanged(sessionId: string): void { + void reread(sessionId); + getChannel()?.postMessage({ sessionId }); +} + +/** + * Subscribe to freshly-read rows. Returns the unsubscribe. Subscribe from a + * MOUNT, or — for a module that owns a cache, as `sessionListCache` does its + * name channel — at module scope; never from a getter. + */ +export function subscribeSessionRowChanges(listener: Listener): () => void { + getChannel(); + listeners.add(listener); + return () => { + listeners.delete(listener); + }; +} From debdff89431766757e156139790a882da824d81a Mon Sep 17 00:00:00 2001 From: Wanjun Gu Date: Sun, 13 Sep 2026 18:35:00 -0700 Subject: [PATCH 2/4] fix(privacy): a declassified tab lowers, a raise reaches every window, and failures say only what was read Repairs the four defects an independent tester found in this PR's first round. Privacy-critical desktop code: needs a human reviewer. No Rust changed; who may declassify and the write-first lock ordering are untouched. D1 - a declassified chat's open tab kept its private icon (past 12 min). ChatStreamRegistry's live tier map only ever rose, "mirroring the ratchet", and the ratchet has one exit. It now adopts each store's newest defined reading (a store with no row still retracts nothing), and a row read made in the window nudges a store holding another tier to re-read, which also covers stores past the change feed's 64-id cap. mergeSessionTiers stays max across sources. D4 - after a pushed lowering, a turn re-raised the chat and another window's History and sidebar rows badged it PUBLIC. A store that sees its chat's tier change now announces it on sessionRowSync (not re-announced when the window was already handed that tier). A list or sidebar answer that raced a row read shows the higher tier and is settled by a third read, instead of adopting an older list over a newer raise. D2 - an answer lost after the daemon wrote was reported as "not marked public". After any non-200 the dialog reads the row: public is a success, private says "still private", unreadable claims neither. D3a - failure toasts never expire, so an overturned failure stayed beside the success. A chat's failure toast is retracted by the next outcome, across a close and reopen, and by a row read showing the chat public. D3b - a Radix modal sets pointer-events:none on , so a toast above the dialog could not be clicked and the press closed the dialog. Toast cards take pointer events back (main.css) and a press on the toast layer is no longer a press outside the dialog (dialog.tsx). --- docs/security/privacy-tiers.md | 12 +- .../useSidebarSessions.test.ts | 57 ++++ .../BioRouterSidebar/useSidebarSessions.ts | 37 ++- .../components/chatGroups/ChatGroupsShell.tsx | 24 +- .../src/components/privacy/sessionTier.ts | 26 +- .../sessions/DeclassifySessionDialog.test.tsx | 259 +++++++++++++++++- .../sessions/DeclassifySessionDialog.tsx | 140 ++++++++-- ui/desktop/src/components/ui/dialog.test.tsx | 38 +++ ui/desktop/src/components/ui/dialog.tsx | 20 ++ .../hooks/chatStreamStore.declassify.test.tsx | 240 ++++++++++++++++ ui/desktop/src/hooks/chatStreamStore.tsx | 83 +++++- ui/desktop/src/styles/main.css | 15 + ui/desktop/src/styles/toastLayer.test.ts | 14 + ui/desktop/src/utils/sessionListCache.test.ts | 108 ++++++++ ui/desktop/src/utils/sessionListCache.ts | 49 ++-- ui/desktop/src/utils/sessionRowSync.test.ts | 32 +++ ui/desktop/src/utils/sessionRowSync.ts | 159 +++++++++-- 17 files changed, 1211 insertions(+), 102 deletions(-) create mode 100644 ui/desktop/src/hooks/chatStreamStore.declassify.test.tsx diff --git a/docs/security/privacy-tiers.md b/docs/security/privacy-tiers.md index c6b64bda6..58b549eb9 100644 --- a/docs/security/privacy-tiers.md +++ b/docs/security/privacy-tiers.md @@ -132,12 +132,20 @@ this section is the ledger. not move the chat's `updated_at`**: a classification change is not use of the chat, and stamping it put months-old chats under History's "Today". The desktop announces the change on its own channel instead (`ui/desktop/src/utils/sessionRowSync.ts`), so a second window's History row, - sidebar row and session page re-read the row in place. **A declassification that only waited out + sidebar row and session page re-read the row in place. ⚠ **That channel carries raises too, and + must.** Pushing only the lowering let a second window badge a chat PUBLIC after a turn had raised + it straight back — a failure the renderer could not produce before the push existed. A chat store + that sees its chat's tier change announces it (`ChatStreamRegistry.noteControllerTier`), and a + list answer that raced a row read shows the higher tier until a third read settles it. The tab + strip's live map follows its store down as well as up; it used to only rise, which kept a + declassified chat's open tab private until a reload. **A declassification that only waited out the store's busy timeout answers `503` with `Retry-After` and a sentence** (`privacy::declassify::DECLASSIFY_STORE_BUSY`), distinct from a genuine failure's `500` (`routes::session::DECLASSIFY_FAILED`); both mean nothing landed, and neither retries inside the call — SQLite's busy handler already waited five seconds, and a second - attempt under the load that causes this was measured to fail again three times in seven. + attempt under the load that causes this was measured to fail again three times in seven. The + desktop never reports an outcome it did not read: after any answer that is not a `200` it reads + the row, because an answer can be lost after the daemon wrote. ⚠ **R18's "no cached grant" is honoured on Linux and NOT guaranteed on macOS (F-13).** R18 asks for a prompt "raised once per operation (no session, no cached grant)". The Linux prompter diff --git a/ui/desktop/src/components/BioRouterSidebar/useSidebarSessions.test.ts b/ui/desktop/src/components/BioRouterSidebar/useSidebarSessions.test.ts index 59ad1f427..77232f7e4 100644 --- a/ui/desktop/src/components/BioRouterSidebar/useSidebarSessions.test.ts +++ b/ui/desktop/src/components/BioRouterSidebar/useSidebarSessions.test.ts @@ -257,4 +257,61 @@ describe('a declassified chat is re-marked in Recents without a reload', () => { // No head refresh was needed — and one could not have reached this row. expect(mocks.listSidebarSessions).not.toHaveBeenCalled(); }); + + /** + * Defect D4 of the 2026-09-13 repair round, the sidebar half. A head refresh + * issued before a turn raised a chat, answered after the raise was patched + * in, drew the row public again. + */ + it('a page that raced a raise does not draw the row public again', async () => { + const publicRow = { ...makeSummary(0), privacy_tier: 'public' as const }; + mocks.listSidebarSessions.mockResolvedValueOnce({ + data: { sessions: [publicRow], has_more: false, next_cursor: null }, + }); + const { result } = renderHook(() => useSidebarSessions()); + await waitFor(() => expect(result.current.sessions).toHaveLength(1)); + + let answerPage: ((value: unknown) => void) | undefined; + mocks.listSidebarSessions.mockReturnValueOnce( + new Promise((resolve) => { + answerPage = resolve; + }) + ); + // A membership nudge from anywhere schedules a head refresh. + act(() => notifySessionListChanged()); + await waitFor(() => expect(mocks.listSidebarSessions).toHaveBeenCalledTimes(2)); + + mocks.getSession.mockResolvedValue({ + data: { id: 'session-0', privacy_tier: 'private', privacy_reason: 'turn:versa_azure' }, + }); + const sibling = new BroadcastChannel('biorouter:session-row'); + try { + sibling.postMessage({ sessionId: 'session-0' }); + await waitFor(() => expect(result.current.sessions[0].privacy_tier).toBe('private')); + } finally { + sibling.close(); + } + expect(mocks.getSession).toHaveBeenCalledTimes(1); + + // The read that settles the disagreement is held open, so the row's state + // in the meantime is observable. + let answerThirdRead: ((value: unknown) => void) | undefined; + mocks.getSession.mockReturnValueOnce( + new Promise((resolve) => { + answerThirdRead = resolve; + }) + ); + await act(async () => { + answerPage!({ data: { sessions: [publicRow], has_more: false, next_cursor: null } }); + }); + + await waitFor(() => expect(mocks.getSession).toHaveBeenCalledTimes(2)); + expect(result.current.sessions[0].privacy_tier).toBe('private'); + await act(async () => { + answerThirdRead!({ + data: { id: 'session-0', privacy_tier: 'private', privacy_reason: 'turn:versa_azure' }, + }); + }); + expect(result.current.sessions[0].privacy_tier).toBe('private'); + }); }); diff --git a/ui/desktop/src/components/BioRouterSidebar/useSidebarSessions.ts b/ui/desktop/src/components/BioRouterSidebar/useSidebarSessions.ts index a7acc176d..fb796640a 100644 --- a/ui/desktop/src/components/BioRouterSidebar/useSidebarSessions.ts +++ b/ui/desktop/src/components/BioRouterSidebar/useSidebarSessions.ts @@ -3,7 +3,11 @@ import { listSidebarSessions, type SessionSummary } from '../../api'; import { userActionHeaders } from '../../utils/userAction'; import { subscribeSessionNameChanges } from '../../utils/sessionNameSync'; import { subscribeSessionListChanges, subscribeSessionRemoved } from '../../utils/sessionListCache'; -import { subscribeSessionRowChanges } from '../../utils/sessionRowSync'; +import { + settleRowsReadDuringFetch, + subscribeSessionRowChanges, + type SessionRowFacts, +} from '../../utils/sessionRowSync'; export const SIDEBAR_SESSION_PAGE_SIZE = 10; @@ -43,12 +47,18 @@ export default function useSidebarSessions(): SidebarSessionsState { const hasMoreRef = useRef(true); const hasLoadedRef = useRef(false); const loadingRef = useRef(false); + // Row reads delivered while a page request was in flight, settled against + // that page's answer. See `settleRowsReadDuringFetch`: without it, a head + // refresh issued before a turn raised a chat and answered after the raise was + // patched in would draw the chat public again. + const rowsReadDuringLoadRef = useRef(new Map()); const loadPage = useCallback(async (reset: boolean) => { if (loadingRef.current || (!reset && !hasMoreRef.current)) return; const cursor = reset ? null : nextCursorRef.current; loadingRef.current = true; + rowsReadDuringLoadRef.current.clear(); setIsLoading(true); try { @@ -60,7 +70,10 @@ export default function useSidebarSessions(): SidebarSessionsState { throwOnError: true, }); const page = response.data; - const mergedSessions = appendSessionPage(sessionsRef.current, page.sessions); + const mergedSessions = appendSessionPage( + sessionsRef.current, + settleRowsReadDuringFetch(page.sessions, rowsReadDuringLoadRef.current, false) + ); const pageHasMore = reset && hasLoadedRef.current ? hasMoreRef.current || page.has_more : page.has_more; @@ -136,15 +149,17 @@ export default function useSidebarSessions(): SidebarSessionsState { setSessions(remaining); }); - // A chat's classification changed in place — a declassification, here or - // in another window (`sessionRowSync`). Patched by id, for the same reason - // the removal above is: `loadPage(true)` re-reads only the HEAD of the - // keyset, and a declassified chat is usually months old and nowhere near - // it. Until 2026-09-13 the daemon stamped `updated_at` on a declassification, - // so the next head refresh happened to carry the chat (moved to the top, - // which was the bug); with the chat left where it belongs, only this - // reaches its row. - const unsubscribeRows = subscribeSessionRowChanges(({ sessionId, privacy_tier }) => { + // A chat's classification changed in place — a declassification, or a raise + // a chat store announced, here or in another window (`sessionRowSync`). + // Patched by id, for the same reason the removal above is: `loadPage(true)` + // re-reads only the HEAD of the keyset, and a declassified chat is usually + // months old and nowhere near it. Until 2026-09-13 the daemon stamped + // `updated_at` on a declassification, so the next head refresh happened to + // carry the chat (moved to the top, which was the bug); with the chat left + // where it belongs, only this reaches its row. + const unsubscribeRows = subscribeSessionRowChanges((facts) => { + const { sessionId, privacy_tier } = facts; + if (loadingRef.current) rowsReadDuringLoadRef.current.set(sessionId, facts); let changed = false; const next = sessionsRef.current.map((session) => { if (session.id !== sessionId || session.privacy_tier === privacy_tier) return session; diff --git a/ui/desktop/src/components/chatGroups/ChatGroupsShell.tsx b/ui/desktop/src/components/chatGroups/ChatGroupsShell.tsx index c27562e8a..fddc1964f 100644 --- a/ui/desktop/src/components/chatGroups/ChatGroupsShell.tsx +++ b/ui/desktop/src/components/chatGroups/ChatGroupsShell.tsx @@ -107,16 +107,22 @@ function renderLayout( * * # The merge is `max`, not "freshest wins" * - * The tier is a permanent ratchet server-side - * (`crates/biorouter/src/privacy/mod.rs`) — public → private, never back — so a - * `private` from ANY source is a fact that still holds, and a `public` is only - * a lower bound. {@link mergeSessionTiers} folds the two with `max` and - * `undefined` stays unmarked. The invariant, which + * The tier is a ratchet server-side (`crates/biorouter/src/privacy/mod.rs`) + * with one exit, the user's declassification — so a `public` is only a lower + * bound, and a `private` holds until a declassification that BOTH sources are + * told about (`sessionRowSync`, the change feed). {@link mergeSessionTiers} + * folds the two with `max` and `undefined` stays unmarked. The invariant, which * `ChatGroupsShell.privacy.test.tsx` pins: this map may render private-from- - * either-source or unmarked, and can never render public over a source that has - * seen private. There is no failure mode in which it over-marks — no source - * here invents a tier, they only report a row. `ChatTabStrip`'s `privacyTiers` - * prop doc states the same thing, and the two must not drift apart again. + * either-source or unmarked, and can never render public over a source that + * still holds private. There is no failure mode in which it over-marks — no + * source here invents a tier, they only report a row. `ChatTabStrip`'s + * `privacyTiers` prop doc states the same thing, and the two must not drift + * apart again. + * + * ⚠ `max` across sources is only right because each source follows its OWN + * row down. The live map once refused to (it "mirrored the ratchet"), so a + * declassified chat's tab stayed private for as long as it was open — defect D1 + * of 2026-09-13; see `ChatStreamRegistry.subscribeSessionTiers`. * * # The cache still has to be warmed here * diff --git a/ui/desktop/src/components/privacy/sessionTier.ts b/ui/desktop/src/components/privacy/sessionTier.ts index 557e6adb1..68832c579 100644 --- a/ui/desktop/src/components/privacy/sessionTier.ts +++ b/ui/desktop/src/components/privacy/sessionTier.ts @@ -8,18 +8,28 @@ import type { SessionClassification } from '../../api/types.gen'; * `SessionClassification` is a two-element lattice — `public < private` — and * the daemon reduces it with `max` over the life of a session * (`crates/biorouter/src/privacy/mod.rs`; CLAUDE.md calls it "a permanent - * ratchet"). It only ever rises. That single fact decides every question a - * caller could otherwise get wrong: + * ratchet"). It rises on its own and falls only when the USER declassifies the + * chat (§12.4, `privacy::declassify`). So: * - * - A reading of `private` can never become false. Whoever saw it saw a fact - * about the row that still holds, however old the reading is. * - A reading of `public` can become false at any moment, and says nothing * about now. It is a lower bound, not an answer. + * - A reading of `private` can become false only through a declassification, + * which every source here is told about (`sessionRowSync`, the change feed). * - * So two readings of the same chat are combined with `max`, never with - * "whichever is fresher". Freshness is not the ordering that matters here, and - * a merge that preferred the newer source would let a source which has not yet - * heard about a ratchet overwrite one that has. + * ⚠ **That second line used to say "can never become false", and the tab strip + * was built on it.** The live map in `ChatStreamRegistry` then refused to + * follow its own store down, so a chat declassified with its tab open kept a + * private tab icon until the renderer reloaded (defect D1, 2026-09-13). The + * rule that survives is narrower, and it is two rules: + * + * - WITHIN one source, the newest reading wins. Each source is re-read when the + * row moves, in either direction, and holds what it last read. + * - ACROSS sources, `max` — these functions. Two sources disagree only while + * one of them has not yet been re-read, and while that is so the chat is + * shown private. A merge that preferred "the fresher source" would let one + * that has not yet heard about a raise overwrite one that has, and that is + * the direction a badge must never be wrong in; the price of `max` is a + * lowering that shows up once the slower source has been re-read too. * * # The direction a mistake must fall in * diff --git a/ui/desktop/src/components/sessions/DeclassifySessionDialog.test.tsx b/ui/desktop/src/components/sessions/DeclassifySessionDialog.test.tsx index 6fc3d62b9..da94c28c7 100644 --- a/ui/desktop/src/components/sessions/DeclassifySessionDialog.test.tsx +++ b/ui/desktop/src/components/sessions/DeclassifySessionDialog.test.tsx @@ -6,9 +6,16 @@ import type { Session } from '../../api'; const mocks = vi.hoisted(() => ({ declassifySession: vi.fn(), - toastError: vi.fn(), + // Returns the id the real `toastError` returns: its dedupe key, so two + // identical failures share one id exactly as they share one toast. + toastError: vi.fn(({ title, msg }: { title: string; msg: string }) => `error:${title}:${msg}`), toastSuccess: vi.fn(), + dismissToast: vi.fn(), announceSessionRowChanged: vi.fn(), + readSessionRowFacts: vi.fn(), + // Row reads delivered in this window, for the listener the dialog module + // installs; `deliverRow` below plays one. + rowListeners: new Set<(facts: { sessionId: string; privacy_tier: string }) => void>(), })); vi.mock('../../api', () => ({ @@ -18,6 +25,7 @@ vi.mock('../../api', () => ({ vi.mock('../../toasts', () => ({ toastError: mocks.toastError, toastSuccess: mocks.toastSuccess, + toastService: { dismiss: mocks.dismissToast }, })); vi.mock('../../utils/userAction', () => ({ @@ -26,8 +34,32 @@ vi.mock('../../utils/userAction', () => ({ vi.mock('../../utils/sessionRowSync', () => ({ announceSessionRowChanged: mocks.announceSessionRowChanged, + readSessionRowFacts: mocks.readSessionRowFacts, + subscribeSessionRowChanges: ( + listener: (facts: { sessionId: string; privacy_tier: string }) => void + ) => { + mocks.rowListeners.add(listener); + return () => mocks.rowListeners.delete(listener); + }, })); +function deliverRow(sessionId: string, privacy_tier: 'public' | 'private') { + for (const listener of [...mocks.rowListeners]) listener({ sessionId, privacy_tier }); +} + +/** The row as `readSessionRowFacts` would hand it back after a failure. */ +function rowReads(privacy_tier: 'public' | 'private' | null) { + mocks.readSessionRowFacts.mockResolvedValue( + privacy_tier === null + ? null + : { + sessionId: s.id, + privacy_tier, + privacy_reason: privacy_tier === 'public' ? 'declassified_by_user' : 'turn:versa_azure', + } + ); +} + /** * Answer the way the generated client (`api/client/client.gen.ts`) really does, * for BOTH ways of calling it — which is what lets a test here fail on the code @@ -70,6 +102,9 @@ const s = { beforeEach(() => { vi.clearAllMocks(); mocks.declassifySession.mockImplementation(answerLikeTheClient(200, '')); + // Every failure re-reads the row before it says anything. Unless a test says + // otherwise, the write did not land. + rowReads('private'); }); afterEach(() => { @@ -442,8 +477,228 @@ describe('DeclassifySessionDialog', () => { await waitFor(() => expect(mocks.toastError).toHaveBeenCalled()); const { title, msg } = mocks.toastError.mock.calls[0][0] as { title: string; msg: string }; expect(msg).not.toContain('[object Object]'); - expect(msg).toMatch(/not marked public/); + // Stated as what the row read back, which is all this dialog knows. + expect(msg).toMatch(/still private/); expect(title).toBe('Could not mark this chat public'); expect(screen.queryByRole('textbox')).toBeNull(); }); }); + +/** + * Defect D2 of the 2026-09-13 repair round. The daemon wrote, and its answer + * never reached the renderer: measured by failing the POST's RESPONSE in the + * running app (CDP `Fetch.failRequest` at the response stage) with the daemon's + * 200 already sent. The database read `public` with one ledger row; the toast + * read "Biorouter could not be reached, so this chat was not marked public.", + * the dialog stayed open, and both windows' rows stayed private. A missing + * answer is not a "no", so the row is asked before a word is said. + */ +describe('an answer that never arrived', () => { + const lostAnswer = () => + mocks.declassifySession.mockRejectedValue(new TypeError('Failed to fetch')); + + it('is a success when the row reads public — the write landed', async () => { + const user = userEvent.setup(); + const onDeclassified = vi.fn(); + const onClose = vi.fn(); + lostAnswer(); + rowReads('public'); + + render( + + ); + await user.type(screen.getByLabelText(/last 6 characters/i), 'def456'); + await user.click(screen.getByRole('button', { name: /Make public/ })); + + await waitFor(() => expect(onDeclassified).toHaveBeenCalledWith('abc123def456')); + expect(mocks.readSessionRowFacts).toHaveBeenCalledWith('abc123def456'); + expect(mocks.toastSuccess).toHaveBeenCalled(); + expect(mocks.toastError).not.toHaveBeenCalled(); + // Every window's rows re-read it, as after any success. + expect(mocks.announceSessionRowChanged).toHaveBeenCalledWith('abc123def456'); + expect(onClose).toHaveBeenCalled(); + }); + + it('says the chat is still private when the row says so, and not more', async () => { + const onDeclassified = vi.fn(); + lostAnswer(); + rowReads('private'); + + render( + + ); + fireEvent.click(screen.getByRole('button', { name: /Make public/ })); + + await waitFor(() => expect(mocks.toastError).toHaveBeenCalled()); + const { msg } = mocks.toastError.mock.calls[0][0] as { msg: string }; + expect(msg).toMatch(/still private/); + expect(onDeclassified).not.toHaveBeenCalled(); + expect(mocks.announceSessionRowChanged).not.toHaveBeenCalled(); + // A lost answer says nothing about the grade. + expect(screen.queryByRole('textbox')).toBeNull(); + }); + + it('claims neither outcome when the row cannot be read either', async () => { + const onDeclassified = vi.fn(); + lostAnswer(); + rowReads(null); + + render( + + ); + fireEvent.click(screen.getByRole('button', { name: /Make public/ })); + + await waitFor(() => expect(mocks.toastError).toHaveBeenCalled()); + const { msg } = mocks.toastError.mock.calls[0][0] as { msg: string }; + expect(msg).toMatch(/could not be asked whether this chat is now public/); + expect(msg).not.toMatch(/not marked public/); + expect(msg).not.toMatch(/still private/); + expect(onDeclassified).not.toHaveBeenCalled(); + expect(mocks.toastSuccess).not.toHaveBeenCalled(); + }); + + it('keeps the daemon’s own sentence when it gave one and the row is unreadable', async () => { + mocks.declassifySession.mockImplementation(answerLikeTheClient(503, STORE_BUSY)); + rowReads(null); + + render( + + ); + fireEvent.click(screen.getByRole('button', { name: /Make public/ })); + + await waitFor(() => + expect(mocks.toastError).toHaveBeenCalledWith({ + title: 'The chat store was busy', + msg: STORE_BUSY, + }) + ); + }); +}); + +/** + * Defect D3a of the 2026-09-13 repair round. Error toasts do not expire, so a + * failure that a later attempt overturned stayed beside "Chat marked public": + * measured still on screen 60 s after the success toast had closed — after two + * busy answers, after a lost answer, and after the escalation's refusal. + */ +describe('a failure report is retracted by the next outcome', () => { + it('a later success dismisses the failure it overturned', async () => { + mocks.declassifySession + .mockImplementationOnce(answerLikeTheClient(503, STORE_BUSY)) + .mockImplementationOnce(answerLikeTheClient(200, '')); + const onDeclassified = vi.fn(); + + render( + + ); + fireEvent.click(screen.getByRole('button', { name: /Make public/ })); + await waitFor(() => expect(mocks.toastError).toHaveBeenCalledTimes(1)); + const failureId = mocks.toastError.mock.results[0].value; + expect(mocks.dismissToast).not.toHaveBeenCalled(); + + await waitFor(() => expect(screen.getByRole('button', { name: /Make public/ })).toBeEnabled()); + fireEvent.click(screen.getByRole('button', { name: /Make public/ })); + + await waitFor(() => expect(onDeclassified).toHaveBeenCalled()); + expect(mocks.dismissToast).toHaveBeenCalledWith(failureId); + expect(mocks.toastSuccess).toHaveBeenCalled(); + }); + + it('reaches a failure raised before the dialog was closed and reopened', async () => { + mocks.declassifySession + .mockImplementationOnce(answerLikeTheClient(503, STORE_BUSY)) + .mockImplementationOnce(answerLikeTheClient(200, '')); + const session = { ...s, id: 'reopened0001', privacy_reason: 'turn:versa_azure' }; + + const first = render( + + ); + fireEvent.click(screen.getByRole('button', { name: /Make public/ })); + await waitFor(() => expect(mocks.toastError).toHaveBeenCalledTimes(1)); + const failureId = mocks.toastError.mock.results[0].value; + // Both entry points unmount the dialog when it closes. + first.unmount(); + + const onDeclassified = vi.fn(); + render( + + ); + fireEvent.click(screen.getByRole('button', { name: /Make public/ })); + await waitFor(() => expect(onDeclassified).toHaveBeenCalled()); + expect(mocks.dismissToast).toHaveBeenCalledWith(failureId); + }); + + it('a declassification made elsewhere retracts the failure too', async () => { + // Fail here, close the dialog, and mark the chat public from another + // window: this window hears it as a row read, and its "still private" toast + // is then a report about a chat that is not. + mocks.declassifySession.mockImplementation(answerLikeTheClient(503, STORE_BUSY)); + const session = { ...s, id: 'elsewhere001', privacy_reason: 'turn:versa_azure' }; + const view = render(); + fireEvent.click(screen.getByRole('button', { name: /Make public/ })); + await waitFor(() => expect(mocks.toastError).toHaveBeenCalledTimes(1)); + const failureId = mocks.toastError.mock.results[0].value; + view.unmount(); + + deliverRow('elsewhere001', 'private'); + expect(mocks.dismissToast).not.toHaveBeenCalled(); + + deliverRow('elsewhere001', 'public'); + expect(mocks.dismissToast).toHaveBeenCalledWith(failureId); + }); + + it('a different failure replaces the earlier one; the same failure keeps its toast', async () => { + const refusal = + "The confirmation did not match the last six characters of this chat's id. Nothing was changed."; + mocks.declassifySession + .mockImplementationOnce(answerLikeTheClient(503, STORE_BUSY)) + .mockImplementationOnce(answerLikeTheClient(503, STORE_BUSY)) + .mockImplementationOnce(answerLikeTheClient(400, refusal)); + const session = { ...s, id: 'replaced0001', privacy_reason: 'turn:versa_azure' }; + + render(); + const press = async (times: number) => { + await waitFor(() => + expect(screen.getByRole('button', { name: /Make public/ })).toBeEnabled() + ); + fireEvent.click(screen.getByRole('button', { name: /Make public/ })); + await waitFor(() => expect(mocks.toastError).toHaveBeenCalledTimes(times)); + }; + + await press(1); + const busyId = mocks.toastError.mock.results[0].value; + await press(2); + // Deduplicated onto the same toast: dismissing it would take away the + // report of the attempt just made. + expect(mocks.dismissToast).not.toHaveBeenCalled(); + + await press(3); + expect(mocks.dismissToast).toHaveBeenCalledWith(busyId); + }); +}); diff --git a/ui/desktop/src/components/sessions/DeclassifySessionDialog.tsx b/ui/desktop/src/components/sessions/DeclassifySessionDialog.tsx index daa02e704..fa6c29ef9 100644 --- a/ui/desktop/src/components/sessions/DeclassifySessionDialog.tsx +++ b/ui/desktop/src/components/sessions/DeclassifySessionDialog.tsx @@ -1,8 +1,12 @@ import { useCallback, useEffect, useLayoutEffect, useRef, useState } from 'react'; import { declassifySession, type Session } from '../../api'; -import { toastError, toastSuccess } from '../../toasts'; +import { toastError, toastService, toastSuccess } from '../../toasts'; import { userActionHeaders } from '../../utils/userAction'; -import { announceSessionRowChanged } from '../../utils/sessionRowSync'; +import { + announceSessionRowChanged, + readSessionRowFacts, + subscribeSessionRowChanges, +} from '../../utils/sessionRowSync'; import { DangerousConfirmDialog } from '../ui/DangerousConfirmDialog'; import { Dialog, @@ -96,14 +100,30 @@ export function confirmationPhrase(sessionId: string): string { * one click. * * So the status is read off the Response, never inferred from the body, and a - * body that is not a sentence gets one written here — still claiming only what - * a failed request guarantees. No Response at all (`status` undefined) means - * the request never reached the daemon; a raw `TypeError: Failed to fetch` is - * not something a person can act on, so it is not shown either. + * body that is not a sentence gets one written here. A 503 is the daemon saying + * the chat store stayed write-locked by other work past its wait: the one + * failure a retry clears, titled so it cannot be read as a refusal. + * + * # A missing answer is not a "no" — defect D2 (2026-09-13) + * + * ⚠ This used to say "no Response means the request never reached the daemon" + * and tell the person **"this chat was not marked public"**. A Response can be + * lost AFTER the daemon wrote: measured by failing the POST's response in the + * renderer with the daemon's 200 already sent — the database read `public` + * with one ledger row while the toast said the chat was not marked public, the + * dialog stayed open, and both windows' rows stayed private. * - * A 503 is the daemon saying the chat store stayed write-locked by other work - * past its wait: the one failure a retry clears, titled so it cannot be read - * as a refusal. + * So nothing here reports an outcome the dialog did not read. After any answer + * that is not a 200 the row is read again (`readSessionRowFacts`), and: + * + * - a row that reads PUBLIC is a success, however it got there (the lost + * answer, or another window's declassification), and is never described + * here — `send` takes the success path instead; + * - a row that reads PRIVATE is the failure, stated as the chat's state + * ("still private"), with the daemon's own sentence when it gave one; + * - a row that cannot be read leaves only the daemon's sentence, if there was + * one — the daemon said nothing changed, and it is the one that knows. With + * neither, the dialog says it could not find out, and claims nothing. */ export interface DeclassifyFailure { status: number | undefined; @@ -113,16 +133,29 @@ export interface DeclassifyFailure { export function describeDeclassifyFailure( status: number | undefined, - body: unknown + body: unknown, + /** The row as read AFTER the failure; `null` when it could not be read. */ + rowTier: 'public' | 'private' | null ): DeclassifyFailure { const sentence = typeof body === 'string' && body.trim().length > 0 ? body.trim() : undefined; let message: string; - if (status === undefined) { - message = 'Biorouter could not be reached, so this chat was not marked public.'; + if (sentence !== undefined) { + message = sentence; + } else if (rowTier === 'private') { + message = + status === undefined + ? 'No answer came back from Biorouter, and this chat is still private. Try again.' + : `Biorouter answered ${status} without saying why, and this chat is still private.`; } else { + // ⚠ Kept short on purpose: the toast clamps its message to three lines and + // cuts a clause past them silently. Both are shorter than a 142-character + // version of the first, which was measured filling all three lines. message = - sentence ?? - `Biorouter answered ${status} without saying why, and this chat was not marked public.`; + status === undefined + ? 'No answer came back from Biorouter, and it could not be asked whether this chat is ' + + 'now public. Reopen chat history to check.' + : `Biorouter answered ${status} without a reason, and could not be asked whether this ` + + 'chat is now public. Reopen chat history to check.'; } return { status, @@ -131,6 +164,48 @@ export function describeDeclassifyFailure( }; } +/** + * The failure toast each chat's dialog last raised, by session id — defect D3a + * (2026-09-13). + * + * Error toasts do not expire (`toasts.tsx`: "a failure that expires unread is a + * failure that was never reported"), so a failure a later attempt overturned + * stayed on screen beside "Chat marked public" — measured still there 60 s + * after the success toast had closed, after two busy answers, after a lost + * answer, and after the escalation's refusal. A report about one attempt is + * retracted by the next outcome of the same operation. + * + * Module-level, not component state, because the dialog is unmounted on close: + * fail, close, reopen and succeed must retract the first failure too. And the + * outcome that overturns a failure need not come from this dialog at all — the + * same chat declassified from another window, or from the CLI while a store + * here holds it, reaches this window as a row read (`sessionRowSync`), and a + * toast still saying "still private" beside a public chat is the same stale + * report. + */ +const outstandingFailureToasts = new Map(); +let stopFollowingRows: (() => void) | null = null; + +/** + * Started the first time a failure is reported, and kept for the renderer's + * life: a toast that outlives its dialog is exactly the case it is for. + */ +function followRowsForOutstandingFailures(): void { + if (stopFollowingRows) return; + stopFollowingRows = subscribeSessionRowChanges(({ sessionId, privacy_tier }) => { + if (privacy_tier === 'public') retractFailureToast(sessionId); + }); +} + +function retractFailureToast(sessionId: string, keep?: string | number): void { + const previous = outstandingFailureToasts.get(sessionId); + if (previous === undefined) return; + outstandingFailureToasts.delete(sessionId); + // An identical failure is deduplicated onto the SAME toast id, so dismissing + // it would take away the toast that is reporting the attempt just made. + if (previous !== keep) toastService.dismiss(previous); +} + type Phase = 'confirm' | 'undo' | 'sending'; export interface DeclassifySessionDialogProps { @@ -211,7 +286,8 @@ export function DeclassifySessionDialog({ const send = useCallback( async (confirmation: string | null) => { setPhase('sending'); - let failure: DeclassifyFailure | null = null; + let status: number | undefined; + let body: unknown; try { // NOT `throwOnError`: the generated client then throws the parsed BODY // and drops the Response, and the status is what separates a refusal @@ -224,18 +300,29 @@ export function DeclassifySessionDialog({ // agent shell (§9.3 A1) and is not evidence of a human. headers: await userActionHeaders(), }); - // Success is a 200 and nothing else. A missing Response (the fetch - // failed) or any other status is a failure, whatever `data` holds — a - // private chat must never be reported public when the write did not - // land. - if (result.response?.status !== 200) { - failure = describeDeclassifyFailure(result.response?.status, result.error); - } + status = result.response?.status; + body = result.error; } catch (error) { - failure = describeDeclassifyFailure(undefined, error); + // A throw here carries no Response, so no status: the answer — if the + // daemon sent one — did not arrive. + status = undefined; + body = error; + } + + // Success is a 200, or a row that READS public after anything else. A + // missing Response is not evidence the write did not land (D2), so the + // row is asked before a word is said; and a private chat is never + // reported public unless the row says so. + let failure: DeclassifyFailure | null = null; + if (status !== 200) { + const row = await readSessionRowFacts(session.id); + if (row?.privacy_tier !== 'public') { + failure = describeDeclassifyFailure(status, body, row?.privacy_tier ?? null); + } } if (failure === null) { + retractFailureToast(session.id); toastSuccess({ title: 'Chat marked public', msg: 'It no longer carries a private marker. The change is recorded.', @@ -248,7 +335,12 @@ export function DeclassifySessionDialog({ return; } - toastError({ title: failure.title, msg: failure.message }); + const toastId = toastError({ title: failure.title, msg: failure.message }); + retractFailureToast(session.id, toastId); + if (toastId !== undefined) { + outstandingFailureToasts.set(session.id, toastId); + followRowsForOutstandingFailures(); + } // A request that carried no confirmation was answered "the confirmation // did not match" (400), so the weak control this dialog rendered was the // wrong one — most likely because the cached row's `turn:*` provenance diff --git a/ui/desktop/src/components/ui/dialog.test.tsx b/ui/desktop/src/components/ui/dialog.test.tsx index 5a6b8598f..a757aacd2 100644 --- a/ui/desktop/src/components/ui/dialog.test.tsx +++ b/ui/desktop/src/components/ui/dialog.test.tsx @@ -57,6 +57,44 @@ describe('DialogContent dismissal contract', () => { expect(onClose).toHaveBeenCalledTimes(1); }); + /** + * Defect D3b (2026-09-13). Toasts sit above every modal and are rendered + * outside any dialog's tree, so a press on one reached Radix as a press + * OUTSIDE the dialog: measured in the running app, one click on an error + * toast's × closed "Make this chat public?" underneath while the toast stayed + * on screen a minute later. The toast layer's own markup is reproduced here — + * `section.Toastify` > `.Toastify__toast-container` > the card — because the + * container is the hook the guard keys on. + */ + it('does not treat a press on a toast as a press on the backdrop', async () => { + const user = userEvent.setup(); + const onClose = vi.fn(); + render(); + await user.click(screen.getByRole('button', { name: 'Open dialog' })); + + const layer = document.createElement('section'); + layer.className = 'Toastify'; + layer.innerHTML = + '
' + + '
' + + '
'; + document.body.appendChild(layer); + try { + fireEvent.pointerDown(layer.querySelector('.Toastify__close-button')!); + await new Promise((resolve) => setTimeout(resolve, 20)); + expect(screen.getByRole('dialog')).toBeInTheDocument(); + expect(onClose).not.toHaveBeenCalled(); + + // …while the backdrop itself still dismisses, so the guard is not simply + // "never dismiss on a press". + fireEvent.pointerDown(document.querySelector('[data-slot="dialog-overlay"]')!); + await waitFor(() => expect(screen.queryByRole('dialog')).not.toBeInTheDocument()); + expect(onClose).toHaveBeenCalledTimes(1); + } finally { + layer.remove(); + } + }); + it('blocks Escape, backdrop, and the close button when not dismissible', async () => { const user = userEvent.setup(); const onClose = vi.fn(); diff --git a/ui/desktop/src/components/ui/dialog.tsx b/ui/desktop/src/components/ui/dialog.tsx index a75d4794b..5b2b863ba 100644 --- a/ui/desktop/src/components/ui/dialog.tsx +++ b/ui/desktop/src/components/ui/dialog.tsx @@ -39,6 +39,16 @@ const DialogOverlay = React.forwardRef< )); DialogOverlay.displayName = DialogPrimitive.Overlay.displayName; +/** + * Whether a pointer target sits in react-toastify's layer. `.Toastify` is the + * container the library renders (`App.tsx`'s `ToastContainer`); the app's own + * card class replaces the library's per-toast class, so the container is the + * one stable hook (see `toastLayer.test.ts`). + */ +function isInsideToastLayer(target: EventTarget | null): boolean { + return target instanceof Element && target.closest('.Toastify') !== null; +} + function DialogContent({ className, children, @@ -102,6 +112,16 @@ function DialogContent({ onPointerDownOutside={(event) => { onPointerDownOutside?.(event); if (!dismissible) event.preventDefault(); + // A press on a TOAST is not a press on the backdrop. Toasts sit above + // every modal (`--z-toast` > `--z-modal`) and are rendered outside + // any dialog's tree, so to Radix a click on one is "outside" — and + // it closed the dialog underneath while the toast, whose × had never + // received the click, stayed on screen. Measured on 2026-09-13 + // (defect D3b): a busy-store error over "Make this chat public?", + // one click on the toast's ×, the dialog gone and the error still + // there a minute later. `main.css` makes the toast layer take the + // click at all; this stops the dialog treating it as a dismissal. + if (isInsideToastLayer(event.target)) event.preventDefault(); }} {...props} > diff --git a/ui/desktop/src/hooks/chatStreamStore.declassify.test.tsx b/ui/desktop/src/hooks/chatStreamStore.declassify.test.tsx new file mode 100644 index 000000000..0bf8987e2 --- /dev/null +++ b/ui/desktop/src/hooks/chatStreamStore.declassify.test.tsx @@ -0,0 +1,240 @@ +/** + * A chat's classification moves in BOTH directions, and every surface that + * draws it has to follow — defects D1 and D4 of the 2026-09-13 repair round. + * + * **D1 — a declassified chat's open tab kept its private icon.** Measured in + * the running app, in one window and across two: the database, the store and + * the sidebar all read `public`, and the tab strip's icon stayed + * `data-privacy="private"` past twelve minutes, until a reload. The registry's + * live map only ever rose ("mirroring the ratchet"), so the store's lowered + * reading was thrown away and the strip's `max` held the stale one. + * + * **D4 — another window drew a private chat PUBLIC.** Declassify in window A + * (the push lowers window B's History and sidebar rows, as designed), then send + * one turn on a private model in A: the database went back to `private` / + * `turn:versa_azure`, A's sidebar followed, and B's History row and sidebar row + * stayed public for the whole watch — the sidebar over two minutes. Nothing + * pushed the raise. + */ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import type { MessageEvent, Session, TokenState } from '../api'; + +const mocks = vi.hoisted(() => ({ + reply: vi.fn(), + observeSessionEvents: vi.fn(), + resumeAgent: vi.fn(), + getSession: vi.fn(async (_options?: unknown) => ({ data: null }) as unknown), + listSessions: vi.fn(async () => ({ data: { sessions: [] } })), + updateFromSession: vi.fn(async () => ({ data: {} })), +})); + +vi.mock('../api', async (importOriginal) => { + const actual = (await importOriginal()) as Record; + return { ...actual, ...mocks }; +}); + +// The long poll is a network side effect with its own tests +// (`sessionMetaSubscription.test.ts`); here it must not be the thing that makes +// a store re-read, or a test of the row channel would pass on the poll. +vi.mock('../utils/sessionMetaSubscription', () => ({ + subscribeToSessionMeta: () => () => {}, +})); + +// The real channel, with the announcement observed. The positive case below +// also listens on the channel as another window would; the negative cases ask +// the spy, because "nothing arrived on a BroadcastChannel" has no barrier that +// Node orders against a different sender. +const announce = vi.hoisted(() => ({ spy: undefined as unknown as ReturnType })); +vi.mock('../utils/sessionRowSync', async (importOriginal) => { + const actual = (await importOriginal()) as typeof import('../utils/sessionRowSync'); + announce.spy = vi.fn(actual.announceSessionRowChanged); + return { ...actual, announceSessionRowChanged: announce.spy }; +}); + +import { ChatStreamRegistry } from './chatStreamStore'; + +const tokenState: TokenState = { + accumulatedInputTokens: 0, + accumulatedOutputTokens: 0, + accumulatedTotalTokens: 0, + inputTokens: 0, + outputTokens: 0, + totalTokens: 0, +}; +const finishFrame = { type: 'Finish', reason: 'stop', token_state: tokenState } as MessageEvent; + +async function* streamOf(...frames: MessageEvent[]) { + for (const frame of frames) yield frame; +} + +function row(id: string, over: Partial = {}): Session { + return { + id, + name: 'A named chat', + working_dir: '/tmp', + conversation: [], + message_count: 0, + total_tokens: 0, + created_at: '', + updated_at: '', + extension_data: {}, + user_set_name: true, + privacy_tier: 'private', + privacy_reason: 'turn:versa_azure', + provider_name: 'versa_azure', + model_config: { model_name: 'gpt-5.5-2026-04-24', toolshim: false, context_limit: 400000 }, + ...over, + } as Session; +} + +const publicRow = (id: string) => + row(id, { privacy_tier: 'public', privacy_reason: 'declassified_by_user' }); + +/** One animation frame: store notifications, and so the tier map, are batched to it (#22). */ +const aFrame = () => new Promise((resolve) => setTimeout(resolve, 60)); + +/** Stands in for ANOTHER window on the row channel (Node's BroadcastChannel under jsdom). */ +const ROW_CHANNEL = 'biorouter:session-row'; + +let seq = 0; +const sid = (label: string) => `declass-${label}-${++seq}`; + +describe('the tab strip follows a declassification (D1)', () => { + beforeEach(() => { + vi.clearAllMocks(); + mocks.getSession.mockResolvedValue({ data: null }); + }); + + it('lowers a tier its store re-read, instead of holding the old private', async () => { + const id = sid('lower'); + mocks.resumeAgent.mockResolvedValue({ data: { session: row(id) } }); + const registry = new ChatStreamRegistry(); + const controller = registry.getController(id); + await controller.loadSession(); + await aFrame(); + expect(registry.getSessionTiersSnapshot()).toEqual({ [id]: 'private' }); + + // What the change feed does ~2 s after a declassification: re-read the row. + mocks.getSession.mockResolvedValue({ data: publicRow(id) }); + await controller.refreshSessionBinding(); + await aFrame(); + + expect(controller.getSnapshot().session?.privacy_tier).toBe('public'); + expect(registry.getSessionTiersSnapshot()).toEqual({ [id]: 'public' }); + }); + + it('a row read in this window makes a store holding another tier re-read at once', async () => { + // The change feed watches at most 64 ids and the registry keeps every store + // it ever made, so it cannot be what every store relies on. + const id = sid('nudge'); + mocks.resumeAgent.mockResolvedValue({ data: { session: row(id) } }); + const registry = new ChatStreamRegistry(); + const stop = registry.followSessionRows(); + const otherWindow = new BroadcastChannel(ROW_CHANNEL); + try { + await registry.getController(id).loadSession(); + await aFrame(); + expect(registry.getSessionTiersSnapshot()).toEqual({ [id]: 'private' }); + + mocks.getSession.mockResolvedValue({ data: publicRow(id) }); + // Another window declassified it and announced so. + otherWindow.postMessage({ sessionId: id }); + + await vi.waitFor(async () => { + await aFrame(); + expect(registry.getSessionTiersSnapshot()).toEqual({ [id]: 'public' }); + }); + } finally { + otherWindow.close(); + stop(); + } + }); +}); + +describe('a raise reaches every window’s list rows (D4)', () => { + let otherWindow: BroadcastChannel; + let heard: string[]; + + beforeEach(() => { + vi.clearAllMocks(); + announce.spy.mockClear(); + mocks.getSession.mockResolvedValue({ data: null }); + heard = []; + otherWindow = new BroadcastChannel(ROW_CHANNEL); + otherWindow.onmessage = (event: globalThis.MessageEvent) => { + heard.push((event.data as { sessionId: string }).sessionId); + }; + }); + + afterEach(() => { + otherWindow.close(); + }); + + it('announces a raise its store sees', async () => { + const id = sid('raise'); + mocks.resumeAgent.mockResolvedValue({ data: { session: publicRow(id) } }); + mocks.getSession.mockResolvedValue({ data: row(id) }); + mocks.reply.mockResolvedValue({ + stream: streamOf( + { + type: 'PrivacyProviderPinned', + provider: 'versa_azure', + model: 'gpt-5.5-2026-04-24', + privacy_tier: 'private', + privacy_reason: 'turn:versa_azure', + } as unknown as MessageEvent, + finishFrame + ), + }); + + const registry = new ChatStreamRegistry(); + const controller = registry.getController(id); + await controller.loadSession(); + await aFrame(); + expect(registry.getSessionTiersSnapshot()).toEqual({ [id]: 'public' }); + + await controller.handleSubmit('hi'); + + await vi.waitFor(() => expect(heard).toContain(id)); + expect(announce.spy).toHaveBeenCalledWith(id); + }); + + it('announces nothing for a store’s first reading of a chat', async () => { + const id = sid('first'); + mocks.resumeAgent.mockResolvedValue({ data: { session: row(id) } }); + + const registry = new ChatStreamRegistry(); + await registry.getController(id).loadSession(); + await aFrame(); + expect(registry.getSessionTiersSnapshot()).toEqual({ [id]: 'private' }); + + await aFrame(); + expect(announce.spy).not.toHaveBeenCalledWith(id); + }); + + it('does not announce again a change this window was already handed', async () => { + // Window B, told of A's declassification: its lists read the row, and its + // own store then catches up. Announcing that would make every window read + // the row a second time for nothing. + const id = sid('echo'); + mocks.resumeAgent.mockResolvedValue({ data: { session: row(id) } }); + const registry = new ChatStreamRegistry(); + const stop = registry.followSessionRows(); + try { + await registry.getController(id).loadSession(); + await aFrame(); + + mocks.getSession.mockResolvedValue({ data: publicRow(id) }); + otherWindow.postMessage({ sessionId: id }); + await vi.waitFor(async () => { + await aFrame(); + expect(registry.getSessionTiersSnapshot()).toEqual({ [id]: 'public' }); + }); + + await aFrame(); + expect(announce.spy).not.toHaveBeenCalledWith(id); + } finally { + stop(); + } + }); +}); diff --git a/ui/desktop/src/hooks/chatStreamStore.tsx b/ui/desktop/src/hooks/chatStreamStore.tsx index c7067f119..c4e19609a 100644 --- a/ui/desktop/src/hooks/chatStreamStore.tsx +++ b/ui/desktop/src/hooks/chatStreamStore.tsx @@ -34,7 +34,11 @@ import { updateCachedSessionList, } from '../utils/sessionListCache'; import { subscribeToSessionMeta } from '../utils/sessionMetaSubscription'; -import { raiseTier } from '../components/privacy/sessionTier'; +import { + announceSessionRowChanged, + lastKnownSessionTier, + subscribeSessionRowChanges, +} from '../utils/sessionRowSync'; import { isReadOnlySubagentChat } from '../components/subagent/subagentReadOnly'; import { isBrowserSurface } from '../utils/surface'; import { @@ -4526,7 +4530,7 @@ export class ChatStreamRegistry { */ followSessionRows(): () => void { if (this.stopSessionMeta) return () => {}; - this.stopSessionMeta = subscribeToSessionMeta({ + const stopMeta = subscribeToSessionMeta({ // Only chats this renderer holds a row for can go stale, and only those // are worth a read on the daemon's side. openSessionIds: () => @@ -4537,6 +4541,27 @@ export class ChatStreamRegistry { void this.controllers.get(sessionId)?.refreshSessionBinding(); }, }); + // A row read THIS window just made (`sessionRowSync`: a declassification + // here or in another window, or a raise another store announced) that + // disagrees with the store holding the same chat. Handed over as a nudge, + // not applied: the store re-reads through `refreshSessionBinding`, the one + // path that orders overlapping reads (`bindingGeneration`). + // + // The long poll above would get there too, within about two seconds — but + // only for the ids it watches, and it watches at most 64 (`MAX_IDS` in + // `routes/session_meta.rs`) while this registry keeps every store it ever + // made. A store past that cap would otherwise keep a declassified chat's + // tab private until the renderer reloaded (defect D1, 2026-09-13). + const stopRows = subscribeSessionRowChanges(({ sessionId, privacy_tier }) => { + const controller = this.controllers.get(sessionId); + if (!controller?.hasLoadedSession()) return; + if (controller.getSnapshot().session?.privacy_tier === privacy_tier) return; + void controller.refreshSessionBinding(); + }); + this.stopSessionMeta = () => { + stopMeta(); + stopRows(); + }; return () => { this.stopSessionMeta?.(); this.stopSessionMeta = null; @@ -4596,11 +4621,27 @@ export class ChatStreamRegistry { * reading to the strip, so the three surfaces cannot disagree — and it needs * no fetch, because the answer was already in the window. * - * ⚠ **This map only ever RISES**, mirroring the daemon's own ratchet - * (`privacy::raise`). A controller whose session momentarily goes null — a - * reload, a rebind — must not retract a `private` it has already reported, or - * the strip would fall back to a cached `public` and un-mark a private chat. - * {@link raiseTier} is the whole rule. + * ⚠ **This map follows its stores DOWN as well as up.** It used to only rise + * ("mirroring the daemon's ratchet"), and the ratchet has one exit: a + * declassification (issue #56 §12.4). So a chat declassified while its tab + * was open kept a private tab icon for as long as it was watched — measured + * past twelve minutes on 2026-09-13 (defect D1), with the store, the sidebar + * and the database all reading `public` — because the store's lowered + * reading was discarded here and `mergeSessionTiers` then took `max` against + * it. Each store's newest DEFINED reading is adopted; it is the daemon's row + * as that store last read it, and every store is followed by the change feed + * and by `sessionRowSync`. + * + * ⚠ A store that holds NO row says nothing, and does not retract what it said. + * The strip must not fall back to a cached `public` because a store is between + * rows. (No production path empties a loaded store today; the rule is kept so + * one cannot silently un-mark a chat.) + * + * ⚠ **A change it sees is announced** (`sessionRowSync`), so every OTHER + * window's History row and sidebar row re-read the chat. Without that, a + * lowering pushed by the declassify dialog and a raise pushed by nobody left + * another window badging a private chat PUBLIC (defect D4). See + * {@link noteControllerTier}. * * ⚠ **O(1) per notification.** `handleControllerActivity` runs on every * snapshot notification, which during a turn is once per animation frame per @@ -4627,14 +4668,30 @@ export class ChatStreamRegistry { } private noteControllerTier(controller: ChatStreamController): void { + const sessionId = controller.sessionId; const reported = controller.getSnapshot().session?.privacy_tier ?? undefined; - const current = this.sessionTiers[controller.sessionId]; - const raised = raiseTier(current, reported); - if (raised === current) return; - this.sessionTiers = { ...this.sessionTiers }; - if (raised) this.sessionTiers[controller.sessionId] = raised; - else delete this.sessionTiers[controller.sessionId]; + const current = this.sessionTiers[sessionId]; + if (reported === undefined || reported === current) return; + this.sessionTiers = { ...this.sessionTiers, [sessionId]: reported }; for (const listener of this.tierListeners) listener(); + + // Tell every window's list surfaces — this one's included — to re-read the + // row. Only a CHANGE against something this window already believed: + // + // - `current` is this store's previous reading, so a turn that raised the + // chat, a change-feed re-read that found it lowered, a CLI declassification + // the feed carried; + // - `lastKnownSessionTier` is the last row this window's list surfaces were + // handed, so a chat opened for the first time HERE whose row no longer + // matches what History was last told about it. + // + // A store's very first reading of a chat nobody here has an opinion about + // announces nothing; that is every chat load. And a change this window's + // list surfaces have already been handed is not announced again: someone + // announced it, every window has read it, and this store is catching up. + const listed = lastKnownSessionTier(sessionId); + const believed = current ?? listed; + if (believed !== undefined && listed !== reported) announceSessionRowChanged(sessionId); } private handleControllerActivity = (controller: ChatStreamController): void => { diff --git a/ui/desktop/src/styles/main.css b/ui/desktop/src/styles/main.css index b53e36fa5..18d579f41 100644 --- a/ui/desktop/src/styles/main.css +++ b/ui/desktop/src/styles/main.css @@ -1862,6 +1862,21 @@ background-color var(--motion-fast) var(--ease-out); } +/* A toast stays clickable while a modal dialog is open. + Radix's modal sets `pointer-events: none` on for as long as the dialog + is open and gives only its own layer `auto`. The toast layer is a child of + that says nothing, so it inherited `none`: a toast drawn ABOVE the + dialog (`--z-toast` > `--z-modal`) could not be clicked, and the press fell + through to the dialog's backdrop and closed the dialog instead. Measured on + 2026-09-13 (defect D3b): an error toast over "Make this chat public?", one + click on its ×, the dialog gone and the toast still there a minute later. + On each card, not on the container, so the gaps between toasts still reach + the backdrop. `dialog.tsx` keeps a press that lands here from counting as a + press outside the dialog. */ +.Toastify__toast-container > * { + pointer-events: auto; +} + .Toastify__toast-container .Toastify__close-button:hover { opacity: 1; color: var(--text-default); diff --git a/ui/desktop/src/styles/toastLayer.test.ts b/ui/desktop/src/styles/toastLayer.test.ts index 10e52dbe2..2c1fe304c 100644 --- a/ui/desktop/src/styles/toastLayer.test.ts +++ b/ui/desktop/src/styles/toastLayer.test.ts @@ -121,3 +121,17 @@ describe('a newer notification lands directly below the older one', () => { expect(APP).toContain('toastClassName={() => TOAST_SURFACE_CLASS_NAME}'); }); }); + +/** + * Defect D3b (2026-09-13). Radix's modal sets `pointer-events: none` on + * while a dialog is open, and the toast layer inherited it: a toast drawn above + * the dialog could not be clicked, and the press fell through to the backdrop + * and closed the dialog. jsdom applies no stylesheet, so this is asserted at the + * source, like everything else here; `dialog.test.tsx` covers the half that + * decides whether a press on a toast dismisses the dialog. + */ +describe('a toast stays clickable over a modal dialog', () => { + it('gives every toast card pointer events back', () => { + expect(CSS).toMatch(/\.Toastify__toast-container\s*>\s*\*\s*\{[^}]*pointer-events:\s*auto\s*;/); + }); +}); diff --git a/ui/desktop/src/utils/sessionListCache.test.ts b/ui/desktop/src/utils/sessionListCache.test.ts index b1b43a01e..b143a7fca 100644 --- a/ui/desktop/src/utils/sessionListCache.test.ts +++ b/ui/desktop/src/utils/sessionListCache.test.ts @@ -283,4 +283,112 @@ describe('sessionListCache', () => { ); expect(mocks.listSessions).not.toHaveBeenCalled(); }); + + /** + * Defect D4 of the 2026-09-13 repair round, the list half. A list request + * issued BEFORE a turn raised a chat can answer AFTER the raise was read and + * patched in; adopting the answer drew the chat public again. Neither reading + * is known to be the later one, so the row shows the higher tier and is read + * a third time. + */ + it('a list answer that raced a raise does not draw the chat public again', async () => { + mocks.listSessions.mockResolvedValueOnce({ + data: { sessions: [{ id: 'raced', privacy_tier: 'public', privacy_reason: null }] }, + }); + await refreshSessionList(); + + let answerList: ((value: unknown) => void) | undefined; + mocks.listSessions.mockReturnValueOnce( + new Promise((resolve) => { + answerList = resolve; + }) + ); + const refresh = refreshSessionList(); + await vi.waitFor(() => expect(mocks.listSessions).toHaveBeenCalledTimes(2)); + + // The raise, announced by the chat's store and read while the list is out. + mocks.getSession.mockResolvedValue({ + data: { id: 'raced', privacy_tier: 'private', privacy_reason: 'turn:versa_azure' }, + }); + announceSessionRowChanged('raced'); + await vi.waitFor(() => + expect(getCachedSessionList()?.[0]).toMatchObject({ privacy_tier: 'private' }) + ); + expect(mocks.getSession).toHaveBeenCalledTimes(1); + + // The list answers with what it saw before the raise. The read that + // settles it is held open, so what the cache shows in the meantime is + // observable rather than overwritten a microtask later. + let answerThirdRead: ((value: unknown) => void) | undefined; + mocks.getSession.mockReturnValueOnce( + new Promise((resolve) => { + answerThirdRead = resolve; + }) + ); + answerList!({ + data: { sessions: [{ id: 'raced', privacy_tier: 'public', privacy_reason: null }] }, + }); + await refresh; + + // The disagreement is settled by a read issued after both… + await vi.waitFor(() => expect(mocks.getSession).toHaveBeenCalledTimes(2)); + // …and until it lands the chat is not drawn public. + expect(getCachedSessionList()?.[0]).toMatchObject({ + privacy_tier: 'private', + privacy_reason: 'turn:versa_azure', + }); + answerThirdRead!({ + data: { id: 'raced', privacy_tier: 'private', privacy_reason: 'turn:versa_azure' }, + }); + await new Promise((resolve) => setTimeout(resolve, 20)); + expect(getCachedSessionList()?.[0]).toMatchObject({ privacy_tier: 'private' }); + }); + + it('a list answer that raced a declassification is settled by a third read', async () => { + mocks.listSessions.mockResolvedValueOnce({ + data: { sessions: [{ id: 'lowered', privacy_tier: 'private', privacy_reason: 'turn:x' }] }, + }); + await refreshSessionList(); + + let answerList: ((value: unknown) => void) | undefined; + mocks.listSessions.mockReturnValueOnce( + new Promise((resolve) => { + answerList = resolve; + }) + ); + const refresh = refreshSessionList(); + await vi.waitFor(() => expect(mocks.listSessions).toHaveBeenCalledTimes(2)); + + mocks.getSession.mockResolvedValue({ + data: { id: 'lowered', privacy_tier: 'public', privacy_reason: 'declassified_by_user' }, + }); + announceSessionRowChanged('lowered'); + await vi.waitFor(() => + expect(getCachedSessionList()?.[0]).toMatchObject({ privacy_tier: 'public' }) + ); + + let answerThirdRead: ((value: unknown) => void) | undefined; + mocks.getSession.mockReturnValueOnce( + new Promise((resolve) => { + answerThirdRead = resolve; + }) + ); + answerList!({ + data: { sessions: [{ id: 'lowered', privacy_tier: 'private', privacy_reason: 'turn:x' }] }, + }); + await refresh; + // Private until the order is known — never public on a guess… + await vi.waitFor(() => expect(mocks.getSession).toHaveBeenCalledTimes(2)); + expect(getCachedSessionList()?.[0]).toMatchObject({ privacy_tier: 'private' }); + // …and public once a read issued after both says so. + answerThirdRead!({ + data: { id: 'lowered', privacy_tier: 'public', privacy_reason: 'declassified_by_user' }, + }); + await vi.waitFor(() => + expect(getCachedSessionList()?.[0]).toMatchObject({ + privacy_tier: 'public', + privacy_reason: 'declassified_by_user', + }) + ); + }); }); diff --git a/ui/desktop/src/utils/sessionListCache.ts b/ui/desktop/src/utils/sessionListCache.ts index 71719729a..c65f48311 100644 --- a/ui/desktop/src/utils/sessionListCache.ts +++ b/ui/desktop/src/utils/sessionListCache.ts @@ -1,7 +1,11 @@ import { listSessions, type Session } from '../api'; import { userActionHeaders } from './userAction'; import { subscribeSessionNameChanges } from './sessionNameSync'; -import { subscribeSessionRowChanges } from './sessionRowSync'; +import { + settleRowsReadDuringFetch, + subscribeSessionRowChanges, + type SessionRowFacts, +} from './sessionRowSync'; let cachedSessions: Session[] | null = null; let inFlightRequest: Promise | null = null; @@ -59,20 +63,28 @@ subscribeSessionNameChanges(({ sessionId, name, userSetName }) => { emitChange(); }); -// A row whose classification changed in place — a declassification, in this -// window or another (`sessionRowSync`). History's badge, its row menu ("Make -// this chat public" is offered on private rows only), Home recents and the tab -// strip's cached tiers all read this cache, so the entry is patched where it -// sits. Nothing is re-sorted: the daemon no longer moves `updated_at` for a -// classification change, so the row's place in the list is still right. -// -// ⚠ Unlike the name channel above, a fact that lands while a list request is in -// flight is NOT re-applied over that request's answer. The row read may have -// been ISSUED before the list was, so replaying it could write an older -// `public` over a newer `private` — the wrong direction for a privacy badge. A -// list that snaps a row back to private is the safe miss, and the next -// announcement or refresh corrects it. -subscribeSessionRowChanges(({ sessionId, privacy_tier, privacy_reason }) => { +/** + * Row reads (`sessionRowSync`) delivered WHILE a list request was in flight, + * settled against that request's answer by `settleRowsReadDuringFetch`. + * + * ⚠ This used to be "not re-applied — a list that snaps a row back to private + * is the safe miss". The snap-back runs in BOTH directions, and the other one + * is not safe: a list issued before a turn raised a chat, landing after the + * raise was patched in, draws the chat public again. Recorded and cleared + * exactly as {@link namesPublishedDuringFetch} is, so it cannot grow. + */ +const rowsReadDuringFetch = new Map(); + +// A row whose classification changed in place — a declassification, or a raise +// a chat store announced, in this window or another (`sessionRowSync`). +// History's badge, its row menu ("Make this chat public" is offered on private +// rows only), Home recents and the tab strip's cached tiers all read this cache, +// so the entry is patched where it sits. Nothing is re-sorted: the daemon no +// longer moves `updated_at` for a classification change, so the row's place in +// the list is still right. +subscribeSessionRowChanges((facts) => { + const { sessionId, privacy_tier, privacy_reason } = facts; + if (inFlightRequest) rowsReadDuringFetch.set(sessionId, facts); if (!cachedSessions) return; const idx = cachedSessions.findIndex((s) => s.id === sessionId); if (idx === -1) return; @@ -235,6 +247,7 @@ export async function refreshSessionList(includeSubagents?: boolean): Promise listSessions({ @@ -248,7 +261,11 @@ export async function refreshSessionList(includeSubagents?: boolean): Promise { announceSessionRowChanged('s4'); await vi.waitFor(() => expect(seen).toHaveLength(1)); }); + + it('remembers the tier it last delivered, and nothing it failed to read', async () => { + mocks.getSession + .mockRejectedValueOnce(new Error('403')) + .mockResolvedValueOnce(row('s5', 'private', 'turn:versa_azure')); + listen(); + + announceSessionRowChanged('s5'); + await vi.waitFor(() => expect(mocks.getSession).toHaveBeenCalledTimes(1)); + await new Promise((resolve) => setTimeout(resolve, 20)); + expect(lastKnownSessionTier('s5')).toBeUndefined(); + + announceSessionRowChanged('s5'); + await vi.waitFor(() => expect(lastKnownSessionTier('s5')).toBe('private')); + }); + + it('answers a failed or mismatched row read with null, never a throw', async () => { + mocks.getSession.mockRejectedValueOnce(new Error('network')); + await expect(readSessionRowFacts('s6')).resolves.toBeNull(); + + mocks.getSession.mockResolvedValueOnce(row('someone-else', 'public', null)); + await expect(readSessionRowFacts('s6')).resolves.toBeNull(); + + mocks.getSession.mockResolvedValueOnce(row('s6', 'public', 'declassified_by_user')); + await expect(readSessionRowFacts('s6')).resolves.toEqual({ + sessionId: 's6', + privacy_tier: 'public', + privacy_reason: 'declassified_by_user', + }); + }); }); diff --git a/ui/desktop/src/utils/sessionRowSync.ts b/ui/desktop/src/utils/sessionRowSync.ts index f0721a2fc..618df634d 100644 --- a/ui/desktop/src/utils/sessionRowSync.ts +++ b/ui/desktop/src/utils/sessionRowSync.ts @@ -3,8 +3,10 @@ import { userActionHeaders } from './userAction'; /** * "A chat's ROW changed in place" — for a change the list surfaces render but - * that moves nothing in the list. Today exactly one writer needs it: a - * declassification (issue #56 §12.4). + * that moves nothing in the list: a chat's classification moving in EITHER + * direction. A declassification (issue #56 §12.4) announces itself from the + * dialog that made it; a raise is announced by whichever window's chat store + * first sees it (`ChatStreamRegistry.noteControllerTier`). * * # Why this exists — item 11 of the 1.90.4 hold (2026-09-13) * @@ -28,9 +30,28 @@ import { userActionHeaders } from './userAction'; * - `useSidebarSessions` patches the row it holds, wherever it sits; * - `SessionHistoryView` moves its page badge. * - * An open chat needs none of this: `GET /sessions/changes` compares + * An open chat's store is followed by `GET /sessions/changes`, which compares * `privacy_tier` and `privacy_reason` (never `updated_at`), and its poll tells - * the chat's controller to re-read within about two seconds. + * the chat's controller to re-read within about two seconds. The registry also + * hands a controller every row read made here, so a store that holds a stale + * tier re-reads at once rather than on the next poll. + * + * # Both directions, or the push is a new way to be wrong + * + * ⚠ **Pushing only the lowering was defect D4 of the 2026-09-13 repair round.** + * Before this channel existed a second window never learned of a + * declassification, so it kept a private badge — stale, but in the safe + * direction. Once the lowering was pushed and nothing pushed the raise, the + * sequence "declassify in A, then send one turn on a private model in A" left + * window B's History row AND its sidebar row badging the chat PUBLIC for as + * long as it was watched (90 s, and the sidebar over two minutes), while the + * database read `private` / `turn:versa_azure`. A channel that can lower a + * badge must also be able to raise it, so a store that observes its chat's tier + * change announces here too — see `ChatStreamRegistry.noteControllerTier`. + * + * {@link lastKnownSessionTier} is what keeps that from echoing: a window that + * has already delivered a read of the new tier (because someone announced it) + * does not announce the same change again when its own store catches up. * * # The announcement is a nudge, never a payload * @@ -45,9 +66,9 @@ import { userActionHeaders } from './userAction'; * row is read here and handed to the subscribers, rather than each subscriber * fetching for itself, so a declassification costs one `GET` per window. * - * ⚠ **Announce only what the daemon accepted.** Call it after the write - * resolved 200; a nudge that outran its write would re-read the value it was - * sent to replace. + * ⚠ **Announce only what has landed.** Call it after the write resolved (or a + * read showed that it had), or after a store READ the new tier; a nudge that + * outran its write would re-read the value it was sent to replace. */ export interface SessionRowFacts { @@ -70,6 +91,13 @@ const listeners = new Set(); const newestRead = new Map(); let readCounter = 0; +/** + * The tier of the last read this window DELIVERED for each chat. See + * {@link lastKnownSessionTier}. Holds one short string per chat the channel has + * carried, which is bounded by the chats whose classification moved. + */ +const delivered = new Map(); + let channel: BroadcastChannel | null = null; function getChannel(): BroadcastChannel | null { @@ -87,10 +115,15 @@ function getChannel(): BroadcastChannel | null { return channel; } -async function reread(sessionId: string): Promise { - if (listeners.size === 0) return; - const generation = ++readCounter; - newestRead.set(sessionId, generation); +/** + * Read one chat's classification from the daemon — the row, not the + * transcript, with the proof-of-user a private chat's row needs. + * + * `null` when the read failed or answered for another chat. Never throws: a + * caller that needs to know what happened to a write asks this, and "could not + * ask" is an answer it has to handle rather than an exception it can forget. + */ +export async function readSessionRowFacts(sessionId: string): Promise { try { const response = await getSession({ path: { session_id: sessionId }, @@ -102,17 +135,29 @@ async function reread(sessionId: string): Promise { throwOnError: true, }); const row = response.data; - if (newestRead.get(sessionId) !== generation) return; - if (!row || row.id !== sessionId || !row.privacy_tier) return; - const facts: SessionRowFacts = { + if (!row || row.id !== sessionId || !row.privacy_tier) return null; + return { sessionId, privacy_tier: row.privacy_tier, privacy_reason: row.privacy_reason ?? null, }; - for (const listener of [...listeners]) listener(facts); } catch { - // Silent, as `refreshSessionBinding` is: a read that failed leaves every - // surface exactly as stale as it was, which is not worth a toast. + return null; + } +} + +async function reread(sessionId: string): Promise { + if (listeners.size === 0) return; + const generation = ++readCounter; + newestRead.set(sessionId, generation); + try { + const facts = await readSessionRowFacts(sessionId); + // Silent on failure, as `refreshSessionBinding` is: a read that failed + // leaves every surface exactly as stale as it was, which is not worth a + // toast. + if (!facts || newestRead.get(sessionId) !== generation) return; + delivered.set(sessionId, facts.privacy_tier); + for (const listener of [...listeners]) listener(facts); } finally { if (newestRead.get(sessionId) === generation) newestRead.delete(sessionId); } @@ -127,6 +172,86 @@ export function announceSessionRowChanged(sessionId: string): void { getChannel()?.postMessage({ sessionId }); } +/** + * Re-read `sessionId`'s row for THIS window only, and deliver it to this + * window's subscribers. No other window is told. + * + * For a disagreement this window found by itself: a list answer that differs + * from a row read which landed while that list was in flight. Nobody else holds + * that pair of readings, so nobody else needs the third one that settles it. + */ +export function rereadSessionRowHere(sessionId: string): void { + void reread(sessionId); +} + +/** + * Reconcile a LIST answer with the row reads this window delivered while that + * list request was in flight, and return the rows to publish. + * + * # Why a list surface cannot just take either one + * + * Neither reading is known to be the later one. A list request issued before a + * turn raised a chat can land after the raise was read and patched in, and a + * surface that adopted the list would put the chat back to PUBLIC — a raise + * undone by an older photograph. Replaying the row read over the list is wrong + * the other way: that read may itself have been issued before the list, so it + * could write an older `public` over a newer `private`. + * + * So a disagreement is settled the only way that does not guess: + * + * 1. until it is settled the row shows the HIGHER tier of the two (a chat is + * never drawn public while either reading says private), and + * 2. the row is read a third time, now — after both — and that read patches + * the surface through the ordinary channel ({@link rereadSessionRowHere}). + * + * A row the two readings agree on, and a row no read touched, is published as + * the list answered. + * + * `withReason` compares `privacy_reason` as well, for a surface whose rows carry + * it (`Session`); `SessionSummary` carries only the tier. + * + * Consumes `readDuringFetch` (it is cleared). + */ +export function settleRowsReadDuringFetch< + T extends { + id: string; + privacy_tier?: SessionClassification | null; + privacy_reason?: string | null; + }, +>(rows: T[], readDuringFetch: Map, withReason: boolean): T[] { + if (readDuringFetch.size === 0) return rows; + const unsettled: string[] = []; + const settled = rows.map((row) => { + const read = readDuringFetch.get(row.id); + if (!read) return row; + const agrees = + row.privacy_tier === read.privacy_tier && + (!withReason || (row.privacy_reason ?? null) === read.privacy_reason); + if (agrees) return row; + unsettled.push(row.id); + if (read.privacy_tier !== 'private' || row.privacy_tier === 'private') return row; + return withReason + ? { ...row, privacy_tier: read.privacy_tier, privacy_reason: read.privacy_reason } + : { ...row, privacy_tier: read.privacy_tier }; + }); + readDuringFetch.clear(); + for (const sessionId of unsettled) rereadSessionRowHere(sessionId); + return settled; +} + +/** + * The tier of the last read of `sessionId` this window delivered to its + * subscribers, or `undefined` when the channel has carried nothing for it. + * + * What it is for: a chat store that sees its tier change asks this before + * announcing, and says nothing when the window has already delivered that tier + * — the change was announced by whoever made it, every window has read it, and + * a second announcement would only make every window read it again. + */ +export function lastKnownSessionTier(sessionId: string): SessionClassification | undefined { + return delivered.get(sessionId); +} + /** * Subscribe to freshly-read rows. Returns the unsubscribe. Subscribe from a * MOUNT, or — for a module that owns a cache, as `sessionListCache` does its From 114c4db788bb260120d9c9b10e031aef254097de Mon Sep 17 00:00:00 2001 From: Wanjun Gu Date: Sun, 13 Sep 2026 20:04:32 -0700 Subject: [PATCH 3/4] fix(desktop): one chat's declassify success no longer retracts another chat's failure report MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Repairs the D3a regression an independent tester found in this PR's repair round. Renderer-only: no Rust changed, and neither the write-first lock ordering (#294) nor who may declassify is touched. Reproduced in the dev app first (sandboxed config, this branch's daemon): chat 20260809_21 failed with sessions.db write-locked (POST -> 503, "The chat store was busy"), then 20260809_23 failed the same way and one toast was still on screen. Retrying 20260809_23 answered 200: "Chat marked public" was the only toast, and 8 s later there were none, with the DB reading _21 private and _23 public. Cause: DeclassifySessionDialog keeps outstanding failure reports by chat, but toastError deduplicated them by title + message, and the busy sentence is the same for every chat. Both reports were one toast id, so _23's retraction dismissed _21's report too. A different failure on one chat, or a public row read about one chat, did the same. Fix: toastError takes an optional dedupeScope, and the dialog raises each report under declassify:, with a title that names its chat ("The chat store was busy — chat 20260809_21" for a placeholder name, "— “Subagent delegation request”" otherwise). Identical failures on the same chat still share an id, so a retry replaces its report instead of stacking. Rejected alternative: reference-counting the shared id. It keeps _21's report alive, but as one toast standing for several chats and saying "this chat". After _23 succeeded, it would sit beside "Chat marked public" still saying "this chat was not marked public", about a chat it doesn't name. Measured after the fix, same steps and chats: two toasts, each naming its chat. After _23's 200, _21's report was still on screen at +8 s and +30 s, and _23's was gone. A second busy failure on _21 left one _21 toast, not two. _21's own 200 then retracted it, and no toasts were left at +8 s. Tests: DeclassifySessionDialog.toastLayer.test.tsx (new, 6) renders the real toasts.tsx and react-toastify container, with only the exit animation replaced, because jsdom runs none. On the unfixed code 5 fail ("expected [] to have a length of 1 but got +0", "expected [ Array(1) ] to have a length of 2"). The same-chat retry guard passes before and after. --- .../sessions/DeclassifySessionDialog.test.tsx | 42 ++- ...eclassifySessionDialog.toastLayer.test.tsx | 306 ++++++++++++++++++ .../sessions/DeclassifySessionDialog.tsx | 61 +++- ui/desktop/src/toasts.test.tsx | 27 +- ui/desktop/src/toasts.tsx | 23 +- 5 files changed, 443 insertions(+), 16 deletions(-) create mode 100644 ui/desktop/src/components/sessions/DeclassifySessionDialog.toastLayer.test.tsx diff --git a/ui/desktop/src/components/sessions/DeclassifySessionDialog.test.tsx b/ui/desktop/src/components/sessions/DeclassifySessionDialog.test.tsx index da94c28c7..19ee69da1 100644 --- a/ui/desktop/src/components/sessions/DeclassifySessionDialog.test.tsx +++ b/ui/desktop/src/components/sessions/DeclassifySessionDialog.test.tsx @@ -1,14 +1,18 @@ import { act, cleanup, fireEvent, render, screen, waitFor } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; -import { DeclassifySessionDialog } from './DeclassifySessionDialog'; +import { DeclassifySessionDialog, declassifyToastSubject } from './DeclassifySessionDialog'; import type { Session } from '../../api'; const mocks = vi.hoisted(() => ({ declassifySession: vi.fn(), // Returns the id the real `toastError` returns: its dedupe key, so two - // identical failures share one id exactly as they share one toast. - toastError: vi.fn(({ title, msg }: { title: string; msg: string }) => `error:${title}:${msg}`), + // identical failures on one chat share one id exactly as they share one toast. + // The toast layer itself is exercised in `DeclassifySessionDialog.toastLayer.test.tsx`. + toastError: vi.fn( + ({ title, msg, dedupeScope }: { title: string; msg: string; dedupeScope?: string }) => + `error[${dedupeScope}]:${title}:${msg}` + ), toastSuccess: vi.fn(), dismissToast: vi.fn(), announceSessionRowChanged: vi.fn(), @@ -399,8 +403,9 @@ describe('DeclassifySessionDialog', () => { await waitFor(() => expect(mocks.toastError).toHaveBeenCalledWith({ - title: 'Could not mark this chat public', + title: 'Could not mark this chat public — “Cohort of 4,102 patients”', msg: refusal, + dedupeScope: 'declassify:abc123def456', }) ); expect(onDeclassified).not.toHaveBeenCalled(); @@ -447,8 +452,9 @@ describe('DeclassifySessionDialog', () => { await waitFor(() => expect(mocks.toastError).toHaveBeenCalledWith({ - title: 'The chat store was busy', + title: 'The chat store was busy — “Cohort of 4,102 patients”', msg: STORE_BUSY, + dedupeScope: 'declassify:abc123def456', }) ); // Not escalated: a busy store says nothing about this chat's grade, so the @@ -479,7 +485,7 @@ describe('DeclassifySessionDialog', () => { expect(msg).not.toContain('[object Object]'); // Stated as what the row read back, which is all this dialog knows. expect(msg).toMatch(/still private/); - expect(title).toBe('Could not mark this chat public'); + expect(title).toBe('Could not mark this chat public — “Cohort of 4,102 patients”'); expect(screen.queryByRole('textbox')).toBeNull(); }); }); @@ -582,8 +588,9 @@ describe('an answer that never arrived', () => { await waitFor(() => expect(mocks.toastError).toHaveBeenCalledWith({ - title: 'The chat store was busy', + title: 'The chat store was busy — “Cohort of 4,102 patients”', msg: STORE_BUSY, + dedupeScope: 'declassify:abc123def456', }) ); }); @@ -702,3 +709,24 @@ describe('a failure report is retracted by the next outcome', () => { expect(mocks.dismissToast).toHaveBeenCalledWith(busyId); }); }); + +describe('declassifyToastSubject', () => { + it('names a placeholder-named chat by its id, since dozens of rows share the name', () => { + expect(declassifyToastSubject('New Session', '20260809_21')).toBe('chat 20260809_21'); + expect(declassifyToastSubject('New chat', '20260809_21')).toBe('chat 20260809_21'); + expect(declassifyToastSubject(' ', '20260809_21')).toBe('chat 20260809_21'); + expect(declassifyToastSubject(undefined, '20260809_21')).toBe('chat 20260809_21'); + }); + + it('quotes any other name, and cuts a long one short', () => { + expect(declassifyToastSubject(' Subagent delegation request ', 'x')).toBe( + '“Subagent delegation request”' + ); + const long = declassifyToastSubject('a'.repeat(59) + '😀😀', 'x'); + expect(long).toBe(`“${'a'.repeat(59)}…”`); + // Characters, not UTF-16 units: an emoji at the cut is kept whole or dropped. + expect([...declassifyToastSubject('b'.repeat(58) + '😀😀😀', 'x')].slice(-3).join('')).toBe( + '😀…”' + ); + }); +}); diff --git a/ui/desktop/src/components/sessions/DeclassifySessionDialog.toastLayer.test.tsx b/ui/desktop/src/components/sessions/DeclassifySessionDialog.toastLayer.test.tsx new file mode 100644 index 000000000..273ec3b3e --- /dev/null +++ b/ui/desktop/src/components/sessions/DeclassifySessionDialog.toastLayer.test.tsx @@ -0,0 +1,306 @@ +import { act, cleanup, fireEvent, render, screen, waitFor } from '@testing-library/react'; +import { useEffect } from 'react'; +import { MemoryRouter } from 'react-router-dom'; +import { ToastContainer, toast, type ToastTransitionProps } from 'react-toastify'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { DeclassifySessionDialog } from './DeclassifySessionDialog'; +import type { Session } from '../../api'; + +/** + * Defect D3a, second round (2026-09-13), measured by an independent tester on + * the desktop and reproduced in the dev app before this file existed: + * + * 1. chat Y fails with the store write-locked → 503, "The chat store was busy"; + * 2. chat X fails the same way → still ONE toast on screen; + * 3. X is retried and succeeds → "Chat marked public", and eight seconds later + * nothing at all. The database read Y private and X public: Y's failure + * report was gone although nothing about Y had been decided. + * + * The dialog kept its outstanding reports by CHAT, and `toastError` deduplicated + * them by CONTENT. The busy sentence is identical for every chat, so both + * reports were one toast, and X's success dismissed it. + * + * ⚠ Why these run against the REAL toast layer — `toasts.tsx` and + * react-toastify's container — when `DeclassifySessionDialog.test.tsx` mocks + * `toastError`: the defect lived in the seam between the two keys, and a mock of + * either half encodes an assumption about the other. What is asserted here is + * what the person sees: which reports are on screen. + */ + +const mocks = vi.hoisted(() => ({ + declassifySession: vi.fn(), + announceSessionRowChanged: vi.fn(), + // What `readSessionRowFacts` reads back after a failure, per chat. + rowTiers: new Map(), + rowListeners: new Set<(facts: { sessionId: string; privacy_tier: string }) => void>(), +})); + +vi.mock('../../api', () => ({ + declassifySession: mocks.declassifySession, +})); + +vi.mock('../../utils/userAction', () => ({ + userActionHeaders: async () => ({ 'X-User-Action': 'test-key' }), +})); + +vi.mock('../../utils/sessionRowSync', () => ({ + announceSessionRowChanged: mocks.announceSessionRowChanged, + readSessionRowFacts: async (sessionId: string) => ({ + sessionId, + privacy_tier: mocks.rowTiers.get(sessionId) ?? 'private', + privacy_reason: 'turn:versa_azure', + }), + subscribeSessionRowChanges: ( + listener: (facts: { sessionId: string; privacy_tier: string }) => void + ) => { + mocks.rowListeners.add(listener); + return () => mocks.rowListeners.delete(listener); + }, +})); + +/** The daemon's 503 body, `privacy::declassify::DECLASSIFY_STORE_BUSY`. */ +const STORE_BUSY = + 'Nothing was changed and this chat was not marked public, because other writes kept the chat ' + + 'store busy. Try again in a moment.'; + +/** + * The daemon's 500 body, `routes::session::DECLASSIFY_FAILED`. Not a 400: that + * one escalates the dialog to the typed phrase, which is not what these test. + */ +const FAILED = + 'Nothing was changed and this chat was not marked public, because Biorouter hit an error. The ' + + 'daemon log records it.'; + +type Answer = { status: number; body?: string }; + +/** Answers each chat's requests in order, the way the generated client returns them. */ +const answers = new Map(); +function answer(sessionId: string, ...queue: Answer[]) { + answers.set(sessionId, [...(answers.get(sessionId) ?? []), ...queue]); +} + +/** + * react-toastify removes a dismissed toast when its exit ANIMATION ends, and + * jsdom runs no animations — so without this every dismissed toast would stay in + * the document and "is it on screen" could never be answered. Only the animation + * is replaced; dismissal, dedup and the container's store are the real ones. + */ +function NoAnimation({ children, isIn, done }: ToastTransitionProps) { + useEffect(() => { + if (!isIn) done(); + }, [isIn, done]); + return <>{children}; +} + +/** Chat Y: a placeholder name, as `20260809_21` has ("New Session"). */ +function chatY(suffix: string): Session { + return { + id: `20260809_21${suffix}`, + name: 'New Session', + working_dir: '/tmp', + created_at: '2026-08-09T12:00:00Z', + updated_at: '2026-08-09T12:00:00Z', + extension_data: {}, + message_count: 4, + privacy_tier: 'private', + privacy_reason: 'turn:versa_azure', + } as unknown as Session; +} + +/** Chat X: a named chat, as `20260809_23` is. */ +function chatX(suffix: string): Session { + return { ...chatY(suffix), id: `20260809_23${suffix}`, name: 'Subagent delegation request' }; +} + +/** + * Open the dialog on `session`, the way both entry points mount it. `press` + * presses Make public in THIS dialog (the single click, so after the undo + * window) and waits for the answer: a failure hands the button back, a success + * calls `onClose`. `close` unmounts it, which is what both entry points do on + * close. One dialog is open at a time, as in the app. + */ +function openDialog(session: Session) { + const onClose = vi.fn(); + const view = render( + + + + ); + const makePublic = () => screen.getByRole('button', { name: /Make public/ }); + return { + press: async () => { + const closedBefore = onClose.mock.calls.length; + await waitFor(() => expect(makePublic()).toBeEnabled()); + fireEvent.click(makePublic()); + await waitFor(() => expect(mocks.declassifySession).toHaveBeenCalledTimes(1)); + await waitFor(() => { + if (onClose.mock.calls.length === closedBefore) expect(makePublic()).toBeEnabled(); + }); + mocks.declassifySession.mockClear(); + }, + close: view.unmount, + }; +} + +/** Fail (or succeed) once on `session` and close the dialog. */ +async function attemptOnce(session: Session) { + const dialog = openDialog(session); + await dialog.press(); + dialog.close(); +} + +/** Every report on screen, as the text a person reads. */ +function reports(): string[] { + return screen + .queryAllByRole('alert') + .map((alert) => (alert.textContent ?? '').replace(/\s+/g, ' ').trim()); +} +const busyReports = () => reports().filter((text) => text.includes('Try again in a moment')); + +beforeEach(() => { + vi.clearAllMocks(); + answers.clear(); + mocks.rowTiers.clear(); + mocks.declassifySession.mockImplementation(async ({ path }: { path: { session_id: string } }) => { + const next = answers.get(path.session_id)?.shift() ?? { status: 200 }; + if (next.status === 200) { + mocks.rowTiers.set(path.session_id, 'public'); + return { + data: { sessionId: path.session_id, privacyTier: 'public' }, + request: {}, + response: { status: 200 }, + }; + } + return { data: undefined, error: next.body, request: {}, response: { status: next.status } }; + }); + render( + + + + ); +}); + +afterEach(async () => { + await act(async () => { + toast.dismiss(); + }); + cleanup(); +}); + +describe('a failure report belongs to its chat', () => { + it('a success on one chat leaves another chat’s failure on screen', async () => { + const y = chatY('a'); + const x = chatX('a'); + answer(y.id, { status: 503, body: STORE_BUSY }); + answer(x.id, { status: 503, body: STORE_BUSY }, { status: 200 }); + + await attemptOnce(y); + const onX = openDialog(x); + await onX.press(); + await onX.press(); + onX.close(); + + await waitFor(() => + expect(reports().some((t) => t.startsWith('Chat marked public'))).toBe(true) + ); + // Nothing about Y was decided, so Y's report is still there — and it is the + // only failure left, because X's own success retracted X's. + expect(busyReports()).toHaveLength(1); + expect(busyReports()[0]).toContain(`chat ${y.id}`); + expect(reports().some((t) => t.includes('Subagent delegation request'))).toBe(false); + }); + + it('two chats failing at once are two reports, each naming its chat', async () => { + const y = chatY('b'); + const x = chatX('b'); + answer(y.id, { status: 503, body: STORE_BUSY }); + answer(x.id, { status: 503, body: STORE_BUSY }); + + await attemptOnce(y); + await attemptOnce(x); + + // The same sentence twice is only useful if each says which chat it is + // about. A placeholder name is shared by dozens of rows, so that chat is + // named by its id. + const busy = busyReports(); + expect(busy).toHaveLength(2); + expect(busy.filter((t) => t.includes(`chat ${y.id}`))).toHaveLength(1); + expect(busy.filter((t) => t.includes('“Subagent delegation request”'))).toHaveLength(1); + }); + + it('two chats with the same name still keep their own reports', async () => { + // Naming the chat in the title is for the person; it is not what keeps the + // reports apart. Two chats can share a name, and then only the chat's id in + // the dedup scope stops one's success taking down the other's report. + const first = { ...chatX('f'), id: '20260809_31f', name: 'Weekly cohort' }; + const second = { ...chatX('f'), id: '20260809_32f', name: 'Weekly cohort' }; + answer(first.id, { status: 503, body: STORE_BUSY }); + answer(second.id, { status: 503, body: STORE_BUSY }, { status: 200 }); + + await attemptOnce(first); + const onSecond = openDialog(second); + await onSecond.press(); + expect(busyReports()).toHaveLength(2); + await onSecond.press(); + onSecond.close(); + + await waitFor(() => + expect(reports().some((t) => t.startsWith('Chat marked public'))).toBe(true) + ); + expect(busyReports()).toHaveLength(1); + }); + + it('a same-chat retry replaces its own report rather than stacking', async () => { + const x = chatX('c'); + answer(x.id, { status: 503, body: STORE_BUSY }, { status: 503, body: STORE_BUSY }); + + const onX = openDialog(x); + await onX.press(); + await onX.press(); + onX.close(); + + expect(busyReports()).toHaveLength(1); + }); + + it('a different failure on one chat does not take down another chat’s report', async () => { + const y = chatY('d'); + const x = chatX('d'); + answer(y.id, { status: 503, body: STORE_BUSY }); + answer(x.id, { status: 503, body: STORE_BUSY }, { status: 500, body: FAILED }); + + await attemptOnce(y); + const onX = openDialog(x); + await onX.press(); + await onX.press(); + onX.close(); + + const busy = busyReports(); + expect(busy).toHaveLength(1); + expect(busy[0]).toContain(`chat ${y.id}`); + // X's busy report was replaced by its new failure, not left beside it. + const faults = reports().filter((t) => t.includes('Biorouter hit an error')); + expect(faults).toHaveLength(1); + expect(faults[0]).toContain('“Subagent delegation request”'); + }); + + it('a public row read about one chat retracts only that chat’s report', async () => { + const y = chatY('e'); + const x = chatX('e'); + answer(y.id, { status: 503, body: STORE_BUSY }); + answer(x.id, { status: 503, body: STORE_BUSY }); + + await attemptOnce(y); + await attemptOnce(x); + expect(busyReports()).toHaveLength(2); + + // X declassified from another window: this window hears it as a row read. + await act(async () => { + for (const listener of [...mocks.rowListeners]) { + listener({ sessionId: x.id, privacy_tier: 'public' }); + } + }); + + await waitFor(() => expect(busyReports()).toHaveLength(1)); + expect(busyReports()[0]).toContain(`chat ${y.id}`); + }); +}); diff --git a/ui/desktop/src/components/sessions/DeclassifySessionDialog.tsx b/ui/desktop/src/components/sessions/DeclassifySessionDialog.tsx index fa6c29ef9..c78616bb8 100644 --- a/ui/desktop/src/components/sessions/DeclassifySessionDialog.tsx +++ b/ui/desktop/src/components/sessions/DeclassifySessionDialog.tsx @@ -7,6 +7,7 @@ import { readSessionRowFacts, subscribeSessionRowChanges, } from '../../utils/sessionRowSync'; +import { isDefaultSessionName } from '../../utils/sessionNameSync'; import { DangerousConfirmDialog } from '../ui/DangerousConfirmDialog'; import { Dialog, @@ -182,8 +183,48 @@ export function describeDeclassifyFailure( * here holds it, reaches this window as a row read (`sessionRowSync`), and a * toast still saying "still private" beside a public chat is the same stale * report. + * + * # One chat, one report — D3a's second round (2026-09-13) + * + * ⚠ Keyed by chat here, and until this round deduplicated by CONTENT in + * `toastError` — and the busy sentence is the same for every chat. Measured in + * the dev app: chat Y failed busy, chat X failed busy (still one toast on + * screen), X was retried and succeeded, and Y's report was gone with Y still + * private. X's retraction had dismissed the one toast both reports shared, and + * a different failure on X, or a row read showing X public, did the same. + * + * So each report is raised under its chat (`dedupeScope`), and names the chat + * it is about (`declassifyToastSubject`). Reference-counting the shared toast + * was the other way to keep Y's report alive, and it is wrong for what the + * person reads: one toast would stand for several chats while saying "this + * chat", so after X succeeded it would sit beside "Chat marked public" still + * saying "this chat was not marked public" — the stale report this map exists + * to retract, now about a chat it does not name. A retry on the SAME chat still + * lands on its own toast id, so it replaces its report rather than stacking. */ const outstandingFailureToasts = new Map(); + +/** + * Which chat a failure toast is about, for its title. Two chats that fail at + * once raise two toasts carrying the same daemon sentence, and those are only + * useful if each says which chat it means. + * + * A placeholder name ("New Session", "New chat", "Session 5" — + * `isDefaultSessionName`) is shared by dozens of rows, so that chat is named by + * its id, which the dialog shows under the name. Any other name is quoted, and + * cut short (by characters, so an emoji is never split) because a toast title + * is not clamped. + */ +export function declassifyToastSubject(name: string | null | undefined, sessionId: string): string { + const trimmed = (name ?? '').trim(); + if (isDefaultSessionName(trimmed)) return `chat ${sessionId}`; + const chars = [...trimmed]; + if (chars.length <= SUBJECT_MAX_CHARS) return `“${trimmed}”`; + const kept = chars.slice(0, SUBJECT_MAX_CHARS - 1).join(''); + return `“${kept.trimEnd()}…”`; +} +const SUBJECT_MAX_CHARS = 60; + let stopFollowingRows: (() => void) | null = null; /** @@ -201,8 +242,9 @@ function retractFailureToast(sessionId: string, keep?: string | number): void { const previous = outstandingFailureToasts.get(sessionId); if (previous === undefined) return; outstandingFailureToasts.delete(sessionId); - // An identical failure is deduplicated onto the SAME toast id, so dismissing - // it would take away the toast that is reporting the attempt just made. + // An identical failure on THIS chat is deduplicated onto the same toast id, + // so dismissing it would take away the toast reporting the attempt just made. + // Another chat's identical failure has its own id and is never reached here. if (previous !== keep) toastService.dismiss(previous); } @@ -278,9 +320,13 @@ export function DeclassifySessionDialog({ // than the window — the session list's own change subscription, a search // debounce — means the request is never sent at all. Pinned by "the undo // window is a deadline, not a countdown a re-render restarts". - const latest = useRef({ onClose, onDeclassified }); + // + // The chat's NAME rides here for the same reason: the daemon renames a chat + // after its early turns, and a `send` keyed on the name would restart the undo + // window when that rename reached this row. + const latest = useRef({ onClose, onDeclassified, sessionName: session.name }); useLayoutEffect(() => { - latest.current = { onClose, onDeclassified }; + latest.current = { onClose, onDeclassified, sessionName: session.name }; }); const send = useCallback( @@ -335,7 +381,12 @@ export function DeclassifySessionDialog({ return; } - const toastId = toastError({ title: failure.title, msg: failure.message }); + // Raised under this chat, and naming it: see `outstandingFailureToasts`. + const toastId = toastError({ + title: `${failure.title} — ${declassifyToastSubject(latest.current.sessionName, session.id)}`, + msg: failure.message, + dedupeScope: `declassify:${session.id}`, + }); retractFailureToast(session.id, toastId); if (toastId !== undefined) { outstandingFailureToasts.set(session.id, toastId); diff --git a/ui/desktop/src/toasts.test.tsx b/ui/desktop/src/toasts.test.tsx index 898c424e8..574e668a0 100644 --- a/ui/desktop/src/toasts.test.tsx +++ b/ui/desktop/src/toasts.test.tsx @@ -2,6 +2,7 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'; const mocks = vi.hoisted(() => ({ success: vi.fn(), + error: vi.fn(), })); // react-toastify is the only thing `toastService.success` actually reaches; stub @@ -9,7 +10,7 @@ const mocks = vi.hoisted(() => ({ vi.mock('react-toastify', () => ({ toast: Object.assign(vi.fn(), { success: mocks.success, - error: vi.fn(), + error: mocks.error, info: vi.fn(), warning: vi.fn(), loading: vi.fn(), @@ -19,7 +20,7 @@ vi.mock('react-toastify', () => ({ }), })); -import { toastService } from './toasts'; +import { toastError, toastService } from './toasts'; describe('toastService.success', () => { beforeEach(() => { @@ -62,3 +63,25 @@ describe('toastService.success', () => { expect(mocks.success.mock.calls[0][1]).toMatchObject({ autoClose: false }); }); }); + +describe('toastError', () => { + beforeEach(() => vi.clearAllMocks()); + + // D3a's second round: two chats that failed with the same sentence shared one + // toast, so the first chat whose failure was overturned dismissed the other's + // report. A scope keeps two subjects apart without giving up dedup within one. + it('a dedupe scope separates identical failures about different subjects', () => { + const busy = { title: 'The chat store was busy', msg: 'Try again in a moment.' }; + toastError({ ...busy, dedupeScope: 'declassify:a' }); + toastError({ ...busy, dedupeScope: 'declassify:b' }); + toastError({ ...busy, dedupeScope: 'declassify:a' }); + toastError(busy); + + const ids = mocks.error.mock.calls.map((call) => call[1].toastId); + expect(ids[0]).not.toBe(ids[1]); + expect(ids[2]).toBe(ids[0]); + // Unscoped callers keep the content key they always had. + expect(ids[3]).toBe('error:The chat store was busy:Try again in a moment.'); + expect(ids[3]).not.toBe(ids[0]); + }); +}); diff --git a/ui/desktop/src/toasts.tsx b/ui/desktop/src/toasts.tsx index de32255df..2a3c4d532 100644 --- a/ui/desktop/src/toasts.tsx +++ b/ui/desktop/src/toasts.tsx @@ -301,6 +301,18 @@ type ToastErrorProps = { * replacing what the user was doing. */ debugFailure?: Omit; + /** + * Narrows the dedup key to one subject: content-identical failures about two + * DIFFERENT subjects are then two toasts, while the same failure about the + * same subject still coalesces. + * + * For a caller that retracts its own report by the returned id. Without a + * scope, two subjects that fail with the same sentence share one toast, and + * the first subject whose failure is overturned dismisses the other's report + * too — defect D3a's second round, where one chat's successful declassify + * took away another chat's "The chat store was busy". + */ + dedupeScope?: string; }; function ToastErrorContent({ @@ -369,7 +381,14 @@ function ToastErrorContent({ ); } -export function toastError({ title, msg, traceback, recoverHints, debugFailure }: ToastErrorProps) { +export function toastError({ + title, + msg, + traceback, + recoverHints, + debugFailure, + dedupeScope, +}: ToastErrorProps) { // An error toast carries actions whenever there is something to copy or a // recovery path to offer — and a toast with actions is not click-to-dismiss, // because the click that misses the button must not destroy the button. @@ -389,7 +408,7 @@ export function toastError({ title, msg, traceback, recoverHints, debugFailure } // never reported. autoClose: false, closeOnClick: !hasActions, - toastId: dedupeKey('error', title, msg), + toastId: dedupeKey(dedupeScope === undefined ? 'error' : `error[${dedupeScope}]`, title, msg), } ); } From 8877ae40145361d795834a2c62f6b4256574ed1c Mon Sep 17 00:00:00 2001 From: Wanjun Gu Date: Sun, 13 Sep 2026 20:40:33 -0700 Subject: [PATCH 4/4] fix(desktop): a declassify failure toast names its chat by id as well The repair before this one made each chat's failure report its own toast and titled it with the chat's name. An independent tester found the gap: names are not unique. Auto-generated names repeat (the seed data holds 20260809_23 and 20260809_25, both "Subagent delegation request"), so two such chats failing at once raised two toasts that read identically, and after one succeeded nothing on the survivor said which chat it meant. A real name is now followed by the chat's id, the same id the dialog shows under the name; a placeholder name is still named by its id alone. New test "tells two chats with the same name apart" fails with the id suffix removed (expected '"Subagent delegation request"' not to be the same) and passes with it. DeclassifySessionDialog.test.tsx: 25 passed. lint:check clean, prettier clean over both files. --- .../sessions/DeclassifySessionDialog.test.tsx | 31 +++++++++++++------ .../sessions/DeclassifySessionDialog.tsx | 16 +++++++--- 2 files changed, 34 insertions(+), 13 deletions(-) diff --git a/ui/desktop/src/components/sessions/DeclassifySessionDialog.test.tsx b/ui/desktop/src/components/sessions/DeclassifySessionDialog.test.tsx index 19ee69da1..b9c4638b5 100644 --- a/ui/desktop/src/components/sessions/DeclassifySessionDialog.test.tsx +++ b/ui/desktop/src/components/sessions/DeclassifySessionDialog.test.tsx @@ -403,7 +403,7 @@ describe('DeclassifySessionDialog', () => { await waitFor(() => expect(mocks.toastError).toHaveBeenCalledWith({ - title: 'Could not mark this chat public — “Cohort of 4,102 patients”', + title: 'Could not mark this chat public — “Cohort of 4,102 patients” (abc123def456)', msg: refusal, dedupeScope: 'declassify:abc123def456', }) @@ -452,7 +452,7 @@ describe('DeclassifySessionDialog', () => { await waitFor(() => expect(mocks.toastError).toHaveBeenCalledWith({ - title: 'The chat store was busy — “Cohort of 4,102 patients”', + title: 'The chat store was busy — “Cohort of 4,102 patients” (abc123def456)', msg: STORE_BUSY, dedupeScope: 'declassify:abc123def456', }) @@ -485,7 +485,9 @@ describe('DeclassifySessionDialog', () => { expect(msg).not.toContain('[object Object]'); // Stated as what the row read back, which is all this dialog knows. expect(msg).toMatch(/still private/); - expect(title).toBe('Could not mark this chat public — “Cohort of 4,102 patients”'); + expect(title).toBe( + 'Could not mark this chat public — “Cohort of 4,102 patients” (abc123def456)' + ); expect(screen.queryByRole('textbox')).toBeNull(); }); }); @@ -588,7 +590,7 @@ describe('an answer that never arrived', () => { await waitFor(() => expect(mocks.toastError).toHaveBeenCalledWith({ - title: 'The chat store was busy — “Cohort of 4,102 patients”', + title: 'The chat store was busy — “Cohort of 4,102 patients” (abc123def456)', msg: STORE_BUSY, dedupeScope: 'declassify:abc123def456', }) @@ -718,15 +720,26 @@ describe('declassifyToastSubject', () => { expect(declassifyToastSubject(undefined, '20260809_21')).toBe('chat 20260809_21'); }); - it('quotes any other name, and cuts a long one short', () => { + it('quotes any other name, cuts a long one short, and follows it with the id', () => { expect(declassifyToastSubject(' Subagent delegation request ', 'x')).toBe( - '“Subagent delegation request”' + '“Subagent delegation request” (x)' ); const long = declassifyToastSubject('a'.repeat(59) + '😀😀', 'x'); - expect(long).toBe(`“${'a'.repeat(59)}…”`); + expect(long).toBe(`“${'a'.repeat(59)}…” (x)`); // Characters, not UTF-16 units: an emoji at the cut is kept whole or dropped. - expect([...declassifyToastSubject('b'.repeat(58) + '😀😀😀', 'x')].slice(-3).join('')).toBe( - '😀…”' + expect(declassifyToastSubject('b'.repeat(58) + '😀😀😀', 'x')).toBe( + `“${'b'.repeat(58)}😀…” (x)` ); }); + + it('tells two chats with the same name apart', () => { + // Auto-generated names repeat: the seed data already holds 20260809_23 and + // 20260809_25, both "Subagent delegation request". Two failures at once must + // not raise two toasts that read the same. + const a = declassifyToastSubject('Subagent delegation request', '20260809_23'); + const b = declassifyToastSubject('Subagent delegation request', '20260809_25'); + expect(a).not.toBe(b); + expect(a).toContain('20260809_23'); + expect(b).toContain('20260809_25'); + }); }); diff --git a/ui/desktop/src/components/sessions/DeclassifySessionDialog.tsx b/ui/desktop/src/components/sessions/DeclassifySessionDialog.tsx index c78616bb8..bcb8e5b06 100644 --- a/ui/desktop/src/components/sessions/DeclassifySessionDialog.tsx +++ b/ui/desktop/src/components/sessions/DeclassifySessionDialog.tsx @@ -213,15 +213,23 @@ const outstandingFailureToasts = new Map(); * `isDefaultSessionName`) is shared by dozens of rows, so that chat is named by * its id, which the dialog shows under the name. Any other name is quoted, and * cut short (by characters, so an emoji is never split) because a toast title - * is not clamped. + * is not clamped — and it is followed by the id too, because a real name is not + * unique either: auto-generated names repeat, and two chats both called + * "Subagent delegation request" failing at once would otherwise raise two + * toasts that read identically. */ export function declassifyToastSubject(name: string | null | undefined, sessionId: string): string { const trimmed = (name ?? '').trim(); if (isDefaultSessionName(trimmed)) return `chat ${sessionId}`; const chars = [...trimmed]; - if (chars.length <= SUBJECT_MAX_CHARS) return `“${trimmed}”`; - const kept = chars.slice(0, SUBJECT_MAX_CHARS - 1).join(''); - return `“${kept.trimEnd()}…”`; + const quoted = + chars.length <= SUBJECT_MAX_CHARS + ? `“${trimmed}”` + : `“${chars + .slice(0, SUBJECT_MAX_CHARS - 1) + .join('') + .trimEnd()}…”`; + return `${quoted} (${sessionId})`; } const SUBJECT_MAX_CHARS = 60;