build(policy): fix four lexer desyncs and extend the dead-import check to all of src

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.
This commit is contained in:
程序员阿江(Relakkes)
2026-08-04 20:42:45 +08:00
parent bde985d202
commit 1aefc402ce
4 changed files with 175 additions and 41 deletions
+26 -9
View File
@@ -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
+8 -7
View File
@@ -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([
+62 -3
View File
@@ -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', () => {
+79 -22
View File
@@ -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))
}