fix(router): stabilize multi-agent review findings (15 mediums + cleanups) - #571

Problem

A multi-agent review (9 subsystems × 6 lenses, every finding adversarially verified) surfaced 191 findings against @rangojs/router at HEAD. Nothing at HEAD corrupts data or crashes the router on the common path, but a cluster of latent corners degrade silently or surprise a consumer who follows the documented contract: a typed-and-documented cache("profileName") form that throws at build, a transition() type that rejects two of its own documented call shapes, cache() callbacks where ctx.env is unknown, a trie that quietly falls back to the regex matcher (and fires an alarming "please report this" warning) on perfectly valid config, an async redirect() under loading() that vanishes into the RSC stream, a build that bakes a corrupt prerender artifact when a Passthrough route also declares loading(), loader-cache failures that never reach onError, and — found in a follow-up gap pass — a HIGH cross-host loader-cache leak where one tenant's cached loader data could be served to another.

This PR stabilizes 15 of 16 verified medium defects plus the HIGH cross-host leak, each with a red→green test, and lands 90+ low/info cleanups (dead exports, dedup, stale docs) and a comment-noise reduction. The full per-finding disposition lives in the tracked follow-up issues linked at the bottom.

Userland improvements

Every consumer-facing change below is shown from the seat of someone writing route definitions, handlers, loaders, or a custom store.

1. cache("profileName") removed — was typed, documented, and build-breaking (M12)

The (profileName: string) overload was advertised in the RouteHelpers type, the helper JSDoc, and docs/design/caching.md, but the top-level non-lazy build-time manifest generator constructs its RangoContext store with no cacheProfiles key, so any cache("name") in the top-level route tree hit the invariant("unknown cache profile") at build. Per the pre-1.0 API-hygiene rule (no advertised-but-broken surface in main), the overload and its runtime string branch are removed.

// BEFORE — typed + documented, but threw "unknown cache profile" at build:
path("/p/:id", Product, () => [ cache("productProfile") ])

// AFTER — named profiles via the directive (works at build AND runtime):
"use cache: productProfile"

// …or pass explicit options inline (unchanged):
path("/p/:id", Product, () => [ cache({ ttl: 60 }) ])

The "use cache: <profile>" directive and the createRouter({ cacheProfiles }) option are untouched — only the route-tree cache("name") call form is gone.

2. transition() now compiles in all four runtime forms (M7)

The runtime (dsl-helpers.ts) has always handled four shapes, and the type's own JSDoc documents the no-arg form as a meaningful choice — but the RouteHelpers.transition type declared only two overloads, so two documented forms were a hard compile error.

transition()                               // BEFORE: TS2554 "Expected 1-2 arguments, but got 0". AFTER: ok
transition(() => [path("/", Home)])        // BEFORE: TS2559. AFTER: ok
transition({ enter: "fade" })              // ok (unchanged)
transition({ enter: "fade" }, () => [...]) // ok (unchanged)

Two overloads added to RouteHelpers.transition: (): TransitionItem and (children) => TransitionItem, mirroring the already-correct cache() overload shape. A type-pin (dsl-helper-types.check.ts) freezes the four-form contract; the pre-fix red (TS2554/TS2559) was captured.

3. cache() callbacks now receive a typed ctx.env (TEnv) (M14)

PathHelpers<TEnv> / RouteHelpers<T, TEnv> are parameterized over the app env, but the cache member declared bare PartialCacheOptions (which defaults TEnv to unknown), so condition/key/tags callbacks saw ctx.env: unknown — exactly on the pattern the JSDoc holds up as the motivating example.

// BEFORE: TS18046 "'ctx.env' is of type 'unknown'" — or you cast `ctx.env as AppEnv`.
// AFTER:  ctx.env is typed as TEnv across condition/key/tags.
urls<{ REGION: string }>(({ path, cache }) => [
  path("/p/:id", Product, () => [
    cache({ key: (ctx) => `${ctx.env.REGION}:product:${ctx.params.id}` }),
  ]),
])

