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 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(); 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) { diff --git a/src-tauri/src/api_server.rs b/src-tauri/src/api_server.rs index 76a1456..b8c6396 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.tab_id, - "name": t.name, - "profile": t.shell, - "status": "running", - "pid": t.pid, - "createdAt": t.created_at, - "mode": "ui", - "tabId": t.tab_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 @@ -439,11 +452,131 @@ 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. +/// +/// 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 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<(ApiSpawnIdentity, Option), String> { + 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, 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 { + 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 }, root_leaf_claim)) +} + async fn create_terminal( State(state): State, Json(payload): Json, @@ -486,25 +619,44 @@ 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, 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(); + t.owning_tab_id.as_deref() == Some(owner) + || t.renderer_terminal_id.as_deref() == Some(owner) + }) + }, + mint_renderer_id, + ) { + 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, @@ -513,9 +665,18 @@ 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 - ) { + ); + // 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 @@ -526,8 +687,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 @@ -536,21 +704,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.tab_id, - "name": t.name, - "profile": t.shell, - "status": "running", - "pid": t.pid, - "createdAt": t.created_at, - "mode": "ui", - "tabId": t.tab_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() } @@ -643,6 +797,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 @@ -655,13 +835,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.tab_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); } @@ -976,20 +1161,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.tab_id, - "name": t.name, - "profile": t.shell, - "status": "running", - "pid": t.pid, - "createdAt": t.created_at, - "mode": "default", - "tabId": t.tab_id - }))) + (StatusCode::OK, Json(terminal_identity_json(terminal.value(), "default"))) } else { (StatusCode::NOT_FOUND, Json(json!({ "error": "Terminal not found" }))) } @@ -1378,6 +1550,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(); @@ -2013,6 +2189,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, @@ -2022,7 +2204,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, @@ -2032,10 +2215,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", @@ -2043,7 +2223,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, @@ -2051,9 +2234,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.tab_id = Some(tab_id); - } new_id } }; @@ -3125,6 +3305,515 @@ 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")); + } + + /// 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")); + } + + /// 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 + } + + /// 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 + /// `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_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"), + no_competing_create, |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, 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"); + } + + /// 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 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, 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, + "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"); + } + + /// 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_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"); + } + + #[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_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-")); + } + + /// 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_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}"); + 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_competing_create, + no_live_terminals, + &mut mint, + ) + .expect("explicit owner"); + assert_eq!(r.owning_tab_id, "tb-explicit"); + 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_competing_create, no_live_terminals, &mut mint, + ) + .expect("split a"); + let (b, _) = resolve_api_spawn_identity( + Some("tb-shared01"), None, Some("pn-b"), + no_competing_create, 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"))); diff --git a/src-tauri/src/commands.rs b/src-tauri/src/commands.rs index 82060d2..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>, @@ -77,6 +94,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(); @@ -105,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() { @@ -112,6 +167,7 @@ pub async fn create_terminal( return create_host_terminal( state.inner(), tid, + owning_tab_id.clone(), cols, rows, shell_path, @@ -144,6 +200,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(), )?; @@ -185,6 +242,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 @@ -194,6 +279,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 +290,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 +314,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 +338,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 +362,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,7 +404,8 @@ fn register_host_terminal( cols, rows, backend: crate::tmux_manager::TerminalBackend::PortablePty, - tab_id: Some(id.to_string()), + renderer_terminal_id: Some(leaf), + owning_tab_id: Some(owner), last_input_source: None, last_input_at: None, prompt_hook, @@ -347,6 +447,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, @@ -360,6 +461,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, @@ -369,7 +473,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 { @@ -998,7 +1103,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 +2215,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, @@ -2263,3 +2369,102 @@ 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"); + } +} + +/// 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", + ); + } +} 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/pty_manager.rs b/src-tauri/src/pty_manager.rs index 7504499..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,7 +873,8 @@ pub fn spawn_terminal( cols, rows, backend: TerminalBackend::PortablePty, - tab_id: Some(tab_id.unwrap_or_else(|| id.clone())), + 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 diff --git a/src-tauri/src/state.rs b/src-tauri/src/state.rs index a2935e0..b247c92 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). @@ -64,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, @@ -104,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 @@ -275,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(), @@ -329,6 +424,97 @@ 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, + } +} + +/// 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, @@ -345,6 +531,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, @@ -610,7 +797,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 @@ -633,13 +821,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.tab_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 @@ -1693,3 +1885,225 @@ 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")); + } +} + +/// 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; + + #[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); + } +} diff --git a/src/renderer/App.tsx b/src/renderer/App.tsx index 9a44c4f..8d72f77 100644 --- a/src/renderer/App.tsx +++ b/src/renderer/App.tsx @@ -45,8 +45,9 @@ 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 { buildApiCreatedTab } from './services/apiCreatedTab'; +import { getAllTerminalIds, resolveExitedTabId } from './store/slices/paneTreeOps'; +import { resolveActivityTabId, type ExternalActivityDetail } from './services/externalActivity'; +import { buildApiCreatedTab, resolveApiCreateIds } from './services/apiCreatedTab'; import { runningActivityTracker } from './services/RunningActivityTracker'; import { notificationService } from './services/NotificationService'; import { resolveActivation } from './services/notificationRouting'; @@ -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 })); } @@ -888,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. @@ -961,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. @@ -997,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 @@ -1095,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 @@ -1129,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', @@ -1331,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/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 e1f5645..ee4870f 100644 --- a/src/renderer/api/tauri-bridge.ts +++ b/src/renderer/api/tauri-bridge.ts @@ -44,12 +44,16 @@ 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 /// 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; @@ -287,7 +291,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 +300,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, }); }, @@ -303,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 }); }, @@ -715,7 +727,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/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/StateManager.ts b/src/renderer/services/StateManager.ts index 3de77ff..741e419 100644 --- a/src/renderer/services/StateManager.ts +++ b/src/renderer/services/StateManager.ts @@ -7,7 +7,8 @@ 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'; 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 @@ -636,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 => { @@ -694,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)!; @@ -703,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)); @@ -735,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/TerminalService.ts b/src/renderer/services/TerminalService.ts index 7ec221d..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 { @@ -85,7 +86,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,13 +115,22 @@ 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 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__/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__/TerminalService.test.ts b/src/renderer/services/__tests__/TerminalService.test.ts index 8193ed1..a56e9fa 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 }); @@ -64,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__/apiCreatedTab.test.ts b/src/renderer/services/__tests__/apiCreatedTab.test.ts index 2bc7540..9dd41fe 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,55 @@ 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. 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: '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', () => { + expect(resolveApiCreateIds({})).toEqual({ + processId: undefined, leafId: undefined, owningTabId: undefined, + }); + }); +}); diff --git a/src/renderer/services/__tests__/externalActivity.test.ts b/src/renderer/services/__tests__/externalActivity.test.ts new file mode 100644 index 0000000..314f50b --- /dev/null +++ b/src/renderer/services/__tests__/externalActivity.test.ts @@ -0,0 +1,133 @@ +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(); + }); + + // 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( + resolveActivityTabId({ owningTabId: 'tb-closed99' }, trees, knownTabIds), + ).toBeNull(); + }); +}); 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..13a5d82 --- /dev/null +++ b/src/renderer/services/__tests__/paneOwnershipSync.test.ts @@ -0,0 +1,169 @@ +/** + * @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, reassertOwnerAfterSpawn } 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); + }); +}); + +/** + * 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/__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/__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/apiCreatedTab.ts b/src/renderer/services/apiCreatedTab.ts index d306e38..f87d145 100644 --- a/src/renderer/services/apiCreatedTab.ts +++ b/src/renderer/services/apiCreatedTab.ts @@ -49,3 +49,51 @@ 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 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; + tabId?: string; + processId?: string; + rendererTerminalId?: string; + owningTabId?: string; +}): ApiCreateIds { + const owningTabId = detail.owningTabId ?? detail.tabId; + const processId = detail.processId ?? detail.terminalId; + return { + 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, + }; +} diff --git a/src/renderer/services/externalActivity.ts b/src/renderer/services/externalActivity.ts new file mode 100644 index 0000000..eed069b --- /dev/null +++ b/src/renderer/services/externalActivity.ts @@ -0,0 +1,68 @@ +/** + * 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 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); + if (owner && knownTabIds.has(owner)) return owner; + 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. + if (detail.terminalId) { + const owner = findTabIdByTerminalId(treesByTabId, detail.terminalId); + if (owner && knownTabIds.has(owner)) return owner; + } + + return null; +} diff --git a/src/renderer/services/paneOwnership.ts b/src/renderer/services/paneOwnership.ts new file mode 100644 index 0000000..e501727 --- /dev/null +++ b/src/renderer/services/paneOwnership.ts @@ -0,0 +1,173 @@ +/** + * 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; +} + +/** + * 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. + * + * 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 { + 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; + 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/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; +} 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; +} 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 34169a3..72ec6bb 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 @@ -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;