Skip to content

Commit f1c0fce

Browse files
committed
refactor(scripts): trim the specifier audit's comments
The audit shipped at 24% comment lines — the header alone retold the whole incident. Cut to 15% (452 -> 401 lines) by collapsing the narrative and keeping only what the code cannot say: the webpack/Turbopack extensionAlias divergence, why '.js' is a probed extension but not a fallback, why paths resolve per-workspace, why targets substitute with replaceAll, why generated output is unverifiable, and the '.claude/' worktree trap in the relative-path check. No behaviour change: 37,437 specifiers still resolve clean.
1 parent 52d6e80 commit f1c0fce

2 files changed

Lines changed: 34 additions & 85 deletions

File tree

.github/workflows/test-build.yml

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -162,8 +162,8 @@ jobs:
162162
- name: Trigger/block initialization cycle audit
163163
run: bun run check:trigger-block-cycle
164164

165-
# `next build` is webpack and `next dev` is Turbopack, so a specifier only
166-
# webpack can resolve builds green here and breaks every dev server.
165+
# This job builds with webpack; devs run Turbopack. A specifier only webpack
166+
# resolves passes here and breaks every dev server.
167167
- name: Import specifier hygiene audit
168168
run: bun run check:import-specifiers
169169

scripts/check-import-specifiers.ts

Lines changed: 32 additions & 83 deletions
Original file line numberDiff line numberDiff line change
@@ -1,41 +1,21 @@
1-
#!/usr/bin/env bun
21
/**
3-
* Resolves every first-party import specifier the way Turbopack does, and fails on any
4-
* that does not land on a real file.
2+
* Resolves every first-party import specifier the way Turbopack does, failing on any that
3+
* does not land on a real file.
54
*
6-
* This exists because `next build` runs webpack and `next dev` runs Turbopack, and the
7-
* two do not resolve the same set of specifiers. Anything webpack accepts and Turbopack
8-
* rejects builds green in CI and 500s on every developer's machine — CI cannot see it,
9-
* because CI never runs the Turbopack graph.
5+
* `next build` runs webpack and `next dev` runs Turbopack, and they do not resolve the same
6+
* specifiers. webpack rewrites `./errors.js` -> `./errors.ts` via `resolve.extensionAlias`;
7+
* Turbopack has no equivalent (vercel/next.js#82945). So that shape builds green in CI and
8+
* 500s on every developer's machine — CI never runs the Turbopack graph.
109
*
11-
* The concrete instance: `packages/utils/src/index.ts` addressed its siblings as
12-
* `./errors.js` while the files are `./errors.ts`. webpack rewrites that through
13-
* `resolve.extensionAlias`; Turbopack has no equivalent (vercel/next.js#82945). Every
14-
* route reaching the `@sim/utils` barrel died with
15-
* `Module not found: Can't resolve './errors.js'`, and the PR that introduced it passed
16-
* CI clean.
10+
* Running real resolution rather than matching that one mistake covers the whole
11+
* "Module not found" class: bad extensions, typo'd paths, stale importers of moved files,
12+
* dead `@/` aliases, and `@sim/*` subpaths a package does not export.
1713
*
18-
* Rather than pattern-match that one mistake, this walks the real resolution algorithm
19-
* with the extensionAlias fallback deliberately absent. That generalises to the whole
20-
* "Module not found" class: `.js` specifiers, typo'd paths, files moved or deleted with
21-
* a stale importer left behind, `@/` aliases pointing nowhere, and `@sim/*` subpaths the
22-
* target package does not actually export.
14+
* Skipped: bare npm specifiers (node_modules' business, and flaky on install state),
15+
* type-only imports (erased before resolution), and tests plus `apps/*/scripts/**`,
16+
* which run under vitest and bun — both of which do resolve `.js` -> `.ts`.
2317
*
24-
* It also keeps one convention rule that resolution cannot express: `@sim/utils` must be
25-
* imported by subpath. `@sim/utils/helpers` is one module; the bare barrel is twelve, and
26-
* pulling the barrel is what dragged the broken specifiers above into a route graph.
27-
*
28-
* Deliberately NOT checked:
29-
* - bare npm specifiers — that is node_modules' business, and `bun install` state makes
30-
* it flaky in a way that would train people to ignore this check
31-
* - type-only imports — erased before any bundler resolves them
32-
* - test files and `apps/*/scripts/**` — vitest and `bun run` resolve `.js` -> `.ts`
33-
* themselves, so their specifiers are correct in context; flagging them is noise, and
34-
* a check that cries wolf is a check someone deletes
35-
*
36-
* Usage:
37-
* bun run scripts/check-import-specifiers.ts
38-
* bun run scripts/check-import-specifiers.ts --verbose
18+
* Usage: `bun run scripts/check-import-specifiers.ts [--verbose]`
3919
*/
4020
import { readdirSync, readFileSync, statSync } from 'node:fs'
4121
import { dirname, join, relative, resolve } from 'node:path'
@@ -47,9 +27,8 @@ const SCAN_DIRS = ['apps/sim', 'apps/realtime', 'apps/docs', 'packages']
4727
const SKIP_DIRS = new Set(['node_modules', '.next', 'dist', 'build', '.turbo'])
4828