Threaded PartialCacheOptions<TEnv> through all four cache overloads on both PathHelpers and RouteHelpers. A compile probe confirms the fix clears all three TS18046 errors.

4. Trie backtracks on a constraint miss — correct route + no false "trie gap" warning (M5)

walkTrie committed to the first structurally-matching branch and validated per-leaf constraints (leaf.cv) only after the walk, in validateAndBuild, which returned null on a miss. Because the constraint check lived outside the recursive frames, a constraint failure could not unwind to a lower-priority sibling that would match. The fix threads leafConstraintsPass() into every candidate terminal (terminal + wildcard-empty + wildcard-rest) so a miss backtracks during the walk; constraintsSatisfied() remains a backstop on final params.

// Route config:
path("/:locale(en|gb)/blog", Blog)
path("/*", CatchAll)

// Request: /de/blog
// BEFORE: trie returned null → fell through to the definition-order regex matcher,
//         and in dev fired:
//   "[rango] resolved via the regex fallback even though the route trie was
//    present … indicates a trie gap. Please report this"  (on perfectly valid config)
// AFTER:  trie backtracks to /* and binds * = "de/blog"; no warning.

Consumer impact: the false R3 warning (which fired once per distinct failing pathname — unbounded for a catch-all locale) stops manufacturing bug reports, and production no longer silently leans on the regex fallback's definition-order precedence (where an early catch-all can shadow a specific route). Red→green tests for the param+wildcard and suffix-param+param sibling cases; 168 pattern/host regression tests pass.

5. notFoundBoundary on an orphan-layout sibling now resolves — parity with errorBoundary (M8)

findNearestErrorBoundary walked the parent chain and inspected each level's orphan-layout siblings; findNearestNotFoundBoundary only walked current.parent. Since an orphan layout gets parent = null after attachOrphanSibling, an orphan-hosted boundary was reachable only via the orphan scan — present for errors, missing for notFound.

// A routeless layout() wrapping a route group, carrying both boundaries:
layout(Shell, () => [
  errorBoundary(<Err />),       // already caught a sibling route's thrown error
  notFoundBoundary(<NotFound />), // BEFORE: silently ignored — sibling notFound() fell
                                  //         through to the router's default 404
                                  // AFTER:  resolves symmetrically
  path("/items/:id", Item),
])

Added the same orphan-sibling scan to findNearestNotFoundBoundary (error-handling.ts). Red→green unit test with an errorBoundary baseline proving the two finders no longer diverge.

6. loading:false route — a rejecting loader no longer takes down its siblings (M10)

On the loading:false branch, each loader's raw promise was started and then await Promise.all(...) ran before any promise was wrapped. A single rejecting loader (1) propagated to the segment-level boundary, collapsing the whole entry and discarding successful sibling loader data, and (2) left the other in-flight raw promises with no .catch, producing unhandled rejections (log noise / isolate churn on workerd).

path("/dashboard", Dashboard, () => [
  loading(false),
  loader(loadStats),   // resolves
  loader(loadFeed),    // rejects
])

// BEFORE (loading:false): the whole Dashboard entry collapses to the SEGMENT
//   error boundary, loadStats data is discarded, and an unhandledRejection fires.
// AFTER: each loader hits its OWN per-loader error boundary; loadStats renders
//   intact; no unhandled rejection. Now matches loading:true and the intercept path.

fresh.ts now wraps each loader with deps.wrapLoaderPromise before awaiting (the wrapped promise resolves to a LoaderDataResult and never rejects), mirroring the already-correct streaming and intercept-resolution paths. Red→green unit test (loading-false-loader-error.test.ts).

7. Dev warning when an async handler returns Response/redirect() under loading() (M9)

