Modularize workspace into focused crates + fix/watch/hooks/baseline + BDD receipt contract - #5
Conversation
…g capabilities - Introduced `builddiag-fix` crate for deterministic auto-fix planning and application of build-contract issues, including support for workspace MSRV, resolver settings, and checksum entries. - Implemented `plan_fixes` and `apply_fixes` functions for planning and applying fixes with options for dry-run and interactive confirmation. - Added tests for various fix scenarios to ensure correct functionality. - Created `builddiag-watch` crate to provide a polling watch loop for monitoring changes in key files and re-running checks. - Implemented customizable watch options, including debounce timing and terminal notifications. - Added tests to verify the watch loop functionality and file tracking.
- Updated `rust-version` in `Cargo.toml` files for all crates to 1.92. - Updated `rust-toolchain.toml` files to reflect the new Rust channel 1.92.0. - Enhanced descriptions, readme, and documentation links in `Cargo.toml` for better clarity and accessibility. - Added new README.md files for several crates to provide detailed information about their purpose and functionality. - Introduced new features and improvements in the `builddiag-watch` crate, including desktop notifications for status changes. - Added new `builddiag-hooks` crate for generating deterministic hook snippets for Git and Husky. - Improved test coverage and added new feature tests for receipt contracts.
Use `if let Ok(...)` instead of `if let Some(...).ok()` (match_result_ok) and collapse nested `if let` blocks (collapsible_if).
|
Warning Review limit reached
More reviews will be available in 51 minutes and 55 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?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 credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. 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, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (108)
✨ 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 |
Summary of ChangesHello @EffortlessSteven, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly expands the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces four new crates (builddiag-fix, builddiag-watch, builddiag-hooks, builddiag-baseline), adding significant new capabilities like auto-fixing, file watching, hook generation, and baseline comparisons. The Rust version is also updated across the workspace. The changes are well-structured and follow the project's architecture. I've found a potential issue with the inline suppression logic that could lead to over-suppression of findings, and a suggestion to improve the auto-fix functionality to preserve Cargo.toml formatting. Overall, this is a great feature addition to the project.
| if location.line.is_none() | ||
| && file_rules | ||
| .line_scoped | ||
| .values() | ||
| .flatten() | ||
| .any(|selector| selector_matches(selector, finding)) | ||
| { | ||
| return true; |
There was a problem hiding this comment.
This logic could lead to over-suppression of findings. It allows any line-scoped suppression comment in a file to suppress a finding that has no line number associated with it. A finding without a line number is typically a file-level finding and should only be matched by file-scoped suppressions (comments on their own line).
For example, if a file has a line-scoped suppression for issue_A on line 10, this logic would also suppress a file-level finding for issue_B if that finding lacks a line number.
If the intent is to suppress findings like wildcard_version which are conceptually tied to a line but might not have a line number from the parser, it would be more robust to ensure the finding generator (depguard) provides a line number for the finding.
I recommend removing this block. A finding without a line number should only be suppressible by file-scoped rules, which are already handled by the logic on lines 385-391.
| fn apply_workspace_manifest_changes( | ||
| manifest_path: &Utf8Path, | ||
| workspace_msrv: Option<&str>, | ||
| set_resolver_v2: bool, | ||
| ) -> Result<bool> { | ||
| let raw = fs::read_to_string(manifest_path).with_context(|| format!("read {manifest_path}"))?; | ||
| let mut value: toml::Value = | ||
| toml::from_str(&raw).with_context(|| format!("parse {manifest_path}"))?; | ||
|
|
||
| let root = value | ||
| .as_table_mut() | ||
| .ok_or_else(|| anyhow!("manifest root is not a table: {manifest_path}"))?; | ||
| let workspace = ensure_table(root, "workspace")?; | ||
|
|
||
| let mut changed = false; | ||
|
|
||
| if set_resolver_v2 && workspace.get("resolver").and_then(toml::Value::as_str) != Some("2") { | ||
| workspace.insert("resolver".to_string(), toml::Value::String("2".to_string())); | ||
| changed = true; | ||
| } | ||
|
|
||
| if let Some(msrv) = workspace_msrv { | ||
| let package = ensure_table(workspace, "package")?; | ||
| if package.get("rust-version").and_then(toml::Value::as_str) != Some(msrv) { | ||
| package.insert( | ||
| "rust-version".to_string(), | ||
| toml::Value::String(msrv.to_string()), | ||
| ); | ||
| changed = true; | ||
| } | ||
| } | ||
|
|
||
| if !changed { | ||
| return Ok(false); | ||
| } | ||
|
|
||
| let rendered = toml::to_string_pretty(&value) | ||
| .with_context(|| format!("render updated manifest {manifest_path}"))?; | ||
| fs::write(manifest_path, rendered).with_context(|| format!("write {manifest_path}"))?; | ||
|
|
||
| Ok(true) | ||
| } |
There was a problem hiding this comment.
The current implementation uses toml::from_str and toml::to_string_pretty to modify the Cargo.toml file. This approach will reformat the entire file and may remove comments, which could be an undesirable side effect for users of builddiag fix.
Consider using the toml_edit crate, which is designed to parse and modify TOML files while preserving formatting and comments. This would make the auto-fix feature less intrusive.
There was a problem hiding this comment.
Pull request overview
This PR introduces four new crates to expand builddiag's developer experience capabilities: builddiag-fix (auto-fix planning/application), builddiag-watch (polling watch loop with debounce), builddiag-hooks (Git/Husky hook snippet generation), and builddiag-baseline (baseline snapshot comparisons and inline suppressions). The entire workspace is also updated to Rust 1.92 with comprehensive documentation improvements and enhanced Cargo.toml metadata for all crates.
Changes:
- Four new crates with deterministic auto-fix, watch mode, hook generation, and baseline/suppression filtering capabilities
- Workspace-wide version bump from 0.2.0 to 0.3.0 with Rust toolchain update to 1.92
- New CLI subcommands (
fix,watch,init-hooks,baseline create/update) with comprehensive BDD test coverage
Reviewed changes
Copilot reviewed 68 out of 69 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
Cargo.toml |
Workspace members updated to include 4 new crates, version bumped to 0.3.0 |
crates/builddiag-watch/ |
New crate for polling watch loop with debounce and desktop notifications |
crates/builddiag-fix/ |
New crate for deterministic auto-fix planning and application |
crates/builddiag-hooks/ |
New crate for hook snippet generation (pre-commit, Git, Husky) |
crates/builddiag-baseline/ |
New crate for baseline snapshots and inline suppression filtering |
crates/builddiag-cli/src/main.rs |
Added new subcommands and baseline/suppression filtering logic |
crates/builddiag-cli/tests/ |
New integration tests for baseline workflows and inline suppressions |
crates/builddiag-cli/tests/features/ |
New BDD scenarios for receipt contract and extended checks |
fixtures/golden/ |
Regenerated golden files for Rust 1.92 and version 0.3.0 |
fixtures/conformance/valid-workspace/ |
Updated rust-toolchain.toml to 1.92.0 |
xtask/tests/metadata_properties.rs |
Updated workspace crate list to include all new crates |
docs/ |
Updated architecture diagrams, crate documentation table, and integration examples |
ROADMAP.md, CHANGELOG.md, README.md |
Documentation updates reflecting new features as released |
All */Cargo.toml |
Enhanced metadata with description, readme, documentation, and keywords |
All */README.md |
New per-crate README files with API documentation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 83d5314a90
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| use std::process::Command; | ||
| use std::time::{Duration, UNIX_EPOCH}; | ||
|
|
||
| const WATCHED_FILE_NAMES: &[&str] = &["Cargo.toml", "rust-toolchain.toml", "checksums.txt"]; |
There was a problem hiding this comment.
WATCHED_FILE_NAMES is hard-coded to checksums.txt, but builddiag’s default checksums input is scripts/tools.sha256, so builddiag watch will not rerun when the checksum file changes in the common/default setup. This causes stale watch results and can hide newly introduced checksum policy violations until some other watched file changes.
Useful? React with 👍 / 👎.
| let git_dir = root.join(".git"); | ||
| if !git_dir.exists() { | ||
| return Err(anyhow::anyhow!( |
There was a problem hiding this comment.
This check only verifies that .git exists, then assumes it is a directory. In git worktrees/submodules, .git is a file, so this passes and the later create_dir_all(<root>/.git/hooks) path fails with Not a directory, making builddiag init-hooks --install fail in those valid repository layouts.
Useful? React with 👍 / 👎.
…ity host check - builddiag-fix: drop redundant `.into_iter()` (clippy::useless_conversion). - builddiag-cli/bdd: drop `as usize` no-op casts (clippy::unnecessary_cast). - builddiag-app: recover poisoned git test mutex (parallel-run safety) and disable GPG signing in test git commands so CI without GPG passes. - xtask: normalize host.os and host.arch in the conformance comparison so the library-parity check works regardless of which OS regenerated the goldens. All tests pass, clippy clean with `-D warnings`, and `xtask conform` shows 10/10 passing.
…ract Extract cross-cutting logic out of builddiag-app/builddiag-checks into dependency-light crates and split the check implementations: - builddiag-paths: deterministic repo-relative, forward-slash path helpers - builddiag-receipt: builddiag.report.v1 -> sensor.report.v1 transformation - builddiag-output-contract: report/sensor contract validation entry points - builddiag-checks-catalog: check id/docs/severity registry + explain lookup - builddiag-checks-checksums, builddiag-checks-deps: split check impls - builddiag-testkit: shared fs/repo/cli test support Add the bdd-receipt-contract spec and new feature files (check_catalog, path_normalization) that assert full receipt payloads (findings, verdict reasons/data, capabilities, artifacts, schema ids) rather than just exit code and top-level verdict. Add a fuzz_paths target. Bump workspace to 0.3.0.
…rates # Conflicts: # crates/builddiag-checks/src/lib.rs # fixtures/golden/all-disabled.native.report.json # fixtures/golden/all-disabled.report.json # fixtures/golden/missing-msrv.native.report.json # fixtures/golden/missing-msrv.report.json # fixtures/golden/valid-workspace.native.report.json # fixtures/golden/valid-workspace.report.json
test_main_uses_injected_args and test_main_args_falls_back_to_env both mutate the global MAIN_ARGS. Under parallel execution it was possible for the fallback test to set MAIN_ARGS to None *after* the injected test set Some(...) but *before* its main() call consumed the slot, making main() fall through to env args, which clap rejects with exit(2) and tears down the whole test process. Gate both tests behind a new MAIN_ARGS_TEST_LOCK so they run serially relative to each other while other tests remain parallel.
EffortlessSteven
changed the title
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_e6ae7a9f-0094-4671-9181-7ed8855299e3) |
The wrapper functions and dispatch arms for the checksums and deps checks call into the optional `builddiag-checks-checksums` / `builddiag-checks-deps` crates unconditionally, so building `builddiag-checks` with the `checksums` or `deps` feature disabled failed to compile (E0433: unlinked crate). The catalog already feature-gates the corresponding `BUILTIN_CHECKS` entries, so the dispatch arms are unreachable when the feature is off; gate the wrappers and arms with the matching `#[cfg(feature = ...)]` to match. Verified: `builddiag-checks` now builds with checksums/deps individually or both disabled; clippy --all-features and the default test suite stay green.
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_609905c1-638b-4966-ae01-882b8bf495dd) |
Replace the crate-local `rel_path` helper with the canonical `builddiag_paths::to_repo_relative`, centralizing path normalization in builddiag-paths as intended by the refactor, and drop the now-unused `camino` direct dependency. Output is unchanged (conform 10/10, all tests green).
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_83119ebb-04cd-49ae-bfc1-36b7cd513719) |
Summary
This PR introduces new capability crates and modularizes the workspace into
focused, dependency-light crates, while tightening the BDD receipt contract.
New capability crates (earlier commits)
builddiag-fix— deterministic auto-fix planner/applier (MSRV, resolver, checksums)builddiag-watch— polling watch loop with debounce and desktop notificationsbuilddiag-hooks— pre-commit/Git/Husky hook snippet generationbuilddiag-baseline— baseline snapshot comparisons / report deltasModularization (latest commit)
Logic extracted out of
builddiag-app/builddiag-checksinto small crates:builddiag-paths— deterministic repo-relative, forward-slash path helpersbuilddiag-receipt—builddiag.report.v1→sensor.report.v1transformationbuilddiag-output-contract— report/sensor contract validation entry pointsbuilddiag-checks-catalog— check id/docs/severity registry + explain lookupbuilddiag-checks-checksums,builddiag-checks-deps— split check implementationsbuilddiag-testkit— shared fs/repo/cli test supportBDD receipt contract
Adds the
bdd-receipt-contractspec and new feature files (check_catalog,path_normalization) that assert full receipt payloads — findings bycheck_id/code/severity, verdict reasons/data, capabilities, artifacts, schema
ids — rather than only exit code and top-level verdict. Receipt/sensor
assertions are now centralized in
builddiag-output-contract. Adds afuzz_pathstarget. Workspace bumped to 0.3.0.Verification (local, Windows)
cargo clippy --all-targets --all-features -- -D warnings— cleancargo test --all— all pass (incl. 57 cucumber scenarios)cargo fmt --all --check— cleancargo run -p xtask -- conform— 10/10 (schema, determinism, survivability,layout, golden, tool-error, library-parity, native-schema, verdict-contract,
native-golden); golden files regenerated after merge with main
Notes
main(PRs Fix CI parallel agents and improve test robustness #6–test: cover builddiag-repo cache and loader error paths #11) into the branch; resolved one test-configconflict in
builddiag-checksand regenerated golden fixtures.plan/ Release (cargo-dist) check is a pre-existing failure on thisrepo's PRs (the custom publish section makes
dist plan's freshness checkfail; same failure on merged PR test: cover builddiag-repo cache and loader error paths #11). When that workflow is regenerated, the
new publishable crates should be added to the publish order.
Note
Medium Risk
Large crate split and orchestration/receipt moves affect every check run path; risk is mitigated by unchanged receipt schemas and heavy test/conform coverage, but integration surface area grew substantially.
Overview
v0.3.0 splits the monolith into focused crates (paths, receipt, output-contract, checks-catalog/checksums/deps, testkit, baseline, fix, watch, hooks) and bumps the whole workspace from 0.2.0.
Orchestration moves sensor/receipt building out of
builddiag-appintobuilddiag-receipt(re-exported APIs, optionalsubstratecapability on repo-state runs). Checks delegate checksum and dependency logic to new microcrates and pull metadata frombuilddiag-checks-catalogbehind feature flags.CLI/product wires
builddiag baseline(regression-only--baseline+ inlinebuilddiag:ignore),watch,fix, andinit-hooks; README/ROADMAP mark Phase 1 DX items done.BDD adds the
bdd-receipt-contractKiro spec (requirements/design for fullbuilddiag.report.v1/sensor.report.v1cucumber assertions and a coverage audit);builddiag-output-contractis a CLI dev-dependency for test validation.Docs, crate READMEs, and GitHub repo metadata are refreshed for the expanded 12-crate layout.
Reviewed by Cursor Bugbot for commit 4cc31af. Bugbot is set up for automated code reviews on this repo. Configure here.