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, + ); +});