fix(middleware): clear nextUrl.basePath for absolute paths outside basePath (part of #1830)

[james-elicx]

Part of #1830.

Failing test: test/e2e/middleware-base-path/test/index.test.ts — "Middleware base tests > should execute from absolute paths".

Mirrors Next.js getNextPathnameInfo: NextURL only treats basePath as active when the pathname actually starts with the configured prefix, so middleware receiving a request without the basePath prefix sees nextUrl.basePath === "" and can redirect it into the basePath. Strips basePath before matcher evaluation and from rewrite targets; passes the original (un-stripped) URL to middleware from both prod-server and deploy adapters. Includes tests/shims.test.ts coverage.

Recovered from a cut-off worker; CI + bonk to verify.

[pkg-pr-new]

Open in StackBlitz

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

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

commit: bed17e1

[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

[james-elicx]

/bigbonk review for issues

[ask-bonk]

/bigbonk review — fix(middleware): clear nextUrl.basePath for absolute paths outside basePath

The core idea is correct and matches Next.js: getNextPathnameInfo only sets basePath when pathHasPrefix(pathname, basePath) (get-next-pathname-info.ts), and NextURL.analyze() does this.basePath = info.basePath ?? '' (next-url.ts). So clearing _basePath to "" when the pathname is outside basePath is the right semantics.

But the change has two blocking problems. The premise note ("Recovered from a cut-off worker; CI + bonk to verify") is accurate — CI is red and there's an App Router parity gap.

---

🔴 1. Breaks 10 existing unit tests in tests/shims.test.ts (CI red)

The PR updated the tests at lines 8178–8228 but left other tests that encode the old behavior untouched. Running vp test run tests/shims.test.ts -t "basePath" gives 10 failed / 39 passed:

• L8315 href includes basePath prefix — expects http://localhost/app/dashboard, gets /dashboard
• L8324 href includes both basePath and locale prefix
• L8372 basePath setter to empty string clears basePath — expects /app, gets ""
• L8440 searchParams mutations ... with basePath and locale
• L8453 clone() preserves locale, basePath, and config
• L8469 NextRequest passes basePath and i18n config through to nextUrl — new NextRequest("http://localhost/fr/dashboard", { nextConfig: { basePath: "/app", i18n } }) now yields nextUrl.basePath === "" instead of "/app"
• L8487 / L8500 / L8542 NextRequest.url ...
• L8615 applies trailingSlash with basePath

These tests all construct a NextURL/NextRequest with a basePath-stripped URL plus basePath: "/app" and expect basePath === "/app". They must be updated to use prefixed input URLs (e.g. http://localhost/app/fr/dashboard) to match the new (correct) semantics — exactly what the PR already did for the few tests it touched.

🔴 2. App Router regression — nextUrl.basePath is now wrong for in-basePath requests

This is the more serious issue. In Next.js, the middleware adapter builds the NextRequest from the original, un-stripped URL (adapter.ts: new NextURL(params.request.url, ...) where params.request.url still carries the basePath). The PR correctly compensates for this on the Pages side by re-adding the prefix in the runMiddleware closures (prod-server.ts:1676, deploy.ts:655).

But App Router was not given the same treatment. applyAppMiddleware passes normalizedPathname: cleanPathname (already basePath-stripped) into executeMiddleware (app-middleware.ts:243). createNextRequest then rebuilds mwUrl.pathname = cleanPathname (stripped) and constructs the NextRequest with nextConfig.basePath: "/app" (middleware-runtime.ts:201-218). With the new _stripBasePath, hasBasePath("/dashboard", "/app") is false, so:

• Before: App Router middleware saw request.nextUrl.basePath === "/app" for /app/dashboard
• After: it sees request.nextUrl.basePath === ""

This diverges from Next.js for the normal in-basePath case. It's exactly the failing L8469 test in miniature (stripped URL + basePath config). The fix needs to ensure App Router feeds the un-stripped pathname into createNextRequest (mirroring the Pages closures), or pass the basePath through so the NextRequest URL carries the prefix.

Note: there's no e2e test covering App Router middleware + basePath, so without a new test this regression would ship silently. Please add one (port from Next.js middleware-base-path).

---

🟡 Minor

• prod-server.ts:1682 / deploy.ts:661 — the re-add logic mwUrl.pathname = basePath + (mwUrl.pathname === "/" ? "" : mwUrl.pathname) duplicates addBasePathToPathname from utils/base-path.ts. Prefer reusing that helper for consistency and to avoid a second copy of the root-path edge case.
• middleware-runtime.ts:241-243 — matchPathname re-strips normalizedPathname. For App Router (cleanPathname already stripped) this is a no-op unless a real route legitimately starts with the basePath string after stripping; acceptable but worth a comment that it relies on cleanPathname already being canonical.

---

Verification

vp test run tests/shims.test.ts -t "basePath"
# Test Files  1 failed (1)
#       Tests  10 failed | 39 passed | 993 skipped

Verdict: correct direction, but not mergeable as-is — fix the 10 broken unit tests (use prefixed URLs) and close the App Router nextUrl.basePath gap with a corresponding test.

[ask-bonk]

Review posted to PR #1872.

Summary

The PR's core change to NextURL._stripBasePath() is correct in principle — I verified against Next.js source that getNextPathnameInfo only sets basePath when the pathname actually starts with the prefix, and NextURL.analyze() does this.basePath = info.basePath ?? ''. So clearing _basePath to "" for out-of-basePath paths is the right semantics.

But I found two blocking issues:

1. 🔴 CI is red — 10 existing unit tests break. The PR updated only the tests at lines 8178–8228 but left 10 other tests/shims.test.ts tests that encode the old behavior (constructing NextURL/NextRequest with a stripped URL + basePath: "/app" and expecting basePath === "/app"). I confirmed with vp test run: 10 failed / 39 passed. These need prefixed input URLs.

2. 🔴 App Router regression. Next.js's middleware adapter feeds the un-stripped URL into NextRequest. The PR correctly compensates on the Pages side (re-adding the prefix in the runMiddleware closures), but App Router still passes the stripped cleanPathname into createNextRequest. The result: App Router middleware now sees nextUrl.basePath === "" for normal in-basePath requests that previously reported "/app" — a silent parity regression (no e2e test covers it).

Plus two minor notes (duplicated addBasePathToPathname logic, and a clarity comment on the matchPathname re-strip).

Verdict: right direction, not mergeable as-is.

github run

[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]

/bigbonk review — fix(middleware): clear nextUrl.basePath for absolute paths outside basePath

Re-reviewed at 98bc211. The two blockers from my previous review are now resolved, but the second commit introduced a new CI-red regression. Verdict: still not mergeable as-is, but much closer.

✅ Resolved since last review

1. tests/shims.test.ts basePath tests are green. The PR updated the remaining tests to use basePath-prefixed input URLs. Verified:

   vp test run tests/shims.test.ts -t "basePath"
   #  Tests  49 passed | 993 skipped
   

2. App Router nextUrl.basePath regression is fixed. createNextRequest now re-adds the basePath before constructing the NextRequest (middleware-runtime.ts:208-215):

   const mwPathname = basePath ? addBasePathToPathname(normalizedPathname, basePath) : normalizedPathname;

   So App Router (which passes the stripped cleanPathname) now feeds an un-stripped pathname into NextRequest, and NextURL strips it back → nextUrl.basePath === "/app" for in-basePath requests. This also resolves my earlier minor note about duplicated re-add logic — both Pages closures and App Router now route through addBasePathToPathname. The double-add in the Pages flow (closure re-adds, then createNextRequest re-adds) is safe because addBasePathToPathname is idempotent (base-path.ts:32 short-circuits when hasBasePath is true).

---

🔴 1. New regression — 9 tests fail in tests/deploy.test.ts (CI red)

The deploy.ts change wrapped runMiddleware in a closure and added addBasePathToPathname to the import, but the generated-entry assertions in tests/deploy.test.ts were not updated. Verified:

vp test run tests/deploy.test.ts
#  Tests  9 failed | 236 passed (245)

Failing tests:

• handles middleware access control responses
• handles middleware redirects
• handles middleware rewrites
• preserves responseHeaders on middleware redirect
• proxies external middleware rewrites before local route handling
• runs middleware before routing
• preserves x-middleware-request-* headers for prod request override handling
• builds reqCtx before middleware runs
• handles basePath stripping and creates a new request with stripped URL for middleware

Two assertion shapes broke:

• 8 sites at tests/deploy.test.ts:757,775,785,795,807,817,1126,1163 assert the literal 'runMiddleware: typeof runMiddleware === "function" ? runMiddleware : null', which no longer appears (it's now a closure).
• 1 site at tests/deploy.test.ts:853 asserts 'import { hasBasePath, stripBasePath } from "vinext/utils/base-path"', but the import is now { addBasePathToPathname, hasBasePath, stripBasePath }.

These assertions need to be updated to match the new generated output (the closure body and the wider import).

🔴 2. No test covers the App Router nextUrl.basePath fix

The App Router regression fix (item ✅2 above) is the core behavioral change of this commit, but nothing tests it. tests/app-router-middleware-next-request.test.ts and tests/app-post-middleware-context.test.ts have zero basePath coverage, and there's no e2e middleware-base-path fixture. Without a test, this fix can silently regress again. Please add a unit test asserting that App Router middleware sees request.nextUrl.basePath === "/app" for an in-basePath request and === "" for an absolute (out-of-basePath) path — ideally ported from Next.js test/e2e/middleware-base-path.

---

🟢 Verified correct

• NextURL._stripBasePath() clearing _basePath to "" when hasBasePath is false matches Next.js getNextPathnameInfo (basePath only set when the pathname starts with the prefix).
• fetchEvent page change from normalizedPathname → matchPathname (middleware-runtime.ts:273) is correct: NextFetchEvent.sourcePage should be the basePath-stripped matched page.
• The matcher re-strip (matchPathname, middleware-runtime.ts:251-253) correctly handles both flows: App Router's already-stripped cleanPathname (no-op) and the Pages flow's prefixed URL (strips before matching).
• The rewrite-target strip (middleware-runtime.ts:399-409) is safe and idempotent for both flows.
• vp check passes on all four changed source files.

---

Verdict: Direction is correct and both prior blockers are fixed. To merge: (1) update the 9 tests/deploy.test.ts assertions to match the new generated worker entry, and (2) add a test covering the App Router nextUrl.basePath behavior.

[ask-bonk]

Review posted to PR #1872.

Summary

Re-reviewed at commit 98bc211. The two blockers from the previous review are now fixed, but the second commit introduced a new CI-red regression.

✅ Resolved:

1. The 10 broken tests/shims.test.ts basePath tests now pass (49 passing) — input URLs were prefixed.
2. App Router nextUrl.basePath regression fixed — createNextRequest now re-adds basePath via addBasePathToPathname before constructing NextRequest, so in-basePath App Router requests correctly report basePath === "/app" again. The double-add in the Pages flow is safe (the helper is idempotent).

🔴 Two remaining blockers:

1. 9 tests fail in tests/deploy.test.ts — the deploy.ts change wrapped runMiddleware in a closure and widened the base-path import, but the generated-entry assertions (8 sites asserting the old runMiddleware: typeof runMiddleware === "function" ? runMiddleware : null literal + 1 import assertion at line 853) were not updated. Verified 9 failed | 236 passed.
2. No test covers the App Router fix — the core behavioral change of this commit has zero coverage; needs a unit test (ideally ported from Next.js middleware-base-path).

Everything else verified correct (_stripBasePath semantics vs Next.js, fetchEvent.page → matchPathname, matcher re-strip, rewrite-target strip, and vp check clean on all source files).

github run

[james-elicx]

/bigbonk review for issues

[ask-bonk]

/bigbonk review — fix(middleware): clear nextUrl.basePath for absolute paths outside basePath

Re-reviewed at be242b4. The new commit (test(middleware): fix stale test assertions broken by basePath wrapper change) fixed 8 of the 9 deploy-test failures from the last review, and the App Router regression remains correctly fixed. But CI is still red — one assertion was missed.

🔴 1. One remaining CI-red failure in tests/deploy.test.ts

The previous commit updated the 8 runMiddleware literal assertions, but missed the import assertion at tests/deploy.test.ts:845. The deploy.ts import was widened to include addBasePathToPathname, so the generated worker entry now emits:

import { addBasePathToPathname, hasBasePath, stripBasePath } from "vinext/utils/base-path";

but the test still asserts the old two-symbol import:

expect(content).toContain(
  'import { hasBasePath, stripBasePath } from "vinext/utils/base-path"',
);

Verified locally:

vp test run tests/deploy.test.ts
#  Tests  1 failed | 244 passed (245)
#  ❯ tests/deploy.test.ts:844:21
#    "handles basePath stripping and creates a new request with stripped URL for middleware"

Fix: update line 845 to assert 'import { addBasePathToPathname, hasBasePath, stripBasePath } from "vinext/utils/base-path"' (or split into separate toContain("addBasePathToPathname") / toContain("stripBasePath") checks).

🔴 2. Still no test covers the App Router nextUrl.basePath fix

The core behavioral change of this PR — createNextRequest re-adding the basePath via addBasePathToPathname(normalizedPathname, basePath) (middleware-runtime.ts:208-210) so App Router middleware sees nextUrl.basePath === "/app" for in-basePath requests — still has zero coverage. tests/middleware-runtime.test.ts has no basePath assertions, and there's no executeMiddleware test exercising the stripped-normalizedPathname + basePath path.

tests/app-route-handler-runtime.test.ts was updated, but it only covers the NextURL out-of-basePath (stripped) case, not the App Router middleware in-basePath re-add. Please add a unit test asserting that executeMiddleware/createNextRequest with normalizedPathname: "/dashboard" + basePath: "/app" yields a NextRequest whose nextUrl.basePath === "/app" (ideally ported from Next.js test/e2e/middleware-base-path). Without it, the App Router fix can silently regress.

---

🟢 Verified correct

• NextURL._stripBasePath() (shims/server.ts:340-347) clearing _basePath to "" when hasBasePath is false matches Next.js getNextPathnameInfo semantics. ✓
• App Router fix is sound. app-rsc-handler.ts:413-419 documents that App Router only reaches middleware when the request is in-basePath (already stripped) or basePath is empty — hadBasePath is hardcoded true. So createNextRequest unconditionally re-adding the prefix is correct for every case App Router currently supports (basePath: false / absolute-path requests are an unrelated known follow-up to #1333). ✓
• Pages flow parity. Both prod-server.ts:1670-1687 and deploy.ts:655-666 wrap runMiddleware in a closure that re-adds the prefix only when hadBasePath && basePath, correctly leaving out-of-basePath absolute requests stripped so middleware sees nextUrl.basePath === "". ✓
• Idempotent double-add. addBasePathToPathname short-circuits when hasBasePath is already true (base-path.ts:32), so the Pages closure re-add + createNextRequest re-add is safe. ✓
• Matcher re-strip (middleware-runtime.ts:251-253) correctly handles both flows. ✓
• vp check clean on all 4 changed source files. ✓
• tests/shims.test.ts -t "basePath" → 49 passed. ✓

---

Verdict: Direction is correct and the logic is sound, but not mergeable as-is — CI is red. To merge: (1) fix the missed import assertion at tests/deploy.test.ts:845, and (2) add a test for the App Router nextUrl.basePath re-add behavior.

[ask-bonk]

Review posted to PR #1872.

Summary

Re-reviewed at commit be242b4. The new commit fixed 8 of the 9 deploy-test failures, but CI is still red — one assertion was missed.

🔴 Two blockers

1. One remaining CI-red failure in tests/deploy.test.ts:845. The previous commit updated the 8 runMiddleware literal assertions but missed the import assertion. The deploy.ts import widened to include addBasePathToPathname, so the generated entry now emits import { addBasePathToPathname, hasBasePath, stripBasePath }, but the test still asserts the old two-symbol import. Verified locally: 1 failed | 244 passed.

2. No test covers the App Router nextUrl.basePath fix. The core behavioral change (createNextRequest re-adding basePath) has zero coverage — needs a unit test, ideally ported from Next.js middleware-base-path.

🟢 Verified correct

• NextURL._stripBasePath() matches Next.js getNextPathnameInfo semantics.
• The App Router fix is sound — app-rsc-handler.ts:413-419 documents App Router only reaches middleware in-basePath (hadBasePath hardcoded true), so the unconditional re-add is correct for all supported cases.
• Pages flow parity (prod-server.ts, deploy.ts) correctly gates the re-add on hadBasePath && basePath.
• addBasePathToPathname is idempotent, so the double-add in the Pages flow is safe.
• vp check clean on all 4 changed source files; shims basePath tests pass (49).

Verdict: Logic is correct, but not mergeable as-is — fix the missed import assertion and add App Router coverage.

github run

[james-elicx]

Review addressed (0f10ff1)

Both blockers from the last bonk review, plus two regressions found in self-review:

1. Fixed the missed import assertion at tests/deploy.test.ts:845 — now asserts the widened import { addBasePathToPathname, hasBasePath, stripBasePath } emitted by the generated worker entry. vp test run tests/deploy.test.ts → 245 passed.
2. Added unit coverage for the App Router nextUrl.basePath re-add — new middleware nextUrl basePath suite in tests/middleware-runtime.test.ts covering: the App Router stripped-pathname re-add (normalizedPathname: "/dashboard" + basePath: "/app" → nextUrl.basePath === "/app", prefixed req.url), the Pages in-basePath flow, the out-of-basePath absolute-path case (App Router: misc correctness stragglers (root-layout redirect, global-not-found default, esm .js, react@experimental, middleware-base-path) #1830 — nextUrl.basePath === ""), and matcher evaluation against the stripped path.
3. Self-review: the unconditional re-add in createNextRequest regressed the Pages out-of-basePath flow — the very case this PR fixes. The adapters deliberately pass a bare URL for absolute-path requests, but createNextRequest re-prefixed it via addBasePathToPathname, so middleware saw nextUrl.basePath === "/root" again. Now gated on a new hadBasePath option: derived from the request URL by default (correct for the prod/deploy Pages adapters, which pass the original URL), and asserted true by callers that pass pre-stripped URLs (App Router applyAppMiddleware, dev-server runMiddleware).
4. Self-review: same regression class in App Route handlers — createTrackedAppRouteRequest receives basePath-stripped URLs, so request.url/nextUrl.href lost the prefix and nextUrl.basePath reported "" (the earlier test edit had codified this). Route handlers only run for in-basePath requests, so the prefix is now re-added before constructing the NextRequest, restoring Next.js parity; the test asserts the prefixed URL + active basePath again.
5. Self-review: NextURL href reassignment parity — _stripBasePath now re-derives the active basePath from the configured value on every parse (new _configBasePath), so setting href across the basePath boundary toggles basePath like Next.js NextURL.analyze() instead of clearing it permanently. Covered by a new shims test.

Verified locally: vp test run on tests/middleware-runtime.test.ts, tests/shims.test.ts, tests/deploy.test.ts, tests/app-route-handler-runtime.test.ts, tests/middleware-runtime-trailing-slash.test.ts, tests/app-router-middleware-next-request.test.ts, tests/app-post-middleware-context.test.ts, tests/pages-api-with-middleware.test.ts — all green; vp check clean.

The webkit E2E failure on be242b4 (hash-rsc-requests.browser.spec.ts module-load SyntaxError) touches no file in this diff, passed on both prior commits of this branch, and main is green — treating as flake pending this run.

[james-elicx]

/bigbonk review for issues

[james-elicx]

/bigbonk review for issues

[james-elicx]

/bigbonk review for issues

[james-elicx]

/bigbonk review for issues

[james-elicx]

/bigbonk review for issues

[ask-bonk]

/bigbonk review — fix(middleware): clear nextUrl.basePath for absolute paths outside basePath

Re-reviewed at 0f10ff1. Both prior blockers are now fixed, CI is green, and the requested coverage exists. This is mergeable.

✅ Resolved since last review

1. The missed import assertion is fixed. tests/deploy.test.ts:843 now asserts the widened import { addBasePathToPathname, hasBasePath, stripBasePath } from "vinext/utils/base-path", matching the generated worker entry. Verified vp test run tests/deploy.test.ts -t "basePath" → 3 passed / 242 skipped, and the full file → 245 passed.

2. The App Router nextUrl.basePath behavior is now covered. A new middleware nextUrl basePath suite in tests/middleware-runtime.test.ts exercises executeMiddleware/createNextRequest directly:

   • App Router stripped-pathname re-add (normalizedPathname: "/dashboard" + basePath: "/app", hadBasePath: true) → nextUrl.basePath === "/app", nextUrl.pathname === "/dashboard", and req.url carries /app/dashboard.
   • Pages in-basePath flow (prefixed URL, auto-derived hadBasePath) → basePath === "/root".
   • Out-of-basePath absolute path (#1830) → basePath === "", prefix not re-added.
   • Matcher evaluation against the stripped pathname.
     tests/app-route-handler-runtime.test.ts additionally asserts the route-handler URL parity (nextUrl.basePath === "/base", prefixed request.url).

🟢 Verified correct

• NextURL._stripBasePath() href-reassignment parity (shims/server.ts:339-357). The new _configBasePath field is the source of truth re-evaluated on every parse: active _basePath is set to "" when the pathname is outside the prefix and re-activated when a later href assignment moves back inside. This matches Next.js getNextPathnameInfo being re-run by NextURL.analyze() on every parse. Covered by a new shims test.
• hadBasePath gating is consistent across all four request paths:
  • prod-server.ts:1601,1676-1687 and deploy.ts:608,655-666 derive hadBasePath from the original (un-stripped) URL and gate the re-add on hadBasePath && basePath — out-of-basePath requests correctly stay bare so middleware sees basePath === "".
  • middleware.ts:159 (dev server) and app-middleware.ts:242 (App Router) pass pre-stripped URLs and correctly assert hadBasePath: true. The App Router invariant is documented at app-rsc-handler.ts:413-419 (only in-basePath requests reach middleware; basePath: false is a known #1333 follow-up).
• Auto-derivation handles basePath-before-locale ordering — hasBasePath("/root/fr/dashboard", "/root") is true since basePath precedes the locale segment.
• addBasePathToPathname is idempotent (base-path.ts:32 short-circuits on hasBasePath), so the Pages double-add (closure re-add → createNextRequest re-add) is safe.
• Matcher re-strip (middleware-runtime.ts:267-277) is a no-op for App Router's already-stripped cleanPathname and strips correctly for the Pages prefixed-URL flow; the documented invariant (cleanPathname is canonical) holds because App Router always strips before passing.
• Rewrite-target strip (middleware-runtime.ts:430-440) normalizes the basePath out of NextURL.href-encoded rewrite targets before downstream routing.
• vp check clean on all 7 changed source files.
• Related suites green: tests/shims.test.ts (1043), tests/middleware-runtime.test.ts, tests/middleware-runtime-trailing-slash.test.ts, tests/app-router-middleware-next-request.test.ts, tests/app-post-middleware-context.test.ts, tests/pages-api-with-middleware.test.ts, tests/app-route-handler-runtime.test.ts — 1349+ passing across the relevant files.

🟡 Minor (non-blocking)

• The runMiddleware re-add closure is duplicated verbatim in prod-server.ts:1678-1686 and deploy.ts:657-665. Consider extracting a small wrapMiddlewareWithBasePath(runMiddleware, basePath, hadBasePath) helper in server/* to keep the two adapters in sync (matches the AGENTS.md "keep dev/prod parity" + "thin generated entries" guidance). Not required for this PR.

---

Verdict: Logic matches Next.js getNextPathnameInfo semantics, all four request paths are consistent, the previously-missing coverage is in place, and CI is green. Approve / mergeable.

[ask-bonk]

Review posted to PR #1872.

Summary

Re-reviewed at commit 0f10ff1. Both prior blockers are now fixed, CI is green, and the PR is mergeable.

Resolved since last review

1. Missed import assertion fixed — tests/deploy.test.ts:843 now asserts the widened three-symbol base-path import. Verified tests/deploy.test.ts → 245 passed.
2. App Router nextUrl.basePath coverage added — new middleware nextUrl basePath suite in tests/middleware-runtime.test.ts directly exercises the App Router re-add, Pages in-basePath flow, the App Router: misc correctness stragglers (root-layout redirect, global-not-found default, esm .js, react@experimental, middleware-base-path) #1830 out-of-basePath case (basePath === ""), and matcher evaluation against the stripped path.

Verified correct

• NextURL._stripBasePath() href-reassignment parity via the new _configBasePath field matches Next.js getNextPathnameInfo re-run semantics.
• hadBasePath gating consistent across all 4 request paths (prod-server, deploy, dev-server middleware.ts, App Router app-middleware.ts); auto-derivation handles basePath-before-locale ordering.
• addBasePathToPathname idempotency makes the Pages double-add safe; matcher re-strip and rewrite-target strip both correct.
• vp check clean on all 7 changed source files; 1349+ tests passing across the relevant suites.

Minor (non-blocking)

• The runMiddleware re-add closure is duplicated verbatim in prod-server.ts and deploy.ts — could be extracted to a shared server/* helper for dev/prod parity, but not required for this PR.

Verdict: Approve / mergeable.

github run

[james-elicx]

Addressed the remaining nit from the last review: extracted the duplicated runMiddleware basePath re-add closure from prod-server.ts and the generated worker entry in deploy.ts into a shared wrapMiddlewareWithBasePath helper in server/pages-request-pipeline.ts (both adapters already import from that module, so the generated entry needs no new import source). Updated the tests/deploy.test.ts assertions to match the generated entry. vp test run tests/deploy.test.ts tests/middleware-runtime.test.ts tests/pages-request-pipeline.test.ts → 291 passed; vp check clean. (fd71a74)

[james-elicx]

/bigbonk review for issues

[ask-bonk]

/bigbonk review — fix(middleware): clear nextUrl.basePath for absolute paths outside basePath

Re-reviewed at fd71a74. This commit addresses the one remaining non-blocking nit from my last review (extract the duplicated runMiddleware basePath re-add closure). The refactor is behavior-preserving, CI is green, and the PR is mergeable.

✅ Resolved since last review

Duplicated closure extracted into a shared helper. wrapMiddlewareWithBasePath(runMiddleware, basePath, hadBasePath) now lives in server/pages-request-pipeline.ts:137-148 and both adapters delegate to it:

• prod-server.ts:1676
• deploy.ts:658 (generated worker entry; the entry already imports from vinext/server/pages-request-pipeline, so no new import source was needed).

This matches the AGENTS.md "keep dev/prod parity" + "thin generated entries" guidance — the URL-reconstruction logic moved out of a template string into a typed server/* module.

🟢 Verified correct

• Behavior-preserving. The original gated per-call (if (hadBasePath && basePath) inside the closure); the helper gates once up-front (if (!hadBasePath || !basePath) return runMiddleware). Since hadBasePath/basePath are per-request constants, deciding once vs. per-call is equivalent. The pass-through (returning the original runMiddleware unchanged) keeps out-of-basePath absolute requests bare so middleware sees nextUrl.basePath === "" (#1830), and the in-basePath branch re-adds the prefix via the idempotent addBasePathToPathname.
• Type signature is faithful — NonNullable<PagesPipelineDeps["runMiddleware"]> for both the param and return matches the (request, ctx, opts) => Promise<MiddlewareResult> shape the pipeline calls at pages-request-pipeline.ts:274. The generated entry only passes runMiddleware when typeof runMiddleware === "function", so the NonNullable contract holds.
• Import cleanup is correct — addBasePathToPathname was removed from the direct imports in both prod-server.ts and deploy.ts (it now lives only inside the helper); no leftover references, and vp check reports no unused-import or type errors.
• Test assertions updated to match the new generated entry: tests/deploy.test.ts:760 asserts wrapMiddlewareWithBasePath(runMiddleware, basePath, hadBasePath) and :845 asserts the narrowed two-symbol base-path import. Verified tests/deploy.test.ts (245) + tests/pages-request-pipeline.test.ts (34) → 279 passed.
• vp check clean on all 3 changed source files.
• CI is fully green — Check, all Vitest shards, all E2E projects (including the previously-flaky app-router-webkit-browser-specific), and all deploys.

🟡 Minor (non-blocking)

• The extracted wrapMiddlewareWithBasePath helper has no direct unit test in tests/pages-request-pipeline.test.ts. Per AGENTS.md ("If you move behavior out of a template string, add a focused unit test for the new helper module"), a small test asserting the three branches — pass-through when !hadBasePath, pass-through when !basePath, and prefix re-add otherwise — would lock in the wrapper's gating contract independent of the generated-entry string assertion. The behavior is exercised indirectly via middleware-runtime.test.ts (createNextRequest/executeMiddleware), so this is not a blocker, just the natural follow-on for the extraction.

---

Verdict: The extraction is correct and behavior-preserving, all prior blockers remain fixed, coverage for the App Router nextUrl.basePath fix is in place, and CI is green. Approve / mergeable.

[ask-bonk]

Review posted to PR #1872.

Summary

Re-reviewed at the latest commit fd71a74, which addresses the one non-blocking nit from the prior review — extracting the duplicated runMiddleware basePath re-add closure into a shared wrapMiddlewareWithBasePath helper.

Verdict: Approve / mergeable.

Verified

• Behavior-preserving extraction — the helper (pages-request-pipeline.ts:137) gates once up-front (if (!hadBasePath || !basePath) return runMiddleware) which is equivalent to the original per-call gate since those values are per-request constants. Both prod-server.ts:1676 and the generated deploy.ts:658 entry now delegate to it.
• Type signature faithful to PagesPipelineDeps["runMiddleware"].
• Import cleanup correct — addBasePathToPathname removed from both files' direct imports with no leftovers; vp check clean on all 3 source files.
• Tests pass — deploy.test.ts (245) + pages-request-pipeline.test.ts (34) = 279 passed; assertions updated to match the new generated entry.
• CI fully green, including the previously-flaky webkit E2E.

Minor (non-blocking)

• The new wrapMiddlewareWithBasePath helper has no direct unit test (only indirect coverage + the generated-string assertion). Per AGENTS.md guidance on extracting behavior from template strings, a focused 3-branch test would be the natural follow-on — but not a blocker.

github run

[james-elicx]

Addressed the follow-on nit: added a focused wrapMiddlewareWithBasePath suite to tests/pages-request-pipeline.test.ts covering the gating contract directly — pass-through when hadBasePath === false, pass-through when basePath is empty, prefix re-add (preserving query/headers/ctx/opts), and idempotent re-add for an already-prefixed URL. vp test run tests/pages-request-pipeline.test.ts → 38 passed; vp check clean. (bed17e1)

[james-elicx]

/bigbonk review for issues

[james-elicx]

/bigbonk review for issues

[ask-bonk]

/bigbonk review — fix(middleware): clear nextUrl.basePath for absolute paths outside basePath

Re-reviewed at bed17e1. This commit adds the focused wrapMiddlewareWithBasePath unit suite that was the one remaining non-blocking follow-on from the prior review. All prior blockers stay fixed, the new helper now has direct coverage, and CI is green. Approve / mergeable.

✅ Resolved since last review

Direct unit coverage for wrapMiddlewareWithBasePath. tests/pages-request-pipeline.test.ts now exercises all three branches of the gating contract independent of the generated-entry string assertion:

• pass-through (same reference) when hadBasePath === false — and asserts the request URL stays bare (/dashboard) so middleware sees nextUrl.basePath === "" (#1830);
• pass-through when basePath is empty;
• prefix re-add when hadBasePath === true, asserting query/headers/ctx/opts are forwarded untouched (http://localhost/root/dashboard?q=1, x-test preserved, ctx/opts passed through);
• idempotent re-add for an already-prefixed URL.

Verified tests/pages-request-pipeline.test.ts → 38 passed.

🟢 Verified correct (re-confirmed against Next.js source)

I pulled the canary next-url.ts and confirmed the core change matches Next.js exactly:

• href reassignment parity. Next.js set href calls this.analyze(), which re-runs getNextPathnameInfo and recomputes this[Internal].basePath = info.basePath ?? '' from options.nextConfig.basePath on every parse. The PR's _configBasePath field + _stripBasePath() on every href set (shims/server.ts:413-417) mirrors this faithfully — basePath toggles "" ↔ /app as the pathname moves out of / back into the prefix. The new shims test (href reassignment re-derives basePath from the configured value) locks this in.
• set basePath parity. Next.js set basePath writes this[Internal].basePath directly without updating the config, so a later href reassignment discards a manual basePath set — the vinext setter behaves identically (manual sets are not persisted into _configBasePath). This is correct Next.js behavior, not a divergence.
• hadBasePath gating is consistent across all four request paths. prod-server.ts:1602 and deploy.ts:608 derive hadBasePath from the original (pre-strip) pathname and gate the re-add via wrapMiddlewareWithBasePath(runMiddleware, basePath, hadBasePath); middleware.ts:159 (dev) and app-middleware.ts:242 (App Router) pass pre-stripped URLs and assert hadBasePath: true. The App Router invariant is documented at app-rsc-handler.ts:413-419 (normalizeRscRequest 404s out-of-basePath before middleware; basePath: false is the tracked #1333 follow-up, not a regression).
• App Router route-handler URL parity. createTrackedAppRouteRequest re-adds the basePath before constructing the NextRequest (app-route-handler-runtime.ts:299-308); the new app-route-handler-runtime.test.ts assertions confirm nextUrl.basePath === "/base", nextUrl.pathname === "/demo", and the prefixed request.url — and crucially the locale segment survives (/base/fr/demo) because the routing layer strips only basePath, not the locale.
• Behavior-preserving extraction. wrapMiddlewareWithBasePath gates once up-front (if (!hadBasePath || !basePath) return runMiddleware), equivalent to the original per-call gate since both are per-request constants; the NonNullable<PagesPipelineDeps["runMiddleware"]> signature is faithful.
• addBasePathToPathname idempotency (base-path.ts:32) makes the Pages double-add (closure re-add → createNextRequest re-add) safe; matcher re-strip (middleware-runtime.ts:278-280) and rewrite-target strip (:437-440) both correct.

Verification

vp test run tests/deploy.test.ts tests/pages-request-pipeline.test.ts tests/middleware-runtime.test.ts   → 295 passed
vp test run tests/shims.test.ts tests/app-route-handler-runtime.test.ts                                   → 1056 passed
vp test run tests/app-router-middleware-next-request.test.ts tests/app-post-middleware-context.test.ts \
            tests/pages-api-with-middleware.test.ts tests/middleware-runtime-trailing-slash.test.ts        → 36 passed
vp check (8 changed source files)                                                                          → clean

Verdict: Logic matches Next.js getNextPathnameInfo/analyze() semantics, all four request paths are consistent, every behavioral change is now covered (including the helper's direct unit test), and CI is green. Approve / mergeable.

[ask-bonk]

Review posted to PR #1872.

Summary

Re-reviewed at the latest commit bed17e1, which adds the focused wrapMiddlewareWithBasePath unit suite — the one remaining non-blocking follow-on from the prior review. Approve / mergeable.

What I verified

• Core semantics match Next.js exactly. I pulled canary next-url.ts and confirmed: set href → analyze() re-runs getNextPathnameInfo and recomputes basePath = info.basePath ?? '' from the config on every parse. The PR's new _configBasePath field + _stripBasePath() on every href set mirrors this faithfully — basePath toggles "" ↔ /app as the pathname moves out of/back into the prefix.
• set basePath parity — Next.js writes the internal basePath directly without updating config, so a later href set discards a manual basePath set. vinext behaves identically (not a divergence).
• hadBasePath gating consistent across all four request paths (prod-server, deploy, dev-server, App Router); App Router's hadBasePath: true is correct because normalizeRscRequest 404s out-of-basePath requests before middleware (documented; basePath: false is the tracked basePath configuration not properly scoped #1333 follow-up).
• App Router route-handler URL parity — re-add preserves the locale segment (/base/fr/demo) since routing strips only basePath.
• New helper test covers all 3 gating branches + query/headers/ctx/opts forwarding + idempotency.

Verification

• tests/deploy.test.ts + pages-request-pipeline.test.ts + middleware-runtime.test.ts → 295 passed
• tests/shims.test.ts + app-route-handler-runtime.test.ts → 1056 passed
• 4 related middleware suites → 36 passed
• vp check clean on all 8 changed source files

No blocking or minor issues found this round. The PR is correct, fully covered, and CI is green.

github run