On a route that declares loading(), an async handler that returns a Response (e.g. async () => redirect("/x")) has its redirect swallowed: the handler result is streamed (not awaited at the resolution boundary), so the Response surfaces only during RSC serialization and is rendered into the stream instead of becoming a 302/308. A synchronous Response return on the same route still redirects correctly. This is a documented limitation; the fix adds a loud dev-only warning at every streamed path so the silent failure stops being silent.

path("/old", Old, () => [
  loading(<Spinner />),
  handler(async () => redirect("/new")),  // async + Response under loading()
])

// BEFORE: no redirect, no warning — the Response renders into the RSC stream.
// AFTER (dev): console.warn — "[rango] Handler for "/old" returned a Response
//   (e.g. redirect()), but it declares loading(): the Response is rendered into
//   the RSC stream, NOT sent as an HTTP redirect. Issue redirects from middleware,
//   a loader, or a synchronous handler return."  (compiled out in production)
//
// Correct shapes (unchanged behavior):
middleware(async (ctx) => { if (!ctx.user) return redirect("/login"); })  // redirects
handler(() => redirect("/new"))                                           // sync → redirects

warnOnStreamedResponse() (helpers.ts) is wired into the loading() branch of fresh.ts and (completed in the gap pass) the revalidation route + parallel-slot paths and the intercept loading() path. It detects a Response on both resolve and reject, because the raw parallel-slot handler is not run through handleHandlerResult. Documented in execution-model.md and skills/route/SKILL.md.

8. Prerender Passthrough + loading() + ctx.passthrough() no longer bakes a corrupt artifact (M16)

matchForPrerender detected the passthrough sentinel synchronously via isPrerenderPassthrough(seg.component), which only works when the build handler's return value lands directly on seg.component. But when the route declares loading(), the handler result is deferred, so for an async build handler (every documented Passthrough build handler is async) seg.component is a Promise resolving to the sentinel — the sync guard inspected __brand on the Promise, returned false, and the build serialized a Flight payload of "a Promise resolving to the frozen sentinel" as a real manifest entry. At runtime that param set served the garbage artifact instead of falling through to the live handler.

path("/p/:id", Product, () => [
  Passthrough(),
  loading(<Spinner />),
  handler(async (ctx) => {
    if (!isPrerenderable(ctx.params.id)) return ctx.passthrough(); // skip baking this param
    return <Product id={ctx.params.id} />;
  }),
])

// BEFORE: build bakes a CORRUPT artifact for the passthrough param; runtime serves garbage.
// AFTER:  build detects the sentinel through the loading()-deferred thenable and
//         skips baking — the param falls through to the live handler at runtime.

Extracted detectPrerenderPassthrough() in prerender.ts that awaits any thenable seg.component before the sentinel check (rejections are swallowed and resurface during serialization, preserving prior error behavior), and wired it into prerender-match.ts. Red→green unit test (prerender-passthrough-detect.test.ts) pins the Promise-wrapped-sentinel case.

9. Loader-cache read/write/revalidate failures now reach onError (M13)

cache-error.ts documents a package-wide contract: cache failures must be LOUD (console + routed through onError with a CacheErrorCategory). Every other axis honored it; read-through-swr.ts — the engine behind opt-in loader caching — swallowed three failures in bare catch {} blocks, giving a consumer relying on onError for cache-health telemetry a silent blind spot precisely on loader caching.

createRouter({
  onError: (err) => report(err),   // a consumer's cache-health telemetry
  cacheStore: myKVStore,
})
path("/p/:id", Product, () => [ loader(loadProduct, () => [cache({ ttl: 60 })]) ])

// A corrupt/truncated stored loader entry, a throwing background revalidation,
// or a failing miss-path write:
//   BEFORE: swallowed in catch {} — onError never fires; you have no signal.
//   AFTER:  routed through reportCacheError with categories:
//     - cache-corrupt        (deserialize of a stored entry — then self-heals via the miss write)
//     - stale-revalidation   (background SWR refresh throws)
//     - cache-write          (miss-path serialize/write throws)

