From d6e9ae19d3322d6bb82257f9fdcedba24f2dac18 Mon Sep 17 00:00:00 2001 From: Dan Shapiro <3732858+danshapiro@users.noreply.github.com> Date: Thu, 27 Aug 2026 10:18:47 -0700 Subject: [PATCH 01/15] docs: add implementation plan for server-version-reload --- .../plans/2026-08-27-server-version-reload.md | 1009 +++++++++++++++++ 1 file changed, 1009 insertions(+) create mode 100644 docs/plans/2026-08-27-server-version-reload.md diff --git a/docs/plans/2026-08-27-server-version-reload.md b/docs/plans/2026-08-27-server-version-reload.md new file mode 100644 index 000000000..c62e9a295 --- /dev/null +++ b/docs/plans/2026-08-27-server-version-reload.md @@ -0,0 +1,1009 @@ +# Server-Build Mismatch Auto-Reload Implementation Plan + +> **For agentic workers:** Execute this plan task by task with a fresh +> implementer and a specification-plus-quality review after every task. Track +> progress with the checkbox steps below. + +**Goal:** When a browser tab connects (or reconnects) to a Freshell server built from a different commit than the client bundle it is running, the client detects the mismatch from the WS `ready` frame and reloads itself exactly once — self-healing the "Fresh-agent snapshot response did not match the shared contract" class of stale-client failures without ever reload-looping. + +**Architecture:** All three producers stamp the same identity — `git rev-parse HEAD` with literal `"unknown"` fallback (Rust at compile time via the existing `build.rs`, Node at first use via a new cached module, client at Vite build time via a `define` constant). The WS `ready` message gains an additive optional `buildId` field (omitted from the wire when the Rust value is `None`, so frozen transcripts stay byte-identical; the Node frame always stamps it, mirroring `bootId`). The client compares its baked `__FRESHELL_BUILD_ID__` against every parsed `ready.buildId`; on mismatch it sets a `sessionStorage` sentinel and calls `location.reload()` exactly once per tab session (the sentinel also self-clears on a subsequent match, re-arming the guard). + +**Tech Stack:** Rust (serde/serde_json, tokio, existing `freshell-protocol`/`freshell-ws` crates), Node.js/ESM (`node:child_process`), React 18 + Vite `define` + Zod, Vitest (jsdom client config / node server config), Playwright (rust-chromium project). + +## Global Constraints + +- **Worktree discipline:** All work happens in `/home/dan/code/freshell/.worktrees/server-version-reload` on branch `the-usual/server-version-reload`. Never run `node dist/server/index.js`; never touch the live 3001 server or `~/.freshell` state. No deploy/restart is part of this plan. +- **Additive contract only ("bootId doctrine"):** `buildId` is optional everywhere and omitted from the wire when the Rust value is `None`. Old clients must not break; the frozen `port/oracle/fixtures/handshake-transcript.json` must remain byte-valid without regenerating it. The Node ready frame ALWAYS stamps `buildId` (string, `"unknown"` fallback) — mirroring how it always stamps `bootId` — and the Rust handshake always stamps `Some(...)` from `WsState.build_id`. Both servers MUST stamp in the same commit (Task 1): the T0 oracle deep-diffs node-vs-rust handshakes, so an intermediate where only one side stamps would fail it. +- **Value semantics on every side:** the value is the full `git rev-parse HEAD` SHA of the repo at build/bake time; when git is unavailable or the output is not 40 lowercase hex chars, the literal `"unknown"`. The client's compare rule: reload iff BOTH ids are present, non-empty, neither is `"unknown"`, and they differ. `"unknown" == "unknown"` is NOT a match-and-clear (it is a no-op) — two unknown builds must never trigger a reload and must never clear an armed sentinel. +- **Loop-guard invariant:** at most ONE reload per tab session. The sentinel key is `freshell.server-build-reload` (`sessionStorage`, value `"1"`), set BEFORE calling `reload()`. If `sessionStorage` throws, no reload happens (fail-safe). A matching `ready` clears the sentinel (self-re-arm). +- **Client module must not crash under Vitest:** the Vitest client config has no `__FRESHELL_BUILD_ID__` define, so the module must use a `typeof __FRESHELL_BUILD_ID__ === 'undefined'` guard (same precedent as `src/lib/perf-logger.ts:45` with `__PERF_LOGGING__`). +- **NodeNext/ESM:** every relative import in `server/` and `shared/` uses `.js` extensions; client code uses `@/` aliases without extensions. +- **Test coordination:** broad suites go through the repo coordinator (`npm run test:vitest -- run ...`); never raw `npx vitest`. Focused Rust tests use `cargo test -p ` directly. +- **Scope boundary:** client-only redeploys (redeploying a new client bundle WITHOUT a server change) are deliberately NOT covered — no auto-refresh loop, no polling, no `/api/server-info` polling fallback. The ready-frame compare is the only trigger. +- **No unrelated restructuring; comments explain invariants, in the existing voice.** + +--- + +### Task 1: Protocol + both servers stamp `ready.buildId` + +**Files:** +- Modify: `shared/ws-protocol.ts:743-750` (`ReadyMessage` type) +- Modify: `crates/freshell-protocol/src/server_messages.rs:792-806` (`Ready` struct) +- Modify: `crates/freshell-ws/src/lib.rs:97-124` (`WsState` struct field), `:529-546` (`build_handshake_with_capabilities`), `:868-917` (`state()` test builder) +- Modify: `crates/freshell-server/src/main.rs:1011-1066` (`WsState` literal) +- Modify: `crates/freshell-protocol/tests/pane_reconcile.rs:52-82` (two `Ready` literals) +- Create: `server/build-id.ts` +- Modify: `server/ws-handler.ts` (import block; field after `:587`; init after `:651`; ready send `:2034-2039`) +- Modify (generated): `port/contract/ws-server-messages.schema.json` (via `npm run contract:generate`) +- Test: `crates/freshell-protocol/tests/roundtrip.rs` (new test after `ready_carries_server_instance_id_and_boot_id`, which ends at line 164) +- Test: `crates/freshell-ws/src/lib.rs` `#[cfg(test)] mod tests` (new test after `handshake_is_ordered_with_shared_bootid`, which ends at line 1026) +- Test: `test/server/build-id.test.ts` (new) +- Test: `test/server/ws-handshake-snapshot.test.ts` (new test after the `includes a bootId in the ready message...` test, which ends at line 301) + +**Interfaces:** +- Consumes: `crates/freshell-server/src/diag.rs:124` `pub(crate) fn build_commit() -> &'static str` (already returns the baked `FRESHELL_BUILD_COMMIT` or `"unknown"`; `build.rs` re-stamps on HEAD moves — no change needed there). +- Produces: `freshell_protocol::Ready { build_id: Option }` (serde camelCase → wire key `buildId`, skipped when `None`); `freshell_ws::WsState { build_id: Arc }`; `server/build-id.ts` exporting `computeBuildId(cwd?: string): string` (pure) and `serverBuildId(): string` (cached per process); TS `ReadyMessage.buildId?: string`; regenerated `port/contract/ws-server-messages.schema.json` with an optional `buildId` on `ready` (still `additionalProperties: false`). Task 2's client schema and Task 3's e2e injection consume the wire key `buildId`. + +- [ ] **Step 1: Write the failing behavioral tests (protocol roundtrip, rust wire, node module, node wire)** + +1a. Add to `crates/freshell-protocol/tests/roundtrip.rs` immediately after the `ready_carries_server_instance_id_and_boot_id` test (line 164): + +```rust +#[test] +fn ready_carries_build_id_and_omits_it_when_absent() { + // deliverable: `ready` accepts an additive optional `buildId` (the git + // commit the server binary was built from) and OMITS it from the wire + // when absent — frozen-transcript inertness, same rule as `bootId`. + let with = r#"{"type":"ready","timestamp":"2026-07-05T04:20:52.546Z","serverInstanceId":"srv-abc","buildId":"a1b2c3d4e5f6a1b2c3d4e5f6a1b2c3d4e5f6a1b2"}"#; + match server_roundtrip(with, "ready") { + ServerMessage::Ready(r) => { + assert_eq!( + r.build_id.as_deref(), + Some("a1b2c3d4e5f6a1b2c3d4e5f6a1b2c3d4e5f6a1b2") + ); + } + other => panic!("expected Ready, got {other:?}"), + } + + let without = r#"{"type":"ready","timestamp":"2026-07-05T04:20:52.546Z","serverInstanceId":"srv-abc"}"#; + let msg: ServerMessage = serde_json::from_str(without).unwrap(); + let reser = serde_json::to_value(&msg).unwrap(); + assert!( + reser.get("buildId").is_none(), + "ready must omit buildId when absent: {reser}" + ); + match msg { + ServerMessage::Ready(r) => assert_eq!(r.build_id, None), + other => panic!("expected Ready, got {other:?}"), + } +} +``` + +1b. Add to `crates/freshell-ws/src/lib.rs` inside `mod tests`, immediately after `handshake_is_ordered_with_shared_bootid` (line 1026): + +```rust + /// The handshake `ready` stamps the build identity (`WsState.build_id`, + /// baked from `diag::build_commit()` by `freshell-server`'s `main.rs`) so + /// the browser client can detect a client/server build mismatch and + /// reload once. Serde omits the field when `None`; a real server always + /// stamps `Some` (sha or `"unknown"`), so presence is asserted here. + #[tokio::test] + async fn handshake_ready_stamps_build_id() { + let msgs = build_handshake(&state()).await; + let ready = serde_json::to_value(&msgs[0]).unwrap(); + assert_eq!(ready["buildId"], "build-3333"); + } +``` + +1c. Create `test/server/build-id.test.ts`: + +```typescript +import { execFileSync } from 'node:child_process' +import * as fs from 'node:fs' +import * as os from 'node:os' +import * as path from 'node:path' +import { fileURLToPath } from 'node:url' +import { describe, expect, it } from 'vitest' +import { computeBuildId, serverBuildId } from '../../server/build-id.js' + +const REPO_ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '../..') + +describe('server build id', () => { + it('returns the current git HEAD sha for the repository', () => { + const expected = execFileSync('git', ['rev-parse', 'HEAD'], { cwd: REPO_ROOT }) + .toString() + .trim() + expect(computeBuildId(REPO_ROOT)).toBe(expected) + }) + + it('falls back to "unknown" outside a git repository', () => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'build-id-no-git-')) + try { + expect(computeBuildId(dir)).toBe('unknown') + } finally { + fs.rmSync(dir, { recursive: true, force: true }) + } + }) + + it('caches the id within a process', () => { + expect(serverBuildId()).toBe(serverBuildId()) + }) +}) +``` + +1d. In `test/server/ws-handshake-snapshot.test.ts`, immediately after the `includes a bootId in the ready message that differs from serverInstanceId` test (ends line 301), add: + +```typescript + it('includes a buildId in the ready message, stable across clients in the same process', async () => { + const ws1 = new WebSocket(`ws://127.0.0.1:${port}/ws`) + const ws2 = new WebSocket(`ws://127.0.0.1:${port}/ws`) + + try { + await Promise.all([ + new Promise((resolve) => ws1.on('open', () => resolve())), + new Promise((resolve) => ws2.on('open', () => resolve())), + ]) + + const [ready1, ready2] = await Promise.all([ + waitForReady(ws1, 10_000), + waitForReady(ws2, 10_000), + ]) + + // Always stamped (sha or "unknown" fallback), stable within the process. + expect(typeof ready1.buildId).toBe('string') + expect((ready1.buildId as string).length).toBeGreaterThan(0) + expect(ready2.buildId).toBe(ready1.buildId) + // Distinct identity axis: not the boot id, not the instance id. + expect(ready1.buildId).not.toBe(ready1.bootId) + } finally { + await closeWs(ws1) + await closeWs(ws2) + } + }) +``` + +- [ ] **Step 2: Run the tests and verify the intended failures** + +```bash +cargo test -p freshell-protocol --test roundtrip ready_carries_build_id_and_omits_it_when_absent +cargo test -p freshell-ws handshake_ready_stamps_build_id +npm run test:vitest -- run test/server/build-id.test.ts test/server/ws-handshake-snapshot.test.ts --config config/vitest/vitest.server.config.ts +``` + +Expected: all FAIL for the missing behavior — the two Rust commands fail to COMPILE (`no field \`build_id\` on struct Ready` / `no field \`build_id\` on struct WsState`); `build-id.test.ts` fails to resolve `../../server/build-id.js` (module missing); the new snapshot test fails on `expect(typeof ready1.buildId).toBe('string')` (the Node ready frame carries no `buildId`). + +- [ ] **Step 3: Add the minimal production implementation** + +3a. `shared/ws-protocol.ts` — in `ReadyMessage` (lines 743-750), add after `bootId`: + +```typescript +export type ReadyMessage = { + type: 'ready' + timestamp: string + serverInstanceId?: string + bootId?: string + /** The git commit the server binary was built from ("unknown" fallback). + * Additive/optional bootId doctrine: the client bakes its own build id at + * Vite build time and reloads once on a mismatch. Omitted from the wire + * when the Rust value is None. */ + buildId?: string + /** Present iff the client's hello opted in via capabilities.paneReconcileV1. */ + capabilities?: ReadyCapabilities +} +``` + +3b. Regenerate the outbound schema bundle (picks up `buildId` as an optional modeled property on `ready` — keeping `test/unit/port/ws-contract-freeze.test.ts`'s "committed schema deep-equals a fresh regeneration" AND `mutation-validation.test.ts`'s `additional-property` case green, since the schema stays `additionalProperties: false`): + +```bash +npm run contract:generate +git diff --stat port/contract/ws-server-messages.schema.json +``` + +Expected: the regenerated diff adds an optional `buildId` property to the `ready` message schema; inventory/message counts unchanged. + +3c. `crates/freshell-protocol/src/server_messages.rs` — in `Ready` (lines 792-806), add after the `server_instance_id` field: + +```rust + /// The git commit this server binary was built from (`"unknown"` + /// fallback), stamped so the browser client can detect a client/server + /// build mismatch and reload once. Omitted from the wire entirely when + /// `None` (frozen-client inertness — same rule as `boot_id`). + #[serde(skip_serializing_if = "Option::is_none")] + pub build_id: Option, +``` + +3d. `crates/freshell-ws/src/lib.rs` — in `WsState`, after the `boot_id` field (line 103): + +```rust + /// The git commit this server binary was built from (`"unknown"` + /// fallback) — baked once per build by `freshell-server`'s `main.rs` from + /// `diag::build_commit()` and stamped into every handshake's `ready`. + pub build_id: Arc, +``` + +In `build_handshake_with_capabilities`, in the `Ready` literal (lines 536-546), add after `server_instance_id`: + +```rust + build_id: Some(state.build_id.as_ref().clone()), +``` + +3e. Fix every struct-literal site so the workspace compiles. In `crates/freshell-ws/src/lib.rs`'s `state()` test builder (after `boot_id: Arc::new("boot-2222".to_string()),` at line 878): + +```rust + build_id: Arc::new("build-3333".to_string()), +``` + +In `crates/freshell-server/src/main.rs`'s `WsState` literal (after the `boot_id: Arc::clone(&boot_id),` line at 1031): + +```rust + // The build identity every handshake `ready` stamps (client-side + // stale-bundle auto-reload). SAME source `GET /api/server-info`'s + // `commit` reports — one source of truth (`diag::build_commit()`). + build_id: Arc::new(crate::diag::build_commit().to_string()), +``` + +In `crates/freshell-protocol/tests/pane_reconcile.rs`, both `Ready` literals (lines 56-61 and 71-79) each get: + +```rust + build_id: None, +``` + +Then enumerate any remaining literal sites: + +```bash +cargo check --workspace --all-targets 2>&1 | rg "missing field" || echo "no missing-field errors" +``` + +Expected: `no missing-field errors` (if any site beyond the ones above is listed, add `build_id: None` — or, for `WsState` literals, a `build_id: Arc::new(...)` value — the same way; every site compiles before proceeding). + +3f. Create `server/build-id.ts`: + +```typescript +import { execFileSync } from 'node:child_process' + +const SHA_PATTERN = /^[0-9a-f]{40}$/ + +/** + * The git commit this server process runs from — the SAME identity the Rust + * server bakes at compile time (`crates/freshell-server/src/diag.rs`'s + * `build_commit()`) and the client bakes at Vite build time + * (`__FRESHELL_BUILD_ID__`). Falls back to the literal `"unknown"` when git + * is unavailable or the output is not a full 40-hex sha; the client's + * compare rule ignores `"unknown"` on both sides, so a git-less deployment + * never triggers a reload and never clears an armed one. + */ +export function computeBuildId(cwd: string = process.cwd()): string { + try { + const sha = execFileSync('git', ['rev-parse', 'HEAD'], { + cwd, + stdio: ['ignore', 'pipe', 'ignore'], + timeout: 5_000, + }) + .toString() + .trim() + return SHA_PATTERN.test(sha) ? sha : 'unknown' + } catch { + return 'unknown' + } +} + +let cached: string | undefined + +/** Per-process cached build id — one git probe per server lifetime. */ +export function serverBuildId(): string { + if (cached === undefined) cached = computeBuildId() + return cached +} +``` + +3g. In `server/ws-handler.ts`: + +Add the import alongside the other relative imports at the top of the file: + +```typescript +import { serverBuildId } from './build-id.js' +``` + +Add the field after `private readonly bootId: string` (line 587): + +```typescript + private readonly buildId: string +``` + +Initialize it after `this.bootId = \`boot-${randomUUID()}\`` (line 651): + +```typescript + this.buildId = serverBuildId() +``` + +Extend the ready send (lines 2034-2039): + +```typescript + this.send(ws, { + type: 'ready', + timestamp: nowIso(), + serverInstanceId: this.serverInstanceId, + bootId: this.bootId, + buildId: this.buildId, + }) +``` + +- [ ] **Step 4: Run the focused tests** + +```bash +cargo test -p freshell-protocol --test roundtrip ready_carries_build_id_and_omits_it_when_absent +cargo test -p freshell-ws handshake_ready_stamps_build_id +npm run test:vitest -- run test/server/build-id.test.ts test/server/ws-handshake-snapshot.test.ts --config config/vitest/vitest.server.config.ts +``` + +Expected: all PASS. + +- [ ] **Step 5: Refactor while green** + +No refactor needed — every addition mirrors the adjacent `bootId` idiom on its side. Do NOT regenerate `port/oracle/fixtures/handshake-transcript.json`: the frozen transcript stays byte-valid because Rust omits `build_id` when deserialized as `None`, and the mutation/oracle suites consume the regenerated SCHEMA (not the live node bytes) for conformance. + +- [ ] **Step 6: Run impacted-test verification** + +This change touches the shared wire protocol, both server implementations, and the generated schema, so the impacted set is: both Rust crates' full test trees, the workspace compile of every literal site, the whole server-config suite (any test asserting handshake/ready shapes), and the port oracle suites. Note on `t0-equivalence-rust.test.ts`: its node-vs-rust deep diff compares `ready` frames value-by-value — both sides now stamp the SAME value (the worktree HEAD sha; `ensureRustServerBuilt` runs `cargo build` at test time and `build.rs` re-stamps on HEAD moves; Node computes the same sha at runtime), or both `"unknown"` in git-less environments, so the diff stays clean. + +```bash +cargo test -p freshell-protocol +cargo test -p freshell-ws +cargo check --workspace --all-targets +npm run test:integration +npm run test:port +``` + +Expected: all PASS. + +- [ ] **Step 7: Commit the task** + +```bash +git add shared/ws-protocol.ts crates/freshell-protocol/src/server_messages.rs crates/freshell-protocol/tests/roundtrip.rs crates/freshell-protocol/tests/pane_reconcile.rs crates/freshell-ws/src/lib.rs crates/freshell-server/src/main.rs server/build-id.ts server/ws-handler.ts test/server/build-id.test.ts test/server/ws-handshake-snapshot.test.ts port/contract/ws-server-messages.schema.json +git commit -m "feat(protocol): both servers stamp additive optional ready.buildId (git HEAD)" +``` + +--- + +### Task 2: Client compares on `ready` and reloads once (module + Vite define + App wiring) + +**Files:** +- Create: `src/lib/server-build-check.ts` +- Modify: `config/vite/vite.config.ts` (git-probe helper near the top-level helpers after line 10; extend the `define` block at lines 58-60) +- Modify: `src/vite-env.d.ts:12` (declare the constant) +- Modify: `src/App.tsx` (import near the other `@/lib` imports; `ReadyMessageSchema` at lines 157-166; call site after the bootId warn block ending at line 1031) +- Test: `test/unit/client/lib/server-build-check.test.ts` (new) +- Test: `test/unit/client/components/App.restart-signals.test.tsx` (new `describe` block at the end of the file, reusing that file's harness helpers) + +**Interfaces:** +- Consumes: Task 1's wire contract (`ReadyMessage.buildId?: string`, parsed by `ReadyMessageSchema`). +- Produces: `checkServerBuildId(options?: ServerBuildCheckOptions): void` from `@/lib/server-build-check`, with `ServerBuildCheckOptions { clientBuildId?: string; serverBuildId?: string; reload?: () => void; storage?: Pick }`; `__FRESHELL_BUILD_ID__: string` available client-side at build time. Task 3's e2e exercises the production wiring end to end. + +- [ ] **Step 1: Write the failing behavioral tests** + +1a. Create `test/unit/client/lib/server-build-check.test.ts`: + +```typescript +import { afterEach, describe, expect, it, vi } from 'vitest' +import { checkServerBuildId } from '@/lib/server-build-check' + +const SENTINEL = 'freshell.server-build-reload' + +function mapStorage() { + const map = new Map() + return { + getItem: (k: string) => map.get(k) ?? null, + setItem: (k: string, v: string) => void map.set(k, v), + removeItem: (k: string) => void map.delete(k), + _map: map, + } +} + +describe('checkServerBuildId', () => { + afterEach(() => { + vi.unstubAllGlobals() + vi.restoreAllMocks() + }) + + it('reloads once and sets the sentinel on a real mismatch', () => { + const storage = mapStorage() + const reload = vi.fn() + checkServerBuildId({ clientBuildId: 'a'.repeat(40), serverBuildId: 'b'.repeat(40), reload, storage }) + expect(reload).toHaveBeenCalledTimes(1) + expect(storage._map.get(SENTINEL)).toBe('1') + }) + + it('never reloads twice: an armed sentinel suppresses the reload', () => { + const storage = mapStorage() + storage._map.set(SENTINEL, '1') + const reload = vi.fn() + checkServerBuildId({ clientBuildId: 'a'.repeat(40), serverBuildId: 'b'.repeat(40), reload, storage }) + expect(reload).not.toHaveBeenCalled() + expect(storage._map.get(SENTINEL)).toBe('1') + }) + + it('a matching ready clears the sentinel (self-re-arm)', () => { + const storage = mapStorage() + storage._map.set(SENTINEL, '1') + const reload = vi.fn() + checkServerBuildId({ clientBuildId: 'a'.repeat(40), serverBuildId: 'a'.repeat(40), reload, storage }) + expect(reload).not.toHaveBeenCalled() + expect(storage._map.get(SENTINEL)).toBeUndefined() + }) + + it('is a no-op when either side is missing, empty, or "unknown"', () => { + for (const opts of [ + { clientBuildId: 'a'.repeat(40), serverBuildId: undefined }, + { clientBuildId: undefined, serverBuildId: 'b'.repeat(40) }, + { clientBuildId: '', serverBuildId: 'b'.repeat(40) }, + { clientBuildId: 'unknown', serverBuildId: 'b'.repeat(40) }, + { clientBuildId: 'a'.repeat(40), serverBuildId: 'unknown' }, + { clientBuildId: 'unknown', serverBuildId: 'unknown' }, + ] as const) { + const storage = mapStorage() + const reload = vi.fn() + checkServerBuildId({ ...opts, reload, storage }) + expect(reload, JSON.stringify(opts)).not.toHaveBeenCalled() + expect(storage._map.get(SENTINEL)).toBeUndefined() + } + }) + + it('an armed sentinel survives an "unknown"-vs-"unknown" ready (never treated as a match)', () => { + const storage = mapStorage() + storage._map.set(SENTINEL, '1') + const reload = vi.fn() + checkServerBuildId({ clientBuildId: 'unknown', serverBuildId: 'unknown', reload, storage }) + expect(reload).not.toHaveBeenCalled() + expect(storage._map.get(SENTINEL)).toBe('1') + }) + + it('does not reload when the sentinel cannot be persisted (fail-safe against reload loops)', () => { + const reload = vi.fn() + const storage = { + getItem: () => { throw new Error('quota') }, + setItem: () => { throw new Error('quota') }, + removeItem: () => { throw new Error('quota') }, + } + checkServerBuildId({ clientBuildId: 'a'.repeat(40), serverBuildId: 'b'.repeat(40), reload, storage }) + expect(reload).not.toHaveBeenCalled() + }) + + it('falls back to the __FRESHELL_BUILD_ID__ global and window defaults when options are omitted', () => { + vi.stubGlobal('__FRESHELL_BUILD_ID__', 'c'.repeat(40)) + const reload = vi.fn() + Object.defineProperty(window.location, 'reload', { value: reload, configurable: true, writable: true }) + sessionStorage.clear() + + checkServerBuildId({ serverBuildId: 'd'.repeat(40) }) + expect(reload).toHaveBeenCalledTimes(1) + expect(sessionStorage.getItem(SENTINEL)).toBe('1') + + // And with the global absent (Vitest has no define), it is a no-op. + vi.unstubAllGlobals() + sessionStorage.removeItem(SENTINEL) + checkServerBuildId({ serverBuildId: 'd'.repeat(40) }) + expect(reload).toHaveBeenCalledTimes(1) + }) +}) +``` + +1b. In `test/unit/client/components/App.restart-signals.test.tsx`, append a new `describe` block at the end of the file. It reuses that file's existing harness plumbing (`createStore`, `renderApp`, `sendReady`, `wsMocks`, `messageHandler`, `stubAudio`, `terminalRestoreMocks`, `fetchSidebarSessionsSnapshot`, `getTerminalDirectoryPage`, `searchTerminalView`, `apiGet`, `defaultServerSettings`, `defaultSettings` — all defined at the top of that file; mirror the existing describe's beforeEach exactly): + +```tsx +describe('App ready buildId → one-shot server-build reload', () => { + beforeEach(() => { + cleanup() + vi.resetAllMocks() + stubAudio() + wsMocks.onReconnect.mockReturnValue(() => {}) + wsMocks.onDisconnect.mockReturnValue(() => {}) + wsMocks.isReady = false + wsMocks.serverInstanceId = undefined + terminalRestoreMocks.addTerminalRestoreRequestId.mockClear() + terminalRestoreMocks.addTerminalFreshRecoveryRequestId.mockClear() + messageHandler = null + + wsMocks.onMessage.mockImplementation((cb: (msg: any) => void) => { + messageHandler = cb + return () => { messageHandler = null } + }) + + fetchSidebarSessionsSnapshot.mockReset() + fetchSidebarSessionsSnapshot.mockResolvedValue([]) + getTerminalDirectoryPage.mockReset() + getTerminalDirectoryPage.mockResolvedValue({ items: [], revision: 1, nextCursor: null }) + searchTerminalView.mockReset() + searchTerminalView.mockResolvedValue({ matches: [] }) + + apiGet.mockImplementation((url: string) => { + if (url === '/api/bootstrap') { + return Promise.resolve({ + settings: defaultServerSettings, + platform: { platform: 'linux' }, + shell: { authenticated: true, ready: true }, + }) + } + if (url === '/api/settings') return Promise.resolve(defaultSettings) + if (url === '/api/platform') return Promise.resolve({ platform: 'linux' }) + return Promise.resolve({}) + }) + + sessionStorage.clear() + Object.defineProperty(window.location, 'reload', { + value: vi.fn(), + configurable: true, + writable: true, + }) + }) + + afterEach(() => { + cleanup() + vi.unstubAllGlobals() + sessionStorage.clear() + }) + + it('mismatched ready buildId triggers exactly one reload, and the sentinel suppresses the next mismatched ready', async () => { + vi.stubGlobal('__FRESHELL_BUILD_ID__', 'a'.repeat(40)) + const store = createStore() + await renderApp(store) + + sendReady({ serverInstanceId: 'srv-1', bootId: 'boot-1', buildId: 'b'.repeat(40) }) + expect(window.location.reload).toHaveBeenCalledTimes(1) + expect(sessionStorage.getItem('freshell.server-build-reload')).toBe('1') + + // Reconnect delivers another mismatched ready (stale server still up): + // the sentinel must suppress the reload. + sendReady({ serverInstanceId: 'srv-1', bootId: 'boot-1', buildId: 'b'.repeat(40) }) + expect(window.location.reload).toHaveBeenCalledTimes(1) + }) + + it('a matching ready clears the sentinel and re-arms the guard', async () => { + vi.stubGlobal('__FRESHELL_BUILD_ID__', 'a'.repeat(40)) + sessionStorage.setItem('freshell.server-build-reload', '1') + const store = createStore() + await renderApp(store) + + // Server caught up to the client build (the post-reload convergence + // case): match → sentinel cleared, no reload. + sendReady({ serverInstanceId: 'srv-1', bootId: 'boot-1', buildId: 'a'.repeat(40) }) + expect(window.location.reload).not.toHaveBeenCalled() + expect(sessionStorage.getItem('freshell.server-build-reload')).toBeNull() + }) + + it('never reloads on missing or "unknown" buildIds', async () => { + vi.stubGlobal('__FRESHELL_BUILD_ID__', 'a'.repeat(40)) + const store = createStore() + await renderApp(store) + + sendReady({ serverInstanceId: 'srv-1', bootId: 'boot-1' }) + sendReady({ serverInstanceId: 'srv-1', bootId: 'boot-1', buildId: 'unknown' }) + expect(window.location.reload).not.toHaveBeenCalled() + expect(sessionStorage.getItem('freshell.server-build-reload')).toBeNull() + }) +}) +``` + +- [ ] **Step 2: Run the tests and verify the intended failures** + +```bash +npm run test:vitest -- run test/unit/client/lib/server-build-check.test.ts test/unit/client/components/App.restart-signals.test.tsx +``` + +Expected: FAIL — `server-build-check.test.ts` cannot resolve `@/lib/server-build-check` (module missing), and the App tests fail because a ready with `buildId` triggers no reload (`expect(window.location.reload).toHaveBeenCalledTimes(1)` sees 0). + +- [ ] **Step 3: Add the minimal production implementation** + +3a. Create `src/lib/server-build-check.ts`: + +```typescript +import { createLogger } from '@/lib/client-logger' + +const log = createLogger('ServerBuildCheck') + +const SERVER_BUILD_RELOAD_SENTINEL = 'freshell.server-build-reload' + +export interface ServerBuildCheckOptions { + /** The client's own baked build id; defaults to `__FRESHELL_BUILD_ID__`. */ + clientBuildId?: string + /** The server's `ready.buildId`. */ + serverBuildId?: string + reload?: () => void + storage?: Pick +} + +/** + * The client's Vite-baked build id (`config/vite/vite.config.ts` defines it + * from `git rev-parse HEAD`). `typeof`-guarded because the Vitest client + * config has no define for it (same precedent as `__PERF_LOGGING__` in + * `src/lib/perf-logger.ts`) — an unbaked id means "cannot compare", never + * "reload". + */ +function resolveClientBuildId(): string | undefined { + if (typeof __FRESHELL_BUILD_ID__ === 'undefined') return undefined + const id = __FRESHELL_BUILD_ID__ + return id.length > 0 ? id : undefined +} + +/** + * Compare the server's `ready.buildId` against our own baked build id and + * reload ONCE on a real mismatch. Invariants: + * - reload iff BOTH ids are present, non-empty, neither is "unknown", and + * they differ ("unknown" == "unknown" is a no-op, never a match-and-clear); + * - the sessionStorage sentinel is set BEFORE reloading and suppresses any + * further reloads this tab session (a half-deployed server can never + * reload-loop; sessionStorage access failure = no reload, fail-safe); + * - a MATCHING ready clears the sentinel (self-re-arm after convergence). + */ +export function checkServerBuildId(options?: ServerBuildCheckOptions): void { + const clientBuildId = options?.clientBuildId ?? resolveClientBuildId() + const serverBuildId = options?.serverBuildId + if (!clientBuildId || !serverBuildId) return + if (clientBuildId === 'unknown' || serverBuildId === 'unknown') return + + const reload = options?.reload ?? (() => window.location.reload()) + const storage = options?.storage ?? window.sessionStorage + + if (clientBuildId === serverBuildId) { + try { + storage.removeItem(SERVER_BUILD_RELOAD_SENTINEL) + } catch { + // Ignore sessionStorage access failures. + } + return + } + + try { + if (storage.getItem(SERVER_BUILD_RELOAD_SENTINEL) === '1') { + log.warn( + `server build ${serverBuildId} still differs from client build ${clientBuildId}; ` + + 'one reload already attempted this tab session — suppressing further reloads', + ) + return + } + storage.setItem(SERVER_BUILD_RELOAD_SENTINEL, '1') + } catch { + // Cannot persist the sentinel: reloading without it risks a loop. + return + } + log.warn( + `server build ${serverBuildId} differs from client build ${clientBuildId}; ` + + 'reloading once to pick up the matching client bundle', + ) + reload() +} +``` + +3b. In `config/vite/vite.config.ts` — add the import at the top (with the other node imports, after line 5): + +```typescript +import { execFileSync } from 'node:child_process' +``` + +Add the helper after `projectRoot` (line 10): + +```typescript +/** + * The client's build identity: the git commit the bundle was built from, + * matching the server-side stamp (`crates/freshell-server/src/diag.rs`'s + * `build_commit()` / `server/build-id.ts`). `"unknown"` fallback — the + * client's compare rule ignores `"unknown"` on both sides. + */ +function computeClientBuildId(): string { + try { + const sha = execFileSync('git', ['rev-parse', 'HEAD'], { + cwd: projectRoot, + stdio: ['ignore', 'pipe', 'ignore'], + }) + .toString() + .trim() + return /^[0-9a-f]{40}$/.test(sha) ? sha : 'unknown' + } catch { + return 'unknown' + } +} +``` + +Extend the existing `define` block (lines 58-60): + +```typescript + define: { + __PERF_LOGGING__: JSON.stringify(env.PERF_LOGGING || ''), + __FRESHELL_BUILD_ID__: JSON.stringify(computeClientBuildId()), + }, +``` + +3c. In `src/vite-env.d.ts`, add after line 12: + +```typescript +declare const __FRESHELL_BUILD_ID__: string +``` + +3d. In `src/App.tsx`: + +Add the import near the other `@/lib` imports (after the `installTestHarness` import at line 35): + +```typescript +import { checkServerBuildId } from '@/lib/server-build-check' +``` + +Extend `ReadyMessageSchema` (lines 157-166), after the `bootId` line: + +```typescript + bootId: z.string().min(1).optional(), + // The server's baked build identity (additive/optional — old servers omit + // it). Compared in checkServerBuildId below; must never fail the WHOLE + // ready frame, hence optional + min(1) only. + buildId: z.string().min(1).optional(), +``` + +Add the call inside the `else` (ready-success) branch, immediately after the `if (!newBootId) { ... }` warn block that ends at line 1031: + +```typescript + // Server-build mismatch detection: the server stamps the git + // commit it was built from (ready.buildId, additive/optional); + // we compare it against our own Vite-baked + // __FRESHELL_BUILD_ID__ and reload ONCE on a mismatch (sentinel + // loop-guard lives in src/lib/server-build-check.ts). + checkServerBuildId({ serverBuildId: ready.data.buildId }) +``` + +- [ ] **Step 4: Run the focused tests** + +```bash +npm run test:vitest -- run test/unit/client/lib/server-build-check.test.ts test/unit/client/components/App.restart-signals.test.tsx +``` + +Expected: PASS. + +- [ ] **Step 5: Refactor while green** + +Verify the Vite define actually bakes the sha into the bundle: + +```bash +npm run build:client +rg -o "$(git rev-parse HEAD)" dist/client/assets/*.js | head -1 +``` + +Expected: at least one match (the baked sha appears in the built bundle). (`npm run build:client` from this worktree writes the worktree's own `dist/client` — the main-checkout `npm run build` production-server guard does not apply here.) + +- [ ] **Step 6: Run impacted-test verification** + +`ReadyMessageSchema` and App's ready handling are shared client-critical paths and the define constant touches the whole client build; the impacted set is the client unit suite plus typecheck and lint: + +```bash +npm run typecheck:client +npm run lint +npm run test:vitest -- run test/unit/client +``` + +Expected: all PASS. + +- [ ] **Step 7: Commit the task** + +```bash +git add src/lib/server-build-check.ts config/vite/vite.config.ts src/vite-env.d.ts src/App.tsx test/unit/client/lib/server-build-check.test.ts test/unit/client/components/App.restart-signals.test.tsx +git commit -m "feat(client): reload once when ready.buildId differs from the baked build id" +``` + +--- + +### Task 3: E2E proof (rust-chromium) + docs + +**Files:** +- Create: `test/e2e-browser/specs/server-build-mismatch-rust.spec.ts` +- Modify: `test/e2e-browser/playwright.config.ts` (add the spec to `RUST_ONLY_SPECS`, whose `/create-protection-isolation-rust\.spec\.ts$/,` entry is at line 204; add the spec to the `rust-chromium` project's `testMatch`, whose `/codex-terminal-bounce-rust\.spec\.ts$/,` entry is at line 371) +- Modify: `AGENTS.md` (one-line note under "Key Architectural Patterns → WebSocket Protocol") + +**Interfaces:** +- Consumes: Tasks 1-2 (both servers stamp `ready.buildId`; the client compares and reloads once; `TestHarness.receiveWsMessage` → `ws.receiveMessageForTest` → `handleIncomingMessage` feeds an injected frame through the real App ready handler — verified at `src/lib/ws-client.ts:917-919`). +- Produces: the user-outcome proof — a stale client against a newer server reboots itself exactly once and converges to a healthy ready connection; repeat mismatches are suppressed by the sentinel. + +- [ ] **Step 1: Write the failing behavioral test** + +Create `test/e2e-browser/specs/server-build-mismatch-rust.spec.ts`: + +```typescript +/** + * Server-build mismatch auto-reload (the-usual/server-version-reload). + * + * The user story: a tab running a client bundle built at commit A connects + * to a server built at commit B; the server's `ready.buildId` differs from + * the client's baked `__FRESHELL_BUILD_ID__`; the client reloads EXACTLY + * ONCE (sentinel `freshell.server-build-reload` in sessionStorage) and + * converges to a healthy ready connection. A repeat mismatched ready must + * NOT reload again — a half-deployed server can never reload-loop. + * + * Mismatch is injected with `harness.receiveWsMessage` (a REAL server + * stamps its own sha, which may or may not equal this worktree's client + * bake — the injection makes the compare deterministic either way). The + * injected frame flows through the production pipeline: ws-client + * `receiveMessageForTest` → `handleIncomingMessage` → App's ready handler + * → `ReadyMessageSchema` → `checkServerBuildId`. + * + * Service workers are blocked (perf-harness precedent, + * recover-my-panes-rust.spec.ts's FRESH_CONTEXT_OPTIONS) so the count of + * navigations is exactly the reloads this feature performs. + * + * Rust-only: registers under `rust-chromium` + RUST_ONLY_SPECS (owns a + * RustServer directly, the e2eServerKind seam not used). + */ +import { test, expect } from '../helpers/fixtures.js' +import { RustServer, ensureRustServerBuilt } from '../helpers/rust-server.js' +import type { TestServerInfo } from '../helpers/test-server.js' +import { TestHarness } from '../helpers/test-harness.js' + +const MISMATCHED_BUILD_ID = 'f'.repeat(40) +const SENTINEL = 'freshell.server-build-reload' + +test.describe('server build mismatch reload (rust)', () => { + let server: RustServer | undefined + let info: TestServerInfo + + test.beforeAll(async () => { + test.setTimeout(600_000) // first release build of freshell-server can take minutes + ensureRustServerBuilt() + server = new RustServer() + info = await server.start() + }) + + test.afterAll(async () => { + await server?.stop().catch(() => {}) + }) + + test('mismatched ready buildId reloads exactly once and converges; the sentinel suppresses repeats', async ({ browser }) => { + const context = await browser.newContext({ serviceWorkers: 'block' }) + const page = await context.newPage() + await page.goto(`${info.baseUrl}/?token=${info.token}&e2e=1`) + let harness = new TestHarness(page) + await harness.waitForHarness() + await harness.waitForConnection() + + // Start counting AFTER the boot-time compare so the real ready's own + // match/mismatch outcome (both artifacts usually share this worktree's + // HEAD) cannot pollute the count; also re-clear the sentinel so the + // injected mismatch is the one that arms it. + await page.evaluate((key) => sessionStorage.removeItem(key), SENTINEL) + let navigations = 0 + page.on('framenavigated', () => { navigations++ }) + + // 1) Injected mismatch → exactly one reload, and the page reboots into a + // healthy ready connection (convergence). + await harness.receiveWsMessage({ + type: 'ready', + timestamp: new Date().toISOString(), + serverInstanceId: 'srv-build-mismatch-probe', + bootId: 'boot-build-mismatch-probe', + buildId: MISMATCHED_BUILD_ID, + }) + await expect.poll(() => navigations, { timeout: 20_000 }).toBe(1) + harness = new TestHarness(page) + await harness.waitForHarness() + await harness.waitForConnection() + + // 2) Re-arm explicitly, then inject the SAME mismatch again: the + // sentinel must suppress the reload (no loop). The explicit re-arm + // keeps this assertion deterministic regardless of whether the real + // server's ready matched the client bake (which would self-clear). + await page.evaluate((key) => sessionStorage.setItem(key, '1'), SENTINEL) + await harness.receiveWsMessage({ + type: 'ready', + timestamp: new Date().toISOString(), + serverInstanceId: 'srv-build-mismatch-probe', + bootId: 'boot-build-mismatch-probe', + buildId: MISMATCHED_BUILD_ID, + }) + await page.waitForTimeout(3_000) + expect(navigations, 'sentinel must suppress the second mismatched ready').toBe(1) + + await context.close() + }) +}) +``` + +Register the spec in `test/e2e-browser/playwright.config.ts`: + +In `RUST_ONLY_SPECS`, after the `/create-protection-isolation-rust\.spec\.ts$/,` entry: + +```typescript + // Server-build mismatch auto-reload: injects a mismatched ready.buildId + // through the test harness and proves ONE sentinel-guarded reload. + // Rust-only: owns a RustServer directly (see the spec header). + /server-build-mismatch-rust\.spec\.ts$/, +``` + +In the `rust-chromium` project's `testMatch` array, after the `/codex-terminal-bounce-rust\.spec\.ts$/,` entry: + +```typescript + // Server-build mismatch auto-reload (the-usual/server-version-reload): + // mismatched ready.buildId → one reload, sentinel suppresses repeats. + /server-build-mismatch-rust\.spec\.ts$/, +``` + +In `AGENTS.md`, under "Key Architectural Patterns", append to the **WebSocket Protocol** paragraph: + +``` +The `ready` frame carries an optional additive `buildId` (the server's baked git commit, `"unknown"` fallback): the client bakes its own at Vite build time (`__FRESHELL_BUILD_ID__`) and, on a mismatch, reloads exactly once per tab session (sessionStorage sentinel `freshell.server-build-reload`), self-healing stale-client contract errors; `"unknown"` on either side never triggers or clears the guard (`src/lib/server-build-check.ts`). +``` + +- [ ] **Step 2: Run the test and verify it passes, then RED-VERIFY it exercises the feature** + +With Tasks 1-2 landed the behavior exists, so the fresh test should be green — but a green-only run is not sufficient proof. First run it green: + +```bash +npx playwright test --config test/e2e-browser/playwright.config.ts --project=rust-chromium test/e2e-browser/specs/server-build-mismatch-rust.spec.ts +``` + +Expected: PASS. + +Then prove it fails for the right reason: temporarily comment out the `checkServerBuildId(...)` call in `src/App.tsx`, rebuild the client, and re-run: + +```bash +npm run build:client +npx playwright test --config test/e2e-browser/playwright.config.ts --project=rust-chromium test/e2e-browser/specs/server-build-mismatch-rust.spec.ts +``` + +Expected: FAIL — `expect.poll` times out with `navigations` stuck at 0 (no reload happens without the compare). + +Restore the call and rebuild: + +```bash +npm run build:client +npx playwright test --config test/e2e-browser/playwright.config.ts --project=rust-chromium test/e2e-browser/specs/server-build-mismatch-rust.spec.ts +``` + +Expected: PASS. (Record all three runs in the task review — the red-verification is mandatory.) + +- [ ] **Step 3: No production implementation step** + +Tasks 1-2 implemented the behavior; this task only proves it end to end. + +- [ ] **Step 4: Run the focused test** + +Same command as Step 2's final run. Expected: PASS. + +- [ ] **Step 5: Refactor while green** + +No refactor needed. Confirm the spec is excluded from the match-all `chromium` project by the `RUST_ONLY_SPECS` entry (`testIgnore: RUST_ONLY_SPECS` at `playwright.config.ts:330`) and runs ONLY under `rust-chromium`. + +- [ ] **Step 6: Run impacted-test verification** + +Playwright registration changed (a new rust-only spec) and AGENTS.md was touched; the impacted set is the rust-chromium smoke that boots a real server (proving the registration change disturbed nothing) plus the two unit files most adjacent to the feature as a final belt-and-suspenders: + +```bash +npx playwright test --config test/e2e-browser/playwright.config.ts --project=rust-chromium test/e2e-browser/specs/continuity-smoke.spec.ts test/e2e-browser/specs/server-build-mismatch-rust.spec.ts +npm run test:vitest -- run test/unit/client/lib/server-build-check.test.ts test/unit/client/components/App.restart-signals.test.tsx +``` + +Expected: all PASS. + +- [ ] **Step 7: Commit the task** + +```bash +git add test/e2e-browser/specs/server-build-mismatch-rust.spec.ts test/e2e-browser/playwright.config.ts AGENTS.md +git commit -m "test(e2e): rust spec proves one-shot sentinel-guarded reload on ready.buildId mismatch" +``` + +--- + +## Post-execution verification (after Task 3) + +Run the coordinated full suite once, from the worktree: + +```bash +npm run check +``` + +Expected: typecheck + full default + server suites PASS (the electron suite is unaffected but runs as part of the coordinated run). + +**User-outcome recap (maps every requirement to its proof):** + +| Requirement | Production behavior | Proof | +| --- | --- | --- | +| Server stamps build identity in `ready` | Rust `WsState.build_id` → `Ready.build_id` (`Some`, sha/`"unknown"`); Node `serverBuildId()` → `buildId`; schema regenerated | roundtrip + wire tests; `test:port`; Node snapshot test | +| Identity = git HEAD, `"unknown"` fallback, everywhere | `build.rs` (existing, HEAD-move aware), `server/build-id.ts`, `computeClientBuildId()` | `build-id.test.ts`; bundle-bake check (Task 2 Step 5) | +| Client compares on every `ready` | `ReadyMessageSchema.buildId` → `checkServerBuildId` in App's ready handler | App.restart-signals describe block | +| Mismatch → reload exactly once | sentinel set before `reload()`; armed sentinel suppresses | unit matrix; e2e navigation count === 1 | +| Never reload-loops (incl. storage failure, repeated mismatches) | fail-safe catch; suppression branch; `"unknown"` no-op | unit cases; e2e repeat-injection step | +| Match clears the sentinel (self-re-arm) | removeItem on equal ids | unit cases; App re-arm test | +| Old servers/forks unaffected (additive contract) | optional field, omitted when `None`; schema stays `additionalProperties: false` | frozen transcript roundtrip; contract-freeze + mutation suites | +| Real-world convergence | reloaded page reconnects and reaches ready | e2e `waitForConnection` after reload | From dec5188b95dfa94dc5126872414256785d9b97c9 Mon Sep 17 00:00:00 2001 From: Dan Shapiro <3732858+danshapiro@users.noreply.github.com> Date: Thu, 27 Aug 2026 11:02:29 -0700 Subject: [PATCH 02/15] docs: harden server-version-reload plan from load-bearing findings --- .../plans/2026-08-27-server-version-reload.md | 57 ++++++++++++++++--- 1 file changed, 48 insertions(+), 9 deletions(-) diff --git a/docs/plans/2026-08-27-server-version-reload.md b/docs/plans/2026-08-27-server-version-reload.md index c62e9a295..da94506fd 100644 --- a/docs/plans/2026-08-27-server-version-reload.md +++ b/docs/plans/2026-08-27-server-version-reload.md @@ -344,7 +344,7 @@ No refactor needed — every addition mirrors the adjacent `bootId` idiom on its - [ ] **Step 6: Run impacted-test verification** -This change touches the shared wire protocol, both server implementations, and the generated schema, so the impacted set is: both Rust crates' full test trees, the workspace compile of every literal site, the whole server-config suite (any test asserting handshake/ready shapes), and the port oracle suites. Note on `t0-equivalence-rust.test.ts`: its node-vs-rust deep diff compares `ready` frames value-by-value — both sides now stamp the SAME value (the worktree HEAD sha; `ensureRustServerBuilt` runs `cargo build` at test time and `build.rs` re-stamps on HEAD moves; Node computes the same sha at runtime), or both `"unknown"` in git-less environments, so the diff stays clean. +This change touches the shared wire protocol, both server implementations, and the generated schema, so the impacted set is: both Rust crates' full test trees, the workspace compile of every literal site, the whole server-config suite (any test asserting handshake/ready shapes), and the port-oracle suites. **`npm run test:port` does NOT run the oracle suites** (`vitest.port.config.ts` excludes `test/unit/port/oracle/**`; they are deliberately outside the coordinator and run only via `npm run test:oracle`, which boots real servers — budget several minutes). Note on `t0-equivalence-rust.test.ts`: its node-vs-rust deep diff compares `ready` frames value-by-value (`buildId` is NOT in the normalization registry, so it is compared RAW) — both sides stamp the SAME value (the worktree HEAD sha: the oracle node target runs from an isolated runtime root under the worktree so `git rev-parse HEAD` walk-up resolves the worktree sha; the rust target is `cargo build`-ed at test time by `ensureRustServerBuilt` and `build.rs` re-stamps on HEAD moves; Node computes the same sha at runtime), or both `"unknown"` in git-less environments, so the diff stays clean — and this run is the proof. ```bash cargo test -p freshell-protocol @@ -352,6 +352,7 @@ cargo test -p freshell-ws cargo check --workspace --all-targets npm run test:integration npm run test:port +npm run test:oracle ``` Expected: all PASS. @@ -471,7 +472,15 @@ describe('checkServerBuildId', () => { it('falls back to the __FRESHELL_BUILD_ID__ global and window defaults when options are omitted', () => { vi.stubGlobal('__FRESHELL_BUILD_ID__', 'c'.repeat(40)) const reload = vi.fn() - Object.defineProperty(window.location, 'reload', { value: reload, configurable: true, writable: true }) + // jsdom 25's Location owns `reload` non-configurably — defineProperty on + // window.location itself throws. Repo precedent (import-retry.test.ts): + // replace window-level with a spread copy. + const originalLocation = window.location + Object.defineProperty(window, 'location', { + value: { ...window.location, reload }, + writable: true, + configurable: true, + }) sessionStorage.clear() checkServerBuildId({ serverBuildId: 'd'.repeat(40) }) @@ -483,6 +492,12 @@ describe('checkServerBuildId', () => { sessionStorage.removeItem(SENTINEL) checkServerBuildId({ serverBuildId: 'd'.repeat(40) }) expect(reload).toHaveBeenCalledTimes(1) + + Object.defineProperty(window, 'location', { + value: originalLocation, + writable: true, + configurable: true, + }) }) }) ``` @@ -491,6 +506,7 @@ describe('checkServerBuildId', () => { ```tsx describe('App ready buildId → one-shot server-build reload', () => { + let originalLocation: Location beforeEach(() => { cleanup() vi.resetAllMocks() @@ -529,16 +545,25 @@ describe('App ready buildId → one-shot server-build reload', () => { }) sessionStorage.clear() - Object.defineProperty(window.location, 'reload', { - value: vi.fn(), - configurable: true, + // jsdom 25's Location owns `reload` non-configurably — defineProperty on + // window.location itself throws. Repo precedent (import-retry.test.ts): + // window-level replacement with save/restore. + originalLocation = window.location + Object.defineProperty(window, 'location', { + value: { ...window.location, reload: vi.fn() }, writable: true, + configurable: true, }) }) afterEach(() => { cleanup() vi.unstubAllGlobals() + Object.defineProperty(window, 'location', { + value: originalLocation, + writable: true, + configurable: true, + }) sessionStorage.clear() }) @@ -633,6 +658,11 @@ function resolveClientBuildId(): string | undefined { * further reloads this tab session (a half-deployed server can never * reload-loop; sessionStorage access failure = no reload, fail-safe); * - a MATCHING ready clears the sentinel (self-re-arm after convergence). + * KNOWN LIMIT (accepted for the self-hosted single-server threat model): + * the "once" guarantee is per server identity — one origin fronted by + * servers built from DIFFERENT commits can oscillate (mismatch → reload → + * match clears → mismatch → …). Deliberately not hardened with a + * clears-per-session cap; revisit only if a split-deploy origin appears. */ export function checkServerBuildId(options?: ServerBuildCheckOptions): void { const clientBuildId = options?.clientBuildId ?? resolveClientBuildId() @@ -922,11 +952,17 @@ In the `rust-chromium` project's `testMatch` array, after the `/codex-terminal-b In `AGENTS.md`, under "Key Architectural Patterns", append to the **WebSocket Protocol** paragraph: ``` -The `ready` frame carries an optional additive `buildId` (the server's baked git commit, `"unknown"` fallback): the client bakes its own at Vite build time (`__FRESHELL_BUILD_ID__`) and, on a mismatch, reloads exactly once per tab session (sessionStorage sentinel `freshell.server-build-reload`), self-healing stale-client contract errors; `"unknown"` on either side never triggers or clears the guard (`src/lib/server-build-check.ts`). +The `ready` frame carries an optional additive `buildId` (the server's baked git commit, `"unknown"` fallback): the client bakes its own at Vite build time (`__FRESHELL_BUILD_ID__`) and, on a mismatch, reloads exactly once per tab session (sessionStorage sentinel `freshell.server-build-reload`), self-healing stale-client contract errors; `"unknown"` on either side never triggers or clears the guard (`src/lib/server-build-check.ts`). The once-guard is per server identity: an origin fronted by mixed-build servers could oscillate (accepted for the single-server self-hosted model). ``` - [ ] **Step 2: Run the test and verify it passes, then RED-VERIFY it exercises the feature** +Build the client fresh first so the served bundle provably contains the feature (the red-verification's validity depends on it): + +```bash +npm run build:client +``` + With Tasks 1-2 landed the behavior exists, so the fresh test should be green — but a green-only run is not sufficient proof. First run it green: ```bash @@ -963,7 +999,7 @@ Same command as Step 2's final run. Expected: PASS. - [ ] **Step 5: Refactor while green** -No refactor needed. Confirm the spec is excluded from the match-all `chromium` project by the `RUST_ONLY_SPECS` entry (`testIgnore: RUST_ONLY_SPECS` at `playwright.config.ts:330`) and runs ONLY under `rust-chromium`. +No refactor needed. Confirm the spec is excluded from the match-all `chromium` project by the `RUST_ONLY_SPECS` entry (`testIgnore: RUST_ONLY_SPECS` at `playwright.config.ts:330`) and runs ONLY under `rust-chromium`. Note the spec also runs on the CLOUD e2e lane when `FRESHELL_E2E_BACKEND=cloud` (`playwright.cloud.config.ts` filters only firefox/webkit/continuity-smoke, so `rust-chromium` survives; the spec is not in `CLOUD_SKIP_SPECS`) — do not add it there; coverage comes from Step 6's backend run. - [ ] **Step 6: Run impacted-test verification** @@ -976,6 +1012,8 @@ npm run test:vitest -- run test/unit/client/lib/server-build-check.test.ts test/ Expected: all PASS. +**Backend proof (repo rule: an affected e2e spec must pass on the configured `FRESHELL_E2E_BACKEND` before a PR is filed):** before the branch is PR'd, run this spec on the configured backend — if `FRESHELL_E2E_BACKEND=cloud`, `npm run test:e2e:cloud` filtered to this spec (this also proves `cargo` availability and the build stamp inside the cloud image); if unset/local, the local runs above satisfy the rule. This is a pre-PR gate recorded in the run log, not part of the task commit. + - [ ] **Step 7: Commit the task** ```bash @@ -987,13 +1025,14 @@ git commit -m "test(e2e): rust spec proves one-shot sentinel-guarded reload on r ## Post-execution verification (after Task 3) -Run the coordinated full suite once, from the worktree: +Run the coordinated full suite once, from the worktree, plus the oracle suites (which `npm run check` deliberately does NOT cover — they live outside the coordinator): ```bash npm run check +npm run test:oracle ``` -Expected: typecheck + full default + server suites PASS (the electron suite is unaffected but runs as part of the coordinated run). +Expected: typecheck + full default + server suites PASS, and the oracle suites (t0-equivalence, handshake-determinism, external-handshake, mutation-validation) PASS — `npm run test:oracle` boots real servers and cargo-builds the workspace, so budget several minutes. Also confirm the Task 3 backend proof was recorded (the spec passing on the configured `FRESHELL_E2E_BACKEND`). **User-outcome recap (maps every requirement to its proof):** From 81b6a08974093062e888ceb5c48c331870ee318a Mon Sep 17 00:00:00 2001 From: Dan Shapiro <3732858+danshapiro@users.noreply.github.com> Date: Thu, 27 Aug 2026 11:32:01 -0700 Subject: [PATCH 03/15] docs: rework server-version-reload plan from fresh-eyes round 1 --- .../plans/2026-08-27-server-version-reload.md | 567 ++++++++++++++---- 1 file changed, 456 insertions(+), 111 deletions(-) diff --git a/docs/plans/2026-08-27-server-version-reload.md b/docs/plans/2026-08-27-server-version-reload.md index da94506fd..cf77c659e 100644 --- a/docs/plans/2026-08-27-server-version-reload.md +++ b/docs/plans/2026-08-27-server-version-reload.md @@ -6,20 +6,24 @@ **Goal:** When a browser tab connects (or reconnects) to a Freshell server built from a different commit than the client bundle it is running, the client detects the mismatch from the WS `ready` frame and reloads itself exactly once — self-healing the "Fresh-agent snapshot response did not match the shared contract" class of stale-client failures without ever reload-looping. -**Architecture:** All three producers stamp the same identity — `git rev-parse HEAD` with literal `"unknown"` fallback (Rust at compile time via the existing `build.rs`, Node at first use via a new cached module, client at Vite build time via a `define` constant). The WS `ready` message gains an additive optional `buildId` field (omitted from the wire when the Rust value is `None`, so frozen transcripts stay byte-identical; the Node frame always stamps it, mirroring `bootId`). The client compares its baked `__FRESHELL_BUILD_ID__` against every parsed `ready.buildId`; on mismatch it sets a `sessionStorage` sentinel and calls `location.reload()` exactly once per tab session (the sentinel also self-clears on a subsequent match, re-arming the guard). +**Architecture:** All three producers stamp the same identity — `git rev-parse HEAD` with literal `"unknown"` fallback — each baked/resolved at the time its ARTIFACT is produced: Rust at compile time (new `crates/freshell-ws/build.rs`, mirroring the existing `freshell-server/build.rs`), Node at `build:server` time (a bake script writes `dist/server/build-id.json`, which the server prefers over a runtime git probe; dev mode via tsx correctly falls back to the runtime probe because it runs current source), client at Vite build time via a `define` constant. The WS `ready` message gains an additive optional `buildId` field (omitted from the wire when the Rust value is `None`, so frozen transcripts stay byte-identical; the Node frame always stamps it, mirroring `bootId`). The client compares its baked `__FRESHELL_BUILD_ID__` against every parsed `ready.buildId`; on mismatch it sets a `sessionStorage` sentinel and calls `location.reload()` exactly once per tab session (the sentinel also self-clears on a subsequent match, re-arming the guard). -**Tech Stack:** Rust (serde/serde_json, tokio, existing `freshell-protocol`/`freshell-ws` crates), Node.js/ESM (`node:child_process`), React 18 + Vite `define` + Zod, Vitest (jsdom client config / node server config), Playwright (rust-chromium project). +**Tech Stack:** Rust (serde/serde_json, tokio, build scripts, existing `freshell-protocol`/`freshell-ws` crates), Node.js/ESM (`node:child_process`, `node:fs`), React 18 + Vite `define` + Zod, Vitest (jsdom client config / node server config), Playwright (rust-chromium project, local lane). ## Global Constraints - **Worktree discipline:** All work happens in `/home/dan/code/freshell/.worktrees/server-version-reload` on branch `the-usual/server-version-reload`. Never run `node dist/server/index.js`; never touch the live 3001 server or `~/.freshell` state. No deploy/restart is part of this plan. -- **Additive contract only ("bootId doctrine"):** `buildId` is optional everywhere and omitted from the wire when the Rust value is `None`. Old clients must not break; the frozen `port/oracle/fixtures/handshake-transcript.json` must remain byte-valid without regenerating it. The Node ready frame ALWAYS stamps `buildId` (string, `"unknown"` fallback) — mirroring how it always stamps `bootId` — and the Rust handshake always stamps `Some(...)` from `WsState.build_id`. Both servers MUST stamp in the same commit (Task 1): the T0 oracle deep-diffs node-vs-rust handshakes, so an intermediate where only one side stamps would fail it. -- **Value semantics on every side:** the value is the full `git rev-parse HEAD` SHA of the repo at build/bake time; when git is unavailable or the output is not 40 lowercase hex chars, the literal `"unknown"`. The client's compare rule: reload iff BOTH ids are present, non-empty, neither is `"unknown"`, and they differ. `"unknown" == "unknown"` is NOT a match-and-clear (it is a no-op) — two unknown builds must never trigger a reload and must never clear an armed sentinel. -- **Loop-guard invariant:** at most ONE reload per tab session. The sentinel key is `freshell.server-build-reload` (`sessionStorage`, value `"1"`), set BEFORE calling `reload()`. If `sessionStorage` throws, no reload happens (fail-safe). A matching `ready` clears the sentinel (self-re-arm). +- **Additive contract only ("bootId doctrine"):** `buildId` is optional everywhere and omitted from the wire when the Rust value is `None`. Old clients must not break; the frozen `port/oracle/fixtures/handshake-transcript.json` must remain byte-valid without regenerating it. The Node ready frame ALWAYS stamps `buildId` (string, `"unknown"` fallback) — mirroring how it always stamps `bootId` — and the Rust handshake stamps `Some(...)` from its crate-baked constant. +- **Build-scoped, not boot-scoped:** build provenance is a compile-time property of the code, so it does NOT ride on `WsState` (whose doc comment scopes it to boot-scoped ids injected by `freshell-server`). `freshell-ws` bakes its own constant via its own `build.rs`; the value equals `freshell-server`'s bake because both crates compile in the same `cargo build` at the same HEAD. +- **Artifact-time semantics everywhere:** each stamp describes the artifact that emits it. Rust bakes at compile; Node's production stamp comes from the `dist/server/build-id.json` written by `build:server` (a stale dist advertises the sha it was BUILT from — never the checkout's current HEAD); tsx dev mode has no bake file next to source and probes runtime HEAD (correct: it runs current source); Vite bakes the client's sha at bundle time. +- **Value semantics on every side:** the value is the full `git rev-parse HEAD` SHA of the repo at build/bake time; when git is unavailable or the output is not 40 lowercase hex chars (Node/Vite enforce the 40-hex check; the Rust scripts accept any successful output), the literal `"unknown"`. Known caveat (accepted, documented): a SHA-256 git checkout would make Rust stamp 64 hex while Node/Vite stamp `"unknown"` — the guard goes inert (no false reloads, no crash); this repo is SHA-1. +- **Client compare rule:** reload iff BOTH ids are present, non-empty, neither is `"unknown"`, and they differ. `"unknown" == "unknown"` is NOT a match-and-clear (it is a no-op) — two unknown builds must never trigger a reload and must never clear an armed sentinel. The compare is direction-free: a NEWER client against an OLDER server also performs one bounded reload per fresh tab session (futile but harmless; shas carry no ordering) — documented, accepted. +- **Loop-guard invariant:** at most ONE code-triggered reload per tab session, per server identity. The sentinel key is `freshell.server-build-reload` (`sessionStorage`, value `"1"`), set BEFORE calling `reload()`. If `sessionStorage` cannot be read or written (property access throwing a SecurityError, quota errors, absent API), no reload happens and the suppression failure is logged (fail-safe with observability). A matching `ready` clears the sentinel (self-re-arm). KNOWN LIMIT (accepted, documented): one origin fronted by servers built from DIFFERENT commits could oscillate (mismatch → reload → match clears → mismatch → …); deliberately not hardened with a clears-per-session cap for the single-server self-hosted threat model. - **Client module must not crash under Vitest:** the Vitest client config has no `__FRESHELL_BUILD_ID__` define, so the module must use a `typeof __FRESHELL_BUILD_ID__ === 'undefined'` guard (same precedent as `src/lib/perf-logger.ts:45` with `__PERF_LOGGING__`). - **NodeNext/ESM:** every relative import in `server/` and `shared/` uses `.js` extensions; client code uses `@/` aliases without extensions. -- **Test coordination:** broad suites go through the repo coordinator (`npm run test:vitest -- run ...`); never raw `npx vitest`. Focused Rust tests use `cargo test -p ` directly. -- **Scope boundary:** client-only redeploys (redeploying a new client bundle WITHOUT a server change) are deliberately NOT covered — no auto-refresh loop, no polling, no `/api/server-info` polling fallback. The ready-frame compare is the only trigger. +- **Test coordination:** broad suites go through the repo coordinator (`npm run test:vitest -- run ...`); never raw `npx vitest`. Focused Rust tests use `cargo test -p ` directly. The port-ORACLE suites are NOT covered by `npm run test:port` / `npm run check` — they run only via `npm run test:oracle`. +- **E2E backend rule:** per repo instructions, when `FRESHELL_E2E_BACKEND` is unset the user chooses local vs cloud before e2e runs — surface that question once at execution kickoff and record the answer in `run-state.md`. This feature's e2e coverage lane is the LOCAL `rust-chromium` project regardless: the new spec is added to `CLOUD_SKIP_SPECS` with a technical justification (the cloud image builds without git metadata, so both stamps are `"unknown"` and the compare is inert there — see Task 3). Never claim a cloud run proves cargo availability: the cloud runtime uses a prebuilt binary and cargo never runs there (`test/e2e-browser/helpers/rust-server.ts:82-90`). +- **Scope boundary:** client-only redeploys (redeploying a new client bundle WITHOUT a server change) are deliberately NOT covered by any auto-trigger — no polling, no `/api/server-info` fallback, no reload loop. The ready-frame compare is the only trigger; a client-only redeploy costs at most one bounded reload per fresh tab session. - **No unrelated restructuring; comments explain invariants, in the existing voice.** --- @@ -29,20 +33,21 @@ **Files:** - Modify: `shared/ws-protocol.ts:743-750` (`ReadyMessage` type) - Modify: `crates/freshell-protocol/src/server_messages.rs:792-806` (`Ready` struct) -- Modify: `crates/freshell-ws/src/lib.rs:97-124` (`WsState` struct field), `:529-546` (`build_handshake_with_capabilities`), `:868-917` (`state()` test builder) -- Modify: `crates/freshell-server/src/main.rs:1011-1066` (`WsState` literal) +- Create: `crates/freshell-ws/build.rs` (crate-local commit bake; adapted from `crates/freshell-server/build.rs`) +- Modify: `crates/freshell-ws/src/lib.rs` (`ready_build_id()` helper + handshake `Ready` literal stamp at :536; wire test in `mod tests` after `handshake_is_ordered_with_shared_bootid` ending :1026) - Modify: `crates/freshell-protocol/tests/pane_reconcile.rs:52-82` (two `Ready` literals) +- Modify: `package.json` (`build:server` script gains the bake step) +- Create: `scripts/bake-server-build-id.mjs` - Create: `server/build-id.ts` - Modify: `server/ws-handler.ts` (import block; field after `:587`; init after `:651`; ready send `:2034-2039`) - Modify (generated): `port/contract/ws-server-messages.schema.json` (via `npm run contract:generate`) - Test: `crates/freshell-protocol/tests/roundtrip.rs` (new test after `ready_carries_server_instance_id_and_boot_id`, which ends at line 164) -- Test: `crates/freshell-ws/src/lib.rs` `#[cfg(test)] mod tests` (new test after `handshake_is_ordered_with_shared_bootid`, which ends at line 1026) - Test: `test/server/build-id.test.ts` (new) - Test: `test/server/ws-handshake-snapshot.test.ts` (new test after the `includes a bootId in the ready message...` test, which ends at line 301) **Interfaces:** -- Consumes: `crates/freshell-server/src/diag.rs:124` `pub(crate) fn build_commit() -> &'static str` (already returns the baked `FRESHELL_BUILD_COMMIT` or `"unknown"`; `build.rs` re-stamps on HEAD moves — no change needed there). -- Produces: `freshell_protocol::Ready { build_id: Option }` (serde camelCase → wire key `buildId`, skipped when `None`); `freshell_ws::WsState { build_id: Arc }`; `server/build-id.ts` exporting `computeBuildId(cwd?: string): string` (pure) and `serverBuildId(): string` (cached per process); TS `ReadyMessage.buildId?: string`; regenerated `port/contract/ws-server-messages.schema.json` with an optional `buildId` on `ready` (still `additionalProperties: false`). Task 2's client schema and Task 3's e2e injection consume the wire key `buildId`. +- Consumes: nothing new — `crates/freshell-server/build.rs` and `diag.rs:124` are untouched (freshell-ws now bakes its own constant; both crates compile at the same HEAD in every workspace build, so the values agree). +- Produces: `freshell_protocol::Ready { build_id: Option }` (serde camelCase → wire key `buildId`, skipped when `None`); `freshell_ws::ready_build_id() -> Option` (the crate-baked sha or `"unknown"`, always `Some` in practice); `server/build-id.ts` exporting `computeBuildId(cwd?: string): string` (pure git probe), `readBakedBuildId(bakePath: string): string | undefined` (pure file read), `resolveServerBuildId(bakePath?: string): string` (bake-wins-else-probe), `serverBuildId(): string` (cached), `_resetServerBuildIdCacheForTests(): void`; `dist/server/build-id.json` (`{"buildId": ""}`) written by `build:server`; TS `ReadyMessage.buildId?: string`; regenerated `port/contract/ws-server-messages.schema.json` with an optional `buildId` on `ready` (still `additionalProperties: false`). Task 2's client schema and Task 3's e2e injection consume the wire key `buildId`. - [ ] **Step 1: Write the failing behavioral tests (protocol roundtrip, rust wire, node module, node wire)** @@ -82,16 +87,18 @@ fn ready_carries_build_id_and_omits_it_when_absent() { 1b. Add to `crates/freshell-ws/src/lib.rs` inside `mod tests`, immediately after `handshake_is_ordered_with_shared_bootid` (line 1026): ```rust - /// The handshake `ready` stamps the build identity (`WsState.build_id`, - /// baked from `diag::build_commit()` by `freshell-server`'s `main.rs`) so - /// the browser client can detect a client/server build mismatch and - /// reload once. Serde omits the field when `None`; a real server always - /// stamps `Some` (sha or `"unknown"`), so presence is asserted here. + /// The handshake `ready` stamps the build identity baked into THIS crate + /// by its `build.rs` (`FRESHELL_WS_BUILD_COMMIT`, the git commit the + /// binary was built from) so the browser client can detect a client/ + /// server build mismatch and reload once. Never absent on the wire from + /// a real server: the baked value is always `Some` (sha or `"unknown"`). #[tokio::test] async fn handshake_ready_stamps_build_id() { let msgs = build_handshake(&state()).await; let ready = serde_json::to_value(&msgs[0]).unwrap(); - assert_eq!(ready["buildId"], "build-3333"); + let baked = ready_build_id().expect("crate always bakes a build id"); + assert!(!baked.is_empty()); + assert_eq!(ready["buildId"], serde_json::json!(baked)); } ``` @@ -103,20 +110,48 @@ import * as fs from 'node:fs' import * as os from 'node:os' import * as path from 'node:path' import { fileURLToPath } from 'node:url' -import { describe, expect, it } from 'vitest' -import { computeBuildId, serverBuildId } from '../../server/build-id.js' +import { afterEach, describe, expect, it, vi } from 'vitest' +import { + _resetServerBuildIdCacheForTests, + computeBuildId, + readBakedBuildId, + resolveServerBuildId, + serverBuildId, +} from '../../server/build-id.js' const REPO_ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '../..') +vi.mock('node:child_process', async (importOriginal) => { + const actual = await importOriginal() + return { ...actual, execFileSync: vi.fn(actual.execFileSync) } +}) + +// The module under test imports execFileSync by name; re-import it mocked. +import { execFileSync as mockedExecFileSync } from 'node:child_process' + +function tempBakeFile(buildId: string | null): { dir: string; bakePath: string } { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'build-id-bake-')) + const bakePath = path.join(dir, 'build-id.json') + if (buildId !== null) { + fs.writeFileSync(bakePath, JSON.stringify({ buildId })) + } + return { dir, bakePath } +} + describe('server build id', () => { - it('returns the current git HEAD sha for the repository', () => { + afterEach(() => { + _resetServerBuildIdCacheForTests() + vi.mocked(mockedExecFileSync).mockClear() + }) + + it('computeBuildId returns the current git HEAD sha for the repository', () => { const expected = execFileSync('git', ['rev-parse', 'HEAD'], { cwd: REPO_ROOT }) .toString() .trim() expect(computeBuildId(REPO_ROOT)).toBe(expected) }) - it('falls back to "unknown" outside a git repository', () => { + it('computeBuildId falls back to "unknown" outside a git repository', () => { const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'build-id-no-git-')) try { expect(computeBuildId(dir)).toBe('unknown') @@ -125,8 +160,57 @@ describe('server build id', () => { } }) - it('caches the id within a process', () => { - expect(serverBuildId()).toBe(serverBuildId()) + it('readBakedBuildId returns the baked value for a well-formed file', () => { + const { dir, bakePath } = tempBakeFile('b'.repeat(40)) + try { + expect(readBakedBuildId(bakePath)).toBe('b'.repeat(40)) + } finally { + fs.rmSync(dir, { recursive: true, force: true }) + } + }) + + it('readBakedBuildId returns undefined for malformed JSON, wrong shapes, or a missing file', () => { + const { dir, bakePath } = tempBakeFile(null) + try { + fs.writeFileSync(bakePath, 'not json {') + expect(readBakedBuildId(bakePath)).toBeUndefined() + fs.writeFileSync(bakePath, JSON.stringify({ buildId: 42 })) + expect(readBakedBuildId(bakePath)).toBeUndefined() + fs.writeFileSync(bakePath, JSON.stringify({ buildId: '' })) + expect(readBakedBuildId(bakePath)).toBeUndefined() + expect(readBakedBuildId(path.join(dir, 'absent.json'))).toBeUndefined() + } finally { + fs.rmSync(dir, { recursive: true, force: true }) + } + }) + + it('resolveServerBuildId prefers the bake file over a runtime git probe', () => { + const { dir, bakePath } = tempBakeFile('c'.repeat(40)) + try { + expect(resolveServerBuildId(bakePath)).toBe('c'.repeat(40)) + } finally { + fs.rmSync(dir, { recursive: true, force: true }) + } + }) + + it('resolveServerBuildId falls back to the runtime git probe when no bake file exists', () => { + const { dir } = tempBakeFile(null) + try { + expect(resolveServerBuildId(path.join(dir, 'build-id.json'))).toBe(computeBuildId(REPO_ROOT)) + } finally { + fs.rmSync(dir, { recursive: true, force: true }) + } + }) + + it('serverBuildId memoizes: the git probe runs once per process', () => { + _resetServerBuildIdCacheForTests() + // Source runs (tsx/vitest) have no bake file next to server/build-id.ts, + // so the first resolution exercises the git probe. + const first = serverBuildId() + const callsAfterFirst = vi.mocked(mockedExecFileSync).mock.calls.length + expect(serverBuildId()).toBe(first) + expect(vi.mocked(mockedExecFileSync).mock.calls.length).toBe(callsAfterFirst) + expect(callsAfterFirst).toBeGreaterThan(0) }) }) ``` @@ -149,7 +233,8 @@ describe('server build id', () => { waitForReady(ws2, 10_000), ]) - // Always stamped (sha or "unknown" fallback), stable within the process. + // Always stamped (bake or runtime probe, "unknown" fallback), stable + // within the process. expect(typeof ready1.buildId).toBe('string') expect((ready1.buildId as string).length).toBeGreaterThan(0) expect(ready2.buildId).toBe(ready1.buildId) @@ -170,7 +255,7 @@ cargo test -p freshell-ws handshake_ready_stamps_build_id npm run test:vitest -- run test/server/build-id.test.ts test/server/ws-handshake-snapshot.test.ts --config config/vitest/vitest.server.config.ts ``` -Expected: all FAIL for the missing behavior — the two Rust commands fail to COMPILE (`no field \`build_id\` on struct Ready` / `no field \`build_id\` on struct WsState`); `build-id.test.ts` fails to resolve `../../server/build-id.js` (module missing); the new snapshot test fails on `expect(typeof ready1.buildId).toBe('string')` (the Node ready frame carries no `buildId`). +Expected: all FAIL for the missing behavior — the Rust roundtrip test fails to COMPILE (`no field \`build_id\` on struct Ready`); the freshell-ws wire test COMPILES (it references no new field) and fails its JSON assertion (`ready["buildId"]` is JSON null ≠ the baked string — the ready frame carries no `buildId`); `build-id.test.ts` fails to resolve `../../server/build-id.js` (module missing); the new snapshot test fails on `expect(typeof ready1.buildId).toBe('string')`. - [ ] **Step 3: Add the minimal production implementation** @@ -212,61 +297,187 @@ Expected: the regenerated diff adds an optional `buildId` property to the `ready pub build_id: Option, ``` -3d. `crates/freshell-ws/src/lib.rs` — in `WsState`, after the `boot_id` field (line 103): +3d. Create `crates/freshell-ws/build.rs` — crate-local commit bake, adapted from `crates/freshell-server/build.rs` (which keeps its own, dirty-flag-inclusive copy; the two crates compile in the same `cargo build` at the same HEAD, so the values agree). Keep the module doc short and point at the original for the full rationale: ```rust - /// The git commit this server binary was built from (`"unknown"` - /// fallback) — baked once per build by `freshell-server`'s `main.rs` from - /// `diag::build_commit()` and stamped into every handshake's `ready`. - pub build_id: Arc, -``` +//! Compile-time build-provenance stamp for `freshell-ws`: bakes the git +//! commit SHA into `FRESHELL_WS_BUILD_COMMIT` so the WS handshake's `ready` +//! can stamp `ready.buildId` (client-side stale-bundle auto-reload). +//! Build provenance is BUILD-scoped, not boot-scoped, so it deliberately +//! does NOT ride on `WsState` (whose contents are boot-scoped ids/state +//! injected by `freshell-server`). The full worktree-aware rationale for +//! the `rerun-if-changed` set lives in `crates/freshell-server/build.rs` — +//! this copy performs the SAME resolved-HEAD/ref/packed-refs watching so a +//! cached rebuild re-stamps when HEAD moves; both crates compile in the +//! same workspace build, so their baked commits agree. Never fails the +//! build over a missing/unavailable `git` (falls back to `"unknown"`). + +use std::path::PathBuf; +use std::process::Command; + +fn main() { + let commit = git_head_commit().unwrap_or_else(|| "unknown".to_string()); + println!("cargo:rustc-env=FRESHELL_WS_BUILD_COMMIT={commit}"); + for path in rerun_paths() { + println!("cargo:rerun-if-changed={}", path.display()); + } +} -In `build_handshake_with_capabilities`, in the `Ready` literal (lines 536-546), add after `server_instance_id`: +/// `git rev-parse HEAD`, trimmed. `None` on any failure (git not on `PATH`, +/// not inside a git checkout, ...) -- the caller falls back to `"unknown"`. +fn git_head_commit() -> Option { + let out = Command::new("git").args(["rev-parse", "HEAD"]).output().ok()?; + if !out.status.success() { + return None; + } + let s = String::from_utf8_lossy(&out.stdout).trim().to_string(); + if s.is_empty() { None } else { Some(s) } +} -```rust - build_id: Some(state.build_id.as_ref().clone()), +/// The exact paths that change when HEAD moves in THIS checkout, resolved +/// worktree-aware via `git rev-parse --git-path` (see the module doc and +/// `crates/freshell-server/build.rs`'s richer version for why each entry is +/// watched). Skipped resolutions degrade to cargo's default heuristics. +fn rerun_paths() -> Vec { + let mut paths = Vec::new(); + let git_path = |arg: &str| { + Command::new("git") + .args(["rev-parse", "--git-path", arg]) + .output() + .ok() + .filter(|o| o.status.success()) + .map(|o| PathBuf::from(String::from_utf8_lossy(&o.stdout).trim())) + .filter(|p| !p.as_os_str().is_empty()) + }; + if let Some(head) = git_path("HEAD") { + paths.push(head); + } + if let Some(head) = git_path("HEAD") { + if let Ok(contents) = std::fs::read_to_string(&head) { + if let Some(ref_name) = contents.strip_prefix("ref: ") { + if let Some(resolved) = git_path(ref_name.trim()) { + paths.push(resolved); + } + } + } + } + if let Some(packed) = git_path("packed-refs") { + if packed.exists() { + paths.push(packed); + } + } + paths +} ``` -3e. Fix every struct-literal site so the workspace compiles. In `crates/freshell-ws/src/lib.rs`'s `state()` test builder (after `boot_id: Arc::new("boot-2222".to_string()),` at line 878): +3e. `crates/freshell-ws/src/lib.rs` — add the read-back helper near the top of the crate (after the imports, before `WsState`), and stamp it in the handshake: ```rust - build_id: Arc::new("build-3333".to_string()), +/// The git commit THIS binary was built from, baked into this crate at +/// compile time by this crate's `build.rs` (`FRESHELL_WS_BUILD_COMMIT`). +/// Falls back to the literal `"unknown"` when git was unavailable at build +/// time (e.g. a source tarball or the Cloud Run image, which builds without +/// git metadata) -- never a runtime failure. Build provenance is +/// BUILD-scoped, so this deliberately does NOT ride on `WsState`. +pub fn ready_build_id() -> Option { + Some(option_env!("FRESHELL_WS_BUILD_COMMIT").unwrap_or("unknown").to_string()) +} ``` -In `crates/freshell-server/src/main.rs`'s `WsState` literal (after the `boot_id: Arc::clone(&boot_id),` line at 1031): +In `build_handshake_with_capabilities`, in the `Ready` literal (line 536), add after `server_instance_id`: ```rust - // The build identity every handshake `ready` stamps (client-side - // stale-bundle auto-reload). SAME source `GET /api/server-info`'s - // `commit` reports — one source of truth (`diag::build_commit()`). - build_id: Arc::new(crate::diag::build_commit().to_string()), + build_id: ready_build_id(), ``` -In `crates/freshell-protocol/tests/pane_reconcile.rs`, both `Ready` literals (lines 56-61 and 71-79) each get: +3f. Fix the remaining protocol-`Ready` literal sites (exactly two, both tests): in `crates/freshell-protocol/tests/pane_reconcile.rs`, both `freshell_protocol::Ready {` literals (lines 56 and 71) each get: ```rust build_id: None, ``` -Then enumerate any remaining literal sites: +Then enumerate any remaining sites: ```bash -cargo check --workspace --all-targets 2>&1 | rg "missing field" || echo "no missing-field errors" +cargo check --workspace --all-targets 2>&1 | rg "missing field \`build_id\`" || echo "no missing-field errors" ``` -Expected: `no missing-field errors` (if any site beyond the ones above is listed, add `build_id: None` — or, for `WsState` literals, a `build_id: Arc::new(...)` value — the same way; every site compiles before proceeding). +Expected: `no missing-field errors`. (Only the three sites above construct the protocol `Ready` today; the check catches any straggler — add `build_id: None` there the same way. Note `WsState` is deliberately untouched: ~36 files construct it and none changes.) + +3g. Create `scripts/bake-server-build-id.mjs`: -3f. Create `server/build-id.ts`: +```javascript +#!/usr/bin/env node +/** + * Bake the build-provenance stamp for the compiled Node server: writes + * `dist/server/build-id.json` = {"buildId": ""}. + * + * WHY a bake file: the running stamp must describe the BUILT ARTIFACT, not + * the checkout. `server/build-id.ts` prefers this file (resolved next to + * its compiled dist/server/build-id.js) and falls back to a runtime + * `git rev-parse HEAD` probe ONLY when no bake file exists next to it — + * which is exactly the tsx-from-source dev case, where the runtime probe + * is correct because dev runs current source. A stale `dist/server` + * started after HEAD moved therefore advertises the sha it was BUILT from, + * never a false "current" one. + * + * Runs after `tsc` in the `build:server` script. Atomic write (tmp+rename). + */ +import { execFileSync } from 'node:child_process' +import * as fs from 'node:fs' +import * as path from 'node:path' +import { fileURLToPath } from 'node:url' + +const repoRoot = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..') +const outPath = path.join(repoRoot, 'dist', 'server', 'build-id.json') + +function computeBuildId() { + try { + const sha = execFileSync('git', ['rev-parse', 'HEAD'], { + cwd: repoRoot, + stdio: ['ignore', 'pipe', 'ignore'], + }) + .toString() + .trim() + return /^[0-9a-f]{40}$/.test(sha) ? sha : 'unknown' + } catch { + return 'unknown' + } +} + +fs.mkdirSync(path.dirname(outPath), { recursive: true }) +const tmpPath = `${outPath}.tmp-${process.pid}` +fs.writeFileSync(tmpPath, `${JSON.stringify({ buildId: computeBuildId() })}\n`) +fs.renameSync(tmpPath, outPath) +console.log(`[bake-server-build-id] wrote ${outPath}`) +``` + +Update `package.json`'s `build:server` script: + +```json + "build:server": "tsc -p tsconfig.server.json && node scripts/bake-server-build-id.mjs", +``` + +3h. Create `server/build-id.ts`: ```typescript import { execFileSync } from 'node:child_process' +import { readFileSync } from 'node:fs' +import { fileURLToPath } from 'node:url' const SHA_PATTERN = /^[0-9a-f]{40}$/ +// Resolved relative to THIS module: next to the compiled +// dist/server/build-id.js in production (where `build:server`'s bake step +// wrote dist/server/build-id.json), or next to server/build-id.ts in +// tsx-from-source runs (where no bake file exists and the runtime probe is +// correct because dev runs current source). +const DEFAULT_BAKE_PATH = fileURLToPath(new URL('build-id.json', import.meta.url)) + /** - * The git commit this server process runs from — the SAME identity the Rust - * server bakes at compile time (`crates/freshell-server/src/diag.rs`'s - * `build_commit()`) and the client bakes at Vite build time + * The git commit the server runs from — the SAME identity the Rust server + * bakes at compile time (`crates/freshell-ws/build.rs`'s + * `FRESHELL_WS_BUILD_COMMIT`) and the client bakes at Vite build time * (`__FRESHELL_BUILD_ID__`). Falls back to the literal `"unknown"` when git * is unavailable or the output is not a full 40-hex sha; the client's * compare rule ignores `"unknown"` on both sides, so a git-less deployment @@ -287,16 +498,41 @@ export function computeBuildId(cwd: string = process.cwd()): string { } } +/** Read a bake file written by `scripts/bake-server-build-id.mjs`. */ +export function readBakedBuildId(bakePath: string): string | undefined { + try { + const raw = JSON.parse(readFileSync(bakePath, 'utf8')) as { buildId?: unknown } + return typeof raw.buildId === 'string' && raw.buildId.length > 0 ? raw.buildId : undefined + } catch { + return undefined + } +} + +/** + * BAKE-WINS-ELSE-PROBE: production (compiled dist/server) prefers the bake + * file written at build:server time, so the stamp describes the BUILT + * ARTIFACT — a stale dist started after HEAD moved advertises the sha it + * was built from, never a false "current" one. Source runs (tsx dev, vitest) + * find no bake file next to the source module and probe runtime HEAD. + */ +export function resolveServerBuildId(bakePath: string = DEFAULT_BAKE_PATH): string { + return readBakedBuildId(bakePath) ?? computeBuildId() +} + let cached: string | undefined -/** Per-process cached build id — one git probe per server lifetime. */ +/** Per-process cached build id — one resolution per server lifetime. */ export function serverBuildId(): string { - if (cached === undefined) cached = computeBuildId() + if (cached === undefined) cached = resolveServerBuildId() return cached } + +export function _resetServerBuildIdCacheForTests(): void { + cached = undefined +} ``` -3g. In `server/ws-handler.ts`: +3i. In `server/ws-handler.ts`: Add the import alongside the other relative imports at the top of the file: @@ -340,30 +576,41 @@ Expected: all PASS. - [ ] **Step 5: Refactor while green** -No refactor needed — every addition mirrors the adjacent `bootId` idiom on its side. Do NOT regenerate `port/oracle/fixtures/handshake-transcript.json`: the frozen transcript stays byte-valid because Rust omits `build_id` when deserialized as `None`, and the mutation/oracle suites consume the regenerated SCHEMA (not the live node bytes) for conformance. +No refactor needed — the Rust stamp mirrors the adjacent `boot_id` idiom, and the Node stamp mirrors `bootId`'s always-stamped treatment. Do NOT regenerate `port/oracle/fixtures/handshake-transcript.json`: the frozen transcript stays byte-valid because Rust omits `build_id` when deserialized as `None`, and the mutation/oracle suites consume the regenerated SCHEMA (not the live node bytes) for conformance. - [ ] **Step 6: Run impacted-test verification** -This change touches the shared wire protocol, both server implementations, and the generated schema, so the impacted set is: both Rust crates' full test trees, the workspace compile of every literal site, the whole server-config suite (any test asserting handshake/ready shapes), and the port-oracle suites. **`npm run test:port` does NOT run the oracle suites** (`vitest.port.config.ts` excludes `test/unit/port/oracle/**`; they are deliberately outside the coordinator and run only via `npm run test:oracle`, which boots real servers — budget several minutes). Note on `t0-equivalence-rust.test.ts`: its node-vs-rust deep diff compares `ready` frames value-by-value (`buildId` is NOT in the normalization registry, so it is compared RAW) — both sides stamp the SAME value (the worktree HEAD sha: the oracle node target runs from an isolated runtime root under the worktree so `git rev-parse HEAD` walk-up resolves the worktree sha; the rust target is `cargo build`-ed at test time by `ensureRustServerBuilt` and `build.rs` re-stamps on HEAD moves; Node computes the same sha at runtime), or both `"unknown"` in git-less environments, so the diff stays clean — and this run is the proof. +This change touches the shared wire protocol, both server implementations, the generated schema, and the `build:server` pipeline, so the impacted set is: both Rust crates' full test trees, the workspace compile, the whole server-config suite (any test asserting handshake/ready shapes), the port contract suites, and the port-ORACLE suites. **`npm run test:port` does NOT run the oracle suites** (`vitest.port.config.ts` excludes `test/unit/port/oracle/**`; they run only via `npm run test:oracle`, which boots real servers — budget several minutes). Notes: + +- `t0-equivalence-rust.test.ts` node-vs-rust deep diff compares `ready` frames value-by-value (`buildId` is NOT in the normalization registry, so it is compared RAW): both sides stamp the SAME value — the worktree HEAD sha (the oracle node target runs from an isolated runtime root under the worktree so `git rev-parse HEAD` walk-up resolves the worktree sha; the rust target is `cargo build`-ed at test time by `ensureRustServerBuilt` and both build scripts re-stamp on HEAD moves; Node's source-run probe resolves the same HEAD) — or both `"unknown"` in git-less environments. This run is the proof. +- `build:server` now emits `dist/server/build-id.json`; confirm with a real build: ```bash cargo test -p freshell-protocol cargo test -p freshell-ws cargo check --workspace --all-targets +npm run build:server +cat dist/server/build-id.json npm run test:integration npm run test:port npm run test:oracle ``` -Expected: all PASS. +Expected: all PASS, and `dist/server/build-id.json` contains the current worktree HEAD sha. - [ ] **Step 7: Commit the task** +Stage by directory so every compiler-enumerated fix lands in the commit (the worktree starts clean; verify nothing unexpected is staged): + ```bash -git add shared/ws-protocol.ts crates/freshell-protocol/src/server_messages.rs crates/freshell-protocol/tests/roundtrip.rs crates/freshell-protocol/tests/pane_reconcile.rs crates/freshell-ws/src/lib.rs crates/freshell-server/src/main.rs server/build-id.ts server/ws-handler.ts test/server/build-id.test.ts test/server/ws-handshake-snapshot.test.ts port/contract/ws-server-messages.schema.json -git commit -m "feat(protocol): both servers stamp additive optional ready.buildId (git HEAD)" +git status --short +git add shared/ port/contract/ws-server-messages.schema.json server/ scripts/bake-server-build-id.mjs test/server/ crates/ package.json +git status --short +git commit -m "feat(protocol): both servers stamp additive optional ready.buildId (artifact-time bake)" ``` +Expected: the first `git status --short` lists exactly the Task 1 files (all under the staged paths); the second shows the staged set; the commit compiles standalone (`cargo check --workspace --all-targets` from a clean checkout of it would pass — every `Ready` literal fix is inside `crates/`). + --- ### Task 2: Client compares on `ready` and reloads once (module + Vite define + App wiring) @@ -469,6 +716,19 @@ describe('checkServerBuildId', () => { expect(reload).not.toHaveBeenCalled() }) + it('does not throw or reload when the sessionStorage PROPERTY itself is inaccessible', () => { + const reload = vi.fn() + const original = Object.getOwnPropertyDescriptor(window, 'sessionStorage') + Object.defineProperty(window, 'sessionStorage', { value: undefined, configurable: true }) + try { + expect(() => checkServerBuildId({ clientBuildId: 'a'.repeat(40), serverBuildId: 'b'.repeat(40), reload })) + .not.toThrow() + expect(reload).not.toHaveBeenCalled() + } finally { + if (original) Object.defineProperty(window, 'sessionStorage', original) + } + }) + it('falls back to the __FRESHELL_BUILD_ID__ global and window defaults when options are omitted', () => { vi.stubGlobal('__FRESHELL_BUILD_ID__', 'c'.repeat(40)) const reload = vi.fn() @@ -502,7 +762,7 @@ describe('checkServerBuildId', () => { }) ``` -1b. In `test/unit/client/components/App.restart-signals.test.tsx`, append a new `describe` block at the end of the file. It reuses that file's existing harness plumbing (`createStore`, `renderApp`, `sendReady`, `wsMocks`, `messageHandler`, `stubAudio`, `terminalRestoreMocks`, `fetchSidebarSessionsSnapshot`, `getTerminalDirectoryPage`, `searchTerminalView`, `apiGet`, `defaultServerSettings`, `defaultSettings` — all defined at the top of that file; mirror the existing describe's beforeEach exactly): +1b. In `test/unit/client/components/App.restart-signals.test.tsx`, append a new `describe` block at the end of the file. It reuses that file's existing harness plumbing (`createStore`, `renderApp`, `sendReady`, `wsMocks`, `messageHandler`, `stubAudio`, `terminalRestoreMocks`, `fetchSidebarSessionsSnapshot`, `getTerminalDirectoryPage`, `searchTerminalView`, `apiGet`, `defaultServerSettings`, `defaultSettings` — all defined at the top of that file; mirror the existing describe's beforeEach exactly). Note the jsdom `sessionStorage` here is REAL and persists across the two simulated reboot cycles below — the unit-level proof that a code-armed sentinel survives the reload boundary: ```tsx describe('App ready buildId → one-shot server-build reload', () => { @@ -567,7 +827,7 @@ describe('App ready buildId → one-shot server-build reload', () => { sessionStorage.clear() }) - it('mismatched ready buildId triggers exactly one reload, and the sentinel suppresses the next mismatched ready', async () => { + it('mismatched ready buildId triggers exactly one reload, and the sentinel (real sessionStorage, persisting across the simulated reboot) suppresses the next mismatched ready', async () => { vi.stubGlobal('__FRESHELL_BUILD_ID__', 'a'.repeat(40)) const store = createStore() await renderApp(store) @@ -576,8 +836,9 @@ describe('App ready buildId → one-shot server-build reload', () => { expect(window.location.reload).toHaveBeenCalledTimes(1) expect(sessionStorage.getItem('freshell.server-build-reload')).toBe('1') - // Reconnect delivers another mismatched ready (stale server still up): - // the sentinel must suppress the reload. + // The reload lands: the page reboots in the SAME tab (real jsdom + // sessionStorage persists), the server is still stale, and the next + // ready must NOT reload again. sendReady({ serverInstanceId: 'srv-1', bootId: 'boot-1', buildId: 'b'.repeat(40) }) expect(window.location.reload).toHaveBeenCalledTimes(1) }) @@ -649,6 +910,19 @@ function resolveClientBuildId(): string | undefined { return id.length > 0 ? id : undefined } +/** + * sessionStorage can throw on PROPERTY ACCESS in hardened contexts (iframe + * sandboxing, privacy modes) — resolving it must be inside the fail-safe, + * never a ready-handler crash. + */ +function defaultStorage(): Pick | undefined { + try { + return window.sessionStorage + } catch { + return undefined + } +} + /** * Compare the server's `ready.buildId` against our own baked build id and * reload ONCE on a real mismatch. Invariants: @@ -656,13 +930,16 @@ function resolveClientBuildId(): string | undefined { * they differ ("unknown" == "unknown" is a no-op, never a match-and-clear); * - the sessionStorage sentinel is set BEFORE reloading and suppresses any * further reloads this tab session (a half-deployed server can never - * reload-loop; sessionStorage access failure = no reload, fail-safe); + * reload-loop; any sessionStorage failure = no reload, logged, fail-safe); * - a MATCHING ready clears the sentinel (self-re-arm after convergence). - * KNOWN LIMIT (accepted for the self-hosted single-server threat model): - * the "once" guarantee is per server identity — one origin fronted by - * servers built from DIFFERENT commits can oscillate (mismatch → reload → - * match clears → mismatch → …). Deliberately not hardened with a - * clears-per-session cap; revisit only if a split-deploy origin appears. + * KNOWN LIMITS (accepted for the self-hosted single-server threat model): + * - the "once" guarantee is per server identity — one origin fronted by + * servers built from DIFFERENT commits can oscillate (mismatch → reload → + * match clears → mismatch → …). Not hardened with a clears-per-session + * cap; revisit only if a split-deploy origin appears. + * - the compare is direction-free (shas carry no ordering), so a NEWER + * client against an OLDER server performs one futile bounded reload per + * fresh tab session. */ export function checkServerBuildId(options?: ServerBuildCheckOptions): void { const clientBuildId = options?.clientBuildId ?? resolveClientBuildId() @@ -671,17 +948,26 @@ export function checkServerBuildId(options?: ServerBuildCheckOptions): void { if (clientBuildId === 'unknown' || serverBuildId === 'unknown') return const reload = options?.reload ?? (() => window.location.reload()) - const storage = options?.storage ?? window.sessionStorage if (clientBuildId === serverBuildId) { + const storage = options?.storage ?? defaultStorage() try { - storage.removeItem(SERVER_BUILD_RELOAD_SENTINEL) + storage?.removeItem(SERVER_BUILD_RELOAD_SENTINEL) } catch { - // Ignore sessionStorage access failures. + // Ignore sessionStorage access failures (already disarmed-or-armed as + // found; nothing reloads on the match path either way). } return } + const storage = options?.storage ?? defaultStorage() + if (!storage) { + log.warn( + `server build ${serverBuildId} differs from client build ${clientBuildId} but ` + + 'sessionStorage is unavailable — suppressing the reload (fail-safe against loops)', + ) + return + } try { if (storage.getItem(SERVER_BUILD_RELOAD_SENTINEL) === '1') { log.warn( @@ -691,8 +977,8 @@ export function checkServerBuildId(options?: ServerBuildCheckOptions): void { return } storage.setItem(SERVER_BUILD_RELOAD_SENTINEL, '1') - } catch { - // Cannot persist the sentinel: reloading without it risks a loop. + } catch (err) { + log.warn('server-build sentinel persistence failed; suppressing the reload', err) return } log.warn( @@ -714,9 +1000,9 @@ Add the helper after `projectRoot` (line 10): ```typescript /** * The client's build identity: the git commit the bundle was built from, - * matching the server-side stamp (`crates/freshell-server/src/diag.rs`'s - * `build_commit()` / `server/build-id.ts`). `"unknown"` fallback — the - * client's compare rule ignores `"unknown"` on both sides. + * matching the server-side stamps (`crates/freshell-ws/build.rs` / + * `server/build-id.ts` + `scripts/bake-server-build-id.mjs`). `"unknown"` + * fallback — the client's compare rule ignores `"unknown"` on both sides. */ function computeClientBuildId(): string { try { @@ -817,16 +1103,17 @@ git commit -m "feat(client): reload once when ready.buildId differs from the bak --- -### Task 3: E2E proof (rust-chromium) + docs +### Task 3: E2E proof (rust-chromium, local lane) + docs **Files:** - Create: `test/e2e-browser/specs/server-build-mismatch-rust.spec.ts` - Modify: `test/e2e-browser/playwright.config.ts` (add the spec to `RUST_ONLY_SPECS`, whose `/create-protection-isolation-rust\.spec\.ts$/,` entry is at line 204; add the spec to the `rust-chromium` project's `testMatch`, whose `/codex-terminal-bounce-rust\.spec\.ts$/,` entry is at line 371) +- Modify: `test/e2e-browser/playwright.cloud.config.ts` (add the spec to `CLOUD_SKIP_SPECS` with justification) - Modify: `AGENTS.md` (one-line note under "Key Architectural Patterns → WebSocket Protocol") **Interfaces:** - Consumes: Tasks 1-2 (both servers stamp `ready.buildId`; the client compares and reloads once; `TestHarness.receiveWsMessage` → `ws.receiveMessageForTest` → `handleIncomingMessage` feeds an injected frame through the real App ready handler — verified at `src/lib/ws-client.ts:917-919`). -- Produces: the user-outcome proof — a stale client against a newer server reboots itself exactly once and converges to a healthy ready connection; repeat mismatches are suppressed by the sentinel. +- Produces: the user-outcome proof on the LOCAL lane — a stale client against a newer server reboots itself exactly once and converges to a healthy ready connection; sessionStorage persistence across a REAL navigation; repeat mismatches suppressed by the sentinel. - [ ] **Step 1: Write the failing behavioral test** @@ -843,19 +1130,31 @@ Create `test/e2e-browser/specs/server-build-mismatch-rust.spec.ts`: * converges to a healthy ready connection. A repeat mismatched ready must * NOT reload again — a half-deployed server can never reload-loop. * - * Mismatch is injected with `harness.receiveWsMessage` (a REAL server + * COVERAGE BOUNDARY (read before judging): what e2e proves here is + * (1) the full production compare-and-reload pipeline through the REAL App + * ready handler (mismatch injected via the test harness — a REAL server * stamps its own sha, which may or may not equal this worktree's client - * bake — the injection makes the compare deterministic either way). The - * injected frame flows through the production pipeline: ws-client - * `receiveMessageForTest` → `handleIncomingMessage` → App's ready handler - * → `ReadyMessageSchema` → `checkServerBuildId`. - * - * Service workers are blocked (perf-harness precedent, - * recover-my-panes-rust.spec.ts's FRESH_CONTEXT_OPTIONS) so the count of - * navigations is exactly the reloads this feature performs. + * bake, so the injection makes the compare deterministic either way), + * (2) sessionStorage persistence across a REAL navigation, and (3) + * suppression of a repeat mismatch. The "code armed the sentinel BEFORE + * reloading" ORDER is proven by the unit suite (App.restart-signals: real + * jsdom sessionStorage persisting across the simulated reboot). Observing + * the code-armed sentinel surviving a REAL navigation e2e is not + * deterministic here: after any reload the boot's REAL ready either matches + * (same-HEAD artifacts → legitimately clears the sentinel) or mismatches + * (stale-bake environments → keeps it), so the post-reload sentinel state + * is environment-dependent — hence the persistence test reads at commit + * time and the suppression test seeds its state AFTER the boot settles. + * Seeding is state setup, the same practice as seeding localStorage in + * other suites; the PERSISTENCE and SUPPRESSION behavior exercised is + * entirely production code. * * Rust-only: registers under `rust-chromium` + RUST_ONLY_SPECS (owns a - * RustServer directly, the e2eServerKind seam not used). + * RustServer directly, the e2eServerKind seam not used). CLOUD-SKIPPED with + * justification (see playwright.cloud.config.ts): the Cloud Run image + * builds WITHOUT git metadata, so both the Rust bake and the Vite define + * are "unknown" there and the compare is inert BY DESIGN — this spec can + * only pass on a lane where at least the client bake is a real sha. */ import { test, expect } from '../helpers/fixtures.js' import { RustServer, ensureRustServerBuilt } from '../helpers/rust-server.js' @@ -880,11 +1179,11 @@ test.describe('server build mismatch reload (rust)', () => { await server?.stop().catch(() => {}) }) - test('mismatched ready buildId reloads exactly once and converges; the sentinel suppresses repeats', async ({ browser }) => { + test('mismatched ready buildId reloads exactly once and converges', async ({ browser }) => { const context = await browser.newContext({ serviceWorkers: 'block' }) const page = await context.newPage() await page.goto(`${info.baseUrl}/?token=${info.token}&e2e=1`) - let harness = new TestHarness(page) + const harness = new TestHarness(page) await harness.waitForHarness() await harness.waitForConnection() @@ -896,8 +1195,8 @@ test.describe('server build mismatch reload (rust)', () => { let navigations = 0 page.on('framenavigated', () => { navigations++ }) - // 1) Injected mismatch → exactly one reload, and the page reboots into a - // healthy ready connection (convergence). + // Injected mismatch → exactly one reload, and the page reboots into a + // healthy ready connection (convergence). await harness.receiveWsMessage({ type: 'ready', timestamp: new Date().toISOString(), @@ -906,15 +1205,51 @@ test.describe('server build mismatch reload (rust)', () => { buildId: MISMATCHED_BUILD_ID, }) await expect.poll(() => navigations, { timeout: 20_000 }).toBe(1) - harness = new TestHarness(page) + const rebooted = new TestHarness(page) + await rebooted.waitForHarness() + await rebooted.waitForConnection() + + await context.close() + }) + + test('sentinel persists across a real navigation', async ({ browser }) => { + const context = await browser.newContext({ serviceWorkers: 'block' }) + const page = await context.newPage() + await page.goto(`${info.baseUrl}/?token=${info.token}&e2e=1`) + const harness = new TestHarness(page) + await harness.waitForHarness() + await harness.waitForConnection() + + // Seed the state the production code would have armed on a previous + // mismatched ready in this tab (see the coverage boundary above). + await page.evaluate((key) => sessionStorage.setItem(key, '1'), SENTINEL) + + // A REAL navigation: sessionStorage must survive it (per-tab, per-origin + // storage) — read at commit time, BEFORE the rebooted app's real ready + // can legitimately match-and-clear it (same-HEAD artifacts match). + await page.reload({ waitUntil: 'commit' }) + const persisted = await page.evaluate((key) => sessionStorage.getItem(key), SENTINEL) + expect(persisted, 'sentinel must survive a real navigation').toBe('1') + + await context.close() + }) + + test('a seeded sentinel suppresses a repeat mismatch (no reload)', async ({ browser }) => { + const context = await browser.newContext({ serviceWorkers: 'block' }) + const page = await context.newPage() + await page.goto(`${info.baseUrl}/?token=${info.token}&e2e=1`) + const harness = new TestHarness(page) await harness.waitForHarness() await harness.waitForConnection() - // 2) Re-arm explicitly, then inject the SAME mismatch again: the - // sentinel must suppress the reload (no loop). The explicit re-arm - // keeps this assertion deterministic regardless of whether the real - // server's ready matched the client bake (which would self-clear). + // Seed AFTER the boot settles (the boot's real ready may legitimately + // match-and-clear an earlier sentinel; seeding here is the setup for + // the suppression proof — the arming ORDER is unit-proven, the + // navigation persistence is proven by the previous test). await page.evaluate((key) => sessionStorage.setItem(key, '1'), SENTINEL) + let navigations = 0 + page.on('framenavigated', () => { navigations++ }) + await harness.receiveWsMessage({ type: 'ready', timestamp: new Date().toISOString(), @@ -923,7 +1258,7 @@ test.describe('server build mismatch reload (rust)', () => { buildId: MISMATCHED_BUILD_ID, }) await page.waitForTimeout(3_000) - expect(navigations, 'sentinel must suppress the second mismatched ready').toBe(1) + expect(navigations, 'persisted sentinel must suppress the repeat mismatch').toBe(0) await context.close() }) @@ -949,10 +1284,21 @@ In the `rust-chromium` project's `testMatch` array, after the `/codex-terminal-b /server-build-mismatch-rust\.spec\.ts$/, ``` +In `CLOUD_SKIP_SPECS` (the cloud config's skip list in `playwright.cloud.config.ts`), add with justification: + +```typescript + // Server-build mismatch reload: the Cloud Run image builds WITHOUT git + // metadata (.dockerignore drops .git), so the Rust bake and the Vite + // define are both "unknown" there and the client's compare is inert BY + // DESIGN — a mismatched ready can never trigger a reload on that lane. + // Coverage lives on the local rust-chromium project. + /server-build-mismatch-rust\.spec\.ts$/, +``` + In `AGENTS.md`, under "Key Architectural Patterns", append to the **WebSocket Protocol** paragraph: ``` -The `ready` frame carries an optional additive `buildId` (the server's baked git commit, `"unknown"` fallback): the client bakes its own at Vite build time (`__FRESHELL_BUILD_ID__`) and, on a mismatch, reloads exactly once per tab session (sessionStorage sentinel `freshell.server-build-reload`), self-healing stale-client contract errors; `"unknown"` on either side never triggers or clears the guard (`src/lib/server-build-check.ts`). The once-guard is per server identity: an origin fronted by mixed-build servers could oscillate (accepted for the single-server self-hosted model). +The `ready` frame carries an optional additive `buildId` (the server's artifact-time-baked git commit, `"unknown"` fallback): the client bakes its own at Vite build time (`__FRESHELL_BUILD_ID__`) and, on a mismatch, reloads exactly once per tab session (sessionStorage sentinel `freshell.server-build-reload`), self-healing stale-client contract errors; `"unknown"` on either side never triggers or clears the guard (`src/lib/server-build-check.ts`). The once-guard is per server identity: an origin fronted by mixed-build servers could oscillate, and a newer client against an older server costs one futile bounded reload per fresh tab session (both accepted for the single-server self-hosted model). ``` - [ ] **Step 2: Run the test and verify it passes, then RED-VERIFY it exercises the feature** @@ -978,7 +1324,7 @@ npm run build:client npx playwright test --config test/e2e-browser/playwright.config.ts --project=rust-chromium test/e2e-browser/specs/server-build-mismatch-rust.spec.ts ``` -Expected: FAIL — `expect.poll` times out with `navigations` stuck at 0 (no reload happens without the compare). +Expected: FAIL — test 1's `expect.poll` times out with `navigations` stuck at 0 (no reload happens without the compare). Tests 2 and 3 still pass with the compare disabled (their sentinels are seeded state), which is exactly why the unit suite owns the arming-order proof — record all three observations. Restore the call and rebuild: @@ -999,7 +1345,7 @@ Same command as Step 2's final run. Expected: PASS. - [ ] **Step 5: Refactor while green** -No refactor needed. Confirm the spec is excluded from the match-all `chromium` project by the `RUST_ONLY_SPECS` entry (`testIgnore: RUST_ONLY_SPECS` at `playwright.config.ts:330`) and runs ONLY under `rust-chromium`. Note the spec also runs on the CLOUD e2e lane when `FRESHELL_E2E_BACKEND=cloud` (`playwright.cloud.config.ts` filters only firefox/webkit/continuity-smoke, so `rust-chromium` survives; the spec is not in `CLOUD_SKIP_SPECS`) — do not add it there; coverage comes from Step 6's backend run. +No refactor needed. Confirm the registration mechanics: excluded from the match-all `chromium` project by the `RUST_ONLY_SPECS` entry (`testIgnore: RUST_ONLY_SPECS` at `playwright.config.ts:330`), included in `rust-chromium`'s `testMatch`, and skipped on the cloud lane by the `CLOUD_SKIP_SPECS` entry with its justification comment. - [ ] **Step 6: Run impacted-test verification** @@ -1010,14 +1356,12 @@ npx playwright test --config test/e2e-browser/playwright.config.ts --project=rus npm run test:vitest -- run test/unit/client/lib/server-build-check.test.ts test/unit/client/components/App.restart-signals.test.tsx ``` -Expected: all PASS. - -**Backend proof (repo rule: an affected e2e spec must pass on the configured `FRESHELL_E2E_BACKEND` before a PR is filed):** before the branch is PR'd, run this spec on the configured backend — if `FRESHELL_E2E_BACKEND=cloud`, `npm run test:e2e:cloud` filtered to this spec (this also proves `cargo` availability and the build stamp inside the cloud image); if unset/local, the local runs above satisfy the rule. This is a pre-PR gate recorded in the run log, not part of the task commit. +Expected: all PASS. Backend note: the repo rule about the configured `FRESHELL_E2E_BACKEND` is honored at execution kickoff (the user chooses local vs cloud once; the answer is recorded in `run-state.md`). This spec's coverage lane is the LOCAL rust-chromium project; it is CLOUD_SKIP'd with justification (no git metadata in the cloud build → both stamps "unknown" → the compare is inert there), and no cloud claim is made about cargo (the cloud runtime uses a prebuilt binary; cargo never runs there per `rust-server.ts:82-90`). - [ ] **Step 7: Commit the task** ```bash -git add test/e2e-browser/specs/server-build-mismatch-rust.spec.ts test/e2e-browser/playwright.config.ts AGENTS.md +git add test/e2e-browser/specs/server-build-mismatch-rust.spec.ts test/e2e-browser/playwright.config.ts test/e2e-browser/playwright.cloud.config.ts AGENTS.md git commit -m "test(e2e): rust spec proves one-shot sentinel-guarded reload on ready.buildId mismatch" ``` @@ -1032,17 +1376,18 @@ npm run check npm run test:oracle ``` -Expected: typecheck + full default + server suites PASS, and the oracle suites (t0-equivalence, handshake-determinism, external-handshake, mutation-validation) PASS — `npm run test:oracle` boots real servers and cargo-builds the workspace, so budget several minutes. Also confirm the Task 3 backend proof was recorded (the spec passing on the configured `FRESHELL_E2E_BACKEND`). +Expected: typecheck + full default + server suites PASS, and the oracle suites (t0-equivalence, handshake-determinism, external-handshake, mutation-validation) PASS — `npm run test:oracle` boots real servers and cargo-builds the workspace, so budget several minutes. Also confirm the Task 3 e2e runs were recorded on the user-chosen backend (this spec's lane is local rust-chromium; cloud is skipped with justification). **User-outcome recap (maps every requirement to its proof):** | Requirement | Production behavior | Proof | | --- | --- | --- | -| Server stamps build identity in `ready` | Rust `WsState.build_id` → `Ready.build_id` (`Some`, sha/`"unknown"`); Node `serverBuildId()` → `buildId`; schema regenerated | roundtrip + wire tests; `test:port`; Node snapshot test | -| Identity = git HEAD, `"unknown"` fallback, everywhere | `build.rs` (existing, HEAD-move aware), `server/build-id.ts`, `computeClientBuildId()` | `build-id.test.ts`; bundle-bake check (Task 2 Step 5) | +| Server stamps build identity in `ready` | Rust `freshell-ws/build.rs` bake → `Ready.build_id` (`Some`, sha/`"unknown"`); Node bake-file-or-probe → `buildId`; schema regenerated | roundtrip + wire tests; `test:port` + `test:oracle`; Node snapshot test | +| Identity = artifact-time git HEAD, `"unknown"` fallback, everywhere | `crates/freshell-ws/build.rs`; `dist/server/build-id.json` written by `build:server` (stale dist advertises its own build); `computeClientBuildId()` | `build-id.test.ts` (bake precedence + probe fallback); bundle-bake check (Task 2 Step 5); `cat dist/server/build-id.json` (Task 1 Step 6) | | Client compares on every `ready` | `ReadyMessageSchema.buildId` → `checkServerBuildId` in App's ready handler | App.restart-signals describe block | -| Mismatch → reload exactly once | sentinel set before `reload()`; armed sentinel suppresses | unit matrix; e2e navigation count === 1 | -| Never reload-loops (incl. storage failure, repeated mismatches) | fail-safe catch; suppression branch; `"unknown"` no-op | unit cases; e2e repeat-injection step | +| Mismatch → reload exactly once | sentinel set before `reload()`; armed sentinel suppresses | unit matrix (real jsdom sessionStorage across the simulated reboot); e2e navigation count === 1 | +| Sentinel survives a REAL navigation; repeat mismatches suppressed | sessionStorage per-tab persistence; suppression branch | e2e test 2 (commit-time persistence read + suppression); unit matrix | +| Never reload-loops (incl. storage failure, repeated mismatches) | fail-safe catch + property-access guard + logged suppression; `"unknown"` no-op | unit cases (throwing storage, undefined sessionStorage, unknown-vs-unknown); e2e repeat-injection step | | Match clears the sentinel (self-re-arm) | removeItem on equal ids | unit cases; App re-arm test | | Old servers/forks unaffected (additive contract) | optional field, omitted when `None`; schema stays `additionalProperties: false` | frozen transcript roundtrip; contract-freeze + mutation suites | -| Real-world convergence | reloaded page reconnects and reaches ready | e2e `waitForConnection` after reload | +| Real-world convergence | reloaded page reconnects and reaches ready | e2e test 1 `waitForConnection` after reload | From 988774bd41560851e53bb87afe19b578026b61d2 Mon Sep 17 00:00:00 2001 From: Dan Shapiro <3732858+danshapiro@users.noreply.github.com> Date: Thu, 27 Aug 2026 11:49:52 -0700 Subject: [PATCH 04/15] docs: fix fresh-eyes round 2 findings in server-version-reload plan --- .../plans/2026-08-27-server-version-reload.md | 32 +++++++++++-------- 1 file changed, 19 insertions(+), 13 deletions(-) diff --git a/docs/plans/2026-08-27-server-version-reload.md b/docs/plans/2026-08-27-server-version-reload.md index cf77c659e..0777ffaac 100644 --- a/docs/plans/2026-08-27-server-version-reload.md +++ b/docs/plans/2026-08-27-server-version-reload.md @@ -84,7 +84,7 @@ fn ready_carries_build_id_and_omits_it_when_absent() { } ``` -1b. Add to `crates/freshell-ws/src/lib.rs` inside `mod tests`, immediately after `handshake_is_ordered_with_shared_bootid` (line 1026): +1b. Add to `crates/freshell-ws/src/lib.rs` inside `mod tests`, immediately after `handshake_is_ordered_with_shared_bootid` (line 1026). Deliberately references NO new symbols, so its RED phase compiles and fails on the assertion: ```rust /// The handshake `ready` stamps the build identity baked into THIS crate @@ -96,9 +96,12 @@ fn ready_carries_build_id_and_omits_it_when_absent() { async fn handshake_ready_stamps_build_id() { let msgs = build_handshake(&state()).await; let ready = serde_json::to_value(&msgs[0]).unwrap(); - let baked = ready_build_id().expect("crate always bakes a build id"); - assert!(!baked.is_empty()); - assert_eq!(ready["buildId"], serde_json::json!(baked)); + assert!( + ready.get("buildId").is_some(), + "ready must stamp buildId: {ready}" + ); + let build_id = ready["buildId"].as_str().expect("buildId is a string"); + assert!(!build_id.is_empty(), "buildId must be non-empty: {build_id}"); } ``` @@ -255,7 +258,7 @@ cargo test -p freshell-ws handshake_ready_stamps_build_id npm run test:vitest -- run test/server/build-id.test.ts test/server/ws-handshake-snapshot.test.ts --config config/vitest/vitest.server.config.ts ``` -Expected: all FAIL for the missing behavior — the Rust roundtrip test fails to COMPILE (`no field \`build_id\` on struct Ready`); the freshell-ws wire test COMPILES (it references no new field) and fails its JSON assertion (`ready["buildId"]` is JSON null ≠ the baked string — the ready frame carries no `buildId`); `build-id.test.ts` fails to resolve `../../server/build-id.js` (module missing); the new snapshot test fails on `expect(typeof ready1.buildId).toBe('string')`. +Expected: all FAIL for the missing behavior — the Rust roundtrip test fails to COMPILE (`no field \`build_id\` on struct Ready`); the freshell-ws wire test COMPILES (it references no new symbols) and fails its first assertion (`ready must stamp buildId` — the ready frame carries no `buildId`); `build-id.test.ts` fails to resolve `../../server/build-id.js` (module missing); the new snapshot test fails on `expect(typeof ready1.buildId).toBe('string')`. - [ ] **Step 3: Add the minimal production implementation** @@ -1047,9 +1050,12 @@ Extend `ReadyMessageSchema` (lines 157-166), after the `bootId` line: ```typescript bootId: z.string().min(1).optional(), // The server's baked build identity (additive/optional — old servers omit - // it). Compared in checkServerBuildId below; must never fail the WHOLE - // ready frame, hence optional + min(1) only. - buildId: z.string().min(1).optional(), + // it). Compared in checkServerBuildId below. Plain `z.string()` (NOT + // min(1)): a present-but-EMPTY buildId must reach the helper and no-op + // there, never fail the WHOLE ready frame and silently disable restart + // detection. Only a non-string TYPE can fail the frame, which no real + // server emits (the helper additionally treats "unknown" as a no-op). + buildId: z.string().optional(), ``` Add the call inside the `else` (ready-success) branch, immediately after the `if (!newBootId) { ... }` warn block that ends at line 1031: @@ -1073,14 +1079,14 @@ Expected: PASS. - [ ] **Step 5: Refactor while green** -Verify the Vite define actually bakes the sha into the bundle: +Verify the Vite define actually bakes the sha into the bundle (explicit pass/fail so automation cannot swallow a failed match through a pipe): ```bash npm run build:client -rg -o "$(git rev-parse HEAD)" dist/client/assets/*.js | head -1 +rg -q "$(git rev-parse HEAD)" dist/client/assets/*.js && echo "BAKE OK: sha present in bundle" || echo "BAKE MISSING: sha absent from bundle" ``` -Expected: at least one match (the baked sha appears in the built bundle). (`npm run build:client` from this worktree writes the worktree's own `dist/client` — the main-checkout `npm run build` production-server guard does not apply here.) +Expected: `BAKE OK: sha present in bundle` (the baked sha appears in the built bundle). (`npm run build:client` from this worktree writes the worktree's own `dist/client` — the main-checkout `npm run build` production-server guard does not apply here.) - [ ] **Step 6: Run impacted-test verification** @@ -1284,7 +1290,7 @@ In the `rust-chromium` project's `testMatch` array, after the `/codex-terminal-b /server-build-mismatch-rust\.spec\.ts$/, ``` -In `CLOUD_SKIP_SPECS` (the cloud config's skip list in `playwright.cloud.config.ts`), add with justification: +In `CLOUD_SKIP_SPECS` (the filename-string skip list in `playwright.cloud.config.ts` — entries are converted to `**/${s}` globs, so this MUST be a plain filename string, not a regex), add with justification: ```typescript // Server-build mismatch reload: the Cloud Run image builds WITHOUT git @@ -1292,7 +1298,7 @@ In `CLOUD_SKIP_SPECS` (the cloud config's skip list in `playwright.cloud.config. // define are both "unknown" there and the client's compare is inert BY // DESIGN — a mismatched ready can never trigger a reload on that lane. // Coverage lives on the local rust-chromium project. - /server-build-mismatch-rust\.spec\.ts$/, + 'server-build-mismatch-rust.spec.ts', ``` In `AGENTS.md`, under "Key Architectural Patterns", append to the **WebSocket Protocol** paragraph: From 69afc80d1c6c049297133a4c088c6bd28530ea20 Mon Sep 17 00:00:00 2001 From: Dan Shapiro <3732858+danshapiro@users.noreply.github.com> Date: Thu, 27 Aug 2026 12:13:12 -0700 Subject: [PATCH 05/15] docs: rework server-version-reload plan from fresh-eyes round 3 --- .../plans/2026-08-27-server-version-reload.md | 165 ++++++++++++++---- 1 file changed, 134 insertions(+), 31 deletions(-) diff --git a/docs/plans/2026-08-27-server-version-reload.md b/docs/plans/2026-08-27-server-version-reload.md index 0777ffaac..3e9fe59b3 100644 --- a/docs/plans/2026-08-27-server-version-reload.md +++ b/docs/plans/2026-08-27-server-version-reload.md @@ -22,7 +22,7 @@ - **Client module must not crash under Vitest:** the Vitest client config has no `__FRESHELL_BUILD_ID__` define, so the module must use a `typeof __FRESHELL_BUILD_ID__ === 'undefined'` guard (same precedent as `src/lib/perf-logger.ts:45` with `__PERF_LOGGING__`). - **NodeNext/ESM:** every relative import in `server/` and `shared/` uses `.js` extensions; client code uses `@/` aliases without extensions. - **Test coordination:** broad suites go through the repo coordinator (`npm run test:vitest -- run ...`); never raw `npx vitest`. Focused Rust tests use `cargo test -p ` directly. The port-ORACLE suites are NOT covered by `npm run test:port` / `npm run check` — they run only via `npm run test:oracle`. -- **E2E backend rule:** per repo instructions, when `FRESHELL_E2E_BACKEND` is unset the user chooses local vs cloud before e2e runs — surface that question once at execution kickoff and record the answer in `run-state.md`. This feature's e2e coverage lane is the LOCAL `rust-chromium` project regardless: the new spec is added to `CLOUD_SKIP_SPECS` with a technical justification (the cloud image builds without git metadata, so both stamps are `"unknown"` and the compare is inert there — see Task 3). Never claim a cloud run proves cargo availability: the cloud runtime uses a prebuilt binary and cargo never runs there (`test/e2e-browser/helpers/rust-server.ts:82-90`). +- **E2E backend rule:** per repo instructions, when `FRESHELL_E2E_BACKEND` is unset the user chooses local vs cloud before e2e runs — surface that question once at execution kickoff, INFORMED that the new spec is cloud-incompatible by construction (the cloud image builds without git metadata, so both stamps are `"unknown"` and the compare is inert there), and record the answer in `run-state.md`. This feature's e2e coverage lane is the LOCAL `rust-chromium` project regardless of the choice; the spec is added to `CLOUD_SKIP_SPECS` with that justification, and if cloud is chosen the PR description documents the skip explicitly so no coverage claim is silent. Never claim a cloud run proves cargo availability: the cloud runtime uses a prebuilt binary and cargo never runs there (`test/e2e-browser/helpers/rust-server.ts:82-90`). - **Scope boundary:** client-only redeploys (redeploying a new client bundle WITHOUT a server change) are deliberately NOT covered by any auto-trigger — no polling, no `/api/server-info` fallback, no reload loop. The ready-frame compare is the only trigger; a client-only redeploy costs at most one bounded reload per fresh tab session. - **No unrelated restructuring; comments explain invariants, in the existing voice.** @@ -40,7 +40,8 @@ - Create: `scripts/bake-server-build-id.mjs` - Create: `server/build-id.ts` - Modify: `server/ws-handler.ts` (import block; field after `:587`; init after `:651`; ready send `:2034-2039`) -- Modify (generated): `port/contract/ws-server-messages.schema.json` (via `npm run contract:generate`) +- Modify: `port/contract/ws-server-messages.schema.json` (via `npm run contract:generate`) +- Modify: `port/oracle/harness/external-server.ts` (`ensureServerBuilt` stamp-freshness guard) - Test: `crates/freshell-protocol/tests/roundtrip.rs` (new test after `ready_carries_server_instance_id_and_boot_id`, which ends at line 164) - Test: `test/server/build-id.test.ts` (new) - Test: `test/server/ws-handshake-snapshot.test.ts` (new test after the `includes a bootId in the ready message...` test, which ends at line 301) @@ -181,6 +182,10 @@ describe('server build id', () => { expect(readBakedBuildId(bakePath)).toBeUndefined() fs.writeFileSync(bakePath, JSON.stringify({ buildId: '' })) expect(readBakedBuildId(bakePath)).toBeUndefined() + // Same validation as the writer: only a 40-hex sha or "unknown" is a + // legitimate stamp; a garbage string must never become authoritative. + fs.writeFileSync(bakePath, JSON.stringify({ buildId: 'garbage-stamp' })) + expect(readBakedBuildId(bakePath)).toBeUndefined() expect(readBakedBuildId(path.join(dir, 'absent.json'))).toBeUndefined() } finally { fs.rmSync(dir, { recursive: true, force: true }) @@ -196,7 +201,7 @@ describe('server build id', () => { } }) - it('resolveServerBuildId falls back to the runtime git probe when no bake file exists', () => { + it('resolveServerBuildId falls back to the runtime git probe when no bake file exists in SOURCE mode', () => { const { dir } = tempBakeFile(null) try { expect(resolveServerBuildId(path.join(dir, 'build-id.json'))).toBe(computeBuildId(REPO_ROOT)) @@ -205,6 +210,22 @@ describe('server build id', () => { } }) + it('resolveServerBuildId fails inert to "unknown" for a compiled artifact without a valid stamp', () => { + const { dir, bakePath } = tempBakeFile(null) + try { + // A compiled artifact (sourceMode: false) must NEVER probe the + // checkout: a stale dist without its stamp advertises "unknown", not + // the current HEAD (which would falsely match a current client). + expect(resolveServerBuildId(path.join(dir, 'build-id.json'), { sourceMode: false })).toBe('unknown') + fs.writeFileSync(bakePath, 'corrupt {') + expect(resolveServerBuildId(bakePath, { sourceMode: false })).toBe('unknown') + fs.writeFileSync(bakePath, JSON.stringify({ buildId: 'garbage-stamp' })) + expect(resolveServerBuildId(bakePath, { sourceMode: false })).toBe('unknown') + } finally { + fs.rmSync(dir, { recursive: true, force: true }) + } + }) + it('serverBuildId memoizes: the git probe runs once per process', () => { _resetServerBuildIdCacheForTests() // Source runs (tsx/vitest) have no bake file next to server/build-id.ts, @@ -505,21 +526,39 @@ export function computeBuildId(cwd: string = process.cwd()): string { export function readBakedBuildId(bakePath: string): string | undefined { try { const raw = JSON.parse(readFileSync(bakePath, 'utf8')) as { buildId?: unknown } - return typeof raw.buildId === 'string' && raw.buildId.length > 0 ? raw.buildId : undefined + const value = raw.buildId + if (typeof value !== 'string') return undefined + // Same validation as the writer and the git probes: a 40-hex sha or the + // literal "unknown". Anything else is a malformed stamp — treat as + // absent, never authoritative (a garbage stamp would cause a needless + // mismatch reload). + return value === 'unknown' || SHA_PATTERN.test(value) ? value : undefined } catch { return undefined } } +// Source runs (tsx dev, vitest) execute THIS .ts module; a compiled +// production artifact executes dist/server/build-id.js. The distinction +// decides what a MISSING bake file means (see resolveServerBuildId). +const SOURCE_MODE = import.meta.url.endsWith('.ts') + /** - * BAKE-WINS-ELSE-PROBE: production (compiled dist/server) prefers the bake - * file written at build:server time, so the stamp describes the BUILT - * ARTIFACT — a stale dist started after HEAD moved advertises the sha it - * was built from, never a false "current" one. Source runs (tsx dev, vitest) - * find no bake file next to the source module and probe runtime HEAD. + * BAKE-WINS-ELSE-FAIL-INERT: a compiled production artifact describes + * itself ONLY by its bake file — a stale dist started after HEAD moved + * advertises the sha it was built from (never a false "current" one), and + * an artifact whose stamp is missing or malformed fails inert to + * "unknown" (it must never impersonate the checkout). Source runs have no + * bake file next to the source module and probe runtime HEAD instead, + * which is correct because they execute current source. */ -export function resolveServerBuildId(bakePath: string = DEFAULT_BAKE_PATH): string { - return readBakedBuildId(bakePath) ?? computeBuildId() +export function resolveServerBuildId( + bakePath: string = DEFAULT_BAKE_PATH, + opts?: { sourceMode?: boolean }, +): string { + const sourceMode = opts?.sourceMode ?? SOURCE_MODE + if (sourceMode) return computeBuildId() + return readBakedBuildId(bakePath) ?? 'unknown' } let cached: string | undefined @@ -536,7 +575,6 @@ export function _resetServerBuildIdCacheForTests(): void { ``` 3i. In `server/ws-handler.ts`: - Add the import alongside the other relative imports at the top of the file: ```typescript @@ -567,6 +605,47 @@ Extend the ready send (lines 2034-2039): }) ``` +3j. In `port/oracle/harness/external-server.ts` — the oracle's node target runs the COMPILED `dist/server/index.js`, and `ensureServerBuilt` rebuilds only when the entry is ABSENT. After this feature, a stale pre-existing `dist` carries a stale bake file, and `npm run test:oracle` would compare a stale Node `buildId` against the fresh cargo-built Rust value — a false implementation divergence. Add a stamp-freshness check so a stale node dist rebuilds (keep the legacy behavior when no bake file exists, so git-less/pre-stamp dists are unaffected): + +```typescript +/** + * Whether the node dist's baked build stamp (written by `build:server`'s + * `scripts/bake-server-build-id.mjs`) matches the CURRENT checkout HEAD. + * True when no bake file exists (pre-stamp dist or git-less build — keep + * the legacy exists-only behavior), when git is unavailable, or when the + * stamp is unreadable: those cases have no stamp semantics to violate. + * False only for a REAL staleness — a bake from an earlier HEAD — which + * must trigger a rebuild so the oracle's node-vs-rust `buildId` comparison + * compares same-HEAD artifacts, never a stale checkout against a fresh + * cargo build. + */ +function nodeBuildStampIsCurrent(root: string): boolean { + const bakePath = path.join(root, 'dist', 'server', 'build-id.json') + if (!fs.existsSync(bakePath)) return true + try { + const baked = (JSON.parse(fs.readFileSync(bakePath, 'utf8')) as { buildId?: unknown }).buildId + if (typeof baked !== 'string' || baked === 'unknown') return true + const head = spawnSync('git', ['rev-parse', 'HEAD'], { cwd: root, encoding: 'utf8' }) + if (head.status !== 0) return true + return baked === head.stdout.trim() + } catch { + return true + } +} +``` + +and change `ensureServerBuilt`'s first guard from + +```typescript + if (fs.existsSync(entry)) return entry +``` + +to + +```typescript + if (fs.existsSync(entry) && nodeBuildStampIsCurrent(root)) return entry +``` + - [ ] **Step 4: Run the focused tests** ```bash @@ -585,7 +664,7 @@ No refactor needed — the Rust stamp mirrors the adjacent `boot_id` idiom, and This change touches the shared wire protocol, both server implementations, the generated schema, and the `build:server` pipeline, so the impacted set is: both Rust crates' full test trees, the workspace compile, the whole server-config suite (any test asserting handshake/ready shapes), the port contract suites, and the port-ORACLE suites. **`npm run test:port` does NOT run the oracle suites** (`vitest.port.config.ts` excludes `test/unit/port/oracle/**`; they run only via `npm run test:oracle`, which boots real servers — budget several minutes). Notes: -- `t0-equivalence-rust.test.ts` node-vs-rust deep diff compares `ready` frames value-by-value (`buildId` is NOT in the normalization registry, so it is compared RAW): both sides stamp the SAME value — the worktree HEAD sha (the oracle node target runs from an isolated runtime root under the worktree so `git rev-parse HEAD` walk-up resolves the worktree sha; the rust target is `cargo build`-ed at test time by `ensureRustServerBuilt` and both build scripts re-stamp on HEAD moves; Node's source-run probe resolves the same HEAD) — or both `"unknown"` in git-less environments. This run is the proof. +- `t0-equivalence-rust.test.ts` node-vs-rust deep diff compares `ready` frames value-by-value (`buildId` is NOT in the normalization registry, so it is compared RAW): both sides stamp the SAME value — the worktree HEAD sha at build time. The node oracle target runs the COMPILED `dist/server/index.js` (its bake file written by `build:server` at the worktree HEAD), and `ensureServerBuilt`'s new stamp-freshness check (step 3j) rebuilds a stale node dist so a pre-existing stale `dist/` can never false-diverge against the fresh cargo-built rust target (`ensureRustServerBuilt`; both Rust build scripts re-stamp on HEAD moves). With git-less environments both stamps are `"unknown"`. This run is the parity proof. - `build:server` now emits `dist/server/build-id.json`; confirm with a real build: ```bash @@ -607,7 +686,7 @@ Stage by directory so every compiler-enumerated fix lands in the commit (the wor ```bash git status --short -git add shared/ port/contract/ws-server-messages.schema.json server/ scripts/bake-server-build-id.mjs test/server/ crates/ package.json +git add shared/ port/contract/ws-server-messages.schema.json port/oracle/harness/external-server.ts server/ scripts/bake-server-build-id.mjs test/server/ crates/ package.json git status --short git commit -m "feat(protocol): both servers stamp additive optional ready.buildId (artifact-time bake)" ``` @@ -656,9 +735,14 @@ describe('checkServerBuildId', () => { vi.restoreAllMocks() }) - it('reloads once and sets the sentinel on a real mismatch', () => { + it('reloads once, arming the sentinel BEFORE the reload fires, and the sentinel suppresses a second mismatch', () => { const storage = mapStorage() - const reload = vi.fn() + const reload = vi.fn(() => { + // Ordering proof: production must persist the sentinel BEFORE + // calling reload — an implementation that reloads first and arms + // second would lose the sentinel across the navigation. + expect(storage._map.get(SENTINEL), 'sentinel must be armed BEFORE reload fires').toBe('1') + }) checkServerBuildId({ clientBuildId: 'a'.repeat(40), serverBuildId: 'b'.repeat(40), reload, storage }) expect(reload).toHaveBeenCalledTimes(1) expect(storage._map.get(SENTINEL)).toBe('1') @@ -722,7 +806,13 @@ describe('checkServerBuildId', () => { it('does not throw or reload when the sessionStorage PROPERTY itself is inaccessible', () => { const reload = vi.fn() const original = Object.getOwnPropertyDescriptor(window, 'sessionStorage') - Object.defineProperty(window, 'sessionStorage', { value: undefined, configurable: true }) + // Harden contexts throw on PROPERTY ACCESS (SecurityError from a + // denying getter), not merely on method calls — install a getter that + // throws so the defaultStorage() fail-safe is actually exercised. + Object.defineProperty(window, 'sessionStorage', { + get() { throw new Error('SecurityError: storage denied') }, + configurable: true, + }) try { expect(() => checkServerBuildId({ clientBuildId: 'a'.repeat(40), serverBuildId: 'b'.repeat(40), reload })) .not.toThrow() @@ -770,6 +860,7 @@ describe('checkServerBuildId', () => { ```tsx describe('App ready buildId → one-shot server-build reload', () => { let originalLocation: Location + let reloadCalls: number beforeEach(() => { cleanup() vi.resetAllMocks() @@ -808,12 +899,24 @@ describe('App ready buildId → one-shot server-build reload', () => { }) sessionStorage.clear() + reloadCalls = 0 // jsdom 25's Location owns `reload` non-configurably — defineProperty on // window.location itself throws. Repo precedent (import-retry.test.ts): - // window-level replacement with save/restore. + // window-level replacement with save/restore. The reload stub asserts + // the sentinel is armed AT CALL TIME (the ordering proof lives here + // too, against real jsdom sessionStorage) and counts invocations. originalLocation = window.location Object.defineProperty(window, 'location', { - value: { ...window.location, reload: vi.fn() }, + value: { + ...window.location, + reload: () => { + expect( + sessionStorage.getItem('freshell.server-build-reload'), + 'sentinel must be armed BEFORE reload fires', + ).toBe('1') + reloadCalls++ + }, + }, writable: true, configurable: true, }) @@ -836,14 +939,14 @@ describe('App ready buildId → one-shot server-build reload', () => { await renderApp(store) sendReady({ serverInstanceId: 'srv-1', bootId: 'boot-1', buildId: 'b'.repeat(40) }) - expect(window.location.reload).toHaveBeenCalledTimes(1) + expect(reloadCalls).toBe(1) expect(sessionStorage.getItem('freshell.server-build-reload')).toBe('1') // The reload lands: the page reboots in the SAME tab (real jsdom // sessionStorage persists), the server is still stale, and the next // ready must NOT reload again. sendReady({ serverInstanceId: 'srv-1', bootId: 'boot-1', buildId: 'b'.repeat(40) }) - expect(window.location.reload).toHaveBeenCalledTimes(1) + expect(reloadCalls).toBe(1) }) it('a matching ready clears the sentinel and re-arms the guard', async () => { @@ -855,7 +958,7 @@ describe('App ready buildId → one-shot server-build reload', () => { // Server caught up to the client build (the post-reload convergence // case): match → sentinel cleared, no reload. sendReady({ serverInstanceId: 'srv-1', bootId: 'boot-1', buildId: 'a'.repeat(40) }) - expect(window.location.reload).not.toHaveBeenCalled() + expect(reloadCalls).toBe(0) expect(sessionStorage.getItem('freshell.server-build-reload')).toBeNull() }) @@ -866,7 +969,7 @@ describe('App ready buildId → one-shot server-build reload', () => { sendReady({ serverInstanceId: 'srv-1', bootId: 'boot-1' }) sendReady({ serverInstanceId: 'srv-1', bootId: 'boot-1', buildId: 'unknown' }) - expect(window.location.reload).not.toHaveBeenCalled() + expect(reloadCalls).toBe(0) expect(sessionStorage.getItem('freshell.server-build-reload')).toBeNull() }) }) @@ -878,7 +981,7 @@ describe('App ready buildId → one-shot server-build reload', () => { npm run test:vitest -- run test/unit/client/lib/server-build-check.test.ts test/unit/client/components/App.restart-signals.test.tsx ``` -Expected: FAIL — `server-build-check.test.ts` cannot resolve `@/lib/server-build-check` (module missing), and the App tests fail because a ready with `buildId` triggers no reload (`expect(window.location.reload).toHaveBeenCalledTimes(1)` sees 0). +Expected: FAIL — `server-build-check.test.ts` cannot resolve `@/lib/server-build-check` (module missing), and the App tests fail because a ready with `buildId` triggers no reload (`expect(reloadCalls).toBe(1)` sees 0). - [ ] **Step 3: Add the minimal production implementation** @@ -1083,10 +1186,10 @@ Verify the Vite define actually bakes the sha into the bundle (explicit pass/fai ```bash npm run build:client -rg -q "$(git rev-parse HEAD)" dist/client/assets/*.js && echo "BAKE OK: sha present in bundle" || echo "BAKE MISSING: sha absent from bundle" +rg -q "$(git rev-parse HEAD)" dist/client/assets/*.js && echo "BAKE OK: sha present in bundle" || { echo "BAKE MISSING: sha absent from bundle"; exit 1; } ``` -Expected: `BAKE OK: sha present in bundle` (the baked sha appears in the built bundle). (`npm run build:client` from this worktree writes the worktree's own `dist/client` — the main-checkout `npm run build` production-server guard does not apply here.) +Expected: `BAKE OK: sha present in bundle` — the command exits 0. A missing bake prints `BAKE MISSING` and exits NONZERO (the failure branch must not mask the failure behind a successful `echo`). (`npm run build:client` from this worktree writes the worktree's own `dist/client` — the main-checkout `npm run build` production-server guard does not apply here.) - [ ] **Step 6: Run impacted-test verification** @@ -1355,14 +1458,14 @@ No refactor needed. Confirm the registration mechanics: excluded from the match- - [ ] **Step 6: Run impacted-test verification** -Playwright registration changed (a new rust-only spec) and AGENTS.md was touched; the impacted set is the rust-chromium smoke that boots a real server (proving the registration change disturbed nothing) plus the two unit files most adjacent to the feature as a final belt-and-suspenders: +Playwright registration changed (a new rust-only spec) and AGENTS.md was touched; the impacted set is the rust-chromium self-test that boots a real Rust server through its own fixture (proving the registration change disturbed nothing — note `continuity-smoke.spec.ts` runs ONLY under its own conditional `continuity-smoke` project, NOT under `rust-chromium`, so it must not be used as the neighbor here) plus the two unit files most adjacent to the feature: ```bash -npx playwright test --config test/e2e-browser/playwright.config.ts --project=rust-chromium test/e2e-browser/specs/continuity-smoke.spec.ts test/e2e-browser/specs/server-build-mismatch-rust.spec.ts +npx playwright test --config test/e2e-browser/playwright.config.ts --project=rust-chromium test/e2e-browser/specs/harness-01-rust-server.spec.ts test/e2e-browser/specs/server-build-mismatch-rust.spec.ts npm run test:vitest -- run test/unit/client/lib/server-build-check.test.ts test/unit/client/components/App.restart-signals.test.tsx ``` -Expected: all PASS. Backend note: the repo rule about the configured `FRESHELL_E2E_BACKEND` is honored at execution kickoff (the user chooses local vs cloud once; the answer is recorded in `run-state.md`). This spec's coverage lane is the LOCAL rust-chromium project; it is CLOUD_SKIP'd with justification (no git metadata in the cloud build → both stamps "unknown" → the compare is inert there), and no cloud claim is made about cargo (the cloud runtime uses a prebuilt binary; cargo never runs there per `rust-server.ts:82-90`). +Expected: all PASS. Backend note: the repo rule about the configured `FRESHELL_E2E_BACKEND` is honored at execution kickoff — the user chooses local vs cloud once, INFORMED that this spec is cloud-incompatible by construction (the cloud image builds without git metadata, so both stamps are `"unknown"` and the compare is inert there). Regardless of the choice, this spec's coverage lane is the LOCAL rust-chromium project and it is CLOUD_SKIP'd with that justification (`playwright.cloud.config.ts`); if the user chooses cloud, the PR description documents the skip explicitly so no coverage claim is silent. No cloud claim is made about cargo (the cloud runtime uses a prebuilt binary; cargo never runs there per `rust-server.ts:82-90`). - [ ] **Step 7: Commit the task** @@ -1388,8 +1491,8 @@ Expected: typecheck + full default + server suites PASS, and the oracle suites ( | Requirement | Production behavior | Proof | | --- | --- | --- | -| Server stamps build identity in `ready` | Rust `freshell-ws/build.rs` bake → `Ready.build_id` (`Some`, sha/`"unknown"`); Node bake-file-or-probe → `buildId`; schema regenerated | roundtrip + wire tests; `test:port` + `test:oracle`; Node snapshot test | -| Identity = artifact-time git HEAD, `"unknown"` fallback, everywhere | `crates/freshell-ws/build.rs`; `dist/server/build-id.json` written by `build:server` (stale dist advertises its own build); `computeClientBuildId()` | `build-id.test.ts` (bake precedence + probe fallback); bundle-bake check (Task 2 Step 5); `cat dist/server/build-id.json` (Task 1 Step 6) | +| Server stamps build identity in `ready` | Rust `freshell-ws/build.rs` bake → `Ready.build_id` (`Some`, sha/`"unknown"`); Node bake-file-or-probe → `buildId`; schema regenerated | roundtrip + wire tests; `test:port` + `test:oracle` (oracle node dist rebuilds when its stamp is stale); Node snapshot test | +| Identity = artifact-time git HEAD, `"unknown"` fallback, everywhere | `crates/freshell-ws/build.rs`; `dist/server/build-id.json` written by `build:server` — compiled artifacts fail inert to `"unknown"` without a valid stamp (never a checkout probe); `computeClientBuildId()` | `build-id.test.ts` (bake precedence, source-vs-compiled split, garbage-stamp rejection); bundle-bake check (Task 2 Step 5); `cat dist/server/build-id.json` (Task 1 Step 6) | | Client compares on every `ready` | `ReadyMessageSchema.buildId` → `checkServerBuildId` in App's ready handler | App.restart-signals describe block | | Mismatch → reload exactly once | sentinel set before `reload()`; armed sentinel suppresses | unit matrix (real jsdom sessionStorage across the simulated reboot); e2e navigation count === 1 | | Sentinel survives a REAL navigation; repeat mismatches suppressed | sessionStorage per-tab persistence; suppression branch | e2e test 2 (commit-time persistence read + suppression); unit matrix | From f137951d6c8e659ca3f04b8eadb99738e9a66a4c Mon Sep 17 00:00:00 2001 From: Dan Shapiro <3732858+danshapiro@users.noreply.github.com> Date: Thu, 27 Aug 2026 12:40:43 -0700 Subject: [PATCH 06/15] feat(protocol): both servers stamp additive optional ready.buildId (artifact-time bake) --- .../freshell-protocol/src/server_messages.rs | 6 + .../freshell-protocol/tests/pane_reconcile.rs | 2 + crates/freshell-protocol/tests/roundtrip.rs | 29 ++++ crates/freshell-ws/build.rs | 68 +++++++++ crates/freshell-ws/src/lib.rs | 28 ++++ package.json | 2 +- port/contract/ws-server-messages.schema.json | 3 + port/oracle/harness/external-server.ts | 27 +++- scripts/bake-server-build-id.mjs | 43 ++++++ server/build-id.ts | 87 ++++++++++++ server/ws-handler.ts | 4 + shared/ws-protocol.ts | 5 + test/server/build-id.test.ts | 132 ++++++++++++++++++ test/server/ws-handshake-snapshot.test.ts | 28 ++++ 14 files changed, 462 insertions(+), 2 deletions(-) create mode 100644 crates/freshell-ws/build.rs create mode 100644 scripts/bake-server-build-id.mjs create mode 100644 server/build-id.ts create mode 100644 test/server/build-id.test.ts diff --git a/crates/freshell-protocol/src/server_messages.rs b/crates/freshell-protocol/src/server_messages.rs index 114ff646b..cfb49b348 100644 --- a/crates/freshell-protocol/src/server_messages.rs +++ b/crates/freshell-protocol/src/server_messages.rs @@ -797,6 +797,12 @@ pub struct Ready { pub boot_id: Option, #[serde(skip_serializing_if = "Option::is_none")] pub server_instance_id: Option, + /// The git commit this server binary was built from (`"unknown"` + /// fallback), stamped so the browser client can detect a client/server + /// build mismatch and reload once. Omitted from the wire entirely when + /// `None` (frozen-client inertness — same rule as `boot_id`). + #[serde(skip_serializing_if = "Option::is_none")] + pub build_id: Option, /// Reconciliation-handshake advertisement (§4.2): `Some` only when the /// client's `hello` opted in via `capabilities.paneReconcileV1`. A client /// must not send `pane.reconcile.request` unless the `ready` it just diff --git a/crates/freshell-protocol/tests/pane_reconcile.rs b/crates/freshell-protocol/tests/pane_reconcile.rs index 84c05cf85..83558a8b4 100644 --- a/crates/freshell-protocol/tests/pane_reconcile.rs +++ b/crates/freshell-protocol/tests/pane_reconcile.rs @@ -57,6 +57,7 @@ fn ready_capabilities_field_is_omitted_when_none() { timestamp: "2026-07-22T00:00:00.000Z".to_string(), boot_id: Some("boot-1".to_string()), server_instance_id: Some("srv-1".to_string()), + build_id: None, capabilities: None, }; let wire = serde_json::to_value(ServerMessage::Ready(ready)).expect("serializes"); @@ -72,6 +73,7 @@ fn ready_capabilities_advertise_pane_reconcile_v1_when_negotiated() { timestamp: "2026-07-22T00:00:00.000Z".to_string(), boot_id: Some("boot-1".to_string()), server_instance_id: Some("srv-1".to_string()), + build_id: None, capabilities: Some(ReadyCapabilities { pane_reconcile_v1: Some(true), pane_reconcile_fresh_agent_v1: None, diff --git a/crates/freshell-protocol/tests/roundtrip.rs b/crates/freshell-protocol/tests/roundtrip.rs index b9d54ba01..6f38eb624 100644 --- a/crates/freshell-protocol/tests/roundtrip.rs +++ b/crates/freshell-protocol/tests/roundtrip.rs @@ -163,6 +163,35 @@ fn ready_carries_server_instance_id_and_boot_id() { } } +#[test] +fn ready_carries_build_id_and_omits_it_when_absent() { + // deliverable: `ready` accepts an additive optional `buildId` (the git + // commit the server binary was built from) and OMITS it from the wire + // when absent — frozen-transcript inertness, same rule as `bootId`. + let with = r#"{"type":"ready","timestamp":"2026-07-05T04:20:52.546Z","serverInstanceId":"srv-abc","buildId":"a1b2c3d4e5f6a1b2c3d4e5f6a1b2c3d4e5f6a1b2"}"#; + match server_roundtrip(with, "ready") { + ServerMessage::Ready(r) => { + assert_eq!( + r.build_id.as_deref(), + Some("a1b2c3d4e5f6a1b2c3d4e5f6a1b2c3d4e5f6a1b2") + ); + } + other => panic!("expected Ready, got {other:?}"), + } + + let without = r#"{"type":"ready","timestamp":"2026-07-05T04:20:52.546Z","serverInstanceId":"srv-abc"}"#; + let msg: ServerMessage = serde_json::from_str(without).unwrap(); + let reser = serde_json::to_value(&msg).unwrap(); + assert!( + reser.get("buildId").is_none(), + "ready must omit buildId when absent: {reser}" + ); + match msg { + ServerMessage::Ready(r) => assert_eq!(r.build_id, None), + other => panic!("expected Ready, got {other:?}"), + } +} + #[test] fn terminal_inventory_and_settings_parse_from_transcript() { let transcript = read_json("port/oracle/fixtures/handshake-transcript.json"); diff --git a/crates/freshell-ws/build.rs b/crates/freshell-ws/build.rs new file mode 100644 index 000000000..13a3c7852 --- /dev/null +++ b/crates/freshell-ws/build.rs @@ -0,0 +1,68 @@ +//! Compile-time build-provenance stamp for `freshell-ws`: bakes the git +//! commit SHA into `FRESHELL_WS_BUILD_COMMIT` so the WS handshake's `ready` +//! can stamp `ready.buildId` (client-side stale-bundle auto-reload). +//! Build provenance is BUILD-scoped, not boot-scoped, so it deliberately +//! does NOT ride on `WsState` (whose contents are boot-scoped ids/state +//! injected by `freshell-server`). The full worktree-aware rationale for +//! the `rerun-if-changed` set lives in `crates/freshell-server/build.rs` — +//! this copy performs the SAME resolved-HEAD/ref/packed-refs watching so a +//! cached rebuild re-stamps when HEAD moves; both crates compile in the +//! same workspace build, so their baked commits agree. Never fails the +//! build over a missing/unavailable `git` (falls back to `"unknown"`). + +use std::path::PathBuf; +use std::process::Command; + +fn main() { + let commit = git_head_commit().unwrap_or_else(|| "unknown".to_string()); + println!("cargo:rustc-env=FRESHELL_WS_BUILD_COMMIT={commit}"); + for path in rerun_paths() { + println!("cargo:rerun-if-changed={}", path.display()); + } +} + +/// `git rev-parse HEAD`, trimmed. `None` on any failure (git not on `PATH`, +/// not inside a git checkout, ...) -- the caller falls back to `"unknown"`. +fn git_head_commit() -> Option { + let out = Command::new("git").args(["rev-parse", "HEAD"]).output().ok()?; + if !out.status.success() { + return None; + } + let s = String::from_utf8_lossy(&out.stdout).trim().to_string(); + if s.is_empty() { None } else { Some(s) } +} + +/// The exact paths that change when HEAD moves in THIS checkout, resolved +/// worktree-aware via `git rev-parse --git-path` (see the module doc and +/// `crates/freshell-server/build.rs`'s richer version for why each entry is +/// watched). Skipped resolutions degrade to cargo's default heuristics. +fn rerun_paths() -> Vec { + let mut paths = Vec::new(); + let git_path = |arg: &str| { + Command::new("git") + .args(["rev-parse", "--git-path", arg]) + .output() + .ok() + .filter(|o| o.status.success()) + .map(|o| PathBuf::from(String::from_utf8_lossy(&o.stdout).trim())) + .filter(|p| !p.as_os_str().is_empty()) + }; + if let Some(head) = git_path("HEAD") { + paths.push(head); + } + if let Some(head) = git_path("HEAD") { + if let Ok(contents) = std::fs::read_to_string(&head) { + if let Some(ref_name) = contents.strip_prefix("ref: ") { + if let Some(resolved) = git_path(ref_name.trim()) { + paths.push(resolved); + } + } + } + } + if let Some(packed) = git_path("packed-refs") { + if packed.exists() { + paths.push(packed); + } + } + paths +} diff --git a/crates/freshell-ws/src/lib.rs b/crates/freshell-ws/src/lib.rs index 573a9f91f..4dbd52e2c 100644 --- a/crates/freshell-ws/src/lib.rs +++ b/crates/freshell-ws/src/lib.rs @@ -20,6 +20,16 @@ //! The crate emits the frozen [`freshell_protocol`] server-message types so its //! wire bytes are contract-locked. +/// The git commit THIS binary was built from, baked into this crate at +/// compile time by this crate's `build.rs` (`FRESHELL_WS_BUILD_COMMIT`). +/// Falls back to the literal `"unknown"` when git was unavailable at build +/// time (e.g. a source tarball or the Cloud Run image, which builds without +/// git metadata) -- never a runtime failure. Build provenance is +/// BUILD-scoped, so this deliberately does NOT ride on `WsState`. +pub fn ready_build_id() -> Option { + Some(option_env!("FRESHELL_WS_BUILD_COMMIT").unwrap_or("unknown").to_string()) +} + pub mod activity; pub mod auto_resume; pub mod backpressure; @@ -537,6 +547,7 @@ pub async fn build_handshake_with_capabilities( timestamp: now_iso(), boot_id: Some(boot_id.clone()), server_instance_id: Some(state.server_instance_id.as_ref().clone()), + build_id: ready_build_id(), capabilities: (pane_reconcile_v1 || pane_reconcile_fresh_agent_v1).then_some( freshell_protocol::ReadyCapabilities { pane_reconcile_v1: pane_reconcile_v1.then_some(true), @@ -1025,6 +1036,23 @@ mod tests { assert_eq!(wire[3]["terminalMeta"], json!([])); } + /// The handshake `ready` stamps the build identity baked into THIS crate + /// by its `build.rs` (`FRESHELL_WS_BUILD_COMMIT`, the git commit the + /// binary was built from) so the browser client can detect a client/ + /// server build mismatch and reload once. Never absent on the wire from + /// a real server: the baked value is always `Some` (sha or `"unknown"`). + #[tokio::test] + async fn handshake_ready_stamps_build_id() { + let msgs = build_handshake(&state()).await; + let ready = serde_json::to_value(&msgs[0]).unwrap(); + assert!( + ready.get("buildId").is_some(), + "ready must stamp buildId: {ready}" + ); + let build_id = ready["buildId"].as_str().expect("buildId is a string"); + assert!(!build_id.is_empty(), "buildId must be non-empty: {build_id}"); + } + /// GAP1 (CFG-03 checklist follow-up) RED/GREEN target: when boot fell /// back, `config.fallback` slots into the ordered handshake right after /// `perf.logging` and before `terminal.inventory` -- mirrors the diff --git a/package.json b/package.json index fe51604c7..926bf6b24 100644 --- a/package.json +++ b/package.json @@ -21,7 +21,7 @@ "prebuild": "tsx scripts/prebuild-guard.ts", "build": "npm run typecheck:client && npm run build:client && npm run build:server", "build:client": "vite build --config config/vite/vite.config.ts", - "build:server": "tsc -p tsconfig.server.json", + "build:server": "tsc -p tsconfig.server.json && node scripts/bake-server-build-id.mjs", "build:electron": "node -e \"const fs=require('fs');fs.rmSync('dist/electron',{recursive:true,force:true});fs.rmSync('node_modules/.cache/tsconfig.electron.tsbuildinfo',{force:true});fs.rmSync('node_modules/.cache/tsconfig.electron-preload.tsbuildinfo',{force:true})\" && tsc -p tsconfig.electron.json && tsc -p tsconfig.electron-preload.json", "build:wizard": "vite build --config config/vite/vite.wizard.config.ts", "build:launch-chooser": "vite build --config config/vite/vite.launch-chooser.config.ts", diff --git a/port/contract/ws-server-messages.schema.json b/port/contract/ws-server-messages.schema.json index 31d608f67..171507416 100644 --- a/port/contract/ws-server-messages.schema.json +++ b/port/contract/ws-server-messages.schema.json @@ -1382,6 +1382,9 @@ "bootId": { "type": "string" }, + "buildId": { + "type": "string" + }, "capabilities": { "additionalProperties": false, "properties": { diff --git a/port/oracle/harness/external-server.ts b/port/oracle/harness/external-server.ts index c6f9f3e37..fc305d2d1 100644 --- a/port/oracle/harness/external-server.ts +++ b/port/oracle/harness/external-server.ts @@ -109,13 +109,38 @@ export function rustServerBinPath(root: string = PROJECT_ROOT): string { return path.join(root, 'target', 'release', 'freshell-server') } +/** + * Whether the node dist's baked build stamp (written by `build:server`'s + * `scripts/bake-server-build-id.mjs`) matches the CURRENT checkout HEAD. + * True when no bake file exists (pre-stamp dist or git-less build — keep + * the legacy exists-only behavior), when git is unavailable, or when the + * stamp is unreadable: those cases have no stamp semantics to violate. + * False only for a REAL staleness — a bake from an earlier HEAD — which + * must trigger a rebuild so the oracle's node-vs-rust `buildId` comparison + * compares same-HEAD artifacts, never a stale checkout against a fresh + * cargo build. + */ +function nodeBuildStampIsCurrent(root: string): boolean { + const bakePath = path.join(root, 'dist', 'server', 'build-id.json') + if (!fs.existsSync(bakePath)) return true + try { + const baked = (JSON.parse(fs.readFileSync(bakePath, 'utf8')) as { buildId?: unknown }).buildId + if (typeof baked !== 'string' || baked === 'unknown') return true + const head = spawnSync('git', ['rev-parse', 'HEAD'], { cwd: root, encoding: 'utf8' }) + if (head.status !== 0) return true + return baked === head.stdout.trim() + } catch { + return true + } +} + /** * Ensure the production node server bundle exists. Builds it with `npm run * build:server` if missing. Safe to call repeatedly — a no-op once built. */ export function ensureServerBuilt(root: string = PROJECT_ROOT): string { const entry = serverEntryPath(root) - if (fs.existsSync(entry)) return entry + if (fs.existsSync(entry) && nodeBuildStampIsCurrent(root)) return entry const result = spawnSync('npm', ['run', 'build:server'], { cwd: root, diff --git a/scripts/bake-server-build-id.mjs b/scripts/bake-server-build-id.mjs new file mode 100644 index 000000000..0e314526f --- /dev/null +++ b/scripts/bake-server-build-id.mjs @@ -0,0 +1,43 @@ +#!/usr/bin/env node +/** + * Bake the build-provenance stamp for the compiled Node server: writes + * `dist/server/build-id.json` = {"buildId": ""}. + * + * WHY a bake file: the running stamp must describe the BUILT ARTIFACT, not + * the checkout. `server/build-id.ts` prefers this file (resolved next to + * its compiled dist/server/build-id.js) and falls back to a runtime + * `git rev-parse HEAD` probe ONLY when no bake file exists next to it — + * which is exactly the tsx-from-source dev case, where the runtime probe + * is correct because dev runs current source. A stale `dist/server` + * started after HEAD moved therefore advertises the sha it was BUILT from, + * never a false "current" one. + * + * Runs after `tsc` in the `build:server` script. Atomic write (tmp+rename). + */ +import { execFileSync } from 'node:child_process' +import * as fs from 'node:fs' +import * as path from 'node:path' +import { fileURLToPath } from 'node:url' + +const repoRoot = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..') +const outPath = path.join(repoRoot, 'dist', 'server', 'build-id.json') + +function computeBuildId() { + try { + const sha = execFileSync('git', ['rev-parse', 'HEAD'], { + cwd: repoRoot, + stdio: ['ignore', 'pipe', 'ignore'], + }) + .toString() + .trim() + return /^[0-9a-f]{40}$/.test(sha) ? sha : 'unknown' + } catch { + return 'unknown' + } +} + +fs.mkdirSync(path.dirname(outPath), { recursive: true }) +const tmpPath = `${outPath}.tmp-${process.pid}` +fs.writeFileSync(tmpPath, `${JSON.stringify({ buildId: computeBuildId() })}\n`) +fs.renameSync(tmpPath, outPath) +console.log(`[bake-server-build-id] wrote ${outPath}`) diff --git a/server/build-id.ts b/server/build-id.ts new file mode 100644 index 000000000..b68aa4ba6 --- /dev/null +++ b/server/build-id.ts @@ -0,0 +1,87 @@ +import { execFileSync } from 'node:child_process' +import { readFileSync } from 'node:fs' +import { fileURLToPath } from 'node:url' + +const SHA_PATTERN = /^[0-9a-f]{40}$/ + +// Resolved relative to THIS module: next to the compiled +// dist/server/build-id.js in production (where `build:server`'s bake step +// wrote dist/server/build-id.json), or next to server/build-id.ts in +// tsx-from-source runs (where no bake file exists and the runtime probe is +// correct because dev runs current source). +const DEFAULT_BAKE_PATH = fileURLToPath(new URL('build-id.json', import.meta.url)) + +/** + * The git commit the server runs from — the SAME identity the Rust server + * bakes at compile time (`crates/freshell-ws/build.rs`'s + * `FRESHELL_WS_BUILD_COMMIT`) and the client bakes at Vite build time + * (`__FRESHELL_BUILD_ID__`). Falls back to the literal `"unknown"` when git + * is unavailable or the output is not a full 40-hex sha; the client's + * compare rule ignores `"unknown"` on both sides, so a git-less deployment + * never triggers a reload and never clears an armed one. + */ +export function computeBuildId(cwd: string = process.cwd()): string { + try { + const sha = execFileSync('git', ['rev-parse', 'HEAD'], { + cwd, + stdio: ['ignore', 'pipe', 'ignore'], + timeout: 5_000, + }) + .toString() + .trim() + return SHA_PATTERN.test(sha) ? sha : 'unknown' + } catch { + return 'unknown' + } +} + +/** Read a bake file written by `scripts/bake-server-build-id.mjs`. */ +export function readBakedBuildId(bakePath: string): string | undefined { + try { + const raw = JSON.parse(readFileSync(bakePath, 'utf8')) as { buildId?: unknown } + const value = raw.buildId + if (typeof value !== 'string') return undefined + // Same validation as the writer and the git probes: a 40-hex sha or the + // literal "unknown". Anything else is a malformed stamp — treat as + // absent, never authoritative (a garbage stamp would cause a needless + // mismatch reload). + return value === 'unknown' || SHA_PATTERN.test(value) ? value : undefined + } catch { + return undefined + } +} + +// Source runs (tsx dev, vitest) execute THIS .ts module; a compiled +// production artifact executes dist/server/build-id.js. The distinction +// decides what a MISSING bake file means (see resolveServerBuildId). +const SOURCE_MODE = import.meta.url.endsWith('.ts') + +/** + * BAKE-WINS-ELSE-FAIL-INERT: a compiled production artifact describes + * itself ONLY by its bake file — a stale dist started after HEAD moved + * advertises the sha it was built from (never a false "current" one), and + * an artifact whose stamp is missing or malformed fails inert to + * "unknown" (it must never impersonate the checkout). Source runs have no + * bake file next to the source module and probe runtime HEAD instead, + * which is correct because they execute current source. + */ +export function resolveServerBuildId( + bakePath: string = DEFAULT_BAKE_PATH, + opts?: { sourceMode?: boolean }, +): string { + const sourceMode = opts?.sourceMode ?? SOURCE_MODE + if (sourceMode) return computeBuildId() + return readBakedBuildId(bakePath) ?? 'unknown' +} + +let cached: string | undefined + +/** Per-process cached build id — one resolution per server lifetime. */ +export function serverBuildId(): string { + if (cached === undefined) cached = resolveServerBuildId() + return cached +} + +export function _resetServerBuildIdCacheForTests(): void { + cached = undefined +} diff --git a/server/ws-handler.ts b/server/ws-handler.ts index 15580db62..81d02f102 100644 --- a/server/ws-handler.ts +++ b/server/ws-handler.ts @@ -3,6 +3,7 @@ import { randomUUID } from 'crypto' import WebSocket, { WebSocketServer } from 'ws' import { z } from 'zod' import { logger } from './logger.js' +import { serverBuildId } from './build-id.js' import { testClockNowMs } from './test-clock.js' import { recordSessionLifecycleEvent } from './session-observability.js' import { getPerfConfig, startPerfTimer } from './perf-logger.js' @@ -585,6 +586,7 @@ export class WsHandler { private readonly serverInstanceId: string private readonly bootId: string + private readonly buildId: string // The runtime validator is authoritative here; we keep the field typed broadly because // the dynamic provider schemas widen discriminated-union inference beyond what TS/Zod model well. // Definitely assigned via rebuildClientMessageSchema() in the constructor (and re-run on dev reload). @@ -649,6 +651,7 @@ export class WsHandler { ? options.serverInstanceId : `srv-${randomUUID()}` this.bootId = `boot-${randomUUID()}` + this.buildId = serverBuildId() this.registry.setServerInstanceId?.(this.serverInstanceId) this.terminalStreamBroker = new TerminalStreamBroker(this.registry) @@ -2036,6 +2039,7 @@ export class WsHandler { timestamp: nowIso(), serverInstanceId: this.serverInstanceId, bootId: this.bootId, + buildId: this.buildId, }) this.scheduleHandshakeSnapshot(ws, state) return diff --git a/shared/ws-protocol.ts b/shared/ws-protocol.ts index bac12b1c2..cb407a360 100644 --- a/shared/ws-protocol.ts +++ b/shared/ws-protocol.ts @@ -745,6 +745,11 @@ export type ReadyMessage = { timestamp: string serverInstanceId?: string bootId?: string + /** The git commit the server binary was built from ("unknown" fallback). + * Additive/optional bootId doctrine: the client bakes its own build id at + * Vite build time and reloads once on a mismatch. Omitted from the wire + * when the Rust value is None. */ + buildId?: string /** Present iff the client's hello opted in via capabilities.paneReconcileV1. */ capabilities?: ReadyCapabilities } diff --git a/test/server/build-id.test.ts b/test/server/build-id.test.ts new file mode 100644 index 000000000..d1a48ab00 --- /dev/null +++ b/test/server/build-id.test.ts @@ -0,0 +1,132 @@ +import { execFileSync } from 'node:child_process' +import * as fs from 'node:fs' +import * as os from 'node:os' +import * as path from 'node:path' +import { fileURLToPath } from 'node:url' +import { afterEach, describe, expect, it, vi } from 'vitest' +import { + _resetServerBuildIdCacheForTests, + computeBuildId, + readBakedBuildId, + resolveServerBuildId, + serverBuildId, +} from '../../server/build-id.js' + +const REPO_ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '../..') + +vi.mock('node:child_process', async (importOriginal) => { + const actual = await importOriginal() + return { ...actual, execFileSync: vi.fn(actual.execFileSync) } +}) + +// The module under test imports execFileSync by name; re-import it mocked. +import { execFileSync as mockedExecFileSync } from 'node:child_process' + +function tempBakeFile(buildId: string | null): { dir: string; bakePath: string } { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'build-id-bake-')) + const bakePath = path.join(dir, 'build-id.json') + if (buildId !== null) { + fs.writeFileSync(bakePath, JSON.stringify({ buildId })) + } + return { dir, bakePath } +} + +describe('server build id', () => { + afterEach(() => { + _resetServerBuildIdCacheForTests() + vi.mocked(mockedExecFileSync).mockClear() + }) + + it('computeBuildId returns the current git HEAD sha for the repository', () => { + const expected = execFileSync('git', ['rev-parse', 'HEAD'], { cwd: REPO_ROOT }) + .toString() + .trim() + expect(computeBuildId(REPO_ROOT)).toBe(expected) + }) + + it('computeBuildId falls back to "unknown" outside a git repository', () => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'build-id-no-git-')) + try { + expect(computeBuildId(dir)).toBe('unknown') + } finally { + fs.rmSync(dir, { recursive: true, force: true }) + } + }) + + it('readBakedBuildId returns the baked value for a well-formed file', () => { + const { dir, bakePath } = tempBakeFile('b'.repeat(40)) + try { + expect(readBakedBuildId(bakePath)).toBe('b'.repeat(40)) + } finally { + fs.rmSync(dir, { recursive: true, force: true }) + } + }) + + it('readBakedBuildId returns undefined for malformed JSON, wrong shapes, or a missing file', () => { + const { dir, bakePath } = tempBakeFile(null) + try { + fs.writeFileSync(bakePath, 'not json {') + expect(readBakedBuildId(bakePath)).toBeUndefined() + fs.writeFileSync(bakePath, JSON.stringify({ buildId: 42 })) + expect(readBakedBuildId(bakePath)).toBeUndefined() + fs.writeFileSync(bakePath, JSON.stringify({ buildId: '' })) + expect(readBakedBuildId(bakePath)).toBeUndefined() + // Same validation as the writer: only a 40-hex sha or "unknown" is a + // legitimate stamp; a garbage string must never become authoritative. + fs.writeFileSync(bakePath, JSON.stringify({ buildId: 'garbage-stamp' })) + expect(readBakedBuildId(bakePath)).toBeUndefined() + expect(readBakedBuildId(path.join(dir, 'absent.json'))).toBeUndefined() + } finally { + fs.rmSync(dir, { recursive: true, force: true }) + } + }) + + it('resolveServerBuildId prefers the bake file over a runtime git probe', () => { + const { dir, bakePath } = tempBakeFile('c'.repeat(40)) + try { + // Compiled-artifact semantics are explicit here: vitest executes this + // module from source (SOURCE_MODE true), and a source run must probe + // runtime HEAD — the bake-wins rule only governs compiled artifacts + // (same pattern as the fail-inert test below). + expect(resolveServerBuildId(bakePath, { sourceMode: false })).toBe('c'.repeat(40)) + } finally { + fs.rmSync(dir, { recursive: true, force: true }) + } + }) + + it('resolveServerBuildId falls back to the runtime git probe when no bake file exists in SOURCE mode', () => { + const { dir } = tempBakeFile(null) + try { + expect(resolveServerBuildId(path.join(dir, 'build-id.json'))).toBe(computeBuildId(REPO_ROOT)) + } finally { + fs.rmSync(dir, { recursive: true, force: true }) + } + }) + + it('resolveServerBuildId fails inert to "unknown" for a compiled artifact without a valid stamp', () => { + const { dir, bakePath } = tempBakeFile(null) + try { + // A compiled artifact (sourceMode: false) must NEVER probe the + // checkout: a stale dist without its stamp advertises "unknown", not + // the current HEAD (which would falsely match a current client). + expect(resolveServerBuildId(path.join(dir, 'build-id.json'), { sourceMode: false })).toBe('unknown') + fs.writeFileSync(bakePath, 'corrupt {') + expect(resolveServerBuildId(bakePath, { sourceMode: false })).toBe('unknown') + fs.writeFileSync(bakePath, JSON.stringify({ buildId: 'garbage-stamp' })) + expect(resolveServerBuildId(bakePath, { sourceMode: false })).toBe('unknown') + } finally { + fs.rmSync(dir, { recursive: true, force: true }) + } + }) + + it('serverBuildId memoizes: the git probe runs once per process', () => { + _resetServerBuildIdCacheForTests() + // Source runs (tsx/vitest) have no bake file next to server/build-id.ts, + // so the first resolution exercises the git probe. + const first = serverBuildId() + const callsAfterFirst = vi.mocked(mockedExecFileSync).mock.calls.length + expect(serverBuildId()).toBe(first) + expect(vi.mocked(mockedExecFileSync).mock.calls.length).toBe(callsAfterFirst) + expect(callsAfterFirst).toBeGreaterThan(0) + }) +}) diff --git a/test/server/ws-handshake-snapshot.test.ts b/test/server/ws-handshake-snapshot.test.ts index 8d01ea0bc..6d98d539d 100644 --- a/test/server/ws-handshake-snapshot.test.ts +++ b/test/server/ws-handshake-snapshot.test.ts @@ -300,6 +300,34 @@ describe('ws handshake snapshot', () => { } }) + it('includes a buildId in the ready message, stable across clients in the same process', async () => { + const ws1 = new WebSocket(`ws://127.0.0.1:${port}/ws`) + const ws2 = new WebSocket(`ws://127.0.0.1:${port}/ws`) + + try { + await Promise.all([ + new Promise((resolve) => ws1.on('open', () => resolve())), + new Promise((resolve) => ws2.on('open', () => resolve())), + ]) + + const [ready1, ready2] = await Promise.all([ + waitForReady(ws1, 10_000), + waitForReady(ws2, 10_000), + ]) + + // Always stamped (bake or runtime probe, "unknown" fallback), stable + // within the process. + expect(typeof ready1.buildId).toBe('string') + expect((ready1.buildId as string).length).toBeGreaterThan(0) + expect(ready2.buildId).toBe(ready1.buildId) + // Distinct identity axis: not the boot id, not the instance id. + expect(ready1.buildId).not.toBe(ready1.bootId) + } finally { + await closeWs(ws1) + await closeWs(ws2) + } + }) + it('sends the same bootId to multiple clients within the same process', async () => { const ws1 = new WebSocket(`ws://127.0.0.1:${port}/ws`) const ws2 = new WebSocket(`ws://127.0.0.1:${port}/ws`) From 2ae0dd136ce918329551619e9904b330d5c55184 Mon Sep 17 00:00:00 2001 From: Dan Shapiro <3732858+danshapiro@users.noreply.github.com> Date: Thu, 27 Aug 2026 13:16:25 -0700 Subject: [PATCH 07/15] feat(client): reload once when ready.buildId differs from the baked build id --- config/vite/vite.config.ts | 22 +++ src/App.tsx | 14 ++ src/lib/server-build-check.ts | 105 +++++++++++++ src/vite-env.d.ts | 2 + .../components/App.restart-signals.test.tsx | 116 +++++++++++++++ .../client/lib/server-build-check.test.ts | 139 ++++++++++++++++++ 6 files changed, 398 insertions(+) create mode 100644 src/lib/server-build-check.ts create mode 100644 test/unit/client/lib/server-build-check.test.ts diff --git a/config/vite/vite.config.ts b/config/vite/vite.config.ts index 9a6a2ef9f..b1a564c81 100644 --- a/config/vite/vite.config.ts +++ b/config/vite/vite.config.ts @@ -3,12 +3,33 @@ import type { HttpProxy } from 'vite' import react from '@vitejs/plugin-react' import path from 'path' import { fileURLToPath } from 'url' +import { execFileSync } from 'node:child_process' import { getNetworkHost } from '../../server/get-network-host.js' const __filename = fileURLToPath(import.meta.url) const __dirname = path.dirname(__filename) const projectRoot = path.resolve(__dirname, '../..') +/** + * The client's build identity: the git commit the bundle was built from, + * matching the server-side stamps (`crates/freshell-ws/build.rs` / + * `server/build-id.ts` + `scripts/bake-server-build-id.mjs`). `"unknown"` + * fallback — the client's compare rule ignores `"unknown"` on both sides. + */ +function computeClientBuildId(): string { + try { + const sha = execFileSync('git', ['rev-parse', 'HEAD'], { + cwd: projectRoot, + stdio: ['ignore', 'pipe', 'ignore'], + }) + .toString() + .trim() + return /^[0-9a-f]{40}$/.test(sha) ? sha : 'unknown' + } catch { + return 'unknown' + } +} + /** * Transport-level proxy failures that mean "the backend is down or restarting": * refused (not yet listening), reset/pipe (killed mid-request), timeout/host @@ -57,6 +78,7 @@ export default defineConfig(({ mode }) => { plugins: [react()], define: { __PERF_LOGGING__: JSON.stringify(env.PERF_LOGGING || ''), + __FRESHELL_BUILD_ID__: JSON.stringify(computeClientBuildId()), }, resolve: { alias: { diff --git a/src/App.tsx b/src/App.tsx index b7835e020..2f53fda83 100644 --- a/src/App.tsx +++ b/src/App.tsx @@ -33,6 +33,7 @@ import { import { handleUiCommand } from '@/lib/ui-commands' import { getAuthToken } from '@/lib/auth' import { installTestHarness } from '@/lib/test-harness' +import { checkServerBuildId } from '@/lib/server-build-check' import { createPerfAuditBridge, installPerfAuditBridge } from '@/lib/perf-audit-bridge' import { getTabSwitchShortcutDirection, getTabLifecycleAction } from '@/lib/tab-switch-shortcuts' import { useThemeEffect } from '@/hooks/useTheme' @@ -159,6 +160,13 @@ const ReadyMessageSchema = z.object({ timestamp: z.string(), serverInstanceId: z.string().min(1), bootId: z.string().min(1).optional(), + // The server's baked build identity (additive/optional — old servers omit + // it). Compared in checkServerBuildId below. Plain `z.string()` (NOT + // min(1)): a present-but-EMPTY buildId must reach the helper and no-op + // there, never fail the WHOLE ready frame and silently disable restart + // detection. Only a non-string TYPE can fail the frame, which no real + // server emits (the helper additionally treats "unknown" as a no-op). + buildId: z.string().optional(), // Server capability ack (present iff our hello opted in). Deliberately a // loose record: an unexpected capabilities shape must never fail the WHOLE // ready frame and silently disable restart detection. @@ -1029,6 +1037,12 @@ export default function App() { if (!newBootId) { log.warn('ready frame carried no bootId; falling back to serverInstanceId for restart detection') } + // Server-build mismatch detection: the server stamps the git + // commit it was built from (ready.buildId, additive/optional); + // we compare it against our own Vite-baked + // __FRESHELL_BUILD_ID__ and reload ONCE on a mismatch (sentinel + // loop-guard lives in src/lib/server-build-check.ts). + checkServerBuildId({ serverBuildId: ready.data.buildId }) const bootIdRestart = !!previousBootId && previousBootId !== newBootId const instanceChanged = !!previousServerInstanceId && !!nextServerInstanceId diff --git a/src/lib/server-build-check.ts b/src/lib/server-build-check.ts new file mode 100644 index 000000000..a884cd223 --- /dev/null +++ b/src/lib/server-build-check.ts @@ -0,0 +1,105 @@ +import { createLogger } from '@/lib/client-logger' + +const log = createLogger('ServerBuildCheck') + +const SERVER_BUILD_RELOAD_SENTINEL = 'freshell.server-build-reload' + +export interface ServerBuildCheckOptions { + /** The client's own baked build id; defaults to `__FRESHELL_BUILD_ID__`. */ + clientBuildId?: string + /** The server's `ready.buildId`. */ + serverBuildId?: string + reload?: () => void + storage?: Pick +} + +/** + * The client's Vite-baked build id (`config/vite/vite.config.ts` defines it + * from `git rev-parse HEAD`). `typeof`-guarded because the Vitest client + * config has no define for it (same precedent as `__PERF_LOGGING__` in + * `src/lib/perf-logger.ts`) — an unbaked id means "cannot compare", never + * "reload". + */ +function resolveClientBuildId(): string | undefined { + if (typeof __FRESHELL_BUILD_ID__ === 'undefined') return undefined + const id = __FRESHELL_BUILD_ID__ + return id.length > 0 ? id : undefined +} + +/** + * sessionStorage can throw on PROPERTY ACCESS in hardened contexts (iframe + * sandboxing, privacy modes) — resolving it must be inside the fail-safe, + * never a ready-handler crash. + */ +function defaultStorage(): Pick | undefined { + try { + return window.sessionStorage + } catch { + return undefined + } +} + +/** + * Compare the server's `ready.buildId` against our own baked build id and + * reload ONCE on a real mismatch. Invariants: + * - reload iff BOTH ids are present, non-empty, neither is "unknown", and + * they differ ("unknown" == "unknown" is a no-op, never a match-and-clear); + * - the sessionStorage sentinel is set BEFORE reloading and suppresses any + * further reloads this tab session (a half-deployed server can never + * reload-loop; any sessionStorage failure = no reload, logged, fail-safe); + * - a MATCHING ready clears the sentinel (self-re-arm after convergence). + * KNOWN LIMITS (accepted for the self-hosted single-server threat model): + * - the "once" guarantee is per server identity — one origin fronted by + * servers built from DIFFERENT commits can oscillate (mismatch → reload → + * match clears → mismatch → …). Not hardened with a clears-per-session + * cap; revisit only if a split-deploy origin appears. + * - the compare is direction-free (shas carry no ordering), so a NEWER + * client against an OLDER server performs one futile bounded reload per + * fresh tab session. + */ +export function checkServerBuildId(options?: ServerBuildCheckOptions): void { + const clientBuildId = options?.clientBuildId ?? resolveClientBuildId() + const serverBuildId = options?.serverBuildId + if (!clientBuildId || !serverBuildId) return + if (clientBuildId === 'unknown' || serverBuildId === 'unknown') return + + const reload = options?.reload ?? (() => window.location.reload()) + + if (clientBuildId === serverBuildId) { + const storage = options?.storage ?? defaultStorage() + try { + storage?.removeItem(SERVER_BUILD_RELOAD_SENTINEL) + } catch { + // Ignore sessionStorage access failures (already disarmed-or-armed as + // found; nothing reloads on the match path either way). + } + return + } + + const storage = options?.storage ?? defaultStorage() + if (!storage) { + log.warn( + `server build ${serverBuildId} differs from client build ${clientBuildId} but ` + + 'sessionStorage is unavailable — suppressing the reload (fail-safe against loops)', + ) + return + } + try { + if (storage.getItem(SERVER_BUILD_RELOAD_SENTINEL) === '1') { + log.warn( + `server build ${serverBuildId} still differs from client build ${clientBuildId}; ` + + 'one reload already attempted this tab session — suppressing further reloads', + ) + return + } + storage.setItem(SERVER_BUILD_RELOAD_SENTINEL, '1') + } catch (err) { + log.warn('server-build sentinel persistence failed; suppressing the reload', err) + return + } + log.warn( + `server build ${serverBuildId} differs from client build ${clientBuildId}; ` + + 'reloading once to pick up the matching client bundle', + ) + reload() +} diff --git a/src/vite-env.d.ts b/src/vite-env.d.ts index 7d8daa201..e6ac24864 100644 --- a/src/vite-env.d.ts +++ b/src/vite-env.d.ts @@ -10,3 +10,5 @@ interface ImportMeta { } declare const __PERF_LOGGING__: string + +declare const __FRESHELL_BUILD_ID__: string diff --git a/test/unit/client/components/App.restart-signals.test.tsx b/test/unit/client/components/App.restart-signals.test.tsx index abf6f7d46..7f6791b3a 100644 --- a/test/unit/client/components/App.restart-signals.test.tsx +++ b/test/unit/client/components/App.restart-signals.test.tsx @@ -440,3 +440,119 @@ describe('App restart signals (bootId + serverInstanceId fallback)', () => { errorSpy.mockRestore() }) }) + +describe('App ready buildId → one-shot server-build reload', () => { + let originalLocation: Location + let reloadCalls: number + beforeEach(() => { + cleanup() + vi.resetAllMocks() + stubAudio() + wsMocks.onReconnect.mockReturnValue(() => {}) + wsMocks.onDisconnect.mockReturnValue(() => {}) + wsMocks.isReady = false + wsMocks.serverInstanceId = undefined + terminalRestoreMocks.addTerminalRestoreRequestId.mockClear() + terminalRestoreMocks.addTerminalFreshRecoveryRequestId.mockClear() + messageHandler = null + + wsMocks.onMessage.mockImplementation((cb: (msg: any) => void) => { + messageHandler = cb + return () => { messageHandler = null } + }) + + fetchSidebarSessionsSnapshot.mockReset() + fetchSidebarSessionsSnapshot.mockResolvedValue([]) + getTerminalDirectoryPage.mockReset() + getTerminalDirectoryPage.mockResolvedValue({ items: [], revision: 1, nextCursor: null }) + searchTerminalView.mockReset() + searchTerminalView.mockResolvedValue({ matches: [] }) + + apiGet.mockImplementation((url: string) => { + if (url === '/api/bootstrap') { + return Promise.resolve({ + settings: defaultServerSettings, + platform: { platform: 'linux' }, + shell: { authenticated: true, ready: true }, + }) + } + if (url === '/api/settings') return Promise.resolve(defaultSettings) + if (url === '/api/platform') return Promise.resolve({ platform: 'linux' }) + return Promise.resolve({}) + }) + + sessionStorage.clear() + reloadCalls = 0 + // jsdom 25's Location owns `reload` non-configurably — defineProperty on + // window.location itself throws. Repo precedent (import-retry.test.ts): + // window-level replacement with save/restore. The reload stub asserts + // the sentinel is armed AT CALL TIME (the ordering proof lives here + // too, against real jsdom sessionStorage) and counts invocations. + originalLocation = window.location + Object.defineProperty(window, 'location', { + value: { + ...window.location, + reload: () => { + expect( + sessionStorage.getItem('freshell.server-build-reload'), + 'sentinel must be armed BEFORE reload fires', + ).toBe('1') + reloadCalls++ + }, + }, + writable: true, + configurable: true, + }) + }) + + afterEach(() => { + cleanup() + vi.unstubAllGlobals() + Object.defineProperty(window, 'location', { + value: originalLocation, + writable: true, + configurable: true, + }) + sessionStorage.clear() + }) + + it('mismatched ready buildId triggers exactly one reload, and the sentinel (real sessionStorage, persisting across the simulated reboot) suppresses the next mismatched ready', async () => { + vi.stubGlobal('__FRESHELL_BUILD_ID__', 'a'.repeat(40)) + const store = createStore() + await renderApp(store) + + sendReady({ serverInstanceId: 'srv-1', bootId: 'boot-1', buildId: 'b'.repeat(40) }) + expect(reloadCalls).toBe(1) + expect(sessionStorage.getItem('freshell.server-build-reload')).toBe('1') + + // The reload lands: the page reboots in the SAME tab (real jsdom + // sessionStorage persists), the server is still stale, and the next + // ready must NOT reload again. + sendReady({ serverInstanceId: 'srv-1', bootId: 'boot-1', buildId: 'b'.repeat(40) }) + expect(reloadCalls).toBe(1) + }) + + it('a matching ready clears the sentinel and re-arms the guard', async () => { + vi.stubGlobal('__FRESHELL_BUILD_ID__', 'a'.repeat(40)) + sessionStorage.setItem('freshell.server-build-reload', '1') + const store = createStore() + await renderApp(store) + + // Server caught up to the client build (the post-reload convergence + // case): match → sentinel cleared, no reload. + sendReady({ serverInstanceId: 'srv-1', bootId: 'boot-1', buildId: 'a'.repeat(40) }) + expect(reloadCalls).toBe(0) + expect(sessionStorage.getItem('freshell.server-build-reload')).toBeNull() + }) + + it('never reloads on missing or "unknown" buildIds', async () => { + vi.stubGlobal('__FRESHELL_BUILD_ID__', 'a'.repeat(40)) + const store = createStore() + await renderApp(store) + + sendReady({ serverInstanceId: 'srv-1', bootId: 'boot-1' }) + sendReady({ serverInstanceId: 'srv-1', bootId: 'boot-1', buildId: 'unknown' }) + expect(reloadCalls).toBe(0) + expect(sessionStorage.getItem('freshell.server-build-reload')).toBeNull() + }) +}) diff --git a/test/unit/client/lib/server-build-check.test.ts b/test/unit/client/lib/server-build-check.test.ts new file mode 100644 index 000000000..768f2f846 --- /dev/null +++ b/test/unit/client/lib/server-build-check.test.ts @@ -0,0 +1,139 @@ +import { afterEach, describe, expect, it, vi } from 'vitest' +import { checkServerBuildId } from '@/lib/server-build-check' + +const SENTINEL = 'freshell.server-build-reload' + +function mapStorage() { + const map = new Map() + return { + getItem: (k: string) => map.get(k) ?? null, + setItem: (k: string, v: string) => void map.set(k, v), + removeItem: (k: string) => void map.delete(k), + _map: map, + } +} + +describe('checkServerBuildId', () => { + afterEach(() => { + vi.unstubAllGlobals() + vi.restoreAllMocks() + }) + + it('reloads once, arming the sentinel BEFORE the reload fires, and the sentinel suppresses a second mismatch', () => { + const storage = mapStorage() + const reload = vi.fn(() => { + // Ordering proof: production must persist the sentinel BEFORE + // calling reload — an implementation that reloads first and arms + // second would lose the sentinel across the navigation. + expect(storage._map.get(SENTINEL), 'sentinel must be armed BEFORE reload fires').toBe('1') + }) + checkServerBuildId({ clientBuildId: 'a'.repeat(40), serverBuildId: 'b'.repeat(40), reload, storage }) + expect(reload).toHaveBeenCalledTimes(1) + expect(storage._map.get(SENTINEL)).toBe('1') + }) + + it('never reloads twice: an armed sentinel suppresses the reload', () => { + const storage = mapStorage() + storage._map.set(SENTINEL, '1') + const reload = vi.fn() + checkServerBuildId({ clientBuildId: 'a'.repeat(40), serverBuildId: 'b'.repeat(40), reload, storage }) + expect(reload).not.toHaveBeenCalled() + expect(storage._map.get(SENTINEL)).toBe('1') + }) + + it('a matching ready clears the sentinel (self-re-arm)', () => { + const storage = mapStorage() + storage._map.set(SENTINEL, '1') + const reload = vi.fn() + checkServerBuildId({ clientBuildId: 'a'.repeat(40), serverBuildId: 'a'.repeat(40), reload, storage }) + expect(reload).not.toHaveBeenCalled() + expect(storage._map.get(SENTINEL)).toBeUndefined() + }) + + it('is a no-op when either side is missing, empty, or "unknown"', () => { + for (const opts of [ + { clientBuildId: 'a'.repeat(40), serverBuildId: undefined }, + { clientBuildId: undefined, serverBuildId: 'b'.repeat(40) }, + { clientBuildId: '', serverBuildId: 'b'.repeat(40) }, + { clientBuildId: 'unknown', serverBuildId: 'b'.repeat(40) }, + { clientBuildId: 'a'.repeat(40), serverBuildId: 'unknown' }, + { clientBuildId: 'unknown', serverBuildId: 'unknown' }, + ] as const) { + const storage = mapStorage() + const reload = vi.fn() + checkServerBuildId({ ...opts, reload, storage }) + expect(reload, JSON.stringify(opts)).not.toHaveBeenCalled() + expect(storage._map.get(SENTINEL)).toBeUndefined() + } + }) + + it('an armed sentinel survives an "unknown"-vs-"unknown" ready (never treated as a match)', () => { + const storage = mapStorage() + storage._map.set(SENTINEL, '1') + const reload = vi.fn() + checkServerBuildId({ clientBuildId: 'unknown', serverBuildId: 'unknown', reload, storage }) + expect(reload).not.toHaveBeenCalled() + expect(storage._map.get(SENTINEL)).toBe('1') + }) + + it('does not reload when the sentinel cannot be persisted (fail-safe against reload loops)', () => { + const reload = vi.fn() + const storage = { + getItem: () => { throw new Error('quota') }, + setItem: () => { throw new Error('quota') }, + removeItem: () => { throw new Error('quota') }, + } + checkServerBuildId({ clientBuildId: 'a'.repeat(40), serverBuildId: 'b'.repeat(40), reload, storage }) + expect(reload).not.toHaveBeenCalled() + }) + + it('does not throw or reload when the sessionStorage PROPERTY itself is inaccessible', () => { + const reload = vi.fn() + const original = Object.getOwnPropertyDescriptor(window, 'sessionStorage') + // Harden contexts throw on PROPERTY ACCESS (SecurityError from a + // denying getter), not merely on method calls — install a getter that + // throws so the defaultStorage() fail-safe is actually exercised. + Object.defineProperty(window, 'sessionStorage', { + get() { throw new Error('SecurityError: storage denied') }, + configurable: true, + }) + try { + expect(() => checkServerBuildId({ clientBuildId: 'a'.repeat(40), serverBuildId: 'b'.repeat(40), reload })) + .not.toThrow() + expect(reload).not.toHaveBeenCalled() + } finally { + if (original) Object.defineProperty(window, 'sessionStorage', original) + } + }) + + it('falls back to the __FRESHELL_BUILD_ID__ global and window defaults when options are omitted', () => { + vi.stubGlobal('__FRESHELL_BUILD_ID__', 'c'.repeat(40)) + const reload = vi.fn() + // jsdom 25's Location owns `reload` non-configurably — defineProperty on + // window.location itself throws. Repo precedent (import-retry.test.ts): + // replace window-level with a spread copy. + const originalLocation = window.location + Object.defineProperty(window, 'location', { + value: { ...window.location, reload }, + writable: true, + configurable: true, + }) + sessionStorage.clear() + + checkServerBuildId({ serverBuildId: 'd'.repeat(40) }) + expect(reload).toHaveBeenCalledTimes(1) + expect(sessionStorage.getItem(SENTINEL)).toBe('1') + + // And with the global absent (Vitest has no define), it is a no-op. + vi.unstubAllGlobals() + sessionStorage.removeItem(SENTINEL) + checkServerBuildId({ serverBuildId: 'd'.repeat(40) }) + expect(reload).toHaveBeenCalledTimes(1) + + Object.defineProperty(window, 'location', { + value: originalLocation, + writable: true, + configurable: true, + }) + }) +}) From f9ac736c8fa6dbae4f5a1c058744c4a757d9e87f Mon Sep 17 00:00:00 2001 From: Dan Shapiro <3732858+danshapiro@users.noreply.github.com> Date: Thu, 27 Aug 2026 13:37:28 -0700 Subject: [PATCH 08/15] test(e2e): rust spec proves one-shot sentinel-guarded reload on ready.buildId mismatch --- AGENTS.md | 2 +- test/e2e-browser/playwright.cloud.config.ts | 6 + test/e2e-browser/playwright.config.ts | 7 + .../specs/server-build-mismatch-rust.spec.ts | 143 ++++++++++++++++++ 4 files changed, 157 insertions(+), 1 deletion(-) create mode 100644 test/e2e-browser/specs/server-build-mismatch-rust.spec.ts diff --git a/AGENTS.md b/AGENTS.md index 60e90ebb2..8dda134e0 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -224,7 +224,7 @@ live in [docs/development/gcloud-robot.md](docs/development/gcloud-robot.md). ### Key Architectural Patterns -**WebSocket Protocol:** Schema-validated messages using Zod. Handshake flow: client sends `hello` with token → server validates → sends `ready`. Message types include `terminal.create/input/resize/detach/attach` and broadcasts like `sessions.updated`. +**WebSocket Protocol:** Schema-validated messages using Zod. Handshake flow: client sends `hello` with token → server validates → sends `ready`. Message types include `terminal.create/input/resize/detach/attach` and broadcasts like `sessions.updated`. The `ready` frame carries an optional additive `buildId` (the server's artifact-time-baked git commit, `"unknown"` fallback): the client bakes its own at Vite build time (`__FRESHELL_BUILD_ID__`) and, on a mismatch, reloads exactly once per tab session (sessionStorage sentinel `freshell.server-build-reload`), self-healing stale-client contract errors; `"unknown"` on either side never triggers or clears the guard (`src/lib/server-build-check.ts`). The once-guard is per server identity: an origin fronted by mixed-build servers could oscillate, and a newer client against an older server costs one futile bounded reload per fresh tab session (both accepted for the single-server self-hosted model). **PTY Lifecycle:** Each terminal has a unique ID. Server maintains 64KB scrollback buffer. On attach, client receives buffer snapshot then streams new output. On detach, process continues running (background session). Configurable idle timeout (15 mins default). diff --git a/test/e2e-browser/playwright.cloud.config.ts b/test/e2e-browser/playwright.cloud.config.ts index 78227deb9..09a26f6a3 100644 --- a/test/e2e-browser/playwright.cloud.config.ts +++ b/test/e2e-browser/playwright.cloud.config.ts @@ -53,6 +53,12 @@ const CLOUD_SKIP_SPECS = [ // Rust-only: asserts e2eServerKind === 'rust' but runs under chromium // project (not in RUST_ONLY_SPECS — pre-existing config gap) 'term28-path-shadow-rust.spec.ts', + // Server-build mismatch reload: the Cloud Run image builds WITHOUT git + // metadata (.dockerignore drops .git), so the Rust bake and the Vite + // define are both "unknown" there and the client's compare is inert BY + // DESIGN — a mismatched ready can never trigger a reload on that lane. + // Coverage lives on the local rust-chromium project. + 'server-build-mismatch-rust.spec.ts', // Environment-sensitive: page lifecycle (pagehide/unload) timing differs // in cloud containers; passes locally but flakes in cloud 'tabs-client-retire.spec.ts', diff --git a/test/e2e-browser/playwright.config.ts b/test/e2e-browser/playwright.config.ts index afb4a8142..b45c3acb6 100644 --- a/test/e2e-browser/playwright.config.ts +++ b/test/e2e-browser/playwright.config.ts @@ -202,6 +202,10 @@ export const RUST_ONLY_SPECS = [ // LANE E create protection: two concurrent RustServers, storm-isolation // proof. See docs/plans/2026-07-25-rust-create-protection.md /create-protection-isolation-rust\.spec\.ts$/, + // Server-build mismatch auto-reload: injects a mismatched ready.buildId + // through the test harness and proves ONE sentinel-guarded reload. + // Rust-only: owns a RustServer directly (see the spec header). + /server-build-mismatch-rust\.spec\.ts$/, /launch-retry-restart-rust\.spec\.ts$/, /double-restart-terminal-restore-rust\.spec\.ts$/, /turn-complete-restart-resume-rust\.spec\.ts$/, @@ -369,6 +373,9 @@ export default defineConfig({ // Rust WS create path's codex-special resume derivation ignoring // `sessionRef` (legacy anchor `ws-handler.ts:2040-2047` was correct). /codex-terminal-bounce-rust\.spec\.ts$/, + // Server-build mismatch auto-reload (the-usual/server-version-reload): + // mismatched ready.buildId → one reload, sentinel suppresses repeats. + /server-build-mismatch-rust\.spec\.ts$/, // MCP bridge pin (Slice 2, docs/plans/2026-07-18-agent-api-mcp-parity-spec.md // §6/§8.3): drives the UNMODIFIED legacy Node MCP stdio binary // against an owned, ephemeral Rust server. Rust-only (no legacy diff --git a/test/e2e-browser/specs/server-build-mismatch-rust.spec.ts b/test/e2e-browser/specs/server-build-mismatch-rust.spec.ts new file mode 100644 index 000000000..63827928e --- /dev/null +++ b/test/e2e-browser/specs/server-build-mismatch-rust.spec.ts @@ -0,0 +1,143 @@ +/** + * Server-build mismatch auto-reload (the-usual/server-version-reload). + * + * The user story: a tab running a client bundle built at commit A connects + * to a server built at commit B; the server's `ready.buildId` differs from + * the client's baked `__FRESHELL_BUILD_ID__`; the client reloads EXACTLY + * ONCE (sentinel `freshell.server-build-reload` in sessionStorage) and + * converges to a healthy ready connection. A repeat mismatched ready must + * NOT reload again — a half-deployed server can never reload-loop. + * + * COVERAGE BOUNDARY (read before judging): what e2e proves here is + * (1) the full production compare-and-reload pipeline through the REAL App + * ready handler (mismatch injected via the test harness — a REAL server + * stamps its own sha, which may or may not equal this worktree's client + * bake, so the injection makes the compare deterministic either way), + * (2) sessionStorage persistence across a REAL navigation, and (3) + * suppression of a repeat mismatch. The "code armed the sentinel BEFORE + * reloading" ORDER is proven by the unit suite (App.restart-signals: real + * jsdom sessionStorage persisting across the simulated reboot). Observing + * the code-armed sentinel surviving a REAL navigation e2e is not + * deterministic here: after any reload the boot's REAL ready either matches + * (same-HEAD artifacts → legitimately clears the sentinel) or mismatches + * (stale-bake environments → keeps it), so the post-reload sentinel state + * is environment-dependent — hence the persistence test reads at commit + * time and the suppression test seeds its state AFTER the boot settles. + * Seeding is state setup, the same practice as seeding localStorage in + * other suites; the PERSISTENCE and SUPPRESSION behavior exercised is + * entirely production code. + * + * Rust-only: registers under `rust-chromium` + RUST_ONLY_SPECS (owns a + * RustServer directly, the e2eServerKind seam not used). CLOUD-SKIPPED with + * justification (see playwright.cloud.config.ts): the Cloud Run image + * builds WITHOUT git metadata, so both the Rust bake and the Vite define + * are "unknown" there and the compare is inert BY DESIGN — this spec can + * only pass on a lane where at least the client bake is a real sha. + */ +import { test, expect } from '../helpers/fixtures.js' +import { RustServer, ensureRustServerBuilt } from '../helpers/rust-server.js' +import type { TestServerInfo } from '../helpers/test-server.js' +import { TestHarness } from '../helpers/test-harness.js' + +const MISMATCHED_BUILD_ID = 'f'.repeat(40) +const SENTINEL = 'freshell.server-build-reload' + +test.describe('server build mismatch reload (rust)', () => { + let server: RustServer | undefined + let info: TestServerInfo + + test.beforeAll(async () => { + test.setTimeout(600_000) // first release build of freshell-server can take minutes + ensureRustServerBuilt() + server = new RustServer() + info = await server.start() + }) + + test.afterAll(async () => { + await server?.stop().catch(() => {}) + }) + + test('mismatched ready buildId reloads exactly once and converges', async ({ browser }) => { + const context = await browser.newContext({ serviceWorkers: 'block' }) + const page = await context.newPage() + await page.goto(`${info.baseUrl}/?token=${info.token}&e2e=1`) + const harness = new TestHarness(page) + await harness.waitForHarness() + await harness.waitForConnection() + + // Start counting AFTER the boot-time compare so the real ready's own + // match/mismatch outcome (both artifacts usually share this worktree's + // HEAD) cannot pollute the count; also re-clear the sentinel so the + // injected mismatch is the one that arms it. + await page.evaluate((key) => sessionStorage.removeItem(key), SENTINEL) + let navigations = 0 + page.on('framenavigated', () => { navigations++ }) + + // Injected mismatch → exactly one reload, and the page reboots into a + // healthy ready connection (convergence). + await harness.receiveWsMessage({ + type: 'ready', + timestamp: new Date().toISOString(), + serverInstanceId: 'srv-build-mismatch-probe', + bootId: 'boot-build-mismatch-probe', + buildId: MISMATCHED_BUILD_ID, + }) + await expect.poll(() => navigations, { timeout: 20_000 }).toBe(1) + const rebooted = new TestHarness(page) + await rebooted.waitForHarness() + await rebooted.waitForConnection() + + await context.close() + }) + + test('sentinel persists across a real navigation', async ({ browser }) => { + const context = await browser.newContext({ serviceWorkers: 'block' }) + const page = await context.newPage() + await page.goto(`${info.baseUrl}/?token=${info.token}&e2e=1`) + const harness = new TestHarness(page) + await harness.waitForHarness() + await harness.waitForConnection() + + // Seed the state the production code would have armed on a previous + // mismatched ready in this tab (see the coverage boundary above). + await page.evaluate((key) => sessionStorage.setItem(key, '1'), SENTINEL) + + // A REAL navigation: sessionStorage must survive it (per-tab, per-origin + // storage) — read at commit time, BEFORE the rebooted app's real ready + // can legitimately match-and-clear it (same-HEAD artifacts match). + await page.reload({ waitUntil: 'commit' }) + const persisted = await page.evaluate((key) => sessionStorage.getItem(key), SENTINEL) + expect(persisted, 'sentinel must survive a real navigation').toBe('1') + + await context.close() + }) + + test('a seeded sentinel suppresses a repeat mismatch (no reload)', async ({ browser }) => { + const context = await browser.newContext({ serviceWorkers: 'block' }) + const page = await context.newPage() + await page.goto(`${info.baseUrl}/?token=${info.token}&e2e=1`) + const harness = new TestHarness(page) + await harness.waitForHarness() + await harness.waitForConnection() + + // Seed AFTER the boot settles (the boot's real ready may legitimately + // match-and-clear an earlier sentinel; seeding here is the setup for + // the suppression proof — the arming ORDER is unit-proven, the + // navigation persistence is proven by the previous test). + await page.evaluate((key) => sessionStorage.setItem(key, '1'), SENTINEL) + let navigations = 0 + page.on('framenavigated', () => { navigations++ }) + + await harness.receiveWsMessage({ + type: 'ready', + timestamp: new Date().toISOString(), + serverInstanceId: 'srv-build-mismatch-probe', + bootId: 'boot-build-mismatch-probe', + buildId: MISMATCHED_BUILD_ID, + }) + await page.waitForTimeout(3_000) + expect(navigations, 'persisted sentinel must suppress the repeat mismatch').toBe(0) + + await context.close() + }) +}) From 738e9346d16f0da3d1adfa69b6c8f77dbe51bcd7 Mon Sep 17 00:00:00 2001 From: Dan Shapiro <3732858+danshapiro@users.noreply.github.com> Date: Thu, 27 Aug 2026 13:59:12 -0700 Subject: [PATCH 09/15] fix(server): make the build-id bake path lazy so non-file import environments can load ws-handler --- server/build-id.ts | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/server/build-id.ts b/server/build-id.ts index b68aa4ba6..02fbf1f2e 100644 --- a/server/build-id.ts +++ b/server/build-id.ts @@ -9,7 +9,16 @@ const SHA_PATTERN = /^[0-9a-f]{40}$/ // wrote dist/server/build-id.json), or next to server/build-id.ts in // tsx-from-source runs (where no bake file exists and the runtime probe is // correct because dev runs current source). -const DEFAULT_BAKE_PATH = fileURLToPath(new URL('build-id.json', import.meta.url)) +function defaultBakePath(): string { + try { + return fileURLToPath(new URL('build-id.json', import.meta.url)) + } catch { + // Non-file: import.meta.url (electron-style loaders): a relative path + // that readFileSync will miss, degrading to the inert "unknown" (compiled) + // or the runtime git probe (source) — never an import crash. + return 'build-id.json' + } +} /** * The git commit the server runs from — the SAME identity the Rust server @@ -66,7 +75,7 @@ const SOURCE_MODE = import.meta.url.endsWith('.ts') * which is correct because they execute current source. */ export function resolveServerBuildId( - bakePath: string = DEFAULT_BAKE_PATH, + bakePath: string = defaultBakePath(), opts?: { sourceMode?: boolean }, ): string { const sourceMode = opts?.sourceMode ?? SOURCE_MODE From 3e5d5de20fb92427a858a40e528eb47824504928 Mon Sep 17 00:00:00 2001 From: Dan Shapiro <3732858+danshapiro@users.noreply.github.com> Date: Thu, 27 Aug 2026 14:15:29 -0700 Subject: [PATCH 10/15] test(server): bound the sidebar-refresh close-wait so flaky failures surface their real error --- .../ws-sidebar-snapshot-refresh.test.ts | 39 +++++++++++++++++-- 1 file changed, 36 insertions(+), 3 deletions(-) diff --git a/test/server/ws-sidebar-snapshot-refresh.test.ts b/test/server/ws-sidebar-snapshot-refresh.test.ts index 51a6c74f9..fff111937 100644 --- a/test/server/ws-sidebar-snapshot-refresh.test.ts +++ b/test/server/ws-sidebar-snapshot-refresh.test.ts @@ -175,7 +175,18 @@ describe('ws sidebar snapshot refresh', () => { await expectNoMessage(ws, (m) => m.type === 'sessions.updated') } finally { ws.terminate() - await new Promise((resolve) => ws.on('close', () => resolve())) + // BOUNDED close-wait: an unbounded wait here (1) could hang until the + // 30s test timeout, masking the test body's real failure with a + // useless "Test timed out", and (2) made flaky failures undiagnosable + // under full-suite load. Resolve on a short grace period — the + // original error (if any) propagates from the test body itself. + await new Promise((resolve) => { + const grace = setTimeout(() => resolve(), 2_000) + ws.on('close', () => { + clearTimeout(grace) + resolve() + }) + }) } }) @@ -258,7 +269,18 @@ describe('ws sidebar snapshot refresh', () => { }) } finally { ws.terminate() - await new Promise((resolve) => ws.on('close', () => resolve())) + // BOUNDED close-wait: an unbounded wait here (1) could hang until the + // 30s test timeout, masking the test body's real failure with a + // useless "Test timed out", and (2) made flaky failures undiagnosable + // under full-suite load. Resolve on a short grace period — the + // original error (if any) propagates from the test body itself. + await new Promise((resolve) => { + const grace = setTimeout(() => resolve(), 2_000) + ws.on('close', () => { + clearTimeout(grace) + resolve() + }) + }) } }) @@ -288,7 +310,18 @@ describe('ws sidebar snapshot refresh', () => { await expectNoMessage(ws, (m) => m.type === 'sessions.updated') } finally { ws.terminate() - await new Promise((resolve) => ws.on('close', () => resolve())) + // BOUNDED close-wait: an unbounded wait here (1) could hang until the + // 30s test timeout, masking the test body's real failure with a + // useless "Test timed out", and (2) made flaky failures undiagnosable + // under full-suite load. Resolve on a short grace period — the + // original error (if any) propagates from the test body itself. + await new Promise((resolve) => { + const grace = setTimeout(() => resolve(), 2_000) + ws.on('close', () => { + clearTimeout(grace) + resolve() + }) + }) } }) }) From 49f00cfdfd8984eafb7953390e8a3eb26d0be0fb Mon Sep 17 00:00:00 2001 From: Dan Shapiro <3732858+danshapiro@users.noreply.github.com> Date: Thu, 27 Aug 2026 14:57:55 -0700 Subject: [PATCH 11/15] fix(port): rebuild stampless node dists, prove e2e match-path convergence, sync plan to as-built --- crates/freshell-ws/src/lib.rs | 7 +++++-- .../plans/2026-08-27-server-version-reload.md | 15 +++++++++++-- port/oracle/harness/external-server.ts | 21 ++++++++++++------- .../specs/server-build-mismatch-rust.spec.ts | 16 ++++++++++++++ 4 files changed, 47 insertions(+), 12 deletions(-) diff --git a/crates/freshell-ws/src/lib.rs b/crates/freshell-ws/src/lib.rs index 4dbd52e2c..e78d3f6cd 100644 --- a/crates/freshell-ws/src/lib.rs +++ b/crates/freshell-ws/src/lib.rs @@ -527,8 +527,11 @@ pub async fn build_handshake(state: &WsState) -> Vec { /// [`build_handshake`], parameterized on the connection's negotiated /// `hello.capabilities.paneReconcileV1` (reconciliation design §4.2): the /// `ready.capabilities` advertisement is emitted **only when the client's -/// `hello` opted in** — today's frozen client doesn't, so the emitted -/// handshake stays byte-for-byte identical to the pinned clean-boot shape. +/// `hello` opted in** — today's frozen client doesn't, so that field stays +/// omitted for it (frozen-client inertness). The handshake overall is no +/// longer byte-for-byte identical to the pinned clean-boot shape: `ready` +/// now always stamps `buildId`, an additive change old clients ignore as +/// an unknown field. /// /// CFG-12: `settings.updated` resolves [`WsState::handshake_settings`] — the /// LIVE tree — fresh on every call (one call per `/ws` connection), matching diff --git a/docs/plans/2026-08-27-server-version-reload.md b/docs/plans/2026-08-27-server-version-reload.md index 3e9fe59b3..0c65eba12 100644 --- a/docs/plans/2026-08-27-server-version-reload.md +++ b/docs/plans/2026-08-27-server-version-reload.md @@ -484,6 +484,8 @@ Update `package.json`'s `build:server` script: 3h. Create `server/build-id.ts`: +> As-built amendment (delta review round 1): the bake path is resolved lazily — the eager form originally planned crashed module import in non-`file:` loaders (see commit 738e9346d). + ```typescript import { execFileSync } from 'node:child_process' import { readFileSync } from 'node:fs' @@ -496,7 +498,16 @@ const SHA_PATTERN = /^[0-9a-f]{40}$/ // wrote dist/server/build-id.json), or next to server/build-id.ts in // tsx-from-source runs (where no bake file exists and the runtime probe is // correct because dev runs current source). -const DEFAULT_BAKE_PATH = fileURLToPath(new URL('build-id.json', import.meta.url)) +function defaultBakePath(): string { + try { + return fileURLToPath(new URL('build-id.json', import.meta.url)) + } catch { + // Non-file: import.meta.url (electron-style loaders): a relative path + // that readFileSync will miss, degrading to the inert "unknown" (compiled) + // or the runtime git probe (source) — never an import crash. + return 'build-id.json' + } +} /** * The git commit the server runs from — the SAME identity the Rust server @@ -553,7 +564,7 @@ const SOURCE_MODE = import.meta.url.endsWith('.ts') * which is correct because they execute current source. */ export function resolveServerBuildId( - bakePath: string = DEFAULT_BAKE_PATH, + bakePath: string = defaultBakePath(), opts?: { sourceMode?: boolean }, ): string { const sourceMode = opts?.sourceMode ?? SOURCE_MODE diff --git a/port/oracle/harness/external-server.ts b/port/oracle/harness/external-server.ts index fc305d2d1..f0df44b53 100644 --- a/port/oracle/harness/external-server.ts +++ b/port/oracle/harness/external-server.ts @@ -112,17 +112,22 @@ export function rustServerBinPath(root: string = PROJECT_ROOT): string { /** * Whether the node dist's baked build stamp (written by `build:server`'s * `scripts/bake-server-build-id.mjs`) matches the CURRENT checkout HEAD. - * True when no bake file exists (pre-stamp dist or git-less build — keep - * the legacy exists-only behavior), when git is unavailable, or when the - * stamp is unreadable: those cases have no stamp semantics to violate. - * False only for a REAL staleness — a bake from an earlier HEAD — which - * must trigger a rebuild so the oracle's node-vs-rust `buildId` comparison - * compares same-HEAD artifacts, never a stale checkout against a fresh - * cargo build. + * False when NO bake file exists: post-feature `build:server` ALWAYS writes + * the bake file (including `"unknown"` for git-less builds), so absence + * means a pre-feature or raw-`tsc` artifact whose ready frame may omit + * `buildId` entirely — reusing it would make the oracle's raw `buildId` + * comparison fail, so it must trigger a rebuild. True only for leniency + * cases that have no comparable stamp semantics to violate: the file + * EXISTS but is malformed/unreadable (defensive — the writer is atomic), + * the stamp is `"unknown"` (a git-less build; a rebuild writes the same), + * or git is unavailable. False too for a REAL staleness — a bake from an + * earlier HEAD — which must trigger a rebuild so the oracle's + * node-vs-rust `buildId` comparison compares same-HEAD artifacts, never a + * stale checkout against a fresh cargo build. */ function nodeBuildStampIsCurrent(root: string): boolean { const bakePath = path.join(root, 'dist', 'server', 'build-id.json') - if (!fs.existsSync(bakePath)) return true + if (!fs.existsSync(bakePath)) return false try { const baked = (JSON.parse(fs.readFileSync(bakePath, 'utf8')) as { buildId?: unknown }).buildId if (typeof baked !== 'string' || baked === 'unknown') return true diff --git a/test/e2e-browser/specs/server-build-mismatch-rust.spec.ts b/test/e2e-browser/specs/server-build-mismatch-rust.spec.ts index 63827928e..f069c7d30 100644 --- a/test/e2e-browser/specs/server-build-mismatch-rust.spec.ts +++ b/test/e2e-browser/specs/server-build-mismatch-rust.spec.ts @@ -87,6 +87,22 @@ test.describe('server build mismatch reload (rust)', () => { await rebooted.waitForHarness() await rebooted.waitForConnection() + // The real post-reload ready must MATCH: in normal e2e runs the harness + // guarantees same-HEAD artifacts — global setup fresh-builds both sides + // (test/e2e-browser/global-setup.ts runs `npm run build:client && npm run + // build:server` at run start) and `ensureRustServerBuilt` restamps the + // Rust binary on HEAD moves — so the real `ready.buildId` equals the + // client's baked `__FRESHELL_BUILD_ID__` and the production match path + // MUST have cleared the sentinel. A failure here means the real ready + // did not MATCH — a genuine cross-artifact stamping regression, not a + // suppression artifact. (Known caveat: a stale dist from a non-harness + // flow will fail this assertion loudly, which is the feature working as + // designed.) + expect( + await page.evaluate((key) => sessionStorage.getItem(key), SENTINEL), + 'real post-reload ready must MATCH and clear the sentinel (same-HEAD harness guarantee)', + ).toBeNull() + await context.close() }) From 37d7d8738efa50e493293d8a0c4d554b26968358 Mon Sep 17 00:00:00 2001 From: Dan Shapiro <3732858+danshapiro@users.noreply.github.com> Date: Thu, 27 Aug 2026 15:25:28 -0700 Subject: [PATCH 12/15] fix(port): strict oracle stamp freshness, race-free e2e persistence read, archival plan banner --- .../plans/2026-08-27-server-version-reload.md | 54 +++++++++------ port/oracle/harness/external-server.ts | 66 ++++++++++++++----- .../specs/server-build-mismatch-rust.spec.ts | 22 +++++-- .../port/oracle-harness-freshness.test.ts | 64 ++++++++++++++++++ 4 files changed, 162 insertions(+), 44 deletions(-) create mode 100644 test/unit/port/oracle-harness-freshness.test.ts diff --git a/docs/plans/2026-08-27-server-version-reload.md b/docs/plans/2026-08-27-server-version-reload.md index 0c65eba12..8a82b4c58 100644 --- a/docs/plans/2026-08-27-server-version-reload.md +++ b/docs/plans/2026-08-27-server-version-reload.md @@ -1,5 +1,17 @@ # Server-Build Mismatch Auto-Reload Implementation Plan +> **STATUS: IMPLEMENTED (2026-08-27) — ARCHIVAL DOCUMENT. DO NOT EXECUTE.** +> Every task below is complete: feature commits `f137951d6` (protocol + both +> servers), `2ae0dd136` (client), `f9ac736c8` (e2e + docs), plus gate-driven +> fixes `738e9346d`, `3e5d5de20`, `49f00cfdfd8`. Steps were checked off during +> execution; the durable execution record lives in +> `.git/worktrees/server-version-reload/usual-sdd/progress.md` and +> `/home/dan/code/freshell/.worktrees/.the-usual-logs/server-version-reload/`. +> Two as-built amendments diverge from the original text and are marked +> inline: the lazy `defaultBakePath()` (Step 3h) and the e2e match-path +> assertion (Task 3). The document is retained as the authoritative spec of +> what was built. + > **For agentic workers:** Execute this plan task by task with a fresh > implementer and a specification-plus-quality review after every task. Track > progress with the checkbox steps below. @@ -50,7 +62,7 @@ - Consumes: nothing new — `crates/freshell-server/build.rs` and `diag.rs:124` are untouched (freshell-ws now bakes its own constant; both crates compile at the same HEAD in every workspace build, so the values agree). - Produces: `freshell_protocol::Ready { build_id: Option }` (serde camelCase → wire key `buildId`, skipped when `None`); `freshell_ws::ready_build_id() -> Option` (the crate-baked sha or `"unknown"`, always `Some` in practice); `server/build-id.ts` exporting `computeBuildId(cwd?: string): string` (pure git probe), `readBakedBuildId(bakePath: string): string | undefined` (pure file read), `resolveServerBuildId(bakePath?: string): string` (bake-wins-else-probe), `serverBuildId(): string` (cached), `_resetServerBuildIdCacheForTests(): void`; `dist/server/build-id.json` (`{"buildId": ""}`) written by `build:server`; TS `ReadyMessage.buildId?: string`; regenerated `port/contract/ws-server-messages.schema.json` with an optional `buildId` on `ready` (still `additionalProperties: false`). Task 2's client schema and Task 3's e2e injection consume the wire key `buildId`. -- [ ] **Step 1: Write the failing behavioral tests (protocol roundtrip, rust wire, node module, node wire)** +- [x] **Step 1: Write the failing behavioral tests (protocol roundtrip, rust wire, node module, node wire)** 1a. Add to `crates/freshell-protocol/tests/roundtrip.rs` immediately after the `ready_carries_server_instance_id_and_boot_id` test (line 164): @@ -271,7 +283,7 @@ describe('server build id', () => { }) ``` -- [ ] **Step 2: Run the tests and verify the intended failures** +- [x] **Step 2: Run the tests and verify the intended failures** ```bash cargo test -p freshell-protocol --test roundtrip ready_carries_build_id_and_omits_it_when_absent @@ -281,7 +293,7 @@ npm run test:vitest -- run test/server/build-id.test.ts test/server/ws-handshake Expected: all FAIL for the missing behavior — the Rust roundtrip test fails to COMPILE (`no field \`build_id\` on struct Ready`); the freshell-ws wire test COMPILES (it references no new symbols) and fails its first assertion (`ready must stamp buildId` — the ready frame carries no `buildId`); `build-id.test.ts` fails to resolve `../../server/build-id.js` (module missing); the new snapshot test fails on `expect(typeof ready1.buildId).toBe('string')`. -- [ ] **Step 3: Add the minimal production implementation** +- [x] **Step 3: Add the minimal production implementation** 3a. `shared/ws-protocol.ts` — in `ReadyMessage` (lines 743-750), add after `bootId`: @@ -657,7 +669,7 @@ to if (fs.existsSync(entry) && nodeBuildStampIsCurrent(root)) return entry ``` -- [ ] **Step 4: Run the focused tests** +- [x] **Step 4: Run the focused tests** ```bash cargo test -p freshell-protocol --test roundtrip ready_carries_build_id_and_omits_it_when_absent @@ -667,11 +679,11 @@ npm run test:vitest -- run test/server/build-id.test.ts test/server/ws-handshake Expected: all PASS. -- [ ] **Step 5: Refactor while green** +- [x] **Step 5: Refactor while green** No refactor needed — the Rust stamp mirrors the adjacent `boot_id` idiom, and the Node stamp mirrors `bootId`'s always-stamped treatment. Do NOT regenerate `port/oracle/fixtures/handshake-transcript.json`: the frozen transcript stays byte-valid because Rust omits `build_id` when deserialized as `None`, and the mutation/oracle suites consume the regenerated SCHEMA (not the live node bytes) for conformance. -- [ ] **Step 6: Run impacted-test verification** +- [x] **Step 6: Run impacted-test verification** This change touches the shared wire protocol, both server implementations, the generated schema, and the `build:server` pipeline, so the impacted set is: both Rust crates' full test trees, the workspace compile, the whole server-config suite (any test asserting handshake/ready shapes), the port contract suites, and the port-ORACLE suites. **`npm run test:port` does NOT run the oracle suites** (`vitest.port.config.ts` excludes `test/unit/port/oracle/**`; they run only via `npm run test:oracle`, which boots real servers — budget several minutes). Notes: @@ -691,7 +703,7 @@ npm run test:oracle Expected: all PASS, and `dist/server/build-id.json` contains the current worktree HEAD sha. -- [ ] **Step 7: Commit the task** +- [x] **Step 7: Commit the task** Stage by directory so every compiler-enumerated fix lands in the commit (the worktree starts clean; verify nothing unexpected is staged): @@ -720,7 +732,7 @@ Expected: the first `git status --short` lists exactly the Task 1 files (all und - Consumes: Task 1's wire contract (`ReadyMessage.buildId?: string`, parsed by `ReadyMessageSchema`). - Produces: `checkServerBuildId(options?: ServerBuildCheckOptions): void` from `@/lib/server-build-check`, with `ServerBuildCheckOptions { clientBuildId?: string; serverBuildId?: string; reload?: () => void; storage?: Pick }`; `__FRESHELL_BUILD_ID__: string` available client-side at build time. Task 3's e2e exercises the production wiring end to end. -- [ ] **Step 1: Write the failing behavioral tests** +- [x] **Step 1: Write the failing behavioral tests** 1a. Create `test/unit/client/lib/server-build-check.test.ts`: @@ -986,7 +998,7 @@ describe('App ready buildId → one-shot server-build reload', () => { }) ``` -- [ ] **Step 2: Run the tests and verify the intended failures** +- [x] **Step 2: Run the tests and verify the intended failures** ```bash npm run test:vitest -- run test/unit/client/lib/server-build-check.test.ts test/unit/client/components/App.restart-signals.test.tsx @@ -994,7 +1006,7 @@ npm run test:vitest -- run test/unit/client/lib/server-build-check.test.ts test/ Expected: FAIL — `server-build-check.test.ts` cannot resolve `@/lib/server-build-check` (module missing), and the App tests fail because a ready with `buildId` triggers no reload (`expect(reloadCalls).toBe(1)` sees 0). -- [ ] **Step 3: Add the minimal production implementation** +- [x] **Step 3: Add the minimal production implementation** 3a. Create `src/lib/server-build-check.ts`: @@ -1183,7 +1195,7 @@ Add the call inside the `else` (ready-success) branch, immediately after the `if checkServerBuildId({ serverBuildId: ready.data.buildId }) ``` -- [ ] **Step 4: Run the focused tests** +- [x] **Step 4: Run the focused tests** ```bash npm run test:vitest -- run test/unit/client/lib/server-build-check.test.ts test/unit/client/components/App.restart-signals.test.tsx @@ -1191,7 +1203,7 @@ npm run test:vitest -- run test/unit/client/lib/server-build-check.test.ts test/ Expected: PASS. -- [ ] **Step 5: Refactor while green** +- [x] **Step 5: Refactor while green** Verify the Vite define actually bakes the sha into the bundle (explicit pass/fail so automation cannot swallow a failed match through a pipe): @@ -1202,7 +1214,7 @@ rg -q "$(git rev-parse HEAD)" dist/client/assets/*.js && echo "BAKE OK: sha pres Expected: `BAKE OK: sha present in bundle` — the command exits 0. A missing bake prints `BAKE MISSING` and exits NONZERO (the failure branch must not mask the failure behind a successful `echo`). (`npm run build:client` from this worktree writes the worktree's own `dist/client` — the main-checkout `npm run build` production-server guard does not apply here.) -- [ ] **Step 6: Run impacted-test verification** +- [x] **Step 6: Run impacted-test verification** `ReadyMessageSchema` and App's ready handling are shared client-critical paths and the define constant touches the whole client build; the impacted set is the client unit suite plus typecheck and lint: @@ -1214,7 +1226,7 @@ npm run test:vitest -- run test/unit/client Expected: all PASS. -- [ ] **Step 7: Commit the task** +- [x] **Step 7: Commit the task** ```bash git add src/lib/server-build-check.ts config/vite/vite.config.ts src/vite-env.d.ts src/App.tsx test/unit/client/lib/server-build-check.test.ts test/unit/client/components/App.restart-signals.test.tsx @@ -1235,7 +1247,7 @@ git commit -m "feat(client): reload once when ready.buildId differs from the bak - Consumes: Tasks 1-2 (both servers stamp `ready.buildId`; the client compares and reloads once; `TestHarness.receiveWsMessage` → `ws.receiveMessageForTest` → `handleIncomingMessage` feeds an injected frame through the real App ready handler — verified at `src/lib/ws-client.ts:917-919`). - Produces: the user-outcome proof on the LOCAL lane — a stale client against a newer server reboots itself exactly once and converges to a healthy ready connection; sessionStorage persistence across a REAL navigation; repeat mismatches suppressed by the sentinel. -- [ ] **Step 1: Write the failing behavioral test** +- [x] **Step 1: Write the failing behavioral test** Create `test/e2e-browser/specs/server-build-mismatch-rust.spec.ts`: @@ -1421,7 +1433,7 @@ In `AGENTS.md`, under "Key Architectural Patterns", append to the **WebSocket Pr The `ready` frame carries an optional additive `buildId` (the server's artifact-time-baked git commit, `"unknown"` fallback): the client bakes its own at Vite build time (`__FRESHELL_BUILD_ID__`) and, on a mismatch, reloads exactly once per tab session (sessionStorage sentinel `freshell.server-build-reload`), self-healing stale-client contract errors; `"unknown"` on either side never triggers or clears the guard (`src/lib/server-build-check.ts`). The once-guard is per server identity: an origin fronted by mixed-build servers could oscillate, and a newer client against an older server costs one futile bounded reload per fresh tab session (both accepted for the single-server self-hosted model). ``` -- [ ] **Step 2: Run the test and verify it passes, then RED-VERIFY it exercises the feature** +- [x] **Step 2: Run the test and verify it passes, then RED-VERIFY it exercises the feature** Build the client fresh first so the served bundle provably contains the feature (the red-verification's validity depends on it): @@ -1455,19 +1467,19 @@ npx playwright test --config test/e2e-browser/playwright.config.ts --project=rus Expected: PASS. (Record all three runs in the task review — the red-verification is mandatory.) -- [ ] **Step 3: No production implementation step** +- [x] **Step 3: No production implementation step** Tasks 1-2 implemented the behavior; this task only proves it end to end. -- [ ] **Step 4: Run the focused test** +- [x] **Step 4: Run the focused test** Same command as Step 2's final run. Expected: PASS. -- [ ] **Step 5: Refactor while green** +- [x] **Step 5: Refactor while green** No refactor needed. Confirm the registration mechanics: excluded from the match-all `chromium` project by the `RUST_ONLY_SPECS` entry (`testIgnore: RUST_ONLY_SPECS` at `playwright.config.ts:330`), included in `rust-chromium`'s `testMatch`, and skipped on the cloud lane by the `CLOUD_SKIP_SPECS` entry with its justification comment. -- [ ] **Step 6: Run impacted-test verification** +- [x] **Step 6: Run impacted-test verification** Playwright registration changed (a new rust-only spec) and AGENTS.md was touched; the impacted set is the rust-chromium self-test that boots a real Rust server through its own fixture (proving the registration change disturbed nothing — note `continuity-smoke.spec.ts` runs ONLY under its own conditional `continuity-smoke` project, NOT under `rust-chromium`, so it must not be used as the neighbor here) plus the two unit files most adjacent to the feature: @@ -1478,7 +1490,7 @@ npm run test:vitest -- run test/unit/client/lib/server-build-check.test.ts test/ Expected: all PASS. Backend note: the repo rule about the configured `FRESHELL_E2E_BACKEND` is honored at execution kickoff — the user chooses local vs cloud once, INFORMED that this spec is cloud-incompatible by construction (the cloud image builds without git metadata, so both stamps are `"unknown"` and the compare is inert there). Regardless of the choice, this spec's coverage lane is the LOCAL rust-chromium project and it is CLOUD_SKIP'd with that justification (`playwright.cloud.config.ts`); if the user chooses cloud, the PR description documents the skip explicitly so no coverage claim is silent. No cloud claim is made about cargo (the cloud runtime uses a prebuilt binary; cargo never runs there per `rust-server.ts:82-90`). -- [ ] **Step 7: Commit the task** +- [x] **Step 7: Commit the task** ```bash git add test/e2e-browser/specs/server-build-mismatch-rust.spec.ts test/e2e-browser/playwright.config.ts test/e2e-browser/playwright.cloud.config.ts AGENTS.md diff --git a/port/oracle/harness/external-server.ts b/port/oracle/harness/external-server.ts index f0df44b53..ae0f4cca8 100644 --- a/port/oracle/harness/external-server.ts +++ b/port/oracle/harness/external-server.ts @@ -109,33 +109,63 @@ export function rustServerBinPath(root: string = PROJECT_ROOT): string { return path.join(root, 'target', 'release', 'freshell-server') } +/** + * Injectable knobs for `nodeBuildStampIsCurrent` — production passes nothing + * (the HEAD probe spawns git); tests pass an explicit `head` or force the + * git-less path so they never depend on the scratch root being a git repo. + */ +export interface NodeStampFreshnessOptions { + /** Explicit HEAD sha to compare against; skips the git probe when given. */ + head?: string + /** Force the git-less environment path (legacy reuse) when false. */ + gitAvailable?: boolean +} + /** * Whether the node dist's baked build stamp (written by `build:server`'s * `scripts/bake-server-build-id.mjs`) matches the CURRENT checkout HEAD. - * False when NO bake file exists: post-feature `build:server` ALWAYS writes - * the bake file (including `"unknown"` for git-less builds), so absence - * means a pre-feature or raw-`tsc` artifact whose ready frame may omit - * `buildId` entirely — reusing it would make the oracle's raw `buildId` - * comparison fail, so it must trigger a rebuild. True only for leniency - * cases that have no comparable stamp semantics to violate: the file - * EXISTS but is malformed/unreadable (defensive — the writer is atomic), - * the stamp is `"unknown"` (a git-less build; a rebuild writes the same), - * or git is unavailable. False too for a REAL staleness — a bake from an - * earlier HEAD — which must trigger a rebuild so the oracle's - * node-vs-rust `buildId` comparison compares same-HEAD artifacts, never a - * stale checkout against a fresh cargo build. + * + * STRICT when HEAD is available: ONLY an exact match counts as current. + * False — rebuild — when NO bake file exists (post-feature `build:server` + * ALWAYS writes the bake file, so absence means a pre-feature or raw-`tsc` + * artifact whose ready frame may omit `buildId` entirely; delta review + * round 1), when the file is malformed/unreadable (defensive — the writer + * is atomic), when the stamp is `"unknown"` (a git-less build), or when the + * stamp is any real-but-different sha. The strictness keeps the oracle from + * ever comparing a git-less/stale node artifact against a fresh cargo-built + * rust binary: a dist built WITHOUT git and reused in this git-full checkout + * advertises `"unknown"` while the rust bake is a fresh sha, which would be + * a spurious oracle failure, not a divergence (delta review round 2). + * + * LENIENT only when HEAD is unavailable — the git probe fails (spawn status + * non-zero, e.g. a git-less environment) or `options.gitAvailable === false`: + * there are no stamp semantics to violate, so the legacy reuse applies. */ -function nodeBuildStampIsCurrent(root: string): boolean { +export function nodeBuildStampIsCurrent( + root: string, + options: NodeStampFreshnessOptions = {}, +): boolean { + let head: string | undefined = options.head + if (options.gitAvailable === false) return true + if (head === undefined) { + const probe = spawnSync('git', ['rev-parse', 'HEAD'], { cwd: root, encoding: 'utf8' }) + // Git-less environment: no stamp semantics to violate, keep legacy reuse. + if (probe.status !== 0) return true + head = probe.stdout.trim() + } + // Missing bake = pre-feature or raw-tsc artifact: its ready frame may omit + // buildId entirely — rebuild (delta review round 1). const bakePath = path.join(root, 'dist', 'server', 'build-id.json') if (!fs.existsSync(bakePath)) return false try { const baked = (JSON.parse(fs.readFileSync(bakePath, 'utf8')) as { buildId?: unknown }).buildId - if (typeof baked !== 'string' || baked === 'unknown') return true - const head = spawnSync('git', ['rev-parse', 'HEAD'], { cwd: root, encoding: 'utf8' }) - if (head.status !== 0) return true - return baked === head.stdout.trim() + // ONLY an exact match with the current HEAD counts as current: + // "unknown", malformed, and mismatched stamps all trigger a rebuild so + // the oracle never compares a git-less/stale node artifact against a + // fresh cargo-built rust binary (delta review round 2). + return typeof baked === 'string' && baked === head } catch { - return true + return false } } diff --git a/test/e2e-browser/specs/server-build-mismatch-rust.spec.ts b/test/e2e-browser/specs/server-build-mismatch-rust.spec.ts index f069c7d30..9ac2754ec 100644 --- a/test/e2e-browser/specs/server-build-mismatch-rust.spec.ts +++ b/test/e2e-browser/specs/server-build-mismatch-rust.spec.ts @@ -21,8 +21,9 @@ * deterministic here: after any reload the boot's REAL ready either matches * (same-HEAD artifacts → legitimately clears the sentinel) or mismatches * (stale-bake environments → keeps it), so the post-reload sentinel state - * is environment-dependent — hence the persistence test reads at commit - * time and the suppression test seeds its state AFTER the boot settles. + * is environment-dependent — hence the persistence test snapshots the + * sentinel at DOCUMENT CREATION (an init script runs before page scripts) + * and the suppression test seeds its state AFTER the boot settles. * Seeding is state setup, the same practice as seeding localStorage in * other suites; the PERSISTENCE and SUPPRESSION behavior exercised is * entirely production code. @@ -119,10 +120,21 @@ test.describe('server build mismatch reload (rust)', () => { await page.evaluate((key) => sessionStorage.setItem(key, '1'), SENTINEL) // A REAL navigation: sessionStorage must survive it (per-tab, per-origin - // storage) — read at commit time, BEFORE the rebooted app's real ready - // can legitimately match-and-clear it (same-HEAD artifacts match). + // storage). The value is snapshotted at DOCUMENT CREATION — an init + // script runs before page scripts — so it is immune to the rebooted + // app's later match-and-clear (same-HEAD artifacts legitimately clear + // the sentinel after the real ready): the reload's `commit` event only + // guarantees the document exists, and by the time the new app has + // received the real (matching) ready it may ALREADY have cleared the + // sentinel before a late `page.evaluate` could read it. `null` would + // mean absent at document start; `'1'` means persisted. + await page.addInitScript((key) => { + ;(window as any).__sentinelAtDocumentStart = window.sessionStorage.getItem(key) + }, SENTINEL) await page.reload({ waitUntil: 'commit' }) - const persisted = await page.evaluate((key) => sessionStorage.getItem(key), SENTINEL) + const persisted = await page + .waitForFunction(() => (window as any).__sentinelAtDocumentStart !== undefined) + .then(() => page.evaluate(() => (window as any).__sentinelAtDocumentStart)) expect(persisted, 'sentinel must survive a real navigation').toBe('1') await context.close() diff --git a/test/unit/port/oracle-harness-freshness.test.ts b/test/unit/port/oracle-harness-freshness.test.ts new file mode 100644 index 000000000..44313f1cb --- /dev/null +++ b/test/unit/port/oracle-harness-freshness.test.ts @@ -0,0 +1,64 @@ +import * as fs from 'node:fs' +import * as os from 'node:os' +import * as path from 'node:path' +import { afterAll, beforeEach, describe, expect, it } from 'vitest' +import { nodeBuildStampIsCurrent } from '../../../port/oracle/harness/external-server.js' + +// Pure-logic coverage for the oracle node dist's stamp-freshness guard: the +// predicate decides whether `ensureServerBuilt` reuses or rebuilds the +// compiled node artifact before the oracle boots it. +// +// The predicate's cwd (git probe) is injectable: tests pass an explicit +// `head` (or force the git-less path via `gitAvailable: false`) so no test +// depends on this scratch root actually being a git worktree. +describe('nodeBuildStampIsCurrent', () => { + const dirs: string[] = [] + + function scratchDist(buildId: unknown): string { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'stamp-freshness-')) + dirs.push(root) + fs.mkdirSync(path.join(root, 'dist', 'server'), { recursive: true }) + if (buildId !== undefined) { + const content = typeof buildId === 'string' ? JSON.stringify({ buildId }) : buildId + fs.writeFileSync(path.join(root, 'dist', 'server', 'build-id.json'), content) + } + fs.writeFileSync(path.join(root, 'dist', 'server', 'index.js'), '// entry') + return root + } + + beforeEach(() => { + /* fresh dirs per test */ + }) + afterAll(() => { + for (const d of dirs) fs.rmSync(d, { recursive: true, force: true }) + }) + + it('is current when the stamp exactly equals the checkout HEAD', () => { + const root = scratchDist('a'.repeat(40)) + expect(nodeBuildStampIsCurrent(root, { head: 'a'.repeat(40) })).toBe(true) + }) + + it('rebuilds when the stamp is a different sha', () => { + const root = scratchDist('f'.repeat(40)) + expect(nodeBuildStampIsCurrent(root, { head: 'a'.repeat(40) })).toBe(false) + }) + + it('rebuilds when the bake file is missing', () => { + const root = scratchDist(undefined) + expect(nodeBuildStampIsCurrent(root, { head: 'a'.repeat(40) })).toBe(false) + }) + + it('rebuilds when the stamp is "unknown" or malformed but HEAD is available', () => { + const unknownRoot = scratchDist('unknown') + expect(nodeBuildStampIsCurrent(unknownRoot, { head: 'a'.repeat(40) })).toBe(false) + const malformedRoot = scratchDist('not json {') + expect(nodeBuildStampIsCurrent(malformedRoot, { head: 'a'.repeat(40) })).toBe(false) + }) + + it('keeps legacy reuse when HEAD is unavailable (no stamp semantics to violate)', () => { + const unknownRoot = scratchDist('unknown') + expect(nodeBuildStampIsCurrent(unknownRoot, { gitAvailable: false })).toBe(true) + const staleRoot = scratchDist('f'.repeat(40)) + expect(nodeBuildStampIsCurrent(staleRoot, { gitAvailable: false })).toBe(true) + }) +}) From 92c7664105d2cee292d6dcaa5c972c137f3635e0 Mon Sep 17 00:00:00 2001 From: Dan Shapiro <3732858+danshapiro@users.noreply.github.com> Date: Thu, 27 Aug 2026 15:59:31 -0700 Subject: [PATCH 13/15] fix(client): sentinel records the attempted server build id so corrected deployments re-arm the guard --- AGENTS.md | 2 +- .../plans/2026-08-27-server-version-reload.md | 130 ++++++++++++------ src/lib/server-build-check.ts | 27 ++-- .../specs/server-build-mismatch-rust.spec.ts | 24 ++-- .../components/App.restart-signals.test.tsx | 13 +- .../client/lib/server-build-check.test.ts | 43 ++++-- 6 files changed, 159 insertions(+), 80 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 8dda134e0..546347515 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -224,7 +224,7 @@ live in [docs/development/gcloud-robot.md](docs/development/gcloud-robot.md). ### Key Architectural Patterns -**WebSocket Protocol:** Schema-validated messages using Zod. Handshake flow: client sends `hello` with token → server validates → sends `ready`. Message types include `terminal.create/input/resize/detach/attach` and broadcasts like `sessions.updated`. The `ready` frame carries an optional additive `buildId` (the server's artifact-time-baked git commit, `"unknown"` fallback): the client bakes its own at Vite build time (`__FRESHELL_BUILD_ID__`) and, on a mismatch, reloads exactly once per tab session (sessionStorage sentinel `freshell.server-build-reload`), self-healing stale-client contract errors; `"unknown"` on either side never triggers or clears the guard (`src/lib/server-build-check.ts`). The once-guard is per server identity: an origin fronted by mixed-build servers could oscillate, and a newer client against an older server costs one futile bounded reload per fresh tab session (both accepted for the single-server self-hosted model). +**WebSocket Protocol:** Schema-validated messages using Zod. Handshake flow: client sends `hello` with token → server validates → sends `ready`. Message types include `terminal.create/input/resize/detach/attach` and broadcasts like `sessions.updated`. The `ready` frame carries an optional additive `buildId` (the server's artifact-time-baked git commit, `"unknown"` fallback): the client bakes its own at Vite build time (`__FRESHELL_BUILD_ID__`) and, on a mismatch, reloads exactly once per tab session (sessionStorage sentinel `freshell.server-build-reload` records the last attempted server build id; the same id never reloads twice, a different (corrected) deployment re-arms the guard), self-healing stale-client contract errors; `"unknown"` on either side never triggers or clears the guard (`src/lib/server-build-check.ts`). The once-guard is per server identity: an origin fronted by mixed-build servers could oscillate, and a newer client against an older server costs one futile bounded reload per fresh tab session (both accepted for the single-server self-hosted model). **PTY Lifecycle:** Each terminal has a unique ID. Server maintains 64KB scrollback buffer. On attach, client receives buffer snapshot then streams new output. On detach, process continues running (background session). Configurable idle timeout (15 mins default). diff --git a/docs/plans/2026-08-27-server-version-reload.md b/docs/plans/2026-08-27-server-version-reload.md index 8a82b4c58..24ddd1e76 100644 --- a/docs/plans/2026-08-27-server-version-reload.md +++ b/docs/plans/2026-08-27-server-version-reload.md @@ -9,12 +9,18 @@ > `/home/dan/code/freshell/.worktrees/.the-usual-logs/server-version-reload/`. > Two as-built amendments diverge from the original text and are marked > inline: the lazy `defaultBakePath()` (Step 3h) and the e2e match-path -> assertion (Task 3). The document is retained as the authoritative spec of -> what was built. - -> **For agentic workers:** Execute this plan task by task with a fresh -> implementer and a specification-plus-quality review after every task. Track -> progress with the checkbox steps below. +> assertion (Task 3). A third as-built amendment (delta review round 3) — +> the reload sentinel records the attempted server build id instead of the +> literal `"1"` — is marked inline at the Global Constraints loop-guard +> bullet and the Task 2 listings. The document is retained as the +> authoritative spec of what was built. + +> **For agentic workers:** This plan has been fully executed and is retained +> as the authoritative specification of what was built. Do not re-execute it. +> Progress, reviews, and verification evidence: the `usual-sdd` ledger in the +> worktree's git directory and the run logs under +> `/home/dan/code/freshell/.worktrees/.the-usual-logs/server-version-reload/`. +> Track historical progress with the (completed) checkbox steps below. **Goal:** When a browser tab connects (or reconnects) to a Freshell server built from a different commit than the client bundle it is running, the client detects the mismatch from the WS `ready` frame and reloads itself exactly once — self-healing the "Fresh-agent snapshot response did not match the shared contract" class of stale-client failures without ever reload-looping. @@ -30,7 +36,8 @@ - **Artifact-time semantics everywhere:** each stamp describes the artifact that emits it. Rust bakes at compile; Node's production stamp comes from the `dist/server/build-id.json` written by `build:server` (a stale dist advertises the sha it was BUILT from — never the checkout's current HEAD); tsx dev mode has no bake file next to source and probes runtime HEAD (correct: it runs current source); Vite bakes the client's sha at bundle time. - **Value semantics on every side:** the value is the full `git rev-parse HEAD` SHA of the repo at build/bake time; when git is unavailable or the output is not 40 lowercase hex chars (Node/Vite enforce the 40-hex check; the Rust scripts accept any successful output), the literal `"unknown"`. Known caveat (accepted, documented): a SHA-256 git checkout would make Rust stamp 64 hex while Node/Vite stamp `"unknown"` — the guard goes inert (no false reloads, no crash); this repo is SHA-1. - **Client compare rule:** reload iff BOTH ids are present, non-empty, neither is `"unknown"`, and they differ. `"unknown" == "unknown"` is NOT a match-and-clear (it is a no-op) — two unknown builds must never trigger a reload and must never clear an armed sentinel. The compare is direction-free: a NEWER client against an OLDER server also performs one bounded reload per fresh tab session (futile but harmless; shas carry no ordering) — documented, accepted. -- **Loop-guard invariant:** at most ONE code-triggered reload per tab session, per server identity. The sentinel key is `freshell.server-build-reload` (`sessionStorage`, value `"1"`), set BEFORE calling `reload()`. If `sessionStorage` cannot be read or written (property access throwing a SecurityError, quota errors, absent API), no reload happens and the suppression failure is logged (fail-safe with observability). A matching `ready` clears the sentinel (self-re-arm). KNOWN LIMIT (accepted, documented): one origin fronted by servers built from DIFFERENT commits could oscillate (mismatch → reload → match clears → mismatch → …); deliberately not hardened with a clears-per-session cap for the single-server self-hosted threat model. +- **Loop-guard invariant:** at most ONE code-triggered reload per tab session, per server build identity. The sentinel key is `freshell.server-build-reload` (`sessionStorage`); it records the last attempted SERVER build id and is written BEFORE calling `reload()` — the same id never reloads twice, and a different (corrected) deployment re-arms the guard (deployments change what a reload fetches). If `sessionStorage` cannot be read or written (property access throwing a SecurityError, quota errors, absent API), no reload happens and the suppression failure is logged (fail-safe with observability). A matching `ready` clears the sentinel (self-re-arm). KNOWN LIMIT (accepted, documented): one origin fronted by servers built from DIFFERENT commits could oscillate (mismatch → reload → match clears → mismatch → …); deliberately not hardened with a clears-per-session cap for the single-server self-hosted threat model. + > As-built amendment (delta review round 3): the sentinel value records the last attempted server build id (originally the literal `"1"`), making the once-guard per (tab session, server build id) — a half-deployed server B no longer suppresses a later corrected deployment C; the match-clears oscillation limit above is unchanged. - **Client module must not crash under Vitest:** the Vitest client config has no `__FRESHELL_BUILD_ID__` define, so the module must use a `typeof __FRESHELL_BUILD_ID__ === 'undefined'` guard (same precedent as `src/lib/perf-logger.ts:45` with `__PERF_LOGGING__`). - **NodeNext/ESM:** every relative import in `server/` and `shared/` uses `.js` extensions; client code uses `@/` aliases without extensions. - **Test coordination:** broad suites go through the repo coordinator (`npm run test:vitest -- run ...`); never raw `npx vitest`. Focused Rust tests use `cargo test -p ` directly. The port-ORACLE suites are NOT covered by `npm run test:port` / `npm run check` — they run only via `npm run test:oracle`. @@ -736,6 +743,8 @@ Expected: the first `git status --short` lists exactly the Task 1 files (all und 1a. Create `test/unit/client/lib/server-build-check.test.ts`: +> As-built amendment (delta review round 3): the sentinel records the attempted server build id (see the Global Constraints loop-guard amendment) — the listing below shows the as-built semantics, including the added re-arm sequence test. + ```typescript import { afterEach, describe, expect, it, vi } from 'vitest' import { checkServerBuildId } from '@/lib/server-build-check' @@ -758,31 +767,50 @@ describe('checkServerBuildId', () => { vi.restoreAllMocks() }) - it('reloads once, arming the sentinel BEFORE the reload fires, and the sentinel suppresses a second mismatch', () => { + it('reloads once, recording the attempted server build id in the sentinel BEFORE the reload fires', () => { const storage = mapStorage() const reload = vi.fn(() => { // Ordering proof: production must persist the sentinel BEFORE // calling reload — an implementation that reloads first and arms // second would lose the sentinel across the navigation. - expect(storage._map.get(SENTINEL), 'sentinel must be armed BEFORE reload fires').toBe('1') + expect(storage._map.get(SENTINEL), 'sentinel must be armed BEFORE reload fires').toBe('b'.repeat(40)) }) checkServerBuildId({ clientBuildId: 'a'.repeat(40), serverBuildId: 'b'.repeat(40), reload, storage }) expect(reload).toHaveBeenCalledTimes(1) - expect(storage._map.get(SENTINEL)).toBe('1') + expect(storage._map.get(SENTINEL)).toBe('b'.repeat(40)) }) - it('never reloads twice: an armed sentinel suppresses the reload', () => { + it('never reloads twice for the same server build id: a recorded sentinel suppresses the reload', () => { const storage = mapStorage() - storage._map.set(SENTINEL, '1') + storage._map.set(SENTINEL, 'b'.repeat(40)) const reload = vi.fn() checkServerBuildId({ clientBuildId: 'a'.repeat(40), serverBuildId: 'b'.repeat(40), reload, storage }) expect(reload).not.toHaveBeenCalled() - expect(storage._map.get(SENTINEL)).toBe('1') + expect(storage._map.get(SENTINEL)).toBe('b'.repeat(40)) }) - it('a matching ready clears the sentinel (self-re-arm)', () => { + it('re-arms for a DIFFERENT mismatched server build id: B attempts once, repeats of B suppress, C reloads again', () => { const storage = mapStorage() - storage._map.set(SENTINEL, '1') + const reload = vi.fn() + // Mismatch vs B: reload, sentinel records B. + checkServerBuildId({ clientBuildId: 'a'.repeat(40), serverBuildId: 'b'.repeat(40), reload, storage }) + expect(reload).toHaveBeenCalledTimes(1) + expect(storage._map.get(SENTINEL)).toBe('b'.repeat(40)) + // Mismatch vs B again (the half-deployed case): the same identity was + // already attempted — suppressed, no reload. + checkServerBuildId({ clientBuildId: 'a'.repeat(40), serverBuildId: 'b'.repeat(40), reload, storage }) + expect(reload).toHaveBeenCalledTimes(1) + expect(storage._map.get(SENTINEL)).toBe('b'.repeat(40)) + // A corrected deployment (C): a different server build id re-arms the + // guard — reloads again, sentinel now records C. + checkServerBuildId({ clientBuildId: 'a'.repeat(40), serverBuildId: 'c'.repeat(40), reload, storage }) + expect(reload).toHaveBeenCalledTimes(2) + expect(storage._map.get(SENTINEL)).toBe('c'.repeat(40)) + }) + + it('a matching ready clears the recorded sentinel (self-re-arm)', () => { + const storage = mapStorage() + storage._map.set(SENTINEL, 'b'.repeat(40)) const reload = vi.fn() checkServerBuildId({ clientBuildId: 'a'.repeat(40), serverBuildId: 'a'.repeat(40), reload, storage }) expect(reload).not.toHaveBeenCalled() @@ -806,13 +834,13 @@ describe('checkServerBuildId', () => { } }) - it('an armed sentinel survives an "unknown"-vs-"unknown" ready (never treated as a match)', () => { + it('a recorded sentinel survives an "unknown"-vs-"unknown" ready (never treated as a match)', () => { const storage = mapStorage() - storage._map.set(SENTINEL, '1') + storage._map.set(SENTINEL, 'b'.repeat(40)) const reload = vi.fn() checkServerBuildId({ clientBuildId: 'unknown', serverBuildId: 'unknown', reload, storage }) expect(reload).not.toHaveBeenCalled() - expect(storage._map.get(SENTINEL)).toBe('1') + expect(storage._map.get(SENTINEL)).toBe('b'.repeat(40)) }) it('does not reload when the sentinel cannot be persisted (fail-safe against reload loops)', () => { @@ -861,7 +889,7 @@ describe('checkServerBuildId', () => { checkServerBuildId({ serverBuildId: 'd'.repeat(40) }) expect(reload).toHaveBeenCalledTimes(1) - expect(sessionStorage.getItem(SENTINEL)).toBe('1') + expect(sessionStorage.getItem(SENTINEL)).toBe('d'.repeat(40)) // And with the global absent (Vitest has no define), it is a no-op. vi.unstubAllGlobals() @@ -926,8 +954,9 @@ describe('App ready buildId → one-shot server-build reload', () => { // jsdom 25's Location owns `reload` non-configurably — defineProperty on // window.location itself throws. Repo precedent (import-retry.test.ts): // window-level replacement with save/restore. The reload stub asserts - // the sentinel is armed AT CALL TIME (the ordering proof lives here - // too, against real jsdom sessionStorage) and counts invocations. + // the sentinel is armed AT CALL TIME with the attempted server build id + // (the ordering proof lives here too, against real jsdom sessionStorage) + // and counts invocations. originalLocation = window.location Object.defineProperty(window, 'location', { value: { @@ -936,7 +965,7 @@ describe('App ready buildId → one-shot server-build reload', () => { expect( sessionStorage.getItem('freshell.server-build-reload'), 'sentinel must be armed BEFORE reload fires', - ).toBe('1') + ).toBe('b'.repeat(40)) reloadCalls++ }, }, @@ -963,7 +992,7 @@ describe('App ready buildId → one-shot server-build reload', () => { sendReady({ serverInstanceId: 'srv-1', bootId: 'boot-1', buildId: 'b'.repeat(40) }) expect(reloadCalls).toBe(1) - expect(sessionStorage.getItem('freshell.server-build-reload')).toBe('1') + expect(sessionStorage.getItem('freshell.server-build-reload')).toBe('b'.repeat(40)) // The reload lands: the page reboots in the SAME tab (real jsdom // sessionStorage persists), the server is still stale, and the next @@ -974,7 +1003,9 @@ describe('App ready buildId → one-shot server-build reload', () => { it('a matching ready clears the sentinel and re-arms the guard', async () => { vi.stubGlobal('__FRESHELL_BUILD_ID__', 'a'.repeat(40)) - sessionStorage.setItem('freshell.server-build-reload', '1') + // A sentinel recorded by an earlier mismatched ready (the attempted + // server build id), as the production code would have persisted it. + sessionStorage.setItem('freshell.server-build-reload', 'b'.repeat(40)) const store = createStore() await renderApp(store) @@ -1010,6 +1041,8 @@ Expected: FAIL — `server-build-check.test.ts` cannot resolve `@/lib/server-bui 3a. Create `src/lib/server-build-check.ts`: +> As-built amendment (delta review round 3): the sentinel records the attempted server build id — see the Global Constraints loop-guard amendment; the listing below is the as-built module. + ```typescript import { createLogger } from '@/lib/client-logger' @@ -1057,15 +1090,18 @@ function defaultStorage(): Pick | * reload ONCE on a real mismatch. Invariants: * - reload iff BOTH ids are present, non-empty, neither is "unknown", and * they differ ("unknown" == "unknown" is a no-op, never a match-and-clear); - * - the sessionStorage sentinel is set BEFORE reloading and suppresses any - * further reloads this tab session (a half-deployed server can never - * reload-loop; any sessionStorage failure = no reload, logged, fail-safe); + * - the sessionStorage sentinel records the ATTEMPTED server build id and is + * written BEFORE reloading: the same server build id never reloads twice + * this tab session (a half-deployed server can never reload-loop), while a + * DIFFERENT mismatched id re-arms the guard — a corrected deployment + * changes what a reload fetches, so it must stay reachable; any + * sessionStorage failure = no reload, logged, fail-safe; * - a MATCHING ready clears the sentinel (self-re-arm after convergence). * KNOWN LIMITS (accepted for the self-hosted single-server threat model): - * - the "once" guarantee is per server identity — one origin fronted by - * servers built from DIFFERENT commits can oscillate (mismatch → reload → - * match clears → mismatch → …). Not hardened with a clears-per-session - * cap; revisit only if a split-deploy origin appears. + * - the mixed-build-origin oscillation door stays open through match-clears: + * one origin fronted by servers built from DIFFERENT commits can oscillate + * (mismatch → reload → match clears → mismatch → …). Not hardened with a + * clears-per-session cap; revisit only if a split-deploy origin appears. * - the compare is direction-free (shas carry no ordering), so a NEWER * client against an OLDER server performs one futile bounded reload per * fresh tab session. @@ -1098,21 +1134,23 @@ export function checkServerBuildId(options?: ServerBuildCheckOptions): void { return } try { - if (storage.getItem(SERVER_BUILD_RELOAD_SENTINEL) === '1') { + if (storage.getItem(SERVER_BUILD_RELOAD_SENTINEL) === serverBuildId) { log.warn( `server build ${serverBuildId} still differs from client build ${clientBuildId}; ` - + 'one reload already attempted this tab session — suppressing further reloads', + + `a reload for build ${serverBuildId} was already attempted this tab session — ` + + 'suppressing further reloads for it', ) return } - storage.setItem(SERVER_BUILD_RELOAD_SENTINEL, '1') + storage.setItem(SERVER_BUILD_RELOAD_SENTINEL, serverBuildId) } catch (err) { log.warn('server-build sentinel persistence failed; suppressing the reload', err) return } log.warn( `server build ${serverBuildId} differs from client build ${clientBuildId}; ` - + 'reloading once to pick up the matching client bundle', + + `reloading once for build ${serverBuildId} to pick up the matching client bundle ` + + '(a different server build id will re-arm this guard)', ) reload() } @@ -1251,6 +1289,8 @@ git commit -m "feat(client): reload once when ready.buildId differs from the bak Create `test/e2e-browser/specs/server-build-mismatch-rust.spec.ts`: +> As-built amendment (delta review round 3): the seeded sentinel value is `MISMATCHED_BUILD_ID` (the attempted server build id the injected mismatch presents), not the literal `"1"` — see the Global Constraints loop-guard amendment. (The listing below otherwise reflects the original text; the as-built spec's match-path assertion and init-script persistence read are the amendment declared in the banner.) + ```typescript /** * Server-build mismatch auto-reload (the-usual/server-version-reload). @@ -1258,7 +1298,8 @@ Create `test/e2e-browser/specs/server-build-mismatch-rust.spec.ts`: * The user story: a tab running a client bundle built at commit A connects * to a server built at commit B; the server's `ready.buildId` differs from * the client's baked `__FRESHELL_BUILD_ID__`; the client reloads EXACTLY - * ONCE (sentinel `freshell.server-build-reload` in sessionStorage) and + * ONCE (sentinel `freshell.server-build-reload` in sessionStorage, which + * records the attempted server build id) and * converges to a healthy ready connection. A repeat mismatched ready must * NOT reload again — a half-deployed server can never reload-loop. * @@ -1353,15 +1394,17 @@ test.describe('server build mismatch reload (rust)', () => { await harness.waitForConnection() // Seed the state the production code would have armed on a previous - // mismatched ready in this tab (see the coverage boundary above). - await page.evaluate((key) => sessionStorage.setItem(key, '1'), SENTINEL) + // mismatched ready in this tab (see the coverage boundary above): the + // sentinel records the attempted server build id, which here is the id + // a mismatched ready would have presented. + await page.evaluate(([key, value]) => sessionStorage.setItem(key, value), [SENTINEL, MISMATCHED_BUILD_ID]) // A REAL navigation: sessionStorage must survive it (per-tab, per-origin // storage) — read at commit time, BEFORE the rebooted app's real ready // can legitimately match-and-clear it (same-HEAD artifacts match). await page.reload({ waitUntil: 'commit' }) const persisted = await page.evaluate((key) => sessionStorage.getItem(key), SENTINEL) - expect(persisted, 'sentinel must survive a real navigation').toBe('1') + expect(persisted, 'sentinel must survive a real navigation').toBe(MISMATCHED_BUILD_ID) await context.close() }) @@ -1377,8 +1420,11 @@ test.describe('server build mismatch reload (rust)', () => { // Seed AFTER the boot settles (the boot's real ready may legitimately // match-and-clear an earlier sentinel; seeding here is the setup for // the suppression proof — the arming ORDER is unit-proven, the - // navigation persistence is proven by the previous test). - await page.evaluate((key) => sessionStorage.setItem(key, '1'), SENTINEL) + // navigation persistence is proven by the previous test). The value is + // MISMATCHED_BUILD_ID: the attempted server build id the injected + // mismatch below will present, so the production suppression branch + // (same id already attempted) is the one exercised. + await page.evaluate(([key, value]) => sessionStorage.setItem(key, value), [SENTINEL, MISMATCHED_BUILD_ID]) let navigations = 0 page.on('framenavigated', () => { navigations++ }) @@ -1430,7 +1476,7 @@ In `CLOUD_SKIP_SPECS` (the filename-string skip list in `playwright.cloud.config In `AGENTS.md`, under "Key Architectural Patterns", append to the **WebSocket Protocol** paragraph: ``` -The `ready` frame carries an optional additive `buildId` (the server's artifact-time-baked git commit, `"unknown"` fallback): the client bakes its own at Vite build time (`__FRESHELL_BUILD_ID__`) and, on a mismatch, reloads exactly once per tab session (sessionStorage sentinel `freshell.server-build-reload`), self-healing stale-client contract errors; `"unknown"` on either side never triggers or clears the guard (`src/lib/server-build-check.ts`). The once-guard is per server identity: an origin fronted by mixed-build servers could oscillate, and a newer client against an older server costs one futile bounded reload per fresh tab session (both accepted for the single-server self-hosted model). +The `ready` frame carries an optional additive `buildId` (the server's artifact-time-baked git commit, `"unknown"` fallback): the client bakes its own at Vite build time (`__FRESHELL_BUILD_ID__`) and, on a mismatch, reloads exactly once per tab session (sessionStorage sentinel `freshell.server-build-reload` records the last attempted server build id; the same id never reloads twice, a different (corrected) deployment re-arms the guard), self-healing stale-client contract errors; `"unknown"` on either side never triggers or clears the guard (`src/lib/server-build-check.ts`). The once-guard is per server identity: an origin fronted by mixed-build servers could oscillate, and a newer client against an older server costs one futile bounded reload per fresh tab session (both accepted for the single-server self-hosted model). ``` - [x] **Step 2: Run the test and verify it passes, then RED-VERIFY it exercises the feature** diff --git a/src/lib/server-build-check.ts b/src/lib/server-build-check.ts index a884cd223..934cab4e7 100644 --- a/src/lib/server-build-check.ts +++ b/src/lib/server-build-check.ts @@ -44,15 +44,18 @@ function defaultStorage(): Pick | * reload ONCE on a real mismatch. Invariants: * - reload iff BOTH ids are present, non-empty, neither is "unknown", and * they differ ("unknown" == "unknown" is a no-op, never a match-and-clear); - * - the sessionStorage sentinel is set BEFORE reloading and suppresses any - * further reloads this tab session (a half-deployed server can never - * reload-loop; any sessionStorage failure = no reload, logged, fail-safe); + * - the sessionStorage sentinel records the ATTEMPTED server build id and is + * written BEFORE reloading: the same server build id never reloads twice + * this tab session (a half-deployed server can never reload-loop), while a + * DIFFERENT mismatched id re-arms the guard — a corrected deployment + * changes what a reload fetches, so it must stay reachable; any + * sessionStorage failure = no reload, logged, fail-safe; * - a MATCHING ready clears the sentinel (self-re-arm after convergence). * KNOWN LIMITS (accepted for the self-hosted single-server threat model): - * - the "once" guarantee is per server identity — one origin fronted by - * servers built from DIFFERENT commits can oscillate (mismatch → reload → - * match clears → mismatch → …). Not hardened with a clears-per-session - * cap; revisit only if a split-deploy origin appears. + * - the mixed-build-origin oscillation door stays open through match-clears: + * one origin fronted by servers built from DIFFERENT commits can oscillate + * (mismatch → reload → match clears → mismatch → …). Not hardened with a + * clears-per-session cap; revisit only if a split-deploy origin appears. * - the compare is direction-free (shas carry no ordering), so a NEWER * client against an OLDER server performs one futile bounded reload per * fresh tab session. @@ -85,21 +88,23 @@ export function checkServerBuildId(options?: ServerBuildCheckOptions): void { return } try { - if (storage.getItem(SERVER_BUILD_RELOAD_SENTINEL) === '1') { + if (storage.getItem(SERVER_BUILD_RELOAD_SENTINEL) === serverBuildId) { log.warn( `server build ${serverBuildId} still differs from client build ${clientBuildId}; ` - + 'one reload already attempted this tab session — suppressing further reloads', + + `a reload for build ${serverBuildId} was already attempted this tab session — ` + + 'suppressing further reloads for it', ) return } - storage.setItem(SERVER_BUILD_RELOAD_SENTINEL, '1') + storage.setItem(SERVER_BUILD_RELOAD_SENTINEL, serverBuildId) } catch (err) { log.warn('server-build sentinel persistence failed; suppressing the reload', err) return } log.warn( `server build ${serverBuildId} differs from client build ${clientBuildId}; ` - + 'reloading once to pick up the matching client bundle', + + `reloading once for build ${serverBuildId} to pick up the matching client bundle ` + + '(a different server build id will re-arm this guard)', ) reload() } diff --git a/test/e2e-browser/specs/server-build-mismatch-rust.spec.ts b/test/e2e-browser/specs/server-build-mismatch-rust.spec.ts index 9ac2754ec..ddd3b0f66 100644 --- a/test/e2e-browser/specs/server-build-mismatch-rust.spec.ts +++ b/test/e2e-browser/specs/server-build-mismatch-rust.spec.ts @@ -4,9 +4,10 @@ * The user story: a tab running a client bundle built at commit A connects * to a server built at commit B; the server's `ready.buildId` differs from * the client's baked `__FRESHELL_BUILD_ID__`; the client reloads EXACTLY - * ONCE (sentinel `freshell.server-build-reload` in sessionStorage) and - * converges to a healthy ready connection. A repeat mismatched ready must - * NOT reload again — a half-deployed server can never reload-loop. + * ONCE (sentinel `freshell.server-build-reload` in sessionStorage, which + * records the attempted server build id) and converges to a healthy ready + * connection. A repeat mismatched ready for the SAME build id must NOT + * reload again — a half-deployed server can never reload-loop. * * COVERAGE BOUNDARY (read before judging): what e2e proves here is * (1) the full production compare-and-reload pipeline through the REAL App @@ -116,8 +117,10 @@ test.describe('server build mismatch reload (rust)', () => { await harness.waitForConnection() // Seed the state the production code would have armed on a previous - // mismatched ready in this tab (see the coverage boundary above). - await page.evaluate((key) => sessionStorage.setItem(key, '1'), SENTINEL) + // mismatched ready in this tab (see the coverage boundary above): the + // sentinel records the attempted server build id, which here is the id + // a mismatched ready would have presented. + await page.evaluate(([key, value]) => sessionStorage.setItem(key, value), [SENTINEL, MISMATCHED_BUILD_ID]) // A REAL navigation: sessionStorage must survive it (per-tab, per-origin // storage). The value is snapshotted at DOCUMENT CREATION — an init @@ -127,7 +130,7 @@ test.describe('server build mismatch reload (rust)', () => { // guarantees the document exists, and by the time the new app has // received the real (matching) ready it may ALREADY have cleared the // sentinel before a late `page.evaluate` could read it. `null` would - // mean absent at document start; `'1'` means persisted. + // mean absent at document start; `MISMATCHED_BUILD_ID` means persisted. await page.addInitScript((key) => { ;(window as any).__sentinelAtDocumentStart = window.sessionStorage.getItem(key) }, SENTINEL) @@ -135,7 +138,7 @@ test.describe('server build mismatch reload (rust)', () => { const persisted = await page .waitForFunction(() => (window as any).__sentinelAtDocumentStart !== undefined) .then(() => page.evaluate(() => (window as any).__sentinelAtDocumentStart)) - expect(persisted, 'sentinel must survive a real navigation').toBe('1') + expect(persisted, 'sentinel must survive a real navigation').toBe(MISMATCHED_BUILD_ID) await context.close() }) @@ -151,8 +154,11 @@ test.describe('server build mismatch reload (rust)', () => { // Seed AFTER the boot settles (the boot's real ready may legitimately // match-and-clear an earlier sentinel; seeding here is the setup for // the suppression proof — the arming ORDER is unit-proven, the - // navigation persistence is proven by the previous test). - await page.evaluate((key) => sessionStorage.setItem(key, '1'), SENTINEL) + // navigation persistence is proven by the previous test). The value is + // MISMATCHED_BUILD_ID: the attempted server build id the injected + // mismatch below will present, so the production suppression branch + // (same id already attempted) is the one exercised. + await page.evaluate(([key, value]) => sessionStorage.setItem(key, value), [SENTINEL, MISMATCHED_BUILD_ID]) let navigations = 0 page.on('framenavigated', () => { navigations++ }) diff --git a/test/unit/client/components/App.restart-signals.test.tsx b/test/unit/client/components/App.restart-signals.test.tsx index 7f6791b3a..d4969b63c 100644 --- a/test/unit/client/components/App.restart-signals.test.tsx +++ b/test/unit/client/components/App.restart-signals.test.tsx @@ -486,8 +486,9 @@ describe('App ready buildId → one-shot server-build reload', () => { // jsdom 25's Location owns `reload` non-configurably — defineProperty on // window.location itself throws. Repo precedent (import-retry.test.ts): // window-level replacement with save/restore. The reload stub asserts - // the sentinel is armed AT CALL TIME (the ordering proof lives here - // too, against real jsdom sessionStorage) and counts invocations. + // the sentinel is armed AT CALL TIME with the attempted server build id + // (the ordering proof lives here too, against real jsdom sessionStorage) + // and counts invocations. originalLocation = window.location Object.defineProperty(window, 'location', { value: { @@ -496,7 +497,7 @@ describe('App ready buildId → one-shot server-build reload', () => { expect( sessionStorage.getItem('freshell.server-build-reload'), 'sentinel must be armed BEFORE reload fires', - ).toBe('1') + ).toBe('b'.repeat(40)) reloadCalls++ }, }, @@ -523,7 +524,7 @@ describe('App ready buildId → one-shot server-build reload', () => { sendReady({ serverInstanceId: 'srv-1', bootId: 'boot-1', buildId: 'b'.repeat(40) }) expect(reloadCalls).toBe(1) - expect(sessionStorage.getItem('freshell.server-build-reload')).toBe('1') + expect(sessionStorage.getItem('freshell.server-build-reload')).toBe('b'.repeat(40)) // The reload lands: the page reboots in the SAME tab (real jsdom // sessionStorage persists), the server is still stale, and the next @@ -534,7 +535,9 @@ describe('App ready buildId → one-shot server-build reload', () => { it('a matching ready clears the sentinel and re-arms the guard', async () => { vi.stubGlobal('__FRESHELL_BUILD_ID__', 'a'.repeat(40)) - sessionStorage.setItem('freshell.server-build-reload', '1') + // A sentinel recorded by an earlier mismatched ready (the attempted + // server build id), as the production code would have persisted it. + sessionStorage.setItem('freshell.server-build-reload', 'b'.repeat(40)) const store = createStore() await renderApp(store) diff --git a/test/unit/client/lib/server-build-check.test.ts b/test/unit/client/lib/server-build-check.test.ts index 768f2f846..052d0fba1 100644 --- a/test/unit/client/lib/server-build-check.test.ts +++ b/test/unit/client/lib/server-build-check.test.ts @@ -19,31 +19,50 @@ describe('checkServerBuildId', () => { vi.restoreAllMocks() }) - it('reloads once, arming the sentinel BEFORE the reload fires, and the sentinel suppresses a second mismatch', () => { + it('reloads once, recording the attempted server build id in the sentinel BEFORE the reload fires', () => { const storage = mapStorage() const reload = vi.fn(() => { // Ordering proof: production must persist the sentinel BEFORE // calling reload — an implementation that reloads first and arms // second would lose the sentinel across the navigation. - expect(storage._map.get(SENTINEL), 'sentinel must be armed BEFORE reload fires').toBe('1') + expect(storage._map.get(SENTINEL), 'sentinel must be armed BEFORE reload fires').toBe('b'.repeat(40)) }) checkServerBuildId({ clientBuildId: 'a'.repeat(40), serverBuildId: 'b'.repeat(40), reload, storage }) expect(reload).toHaveBeenCalledTimes(1) - expect(storage._map.get(SENTINEL)).toBe('1') + expect(storage._map.get(SENTINEL)).toBe('b'.repeat(40)) }) - it('never reloads twice: an armed sentinel suppresses the reload', () => { + it('never reloads twice for the same server build id: a recorded sentinel suppresses the reload', () => { const storage = mapStorage() - storage._map.set(SENTINEL, '1') + storage._map.set(SENTINEL, 'b'.repeat(40)) const reload = vi.fn() checkServerBuildId({ clientBuildId: 'a'.repeat(40), serverBuildId: 'b'.repeat(40), reload, storage }) expect(reload).not.toHaveBeenCalled() - expect(storage._map.get(SENTINEL)).toBe('1') + expect(storage._map.get(SENTINEL)).toBe('b'.repeat(40)) }) - it('a matching ready clears the sentinel (self-re-arm)', () => { + it('re-arms for a DIFFERENT mismatched server build id: B attempts once, repeats of B suppress, C reloads again', () => { const storage = mapStorage() - storage._map.set(SENTINEL, '1') + const reload = vi.fn() + // Mismatch vs B: reload, sentinel records B. + checkServerBuildId({ clientBuildId: 'a'.repeat(40), serverBuildId: 'b'.repeat(40), reload, storage }) + expect(reload).toHaveBeenCalledTimes(1) + expect(storage._map.get(SENTINEL)).toBe('b'.repeat(40)) + // Mismatch vs B again (the half-deployed case): the same identity was + // already attempted — suppressed, no reload. + checkServerBuildId({ clientBuildId: 'a'.repeat(40), serverBuildId: 'b'.repeat(40), reload, storage }) + expect(reload).toHaveBeenCalledTimes(1) + expect(storage._map.get(SENTINEL)).toBe('b'.repeat(40)) + // A corrected deployment (C): a different server build id re-arms the + // guard — reloads again, sentinel now records C. + checkServerBuildId({ clientBuildId: 'a'.repeat(40), serverBuildId: 'c'.repeat(40), reload, storage }) + expect(reload).toHaveBeenCalledTimes(2) + expect(storage._map.get(SENTINEL)).toBe('c'.repeat(40)) + }) + + it('a matching ready clears the recorded sentinel (self-re-arm)', () => { + const storage = mapStorage() + storage._map.set(SENTINEL, 'b'.repeat(40)) const reload = vi.fn() checkServerBuildId({ clientBuildId: 'a'.repeat(40), serverBuildId: 'a'.repeat(40), reload, storage }) expect(reload).not.toHaveBeenCalled() @@ -67,13 +86,13 @@ describe('checkServerBuildId', () => { } }) - it('an armed sentinel survives an "unknown"-vs-"unknown" ready (never treated as a match)', () => { + it('a recorded sentinel survives an "unknown"-vs-"unknown" ready (never treated as a match)', () => { const storage = mapStorage() - storage._map.set(SENTINEL, '1') + storage._map.set(SENTINEL, 'b'.repeat(40)) const reload = vi.fn() checkServerBuildId({ clientBuildId: 'unknown', serverBuildId: 'unknown', reload, storage }) expect(reload).not.toHaveBeenCalled() - expect(storage._map.get(SENTINEL)).toBe('1') + expect(storage._map.get(SENTINEL)).toBe('b'.repeat(40)) }) it('does not reload when the sentinel cannot be persisted (fail-safe against reload loops)', () => { @@ -122,7 +141,7 @@ describe('checkServerBuildId', () => { checkServerBuildId({ serverBuildId: 'd'.repeat(40) }) expect(reload).toHaveBeenCalledTimes(1) - expect(sessionStorage.getItem(SENTINEL)).toBe('1') + expect(sessionStorage.getItem(SENTINEL)).toBe('d'.repeat(40)) // And with the global absent (Vitest has no define), it is a no-op. vi.unstubAllGlobals() From f8744bb2cbcb073a5cb820df4ea5cfcf6ba1e18f Mon Sep 17 00:00:00 2001 From: Dan Shapiro <3732858+danshapiro@users.noreply.github.com> Date: Thu, 27 Aug 2026 16:15:37 -0700 Subject: [PATCH 14/15] docs: correct the archival plan's oracle-freshness amendment note --- docs/plans/2026-08-27-server-version-reload.md | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/docs/plans/2026-08-27-server-version-reload.md b/docs/plans/2026-08-27-server-version-reload.md index 24ddd1e76..5e3abb7a4 100644 --- a/docs/plans/2026-08-27-server-version-reload.md +++ b/docs/plans/2026-08-27-server-version-reload.md @@ -635,7 +635,14 @@ Extend the ready send (lines 2034-2039): }) ``` -3j. In `port/oracle/harness/external-server.ts` — the oracle's node target runs the COMPILED `dist/server/index.js`, and `ensureServerBuilt` rebuilds only when the entry is ABSENT. After this feature, a stale pre-existing `dist` carries a stale bake file, and `npm run test:oracle` would compare a stale Node `buildId` against the fresh cargo-built Rust value — a false implementation divergence. Add a stamp-freshness check so a stale node dist rebuilds (keep the legacy behavior when no bake file exists, so git-less/pre-stamp dists are unaffected): +3j. In `port/oracle/harness/external-server.ts` — the oracle's node target runs the COMPILED `dist/server/index.js`, and `ensureServerBuilt` rebuilds only when the entry is ABSENT. After this feature, a stale pre-existing `dist` carries a stale bake file, and `npm run test:oracle` would compare a stale Node `buildId` against the fresh cargo-built Rust value — a false implementation divergence. Add a stamp-freshness check so a stale node dist rebuilds. + +> As-built amendments (delta reviews rounds 1-2): the final predicate is STRICTER than the +> first draft below — with Git HEAD available, ONLY a bake stamp exactly equal to HEAD is +> current; a MISSING, unreadable, mismatched, or `"unknown"` stamp all trigger a rebuild +> (a stampless/raw-`tsc` or git-less-built artifact must never be reused against a fresh +> cargo-built rust binary). Git-unavailable environments keep the legacy reuse behavior. +> Directly tested by `test/unit/port/oracle-harness-freshness.test.ts`. ```typescript /** From 56d0445401afcbbdd4f9bc02a605d45c54531eba Mon Sep 17 00:00:00 2001 From: Dan Shapiro <3732858+danshapiro@users.noreply.github.com> Date: Thu, 27 Aug 2026 17:11:26 -0700 Subject: [PATCH 15/15] style: cargo fmt on the new build-stamp rust code --- crates/freshell-protocol/tests/roundtrip.rs | 3 ++- crates/freshell-ws/build.rs | 11 +++++++++-- crates/freshell-ws/src/lib.rs | 11 +++++++++-- 3 files changed, 20 insertions(+), 5 deletions(-) diff --git a/crates/freshell-protocol/tests/roundtrip.rs b/crates/freshell-protocol/tests/roundtrip.rs index 6f38eb624..d700a672e 100644 --- a/crates/freshell-protocol/tests/roundtrip.rs +++ b/crates/freshell-protocol/tests/roundtrip.rs @@ -179,7 +179,8 @@ fn ready_carries_build_id_and_omits_it_when_absent() { other => panic!("expected Ready, got {other:?}"), } - let without = r#"{"type":"ready","timestamp":"2026-07-05T04:20:52.546Z","serverInstanceId":"srv-abc"}"#; + let without = + r#"{"type":"ready","timestamp":"2026-07-05T04:20:52.546Z","serverInstanceId":"srv-abc"}"#; let msg: ServerMessage = serde_json::from_str(without).unwrap(); let reser = serde_json::to_value(&msg).unwrap(); assert!( diff --git a/crates/freshell-ws/build.rs b/crates/freshell-ws/build.rs index 13a3c7852..c2dc78a34 100644 --- a/crates/freshell-ws/build.rs +++ b/crates/freshell-ws/build.rs @@ -24,12 +24,19 @@ fn main() { /// `git rev-parse HEAD`, trimmed. `None` on any failure (git not on `PATH`, /// not inside a git checkout, ...) -- the caller falls back to `"unknown"`. fn git_head_commit() -> Option { - let out = Command::new("git").args(["rev-parse", "HEAD"]).output().ok()?; + let out = Command::new("git") + .args(["rev-parse", "HEAD"]) + .output() + .ok()?; if !out.status.success() { return None; } let s = String::from_utf8_lossy(&out.stdout).trim().to_string(); - if s.is_empty() { None } else { Some(s) } + if s.is_empty() { + None + } else { + Some(s) + } } /// The exact paths that change when HEAD moves in THIS checkout, resolved diff --git a/crates/freshell-ws/src/lib.rs b/crates/freshell-ws/src/lib.rs index e78d3f6cd..cab37f3c3 100644 --- a/crates/freshell-ws/src/lib.rs +++ b/crates/freshell-ws/src/lib.rs @@ -27,7 +27,11 @@ /// git metadata) -- never a runtime failure. Build provenance is /// BUILD-scoped, so this deliberately does NOT ride on `WsState`. pub fn ready_build_id() -> Option { - Some(option_env!("FRESHELL_WS_BUILD_COMMIT").unwrap_or("unknown").to_string()) + Some( + option_env!("FRESHELL_WS_BUILD_COMMIT") + .unwrap_or("unknown") + .to_string(), + ) } pub mod activity; @@ -1053,7 +1057,10 @@ mod tests { "ready must stamp buildId: {ready}" ); let build_id = ready["buildId"].as_str().expect("buildId is a string"); - assert!(!build_id.is_empty(), "buildId must be non-empty: {build_id}"); + assert!( + !build_id.is_empty(), + "buildId must be non-empty: {build_id}" + ); } /// GAP1 (CFG-03 checklist follow-up) RED/GREEN target: when boot fell