refactor: delete the legacy route adapter - #1730
Conversation
Every route module is a Hono sub-app now, so the adapter that carried catalog routes through admission has nothing left to serve. Gone: `legacy()` and `legacyMatch()` in the app, `RouteDefinition`, `Route`, `defineRoute()`, `defineRoutes()`, and `extractRepoParams()` in the route vocabulary, `RouteCatalogEntry`, and `legacyRoutes()` in test support. `createControlPlaneApp(modules, host)` mounts modules only, and the catalog is the ordered list of them. `RouteAdmissionPolicy` is an interface over the route policy plus authorization and service-actor claims; `cacheControl` lives on the admission policy. Kept on purpose: `RouteParams`, admission's decoded-parameter dictionary (Hono exports no such type), and `rawRouteParams()`, which the malformed-encoding guard reads. Test fixtures that need synthetic routes build a module and pass it to `createTestRequestHandler([module])`. The two Worker-boundary suites build their shadow catalogs the same way: conformance echoes the raw read-back, and the matrix's raw-segments probe from #1720 now asserts the decode-once contract on the values handlers receive. Both snapshots are byte-identical. 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 (1)
💤 Files with no reviewable changes (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughThe control-plane router migrated from legacy route catalog entries to Hono ChangesRoute module migration
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to The routing migration removes the legacy adapter and mounts Hono route modules directly. Reported checks are clean, and no current merge-blocking risk is identified. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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: #1730, refactor: delete the legacy route adapter
Author: @ColeMurray
Changes: 16 files, +255/-343
This cleanly removes the legacy route adapter now that the catalog consists entirely of Hono modules. Route registration order, admission enforcement, response policy, raw malformed-encoding checks, and decoded handler parameters remain covered by the updated tests.
Critical Issues
None.
Suggestions
None.
Nitpicks
None.
Positive Feedback
- The catalog is now constrained to
readonly RouteModule[], making legacy route reintroduction a compile-time error. assertModuleAdmits()remains at the composition boundary, preserving fail-closed admission enforcement after deleting the adapter.- The replacement tests explicitly cover route contract ordering, policy propagation, malformed encodings, and decode-once behavior.
Questions
None.
Verification
- Control-plane typecheck passed.
- Control-plane ESLint passed.
- All 3,512 control-plane unit tests passed.
- The 9 affected catalog-conformance and admission-matrix integration tests passed.
Verdict
Approve: Ready to merge; no blocking or non-blocking findings.
There was a problem hiding this comment.
The production refactor is a strong structural simplification: it deletes the catalog union and legacy dispatch adapter, leaves a single module-mounting path, preserves admission at the canonical boundary, and reduces the overall diff without pushing any file across the 1k-line threshold. One requested cleanup remains:
[deep review] packages/control-plane/src/routing/hono-app.test.ts:269-272 is now an exact duplicate of the module path-grammar test at lines 247-250: both build a Hono module containing GET /files/* behind admit() and assert the same construction-time error. The second test used to earn its place by exercising the legacy registration path, but that path no longer exists. Keeping both preserves a conceptual distinction this refactor is explicitly deleting and adds maintenance noise without coverage. Remove this test, or replace it only if there is a genuinely different invariant left to exercise.
This finding is top-level because the redundant test's lines are unchanged context and cannot be represented as an inline diff anchor.
Terraform Validation Results
Pushed by: @ColeMurray, Action: |
|
Done in 1fac87d. The second grammar test only existed for the legacy registration path; with that path deleted it was a duplicate of the module test, so it's removed. |
Follow-up promised on #1728: the deep review asked for focused automation modules behind a tiny composition root, and we agreed to land it as a moves-only PR once the Hono series finished (#1730 merged). **Moves only.** No behavior, policy, message, or logic changes; no renames beyond what a move requires. `routes/catalog.ts` still imports `automationRoutes` from `./automations`, which now mounts the modules below in the previous registration order, so route precedence is unchanged. | File | Routes | Lines | |---|---|---| | `automations.ts` | composition root | 24 | | `automation-slack-settings.ts` | 2 | 85 | | `automation-list.ts` | 1 | 149 | | `automation-crud.ts` | 4 | 682 | | `automation-lifecycle.ts` | 3 | 178 | | `automation-runs.ts` | 2 | 64 | | `automation-keys.ts` | 1 | 102 | | `automation-validation.ts` | helpers | 344 | | `automation-shared.ts` | admission | 27 | Shared pieces: `automation-validation.ts` holds the request validation and target-selection helpers that create and update both use; `automation-shared.ts` holds the two `admit()` constants and the admitted-automation accessor. Each module keeps its own `createLogger("router:automations")`, so log output is identical. **Tests** split the same way, one suite per module, each dispatching through its own sub-app via `createTestRequestHandler([module])`: | File | Suites | Lines | |---|---|---| | `automation-list.test.ts` | list | 151 | | `automation-create.test.ts` | create | 760 | | `automation-update.test.ts` | get, update, delete | 759 | | `automation-lifecycle.test.ts` | pause, resume, trigger | 220 | | `automation-runs.test.ts` | invocations, run | 135 | | `automation-keys.test.ts` | regenerate-key | 106 | | `automations.test-support.ts` | store doubles, request builder, sample row, mock defaults | 219 | Create and update are separate files so neither passes 1,000 lines. All 113 tests are kept with their names and intent. `vi.mock` declarations are per file by construction (Vitest hoists them per module), and each suite declares only the mocks its module reaches; the doubles they hand out are shared. ## Verification | Check | Result | |---|---| | Typecheck (src, test, integration) | clean | | ESLint, Prettier | clean | | Unit | 239 files, 3,511 passed (113 automation tests across 6 files) | | Integration (workerd, real D1) | 96 files, 1,129 passed | | Matrix and conformance snapshots | byte-identical | | Largest file in the diff | `automation-create.test.ts`, under 1,000 lines | https://claude.ai/code/session_01KdDpTgGEjXpBA9SaGQVUH1 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added comprehensive automation management, including creation, editing, deletion, pausing, resuming, and manual triggering. * Added automation listing with search, repository filtering, pagination, and recent execution details. * Added access to automation runs and invocation history. * Added webhook and Sentry credential regeneration. * Added Slack channel configuration endpoints. * Added validation for schedules, triggers, targets, providers, environments, permissions, and Slack conditions. * **Refactor** * Organized automation functionality into dedicated areas while preserving the existing automation API. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
1 participant
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()andlegacyMatch()inrouting/hono-app.ts;createControlPlaneApp(modules, host)mounts modules only.RouteDefinition,Route,defineRoute(),defineRoutes(), andextractRepoParams()inroutes/shared.ts.RouteAdmissionPolicyis now an interface overRoutePolicyplusauthorizationandserviceActorClaims;cacheControllives onAdmissionPolicyinrouting/admit.ts.RouteCatalogEntry; the catalog isreadonly RouteModule[].legacyRoutes()in test support;matchRoute()no longer fabricates aRegExpMatchArray.routes/shared.test.ts, replaced byroutes/repository-params.test.tsover 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 refactor: convert the sessions cluster to Hono sub-apps #1724).Tests
Fixtures that need synthetic routes build a
Honomodule and pass it tocreateTestRequestHandler([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 throughrawRouteParams(), and the matrix's "raw path segments" probe from #1720 is now the decode-once assertion on what handlers receive (abc%2Fdefarrives asabc/def,web%252Fappasweb%2Fapp).Verification
https://claude.ai/code/session_01KdDpTgGEjXpBA9SaGQVUH1
Summary by CodeRabbit
Bug Fixes
group/subgroup.Refactor