WaitUntilHost now intersects CacheErrorReporter; loader-cache.ts already passes the request context (which provides both waitUntil and the reporter), so onError fires from the detached waitUntil with no new plumbing. Test coverage extended in read-through-swr.test.ts (previously hit/stale/miss only).

10. HIGH — cross-host loader-cache leak in a multi-tenant host router (gap pass)

The route-level cache keys by `${host}${pathname}` (cache-scope) and "use cache" pushes ctx.url.host — but the loader-level cache() default key omitted the host entirely (loader:{loaderId}:{pathname}:{params}). In a multi-tenant host router serving the same pathname for different hosts, one host's cached loader data could be served to another tenant.

// A host router serving the same path for many tenants:
host("*.example.com", () => [
  path("/dashboard", Dashboard, () => [
    loader(loadTenantData, () => [cache({ ttl: 60 })]),
  ]),
])

// Default loader cache key:
//   BEFORE:  loader:loadTenantData:/dashboard
//            → a.example.com and b.example.com SHARE one entry → b sees a's data.
//   AFTER:   loader:loadTenantData:a.example.com/dashboard
//            loader:loadTenantData:b.example.com/dashboard   (isolated per host)

getDefaultLoaderCacheKey now threads getRequestContext()?.url?.host ?? "localhost" into the default key, matching the route-level cache and "use cache" identity. A custom options.key or a store-level keyGenerator still takes precedence (3-tier resolution unchanged). Red→green cross-host test (loader-cache.test.ts) asserts keyA !== keyB for two hosts at the same path. (No app in the repo currently runs a multi-tenant loader cache, so this never shipped a live exploit — but it is a correctness HIGH on a supported configuration.)

11. Bonus consumer fix: redirect() basename no longer double-prefixes a query/hash-only URL

// With an app basename of "/admin":
redirect("/admin?tab=x")   // BEFORE: "/admin/admin?tab=x"   AFTER: "/admin?tab=x"
redirect("/admin#frag")    // BEFORE: "/admin/admin#frag"    AFTER: "/admin#frag"

The auto-prefix guard now treats the basename as already-present when followed by /, ?, #, or end-of-string. Red→green tests in redirect.test.ts.

12. Public surface cleanup (pre-1.0 API hygiene)

Removed dead exports (no real consumer; cheapest to remove before first stable adoption):

// BEFORE — type-checked but referenced an internal-only component:
import { OutletProvider } from "@rangojs/router/client";
// AFTER — gone from the public API. OutletProvider stays internal to client.tsx
// (used by Outlet/ParallelOutlet); compose with the public <Outlet /> instead.

Added missing load-bearing re-exports that public JSDoc already references but were unimportable:

// AFTER — now importable from the canonical type entry (@rangojs/router):
import type {
  PrerenderPassthroughResult,                       // the ctx.passthrough() return type
  OriginCheckConfig, OriginCheckContext, OriginCheckPhase, // RangoOptions.originCheck JSDoc
  MiddlewareFn,                                      // so "extract the middleware fn and unit-test it" has a name
} from "@rangojs/router";

MiddlewareFn in particular gives the documented "extract the middleware function and unit-test it with runMiddleware" pattern a nameable type.

Internal improvements

