fix(transforms): prune destructured server values from the client artifact

[kojiwakayama]

Description

A hook-only destructured server value shipped to the browser. moduleScopeDeclarations collected binding names only from plain-identifier declarators; any VariableDeclaration containing a destructuring pattern hit the bail-out branch and was discarded from the collector entirely, so it could never be pruned:

import { getEnv } from "veryfront";
const { a } = getEnv("X");                                  // ← survived, with its import
export async function getServerData() { return { props: { a } }; }
export default function Page() { return null; }

The simple-identifier form (const a = getEnv("X")) was already pruned correctly, so the leak was purely a function of how the value was bound. It was pinned in the suite as a documented limitation; this replaces that pin with the behaviour.

The fix

patternBindings() walks object/array patterns and returns identifiers in binding positions. Value positions are deliberately excluded, because they are reads:

shape	binding	read
{ a }, { k: a }	a	—
{ [expr]: a }	a	expr
{ a = def }	a	def
{ ...rest }, [...rest]	rest	—
[a, , b]	a, b	— (hole binds nothing)
{ a: { b } }	b	—

Adding a value-position identifier to the excluded set in dropUnusedModuleScopeBindings would hide a live client read and let the pass delete code the browser still needs. Over-pruning is a worse failure than the leak this closes, so an unmodelled shape returns null and the caller keeps the whole statement — preserving the existing fail-safe.

The consumer needed no change. ModuleScopeDecl already carried plural names / bindingIds, and the liveness check already used names.some(...) / names.every(...). Only the collector never populated them.

Related Issue(s)

Refs veryfront/veryfront-issue-inbox#607

Scope note: this is deliberately only the destructured-value leak. It does not touch the deferred-execution classifier, the intrinsic-tampering analysis, or the strip/compile ordering — those live in #3846 and #3855 and are tracked on veryfront/veryfront-issue-inbox#605.

Type of Change

• Bug fix (non-breaking change that fixes an issue)

Checklist

• I have added tests that prove my fix is effective
• deno fmt --check, deno lint, deno check clean
• Documentation changes (n/a — no public API change)

Why patternBindings is exported

Because the binding/value split is not observable through this stage end to end today, and I would rather say so than imply coverage that does not exist.

I mutated the fix to the naive version — collecting default and computed-key identifiers as bindings — and it produced byte-identical stage output on every module shape I probed (8 targeted differential shapes, plus a 12-case end-to-end probe). The end-to-end over-prune guards in this PR passed against the naive variant too. As written, they do not discriminate it.

So the invariant is pinned at the seam where it lives. Both naive variants now fail:

variant	patternBindings tests
AssignmentPattern.right treated as a binding	FAILED (4 steps)
computed ObjectProperty.key treated as a binding	FAILED (2 steps)
this PR	ok — 140 steps

The end-to-end guards are still worth keeping — they pin real behaviour — but the seam tests are what actually protect the property if hookClosure construction changes later.

Verification

deno test src/transforms/               160 passed (2629 steps)  0 failed
deno test src/transforms/pipeline/stages/  22 passed  (332 steps)  0 failed
deno fmt --check src/transforms/pipeline/stages/   clean
deno lint          src/transforms/pipeline/stages/  clean
deno check         src/transforms/index.ts          clean

Independent probe against the real stage (not the test helper), asserting the secret and the server import are absent from the emitted artifact — 12/12, including the three over-prune guards. Notably { a, b } with b live on the client correctly keeps the server value: under-pruning, never over-pruning.

Size

+258 / -12 across two files.

[coderabbitai]

Warning

Review limit reached

@kojiwakayama, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 22 minutes

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits within each organization.

For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 2b43e417-b9f0-45c4-a4e0-0aa6b5059196

📥 Commits

Reviewing files that changed from the base of the PR and between c4e93cd and dbf3db4.

📒 Files selected for processing (2)

• src/transforms/pipeline/stages/browser-server-exports-strip.test.ts
• src/transforms/pipeline/stages/browser-server-exports-strip.ts

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

📦 Client bundle boundary

Entrypoint	Modules	Source size	Server leaks
src/index.client.ts	326	1944 KiB	✅ 0

A server module in a client graph aborts hydration in the browser. New leaks fail CI; known leaks are tracked in scripts/lint/client-bundle-baseline.json to burn down.

[kojiwakayama]

@coderabbitai review

Context for the review, since the earlier run hit the rate limit before it started.

What this changes. moduleScopeDeclarations discarded any VariableDeclaration containing a destructuring pattern, so a hook-only const { a } = getEnv("X") was never pruned and shipped to the browser with its import. patternBindings() now walks object/array patterns and returns identifiers in binding positions only.

