diff --git a/.changeset/cli-serve-port-input-validation.md b/.changeset/cli-serve-port-input-validation.md new file mode 100644 index 0000000000..e084137aba --- /dev/null +++ b/.changeset/cli-serve-port-input-validation.md @@ -0,0 +1,51 @@ +--- +"@objectstack/cli": minor +--- + +fix(cli): `os serve` refuses a port that cannot be a port, naming what the operator set (#12662) + +`--port` was a string flag whose only consumer was a bare `parseInt`. `os serve +--port abc` therefore became `NaN`, travelled the whole port policy untouched, +and reached the real `listen()` — which refused it at the socket layer: + +``` +ERR_SOCKET_BAD_PORT: options.port should be >= 0 and < 65536. Received type number (NaN). +``` + +An operator who mistyped a flag got back an error naming an internal option, +raised from a code path with no connection to the thing they typed. `--port +99999` parses fine and died in exactly the same place, and `PORT=abc` / +`OS_PORT=abc` are the same defect through a different door. + +The value is now checked before anything is done with it: + +``` + ✗ Invalid port: OS_PORT="abc" + A port must be a whole number from 0 to 65535 — 0 is legal, and + asks the kernel for any free port. Nothing was started, and no socket + was opened. + Correct OS_PORT in this process's environment (for example OS_PORT=3000), + or override it with --port 3000. +``` + +It names **which** input was used — `--port`, `PORT` or `OS_PORT` — because a +refusal that only said "invalid port" would repeat the defect one level up. The +range it states is interpolated from the bounds the code enforces, so the +sentence cannot drift from the check. `0` is accepted: `listen(0)` binds a +kernel-assigned port, so refusing it would have broken a working input in the +name of validating it, and the ceiling is 65535 — one less than the `< 65536` +the kernel's own message names. + +The check sits ahead of the port-conflict policy, so all three boot paths (the +development auto-shift, the production refusal, and a boot that enters neither) +are covered by one guard, and all three inputs are covered with it: `PORT` and +`OS_PORT` never reach flag parsing at all — they are read by the flag's +`default`, which oclif never runs a flag's parser over. + +**No value that boots today is refused.** `parseInt` remains the reader, so +every spelling it tolerates — `" 3000"` with the leading whitespace production +environments carry, `"3000.0"`, `"0x0BB8"`, `"+3000"` — still boots, on the same +port, byte for byte. Only the values that used to die at the socket are refused, +and now they are refused early, in the operator's own vocabulary. Written to +**stderr** like every other `os serve` diagnostic: `stdout` carries JSON-RPC +frames whenever the stdio MCP transport is mounted. 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 b165e5968f..ea188e3f02 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 @@ -166,9 +166,12 @@ describe('#12620: an exhausted port search is announced, in the words the search 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. + // port outside 0–65535 — reachable when the walk crosses the ceiling. + // Measured, not supposed: `net`'s `listen()` throws there, inside the + // probe's promise executor. (It used to be reachable a second way, from + // `--port` text that parsed to NaN; #12662 refuses that value before the + // port policy runs, so the crossing walk is the only route left. This case + // is unaffected either way: it constructs the rejection directly.) // // ⭐ 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 diff --git a/packages/cli/src/commands/serve-port-validation.test.ts b/packages/cli/src/commands/serve-port-validation.test.ts new file mode 100644 index 0000000000..78c89140e7 --- /dev/null +++ b/packages/cli/src/commands/serve-port-validation.test.ts @@ -0,0 +1,378 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #12662 — a port value that cannot be a port is refused HERE, in the + * operator's own vocabulary, instead of dying at the socket layer. + * + * ## The defect + * + * `--port` was a `Flags.string` whose only consumer was a bare + * `parseInt(flags.port)`. `--port abc` therefore became `NaN`, travelled the + * whole port policy, and reached the real `listen()`, which refused it with + * `ERR_SOCKET_BAD_PORT: options.port should be >= 0 and < 65536` — an error + * naming an internal option, raised from a code path with no connection to the + * flag the operator typed. `--port 99999` parses fine and died identically. + * `PORT=abc` and `OS_PORT=abc` are the same defect through a different door. + * + * ## Why this file binds no sockets + * + * Validation belongs BEFORE the socket, so a test that needs a socket to + * observe it is testing the wrong layer. Everything below drives the three + * exported pure functions and the live source text. Zero sockets, zero spawns — + * the ruling `serve-exhausted-port-search-notice.test.ts` records for its own + * path, for the same measured reason (#12441: real-port contention took a full + * CLI suite red in this container). + * + * ## The arms, and what each one actually decides + * + * 1. **The three reported inputs** — `--port`, `PORT`, `OS_PORT` — are each + * refused, and the refusal NAMES the one that was used. A refusal saying + * only "invalid port" would repeat this card's own defect one level up. + * 2. **The bounds are the measured bounds.** `0` is accepted (it is a real + * request — `listen(0)` binds a kernel-assigned port), and the ceiling is + * 65535, not the 65536 the kernel's message names. + * 3. **Nothing that boots today is refused.** The `TODAY` table is the + * executable form of that claim, and it is the arm that fails the moment + * someone "tightens" this to what `Flags.integer` accepts — which refuses + * `" 3000"`, `"3000.0"`, `"0x0BB8"`, `"+3000"` and `"3e3"`, every one of + * which boots a server today. That would narrow a published CLI's accepted + * input, which is a contract question and not this card's to answer; this + * arm is where such a change has to come and argue. + * 4. **The range the message states is the range the code enforces** — read + * back out of the validator rather than out of a constant, so a + * hand-written second copy of either bound reds here. + * 5. **The source naming mirrors `readEnvWithDeprecation`** and is pinned + * against it, empty-string case included. + * 6. **Placement**, decided against the live source: the guard precedes the + * `portAutoShiftAllowed` branch, so all three boot paths are downstream of + * one check, and it writes through `printDiagnostic` (stderr, #7915). + * 7. **Mutual exclusion** from the three sibling port notices — two of those + * patterns are `PORT_TAKEN_PATTERNS` in `test/helpers/serve-process.ts`, + * which every spawner in this package uses to decide a boot lost a port + * race. A refusal tripping them would be mis-reported as contention. + */ + +import { describe, it, expect, afterEach } from 'vitest'; +import { readFileSync } from 'node:fs'; +import { resolve } from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { readEnvWithDeprecation } from '@objectstack/types'; + +import { + parseRequestedPort, + describePortSource, + formatInvalidPortNotice, + type PortInputSource, +} 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. + * + * The ESC byte is built with `String.fromCharCode` rather than written: this + * repo refuses raw control bytes in source (`check:nul-bytes`), and an escape + * spelling inside a regex literal is exactly the thing an editing tool + * materialises into a real byte. + */ +const SGR = new RegExp(`${String.fromCharCode(27)}\\[[0-9;]*m`, 'g'); +const plain = (text: string): string => text.replace(SGR, ''); + +/** + * What `os serve` does with each spelling TODAY, on `main`, before this change. + * + * `parsed` is `parseInt(raw)` — the reader this card keeps. `bindsToday` is + * whether that number reaches a bound socket, MEASURED with + * `net.createServer().listen(v)` on this checkout (Node v22.22.2): + * + * ``` + * listen(0) → OK (bound 43025, kernel-assigned) listen(65536) → ERR_SOCKET_BAD_PORT + * listen(3) → OK listen(99999) → ERR_SOCKET_BAD_PORT + * listen(65535) → OK listen(-1) → ERR_SOCKET_BAD_PORT + * listen(NaN) → ERR_SOCKET_BAD_PORT + * ``` + * + * `3e3` is in this table on purpose and its `parsed` is **3**, not 3000: + * `parseInt` stops at the `e`. That boot succeeds today on a port the operator + * never named — a silent coercion, a different defect from this one, and + * preserved here rather than repaired, because repairing it would narrow what + * boots. + */ +/** + * Does `notice` name this exact source spelling? + * + * Substring containment is not enough in one direction, and it is not a + * quibble: `OS_PORT="abc"` CONTAINS `PORT="abc"`, so a plain + * `toContain`/`not.toContain` pair reports the OS_PORT refusal as ALSO naming + * PORT — measured, it reds this file's first arm. The anchor is WIDENED (a + * preceding `_` disqualifies the match) rather than the assertion loosened: a + * message that really did name both sources would still carry a `PORT=` with no + * `_` in front of it, and would still be caught. + */ +const names = (notice: string, spelled: string): boolean => + new RegExp(`(? = [ + { raw: '3000', parsed: 3000, bindsToday: true }, + { raw: '0', parsed: 0, bindsToday: true, note: 'kernel-assigned — legal, and a floor of 1 would refuse it' }, + { raw: '1', parsed: 1, bindsToday: true }, + { raw: '65535', parsed: 65535, bindsToday: true, note: 'the ceiling itself' }, + { raw: ' 3000', parsed: 3000, bindsToday: true, note: 'production env vars carry whitespace' }, + { raw: '3000 ', parsed: 3000, bindsToday: true }, + { raw: '3000.0', parsed: 3000, bindsToday: true }, + { raw: '0x0BB8', parsed: 3000, bindsToday: true }, + { raw: '+3000', parsed: 3000, bindsToday: true }, + { raw: '08080', parsed: 8080, bindsToday: true }, + { raw: '3e3', parsed: 3, bindsToday: true, note: 'binds 3, NOT 3000 — preserved, not repaired' }, + { raw: 'abc', parsed: Number.NaN, bindsToday: false, note: "the card's own repro" }, + { raw: '', parsed: Number.NaN, bindsToday: false, note: "OS_PORT='' is DEFINED, so no fallback to 3000" }, + { raw: ' ', parsed: Number.NaN, bindsToday: false }, + { raw: '65536', parsed: 65536, bindsToday: false, note: "the kernel's own `< 65536`, off by one" }, + { raw: '99999', parsed: 99999, bindsToday: false, note: "the card's second repro" }, + { raw: '-1', parsed: -1, bindsToday: false }, +]; + +describe('#12662: an invalid port is refused before the socket, naming what the operator set', () => { + afterEach(() => { + delete process.env.OS_PORT; + delete process.env.PORT; + }); + + it('refuses the three reported inputs, and NAMES the one that was used', () => { + const cases: Array<{ source: PortInputSource; expected: string }> = [ + { source: '--port', expected: '--port "abc"' }, + { source: 'PORT', expected: 'PORT="abc"' }, + { source: 'OS_PORT', expected: 'OS_PORT="abc"' }, + ]; + + expect(parseRequestedPort('abc'), '`abc` is no longer refused').toBeNull(); + + for (const { source, expected } of cases) { + const notice = plain(formatInvalidPortNotice('abc', source)); + expect(notice, `the refusal does not name ${source}`).toContain(expected); + expect(names(notice, expected), `the ${source} spelling is not matchable`).toBe(true); + + // The discrimination, not merely the presence: naming ONE source means + // not naming the others. Without this, a message listing all three + // ("--port / PORT / OS_PORT is invalid") would satisfy every assertion + // above while repeating the defect the card is about — the operator would + // still have to work out which one is theirs. + for (const other of cases) { + if (other.source === source) continue; + expect( + names(notice, other.expected), + `the refusal for ${source} also names ${other.source}`, + ).toBe(false); + } + } + }); + + it('keeps `--port 99999` from the socket the way `--port abc` is kept from it', () => { + // The card's second repro: numerically fine, unbindable, and today it dies + // at the same place with the same raw error. + expect(parseRequestedPort('99999')).toBeNull(); + const notice = plain(formatInvalidPortNotice('99999', '--port')); + expect(notice).toContain('--port "99999"'); + // The refusal must not read as a typo diagnosis: the value IS a number. + expect(notice).toContain('65535'); + }); + + it('accepts the MEASURED bounds — 0 is a request, and the ceiling is 65535', () => { + // `0` is the trap in the low direction. `listen(0)` binds a kernel-assigned + // port, so `os serve --port 0` boots today; a floor of 1 would have refused + // a working input in the name of validating it. + expect(parseRequestedPort('0'), '0 is no longer accepted — a working input was refused').toBe(0); + + // And the high one: the kernel's own message says `< 65536`, an exclusive + // bound. The largest port that binds is one less. + expect(parseRequestedPort('65535')).toBe(65535); + expect( + parseRequestedPort('65536'), + '65536 was taken from the message instead of measured', + ).toBeNull(); + expect(parseRequestedPort('-1')).toBeNull(); + }); + + it('refuses NOTHING that boots today — the whole table, both directions', () => { + for (const { raw, parsed, bindsToday, note } of TODAY) { + const label = `${JSON.stringify(raw)}${note ? ` (${note})` : ''}`; + + // Guard the table first. If `parseInt` ever stopped producing these + // values, every verdict below would be measuring something else while + // still passing. + if (Number.isNaN(parsed)) { + expect(parseInt(raw), `the table's parseInt record is stale for ${label}`).toBeNaN(); + } else { + expect(parseInt(raw), `the table's parseInt record is stale for ${label}`).toBe(parsed); + } + + const verdict = parseRequestedPort(raw); + if (bindsToday) { + // THE ANTI-NARROWING PIN. Not "is not null" — the SAME port, so a + // repair that accepts the value but re-interprets it is caught too. + expect( + verdict, + `${label} boots today and is now refused — this narrows the accepted input`, + ).toBe(parsed); + } else { + expect( + verdict, + `${label} dies at listen() today and is still not refused here`, + ).toBeNull(); + } + } + + // Anti-vacuity for the loop: both verdicts have to occur in it, or a table + // that drifted to all-accept (or all-refuse) would pass silently. + expect(TODAY.filter((row) => row.bindsToday).length).toBeGreaterThan(1); + expect(TODAY.filter((row) => !row.bindsToday).length).toBeGreaterThan(1); + }); + + it('states the range it ENFORCES — read back from the validator, not from a constant', () => { + // The bounds are recovered by ASKING the validator, so this arm cannot be + // satisfied by a message that hand-wrote its own copy of them — the failure + // mode this card's own family (#12620's `PORT_SEARCH_SPAN`) exists to + // prevent. Import the constants and the pin would only prove the message + // agrees with itself. + let floor: number | null = null; + let ceiling: number | null = null; + for (let candidate = -1; candidate <= 65_540; candidate++) { + if (parseRequestedPort(String(candidate)) === null) continue; + if (floor === null) floor = candidate; + ceiling = candidate; + } + + expect(floor, 'the validator accepts nothing at all').not.toBeNull(); + expect(ceiling).not.toBeNull(); + + const notice = plain(formatInvalidPortNotice('abc', '--port')); + expect( + notice, + `the message names a range other than the enforced ${floor}-${ceiling}`, + ).toContain(`from ${floor} to ${ceiling}`); + }); + + it('names the env var by the same precedence `readEnvWithDeprecation` reads it', () => { + // `describePortSource` MIRRORS that precedence, and a mirror drifts. This is + // the pin: for every combination, the name the refusal would print is the + // name the value actually came from. + const combinations: Array<{ OS_PORT?: string; PORT?: string }> = [ + { OS_PORT: '9001', PORT: '9002' }, + { OS_PORT: '9001' }, + { PORT: '9002' }, + // The case a truthiness check gets wrong: an empty `OS_PORT` is DEFINED, + // so it wins, and `readEnvWithDeprecation` returns `''` rather than + // falling through to `PORT` (or to the 3000 default). + { OS_PORT: '', PORT: '9002' }, + ]; + + for (const combination of combinations) { + delete process.env.OS_PORT; + delete process.env.PORT; + Object.assign(process.env, combination); + + const source = describePortSource(true, process.env); + const value = readEnvWithDeprecation('OS_PORT', 'PORT', { silent: true }); + + expect(source, `no env var named for ${JSON.stringify(combination)}`).not.toBe( + 'the built-in default', + ); + expect( + process.env[source as 'OS_PORT' | 'PORT'], + `the refusal would name ${source}, but the value came from elsewhere`, + ).toBe(value); + } + + // The two ends of the discriminator itself. + expect(describePortSource(false, { PORT: '9002' })).toBe('--port'); + expect(describePortSource(true, {})).toBe('the built-in default'); + }); + + it('sits ahead of the port policy, so all three boot paths are downstream of one check', () => { + const guard = SERVE_SOURCE.indexOf('const parsedPort = parseRequestedPort(flags.port);'); + const autoShift = SERVE_SOURCE.indexOf('if (portAutoShiftAllowed) {'); + const productionBranch = SERVE_SOURCE.indexOf( + '} else if (!(await isPortAvailable(requestedPort)))', + ); + + // Every anchor has to exist, or the ordering assertions below compare -1s + // and pass while measuring nothing. + expect(guard, 'the port guard has no call site').toBeGreaterThan(-1); + expect(autoShift, 'the `portAutoShiftAllowed` branch head is gone').toBeGreaterThan(-1); + expect(productionBranch, 'the production `else if` is gone').toBeGreaterThan(-1); + + // The coverage argument, as an assertion: dev auto-shift, production, and + // the boot that enters neither all run AFTER this line. A guard that + // migrated into either branch would cover that branch alone — precisely + // what a `Flags.integer` repair could not fix for `PORT`/`OS_PORT` either. + expect(guard).toBeLessThan(autoShift); + expect(guard).toBeLessThan(productionBranch); + + // …and it refuses before anything binds: the first probe of the requested + // port sits inside the branch that follows the guard. + expect(SERVE_SOURCE.indexOf('await isPortAvailable(requestedPort)')).toBeGreaterThan(guard); + }); + + it('writes the refusal through `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 + // refusal written anywhere else reds that suite from this file. + expect(SERVE_SOURCE).toContain( + 'printDiagnostic(formatInvalidPortNotice(flags.port, portSource));', + ); + expect(SERVE_SOURCE, '`printDiagnostic` no longer writes to stderr').toMatch( + /const printDiagnostic = \(text = ''\) => \{\s*\n\s*if \(!bootQuiet\) process\.stderr\.write/, + ); + }); + + it('cannot be mistaken for the three notices it sits beside', () => { + const notice = plain(formatInvalidPortNotice('abc', 'PORT')); + + /** #12543's drift notice. */ + 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/; + + expect(notice, "the invalid-port refusal reads as #12543's drift notice").not.toMatch( + DRIFT_NOTICE, + ); + expect(notice, 'the invalid-port refusal reads as the production refusal').not.toMatch( + PRODUCTION_REFUSAL, + ); + + // Load-bearing beyond legibility: `PORT_TAKEN_PATTERNS` in + // `test/helpers/serve-process.ts` turns these into a "port contention" + // verdict for every spawner in this package. A refusal that tripped one + // would be reported as a lost port race — a boot failing for a reason it + // did not fail for. + expect(notice, 'the refusal now trips the EADDRINUSE contention pattern').not.toMatch( + /EADDRINUSE[^\n]*?:(\d+)/, + ); + expect(notice, "the refusal claims a span it never walked (#12620's notice)").not.toMatch( + /probed/, + ); + + // …and the patterns are live instruments, not dead regexes: each must still + // match the text it was written for, or the negatives prove nothing. + expect(' Port 3000 is in use — serving on 3001 instead.').toMatch(DRIFT_NOTICE); + expect(' Port 3000 is already in use.').toMatch(PRODUCTION_REFUSAL); + }); + + it('renders the raw text so whitespace and control bytes are visible, not pasted', () => { + // The whitespace case is the one an operator stares at without seeing. It is + // also NOT refused (see the table) — this covers only how a value that IS + // refused is shown. + expect(plain(formatInvalidPortNotice(' abc ', 'PORT'))).toContain('PORT=" abc "'); + + // A control byte is escaped rather than written to the terminal. Built with + // `String.fromCharCode` so this file carries no raw control byte itself. + const withControlByte = `30${String.fromCharCode(0)}0`; + expect(plain(formatInvalidPortNotice(withControlByte, 'PORT'))).toContain('\\u0000'); + }); +}); diff --git a/packages/cli/src/commands/serve.ts b/packages/cli/src/commands/serve.ts index 05436e8d6e..fcd9489562 100644 --- a/packages/cli/src/commands/serve.ts +++ b/packages/cli/src/commands/serve.ts @@ -303,9 +303,12 @@ export function formatExhaustedPortSearchNotice(requestedPort: number, cause: un // ⚠️ 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 + // outside 0–65535 — reachable by a walk that starts high enough to cross the + // ceiling. (⚠️ It used to be reachable a second way, from `--port` text that + // `parseInt` turned into `NaN`; since #12662 that value is refused before + // this policy runs, so a crossing walk is the only route left. The branch + // below is unchanged: the walk route still reaches it.) On that path + // 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. @@ -336,6 +339,181 @@ export function formatExhaustedPortSearchNotice(requestedPort: number, cause: un ); } +/** + * The port values a real `listen()` accepts — MEASURED here, not copied from + * the kernel's error text (#12662). + * + * Measured in this checkout (Node v22.22.2), `net.createServer().listen(v)`: + * + * ``` + * listen(0) → OK, bound 43025 ← kernel-assigned: 0 is a REQUEST, not an error + * listen(65535) → OK, bound 65535 + * listen(65536) → ERR_SOCKET_BAD_PORT: options.port should be >= 0 and < 65536 + * listen(-1) → ERR_SOCKET_BAD_PORT + * listen(NaN) → ERR_SOCKET_BAD_PORT + * listen(3000.5) → ERR_SOCKET_BAD_PORT + * ``` + * + * ⚠️ Two traps, and both are why these numbers are measured rather than read + * off the message. **`0` is legal** — a floor of `1` would refuse a value that + * boots today (`os serve --port 0` binds a kernel-assigned port). And the + * ceiling is **65535, not 65536**: the kernel's own sentence says `< 65536`, + * an exclusive bound, one past the largest port that binds. + * + * ⛔ Never hand-write either number anywhere else. The refusal in + * {@link formatInvalidPortNotice} reads both from here — the rule + * {@link PORT_SEARCH_SPAN} already carries in this file (#12620), for the same + * reason: a range a diagnostic STATES has to be the range the code ENFORCES, + * or the diagnostic becomes the next defect. + */ +const MIN_PORT = 0; +const MAX_PORT = 65535; + +/** + * Which input actually supplied the port text. + * + * ⭐ This type exists because of what the defect WAS. An operator who typed + * `--port abc` got back `ERR_SOCKET_BAD_PORT … options.port …` — an error + * naming an internal option, thrown from a code path with no connection to the + * thing they typed. A refusal that said only "invalid port" would commit the + * same defect one level up, so the refusal names the source, and this is the + * vocabulary it names it from. + */ +export type PortInputSource = '--port' | 'OS_PORT' | 'PORT' | 'the built-in default'; + +/** + * Name the input that supplied `flags.port`. + * + * `setFromDefault` is oclif's own parse metadata: `false` when the value came + * from argv, `true` when the flag's `default` supplied it. It is the ONLY + * signal that separates `--port` from the environment here, because + * `PORT`/`OS_PORT` never reach flag parsing at all — they are read by the + * `default` expression on the flag. MEASURED against this checkout's + * `@oclif/core` (4.13.3), both in `lib/parser/parse.js` and at runtime: the + * default branch's value function is `async () => flag.default`, and unlike + * the argv and `flag.env` branches it never calls `parseFlagOrThrowError`. A + * flag's own `parse` therefore cannot see a default, which is exactly why the + * validation this function feeds lives at the consumer instead of on the flag. + * + * ⚠️ The env half MIRRORS `readEnvWithDeprecation('OS_PORT', 'PORT')`'s + * precedence, and a mirror can drift from what it mirrors. It is pinned rather + * than trusted: `serve-port-validation.test.ts` asserts the two agree for every + * combination of the two variables — including `OS_PORT=''`, which is DEFINED + * and therefore wins. That case is why the test below is `!== undefined` and + * not a truthiness check: an `||` slip here would name `PORT` for a value that + * came from `OS_PORT`. + */ +export function describePortSource( + setFromDefault: boolean, + env: { OS_PORT?: string; PORT?: string } = process.env, +): PortInputSource { + if (!setFromDefault) return '--port'; + if (env.OS_PORT !== undefined) return 'OS_PORT'; + if (env.PORT !== undefined) return 'PORT'; + return 'the built-in default'; +} + +/** + * The port `flags.port` names, or `null` when that text cannot be a port. + * + * ## What this refuses, and why it is exactly that set + * + * `null` for precisely the values a real `listen()` refuses: `NaN`, anything + * below {@link MIN_PORT}, anything above {@link MAX_PORT}. Those are the + * inputs that used to travel all the way to the socket layer and die there on + * `ERR_SOCKET_BAD_PORT`, naming `options.port` instead of the flag or the + * environment variable the operator actually set. + * + * ## ⛔ `parseInt`'s tolerance is PRESERVED, and that is deliberate + * + * The obvious repair is a validating `Flags.integer({ min, max })`, whose + * parser is `/^-?\d+$/`. It was measured and NOT taken, for two reasons: + * + * 1. It cannot see the environment. `PORT`/`OS_PORT` arrive through the + * flag's `default`, and oclif never runs a flag's `parse` on a default + * (measured above, in {@link describePortSource}) — so an integer flag + * fixes `--port abc` and leaves `PORT=abc` and `OS_PORT=abc`, two of the + * three reported paths, dying exactly as before. + * 2. It would NARROW what boots. `/^-?\d+$/` refuses `" 3000"` (production + * env vars carry whitespace), `"3000.0"`, `"0x0BB8"`, `"+3000"` and + * `"3e3"` — every one of which `parseInt` accepts and every one of which + * boots a server today. + * + * So this function keeps `parseInt` as the reader and adds only the refusal. + * The accept set is therefore UNCHANGED: every value that boots today still + * boots, byte for byte, on the same port. What changes is only that the values + * which used to reach `listen()` and die raw are now refused here, in the + * operator's own vocabulary, before any socket exists. + * + * ⚠️ `parseInt`'s tolerance also means `--port 3e3` binds port **3**, not + * 3000, and this function preserves that too — a silent coercion, and a + * separate defect from the one this card repairs. It is filed rather than + * fixed here: tightening the accepted spelling would narrow the accept set, + * which is a contract question and not this card's to answer. + */ +export function parseRequestedPort(raw: string): number | null { + const parsed = parseInt(raw); + // `parseInt` yields an integer or `NaN`; `Number.isInteger` refuses the + // second. This is the `--port abc` / `PORT=abc` / `OS_PORT=abc` path, and + // also `PORT=''` — an env var that is DEFINED but empty, which + // `readEnvWithDeprecation` returns as `''` rather than falling back to 3000. + if (!Number.isInteger(parsed)) return null; + // And the numerically-fine-but-unbindable path: `--port 99999`, `--port -1`. + if (parsed < MIN_PORT || parsed > MAX_PORT) return null; + return parsed; +} + +/** + * The refusal for a port value that cannot be one (#12662). + * + * ⭐ Held to the standard the card is about. Two things it must do that the + * error it replaces did not: + * + * - **Name the source the operator actually used.** `--port`, `PORT` or + * `OS_PORT` — decided by {@link describePortSource}, not guessed here. + * - **State the range, read from {@link MIN_PORT}/{@link MAX_PORT}.** ⛔ Never + * a second, hand-written copy of those numbers: this sentence exists to be + * accurate about the bounds the code enforces, so it interpolates them. + * + * ⚠️ The raw text is rendered with `JSON.stringify`, which is not decoration. + * It makes `" 3000"` distinguishable from `"3000"` on the screen — the + * whitespace case is the most likely thing an operator is staring at without + * seeing — and it escapes control bytes rather than writing them to a terminal. + * + * ⚠️ KNOWN LIMIT, stated rather than papered over: `os dev` forwards its own + * `--port` (and `$PORT`, promoted to a flag) to the `serve` child on argv, + * and `os start` forwards its `--port` as `PORT` in the child's environment. + * On those spawns this names the channel the value arrived on, which is not + * always the one the operator typed. Both parent commands own their own flag + * validation; this is `serve` naming what `serve` can see. + * + * CHANNEL — the same `printDiagnostic` (stderr) as its two siblings, for the + * reason #7915 measured: `stdout` carries JSON-RPC frames whenever the stdio + * MCP transport is mounted, where one non-frame line reaches a conforming + * client as a transport error. + */ +export function formatInvalidPortNotice(raw: string, source: PortInputSource): string { + const shown = JSON.stringify(raw); + const spelled = source === '--port' + ? `--port ${shown}` + : source === 'the built-in default' + ? `the built-in default (${shown})` + : `${source}=${shown}`; + const fix = source === '--port' || source === 'the built-in default' + ? ' Pass a whole number instead, for example --port 3000.' + : ` Correct ${source} in this process's environment (for example ${source}=3000),\n` + + ' or override it with --port 3000.'; + + return ( + '\n' + + chalk.red(` ✗ Invalid port: ${spelled}\n`) + + chalk.dim(` A port must be a whole number from ${MIN_PORT} to ${MAX_PORT} — ${MIN_PORT} is legal, and\n`) + + chalk.dim(' asks the kernel for any free port. Nothing was started, and no socket\n') + + chalk.dim(' was opened.\n') + + chalk.dim(fix) + ); +} + /** * The IDENTITIES a capability provider registers under: full `plugin.name` ids * (`com.objectstack.mcp`) and/or exported class names (`MCPServerPlugin`). @@ -1390,7 +1568,11 @@ export default class Serve extends Command { }; async run(): Promise { - const { args, flags } = await this.parse(Serve); + // `metadata` is oclif's record of WHERE each flag's value came from. + // `metadata.flags.port.setFromDefault` is the only signal that separates a + // `--port` the operator typed from a value the flag's `default` read out of + // `OS_PORT`/`PORT`, and the port refusal below has to name which (#12662). + const { args, flags, metadata } = await this.parse(Serve); // ── stdout belongs to the protocol, never to diagnostics (#7915) ── // Everything `serve` and the kernel it boots would write to stdout is @@ -1490,7 +1672,36 @@ export default class Serve extends Command { process.env.NODE_ENV = 'production'; } - const requestedPort = parseInt(flags.port); + // ── The port has to BE a port before anything is done with it (#12662) ── + // `--port abc` used to leave this line as `NaN`, travel through the whole + // port policy below, and reach the real `listen()`, which refused it with + // `ERR_SOCKET_BAD_PORT: options.port should be >= 0 and < 65536` — an error + // naming an internal option, raised from a code path with no connection to + // the flag the operator typed. `--port 99999` parses fine and died the same + // way at the same place. + // + // ⭐ This sits BEFORE `portAutoShiftAllowed`, and that placement IS the + // coverage argument: the dev auto-shift branch, the production refusal in + // the `else if`, and a boot that enters neither are all downstream of this + // line, so one check covers all three. It is also ahead of every socket — + // nothing above probes, binds, or resolves anything. + // + // The three reported inputs converge here too, which is the other half of + // the argument. `PORT` and `OS_PORT` never reach flag parsing: they are read + // by the flag's `default`, and oclif runs no flag `parse` on a default + // (measured — see {@link describePortSource}). A validating + // `Flags.integer({ min, max })` would therefore have guarded `--port` alone + // and left the other two dying exactly as before. + const portSource = describePortSource(metadata.flags.port?.setFromDefault === true); + const parsedPort = parseRequestedPort(flags.port); + if (parsedPort === null) { + // One write, then exit — for the reason spelled out at the production + // refusal below: `this.exit(1)` reaches `process.exit` without draining a + // piped stdout, so a multi-call diagnostic loses its tail. + printDiagnostic(formatInvalidPortNotice(flags.port, portSource)); + this.exit(1); + } + const requestedPort = parsedPort; let port = requestedPort; // Port-conflict policy differs by mode: //