[james-elicx]

Summary

• preserve middleware rewrite query state in Pages data and HTML responses
• isolate query-varying ISR entries while retaining caching for query-invariant rewrites
• allow middleware rewrites for missing Pages chunks with consistent Node and Worker 404 semantics

Next.js parity

Addresses middleware rewrite query, rewritten SSG, and missing static chunk failures from the middleware-general and middleware-trailing-slash suites.

Validation

• targeted request/data/deploy tests — 388 passed
• targeted Pages/cache tests — 159 passed
• targeted format, lint, and type checks
• independent review loop — clean

Full suites and broad E2E validation are deferred to CI.

[james-elicx]

/bigbonk review for issues

[pkg-pr-new]

Open in StackBlitz

npm i https://pkg.pr.new/@vinext/cloudflare@2216

npm i https://pkg.pr.new/create-vinext-app@2216

npm i https://pkg.pr.new/@vinext/types@2216

npm i https://pkg.pr.new/vinext@2216

commit: 1726eaa

[github-actions]

Example	Preview	Production	Original
app-router-cloudflare	preview	production
pages-router-cloudflare	preview	production
app-router-playground	preview	production	original
realworld-api-rest	preview	production
nextra-docs-template	preview	production
benchmarks	preview	production
hackernews	preview	production	original
workers-cache	preview	production
web	preview	production

[ask-bonk]

Review: middleware rewrite parity

I reproduced the changes against the fixtures and ran the targeted suites (pages-request-pipeline, pages-page-data, pages-page-response — 159 passed). The pipeline/static-asset-miss and prod-handler cache-isolation logic look solid and well-tested. One dev/prod parity bug stands out, plus a couple of smaller notes.

Blocking: dev server skips getStaticProps for rewritten SSG pages (dev/prod divergence)

In server/dev-server.ts, the entire getStaticProps block is now gated on !hasMiddlewareRewrite:

if (
  typeof pageModule.getStaticProps === "function" &&
  !isFallbackRender &&
  !hasMiddlewareRewrite   // <-- skips the whole block, including the getStaticProps() CALL
) { ... }

This disables all ISR handling for a rewritten SSG page — but crucially it also skips calling getStaticProps at all, so the page renders with empty pageProps.

The production handler (pages-page-data.ts resolvePagesPageData) does not do this. There, getStaticProps is still invoked for middleware rewrites; only the cache key is isolated (isrCachePathname with sorted query) and the CDN cache is bypassed (bypassCdnCache). So prod returns correct props; dev returns empty props.

I confirmed this by adding a temporary middleware rewrite (/mw-rewrite-ssg-probe → /isr-test, an SSG page returning message: "Hello from ISR") and fetching it from the dev server. The rendered HTML was:

<div><h1>ISR Page</h1><p data-testid="message"></p><p data-testid="timestamp"></p></div>
...
<script id="__NEXT_DATA__" ...>{"props":{"pageProps":{}},"page":"/isr-test","query":{},...}</script>

pageProps is empty and "Hello from ISR" is absent — getStaticProps never ran. In production the same rewrite would render the static props correctly. This is exactly the kind of dev/prod divergence AGENTS.md warns about ("Always check dev and prod server parity").

The dev server should mirror the prod approach: keep calling getStaticProps, and isolate/bypass the cache (per-query key + no-store) rather than skipping the data-fetch entirely. The current PR has no dev integration test covering a rewrite to a getStaticProps page — all the mw-rewrite-* fixtures target getServerSideProps, so this gap is invisible to the suite. Please add a dev test that rewrites to an SSG page and asserts the props render.

Note: ISR cache growth from per-query keys

getPagesMiddlewareRewriteCacheState writes a distinct ISR entry per unique sorted query string (/posts/post?from=middleware). For query-varying rewrites this is unbounded in the number of distinct query combinations. The CDN response is correctly no-store, but the entries are still persisted via isrSet. With the default in-memory cache handler this is a potential memory-growth vector under adversarial/high-cardinality query traffic. Worth confirming whether Next.js persists these at all, or treats query-varying SSG rewrites as fully dynamic (no ISR write). If Next.js does not write them, we should match that rather than store an entry per query.

Confirmed-correct

• initialStaticAssetMiss is computed pre-basePath-strip in deploy.ts with basePath passed to isNextStaticPath, and the pipeline re-checks the post-rewrite resolvedPathname with empty basePath (already stripped) — consistent across worker and Node.
• The middlewareRewrite filesystem phase + re-probe-then-404 flow matches the new tests and the ported Next.js middleware-general missing-chunk behavior.
• hasMiddlewareRewrite wiring from pipeline → renderOptions → dev SSR handler (7th positional arg) lines up.

[ask-bonk]

Review posted to PR #2216. Summary of findings:

Blocking issue: dev/prod parity bug

server/dev-server.ts skips getStaticProps entirely for middleware-rewritten SSG pages. The whole getStaticProps block is gated on !hasMiddlewareRewrite, which skips not just ISR caching but the actual getStaticProps() call — so rewritten SSG pages render with empty pageProps in dev.

Production (pages-page-data.ts resolvePagesPageData) does the opposite: it still calls getStaticProps and only isolates the cache key (isrCachePathname with sorted query) + bypasses the CDN cache (bypassCdnCache). So prod returns correct props; dev returns empty.

I reproduced this with a temporary rewrite (/mw-rewrite-ssg-probe → /isr-test):

• Dev rendered "props":{"pageProps":{}} and an empty <p data-testid="message"> — getStaticProps never ran.
• The PR's mw-rewrite-* fixtures all target getServerSideProps, so no test catches this gap.

Secondary note

Per-query ISR cache keys (getPagesMiddlewareRewriteCacheState) write an entry per unique sorted query string via isrSet — potential unbounded growth under high-cardinality query traffic. Worth confirming whether Next.js persists these at all.

Verified correct

• initialStaticAssetMiss basePath handling is consistent across worker/Node.
• middlewareRewrite phase + re-probe-then-404 flow matches tests and ported Next.js behavior.
• hasMiddlewareRewrite wiring is correct.
• Targeted suites: 159 passed.

github run

[github-actions]

Performance benchmarks

Compared 1726eaa against base 3d30a3c using alternating same-runner rounds. Next.js was unchanged and skipped.

0 improved · 0 regressed · 6 within ±1.5%

Scenario	Framework	Baseline	Current	Change
Client bundle size (gzip)	vinext	130.6 KB	130.6 KB	⚫ +0.0%
Client entry size (gzip)	vinext	118.1 KB	118.1 KB	⚫ +0.0%
Dev server cold start	vinext	2.94 s	2.98 s	⚫ +1.4%
Production build time	vinext	3.18 s	3.21 s	⚫ +0.9%
RSC entry closure size (gzip)	vinext	101.8 KB	101.8 KB	⚫ -0.0%
Server bundle size (gzip)	vinext	169.1 KB	169.1 KB	⚫ +0.0%

View detailed results and traces

_{🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head}

[james-elicx]

/bigbonk review for issues

[ask-bonk]

Review: middleware rewrite parity (re-review after 3b0be5e)

The blocking dev/prod parity bug from the earlier review (getStaticProps skipped entirely for rewritten SSG pages) is fixed by 3b0be5e "fix(pages): run static props after middleware rewrites". The dev gate is now !isFallbackRender (dev-server.ts:997), hasMiddlewareRewrite only bypasses the cache read (dev-server.ts:1003), emits no-store (dev-server.ts:1708), and skips the cache write (dev-server.ts:1811). getStaticProps now runs for rewritten SSG pages. Good.

