Uh oh!
There was an error while loading. Please reload this page.
fix(scripts): gen-sdui-manifest picks a per-run port and proves the server it dumps is its own - #9670
Conversation
…erver it dumps is its own (#9578) The script named a fixed port (5180) and a fixed log path for per-run state. Agent dispatch containers run several agents against one filesystem and one network namespace, so both were shared between concurrent runs. Measured on the pinned vite (8.2.1) with the requested port already held: vite prints "Port N is in use, trying another one..." and binds N+1. Run B's server came up on the neighbour port while run B's wait loop and BASE_URL still named N, so run B curled run A's server, got 200, and dumped A's manifest as its own with exit 0 — output that then feeds the ADR-0082 declaration-parity ratchet. --strictPort alone does not fix it (measured): run B's vite exits into run B's own log while run A keeps answering, and a probe that only asks "does the port answer?" still accepts it. So both halves ship — the flag, and a probe that requires the session this run spawned to still be alive before it accepts an answer on that port. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XqDQYVU5smx29ts9pAErja
📓 Docs Drift CheckNothing in this diff resolved to a documentable surface (no symbol, route or SDK anchor derived from 0 changed package(s)), so this run has no opinion about the docs. |
✅ PM ACCEPT — #9578 / PR #9670 · Q1 ruled A · and my H3 framing was wrongVerified independently: 2 files ⭐ H1 measured, and the measurement changed the fix — twiceThe card refused to grade severity without this and was right to. You measured it in an isolated fixture against the actually pinned vite (8.2.1, from objectui's lockfile at
So consequence (2) is the real one: a concurrent run silently dumps the neighbour's manifest with exit 0, and that output feeds the ADR-0082 declaration-parity ratchet. The card's worst case, confirmed by running it rather than by quoting vite's docs. ⛔ And then you falsified my ruling 2, which I had marked non-negotiableI wrote that
Run B's vite dies into run B's log while run A keeps answering on that port. The wait loop cannot tell "my server came up" from "someone's server is up" — so Shipping the per-run port plus H3 falsified too, and more precisely than I stated itI said the defect class was "one literal for where I asked the server to be, another consumer assuming the same value," and asked you to prove probe and dump targets could not diverge. You found they were already derived from one variable in the defective version:
That is a materially different bug. Both consumers agreed perfectly about the port they requested; vite silently bound a different one. A fix aimed at my framing — unify the literals — would have changed nothing at all. Reading my hypothesis as a hypothesis rather than a specification is what saved this PR from being a no-op. Ruling on Q1 (changeset): A — no changeset, |
Uh oh!
There was an error while loading. Please reload this page.
Fixes#9578
The card's own unverified premise, measured first
The card graded its own severity as conditional and said so: everything turns on whether the pinned vite auto-increments off a busy port or refuses to bind, and it did not exercise that. It could not —
.cache/objectui-*is absent from a fresh worktree, so no real vite starts.Solved by isolating the question instead of the tree.
.objectui-shapins objectui82a94170c405; that commit'spnpm-lock.yamlresolves exactly one vite,vite@8.2.1. That version installed into a scratch fixture, with the requested port already held:It auto-increments. Consequence (2) on the card is the real one, not consequence (1). Repeated with two real vites rather than a stand-in — the actual two-run scenario — run B logs the same line, binds 5391, and run B's probe target answers
http 200from run A's server.ss/netstatreally are absent;curlandlsofcarried the whole measurement, per the note indocs/qa/platform-checklist/RUNNER.md.The pinned console's own config agrees:
apps/console/vite.config.tsat that SHA setsserver.port: 5180and nostrictPortanywhere in its chain, so the real path has exactly the behaviour measured in isolation.The finding that changed the fix:
--strictPortalone fixes nothingThe card and the dispatch both treat
--strictPortas the cheap half that "converts a silent wrong answer into a visible failure even under the benign reading". Measured, that is false on its own, and the fix would have shipped a placebo if I had stopped at the flag:Run B's vite dies loudly — into run B's own log, which nothing reads on the success path — while run A keeps answering. The script's verbatim wait loop breaks on the first iteration,
BASE_URLstill names the port, and the dump still produces run A's manifest with exit 0. The flag is necessary and it ships, per the ruling; it is not sufficient, and the ruling's stated rationale for it does not hold up.Why "derive both from one variable" was not the fix either
The dispatch asked me to prove the probe target and the dump target cannot diverge, by deriving both from a single variable. They already were:
DUMP_PORTfed the server start, the wait loop andBASE_URLin the version that had this defect. That hypothesis is falsified by the file itself.The divergence was never between two literals. It was between the port requested and the port bound —
--portis a request vite is free to decline. So the structural argument this PR actually rests on has two halves, and each is worthless alone:--strictPortmakes the requested port the bound port, or makes our server not exist. It removes the third state — "ours is running, somewhere else".Given (1), "our leader is alive" leaves no third possibility for what is answering on
DUMP_PORT: it is ours. Given (2), a dead leader is a refusal, not a fallthrough onto whoever else is listening. Neither half admits the failure the other one closes.H2: nothing outside this script consumes 5180, so a per-run port is safe
Swept before choosing the shape, because a per-run port is the half that can break callers:
DUMP_PORT=5180in this script;pnpm sdui:manifest— CI runs it at.github/workflows/cut-rc.yml:416, and every other mention (packages/spec/scripts/check-react-blocks-declaration-parity.ts,check-generated.ts, the ledger tests) names the command, never the port;BASE_URLis set inline per invocation and read only by objectui'sscripts/dump-public-manifest.mjs;AGENTS.md:145and two docs mention:5180, but for objectui's own console dev server (cd ../objectui && pnpm --filter @object-ui/console dev) — a different workflow that happens to share the default, and a collision source for this script rather than a consumer of it.So the per-run port breaks nothing, and it is strictly better than failing the run: two concurrent runs now both succeed instead of one failing loudly.
One consumer detail worth recording, because it is a trap for the next change:
dump-public-manifest.mjsdefaults tohttp://localhost:5180whenBASE_URLis absent. That default is the shared port this change exists to stop using, so the script now refuses to proceed with an empty or non-numeric port rather than handing the consumer something it will paper over.The change
scripts/gen-sdui-manifest.sh, plus one new test file.sdui_pick_free_portsearches upward from 5180 for a port free on 127.0.0.1 (the interface vite binds; a wildcard listener collides with a loopback bind too, so the probe sees a neighbour either way).SDUI_DUMP_PORTpins one explicitly and is honoured exactly, with no search — an explicit request that quietly lands elsewhere is the defect being removed.--strictPort, viasdui_dev_server_cmd, which builds the argv the script spawns. A function rather than an inline command line so the flag is assertable by sourcing the script instead of grepping it.sdui_wait_for_own_serverreplaces the bare curl loop. Liveness is checked before the curl and again after a successful one, since a neighbour's 200 is indistinguishable from ours. A collision now fails in about a second instead of waiting 90 and then answering confidently wrong.mktemp, matching the pidfile beside it, and every failure branch now names the qualified path — including the pre-existing Playwright branch, which used to point readers at a file another run may have truncated.curl-not-ssrationale moved into the helper it belongs to rather than being lost.The failure path is deliberately a refusal: the alternative to failing is dumping whatever else answers on that port, which is the bug.
Pinning it
packages/spec/scripts/gen-sdui-manifest-collision.test.tssources the script (it returns right after defining its helpers, so no generation runs) and drives the real functions. No vite, no console build — the contract under test is the shell script's. The test picks its own ports through the script's own helper, so it cannot collide with a concurrent agent, which would be a poor look in this PR of all PRs.Two vacuity guards carry as much weight as the assertions, per the lesson the cleanup test beside it paid for:
NEIGHBOUR_BODYproves the neighbour really was answering at the exact URL spelling the script probes, andOUR_LEADER_ALIVEproves our stand-in really was gone. Without them a green "refused" could mean nothing was listening and nothing was checked.Ablations, each run against the final commit, each turning a different assertion red:
--strictPortfrom the spawned argvexpected 'pnpm --filter …' to contain '--strictPort'expected 'yes' to be 'no'(the original defect, reproduced under test)/tmplog pathexpected '1' to be '0'expected '5180' not to be '5180'The third one is reported only after a correction: its first mutation regex did not match, and the run that "passed" was an ablation that never applied. That is an invalid ablation, not a green one, so it was rewritten and re-run.
Verification
All at
9d06e3c90, the final commit; worktree clean at the time of each run.pnpm --filter @objectstack/spec test— 411 files / 10943 tests passedpnpm --filter @objectstack/spec typecheck— green, includingcheck:test-typecheck(the new file compiles; debt unchanged at 55 files / 263 errors)node scripts/pm/dispatch-gates.mjs, which named families the dispatch did not because adding a test file moves gates the script path alone does not —check:nul-bytes,check:merge-driver,check:type-source-resolution,check:engine-double-contract,check:where-matcher,check:query-options-erasure,check-affected-docs, and speccheck:empty-state/check:liveness/check:strictness-ledger/check:variant-docs— all greenpnpm --filter '@objectstack/spec^...' buildmatched no projects: spec has no workspace dependencies, so there is no closure to build. Stated rather than left silent, since a zero-match filter exits 0 and reads like a pass.check:dev-prereqs,check:type-check-coverage,check:type-check-debt. All three need the whole workspace built; herecheck:dev-prereqsexits 1 with "The workspace is not built — 1 unmet precondition, not a list of problems", reporting 67/67 packages missingdist/. That is the precondition, not a finding about this diff. CI runs them.One line in the suite output,
✗ the dev server this run started is no longer running., is the new test's own refusal leg printing the script's real diagnostic on the path where it is supposed to refuse. Expected output, not a failure.H4 sweep — the other concurrency-unsafe script of this shape
scripts/swept for fixed ports and unqualified/tmppaths used as per-run state. One other hit, filed as #9647 and not touched here:scripts/publish-smoke.shdefaultsSMOKE_PORTto 3210, derivesBASE_URLfrom it, and runsobjectstack dev --port "$SMOKE_PORT" --fresh.packages/cli/src/commands/serve.tsauto-shifts off a busy port wheneverflags.devis set — the exact path that script takes — so a concurrent run can smoke-test the neighbour's app. It is better off in two ways (mktemp'd work dir and log; an env override on the port) and I did not measure it end to end, which is why it is a card and not a rider on this PR.Everything else is clean:
downstream-smoke.shandrelease-spec-changes.shalreadymktemptheir per-run state, and the two remaining/tmphits (check-type-check-coverage.mjs,collect-release-notes.sh) are documentation examples rather than state.No changeset — flagged, because the dispatch asked for one
The dispatch ruling said to ship a
.changeset/*.md. I have not, and this is the one place I depart from it. This PR changes repo tooling and adds a test; it releases nothing.scripts/check-empty-changeset.mjsrejects newly added empty-frontmatter changesets in this repo, so the only way to satisfy the ruling literally is a non-empty changeset naming a package — which would cut a real@objectstack/specrelease whose CHANGELOG entry describes a maintainer script that is not in the package. The direct precedent is #9580, which touched these same two files, shipped no changeset, and carriedskip-changeset. Same treatment here, label applied.Out of scope and untouched: #9647 remains open.
Generated by Claude Code