refactor: admit routes through Hono middleware - #1721
Merged
Conversation
Route admission runs as admit(policy) middleware ahead of each catalog handler, and a lifecycle middleware owns the guards, request context, request log, authorization audit, and common headers. A handler that answers without admission running ahead of it is refused with a 500. The raw-path regex re-match, Route.pattern, parsePattern, the dispatch chain, and the start-time WeakMap are gone. Admission reads raw path parameters by position from the pathname, and a legacy adapter rebuilds the match array handlers still take, so no route module changes. The app takes a host for its background-task port instead of a Cloudflare ExecutionContext. Claude-Session: https://claude.ai/code/session_01KdDpTgGEjXpBA9SaGQVUH1
Terraform Validation Results
Pushed by: @ColeMurray, Action: |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (8)
Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughThe control-plane router now uses path-based route definitions, structured parameters, admission middleware, and a host-based Hono lifecycle. Tests use shared route matching and validate admission, errors, CORS, logging, and route contracts. ChangesControl-plane routing
Estimated code review effort: 4 (Complex) | ~60 minutes Merge Risk: 🔵 Low · up to The refactor centralizes route admission and error handling, but an unadmitted handler could still execute before its response is rejected, and an early host-setup failure may bypass the intended sanitized error path. Current in-repository composition limits both cases, so the PR is mergeable with explicit owner awareness or follow-up. Sequence Diagram(s)sequenceDiagram
participant Client
participant ControlPlaneApp
participant admit
participant admitRoute
participant RouteHandler
Client->>ControlPlaneApp: Send request
ControlPlaneApp->>admit: Evaluate route policy
admit->>admitRoute: Pass named RouteParams
admitRoute-->>admit: Return admission result
admit->>RouteHandler: Invoke admitted route
RouteHandler-->>ControlPlaneApp: Return response
ControlPlaneApp-->>Client: Finalize response
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![open-inspect[bot]](https://avatars.githubusercontent.com/in/2661131?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
The middleware migration has a promising direction, but it currently introduces three structural boundary problems that should be resolved before merging. Most importantly, fallible admission work occurs before route state is recorded, so one entire failure class bypasses the lifecycle finalizer. The new raw-parameter extractor also accepts route declarations that violate its own RouteParams contract, and the host abstraction removes a faithful adapter only by replacing it with unknown and reciprocal casts. These are not cosmetic concerns: they create hidden response modes and make the central routing boundary less type-safe. No changed file crosses the 1k-line threshold, and the focused routing suites plus all current CI checks pass, but the missing cases were confirmed with temporary regression probes.
Contributor
There was a problem hiding this comment.
Summary
PR #1721, refactor: admit routes through Hono middleware, by @ColeMurray moves route admission and request lifecycle behavior into Hono middleware while preserving the legacy handler interface. The overall structure is well tested, but an exception during admission bypasses the new response-finalization guarantees and should be fixed before merge.
Files changed: 27, with 716 additions and 498 deletions.
Critical Issues
- [Error handling / observability]
packages/control-plane/src/routing/hono-app.ts:149- If admission throws beforec.set("admission", ...), Hono creates the sanitized 500 and this branch returns it without CORS, correlation headers, or the selected route's cache policy. A service-auth identity lookup failure is one concrete path to this state. Preserve enough route policy before awaiting admission, finalize this response, and cover the case with a test.
Suggestions
- [Route contract]
packages/control-plane/src/routing/hono-app.ts:40- Reject duplicate parameter names during route validation. The removed named-capture regex rejected these at construction, whilerawRouteParams()now silently overwrites an earlier value. - [Testing]
packages/control-plane/src/router.policy.test.ts:85- Check parameter names as complete path segments rather than substrings so:idcannot be satisfied by:identity.
Nitpicks
None.
Positive Feedback
- The default-deny behavior correctly clears prior response headers, preventing an unadmitted handler from leaking cookies or other metadata.
- The conformance fixture exercises all 171 routes with encoded slashes, which gives strong coverage for the raw-parameter compatibility requirement.
- HEAD, OPTIONS, unknown-route,
HttpError, ordinaryError, and non-Errorthrow behavior are covered directly.
Questions
None.
Verdict
Request Changes: address the admission-exception response finalization gap. The duplicate-parameter and segment-exact checks are recommended hardening.
Verification: 147 focused unit tests passed, the 171-route integration conformance test passed, and control-plane typecheck, lint, and diff checks passed.
An error thrown inside admission now carries the selected route's response policy and the common headers. The host port is typed against the platform execution context with a faithful test adapter, raw parameters use a null-prototype dictionary and refuse a segment mismatch, duplicate parameter names are rejected at build, and the policy test matches parameter names as whole segments. Claude-Session: https://claude.ai/code/session_01KdDpTgGEjXpBA9SaGQVUH1
Terraform Validation Results
Pushed by: @ColeMurray, Action: |
ColeMurray
added a commit
that referenced
this pull request
Sep 3, 2026
## Summary Third step of the series retiring the routing adapter (#1720 guardrails, #1721 mechanism). This PR converts the first six route modules to native Hono sub-apps and establishes the pattern the remaining module PRs follow. 12 routes convert; the other 159 still go through the catalog adapter. **A converted module** is a `Hono` sub-app whose routes register with `admit(policy)` as their first middleware and a native handler behind it: ```ts export const keyboardShortcutRoutes = new Hono<ControlPlaneHonoEnv>(); keyboardShortcutRoutes.get( "/keyboard-shortcuts", admit({ ...SCM_AGNOSTIC_HUMAN_USER_ROUTE, authorization: ACTIVE_SELF }), async (c) => { const { ctx } = c.var.admitted; // ctx.principal is typed UserPrincipal by the policy ... } ); ``` `admit()` is now generic over its policy and sets `c.var.admitted` as `{ request, ctx }`, where `request` is the request as authentication returned it (sig1 verification consumes the original body) and `ctx` is the request context narrowed by the policy's authentication kind, exactly as `RouteContext<Authentication>` narrowed the legacy handler's fourth argument. Handlers use `c.req.param()` for path parameters; none of the six modules has one, so no decoding question arises in this PR. **The catalog mounts modules in place.** `catalog` (renamed from `routes`) is an ordered list of `Route | RouteModule`. `createControlPlaneApp` mounts a sub-app with `app.route("/", module)` where it appears and registers a legacy route through the adapter where it appears, so registration order, and therefore precedence, is unchanged. The path grammar and duplicate-parameter checks run for module routes too. **Route contracts come from Hono's own route list.** `listRouteContracts(app)` walks `app.routes` and yields `{ method, path, ...policy }` for every entry whose handler is the `admit()` middleware, which carries the policy it enforces. The policy tests, the route-admission matrix, its sentinel suite, and the conformance suite iterate that list instead of the catalog array, so they cover converted and legacy routes alike. Both integration snapshots are byte-identical to `main`. **Handler-level tests become request-level.** `analytics.test.ts` went through `createTestRequestHandler([analyticsRoutes])` with `authenticate` mocked to return a user and `ownerAuthorizationDatabase()` answering the effective-authorization lookup, which is the pattern `router.authorization-audit.test.ts` already used. Same assertions, plus a denial case that proves no store is touched. The remaining 26 handler-level test files convert with their modules. Converted: `health` (moved out of `catalog.ts` into its own module), `keyboard-shortcuts`, `model-preferences`, `sign-in-providers`, `audit-events`, `analytics`. ## Verification - Unit: 234 files, 3,474 passed. - Integration (workerd, real D1): 96 files, 1,128 passed. Route-matrix and conformance snapshots unchanged. - Typecheck (`src`, `tsconfig.test.json`, `test/integration`), ESLint, and Prettier clean. https://claude.ai/code/session_01KdDpTgGEjXpBA9SaGQVUH1 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added a health check endpoint reporting service availability. - Expanded support for modular route handling while preserving existing endpoint access policies. - Added route contract reporting for endpoint methods, paths, and access requirements. - **Bug Fixes** - Preserved authentication, authorization, caching, and request-context behavior across updated endpoints. - Improved validation for unsupported route patterns and improperly configured routes. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
ColeMurray
added a commit
that referenced
this pull request
Sep 3, 2026
## Summary Fourth step of the series retiring the routing adapter (#1720, #1721, #1723). This PR converts the sessions cluster, the largest and most policy-diverse group, to native Hono sub-apps: 42 routes across 13 session modules plus the Slack notification route. It is the design proof for path parameters and for the sandbox-fallback and exact-sandbox policies. **Modules.** Each session module is a `Hono` sub-app registering `admit(policy)` first, and `sessions.ts` nests them under one sub-app in the old precedence order (`/sessions/inbox` still registers before `/sessions/:id`). The catalog mounts that sub-app and the Slack notification module where the spread and the inline route used to sit. **Handlers** keep their bodies and take a typed params object in place of the match array: ```ts export async function handleGetChild( request: Request, env: Env, params: { id: string; childId: string }, ctx: SessionRouteContext ): Promise<Response> ``` Registration passes `c.req.param()`, which Hono types from the path literal, so a handler declaring `childId` cannot be wired to a route without one. `withSessionRuntime(env, ctx)` replaces the `sessionRoute()` wrapper: it attaches the runtime client at the call site, and the handler-level tests use the same function to build their context. **Runtime proxy.** The three factories (`simpleProxy`, `legacyTokenRefresh`, `lifecycleProxy`) now build handlers rather than catalog entries, collected in `sessionRuntimeProxyHandlers` and registered one route each. The tests select handlers from that map by the production contract instead of scanning a route array. **Decode once, in Hono.** Admission now reads `c.req.param()`, so the session id the sandbox binding verifies, the automation id the ownership check loads, and any actorless-grant path parameter are Hono's decoded values, and admission's own `decodeURIComponent` calls are removed. `rawRouteParams` survives only inside the legacy adapter, for the modules not yet converted, and goes with them. ## Behavior change An encoded session id now resolves the same session as its plain form: `GET /sessions/%74est-…` returns the session rather than 404, and a sandbox token on `/sessions/%74est-…/tunnel-urls` verifies rather than 401. The guardrail tests from #1720 record this as their new expectation. No other route observed a different status: both integration snapshots are byte-identical to `main`. ## Tests Handler-level tests pass params objects and get their session runtime from `withSessionRuntime`; assertions are unchanged. `router.create-session.test.ts` calls the exported `handleCreateSession` directly. No test lost coverage; the route-matrix suite covers every converted route per credential class as before. ## Verification - Unit: 234 files, 3,476 passed. - Integration (workerd, real D1): 96 files, 1,128 passed. Route-matrix and conformance snapshots unchanged. - Typecheck (`src`, `tsconfig.test.json`, `test/integration`), ESLint, and Prettier clean. ## Review follow-up (71224c4) - **Malformed path segments are refused at admission.** Hono leaves a segment it cannot decode as it arrived, so the automation-id requirement's 400 had become a 404. `admit()` now checks every raw `:param` segment once and answers `400 { error: "Invalid path encoding" }` on every route before any lookup. This replaces the three route-specific messages `main` produced for this input (`Invalid automation route`, `Invalid user ID` on malformed member ids, and the session routes' 404); the two RBAC integration expectations and the routing-compatibility probe record the uniform envelope. Both matrix snapshots are still byte-identical. - **One call-site adapter.** `dispatch(c, handler)` in `routing/admit.ts` is the only place a Hono context is unpacked for a handler; `dispatchSession(c, handler)` adds the runtime client. A route reads `admit(policy), (c) => dispatchSession(c, handleX)`. The context type is checked against the policy, so a user-only handler behind a user-or-service policy fails to compile (a curried `handle(handler)` form was tried and rejected because Hono infers the environment from the returned function and that check disappears). - **Proxy tests dispatch through the production sub-app.** The test-only handler map is gone; the runtime proxy registers its handlers directly, and `session-runtime-proxy.test.ts` sends real requests through `sessionRuntimeProxyRoutes`, with a 17-row wiring table that fails if a route is bound to the wrong proxy. https://claude.ai/code/session_01KdDpTgGEjXpBA9SaGQVUH1 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Improvements** * Standardized routing and authorization across session, attachment, media, child-session, prompt, pull-request, skills, WebSocket token, and notification endpoints. * Preserved existing endpoint paths, permissions, upload validation, streaming, and error handling. * Improved handling of percent-encoded path values so valid requests resolve correctly. * **Bug Fixes** * Malformed percent-encoded path segments now return a clear 400 error before processing. * **Tests** * Expanded coverage for routing, authorization, encoded paths, and request dispatch. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
ColeMurray
added a commit
that referenced
this pull request
Sep 3, 2026
Last PR of the Hono series (#1720, #1721, #1723, #1724, #1726, #1727, #1728, #1729). Every route module is a Hono sub-app, so the adapter that carried catalog routes through admission has nothing left to serve. ## Removed - `legacy()` and `legacyMatch()` in `routing/hono-app.ts`; `createControlPlaneApp(modules, host)` mounts modules only. - `RouteDefinition`, `Route`, `defineRoute()`, `defineRoutes()`, and `extractRepoParams()` in `routes/shared.ts`. `RouteAdmissionPolicy` is now an interface over `RoutePolicy` plus `authorization` and `serviceActorClaims`; `cacheControl` lives on `AdmissionPolicy` in `routing/admit.ts`. - `RouteCatalogEntry`; the catalog is `readonly RouteModule[]`. - `legacyRoutes()` in test support; `matchRoute()` no longer fabricates a `RegExpMatchArray`. - `routes/shared.test.ts`, replaced by `routes/repository-params.test.ts` over the decoded pair. ## Kept on purpose - `RouteParams`: admission's decoded-parameter dictionary. Hono exports no such type. - `rawRouteParams()`: the malformed-encoding guard's raw read-back (added in #1724). ## Tests Fixtures that need synthetic routes build a `Hono` module and pass it to `createTestRequestHandler([module])`: the lifecycle suite, the contract lister's suite, and the authorization-audit suite's eleven routes. The two Worker-boundary suites build their shadow catalogs the same way: conformance echoes the raw read-back through `rawRouteParams()`, and the matrix's "raw path segments" probe from #1720 is now the decode-once assertion on what handlers receive (`abc%2Fdef` arrives as `abc/def`, `web%252Fapp` as `web%2Fapp`). ## Verification | Check | Result | |---|---| | Typecheck (src, test, integration) | clean | | ESLint, Prettier | clean | | Unit | 234 files, 3,512 passed | | Integration (workerd, real D1) | 96 files, 1,129 passed | | Matrix and conformance snapshots | byte-identical | https://claude.ai/code/session_01KdDpTgGEjXpBA9SaGQVUH1 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Repository paths now support nested owner namespaces, such as `group/subgroup`. - Repository names containing slash characters are rejected with a clear validation message. - URL path parameters are decoded consistently, improving handling of encoded repository and member identifiers. - **Refactor** - Control-plane routing now uses the current route-module system, providing consistent admission, route matching, and request handling behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Second step of the series retiring the routing adapter left by #1716 (guardrails landed in #1720). This PR replaces the mechanism around every route while leaving all 171 route handlers untouched; after it, the remaining shim is one
legacy()adapter function that later PRs delete module by module.Admission is Hono middleware.
admit(policy)(src/routing/admit.ts) evaluates the route's declarative policy through the unchangedadmitRoute()pipeline and setsc.var.admission. Denials answer from the middleware, with an authorization denial audited before anything is logged, as before. A principal-less policy that requires authorization is refused when the middleware is built.The lifecycle is one
app.use("*")(src/routing/hono-app.ts) that owns whatdispatchMatchedRouteand the outer handler used to: the DB guard (undecorated 503), the HEAD guard (404, never Hono's implicit GET), the request context and start time, the request log, the audit of allowed decisions after the handler including the 500 path, and the common response headers with the route'sCache-Control.app.onErrormapsHttpErrorto the{ error }envelope and logs anything else as the sanitized 500; a non-Errorthrow takes the same path.Default deny. If a handler answers without
admit()having run ahead of it, the lifecycle replaces the response with a sanitized 500, drops the handler's headers, and logsrouter.unadmitted_response. Every policy, includingpublic, sets the admission variable, so a route registered withoutadmit()fails closed on its first request; the matrix suite drives every route in CI. This is the request-time check chosen in review of #1720 over the build-time registry.Host-injected entrypoint.
createControlPlaneApp(catalog, host)takes a host whose one job is to build theBackgroundTasksport from whatever the platform passed as the execution context.cloudflareHostwrapswaitUntil; the unit-test host hands the port straight through, which deletes the fakeExecutionContextinrouter.test-support.ts.handleControlPlaneHttpandcreateControlPlaneHttpHandlerkeep their signatures, soindex.tsis unchanged.Deleted:
Route.pattern,parsePattern, the raw-path re-match and itsrouter.match_mismatchbranch,route-dispatch.ts, the start-timeWeakMap. Admission now takesparams: RouteParams(raw segments read back from the pathname by position,src/routing/route-params.ts) instead of aRegExpMatchArray, andlegacyMatch()rebuilds the array handlers still read from those params.Test changes. Unit tests that selected routes by
route.patternusematchRoute()/routePathPattern()from test support instead; no assertions changed. The conformance snapshot drops thepatternfield from each of its 171 rows (identities, groups, policies, and order verified identical before and after). Newhono-app.test.tscovers the default deny (including header non-leakage), preflight and 404 exemptions, HEAD,HttpErrormapping, unexpected and non-Errorthrows, and both build-time refusals.Behavior notes
app.fetch. The difference is the router lookup.Verification
src,tsconfig.test.json,test/integration), ESLint, and Prettier clean.https://claude.ai/code/session_01KdDpTgGEjXpBA9SaGQVUH1
Summary by CodeRabbit