The static-asset-miss → middleware-rewrite → re-probe → 404 flow is consistent across worker (deploy.ts) and Node (prod-server.ts), and the pipeline tests cover all three branches (plain 404, rewrite-to-served, rewrite-to-missing-asset). getPagesMiddlewareRewriteCacheState is well-tested.

Dev/prod divergence on query-invariant rewrites — and the new E2E test only covers dev

The new E2E test (tests/e2e/pages-router/isr.spec.ts — "middleware rewrite to an SSG page still runs getStaticProps") asserts:

expect(res.headers()["cache-control"]).toBe("no-store, must-revalidate");

That test lives in tests/e2e/pages-router/, whose Playwright project runs vp dev (playwright.config.ts:34-44). The prod project (pages-router-prod) has a different testDir (tests/e2e/pages-router-prod), so this test never runs against a production build. It therefore only validates dev behavior — and dev and prod do not agree here:

• Prod computes cache state from the resolved URL (pages-page-handler.ts:441). For /mw-rewrite-isr → /isr-test (no query), getPagesMiddlewareRewriteCacheState("/isr-test", true) returns { cachePathname: "/isr-test", bypassCdnCache: false }. So prod:
  • reads/writes the ISR cache under the bare pages:/isr-test key, shared with direct /isr-test requests, and
  • emits normal MISS s-maxage cache-control — no-store is only set when bypassCdnCache is true (pages-page-response.ts:640).
• Dev receives only the boolean hasMiddlewareRewrite (index.ts:4290), never the bypassCdnCache/isrCachePathname distinction. So for any rewrite — query or not — dev bypasses the cache and emits no-store.

Net result for /mw-rewrite-isr → /isr-test:

• Dev: Cache-Control: no-store, must-revalidate, no cache entry. ✅ (matches the test)
• Prod: Cache-Control: s-maxage=... (MISS), reads/writes pages:/isr-test. ❌ (test never runs here; would fail the assertion)

This is the same class of dev/prod parity issue AGENTS.md warns about, just shifted from "skips getStaticProps" to "caches differently." Recommended:

1. Decide the intended behavior and make dev and prod agree. Either query-invariant rewrites cache under the resolved key in both (test should assert s-maxage and the suite should exercise prod), or they're no-store in both. Dev currently can't distinguish the two cases because the pipeline only forwards a boolean — thread the getPagesMiddlewareRewriteCacheState result (or at least bypassCdnCache + cachePathname) into createSSRHandler so dev mirrors prod exactly.
2. Add a prod E2E (or pages-router.test.ts) assertion for this scenario so the prod cache-control/cache-key behavior is actually covered. As written, the only test guards dev.
3. Confirm the cache-sharing semantics. In prod a query-invariant rewrite to an SSG page now shares the target's pages:/isr-test entry with direct hits. Probably fine (same output), but please confirm against Next.js whether a rewrite-to-SSG shares the target's cache vs. being treated as dynamic, and note the behavior change in the PR description.

Note (carried over): per-query ISR entries persist in prod, not dev (a third divergence)

pages-page-response.ts:597-623 writes an ISR entry whenever isrRevalidateSeconds > 0, ignoring bypassCdnCache. For a query-varying rewrite (bypassCdnCache: true, key /target?a=1&z=2) the CDN response is correctly no-store, but the entry is still persisted via isrSet — one entry per unique sorted query string. The PR's test (stores query-varying rewrite ISR output but keeps the CDN response private) codifies this as intentional. Dev does not write these (dev-server.ts:1811), so this is a third dev/prod split. Under high-cardinality/adversarial query traffic with the default in-memory cache handler, the prod write is an unbounded-growth vector. Worth confirming against Next.js whether query-varying SSG rewrites persist at all or are treated as fully dynamic (no ISR write), and aligning dev and prod either way.

Summary

• Blocking parity bug from prior review: resolved by 3b0be5e.
• Static-asset-miss rewrite flow: correct and well-tested across runtimes.
• New concern: query-invariant rewrites emit no-store/no-write in dev but s-maxage/shared-key/write in prod. The new E2E test only runs against dev, so the prod path is unverified and currently divergent. Reconcile dev/prod and add prod coverage.
• Note: per-query ISR writes persist in prod but not dev — confirm Next.js parity and align.

[ask-bonk]

To https://github.com/cloudflare/vinext.git
! [rejected] codex/fix-middleware-rewrite-parity -> codex/fix-middleware-rewrite-parity (fetch first)
error: failed to push some refs to 'https://github.com/cloudflare/vinext.git'
hint: Updates were rejected because the remote contains work that you do not
hint: have locally. This is usually caused by another repository pushing to
hint: the same ref. If you want to integrate the remote changes, use
hint: 'git pull' before pushing again.
hint: See the 'Note about fast-forwards' in 'git push --help' for details.

github run

[ask-bonk]

@james-elicx Bonk workflow was cancelled.

View workflow run · To retry, trigger Bonk again.

[james-elicx]

/bigbonk review for issues

[james-elicx]

/bigbonk review for issues

[ask-bonk]

Re-review (HEAD 65bb56c) — issues persist

No new commits have landed since the prior re-review; HEAD is still 65bb56c. The dev/prod cache-parity concern from the 00:43 review is unresolved, and on closer inspection it affects both new E2E scenarios, not just the ISR one. I independently re-verified the code paths below.

Blocking: dev and prod disagree on cache-control for query-invariant rewrites, and both new E2E tests only run in dev

The pipeline forwards only a boolean hasMiddlewareRewrite to the dev SSR handler (index.ts:4290, dev-server.ts 7th positional arg). Dev therefore treats every rewrite identically — bypass cache + no-store — while prod uses the richer getPagesMiddlewareRewriteCacheState result ({ cachePathname, bypassCdnCache }) and only emits no-store when bypassCdnCache is true (i.e. only for query-varying rewrites).

getPagesMiddlewareRewriteCacheState (pages-page-handler.ts:92) returns bypassCdnCache: false whenever the rewrite target has no query string. Verified:

• getPagesMiddlewareRewriteCacheState("/isr-test", true) → { cachePathname: "/isr-test", bypassCdnCache: false }
• getPagesMiddlewareRewriteCacheState("/isr-test?from=mw", true) → { cachePathname: "/isr-test?from=mw", bypassCdnCache: true }

Both new E2E tests rewrite to query-less targets, so prod takes the bypassCdnCache: false branch in both cases:

1. /mw-rewrite-isr → /isr-test (ISR page). Test asserts cache-control: no-store, must-revalidate.

• Dev: no-store (matches). ✅
• Prod: bypassCdnCache:false + isrRevalidateSeconds>0 → pages-page-response.ts:642 emits MISS s-maxage, and the response writes/reads the shared pages:/isr-test ISR key (pages-page-response.ts:608, pages-page-data.ts:773). Not no-store. ❌

2. /mw-rewrite-static-gsp → /static-gsp (non-ISR getStaticProps, no revalidate). Test asserts cache-control: no-store, must-revalidate for both the HTML and the _next/data JSON.

