fix(transforms): close four residual destructuring leak shapes (tracked by #605) - #3846
fix(transforms): close four residual destructuring leak shapes (tracked by #605)#3846kojiwakayama wants to merge 112 commits into
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
📦 Client bundle boundary
A server module in a client graph aborts hydration in the browser. New leaks fail CI; known leaks are tracked in |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3a3b1cfc05
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Exact-head review follow-up 897eb9f addresses the effectful name-helper finding with two RED-GREEN fail-closed regressions. The full transform-stage suite passes 22 tests and 408 steps; repository lint, full typecheck, formatting, and diff checks pass. The review thread is replied to and resolved. @codex review exact current head 897eb9f. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 897eb9fb06
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Reject reassigned compiler name helpers
Fresh evidence beyond the resolved descriptor-shape case is that helper bindings are accepted solely from their initializers without checking later writes. If the exact helper is subsequently reassigned, such as __name = registerClient, a later __name(dead, "dead") is still classified as compiler metadata and removed with dead, silently deleting the registerClient side effect; require both the helper and captured define-property alias to remain unassigned before granting this exemption.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
veryfront-code/src/transforms/pipeline/stages/browser-server-exports-strip.ts
Lines 1614 to 1618 in 1c11d02
Reject reassigned compiler-name helper bindings
When a recognized helper is reassigned before an otherwise-dead registration, this set still identifies calls through the binding as inert because only its original initializer is inspected. For example, setName = (target) => (bootClient(), target); const dead = setName(() => KEY, "dead") causes the reachability pass to delete the dead initializer and its observable bootClient() call. Fresh evidence beyond the resolved helper thread is that neither the helper binding nor its captured defineProperty alias is checked for assignment-like writes; reject reassigned aliases/helpers before granting the compiler-metadata exemption.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
1c11d02 to
296c4f5
Compare
There was a problem hiding this comment.
💡 Codex Review
veryfront-code/src/transforms/pipeline/stages/browser-server-exports-strip.ts
Lines 1270 to 1271 in 296c4f5
Bind catch parameters before scanning defaults
When a stripped hook contains catch ({ KEY = KEY }), the right-hand KEY resolves to the catch binding itself and throws from its TDZ, but this traversal scans the pattern before binding its names and therefore records a reference to a module-level KEY. If that outer binding is initialized by const KEY = bootClient(), it is incorrectly added to the hook closure and removed, silently deleting the module-evaluation side effect even though the hook never reads that binding. Bind all catch-pattern names in the catch scope before visiting computed keys and default expressions.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Exact-head follow-up 3d98ef9 addresses the catch-pattern TDZ review finding with a RED-GREEN regression. RED showed |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3d98ef906d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b0784cebcb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3d773e4d6e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Heads up to whoever is pushing here: this analysis may be deleted, not reviewedThis branch was created as part A of the #3825 split, per the decision recorded at veryfront/veryfront-issue-inbox#112 (comment 5324205750): the destructured-server-value leak and the DCE hardening land here, and the intrinsic-tampering analysis waits on a spike. Since it was opened, six commits have been added that are intrinsic-tampering work ( The spike has run, and the premise for this analysis is factually wrongFull result at veryfront/veryfront-issue-inbox#112 (comment 5324403418). Spike branch The comment above
Deadness was confirmed empirically, not by reading: neutering Moving the strip ahead of compile (both the array position in What the reorder does NOT remove, so this is not a free winTwo challengers attacked the spike and both disagreed with its own verdict. Pre-compile input carries TS nodes that post-compile input never had, and two walkers in this file disagree about whether they exist:
The correct fix is a TS-aware split between erased type nodes and value-emitting ones ( Why the reorder still winsThe substituted analysis is decidable. Classifying TS nodes as erased versus value-emitting is finite and has a correct answer. Proving a module has not replaced One case is outright improved: What is worth keeping regardlessThe destructuring support in this work closes a real leak that is still open on Suggested sequence
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7e945534dc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Addressed the fresh inline-class execution finding at exact head 9a7044b with RED-GREEN constructor and instance-field regressions plus an uncalled-method contrast guard. Focused transform passes 228 steps; all transform stages pass 22 suites and 420 steps; typecheck, lint:ci, formatting across 5,152 files, and diff checks pass. @codex review exact head 9a7044b. This PR remains human-gated and is not queued for merge. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9a7044b395
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ed on unstubable hooks
The browser server-exports strip left a destructured module-scope server
value (`const { apiKey } = getEnv(...)`) in the client artifact, because
the declaration collector handled only simple-identifier declarators. A
pattern declarator is now a removal candidate as a single unit: it is
dropped — with its initialiser call and the imports it was the last user
of — only when every name it binds is exclusively part of the stripped
hook's dependency closure, so a pattern the client still partly reads
survives whole.
Two fail-closed guards are added on the same server/client boundary:
- A hook the pass identifies but cannot stub (a class declaration, an
imported binding re-exported under a hook name) now stops the build
instead of shipping the module unchanged.
- After pruning, the pass verifies that no binding it decided to drop
still appears in the output it is about to emit; a violated invariant
raises ServerExportStripError rather than leaking.
Investigated moving this DCE onto esbuild's tree-shaker (issue ask) and
recorded in the header why it cannot own the job: verified against
esbuild 0.28.1 in both transform and bundle mode, a destructuring of a
call is never shaken (even @__PURE__-annotated), an impure hook-only
initialiser is indistinguishable from client init without the closure
analysis, keepNames registrations pin hook-only helpers alive, and no
mode expresses the delete-hook-owned / reduce-unrelated import policy.
Refs veryfront/veryfront-issue-inbox#112
… emitted artifact
A module-scope assignment to a server-hook binding (export let
getServerData = stub; getServerData = realLoader) defeated stubbing: the
pass reported the hook as emptied while the real loader shipped to the
browser and overwrote the stub at evaluation time. Any assignment-like
write to a hook binding now raises ServerExportStripError.
The post-strip output verification now re-parses the artifact about to
be emitted and scans it for every dropped binding, as an import or a
reference, instead of scanning the same tree the nodes were structurally
deleted from - so a regression anywhere up to and including the
generator stops the build instead of leaking.
Also pins the reviewed sibling-default probe (const { retries, delay =
retries * 2 } = getEnv(...)) as a regression test.
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex Please review exact head Please check that one proven empty-object alternative is sufficient to analyze the supported pattern and handler, while other nullish or unsupported alternatives remain in the completion union and do not make unsafe property/default assumptions. Exact-tree local verification:
Fully paginated GraphQL audit: 143 threads across 2 pages, 0 unresolved at request time. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4a4b4b05a7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Triage note — this PR has grown past its stated scope, and that is why it cannot be approvedMeasured against the merge base (
Commits are still landing today — the four most recent being This is the same failure mode this PR was created to escape. Its own description says #3825 "grew more than fourfold while under review"; this split has grown more than threefold past its stated scope while under review. That, not reviewer availability, is why What would unblock it
What is worth preserving regardless of this PR's fateThe 10-shape leak probe in the description above. It is the most valuable artifact here — especially the record that the last four shapes silently retain the server import and the secret on the full #3825 branch. Whatever lands, that belongs in the test corpus as a merge gate. HousekeepingThis description references Full plan: veryfront/veryfront-issue-inbox#605. |
|
@codex Please review exact head Please re-check nested catches, empty patterns over object/array/function/primitive values, and mixed nullish alternatives for false reachability in enclosing handlers. Exact-tree local verification:
Fully paginated GraphQL audit: 144 threads across 2 pages, 0 unresolved at request time. |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Freeze applied at No further code commits will be pushed to #3846. The current frozen head is locally green (440 focused steps; 22 transform suites / 632 steps; typecheck; full I will not merge this aggregate: its current confidence is below the requested 90% threshold because the security-critical diff is too large and has no approving review. The next implementation path is the minimal #607 destructuring fix in a separate PR. |
|
Post-freeze update: the minimal #607 replacement landed through the merge queue as #3861 at This aggregate is no longer required to close the live destructured-value leak. #3846 remains frozen at |
Review: 50/100: Request changes. Design or correctness problem needing rework.The transform work here is real and the tests are the best fail-first evidence I have seen on this repo: 205 of 440 steps go red on the merge base and all 440 pass on your head. But the headline claim is already delivered on I agree with your own recommended disposition. Keep the branch frozen and extract the residual delta. Score breakdown
What the 9136 lines areMeasured against merge base
So there is no snapshot regeneration hiding anything. Every line is real. That is the problem, not the reassurance: 9,135 of the 9,136 lines are code and tests a human has to review. Inside the source file, two functions carry most of the growth:
Together that is 2,762 of the 3,515 added source lines, and neither one is the destructuring fix. The destructuring fix is Blocking issues
Still leaking after this PRI built a 16-shape corpus (esbuild-shaped module, hook-only helper,
Three shapes still ship the secret after this PR:
Your file header states the rule that produces all three ("this pass removes bindings, never side effects"), so they are consistent with the design. But they are not in your leak-probe table in the PR body, and the table is what a reader will treat as the coverage claim. Add these three rows to it. A reader who sees "part A closes the destructured server-value leak" will not guess that Landing part A alone is safe in the narrow sense (no fail-open, no over-pruning, strictly better than the merge base). It is not safe in the reporting sense while the body implies the class is closed. Non-blocking
Interaction with #3855They conflict hard, and they cannot both land in their current shapes.
Required order if any of this lands: #3855 first, then rebase whatever survives of this branch on top of it. The reorder is the cheaper, better-scoped change (+3265 / -419 across 9 files, with a documented migration note), and it decides the input shape every rule in this PR is written against. Doing it the other way means writing the keepNames machinery twice. Verification performedWorktree at frozen head Is it inert? No. Does the test fail first? Yes, and this is the strongest part of the PR. I checked out the merge base into a separate worktree, copied in only Result on the merge base: Result on your head: Leak corpus. I wrote 16 fixtures covering plain, nested, default, rename, object rest, array, array rest, computed key, function parameter, deep mixed, hoisted var, re-export, Over-pruning corpus. 7 fixtures that must keep live client code: destructured value the page reads, side-effect init read at load, unread destructured getter call, mutual helper cycle reached from the page, class heritage from a client import, decorator import, value shared by hook and client. All 7 pass on your head with nothing lost. No over-pruning found. Suites, typecheck, lint on your head, exit codes captured with a redirect and Mergeability. Already fixed elsewhere? Yes, mostly. Generated manifest. I gunzipped both base64 blobs in Python. Base and head both decode to 990,899 identical bytes. Review threads. 144 threads across the PR, 0 unresolved. I read the review bodies through What I could not verify. I did not measure |
1 similar comment
Review: 50/100: Request changes. Design or correctness problem needing rework.The transform work here is real and the tests are the best fail-first evidence I have seen on this repo: 205 of 440 steps go red on the merge base and all 440 pass on your head. But the headline claim is already delivered on I agree with your own recommended disposition. Keep the branch frozen and extract the residual delta. Score breakdown
What the 9136 lines areMeasured against merge base
So there is no snapshot regeneration hiding anything. Every line is real. That is the problem, not the reassurance: 9,135 of the 9,136 lines are code and tests a human has to review. Inside the source file, two functions carry most of the growth:
Together that is 2,762 of the 3,515 added source lines, and neither one is the destructuring fix. The destructuring fix is Blocking issues
Still leaking after this PRI built a 16-shape corpus (esbuild-shaped module, hook-only helper,
Three shapes still ship the secret after this PR:
Your file header states the rule that produces all three ("this pass removes bindings, never side effects"), so they are consistent with the design. But they are not in your leak-probe table in the PR body, and the table is what a reader will treat as the coverage claim. Add these three rows to it. A reader who sees "part A closes the destructured server-value leak" will not guess that Landing part A alone is safe in the narrow sense (no fail-open, no over-pruning, strictly better than the merge base). It is not safe in the reporting sense while the body implies the class is closed. Non-blocking
Interaction with #3855They conflict hard, and they cannot both land in their current shapes.
Required order if any of this lands: #3855 first, then rebase whatever survives of this branch on top of it. The reorder is the cheaper, better-scoped change (+3265 / -419 across 9 files, with a documented migration note), and it decides the input shape every rule in this PR is written against. Doing it the other way means writing the keepNames machinery twice. Verification performedWorktree at frozen head Is it inert? No. Does the test fail first? Yes, and this is the strongest part of the PR. I checked out the merge base into a separate worktree, copied in only Result on the merge base: Result on your head: Leak corpus. I wrote 16 fixtures covering plain, nested, default, rename, object rest, array, array rest, computed key, function parameter, deep mixed, hoisted var, re-export, Over-pruning corpus. 7 fixtures that must keep live client code: destructured value the page reads, side-effect init read at load, unread destructured getter call, mutual helper cycle reached from the page, class heritage from a client import, decorator import, value shared by hook and client. All 7 pass on your head with nothing lost. No over-pruning found. Suites, typecheck, lint on your head, exit codes captured with a redirect and Mergeability. Already fixed elsewhere? Yes, mostly. Generated manifest. I gunzipped both base64 blobs in Python. Base and head both decode to 990,899 identical bytes. Review threads. 144 threads across the PR, 0 unresolved. I read the review bodies through What I could not verify. I did not measure |
The branch re-ran the manifest generator, which rewrote the base64 blob on templates/manifest.generated.ts:3. Both blobs gunzip to the same 990,916 bytes (sha256 f75bb1152097c258759e78085ef83cc78a8587aea56116ebb9a6501b348fc270), so the line changed only because gzip produced a different encoding of identical content. Zero behavior, but it is one of the three files this branch conflicts with main on. Restore the merge base version and let the manifest come from main.
AGENTS.md applies the public copy rules to public comments and bans em and en dash characters. This file carried 50 of them. Replace each with a comma, colon, parentheses or a sentence break. Comments only. Every changed line starts with `*` or `//`, so the transform emits the same output.
AGENTS.md asks you to name the product rather than say "the framework". This is build-error copy an author reads, so it should say which tool strips the hook. Nothing reads this string but the error message it builds.
This branch makes a hook reached through a destructuring pattern
(`const { getServerData } = loaders`) stop the browser build. On the merge base
both forms built and shipped the loader; verified by running
`stripServerOnlyExports` on each shape against both trees.
That turns a working authoring form into a build error, so the guide has to say
so. Documents the break and the remedy, next to the existing hook rules.
The merge base manifest is stale against the templates on this branch. generate:manifests:check fails with it restored, so the regeneration on this branch is load bearing, not a no-op.
kwakayama
changed the title
Review fix pass: hygiene and honesty only, zero behavior changePushed Your branch freeze note said no commits after What changed
One review finding was wrong, and I reverted itThe review called With your branch version it passes: Gates, after the last editStill open, and yours to decideNothing here moves the PR toward mergeable. Four findings from the review remain:
|
Re-review: 34/100. I rebuilt the leak corpus and reached a different conclusion, including about my own earlier review.I was asked to extract the "four residual leak shapes" into a small PR off I did not open a PR, and I am recommending against opening one, because the extraction would extract a policy reversal rather than a bugfix. That decision is not mine to make quietly. It belongs to #605. Correcting my own earlier reviewFour claims in the 50/100 review above are wrong. I am listing them first because the rest of this comment depends on them being wrong.
The measurementTrees: merge base
I have written "keeps" rather than "leak" deliberately. In every one of those rows the pass keeps the declarator, and the secret survives because the initialiser survives with it. That is the documented rule in your own file header at So the six-row delta is real, but it is not six leaks closed. It is six declarators that What deleting them costs
The merge base passes. That matters: this is not a missing backport of #3861. The behavior already held before #3861, which only wrote it down. This branch changed it. Concretely, on this head: import { startAnalytics } from "../lib/client/analytics.ts";
const { token, client } = startAnalytics();
export async function getServerData() { return { props: { token } }; }
export default function Page() { return null; }The browser output loses This is not inert. This is a design disagreement, not a merge conflictBoth rules are defensible, and they are mutually exclusive:
Your file documents the second rule explicitly at That is what makes the Why I did not open the extraction PRI built the branch and prototyped it. Relaxing the closure check from It also turns That is the whole problem in miniature. A small, clean, well tested PR closing those shapes is achievable and would look excellent in review, and it would still be a silent reversal of The one shape that looks genuinely separable is the hoisted Score: 34/100
This cannot reach 90 as an aggregate, and neither can an extraction, until the rule is chosen. What would unblock thisOne decision, in #605: when a destructuring declarator binds a name the hook closure owns and a name nothing reads, does the initialiser survive? Answer it, and the rest follows mechanically:
Either way #3855 still lands first, for the reasons in the earlier review. Nothing here changes that. I have not merged, closed or pushed anything. |
Re-review: 34/100. I rebuilt the leak corpus and reached a different conclusion, including about my own earlier review.I was asked to extract the "four residual leak shapes" into a small PR off I did not open a PR, and I am recommending against opening one, because the extraction would extract a policy reversal rather than a bugfix. That decision is not mine to make quietly. It belongs to #605. Correcting my own earlier reviewFour claims in the 50/100 review above are wrong. I am listing them first because the rest of this comment depends on them being wrong.
The measurementTrees: merge base
I have written "keeps" rather than "leak" deliberately. In every one of those rows the pass keeps the declarator, and the secret survives because the initialiser survives with it. That is the documented rule in your own file header at So the six-row delta is real, but it is not six leaks closed. It is six declarators that What deleting them costs
The merge base passes. That matters: this is not a missing backport of #3861. The behavior already held before #3861, which only wrote it down. This branch changed it. Concretely, on this head: import { startAnalytics } from "../lib/client/analytics.ts";
const { token, client } = startAnalytics();
export async function getServerData() { return { props: { token } }; }
export default function Page() { return null; }The browser output loses This is not inert. This is a design disagreement, not a merge conflictBoth rules are defensible, and they are mutually exclusive:
Your file documents the second rule explicitly at That is what makes the Why I did not open the extraction PRI built the branch and prototyped it. Relaxing the closure check from It also turns That is the whole problem in miniature. A small, clean, well tested PR closing those shapes is achievable and would look excellent in review, and it would still be a silent reversal of The one shape that looks genuinely separable is the hoisted Score: 34/100
This cannot reach 90 as an aggregate, and neither can an extraction, until the rule is chosen. Lead verification, independent of the aboveI re-ran the three fixtures myself on fresh detached worktrees before posting this, because the conclusion reverses our own earlier review. They are not invented fixtures. They live in All three are present on What would unblock thisOne decision, in #605: when a destructuring declarator binds a name the hook closure owns and a name nothing reads, does the initialiser survive? Answer it, and the rest follows mechanically:
Either way #3855 still lands first, for the reasons in the earlier review. Nothing here changes that. I have not merged, closed or pushed anything. |
|
Closing this aggregate as superseded, not merging it. The live destructuring work that was safe to land was delivered by #3861; the still-open residual shapes remain tracked in veryfront/veryfront-issue-inbox#607. The durable artifact from this branch, its 10-case keepNames leak corpus, is now extracted with the reproduced duplicate-helper fail-closed fix in #3955. That replacement is based on current main, is independently reviewed, and passed the full local pre-push gate. The deferred-execution classifier is intentionally not extracted. It never reached main and does not justify carrying this 100-plus-commit, conflicting rewrite forward. |
The oversized transform branch carried a useful keepNames-shaped leak probe even though the branch itself is not a safe merge path. The corpus now runs at the real strip seam and accepts only two outcomes: the server chain is removed, or the module is rejected before it can reach the browser. One row exposed a live silent-retention case when the compiler name helper is declared twice. That path now fails closed only when the duplicate helper registration is the only browser read of a hook-owned target. Constraint: Preserve the #3846 corpus without importing its deferred-execution classifier or broad rewrite Rejected: Merge the #3846 classifier | too broad for the requested corpus and already superseded for the live destructuring leak Rejected: Treat duplicate helper declarations as compiler metadata | would silently assume a helper proof the module invalidated Confidence: high Scope-risk: narrow Directive: Keep this as a fail-closed corpus; silent retention of server imports or ORDERS_SECRET must remain a test failure Tested: deno task test:file src/transforms/pipeline/stages/browser-server-exports-leak-corpus.test.ts Tested: deno task test:file src/transforms/pipeline/stages/browser-server-exports-strip.test.ts Tested: deno task test:file src/transforms/pipeline/stages Tested: deno fmt --check src/transforms/pipeline/stages/browser-server-exports-leak-corpus.test.ts src/transforms/pipeline/stages/browser-server-exports-strip.ts Tested: deno lint src/transforms/pipeline/stages/browser-server-exports-leak-corpus.test.ts src/transforms/pipeline/stages/browser-server-exports-strip.ts Tested: deno check src/transforms/pipeline/stages/browser-server-exports-leak-corpus.test.ts src/transforms/pipeline/stages/browser-server-exports-strip.ts Not-tested: Full repository test suite Related: #3846
The oversized transform branch carried a useful keepNames-shaped leak probe even though the branch itself is not a safe merge path. The corpus now runs at the real strip seam and accepts only two outcomes: the server chain is removed, or the module is rejected before it can reach the browser. The corpus exposed silent retention when a compiler name helper is declared twice. Review then exposed the symmetric deletion risk when the helper has one declaration but its defineProperty alias has several. Both paths now fail closed only when the ambiguous registration is the sole browser read of a hook-owned target. Browser-read registrations stay intact. Constraint: Preserve the #3846 corpus without importing its deferred-execution classifier or broad rewrite Rejected: Merge the #3846 classifier | too broad for the requested corpus and already superseded for the live destructuring leak Rejected: Treat ambiguous helper or defineProperty bindings as compiler metadata | could silently retain a server chain or delete a real browser registration Confidence: high Scope-risk: narrow Directive: Keep this as a fail-closed corpus; silent retention of server imports or ORDERS_SECRET must remain a test failure Tested: deno task test:file src/transforms/pipeline/stages/browser-server-exports-leak-corpus.test.ts Tested: deno task test:file src/transforms/pipeline/stages/browser-server-exports-strip.test.ts Tested: deno task test:file src/transforms/pipeline/stages Tested: deno fmt --check, deno lint, deno check, and git diff --check for the changed files Tested: Repository pre-push format, lint, typecheck, and unit gates Related: #3846
The oversized transform branch carried a useful keepNames-shaped leak probe even though the branch itself is not a safe merge path. The corpus now runs at the real strip seam and accepts only two outcomes: the server chain is removed, or the module is rejected before it can reach the browser. The corpus exposed silent retention when a compiler name helper is declared twice. Review then exposed the symmetric deletion risk when the helper has one declaration but its defineProperty alias has several. A later review found the same ambiguity when the registered target only enters the hook closure during pruning. The validation now runs inside the pruning loop before metadata registrations are removed, so directly and indirectly hook-owned ambiguous targets fail closed while browser-read registrations stay intact. Constraint: Preserve the #3846 corpus without importing its deferred-execution classifier or broad rewrite Rejected: Merge the #3846 classifier | too broad for the requested corpus and already superseded for the live destructuring leak Rejected: Treat ambiguous helper or defineProperty bindings as compiler metadata | could silently retain a server chain or delete a real browser registration Confidence: high Scope-risk: narrow Directive: Keep this as a fail-closed corpus; silent retention of server imports or ORDERS_SECRET must remain a test failure Tested: RED deno task test:file src/transforms/pipeline/stages/browser-server-exports-leak-corpus.test.ts failed on the indirect closure repro before the loop validation move Tested: deno task test:file src/transforms/pipeline/stages/browser-server-exports-leak-corpus.test.ts Tested: deno task test:file src/transforms/pipeline/stages/browser-server-exports-strip.test.ts Tested: deno fmt --check, deno lint, deno check, and git diff --check for the changed files Tested: git merge-tree composition with e0918d9 Not-tested: Full repository test suite after this amendment Related: #3846 Related: #3955
The oversized transform branch carried a useful keepNames-shaped leak probe even though the branch itself is not a safe merge path. The corpus now runs at the real strip seam and accepts only two outcomes: the server chain is removed, or the module is rejected before it can reach the browser. The corpus exposed silent retention when a compiler name helper is declared twice. Review then exposed the symmetric deletion risk when the helper has one declaration but its defineProperty alias has several. A later review found the same ambiguity when the registered target only enters the hook closure during pruning. The validation now runs inside the pruning loop before metadata registrations are removed, and it carries the caller module path into the fail-closed error. Directly and indirectly hook-owned ambiguous targets fail closed while browser-read registrations stay intact. Constraint: Preserve the #3846 corpus without importing its deferred-execution classifier or broad rewrite Rejected: Merge the #3846 classifier | too broad for the requested corpus and already superseded for the live destructuring leak Rejected: Treat ambiguous helper or defineProperty bindings as compiler metadata | could silently retain a server chain or delete a real browser registration Confidence: high Scope-risk: narrow Directive: Keep this as a fail-closed corpus; silent retention of server imports or ORDERS_SECRET must remain a test failure Tested: RED deno task test:file src/transforms/pipeline/stages/browser-server-exports-leak-corpus.test.ts failed before filePath was threaded into ambiguity errors Tested: deno task test:file src/transforms/pipeline/stages/browser-server-exports-leak-corpus.test.ts Tested: deno task test:file src/transforms/pipeline/stages/browser-server-exports-strip.test.ts Tested: deno task fmt Tested: deno task lint Tested: deno task typecheck Tested: git diff --check Tested: git merge-tree composition with e0918d9 remains an existing concurrent-branch overlap in browser-server-exports-strip.ts Not-tested: Full repository test suite after this amendment Related: #3846 Related: #3955
The oversized transform branch carried a useful keepNames-shaped leak probe even though the branch itself is not a safe merge path. The corpus now runs at the real strip seam and accepts only two outcomes: the server chain is removed, or the module is rejected before it can reach the browser. The corpus exposed silent retention when a compiler name helper is declared twice. Review then exposed the symmetric deletion risk when the helper has one declaration but its defineProperty alias has several. Later reviews found ambiguity when the registered target only enters the hook closure during pruning and when a valid helper registration masks the duplicated helper registration for the same hook-only target. Ambiguity liveness now uses the same compiler name-registration target exclusions as pruning, still includes ambiguous alias candidates, and carries the caller module path into the fail-closed error. Directly and indirectly hook-owned ambiguous targets fail closed while browser-read registrations stay intact. Constraint: Preserve the #3846 corpus without importing its deferred-execution classifier or broad rewrite Rejected: Merge the #3846 classifier | too broad for the requested corpus and already superseded for the live destructuring leak Rejected: Treat ambiguous helper or defineProperty bindings as compiler metadata | could silently retain a server chain or delete a real browser registration Confidence: high Scope-risk: narrow Directive: Keep this as a fail-closed corpus; silent retention of server imports or ORDERS_SECRET must remain a test failure Tested: RED deno task test:file src/transforms/pipeline/stages/browser-server-exports-leak-corpus.test.ts failed before filePath was threaded into ambiguity errors Tested: RED deno task test:file src/transforms/pipeline/stages/browser-server-exports-leak-corpus.test.ts failed on the mixed duplicated-helper plus valid-helper repro before name-registration targets were excluded from ambiguity liveness Tested: deno task test:file src/transforms/pipeline/stages/browser-server-exports-leak-corpus.test.ts Tested: deno task test:file src/transforms/pipeline/stages/browser-server-exports-strip.test.ts Tested: deno task fmt Tested: deno task lint Tested: deno task typecheck Tested: git diff --check Not-tested: Full repository test suite after this amendment Related: #3846 Related: #3955
2 participants
Important
Branch freeze: code frozen at
6e5ab93ffe3792cb90cc1d560d1b4309d98b362f.Commits after that SHA, up to
9ebb577b0d, are documentation and commentchanges only, from the review pass below. They carry zero behavior change:
the transform stage suite reports the same 26 passed / 708 steps before and
after. The transform logic is still frozen at
6e5ab93ff.This PR is not approved for merge. It requires a human architectural review because it is a large, security-critical transform rewrite. Green CI and resolved automated review threads are necessary but not sufficient.
Current status
This work now follows:
mainThe earlier reference to issue #112 is historical. That issue is closed and replaced by #605 and #607.
Measured against merge base
c4e93cd8con 2026-08-19:6e5ab93ffThe previous description's claims of 31 commits and +3,511 / -261 are obsolete. This branch grew materially while under review, so no earlier review can be treated as approval of the frozen aggregate.
Recommended disposition
moduleScopeDeclarationsinto a small PR with RED-first coverage for safe destructuring patterns.Frozen-head validation
Local validation on
6e5ab93ff:deno task typecheck: passdeno task lint:ci: passGitHub CI and the requested exact-head review must still finish on this frozen SHA before any further readiness assessment.
Leak probe corpus
An esbuild
keepNames-shaped module with a hook-only helper, a server import, and a secret, varying one line. The merge gate is whetherimport { db } from "../lib/server/db.ts"orgetEnv("ORDERS_SECRET")remains in the browser output.typeof v === "object"v?.constructor === Objecte.constructor.namev.__proto__v instanceof Functiontypeof eval__namereassigned__namedeclared twiceconst Obj2 = globalThis.ObjectglobalThis.Object = ObjectThe final four shapes silently retain the server import and secret on the closed #3825 superset. Preserve this corpus regardless of this PR's disposition.
Residual destructuring leaks
These shapes still ship
getEnv("ORDERS_SECRET")into the browser output afterthis PR. They follow from the rule stated in the stage file header, that this
pass removes bindings and never removes side effects, so they are known and by
design rather than defects. They are recorded here because the corpus above is
what a reader treats as the coverage claim.
for (const { apiKey } of getEnv("ORDERS_SECRET"))({ apiKey } = getEnv("ORDERS_SECRET"))catch ({ message: apiKey2 })reached from a top leveltrythat throws the secretClosing these requires removing side effects, not bindings, which is a
different pass. Track it under #605.
Scope relative to
mainmainalready strips most destructuring shapes. PR #3861 (a4ec5b2800, merged2026-08-19, +161 / -22) closed veryfront-issue-inbox#607 and handles plain,
nested, renamed, rest, array, array rest, parameter and deep mixed patterns.
Measured against
origin/mainat3a109046d0, this branch's remaining delta isfour shapes:
varin a patternThe title and the #607 framing predate #3861. Read this PR as those four shapes
plus the deferred execution work, not as the destructured leak fix.
Safety invariants