feat: unify form submissions on <form action=${action}> - #1205
Conversation
The renderers now treat an unquoted action= hole on a <form> as a binding
rather than a value: they resolve the action's identity, omit the action
attribute so the form posts to the page's own url, force method="post" and
an enctype, and emit one hidden field carrying the identity.
Every other function-in-an-action shape still refuses, so there is exactly
one way to write this and no silently-broken near-miss. formaction= on a
submit button stays refused: a per-submitter identity would have to ride the
submitter's own name/value pair, which is also how a multi-button form tells
its buttons apart.
The forcing decision waits for the tag's `>` rather than firing at the hole,
because an attribute written after the hole still counts and a hole-provided
method=${m} cannot be read off the source template at all. The scan is a real
tokenizer, since a regex reports a method attribute for a form whose
data-note happens to contain the text.
A non-GET request to a page's own url now runs the server action named by the __webjs_action hidden field, replacing the page `action` export. The action receives the FormData, and its declared validate / middleware / invalidates config exports run here too: skipping them would leave an action protected over RPC and open over a form, which is a privilege gap rather than a missing feature. The identity scheme is the RPC endpoint's, so both transports resolve the same string to the same function. Registering it rides the 'use server' load hook, which is why that hook now installs unconditionally and only seed collection follows webjs.seed: gating identity on seeding would have made seed:false silently break every no-JS form. Origin is verified the way the RPC endpoint verifies it. The page-action path this replaces had no check at all and was shielded only by SameSite=Lax cookies. A submission whose hash no longer resolves re-renders the page at 422 with a resubmit message and the typed values, so a deploy landing under an open form never looks like a silent success. A non-GET that binds nothing is a 405 rather than the old 404, which claimed the url did not exist.
Adds the cross-runtime dispatch proof (identity round trip, multipart upload, CSRF, skew, 405) and runs it in the bun CI job. Every layer it touches is implemented separately by the two runtimes: the load hook that registers identity, FormData parsing, and Web Crypto. The tests scrape the identity out of the rendered page rather than computing it, so they prove the renderer and the dispatcher agree rather than that each half is internally consistent. Fixtures that used the page `action` export move to a bound form, and the blog feedback page moves its logic into a module action, which is what the e2e probe now exercises.
A GET action is CSRF-exempt and rides its arguments in the url, so it cannot
answer a form POST and the dispatcher returns 405. The rule reads the imported
action's own file, so it fires only when the binding really does resolve to
one.
It reads a placeholder redaction rather than either mask: the masks blank a
tagged template outright, so a real html`<form action=${fn}>` would be
invisible, while placeholder mode keeps the hole as live code and collapses a
literal body to one token. That is what keeps a docs page showing this exact
shape as a code sample from failing the checker the framework ships.
The router test now submits an attribute-less form, which pins that the router resolves it to the page's own url and carries the identity field plus a submit button's own name/value. The real-browser render test asserts through `new FormData(form)`, the same serialization a native submission does, so a field emitted outside the form or a method that is not a POST shows up here rather than at runtime.
The identity field has to survive from the renderer into a native submission for anything to happen at all, so the suite now asserts the markup itself before exercising it. A missing field would otherwise surface as an empty POST body and read like an action bug.
Every gallery form now binds an imported action instead of routing through a per-page adapter, and the actions take the FormData directly. The todo example keeps one intent-dispatching action because its rows carry two submit buttons, which is the shape the formaction refusal points at. Generating an app caught a real bug this could not have caught otherwise: the file-storage action's 'use server' sat under an eight-line comment header, past the window the directive scan reads, so the action never registered. It worked before only because the page action called it server-side. Directive hoisted, with a note saying why it goes first. Also drops the last `action=""` from the templates, which the spec treats as a conformance error.
The muscle-memory gotcha was the dangerous one: it taught at length, with a
table, that `<form action=${fn}>` is a hard error, which is now the blessed
shape. It is inverted rather than deleted, because the source-leak reasoning
still explains why every OTHER spelling refuses.
The write-path reference, the optimistic-ui form guidance, and the skill's own
walkthrough move from the page `action` export to the binding. data-and-actions
gains the section it never had: it documented every other way an action is
invoked and said nothing about forms.
The new invariant lands as 12 rather than slotting in at 11, so the prose
invariant keeps the number the hook message and four skill files already cite.
Troubleshooting and the Next migration page were inverted: both taught that
`<form action=${fn}>` is refused, which is now the blessed shape. Server
actions, progressive enhancement, and file storage move off the page `action`
export, and the CSRF pages broaden to say the form path is origin-checked too,
which the old page-action path was not.
Adds troubleshooting entries for the two responses that are new and would
otherwise read as bugs: the 405 on a form that binds nothing, and the 422
resubmit on deploy skew.
The #756 spoof-survival assertion rode a page `action`, so it stopped exercising anything once that export was removed. It now scrapes the identity off the rendered page and submits it, which is also a stronger check: the rebuild it guards happens inside the form dispatcher.
|
Design rationale: why the forcing decision waits for the tag's The obvious place to rewrite the form is at the An attribute written AFTER the hole still counts. The second reason is worse: So the hole records the identity and the The attribute scan is a real tokenizer rather than a regex over the tag text. |
|
Design rationale: identity rides an always-on load hook, not the seed facade Resolving a bound action at SSR needs something that watched every Reusing it directly would have been a bug: seeding is switchable off ( With collection off there is no Proxy at all now, which is a small win the old shape did not have: outside a collector the Proxy was a transparent passthrough anyway, so with seeding disabled it was pure indirection on every action call for the process lifetime. Identity is a WeakMap rather than a property on the function. The wrapped value can be a Proxy whose property writes pass through to the target, so stamping would mutate the app's own function object, and a frozen action would make the stamp fail silently. Both the wrapper and the original are registered, because which one a caller holds depends on the seed switch. The generated browser stub is the mirror image: it stamps its own identity on itself (non-enumerable, so |
|
Decision: Multiple actions per form is legal HTML and the obvious thing to want, so the refusal deserves a reason rather than an omission. A per-submitter identity has to travel with the submitter, and the only channel a browser gives is the submitter's own Neither is worth it, because the shape it enables is already expressible: a form with several buttons binds ONE action and dispatches on the submitter's Refusing states the boundary. Half-supporting it (accepting the attribute and silently using the form's action) would be the worst outcome: a button that looks wired and posts somewhere else. |
|
Decision: a bare Two responses here are new rather than ported, and both were picked over a quieter alternative. 405, not the old 404. Under the page A skew submission re-renders at 422 rather than 404ing. A form held open across a deploy submits a hash the new build has never seen. The three options were a 404, a silent no-op, and a re-render. The 404 discards everything typed; the no-op shows a success page for a write that did not happen, which is the worst of the three. So it re-renders the page with a resubmit message AND the submitted values on the standard Distinguishing skew from a wrong function name matters: an unknown HASH is skew (re-render), while an unknown FUNCTION inside a known file is a real 404. Re-rendering the second would send someone round a loop that can never succeed, so |
|
Discovered while dogfooding: a Generating a scaffold app and booting it caught a bug nothing else in the suite could have, which is the argument for that gate existing.
It was already true before this PR. The file simply worked, because the page Fixed by hoisting the directive to line 1 in the template, with a comment saying why it goes first. A directive prologue is a JS concept that allows comments before it, so the five-line window is the framework's own narrowing rather than a language rule. I left the window alone here: widening it is a change to how every |
This is the property that makes the binding worth having over the @submit + preventDefault shape it replaces: that binding is an interactivity signal, so it forced the component into the browser to do what a plain HTML form already does. Elision and the submission are asserted together, since "the module was elided" alone would pass just as well if the form had stopped working.
Renaming page-action.js left six files describing a module that no longer exists, which is the kind of drift that sends someone looking for the wrong file.
…tion Three real defects from review, plus the tests that would have caught them. sameSiteRedirect rejected `//host` and `/\host` but accepted `/<TAB>/host`. The URL parser REMOVES tab, LF, and CR before parsing, so that reaches the browser as `//host` and resolves cross-origin; Headers rejects LF and CR but a tab is a legal field-value character and rides to the wire intact. The guard now strips those three first and validates what is left, which also makes the returned value exactly what the browser sees. An action module that THREW at import was reported as deploy skew, so a `.server.ts` that fails at load (a missing env var read at module scope) answered every submission with "please submit again" forever while the real error was discarded: no log, no onError. It is now a distinct `load-failed` reason that becomes a sanitized, digest-logged 500, matching what the same module does through a page render or an RPC call. An author-set `enctype="text/plain"` was preserved, so that form ran under JS and 405'd without it, which is the works-one-way-only near-miss this module refuses everywhere else. Refused at render on both renderers. Also scopes `form-action-not-a-get-action` to GET. It flagged PUT/PATCH/DELETE too, which the dispatcher runs perfectly well: a browser form always submits as a POST, and the declared verb governs the RPC transport rather than whether the function can serve a form.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
I went over this with fresh eyes and found three things worth fixing, one of them a real open redirect. Comments inline, anchored on the lines as they stand now.
The redirect one is the only genuinely dangerous find: the same-site guard handled //host and /\host but not the whitespace variant, and the URL parser strips tabs before the origin check ever runs. Worth remembering that any guard reading a URL as a string has to normalize it the way the parser will.
The other two are the same shape as each other: a failure that presents as something benign. A broken action module reading as "deploy skew", and a text/plain form working under JS, both look fine until someone is debugging them at 2am with no log line.
Two test problems too, and they are the kind I care about more than the code: a #756 guard that had gone vacuous (asserting a negative regex against an empty string), and a fixture whose comment claimed to prove a rebuild it had stopped touching. A test that cannot fail is worse than no test, because it reads as coverage.
Beyond these the diff is in good shape. Sharing the RPC endpoint's identity naming is the right call, and the tests that scrape the identity out of the rendered page rather than computing it are doing real work: that is what catches the two halves drifting apart.
The client applied parts in template order and validated at the action hole, so
an attribute written AFTER it had not been committed yet. `<form action=${fn}
method=${'get'}>` therefore passed on the client and threw at SSR, and the
client shipped a form that submits its fields in the query string and never
runs the action, which is the outcome the guard exists to prevent. The bind is
now queued and flushed once the pass finishes, so both renderers see the same
final attributes.
A re-render needed its own answer: it re-applies only the parts whose values
changed, so with the action unchanged nothing revisits the form and an
`enctype=${e}` flip to text/plain slipped through. A write to `method` /
`enctype` on an already-bound form is now checked where it happens.
Also drops `unknown-fn` from the identity union (the caller decides that, not
this function) and fixes two doc lines pointing at the removed export.
The load-failure test is constructed as a submission naming a module the page
never imported, rather than by rewriting a file and re-importing: Bun serves
the cached module and ignores the `?t=` cache-bust query, so the rewrite shape
passed on Node and failed the Bun matrix. The new shape is also the real one.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Second pass over the fixes from the last round. One of them did not hold up, and it is the interesting kind of bug: a fix that is correct in the abstract and wrong in the order it runs.
The enctype guard I added claimed the two renderers refuse identically. They did not. SSR validates the whole start tag when it reaches the >, so it always sees the final attributes; the client validated at the action hole, where an attribute written after it has not been committed yet. So <form action=${fn} method=${'get'}> threw on the server and sailed through on the client, shipping exactly the form-that-posts-nowhere the guard exists to stop. Queued and flushed at the end of the pass now.
Chasing that turned up a second one the reviewer did not raise: a re-render only re-applies the parts whose values changed, so with the action unchanged, nothing revisits the form and an enctype flip lands unchecked. That needed a check at the write site rather than at the bind.
The rest were smaller: an untested onError, a union advertising a value the function cannot return, and a narrowing whose justification had no runtime test behind it.
Worth recording separately: the load-failure test I wrote first rewrote a module and re-imported it with a cache-busting query. That passed on Node and failed the Bun matrix, because Bun serves the cached module and ignores the query. Rebuilt around a shape that does not depend on re-evaluation, which is also closer to what actually happens in production.
The bind queue is module-level, so a pass that throws BETWEEN queueing a form and reaching its flush left the entry behind for the next render to apply. Measured: a template whose form carries method="get" and whose next element throws a formaction refusal poisons the FOLLOWING unrelated render, which fails with the first render's error. Draining at flush time only covers a bind that throws inside its own batch, so each commit pass now discards what it queued in a finally.
Three ways a bound form could still be broken from the client, each in a branch
the write-path check did not instrument.
A REMOVAL (`method=${null}`) took the early `removeAttribute` branch and never
reached the check, leaving a form with no method, which submits as GET and
sends no body. A QUOTED hole compiles to `attr-mixed` rather than `attr`, so
`method="${verb}"` was unvalidated on every re-render, and that is the more
common spelling. And an action hole resolving to a URL string
(`action=${flag ? act : '/legacy'}`) left the form in the bound set forever, so
later `method` writes threw on a form that was no longer bound while the stale
identity field stayed in the markup.
Instrumenting branches one at a time is what missed two of the three, so the
check moved to the end of the pass: a write only NOTES the form, and the flush
re-validates it. Absent is treated as invalid there, because binding always
leaves both attributes present, so absent means this pass removed one.
The tests now hoist the action function. Minting a fresh stub per render made
the action part re-apply and re-bind every time, so each half of the earlier
fix was satisfied by the other and neither was pinned. Measured after hoisting:
reverting the deferral reds 3, reverting the write-path check reds 3 different
ones.
The commit path has five, and every existing binding test went through the two a root-level template uses. The nested-hole and keyed-list sites now have coverage that reds when their flush is removed; the streamed-continuation site is the one with no enclosing pass to fall back on, so a missing flush there would ship a form with no identity field until an unrelated later render drained the queue.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Third pass. The pattern in my last two rounds of fixes was instrumenting one branch at a time, and this round is what that cost.
The write-path check I added covered exactly one of the three ways a bound form's method can change. A REMOVAL (method=${null}) took the early removeAttribute branch and never reached it, leaving a form with no method, which submits as GET. A QUOTED hole compiles to attr-mixed rather than attr, so method="${verb}" went unvalidated on every re-render, and that is the spelling most people reach for first. So the check moved to the end of the pass: a write only notes the form, and the flush re-validates it. One place, every branch.
A separate one, same family: _boundForms was add-only, so a form whose action hole later resolved to a URL string stayed "bound" forever. Later method writes threw on a form that was not bound any more, and the stale identity field sat in the markup, meaning the form would have posted the old action's identity to its new url.
The finding I care most about is the tests. Both halves of the previous round were satisfied by each other: minting a fresh stub inside the template made the action part re-apply and re-bind on every render, so the write-path assertions passed on the strength of the deferral fix and pinned nothing of their own. Hoisting the function fixes it, and I measured each half separately afterwards: reverting the deferral reds 3, reverting the write-path note reds 3 different ones, reverting the release reds 1.
Also filled the flush-site gap. There are five, every test went through the two a root-level template uses, and removing the nested-hole one now reds two tests instead of none.
The removal branch is a separate path from the string one, so `action=${null}`
left the form in the bound set with its identity field intact: it would still
have submitted to the old action, while SSR renders that same template with no
identity at all.
…nforce on release
`method` and `enctype` reach a form through four commit branches, not two.
`?enctype=${b}` toggling on writes an EMPTY enctype, which is unparseable and
which SSR refuses; `.method=` is a reflected IDL attribute, so it writes the
content attribute in a real browser (linkedom keeps it an expando, which is why
that one needs saying rather than testing under linkedom alone). Both are now
noted like any other write.
Releasing a binding also has to take back the attributes the BIND forced, and
only those. SSR of `<form action=${'/legacy'}>` emits no method at all, so a
client that kept `method="post" enctype="multipart/form-data"` would POST
multipart to a url the server renders as an ordinary GET form. What the bind
added is tracked per form, so an author's own `method="post"` survives the
release untouched.
The missing-attribute error explained a method even when the enctype was the
one that went missing, and the test matched loosely enough not to notice.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Fourth pass, and the same lesson as the third: I keep counting the write paths wrong.
Last round I moved the check to the end of the pass so it would cover every branch, and said so. It covered two of four. method and enctype also reach a form through a BOOLEAN binding (?enctype=${b} toggling on writes an empty enctype, which is unparseable) and through a PROPERTY one, which on a form is a reflected IDL attribute and so writes the content attribute for real in a browser. Both noted now.
The release path had the mirror problem: it took back the identity field but left the method="post" and enctype the bind had forced. SSR of the same post-release state emits no method at all, so the client would have POSTed multipart to a url the server renders as an ordinary GET form. It now tracks what the bind added per form and takes back exactly that, so an author's own method="post" survives.
Two of the reported findings did not survive checking, and are worth recording as rejected rather than quietly dropped: ?method=${false} and ?method=${true} were called divergences, but both renderers already agree on them (the first ends at method="post" on both, the second throws on both). The bool gap was real only for enctype.
Also fixed an error message that explained a missing method even when the enctype was the one that went missing, and a test loose enough not to notice.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Two more from the same pass that GitHub would not anchor inline, so recording them here.
packages/core/src/form-action.js, releaseFormAction. It took back the identity field but left the method="post" and enctype the bind had forced. SSR of <form action=${'/legacy'}> emits no method at all, so the client would have POSTed multipart to a url the server renders as an ordinary GET form. Now tracks what the bind added per form and takes back exactly that, so an author's own method="post" survives untouched. Both halves have a test, and reverting it reds two.
The streamed-continuation flush site had no test, which is the one I would least want missing: consumeAsyncStream swallows a throw into a console.error, so a form that failed to bind inside an asyncAppend chunk ships silently instead of failing the render. Covered now, and removing that flush reds it.
The client guard asked "what was just written?" and instrumented each write
path, so every review round found another path it had missed: a removal, a
quoted hole, a boolean, a property, the encoding alias. That question is
unanswerable one branch at a time, because whether a form can submit is a fact
about the whole start tag, and because updateInstance re-applies only the holes
whose own value changed.
The client now reconstructs what SSR would have emitted, from the template's
recorded statics plus this pass's values resolved per part kind, and feeds it
to the same predicate SSR calls. Reading the final DOM cannot work and was the
first thing I tried: `?method=${false}` and `method=${null}` both leave no
method attribute, and SSR resolves them to opposite answers, so the difference
survives only in the template.
The record is built once per template literal in assignPaths, the one place
that sees an element's static attributes and its part indices together. Being a
compile-time constant, it cannot drift the way remembering "did the bind supply
this" at runtime did. Release needs no bookkeeping either: an attribute is the
framework's exactly when the template supplies nothing for it on this pass.
Two tests changed because the old design's messages were inventions. A null
hole now fails with the message SSR actually produces, and it fails on the
first render, which the write-tracking design only caught on a re-render.
A `.method` / `.enctype` / `.encoding` prop on a bound form is dropped at SSR
and applied for real in the browser, where all three are reflected IDL
attributes: measured, SSR emits method="post" while a browser ends at
method="get". Two action holes on one form is the other, SSR emitting the
second as a plain url alongside the identity field while the client resolves
last-wins.
Neither can be reconciled, because the client cannot un-know an assignment it
made and the server cannot know one it never saw, so both are refused. That is
the argument that already refuses `.action` on a form.
Both are properties of the template, checked only once the value proves the
form is really bound, so an ordinary `<form action=${'/search'} .method=${m}>`
is untouched. Duplicate holes are counted by HOLE rather than by value, so the
refusal does not depend on what the second one resolved to, which is what keeps
the client's compile-time count and SSR's scan agreeing.
The per-tag shape state resets at every `>`, bound or not, so one form's prop
binding cannot refuse an innocent later form.
Rethrowing reached the handler's last-resort catch, which reported the SAME error to onError a second time and answered a bare 500 with no digest, so the one thing that makes a prod error diagnosable was lost. The RPC path already answers this case itself.
Five rounds each found the client and SSR disagreeing about the same feature in a new way, because each was checked against its own expectations rather than against the other. One template now drives both and the outcomes are compared, so a divergence is a failing row rather than a review finding. Lives in the browser suite deliberately: method / enctype / encoding are reflected IDL attributes, and linkedom implements no reflection, so a prop row asserted node-side would pass for the wrong reason. Attributes are compared sorted, since order differs between appending to a string and setting properties and neither renderer promises it.
Invariant 12 and the muscle-memory refusal table gain the property-binding and
duplicate-hole refusals, and state the rule the redesign turns on: WebJs judges
method and enctype from your TEMPLATE, not the rendered element, so
?method=${false} emits nothing and is supplied while method=${null} renders an
empty value and is refused. Both leave no attribute in the DOM, which is
exactly why the element cannot be the source of truth.
|
Design rationale: why the client guard was rewritten rather than patched again Five review rounds, five defects, all in the client-side attribute guard, each one found in the previous round's fix. That is not bad luck, it is a design that cannot converge, and it is worth writing down why before someone tries the same shape again. The guard asked what was just written and instrumented each commit branch. Four branches can write a form's The property being enforced, can this form submit, is a fact about the whole start tag. SSR never had any of these bugs because it validates the assembled tag at its The obvious alternative is wrong, and I built it first. Reading the final DOM at end of pass looks like it should work and cannot: Both leave no Two consequences worth stating because they look like holes: A write from OUTSIDE the template (a Two shapes are refused rather than reconciled. A |
Round 1 of the deep review found four ways a bound form could still render
differently with JS than without it, all of them shapes neither renderer
refused.
A static action="/url" beside the bound hole survives SSR (the hole drops
only its own) while the client's reconcile removes it, so the no-JS
submission posts somewhere else entirely. An encoding= CONTENT attribute is
inert in a browser but the client folded it into enctype, so the same form
uploaded multipart without JS and urlencoded with it. A padded method=" post "
passed the guard and was emitted untouched, and an enumerated attribute
matched against exact keywords falls to its invalid-value default, so the
form submitted as a GET with no body. And recording only the FIRST action
hole meant `action=${url} action=${boundFn}` took the release path on the
client while SSR threw.
The release path had its own version of the same mistake: it decides which
attributes to remove by asking whether the template supplies anything for
them, and a .method property binding is not an attribute part, so it wiped
the author's own reflected value on every re-render.
The parity table gains a row per shape, and the two reflection-dependent
ones live in the browser file because linkedom's HTMLFormElement reflects
nothing and would pass them for the wrong reason.
Identity rides the 'use server' load hook, and the hook only registers a function the facade wrapped. The facade declines to wrap in two cases that used to be harmless (no seeding for that module, RPC unaffected) and are now a hard render failure: a module containing `export *`, and one whose source merely mentions it. The pattern was /\bexport\s*\*/ over the raw source and \s* spans newlines, so the word "export" ending a JSDoc line matched the next line's leading asterisk. Reflowing a comment could turn a working no-JS form into a 500 on the page rendering it, with an error saying the author's action is not a server action. The `export *` bail-out is obsolete anyway: #538 gave the facade its own `export * from` catch-all covering exactly what the bail-out protected, and nobody removed it. A star re-export now facades like any other module. Three more in the same area. A re-export barrel is faceted too and its body runs AFTER the module it re-exports from, so an unconditional registration re-filed the function under the BARREL; the dispatcher then read validate / middleware / method / invalidates off a namespace carrying none of them and ran the submission with the action's validation and auth middleware skipped. Registration is now first-wins, which is the defining module. An action the index does not contain was given a minted hash the dispatcher could never resolve, so every submission answered "please submit again" forever with nothing logged; it returns null now and the renderer refuses loudly. And the scan fallback cache-busted its own imports in dev, comparing function identity against fresh module instances that could never match, so on the one runtime it exists for it imported every action module and returned null. Also on the dispatch path: the rebuilt Request dropped req.signal, so actionSignal() never fired on a client disconnect; and invalidates tags were evicted server-side but never reported, so the browser tag coordinator kept serving a stale cached GET. The two scaffold-shipped tests that POST to the signup page were still sending no identity, taking a 405 and reporting it as an unmigrated database. They, and every app test after them, now use a first-class submitForm() helper rather than hand-rolling the scrape.
Four surfaces stated the formaction refusal as a mechanism impossibility (a per-submitter identity 'would have to ride the submitter's name/value pair, which is how a multi-button form tells its buttons apart'). That reasoning is wrong, #1207 uses exactly that channel, and in an AI-first framework a false browser-mechanics constraint in the docs is what the next agent reasons from. All of them now say scoping decision, tracked in #1207. Invariant 12's 'every near-miss throws' needed the same honesty: the guarantee is form-level and stops at the submitter, so a button's formmethod / formenctype can still defeat a bound form with neither renderer noticing. Stated as a known gap rather than left to contradict #1207. Three stale-by-rename comments: packages/server/AGENTS.md still named registerSeedHooks / __seedWrap and still said the hook installs only when seeding is on, which is the exact wrong conclusion the unconditional install exists to prevent; and two ssr.js comments described a pageModule seam form-dispatch.js never uses (the loader is no longer exported at all). The segment-middleware line read as though the page's middleware gates the action. It does not: an identity names the action, not the page, and the same action is reachable at its RPC endpoint where no segment middleware runs. Says so now, and points at where authorization belongs. submitForm is documented in testing.md as the way to drive a no-JS write path, so no app test hand-rolls the identity scrape again.
Every existing test of the client half stubs one end. The linkedom tests and the browser parity table hand-stamp a stand-in function with the identity key; the browser router test hand-writes the hidden input into its template; and the e2e fixture at /feedback is a plain page with no components, so it never hydrates and reconcileFormAction never runs there at all. The shape the scaffold actually ships is the opposite: the todo gallery binds an action on a component that also carries @submit and optimistic state, so it ships, hydrates, re-renders, and rebuilds the identity field from the stamp the GENERATED stub puts on itself. Anything that stopped the browser-served stub carrying that stamp would throw in the component's render and blank the card, with every suite green. /feedback/live is that shape, and it asserts hydration happened before reading the form, so the assertions cannot pass on the SSR markup alone.
…pares It defaulted DATABASE_URL to ./dev.db while .env.example and drizzle.config.ts both use db/dev.db, so the signup flow found no users table and skipped with 'run db:migrate' -- and running db:migrate migrated a different file, so the skip was permanent. Verified by generating an app: signup -> login -> dashboard now runs for real (0 skipped) instead of reporting a database that was already set up.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Review round 1 (deep tier): 23 findings, all addressed
Ran at the deep tier because the client guard failed five rounds before the rewrite, so "the redesign is probably clean" was not something to assume. 16 agents, 9 lenses, adversarial jury. It returned 3 confirmed and 20 unverified, where unverified means the jury budget ran out at 12 of 26 deduped rather than that the findings were doubted. I triaged all 20 myself; every one reproduced.
The redesign held: not one finding was in the shared-oracle mechanism. No missed commit branch, no stale bookkeeping, no queue poisoning. That was the class the rewrite was meant to close, and it closed. What round 1 found instead was a ring of shapes around it.
Where the real risk turned out to be: action identity
The worst finding is not in the renderer at all. Identity rides the 'use server' load hook, and the hook only registers a function the seed facade wrapped. The facade declines to wrap when the source matches /\bexport\s*\*/, and \s* spans newlines, so the word "export" ending a JSDoc line matches the next line's leading asterisk. Before this PR that cost a seed (documented fail-open). Now it means <form action=${fn}> throws "is not a server action" at SSR for a function that works perfectly over RPC. Reflowing a comment could 500 a page.
The export * bail-out was obsolete anyway: #538 gave the facade its own export * from catch-all covering exactly what the bail-out protected, and nobody removed it.
Three more in the same subsystem, each independently able to break a form:
- A re-export barrel stole the identity.
export { createTodo } from './create.server.ts'is faceted too, and its body runs after the module it re-exports from, so an unconditional registration re-filed the function under the barrel. The dispatcher then readvalidate/middleware/method/invalidatesoff a namespace carrying none of them, and ran the submission with the action's validation and auth middleware silently skipped. That is the exact protected-over-RPC / open-over-a-form gap this PR claims to close. - An unindexed action minted a hash the dispatcher could never resolve, so every submission answered "please submit again" forever, with nothing logged.
- The scan fallback was dead on the one runtime it exists for: it cache-busted its own imports in dev while matching on function-object identity, which a fresh module instance can never satisfy.
Renderer shapes that still disagreed
Four, none in the oracle, all now refused or ignored identically by both sides with a parity-table row each:
| shape | what happened |
|---|---|
<form action="/legacy" action=${fn}> |
SSR keeps the static attribute (the hole drops only its own), the client removes it: no-JS posts to /legacy, JS posts to the page |
encoding=${...} as an attribute |
the client folded it into enctype, SSR ignored it, so the same form uploaded multipart without JS and urlencoded with it |
method=${' post '} |
enumerated attributes match exact keywords with no whitespace stripping, so it fell to the invalid-value default and submitted as a GET with no body. The guard trimmed before checking, then emitted the padded value |
action=${url} action=${boundFn} |
the client recorded only the first action hole, took the release path, and shipped a form SSR refuses outright |
Plus the release path's own version of the same mistake: it decides what to remove by asking whether the template supplies anything, and a .method property binding is not an attribute part, so it wiped the author's own reflected value on every re-render.
Two tests that were not testing anything
- The scaffold's
auth.test.tsandgreet.test.tsstill POST to the signup page without__webjs_action, take a 405, and report it as an unmigrated database. Every assertion past that point (session cookie, dashboard render,actionContext()) has been silently dead since this PR. - Chasing that turned up a second cause: the auth test defaults
DATABASE_URLto./dev.dbwhiledb:migratemigratesdb/dev.db, so the skip message told you to run a command that migrated a different file. Fixing only the 405 would have left the test just as vacuous.
The root cause is a DX hole this PR opened: binding an action made the hidden identity field load-bearing, and shipped no test-time way to get it. So submitForm() is now a first-class @webjsdev/server/testing helper. Verified by generating an app: both tests now run for real, 0 skipped.
One test of mine that was not discriminating
I added an e2e case for a hydrated component's bound form and it passed with reconcileFormActions disabled entirely, because hydration only patches the SSR'd form, identity field included. Rewrote it around a row created in the browser, which has no server markup to inherit. It reds correctly now.
That needed a second local discovery: e2e loads the built packages/core/dist bundle, not src/, so my first counterfactual was measuring stale code. CI is unaffected (npm ci runs core's prepare), but a local e2e counterfactual has to rebuild dist or it proves nothing.
Docs that stated a debunked claim as fact
Four surfaces said formaction=${fn} is refused because a per-submitter identity "would have to ride the submitter's name/value pair, which is how a multi-button form tells its buttons apart". That reasoning is wrong, #1207 uses exactly that channel, and in an AI-first framework a false browser-mechanics constraint in the docs is what the next agent reasons from. All now say scoping decision, tracked in #1207.
Invariant 12's "every near-miss throws" needed the same honesty: the guarantee is form-level and stops at the submitter, so a button's formmethod / formenctype still defeats a bound form with neither renderer noticing. Written down as a known gap rather than left to contradict #1207.
One thing I deliberately did not fully close
invalidates tags now ride the response, but fetch follows the success 303 transparently and JS cannot read a redirect's headers. The 422 path carries them and the router acts on them; after a successful PRG the client learns nothing. Closing that needs cross-request state (a cookie), which is not worth inventing here. It is stated in the code comment rather than left to look like an oversight. The redirect's own render is server-side and seeds fresh data, so what stays exposed is a later client refetch of a separately cached GET key.
Verification
Node 3600 (one unrelated load-flake, dev-extra-watch, 3/3 green in isolation) · Browser 640/630/640 across Chromium/Firefox/WebKit, 0 failed · E2E 82/82 against a freshly built bundle · Bun matrix 271 pass, 0 genuine failures · scaffold generates, checks, boots, both bound forms render identities, app tests 4/4 with 0 skipped · webjs check clean on blog and website.
Every fix has a counterfactual proven at this commit: reverting it reds a named test. Twelve of them, listed per-commit.
…ing module The delta review caught a regression I introduced. Preferring the defining module is right when it is reachable, but the action index only walks the app tree, so an action defined in a linked workspace package and re-exported through an in-app 'use server' barrel has an indexed name ONLY under the barrel. Filing it under the defining path alone made that identity unresolvable, and returning null there turned a page that used to render into a 500 saying the author's action is not a server action. Every registration is kept now, defining module first, and the resolver takes the first one the index knows. The defining module still wins whenever it is indexed, which is what keeps a barrel from hiding the action's validate / middleware / method / invalidates config exports. When nothing resolves, the server logs the real cause once, naming the file and the app directory, because the renderer's message can only say "not a server action" and that would send the author looking in the wrong place. Three more from the same round, all mine: hasStar became dead code when its only consumer went away, which also made the comment-regex test vacuous: reverting the tightened pattern left it green. Removed the field, corrected the JSDoc that still described the deleted bail-out, and re-pointed the test at the behaviour that survives. The submitForm comment-stripping test put its decoy form in a TEXT HOLE, which SSR escapes, so no comment ever reached the page and the strip was never exercised. It is a literal template comment now, and reverting the strip reds it. The abort-signal comment claimed the same action cancels correctly over RPC. That is false on Node, where toWebRequest builds its Request with no signal at all, so nothing aborts on either transport. The forward is real and matters on Bun; the comment and the test title now say only that.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Review round 2 (delta, b66d7e31..e56c42eb): 6 findings, all addressed
Scoped to the round-1 fix commits. It cleared two categories outright and found six real problems in the rest, three of them defects I introduced in round 1 and two of them round-1 tests of mine that did not test their own fix.
The regression: I broke a case that used to work
Round 1 made identity prefer the defining module (so a barrel cannot hide an action's config exports) and return null instead of minting an unresolvable hash. Each half is right. Together they are not.
The action index only walks the app tree. So an action defined in a linked workspace package and re-exported through an in-app 'use server' barrel has an indexed name only under the barrel. Filing it under the defining path alone made the identity unresolvable, and the null then produced a 500 reading "the function in action= on <form> is not a server action" for a function that plainly is one, on a page that rendered before my change. I traded a silent-wrong-config bug for a hard failure on a shape that worked.
Every registration is kept now, defining module first, and the resolver takes the first one the index knows:
- defining module indexed: it wins, so the barrel still cannot hide
validate/middleware/method/invalidates - defining module outside the app tree: the barrel carries it, and the form works
- nothing indexed: still null, but the server logs the real cause once, naming the file and the app directory, because the renderer's message can only say "not a server action" and that sends the author looking in the wrong place
Verified against the reviewer's own fixture (action in node_modules, in-app barrel, <form action=${createTodo}>): GET 200, identity emitted, submit 303. Reverting either half reds a named test.
Two round-1 tests of mine that were not testing their fix
Exactly the standard I set for this loop, applied to me.
hasStarbecame dead code when I deleted its only consumer, so reverting my tightened regex left the "a comment does not change how a module loads" test green. The guarantee is now structural rather than regex-shaped (a star re-export facades like everything else), so I removed the field, corrected the JSDoc that still described the deleted bail-out, and re-pointed the test at what survives.- The
submitFormcomment-stripping test put its decoy form in a text hole, which SSR escapes, so no HTML comment ever reached the page and the strip was never exercised. It is a literal template comment now, and deleting the strip reds it. The</form>-in-an-attribute half was discriminating already.
A comment of mine that was false
I justified forwarding req.signal with "while the same action cancels correctly over RPC". On Node that is wrong: toWebRequest builds its Request with no signal at all, so nothing aborts on either transport there. The forward is real and does its job on Bun, where Bun.serve supplies a live signal. The comment and the test title now claim only that, and name the Node listener gap as sitting ahead of this line rather than being closed by it. That gap is pre-existing and outside this PR.
One divergence kept deliberately, now written down
The release path's new prop carve-out leaves an unbound <form action=${url} .method=${'post'}> with method set on the client and absent at SSR. Before, the client removed it and matched by accident. This is the ordinary "a .prop on a native element drops at SSR" rule applying to an ordinary form, identical to what the same template does with no action hole at all, and the alternative destroys the author's binding on every re-render. Recorded in the code so it is not re-filed as a parity bug.
Cleared
Parity across renderTemplate / streamTemplate / client: no new refusal landed in only one renderer. Each shape was traced through all three (method=" post ", ?method=${true}, ?method=${false}, method=${null}, the encoding= attribute versus the .encoding property, and the bound-hole-written-second case). One message-only difference where a template violates two rules at once: both refuse, they just name a different one first.
Prose docs: invariant 12, the three new gotchas rows, the invalidates-receives-FormData bullet, the segment-middleware correction, the submitForm section, both website pages, and the registerActionHooks rename all check out against the code. One apparent scaffold-skill drift was a stale local artifact; that copy is gitignored and regenerated at prepack.
Also verified with real fixtures rather than reasoning: removing the export * bail-out is safe for name collisions with the catch-all, a star-only module, export * as ns from, and a circular star re-export.
Verification
Node 3601 (3600 pass, 0 fail) · Browser 640/630/640 across Chromium/Firefox/WebKit, 0 failed · E2E 82/82 against a freshly built bundle · Bun matrix 271 pass, 0 genuine failures.
Four new counterfactuals, each reddening a named test.
Not marking ready. Two rounds have now found real defects, one of them mine, so the next round decides.
… misdiagnosing Round 3, all three mine. The fallback keeps a page working when the defining module is outside the action index, but the dispatcher then reads validate / middleware / method / invalidates off the BARREL, and a re-export carries only the function. That is the exact outcome the defining-module preference exists to prevent, reached through the other door, and it was silent. It still falls back rather than refusing, because the RPC endpoint resolves the same barrel hash the same way: both transports agree, and refusing would break a page that works. But it warns now, once per defining module, naming what will not run. The unindexed warning asserted one cause and gave a remedy that can fail. A symlinked appDir makes the walked and loaded paths differ for a file that IS inside the app, so blaming a package that does not exist sends the author somewhere there is nothing to find; it lists the causes now. And "re-export it from a use server module" silently does not work for a star re-export, which cannot be enumerated, so the facade never wraps it and the page fails exactly as before. The message says NAMED. The once-per-file assertion did not pin once-per-file: only one resolve happened inside the capture window, and the helper warns at most once per call anyway, so deleting the guard left it green. It resolves twice now. Plus the leftover first-wins paragraph that duplicated and contradicted the append-based one, and the module map still naming actionIdentityOf for a path that now goes through actionIdentitiesOf.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Review round 3 (delta, e56c42eb..573a2f5c): 3 findings, all mine, all addressed
Scoped to the round-2 fix commit, because round 2 had found that a round-1 fix introduced a regression. The question this round asked was whether fixing that regression introduced another one.
The array-based identity itself is sound
Verified with real modules loaded through the installed hook, on Node and Bun:
- a barrel imported BEFORE the defining module still lists the definer first (ESM evaluates dependencies before importers), so the ordering cannot race
- a diamond (two barrels over one definer) keeps the definer first
- an alias re-export registers both names, correctly
- dedup holds: 5000 nested registrations of the same
(file, fnName)leave the list at length 1, and a dev cache-bust produces a new function object rather than growing an existing list.nin the O(n) scan is the number of distinct modules exporting one function, which is a handful - nothing else consumes the accessor in a way that depended on the old single-entry shape
What it found: the fallback was silent about what it costs
Round 2's fallback keeps a page working when the defining module is outside the action index. But the dispatcher then loads whatever module the identity names and reads validate / middleware / method / invalidates off THAT namespace, and a re-export carries only the function. So a form bound through the fallback runs with the action's validation and auth middleware skipped, which is the exact outcome the defining-module preference exists to prevent, reached through the other door.
It still falls back rather than refusing, and the deciding fact is one the reviewer measured: the RPC endpoint resolves the same barrel hash the same way, skipping the same config. So this is parity between the two transports, not a form-specific hole, and refusing would break a page that works while leaving the RPC path open anyway. What was wrong is that it was silent. It warns now, once per defining module, naming exactly what will not run.
The warning I added in round 2 misdiagnosed
Two defects, both reproduced by the reviewer:
- It asserted one cause. There are at least two, needing opposite fixes: the module is genuinely outside the app tree, or it is inside but was reached through a symlinked
appDir, so the walked path and the loaded path differ (the index stores the walked path; the ESM loader realpaths the module). In the symlink case every action in the app trips it and the author is confidently sent looking for a package that does not exist. It lists the causes now. - Its remedy could silently fail. "Re-export it from a
'use server'module inside the app" does not work forexport * from, which cannot be enumerated, so the facade never wraps it and the page fails in exactly the same way. The message says NAMED, and says why.
A test of mine that did not test its own claim
assert.equal(warned.length, 1, 'exactly one warning, not one per render') could not fail: only one resolveActionIdentity call happened inside the capture window, and the helper warns at most once per call regardless, so deleting the _warnedUnindexed guard left it green. It resolves twice now, and removing the guard reds it.
Also cleaned up
The leftover first-wins paragraph that duplicated and contradicted the append-based one directly below it, and packages/server/AGENTS.md still naming actionIdentityOf for a path that now goes through actionIdentitiesOf.
I kept actionIdentityOf rather than deleting it as dead: unlike hasStar in round 2, it has real consumers (six test assertions across two files) and is the natural accessor for the defining entry.
Cleared
Nothing rounds 1 and 2 fixed was broken: a star re-export still facades and wraps its own exports, the JSDoc-comment case still produces a wrapped export, and a barrel still cannot hide config exports when the defining module is indexed.
The reviewer independently confirmed the submitForm fixture fix is real (the literal comment survives SSR verbatim, and reverting the strip makes the helper post the ghost identity), and re-verified the Node-vs-Bun signal claims in the round-2 comment.
One pre-existing crash surfaced and confirmed as NOT from this PR: two 'use server' modules that re-export from each other in a cycle fail at load with ReferenceError: Cannot access 'fn' before initialization, because the facade turns export function into export const. Restoring the old bail-out reproduces it byte-identically, so it predates #1155. Untracked so far; exotic and unrelated to form actions.
Verification
Node 3602 (3601 pass, 0 fail) · Browser all three engines green · Bun matrix 271 pass, 0 genuine failures.
Two new counterfactuals, each reddening a named test, including the once-per-file guard the previous assertion could not catch.
Round 4, both mine. The warning fired on any fallback and then stated the consequence as fact: the action's validate / middleware / method / invalidates "does NOT run". It cannot know that. Whether anything is lost depends on the resolved module's namespace, which this path deliberately does not load (a render-time resolve that eager-loads every action module is the cost scanForIdentity exists to avoid). Two ordinary shapes made it false: an action with no config exports at all, which is most of them, and a barrel that re-exports the config alongside the function. The second was one of the remedies the message itself recommended, so following its advice left it printing the same false claim. It states the condition now, and says plainly that nothing is wrong if the action declares none of them. The unindexed message presented its two causes as exhaustive; a symlinked subdirectory inside the project is a third. "Common causes" now. And the "no warning when the defining module IS indexed" half of the new test was vacuous: it reused the same defining path the first half had already put in the dedupe set, so the assertion held no matter what the resolution did. Measured: deleting the entire condition it names left every test in the file green. It uses fresh paths and a fresh function now, and the same mutation reds it.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Review round 4 (delta, 573a2f5c..eb45e4e8): 2 findings, both mine, both addressed
A diagnostic that stayed wrong in the state it told you to reach
The bypass warning added in round 3 fired on any fallback and then stated the consequence as fact: the action's validate / middleware / method / invalidates "does NOT run". It cannot know that. Whether anything is lost depends on the resolved module's namespace, which this path deliberately does not load, since a render-time resolve that eager-loads every action module is exactly the cost scanForIdentity exists to avoid.
Two ordinary shapes made the claim false, and the reviewer reproduced both:
- an action with no config exports at all, which is most of them, still got told its config would not run
- a barrel that re-exports the config alongside the function (
export { updatePost, validate } from ...) loses nothing, and the reviewer measuredvalidate actually present on the dispatched namespace? truewhile the warning still asserted the opposite
The second is the sharper one: re-exporting the config alongside was a remedy the message itself recommended, so following its advice left it printing the same false claim. The message states the condition now, shows the re-export form that carries the config, and says plainly that nothing is wrong if the action declares none of them.
Also in the same message pair: the unindexed warning presented its two causes as exhaustive. A symlinked subdirectory inside the project is a third (fs-walk skips those). "Common causes" now.
A test half that could not fail
The "no warning when the defining module IS indexed" assertion reused the same defining path the first half of the same test had already inserted into the dedupe set, so warned2.length === 0 was guaranteed by the guard regardless of the resolution logic. The reviewer proved it: replacing if (entry !== known[0]) warnConfigExportsBypassed(...) with an unconditional call, deleting the entire thing the assertion names, left all 7 tests green.
Fresh paths and a fresh function now. The same mutation reds the named test.
Cleared
The entry !== known[0] condition itself is sound: actionIdentitiesOf returns the live array, the loop is synchronous with no await inside, entries are deduped on (file, fnName), so there is no rebuild path that could give a same-content different-reference element. It cannot warn when the resolved file equals the defining file either, since the same file implies the same lookup result.
_warnedBypass keyed per defining file means two actions in one file warn once. Bounded and not a defect: one-action-per-configured-file makes a two-action file one with no config exports, so nothing is bypassed.
Nothing rounds 1 to 3 fixed was broken. The defining-module preference and the unindexed-returns-null case both still hold and both are still discriminating under mutation.
Process note
I lost these two source edits once mid-round: I ran the counterfactual with git checkout while the fix was still uncommitted, which restored HEAD and discarded it. That is the documented trap, and it is the second time this PR has hit it. Redone with the fix committed first, which is the only order that works.
Verification
Node 3602 (3601 pass, 0 fail) · Bun matrix 271 pass, 0 genuine failures · the round-3 counterfactuals re-run and still discriminating.
Round 5, both mine.
The message told the author to re-export the action's config alongside it
("export { updatePost, validate } from ..."). That is actively harmful.
Config is read off the dispatched namespace BY NAME, with no association
back to a particular function, and a barrel re-exports more than one action
by construction, which is the only reason this warning fires at all. So
following the advice attaches one action's validator to every sibling
dispatched through that barrel. Verified: a barrel carrying createTodo plus
a re-exported validate rejects every createTodo submission with pkgB's
validator. `webjs check` cannot catch it either, because
one-action-per-configured-file scans for `export const validate` and a
re-export clause never matches. The closing "nothing is wrong if the action
declares none" was false in exactly the state the snippet produced.
The remedy is now moving the action inside the app, with an explicit
do-not-do-this for the barrel trick and the reason.
Three properties of these messages had no assertion at all, so reverting
each left the whole file green: the "Common causes" wording round 4 asked
for, the remedy text, and the closing line. Pinned, along with the argument
order, which was also unpinned: swapping defining and used makes the message
name the barrel as the action's own module and sends the author to the wrong
file.
Also the inline comment that still asserted unconditionally what the warning
stopped asserting, ten lines above the JSDoc explaining why it must not.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Review round 5 (delta, eb45e4e8..091e24b8): 2 findings, both mine, both addressed
The streak continues. Round 5 found that the round-4 message fix contained advice that is not merely inaccurate but harmful.
The remedy I recommended breaks sibling actions
Round 4's rewrite closed with a copy-pasteable snippet: export { updatePost, validate } from .... Config is read off the dispatched namespace by name, via actionConfigFn(mod, 'validate'), with no association back to a particular function. And a barrel re-exports more than one action by construction, which is the only reason this warning fires at all.
So following the advice attaches one action's validator to every sibling dispatched through that barrel. Reproduced, verbatim output:
createTodo -> validate found: true {"fieldErrors":{"title":"updatePost validator ran"}}
updatePost -> validate found: true {"fieldErrors":{"title":"updatePost validator ran"}}
createTodo declares no config of its own, so the message's closing reassurance told its author nothing was wrong while pkgB's validator rejected every one of its submissions. webjs check cannot catch it either: one-action-per-configured-file scans for export const validate, and a re-export clause never matches that pattern.
The remedy is now moving the action inside the app, with an explicit do-not-do-this for the barrel trick and the reason why.
Three claims of that same message had no test at all
Reverting each left the entire file green:
- the
Common causes:wording round 4 asked for (the existing assertion matched both spellings) - the remedy text
- the closing "loses nothing" line
And a fourth, pre-existing: swapping the arguments to warnConfigExportsBypassed makes the message name the barrel as the action's own module and the defining module as what it dispatches through, sending the author to the wrong file. Nothing pinned the roles.
All four are pinned now, each verified to red under its own mutation.
Cleared
State leak: _warnedBypass and _warnedUnindexed are process-global and never cleared, but no test in the file is shadowed by an entry another test added, and the mutation results prove both halves reach the guarded code rather than passing by dedupe.
Discrimination of the behavioural assertions: six separate mutations each red the named test, including deleting the entry !== known[0] condition (which the round-4 version survived), reversing the identity order, and deleting either dedupe guard.
Verification
Node 3602 (3601 pass, 0 fail) · Bun matrix 271 pass, 0 genuine failures · ten counterfactuals across this round and the last, each reddening a named test.
Round 6, and it is the same class of defect round 5 caught, one direction over. The message said the action's own config does not travel with a re-export, and closed with "an action that declares none of them loses nothing here". That second half is false. Config is matched BY NAME off the dispatched module, so it flows the other way too: a module that re-exports an action AND declares a validate for one of its own exports applies that validator to the re-exported action. Verified against the real dispatcher: a config-less updatePost dispatched through such a barrel answers 422 with a field error written for a different action. The method variant answers 405 naming an action that declares no method at all. Worse, the previous round PINNED that false sentence with an assertion, so the claim was protected by a test. The test now pins both directions and asserts the sentence is gone. Two more from the same round. "A barrel re-exports more than one action by construction, which is the only reason this warning fires" is false: a wrapper re-exporting exactly one action warns too, and there adding the config to the wrapper is safe and is the ONLY option when the action lives in a workspace package several apps share. So the blanket "do NOT" was wrong and the sole remedy (move it into the app) was unavailable for the very case the code names. The remedy now offers both, with the condition that makes one of them safe. And a new test pins the BEHAVIOUR rather than the wording: if config ever becomes per-function, that test fails and the message has to change with it.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Review round 6 (delta, 091e24b8..3a090645): 2 findings, both mine, both addressed
Round 5's finding was that the warning described the config bypass in one direction. Round 6's finding is that the fix described it in one direction too, just a different half of the sentence. Same class, one round apart.
Config flows both ways, and I only fixed one
actionConfigFn(mod, name) is a bare name lookup off the dispatched module, with no association back to a particular function. So:
- config declared beside the action does NOT travel with a plain
export { fn } fromre-export (what the message said) - config the DISPATCHING module declares DOES apply to this action, written for one of its own exports (what the message missed)
The second is why the closing line, "an action that declares none of them loses nothing here", was false. Verified against the real dispatcher: a config-less updatePost re-exported through a barrel that has a validate for its own sibling answers 422 with a field error written for a different action. The method variant answers 405 naming an action that declares no method at all. webjs check catches neither, because one-action-per-configured-file counts export function / export const, so a re-exported action is invisible and the file reads as single-action.
Worse: round 5 pinned that false sentence with an assertion, so the claim was protected by a test. The test now pins both directions and asserts the sentence is gone.
The blanket "do NOT" was wrong, and the only remedy offered was unavailable
"A barrel re-exports more than one action by construction, which is the only reason this warning fires" is false. A wrapper re-exporting exactly one action warns too, and in that state adding the config to the wrapper is safe and is the ONLY option, because the action lives in a workspace package several apps share, which is the case the code itself names as producing this warning. So the message forbade the one thing that works while offering a remedy ("move the action inside the app directory") that case cannot take.
Both are offered now, with the condition that makes one of them safe.
A test that pins behaviour instead of wording
Six rounds of this warning being subtly wrong about the mechanism suggests asserting on message text is not enough. There is now a test driving the REAL dispatcher: a config-less action re-exported through a module with its own validate gets 422 with the sibling's error. If config ever becomes per-function, that test fails and the message has to change with it, rather than quietly becoming wrong again.
Cleared
Assertion discrimination: every pinned string reds a named test under mutation, including the argument-order pair (the reviewer confirmed the regex escaping is right and that both anchored assertions cannot pass with the arguments swapped).
Regressions: none. Node suite green.
Verification
Node 3603 (3602 pass, 0 fail) · Bun matrix 271 pass, 0 genuine failures · three new counterfactuals, each reddening the named test.
…sidue Round 7, both in the sentence round 6 added. The dedicated-module remedy was stated as sufficient, and it is not on its own: among indexed re-exporters the identity goes to the first one LOADED, and module load is lazy per route, so a shared barrel that also re-exports the action can silently win over the dedicated module depending on which page a visitor hits first. Measured: the same app answers the same submission with the barrel's sibling validator or the dedicated module's own, decided by whether the barrel's page was rendered first. The message now states the condition (no other in-app module may re-export it) and why. And the remedy cannot silence the warning. The defining module stays outside the index, so the fallback still runs, and the check cannot see inside the dispatched module to know the config now travels with it. For the workspace package case, which the JSDoc names as the one this remedy exists for, that meant a warning on every boot prescribing what the author already did, the exact diagnostic-persists-after-its-own-remedy class round 4 caught. The message now says the notice keeps printing and that the remedied state is working as intended. Both new sentences are pinned, plus the stranded comment line break and the pure-barrel-reads-as-zero-callables correction in the JSDoc.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Review round 7 (delta, 3a090645..0deef540): 2 findings, both in the round-6 sentence, both addressed
The dedicated-module remedy was stated as sufficient, and it is not on its own
Among indexed re-exporters, the identity goes to the first one LOADED (registration order), and module load is lazy per route. So when a shared barrel ALSO re-exports the action, whether the dedicated module's config applies is decided by which page the first visitor hits. Measured: the same app answered the same submission with the barrel's sibling validator or the dedicated module's own, depending only on whether the barrel's page was rendered first.
The message now states the condition: no other in-app module may re-export it, and why (first loaded wins).
The remedy cannot silence the warning, and the message did not say so
In the remedied state the defining module is still outside the index, so the fallback still runs and this check cannot see inside the dispatched module to know the config now travels with it. For the workspace-package case, the one the JSDoc names as what this remedy exists for, that meant a warning on every boot prescribing what the author already did. That is the diagnostic-persists-after-its-own-remedy class round 4 caught, reproduced here by both the reviewer and an independent probe.
The message now says the notice keeps printing and that the remedied state is working as intended.
Cleared
The two directional claims are accurate (the second proven against the real dispatcher by the round-6 behavioural test, which the reviewer mutation-tested: neutering config resolution flips it to 303, exactly what a per-function fix would produce, so nothing else can satisfy it). All seven re-pinned assertions discriminate. A symlinked appDir cannot reach this warning at all (every module is then unindexed and the OTHER warning fires, which lists that cause), so the remedy set is coherent for every state this one can fire in. Nothing rounds 1 to 6 fixed is broken: routing + seed suites 114/114, full Node suite green.
Both new sentences are pinned and each counterfactual reds the named test.
Verification
Node 3603 (3602 pass, 0 fail) · Bun matrix 271 pass, 0 genuine failures.
1 participant
Closes #1155
WebJs had two ways to handle a form submission and they did not compose. A page
actionexport worked without JS but had to live in the page module, so every module action needed a hand-written per-page adapter. A'use server'action called from a component composed withmodules/, but did nothing with JS off and its@submitbinding forced the whole component to ship. Two mechanisms doing one job is what makes an agent guess, so this collapses them into one.The one way:
The renderers resolve the action's identity (
<file-hash>/<export>, the naming the RPC endpoint already uses), omit theactionattribute so the form posts to the page's own url, forcemethod="post"and an enctype, and emit one hidden__webjs_actionfield carrying the identity. The server resolves that field back to the function and runs it. Identical semantics with JS on and off by construction: the client router intercepts the submit and posts the same body to the same url.The page
actionexport is removed.What the action receives, and what runs
A form-bound action always receives the
FormData, on both paths. Its declared config exports all run on the form boundary too:validate(on the FormData, its transform-return becoming the typed input), themiddlewarechain (with the page's route context), andinvalidates(only when the action actually ran). Skipping any of them would leave an action protected over RPC and open over a form, which is a privilege gap rather than a missing feature.Responses
303PRG toresult.redirect(same-site local only) or the page's own path422re-render of the same page with the result onactionData405+Allow: GET, HEAD422re-render with a resubmit message and the typed valuesmethod = 'GET'405, plus awebjs checkerror at edit time403The
405replaces the old404, which claimed a url that renders fine did not exist. The skew422is the deploy case: a404would discard everything typed and a silent no-op would show success for a write that never happened.Security
verifyOriginnow covers the form path. The page-action path it replaces had no origin check at all and was shielded only bySameSite=Laxcookies, so this closes a real gap rather than porting one.formaction=${fn}on a submit button is refused here and tracked separately at #1207. My earlier reasoning for that refusal was wrong: I claimed the identity "would have to ride the submitter'sname/valuepair", but that is only one of the two channels a browser gives you, and the other (the URL the submitter'sformactionpoints at) is free. #1207 records the working mechanism and the caveat that sank the URL variant.Test plan
validate/middlewareon the form path, CSRF, the new check rule with its docs-sample carve-out.npm run test:browser, Chromium + Firefox + WebKit, 640 / 630 / 640, 0 failed): the bound form asserted throughnew FormData(form), the same serialization a native submit performs; the router posting an attribute-less form to the page url with the identity and a submitter's name/value.WEBJS_E2E=1, 82/82): the served markup with JS OFF, then invalid submit (422 + repopulated) and valid submit (303) with JS off and on, plus a one-POST-no-reload network probe.node scripts/run-bun-tests.js: 271 pass, 26 documented node-only skips, 0 genuine failures): newtest/bun/form-action-dispatch.mjs(identity round trip, multipart upload, CSRF, skew, 405) run on both runtimes and wired into thebunCI job.webjs check, and both submit paths work end to end. That gate caught a real bug nothing else could have: the file-storage action's'use server'sat under an eight-line comment header, past the window the directive scan reads, so it never registered as an action./, the four edited docs pages, and/uiin prod mode with no broken preloads;webjs checkclean on both blog and website; redirect hosts covered by their mapping tests.All 10 CI checks pass.
Unrelated to this diff, recorded so they are not mistaken for it: the
/ui/buttonpreload probe needs untrackedwebsite/components/ui/files; a Firefox browser-suite interruption main shows at the same rate;test/bun/dev-extra-watchis timing-sensitive under parallel load (3/3 in isolation); andjspmGenerate #446went red once on live-CDN inconsistency and passed on re-run with no code change (this branch has an empty diff for every vendor file).One pre-existing bug surfaced while reviewing and confirmed NOT to be from this PR: two
'use server'modules that re-export from each other in a cycle crash at load (ReferenceError: Cannot access 'fn' before initialization), because the seed facade turnsexport functionintoexport const. Restoring the pre-PR bail-out reproduces it byte-identically. Untracked; exotic and unrelated to form actions.Docs
AGENTS.md (new invariant 12, the html-prefix table, the recipes), the agent skill (SKILL.md plus routing-and-pages, data-and-actions, optimistic-ui, muscle-memory-gotchas, auth-and-sessions, testing), the docs site (server-actions, progressive-enhancement, file-storage, troubleshooting, migrating-from-nextjs, ssr, components, security, authentication, architecture, error-handling and five smaller renames), the scaffold gallery, and one blog post.
Two docs surfaces were INVERTED rather than merely stale, since both taught that
<form action=${fn}>is a hard error: troubleshooting and the Next migration page. The source-leak reasoning is kept, because it still explains why every other spelling refuses.How the client decides (rewritten after five review rounds)
The client guard originally asked "what was just written?" and instrumented each commit branch. Five review rounds each found another branch it had missed (a removal, a quoted hole, a boolean, a property, the
encodingalias), because that question is unanswerable one branch at a time: whether a form can submit is a fact about the whole start tag, andupdateInstancere-applies only the holes whose own value changed.It now reconstructs what SSR would have emitted, from the template's recorded statics plus this pass's values resolved per part kind, and feeds that to the same predicate SSR calls (
resolveBoundFormAttrs).Reading the final DOM instead was my first design and is provably wrong, which is worth recording because it looks like the obvious answer:
<form action=${fn} ?method=${false}>postis supplied, worksmethod<form action=${fn} method=${null}>method="", refusedmethodIdentical DOM, opposite verdicts. The distinction survives only in the template.
What this deletes:
noteBoundFormAttrWriteand its four call sites,_forcedAttrs,_pendingBinds,_pendingRevalidate, and the fivetry/finallydiscard wrappers. Four of the five rounds' defect classes become unrepresentable rather than fixed. There is no module-global mutable state in the commit path any more, so the cross-render poisoning class is gone by construction, and "is this attribute ours" is recomputed each pass rather than remembered, so it cannot go stale.Two shapes are refused rather than reconciled, because no reconciliation can close them: a
.method/.enctype/.encodingproperty binding on a bound form (a.propdrops at SSR and applies for real in the browser, where all three reflect), and twoactionholes on one form. Both are the argument that already refuses.action.The parity table
packages/core/test/rendering/browser/ssr-client-parity.test.jsnow drives one template through both renderers and compares the outcomes: 12 accept rows, 13 refuse rows, attributes compared sorted since order is not something either renderer promises. It is in the browser suite deliberately, because linkedom implements no form reflection and a.proprow asserted node-side would pass for the wrong reason.This is what would have caught rounds 2 through 5 in round 1. Proven with teeth: breaking the boolean rule (the exact bug the DOM-read design would have shipped) reds one row on all three engines, and reverting the mixed-attribute reconstruction reds the
method="pos${'t'}"row.Review history
Six rounds, the first at the deep tier (16 agents, 9 lenses, adversarial jury). Recorded because the shape of it is the useful part.
Round 1 found 23 issues and none in the shared-oracle mechanism: the redesign above held, and what it found was a ring of shapes around it, plus a much sharper problem in action identity than anything in the renderer. Rounds 2 through 6 each found a defect in the previous round's fix, including two regressions I introduced and several tests of mine that could not fail.
The distribution is worth stating plainly. The shipped code path (dispatch, identity resolution, renderer parity) has been stable since round 2. The last three rounds were all about the wording of one diagnostic warning on an edge case, which is why round 6 replaced "assert on the message text" with a test that drives the real dispatcher: if config resolution ever becomes per-function, that test fails and forces the message to change with it, rather than quietly going wrong again.
Two things the loop caught that are worth carrying forward:
packages/core/distbundle, notsrc/, so a local e2e counterfactual proves nothing unless dist is rebuilt first. CI is unaffected (npm ciruns core'sprepare).