• Dev: forces no-store for any rewrite+GSP (HTML dev-server.ts:1708; data dev-server.ts:1453). ✅
• Prod HTML: bypassCdnCache:false, isrRevalidateSeconds is null, not GSSP → none of the Cache-Control branches in renderPagesPageResponse fire (pages-page-response.ts:640-658). No no-store is set. ❌
• Prod data: buildPagesCacheResponse only sets no-store when bypassCdnCache is true (pages-page-data.ts:507). Here it's false. ❌

Both new tests live in tests/e2e/pages-router/, whose Playwright project runs vp dev (playwright.config.ts:34-38). The prod project (pages-router-prod) has a separate testDir (tests/e2e/pages-router-prod/) and does not include these scenarios (confirmed — only gssp-data-dedup, interactive, javascript-urls, production specs there). So the prod paths above are never exercised, and the no-store assertions would fail if they were.

This is the dev/prod parity class AGENTS.md explicitly warns about ("Always check dev and prod server parity"). Recommended:

1. Make dev and prod agree. Thread the getPagesMiddlewareRewriteCacheState result (bypassCdnCache + cachePathname) into createSSRHandler instead of a bare boolean, so dev mirrors prod exactly: query-invariant rewrites cache under the resolved key, query-varying rewrites get no-store. Today dev simply cannot distinguish the two cases.
2. Add prod coverage. Add a pages-router-prod E2E (or a pages-router.test.ts assertion) for both scenarios so the prod cache-control / cache-key behavior is actually verified. As written, the only tests guard dev.
3. Confirm cache-sharing semantics vs Next.js. In prod a query-invariant rewrite to an SSG page now shares the target's pages:/isr-test entry with direct hits. Confirm against Next.js whether a rewrite-to-SSG shares the target's cache or is treated as dynamic, and note the behavior change in the PR description.

Note (carried over): per-query ISR entries persist in prod, not dev

pages-page-response.ts:598-623 writes an ISR entry whenever isrRevalidateSeconds > 0, ignoring bypassCdnCache. For a query-varying rewrite (bypassCdnCache:true, key /target?a=1&z=2) the CDN response is correctly no-store, but the entry is still persisted via isrSet — one entry per unique sorted query string. The PR's pages-page-response.test.ts case (stores query-varying rewrite ISR output but keeps the CDN response private) codifies this as intentional. Dev does not write these (dev-server.ts:1811), so this is a third dev/prod split. Under high-cardinality/adversarial query traffic with the default in-memory cache handler, this is an unbounded-growth vector. Worth confirming against Next.js whether query-varying SSG rewrites persist at all (or are treated as fully dynamic with no ISR write) and aligning dev and prod either way.

Confirmed correct

• initialStaticAssetMiss is computed pre-basePath-strip in deploy.ts with basePath passed to isNextStaticPath; the pipeline re-checks the post-rewrite resolvedPathname with empty basePath — consistent across worker and Node.
• The middlewareRewrite filesystem phase + re-probe-then-404 flow matches the new pipeline tests and the ported Next.js middleware-general missing-chunk behavior (middleware.ts _app-non-existent.js → /about).
• getPagesMiddlewareRewriteCacheState query sorting / bypass logic is well unit-tested in pages-page-data.test.ts and pages-page-response.test.ts.

Summary

• Blocking: query-invariant middleware rewrites emit no-store/no-cache in dev but s-maxage/shared-key/write (ISR) or no-cache-control (non-ISR GSP) in prod. Both new E2E tests target query-less rewrites and run only against vp dev, so the divergent prod paths are unverified and would fail the no-store assertions. Reconcile dev/prod by threading the full cache-state into dev, and add prod coverage.
• Note: per-query ISR writes persist in prod but not dev — confirm Next.js parity and align.
• Static-asset-miss rewrite flow and the missing-chunk 404 behavior: correct and well-tested across runtimes.

[ask-bonk]

To https://github.com/cloudflare/vinext.git
! [rejected] codex/fix-middleware-rewrite-parity -> codex/fix-middleware-rewrite-parity (fetch first)
error: failed to push some refs to 'https://github.com/cloudflare/vinext.git'
hint: Updates were rejected because the remote contains work that you do not
hint: have locally. This is usually caused by another repository pushing to
hint: the same ref. If you want to integrate the remote changes, use
hint: 'git pull' before pushing again.
hint: See the 'Note about fast-forwards' in 'git push --help' for details.

github run

[james-elicx]

/bigbonk review for issues

[james-elicx]

/bigbonk review for issues

[ask-bonk]

Re-review (HEAD 77bbae6) — blocking dev/prod parity issue still unresolved

A new commit landed since the last re-review: 77bbae6 "fix(pages): match dev rewrite cache control". But it does not resolve the dev/prod divergence flagged in the prior three reviews — it only renames the dev cache-control string and updates the dev-only E2E assertions to match. The structural problem stands. I re-verified every path below against HEAD.

What 77bbae6 actually changed

It swapped the dev cache-control literal from no-store, must-revalidate to no-cache, must-revalidate in two spots (dev-server.ts:1457 data, dev-server.ts:1710 HTML) and updated the three assertions in tests/e2e/pages-router/isr.spec.ts:32,41,50 to expect the new string. No code was added to make dev distinguish query-invariant from query-varying rewrites, and no prod coverage was added.

Blocking: dev emits no-cache for all GSP rewrites; prod only does so for query-varying rewrites

The pipeline still forwards a bare boolean to dev (index.ts:4290 → dev-server.ts:477 7th positional arg hasMiddlewareRewrite). Dev therefore treats every rewrite to a getStaticProps page identically: force no-cache, must-revalidate and bypass the cache (dev-server.ts:1456-1457, 1710, 1003, 1811).

Prod uses the richer getPagesMiddlewareRewriteCacheState(routeUrl, hasRewrite) (pages-page-handler.ts:441), which returns bypassCdnCache: false whenever the rewrite target has no query string (pages-page-handler.ts:97-98):

• getPagesMiddlewareRewriteCacheState("/isr-test", true) → { cachePathname: "/isr-test", bypassCdnCache: false }
• getPagesMiddlewareRewriteCacheState("/static-gsp", true) → { cachePathname: "/static-gsp", bypassCdnCache: false }

Both new fixture rewrites target query-less paths (middleware.ts:33-39: /mw-rewrite-isr → /isr-test, /mw-rewrite-static-gsp → /static-gsp), so prod takes the bypassCdnCache: false branch in both cases — i.e. prod does not emit no-cache/no-store:

1. /mw-rewrite-isr → /isr-test (ISR, revalidate: 1). Test asserts HTML cache-control: no-cache, must-revalidate.

• Dev: no-cache, must-revalidate. ✅ (matches test)
• Prod: bypassCdnCache:false + isrRevalidateSeconds>0 → pages-page-response.ts:642 emits a MISS s-maxage via applyCdnResponseHeaders, and reads/writes the shared pages:/isr-test ISR key (pages-page-response.ts:608-609, pages-page-data.ts:773-774). Not no-cache. ❌

2. /mw-rewrite-static-gsp → /static-gsp (non-ISR getStaticProps, no revalidate). Test asserts both HTML and _next/data JSON are no-cache, must-revalidate.

• Dev: forces no-cache for any rewrite+GSP (HTML dev-server.ts:1710; data dev-server.ts:1456-1457). ✅
• Prod HTML: bypassCdnCache:false, isrRevalidateSeconds null, not GSSP → none of the Cache-Control branches at pages-page-response.ts:640-657 fire. No no-cache set. ❌
• Prod data: for a getStaticProps page gsspRes is null, so the _next/data branch at pages-page-handler.ts:694-707 never sets Cache-Control; buildPagesCacheResponse only sets no-store when bypassCdnCache is true (pages-page-data.ts:507-508), which is false here. No no-cache set. ❌

