Skip to content

Commit ed7d59c

Browse files
fix(cli): the #12525 read-back's anti-vacuity pins read CODE, and bind the argument rather than the argument list (#18519)
Part of #17633 Clause-②: no The nightly `e2e` tier was red on `main` in `packages/cli/test/serve-port-readback.e2e.test.ts`. Reproduced first, at the base this branch was cut from, with the tier switch the nightly uses. Two assertions failed, and **neither was about the product** — both source-text pins in the file's `ANTI-VACUITY` describe had decayed while the invariants they claim still hold. ## What was red, and why `vitest run --project integration test/serve-port-readback.e2e.test.ts` under `OS_TEST_TIERS=nightly`, at `8cf527f8e`: ``` AssertionError: the `API:` row is no longer printed BEFORE the banner tail — keying the read-back on the tail no longer proves the row is in the buffer: expected 47343 to be less than 38075 AssertionError: the ready banner no longer derives its API row from `resolveAuthBaseUrl(boundPort)` — if it now uses the REQUESTED port, the #12525 read-back is vacuous by construction: expected '// Copyright (c) 2025 ObjectStack. Li…' to contain 'externalBaseOrigin: resolveAuthBaseUr…' Test Files 1 failed (1) Tests 2 failed | 11 passed (13) ``` Both pins read the **raw** source file, and a raw read is unsound in both directions. **1. The print-order pin fabricated.** It ran `indexOf` over the whole of `src/utils/format.ts`. `ed5a1e7f2` (#17892) added a docblock quoting `Press Ctrl+C to stop` at line 800 — 281 lines **above** the `console.error` at line 1081 that prints it. So the raw read put the tail at offset 38075 and the `API:` row at 47343, and the assertion reported a print order that had never changed. Read in code position the two offsets are 47343 and 53551, i.e. line 984 before line 1081, exactly as the pin claims. **2. The banner pin held a byte-exact call that a legitimate refactor moved.** `89a652b83` (#17725, `dev --cert/--key`) made the call `resolveAuthBaseUrl(boundPort, boundProtocol)`. The port argument is unchanged and still the bound one; only the argument list grew. That commit updated the **per-PR** sibling pin, `packages/cli/src/commands/serve-bound-port-publication.test.ts`, in the same diff — and could not update this copy, because this file runs only on the nightly `main` sweep and no pull request can redden it. ## The fix Both pins now read through `scripts/js-comment-mask.mjs`, this tree's one answer to code-versus-prose, which blanks comment spans in place so every offset and line number survives. That repairs the fabrication and closes the other direction at the same time: a comment naming a pinned spelling can no longer satisfy a pin with no code behind it, which is exactly the vacuum the describe exists to prevent. The mask carries its own control — blanked in place (`code.length === raw.length`) and blanked something (`code !== raw`). The banner pin now binds the **argument** rather than the argument **list**: the row is derived from `boundPort`, whatever else the call grows. That is the division of labour the two tiers imply — the per-PR sibling keeps the byte-exact line and reddens on the PR that moves it, with that PR's author reading the failure; a nightly-only copy of the same byte-exact claim can only go red a day later on a card nobody can attribute. **That this is narrower, not looser, is proven rather than asserted** — inside the test, on synthetic text, so the proof survives any future rewording of `serve.ts`: ```ts expect('externalBaseOrigin: resolveAuthBaseUrl(port).baseOrigin').not.toMatch(BANNER_FROM_BOUND_PORT); expect('externalBaseOrigin: resolveAuthBaseUrl(requestedPort, boundProtocol).baseOrigin') .not.toMatch(BANNER_FROM_BOUND_PORT); ``` …beside a standing negative asserting neither requested-port spelling is in `serve.ts` at all. ⛔ No test renamed, skipped, quarantined or deleted, and no assertion dropped. The describe gains four assertions and loses none. ## Verification Everything below was run in a dedicated worktree at `63d19926e`, with the dependency closure built first. **Red, then green, same command, both with the tier env set.** The red leg restores the base copy of the test file onto the built tree, so the only variable is this diff: | tree | result | |:---|:---| | test file at `8cf527f8e` (base) | `Tests 2 failed \| 11 passed (13)`, the two assertions quoted above | | this branch | `Tests 13 passed (13)` — both real `os serve` boots included, 17.36s | Restore proven by blob equality (`d00e065c02c1b76bf42b641abed34b094cddf4c1` back on disk) and an empty `git diff HEAD` with an empty `git status --porcelain`. **Ablation — two legs, each mutating product code on disk to prove the repaired pin still binds.** No product code is changed by this PR; these mutations were made, measured and reverted. | leg | mutation | on-disk proof | pin | |:---|:---|:---|:---| | A | `resolveAuthBaseUrl(boundPort, boundProtocol)` becomes `resolveAuthBaseUrl(port, boundProtocol)` in `serve.ts` | bound-spelling 1 to 0, requested-spelling 0 to 1; blob `803b955f` to `a2f0aef3` | RED, naming the requested port | | B | the `API:` row print in `format.ts` renamed so it no longer prints `API:` | api-row-prints 1 to 0; blob `e3dda5c8` to `9ac9622f` | RED: `printServerReady no longer prints an API: row` | Both restored: blob equal to the `HEAD` blob, `git diff HEAD` empty, `git status --porcelain` empty. **Gate families.** `node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack` from the actual changed path, reconciled with `--ran`: **49 derived / 49 run / 0 NOT-MEASURED / 0 UNRUN**. - 47 green. - `pnpm check:dual-build-cjs-loads` exited **3** (`PREREQUISITE NOT MET`, packages with no `dist/`) on the first pass — recorded as neither pass nor fail, then re-run after a full `turbo run build` and **green**: 104 published require entry points across 67 packages load, 620 emitted CommonJS files parse. - `pnpm check:cross-package-test-inputs` exits **1**, rooted in `packages/cli/test/init-created-files-summary.e2e.test.ts` — pre-existing wherever `packages/spec/dist` is built, already filed as #18353 / #18348, and not a path this diff touches. It does not name this PR's file (0 occurrences in its output). **Beyond the derivation** (the standing blind spot — the derivation never names the lint family): - `pnpm lint` — repo-wide, `eslint . --no-inline-config`, **exit 0** at `63d19926e`. - `pnpm --filter @objectstack/cli typecheck` — exit 0, test layer included (`tsconfig.test.json`, ledger unchanged at 3 files / 28 errors / 6 pinned signatures). - `pnpm --filter @objectstack/cli exec vitest run --project unit` — `Test Files 211 passed (211)`, `Tests 3003 passed (3003)`. That tier carries `test/vitest-tiers-partition.test.ts`, which is what would notice if the new import had moved this file between tiers. It did not. ## Changeset `skip-changeset`, graded from what actually moves rather than from the shape of the diff. `packages/cli` declares `files: ["dist", "README.md", "CHANGELOG.md"]`; `npm pack --dry-run --json` ships **533 files, 0 of them under `test/`** and 0 matching `serve-port-readback`. The one changed path is not in any published tarball, so no released package moves. ## Acceptance notes - **This PR carries no closing keyword for #17633, deliberately.** The nightly workflow files nothing, edits nothing and closes nothing on green, so closure is a later reading of a green nightly and not something a merge can assert. Closing on the repair rather than on the confirmation would throw the card away along with its `os-nightly-tiers` marker if the next sweep is red for any other reason. - The card's `priority:p1` and everything in its comment thread were graded fresh and not inherited: the thread discusses older, already-repaired defects in other files, and the body — rewritten in place by the nightly's `report` job — names only this file. - Observation, not filed: the two source pins repaired here are near-duplicates of pins in `packages/cli/src/commands/serve-bound-port-publication.test.ts`, which runs per-PR. A byte-exact source pin that lives **only** in a nightly-only tier cannot be kept honest by the PR that moves the source — that is the mechanism behind this red, and this file is unlikely to be the only place it holds. Whether such pins belong in the nightly tier at all is a wider question than this card. --- _Generated by [Claude Code](https://claude.ai/code/session_01DvvamiacK328idtBYJBxV3)_ Co-authored-by: Claude <noreply@anthropic.com>
1 parent 9e6c6cf commit ed7d59c