The property most worth attacking: value positions inside a pattern must stay visible to referencedIdentifiers — AssignmentPattern.right (a default) and a computed ObjectProperty.key are reads, not bindings. If either is returned as a binding it joins the excluded set in dropUnusedModuleScopeBindings, hides a live client read, and the pass deletes code the browser still needs. Over-pruning is a worse failure than the leak being closed here, so please probe that direction hardest.

Known gap I would like a second opinion on. The binding/value split is not observable through this stage end to end today. I mutated the fix to the naive variant (collecting defaults and computed keys as bindings) and it produced byte-identical stage output on every shape I probed — 8 targeted differential shapes plus a 12-case end-to-end probe — with the end-to-end over-prune guards still passing. That is why patternBindings is exported and pinned directly; both naive variants fail those seam tests. If you can construct a module shape where the naive variant and this one differ end to end, that is the test this PR is missing.

Shapes already probed (all strip correctly, hook consuming the bound name): nested pattern as a default ({ a: { b } = {} }), mixed declarators, TS type annotation on the pattern, deep obj/array/obj nesting, rest inside nested object, default whose value is a call, let rather than const, renamed + default combined. Holes bind nothing; unmodelled shapes return null and the caller keeps the whole statement, preserving the existing fail-safe.

Areas I consider most likely to hide a defect: interaction with the scope-aware import-liveness walkers and the shadowing cases, and whether adding module-scope pattern bindings to excluded can drop an import that was previously kept alive only by a binding occurrence.

[coderabbitai]

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- changed files ---'
git diff --name-status HEAD^ HEAD

printf '%s\n' '--- diff ---'
git diff --find-renames --unified=80 HEAD^ HEAD -- \
  src/transforms/pipeline/stages/browser-server-exports-strip.ts \
  src/transforms/pipeline/stages/browser-server-exports-strip_test.ts

printf '%s\n' '--- source outlines ---'
ast-grep outline src/transforms/pipeline/stages/browser-server-exports-strip.ts --items all --view expanded || true

printf '%s\n' '--- candidate test files ---'
fd -t f 'browser-server-exports-strip.*test|browser-server-exports-strip_test' src || true

Length of output: 15728

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- pattern and reference helpers ---'
sed -n '210,625p' src/transforms/pipeline/stages/browser-server-exports-strip.ts

printf '%s\n' '--- lexical reference walker ---'
sed -n '625,1090p' src/transforms/pipeline/stages/browser-server-exports-strip.ts

printf '%s\n' '--- module and import pruning ---'
sed -n '1180,1380p' src/transforms/pipeline/stages/browser-server-exports-strip.ts

printf '%s\n' '--- complete test diff ---'
git diff --find-renames --unified=60 HEAD^ HEAD -- \
  src/transforms/pipeline/stages/browser-server-exports-strip.test.ts

printf '%s\n' '--- test structure ---'
ast-grep outline src/transforms/pipeline/stages/browser-server-exports-strip.test.ts \
  --items all --view expanded || true

printf '%s\n' '--- direct patternBindings tests ---'
rg -n -C 12 'patternBindings|computed|AssignmentPattern|destructur|default' \
  src/transforms/pipeline/stages/browser-server-exports-strip.test.ts

Length of output: 138300

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- strip pipeline order ---'
nl -ba src/transforms/pipeline/stages/browser-server-exports-strip.ts | sed -n '1225,1465p'

printf '%s\n' '--- relevant tests with line numbers ---'
nl -ba src/transforms/pipeline/stages/browser-server-exports-strip.test.ts | sed -n '810,940p'

printf '%s\n' '--- references to pruning calls ---'
rg -n -C 8 'dropUnusedModuleScopeBindings\(|dropUnusedImportBindings\(|hookReferencedIdentifiers\(' \
  src/transforms/pipeline/stages/browser-server-exports-strip.ts

Length of output: 915

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- strip pipeline order ---'
sed -n '1225,1465p' src/transforms/pipeline/stages/browser-server-exports-strip.ts

printf '%s\n' '--- relevant tests ---'
sed -n '810,940p' src/transforms/pipeline/stages/browser-server-exports-strip.test.ts

printf '%s\n' '--- references to pruning calls ---'
rg -n -C 8 'dropUnusedModuleScopeBindings\(|dropUnusedImportBindings\(|hookReferencedIdentifiers\(' \
  src/transforms/pipeline/stages/browser-server-exports-strip.ts

Length of output: 20671

---

@kojiwakayama I found one test gap. I found no defect in patternBindings().

