refactor: convert settings, skills, and RBAC routes to Hono sub-apps - #1729
Conversation
Converts model-provider-accounts, integration-settings, commit-signing, scm-settings, mcp-servers, skills, and rbac to Hono sub-apps mounted at their catalog positions (66 routes). Each route is admit(policy) followed by dispatch(c, handler); handlers take the parameters Hono decoded, typed from the path literal, and the policies are unchanged. Handlers stop decoding. integration-settings and scm-settings read the repository pair through repositoryParams(), a validate-only check over the segments Hono already decoded; rbac drops its decodePathSegment and keeps the canonical-user-id check with its "Invalid user ID" answer. Malformed percent-encoding is refused by admission on every route, so the role route's own decode-failure branch is gone. model-provider-accounts registers its handlers directly instead of building routes through managementRoute(). The scm-settings tests dispatch through the production sub-app with a mocked store and a per-route wiring table; the policy test's malformed-role case is request-level and shows D1 is never touched. 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 (4)
Included review availability: Your plan provides up to 8 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughControl-plane route modules migrated from legacy route arrays to typed Hono routers with ChangesControl-plane routing
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to Repository path validation is now centralized while preserving valid repository identifiers and rejecting malformed paths with the existing client-error behavior. The covered routing and validation changes show no remaining merge-blocking risk. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Title checkExplanation The title accurately describes the Hono sub-app migration for settings, skills, and RBAC routes. It omits several additional migrated route modules, but it remains clear and related to the primary refactor.
✨ 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 #1729, refactor: convert settings, skills, and RBAC routes to Hono sub-apps, by @ColeMurray converts seven control-plane route modules from legacy route arrays to admitted Hono sub-apps while preserving their catalog positions and request policies. The PR changes 11 files with 702 additions and 711 deletions; I found no blocking correctness, security, performance, or maintainability issues.
Critical Issues
None.
Suggestions
None.
Nitpicks
None.
Positive Feedback
- Authentication, authorization, SCM support, cache directives, and route ordering remain consistent with the previous declarations.
- The decode-once boundary is centralized in Hono admission, and repository pair validation preserves nested owner namespaces while rejecting decoded slashes in repository names.
- The request-level SCM settings tests exercise production routing, admission, store wiring, malformed settings, storage failures, and single-decoding behavior.
Questions
None.
Verification
- Control-plane unit suite: 234 files, 3,502 tests passed.
- Focused routing suites: 3 files, 128 tests passed.
- Control-plane TypeScript typecheck passed.
git diff --checkpassed.- GitHub CI checks are passing.
Verdict
Approve: Ready to merge.
There was a problem hiding this comment.
The Hono conversions preserve the existing route policies and catalog ordering, and they reduce the size of the converted modules. One maintainability blocker remains: repository-pair validity is now implemented independently in the route layer even though the shared repository identity module already owns the same invariant. This boundary is subtle (nested owners are valid; names containing decoded slashes are not), so two implementations are likely to drift as more route families migrate. Please factor validation of an already-decoded RepositoryPair into the shared identity module, have decodeRepositoryPathSegments compose it, and keep only HTTP error mapping in the route adapter. Add the decoded/nested-owner cases to the shared contract tests.
Verification: focused control-plane tests (116) passed; shared repository contract tests (35) passed; git diff --check passed; all GitHub CI checks are green.
| owner: string; | ||
| name: string; | ||
| }): { owner: string; name: string } | Response { | ||
| const repository = parseRepositoryFullName( |
There was a problem hiding this comment.
[deep review] This duplicates the repository-pair validity algorithm already embedded in decodeRepositoryPathSegments (parse(format(pair)) plus the exact round-trip check), creating two canonical implementations of a subtle invariant. Nested owners make this especially risky: future rule changes can make legacy and Hono routes disagree. The code-judo move is to extract a shared validator for an already-decoded RepositoryPair, make decodeRepositoryPathSegments decode and delegate to it, and let this route adapter only rename fields/map null to the HTTP error. Please keep the nested-owner and slash-in-name cases in the shared contract tests so every transport gets the same semantics.
There was a problem hiding this comment.
Done in e7312b1. validateRepositoryPathSegments(owner, name) in the shared identity module now owns the rule; decodeRepositoryPathSegments decodes and delegates, and the route adapter only maps null to the 400. The decoded and nested-owner cases are in the shared contract tests. The same commit is on #1727, which carries the identical file.
… 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
PR 6 of the Hono follow-up series (after #1724). Converts seven modules to Hono sub-apps, mounted at their existing catalog positions so precedence is unchanged:
Every route reads
admit(policy)followed by(c) => dispatch(c, handler). Handlers take the parameters Hono decoded, typed from the path literal, and each route's authentication, authorization, SCM support, and cache policy are whatdefineRoute(s)declared before.Decode once. No handler in these modules calls
decodeURIComponentany more.:owner/:namethroughrepositoryParams()(newroutes/repository-params.ts), a validate-only check over the pair Hono already decoded. PR 4 adds the identical file for the repository family.decodePathSegment. Member ids keep the canonical-user-id check and itsInvalid user IDanswer, so the matrix case for a doubly-encoded member id still holds. The role route had no validity rule beyond decodability, and admission now refuses a segment Hono cannot decode on every route, so itsInvalid role IDbranch is gone rather than replaced by an invented pattern.Routeobjects throughmanagementRoute(); theverify/disable/enabletrio registers in a loop.Tests.
scm-settings.test.tsdispatches through the production sub-app with a mocked store: a six-row wiring table asserts which store method each route reaches and that no other does, the storage-failure and malformed-settings cases are kept, and a new case showsweb%2Fappis refused after one decode whileweb%252Fappreaches the store asweb%2Fapp./roles/%E0%A4%Aanswers 400Invalid path encodingbefore authentication, and D1 is never prepared.Verification (on the tree rebased onto
mainat 7b561a5)https://claude.ai/code/session_01KdDpTgGEjXpBA9SaGQVUH1
Summary by CodeRabbit
Bug Fixes
400 Invalid path encodingresponse.400response.Improvements