From a350b39af99a212b2fe9440d38c8dd883d698e9b Mon Sep 17 00:00:00 2001 From: Wanjun Gu Date: Thu, 10 Sep 2026 10:59:55 -0700 Subject: [PATCH 1/3] fix(privacy): a turn runs on the provider its session ROW names (M4) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Gate B only looked at the session row when the CLASSIFICATION demanded a repair. A row rewritten by anything outside this agent's process — the documented `biorouter session --resume … --provider …`, a second daemon, a schedule — therefore moved the row and the composer while the live agent, bound once at resume by `restore_provider_from_session`, went on serving what it had. Measured on merged main: `token_events` id 3662 recorded `model_id = gpt-5.5-2026-04-24` for a turn whose `sessions.model_config_json` said `gpt-4.1-2025-04-14`. The visible symptom was the composer chip going A -> B -> A inside 3.8s of one Send, but the flicker is downstream: the turn's own frame and the post-turn row re-read disagreed because the turn really did run somewhere the row did not name. Both endpoints were `versa_azure` here, so nothing crossed a tier; a row rewritten ACROSS tiers is the same mechanism with a disclosure at the end of it. `Agent::reply` now compares the row's `provider_name` and its model NAME against the live binding and, when they differ, rebinds from the row through the same construction path Gate B's repair uses — so Gate A still applies, a row naming a provider its own tier forbids is refused exactly as before, and the test seam still works. The pin the turn reports is then the row's by construction. Three properties are load-bearing and each has a test that fails without it: - Conditional on an actual difference. `restore_provider_from_session` is unconditional, which is precisely why resume is forbidden to call it (`resume_only_restores_a_provider_when_the_live_agent_is_missing_one`: it "discards its provider-local session"). Rebuilding a provider that already matches throws away a live Codex or Claude Code child for nothing; rebinding only on a disagreement cannot. - Provider name and model NAME, never the whole `ModelConfig`. A lead/worker composite re-serialises its routing state on every `get_model_config()`, so a wider comparison would report drift on any turn that advanced lead->worker and rebuild the composite from a stale snapshot, every turn. - An un-honourable row keeps the legal binding rather than refusing the turn. Refusing there would stop safe work on a chat somebody else broke. `rebind_from_row` now takes DR-15's master switch from the one read at the seam instead of applying its tier check unconditionally: Gate A's own statement admits a public provider onto a private row when the switch is off, so that state is producible, and a rebind that refused it would ignore a binding the user had just chosen with the barrier turned off. No call site was added to `privacy::floor(` or to the ratchet needles; the repo-grep censuses are unchanged (privacy_guard_wiring 3 passed, privacy_capability 4 passed). Tests, all with BIOROUTER_DISABLE_KEYRING=true: agents::agent::gate 38 passed (baseline 33, +5) privacy:: 232 passed subagent 197 passed -p biorouter --lib 3775 passed, 0 failed, 2 ignored Falsified both ways before being trusted: with the drift predicate wired to `false`, the two adoption tests fail with `left: (0, 1) right: (1, 0)` — the stale provider serving the turn, which is the runtime symptom verbatim; with it wired to `true`, the two no-churn tests fail instead. --- crates/biorouter/src/agents/agent.rs | 396 ++++++++++++++++++++++++++- 1 file changed, 393 insertions(+), 3 deletions(-) diff --git a/crates/biorouter/src/agents/agent.rs b/crates/biorouter/src/agents/agent.rs index 4d1fd9be7..fad9a7b7c 100644 --- a/crates/biorouter/src/agents/agent.rs +++ b/crates/biorouter/src/agents/agent.rs @@ -1991,6 +1991,53 @@ pub(crate) fn is_parking_workspace_tool(name: &str) -> bool { ) } +/// Whether the session ROW names a binding other than the one `bound` is. +/// +/// M4's drift test, and the whole of it: a chat's row is the standing record of +/// what it runs on, so a row that has moved away from the live binding is a turn +/// about to run somewhere the user is not being told about. +/// +/// ⚠ **Provider name and model NAME only — never the whole `ModelConfig`.** That +/// narrowness is load-bearing in both directions. +/// +/// Too wide and this fires on turns where nothing moved. A lead/worker +/// composite's `get_model_config()` is DYNAMIC: it re-serialises the routing +/// state on every call (`LeadWorkerProvider::get_model_config` → +/// `PersistedProviderConfig::with_routing_state`), so comparing whole configs +/// would report drift on any turn that advanced lead→worker and rebuild the +/// composite from a stale snapshot, every turn, forever. `model_name`, by +/// contrast, is the LEAD's model name and constant across routing states. +/// +/// Too narrow — provider name alone — and it misses the case that was actually +/// measured, which kept `versa_azure` and only changed the model. +/// +/// The accessors are the ones every write already round-trips through: +/// `update_provider` persists `provider.get_name()` and +/// `providers::persisted_model_config(provider)`, and every `restore_binding` +/// implementation carries `get_model_config()`'s `model_name` through unchanged +/// (the registry default passes the config itself; the exact-restore bindings +/// clone `self.model` and add a marker to `request_params`). So a binding that +/// came from this row compares EQUAL by construction, and a phantom rebind is +/// not reachable through a normalisation difference. +/// +/// A row with no `provider_name` names nothing to honour. A row with a +/// `provider_name` but no `model_config` is a legacy row whose model would have +/// to be invented from global config — [`Agent::rebind_from_row`] does invent +/// one, but inventing it here would report drift against a value the row never +/// stated. Both answer `false`. +fn row_names_another_binding(row: &Session, bound: &dyn Provider) -> bool { + let Some(row_provider) = row.provider_name.as_deref() else { + return false; + }; + if row_provider != bound.get_name() { + return true; + } + match row.model_config.as_ref() { + Some(model_config) => model_config.model_name != bound.get_model_config().model_name, + None => false, + } +} + pub struct ToolCategorizeResult { pub frontend_requests: Vec, pub remaining_requests: Vec, @@ -6448,7 +6495,21 @@ impl Agent { /// for byte — and re-entering Gate A to write a row's own value back to /// itself would put a database write on the front of every turn of a /// rehydrated session. - async fn rebind_from_row(&self, row: &Session) -> Result { + /// + /// ⚠ `privacy_enforced` is DR-15's master switch, **passed in from the one + /// read at the seam** rather than read here. Two reasons, and the second is + /// the reason it is a parameter and not a `privacy_tiers_enabled()` call one + /// line down. First, the seam samples the switch exactly once so that the + /// turn barrier and the ratchet under it cannot observe it at different + /// instants; a second read inside this function would reopen precisely that + /// window. Second, the tier check below has to answer the switch at all: + /// with the switch off, Gate A's own statement admits a public provider onto + /// a private row (`bind_provider_if_allowed` folds the toggle into its bound + /// parameter), so a row in that state is producible — and a rebind that + /// refused it would silently ignore a binding the user had just chosen, in a + /// configuration where they have turned the barrier off. The existing caller + /// is itself gated on the switch, so for it this changes nothing. + async fn rebind_from_row(&self, row: &Session, privacy_enforced: bool) -> Result { let Some(provider_name) = row.provider_name.clone() else { return Ok(false); }; @@ -6479,7 +6540,7 @@ impl Agent { let provider = crate::providers::create_from_persisted(&provider_name, model_config).await?; - if !crate::privacy::bind_allowed(provider.tier(), row.privacy_tier) { + if privacy_enforced && !crate::privacy::bind_allowed(provider.tier(), row.privacy_tier) { return Ok(false); } *self.provider.lock().await = Some(provider); @@ -8341,6 +8402,73 @@ impl Agent { // revisits a row. let privacy_enforced = crate::privacy::privacy_tiers_enabled(); if let Some(row) = privacy_row.as_ref() { + // M4. The row is the standing record of what this chat runs on, and + // until now only the PRIVACY arm below could make a turn honour it — + // so a row rewritten by anything outside this agent's process + // (`biorouter session --resume … --provider …`, a second daemon, a + // schedule) moved the row and the composer while the live agent went + // on serving the binding it took at resume. Measured: `token_events` + // recorded `gpt-5.5` for a turn whose row said `gpt-4.1`, and the + // chip flickered A → B → A across one Send because the turn's own + // frame and the post-turn row re-read disagreed. + // + // ⚠ The client-side flicker is the symptom; the turn running + // somewhere the row does not name is the defect. Fixing only the + // renderer would have made the composer confidently state a binding + // no turn used. Rebinding here makes the pin and the row agree BY + // CONSTRUCTION — the frame a few lines down reads the provider this + // rebind installed — so there is nothing left for a client to + // reconcile. + // + // ⚠ Conditional on an actual difference, and that is what separates + // it from `restore_provider_from_session`, which resume is + // deliberately forbidden to call for exactly this reason + // (`routes/agent.rs`'s `resume_only_restores_a_provider_when_the_live_agent_is_missing_one`: + // "resume unconditionally rebinds a live Codex or Claude child and + // discards its provider-local session"). An unconditional rebind + // rebuilds the provider even when the row and the binding already + // agree, which throws away a live coding-agent child session for + // nothing. A rebind only when they DISAGREE cannot: the child + // belonged to a binding the row no longer names. + // + // ⚠ NOT gated on `privacy_enforced`. Honouring the row is a + // correctness property, not a privacy control — DR-15's switch turns + // off the gates and the ratchet, not the question of which model a + // chat runs on. The tier check that DOES belong to the switch lives + // inside `rebind_from_row`, which is why the flag is threaded into it + // rather than re-read there. + if let Some(bound) = self.bound_provider_unchecked().await { + if row_names_another_binding(row, bound.as_ref()) { + match self.rebind_from_row(row, privacy_enforced).await { + // Bound to what the row names. The privacy arm below + // re-reads the binding, so it judges the NEW one. + Ok(true) => {} + // The row names nothing usable — a provider whose tier + // its own classification forbids, or one the factory + // could not build (missing credentials, a catalog entry + // that is gone). Keep the binding we have and let the + // privacy arm decide, which is the same answer as today: + // this turn was already going to run on it, and it is + // still the only binding that exists. + // + // ⚠ Deliberately not a refusal. A refusal here would + // stop turns that are running safely, on a chat whose + // row someone else broke — new behaviour for a case that + // is not a disclosure. The privacy arm still refuses the + // one case that is. + Ok(false) => debug!( + session_id = %session_config.id, + "session row names a binding this turn cannot adopt; \ + keeping the live provider" + ), + Err(e) => warn!( + session_id = %session_config.id, + "could not rebind to the provider the session row names ({e}); \ + keeping the live provider" + ), + } + } + } let bound = self.bound_provider_unchecked().await; // No provider bound at all reads as Public — the fail-SAFE side. // Public is the less privileged tier, so an agent with nothing @@ -8350,7 +8478,7 @@ impl Agent { .map(|provider| provider.tier()) .unwrap_or(ProviderTier::Public); if privacy_enforced && !crate::privacy::bind_allowed(bound_tier, row.privacy_tier) { - match self.rebind_from_row(row).await { + match self.rebind_from_row(row, privacy_enforced).await { // 2. The row still names a provider whose tier satisfies // the classification: rebind and continue. // @@ -20399,6 +20527,268 @@ mod gate_b_turn_tests { ); } + // ─── M4: the row moved, the live agent did not ──────────────────────── + // + // Everything below is about a row rewritten by something that is not this + // agent — `biorouter session --resume … --provider …`, a second daemon, a + // schedule. `update_provider` writes the row AND rebinds, so no in-process + // sequence produces the state; `point_row_at` does, through Gate A's own + // statement, so the fixture cannot build one the bind path would refuse. + + /// A second private provider, distinguishable from [`private_provider`] by + /// its model alone — which is the case that was actually measured, and the + /// one a provider-name comparison would miss. + fn other_private_model() -> (Arc, Arc) { + counted("versa_azure", "gpt-4.1", ProviderTier::Private) + } + + #[tokio::test] + async fn a_row_rewritten_to_another_model_moves_the_next_turn_onto_it() { + // The measured defect, in one test. `token_events` recorded + // `gpt-5.5-2026-04-24` for a turn whose `sessions.model_config_json` + // said `gpt-4.1-2025-04-14`: the CLI rewrote the row from another + // process, and the live agent — bound once at resume by + // `restore_provider_from_session` — never heard about it. Gate B only + // looked at the row when the CLASSIFICATION demanded it, and here it + // does not: both providers are Private, so the tier check passes and the + // turn ran on the stale binding. + let (bound, bound_completions) = counted("versa_azure", "gpt-5.5", ProviderTier::Private); + let (_dir, agent, s) = agent_on(Arc::clone(&bound)).await; + let sm = manager(&agent); + let (row_provider, row_completions) = other_private_model(); + point_row_at(&sm, &s.id, &row_provider).await; + ratchet_to_private(&sm, &s.id).await; + seams::override_rebind_provider( + sm.as_ref(), + &s.id, + "versa_azure", + Arc::clone(&row_provider), + ); + + let events = drain( + agent + .reply(Message::user().with_text("hi"), cfg(&s), None) + .await + .unwrap(), + ) + .await; + + // The decisive assertion, and the one the runtime measurement made: + // WHICH model served the turn. "No error was returned" establishes + // nothing here — the stale binding answers perfectly well. + assert_eq!( + ( + row_completions.load(Ordering::SeqCst), + bound_completions.load(Ordering::SeqCst) + ), + (1, 0), + "the turn must run on the model the ROW names, not on the stale \ + binding:\n{}", + rendered(&events) + ); + assert_eq!( + pinned(&events), + vec![("versa_azure".to_string(), "gpt-4.1".to_string())], + "and its first frame must name that model, so the pin and the row \ + agree by construction:\n{}", + rendered(&events) + ); + assert_eq!( + agent + .provider() + .await + .unwrap() + .get_model_config() + .model_name, + "gpt-4.1" + ); + } + + #[tokio::test] + async fn a_row_rewritten_to_another_provider_moves_a_public_turn_too() { + // The same defect with the privacy arm provably out of the way: a public + // chat, a public provider on both sides, `bind_allowed` true throughout. + // An implementation that hung the drift check off the classification + // would pass the test above (it ratchets) and fail this one. + let (bound, bound_completions) = + counted("anthropic", "claude-opus-4", ProviderTier::Public); + let (_dir, agent, s) = agent_on(Arc::clone(&bound)).await; + let sm = manager(&agent); + let (row_provider, row_completions) = counted("openai", "gpt-4o", ProviderTier::Public); + point_row_at(&sm, &s.id, &row_provider).await; + seams::override_rebind_provider(sm.as_ref(), &s.id, "openai", Arc::clone(&row_provider)); + + let events = drain( + agent + .reply(Message::user().with_text("hi"), cfg(&s), None) + .await + .unwrap(), + ) + .await; + + assert_eq!( + ( + row_completions.load(Ordering::SeqCst), + bound_completions.load(Ordering::SeqCst) + ), + (1, 0), + "a public chat's row is just as much the standing answer:\n{}", + rendered(&events) + ); + assert_eq!( + pinned(&events), + vec![("openai".to_string(), "gpt-4o".to_string())], + "{}", + rendered(&events) + ); + assert_eq!( + reread(&sm, &s.id).await.privacy_tier, + SessionClassification::Public, + "adopting a PUBLIC provider must not ratchet anything" + ); + } + + #[tokio::test] + async fn a_row_that_already_agrees_leaves_the_live_provider_instance_alone() { + // The reason this check is conditional rather than an unconditional + // `restore_provider_from_session` at the top of every turn. Resume is + // forbidden to do that — `routes/agent.rs`'s + // `resume_only_restores_a_provider_when_the_live_agent_is_missing_one` + // spells out why: "resume unconditionally rebinds a live Codex or Claude + // child and discards its provider-local session". A `claude_code` + // provider carries a live child session in the INSTANCE, so rebuilding + // an equal-but-fresh one throws the conversation away for nothing. + // + // The override is registered pointing at a DIFFERENT instance of the + // same name and model, so an implementation that rebound whenever it + // could — rather than only when the row disagrees — swaps the pointer + // and fails here. Asserting on the name would not catch it. + let bound = private_provider(); + let (_dir, agent, s) = agent_on(Arc::clone(&bound)).await; + let sm = manager(&agent); + let decoy = private_provider(); + assert!( + !Arc::ptr_eq(&bound, &decoy), + "the decoy must be a second instance" + ); + seams::override_rebind_provider(sm.as_ref(), &s.id, "versa_azure", Arc::clone(&decoy)); + + let _ = drain( + agent + .reply(Message::user().with_text("hi"), cfg(&s), None) + .await + .unwrap(), + ) + .await; + + assert!( + Arc::ptr_eq(&agent.provider().await.unwrap(), &bound), + "a row that names what is already bound must not rebuild the provider" + ); + } + + #[tokio::test] + async fn a_row_naming_a_forbidden_provider_keeps_the_legal_binding_and_still_runs() { + // Drift the tier cannot honour: the row is private and names a PUBLIC + // provider (producible only out of band — Gate A refuses that write — + // hence the point-then-ratchet order below), while the live binding is + // private and perfectly legal for this chat. + // + // ⚠ Deliberately NOT a refusal. Nothing here is a disclosure: the turn + // was already going to run on a private provider and still does. Turning + // an un-honourable row into a refused turn would stop safe work on a + // chat whose row somebody else broke — a new failure mode invented by + // the fix for a different bug. + let (bound, bound_completions) = counted("versa_azure", "gpt-5.5", ProviderTier::Private); + let (_dir, agent, s) = agent_on(Arc::clone(&bound)).await; + let sm = manager(&agent); + let row_provider = public_provider(); + point_row_at(&sm, &s.id, &row_provider).await; + ratchet_to_private(&sm, &s.id).await; + seams::override_rebind_provider(sm.as_ref(), &s.id, "anthropic", Arc::clone(&row_provider)); + + let events = drain( + agent + .reply(Message::user().with_text("hi"), cfg(&s), None) + .await + .unwrap(), + ) + .await; + + assert!( + !events.iter().any(is_refusal), + "a row the tier forbids must not refuse a turn the tier permits:\n{}", + rendered(&events) + ); + assert_eq!( + bound_completions.load(Ordering::SeqCst), + 1, + "the legal binding still served the turn:\n{}", + rendered(&events) + ); + assert_eq!( + pinned(&events), + vec![("versa_azure".to_string(), "gpt-5.5".to_string())], + "and the frame names what actually ran, not what the row asked for:\n{}", + rendered(&events) + ); + } + + // ⚠ There is deliberately NO turn-level test for the legacy row (a + // `provider_name` with a NULL `model_config`). One was written, and it + // passed with the drift predicate hard-wired to `true` — the rebind it was + // meant to catch never happened, because `rebind_from_row` invents a model + // from `Config::global()` for such a row and that read simply fails in a + // test process. It would have stood as evidence for a property it could not + // observe. The case is covered decisively one level down, in + // `the_drift_test_reads_the_two_fields_a_row_actually_states`, which does + // fail when the predicate is widened. + + #[tokio::test] + async fn the_drift_test_reads_the_two_fields_a_row_actually_states() { + // A unit on the predicate itself, because the turn tests above can only + // reach it through a whole `reply`. The composite case is the one worth + // pinning: `LeadWorkerProvider::get_model_config` re-serialises its + // ROUTING STATE on every call, so a predicate that compared whole + // `ModelConfig`s would report drift on any turn that advanced + // lead→worker and rebuild the composite from a stale snapshot — every + // turn, forever. `model_name` is the lead's and does not move. + // + // A real row rather than a literal: `Session` has no `Default`, and a + // hand-built one is a statement about the shape this test believes the + // store writes rather than the shape it writes. + let bound = private_provider(); + let (_dir, agent, session) = agent_on(Arc::clone(&bound)).await; + let row = |provider: Option<&str>, model: Option<&str>| { + let mut row = session.clone(); + row.provider_name = provider.map(str::to_string); + row.model_config = model.map(ModelConfig::new_or_fail); + row + }; + let _ = &agent; + + assert!(!row_names_another_binding( + &row(Some("versa_azure"), Some("gpt-5.5")), + bound.as_ref() + )); + assert!(row_names_another_binding( + &row(Some("versa_azure"), Some("gpt-4.1")), + bound.as_ref() + )); + assert!(row_names_another_binding( + &row(Some("anthropic"), Some("gpt-5.5")), + bound.as_ref() + )); + assert!( + !row_names_another_binding(&row(Some("versa_azure"), None), bound.as_ref()), + "a legacy row states no model" + ); + assert!( + !row_names_another_binding(&row(None, None), bound.as_ref()), + "a row that names no provider names nothing to honour" + ); + } + #[tokio::test] async fn an_unrepairable_mismatch_refuses_this_turn_and_leaves_the_row_alone() { let (_dir, agent, s) = agent_on(public_provider()).await; From ad30de8783bd13c6b0cdcb1c45bae4fe0e42da70 Mon Sep 17 00:00:00 2001 From: Wanjun Gu Date: Thu, 10 Sep 2026 11:00:07 -0700 Subject: [PATCH 2/3] fix(desktop): a stale row read cannot overwrite a newer binding (M4) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `refreshSessionBinding` is async, adopts the row it reads OVER the turn-reported pin, and since #211 has two callers that can overlap — the end of a turn, and the `/sessions/changes` nudge. It applied its answer with no ordering token and no check that the row was even about this chat, so an older request landing last would overwrite a newer fact. That is the second half of the A -> B -> A the composer was measured doing across one Send. Two guards, each failing exactly one test when removed: - A generation token, bumped by a PIN that actually moves and by the START of a refresh. The rule that falls out is the one the store needs: a row replaces the pin only if it was read AFTER that pin was reported. A read that began earlier is not a disagreement, it is an older photograph. An unchanged pin deliberately does NOT bump it — "the daemon re-reported the same binding" supersedes nothing, and dropping an in-flight read there would cost freshness for no ordering gain. - An identity check on the response. Nothing downstream re-checks the id: every patch writes into this controller's snapshot and into its entry in the shared session list, so a mismatched payload would silently relabel this chat with another one's binding and classification. `sessionBindingSync`'s module doc opened by asserting that a chat's row is what it runs on. That was half true, and the missing half was this finding: the row was what the chat ran on the last time an agent was built for it. The premise is corrected rather than deleted, and points at the daemon-side change that makes it true again. chatStreamStore.binding.test.tsx 19 passed (16 before, +3) targeted binding/pinnedModel/sync/privacy suites 138 passed npm run test:run 440 files, 4920 passed, 1 skipped, 0 errors npm run lint:check clean; prettier clean on all 3 changed files Falsified: removing the generation guard fails "drops an answer that was requested before the pin it would overwrite" with `expected { provider: 'codex' } to deeply equal { provider: 'versa_azure' }`; removing the identity guard fails "drops a row that is about a different chat" with `expected 'versa_azure' to be 'codex'`. --- .../hooks/chatStreamStore.binding.test.tsx | 106 ++++++++++++++++++ ui/desktop/src/hooks/chatStreamStore.tsx | 75 +++++++++++-- ui/desktop/src/utils/sessionBindingSync.ts | 14 +++ 3 files changed, 186 insertions(+), 9 deletions(-) diff --git a/ui/desktop/src/hooks/chatStreamStore.binding.test.tsx b/ui/desktop/src/hooks/chatStreamStore.binding.test.tsx index b86e807c1..03ab0c65d 100644 --- a/ui/desktop/src/hooks/chatStreamStore.binding.test.tsx +++ b/ui/desktop/src/hooks/chatStreamStore.binding.test.tsx @@ -584,3 +584,109 @@ describe('a row changed elsewhere reaches this window', () => { expect(getCachedSessionList()).toBe(before); }); }); + +/** + * M4 — a row read that is already out of date must not win. + * + * `refreshSessionBinding` is `async`, adopts the row it reads OVER the + * turn-reported pin, and since #211 has two callers that can overlap: the end of + * a turn, and the `/sessions/changes` nudge. Measured in the running app as a + * composer chip going `gpt-4.1` → `gpt-5.5` → `gpt-4.1` across a single Send, + * ending on a spelling the turn had not used. + * + * The daemon fixes the underlying fact (Gate B rebinds from the row, so a turn's + * pin and its row agree). These two are the renderer's own guard, and they are + * about ORDER rather than about which value is right. + */ +describe('a stale row read cannot overwrite a newer fact', () => { + it('drops an answer that was requested before the pin it would overwrite', async () => { + const sid = `bind-stale-${++sessionSeq}`; + mocks.resumeAgent.mockResolvedValue({ data: { session: boundSession(sid) } }); + + // Held open, so the read is provably still in flight when the newer fact + // lands. Without that there is no interleaving to test. + let releaseRow!: () => void; + const rowRead = new Promise((resolve) => { + releaseRow = resolve; + }); + mocks.getSession.mockImplementation(async () => { + await rowRead; + // The row as it was BEFORE the switch below — the stale answer. + return { data: boundSession(sid) }; + }); + + const controller = new ChatStreamRegistry().getController(sid); + await controller.loadSession(); + + const refreshed = controller.refreshSessionBinding(); + // A newer statement of the same fact, made while that read is parked. + announceSessionBinding({ + sessionId: sid, + provider: 'versa_azure', + model: 'gpt-4.1-2025-04-24', + contextLimit: 1_050_000, + }); + releaseRow(); + await refreshed; + + expect(controller.getSnapshot().pinnedModel).toEqual({ + provider: 'versa_azure', + model: 'gpt-4.1-2025-04-24', + }); + expect(controller.getSnapshot().session?.provider_name).toBe('versa_azure'); + expect(controller.getSnapshot().session?.model_config?.model_name).toBe('gpt-4.1-2025-04-24'); + }); + + it('drops a row that is about a different chat', async () => { + const sid = `bind-wrongid-${++sessionSeq}`; + mocks.resumeAgent.mockResolvedValue({ data: { session: boundSession(sid) } }); + // Nothing downstream re-checks the id: every patch writes into THIS + // controller's snapshot and into its entry in the shared session list. A + // mismatched payload would silently relabel this chat with another one's + // binding and classification. + mocks.getSession.mockResolvedValue({ + data: boundSession(`${sid}-someone-else`, { + privacy_tier: 'private', + privacy_reason: 'turn:versa_azure', + provider_name: 'versa_azure', + model_config: { model_name: 'gpt-5.5-2026-04-24', toolshim: false }, + }), + }); + + const controller = new ChatStreamRegistry().getController(sid); + await controller.loadSession(); + await controller.refreshSessionBinding(); + + expect(controller.getSnapshot().session?.provider_name).toBe('codex'); + expect(controller.getSnapshot().session?.privacy_tier).toBe('public'); + expect(controller.getSnapshot().pinnedModel).toBeUndefined(); + }); + + /** + * The guard must not simply disable the feature. A read that STARTS after the + * pin is the later fact and still applies — which is #211's whole purpose, and + * the case `moves the turn-reported pin onto the row it just re-read` covers + * through a turn. This is the same property reached directly, so a change to + * the ordering rule cannot pass by breaking only the round-about path. + */ + it('still adopts a row read after the pin was reported', async () => { + const sid = `bind-fresh-${++sessionSeq}`; + mocks.resumeAgent.mockResolvedValue({ data: { session: boundSession(sid) } }); + mocks.getSession.mockResolvedValue({ + data: boundSession(sid, { + provider_name: 'versa_azure', + model_config: { model_name: 'gpt-4.1-2025-04-24', toolshim: false }, + }), + }); + + const controller = new ChatStreamRegistry().getController(sid); + await controller.loadSession(); + announceSessionBinding({ sessionId: sid, provider: 'codex', model: 'gpt-6-astra' }); + await controller.refreshSessionBinding(); + + expect(controller.getSnapshot().pinnedModel).toEqual({ + provider: 'versa_azure', + model: 'gpt-4.1-2025-04-24', + }); + }); +}); diff --git a/ui/desktop/src/hooks/chatStreamStore.tsx b/ui/desktop/src/hooks/chatStreamStore.tsx index 0da0a1550..d8914eea4 100644 --- a/ui/desktop/src/hooks/chatStreamStore.tsx +++ b/ui/desktop/src/hooks/chatStreamStore.tsx @@ -1395,19 +1395,51 @@ class ChatStreamController { }); }; + /** + * Ordering token for {@link refreshSessionBinding}, and the whole of the + * renderer's half of M4. + * + * `refreshSessionBinding` is `async`, has two callers that can overlap (the + * end of a turn, and the `/sessions/changes` nudge), and finishes by adopting + * the row it read OVER the turn-reported pin. Without a token there is nothing + * to stop the answer to an older request from landing last and overwriting a + * newer fact — the A → B → A the composer was measured doing across one Send. + * + * It is bumped by exactly two things, and both are "something newer than a row + * read already in flight is now known": + * + * - a PIN that actually moves ({@link setPinnedModel}), which is a turn saying + * what it is running on or a bind the daemon has just accepted; + * - the START of a refresh, so that of two overlapping reads only the later + * one may apply. + * + * The rule that falls out is the one the store needs: **a row replaces the pin + * only if it was read AFTER that pin was reported.** A read that began earlier + * is not a disagreement, it is an older photograph. + * + * ⚠ An unchanged pin does NOT bump it. "The daemon re-reported the same + * binding" supersedes nothing, and dropping an in-flight read there would + * leave the row stale until the next nudge — a liveness cost paid for no + * ordering gain. + */ + private bindingGeneration = 0; + /** * Record the binding the privacy barrier pinned this chat to. * * Idempotent by value: the frame arrives on every repaired turn, and a fresh * object each time would re-render the composer — including its model chip * and context gauge — once per turn for no change at all. + * + * ⚠ The value comparison also decides whether {@link bindingGeneration} + * moves, so it is read off `this.snapshot` here rather than inside the + * updater. An updater is a pure function of `prev` and must stay one. */ private setPinnedModel = (pinned: PinnedModelView): void => { - this.updateSnapshot((prev) => - prev.pinnedModel?.provider === pinned.provider && prev.pinnedModel?.model === pinned.model - ? prev - : { ...prev, pinnedModel: pinned } - ); + const current = this.snapshot.pinnedModel; + if (current?.provider === pinned.provider && current?.model === pinned.model) return; + this.bindingGeneration += 1; + this.updateSnapshot((prev) => ({ ...prev, pinnedModel: pinned })); }; /** @@ -1844,6 +1876,7 @@ class ChatStreamController { */ async refreshSessionBinding(): Promise { if (!this.sessionId || !this.snapshot.session) return; + const generation = ++this.bindingGeneration; try { const response = await getSession({ path: { session_id: this.sessionId }, @@ -1856,6 +1889,22 @@ class ChatStreamController { }); const row = response.data; if (!row) return; + // Two ways an answer can be about something other than what is on screen + // now, checked in that order because the first is unconditional and the + // second is about time. + // + // ⚠ A row for ANOTHER chat. Nothing downstream re-checks the id — every + // patch below writes into THIS controller's snapshot and into its entry in + // the shared session list — so a mismatched payload would silently + // relabel this chat with another one's binding and tier. Cheap, and the + // one check here that does not depend on ordering. + if (row.id !== this.sessionId) return; + // ⚠ Superseded while in flight. See {@link bindingGeneration}: a pin + // reported after this read began, or a second refresh started after it, + // is the later fact. Adopting an older row over it is the flicker M4 + // measured, and it is worse than a flicker — it makes the composer state + // a binding no turn used. + if (this.bindingGeneration !== generation) return; this.updateSnapshot((prev) => { if (!prev.session) return prev; if ( @@ -1897,10 +1946,18 @@ class ChatStreamController { // composer kept reading `gpt-5.5-2026-04-24` off the pin. // // Replacing rather than clearing keeps the field meaning what it says, and - // the row is the LATER fact: a pin is what a turn reported when it began, - // and this row was read now. The two can only disagree when the row moved - // afterwards — Gate B's repair binds FROM the row, so a repaired turn's - // pin and its row agree by construction. + // the row is the LATER fact — but only because the two guards above have + // established that it is. "This row was read now" was an assumption when + // this comment was written and it was wrong: the read is `async` and has + // two callers that overlap, so an answer landing here may have been + // requested before the pin it is about to overwrite even existed. M4 + // measured that as a chip going A → B → A inside one Send. + // {@link bindingGeneration} is what makes the sentence true again. + // + // The daemon closes the same gap from the other side: Gate B now rebinds + // FROM the row whenever the row names a different provider or model, not + // only when the classification forces a repair — so a turn's pin and its + // row agree by construction and there is normally nothing here to prefer. if (row.provider_name && row.model_config?.model_name) { this.setPinnedModel({ provider: row.provider_name, diff --git a/ui/desktop/src/utils/sessionBindingSync.ts b/ui/desktop/src/utils/sessionBindingSync.ts index 157ecd46b..3ac6e1fb3 100644 --- a/ui/desktop/src/utils/sessionBindingSync.ts +++ b/ui/desktop/src/utils/sessionBindingSync.ts @@ -8,6 +8,20 @@ * `session.provider_name` + `session.model_config` whenever either is set, and * only falls back to the app-wide selection for a chat that names neither. * + * ⚠ **That premise was HALF true when it was written, and the missing half was + * finding M4.** `restore_provider_from_session` binds the row once, at resume, + * and nothing afterwards told a live agent that the row had moved. So while an + * agent existed, the row was not what the chat ran on — it was what the chat + * ran on the last time one was built. Measured: `biorouter session --resume … + * --provider …` rewrote the row from another process, this module's feed carried + * the change, the chip adopted it, and `token_events` then recorded the OLD + * model for the next turn. Everything here is downstream of the daemon making + * the sentence true: Gate B (`Agent::reply`) now rebinds from the row whenever + * the row names a different provider or model, so a turn's own frame and the + * row it re-reads afterwards state the same thing. Reading a fresh row is + * therefore worth doing because it agrees with the turn, not because it + * overrules one. + * * The renderer cannot simply prefer the row, though, because its copy of the * row is a cache. `ChatStreamController` reads it once (from `/agent/resume`) * and then only ever patches it; nothing refetches it. So a per-chat model From 10482c2710b9bfb45c85686415489072c2b8f21b Mon Sep 17 00:00:00 2001 From: Wanjun Gu Date: Thu, 10 Sep 2026 11:00:15 -0700 Subject: [PATCH 3/3] fix(cli): a scripted `session --resume` no longer panics at EOF (M16) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `--interactive` says a person MAY be at the keyboard, never that one is. `biorouter session --resume --session-id … --provider … , model: Option<&str>) -> None } +/// Whether a `cliclack` prompt failed because there is nobody to ask, as +/// opposed to because somebody answered by cancelling (M16). +/// +/// `cliclack` returns [`std::io::ErrorKind::NotConnected`] from its own +/// `is_term()` check before it draws anything — that is a scripted run +/// (` bool { + matches!( + error.kind(), + std::io::ErrorKind::NotConnected | std::io::ErrorKind::UnexpectedEof + ) +} + /// An [`Agent`] whose session manager is the given (private) store, with every /// other config knob identical to [`Agent::new`]. fn agent_with_session_manager(session_manager: Arc) -> Agent { @@ -926,11 +946,37 @@ pub async fn build_session(session_config: SessionBuilderConfig) -> CliSession { let current_workdir = std::env::current_dir().expect("Failed to get current working directory"); if current_workdir != session.working_dir { - if session_config.interactive { - let change_workdir = cliclack::confirm(format!("{} The original working directory of this chat was set to {}. Your current directory is {}. Do you want to switch back to the original working directory?", style("WARNING:").yellow(), style(session.working_dir.display()).cyan(), style(current_workdir.display()).cyan())) + // M16. `--interactive` says a person MAY be at the keyboard, never + // that one is: `biorouter session --resume … Some(change_workdir), + Err(e) if is_end_of_input(&e) => None, + Err(e) => panic!("Failed to get user input: {e:?}"), + } + } else { + None + }; + if let Some(change_workdir) = asked_and_answered { if change_workdir { if !session.working_dir.exists() { output::render_error(&format!( @@ -1172,6 +1218,31 @@ async fn keyring_advice(provider_name: &str) -> &'static str { mod tests { use super::*; + /// M16. `biorouter session --resume --session-id … --provider …