Skip to content

Commit 2e1842b

Browse files
kai392jak-glitchcursoragent
authored
fix(checks): require real import statements in checkers-wired home scan (#10119)
Prose/comment mentions of a sibling check-*.ts no longer count as imported homes, so dropping a checker from test:ci fails instead of being masked by a sentence (#10048). Co-authored-by: kai392 <chengjunkai4@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com>
1 parent 117f750 commit 2e1842b

2 files changed

Lines changed: 67 additions & 9 deletions

File tree

scripts/check-checkers-wired.ts

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -109,6 +109,22 @@ export type CheckerHome =
109109
| { kind: "allowed"; via: string }
110110
| { kind: "none" };
111111

112+
/** Drop line (`//`) and block comments so a prose mention of a checker is not treated as an import (#10048). */
113+
function stripScriptComments(source: string): string {
114+
return source.replace(/\/\*[\s\S]*?\*\//g, "").replace(/\/\/.*$/gm, "");
115+
}
116+
117+
/**
118+
* True when `source` has a real import, re-export, or dynamic-import of `./base` (optional .ts or .js).
119+
* Specifier must sit in statement position — a bare substring or comment mention does not count (#10048).
120+
*/
121+
function sourceImportsChecker(source: string, base: string): boolean {
122+
const code = stripScriptComments(source);
123+
const escaped = base.replace(/[.*+?^${}()|[\]\\]/g, "\\$&");
124+
const specifier = `\\./${escaped}(?:\\.(?:ts|js))?`;
125+
return new RegExp(`(?:(?:import|export)\\s[^;]*?\\sfrom\\s*|import\\s*\\(\\s*)['"]${specifier}['"]`).test(code);
126+
}
127+
112128
/** Where a checker actually runs, or `none`. Pure, so every branch is testable without a filesystem. */
113129
export function resolveCheckerHome(input: {
114130
file: string;
@@ -126,10 +142,10 @@ export function resolveCheckerHome(input: {
126142
const workflowRef = invokers.find((name) => input.workflowText.includes(name)) ?? (input.workflowText.includes(input.file) ? input.file : undefined);
127143
if (workflowRef !== undefined) return { kind: "workflow", via: workflowRef };
128144

129-
// Imported by a sibling script => a shared module, not an entry point. Matched on the extensionless
130-
// specifier because a TS import may or may not carry `.ts`/`.js`.
145+
// Imported by a sibling script => a shared module, not an entry point. Require an actual import /
146+
// re-export / dynamic-import statement after stripping comments — a prose mention must not count (#10048).
131147
const base = input.file.replace(/\.ts$/, "");
132-
if (input.otherScriptSources.some((source) => source.includes(`${base}.ts`) || source.includes(`${base}.js`) || source.includes(`./${base}"`) || source.includes(`./${base}'`))) {
148+
if (input.otherScriptSources.some((source) => sourceImportsChecker(source, base))) {
133149
return { kind: "imported", via: base };
134150
}
135151

test/unit/check-checkers-wired.test.ts

Lines changed: 48 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,13 @@ describe("reachableNpmScripts", () => {
5050
describe("resolveCheckerHome", () => {
5151
const base = { scripts: {}, reachableFromTestCi: new Set<string>(), workflowText: "", otherScriptSources: [], allowed: {} };
5252

53+
// Assembled at runtime: `check-import-specifiers.ts` greps this repo's own sources for module
54+
// specifiers and cannot tell a fixture string from a real import, so a literal here fails that
55+
// sibling checker. The value under test is identical either way.
56+
const fromSpecifier = (name: string, suffix = "") => `import { helper } from "./${name}${suffix}";\n`;
57+
const exportFromSpecifier = (name: string, suffix = "") => `export { helper } from "./${name}${suffix}";\n`;
58+
const dynamicImportSpecifier = (name: string, suffix = "") => `await import("./${name}${suffix}");\n`;
59+
5360
it("finds a checker wired into the local gate", () => {
5461
const home = resolveCheckerHome({
5562
...base,
@@ -69,12 +76,47 @@ describe("resolveCheckerHome", () => {
6976
});
7077

7178
it("treats a checker imported by a sibling script as a shared module, not an entry point", () => {
72-
// Assembled at runtime rather than written as a literal: `check-import-specifiers.ts` greps this repo's
73-
// own sources for module specifiers and cannot tell a fixture string from a real import, so a literal
74-
// here fails that sibling checker. The value under test is identical either way.
75-
const importLine = `import { x } from "./${"check-foo-core"}.ts";`;
76-
const home = resolveCheckerHome({ ...base, file: "check-foo-core.ts", otherScriptSources: [importLine] });
77-
expect(home.kind).toBe("imported");
79+
const home = resolveCheckerHome({ ...base, file: "check-foo-core.ts", otherScriptSources: [fromSpecifier("check-foo-core")] });
80+
expect(home).toEqual({ kind: "imported", via: "check-foo-core" });
81+
});
82+
83+
it("matches import specifiers with .js and .ts suffixes, re-exports, and dynamic import (#10048)", () => {
84+
expect(resolveCheckerHome({ ...base, file: "check-foo.ts", otherScriptSources: [fromSpecifier("check-foo", ".js")] })).toEqual({
85+
kind: "imported",
86+
via: "check-foo",
87+
});
88+
expect(resolveCheckerHome({ ...base, file: "check-foo.ts", otherScriptSources: [fromSpecifier("check-foo", ".ts")] })).toEqual({
89+
kind: "imported",
90+
via: "check-foo",
91+
});
92+
expect(resolveCheckerHome({ ...base, file: "check-foo.ts", otherScriptSources: [exportFromSpecifier("check-foo")] })).toEqual({
93+
kind: "imported",
94+
via: "check-foo",
95+
});
96+
expect(resolveCheckerHome({ ...base, file: "check-foo.ts", otherScriptSources: [dynamicImportSpecifier("check-foo")] })).toEqual({
97+
kind: "imported",
98+
via: "check-foo",
99+
});
100+
});
101+
102+
it("does not treat a // comment mention as an imported home (#10048)", () => {
103+
expect(resolveCheckerHome({ ...base, file: "check-foo.ts", otherScriptSources: ["// see check-foo.ts for the pattern\n"] })).toEqual({ kind: "none" });
104+
});
105+
106+
it("does not treat a /* */ block comment mention as an imported home (#10048)", () => {
107+
expect(resolveCheckerHome({ ...base, file: "check-foo.ts", otherScriptSources: ["/* check-foo.ts is the sibling */\n"] })).toEqual({ kind: "none" });
108+
});
109+
110+
it("does not treat a near-miss specifier as an imported home (#10048)", () => {
111+
// `./check-foobar` must not mark `check-foo.ts` as imported — the old substring scan would.
112+
expect(resolveCheckerHome({ ...base, file: "check-foo.ts", otherScriptSources: [fromSpecifier("check-foobar")] })).toEqual({ kind: "none" });
113+
});
114+
115+
it("does not treat the check-fixture-clock-races prose mention as an imported home (#10048)", () => {
116+
// Live sentence from scripts/check-fixture-clock-races.ts:95 — the bug that motivated the tighter matcher.
117+
const prose =
118+
" // STRING LITERALS, exactly as check-turbo-typecheck-inputs.ts's own fixtures do. Those are not real\n";
119+
expect(resolveCheckerHome({ ...base, file: "check-turbo-typecheck-inputs.ts", otherScriptSources: [prose] })).toEqual({ kind: "none" });
78120
});
79121

80122
it("accepts an explicit allowlist entry, carrying its reason", () => {

0 commit comments

Comments
 (0)