Skip to content

test(cli): put the serve↔bootStack parity scan on the shared comment masker - #10513

Merged
os-elon merged 4 commits into
mainfrom
claude/issue-10453-cli-contract-tests-comment-strip
Aug 21, 2026
Merged

test(cli): put the serve↔bootStack parity scan on the shared comment masker#10513
os-elon merged 4 commits into
mainfrom
claude/issue-10453-cli-contract-tests-comment-strip

Conversation

@os-elon

@os-elonos-elon commented Aug 21, 2026

Copy link
Copy Markdown
Collaborator

Part of #10453

The card names twopackages/cli contract tests as carrying #9367's naive comment strip. Re-derived at head (5c3faa70d) by searching for the strip pattern rather than trusting the paths: only one of them does. So this PR converts the one that does, and reports the other half as a dead premise rather than inventing work for it. Part of, not Fixes, for that reason — the card's second half is a PM call, not mine.

What is true at head

card's claimmeasured at 5c3faa70d
serve-verify-security-parity.contract.test.ts defines the naive stripCommentsholds — converted here
serve-email-config-parity.contract.test.ts defines the samefalse — it has no comment handling at all, and git log -S stripComments on that path returns nothing: it never had one

The email scan reads serve.tsraw and matches /cfgEmail\.[A-Za-z_$][\w$]*/g over it, comments included. That is a real hazard, in the FABRICATES direction the masker's own header calls the worse one — but it is a different defect from the one this card decided, its fix is not this card's fix, and the card's reasoning for including it was based on a strip that is not there. Filed separately rather than ridden in here.

The conversion

serve.ts is #9367's textbook subject. Its 5d. header names the route wildcard /api/v1/auth/*; that opener starts a phantom block comment which closes against the webpackIgnore pragma ten lines below, deleting the hasAuthPlugin computation (2136) and the if (!hasAuthPlugin && tierEnabled('auth')) gate (2139) in between.

stripserve.ts linescode-bearing lines
naive, block pass first (what this file carried)4590 → 34001895
shared maskComments4590 → 45902141

1. Does the swap change either test's verdict today? — No, and that is reported as such

All four assertions this file makes produce identical results under both implementations, on both subjects:

serve.ts naive constructions : ["appSecurityPluginOptions(config)"]
serve.ts masker constructions: ["appSecurityPluginOptions(config)"]
harness.ts naive constructions : ["appSecurityPluginOptions(config)"]
harness.ts masker constructions: ["appSecurityPluginOptions(config)"]
appDefaultPermissionSetName present: false / false (both, both files)
/fallbackPermissionSet\s*:/ matches: false / false (both, both files)

Every measured construction already sat outside the swallowed span. This removes a latent divergence; it fixes no live defect, and is not claimed to. The ablation below re-measures the same fact from the other side: with the separator reverted, the three parity assertions stay green while only the new pin goes red.

2. Reverse verification — the inputs the two implementations disagree on

Four specimens, each an ordinary shape carrying a comment opener that is not a comment. The naive strip's phantom span deletes the construction between the opener and the next terminator; the masker resolves all four. Pinned in the file:

