Skip to content

Commit 904c0af

Browse files
yinlianghuiclaude
andauthored
fix(guards): route the route-ledger host-app-reach limb through the shared comment mask (#12474)
The host-app-reach limb in three route-ledger conformance guards scanned RAW source, so a COMMENT quoting `getRawApp`, `'http-server'` or `'http.server'` scored as an extra host-app reacher and failed an IDENTITY assertion by naming a file that reaches for nothing. Every sibling limb in those same files already stripped comments first, and each file's own header says why. The limb now runs through `reachesHostApp()`, which strips comments and leaves string, template and regex literals INTACT — two of the three spellings are service keys, which are string literals, so masking literals here would have turned the false positive into a silent disarm. While converting: the three files answered "comment or code?" with their own character scanners. They now import `stripComments` from `scripts/js-comment-mask.mjs`, the tree's one answer, and their rows in `check-comment-mask-adoption.mjs` are deleted in this PR — the half that gate's `stale` branch exists to demand (23 rows -> 20, shrink-only). That swap was measured, and it found live code loss on two of the three: the private scanners read the `//` inside a regex literal as a line-comment opener and deleted real code to end of line — 40 bytes in `packages/metadata/src/ plugin.ts` and 38 bytes in `packages/cloud-connection/src/ marketplace-proxy-plugin.ts`, the latter inside a declared MOUNT SOURCE. The imports are declared in `scripts/cross-package-test-inputs.mjs` and hashed by `turbo.json` for both new consumers. Claude-Session: https://claude.ai/code/session_01UjM2ia8Av1v5NqfqQEQmC6 Co-authored-by: Claude <noreply@anthropic.com>
1 parent 4635f3e commit 904c0af

6 files changed

Lines changed: 316 additions & 157 deletions

File tree

packages/cloud-connection/src/cloud-connection-route-ledger.conformance.test.ts

Lines changed: 85 additions & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,13 @@ import { readdirSync, readFileSync } from 'node:fs';
5353
import { dirname, join } from 'node:path';
5454
import { fileURLToPath } from 'node:url';
5555
import { describe, it, expect } from 'vitest';
56+
// The repo's ONE answer to "is this span a comment, or code?" — its header
57+
// carries the two private-stripper families that drifted apart and the
58+
// parser-differential sweep that measured which way each fails. This package
59+
// already imports the module next door in `canonical-expression-envelopes.test.ts`.
60+
// `stripComments` (not `maskComments`) is the projection this file wants: every
61+
// finding here reports a `file:line` or a bare file name, never an offset.
62+
import { stripComments } from '../../../scripts/js-comment-mask.mjs';
5663
import { CLOUD_CONNECTION_ROUTE_LEDGER } from './cloud-connection-route-ledger.js';
5764

5865
/**
@@ -121,63 +128,28 @@ const DECLARED_COMPUTED_MOUNTS = [
121128
// ---------------------------------------------------------------------------
122129

123130
/**
124-
* Strip comments before scanning. Prose cannot mount a route, and this
125-
* package's headers quote every wire path they serve — so a raw-text scan would
126-
* report a documented path as an unledgered mount, a false red on an accurate
127-
* package. The three string forms are tracked so a literal CONTAINING comment
128-
* punctuation (`'/api/v1/x/*'`, `'https://host'`) is never mistaken for a
129-
* comment opener; `comment-stripper` below pins both directions, because a
130-
* stripper that swallowed real code would make this scan silently blind, which
131-
* is the failure that actually matters here.
131+
* WHY COMMENTS ARE REMOVED BEFORE ANY SCAN HERE. Prose cannot mount a route and
132+
* cannot reach for a host app, and this package's headers quote every wire path
133+
* they serve — a raw-text scan reports a documented path as an unledgered mount
134+
* and a documented `getRawApp()` as a second reacher: a false red on an
135+
* accurate package.
136+
*
137+
* This file used to answer that question with its own character scanner. It was
138+
* converted to `scripts/js-comment-mask.mjs` (#12398), the tree's one answer to
139+
* it, and the swap was MEASURED rather than assumed: over this package's 13
140+
* scanned source files the two differ on exactly one,
141+
* `marketplace-proxy-plugin.ts`, where the private scanner read the `//` inside
142+
* the regex literal `/\/packages\/[^/]+\/versions\//` as a line-comment opener
143+
* and deleted the 38 characters of REAL CODE that followed it to end of line —
144+
* inside a declared MOUNT SOURCE, which is source this census reads. That is
145+
* the naive-`//` family the shared module's header measures, live here.
146+
*
147+
* `stripComments` keeps line numbers (block-comment newlines survive — this
148+
* package's headers run to eighty lines and every finding quotes `file:line`)
149+
* and keeps string, template and regex literals INTACT, which is what lets the
150+
* census resolve wire paths out of them at all. `scan machinery` at the foot of
151+
* this file pins both directions.
132152
*/
133-
export function stripComments(source: string): string {
134-
let out = '';
135-
let i = 0;
136-
while (i < source.length) {
137-
const c = source[i];
138-
const next = source[i + 1];
139-
if (c === '/' && next === '/') {
140-
while (i < source.length && source[i] !== '\n') i++;
141-
continue;
142-
}
143-
if (c === '/' && next === '*') {
144-
i += 2;
145-
// Newlines inside the block are PRESERVED. Line numbers are what
146-
// every finding below points a reader at, and this package's
147-
// headers run to eighty lines — swallowing them would send someone
148-
// to a line eighty short of the mount, which reads as a wrong
149-
// report rather than as the accurate one it is.
150-
while (i < source.length && !(source[i] === '*' && source[i + 1] === '/')) {
151-
if (source[i] === '\n') out += '\n';
152-
i++;
153-
}
154-
i += 2;
155-
continue;
156-
}
157-
if (c === '\'' || c === '"' || c === '`') {
158-
const quote = c;
159-
out += c;
160-
i++;
161-
while (i < source.length) {
162-
if (source[i] === '\\') {
163-
out += source.slice(i, i + 2);
164-
i += 2;
165-
continue;
166-
}
167-
out += source[i];
168-
if (source[i] === quote) {
169-
i++;
170-
break;
171-
}
172-
i++;
173-
}
174-
continue;
175-
}
176-
out += c;
177-
i++;
178-
}
179-
return out;
180-
}
181153

182154
/** Module-scope `const NAME = '<literal>';` bindings, for resolving mount paths. */
183155
export function constantBindings(code: string): Map<string, string> {
@@ -293,6 +265,30 @@ function packageSourceFiles(): string[] {
293265
/** The spellings by which a module in this package reaches the HOST app. */
294266
const HOST_APP_REACH = /getRawApp|['"`]http-server['"`]|['"`]http\.server['"`]/;
295267

268+
/**
269+
* Does `source` reach for the host app IN CODE?
270+
*
271+
* COMMENTS ARE REMOVED FIRST, and that half is the whole of #12398. A docblock
272+
* explaining why a mount sits where it does — "the mount takes the
273+
* framework-native handle through `IHttpServer.getRawApp()`" — is prose, and
274+
* prose reaches for nothing. Scanned raw it scored as an extra reacher and
275+
* failed an IDENTITY assertion by naming a file that reaches for nothing, whose
276+
* own failure text then invites the wrong repair: widening the expected list,
277+
* which retires the only property the assertion has.
278+
*
279+
* STRING, TEMPLATE AND REGEX LITERALS ARE LEFT INTACT, and that half is what
280+
* keeps the fix from being a silent disarm. Two of the three spellings above
281+
* ARE string literals — `ctx.getService('http.server')` reaches for the host
282+
* app entirely inside quotes — so a probe that masked literals as well as
283+
* comments would detect nothing and this identity would pass vacuously. Both
284+
* directions are pinned in `scan machinery` at the foot of this file.
285+
*/
286+
const reachesHostApp = (source: string): boolean => HOST_APP_REACH.test(stripComments(source));
287+
288+
/** Which of `files` reach for the host app in code. Driveable for the pins. */
289+
const filesReachingHostApp = (files: readonly string[], read: (f: string) => string): string[] =>
290+
files.filter((f) => reachesHostApp(read(f)));
291+
296292
const ledgerRoutes = (): Set<string> => new Set(CLOUD_CONNECTION_ROUTE_LEDGER.map((e) => e.route));
297293
const liveCensus = (): Census => censusOf(MOUNT_SOURCES, readSource);
298294

@@ -369,7 +365,7 @@ describe('cloud-connection mount population', () => {
369365
// An IDENTITY, not a count: the day a FIFTH module in this package
370366
// resolves `http-server` or calls `getRawApp()`, this names it — and
371367
// the census above, which only reads MOUNT_SOURCES, would not have.
372-
const reaching = packageSourceFiles().filter((f) => HOST_APP_REACH.test(readSource(f)));
368+
const reaching = filesReachingHostApp(packageSourceFiles(), readSource);
373369
expect(
374370
reaching,
375371
'files reaching for the host HTTP app. A registrar not listed in MOUNT_SOURCES is invisible '
@@ -475,6 +471,38 @@ describe('scan machinery, pinned in both directions', () => {
475471
expect(census.routes[0].line).toBe(4);
476472
});
477473

474+
it('the host-app reach probe does not count PROSE — the #12398 false positive', () => {
475+
// The exact docblock shape that fired it: a module explaining that the
476+
// mount takes the framework-native handle, in a comment.
477+
expect(reachesHostApp('// the mount takes the handle through `IHttpServer.getRawApp()`\n')).toBe(false);
478+
expect(reachesHostApp("/*\n * resolves 'http-server' before mounting\n */\n")).toBe(false);
479+
expect(reachesHostApp("/* the ctx.getService('http.server') seam, explained */\n")).toBe(false);
480+
});
481+
482+
it('the host-app reach probe still counts a REACH THAT LIVES IN A STRING', () => {
483+
// The direction that makes the fix a fix rather than a disarm: two of
484+
// the three spellings are service keys, which are string literals.
485+
expect(reachesHostApp("const s = ctx.getService('http.server');\n")).toBe(true);
486+
expect(reachesHostApp('const s = ctx.getService("http-server");\n')).toBe(true);
487+
expect(reachesHostApp('const s = ctx.getService(`http-server`);\n')).toBe(true);
488+
expect(reachesHostApp('const app = server.getRawApp();\n')).toBe(true);
489+
});
490+
491+
it('a genuine FIFTH reacher is still named — anti-vacuity on the identity limb', () => {
492+
// LOAD-BEARING POSITIVE for #12398's fix, driven through the same
493+
// function the live limb calls with source injected. Without it, a
494+
// strip that quietly stopped matching anything would leave the identity
495+
// green forever — the failure direction the whole family distrusts.
496+
const fake: Record<string, string> = {
497+
'cloud-connection-plugin.ts': 'const app = http.getRawApp();\n',
498+
'prose-only.ts': '// getRawApp() is reached in the four mount sources, never here\n',
499+
'zzz-fifth-reacher.ts': "const s = ctx.getService('http.server');\n",
500+
};
501+
expect(
502+
filesReachingHostApp(Object.keys(fake).sort(), (f) => fake[f]),
503+
).toEqual(['cloud-connection-plugin.ts', 'zzz-fifth-reacher.ts']);
504+
});
505+
478506
it('resolves the three argument spellings this package actually uses', () => {
479507
const constants = new Map([['ROUTE_BASE', '/api/v1/marketplace/install-local'], ['P', '/api/v1/cloud-connection']]);
480508
// bare identifier — marketplace-install-local-plugin.ts

packages/metadata/src/metadata-route-ledger.conformance.test.ts

Lines changed: 86 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,14 @@ import { readdirSync, readFileSync, statSync } from 'node:fs';
3131
import { dirname, join, relative, sep } from 'node:path';
3232
import { fileURLToPath } from 'node:url';
3333
import { describe, it, expect } from 'vitest';
34+
// The repo's ONE answer to "is this span a comment, or code?" — see its header
35+
// for the two private-stripper families that drifted apart and why neither was
36+
// safe. `stripComments` (not `maskComments`) is the projection this file wants:
37+
// every finding here reports a `file:line` or a bare file name, never an
38+
// offset, and the module's own guidance is to pick by what the caller reports.
39+
// The `.mjs` specifier is deliberate; `scripts/js-comment-mask.d.mts` beside it
40+
// is a hand-written declaration, so this import needs no `allowJs`.
41+
import { stripComments } from '../../../scripts/js-comment-mask.mjs';
3442
import { METADATA_ROUTE_LEDGER } from './metadata-route-ledger.js';
3543

3644
/**
@@ -60,49 +68,26 @@ const NON_ROUTE_MEMBERS = new Set(['use', 'notFound', 'onError', 'fire', 'fetch'
6068
// ---------------------------------------------------------------------------
6169

6270
/**
63-
* Strip comments before scanning. Prose cannot mount a route, and this
64-
* package's headers quote the wire paths they serve — a raw-text scan would
65-
* report a documented path as an unledgered mount. Newlines inside block
66-
* comments are PRESERVED so every finding's `file:line` points at the real
67-
* line; `hmr-routes.ts` opens with a 27-line header, so swallowing them would
68-
* report the mount 27 lines short of where it is.
71+
* WHY COMMENTS ARE REMOVED BEFORE ANY SCAN HERE. Prose cannot mount a route and
72+
* cannot reach for a host app, and this package's headers quote the wire paths
73+
* and the handles they serve — a raw-text scan reports a documented path as an
74+
* unledgered mount, and a documented `getRawApp()` as a second reacher.
75+
*
76+
* This file used to answer that question with its own character scanner. It was
77+
* converted to `scripts/js-comment-mask.mjs` (#12398), which is the tree's one
78+
* answer to it, and the swap was MEASURED rather than assumed: over this
79+
* package's 29 scanned source files the two differ on exactly one, `plugin.ts`,
80+
* where the private scanner read the `//` inside the regex literal
81+
* `/^https?:\/\//i` as a line-comment opener and deleted the 40 characters of
82+
* REAL CODE that followed it to end of line. That is the naive-`//` family the
83+
* shared module's header measures, live in the very file this guard's identity
84+
* limb pins.
85+
*
86+
* `stripComments` keeps line numbers (block-comment newlines survive) and keeps
87+
* string, template and regex literals INTACT — both properties are load-bearing
88+
* below: `hmr-routes.ts` opens with a 27-line header, and the host-app reach
89+
* this file detects is partly a SERVICE KEY, which is a string literal.
6990
*/
70-
export function stripComments(source: string): string {
71-
let out = '';
72-
let i = 0;
73-
while (i < source.length) {
74-
const c = source[i];
75-
const next = source[i + 1];
76-
if (c === '/' && next === '/') {
77-
while (i < source.length && source[i] !== '\n') i++;
78-
continue;
79-
}
80-
if (c === '/' && next === '*') {
81-
i += 2;
82-
while (i < source.length && !(source[i] === '*' && source[i + 1] === '/')) {
83-
if (source[i] === '\n') out += '\n';
84-
i++;
85-
}
86-
i += 2;
87-
continue;
88-
}
89-
if (c === '\'' || c === '"' || c === '`') {
90-
const quote = c;
91-
out += c;
92-
i++;
93-
while (i < source.length) {
94-
if (source[i] === '\\') { out += source.slice(i, i + 2); i += 2; continue; }
95-
out += source[i];
96-
if (source[i] === quote) { i++; break; }
97-
i++;
98-
}
99-
continue;
100-
}
101-
out += c;
102-
i++;
103-
}
104-
return out;
105-
}
10691

10792
/**
10893
* Blank out string CONTENTS, preserving quotes, length and newlines.
@@ -258,6 +243,30 @@ function packageSourceFiles(dir = SRC_DIR): string[] {
258243
/** The spellings by which a module reaches the HOST app. */
259244
const HOST_APP_REACH = /getRawApp|['"`]http-server['"`]|['"`]http\.server['"`]/;
260245

246+
/**
247+
* Does `source` reach for the host app IN CODE?
248+
*
249+
* COMMENTS ARE REMOVED FIRST, and that half is the whole of #12398. A docblock
250+
* explaining why a mount sits outside the auth seam — "the mount takes the
251+
* framework-native handle through `IHttpServer.getRawApp()`" — is prose, and
252+
* prose reaches for nothing. Scanned raw it scored as a second reacher and
253+
* failed an IDENTITY assertion by naming a file that reaches for nothing, whose
254+
* own failure text then invites the wrong repair: widening the expected list,
255+
* which retires the only property the assertion has.
256+
*
257+
* STRING, TEMPLATE AND REGEX LITERALS ARE LEFT INTACT, and that half is what
258+
* keeps the fix from being a silent disarm. Two of the three spellings above
259+
* ARE string literals — `ctx.getService('http.server')` reaches for the host
260+
* app entirely inside quotes — so the sibling `maskStrings` below must never be
261+
* applied here. Both directions are pinned in `scan machinery` at the foot of
262+
* this file, including a genuine second reacher that lives in a string.
263+
*/
264+
const reachesHostApp = (source: string): boolean => HOST_APP_REACH.test(stripComments(source));
265+
266+
/** Which of `files` reach for the host app in code. Driveable for the pins. */
267+
const filesReachingHostApp = (files: readonly string[], read: (f: string) => string): string[] =>
268+
files.filter((f) => reachesHostApp(read(f)));
269+
261270
/** A mount-shaped call on a handle named `app` — how a SECOND registrar would look. */
262271
const MOUNT_SHAPED = /\bapp\s*\.\s*(?:get|post|put|patch|delete|options|head|all)\s*\(/;
263272

@@ -315,7 +324,7 @@ describe('metadata mount population', () => {
315324
it('plugin.ts is the only file that reaches for the host app', () => {
316325
// An IDENTITY, not a count: the day a second module resolves
317326
// `http-server` or calls `getRawApp()`, this names it.
318-
const reaching = packageSourceFiles().filter((f) => HOST_APP_REACH.test(readSource(f)));
327+
const reaching = filesReachingHostApp(packageSourceFiles(), readSource);
319328
expect(
320329
reaching,
321330
'files reaching for the host HTTP app. A second registrar is invisible to the census above — '
@@ -442,6 +451,40 @@ describe('scan machinery, pinned in both directions', () => {
442451
expect(masked).toContain('http.getRawApp()');
443452
});
444453

454+
it('the host-app reach probe does not count PROSE — the #12398 false positive', () => {
455+
// The exact docblock that fired it: a module explaining that the mount
456+
// takes the framework-native handle, in a comment.
457+
expect(reachesHostApp('// the mount takes the handle through `IHttpServer.getRawApp()`\n')).toBe(false);
458+
expect(reachesHostApp("/*\n * resolves 'http-server' before mounting\n */\n")).toBe(false);
459+
expect(reachesHostApp("/* the ctx.getService('http.server') seam, explained */\n")).toBe(false);
460+
});
461+
462+
it('the host-app reach probe still counts a REACH THAT LIVES IN A STRING', () => {
463+
// The direction that makes the fix a fix rather than a disarm. Two of
464+
// the three spellings are service keys — string literals — so a probe
465+
// that masked literals as well as comments would detect nothing here
466+
// and the identity limb would pass vacuously forever.
467+
expect(reachesHostApp("const s = ctx.getService('http.server');\n")).toBe(true);
468+
expect(reachesHostApp('const s = ctx.getService("http-server");\n')).toBe(true);
469+
expect(reachesHostApp('const s = ctx.getService(`http-server`);\n')).toBe(true);
470+
expect(reachesHostApp('const app = server.getRawApp();\n')).toBe(true);
471+
});
472+
473+
it('a genuine SECOND reacher is still named — anti-vacuity on the identity limb', () => {
474+
// LOAD-BEARING POSITIVE for #12398's fix: driven through the same
475+
// function the live limb calls, with source injected. Without it, a
476+
// strip that quietly stopped matching anything would leave the identity
477+
// green forever — the failure direction the whole family distrusts.
478+
const fake: Record<string, string> = {
479+
'plugin.ts': 'const app = http.getRawApp();\n',
480+
'routes/prose-only.ts': '// getRawApp() is reached in plugin.ts, never here\n',
481+
'routes/second-reacher.ts': "const s = ctx.getService('http.server');\n",
482+
};
483+
expect(
484+
filesReachingHostApp(Object.keys(fake).sort(), (f) => fake[f]),
485+
).toEqual(['plugin.ts', 'routes/second-reacher.ts']);
486+
});
487+
445488
it('resolves both binding spellings this package uses, and refuses the rest', () => {
446489
const b = pathBindings("const routePath = options.path ?? '/api/v1/dev/metadata-events';\nconst FIXED = '/api/v1/fixed';\n");
447490
expect(b.get('routePath')).toBe('/api/v1/dev/metadata-events');

0 commit comments

Comments
 (0)