fix: refuse a function in form action=, it leaked server action source by vivek7405 · Pull Request #1156 · webjsdev/webjs

vivek7405 · 2026-07-28T11:11:34Z

<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">

Whatever the closure showed, connection strings, internal paths, query shapes, shipped to every visitor. 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. The guard is name-based and deliberately narrow: only action and formaction, where a stringified function is never useful on any tag. Every other attribute keeps its existing behaviour, and a string-valued action is byte-identical to before.

Three commit sites, not one

SSR has two independent template state machines and the client renderer has a third. The first pass guarded only the buffered renderTemplate, so a Suspense-streamed page still leaked while the buffered path already refused. Probed rather than assumed:

<form action="async function leaky(fd) { const DB='postgres://u:LEAKED@h'; return DB; }"></form>
LEAKED present: true

All three now route through one assertNotFunctionActionAttr in packages/core/src/form-action.js, and the streaming path has its own tests rather than inheriting coverage by assumption.

Scope

Only the leak. The form-binding feature that gives <form action=${action}> a working meaning is #1155 and ships separately, so this branch is independently reviewable and mergeable. Dormant machinery for it was briefly on this branch and was removed in ee7d100 (recoverable from c04d941).

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.

Test plan

Unit, 13 new tests: every hole shape (action=${fn}, action="${fn}", mixed action="/x/${fn}", formaction= on a submit button) throws on the buffered path, the streaming path, and the client renderer
The thrown message never embeds the source it is withholding
String-valued action renders byte-identically; functions in other attributes are unchanged
Counterfactual, run with the fix committed first and the guard then disabled: 7 of 10 go red and exactly the 3 passthrough tests stay green
Full Node suite diffed against an origin/main baseline in a separate worktree: failure sets byte-identical after normalizing paths, 70 pre-existing failures both sides (a jspm 401 vendor-resolve cascade, environmental), zero regressions
204 rendering tests green

Bun parity: N/A. The renderer is runtime-neutral core, and no listener, serializer, stripper or node:* surface is touched.

Dogfood four-app check: reported below before this is marked ready.

Docs

.agents/skills/webjs/references/muscle-memory-gotchas.md: new entry under "Coming from Next.js" naming the habit and what to write instead.
Docs site / AGENTS.md / scaffold: N/A here. Nothing about the documented form surface changes, since the page action export is untouched and <form action=${fn}> was never a documented pattern. Those surfaces move with Unify form submissions on <form action=${action}>, drop the page action export #1155.

vivek7405 · 2026-07-28T11:17:55Z

