From ffdb2be0d8723a782cff4f57af2f5e85bd8cc936 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 27 Aug 2026 03:37:29 +0000 Subject: [PATCH 1/3] feat(cli): os serve announces an exhausted port search, carrying the message the search threw (#12620) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In development `os serve` walks forward from the requested port looking for a free one. The walk gives up after 101 ports and throws a message that names the problem exactly — and the caller's `catch { /* Ignore */ }` discarded it, fell through, and bound the requested port anyway: the one port the search had just proven was taken. The boot then died on the kernel's raw EADDRINUSE with the accurate explanation thrown away one line earlier. This is the one shape in the whole port policy that reaches NEITHER half of the family's legibility work. The production `Port ... is already in use` line lives in the `else if` this boot never enters, and the shifted-port notice is gated on `port !== requestedPort`, which is false here because the assignment threw before it could happen. The fallthrough itself is unchanged and stays deliberate; whether an exhausted search should refuse instead is #11113's production/development policy split and is not decided here. Only the silence is repaired. The notice CARRIES the thrown message as its headline rather than paraphrasing it, so one fact keeps one spelling. The width it reports is derived from the same constant the walk uses, so the range it names is always the range it probed. Channel is `printDiagnostic` — stderr, the same helper, stream and boot position as the two sibling notices; stdout carries JSON-RPC frames whenever the stdio MCP transport is mounted. `getAvailablePort` now takes its port probe as a parameter, defaulting to the real one. That is what makes the exhausted path testable without holding 101 real ports — a test that would be slow, flaky and hostile to a shared container whose ephemeral range is already crowded. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01UjujZN219uFzBhSYfMykCd --- .../cli-serve-exhausted-port-search-notice.md | 44 +++ ...serve-exhausted-port-search-notice.test.ts | 265 ++++++++++++++++++ packages/cli/src/commands/serve.ts | 120 +++++++- 3 files changed, 422 insertions(+), 7 deletions(-) create mode 100644 .changeset/cli-serve-exhausted-port-search-notice.md create mode 100644 packages/cli/src/commands/serve-exhausted-port-search-notice.test.ts diff --git a/.changeset/cli-serve-exhausted-port-search-notice.md b/.changeset/cli-serve-exhausted-port-search-notice.md new file mode 100644 index 0000000000..60e3eef15c --- /dev/null +++ b/.changeset/cli-serve-exhausted-port-search-notice.md @@ -0,0 +1,44 @@ +--- +"@objectstack/cli": minor +--- + +feat(cli): `os serve` says so when its port search runs out of ports, in the words the search threw (#12620) + +In development (`os dev`, `--dev`, or `NODE_ENV=development`) `os serve` walks +forward from the requested port looking for a free one. That walk gives up after +101 ports, and when it does it throws a message naming the problem exactly — +which the caller then discarded, fell through, and bound the requested port +anyway: the one port the search had *just proven* was taken. + +That boot died on the kernel's raw `EADDRINUSE` with no explanation anywhere. It +is the one shape in the whole port policy that reached **neither** half of this +family's legibility work: the production `Port … is already in use` line lives in +a branch this boot never enters, and the shifted-port notice (#12543) is printed +only when the bound port differs from the requested one — on this path they are +the same, because the search threw before it could assign. The accurate sentence +existed and was thrown away one line earlier. + +The fallthrough is unchanged and still deliberate — a developer whose whole next +span is busy arguably does want the requested port attempted rather than a hard +refusal, and whether it should refuse instead is a separate policy question +(#11113). What changed is that it is no longer silent: + +``` + ⚠ Could not find an available port starting from 32869 + Development auto-shift probed 101 ports (32869–32969) and every + one was busy, so this server is falling back to 32869 — the port the + search has just proven is taken. The bind that follows will almost + certainly fail with a raw EADDRINUSE from the kernel, and this notice + is the only place that says why. + Free a port in 32869–32969, or pick another via PORT= (or --port ). +``` + +The first line is the search's own thrown message, carried rather than +paraphrased, so there is one spelling of that fact and not two that can drift +apart. The width it reports is read from the same constant the walk uses, so the +range it names is always the range it actually probed. + +Written to **stderr**, like every other `os serve` diagnostic: `stdout` carries +JSON-RPC frames whenever the stdio MCP transport is mounted, so nothing but +protocol may go there. It prints only when the search is exhausted — an ordinary +auto-shift and a production boot are both unchanged, byte for byte. diff --git a/packages/cli/src/commands/serve-exhausted-port-search-notice.test.ts b/packages/cli/src/commands/serve-exhausted-port-search-notice.test.ts new file mode 100644 index 0000000000..b1f898fd07 --- /dev/null +++ b/packages/cli/src/commands/serve-exhausted-port-search-notice.test.ts @@ -0,0 +1,265 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #12620 — when the dev port search runs out of ports, `os serve` SAYS SO, + * carrying the message the search itself threw. + * + * ## The defect was a discarded sentence, not a wrong behaviour + * + * `getAvailablePort` throws `Could not find an available port starting from + * ` — a sentence that names the problem exactly. The caller's `catch` used + * to read `// Ignore — fall through and try the requested port` and did exactly + * that: it dropped the sentence and bound `requestedPort` anyway, the port the + * search had just proven was taken. The boot then died on the kernel's raw + * `EADDRINUSE` with nothing anywhere explaining it. + * + * ⛔ The fallthrough itself is NOT changed and this file does not argue for + * changing it. Whether an exhausted search should refuse instead is #11113's + * production/development policy split and belongs to that card. + * + * ## ⛔ Why this file binds no sockets + * + * The obvious test — hold 101 real ports and boot — is the wrong test: slow, + * flaky, and hostile to a shared many-agent container whose ephemeral range is + * already crowded. #12441 measured that contention taking a full CLI suite red + * (`1 failed | 2101 passed`, clean on an isolated re-run), which is why + * `serve-port-bind-probe.test.ts` exists at all. So the SEAM is driven instead: + * `getAvailablePort` now takes its port probe as a parameter, defaulting to the + * real one, and every runtime assertion below drives that parameter. Zero + * sockets, zero spawns. + * + * ⚠️ That parameter is the change that made this card testable at all. Without + * it the exhausted path is reachable only by exhausting a real range, which is + * the test this card's ruling forbids. + * + * ## THE THREE-WAY DISCRIMINATION (the anti-vacuity requirement) + * + * A test asserting only "the notice appears when the search is exhausted" + * passes just as green against code that prints it unconditionally. Three arms, + * each pinned at the level it is actually decidable at: + * + * 1. **exhausted search** → the notice appears, carrying the thrown message. + * RUNTIME, at the seam. + * 2. **ordinary auto-shift** (`port !== requestedPort`, search SUCCEEDED) → + * this notice does not appear; #12543's drift notice does. RUNTIME, at the + * seam — and exactly, not by proxy: a successful search RETURNS, so the + * `catch` never runs and the notice is unreachable by construction. The + * drift half of that arm has its own landed runtime pin in + * `test/serve-port-drift-notice.e2e.test.ts`, which spawns a real boot + * against a real HTTP neighbour; it is not re-spawned here. + * 3. **production branch** (`portAutoShiftAllowed` false) → neither notice; + * the existing `Port … is already in use` line fires. STRUCTURAL, against + * the live source: this notice's only call site is lexically inside the + * `if (portAutoShiftAllowed)` block and the production line is in the + * `else if`, so a production boot cannot reach it. The production line's + * own runtime pin is landed in + * `test/serve-node-env-production-default.e2e.test.ts`. + * + * ⚠️ Arm 3 is deliberately not a fourth spawner file. This package's own + * `vitest.config.ts` records that its 39 spawner files carry 89.4% of its test + * wall at a ~5.5-6.0s floor each, and arms 2 and 3 already hold the landed + * runtime pins named above. What was missing from them was the discrimination + * against THIS notice, and that is what the source-anchored arm supplies. + * + * ## The mutual-exclusion arm, and why it is not decoration + * + * The three notices are also asserted to be pairwise non-matching. One of those + * pairs is load-bearing beyond legibility: `PORT_TAKEN_PATTERNS` in + * `test/helpers/serve-process.ts` turns `/Port (\d+) is already in use/` and + * `/EADDRINUSE[^\n]*?:(\d+)/` into a "port contention" verdict for every + * spawner in this package. This notice names EADDRINUSE in prose on purpose — + * that is what the operator is about to see — so if it ever grew a `:` + * after that word, an exhausted-search boot would be mis-reported as a lost + * port race by an unrelated file. + */ + +import { describe, it, expect } from 'vitest'; +import { readFileSync } from 'node:fs'; +import { resolve } from 'node:path'; +import { fileURLToPath } from 'node:url'; + +import { formatExhaustedPortSearchNotice, getAvailablePort } from './serve.js'; + +/** Seeded from `import.meta.url`, the spelling `check:cross-package-test-inputs` recognises. */ +const HERE = resolve(fileURLToPath(import.meta.url), '..'); + +/** The live source, for the arms decided lexically rather than at runtime. */ +const SERVE_SOURCE = readFileSync(resolve(HERE, 'serve.ts'), 'utf8'); + +/** + * Strip SGR escapes. `chalk` is inert under a non-TTY runner but not + * guaranteed to be, and an assertion that only passes when colour happens to be + * off is a flake waiting for the first person who runs this attached. + */ +const plain = (text: string): string => text.replace(/\u001B\[[0-9;]*m/g, ''); + +/** An arbitrary start port. Nothing is bound, so it only has to be a number. */ +const START = 34_500; + +/** A probe that never finds a free port — the exhausted path, without a socket. */ +function alwaysBusy(): { probe: (port: number) => Promise; probed: number[] } { + const probed: number[] = []; + return { + probed, + probe: async (port: number) => { + probed.push(port); + return false; + }, + }; +} + +/** #12543's notice, as its landed text. Held against the source below so it cannot go stale. */ +const DRIFT_NOTICE = /Port (\d+) is in use — serving on (\d+) instead\./; +/** #11113's production refusal — and the first of `PORT_TAKEN_PATTERNS`. */ +const PRODUCTION_REFUSAL = /Port (\d+) is already in use/; + +describe('#12620: an exhausted port search is announced, in the words the search threw', () => { + it('ARM 1 — the notice CARRIES the thrown message verbatim, as its headline', async () => { + const { probe } = alwaysBusy(); + + const thrown = await getAvailablePort(START, probe).then( + (port) => { + throw new Error(`the search returned ${port} against a probe that never says yes`); + }, + (err: unknown) => err, + ); + + expect(thrown, 'the exhausted search no longer throws').toBeInstanceOf(Error); + const message = (thrown as Error).message; + + // Guard the guard: an empty message would make the assertion below + // vacuously true. + expect(message, 'the thrown message no longer names the start port').toContain(String(START)); + + const notice = plain(formatExhaustedPortSearchNotice(START, thrown)); + + // ⭐ The pin. Not "says something similar" — the error's own text, and as + // the headline, so a reader meets it first rather than digging for it. + expect( + notice.split('\n').find((line) => line.includes('⚠')), + 'the notice paraphrases the thrown message instead of carrying it', + ).toBe(` ⚠ ${message}`); + }); + + it('ARM 1 — the width it reports is the width it actually probed, endpoints included', async () => { + const { probe, probed } = alwaysBusy(); + await getAvailablePort(START, probe).catch((err: unknown) => err); + + // ⚠️ The search is a plain contiguous `port++` walk. A neighbouring card + // described it as skipping ports; it does not, and a notice written from + // that reading would name a range it never looked at. + expect( + probed.every((port, i) => port === START + i), + `the search is no longer contiguous: ${probed.slice(0, 5).join(',')}…`, + ).toBe(true); + expect(probed[0], 'the walk no longer starts where it was asked to').toBe(START); + + // ⭐ The anti-off-by-one arm, and the reason it compares against `probed` + // rather than against a literal: an inaccurate number inside a diagnostic + // that exists to be accurate would be this card's own defect. BOTH terms + // are measured here — how many ports the walk touched, and the last one it + // reached. + const notice = plain(formatExhaustedPortSearchNotice(START, new Error('x'))); + expect(notice).toContain(`${probed.length} ports (${START}–${probed[probed.length - 1]})`); + }); + + it('ARM 2 — a search that SUCCEEDS returns, so the notice is unreachable on the drift path', async () => { + // Busy at the requested port, free at the next one: the ordinary auto-shift. + const probed: number[] = []; + const port = await getAvailablePort(START, async (candidate) => { + probed.push(candidate); + return candidate !== START; + }); + + // The drift really is a drift — without this the arm proves nothing. + expect(port, 'the probe did not produce a shift to discriminate against').not.toBe(START); + expect(port).toBe(START + 1); + expect(probed).toEqual([START, START + 1]); + + // ⭐ THE DISCRIMINATION. `getAvailablePort` RESOLVED, so the caller's + // `catch` — the only place this notice is written — cannot run. The notice + // is not merely absent on this path; it is unreachable. + // + // What fires instead is #12543's drift notice, guarded on + // `port !== requestedPort`: true here, and false on the exhausted path, + // where the assignment never happens at all. Both guards are read from the + // live source rather than described. + expect(SERVE_SOURCE, 'the notice moved out of the catch that guards it').toMatch( + /catch \(searchExhausted\) \{[\s\S]*?printDiagnostic\(formatExhaustedPortSearchNotice\(requestedPort, searchExhausted\)\);/, + ); + expect(SERVE_SOURCE, "#12543's drift notice is no longer gated on a real shift").toContain( + 'if (port !== requestedPort) {', + ); + expect(SERVE_SOURCE, "#12543's drift wording moved; this file's discrimination is stale").toMatch( + /Port \$\{requestedPort\} is in use — serving on \$\{port\} instead\./, + ); + }); + + it('ARM 3 — the notice is inside the auto-shift branch, so production never reaches it', () => { + const autoShift = SERVE_SOURCE.indexOf('if (portAutoShiftAllowed) {'); + const productionBranch = SERVE_SOURCE.indexOf('} else if (!(await isPortAvailable(requestedPort)))'); + const noticeCallSite = SERVE_SOURCE.indexOf('printDiagnostic(formatExhaustedPortSearchNotice('); + const productionLine = SERVE_SOURCE.indexOf('is already in use.'); + + // Every anchor has to exist, or the ordering assertions below compare -1s + // and pass while measuring nothing. + expect(autoShift, 'the `portAutoShiftAllowed` branch head is gone').toBeGreaterThan(-1); + expect(productionBranch, 'the production `else if` is gone').toBeGreaterThan(-1); + expect(noticeCallSite, 'the exhausted-search notice has no call site').toBeGreaterThan(-1); + expect(productionLine, 'the production in-use line is gone').toBeGreaterThan(-1); + + // ⭐ The notice sits between the branch head and the `else if`; the + // production line sits after it. A production boot never enters the block + // this notice lives in. + expect(noticeCallSite).toBeGreaterThan(autoShift); + expect(noticeCallSite).toBeLessThan(productionBranch); + expect(productionLine).toBeGreaterThan(productionBranch); + }); + + it('ARM 3 — the sink is `printDiagnostic`, which is stderr (#7915 stdout purity)', () => { + // `stdout` is the JSON-RPC channel whenever the stdio MCP transport is + // mounted, which is what `serve-stdio-stdout-purity.e2e.test.ts` pins. A + // diagnostic written anywhere else reds that suite from this file. + expect(SERVE_SOURCE).toContain('printDiagnostic(formatExhaustedPortSearchNotice('); + expect(SERVE_SOURCE, '`printDiagnostic` no longer writes to stderr').toMatch( + /const printDiagnostic = \(text = ''\) => \{\s*\n\s*if \(!bootQuiet\) process\.stderr\.write/, + ); + }); + + it('MUTUAL EXCLUSION — the three notices cannot be mistaken for one another', () => { + const notice = plain( + formatExhaustedPortSearchNotice( + START, + new Error(`Could not find an available port starting from ${START}`), + ), + ); + + expect(notice, "the exhausted notice reads as #12543's drift notice").not.toMatch(DRIFT_NOTICE); + + // ⚠️ Load-bearing beyond legibility: this pattern is the first of + // `PORT_TAKEN_PATTERNS` in `test/helpers/serve-process.ts`, which every + // spawner in this package uses to decide that a boot lost a port race. + expect(notice, 'the exhausted notice reads as the production refusal').not.toMatch( + PRODUCTION_REFUSAL, + ); + + // The other half of that helper's pattern. + expect(notice, 'the notice now trips the EADDRINUSE contention pattern').not.toMatch( + /EADDRINUSE[^\n]*?:(\d+)/, + ); + + // …and both patterns are live instruments, not dead regexes: each must + // still match the text it was written for, or the two negatives above + // prove nothing at all. + expect(' ⚠ Port 3000 is in use — serving on 3001 instead.').toMatch(DRIFT_NOTICE); + expect(' ✗ Port 3000 is already in use.').toMatch(PRODUCTION_REFUSAL); + }); + + it('carries a non-Error rejection intact rather than rendering it as [object Object]', () => { + // The probe is injectable, so the seam can now reject with anything. The + // ruling is to carry the text the seam produced, whatever it is. + const notice = plain(formatExhaustedPortSearchNotice(START, 'probe socket exploded')); + expect(notice).toContain('probe socket exploded'); + expect(notice).not.toContain('[object Object]'); + }); +}); diff --git a/packages/cli/src/commands/serve.ts b/packages/cli/src/commands/serve.ts index 906eaf46db..1448d59213 100644 --- a/packages/cli/src/commands/serve.ts +++ b/packages/cli/src/commands/serve.ts @@ -189,18 +189,109 @@ const isPortAvailable = (port: number): Promise => { }); }; -// Helper to find available port (dev convenience — see the gated caller). -const getAvailablePort = async (startPort: number): Promise => { +/** + * How far past `startPort` the dev auto-shift search walks before giving up. + * + * ⚠️ This is a SPAN, not a count: the search probes `startPort` itself and then + * every port up to and including `startPort + PORT_SEARCH_SPAN`, so the number + * of ports actually probed is `PORT_SEARCH_SPAN + 1`. Both numbers are read + * from this one constant — by the loop below and by + * {@link formatExhaustedPortSearchNotice}, which states the range to the + * operator. ⛔ Never hand-write either number anywhere else: an off-by-one + * inside a diagnostic that exists to be accurate is the defect it was added to + * fix (#12620). + */ +const PORT_SEARCH_SPAN = 100; + +/** + * The one thing {@link getAvailablePort} does to the outside world, injectable + * so its exhausted path can be driven without binding a socket. + * + * ⛔ The alternative is a test that holds 101 real ports: slow, flaky, and + * hostile to a shared many-agent container whose ephemeral range is already + * crowded (#12441 measured that contention costing a suite a red run). The + * production call site passes nothing and gets {@link isPortAvailable}. + */ +type PortProbe = (port: number) => Promise; + +/** + * Helper to find available port (dev convenience — see the gated caller). + * + * A plain contiguous `port++` walk: `startPort`, `startPort + 1`, … with **no + * skip**. ⚠️ Nearby cards have described this search as hopping over ports; it + * does not, and a diagnostic written from that reading would be wrong. + */ +export const getAvailablePort = async ( + startPort: number, + probe: PortProbe = isPortAvailable, +): Promise => { let port = startPort; - while (!(await isPortAvailable(port))) { + while (!(await probe(port))) { port++; - if (port > startPort + 100) { - throw new Error(`Could not find an available port starting from ${startPort}`); + if (port > startPort + PORT_SEARCH_SPAN) { + throw new Error(`Could not find an available port starting from ${startPort}`); } } return port; }; +/** + * The notice for a search that ran out of ports (#12620). + * + * ## The fallthrough is deliberate; the SILENCE was the defect + * + * When {@link getAvailablePort} exhausts its span the caller swallows the throw + * and binds `requestedPort` anyway — which is defensible on its own terms: a + * dev whose whole next span is busy arguably wants the requested port attempted + * rather than a hard refusal. ⛔ Whether it should refuse instead is #11113's + * production/development policy split and is deliberately NOT decided here; + * nothing in this function changes what `os serve` does. + * + * What is not defensible is doing it in silence. This is the one shape in the + * whole port policy where the operator gets **neither** half of the legibility + * work this family has landed: the production `Port … is already in use` line + * lives in the `else if` this boot never enters, and #12543's drift notice is + * gated on `port !== requestedPort`, which on this path is false because the + * assignment never happened. The boot then dies on the kernel's raw error with + * the accurate explanation discarded one line earlier. + * + * ## ⭐ It CARRIES the thrown message; it does not paraphrase it + * + * `Could not find an available port starting from ${startPort}` already names + * the problem exactly. Rewriting it into a fresh sentence would create a second + * spelling of one fact, free to drift from the first. So the caught error's own + * `message` is the headline, verbatim, and every other line here adds a + * DIFFERENT fact: how wide the search was, what happens next, and what to do. + * + * `cause` is typed `unknown` and rendered defensively rather than narrowed to + * `Error`: the point is to carry whatever text the seam actually produced, and + * a probe that rejects with a non-`Error` must not turn this notice into + * `[object Object]`. + * + * ## CHANNEL — measured, not chosen (#7915) + * + * The caller writes this through `printDiagnostic`, straight to **stderr**. + * `stdout` is the JSON-RPC channel whenever the stdio MCP transport is mounted, + * where one non-frame line reaches a conforming client as a transport error; + * `serve-stdio-stdout-purity.e2e.test.ts` exists to pin exactly that. + */ +export function formatExhaustedPortSearchNotice(requestedPort: number, cause: unknown): string { + // ⭐ The thrown text, carried — not restated. See the docblock. + const thrown = cause instanceof Error ? cause.message : String(cause); + const lastProbed = requestedPort + PORT_SEARCH_SPAN; + const probedCount = PORT_SEARCH_SPAN + 1; + return ( + '\n' + + chalk.yellow(` ⚠ ${thrown}\n`) + + chalk.dim(` Development auto-shift probed ${probedCount} ports (${requestedPort}–${lastProbed}) and every\n`) + + chalk.dim(` one was busy, so this server is falling back to ${requestedPort} — the port the\n`) + + chalk.dim(' search has just proven is taken. The bind that follows will almost\n') + + chalk.dim(' certainly fail with a raw EADDRINUSE from the kernel, and this notice\n') + + chalk.dim(' is the only place that says why.\n') + + chalk.dim(` Free a port in ${requestedPort}–${lastProbed}, or pick another via PORT= (or --port ).`) + ); +} + /** * The IDENTITIES a capability provider registers under: full `plugin.name` ids * (`com.objectstack.mcp`) and/or exported class names (`MCPServerPlugin`). @@ -1371,8 +1462,23 @@ export default class Serve extends Command { if (portAutoShiftAllowed) { try { port = await getAvailablePort(requestedPort); - } catch { - // Ignore — fall through and try the requested port. + } catch (searchExhausted) { + // ── The FALLTHROUGH is CORRECT; its SILENCE was the defect (#12620) ── + // This `catch` used to read `// Ignore — fall through and try the + // requested port`, and it did exactly that: it discarded an error whose + // message already named the problem exactly, then fell through and + // bound the port the search had just proven was taken. The boot died on + // the kernel's raw EADDRINUSE with nothing anywhere explaining it — + // this is the one path in the whole port policy that reaches NEITHER + // half of the family's legibility work. The `else if` below owns the + // production wording and is never entered here; #12543's drift notice + // sits under `port !== requestedPort` a few lines down and is false + // here, because the assignment above threw before it could happen. + // + // ⛔ The fallthrough itself stays. Whether an exhausted search should + // refuse instead is #11113's production/development policy split, and + // answering it here would be answering a different card. + printDiagnostic(formatExhaustedPortSearchNotice(requestedPort, searchExhausted)); } if (port !== requestedPort) { // ── The shift is CORRECT; its SILENCE was the defect (#12543) ──── From 8a0a8538802b4bb4f1cb6fa053598c92684a4b7f Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 27 Aug 2026 04:15:14 +0000 Subject: [PATCH 2/3] fix(cli): make the span claim only for the failure it is true of (#12620) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `catch` this notice feeds catches EVERY rejection from the port search, not only an exhausted walk — and the notice asserted a probed range unconditionally. `isPortAvailable` rejects synchronously with ERR_SOCKET_BAD_PORT for any port outside 0-65535, which is reachable two ways: a walk that starts high enough to cross the ceiling, and `--port` text that `parseInt` turns into NaN. Measured against `net` rather than supposed — `listen()` throws for both, inside the probe's promise executor. On those paths nothing was exhausted, so the notice would have printed `probed 101 ports (NaN-NaN)`: an inaccurate diagnostic inside the diagnostic added to stop exactly that. The walk now throws a `PortSearchExhaustedError` carrying its own startPort, lastPort and probedCount, and the span sentence is reachable only through it. Its message is unchanged, so the sentence the notice carries verbatim is untouched and the exhausted-path output is byte-identical to before; the type says WHICH failure this is, not the same thing differently. The numbers are read off the error the walk recorded them on rather than re-derived from `requestedPort`, so there is no second source that could disagree. Any other rejection still carries its own message as the headline — that ruling does not bend by branch — with a body that claims no range. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01UjujZN219uFzBhSYfMykCd --- ...serve-exhausted-port-search-notice.test.ts | 46 ++++++++++++--- packages/cli/src/commands/serve.ts | 56 +++++++++++++++++-- 2 files changed, 87 insertions(+), 15 deletions(-) diff --git a/packages/cli/src/commands/serve-exhausted-port-search-notice.test.ts b/packages/cli/src/commands/serve-exhausted-port-search-notice.test.ts index b1f898fd07..a15950eded 100644 --- a/packages/cli/src/commands/serve-exhausted-port-search-notice.test.ts +++ b/packages/cli/src/commands/serve-exhausted-port-search-notice.test.ts @@ -143,7 +143,7 @@ describe('#12620: an exhausted port search is announced, in the words the search it('ARM 1 — the width it reports is the width it actually probed, endpoints included', async () => { const { probe, probed } = alwaysBusy(); - await getAvailablePort(START, probe).catch((err: unknown) => err); + const thrown = await getAvailablePort(START, probe).catch((err: unknown) => err); // ⚠️ The search is a plain contiguous `port++` walk. A neighbouring card // described it as skipping ports; it does not, and a notice written from @@ -159,10 +159,41 @@ describe('#12620: an exhausted port search is announced, in the words the search // that exists to be accurate would be this card's own defect. BOTH terms // are measured here — how many ports the walk touched, and the last one it // reached. - const notice = plain(formatExhaustedPortSearchNotice(START, new Error('x'))); + const notice = plain(formatExhaustedPortSearchNotice(START, thrown)); expect(notice).toContain(`${probed.length} ports (${START}–${probed[probed.length - 1]})`); }); + it('makes NO span claim for a rejection that is not an exhausted walk', async () => { + // ⚠️ The `catch` in serve.ts catches every rejection, not only exhaustion. + // `isPortAvailable` rejects synchronously with ERR_SOCKET_BAD_PORT for any + // port outside 0–65535 — reachable when the walk crosses the ceiling, and + // when `--port` text parses to NaN. Measured, not supposed: `net`'s + // `listen()` throws for both, inside the probe's promise executor. + // + // ⭐ On those paths nothing was probed, so a body claiming a range would + // print `NaN–NaN` and assert a search that never ran — an inaccurate + // diagnostic inside the diagnostic added to stop exactly that. + const badPort = new RangeError('options.port should be >= 0 and < 65536. Received NaN.'); + const notice = plain(formatExhaustedPortSearchNotice(Number.NaN, badPort)); + + // The thrown text is still carried — that ruling does not bend by branch. + expect(notice).toContain(badPort.message); + // …but the claim that is false here is simply not made. + expect(notice, 'a span was claimed for a search that never walked one').not.toMatch( + /probed \d+ ports/, + ); + expect(notice, 'the notice printed a NaN range').not.toContain('NaN–'); + + // Anti-vacuity for the two negatives above: the real exhausted notice DOES + // make both claims, so their absence here is a discrimination and not a + // regex that stopped matching anything. + const { probe } = alwaysBusy(); + const real = await getAvailablePort(START, probe).catch((err: unknown) => err); + const exhausted = plain(formatExhaustedPortSearchNotice(START, real)); + expect(exhausted).toMatch(/probed \d+ ports/); + expect(exhausted).toContain(`${START}–`); + }); + it('ARM 2 — a search that SUCCEEDS returns, so the notice is unreachable on the drift path', async () => { // Busy at the requested port, free at the next one: the ordinary auto-shift. const probed: number[] = []; @@ -226,13 +257,10 @@ describe('#12620: an exhausted port search is announced, in the words the search ); }); - it('MUTUAL EXCLUSION — the three notices cannot be mistaken for one another', () => { - const notice = plain( - formatExhaustedPortSearchNotice( - START, - new Error(`Could not find an available port starting from ${START}`), - ), - ); + it('MUTUAL EXCLUSION — the three notices cannot be mistaken for one another', async () => { + const { probe } = alwaysBusy(); + const thrown = await getAvailablePort(START, probe).catch((err: unknown) => err); + const notice = plain(formatExhaustedPortSearchNotice(START, thrown)); expect(notice, "the exhausted notice reads as #12543's drift notice").not.toMatch(DRIFT_NOTICE); diff --git a/packages/cli/src/commands/serve.ts b/packages/cli/src/commands/serve.ts index 1448d59213..05436e8d6e 100644 --- a/packages/cli/src/commands/serve.ts +++ b/packages/cli/src/commands/serve.ts @@ -221,6 +221,27 @@ type PortProbe = (port: number) => Promise; * skip**. ⚠️ Nearby cards have described this search as hopping over ports; it * does not, and a diagnostic written from that reading would be wrong. */ +export class PortSearchExhaustedError extends Error { + /** The first port probed — the walk's own record of it, not the caller's. */ + readonly startPort: number; + /** The last port probed. Inclusive, and derived from {@link PORT_SEARCH_SPAN}. */ + readonly lastPort: number; + /** How many ports were probed: every one of them, and every one busy. */ + readonly probedCount: number; + + constructor(startPort: number) { + // ⭐ The message is UNCHANGED from the plain `Error` this replaces. It is + // the sentence the notice carries verbatim, so it is not the class's to + // reword — the type is added to say WHICH failure this is, not to say it + // differently. + super(`Could not find an available port starting from ${startPort}`); + this.name = 'PortSearchExhaustedError'; + this.startPort = startPort; + this.lastPort = startPort + PORT_SEARCH_SPAN; + this.probedCount = PORT_SEARCH_SPAN + 1; + } +} + export const getAvailablePort = async ( startPort: number, probe: PortProbe = isPortAvailable, @@ -229,7 +250,7 @@ export const getAvailablePort = async ( while (!(await probe(port))) { port++; if (port > startPort + PORT_SEARCH_SPAN) { - throw new Error(`Could not find an available port starting from ${startPort}`); + throw new PortSearchExhaustedError(startPort); } } return port; @@ -278,17 +299,40 @@ export const getAvailablePort = async ( export function formatExhaustedPortSearchNotice(requestedPort: number, cause: unknown): string { // ⭐ The thrown text, carried — not restated. See the docblock. const thrown = cause instanceof Error ? cause.message : String(cause); - const lastProbed = requestedPort + PORT_SEARCH_SPAN; - const probedCount = PORT_SEARCH_SPAN + 1; + + // ⚠️ The `catch` this feeds catches EVERY rejection, not only an exhausted + // walk, and the two cannot share a body. `isPortAvailable` rejects + // synchronously with `ERR_SOCKET_BAD_PORT` whenever it is handed a port + // outside 0–65535 — reachable two ways: a walk that starts high enough to + // cross the ceiling, and `--port` text that `parseInt` turns into `NaN`. On + // those paths nothing was exhausted, so a body claiming a probed range would + // print `NaN–NaN` and assert a search that never ran. A diagnostic that + // exists to be accurate does not get to guess, so the span sentence is + // reached only through the error that actually carries the span. + if (!(cause instanceof PortSearchExhaustedError)) { + return ( + '\n' + + chalk.yellow(` ⚠ ${thrown}\n`) + + chalk.dim(' The development port search could not run to completion, so this server\n') + + chalk.dim(` is falling back to the port it was asked for (${requestedPort}). If that port is\n`) + + chalk.dim(' taken the bind that follows will fail with a raw EADDRINUSE from the\n') + + chalk.dim(' kernel, and this notice is the only place that says why.') + ); + } + + // Every number below is the ERROR's own, recorded by the walk that failed — + // never re-derived here from `requestedPort`, which is a second source that + // could disagree with the first. + const { startPort, lastPort, probedCount } = cause; return ( '\n' + chalk.yellow(` ⚠ ${thrown}\n`) - + chalk.dim(` Development auto-shift probed ${probedCount} ports (${requestedPort}–${lastProbed}) and every\n`) - + chalk.dim(` one was busy, so this server is falling back to ${requestedPort} — the port the\n`) + + chalk.dim(` Development auto-shift probed ${probedCount} ports (${startPort}–${lastPort}) and every\n`) + + chalk.dim(` one was busy, so this server is falling back to ${startPort} — the port the\n`) + chalk.dim(' search has just proven is taken. The bind that follows will almost\n') + chalk.dim(' certainly fail with a raw EADDRINUSE from the kernel, and this notice\n') + chalk.dim(' is the only place that says why.\n') - + chalk.dim(` Free a port in ${requestedPort}–${lastProbed}, or pick another via PORT= (or --port ).`) + + chalk.dim(` Free a port in ${startPort}–${lastPort}, or pick another via PORT= (or --port ).`) ); } From 0ea410a0c5512285aed39f6cb2d7df2e56a1ba46 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 27 Aug 2026 04:18:56 +0000 Subject: [PATCH 3/3] test(cli): the no-span-claim arm was blind to the regression it exists to catch (#12620) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An ablation that neutered the `instanceof PortSearchExhaustedError` guard — so the span body runs for EVERY rejection, the exact regression the guard was added to prevent — left all eight cases GREEN. The predicate was `/probed \d+ ports/`. Removing the guard does not produce a wrong NUMBER; it produces `undefined`, because the span body reads startPort, lastPort and probedCount off an error that does not carry them. `\d+` does not match `probed undefined ports`, so the assertion passed against the one shape it was written for. The `NaN–` check missed it for the same reason. Widened to `/probed/` plus an explicit placeholder check, and the reasoning is recorded at the assertion so the narrow spelling does not come back. The positive control on the real exhausted notice is unchanged, so the negatives still discriminate rather than matching nothing. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01UjujZN219uFzBhSYfMykCd --- ...serve-exhausted-port-search-notice.test.ts | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/packages/cli/src/commands/serve-exhausted-port-search-notice.test.ts b/packages/cli/src/commands/serve-exhausted-port-search-notice.test.ts index a15950eded..b165e5968f 100644 --- a/packages/cli/src/commands/serve-exhausted-port-search-notice.test.ts +++ b/packages/cli/src/commands/serve-exhausted-port-search-notice.test.ts @@ -178,11 +178,22 @@ describe('#12620: an exhausted port search is announced, in the words the search // The thrown text is still carried — that ruling does not bend by branch. expect(notice).toContain(badPort.message); + // …but the claim that is false here is simply not made. - expect(notice, 'a span was claimed for a search that never walked one').not.toMatch( - /probed \d+ ports/, - ); - expect(notice, 'the notice printed a NaN range').not.toContain('NaN–'); + // + // ⚠️ The predicate here is `/probed/`, NOT `/probed \d+ ports/`, and the + // difference is the whole test. Removing the branch guard does not produce + // a WRONG number — it produces `undefined`, because the span body reads + // `startPort`/`lastPort`/`probedCount` off an error that does not carry + // them. A `\d+` pattern does not match `probed undefined ports`, so the + // narrow spelling passed against precisely the regression this case exists + // to catch. Measured: an ablation that neutered the guard left all eight + // cases green. + expect(notice, 'a span was claimed for a search that never walked one').not.toMatch(/probed/); + expect( + notice, + 'the notice rendered a placeholder where a number belongs', + ).not.toMatch(/undefined|NaN–/); // Anti-vacuity for the two negatives above: the real exhausted notice DOES // make both claims, so their absence here is a discrimination and not a