Skip to content

ci: pass --manifest-path to cargo dylint so a broken manifest fails the mordant job - #39982

Closed
robobun wants to merge 1 commit into
mainfrom
farm/63387200/mordant-lib-not-all
Closed

robobun wants to merge 1 commit into
mainfrom
farm/63387200/mordant-lib-not-all

Conversation

@robobun

@robobun robobun commented Aug 21, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

Fix

  • Pass --manifest-path Cargo.toml to cargo dylint, in the job and in the two package.json scripts. --all stays.
  • dylint only tolerates a failing cargo metadata when 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 for cargo check and for the target directory, so a normal run is unchanged.
  • Verified: with a duplicated bun_css.workspace = true line, 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.toml still finds the mordant entry. The job also runs on this PR, which covers the normal path.

Background

  • Mordant writes the findings over mordant-baseline.toml to target/mordant/over-baseline.txt. The step after dylint fails if that file is non-empty. A run that never started leaves no file.
  • dylint reads the [workspace.metadata.dylint] entry in the root Cargo.toml through cargo metadata, which parses every manifest in the workspace.
  • pull_request jobs 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.toml line 47 duplicated. Before:

$ cargo dylint --all --workspace -- --keep-going; echo exit=$?
Warning: duplicate key
Warning: No libraries were found.
exit=0

After:

$ cargo dylint --manifest-path Cargo.toml --all --workspace -- --keep-going; echo exit=$?
Error: `cargo metadata` exited with an error: error: duplicate key
  --> src/runtime/Cargo.toml:48:9
   |
48 | bun_css.workspace = true
   |         ^^^^^^^^^
error: failed to load manifest for workspace member `/workspace/bun/src/runtime`
exit=1

Not covered: a tree whose manifests parse but whose [workspace.metadata.dylint] table is gone still gets Warning: No libraries were found. and exit 0 (checked by removing the table). cargo dylint --lib mordant would 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 --all itself fail on an empty result is a change in dylint (warn_if_empty in dylint/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

@coderabbitai

coderabbitai Bot commented Aug 21, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: fabbbf36-4432-49b2-9538-2300e8115688

📥 Commits

Reviewing files that changed from the base of the PR and between be10e3b and f5e7285.

📒 Files selected for processing (3)
  • .github/workflows/CLAUDE.md
  • .github/workflows/rust-lints.yml
  • package.json

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.


Walkthrough

Changes

Mordant Dylint commands now pass --manifest-path Cargo.toml. Workspace-wide linting remains enabled. Documentation records that Cargo metadata failures now fail the lint step.

Mordant lint targeting

Layer / File(s) Summary
Explicit manifest path
.github/workflows/rust-lints.yml, package.json, .github/workflows/CLAUDE.md
Mordant commands now pass --manifest-path Cargo.toml and retain workspace-wide execution. Documentation describes metadata failure handling.

Suggested reviewers: cirospaciari, alii

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly and concisely describes the main CI change and its failure-handling purpose.
Description check ✅ Passed The description explains the problem, fix, verification, and background; it covers the template requirements.

Comment @coderabbitai help to get the list of available commands.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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] in Cargo.toml points at scarletindustries/mordant, so --lib mordant resolves the same library --all did.
  • Checked no stray dylint --all remains; all three invocations and the CLAUDE.md table/notes are updated in lockstep.
  • The job is continue-on-error: true and 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() in library_packages/mod.rs, name_as_lib in lib.rs), shows a reproduction of both the silent-pass and the new hard-fail, and notes that cargo dylint list reports the name as mordant.
  • All sibling sites are updated together (workflow step, both package.json scripts, doc table, doc bullet); a repo-wide grep for dylint --all finds no leftovers.
  • The workflow's path filter includes both .github/workflows/rust-lints.yml and package.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.

@robobun

robobun commented Aug 21, 2026 •

Copy link
Copy Markdown
Collaborator Author

Status: ready for review. No open review threads.

Shape, after discussion: the command keeps --all and adds --manifest-path Cargo.toml (head f5e7285). The first version of this PR used --lib mordant instead. Both bot reviews have since been redone against this head and found nothing.

How the hole was reproduced: duplicate the bun_css.workspace = true line in src/runtime/Cargo.toml (the state of #30036's merge with main) and run the old and the new command. cargo dylint --all --workspace prints Warning: duplicate key and Warning: No libraries were found. and exits 0. cargo dylint --manifest-path Cargo.toml --all --workspace exits 1 with the full cargo error. The transcript is in the PR body.

The normal path runs on this PR itself. The mordant job is green on both heads. On f5e7285 the dylint step ran for 110 s with both caches restored, so it linted the workspace with the new command (https://github.com/oven-sh/bun/actions/runs/32505697023/job/96845246081).

@alii
alii marked this pull request as draft August 21, 2026 16:52
…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.
@robobun
robobun force-pushed the farm/63387200/mordant-lib-not-all branch from be10e3b to f5e7285 Compare August 21, 2026 16:58
@robobun robobun changed the title ci: run mordant with --lib mordant so a run that lints nothing fails ci: pass --manifest-path to cargo dylint so a broken manifest fails the mordant job Aug 21, 2026
@alii
alii marked this pull request as ready for review August 21, 2026 17:01
@robobun

robobun commented Aug 21, 2026

Copy link
Copy Markdown
Collaborator Author

The title check warning above compares the title against a summary of be10e3b, the first version of this PR (--lib mordant). The current head f5e7285 keeps --all and adds --manifest-path Cargo.toml, which is what the title says. No title change is needed.

@coderabbitai review

The mordant job on f5e7285 is green. Its dylint step ran for 110 s with both caches restored, so it linted the workspace with the new command rather than exiting early.

@coderabbitai

coderabbitai Bot commented Aug 21, 2026 •

Copy link
Copy Markdown
Contributor
🧩 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 || true

Length of output: 30663


@robobun Review complete. I found no actionable issues in f5e7285d26.

The workflow and both local scripts retain --all and add --manifest-path Cargo.toml. The documentation matches the command behavior and the metadata-failure rationale. The title warning applies to the superseded --lib mordant revision, so no title change is needed.

⚠️ Action not completed

Already reviewed.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 dylint call sites in the repo — the workflow step, both rust: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 mordant revision; 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.

@robobun

robobun commented Sep 21, 2026

Copy link
Copy Markdown
Collaborator Author

Closing: since #43661 the mordant job runs cargo mordant and not cargo dylint, so the call that this change edits no longer exists. With a duplicate bun_css.workspace = true line in src/runtime/Cargo.toml, cargo mordant --workspace --keep-going now prints the cargo error and exits 1.

@robobun robobun closed this Sep 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants