From 1f100d1831d3e57f6b9af67a237ce565f79c5f8b Mon Sep 17 00:00:00 2001 From: Randall Stillwell Date: Tue, 26 May 2026 22:50:16 -0500 Subject: [PATCH] chore(lint): forbid require() in scripts/** under "type": "module" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add an ESLint no-restricted-syntax rule scoped to scripts/**/*.js that flags any CallExpression with callee name `require`. The recurring bug pattern: helper scripts under scripts/ that use CJS require() throw `ReferenceError: require is not defined` on Node 22.x because package.json has had "type": "module" since bump-next-js. The bug has bitten twice in two convoys — once in drop-public-setup Brief 2 (setup-neon-db.js, commit b63b509) and again in fix-reset-db-script (reset-db.js, PR #25 squash 3ab9bf8). Both were caught at first run, not at lint time. This rule would have caught both at PR time. Rule shape: a second flat-config block at the end of eslint.config.mjs (NOT in the root rules block) targeting only scripts/**/*.js. The error message points at .convoys/fix-reset-db-script.md so the next agent who trips it gets a 1-click path to the exemplar fix (ESM top-level imports for dotenv, neon, bcrypt) instead of having to re-derive it. scripts/migrations/** is already in globalIgnores from pick-a-name Brief 2 and stays excluded. Blast-radius rationale (scripts/** only, not all .js at repo root): matches the actual observed bug surface. pages/api/** is already correctly ESM-imported throughout (verified across add-rate-limiting, cors-tighten, and the add-route skill). The config files (postcss.config.js, tailwind.config.js, next.config.js) intentionally use CJS-style exports that the next-config base rules already handle correctly. A repo-wide ban would produce zero true positives outside scripts/** today and would require explicit allowlist for every config file — strictly more code, more maintenance, zero benefit. Convoy file: .convoys/lint-against-cjs-in-esm-scripts.md (P3 polish, parent-owned, no architect — preventative one-line rule following two proven bug recurrences). Verification: - node --check eslint.config.mjs: exit 0 - npm run lint: 128 problems (81 errors, 47 warnings) — baseline preserved verbatim, zero new false positives in current tree - Negative test: prepended `const x = require('fs');` to scripts/reset-db.js, ran npm run lint, observed exit 1 with 129 problems and the rule firing at line 20:11 with the documented message, then reverted to 128 problems clean - npm run test:run: 21/21 pass (no test surface touched) - Grep: 0 require( occurrences in scripts/**/*.js (current tree is clean; rule starts with zero positives to silence on day 1) Surfaces no new follow-up — this convoy IS the follow-up surfaced by fix-reset-db-script. Co-authored-by: Cursor --- .convoys/lint-against-cjs-in-esm-scripts.md | 176 ++++++++++++++++++++ eslint.config.mjs | 9 + 2 files changed, 185 insertions(+) create mode 100644 .convoys/lint-against-cjs-in-esm-scripts.md diff --git a/.convoys/lint-against-cjs-in-esm-scripts.md b/.convoys/lint-against-cjs-in-esm-scripts.md new file mode 100644 index 0000000..059477b --- /dev/null +++ b/.convoys/lint-against-cjs-in-esm-scripts.md @@ -0,0 +1,176 @@ +--- +name: lint-against-cjs-in-esm-scripts +classification: hygiene +success_metric: future helper scripts that re-introduce CJS `require()` calls under `package.json` "type": "module" fail at lint time, not at first execution +status: open +created: 2026-05-26 +--- + +# lint-against-cjs-in-esm-scripts (P3 polish — parent-owned) + +**Priority:** P3 polish (one-line ESLint rule; no architect required) +**Convoy owner:** parent +**Opened:** 2026-05-26 + +## Background — the recurring bug pattern + +Since `bump-next-js` flipped `package.json` to `"type": "module"`, +any helper script under `scripts/` that uses CJS `require()` throws +`ReferenceError: require is not defined` on Node 22.x at first run. +The same bug has now bitten the repo twice in two convoys: + +1. **`drop-public-setup` Brief 2** (commit `b63b509`, 2026-05-23): + `scripts/setup-neon-db.js` was still CJS post-`bump-next-js`; `npm + run setup-db` was silently broken until Brief 2 swept it to ESM + imports. The convoy retro called this out as "the seed script + silently stopped executing after `bump-next-js`." +2. **`fix-reset-db-script` Brief 1** (commit `3ab9bf8`, PR #25, + 2026-05-26): three `require()` calls in `scripts/reset-db.js` + (lines 10, 12, 142) — same bug, same blast radius (`npm run + reset-db` throws `ReferenceError`), same fix shape (verbatim + mirror of post-`drop-public-setup` `setup-neon-db.js`). + +Both bugs were caught at first run, not at lint time. A small +ESLint rule scoped to `scripts/**/*.js` would have caught both at +PR time and is cheap insurance against a third recurrence. + +## Design decision — `scripts/**` only (NOT all `.js`) + +Two reasonable scopes: + +- **`scripts/**/*.js` (chosen):** matches the actual blast radius — + every observed instance of the bug has been in a helper script. + Per-file-block override in `eslint.config.mjs` via a second flat- + config entry. Zero impact on `pages/api/**` (already correctly + ESM-imported throughout) and zero impact on the root `*.config.js` + files (which are intentionally CJS-shaped and which the next-config + base rules already handle correctly). +- **All `.js` files at repo root (rejected):** broader-than-necessary + blast radius. `pages/api/**` already uses ESM `import` everywhere + (`add-rate-limiting`, `cors-tighten`, and `add-route` skill all + verified this in the last three months). A repo-wide ban would + produce zero true positives outside `scripts/**` today and would + risk breaking config-file shapes that legitimately use CJS + (`postcss.config.js`, `tailwind.config.js` are flagged by + `import/no-anonymous-default-export` today but read CJS-style + exports under the hood — see also Gotcha #9 + #10). + +The scoped rule is a 7-line flat-config block; the broader rule +would require explicit allowlist for every config file, which is +strictly more code and more maintenance. + +## The fix + +Add a new flat-config block at the end of `eslint.config.mjs` (after +the existing `globalIgnores(...)` call, NOT inside the root rules +block) targeting only `scripts/**/*.js`: + +```js +{ + files: ['scripts/**/*.js'], + rules: { + 'no-restricted-syntax': ['error', { + selector: 'CallExpression[callee.name="require"]', + message: 'Use ESM `import` syntax. `package.json` has "type": "module"; require() throws ReferenceError at runtime. See .convoys/fix-reset-db-script.md.', + }], + }, +}, +``` + +The error message points at `.convoys/fix-reset-db-script.md` so that +the next agent / contributor who triggers the rule gets a 1-click path +to the exemplar fix (ESM top-level imports for `dotenv`, `neon`, +`bcrypt`) instead of having to re-derive it. + +`scripts/migrations/**` is already in `globalIgnores` (from +`pick-a-name` Brief 2's migration script) and stays ignored — the +rule does not fire there even though the migration script is ESM and +correctly uses `import` (no need to re-lint files already excluded). + +The rule fires on `CallExpression[callee.name="require"]` — the AST +shape of a plain `require('foo')` call. It does NOT fire on +`createRequire(import.meta.url)` patterns (which use `Module.createRequire`) +should one ever be needed; the AST callee is `createRequire`, not +`require`. If a future helper script legitimately needs CJS interop, +the right path is `await import('foo')` (ESM dynamic import) — the +rule will not block that either. + +## Verification plan + +1. `node --check eslint.config.mjs` → exit 0 (config parses). +2. `npm run lint` → exit 1 with **128 problems (81 errors, 47 + warnings)** — verbatim match of the pre-convoy baseline (no + regression, no new false positives in the current tree). +3. **Negative test (apply, run, revert):** prepend + `const x = require('fs');` to `scripts/reset-db.js`, run + `npm run lint`, confirm exit 1 with the new rule firing at the + expected line/column and the documented message, then revert. +4. `npm run test:run` → 21/21 pass (no test surface touched; runs + only to confirm vitest is still green). +5. `rg "require\(" scripts/ --type js` → 0 hits (sanity check + confirming the current tree is clean and the rule has zero + positives to silence on day 1). + +## Risks + +- **False positives if anyone legitimately needs `require()` in + `scripts/**`.** None today (verified by step 5 — zero `require(` + hits across all helper scripts in the current tree after PR #25 + and the `drop-public-setup` B2 sweep). If a future script + legitimately needs CJS interop (e.g. a dependency that only + exports CJS without an ESM wrapper), the fix is `await + import('foo')` — ESM dynamic import works in any ESM script and + is not flagged by the rule. If that's somehow not viable, the + escape hatch is a per-line `// eslint-disable-next-line + no-restricted-syntax` with a comment explaining why ESM doesn't + work; lint baseline tracking will catch the disable directive in + review. +- **Rule scope drift.** If someone adds a new top-level scripts + directory (`tools/`, `cli/`, etc.) the rule won't fire there. Low + risk — this repo has consolidated on `scripts/` since inception + and there's no signal of a second scripts directory being added. + Tracked here so the next refactor that reshapes the helper-script + layout knows to extend the `files:` glob. +- **ESLint v10 bump.** When `bump-eslint-10` lands (currently + upstream-blocked per Gotcha #10), re-verify this rule's selector + syntax against the v10 AST behavior. `no-restricted-syntax` is a + stable core rule going back to ESLint v1; no v10 deprecation is + expected, but the smoke check is cheap. + +## Acceptance criteria + +- `node --check eslint.config.mjs` exit 0 +- `npm run lint` exit 1 with 128 problems (baseline preserved) +- `npm run test:run` 21/21 pass +- Negative test passes (rule fires on synthetic `require()` insertion, + reverts cleanly to 128 problems after the synthetic edit is + removed) +- Grep: 0 `require(` occurrences in `scripts/**/*.js` (current tree + is clean — rule starts with zero positives to silence) + +## Out of scope + +- Sweeping any other `scripts/**` file — current tree is clean + (verified by the grep step above). The rule is preventative, + not retroactive. +- Broadening the rule to all `.js` files at repo root — see + § Design decision; `pages/api/**` is already correctly ESM and + the config files (`postcss.config.js`, `tailwind.config.js`, + `next.config.js`) intentionally use CJS-style exports that the + next-config base rules handle correctly. +- Bumping any deps (ESLint stays at v9 per Gotcha #10; no + `typescript-eslint` interaction since the rule is a core rule). +- The `purge-weak-creds-from-helpers` follow-up (the + `scripts/create-test-users.js` portion remains queued; this + convoy only adds the lint rule, not the weak-creds sweep). + +## Owns + +Parent (single-file ESLint config edit; no architect or implementer +subagent required — proven-pattern follow-up to PR #25). + +## As-shipped + +_(Stub — doc-writer to fill in post-merge with squash commit SHA, +PR URL, observed lint baseline before/after, CI gate results, and +any deviations from the planned shape.)_ diff --git a/eslint.config.mjs b/eslint.config.mjs index 1bfbd39..1705fe9 100644 --- a/eslint.config.mjs +++ b/eslint.config.mjs @@ -11,6 +11,15 @@ const eslintConfig = defineConfig([ 'next-env.d.ts', 'scripts/migrations/**', ]), + { + files: ['scripts/**/*.js'], + rules: { + 'no-restricted-syntax': ['error', { + selector: 'CallExpression[callee.name="require"]', + message: 'Use ESM `import` syntax. `package.json` has "type": "module"; require() throws ReferenceError at runtime. See .convoys/fix-reset-db-script.md.', + }], + }, + }, ]); export default eslintConfig;