feat: implement dylint rules feature with dynamic library handling and build process#30
Merged
Artifizer merged 3 commits intocyberfabric:mainfrom Apr 17, 2026
Merged
Conversation
…d build process Signed-off-by: Bechma <19294519+Bechma@users.noreply.github.com>
Contributor
📝 WalkthroughWalkthroughImplements workspace-scoped Changes
Sequence DiagramsequenceDiagram
participant Dev as Developer
participant Cargo as Cargo
participant BuildRS as build.rs
participant Git as GitCache
participant Rustup as rustup
participant FS as FileSystem
participant CLI as cf CLI
participant Dylint as Dylint Engine
Dev->>Cargo: cargo build --features dylint-rules
Cargo->>BuildRS: run build script
BuildRS->>Git: ensure cached lint workspace (clone/fetch)
Git-->>BuildRS: repo available
BuildRS->>Rustup: rustup toolchain list
Rustup-->>BuildRS: installed toolchains
alt toolchain missing
BuildRS->>Rustup: rustup toolchain install <channel> --profile minimal
Rustup-->>BuildRS: toolchain installed
end
BuildRS->>Rustup: rustup run <channel> cargo build --release --workspace --target-dir <OUT_DIR>
Rustup->>FS: compile -> produce dylibs (.so/.dylib/.dll)
FS-->>BuildRS: dylibs produced
BuildRS->>FS: copy dylibs to OUT_DIR/dylint_libs and generate generated_libs.rs (include_bytes!)
FS-->>Cargo: embedded artifacts ready
Dev->>CLI: cf lint -p <path> --dylint
CLI->>FS: parse_and_chdir (if -p), load generated_libs
CLI->>FS: write embedded dylibs to temp dir
CLI->>Dylint: dylint::run(check, dylib_paths, workspace_root)
Dylint-->>CLI: lint results
CLI-->>Dev: report lint results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (9)
crates/cli/build.rs (4)
13-17: Duplicateuseimports insidebuild_dylint_rules.
use anyhow::{Context, bail};(Line 14) anduse std::fs;(Line 17) shadow the file-level imports at Line 2 and Line 3 with no added value. Drop the inner ones and keep only the extraenv/fmt::Write as _imports if desired.♻️ Proposed cleanup
fn build_dylint_rules() -> anyhow::Result<()> { - use anyhow::{Context, bail}; use std::env; use std::fmt::Write as _; - use std::fs;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/build.rs` around lines 13 - 17, Inside the build_dylint_rules function, remove the redundant inner use statements that duplicate file-level imports: drop `use anyhow::{Context, bail};` and `use std::fs;` and keep only the genuinely local imports (e.g., `use std::env;` and `use std::fmt::Write as _`) so the function no longer shadows the module-level `anyhow` and `std::fs` imports; adjust the function signature `fn build_dylint_rules()` accordingly to rely on the top-level imports.
166-171:panic!vs cargo-friendly build error.Panicking with
{error:#}works but produces a noisy Rust panic backtrace. Cargo-native failure is more readable: print each error line withcargo:warning=and thenstd::process::exit(1)(or simplyreturnafter emitting the message and letting a non-zero exit happen naturally from amain() -> Result).♻️ Optional refactor
-fn main() { - #[cfg(feature = "dylint-rules")] - if let Err(error) = build_dylint_rules() { - panic!("{error:#}"); - } -} +fn main() { + #[cfg(feature = "dylint-rules")] + if let Err(error) = build_dylint_rules() { + for line in format!("{error:#}").lines() { + println!("cargo:warning={line}"); + } + std::process::exit(1); + } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/build.rs` around lines 166 - 171, Replace the panic! in main with a cargo-friendly error emission: when build_dylint_rules() returns Err(error) emit the error lines using cargo:warning= (e.g. via eprintln!("cargo:warning={}", ...)) for each relevant message and then exit with a non-zero status (std::process::exit(1)) or change main to return Result and return Err so Cargo reports a clean failure; update the main() error path that currently panics!("{error:#}") to use these steps referencing main and build_dylint_rules.
229-266: Duplicate ofensure_toolchain_installedincrates/cli/src/lint/mod.rs.This function is essentially identical to
ensure_toolchain_installedincrates/cli/src/lint/mod.rs(Lines 54-90). Sincebuild.rsand the crate can't share a module directly, one pragmatic option is to move the helper into a tinyshared/module andinclude!it from both sides, or accept the duplication but add a comment cross-referencing the two copies so they stay in sync.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/build.rs` around lines 229 - 266, There are two nearly identical functions named ensure_toolchain_installed (one in build.rs and one in crates/cli/src/lint/mod.rs); to fix this, extract the function into a single shared snippet and include it from both places (or if you prefer to keep duplication, add a clear cross-reference comment). Concretely: create a small shared source file (e.g., shared/ensure_toolchain_installed.rs) containing the ensure_toolchain_installed function with its #[cfg(feature = "dylint-rules")] attribute and required imports (anyhow, Command, etc.), then use include! to pull that file into build.rs and into crates/cli/src/lint/mod.rs so both compile units reuse the same implementation; alternatively, add a comment in both copies referencing the other file path and note that changes must be mirrored to keep them in sync.
134-138: Stale comment: "main.rs includes this file".
generated_libs.rsis included bycrates/cli/src/lint/mod.rs, not bymain.rs.📝 Proposed wording fix
- // Build a `generated_libs.rs` that hard-codes every versioned dylib as - // raw bytes via `include_bytes!`. main.rs includes this file and writes - // the bytes to a temp directory at runtime, so the binary is fully - // self-contained. + // Build a `generated_libs.rs` that hard-codes every versioned dylib as + // raw bytes via `include_bytes!`. `crates/cli/src/lint/mod.rs` includes + // this file and writes the bytes to a temp directory at runtime, so the + // binary is fully self-contained.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/build.rs` around lines 134 - 138, Update the stale comment in the generated_libs.rs generation block in build.rs: replace "main.rs includes this file" with the correct include target (crates/cli/src/lint/mod.rs) and adjust surrounding wording so it accurately states that generated_libs.rs is included by the lint module rather than main.rs; keep the explanation that the file hard-codes versioned dylibs via include_bytes! and that the bytes are written to a temp directory at runtime so the binary is self-contained.crates/cli/src/lint/mod.rs (3)
20-35:--clippysilently does nothing;cyberfabric lintwith no flags is a no-op.
LintArgs::runreturnsOk(())without any output when--clippyalone is passed, or when neither flag is passed. Users get no indication that their command did nothing. Consider at minimum printing a notice (e.g., "clippy integration not implemented yet") or requiring at least one of--clippy/--dylintvia clap'sArgGroup(required = true).♻️ Possible refactor
pub fn run(&self) -> Result<()> { if self.dylint { run_dylint()?; } + if self.clippy { + eprintln!("note: --clippy is accepted but not yet wired to invoke clippy"); + } + if !self.clippy && !self.dylint { + eprintln!("note: nothing to do; pass --dylint or --clippy"); + } Ok(()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/src/lint/mod.rs` around lines 20 - 35, LintArgs::run currently does nothing for --clippy and when no flags are passed, leaving users without feedback; update LintArgs::run to validate flags and provide clear output or enforce required flag semantics: if neither self.clippy nor self.dylint is set, print a helpful message (e.g., "no lints selected; pass --clippy or --dylint") and return an Err or nonzero exit code, and if self.clippy is true print a placeholder notice like "clippy integration not implemented yet" (or call the real clippy runner when available); alternatively, enforce at argument parsing time by adding a required ArgGroup for clippy and dylint so clap prevents running without a flag. Ensure changes reference LintArgs::run, the clippy/dylint fields, and any clap ArgGroup configuration you add.
94-98: Stale comment: tmp_dir drops at end ofrun_dylint, notmain.
tmp_diris a local ofrun_dylint, so the dylibs are deleted the moment this function returns — which is fine becausedylint::runhas finished synchronously, but the "end of main" wording is misleading for future readers.📝 Proposed wording fix
- // Write every embedded dylib to a per-run temp directory so dylint can - // dlopen them. The temp dir (and its contents) is removed automatically - // when `tmp_dir` drops at the end of main, which is safe because dylint - // has already finished using the files by then. + // Write every embedded dylib to a per-run temp directory so dylint can + // dlopen them. The temp dir (and its contents) is removed when `tmp_dir` + // drops at the end of this function, which is safe because `dylint::run` + // is synchronous and has already finished using the files by then.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/src/lint/mod.rs` around lines 94 - 98, The comment above the tempfile::tempdir() call is misleading: tmp_dir is a local in run_dylint, not main; update the comment near tmp_dir to state that the temp dir and its contents are removed when tmp_dir drops at the end of run_dylint (after dylint::run completes) rather than "end of main", and keep the existing explanation that this is safe because dylint::run finishes synchronously.
67-75: Prefix match on installed toolchains could over-match.
starts_with(&format!("{toolchain}-"))will also match unrelated toolchains whentoolchainis a prefix of another installed one. Giventoolchainhere is already<channel>-<host>(very specific), collisions are unlikely, but iftoolchainever becomes just<channel>(e.g.nightly), the prefix check would match any dated nightly (nightly-2024-…), potentially hiding a needed install. Consider exact-match only, or matching against the dated form explicitly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/src/lint/mod.rs` around lines 67 - 75, The current existence check for installed toolchains (variables installed, installed_prefix and toolchain in the .any(...) closure) uses starts_with(&installed_prefix) which can over-match when toolchain is a prefix; change the match to be exact or to explicitly detect the dated form: keep installed == toolchain for exact matches, and replace the starts_with check with a check that the installed string starts with format!("{toolchain}-") AND the character immediately after the dash is a digit (or otherwise validate the dated suffix), so only true dated toolchain names (e.g., "nightly-2024-...") match and unrelated longer names do not.crates/cli/Cargo.toml (2)
49-51: Redundanttempfiledev-dependency.
tempfileis now a regular dependency (line 43), so its[dev-dependencies]entry is redundant and can be removed.♻️ Proposed cleanup
[dev-dependencies] -tempfile = { workspace = true } module-parser = { workspace = true, features = ["test-utils"] }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/Cargo.toml` around lines 49 - 51, The Cargo.toml dev-dependencies block contains a redundant entry for tempfile (under [dev-dependencies]) because tempfile is already declared as a regular dependency; remove the tempfile = { workspace = true } line from the [dev-dependencies] section while keeping module-parser = { workspace = true, features = ["test-utils"] } intact so only module-parser remains as a dev-dependency.
22-23: Usedep:prefix syntax for the optionaldylintdependency and remove the redundant dev-dependency entry.The current syntax
dylint-rules = ["dylint"]implicitly exposes adylintfeature alongside the intendeddylint-rulesfeature. Usedep:to gate the optional dependency explicitly:dylint-rules = ["dep:dylint"].Additionally,
tempfileis declared in both[dependencies](line 43) and[dev-dependencies](line 50). The dev-dependencies entry is redundant; remove it.♻️ Proposed changes
[features] -dylint-rules = ["dylint"] +dylint-rules = ["dep:dylint"] [dev-dependencies] tempfile = { workspace = true }(Remove line 50 entirely.)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/Cargo.toml` around lines 22 - 23, Update the Cargo.toml feature declaration to gate the optional dylint dependency explicitly by changing the feature value from "dylint" to "dep:dylint" for the feature named dylint-rules, and remove the redundant dev-dependency entry for tempfile so it only appears under [dependencies]; edit the dylint-rules feature and delete the tempfile entry in [dev-dependencies] accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/cli/build.rs`:
- Around line 25-27: The build script currently skips updating the cached lint
checkout when lint_build_dir.exists(), causing upstream toolchain/lint changes
(or an updated unpinned main) to be ignored; update ensure_lints_dir / the logic
around lint_build_dir and the early return so that either (a) you detect and
fetch/pull the remote (git fetch + git reset/checkout or git pull) when the
configured revision/branch (the unpinned main or a pinned commit) has changed,
or (b) store the resolved commit SHA alongside the cache and invalidate/remove
lint_build_dir (and regenerate generated_libs.rs) when that SHA differs; also
ensure the build script emits appropriate rerun-if-changed metadata (e.g., on
the pinned revision file or stored SHA) so cached artifacts are rebuilt when the
source revision changes and avoid silently returning Ok() from the
lint_build_dir.exists() branch.
- Line 10: The code pins the external lints repo to the mutable "main" branch
(LINTS_REPO_URL) which makes builds non-reproducible and unsafe; add a new
constant (e.g. LINTS_REPO_REV) containing a specific commit SHA or tag and
update clone_lints_repo to clone the repo and then explicitly checkout that
revision (or use git fetch && git checkout <SHA>), and after checkout verify the
HEAD matches LINTS_REPO_REV before proceeding to build (alternatively vendor the
workspace as a submodule/subtree and remove the live clone); update any
references in the clone_lints_repo logic (and the constants near LINTS_REPO_URL)
at the other clone code paths (lines ~174-193) to use the pinned revision and a
verification step.
- Around line 1-7: The unconditional import use std::fs; is unused when the
"dylint-rules" feature is disabled; gate it with the same cfg attribute as the
other imports so it is only compiled when that feature is enabled. Update the
import to #[cfg(feature = "dylint-rules")] use std::fs; so the symbol matches
the existing conditional imports (see build.rs and the other imports like use
std::path::{Path, PathBuf} and use std::process::Command).
- Around line 105-132: The current loop in build.rs that iterates
fs::read_dir(&release_dir) and filters by dll_prefix/dll_suffix can pick up
unrelated artifacts; replace this with parsing cargo build --message-format=json
(or cargo metadata) to collect only compiler-artifact messages whose target.kind
contains "cdylib" and whose package is a workspace member, then copy only those
artifact filenames into libs_dir using the existing versioned naming (use the
same dll_prefix, dll_suffix, versioned_toolchain, libs_dir variables);
alternatively, implement an explicit allow-list of lint crate names and filter
the computed stem (from strip_prefix/strip_suffix logic) against that list
before copying to avoid non-lint cdylib/proc-macro/dSYM files being installed.
In `@crates/cli/src/lint/mod.rs`:
- Around line 139-142: The run_dylint function currently panics via
unimplemented! when the "dylint-rules" feature is disabled; change it to return
a proper anyhow::Error instead so the CLI surfaces a predictable failure (e.g.,
use anyhow::bail! or return Err(anyhow::anyhow!(...)) with a clear message that
the dylint-rules feature is not enabled). Update the #[cfg(not(feature =
"dylint-rules"))] run_dylint signature to construct and return that error so
callers of run_dylint get a Result::Err rather than a panic.
---
Nitpick comments:
In `@crates/cli/build.rs`:
- Around line 13-17: Inside the build_dylint_rules function, remove the
redundant inner use statements that duplicate file-level imports: drop `use
anyhow::{Context, bail};` and `use std::fs;` and keep only the genuinely local
imports (e.g., `use std::env;` and `use std::fmt::Write as _`) so the function
no longer shadows the module-level `anyhow` and `std::fs` imports; adjust the
function signature `fn build_dylint_rules()` accordingly to rely on the
top-level imports.
- Around line 166-171: Replace the panic! in main with a cargo-friendly error
emission: when build_dylint_rules() returns Err(error) emit the error lines
using cargo:warning= (e.g. via eprintln!("cargo:warning={}", ...)) for each
relevant message and then exit with a non-zero status (std::process::exit(1)) or
change main to return Result and return Err so Cargo reports a clean failure;
update the main() error path that currently panics!("{error:#}") to use these
steps referencing main and build_dylint_rules.
- Around line 229-266: There are two nearly identical functions named
ensure_toolchain_installed (one in build.rs and one in
crates/cli/src/lint/mod.rs); to fix this, extract the function into a single
shared snippet and include it from both places (or if you prefer to keep
duplication, add a clear cross-reference comment). Concretely: create a small
shared source file (e.g., shared/ensure_toolchain_installed.rs) containing the
ensure_toolchain_installed function with its #[cfg(feature = "dylint-rules")]
attribute and required imports (anyhow, Command, etc.), then use include! to
pull that file into build.rs and into crates/cli/src/lint/mod.rs so both compile
units reuse the same implementation; alternatively, add a comment in both copies
referencing the other file path and note that changes must be mirrored to keep
them in sync.
- Around line 134-138: Update the stale comment in the generated_libs.rs
generation block in build.rs: replace "main.rs includes this file" with the
correct include target (crates/cli/src/lint/mod.rs) and adjust surrounding
wording so it accurately states that generated_libs.rs is included by the lint
module rather than main.rs; keep the explanation that the file hard-codes
versioned dylibs via include_bytes! and that the bytes are written to a temp
directory at runtime so the binary is self-contained.
In `@crates/cli/Cargo.toml`:
- Around line 49-51: The Cargo.toml dev-dependencies block contains a redundant
entry for tempfile (under [dev-dependencies]) because tempfile is already
declared as a regular dependency; remove the tempfile = { workspace = true }
line from the [dev-dependencies] section while keeping module-parser = {
workspace = true, features = ["test-utils"] } intact so only module-parser
remains as a dev-dependency.
- Around line 22-23: Update the Cargo.toml feature declaration to gate the
optional dylint dependency explicitly by changing the feature value from
"dylint" to "dep:dylint" for the feature named dylint-rules, and remove the
redundant dev-dependency entry for tempfile so it only appears under
[dependencies]; edit the dylint-rules feature and delete the tempfile entry in
[dev-dependencies] accordingly.
In `@crates/cli/src/lint/mod.rs`:
- Around line 20-35: LintArgs::run currently does nothing for --clippy and when
no flags are passed, leaving users without feedback; update LintArgs::run to
validate flags and provide clear output or enforce required flag semantics: if
neither self.clippy nor self.dylint is set, print a helpful message (e.g., "no
lints selected; pass --clippy or --dylint") and return an Err or nonzero exit
code, and if self.clippy is true print a placeholder notice like "clippy
integration not implemented yet" (or call the real clippy runner when
available); alternatively, enforce at argument parsing time by adding a required
ArgGroup for clippy and dylint so clap prevents running without a flag. Ensure
changes reference LintArgs::run, the clippy/dylint fields, and any clap ArgGroup
configuration you add.
- Around line 94-98: The comment above the tempfile::tempdir() call is
misleading: tmp_dir is a local in run_dylint, not main; update the comment near
tmp_dir to state that the temp dir and its contents are removed when tmp_dir
drops at the end of run_dylint (after dylint::run completes) rather than "end of
main", and keep the existing explanation that this is safe because dylint::run
finishes synchronously.
- Around line 67-75: The current existence check for installed toolchains
(variables installed, installed_prefix and toolchain in the .any(...) closure)
uses starts_with(&installed_prefix) which can over-match when toolchain is a
prefix; change the match to be exact or to explicitly detect the dated form:
keep installed == toolchain for exact matches, and replace the starts_with check
with a check that the installed string starts with format!("{toolchain}-") AND
the character immediately after the dash is a digit (or otherwise validate the
dated suffix), so only true dated toolchain names (e.g., "nightly-2024-...")
match and unrelated longer names do not.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3a86b937-394e-42ea-befb-09a69269a5bc
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
AGENTS.mdCargo.tomlREADME.mdSKILLS.mdcrates/cli/Cargo.tomlcrates/cli/build.rscrates/cli/src/lint/mod.rs
Comment on lines
+105
to
+132
| for entry in fs::read_dir(&release_dir).context("could not read release dir")? { | ||
| let entry = entry?; | ||
| let path = entry.path(); | ||
| let filename = match path.file_name() { | ||
| Some(f) => f.to_string_lossy().into_owned(), | ||
| None => continue, | ||
| }; | ||
|
|
||
| // Only consider shared library files that don't already have the versioned name. | ||
| if !filename.starts_with(dll_prefix) | ||
| || !filename.ends_with(dll_suffix) | ||
| || filename.contains('@') | ||
| { | ||
| continue; | ||
| } | ||
|
|
||
| let stem = filename | ||
| .strip_prefix(dll_prefix) | ||
| .context("wrong library prefix")? | ||
| .strip_suffix(&format!(".{dll_suffix}")) | ||
| .context("wrong library suffix")?; | ||
|
|
||
| let versioned = format!("{dll_prefix}{stem}@{versioned_toolchain}.{dll_suffix}"); | ||
| let dest = libs_dir.join(&versioned); | ||
| fs::copy(&path, &dest) | ||
| .with_context(|| format!("failed to copy {filename} -> {versioned}"))?; | ||
| println!("cargo:warning=dylint lint installed: {versioned}"); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# See if the lint workspace's members are discoverable so an allow-list is feasible
rg -nP 'members\s*=' --type=tomlRepository: cyberfabric/cf-cli
Length of output: 1787
🏁 Script executed:
# Search for Cargo.toml files with workspace member definitions
find . -name "Cargo.toml" -type f | head -20Repository: cyberfabric/cf-cli
Length of output: 132
🏁 Script executed:
# Also check the structure of the repository
ls -la | head -20Repository: cyberfabric/cf-cli
Length of output: 1140
🏁 Script executed:
# Check the root Cargo.toml for workspace members
cat Cargo.tomlRepository: cyberfabric/cf-cli
Length of output: 1501
🏁 Script executed:
# Examine the build.rs file to understand the full context
cat -n crates/cli/build.rsRepository: cyberfabric/cf-cli
Length of output: 11699
Iterating release/ picks up unrelated artifacts; prefer cargo metadata --message-format=json or a name allow-list.
The copy loop accepts every lib<…>.so / .dylib / .dll in target/release/ that doesn't already contain @. When building the lint workspace with --workspace, Cargo will deposit artifacts for all workspace members and their transitive dependencies there. Any cdylib or proc-macro artifacts (e.g., from libdylint_link, test fixtures, or dSYM companions) matching the filename pattern would then be embedded as lint libraries and attempted to be loaded at runtime.
Safer options:
- Parse
cargo build --message-format=jsonoutput and only copycompiler-artifactentries whosetarget.kindcontainscdyliband whose package is a workspace member of the lint workspace. - Maintain an explicit list of expected lint crate names and filter on
stem.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/cli/build.rs` around lines 105 - 132, The current loop in build.rs
that iterates fs::read_dir(&release_dir) and filters by dll_prefix/dll_suffix
can pick up unrelated artifacts; replace this with parsing cargo build
--message-format=json (or cargo metadata) to collect only compiler-artifact
messages whose target.kind contains "cdylib" and whose package is a workspace
member, then copy only those artifact filenames into libs_dir using the existing
versioned naming (use the same dll_prefix, dll_suffix, versioned_toolchain,
libs_dir variables); alternatively, implement an explicit allow-list of lint
crate names and filter the computed stem (from strip_prefix/strip_suffix logic)
against that list before copying to avoid non-lint cdylib/proc-macro/dSYM files
being installed.
…gration Signed-off-by: Bechma <19294519+Bechma@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
crates/cli/src/lint/mod.rs (1)
63-72: Optional: create the temp dir after toolchain installation succeeds.
tempfile::tempdir()on line 68 is created beforeensure_toolchain_installedruns (lines 70‑72). If rustup fails to install a toolchain, we've already allocated a scratch directory for nothing. Swapping the order keeps failure paths cheaper and the tempdir lifetime tighter.♻️ Proposed refactor
fn run_dylint() -> Result<()> { - // Write every embedded dylib to a per-run temp directory so dylint can - // dlopen them. ... - let tmp_dir = tempfile::tempdir().context("could not create temp dir for dylibs")?; - for toolchain in embedded_toolchains()? { ensure_toolchain_installed(&toolchain)?; } + + // Write every embedded dylib to a per-run temp directory so dylint can + // dlopen them. ... + let tmp_dir = tempfile::tempdir().context("could not create temp dir for dylibs")?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/src/lint/mod.rs` around lines 63 - 72, Move the creation of the temporary directory so it happens after toolchain installation succeeds: in run_dylint(), call embedded_toolchains()? and loop calling ensure_toolchain_installed(&toolchain)? first, and only after that create tmp_dir via tempfile::tempdir().context(...) so the tempdir is allocated only when ensure_toolchain_installed completes successfully; update any code that uses tmp_dir within run_dylint to reflect the new position/scope.crates/cli/shared/ensure_toolchain_installed.rs (1)
20-20: Minor: add context to the UTF‑8 decoding error.Every other fallible step in this function gets an
anyhow::Context, butString::from_utf8(installed.stdout)?propagates a bareFromUtf8Error. Adding context keeps error messages consistent and easier to debug ifrustupever emits non‑UTF‑8 stdout.♻️ Proposed refactor
- let installed = String::from_utf8(installed.stdout)?; + let installed = String::from_utf8(installed.stdout) + .context("rustup toolchain list returned non-UTF-8 output")?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/shared/ensure_toolchain_installed.rs` at line 20, Replace the bare UTF‑8 decode that can return a FromUtf8Error with an anyhow::Context‑wrapped call so the error includes context; specifically change the String::from_utf8(installed.stdout)? usage in ensure_toolchain_installed to String::from_utf8(installed.stdout).context("decoding rustup stdout as UTF-8")? (ensure anyhow::Context is in scope) so decoding failures include a clear message about rustup stdout.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/cli/src/lint/mod.rs`:
- Around line 29-44: LintArgs defines a parsed --clippy flag but run() ignores
it, so invoking `cyberfabric lint --clippy` does nothing; fix by either wiring
the flag to call the clippy path (add a `run_clippy()` function or call the
existing clippy invocation from `run()` when `self.clippy` is true, analogous to
how `self.dylint` calls `run_dylint()` inside `impl LintArgs::run`) or remove
the `#[arg(long)] clippy: bool` field and update documentation (SKILLS.md)
accordingly so the CLI and docs remain consistent.
---
Nitpick comments:
In `@crates/cli/shared/ensure_toolchain_installed.rs`:
- Line 20: Replace the bare UTF‑8 decode that can return a FromUtf8Error with an
anyhow::Context‑wrapped call so the error includes context; specifically change
the String::from_utf8(installed.stdout)? usage in ensure_toolchain_installed to
String::from_utf8(installed.stdout).context("decoding rustup stdout as UTF-8")?
(ensure anyhow::Context is in scope) so decoding failures include a clear
message about rustup stdout.
In `@crates/cli/src/lint/mod.rs`:
- Around line 63-72: Move the creation of the temporary directory so it happens
after toolchain installation succeeds: in run_dylint(), call
embedded_toolchains()? and loop calling ensure_toolchain_installed(&toolchain)?
first, and only after that create tmp_dir via tempfile::tempdir().context(...)
so the tempdir is allocated only when ensure_toolchain_installed completes
successfully; update any code that uses tmp_dir within run_dylint to reflect the
new position/scope.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 12dc1ff6-4886-47fd-87b3-97e1532de819
📒 Files selected for processing (4)
crates/cli/Cargo.tomlcrates/cli/build.rscrates/cli/shared/ensure_toolchain_installed.rscrates/cli/src/lint/mod.rs
✅ Files skipped from review due to trivial changes (1)
- crates/cli/build.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/cli/Cargo.toml
…tegration Signed-off-by: Bechma <19294519+Bechma@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
crates/cli/src/lint/mod.rs (1)
29-44:⚠️ Potential issue | 🟡 Minor
--clippyis still parsed but never honored.
self.clippyis declared at Line 30 butrun()only branches onself.dylint.cyberfabric lint --clippysilently succeeds without invoking clippy. Either wire it up (or stub with a clear error) and updateSKILLS.md/README.md, or remove the field until it lands.- #[arg(long)] - clippy: bool, #[arg(long)] dylint: bool,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/src/lint/mod.rs` around lines 29 - 44, The LintArgs struct exposes a clippy flag that is parsed but ignored in LintArgs::run; update run to handle self.clippy by either invoking a clippy runner function (e.g., call a new run_clippy() alongside run_dylint()) or return a clear error indicating clippy support is not yet implemented; ensure you reference LintArgs::run, the self.clippy field, and the existing run_dylint() symbol when adding the branch or error, and adjust any docs (SKILLS.md/README.md) to match the chosen behavior or remove the clippy field if you opt to postpone support.crates/cli/build.rs (2)
116-143:⚠️ Potential issue | 🟡 MinorRelease-dir iteration still embeds every non-versioned
lib*.<ext>— unrelated cdylibs will leak in.When building
--workspace,target/release/accumulates cdylib artifacts from transitive deps (e.g.libdylint_link) and, depending on the lint workspace's members, potentially proc-macro / test fixture dylibs. They all satisfy the currentstarts_with(dll_prefix) && ends_with(dll_suffix) && !contains('@')filter and get versioned + embedded + laterdlopened bydylint::run.Prefer one of:
- Drive the copy off
cargo build --message-format=json, keeping onlycompiler-artifactmessages whosetarget.kindcontainscdyliband whosepackage_idis a direct member of the lint workspace.- Maintain an explicit allow-list of lint crate stems and filter on
stembefore copying.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/build.rs` around lines 116 - 143, The current loop in build.rs copies every file in release_dir matching dll_prefix/dll_suffix into libs_dir, which pulls in unrelated cdylibs; restrict this by adding an explicit allow-list check on the computed stem before performing the copy: after computing stem (from strip_prefix/strip_suffix) validate stem against a const or env-provided HashSet/List of accepted lint crate stems (e.g., allowed_stems containing the real lint crate names) and only proceed to construct versioned, dest, fs::copy and println! if stem is in that allow-list; update any error/context messages to mention the allow-list filter and ensure allowed stems are populated from the workspace configuration or a maintained constant to avoid leaking transitive cdylibs to dlint::run.
73-101:⚠️ Potential issue | 🟡 MinorCache is not invalidated when
LINTS_REPO_REVISIONchanges.Pinning the revision is correct, but the caching path doesn't observe the pin:
ensure_lints_dir(Line 226-232) short-circuits wheneverrepo_dir/.gitanddylint_lints/Cargo.tomlexist, without checking thatHEADmatchesLINTS_REPO_REVISION.lint_build_dir.exists()here (Line 73) reuses the previously built release artifacts unconditionally.Bumping
LINTS_REPO_REVISIONon a machine that already has a populatedOUT_DIRwill silently keep building against the old checkout + old artifacts. Consider either (a) storing the pinned SHA in a sentinel file next to the cache and wiping bothrepo_dirandlint_build_dirwhen it differs, or (b) always runninggit fetch --depth 1 origin <rev> && git checkout --detach FETCH_HEADon the cached repo and rebuilding when HEAD moves.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/build.rs` around lines 73 - 101, ensure_lints_dir and the lint build cache (lint_build_dir) are not invalidated when LINTS_REPO_REVISION changes; add a sentinel check and invalidation: when preparing the cached repo_dir/dylint_lints/Cargo.toml in ensure_lints_dir, read the current HEAD (or store/compare a sentinel file) and compare it to LINTS_REPO_REVISION; if they differ, remove both repo_dir and lint_build_dir (or otherwise wipe the cache) so a fresh checkout and rebuild occur; alternatively, if you prefer in-place update, run git fetch --depth 1 origin <LINTS_REPO_REVISION> && git checkout --detach FETCH_HEAD in the cached repo and then force a rebuild by removing lint_build_dir if HEAD moved. Ensure the code paths referencing lint_build_dir.exists(), ensure_lints_dir, repo_dir, OUT_DIR, and LINTS_REPO_REVISION implement this compare-and-invalidate behavior so builds are deterministic when the pinned revision changes.
🧹 Nitpick comments (1)
crates/cli/build.rs (1)
147-149: Nit: stray//in the comment block.Line 148 reads
// // this file and writes the bytes .... Looks like a leftover from reflowing the comment; drop the extra//.- // Build a `generated_libs.rs` that hard-codes every versioned dylib as - // raw bytes via `include_bytes!`. `crates/cli/src/lint/mod.rs` includes - // // this file and writes the bytes to a temp directory at runtime, so the - // binary is fully self-contained + // Build a `generated_libs.rs` that hard-codes every versioned dylib as + // raw bytes via `include_bytes!`. `crates/cli/src/lint/mod.rs` includes + // this file and writes the bytes to a temp directory at runtime, so the + // binary is fully self-contained.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/cli/build.rs` around lines 147 - 149, Remove the stray double-comment prefix in the comment block around the include_bytes! explanation: locate the comment in build.rs that reads "// // this file and writes the bytes ..." and change it to a single comment prefix so it reads "// this file and writes the bytes ..."; ensure you only remove the extra "// " and do not alter surrounding text or meaning in that comment near the include_bytes! description.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@crates/cli/build.rs`:
- Around line 116-143: The current loop in build.rs copies every file in
release_dir matching dll_prefix/dll_suffix into libs_dir, which pulls in
unrelated cdylibs; restrict this by adding an explicit allow-list check on the
computed stem before performing the copy: after computing stem (from
strip_prefix/strip_suffix) validate stem against a const or env-provided
HashSet/List of accepted lint crate stems (e.g., allowed_stems containing the
real lint crate names) and only proceed to construct versioned, dest, fs::copy
and println! if stem is in that allow-list; update any error/context messages to
mention the allow-list filter and ensure allowed stems are populated from the
workspace configuration or a maintained constant to avoid leaking transitive
cdylibs to dlint::run.
- Around line 73-101: ensure_lints_dir and the lint build cache (lint_build_dir)
are not invalidated when LINTS_REPO_REVISION changes; add a sentinel check and
invalidation: when preparing the cached repo_dir/dylint_lints/Cargo.toml in
ensure_lints_dir, read the current HEAD (or store/compare a sentinel file) and
compare it to LINTS_REPO_REVISION; if they differ, remove both repo_dir and
lint_build_dir (or otherwise wipe the cache) so a fresh checkout and rebuild
occur; alternatively, if you prefer in-place update, run git fetch --depth 1
origin <LINTS_REPO_REVISION> && git checkout --detach FETCH_HEAD in the cached
repo and then force a rebuild by removing lint_build_dir if HEAD moved. Ensure
the code paths referencing lint_build_dir.exists(), ensure_lints_dir, repo_dir,
OUT_DIR, and LINTS_REPO_REVISION implement this compare-and-invalidate behavior
so builds are deterministic when the pinned revision changes.
In `@crates/cli/src/lint/mod.rs`:
- Around line 29-44: The LintArgs struct exposes a clippy flag that is parsed
but ignored in LintArgs::run; update run to handle self.clippy by either
invoking a clippy runner function (e.g., call a new run_clippy() alongside
run_dylint()) or return a clear error indicating clippy support is not yet
implemented; ensure you reference LintArgs::run, the self.clippy field, and the
existing run_dylint() symbol when adding the branch or error, and adjust any
docs (SKILLS.md/README.md) to match the chosen behavior or remove the clippy
field if you opt to postpone support.
---
Nitpick comments:
In `@crates/cli/build.rs`:
- Around line 147-149: Remove the stray double-comment prefix in the comment
block around the include_bytes! explanation: locate the comment in build.rs that
reads "// // this file and writes the bytes ..." and change it to a single
comment prefix so it reads "// this file and writes the bytes ..."; ensure you
only remove the extra "// " and do not alter surrounding text or meaning in that
comment near the include_bytes! description.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 012ae211-e1c4-4017-a5bd-1c5ef351149f
📒 Files selected for processing (6)
crates/cli/build.rscrates/cli/shared/ensure_toolchain_installed.rscrates/cli/src/common.rscrates/cli/src/lint/mod.rscrates/module-parser/src/metadata.rscrates/module-parser/src/source.rs
✅ Files skipped from review due to trivial changes (3)
- crates/cli/src/common.rs
- crates/module-parser/src/source.rs
- crates/module-parser/src/metadata.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/cli/shared/ensure_toolchain_installed.rs
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
2 participants
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.
Summary by CodeRabbit
New Features
lintwith optional Dylint support and added-p/--pathto target a workspace directory;--clippyis accepted but not yet invoked.Documentation
lintusage,--dylint,--clippy, and workspace path behavior.Chores
Tests