From 1aefc402ce2b5cc95632a05643d5c3c2d895b8af Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E7=A8=8B=E5=BA=8F=E5=91=98=E9=98=BF=E6=B1=9F=28Relakkes?= =?UTF-8?q?=29?= Date: Tue, 4 Aug 2026 20:42:45 +0800 Subject: [PATCH] build(policy): fix four lexer desyncs and extend the dead-import check to all of src MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The check landed scoped to src/server/ws because blanking desynced on 6 of 2149 files and a desync reports a live import as dead. All four causes are fixed, so the scope is now every source root no compiler checks — src, scripts and adapters — and 0 of 2541 files desync. The bugs, each with a regression test that places the import's only reference after the construct so a desync makes it go dead: - The token before a slash was read back out of the raw source, so the last word of a preceding comment decided whether `/` opened a regex. In useIssueFlagBanner.ts `// …correction tone` made the next line's regex lex as a division, and its apostrophe opened a string that ate the line. A comment is whitespace to the grammar; it now contributes nothing. - That token accumulated across whitespace, so `return false` became one token named `returnfalse` and the following `return /re/` no longer looked like a keyword. This is what broke markdownImages.ts and dead-imports.ts itself. - `input! / 10` divides but `!/re/.test(x)` negates, and both put `!` before the slash. What precedes the `!` settles it. - Character classes and quotes inside regular expressions, fixed earlier. desktop/ stays out of scope: its tsconfig already sets noUnusedLocals, and scanning it anyway finds nothing — the cross-check that this agrees with a real compiler. `blankingIsSound` still refuses to analyse a file whose blanked form no longer parses, so a future desync is reported, not acted on. Routing follows the scope: policyPrefixes now names adapters/, scripts/ and src/ instead of the three scripts/ subdirectories and src/server/ws/. The check reads these files rather than importing them, so the import graph cannot select the lane on its own. `does not widen docs, policy, or coverage lanes` split in two — its fixture selects the policy lane through its own files now, so the dependent-must-not-widen invariant moved to a desktop fixture that still shows it. Verified by mutation, each reverted from an explicit backup: planting `plantedProbeSymbol` into one file per root reported all four and failed check:policy; reverting each of the three lexer fixes failed exactly its own regression test; removing 'src/' from policyPrefixes failed the routing test and made change-policy report policy=false for a src-only diff. check:policy is 223 pass / 0 fail in 8.6s, up from 2s — the scan is 3.8s over 2150 files and the planted-import test covers every one of them. --- scripts/pr/change-policy.test.ts | 35 ++++++++--- scripts/pr/change-policy.ts | 15 ++--- scripts/pr/dead-imports.test.ts | 65 +++++++++++++++++++- scripts/pr/dead-imports.ts | 101 ++++++++++++++++++++++++------- 4 files changed, 175 insertions(+), 41 deletions(-) diff --git a/scripts/pr/change-policy.test.ts b/scripts/pr/change-policy.test.ts index 09ed85ef..d0dddab5 100644 --- a/scripts/pr/change-policy.test.ts +++ b/scripts/pr/change-policy.test.ts @@ -126,14 +126,19 @@ describe('evaluateChangePolicy', () => { expect(result.checks.persistence).toBe(true) }) - test('routes a ws-only change to the policy lane that owns its dead-import check', () => { - // scripts/pr/dead-imports.test.ts reads src/server/ws rather than importing it, - // so the import graph cannot pull the policy lane in. Without the prefix the + test('routes every source root the dead-import check owns to the policy lane', () => { + // scripts/pr/dead-imports.test.ts reads these roots rather than importing them, + // so the import graph cannot pull the policy lane in. Without the prefixes the // check exists and never runs on the diffs it was written for. - const result = evaluateChangePolicy(['src/server/ws/handler.ts']) - - expect(result.checks.server).toBe(true) - expect(result.checks.policy).toBe(true) + expect(evaluateChangePolicy(['src/server/ws/handler.ts']).checks.policy).toBe(true) + expect(evaluateChangePolicy(['src/utils/attachments.ts']).checks.policy).toBe(true) + expect(evaluateChangePolicy(['adapters/feishu/index.ts']).checks.policy).toBe(true) + expect(evaluateChangePolicy(['scripts/perf/local-index-benchmark.ts']).checks.policy).toBe(true) + // Surfaces still route to their own lanes; policy is additive, not a takeover. + expect(evaluateChangePolicy(['src/server/ws/handler.ts']).checks.server).toBe(true) + expect(evaluateChangePolicy(['adapters/feishu/index.ts']).checks.adapters).toBe(true) + // desktop/ is checked by its own tsconfig, so it must not select this lane. + expect(evaluateChangePolicy(['desktop/src/pages/Settings.tsx']).checks.policy).toBe(false) }) test('keeps quality ownership and contributor contracts on the policy lane', () => { @@ -341,7 +346,7 @@ describe('evaluateChangePolicy dependent-file widening', () => { expect(result.blocked).toBe(false) }) - test('does not widen docs, policy, or coverage lanes', () => { + test('does not widen docs or coverage lanes', () => { const result = evaluateChangePolicy( ['src/server/services/providerService.ts', 'src/server/__tests__/provider.test.ts'], [], @@ -349,11 +354,23 @@ describe('evaluateChangePolicy dependent-file widening', () => { ) expect(result.checks.docs).toBe(false) - expect(result.checks.policy).toBe(false) // Coverage still reflects the diff, which already contains executable sources. expect(result.checks.coverage).toBe(true) }) + test('does not widen the policy lane', () => { + // Split out of the case above once `src/` became a policy prefix: that fixture + // selects the lane through its own changed files now, so it can no longer show + // what this asserts — a dependent the import graph added must never select it. + const result = evaluateChangePolicy( + ['desktop/src/pages/Settings.tsx'], + [], + ['scripts/pr/check-pr.ts', 'src/utils/attachments.ts', 'adapters/feishu/index.ts'], + ) + + expect(result.checks.policy).toBe(false) + }) + test('selects the agent flow for protocol clients the import graph cannot reach', () => { // Regression for d14154379 -> 4626dbef4: adapters/common/http-client.ts hardcoded // permissionMode:'default' on POST /api/sessions, short-circuiting the server's diff --git a/scripts/pr/change-policy.ts b/scripts/pr/change-policy.ts index f5cbf3cf..ef079d57 100644 --- a/scripts/pr/change-policy.ts +++ b/scripts/pr/change-policy.ts @@ -154,13 +154,14 @@ const persistencePrefixes = [ const policyPrefixes = [ '.github/workflows/', - 'scripts/git-hooks/', - 'scripts/pr/', - 'scripts/quality-gate/', - // scripts/pr/dead-imports.ts owns this directory. The lane is selected by prefix, - // and that check reads its files rather than importing them, so the import graph - // cannot route a ws-only diff here on its own. - 'src/server/ws/', + // `scripts/pr/dead-imports.ts` owns these three source roots — every root no + // compiler checks for unreferenced imports. The lane is selected by prefix, and + // that check reads its files rather than importing them, so the import graph + // cannot route a diff here on its own. `scripts/` also covers the git hooks, the + // PR tooling and the quality gate, which selected this lane before. + 'adapters/', + 'scripts/', + 'src/', ] const policyExactPaths = new Set([ diff --git a/scripts/pr/dead-imports.test.ts b/scripts/pr/dead-imports.test.ts index 59fedeeb..11df9b65 100644 --- a/scripts/pr/dead-imports.test.ts +++ b/scripts/pr/dead-imports.test.ts @@ -198,6 +198,60 @@ describe('findDeadImports', () => { expect(findDeadImports(source)).toEqual([]) }) + // Each of the three below places the import's only reference *after* the tricky + // construct on the same line. A desync blanks to the end of that line, so the + // import goes dead — put the reference on a later line and the assertion holds + // even while the lexer is broken. + + it('does not let a comment decide how the next line lexes', () => { + // A comment is whitespace to the grammar. Reading the last word out of the raw + // source made `tone` the token before the slash, so the regex lexed as a + // division and its apostrophe opened a string that ate the rest of the line — + // src/hooks/useIssueFlagBanner.ts, verbatim. + // The comment has to sit between the last code token and the slash, which is + // where the array of patterns in that file puts it. + const source = [ + "import { match } from './x.js'", + 'export const value =', + ' // comma or exclamation implies correction tone', + " /\\bthat'?s (wrong|incorrect)\\b/i.test(match)", + '', + ].join('\n') + + expect(blankingIsSound(source)).toBe(true) + expect(findDeadImports(source)).toEqual([]) + }) + + it('does not run two keywords together into one token', () => { + // `return false` accumulated as `returnfalse`, so the next `return /re/` no + // longer looked like a keyword and its character class swallowed the line. + const source = [ + "import { probe } from './x.js'", + 'export function guard(value: string): boolean {', + ' if (!value) return false', + " return /[\\w$)\\]'\"`]/.test(probe(value))", + '}', + '', + ].join('\n') + + expect(blankingIsSound(source)).toBe(true) + expect(findDeadImports(source)).toEqual([]) + }) + + it('tells a non-null assertion apart from a negated regex test', () => { + // `estimates[i]! / total` divides; `!/re/.test(x)` negates. Both put `!` + // immediately before the slash. + const source = [ + "import { share, guard } from './x.js'", + 'export const ratio = (parts: number[], total: number) => parts[0]! / share(total)', + 'export const clean = (value: string) => !/[<>]/.test(guard(value))', + '', + ].join('\n') + + expect(blankingIsSound(source)).toBe(true) + expect(findDeadImports(source)).toEqual([]) + }) + it('does not mistake division for a regular expression', () => { // `(a) / 2 ... /` would swallow the code between two divisions. const source = [ @@ -227,11 +281,16 @@ describe('blankNonCode', () => { describe('dead imports in owned source', () => { const { dead, scannedFiles, degradedFiles } = scanDeadImports(ROOT) - it('scans the directory it claims to own', () => { + it('scans the directories it claims to own', () => { // An empty or mis-rooted scan passes every other assertion in this file. - expect(DEAD_IMPORT_ROOTS).toEqual(['src/server/ws']) + expect(DEAD_IMPORT_ROOTS).toEqual(['src', 'scripts', 'adapters']) expect(scannedFiles).toContain('src/server/ws/handler.ts') - expect(scannedFiles.length).toBeGreaterThanOrEqual(5) + expect(scannedFiles).toContain('scripts/pr/dead-imports.ts') + expect(scannedFiles).toContain('adapters/feishu/index.ts') + expect(scannedFiles.length).toBeGreaterThan(1_000) + // desktop/ is out of scope on purpose — its tsconfig already sets + // noUnusedLocals — so a root list that swept it in would be a mistake. + expect(scannedFiles.some((file) => file.startsWith('desktop/'))).toBe(false) }) it('analysed every file it scanned', () => { diff --git a/scripts/pr/dead-imports.ts b/scripts/pr/dead-imports.ts index d35896d2..a420d8dc 100644 --- a/scripts/pr/dead-imports.ts +++ b/scripts/pr/dead-imports.ts @@ -14,17 +14,24 @@ import { join, relative, sep } from 'node:path' * it adds 646 unused-symbol diagnostics to a baseline of 3225 that already fails, so * it would have to land disabled and would never go green. * - * This is deliberately narrower than `noUnusedLocals` — imports only, one directory — - * because that is the part that was actually unowned, and a check nobody can keep - * green gets switched off. + * This is deliberately narrower than `noUnusedLocals` — imports only — because that + * is the part that was actually unowned, and a check nobody can keep green gets + * switched off. It found 27 more dead imports than the handler.ts split left, all + * removed before it landed. * * The analysis is lexical, like `module-graph.ts`: an import whose binding appears * nowhere else in its own file is dead regardless of what it resolves to, and that is * decidable from the file alone without a compiler or a per-run install. */ -/** Directories this check owns. Grow it only alongside a run that comes back clean. */ -export const DEAD_IMPORT_ROOTS = ['src/server/ws'] as const +/** + * Directories this check owns: every source root no compiler checks. + * + * `desktop/` is absent because `desktop/tsconfig.json` already sets + * `noUnusedLocals`, which is strictly stronger. Running this scan over it finds + * nothing, which is the cross-check that the analysis agrees with a real compiler. + */ +export const DEAD_IMPORT_ROOTS = ['src', 'scripts', 'adapters'] as const /** * Imports kept despite having no reference in their own file, keyed by @@ -49,8 +56,7 @@ export type DeadImport = { * Characters after which a `/` opens a regular expression rather than dividing. * * `)` and `}` are absent on purpose: `(a + b) / 2` is ordinary and a regex directly - * after a closing bracket is not. `!` is present for `!/re/.test(x)` and costs the - * postfix case, `input! / 10`, which `blankingIsSound` catches instead. + * after a closing bracket is not. */ const REGEX_MAY_FOLLOW = new Set( ['', '(', '[', '{', ',', ';', ':', '=', '!', '&', '|', '?', '+', '-', '*', '%', '^', '~', '<', '>'], @@ -61,6 +67,24 @@ const REGEX_MAY_FOLLOW_KEYWORD = new Set( ['return', 'typeof', 'instanceof', 'in', 'of', 'new', 'delete', 'void', 'throw', 'case', 'do', 'else', 'yield', 'await'], ) +/** + * Whether the `!` before a slash ends an expression instead of negating one. + * + * `!` is the one character that reaches the slash from both sides: `!/re/.test(x)` + * negates a regex test, and `estimates[i]! / total` is a TypeScript non-null + * assertion followed by a division. What precedes the `!` settles it — an operand + * ends with an identifier, a closing bracket or a literal. + */ +function endsExpressionBeforeSlash(blanked: readonly string[], slashIndex: number): boolean { + let cursor = slashIndex - 1 + while (cursor >= 0 && /\s/.test(blanked[cursor]!)) cursor -= 1 + if (cursor < 0 || blanked[cursor] !== '!') return false + cursor -= 1 + while (cursor >= 0 && /\s/.test(blanked[cursor]!)) cursor -= 1 + if (cursor < 0) return false + return /[\w$)\]'"`]/.test(blanked[cursor]!) +} + /** * Blanks comments and literal text so an identifier scan sees code only. * @@ -83,6 +107,15 @@ export function blankNonCode(source: string): string { let braceDepth = 0 /** Last significant code character, which decides `/` division vs regex. */ let previous = '' + /** + * The last identifier token, empty if the last token was not one. Accumulated + * from code only — reading it back out of the raw source let a preceding + * comment's last word decide, which is how `// …correction tone` made the regex + * on the next line lex as a division. + */ + let word = '' + /** Whether the cursor is inside that identifier, so `return false` stays two. */ + let inWord = false let index = 0 /** Blanks a template's literal run; returns the index after it. */ @@ -98,6 +131,8 @@ export function blankNonCode(source: string): string { cursor += 1 } blankTo(from, cursor) + word = '' + inWord = false if (source[cursor] === '`') { templateStack.pop() previous = '`' @@ -112,10 +147,13 @@ export function blankNonCode(source: string): string { const char = source[index]! const next = source[index + 1] + // A comment is whitespace to the grammar, so it leaves `previous` and `word` + // exactly as the code before it left them. if (char === '/' && next === '/') { let end = source.indexOf('\n', index) if (end === -1) end = source.length blankTo(index, end) + inWord = false index = end continue } @@ -124,17 +162,19 @@ export function blankNonCode(source: string): string { const closing = source.indexOf('*/', index + 2) const end = closing === -1 ? source.length : closing + 2 blankTo(index, end) + inWord = false index = end continue } if (char === '/') { - const wordBefore = source.slice(0, index).match(/([A-Za-z_$][\w$]*)\s*$/) - const startsRegex = wordBefore - ? REGEX_MAY_FOLLOW_KEYWORD.has(wordBefore[1]!) - : REGEX_MAY_FOLLOW.has(previous) + const startsRegex = word + ? REGEX_MAY_FOLLOW_KEYWORD.has(word) + : REGEX_MAY_FOLLOW.has(previous) && !endsExpressionBeforeSlash(out, index) if (!startsRegex) { previous = '/' + word = '' + inWord = false index += 1 continue } @@ -153,6 +193,8 @@ export function blankNonCode(source: string): string { } blankTo(index + 1, cursor) previous = '/' + word = '' + inWord = false index = cursor + 1 continue } @@ -166,6 +208,8 @@ export function blankNonCode(source: string): string { } blankTo(index + 1, cursor) previous = char + word = '' + inWord = false index = cursor + 1 continue } @@ -179,6 +223,8 @@ export function blankNonCode(source: string): string { if (char === '{') { braceDepth += 1 previous = '{' + word = '' + inWord = false index += 1 continue } @@ -186,6 +232,8 @@ export function blankNonCode(source: string): string { if (char === '}') { braceDepth -= 1 previous = '}' + word = '' + inWord = false index += 1 if (templateStack.length > 0 && templateStack[templateStack.length - 1] === braceDepth) { index = consumeTemplateText(index) @@ -193,9 +241,20 @@ export function blankNonCode(source: string): string { continue } - // Whitespace leaves `previous` alone, so a line break does not look like a - // fresh statement to the `/` decision above. - if (!/\s/.test(char)) previous = char + // Whitespace leaves both alone, so a line break does not look like a fresh + // statement to the `/` decision above, and `return\n /re/` still lexes. + if (/[\w$]/.test(char)) { + word = inWord ? word + char : char + inWord = true + previous = char + } else if (!/\s/.test(char)) { + word = '' + inWord = false + previous = char + } else { + // Whitespace ends the token but keeps it, so `return\n /re/` still lexes. + inWord = false + } index += 1 } @@ -274,17 +333,15 @@ function parses(source: string): boolean { * Blanking only ever replaces comment and literal text with spaces, so the result * must still parse. When it does not, the lexer mistook code for a literal — and a * blanked reference reads as an unused import. This is the guard that makes a - * lexical analysis safe to fail a build on. 6 of 2149 files in this repository - * desync today — regular expressions holding a quote, and `input! / 10`, where the - * `!` of a non-null assertion looks like the `!` of a negated regex test — and every - * one of them is reported rather than silently mis-analysed. None are under - * `DEAD_IMPORT_ROOTS`; widening those roots means teaching the lexer first. + * lexical analysis safe to fail a build on, and it caught every desync this checker + * has had: a regular expression holding a quote, `input! / 10` reading as a negated + * regex test, `return false` accumulating into a single token, and a comment's last + * word deciding how the next line lexed. All four are fixed; the guard stays. * - * A file the transpiler cannot parse at all is degraded too: there is nothing to - * compare against. + * A file that never parsed is degraded too and needs no separate check, because + * blanking cannot repair a syntax error. */ export function blankingIsSound(source: string): boolean { - if (!parses(source)) return false return parses(blankNonCode(source)) }