From ebd9be7da6a58501d6c615e83746925cb260559f Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 22 Aug 2026 03:25:16 +0000 Subject: [PATCH] test(cli): mask comments before scanning serve.ts in two contract/pin scans (#10514) Two `packages/cli` scans read `serve.ts` raw, with no comment/code separator at all, so prose counted as code -- the FABRICATES direction `scripts/js-comment-mask.mjs`'s own header calls the worse one. Neither scan was wrong today, but both were one ordinary comment away from being wrong: - `serve-email-config-parity.contract.test.ts`'s `keysReadFromConfigEmail()` matched `cfgEmail\.\w+` over raw source, so a comment naming an undeclared key would have fabricated a false "undeclared key" red, and -- the worse direction -- a comment naming a declared-but-unread key would have silently restored the exact `DECLARED_BUT_UNREAD` exemption this file's own docblock says was deleted for good after #5447/#5470. - `serve-multi-node-cap-advisory.pin.test.ts`'s four shape assertions matched regexes over raw source, so a reverted call could hide behind a trailing comment quoting the shape it replaced. Both now scan `maskComments(SERVE_SOURCE)` instead. `interfaceFields()`'s own narrower, brace-matched strip is untouched -- out of scope per the issue. Re-verified per the card's release condition: on origin/main at branch point (047ac86ee9), the raw / naive-stripped / masked key sets for subject 1 and the raw / masked match verdicts for subject 2's four assertions are all identical -- no verdict moved by #10453/#10919/#10948/#10956's changes to serve.ts since the card was written. `CROSS_PACKAGE_TEST_INPUTS['@objectstack/cli']` and the turbo.json `@objectstack/cli#test` input already carry `scripts/js-comment-mask.mjs`/`.d.mts` (landed with #10453/PR #10513), so no registration work was needed -- just the two-site routing. Adds vacuity-proof tests demonstrating the FABRICATES shapes on synthetic sources: a comment containing the exact pattern each scan keys on is shown producing the wrong verdict when read raw, and the right one once masked. Fresh answer to the open question #10909's dev never returned: yes, editing comment prose in serve.ts could move both scans' results pre-fix (demonstrated above); post-fix it cannot. Side effect caught by `check:cross-package-test-inputs`: adding the `maskComments` import gave `serve-email-config-parity.contract.test.ts` its first real escaping read, which newly exposed a PRE-EXISTING prose mention of `packages/spec/src/system/email-config.test.ts` (a doc cross-reference, never actually read) to the gate's literal collector. Reworded to name it without a single quoted repo-relative literal, following the precedent already set in this directory's `serve-multi-node-cap-advisory.pin.test.ts` for the identical gate. Fixes #10514 --- ...serve-email-config-parity.contract.test.ts | 77 ++++++++++++++++++- .../serve-multi-node-cap-advisory.pin.test.ts | 57 +++++++++++++- 2 files changed, 126 insertions(+), 8 deletions(-) diff --git a/packages/cli/src/commands/serve-email-config-parity.contract.test.ts b/packages/cli/src/commands/serve-email-config-parity.contract.test.ts index 8a0fbe72f1..00e21d122a 100644 --- a/packages/cli/src/commands/serve-email-config-parity.contract.test.ts +++ b/packages/cli/src/commands/serve-email-config-parity.contract.test.ts @@ -29,8 +29,12 @@ // This package's `tsconfig.json` includes `src` (tests and all), so the // compile-time witness below is real: `pnpm --filter @objectstack/cli // typecheck` fails on a config the schema cannot express, before any test runs. -// The spec-side companion (`packages/spec/src/system/email-config.test.ts`) has -// to be runtime-only — that package excludes `**/*.test.ts` from its tsconfig +// The spec-side companion — `@objectstack/spec`'s own `system/email-config.test.ts`, +// named without its repo-relative path here for the reason this directory's +// `serve-multi-node-cap-advisory.pin.test.ts` gives for the same gate: a quoted +// literal starting at `packages/` would force `check:cross-package-test-inputs` +// to demand a glob for a file this test never actually reads — has to be +// runtime-only, since that package excludes `**/*.test.ts` from its tsconfig // (#5286). import { describe, it, expect } from 'vitest'; @@ -40,12 +44,26 @@ import { fileURLToPath } from 'node:url'; import { EmailServiceConfigSchema } from '@objectstack/spec/system'; import type { EmailServiceConfig } from '@objectstack/spec/system'; import { resolveEmailCapabilityArg } from './serve.js'; +// The repo's one comment/code separator (#9367). This file used to scan RAW +// source with no separator at all (#10514): a docblock or a `// TODO: also +// read cfgEmail.foo` line was indistinguishable from a real dot access. Typed +// by the hand-written `scripts/js-comment-mask.d.mts` next to it (this +// package's `tsconfig.json` includes `src`), so this import needs no +// suppression. +import { maskComments } from '../../../../scripts/js-comment-mask.mjs'; const SERVE_SOURCE = readFileSync( path.resolve(path.dirname(fileURLToPath(import.meta.url)), 'serve.ts'), 'utf8', ); +/** + * `serve.ts` with every comment span blanked (offsets preserved, bytes + * replaced with spaces) — see `keysReadFromConfigEmail` below for why the + * scan reads this instead of `SERVE_SOURCE` directly (#10514). + */ +const MASKED_SERVE_SOURCE = maskComments(SERVE_SOURCE); + /** * Every `config.email` key the resolver reads, straight from its source — the * issue's own repro command: @@ -57,9 +75,14 @@ const SERVE_SOURCE = readFileSync( * keys), which is what makes a source scan an exact measure rather than an * approximation. Should that ever stop being true, this comment is the place * the next reader learns the scan has to change with it. + * + * Takes `source` explicitly (default the real file's masked text, #10514) + * rather than closing over `SERVE_SOURCE`/`MASKED_SERVE_SOURCE` directly, so + * the vacuity-proof tests below can drive the exact same regex over a raw vs. + * a masked variant of a shape and show the two disagree. */ -function keysReadFromConfigEmail(): string[] { - const reads = SERVE_SOURCE.match(/cfgEmail\.[A-Za-z_$][\w$]*/g) ?? []; +function keysReadFromConfigEmail(source: string = MASKED_SERVE_SOURCE): string[] { + const reads = source.match(/cfgEmail\.[A-Za-z_$][\w$]*/g) ?? []; return [...new Set(reads.map((r) => r.slice('cfgEmail.'.length)))].sort(); } @@ -127,6 +150,52 @@ describe('EmailServiceConfigSchema ↔ resolveEmailCapabilityArg', () => { }); }); +/** + * Vacuity proof (#10514): both directions the raw scan was one ordinary + * comment away from getting wrong, reproduced on synthetic sources shaped + * like the real resolver so the two legs (raw vs. masked) can be compared + * without waiting for `serve.ts` to actually regress. Each `it` shows the RAW + * leg producing the wrong verdict — the verdict this file's scan would have + * produced before #10514 — and the MASKED leg producing the right one. + */ +describe('the key scan ignores prose that looks like a cfgEmail read (#10514)', () => { + it('does not let a comment fabricate an undeclared key ("declares every key" direction)', () => { + const synthetic = [ + 'function resolveEmailCapabilityArg(cfgEmail = {}) {', + ' // TODO: someday also read cfgEmail.bogusKey from the config', + " const provider = cfgEmail.provider;", + ' return { provider };', + '}', + ].join('\n'); + + // Pre-#10514 (raw): the comment's `cfgEmail.bogusKey` is indistinguishable + // from a real dot access — this is what would have made "declares every + // config.email key the resolver reads" go RED over a comment alone. + expect(keysReadFromConfigEmail(synthetic)).toEqual(['bogusKey', 'provider']); + // Post-#10514 (masked): the comment is blanked, so only the real read + // survives. + expect(keysReadFromConfigEmail(maskComments(synthetic))).toEqual(['provider']); + }); + + it('does not let a comment fabricate a read of a declared-but-unread key — the #5447 shape, "the worse one" ("reads every key" direction)', () => { + const synthetic = [ + 'function resolveEmailCapabilityArg(cfgEmail = {}) {', + " const provider = cfgEmail.provider;", + ' // historically we also honoured cfgEmail.persist here', + ' return { provider };', + '}', + ].join('\n'); + + // Pre-#10514 (raw): the comment alone counts as a "read" of `persist` — + // silently restoring the exact `DECLARED_BUT_UNREAD` exemption this + // file's docblock (above) says was deleted for good after #5447/#5470. + expect(keysReadFromConfigEmail(synthetic)).toContain('persist'); + // Post-#10514 (masked): the comment does not count, so a schema key with + // no real reader still reads as unread here. + expect(keysReadFromConfigEmail(maskComments(synthetic))).not.toContain('persist'); + }); +}); + /** * Compile-time half — the author-facing symptom #5307 is written about. Every * value here is one the runtime has honoured since #5160; before the diff --git a/packages/cli/src/commands/serve-multi-node-cap-advisory.pin.test.ts b/packages/cli/src/commands/serve-multi-node-cap-advisory.pin.test.ts index 29b14ab6a2..b935e0bd9a 100644 --- a/packages/cli/src/commands/serve-multi-node-cap-advisory.pin.test.ts +++ b/packages/cli/src/commands/serve-multi-node-cap-advisory.pin.test.ts @@ -34,6 +34,15 @@ import { describe, it, expect } from 'vitest'; import { readFileSync } from 'node:fs'; import { dirname, join, resolve } from 'node:path'; import { fileURLToPath } from 'node:url'; +// The repo's one comment/code separator (#9367). The four shape assertions +// below used to match against RAW `SERVE_SOURCE` (#10514): a trailing comment +// describing the old call shape (e.g. quoting a reverted +// `checkMultiNodeAllowed(replicas)`) was indistinguishable from the real call. +// `interfaceFields()` further down does its own narrower, brace-matched strip +// over an `export interface` body and is deliberately left alone — out of +// scope for #10514, noted there so a future re-derivation doesn't read it as +// the same defect. +import { maskComments } from '../../../../scripts/js-comment-mask.mjs'; const HERE = dirname(fileURLToPath(import.meta.url)); @@ -43,6 +52,15 @@ const REPO_ROOT = resolve(HERE, '../../../..'); /** `packages/cli/src/commands/serve.ts` — the consumer. */ const SERVE_SOURCE = readFileSync(resolve(HERE, 'serve.ts'), 'utf8'); +/** + * `SERVE_SOURCE` with every comment span blanked (offsets preserved) — what + * the four shape assertions below actually match against (#10514), so a + * comment naming `checkMultiNodeAllowed(…)` cannot satisfy — or hide behind — + * any of them. `interfaceFields()` still reads raw `SERVE_SOURCE`; see the + * import comment above for why that is out of scope here. + */ +const MASKED_SERVE_SOURCE = maskComments(SERVE_SOURCE); + /** * The producer, read from source rather than imported: the CLI has no * dependency on this package (that is the whole reason the cast exists), and @@ -115,8 +133,8 @@ describe('os serve ↔ multi-node gate', () => { it('calls the gate WITH a requested node count', () => { // The exact regression: `checkMultiNodeAllowed()`. Passing nothing makes the // licensed-overflow verdict unreachable rather than merely unread. - expect(SERVE_SOURCE).not.toMatch(/checkMultiNodeAllowed\(\s*\)/); - expect(SERVE_SOURCE).toMatch(/checkMultiNodeAllowed\(\s*[^)\s]/); + expect(MASKED_SERVE_SOURCE).not.toMatch(/checkMultiNodeAllowed\(\s*\)/); + expect(MASKED_SERVE_SOURCE).toMatch(/checkMultiNodeAllowed\(\s*[^)\s]/); }); it('passes the operator-declared replica count', () => { @@ -124,11 +142,11 @@ describe('os serve ↔ multi-node gate', () => { // desired count, identical in every replica, not a live membership count — // which is right for an advisory message about the operator's own // configuration, and is NOT sufficient for enforcement. - expect(SERVE_SOURCE).toMatch(/checkMultiNodeAllowed\(\s*Number\(process\.env\.OS_CLUSTER_REPLICAS\)\s*\)/); + expect(MASKED_SERVE_SOURCE).toMatch(/checkMultiNodeAllowed\(\s*Number\(process\.env\.OS_CLUSTER_REPLICAS\)\s*\)/); }); it('types the dynamic import with the mirrored verdict, not an inline literal', () => { - expect(SERVE_SOURCE).toMatch( + expect(MASKED_SERVE_SOURCE).toMatch( /checkMultiNodeAllowed:\s*\(requested\?:\s*number\)\s*=>\s*MultiNodeGateVerdict/, ); }); @@ -152,3 +170,34 @@ describe('os serve ↔ multi-node gate', () => { ).toEqual(producer); }); }); + +/** + * Vacuity proof (#10514): a synthetic regression shaped exactly like the + * issue's own repro — the zero-arg call reintroduced, with a trailing comment + * quoting the OLD argued call, the way a careless revert reads. Both legs are + * shown so the RAW leg's wrong verdict — what this pin's assertions would + * have produced before #10514 — is visible next to the MASKED leg's correct + * one, not just asserted. + */ +describe('the shape assertions ignore a comment that quotes the old call (#10514)', () => { + it('a reverted zero-arg call cannot hide behind a comment describing the argued call it replaced', () => { + const regressed = [ + 'const __gate = checkMultiNodeAllowed();', + '// checkMultiNodeAllowed(Number(process.env.OS_CLUSTER_REPLICAS)) used to be called here', + ].join('\n'); + + // Pre-#10514 (raw): the negative assertion correctly catches the bad + // shape… + expect(regressed).toMatch(/checkMultiNodeAllowed\(\s*\)/); + // …but the positive assertion is ALSO satisfied — by the comment alone — + // which is exactly how this pin's "calls the gate WITH a requested node + // count" test would have stayed green over the regression it exists to + // catch. + expect(regressed).toMatch(/checkMultiNodeAllowed\(\s*[^)\s]/); + + // Post-#10514 (masked): the comment is blanked, so the positive assertion + // correctly fails to find an argued call — the regression is no longer + // hidden. + expect(maskComments(regressed)).not.toMatch(/checkMultiNodeAllowed\(\s*[^)\s]/); + }); +});