fix: refuse a function in form action=, it leaked server action source

[vivek7405]

Closes #1154

<form action=${importedAction}> wrote the action's source into the served HTML. At SSR a 'use server' import is the REAL function (the RPC stub only exists in the browser) and action= is an ordinary escaped attribute hole, so the renderer stringified it:

<form action="async function createTodo(input) { const DB_SECRET = 'postgres://user:pw@host'; return { success: true }; }" method="post">

The body itself shipped to every visitor: the logic, the query shapes, the table names, and any literal written inline, a connection string included. Assume outer values go too. On Node an outer const the body reads appears only as its identifier, but Bun can fold a module-scope literal into the body before the engine sees it, so the same action reports Authorization: "Bearer sk_live_…". Do not go looking for the rule that decides when. Four separate rules were proposed and measured over this review (export status, read count, declaration position, whether the module has an import) and every one was falsified by the next on the same Bun version, so the boundary is finer than any of them and is not worth encoding. Nothing reachable from the action can be called safe. It is also the first thing a Next-trained author or agent writes, since <form action={serverAction}> is the canonical Next form, so it is reachable by accident rather than by misuse.

Now refused at render time instead, in all three renderers.

What is refused, and what is not

The guard matches the SHAPE, not one spelling of it. Getting this boundary right took most of the work here, because the first cut matched on the raw attribute name and a single extra character walked around it.

