refactor(dashboard): move the route table into ordered slices - #3160
Conversation
github-actions
Bot
added
the
readiness: checking
GPT 5.6 Review — ✅ human override acceptedHuman judgment by @CrysisDeu overrides the GPT 5.6 finding for This comment is updated in place on each push. The model was not re-run because an authorized human decision supersedes it. False positive or not applicable? A repository writer can comment: |
Design Review (Fable 5) — ✅ PASSAdvisory design-level review of Design-Verdict: PASS A mechanical, order-preserving extraction that turns an undocumented load-bearing invariant into an enforced one — root cause fixed, no new surface, fully reversible. [DESIGN-REVIEWED] f72ff78 |
github-actions
Bot
added
readiness: action required
Opus 4.8 Review — ✅ no blocking findingsReviewed Review detailsThe candidate file contains no candidates — the discovery pass found nothing. My role is strictly a filter (Step 2: I may not add findings of my own). With no candidates to falsify, there are no survivors. No findings. [OPUS-REVIEWED] f72ff78 Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
641ae51 to
49227d9
Compare
github-actions
Bot
added
readiness: checking
start_dashboard carried all 447 route registrations inline, 1,889 lines of a
4,180-line module, of which 501 of 594 statements were bare registration calls.
They now live in dashboard/routes/, one module per section of the original
table, each exposing register(app). server.py drops to 3,463 lines and
start_dashboard to 1,218.
The registrations were already contiguous (L2399-3074, no gap over 25 lines)
and captured exactly one local, `app`; the four other names they referenced are
each defined and used within the block. Everything else resolved to a
module-level handler import, so each slice imports from the true origin and no
slice imports back from server.py -- there is no cycle.
Ordering is the property this change had to protect. aiohttp resolves a request
against its routes in REGISTRATION order, 397 of the 1,306 runtime routes carry
a pattern segment, and the table depends on that in several places: a literal
path is deliberately registered before a pattern that would swallow it, and the
inline comments said so. The slices are therefore contiguous cuts at the
author's own section boundaries, called in the original sequence, so global
order is preserved by construction rather than by per-pair reasoning.
Verified behaviourally, not structurally: the complete runtime route set --
1,306 entries of method, path, resolved handler, in router order -- is
byte-identical before and after, captured by building the real app through the
suite's own harness. The 41 imports the move left dead in server.py were
removed only after checking none is a name a test rebinds on the module.
test_dashboard_route_table.py adds the guards the split makes necessary:
- the registrar tuple is explicit and not alphabetical
- one test builds the live router ONCE and checks all three ordering properties
against it: the EFFECTIVE registration order matches the tuple, no literal is
shadowed by an earlier matching pattern, and no pattern is shadowed by a
DIFFERENT earlier pattern. One build rather than three, since _dashboard runs
a full startup per use and the router is identical in all three cases
- every slice exposes register(), and the files on disk match the registrar list
The tuple pin alone cannot fail when register_all's iteration changes, which a
mutation confirmed -- reversing the loop left the tuple untouched and every test
green until the live-router check existed. Both order failure modes are
mutation-verified. The pattern-vs-pattern case is the third way to break
ordering and the likeliest future mistake: a new /api/chat/slots/{x}/... variant
added to the wrong slice would take its sibling's traffic; it is flagged only
when the two resolve to different handlers, since the table legitimately points
several methods of one pattern at one handler.
The shadowing checks read each resource's own compiled pattern instead of
rebuilding one from `canonical`, which drops a pattern's inner regex and would
report a four-file allowlist as a catch-all, and they use fullmatch, since a
prefix match reports `/api/channels/{id}` as shadowing
`/api/channels/weixin/qr/status`. Ownership is defined as "a slice registers
this path" rather than by the handler's module, because the table registers
/api/suggestions and /api/tips/* whose handlers live outside dashboard/ -- and
filtering on the handler module would have silently excluded exactly the routes
the check exists for.
It also surfaced a pre-existing overlap this change does not introduce or fix:
the app platform's /api/apps/{name}/config is registered before the literal
/config routes of the builtin apps whose own API base is two segments deep, so
the generic handler answers instead of theirs and skips their enabled-gating.
The check is scoped to paths the dashboard table owns and the finding is called
out in its docstring rather than silently allowlisted.
Two tests scanned start_dashboard's source for routes and for the
sso_login_handler seam; both now scan the slices too, so they hold wherever a
route lives. Two more that scan for a specific route line were widened the same
way.
49227d9 to
f72ff78
Compare
github-actions
Bot
added
readiness: action required
|
Rebutted — the eager import of I traced the first import of
Measured cost of
The branch is marginally faster, i.e. within noise. There is no readiness delay to remove because there is no added import. On the suggested fix specifically: making slice loading lazy inside One correction to the report's premise: Happy to be shown otherwise if there is a path to |
|
/ai-review override gpt f72ff78: apps.routes is already imported at module scope by dashboard/handlers/security.py on main, so this PR adds no eager import — traced identical chains and measured 823ms vs 836ms for |
Human judgment recorded@CrysisDeu marked the gpt AI finding as false positive, not applicable, or explicitly accepted for
This decision applies only to this commit. A new push requires a new judgment. |
github-actions
Bot
added
readiness: checking
Review record at
|
bolichen97
left a comment
bolichen97
left a comment
There was a problem hiding this comment.
Tier 1 auto-approve: refactor (19 files). Criteria: no conflict, no requested changes, no security surface, AI reviewers green. Category: route table reorganization — mechanical code motion, no behavior change.
github-actions
Bot
removed
the
readiness: passed
…tdev#3160) start_dashboard carried all 447 route registrations inline, 1,889 lines of a 4,180-line module, of which 501 of 594 statements were bare registration calls. They now live in dashboard/routes/, one module per section of the original table, each exposing register(app). server.py drops to 3,463 lines and start_dashboard to 1,218. The registrations were already contiguous (L2399-3074, no gap over 25 lines) and captured exactly one local, `app`; the four other names they referenced are each defined and used within the block. Everything else resolved to a module-level handler import, so each slice imports from the true origin and no slice imports back from server.py -- there is no cycle. Ordering is the property this change had to protect. aiohttp resolves a request against its routes in REGISTRATION order, 397 of the 1,306 runtime routes carry a pattern segment, and the table depends on that in several places: a literal path is deliberately registered before a pattern that would swallow it, and the inline comments said so. The slices are therefore contiguous cuts at the author's own section boundaries, called in the original sequence, so global order is preserved by construction rather than by per-pair reasoning. Verified behaviourally, not structurally: the complete runtime route set -- 1,306 entries of method, path, resolved handler, in router order -- is byte-identical before and after, captured by building the real app through the suite's own harness. The 41 imports the move left dead in server.py were removed only after checking none is a name a test rebinds on the module. test_dashboard_route_table.py adds the guards the split makes necessary: - the registrar tuple is explicit and not alphabetical - one test builds the live router ONCE and checks all three ordering properties against it: the EFFECTIVE registration order matches the tuple, no literal is shadowed by an earlier matching pattern, and no pattern is shadowed by a DIFFERENT earlier pattern. One build rather than three, since _dashboard runs a full startup per use and the router is identical in all three cases - every slice exposes register(), and the files on disk match the registrar list The tuple pin alone cannot fail when register_all's iteration changes, which a mutation confirmed -- reversing the loop left the tuple untouched and every test green until the live-router check existed. Both order failure modes are mutation-verified. The pattern-vs-pattern case is the third way to break ordering and the likeliest future mistake: a new /api/chat/slots/{x}/... variant added to the wrong slice would take its sibling's traffic; it is flagged only when the two resolve to different handlers, since the table legitimately points several methods of one pattern at one handler. The shadowing checks read each resource's own compiled pattern instead of rebuilding one from `canonical`, which drops a pattern's inner regex and would report a four-file allowlist as a catch-all, and they use fullmatch, since a prefix match reports `/api/channels/{id}` as shadowing `/api/channels/weixin/qr/status`. Ownership is defined as "a slice registers this path" rather than by the handler's module, because the table registers /api/suggestions and /api/tips/* whose handlers live outside dashboard/ -- and filtering on the handler module would have silently excluded exactly the routes the check exists for. It also surfaced a pre-existing overlap this change does not introduce or fix: the app platform's /api/apps/{name}/config is registered before the literal /config routes of the builtin apps whose own API base is two segments deep, so the generic handler answers instead of theirs and skips their enabled-gating. The check is scoped to paths the dashboard table owns and the finding is called out in its docstring rather than silently allowlisted. Two tests scanned start_dashboard's source for routes and for the sso_login_handler seam; both now scan the slices too, so they hold wherever a route lives. Two more that scan for a specific route line were widened the same way. Co-authored-by: t <t@t>
2 participants
Problem
start_dashboardcarried all 447 HTTP route registrations inline: 1,889 lines ofa 4,180-line module, in which 501 of 594 statements were bare
app.router.add_*calls. Finding where a path is served meant scrolling a676-line wall of registrations, and the function that binds the port, builds
middleware, starts services and wires shutdown hooks was mostly a route table
written longhand.
Why it matters
The registration order in that block is load-bearing and undocumented outside
comments. aiohttp resolves a request against its routes in registration order,
397 of the 1,306 runtime routes carry a pattern segment, and several literals are
deliberately registered before a pattern that would swallow them — the inline
comments said so:
Nothing enforced any of it. A route added in the wrong place becomes silently
unreachable: the pattern matches first and its handler answers a path it was
never meant to serve, so the symptom is a wrong response body or a 404 for a path
that is still in the table.
Fix (symptom → root cause → change)
The root cause is that the table had no home of its own, so it grew inside the
startup function. It now lives in
dashboard/routes/, one module per section ofthe original table, each exposing
register(app), invoked byregister_all().server.pydrops to 3,463 lines andstart_dashboardto 1,218.The move was cheap because the block was already well-formed: the registrations
were contiguous (L2399–3074, no gap over 25 lines) and captured exactly one local,
app. The four other names they referenced are each defined and used inside theblock. Everything else resolved to a module-level handler import, so each slice
imports from the true origin and no slice imports back from
server.py—there is no cycle, verified in all four import orders in separate interpreters.
Ordering is the property the change had to protect, so the slices are
contiguous cuts at the author's own section boundaries, called in the original
sequence. Global order is then preserved by construction rather than by
per-pair reasoning.
register_all(app)also sits at the same position relative to_register_mcp_routes(app)as the old block, so paths registered in both places(
/api/notifications) keep their resolution order.Tests
test/test_dashboard_route_table.py(new) pins what nothing checked before:register, and the files on disk match the registrar listTwo details worth the reviewer's attention, both found by the tests misbehaving:
register_all's iteration toreversed(...)left the tuple untouched and everytest green. That is why the effective-order check against the live router
exists. Both order failure modes are now mutation-verified, and each is caught
by a different test.
rebuilt from
canonical—canonicaldrops a pattern's inner regex, rendering/{name:manifest\.json|sw\.js|icon-\d+\.png|pcm-worklet\.js}as bare/{name}, which reports a four-file allowlist as a catch-all. They also usefullmatch, because a prefix match reports/api/channels/{id}as shadowing/api/channels/weixin/qr/status.Ownership is defined as "a slice registers this path", not by the handler's
module: the table registers
/api/suggestionsand/api/tips/*whose handlerslive outside
dashboard/, and filtering on the handler module would havesilently excluded exactly the routes the check exists for.
Four existing tests scanned
server.py's source for a route line or for thesso_login_handlerseam; each now scans the slices too, so they hold wherever aroute lives.
Manual verification
The complete runtime route set is byte-identical — 1,306 entries of method,
path, resolved handler, in router order — captured by building the real app
through the suite's own
_dashboardharness and diffed againstorigin/main.Deliberately unsorted: a sorted snapshot would prove the same set of routes
exists while hiding a shadowing regression, which is the actual risk of moving
registrations between modules. Re-verified after the rebase and after pruning
imports, since a wrongly-removed import would break a registration.
Also verified: all four import orders (slice first,
serverfirst, package-onlythen dispatch, every slice standalone); and that the 41 imports the move left dead
in
server.pyare dead — checked by intersection against every name any testrebinds on the module, not by eye.
Full suite: 49,742 pass. Compared against a same-commit baseline, the delta
was 7 candidate failures; 2 were real and are fixed here (both source-text scans
of
server.py), and 5 are environmental — 4 pass in isolation and the fifth is adifferent test in that class asserting on inherited environment variables.
Screenshots
N/A — no user-visible surface changes. Every route resolves to the same handler as
before, in the same order.
One cost a maintainer should decide knowingly
I scanned all 195 open PRs (
gh pr listdefaults to 30, which would havechecked a sixth of them). 20 of them add route lines to the exact block this PR
removes. None rewrites the file — the largest is #2713 at +54/−1 — so none can
re-home this work, and the file-overlap gate clears. But merging this will make
all 20 conflict. Each conflict is trivial (move the added line into the right
slice) and it is a one-time cost that permanently stops the file growing, but it
is 20 branches of friction and worth timing deliberately.
Local adversarial review
Two model-pinned reviewers mirroring the CI lanes (
gpt-5.6-sol/codex-review.yml,claude-opus-4.8/claude-review.yml+ base-refAUTOSDE.yaml) ran before this was pushed. Both returned no blockingfindings. Opus verified the claim rather than accepting it — reconstructed the
declared slice order as byte-for-byte the original sequence, confirmed the App
Platform tail is preserved verbatim, and re-checked all 41 import removals with
named explanations for the three greps that look like hits but are comments and a
CSP literal.
Its three advisory findings are all fixed in this commit:
excluded
/api/suggestionsand/api/tips/*, whose handlers live outsidedashboard/. A real hole in a test written to prevent that class. Ownership isnow "a slice registers this path".
ordering, and the likeliest future mistake. Added as its own check, flagging
only overlaps that resolve to different handlers, since the table
legitimately points several methods of one pattern at one handler.
system.py'sfrom kiro_crew.apps.routes import register_app_routeswas a verbatim move.Tested for a cycle in both import orders, found none, so it is hoisted to
module scope rather than annotated.
Its fourth item is informational and accepted-and-deferred: it independently
confirmed the
/api/apps/{name}/configoverlap is real for the apps whose APIbase is two segments deep (and that
crew_companion's three-segment/reminders/configis not affected), and judged that scoping the guard anddocumenting it is the right disposition, since failing on a pre-existing defect
would block an unrelated pure refactor. It belongs in its own PR against
apps/routes.py.