Test gap — Line 889 and Line 904: Both over-prune guards read fallback or keyName from Page. That independent client read keeps the import and binding live for both the correct collector and the naive collector. Therefore, these tests do not distinguish the mutation that incorrectly adds AssignmentPattern.right or a computed ObjectProperty.key to bindingIds.

Use this module shape instead:

import { getEnv } from "veryfront";
import { fallback } from "../lib/client.js";
const { a = fallback } = getEnv("X");
export async function getServerData() { return { props: { a } }; }
export default function Page() { return null; }

The correct collector keeps the destructuring declaration because its default expression reads fallback. It therefore keeps both imports. A naive collector hides the fallback occurrence, removes the destructuring declaration, adds fallback to hookClosure, and then removes the fallback import. The computed-key case has the same differential behavior:

const { [keyName]: a } = getEnv("X");

Replace the Page reads in both guards with return null, then assert that the destructuring declaration or getEnv import remains, and that the client import remains. This directly proves the end-to-end safety property described in the PR.

---

🧠 Learnings used

Learnt from: kojiwakayama
Repo: veryfront/veryfront-code PR: 0
File: :0-0
Timestamp: 2026-08-19T00:29:10.484Z
Learning: In `src/transforms/pipeline/stages/browser-server-exports-strip.ts`, removal of an unrelated unused named import that the stripped hooks do not own depends on TypeScript unused-import elision in the `ts` and `tsx` esbuild loaders, not on `treeShaking`. The elision occurs in both development and production. JavaScript-family loaders do not elide these imports, so the pre-existing JavaScript/JSX behavior must be evaluated separately.

⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[kojiwakayama]

@coderabbitai Thanks — and agreed on the finding: the two over-prune guards do not discriminate the mutation. That is exactly the gap, and I had reached the same conclusion independently before opening this PR.

But the proposed replacement does not discriminate it either. I ran your exact shapes against both collectors before acting on the suggestion, and the outputs are byte-identical.

Your suggested module, verbatim:

import { getEnv } from "veryfront";
import { fallback } from "../lib/client.js";
const { a = fallback } = getEnv("X");
export async function getServerData() { return { props: { a } }; }
export default function Page() { return null; }

Correct collector, actual stage output:

export async function getServerData() {
  throw new Error("server-only");
}
export default function Page() {
  return null;
}

Both imports are gone, and the destructuring declaration with them — not kept, as the prediction expected. Same output from the naive collector, and the same for the { [keyName]: a } variant. A diff over both collectors on both shapes reports no difference.

The prediction was that "the correct collector keeps the destructuring declaration because its default expression reads fallback". It does not, and the reason is the ordering inside dropUnusedModuleScopeBindings:

1. The hook body is emptied before pruning runs, so a has no remaining reference.
2. inClosure is true (a is in hookClosure) and unused is true (a appears only in its own binding position, which is in excluded) — so the declaration is removed under either collector.
3. Removing it deletes the default expression too, so the fallback occurrence disappears regardless of whether it was ever counted. The fixpoint loop then drops the now-dead import.

The default's read only matters while the declaration survives, and the declaration cannot survive in a shape where the hook is its only consumer — which is precisely the shape needed for the leak. That is the asymmetry that makes this property undecidable end to end here, and it is why I moved the pin to the seam rather than writing a better module fixture.

I have pushed dbf3db42e, which does not change the tests you flagged but corrects their comment so they are not mistaken for a guard:

These pin the resulting BEHAVIOUR. They are deliberately NOT the guard for that property, and should not be read as one: a collector that wrongly treats defaults and computed keys as bindings passes all three. Two independent attempts to build a discriminating end-to-end case failed …

Your review is now one of those two attempts, recorded in the comment. That is more useful than a fixture that looks like a guard and is not.

The discriminating tests remain the patternBindings block: mutating AssignmentPattern.right into a binding fails 4 steps, and mutating a computed ObjectProperty.key fails 2. Both pass on the code as written.

If you can construct a shape where the declaration survives (a sibling binding the client reads) and the default's read is load-bearing for a third binding's liveness, that would discriminate and I will add it. I probed that direction — including const { a = fb, b } = getEnv("X") with b live on the client — and found no difference, because the surviving declaration keeps the default's read counted under both collectors.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- requested commit ---'
git show --stat --oneline --decorate --no-renames dbf3db42e

printf '%s\n' '--- requested commit test diff ---'
git show --format=fuller --find-renames --unified=30 dbf3db42e -- \
  src/transforms/pipeline/stages/browser-server-exports-strip.test.ts

