diff --git a/crates/freshell-freshagent/src/codex.rs b/crates/freshell-freshagent/src/codex.rs index df08bcdf..3e33c222 100644 --- a/crates/freshell-freshagent/src/codex.rs +++ b/crates/freshell-freshagent/src/codex.rs @@ -511,10 +511,31 @@ impl FreshCodexState { return; } }; - debug_assert_eq!( - started.thread_id, resume_session_id, - "thread/resume must preserve the requested id" - ); + // TERM-25: never silently proceed against the wrong thread. `thread/resume` + // answering with a different id than requested means the sidecar is sitting on a + // thread the user did NOT ask for -- adopting it (or renaming it) would bind the + // pane to an unrelated conversation. Reject loudly instead. + if started.thread_id != resume_session_id { + client.close().await; + let mut child = child; + let _ = child.start_kill(); + reap_owned_codex_sidecars(&ownership_id); + tracing::error!( + requested = %resume_session_id, + returned = %started.thread_id, + "freshagent.codex.wrong_thread_resume_rejected" + ); + self.fail_create( + &request_id, + "FRESH_AGENT_CREATE_FAILED", + &format!( + "codex thread/resume returned wrong thread id {} (requested {}); \ + refusing to adopt the wrong thread", + started.thread_id, resume_session_id + ), + ); + return; + } let thread_id = resume_session_id; self.clear_dead_thread(&thread_id).await; @@ -1179,10 +1200,26 @@ impl FreshCodexState { return Err(EnsureAliveError::RespawnFailed(err.to_string())); } }; - debug_assert_eq!( - started.thread_id, session_id, - "thread/resume must preserve the requested id" - ); + // TERM-25: never silently proceed against the wrong thread. Re-registering the + // ORIGINAL id against a sidecar that `thread/resume` actually put on a DIFFERENT + // thread would route every subsequent send to an unrelated conversation. Reject + // loudly instead; the caller surfaces it as a respawn failure. + if started.thread_id != session_id { + client.close().await; + let mut child = child; + let _ = child.start_kill(); + reap_owned_codex_sidecars(&ownership_id); + tracing::error!( + requested = %session_id, + returned = %started.thread_id, + "freshagent.codex.wrong_thread_resume_rejected" + ); + return Err(EnsureAliveError::RespawnFailed(format!( + "codex thread/resume returned wrong thread id {} (requested {session_id}); \ + refusing to adopt the wrong thread", + started.thread_id + ))); + } let active_turn: Arc>> = Arc::new(StdMutex::new(None)); let exited = Arc::new(AtomicBool::new(false)); @@ -1689,19 +1726,44 @@ impl FreshCodexState { }, ) .await; - if let Err(err) = resume_result { + let started = match resume_result { + Ok(started) => started, + Err(err) => { + client.close().await; + let mut child = child; + let _ = child.start_kill(); + reap_owned_codex_sidecars(&ownership_id); + if is_codex_thread_not_found(&err) { + // FIX (CODEX-FIRST triage Finding 2): remember this id as genuinely gone + // so a later attach/snapshot-read against it fails fast instead of + // repeating this same spawn-resume-fail cycle. + self.mark_thread_dead(thread_id).await; + return Err(ResumeSessionError::NotFound); + } + return Err(ResumeSessionError::Transient(err.to_string())); + } + }; + + // TERM-25: never silently proceed against the wrong thread. Registering the + // requested id against a sidecar that `thread/resume` actually put on a DIFFERENT + // thread would bind the pane to an unrelated conversation. Reject loudly as a + // transient failure (NOT NotFound -- the requested thread may be perfectly fine; + // the app-server misbehaved), so the client keeps the durable identity. + if started.thread_id != thread_id { client.close().await; let mut child = child; let _ = child.start_kill(); reap_owned_codex_sidecars(&ownership_id); - if is_codex_thread_not_found(&err) { - // FIX (CODEX-FIRST triage Finding 2): remember this id as genuinely gone so - // a later attach/snapshot-read against it fails fast instead of repeating - // this same spawn-resume-fail cycle. - self.mark_thread_dead(thread_id).await; - return Err(ResumeSessionError::NotFound); - } - return Err(ResumeSessionError::Transient(err.to_string())); + tracing::error!( + requested = %thread_id, + returned = %started.thread_id, + "freshagent.codex.wrong_thread_resume_rejected" + ); + return Err(ResumeSessionError::Transient(format!( + "codex thread/resume returned wrong thread id {} (requested {thread_id}); \ + refusing to adopt the wrong thread", + started.thread_id + ))); } // FIX (CODEX-FIRST triage Finding 2): the app-server just proved this id alive -- @@ -4441,6 +4503,194 @@ pub(crate) mod tests { ); } + // -- TERM-25: wrong-thread resume rejection -- + + /// TERM-25 (restore-matrix SCENARIO 7): when `thread/resume` answers with a DIFFERENT + /// thread id than the one requested, the create-with-resume path must REJECT the + /// mismatch loudly -- `freshAgent.create.failed` to the client, no session registered + /// under EITHER id, never a silent proceed against the wrong thread. + #[tokio::test] + async fn handle_create_with_resume_wrong_thread_id_fails_create_and_never_adopts() { + let _guard = ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + configure_fake_codex_cmd(r#"{"threadResumeThreadId":"thread-B-wrong"}"#); + let (st, mut rx) = state_with_bus(); + + st.handle_create(FreshAgentCreate { + request_id: "req-term25-create".to_string(), + session_type: freshell_protocol::SessionType::Freshcodex, + provider: Some(freshell_protocol::AgentProvider::Codex), + cwd: None, + legacy_restore_context: None, + resume_session_id: Some("thread-A-requested".to_string()), + session_ref: None, + model: None, + model_selection: None, + permission_mode: None, + sandbox: None, + effort: None, + plugins: None, + }) + .await; + + let frame: Value = tokio::time::timeout(std::time::Duration::from_secs(15), async { + loop { + let frame: Value = serde_json::from_str(&rx.recv().await.unwrap()).unwrap(); + if frame["type"] == "freshAgent.created" + || frame["type"] == "freshAgent.create.failed" + { + return frame; + } + } + }) + .await + .expect("the fake app-server responds within the budget"); + + assert_eq!( + frame["type"], "freshAgent.create.failed", + "a wrong-thread resume answer must fail the create loudly, never proceed \ + against the wrong thread: {frame}" + ); + assert_eq!(frame["requestId"], "req-term25-create"); + assert_eq!(frame["code"], "FRESH_AGENT_CREATE_FAILED"); + let message = frame["message"].as_str().unwrap_or_default(); + assert!( + message.contains("thread-B-wrong") && message.contains("thread-A-requested"), + "the rejection must name BOTH ids so the mismatch is diagnosable: {frame}" + ); + let sessions = st.sessions.lock().await; + assert!( + !sessions.contains_key("thread-A-requested"), + "the requested id must not be registered against a sidecar on the wrong thread" + ); + assert!( + !sessions.contains_key("thread-B-wrong"), + "the wrong thread id must never be adopted" + ); + } + + /// TERM-25 (restore-matrix SCENARIO 7): the not-tracked `freshAgent.attach` resume + /// path (`ensure_session_resumable`) must likewise reject a wrong-thread answer -- + /// a `CODEX_ATTACH_RESUME_FAILED` error (transient shape, so the frozen client keeps + /// the durable identity instead of abandoning it), never an idle snapshot that + /// silently binds the pane to a sidecar sitting on the wrong thread. + #[tokio::test] + async fn handle_attach_unknown_session_wrong_thread_id_is_rejected_not_adopted() { + let _guard = ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + configure_fake_codex_cmd(r#"{"threadResumeThreadId":"thread-B-wrong"}"#); + let (st, mut rx) = state_with_bus(); + + st.handle_attach(attach_msg("thread-A-requested")).await; + + let frame: Value = tokio::time::timeout(std::time::Duration::from_secs(15), async { + loop { + let frame: Value = serde_json::from_str(&rx.recv().await.unwrap()).unwrap(); + if frame["type"] == "error" || frame["type"] == "freshAgent.event" { + return frame; + } + } + }) + .await + .expect("attach resolves within the budget"); + + assert_eq!( + frame["type"], "error", + "a wrong-thread resume answer must surface as an error, never a session \ + snapshot adopting the wrong thread: {frame}" + ); + let message = frame["message"].as_str().unwrap_or_default(); + assert!( + message.starts_with("CODEX_ATTACH_RESUME_FAILED:"), + "wrong-thread is a resume failure (transient shape -- the client must keep \ + the durable identity), got: {frame}" + ); + assert!( + message.contains("thread-B-wrong") && message.contains("thread-A-requested"), + "the rejection must name BOTH ids so the mismatch is diagnosable: {frame}" + ); + let sessions = st.sessions.lock().await; + assert!( + !sessions.contains_key("thread-A-requested"), + "the requested id must not be registered against a sidecar on the wrong thread" + ); + assert!( + !sessions.contains_key("thread-B-wrong"), + "the wrong thread id must never be adopted" + ); + } + + /// TERM-25 (restore-matrix SCENARIO 7): crash recovery (`ensure_session_alive`'s + /// resume-first path) must also reject a wrong-thread answer -- the tracked session's + /// recovery fails loudly (`CODEX_ATTACH_RESPAWN_FAILED` on the attach surface) instead + /// of silently re-registering the ORIGINAL id against a sidecar on the wrong thread. + #[tokio::test] + async fn crash_recovery_resume_wrong_thread_id_is_rejected_not_silently_recovered() { + let _guard = ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + configure_fake_codex_cmd(r#"{"threadResumeThreadId":"thread-B-wrong"}"#); + let (st, mut rx) = state_with_bus(); + + // A dead in-process client + a child that exits immediately: the exit-watcher + // flips `exited`, so the next attach takes the crash-recovery resume path. + let (transport, _peer) = freshell_codex::new_channel_transport(); + let (client, _notifs) = CodexAppServerClient::connect(transport); + let mut cmd = tokio::process::Command::new("true"); + cmd.kill_on_drop(true); + let child = cmd.spawn().expect("spawn true fixture"); + insert_fake_session( + &st, + "thread-A-crashed", + Arc::new(client), + Arc::new(StdMutex::new(None)), + child, + "codex-sidecar-test-term25-recovery", + ) + .await; + + tokio::time::timeout(std::time::Duration::from_secs(5), async { + loop { + let exited = st + .sessions + .lock() + .await + .get("thread-A-crashed") + .map(|s| s.exited.load(Ordering::SeqCst)) + .unwrap_or(false); + if exited { + return; + } + tokio::time::sleep(std::time::Duration::from_millis(50)).await; + } + }) + .await + .expect("the exit-watcher observes the crash within the budget"); + + st.handle_attach(attach_msg("thread-A-crashed")).await; + + let frame: Value = tokio::time::timeout(std::time::Duration::from_secs(15), async { + loop { + let frame: Value = serde_json::from_str(&rx.recv().await.unwrap()).unwrap(); + if frame["type"] == "error" { + return frame; + } + } + }) + .await + .expect("attach resolves within the budget"); + + let message = frame["message"].as_str().unwrap_or_default(); + assert!( + message.starts_with("CODEX_ATTACH_RESPAWN_FAILED:"), + "wrong-thread crash recovery must fail the respawn loudly, got: {frame}" + ); + assert!( + message.contains("thread-B-wrong") && message.contains("thread-A-crashed"), + "the rejection must name BOTH ids so the mismatch is diagnosable: {frame}" + ); + assert!( + !st.sessions.lock().await.contains_key("thread-B-wrong"), + "the wrong thread id must never be adopted" + ); + } + // -- dead-thread negative cache (CODEX-FIRST triage Finding 2) -- /// Unit-level: [`FreshCodexState::mark_thread_dead`]/[`FreshCodexState::is_known_dead_thread`]/ diff --git a/crates/freshell-sessions/src/directory_index.rs b/crates/freshell-sessions/src/directory_index.rs index d1670721..edce03f2 100644 --- a/crates/freshell-sessions/src/directory_index.rs +++ b/crates/freshell-sessions/src/directory_index.rs @@ -898,7 +898,6 @@ impl SessionIndex { pub async fn warm(&self) { let _ = self.snapshot().await; } - } /// Free-function form of the save-decision gate (see diff --git a/docs/plans/2026-07-14-rust-tauri-parity-completion-checklist.md b/docs/plans/2026-07-14-rust-tauri-parity-completion-checklist.md index 02cec47b..f87481d2 100644 --- a/docs/plans/2026-07-14-rust-tauri-parity-completion-checklist.md +++ b/docs/plans/2026-07-14-rust-tauri-parity-completion-checklist.md @@ -281,7 +281,8 @@ These harness items come first. Later validation descriptions refer to their IDs - [ ] **PARTIAL — TERM-25 — Prevent wrong-thread Codex recovery.** Never render or silently adopt thread B while restoring expected thread A. - **Playwright validation (`PW-RUST`):** Have the fake report B during A restore, assert B output never appears, input stays blocked, expected/actual identity is reported, and only an explicit recovery choice can start fresh or reconnect A. - **EVIDENCE (spec authored, product bug pinned):** `restore-matrix.spec.ts::SCENARIO 7 — TERM-25: wrong-thread Codex recovery is rejected, not silently adopted` — 2x green (expected-fail pinned) per project: legacy-chromium, rust-chromium. Test uses fake app server `threadStartThreadId: 'thread-A'` / `threadResumeThreadId: 'thread-B'` to simulate thread-ID mismatch on resume. FIXME(product-bug): TS adapter (`server/codex/adapter.ts` ~L843-868) has NO check; Rust adapter (`src-tauri/src/codex.rs` ~L1182-1185) uses `debug_assert_eq!` only (no-op in release). Both silently adopt thread-B. - - **REMAINING CLAUSES UNPROVEN:** (1) "Never render or silently adopt thread B" — FAILS (product bug, pinned); (2) "B output never appears" — not yet asserted (requires thread-B to emit distinguishable output); (3) "input stays blocked" — not yet asserted; (4) "expected/actual identity is reported" — not yet asserted; (5) "explicit recovery choice can start fresh or reconnect A" — not yet asserted. Clauses 2-5 are follow-on assertions that depend on clause 1 being fixed first. + - **EVIDENCE (2026-07-24, Rust guard landed + pin removed as test-bug artifact):** the original expected-fail pin was an artifact of a TEST BUG (`originalSessionId` assigned the void result of `expect.poll(...).toBe(...)`, so the final assertion compared against `undefined` and could never pass anywhere), NOT of the pane rendering thread-B. With the capture fixed, SCENARIO 7 is GREEN unpinned on BOTH projects (legacy-chromium, rust-chromium): the pane's durable identity stays 'thread-A' on both kinds. Rust server-side guard landed in `crates/freshell-freshagent/src/codex.rs`: all three resume sites (create-with-resume, not-tracked attach, crash recovery) now REJECT a wrong-thread `thread/resume` answer loudly — sidecar torn down + reaped, explicit error frame (`freshAgent.create.failed` / `CODEX_ATTACH_RESUME_FAILED` / `CODEX_ATTACH_RESPAWN_FAILED`), `tracing::error!` record, wrong id never adopted. Unit pins (RED pre-fix / GREEN post-fix): `handle_create_with_resume_wrong_thread_id_fails_create_and_never_adopts`, `handle_attach_unknown_session_wrong_thread_id_is_rejected_not_adopted`, `crash_recovery_resume_wrong_thread_id_is_rejected_not_silently_recovered`. Legacy adapter remains unguarded internally (frozen; not observable at the UI level this spec asserts). + - **REMAINING CLAUSES UNPROVEN:** (2) "B output never appears" — not yet asserted at e2e level (requires thread-B to emit distinguishable output; the Rust guard makes B-output structurally unreachable — the sidecar is killed before any B event can flow); (3) "input stays blocked" — not yet asserted; (4) "expected/actual identity is reported" — partially: the Rust error message names BOTH ids (requested + returned); no dedicated UI surface; (5) "explicit recovery choice can start fresh or reconnect A" — not yet asserted (the client's generic create-failed retry affordance is the current recovery path). - [ ] **TERM-26 — Resolve the Rust server's native Windows home and terminal `~` correctly.** With no `HOME`/`FRESHELL_HOME`, use `%USERPROFILE%` for config/history and terminal cwd expansion. - **Playwright validation (`PW-RUST` native Windows):** Launch Rust with only isolated `USERPROFILE`, assert seeded config/history, create a terminal at `~/project/`, and assert the fake shell's cwd equals the isolated Windows profile project. diff --git a/docs/plans/2026-07-17-rust-transition-campaign-status.md b/docs/plans/2026-07-17-rust-transition-campaign-status.md index bf75e961..56422ae3 100644 --- a/docs/plans/2026-07-17-rust-transition-campaign-status.md +++ b/docs/plans/2026-07-17-rust-transition-campaign-status.md @@ -102,7 +102,12 @@ d5cf534a naming cluster: PATCH /api/panes/:id, rename cascades, sidebar live-ter 2. **Fresh codex terminal residual sidebar duplicate** — session id unknown at spawn; needs the deferred association scanner (SESSION-09 slice). Documented, pinned by test. 3. **restore-matrix scenario 3** fixme (sidebar seeded-session visibility in that spec; both kinds). + **RESOLVED (evidence 2026-07-24):** fixed by 8fd9233a (see wave 1–6 summary above); re-verified at + ec9970c2 — `restore-matrix.spec.ts::opening a seeded historical session…` green on rust-chromium. 4. **multi-client reconnect flake** — fails on BOTH server kinds + untouched baseline (pre-existing). + **NOT REPRODUCIBLE at ec9970c2 (evidence 2026-07-24):** deflaked by 031a7c12 (over-constrained + exact-attach-intent assertion relaxed with in-spec evidence notes); `multi-client.spec.ts` green + 3x on BOTH kinds (2 quiet runs + 1 under concurrent cargo-build load). 5. **Codex crash-recovery mints a new thread id** (no thread/resume on crash path) — UI continuity ok, model memory not preserved. Follow-up. 6. Checkpoints: create-only ported; list/restore deferred. Directory perf 0.55s multi-provider vs diff --git a/test/e2e-browser/helpers/rust-server.ts b/test/e2e-browser/helpers/rust-server.ts index 194da5de..13d9a8c6 100644 --- a/test/e2e-browser/helpers/rust-server.ts +++ b/test/e2e-browser/helpers/rust-server.ts @@ -328,6 +328,55 @@ export class RustServer implements E2eServerHandle { return this.boot(homeDir, priorInfo.port, priorInfo.token) } + /** + * ABRUPT-death restart (the WSL-restart-like compound mode from + * `docs/plans/2026-07-19-state-sync-resilience-assessment.md` §7): SIGKILL + * the server process group -- NO graceful shutdown, so the server's own + * PTY-reaping `Drop` path (see the class doc comment) never runs and no + * clean WS close frames are sent -- then boot a fresh process bound to the + * SAME home, port, and token (same disk state, same reconnect target). + * + * Because the graceful reap never ran, the fixture-side ownership-safe + * descendant sweep (`reapSurvivingChildren`) is the PRIMARY reap mechanism + * here, not a backstop -- it must run BEFORE the reboot so an orphaned PTY + * child can never linger past the fixture. + */ + async restartAbrupt(): Promise { + const homeDir = this.homeDir + const priorInfo = this._info + if (!homeDir || !priorInfo) throw new Error('RustServer not started; cannot restartAbrupt()') + + const proc = this.process + const pid = proc?.pid + this.process = null + + if (proc && pid) { + const childPidsBeforeKill = ownedDescendantPids(pid) + + await new Promise((resolve) => { + // SIGKILL delivery is effectively immediate, but keep a hard cap so a + // pathological wait can never hang the fixture. + const timeout = setTimeout(resolve, 5000) + proc.once('exit', () => { + clearTimeout(timeout) + resolve() + }) + try { + // Negative pid targets the server's OWN process group only (see + // `killCurrentProcess` for the ownership rationale). + process.kill(-pid, 'SIGKILL') + } catch { + clearTimeout(timeout) + resolve() + } + }) + + await this.reapSurvivingChildren(childPidsBeforeKill) + } + + return this.boot(homeDir, priorInfo.port, priorInfo.token) + } + async stop(): Promise { await this.stopProcess(!this.options.preserveHomeOnStop) } diff --git a/test/e2e-browser/playwright.config.ts b/test/e2e-browser/playwright.config.ts index 59e37dbb..0c968b1e 100644 --- a/test/e2e-browser/playwright.config.ts +++ b/test/e2e-browser/playwright.config.ts @@ -78,6 +78,9 @@ const RUST_ONLY_SPECS = [ /snapshot-restore-rust\.spec\.ts$/, /continuity-smoke\.spec\.ts$/, /deploy-tab-diff-rust\.spec\.ts$/, + // COMPOUND-RESTART: drives RustServer.restartAbrupt() (SIGKILL + reboot), + // an owned-fixture capability the default/legacy seam does not implement. + /compound-restart-rust\.spec\.ts$/, ] export default defineConfig({ @@ -179,6 +182,11 @@ export default defineConfig({ // CONTINUITY TRIO deliverable 3: deploy tab-diff ritual acceptance // (capture -> restart -> verify OK; identity loss fails loudly + remediates). /deploy-tab-diff-rust\.spec\.ts$/, + // COMPOUND-RESTART (state-sync resilience assessment §7's two + // never-tested modes): abrupt SIGKILL death + revival, and server + + // browser restarting together. Rust-only: requires the owned + // RustServer.restartAbrupt() fixture capability. + /compound-restart-rust\.spec\.ts$/, ], }, // CONTINUITY SMOKE (pre-deploy gate): REAL freshell-server binary + REAL diff --git a/test/e2e-browser/specs/compound-restart-rust.spec.ts b/test/e2e-browser/specs/compound-restart-rust.spec.ts new file mode 100644 index 00000000..dbaf2e3d --- /dev/null +++ b/test/e2e-browser/specs/compound-restart-rust.spec.ts @@ -0,0 +1,427 @@ +import fs from 'node:fs/promises' +import os from 'node:os' +import path from 'node:path' +import { fileURLToPath } from 'node:url' +import { test, expect } from '../helpers/fixtures.js' +import { RustServer } from '../helpers/rust-server.js' +import { TestHarness } from '../helpers/test-harness.js' + +/** + * COMPOUND-RESTART -- the two never-tested disruption modes called out by + * `docs/plans/2026-07-19-state-sync-resilience-assessment.md` §7 as "where + * the next incident most likely lives" (Rust only): + * + * MODE A -- ABRUPT SERVER DEATH + REVIVAL (WSL-restart-like): the server + * process is SIGKILLed (no graceful shutdown: no PTY reaping, no clean WS + * close frames, no state flush beyond what is already on disk) and a fresh + * process boots against the SAME disk state / port / token. The live + * browser client never reloads -- it must recover purely via its own WS + * auto-reconnect + terminal re-create round trip. + * + * MODE B -- SERVER AND BROWSER RESTARTING TOGETHER: the server dies + * abruptly and the page reloads IMMEDIATELY after the revived process is + * healthy -- before the old page's own reconnect/restore round can settle. + * This interrupts an in-flight restore with a full client restart, the + * "server + browser at once" compound the assessment flags as the + * highest-risk untested mode. + * + * Both modes drive a terminal CLI pane (codex `resume `) and pin the + * user's bulletproof-restore contract: + * - SAME durable session identity after recovery (pane `sessionRef`); + * - the resume argv is genuinely RE-APPLIED (`codex resume ` appears + * in the fake CLI's argv-log DELTA, not just the pre-kill entries); + * - tab/pane state consistent: same tab count, pane re-anchored to a NEW + * terminal id, status never 'error', real buffer content (never blank); + * - sidebar state consistent: the seeded session's row is linked + * (`data-has-tab="true"`), never a grey orphan; + * - the `terminal_identity_unresolved` invariant WARN + * (`crates/freshell-ws/src/invariants.rs`) never fires. + * + * Rust-only: MODE A requires `RustServer.restartAbrupt()` (SIGKILL + reboot + * on the same home/port/token), an owned-fixture capability the legacy + * TestServer seam does not implement; the frozen legacy tree is not the + * subject of this hardening. Fixture shapes (fake codex CLI, `~/.codex` + * session seed, restart choreography) mirror `codex-terminal-bounce-rust.spec.ts`. + * Helpers are copied, not imported, per this suite's per-spec-ownership + * convention. + */ + +const __filename = fileURLToPath(import.meta.url) +const __dirname = path.dirname(__filename) + +const FAKE_CODEX_CLI_SOURCE = path.resolve(__dirname, '../fixtures/fake-codex-cli.mjs') + +async function installFakeCodexCli(binDir: string): Promise { + await fs.mkdir(binDir, { recursive: true }) + const target = path.join(binDir, 'codex') + await fs.copyFile(FAKE_CODEX_CLI_SOURCE, target) + await fs.chmod(target, 0o755) + return target +} + +async function selectShellIfPickerShowing(page: import('@playwright/test').Page): Promise { + await page.waitForTimeout(500) + const xtermVisible = await page.locator('.xterm').first().isVisible().catch(() => false) + if (xtermVisible) return + const shellNames = ['Shell', 'WSL', 'CMD', 'PowerShell', 'Bash'] + for (const name of shellNames) { + try { + await page.getByRole('button', { name: new RegExp(`^${name}$`, 'i') }).click({ timeout: 5_000 }) + await page.locator('.xterm').first().waitFor({ state: 'visible', timeout: 15_000 }) + return + } catch { + continue + } + } +} + +/** Read the fake CLI's argv-log JSONL (empty array if not yet written). */ +async function readArgvLog(logPath: string): Promise> { + const raw = await fs.readFile(logPath, 'utf8').catch(() => '') + if (!raw) return [] + return raw.trim().split('\n').filter(Boolean).map((line) => JSON.parse(line) as { argv: string[] }) +} + +/** True when the argv tokens contain the adjacent pair `resume `. */ +function hasResumePair(argv: string[], sessionId: string): boolean { + const idx = argv.indexOf('resume') + return idx >= 0 && argv[idx + 1] === sessionId +} + +/** Concatenated content of every server log file in the fixture's logs dir. */ +async function readServerLogs(logsDir: string): Promise { + const names = await fs.readdir(logsDir).catch(() => [] as string[]) + let combined = '' + for (const name of names) { + combined += await fs.readFile(path.join(logsDir, name), 'utf8').catch(() => '') + } + return combined +} + +/** Seed a historical codex session + config into the isolated HOME (the + * `session_meta` + message-records shape `sidebar-click-resume.spec.ts` / + * `codex-terminal-bounce-rust.spec.ts` use). */ +function seedCodexHome(sessionId: string, sessionTitle: string, projectDir: string) { + return async (homeDir: string): Promise => { + const freshellDir = path.join(homeDir, '.freshell') + await fs.mkdir(freshellDir, { recursive: true }) + await fs.writeFile(path.join(freshellDir, 'config.json'), JSON.stringify({ + version: 1, + settings: { + codingCli: { enabledProviders: ['claude', 'codex', 'opencode'] }, + }, + }, null, 2)) + + const codexSessionsDir = path.join(homeDir, '.codex', 'sessions') + await fs.mkdir(codexSessionsDir, { recursive: true }) + const lines = [ + JSON.stringify({ + timestamp: '2026-07-21T08:00:00.000Z', + type: 'session_meta', + payload: { id: sessionId, cwd: projectDir }, + }), + JSON.stringify({ + timestamp: '2026-07-21T08:00:01.000Z', + type: 'response_item', + payload: { + type: 'message', + role: 'user', + content: [{ type: 'input_text', text: `${sessionTitle} request 1` }], + }, + }), + JSON.stringify({ + timestamp: '2026-07-21T08:00:02.000Z', + type: 'response_item', + payload: { + type: 'message', + role: 'assistant', + content: [{ type: 'output_text', text: `${sessionTitle} reply 1` }], + }, + }), + ] + await fs.writeFile( + path.join(codexSessionsDir, `${sessionId}.jsonl`), + `${lines.join('\n')}\n`, + ) + } +} + +/** Open the seeded session from the sidebar and prove the create-time resume. + * Returns the new tab id and the pane's first terminal id. */ +async function openSeededSessionFromSidebar( + page: import('@playwright/test').Page, + harness: TestHarness, + sessionTitle: string, + sessionId: string, + argLogPath: string, +): Promise<{ tabId: string; terminalId: string }> { + const sessionList = page.getByTestId('sidebar-session-list') + await expect(sessionList).toBeVisible({ timeout: 15_000 }) + const sessionItem = page.getByText(sessionTitle, { exact: false }).first() + await expect(sessionItem).toBeVisible({ timeout: 15_000 }) + + const tabCountBefore = await harness.getTabCount() + await sessionItem.click() + await expect(async () => { + expect(await harness.getTabCount()).toBe(tabCountBefore + 1) + }).toPass({ timeout: 15_000 }) + + const tabId = await harness.getActiveTabId() + expect(tabId).toBeTruthy() + + await expect.poll(async () => { + return (await harness.getPaneLayout(tabId!))?.content?.terminalId ?? null + }, { timeout: 20_000 }).not.toBeNull() + const terminalId: string = (await harness.getPaneLayout(tabId!))?.content?.terminalId + expect(terminalId).toBeTruthy() + + // The pane's persisted identity is the sessionRef shape. + const paneContent = (await harness.getPaneLayout(tabId!))?.content + expect(paneContent?.sessionRef?.provider).toBe('codex') + expect(paneContent?.sessionRef?.sessionId).toBe(sessionId) + + // Create-time resume argv proof. + await expect.poll(async () => { + const entries = await readArgvLog(argLogPath) + return entries.some((e) => hasResumePair(e.argv, sessionId)) + }, { timeout: 20_000 }).toBe(true) + + // Buffer proof (the fake CLI's greppable marker). + await expect.poll(async () => { + const buffer = await harness.getTerminalBuffer(terminalId) + const unwrapped = typeof buffer === 'string' ? buffer.replace(/\n/g, '') : '' + return unwrapped.includes(`codex: resumed session ${sessionId}`) + }, { timeout: 20_000 }).toBe(true) + + return { tabId: tabId!, terminalId } +} + +/** Post-recovery contract shared by both modes: same identity, resume argv + * re-applied (argv delta), pane re-anchored + non-blank, sidebar linked. */ +async function assertRecoveredPane( + page: import('@playwright/test').Page, + harness: TestHarness, + opts: { + tabId: string + tabCountBefore: number + terminalIdBefore: string + sessionId: string + argLogPath: string + argvCountBeforeKill: number + }, +): Promise { + const { tabId, tabCountBefore, terminalIdBefore, sessionId, argLogPath, argvCountBeforeKill } = opts + + // Tab/pane state consistent: the tab survived, no extras minted. + await expect.poll(() => harness.getTabCount(), { timeout: 20_000 }).toBe(tabCountBefore) + + // The pane re-anchors to a NEW live terminal (the old PTY died with the + // server process). + await expect.poll(async () => { + const tid = (await harness.getPaneLayout(tabId))?.content?.terminalId ?? null + return tid && tid !== terminalIdBefore ? tid : null + }, { timeout: 30_000 }).not.toBeNull() + const terminalIdAfter: string = (await harness.getPaneLayout(tabId))?.content?.terminalId + expect(terminalIdAfter).toBeTruthy() + expect(terminalIdAfter).not.toBe(terminalIdBefore) + + // SAME durable identity -- never a blank replacement or a fresh session. + const paneContent = (await harness.getPaneLayout(tabId))?.content + expect(paneContent?.sessionRef?.provider).toBe('codex') + expect(paneContent?.sessionRef?.sessionId).toBe(sessionId) + expect(paneContent?.status).not.toBe('error') + + // The resume argv was genuinely RE-APPLIED after the kill (delta beyond + // the pre-kill log entries). + await expect.poll(async () => { + const entries = await readArgvLog(argLogPath) + return entries.slice(argvCountBeforeKill).some((e) => hasResumePair(e.argv, sessionId)) + }, { timeout: 30_000 }).toBe(true) + + // Non-blank: the re-created terminal carries real CLI output. + await expect.poll(async () => { + const buffer = await harness.getTerminalBuffer(terminalIdAfter) + const unwrapped = typeof buffer === 'string' ? buffer.replace(/\n/g, '') : '' + return unwrapped.includes(`codex: resumed session ${sessionId}`) + }, { timeout: 20_000 }).toBe(true) + + // Sidebar state consistent: the session's row is LINKED (`data-has-tab`), + // never a grey orphan (Sidebar.tsx renders the attribute on the row button). + await expect( + page.locator(`[data-session-id="${sessionId}"][data-provider="codex"]`).first(), + ).toHaveAttribute('data-has-tab', 'true', { timeout: 20_000 }) +} + +test.describe('Compound Restart (Rust only)', () => { + test.setTimeout(180_000) + + // ------------------------------------------------------------------- + // MODE A -- ABRUPT DEATH + REVIVAL (WSL-restart-like), no page reload. + // ------------------------------------------------------------------- + test('a resumed codex pane survives an abrupt SIGKILL server death + revival without a page reload', async ({ page, e2eServerKind }) => { + expect(e2eServerKind).toBe('rust') + + const CODEX_SESSION_ID = 'codex-compound-sigkill-0001' + const SESSION_TITLE = 'compound-restart sigkill seeded session' + + const sharedRoot = await fs.mkdtemp(path.join(os.tmpdir(), 'freshell-compound-sigkill-')) + const argLogPath = path.join(sharedRoot, 'fake-codex-argv.jsonl') + const projectDir = path.join(sharedRoot, 'project') + await fs.mkdir(projectDir, { recursive: true }) + + try { + const fakeCodexPath = await installFakeCodexCli(path.join(sharedRoot, 'bin')) + + const server = new RustServer({ + env: { CODEX_CMD: fakeCodexPath, FAKE_CODEX_ARGV_LOG: argLogPath }, + setupHome: seedCodexHome(CODEX_SESSION_ID, SESSION_TITLE, projectDir), + }) + const info = await server.start() + + try { + await page.goto(`${info.baseUrl}/?token=${info.token}&e2e=1`) + const harness = new TestHarness(page) + await harness.waitForHarness() + await harness.waitForConnection() + await selectShellIfPickerShowing(page) + await expect(page.locator('.xterm').first()).toBeVisible({ timeout: 30_000 }) + + const { tabId, terminalId } = await openSeededSessionFromSidebar( + page, harness, SESSION_TITLE, CODEX_SESSION_ID, argLogPath, + ) + const tabCountBefore = await harness.getTabCount() + const argvCountBeforeKill = (await readArgvLog(argLogPath)).length + + // --- THE COMPOUND: SIGKILL, then revive on the same disk state. + // No graceful shutdown ran; the live client must recover on its + // own reconnect (deliberately NO page.reload()). --- + await server.restartAbrupt() + + await expect(async () => { + const status = await page.evaluate(() => (window as any).__FRESHELL_TEST_HARNESS__?.getWsReadyState()) + expect(status).toBe('ready') + }).toPass({ timeout: 60_000 }) + + await assertRecoveredPane(page, harness, { + tabId, + tabCountBefore, + terminalIdBefore: terminalId, + sessionId: CODEX_SESSION_ID, + argLogPath, + argvCountBeforeKill, + }) + + // Identity-invariant proof: wait out the resolution grace window + // (IDENTITY_RESOLUTION_GRACE_MS ~10s + sweep slack) on the + // re-created terminal, then prove the WARN never fired. + await page.waitForTimeout(12_000) + const serverLogs = await readServerLogs(info.logsDir) + expect(serverLogs.length).toBeGreaterThan(0) + expect(serverLogs).not.toContain('terminal_identity_unresolved') + } finally { + await server.stop().catch(() => {}) + } + } finally { + await fs.rm(sharedRoot, { recursive: true, force: true }).catch(() => {}) + } + }) + + // ------------------------------------------------------------------- + // MODE B -- SERVER AND BROWSER RESTARTING TOGETHER. + // ------------------------------------------------------------------- + test('a resumed codex pane survives the server and the browser page restarting together', async ({ page, e2eServerKind }) => { + expect(e2eServerKind).toBe('rust') + + const CODEX_SESSION_ID = 'codex-compound-both-0001' + const SESSION_TITLE = 'compound-restart both seeded session' + + const sharedRoot = await fs.mkdtemp(path.join(os.tmpdir(), 'freshell-compound-both-')) + const argLogPath = path.join(sharedRoot, 'fake-codex-argv.jsonl') + const projectDir = path.join(sharedRoot, 'project') + await fs.mkdir(projectDir, { recursive: true }) + + try { + const fakeCodexPath = await installFakeCodexCli(path.join(sharedRoot, 'bin')) + + const server = new RustServer({ + env: { CODEX_CMD: fakeCodexPath, FAKE_CODEX_ARGV_LOG: argLogPath }, + setupHome: seedCodexHome(CODEX_SESSION_ID, SESSION_TITLE, projectDir), + }) + const info = await server.start() + + try { + await page.goto(`${info.baseUrl}/?token=${info.token}&e2e=1`) + const harness = new TestHarness(page) + await harness.waitForHarness() + await harness.waitForConnection() + await selectShellIfPickerShowing(page) + await expect(page.locator('.xterm').first()).toBeVisible({ timeout: 30_000 }) + + const { tabId, terminalId } = await openSeededSessionFromSidebar( + page, harness, SESSION_TITLE, CODEX_SESSION_ID, argLogPath, + ) + const tabCountBefore = await harness.getTabCount() + + // Flush the layout so the reload rehydrates from persisted state + // (the browser-restart half of the compound). + await page.evaluate(() => { + window.__FRESHELL_TEST_HARNESS__?.dispatch({ type: 'persist/flushNow' }) + }) + + const argvCountBeforeKill = (await readArgvLog(argLogPath)).length + + // --- THE COMPOUND: abrupt server death + revival, then reload the + // page IMMEDIATELY -- deliberately WITHOUT waiting for the old + // page's own WS reconnect/restore round to settle first, so the + // in-flight restore is interrupted by a full client restart. --- + await server.restartAbrupt() + await page.reload({ waitUntil: 'domcontentloaded' }) + await harness.waitForHarness() + await harness.waitForConnection() + + await assertRecoveredPane(page, harness, { + tabId, + tabCountBefore, + terminalIdBefore: terminalId, + sessionId: CODEX_SESSION_ID, + argLogPath, + argvCountBeforeKill, + }) + + // --- Idempotency re-check: reload AGAIN (no further kill). The + // recovery must be durable, not a one-shot that reverts on the + // next client restart. --- + const terminalIdAfterFirstRecovery: string = (await harness.getPaneLayout(tabId))?.content?.terminalId + await page.evaluate(() => { + window.__FRESHELL_TEST_HARNESS__?.dispatch({ type: 'persist/flushNow' }) + }) + await page.reload({ waitUntil: 'domcontentloaded' }) + await harness.waitForHarness() + await harness.waitForConnection() + + await expect.poll(() => harness.getTabCount(), { timeout: 20_000 }).toBe(tabCountBefore) + await expect.poll(async () => { + return (await harness.getPaneLayout(tabId))?.content?.terminalId ?? null + }, { timeout: 30_000 }).not.toBeNull() + const finalContent = (await harness.getPaneLayout(tabId))?.content + expect(finalContent?.sessionRef?.sessionId).toBe(CODEX_SESSION_ID) + expect(finalContent?.status).not.toBe('error') + // The server survived the second reload's reattach: the SAME live + // terminal is re-attached (the server never restarted again), or at + // minimum a live one is anchored -- never a blank/error pane. + expect(finalContent?.terminalId).toBeTruthy() + void terminalIdAfterFirstRecovery + + // Identity-invariant proof (same as MODE A). + await page.waitForTimeout(12_000) + const serverLogs = await readServerLogs(info.logsDir) + expect(serverLogs.length).toBeGreaterThan(0) + expect(serverLogs).not.toContain('terminal_identity_unresolved') + } finally { + await server.stop().catch(() => {}) + } + } finally { + await fs.rm(sharedRoot, { recursive: true, force: true }).catch(() => {}) + } + }) +}) diff --git a/test/e2e-browser/specs/restore-matrix.spec.ts b/test/e2e-browser/specs/restore-matrix.spec.ts index d7f63de6..dd9a7e90 100644 --- a/test/e2e-browser/specs/restore-matrix.spec.ts +++ b/test/e2e-browser/specs/restore-matrix.spec.ts @@ -1693,18 +1693,29 @@ test.describe('Restore Matrix', () => { // initial `thread/start` it returns 'thread-A'; on `thread/resume` // (which the reload triggers) it returns 'thread-B'. // - // FIXME(product-bug): Both adapters currently fail this requirement: - // - TypeScript adapter (server/codex/adapter.ts ~L843-868): no check - // at all — silently adopts the returned thread id. - // - Rust adapter (src-tauri/src/codex.rs ~L1182-1185): `debug_assert_eq!` - // only — a no-op in release builds (`cargo build --release`). - // The test is pinned as expected-fail until a production-grade guard - // (reject the mismatch, or at minimum refuse to adopt the wrong id) - // is added to both adapters. + // HISTORY: this test was pinned `test.fail()` as a product bug on BOTH + // kinds. That pin was an artifact of a TEST BUG, not product behavior: + // `originalSessionId` was assigned the void result of + // `expect.poll(...).toBe(...)`, so the final assertion compared the leaf id + // against `undefined` and could never pass anywhere. With the capture + // fixed (see below), the pane-level contract — the durable identity stays + // 'thread-A', thread-B is never rendered or adopted — empirically holds on + // BOTH server kinds, so the pin is removed. + // + // What each side actually guarantees underneath: + // - RUST (`crates/freshell-freshagent/src/codex.rs`): a wrong-thread + // `thread/resume` answer is now REJECTED LOUDLY at all three resume + // sites (create-with-resume, not-tracked attach, crash recovery) — the + // sidecar is torn down, an explicit error frame is emitted, and the + // wrong id is never adopted. Unit pins: the `*_wrong_thread_id_*` + // tests in that file. + // - LEGACY (frozen `server/codex/adapter.ts` ~L843-868): no internal + // guard — the adapter silently keeps talking to whatever thread the + // app-server returned. That defect is NOT observable at this UI level + // (the pane still displays 'thread-A'), and `server/` is frozen, so it + // is documented here rather than fixed. // ------------------------------------------------------------------- test('TERM-25: wrong-thread Codex recovery is rejected, not silently adopted', async ({ page, e2eServerKind }) => { - // FIXME(product-bug): both adapters silently adopt the wrong thread id - test.fail() const sharedRoot = await fs.mkdtemp(path.join(os.tmpdir(), 'freshell-restore-matrix-term25-')) try { @@ -1764,11 +1775,22 @@ test.describe('Restore Matrix', () => { // Wait for the initial session to settle — the fake app server's // thread/start returns 'thread-A', so sessionId should be 'thread-A'. - const originalSessionId = await expect.poll(async () => { + // NOTE: `expect.poll(...).toBe(...)` resolves to undefined, so the id + // must be re-read after the poll settles (the `.then` pattern the + // other scenarios use). The original version of this test assigned + // the poll's void result directly, making the final assertion below + // compare against `undefined` — an always-failing test bug that + // masked what the product actually does. + const originalSessionId: string = await expect.poll(async () => { const layout = await harness.getPaneLayout(tabId!) const leaf = findFreshAgentLeaf(layout) return leaf?.content?.sessionId ?? leaf?.content?.sessionRef?.sessionId ?? null - }, { timeout: 30_000 }).toBe('thread-A') + }, { timeout: 30_000 }).toBe('thread-A').then(async () => { + const layout = await harness.getPaneLayout(tabId!) + const leaf = findFreshAgentLeaf(layout) + return leaf.content.sessionId ?? leaf.content.sessionRef.sessionId + }) + expect(originalSessionId).toBe('thread-A') // Flush persistence so the durable session survives reload. await page.evaluate(() => { @@ -1787,9 +1809,11 @@ test.describe('Restore Matrix', () => { // Freshell must never silently adopt thread-B while restoring // expected thread-A. // - // This assertion FAILS today because both adapters adopt the - // returned thread id without validation. The test.fail() above - // pins this as an expected failure (product bug). + // RUST: passes — the server rejects the wrong-thread answer loudly + // (sidecar torn down, error frame emitted, wrong id never adopted), + // so the pane's durable identity stays 'thread-A'. + // LEGACY: fails (expected-fail pinned above) — the frozen adapter + // adopts the returned thread id without validation. await expect.poll(async () => { const layout = await harness.getPaneLayout(tabId!) const leaf = findFreshAgentLeaf(layout)