Skip to content

fix(cli): os serve refuses a port that cannot be a port, naming what the operator set (#12662) - #12676

Merged
os-litant merged 1 commit into
mainfrom
claude/issue-12662-port-flag-validation
Aug 27, 2026
Merged

fix(cli): os serve refuses a port that cannot be a port, naming what the operator set (#12662)#12676
os-litant merged 1 commit into
mainfrom
claude/issue-12662-port-flag-validation

Conversation

@os-litant

Copy link
Copy Markdown
Collaborator

Fixes#12662

--port was a Flags.string whose only consumer was a bare parseInt. os serve --port abc therefore became NaN, travelled the whole port policy untouched, and reached the real listen() — which refused it at the socket layer with ERR_SOCKET_BAD_PORT, a sentence about options.port bounded at 0 and below 65536. The operator mistyped a flag and got back an error naming an internal option, raised from a code path with no connection to the thing they typed. --port 99999 parses fine and died in the same place, and PORT=abc / OS_PORT=abc are the same defect through a different door.

 ✗ Invalid port: OS_PORT="abc"
A port must be a whole number from 0 to 65535 — 0 is legal, and
asks the kernel for any free port. Nothing was started, and no socket
was opened.
Correct OS_PORT in this process's environment (for example OS_PORT=3000),
or override it with --port 3000.

The bounds are measured, not copied

node -e "require('net').createServer().listen(N)", this checkout, Node v22.22.2:

Nresult
0OK — bound 43025 (kernel-assigned)
1, 3, 1023, 3000, 65534, 65535OK
65536, 70000, -1, NaN, 3000.5ERR_SOCKET_BAD_PORT

So 0 is accepted: a floor of 1 would have refused a value that boots today. And the ceiling is 65535, one less than the exclusive 65536 the kernel's own sentence names. Both numbers live in one pair of constants and the refusal interpolates them, the rule #12620 landed in this file for PORT_SEARCH_SPAN.

Why not a validating Flags.integer({ min, max }) — measured, then not taken

Two measurements, both against this repo's @oclif/core 4.13.3.

1. It cannot see the environment. In lib/parser/parse.js the default branch's value function simply returns flag.default; unlike the argv and flag.env branches it never calls parseFlagOrThrowError. Confirmed at runtime — an integer flag with min: 0, max: 65535:

--port "abc" → THREW CLIError: Parsing --port … Expected an integer but received: abc
--port "99999" → THREW CLIError: … less than or equal to 65535 but received: 99999
(no argv) default="abc" → value="abc" typeof=string setFromDefault=true ← unvalidated
(no argv) default=999999 → value=999999 typeof=number setFromDefault=true ← unvalidated

PORT / OS_PORT arrive through that default, so an integer flag would have guarded --port alone and left two of the three reported paths dying exactly as before — one third of the card.

2. It would narrow what boots.Flags.integer's parser is /^-?\d+$/, which refuses " 3000", "3000 ", "3000.0", "0x0BB8", "+3000" and "3e3" — every one of which parseInt accepts and every one of which boots a server today. Leading whitespace on a production PORT is the realistic one.

So parseInt stays the reader and only the refusal is added, at the point all three inputs converge and ahead of the port-conflict policy: one guard covers the development auto-shift, the production refusal and a boot that enters neither, and it runs before anything probes or binds. metadata.flags.port.setFromDefault (oclif's own record) is what separates --port from the environment; the env half mirrors readEnvWithDeprecation('OS_PORT', 'PORT') and is pinned against it, OS_PORT="" included.

The accepted-input set is unchanged, and that is a test

serve-port-validation.test.ts carries a 17-row table of what each spelling does today (parseInt value, and whether that number reaches a bound socket). Every row that boots today must still return the same port; every row that dies at listen() today must be refused. That arm reds the moment anyone tightens this to integer-flag semantics — which is where such a change should have to come and argue, because it would narrow a published CLI's accepted input.

One consequence of keeping parseInt is preserved rather than hidden: --port 3e3 still binds port 3, not 3000. That is a different defect — a value accepted as something other than what it says, rather than a value that dies — and repairing it narrows the accept set, so it is filed as #12674 with the contract question stated rather than answered here.

Known limit, stated rather than papered over

os dev forwards its own --port (and $PORT, promoted to a flag) to the serve child on argv; os start forwards its --port as PORT in the child's environment. On those spawns this names the channel the value arrived on, which is not always the one the operator typed. Filed separately as #12673 — both parent commands own their own flag validation.

Verification

All of it on 6989019bb, the tree of the commit in this PR.

  • pnpm --filter @objectstack/cli exec vitest run196 files, 2227 tests, all passing (the whole CLI suite, because this is a boot path).
  • pnpm --filter @objectstack/cli typecheckTYPECHECK_EXIT=0. This package's tsconfig.json has no exclude, so the new test file is inside the program (the build tsconfig excludes tests, deliberately).
  • pnpm lint (eslint . --no-inline-config, repo-wide) — exit 0, 56s.
  • Gate families from node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack: 24 path-matched + 6 convention-triggered families run locally, all exit 0 — including check:cross-package-test-inputs, check:type-check-coverage, check:type-check-debt (ratchet, re-run with the closure built), check:i18n, check:i18n-coverage, check:engine-double-contract, check:where-matcher, check:query-options-erasure, check:nul-bytes, and the changeset family.
  • node scripts/pm/check-half-states.mjs exits 3 — PREREQUISITE NOT MET, this container has no usable GitHub credential for bare REST. That is NOT MEASURED, not a red.

Ablation — no sockets bound; the guard is what is deleted, and the mutation was confirmed on disk (anchored grep counts for injected and removed text) before any verdict was read, with restore proven by blob hash and an empty git diff HEAD, never an exit code. This is a source-resolved suite, so no rebuild is involved in either leg; the mutation and the restore both act on the file the test imports:

ablationresult
the range/NaN refusal removed5 failed / 5 passed `abc` is no longer refused: expected NaN to be null, expected 99999 to be null, 65536 was taken from the message instead of measured
source naming collapsed to --port1 failedthe refusal would name --port, but the value came from elsewhere
the guard unwired from the boot1 failedthe port guard has no call site: expected -1 to be greater than -1

One negative assertion was widened during the work rather than loosened: OS_PORT="abc"containsPORT="abc", so a plain not.toContain pair reported the OS_PORT refusal as also naming PORT. The anchor now disqualifies a match preceded by _; a message that really named both would still be caught.

Changeset

minor on @objectstack/cli, derived rather than copied: this adds an observable refusal path and a new operator-facing message to a published package — a functional improvement, not a silent internal repair, so AGENTS.md's "pure bug fixes do not require a changeset" does not cover it. It removes and renames nothing and narrows no accepted input, so it is not breaking and carries no ADR-0087 marker (check-adr-0087-registration agrees: no declared-breaking changeset in this diff). #12620 landed minor for the sibling notice in this file; this arrives at the same answer by its own route.

Generated by Claude Code


Generated by Claude Code

…the operator set (#12662)
`--port` was a string flag whose only consumer was a bare `parseInt`, so
`--port abc` became `NaN`, travelled the whole port policy untouched, and
reached the real `listen()` — which refused it at the socket layer with
`ERR_SOCKET_BAD_PORT: options.port should be >= 0 and < 65536`. The operator
mistyped a flag and got back an error naming an internal option, from a code
path with no connection to the thing they typed. `--port 99999` died the same
way, and `PORT=abc` / `OS_PORT=abc` are the same defect through another door.
The value is now checked at the point all three inputs converge, before the
port-conflict policy and before any socket exists. The refusal names which
input was used, and states the range by interpolating the bounds the code
enforces rather than a second hand-written copy of them.
The bounds are measured, not copied: `listen(0)` binds a kernel-assigned port,
so 0 is accepted; the ceiling is 65535, one less than the `< 65536` the
kernel's own message names.
`Flags.integer({ min, max })` was measured and not taken. oclif never runs a
flag's parser over a `default`, and `PORT`/`OS_PORT` arrive through the default
— so an integer flag would have guarded `--port` alone and left two of the
three reported paths dying exactly as before. It would also have narrowed what
boots: its `/^-?\d+$/` refuses `" 3000"`, `"3000.0"`, `"0x0BB8"`, `"+3000"`
and `"3e3"`, all of which boot today. `parseInt` therefore remains the reader
and only the refusal is added, so the accepted input set is unchanged.
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 8 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 68bf4efc6ac3e18385496432012eaf9aba09cc39.

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)
  • the SDK route bridge reached 47 of 219 client-bound route-ledger rows — the other 172 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 172: 14 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 102 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • 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 68bf4efc6ac3e18385496432012eaf9aba09cc39packageMentionDocs.

Which tree this was computed on

This run read content/docs from 75831a38a2188b7289fa7f447fed8df660698c29 — the merge of head 6989019bb5fa03f19e56b6e93a23d32343ba34ca into base 68bf4efc6ac3e18385496432012eaf9aba09cc39, 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 75831a38a2188b7289fa7f447fed8df660698c29 && git checkout 75831a38a2188b7289fa7f447fed8df660698c29
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 68bf4efc6ac3e18385496432012eaf9aba09cc39 6989019bb5fa03f19e56b6e93a23d32343ba34ca && git checkout -B drift-repro 68bf4efc6ac3e18385496432012eaf9aba09cc39 && git merge --no-ff 6989019bb5fa03f19e56b6e93a23d32343ba34ca
node scripts/docs-audit/affected-docs.mjs --json 68bf4efc6ac3e18385496432012eaf9aba09cc39

⚠️ 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 68bf4efc6ac3e18385496432012eaf9aba09cc39 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

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

Labels

documentationImprovements or additions to documentationsize/lteststooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[finding] os serve --port abc is never validated — parseInt yields NaN and the boot dies on a raw ERR_SOCKET_BAD_PORT

2 participants

@os-litant@claude