ci: bump the mordant pin (sixteen new lints) and record their baseline - #38846
Merged
Merged
Conversation
Contributor
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 42 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
LGTM — mechanical mordant pin bump plus regenerated baseline; the job is advisory (continue-on-error) so no risk to other PRs.
What was reviewed
Cargo.toml: only the[workspace.metadata.dylint]rev changes; the workflow's sed-based cache key extractor still matches.mordant-baseline.toml: existing entries preserved, new per-(lint, file) counts added in the documented"lint:path" = Nshape under per-crate tables.- Confirmed this follows the documented bump procedure in
.github/workflows/CLAUDE.mdand touches no compiled code.
Extended reasoning...
Overview
The PR bumps the pinned git revision of the mordant dylint lint pack in Cargo.toml's [workspace.metadata.dylint] section from 3bcf116… to 4efc663…, and regenerates mordant-baseline.toml to record the pre-existing hit counts for the sixteen lints the new revision adds. No source, build script, or workflow file is touched.
Security risks
None. The dylint metadata is consumed only by cargo dylint inside the mordant CI job (and locally via bun run rust:mordant); it is not part of the shipped binary's dependency graph. The baseline file is a TOML table of integers read only by scripts/rust-mordant.ts.
Level of scrutiny
Low. This is CI tooling configuration with no effect on the compiled product. The mordant workflow itself is continue-on-error: true, so even a mis-recorded baseline count cannot block merges elsewhere — the worst case is the advisory job going red on this PR, which CI would surface. The change matches the repo's own documented procedure for bumping mordant (change the rev, regenerate the baseline, put triage in the description).
Other factors
- Verified the workflow's
sedextraction of the rev (.github/workflows/mordant.yml:85) still matches the unchanged line format, so the driver/library cache key updates correctly. - The diff to
mordant-baseline.tomlis purely additive to existing sections plus new crate sections; no prior counts were lowered or removed, so no existing suppression is silently widened. - No CODEOWNERS-sensitive paths, no outstanding reviewer comments (only a CodeRabbit rate-limit notice).
alii
added a commit
that referenced
this pull request
Aug 15, 2026
…e crate (#38852) The mordant job went red on main after #38846 for two reasons that this fixes together. The workspace denies warnings, so a finding that is not in mordant-baseline.toml is a hard error: the crate stops compiling, every crate after it is never linted, and the log shows one finding instead of all of them. rust:mordant now caps lints at warn and fails the run itself at the end if any finding was printed, with a line saying how to regenerate the baseline. Same red/green outcome, complete report. The one finding that tripped it is Linux-only: dependent_field on RemoveFileParent in the shell's rm builtin, whose fields differ under cfg (allow_enqueue does not exist on Linux, so `enqueued` reads as depending on `treat_as_dir` alone there). The baseline was generated on macOS and did not have it; added.
This was referenced Aug 15, 2026
Merged
alii
added a commit
that referenced
this pull request
Aug 15, 2026
### Problem - mordant's `arg_named_like_other_param` reports `src/ast/lib.rs:1651`, in `Log::add_resolve_error_with_level`: the local `text` is passed as `BabyString::r#in`'s `parent` parameter, and `r#in` also has a parameter named `text` of the same type (`&[u8]`), so the call reads as if its arguments were swapped. - The call is not swapped. `BabyString::r#in(parent, text)` (`src/ast/lib.rs:1080`) searches for `text` inside `parent` and records the offset and length it found there; the caller wants the specifier located inside the formatted message, which is what `r#in(&text, specifier_arg)` does. The other two callers (`src/jsc/VirtualMachine.rs:4698`, `src/runtime/jsc_hooks.rs:5496`) pass the same order. Only `r#in`'s parameter names are misleading: its `text` is the needle, while every caller's `text` is the haystack. ### Fix - Rename `r#in`'s parameters to `container` and `substring`. `container` is the name `BabyString::slice` already uses for the same string (a `BabyString` only means something against the string it was built from), and `substring` says what the second argument is. Callers are unchanged. No behavior change: the body is the same code with the names substituted. - `mordant-baseline.toml` regenerated with `bun run rust:mordant:baseline`. It drops the `arg_named_like_other_param:src/ast/lib.rs` entry this change fixes, plus two entries that were already stale on main because the findings were removed by changes that landed after the baseline was recorded in #38846: `always_unwrapped_option:src/install/PackageInstall.rs` (the `Option<Walker>` removed in #38271) and `narrowed_two_ways:src/runtime/node/node_crypto_binding.rs` (the key length made `usize` in #37648). Happy to drop those two hunks if you would rather keep this to the one line. - Verified: - `bun run rust:mordant` (cargo-dylint 6.0.3, the mordant revision pinned in `Cargo.toml`) over the whole workspace: no findings and no `target/mordant/over-baseline.txt` (the CI gate), both against the baseline on main and against the regenerated one. - `bun bd test test/js/bun/resolve/resolve-error.test.ts test/js/node/missing-module.test.js test/js/bun/resolve/import-meta.test.js test/js/bun/resolve/import-meta-resolve.test.mjs test/regression/issue/29264.test.ts`: 71 pass. `resolve-error.test.ts` reads `.specifier` off runtime `ResolveMessage`s and off `Bun.build` logs, which is the value `r#in` computes; `import-meta-resolve.test.mjs` covers the empty-specifier branch. - `bun bd test test/js/bun/http/serve.test.ts -t "dev error page"`: 2 pass (the dev error page is the other reader of the stored offset). - No new test: Rust parameter names are not visible to callers, so no test can tell this change apart from main. The check for it is the `mordant` job in the Rust lints workflow, which runs against the regenerated baseline. ### Background - `BabyString` (`src/ast/lib.rs`) packs a 16-bit offset and a 16-bit length into a `u32`. A resolve error stores its specifier this way, as a position inside the message's own text, instead of keeping a second copy of the string; `BabyString::r#in` builds it and `BabyString::slice` reads it back (`src/jsc/ResolveMessage.rs`, `src/runtime/server/DevErrorPage.rs`). The method is spelled `r#in` because `in` is a keyword; the name is carried over from the logger this was ported from. - mordant is the lint pack `bun run rust:mordant` runs in the Rust lints workflow. `mordant-baseline.toml` is a ratchet: per (lint, file) counts of the findings that predate the job, so a PR fails only when it adds one. Fixing a finding means regenerating the file so its entry disappears, which is the second hunk here. Co-authored-by: Alistair Smith <hi@alistair.sh>
alii
added a commit
that referenced
this pull request
Aug 15, 2026
…er the dependency (#39127) ### Problem - mordant's `arg_named_like_other_param` flags `src/install/resolution.rs:34`: `version_buf` is passed as `Group::satisfies`'s `group_buf` parameter, and `satisfies` also has a `version_buf` parameter of the same type (`satisfies(version, group_buf, version_buf)` in `src/semver/SemverQuery.rs:556`). - The call is not transposed. In `Resolution::satisfies_dependency_version`, `version` is a `dependency::Version` (the declared range, which is the group), so its buffer belongs in `group_buf`; `self` is the resolution holding the concrete version, so `resolution_buf` belongs in `satisfies`'s `version_buf`. The git and github arms agree (`self` is `lhs` with `resolution_buf`, the dependency is `rhs` with its own buffer). The two vocabularies just collide on the word `version`. - All three callers (`PackageManagerEnqueue.rs:2915`, `:2933`, `lockfile/bun.lock.rs:3414`) pass the same lockfile string buffer in both slots, so a swap would not have been observable there either way. ### Fix - Rename the parameters to `dep_version` / `dep_version_buf`. `resolution_buf` is unchanged. No behavior change, so no new test. - Regenerate the `[bun_install]` section of `mordant-baseline.toml` (`MORDANT_BASELINE_WRITE=1 cargo dylint --all -p bun_install --no-deps`). Besides the cleared `arg_named_like_other_param:src/install/resolution.rs` entry, this drops `always_unwrapped_option:src/install/PackageInstall.rs`: the `walker: Option<Walker>` field it counted was removed in #38271, which landed after the baseline was recorded in #38846. - Verified: - `cargo dylint --all -p bun_install --no-deps` (the pinned mordant) with this baseline: no findings. With the rename reverted and the same baseline, it reports exactly the `resolution.rs:34` finding as over the baseline, so the baseline edit and the rename match. - `bun bd` builds; the three callers' paths pass on it: `bun bd test test/cli/install/test-dev-peer-dependency-priority.test.ts` (4/4), `test/cli/install/bun-lock.test.ts` (40/40, peer resolution while loading bun.lock), `test/cli/install/bun-install-patch.test.ts` (20/20, patched package lookup). ### Background - `Group::satisfies(version, group_buf, version_buf)` tests a concrete semver version against a parsed range (the "group"). Both sides store their string pieces (prerelease and build tags) as offsets into a byte buffer, which is why each side passes the buffer that backs it. - `mordant-baseline.toml` is the ratchet for the mordant lint pack: it records the per-(lint, file) finding counts that predate the job, so CI fails only on new findings. Fixing a site means its entry goes away; entries whose findings were fixed without touching the file (like the `PackageInstall.rs` one) are harmless but stale until the section is regenerated. Co-authored-by: Alistair Smith <hi@alistair.sh>
alii
added a commit
that referenced
this pull request
Aug 15, 2026
### Problem
- `IncrementalGraph::insert_stale_extra`
(src/runtime/bake/dev_server/incremental_graph.rs:1286) takes two bools,
`is_ssr_graph` and `is_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.
- mordant reports it as `bare_bool_args` (the
`bare_bool_args:src/runtime/bake/dev_server/incremental_graph.rs` entry
in `mordant-baseline.toml`).
### Fix
- The graph flag becomes a `bake::Graph`. Every caller already holds
one: the client graph passes `Graph::Client`, the server graph passes
`Graph::Server` (the RSC graph) or `Graph::Ssr`, and
`get_log_for_resolution_failures` passes its `graph` straight through
instead of `graph == bake::Graph::Ssr`. Inside, the server arm derives
`is_ssr_graph` from it, so the body is otherwise unchanged. A
`debug_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.
- The route flag becomes a two-variant `incremental_graph::RouteKind {
NotRoute, Route }` (same shape as `BakeRouteKind` in
src/bundler/OutputFile.rs). Its one-line doc records the contract the
bool left implicit: a `Route` inserted on the server must also be
registered in `DevServer::route_lookup`, because `trace_dependencies`
panics on a route it cannot find there. Both server callers already do
this right after the insert.
- `insert_stale` stays a forwarder and now passes `RouteKind::NotRoute`;
its four callers take the `bake::Graph` change.
- No behavior change: every call passes the same values as before. That
includes the client HTML route at DevServer.rs:5229, which still passes
`Route` even though the client graph ignores the flag (it always has;
HTML routes are found through `html_route_bundle_index`). Dropping it
there would have been a judgment call outside this change.
- `mordant-baseline.toml` is regenerated with `bun 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 #38846 recorded the counts; main has since
removed those itself, and merging main in folded that away.)
- No new test under test/: nothing observable changed, so a JS-level
test would pass with and without this diff. The regression guard is the
removed baseline entry itself: with it gone, the `mordant` job fails on
any future call that passes bare bools here again (the control run below
is that failure, produced on purpose).
- Verified:
- `bun bd test` on
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 and `get_file_id_for_router`, react refresh, HTML route bundles,
resolution failures on both graphs, and the route hot-reload path that
relies on `is_route`).
- After merging main (which brought in #39151's changes to the same
file), html, bundle and incremental-graph-edge-deletion re-run on the
debug build: 32 pass, 0 fail.
- `bun run rust:mordant` on this branch: no warnings, no
`target/mordant/over-baseline.txt`.
- Control: the same run with the two source files reverted (baseline
entry still removed) reports exactly the finding above, `bun_runtime 1`
over baseline.
- `cargo clippy -p bun_runtime --no-deps`: clean.
### Background
- The dev server keeps two `IncrementalGraph`s, one per `bake::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, and `Graph::Ssr`, the copies of client components
bundled for rendering on the server. A server `File` records which of
the two it belongs to in `is_rsc` / `is_ssr`; a file can be in both.
- "Stale" insertion registers a path in the graph without bundled
content so it can be traced and queued for bundling; routes, framework
entry points and files that failed to resolve all enter this way.
- `route_lookup` maps a server file index to the framework route it
belongs to. `trace_dependencies` uses `File::is_route` to decide whether
to consult it when a change propagates to that file, which is why the
`Route` flag carries a registration obligation.
- mordant is the advisory lint pack run by the `rust-lints` workflow;
`mordant-baseline.toml` holds 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.
---------
Co-authored-by: Alistair Smith <hi@alistair.sh>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Moves the pinned mordant revision to 4efc663, which adds sixteen lints (duplicated matches and helpers, parallel vecs, bool parameter runs, integer aliases used interchangeably, a Result collapsed to a bool and dropped, the same place narrowed two ways, and a few more; scarletindustries/mordant#12 has the list). mordant-baseline.toml is regenerated against main so the job stays advisory and only reports instances a change adds; the recorded counts are what exists today (largest: 81 same_match_twice, 38 collapsed_error, 20 bool_params). #38841 already removes a good share of the same_match_twice and reimplemented_helper entries and can shrink the file when it lands.
Nothing else changes: same nightly, same dylint version, so the workflow's caches key over on the rev alone.