From 9bf969933c75bb732649b6d7dc1d5ff096191cc4 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 29 Aug 2026 01:01:11 +0000 Subject: [PATCH] fix(cli): forward `os start --port` on the channel its child reads first (#12992) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `os start --port N` printed N and bound something else whenever `$OS_PORT` was set. Two independent halves, both repaired. The channel: `start` wrote the flag as `PORT` and left the inherited `OS_PORT` beside it, but the `serve` child resolves `OS_PORT` FIRST — so an explicit flag travelled on the channel its own child ranks last. The parent now writes the canonical name with its alias, in agreement. Same edit forwards `--port 0`, which the falsy guard used to drop. The banner: `start`'s `Console:` row was a second resolution of the same question with the opposite precedence, and asserted a Console mount it could not know (measured 404 on the same boot). Both facts belong to the child, which states them after its listen(). The row is removed, not recomputed. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01UjujZN219uFzBhSYfMykCd --- .changeset/start-port-forwarding-channel.md | 49 +++ .../start-port-forwarding-channel.pin.test.ts | 209 +++++++++++++ packages/cli/src/commands/start.ts | 134 +++++++- .../start-port-banner-agreement.e2e.test.ts | 296 ++++++++++++++++++ 4 files changed, 676 insertions(+), 12 deletions(-) create mode 100644 .changeset/start-port-forwarding-channel.md create mode 100644 packages/cli/src/commands/start-port-forwarding-channel.pin.test.ts create mode 100644 packages/cli/test/start-port-banner-agreement.e2e.test.ts diff --git a/.changeset/start-port-forwarding-channel.md b/.changeset/start-port-forwarding-channel.md new file mode 100644 index 0000000000..91a1566018 --- /dev/null +++ b/.changeset/start-port-forwarding-channel.md @@ -0,0 +1,49 @@ +--- +"@objectstack/cli": patch +--- + +fix(cli): `os start --port` now wins over `$OS_PORT`, and `start` stops printing an address it is not serving (#12992) + +`os start --port N` printed `N` and then bound something else, whenever +`$OS_PORT` was set. Measured on a real boot before the repair: + +``` +OS_PORT=41077 os start --port 41078 + banner: Console: http://localhost:41078/_console/ + curl answers on: 41077 +``` + +Two independent halves, both repaired. + +**The forwarding channel.** `start` wrote the flag into the child's `PORT` and +never cleared the inherited `OS_PORT` beside it. The `serve` child resolves +`readEnvWithDeprecation('OS_PORT', 'PORT')` — `OS_PORT` first — so an explicit +`--port` travelled on the channel its own child ranks **last** and lost to an +environment variable the flag's help text says it overrides. The child's +precedence was correct and is unchanged; the parent now writes the canonical +`OS_PORT` together with its `PORT` alias, so the flag arrives first in the order +the child already reads and every other reader of the child's environment +(app code and libraries that read `process.env.PORT` directly) sees the same +port. No deprecation notice is reachable from either spelling: `OS_PORT` is the +*preferred* name of that pair, and every read site passes `{ silent: true }`. + +Same edit fixes `os start --port 0`, which a falsy guard used to drop entirely — +`0` is a legal port that asks the kernel for a free one (`MIN_PORT = 0`). +Measured before: `os start --port 0` printed `http://localhost:0/_console/` and +bound the inherited `41077`. + +**The lying banner.** `start`'s `Console:` row was a *second* resolution of a +question the child answers for itself, computed with the opposite precedence and +reconciled with nothing. It also asserted a mount it could not know: on the same +boot, `/_console/` answered **404**, because whether a Console is served depends +on the `ConsoleUI` plugin loading in the child. Both facts belong to `serve`, +which already states them together after its `listen()` — the `API:` row always, +the `Console:` row when the plugin actually loaded, both addressed through the +external-base resolver. So `start` no longer prints an address at all, and the +one address it used to print is gone rather than recomputed. + +**User-visible:** `os start` prints one fewer row before the server boots. The +Console URL now comes from the `serve` ready banner, after the bind, and appears +only when a Console is really mounted. Because it is derived from the actual +bind rather than predicted, it is also correct under causes this change does not +touch, including the development auto-shift off a busy port. diff --git a/packages/cli/src/commands/start-port-forwarding-channel.pin.test.ts b/packages/cli/src/commands/start-port-forwarding-channel.pin.test.ts new file mode 100644 index 0000000000..943d2d46cd --- /dev/null +++ b/packages/cli/src/commands/start-port-forwarding-channel.pin.test.ts @@ -0,0 +1,209 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * Pin: **`os start` hands its `--port` to the child on the channel the child + * reads FIRST, and states no address of its own** (#12992). + * + * ## The defect, measured on a real boot + * + * ``` + * OS_PORT=41077 os start --port 41078 + * banner: Console: http://localhost:41078/_console/ + * curl finds it on: 41077 + * ``` + * + * Two independent halves, and this file pins both: + * + * 1. **The channel.** `start` wrote the flag as `PORT` and never cleared the + * inherited `OS_PORT`. The child resolves + * `readEnvWithDeprecation('OS_PORT', 'PORT')` — `OS_PORT` first — so an + * explicit flag lost to an environment variable its own help text says it + * overrides. + * 2. **The address.** The banner was a SECOND resolution of the same question, + * computed in the parent with the opposite precedence. Nothing reconciled + * the two, so `start` printed a URL it was not serving. + * + * ## ⚠️ The instrument: `OS_PORT` CONTAINS `PORT` + * + * Every assertion below is over object KEYS and exact values, never substring + * containment of a rendered message — `serve-port-validation.test.ts` documents + * why a `toContain`/`not.toContain` pair on this pair of names reports the + * `OS_PORT` reading as also naming `PORT`. Where this file does assert that a + * value is ABSENT, the same probe is first shown finding it present (the + * positive control in `forwards nothing at all when no flag was given`), so an + * absence here is a measurement rather than a probe that never worked. + * + * ## Why the behavioural half reads through the CHILD's reader + * + * The point of the card is that the parent and the child answered one question + * two ways. A test that re-implemented the child's precedence would be a THIRD + * answer, free to agree with the parent while the real child disagreed. So the + * composed environment is handed to `readEnvWithDeprecation('OS_PORT', 'PORT', + * { silent: true })` — the exact expression `commands/serve.ts` uses for its + * port flag's default — and that reader's answer is what is asserted. + */ + +import { describe, it, expect, afterEach } from 'vitest'; +import { readFileSync } from 'node:fs'; +import ts from 'typescript'; +import { readEnvWithDeprecation } from '@objectstack/types'; +import { parseRequestedPort } from '../utils/port-contract.js'; +import { childPortEnv } from './start.js'; + +/** The operator's own value, inherited by `start` and passed down. */ +const OPERATOR_OS_PORT = '41077'; +/** What the operator typed at `--port`. It must win. */ +const FLAG_PORT = 41078; + +const ORIGINAL_ENV = { ...process.env }; +afterEach(() => { + for (const key of Object.keys(process.env)) { + if (!(key in ORIGINAL_ENV)) delete process.env[key]; + } + Object.assign(process.env, ORIGINAL_ENV); +}); + +/** + * The child environment `start` composes, reduced to the part this card is + * about: an inherited environment, plus whatever the port channel contributes. + */ +function composeChildEnv(flagPort: number | undefined, inherited: Record = {}) { + return { ...inherited, ...childPortEnv(flagPort) }; +} + +/** + * What the CHILD would resolve from that environment — through the child's own + * reader, not a re-derivation of its precedence. + */ +function portTheChildWouldRead(childEnv: Record): string | undefined { + for (const key of ['OS_PORT', 'PORT']) delete process.env[key]; + Object.assign(process.env, childEnv); + return readEnvWithDeprecation('OS_PORT', 'PORT', { silent: true }); +} + +describe('`os start` forwards --port on the channel its child reads first', () => { + it('an explicit --port beats an inherited $OS_PORT — the whole defect', () => { + const childEnv = composeChildEnv(FLAG_PORT, { OS_PORT: OPERATOR_OS_PORT }); + + // Before the repair this read back '41077': the flag travelled as `PORT`, + // which the child ranks LAST, and the inherited OS_PORT won. + expect(portTheChildWouldRead(childEnv)).toBe(String(FLAG_PORT)); + expect(parseRequestedPort(portTheChildWouldRead(childEnv)!)).toBe(FLAG_PORT); + }); + + it('states the one value on the canonical name AND its legacy alias', () => { + const env = childPortEnv(FLAG_PORT); + + // Exact values, never containment — see the instrument note in the header. + expect(env.OS_PORT).toBe(String(FLAG_PORT)); + expect(env.PORT).toBe(String(FLAG_PORT)); + // ⭐ The two must AGREE. A child whose environment named two different + // ports would be this card's defect moved one layer down, where app code + // reading `process.env.PORT` directly (see + // `examples/app-showcase/src/system/self-url.ts`) would compute an address + // for a port nothing is listening on. + expect(env.OS_PORT).toBe(env.PORT); + }); + + it('overwrites BOTH inherited spellings, leaving no stale port behind', () => { + const childEnv = composeChildEnv(FLAG_PORT, { OS_PORT: OPERATOR_OS_PORT, PORT: '39999' }); + + expect(childEnv.OS_PORT).toBe(String(FLAG_PORT)); + expect(childEnv.PORT).toBe(String(FLAG_PORT)); + expect(Object.values(childEnv)).not.toContain(OPERATOR_OS_PORT); + }); + + it('forwards --port 0, which the falsy guard used to drop', () => { + // `port-contract.ts` measures `listen(0) → OK` and declares MIN_PORT = 0: + // zero is a REQUEST for a kernel-assigned port, not an error. The old + // `flags.port ? …` guard was falsy for it, so `os start --port 0` forwarded + // nothing — measured on the unrepaired command, it printed + // `http://localhost:0/_console/` and bound the inherited 41077 instead. + const childEnv = composeChildEnv(0, { OS_PORT: OPERATOR_OS_PORT }); + + expect(childEnv.OS_PORT).toBe('0'); + expect(portTheChildWouldRead(childEnv)).toBe('0'); + expect(parseRequestedPort('0')).toBe(0); + }); + + it('forwards nothing at all when no flag was given — with its positive control', () => { + const hasPortKey = (env: Record) => + Object.prototype.hasOwnProperty.call(env, 'OS_PORT') + || Object.prototype.hasOwnProperty.call(env, 'PORT'); + + // ⭐ POSITIVE CONTROL first: the same probe, on the same helper, DOES see + // the keys when a flag is given. Without this line the assertion below + // would pass just as happily against a probe that can never see anything. + expect(hasPortKey(childPortEnv(FLAG_PORT))).toBe(true); + expect(hasPortKey(childPortEnv(undefined))).toBe(false); + + // …so an operator's own environment reaches the child untouched, under its + // own names, which is what `start`'s refusal door one process earlier + // depends on being true. + const childEnv = composeChildEnv(undefined, { OS_PORT: OPERATOR_OS_PORT, PORT: '39999' }); + expect(childEnv.OS_PORT).toBe(OPERATOR_OS_PORT); + expect(childEnv.PORT).toBe('39999'); + expect(portTheChildWouldRead(childEnv)).toBe(OPERATOR_OS_PORT); + }); +}); + +describe('structural: `start` states no address of its own', () => { + /** + * Find every `localhost:` address BUILT in a file, off the AST. + * + * Deliberately not a text scan, for the reason the sibling pin + * (`artifact-child-env.pin.test.ts`) records: a regex comment-stripper once + * reported `start.ts` clean while the file carried the very write under test, + * because a `/*` inside a flag description opened a phantom block comment. The + * parser decides what is code and what is prose. + * + * Template literals ONLY. A plain string cannot interpolate a port, and the + * prose in this file's own header talks about `http://localhost:41078/` — the + * detector must not be confused by either. + */ + const localhostAddressesBuilt = (file: string): string[] => { + const src = readFileSync(new URL(`./${file}`, import.meta.url), 'utf8'); + const sourceFile = ts.createSourceFile(file, src, ts.ScriptTarget.Latest, true); + const hits: string[] = []; + + const at = (node: ts.Node) => + `${file}:${sourceFile.getLineAndCharacterOfPosition(node.getStart(sourceFile)).line + 1}`; + + const visit = (node: ts.Node): void => { + if (ts.isTemplateExpression(node)) { + const text = node.head.text + node.templateSpans.map((s) => s.literal.text).join(''); + if (/localhost:/.test(node.head.text) || /localhost:?$/.test(node.head.text.trimEnd())) { + hits.push(`${at(node)} \`${text}\``); + } + } + ts.forEachChild(node, visit); + }; + + visit(sourceFile); + return hits; + }; + + it('start.ts interpolates no localhost address', () => { + expect( + localhostAddressesBuilt('start.ts'), + 'start.ts must not compose an address from a port it resolved itself. Both facts such a ' + + 'row asserts — the bound port and whether a Console is mounted — belong to the `serve` ' + + 'child, which states them together after its listen(). See the ⛔ block above ' + + "`printStep('Starting server...')`.", + ).toEqual([]); + }); + + it('the detector can see one — positive control', () => { + // The exact line this card deleted, fed to the same scanner. If this ever + // returns [], the assertion above is vacuous and proves nothing. + const specimen = "const p = 1; const s = `http://localhost:${p}/_console/`;"; + const sourceFile = ts.createSourceFile('specimen.ts', specimen, ts.ScriptTarget.Latest, true); + const hits: string[] = []; + const visit = (node: ts.Node): void => { + if (ts.isTemplateExpression(node) && /localhost:/.test(node.head.text)) hits.push('hit'); + ts.forEachChild(node, visit); + }; + visit(sourceFile); + expect(hits).toHaveLength(1); + }); +}); diff --git a/packages/cli/src/commands/start.ts b/packages/cli/src/commands/start.ts index 3c967b1f14..10224fa03b 100644 --- a/packages/cli/src/commands/start.ts +++ b/packages/cli/src/commands/start.ts @@ -306,9 +306,10 @@ export default class Start extends Command { } printKV('Database', redactConnectionUrl(databaseUrl), '🗄️'); printKV('Environment', environmentId, '🎯'); - // 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. + // The port TEXT this command was given, for the refusal door below — + // ⛔ never for a URL. Nothing in this parent may print a port: see + // {@link childPortEnv} for the channel, and the spawn below for why the + // address this command used to advertise is now the child's to state. const envPort = readEnvWithDeprecation('OS_PORT', 'PORT', { silent: true }); // ── start's own door on the ONE port contract (#12673) ──────────────── @@ -331,11 +332,11 @@ export default class Start extends Command { // 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 + // normalised argv by this point (`--port 08080` parses to `8080`), and + // {@link childPortEnv} writes `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: when no flag is given `start` writes neither + // port variable, the child inherits `$PORT`/`$OS_PORT` 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; @@ -350,9 +351,36 @@ export default class Start extends Command { } } - const bannerPort = flags.port ?? envPort ?? 3000; - if (flags.ui) printKV('Console', `http://localhost:${bannerPort}/_console/`, '🖥️'); - + // ── ⛔ NO `Console:` row here, and no other address either (#12992) ────── + // This command used to print `http://localhost:${flags.port ?? envPort ?? + // 3000}/_console/` at exactly this point, and the line was wrong in TWO + // independent ways at once — both measured on a real boot of + // `OS_PORT=41077 os start --port 41078`: + // + // 1. The PORT. It was a SECOND resolution of a question the child answers + // for itself, and the two answers disagreed: this one ranked the flag + // first, the child ranks `$OS_PORT` first, so the banner said 41078 + // while `curl` found the server on 41077. + // 2. The MOUNT. `/_console/` was advertised unconditionally under + // `flags.ui`, but whether a Console is actually served depends on the + // `ConsoleUI` plugin loading in the CHILD. On the same boot that path + // answered **404** — the row promised a page that was never mounted. + // + // ⭐ Both facts belong to the child and neither is knowable here. `serve` + // already states them together, AFTER its `listen()`, from the port it + // really bound and gated on the plugin really loading: `printServerReady` + // prints the `API:` row always and the `Console:` row when + // `loadedPlugins.includes('ConsoleUI')`, addressing both through the + // external-base resolver so they stay right behind a proxy too. + // + // ⛔ Do not reintroduce an address here fed from the child's `ipc` + // `objectstack:listening` message (the channel `dev` opens, and which + // `serve` publishes on unconditionally — so it IS available). It was + // measured and declined: the message carries `{ port, url }` and NOT the + // mount fact, so a row rebuilt from it would fix defect 1 and keep defect + // 2, and on a healthy boot it would restate — two lines later, in a second + // spelling — a row the child had already printed correctly. One process + // knows both facts; that process prints them. printStep('Starting server...'); // ── Child env ─────────────────────────────────────────────────── @@ -380,7 +408,7 @@ export default class Start extends Command { OS_HOME: homeDir, OS_ENVIRONMENT_ID: environmentId, OS_DATABASE_URL: databaseUrl, - ...(flags.port ? { PORT: String(flags.port) } : {}), + ...childPortEnv(flags.port), ...(flags['database-driver'] ? { OS_DATABASE_DRIVER: flags['database-driver'] } : {}), ...(flags['database-auth-token'] ? { OS_DATABASE_AUTH_TOKEN: flags['database-auth-token'] } : {}), AUTH_SECRET: authSecret, @@ -419,6 +447,88 @@ export default class Start extends Command { } } +/** + * The port `start` hands its `serve` child, as environment (#12992). + * + * ## ⭐ The channel is the defect, not the value + * + * `start` used to write the flag as `{ PORT: String(flags.port) }` and leave an + * inherited `$OS_PORT` in place beside it. The child resolves + * `readEnvWithDeprecation('OS_PORT', 'PORT')` — `OS_PORT` FIRST — so an explicit + * `--port` was handed down on the channel its own child ranks LAST and lost to + * an environment variable the flag's help text says it overrides. Measured on a + * real boot before this function existed: + * + * ``` + * OS_PORT=41077 os start --port 41078 → curl finds the server on 41077 + * ``` + * + * The child's precedence is CORRECT and is not what changed. The parent now + * writes the canonical name, so the explicit flag arrives first in the order the + * child already reads. + * + * ## Why BOTH names, and why that is one statement rather than two channels + * + * `OS_PORT` alone would satisfy the CLI, because the CLI reads the pair through + * one reader with a declared precedence. But the child's environment is read by + * more than the CLI: app code and third-party libraries read `process.env.PORT` + * directly, and this repo has such a consumer in + * `examples/app-showcase/src/system/self-url.ts` + * (`env.OS_PORT?.trim() || env.PORT?.trim()`). Leaving a stale `PORT` behind + * would repair the bind and leave the app computing its own address from a port + * nothing is listening on — this card's defect, one layer down. So the pair is + * written together and always agrees: ONE value, on a canonical name and its own + * documented alias, not two channels that could ever disagree. + * + * ⛔ NOT forwarded as `--port` on argv, though `dev` does exactly that and it + * would also make the flag win. Argv and environment are two mechanisms with + * DIFFERENT precedences, and a `start` that stated the same port on both would + * be the shape this card is about: if they ever drifted, argv would silently win + * while the environment — the thing the app reads — lied. `dev` forwards on argv + * because it writes no port into the child's environment at all; one command, + * one channel, in both cases. + * + * ## The deprecation hazard the card flagged: MEASURED ABSENT, and inverted + * + * The card warned that writing `OS_PORT` might surface a deprecation notice the + * operator never caused. It cannot, for two independent reasons: + * + * - `OS_PORT` is the **preferred** argument of `readEnvWithDeprecation`, not + * the legacy one. The warning branch fires only when the preferred name is + * `undefined` and a LEGACY alias supplies the value, so setting the preferred + * name is the one input that can never reach it. `PORT` — what this command + * used to write, and still writes beside it — is the legacy half of the pair. + * - Every read site of this pair in the repository passes `{ silent: true }` + * (`commands/dev.ts`, `commands/serve.ts`, and the door in this file), which + * `env.ts` documents as the setting for aliases that are "accepted + * conventions rather than true legacy names — e.g. `PORT`, which PaaS + * platforms inject automatically". So no spelling of this pair can warn. + * + * Driven through a real `serve` child, all four shapes were silent: today's + * `PORT`-only write (bound the WRONG port, 41077), writing `OS_PORT`, deleting + * `OS_PORT`, and argv — the last three all bound 41078 and none printed a + * deprecation line. + * + * ## ⛔ `!== undefined`, never `flags.port ?` + * + * The falsy guard this replaces DROPPED `--port 0`, and 0 is a legal, useful + * port: `utils/port-contract.ts` declares `MIN_PORT = 0` from a measurement and + * states that 0 is "a REQUEST, not an error" — it asks the kernel for any free + * port. Measured on the unrepaired command, `OS_PORT=41077 os start --port 0` + * printed `http://localhost:0/_console/` and bound **41077**: the flag was never + * forwarded at all. The refusal door a few lines up already spells the test + * `flags.port !== undefined`; this is the same question asked the same way. + * + * @param flagPort `flags.port` — oclif has already normalised it to an integer. + * @returns the keys to merge into the child env; EMPTY when no flag was given, + * so an operator's own `$PORT`/`$OS_PORT` are inherited untouched. + */ +export function childPortEnv(flagPort: number | undefined): Record { + if (flagPort === undefined) return {}; + const value = String(flagPort); + return { OS_PORT: value, PORT: value }; +} + /** * Resolve the database URL for `objectstack start` — start's flag surface * mapped onto the ONE shared resolution (`resolveProjectDatabaseUrl`, #6469) diff --git a/packages/cli/test/start-port-banner-agreement.e2e.test.ts b/packages/cli/test/start-port-banner-agreement.e2e.test.ts new file mode 100644 index 0000000000..6a40180c48 --- /dev/null +++ b/packages/cli/test/start-port-banner-agreement.e2e.test.ts @@ -0,0 +1,296 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * `os start` serves the port it prints — end to end, on a real boot (#12992). + * + * ## Why this one has to be a real child process + * + * The defect was not inside either process. `start` resolved the port one way + * for its banner and handed the child a value on a channel the child resolved + * the OTHER way, and each half was self-consistent: a unit test of the parent + * saw a banner matching the flag, a unit test of the child saw a child honouring + * its own documented precedence. Only a boot that prints and then binds can see + * them disagree. Measured on `origin/main` before the repair: + * + * ``` + * OS_PORT=41077 os start --port 41078 + * banner: 🖥️ Console: http://localhost:41078/_console/ + * curl answers on: 41077 + * ``` + * + * ## What is asserted, and why it is not "the banner names the flag" + * + * The contract is AGREEMENT, not a particular number: every address `os start` + * prints names the port the server actually bound. So the bound port is read + * back out of the child's own ready banner (`boundPortFromBanner`, #12525) — + * never assumed from what this harness passed in, which is exactly the value a + * drift makes wrong — and the full output is then required to contain no other + * address at all. That phrasing survives causes this card never touched, + * including the #12543 auto-shift. + * + * ## ⚠️ Instrument: `OS_PORT` CONTAINS `PORT`, and absence needs a control + * + * The ports are compared as NUMBERS parsed out of `http://localhost:`, never + * by substring — `serve-port-validation.test.ts:111` records what a bare + * containment assertion does to this pair of names. And the "the losing port + * appears nowhere" assertion is paired with a positive control run through the + * SAME regex on the SAME captured text, so an empty result means the port is + * absent rather than that the probe never worked. + * + * ## ⛔ Why no leg passes `--no-ui` + * + * The row this card deleted was gated on `flags.ui`, so every assertion below + * would be blind to its return under `--no-ui` — measured: with the row put + * back, a `--no-ui` run of this whole file stayed GREEN and only the + * structural pin caught it. The legs therefore run the DEFAULT surface, which + * is both the operator's path and the only one where a parent-side prediction + * is observable. + * + * ## Spawn shape + * + * `bin/run.js` with `NODE_ENV` unset, deliberately (hence + * `requireBuiltCli`): that is the entrypoint an operator runs, and it is what + * lets `start` apply its own `NODE_ENV=production` default to the child — which + * closes `serve`'s auto-shift branch and makes the bind deterministic. A + * `bin/run-dev.js` child would pin `NODE_ENV=development`, re-open auto-shift, + * and turn a busy-port race on this shared container into a flake. + */ + +import { describe, it, expect, beforeAll, afterEach } from 'vitest'; +import { spawn, type ChildProcess } from 'node:child_process'; +import { mkdtempSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join, resolve, dirname } from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { + childEnv, + boundPortFromBanner, + requireBuiltCli, + reservePort, + holdPort, + TSX, + RUN_JS_RESOLVES_FROM_DIST, +} from './helpers/serve-process.js'; + +const HERE = dirname(fileURLToPath(import.meta.url)); +const RUN_JS = resolve(HERE, '../bin/run.js'); +const RUN_DEV_JS = resolve(HERE, '../bin/run-dev.js'); + +/** The banner tail `printServerReady` ends with — the boot is fully printed. */ +const BANNER_TAIL = /Press Ctrl\+C to stop/; + +const BOOT_TIMEOUT_MS = 180_000; + +// ESC spelled as a char code, never written as a raw control byte in source — +// the same construction `helpers/serve-process.ts` uses for its own stripper. +const ANSI_SGR = new RegExp(`${String.fromCharCode(27)}\\[[0-9;]*m`, 'g'); +const stripAnsi = (text: string): string => text.replace(ANSI_SGR, ''); + +/** Every `http://localhost:` address in some output, as numbers. */ +const addressesIn = (output: string): number[] => + [...stripAnsi(output).matchAll(/http:\/\/localhost:(\d+)\b/g)].map((m) => Number(m[1])); + +/** + * ⛔ Kill the process GROUP, never the child alone. + * + * `os start` is a supervisor: it spawns `os serve` as a SEPARATE process and + * then waits. Signalling only the `start` pid leaves that grandchild running — + * it is reparented to init, keeps its port, and keeps its ~450 MB resident. + * MEASURED while writing this file: repeated runs accumulated 13 orphaned + * `serve` processes, took the container to 14.4 GB of 16 GB and load 29, and + * the next run of this very suite then failed by TIMEOUT rather than by any + * assertion — a false red that says nothing about the code under test. + * + * `detached: true` gives the child its own process group; a negative pid + * signals that whole group, supervisor and server together. + */ +const killTree = (child: ChildProcess | undefined, signal: NodeJS.Signals): void => { + if (child?.pid === undefined) return; + try { process.kill(-child.pid, signal); } catch { /* group already gone */ } +}; + +let running: ChildProcess | undefined; +let workdir: string | undefined; + +afterEach(() => { + killTree(running, 'SIGKILL'); + running = undefined; + if (workdir) rmSync(workdir, { recursive: true, force: true }); + workdir = undefined; +}); + +/** Boot a real `os start` and capture everything it printed. */ +function bootStart( + env: Record, + args: string[], + entry: { exec: string; argv: string[] } = { exec: process.execPath, argv: [RUN_JS] }, +): Promise { + workdir = mkdtempSync(join(tmpdir(), 'os-start-port-')); + const home = join(workdir, 'home'); + + return new Promise((resolveBoot, rejectBoot) => { + const child = spawn(entry.exec, [...entry.argv, 'start', ...args], { + cwd: workdir, + // `childEnv`, never a bare `...process.env` — see its header (#11267). + // `NODE_ENV: undefined` is required by the built entrypoint (#11464): + // `development`/`test` sends oclif's command lookup back to `src/`. + env: childEnv({ + NODE_ENV: undefined, + NO_COLOR: '1', + OS_HOME: home, + OS_LOG_LEVEL: 'error', + ...env, + }), + stdio: ['ignore', 'pipe', 'pipe'], + // Own process group, so `killTree` can reach the `serve` grandchild. + detached: true, + }); + running = child; + + let output = ''; + let settled = false; + const finish = (err?: Error) => { + if (settled) return; + settled = true; + clearTimeout(timer); + killTree(child, 'SIGTERM'); + if (err) rejectBoot(err); else resolveBoot(output); + }; + + const timer = setTimeout( + () => finish(new Error(`os start never printed a complete banner.\n--- output ---\n${output}`)), + BOOT_TIMEOUT_MS, + ); + + const onData = (d: unknown) => { + output += String(d); + if (BANNER_TAIL.test(stripAnsi(output))) finish(); + }; + child.stdout?.on('data', onData); + child.stderr?.on('data', onData); + child.on('error', (err) => finish(err)); + child.on('exit', (code) => finish( + BANNER_TAIL.test(stripAnsi(output)) + ? undefined + : new Error(`os start exited ${code} before its banner.\n--- output ---\n${output}`), + )); + }); +} + +describe('`os start` — every address it prints is the port it bound', () => { + beforeAll(() => { + requireBuiltCli(RUN_JS_RESOLVES_FROM_DIST); + }); + + it( + 'an explicit --port beats an inherited $OS_PORT, and the banner agrees', + async () => { + // Two DIFFERENT free ports: the one the operator exported, and the one + // they typed. The defect is only visible when they disagree. + const envPort = reservePort(); + let flagPort = reservePort(); + if (flagPort === envPort) flagPort = reservePort(); + expect(flagPort).not.toBe(envPort); + + const output = await bootStart( + { OS_PORT: String(envPort) }, + ['--port', String(flagPort)], + ); + + // ── The bound port, read out of the CHILD's own banner ───────────── + const readback = boundPortFromBanner(output); + expect(readback.state, `banner unreadable.\n--- output ---\n${output}`).toBe('bound'); + // The explicit flag wins over the inherited environment variable, which + // is what `--port`'s help text ("overrides $PORT") has always promised. + expect(readback).toEqual({ state: 'bound', port: flagPort }); + + // ── …and NOTHING printed names the losing port ───────────────────── + const printed = addressesIn(output); + // ⭐ POSITIVE CONTROL: the same probe, over the same captured text, does + // find the bound port. Without this the assertion below would pass on an + // empty capture, a child that printed nothing, or a broken regex. + expect(printed, `no localhost address at all in:\n${output}`).toContain(flagPort); + expect( + printed.filter((p) => p !== flagPort), + 'os start printed an address it is not serving — the banner and the bind ' + + `disagreed again.\n--- output ---\n${output}`, + ).toEqual([]); + }, + BOOT_TIMEOUT_MS + 30_000, + ); + + it( + 'with no --port, an inherited $OS_PORT is still honoured and still agrees', + async () => { + // The other half of the contract: the repair must not have made the flag + // win by breaking the environment channel it is supposed to outrank. + const envPort = reservePort(); + + const output = await bootStart({ OS_PORT: String(envPort) }, []); + + expect(boundPortFromBanner(output)).toEqual({ state: 'bound', port: envPort }); + + const printed = addressesIn(output); + expect(printed, `no localhost address at all in:\n${output}`).toContain(envPort); + expect( + printed.filter((p) => p !== envPort), + `os start printed an address it is not serving.\n--- output ---\n${output}`, + ).toEqual([]); + }, + BOOT_TIMEOUT_MS + 30_000, + ); + + it( + 'still agrees when the CHILD moves the port under it — the auto-shift case', + async () => { + // ⭐ The generalisation the ruling asked for. The two halves of this card + // (the channel, the recomputed banner) are one instance of a wider + // property: `start` must not answer "which port?" at all. This leg proves + // the property under a cause this card never touched — #12543's + // development auto-shift, where the port changes AFTER `start` has + // handed it over and the child hops to the next free one. + // + // Any parent-side prediction is wrong here BY CONSTRUCTION, whatever + // precedence it uses and however correct the forwarding channel is: + // nothing in the parent can know the hop happened. + // + // `bin/run-dev.js` is the entrypoint, deliberately: it pins + // `NODE_ENV=development`, which is what opens `serve`'s auto-shift branch + // (`flags.dev || NODE_ENV === 'development'`). ⛔ Not `bin/run.js` with an + // explicit `NODE_ENV=development` — that is the ts-path reroute + // `check:cli-test-child-env` rule 3 refuses at a built-entrypoint spawn. + const held = await holdPort(); + try { + const output = await bootStart( + {}, + ['--port', String(held.port)], + { exec: TSX, argv: [RUN_DEV_JS] }, + ); + + const readback = boundPortFromBanner(output); + expect(readback.state, `banner unreadable.\n--- output ---\n${output}`).toBe('bound'); + const bound = (readback as { state: 'bound'; port: number }).port; + + // The port really did move — without this the leg would pass for the + // trivial reason that nothing shifted, and would be measuring nothing. + expect( + bound, + 'the port did not shift, so this leg tested nothing: something freed ' + + `${held.port} before the child bound it.\n--- output ---\n${output}`, + ).not.toBe(held.port); + + const printed = addressesIn(output); + expect(printed, `no localhost address at all in:\n${output}`).toContain(bound); + expect( + printed.filter((p) => p !== bound), + 'os start printed the port it ASKED for, not the one the child bound. A ' + + 'parent-side prediction cannot survive the auto-shift — the address has ' + + `to come from the child.\n--- output ---\n${output}`, + ).toEqual([]); + } finally { + await held.release(); + } + }, + BOOT_TIMEOUT_MS + 30_000, + ); +});