• SSR double-render fixed (render-pipeline feat: add error boundaries and notFound boundaries with default fallbacks #2). createSSRHandler did not memoize reconstructedRoot like its sibling payload/handles promises, so React.use() on a fresh renderSegments promise suspended and replayed SsrRoot, re-running the entire segment-tree build on every initial document render. Now rootPromise ??= Promise.resolve(renderSegments(...)) so it runs once.
• Vite build-time loader pre-scan gated to the RSC environment (build feat: add error boundaries and notFound boundaries with default fallbacks #2). plugin-rsc runs ~5 build passes (rsc-scan, ssr-scan, rsc, client, ssr) over a single shared plugin instance; the full-project walk-and-read pre-scan ran on every pass even though only the RSC pass consumes the registry — roughly 5× the build-time I/O. expose-internal-ids.ts now skips when this.environment is known and not rsc (an unavailable environment still falls through, so no empty registry).
• Slot-name "D" mis-parse fixed (render Router idea 2 revisit #1). Loader-to-parent association split the loader segment id on the first bare "D" (segment.id.split("D")[0]). For an intercept slot loader ${parent}.${slotName}D${i}.${loaderId}, a slot name containing an uppercase D (e.g. @Detail) cut inside the slot name, so the loader matched no parent and its data was silently dropped. New loaderParentId() respects the id grammar. Red→green test in segment-system.test.tsx.
• Dead navigation-store singleton removed. initNavigationStore / getNavigationStore / resetNavigationStore had no production caller (the boot path calls createNavigationStore() directly via the module handle); the singleton only ever threw in a running app. Removed the trio plus its self-referential test block.
• Dedup into shared modules. paramsEqual → router/params-util.ts; observeStreamedHandler → segment-resolution/streamed-handler-telemetry.ts (was an inline copy in fresh.ts).
• ~10 dead types/exports removed beyond the public ones above (RouterDependencies, InterceptResolutionDeps, LoaderRevalidationResult, RouteContentWrapperCallback, stray export/re-export keywords).
• Stale-doc fixes: manifests.md (the eager setRouteTrie/setPrecomputedEntries example that primed the exact CLAUDE.md rule Router idea 2 revisit #1 bundle regression), use-cache-api-design.md (phantom JSON fallback), caching.md, feature-file-map.md (phantom paths), host/pattern-matcher.ts (apex heuristic comment), internal/execution-model.md (the loading()+redirect limitation), SSR JSDoc package names (@rangojs/router/ssr, not rsc-router/ssr).
• Comment-noise reduction (cosmetic). ~3.3k lines of redundant comments stripped (decorative banners, next-line restatements, obvious step narration) across the shipped source, keeping the load-bearing ones: WHY/rationale, non-obvious invariants and scar-tissue, public-API JSDoc, and contract notes (handler-first ordering, cache scope, dev/prod parity, bundle hygiene). Directive comments (@ts-expect-error, eslint/oxlint-disable, @__PURE__) and the use client/use server/use cache string directives are untouched.

Behavior-change semantics table

Code-change summary

Correctness

• src/segment-system.tsx — loaderParentId() grammar-aware slot-D parse; isRenderableLoading three-term gate extracted.
• src/ssr/index.tsx — memoize rootPromise (single initial render).
• src/router/trie-matching.ts — leafConstraintsPass() threaded into walk frames; constraintsSatisfied() shared backstop.
• src/router/error-handling.ts — orphan-sibling scan in findNearestNotFoundBoundary.
• src/router/segment-resolution/fresh.ts — wrap-before-await on the loading:false branch; warnOnStreamedResponse on the loading() branch.
• src/router/segment-resolution/helpers.ts — warnOnStreamedResponse() (resolve and reject).
• src/router/segment-resolution/revalidation.ts, src/router/intercept-resolution.ts — warnOnStreamedResponse wired into revalidation route/parallel-slot and intercept loading() paths.
• src/prerender.ts + src/router/prerender-match.ts — detectPrerenderPassthrough() awaits thenable components; intercept flatten simplified.
• src/cache/read-through-swr.ts — three reportCacheError paths (cache-corrupt/stale-revalidation/cache-write); WaitUntilHost & CacheErrorReporter.
• src/router/segment-resolution/loader-cache.ts — host threaded into getDefaultLoaderCacheKey (cross-host leak).
• src/route-definition/redirect.ts — basename guard for ?/#/EOS.

Types / public API

• src/route-definition/helpers-types.ts — transition() overloads; cache → PartialCacheOptions<TEnv>.
• src/urls/path-helper-types.ts — cache → PartialCacheOptions<TEnv>.
• src/route-definition/dsl-helpers.ts — drop the string profile branch + | string param + JSDoc.
• src/index.rsc.ts — + OriginCheck* / PrerenderPassthroughResult / MiddlewareFn.
• src/client.tsx / src/client.rsc.tsx — drop public OutletProvider; segment-system.tsx points at ./outlet-provider.
• src/cache/index.ts, src/cache/cf/index.ts, src/cache/types.ts — dead cache exports/types removed.

Perf / internal

• src/vite/plugins/expose-internal-ids.ts — gate the build-time pre-scan to the RSC env (was ~5×).
• new src/router/params-util.ts, src/router/segment-resolution/streamed-handler-telemetry.ts (dedup).
• src/browser/navigation-store.ts / navigation-store-handle.ts / rsc-router.tsx — dead singleton removed.

Test plan

• Red→green unit tests, one per fix, each proven red-before-green: slot-D parse (segment-system.test.tsx), trie backtracking (trie-matching.test.ts), orphan notFoundBoundary (boundary-orphan-resolution.test.ts), loading:false loader isolation (loading-false-loader-error.test.ts), loud cache errors (read-through-swr.test.ts), prerender-passthrough detection (prerender-passthrough-detect.test.ts), dev warning (warn-streamed-response.test.ts), cross-host loader key (loader-cache.test.ts), redirect double-prefix (redirect.test.ts), and type-pins for transition()/cache() TEnv (dsl-helper-types.check.ts, with the TS2554/TS2559/TS18046 red captured).
• Semantic matrix — 39 passed (dev + production). No change to middleware scope, handler-first ordering, context visibility, or PE/JS parity.
• Full gate green on 0.0.0-experimental.124: typecheck, 3843 unit tests, lint (oxlint), format (oxfmt).
• Open item — deferred to e2e coverage gaps from review: prerender Passthrough+loading(), redirect under loading() #567: dev+prod e2e for prerender Passthrough+loading()+ctx.passthrough() (M16) and async-redirect()-under-loading() (M9). Both are unit-pinned here; the e2e fixtures are written and embedded in the issue. (Per the repo's dev+prod e2e rule, this gap is flagged, not silently closed.)

Notes / deferred

Tracked as GitHub issues, each with userland examples:

• e2e coverage gaps from review: prerender Passthrough+loading(), redirect under loading() #567 — e2e coverage gaps: prerender Passthrough+loading() (M16), async-redirect-under-loading() (M9), intercept _currentSegmentId, server-action-bridge over-invalidation, document-cache private,s-maxage; plus a layer-by-layer unit-vs-e2e assessment using the new @rangojs/router/testing primitives.
• Matrix-critical refactors from review (fresh/revalidation unification, trie suffix order, loader-cache rebind) #568 — matrix-critical refactors: fresh/revalidation unification, M11 orphan-revalidation extraction (deferred here — the orphan path has two deliberate divergences and the copies have drifted before, so a blind extraction is risky), parallel-slot dedup, merge-clone Response, yieldFromStore dedup, loader-cache ctx.use rebind, trie longest-suffix.
• Pre-1.0 public-API decisions from review (router.use, useAction types, buildEnv, plugin name, ...) #569 — pre-1.0 public-API decisions: router.use pattern-param overload, buildEnv per-preset type restructure, plugin rename vite-plugin-poke → @rangojs/router:poke, StaticHandlerOptions doc-sync.
• Safe cleanup backlog from review (write-only fields, dedup helpers, test-only dupes) #570 — safe cleanup backlog: write-only fields, remaining dedup helpers, test-only dupes.
• Gap-review follow-ups: security defense-in-depth + @rangojs/router/testing refinements #572 — gap-review security/testing follow-ups surfaced alongside the cross-host fix.

~13 low/info findings deliberately dropped (intentional patterns, negligible perf, consistency warts with no present defect) — each recorded in the tracker rather than actioned.