The new E2E tests only run against dev, so the prod paths are unverified

Both new tests live in tests/e2e/pages-router/isr.spec.ts, and the pages-router Playwright project runs vp dev (playwright.config.ts:34-38, command: "... vp dev --port 4173", baseURL: localhost:4173). The prod project pages-router-prod uses a separate testDir: ./tests/e2e/pages-router-prod (playwright.config.ts:100) and does not include these scenarios. So the no-cache assertions only ever exercise dev — the divergent prod paths above are never run and would fail those same assertions.

This is exactly the dev/prod parity class AGENTS.md calls out ("Always check dev and prod server parity"). The earlier getStaticProps-skip bug was fixed by 3b0be5e; this is the same divergence shifted to "caches differently," and 77bbae6 did not close it.

Recommended:

1. Make dev and prod agree. Thread the getPagesMiddlewareRewriteCacheState result ({ cachePathname, bypassCdnCache }) from the pipeline into createSSRHandler instead of the bare boolean, so dev mirrors prod: query-invariant rewrites cache under the resolved key (s-maxage / shared key), query-varying rewrites get no-store + bypass. Dev currently cannot distinguish the two.
2. Add prod coverage. Add a pages-router-prod E2E (or pages-router.test.ts) assertion for both scenarios so the prod cache-control / cache-key behavior is actually verified, not just dev.
3. Confirm cache-sharing semantics vs Next.js. In prod a query-invariant rewrite to an SSG page now shares the target's pages:/isr-test entry with direct /isr-test hits. Confirm against Next.js whether a rewrite-to-SSG shares the target's cache or is treated as dynamic, and note the behavior change in the PR description.

Note (carried over): per-query ISR entries persist in prod, not dev

pages-page-response.ts:597-623 writes an ISR entry whenever isrRevalidateSeconds > 0, ignoring bypassCdnCache. For a query-varying rewrite (bypassCdnCache:true, key /target?a=1&z=2) the CDN response is correctly no-store, but the entry is still persisted via isrSet — one entry per unique sorted query string. pages-page-response.test.ts ("stores query-varying rewrite ISR output but keeps the CDN response private") codifies this as intentional. Dev does not write these (dev-server.ts:1811), a third dev/prod split. Under high-cardinality/adversarial query traffic with the default in-memory cache handler, this is an unbounded-growth vector. Confirm against Next.js whether query-varying SSG rewrites persist at all (or are treated as fully dynamic, no ISR write) and align dev and prod either way.

Confirmed correct

• initialStaticAssetMiss is computed pre-basePath-strip in deploy.ts (isNextStaticPath(pathname, basePath, assetPathPrefix)) and prod-server.ts, and the pipeline re-checks the post-rewrite resolvedPathname with empty basePath — consistent across worker and Node.
• The middlewareRewrite filesystem phase + re-probe-then-404 flow matches the new pipeline tests and the ported Next.js middleware-general missing-chunk behavior (middleware.ts:43-45 _app-non-existent.js → /about).
• getPagesMiddlewareRewriteCacheState query sorting / bypass logic is well unit-tested in pages-page-data.test.ts and pages-page-response.test.ts (96 passed locally).

Summary

• Blocking (unresolved): query-invariant middleware rewrites emit no-cache/no-write in dev but s-maxage/shared-key/write (ISR) or no-cache-control (non-ISR GSP) in prod. 77bbae6 only changed the dev string and the dev-only assertions; it did not reconcile dev/prod or add prod coverage. Both new E2E tests run only against vp dev, so the divergent prod paths are untested. Thread the full cache-state into dev and add prod coverage.
• Note: per-query ISR writes persist in prod but not dev — confirm Next.js parity and align.
• Static-asset-miss rewrite flow and missing-chunk 404 behavior: correct and well-tested across runtimes.

[ask-bonk]

Review posted to PR #2216.

Summary

HEAD is now 77bbae6 ("fix(pages): match dev rewrite cache control"), a new commit since the last re-review. I verified it does not resolve the blocking dev/prod parity issue raised in all three prior reviews.

What the new commit actually did: It only swapped the dev cache-control literal from no-store, must-revalidate to no-cache, must-revalidate (dev-server.ts:1457, :1710) and updated the three dev-only E2E assertions to match (isr.spec.ts:32,41,50). No structural change.

Blocking issue (unresolved):

• The pipeline still forwards a bare boolean hasMiddlewareRewrite to dev (index.ts:4290), so dev forces no-cache for every rewrite to a GSP page.
• Prod uses getPagesMiddlewareRewriteCacheState, which returns bypassCdnCache: false for query-less rewrite targets. Both new fixture rewrites (/mw-rewrite-isr → /isr-test, /mw-rewrite-static-gsp → /static-gsp) are query-less, so prod emits s-maxage+shared-key (ISR) or no cache-control (non-ISR GSP) — never no-cache.
• Both new E2E tests run only under vp dev (pages-router Playwright project); the pages-router-prod project has a separate testDir that doesn't include these scenarios. So the divergent prod paths are never exercised and would fail the assertions.

Recommendations: thread the full { cachePathname, bypassCdnCache } state into the dev SSR handler, add prod E2E coverage, and confirm cache-sharing semantics vs Next.js.

