test(guard): no legacy list left; flag raw listeners on a fixed port (#440)

With the sweep done, LEGACY_FIXED_PORT_FILES and its staleness test go. A second rule flags a raw listen(<number or …PORT>) 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) <noreply@anthropic.com>
This commit is contained in:
Randalix
2026-10-09 16:29:02 +02:00
co-authored by Claude Opus 5.5
parent eceaecf076
commit fda1897109
5 changed files with 57 additions and 78 deletions
+1 -1
View File
@@ -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. 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. Tests are tmux-safe by design: under vitest, the tmux layer becomes an in-memory mock, so tests cannot touch real sessions.
+1 -1
View File
@@ -12,6 +12,6 @@ Quick pointers:
- Type check: `tsc --noEmit` · Lint: `npm run lint` · Format: `npm run format:check` - 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/<file>.test.ts` for one file - Tests: `npm test` (the CI gate, safe to run bare) or `npm test -- test/<file>.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): ...`) - Branch off `master` for all work; Conventional Commit-style messages (`fix(mobile): ...`)
- Never commit secrets or local state from `~/.codeman/` - Never commit secrets or local state from `~/.codeman/`
+2 -2
View File
@@ -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`. - **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` - **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`). - **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()`. **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) **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 <pane>` for what actually reached the PTY), not on HTTP 200. ⚠️ **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 <pane>` for what actually reached the PTY), not on HTTP 200.
+2 -3
View File
@@ -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 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 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`), (`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/**`, or use `app.inject()` when no socket is needed. Mobile tests call `createTestServer()` and
via `createTestServer(PORT)`) keep the fixed ports in `test/mobile/README.md` for now, read `server.boundPort`. Never 3000.
because that helper caches servers by port.
## Finding your way around ## Finding your way around
+51 -71
View File
@@ -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 * 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 * 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 * suite runs files serially (`fileParallelism: false`), so the port pairs #440 found
* found never met inside one run, only across runs. `new WebServer(0, …)` binds * never met inside one run, only across runs. Binding port 0 takes whatever the OS
* whatever the OS hands out and `boundPort` reads it back, so there is nothing left * hands out, so there is nothing left to collide on.
* to collide on.
* *
* A WebServer built under test/ whose port argument is not the literal `0` fails — * Two rules, both over every file under test/:
* `new WebServer(…)`, a class declared `extends WebServer`, or a destructured alias * - A `WebServer` is built with the literal `0` as its port — `new WebServer(…)`, a
* (`{ WebServer: T }`) — unless the file is in LEGACY_FIXED_PORT_FILES, the files that * class declared `extends WebServer`, or a destructured alias (`{ WebServer: T }`) —
* construct one with a non-zero port today (a few never call `start()`); the follow-up * and the test reads `server.boundPort`.
* sweep converts them. A converted file cannot stay listed: an entry whose file no * - A raw server (`http`/`net`/Fastify `listen`, `new WebSocketServer`) never listens
* longer matches fails too. * 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 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 * not at its callers; `import { WebServer as X }`; `new mod.WebServer(…)`; a port held
* calling `createTestServer(PORT)` are not checked), `import { WebServer as X }`, and * in a variable that is not named `…PORT`.
* `new mod.WebServer(…)`. Raw `listen({ port: N })` and `new WebSocketServer({ port: N })`
* belong to the sweep.
* *
* Port: N/A (pure static analysis). * Port: N/A (pure static analysis).
*/ */
import { readdirSync, readFileSync, statSync } from 'node:fs'; 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 { fileURLToPath } from 'node:url';
import { describe, expect, it } from 'vitest'; import { describe, expect, it } from 'vitest';
const TEST_ROOT = fileURLToPath(new URL('.', import.meta.url)); 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[] { function testFiles(dir: string): string[] {
const out: string[] = []; const out: string[] = [];
for (const name of readdirSync(dir)) { 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 SELF = fileURLToPath(import.meta.url);
const scanned = testFiles(TEST_ROOT) const scanned = testFiles(TEST_ROOT)
.filter((f) => f !== SELF) .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'); const fixed = (args: string[]) => args.some((a) => a !== '0');
describe('test servers bind an ephemeral port', () => { 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']); 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 const offenders = scanned
.filter((f) => fixed(f.args) && !LEGACY_FIXED_PORT_FILES.has(f.rel)) .filter((f) => fixed(f.args))
.map( .map(
(f) => (f) =>
`${f.rel}: new WebServer(${f.args.find((a) => a !== '0')}, …) — use new WebServer(0, …) and server.boundPort` `${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([]); expect(offenders).toEqual([]);
}); });
it('the legacy list only names files that still need converting', () => { it('no test listens on a fixed port', () => {
const stale = [...LEGACY_FIXED_PORT_FILES].filter((rel) => !scanned.some((f) => f.rel === rel && fixed(f.args))); const offenders = scanned
expect(stale).toEqual([]); .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([]);
}); });
}); });