fix: close three gaps opened by shipping the boundary boot set

Making the 403 / 401 / 404 boundaries render their layout chain also made
their modules ship, and three things that were harmless while they never
shipped are not harmless now.

webjs check's always-ship set listed errors, loadings and the not-found
pages but not forbidden, unauthorized or global-not-found. Those reach the
browser now, so a forbidden.ts importing a server-only module is the exact
throw-at-load crash the rule exists to catch, and it passed a green check.

instrumentation-client was missing from every boundary boot set, so the
error and 403 pages were the only pages running app modules without the
app's client error reporting installed, which is where it matters most.

A layout throwing while wrapping a 403 / 401 / 404 was swallowed by a bare
catch. Those paths execute layout modules for the first time, so a real
crash degraded to a chrome-less page with nothing in dev saying why and
nothing reaching the APM sink. It reports through the same sinks a page
render error does, then degrades as before.

Author: vivek7405

packages/server/src/check/runner-support.js

@@ -159,13 +159,30 @@ export async function checkServerImportInBrowserModule(appDir, violations) {
   for (const page of routeTable.pages || []) {
     for (const f of page.errors || []) alwaysShipRouteModules.set(f, 'error boundary');
     for (const f of page.loadings || []) alwaysShipRouteModules.set(f, 'loading boundary');
+    // forbidden / unauthorized joined this set with #1298. They used to render
+    // with an empty boot set, so their modules never reached the browser and
+    // omitting them here was harmless. They now render inside their layout
+    // chain AND ship, so a `forbidden.ts` doing `import { auth } from
+    // '#lib/auth.server.ts'` (a natural thing on a 403: "signed in as X, but
+    // not permitted") is a real throw-at-load crash that takes every sibling
+    // registration in the same module script with it.
+    for (const f of page.forbiddens || []) alwaysShipRouteModules.set(f, 'forbidden boundary');
+    for (const f of page.unauthorizeds || []) alwaysShipRouteModules.set(f, 'unauthorized boundary');
   }
   if (routeTable.notFound) alwaysShipRouteModules.set(routeTable.notFound, 'not-found page');
   if (routeTable.notFounds) {
     for (const f of routeTable.notFounds.values()) {
       alwaysShipRouteModules.set(f, 'not-found page');
     }
   }
+  // global-not-found ships for the same reason: a thrown notFound() with no
+  // nearer boundary renders it against the matched route, so it wraps and
+  // boots. global-ERROR is deliberately absent: it returns its own document
+  // verbatim with no importmap and no boot script, so nothing of it ever
+  // reaches the browser.
+  if (routeTable.globalNotFound) {
+    alwaysShipRouteModules.set(routeTable.globalNotFound, 'global-not-found page');
+  }
 
   // The elision flag mirrors `dev.js`: respect `webjs.elide === false` and the
   // WEBJS_ELIDE override. When elision is OFF, the build ships EVERY component

packages/server/src/ssr/render.js

@@ -414,6 +414,25 @@ async function renderChain(route, ctx, dev, suspenseCtx, have, pageModule) {
   return { html: body + (await loadingTemplates(route, ctx, dev)), reduced: chain.reduced };
 }
 
+/**
+ * Report a throw from a layout wrapped around a 403 / 401 / 404 boundary
+ * (#1298) to the same sinks a page-render error reaches, then let the caller
+ * degrade to the standalone render. Best-effort on both sinks: a throwing sink
+ * must never affect the response.
+ *
+ * @param {unknown} err
+ * @param {{ onError?: (e: unknown) => void, onDevError?: (e: unknown) => void }} opts
+ */
+function reportBoundaryLayoutError(err, opts) {
+  if (typeof opts.onError === 'function') {
+    try { opts.onError(err); } catch { /* a throwing sink must not affect the response */ }
+  }
+  if (typeof opts.onDevError === 'function') {
+    try { opts.onDevError(err); } catch { /* a throwing sink must not affect the response */ }
+  }
+  console.error('[webjs] a layout threw while wrapping a boundary page:', err);
+}
+
 /**
  * The boot module set for a boundary response: what actually RENDERED, the
  * boundary module plus the layouts wrapping it (#1298).
@@ -434,7 +453,8 @@ async function renderChain(route, ctx, dev, suspenseCtx, have, pageModule) {
  * @param {string} boundaryFile
  * @param {string[]} wrapLayouts
  * @param {{ appDir: string, inertRouteModules?: Set<string>,
- *   importOnlyRouteModules?: Map<string, string[]> }} opts
+ *   importOnlyRouteModules?: Map<string, string[]>,
+ *   instrumentationClient?: string }} opts
  * @returns {string[]}
  */
 function boundaryModuleUrls(boundaryFile, wrapLayouts, opts) {
@@ -451,6 +471,18 @@ function boundaryModuleUrls(boundaryFile, wrapLayouts, opts) {
     if (emit) emit.forEach(push);
     else push(f);
   }
+  // instrumentation-client.{js,ts} (#848) rides the SAME first-import contract
+  // it has on the happy path: it runs before app modules so the app's client
+  // error reporting is installed before anything can throw. A boundary page is
+  // where that matters most, so it must not be the one place it is missing.
+  // Only when the set is non-empty: an empty set means no boot script at all,
+  // and instrumentation alone is not a reason to start emitting one.
+  if (opts.instrumentationClient && urls.length) {
+    const u = toUrlPath(opts.instrumentationClient, opts.appDir);
+    const i = urls.indexOf(u);
+    if (i !== -1) urls.splice(i, 1);
+    urls.unshift(u);
+  }
   return urls;
 }
 
@@ -512,9 +544,14 @@ async function ssrBoundaryHtml(file, heading, opts) {
           } else {
             body = await renderToString(tree, { ssr: true, dev: opts.dev });
           }
-        } catch {
+        } catch (layoutErr) {
           // A wrapped layout threw. Degrade to the standalone render this has
-          // always produced, and to its empty boot set with it.
+          // always produced, and to its empty boot set with it. REPORT it
+          // first: these paths execute layout modules for the first time since
+          // #1298, so a genuine layout crash would otherwise vanish, leaving a
+          // developer looking at a chrome-less boundary page with nothing
+          // saying why, and an APM sink that never heard about it.
+          reportBoundaryLayoutError(layoutErr, opts);
           body = await renderToString(tree, { ssr: true, dev: opts.dev });
           moduleUrls = [];
         }