4929
/**
50-
* Extensions a bundler probes for an extensionless specifier. `.js` is present because a
51-
* real `foo.js` next to the importer resolves fine — what does NOT happen is `./foo.js`
52-
* falling back to `foo.ts`, and that asymmetry is the entire bug this guard exists for.
30+
* `.js` is listed because a real `foo.js` resolves fine. What does not happen is `./foo.js`
31+
* falling back to `foo.ts` — that asymmetry is the bug this guard exists for.
5332
*/
5433
const EXTENSIONS = ['.ts', '.tsx', '.js', '.jsx', '.mjs', '.cjs', '.json']
5534

@@ -58,19 +37,12 @@ const SPECIFIER_RE =
5837
/(?:^|\n)\s*(?:import|export)\s+(?!type\s)(?:[\s\S]*?from\s*)?['"]([^'"]+)['"]/g
5938
/** `import(...)` — resolved at call time, but the path still has to exist. */
6039
const DYNAMIC_RE = /\bimport\s*\(\s*['"]([^'"]+)['"]\s*\)/g
61-
/**
62-
* `require('@/...')` — this repo uses lazy requires deliberately to break import cycles
63-
* (`tools/params.ts` reaches `@/blocks` that way, `blocks/blocks/agent.ts` reaches
64-
* `@/blocks/registry`). Those edges resolve exactly like static ones, so a bad specifier
65-
* in one fails identically and must be checked.
66-
*/
40+
/** Lazy `require()` is used here to break import cycles; those edges resolve like static ones. */
6741
const REQUIRE_RE = /\brequire\s*\(\s*['"]([^'"]+)['"]\s*\)/g
6842

6943
/**
70-
* Packages that must be imported by subpath. Opt-in rather than opt-out: `@sim/emcn` and
71-
* `@sim/desktop-bridge` are barrel-first by design, and flagging their 33 call sites would
72-
* bury the one rule that matters. `@sim/utils` is subpath-only by documented convention
73-
* (CLAUDE.md, "Common Utilities") and is the package whose barrel took routes down.
44+
* Subpath-only packages. Opt-in: `@sim/emcn` and `@sim/desktop-bridge` are barrel-first by
45+
* design, so flagging them would bury the one rule that matters.
7446
*/
7547
const SUBPATH_REQUIRED = new Set(['@sim/utils'])
7648

@@ -99,18 +71,11 @@ function walk(dir: string, acc: string[] = []): string[] {
9971
}
10072

