From fda189710951eea99c6e4fab26c500eb325f7de8 Mon Sep 17 00:00:00 2001 From: Randalix Date: Fri, 9 Oct 2026 16:29:02 +0200 Subject: [PATCH] test(guard): no legacy list left; flag raw listeners on a fixed port (#440) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit With the sweep done, LEGACY_FIXED_PORT_FILES and its staleness test go. A second rule flags a raw listen() and a port: with a number or …PORT constant inside listen({ … }) or new WebSocketServer({ … }); a socket path and a lower-case variable pass. CLAUDE.md, AGENTS.md, CONTRIBUTING.md and the wiki's Contributing page lose the mobile exception, and CLAUDE.md names closedPort() instead of port + 1. Co-Authored-By: Claude Opus 5.5 (1M context) --- .github/CONTRIBUTING.md | 2 +- AGENTS.md | 2 +- CLAUDE.md | 4 +- docs/wiki/Contributing.md | 5 +- test/test-ports-guard.test.ts | 122 ++++++++++++++-------------------- 5 files changed, 57 insertions(+), 78 deletions(-) diff --git a/.github/CONTRIBUTING.md b/.github/CONTRIBUTING.md index fab29389..02874dd1 100644 --- a/.github/CONTRIBUTING.md +++ b/.github/CONTRIBUTING.md @@ -57,7 +57,7 @@ Expect `test:browser`/`test:mobile`/`test:perf` to fail where the machine cannot The browser suite also runs nightly (and on demand) in `.github/workflows/browser-suite.yml`; it is informational, not a gate. -If you add a test that binds a port, bind port 0 (`new WebServer(0, …)` + `server.boundPort`, or `listen({ port: 0 })` + `address().port`), or use `app.inject()` when no socket is needed; `test/test-ports-guard.test.ts` fails a `WebServer` built on any other port. Mobile tests (`test/mobile/**`, via `createTestServer(PORT)`) keep the fixed-port convention in `test/mobile/README.md` for now, because that helper caches servers by port. Never 3000. +If you add a test that binds a port, bind port 0 (`new WebServer(0, …)` + `server.boundPort`, or `listen({ port: 0 })` + `address().port`), or use `app.inject()` when no socket is needed; mobile tests call `createTestServer()` and read `server.boundPort`. `test/test-ports-guard.test.ts` fails a `WebServer` built on any other port and a raw `listen` on a fixed one. Never 3000. Tests are tmux-safe by design: under vitest, the tmux layer becomes an in-memory mock, so tests cannot touch real sessions. diff --git a/AGENTS.md b/AGENTS.md index e0cf802b..a3b084da 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -12,6 +12,6 @@ Quick pointers: - Type check: `tsc --noEmit` · Lint: `npm run lint` · Format: `npm run format:check` - Tests: `npm test` (the CI gate, safe to run bare) or `npm test -- test/.test.ts` for one file -- Route tests use `app.inject()`; new tests needing a socket bind port 0 (`new WebServer(0, …)` + `boundPort`), except mobile tests, which keep `createTestServer(PORT)` for now +- Route tests use `app.inject()`; new tests needing a socket bind port 0 (`new WebServer(0, …)` + `boundPort`); mobile tests use `createTestServer()` and read `server.boundPort` - Branch off `master` for all work; Conventional Commit-style messages (`fix(mobile): ...`) - Never commit secrets or local state from `~/.codeman/` diff --git a/CLAUDE.md b/CLAUDE.md index d122f877..185820fe 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -436,7 +436,7 @@ One module per domain in `src/web/routes/` (plus a barrel; `ls src/web/routes/` - **App setting**: decide per-device vs synced first. Per-device keys go in the `displayKeys` set in settings-ui.js and must NOT be added to `SettingsUpdateSchema` (it is `.strict()`). ⚠️ Anything in `PUT /api/settings` that acts on a setting (the `toggleService` watcher calls) must resolve from **`merged`** (persisted + incoming), never from the raw request body: a partial PUT omits keys it doesn't intend to change, and `body.x ?? default` turns every omission into "apply the default" and silently resets live services. Pinned by `test/routes/system-routes-settings-partial-put.test.ts`. - **Hook event**: Add to `HookEventType`, add hook in `hooks-config.ts:generateHooksConfig()`, update `HookEventSchema` - **Mobile feature**: Add to relevant singleton, guard with `MobileDetection.isMobile()`. New header buttons must stay off phones (`test/mobile-header-buttons-policy.test.ts`). -- **New test**: a test that constructs `WebServer` itself binds port 0 (`new WebServer(0, …)`, then `server.boundPort`; enforced by `test/test-ports-guard.test.ts`), and a raw Fastify/`ws` server listens on `port: 0` and reads `address().port`. Mobile tests (`test/mobile/**`, via `createTestServer(PORT)`) keep the fixed-port convention in `test/mobile/README.md` until the follow-up sweep, because that helper caches servers by port. Route tests use `app.inject()` (no port needed); see `test/routes/_route-test-utils.ts`. +- **New test**: a test that constructs `WebServer` itself binds port 0 (`new WebServer(0, …)`, then `server.boundPort`), and a raw `http`/`net`/Fastify/`ws` server listens on `0` and reads `address().port`; mobile tests get theirs from `createTestServer()` and read `boundPort` too. Both enforced by `test/test-ports-guard.test.ts`. Route tests use `app.inject()` (no port needed); see `test/routes/_route-test-utils.ts`. **Validation**: Zod v4 (different API from v3). Define schemas in `schemas.ts`, use `.parse()`/`.safeParse()`. @@ -475,7 +475,7 @@ Raw `npx vitest` skips the config (and with it `setup.ts`); always use `npm test **Tmux safety**: under vitest (`VITEST`), `TmuxManager` no-ops ALL shell commands (`IS_TEST_MODE` in `src/tmux-manager.ts`), docker IO is no-op'd likewise, and `Session` spawns an echo PTY (`TEST_PTY_SCRIPT`) instead of attaching tmux. `test/setup.ts` gives each file a temp `HOME`/`USERPROFILE` and strips `CODEMAN_PASSWORD`/`CODEMAN_USERNAME`, `CODEMAN_GESTURE` and `CODEMAN_INSTANCE`/`CODEMAN_DATA_DIR`/`CODEMAN_TMUX_SOCKET` (pinned by `test/test-env-isolation.test.ts`). ⚠️ `CODEMAN_DATA_DIR` overrides the temp HOME, so never drop its strip; strip `CODEMAN_INSTANCE` in the setup file, never in a hook (captured at first import). ⚠️ Delete case trees only via `safeRmHomeTree()`. ⚠️ Raw `npx vitest` without `--config` skips `setup.ts` and its isolation. → [architecture-invariants#test-isolation-tmux-docker-and-home](docs/architecture-invariants.md#test-isolation-tmux-docker-and-home) -**Ports**: a test that constructs `WebServer` binds port 0 and reads `server.boundPort`; `test/test-ports-guard.test.ts` fails any other port outside its shrink-only legacy list. A raw Fastify/`ws` server listens on `port: 0` and reads `address().port`. The mobile suite keeps fixed ports (3150+, per `test/mobile/README.md`) until the follow-up sweep, since `createTestServer()` caches servers by port. Never 3000 (the live instance). +**Ports**: every test binds port 0. A test that constructs `WebServer` reads `server.boundPort` (mobile tests: `createTestServer()`, then `server.boundPort`); a raw `http`/`net`/Fastify/`ws` server listens on `0` and reads `address().port`. `test/test-ports-guard.test.ts` fails a `WebServer` built on any other port and a raw `listen` on a number or `…PORT` constant. A test that needs a port nothing listens on binds 0, reads it and closes (`closedPort()` in `test/daemon-control.test.ts`), never "the server's port + 1". ⚠️ **Browser tests can pass vacuously on mobile input paths.** Two traps, both hit on 2026-07-27 while fixing the phone Enter button: **(1)** driving input with `app.sendInput('…')` writes PAST the `LocalEchoOverlay`, so `pendingText` stays empty and any overlay bug is invisible — type with `page.keyboard.type()` instead; **(2)** headless Chromium reports `MobileDetection.isTouchDevice()` **false even with `hasTouch: true`**, so `_localEchoEnabled` is off and the local-echo branch never executes. Force it (`app._localEchoEnabled = true`) or the test proves nothing. Assert on real state (`app._localEchoOverlay.pendingText`, plus `tmux -L codeman capture-pane -p -t ` for what actually reached the PTY), not on HTTP 200. diff --git a/docs/wiki/Contributing.md b/docs/wiki/Contributing.md index 6872ca90..171203b7 100644 --- a/docs/wiki/Contributing.md +++ b/docs/wiki/Contributing.md @@ -63,9 +63,8 @@ provide what they need; that means "not runnable here", not a regression. Tests are tmux-safe by design: under vitest the tmux layer becomes an in-memory mock, so tests cannot touch real sessions. If you add a test that binds a port, bind port 0 (`new WebServer(0, …)` + `server.boundPort`, or `listen({ port: 0 })` + `address().port`), -or use `app.inject()` when no socket is needed. Never 3000. Mobile tests (`test/mobile/**`, -via `createTestServer(PORT)`) keep the fixed ports in `test/mobile/README.md` for now, -because that helper caches servers by port. +or use `app.inject()` when no socket is needed. Mobile tests call `createTestServer()` and +read `server.boundPort`. Never 3000. ## Finding your way around diff --git a/test/test-ports-guard.test.ts b/test/test-ports-guard.test.ts index 20c7d8b8..79018d90 100644 --- a/test/test-ports-guard.test.ts +++ b/test/test-ports-guard.test.ts @@ -1,84 +1,33 @@ /** - * @fileoverview Static guard: a test builds `WebServer` on an ephemeral port. + * @fileoverview Static guard: a test binds an ephemeral port. * * A fixed port is a red suite on any machine where something else holds it, and a * collision between two runs on one host (two worktrees, or CI plus a local run): the - * suite runs files serially (`fileParallelism: false`), so the four port pairs #440 - * found never met inside one run, only across runs. `new WebServer(0, …)` binds - * whatever the OS hands out and `boundPort` reads it back, so there is nothing left - * to collide on. + * suite runs files serially (`fileParallelism: false`), so the port pairs #440 found + * never met inside one run, only across runs. Binding port 0 takes whatever the OS + * hands out, so there is nothing left to collide on. * - * A WebServer built under test/ whose port argument is not the literal `0` fails — - * `new WebServer(…)`, a class declared `extends WebServer`, or a destructured alias - * (`{ WebServer: T }`) — unless the file is in LEGACY_FIXED_PORT_FILES, the files that - * construct one with a non-zero port today (a few never call `start()`); the follow-up - * sweep converts them. A converted file cannot stay listed: an entry whose file no - * longer matches fails too. + * Two rules, both over every file under test/: + * - A `WebServer` is built with the literal `0` as its port — `new WebServer(…)`, a + * class declared `extends WebServer`, or a destructured alias (`{ WebServer: T }`) — + * and the test reads `server.boundPort`. + * - A raw server (`http`/`net`/Fastify `listen`, `new WebSocketServer`) never listens + * on a number or a `…PORT` constant: `listen(0, …)` / `{ port: 0 }`, then + * `address().port`. * * Not covered: a helper that takes the port as a parameter is checked at the helper, - * not at its callers (test/mobile/helpers/server.ts is listed, so the mobile tests - * calling `createTestServer(PORT)` are not checked), `import { WebServer as X }`, and - * `new mod.WebServer(…)`. Raw `listen({ port: N })` and `new WebSocketServer({ port: N })` - * belong to the sweep. + * not at its callers; `import { WebServer as X }`; `new mod.WebServer(…)`; a port held + * in a variable that is not named `…PORT`. * * Port: N/A (pure static analysis). */ import { readdirSync, readFileSync, statSync } from 'node:fs'; -import { join, relative, sep } from 'node:path'; +import { join, relative } from 'node:path'; import { fileURLToPath } from 'node:url'; import { describe, expect, it } from 'vitest'; const TEST_ROOT = fileURLToPath(new URL('.', import.meta.url)); -/** Predates the guard; converted in the follow-up sweep. Shrink only. */ -const LEGACY_FIXED_PORT_FILES = new Set( - [ - 'admin-routes.test.ts', - 'base-path-server.test.ts', - 'capture-geometry-retry.browser.test.ts', - 'capture-load-window.browser.test.ts', - 'case-custom-path.browser.test.ts', - 'doctor-settings.browser.test.ts', - 'edge-cases.test.ts', - 'file-link-click.test.ts', - 'git-status.browser.test.ts', - 'hooks-config.test.ts', - 'http-contract.test.ts', - 'inline-rename.test.ts', - 'integration-flows.test.ts', - 'key-tester.browser.test.ts', - 'mobile/helpers/server.ts', - 'opencode-resize.test.ts', - 'operation-lightspeed.test.ts', - 'ownership-scoping.test.ts', - 'pane-exit-sweep.test.ts', - 'paste-image-dir-shared.test.ts', - 'perf-browser.test.ts', - 'quick-start.test.ts', - 'ralph-integration.test.ts', - 'scheduled-runs.test.ts', - 'security-regression.test.ts', - 'session-cleanup.test.ts', - 'session-pane-exit.test.ts', - 'session.test.ts', - 'shift-enter-keypress.browser.test.ts', - 'split-pane-auto-collapse.browser.test.ts', - 'split-pane-orchestration.browser.test.ts', - 'split-pane-terminal.browser.test.ts', - 'sse-cors-headers.test.ts', - 'sse-events.test.ts', - 'sse-routing-remote.test.ts', - 'sse-subscription-filter.test.ts', - 'static-cache-headers.test.ts', - 'terminal-copy-shortcut.test.ts', - 'terminal-keycode229-recovery.browser.test.ts', - 'webgl-fallback.test.ts', - 'webhook-settings.browser.test.ts', - 'webview-lost-root-frame.test.ts', - 'webview-sse.test.ts', - ].map((p) => p.split('/').join(sep)) -); - function testFiles(dir: string): string[] { const out: string[] = []; for (const name of readdirSync(dir)) { @@ -108,10 +57,29 @@ function webServerPortArgs(source: string): string[] { ); } +/** A fixed port as a reader would see it: a non-zero number, or a constant named `…PORT`. */ +const FIXED_PORT = /^(?:[1-9]\d*|[A-Z_]*PORT)$/; + +/** + * Fixed ports handed to a raw server in `source`: the first argument of `.listen(…)`, + * and `port:` inside `.listen({ … })` or `new WebSocketServer({ … })`. A socket path or + * `0` is fine; so is anything held in a lower-case variable (see the header). + */ +function rawListenerFixedPorts(source: string): string[] { + const firstArgs = [...source.matchAll(/\.listen\(\s*([^,){\s]+)/g)].map((m) => m[1]); + const options = [...source.matchAll(/(?:\.listen|new WebSocketServer)\(\s*\{[^}]*?\bport\s*:\s*([^,}\s]+)/g)].map( + (m) => m[1] + ); + return [...firstArgs, ...options].filter((arg) => FIXED_PORT.test(arg)); +} + const SELF = fileURLToPath(import.meta.url); const scanned = testFiles(TEST_ROOT) .filter((f) => f !== SELF) - .map((file) => ({ rel: relative(TEST_ROOT, file), args: webServerPortArgs(readFileSync(file, 'utf8')) })); + .map((file) => { + const source = readFileSync(file, 'utf8'); + return { rel: relative(TEST_ROOT, file), args: webServerPortArgs(source), raw: rawListenerFixedPorts(source) }; + }); const fixed = (args: string[]) => args.some((a) => a !== '0'); describe('test servers bind an ephemeral port', () => { @@ -124,9 +92,19 @@ describe('test servers bind an ephemeral port', () => { expect(webServerPortArgs('const { WebServer: T } = mod;\nreturn new T(port, false);')).toEqual(['port']); }); - it('no test outside the legacy list builds WebServer on a fixed port', () => { + it('reads raw listeners the way a reader would', () => { + expect(rawListenerFixedPorts("server.listen(0, '127.0.0.1', done)")).toEqual([]); + expect(rawListenerFixedPorts("server.listen(PORT, '127.0.0.1', done)")).toEqual(['PORT']); + expect(rawListenerFixedPorts('srv.listen(3216)')).toEqual(['3216']); + expect(rawListenerFixedPorts("await app.listen({ port: TEST_PORT, host: '127.0.0.1' })")).toEqual(['TEST_PORT']); + expect(rawListenerFixedPorts("await app.listen({ port: 0, host: '127.0.0.1' })")).toEqual([]); + expect(rawListenerFixedPorts('new WebSocketServer({ port: 8081 })')).toEqual(['8081']); + expect(rawListenerFixedPorts('net.createServer().listen(socketPath)')).toEqual([]); + }); + + it('no test builds WebServer on a fixed port', () => { const offenders = scanned - .filter((f) => fixed(f.args) && !LEGACY_FIXED_PORT_FILES.has(f.rel)) + .filter((f) => fixed(f.args)) .map( (f) => `${f.rel}: new WebServer(${f.args.find((a) => a !== '0')}, …) — use new WebServer(0, …) and server.boundPort` @@ -134,8 +112,10 @@ describe('test servers bind an ephemeral port', () => { expect(offenders).toEqual([]); }); - it('the legacy list only names files that still need converting', () => { - const stale = [...LEGACY_FIXED_PORT_FILES].filter((rel) => !scanned.some((f) => f.rel === rel && fixed(f.args))); - expect(stale).toEqual([]); + it('no test listens on a fixed port', () => { + const offenders = scanned + .filter((f) => f.raw.length > 0) + .map((f) => `${f.rel}: listens on ${f.raw.join(', ')} — listen on 0 and read address().port`); + expect(offenders).toEqual([]); }); });