From 96fc0f6a177d9da2e40f7af19f41dfcd73dd9f22 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 28 Aug 2026 06:44:17 +0000 Subject: [PATCH] =?UTF-8?q?feat(cli):=20one=20port=20contract,=20three=20d?= =?UTF-8?q?oors=20=E2=80=94=20dev/start/serve=20refuse=20in=20the=20operat?= =?UTF-8?q?or's=20own=20spelling?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extract the port validation that #12662 landed in `serve.ts` — the range constants, the `parseInt`-based reader and the refusal prose — into `packages/cli/src/utils/port-contract.ts`, and have `dev`, `start` and `serve` each call it at their own door, before spawning anything. Before this, only `serve` validated. A value typed at `dev` or `start` reached the spawned child on a channel that renamed it, so the refusal named a spelling the operator had not used: `PORT=abc os dev` was refused as `--port "abc"`, and `os start --port 99999` as `PORT="99999"`. The range is declared in exactly one place in the repository, which is what the ruling protects: #12620 and #12662 both declined to copy it to a second entry point, and a shared single source is the front of that judgement. `start` deliberately does NOT gain `Flags.integer({ min, max })` — measured against @oclif/core 4.13.3, neither a flag `parse` nor an integer `min`/`max` runs over a value supplied by a flag's `default`, which is how $PORT and $OS_PORT reach the CLI, so a flag-layer bound would be inert on two of the three channels and would be a second copy of the range besides. The end-to-end accept set is unchanged, measured rather than argued: 18 port texts driven through all three real commands on all three channels, before and after, verdict-identical row for row. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01UjujZN219uFzBhSYfMykCd --- .changeset/single-source-port-contract.md | 45 ++ packages/cli/src/commands/dev.ts | 50 +++ .../serve-port-text-read-notice.test.ts | 6 +- .../commands/serve-port-validation.test.ts | 6 +- packages/cli/src/commands/serve.ts | 339 +-------------- packages/cli/src/commands/start.ts | 48 ++- .../utils/port-contract-single-source.test.ts | 278 ++++++++++++ packages/cli/src/utils/port-contract.ts | 408 ++++++++++++++++++ 8 files changed, 849 insertions(+), 331 deletions(-) create mode 100644 .changeset/single-source-port-contract.md create mode 100644 packages/cli/src/utils/port-contract-single-source.test.ts create mode 100644 packages/cli/src/utils/port-contract.ts diff --git a/.changeset/single-source-port-contract.md b/.changeset/single-source-port-contract.md new file mode 100644 index 0000000000..18bced665f --- /dev/null +++ b/.changeset/single-source-port-contract.md @@ -0,0 +1,45 @@ +--- +'@objectstack/cli': patch +--- + +`os dev` / `os start` / `os serve` refuse an impossible port at their own door, from one shared contract + +The port range, the reader that turns operator text into a port, and the refusal +prose now live in a single module (`packages/cli/src/utils/port-contract.ts`) +that all three commands import. Before this, only `serve` validated: a value +typed at `dev` or `start` travelled to the spawned `serve` child and was refused +one process later, under the name of the CHANNEL it arrived on rather than the +spelling the operator had used. + +**FROM → TO — what changes, stated precisely.** + +- **The end-to-end accept set does not change.** Since #12662 every value listed + below already ended in a refusal; what moves is WHERE the refusal happens and + WHAT it names. Measured before and after by driving a table of 18 port texts + through all three real commands on all three channels (`--port`, `$PORT`, + `$OS_PORT`) — 162 runs per side, every row identical in verdict. Values that + boot today still boot, on the same port: ` 3000`, `3000 `, `+3000`, `08080`, + `3e3`, `0x0BB8`, `3000.0`, `3000abc` and `0b111` are all accepted, exactly as + before, and `parseInt`'s tolerance is deliberately preserved — a strict-decimal + reader would refuse six values that start a server today. +- **The refusal moves earlier: from the `serve` child to the parent's own door, + before anything is spawned and before any socket exists.** +- **The refusal names the operator's spelling.** + - `PORT=abc os dev` — FROM `✗ Invalid port: --port "abc"` TO + `✗ Invalid port: PORT="abc"`. + - `OS_PORT=abc os dev` — FROM `✗ Invalid port: --port "abc"` TO + `✗ Invalid port: OS_PORT="abc"`. + - `os start --port 99999` — FROM `✗ Invalid port: PORT="99999"` TO + `✗ Invalid port: --port "99999"`. + - `os serve` is unchanged in every respect; it already named what it could see. +- **`os dev --port ""` is unchanged: still dropped, not refused.** An empty + string is falsy, so `dev` forwards nothing and the child resolves its own + default — measured, and preserved deliberately, because refusing it would + narrow a published command's accept set. + +No new flag, no new environment variable, no new configuration key. The range is +declared in exactly one place in the repository; `os start`'s `--port` did NOT +gain `Flags.integer({ min, max })`, because that bound would be a second copy of +the range and, measured against `@oclif/core` 4.13.3, neither a flag's `parse` +nor an integer `min`/`max` runs over a value supplied by a flag's `default` — +which is how `$PORT` and `$OS_PORT` reach the CLI. diff --git a/packages/cli/src/commands/dev.ts b/packages/cli/src/commands/dev.ts index 835041eb53..fb77bfa5e9 100644 --- a/packages/cli/src/commands/dev.ts +++ b/packages/cli/src/commands/dev.ts @@ -18,6 +18,11 @@ import { } from '../utils/dev-restart.js'; import { childEnvWithResolvedArtifact } from '../utils/internal-artifact-channel.js'; import { readEnvWithDeprecation, isMcpServerEnabled } from '@objectstack/types'; +// The ONE port contract, shared with `start` and with the `serve` child this +// command spawns (#12673). ⛔ Nothing about ports is declared in this file — +// no range, no reader, no wording; a second copy of the bound is exactly what +// #12620 and #12662 protected against. +import { describePortSource, parseRequestedPort, formatInvalidPortNotice } from '../utils/port-contract.js'; import type { ResolvedProjectDatabaseUrl } from '@objectstack/runtime'; /** @@ -385,6 +390,51 @@ export default class Dev extends Command { printKV('Database', redactConnectionUrl(effectiveDb), '🗄️'); const port = flags.port ?? readEnvWithDeprecation('OS_PORT', 'PORT', { silent: true }); + + // ── dev's own door on the ONE port contract (#12673) ────────────── + // Everything about the port — the range, the reader, the refusal prose — + // is imported from `utils/port-contract.ts`; this command declares none + // of it. That is the point of the card: `os dev` had no port validation + // at all, so an impossible value travelled to the `serve` child, which + // refused it under the name of the CHANNEL it arrived on. Measured on + // `origin/main` before this door existed: `PORT=abc os dev` and + // `OS_PORT=abc os dev` were both refused as `--port "abc"` — the one + // spelling the operator had not used, because the forwarding below + // renames every source to `--port`. + // + // ⭐ Placement is adjacent to `port` ON PURPOSE, not merely convenient. + // The refusal has to cover exactly the text this command FORWARDS, and + // the `port ? …` guard in the spawn argv below is what decides that. A + // door hoisted to the top of `run()` would be a second copy of that + // decision, free to drift from it; four lines apart, the two read one + // variable. It is still ahead of every spawn, every socket and every + // child process, which is all "before spawning" has ever meant here. + // + // ⛔ The `if (port)` is load-bearing, and MEASURED rather than assumed: + // `os dev --port ""` boots today. An empty string is falsy, so the guard + // below drops it and the child resolves its own default — so a door that + // refused every non-parsing text would refuse a value that starts a + // server, narrowing a published command's accept set, which this card is + // forbidden to do. (`PORT=""` reaches the child by inheritance instead, + // and `serve` refuses it there naming `PORT` — correctly, since that IS + // the spelling the operator set.) + if (port) { + // `flags.port === undefined` answers the same question oclif's + // `setFromDefault` answers for `serve`: did this come from argv? This + // flag carries no `default`, so there is no oclif metadata to read — + // and none to trust either, since oclif runs neither a flag `parse` + // nor an integer `min`/`max` over a default (measured; see the + // `describePortSource` docblock). + const portSource = describePortSource(flags.port === undefined); + if (parseRequestedPort(port) === null) { + // stderr, like `serve`'s refusal and for the same #7915 reason: this + // command's stdout is the fd the child's stdio MCP transport writes + // JSON-RPC frames to. + process.stderr.write(`${formatInvalidPortNotice(port, portSource)}\n`); + process.exit(1); + } + } + const binPath = process.argv[1]; const requestedPort = port ?? '3000'; diff --git a/packages/cli/src/commands/serve-port-text-read-notice.test.ts b/packages/cli/src/commands/serve-port-text-read-notice.test.ts index cc02fdeb10..827a8298db 100644 --- a/packages/cli/src/commands/serve-port-text-read-notice.test.ts +++ b/packages/cli/src/commands/serve-port-text-read-notice.test.ts @@ -60,7 +60,11 @@ import { portTextReadNotice, formatInvalidPortNotice, type PortInputSource, -} from './serve.js'; +// Moved out of `serve.ts` by #12673: the port contract is now ONE module that +// `dev`, `start` and `serve` all import, so this suite reads it from its home +// rather than through the command that used to declare it. The assertions are +// unchanged — only the path is. +} from '../utils/port-contract.js'; /** Seeded from `import.meta.url`, the spelling `check:cross-package-test-inputs` recognises. */ const HERE = resolve(fileURLToPath(import.meta.url), '..'); diff --git a/packages/cli/src/commands/serve-port-validation.test.ts b/packages/cli/src/commands/serve-port-validation.test.ts index 78c89140e7..67c4b90288 100644 --- a/packages/cli/src/commands/serve-port-validation.test.ts +++ b/packages/cli/src/commands/serve-port-validation.test.ts @@ -63,7 +63,11 @@ import { describePortSource, formatInvalidPortNotice, type PortInputSource, -} from './serve.js'; +// Moved out of `serve.ts` by #12673: the port contract is now ONE module that +// `dev`, `start` and `serve` all import, so this suite reads it from its home +// rather than through the command that used to declare it. The assertions are +// unchanged — only the path is. +} from '../utils/port-contract.js'; /** Seeded from `import.meta.url`, the spelling `check:cross-package-test-inputs` recognises. */ const HERE = resolve(fileURLToPath(import.meta.url), '..'); diff --git a/packages/cli/src/commands/serve.ts b/packages/cli/src/commands/serve.ts index 911b9a77f4..b4077e2f0b 100644 --- a/packages/cli/src/commands/serve.ts +++ b/packages/cli/src/commands/serve.ts @@ -49,6 +49,17 @@ import { isEmailTransportProvider, emailProviderRequiresApiKey, unsupportedProvi import { isSmsTransportProvider, SMS_TRANSPORT_PROVIDERS } from '@objectstack/service-sms'; import { resolveObjectStackHome } from '@objectstack/runtime'; import { LOG_LEVELS, resolveLogLevel, readLogLevelEnv } from '../utils/log-level.js'; +// The ONE port contract — range, reader and refusal prose — shared with the two +// commands that spawn this one (#12673). `serve`'s own door below calls it +// exactly as `dev` and `start` call it, and it is the only place the range is +// declared; see the module docblock for why it is not an oclif flag `parse`. +import { + describePortSource, + parseRequestedPort, + formatInvalidPortNotice, + portTextReadNotice, + type PortInputSource, +} from '../utils/port-contract.js'; import { BootLogCapture, isVerboseBootLevel } from '../utils/boot-log-capture.js'; import { graftAuthoredRuntimeMembers, isAppPluginLike } from '../utils/graft-runtime-hooks.js'; import { redactConnectionUrl, describeDriverConnection } from '../utils/connection-display.js'; @@ -358,334 +369,6 @@ 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; -} - -/** - * Spell a port input the way the operator set it: `--port "3e3"`, `PORT="3e3"`, - * `OS_PORT="3e3"`, or `the built-in default ("3000")`. - * - * ⭐ ONE spelling of one fact. Both notices that name the input read it from - * here — {@link formatInvalidPortNotice} (#12662) and - * {@link portTextReadNotice} (#12674) — which is the rule - * {@link PORT_SEARCH_SPAN}'s docblock established for numbers, applied to - * prose. Two hand-written copies of "how this input is written back" are two - * things free to drift, and an operator who cannot recognise what they typed is - * the defect both notices exist to fix. - * - * ⚠️ `JSON.stringify` is not decoration. It makes `" 3000"` distinguishable - * from `"3000"` on the screen — whitespace is the likeliest thing an operator - * is staring at without seeing — and it escapes control bytes instead of - * writing them to a terminal. - */ -function spellPortInput(raw: string, source: PortInputSource): string { - const shown = JSON.stringify(raw); - return source === '--port' - ? `--port ${shown}` - : source === 'the built-in default' - ? `the built-in default (${shown})` - : `${source}=${shown}`; -} - -/** - * 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 source spelling is {@link spellPortInput}'s, shared with #12674's - * read notice so that one input is named one way — `JSON.stringify` rendering - * included, which is not decoration: it makes `" 3000"` distinguishable from - * `"3000"` on the screen and 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 spelled = spellPortInput(raw, source); - 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 port the TEXT says, on a strict reading — or `null` when the text does - * not say a port at all. - * - * ## This boundary IS the card (#12674) - * - * {@link parseRequestedPort} keeps `parseInt`, deliberately: #12662's ruling is - * that no value which boots today may be refused, and a tightening would narrow - * a published CLI's accepted input. But `parseInt`'s tolerance changes the - * ANSWER, not merely the spelling — `--port 3e3` binds port **3**, `--port - * 0x0BB8` binds 3000, `--port 3000abc` binds 3000. The server comes up on a - * port the operator never named and nothing says so. ⭐ That silence, not the - * tolerance, is what is repaired here. - * - * So the notice fires on a DIFFERENCE, which makes the definition of "the same" - * the whole precision of this card. MEASURED on this checkout (Node v22.22.2): - * - * ``` - * " 3000" → 3000 "+3000" → 3000 "08080" → 8080 ← says what it selected - * "3000 " → 3000 "3000" → 3000 - * "3e3" → 3 "0x0BB8" → 3000 "3000.0" → 3000 ← does NOT - * "1e10" → 1 "0b111" → 0 "3000abc" → 3000 - * ``` - * - * - **Whitespace is not a difference.** `" 3000"` reads as 3000 to a human and - * to `parseInt` alike, and production `PORT` values carry whitespace. A - * boundary that counted it would drone a notice on every boot of the most - * ordinary deployment there is — noise on the one input shape that is both - * common and harmless. ⭐ This half of the line is why it is drawn on the - * TRIMMED text. - * - **A leading `+` is not a difference**, and **leading zeros are not**: - * `"+3000"` says 3000, `"08080"` says 8080 (`parseInt` has read no leading - * zero as octal since ES5 — measured above, not assumed). - * - **Everything else IS**, because it means the port was not read off the - * digits: an exponent, a radix prefix, a fraction, a separator, or trailing - * text. - * - * ⛔ Nothing here refuses anything. The accept set stays EXACTLY - * {@link parseRequestedPort}'s. Whether the CLI should take only strict decimal - * text is a contract question, left open on purpose (#12674), and #12673 is - * blocked on the same one. - * - * ⛔ Not `Number()`, which is the near-miss worth naming: it AGREES with - * `parseInt` on `"0x0BB8"` (both 3000, measured), so a boundary built on it - * would be blind to a hex literal — one of the two coercions this exists to - * see. It disagrees on `"3e3"` (3000 vs 3), which is the other one. - */ -export function strictPortReading(raw: string): number | null { - const trimmed = raw.trim(); - if (!/^[+-]?\d+$/.test(trimmed)) return null; - const value = Number(trimmed); - return Number.isInteger(value) ? value : null; -} - -/** - * The notice for a port that was read as something other than what it says - * (#12674) — or `null` when the text and the port agree. - * - * ## ⭐ The DECISION lives in here, not at the call site - * - * A notice printed unconditionally satisfies every assertion that only checks - * what a mismatch prints. Returning `null` for the agreeing case puts that arm - * where a test can drive it directly, at the same seam, with no source at all: - * `portTextReadNotice(' 3000', 'PORT', 3000)` is `null` or this is broken. The - * call site is then one `if`, and the three-way split it completes is: - * mismatch → this notice; agreement → nothing; a value that cannot be a port - * at all → {@link formatInvalidPortNotice}'s refusal, which exits before this - * function is ever reached. - * - * ## What it states — and the second reading it must NOT invent - * - * Two facts, both of which the operator lacks: the text they set, and the port - * it selected. ⛔ Never a third — the number they MEANT. `3e3` looks like 3000 - * to a reader, but `3000abc` and `0b111` have no such second reading, and a - * diagnostic that guessed would be wrong the first time it met one. It reports - * that the text does not say the selected port, and stops there. - * - * ⚠️ It says "asked for", not "bound": in development the auto-shift below - * may still move off this port, and #12543's drift notice — which prints just - * after this one — owns that fact. Two notices, two facts, neither restating - * the other. - * - * ## ⚠️ KNOWN LIMIT — the source it names on a spawn (#12673) - * - * MEASURED, and stated rather than papered over. `os dev` reads - * `flags.port ?? readEnvWithDeprecation('OS_PORT', 'PORT')` and forwards the - * result to the `serve` child as `--port ` on argv (`commands/dev.ts`). - * So `PORT=3e3 os dev` reaches this function as `--port "3e3"`, and the notice - * names the channel the value ARRIVED on rather than the one the operator set. - * The text and the port — the two facts above — are still exactly right; only - * the source label is the child's rather than the parent's. ⛔ It is NOT fixed - * here: `os dev`'s own port handling is #12673, which is blocked on the same - * accept-set question this card was scoped away from. - * - * `os start` is measured too and does not have the defect on this path: its - * `--port` is a `Flags.integer` (parser `/^-?\d+$/` in `@oclif/core` 4.13.3), - * so a spelling this notice would fire on is refused by the parent before any - * spawn, and an env-supplied value reaches the child through the inherited - * environment under its own name. - * - * CHANNEL — the same `printDiagnostic` (stderr) as its three 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. `serve-stdio-stdout-purity.e2e.test.ts` pins it. - */ -export function portTextReadNotice( - raw: string, - source: PortInputSource, - port: number, -): string | null { - if (strictPortReading(raw) === port) return null; - - const spelled = spellPortInput(raw, source); - const fix = source === '--port' || source === 'the built-in default' - ? ' If that is not the port you meant, write it as a plain decimal number\n' - + ' (for example --port 3000).' - : ` If that is not the port you meant, correct ${source} in this process's\n` - + ` environment (for example ${source}=3000), or override it with --port 3000.`; - - return ( - '\n' - + chalk.yellow(` ⚠ ${spelled} was read as port ${port}.\n`) - + chalk.dim(' That text is not a plain decimal number, and the reader that accepts it\n') - + chalk.dim(' is tolerant: it honours a leading 0x as hexadecimal and discards\n') - + chalk.dim(' everything from the first character that cannot continue the number.\n') - + chalk.dim(` Nothing downstream reads it again — ${port} is the port this server asked\n`) - + chalk.dim(' for, whatever the text looks like.\n') - + chalk.dim(fix) - ); -} - /** * The IDENTITIES a capability provider registers under: full `plugin.name` ids * (`com.objectstack.mcp`) and/or exported class names (`MCPServerPlugin`). diff --git a/packages/cli/src/commands/start.ts b/packages/cli/src/commands/start.ts index ac91009016..3c967b1f14 100644 --- a/packages/cli/src/commands/start.ts +++ b/packages/cli/src/commands/start.ts @@ -14,6 +14,11 @@ import { redactConnectionUrl } from '../utils/connection-display.js'; import { databaseDriverFlag } from '../utils/database-driver-flag.js'; import { childEnvWithResolvedArtifact } from '../utils/internal-artifact-channel.js'; import { readEnvWithDeprecation } from '@objectstack/types'; +// The ONE port contract, shared with `dev` and with the `serve` child this +// command spawns (#12673). ⛔ Nothing about ports is declared in this file — +// no range, no reader, no wording; a second copy of the bound is exactly what +// #12620 and #12662 protected against. +import { describePortSource, parseRequestedPort, formatInvalidPortNotice } from '../utils/port-contract.js'; import type { ResolvedProjectDatabaseUrl } from '@objectstack/runtime'; /** @@ -304,7 +309,48 @@ export default class Start extends Command { // Resolve the port the child `serve` will actually bind, matching its // flag default (`--port` > $OS_PORT/$PORT > 3000). Using `flags.port` // alone printed the wrong URL whenever the port came from the env. - const bannerPort = flags.port ?? readEnvWithDeprecation('OS_PORT', 'PORT', { silent: true }) ?? 3000; + const envPort = readEnvWithDeprecation('OS_PORT', 'PORT', { silent: true }); + + // ── start's own door on the ONE port contract (#12673) ──────────────── + // The same module `dev` and the `serve` child call, so the range exists + // once in the repository and all three doors enforce one set. What this + // repairs, measured on `origin/main` before the door existed: + // `os start --port 99999` was refused a process later as `PORT="99999"`, + // because the spawn below hands `flags.port` to the child as `PORT` — the + // operator typed `--port` and was told about an environment variable they + // had never set. + // + // ⛔ NOT `Flags.integer({ min, max })` on the flag, though this is the one + // command where that would have looked sufficient. Two measured reasons. + // (1) It is inert on the environment: oclif runs neither a flag `parse` + // nor an integer `min`/`max` over a value supplied by a `default`, and + // `$PORT`/`$OS_PORT` reach the child through exactly such a default on + // `serve`'s flag — so a bound declared here would guard `--port` and leave + // both env spellings behaving as before. (2) The bound would be a SECOND + // copy of the range, which is what #12620 and #12662 both declined to + // create and what this card's ruling names as the thing to protect. + // + // ⚠️ The text validated is the text FORWARDED. `Flags.integer` has already + // normalised argv by this point (`--port 08080` parses to `8080`), and the + // child env below is written as `PORT: String(flags.port)` — so + // `String(flags.port)` is literally what the child will read, not a + // reconstruction of it. The env branch needs no such care: `start` does not + // rewrite `$PORT`/`$OS_PORT`, the child inherits them under their own + // names, and this door refuses them under those same names one process + // earlier. + const portText = flags.port !== undefined ? String(flags.port) : envPort; + if (portText !== undefined) { + const portSource = describePortSource(flags.port === undefined); + if (parseRequestedPort(portText) === null) { + // stderr — `redirectStdoutToStderr()` above already routes this + // command's stdout there; writing the refusal to stderr directly + // states the channel instead of depending on that redirection. + process.stderr.write(`${formatInvalidPortNotice(portText, portSource)}\n`); + process.exit(1); + } + } + + const bannerPort = flags.port ?? envPort ?? 3000; if (flags.ui) printKV('Console', `http://localhost:${bannerPort}/_console/`, '🖥️'); printStep('Starting server...'); diff --git a/packages/cli/src/utils/port-contract-single-source.test.ts b/packages/cli/src/utils/port-contract-single-source.test.ts new file mode 100644 index 0000000000..4f785f40ee --- /dev/null +++ b/packages/cli/src/utils/port-contract-single-source.test.ts @@ -0,0 +1,278 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #12673 — the port range is declared ONCE, and all three doors refuse from it. + * + * ## The criterion this file exists to falsify + * + * `os dev`, `os start` and `os serve` all take a port and the first two spawn + * the third. The maintainer's ruling (2026-08-28, option 甲) is that they share + * ONE contract — not that each grows its own bound. #12620 and #12662 had both + * declined to copy the range to a second entry point, and the ruling is the + * front of that judgement rather than its reverse: *"still exactly one copy of + * the range, which is what #12620/#12662 protected"*. So the criterion is a + * ZERO — no second declaration anywhere — and the risk with any zero is that it + * is produced by a scan which would find nothing whatever the source said. + * + * ⭐ Hence the POSITIVE CONTROL below. The same scan, over the same corpus, must + * find `PORT_SEARCH_SPAN` — a constant known to exist, in a file the port range + * used to share. A zero next to a hit is a measurement; a zero on its own is + * only a grep that ran. + * + * ⛔ And the control must not be a SUBSTRING of the term under test, which is a + * live hazard on this card rather than a general caution: it touches + * `PORT` and `OS_PORT`, and `OS_PORT="abc"` CONTAINS `PORT="abc"` — a + * containment check for the second is satisfied by the first, and that exact + * overlap has produced a false positive in this lane already. Both directions + * of non-containment are asserted mechanically below rather than eyeballed, and + * the spelling assertions read the named source by EXTRACTION, never by + * `includes`. + * + * ## What the other cases pin + * + * The single source is only half the ruling. The other half is that each door + * refuses **at its own door, in the operator's own spelling, before spawning** — + * so this file also pins that all three commands import the contract, that + * neither parent declares a bound of its own, that each parent's refusal sits + * ahead of its spawn, and that the accept set is exactly + * {@link parseRequestedPort}'s for the table measured end to end through the + * three real commands. + */ + +import { describe, it, expect } from 'vitest'; +import { readdirSync, readFileSync, statSync } from 'node:fs'; +import { join, resolve } from 'node:path'; +import { fileURLToPath } from 'node:url'; + +// The one code/prose separator (`scripts/js-comment-mask.mjs`), typed by the +// hand-written `.d.mts` beside it. A private `stripComments` here would be one +// of the two drifting families that module's header documents — and this file +// asks "is this a DECLARATION or a sentence about one", which is exactly the +// question it answers. +import { maskComments } from '../../../../scripts/js-comment-mask.mjs'; + +import { + MIN_PORT, + MAX_PORT, + parseRequestedPort, + describePortSource, + formatInvalidPortNotice, +} from './port-contract.js'; + +/** …/packages/cli/src/utils */ +const HERE = resolve(fileURLToPath(import.meta.url), '..'); +/** …/packages/cli/src — the whole CLI source tree, this package's own. */ +const SRC = resolve(HERE, '..'); + +const CONTRACT = 'utils/port-contract.ts'; +const DOORS = ['commands/serve.ts', 'commands/dev.ts', 'commands/start.ts'] as const; + +/** Every `.ts` file under `packages/cli/src`, as package-relative paths. */ +function everySourceFile(dir = SRC, prefix = ''): string[] { + const found: string[] = []; + for (const entry of readdirSync(dir)) { + const full = join(dir, entry); + if (statSync(full).isDirectory()) { + found.push(...everySourceFile(full, `${prefix}${entry}/`)); + } else if (entry.endsWith('.ts')) { + found.push(`${prefix}${entry}`); + } + } + return found; +} + +const FILES = everySourceFile(); +/** Path → source with COMMENT spans blanked, so prose cannot answer for code. */ +const CODE = new Map(FILES.map((p) => [p, maskComments(readFileSync(join(SRC, p), 'utf8'))])); + +const read = (p: string): string => { + const source = CODE.get(p); + if (source === undefined) throw new Error(`${p} is not in packages/cli/src — the corpus moved`); + return source; +}; + +/** + * Strip SGR escapes. `chalk` is inert under a non-TTY runner but not guaranteed + * to be, and every assertion here is about TEXT. + * + * ⛔ The ESC byte is built with `String.fromCharCode` rather than written — the + * spelling its neighbour `serve-port-validation.test.ts` already uses, for a + * reason this file re-measured the hard way: written as an escape inside a + * regex literal it is materialised into a REAL control byte by the editing + * tool, and `check:nul-bytes` catches it (it did, on the first draft of this + * file, at this exact line). + */ +const SGR = new RegExp(`${String.fromCharCode(27)}\\[[0-9;]*m`, 'g'); +const plain = (text: string): string => text.replace(SGR, ''); + +/** + * The name a refusal states, read by EXTRACTION. + * + * ⛔ Never `notice.includes('PORT=…')` — see this file's header: `OS_PORT="x"` + * contains `PORT="x"`, so containment cannot tell the two channels apart. This + * reads the token between the colon and its separator and returns it whole, so + * `OS_PORT` and `PORT` are different answers rather than overlapping ones. + */ +function nameStatedBy(notice: string): string { + const match = plain(notice).match(/✗ Invalid port: (--port|[A-Za-z_][A-Za-z0-9_]*)(?:[ =]|$)/m); + if (!match) throw new Error(`no source name in refusal: ${JSON.stringify(plain(notice).slice(0, 80))}`); + return match[1]; +} + +/** A declaration of `name`, in code — `export` prefix and all. */ +const declarationOf = (name: string): RegExp => + new RegExp(String.raw`(?:^|[^\w$])(?:const|let|var)\s+${name}\b`); + +describe('#12673 — one port range, three doors', () => { + it('declares the range in exactly one file, and the scan proves it can see a neighbour', () => { + const declares = (name: string): string[] => + FILES.filter((p) => declarationOf(name).test(read(p))); + + // ── The ZERO: no second declaration of either bound, anywhere in the CLI ── + expect(declares('MIN_PORT'), 'MIN_PORT is declared outside the contract module').toEqual([CONTRACT]); + expect(declares('MAX_PORT'), 'MAX_PORT is declared outside the contract module').toEqual([CONTRACT]); + + // ── The POSITIVE CONTROL: the same scan, same corpus, a constant that IS + // there. Without this, both assertions above pass on a scan that reads + // nothing — the corpus could be empty and the regex could be wrong. + const control = declares('PORT_SEARCH_SPAN'); + expect(control, 'the scan found nothing at all — the zeros above measure nothing') + .toEqual(['commands/serve.ts']); + + // …and the control is independent of the terms under test in BOTH + // directions. Asserted, not eyeballed: this card's own `PORT` / `OS_PORT` + // pair is a live example of a containment that reads as a match. + for (const term of ['MIN_PORT', 'MAX_PORT']) { + expect('PORT_SEARCH_SPAN'.includes(term), `the control contains ${term}`).toBe(false); + expect(term.includes('PORT_SEARCH_SPAN'), `${term} contains the control`).toBe(false); + } + + // The corpus itself has to be real, or `FILES.filter` filters nothing. + expect(FILES.length, 'no CLI sources were scanned').toBeGreaterThan(100); + expect(FILES, 'the contract module is not in the scanned corpus').toContain(CONTRACT); + }); + + it('writes the two numbers nowhere but the contract module', () => { + // Both bounds are MEASURED facts about `listen()` (see the module), and a + // second hand-written copy is the drift this card exists to prevent. Tests + // are excluded on purpose: an expectation that read the bound from the + // module would assert `x === x` and pin nothing, so `65535` appearing in a + // `.test.ts` is the point rather than a violation. + const numeric = /(? p !== CONTRACT && !p.endsWith('.test.ts')) + .filter((p) => numeric.test(read(p))); + expect(offenders, 'a port bound is written as a literal outside the contract module').toEqual([]); + + // Control for the line above — the same regex, over a string that has one. + expect(numeric.test('const x = 65535;'), 'the numeric scan is a dead regex').toBe(true); + // …and it is the CODE that is scanned: `serve.ts` still explains the range + // in prose, and that sentence must not be read as a declaration. + expect(readFileSync(join(SRC, 'commands/serve.ts'), 'utf8')).toContain('65535'); + }); + + it('gives every door the same import and no bound of its own', () => { + for (const door of DOORS) { + const source = read(door); + expect(source, `${door} does not import the port contract`) + .toContain("from '../utils/port-contract.js'"); + expect(source, `${door} does not call the shared reader`).toContain('parseRequestedPort('); + expect(source, `${door} does not call the shared refusal`).toContain('formatInvalidPortNotice('); + expect(source, `${door} does not name the source it refuses in`).toContain('describePortSource('); + expect(declarationOf('MIN_PORT').test(source), `${door} declares its own floor`).toBe(false); + expect(declarationOf('MAX_PORT').test(source), `${door} declares its own ceiling`).toBe(false); + } + }); + + it('puts each parent’s refusal AHEAD of its spawn', () => { + // The ruling's words: each door validates "BEFORE spawning". A door that + // drifted below the spawn would still refuse — one process too late, which + // is the whole defect it replaces. + const dev = read('commands/dev.ts'); + const devDoor = dev.indexOf('if (parseRequestedPort(port) === null) {'); + const devSpawn = dev.indexOf('const spawnServeChild = '); + expect(devDoor, 'dev has no port door').toBeGreaterThan(-1); + expect(devSpawn, 'dev’s serve spawn is gone').toBeGreaterThan(-1); + expect(devDoor).toBeLessThan(devSpawn); + + const start = read('commands/start.ts'); + const startDoor = start.indexOf('if (parseRequestedPort(portText) === null) {'); + const startSpawn = start.indexOf('const child = spawn('); + expect(startDoor, 'start has no port door').toBeGreaterThan(-1); + expect(startSpawn, 'start’s serve spawn is gone').toBeGreaterThan(-1); + expect(startDoor).toBeLessThan(startSpawn); + }); + + it('keeps dev’s empty `--port` DROPPED rather than refused', () => { + // MEASURED on `origin/main` through the real command: `os dev --port ""` + // boots. The empty string is falsy, so the forwarding guard drops it and + // the child resolves its own default — a door that refused every text + // `parseRequestedPort` rejects would refuse a value that starts a server, + // narrowing a published command's accept set. The door therefore shares the + // forwarding guard rather than restating it, and both read one variable. + const dev = read('commands/dev.ts'); + expect(dev, 'dev’s door no longer shares the forwarding guard').toMatch( + /if \(port\) \{[\s\S]{0,600}?parseRequestedPort\(port\)/, + ); + expect(dev, 'dev’s forwarding guard changed shape').toContain("...(port ? ['--port', port] : [])"); + }); + + it('states the operator’s own spelling — read by extraction, never containment', () => { + const fromFlag = formatInvalidPortNotice('abc', describePortSource(false, {})); + const fromOsPort = formatInvalidPortNotice('abc', describePortSource(true, { OS_PORT: 'abc' })); + const fromPort = formatInvalidPortNotice('abc', describePortSource(true, { PORT: 'abc' })); + + expect(nameStatedBy(fromFlag)).toBe('--port'); + expect(nameStatedBy(fromOsPort)).toBe('OS_PORT'); + expect(nameStatedBy(fromPort)).toBe('PORT'); + + // ⭐ The trap, demonstrated rather than described. The naive containment + // check for the `PORT` channel is TRUE of the `OS_PORT` refusal, because + // one spelling contains the other; the extraction above separates them. + expect(plain(fromOsPort).includes('PORT="abc"'), 'the naive containment check').toBe(true); + expect(nameStatedBy(fromOsPort), 'the exact reading').not.toBe('PORT'); + + // Every refusal states the bounds it enforces, read from the module. + for (const notice of [fromFlag, fromOsPort, fromPort]) { + expect(plain(notice)).toContain(`from ${MIN_PORT} to ${MAX_PORT}`); + } + }); + + it('keeps the accept set exactly parseRequestedPort’s', () => { + // MEASURED end to end, before and after this change, by driving each value + // through the three REAL commands on all three channels (54 rows per + // command). `accepted` here is the end-to-end verdict those runs recorded; + // the door's own decision is `parseRequestedPort`, and the two agree row + // for row. ⭐ The coerced rows are the point: a strict-decimal reader would + // refuse six values that boot a server today. + const TABLE: Array<{ raw: string; accepted: boolean; note: string }> = [ + { raw: '3000', accepted: true, note: 'plain decimal' }, + { raw: '65535', accepted: true, note: 'the ceiling itself' }, + { raw: '0', accepted: true, note: '0 is a REQUEST for a kernel-assigned port' }, + { raw: ' 3000', accepted: true, note: 'leading space — production env vars carry it' }, + { raw: '3000 ', accepted: true, note: 'trailing space' }, + { raw: '+3000', accepted: true, note: 'explicit sign' }, + { raw: '08080', accepted: true, note: 'leading zero, decimal since ES5' }, + { raw: '3e3', accepted: true, note: 'coerced — selects port 3, not 3000' }, + { raw: '0x0BB8', accepted: true, note: 'coerced — hex, selects 3000' }, + { raw: '3000.0', accepted: true, note: 'coerced — selects 3000' }, + { raw: '3000abc', accepted: true, note: 'coerced — trailing text discarded' }, + { raw: '0b111', accepted: true, note: 'coerced — selects 0' }, + { raw: '65536', accepted: false, note: 'one past the ceiling' }, + { raw: '99999', accepted: false, note: 'the issue’s own example' }, + { raw: '-1', accepted: false, note: 'below the floor' }, + { raw: 'abc', accepted: false, note: 'not a number at all' }, + { raw: '', accepted: false, note: 'defined but empty — never falls back to 3000' }, + { raw: ' ', accepted: false, note: 'whitespace only' }, + ]; + + for (const { raw, accepted, note } of TABLE) { + expect(parseRequestedPort(raw) !== null, `${JSON.stringify(raw)} (${note})`).toBe(accepted); + } + + // Anti-vacuity: the table has to hold both verdicts, or the loop asserts + // one arm and reads as a pass over a reader that answers a constant. + expect(TABLE.some((r) => r.accepted)).toBe(true); + expect(TABLE.some((r) => !r.accepted)).toBe(true); + }); +}); diff --git a/packages/cli/src/utils/port-contract.ts b/packages/cli/src/utils/port-contract.ts new file mode 100644 index 0000000000..62a167b170 --- /dev/null +++ b/packages/cli/src/utils/port-contract.ts @@ -0,0 +1,408 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * The ONE port contract this CLI has: the range a real `listen()` accepts, the + * reader that turns operator text into a port, and the prose every door refuses + * in (#12673). + * + * ## Why this is a module and not three copies + * + * `os dev`, `os start` and `os serve` all take a port, and the first two spawn + * the third. Before this module only `serve` validated, so a value typed at + * `dev` or `start` was refused one process later, by the child, under the name + * of the CHANNEL it arrived on rather than the spelling the operator used: + * `PORT=abc os dev` was refused as `--port "abc"`, and `os start --port 99999` + * as `PORT="99999"`. Both are accurate about what the child could see and wrong + * about what the operator did. + * + * The repair the maintainer ruled for (2026-08-28, option 甲) is a single source + * all three doors import, NOT a second copy of the bound at each entry point — + * #12620 and #12662 both went out of their way to avoid that copy, and this + * module is the front of that judgement rather than its reverse. {@link MIN_PORT} + * and {@link MAX_PORT} are declared here and nowhere else in the repository; + * `port-contract-single-source.test.ts` fails if a second declaration appears. + * + * ## ⛔ What a door may NOT do: narrow the accept set + * + * Every function below keeps the reader it had in `serve` — `parseInt`, with all + * of its tolerance. Moving the refusal earlier must not change WHICH values + * boot, and the tolerance is wider than any intuition about ports: ` 3000`, + * `3000 `, `+3000`, `08080`, `3e3`, `0x0BB8`, `3000.0`, `3000abc` and `0b111` + * all boot a server today (measured through the three real commands, before and + * after this change). A door that reached for a strict-decimal regex would + * refuse six of them. + * + * ## ⚠️ Why the doors call this module EXPLICITLY, before spawning + * + * Not from an oclif flag `parse`, and not from `Flags.integer({ min, max })`. + * MEASURED against this checkout's `@oclif/core` 4.13.3 — see + * {@link describePortSource} for the runtime probe and its numbers — a flag's + * `parse` never runs over that flag's `default`, and `Flags.integer`'s + * `min`/`max` never runs over one either. `serve` reads `$OS_PORT`/`$PORT` + * through exactly such a `default`, so a validator hung on the flag layer is + * inert on two of the three channels this contract has to name. The ruling + * grants the fallback and this module is it: an explicit pre-spawn call. + */ +import chalk from 'chalk'; + +/** + * 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 + * `PORT_SEARCH_SPAN` already carries in `commands/serve.ts` (#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. + */ +export const MIN_PORT = 0; +export 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 the port text. + * + * `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. + * + * ⚠️ RE-MEASURED for #12673 against the same `@oclif/core` 4.13.3, by driving + * `Parser.parse` directly. Two results, and the second closes off the cheap + * repair the issue asked about: + * + * ``` + * string flag, value from argv → parse RAN + * string flag, value from `default` → parse did NOT run + * string flag, `default` is a FUNCTION + * reading process.env → parse did NOT run ← serve's shape + * string flag, value from the `env:` option → parse RAN + * integer flag {min:0,max:65535} argv 99999 → REFUSED + * integer flag {min:0,max:65535} default 99999 → ACCEPTED, value 99999 + * integer flag {min:0,max:65535} `env:` 99999 → REFUSED + * ``` + * + * So `Flags.integer({ min, max })` is inert over a `default` too, not merely + * `parse`: a bound declared on the flag would guard `--port` and leave both + * environment channels exactly as they were. And oclif's own `env:` OPTION is a + * DIFFERENT channel from a `default` that happens to read `process.env` — the + * first is validated, the second is not, and no command in this CLI uses the + * first. That asymmetry is why every door calls this module explicitly, before + * spawning, rather than hanging a validator on its flag. + * + * ⚠️ `dev`'s port flag carries no oclif `default` at all, so `dev` passes + * `flags.port === undefined` for `setFromDefault` — the same question ("did + * this value come from argv?") asked of a flag that answers it differently. + * + * ⚠️ 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; +} + +/** + * Spell a port input the way the operator set it: `--port "3e3"`, `PORT="3e3"`, + * `OS_PORT="3e3"`, or `the built-in default ("3000")`. + * + * ⭐ ONE spelling of one fact. Both notices that name the input read it from + * here — {@link formatInvalidPortNotice} (#12662) and + * {@link portTextReadNotice} (#12674) — which is the rule + * `PORT_SEARCH_SPAN`'s docblock in `commands/serve.ts` established for numbers, applied to + * prose. Two hand-written copies of "how this input is written back" are two + * things free to drift, and an operator who cannot recognise what they typed is + * the defect both notices exist to fix. + * + * ⚠️ `JSON.stringify` is not decoration. It makes `" 3000"` distinguishable + * from `"3000"` on the screen — whitespace is the likeliest thing an operator + * is staring at without seeing — and it escapes control bytes instead of + * writing them to a terminal. + */ +function spellPortInput(raw: string, source: PortInputSource): string { + const shown = JSON.stringify(raw); + return source === '--port' + ? `--port ${shown}` + : source === 'the built-in default' + ? `the built-in default (${shown})` + : `${source}=${shown}`; +} + +/** + * 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 source spelling is {@link spellPortInput}'s, shared with #12674's + * read notice so that one input is named one way — `JSON.stringify` rendering + * included, which is not decoration: it makes `" 3000"` distinguishable from + * `"3000"` on the screen and escapes control bytes rather than writing them to + * a terminal. + * + * ⭐ The limit this used to carry is CLOSED, and how it was closed is the whole + * of #12673. `os dev` forwards its port to the `serve` child as `--port ` + * on argv and `os start` forwards its own as `PORT` in the child's environment, + * so on a spawn the child can only ever name the CHANNEL — measured before the + * repair: `PORT=abc os dev` was refused as `--port "abc"`, `OS_PORT=abc os dev` + * as `--port "abc"`, and `os start --port 99999` as `PORT="99999"`. The repair + * is not to teach the child about its parents. Each parent now calls this same + * function at its own door, with its own {@link describePortSource} reading, + * BEFORE it spawns anything — so an input that would have been renamed on the + * way down is refused before the rename can happen, and this function keeps + * naming exactly what the process it runs in 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 spelled = spellPortInput(raw, source); + 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 port the TEXT says, on a strict reading — or `null` when the text does + * not say a port at all. + * + * ## This boundary IS the card (#12674) + * + * {@link parseRequestedPort} keeps `parseInt`, deliberately: #12662's ruling is + * that no value which boots today may be refused, and a tightening would narrow + * a published CLI's accepted input. But `parseInt`'s tolerance changes the + * ANSWER, not merely the spelling — `--port 3e3` binds port **3**, `--port + * 0x0BB8` binds 3000, `--port 3000abc` binds 3000. The server comes up on a + * port the operator never named and nothing says so. ⭐ That silence, not the + * tolerance, is what is repaired here. + * + * So the notice fires on a DIFFERENCE, which makes the definition of "the same" + * the whole precision of this card. MEASURED on this checkout (Node v22.22.2): + * + * ``` + * " 3000" → 3000 "+3000" → 3000 "08080" → 8080 ← says what it selected + * "3000 " → 3000 "3000" → 3000 + * "3e3" → 3 "0x0BB8" → 3000 "3000.0" → 3000 ← does NOT + * "1e10" → 1 "0b111" → 0 "3000abc" → 3000 + * ``` + * + * - **Whitespace is not a difference.** `" 3000"` reads as 3000 to a human and + * to `parseInt` alike, and production `PORT` values carry whitespace. A + * boundary that counted it would drone a notice on every boot of the most + * ordinary deployment there is — noise on the one input shape that is both + * common and harmless. ⭐ This half of the line is why it is drawn on the + * TRIMMED text. + * - **A leading `+` is not a difference**, and **leading zeros are not**: + * `"+3000"` says 3000, `"08080"` says 8080 (`parseInt` has read no leading + * zero as octal since ES5 — measured above, not assumed). + * - **Everything else IS**, because it means the port was not read off the + * digits: an exponent, a radix prefix, a fraction, a separator, or trailing + * text. + * + * ⛔ Nothing here refuses anything. The accept set stays EXACTLY + * {@link parseRequestedPort}'s. Whether the CLI should take only strict decimal + * text is a contract question, left open on purpose (#12674), and #12673 is + * blocked on the same one. + * + * ⛔ Not `Number()`, which is the near-miss worth naming: it AGREES with + * `parseInt` on `"0x0BB8"` (both 3000, measured), so a boundary built on it + * would be blind to a hex literal — one of the two coercions this exists to + * see. It disagrees on `"3e3"` (3000 vs 3), which is the other one. + */ +export function strictPortReading(raw: string): number | null { + const trimmed = raw.trim(); + if (!/^[+-]?\d+$/.test(trimmed)) return null; + const value = Number(trimmed); + return Number.isInteger(value) ? value : null; +} + +/** + * The notice for a port that was read as something other than what it says + * (#12674) — or `null` when the text and the port agree. + * + * ## ⭐ The DECISION lives in here, not at the call site + * + * A notice printed unconditionally satisfies every assertion that only checks + * what a mismatch prints. Returning `null` for the agreeing case puts that arm + * where a test can drive it directly, at the same seam, with no source at all: + * `portTextReadNotice(' 3000', 'PORT', 3000)` is `null` or this is broken. The + * call site is then one `if`, and the three-way split it completes is: + * mismatch → this notice; agreement → nothing; a value that cannot be a port + * at all → {@link formatInvalidPortNotice}'s refusal, which exits before this + * function is ever reached. + * + * ## What it states — and the second reading it must NOT invent + * + * Two facts, both of which the operator lacks: the text they set, and the port + * it selected. ⛔ Never a third — the number they MEANT. `3e3` looks like 3000 + * to a reader, but `3000abc` and `0b111` have no such second reading, and a + * diagnostic that guessed would be wrong the first time it met one. It reports + * that the text does not say the selected port, and stops there. + * + * ⚠️ It says "asked for", not "bound": in development the auto-shift below + * may still move off this port, and #12543's drift notice — which prints just + * after this one — owns that fact. Two notices, two facts, neither restating + * the other. + * + * ## ⚠️ The source it names on a spawn — still the CHILD's, deliberately + * + * `os dev` reads `flags.port ?? readEnvWithDeprecation('OS_PORT', 'PORT')` and + * forwards the result to the `serve` child as `--port ` on argv + * (`commands/dev.ts`), so `PORT=3e3 os dev` reaches this function as + * `--port "3e3"` and this notice names `--port`. That is not the defect #12673 + * repaired, and it is not repaired here either: the REFUSAL was moved to the + * parents (see {@link formatInvalidPortNotice}) because a refusal must name + * what the operator can act on, while this notice's two facts — the text and + * the port it selected — are true of the value in the child's own hands and + * remain right whichever spelling delivered it. ⛔ Do not "fix" the label by + * teaching the child about its parents: that is option 乙 from the #12673 + * design, and it was rejected for adding an invisible inter-process protocol + * that fails silently when a future author forgets to pass it. + * + * `os start` normalises before forwarding — its `--port` is a `Flags.integer` + * (parser `/^-?\d+$/` in `@oclif/core` 4.13.3), so the child receives + * `PORT=8080` for an `--port 08080`, and a spelling this notice fires on can + * only reach it through the environment, under its own name. + * + * CHANNEL — the same `printDiagnostic` (stderr) as its three 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. `serve-stdio-stdout-purity.e2e.test.ts` pins it. + */ +export function portTextReadNotice( + raw: string, + source: PortInputSource, + port: number, +): string | null { + if (strictPortReading(raw) === port) return null; + + const spelled = spellPortInput(raw, source); + const fix = source === '--port' || source === 'the built-in default' + ? ' If that is not the port you meant, write it as a plain decimal number\n' + + ' (for example --port 3000).' + : ` If that is not the port you meant, correct ${source} in this process's\n` + + ` environment (for example ${source}=3000), or override it with --port 3000.`; + + return ( + '\n' + + chalk.yellow(` ⚠ ${spelled} was read as port ${port}.\n`) + + chalk.dim(' That text is not a plain decimal number, and the reader that accepts it\n') + + chalk.dim(' is tolerant: it honours a leading 0x as hexadecimal and discards\n') + + chalk.dim(' everything from the first character that cannot continue the number.\n') + + chalk.dim(` Nothing downstream reads it again — ${port} is the port this server asked\n`) + + chalk.dim(' for, whatever the text looks like.\n') + + chalk.dim(fix) + ); +}