10173
/**
102-
* True when a path lands in output the scanner itself refuses to read as source:
103-
* `node_modules`, a build directory, or any dot-directory.
74+
* Build output the scanner will not read as source, so it cannot assert on its presence
75+
* either — `apps/docs/.source` is generated by fumadocs-mdx and absent from a fresh checkout.
10476
*
105-
* These are generated and gitignored, produced by a build step that has not necessarily run
106-
* yet — `apps/docs/.source` is emitted by fumadocs-mdx, so `@/.source/server` resolves on a
107-
* machine that has built the docs and is absent from a fresh CI checkout. Asserting on them
108-
* makes the verdict depend on build order rather than on the source, which is exactly the
109-
* kind of flake that gets a CI gate switched off.
110-
*
111-
* Only the repo-relative portion is inspected. The absolute path can itself sit under a
112-
* dot-directory — a git worktree lives in `.claude/worktrees/…` — which would otherwise make
113-
* every specifier in the repo look generated.
77+
* Only the repo-relative portion is inspected: a git worktree lives under `.claude/`, which
78+
* would otherwise make every specifier in the repo look generated.
11479
*/
11580
function isGeneratedPath(absolute: string): boolean {
11681
const rel = relative(ROOT, absolute)
@@ -140,24 +105,17 @@ function probe(base: string): string | null {
140105
}
141106

142107
/**
143-
* `paths` from the workspace that owns a file, resolved to absolute prefixes.
144-
*
145-
* Per-workspace, not global: `@/*` is `apps/sim/*` inside apps/sim but `apps/realtime/src/*`
146-
* inside apps/realtime, and apps/sim additionally maps `@sim/db/*` straight at the package
147-
* directory — which legitimately bypasses that package's `exports` map. Resolving with one
148-
* hardcoded alias reported ~30 false positives against apps/realtime alone.
108+
* `paths` from the workspace owning a file. Per-workspace, not global: `@/*` differs between
109+
* apps/sim and apps/realtime, and apps/sim maps `@sim/db/*` straight at the package directory,
110+
* bypassing its `exports` map.
149111
*/
150112
interface PathRule {
151113
prefix: string
152114
suffix: string
153115
wildcard: boolean
154116
/**
155-
* Absolute targets; `*` is retained verbatim and substituted at match time.
156-
*
157-
* Substituted with `replaceAll`, not `replace`: Node's own `exports` resolver uses a
158-
* global regex, so a target carrying more than one `*` gets every occurrence filled.
159-
* Replacing only the first would leave a literal `*` in the path and report a valid
160-
* subpath as missing.
117+
* Absolute targets, `*` substituted at match time with `replaceAll` — Node's `exports`
118+
* resolver uses a global regex, so a target with two wildcards fills both.
161119
*/
162120
targets: string[]
163121
}
@@ -205,10 +163,7 @@ function workspaceFor(file: string): Workspace | undefined {
205163
return workspaces.find((w) => file.startsWith(`${w.dir}/`))
206164
}
207165

208-
/**
209-
* Sentinel for "a tsconfig path matched, but every candidate target is generated output".
210-
* Distinct from `null` (matched and genuinely missing) and `undefined` (no pattern matched).
211-
*/
166+
/** Matched a tsconfig path, but every target is generated — distinct from missing (`null`). */
212167
const GENERATED = Symbol('generated')
213168

214169
/** Resolve through the owning workspace's tsconfig `paths`. */
@@ -332,10 +287,9 @@ const violations: Violation[] = []
332287
let checked = 0
333288

334289
/**
335-
* Blank out comments while preserving every byte offset, so reported line numbers stay
336-
* exact. TSDoc routinely contains example imports — `packages/db/triggers.ts` documents
337-
* `import { ensureRowCountTriggers } from '@sim/db/triggers'`, a subpath the package
338-
* deliberately does not export — and scanning raw source reports those as broken.
290+
* Blank comments in place, preserving byte offsets so line numbers stay exact. TSDoc carries
291+
* example imports that are not real edges — `packages/db/triggers.ts` documents a subpath the
292+
* package deliberately does not export.
339293
*/
340294
function blankComments(src: string): string {
341295
return src
@@ -367,12 +321,7 @@ for (const file of files) {
367321
let m = pattern.exec(src)
368322
while (m !== null) {
369323
const spec = m[1]
370-
/**
371-
* Anchor to the specifier, not to `m.index`. SPECIFIER_RE opens with `(?:^|\n)`, so
372-
* `m.index` is the newline ENDING the previous line — reporting it put every violation
373-
* one line early. The specifier's own offset is exact, and for a multi-line import it
374-
* points at the `from '...'` line, which is where the reader needs to look anyway.
375-
*/
324+
// `m.index` is the newline ending the previous line; the specifier's offset is exact.
376325
const at = m.index + m[0].lastIndexOf(spec)
377326
const outcome = resolveSpecifier(spec, file)
378327
if (outcome) {

0 commit comments

Comments
 (0)