Phase 1 landed: the source leak is closed (#1154)

7d866963 refuses a function in action=/formaction= on both renderers. The leak was worse than I first assumed: all four hole shapes published the function body, not just the unquoted one.

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

Two things I decided while writing it.

The guard lives in its own module (packages/core/src/form-action.js) rather than being inlined at each commit site. There are three of them (SSR after-eq, SSR attr-quoted/attr-unquoted, client applyPart's attr and attr-mixed), and a guard duplicated across server and client is exactly the kind of thing that drifts when one side gets edited later. The module also already holds ACTION_FIELD, which phase 2 needs.

It is name-based and narrow on purpose. Only action and formaction throw. A function stringified into any other attribute keeps today's behaviour. That is arguably also a bug, but widening the claim here would change unrelated rendering behaviour under a security fix, and I would rather that be its own decision.

Verification: 10 new tests. Counterfactual run properly, with the fix committed first and the guard then disabled: 7 of the 10 go red, and exactly the 3 passthrough tests stay green, which is the right signature (they do not depend on the guard).

Also ran the full Node suite against an origin/main baseline in a separate worktree, since a renderer change touching every attribute commit deserves it. Failure sets are byte-identical after normalizing worktree paths: 70 pre-existing failures on both sides (a jspm 401 vendor-resolve cascade, environmental, unrelated to this branch), and the branch adds exactly 10 tests and 10 passes. Zero regressions.

Phase 2 (the dispatch + removing the page action export) is next.

vivek7405 · 2026-07-28T11:23:17Z

My first fix was incomplete: the streaming renderer leaked too

Found this while wiring the identity resolver, not from a test failing. SSR has two independent template state machines, and I had only guarded one:

renderTemplate (buffered, renderToString) is the one I fixed in 7d866963.
streamTemplate (the Suspense streaming path, reached via renderToStream) had no guard at all.

Probed it directly rather than assuming, and it emitted the whole function body:

<form action="async function leaky(fd) { const DB='postgres://u:LEAKED@h'; return DB; }"></form>
LEAKED present: true

So for the window between those two commits, a streamed page was still leaking while the buffered path already refused. Fixed in cf72805a.

The interesting part is the cause, not the miss. Two renderers implementing one contract is the setup for exactly this, and the contract was duplicated at each commit site, so guarding one did nothing for the other. The fix routes both through a single commitFormActionAttr, and the streaming path now has its own tests instead of inheriting coverage by assumption. I would treat any future rule about attribute commits the same way: if it is not in that helper, it will drift.

Worth noting for anyone reviewing: there are five >-transition sites per state machine that can close an open tag, and the identity field has to flush at each one. I covered in-tag, attr-name, after-eq, and attr-unquoted in both. That asymmetry is a smell I would rather not have, but restructuring the parsers is well outside this PR.

Identity plumbing also landed in the same commit: the resolver seam copies the CSP nonce provider shape, so @webjsdev/server installs it and the browser bundle never does, which means a client-side render keeps refusing every function rather than trusting one. Forced method="post" and enctype="multipart/form-data" are in too, both load-bearing for the no-JS path (a missing method GETs and never runs the action; a missing enctype turns a file into a filename string).

204 rendering tests green.

At SSR a `'use server'` import resolves to the REAL function (the RPC stub exists only in the browser), and `action=` is an ordinary escaped attribute hole, so the renderer stringified the function straight into the served HTML. A page doing what every Next-trained author writes, `<form action=${createTodo}>`, published the action's whole body, including any connection string or internal path its closure showed, to every visitor. Both hole shapes leaked identically (`action=${fn}` and `action="${fn}"`), as did a mixed value and `formaction=` on a submit button. Refuse it instead, on the server and on the client, so a client re-render cannot write the source into the live DOM either. The guard is name-based and deliberately narrow: only `action` and `formaction`, where a stringified function is never useful on any tag. Every other attribute keeps its existing behaviour, and a string-valued action is byte-identical to before. The thrown message names the fix without echoing the source.

The first pass guarded `renderTemplate` only. SSR has a SECOND, independent state machine, `streamTemplate`, on the Suspense streaming path, and it had no guard at all, so a streamed page still wrote a bound function's whole source into the response while the buffered path already refused it. Two renderers with one contract between them is what allowed the gap, so the decision now lives in a single `commitFormActionAttr` helper both call, rather than in duplicated logic at each site. The streaming path gets its own tests instead of being assumed to inherit the buffered path's. Also adds the identity plumbing the form binding needs: a server-installed resolver (the same shape as the CSP nonce provider, so the browser bundle never gets one and keeps refusing every function), the buffered hidden-field emission, and the forced method/enctype that a no-JS submission depends on.

The identity resolver, the hidden-field emission and the forced method/enctype belong to the form-submission unification, which is a separate change. They were unreachable here anyway (nothing installs a resolver), and shipping dormant code alongside a security fix makes the fix harder to review and to reason about on its own. What stays is the whole of the leak fix, including the streaming-path guard, so this branch is now exactly the bug fix and is independently mergeable. The removed code is recoverable from cf72805.

It has no consumer outside form-action.js, and the module is internal, so exporting it only widens what a future edit has to consider.

A review round is a reading pass, but the loop had no rule against attaching a multi-minute suite to each round, so a session could spend most of its time re-running e2e and the full Node suite between rounds while nothing they cover had changed. States the split explicitly: fast layers during the loop, slow layers once after the last clean round, and prefer CI (which already gates the merge on all of them) over a local re-run.

Review found `.action=${fn}` on a native element unguarded. `action` is a reflected IDL attribute, so assigning a function stringifies its source into the element's own attribute in a real browser. The linkedom tests could not see it (linkedom does not reflect), which is why it survived the earlier passes and why this ships with a real-browser test. A custom element's `.action` is an ordinary author-defined property that never reflects, so a function there stays legitimate and is not refused. Also corrects three things the review caught in this PR's own claims: the streaming machine is reached only through `renderToStream(v, {ssr:false})` and no page render uses it, so the earlier "a streamed page leaked" wording overstated a public-API hole as a live leak; the streaming quoted and mixed holes had no coverage, so that branch could have been deleted with the suite still green; and two client assertions were tautological, since the host is empty whenever the render throws.

…dvice

vivek7405 · 2026-07-28T13:24:54Z

Closing this without merging.

The branch feat/form-action-unification stays on the remote, so nothing here is lost and this can be reopened or cherry-picked at any point.

State at close, for whoever picks it up:

Scope is SSR stringifies a function-valued form action, leaking server source #1154 only. The form-binding feature (Unify form submissions on <form action=${action}>, drop the page action export #1155) is NOT here; the dormant scaffolding for it was removed in ee7d100 and is recoverable from c04d941.
Nine files: the guard (packages/core/src/form-action.js), its three commit sites (buffered SSR, streaming SSR, client applyPart), three test files, and three doc surfaces.
19 guard tests pass locally.

Why I would not merge it as it stands. A review round found ten findings, three of them real bugs in code I had already reviewed twice myself:

.action=${fn} on a native form was unguarded. action is a reflected IDL attribute, so assigning a function stringifies its source into the element's own attribute in a real browser. The linkedom tests could not see it, which is why it survived.
The streaming machine's quoted and mixed branch had no coverage, so it could have been deleted with the suite still green.
My own commit message and comments claimed a streamed PAGE leaked. It could not: packages/server/src/ssr.js never calls renderToStream, so that was a public-API hole, not a live leak.

All ten were fixed, but the re-review of those fixes never ran, so the fixes are backed only by the tests written alongside them. The newest and least-exercised piece is the case 'prop' guard and its native-vs-custom-element carve-out; that judgment call is exactly the kind of thing a second reader should check.

If this is resumed, the order I would take: re-review the fix set first, then decide whether the prop-path guard belongs in the same change as the attribute-path guard or in its own.

The underlying leak is real and still unfixed on main, so #1154 stays open.

The loop required a fresh subagent reviewer but said nothing about what to do when that reviewer never comes back. Every failure mode (a declined permission prompt, an internal error, a killed background task) ended the same way: the loop stalled silently, or worse, quietly degraded to an inline self-review that then counted as a completed round. That degradation is the expensive part. On PR #1156 two inline rounds reported clean and the first real subagent round that followed found ten findings, three of them genuine bugs in code those rounds had already passed. An inline pass is worse than no review because it looks like one. So: harness status is the only liveness signal (an agent's own prose never is), a failed spawn means the round did not happen, an inline pass is never a round, and a blocked reviewer stops the loop and is reported in the same turn rather than becoming an unreported wait. The reporting shape now says only a round a subagent completed counts toward the total.

Two fixes that share a cause with everything else in this PR: a rule stated twice drifts, and a loop that stops to wait is a loop nobody runs. Closes #1157. A round is a reading pass over the diff and costs seconds, but the skill never said what NOT to run around one, so sessions re-ran multi-minute suites between rounds and then watched CI on top. Measured on #1156: a single 32-minute gap between two commits, spent running the Node and e2e suites twice each, none of which changed a finding and all of which CI re-ran anyway. Now only the touched test files run per fix, the suites and the CI read happen once after the last clean round, and the text says outright that this changes WHEN they run and never WHETHER, since an earlier attempt at this wording read as permission to skip a layer. Closes #1160. Step 2 of the loop restated the review-posting mechanics and had drifted from the authoritative section on both points: it sent findings through the standalone-comment API rather than one review object, used issue comments for round summaries, and had each finding carry its own disposition. All three are what the posting rules forbid, and the first is how a review on #1115 ended up ungrouped. Step 2 now defers to those rules instead of paraphrasing them, keeping only what is genuinely its own. A pointer cannot drift; a paraphrase will.

* docs: stop a dead review subagent stalling the self-review loop The loop required a fresh subagent reviewer but said nothing about what to do when that reviewer never comes back. Every failure mode (a declined permission prompt, an internal error, a killed background task) ended the same way: the loop stalled silently, or worse, quietly degraded to an inline self-review that then counted as a completed round. That degradation is the expensive part. On PR #1156 two inline rounds reported clean and the first real subagent round that followed found ten findings, three of them genuine bugs in code those rounds had already passed. An inline pass is worse than no review because it looks like one. So: harness status is the only liveness signal (an agent's own prose never is), a failed spawn means the round did not happen, an inline pass is never a round, and a blocked reviewer stops the loop and is reported in the same turn rather than becoming an unreported wait. The reporting shape now says only a round a subagent completed counts toward the total. * docs: fix the liveness check to name a mechanism that works TaskList is the shared to-do list, not a registry of running subagents, so "empty means nothing in flight" reads empty in any session that never called TaskCreate. It reports "dead" identically whether the reviewer is dead or running, which is worse than no check. Issue #1158 specified it, but the premise does not hold. The real signal for the mandated foreground spawn is the Agent call's own result in the same turn, and for a stray background agent it is the harness completion notification or TaskOutput on its name. Also drops the claim that backgrounding is what lets a spawn be declined (a refusal lands on the Agent call either way), and cross-references the new blocked path from the two downstream absolutes that said the loop can only ever end on a clean round. * docs: name a background reviewer's real status signal, not TaskOutput TaskOutput is deprecated and its own description says a local agent's result comes from the Agent tool result, not from the output file (which is the whole subagent transcript). Pointing at it by agent name was the same defect as the TaskList one it replaced: a named mechanism that does not do the job. The harness completion notification arrives on its own and carries the real result, so that is what the file names now. Also: the failed-spawn bullet said "do not report it" next to a bullet demanding you report the blockage, which reads as the exact silent stall this is meant to kill, so it now says do not report it AS A ROUND. And the standalone-review entry point now points at both halves of step 1, since it was sending readers to the liveness rules while skipping the working-tree-safety block that has actually corrupted a checkout. * docs: close the paths that still stall or pass a non-review as a round Tracing the four failure paths through the new wording found daylight in all of them. A spawn that dies after its isolated worktree exists is the likeliest leaker of a stray worktree, and declaring a failed spawn "not a round" had quietly excused it from the repo-health check that fires after each round. The check now keys off the spawn. A killed BACKGROUND agent was routed straight to a blocked loop even though the mandated foreground reviewer had never been tried, so a perfectly reviewable PR could halt on evidence about an agent that was never the reviewer. It now writes that agent off and spawns the real one. A reviewer can return successfully having reviewed nothing ("I could not fetch the diff"), which was neither a finding list nor a literal CLEAN and hit no branch at all. That is now explicitly a failed round, because absence of findings is not a clean round. Retry is pinned: one per round, mechanical failures only, and a fresh round is not a way to buy a second retry. The durable record (draft PR plus the blockage in the body) now applies in every mode rather than only autonomously, since the interactive mode had the weaker record. Failure handling gains the blocked-reviewer case, and the spawned prompt template no longer justifies its git prohibition with a shared working directory the mandated isolation makes false; worktrees sharing one .git directory is the true reason and it survives isolation. * chore: allow the Agent tool so review spawns stop prompting A declined permission prompt is one of the four reviewer failures #1158 names, and it is the only one with a configuration fix rather than a prose one. The repo mandates the self-review loop for every agent that works it, so the permission that lets the loop run belongs in the repo's own committed settings next to the hooks that enforce the rest. Tool-level is the only granularity available, so this allows every Agent spawn rather than only reviewers. * docs: order the blocked-reviewer rules and drop the stale duplicates Read top-down, step 1 told an agent to declare the loop blocked and edit the PR body before it ever reached the bullet granting a retry, so an internal error ended in a blocked PR while still being retryable. Retry now comes first and the ordering is stated outright. The bullets are also no longer referenced by ordinal, since the previous commit added two and left the lead-in saying four, which would have stopped a literal reader one bullet short of the retry rule itself. Three duplicates had drifted from the rules they restate. The working-tree block still opened with the SHARED working directory premise that the prompt template stopped using, the standalone-review pointer still summarized the repo-health check as per-round after it moved to per-spawn, and the Failure handling entry told an autonomous agent to ask the user how to proceed, which AGENTS.md forbids outright. Each now matches its source, and the blocked path no longer demotes a ready PR that another session opened. * docs: give the reviewer a way to say it could not review The returns-without-reviewing rule assumed an output shape the prompt template forbade. The template sanctioned exactly two answers, a finding list or the literal CLEAN, so a reviewer that never fetched the diff had found nothing genuinely wrong and was steered straight into CLEAN, which the loop accepts as converged. The template now defines a BLOCKED sentinel for that case and says outright that reporting CLEAN for a review you could not perform is the worst available outcome. Two rules that admitted the same non-round are fixed with it: the reporting shape tested for a completed call and a subagent result, both of which a non-review satisfies, and the per-round posting rule told a findless round to post a clean review object even when nothing was reviewed. Also: worktree isolation was described as containing every git op, which is the claim the shared .git premise replaces, so it now says isolation covers the working tree while the read-only prohibition covers the refs and config it cannot reach. The two retry paths were unified onto one budget, a recovered failure is reported in the turn rather than through the blocked-PR procedure, the reviewer prompt no longer assumes the branch is checked out locally, and a blocked review is reported to the user instead of being written onto the PR, since a spawn that could not run is session tooling and not a fact about the change. * docs: re-spawn a broken reviewer instead of interrupting the user The blocked path stopped the loop and asked the user how to proceed after a single retry. That is the wrong trade: a spawn that broke mechanically is harness noise, recovering costs seconds, and handing back a half-finished loop costs the review its whole momentum. The user asked for a converged review, not a status update. So a mechanical failure is now re-spawned until one takes, with the prompt adjusted when the reviewer named what it was missing, and varied rather than repeated after a few identical failures. Nothing about this relaxes the rule the block exists for: an inline pass is still never a substitute for keeping the loop moving. A reviewer that genuinely cannot be produced remains the one stopping point, it withholds ready-for-review, and it is reported once at the end rather than as a mid-loop interruption. * docs: keep the loop fast and stop restating the review-posting rules Two fixes that share a cause with everything else in this PR: a rule stated twice drifts, and a loop that stops to wait is a loop nobody runs. Closes #1157. A round is a reading pass over the diff and costs seconds, but the skill never said what NOT to run around one, so sessions re-ran multi-minute suites between rounds and then watched CI on top. Measured on #1156: a single 32-minute gap between two commits, spent running the Node and e2e suites twice each, none of which changed a finding and all of which CI re-ran anyway. Now only the touched test files run per fix, the suites and the CI read happen once after the last clean round, and the text says outright that this changes WHEN they run and never WHETHER, since an earlier attempt at this wording read as permission to skip a layer. Closes #1160. Step 2 of the loop restated the review-posting mechanics and had drifted from the authoritative section on both points: it sent findings through the standalone-comment API rather than one review object, used issue comments for round summaries, and had each finding carry its own disposition. All three are what the posting rules forbid, and the first is how a review on #1115 ended up ungrouped. Step 2 now defers to those rules instead of paraphrasing them, keeping only what is genuinely its own. A pointer cannot drift; a paraphrase will. * docs: resync the three readers of the re-spawn rule Switching the blocked path from one-retry-then-ask to re-spawn-until-it- takes updated the rule and left three places still describing the old one: the failed-spawn bullet told you to report each failure in the turn it happened, the returns-without-reviewing bullet still budgeted a single shared retry, and the Failure handling entry still said to retry once and then ask the user how to proceed. That last one is the same drift this PR keeps finding, in the same file, from my own edit. * docs: shape the loop as a parallel fan-out plus delta verification Measured on this PR's own review: five sequential broad rounds cost 608k subagent tokens and 25 minutes, dominated not by local commands (5.6s of I/O per round) but by each reviewer ingesting a growing payload, and the prior-comments feed alone reached 171 KB by round 5. A parallel fan-out of four narrow lenses then covered more surface in one round's wall clock. So the loop now spawns round 1 as 3-4 narrow reviewers in parallel (correctness, internal consistency, issue satisfaction, mechanical), makes every later round delta-scoped on the fixes, and starves reviewers of context: the diff and touched files only, never prior PR comments, never an exclusion list, with duplicates deduped by the author for free. The same fan-out reviewed this change and its findings are fixed here too: the order lead-in referenced a retry distinction that no longer exists, two bullets pointed at "the bullet below" that was not below, the prompt-spec claimed the template ends with CLEAN when BLOCKED comes after it, the returns-without-reviewing bullet over-claimed the sentinel's coverage, the template never received the branch its spec promised, the reject path still wrote reasons into the PR body, the two blocked-loop summaries omitted the never-reviews case, ready-to-merge was announceable before the deferred suites and CI read, and the no-machinery-tells voice rule contradicted the body's required test plan and dogfood evidence. * docs: convert the per-round machinery to the fan-out shape Adding fan-out and delta rounds to the preamble left the downstream machinery written for one broad sequential reviewer. The delta verify on the previous commit caught all of it: the template still told reviewers to rotate focus per round, a delta round had no prompt shape and would re-read the whole surface, the starve rule forbade the AGENTS.md read the spec required two bullets later, step 3 banned the fan-out re-run the preamble allows, step 4 ended the loop on a clean round 1 that the minimum-two-rounds rule sends to a delta pass, nothing said a fan-out is clean only when every lens is, and the strengthened Contains-verbatim requirement was violated by the file's own template. A fan-out now has explicit round semantics: each lens re-spawns individually without voiding the others, the round completes when every lens returns findings or CLEAN, its findings post as one aggregated review object, and the whole fan-out counts as one round. * docs: close the last two seams in the fan-out conversion The starve rule enumerated a reviewer's permitted context exhaustively and left out the PR title and body that template step 2 has every reviewer fetch, so the rule and the template disagreed about what may be ingested. The body is the author's claims, which the review checks against, so it joins the enumeration rather than leaving the template. And the delta pass a clean fan-out earns had no instantiable scope: with no fix commits, every delta prompt shape pointed at a diff that does not exist. The spec and template now say that pass scopes to the riskiest section of the original diff, matching the minimum-two-rounds rule that created it. * docs: state the delta scope identically in all three places it appears The previous commit widened the delta scope in two of the three places that state it and missed the round-shape bullet that governs the loop, which still scoped every later round to fix commits that do not exist after a clean fan-out. Same for the starve enumeration: expanded in the rule, unexpanded in the prompt-construction bullet built from it, so an agent composing from the bullets starved the reviewer of the title and body the rule had just admitted. The template's charter placeholder also offered no instantiation for the clean-fan-out pass. All three now carry the same rule with the same carve-out. * docs: give the all-rejected round a delta scope too The no-fix-commits fallback was conditioned on "after a clean fan-out", but a round whose findings were ALL rejected also produces no fix commits and is still followed by a mandatory round, so it had no defined scope anywhere: the headline clause was empty and the carve-out did not apply. The condition is now "a round that produced no fix commits", with both causes named, in all four places the rule appears. Also pinned --repo on the title-and-body fetch so a prompt composed from the bullets works when the branch is not checked out locally, matching the template. * docs: trigger step 3's fallback on the mechanism, not two of its causes Findings can be fixed, rejected, or filed, and a round of only rejected and filed findings produces no fix commits either. Step 3's fallback fired only on "every finding rejected", so the filed case executed the main clause against an empty diff. The trigger is now the mechanism itself, the round produced no fix commits, which cannot be under-inclusive whatever dispositions compose it. * docs: state the no-fix-commits condition mechanism-only, everywhere The previous fix put "rejected or filed" in one statement while the three sibling glosses kept the older two-cause enumeration, recreating the drift it was fixing. Cause lists rot every time a disposition is added, so all four statements now name only the mechanism, a round that produced no fix commits, which stays correct whatever composes it. * docs: delta rounds narrow the question, never the evidence "Fed the fix commits' diff plus the paragraphs they touched" read as the reviewer seeing only that, which for a multi-file change would miss a fix's ripple effects somewhere else, the user's exact concern. What made the delta rounds on this PR effective was the opposite split: the fix as the question, the full changed files as evidence, and a mandate to grep every symbol or concept the fix touches across the whole PR surface. That is now what all three statements say. The costly thing a delta round drops is re-auditing the unchanged surface from scratch, the all-pairs reasoning that makes broad rounds slow, not context. * docs: convert step 3 and the intro to the question-evidence model The question-never-evidence rewrite reached three statements of the delta rule and missed the fourth, step 3 of the loop, the one an agent executes literally, which still narrowed the evidence and cited the round-shape rules while saying the opposite of them. The section intro also still promised that later rounds re-read only what changed, priming evidence-scoping two bullets above the rule that forbids it. Both now carry the same model: audit once, then narrow the question while holding the full changed files as evidence. * docs: scale review depth with risk, richest for shipped source WebJs is a framework, so a bug in packages/*/src ships into every end-user app. The round-shape rules now say so: shipped source gets the full shape at its richest (correctness and security lenses, a lens on whether the tests would fail if the change were reverted, a bias toward re-running the fan-out when a fix spans files), while a docs or skill PR may run leaner. The fast-loop rules cut waste, never rigor, and for shipped source the tie breaks toward more review, not less. * docs: single-source the lens list and the fan-out re-run trigger The risk bullet restated two rules and immediately drifted from both: its lens set could not coexist with the fan-out's definitional list, and its spans-files re-run bias contradicted the two "only" statements of the re-run trigger. The lens list is now stated as swappable defaults, the trigger lives in one place (the delta bullet, with the shipped-source lowering inline), and the other statements defer instead of restating. Also converted the last surviving scope-phrased statement of the riskiest-section pass to question form. * docs: adversarial refutation, model diversity, and a deep-review tier Three review-quality upgrades the loop lacked. A shipped-source finding whose fix would be expensive or behavior-changing now gets a refuter before anyone acts on it, because a confident false positive buys a wrong fix plus the delta rounds to unwind it. At least one fan-out lens runs on a different model, since same-family reviewers share the same blind spots. And the riskiest surfaces get a committed deep-review workflow (.claude/workflows/deep-review.js): six finder lenses in parallel, one cross-model, then an adaptive adversarial jury per finding (three refuters for critical, two for major, one for minor, majority kills), returning only what survives refutation, with rejected findings carrying their refuters' reasons. It is the local, no-billing analog of the hosted ultra review, and its confirmed findings feed the normal loop. Syntax validated under the workflow runtime's own wrapping, since a bare module check rejects the top-level return the runtime allows. * docs: actually commit deep-review, generic and safely spawned The previous commit pointed the skill at .claude/workflows/deep-review.js and shipped no such file: .gitignore's .claude/* allowlist has no workflows entry, so git add -A silently skipped it, and the delta round caught the skill sending the riskiest PRs to a workflow that cannot run. The allowlist now unignores .claude/workflows/ and the file is tracked. The workflow is also repo-generic now, per the user: no framework naming, args accept a PR number, owner/repo#N, or { pr, repo }, and when no repo is given each agent detects it from its own cwd via gh. The jury's fail-open stance is documented inline (an empty jury or a tied vote leaves the finding confirmed, never silently dropped). The refuter gate in the skill inherits step 1's spawn rules explicitly (worktree isolation, read-only git, per-spawn repo-health check, re-spawn on failure), and a refuter that cannot be produced means the finding proceeds ungated, since the gate is an optimization and must fail toward treating the finding as real. * docs: pin two distinct models and fix the workflow's rough edges Pinning one lens to fable guaranteed nothing once the session itself runs fable: the pin coincided with the default and every lens ran the same model, making the different-model claim false exactly where the workflow was written to run. Two lenses now pin two distinct models (fable and opus), so at least one always differs from whatever the orchestrating session runs. Also from the delta round: the repo-given SAFETY note told agents to pass --repo to every gh call, but gh api has no --repo flag and errors on it, so the one command needed for reading files at head failed as instructed; the note now splits gh pr from gh api and the fetch line carries the repo in its URL path. And the clean-pass return now has the same shape as the findings return, so a caller reading stats never gets undefined on one path. * docs: pin every deep-review lens, half opus and half fable The session default is opus, so inherit-by-default meant four of six lenses ran the author's own model. All six now pin explicitly: opus for correctness, blast-radius, and tests; fable for security, invariants-docs, and fresh-eyes. Two model families always read the diff, deterministically, whatever the session runs. * docs: let a scout propose dynamic lenses per PR in deep-review Six fixed lenses cover the categories every PR shares, but the angles a specific PR earns depend on what it actually does: concurrency for async coordination, wire compatibility for a serializer change, migration paths for a schema move. A new Scope phase runs one scout that reads the diff, is told the fixed six so it never duplicates them, and proposes zero to six additional lens charters, zero being a fine answer for a PR the fixed set already covers. Dynamic lenses alternate between fable and opus by index, run in the same Find fan-out, and report through the same dedup and jury pipeline. A caller can also pass explicit lenses via args, which skips the scout. Stats now name the dynamic lenses used so a run's shape is auditable from its result. * docs: deep-review is round 1 for every task, no risk bifurcation The user directed a uniform rule: every task entering the review loop starts with the deep-review workflow, whatever the work touches. The scout removed the last advantage the ad-hoc fan-out held (per-PR lens tailoring), the workflow's cost self-scales because juries convene per finding, and a uniform rule cannot be misjudged the way a by-risk bifurcation can. The fan-out machinery, the lens-swapping guidance, and the match-depth-to-risk bullet are deleted rather than kept alongside, since duplicated review rules drifting apart is this file's documented disease. The refuter gate survives scoped to delta-round findings, whose reports are the only ones that now arrive without a jury behind them. Two rules added at the user's direction: deep-review re-runs in full when the PR's scope changes after round 1 (new work folded in, a pivot, an issue absorbed mid-review), because a delta round verifies fixes to an audited surface and cannot stand in for the audit of a surface round 1 never saw. And a standalone "review the PR" request always starts with a fresh deep-review, regardless of prior cycles, because the ask itself says the existing trail is not trusted or not current. The trivial-change skip is unchanged: a one-line typo still enters no loop at all. * docs: convert the per-round machinery to a workflow-shaped round 1 The unification made round 1 a Workflow run and left the machinery written for Agent-tool spawns, the same seam the fan-out conversion hit. The delta round enumerated all of it. The liveness rules now name the Workflow run's completion notification as round 1's status signal, and the clean-round tests recognize each reviewer's expected result shape, since a deep-review run returns a structured result and never the literal CLEAN the old test demanded. The foreground mandate and prompt spec are scoped to the delta verifier. Working-tree safety states honestly that deep-review's internal agents run on the prompt defense alone, which is exactly why the repo-health check now fires after workflow runs too. Round semantics for a jury-adjudicated round are defined: confirmed findings are the round's findings, jury-rejected ones return for the audit trail only and force nothing, while a delta finding the author rejects still forces a round because the author's own rejection is unadjudicated in a way a jury's is not. A manual review ask overrides the trivial-change skip: when the user asks for a review, they get one. * docs: finish converting the three spawn-era recaps The liveness tail still wrote off every dead background agent as never the mandated reviewer, which stopped being true the moment round 1 became a background Workflow run; it now distinguishes a stray agent from the round's own reviewer and names the right recovery for each. The standalone-review recap still restated the old both-defenses-always scheme its source no longer says, and the Failure handling bullet still spoke only of re-spawning a subagent. All three now match the workflow-shaped machinery they summarize. * docs: async reviewer spawns with a transcript watchdog The foreground mandate rebuilt the exact blind wait this PR exists to kill: a synchronous spawn blocks the whole turn, so a hung reviewer is indistinguishable from a working one for as long as the harness takes to give up, and one delta round that should cost minutes ate 20 that way, unwatchable from inside the blocked call. Reviewers now spawn in the background with a watchdog on the task's transcript file, firing on two minutes of stalled mtime or ten minutes total. The peek it triggers reads harness evidence only, task status and whether the transcript is still growing; the words in a transcript remain an agent's own prose and count for nothing, so the #1158 rule survives intact. A stalled reviewer is stopped and re-spawned, a progressing one is left alone, and a hang past the watchdog joins the failure taxonomy. * docs: watchdog checks the reviewer transcript every 30 seconds * docs: one watchdog for both reviewer kinds, with a locatable path The async flip left one clause still calling the delta verifier foreground, inside the non-negotiable block, which was the one sentence that could rebuild the blind wait. The liveness split also gave the round-1 workflow run no mid-flight signal at all, a blind wait for the longest-running reviewer kind, while the failure taxonomy assumed it had one. Both reviewer kinds now share the same pair of signals: the harness notification on completion, the watchdog's output-file activity probe mid-flight, TaskStop on either task id. And the watchdog's path is now specified rather than implied: taken from the spawn's own tool result or task notification, never guessed, since a guess once aimed a watchdog at its own output file; with no path available it degrades to a hard timer on the same thresholds. * docs: use async and multi-agent features across the four heavy skills An audit of all nine skills (filed as #1161, #1162, #1163, all closed by this PR) found four whose dominant cost is independent work run serially, and five that are correctly sequential (a board query, a write-up, a post gain nothing from agents). start-work: the deferred suites and the CI read launch as one batch of background tasks after the last clean round, with every result collected before anything is reported, so the wall clock is the slowest suite rather than the sum, and the WHEN-not-WHETHER rule is explicitly untouched. Also pinned: reviewers are one-shot, never persistent teammates, because accumulated context is the blind-spot sharing the fresh-reviewer rule exists to kill. file-issue: multi-area grounding fans out as parallel read-only Explore agents, one per concern, with the cold-start test unchanged as the gate. doc-sync and scaffold-sync: surface sweeps fan out as read-only Explore agents while edits stay in the main session, since parallel writers on overlapping docs collide; scaffold verifications run as parallel background boots, preferring the portless in-process harness so runs cannot collide, with distinct --port values when a real listen is unavoidable. * docs: the watchdog's mtime probe is meaningless for isolated agents A worktree-isolated agent's output file in the tasks directory is a static stub whose mtime never moves, so probing it reports a stall for every healthy isolated reviewer, and two were killed mid-review on exactly that misread before the CLEAN verdict arrived from the second. For isolated reviewers the watchdog now degrades to the hard timer alone, and only the hard cap or a killed/errored task status justifies a kill. * docs: parallel investigation and verification in the last two skills Closes #1164: research-record investigations fan out one read-only Explore agent per option or angle, with the writeup and its judgment staying with the caller, and the where-the-record-lands rules untouched. Closes #1165: blog-write delegates the de-dup sweep to one Explore agent returning per-post summaries, and runs the dogfood verification's independent parts as one batch of background tasks, Node and Bun boots in parallel with both still mandatory, every result collected before a post is called verified, and browser-dependent probes kept in the main session. * docs: two watchdog modes, decided by what the output file is The stub caveat was one sentence patched into a paragraph whose every other sentence still described the transcript model: the mtime-fire clause fired on every healthy isolated reviewer, the stalled-gets- stopped and tail-peek sentences contradicted the nothing-softer kill rule, the 30-second cadence was decorative for the mandated shape, the liveness paragraph still promised a mid-flight probe for both kinds, and failure handling kept a stalled transcript as a kill trigger. The watchdog now has two explicit modes: transcript mode (the file grows, mtime probe, stall fires, TaskStop and re-spawn) and stub mode (the permanent healthy state of every isolated reviewer, hard timer alone, kill only on the cap or a killed/errored status), and the liveness promise is corrected from never-a-blind-wait to never-an-UNBOUNDED wait, which is the guarantee that was actually true all along. * docs: the headline promises bounded, not blind-free, like the rules under it * docs: hard-cap deep-review at 24 agents per run The run's worst case was ~41 agents (scout, twelve finders, full juries), which is a token bill nobody chose per review. A whole-run budget now caps it at 24 by default, overridable via maxAgents and clamped 8 to 60, degrading gracefully: the six fixed lenses always run, dynamic lenses trim to what remains after reserving four jury slots, juries allocate severity-first from the leftover, and a finding the budget cannot verify is returned in an unverified list with a log line, never silently dropped, per the no-silent-caps rule. Both return paths carry the unverified list and the effective cap in stats so a run's shape is auditable from its result. * docs: the args comment and error message name every accepted key * docs: one task per git worktree, always, hook-enforced Closes #1166. The worktree rule was conditional on concurrency, with a lone-agent plain-branch carve-out whose premise is unverifiable: an agent cannot know another session will not start mid-task, and this repo runs several a night. The start-work skill's steps 3 and 4 now cut a worktree from origin/main unconditionally (a dirty primary neither blocks nor gets fixed, since it is never edited at all), AGENTS.md states the rule without the carve-out, and the instagram skill roots its own worktree at the primary checkout so it cannot nest under a task worktree. Enforcement is real now rather than fictional: AGENTS.md pointed at .claude/hooks/guard-branch-context.sh, which does not exist in the repo. The new require-worktree-for-edits.sh hook blocks (exit 2) any tracked-file edit in a repo's PRIMARY checkout, judged against the FILE's directory rather than the session cwd, since the harness resets cwd to the primary while edits legitimately target worktree files by absolute path. Untracked and gitignored files stay editable, primary detection is the git-dir/git-common-dir equality invariant, and WEBJS_NO_WORKTREE_GATE=1 is the escape hatch, all covered by seven tests including the counterfactual block case. * fix: the worktree gate now guards subdirectories, not just the root The review ran the hook itself and caught it failing open for every tracked file in a SUBDIRECTORY of the primary checkout: invoked via -C on a subdir, git prints --git-dir absolute and --git-common-dir relative (../../.git), so the raw string compare misclassified the primary as a linked worktree, and the gate guarded only root-level files while the tests stayed green by testing exactly those. Both paths now resolve through --path-format=absolute, the test repo carries a nested tracked file whose block case fails against the old hook, and the escape-hatch env var is pinned off in non-bypass tests so an inherited value cannot flip the assertions. Also from the round: the instagram recipe roots every path (mkdir, cp, cd, teardown) at one captured PRIMARY variable instead of half at the primary and half at the session cwd; the start-work skill's three surviving checkout-in-place passages now describe the worktree model, where HEAD-on-main is the primary's healthy state and the task worktree is the only holder of its branch; and AGENTS.md's autonomous-mode line cuts a worktree from origin/main instead of branching in place with the retired feature/ prefix. * docs: teardown re-derives its variables, recovery anchors its checkout The instagram teardown's rooting was illusory: Step 4's user confirmation sits between setup and teardown, shell state does not survive across tool calls, and git -C "" silently degrades to cwd-relative, so the teardown was exactly the unrooted form the prior round flagged. It now re-derives PRIMARY and BR at the top of its own block, and the derivation swaps awk for sed since awk splits a path containing spaces. The start-work recovery command anchors its checkout to the task worktree: run bare from the primary it would succeed and park the PRIMARY on the feature branch, because a detached worktree no longer holds the branch and the git-refuses safety net does not apply. * docs: cap fable-pinned reviewers at three per deep-review run Dynamic lenses alternated fable and opus by index, so a full run could pin six reviewers to fable. A MAX_FABLE budget now caps the pinned fable count across the whole run: the three fixed fable lenses (security, invariants-docs, fresh-eyes) spend it, and every dynamic lens takes opus. Model diversity is unchanged, since both families still always read the diff, and stats report the effective fable count so a run's split is auditable from its result.

vivek7405 self-assigned this Jul 28, 2026

vivek7405 added 4 commits July 28, 2026 17:08

chore: open work on #1154 + #1155

9c6ea59

vivek7405 force-pushed the feat/form-action-unification branch from 02fd4a5 to ee7d100 Compare July 28, 2026 11:38

vivek7405 changed the title ~~feat!: unify form submissions on <form action=${action}>~~ fix: refuse a function in form action=, it leaked server action source Jul 28, 2026

refactor: keep isFormActionAttr internal to the guard module

1c573bd

It has no consumer outside form-action.js, and the module is internal, so exporting it only widens what a future edit has to consider.

vivek7405 marked this pull request as ready for review July 28, 2026 12:12

vivek7405 added 11 commits July 28, 2026 17:47

docs: name the five checks that actually gate a merge

685f6a2

docs: never block on CI between review rounds

79beec9

docs: fix the contradiction about which CI checks are required

f4ac00f

docs: simplify the no-suites-in-the-loop rule

f1b06c2

docs: drop the last suggestion to block on CI

cebb1ba

docs: fall back to inline review when the subagent cannot run

006fd76

docs: require the subagent reviewer, surface failures immediately

d80a83f

docs: cover the form-action throw on the docs site, fix the action= a…

e2e95c4

…dvice

chore: move the review-loop skill changes out of this PR

c74847c

vivek7405 closed this Jul 28, 2026

This was referenced Jul 28, 2026

Stop the self-review loop re-running suites and waiting on CI #1157

Closed

Never let a dead review subagent stall or downgrade the loop #1158

Closed

vivek7405 mentioned this pull request Jul 28, 2026

docs: make the self-review loop fast, parallel, and stall-proof #1159

Merged

vivek7405 mentioned this pull request Jul 28, 2026

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

Merged

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

fix: refuse a function in form action=, it leaked server action source#1156
vivek7405 wants to merge 16 commits into
mainfrom
feat/form-action-unification

vivek7405 commented Jul 28, 2026 •

edited

Loading

Uh oh!

vivek7405 commented Jul 28, 2026

Uh oh!

vivek7405 commented Jul 28, 2026

Uh oh!

vivek7405 commented Jul 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

vivek7405 commented Jul 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Three commit sites, not one

Scope

Test plan

Docs

Uh oh!

vivek7405 commented Jul 28, 2026

Uh oh!

vivek7405 commented Jul 28, 2026

Uh oh!

vivek7405 commented Jul 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

vivek7405 commented Jul 28, 2026 •

edited

Loading