@@ -564,7 +601,9 @@ async function ssrNotFoundHtml(notFoundFile, opts) {
           } else {
             body = await renderToString(tree, { ssr: true, dev: opts.dev });
           }
-        } catch {
+        } catch (layoutErr) {
+          // Same degradation, and the same reporting, as ssrBoundaryHtml above.
+          reportBoundaryLayoutError(layoutErr, opts);
           body = await renderToString(tree, { ssr: true, dev: opts.dev });
           moduleUrls = [];
         }

packages/server/test/check/no-server-import-in-browser-module.test.js

@@ -378,6 +378,76 @@ export default async function NotFound() {
   }
 });
 
+// forbidden.ts / unauthorized.ts / global-not-found.ts joined the shipping set
+// with #1298: they render inside their layout chain now, and ship with it. The
+// rule has to see them, or the exact #963 crash it exists to catch (a
+// throw-at-load stub taking every sibling registration with it) passes a green
+// `webjs check`. A 403 that greets the signed-in user by name is the natural
+// shape, so this is not a hypothetical import.
+test('a forbidden boundary importing a server module IS flagged (#1298)', async () => {
+  const appDir = await makeApp({
+    'lib/auth.server.ts': AUTH_SERVER,
+    'app/admin/page.ts': `export default function Admin() { return '<h1>admin</h1>'; }\n`,
+    'app/admin/forbidden.ts': `import { auth } from '../../lib/auth.server.ts';
+export default async function Forbidden() {
+  const session = await auth();
+  return \`<h1>No access for \${session.user ?? 'guest'}</h1>\`;
+}
+`,
+  });
+  try {
+    const violations = await checkConventions(appDir);
+    const hits = find(violations, 'forbidden.ts');
+    assert.equal(hits.length, 1, 'a forbidden boundary that ships and imports a server module must be flagged');
+    assert.ok(hits[0].message.includes('auth.server.ts'), 'names the offending server import');
+    assert.ok(/forbidden boundary/.test(hits[0].message), 'identifies it as a forbidden boundary');
+  } finally {
+    await rm(appDir, { recursive: true, force: true });
+  }
+});
+
+test('an unauthorized boundary importing a server module IS flagged (#1298)', async () => {
+  const appDir = await makeApp({
+    'lib/auth.server.ts': AUTH_SERVER,
+    'app/private/page.ts': `export default function P() { return '<h1>private</h1>'; }\n`,
+    'app/private/unauthorized.ts': `import { auth } from '../../lib/auth.server.ts';
+export default async function Unauthorized() {
+  const session = await auth();
+  return \`<h1>Sign in (\${session.user ?? 'guest'})</h1>\`;
+}
+`,
+  });
+  try {
+    const violations = await checkConventions(appDir);
+    const hits = find(violations, 'unauthorized.ts');
+    assert.equal(hits.length, 1, 'an unauthorized boundary that ships and imports a server module must be flagged');
+    assert.ok(/unauthorized boundary/.test(hits[0].message), 'identifies it as an unauthorized boundary');
+  } finally {
+    await rm(appDir, { recursive: true, force: true });
+  }
+});
+
+test('a global-not-found page importing a server module IS flagged (#1298)', async () => {
+  const appDir = await makeApp({
+    'lib/auth.server.ts': AUTH_SERVER,
+    'app/page.ts': `export default function Home() { return '<h1>home</h1>'; }\n`,
+    'app/global-not-found.ts': `import { auth } from '../lib/auth.server.ts';
+export default async function GlobalNotFound() {
+  const session = await auth();
+  return \`<h1>404 for \${session.user ?? 'guest'}</h1>\`;
+}
+`,
+  });
+  try {
+    const violations = await checkConventions(appDir);
+    const hits = find(violations, 'global-not-found.ts');
+    assert.equal(hits.length, 1, 'a global-not-found page that ships and imports a server module must be flagged');
+    assert.ok(/global-not-found page/.test(hits[0].message), 'identifies it as a global-not-found page');
+  } finally {
+    await rm(appDir, { recursive: true, force: true });
+  }
+});
+
 test('a loading boundary importing a server module IS flagged', async () => {
   const appDir = await makeApp({
     'lib/auth.server.ts': AUTH_SERVER,

test/ssr/ssr.test.js

@@ -2083,6 +2083,50 @@ test('boundary: a 404 for a URL that matched no route stays a bare document', as
   assert.ok(!body.includes('<script type="module">'), 'and no boot script, since no chain rendered');
 });
 
+test('boundary: instrumentation-client boots FIRST on a boundary page too (#848)', async () => {
+  // The boundary page is where the app's client error reporting matters most,
+  // so it must not be the one page that runs app modules without it.
+  const { route, appDir } = crashApp({
+    files: { 'instrumentation-client.js': `console.log('boot');\n` },
+  });
+  const resp = await SILENT(() => ssrPage(route, {}, new URL('http://localhost/docs/crash'), {
+    dev: false, appDir, instrumentationClient: join(appDir, 'instrumentation-client.js'),
+  }));
+  const body = await resp.text();
+  const boot = body.match(/<script type="module">([\s\S]*?)<\/script>/);
+  assert.ok(boot, 'the boundary page has a boot script');
+  const imports = [...boot[1].matchAll(/import\s+"([^"]+)"/g)].map((m) => m[1]);
+  assert.equal(imports[0], '/instrumentation-client.js', 'and instrumentation is its FIRST import');
+  assert.ok(imports.includes('/docs/error.js'), 'the boundary module still boots');
+});
+
+test('boundary: a layout that throws while wrapping a 403 is REPORTED, not swallowed', async () => {
+  // These paths execute layout modules for the first time since #1298, so a
+  // genuine layout crash would otherwise vanish: a chrome-less boundary page
+  // with nothing saying why, and an APM sink that never heard about it.
+  const { route, appDir } = makeBoundaryApp({
+    files: {
+      'layout.js': `export default function Root() { throw new Error('layout-boom-403'); }\n`,
+      'admin/forbidden.js': HTML_IMPORT + `export default function F() { return html\`<p id="fb">no</p>\`; }\n`,
+      'admin/page.js': `import { forbidden } from ${JSON.stringify(WEBJS_MODULE_URL)};\nexport default function Page() { forbidden(); }\n`,
+    },
+    page: 'admin/page.js',
+    layouts: ['layout.js'],
+    forbiddens: ['admin/forbidden.js'],
+  });
+  const seen = [];
+  const resp = await SILENT(() => ssrPage(route, {}, new URL('http://localhost/admin'), {
+    dev: false, appDir, onError: (e) => seen.push(e),
+  }));
+  assert.equal(resp.status, 403, 'it still degrades to the standalone 403 rather than failing');
+  const body = await resp.text();
+  assert.ok(body.includes('id="fb"'), 'the boundary itself still rendered');
+  assert.equal(markersOf(body).length, 0, 'with no markers, since the chain could not render');
+  assert.ok(!body.includes('<script type="module">'), 'and no boot set for a chain that did not render');
+  assert.equal(seen.length, 1, 'the layout throw reached the onError sink');
+  assert.match(String(seen[0].message), /layout-boom-403/);
+});
+
 test('boundary: a boundary response is never storable and never reduced', async () => {
   // Two independent guarantees. The HTML cache refuses a non-200 outright, and
   // the reduced X-Webjs-Have path is structurally unreachable: the boundary