Skip to content

test(guard): widen the client-bundle guard to every "use client" entry point (#10692) - #10700

Merged
diegosouzapw merged 1 commit into
release/v3.8.50from
fix/10692-widen-client-guard
Aug 19, 2026
Merged

diegosouzapw merged 1 commit into
release/v3.8.50from
fix/10692-widen-client-guard

Conversation

@diegosouzapw

Copy link
Copy Markdown
Owner

Refs #10692. Follow-up to #10695, which fixed the leak; this makes the invariant checkable
everywhere instead of at two hand-picked entry points.

What changed in the guard

It now walks the static import graph from every "use client" file in src/ (753 of them)
plus the two modules #10695 pinned by hand. Full sweep: ~750ms.

Two exclusions, both load-bearing:

What the widened sweep found

Five value-form imports across three files under dashboard/batch, each reaching
db/core → the SQLite driver:

File Import
batch/page.tsx { FileRecord }, { BatchRecord }
batch/files/page.tsx { FileRecord }, { BatchRecord }
batch/batch-utils.ts { BatchRecord }, { FileRecord }

All five bind only interfaces (BatchRecord, FileRecord) used in type position, so the
compiler was eliding them and the build stayed green. That is precisely the shape #10692 had
before #10647 removed the toolchain's tolerance: one value-carrying binding added to any of
those import {} clauses and the SQLite driver lands in the browser bundle again.

Marking them import type makes the elision explicit rather than incidental — no runtime
change.

Worth noting for anyone reading the earlier investigation: my report said 2 findings in 2
files
. That undercounted — the graph walk reports only the first chain per entry point, so
fixing one import exposed the next. The real number was 5 across 3 files, each found only after
the previous was fixed.

Validation

The widened guard was run before the fixes and failed naming the exact chains; it passes
after. typecheck:core clean, eslint clean, and the sibling #10674 import guard still green.

Not done here

verbatimModuleSyntax / the consistent-type-imports ESLint rule would make this class
impossible by construction rather than by vigilance. That is a repo-wide change and deserves its
own evaluation — flagged, not attempted.

…y point (#10692)

The guard shipped with #10695 watched two hand-picked modules. It now walks the static
import graph from all 753 "use client" files in src/ (plus the two originally pinned
entries), so the invariant is verified across the repo instead of where someone
remembered to look. Full sweep runs in ~750ms.

Two exclusions make that practical:

- `import type` is not an edge — TypeScript erases it before the bundler sees it.
  Counting type imports turns 3 real findings into 29; a guard that cries wolf gets
  switched off.
- Dynamic `import()` is still not followed. It does not break a bundle edge (that was
  tried for #10692 and failed) but it does move the module into a chunk the browser
  fetches on demand, which is a legitimate boundary.

The widened sweep immediately found what the narrow one could not: five value-form
imports of `db/batches` / `db/files` across three files under dashboard/batch, each
reaching db/core → the SQLite driver. All five bind only interfaces (BatchRecord,
FileRecord) used in type position, so the compiler was eliding them and the build stayed
green — the same latent shape as #10692 before #10647 removed the toolchain's tolerance.
Marking them `import type` makes the elision explicit instead of incidental.

Refs #10692
@jonlwheat2-gif

Copy link
Copy Markdown
Contributor

Found while validating on Windows: this guard is a silent no-op there

SERVER_ONLY (lines 33–39) is written with forward slashes (src/lib/db/core.ts), but on Windows path.relative() returns backslash separators (src\lib\db\core.ts). So SERVER_ONLY.has(resolved) at line 143 never matched, findServerOnlyPath() always returned null, and the test passed unconditionally. On Windows this guard could not catch a server-only module reaching a client bundle — and it could not have caught the original #10692 leak either (the #10695 guard has the same line).

Proof it was broken: with the dashboard/batch imports reverted to value form (import { BatchRecord }), the unpatched guard still passed on Windows. After the fix it fails, naming the chains:

src/app/(dashboard)/dashboard/batch/files/page.tsx
  → src/lib/db/files.ts
  → src/lib/db/core.ts
src/app/(dashboard)/dashboard/batch/page.tsx
  → src/lib/db/files.ts
  → src/lib/db/core.ts

The fix — tests/unit/client-bundle-no-server-only-10692.test.ts, 2 changes

Change 1 — resolveSpecifier(), the return inside the candidate loop (was line 79, now 79–82):

   for (const candidate of candidates) {
     if (fs.existsSync(candidate) && fs.statSync(candidate).isFile()) {
-      return path.relative(REPO_ROOT, candidate);
+      // Normalize to forward slashes: `SERVER_ONLY` is written with `/`, but on Windows
+      // `path.relative` returns `\` separators — an unnormalized match would silently
+      // pass there, turning this guard into a no-op on Windows.
+      return path.relative(REPO_ROOT, candidate).replace(/\/g, "/");
     }
   }

Change 2 — walk(), the entry accumulation (line 161):

     } else if (/\.tsx?$/.test(entry.name)) {
-      acc.push(path.relative(REPO_ROOT, full));
+      acc.push(path.relative(REPO_ROOT, full).replace(/\/g, "/"));
     }

Change 2 keeps the entry paths from clientEntryPoints() in the same normalized form, so the BFS seen set and the edgesOf() cache keys never mix \// spellings of the same file.

Validation (Windows, Node 24)

  • Value imports restored → guard fails, naming the two chains above
  • import type fixes in place → guard passes (3.1s for the 752-entry sweep)

On the branch

Pushed to fix/10692-widen-client-guard as 78454e5e5 — fix(guard): normalize resolved paths so the client-bundle guard works on Windows (#10692) — on top of 7a6c90e.

@diegosouzapw

Copy link
Copy Markdown
Owner Author

The 7 red checks are inherited base-red, not this PR — proven, not assumed.

Both this PR and # fail on the exact same 7 jobs despite having no file in common. Reproduced on the pure base tip d7368e243d in a disposable worktree with no commit from any branch of mine: tests 26 / pass 10 / fail 16.

The failures are in areas this diff does not touch: Antigravity cloudcode envelope + public models, GLM provider import (4 tests), free-model catalog qwen-web ids, i18n key drift (#6695), /api/dead, open-sse/executors/index.ts TS2345, geminiWeb.ts file size, and a Qwen Code JSON config parse.

Tracked under #9985.

@diegosouzapw
diegosouzapw merged commit b754e44 into release/v3.8.50 Aug 19, 2026
11 checks passed
@diegosouzapw
diegosouzapw deleted the fix/10692-widen-client-guard branch August 19, 2026 00:51
@jonlwheat2-gif

jonlwheat2-gif commented Aug 19, 2026 •

Copy link
Copy Markdown
Contributor

Required follow-up: the merged guard is a no-op on Windows — 2 line-item changes

The widened guard merged via this PR walks the import graph correctly on POSIX, but on Windows path.relative() returns backslash separators (src\lib\db\core.ts) while SERVER_ONLY (line 33) holds forward slashes (src/lib/db/core.ts). The membership check at line 143 (SERVER_ONLY.has(resolved)) therefore never matches, findServerOnlyPath() always returns null, and the test passes unconditionally. Verified on Windows (Node 24): with the #10692 leak shape restored (dashboard/batch value imports → db/core → SQLite driver), the merged guard reports ✅ green; the same tree on Linux reports ❌ red.

All changes are in tests/unit/client-bundle-no-server-only-10692.test.ts (line numbers = current merged file):


ITEM 1 — resolveSpecifier(), line 79 (inside the for (const candidate ...) loop).

Before (line 79):

      return path.relative(REPO_ROOT, candidate);

After (lines 79–82):

      // Normalize to forward slashes: `SERVER_ONLY` is written with `/`, but on Windows
      // `path.relative` returns `\` separators — an unnormalized match would silently
      // pass there, turning this guard into a no-op on Windows.
      return path.relative(REPO_ROOT, candidate).replace(/\/g, "/");

ITEM 2 — walk(), line 158 (the acc.push(...) for .ts/.tsx files).

Before (line 158):

      acc.push(path.relative(REPO_ROOT, full));

After (line 161 — line 158 plus the 3 comment lines added by ITEM 1):

      acc.push(path.relative(REPO_ROOT, full).replace(/\/g, "/"));

Keep both: ITEM 1 fixes the leak detection; ITEM 2 keeps clientEntryPoints() keys in the same normalized form so the BFS seen set and the edgesOf() cache never mix \// spellings of the same file.


Verification required (both platforms):

  1. Windows: leak shape restored → test must FAIL naming files/page.tsx → files.ts → core.ts and page.tsx → files.ts → core.ts; healthy tree → PASS.
  2. Linux: identical results (the .replace() is a no-op on POSIX, so behavior is unchanged — confirmed in a node:24 container).

Ready-to-merge commit on jonlwheat2-gif/OmniRoute:fix/10692-widen-client-guard → 78454e5e5. Refs #10692.

HouMinXi pushed a commit to HouMinXi/OmniRoute that referenced this pull request Aug 20, 2026
…y point (diegosouzapw#10692) (diegosouzapw#10700)

The guard shipped with diegosouzapw#10695 watched two hand-picked modules. It now walks the static
import graph from all 753 "use client" files in src/ (plus the two originally pinned
entries), so the invariant is verified across the repo instead of where someone
remembered to look. Full sweep runs in ~750ms.

Two exclusions make that practical:

- `import type` is not an edge — TypeScript erases it before the bundler sees it.
  Counting type imports turns 3 real findings into 29; a guard that cries wolf gets
  switched off.
- Dynamic `import()` is still not followed. It does not break a bundle edge (that was
  tried for diegosouzapw#10692 and failed) but it does move the module into a chunk the browser
  fetches on demand, which is a legitimate boundary.

The widened sweep immediately found what the narrow one could not: five value-form
imports of `db/batches` / `db/files` across three files under dashboard/batch, each
reaching db/core → the SQLite driver. All five bind only interfaces (BatchRecord,
FileRecord) used in type position, so the compiler was eliding them and the build stayed
green — the same latent shape as diegosouzapw#10692 before diegosouzapw#10647 removed the toolchain's tolerance.
Marking them `import type` makes the elision explicit instead of incidental.

Refs diegosouzapw#10692

Co-authored-by: Xiangzhe <bakryun0718@proton.me>
giauphan pushed a commit to giauphan/OmniRoute that referenced this pull request Aug 20, 2026
…y point (diegosouzapw#10692) (diegosouzapw#10700)

The guard shipped with diegosouzapw#10695 watched two hand-picked modules. It now walks the static
import graph from all 753 "use client" files in src/ (plus the two originally pinned
entries), so the invariant is verified across the repo instead of where someone
remembered to look. Full sweep runs in ~750ms.

Two exclusions make that practical:

- `import type` is not an edge — TypeScript erases it before the bundler sees it.
  Counting type imports turns 3 real findings into 29; a guard that cries wolf gets
  switched off.
- Dynamic `import()` is still not followed. It does not break a bundle edge (that was
  tried for diegosouzapw#10692 and failed) but it does move the module into a chunk the browser
  fetches on demand, which is a legitimate boundary.

The widened sweep immediately found what the narrow one could not: five value-form
imports of `db/batches` / `db/files` across three files under dashboard/batch, each
reaching db/core → the SQLite driver. All five bind only interfaces (BatchRecord,
FileRecord) used in type position, so the compiler was eliding them and the build stayed
green — the same latent shape as diegosouzapw#10692 before diegosouzapw#10647 removed the toolchain's tolerance.
Marking them `import type` makes the elision explicit instead of incidental.

Refs diegosouzapw#10692

Co-authored-by: Xiangzhe <bakryun0718@proton.me>
muhamadgalihsaputra pushed a commit to niyatna/NiyatnaRoute that referenced this pull request Sep 27, 2026
…y point (diegosouzapw#10692) (diegosouzapw#10700)

The guard shipped with diegosouzapw#10695 watched two hand-picked modules. It now walks the static
import graph from all 753 "use client" files in src/ (plus the two originally pinned
entries), so the invariant is verified across the repo instead of where someone
remembered to look. Full sweep runs in ~750ms.

Two exclusions make that practical:

- `import type` is not an edge — TypeScript erases it before the bundler sees it.
  Counting type imports turns 3 real findings into 29; a guard that cries wolf gets
  switched off.
- Dynamic `import()` is still not followed. It does not break a bundle edge (that was
  tried for diegosouzapw#10692 and failed) but it does move the module into a chunk the browser
  fetches on demand, which is a legitimate boundary.

The widened sweep immediately found what the narrow one could not: five value-form
imports of `db/batches` / `db/files` across three files under dashboard/batch, each
reaching db/core → the SQLite driver. All five bind only interfaces (BatchRecord,
FileRecord) used in type position, so the compiler was eliding them and the build stayed
green — the same latent shape as diegosouzapw#10692 before diegosouzapw#10647 removed the toolchain's tolerance.
Marking them `import type` makes the elision explicit instead of incidental.

Refs diegosouzapw#10692

Co-authored-by: Xiangzhe <bakryun0718@proton.me>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants