Skip to content

feat(cli): os serve announces a shifted port, naming the one you asked for and the one it took (#12543) - #12621

Merged
os-litant merged 2 commits into
mainfrom
claude/issue-12543-port-drift-notice
Aug 27, 2026
Merged

feat(cli): os serve announces a shifted port, naming the one you asked for and the one it took (#12543)#12621
os-litant merged 2 commits into
mainfrom
claude/issue-12543-port-drift-notice

Conversation

@os-litant

@os-litant os-litant commented Aug 27, 2026

Copy link
Copy Markdown
Collaborator

Fixes #12543

In development os serve hops 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.ts holds both numbers in one scope at the moment it shifts, so it now says both, once, when they differ.

⚠️ Two phrases below are written without angle brackets on purpose — a generic parameter and a placeholder were both silently eaten out of the first version of this body by GitHub's sanitizer, inside backticks included. They are spelled in words where that happened.

Anchors — re-derived on origin/main @ 7a25e7d60, cited as phrases

what the phrase it is anchored to
the search const getAvailablePort = async (startPort: number)
the policy switch `const portAutoShiftAllowed = flags.dev
the call port = await getAvailablePort(requestedPort);
the channel helper if (!bootQuiet) process.stderr.write(text + '\n');

⛔ The flags.dev || process.env.NODE_ENV === 'development' condition is untouched, portAutoShiftAllowed is neither narrowed nor gated, and no shifted boot is made to fail. The diff adds one if (port !== requestedPort) block and nothing else in serve.ts.

What it prints

  ⚠ Port 34895 is in use — serving on 34896 instead.
     Development auto-shift: 34895 was not free, so this server took
     the next one that was (34896). Anything still pointed at 34895 — a
     proxy, an OAuth callback URL, another terminal, a test harness — is
     talking to whatever holds 34895, not to this server.

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.

  1. stdout is never available to a diagnostic in this command, in any mode. run()'s first statement is redirectStdoutToStderr(), held for the life of the process, and the comment above it says why: with OS_MCP_STDIO_ENABLED=true the 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.
  2. The notice therefore goes through printDiagnosticprocess.stderr.write directly — which is the same helper, the same stream and the same point in the boot as the production-mode refusal in the sibling else if branch. That refusal is this notice's exact counterpart under the other half of the policy, so they now match.
  3. A logger.warn here 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) intercepts process.stdout.write, console.log and console.debug and replays kernel records only after the banner. The notice sits well before that window opens — bootQuiet is still its initial false at this point — and it writes to stderr rather than through console.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 as STDOUT_BYTES=0.

Premise re-derivation — ⚠️ one fork, reported and NOT reconciled

The card measured: port 32869 held ⇒ child bound 32871 (explicitly "not 32870"), printed Server 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.createServer neighbour:

NEIGHBOUR_HOLDS=34895
REQUESTED=34895 BOUND=34896 DELTA=1
ANSWER_ON_REQUESTED_PORT={"iAm":"A NEIGHBOURING AGENT DEV SERVER, not os serve"}
REACHED_BANNER=true   CHILD_EXITED=false   STDOUT_BYTES=0
  • ✅ The drift happens, the boot succeeds, the banner prints the bound port (API: http://localhost:34896/), the child never exits.
  • ✅ The requested port is answered by the stranger, with the card's exact string.
  • ⚠️ DELTA=1, not 2. getAvailablePort's walk did not skip a port here — it took requested + 1. The card's 32869 ⇒ 32871 was almost certainly a second holder on 32870 at that moment on this shared container, not a property of the search: the loop is a plain port++ 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.

The file owns its spawn rather than calling runServe(), because runServe() 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 shared holdPort() 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.ts and the positive case re-run:

HEAD_BLOB=12f79c1f6b82bbf481e16c205aca6cdf35e4fd64
BEFORE_BLOB=12f79c1f6b82bbf481e16c205aca6cdf35e4fd64   BEFORE_MARKER_COUNT=1
AFTER_BLOB=17e710912e6acc4ed102dec4487915a4111e3732    AFTER_MARKER_COUNT=0
MUTATION_CONFIRMED_ON_DISK=yes
  × POSITIVE — a really-held port produces a notice carrying REQUESTED and BOUND
  AssertionError: no drift notice on stderr for a boot that DID drift
ABLATED_VITEST_EXIT=1
RESTORED_BLOB=12f79c1f6b82bbf481e16c205aca6cdf35e4fd64  GIT_DIFF_HEAD_EMPTY_EXIT=0  RESTORE_MARKER_COUNT=1

Two things worth reading there. The ablated blob 17e710912 is serve.ts exactly as it stands on main — 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 loads packages/cli/src through tsx via bin/run-dev.js, so the edited source is what ran. Restore is proven by blob hash and an empty git diff HEAD, never by an exit code; the script carried a trap … EXIT INT TERM whose restore leg was git checkout HEAD -- followed by the absolute path to the file.

Verification run — narrowing declared

⚠️ The full packages/cli vitest 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 commit 6c296f89b, working tree clean:

run result
serve-port-drift-notice.e2e.test.ts Test 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.ts Test Files 2 passed (2) · Tests 22 passed (22) — includes the forced-drift arm, the case most exposed to a new stderr line
pnpm --filter @objectstack/cli typecheck exit 0
dependency closure pnpm --filter '@objectstack/cli^...' build lock VERDICT command-exit 0
22 gate families (check: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, …) all exit 0

⚠️ Two gates report COULD NOT MEASURE on this worktree and are stated as such rather than as passes — both refuse because only the CLI's dependency closure is built here, not the whole workspace, and both say so in their own words:

  • check:i18n-coverage"COULD NOT MEASURE — 1 of 12 config(s) failed to lint", because @objectstack/connector-mcp has 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 hidden test/ layer (tsconfig.json says include: ["src"], so pnpm --filter @objectstack/cli typecheck reads no file under test/ — 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 to os 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-registration exit 0).

Also in this branch

A second commit fixes the teardown of the new file's spawns: a bare child.kill('SIGKILL') lands on the tsx shim, which cannot forward it, so the real os serve 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. 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 getAvailablePort exhausts 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 === requestedPort there). ⛔ Not folded in: repairing it changes what os serve does, which this card's rulings forbid.


Generated by Claude Code

os-litant and others added 2 commits August 26, 2026 23:53
…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
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/cli, touching 1 documentable anchor(s).

17 hand-written doc(s) name something this change touched — list omitted above 15 rows. Re-derive on the tree named below: node scripts/docs-audit/affected-docs.mjs --json 527e0505d8729ae022121a992b62326035953a82.

4 release-owned page(s) also affected — read-only, see AGENTS.md Documentation Guardrails.

What this run could not see
  • 1 name(s) were too generic to anchor anything (single lowercase words)
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 23 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json 527e0505d8729ae022121a992b62326035953a82packageMentionDocs.

Which tree this was computed on

This run read content/docs from 48b551b8614d24be04aa712a3a1c415847cb968a — the merge of head 6c296f89bfbc5338867c5d595c99ae3f185bfab6 into base 527e0505d8729ae022121a992b62326035953a82, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# 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

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs 527e0505d8729ae022121a992b62326035953a82 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation size/m tests tooling

Projects

None yet

1 participant