Repository navigation
Conversation
|
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)
Included review availability: 1 review is currently available. Your included PR review attempts over the past 7 days set your current allowance at 2 reviews per hour. WalkthroughChangesMordant Dylint commands now pass Mordant lint targeting
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
LGTM — a one-flag swap in an advisory CI job, applied consistently across the workflow, package.json scripts, and docs.
What was reviewed:
- Confirmed
[workspace.metadata.dylint]inCargo.tomlpoints atscarletindustries/mordant, so--lib mordantresolves the same library--alldid. - Checked no stray
dylint --allremains; all three invocations and the CLAUDE.md table/notes are updated in lockstep. - The job is
continue-on-error: trueand this PR's own paths trigger it, so the happy path is exercised here.
Extended reasoning...
Overview
This PR changes the mordant dylint invocation from cargo dylint --all to cargo dylint --lib mordant in three places: the mordant job step in .github/workflows/rust-lints.yml, and the rust:mordant / rust:mordant:baseline scripts in package.json. It also updates the workflow documentation in .github/workflows/CLAUDE.md (the job table and a new bullet explaining why --lib is used) and adds an inline comment above the workflow step. No runtime, test, or build code is touched.
Security risks
None. This is a CI lint-job flag change and documentation update. It does not touch auth, permissions, secrets, or any code that ships in the binary.
Level of scrutiny
Low. The mordant job is explicitly advisory (continue-on-error: true) and skipped on merge_group, so even a mistake here cannot block merges or affect release artifacts. The change is a single-flag substitution whose behavior on the happy path is identical (mordant is the only library listed under [workspace.metadata.dylint]), and whose behavior on the failure path was verified empirically in the PR description with before/after transcripts. I confirmed against Cargo.toml that the pinned library is indeed scarletindustries/mordant, so the name passed to --lib matches what dylint will resolve.
Other factors
- The PR description is unusually thorough: it names the upstream dylint code path (
cargo_metadata()inlibrary_packages/mod.rs,name_as_libinlib.rs), shows a reproduction of both the silent-pass and the new hard-fail, and notes thatcargo dylint listreports the name asmordant. - All sibling sites are updated together (workflow step, both package.json scripts, doc table, doc bullet); a repo-wide grep for
dylint --allfinds no leftovers. - The workflow's path filter includes both
.github/workflows/rust-lints.ymlandpackage.json, so the mordant job runs on this PR and covers the normal (metadata-readable) path. - No prior reviews or outstanding comments on the timeline.
|
Status: ready for review. No open review threads. Shape, after discussion: the command keeps How the hole was reproduced: duplicate the The normal path runs on this PR itself. The |
…he mordant job Without --manifest-path, dylint turns a failing cargo metadata into a warning that repeats the first line of the cargo error, then finds no libraries, warns again and exits 0 with nothing linted. The step after it sees no over-baseline.txt and passes. With --manifest-path, dylint propagates the cargo metadata error and the step fails with the full cargo output. The two package.json scripts change the same way so a local run on a broken tree does not report clean either.
be10e3b to
f5e7285
Compare
|
The title check warning above compares the title against a summary of be10e3b, the first version of this PR ( @coderabbitai review The |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '=== Current commit ==='
git rev-parse HEAD
git status --short
printf '%s\n' '=== PR diff against base ==='
git diff --find-renames --unified=80 'HEAD^' -- .github/workflows/rust-lints.yml package.json .github/workflows/CLAUDE.md || true
printf '%s\n' '=== Dylint invocations and surrounding context ==='
rg -n -C 4 --glob 'rust-lints.yml' --glob 'package.json' --glob 'CLAUDE.md' \
'cargo dylint|rust:mordant|manifest-path|--all|--workspace' .github package.json || trueLength of output: 30663
The workflow and both local scripts retain
|
There was a problem hiding this comment.
LGTM — adds --manifest-path Cargo.toml to the cargo dylint invocations so a broken workspace manifest fails the mordant step instead of silently linting nothing.
What was reviewed:
- Checked all
cargo dylintcall sites in the repo — the workflow step, bothrust:mordant*package.json scripts, and the CLAUDE.md table are updated consistently; no stragglers. - The flag points at the repo-root manifest the job already runs against, so the normal path is unchanged; the mordant job is
continue-on-error: true, so worst case is advisory. - Ruled out the CodeRabbit title-mismatch warning — it referenced the earlier
--lib mordantrevision; the current diff matches the title.
Extended reasoning...
Overview
This PR adds --manifest-path Cargo.toml to every cargo dylint invocation in the repo: the mordant step in .github/workflows/rust-lints.yml, the rust:mordant and rust:mordant:baseline scripts in package.json, and the corresponding table row plus a new explanatory bullet in .github/workflows/CLAUDE.md. The workflow step also gains an inline comment explaining why the flag is passed even though it names the default manifest.
Security risks
None. This touches only CI lint tooling and internal documentation — no runtime code, no dependencies, no secrets handling. The added flag references a file already in the repo root.
Level of scrutiny
Low. The mordant job is advisory (continue-on-error: true) and does not gate merges, so even a regression here would only affect an informational check. The change is a single well-understood cargo flag pointing at the location dylint already defaults to; the only behavioral difference is that dylint now propagates cargo metadata failures instead of downgrading them to a warning when it had to guess the manifest. The PR description includes a before/after transcript demonstrating exactly this, and the job runs on this PR's own head to cover the happy path.
Other factors
A repo-wide grep confirms all four cargo dylint occurrences are updated in lockstep, satisfying the "fix the whole class" guidance. The added CLAUDE.md bullet and workflow comment document the non-obvious "why" so a future reader doesn't strip the seemingly-redundant flag. The CodeRabbit "title check" warning is stale — it was generated against the first revision that used --lib mordant; the current head (f5e7285) matches the title as the robobun follow-up comment notes. No prior human review comments are outstanding.
|
Closing: since #43661 the |
Problem
mordantjob can pass without linting anything. The last five runs on windows: statically merge .node addons into --compile output #30036 logWarning: duplicate key, thenWarning: No libraries were found., check zero crates, and pass. The run before them reported a finding insrc/exe_format/pe.rsthat is still there.cargo metadatafails and no--manifest-pathwas given, dylint (cargo_metadata()indylint/src/library_packages/mod.rs, same in 6.0.4) prints the first error line as a warning and continues with no libraries.--allthen has nothing to run and exits 0, and the over-baseline step passes.bun_css.workspace = truetwice insrc/runtime/Cargo.toml(Restore bun_css dependency of bun_runtime #39586 put it back on main, the PR adds it too).Fix
--manifest-path Cargo.tomltocargo dylint, in the job and in the twopackage.jsonscripts.--allstays.cargo metadatawhen it had to guess the manifest. With an explicit path it returns the error, so the step fails with the full cargo output (file and line). The same path is used forcargo checkand for the target directory, so a normal run is unchanged.bun_css.workspace = trueline, the current command exits 0 and the new one exits 1 with the cargo error (transcript in the notes).cargo dylint list --manifest-path Cargo.tomlstill finds themordantentry. The job also runs on this PR, which covers the normal path.Background
mordant-baseline.tomltotarget/mordant/over-baseline.txt. The step after dylint fails if that file is non-empty. A run that never started leaves no file.[workspace.metadata.dylint]entry in the rootCargo.tomlthroughcargo metadata, which parses every manifest in the workspace.pull_requestjobs run on the merge of the PR with main, so the manifests on the PR branch itself can be correct.Notes
Reproduction on this branch with
src/runtime/Cargo.tomlline 47 duplicated. Before:After:
Not covered: a tree whose manifests parse but whose
[workspace.metadata.dylint]table is gone still getsWarning: No libraries were found.and exit 0 (checked by removing the table).cargo dylint --lib mordantwould fail in that case too, at the price of a less direct message (Could not find --lib mordant) and of dropping--all. This PR was first written that way and switched after discussion. Making--allitself fail on an empty result is a change in dylint (warn_if_emptyindylint/src/lib.rs).Found while reading the mordant results of every robobun run since the job landed. Of the 37 green runs read in full, 5 had linted nothing. All 5 are on #30036, from the first run after #39586 merged onward.
no test proof · iteration 0 · docs-only change; test-proof not applicable