Written as	Refused	Why
action= / formaction=	yes	ordinary attribute, stringified into the HTML
any quoted hole, sigil or not	yes	quoting turns a binding hole back into a plain attribute
action=${[fn]}	yes	Array.prototype.toString stringifies elements, so it leaks identically
.action= on a native form (and .formAction= on a button/input)	yes	there the property is a reflected IDL attribute, so the source lands in the live DOM
.action= on any other native tag	no	a plain expando that reflects nothing, so refusing it would break working code rather than close a leak
?action=	yes	never leaked, but a truthy function silently emitted a bare action=""
.action= on a custom element	no	an author-defined property, not a reflected IDL attribute (holds for a PLAIN prop; one declared reflect: true writes the source on a path outside these commit sites, see #1169)
@action= unquoted	no	an event listener, and a function is exactly what one takes

Every other attribute keeps today's stringify behaviour, and a string-valued action is byte-identical to before. Both carve-outs and the scope boundary have their own tests, so a guard that refused everything would fail the suite.

One rule, every commit site

SSR has two independent template state machines and the client renderer has four applyPart clauses (attr, attr-mixed, prop, bool), each committing on its own branch. Every one of them routes through packages/core/src/form-action.js, because these renderers drifted apart three separate times during this change and duplicated logic is what allowed it.

Two entry points there, and the distinction is load-bearing. assertNotFunctionActionAttr is name-based: an ATTRIBUTE is stringified into the markup on whatever tag it appears. assertNotFunctionReflectedActionProp is name-and-tag-based: a PROPERTY only reaches the markup where the DOM reflects it, which is action on a <form> and formAction on a <button> / <input>. Using the first on the property path is what refused <div .action=${fn}>, a binding that never leaked.

The client prop path is the one linkedom cannot see: on a form, action is a reflected IDL attribute, so el.action = fn writes the source into the element's own markup in a real browser and not under a DOM shim. That is why it ships with a browser test rather than another linkedom one.

Scope

Only the leak. The form-binding feature that gives <form action=${action}> a working meaning is #1155 and ships separately.

Consequence worth stating: until #1155 lands, <form action=${fn}> is an error with no replacement. That is intended. It was never a working pattern, it was a silent data leak.

Inside a component the error does not surface: per-component SSR error isolation contains it, so production renders the component empty and the page still returns 200. Nothing leaks either way, and every doc surface below now says so, since a form that silently vanished is a confusing way to learn you wrote a Next binding.

Two qualifications the docs now carry, because "renders the component empty" understates both. The isolation replaces the element through its matching closing tag, so anything slotted into it disappears too: put the bad form in a shared header and every route returns 200 with a blank body. And on a route with a loading.{js,ts} the page renders inside a Suspense boundary after the 200 is flushed, where the throw is swallowed with no log and no onError at all, so the server-log line the docs tell you to check does not exist there. That silence is a known framework gap rather than intended behaviour.

History

This supersedes #1156, which carried the same fix and was closed unmerged because the re-review of its final fix set never ran. That review has now run.

Test plan

• Unit: every refused shape on the buffered SSR machine, the streaming machine, and the client, plus every carve-out and passthrough. Rendering suite green
• The thrown message never embeds the source it is withholding
• Browser (Chromium, Firefox, WebKit): the reflected-property leak is refused in a real DOM, 5 tests green on each engine
• Counterfactual, unit: disabling the guard reds the refusal tests and leaves exactly the carve-outs and passthroughs green
• Counterfactual, browser: reverting the prop clause reds exactly the reflection test on all three engines
• The streaming test drains with a large synchronous read burst, since recovery is one chunk per read and for await or a sequential reader loop both call a real leak clean; running out of the burst is a hard failure rather than a clean-looking drain
• Counterfactual, scope boundary: a renderer change that dropped function values in every attribute reds the scope test
• Every guard call site mapped to a test that reds when that call site alone is reverted. No orphans. The .prop clause is pinned on BOTH sides: over-firing reds 4 tests, removing it reds 6
• Bun parity: test/bun/form-action-guard.mjs green on Node 26.1 and Bun 1.3.14, covering both SSR machines, the carve-outs and the cyclic-array case
• Full local suite: the only failures are the dogfood: a fresh git worktree can't resolve @webjsdev/* (no node_modules) #954 fresh-worktree trap (no built dist/, ws named exports through symlinked modules), verified identical with this branch's changes reverted. CI, which has real node_modules, is green on all ten jobs
• Dogfood: website boots 200 on /, /docs/troubleshooting, /docs/migrating-from-nextjs, /docs/server-actions, /ui, /ui/button in prod mode with no broken modulepreload hints
• Scaffold: N/A, no generated code changes

Docs

• .agents/skills/webjs/references/muscle-memory-gotchas.md: the Next-habit entry, the full refused/allowed table, the isolation caveats, and the note that String(fn) leaks from any hole rather than only a comment.
• .agents/skills/webjs/references/components.md: the error boundary covers the COMMIT as well as the fetch.
• website/app/docs/troubleshooting/page.ts: the throw, keyed by symptom, including where it does and does not surface.
• website/app/docs/components/page.ts: a carve-out where the three binding rules are stated, since the guard falsifies all three for these two names.
• website/app/docs/ssr/page.ts: the same carve-out on the template-hole table, which carried the identical three claims.
• website/app/docs/migrating-from-nextjs/page.ts: corrects the action= advice.
• examples/blog/CONVENTIONS.md and examples/blog/.agents/rules/workflow.md: the dogfood app's agent rules still told authors to bind a server action into action=, the exact shape now refused.
• .github/workflows/ci.yml: a Bun job step for the parity proof.
• AGENTS.md / scaffold: N/A. The page action export is untouched and <form action=${fn}> was never a documented pattern; the scaffold templates were checked and teach nothing that changes. Those surfaces move with Unify form submissions on <form action=${action}>, drop the page action export #1155.

The async-commit fixes that came with it

_commitAsync now catches a throw from the DOM commit and routes it to _handleRenderError, the boundary the fetch half already used. Without it the refusal escaped as an unhandled rejection on the async render() path, renderError() never ran, and updateComplete never settled again for that instance.

Not caused by the guard: an unrelated commit throw (a value whose toString throws) behaves identically, so this is a pre-existing hole the guard made reachable by an ordinary authoring mistake. Fixed here rather than filed, because this PR ships a doc claiming per-component containment and that claim was false on the async path. Covered by a linkedom test and a browser test on all three engines.

Two follow-on fixes from later review rounds, both on the same path. The rejection had to be handled at the site that AWAITS the commit, not only inside _commitAsync: update() is a documented override point returning any thenable, and a rejecting one reproduced the original wedge verbatim. And that handler then needed the render-token check both _commitAsync handlers already had, or a superseded cycle's late rejection committed its error state over the newer render that had replaced it.

A fourth followed: String(error) was evaluated in the argument expression, outside _handleRenderError's own try/catch, so a rejection reason that cannot be coerced to a primitive threw out of the handler and skipped the release it exists to guarantee. The release now sits in a finally.

Each piece reds a different test when reverted alone. Worth stating because an earlier version of this line claimed the try/catch alone reds both tests, and that was wrong: with the rejection handler in place, reverting the try/catch produces identical observable behaviour on the ordinary path. What distinguishes it is an update() override that discards _commitAsync's promise, which is now its own test. The three settlement assertions were also rewritten to require a RESOLVED updateComplete rather than merely a settled one, since mapping both outcomes to one label made them blind to a change that rejected it.

Out of scope here, and still open: #1168 (the scaffold gallery teaching action=""), #1169 (a reflect: true prop leaks the same source on the reflection path, which is why the custom-element carve-out above is qualified), and #1172 (a throw mid-commit desyncs repeat() / guard() directive state from the DOM, pre-existing and reproducible without this guard).

Three further paths were found during review and deliberately closed as not planned rather than tracked, each with its reproduction preserved on the issue: a function in an on* inline-handler attribute (#1170), the unlogged Suspense-boundary swallow (#1174), and the general case that a function in ANY hole stringifies its source (#1175). Where a reader could hit one of those, the docs warn about it directly instead.

[vivek7405]

Decision: the prop-path guard stays in this change

I left this open when I closed #1156: .action=${fn} goes through a different commit site than action=${fn}, so it was a fair question whether it belongs in the same change as the attribute guard or in its own.

Keeping it here. Splitting would mean shipping a security fix that anyone can walk around by typing one extra character, and the two are the same leak with the same cause, a function reaching a reflected action on a native element. A fix that closes three doors and leaves the fourth open is not a floor, it is a false sense of one.

The part that deserves a second reader is the carve-out, not the guard: a native element's .action reflects, so a function stringifies into its own content attribute in a real browser, while a custom element's .action is an ordinary author-defined property that never reflects and where a function is a legitimate value. The whole distinction rests on part.el.localName.includes('-'). That is the standard custom-element test and it matches how the rest of the renderer decides the same question, but it is the newest line here and it is doing real work.

Worth noting why it needs a real browser to test at all: linkedom does not reflect IDL attributes, so the unit layer physically cannot observe this leak. That is why it survived two of my own passes on #1156 and why the coverage for it is a browser test rather than another linkedom one.

[vivek7405]

Ran a full pass over this before letting it near main, since the last version of it was closed for exactly the reason a security fix gets closed: the fixes were real but nothing had re-read them.

It found three ways to walk straight past the guard, and they are all the same mistake in different clothes. The guard matched on the raw attribute name, but the SSR state machines hand it the name as AUTHORED, and only the unquoted after-eq branch strips the binding sigil. So .action="${fn}" arrives as .action, never equals action, and falls through to being stringified. ?action= and @action= the same. One character, and the leak is back. The value check had the identical shape of hole: it asked typeof val === 'function' while every commit site does String(val), and arrays stringify their elements, so action=${[serverAction]} published the body just as the bare function did.

What I take from that: the guard was written against the shapes I had in mind rather than against the operation it is guarding, which is String(val) reaching the output. Both fixes now sit at that level instead, matching on the value's shape and on the name with any sigil stripped, so a fourth spelling I have not thought of is more likely to be covered than not.

The Bun parity test paid for itself the moment it existed. The streaming machine drops every prop binding without the native-vs-custom split the buffered one makes, so .action=${fn} was refused by one SSR renderer and waved through by the other. No unit test could see it, because both machines emit the same empty output either way; only asserting the throw across both surfaced it. That is the second time on this change that two renderers with one contract between them have drifted, which is worth remembering the next time something looks like it only needs guarding in one place.

Also worth stating plainly, because it is not obvious from the diff: inside a component this error does not surface. Per-component SSR isolation swallows it, so production renders the component empty and the page still returns 200. No leak, which is the part that matters, but a form that has silently vanished is a confusing way to learn you wrote a Next binding. The troubleshooting entry now says so, and there is a test pinning that the isolated path stays leak-free.

[vivek7405]

Audit trail: what was raised and argued down

Recording the findings that did not survive scrutiny, so the reasoning is on the PR rather than lost.

"No test renders the guard through a component, where SSR error isolation swallows the throw." Rejected as stated, then partly acted on anyway. The claim that this is a coverage hole in the GUARD is wrong: the guard fires identically inside a component, and the isolation is downstream of it. But the underlying observation was correct and worth something, because the isolation makes the refusal invisible in production. So the behaviour is now documented and there is a test pinning that the isolated path emits no source. The finding was wrong about what was broken and right about what was missing.

"examples/blog/CONVENTIONS.md:701 still says to use <form action=…> bound to a server action." Rejected. The line describes a form posting to a URL, which is the shape that still works and always did. Nothing about a string-valued action changed here.

Two duplicates. The quoted-sigil bypass came back twice at different severities, and the action="" contradiction came back twice pointing at the doc and at the scaffold. Deduped rather than actioned separately.

On the action="" split. The doc claim was true and the scaffold is the thing that is wrong, so softening the wording is the concession, not the fix. It is here because the alternative was editing three gallery templates and a skill reference inside a security fix, which needs the scaffold-sync verification and would make this diff harder to reason about. #1168 carries the real cleanup and restoring the stronger wording.

[vivek7405]

Second pass, scoped to the fixes from the first one. It found eight things and six of them were mine, introduced while fixing the original leak. That is the useful result here, not the original bug.

The one that matters most: carriesFunction recursed without tracking visited arrays, but the thing it models does not. Array.prototype.join has a cycle guard, so String(a) on a self-referential array is ''. I turned a render that used to emit action="" into a stack overflow. A guard against leaking became a way to crash, which is a strictly worse trade than the bug it was closing, and no test caught it because nobody writes a cyclic array on purpose.

Two more of the same kind. Guarding event || prop in the streaming machine was a false positive twice over: an event binding never stringifies its value, a function is exactly what one takes, so <my-el @action=${handler}> was refused for nothing. And because the buffered machine guards only prop, the commit whose message said "so the two machines agree" put them back out of step in the other direction. Third time on this change that the two SSR renderers have drifted. I now think the real lesson is that the guard should not be making per-branch decisions in two hand-written state machines at all, but that is a bigger change than this PR should carry.

The test findings are the ones I would have been most annoyed to have shipped. Five client assertions I added were exactly the assertion the file's own note says proves nothing, because a throwing part leaves the host empty; I had read that note and written the anti-pattern under it anyway. The browser test's reflection checks sat inside an if (form) that was never true, so the one file whose entire reason to exist is observing IDL reflection in a real browser was observing nothing. Both are restructured to render a working form first and swap the bad value in, so there is something live to assert against.

The Bun proof also only enumerated quoted shapes, which is why it could not see the machine divergence it was written to catch. It now covers the unquoted sigils, asserts the streaming refusal matches the right message rather than merely throwing, and pins the carve-outs, so a guard that refused everything would fail it.

[vivek7405]

Third pass, scoped to the last round's fixes. Two findings, both in the test rewrite, and both the same kind of mistake: the tests looked more thorough than they were.

The first is a coverage regression I caused by refactoring. Pulling those cases onto a shared helper silently dropped ?action="${fn}", because a quoted bool renders a literal ?action attribute and does not fit the helper's good-render step. It was dropped for not fitting, not for being wrong, and the commit message described the work as a widening. That is the failure mode of tidying tests: the diff reads as an improvement and the deletion rides along invisibly.

The second is subtler and I would not have found it by reading. A quoted single hole compiles to an attr-mixed part, not a plain attr one, so the quoted cases were pinning the guard in the piece loop while the comment above them said attr. Confirmed by neutering each clause separately: with the attr case disabled the quoted tests stay green. Everything was covered, but a maintainer trusting the comment would have edited the branch those tests do not watch.

Both are fixed. The comment now names which clause each group pins and says it was verified by reverting rather than reasoned about, since the obvious reading is backwards.

Worth recording what this round confirmed rather than found, because it is the part I was least sure of: the cycle-guard Set was fuzzed against a reference reachability walk over 200k random cyclic and DAG inputs with no divergence, so no false negative is constructible; all three renderers agree on all sixteen shapes; every row of the new docs table matches the code; and the browser file does run in real Chromium, where the custom-element carve-out passes with no definition registered.

[vivek7405]

Fourth pass. Three findings, all in the file whose previous commit existed to fix exactly this defect class, which is the part worth sitting with.

The sharp one: I hand-rolled the restored quoted-bool test on a reason that is false. I wrote that a quoted ?action does not fit the helper's good-render step. It does. Quoting moves the hole into attr-mixed like every other quoted shape, so the good value renders ?action="/submit" and the helper's default check passes; I imported the reasoning from the UNQUOTED bool case, which is the one that genuinely needs an override. The cost was not stylistic: the hand-rolled body silently dropped two of the helper's assertions, and the false justification went into the commit message too, so the history now argues for a constraint that does not exist.

The other two are stale or overreaching claims about which clause each test pins. The file header still said the client guard lives in the 'attr' and 'attr-mixed' cases when there are four and the file pins all four, so a reader would conclude 'prop' and 'bool' were unguarded here. The section comment erred the other way, crediting the unquoted cases with pinning 'attr', which is true of the array-wrapped one and false of the unquoted bool.

Three rounds in a row have now found a wrong claim about this file rather than a wrong behaviour. The tests were right every time; the prose describing them was not. So the header now carries the full four-clause map, and every row was established by neutering that clause and recording which tests go red. That is also how I checked this fix, rather than by reading the renderer again, which is what produced the wrong claims in the first place.

Also folded in a real coverage gap I hit while mapping the call sites: the streaming machine's native-prop clause was pinned only by the Bun proof, so deleting it left the entire rendering suite green. Covered there now too.

[vivek7405]

Browser verification: three engines, and what the counterfactual exposed

Ran the browser file properly rather than trusting that it passes. It is green in Chromium, Firefox and WebKit, which is worth more than I expected here, since the whole reason this file exists is that action is a reflected IDL attribute and linkedom does not reflect. Three independent engines agreeing that the property reflects, and that the guard stops it, is the strongest evidence available for this bug.

Then I reverted the prop guard to check the file is not decorative. It is not: three cases went red on all three engines. But two of them went red for the wrong reason. The hosts mount into the live document, which the reflection assertions genuinely need, and nothing removed them, so document.body carried every earlier case's markup and the document-wide leak assertions had quietly stopped describing the test they sit in. The custom-element case failed on a marker its neighbour had left behind.

Nothing was wrong with the guard, and nothing was wrong with the passing run. The damage would have landed on whoever eventually saw those three failures and started debugging the two tests that were fine. Fixed by removing each host after its test, after which the same revert fails exactly one case, the reflection one that actually covers the clause.

Worth noting how this was found, since it is the same lesson as the last three rounds: running the counterfactual surfaced it, reading the file did not, and the file had passed every prior round.

[vivek7405]

Fifth pass. Two findings, and one of them is the most consequential test problem on this PR so far.

functions in OTHER attributes keep the existing stringify behaviour is the test standing guard over the whole reason this change is safe to make: that only action and formaction are claimed and nothing else changed. It asserted that the output starts with <div title=", which an empty title="" satisfies exactly as well as the real stringified function. So the one test holding the scope boundary would have stayed green through a change that dropped function values in every attribute. It now asserts the source is genuinely still written out, and I confirmed it goes red against exactly that change.

That is worth separating from the four rounds of comment findings before it. Those were prose describing correct tests. This was a test whose assertion did not cover the claim in its own name, in the place where being wrong would have mattered most.

The second finding is the same section comment as last round, wrong again in a new way: it counted four quoted cases where the section has three, and said "the array-wrapped case" while the section holds two that land in different clauses. I have now had three attempts at a per-section clause summary and been wrong twice, so it is deleted rather than corrected a third time. The header map is keyed by hole shape and every test names its shape, so the map already answers the question; the section summary was only ever a second copy, and the second copy is what kept drifting.

Independently confirmed this round, so it does not need re-deriving: all twelve call sites map to a test that reds on its own revert with no orphans, every row of the header map holds (including the sigil-free action="${fn}" and the uppercase and single-quoted variants), the quoted-bool test reds when its clause is reverted, and the new streaming test is the only thing that reds for render-server.js:1905.

[vivek7405]

Sixth pass. Three findings, and the first one shows the previous round's fix was half a fix.

The scope-boundary test is the one holding up this PR's central claim: that only action and formaction are touched and nothing else moved. Last round I found it vacuous and strengthened it. It turns out I strengthened it on renderToString and nowhere else. Applying the exact widening it exists to catch, dropping function values in every attribute, still left the entire suite green on the streaming machine and on the client. The cross-runtime proof could not have caught it either, because every one of its passthroughs was action-valued. So the boundary was pinned on one renderer out of three, and I had already reported it as fixed.

That is the more interesting failure than the original vacuous assertion. Fixing a test at the site where it was reported, rather than everywhere the property it asserts has to hold, is how a fix looks complete and is not. All three renderers and both SSR machines in the proof now carry it, each verified against the widening.

The second finding is nicely circular: the browser file's guard against being inert was itself inert. It asserted form.action to prove the property really reflects in that browser, but the IDL getter returns whatever was assigned even where nothing reflects, which is precisely what linkedom does and precisely the layer this file exists to escape. It reads the content attribute now and passes on all three engines, so reflection is finally being asserted rather than assumed.

Third, the comment above it claimed every assertion below would hold vacuously on a fresh host. Two of them would fail instead. Narrowed to the absence assertions, which are the ones that actually go hollow.

[vivek7405]

A fifth leak path, deliberately not fixed here: #1169

Went looking for whether any OTHER attribute-stringification path could leak the same way, rather than assuming the four commit sites this PR guards are the whole surface. One more exists.

A reactive property declared reflect: true writes a function's source straight into its attribute during SSR:

<reflect-probe action="async function leaky(){ const c='REFLECT_SECRET'; return c; }" data-wj-host>

It is not an action problem. title and label do exactly the same, so it is the generic reflect path stringifying whatever it is handed. That is why it is #1169 and not a commit on this branch: #1154 scoped this change to action/formaction and said explicitly not to widen the claim, and widening a security fix mid-review to cover a different mechanism is how a reviewable diff stops being one.

The part I most wanted to know, and the answer is reassuring. This does not chain with the custom-element carve-out this PR introduces. <my-el .action=${fn}> is deliberately allowed here, so the obvious worry is that it becomes a route into the reflect leak. It does not: the serializer drops the function before it ever reaches the instance property (Cannot serialize a function), verified. Reaching #1169 requires the author to assign the function to a reflected prop inside their own component. So the carve-out is not load-bearing for it, and I am comfortable shipping the carve-out as it stands.

Recorded that negative result in #1169 too, since it is the first thing anyone picking it up would otherwise re-derive.

[vivek7405]

Seventh pass. Three findings, and the first is the most serious test defect on this PR.

the streaming renderer never emits the source could not fail. It drained through a helper whose accumulator is a local, so the throw discarded it and the string the test inspected was always empty, whatever the renderer had put on the wire. The reviewer proved it the only way that counts: patched the streaming machine to enqueue the source and then refuse, confirmed a consumer really received 70 bytes containing the marker, and the file stayed green.

The distinction it was failing to make is the one that matters for a streaming renderer. A stream can flush bytes and then refuse, and those bytes are already gone. "It threw" and "the client received nothing" are different claims, and only the second is worth asserting. It drains into a caller-owned sink now and reds against that same probe.

The Bun proof had a quieter version of the same disease. Its leaky read the marker from a const rather than inlining it, and String(fn) reproduces source, so the stringified function carried the identifier and never the marker. Every !includes(BUN_PARITY_SECRET) assertion in that file was trivially true, including the one checking that a refusal message does not echo the source it withholds. I found that by making a legitimate change and watching it fail for the wrong reason. Inlined, and that assertion now reds when the message embeds the source.

Third, the browser comment I narrowed last round was still wrong, in a new direction: the element-scoped assertions do not hold vacuously on an empty host, they throw, and the only one that passes is the document-wide check, which is the weakest. Third attempt at that comment.

The reviewer also enumerated the remaining attribute-stringify paths and confirmed what I filed as #1169: component.js:677 (reflect) and render-server.js:1509 are unpinned by any scope test, but cannot produce a <form action=…> since reflection only writes on a custom-element host, so they are not a #1154 leak path. Hydration reuses the guarded applyPart, so no gap there either.

[vivek7405]

Eleventh pass. Four findings, one of which I disagree with on the facts, and I checked before rejecting it.

The drain was wrong a fourth time, and worse than that, it had restored the exact false green the previous commit claimed to remove: a fixed batch of 16 reads passes a leak sitting behind 20 flushed chunks. The mechanism I wrote was also wrong. Nothing is pending during any of this. Every chunk is enqueued synchronously inside the ReadableStream constructor and controller.error() runs a microtask after it returns, so what a consumer keeps is what its synchronously-issued reads DEQUEUED before that microtask, one chunk per read. Which means every fixed burst is a guess about how much a given render flushes first, and I had guessed four times.

The bound cannot be removed, since the burst is sized before anything is awaited. So exhausting it is now a hard failure that names itself and says to raise it, instead of a drain that looks clean. Leaks behind 3, 20 and 100 pad chunks all red. That is the fix I should have reached for two rounds ago: when a bound is unavoidable, make crossing it loud rather than picking a bigger number.

Where I disagree. The review says Bun's constant folding holds "only for a single-reference, non-exported module-scope const", and lists export const and a twice-referenced const as not folding. Measured on bun 1.3.14, both fold:

export const K = 'sk_live_VALUE'  ->  bun: { h: "Bearer sk_live_VALUE" }
const K, read by two functions    ->  bun: { h: "Bearer sk_live_VALUE" }

let is the one that does not. So folding is BROADER than the finding says, not narrower, and the unconditional prose was closer to right than the correction would have made it. I have taken the part that stands (the claim had no conditions at all, and one exists) and stated what I measured, with a warning not to build a habit on a transpiler's boundary.

The remaining two are accurate and fixed: the Bun proof cited the file header as evidence for module-scope folding when that sample shows a function-local const, so it carries its own measurement now.

[vivek7405]

Twelfth pass. Six findings, and the Bun folding thread is the one worth drawing a conclusion from rather than just fixing.

Three attempts have now been made to state when Bun folds a module-scope const into a function body. I said export status and read count do not matter. Round eleven's reviewer said they do, and named the opposite of both. This round says neither: the axis is DECLARATION POSITION, only the first module-scope const in a file folds. I measured it and that is correct, which means every version of this claim so far has been wrong, including the one I rejected a finding to defend.

So the fix is not a fourth rule. It is to stop putting the rule in guidance at all. Three wrong answers in three attempts is the signal that this belongs to a transpiler and not in a doc someone will rely on, and the docs now say exactly that: the behaviour is real, the boundary is not worth learning, treat everything reachable from the action as exposed. That last sentence has been the correct advice the whole way through and is the only part that never needed revising.

The drain note was wrong about the mechanism too, in the direction that matters. I wrote that everything is enqueued synchronously in the ReadableStream constructor and nothing is ever pending. streamTemplate awaits at every text hole, so a three-hole template puts 2 of 6 chunks in the constructor and 4 after it, straight into pending reads. Both paths are real. What survives is the per-read recovery, which is what the burst was actually built on, so the design stands and only the explanation was fiction.

The exhaustion guard I added last round had an off-by-one that would have fired on an innocent stream: exactly DRAIN_BURST chunks followed by a close is indistinguishable from running over, and it threw on both. One extra read settles it.

Also fixed: a ladder row that contradicted the paragraph above it, a stale PR-body bullet describing the previous design, and the Bun proof's own rationale, which depended on declaration order in a file where const runtime already holds the first slot, so the construction it warns about behaves identically on both runtimes there.

[vivek7405]

Thirteenth pass. Four findings, one rejected on evidence, and the folding thread reached the point where the right move is to stop answering the question.

That thread has now produced FOUR rules for when Bun folds a module-scope const into a function body, each measured, each falsified by the next on the same Bun version: export status, then read count, then declaration position, now whether the module has an import. I verified the newest one and it does reproduce (no import folds everything, an import folds nothing), and it is still wrong, because a module with no import folded only its first const when each const was read by a different function. Five would be no better than four. Whatever the optimizer keys on is finer than any of these, and the docs now say plainly that four attempts were made and all four failed. A reader is better served knowing the boundary is unknowable than by a rule that will be wrong again. The advice underneath has never changed: treat everything reachable from the action as exposed.

Where I disagree, with evidence. The review says the streaming test's two assertions "cannot fail" and that "there is no reachable state in which either assertion reds". There is, and it is the state the test exists for. With the machine patched to write before refusing, the render still rejects, so assert.rejects is satisfied and execution reaches the assertions, which then red on the source in sink.text. Verified again at 20 pad chunks on this HEAD. The finding describes the GUARD-REMOVAL counterfactual, where there is no rejection at all and assert.rejects fails first, and that is a different scenario from the regression this test guards.

The part of it that is right, I had already committed before this round landed: on the shipped path nothing is ever enqueued, so sink.text is empty and both assertions hold trivially there. That is now stated in the file, along with the point that the whole drain apparatus buys nothing on the shipped path and exists entirely for the counterfactual.

Also narrowed the docstring's account of the two enqueue paths. Both exist, but no template in this file has a text hole, so every chunk in every number quoted there is written during the constructor and the second path plays no part in any of it.

[vivek7405]

Fourteenth pass. Five findings, and after seven rounds spent on test docstrings and prose, this one went back into the guard and found two things there.

The one branch nothing tested. Mutation testing every branch of isFormActionAttr against the suite: seven of eight mutants red it, and the survivor was .toLowerCase(). Deleting it kept all 46 unit tests AND the entire Bun parity table green while this rendered in full:

form ACTION=${fn}        -> RENDERED  leaks=true
button formAction=${fn}  -> RENDERED  leaks=true
form Action="${fn}"      -> RENDERED  leaks=true
form action=${fn}        -> REFUSED

Every shape tested up to now spelled the attribute lowercase, so nothing pinned the call that collapses case. Behaviour at HEAD was already correct, so this was coverage only, but the uncovered spelling is the one this guard exists for: formAction={serverAction} is React's canonical submit binding, so the Next muscle memory arrives in exactly the casing no test covered. Pinned now on both SSR machines, the client, and the Bun table; the mutant reds 6 unit tests and the parity file.

Worth noting what this says about the previous rounds. The suite that missed this is the same suite that had been rewritten four times over one streaming drain. Coverage of the interesting path was never the thing those rounds were measuring.

A refusal that hung the component instead of isolating it. The PR claims per-component error isolation. That is true for a sync render and false for an async one, measured:

sync,  guard throws:  updateComplete resolved     renderError called      0 unhandled
async, guard throws:  updateComplete NEVER SETTLES  renderError NOT called  1 unhandled

.then(onFulfil, onRejected) does not route onFulfil's own throw to its sibling handler, and _performRender's .then had no rejection handler. So the error escaped to unhandledrejection in production, renderError() never ran, __pendingAsyncCommits stayed at 1 forever (which also disables the _resolveUpdate escape for later non-committing cycles), and updateComplete never settled again for that instance. Any await el.updateComplete hangs, in author code and in the framework's own harness.

Checked whether the guard caused it: it does not. An unrelated commit throw (a value whose toString throws, no form action anywhere) behaves identically, so this is a pre-existing hole in the #469 async path that the guard merely made reachable by an ordinary authoring mistake. Fixed here anyway rather than filed, because this PR's own rule for the array-cycle case was that refusing to leak must not become a new way to crash, and because shipping a doc claiming containment while the async path hangs is the kind of comfortable sentence the last four rounds kept catching. The fix makes the claim true instead of weakening it. Counterfactual: reverting the try/catch reds exactly the new test and nothing else.

Docs. The components page states the three binding rules unqualified (an attribute hole is stringified, a native property binding drops at SSR, a truthy boolean hole emits the attribute) and the guard falsifies all three for action=. A reader following that page writes .action=${handler} on a form and hits a hard error the page says cannot happen. Carve-out noted where the rules are stated.

Two findings not fixed here, both real, both out of scope.

• A function in an on* inline-handler attribute leaks identically (<button onclick=${deleteTodo}> emits the whole body). Verified on all three of onclick, onsubmit, camelCase onClick. Filed as fix: a function in an on* attribute leaks the action source, like action= did #1170, with the counter-argument recorded: React's onClick={fn} takes a client handler whose source already ships with its module, so the catastrophic case needs an author to bind an imported 'use server' action to an inline handler, which is not a Next idiom. Lower severity than action=, still unguarded.
• A reflect: true prop holding a function leaks through _reflectAttribute, which the renderer guard structurally cannot see. Already filed as A reflect:true prop stringifies a function, leaking its source #1169.

Rejected, with the reasoning kept. A commented-out <form action=${fn}> still writes the source, since the comment state commits with a bare String(val). The argument was that this PR creates the reachability, because the new error prompts an author to comment the block out. The jury split and refuted it: the same is true of every value in a comment, the PR's throw does not make commenting-out more likely than any other render error, and the leak-through-comment path predates this change. Left as-is.

Full suite: 3018 tests, and the 59 core failures reproduce identically with my changes reverted (855 tests / 59 fail with, 848 / 59 fail without), all of them the #954 fresh-worktree trap (no built dist/, ws named exports through the symlinked modules). Browser green on Chromium, Firefox and WebKit. Bun table green on 1.3.14.

[vivek7405]

Fifteenth pass. Five confirmed, four rejected, two left unverified when the agent budget ran out. Two of the rejected ones I checked myself and one of those rejections was wrong, which is the finding I would most have regretted taking on trust.

The carve-out this PR documents is false for a reflecting prop. Three surfaces plus two code comments said a custom element's .action is safe because the property "never reflects". A prop declared reflect: true does reflect, through _reflectAttribute, which is not a template commit site and so is somewhere the guard cannot see. Measured both directions:

SSR,    self-assigned reflect:true prop     LEAKS   action="async function leaky(i) { const conn = 'postgres://…"
client, <my-el .action=${fn}> on that prop  LEAKS   into the live attribute
SSR,    <my-el .action=${fn}> binding       clean   (the unserializable value is dropped)

The jury refuted this one 1-1 and I filed it as rejected, then checked it because a doc sentence claiming something is safe is cheap to verify and expensive to get wrong. The conclusion holds for a plain property, which is what all five places now say, with the reflection path pointed at #1169. This is the second time on this PR that "your credential is safe" turned out to be the dangerous sentence, and both times it was written while correcting an overstatement in the other direction.

Narrative debt on a public URL. The troubleshooting page told a reader that four candidate rules were proposed and falsified "while this guard was being written". A user hitting this error next year was never party to that review, cannot verify the count, and gains nothing over the sentence before it. The same artifact was in the agent skill, in form-action.js's docblock ("it already drifted once"), in a render-server.js comment, and in a CI comment ("twice drifted apart"), each describing a defect that existed only between commits on this branch and never shipped. Someone reading them later goes looking through git history for regressions that do not exist. The durable reasons survive without the history; the counts and the incident reports are gone. The Bun table also now says explicitly that it is one measurement on two versions rather than a per-runtime guarantee, since the safe way to skim it was the wrong reading.

The dogfood app still taught the refused shape. examples/blog/CONVENTIONS.md and its workflow.md both told agents to use <form action=…> bound to a server action. An agent following that rule writes the exact binding this PR turns into a hard error, and inside a component the isolation swallows it, so the form silently vanishes in production. Both corrected. The scaffold templates were checked and are clean.

A browser test for the containment fix. AGENTS.md holds that a unit test is necessary but not sufficient for a component change, and the linkedom test substituted a throwing toString on a detached container. The shipped combination is an async render returning .action=${fn} on a native form, where action is a reflected IDL attribute no DOM shim models. Now tested beside the existing rejected-fetch test; reverting the try/catch reds it on Chromium, Firefox and WebKit.

Three findings about renderer state, filed as #1172 rather than fixed here. The review found that the new throw destabilises directive bookkeeping, and it is right, but not about the cause. Reproduced every case with a throw that has nothing to do with this guard (a value whose toString throws), so all three are pre-existing and reachable on main today:

repeat()  1. initial      <li>one</li><li>two</li><li>three</li>
          2. throws mid-walk
          3. VALID render <li>one</li><li>one</li><li>two</li><li>three</li>   <- duplicated, never self-heals, nothing logged
guard()   3. re-render    the guarded region is gone permanently
watch()   the throw reaches the window instead of renderError(), self-heals on the next value

The repeat() one is the bad one: after a single refused commit, a later fully valid render silently duplicates a row forever with no error to point at. What this PR contributes is a much more reachable way to throw during a commit, from an exotic value to a Next-shaped mistake, which is worth saying in the issue and is not the same as causing it. Fixing three renderer state machines inside a security fix would be the wrong trade; #1172 carries the measurements and the per-case acceptance criteria.

Also filed: #1170, a function in an on* inline-handler attribute leaks identically (verified on onclick, onsubmit, camelCase onClick), with the counter-argument recorded that React's onClick takes a client handler whose source already ships, so it is lower severity than action=.

Rejected and left alone. A commented-out <form action=${fn}> still writes the source, argued as newly reachable because the error prompts an author to comment the block out. The jury split and refuted it: every value in a comment behaves that way, and the path predates this change.

Two findings went unverified for lack of agent budget. Worth another round on that basis alone, independent of what this one found.

[vivek7405]

Sixteenth pass, run with a raised agent cap since the last round left two findings unverified on budget. Five confirmed, two refuted. The first one is a defect this PR introduced, which no previous round found and no test on either side of it existed.

The guard refused four shapes that never leaked. The .prop clause was gated on "is not a custom element". That is not the same set as "writes the value into the markup". action is a reflected IDL attribute on <form> and formAction on <button> / <input>; anywhere else both are plain expandos that stringify nothing. Measured before and after:

                                  main      this PR (before)   now
<form .action=${fn}>              renders   THROWS             THROWS
<button .formAction=${fn}>        renders   THROWS             THROWS
<div .action=${fn}>               renders   THROWS             renders, no leak
<button .action=${fn}>            renders   THROWS             renders, no leak
<li .action=${fn}>                renders   THROWS             renders, no leak
<div .formAction=${fn}>           renders   THROWS             renders, no leak

So a delegated-command binding (<button .action=${() => save(row)}>, whose handler reads e.target.action) became a render error, and inside a component the isolation swallows it, meaning that element just silently disappears in production. A guard that breaks working code to prevent a leak that cannot occur there is not a narrower claim, it is a wrong one.

The whole 52-test suite passed before AND after the fix, because nothing covered a .prop binding on a native element that is not a form. Both sides are pinned now, on both SSR machines, the client, and the Bun table. Over-firing again reds 4 tests; removing the prop guard entirely reds 6. That two-sided counterfactual is the thing this clause never had.

The containment fix was fixed at the wrong altitude. Round 14's fix guaranteed _commitAsync never rejects. Its own comment blamed _performRender's .then for having no rejection handler, and then did not add one. update() is a documented override point returning any thenable, so async update() { await this.prepare(); return super.update(); } with a rejecting prepare() reproduced the original wedge verbatim: counter stuck at 1, updateComplete never settling, unhandled rejection. Handled at the awaiting site now, rather than in the one implementation that happens not to reject. I should have done this two rounds ago when I wrote the sentence identifying it.

What "renders the component empty" was hiding. Two things, both of which send an operator to the wrong place:

• The isolation replaces the element through its matching closing tag, so a component's projected children go with it. Put the bad form in a shared header or shell and every route returns 200 with a completely blank body, which reads like a routing failure rather than one broken component.
• On a route with a loading.{js,ts}, the page renders inside a Suspense boundary AFTER the 200 and the shell are flushed, and that catch emits nothing, logs nothing, and never calls onError. The docs told the reader to check the server log for a message that does not exist on that path. Core's equivalent boundary loop does log, so this is two implementations of one policy disagreeing rather than a decision. Filed as fix: a loading.ts route swallows a page render error at 200, unlogged #1174; both doc surfaces now say the silence exists and is tracked.

The comment hole, refuted twice, documented anyway. A hole inside an HTML comment is emitted raw, so <!-- <form action=${createTodo}> --> still ships the whole body with no throw and no log. The jury refuted this as a PR defect both times it was raised and I agree with that: it is pre-existing, and the argument that this PR causes it by prompting people to comment things out is speculative. But "the refusal covers the shape, not one spelling" is written in two places I control, and it is false for this spelling. Commenting the form out is also a genuinely likely next move for someone who just hit the error. Cheaper to name the exception than to defend the absolute.

Also refuted: that the commit-throw recovery leaves a corrupted repeat() instance live. Correct in mechanism and already #1172; not new.

Full suite 861 tests with the same 59 pre-existing worktree failures as every round (verified identical with my changes reverted, two rounds ago). Browser green on Chromium, Firefox and WebKit. Bun table green at 13 refused shapes and 5 carve-outs.

Worth stating plainly: three of the last three rounds found something in the shipped guard rather than in prose, and this one found a regression the previous fifteen missed.

[vivek7405]

Seventeenth pass. Five confirmed, none refuted, and two of them are defects I introduced LAST round while fixing the round before. That is the part worth recording.

A superseded cycle's rejection clobbered the live DOM. The rejection handler I added in round sixteen wrote the error state without the render-token check that both handlers inside _commitAsync already do. Measured:

after B commits :  <p class="live">v=B</p>
after A rejects :  <p class="err">ERR stale cycle A failed</p>     <- A was discarded two ticks earlier

So a cycle that was correctly thrown away could wipe the render that replaced it, and settle()'s own token check then returned early so nothing repaired it. Reachable through the exact path the handler was added for, plus #492's abort of a superseded render's in-flight action surfacing as an AbortError. The check now gates the DOM write only; settle() still runs unconditionally, because the decrement it owns is what stops the counter wedging.

A published-docs typo, from the same round. <code>loading.\${''}js</code>. In a template literal \$ suppresses the interpolation, so the page would have rendered the literal string loading.${''}js to every visitor. Verified by cooking the exact byte sequence rather than by reading it. Written while adding a caveat about a different failure being invisible, which is a fair joke at my expense.

The try/catch had no test that could fail. Round sixteen's rejection handler made round fourteen's test non-discriminating: reverting the try/catch alone leaves both tests green, because clientRender's throw then rejects the promise that IS pendingCommit, and the new handler produces the same two effects in the same order. My test plan claimed "reverting the try/catch reds both", which was true when written and false after the next commit. The distinguishing case is an update() override returning its own thenable, which discards _commitAsync's promise so the rejection has no handler at all; that is now its own test. Three-way counterfactual, each piece reverted alone:

revert the try/catch          -> reds: commit throw with a discarded commit promise
revert the token check        -> reds: superseded cycle rejecting late
revert the rejection handler  -> reds: rejected thenable from update(), and the superseded case

Three pieces, three distinct tests, no overlap. The PR body's claim is corrected rather than quietly dropped, since "the counterfactual proved it" was the evidence offered for that fix.

The comment hole, filed at last. Raised in round fifteen, refuted; raised in sixteen, refuted; confirmed here on the narrow ground that it was the one residual leak path with no tracking issue. Checking it properly changed the shape of the answer: String(fn) returns source text in EVERY hole position, not just a comment.

comment          <!-- <form action=${fn}> -->   leaks
text child       <div>${fn}</div>               leaks
script body      <script>${fn}</script>         leaks
unrelated attr   <div title=${fn}></div>        leaks

So the refuters were right that this is generic String() behaviour and not a bypass of the guard, and the finder was right that it needed tracking. Filed as #1175 for the general case, which subsumes the comment spelling and asks the real question: whether a bare function should be refused in any non-binding hole, since it is never meaningful output. Noted there that it should land after #1172, because a broader throw surface makes that directive-state corruption easier to reach. The agent doc no longer implies comments are special.

Full suite 863 tests with the same 59 pre-existing worktree failures. Browser green on three engines, Bun table green, website typecheck clean.

Three rounds in a row have now found something in shipped behaviour, and this one found two regressions introduced by the previous round's fixes. That is an argument for the loop, and also an argument that fixes made late in a long review deserve the same counterfactual discipline as the original change, which these two did not get.

[vivek7405]

Eighteenth pass, and the last one. Three findings, all minor, all fixed here. The loop stops on this round by decision rather than because a round came back empty, and the reasoning for that is at the bottom.

Error reporting could throw and skip the release it guarantees. String(error) was evaluated in the argument expression, outside _handleRenderError's own try/catch, so a rejection reason that cannot be coerced to a primitive (a null-prototype object, a revoked Proxy) threw a TypeError straight out of the handler:

Promise.reject(Object.create(null))  ->  __pendingAsyncCommits 1, updateComplete NEVER, renderError calls 0

That is exactly the wedge the handler's own comment claims it closes. Not a regression, since main wedges on any rejecting update() at all, but an incomplete close of the hole this PR is fixing. The release moved into a finally and the coercion can no longer throw. Third finding in this area across three rounds, each one a different way the same handler failed to be total.

All three settlement assertions accepted a rejection as success. They measured with .then(() => 'settled', () => 'settled'), which cannot tell resolve from reject. So a change that REJECTED updateComplete instead of resolving it would keep renderError() firing, keep the counter releasing, keep all three tests green, and start throwing inside every await el.updateComplete in app and framework code, which is the unhandled rejection this PR set out to eliminate. The one property most worth pinning was the one the tests declined to assert. They now require resolved.

I wrote those races myself, three rounds apart, and copied the same shortcut each time. It is the identical failure this PR has now hit five separate ways: an assertion is only as good as its ability to observe the thing it claims. Discarded stream chunks, an IDL getter instead of the content attribute, a marker that never reached the string, a counterfactual made non-discriminating by a later commit, and now a race that maps both outcomes to one label.

A doc surface missed. website/app/docs/ssr/page.ts carries a template-hole table stating the attribute, boolean, and native-.prop semantics unqualified. Those are the same three statements I added a carve-out for on the components page, on the same day, and I did not check whether a second page carried them. Fixed.

Full suite 863 tests with the established 59 pre-existing worktree failures. Browser green on Chromium, Firefox and WebKit. Bun table green. Website typecheck clean. All ten CI jobs were green on the previous head and are re-running now.

---

Why this is the last round. The guard itself, which is what #1154 is about, has not produced a finding since round sixteen narrowed its .prop clause, and that narrowing is pinned on both sides. Everything rounds sixteen through eighteen found lives in the async-commit repair added in round fourteen, which was scope creep: necessary scope creep, since this PR ships a doc claiming per-component containment and that claim was false on the async path, but scope creep nonetheless. That code is now three pieces with three discriminating tests and a three-way counterfactual where reverting each alone reds a different test, which is the discipline it lacked when rounds sixteen and seventeen found bugs in it.

The honest read on the loop: rounds five through ten earned their cost, rounds eleven through thirteen were spent re-deriving a transpiler's internals and were mostly waste, and rounds fourteen through eighteen each found something real, with two of those findings being regressions introduced by the previous round's own fixes. That last pattern is the argument both for the loop and against running it forever: late fixes are exactly where the discipline slips, and each new fix is a new thing to review. Stopping now is a judgment that the remaining risk is smaller than the risk of another round of repairs, not a claim that a nineteenth pass would find nothing.