I also carried over the per-query ISR persistence note (prod writes, dev doesn't) and confirmed the static-asset-miss rewrite flow and missing-chunk 404 behavior are correct and well-tested across runtimes (96 unit tests passing locally).

github run

[james-elicx]

/bigbonk review for issues

[ask-bonk]

Re-review (HEAD 09e99fb) — blocking dev/prod parity issue resolved

A new commit landed since the last re-review: 09e99fb "fix(pages): align middleware rewrite cache state". Unlike 77bbae6 (which only renamed the dev cache-control literal), this commit does the structural fix the prior three reviews asked for. I re-verified every path below against HEAD and ran the targeted suites.

Blocking issue from prior reviews: fixed

The pipeline no longer forwards a bare boolean to dev. It now threads the full getPagesMiddlewareRewriteCacheState result into createSSRHandler (index.ts:4283-4298), and the dev handler's 7th arg is now middlewareRewriteCacheState?: PagesMiddlewareRewriteCacheState (dev-server.ts:478). Dev derives the same three values prod uses (dev-server.ts:545-547):

• hasMiddlewareRewrite = state !== undefined
• rewriteCachePathname = state?.cachePathname ?? url.split("?")[0]
• bypassMiddlewareRewriteCache = state?.bypassCdnCache === true

Every dev gate that previously keyed off the bare boolean now keys off bypassMiddlewareRewriteCache and uses rewriteCachePathname for the ISR key: cache read (dev-server.ts:1008), data persist (dev-server.ts:1437), data no-cache (dev-server.ts:1462), HTML no-cache (dev-server.ts:1716), ISR write (dev-server.ts:1822, :1838), and fallback (dev-server.ts:792-794). So dev now only forces no-cache for query-varying rewrites (bypassCdnCache: true) and falls through to normal ISR/s-maxage handling for query-invariant rewrites — matching prod.

The function was also extracted into a shared module server/pages-middleware-rewrite-cache.ts and re-exported from pages-page-handler.ts, so dev (index.ts) and prod (pages-page-handler.ts:428) call the identical implementation. This closes the divergence at the source rather than papering over it.

I confirmed dev and prod now agree on the two query-less fixture rewrites:

• /mw-rewrite-isr → /isr-test (ISR, revalidate: 1): bypassCdnCache:false, isrRevalidateSeconds>0. Dev hits dev-server.ts:1718-1724 → buildMissIsrCacheControl(1) (s-maxage=1, MISS); prod hits pages-page-response.ts:642-652 → same. Both read/write the shared pages:/isr-test key. ✅
• /mw-rewrite-static-gsp → /static-gsp (non-ISR GSP): bypassCdnCache:false, no revalidate, not GSSP. Dev: no Cache-Control branch fires; prod: none of pages-page-response.ts:640-657 fires. Both emit no Cache-Control. ✅

Prod coverage: added

A new pages-router-prod E2E spec now exercises both scenarios against a real production build: tests/e2e/pages-router-prod/middleware-rewrite-cache.spec.ts. That project runs cli.js build && cli.js start --port 4175 (playwright.config.ts:99-110) against tests/fixtures/pages-basic (build id test-build-id, matching the data-URL assertion). It asserts the ISR rewrite returns s-maxage=1 and the static-GSP rewrite (HTML + _next/data JSON) returns no Cache-Control — i.e. the actual prod behavior, now identical to the dev isr.spec.ts assertions (isr.spec.ts:32,39,48). Recommendation #2 from the prior reviews is satisfied.

Targeted suites pass locally: pages-request-pipeline, pages-page-data, pages-page-response, pages-router — 476 passed. Format/lint/type checks clean on the changed files.

Remaining note (carried over): per-query ISR write still persists in prod but not dev

This is the one item not addressed by 09e99fb. For a query-varying rewrite (bypassCdnCache: true):

• Dev skips the ISR write — the write is gated on !bypassMiddlewareRewriteCache (dev-server.ts:1818-1822), and the data persist on !bypassMiddlewareRewriteCache (dev-server.ts:1437).
• Prod still writes one ISR entry per unique sorted query string. The write in pages-page-response.ts:597-623 is gated only on !scriptNonce && isrRevalidateSeconds > 0 — it does not check bypassCdnCache. The CDN response is correctly no-store (pages-page-response.ts:640), but the per-query entry is still persisted via schedulePagesIsrCacheWrite under the isolated cachePathname key.

So dev and prod still disagree for query-varying rewrites: prod populates the cache (under an isolated key), dev does not. pages-page-response.test.ts ("stores query-varying rewrite ISR output but keeps the CDN response private") codifies the prod write as intentional. Under high-cardinality/adversarial query traffic with the default in-memory cache handler, this remains an unbounded-growth vector. Recommend: confirm against Next.js whether query-varying SSG rewrites persist at all (or are treated as fully dynamic with no ISR write) and align dev and prod either way. Non-blocking, but worth resolving or explicitly documenting in the PR description.

Confirmed correct (unchanged)

• initialStaticAssetMiss is computed pre-basePath-strip in deploy.ts / prod-server.ts, and the pipeline re-checks the post-rewrite resolvedPathname with empty basePath — consistent across worker and Node.
• The middlewareRewrite filesystem phase + re-probe-then-404 flow matches the pipeline tests and the ported Next.js middleware-general missing-chunk behavior.
• getPagesMiddlewareRewriteCacheState query sorting / bypass logic is well unit-tested (pages-page-data.test.ts, pages-page-response.test.ts, pages-request-pipeline.test.ts).

Summary

• Blocking dev/prod cache-control parity bug: resolved. 09e99fb threads the full { cachePathname, bypassCdnCache } state into dev via a shared function, so dev and prod now agree for query-invariant rewrites, and prod E2E coverage was added. Recommendations Virtual module imports break esbuild dependency optimization when vinext is installed from npm #1 and Published npm package ships sourcemaps pointing to missing source files #2 from the prior reviews are done.
• Remaining (non-blocking): query-varying rewrites still write a per-query ISR entry in prod but not dev. Confirm Next.js parity and align dev/prod (or document the intentional behavior). This is recommendation fix: exclude vinext from optimizeDeps to prevent virtual module resolution errors #3 / the carried-over note.
• Static-asset-miss rewrite flow and missing-chunk 404 behavior: correct and well-tested across runtimes.

[ask-bonk]

Review posted to PR #2216.

Summary

HEAD is now 09e99fb ("fix(pages): align middleware rewrite cache state"), a new commit since the last re-review. I verified it resolves the blocking dev/prod parity issue raised in the prior three reviews.

What the new commit did (the real structural fix):

• The pipeline now threads the full getPagesMiddlewareRewriteCacheState result ({ cachePathname, bypassCdnCache }) into the dev SSR handler instead of a bare boolean (index.ts:4283-4298, dev-server.ts:478).
• Dev now distinguishes query-invariant from query-varying rewrites: it only forces no-cache when bypassCdnCache is true, and otherwise falls through to normal s-maxage/ISR handling — matching prod.
• The function was extracted into a shared module pages-middleware-rewrite-cache.ts so dev and prod call the identical implementation (divergence closed at the source).
• A new pages-router-prod E2E spec (middleware-rewrite-cache.spec.ts) exercises both scenarios against a real production build — the prod coverage the prior reviews asked for.

Verification:

• Dev and prod now agree on both query-less fixture rewrites (ISR → s-maxage=1; static-GSP → no Cache-Control).
• Targeted suites: 476 passed. Format/lint/type checks clean.

One remaining non-blocking note (carried over): query-varying rewrites still write a per-query ISR entry in prod (pages-page-response.ts:597-623, not gated on bypassCdnCache) but not in dev. Recommend confirming Next.js parity and aligning, or documenting the intentional behavior.

The blocking issue is resolved; recommendations #1 and #2 are done, and #3 (the per-query write divergence) remains as a documented follow-up.

github run

[james-elicx]

/bigbonk review for issues

[ask-bonk]

Re-review (HEAD 09e99fb) — blocking parity issue stays resolved; one non-blocking divergence remains

No new commits since the last re-review (HEAD is still 09e99fb "fix(pages): align middleware rewrite cache state"). I independently re-verified every path against HEAD and ran the targeted suites — pages-page-data, pages-page-response, pages-request-pipeline: 159 passed.

Blocking dev/prod cache-control parity bug: confirmed resolved

The structural fix is in place. The pipeline threads the full getPagesMiddlewareRewriteCacheState result into the dev SSR handler instead of a bare boolean:

• index.ts:4283-4298 passes getPagesMiddlewareRewriteCacheState(pipelineResult.resolvedUrl, hasMiddlewareRewrite) as the 7th arg.
• dev-server.ts:478 types it as middlewareRewriteCacheState?: PagesMiddlewareRewriteCacheState.
• dev-server.ts:545-547 derives hasMiddlewareRewrite, rewriteCachePathname, and bypassMiddlewareRewriteCache from it.

Every dev gate now keys off bypassMiddlewareRewriteCache (cache read :1004, data persist :1437, data no-cache :1462, HTML no-cache :1716, ISR write :1818, fallback :790) and uses rewriteCachePathname for the ISR key. So dev only forces no-cache for query-varying rewrites and falls through to normal s-maxage/ISR handling for query-invariant rewrites — matching prod.

The function lives in the shared server/pages-middleware-rewrite-cache.ts module re-exported from pages-page-handler.ts:53, so dev (index.ts) and prod (pages-page-handler.ts:428) call the identical implementation. Divergence closed at the source.

Verified dev and prod agree on both query-less fixture rewrites:

• /mw-rewrite-isr → /isr-test (ISR, revalidate: 1): bypassCdnCache:false, isrRevalidateSeconds>0 → both emit s-maxage=1 MISS and read/write the shared pages:/isr-test key.
• /mw-rewrite-static-gsp → /static-gsp (non-ISR GSP): bypassCdnCache:false, no revalidate → both emit no Cache-Control.

Prod E2E coverage was added in tests/e2e/pages-router-prod/middleware-rewrite-cache.spec.ts, exercising both scenarios against a real production build (cli.js build && cli.js start). Prior recommendations #1 and #2 are done.

Remaining (non-blocking): per-query ISR write persists in prod but not dev

The one item not addressed. For a query-varying rewrite (bypassCdnCache: true):

• Dev skips both the ISR write (dev-server.ts:1818, gated on !bypassMiddlewareRewriteCache) and the data persist (dev-server.ts:1437).
• Prod still writes one ISR entry per unique sorted query string. The write in pages-page-response.ts:597-623 is gated only on !options.scriptNonce && isrRevalidateSeconds > 0 — it does not check bypassCdnCache. The CDN response is correctly no-store (pages-page-response.ts:640), but the per-query entry is persisted via schedulePagesIsrCacheWrite under the isolated cachePathname key.

pages-page-response.test.ts ("stores query-varying rewrite ISR output but keeps the CDN response private") codifies the prod write as intentional, so this is a deliberate behavior — but it is still a third dev/prod split, and under high-cardinality / adversarial query traffic with the default in-memory cache handler it is an unbounded-growth vector. Recommend confirming against Next.js whether query-varying SSG rewrites persist at all (or are treated as fully dynamic with no ISR write), aligning dev and prod either way, and noting the intended behavior in the PR description.

Confirmed correct (unchanged)

• initialStaticAssetMiss is computed pre-basePath-strip in deploy.ts (isNextStaticPath(pathname, basePath, assetPathPrefix)) and prod-server.ts; the pipeline re-checks the post-rewrite resolvedPathname with empty basePath — consistent across worker and Node.
• The middlewareRewrite filesystem phase + re-probe-then-404 flow matches the new pipeline tests and the ported Next.js middleware-general missing-chunk behavior.
• getPagesMiddlewareRewriteCacheState query sorting / bypass logic is well unit-tested. pages-page-response.ts:646's cache-tag uses routeUrl (not isrCachePathname), which is equivalent for query-invariant rewrites and unreachable for query-varying ones (those take the bypassCdnCache branch at :640) — no bug.

Summary

• Blocking dev/prod cache-control parity bug: resolved — full cache-state threaded into dev via a shared function, prod E2E coverage added.
• Remaining (non-blocking): query-varying rewrites write a per-query ISR entry in prod but not dev. Confirm Next.js parity, align dev/prod, and document the intended behavior.
• Static-asset-miss rewrite flow and missing-chunk 404 behavior: correct and well-tested across runtimes.

[ask-bonk]

Review posted to PR #2216.

Summary

HEAD is 09e99fb — no new commits since the last re-review. I independently re-verified every code path and ran the targeted suites (pages-page-data, pages-page-response, pages-request-pipeline — 159 passed).

Blocking dev/prod cache-control parity bug: confirmed resolved. 09e99fb threads the full getPagesMiddlewareRewriteCacheState result ({ cachePathname, bypassCdnCache }) into the dev SSR handler (index.ts:4283-4298, dev-server.ts:478) instead of a bare boolean. Dev now distinguishes query-invariant from query-varying rewrites and matches prod. The function lives in a shared module so dev and prod call the identical implementation. Prod E2E coverage was added (tests/e2e/pages-router-prod/middleware-rewrite-cache.spec.ts). Prior recommendations #1 and #2 are done.

One remaining non-blocking divergence: query-varying rewrites still write a per-query ISR entry in prod (pages-page-response.ts:597-623, not gated on bypassCdnCache) but not in dev. The CDN response is correctly no-store, and the prod write is codified as intentional in pages-page-response.test.ts — but it's a dev/prod split and a potential unbounded-growth vector under high-cardinality query traffic. Recommend confirming Next.js parity, aligning dev/prod, and documenting the intended behavior in the PR description.

Confirmed correct: static-asset-miss rewrite flow (deploy.ts / prod-server.ts basePath handling consistent across worker/Node), the middlewareRewrite phase + re-probe-then-404 flow, and getPagesMiddlewareRewriteCacheState query sorting/bypass logic.

github run

[james-elicx]

Worker C pass for deploy-suite run 28478866791 / job 84413308650: I think this PR already covers the relevant Pages Router middleware rewrite/query/cache-state issue family behind the middleware-general and middleware-trailing-slash failures, and it likely overlaps the middleware-rewrites failures called out in that run as well. I am skipping a duplicate fix/worktree for that backlog item.

[james-elicx]

Pages Router middleware/trailing-slash cluster pass for deploy-suite run 28478866791 / job 84413308650: this PR appears to already cover the relevant Pages Router middleware rewrite/data-request/cache-state failures from middleware-trailing-slash and the overlapping middleware-general suite. Skipping a duplicate implementation for this assignment.

[james-elicx]

Run 28938793088 (Next.js v16.3.0-canary.80) still fails the Pages Router middleware rewrite behavior this PR owns, including:

• test/e2e/middleware-general/test/index.test.ts and node-runtime.test.ts — rewritten SSG query/data-request/shallow-link assertions
• test/e2e/middleware-trailing-slash/test/index.test.ts — the corresponding trailing-slash assertions

I think this PR is the existing fix for those failures, so I am skipping duplicate implementation in the current functional-backlog pass.

[james-elicx]

Run 29871622126 / job 88775791401 still fails the functional Pages middleware rewrite cluster in middleware-general and middleware-trailing-slash: middleware-added query values are dropped before SSG props/router state, and shallow linking through the rewrite fails. I think this PR owns the rewrite/data-state portion of that cluster. I am keeping it mapped here rather than opening a duplicate fix; current-main revival must rerun the exact targeted v16.2.6 suites.

[james-elicx]

/bigbonk review for issues

[james-elicx]

/bigbonk review for issues

[james-elicx]

/bigbonk review for issues

[ask-bonk]

@james-elicx Bonk workflow was cancelled.

View workflow run · To retry, trigger Bonk again.

[james-elicx]

/bigbonk review for issues

[james-elicx]

/bigbonk review for issues

[ask-bonk]

Re-review (HEAD d3daec8) — no blocking issues; two non-blocking notes

The branch has been reworked since the previously-approved 09e99fb (new commits: aeb0ba4 align cache state, 758ce89 preserve render state, d3daec8 expect static GSP cache header). I re-verified every changed path against HEAD and ran the targeted suites — all green:

• pages-page-data + pages-page-handler + pages-page-response: 170 passed
• pages-request-pipeline + deploy + features + shims: 1992 passed
• pages-router: 374 passed
• vp check on all 9 changed source files: clean (format, lint, types)

Dev/prod cache-control parity — resolved, and cleaner than before

The prior review chain's blocking concern (dev vs prod disagreeing on cache-control for query-invariant rewrites) is resolved. The prod side threads getPagesMiddlewareRewriteCacheState(renderRouteUrl, hasMiddlewareRewrite) into a single rewriteCacheState (pages-page-handler.ts:612) that drives the ISR key (isrCachePathname, :616), the _next/data cache-control (:961), the HTML cache-control (pages-page-response.ts:674), and cached HIT/STALE headers (pages-page-data.ts:452, :936). bypassCdnCache is true only for query-varying rewrites (pages-middleware-rewrite-cache.ts:11).

The dev handler no longer needs to mirror the full cache-state machinery: dev never persists ISR (Next.js re-runs getStaticProps every request) and always emits DEV_PAGES_CACHE_CONTROL = "no-cache, must-revalidate" for all GSP pages — direct hits and rewrites alike (dev-server.ts:1431, :1640). So dev is internally consistent (a rewrite behaves like a direct hit), prod is internally consistent, and the only dev↔prod difference is the inherent "dev never caches GSP" model, not something this PR introduces. middlewareRewriteCacheState is used in dev solely to keep the request query in the static-props render + __NEXT_DATA__ (dev-server.ts:870, :1594), matching prod's post-rewrite query threading.

Prod E2E coverage exists in tests/e2e/pages-router-prod/middleware-rewrite-cache.spec.ts (real cli.js build && cli.js start), asserting the ISR rewrite (s-maxage=1), the query-invariant static-GSP rewrite (no no-store), and the query-varying rewrite (no-store + preserved source router state). I traced asPath (/mw-rewrite-static-gsp-query?variant=one via renderAsPath ?? renderRouteUrl, pages-page-handler.ts:589-592, :476) and query ({variant, from} via nextDataQuery) — both match the assertions.

Note (carried over, non-blocking): per-query ISR entries persist in prod, not dev

For a query-varying ISR rewrite (bypassCdnCache: true, revalidate > 0) prod still writes one ISR entry per unique sorted query string. The write in pages-page-response.ts:621-657 gates only on !scriptNonce && isrRevalidateSeconds !== null — it does not check bypassCdnCache — so it persists pages:/target?a=1&z=2 under the isolated key even though the CDN response is correctly no-store. pages-page-response.test.ts:201 ("stores query-varying rewrite ISR output but keeps the CDN response private") codifies this as intentional. Dev writes nothing here, so it's a dev/prod split, and under high-cardinality / adversarial query traffic with the default in-memory cache handler it is an unbounded-growth vector. The two new fixtures don't trigger it (static-gsp has no revalidate), so it's latent, not currently exercised end-to-end. Recommend confirming against Next.js whether query-varying SSG rewrites persist at all (vs. treated as fully dynamic with no ISR write) and either aligning dev/prod or documenting the intent in the PR description.

Note (new, low-severity): finalizeMissingStaticAssetResponse can forward a stale Content-Length

worker-utils.ts:39-49 now copies all headers from the original (rewritten) 404 response and only overrides Content-Type + body:

const notFound = notFoundStaticAssetResponse();
const headers = new Headers(response.headers);   // copies everything, incl. Content-Length
headers.set("Content-Type", notFound.headers.get("Content-Type")!);
return new Response(notFound.body, { status: 404, headers });

This is a deliberate change to preserve middleware headers (the deploy test asserts x-from-middleware survives), and it also newly affects the worker path (pages-router-entry.ts:227, app-router-entry.ts:185), which previously returned a clean notFoundStaticAssetResponse(). If the upstream rewritten 404 ever carried a Content-Length (a buffered, non-streamed render), it would now be forwarded alongside the shorter "Not Found" body — an HTTP framing mismatch. On Node, sendWebResponse only strips content-length when compressing (prod-server.ts:943), so a client without Accept-Encoding would see the stale value; Cloudflare recomputes it, so the worker path is safe. Most rewrite-to-404 renders stream (no Content-Length), so this is latent rather than active — but it'd be cheap to defensively headers.delete("content-length") (and content-encoding) in finalizeMissingStaticAssetResponse since the body is being replaced.

Confirmed correct

• i18n trailing-slash preservation (pages-i18n.ts:121): switching from "/" + parts.slice(1).join("/") to pathname.slice(locale.length + 1) || "/" correctly preserves a configured trailing slash (/fr/about/ → /about/) and handles bare-locale (/fr → /). Covered by the new features.test.ts case.
• NextRequest.url file-shape guard (shims/server.ts): /\.[^/]+$/.test(url.pathname) keeps manifest.json from becoming manifest.json/ under trailingSlash: true while nextUrl still reflects the configured policy. Covered by the new shims.test.ts case.
• Missing-chunk rewrite → re-probe → 404 flow (pages-request-pipeline.ts middlewareRewrite phase, prod-server.ts:2014) is consistent across worker and Node and matches the ported Next.js middleware-general _app-non-existent.js → /about behavior (pages-router.test.ts prod case passes).
• Post-rewrite query is preserved into __NEXT_DATA__.query (pages-page-response.ts:285, pages-router.test.ts:2304).

Summary

• No blocking issues. The dev/prod cache-control parity concern from the prior chain is resolved (prod threads the full cache-state; dev's uniform no-cache for GSP is inherent Next.js dev behavior, not a divergence). Targeted suites and checks pass.
• Non-blocking: (1) query-varying ISR rewrites persist a per-query entry in prod but not dev — confirm Next.js parity / document intent; (2) finalizeMissingStaticAssetResponse should defensively drop Content-Length/Content-Encoding when it swaps in the "Not Found" body.

[ask-bonk]

Review posted to PR #2216.

Summary

HEAD is d3daec8 — the branch was reworked since the previously-approved 09e99fb (new commits aeb0ba4, 758ce89, d3daec8). I re-verified every changed path and ran all targeted suites plus vp check — everything is green.

No blocking issues. The dev/prod cache-control parity concern that ran through the prior review chain is resolved, and in a cleaner way than the earlier 09e99fb approach:

• Prod threads the full getPagesMiddlewareRewriteCacheState result ({ cachePathname, bypassCdnCache }) through the ISR key, _next/data, HTML, and cached HIT/STALE headers. bypassCdnCache is true only for query-varying rewrites.
• Dev doesn't need to replicate the machinery: it never persists ISR and always emits no-cache, must-revalidate for GSP pages (direct hits and rewrites alike), which is inherent Next.js dev behavior — not a divergence introduced by this PR. middlewareRewriteCacheState is used in dev only to keep the request query in the render + __NEXT_DATA__.
• Prod E2E coverage now lives in tests/e2e/pages-router-prod/middleware-rewrite-cache.spec.ts (real build+start).

Two non-blocking notes:

1. Per-query ISR persistence — query-varying ISR rewrites still write one entry per sorted query string in prod (pages-page-response.ts:621-657, not gated on bypassCdnCache) but not in dev. Codified as intentional by the unit test; latent (no fixture triggers it) but an unbounded-growth vector. Confirm Next.js parity / document intent.
2. finalizeMissingStaticAssetResponse stale Content-Length — the helper now copies all upstream headers and only swaps Content-Type + body, so a buffered rewritten 404 carrying Content-Length could forward a mismatched value on Node without compression. Cheap fix: defensively delete content-length/content-encoding since the body is replaced.

Confirmed correct: i18n trailing-slash preservation, the NextRequest.url file-shape guard, the missing-chunk rewrite→re-probe→404 flow (worker/Node parity), and post-rewrite query threading into __NEXT_DATA__.

github run

[james-elicx]

Refreshed and revalidated at 1726eaa2:

• query-varying middleware rewrites remain private through recursive custom /404 renders
• origin ISR reads and writes are bypassed
• shared cache headers and cache tags are removed
• normal public/invariant caching remains intact
• focused tests: 473/473
• targeted vp check: clean
• independent sub-agent re-review: no findings

The exact Next.js middleware suite and fresh CI are now being run.

[james-elicx]

/bigbonk review for issues

[ask-bonk]

Re-review (HEAD 1726eaa) — both prior non-blocking notes now resolved; no blocking issues

A new commit landed since the last re-review: 1726eaa "fix(pages): bypass rewrite-specific origin caching". It directly addresses both carried-over non-blocking notes from the d3daec8 review — the per-query ISR persistence divergence and the finalizeMissingStaticAssetResponse stale Content-Length. I re-verified every changed path against HEAD and ran the targeted suites — all green.

Prior note #1 (per-query ISR write persists in prod, not dev): resolved

The fix introduces a bypassOriginCache flag alongside the existing bypassCdnCache, threaded through a small, well-scoped change:

• pages-middleware-rewrite-cache.ts:7 adds bypassOriginCache, set true for query-varying rewrites (same branch as bypassCdnCache). It also exports request-local no-op cache deps bypassedPagesIsrGet/bypassedPagesIsrSet (module-level stateless constants — safe to share across requests).
• pages-page-data.ts:1066-1067 swaps both isrGet and isrSet to the bypassed versions when bypassOriginCache is set, so a query-varying rewrite neither reads nor writes the shared origin ISR entry.
• pages-page-response.ts:624 applies the same swap to the HTML tee-and-write path (setCache: requestIsrSet), so even when the write branch (:625-661) runs, setCache is the no-op. This closes the exact gap the prior review flagged (pages-page-response.ts writing regardless of bypassCdnCache).
• Recursive /404 render: pages-page-handler.ts:907 propagates __bypassRequestCache: bypassOriginCache || bypassCdnCache into the forced-route /404 re-render, and :634-635 ORs it back in — so a source page's notFound under a query-varying rewrite keeps the recursive custom-404 render out of the shared cache too. Covered by the new pages-page-handler.test.ts case ("keeps query-varying rewrite notFound error renders out of shared caches" — asserts originGet/originSet never called and no cdn-cache-control/cache-tag).

Prod now matches dev (which already skipped both read and write): query-varying rewrites are fully private in both runtimes, so the unbounded-growth vector under high-cardinality query traffic is gone. Query-invariant rewrites still share the target's pages:/target key and emit normal s-maxage in both runtimes. New unit coverage is thorough: pages-page-data.test.ts adds cases for bypassed reads, query-invariant reuse (HIT), stale revalidation, and no-persist for render/fallback/redirect/notFound results.

Prior note #2 (finalizeMissingStaticAssetResponse stale Content-Length): resolved

worker-utils.ts:18-34 adds a REPLACED_BODY_HEADERS set (content-length, content-encoding, transfer-encoding, etag, content-range, etc.), and :64 deletes all of them before swapping in the "Not Found" body. This is exactly the defensive fix suggested previously — a buffered rewritten 404 carrying Content-Length can no longer forward a mismatched value. prod-server.ts:2014-2019 now routes the Node missing-chunk 404 through the same shared helper (instead of an ad-hoc res.end("Not Found")), so worker and Node produce byte-identical framing and both preserve middleware headers (x-from-middleware) while stripping body headers. The new deploy.test.ts cases cover this.

Verification

• Targeted suites: pages-page-data + pages-page-handler + pages-page-response — 172 passed; deploy + pages-router + features + shims + pages-request-pipeline — 2366 passed.
• tsc --noEmit on the vinext package: clean.

Confirmed correct (unchanged from prior reviews)

• Dev/prod cache-control parity holds: dev's uniform no-cache, must-revalidate for GSP pages (dev-server.ts:140) is inherent Next.js dev behavior (dev never persists ISR, re-runs getStaticProps per request), not a divergence this PR introduces. middlewareRewriteCacheState is used in dev only to keep the request query in the render + __NEXT_DATA__ (dev-server.ts:870, :1594).
• initialStaticAssetMiss basePath handling is consistent across worker (deploy.ts) and Node (prod-server.ts); the middlewareRewrite filesystem phase + re-probe-then-404 flow (pages-request-pipeline.ts:529-534) matches the ported Next.js middleware-general missing-chunk behavior.
• i18n trailing-slash preservation (pages-i18n.ts) and the NextRequest.url file-shape guard (shims/server.ts) remain correct and covered.

Summary

• No blocking issues.
• Both non-blocking notes from the d3daec8 review are resolved by 1726eaa: query-varying rewrites are now fully private in prod (read + write bypassed, incl. the recursive /404 render), matching dev; and finalizeMissingStaticAssetResponse strips stale body headers with worker/Node parity.
• The change is well-scoped and thoroughly unit-tested. LGTM.

[ask-bonk]

Review posted to PR #2216.

Summary

HEAD is now 1726eaa ("fix(pages): bypass rewrite-specific origin caching") — a new commit since the last review at d3daec8. I verified it resolves both carried-over non-blocking notes from the prior review, with no new blocking issues.

Prior note #1 (per-query ISR persistence divergence): resolved. The commit adds a bypassOriginCache flag (set for query-varying rewrites) that swaps both isrGet and isrSet to no-op versions (pages-page-data.ts:1066-1067, pages-page-response.ts:624). Prod now neither reads nor writes the shared origin ISR entry for query-varying rewrites, matching dev — closing the unbounded-growth vector. Critically, it also propagates __bypassRequestCache into the recursive custom-/404 render (pages-page-handler.ts:907), so a source notFound under a query-varying rewrite stays private too. Thoroughly unit-tested.

Prior note #2 (finalizeMissingStaticAssetResponse stale Content-Length): resolved. worker-utils.ts now deletes all body-related headers (REPLACED_BODY_HEADERS) before swapping in the "Not Found" body, and prod-server.ts routes the Node missing-chunk 404 through the same shared helper — worker/Node byte-identical framing, middleware headers preserved.

Verification: targeted suites 172 + 2366 passed; tsc --noEmit clean.

The change is well-scoped, matches Next.js behavior, and has good test coverage. No blocking issues — LGTM.

github run

[james-elicx]

Exact-head targeted Next.js v16.2.6 proof for the base middleware and data-state layer at 1726eaa2eeae05bc7504b3483d49189cc2c9a169:

• middleware-general/test/index.test.ts: 64/66; only the two shallow-link variants fail
• middleware-general/test/node-runtime.test.ts: 66/68; only the same two shallow-link variants fail
• middleware-trailing-slash/test/index.test.ts: 22/23; only shallow linking fails
• middleware-rewrites/test/index.test.ts: 55 passed, 1 failed, 2 intentional manifest skips; only the recursive client rewrite row fails

All PR #2216-owned rewrite-query propagation, data-request normalization, non-data request shape, missing-asset, and cache-isolation assertions pass on this live head. The remaining client-navigation rows are intentionally implemented by the stacked PR #2454, whose exact composed head passes the full matrix: 66/66, 68/68, 23/23, and 56 passed with 2 intentional skips.

Focused vinext tests remain 473/473; independent review, Bonk, and official CI are clean.