lint:pkg: enforce module-eval side-effect freedom (queue #93)

[Goosterhof]

What

Promotes the package-global "sideEffects": false manifest claim — declared on every package by queue #70 / PR #101, where lint:pkg already fails if the flag is missing — from an unenforced Level-4/Level-6 promise to a Level-1 CI gate that fails the moment a side-effecting top-level statement lands in any package source.

The flag is a bundler-facing promise: a consumer's bundler tree-shakes away any module whose exports are unused, on the assumption that dropping it changes nothing. Today nothing verifies that premise. If a future author adds a top-level effect (import './register-globals', a module-eval console.warn, an Object.defineProperty, a prototype patch), the manifest still says false, the bundler drops the module, and the effect silently vanishes at the consumer with zero gate signal.

The invariant (§B)

sideEffects: false is a package-global claim, so the gate scope is every source module in every package — not just index.ts and its re-exports. (This deliberately widens the queue #93 wording, which framed the target as index.ts + its re-exported files. A side effect in a file that is merely imported — not re-exported — by a re-exported file would slip a re-export-graph trace, yet the bundler's sideEffects:false assumption still covers it. Walking all packages/*/src/**/*.ts is both simpler and the semantically correct match.)

Each source file is parsed with the TypeScript compiler API (import ts from 'typescript' — already a devDep for Gate 5 tsc, no new dependency). The only top-level statement kinds permitted are: imports with at least one specifier; export … from / export * / export { … }; export default of a function or class; interface/type/enum/namespace; const/let/var; function/class declarations. Anything else FAILS — chiefly any bare ExpressionStatement (call / assignment), any specifier-less side-effect import, or top-level control flow.

Why scripts/lint-pkg.mjs, not a vitest arch test (§C)

• Keeps the change out of packages/*/src/ → coverage Gate 7 (100%/pkg) and mutation Gate 8 (Stryker 90%/pkg) stay untouched. A vitest arch test would itself demand coverage/mutation on its helper code.
• Co-locates with the manifest flag it guards: fs-dialog: forward host <dialog> ARIA attributes via dialog.open() options (#67) #70 declares sideEffects:false, chore(deps): bump vue-component-type-helpers from 3.2.8 to 3.2.9 #93 verifies its premise — same Gate-6 enforcer, adjacent check next to publint/attw (fs-packages M1 moderate remediation (Armorer) #33/fs-router 0.1.0 published peer-dep is stale (vue-router ^4.5.0 vs source ^5.0.6) — republish as 0.1.1 #63) and engines.node (Fix setById reactivity by invalidating adapted cache #31).
• One pass over all packages, identical to the existing loop.

Load-bearing proof (§D — RED/GREEN, captured verbatim)

A gate that never fires is indistinguishable from a no-op (the queue #63 lesson — that gate was a silent no-op in CI for ~20 days). Proven both directions before calling it done.

RED — top-level console.warn injected into packages/helpers/src/index.ts:

$ npm run lint:pkg   # EXIT CODE: 1
lint:pkg gate FAILED (1):
  - @script-development/fs-helpers: packages/helpers/src/index.ts:1 — top-level expression statement (call / assignment evaluates at module load) (queue #93 — sideEffects:false requires module-eval side-effect freedom)

RED — specifier-less import import './type-guards'; (distinct ban-list branch):

$ npm run lint:pkg   # EXIT CODE: 1
lint:pkg gate FAILED (1):
  - @script-development/fs-helpers: packages/helpers/src/index.ts:1 — specifier-less side-effect import (`import '...'`) (queue #93 — sideEffects:false requires module-eval side-effect freedom)

GREEN — injection reverted, clean tree:

$ npm run lint:pkg   # EXIT CODE: 0
lint:pkg gate PASS — 11 packages + root clean (engines.node present; publint suggestions/warnings/errors all treated as fatal; every package source module asserted module-eval side-effect-free per sideEffects:false, queue #93).

All 35 source files across the 11 packages classify clean today.

Gate results

Gate	Result
npm run lint:pkg (modified gate)	PASS clean + RED/GREEN pair above
npm run lint (oxlint)	PASS (exit 0)
npm run format:check (oxfmt)	PASS (.mjs re-formatted via npm run format; .md outside oxfmt scope)
npm run build	PASS (unaffected — no src/ change)
npm run typecheck	PASS (unaffected)

Doctrine propagation (§F)

CLAUDE.md "No top-level side effects" bullet updated to cite the now-enforcing gate and the queue #93 lineage — the front door reflects the Level-1 promotion.

Lineage

Sister to #70 (manifest-flag compliance) and #63 (the honest-gate / no-silent-no-op lesson). Author: Goosterhof → no self-approval; awaits script-development ally formal review.

🤖 Generated with Claude Code

[cloudflare-workers-and-pages]

Deploying fs-packages with    Cloudflare Pages

Latest commit:	384aadf
Status:	✅  Deploy successful!
Preview URL:	https://d503686d.fs-packages.pages.dev
Branch Preview URL:	https://armorer-queue-93-sideeffects.fs-packages.pages.dev

View logs

[jasperboerhof]

PR Reviewer · claimed

• Target: fs-packages
• PR: lint:pkg: enforce module-eval side-effect freedom (queue #93) #104
• Branch: armorer/queue-93-sideeffects-gate
• AC anchor: none (no Kendo board for fs-packages; no plan dir on branch; PR body has structured §B/§C/§D sections but no Acceptance Criteria heading)
• Worktree: ~/Code/agent-worktrees/fs-packages/review-104

[jasperboerhof]

PR Reviewer · 6/10 · REVISE

• PR: lint:pkg: enforce module-eval side-effect freedom (queue #93) #104 · fs-packages
• AC anchor: none
• Reviewers run: Acceptance, Simplicity, Surface, Silent failures, Efficiency skipped — no triggers
• Acceptance: SKIP
• Simplicity: PASS · 9/10
• Surface: PASS · 9/10
• Silent failures: NEEDS WORK · 6/10

Findings

• MAJOR · scripts/lint-pkg.mjs:71 — Silent empty-fallback defeats the gate's own purpose. listSourceFiles wraps readdirSync(srcDir) in try { ... } catch { return out; } — on ANY read error (ENOENT missing src/, EACCES, transient FS error) it returns [] with no stderr line and no failure recorded. listPackageDirs (line 77-79, pre-existing) filters packages only on package.json existing, NOT on src/ existing, so a package whose sources are absent/renamed/unreadable flows straight into the side-effect loop, yields sideEffectFailures===0, and main() prints <pkg>: 0 source file(s) side-effect-free OK (line 282-283) — a GREEN pass having scanned zero files. This is a CI gate whose entire stated reason for existing (header line 30-32, CLAUDE.md) is to stop a top-level side effect from 'silently vanishing'; the catch lets the gate itself silently no-op. Fix: treat a missing src/ on a package that passed listPackageDirs as a failure (or at minimum emit a stderr warning + non-zero file count assertion), and narrow the catch so EACCES/real read errors push a failure rather than masquerading as 'no files'.
• MINOR · scripts/lint-pkg.mjs:62 — TEST_FILE_RE / TEST_DIR_RE guards in listSourceFiles currently match nothing — tests live in packages/*/tests/ (siblings of src/), already excluded by the src/** walk. The code's own comment labels this defensive 'belt-and-suspenders' for a possible future co-located spec. Cheap (two regexes) and guards a real future risk (a top-level describe(...) in a co-located spec would otherwise be wrongly flagged), so noted only — not a demand to remove. Plan-less mode, so flagged per the universal dead-scaffolding heuristic, but the defensiveness is justified.
• MINOR · scripts/lint-pkg.mjs:196 — Row 5 (silent degradation): the side-effect check prints ${name}: ${sourceFiles.length} source file(s) side-effect-free OK even when listSourceFiles() returns []. A future restructure that moves sources out of packages/*/src/ would make the gate report a clean PASS having parsed zero files — the same false-negative shape queue fs-router 0.1.0 published peer-dep is stale (vue-router ^4.5.0 vs source ^5.0.6) — republish as 0.1.1 #63 fixed for the publint block in this very script (header comment lines 14-26). Empirically NOT triggered today (verified the gate parses 5/3/1/... non-zero files across all 11 packages and correctly FAILs on an injected top-level console.warn), so this is a latent guard-gap, not a live defect. Remediation: assert sourceFiles.length > 0 per package (every published package has at least src/index.ts), or fail the gate if the aggregate source-file count across all packages is zero.

Action

revise — address MAJORs

[Goosterhof]

No blockers. One concern, two nits. The RED/GREEN proof in the PR body is the right discipline for a gate promotion — both the bare-expression and the specifier-less-import ban branches are exercised before calling it done, which is the queue #63 lesson applied honestly. Verdict: COMMENT, mergeable after a look at the concern.

The choice to walk all packages/*/src/**/*.ts rather than trace the re-export graph is correct and the §B rationale defends it well: sideEffects: false is a package-global claim, so a side effect in a merely-imported (not re-exported) module is still covered by the bundler's drop-this-module assumption. A re-export-graph trace would miss exactly that case.

Concern

The gate's enforcement is narrower than its stated invariant — top-level const/let/var initializers are never inspected. classifyTopLevelStatement returns null unconditionally for ts.SyntaxKind.VariableStatement (scripts/lint-pkg.mjs, the case ts.SyntaxKind.VariableStatement: arm of the permit block). That means a top-level binding whose initializer evaluates a side effect at module load — const _ = console.warn('loaded'), const patched = Object.defineProperty(globalThis, ...), const ready = registerGlobals() — passes the gate while doing exactly the thing the PR title forbids ("module-eval side-effect freedom"). The bare ExpressionStatement form (console.warn('loaded') with no binding) is caught; wrap the identical call in const _ = and it is not. An author working around the gate after seeing it fire on the unbound form would reach for the bound form as the first move.

This is defensible today because the factory + barrel pattern means every existing top-level binding is either a literal (DIALOG_STYLE, DEFAULT_TIMEOUT_MS) or an arrow-function factory, never an evaluated call — I checked all 35 source files and none binds a call expression. So the gate is correct for the current corpus. But the gate exists precisely to catch the future author who breaks the pattern, and the most natural way to introduce a module-eval effect is a top-level const x = effectfulCall(), which sails through. The honest-gate framing of the PR body ("a gate that never fires is indistinguishable from a no-op") cuts the other way here: a gate that fires on the unbound effect but not the bound one gives false confidence on the harder half of the problem.

Two ways to close it, pick one:

1. Inspect VariableStatement initializers. For each declared binding, permit literal initializers, identifiers, arrow/function/class expressions, and new-of-a-known-pure-constructor patterns; flag a CallExpression initializer (other than the factory-call exceptions you'd whitelist) as module-eval. More precise, more code, more edge cases (new WeakMap() at cached-adapter-store.ts is an evaluated initializer that is genuinely side-effect-free, so a blanket call/new ban would false-positive — you'd need an allowlist).
2. Narrow the claim to match the enforcement. If initializer inspection is judged not worth the complexity, say so explicitly: the gate asserts the statement kinds are declaration-only, not that initializers are pure, and the const-initializer-purity guarantee rests on the factory-pattern convention + review rather than the parser. That keeps the doctrine note in CLAUDE.md honest about where the machine stops and the convention starts.

Either is fine. What's not fine is leaving the CLAUDE.md bullet asserting CI "asserts the top-level statement list is module-eval side-effect-free" when a whole class of module-eval side effect (bound initializers) is unenforced.

Nits

The inline comment above the ExportAssignment arm says "Only a function- or class-expression default is side-effect-free," but the code also permits ts.SyntaxKind.ArrowFunction (correctly — export default () => {} is inert). Comment understates the permit list; align it to mention arrow functions.

ExportAssignment covers both export default <expr> and export = <expr> (the latter has isExportEquals === true). The arm doesn't branch on node.isExportEquals, so export = someCall() is classified by the same expression check — which happens to be the right outcome, but the comment frames the arm purely as export default. Either branch on isExportEquals or widen the comment so the export = case isn't a surprise to the next reader. No behavior change needed.

[Goosterhof]

⚠️ Concerns

0 blockers · 2 concerns · 2 nits · 1 praise

This promotes the sideEffects: false manifest premise from a doctrine note to a Level-1 lint:pkg gate by parsing every packages/*/src/**/*.ts with the TypeScript compiler API and rejecting non-declaration top-level statements. The gate is honest (RED/GREEN proven, no silent no-op), the TS-API usage is correct, and it earns its keep — but the invariant it advertises is narrower than the invariant it actually enforces, and that gap should be named in the code and the doctrine rather than left implicit.

Concerns

• scripts/lint-pkg.mjs:170 (VariableStatement → permitted) — the gate classifies by statement kind, not by whether a permitted statement evaluates at load. A top-level const x = sideEffectCall() / let p = patchPrototype() / var q = (function(){ globalThis.x = 1; })() is a VariableStatement and passes clean, yet its initializer runs at module-eval — exactly the thing sideEffects: false promises won't happen.

  • Reproduced against this PR's own classifyTopLevelStatement: export const c = sideEffectCall(); → [null] PERMITTED; let z = patchProto(); → PERMITTED; var q = (function(){ globalThis.x=1; return 1; })(); → PERMITTED.
  • The PR body and the CLAUDE.md edit both claim the gate stops "a module-eval console.warn / Object.defineProperty / prototype patch." That holds only when written as a bare ExpressionStatement. const _ = Object.defineProperty(...) — the form a determined author or an auto-formatter reaches for — slips through. The advertised invariant ("module-eval side-effect-free") is stronger than the enforced invariant ("no side-effecting statement kinds").
  • Not a current false-negative: grep over packages/*/src finds zero top-level call-initializers today, so the corpus is clean and this is a future-regression hole, not a live miss. That's why it's a concern, not a blocker.
  • Fix — option A: narrow the claim. Change the §B/CLAUDE.md wording from "module-eval side-effect-free" to "rejects side-effecting top-level statement forms (bare expression statements, specifier-less imports, top-level control flow); initializer-expression side effects in const/let/var are out of scope." An honest narrower claim beats an overclaiming gate. Option B: tighten the gate — flag a VariableStatement whose declaration initializer is a CallExpression/NewExpression that isn't a recognized pure-factory shape. This is the harder path (you'd need an allowlist for legitimate const X = makeEnum()-style pure calls and you don't have one today), so A is the pragmatic move unless the Commander wants the stronger guarantee.
  • Test: add a RED case for export const x = effectfulCall(); to the proof set. Under option A it documents the boundary; under option B it must fail the gate.

• scripts/lint-pkg.mjs:113 (.ts-only walk skips .tsx) — listSourceFiles collects only *.ts (and explicitly the body confirms packages/*/src/**/*.ts). A package shipping a .tsx source — none exist today (find packages/*/src -name '*.tsx' is empty) — would ship every top-level statement unguarded.

  • The corpus is JSX-free frontend-service packages, so this is latent, not active. But the manifest flag is package-global; the gate's file selection isn't. Either widen the glob to .tsx now (cheap, zero current cost) or note the .ts-only scope explicitly in the §B comment so the next .tsx package author knows the gate doesn't cover them.

Nits

• scripts/lint-pkg.mjs:223 — the default: branch label top-level ${ts.SyntaxKind[node.kind] ?? 'statement'} will print the raw enum name (e.g. top-level ForStatement). Fine for a CI failure, but ForStatement/IfStatement/TryStatement are common enough to deserve the same human phrasing the expression-statement branch gets ("top-level control-flow statement (...)") so an ally reading the failure doesn't have to know TS AST kind names.

• scripts/lint-pkg.mjs:139 — TEST_DIR_RE is tested against /${entry.name}/ per-entry, which works, but the (^|[/\\])...([/\\]|$) anchoring is overbuilt for a single path segment; a plain __tests__|tests|test segment compare reads clearer. Cosmetic — the belt-and-suspenders intent is sound and the comment explains it.

Praise

• The decision to widen scope from queue #93's original "index.ts + re-export graph" wording to all packages/*/src/**/*.ts, justified in §B by the bundler's assumption covering merely-imported (non-re-exported) modules, is the correct semantic match for a package-global flag — and walking every file is genuinely simpler than tracing a re-export graph. That's the load-bearing call in this PR and it's right.

Automated war-room agent review — posted because this PR carries the Agent Review Requested label.

[jasperboerhof]

PR Reviewer · claimed

• Target: fs-packages
• PR: lint:pkg: enforce module-eval side-effect freedom (queue #93) #104
• Branch: armorer/queue-93-sideeffects-gate
• AC anchor: none (no kendo board; no plan dir; no Acceptance section in PR body)
• Worktree: ~/Code/agent-worktrees/fs-packages/review-104

[jasperboerhof]

PR Reviewer · 6/10 · REVISE — 🔴 2 🟡 1

fs-packages #104 · AC anchor: none
Scores: acceptance SKIP · simplicity 8 · surface 6 · silent-failure 6 · efficiency 9

🔴 MAJOR

scripts/lint-pkg.mjs:127 — side-effect gate permits side-effectful top-level VariableStatement initializers — a false-negative vs its documented coverage

why + fix

The header comment + CLAUDE.md (queue-#​93) claim the gate stops a future top-level console.warn / Object.defineProperty / prototype patch, but only bare ExpressionStatement forms are caught (line 146). The same effects wrapped in an initializer — const _ = Object.defineProperty(globalThis, ...), export const x = registerSideEffect(), const y = (() => { patchPrototype(); return 1; })() — are module-eval side effects that tree-shake away silently under sideEffects:false, yet sail through. Not a BLOCKER: idiomatic forms ARE caught, the factory+barrel convention makes const-call-init non-idiomatic, the gate is fail-closed (failures push + process.exit(1) at :308), and no current source file trips it — but the rule does not enforce the full surface its own doctrine note claims.

Fix: in classifyTopLevelStatement, stop returning null for VariableStatement — walk each declaration initializer and flag Call/New/IIFE/assignment-bearing expressions, permitting literal/identifier/arrow/function/class/object/array inits (mirror the ExportAssignment handling at :131-145). Or narrow the CLAUDE.md / header prose if const-call-init is intentionally out of scope.

scripts/lint-pkg.mjs:123 — bare empty catch in listSourceFiles() turns ANY readdir failure into a false green PASS

why + fix

The catch (no error binding) returns [] on every failure mode — EACCES, symlink loop (ELOOP), transient CI I/O — not just the legitimate src/-absent case. The per-package loop then iterates zero files, sideEffectFailures stays 0, and the gate prints "0 source file(s) side-effect-free OK" and goes green (wired at ci.yml:23). A package with a real top-level side effect could ship green purely because its src/ could not be read — the exact silent failure the #​93 gate exists to prevent, turned on the gate itself. Asymmetry: the sibling readFileSync (~:199) has NO catch and correctly crashes loud; only the directory-read path fails silent.

Fix: inspect the caught error and only swallow the genuinely-absent case (if err.code === "ENOENT" return out); re-throw or push a failure on everything else. Better: treat a missing src/ as a failure too — a package with no readable sources cannot be asserted side-effect-free.

🟡 MINOR

scripts/lint-pkg.mjs:115 — TEST_*_RE guards match nothing — tests live in tests/, already excluded by the src/** walk

why + fix

Defensive handling for a co-located-spec layout (packages/*/src/**) the current tree rules out; the inline comment calls it belt-and-suspenders for a future package that co-locates specs under src/. Fires on no file today.

Fix: drop the two regexes since src/** already excludes the tests/ siblings, OR keep them as a deliberate, well-commented forward-guard. Low stakes either way.

Action

revise — address MAJORs

[Goosterhof]

⚠️ Concerns

0 blockers · 2 concerns · 2 nits · 1 praise

This promotes the sideEffects: false manifest premise from a Level-4 doctrine note to a Level-1 lint:pkg gate by parsing every packages/*/src/**/*.ts with the TypeScript compiler API and rejecting non-declaration top-level statements. The gate is honest — RED/GREEN proven both directions, no silent no-op (the queue #63 lesson applied), and the package-global file walk is the correct match for a package-global manifest claim. The remaining issue is a gap between the invariant the PR advertises and the invariant it actually enforces, which should be named in the code and the doctrine rather than left implicit.

Concerns

• scripts/lint-pkg.mjs — case ts.SyntaxKind.VariableStatement: return null; — the gate classifies by statement kind, not by whether a permitted statement evaluates at module load, so a top-level const/let/var whose initializer is a side-effecting call passes clean.

  • Trace: classifyTopLevelStatement returns null unconditionally for VariableStatement. So const _ = console.warn('loaded') → permitted; const patched = Object.defineProperty(globalThis, ...) → permitted; const ready = registerGlobals() → permitted — each runs its effect at module-eval, which is exactly what sideEffects: false promises won't happen.
  • The bare ExpressionStatement form (console.warn('loaded') with no binding) is caught; wrap the identical call in const _ = and it slips. An author who sees the gate fire on the unbound form reaches for the bound form as the first workaround.
  • Not a live false-negative: the factory + barrel corpus binds only literals and arrow factories today (a packages/*/src grep finds zero call-initializers), so this is a future-regression hole, not a current miss — hence concern, not blocker.
  • Mismatch worth fixing: the PR body and the CLAUDE.md edit both claim the gate stops "a module-eval console.warn / Object.defineProperty / prototype patch." That holds only for the bare-statement form. The advertised invariant ("module-eval side-effect-free") is stronger than the enforced one ("no side-effecting statement kinds").
  • Fix — option A (pragmatic): narrow the claim. Reword §B / CLAUDE.md to "rejects side-effecting top-level statement forms (bare expression statements, specifier-less imports, top-level control flow); initializer-expression side effects in const/let/var rest on the factory-pattern convention + review." An honest narrower claim beats an overclaiming gate.
  • Fix — option B (stronger guarantee): flag a VariableStatement whose declaration initializer is a CallExpression/NewExpression that isn't a recognized pure shape. Harder — you'd need an allowlist for legitimate evaluated-but-pure initializers (e.g. new WeakMap()), which you don't have today, so this risks false-positives without that scaffolding.
  • Test: add a RED case for export const x = effectfulCall(); to the proof set. Under A it documents the boundary; under B it must fail the gate.

• scripts/lint-pkg.mjs — listSourceFiles walks *.ts only, skipping .tsx — the manifest flag is package-global; the gate's file selection isn't.

  • No .tsx exists today (find packages/*/src -name '*.tsx' is empty), so this is latent, not active — these are JSX-free frontend-service packages.
  • A future package shipping a .tsx source would ship every top-level statement unguarded with zero gate signal. Either widen the glob to include .tsx now (cheap, zero current cost) or note the .ts-only scope explicitly in the §B comment so the next .tsx author knows the gate doesn't reach them.

Nits

• scripts/lint-pkg.mjs — the comment above the ExportAssignment arm says "Only a function- or class-expression default is side-effect-free," but the code also (correctly) permits ts.SyntaxKind.ArrowFunction — export default () => {} is inert. Align the comment to the actual permit list.

• scripts/lint-pkg.mjs — the default: branch prints the raw enum name (top-level ForStatement, top-level IfStatement). Fine for a CI failure, but For/If/Try/While are common enough to deserve the same human phrasing the expression-statement branch gets ("top-level control-flow statement"), so an ally reading the failure needn't know TS AST kind names.

Praise

• The package-global file walk over re-export-graph tracing (§B). sideEffects: false is a package-global claim, so a side effect in a merely-imported (not re-exported) module is still covered by the bundler's drop-this-module assumption — a re-export-graph trace would miss exactly that case. The §B rationale defends the wider scope correctly, and it's the simpler implementation too.

Automated war-room agent review — posted because this PR carries the Agent Review Requested label.