bake: name both flags at every insert_stale_extra call - #39171
Conversation
insert_stale_extra took two bools, is_ssr_graph and is_route, and three of its four calls passed them as bare literals (`false, true`), so nothing at the call site said which was which. The graph flag becomes a bake::Graph, which is what every caller already holds: the client graph takes Graph::Client, the server graph takes Graph::Server (RSC) or Graph::Ssr, and get_log_for_resolution_failures passes its graph straight through instead of `graph == Graph::Ssr`. A debug assertion checks the graph matches the side. The route flag becomes a two-variant RouteKind, whose doc also records the contract that a Route inserted on the server has to be registered in route_lookup, since trace_dependencies panics otherwise. No behavior change: every call passes the same values it did before, including the client HTML route site, where the route flag is ignored on the client as it always was. mordant-baseline.toml is regenerated. Besides this finding it drops two entries whose findings were already fixed on main after the counts were recorded (PackageInstall.rs by #38271, node_crypto_binding.rs by #37648); everything else is unchanged.
|
Status: ready for review.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
WalkthroughChangesThe incremental graph API now accepts explicit Bake graph routing
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
LGTM — mechanical refactor replacing two bare bool params with typed enums; every call site maps to the same value as before.
What was reviewed:
- Traced all 8
insert_stale/insert_stale_extracall sites: each newbake::Graph/RouteKindargument produces the identicalis_ssr_graph/is_routevalue the old bool did. get_log_for_resolution_failuresserver branch:graph == Ssrderivation moved into the callee, semantically unchanged.bake::Graphhas exactly {Client, Server, Ssr}; the newdebug_assert!correctly guards against passing the wrong side's variant.- mordant-baseline.toml: the two extra dropped entries are already-fixed-on-main findings (#38271, #37648), consistent with a full
rust:mordant:baselineregen.
Extended reasoning...
Overview
This PR replaces two bare bool parameters on IncrementalGraph::insert_stale_extra (is_ssr_graph, is_route) with typed values: the existing three-variant bake::Graph enum and a new two-variant RouteKind enum. The forwarder insert_stale is updated in step, and all 8 call sites in DevServer.rs are converted. mordant-baseline.toml is regenerated, dropping the bare_bool_args entry this fixes plus two entries already resolved on main.
Behavior preservation
I traced every changed call site against the old bool arguments:
client_graph.insert_stale(..., false)→Graph::Client: client arm never readis_ssr_graph.server_graph.insert_stale_extra(..., false, true)(framework router entry,get_file_id_for_router) →(Graph::Server, RouteKind::Route):is_ssr_graph = (Graph::Server == Ssr) = false,is_route = true. Identical.client_graph.insert_stale_extra(..., false, true)(HTML route) →(Graph::Client, RouteKind::Route): client arm ignores both flags. Identical.get_log_for_resolution_failures: the server branch previously passedgraph == bake::Graph::Ssr; now it passesgraphand the callee computesgraph == bake::Graph::Ssrin theSide::Serverarm. Identical. The client branch is guarded bygraph == Graph::Client, so passinggraphthrough satisfies the new debug_assert.
The added debug_assert! matching SIDE against the passed Graph variant is a strict tightening (compiles out in release) and cannot fire on any existing call site. bake::Graph (bundle_v2.rs:362) has exactly the three variants the assert covers.
Security risks
None. This is a type-level refactor of internal dev-server graph bookkeeping with no user-facing input handling, no I/O, and no change to control flow.
Level of scrutiny
Low-to-medium. The change is mechanical and self-contained to the bake dev server. The author ran the full relevant bake test suite (80 tests across 8 files covering every changed call site), verified mordant is clean with a control run, and clippy is clean. The PR description is thorough and accurately describes what was done and why the extra baseline entries drop.
Other factors
- No prior reviews or outstanding comments on the PR.
- The two incidental
mordant-baseline.tomldeletions are explained with linked PRs and are the expected side effect of a full baseline regen; #39151 in flight makes the same drops. - The new
RouteKinddoc comment records the previously-implicit contract that serverRouteinsertions must be registered inroute_lookup, which is a nice clarity win with no code effect.
There was a problem hiding this comment.
I reviewed this PR and found no bugs — every insert_stale/insert_stale_extra call site passes the same effective values as before (Graph::Server/Graph::Client → is_ssr_graph = false, Graph::Ssr → true; RouteKind maps 1:1 to the old bool), and the new debug_assert! holds at all seven callers. Deferring only because the two comment-cop threads on the added doc comments are still open; a maintainer should decide whether to resolve them (they document the route_lookup registration invariant, not a workaround) or trim them.
What was reviewed:
- All 4
insert_staleand 3insert_stale_extracallers traced against the old bool values — no semantic drift. get_log_for_resolution_failureselse-branch:graphis already!= Clientthere, so the debug_assert holds andis_ssr_graphderivation matches the oldgraph == bake::Graph::Ssr.bake::Graphconfirmed three-variant (Client/Server/Ssr); client arm never readsis_ssr_graph, so moving its computation into the server arm is safe.- mordant-baseline.toml diff drops only the one fixed entry.
Extended reasoning...
Overview
This PR replaces two bare bool parameters on IncrementalGraph::insert_stale_extra with typed values: is_ssr_graph: bool becomes graph: bake::Graph (the existing three-variant Client/Server/Ssr enum), and is_route: bool becomes a new two-variant RouteKind enum. The forwarder insert_stale and all seven call sites in DevServer.rs are updated. A debug_assert! guards that the passed Graph matches the const-generic SIDE of the graph. The mordant baseline entry for the fixed bare_bool_args finding is removed.
Behavior preservation
I traced each caller against the old signature. The three direct insert_stale_extra calls all previously passed (false, true); they now pass (Graph::Server, Route) or (Graph::Client, Route), and since is_ssr_graph is now derived as graph == Graph::Ssr inside the server arm (and ignored on the client arm), the effective values are identical. The four insert_stale callers previously passed false (client) or graph == bake::Graph::Ssr (server resolution-failure path); passing graph through and deriving inside yields the same result. The debug_assert! holds at every call site: client callers pass Graph::Client, server callers pass Server or Ssr, and the resolution-failure else-branch is guarded by graph != Client.
Security risks
None. This is a type-level refactor of an internal dev-server graph API with no user-facing surface, no parsing of external input, and no change to control flow in release builds.
Level of scrutiny
Low-to-medium. The change is mechanical and self-contained to the bake dev server, with a well-argued PR description, passing bake test suite (80 tests), and a control run showing the mordant lint fires without the source change. The only new runtime code is a debug_assert! that compiles out in release.
Other factors / why deferring
The comment-cop bot left two unresolved inline threads on the new doc comments (the RouteKind doc and the insert_stale_extra graph parameter doc). The bot is a blunt heuristic that flags any 2+ line comment block on claude-labeled PRs and does not fail CI. The flagged comments document a real invariant — that RouteKind::Route on the server obliges the caller to register in route_lookup, which trace_dependencies otherwise panics on — so they read as legitimate API documentation rather than workaround justification. But whether to keep or trim them is a repo-style call a maintainer should make, and I don't want to approve over open review threads.
The graph parameter's meaning is already spelled out by the debug assertion below it and by how the server arm reads it, so that paragraph goes; RouteKind keeps the one fact the code does not show.
There was a problem hiding this comment.
LGTM — mechanical bool→enum refactor with no behavior change.
What was reviewed:
- Traced all six
insert_stale/insert_stale_extracall sites: each passes the same effectiveis_ssr_graph/is_routevalues as before;bake::Graphhas exactly {Client, Server, Ssr} so the newdebug_assertandgraph == Ssrderivation are exhaustive. - Confirmed the client-side arm never reads
graphorroutebeyond the assert, matching the old behavior of ignoring both bools on the client. - The comment-cop notices fired on the pre-ff1ceeb0 doc; that commit trimmed
RouteKind's doc to one line, so they're addressed.
Extended reasoning...
Overview
This PR replaces two positional bool parameters on IncrementalGraph::insert_stale_extra (is_ssr_graph, is_route) with typed values: the existing three-variant bake::Graph enum and a new two-variant RouteKind enum. The forwarder insert_stale and all six call sites in DevServer.rs are updated, a debug_assert guards against passing a client graph value to the server-side graph or vice versa, and the corresponding bare_bool_args entry is dropped from mordant-baseline.toml.
Behavior preservation
I traced each call site against the old arguments. Framework router entry (Graph::Server, Route ← false, true), react-refresh and framework client entry (Graph::Client ← false, ignored on client), get_log_for_resolution_failures (passes graph through; the server arm now computes is_ssr_graph = graph == Ssr, identical to the old graph == bake::Graph::Ssr at the call site), HTML route bundle (Graph::Client, Route ← false, true, both still ignored on the client arm), and get_file_id_for_router (Graph::Server, Route ← false, true). bake::Graph has exactly three variants (bundle_v2.rs:362), so the debug_assert partition and the == Ssr derivation are exhaustive. No path changes semantics.
Security risks
None. This is a type-level refactor of internal dev-server bookkeeping with no user-facing input handling, no unsafe blocks added, and no change to control flow in release builds (the only new runtime code is a debug_assert).
Level of scrutiny
Low-to-medium. The change is mechanical and self-contained to the bake dev server, follows an established pattern in this repo (several recent PRs in the git log do the same bool→enum conversion for other mordant bare_bool_args findings), and the removed baseline entry is the regression guard. The author ran the full bake dev test suite (80 tests) plus mordant/clippy, and did a control run confirming mordant re-flags with the source reverted.
Other factors
The two comment-cop bot comments (posted 19:31 UTC) predate the response commit ff1ceeb ("trim the RouteKind doc to the route_lookup obligation", built ~19:38 UTC per the updated robobun comment), which reduced the RouteKind doc to the single line now in the diff — so those are addressed. No prior review from me on this PR. No outstanding human reviewer comments.
Problem
IncrementalGraph::insert_stale_extra(src/runtime/bake/dev_server/incremental_graph.rs:1286) takes two bools,is_ssr_graphandis_route, and three of its four calls pass them as bare literals:insert_stale_extra(path, false, true)at DevServer.rs:952, :5229 and :5850. Nothing at those sites says which flag is which, and the swapped call compiles.bare_bool_args(thebare_bool_args:src/runtime/bake/dev_server/incremental_graph.rsentry inmordant-baseline.toml).Fix
bake::Graph. Every caller already holds one: the client graph passesGraph::Client, the server graph passesGraph::Server(the RSC graph) orGraph::Ssr, andget_log_for_resolution_failurespasses itsgraphstraight through instead ofgraph == bake::Graph::Ssr. Inside, the server arm derivesis_ssr_graphfrom it, so the body is otherwise unchanged. Adebug_assert!checks the graph passed matches the side of the graph it is inserted into, since a three-variant enum could otherwise be handed to the wrong side.incremental_graph::RouteKind { NotRoute, Route }(same shape asBakeRouteKindin src/bundler/OutputFile.rs). Its one-line doc records the contract the bool left implicit: aRouteinserted on the server must also be registered inDevServer::route_lookup, becausetrace_dependenciespanics on a route it cannot find there. Both server callers already do this right after the insert.insert_stalestays a forwarder and now passesRouteKind::NotRoute; its four callers take thebake::Graphchange.Routeeven though the client graph ignores the flag (it always has; HTML routes are found throughhtml_route_bundle_index). Dropping it there would have been a judgment call outside this change.mordant-baseline.tomlis regenerated withbun run rust:mordant:baseline; against current main the only difference is this finding's entry. (The first revision also dropped two entries for findings fixed on main after ci: bump the mordant pin (sixteen new lints) and record their baseline #38846 recorded the counts; main has since removed those itself, and merging main in folded that away.)mordantjob fails on any future call that passes bare bools here again (the control run below is that failure, produced on purpose).bun bd teston test/bake/dev/{html,esm,bundle,incremental-graph-edge-deletion,react-response,react-spa,ssg-pages-router,server-sourcemap}.test.ts: 80 pass, 0 fail. These cover every changed call site (framework router setup andget_file_id_for_router, react refresh, HTML route bundles, resolution failures on both graphs, and the route hot-reload path that relies onis_route).bun run rust:mordanton this branch: no warnings, notarget/mordant/over-baseline.txt.bun_runtime 1over baseline.cargo clippy -p bun_runtime --no-deps: clean.Background
IncrementalGraphs, one perbake::Side(client and server). The server graph holds files for two bundler graphs at once:Graph::Server, the RSC graph where routes and server components live, andGraph::Ssr, the copies of client components bundled for rendering on the server. A serverFilerecords which of the two it belongs to inis_rsc/is_ssr; a file can be in both.route_lookupmaps a server file index to the framework route it belongs to.trace_dependenciesusesFile::is_routeto decide whether to consult it when a change propagates to that file, which is why theRouteflag carries a registration obligation.rust-lintsworkflow;mordant-baseline.tomlholds per-(lint, file) counts of pre-existing findings, and a PR fails the job only if it adds one. Removing a fixed finding's entry keeps it from coming back.