specimennaive stripmasker
a route wildcard in a line comment (serve.ts's own shape)[]['appSecurityPluginOptions(config)']
a route wildcard in a string literal[]['appSecurityPluginOptions(config)']
a comment opener in a template literal[]['appSecurityPluginOptions(config)']
a comment opener in a template nested inside an interpolation[]['appSecurityPluginOptions(config)']

Note the direction: not a mangled argument, but a boot path that reads as constructing nothing. Two such paths satisfy "the two agree with each other" vacuously — the parity claim passing over no evidence.

Offsets too: maskComments blanks rather than deletes, so length and line count survive (185 → 185 on the first specimen). The naive strip collapses that specimen to 0 bytes. The paren-walking securityPluginConstructions slices by offset and names one in its own error text, so this is load-bearing, not cosmetic.

The pin exercises the binding the scan uses, not maskComments directly

First draft of this pin called maskComments itself — which would have stayed green through the exact edit it exists to catch (someone restoring a private strip in this file). The masking step is now a named separateCodeFromProse, used by both BOOT_PATHS and the pin. Same shape of hole as the mention-is-not-a-call ablation this file already records.

Ablation, at the committed state, restored byte-identically

Prediction recorded before running: 5 red (4 specimen + the offset pin), the 4 naive-strip legs green, and the 3 original parity assertions green. Observed exactly that:

Tests 5 failed | 11 passed (16)
→ expected [] to deeply equal [ 'appSecurityPluginOptions(config)' ] (x4)
→ expected '' to have a length of 185 but got +0

Restored with git checkout HEAD -- packages/cli/src/commands/serve-verify-security-parity.contract.test.ts; sha256 back to 8b5482d8969a202f87e6384daccf3c074616d90b68826643aadc8586dc7d9b42, git status clean. No dist/ sits between the mutation and the run — vitest loads this test file from source and the masker by relative path — so no rebuild leg applies, and the observed red is the proof the mutation reached the run.

3. #10427 cross-check — ruled out by a stronger instrument than the card asked for

The card suggests ruling out the open scanSource desync by checking masker and naive strip agree on every anchor count. They do, but that is a weak check, and serve.tsdoes carry the trigger family (2 nested-template shapes, 3 escaped backticks). So both subjects were compared byte-for-byte against @typescript-eslint/parser's comment ranges instead:

packages/cli/src/commands/serve.ts: comment bytes read as CODE (FABRICATES) = 0; code bytes read as COMMENT (BLINDS) = 0
packages/verify/src/harness.ts: comment bytes read as CODE (FABRICATES) = 0; code bytes read as COMMENT (BLINDS) = 0

Neither subject is in #10427's population. The boundary is recorded in the file: the case the masker does not get right is a nested template carrying an escaped backtick — measured here too, confirming #10427's reproducer — and that is that issue's to close, not this file's.

Cross-package declaration — added here, because #10450 has not landed

The maskComments import escapes the package. PR #10450 adds exactly this declaration, but it is still open, so this PR carries its own: scripts/js-comment-mask.mjs in CROSS_PACKAGE_TEST_INPUTS['@objectstack/cli'] plus the matching $TURBO_ROOT$ input on @objectstack/cli#test. The array lines are byte-identical to #10450's so whichever lands second resolves to one line. ⚠️check:cross-package-test-inputs did not demand it — its literal collector cannot see an escaping relative import specifier (#10452) — so it is declared by hand and the roster comment says so.

Verification — at final head 80024aa31

Exit codes captured before any pipe; each verdict line is the gate's own text.

Gate set re-derived after the final commit with node scripts/pm/dispatch-gates.mjs (no path arguments — it takes the change set from the merge base itself): 4 path-matched + 5 convention-triggered. Families it named that the dispatch brief did not: check:slot-lookup, check-affected-docs, check:query-options-erasure, check:type-check-coverage, check:type-check-debt, check:engine-double-contract, check:where-matcher.

  • pnpm --filter @objectstack/cli typecheck — echoed > @objectstack/cli@17.1.0 typecheck then > tsc --noEmit, no error TS (script name verified echoed, so not a zero-match silent pass). This is also what proves the @ts-expect-error on the untyped .mjs import is a real suppression and not a phantom — an unused directive is itself a tsc error.
  • pnpm --filter @objectstack/cli testTest Files 138 passed (138) · Tests 1539 passed (1539)
  • this file alone, verbose — Test Files 1 passed (1) · Tests 16 passed (16)
  • check:cross-package-test-inputsOK: 12 package(s) read outside themselves, all declared, and turbo.json hashes every declared glob. (and All 60 self-test cases passed.)
  • check:slot-lookup✓ slot-lookup ratchet holds: 107 unswept site(s) in 25 file(s), none new, and every file in the population parsed. · baseline key set verified against 5c3faa7: no files added.
  • check:query-options-erasure✓ query-options-erasure ratchet holds: 67 unswept non-test site(s) in 17 file(s), none new, and every file measured parsed.
  • check:engine-double-contractcheck-engine-double-contract: OK — 340 pinned, 133 in the DEBT ledger, 2 exempt.
  • check:where-matcher✓ where-matcher conformance holds: 266 matcher(s) discovered, 266 answer the combinator battery correctly or refuse it loudly (161 refuse).
  • check:type-check-coveragecheck-type-check-coverage: OK — 64/77 workspace packages type-checked (plus the root), 13 in the DEBT ledger
  • scripts/docs-audit/check-affected-docs.mjs — exit 0, ✓ affected-docs self-test: 262 cases pass.
  • check:nul-bytescheck-nul-bytes: OK (scanned 6143 text file(s) ... no raw ASCII control bytes). Plus a hand scan of the three edited files for control bytes: clean.

⚠️One declared narrowing.check:type-check-debt --re-measure was not run. It requires the whole workspace built (this worktree has cli's closure, 55 of 78) and re-measures every ledger entry; under the shared verify lock that is a large spend for a change that provably cannot move it. @objectstack/cli's ledger entry is in TEST_DEBT, and its own note says the 65 files it counts are the sibling test/ tree — "the other 57 test files sit under src and always compiled". The file edited here is under src, so it is read by the tsc --noEmit run above, which is clean. The structural half (check:type-check-coverage) is green, and no other ledger package is touched by this three-file diff. CI runs the family regardless.

Changeset

Not owed — this diff is one test file, one gate roster comment plus glob, and a turbo.json input. No published artifact and no runtime behaviour changes. skip-changeset applied.


Generated by Claude Code

…masker (#10453)
#9367 converted six `scripts/check-*.mjs` gates off a private naive
`stripComments` and onto the string-aware `maskComments`. This scan was
outside that population only because it is a test, and it still carried the
original spelling — block pass first, the order that swallows code.
`serve.ts` is the conversion's textbook subject: its `5d.` header names the
route wildcard `/api/v1/auth/*`, whose opener starts a phantom block comment
running to the `webpackIgnore` pragma ten lines below, deleting the
`hasAuthPlugin` computation and the auth gate in between. Measured at
`5c3faa70d`: the naive strip keeps 1895 code-bearing lines of `serve.ts`, the
masker keeps 2141.
No verdict moved. Every construction this scan measures already sat outside
the swallowed span, so the divergence was latent — which is exactly why it had
to be closed by hand rather than by a red build.
A new block pins the separator itself on four inputs where the two
implementations disagree, so reverting the swap is red rather than silent. The
masker's read of both real subjects was also checked byte-for-byte against
`@typescript-eslint/parser`'s comment ranges (0 bytes of disagreement either
way), which rules out #10427's open `scanSource` desync for this pair.
The import escapes the package, so it is declared in
`CROSS_PACKAGE_TEST_INPUTS` and hashed by `@objectstack/cli#test`. That gate
did not demand it — see #10452.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019bmVFqoQPq63zhKrxdYG1r
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

Nothing 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.

os-elonand others added 2 commits August 21, 2026 02:04
…real .d.mts
`scripts/js-comment-mask.d.mts` landed on main in 0681a76, after this branch
forked at 5c3faa7. It hand-declares `maskComments(source: string): string`,
so the import no longer errors and the suppression above it became TS2578
"Unused '@ts-expect-error' directive" — red on `typecheck-workspace`, and red
on `typecheck-debt` as a cascade, because the re-measure compares each ledger
entry's raw `tsc --noEmit` count against its recorded number and this pushed
`@objectstack/cli` above its own.
Neither tsc was wrong: the branch's tree had no declaration (directive
load-bearing, typecheck clean) and CI's merged tree has one. Reproduced
locally only after merging origin/main, which is the honest way to see it.
Not a silent deletion — the import is now genuinely typed rather than `any`.
Probed by passing `maskComments(12345)` into a `number`, which tsc rejects
with TS2345 (parameter is `string`) and TS2322 (return is `string`); the
probe was reverted byte-identically. The comment that justified the
suppression is replaced rather than left standing, since its claim that the
module has "no type declarations" is now false.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019bmVFqoQPq63zhKrxdYG1r
`check:cross-package-test-inputs` went red once the previous commit named
`scripts/js-comment-mask.d.mts` in the test's prose: the gate's literal
collector takes quoted paths without parsing, so a mention forces a
declaration. This roster already settles that trade for `check-nul-bytes.mjs`
— declare the file rather than reword a comment to dodge a scanner.
Here declaring is also the substantively right answer, not just the cheap one.
The `.d.mts` is what gives `maskComments` its type, so cli's `tsc --noEmit`
verdict is a function of that file — and this branch is the proof: the file
arriving on main is what turned an `@ts-expect-error` into a TS2578 and took
two typecheck lanes red without cli's own tests ever re-running.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019bmVFqoQPq63zhKrxdYG1r
@os-elonClaude

Copy link
Copy Markdown
CollaboratorAuthor

CI red — root-caused, reproduced, fixed at 51a3a986a

Neither tsc was wrong. They were reading different trees.

scripts/js-comment-mask.d.mts landed on main in 0681a76b8 ("feat(spec): gate the schema-free principle", #10398), after this branch forked at 5c3faa70d. It is a hand-written declaration exporting maskComments(source: string): string. So:

  • on this branch's tree the .mjs had no types → the import errored → the @ts-expect-error was load-bearing → tsc clean (what I measured, correctly, at the time);
  • on CI's merged tree the import resolves with types → the directive is unused → TS2578 (what CI measured, also correctly).

Reproduced before fixing

Merged origin/main into the branch, reinstalled, rebuilt the closure, then:

src/commands/serve-verify-security-parity.contract.test.ts(42,1): error TS2578: Unused '@ts-expect-error' directive.
Exit status 2

Byte-identical to the CI line, exit 2 captured before any pipe. Not a speculative fix.

The fix, and why it is not a silent deletion

The directive is gone because the module now has an authoritative declaration, not because CI complained. No @ts-ignore — that would silence the check in both directions.

The comment that justified the suppression claimed the module has "no type declarations". That is now false, so it was replaced rather than left standing.

Then I checked the import is genuinely typed rather than silently any — "clean" would be the answer either way, and any is the failure mode worth ruling out. Probe: const probe: number = maskComments(12345).

error TS2322: Type 'string' is not assignable to type 'number'. ← return is string
error TS2345: Argument of type 'number' is not assignable to parameter of type 'string'. ← param is string

Probe reverted byte-identically. Net effect: maskComments now arrives typed instead of any, so the source: string annotation on BOOT_PATHS is a real check.

typecheck-debt — cascading, not independent

Same root cause. The re-measure compares each ledger entry's raw tsc --noEmit count against its recorded number, and the gate's own output says why one new error was enough:

surplus: none — every entry sits exactly at its measurement, so any new error is red.

@objectstack/cli has zero slack, so the single TS2578 put it over. Green after the one-line fix, with that fix as the sole delta — measured on the same lane command, not assumed:

check-type-check-coverage --re-measure: OK — 33 ledger entr(ies) re-measured in 261.7s, 1924 raw tsc error(s) total, none above its recorded number.

A second, genuinely independent failure the merge surfaced

check:cross-package-test-inputs went red after the fix, for a different reason worth recording:

- @objectstack/cli names path(s) no declared glob covers:
scripts/js-comment-mask.d.mts (named in .../serve-verify-security-parity.contract.test.ts)

Naming the .d.mts in the replacement comment was enough — the collector takes quoted paths without parsing, so a mention forces a declaration. This roster already settles that trade for check-nul-bytes.mjs: declare the file, never reword prose to dodge a scanner. Declaring is also substantively right here — the .d.mts is what gives maskComments its type, so cli's typecheck verdict is a function of it, and this branch is the proof: that file arriving is what took two lanes red while cli's own tests never re-ran. Added to the roster and to @objectstack/cli#test in turbo.json.

Re-verification at 51a3a986a

Exit codes captured before any pipe; verdicts are each runner's own line. Gate set re-derived after the final commit (node scripts/pm/dispatch-gates.mjs, no args) against the new merge base dd41df30c — same 4 path-matched + 5 convention-triggered families as before.

  • pnpm --filter @objectstack/cli typecheckTC_EXIT=0, no error TS
  • pnpm --filter @objectstack/cli testTest Files 138 passed (138) · Tests 1539 passed (1539), TEST_EXIT=0
  • check:cross-package-test-inputsOK: 12 package(s) read outside themselves, all declared, and turbo.json hashes every declared glob.
  • check:type-check-debtDEBT_EXIT=0 (quoted above)
  • check:slot-lookupSLOT_EXIT=0, ratchet holds: 107 unswept site(s) in 25 file(s), none new
  • check:query-options-erasureQOE_EXIT=0, ratchet holds: 67 unswept non-test site(s) in 17 file(s), none new
  • check:type-check-coverageOK — 64/77 workspace packages type-checked
  • check:engine-double-contract — exit 0 · check:where-matcher — exit 0, baseline key set verified against dd41df3: no files added
  • check:nul-bytesOK (scanned 6163 text file(s) ... no raw ASCII control bytes)
  • check-affected-docs — exit 0

The declared narrowing from the first round is withdrawn: check:type-check-debt --re-measure was the lane in question and it has now been run in full, on the full package build CI uses.

Reverse verification re-run after the fix, as asked

Ablated separateCodeFromProse back to the naive strip at the committed state — the pin still fails for the right reason, identically to the first round:

Tests 5 failed | 11 passed (16)
→ expected [] to deeply equal [ 'appSecurityPluginOptions(config)' ] (x4)
→ expected '' to have a length of 185 but got +0

The three original parity assertions stayed green, so the latency claim still holds. Restored byte-identically — sha25678aeb4d5511cfe0185d6aa1b19e8c5dc4cf049a88b81e42f2c037f261396a29b, git status clean.

No expect edited, no gate weakened, no threshold moved, nothing skipped. PR stays draft.


Generated by Claude Code


Generated by Claude Code

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

Labels

size/mskip-changesetPR has no user-facing published change; bypasses the changeset gatetests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@os-elon