1 file changed

Lines changed: 87 additions & 4 deletions

File tree

packages/cli/test/serve-port-readback.e2e.test.ts

Lines changed: 87 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,13 @@ import { tmpdir } from 'node:os';
5252
import { join, resolve } from 'node:path';
5353
import { fileURLToPath } from 'node:url';
5454

55+
// The tree's ONE answer to "is this span code, or prose" — the same import
56+
// `test/vitest-tiers-partition.test.ts` and `src/commands/
57+
// serve-bound-port-publication.test.ts` use. The ANTI-VACUITY describe below
58+
// is the only consumer here; see its header for why a raw read is unsound in
59+
// BOTH directions.
60+
import { maskComments } from '../../../scripts/js-comment-mask.mjs';
61+
5562
import {
5663
boundPortFromBanner,
5764
holdPort,
@@ -198,10 +205,45 @@ describe('#12525: the child\'s REAL port is read back out of its own banner', ()
198205
// above stays green while `boundPortFromBanner()` returns `no-banner`
199206
// forever — which is a SILENT SKIP on every real boot, i.e. the exact false
200207
// green this card is about, restored with nothing red to show for it.
201-
const formatSource = () => readFileSync(resolve(HERE, '../src/utils/format.ts'), 'utf8');
208+
//
209+
// ## ⛔ EVERY read in this describe is of CODE, never of the raw file
210+
//
211+
// `scripts/js-comment-mask.mjs` blanks comment spans in place — spaces for
212+
// comment bytes, newlines kept — so every offset and line number survives
213+
// the mask and an `indexOf` still points at the line it names.
214+
//
215+
// A raw read is unsound in BOTH directions, and this file has now been bitten
216+
// by each of them on a nightly that no pull request could have reddened:
217+
//
218+
// - it FABRICATES. `format.ts` gained a docblock quoting
219+
// `Press Ctrl+C to stop` 281 lines ABOVE the `console.error` that prints
220+
// it, so a whole-file `indexOf` put the tail before the `API:` row and
221+
// the order assertion reported a print order that had never changed.
222+
// - it VANISHES a pin. A comment naming the exact spelling a `toContain`
223+
// looks for satisfies that pin with no code behind it — the vacuum this
224+
// whole describe exists to prevent, reintroduced by its own instrument.
225+
//
226+
// Reading the raw bytes too is deliberate: the mask's own anti-vacuity
227+
// control needs something to compare against.
228+
const sourceOf = (relPath: string): { raw: string; code: string } => {
229+
const raw = readFileSync(resolve(HERE, relPath), 'utf8');
230+
return { raw, code: maskComments(raw) };
231+
};
232+
233+
/**
234+
* The mask ran, blanked in place, and blanked SOMETHING. Without it every
235+
* negative below would pass against an empty string and every positive
236+
* would fail for a reason that is not about the source under test.
237+
*/
238+
const expectMasked = ({ raw, code }: { raw: string; code: string }): void => {
239+
expect(code.length, 'the mask changed the file length — offsets no longer line up').toBe(raw.length);
240+
expect(code, 'the mask returned the file unchanged — it blanked no comment at all').not.toBe(raw);
241+
};
202242

203243
it('printServerReady still prints an `API:` row and still ends with the tail', () => {
204-
const source = formatSource();
244+
const format = sourceOf('../src/utils/format.ts');
245+
expectMasked(format);
246+
const source = format.code;
205247
const apiAt = source.indexOf('API:');
206248
const tailAt = source.indexOf('Press Ctrl+C to stop');
207249

@@ -238,13 +280,54 @@ describe('#12525: the child\'s REAL port is read back out of its own banner', ()
238280
// number that was REQUESTED and it stays 0 under `--port 0` (#13062).
239281
// The banner now reads the transport's own answer, so the premise below
240282
// is the stronger one it was always meant to be.
241-
const serveSource = readFileSync(resolve(HERE, '../src/commands/serve.ts'), 'utf8');
283+
//
284+
// ## ⭐ The ARGUMENT is the invariant; the argument LIST is not
285+
//
286+
// This same claim is pinned twice in this package, on purpose and at two
287+
// different strengths, because the two copies run in different tiers:
288+
//
289+
// - `src/commands/serve-bound-port-publication.test.ts` runs per-PR
290+
// (`queue`) and holds the BYTE-EXACT call, `boundProtocol` argument
291+
// and all. It reddens on the pull request that rewords the line, with
292+
// that PR's author reading the failure.
293+
// - this file runs ONLY on the nightly `main` sweep, so an exact-spelling
294+
// pin here cannot be kept honest by the PR that moves the spelling: it
295+
// goes red a day later, on a card nobody can attribute. That is not a
296+
// hypothetical — it is what happened, and what this test is being
297+
// repaired from. So this copy binds the SEMANTIC premise the read-back
298+
// rests on and nothing more: whatever else the call grows, the port it
299+
// is handed is the BOUND one.
300+
//
301+
// ⛔ Binding less is not binding loosely — the matcher still fails on every
302+
// spelling that would make this file vacuous, and that discrimination is
303+
// PROVEN below on synthetic text rather than asserted, so the proof holds
304+
// whatever `serve.ts` goes on to say.
305+
const BANNER_FROM_BOUND_PORT = /externalBaseOrigin:\s*resolveAuthBaseUrl\(\s*boundPort\s*[,)]/;
306+
const BANNER_FROM_REQUESTED_PORT = /externalBaseOrigin:\s*resolveAuthBaseUrl\(\s*(?:port|requestedPort)\s*[,)]/;
307+
308+
expect('externalBaseOrigin: resolveAuthBaseUrl(boundPort).baseOrigin').toMatch(BANNER_FROM_BOUND_PORT);
309+
expect('externalBaseOrigin: resolveAuthBaseUrl(boundPort, boundProtocol).baseOrigin')
310+
.toMatch(BANNER_FROM_BOUND_PORT);
311+
// ⛔ …and the regression it exists to catch does NOT satisfy it, in either
312+
// of the two names the requested port goes by in `run()`.
313+
expect('externalBaseOrigin: resolveAuthBaseUrl(port).baseOrigin').not.toMatch(BANNER_FROM_BOUND_PORT);
314+
expect('externalBaseOrigin: resolveAuthBaseUrl(requestedPort, boundProtocol).baseOrigin')
315+
.not.toMatch(BANNER_FROM_BOUND_PORT);
316+
317+
const serve = sourceOf('../src/commands/serve.ts');
318+
expectMasked(serve);
319+
const serveSource = serve.code;
242320
expect(serveSource).toContain('port = await getAvailablePort(requestedPort)');
243321
expect(
244322
serveSource,
245323
'the ready banner no longer derives its API row from `resolveAuthBaseUrl(boundPort)` — '
246324
+ 'if it now uses the REQUESTED port, the #12525 read-back is vacuous by construction',
247-
).toContain('externalBaseOrigin: resolveAuthBaseUrl(boundPort).baseOrigin');
325+
).toMatch(BANNER_FROM_BOUND_PORT);
326+
expect(
327+
serveSource,
328+
'the banner row is built from the REQUESTED port — #12525 read-back is vacuous: '
329+
+ 'the harness can never disagree with a banner derived from what it asked for',
330+
).not.toMatch(BANNER_FROM_REQUESTED_PORT);
248331
expect(
249332
serveSource,
250333
'the bound port is no longer resolved off the transport — `boundPort` is what the '

0 commit comments

Comments
 (0)