printf '%s\n' '--- pruning helpers outline ---'
ast-grep outline src/transforms/pipeline/stages/browser-server-exports-strip.ts \
  --items all --view expanded | sed -n '1,240p'

printf '%s\n' '--- pruning and hook-removal order ---'
rg -n -C 12 \
  'function dropUnusedModuleScopeBindings|dropUnusedModuleScopeBindings\(|hookClosure|remove.*hook|replace.*hook|remove.*server' \
  src/transforms/pipeline/stages/browser-server-exports-strip.ts

Length of output: 25355

---

@kojiwakayama Thanks for the correction and the detailed trace.

I verified dbf3db42e. My proposed replacement fixture was incorrect.

stripServerOnlyExports empties the hook before it calls dropUnusedModuleScopeBindings. After that step, a has no surviving reference. The pass removes the declarator under both collectors. That removal also removes fallback or keyName before import liveness runs.

The updated comment correctly states that the end-to-end cases do not discriminate the binding/value classification. The direct patternBindings tests are the correct discriminating guard for this invariant.

I withdraw the replacement-fixture recommendation. The original finding about the misleading guard description is addressed by the verified comment update.

🐇

---

✏️ Learnings added

Learnt from: kojiwakayama
URL: https://github.com/veryfront/veryfront-code/pull/3862

Timestamp: 2026-08-19T07:29:35.598Z
Learning: In `src/transforms/pipeline/stages/browser-server-exports-strip.ts`, `stripServerOnlyExports` captures `hookClosure`, empties server-only hook bodies, then calls `dropUnusedModuleScopeBindings`. For a hook-only destructured binding such as `const { a = fallback } = getEnv("X")`, the declaration is removed after the hook is emptied because `a` has no surviving reference. This removes the default expression before import liveness runs. Therefore, an end-to-end fixture without an independent client read of `fallback` cannot distinguish whether `patternBindings` incorrectly classified `AssignmentPattern.right` or a computed `ObjectProperty.key` as a binding. The direct `patternBindings` tests are the discriminating guard for this binding/value classification.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

---

If you found this review helpful, would you consider giving us a shout-out on X?

Thank you for using CodeRabbit!

_{You are interacting with an AI system.}

[kojiwakayama]

Superseded by #3861 — closing

#3861 merged as a4ec5b280 while this was in review and fixes the same leak in moduleScopeDeclarations. Duplicate work, my side: I opened this before checking whether anyone else was on veryfront/veryfront-issue-inbox#607.

Its implementation is the one to keep. It reuses the existing patternBindingIds traversal rather than adding a second walker, and it is more conservative than mine in a way I had not justified — it fails closed on patterns with defaults or computed keys, on the grounds that evaluating those positions can run unrelated client code.

I measured a4ec5b280 before closing this. Three shapes strip; four still leak:

shape	main @ a4ec5b280
const { a } = getEnv(S)	strips ✅
const { o: { i } } = getEnv(S)	strips ✅
const [x] = getEnv(S)	strips ✅
const { a, ...rest } = getEnv(S)	leaks
const { a = 1 } = getEnv(S)	leaks (deliberate)
const { ["k"]: a } = getEnv(S)	leaks (deliberate)
const { k: a = 1 } = getEnv(S)	leaks (deliberate)

That residual is recorded on veryfront/veryfront-issue-inbox#607, which I have reopened — it had been closed as completed.

I also tried to land the { a, ...rest } case here as a follow-up and withdrew it: relaxing inClosure from every to some closes that leak but breaks browser-server-exports-strip.test.ts:901, which deliberately keeps const { token, client } = loadSecret() because that initialiser is an unresolved global rather than a framework import. Dropping it would be an over-prune. The real fix needs the declaration path to express "is this initialiser safe to drop", mirroring isKnownDroppableSource on the import path — a design addition, not a tidy-up. Full analysis and a ready-to-use RED test are on #607.

One thing worth taking from this branch

patternBindings was exported and pinned by direct seam tests, because the binding/value split is not observable end to end. A naive collector that treats defaults and computed keys as bindings produces byte-identical stage output on every shape probed, and CodeRabbit's independently proposed end-to-end fixture does not discriminate it either (verified — its exact modules give identical output under both collectors). Only the seam tests kill those mutants:

mutation	seam tests
AssignmentPattern.right treated as a binding	FAILED (4 steps)
computed ObjectProperty.key treated as a binding	FAILED (2 steps)

#3861's equivalent guards are end-to-end and would pass under both. If that split is ever meant to hold as an invariant, it needs a test at the seam. Recorded on #607.

No hard feelings toward the duplicate — the residual measurement and the withdrawn follow-up are the useful output.