From c697e8cc1d0a9aa26fed457476b4a13cd894fa3d Mon Sep 17 00:00:00 2001 From: Tam Tran Date: Fri, 14 Aug 2026 21:18:33 -0500 Subject: [PATCH 01/22] fix(terminal-core): preserve cache fields dropped on every remount MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit TerminalEngine.mount()'s end-of-mount cache rebuild (the delete-then-set that reorders the entry for LRU eviction) copied ~25 TerminalCacheEntry fields into a fresh object literal but omitted four the type declares: agentColorLocked, lastSnapshot, lastDataAt, lastInputAt. Every remount (tab switch, pane collapse, etc.) silently reset them to undefined: - agentColorLocked gated the color-OSC guard, so a per-agent color lock broke on any remount and the running program's palette OSCs could start overwriting the assigned scheme. - lastDataAt/lastInputAt are the quiet-period gates for the heal/resync settle checks; undefined short-circuits every "&&" guard as already settled, risking a term.reset() mid-keystroke right after a remount. - lastSnapshot losing its value forces one guaranteed extra full repaint on the next mirror-mode resync. Fix: spread the existing cache entry first, before the explicit field list, so any field TerminalCacheEntry declares survives a remount by default instead of needing to be named here. The explicit keys still come after the spread and win where the rebuild intentionally overwrites/resets a field (terminal, fitAddon, disposables, kbState, win32State, etc.) — spreading first cannot clobber them. Added a cache.test.ts case that sets all four fields, remounts on the same cacheKey, and asserts they survive; confirmed it fails against the prior literal and passes with the spread. --- packages/terminal-core/src/TerminalEngine.ts | 8 +++++ .../terminal-core/src/__tests__/cache.test.ts | 36 +++++++++++++++++++ 2 files changed, 44 insertions(+) diff --git a/packages/terminal-core/src/TerminalEngine.ts b/packages/terminal-core/src/TerminalEngine.ts index 5988e20..8a4630e 100644 --- a/packages/terminal-core/src/TerminalEngine.ts +++ b/packages/terminal-core/src/TerminalEngine.ts @@ -1726,6 +1726,14 @@ export class TerminalEngine { // that order IS the LRU order enforceCacheCap() evicts from. terminalCache.delete(this.cacheKey); terminalCache.set(this.cacheKey, { + // Spread the existing entry FIRST so any field TerminalCacheEntry declares + // that isn't explicitly re-listed below survives this rebuild by default. + // Without this, agentColorLocked/lastSnapshot/lastDataAt/lastInputAt were + // silently dropped on every remount (never listed here even though the + // type declares them) — see terminal-cache-drops-fields-on-mount. Explicit + // keys AFTER the spread still win where this rebuild must overwrite/reset + // a field (terminal, fitAddon, disposables, kbState, win32State, ...). + ...existingCache, terminal: term, processId: existingCache?.processId, fitAddon: fit, diff --git a/packages/terminal-core/src/__tests__/cache.test.ts b/packages/terminal-core/src/__tests__/cache.test.ts index 85dfff7..5779341 100644 --- a/packages/terminal-core/src/__tests__/cache.test.ts +++ b/packages/terminal-core/src/__tests__/cache.test.ts @@ -239,6 +239,42 @@ it('never evicts an entry whose element is still in the DOM', () => { expect(terminalCache.size).toBeLessThanOrEqual(MAX_TERMINAL_CACHE_ENTRIES); }); +// --- mount()-end cache rebuild must not drop fields it doesn't explicitly list ----- +// +// TerminalEngine's mount()-end rebuild (the delete-then-set that reorders the Map key +// for LRU) used to copy the cache entry field-by-field into a fresh object literal. +// Any TerminalCacheEntry field NOT named in that literal was silently dropped on every +// remount. agentColorLocked/lastSnapshot/lastDataAt/lastInputAt were the four fields +// missing from the literal (see terminal-cache-drops-fields-on-mount memory note). + +it('a remount preserves agentColorLocked, lastSnapshot, lastDataAt and lastInputAt', () => { + const cacheKey = 'field-preserve'; + + const engine1 = new TerminalEngine(makeFakeBridge(), { cacheKey }); + engine1.mount(makeContainer()); + + const beforeRemount = terminalCache.get(cacheKey)!; + beforeRemount.agentColorLocked = true; + beforeRemount.lastSnapshot = 'snapshot-marker'; + beforeRemount.lastDataAt = 111; + beforeRemount.lastInputAt = 222; + + engine1.unmount(); + + // A fresh engine on the SAME cacheKey (e.g. a tab switch) takes the reattach + // path, which ends in the delete-then-set rebuild under test. + const engine2 = new TerminalEngine(makeFakeBridge(), { cacheKey }); + engine2.mount(makeContainer()); + + const afterRemount = terminalCache.get(cacheKey)!; + expect(afterRemount.agentColorLocked).toBe(true); + expect(afterRemount.lastSnapshot).toBe('snapshot-marker'); + expect(afterRemount.lastDataAt).toBe(111); + expect(afterRemount.lastInputAt).toBe(222); + + engine2.unmount(); +}); + // --- refreshGlyphAtlases (standby/resume blank-text repair) --------------------- function webglEntry(onClear: () => void) { From c81b167041a3dcae926dc3b9f4b14fade5ff65a1 Mon Sep 17 00:00:00 2001 From: Tam Tran Date: Fri, 14 Aug 2026 23:20:08 -0500 Subject: [PATCH 02/22] refactor(identity): split Terminal.tab_id into renderer_terminal_id + owning_tab_id Terminal.tab_id was overloaded: eight readers wanted the renderer LEAF id (tb-* root / tm-* split) and one (emit_external_activity) wanted the owning TAB, which state.tabs never contains for a split leaf. Renamed the field to renderer_terminal_id (semantics unchanged, #[serde(rename = "tab_id")] keeps the wire key identical in both directions) and added a genuinely new owning_tab_id field, defaulting to None so pre-P0-A payloads still deserialise. Pure mechanical rename at all 13 call sites; no behaviour change yet (pty_manager.rs keeps its pc-* fallback for now, removed in a later task). --- src-tauri/src/api_server.rs | 16 +++--- src-tauri/src/commands.rs | 8 ++- src-tauri/src/pty_manager.rs | 3 +- src-tauri/src/state.rs | 105 ++++++++++++++++++++++++++++++++++- 4 files changed, 118 insertions(+), 14 deletions(-) diff --git a/src-tauri/src/api_server.rs b/src-tauri/src/api_server.rs index 76a1456..53a176a 100644 --- a/src-tauri/src/api_server.rs +++ b/src-tauri/src/api_server.rs @@ -351,14 +351,14 @@ async fn list_terminals(State(state): State) -> impl IntoResponse { "processId": t.id, // Stable renderer id: `tm-` for a split pane, `tb-` for a root/solo // pane (where it equals the tabId). This is the UI-level terminal id. - "terminalId": t.tab_id, + "terminalId": t.renderer_terminal_id, "name": t.name, "profile": t.shell, "status": "running", "pid": t.pid, "createdAt": t.created_at, "mode": "ui", - "tabId": t.tab_id, + "tabId": t.renderer_terminal_id, // Command-suggest reads this on reload-reattach to re-seed its prompt // gate DISARMED; the ARMED decision is sampled pre-mount via the // probe_reattach_prompt_gate command, NOT here — a fetch-time sample @@ -541,14 +541,14 @@ async fn create_terminal( "id": t.id, "processId": t.id, // Stable renderer id (`tm-` split / `tb-` root) — the UI terminal id. - "terminalId": t.tab_id, + "terminalId": t.renderer_terminal_id, "name": t.name, "profile": t.shell, "status": "running", "pid": t.pid, "createdAt": t.created_at, "mode": "ui", - "tabId": t.tab_id, + "tabId": t.renderer_terminal_id, "promptHook": t.prompt_hook }))).into_response() } else { @@ -658,7 +658,7 @@ fn emit_external_activity(state: &AppState, terminal_id: & let tab_id = state .terminals .get(terminal_id) - .and_then(|t| t.tab_id.clone()); + .and_then(|t| t.renderer_terminal_id.clone()); if let Err(e) = state.app_handle.emit( "terminal:external-activity", json!({ "terminalId": terminal_id, "tabId": tab_id }), @@ -981,14 +981,14 @@ async fn get_terminal( "id": t.id, "processId": t.id, // Stable renderer id (`tm-` split / `tb-` root) — the UI terminal id. - "terminalId": t.tab_id, + "terminalId": t.renderer_terminal_id, "name": t.name, "profile": t.shell, "status": "running", "pid": t.pid, "createdAt": t.created_at, "mode": "default", - "tabId": t.tab_id + "tabId": t.renderer_terminal_id }))) } else { (StatusCode::NOT_FOUND, Json(json!({ "error": "Terminal not found" }))) @@ -2052,7 +2052,7 @@ async fn fleet_local_run( log::warn!("Failed to emit api:createTerminalTab for fleet terminal: {}", e); } if let Some(mut entry) = state.terminals.get_mut(&new_id) { - entry.tab_id = Some(tab_id); + entry.renderer_terminal_id = Some(tab_id); } new_id } diff --git a/src-tauri/src/commands.rs b/src-tauri/src/commands.rs index 82060d2..ee6477d 100644 --- a/src-tauri/src/commands.rs +++ b/src-tauri/src/commands.rs @@ -305,7 +305,8 @@ fn register_host_terminal( cols, rows, backend: crate::tmux_manager::TerminalBackend::PortablePty, - tab_id: Some(id.to_string()), + renderer_terminal_id: Some(id.to_string()), + owning_tab_id: None, last_input_source: None, last_input_at: None, prompt_hook, @@ -998,7 +999,7 @@ pub async fn close_terminal( ) -> Result<(), String> { // Get the terminal info to retrieve the PID + renderer id. let (pid, tab_id) = if let Some(terminal) = state.terminals.get(&id) { - (terminal.pid, terminal.tab_id.clone()) + (terminal.pid, terminal.renderer_terminal_id.clone()) } else { return Err("Terminal not found".to_string()); }; @@ -2110,7 +2111,8 @@ mod scrollback_restore_tests { cols: 80, rows: 24, backend: crate::tmux_manager::TerminalBackend::PortablePty, - tab_id: Some(id.to_string()), + renderer_terminal_id: Some(id.to_string()), + owning_tab_id: Some(id.to_string()), last_input_source: None, last_input_at: None, prompt_hook: false, diff --git a/src-tauri/src/pty_manager.rs b/src-tauri/src/pty_manager.rs index 7504499..8bc9241 100644 --- a/src-tauri/src/pty_manager.rs +++ b/src-tauri/src/pty_manager.rs @@ -862,7 +862,8 @@ pub fn spawn_terminal( cols, rows, backend: TerminalBackend::PortablePty, - tab_id: Some(tab_id.unwrap_or_else(|| id.clone())), + renderer_terminal_id: Some(tab_id.unwrap_or_else(|| id.clone())), + owning_tab_id: None, last_input_source: None, last_input_at: None, // Mirrors the injected-hook decision above, so reattach can re-arm the diff --git a/src-tauri/src/state.rs b/src-tauri/src/state.rs index a2935e0..6702344 100644 --- a/src-tauri/src/state.rs +++ b/src-tauri/src/state.rs @@ -39,7 +39,30 @@ pub struct Terminal { pub rows: u16, #[serde(default)] pub backend: TerminalBackend, - pub tab_id: Option, + /// The **stable renderer LEAF id** that owns this PTY: `tb-*` for a tab's + /// root/solo pane, `tm-*` for a split pane. Unique per UI pane. It is the + /// PRIMARY KEY of `terminal_history` (`history_store.rs:93-98`) and the + /// `terminalId` of every API identity response. + /// + /// `None` when **no renderer pane owns this terminal** (a headless API or + /// fleet spawn). Such a terminal is deliberately kept OUT of the history + /// table — see `history_key`. Before P0-A this was never `None` at runtime; + /// it fell back to the ephemeral `pc-*` process id, which cannot survive a + /// restart (design 011 §5, corrected after review 086). + /// + /// `#[serde(rename)]`, NOT `alias`: `alias` accepts the old key inbound but + /// EMITS the Rust field name, silently changing the wire contract. `rename` + /// preserves `tab_id` in both directions (design 011 §6). + #[serde(rename = "tab_id")] + pub renderer_terminal_id: Option, + /// The **tab** that owns the pane above. Equal to `renderer_terminal_id` + /// for a root/solo pane; different for a split. `None` when unknown (a + /// headless spawn, or a client that predates P0-A). + /// + /// NEW in P0-A: the backend had no notion of tab ownership at all before — + /// it lived only in the renderer's `panesSlice.treesByTabId`. + #[serde(default)] + pub owning_tab_id: Option, /// Source of the most recent PTY write: "user" (Tauri invoke = keystrokes/ /// paste) or "api" (REST/MCP input/execute). Drives the per-agent color-scheme /// revert-vs-sticky decision (see docs/plan/007-agent-color-schemes-plan.md). @@ -633,7 +656,7 @@ impl AppState { // since a dead terminal is never persisted again. let guard_arc = self.history_persist_guard(id); let _guard = guard_arc.lock().unwrap_or_else(|e| e.into_inner()); - let Some(tab_id) = self.terminals.get(id).and_then(|t| t.tab_id.clone()) else { return }; + let Some(tab_id) = self.terminals.get(id).and_then(|t| t.renderer_terminal_id.clone()) else { return }; // Skip when the parser is absent or the whole buffer is blank (brand-new or // already-cleared terminal) so we never persist a blank blob that would replay as // an empty "session restored" divider with nothing above it. @@ -1693,3 +1716,81 @@ mod reattach_plan_tests { assert!(teardown.is_empty()); } } + +#[cfg(test)] +mod terminal_identity_serde_tests { + use super::{Terminal, TerminalBackend}; + + fn sample() -> Terminal { + Terminal { + id: "pc-abc123def".into(), + pid: 4242, + shell: "pwsh".into(), + name: "Terminal-pwsh".into(), + created_at: "2026-08-14T10:00:00+07:00".into(), + cols: 120, + rows: 40, + backend: TerminalBackend::PortablePty, + renderer_terminal_id: Some("tm-9f2c1a4b7".into()), + owning_tab_id: Some("tb-4e8d0c2f1".into()), + last_input_source: None, + last_input_at: None, + prompt_hook: false, + } + } + + /// The EMITTED key must stay `tab_id`. `#[serde(alias = "tab_id")]` would + /// accept the old key inbound but emit `renderer_terminal_id`, silently + /// changing the output contract — `rename` preserves the key in BOTH + /// directions (design 011 §6). This repo has already shipped one silent + /// serde-key misroute (fleet MCP `targetOS`), so assert the emitted key + /// itself, not merely that a round-trip survives. + #[test] + fn the_emitted_renderer_id_key_is_still_tab_id() { + let v = serde_json::to_value(sample()).expect("serialize"); + let obj = v.as_object().expect("object"); + assert!( + obj.contains_key("tab_id"), + "emitted keys were {:?}", + obj.keys().collect::>() + ); + assert!( + !obj.contains_key("renderer_terminal_id"), + "the Rust field name must NOT leak onto the wire" + ); + assert_eq!(obj["tab_id"], serde_json::json!("tm-9f2c1a4b7")); + } + + /// The new field is additive and emits under its own key. + #[test] + fn owning_tab_id_is_emitted_alongside() { + let v = serde_json::to_value(sample()).expect("serialize"); + assert_eq!(v["owning_tab_id"], serde_json::json!("tb-4e8d0c2f1")); + } + + /// A payload written by a build that predates P0-A has `tab_id` and no + /// owner. It must still deserialise (success criterion 6). + #[test] + fn a_legacy_payload_without_an_owner_still_deserialises() { + let legacy = serde_json::json!({ + "id": "pc-abc123def", + "pid": 4242, + "shell": "pwsh", + "name": "Terminal-pwsh", + "created_at": "2026-08-14T10:00:00+07:00", + "tab_id": "tb-4e8d0c2f1" + }); + let t: Terminal = serde_json::from_value(legacy).expect("legacy payload"); + assert_eq!(t.renderer_terminal_id.as_deref(), Some("tb-4e8d0c2f1")); + assert_eq!(t.owning_tab_id, None); + } + + #[test] + fn a_round_trip_preserves_all_three_identities() { + let json = serde_json::to_string(&sample()).expect("serialize"); + let back: Terminal = serde_json::from_str(&json).expect("deserialize"); + assert_eq!(back.id, "pc-abc123def"); + assert_eq!(back.renderer_terminal_id.as_deref(), Some("tm-9f2c1a4b7")); + assert_eq!(back.owning_tab_id.as_deref(), Some("tb-4e8d0c2f1")); + } +} From e4a1017de5d099bf5b14e1d779c046b4ef55367b Mon Sep 17 00:00:00 2001 From: Tam Tran Date: Fri, 14 Aug 2026 23:22:11 -0500 Subject: [PATCH 03/22] feat(api): expose owningTabId on every terminal identity response list_terminals, create_terminal and get_terminal each hand-built the same identity JSON block and had already drifted (get_terminal omitted promptHook and used a different mode string). Factored them through one terminal_identity_json helper so they can't drift again, and it adds the new owningTabId field to all three. tabId keeps its existing meaning (the renderer leaf) as a deprecated alias of terminalId, so no existing client observes a change other than the additive field. --- src-tauri/src/api_server.rs | 146 +++++++++++++++++++++++------------- 1 file changed, 95 insertions(+), 51 deletions(-) diff --git a/src-tauri/src/api_server.rs b/src-tauri/src/api_server.rs index 53a176a..3064131 100644 --- a/src-tauri/src/api_server.rs +++ b/src-tauri/src/api_server.rs @@ -343,29 +343,42 @@ fn health_body(instance_id: &str) -> serde_json::Value { }) } +/// The identity + status block every terminal-shaped API response carries. +/// +/// One function so `list_terminals`, `create_terminal` and `get_terminal` +/// cannot drift — they were three hand-copied `json!` literals that already +/// disagreed (`get_terminal` omitted `promptHook`, and used mode "default"). +/// +/// Key contract (design 011 §4), all of it load-bearing for existing clients: +/// `id` / `processId` — the PTY routing key. Unchanged. +/// `terminalId` — the renderer LEAF (`tb-*` root, `tm-*` split). +/// `tabId` — DEPRECATED alias of `terminalId`. Kept byte-identical +/// so no existing API/MCP client breaks. Removing it is +/// a major-version change, explicitly not done here. +/// `owningTabId` — NEW: the tab that owns the leaf. `null` for a +/// headless (no-renderer-pane) terminal. +fn terminal_identity_json(t: &crate::state::Terminal, mode: &str) -> serde_json::Value { + json!({ + "id": t.id, + "processId": t.id, + "terminalId": t.renderer_terminal_id, + "tabId": t.renderer_terminal_id, + "owningTabId": t.owning_tab_id, + "name": t.name, + "profile": t.shell, + "status": "running", + "pid": t.pid, + "createdAt": t.created_at, + "mode": mode, + // Command-suggest reads this on reload-reattach to re-seed its prompt + // gate DISARMED; the ARMED decision is sampled pre-mount via + // probe_reattach_prompt_gate, NOT here (review 008 M-1). + "promptHook": t.prompt_hook, + }) +} + async fn list_terminals(State(state): State) -> impl IntoResponse { - let terminals: Vec<_> = state.terminals.iter().map(|entry| { - let t = entry.value(); - json!({ - "id": t.id, - "processId": t.id, - // Stable renderer id: `tm-` for a split pane, `tb-` for a root/solo - // pane (where it equals the tabId). This is the UI-level terminal id. - "terminalId": t.renderer_terminal_id, - "name": t.name, - "profile": t.shell, - "status": "running", - "pid": t.pid, - "createdAt": t.created_at, - "mode": "ui", - "tabId": t.renderer_terminal_id, - // Command-suggest reads this on reload-reattach to re-seed its prompt - // gate DISARMED; the ARMED decision is sampled pre-mount via the - // probe_reattach_prompt_gate command, NOT here — a fetch-time sample - // would be stale by the time the engine mounts (review 008 M-1). - "promptHook": t.prompt_hook - }) - }).collect(); + let terminals: Vec<_> = state.terminals.iter().map(|e| terminal_identity_json(e.value(), "ui")).collect(); // Owner discriminator. Terminals live in this process's own AppState, so // every entry above belongs to this instance by construction — the useful // guarantee is therefore at the RESPONSE level: a client that reaches the @@ -536,21 +549,7 @@ async fn create_terminal( } if let Some(t) = state.terminals.get(&id) { - let t = t.value(); - (StatusCode::OK, Json(json!({ - "id": t.id, - "processId": t.id, - // Stable renderer id (`tm-` split / `tb-` root) — the UI terminal id. - "terminalId": t.renderer_terminal_id, - "name": t.name, - "profile": t.shell, - "status": "running", - "pid": t.pid, - "createdAt": t.created_at, - "mode": "ui", - "tabId": t.renderer_terminal_id, - "promptHook": t.prompt_hook - }))).into_response() + (StatusCode::OK, Json(terminal_identity_json(t.value(), "ui"))).into_response() } else { (StatusCode::OK, Json(json!({ "id": id, "status": "running" }))).into_response() } @@ -976,20 +975,7 @@ async fn get_terminal( Path(id): Path, ) -> impl IntoResponse { if let Some(terminal) = state.terminals.get(&id) { - let t = terminal.value(); - (StatusCode::OK, Json(json!({ - "id": t.id, - "processId": t.id, - // Stable renderer id (`tm-` split / `tb-` root) — the UI terminal id. - "terminalId": t.renderer_terminal_id, - "name": t.name, - "profile": t.shell, - "status": "running", - "pid": t.pid, - "createdAt": t.created_at, - "mode": "default", - "tabId": t.renderer_terminal_id - }))) + (StatusCode::OK, Json(terminal_identity_json(terminal.value(), "default"))) } else { (StatusCode::NOT_FOUND, Json(json!({ "error": "Terminal not found" }))) } @@ -3125,6 +3111,64 @@ async fn handle_socket(socket: WebSocket, state: AppState) { mod tests { use super::*; + fn identity_sample() -> crate::state::Terminal { + crate::state::Terminal { + id: "pc-abc123def".into(), + pid: 4242, + shell: "pwsh".into(), + name: "Terminal-pwsh".into(), + created_at: "2026-08-14T10:00:00+07:00".into(), + cols: 120, + rows: 40, + backend: crate::tmux_manager::TerminalBackend::PortablePty, + renderer_terminal_id: Some("tm-9f2c1a4b7".into()), + owning_tab_id: Some("tb-4e8d0c2f1".into()), + last_input_source: None, + last_input_at: None, + prompt_hook: true, + } + } + + /// Exact key names, asserted (design 011 §7 test 4). `tabId` stays a + /// DEPRECATED alias of `terminalId` — redefining it would silently break + /// every existing API/MCP client (D4). + #[test] + fn an_identity_response_carries_all_three_ids_under_exact_keys() { + let v = terminal_identity_json(&identity_sample(), "ui"); + assert_eq!(v["id"], json!("pc-abc123def")); + assert_eq!(v["processId"], json!("pc-abc123def")); + assert_eq!(v["terminalId"], json!("tm-9f2c1a4b7")); + assert_eq!(v["tabId"], json!("tm-9f2c1a4b7")); + assert_eq!(v["owningTabId"], json!("tb-4e8d0c2f1")); + assert_eq!(v["mode"], json!("ui")); + assert_eq!(v["promptHook"], json!(true)); + } + + /// Root-pane invariant (design 011 §7 test 5): leaf == owner. + #[test] + fn a_root_pane_reports_the_same_value_for_leaf_and_owner() { + let mut t = identity_sample(); + t.renderer_terminal_id = Some("tb-4e8d0c2f1".into()); + let v = terminal_identity_json(&t, "ui"); + assert_eq!(v["terminalId"], v["owningTabId"]); + } + + /// Correction C1: before P0-A `tab_id` was never None, so this shape could + /// not occur. It can now — a headless API/fleet spawn has no renderer pane — + /// and it must serialise as JSON null, NOT as the `pc-` process id. + #[test] + fn a_headless_terminal_reports_null_identities_not_a_process_id() { + let mut t = identity_sample(); + t.renderer_terminal_id = None; + t.owning_tab_id = None; + let v = terminal_identity_json(&t, "ui"); + assert_eq!(v["terminalId"], json!(null)); + assert_eq!(v["tabId"], json!(null)); + assert_eq!(v["owningTabId"], json!(null)); + // The PTY is still addressable — only the renderer identities are absent. + assert_eq!(v["id"], json!("pc-abc123def")); + } + #[test] fn the_release_and_dev_renderers_are_both_allowed() { assert!(origin_allowed(Some("http://tauri.localhost"), Some("127.0.0.1:42031"))); From 2034475f5e827fed41917af941186b92aab58415 Mon Sep 17 00:00:00 2001 From: Tam Tran Date: Fri, 14 Aug 2026 23:23:30 -0500 Subject: [PATCH 04/22] feat(api): emit owningTabId on terminal:external-activity emit_external_activity only sent tabId, which for a split pane is the tm-* leaf. flagTabActivity resolves its argument against state.tabs, which holds only root tab ids, so a leaf silently matched nothing and the tab's activity indicator never lit. Extracted the payload into external_activity_payload so the routing contract is unit-testable, and it now also carries owningTabId/processId/rendererTerminalId. The legacy terminalId/tabId keys are unchanged for existing consumers. --- src-tauri/src/api_server.rs | 76 +++++++++++++++++++++++++++++++++++-- 1 file changed, 73 insertions(+), 3 deletions(-) diff --git a/src-tauri/src/api_server.rs b/src-tauri/src/api_server.rs index 3064131..a7e0068 100644 --- a/src-tauri/src/api_server.rs +++ b/src-tauri/src/api_server.rs @@ -642,6 +642,32 @@ async fn resize_terminal( } } +/// The `terminal:external-activity` event body. Pure so the routing contract — +/// which id the renderer is supposed to flash a tab with — is unit-testable. +/// +/// `terminal_id`/`processId` is the DashMap KEY (a `pc-*` id on the in-process +/// path, the renderer leaf on the sidecar path). It is deliberately NOT the +/// same thing `terminalId` means in a REST response; `rendererTerminalId` is +/// the new, unambiguous name for the leaf. +fn external_activity_payload( + process_id: &str, + renderer_terminal_id: Option<&str>, + owning_tab_id: Option<&str>, +) -> serde_json::Value { + json!({ + // Unchanged for existing consumers. + "terminalId": process_id, + "tabId": renderer_terminal_id, + // NEW, unambiguous names. + "processId": process_id, + "rendererTerminalId": renderer_terminal_id, + // NEW: what `flagTabActivity` actually needs. A `tm-*` leaf resolves + // against nothing in `state.tabs`, so before P0-A a split pane's + // activity indicator was silently dropped (design 011 §1.1 item 4). + "owningTabId": owning_tab_id, + }) +} + /// Emit a one-shot "external interaction" signal so the UI can flash the owning /// tab. Fired only from the external-only REST handlers (write input / execute /// prompt) — user keystrokes go through a Tauri invoke command and never reach @@ -654,13 +680,18 @@ fn emit_external_activity(state: &AppState, terminal_id: & t.last_input_source = Some("api".to_string()); t.last_input_at = Some(chrono::Utc::now().timestamp_millis()); } - let tab_id = state + let (renderer_terminal_id, owning_tab_id) = state .terminals .get(terminal_id) - .and_then(|t| t.renderer_terminal_id.clone()); + .map(|t| (t.renderer_terminal_id.clone(), t.owning_tab_id.clone())) + .unwrap_or((None, None)); if let Err(e) = state.app_handle.emit( "terminal:external-activity", - json!({ "terminalId": terminal_id, "tabId": tab_id }), + external_activity_payload( + terminal_id, + renderer_terminal_id.as_deref(), + owning_tab_id.as_deref(), + ), ) { log::trace!("Failed to emit terminal:external-activity: {}", e); } @@ -3169,6 +3200,45 @@ mod tests { assert_eq!(v["id"], json!("pc-abc123def")); } + /// Correction C4. `flagTabActivity` (tabsSlice.ts:133-141) resolves its + /// argument against `state.tabs`, which holds ONLY root tab ids — a `tm-*` + /// leaf finds nothing and the dispatch silently no-ops. The payload must + /// therefore carry the OWNER explicitly. + #[test] + fn a_split_panes_activity_payload_carries_the_owning_tab() { + let v = external_activity_payload( + "pc-abc123def", + Some("tm-9f2c1a4b7"), + Some("tb-4e8d0c2f1"), + ); + assert_eq!(v["owningTabId"], json!("tb-4e8d0c2f1")); + assert_eq!(v["rendererTerminalId"], json!("tm-9f2c1a4b7")); + } + + /// The two pre-existing keys must not move: `terminalId` here has always + /// been the PROCESS id (the DashMap key passed by the caller), unlike every + /// REST response where it is the leaf. That asymmetry is why the new + /// explicit `processId` / `rendererTerminalId` keys exist. + #[test] + fn the_legacy_activity_keys_are_unchanged() { + let v = external_activity_payload( + "pc-abc123def", + Some("tm-9f2c1a4b7"), + Some("tb-4e8d0c2f1"), + ); + assert_eq!(v["terminalId"], json!("pc-abc123def")); + assert_eq!(v["processId"], json!("pc-abc123def")); + assert_eq!(v["tabId"], json!("tm-9f2c1a4b7")); + } + + #[test] + fn an_unknown_terminal_yields_nulls_rather_than_a_missing_key() { + let v = external_activity_payload("pc-gone", None, None); + assert_eq!(v["rendererTerminalId"], json!(null)); + assert_eq!(v["owningTabId"], json!(null)); + assert_eq!(v["terminalId"], json!("pc-gone")); + } + #[test] fn the_release_and_dev_renderers_are_both_allowed() { assert!(origin_allowed(Some("http://tauri.localhost"), Some("127.0.0.1:42031"))); From 44c15b6f3911b3e1e89968e9732c7565bf6f4148 Mon Sep 17 00:00:00 2001 From: Tam Tran Date: Fri, 14 Aug 2026 23:25:43 -0500 Subject: [PATCH 05/22] feat(api): mint a distinct tm- leaf for every pane added to a live tab Adds resolve_api_spawn_identity, the pure decision function behind the fix for the terminal_history primary-key collision: two API-created splits in one tab used to store the same tab_id, reaping one PTY and letting closing one pane delete the other's scrollback. Per spec D7, a fresh tm- leaf is minted whenever the resolved owner already has a live terminal registered against it -- not only when the caller supplied a pane_id. pane_id alone is not a reliable "this is a split" signal: it's optional in the MCP tool, and App.tsx Mode 2 splits an already-populated tab without one. Also closes ground-truth correction C3: a caller-supplied tm- id in the tab field used to be silently discarded and replaced with an unrelated fresh tb-, landing the pane in the wrong tab with no diagnostic. Now fails closed with a message naming the right field (owningTabId). Not yet wired into create_terminal -- that's the next task, kept separate so this one stays a pure, independently-testable decision. --- src-tauri/src/api_server.rs | 243 ++++++++++++++++++++++++++++++++++++ 1 file changed, 243 insertions(+) diff --git a/src-tauri/src/api_server.rs b/src-tauri/src/api_server.rs index a7e0068..9859a11 100644 --- a/src-tauri/src/api_server.rs +++ b/src-tauri/src/api_server.rs @@ -452,11 +452,98 @@ struct CreateTerminalReq { cwd: Option, #[serde(alias = "tabId")] tab_id: Option, + /// The tab that should own the new pane. Preferred over `tab_id`, which is + /// ambiguous for a split (a client reading `tabId` back off a split pane + /// gets a `tm-` LEAF, not a tab). + #[serde(alias = "owningTabId")] + owning_tab_id: Option, #[serde(alias = "paneId")] pane_id: Option, direction: Option, } +/// The two renderer identities an API-created terminal registers. +#[derive(Debug, PartialEq, Eq)] +struct ApiSpawnIdentity { + /// Unique per UI pane. The `terminal_history` PRIMARY KEY and the + /// `terminalId` of every response. + renderer_terminal_id: String, + /// The tab the pane belongs to. + owning_tab_id: String, +} + +/// Mint a renderer id: `-<9 hex chars of a v4 uuid>`, matching the +/// format the renderer's own generator produces (`utils/id.ts:1-8`). +fn mint_renderer_id(prefix: &str) -> String { + let raw = uuid::Uuid::new_v4().to_string().replace('-', ""); + format!("{prefix}-{}", &raw[..9]) +} + +/// Decide both renderer identities for `POST /api/terminals`. +/// +/// `mint` supplies the id so the decision is deterministically testable; +/// production passes `mint_renderer_id`. +/// +/// Rules (design 011 §5 "The corrected write"): +/// * An explicit `owningTabId`, else `tabId`, is the OWNER — accepted verbatim +/// when it starts with `tb-`, exactly as `api_server.rs:494` did before. +/// * A `tm-` value in either field is a PANE id, not a tab id. Before P0-A it +/// was silently discarded and replaced with an unrelated fresh `tb-` +/// (ground-truth correction C3), so the pane appeared in the wrong tab. Fail +/// closed with a message naming the right field. The spec's §5 snippet does +/// not cover this case; this is a GAP FILL, flagged in the plan header. +/// * The leaf must be a fresh `tm-` whenever this create lands in a tab that +/// **already holds a live terminal** — only a tab's FIRST pane may use the +/// tab's own id as its leaf (design 011 §3, D7). `pane_id.is_some()` is one +/// way to know that, but NOT the only one: `paneId` is optional in the MCP +/// tool and `App.tsx` Mode 2 splits a populated tab without it, so keying +/// off `pane_id` alone re-registers the tab root's leaf onto a second live +/// terminal — the `terminal_history` PRIMARY KEY collision P0-A exists to +/// remove (review 095 B1). +/// * Otherwise this is the tab's first/solo pane and leaf == owner, as before. +/// +/// `owner_has_live_terminal` is injected rather than read from `AppState` so this +/// stays a pure unit-testable decision (the Windows test binary cannot build the +/// `integration-tests` feature that `mock_app` needs). A freshly minted owner +/// trivially has no live terminal, so the extra probe cannot disturb the +/// new-tab path. +fn resolve_api_spawn_identity( + tab_id: Option<&str>, + owning_tab_id: Option<&str>, + pane_id: Option<&str>, + owner_has_live_terminal: impl Fn(&str) -> bool, + mut mint: impl FnMut(&str) -> String, +) -> Result { + let owner_hint = owning_tab_id + .or(tab_id) + .map(str::trim) + .filter(|s| !s.is_empty()); + + let owning_tab_id = match owner_hint { + Some(id) if id.starts_with("tb-") => id.to_string(), + Some(id) if id.starts_with("tm-") => { + return Err(format!( + "'{id}' is a pane (leaf) id, not a tab id — pass the owning tab id \ + (the `owningTabId` field of GET /api/terminals/{{id}})" + )) + } + // Absent, blank, or an unrecognised format: mint one, as before. + _ => mint("tb"), + }; + + // D7: the trigger is "this tab is already occupied", not "the caller named a + // pane". `pane_id` is kept as a signal because a caller that DOES name a pane + // is telling us it wants a split even if the tab's live set is momentarily + // empty (e.g. its only PTY just exited). + let renderer_terminal_id = if pane_id.is_some() || owner_has_live_terminal(&owning_tab_id) { + mint("tm") + } else { + owning_tab_id.clone() + }; + + Ok(ApiSpawnIdentity { renderer_terminal_id, owning_tab_id }) +} + async fn create_terminal( State(state): State, Json(payload): Json, @@ -3239,6 +3326,162 @@ mod tests { assert_eq!(v["terminalId"], json!("pc-gone")); } + /// Deterministic id minting so the tests assert values, not shapes. + fn counting_mint() -> impl FnMut(&str) -> String { + let mut n = 0u32; + move |prefix: &str| { + n += 1; + format!("{prefix}-{n:09}") + } + } + + /// An EMPTY tab: no live terminal is registered against any owner yet. + /// Production passes a closure over `state.terminals` (Task 5 Step 3). + fn no_live_terminals(_owner: &str) -> bool { + false + } + + /// THE REGRESSION TEST (design 011 §7 test 1). Two API creates targeting the + /// same tab with a pane_id — the split-a-pane flow — must produce DISTINCT + /// leaves and the SAME owner. Before P0-A both stored `tb-shared01` as + /// `tab_id`, which is the `terminal_history` PRIMARY KEY: one PTY got reaped + /// by StateManager's reconcile, and closing either pane deleted the other's + /// scrollback. + #[test] + fn spawn_identity_two_api_splits_get_distinct_leaves_and_one_owner() { + let mut mint = counting_mint(); + let a = resolve_api_spawn_identity( + Some("tb-shared01"), None, Some("pn-a"), no_live_terminals, &mut mint, + ) + .expect("split a"); + // By the time split b arrives, split a is live in that tab — so BOTH + // signals are true here, and either alone must be enough (see D7). + let b = resolve_api_spawn_identity( + Some("tb-shared01"), None, Some("pn-b"), |owner| owner == "tb-shared01", &mut mint, + ) + .expect("split b"); + + assert_ne!(a.renderer_terminal_id, b.renderer_terminal_id); + assert!(a.renderer_terminal_id.starts_with("tm-")); + assert!(b.renderer_terminal_id.starts_with("tm-")); + assert_eq!(a.owning_tab_id, "tb-shared01"); + assert_eq!(b.owning_tab_id, "tb-shared01"); + } + + /// Root-pane invariant (design 011 §7 test 5), stated the way it is actually + /// true: the leaf equals the owner for a tab's FIRST live terminal. The + /// earlier revision of this test asserted `pane_id == None ⇒ leaf == owner` + /// unconditionally, which locked in the review-095 B1 gap as correct. + #[test] + fn spawn_identity_first_create_into_an_empty_tab_keeps_leaf_equal_to_owner() { + let mut mint = counting_mint(); + let r = resolve_api_spawn_identity( + Some("tb-shared01"), None, None, no_live_terminals, &mut mint, + ) + .expect("root"); + assert_eq!(r.renderer_terminal_id, "tb-shared01"); + assert_eq!(r.owning_tab_id, "tb-shared01"); + } + + /// Design 011 §7 test 9 / D7 — the gap review 095 B1 found, which the suite + /// previously asserted as CORRECT. `paneId` is OPTIONAL in the MCP tool + /// (`mcp-server/src/server.ts:67`), and `App.tsx` Mode 2 + /// (`:1085` `else if (tabId && !paneId)`) serves exactly this shape: it + /// picks a pane in the named tab and splits it (`:1305-1335`). Mode 0 cannot + /// claim it — that branch requires `!tabExists(tabId)` (`:904`). Deciding on + /// `pane_id` alone therefore hands this terminal the leaf the tab's root + /// pane already holds (`TerminalContainer.tsx:108-113`): two live terminals + /// on one `terminal_history` PRIMARY KEY, i.e. the bug in the test above, + /// reached without a `pane_id`. + #[test] + fn spawn_identity_second_create_into_a_populated_tab_gets_a_distinct_leaf() { + let mut mint = counting_mint(); + let root = resolve_api_spawn_identity( + Some("tb-shared01"), None, None, no_live_terminals, &mut mint, + ) + .expect("first create"); + assert_eq!(root.renderer_terminal_id, "tb-shared01"); + + // The identical call, with the tab now occupied by `root`. + let second = resolve_api_spawn_identity( + Some("tb-shared01"), + None, + None, // NO pane_id — the Mode 2 call shape + |owner| owner == "tb-shared01", + &mut mint, + ) + .expect("second create"); + + assert_ne!( + second.renderer_terminal_id, root.renderer_terminal_id, + "a renderer leaf id must be unique per live terminal" + ); + assert!( + second.renderer_terminal_id.starts_with("tm-"), + "a tab's second pane is a split whatever the caller sent, got {}", + second.renderer_terminal_id + ); + assert_eq!(second.owning_tab_id, "tb-shared01", "and it stays in that tab"); + } + + #[test] + fn spawn_identity_no_caller_id_mints_a_tab_exactly_as_before() { + let mut mint = counting_mint(); + let r = resolve_api_spawn_identity(None, None, None, no_live_terminals, &mut mint) + .expect("minted"); + assert_eq!(r.owning_tab_id, "tb-000000001"); + assert_eq!(r.renderer_terminal_id, "tb-000000001"); + } + + #[test] + fn spawn_identity_an_empty_or_unrecognised_tab_id_still_mints_rather_than_failing() { + let mut mint = counting_mint(); + assert!( + resolve_api_spawn_identity(Some(" "), None, None, no_live_terminals, &mut mint) + .expect("blank") + .owning_tab_id + .starts_with("tb-") + ); + assert!(resolve_api_spawn_identity( + Some("legacy-monitor-id"), None, None, no_live_terminals, &mut mint, + ) + .expect("junk") + .owning_tab_id + .starts_with("tb-")); + } + + /// Correction C3. `api_server.rs:494` recognised `tb-` ONLY: a caller that + /// did the "right" thing and sent a genuine `tm-` id had it silently thrown + /// away and replaced by an unrelated fresh `tb-`, so the pane landed in the + /// WRONG tab with no diagnostic. Fail closed instead, and name the field + /// that carries the correct value. + #[test] + fn spawn_identity_a_pane_leaf_id_in_the_tab_field_is_rejected_not_silently_replaced() { + let mut mint = counting_mint(); + let err = resolve_api_spawn_identity( + Some("tm-9f2c1a4b7"), None, Some("pn-a"), no_live_terminals, &mut mint, + ) + .expect_err("a tm- id is a pane id, not a tab id"); + assert!(err.contains("tm-9f2c1a4b7"), "the message must name the offending id: {err}"); + assert!(err.contains("owningTabId"), "the message must name the right field: {err}"); + } + + /// An explicit `owningTabId` wins over `tabId` — it is the unambiguous field. + #[test] + fn spawn_identity_an_explicit_owning_tab_id_takes_precedence() { + let mut mint = counting_mint(); + let r = resolve_api_spawn_identity( + Some("tb-ignored1"), + Some("tb-explicit"), + Some("pn-a"), + no_live_terminals, + &mut mint, + ) + .expect("explicit owner"); + assert_eq!(r.owning_tab_id, "tb-explicit"); + assert!(r.renderer_terminal_id.starts_with("tm-")); + } + #[test] fn the_release_and_dev_renderers_are_both_allowed() { assert!(origin_allowed(Some("http://tauri.localhost"), Some("127.0.0.1:42031"))); From 27faf0fd66427262877985abcbb5ff0d050f69d9 Mon Sep 17 00:00:00 2001 From: Tam Tran Date: Fri, 14 Aug 2026 23:26:28 -0500 Subject: [PATCH 06/22] refactor(pty): spawn_terminal carries the leaf and the owning tab spawn_terminal took one overloaded tab_id and, when it was None, fell back to the pc-* process id -- which persist_terminal_history then upserted like any other key, filing a history row under an id that cannot survive a restart (ground-truth correction C1: the field was never actually None at runtime because of this fallback). Split it into renderer_terminal_id (no fallback -- None means no renderer pane owns this PTY) and a new owning_tab_id parameter. This deliberately does not compile in isolation: the four call sites (api_server.rs create_terminal and fleet_local_run, commands.rs the in-process branch and host_fallback) are wired up by the next two tasks, which need the new owning_tab_id parameter to exist first. --- src-tauri/src/pty_manager.rs | 27 +++++++++++++++++++-------- 1 file changed, 19 insertions(+), 8 deletions(-) diff --git a/src-tauri/src/pty_manager.rs b/src-tauri/src/pty_manager.rs index 8bc9241..e10b5aa 100644 --- a/src-tauri/src/pty_manager.rs +++ b/src-tauri/src/pty_manager.rs @@ -701,12 +701,23 @@ pub fn spawn_terminal( cwd: Option, shell_name: String, terminal_name: String, - // Stable renderer id (tb-…) this terminal persists history under. Registered - // WITH the Terminal before the reader thread starts: a caller patching it in - // after spawn returns would race a fast-exiting shell's exit-path persist, - // which would then file the final scrollback under the ephemeral pc- id - // (review 062 agy F-01). None (API/fleet callers) keeps the pc- id default. - tab_id: Option, + // The stable renderer LEAF id (`tb-*` root, `tm-*` split) this terminal + // persists history under. Registered WITH the Terminal before the reader + // thread starts: a caller patching it in after spawn returns would race a + // fast-exiting shell's exit-path persist, which would then file the final + // scrollback under the ephemeral pc- id (review 062 agy F-01). + // + // `None` means NO renderer pane owns this PTY (headless API/fleet spawn). + // It must NOT fall back to `id`: a `Some(pc-*)` value is persisted like any + // other (`state.rs:642`), producing a history row keyed by an id that does + // not survive a restart, and violating the invariant that a renderer + // identity is always a `tb-*`/`tm-*` leaf (design 011 §5, corrected after + // review 086). Before P0-A this fallback made the field never-None at + // runtime (ground-truth correction C1). + renderer_terminal_id: Option, + // The tab that owns the pane above; `None` when unknown. Equal to + // `renderer_terminal_id` for a root/solo pane. + owning_tab_id: Option, // Restored scrollback (blob + divider) to seed the fresh parser with, BEFORE // the reader thread starts — so persisted history precedes live output and the // next flush preserves it instead of overwriting the stored row with only this @@ -862,8 +873,8 @@ pub fn spawn_terminal( cols, rows, backend: TerminalBackend::PortablePty, - renderer_terminal_id: Some(tab_id.unwrap_or_else(|| id.clone())), - owning_tab_id: None, + renderer_terminal_id, + owning_tab_id, last_input_source: None, last_input_at: None, // Mirrors the injected-hook decision above, so reattach can re-arm the From f468eb6b9923cc9cf6a025d4e154c71c23d36ea9 Mon Sep 17 00:00:00 2001 From: Tam Tran Date: Fri, 14 Aug 2026 23:31:50 -0500 Subject: [PATCH 07/22] fix(api): stop two API splits in one tab colliding on the history key Wires resolve_api_spawn_identity (previously added as a standalone, unused decision function) into create_terminal: both renderer identities are now resolved BEFORE the spawn call, so the Terminal registers with them up front instead of the single ambiguous tab_id the handler used to accept verbatim. The api:createTerminalTab payload gains processId/rendererTerminalId/ owningTabId alongside the existing terminalId/tabId keys, which keep their pre-existing meanings (backend process id and owning tab, respectively) so no existing renderer consumer breaks. Proven end to end at the storage layer: two API splits targeting the same tab now occupy two terminal_history rows, and closing one no longer deletes the other's scrollback (they used to share one row keyed by the tab id). This does not compile in isolation -- it depends on the owning_tab_id parameter Task 7 (commands.rs) still needs to thread through the sidecar path, and fleet_local_run (api_server.rs) still calls the old spawn_terminal signature. The tree recompiles once that lands too. --- src-tauri/src/api_server.rs | 108 ++++++++++++++++++++++++++++++------ 1 file changed, 90 insertions(+), 18 deletions(-) diff --git a/src-tauri/src/api_server.rs b/src-tauri/src/api_server.rs index 9859a11..54c63a0 100644 --- a/src-tauri/src/api_server.rs +++ b/src-tauri/src/api_server.rs @@ -586,21 +586,32 @@ async fn create_terminal( let rows = payload.rows.unwrap_or(24); log::info!("Creating terminal with size {}x{}, profile: {}", cols, rows, shell_name); - // Resolve or generate a proper tb- prefixed tab ID. Computed BEFORE the spawn - // so the Terminal registers with it up front (review 062 F-01: patching - // tab_id in after spawn races a fast-exiting shell's exit-path persist). - let tab_id = match payload.tab_id.as_ref() { - Some(tid) if !tid.is_empty() => { - if tid.starts_with("tb-") { - tid.clone() - } else { - let raw_uuid = uuid::Uuid::new_v4().to_string().replace("-", ""); - format!("tb-{}", &raw_uuid[..9]) - } - } - _ => { - let raw_uuid = uuid::Uuid::new_v4().to_string().replace("-", ""); - format!("tb-{}", &raw_uuid[..9]) + // Resolve BOTH renderer identities BEFORE the spawn, so the Terminal + // registers with them up front (review 062 F-01: patching an id in after + // spawn returns races a fast-exiting shell's exit-path persist, which then + // files the final scrollback under the ephemeral pc- id). + let identity = match resolve_api_spawn_identity( + payload.tab_id.as_deref(), + payload.owning_tab_id.as_deref(), + payload.pane_id.as_deref(), + // D7 / review 095 B1: a create that lands in an already-occupied tab is a + // SPLIT even with no `paneId` (App.tsx Mode 2), so the leaf must be fresh. + // A terminal claims a tab either as its owner or — for a tab root, and for + // anything registered before P0-A — as its own renderer leaf. + // Read-only iteration that completes before the spawn: no shard guard is + // held across `spawn_terminal`, and nothing inside takes another lock. + |owner: &str| { + state.terminals.iter().any(|e| { + let t = e.value(); + t.owning_tab_id.as_deref() == Some(owner) + || t.renderer_terminal_id.as_deref() == Some(owner) + }) + }, + mint_renderer_id, + ) { + Ok(i) => i, + Err(e) => { + return (StatusCode::BAD_REQUEST, Json(json!({ "error": e }))).into_response() } }; @@ -613,7 +624,8 @@ async fn create_terminal( shell_cwd, shell_name.clone(), terminal_name.clone(), - Some(tab_id.clone()), + Some(identity.renderer_terminal_id.clone()), + Some(identity.owning_tab_id.clone()), None, // API-created terminal: fresh session, no restored scrollback ) { Ok(id) => { @@ -626,8 +638,15 @@ async fn create_terminal( if let Err(e) = state.app_handle.emit("api:createTerminalTab", serde_json::json!({ "name": terminal_name, "profile": shell_name, - "terminalId": id, // Pass the actual backend ID - "tabId": Some(tab_id.clone()), + // UNCHANGED: this key has always carried the backend PROCESS id + // here (unlike a REST response, where `terminalId` is the leaf). + // Mode 0 in App.tsx reads it as the process id. + "terminalId": id, + "tabId": Some(identity.owning_tab_id.clone()), + // NEW, unambiguous names — see App.tsx Modes 0/1. + "processId": id, + "rendererTerminalId": identity.renderer_terminal_id.clone(), + "owningTabId": identity.owning_tab_id.clone(), "paneId": payload.pane_id, "direction": payload.direction, "targetWindow": target_window @@ -3482,6 +3501,59 @@ mod tests { assert!(r.renderer_terminal_id.starts_with("tm-")); } + fn identity_temp_db() -> std::path::PathBuf { + use std::sync::atomic::{AtomicU32, Ordering}; + static N: AtomicU32 = AtomicU32::new(0); + let mut p = std::env::temp_dir(); + p.push(format!( + "termflow_identity_{}_{}.db", + std::process::id(), + N.fetch_add(1, Ordering::Relaxed) + )); + let _ = std::fs::remove_file(&p); + p + } + + /// Design 011 §7 test 2 — history isolation, end to end at the storage + /// layer. Two API splits in one tab must occupy two rows, and closing one + /// (`commands.rs:1028-1030` deletes by the renderer id) must leave the + /// other's scrollback intact. Before P0-A both ids were `tb-shared01`, so + /// the second upsert clobbered the first and the delete wiped both. + #[test] + fn two_api_splits_no_longer_share_one_history_row() { + let mut mint = counting_mint(); + // `no_live_terminals` (Task 4's helper): with a `pane_id` present the + // occupancy probe is not even needed to force distinct leaves. + let a = resolve_api_spawn_identity( + Some("tb-shared01"), None, Some("pn-a"), no_live_terminals, &mut mint, + ) + .expect("split a"); + let b = resolve_api_spawn_identity( + Some("tb-shared01"), None, Some("pn-b"), no_live_terminals, &mut mint, + ) + .expect("split b"); + + let store = crate::history_store::HistoryStore::new(); + store.init(&identity_temp_db()); + store.upsert(&a.renderer_terminal_id, &["pane A scrollback".to_string()], 1); + store.upsert(&b.renderer_terminal_id, &["pane B scrollback".to_string()], 2); + + assert_eq!( + store.get(&a.renderer_terminal_id), + Some(vec!["pane A scrollback".to_string()]), + "pane A's history must not be overwritten by pane B's flush" + ); + + // Closing pane A. + store.delete(&a.renderer_terminal_id); + assert_eq!(store.get(&a.renderer_terminal_id), None); + assert_eq!( + store.get(&b.renderer_terminal_id), + Some(vec!["pane B scrollback".to_string()]), + "closing one split must not delete the other's scrollback" + ); + } + #[test] fn the_release_and_dev_renderers_are_both_allowed() { assert!(origin_allowed(Some("http://tauri.localhost"), Some("127.0.0.1:42031"))); From 320c8d7ea6ef5a9ec5c53a1d9939a6a650fba928 Mon Sep 17 00:00:00 2001 From: Tam Tran Date: Fri, 14 Aug 2026 23:31:59 -0500 Subject: [PATCH 08/22] feat(sidecar): thread owning_tab_id through the PTY-host spawn path On the sidecar path the app terminalId IS the DashMap key, the reattach key, and the vt100 screen key -- ground-truth correction C2 confirmed four further maps (host_terminals, host_reattach_pending, host_stream_offsets, host_close_pending) key off that same value. P0-A must not disturb it, so the change here is purely additive: an owning_tab_id parameter threaded through create_terminal -> create_host_terminal -> register_host_terminal/ host_fallback, with the leaf/owner pair computed by one small helper (host_identity) shared by both terminal-registration call sites. A caller that sends no owner (a renderer that predates P0-A) degrades to the pre-P0-A behaviour: the pane owns itself. Task 11 (renderer) is what makes the owner always present going forward. --- src-tauri/src/commands.rs | 75 ++++++++++++++++++++++++++++++++++----- 1 file changed, 67 insertions(+), 8 deletions(-) diff --git a/src-tauri/src/commands.rs b/src-tauri/src/commands.rs index ee6477d..623074e 100644 --- a/src-tauri/src/commands.rs +++ b/src-tauri/src/commands.rs @@ -77,6 +77,9 @@ pub async fn create_terminal( profile_id: Option, cwd: Option, tab_id: Option, + // The tab that owns the pane `tab_id` names. Equal to `tab_id` for a + // root/solo pane. Optional so a renderer that predates P0-A still works. + owning_tab_id: Option, ) -> Result { let profiles = pty_manager::get_available_shells(); let mut shell_name = "default".to_string(); @@ -112,6 +115,7 @@ pub async fn create_terminal( return create_host_terminal( state.inner(), tid, + owning_tab_id.clone(), cols, rows, shell_path, @@ -144,6 +148,7 @@ pub async fn create_terminal( // it in after spawn returned raced a fast-exiting shell's exit persist, // which then filed history under the ephemeral pc- id (review 062 F-01). tab_id, + owning_tab_id, history_prefix.clone(), )?; @@ -194,6 +199,7 @@ pub fn adopt_console_window( async fn create_host_terminal( state: &AppState, id: String, + owning_tab_id: Option, cols: u16, rows: u16, shell_path: Option, @@ -204,13 +210,13 @@ async fn create_host_terminal( // Ensure the sidecar is up FIRST (single-flight). If unavailable, fall back // to the in-process path immediately — no host state is registered. if let Err(e) = state.ensure_pty_host().await { - return host_fallback(state, &id, cols, rows, shell_path, shell_name, shell_args, cwd, &e); + return host_fallback(state, &id, owning_tab_id.as_deref(), cols, rows, shell_path, shell_name, shell_args, cwd, &e); } let client = match state.pty_host_clone() { Some(c) => c, None => { return host_fallback( - state, &id, cols, rows, shell_path, shell_name, shell_args, cwd, + state, &id, owning_tab_id.as_deref(), cols, rows, shell_path, shell_name, shell_args, cwd, "pty-host not connected", ) } @@ -228,7 +234,7 @@ async fn create_host_terminal( // Restore the real pid, register routing BEFORE attach releases replay // bytes, then nudge a repaint so a live TUI redraws. if let Some((_, pid)) = state.host_reattach_pending.remove(&id) { - register_host_terminal(state, &id, pid, &shell_name, cols, rows, prompt_hook); + register_host_terminal(state, &id, owning_tab_id.as_deref(), pid, &shell_name, cols, rows, prompt_hook); // Backlog 011: this is the core-restart hot-swap reattach, which reconcile // (empty terminal list) could not seed. Stash the hook so the renderer can // re-arm the command-suggest prompt gate once createTerminal resolves. @@ -252,7 +258,7 @@ async fn create_host_terminal( // BEFORE spawning, so early output (shell banner / first prompt / OSC cwd) // has a registered screen to land in instead of being dropped by the // consumer's "unknown id" gate. - register_host_terminal(state, &id, 0, &shell_name, cols, rows, prompt_hook); + register_host_terminal(state, &id, owning_tab_id.as_deref(), 0, &shell_name, cols, rows, prompt_hook); // Seed + stage BEFORE the spawn so restored history precedes the shell's // first output in the parser. On spawn failure, cleanup_terminal_state // removes both the parser and the staged prefix; host_fallback restages. @@ -276,22 +282,35 @@ async fn create_host_terminal( Err(e) => { // Undo the provisional registration, then fall back in-process. state.cleanup_terminal_state(&id); - host_fallback(state, &id, cols, rows, shell_path, shell_name, shell_args, cwd, &e) + host_fallback(state, &id, owning_tab_id.as_deref(), cols, rows, shell_path, shell_name, shell_args, cwd, &e) } } } +/// The identities a sidecar-hosted terminal registers: `(map_key_and_leaf, owner)`. +/// +/// On this path the app terminalId IS the DashMap key, the sidecar session id, +/// the output-broadcast id and the vt100 screen key — that alignment is the +/// reattach contract (`commands.rs:188-192`) and P0-A leaves it untouched. +/// The only new thing is the owner, which defaults to the leaf (correct for a +/// root/solo pane, and the pre-P0-A behaviour for everything else). +fn host_identity(id: &str, owning_tab_id: Option<&str>) -> (String, String) { + (id.to_string(), owning_tab_id.unwrap_or(id).to_string()) +} + /// Register a host-owned terminal's routing state: authoritative screen, host /// ownership, and the Terminal record (keyed by the stable id == tab_id). fn register_host_terminal( state: &AppState, id: &str, + owning_tab_id: Option<&str>, pid: u32, shell_name: &str, cols: u16, rows: u16, prompt_hook: bool, ) { + let (leaf, owner) = host_identity(id, owning_tab_id); state.init_screen(id, rows, cols); state.host_terminals.insert(id.to_string(), ()); state.terminals.insert( @@ -305,8 +324,8 @@ fn register_host_terminal( cols, rows, backend: crate::tmux_manager::TerminalBackend::PortablePty, - renderer_terminal_id: Some(id.to_string()), - owning_tab_id: None, + renderer_terminal_id: Some(leaf), + owning_tab_id: Some(owner), last_input_source: None, last_input_at: None, prompt_hook, @@ -348,6 +367,7 @@ fn stage_scrollback(state: &AppState, history_key: &str, t fn host_fallback( state: &AppState, tab_id: &str, + owning_tab_id: Option<&str>, cols: u16, rows: u16, shell_path: Option, @@ -361,6 +381,9 @@ fn host_fallback( // Seed + register the tab_id via spawn_terminal (both land before the reader // thread starts), then stage the renderer's one-shot prefix under the new id. let history_prefix = restore_prefix(state, tab_id); + // Same rule as register_host_terminal: the leaf is the id, the owner + // defaults to the leaf. One definition, two call sites. + let (leaf, owner) = host_identity(tab_id, owning_tab_id); let fallback_id = pty_manager::spawn_terminal( state.clone(), cols, @@ -370,7 +393,8 @@ fn host_fallback( cwd, shell_name, name, - Some(tab_id.to_string()), + Some(leaf), + Some(owner), history_prefix.clone(), )?; if let Some(prefix) = history_prefix { @@ -2265,3 +2289,38 @@ mod freedesktop_icon_tests { let _ = fs::remove_dir_all(&root); } } + +#[cfg(test)] +mod host_identity_tests { + use super::host_identity; + + /// On the PTY-host sidecar path the DashMap KEY *is* the renderer leaf + /// (`commands.rs:188-192`), and `host_terminals` / `host_reattach_pending` / + /// `host_stream_offsets` / `host_close_pending` (`state.rs:235,240,260,270`) + /// are all keyed by that same value. P0-A must not move it — hot-swap + /// reattach depends on it (ground-truth correction C2). + #[test] + fn a_split_pane_keeps_its_leaf_as_the_map_and_reattach_key() { + let (key, owner) = host_identity("tm-9f2c1a4b7", Some("tb-4e8d0c2f1")); + assert_eq!(key, "tm-9f2c1a4b7", "the sidecar reattach key must stay the leaf"); + assert_eq!(owner, "tb-4e8d0c2f1"); + } + + /// A root/solo pane owns itself — the invariant that made the old collapse + /// invisible until an API split existed (design 011 §3). + #[test] + fn a_root_pane_owns_itself_when_no_owner_is_supplied() { + let (key, owner) = host_identity("tb-4e8d0c2f1", None); + assert_eq!(key, "tb-4e8d0c2f1"); + assert_eq!(owner, "tb-4e8d0c2f1"); + } + + /// A renderer that predates P0-A sends no owner; a split pane then falls + /// back to owning itself. That is the OLD behaviour, preserved — it is no + /// worse than today, and Task 10 makes the renderer always send one. + #[test] + fn a_missing_owner_degrades_to_the_leaf_not_to_none() { + let (_, owner) = host_identity("tm-9f2c1a4b7", None); + assert_eq!(owner, "tm-9f2c1a4b7"); + } +} From 9c63dc59db9f0d488cbc38b65f11df9d0d2bd482 Mon Sep 17 00:00:00 2001 From: Tam Tran Date: Fri, 14 Aug 2026 23:32:49 -0500 Subject: [PATCH 09/22] fix(fleet): register both renderer ids at spawn instead of patching after fleet_local_run spawned with tab_id: None, then patched entry.renderer_terminal_id in after the fact -- the exact patch-after-spawn race review 062 F-01 already closed for create_terminal: a fast-exiting shell's exit-path persist can run in that window and file the final scrollback under the ephemeral pc- id. Mints the tb- identity before the spawn and passes it as both renderer_terminal_id and owning_tab_id, matching create_terminal's pattern. fleet_terminals also gains terminalId/owningTabId so GET /api/fleet/ terminals has identity parity with the other terminal responses; the MCP list_terminals tool proxies this body verbatim. This is the last of the four spawn_terminal call sites Task 6 left broken; the crate compiles clean again and the full suite is green (286 tests). --- src-tauri/src/api_server.rs | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/src-tauri/src/api_server.rs b/src-tauri/src/api_server.rs index 54c63a0..34e9756 100644 --- a/src-tauri/src/api_server.rs +++ b/src-tauri/src/api_server.rs @@ -1501,6 +1501,10 @@ async fn fleet_terminals(State(state): State) -> impl IntoResponse { "machineId": machine_id, "os": os, "deviceName": device_name, + // Identity parity with the other terminal responses (design 011 + // §4). The MCP `list_terminals` tool proxies this body verbatim. + "terminalId": t.renderer_terminal_id, + "owningTabId": t.owning_tab_id, }) }) .collect(); @@ -2136,6 +2140,12 @@ async fn fleet_local_run( None => (None, None, None, "default".to_string()), }; let terminal_name = payload.label.clone().unwrap_or_else(|| "Fleet".to_string()); + // Mint the renderer identity BEFORE the spawn. Patching + // `entry.renderer_terminal_id` in afterwards is the exact pattern + // `pty_manager.rs:704-708` records as a fixed bug (review 062 F-01): + // a fast-exiting shell's exit-path persist can run in that window and + // file the final scrollback under the ephemeral pc- id. + let fleet_tab_id = mint_renderer_id("tb"); let new_id = match crate::pty_manager::spawn_terminal( state.clone(), 80, @@ -2145,7 +2155,8 @@ async fn fleet_local_run( shell_cwd, shell_name.clone(), terminal_name.clone(), - None, // tab_id: keep the pc- id default (tb- alias is cosmetic) + Some(fleet_tab_id.clone()), + Some(fleet_tab_id.clone()), None, // fleet terminal: fresh session, no restored scrollback ) { Ok(id) => id, @@ -2155,10 +2166,7 @@ async fn fleet_local_run( } }; // Make the fleet terminal VISIBLE as a labeled UI tab, mirroring - // create_terminal. The backend (`pc-`) id stays the map key; the - // `tb-` tab id is a cosmetic renderer alias. - let raw_uuid = uuid::Uuid::new_v4().to_string().replace('-', ""); - let tab_id = format!("tb-{}", &raw_uuid[..9]); + // create_terminal. The backend (`pc-`) id stays the map key. let target_window = state.resolve_active_window_label(); if let Err(e) = state.app_handle.emit( "api:createTerminalTab", @@ -2166,7 +2174,10 @@ async fn fleet_local_run( "name": terminal_name, "profile": shell_name, "terminalId": new_id, - "tabId": Some(tab_id.clone()), + "tabId": Some(fleet_tab_id.clone()), + "processId": new_id, + "rendererTerminalId": fleet_tab_id.clone(), + "owningTabId": fleet_tab_id.clone(), "paneId": serde_json::Value::Null, "direction": serde_json::Value::Null, "targetWindow": target_window, @@ -2174,9 +2185,6 @@ async fn fleet_local_run( ) { log::warn!("Failed to emit api:createTerminalTab for fleet terminal: {}", e); } - if let Some(mut entry) = state.terminals.get_mut(&new_id) { - entry.renderer_terminal_id = Some(tab_id); - } new_id } }; From 4ba5b6795bc7444f5e211d3054c5a8b0aacbe54c Mon Sep 17 00:00:00 2001 From: Tam Tran Date: Fri, 14 Aug 2026 23:34:15 -0500 Subject: [PATCH 10/22] fix(history): never key persisted scrollback by a PTY process id Ground-truth correction C1: before P0-A, Terminal.tab_id was never actually None at runtime (every write site wrapped it in Some(...)), so the "skip terminals with no renderer id" guard in persist_terminal_history was dead code that happened to look correct. Now that renderer_terminal_id can genuinely be None (a headless API/fleet spawn), and as defence in depth against ever reintroducing the old pc-* fallback, extracted the "is this a valid history key" decision into a pure history_key function: a pc-* process id -- regenerated on every spawn, so a row keyed by one is orphaned the moment the app restarts -- is never a valid key, same as None. --- src-tauri/src/state.rs | 58 +++++++++++++++++++++++++++++++++++++++--- 1 file changed, 55 insertions(+), 3 deletions(-) diff --git a/src-tauri/src/state.rs b/src-tauri/src/state.rs index 6702344..f88226b 100644 --- a/src-tauri/src/state.rs +++ b/src-tauri/src/state.rs @@ -352,6 +352,23 @@ impl Clone for AppState { } } +/// The key a terminal's scrollback is filed under in `terminal_history`, or +/// `None` to skip persistence entirely. +/// +/// Pure so the "a process id is never a history key" rule (design 011 §5) is +/// unit-testable without a live PTY or a Tauri `AppHandle` — inline +/// `#[cfg(test)]` only; the `integration-tests` feature breaks the Windows test +/// binary. +pub(crate) fn history_key(renderer_terminal_id: Option<&str>) -> Option<&str> { + match renderer_terminal_id { + // A `pc-` id is a PTY process id: it is regenerated on every spawn, so a + // row keyed by one is orphaned the moment the app restarts and can never + // be matched to a pane again. + Some(id) if id.starts_with("pc-") => None, + other => other, + } +} + impl AppState { pub fn new( output_tx: broadcast::Sender, @@ -633,7 +650,8 @@ impl AppState { Some(blob) } - /// Persist one terminal's RENDERED scrollback under its renderer id (tab_id). + /// Persist one terminal's RENDERED scrollback under its renderer leaf id + /// (`renderer_terminal_id` — `tb-*`/`tm-*`). /// Skips terminals that are gone or have no renderer id (e.g. API-created PTYs). /// /// We persist the authoritative vt100 parser's FULL buffer (scrollback + visible @@ -656,13 +674,17 @@ impl AppState { // since a dead terminal is never persisted again. let guard_arc = self.history_persist_guard(id); let _guard = guard_arc.lock().unwrap_or_else(|e| e.into_inner()); - let Some(tab_id) = self.terminals.get(id).and_then(|t| t.renderer_terminal_id.clone()) else { return }; + let renderer_id = self + .terminals + .get(id) + .and_then(|t| t.renderer_terminal_id.clone()); + let Some(key) = history_key(renderer_id.as_deref()) else { return }; // Skip when the parser is absent or the whole buffer is blank (brand-new or // already-cleared terminal) so we never persist a blank blob that would replay as // an empty "session restored" divider with nothing above it. let Some(snapshot) = self.full_scrollback_snapshot(id) else { return }; let blob = String::from_utf8_lossy(&snapshot).into_owned(); - self.history_store.upsert(&tab_id, std::slice::from_ref(&blob), now_ms); + self.history_store.upsert(key, std::slice::from_ref(&blob), now_ms); } /// The per-terminal persistence lock (see `history_persist_locks`). The Arc is @@ -1794,3 +1816,33 @@ mod terminal_identity_serde_tests { assert_eq!(back.owning_tab_id.as_deref(), Some("tb-4e8d0c2f1")); } } + +#[cfg(test)] +mod history_key_tests { + use super::history_key; + + #[test] + fn a_renderer_leaf_is_a_valid_history_key() { + assert_eq!(history_key(Some("tb-4e8d0c2f1")), Some("tb-4e8d0c2f1")); + assert_eq!(history_key(Some("tm-9f2c1a4b7")), Some("tm-9f2c1a4b7")); + } + + /// Ground-truth correction C1: before P0-A this could not happen — every + /// write site wrapped `Some(...)` and the `else { return }` guard at + /// state.rs:636 was dead code. A headless API/fleet PTY now genuinely has no + /// renderer id, and must simply not be persisted. + #[test] + fn no_renderer_id_means_no_history_row() { + assert_eq!(history_key(None), None); + } + + /// Defence in depth. `spawn_terminal`'s old `unwrap_or_else(|| id.clone())` + /// produced `Some("pc-…")`, which `persist_terminal_history` upserted like + /// any other key (state.rs:642) — a row keyed by an id that cannot survive a + /// restart, orphaned forever. Even if someone reintroduces that fallback, + /// the row must not be written. + #[test] + fn a_process_id_is_never_a_history_key() { + assert_eq!(history_key(Some("pc-abc123def")), None); + } +} From 775062dad33c1ba78c82cec879c439570af3c043 Mon Sep 17 00:00:00 2001 From: Tam Tran Date: Fri, 14 Aug 2026 23:51:13 -0500 Subject: [PATCH 11/22] feat(renderer): send the owning tab id when spawning a terminal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Design 011 §6 (review 086 Q6.3): assigning the Rust owning_tab_id field without this is a no-op for every UI-created terminal -- the renderer never sent an owner, so Task 7's backend parameter always saw None. TerminalService.createTerminal gains an optional 7th owningTabId parameter, threaded through the Tauri bridge and ElectronAPI type down to the create_terminal invoke (Tauri maps the camelCase JS key onto the Rust command's snake_case owning_tab_id parameter). TerminalPane resolves the owner via findTabIdByTerminalId (falling back to the pane's own terminalId for a root/solo pane) at both call sites: the first-spawn mount effect and handleRestart. --- src/renderer/api/tauri-bridge.ts | 5 ++- .../components/Panes/TerminalPane.tsx | 28 +++++++++++----- src/renderer/services/TerminalService.ts | 15 +++++++-- .../__tests__/TerminalService.test.ts | 33 +++++++++++++++++++ src/renderer/types/electron.d.ts | 2 +- 5 files changed, 71 insertions(+), 12 deletions(-) diff --git a/src/renderer/api/tauri-bridge.ts b/src/renderer/api/tauri-bridge.ts index e1f5645..16ef2ee 100644 --- a/src/renderer/api/tauri-bridge.ts +++ b/src/renderer/api/tauri-bridge.ts @@ -287,7 +287,7 @@ const tauriBridge: ElectronAPI = { }, // Terminal Operations - createTerminal: async (profile?: string, _name?: string, cwd?: string, tabId?: string, cols?: number, rows?: number) => { + createTerminal: async (profile?: string, _name?: string, cwd?: string, tabId?: string, cols?: number, rows?: number, owningTabId?: string) => { // We pass profile (id) to Rust, it resolves to path/args // We also pass cwd if provided; use fitted size when known, else fall back to 80×24 return invoke('create_terminal', { @@ -296,6 +296,9 @@ const tauriBridge: ElectronAPI = { profileId: profile, cwd, tabId, + // Tauri maps camelCase JS keys onto snake_case Rust parameters, so this + // reaches `create_terminal(… owning_tab_id: Option)`. + owningTabId, }); }, diff --git a/src/renderer/components/Panes/TerminalPane.tsx b/src/renderer/components/Panes/TerminalPane.tsx index 920acd0..813c25c 100644 --- a/src/renderer/components/Panes/TerminalPane.tsx +++ b/src/renderer/components/Panes/TerminalPane.tsx @@ -258,8 +258,16 @@ export const TerminalPane: React.FC = ({ const terminalName = name || tab?.title || 'Terminal'; console.log(`TerminalPane: Determining name - pane name: "${name}", tab title: "${tab?.title}", final: "${terminalName}"`); + // Ownership lives only in the pane tree, so resolve it here. A tab root's + // leaf id IS its tab id, so the `|| terminalId` fallback is correct for a + // solo pane and for a pane whose tree has not been committed yet. + const owningTabId = + findTabIdByTerminalId(store.getState().panes.treesByTabId, terminalId) || terminalId; + // Create the promise and store it immediately - const initPromise = terminalService.createTerminal(terminalId, finalShellType, terminalName, cwd); + const initPromise = terminalService.createTerminal( + terminalId, finalShellType, terminalName, cwd, undefined, undefined, owningTabId, + ); terminalInitPromises.set(terminalId, initPromise); terminalInitMap.set(terminalId, true); @@ -470,13 +478,23 @@ export const TerminalPane: React.FC = ({ // handled by the backend (pty_manager.rs is_dir()-checks the spawn cwd). const cwd = getCwdSnapshot(terminalId) ?? takeInitialCwd(terminalId) ?? profile?.cwd; const terminalName = name || tab?.title || 'Terminal'; + // A tab can be marked "exited" once every pane in its tree has exited + // (see App.tsx handleTerminalProcessExit / resolveExitedTabId), even for + // a non-root pane's terminalId — so resolve the owning tab rather than + // assuming terminalId === tab.id. Resolved up front so it can also be + // forwarded to the backend at spawn (design 011 §6). + const ownerTabId = + findTabIdByTerminalId(store.getState().panes.treesByTabId, terminalId) || terminalId; try { const newPid = await terminalService.createTerminal( terminalId, finalShellType, terminalName, - cwd + cwd, + undefined, + undefined, + ownerTabId, ); // The engine re-attaches to the new process when processId changes below. setProcessId(newPid); @@ -486,12 +504,6 @@ export const TerminalPane: React.FC = ({ clearCwdSnapshot(terminalId); // A restarted session is a fresh shell — return its zoom to 100%. dispatch(resetZoom(terminalId)); - // A tab can be marked "exited" once every pane in its tree has exited - // (see App.tsx handleTerminalProcessExit / resolveExitedTabId), even for - // a non-root pane's terminalId — so resolve the owning tab rather than - // assuming terminalId === tab.id. - const ownerTabId = - findTabIdByTerminalId(store.getState().panes.treesByTabId, terminalId) || terminalId; dispatch(clearTabExited(ownerTabId)); } catch (error) { console.error('TerminalPane: Failed to restart session:', error); diff --git a/src/renderer/services/TerminalService.ts b/src/renderer/services/TerminalService.ts index 7ec221d..032db24 100644 --- a/src/renderer/services/TerminalService.ts +++ b/src/renderer/services/TerminalService.ts @@ -85,7 +85,18 @@ class TerminalServiceClass { this.listenersInitialized = true; } - async createTerminal(terminalId: string, shellType: string = 'default', name?: string, cwd?: string, cols?: number, rows?: number): Promise { + async createTerminal( + terminalId: string, + shellType: string = 'default', + name?: string, + cwd?: string, + cols?: number, + rows?: number, + /** The tab that owns this pane. Equal to `terminalId` for a tab root; the + * owning `tb-` id for a split (`tm-`) pane. Design 011 §6: the backend + * cannot derive it — ownership lives only in `panes.treesByTabId`. */ + owningTabId?: string, + ): Promise { try { console.log(`TerminalService: Creating terminal ${terminalId} with shell type: "${shellType}", name: ${name}, cwd: ${cwd}`); @@ -103,7 +114,7 @@ class TerminalServiceClass { // Call IPC to create actual PTY process console.log(`TerminalService: Calling electronAPI.createTerminal with profileId: "${shellType}", cwd: "${cwd}", tabId: "${terminalId}"`); - const processId = await window.electronAPI.createTerminal(shellType, name, cwd, terminalId, cols, rows); + const processId = await window.electronAPI.createTerminal(shellType, name, cwd, terminalId, cols, rows, owningTabId); console.log(`TerminalService: Got process ID ${processId} for terminal ${terminalId} with shell type "${shellType}"`); // Store the mapping diff --git a/src/renderer/services/__tests__/TerminalService.test.ts b/src/renderer/services/__tests__/TerminalService.test.ts index 8193ed1..437c455 100644 --- a/src/renderer/services/__tests__/TerminalService.test.ts +++ b/src/renderer/services/__tests__/TerminalService.test.ts @@ -50,6 +50,39 @@ describe('TerminalService console-window adoption', () => { }); }); +describe('TerminalService.createTerminal owning-tab plumbing', () => { + let createTerminal: jest.Mock; + + beforeEach(() => { + createTerminal = jest.fn().mockResolvedValue('pc-owner-1'); + (window as any).electronAPI = { + createTerminal, + adoptConsoleWindow: jest.fn().mockResolvedValue(undefined), + }; + }); + + // Design 011 §6: the owner must reach the backend AT SPAWN. Without it the + // Rust owning_tab_id is null for every UI-created terminal and the + // split-pane activity fix cannot work. + it('forwards the owning tab id to the bridge', async () => { + await terminalService.createTerminal( + 'tm-owner-leaf', 'default', 'Terminal', undefined, 120, 40, 'tb-owner-tab', + ); + expect(createTerminal).toHaveBeenCalledWith( + 'default', 'Terminal', undefined, 'tm-owner-leaf', 120, 40, 'tb-owner-tab', + ); + }); + + // A root/solo pane owns itself; callers that pass nothing must still work + // (the backend treats `undefined` as "unknown" and falls back to the leaf). + it('omits the owner when the caller does not know one', async () => { + await terminalService.createTerminal('tb-solo-1'); + expect(createTerminal).toHaveBeenCalledWith( + 'default', undefined, undefined, 'tb-solo-1', undefined, undefined, undefined, + ); + }); +}); + describe('TerminalService.stashPromptGate (backlog 011 hot-swap reattach seed)', () => { it('stashes a gate that takePromptGateHandoff drains exactly once', () => { terminalService.stashPromptGate('tb-seed-1', { seen: true, armed: false }); diff --git a/src/renderer/types/electron.d.ts b/src/renderer/types/electron.d.ts index 34169a3..679560f 100644 --- a/src/renderer/types/electron.d.ts +++ b/src/renderer/types/electron.d.ts @@ -139,7 +139,7 @@ export interface ElectronAPI { getActiveProcesses?: () => Promise; // Terminal management - createTerminal: (profile?: string, name?: string, cwd?: string, tabId?: string, cols?: number, rows?: number) => Promise; + createTerminal: (profile?: string, name?: string, cwd?: string, tabId?: string, cols?: number, rows?: number, owningTabId?: string) => Promise; /** * Windows: make the calling window the owner of this shell's ConPTY * pseudo-console window, so console-app dialogs parented to From 12558c4798c451beebc80bf10e8cf4b34e17f1e1 Mon Sep 17 00:00:00 2001 From: Tam Tran Date: Fri, 14 Aug 2026 23:52:12 -0500 Subject: [PATCH 12/22] fix(renderer): light the owning tab when an API call hits a split pane Ground-truth correction C4: App.tsx's handleExternalActivity did `detail.tabId ?? null` and only fell back to findTabIdByTerminalId when that was falsy. For a split pane the backend always sent a TRUTHY tm- leaf as tabId, so the fallback never ran -- and even if it had, detail.terminalId is the PROCESS id (a pc-* value), which matches no pane-tree leaf on the in-process path either. flagTabActivity resolves its argument against state.tabs, which holds only root tab ids, so a split pane's activity indicator was silently dropped in every case. Extracted resolveActivityTabId as a pure module (the house pattern -- runningActivity.ts/RunningActivityTracker.ts, notificationLogic.ts/ NotificationService.ts) so the resolution order is testable without React/Redux: prefer the new explicit owningTabId, then walk the pane tree for a renderer leaf (rendererTerminalId, then the deprecated tabId alias), then fall back to the legacy terminalId path for events from a backend build that predates P0-A. Every candidate is checked against the live tab set so a closed tab can never resurrect an indicator. --- src/renderer/App.tsx | 15 +++-- src/renderer/api/tauri-bridge.ts | 2 +- .../__tests__/externalActivity.test.ts | 66 +++++++++++++++++++ src/renderer/services/externalActivity.ts | 57 ++++++++++++++++ 4 files changed, 133 insertions(+), 7 deletions(-) create mode 100644 src/renderer/services/__tests__/externalActivity.test.ts create mode 100644 src/renderer/services/externalActivity.ts diff --git a/src/renderer/App.tsx b/src/renderer/App.tsx index 9a44c4f..9c84097 100644 --- a/src/renderer/App.tsx +++ b/src/renderer/App.tsx @@ -45,7 +45,8 @@ import { refreshGlyphAtlases } from '@termflow/terminal-core'; import { addTab, markTabExited, flagTabActivity, setActiveTab } from './store/slices/tabsSlice'; import { RootState, store } from './store'; import { getCurrentWindow } from '@tauri-apps/api/window'; -import { findTabIdByTerminalId, getAllTerminalIds, resolveExitedTabId } from './store/slices/paneTreeOps'; +import { getAllTerminalIds, resolveExitedTabId } from './store/slices/paneTreeOps'; +import { resolveActivityTabId, type ExternalActivityDetail } from './services/externalActivity'; import { buildApiCreatedTab } from './services/apiCreatedTab'; import { runningActivityTracker } from './services/RunningActivityTracker'; import { notificationService } from './services/NotificationService'; @@ -670,11 +671,13 @@ const App: React.FC = () => { }; const handleExternalActivity = (event: CustomEvent) => { - const detail = (event.detail || {}) as { terminalId?: string; tabId?: string | null }; - let tabId: string | null = detail.tabId ?? null; - if (!tabId && detail.terminalId) { - tabId = findTabIdByTerminalId(store.getState().panes.treesByTabId, detail.terminalId); - } + const detail = (event.detail || {}) as ExternalActivityDetail; + const state = store.getState(); + const tabId = resolveActivityTabId( + detail, + state.panes.treesByTabId, + new Set(state.tabs.tabs.map(t => t.id)), + ); if (tabId) { dispatch(flagTabActivity({ tabId })); } diff --git a/src/renderer/api/tauri-bridge.ts b/src/renderer/api/tauri-bridge.ts index 16ef2ee..bf9ad8b 100644 --- a/src/renderer/api/tauri-bridge.ts +++ b/src/renderer/api/tauri-bridge.ts @@ -718,7 +718,7 @@ if (typeof window !== 'undefined') { // Flash the owning tab when an external MCP/API call interacts with a terminal. trackUnlisten(listen('terminal:external-activity', (event: any) => { window.dispatchEvent(new CustomEvent('terminal:external-activity', { - detail: event.payload, // { terminalId, tabId } + detail: event.payload, // { terminalId, processId, tabId, rendererTerminalId, owningTabId } })); })); diff --git a/src/renderer/services/__tests__/externalActivity.test.ts b/src/renderer/services/__tests__/externalActivity.test.ts new file mode 100644 index 0000000..d9f1437 --- /dev/null +++ b/src/renderer/services/__tests__/externalActivity.test.ts @@ -0,0 +1,66 @@ +import { resolveActivityTabId } from '../externalActivity'; + +// A tab with a root pane and one split leaf. +const trees = { + 'tb-4e8d0c2f1': { + id: 'pn-root', + type: 'split' as const, + direction: 'vertical' as const, + children: [ + { id: 'pn-a', type: 'terminal' as const, terminalId: 'tb-4e8d0c2f1' }, + { id: 'pn-b', type: 'terminal' as const, terminalId: 'tm-9f2c1a4b7' }, + ], + }, +} as any; +const knownTabIds = new Set(['tb-4e8d0c2f1']); + +describe('resolveActivityTabId', () => { + // The whole point of P0-A's activity fix. + it('uses owningTabId when the backend supplies one', () => { + expect( + resolveActivityTabId( + { owningTabId: 'tb-4e8d0c2f1', rendererTerminalId: 'tm-9f2c1a4b7' }, + trees, + knownTabIds, + ), + ).toBe('tb-4e8d0c2f1'); + }); + + // Correction C4: App.tsx:672-681 did `detail.tabId ?? null` and only fell back + // when that was falsy. For a split the backend sent a TRUTHY `tm-` leaf, so + // the fallback never ran and `flagTabActivity` silently no-opped against + // `state.tabs`, which holds only root tab ids. + it('never returns a leaf id that no tab owns', () => { + expect( + resolveActivityTabId({ tabId: 'tm-9f2c1a4b7' }, trees, knownTabIds), + ).toBe('tb-4e8d0c2f1'); + }); + + it('walks the pane tree when only a renderer leaf is available', () => { + expect( + resolveActivityTabId({ rendererTerminalId: 'tm-9f2c1a4b7' }, trees, knownTabIds), + ).toBe('tb-4e8d0c2f1'); + }); + + it('accepts a legacy tabId that IS a real tab', () => { + expect( + resolveActivityTabId({ tabId: 'tb-4e8d0c2f1' }, trees, knownTabIds), + ).toBe('tb-4e8d0c2f1'); + }); + + // Correction C4, second half: on the in-process path `terminalId` is the + // PROCESS id, which matches no pane-tree leaf — so it must never be treated + // as one. + it('does not mistake a process id for a leaf', () => { + expect( + resolveActivityTabId({ terminalId: 'pc-abc123def' }, trees, knownTabIds), + ).toBeNull(); + }); + + it('returns null rather than guessing when nothing resolves', () => { + expect(resolveActivityTabId({}, trees, knownTabIds)).toBeNull(); + expect( + resolveActivityTabId({ owningTabId: 'tb-closed99' }, trees, knownTabIds), + ).toBeNull(); + }); +}); diff --git a/src/renderer/services/externalActivity.ts b/src/renderer/services/externalActivity.ts new file mode 100644 index 0000000..429033f --- /dev/null +++ b/src/renderer/services/externalActivity.ts @@ -0,0 +1,57 @@ +/** + * Pure resolver for the `terminal:external-activity` event's target tab. + * + * The payload (api_server.rs `external_activity_payload`) carries up to four + * ids, and only one of them is a TAB: + * `terminalId` / `processId` — the backend PTY id (`pc-*` in-process). + * `tabId` / `rendererTerminalId` — the renderer LEAF (`tb-*` root, `tm-*` split). + * `owningTabId` — the tab. NEW in P0-A. + * + * `flagTabActivity` (tabsSlice.ts:133-141) resolves its argument against + * `state.tabs`, which holds ONLY root tab ids — handing it a `tm-*` leaf is a + * silent no-op, which is why every split pane's activity indicator was dropped. + * Mirrors RunningActivityTracker.resolveTab (:267-271), the pattern that already + * gets this right. + */ +import { findTabIdByTerminalId } from '../store/slices/paneTreeOps'; +import type { PaneNode } from '../store/slices/panesSlice'; + +export interface ExternalActivityDetail { + terminalId?: string; + processId?: string; + tabId?: string | null; + rendererTerminalId?: string | null; + owningTabId?: string | null; +} + +export function resolveActivityTabId( + detail: ExternalActivityDetail, + treesByTabId: Record, + knownTabIds: Set, +): string | null { + // 1. The backend told us the owner outright. Trust it only if the tab is + // still open — a closed tab must not resurrect an indicator. + if (detail.owningTabId && knownTabIds.has(detail.owningTabId)) { + return detail.owningTabId; + } + + // 2. Resolve a renderer leaf through the pane tree. `tabId` is a deprecated + // alias of the leaf, so it is a leaf candidate, NOT a tab candidate — + // except in the one case where it is genuinely a root tab id (below). + for (const leaf of [detail.rendererTerminalId, detail.tabId]) { + if (!leaf) continue; + const owner = findTabIdByTerminalId(treesByTabId, leaf); + if (owner && knownTabIds.has(owner)) return owner; + if (knownTabIds.has(leaf)) return leaf; + } + + // 3. Last resort: an event from a build that only sent the process id. This + // matches nothing on the in-process path (leaves are never `pc-*`), and is + // kept only for the sidecar path, where the map key IS the leaf. + if (detail.terminalId) { + const owner = findTabIdByTerminalId(treesByTabId, detail.terminalId); + if (owner && knownTabIds.has(owner)) return owner; + } + + return null; +} From 88f4911c56098393b8e07866b6820506e140be79 Mon Sep 17 00:00:00 2001 From: Tam Tran Date: Fri, 14 Aug 2026 23:53:26 -0500 Subject: [PATCH 13/22] fix(renderer): reconcile terminals by leaf id so splits are not reaped reconcileExistingTerminals grouped live PTYs by term.tabId to decide which duplicates to reap. Before the Task 4/5 backend fix, two API splits in one tab shared that value, so reconcile treated a live pane as a duplicate of its sibling and closed one of them outright. Extracted the grouping step as groupLiveTerminalsByLeaf, a pure module testable without fetch/localStorage/Redux, and switched the correlation key to term.terminalId -- the renderer LEAF -- explicitly not tabId, which api_server.rs's terminal_identity_json documents as a deprecated alias of the same field and could diverge from it in the future. The sort-newest-first step moves into the helper itself. Note: with the Task 2 backend response shape as actually landed, tabId and terminalId are wire-identical in every /api/terminals response (both copy renderer_terminal_id), so this change does not alter today's grouping output -- the reaping bug itself was already closed by Task 4/5 minting a distinct tm- leaf per split. This is defense-in-depth: it stops a future change to the deprecated tabId alias from silently reintroducing the collision here. --- src/renderer/services/StateManager.ts | 40 +++++-------- .../__tests__/reconcileTerminals.test.ts | 60 +++++++++++++++++++ src/renderer/services/reconcileTerminals.ts | 40 +++++++++++++ 3 files changed, 115 insertions(+), 25 deletions(-) create mode 100644 src/renderer/services/__tests__/reconcileTerminals.test.ts create mode 100644 src/renderer/services/reconcileTerminals.ts diff --git a/src/renderer/services/StateManager.ts b/src/renderer/services/StateManager.ts index 3de77ff..644b49b 100644 --- a/src/renderer/services/StateManager.ts +++ b/src/renderer/services/StateManager.ts @@ -8,6 +8,7 @@ import { restoreTabPanesInPlace } from './tabPanesStore'; import { generateId } from '../utils/id'; import { terminalService } from './TerminalService'; import { pruneCwds, seedRestoredCwds } from './stateManagerCwd'; +import { groupLiveTerminalsByLeaf } from './reconcileTerminals'; import { getAllCwdSnapshots } from './cwdSnapshot'; import { reattachPromptGate, markArmProbePending } from './reattachGate'; import { stateKey, layoutsKey, apiTokenKey, currentProfile, isForeignInstance } from './profileScope'; @@ -242,10 +243,12 @@ class StateManagerClass { /** * Reattach restored panes to PTYs that are still alive in the backend, instead * of spawning fresh ones (which orphans the survivors). The backend tags every - * terminal with the renderer terminalId that created it (its `tabId` field), so - * we can map each saved pane back to its live process. Best-effort: any failure - * (API unreachable, exposed-mode 401, prod mixed-content) is swallowed and the - * normal spawn path runs — no regression. + * terminal with the renderer terminalId that created it (its `terminalId` + * field — the `tb-*`/`tm-*` leaf; `tabId` is a deprecated alias and two splits + * in one tab share an `owningTabId`, so grouping by either would reap a live + * PTY), so we can map each saved pane back to its live process. Best-effort: + * any failure (API unreachable, exposed-mode 401, prod mixed-content) is + * swallowed and the normal spawn path runs — no regression. */ private async reconcileExistingTerminals(appState: AppState): Promise { try { @@ -299,26 +302,14 @@ class StateManagerClass { const list: any[] = Array.isArray(data) ? data : data?.terminals ?? []; - // Group every live PTY by the renderer id that spawned it (its `tabId`), - // restricted to ids the restore is about to recreate. We only consider these - // "wanted" ids so API-created terminals (mode "api", no UI tab) and other - // windows' terminals are never touched. - const byRenderer = new Map< - string, - Array<{ processId: string; createdAt: number; promptHook: unknown }> - >(); - for (const term of list) { - const rendererId: string | undefined = term?.tabId; // id that spawned it - const processId: string | undefined = term?.id ?? term?.processId; - if (!rendererId || !processId || !wanted.has(rendererId)) continue; - const createdAt = Date.parse(term?.createdAt ?? '') || 0; - const arr = byRenderer.get(rendererId) ?? []; - // promptHook re-arms command-suggest's prompt gate on reattach (see - // reattachPromptGate) — a reload wipes the in-memory gate, so without it - // an agent CLI running across the reload leaks input into the popup. - arr.push({ processId, createdAt, promptHook: term?.promptHook }); - byRenderer.set(rendererId, arr); - } + // Group every live PTY by the renderer LEAF that spawned it (its + // `terminalId` field — the `tb-*`/`tm-*` leaf; `tabId` is a deprecated + // alias and two splits in one tab share an `owningTabId`, so grouping by + // either would reap a live PTY), restricted to ids the restore is about + // to recreate. We only consider these "wanted" ids so API-created + // terminals (mode "api", no UI tab) and other windows' terminals are + // never touched. + const byRenderer = groupLiveTerminalsByLeaf(list, wanted); // Reattach to the NEWEST PTY per id, and REAP the older duplicates: a prior // reload that failed to reattach leaves several live PTYs sharing one tabId, @@ -326,7 +317,6 @@ class StateManagerClass { // self-heals the leak on the next load instead of letting orphans accumulate. const orphansToClose: string[] = []; for (const [rendererId, candidates] of byRenderer) { - candidates.sort((a, b) => b.createdAt - a.createdAt); // newest first const [keep, ...stale] = candidates; // Registers id→process AND seeds the init guards so the mount effect // reuses the live PTY (covers tab-root and split panes). The prompt-gate diff --git a/src/renderer/services/__tests__/reconcileTerminals.test.ts b/src/renderer/services/__tests__/reconcileTerminals.test.ts new file mode 100644 index 0000000..266a3c2 --- /dev/null +++ b/src/renderer/services/__tests__/reconcileTerminals.test.ts @@ -0,0 +1,60 @@ +import { groupLiveTerminalsByLeaf } from '../reconcileTerminals'; + +const wanted = new Set(['tb-4e8d0c2f1', 'tm-9f2c1a4b7']); + +describe('groupLiveTerminalsByLeaf', () => { + // THE REAPING REGRESSION (design 011 §7 test 6). Two API splits in one tab + // used to arrive with the SAME `tabId`, so reconcile grouped them together, + // kept the newest by createdAt and CLOSED the other — killing a live PTY. + // Correlating on the leaf keeps both. + it('keeps two splits that share an owning tab', () => { + const groups = groupLiveTerminalsByLeaf( + [ + { id: 'pc-aaa', terminalId: 'tb-4e8d0c2f1', owningTabId: 'tb-4e8d0c2f1', createdAt: '2026-08-14T10:00:00Z' }, + { id: 'pc-bbb', terminalId: 'tm-9f2c1a4b7', owningTabId: 'tb-4e8d0c2f1', createdAt: '2026-08-14T10:00:01Z' }, + ], + wanted, + ); + expect(groups.size).toBe(2); + expect(groups.get('tb-4e8d0c2f1')!.map(t => t.processId)).toEqual(['pc-aaa']); + expect(groups.get('tm-9f2c1a4b7')!.map(t => t.processId)).toEqual(['pc-bbb']); + }); + + // Genuine duplicates — a reload that failed to reattach leaves several PTYs on + // ONE leaf. Those must still group so the older ones are reaped. + it('still groups genuine duplicates of one leaf, newest first', () => { + const groups = groupLiveTerminalsByLeaf( + [ + { id: 'pc-old', terminalId: 'tb-4e8d0c2f1', createdAt: '2026-08-14T10:00:00Z' }, + { id: 'pc-new', terminalId: 'tb-4e8d0c2f1', createdAt: '2026-08-14T11:00:00Z' }, + ], + wanted, + ); + expect(groups.get('tb-4e8d0c2f1')!.map(t => t.processId)).toEqual(['pc-new', 'pc-old']); + }); + + it('ignores terminals the restore is not about to recreate', () => { + const groups = groupLiveTerminalsByLeaf( + [{ id: 'pc-other', terminalId: 'tb-someoneelse', createdAt: '' }], + wanted, + ); + expect(groups.size).toBe(0); + }); + + // A headless API/fleet PTY now reports `terminalId: null` (correction C1). + it('skips a terminal with no renderer identity', () => { + const groups = groupLiveTerminalsByLeaf( + [{ id: 'pc-headless', terminalId: null, owningTabId: null, createdAt: '' }], + wanted, + ); + expect(groups.size).toBe(0); + }); + + it('accepts processId when id is absent, and carries promptHook through', () => { + const groups = groupLiveTerminalsByLeaf( + [{ processId: 'pc-ccc', terminalId: 'tb-4e8d0c2f1', createdAt: '', promptHook: true }], + wanted, + ); + expect(groups.get('tb-4e8d0c2f1')![0]).toMatchObject({ processId: 'pc-ccc', promptHook: true }); + }); +}); diff --git a/src/renderer/services/reconcileTerminals.ts b/src/renderer/services/reconcileTerminals.ts new file mode 100644 index 0000000..0b89245 --- /dev/null +++ b/src/renderer/services/reconcileTerminals.ts @@ -0,0 +1,40 @@ +/** + * Pure grouping step of StateManager.reconcileExistingTerminals, extracted so + * the correlation key is testable without fetch/localStorage/Redux. + * + * The key is the renderer LEAF (`terminalId`), never `tabId` and never + * `owningTabId`. Two API-created splits in one tab legitimately share an owner; + * grouping by the owner made them look like duplicates, and the caller closes + * every candidate but the newest — reaping a live PTY (design 011 §1.1 item 1). + */ +export interface LiveTerminal { + processId: string; + createdAt: number; + promptHook: unknown; +} + +export function groupLiveTerminalsByLeaf( + list: any[], + wanted: Set, +): Map { + const byLeaf = new Map(); + for (const term of list ?? []) { + // `terminalId` is the leaf in every response shape (api_server.rs + // `terminal_identity_json`). `tabId` is a deprecated alias of the same + // value and is deliberately NOT read here, so a future redefinition of it + // cannot silently change which PTYs get reaped. + const leaf: string | undefined = term?.terminalId ?? undefined; + const processId: string | undefined = term?.id ?? term?.processId; + if (!leaf || !processId || !wanted.has(leaf)) continue; + const arr = byLeaf.get(leaf) ?? []; + arr.push({ + processId, + createdAt: Date.parse(term?.createdAt ?? '') || 0, + promptHook: term?.promptHook, + }); + byLeaf.set(leaf, arr); + } + // Newest first — the caller reattaches to [0] and reaps the rest. + for (const arr of byLeaf.values()) arr.sort((a, b) => b.createdAt - a.createdAt); + return byLeaf; +} From f3f63b04c50c87caa8338d28fe0acf7c045c69de Mon Sep 17 00:00:00 2001 From: Tam Tran Date: Fri, 14 Aug 2026 23:57:00 -0500 Subject: [PATCH 14/22] fix(renderer): bind an API split to its minted leaf, not to its process id handleAPICreateTerminalTab's Modes 1 and 2 both did registerExistingTerminal(terminalId, terminalId) with a pc-* value on both sides, then wrote that same process id into the pane tree as its terminalId -- binding the pane-tree leaf to a process id instead of a renderer identity. StateManager.sanitizeLayoutData later rewrites any non-tb-/tm- leaf to a fresh tm-* on the next restore, orphaning the binding this event set up and losing the pane's restored cwd. resolveApiCreateIds (apiCreatedTab.ts) disambiguates the event payload: Task 5's emit carries explicit processId/rendererTerminalId/owningTabId alongside the legacy terminalId/tabId pair (terminalId on THIS event has always been the process id, unlike a REST response where it's the leaf), falling back to the legacy keys for an event from a backend that predates P0-A. Modes 1 and 2 now register leaf -> process and seed the pane tree with the leaf, matching the identity Task 4/5's backend already minted for a split. Mode 0 is unaffected -- for a root create leafId === owningTabId === targetTabId, so its existing registerExistingTerminal(targetTabId, terminalId) stays correct. Renamed two call sites' local `process ${processId}` log/registration argument to the outer-scope `terminalId` (which already carries the resolved process id) rather than the freshly destructured `processId`, since both Mode 1 and Mode 2 separately re-declare a block-scoped `processId` later in the same block to hold the post-split terminal's own process id -- using the new binding there would have shadowed it. --- src/renderer/App.tsx | 56 +++++++++++++------ .../services/__tests__/apiCreatedTab.test.ts | 44 ++++++++++++++- src/renderer/services/apiCreatedTab.ts | 34 +++++++++++ 3 files changed, 116 insertions(+), 18 deletions(-) diff --git a/src/renderer/App.tsx b/src/renderer/App.tsx index 9c84097..8d72f77 100644 --- a/src/renderer/App.tsx +++ b/src/renderer/App.tsx @@ -47,7 +47,7 @@ import { RootState, store } from './store'; import { getCurrentWindow } from '@tauri-apps/api/window'; import { getAllTerminalIds, resolveExitedTabId } from './store/slices/paneTreeOps'; import { resolveActivityTabId, type ExternalActivityDetail } from './services/externalActivity'; -import { buildApiCreatedTab } from './services/apiCreatedTab'; +import { buildApiCreatedTab, resolveApiCreateIds } from './services/apiCreatedTab'; import { runningActivityTracker } from './services/RunningActivityTracker'; import { notificationService } from './services/NotificationService'; import { resolveActivation } from './services/notificationRouting'; @@ -891,10 +891,19 @@ const App: React.FC = () => { const handleAPICreateTerminalTab = async (event: CustomEvent) => { try { - const options = event.detail as { name: string; profile: string; tabId?: string; paneId?: string; direction?: 'horizontal' | 'vertical'; terminalId?: string }; + const options = event.detail as { + name: string; profile: string; tabId?: string; paneId?: string; + direction?: 'horizontal' | 'vertical'; terminalId?: string; + // P0-A (Task 5's emit): the unambiguous ids. `terminalId`/`tabId` above + // are the legacy pair this event has always carried. + processId?: string; rendererTerminalId?: string; owningTabId?: string; + }; console.log('API: Creating terminal tab', options); - const { name, profile, tabId, paneId, direction, terminalId } = options; + const { name, profile, paneId, direction } = options; + const { processId, leafId, owningTabId } = resolveApiCreateIds(options); + const tabId = owningTabId; + const terminalId = processId; // `store` is the file-scoped Redux store import (the same instance as // window.__REDUX_STORE__) — always defined, so no local alias/shadow. @@ -964,14 +973,23 @@ const App: React.FC = () => { if (tabId && paneId) { // Mode 1: Split an existing pane in a specific tab - console.log(`API: Splitting pane ${paneId} in tab ${tabId} with existing terminalId: ${terminalId}`); - - // Register terminalId with terminalService first if we have one - if (terminalId) { + // `terminalId` here is the resolved backend PROCESS id (see the + // resolveApiCreateIds destructure above) — used, not the block-scoped + // `processId` this branch later re-derives from the split's own pane + // tree, to avoid shadowing it. + console.log(`API: Splitting pane ${paneId} in tab ${tabId} with leaf ${leafId} / process ${terminalId}`); + + // Bind the backend PTY to the LEAF id, not to itself. Before P0-A this + // was `registerExistingTerminal(terminalId, terminalId)` with a `pc-*` + // value on both sides, so the pane tree carried a process id as its + // leaf — which StateManager.sanitizeLayoutData then rewrote to a fresh + // `tm-*` on the next restore, orphaning the binding (design 011 §6). + // TerminalPane's mount effect checks `getProcessId(terminalId)` FIRST + // (TerminalPane.tsx:174-176), so this binding is what stops it spawning + // a second PTY on top of the API-created one. + if (leafId && terminalId) { const terminalService = (window as any).terminalService; - if (terminalService) { - terminalService.registerExistingTerminal(terminalId, terminalId); - } + terminalService?.registerExistingTerminal(leafId, terminalId); } // Get the pane tree before the split to find existing terminal IDs. @@ -1000,7 +1018,7 @@ const App: React.FC = () => { direction: direction || 'vertical', // Default to vertical if not specified shellType: profile || defaultProfile || 'cmd', name: name, - terminalId: terminalId // Reuse the existing backend process + terminalId: leafId, // the tm- leaf the backend minted })); // The new terminal will be created automatically by TerminalPane @@ -1098,10 +1116,14 @@ const App: React.FC = () => { } // The backend already spawned the process; register it so the new pane - // reuses it (identity map: pane terminalId === backend processId) instead - // of spawning a second, orphaned terminal. - if (terminalId) { - (window as any).terminalService?.registerExistingTerminal(terminalId, terminalId); + // reuses it (identity map: pane terminalId === LEAF, bound to the + // backend process) instead of spawning a second, orphaned terminal. + // `terminalId` here is the resolved process id (see the + // resolveApiCreateIds destructure above), used rather than the + // block-scoped `processId` this branch later re-derives, to avoid + // shadowing it. + if (leafId && terminalId) { + (window as any).terminalService?.registerExistingTerminal(leafId, terminalId); } // Inspect THIS tab's tree from the authoritative per-tab store (not the @@ -1132,7 +1154,7 @@ const App: React.FC = () => { // Seed a single terminal in THIS tab without activating it. const { splitPaneInTab } = await import('./store/slices/panesSlice'); - const newTerminalId = terminalId || generateId('tm'); + const newTerminalId = leafId || generateId('tm'); dispatch(splitPaneInTab({ tabId: tabId, direction: direction || 'vertical', @@ -1334,7 +1356,7 @@ const App: React.FC = () => { direction: autoDirection, shellType: profile || defaultProfile || 'cmd', name: name, - terminalId: terminalId // Reuse the backend-created process (avoids orphaning it) + terminalId: leafId // the leaf the backend minted for this pane })); console.log('API: Dispatched splitPaneInTab action'); diff --git a/src/renderer/services/__tests__/apiCreatedTab.test.ts b/src/renderer/services/__tests__/apiCreatedTab.test.ts index 2bc7540..89d175d 100644 --- a/src/renderer/services/__tests__/apiCreatedTab.test.ts +++ b/src/renderer/services/__tests__/apiCreatedTab.test.ts @@ -1,4 +1,4 @@ -import { buildApiCreatedTab } from '../apiCreatedTab'; +import { buildApiCreatedTab, resolveApiCreateIds } from '../apiCreatedTab'; describe('buildApiCreatedTab', () => { it('pins the title (titleIsCustom: true) when the caller supplies a name', () => { @@ -59,3 +59,45 @@ describe('buildApiCreatedTab', () => { }); }); }); + +describe('resolveApiCreateIds', () => { + it('reads the explicit P0-A keys for a split', () => { + expect( + resolveApiCreateIds({ + terminalId: 'pc-abc123def', + tabId: 'tb-4e8d0c2f1', + processId: 'pc-abc123def', + rendererTerminalId: 'tm-9f2c1a4b7', + owningTabId: 'tb-4e8d0c2f1', + }), + ).toEqual({ + processId: 'pc-abc123def', + leafId: 'tm-9f2c1a4b7', + owningTabId: 'tb-4e8d0c2f1', + }); + }); + + it('gives a root create the same leaf and owner', () => { + expect( + resolveApiCreateIds({ + processId: 'pc-root1', + rendererTerminalId: 'tb-4e8d0c2f1', + owningTabId: 'tb-4e8d0c2f1', + }), + ).toEqual({ processId: 'pc-root1', leafId: 'tb-4e8d0c2f1', owningTabId: 'tb-4e8d0c2f1' }); + }); + + // A payload from a build that predates P0-A: `terminalId` was the process id + // and `tabId` the owning tab, with no leaf at all. + it('falls back to the legacy keys', () => { + expect( + resolveApiCreateIds({ terminalId: 'pc-legacy', tabId: 'tb-legacy1' }), + ).toEqual({ processId: 'pc-legacy', leafId: 'tb-legacy1', owningTabId: 'tb-legacy1' }); + }); + + it('reports missing ids as undefined rather than inventing them', () => { + expect(resolveApiCreateIds({})).toEqual({ + processId: undefined, leafId: undefined, owningTabId: undefined, + }); + }); +}); diff --git a/src/renderer/services/apiCreatedTab.ts b/src/renderer/services/apiCreatedTab.ts index d306e38..0a6bd6d 100644 --- a/src/renderer/services/apiCreatedTab.ts +++ b/src/renderer/services/apiCreatedTab.ts @@ -49,3 +49,37 @@ export function buildApiCreatedTab(options: ApiCreatedTabOptions): ApiCreatedTab return tab; } + +/** The three ids an `api:createTerminalTab` event can carry. */ +export interface ApiCreateIds { + /** Backend PTY id (`pc-*`) to bind the pane to. */ + processId?: string; + /** Renderer pane-tree leaf (`tb-*` root, `tm-*` split). */ + leafId?: string; + /** The tab that owns the leaf (`tb-*`). */ + owningTabId?: string; +} + +/** + * Disambiguate the event payload. `terminalId` on THIS event has always been the + * backend PROCESS id (api_server.rs `create_terminal` emit), unlike a REST + * response where it is the leaf — which is exactly why P0-A added the explicit + * `processId` / `rendererTerminalId` / `owningTabId` keys. The legacy keys are + * still read so an event from an older backend keeps working. + */ +export function resolveApiCreateIds(detail: { + terminalId?: string; + tabId?: string; + processId?: string; + rendererTerminalId?: string; + owningTabId?: string; +}): ApiCreateIds { + const owningTabId = detail.owningTabId ?? detail.tabId; + return { + processId: detail.processId ?? detail.terminalId, + // Before P0-A no leaf was sent; the owning tab was the only renderer id + // available, and it IS the leaf for a root pane. + leafId: detail.rendererTerminalId ?? owningTabId, + owningTabId, + }; +} From 0069e0289cbdcb2b932f79590f4fcbb324d08260 Mon Sep 17 00:00:00 2001 From: Tam Tran Date: Fri, 14 Aug 2026 23:59:13 -0500 Subject: [PATCH 15/22] fix(renderer): remap terminalCwds when sanitisation rewrites a leaf id MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Design 011 §6 (review 086 Q4). sanitizeLayoutData rewrites a non-tb-/tm- split leaf to a fresh tm-* id, but terminalCwds is keyed separately and was never remapped -- the saved directory stayed filed under the OLD id and the restored pane silently lost its cwd, starting in the profile default instead. Task 14 makes this reachable more often, since an API split used to persist a pc-* leaf. remapCwds (stateManagerCwd.ts) re-keys the saved directories using the id-rewrite mapping sanitizeNode now records; an entry that already exists under the new id is treated as fresher and wins. sanitizeNode runs TWICE over the same logical tree -- once in the tabPanes loop (whose output restoreTabPanesInPlace actually restores) and once standalone over paneTree (whose output restoreState never dispatches). For a legacy leaf needing regeneration, each pass calls generateId('tm') independently and gets a different id; without a guard the second (unused) id would overwrite the first (real) one in terminalIdMap, and remapCwds would then re-key the cwd onto an id no restored pane carries. The guard keeps the FIRST mapping, since the tabPanes pass runs first. Added a dedicated end-to-end test against sanitizeLayoutData itself (StateManager.sanitizeLayoutData.test.ts) -- the pure remapCwds unit tests can't see this double-pass collision. --- src/renderer/services/StateManager.ts | 34 ++++++++++--- .../StateManager.sanitizeLayoutData.test.ts | 50 +++++++++++++++++++ .../__tests__/stateManagerCwd.test.ts | 29 ++++++++++- src/renderer/services/stateManagerCwd.ts | 19 +++++++ 4 files changed, 124 insertions(+), 8 deletions(-) create mode 100644 src/renderer/services/__tests__/StateManager.sanitizeLayoutData.test.ts diff --git a/src/renderer/services/StateManager.ts b/src/renderer/services/StateManager.ts index 644b49b..741e419 100644 --- a/src/renderer/services/StateManager.ts +++ b/src/renderer/services/StateManager.ts @@ -7,7 +7,7 @@ import { clearTabPanes } from '../components/TerminalContainer'; import { restoreTabPanesInPlace } from './tabPanesStore'; import { generateId } from '../utils/id'; import { terminalService } from './TerminalService'; -import { pruneCwds, seedRestoredCwds } from './stateManagerCwd'; +import { pruneCwds, seedRestoredCwds, remapCwds } from './stateManagerCwd'; import { groupLiveTerminalsByLeaf } from './reconcileTerminals'; import { getAllCwdSnapshots } from './cwdSnapshot'; import { reattachPromptGate, markArmProbePending } from './reattachGate'; @@ -626,15 +626,17 @@ class StateManagerClass { /** * Helper to sanitize state and layouts to ensure they use correct prefixed IDs and avoid GUIDs. */ - private sanitizeLayoutData(data: T): T { const tabIdMap = new Map(); const paneIdMap = new Map(); + const terminalIdMap = new Map(); // 1. Map old tab IDs to new tab IDs const sanitizedTabs = (data.tabs || []).map(tab => { @@ -684,6 +686,7 @@ class StateManagerClass { if (newNode.type === 'terminal') { if (newNode.terminalId) { + const oldTerminalId = newNode.terminalId; // If it was matching the old tab ID (main terminal of that tab) if (tabIdMap.has(newNode.terminalId)) { newNode.terminalId = tabIdMap.get(newNode.terminalId)!; @@ -693,6 +696,22 @@ class StateManagerClass { // Split terminal ID that is not tb- or tm- newNode.terminalId = generateId('tm'); } + // GUARD (blast-radius review 092 B1): `sanitizeNode` runs TWICE over + // the same logical tree — once inside the `tabPanes` loop below + // (whose output IS what `restoreTabPanesInPlace` actually restores) + // and once standalone over `paneTree` (whose output `restoreState` + // never dispatches — no `setPaneTree` call exists there). For a + // legacy leaf id that needs regeneration, each pass independently + // calls the non-deterministic `generateId('tm')` and gets a + // DIFFERENT id. The tabPanes pass runs first (it is physically + // earlier in this function), so its id is the one that ends up on + // screen — the FIRST mapping must therefore win. Without this guard + // the second (discarded) id silently overwrites the first, and + // `remapCwds` re-keys the cwd onto an id no restored pane carries — + // reproducing the exact bug this task exists to fix. + if (newNode.terminalId !== oldTerminalId && !terminalIdMap.has(oldTerminalId)) { + terminalIdMap.set(oldTerminalId, newNode.terminalId); + } } } else if (newNode.type === 'split' && newNode.children) { newNode.children = newNode.children.map((child: any) => sanitizeNode(child, tabId)); @@ -725,6 +744,7 @@ class StateManagerClass { activeTabId: sanitizedActiveTabId, paneTree: sanitizedPaneTree, activePaneId: sanitizedActivePaneId, + terminalCwds: remapCwds(data.terminalCwds || {}, terminalIdMap), }; if (data.tabPanes) { diff --git a/src/renderer/services/__tests__/StateManager.sanitizeLayoutData.test.ts b/src/renderer/services/__tests__/StateManager.sanitizeLayoutData.test.ts new file mode 100644 index 0000000..59c04c3 --- /dev/null +++ b/src/renderer/services/__tests__/StateManager.sanitizeLayoutData.test.ts @@ -0,0 +1,50 @@ +/** + * @jest-environment jsdom + * + * Blast-radius review 092 B1. `sanitizeLayoutData` calls `sanitizeNode` TWICE + * over the same logical tree -- once inside the `tabPanes` loop (whose output + * IS what `restoreTabPanesInPlace` actually restores), once standalone over + * `paneTree` (whose output `restoreState` never dispatches). For a legacy leaf + * id, each pass independently mints a DIFFERENT fresh `tm-*` id; without a + * guard on `terminalIdMap`, whichever pass runs second overwrites the first + * pass's (real, restored) mapping with its own throwaway one, and `remapCwds` + * then re-keys the cwd onto an id no restored pane carries. This exercises + * `sanitizeLayoutData` itself -- the pure `remapCwds` unit tests cannot see + * this double-`sanitizeNode` collision. + */ +jest.mock('../../components/TerminalContainer', () => ({ clearTabPanes: jest.fn() })); +jest.mock('../../utils/id', () => { + let n = 0; + // Deterministic but DISTINCT per call — mirrors what real (random) + // `generateId` also does across the two independent `sanitizeNode` passes, + // without relying on randomness to prove the point. + return { generateId: jest.fn((prefix: string) => `${prefix}-regen${++n}`) }; +}); + +import { StateManager } from '../StateManager'; + +describe('sanitizeLayoutData (end-to-end, review 092 B1)', () => { + it('keeps terminalCwds keyed by the id the RESTORED pane tree actually carries', () => { + // A pre-P0-A collided leaf: neither tb- nor tm- prefixed, so sanitizeNode + // regenerates it on both passes. + const legacyLeaf = { type: 'terminal', id: 'pn-existing1', terminalId: 'legacy-collided-id' }; + const raw = { + tabs: [{ id: 'tb-mytab001' }], + activeTabId: 'tb-mytab001', + activePaneId: 'pn-existing1', + // Separate object copies, as they are after a JSON round-trip through + // localStorage — same logical tree, exactly as saveState persists it. + paneTree: JSON.parse(JSON.stringify(legacyLeaf)), + tabPanes: { 'tb-mytab001': JSON.parse(JSON.stringify(legacyLeaf)) }, + terminalCwds: { 'legacy-collided-id': 'D:\\work' }, + }; + + const result = (StateManager as any).sanitizeLayoutData(raw); + + // Whatever id restoreTabPanesInPlace will actually restore the pane + // under — NOT the throwaway id the standalone paneTree pass produced. + const restoredId = result.tabPanes['tb-mytab001'].terminalId; + expect(Object.keys(result.terminalCwds)).toEqual([restoredId]); + expect(result.terminalCwds[restoredId]).toBe('D:\\work'); + }); +}); diff --git a/src/renderer/services/__tests__/stateManagerCwd.test.ts b/src/renderer/services/__tests__/stateManagerCwd.test.ts index bc58de6..72d173b 100644 --- a/src/renderer/services/__tests__/stateManagerCwd.test.ts +++ b/src/renderer/services/__tests__/stateManagerCwd.test.ts @@ -3,7 +3,7 @@ * The map is persisted alongside the pane trees and seeded back before any pane * spawns. Legacy saved state (no terminalCwds key) must still load. */ -import { pruneCwds, seedRestoredCwds } from '../stateManagerCwd'; +import { pruneCwds, seedRestoredCwds, remapCwds } from '../stateManagerCwd'; import { getCwdSnapshot, setCwdSnapshot, getAllCwdSnapshots, __resetCwdSnapshots } from '../cwdSnapshot'; jest.mock('../TerminalService', () => ({ terminalService: { getProcessId: () => undefined } })); @@ -49,3 +49,30 @@ describe('seedRestoredCwds', () => { expect(getCwdSnapshot('tm-1')).toBe('D:\\fresh'); }); }); + +describe('remapCwds', () => { + // Review 086 Q4: sanitizeLayoutData rewrites a stale split leaf to a fresh + // `tm-` id. Without the same remap, `terminalCwds` still points at the OLD id + // and the restored pane silently loses its directory. + it('moves a cwd onto the id its leaf was rewritten to', () => { + expect( + remapCwds( + { 'pc-abc123def': 'D:\\work', 'tb-4e8d0c2f1': 'D:\\other' }, + new Map([['pc-abc123def', 'tm-9f2c1a4b7']]), + ), + ).toEqual({ 'tm-9f2c1a4b7': 'D:\\work', 'tb-4e8d0c2f1': 'D:\\other' }); + }); + + it('leaves an unmapped map untouched', () => { + expect(remapCwds({ 'tb-1': '/a' }, new Map())).toEqual({ 'tb-1': '/a' }); + }); + + it('lets an existing entry for the NEW id win over a remapped one', () => { + expect( + remapCwds( + { 'pc-old': '/stale', 'tm-new': '/fresh' }, + new Map([['pc-old', 'tm-new']]), + ), + ).toEqual({ 'tm-new': '/fresh' }); + }); +}); diff --git a/src/renderer/services/stateManagerCwd.ts b/src/renderer/services/stateManagerCwd.ts index d76e08f..ae50681 100644 --- a/src/renderer/services/stateManagerCwd.ts +++ b/src/renderer/services/stateManagerCwd.ts @@ -23,3 +23,22 @@ export function seedRestoredCwds(saved: Record | undefined): voi } } } + +/** Re-key saved directories when sanitisation rewrites a pane's terminal id. + * Without this the cwd is orphaned under the old id and the restored pane + * starts in the profile default instead of where the user left it + * (design 011 §6, review 086 Q4). An entry that already exists under the NEW + * id is fresher and wins. */ +export function remapCwds( + all: Record, + mapping: Map, +): Record { + if (mapping.size === 0) return { ...all }; + const out: Record = {}; + for (const [terminalId, cwd] of Object.entries(all)) { + const target = mapping.get(terminalId) ?? terminalId; + if (mapping.has(terminalId) && Object.prototype.hasOwnProperty.call(all, target)) continue; + out[target] = cwd; + } + return out; +} From a3aa836811233252d2ed8c82c72fc5fe667e8667 Mon Sep 17 00:00:00 2001 From: Tam Tran Date: Sat, 15 Aug 2026 00:01:45 -0500 Subject: [PATCH 16/22] docs(mcp): describe the three terminal id spaces and expose owningTabId --- mcp-server/src/server.ts | 33 ++++++++++++++++++++-------- mcp-server/test/server.test.ts | 39 ++++++++++++++++++++++++++++++++++ 2 files changed, 63 insertions(+), 9 deletions(-) diff --git a/mcp-server/src/server.ts b/mcp-server/src/server.ts index a4bc7d4..c07b8b2 100644 --- a/mcp-server/src/server.ts +++ b/mcp-server/src/server.ts @@ -35,7 +35,7 @@ export function createMcpServer({ api, getCallerId }: McpServerDeps): McpServer server.registerTool( "list_terminals", { - description: "List active terminal sessions across the fleet. Each entry includes machineId, os, and deviceName; local terminals are tagged with this machine.", + description: "List active terminal sessions across the fleet. Each entry includes machineId, os, and deviceName; local terminals are tagged with this machine. Each entry also carries `terminalId` (the renderer pane) and `owningTabId` (its tab).", }, async () => { try { @@ -56,19 +56,31 @@ export function createMcpServer({ api, getCallerId }: McpServerDeps): McpServer server.registerTool( "create_terminal", { - description: "Spawn a new terminal process (supports split panel layout)", + description: + "Spawn a new terminal process (supports split panel layout). Terminal ids come " + + "in three flavours and are NOT interchangeable: `terminalId`/`processId` (`pc-…`) " + + "addresses the PTY for every other tool; `owningTabId` (`tb-…`) names a TAB; and " + + "the `terminalId` field of a terminal-detail response is the renderer PANE " + + "(`tb-…` for a solo pane, `tm-…` for a split).", inputSchema: { name: z.string().optional().describe("Name of the terminal session"), profile: z.string().optional().describe("Shell profile ID (e.g., 'powershell', 'cmd', 'git-bash'). Defaults to system default."), cols: z.number().optional().default(120), rows: z.number().optional().default(40), cwd: z.string().optional().describe("Current working directory"), - tabId: z.string().optional().describe("Tab ID where the terminal pane should be created/split"), + owningTabId: z.string().optional().describe( + "The TAB (`tb-…`) the new pane should belong to — read it from " + + "get_terminal_detail's `owningTabId`. Preferred over `tabId`." + ), + tabId: z.string().optional().describe( + "DEPRECATED alias of owningTabId. Must be a TAB id (`tb-…`); passing a " + + "pane id (`tm-…`) is rejected with 400 — use owningTabId instead." + ), paneId: z.string().optional().describe("Pane ID within the tab to split"), direction: z.enum(["horizontal", "vertical"]).optional().describe("Split direction: 'horizontal' (split right) or 'vertical' (split bottom)"), }, }, - async ({ name, profile, cols, rows, cwd, tabId, paneId, direction }) => { + async ({ name, profile, cols, rows, cwd, owningTabId, tabId, paneId, direction }) => { try { const response = await api.post(`/terminals`, { name, @@ -76,6 +88,7 @@ export function createMcpServer({ api, getCallerId }: McpServerDeps): McpServer cols, rows, cwd, + owningTabId, tabId, paneId, direction, @@ -223,7 +236,7 @@ export function createMcpServer({ api, getCallerId }: McpServerDeps): McpServer server.registerTool( "get_terminal_detail", { - description: "Get detailed information about a specific terminal session (including its tabId)", + description: "Get detailed information about a specific terminal session, including its renderer pane id (`terminalId`) and the tab that owns it (`owningTabId`). `tabId` is a deprecated alias of `terminalId` and is NOT a tab id for a split pane.", inputSchema: { terminalId: z.string().describe(`The ID of the terminal session to retrieve. ${ME_HINT}`), }, @@ -245,10 +258,12 @@ export function createMcpServer({ api, getCallerId }: McpServerDeps): McpServer "get_my_terminal", { description: - "Get YOUR OWN terminal's identity and details (id, pid, tabId, name) — the terminal " + - "this agent is running in. Resolved from the X-Termflow-Terminal-Id header (mapped " + - "from the $TERMFLOW_TERMINAL_ID env var injected into every terminal). Use the returned " + - 'id, or the "me" shorthand, to target your own terminal with the other tools.', + "Get YOUR OWN terminal's identity and details (id, pid, terminalId, owningTabId, name) " + + "— the terminal this agent is running in. Resolved from the X-Termflow-Terminal-Id " + + "header (mapped from the $TERMFLOW_TERMINAL_ID env var injected into every terminal). " + + "The response carries `terminalId` (this pane) and `owningTabId` (the tab it lives in); " + + "pass the latter to create_terminal to open a sibling pane in the same tab. Use the " + + 'returned id, or the "me" shorthand, to target your own terminal with the other tools.', }, async () => { try { diff --git a/mcp-server/test/server.test.ts b/mcp-server/test/server.test.ts index b7fd935..8b99df2 100644 --- a/mcp-server/test/server.test.ts +++ b/mcp-server/test/server.test.ts @@ -250,6 +250,45 @@ describe("list_machines tool", () => { }); }); +describe("terminal identity is described unambiguously to AI clients", () => { + async function toolMap(client: Client) { + const { tools } = await client.listTools(); + return new Map(tools.map((t: any) => [t.name, t])); + } + + // Ground-truth correction C5: `tabId` was described as "Tab ID where the + // terminal pane should be created/split", but for a split the backend + // returns a `tm-` LEAF under that key — so an agent round-tripping it + // created the pane in the wrong tab (and, before P0-A, silently got a brand + // new unrelated tab). + it("create_terminal steers the caller to owningTabId", async () => { + const { api } = makeFakeApi(); + const client = await connectClient(createMcpServer({ api, getCallerId: () => "pc-self" })); + const tool: any = (await toolMap(client)).get("create_terminal"); + const described = JSON.stringify(tool.inputSchema); + expect(described).toContain("owningTabId"); + }); + + it("create_terminal forwards owningTabId to the backend", async () => { + const { api, calls } = makeFakeApi(); + const client = await connectClient(createMcpServer({ api, getCallerId: () => "pc-self" })); + await client.callTool({ + name: "create_terminal", + arguments: { owningTabId: "tb-4e8d0c2f1", paneId: "pn-a", direction: "vertical" }, + }); + const post = calls.find((c) => c.method === "post" && c.url === "/terminals"); + expect((post?.body as any)?.owningTabId).toBe("tb-4e8d0c2f1"); + }); + + it("get_terminal_detail and get_my_terminal advertise owningTabId", async () => { + const { api } = makeFakeApi(); + const client = await connectClient(createMcpServer({ api, getCallerId: () => "pc-self" })); + const tools = await toolMap(client); + expect(tools.get("get_terminal_detail")!.description).toContain("owningTabId"); + expect(tools.get("get_my_terminal")!.description).toContain("owningTabId"); + }); +}); + describe("get_terminal_screen tool", () => { it("posts { terminalId } to /fleet/screen for a local screen", async () => { const { api, calls } = makeFakeApi(); From 6941b4c09aba82a5de32ab96569f2208fa6ebe79 Mon Sep 17 00:00:00 2001 From: Tam Tran Date: Sat, 15 Aug 2026 00:57:58 -0500 Subject: [PATCH 17/22] fix(api): reserve the owner across a create's spawn to close the root-leaf race MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit External review 099 T2-F1 (CRITICAL). The leaf-uniqueness rule was decided by a read-only scan of `state.terminals` that completes BEFORE `spawn_terminal`, which registers the new Terminal LAST -- after PTY creation, writer, and screen parser (pty_manager.rs:862-871, an order that is load-bearing for the close/delete existence gate and is deliberately left alone here). Axum serves requests in parallel, so two POST /api/terminals with the same caller-supplied, currently unoccupied `tb-` owner and no `paneId` both scanned "unoccupied", both took `leaf == owner`, and both registered a live Terminal on the SAME renderer leaf -- one `terminal_history` PRIMARY KEY for two panes, the exact invariant P0-A exists to establish (design 011 §3 / success criterion 7). Fix: reserve the OWNER, not the leaf, for the decision -> registration window. * `RootLeafClaims` (state.rs): a DashMap of owners with an in-flight root-leaf claim. `try_claim` takes the reservation with ONE atomic insert -- returning whether it was newly inserted -- never contains-then-insert. * Order is claim FIRST, scan `terminals` SECOND. Scanning first only narrows the hole: A scans empty, A registers, A releases, B claims (now free) and still acts on its stale scan. Claiming first closes it, because a claim is released only after the winner's Terminal is visible. * `resolve_api_spawn_identity` now returns the guard alongside the identity: reserved + unoccupied -> tab root, leaf == owner; already claimed or already registered -> split, fresh `tm-` leaf. A `paneId` create is unconditionally a split and reserves nothing. * Release is RAII (`Drop`), so no early return can leak it, and the handler drops it explicitly right after `spawn_terminal` returns -- covering the failure path and making the guard's lifetime visible. A leaked claim would be degraded but safe (later creates mint `tm-`); releasing early is the unsafe direction. Tests (all fail against the pre-fix code, verified): * `a_create_inside_another_creates_spawn_window_cannot_take_the_same_root_leaf` drives the real interleaving -- B resolves while A holds its claim and has not yet registered -- and asserts distinct leaves, then that the post-release create still splits. * `only_one_of_many_racing_creates_gets_the_root_leaf` runs 8 real threads on a barrier, exercising the atomicity of the claim itself. * `a_failed_spawn_releases_the_owner_reservation` asserts the reservation is released when the spawn fails and the retry is a root again. cargo test: 292 passed, 0 failed. --- src-tauri/src/api_server.rs | 326 ++++++++++++++++++++++++++++++++---- src-tauri/src/state.rs | 73 ++++++++ 2 files changed, 362 insertions(+), 37 deletions(-) diff --git a/src-tauri/src/api_server.rs b/src-tauri/src/api_server.rs index 34e9756..b8c6396 100644 --- a/src-tauri/src/api_server.rs +++ b/src-tauri/src/api_server.rs @@ -502,18 +502,35 @@ fn mint_renderer_id(prefix: &str) -> String { /// remove (review 095 B1). /// * Otherwise this is the tab's first/solo pane and leaf == owner, as before. /// -/// `owner_has_live_terminal` is injected rather than read from `AppState` so this -/// stays a pure unit-testable decision (the Windows test binary cannot build the +/// CONCURRENCY (review 099 T2-F1). "Already holds a live terminal" is read from +/// `terminals`, but the create that would make it true registers its `Terminal` +/// only at the very END of `spawn_terminal` (`pty_manager.rs:862-871`, an order +/// that must not change). Two parallel POSTs naming the same empty tab therefore +/// both read "unoccupied" and both took the tab id as their leaf. So occupancy is +/// now the disjunction of two sources, and the second is taken as a RESERVATION: +/// +/// 1. `claim_root_leaf` — atomically reserves the OWNER for this create and +/// hands back an RAII guard; `None` means another create already holds it. +/// Claimed FIRST, before the scan, because a claim is released only after +/// the winner's `Terminal` is visible (see `RootLeafClaims::try_claim`). +/// 2. `owner_has_live_terminal` — the pre-existing scan, for creates that +/// already finished. +/// +/// The returned guard belongs to the CALLER and must outlive `spawn_terminal`. +/// +/// Both hooks are injected rather than read from `AppState` so this stays a pure +/// unit-testable decision (the Windows test binary cannot build the /// `integration-tests` feature that `mock_app` needs). A freshly minted owner -/// trivially has no live terminal, so the extra probe cannot disturb the -/// new-tab path. -fn resolve_api_spawn_identity( +/// trivially has neither a live terminal nor a competing claim, so the extra +/// probes cannot disturb the new-tab path. +fn resolve_api_spawn_identity( tab_id: Option<&str>, owning_tab_id: Option<&str>, pane_id: Option<&str>, + claim_root_leaf: impl FnOnce(&str) -> Option, owner_has_live_terminal: impl Fn(&str) -> bool, mut mint: impl FnMut(&str) -> String, -) -> Result { +) -> Result<(ApiSpawnIdentity, Option), String> { let owner_hint = owning_tab_id .or(tab_id) .map(str::trim) @@ -535,13 +552,29 @@ fn resolve_api_spawn_identity( // pane". `pane_id` is kept as a signal because a caller that DOES name a pane // is telling us it wants a split even if the tab's live set is momentarily // empty (e.g. its only PTY just exited). - let renderer_terminal_id = if pane_id.is_some() || owner_has_live_terminal(&owning_tab_id) { - mint("tm") + let (renderer_terminal_id, root_leaf_claim) = if pane_id.is_some() { + // Unconditionally a split: a fresh `tm-` is unique by construction, so + // there is nothing to reserve and no reason to block a concurrent root. + (mint("tm"), None) } else { - owning_tab_id.clone() + match claim_root_leaf(&owning_tab_id) { + // We reserved the owner AND no finished create holds it: this is the + // tab root, leaf == owner. The guard travels back to the caller. + Some(claim) if !owner_has_live_terminal(&owning_tab_id) => { + (owning_tab_id.clone(), Some(claim)) + } + // Occupied by a registered terminal. We drop the reservation right + // here: we are not taking the root leaf, so holding it would only + // stall the next create for no gain. + Some(_) => (mint("tm"), None), + // Another create is IN FLIGHT as this tab's root. It may not be + // visible in `terminals` yet, but it has already committed to the + // tab id as its leaf — so this one is a split. + None => (mint("tm"), None), + } }; - Ok(ApiSpawnIdentity { renderer_terminal_id, owning_tab_id }) + Ok((ApiSpawnIdentity { renderer_terminal_id, owning_tab_id }, root_leaf_claim)) } async fn create_terminal( @@ -590,16 +623,22 @@ async fn create_terminal( // registers with them up front (review 062 F-01: patching an id in after // spawn returns races a fast-exiting shell's exit-path persist, which then // files the final scrollback under the ephemeral pc- id). - let identity = match resolve_api_spawn_identity( + let (identity, root_leaf_claim) = match resolve_api_spawn_identity( payload.tab_id.as_deref(), payload.owning_tab_id.as_deref(), payload.pane_id.as_deref(), + // Reserve the owner for the decision→registration window (review 099 + // T2-F1). Atomic insert, taken BEFORE the scan below; released by the + // guard's `Drop` once `spawn_terminal` has returned. + |owner: &str| state.root_leaf_claims.try_claim(owner), // D7 / review 095 B1: a create that lands in an already-occupied tab is a // SPLIT even with no `paneId` (App.tsx Mode 2), so the leaf must be fresh. // A terminal claims a tab either as its owner or — for a tab root, and for // anything registered before P0-A — as its own renderer leaf. // Read-only iteration that completes before the spawn: no shard guard is // held across `spawn_terminal`, and nothing inside takes another lock. + // On its own this is a TOCTOU read, which is why the reservation above + // covers the creates it cannot see yet. |owner: &str| { state.terminals.iter().any(|e| { let t = e.value(); @@ -611,11 +650,13 @@ async fn create_terminal( ) { Ok(i) => i, Err(e) => { + // No claim exists yet on this path (resolution failed before it was + // taken), so there is nothing to release. return (StatusCode::BAD_REQUEST, Json(json!({ "error": e }))).into_response() } }; - match crate::pty_manager::spawn_terminal( + let spawned = crate::pty_manager::spawn_terminal( state.clone(), cols, rows, @@ -627,7 +668,15 @@ async fn create_terminal( Some(identity.renderer_terminal_id.clone()), Some(identity.owning_tab_id.clone()), None, // API-created terminal: fresh session, no restored scrollback - ) { + ); + // EARLIEST CORRECT RELEASE, and the only one that matters: `spawn_terminal` + // has either registered the `Terminal` (so the scan above now sees this tab + // as occupied) or failed (so the owner is free again). Explicit rather than + // left to end-of-scope so the guard's lifetime — everything above this line — + // is visible; `Drop` still covers the `?`-free early return paths. + drop(root_leaf_claim); + + match spawned { Ok(id) => { // Notify the UI to create a tab for this new terminal. We BROADCAST (a // bare emit_to is documented as not reaching the JS listener here — see @@ -3368,6 +3417,13 @@ mod tests { false } + /// No other create is in flight, so the owner reservation always succeeds. + /// `()` stands in for the RAII guard in tests that don't exercise it; + /// production passes `state.root_leaf_claims.try_claim`. + fn no_competing_create(_owner: &str) -> Option<()> { + Some(()) + } + /// THE REGRESSION TEST (design 011 §7 test 1). Two API creates targeting the /// same tab with a pane_id — the split-a-pane flow — must produce DISTINCT /// leaves and the SAME owner. Before P0-A both stored `tb-shared01` as @@ -3377,14 +3433,16 @@ mod tests { #[test] fn spawn_identity_two_api_splits_get_distinct_leaves_and_one_owner() { let mut mint = counting_mint(); - let a = resolve_api_spawn_identity( - Some("tb-shared01"), None, Some("pn-a"), no_live_terminals, &mut mint, + let (a, _) = resolve_api_spawn_identity( + Some("tb-shared01"), None, Some("pn-a"), + no_competing_create, no_live_terminals, &mut mint, ) .expect("split a"); // By the time split b arrives, split a is live in that tab — so BOTH // signals are true here, and either alone must be enough (see D7). - let b = resolve_api_spawn_identity( - Some("tb-shared01"), None, Some("pn-b"), |owner| owner == "tb-shared01", &mut mint, + let (b, _) = resolve_api_spawn_identity( + Some("tb-shared01"), None, Some("pn-b"), + no_competing_create, |owner| owner == "tb-shared01", &mut mint, ) .expect("split b"); @@ -3402,11 +3460,13 @@ mod tests { #[test] fn spawn_identity_first_create_into_an_empty_tab_keeps_leaf_equal_to_owner() { let mut mint = counting_mint(); - let r = resolve_api_spawn_identity( - Some("tb-shared01"), None, None, no_live_terminals, &mut mint, + let (r, claim) = resolve_api_spawn_identity( + Some("tb-shared01"), None, None, + no_competing_create, no_live_terminals, &mut mint, ) .expect("root"); assert_eq!(r.renderer_terminal_id, "tb-shared01"); + assert!(claim.is_some(), "a create that takes the root leaf must hold the reservation"); assert_eq!(r.owning_tab_id, "tb-shared01"); } @@ -3423,21 +3483,32 @@ mod tests { #[test] fn spawn_identity_second_create_into_a_populated_tab_gets_a_distinct_leaf() { let mut mint = counting_mint(); - let root = resolve_api_spawn_identity( - Some("tb-shared01"), None, None, no_live_terminals, &mut mint, + let claims = std::sync::Arc::new(crate::state::RootLeafClaims::default()); + let (root, root_claim) = resolve_api_spawn_identity( + Some("tb-shared01"), None, None, + |o| claims.try_claim(o), no_live_terminals, &mut mint, ) .expect("first create"); assert_eq!(root.renderer_terminal_id, "tb-shared01"); + // The root has REGISTERED and released by the time the second create + // arrives; occupancy is now carried by the `terminals` scan alone. + drop(root_claim); + assert!(!claims.is_claimed("tb-shared01")); // The identical call, with the tab now occupied by `root`. - let second = resolve_api_spawn_identity( + let (second, second_claim) = resolve_api_spawn_identity( Some("tb-shared01"), None, None, // NO pane_id — the Mode 2 call shape + |o| claims.try_claim(o), |owner| owner == "tb-shared01", &mut mint, ) .expect("second create"); + assert!( + second_claim.is_none(), + "a split reserves nothing — its `tm-` leaf is unique by construction" + ); assert_ne!( second.renderer_terminal_id, root.renderer_terminal_id, @@ -3451,11 +3522,185 @@ mod tests { assert_eq!(second.owning_tab_id, "tb-shared01", "and it stays in that tab"); } + /// A `terminals`-scan stand-in that only reports what has been REGISTERED, + /// so a test can place a create inside the decision→registration window. + fn registered_scan( + registered: &std::sync::Arc>, + ) -> impl Fn(&str) -> bool + '_ { + move |owner: &str| registered.contains_key(owner) + } + + /// THE T2-F1 REGRESSION TEST (external review 099). The interleaving the + /// sequential tests above structurally cannot reach: create B resolves while + /// create A is between its identity decision and its `terminals` insert. + /// + /// Against the pre-fix code both creates took `tb-shared01` as their leaf — + /// two live terminals on one `terminal_history` PRIMARY KEY, the exact + /// invariant P0-A exists to establish (design 011 §3, success criterion 7). + #[test] + fn a_create_inside_another_creates_spawn_window_cannot_take_the_same_root_leaf() { + let claims = std::sync::Arc::new(crate::state::RootLeafClaims::default()); + // Stands in for `state.terminals`: a create appears here only when + // `spawn_terminal` reaches its final insert (`pty_manager.rs:867`). + let registered = std::sync::Arc::new(dashmap::DashMap::new()); + let mut mint = counting_mint(); + + // Create A decides its identity. Its PTY is still being built, so it has + // NOT registered — `registered` is empty. + let (a, a_claim) = resolve_api_spawn_identity( + Some("tb-shared01"), None, None, + |o| claims.try_claim(o), + registered_scan(®istered), + &mut mint, + ) + .expect("create A"); + assert_eq!(a.renderer_terminal_id, "tb-shared01", "A is the tab root"); + assert!(a_claim.is_some(), "A must hold the owner reservation across its spawn"); + assert!(claims.is_claimed("tb-shared01")); + + // Create B arrives INSIDE that window. The scan still says "empty" — + // that is precisely the stale read T2-F1 is about. + let (b, b_claim) = resolve_api_spawn_identity( + Some("tb-shared01"), None, None, + |o| claims.try_claim(o), + registered_scan(®istered), + &mut mint, + ) + .expect("create B"); + assert_ne!( + b.renderer_terminal_id, a.renderer_terminal_id, + "two live terminals must never carry the same renderer leaf" + ); + assert!( + b.renderer_terminal_id.starts_with("tm-"), + "B lost the root race, so it is a split: {}", + b.renderer_terminal_id + ); + assert_eq!(b.owning_tab_id, "tb-shared01", "and it still lands in that tab"); + assert!(b_claim.is_none(), "a split reserves nothing"); + + // A now registers and releases, in that order. + registered.insert(a.renderer_terminal_id.clone(), ()); + drop(a_claim); + assert!(!claims.is_claimed("tb-shared01"), "the reservation is released once registered"); + + // The window is closed, but the tab is now genuinely occupied — the + // release must NOT hand the root leaf to the next create. + let (c, _) = resolve_api_spawn_identity( + Some("tb-shared01"), None, None, + |o| claims.try_claim(o), + registered_scan(®istered), + &mut mint, + ) + .expect("create C"); + assert!( + c.renderer_terminal_id.starts_with("tm-"), + "after A registered, later creates are splits: {}", + c.renderer_terminal_id + ); + } + + /// The release must also happen when the spawn FAILS: a leaked reservation + /// would permanently force every future create into that tab to mint a + /// `tm-`, leaving the tab with no root pane. RAII, so a `?`/early return + /// cannot skip it. + #[test] + fn a_failed_spawn_releases_the_owner_reservation() { + let claims = std::sync::Arc::new(crate::state::RootLeafClaims::default()); + let registered = std::sync::Arc::new(dashmap::DashMap::new()); + let mut mint = counting_mint(); + + { + let (first, claim) = resolve_api_spawn_identity( + Some("tb-shared01"), None, None, + |o| claims.try_claim(o), + registered_scan(®istered), + &mut mint, + ) + .expect("first create"); + assert_eq!(first.renderer_terminal_id, "tb-shared01"); + assert!(claims.is_claimed("tb-shared01")); + // `spawn_terminal` returns Err: nothing is ever registered, and the + // guard goes out of scope exactly as it does in `create_terminal`. + drop(claim); + } + assert!( + !claims.is_claimed("tb-shared01"), + "a failed spawn must not leak the reservation" + ); + + // The retry is a first create again, not a split. + let (retry, retry_claim) = resolve_api_spawn_identity( + Some("tb-shared01"), None, None, + |o| claims.try_claim(o), + registered_scan(®istered), + &mut mint, + ) + .expect("retry"); + assert_eq!( + retry.renderer_terminal_id, "tb-shared01", + "the tab is still empty, so its root leaf is still available" + ); + assert!(retry_claim.is_some()); + } + + /// The same schedule under REAL threads, so the atomicity of the claim + /// itself is exercised rather than modelled: N creates race for one empty + /// tab, all inside each other's spawn window (every guard is held until the + /// end). Exactly one may come out as the tab root. + #[test] + fn only_one_of_many_racing_creates_gets_the_root_leaf() { + const RACERS: usize = 8; + let claims = std::sync::Arc::new(crate::state::RootLeafClaims::default()); + let barrier = std::sync::Arc::new(std::sync::Barrier::new(RACERS)); + + let handles: Vec<_> = (0..RACERS) + .map(|_| { + let claims = std::sync::Arc::clone(&claims); + let barrier = std::sync::Arc::clone(&barrier); + std::thread::spawn(move || { + barrier.wait(); + // Nothing has registered yet — every racer's scan says + // "empty", exactly as in the reported defect. + resolve_api_spawn_identity( + Some("tb-shared01"), None, None, + |o| claims.try_claim(o), + no_live_terminals, + mint_renderer_id, + ) + .expect("racing create") + }) + }) + .collect(); + + // Guards stay alive in `results` for the whole assertion block: all + // RACERS creates are still "in flight". + let results: Vec<_> = handles.into_iter().map(|h| h.join().expect("thread")).collect(); + + let roots = results + .iter() + .filter(|(id, _)| id.renderer_terminal_id == "tb-shared01") + .count(); + assert_eq!(roots, 1, "exactly one racer may take the tab id as its leaf"); + + let leaves: std::collections::HashSet<_> = + results.iter().map(|(id, _)| id.renderer_terminal_id.clone()).collect(); + assert_eq!(leaves.len(), RACERS, "every racer's leaf must be distinct: {leaves:?}"); + assert!(results.iter().all(|(id, _)| id.owning_tab_id == "tb-shared01")); + assert_eq!( + results.iter().filter(|(_, claim)| claim.is_some()).count(), + 1, + "and only the root holds a reservation" + ); + } + #[test] fn spawn_identity_no_caller_id_mints_a_tab_exactly_as_before() { let mut mint = counting_mint(); - let r = resolve_api_spawn_identity(None, None, None, no_live_terminals, &mut mint) - .expect("minted"); + let (r, _) = resolve_api_spawn_identity( + None, None, None, no_competing_create, no_live_terminals, &mut mint, + ) + .expect("minted"); assert_eq!(r.owning_tab_id, "tb-000000001"); assert_eq!(r.renderer_terminal_id, "tb-000000001"); } @@ -3463,16 +3708,19 @@ mod tests { #[test] fn spawn_identity_an_empty_or_unrecognised_tab_id_still_mints_rather_than_failing() { let mut mint = counting_mint(); - assert!( - resolve_api_spawn_identity(Some(" "), None, None, no_live_terminals, &mut mint) - .expect("blank") - .owning_tab_id - .starts_with("tb-") - ); assert!(resolve_api_spawn_identity( - Some("legacy-monitor-id"), None, None, no_live_terminals, &mut mint, + Some(" "), None, None, no_competing_create, no_live_terminals, &mut mint, + ) + .expect("blank") + .0 + .owning_tab_id + .starts_with("tb-")); + assert!(resolve_api_spawn_identity( + Some("legacy-monitor-id"), None, None, + no_competing_create, no_live_terminals, &mut mint, ) .expect("junk") + .0 .owning_tab_id .starts_with("tb-")); } @@ -3486,7 +3734,8 @@ mod tests { fn spawn_identity_a_pane_leaf_id_in_the_tab_field_is_rejected_not_silently_replaced() { let mut mint = counting_mint(); let err = resolve_api_spawn_identity( - Some("tm-9f2c1a4b7"), None, Some("pn-a"), no_live_terminals, &mut mint, + Some("tm-9f2c1a4b7"), None, Some("pn-a"), + no_competing_create, no_live_terminals, &mut mint, ) .expect_err("a tm- id is a pane id, not a tab id"); assert!(err.contains("tm-9f2c1a4b7"), "the message must name the offending id: {err}"); @@ -3497,10 +3746,11 @@ mod tests { #[test] fn spawn_identity_an_explicit_owning_tab_id_takes_precedence() { let mut mint = counting_mint(); - let r = resolve_api_spawn_identity( + let (r, _) = resolve_api_spawn_identity( Some("tb-ignored1"), Some("tb-explicit"), Some("pn-a"), + no_competing_create, no_live_terminals, &mut mint, ) @@ -3532,12 +3782,14 @@ mod tests { let mut mint = counting_mint(); // `no_live_terminals` (Task 4's helper): with a `pane_id` present the // occupancy probe is not even needed to force distinct leaves. - let a = resolve_api_spawn_identity( - Some("tb-shared01"), None, Some("pn-a"), no_live_terminals, &mut mint, + let (a, _) = resolve_api_spawn_identity( + Some("tb-shared01"), None, Some("pn-a"), + no_competing_create, no_live_terminals, &mut mint, ) .expect("split a"); - let b = resolve_api_spawn_identity( - Some("tb-shared01"), None, Some("pn-b"), no_live_terminals, &mut mint, + let (b, _) = resolve_api_spawn_identity( + Some("tb-shared01"), None, Some("pn-b"), + no_competing_create, no_live_terminals, &mut mint, ) .expect("split b"); diff --git a/src-tauri/src/state.rs b/src-tauri/src/state.rs index f88226b..05ff402 100644 --- a/src-tauri/src/state.rs +++ b/src-tauri/src/state.rs @@ -87,6 +87,72 @@ fn default_terminal_rows() -> u16 { 24 } +/// Owners (`tb-*` tab ids) with an **in-flight root-leaf claim**: an API create +/// has decided to take that tab id as its pane leaf but has not registered its +/// `Terminal` yet. +/// +/// This exists because the leaf-uniqueness rule (design 011 §3, D7) is decided +/// from a READ of `terminals` while the write that would make it true happens +/// much later: `spawn_terminal` registers the `Terminal` LAST, after the PTY, +/// writer and screen parser are in place (`pty_manager.rs:862-871` — that order +/// is load-bearing for the close/delete existence gate and must not be moved). +/// Axum serves requests in parallel, so two POSTs naming the same empty tab both +/// scanned "unoccupied" and both took the tab id as their leaf — the exact +/// collision P0-A removes (external review 099, T2-F1). +/// +/// The fix reserves the OWNER, not the leaf, for that window: the claim is taken +/// ATOMICALLY (a single `DashMap::insert`, never contains-then-insert) BEFORE +/// the `terminals` scan, and released only once registration has happened or the +/// spawn has failed. See `try_claim` for the ordering argument. +#[derive(Default)] +pub struct RootLeafClaims(DashMap); + +impl RootLeafClaims { + /// Reserve `owner` for this create, or return `None` because another create + /// is already mid-flight for it (that one is the tab root; this one is a + /// split and must mint a fresh `tm-` leaf). + /// + /// ORDER MATTERS: callers must claim FIRST and scan `terminals` SECOND. + /// Scanning first would leave the same hole one notch narrower — A scans + /// empty, A registers, A releases, B claims (now free) and B still believes + /// the tab is empty from its stale scan. Claiming first closes it: a claim + /// only becomes free again *after* the winner's `Terminal` is visible in + /// `terminals`, so whoever claims next either sees it and splits, or is + /// genuinely first. + pub fn try_claim(self: &Arc, owner: &str) -> Option { + // `insert` returns the PREVIOUS value: `None` means we are the ones who + // put it there. One atomic shard operation — a `contains_key` followed + // by an `insert` would reintroduce the very race this closes. + self.0.insert(owner.to_string(), ()).is_none().then(|| RootLeafClaim { + owner: owner.to_string(), + claims: Arc::clone(self), + }) + } + + #[cfg(test)] + pub fn is_claimed(&self, owner: &str) -> bool { + self.0.contains_key(owner) + } +} + +/// RAII release for a `RootLeafClaims` reservation. +/// +/// Drop, not an explicit release call, so an early `return`/`?` on any spawn +/// failure path cannot leak the claim. A leaked claim is degraded-but-safe +/// (every later create into that tab mints a `tm-` leaf instead of reusing the +/// tab id); releasing it too EARLY is the unsafe direction, so hold it until +/// `spawn_terminal` has returned. +pub struct RootLeafClaim { + owner: String, + claims: Arc, +} + +impl Drop for RootLeafClaim { + fn drop(&mut self) { + self.claims.0.remove(&self.owner); + } +} + #[derive(Clone, Debug)] pub struct ChannelPayload { pub id: String, @@ -127,6 +193,11 @@ pub struct AppState { // managed state and all task clones see the same value. pub pending_open_path: Arc>>, pub terminals: Arc>, + // Tabs whose root leaf is claimed by an API create that has not registered + // its `Terminal` yet. Closes the decision→registration window in which two + // concurrent creates could both take a tab's id as their pane leaf (review + // 099 T2-F1). See `RootLeafClaims`. + pub root_leaf_claims: Arc, // Values are Arc'd so PTY write paths clone the Arc and DROP the DashMap // shard guard before locking the inner Mutex. Holding a shard guard across // the send/probe `.await` sleeps (up to ~48 s) blocked any insert/remove on @@ -298,6 +369,7 @@ impl Clone for AppState { Self { pending_open_path: self.pending_open_path.clone(), terminals: self.terminals.clone(), + root_leaf_claims: self.root_leaf_claims.clone(), shell_writer_channels: self.shell_writer_channels.clone(), ptys: self.ptys.clone(), output_tx: self.output_tx.clone(), @@ -385,6 +457,7 @@ impl AppState { Self { pending_open_path: Arc::new(std::sync::Mutex::new(None)), terminals: Arc::new(DashMap::new()), + root_leaf_claims: Arc::new(RootLeafClaims::default()), shell_writer_channels: Arc::new(DashMap::new()), ptys: Arc::new(DashMap::new()), output_tx, From e94e5b1a604b7d7edcfc5a2860ed862f33917d50 Mon Sep 17 00:00:00 2001 From: Tam Tran Date: Sat, 15 Aug 2026 01:12:54 -0500 Subject: [PATCH 18/22] fix(p0a): keep the backend owning tab in step with pane moves (review 099 T2-F2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `owning_tab_id` was written once at spawn and never again, so moving a pane into another tab left the backend naming the tab the pane had left. That is not cosmetic: the stale owner is echoed by get_terminal_detail/get_my_terminal, and the MCP tool descriptions tell an agent to pass that owningTabId back when it creates a sibling pane — so the agent's next pane is created in the wrong tab. External activity also lit the wrong (still-open) tab. Before P0-A a split pane's indicator was merely DROPPED; actively routing new work somewhere wrong is a regression, not a pre-existing condition, so the plan's "safe to defer / no worse than today" judgement was wrong. Backend: `state::retarget_owning_tab` + the `set_terminal_owning_tab` command. Keyed by the renderer LEAF (what the pane tree holds, unique per live pane, and the same identity on both spawn paths), matched and written under one shard guard. Guarded like the create path — fail closed on a `tm-` "tab", reject blank ids — and a leaf with no live PTY is a miss, not an error. Renderer: the update is driven off `panes.treesByTabId` itself rather than off the individual dispatch sites. The plan's proposed hook (TerminalService. bindProcess) cannot work — a moved pane already has a mapping and takes TerminalPane's reuse path without ever binding. Diffing the tree covers every reparent path by construction: same-window drag (movePaneToTab), cross-window drop (insertPaneIntoTab), detached-window boot and whole-tab reattach (addTabTree), reload/restore reattach, and any future programmatic move. resolveActivityTabId now treats the emitted owner as a HINT: the pane tree is consulted FIRST and wins whenever it has an answer, so a moved pane lights the tab it is in. The hint is kept as a fallback because it is the only answer available before the renderer has inserted an API-created pane into the tree, and on the headless/sidecar paths. Tests: retarget_owning_tab (inline, 7 cases) covers the owner update, leaf-not- map-key matching, misses and the rejected inputs; paneOwnership / paneOwnershipSync cover the diff rules and prove a real movePaneToTab dispatch reaches the bridge; externalActivity covers a moved pane's activity landing on the NEW tab for both a tm- and a tb- leaf. --- src-tauri/src/commands.rs | 28 +++ src-tauri/src/lib.rs | 1 + src-tauri/src/state.rs | 188 ++++++++++++++++++ src/renderer/api/browser-bridge.ts | 4 + src/renderer/api/tauri-bridge.ts | 9 + .../__tests__/externalActivity.test.ts | 67 +++++++ .../services/__tests__/paneOwnership.test.ts | 93 +++++++++ .../__tests__/paneOwnershipSync.test.ts | 114 +++++++++++ src/renderer/services/externalActivity.ts | 29 ++- src/renderer/services/paneOwnership.ts | 127 ++++++++++++ src/renderer/store/index.ts | 7 + src/renderer/types/electron.d.ts | 7 + 12 files changed, 665 insertions(+), 9 deletions(-) create mode 100644 src/renderer/services/__tests__/paneOwnership.test.ts create mode 100644 src/renderer/services/__tests__/paneOwnershipSync.test.ts create mode 100644 src/renderer/services/paneOwnership.ts diff --git a/src-tauri/src/commands.rs b/src-tauri/src/commands.rs index 623074e..0139797 100644 --- a/src-tauri/src/commands.rs +++ b/src-tauri/src/commands.rs @@ -190,6 +190,34 @@ pub fn adopt_console_window( Ok(()) } +/// Tell the backend that a pane moved into a different tab, so the owner stored +/// at spawn stops naming the tab the pane left (review 099 T2-F2). +/// +/// The renderer is the authority here: tab ownership lives only in +/// `panes.treesByTabId`, and the backend cannot derive it. Fired from the pane +/// tree's own change subscription (`services/paneOwnership.ts`), which is why it +/// covers every reparent path — same-window drag, cross-window drop, detached +/// window boot — rather than only fresh process binding. +/// +/// `renderer_terminal_id` is the LEAF (`tb-*` root, `tm-*` split), not the +/// process id: the leaf is what the pane tree holds and it is unique per live +/// pane (design 011 §3, D7). Best-effort like `adopt_console_window` — an +/// unmatched leaf is not an error, since the renderer fires this off its own +/// tree lifecycle and a pane's PTY may not exist (yet, or any more). +#[tauri::command] +pub fn set_terminal_owning_tab( + state: State<'_, AppState>, + renderer_terminal_id: String, + owning_tab_id: String, +) -> Result<(), String> { + if !crate::state::retarget_owning_tab(&state.terminals, &renderer_terminal_id, &owning_tab_id)? { + log::debug!( + "set_terminal_owning_tab: no live terminal carries leaf {renderer_terminal_id}" + ); + } + Ok(()) +} + /// Spawn a terminal hosted by the PTY-host sidecar. The app terminalId IS the /// stable `tab_id` (the reattach key), so the sidecar session, the output /// broadcast id, and the vt100 screen key all align — live routing works with diff --git a/src-tauri/src/lib.rs b/src-tauri/src/lib.rs index b122e22..3116287 100644 --- a/src-tauri/src/lib.rs +++ b/src-tauri/src/lib.rs @@ -1248,6 +1248,7 @@ pub fn run() { .invoke_handler(tauri::generate_handler![ commands::create_terminal, commands::adopt_console_window, + commands::set_terminal_owning_tab, commands::restart_for_update, commands::hotswap_available, commands::take_reattach_prompt_hook, diff --git a/src-tauri/src/state.rs b/src-tauri/src/state.rs index 05ff402..b247c92 100644 --- a/src-tauri/src/state.rs +++ b/src-tauri/src/state.rs @@ -441,6 +441,80 @@ pub(crate) fn history_key(renderer_terminal_id: Option<&str>) -> Option<&str> { } } +/// Repoint a live terminal's OWNING TAB after its pane was moved into a +/// different tab. +/// +/// `owning_tab_id` is written once, at spawn (`pty_manager::spawn_terminal`), +/// but the pane it names moves: a same-window drag dispatches `movePaneToTab` +/// and a cross-window drop re-parents the leaf into another window's tab. The +/// terminal's IDENTITY does not change — the leaf travels with the pane — so +/// nothing else in the system notices, and the stored owner keeps naming a tab +/// the pane has left. +/// +/// That is not cosmetic (external review 099, T2-F2). The stale owner is echoed +/// by `terminal_identity_json` to `get_terminal_detail` / `get_my_terminal`, and +/// the MCP tool descriptions tell an agent to pass that `owningTabId` straight +/// back when it creates a sibling pane — so the agent's next pane is created in +/// the wrong tab. It is also emitted on `terminal:external-activity`, lighting +/// the wrong tab. Silently dropping a split's indicator (the pre-P0-A behaviour) +/// is not equivalent to actively routing new work somewhere wrong. +/// +/// Keyed by the renderer LEAF rather than by the `terminals` map key, because +/// the leaf is what the renderer's pane tree — the authority on ownership — +/// actually holds; P0-A's uniqueness invariant (design 011 §3, D7) makes it +/// unambiguous, and it is the one identity that means the same thing on both +/// spawn paths (the sidecar path registers under the leaf, the in-process path +/// under a `pc-` id). +/// +/// Returns whether a terminal matched. A miss is NOT an error: panes are moved +/// freely, and a leaf can belong to a pane whose PTY has not spawned yet, has +/// already exited, or lives in another instance. +/// +/// Takes the map rather than `AppState` so the guard rules stay unit-testable in +/// an inline `#[cfg(test)]` module — the `integration-tests` feature that +/// `mock_app` needs breaks the Windows test binary. +pub(crate) fn retarget_owning_tab( + terminals: &DashMap, + renderer_terminal_id: &str, + owning_tab_id: &str, +) -> Result { + let leaf = renderer_terminal_id.trim(); + let owner = owning_tab_id.trim(); + if leaf.is_empty() || owner.is_empty() { + return Err("both a renderer terminal (leaf) id and an owning tab id are required".to_string()); + } + // Fail closed on the one value that is definitely NOT a tab, exactly as the + // API create path does (`api_server::resolve_api_spawn_identity`). Anything + // else is accepted verbatim: there is nothing to mint on an update path, and + // a layout restored from before the `tb-` convention still has to be able to + // correct its own ownership. + if owner.starts_with("tm-") { + return Err(format!( + "'{owner}' is a pane (leaf) id, not a tab id — pass the owning tab id" + )); + } + // `iter_mut`, not scan-then-`get_mut`: the match and the write happen under + // the same shard guard, so a concurrent writer cannot slip between them. + // Nothing inside takes another lock, so this cannot deadlock against the + // read-only occupancy scan in `create_terminal`. + for mut entry in terminals.iter_mut() { + if entry.renderer_terminal_id.as_deref() != Some(leaf) { + continue; + } + if entry.owning_tab_id.as_deref() != Some(owner) { + let previous = entry.owning_tab_id.clone(); + entry.owning_tab_id = Some(owner.to_string()); + log::info!( + "Terminal {} (leaf {leaf}) re-parented: owning tab {:?} -> {owner}", + entry.id, + previous + ); + } + return Ok(true); + } + Ok(false) +} + impl AppState { pub fn new( output_tx: broadcast::Sender, @@ -1890,6 +1964,120 @@ mod terminal_identity_serde_tests { } } +/// Review 099 T2-F2: the owner recorded at spawn goes stale the moment a pane is +/// dragged into another tab, and it is what `get_terminal_detail` hands an agent +/// to create a sibling pane with. +#[cfg(test)] +mod retarget_owning_tab_tests { + use super::{retarget_owning_tab, Terminal, TerminalBackend}; + use dashmap::DashMap; + + /// One live terminal: process `pc-1`, pane leaf `tm-x`, owned by tab `tb-a`. + fn one_split_pane() -> DashMap { + let map = DashMap::new(); + map.insert( + "pc-1".to_string(), + Terminal { + id: "pc-1".into(), + pid: 4242, + shell: "pwsh".into(), + name: "Terminal-pwsh".into(), + created_at: "2026-08-15T10:00:00+07:00".into(), + cols: 80, + rows: 24, + backend: TerminalBackend::PortablePty, + renderer_terminal_id: Some("tm-x".into()), + owning_tab_id: Some("tb-a".into()), + last_input_source: None, + last_input_at: None, + prompt_hook: false, + }, + ); + map + } + + /// THE regression: after the pane moves from tab A to tab B, the backend + /// owner must be tab B — otherwise activity lights A and an agent asking for + /// `owningTabId` creates its next pane in A. + #[test] + fn a_moved_pane_updates_the_stored_owner() { + let terminals = one_split_pane(); + assert_eq!(retarget_owning_tab(&terminals, "tm-x", "tb-b"), Ok(true)); + let t = terminals.get("pc-1").expect("terminal"); + assert_eq!(t.owning_tab_id.as_deref(), Some("tb-b")); + // The leaf is the pane's identity and travels WITH it — a move must not + // touch it (that is what makes history/reattach survive the move). + assert_eq!(t.renderer_terminal_id.as_deref(), Some("tm-x")); + } + + /// The map is keyed by the PROCESS id; the renderer only ever knows the leaf. + #[test] + fn it_matches_on_the_leaf_not_on_the_map_key() { + let terminals = one_split_pane(); + assert_eq!( + retarget_owning_tab(&terminals, "pc-1", "tb-b"), + Ok(false), + "the map key is not a renderer identity" + ); + assert_eq!( + terminals.get("pc-1").expect("terminal").owning_tab_id.as_deref(), + Some("tb-a"), + ); + } + + /// Panes move freely; a leaf with no live PTY (never spawned, already exited, + /// or another instance's) is an ordinary no-op, not a failure the renderer + /// should surface. + #[test] + fn an_unknown_leaf_is_a_miss_not_an_error() { + let terminals = one_split_pane(); + assert_eq!(retarget_owning_tab(&terminals, "tm-gone", "tb-b"), Ok(false)); + } + + #[test] + fn a_no_op_move_back_to_the_same_tab_still_reports_a_match() { + let terminals = one_split_pane(); + assert_eq!(retarget_owning_tab(&terminals, "tm-x", "tb-a"), Ok(true)); + assert_eq!( + terminals.get("pc-1").expect("terminal").owning_tab_id.as_deref(), + Some("tb-a"), + ); + } + + /// Same fail-closed rule as the create path: a `tm-` value is a pane, and + /// accepting it would file a terminal under an owner no tab can ever match. + #[test] + fn a_pane_id_is_rejected_as_an_owner() { + let terminals = one_split_pane(); + let err = retarget_owning_tab(&terminals, "tm-x", "tm-sibling").expect_err("must reject"); + assert!(err.contains("not a tab id"), "unhelpful message: {err}"); + assert_eq!( + terminals.get("pc-1").expect("terminal").owning_tab_id.as_deref(), + Some("tb-a"), + "a rejected call must not have written anything" + ); + } + + #[test] + fn blank_ids_are_rejected() { + let terminals = one_split_pane(); + assert!(retarget_owning_tab(&terminals, " ", "tb-b").is_err()); + assert!(retarget_owning_tab(&terminals, "tm-x", " ").is_err()); + } + + /// A layout persisted before the `tb-` convention still has to be able to + /// correct itself — there is nothing to mint on an update path. + #[test] + fn a_legacy_non_tb_tab_id_is_accepted_verbatim() { + let terminals = one_split_pane(); + assert_eq!(retarget_owning_tab(&terminals, "tm-x", "tab-legacy-7"), Ok(true)); + assert_eq!( + terminals.get("pc-1").expect("terminal").owning_tab_id.as_deref(), + Some("tab-legacy-7"), + ); + } +} + #[cfg(test)] mod history_key_tests { use super::history_key; diff --git a/src/renderer/api/browser-bridge.ts b/src/renderer/api/browser-bridge.ts index ed25312..c4d5e44 100644 --- a/src/renderer/api/browser-bridge.ts +++ b/src/renderer/api/browser-bridge.ts @@ -177,6 +177,10 @@ class BrowserBridge implements ElectronAPI { /// No-op in the browser: there is no OS window here to own a console dialog. async adoptConsoleWindow(_processId: string): Promise { } + /// No-op in the browser: the REST surface exposes no ownership update, and a + /// browser session has no pane-drag/detach paths to move a pane between tabs. + async setTerminalOwningTab(_rendererTerminalId: string, _owningTabId: string): Promise { } + async closeTerminal(id: string): Promise { try { await fetch(`${API_BASE_URL}/terminals/${id}`, { diff --git a/src/renderer/api/tauri-bridge.ts b/src/renderer/api/tauri-bridge.ts index bf9ad8b..832b4a9 100644 --- a/src/renderer/api/tauri-bridge.ts +++ b/src/renderer/api/tauri-bridge.ts @@ -50,6 +50,10 @@ interface ElectronAPI { /// `az login` WAM prompt) open in front instead of behind the app. Fired on /// every process bind, so a pane moved between windows re-owns to the new one. adoptConsoleWindow: (processId: string) => Promise; + /// Re-point a live terminal's owning tab after its pane was moved into a + /// different tab. Keyed by the renderer LEAF (`tb-*` root, `tm-*` split) — + /// see services/paneOwnership.ts. + setTerminalOwningTab: (rendererTerminalId: string, owningTabId: string) => Promise; getActiveWindow: () => Promise; setActiveWindow: (label: string) => Promise; closeTerminal: (id: string) => Promise; @@ -306,6 +310,11 @@ const tauriBridge: ElectronAPI = { await invoke('adopt_console_window', { terminalId: processId }); }, + setTerminalOwningTab: async (rendererTerminalId: string, owningTabId: string) => { + // Tauri maps camelCase JS keys onto the snake_case Rust parameters. + await invoke('set_terminal_owning_tab', { rendererTerminalId, owningTabId }); + }, + closeTerminal: async (id) => { return invoke('close_terminal', { id }); }, diff --git a/src/renderer/services/__tests__/externalActivity.test.ts b/src/renderer/services/__tests__/externalActivity.test.ts index d9f1437..314f50b 100644 --- a/src/renderer/services/__tests__/externalActivity.test.ts +++ b/src/renderer/services/__tests__/externalActivity.test.ts @@ -57,6 +57,73 @@ describe('resolveActivityTabId', () => { ).toBeNull(); }); + // Review 099 T2-F2. Tab A had two panes; the split leaf was dragged into tab + // B, which leaves A OPEN — so the owner the backend recorded at spawn still + // names a live tab and the old "trust it if the tab exists" rule lit A. The + // tree knows the pane is in B. + it('lights the NEW tab after a pane moved, even when the emitted owner is stale', () => { + const movedTrees = { + 'tb-4e8d0c2f1': { id: 'pn-a', type: 'terminal' as const, terminalId: 'tb-4e8d0c2f1' }, + 'tb-target007': { + id: 'pn-root-b', + type: 'split' as const, + direction: 'vertical' as const, + children: [ + { id: 'pn-c', type: 'terminal' as const, terminalId: 'tb-target007' }, + { id: 'pn-b', type: 'terminal' as const, terminalId: 'tm-9f2c1a4b7' }, + ], + }, + } as any; + const bothOpen = new Set(['tb-4e8d0c2f1', 'tb-target007']); + + expect( + resolveActivityTabId( + { owningTabId: 'tb-4e8d0c2f1', rendererTerminalId: 'tm-9f2c1a4b7', tabId: 'tm-9f2c1a4b7' }, + movedTrees, + bothOpen, + ), + ).toBe('tb-target007'); + }); + + // Same for a TAB ROOT leaf dragged into another tab: its leaf id is still + // `tb-`, and the tab it names is still open, so both the stale owner and the + // "leaf that is itself a tab" shortcut point at the wrong tab. + it('lights the NEW tab when the moved pane carried a root tb- leaf', () => { + const movedTrees = { + 'tb-source001': { id: 'pn-keep', type: 'terminal' as const, terminalId: 'tm-kept0001' }, + 'tb-target007': { + id: 'pn-root-b', + type: 'split' as const, + direction: 'vertical' as const, + children: [ + { id: 'pn-c', type: 'terminal' as const, terminalId: 'tb-target007' }, + { id: 'pn-moved', type: 'terminal' as const, terminalId: 'tb-source001' }, + ], + }, + } as any; + const bothOpen = new Set(['tb-source001', 'tb-target007']); + + expect( + resolveActivityTabId( + { owningTabId: 'tb-source001', rendererTerminalId: 'tb-source001' }, + movedTrees, + bothOpen, + ), + ).toBe('tb-target007'); + }); + + // The hint still earns its place: an API-created pane can produce activity + // before the renderer has inserted it into the tree. + it('falls back to the emitted owner when the tree has no answer yet', () => { + expect( + resolveActivityTabId( + { owningTabId: 'tb-4e8d0c2f1', rendererTerminalId: 'tm-notyetinserted' }, + trees, + knownTabIds, + ), + ).toBe('tb-4e8d0c2f1'); + }); + it('returns null rather than guessing when nothing resolves', () => { expect(resolveActivityTabId({}, trees, knownTabIds)).toBeNull(); expect( diff --git a/src/renderer/services/__tests__/paneOwnership.test.ts b/src/renderer/services/__tests__/paneOwnership.test.ts new file mode 100644 index 0000000..58e3457 --- /dev/null +++ b/src/renderer/services/__tests__/paneOwnership.test.ts @@ -0,0 +1,93 @@ +import { collectLeafOwners, diffOwnerChanges } from '../paneOwnership'; +import type { PaneNode } from '../../store/slices/panesSlice'; + +const leaf = (id: string, terminalId: string): PaneNode => ({ id, type: 'terminal', terminalId }); + +const split = (id: string, children: PaneNode[]): PaneNode => ({ + id, + type: 'split', + direction: 'vertical', + children, +}); + +const bound = (...ids: string[]) => (id: string) => ids.includes(id); +const noneBound = () => false; + +describe('collectLeafOwners', () => { + it('maps every leaf in every tab, however deeply nested', () => { + const owners = collectLeafOwners({ + 'tb-a': split('pn-1', [leaf('pn-2', 'tb-a'), split('pn-3', [leaf('pn-4', 'tm-x'), leaf('pn-5', 'tm-y')])]), + 'tb-b': leaf('pn-6', 'tb-b'), + }); + expect(Object.fromEntries(owners)).toEqual({ + 'tb-a': 'tb-a', + 'tm-x': 'tb-a', + 'tm-y': 'tb-a', + 'tb-b': 'tb-b', + }); + }); + + it('ignores split containers, which own no terminal', () => { + expect(collectLeafOwners({ 'tb-a': split('pn-1', [leaf('pn-2', 'tb-a')]) }).has('pn-1')).toBe(false); + }); +}); + +describe('diffOwnerChanges', () => { + // The bug this whole change exists for: a pane dragged from tab A to tab B. + it('reports a leaf that changed tab', () => { + const before = collectLeafOwners({ 'tb-a': split('pn-1', [leaf('pn-2', 'tb-a'), leaf('pn-3', 'tm-x')]) }); + const after = collectLeafOwners({ + 'tb-a': leaf('pn-2', 'tb-a'), + 'tb-b': split('pn-4', [leaf('pn-5', 'tb-b'), leaf('pn-3', 'tm-x')]), + }); + expect(diffOwnerChanges(before, after, noneBound)).toEqual([ + { rendererTerminalId: 'tm-x', owningTabId: 'tb-b' }, + ]); + }); + + it('says nothing when a tree changes shape without changing ownership', () => { + const before = collectLeafOwners({ 'tb-a': split('pn-1', [leaf('pn-2', 'tb-a'), leaf('pn-3', 'tm-x')]) }); + // Same two leaves, swapped positions and resized — no reparent. + const after = collectLeafOwners({ 'tb-a': split('pn-9', [leaf('pn-3', 'tm-x'), leaf('pn-2', 'tb-a')]) }); + expect(diffOwnerChanges(before, after, bound('tm-x', 'tb-a'))).toEqual([]); + }); + + // A pane dropped in from ANOTHER WINDOW is brand new to this tree, but its + // backend owner is the tab it came from. `attachExistingTerminal` has already + // bound it here, which is what distinguishes it from a fresh split. + it('reports a newly seen leaf that already has a live process', () => { + const after = collectLeafOwners({ 'tb-b': split('pn-1', [leaf('pn-2', 'tb-b'), leaf('pn-3', 'tm-fromA')]) }); + expect(diffOwnerChanges(collectLeafOwners({}), after, bound('tm-fromA'))).toEqual([ + { rendererTerminalId: 'tm-fromA', owningTabId: 'tb-b' }, + ]); + }); + + it('stays silent for a freshly split pane whose PTY has not spawned yet', () => { + const before = collectLeafOwners({ 'tb-a': leaf('pn-2', 'tb-a') }); + const after = collectLeafOwners({ 'tb-a': split('pn-1', [leaf('pn-2', 'tb-a'), leaf('pn-3', 'tm-new')]) }); + // The spawn itself carries the right owner, so the update would be a no-op. + expect(diffOwnerChanges(before, after, bound('tb-a'))).toEqual([]); + }); + + // Detached-window boot: the first tree this window ever sees is one it was + // handed, with its PTYs already attached. + it('reports attached leaves on the first observation of a tree', () => { + const after = collectLeafOwners({ 'tb-detached': leaf('pn-1', 'tm-moved') }); + expect(diffOwnerChanges(null, after, bound('tm-moved'))).toEqual([ + { rendererTerminalId: 'tm-moved', owningTabId: 'tb-detached' }, + ]); + }); + + it('does not report a cold start whose panes have not spawned yet', () => { + const after = collectLeafOwners({ 'tb-a': leaf('pn-1', 'tb-a') }); + expect(diffOwnerChanges(null, after, noneBound)).toEqual([]); + }); + + // A pane that left this window (moved out / tab closed) is the DESTINATION's + // business to report; a departure says nothing about the new owner. + it('reports nothing for a leaf that disappeared', () => { + const before = collectLeafOwners({ 'tb-a': split('pn-1', [leaf('pn-2', 'tb-a'), leaf('pn-3', 'tm-x')]) }); + const after = collectLeafOwners({ 'tb-a': leaf('pn-2', 'tb-a') }); + expect(diffOwnerChanges(before, after, bound('tm-x'))).toEqual([]); + }); +}); diff --git a/src/renderer/services/__tests__/paneOwnershipSync.test.ts b/src/renderer/services/__tests__/paneOwnershipSync.test.ts new file mode 100644 index 0000000..1708311 --- /dev/null +++ b/src/renderer/services/__tests__/paneOwnershipSync.test.ts @@ -0,0 +1,114 @@ +/** + * @jest-environment jsdom + * + * Wiring for review 099 T2-F2: dispatching a real reparent through the real + * pane reducers must tell the backend the pane's new owning tab. The sibling + * suite (paneOwnership.test.ts) covers the diff rules in isolation; this covers + * the part that actually regressed — nobody calling them. + */ +import { configureStore } from '@reduxjs/toolkit'; +import panesReducer, { + addTabTree, + insertPaneIntoTab, + movePaneToTab, + PaneNode, +} from '../../store/slices/panesSlice'; +import { attachPaneOwnershipSync } from '../paneOwnership'; + +const leaf = (id: string, terminalId: string): PaneNode => ({ id, type: 'terminal', terminalId }); + +const split = (id: string, children: PaneNode[]): PaneNode => ({ + id, + type: 'split', + direction: 'vertical', + size: 50, + children, +}); + +const makeStore = () => configureStore({ reducer: { panes: panesReducer } }); + +let setTerminalOwningTab: jest.Mock; +let store: ReturnType; +let unsubscribe: () => void; + +beforeEach(() => { + setTerminalOwningTab = jest.fn().mockResolvedValue(undefined); + (window as any).electronAPI = { setTerminalOwningTab }; + // No pane is bound to a live PTY in this window unless a test says otherwise. + (window as any).terminalService = { getProcessId: () => undefined }; + store = makeStore(); + unsubscribe = attachPaneOwnershipSync(store as any); +}); + +afterEach(() => unsubscribe()); + +describe('attachPaneOwnershipSync', () => { + it('reports a same-window drag of a split pane into another tab', () => { + store.dispatch(addTabTree({ + tabId: 'tb-drag-src', + tree: split('pn-src-root', [leaf('pn-src-a', 'tb-drag-src'), leaf('pn-src-b', 'tm-dragged')]), + })); + store.dispatch(addTabTree({ tabId: 'tb-drag-dst', tree: leaf('pn-dst', 'tb-drag-dst') })); + expect(setTerminalOwningTab).not.toHaveBeenCalled(); + + // Exactly what PaneDragController.commitDrop dispatches on a cross-tab drop. + store.dispatch(movePaneToTab({ + sourceTabId: 'tb-drag-src', + sourcePaneId: 'pn-src-b', + targetTabId: 'tb-drag-dst', + targetPaneId: 'pn-dst', + zone: 'right', + })); + + expect(setTerminalOwningTab).toHaveBeenCalledTimes(1); + expect(setTerminalOwningTab).toHaveBeenCalledWith('tm-dragged', 'tb-drag-dst'); + }); + + it('reports a cross-window drop, whose pane is attached before it is inserted', () => { + // applyCrossWindowPayload attaches the live PTY first, THEN dispatches — that + // binding is how this window tells an incoming pane from a fresh split. + (window as any).terminalService = { + getProcessId: (id: string) => (id === 'tm-fromOtherWindow' ? 'pc-live' : undefined), + }; + store.dispatch(addTabTree({ tabId: 'tb-window-b', tree: leaf('pn-wb', 'tb-window-b') })); + expect(setTerminalOwningTab).not.toHaveBeenCalled(); + + store.dispatch(insertPaneIntoTab({ + tabId: 'tb-window-b', + targetPaneId: 'pn-wb', + zone: 'right', + node: leaf('pn-incoming', 'tm-fromOtherWindow'), + })); + + expect(setTerminalOwningTab).toHaveBeenCalledTimes(1); + expect(setTerminalOwningTab).toHaveBeenCalledWith('tm-fromOtherWindow', 'tb-window-b'); + }); + + it('stays quiet when a tab tree changes without any pane changing tab', () => { + store.dispatch(addTabTree({ tabId: 'tb-quiet', tree: leaf('pn-q', 'tb-quiet') })); + store.dispatch(addTabTree({ + tabId: 'tb-quiet', + tree: split('pn-q-root', [leaf('pn-q', 'tb-quiet'), leaf('pn-q2', 'tm-new-split')]), + })); + expect(setTerminalOwningTab).not.toHaveBeenCalled(); + }); + + it('never fires twice for the same move', () => { + store.dispatch(addTabTree({ + tabId: 'tb-once-src', + tree: split('pn-o-root', [leaf('pn-o-a', 'tb-once-src'), leaf('pn-o-b', 'tm-once')]), + })); + store.dispatch(addTabTree({ tabId: 'tb-once-dst', tree: leaf('pn-o-dst', 'tb-once-dst') })); + store.dispatch(movePaneToTab({ + sourceTabId: 'tb-once-src', + sourcePaneId: 'pn-o-b', + targetTabId: 'tb-once-dst', + targetPaneId: 'pn-o-dst', + zone: 'bottom', + })); + // A later, unrelated tree change must not re-announce the settled owner. + store.dispatch(addTabTree({ tabId: 'tb-unrelated', tree: leaf('pn-u', 'tb-unrelated') })); + + expect(setTerminalOwningTab).toHaveBeenCalledTimes(1); + }); +}); diff --git a/src/renderer/services/externalActivity.ts b/src/renderer/services/externalActivity.ts index 429033f..eed069b 100644 --- a/src/renderer/services/externalActivity.ts +++ b/src/renderer/services/externalActivity.ts @@ -29,15 +29,16 @@ export function resolveActivityTabId( treesByTabId: Record, knownTabIds: Set, ): string | null { - // 1. The backend told us the owner outright. Trust it only if the tab is - // still open — a closed tab must not resurrect an indicator. - if (detail.owningTabId && knownTabIds.has(detail.owningTabId)) { - return detail.owningTabId; - } - - // 2. Resolve a renderer leaf through the pane tree. `tabId` is a deprecated - // alias of the leaf, so it is a leaf candidate, NOT a tab candidate — - // except in the one case where it is genuinely a root tab id (below). + // 1. The PANE TREE IS AUTHORITATIVE. Resolve the renderer leaf through it + // first: it is this window's live record of which tab holds which pane, + // whereas the emitted owner is a backend copy written at spawn that a pane + // move can invalidate (review 099 T2-F2 — `setTerminalOwningTab` repairs + // it, but an event already in flight can still carry the old value, and a + // build/instance without the repair carries it always). Whenever the tree + // has an answer it wins, so a moved pane lights the tab it is IN. + // `tabId` is a deprecated alias of the leaf, so it is a leaf candidate, NOT + // a tab candidate — except in the one case where it is genuinely a root tab + // id (the second half of the loop body). for (const leaf of [detail.rendererTerminalId, detail.tabId]) { if (!leaf) continue; const owner = findTabIdByTerminalId(treesByTabId, leaf); @@ -45,6 +46,16 @@ export function resolveActivityTabId( if (knownTabIds.has(leaf)) return leaf; } + // 2. The backend's owner, now a HINT rather than the first answer: used only + // where the tree has none. That is a real case, not a formality — an + // API-created pane's first write can beat the renderer's own insertion into + // the tree, and the sidecar/headless paths never enter it at all. Still + // gated on the tab being open, so a closed tab cannot resurrect an + // indicator. + if (detail.owningTabId && knownTabIds.has(detail.owningTabId)) { + return detail.owningTabId; + } + // 3. Last resort: an event from a build that only sent the process id. This // matches nothing on the in-process path (leaves are never `pc-*`), and is // kept only for the sidecar path, where the map key IS the leaf. diff --git a/src/renderer/services/paneOwnership.ts b/src/renderer/services/paneOwnership.ts new file mode 100644 index 0000000..f982124 --- /dev/null +++ b/src/renderer/services/paneOwnership.ts @@ -0,0 +1,127 @@ +/** + * Keeps the BACKEND's `owning_tab_id` in step with the renderer pane tree. + * + * The backend records a terminal's owning tab once, at spawn. Moving a pane + * changes its tab but NOT its identity — the leaf travels with the pane — so + * nothing downstream notices and the stored owner keeps naming the tab the pane + * left. That stale owner is echoed by `get_terminal_detail`/`get_my_terminal`, + * and the MCP tool descriptions tell an agent to pass it straight back when it + * creates a sibling pane, so the agent's next pane lands in the wrong tab + * (external review 099, T2-F2). It is also emitted on + * `terminal:external-activity`, lighting the wrong tab. + * + * WHY THIS HANGS OFF THE TREE, NOT OFF A LIFECYCLE HOOK + * The obvious hook — `TerminalService.bindProcess` — is not enough: a moved pane + * already has a mapping, so `TerminalPane` takes its reuse path and never binds + * (TerminalPane.tsx:167-202). Rather than chase every dispatch site, this + * derives the answer from `panes.treesByTabId` itself — the authority on + * ownership — and pushes only what actually CHANGED. Every reparent path is then + * covered by construction: + * - same-window drag `movePaneToTab` (PaneDragController.tsx) + * - cross-window drop `insertPaneIntoTab` (dnd/detach.ts) + * - detached window boot / + * whole-tab reattach `addTabTree` (dnd/detach.ts) + * - reload/restore reattach `setPaneTree` + `addTabTree` + * (StateManager / TerminalContainer) + * - any future programmatic move, without a new call site. + * + * Reads its two collaborators off `window` (the bridge and the terminal service, + * both installed by `index.tsx` at bootstrap) exactly as App.tsx does, so the + * module adds no import edge into the store's graph and no import-order hazard. + */ +import type { PaneNode } from '../store/slices/panesSlice'; + +/** renderer leaf id (`tb-*` root, `tm-*` split) -> the tab that owns it. */ +export type LeafOwners = Map; + +export interface OwnerChange { + rendererTerminalId: string; + owningTabId: string; +} + +/** Flatten every tab's pane tree into leaf -> owning tab. */ +export function collectLeafOwners(treesByTabId: Record): LeafOwners { + const owners: LeafOwners = new Map(); + const walk = (node: PaneNode | null | undefined, tabId: string): void => { + if (!node) return; + if (node.type === 'terminal' && node.terminalId) owners.set(node.terminalId, tabId); + node.children?.forEach((child) => walk(child, tabId)); + }; + for (const tabId of Object.keys(treesByTabId)) walk(treesByTabId[tabId], tabId); + return owners; +} + +/** + * The ownership updates this window owes the backend. + * + * Two kinds of leaf need one, and only these two: + * + * (a) A leaf this window already tracked now sits under a different tab — a + * same-window move. + * (b) A leaf this window has never seen that ALREADY has a live process + * binding — it arrived from somewhere else (cross-window drop, detached + * window boot, reload reattach), so its backend owner names the tab it + * came from. + * + * The binding test is what separates (b) from a freshly split pane: a new pane's + * leaf enters the tree BEFORE `TerminalPane` spawns its PTY, so it has no + * binding and needs nothing — the spawn itself carries the right owner. Pushing + * for it anyway would be a harmless no-op backend-side, but it would also mean + * every startup fired one invoke per pane for nothing. + */ +export function diffOwnerChanges( + previous: LeafOwners | null, + next: LeafOwners, + hasLiveProcess: (rendererTerminalId: string) => boolean, +): OwnerChange[] { + const changes: OwnerChange[] = []; + for (const [leaf, owningTabId] of next) { + const before = previous?.get(leaf); + if (before === owningTabId) continue; + if (before !== undefined || hasLiveProcess(leaf)) { + changes.push({ rendererTerminalId: leaf, owningTabId }); + } + } + return changes; +} + +/** The slice of the store this needs — structural, so no import of the store. */ +interface PaneOwnershipStore { + getState: () => { panes: { treesByTabId: Record } }; + subscribe: (listener: () => void) => () => void; +} + +/** + * Watch `panes.treesByTabId` and push every ownership change to the backend. + * Returns the store's unsubscribe. + * + * Best-effort and fire-and-forget: a failed update costs the correct routing of + * a later API/MCP call, never the move itself. + */ +export function attachPaneOwnershipSync(store: PaneOwnershipStore): () => void { + // Trees are immutable per change (RTK/immer), so an identity check keeps every + // unrelated dispatch — every keystroke-driven action — down to one comparison. + let lastTrees: Record | null = null; + let lastOwners: LeafOwners | null = null; + + return store.subscribe(() => { + const trees = store.getState().panes.treesByTabId; + if (trees === lastTrees) return; + lastTrees = trees; + + const next = collectLeafOwners(trees); + const terminalService = (window as any).terminalService; + const changes = diffOwnerChanges(lastOwners, next, (leaf) => !!terminalService?.getProcessId?.(leaf)); + lastOwners = next; + + for (const change of changes) { + window.electronAPI?.setTerminalOwningTab?.(change.rendererTerminalId, change.owningTabId) + ?.catch((e: unknown) => { + console.warn( + `Failed to re-parent terminal ${change.rendererTerminalId} to tab ${change.owningTabId}`, + e, + ); + }); + } + }); +} diff --git a/src/renderer/store/index.ts b/src/renderer/store/index.ts index f709b8f..70122d5 100644 --- a/src/renderer/store/index.ts +++ b/src/renderer/store/index.ts @@ -6,6 +6,7 @@ import layoutsReducer from './slices/layoutsSlice'; import uiReducer from './slices/uiSlice'; import zoomReducer from './slices/zoomSlice'; import peersReducer from './slices/peersSlice'; +import { attachPaneOwnershipSync } from '../services/paneOwnership'; // Simple logging middleware for debugging const loggingMiddleware = (storeAPI: any) => (next: any) => (action: any) => { @@ -54,6 +55,12 @@ if (typeof window !== 'undefined') { } w.__TAB_PANES__ = w.tabPanes; }); + + // Tell the backend when a pane changes tab (review 099 T2-F2). Driven off the + // pane tree rather than off the individual move/attach dispatch sites, so no + // reparent path can be added later that forgets to report itself — see + // services/paneOwnership.ts. + attachPaneOwnershipSync(store); } export type RootState = ReturnType; diff --git a/src/renderer/types/electron.d.ts b/src/renderer/types/electron.d.ts index 679560f..72ec6bb 100644 --- a/src/renderer/types/electron.d.ts +++ b/src/renderer/types/electron.d.ts @@ -147,6 +147,13 @@ export interface ElectronAPI { * TermFlow instead of behind it. Optional — desktop bridges only. */ adoptConsoleWindow?: (processId: string) => Promise; + /** + * Re-point a live terminal's owning tab after its pane was moved into another + * tab (same-window drag, cross-window drop, detached-window boot). Keyed by + * the renderer LEAF, since that is what the pane tree holds. Optional — only + * bridges backed by a real terminal registry implement it. + */ + setTerminalOwningTab?: (rendererTerminalId: string, owningTabId: string) => Promise; // P0a active-window routing: which window receives API/MCP-created terminals. // Optional — only the Tauri bridge implements it (browser bridge is single-window). getActiveWindow?: () => Promise; From ce8cf1bae87cf7b7fb16856dd9f341dd5655fc06 Mon Sep 17 00:00:00 2001 From: Tam Tran Date: Sat, 15 Aug 2026 01:17:18 -0500 Subject: [PATCH 19/22] fix(p0a): correct legacy leaf fallback and widen ElectronAPI createTerminal sig Fixes review 099 non-blocking findings T2-F3 and T2-F4: - resolveApiCreateIds' legacy-backend fallback used to set leafId = owningTabId. Every caller that reads leafId (App.tsx Mode 1/Mode 2) mints a sibling pane in a tab that may already have an occupied root pane at leaf === owningTabId, so that fallback handed the new pane the root's own leaf -- overwriting the root's TerminalService mapping and duplicating its pane-tree identity for a legacy split event. Fall back to the unique processId instead, matching the pre-P0-A behaviour (a process id briefly doubling as a leaf until StateManager.sanitizeLayoutData remaps it to a fresh tm-* on next restore) -- a known, already-handled degradation rather than a fresh collision. Corrected the "keeps working" claim in the function's doc comment and updated/added unit tests. - Widened the local ElectronAPI.createTerminal interface in tauri-bridge.ts to seven params (added owningTabId), matching both the implementation (which already forwards it) and the global electron.d.ts declaration. --- src/renderer/api/tauri-bridge.ts | 2 +- .../services/__tests__/apiCreatedTab.test.ts | 16 ++++++++++--- src/renderer/services/apiCreatedTab.ts | 24 +++++++++++++++---- 3 files changed, 33 insertions(+), 9 deletions(-) diff --git a/src/renderer/api/tauri-bridge.ts b/src/renderer/api/tauri-bridge.ts index 832b4a9..ee4870f 100644 --- a/src/renderer/api/tauri-bridge.ts +++ b/src/renderer/api/tauri-bridge.ts @@ -44,7 +44,7 @@ interface ElectronAPI { getTerminalSnapshot: (terminalId: string, cols?: number, rows?: number) => Promise; getTerminalFullScrollback: (terminalId: string) => Promise<{ blob: string; rows: number; cols: number }>; getActiveProcesses: () => Promise; - createTerminal: (profile?: string, name?: string, cwd?: string, tabId?: string, cols?: number, rows?: number) => Promise; + createTerminal: (profile?: string, name?: string, cwd?: string, tabId?: string, cols?: number, rows?: number, owningTabId?: string) => Promise; /// Windows: make THIS window the owner of the shell's ConPTY pseudo-console /// window, so dialogs a console program parents to `GetConsoleWindow()` (the /// `az login` WAM prompt) open in front instead of behind the app. Fired on diff --git a/src/renderer/services/__tests__/apiCreatedTab.test.ts b/src/renderer/services/__tests__/apiCreatedTab.test.ts index 89d175d..9dd41fe 100644 --- a/src/renderer/services/__tests__/apiCreatedTab.test.ts +++ b/src/renderer/services/__tests__/apiCreatedTab.test.ts @@ -88,11 +88,21 @@ describe('resolveApiCreateIds', () => { }); // A payload from a build that predates P0-A: `terminalId` was the process id - // and `tabId` the owning tab, with no leaf at all. - it('falls back to the legacy keys', () => { + // and `tabId` the owning tab, with no leaf at all. The leaf falls back to the + // unique process id — NOT the owning tab id — because every caller that reads + // `leafId` (App.tsx Mode 1/Mode 2) is minting a sibling pane in a tab that may + // already have an occupied root pane at leaf === owningTabId; reusing that + // leaf would duplicate the root's pane-tree identity (review 099 T2-F3). + it('falls back to the legacy process id as the leaf (not the owning tab id)', () => { expect( resolveApiCreateIds({ terminalId: 'pc-legacy', tabId: 'tb-legacy1' }), - ).toEqual({ processId: 'pc-legacy', leafId: 'tb-legacy1', owningTabId: 'tb-legacy1' }); + ).toEqual({ processId: 'pc-legacy', leafId: 'pc-legacy', owningTabId: 'tb-legacy1' }); + }); + + it('falls back to the owning tab id as a last resort when even the process id is missing', () => { + expect( + resolveApiCreateIds({ tabId: 'tb-legacy1' }), + ).toEqual({ processId: undefined, leafId: 'tb-legacy1', owningTabId: 'tb-legacy1' }); }); it('reports missing ids as undefined rather than inventing them', () => { diff --git a/src/renderer/services/apiCreatedTab.ts b/src/renderer/services/apiCreatedTab.ts index 0a6bd6d..f87d145 100644 --- a/src/renderer/services/apiCreatedTab.ts +++ b/src/renderer/services/apiCreatedTab.ts @@ -65,7 +65,10 @@ export interface ApiCreateIds { * backend PROCESS id (api_server.rs `create_terminal` emit), unlike a REST * response where it is the leaf — which is exactly why P0-A added the explicit * `processId` / `rendererTerminalId` / `owningTabId` keys. The legacy keys are - * still read so an event from an older backend keeps working. + * still read so an event from an older backend does not crash — but review 099 + * T2-F3 found that "keeps working" was too strong a claim for a legacy SPLIT + * event: see the leafId fallback below for the corrected (degraded, not + * regressed) behaviour. */ export function resolveApiCreateIds(detail: { terminalId?: string; @@ -75,11 +78,22 @@ export function resolveApiCreateIds(detail: { owningTabId?: string; }): ApiCreateIds { const owningTabId = detail.owningTabId ?? detail.tabId; + const processId = detail.processId ?? detail.terminalId; return { - processId: detail.processId ?? detail.terminalId, - // Before P0-A no leaf was sent; the owning tab was the only renderer id - // available, and it IS the leaf for a root pane. - leafId: detail.rendererTerminalId ?? owningTabId, + processId, + // Before P0-A no leaf was sent. Every consumer of `leafId` (App.tsx Mode 1 + // and Mode 2) is minting a NEW sibling pane in a tab that may already have + // an occupied root pane whose leaf === owningTabId — so falling back to + // owningTabId here would hand that new pane the root's own leaf, which + // App.tsx then rebinds in TerminalService and inserts a second pane-tree + // node carrying it, corrupting both the root's PTY mapping and the tree's + // identity uniqueness (review 099 T2-F3). Fall back to the unique + // `processId` instead: this reproduces the exact pre-P0-A behaviour, where + // a process id briefly doubles as a leaf until StateManager.sanitizeLayoutData + // remaps it to a fresh `tm-*` on the next restore (design 011 §6) — a known, + // already-handled degradation, not a fresh collision. `owningTabId` remains + // the final fallback only for the no-ids-at-all case. + leafId: detail.rendererTerminalId ?? processId ?? owningTabId, owningTabId, }; } From a1530443fd92ebb1d819e2259f23e26d7c36824c Mon Sep 17 00:00:00 2001 From: Tam Tran Date: Sat, 15 Aug 2026 01:53:02 -0500 Subject: [PATCH 20/22] fix(api): reserve the root leaf on the renderer create path too (review 101 F1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The reservation added in 6941b4c lived only in api_server::create_terminal, so it serialised the REST path against itself and left the renderer's own create outside it entirely. A restart-in-place of a dead tab root spawns with renderer_terminal_id == owning_tab_id == tb-a; a REST create for the same tab landing before spawn_terminal's final terminals.insert scanned the tab as empty and took tb-a as its leaf too, registering one live leaf twice. Takes the same claim here, which closes the renderer-first ordering: the REST path's try_claim then returns None and correctly mints a tm- split. We claim but never refuse on contention — this call is a user action on a pane that already owns its leaf and must not fail — so the reverse ordering stays open by design. That one is a product question (which creator wins a contested root leaf), not a lock, and the warning makes it observable instead of silent. Options written up in fabric docs/progress/010. Decision extracted as a pure fn so it is testable without a tauri::State. --- src-tauri/src/commands.rs | 116 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 116 insertions(+) diff --git a/src-tauri/src/commands.rs b/src-tauri/src/commands.rs index 0139797..06b8316 100644 --- a/src-tauri/src/commands.rs +++ b/src-tauri/src/commands.rs @@ -69,6 +69,23 @@ pub fn get_os_build_number() -> u32 { 0 } +/// Which owner, if any, this create must reserve before it spawns. +/// +/// `Some(owner)` exactly when the spawn will register a terminal whose renderer +/// leaf IS a tab id — the only case that can collide with another creator, since +/// a `tm-` split leaf is freshly minted and unique by construction. Pure so the +/// decision can be tested without a `tauri::State`. +fn root_leaf_owner_to_reserve(tab_id: Option<&str>, owning_tab_id: Option<&str>) -> Option { + match (tab_id, owning_tab_id) { + // A tab root: the leaf IS the owner (design 011 §3). + (Some(leaf), Some(owner)) if leaf == owner => Some(owner.to_string()), + // A renderer that predates P0-A sends no owner; a `tb-` leaf is a root. + (Some(leaf), None) if leaf.starts_with("tb-") => Some(leaf.to_string()), + // A split leaf, or no leaf at all — nothing to reserve. + _ => None, + } +} + #[tauri::command] pub async fn create_terminal( state: State<'_, AppState>, @@ -108,6 +125,41 @@ pub async fn create_terminal( let terminal_name = format!("Terminal-{}", shell_name); + // Reserve the owner across THIS spawn too (external review 101, F1). + // + // `6941b4c` put the reservation only in `api_server::create_terminal`, which + // serialised the REST path against itself but left this path — the renderer's + // own create — outside it entirely. A restart-in-place of a dead tab root + // spawns with `renderer_terminal_id == owning_tab_id == tb-a`; a REST create + // for `tb-a` landing in the window before `spawn_terminal`'s final + // `terminals.insert` would scan the tab as empty and take `tb-a` as its leaf + // too, registering the same live leaf twice. Taking the same claim here + // closes the renderer-first ordering: the REST path's `try_claim` then + // returns `None` and it correctly mints a `tm-` split leaf instead. + // + // We claim but never REFUSE on contention: this call is a user action on a + // pane that already exists and owns its leaf, so it must not fail. The + // reverse ordering — a REST create winning the claim and committing to `tb-a` + // before this spawn registers — is therefore still open, and closing it is a + // design question (which creator wins a contested root leaf), not a lock: + // see docs/progress/010 for the options. The warning below is what makes that + // window observable instead of silent. + let root_leaf_owner = root_leaf_owner_to_reserve(tab_id.as_deref(), owning_tab_id.as_deref()); + // Held to the end of this command (and dropped on the sidecar path's early + // return) — releasing it before `spawn_terminal` has registered would reopen + // the very window it exists to cover. + let _root_leaf_claim = root_leaf_owner.as_deref().and_then(|owner| { + let claim = state.root_leaf_claims.try_claim(owner); + if claim.is_none() { + log::warn!( + "create_terminal: root leaf {owner} is already claimed by an in-flight create; \ + proceeding because a renderer create owns its pane, but this is the contested \ + ordering external review 101 F1 describes" + ); + } + claim + }); + // Opt-in PTY-host sidecar path (Windows). Requires a stable tab_id as the // reattach key; without one we fall through to the in-process path. if crate::pty_host_client::enabled() { @@ -2352,3 +2404,67 @@ mod host_identity_tests { assert_eq!(owner, "tm-9f2c1a4b7"); } } + +/// The renderer create path's own root-leaf reservation (external review 101, F1). +/// +/// Plain `#[cfg(test)]` — nothing here needs tauri's `test` feature, which breaks +/// the Windows test binary at loader time (see the gate on +/// `scrollback_restore_tests` above). +#[cfg(test)] +mod root_leaf_reservation_tests { + use super::root_leaf_owner_to_reserve; + use crate::state::RootLeafClaims; + use std::sync::Arc; + + #[test] + fn a_tab_root_reserves_its_own_id() { + // leaf == owner is the definition of a tab root (design 011 §3), and it + // is the only shape the REST path can also decide to take. + assert_eq!( + root_leaf_owner_to_reserve(Some("tb-a1b2c3"), Some("tb-a1b2c3")), + Some("tb-a1b2c3".to_string()), + ); + } + + #[test] + fn a_split_pane_reserves_nothing() { + // A `tm-` leaf is minted fresh, so it cannot collide and must not take a + // claim — doing so would stall an unrelated root create for no gain. + assert_eq!( + root_leaf_owner_to_reserve(Some("tm-9f2c1a4"), Some("tb-a1b2c3")), + None, + ); + } + + #[test] + fn a_pre_p0a_renderer_sending_no_owner_still_reserves_a_tb_leaf() { + // `owning_tab_id` is Optional precisely so an older renderer keeps + // working; a `tb-` leaf from one of those IS a tab root. + assert_eq!( + root_leaf_owner_to_reserve(Some("tb-a1b2c3"), None), + Some("tb-a1b2c3".to_string()), + ); + assert_eq!(root_leaf_owner_to_reserve(Some("tm-9f2c1a4"), None), None); + assert_eq!(root_leaf_owner_to_reserve(None, None), None); + } + + #[test] + fn the_renderer_and_the_rest_path_cannot_both_hold_one_owner() { + // The whole point of F1's fix: these two paths now contend for the SAME + // map, so whichever gets there first excludes the other. Before the fix + // the renderer path never touched this map at all, so both could commit + // to the same live leaf. + let claims: Arc = Arc::new(RootLeafClaims::default()); + let renderer = claims.try_claim("tb-a1b2c3"); + assert!(renderer.is_some(), "the first creator reserves the owner"); + assert!( + claims.try_claim("tb-a1b2c3").is_none(), + "a concurrent REST create must be refused and mint a tm- split leaf", + ); + drop(renderer); + assert!( + claims.try_claim("tb-a1b2c3").is_some(), + "the owner is free again once the winning spawn has registered", + ); + } +} From ecbeee0e228c4a3d3198beb8b4fa56334b8e851c Mon Sep 17 00:00:00 2001 From: Tam Tran Date: Sat, 15 Aug 2026 01:53:12 -0500 Subject: [PATCH 21/22] fix(panes): re-assert a pane's owner once its spawn registers (review 101 F2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A pane dragged between tabs while its own create is still in flight loses the move for the rest of the session. The tree subscription fires, but the backend can only retarget a terminal it has already registered and spawn_terminal registers LAST, so set_terminal_owning_tab matches nothing — and it reports that as Ok(()), which it must, since the renderer fires off its own tree lifecycle and a pane's PTY may legitimately not exist. The subscription has meanwhile advanced lastOwners, so no later tree change re-sends it. The pane then sits visibly in the new tab while get_terminal_detail keeps naming the old one, and an agent told to pass owningTabId back creates its next pane in the wrong tab. Re-asserts the tree's current owner at the one moment the race resolves: the create has returned, so the leaf IS registered. Costs zero IPC in the common case, where the owner the spawn carried still matches the tree. Wired at TerminalService.createTerminal, the single choke point every renderer create passes through. Tested on both sides — the rule in paneOwnershipSync, and the wiring in TerminalService, because 'nobody called them' is exactly how this regressed the first time. --- src/renderer/services/TerminalService.ts | 10 ++++ .../__tests__/TerminalService.test.ts | 48 ++++++++++++++++ .../__tests__/paneOwnershipSync.test.ts | 57 ++++++++++++++++++- src/renderer/services/paneOwnership.ts | 46 +++++++++++++++ 4 files changed, 160 insertions(+), 1 deletion(-) diff --git a/src/renderer/services/TerminalService.ts b/src/renderer/services/TerminalService.ts index 032db24..7f79466 100644 --- a/src/renderer/services/TerminalService.ts +++ b/src/renderer/services/TerminalService.ts @@ -1,5 +1,6 @@ import { termDiag } from '../utils/diag'; import { clearZoom } from '../store/slices/zoomSlice'; +import { reassertOwnerAfterSpawn } from './paneOwnership'; import type { PromptGate } from '@termflow/terminal-core'; export interface TerminalProcess { @@ -121,6 +122,15 @@ class TerminalServiceClass { this.bindProcess(terminalId, processId); console.log(`TerminalService: Mapped terminal ${terminalId} to process ${processId}`); + // The spawn carried the owner resolved BEFORE the await, and the backend + // only registers the terminal at the very end of it — so a pane dragged to + // another tab while this create was in flight had its ownership update + // land on a terminal that did not exist yet, and nothing re-sends it + // (external review 101, F2). This is the first moment the leaf is + // registered, so it is where that correction belongs. No-ops unless the + // tree moved under us. + reassertOwnerAfterSpawn(terminalId, owningTabId); + return processId; } catch (error) { console.error('Failed to create terminal:', error); diff --git a/src/renderer/services/__tests__/TerminalService.test.ts b/src/renderer/services/__tests__/TerminalService.test.ts index 437c455..a56e9fa 100644 --- a/src/renderer/services/__tests__/TerminalService.test.ts +++ b/src/renderer/services/__tests__/TerminalService.test.ts @@ -97,3 +97,51 @@ describe('TerminalService.stashPromptGate (backlog 011 hot-swap reattach seed)', expect(terminalService.takePromptGateHandoff('tb-seed-2')).toBeUndefined(); }); }); + +/** + * External review 101, F2 — the WIRING, not the rule. + * + * `paneOwnershipSync.test.ts` covers what `reassertOwnerAfterSpawn` decides. + * This covers the part that regressed the last time around: nobody calling it. + * `createTerminal` is the single choke point every renderer create passes + * through, and the moment it returns is the first moment the backend has the + * terminal registered — which is exactly what a mid-spawn pane move needs. + */ +describe('TerminalService.createTerminal re-asserts pane ownership after the spawn', () => { + const { attachPaneOwnershipSync } = require('../paneOwnership'); + const panesReducer = require('../../store/slices/panesSlice').default; + const { addTabTree, insertPaneIntoTab } = require('../../store/slices/panesSlice'); + const { configureStore } = require('@reduxjs/toolkit'); + + let setTerminalOwningTab: jest.Mock; + let unsubscribe: () => void; + + beforeEach(() => { + setTerminalOwningTab = jest.fn().mockResolvedValue(undefined); + (window as any).electronAPI = { + createTerminal: jest.fn().mockResolvedValue('pc-reassert-1'), + setTerminalOwningTab, + }; + const store = configureStore({ reducer: { panes: panesReducer } }); + unsubscribe = attachPaneOwnershipSync(store); + // The pane is born under tb-src, then dragged to tb-dst while its create is + // still in flight — so the owner the spawn carries is already stale by the + // time the backend registers the terminal. + store.dispatch(addTabTree({ tabId: 'tb-src', tree: { id: 'pn-a', type: 'terminal', terminalId: 'tm-reassert' } })); + store.dispatch(addTabTree({ tabId: 'tb-dst', tree: { id: 'pn-b', type: 'terminal', terminalId: 'tb-dst' } })); + store.dispatch(insertPaneIntoTab({ + tabId: 'tb-dst', + targetPaneId: 'pn-b', + zone: 'right', + node: { id: 'pn-a', type: 'terminal', terminalId: 'tm-reassert' }, + })); + setTerminalOwningTab.mockClear(); + }); + + afterEach(() => unsubscribe()); + + it('pushes the tree\'s current owner, not the one the spawn carried', async () => { + await terminalService.createTerminal('tm-reassert', 'default', undefined, undefined, undefined, undefined, 'tb-src'); + expect(setTerminalOwningTab).toHaveBeenCalledWith('tm-reassert', 'tb-dst'); + }); +}); diff --git a/src/renderer/services/__tests__/paneOwnershipSync.test.ts b/src/renderer/services/__tests__/paneOwnershipSync.test.ts index 1708311..13a5d82 100644 --- a/src/renderer/services/__tests__/paneOwnershipSync.test.ts +++ b/src/renderer/services/__tests__/paneOwnershipSync.test.ts @@ -13,7 +13,7 @@ import panesReducer, { movePaneToTab, PaneNode, } from '../../store/slices/panesSlice'; -import { attachPaneOwnershipSync } from '../paneOwnership'; +import { attachPaneOwnershipSync, reassertOwnerAfterSpawn } from '../paneOwnership'; const leaf = (id: string, terminalId: string): PaneNode => ({ id, type: 'terminal', terminalId }); @@ -112,3 +112,58 @@ describe('attachPaneOwnershipSync', () => { expect(setTerminalOwningTab).toHaveBeenCalledTimes(1); }); }); + +/** + * External review 101, F2 — the move a spawn swallows. + * + * The subscription can only tell the backend about a leaf the backend has + * already registered, and `spawn_terminal` registers LAST. A pane dragged while + * its own create is still in flight therefore gets an update that lands on + * nothing, and because `lastOwners` has already advanced no later tree change + * re-sends it. `reassertOwnerAfterSpawn` is the repair, fired at the one moment + * the leaf is known to be registered. + */ +describe('reassertOwnerAfterSpawn', () => { + it('re-sends the owner when the pane moved while its create was in flight', () => { + // The pane is created under tb-a; that is the owner the spawn carries. + store.dispatch(addTabTree({ tabId: 'tb-a', tree: leaf('pn-1', 'tm-x') })); + store.dispatch(addTabTree({ tabId: 'tb-b', tree: leaf('pn-2', 'tb-b') })); + // Mid-spawn drag to tb-b. No process is bound yet, so the subscription's + // own update either never fires or lands on an unregistered leaf. + store.dispatch( + insertPaneIntoTab({ + tabId: 'tb-b', + targetPaneId: 'pn-2', + zone: 'right', + node: leaf('pn-1', 'tm-x'), + }), + ); + setTerminalOwningTab.mockClear(); + + // The create now returns. It carried tb-a; the tree says tb-b. + reassertOwnerAfterSpawn('tm-x', 'tb-a'); + + expect(setTerminalOwningTab).toHaveBeenCalledWith('tm-x', 'tb-b'); + }); + + it('sends nothing when the tree still agrees with the owner the spawn carried', () => { + store.dispatch(addTabTree({ tabId: 'tb-a', tree: leaf('pn-1', 'tm-x') })); + setTerminalOwningTab.mockClear(); + + reassertOwnerAfterSpawn('tm-x', 'tb-a'); + + // The common case is every ordinary create; it must cost zero IPC. + expect(setTerminalOwningTab).not.toHaveBeenCalled(); + }); + + it('sends nothing for a leaf this window does not hold', () => { + store.dispatch(addTabTree({ tabId: 'tb-a', tree: leaf('pn-1', 'tm-x') })); + setTerminalOwningTab.mockClear(); + + // Detached to another window, or a tree not committed yet: this window has + // no correction to offer and must not guess one. + reassertOwnerAfterSpawn('tm-gone', 'tb-a'); + + expect(setTerminalOwningTab).not.toHaveBeenCalled(); + }); +}); diff --git a/src/renderer/services/paneOwnership.ts b/src/renderer/services/paneOwnership.ts index f982124..e501727 100644 --- a/src/renderer/services/paneOwnership.ts +++ b/src/renderer/services/paneOwnership.ts @@ -91,6 +91,51 @@ interface PaneOwnershipStore { subscribe: (listener: () => void) => () => void; } +/** + * The store `attachPaneOwnershipSync` was given, so `reassertOwnerAfterSpawn` + * can read the tree without taking an import edge into the store graph — the + * same constraint the module header states. Null until bootstrap has run, which + * makes the re-assert a safe no-op for anything created before then. + */ +let ownershipStore: PaneOwnershipStore | null = null; + +/** + * Push a leaf's CURRENT owner once its PTY is actually registered. + * + * Closes the window external review 101 F2 describes. The subscription above + * fires on the tree change, but the backend can only retarget a terminal it has + * already registered, and `spawn_terminal` registers LAST — so a pane dragged + * between tabs while its own spawn is still in flight gets an update that hits + * nothing. `set_terminal_owning_tab` treats an unmatched leaf as a successful + * no-op (it must: the renderer fires off its own tree lifecycle, and a pane's + * PTY may legitimately not exist), and the subscription has already advanced + * `lastOwners`, so no later tree change re-sends it. The move is then lost for + * the rest of the session: the pane sits visibly in the new tab while + * `get_terminal_detail` keeps naming the old one. + * + * Called at the one moment that race resolves — the create has returned, so the + * terminal IS registered. Sends nothing in the common case, where the owner the + * spawn carried is still the owner the tree holds. + */ +export function reassertOwnerAfterSpawn( + rendererTerminalId: string, + ownerSentAtSpawn: string | undefined, +): void { + if (!ownershipStore) return; + const current = collectLeafOwners(ownershipStore.getState().panes.treesByTabId).get( + rendererTerminalId, + ); + // No entry means the pane left this window (or the tree has not been committed + // yet); either way this window has no correction to offer. + if (!current || current === ownerSentAtSpawn) return; + window.electronAPI?.setTerminalOwningTab?.(rendererTerminalId, current)?.catch((e: unknown) => { + console.warn( + `Failed to re-assert owner ${current} for terminal ${rendererTerminalId} after spawn`, + e, + ); + }); +} + /** * Watch `panes.treesByTabId` and push every ownership change to the backend. * Returns the store's unsubscribe. @@ -99,6 +144,7 @@ interface PaneOwnershipStore { * a later API/MCP call, never the move itself. */ export function attachPaneOwnershipSync(store: PaneOwnershipStore): () => void { + ownershipStore = store; // Trees are immutable per change (RTK/immer), so an identity check keeps every // unrelated dispatch — every keystroke-driven action — down to one comparison. let lastTrees: Record | null = null; From 20bbe6e3ce52e7755b131e4dfe3a00e0622d24a2 Mon Sep 17 00:00:00 2001 From: Tam Tran Date: Sat, 15 Aug 2026 01:58:33 -0500 Subject: [PATCH 22/22] ci: stop setup-bun fighting the in-use bun.exe on the self-hosted runner MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both e2e jobs run on [self-hosted] — the Windows box where bun is already installed and usually already running. oven-sh/setup-bun downloads a fresh copy and fails to overwrite it with 'EBUSY: resource busy or locked', which shows up as a random red X on an otherwise green PR. It is what failed this PR's e2e run; nothing in the suite itself broke. rust-tests.yml has carried the fix for a while and e2e-tests.yml was simply missed. Ported verbatim rather than invented: install on GitHub-hosted, verify on Windows, so a future move off self-hosted still gets bun. --- .github/workflows/e2e-tests.yml | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/.github/workflows/e2e-tests.yml b/.github/workflows/e2e-tests.yml index afc77d7..80612c0 100644 --- a/.github/workflows/e2e-tests.yml +++ b/.github/workflows/e2e-tests.yml @@ -28,9 +28,21 @@ jobs: with: node-version: '18' - - name: Setup Bun + # This job runs on the self-hosted Windows box, where bun is already + # installed and usually already RUNNING — oven-sh/setup-bun then fails + # trying to overwrite the in-use bun.exe with `EBUSY: resource busy or + # locked`, which surfaces as a random red X on an otherwise green PR. + # rust-tests.yml has carried this workaround for a while; e2e-tests.yml + # was missed. Same two-step shape, so the jobs stay comparable and a + # future move to a GitHub-hosted runner still installs bun. + - name: Setup Bun (GitHub-hosted) + if: runner.os != 'Windows' uses: oven-sh/setup-bun@v2 + - name: Verify Bun (Windows) + if: runner.os == 'Windows' + run: bun --version + - name: Install dependencies run: bun install --frozen-lockfile @@ -95,9 +107,15 @@ jobs: with: node-version: '18' - - name: Setup Bun + # Same EBUSY workaround as the e2e job above — see the comment there. + - name: Setup Bun (GitHub-hosted) + if: runner.os != 'Windows' uses: oven-sh/setup-bun@v2 + - name: Verify Bun (Windows) + if: runner.os == 'Windows' + run: bun --version + - name: Install dependencies run: bun install --frozen-lockfile