From 9c6ea59e069ae333248729668d1a321f943d7840 Mon Sep 17 00:00:00 2001 From: Vivek Date: Tue, 28 Jul 2026 16:41:07 +0530 Subject: [PATCH 01/16] chore: open work on #1154 + #1155 From 9dafdf8bd0dc4b36b2e599fc4039953a74745366 Mon Sep 17 00:00:00 2001 From: Vivek Date: Tue, 28 Jul 2026 16:47:02 +0530 Subject: [PATCH 02/16] fix: refuse a function in form action=, it leaked server source At SSR a `'use server'` import resolves to the REAL function (the RPC stub exists only in the browser), and `action=` is an ordinary escaped attribute hole, so the renderer stringified the function straight into the served HTML. A page doing what every Next-trained author writes, `
`, published the action's whole body, including any connection string or internal path its closure showed, to every visitor. Both hole shapes leaked identically (`action=${fn}` and `action="${fn}"`), as did a mixed value and `formaction=` on a submit button. Refuse it instead, on the server and on the client, so a client re-render cannot write the source into the live DOM either. The guard is name-based and deliberately narrow: only `action` and `formaction`, where a stringified function is never useful on any tag. Every other attribute keeps its existing behaviour, and a string-valued action is byte-identical to before. The thrown message names the fix without echoing the source. --- .../webjs/references/muscle-memory-gotchas.md | 17 ++++ packages/core/src/form-action.js | 44 ++++++++++ packages/core/src/render-client.js | 14 +++- packages/core/src/render-server.js | 7 ++ .../form-action-attr-guard-client.test.js | 51 ++++++++++++ .../rendering/form-action-attr-guard.test.js | 80 +++++++++++++++++++ 6 files changed, 211 insertions(+), 2 deletions(-) create mode 100644 packages/core/src/form-action.js create mode 100644 packages/core/test/rendering/form-action-attr-guard-client.test.js create mode 100644 packages/core/test/rendering/form-action-attr-guard.test.js diff --git a/.agents/skills/webjs/references/muscle-memory-gotchas.md b/.agents/skills/webjs/references/muscle-memory-gotchas.md index bb1cf6f2e..2d11d8593 100644 --- a/.agents/skills/webjs/references/muscle-memory-gotchas.md +++ b/.agents/skills/webjs/references/muscle-memory-gotchas.md @@ -43,6 +43,23 @@ const users = await getUsers(); There is no React `cache()`, `use()`, or `unstable_cache`. Caching is the `cache()` query helper, `export const revalidate` on a page, or `export const cache` on a GET action. +### A plain function in `` is refused, not stringified + +Next binds a Server Action with ``, and React serializes the binding into hidden fields. WebJs reads the same shape, but only for a real `'use server'` action. Any other function is a hard render error. + +The reason is a source leak. During SSR a `.server.ts` import is the ACTUAL function (the RPC stub exists only in the browser), and `action=` is an ordinary attribute hole, so stringifying it would write the function's body, secrets included, into the HTML every visitor downloads. So the renderer throws instead, on the server and on the client, for `action=` and `formaction=` alike, in every hole shape (`action=${fn}`, `action="${fn}"`, `action="/x/${fn}"`). + +```ts +// WRONG: a local handler. Throws at render; it would have leaked its body. +const onSubmit = async (fd: FormData) => { /* ... */ }; +html``; +// RIGHT: a 'use server' action imported from a *.server.ts module. +import { submitFeedback } from '#modules/feedback/actions/submit-feedback.server.ts'; +html``; +``` + +A string stays a string: `action="/search"` and `action=${'/search'}` are unchanged. Other attributes keep their existing stringify behaviour; only `action` and `formaction` are claimed. + ### `params` and `searchParams` are awaitable AND synchronously readable Next 15/16 made `params` / `searchParams` Promises. WebJs supports BOTH, so either muscle memory is correct. diff --git a/packages/core/src/form-action.js b/packages/core/src/form-action.js new file mode 100644 index 000000000..fdbe77ec1 --- /dev/null +++ b/packages/core/src/form-action.js @@ -0,0 +1,44 @@ +/** + * Form-action binding shared bits (#1154, #1155). + * + * Both renderers (server + client) import from here so the guard cannot + * drift between SSR and hydration. + */ + +/** + * The hidden field that carries a form-bound action's identity + * (`/`) from the SSR'd markup to the server dispatcher (#1155). + * Reserved name; user form fields must not use it. + */ +export const ACTION_FIELD = '__webjs_action'; + +/** + * Refuse to stringify a function interpolated into a form-action attribute + * (#1154). At SSR a `'use server'` import is the REAL server function (the + * RPC stub only exists in the browser), and a plain attribute hole commits + * via `String(val)`, so without this guard the action's SOURCE, secrets + * included, is serialized into the served HTML. The client renderer guards + * the same commit so a client re-render cannot write the source into the + * live DOM either. + * + * Name-based on purpose (`action`, plus `formaction`, the submit-button + * override, which leaks identically): a function stringified under these + * names is never useful on any tag, and other attributes keep today's + * stringify behaviour so the claim stays narrow. + * + * @param {unknown} val the hole's resolved value + * @param {string} attrName the attribute being committed (any case) + * @param {string} [tag] lowercased owner tag, for the error message + */ +export function assertNotFunctionActionAttr(val, attrName, tag) { + if (typeof val !== 'function') return; + const name = String(attrName).toLowerCase(); + if (name !== 'action' && name !== 'formaction') return; + throw new Error( + `[webjs] a function was interpolated into ${name}= on <${tag || 'form'}>. ` + + `Stringifying a function into HTML would leak its source (a 'use server' ` + + `action's body included) to every visitor, so this is refused. ` + + `Bind a 'use server' action exported from a *.server.{js,ts} module, ` + + `or pass a URL string.` + ); +} diff --git a/packages/core/src/render-client.js b/packages/core/src/render-client.js index 7065d8a1c..e7a9b7ac2 100644 --- a/packages/core/src/render-client.js +++ b/packages/core/src/render-client.js @@ -1,6 +1,7 @@ import { isTemplate, MARKER } from './html.js'; import { BINDING_PREFIXES, isBindingPrefix } from './binding-prefixes.js'; import { escapeAttr } from './escape.js'; +import { assertNotFunctionActionAttr } from './form-action.js'; import { isRepeat } from './repeat.js'; import { isUnsafeHTML, isLive, isKeyed, isGuard, isTemplateContent, isRef, isCache, isUntil, isAsyncAppend, isAsyncReplace, isWatch } from './directives.js'; import { Signal } from './signal.js'; @@ -671,7 +672,13 @@ function applyPart(part, value, _prev, allValues) { break; case 'attr': { if (value == null || value === false) part.el.removeAttribute(part.name); - else part.el.setAttribute(part.name, String(value)); + else { + // #1154: refuse to stringify a function into action=/formaction= + // (mirrors the SSR guard, so a client re-render cannot write a + // server action's source into the live DOM). + assertNotFunctionActionAttr(value, part.name, part.el.localName); + part.el.setAttribute(part.name, String(value)); + } break; } case 'prop': @@ -692,7 +699,10 @@ function applyPart(part, value, _prev, allValues) { const mp = /** @type {{ statics: string[], group: number[] }} */ (/** @type any */ (part)); let val = mp.statics[0]; for (let j = 0; j < mp.group.length; j++) { - val += String((allValues ? allValues[mp.group[j]] : value) ?? ''); + const piece = allValues ? allValues[mp.group[j]] : value; + // #1154: same function guard for each piece of a mixed attribute. + assertNotFunctionActionAttr(piece, part.name, part.el.localName); + val += String(piece ?? ''); val += mp.statics[j + 1] || ''; } part.el.setAttribute(part.name, val); diff --git a/packages/core/src/render-server.js b/packages/core/src/render-server.js index d4c5e81b5..b41895169 100644 --- a/packages/core/src/render-server.js +++ b/packages/core/src/render-server.js @@ -1,6 +1,7 @@ import { html, isTemplate } from './html.js'; import { BINDING_PREFIXES } from './binding-prefixes.js'; import { escapeText, escapeAttr } from './escape.js'; +import { assertNotFunctionActionAttr } from './form-action.js'; import { lookup, lookupModuleUrl, allTags } from './registry.js'; import { stylesToString, isCSS } from './css.js'; import { isRepeat } from './repeat.js'; @@ -323,11 +324,17 @@ async function renderTemplate(tr, ctx) { state = 'in-tag'; attrName = ''; } else { + // #1154: never stringify a function into action=/formaction= (it + // would serialize a server action's source into the served HTML). + assertNotFunctionActionAttr(val, attrName, currentTag); out += `"${escapeAttr(String(val ?? ''))}"`; state = 'in-tag'; attrName = ''; } } else if (state === 'attr-quoted' || state === 'attr-unquoted') { + // Same guard for a hole inside a quoted/unquoted value, the + // `action="${fn}"` and mixed `action="/x/${fn}"` shapes (#1154). + assertNotFunctionActionAttr(val, attrName, currentTag); out += escapeAttr(String(val ?? '')); } } diff --git a/packages/core/test/rendering/form-action-attr-guard-client.test.js b/packages/core/test/rendering/form-action-attr-guard-client.test.js new file mode 100644 index 000000000..8974e4f78 --- /dev/null +++ b/packages/core/test/rendering/form-action-attr-guard-client.test.js @@ -0,0 +1,51 @@ +// #1154, client half: the browser renderer refuses the same commit, so a +// client re-render can never write a function's source into the live DOM +// (`applyPart`'s 'attr' and 'attr-mixed' cases). Runs under linkedom. +import { test, before } from 'node:test'; +import assert from 'node:assert/strict'; +import { parseHTML } from 'linkedom'; + +before(() => { + const { window } = parseHTML(''); + globalThis.document = window.document; + globalThis.DocumentFragment = window.DocumentFragment; + globalThis.Node = window.Node; + globalThis.Element = window.Element; + globalThis.Comment = window.Comment; + globalThis.Text = window.Text; + globalThis.NodeFilter = window.NodeFilter; + globalThis.HTMLElement = window.HTMLElement; +}); + +let html, render; +before(async () => { + ({ html } = await import('../../src/html.js')); + ({ render } = await import('../../src/render-client.js')); +}); + +async function fakeAction() { const S = 'CLIENT_SECRET'; return S; } + +test('client render of action=${fn} throws, DOM never carries the source', () => { + const host = document.createElement('div'); + assert.throws( + () => render(html``, host), + /function was interpolated into action=/, + ); + assert.ok(!host.innerHTML.includes('CLIENT_SECRET')); +}); + +test('client render of mixed action="/x/${fn}" throws', () => { + const host = document.createElement('div'); + assert.throws( + () => render(html`
`, host), + /function was interpolated into action=/, + ); + assert.ok(!host.innerHTML.includes('CLIENT_SECRET')); +}); + +test('string action still renders on the client', () => { + const host = document.createElement('div'); + render(html`
`, host); + const form = host.querySelector('form'); + assert.equal(form.getAttribute('action'), '/submit'); +}); diff --git a/packages/core/test/rendering/form-action-attr-guard.test.js b/packages/core/test/rendering/form-action-attr-guard.test.js new file mode 100644 index 000000000..7ad7a6643 --- /dev/null +++ b/packages/core/test/rendering/form-action-attr-guard.test.js @@ -0,0 +1,80 @@ +// #1154: a function interpolated into a form-action attribute must throw, +// never stringify. At SSR a `'use server'` import is the REAL function, so +// `String(fn)` would serialize the action's source (secrets included) into +// the served HTML. Covers every hole shape that used to leak (unquoted, +// quoted, mixed, and `formaction` on a submit button), plus the byte-identical +// passthrough for string-valued action attributes. +import { test, before } from 'node:test'; +import assert from 'node:assert/strict'; + +let html, renderToString; +before(async () => { + ({ html } = await import('../../src/html.js')); + ({ renderToString } = await import('../../src/render-server.js')); +}); + +// The secret sentinel must never appear in any output, thrown or not. +const SECRET = 'postgres://user:SECRET@host/db'; +async function leaky(input) { const conn = SECRET; return { success: true, conn, input }; } + +test('unquoted action=${fn} throws instead of leaking source', async () => { + await assert.rejects( + () => renderToString(html`
`, { ssr: true }), + /function was interpolated into action=/, + ); +}); + +test('quoted action="${fn}" throws identically', async () => { + await assert.rejects( + () => renderToString(html`
`, { ssr: true }), + /function was interpolated into action=/, + ); +}); + +test('mixed hole action="/x/${fn}" throws', async () => { + await assert.rejects( + () => renderToString(html`
`, { ssr: true }), + /function was interpolated into action=/, + ); +}); + +test('formaction=${fn} on a submit button throws', async () => { + await assert.rejects( + () => renderToString(html`
`, { ssr: true }), + /function was interpolated into formaction=/, + ); +}); + +test('no thrown message ever carries the function source', async () => { + for (const tpl of [ + html`
`, + html`
`, + ]) { + let msg = ''; + try { await renderToString(tpl, { ssr: true }); } catch (e) { msg = String(e && e.message); } + assert.ok(msg.length > 0, 'render must throw'); + assert.ok(!msg.includes('SECRET'), 'error message must not embed the source'); + } +}); + +test('string-valued action renders byte-identically to before', async () => { + assert.equal( + await renderToString(html`
`, { ssr: true }), + '
', + ); + assert.equal( + await renderToString(html`
`, { ssr: true }), + '
', + ); + assert.equal( + await renderToString(html`
`, { ssr: true }), + '
', + ); +}); + +test('functions in OTHER attributes keep the existing stringify behaviour', async () => { + // Narrow claim: only action/formaction throw. Anything else is unchanged + // (arguably also a bug, but out of #1154's scope by design). + const out = await renderToString(html`
`, { ssr: true }); + assert.ok(out.startsWith('
/` identity + * @param {(name: string) => boolean} has whether the form already set an attribute + * @returns {{ forced: string, field: string }} + */ +export function formActionMarkup(id, has) { + let forced = ''; + if (!has('method')) forced += ' method="post"'; + if (!has('enctype')) forced += ' enctype="multipart/form-data"'; + const field = ``; + return { forced, field }; +} + +/** Minimal attribute escape; the identity is framework-generated, so this is belt and braces. */ +function escapeAttrValue(s) { + return String(s).replace(/&/g, '&').replace(/"/g, '"').replace(/} seenAttrs attribute names already written on this tag + * @returns {{ attrValue: string, forced: string, field: string } | null} + */ +export function commitFormActionAttr(val, attrName, tag, seenAttrs) { + if (typeof val !== 'function') return null; + if (!isFormActionAttr(attrName)) { + // A function on any other attribute keeps the pre-existing stringify + // behaviour: widening the claim here would change unrelated rendering. + return null; + } + const id = resolveFormActionOrThrow(val, attrName, tag); + const { forced, field } = formActionMarkup(id, (n) => seenAttrs.has(n)); + // The form posts to its OWN url; the identity rides the hidden field, which + // is what lets one action be bound from any page (#1155) while the failure + // re-render still targets the page the form was on. + return { attrValue: '""', forced, field }; } diff --git a/packages/core/src/render-server.js b/packages/core/src/render-server.js index b41895169..4659e751b 100644 --- a/packages/core/src/render-server.js +++ b/packages/core/src/render-server.js @@ -1,7 +1,7 @@ import { html, isTemplate } from './html.js'; import { BINDING_PREFIXES } from './binding-prefixes.js'; import { escapeText, escapeAttr } from './escape.js'; -import { assertNotFunctionActionAttr } from './form-action.js'; +import { assertNotFunctionActionAttr, commitFormActionAttr } from './form-action.js'; import { lookup, lookupModuleUrl, allTags } from './registry.js'; import { stylesToString, isCSS } from './css.js'; import { isRepeat } from './repeat.js'; @@ -152,6 +152,13 @@ async function renderTemplate(tr, ctx) { let commentDashes = 0; let currentTag = ''; // lowercased tag name currently being parsed let rawTail = ''; // rolling lowercased tail, tracks / + // Form-action binding (#1155). A `
` commits its + // attribute mid-tag, but the identity field it needs is a CHILD, so the + // markup is buffered here and flushed the moment the open tag closes. + // `seenAttrs` tracks what the author already wrote on the current tag so a + // forced `method` / `enctype` never duplicates an explicit one. + let pendingFormField = ''; + let seenAttrs = new Set(); for (let i = 0; i < strings.length; i++) { const s = strings[i]; @@ -195,6 +202,9 @@ async function renderTemplate(tr, ctx) { case 'in-tag': out += c; if (c === '>') { + // The open tag just closed, so a buffered form-action identity + // field can now be emitted as the form's first child (#1155). + if (pendingFormField) { out += pendingFormField; pendingFormField = ''; } state = isRawtextTag(currentTag) ? 'rawtext' : 'text'; if (state === 'rawtext') rawTail = ''; } else if (!/\s/.test(c) && c !== '/') { @@ -213,20 +223,34 @@ async function renderTemplate(tr, ctx) { } break; case 'attr-name': - if (c === '=') { state = 'after-eq'; out += c; } - else if (/\s/.test(c)) { state = 'in-tag'; attrName = ''; out += c; } - else if (c === '>') { state = 'text'; attrName = ''; out += c; } + // Record every attribute the author wrote on this tag, so a forced + // method/enctype (#1155) never duplicates an explicit one. + if (c === '=') { state = 'after-eq'; seenAttrs.add(attrName.toLowerCase()); out += c; } + else if (/\s/.test(c)) { state = 'in-tag'; seenAttrs.add(attrName.toLowerCase()); attrName = ''; out += c; } + else if (c === '>') { + state = 'text'; + seenAttrs.add(attrName.toLowerCase()); + attrName = ''; + out += c; + if (pendingFormField) { out += pendingFormField; pendingFormField = ''; } + } else { attrName += c; out += c; } break; case 'after-eq': if (c === '"' || c === "'") { state = 'attr-quoted'; attrQuote = c; out += c; } else if (/\s/.test(c)) { state = 'in-tag'; attrName = ''; out += c; } - else if (c === '>') { state = 'text'; attrName = ''; out += c; } + else if (c === '>') { + state = 'text'; attrName = ''; out += c; + if (pendingFormField) { out += pendingFormField; pendingFormField = ''; } + } else { state = 'attr-unquoted'; out += c; } break; case 'attr-unquoted': if (/\s/.test(c)) { state = 'in-tag'; attrName = ''; out += c; } - else if (c === '>') { state = 'text'; attrName = ''; out += c; } + else if (c === '>') { + state = 'text'; attrName = ''; out += c; + if (pendingFormField) { out += pendingFormField; pendingFormField = ''; } + } else out += c; break; case 'attr-quoted': @@ -324,16 +348,22 @@ async function renderTemplate(tr, ctx) { state = 'in-tag'; attrName = ''; } else { - // #1154: never stringify a function into action=/formaction= (it - // would serialize a server action's source into the served HTML). - assertNotFunctionActionAttr(val, attrName, currentTag); - out += `"${escapeAttr(String(val ?? ''))}"`; + // A function bound to action=/formaction= is either an identifiable + // server action (emit the identity field, #1155) or refused + // outright (#1154); it is never stringified. + const bound = commitFormActionAttr(val, attrName, currentTag, seenAttrs); + if (bound) { + out += bound.attrValue + bound.forced; + pendingFormField = bound.field; + } else { + out += `"${escapeAttr(String(val ?? ''))}"`; + } state = 'in-tag'; attrName = ''; } } else if (state === 'attr-quoted' || state === 'attr-unquoted') { - // Same guard for a hole inside a quoted/unquoted value, the - // `action="${fn}"` and mixed `action="/x/${fn}"` shapes (#1154). + // A hole INSIDE a value (`action="${fn}"`, mixed `action="/x/${fn}"`) + // cannot carry an identity field, so a function here is always refused. assertNotFunctionActionAttr(val, attrName, currentTag); out += escapeAttr(String(val ?? '')); } @@ -1775,6 +1805,9 @@ async function streamTemplate(tr, ctx, controller) { let rawTail = ''; // Buffer used for attribute handling where we may need to backtrack. let buf = ''; + // Form-action binding (#1155), mirroring renderTemplate. + let pendingFormField = ''; + let seenAttrs = new Set(); for (let i = 0; i < strings.length; i++) { const s = strings[i]; @@ -1788,8 +1821,8 @@ async function streamTemplate(tr, ctx, controller) { case 'tag-open': buf += c; if (c === '!') state = 'bang-1'; - else if (c === '/') { state = 'tag-name'; currentTag = ''; } - else if (/[a-zA-Z]/.test(c)) { state = 'tag-name'; currentTag = c.toLowerCase(); } + else if (c === '/') { state = 'tag-name'; currentTag = ''; seenAttrs = new Set(); } + else if (/[a-zA-Z]/.test(c)) { state = 'tag-name'; currentTag = c.toLowerCase(); seenAttrs = new Set(); } else state = 'text'; break; case 'bang-1': @@ -1818,6 +1851,7 @@ async function streamTemplate(tr, ctx, controller) { case 'in-tag': buf += c; if (c === '>') { + if (pendingFormField) { buf += pendingFormField; pendingFormField = ''; } state = isRawtextTag(currentTag) ? 'rawtext' : 'text'; if (state === 'rawtext') rawTail = ''; } else if (!/\s/.test(c) && c !== '/') { @@ -1836,20 +1870,29 @@ async function streamTemplate(tr, ctx, controller) { } break; case 'attr-name': - if (c === '=') { state = 'after-eq'; buf += c; } - else if (/\s/.test(c)) { state = 'in-tag'; attrName = ''; buf += c; } - else if (c === '>') { state = 'text'; attrName = ''; buf += c; } + if (c === '=') { state = 'after-eq'; seenAttrs.add(attrName.toLowerCase()); buf += c; } + else if (/\s/.test(c)) { state = 'in-tag'; seenAttrs.add(attrName.toLowerCase()); attrName = ''; buf += c; } + else if (c === '>') { + state = 'text'; seenAttrs.add(attrName.toLowerCase()); attrName = ''; buf += c; + if (pendingFormField) { buf += pendingFormField; pendingFormField = ''; } + } else { attrName += c; buf += c; } break; case 'after-eq': if (c === '"' || c === "'") { state = 'attr-quoted'; attrQuote = c; buf += c; } else if (/\s/.test(c)) { state = 'in-tag'; attrName = ''; buf += c; } - else if (c === '>') { state = 'text'; attrName = ''; buf += c; } + else if (c === '>') { + state = 'text'; attrName = ''; buf += c; + if (pendingFormField) { buf += pendingFormField; pendingFormField = ''; } + } else { state = 'attr-unquoted'; buf += c; } break; case 'attr-unquoted': if (/\s/.test(c)) { state = 'in-tag'; attrName = ''; buf += c; } - else if (c === '>') { state = 'text'; attrName = ''; buf += c; } + else if (c === '>') { + state = 'text'; attrName = ''; buf += c; + if (pendingFormField) { buf += pendingFormField; pendingFormField = ''; } + } else buf += c; break; case 'attr-quoted': @@ -1891,11 +1934,21 @@ async function streamTemplate(tr, ctx, controller) { state = 'in-tag'; attrName = ''; } else { - buf += `"${escapeAttr(String(val ?? ''))}"`; + // Same contract as the buffered renderer (they must not diverge; see + // commitFormActionAttr). Before this, a STREAMED page emitted a bound + // action's source verbatim while the buffered path already refused it. + const bound = commitFormActionAttr(val, attrName, currentTag, seenAttrs); + if (bound) { + buf += bound.attrValue + bound.forced; + pendingFormField = bound.field; + } else { + buf += `"${escapeAttr(String(val ?? ''))}"`; + } state = 'in-tag'; attrName = ''; } } else if (state === 'attr-quoted' || state === 'attr-unquoted') { + assertNotFunctionActionAttr(val, attrName, currentTag); buf += escapeAttr(String(val ?? '')); } } diff --git a/packages/core/test/rendering/form-action-attr-guard.test.js b/packages/core/test/rendering/form-action-attr-guard.test.js index 7ad7a6643..878292a9d 100644 --- a/packages/core/test/rendering/form-action-attr-guard.test.js +++ b/packages/core/test/rendering/form-action-attr-guard.test.js @@ -7,12 +7,18 @@ import { test, before } from 'node:test'; import assert from 'node:assert/strict'; -let html, renderToString; +let html, renderToString, renderToStream; before(async () => { ({ html } = await import('../../src/html.js')); - ({ renderToString } = await import('../../src/render-server.js')); + ({ renderToString, renderToStream } = await import('../../src/render-server.js')); }); +async function drain(stream) { + let out = ''; + for await (const c of stream) out += typeof c === 'string' ? c : new TextDecoder().decode(c); + return out; +} + // The secret sentinel must never appear in any output, thrown or not. const SECRET = 'postgres://user:SECRET@host/db'; async function leaky(input) { const conn = SECRET; return { success: true, conn, input }; } @@ -72,6 +78,30 @@ test('string-valued action renders byte-identically to before', async () => { ); }); +// The STREAMING renderer is a second, independent state machine. It was missed +// by the first pass of this fix and kept emitting the action's whole body while +// the buffered renderer already refused it, so it gets its own coverage rather +// than being assumed to inherit the guard. +test('the streaming renderer refuses the same function (ssr:false path)', async () => { + await assert.rejects( + () => drain(renderToStream(html`
`, { ssr: false })), + /function was interpolated into action=/, + ); +}); + +test('the streaming renderer never emits the source', async () => { + let out = ''; + try { + out = await drain(renderToStream(html`
`, { ssr: false })); + } catch { /* expected */ } + assert.ok(!out.includes('SECRET'), 'streamed output must not carry the function source'); +}); + +test('streaming keeps string-valued actions working', async () => { + const out = await drain(renderToStream(html`
`, { ssr: false })); + assert.match(out, /action="\/submit"/); +}); + test('functions in OTHER attributes keep the existing stringify behaviour', async () => { // Narrow claim: only action/formaction throw. Anything else is unchanged // (arguably also a bug, but out of #1154's scope by design). From ee7d1009dd86d5b5cfa3a11029ec2e2131097f65 Mon Sep 17 00:00:00 2001 From: Vivek Date: Tue, 28 Jul 2026 17:08:12 +0530 Subject: [PATCH 04/16] refactor: drop the unreleased form-binding machinery from this branch The identity resolver, the hidden-field emission and the forced method/enctype belong to the form-submission unification, which is a separate change. They were unreachable here anyway (nothing installs a resolver), and shipping dormant code alongside a security fix makes the fix harder to review and to reason about on its own. What stays is the whole of the leak fix, including the streaming-path guard, so this branch is now exactly the bug fix and is independently mergeable. The removed code is recoverable from cf72805a. --- .../webjs/references/muscle-memory-gotchas.md | 13 +- packages/core/src/form-action.js | 135 +++--------------- packages/core/src/render-server.js | 97 ++++--------- 3 files changed, 47 insertions(+), 198 deletions(-) diff --git a/.agents/skills/webjs/references/muscle-memory-gotchas.md b/.agents/skills/webjs/references/muscle-memory-gotchas.md index 2d11d8593..5a947ea3c 100644 --- a/.agents/skills/webjs/references/muscle-memory-gotchas.md +++ b/.agents/skills/webjs/references/muscle-memory-gotchas.md @@ -43,19 +43,18 @@ const users = await getUsers(); There is no React `cache()`, `use()`, or `unstable_cache`. Caching is the `cache()` query helper, `export const revalidate` on a page, or `export const cache` on a GET action. -### A plain function in `
` is refused, not stringified +### A function in `` is refused, not stringified -Next binds a Server Action with ``, and React serializes the binding into hidden fields. WebJs reads the same shape, but only for a real `'use server'` action. Any other function is a hard render error. +Next binds a Server Action with `` and React serializes the binding into hidden fields. WebJs does not read that shape. A function interpolated into `action=` is a hard render error. -The reason is a source leak. During SSR a `.server.ts` import is the ACTUAL function (the RPC stub exists only in the browser), and `action=` is an ordinary attribute hole, so stringifying it would write the function's body, secrets included, into the HTML every visitor downloads. So the renderer throws instead, on the server and on the client, for `action=` and `formaction=` alike, in every hole shape (`action=${fn}`, `action="${fn}"`, `action="/x/${fn}"`). +The reason is a source leak. During SSR a `.server.ts` import is the ACTUAL function (the RPC stub exists only in the browser), and `action=` is an ordinary attribute hole, so stringifying it would write the function's body, secrets included, into the HTML every visitor downloads. The renderer throws instead, on the server and on the client, for `action=` and `formaction=` alike, in every hole shape (`action=${fn}`, `action="${fn}"`, `action="/x/${fn}"`). ```ts -// WRONG: a local handler. Throws at render; it would have leaked its body. -const onSubmit = async (fd: FormData) => { /* ... */ }; -html``; -// RIGHT: a 'use server' action imported from a *.server.ts module. +// WRONG: throws at render; it would have leaked the action's body. import { submitFeedback } from '#modules/feedback/actions/submit-feedback.server.ts'; html``; +// RIGHT: post to the page's own url and handle it in the page's `action` export. +html``; ``` A string stays a string: `action="/search"` and `action=${'/search'}` are unchanged. Other attributes keep their existing stringify behaviour; only `action` and `formaction` are claimed. diff --git a/packages/core/src/form-action.js b/packages/core/src/form-action.js index 4159bed7b..d56debaf5 100644 --- a/packages/core/src/form-action.js +++ b/packages/core/src/form-action.js @@ -1,25 +1,21 @@ /** - * Form-action binding shared bits (#1154, #1155). + * Form-action attribute guard (#1154). * - * Both renderers (server + client) import from here so the guard cannot - * drift between SSR and hydration. + * Both SSR state machines and the client renderer import from here so the + * rule cannot drift between them. It already drifted once: the guard was + * added to the buffered renderer alone, leaving the Suspense-streaming path + * emitting a server action's whole body into the response. */ /** - * The hidden field that carries a form-bound action's identity - * (`/`) from the SSR'd markup to the server dispatcher (#1155). - * Reserved name; user form fields must not use it. - */ -export const ACTION_FIELD = '__webjs_action'; - -/** - * Refuse to stringify a function interpolated into a form-action attribute - * (#1154). At SSR a `'use server'` import is the REAL server function (the - * RPC stub only exists in the browser), and a plain attribute hole commits - * via `String(val)`, so without this guard the action's SOURCE, secrets - * included, is serialized into the served HTML. The client renderer guards - * the same commit so a client re-render cannot write the source into the - * live DOM either. + * Refuse to stringify a function interpolated into a form-action attribute. + * + * At SSR a `'use server'` import is the REAL server function (the RPC stub + * only exists in the browser), and a plain attribute hole commits via + * `String(val)`, so without this guard the action's SOURCE, secrets included, + * is serialized into the served HTML. The client renderer guards the same + * commit so a client re-render cannot write the source into the live DOM + * either. * * Name-based on purpose (`action`, plus `formaction`, the submit-button * override, which leaks identically): a function stringified under these @@ -50,106 +46,7 @@ export function isFormActionAttr(attrName) { */ function formActionError(attrName, tag) { return `[webjs] a function was interpolated into ${String(attrName).toLowerCase()}= ` - + `on <${tag || 'form'}>, and it is not a 'use server' action. Stringifying a ` - + `function into HTML would leak its source (a server action's body included) ` - + `to every visitor, so this is refused. Bind a 'use server' action exported ` - + `from a *.server.{js,ts} module, or pass a URL string.`; -} - -/** - * Resolves a form-bound action function to its wire identity, `/` - * (#1155). Server-only wiring, installed exactly like the CSP nonce provider: - * `@webjsdev/server` calls `setFormActionResolver` at load time, the browser - * bundle never does, so a client-side render keeps refusing every function. - * - * @type {((fn: Function) => string | null) | null} - */ -let _resolver = null; - -/** - * Internal: server-only wiring. Installed once by `@webjsdev/server`. - * @param {(fn: Function) => string | null} fn - */ -export function setFormActionResolver(fn) { - _resolver = fn; -} - -/** - * Resolve a form-bound action to `/`, or throw the refusal above when - * it is not an identifiable `'use server'` action. - * - * Throwing on an unresolvable function is load-bearing for progressive - * enhancement, not defensive: emitting the form WITHOUT the identity field - * would render a form that posts to the page and dispatches nothing, a silent - * no-op with no error anywhere. Failing the render is the only outcome that - * cannot ship a dead form. - * - * @param {Function} val - * @param {string} attrName - * @param {string} [tag] - * @returns {string} the `/` identity - */ -export function resolveFormActionOrThrow(val, attrName, tag) { - const id = _resolver ? _resolver(val) : null; - if (!id) throw new Error(formActionError(attrName, tag)); - return id; -} - -/** - * The hidden field carrying the action identity, plus the attributes the - * framework forces rather than trusting the author to write. - * - * `method="post"`: a form with no method GETs, so the action would never run. - * `enctype="multipart/form-data"`: without it a no-JS file upload silently - * arrives as a filename string instead of the file. - * - * @param {string} id the `/` identity - * @param {(name: string) => boolean} has whether the form already set an attribute - * @returns {{ forced: string, field: string }} - */ -export function formActionMarkup(id, has) { - let forced = ''; - if (!has('method')) forced += ' method="post"'; - if (!has('enctype')) forced += ' enctype="multipart/form-data"'; - const field = ``; - return { forced, field }; -} - -/** Minimal attribute escape; the identity is framework-generated, so this is belt and braces. */ -function escapeAttrValue(s) { - return String(s).replace(/&/g, '&').replace(/"/g, '"').replace(/} seenAttrs attribute names already written on this tag - * @returns {{ attrValue: string, forced: string, field: string } | null} - */ -export function commitFormActionAttr(val, attrName, tag, seenAttrs) { - if (typeof val !== 'function') return null; - if (!isFormActionAttr(attrName)) { - // A function on any other attribute keeps the pre-existing stringify - // behaviour: widening the claim here would change unrelated rendering. - return null; - } - const id = resolveFormActionOrThrow(val, attrName, tag); - const { forced, field } = formActionMarkup(id, (n) => seenAttrs.has(n)); - // The form posts to its OWN url; the identity rides the hidden field, which - // is what lets one action be bound from any page (#1155) while the failure - // re-render still targets the page the form was on. - return { attrValue: '""', forced, field }; + + `on <${tag || 'form'}>. Stringifying a function into HTML would leak its ` + + `source (a 'use server' action's body included) to every visitor, so this ` + + `is refused. Pass a URL string instead.`; } diff --git a/packages/core/src/render-server.js b/packages/core/src/render-server.js index 4659e751b..3d9051f3e 100644 --- a/packages/core/src/render-server.js +++ b/packages/core/src/render-server.js @@ -1,7 +1,7 @@ import { html, isTemplate } from './html.js'; import { BINDING_PREFIXES } from './binding-prefixes.js'; import { escapeText, escapeAttr } from './escape.js'; -import { assertNotFunctionActionAttr, commitFormActionAttr } from './form-action.js'; +import { assertNotFunctionActionAttr } from './form-action.js'; import { lookup, lookupModuleUrl, allTags } from './registry.js'; import { stylesToString, isCSS } from './css.js'; import { isRepeat } from './repeat.js'; @@ -152,13 +152,6 @@ async function renderTemplate(tr, ctx) { let commentDashes = 0; let currentTag = ''; // lowercased tag name currently being parsed let rawTail = ''; // rolling lowercased tail, tracks / - // Form-action binding (#1155). A `` commits its - // attribute mid-tag, but the identity field it needs is a CHILD, so the - // markup is buffered here and flushed the moment the open tag closes. - // `seenAttrs` tracks what the author already wrote on the current tag so a - // forced `method` / `enctype` never duplicates an explicit one. - let pendingFormField = ''; - let seenAttrs = new Set(); for (let i = 0; i < strings.length; i++) { const s = strings[i]; @@ -202,9 +195,6 @@ async function renderTemplate(tr, ctx) { case 'in-tag': out += c; if (c === '>') { - // The open tag just closed, so a buffered form-action identity - // field can now be emitted as the form's first child (#1155). - if (pendingFormField) { out += pendingFormField; pendingFormField = ''; } state = isRawtextTag(currentTag) ? 'rawtext' : 'text'; if (state === 'rawtext') rawTail = ''; } else if (!/\s/.test(c) && c !== '/') { @@ -223,34 +213,20 @@ async function renderTemplate(tr, ctx) { } break; case 'attr-name': - // Record every attribute the author wrote on this tag, so a forced - // method/enctype (#1155) never duplicates an explicit one. - if (c === '=') { state = 'after-eq'; seenAttrs.add(attrName.toLowerCase()); out += c; } - else if (/\s/.test(c)) { state = 'in-tag'; seenAttrs.add(attrName.toLowerCase()); attrName = ''; out += c; } - else if (c === '>') { - state = 'text'; - seenAttrs.add(attrName.toLowerCase()); - attrName = ''; - out += c; - if (pendingFormField) { out += pendingFormField; pendingFormField = ''; } - } + if (c === '=') { state = 'after-eq'; out += c; } + else if (/\s/.test(c)) { state = 'in-tag'; attrName = ''; out += c; } + else if (c === '>') { state = 'text'; attrName = ''; out += c; } else { attrName += c; out += c; } break; case 'after-eq': if (c === '"' || c === "'") { state = 'attr-quoted'; attrQuote = c; out += c; } else if (/\s/.test(c)) { state = 'in-tag'; attrName = ''; out += c; } - else if (c === '>') { - state = 'text'; attrName = ''; out += c; - if (pendingFormField) { out += pendingFormField; pendingFormField = ''; } - } + else if (c === '>') { state = 'text'; attrName = ''; out += c; } else { state = 'attr-unquoted'; out += c; } break; case 'attr-unquoted': if (/\s/.test(c)) { state = 'in-tag'; attrName = ''; out += c; } - else if (c === '>') { - state = 'text'; attrName = ''; out += c; - if (pendingFormField) { out += pendingFormField; pendingFormField = ''; } - } + else if (c === '>') { state = 'text'; attrName = ''; out += c; } else out += c; break; case 'attr-quoted': @@ -348,22 +324,16 @@ async function renderTemplate(tr, ctx) { state = 'in-tag'; attrName = ''; } else { - // A function bound to action=/formaction= is either an identifiable - // server action (emit the identity field, #1155) or refused - // outright (#1154); it is never stringified. - const bound = commitFormActionAttr(val, attrName, currentTag, seenAttrs); - if (bound) { - out += bound.attrValue + bound.forced; - pendingFormField = bound.field; - } else { - out += `"${escapeAttr(String(val ?? ''))}"`; - } + // #1154: never stringify a function into action=/formaction= (it + // would serialize a server action's source into the served HTML). + assertNotFunctionActionAttr(val, attrName, currentTag); + out += `"${escapeAttr(String(val ?? ''))}"`; state = 'in-tag'; attrName = ''; } } else if (state === 'attr-quoted' || state === 'attr-unquoted') { - // A hole INSIDE a value (`action="${fn}"`, mixed `action="/x/${fn}"`) - // cannot carry an identity field, so a function here is always refused. + // Same guard for a hole inside a quoted/unquoted value, the + // `action="${fn}"` and mixed `action="/x/${fn}"` shapes (#1154). assertNotFunctionActionAttr(val, attrName, currentTag); out += escapeAttr(String(val ?? '')); } @@ -1805,9 +1775,6 @@ async function streamTemplate(tr, ctx, controller) { let rawTail = ''; // Buffer used for attribute handling where we may need to backtrack. let buf = ''; - // Form-action binding (#1155), mirroring renderTemplate. - let pendingFormField = ''; - let seenAttrs = new Set(); for (let i = 0; i < strings.length; i++) { const s = strings[i]; @@ -1821,8 +1788,8 @@ async function streamTemplate(tr, ctx, controller) { case 'tag-open': buf += c; if (c === '!') state = 'bang-1'; - else if (c === '/') { state = 'tag-name'; currentTag = ''; seenAttrs = new Set(); } - else if (/[a-zA-Z]/.test(c)) { state = 'tag-name'; currentTag = c.toLowerCase(); seenAttrs = new Set(); } + else if (c === '/') { state = 'tag-name'; currentTag = ''; } + else if (/[a-zA-Z]/.test(c)) { state = 'tag-name'; currentTag = c.toLowerCase(); } else state = 'text'; break; case 'bang-1': @@ -1851,7 +1818,6 @@ async function streamTemplate(tr, ctx, controller) { case 'in-tag': buf += c; if (c === '>') { - if (pendingFormField) { buf += pendingFormField; pendingFormField = ''; } state = isRawtextTag(currentTag) ? 'rawtext' : 'text'; if (state === 'rawtext') rawTail = ''; } else if (!/\s/.test(c) && c !== '/') { @@ -1870,29 +1836,20 @@ async function streamTemplate(tr, ctx, controller) { } break; case 'attr-name': - if (c === '=') { state = 'after-eq'; seenAttrs.add(attrName.toLowerCase()); buf += c; } - else if (/\s/.test(c)) { state = 'in-tag'; seenAttrs.add(attrName.toLowerCase()); attrName = ''; buf += c; } - else if (c === '>') { - state = 'text'; seenAttrs.add(attrName.toLowerCase()); attrName = ''; buf += c; - if (pendingFormField) { buf += pendingFormField; pendingFormField = ''; } - } + if (c === '=') { state = 'after-eq'; buf += c; } + else if (/\s/.test(c)) { state = 'in-tag'; attrName = ''; buf += c; } + else if (c === '>') { state = 'text'; attrName = ''; buf += c; } else { attrName += c; buf += c; } break; case 'after-eq': if (c === '"' || c === "'") { state = 'attr-quoted'; attrQuote = c; buf += c; } else if (/\s/.test(c)) { state = 'in-tag'; attrName = ''; buf += c; } - else if (c === '>') { - state = 'text'; attrName = ''; buf += c; - if (pendingFormField) { buf += pendingFormField; pendingFormField = ''; } - } + else if (c === '>') { state = 'text'; attrName = ''; buf += c; } else { state = 'attr-unquoted'; buf += c; } break; case 'attr-unquoted': if (/\s/.test(c)) { state = 'in-tag'; attrName = ''; buf += c; } - else if (c === '>') { - state = 'text'; attrName = ''; buf += c; - if (pendingFormField) { buf += pendingFormField; pendingFormField = ''; } - } + else if (c === '>') { state = 'text'; attrName = ''; buf += c; } else buf += c; break; case 'attr-quoted': @@ -1934,16 +1891,12 @@ async function streamTemplate(tr, ctx, controller) { state = 'in-tag'; attrName = ''; } else { - // Same contract as the buffered renderer (they must not diverge; see - // commitFormActionAttr). Before this, a STREAMED page emitted a bound - // action's source verbatim while the buffered path already refused it. - const bound = commitFormActionAttr(val, attrName, currentTag, seenAttrs); - if (bound) { - buf += bound.attrValue + bound.forced; - pendingFormField = bound.field; - } else { - buf += `"${escapeAttr(String(val ?? ''))}"`; - } + // The SAME guard as the buffered renderer above. This is a second, + // independent state machine, so it does not inherit that one: before + // this, a STREAMED page emitted a bound function's whole source while + // the buffered path already refused it. + assertNotFunctionActionAttr(val, attrName, currentTag); + buf += `"${escapeAttr(String(val ?? ''))}"`; state = 'in-tag'; attrName = ''; } From 1c573bd3802df3d9d0ca2c9e050a258864f07476 Mon Sep 17 00:00:00 2001 From: Vivek Date: Tue, 28 Jul 2026 17:40:04 +0530 Subject: [PATCH 05/16] refactor: keep isFormActionAttr internal to the guard module It has no consumer outside form-action.js, and the module is internal, so exporting it only widens what a future edit has to consider. --- packages/core/src/form-action.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/core/src/form-action.js b/packages/core/src/form-action.js index d56debaf5..4cb80e997 100644 --- a/packages/core/src/form-action.js +++ b/packages/core/src/form-action.js @@ -33,7 +33,7 @@ export function assertNotFunctionActionAttr(val, attrName, tag) { } /** @param {string} attrName @returns {boolean} */ -export function isFormActionAttr(attrName) { +function isFormActionAttr(attrName) { const name = String(attrName).toLowerCase(); return name === 'action' || name === 'formaction'; } From a958d727d3a5cf9ff2790ee9f897d925d4da9cba Mon Sep 17 00:00:00 2001 From: Vivek Date: Tue, 28 Jul 2026 17:47:43 +0530 Subject: [PATCH 06/16] docs: stop the review loop re-running the slow test layers A review round is a reading pass, but the loop had no rule against attaching a multi-minute suite to each round, so a session could spend most of its time re-running e2e and the full Node suite between rounds while nothing they cover had changed. States the split explicitly: fast layers during the loop, slow layers once after the last clean round, and prefer CI (which already gates the merge on all of them) over a local re-run. --- .claude/skills/webjs-start-work/SKILL.md | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/.claude/skills/webjs-start-work/SKILL.md b/.claude/skills/webjs-start-work/SKILL.md index 0fb19120d..44e771d3b 100644 --- a/.claude/skills/webjs-start-work/SKILL.md +++ b/.claude/skills/webjs-start-work/SKILL.md @@ -135,6 +135,10 @@ Doc drift is the #1 way a framework rots. Documentation MUST stay in sync with c - **Smoke** (`test/examples/*/smoke/*`): the example apps still boot and serve their key routes. - **Cross-runtime (Bun)** (`node scripts/run-bun-tests.js`, needs `bun` on PATH; the `test/bun/*.mjs` scripts run under both runtimes): webjs runs on **Node 24+ OR Bun** (#508), so a change to runtime-sensitive code MUST be proven on Bun, not just Node. "Runtime-sensitive" = the serializer (Blob/File/FormData/typed arrays), the server request/listener path (the node:http vs `Bun.serve` shells, SSE, WebSocket upgrade, compression, timeouts), streams + `node:fs` (anything using `Readable.fromWeb` / `pipeline` / `createWriteStream`), `node:crypto`, the TS stripper, `AsyncLocalStorage`, or ANY `node:*` API whose behaviour Bun may implement differently. The Bun matrix (`scripts/run-bun-tests.js`) re-runs the `node:test` suite under `bun test` and FAILS on a genuine divergence; a divergence is a REAL bug to fix in the framework (this session found 5: a FormData fresh-identity serializer crash, a `Readable.fromWeb` `put()` hang, the amaro vs Node TS-strip error code, a JSC vs V8 error-message format, a link-unsafe `node:module` named import), not something to skip. Add a `test/bun/.mjs` cross-runtime assert script (wired into the CI `bun` job) for any new surface that touches the listener / serializer / streaming path. A test that is legitimately Node-only (asserts a node:http internal, the built-in stripper, `module.registerHooks` seeding, the node `ws`-library subsystem) goes on the runner's documented `DENYLIST` with a reason and a note of where the Bun behaviour IS covered; if a file MIXES runtime-agnostic and Node-only tests, SPLIT the Node-only ones into their own file so the rest still runs on Bun. See the skill's `references/testing.md`. + **Run the slow layers ONCE, at the end. Never inside the review loop.** The layers below cost minutes each (e2e ~4 min, the full Node suite ~5 min, the Bun matrix and the browser suite similar), so running them per review round burns most of a session on repeated work that cannot have changed. The correct order is: fast layers during the loop, slow layers once after the LAST clean round, and only for the layers the change can actually affect. See "Cost discipline in the loop" under the self-review loop below for the full rule. + + Also: **CI already runs the browser, e2e (Node AND Bun), Bun-matrix, four-app, Postgres, Docker and Build jobs as required checks on every PR**, and merge is gated on them. So the local run is a fast-feedback convenience, not the gate. Once the branch is pushed and the loop is clean, prefer pushing and reading `gh pr checks ` over re-running the same suites locally. Do NOT run a slow suite locally to "confirm" something CI is about to check anyway, unless you need the failure detail sooner than CI can give it. + The trap: **`npm test` does NOT run the browser, e2e, or Bun layers** (browser needs `wtr`; e2e is gated behind `WEBJS_E2E=1`; the Bun matrix is a separate `node scripts/run-bun-tests.js` and runs only the Node path otherwise). A green `npm test` is necessary but NOT sufficient. If the change can affect client behaviour or the served wire, you MUST run `npm run test:browser` and/or `WEBJS_E2E=1 node --test test/e2e/e2e.test.mjs`; if it touches runtime-sensitive code (above), you MUST run `node scripts/run-bun-tests.js` (with `bun` installed) and the `test/bun/*.mjs` scripts under Bun, then report the result. Reasoning "the unit tests pass" while shipping a change that alters what the browser downloads, OR that diverges on Bun, is the exact failure this rule exists to prevent. Acceptance criteria phrased in browser terms ("network probe", "renders without JS", "hydrates", "no console errors") are a hard signal that an e2e or browser test is REQUIRED, not optional. For each layer, either add/update coverage and run it, or write "N/A because " in the PR body. If a pre-existing test in a layer you ran is already red on `main`, say so explicitly (with proof) rather than letting it look like your regression. @@ -310,6 +314,24 @@ Beyond review findings, proactively record the *reasoning* behind a PR as commen **What's worth capturing (judgement, not a checklist):** why an approach won over a credible alternative; an experiment tried and reverted, with the reason; a tradeoff accepted knowingly (a cold-start cost, a known-small race window left in, a documented edge case); a constraint or invariant discovered mid-work; anything you would want explained if you returned to the PR with no memory of the conversation. Skip the trivial: routine fixes, mechanical edits, anything the diff already makes obvious. The bar is "would a future agent be missing important context without this", not "log everything". When the PR body already covers a decision, a short comment is fine or skip it; do not duplicate the whole body into a comment. +### Cost discipline in the loop (read before the first round) + +A review round is a READING pass, not a test run. The reviewer subagent reads the diff and the files; it does not execute suites. So a round costs seconds of wall clock, and the loop stays cheap **only if you do not attach a slow test run to each round**. + +**During the loop, run ONLY the fast layers:** +- the specific unit test files covering the code you touched (`node --test `), and +- targeted probe scripts when you need to confirm real behaviour rather than infer it. + +**Do NOT run, at any point inside the loop:** the e2e suite (`WEBJS_E2E=1 …`, ~4 min), the full Node suite (`node scripts/run-node-tests.js`, ~5 min), the browser suite (`npm run test:browser`), the Bun matrix (`node scripts/run-bun-tests.js`), or the four-app dogfood boot check. A finding fixed in round 2 cannot change what the e2e did in round 1 in any way you could act on before the loop ends, so running them per round is pure repetition. + +**After the LAST clean round, run the slow layers exactly once**, and only the ones the change can affect (a pure-prose docs change needs none; a renderer change needs browser and e2e; a listener/serializer change needs Bun). Report those results in the PR body. + +**Prefer CI over a local slow run.** Every PR's required checks already include Unit+integration, Browser, E2E (Node and Bun), Bun matrix, four-app, Postgres, Docker and Build, and merge is blocked until they pass. So after the loop converges: push, `gh pr ready `, then `gh pr checks ` (add `--watch --interval 30` to block until they settle). Re-running the same suite locally to "confirm" what CI is about to run is duplicated work; do it only when you need the failure detail sooner than CI can produce it. + +**Baseline comparisons are worth it once, never repeatedly.** When a suite has pre-existing failures, comparing the failure SET against an `origin/main` worktree is the only way to tell a regression from ambient breakage, and it is worth the one run. Doing it again after each fix is not: re-diff only if a NEW failure name appears. + +If you catch yourself starting a multi-minute suite while the loop is still finding things, stop. That is the exact waste this section exists to prevent. + ### How the loop works The draft PR is already open (step 6), so reviews post to it from the first round. Do NOT mark it ready for review or report "ready for merge" yet. Run rounds of self-review until ONE round finds zero new issues. Each round must: @@ -340,7 +362,7 @@ The draft PR is already open (step 6), so reviews post to it from the first roun 3. **If the round found any findings (even rejected ones)**, run another round with a fresh subagent. The new round picks a slightly different focus prompt: if round 1 was broad, round 2 zooms in on the file you most edited; if round 2 zoomed in, round 3 zooms out to cross-file consistency, etc. Rotate focus to avoid the agent rediscovering the same surface. -4. **If the round reports `CLEAN`**, the loop is done. +4. **If the round reports `CLEAN`**, the loop is done. THIS is the point at which the slow layers run, once (see "Cost discipline in the loop" above), not before. The minimum is TWO rounds. A clean first round is rare and usually means the review was too shallow; if round 1 is clean, spawn a second one with a sharper, narrower focus before believing the result. From 685f6a24e7cecc69d6a16c7c431f15e369c37938 Mon Sep 17 00:00:00 2001 From: Vivek Date: Tue, 28 Jul 2026 17:48:31 +0530 Subject: [PATCH 07/16] docs: name the five checks that actually gate a merge --- .claude/skills/webjs-start-work/SKILL.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.claude/skills/webjs-start-work/SKILL.md b/.claude/skills/webjs-start-work/SKILL.md index 44e771d3b..e31e4def5 100644 --- a/.claude/skills/webjs-start-work/SKILL.md +++ b/.claude/skills/webjs-start-work/SKILL.md @@ -326,7 +326,9 @@ A review round is a READING pass, not a test run. The reviewer subagent reads th **After the LAST clean round, run the slow layers exactly once**, and only the ones the change can affect (a pure-prose docs change needs none; a renderer change needs browser and e2e; a listener/serializer change needs Bun). Report those results in the PR body. -**Prefer CI over a local slow run.** Every PR's required checks already include Unit+integration, Browser, E2E (Node and Bun), Bun matrix, four-app, Postgres, Docker and Build, and merge is blocked until they pass. So after the loop converges: push, `gh pr ready `, then `gh pr checks ` (add `--watch --interval 30` to block until they settle). Re-running the same suite locally to "confirm" what CI is about to run is duplicated work; do it only when you need the failure detail sooner than CI can produce it. +**Prefer CI over a local slow run, but know exactly what CI gates.** Only FIVE checks are required by `main` branch protection: `Conventions (webjs check)`, `Unit + integration (node --test)`, `Browser (web-test-runner / Playwright)`, `E2E (Puppeteer against the blog example)`, and `Build (@webjsdev/core dist)`. Those five you can safely leave to CI: push, `gh pr ready `, then `gh pr checks ` (add `--watch --interval 30` to block until they settle), and re-run locally only when you need the failure detail sooner. + +The other jobs (`Bun runtime smoke + test matrix`, `E2E (blog served on Bun)`, `In-repo app tests`, `Postgres prod-engine round-trip`, `Docker image build`) DO run on every PR but are NOT required, so a red one does not block the merge button. Verify the list rather than trusting this paragraph, since protection changes: `gh api repos/webjsdev/webjs/branches/main/protection --jq '.required_status_checks.contexts'`. Consequence that matters: **Bun parity is not enforced by the merge gate.** When the change touches runtime-sensitive code, read the Bun job's result explicitly (or run it locally) before merging; do not assume a green merge button means Bun passed. **Baseline comparisons are worth it once, never repeatedly.** When a suite has pre-existing failures, comparing the failure SET against an `origin/main` worktree is the only way to tell a regression from ambient breakage, and it is worth the one run. Doing it again after each fix is not: re-diff only if a NEW failure name appears. From 79beec99c87529ddd4f0ffdc232287e9834151ca Mon Sep 17 00:00:00 2001 From: Vivek Date: Tue, 28 Jul 2026 17:50:16 +0530 Subject: [PATCH 08/16] docs: never block on CI between review rounds --- .claude/skills/webjs-start-work/SKILL.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.claude/skills/webjs-start-work/SKILL.md b/.claude/skills/webjs-start-work/SKILL.md index e31e4def5..e6d466988 100644 --- a/.claude/skills/webjs-start-work/SKILL.md +++ b/.claude/skills/webjs-start-work/SKILL.md @@ -330,6 +330,8 @@ A review round is a READING pass, not a test run. The reviewer subagent reads th The other jobs (`Bun runtime smoke + test matrix`, `E2E (blog served on Bun)`, `In-repo app tests`, `Postgres prod-engine round-trip`, `Docker image build`) DO run on every PR but are NOT required, so a red one does not block the merge button. Verify the list rather than trusting this paragraph, since protection changes: `gh api repos/webjsdev/webjs/branches/main/protection --jq '.required_status_checks.contexts'`. Consequence that matters: **Bun parity is not enforced by the merge gate.** When the change touches runtime-sensitive code, read the Bun job's result explicitly (or run it locally) before merging; do not assume a green merge button means Bun passed. +**Never BLOCK on CI between rounds.** Pushing is fire-and-forget: push, then immediately start the next review round while the jobs run. Do NOT sit on `gh pr checks --watch` between rounds, and do NOT pause the loop "until CI comes back". CI takes minutes, a round takes seconds, and a round that finds something means another push anyway, which supersedes the run you were waiting on. Read CI EXACTLY ONCE, after the last clean round, with a plain `gh pr checks ` (no `--watch`); only if something is still pending at that point is `--watch` appropriate. Waiting on a CI run that a later commit will invalidate is the same waste as re-running e2e per round. + **Baseline comparisons are worth it once, never repeatedly.** When a suite has pre-existing failures, comparing the failure SET against an `origin/main` worktree is the only way to tell a regression from ambient breakage, and it is worth the one run. Doing it again after each fix is not: re-diff only if a NEW failure name appears. If you catch yourself starting a multi-minute suite while the loop is still finding things, stop. That is the exact waste this section exists to prevent. From f4ac00f48a268ff7c7e3cf94a13225aad1ee5b25 Mon Sep 17 00:00:00 2001 From: Vivek Date: Tue, 28 Jul 2026 17:50:47 +0530 Subject: [PATCH 09/16] docs: fix the contradiction about which CI checks are required --- .claude/skills/webjs-start-work/SKILL.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.claude/skills/webjs-start-work/SKILL.md b/.claude/skills/webjs-start-work/SKILL.md index e6d466988..ffb8f4b88 100644 --- a/.claude/skills/webjs-start-work/SKILL.md +++ b/.claude/skills/webjs-start-work/SKILL.md @@ -137,7 +137,7 @@ Doc drift is the #1 way a framework rots. Documentation MUST stay in sync with c **Run the slow layers ONCE, at the end. Never inside the review loop.** The layers below cost minutes each (e2e ~4 min, the full Node suite ~5 min, the Bun matrix and the browser suite similar), so running them per review round burns most of a session on repeated work that cannot have changed. The correct order is: fast layers during the loop, slow layers once after the LAST clean round, and only for the layers the change can actually affect. See "Cost discipline in the loop" under the self-review loop below for the full rule. - Also: **CI already runs the browser, e2e (Node AND Bun), Bun-matrix, four-app, Postgres, Docker and Build jobs as required checks on every PR**, and merge is gated on them. So the local run is a fast-feedback convenience, not the gate. Once the branch is pushed and the loop is clean, prefer pushing and reading `gh pr checks ` over re-running the same suites locally. Do NOT run a slow suite locally to "confirm" something CI is about to check anyway, unless you need the failure detail sooner than CI can give it. + Also: **CI runs the browser, e2e (Node AND Bun), Bun-matrix, four-app, Postgres, Docker and Build jobs on every PR**, so a local run is fast feedback, not the gate. Only five of those are REQUIRED by branch protection though, and the Bun ones are not among them (see "Prefer CI over a local slow run" below for the exact list and what it means for Bun parity). Do NOT run a slow suite locally to "confirm" something CI is about to check anyway, unless you need the failure detail sooner than CI can give it. The trap: **`npm test` does NOT run the browser, e2e, or Bun layers** (browser needs `wtr`; e2e is gated behind `WEBJS_E2E=1`; the Bun matrix is a separate `node scripts/run-bun-tests.js` and runs only the Node path otherwise). A green `npm test` is necessary but NOT sufficient. If the change can affect client behaviour or the served wire, you MUST run `npm run test:browser` and/or `WEBJS_E2E=1 node --test test/e2e/e2e.test.mjs`; if it touches runtime-sensitive code (above), you MUST run `node scripts/run-bun-tests.js` (with `bun` installed) and the `test/bun/*.mjs` scripts under Bun, then report the result. Reasoning "the unit tests pass" while shipping a change that alters what the browser downloads, OR that diverges on Bun, is the exact failure this rule exists to prevent. From f1b06c27a9a76dd883569777cee63c68a7ed099e Mon Sep 17 00:00:00 2001 From: Vivek Date: Tue, 28 Jul 2026 17:52:29 +0530 Subject: [PATCH 10/16] docs: simplify the no-suites-in-the-loop rule --- .claude/skills/webjs-start-work/SKILL.md | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/.claude/skills/webjs-start-work/SKILL.md b/.claude/skills/webjs-start-work/SKILL.md index ffb8f4b88..b84dfe5d9 100644 --- a/.claude/skills/webjs-start-work/SKILL.md +++ b/.claude/skills/webjs-start-work/SKILL.md @@ -318,19 +318,19 @@ Beyond review findings, proactively record the *reasoning* behind a PR as commen A review round is a READING pass, not a test run. The reviewer subagent reads the diff and the files; it does not execute suites. So a round costs seconds of wall clock, and the loop stays cheap **only if you do not attach a slow test run to each round**. -**During the loop, run ONLY the fast layers:** -- the specific unit test files covering the code you touched (`node --test `), and -- targeted probe scripts when you need to confirm real behaviour rather than infer it. +**Run NO test suite inside the loop.** Not the e2e suite, not the full Node suite, not the browser suite, not the Bun matrix, not the four-app boot check, and not `npm test` either. A round reads code; a suite tells you nothing a reading pass needs, and a finding fixed in round 2 invalidates whatever round 1's run told you anyway. -**Do NOT run, at any point inside the loop:** the e2e suite (`WEBJS_E2E=1 …`, ~4 min), the full Node suite (`node scripts/run-node-tests.js`, ~5 min), the browser suite (`npm run test:browser`), the Bun matrix (`node scripts/run-bun-tests.js`), or the four-app dogfood boot check. A finding fixed in round 2 cannot change what the e2e did in round 1 in any way you could act on before the loop ends, so running them per round is pure repetition. +The only two things worth executing mid-loop are both seconds long and are part of MAKING a fix, not part of reviewing it: +- the specific test file(s) covering the line you just changed (`node --test `), to confirm the fix does what you think, and +- a targeted probe script when you need to observe real behaviour instead of inferring it from the source. -**After the LAST clean round, run the slow layers exactly once**, and only the ones the change can affect (a pure-prose docs change needs none; a renderer change needs browser and e2e; a listener/serializer change needs Bun). Report those results in the PR body. +**Verification happens once, after the last clean round, and CI is the mechanism.** Push, `gh pr ready `, and let the pipeline run everything in parallel on its own hardware. Run a suite locally at that point only for what CI does not gate (Bun parity, see below) or when you need the failure detail sooner than CI can produce it. Report the results in the PR body. **Prefer CI over a local slow run, but know exactly what CI gates.** Only FIVE checks are required by `main` branch protection: `Conventions (webjs check)`, `Unit + integration (node --test)`, `Browser (web-test-runner / Playwright)`, `E2E (Puppeteer against the blog example)`, and `Build (@webjsdev/core dist)`. Those five you can safely leave to CI: push, `gh pr ready `, then `gh pr checks ` (add `--watch --interval 30` to block until they settle), and re-run locally only when you need the failure detail sooner. The other jobs (`Bun runtime smoke + test matrix`, `E2E (blog served on Bun)`, `In-repo app tests`, `Postgres prod-engine round-trip`, `Docker image build`) DO run on every PR but are NOT required, so a red one does not block the merge button. Verify the list rather than trusting this paragraph, since protection changes: `gh api repos/webjsdev/webjs/branches/main/protection --jq '.required_status_checks.contexts'`. Consequence that matters: **Bun parity is not enforced by the merge gate.** When the change touches runtime-sensitive code, read the Bun job's result explicitly (or run it locally) before merging; do not assume a green merge button means Bun passed. -**Never BLOCK on CI between rounds.** Pushing is fire-and-forget: push, then immediately start the next review round while the jobs run. Do NOT sit on `gh pr checks --watch` between rounds, and do NOT pause the loop "until CI comes back". CI takes minutes, a round takes seconds, and a round that finds something means another push anyway, which supersedes the run you were waiting on. Read CI EXACTLY ONCE, after the last clean round, with a plain `gh pr checks ` (no `--watch`); only if something is still pending at that point is `--watch` appropriate. Waiting on a CI run that a later commit will invalidate is the same waste as re-running e2e per round. +**Never wait for CI during the loop.** Push and immediately start the next round. No `gh pr checks --watch`, no pausing "until CI comes back". Read CI once, after the last clean round. **Baseline comparisons are worth it once, never repeatedly.** When a suite has pre-existing failures, comparing the failure SET against an `origin/main` worktree is the only way to tell a regression from ambient breakage, and it is worth the one run. Doing it again after each fix is not: re-diff only if a NEW failure name appears. From cebb1bab0eacf67a209c25b1372a1b8499788d1f Mon Sep 17 00:00:00 2001 From: Vivek Date: Tue, 28 Jul 2026 17:52:45 +0530 Subject: [PATCH 11/16] docs: drop the last suggestion to block on CI --- .claude/skills/webjs-start-work/SKILL.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.claude/skills/webjs-start-work/SKILL.md b/.claude/skills/webjs-start-work/SKILL.md index b84dfe5d9..ce8b278b7 100644 --- a/.claude/skills/webjs-start-work/SKILL.md +++ b/.claude/skills/webjs-start-work/SKILL.md @@ -316,7 +316,7 @@ Beyond review findings, proactively record the *reasoning* behind a PR as commen ### Cost discipline in the loop (read before the first round) -A review round is a READING pass, not a test run. The reviewer subagent reads the diff and the files; it does not execute suites. So a round costs seconds of wall clock, and the loop stays cheap **only if you do not attach a slow test run to each round**. +A review round is a READING pass, not a test run. It costs seconds. Anything that turns a round into minutes is waste. **Run NO test suite inside the loop.** Not the e2e suite, not the full Node suite, not the browser suite, not the Bun matrix, not the four-app boot check, and not `npm test` either. A round reads code; a suite tells you nothing a reading pass needs, and a finding fixed in round 2 invalidates whatever round 1's run told you anyway. @@ -326,7 +326,7 @@ The only two things worth executing mid-loop are both seconds long and are part **Verification happens once, after the last clean round, and CI is the mechanism.** Push, `gh pr ready `, and let the pipeline run everything in parallel on its own hardware. Run a suite locally at that point only for what CI does not gate (Bun parity, see below) or when you need the failure detail sooner than CI can produce it. Report the results in the PR body. -**Prefer CI over a local slow run, but know exactly what CI gates.** Only FIVE checks are required by `main` branch protection: `Conventions (webjs check)`, `Unit + integration (node --test)`, `Browser (web-test-runner / Playwright)`, `E2E (Puppeteer against the blog example)`, and `Build (@webjsdev/core dist)`. Those five you can safely leave to CI: push, `gh pr ready `, then `gh pr checks ` (add `--watch --interval 30` to block until they settle), and re-run locally only when you need the failure detail sooner. +**Prefer CI over a local slow run, but know exactly what CI gates.** Only FIVE checks are required by `main` branch protection: `Conventions (webjs check)`, `Unit + integration (node --test)`, `Browser (web-test-runner / Playwright)`, `E2E (Puppeteer against the blog example)`, and `Build (@webjsdev/core dist)`. Those five you can safely leave to CI: push, `gh pr ready `, then read `gh pr checks ` once. Re-run one locally only when you need the failure detail sooner. The other jobs (`Bun runtime smoke + test matrix`, `E2E (blog served on Bun)`, `In-repo app tests`, `Postgres prod-engine round-trip`, `Docker image build`) DO run on every PR but are NOT required, so a red one does not block the merge button. Verify the list rather than trusting this paragraph, since protection changes: `gh api repos/webjsdev/webjs/branches/main/protection --jq '.required_status_checks.contexts'`. Consequence that matters: **Bun parity is not enforced by the merge gate.** When the change touches runtime-sensitive code, read the Bun job's result explicitly (or run it locally) before merging; do not assume a green merge button means Bun passed. From 006fd76e19ee9725d97564904eed6efefd03ebc1 Mon Sep 17 00:00:00 2001 From: Vivek Date: Tue, 28 Jul 2026 17:59:34 +0530 Subject: [PATCH 12/16] docs: fall back to inline review when the subagent cannot run --- .claude/skills/webjs-start-work/SKILL.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/.claude/skills/webjs-start-work/SKILL.md b/.claude/skills/webjs-start-work/SKILL.md index ce8b278b7..8c358de09 100644 --- a/.claude/skills/webjs-start-work/SKILL.md +++ b/.claude/skills/webjs-start-work/SKILL.md @@ -357,6 +357,14 @@ The draft PR is already open (step 6), so reviews post to it from the first roun - Asks for a numbered list with `file:line` references. Problems only, no suggestions. - Ends with: "If you find nothing genuinely wrong, say exactly `CLEAN` and stop. Do not pad." + **If the subagent cannot run, fall back; never stall.** The subagent is the PREFERRED reviewer, not a required one. It can fail in three ways that all look different and must all be handled the same: the spawn is declined at the permission prompt, the tool returns an error or a missing result, or the agent returns something unusable. In every case: retry AT MOST once, then **review the diff yourself inline and keep the loop going**. A blocked reviewer must never become a blocked loop. + + Two rules that make the fallback honest: + - **A failed tool call is not work in progress.** An error or a missing result means nothing is running. Do not wait for it, do not poll for it, and do not tell the user it is "still going". If unsure, confirm with `TaskList` (empty means nothing is in flight) and move on immediately. + - **Self-review shares your blind spots, so compensate.** When reviewing your own diff, do not only re-read it: write a throwaway probe script that EXERCISES the claim (render the thing, call the function, assert the observable behaviour) and run it. Reading your own code confirms what you meant; probing it shows what it does. On this repo that difference has already caught a real bug that pure reading missed. + + Say which mode the review ran in when you report, so a weaker inline round is visible rather than passing as a fresh-eyes one. + 2. **For each finding the subagent reports**, do exactly ONE of these three. There is no fourth option, and "mention it and move on" is not allowed: - **Fix it** on the branch (commit + push to update the PR), OR - **Reject it** explicitly with a one-sentence reason written in your reply to the user and in the PR body. Rejection has to be defensible (e.g. "the agent flagged X as a security issue but X runs server-side only and never reaches user input"). False positives are real; reject them on the merits, don't just hand-wave. OR From d80a83fc101fe97fd7cbbfa729556172b02f9808 Mon Sep 17 00:00:00 2001 From: Vivek Date: Tue, 28 Jul 2026 18:10:27 +0530 Subject: [PATCH 13/16] docs: require the subagent reviewer, surface failures immediately --- .claude/skills/webjs-start-work/SKILL.md | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/.claude/skills/webjs-start-work/SKILL.md b/.claude/skills/webjs-start-work/SKILL.md index 8c358de09..4c2cd18bf 100644 --- a/.claude/skills/webjs-start-work/SKILL.md +++ b/.claude/skills/webjs-start-work/SKILL.md @@ -357,13 +357,14 @@ The draft PR is already open (step 6), so reviews post to it from the first roun - Asks for a numbered list with `file:line` references. Problems only, no suggestions. - Ends with: "If you find nothing genuinely wrong, say exactly `CLEAN` and stop. Do not pad." - **If the subagent cannot run, fall back; never stall.** The subagent is the PREFERRED reviewer, not a required one. It can fail in three ways that all look different and must all be handled the same: the spawn is declined at the permission prompt, the tool returns an error or a missing result, or the agent returns something unusable. In every case: retry AT MOST once, then **review the diff yourself inline and keep the loop going**. A blocked reviewer must never become a blocked loop. + **The subagent reviewer is REQUIRED. Reviewing your own diff inline is NOT an acceptable substitute.** The whole value is that the reviewer has none of your context and therefore none of your blind spots; a self-review re-derives the same assumptions that produced the bug. So never quietly downgrade a round to an inline pass and never report a round as done when it ran inline. - Two rules that make the fallback honest: - - **A failed tool call is not work in progress.** An error or a missing result means nothing is running. Do not wait for it, do not poll for it, and do not tell the user it is "still going". If unsure, confirm with `TaskList` (empty means nothing is in flight) and move on immediately. - - **Self-review shares your blind spots, so compensate.** When reviewing your own diff, do not only re-read it: write a throwaway probe script that EXERCISES the claim (render the thing, call the function, assert the observable behaviour) and run it. Reading your own code confirms what you meant; probing it shows what it does. On this repo that difference has already caught a real bug that pure reading missed. + **Surface a broken subagent IMMEDIATELY, in the same turn you notice.** The failure modes look different and are all handled the same way: the spawn is declined at the permission prompt, the tool returns an error or a missing result, the agent returns something unusable, or it produces nothing for an unreasonably long time. In every case: + - **A failed or missing tool result is NOT work in progress.** Nothing is running. Do not wait on it, do not poll it, and never tell the user it is "still going". Confirm with `TaskList` (empty means nothing is in flight) and say so at once. + - **Always spawn the reviewer with `run_in_background: false`**, so a failure comes back as a result you can see rather than a task you might sit on. + - **Tell the user the moment a round cannot run**, state plainly that the review is blocked and why, and ask how to proceed. Retry once if the failure looks transient. Do NOT silently substitute a weaker review, and do NOT let the loop stall in silence, which is the worst of both (no review AND no progress). - Say which mode the review ran in when you report, so a weaker inline round is visible rather than passing as a fresh-eyes one. + A blocked reviewer stops the loop and becomes a question for the user. It never becomes an inline round, and it never becomes an unreported wait. 2. **For each finding the subagent reports**, do exactly ONE of these three. There is no fourth option, and "mention it and move on" is not allowed: - **Fix it** on the branch (commit + push to update the PR), OR From 26da33b6bafb8b33c1c8a6844b408e5299f99051 Mon Sep 17 00:00:00 2001 From: Vivek Date: Tue, 28 Jul 2026 18:38:10 +0530 Subject: [PATCH 14/16] fix: guard the .action property binding, the third leak path Review found `.action=${fn}` on a native element unguarded. `action` is a reflected IDL attribute, so assigning a function stringifies its source into the element's own attribute in a real browser. The linkedom tests could not see it (linkedom does not reflect), which is why it survived the earlier passes and why this ships with a real-browser test. A custom element's `.action` is an ordinary author-defined property that never reflects, so a function there stays legitimate and is not refused. Also corrects three things the review caught in this PR's own claims: the streaming machine is reached only through `renderToStream(v, {ssr:false})` and no page render uses it, so the earlier "a streamed page leaked" wording overstated a public-API hole as a live leak; the streaming quoted and mixed holes had no coverage, so that branch could have been deleted with the suite still green; and two client assertions were tautological, since the host is empty whenever the render throws. --- .claude/skills/webjs-start-work/SKILL.md | 14 +-- packages/core/src/form-action.js | 8 +- packages/core/src/render-client.js | 9 ++ packages/core/src/render-server.js | 6 +- .../browser/form-action-guard.test.js | 87 +++++++++++++++++++ .../form-action-attr-guard-client.test.js | 42 ++++++++- .../rendering/form-action-attr-guard.test.js | 36 +++++++- 7 files changed, 184 insertions(+), 18 deletions(-) create mode 100644 packages/core/test/rendering/browser/form-action-guard.test.js diff --git a/.claude/skills/webjs-start-work/SKILL.md b/.claude/skills/webjs-start-work/SKILL.md index 4c2cd18bf..68cb16cb7 100644 --- a/.claude/skills/webjs-start-work/SKILL.md +++ b/.claude/skills/webjs-start-work/SKILL.md @@ -135,9 +135,9 @@ Doc drift is the #1 way a framework rots. Documentation MUST stay in sync with c - **Smoke** (`test/examples/*/smoke/*`): the example apps still boot and serve their key routes. - **Cross-runtime (Bun)** (`node scripts/run-bun-tests.js`, needs `bun` on PATH; the `test/bun/*.mjs` scripts run under both runtimes): webjs runs on **Node 24+ OR Bun** (#508), so a change to runtime-sensitive code MUST be proven on Bun, not just Node. "Runtime-sensitive" = the serializer (Blob/File/FormData/typed arrays), the server request/listener path (the node:http vs `Bun.serve` shells, SSE, WebSocket upgrade, compression, timeouts), streams + `node:fs` (anything using `Readable.fromWeb` / `pipeline` / `createWriteStream`), `node:crypto`, the TS stripper, `AsyncLocalStorage`, or ANY `node:*` API whose behaviour Bun may implement differently. The Bun matrix (`scripts/run-bun-tests.js`) re-runs the `node:test` suite under `bun test` and FAILS on a genuine divergence; a divergence is a REAL bug to fix in the framework (this session found 5: a FormData fresh-identity serializer crash, a `Readable.fromWeb` `put()` hang, the amaro vs Node TS-strip error code, a JSC vs V8 error-message format, a link-unsafe `node:module` named import), not something to skip. Add a `test/bun/.mjs` cross-runtime assert script (wired into the CI `bun` job) for any new surface that touches the listener / serializer / streaming path. A test that is legitimately Node-only (asserts a node:http internal, the built-in stripper, `module.registerHooks` seeding, the node `ws`-library subsystem) goes on the runner's documented `DENYLIST` with a reason and a note of where the Bun behaviour IS covered; if a file MIXES runtime-agnostic and Node-only tests, SPLIT the Node-only ones into their own file so the rest still runs on Bun. See the skill's `references/testing.md`. - **Run the slow layers ONCE, at the end. Never inside the review loop.** The layers below cost minutes each (e2e ~4 min, the full Node suite ~5 min, the Bun matrix and the browser suite similar), so running them per review round burns most of a session on repeated work that cannot have changed. The correct order is: fast layers during the loop, slow layers once after the LAST clean round, and only for the layers the change can actually affect. See "Cost discipline in the loop" under the self-review loop below for the full rule. + **Run these layers ONCE, at the end, not once per review round.** They cost minutes each (e2e ~4 min, the full Node suite ~5 min, the Bun matrix and the browser suite similar), so running them between rounds burns a session on repeats. This changes WHEN they run, never WHETHER: everything the paragraph below mandates still has to happen before the PR is marked ready. See "Cost discipline in the loop" under the self-review loop for the timing rule. - Also: **CI runs the browser, e2e (Node AND Bun), Bun-matrix, four-app, Postgres, Docker and Build jobs on every PR**, so a local run is fast feedback, not the gate. Only five of those are REQUIRED by branch protection though, and the Bun ones are not among them (see "Prefer CI over a local slow run" below for the exact list and what it means for Bun parity). Do NOT run a slow suite locally to "confirm" something CI is about to check anyway, unless you need the failure detail sooner than CI can give it. + Also: **CI runs the browser, e2e (Node AND Bun), Bun-matrix, four-app, Postgres, Docker and Build jobs on every PR**, so for those a local run is fast feedback rather than the only signal. Note that only five checks are REQUIRED by branch protection, and they are not simply five of the jobs just listed: they are `Conventions (webjs check)`, `Unit + integration (node --test)`, `Browser (web-test-runner / Playwright)`, `E2E (Puppeteer against the blog example)` and `Build (@webjsdev/core dist)`. Everything else, INCLUDING both Bun jobs, runs without gating the merge button. The trap: **`npm test` does NOT run the browser, e2e, or Bun layers** (browser needs `wtr`; e2e is gated behind `WEBJS_E2E=1`; the Bun matrix is a separate `node scripts/run-bun-tests.js` and runs only the Node path otherwise). A green `npm test` is necessary but NOT sufficient. If the change can affect client behaviour or the served wire, you MUST run `npm run test:browser` and/or `WEBJS_E2E=1 node --test test/e2e/e2e.test.mjs`; if it touches runtime-sensitive code (above), you MUST run `node scripts/run-bun-tests.js` (with `bun` installed) and the `test/bun/*.mjs` scripts under Bun, then report the result. Reasoning "the unit tests pass" while shipping a change that alters what the browser downloads, OR that diverges on Bun, is the exact failure this rule exists to prevent. @@ -318,13 +318,13 @@ Beyond review findings, proactively record the *reasoning* behind a PR as commen A review round is a READING pass, not a test run. It costs seconds. Anything that turns a round into minutes is waste. -**Run NO test suite inside the loop.** Not the e2e suite, not the full Node suite, not the browser suite, not the Bun matrix, not the four-app boot check, and not `npm test` either. A round reads code; a suite tells you nothing a reading pass needs, and a finding fixed in round 2 invalidates whatever round 1's run told you anyway. +**Do not run a whole SUITE inside the loop.** Not the e2e suite, not the full Node suite, not the browser suite, not the Bun matrix, not the four-app boot check. A round reads code; a suite tells a reading pass nothing, and a finding fixed in round 2 invalidates whatever round 1's run reported anyway. -The only two things worth executing mid-loop are both seconds long and are part of MAKING a fix, not part of reviewing it: -- the specific test file(s) covering the line you just changed (`node --test `), to confirm the fix does what you think, and -- a targeted probe script when you need to observe real behaviour instead of inferring it from the source. +What you DO run mid-loop, because both are seconds long and both are part of MAKING a fix rather than reviewing it: +- the specific test file(s) covering the line you just changed (`node --test `). This also satisfies the repo's commit rule, which requires a commit's own tests to pass before it lands, NOT the whole suite to be re-run per commit. +- a targeted probe script, when you need to observe real behaviour instead of inferring it from the source. -**Verification happens once, after the last clean round, and CI is the mechanism.** Push, `gh pr ready `, and let the pipeline run everything in parallel on its own hardware. Run a suite locally at that point only for what CI does not gate (Bun parity, see below) or when you need the failure detail sooner than CI can produce it. Report the results in the PR body. +**The full verification happens once, after the last clean round.** Run every layer the Definition of done requires for this change (browser and e2e for anything browser-facing, the Bun matrix for anything runtime-sensitive, the four-app check for a framework change), then push and read CI. This rule defers those runs to a single pass; it never excuses skipping one. Report the results in the PR body. **Prefer CI over a local slow run, but know exactly what CI gates.** Only FIVE checks are required by `main` branch protection: `Conventions (webjs check)`, `Unit + integration (node --test)`, `Browser (web-test-runner / Playwright)`, `E2E (Puppeteer against the blog example)`, and `Build (@webjsdev/core dist)`. Those five you can safely leave to CI: push, `gh pr ready `, then read `gh pr checks ` once. Re-run one locally only when you need the failure detail sooner. diff --git a/packages/core/src/form-action.js b/packages/core/src/form-action.js index 4cb80e997..a2c19df2d 100644 --- a/packages/core/src/form-action.js +++ b/packages/core/src/form-action.js @@ -3,8 +3,12 @@ * * Both SSR state machines and the client renderer import from here so the * rule cannot drift between them. It already drifted once: the guard was - * added to the buffered renderer alone, leaving the Suspense-streaming path - * emitting a server action's whole body into the response. + * added to the buffered `renderTemplate` alone, leaving `streamTemplate` + * stringifying the function. That second machine is reached only through + * `renderToStream(v, { ssr: false })`, which no page render uses (the server + * renders every page, Suspense included, via `renderToString`), so it was a + * public-API hole rather than a live page leak. Worth closing on its own + * terms, and worth noting as the reason the rule lives in one place. */ /** diff --git a/packages/core/src/render-client.js b/packages/core/src/render-client.js index e7a9b7ac2..17e0f2b6e 100644 --- a/packages/core/src/render-client.js +++ b/packages/core/src/render-client.js @@ -682,6 +682,15 @@ function applyPart(part, value, _prev, allValues) { break; } case 'prop': + // `.action=${fn}` on a NATIVE element is a leak too, not just the + // attribute form: `action` is a reflected IDL attribute, so assigning a + // function stringifies it into the element's own `action` content + // attribute in a real browser. Guarded on native elements only; a custom + // element's `.action` is an ordinary author-defined property that never + // reflects, and passing a function to one is legitimate. + if (!part.el.localName.includes('-')) { + assertNotFunctionActionAttr(value, part.name, part.el.localName); + } /** @type any */ (part.el)[part.name] = value; break; case 'bool': diff --git a/packages/core/src/render-server.js b/packages/core/src/render-server.js index 3d9051f3e..24bf7576b 100644 --- a/packages/core/src/render-server.js +++ b/packages/core/src/render-server.js @@ -1893,8 +1893,10 @@ async function streamTemplate(tr, ctx, controller) { } else { // The SAME guard as the buffered renderer above. This is a second, // independent state machine, so it does not inherit that one: before - // this, a STREAMED page emitted a bound function's whole source while - // the buffered path already refused it. + // this it stringified the function while the buffered path already + // refused it. Reached only via `renderToStream(v, { ssr: false })`, + // which no page render uses, so this was a public-API hole rather + // than a live page leak. assertNotFunctionActionAttr(val, attrName, currentTag); buf += `"${escapeAttr(String(val ?? ''))}"`; state = 'in-tag'; diff --git a/packages/core/test/rendering/browser/form-action-guard.test.js b/packages/core/test/rendering/browser/form-action-guard.test.js new file mode 100644 index 000000000..6918ef7ed --- /dev/null +++ b/packages/core/test/rendering/browser/form-action-guard.test.js @@ -0,0 +1,87 @@ +/** + * The form-action guard, in a REAL browser (#1154). + * + * The node-side coverage runs under linkedom, which does not implement IDL + * attribute reflection: there, `form.action = fn` sets a plain JS property and + * the `action` content attribute stays untouched, so the leak the property + * binding causes is INVISIBLE to that layer. In a real browser `action` is a + * reflected IDL attribute, so the assignment stringifies the function into the + * element's own markup, which is the actual leak. That difference is exactly + * why this file exists rather than trusting the linkedom tests. + * + * It also pins the real-DOM behaviour of the attribute path: a refused render + * must leave the previously-rendered value in place, never a half-written one. + */ + +import { html } from '../../../src/html.js'; +import { render } from '../../../src/render-client.js'; + +import { assert } from '../../../../../test/browser-assert.js'; + +// The sentinel lives inside the function body, so it appears in the output only +// if the function was stringified. +async function secretAction(formData) { + const CONNECTION = 'postgres://user:BROWSER_LEAK_MARKER@host/db'; + return CONNECTION; +} + +function mount() { + const host = document.createElement('div'); + document.body.appendChild(host); + return host; +} + +suite('form-action guard in a real browser', () => { + + test('action=${fn} throws and writes nothing to the document', () => { + const host = mount(); + let threw = null; + try { render(html``, host); } + catch (e) { threw = e; } + assert.ok(threw, 'render must throw'); + assert.ok(/function was interpolated into action=/.test(threw.message), threw.message); + assert.ok(!document.body.innerHTML.includes('BROWSER_LEAK_MARKER'), 'no source in the document'); + }); + + test('.action=${fn} property binding cannot reflect the source into the attribute', () => { + // THE case linkedom cannot see. Without the guard, `el.action = fn` here + // reflects the stringified function into the live `action` attribute. + const host = mount(); + let threw = null; + try { render(html`
`, host); } + catch (e) { threw = e; } + assert.ok(threw, 'render must throw'); + assert.ok(!document.body.innerHTML.includes('BROWSER_LEAK_MARKER'), 'no source in the document'); + const form = host.querySelector('form'); + if (form) { + assert.ok(!String(form.getAttribute('action') || '').includes('BROWSER_LEAK_MARKER'), 'no source in the action attribute'); + assert.ok(!String(form.action || '').includes('BROWSER_LEAK_MARKER'), 'no source on the action property'); + } + }); + + test('a refused re-render leaves the previously rendered action intact', () => { + const host = mount(); + const tpl = (a) => html`
`; + render(tpl('/submit'), host); + const before = host.querySelector('form').getAttribute('action'); + assert.equal(before, '/submit'); + + let threw = null; + try { render(tpl(secretAction), host); } catch (e) { threw = e; } + assert.ok(threw, 'the re-render must throw'); + assert.equal(host.querySelector('form').getAttribute('action'), '/submit', 'prior value survives'); + assert.ok(!document.body.innerHTML.includes('BROWSER_LEAK_MARKER'), 'no source in the document'); + }); + + test('a real form still submits to a string action', () => { + // The guard must not disturb the ordinary case: a string action has to + // survive into a working, submittable form. + const host = mount(); + render(html`
`, host); + const form = host.querySelector('form'); + assert.equal(form.getAttribute('action'), '/feedback'); + assert.equal(form.method, 'post'); + assert.equal(new FormData(form).get('msg'), 'hi'); + }); + +}); diff --git a/packages/core/test/rendering/form-action-attr-guard-client.test.js b/packages/core/test/rendering/form-action-attr-guard-client.test.js index 8974e4f78..646b56ea4 100644 --- a/packages/core/test/rendering/form-action-attr-guard-client.test.js +++ b/packages/core/test/rendering/form-action-attr-guard-client.test.js @@ -25,13 +25,18 @@ before(async () => { async function fakeAction() { const S = 'CLIENT_SECRET'; return S; } -test('client render of action=${fn} throws, DOM never carries the source', () => { +// NOTE on what these DON'T assert: `createInstance` applies every part before +// it calls `container.replaceChildren(...)`, so a throwing part leaves `host` +// empty no matter what. An `assert.ok(!host.innerHTML.includes(SECRET))` here +// would pass unconditionally and prove nothing. The load-bearing assertion is +// the throw itself, plus the SECOND-render cases below, where the host DOES +// hold a live form and a leak would therefore be observable. +test('client render of action=${fn} throws', () => { const host = document.createElement('div'); assert.throws( () => render(html`
`, host), /function was interpolated into action=/, ); - assert.ok(!host.innerHTML.includes('CLIENT_SECRET')); }); test('client render of mixed action="/x/${fn}" throws', () => { @@ -40,7 +45,38 @@ test('client render of mixed action="/x/${fn}" throws', () => { () => render(html`
`, host), /function was interpolated into action=/, ); - assert.ok(!host.innerHTML.includes('CLIENT_SECRET')); +}); + +// Re-render over an ALREADY-MOUNTED form. Here the host really does hold a live +// element, so if the guard let the value through, the source would be sitting +// in the DOM and this would catch it. +test('re-render swapping a string action for a function throws, live DOM stays clean', () => { + const host = document.createElement('div'); + const tpl = (a) => html`
`; + render(tpl('/ok'), host); + assert.equal(host.querySelector('form').getAttribute('action'), '/ok'); + assert.throws(() => render(tpl(fakeAction), host), /function was interpolated into action=/); + assert.ok(!host.innerHTML.includes('CLIENT_SECRET'), 'live DOM must not carry the source'); + assert.equal(host.querySelector('form').getAttribute('action'), '/ok', 'the prior value must survive'); +}); + +// `.action=${fn}` is a PROPERTY binding, a different commit site from the +// attribute one. `action` is a reflected IDL attribute, so assigning a function +// writes its source into the element's own `action` in a real browser. +test('client .action=${fn} property binding on a native form throws', () => { + const host = document.createElement('div'); + assert.throws( + () => render(html`
`, host), + /function was interpolated into action=/, + ); +}); + +test('a custom element keeps accepting a function on .action', () => { + // Not a reflected IDL attribute, just an author-defined property, so passing + // a function is legitimate and must not be refused. + const host = document.createElement('div'); + render(html``, host); + assert.equal(typeof host.querySelector('my-widget').action, 'function'); }); test('string action still renders on the client', () => { diff --git a/packages/core/test/rendering/form-action-attr-guard.test.js b/packages/core/test/rendering/form-action-attr-guard.test.js index 878292a9d..220d0c888 100644 --- a/packages/core/test/rendering/form-action-attr-guard.test.js +++ b/packages/core/test/rendering/form-action-attr-guard.test.js @@ -78,10 +78,12 @@ test('string-valued action renders byte-identically to before', async () => { ); }); -// The STREAMING renderer is a second, independent state machine. It was missed -// by the first pass of this fix and kept emitting the action's whole body while -// the buffered renderer already refused it, so it gets its own coverage rather -// than being assumed to inherit the guard. +// The STREAMING renderer is a second, independent state machine, missed by the +// first pass of this fix. It is reached only through +// `renderToStream(v, { ssr: false })`, which no page render uses (the server +// renders every page, Suspense included, via `renderToString`), so this was a +// public-API hole and not a live page leak. It gets its own coverage rather +// than being assumed to inherit the buffered renderer's guard. test('the streaming renderer refuses the same function (ssr:false path)', async () => { await assert.rejects( () => drain(renderToStream(html`
`, { ssr: false })), @@ -97,6 +99,32 @@ test('the streaming renderer never emits the source', async () => { assert.ok(!out.includes('SECRET'), 'streamed output must not carry the function source'); }); +// The unquoted shape lands in the streaming machine's `after-eq` branch; the +// quoted and mixed shapes land in its SEPARATE `attr-quoted`/`attr-unquoted` +// branch. Without these two, that second branch could be deleted outright and +// the suite would stay green, which is the same guard-asymmetry this whole +// change exists to prevent, just moved into the test layer. +test('the streaming renderer refuses a quoted hole', async () => { + await assert.rejects( + () => drain(renderToStream(html`
`, { ssr: false })), + /function was interpolated into action=/, + ); +}); + +test('the streaming renderer refuses a mixed hole', async () => { + await assert.rejects( + () => drain(renderToStream(html`
`, { ssr: false })), + /function was interpolated into action=/, + ); +}); + +test('the streaming renderer refuses formaction', async () => { + await assert.rejects( + () => drain(renderToStream(html``, { ssr: false })), + /function was interpolated into formaction=/, + ); +}); + test('streaming keeps string-valued actions working', async () => { const out = await drain(renderToStream(html`
`, { ssr: false })); assert.match(out, /action="\/submit"/); From e2e95c4460ba96d85e13a10004224e27e4241c20 Mon Sep 17 00:00:00 2001 From: Vivek Date: Tue, 28 Jul 2026 18:39:04 +0530 Subject: [PATCH 15/16] docs: cover the form-action throw on the docs site, fix the action= advice --- .agents/skills/webjs/references/muscle-memory-gotchas.md | 7 +++++-- website/app/docs/migrating-from-nextjs/page.ts | 2 +- website/app/docs/troubleshooting/page.ts | 5 +++++ 3 files changed, 11 insertions(+), 3 deletions(-) diff --git a/.agents/skills/webjs/references/muscle-memory-gotchas.md b/.agents/skills/webjs/references/muscle-memory-gotchas.md index 5a947ea3c..2c0f22fb1 100644 --- a/.agents/skills/webjs/references/muscle-memory-gotchas.md +++ b/.agents/skills/webjs/references/muscle-memory-gotchas.md @@ -53,8 +53,11 @@ The reason is a source leak. During SSR a `.server.ts` import is the ACTUAL func // WRONG: throws at render; it would have leaked the action's body. import { submitFeedback } from '#modules/feedback/actions/submit-feedback.server.ts'; html`
`; -// RIGHT: post to the page's own url and handle it in the page's `action` export. -html``; +// RIGHT: omit action entirely to post to the page's own url, and handle the +// submission in that page's `action` export. (Omit it rather than writing +// action="": the HTML spec requires a non-empty URL when the attribute is +// present, so the empty string is a conformance error.) +html``; ``` A string stays a string: `action="/search"` and `action=${'/search'}` are unchanged. Other attributes keep their existing stringify behaviour; only `action` and `formaction` are claimed. diff --git a/website/app/docs/migrating-from-nextjs/page.ts b/website/app/docs/migrating-from-nextjs/page.ts index 005c648c8..6ab4f9573 100644 --- a/website/app/docs/migrating-from-nextjs/page.ts +++ b/website/app/docs/migrating-from-nextjs/page.ts @@ -103,7 +103,7 @@ Counter.register('my-counter');

What ports cleanly, and what does not

Ports directly: the app/ directory layout, dynamic segments ([id], [...rest], [[...rest]]), route groups ((group)), the metadata API, route handlers, middleware, and the loading / error / not-found conventions.

-

Needs rethinking: anything written as a Client Component becomes a web component; anything fetching server data moves into a .server action; React state becomes signals; next/link becomes a plain link. Write progressive-enhancement-first: a <form> plus a server action instead of a fetch in a click handler, since the form works without JavaScript and the client router upgrades it automatically.

+

Needs rethinking: anything written as a Client Component becomes a web component; anything fetching server data moves into a .server action; React state becomes signals; next/link becomes a plain link. Write progressive-enhancement-first: a <form> plus a server action instead of a fetch in a click handler, since the form works without JavaScript and the client router upgrades it automatically. Note the binding differs from Next's: <form action={serverAction}> has no WebJs equivalent and is refused at render (it would leak the action's source into the HTML), so omit action to post to the page's own URL and handle the submission in that page's action export.

Not provided: image and font optimization, i18n, and a static export. WebJs is a no-build, standards-based framework, so these are libraries you layer on, not built-ins.

Next steps: read Getting Started to scaffold an app, Architecture for the execution model in depth, and Progressive Enhancement for the design posture that replaces the Client Component habit.

`; diff --git a/website/app/docs/troubleshooting/page.ts b/website/app/docs/troubleshooting/page.ts index 7c564cd20..583d3ff09 100644 --- a/website/app/docs/troubleshooting/page.ts +++ b/website/app/docs/troubleshooting/page.ts @@ -41,6 +41,11 @@ export default function Troubleshooting() {

Cause: WebJs instruments the native DOM mutation methods on every light-DOM host to keep the slot API live, and it installs those interceptors as own properties ON THE ELEMENT INSTANCE. An own instance property shadows a method defined on the class prototype, so the framework's interceptor wins and your same-named class method is never reached. The instrumented names are append, prepend, before, after, replaceWith, replaceChildren, remove, appendChild, insertBefore, removeChild, and replaceChild. TypeScript does not catch it because a shorter override (zero or one argument) is assignable to the native signature.

Fix: rename the member to a non-native name (appendRow(), removeItem()) and update its call sites. Only these native mutation members are affected; render() and the lifecycle hooks are meant to be overridden. The no-shadowed-native-member check rule catches this ahead of time (it flags only a real instance method, never a static member, a nested-object property, or a static shadow = true component, whose native shadow slots are not instrumented). See Components.

+

An error saying a function was interpolated into action=

+

Symptom: a page or component throws at render with a function was interpolated into action= on <form>, usually right after binding a server action to a form the way Next.js does it.

+

Cause: during SSR an imported 'use server' action is the REAL function (the RPC stub exists only in the browser), and action= is an ordinary attribute hole, so stringifying it would write the action's whole body, including anything its closure shows such as a connection string, into the HTML every visitor downloads. WebJs refuses rather than emitting it. The same applies to formaction= on a submit button, to every hole shape (action=\${fn}, action="\${fn}", action="/x/\${fn}"), and to the .action=\${fn} property form on a native form, where the reflected IDL attribute would carry the source into the DOM.

+

Fix: omit action so the form posts to the page's own URL, and handle the submission in that page's action export (write <form method="post"> rather than action="", which the HTML spec treats as a conformance error since the attribute must be a non-empty URL when present). A string action is unaffected. See Server Actions and Progressive Enhancement.

+

An error saying static properties is no longer supported

Symptom: a component throws at construction with static properties is no longer supported. Declare reactive properties via the factory instead.

Cause: the class body has a hand-written static properties = { ... } field. WebJs declares reactive properties only through the WebComponent({ ... }) base-class factory now, and the runtime throws on a direct static properties.

From c74847c7234364bd9091073a649dd3b2430e365c Mon Sep 17 00:00:00 2001 From: Vivek Date: Tue, 28 Jul 2026 18:46:19 +0530 Subject: [PATCH 16/16] chore: move the review-loop skill changes out of this PR --- .claude/skills/webjs-start-work/SKILL.md | 37 +----------------------- 1 file changed, 1 insertion(+), 36 deletions(-) diff --git a/.claude/skills/webjs-start-work/SKILL.md b/.claude/skills/webjs-start-work/SKILL.md index 68cb16cb7..0fb19120d 100644 --- a/.claude/skills/webjs-start-work/SKILL.md +++ b/.claude/skills/webjs-start-work/SKILL.md @@ -135,10 +135,6 @@ Doc drift is the #1 way a framework rots. Documentation MUST stay in sync with c - **Smoke** (`test/examples/*/smoke/*`): the example apps still boot and serve their key routes. - **Cross-runtime (Bun)** (`node scripts/run-bun-tests.js`, needs `bun` on PATH; the `test/bun/*.mjs` scripts run under both runtimes): webjs runs on **Node 24+ OR Bun** (#508), so a change to runtime-sensitive code MUST be proven on Bun, not just Node. "Runtime-sensitive" = the serializer (Blob/File/FormData/typed arrays), the server request/listener path (the node:http vs `Bun.serve` shells, SSE, WebSocket upgrade, compression, timeouts), streams + `node:fs` (anything using `Readable.fromWeb` / `pipeline` / `createWriteStream`), `node:crypto`, the TS stripper, `AsyncLocalStorage`, or ANY `node:*` API whose behaviour Bun may implement differently. The Bun matrix (`scripts/run-bun-tests.js`) re-runs the `node:test` suite under `bun test` and FAILS on a genuine divergence; a divergence is a REAL bug to fix in the framework (this session found 5: a FormData fresh-identity serializer crash, a `Readable.fromWeb` `put()` hang, the amaro vs Node TS-strip error code, a JSC vs V8 error-message format, a link-unsafe `node:module` named import), not something to skip. Add a `test/bun/.mjs` cross-runtime assert script (wired into the CI `bun` job) for any new surface that touches the listener / serializer / streaming path. A test that is legitimately Node-only (asserts a node:http internal, the built-in stripper, `module.registerHooks` seeding, the node `ws`-library subsystem) goes on the runner's documented `DENYLIST` with a reason and a note of where the Bun behaviour IS covered; if a file MIXES runtime-agnostic and Node-only tests, SPLIT the Node-only ones into their own file so the rest still runs on Bun. See the skill's `references/testing.md`. - **Run these layers ONCE, at the end, not once per review round.** They cost minutes each (e2e ~4 min, the full Node suite ~5 min, the Bun matrix and the browser suite similar), so running them between rounds burns a session on repeats. This changes WHEN they run, never WHETHER: everything the paragraph below mandates still has to happen before the PR is marked ready. See "Cost discipline in the loop" under the self-review loop for the timing rule. - - Also: **CI runs the browser, e2e (Node AND Bun), Bun-matrix, four-app, Postgres, Docker and Build jobs on every PR**, so for those a local run is fast feedback rather than the only signal. Note that only five checks are REQUIRED by branch protection, and they are not simply five of the jobs just listed: they are `Conventions (webjs check)`, `Unit + integration (node --test)`, `Browser (web-test-runner / Playwright)`, `E2E (Puppeteer against the blog example)` and `Build (@webjsdev/core dist)`. Everything else, INCLUDING both Bun jobs, runs without gating the merge button. - The trap: **`npm test` does NOT run the browser, e2e, or Bun layers** (browser needs `wtr`; e2e is gated behind `WEBJS_E2E=1`; the Bun matrix is a separate `node scripts/run-bun-tests.js` and runs only the Node path otherwise). A green `npm test` is necessary but NOT sufficient. If the change can affect client behaviour or the served wire, you MUST run `npm run test:browser` and/or `WEBJS_E2E=1 node --test test/e2e/e2e.test.mjs`; if it touches runtime-sensitive code (above), you MUST run `node scripts/run-bun-tests.js` (with `bun` installed) and the `test/bun/*.mjs` scripts under Bun, then report the result. Reasoning "the unit tests pass" while shipping a change that alters what the browser downloads, OR that diverges on Bun, is the exact failure this rule exists to prevent. Acceptance criteria phrased in browser terms ("network probe", "renders without JS", "hydrates", "no console errors") are a hard signal that an e2e or browser test is REQUIRED, not optional. For each layer, either add/update coverage and run it, or write "N/A because " in the PR body. If a pre-existing test in a layer you ran is already red on `main`, say so explicitly (with proof) rather than letting it look like your regression. @@ -314,28 +310,6 @@ Beyond review findings, proactively record the *reasoning* behind a PR as commen **What's worth capturing (judgement, not a checklist):** why an approach won over a credible alternative; an experiment tried and reverted, with the reason; a tradeoff accepted knowingly (a cold-start cost, a known-small race window left in, a documented edge case); a constraint or invariant discovered mid-work; anything you would want explained if you returned to the PR with no memory of the conversation. Skip the trivial: routine fixes, mechanical edits, anything the diff already makes obvious. The bar is "would a future agent be missing important context without this", not "log everything". When the PR body already covers a decision, a short comment is fine or skip it; do not duplicate the whole body into a comment. -### Cost discipline in the loop (read before the first round) - -A review round is a READING pass, not a test run. It costs seconds. Anything that turns a round into minutes is waste. - -**Do not run a whole SUITE inside the loop.** Not the e2e suite, not the full Node suite, not the browser suite, not the Bun matrix, not the four-app boot check. A round reads code; a suite tells a reading pass nothing, and a finding fixed in round 2 invalidates whatever round 1's run reported anyway. - -What you DO run mid-loop, because both are seconds long and both are part of MAKING a fix rather than reviewing it: -- the specific test file(s) covering the line you just changed (`node --test `). This also satisfies the repo's commit rule, which requires a commit's own tests to pass before it lands, NOT the whole suite to be re-run per commit. -- a targeted probe script, when you need to observe real behaviour instead of inferring it from the source. - -**The full verification happens once, after the last clean round.** Run every layer the Definition of done requires for this change (browser and e2e for anything browser-facing, the Bun matrix for anything runtime-sensitive, the four-app check for a framework change), then push and read CI. This rule defers those runs to a single pass; it never excuses skipping one. Report the results in the PR body. - -**Prefer CI over a local slow run, but know exactly what CI gates.** Only FIVE checks are required by `main` branch protection: `Conventions (webjs check)`, `Unit + integration (node --test)`, `Browser (web-test-runner / Playwright)`, `E2E (Puppeteer against the blog example)`, and `Build (@webjsdev/core dist)`. Those five you can safely leave to CI: push, `gh pr ready `, then read `gh pr checks ` once. Re-run one locally only when you need the failure detail sooner. - -The other jobs (`Bun runtime smoke + test matrix`, `E2E (blog served on Bun)`, `In-repo app tests`, `Postgres prod-engine round-trip`, `Docker image build`) DO run on every PR but are NOT required, so a red one does not block the merge button. Verify the list rather than trusting this paragraph, since protection changes: `gh api repos/webjsdev/webjs/branches/main/protection --jq '.required_status_checks.contexts'`. Consequence that matters: **Bun parity is not enforced by the merge gate.** When the change touches runtime-sensitive code, read the Bun job's result explicitly (or run it locally) before merging; do not assume a green merge button means Bun passed. - -**Never wait for CI during the loop.** Push and immediately start the next round. No `gh pr checks --watch`, no pausing "until CI comes back". Read CI once, after the last clean round. - -**Baseline comparisons are worth it once, never repeatedly.** When a suite has pre-existing failures, comparing the failure SET against an `origin/main` worktree is the only way to tell a regression from ambient breakage, and it is worth the one run. Doing it again after each fix is not: re-diff only if a NEW failure name appears. - -If you catch yourself starting a multi-minute suite while the loop is still finding things, stop. That is the exact waste this section exists to prevent. - ### How the loop works The draft PR is already open (step 6), so reviews post to it from the first round. Do NOT mark it ready for review or report "ready for merge" yet. Run rounds of self-review until ONE round finds zero new issues. Each round must: @@ -357,15 +331,6 @@ The draft PR is already open (step 6), so reviews post to it from the first roun - Asks for a numbered list with `file:line` references. Problems only, no suggestions. - Ends with: "If you find nothing genuinely wrong, say exactly `CLEAN` and stop. Do not pad." - **The subagent reviewer is REQUIRED. Reviewing your own diff inline is NOT an acceptable substitute.** The whole value is that the reviewer has none of your context and therefore none of your blind spots; a self-review re-derives the same assumptions that produced the bug. So never quietly downgrade a round to an inline pass and never report a round as done when it ran inline. - - **Surface a broken subagent IMMEDIATELY, in the same turn you notice.** The failure modes look different and are all handled the same way: the spawn is declined at the permission prompt, the tool returns an error or a missing result, the agent returns something unusable, or it produces nothing for an unreasonably long time. In every case: - - **A failed or missing tool result is NOT work in progress.** Nothing is running. Do not wait on it, do not poll it, and never tell the user it is "still going". Confirm with `TaskList` (empty means nothing is in flight) and say so at once. - - **Always spawn the reviewer with `run_in_background: false`**, so a failure comes back as a result you can see rather than a task you might sit on. - - **Tell the user the moment a round cannot run**, state plainly that the review is blocked and why, and ask how to proceed. Retry once if the failure looks transient. Do NOT silently substitute a weaker review, and do NOT let the loop stall in silence, which is the worst of both (no review AND no progress). - - A blocked reviewer stops the loop and becomes a question for the user. It never becomes an inline round, and it never becomes an unreported wait. - 2. **For each finding the subagent reports**, do exactly ONE of these three. There is no fourth option, and "mention it and move on" is not allowed: - **Fix it** on the branch (commit + push to update the PR), OR - **Reject it** explicitly with a one-sentence reason written in your reply to the user and in the PR body. Rejection has to be defensible (e.g. "the agent flagged X as a security issue but X runs server-side only and never reaches user input"). False positives are real; reject them on the merits, don't just hand-wave. OR @@ -375,7 +340,7 @@ The draft PR is already open (step 6), so reviews post to it from the first roun 3. **If the round found any findings (even rejected ones)**, run another round with a fresh subagent. The new round picks a slightly different focus prompt: if round 1 was broad, round 2 zooms in on the file you most edited; if round 2 zoomed in, round 3 zooms out to cross-file consistency, etc. Rotate focus to avoid the agent rediscovering the same surface. -4. **If the round reports `CLEAN`**, the loop is done. THIS is the point at which the slow layers run, once (see "Cost discipline in the loop" above), not before. +4. **If the round reports `CLEAN`**, the loop is done. The minimum is TWO rounds. A clean first round is rare and usually means the review was too shallow; if round 1 is clean, spawn a second one with a sharper, narrower focus before believing the result.