Uh oh!
There was an error while loading. Please reload this page.
feat(cli): os serve announces a shifted port, naming the one you asked for and the one it took (#12543) - #12621
Conversation
…und (#12543) In development `os serve` hops to the next free port when the requested one is taken. The hop is correct and stays exactly as it is (#11113 owns the production half, which refuses to drift). What was missing is that it happened silently: the ready banner prints the port that was BOUND and no line said it was not the port that was ASKED FOR, so every reader had to hold both numbers and compare — the work five landed consumer-side PRs each redid by hand. serve.ts holds requestedPort and port in one scope at the moment it shifts, so it now says both, once, when they differ. Channel is measured, not chosen: stdout is a JSON-RPC channel whenever the stdio MCP transport is mounted, so the notice goes through printDiagnostic to stderr — the same helper, stream and boot position as the production refusal in the sibling else branch. It also sits well before the boot-quiet window opens, so unlike a boot-phase logger.warn it cannot be swallowed and it survives a boot that later dies. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UjujZN219uFzBhSYfMykCd
…hem (#12543) A bare SIGKILL lands on the `tsx` shim, which cannot forward it, so the real `os serve` child survived, re-parented to init and still holding the runner's stdio pipes — measured while writing this file, where it kept a probe alive long after its assertions had passed. Use the SIGTERM-then-SIGKILL shape every other spawner in this directory already uses, and await the exit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UjujZN219uFzBhSYfMykCd
📓 Docs Drift CheckThis PR changes 1 package(s): 17 hand-written doc(s) name something this change touched — list omitted above 15 rows. Re-derive on the tree named below: ⛔ 4 release-owned page(s) also affected — read-only, see AGENTS.md Documentation Guardrails. What this run could not see
Coarse fallback — 23 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): Which tree this was computed onThis run read A worktree cut from an older # while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 48b551b8614d24be04aa712a3a1c415847cb968a && git checkout 48b551b8614d24be04aa712a3a1c415847cb968a
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 527e0505d8729ae022121a992b62326035953a82 6c296f89bfbc5338867c5d595c99ae3f185bfab6 && git checkout -B drift-repro 527e0505d8729ae022121a992b62326035953a82 && git merge --no-ff 6c296f89bfbc5338867c5d595c99ae3f185bfab6
node scripts/docs-audit/affected-docs.mjs --json 527e0505d8729ae022121a992b62326035953a82
|
Uh oh!
There was an error while loading. Please reload this page.
Fixes#12543
In development
os servehops to the next free port when the requested one is taken. That behaviour is correct and is unchanged here — auto-shift is deliberate, and #11113 owns the production half, where a busy port is a loud refusal. ⭐ The defect this PR fixes is that a correct behaviour was silent: the ready banner prints the port that was BOUND, and no line anywhere said it was not the port that was ASKED FOR.serve.tsholds both numbers in one scope at the moment it shifts, so it now says both, once, when they differ.Anchors — re-derived on
origin/main@7a25e7d60, cited as phrasesconst getAvailablePort = async (startPort: number)port = await getAvailablePort(requestedPort);if (!bootQuiet) process.stderr.write(text + '\n');⛔ The
flags.dev || process.env.NODE_ENV === 'development'condition is untouched,portAutoShiftAllowedis neither narrowed nor gated, and no shifted boot is made to fail. The diff adds oneif (port !== requestedPort)block and nothing else inserve.ts.What it prints
Three facts, not one: what was asked for, that it could not be had, what was taken. A line naming only the bound port is what the banner already prints, and it is what five landed consumer-side PRs (#12523, #12546, #12552, #12565, plus #12441's probe) each had to parse and compare by hand.
CHANNEL — measured, not chosen
This was the sharpest constraint on the card, so here is what was measured rather than assumed.
stdoutis never available to a diagnostic in this command, in any mode.run()'s first statement isredirectStdoutToStderr(), held for the life of the process, and the comment above it says why: withOS_MCP_STDIO_ENABLED=truethe MCP stdio transport owns stdout and its protocol is newline-delimited JSON. The transport keeps its own handle to the real stdout (packages/mcp,protocol-stdout.ts) rather than depending on who booted it.printDiagnostic—process.stderr.writedirectly — which is the same helper, the same stream and the same point in the boot as the production-mode refusal in the siblingelse ifbranch. That refusal is this notice's exact counterpart under the other half of the policy, so they now match.logger.warnhere would have been a notice nobody sees. The boot-quiet window (CLI startup banner does not surface flow-name shadowing, though it already reads the rows that carry it #12028 / PR fix(cli): the startup banner names a contested flow name and says which definition is armed (#12028) #12562) interceptsprocess.stdout.write,console.logandconsole.debugand replays kernel records only after the banner. The notice sits well before that window opens —bootQuietis still its initialfalseat this point — and it writes to stderr rather than throughconsole.log, so it is outside the window on both counts. It also survives a boot that dies later, which matters: a drifted port is a plausible cause of such a death.Proof the channel choice is safe, not merely argued:
serve-stdio-stdout-purity.e2e.test.ts— the file that exists to pin stdout purity in JSON-RPC modes — passes unchanged (Tests 1 passed (1)). And the new positive case asserts directly that a drifted child's stdout is the empty string, which the standalone probe measured asSTDOUT_BYTES=0.Premise re-derivation —⚠️ one fork, reported and NOT reconciled
The card measured: port
32869held ⇒ child bound32871(explicitly "not 32870"), printedServer is ready, never exited; and the harness's own next request to the reserved port was answered by the neighbour.Re-derived on this tree with a real
http.createServerneighbour:API: http://localhost:34896/), the child never exits.getAvailablePort's walk did not skip a port here — it tookrequested + 1. The card's32869 ⇒ 32871was almost certainly a second holder on32870at that moment on this shared container, not a property of the search: the loop is a plainport++with no skip in it. ⛔ Nothing is reconciled — the number measured is the number reported, and the card's "not 32870" does not reproduce.The mechanism the card is about is unaffected by the fork: the drift is real, correct, and was silent.
Tests
New:
packages/cli/test/serve-port-drift-notice.e2e.test.ts— one regex asserted PRESENT in the drifted boot and ABSENT in the clean one, so neither case can pass by the regex having quietly stopped matching.serve-mcp-*/serve-stdio-*spawners still fail port contention with the generic "serve exited 1" — they get #12441's probe but not its named error #12526 and ⚠️serve-process-child-env.e2e.test.tsasserts a SECURITY posture against the port it asked for, not the one the child bound — a lost race lets a neighbouring server answer #12548 used). Asserts the drift really happened first, then that the notice names both numbers, then that stdout is empty, then that the requested port is still answered by the stranger.The file owns its spawn rather than calling
runServe(), becauserunServe()now rejects a drifted boot (#12525's read-back doing its job), which would make the condition under test unreachable. It owns its HTTP neighbour because the sharedholdPort()binds a bare TCP socket and cannot answer — and "something else answered where you were pointed" is the specific harm here.Ablation
The notice block was deleted from
serve.tsand the positive case re-run:Two things worth reading there. The ablated blob
17e710912isserve.tsexactly as it stands onmain— the mutation reproduces the pre-change file byte for byte, so the ablation removes this PR's contribution and nothing else. And the failure lands after the "the child bound a different port" guard passed: the boot still drifted and the notice was simply absent, which is red for the right reason rather than a setup error. No rebuild stands between the mutation and the test — the child loadspackages/cli/srcthroughtsxviabin/run-dev.js, so the edited source is what ran. Restore is proven by blob hash and an emptygit diff HEAD, never by an exit code; the script carried atrap … EXIT INT TERMwhose restore leg wasgit checkout HEAD --followed by the absolute path to the file.Verification run — narrowing declared
packages/clivitest suite does not finish inside this container's ~10-minute foreground window, so it was narrowed to a stated consumer set and every run went through the shared verify lock. Union re-run on the final commit6c296f89b, working tree clean:serve-port-drift-notice.e2e.test.tsTest Files 1 passed (1)·Tests 2 passed (2)serve-stdio-stdout-purity.e2e.test.ts(the channel pin)Tests 1 passed (1)serve-port-readback.e2e.test.ts+serve-port-bind-probe.test.tsTest Files 2 passed (2)·Tests 22 passed (22)— includes the forced-drift arm, the case most exposed to a new stderr linepnpm --filter @objectstack/cli typecheckpnpm --filter '@objectstack/cli^...' buildVERDICT command-exit 0check:nul-bytes,check:cli-test-child-env,check:cross-package-test-inputs,check:engine-double-contract,check:where-matcher,check:query-options-erasure,check:route-envelope,check:type-check-coverage,check:i18n, the changeset family, …)check:i18n-coverage— "COULD NOT MEASURE — 1 of 12 config(s) failed to lint", because@objectstack/connector-mcphas no build output here. Its own text: "Nothing was compared … this result says NOTHING about whether any declared label went untranslated."check:type-check-debt— "--re-measure cannot run: 1 workspace dependenc(ies) … have no built type entry point on disk". It refuses outright rather than measuring a different world.The risk that hides behind the second one was closed directly instead:
TEST_DEBT['@objectstack/cli']is a frozen count of 146 over the hiddentest/layer (tsconfig.jsonsaysinclude: ["src"], sopnpm --filter @objectstack/cli typecheckreads no file undertest/— its green says nothing about the new file). The new test file was therefore type-checked directly under the ledger's compiler settings: 0 diagnostics, so it cannot raise the frozen count. That measurement is sound for this file in particular because it imports no@objectstack/*package at all — only node builtins,vitest, and its sibling helper — so the unresolved-workspace-import hazard the gate warns about has nothing to act on here. CI runs both gates on a fully built tree and is the authority.Changeset
.changeset/cli-serve-port-drift-notice.md,@objectstack/cli: minor. Rule applied: AGENTS.md §Post-Task Checklist — "Add a changeset for feature work … Pure bug fixes do not require a changeset." This adds user-visible output toos serve, so it is a functional improvement rather than a pure fix. Not breaking, so no ADR-0087 disposition marker is owed (check:adr-0087-registrationexit 0).Also in this branch
A second commit fixes the teardown of the new file's spawns: a bare
child.kill('SIGKILL')lands on thetsxshim, which cannot forward it, so the realos servesurvived re-parented to init and still holding the runner's stdio pipes — measured while writing this file, where it kept a probe alive long after its assertions had passed. It now uses the SIGTERM-then-SIGKILL shape every other spawner in this directory already uses. Swept the directory: no other file has a SIGKILL without a SIGTERM beside it, so nothing else is owed.Out of scope, filed separately
#12620 — when
getAvailablePortexhausts its 100-port search it throws a message that names the problem exactly, the caller discards it, and boot falls through to bind the port it just proved was busy. That path gets neither the production refusal (wrong branch) nor this PR's notice (port === requestedPortthere). ⛔ Not folded in: repairing it changes whatos servedoes, which this card's rulings forbid.Generated by Claude Code