refactor: convert the repository family to Hono sub-apps - #1727
Conversation
|
Warning Review limit reachedNext included review available in 6 minutes. View limit detailsLimit details: You’ve used all 8 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (4)
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 (10)
Included review availability: Your plan provides up to 8 included reviews per hour; 4 remain after this review. 📝 WalkthroughWalkthroughThe control-plane routes migrated from custom route arrays to typed Hono routers. Handlers now use typed path parameters and shared admission and dispatch helpers. Repository validation moved to ChangesRepository routing
Environment and secret routing
Image-build routing
Route catalog mounting
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to The route migration preserves the existing route catalog and authorization behavior, with no concrete merge-blocking risk identified. 🚥 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}
There was a problem hiding this comment.
Summary
PR #1727, refactor: convert the repository family to Hono sub-apps, by @ColeMurray converts the repository, secrets, environment, environment-secrets, and image-build route families to admitted Hono sub-apps while preserving route order and policy contracts. Reviewed 10 changed files (+426/-490); the implementation is ready to merge.
Critical Issues
None.
Suggestions
None.
Nitpicks
None.
Positive Feedback
dispatchkeeps decoded parameter types tied to each path literal, reducing wrong-handler wiring risk at compile time.repositoryParamspreserves the decode-once contract, including nested owners and rejection of decoded slashes in repository names.- The updated tests exercise production sub-app dispatch and admission rather than invoking handlers through hand-built matches.
Questions
None.
Verification
- Control-plane unit suite: 234 files, 3,495 tests passed.
- Relevant workerd integration suites: 2 files, 18 tests passed.
- Control-plane typecheck and lint passed.
git diff --checkpassed.
Verdict
Approve: No blocking or non-blocking findings.
Repos, secrets, environments, environment secrets, and image builds register their 27 routes as Hono sub-apps mounted at the catalog positions the spreads held, so precedence is unchanged. Handlers take the parameters Hono decoded, typed from the path, through the `dispatch(c, handler)` adapter; parameterless handlers keep their three-argument form. The owner/name handlers stop decoding. `repositoryParams()` validates the pair Hono already decoded and answers the same 400 as before, and no handler in this family calls `decodeURIComponent` any more; admission refuses a segment that does not decode. The repos, environment-secrets, and image-build trigger suites now dispatch requests through the production sub-apps instead of calling handlers with hand-built match arrays, so a route bound to the wrong handler fails a test. Both integration snapshots are unchanged. Claude-Session: https://claude.ai/code/session_01KdDpTgGEjXpBA9SaGQVUH1
c14e995 to
99ae138
Compare
Terraform Validation Results
Pushed by: @ColeMurray, Action: |
There was a problem hiding this comment.
Deep code-quality audit completed against the current rebased head (99ae138).
No blocking or non-blocking findings. This conversion removes more code than it adds, keeps every affected file below 1,000 lines, preserves catalog precedence and admission policies, and strengthens the changed tests by exercising the production Hono routing boundary instead of hand-built matcher arrays. The repository parameter helper is narrow and preserves the decode-once invariant without leaking Hono concerns into shared repository identity utilities.
Verification: reviewed all 10 changed files and surrounding admission, route-contract, repository identity, and test-support code; git diff --check is clean; GitHub unit, integration, typecheck, lint, web, and bot checks pass.
… module Review follow-up. `repositoryParams()` re-implemented the pair round-trip check that `decodeRepositoryPathSegments` embeds, leaving two owners of a subtle rule (a nested owner is valid, a slash in a name is not). `validateRepositoryPathSegments(owner, name)` now holds it once in the shared package; the decoder decodes and delegates, and the route adapter only maps null to the 400. The decoded and nested-owner cases join the shared contract tests. Claude-Session: https://claude.ai/code/session_01KdDpTgGEjXpBA9SaGQVUH1
Terraform Validation Results
Pushed by: @ColeMurray, Action: |
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 -->
1 participant
Rebased onto
mainafter #1724 merged; the diff is this PR alone.PR 4 of the Hono follow-up series (plan:
docs/internal/2026-09-02-control-plane-hono-follow-ups.mdin prod). Converts the repository family to Hono sub-apps; 81 of 171 routes are now native.What changed
repossecretsenvironmentsenvironment-secretsimage-buildsHonosub-app mounted at the catalog position its spread held, so precedence is unchanged.dispatch(c, handler)adapter from refactor: convert the sessions cluster to Hono sub-apps #1724. Parameterless handlers keep their three-argument form.defineRoute(s)declared; repeated ones are hoisted intoadmit()constants.Decode once (D-B)
The owner/name handlers stop decoding.
repositoryParams()(new,routes/repository-params.ts) validates the pair Hono already decoded and answers the sameOwner and name must be valid repository path segments400 as before. No handler in this family callsdecodeURIComponent; a segment that does not decode is refused by admission (#1724). The matrix's repo cases (group%2Fsubgroup,web%2Fapp,web%252Fapp) hold without edits.extractRepoParamsstays inroutes/shared.tsfor the modules PR 6 converts; PR 8 deletes it.Tests
repos.test.ts,environment-secrets.test.ts, andimage-builds.trigger.test.tsnow dispatch requests through the production sub-apps (createTestRequestHandler([module]), mockedauthenticate, owner authorization database) instead of calling handlers with hand-built match arrays, so a route bound to the wrong handler fails a test. One fixture change: the environment-secretsbatchmock returns[](D1's shape) because the request path goes through the instrumented database, which reads the batch result.Verification
tsconfig.json+tsconfig.test.json), ESLint, Prettierhttps://claude.ai/code/session_01KdDpTgGEjXpBA9SaGQVUH1
Summary by CodeRabbit
Refactor
Tests