Pending changes exported from your codespace - #4
Open
ctx-eng wants to merge 4 commits into
Open
Conversation
Contributor
Reviewer's GuideIntroduce a unified timing/metrics subsystem, richer performance visualization and analysis (HTML dashboard, per-crate timings, historical trends), smarter CI and cache tooling, and feature/profile optimization plumbing across the CLI, while wiring everything together in main and docs. Sequence diagram for tracer HTML dashboard generation with trends and per-crate timingssequenceDiagram
participant Main as main
participant Tracer as tracer_run
participant Cargo as collect_per_crate_timings
participant Store as TimingStore
participant Trend as load_trend_data
participant HTML as generate_html_report
Main->>Tracer: run(TraceOptions)
Tracer->>Tracer: measure_phases(root)
Tracer->>Tracer: identify_bottlenecks(phases)
alt options.collect_per_crate
Tracer->>Cargo: collect_per_crate_timings(root)
Cargo-->>Tracer: Vec<PerCrateTiming>
else
Tracer->>Tracer: per_crate_data = Vec::new()
end
Tracer->>Trend: load_trend_data(root)
Trend->>Store: TimingStore::load(get_store_path(root))
Store-->>Trend: TimingStore
Trend-->>Tracer: HashMap<String, Vec<(u64,u64)>>
opt export_html
Tracer->>HTML: generate_html_report(&DashboardData)
HTML-->>Tracer: String (html)
Tracer->>Tracer: fs::write(html_path, html)
end
Tracer-->>Main: Result<()>
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
Warning Review limit reached
Next review available in: 9 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: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughAdds a persistent build-timing store used across benchmark, regression, trend, and tracer modules; introduces CPU-aware heuristics for codegen-units and parallel build checks; adds a dependency feature audit module and cross-crate split suggestions; extends CI with enforcement/policy options and remote cache validation; updates CLI, docs, and scripts. ChangesCargo Accelerate feature additions
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant CiModule
participant Filesystem
CLI->>CiModule: run(CiOptions{enforce_policy, budget})
CiModule->>CiModule: build_workflow(enforce_policy, budget)
CiModule->>Filesystem: write build.yml
CiModule->>Filesystem: write .cargo-accelerate/pre-commit.sh
CiModule->>CiModule: check_ci_parity()
CiModule->>CLI: print_enforcement_summary()
sequenceDiagram
participant BuildAudit
participant TimingStore
participant Disk
BuildAudit->>TimingStore: load(store_path)
BuildAudit->>TimingStore: record_build_run(command, duration)
TimingStore->>Disk: save(store_path)
Estimated code review effort: 4 (Complex) | ~75 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ 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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- The timestamp helpers (
chrono_like_timestamp,time_to_datetime,is_leap) are now implemented in bothtimings.rsandtracer.rs; consider centralizing these in a shared module to avoid duplication and keep their behavior consistent. - The
validate_remote_cachetests mutate process environment variables globally (and even useunsafearoundset_var/remove_var); consider wrapping env changes in a small helper that saves/restores the previous values to reduce flakiness when tests run in parallel and avoidunsafehere altogether. plan.mdappears to include AI-style narrative text (e.g., "I'll help you analyze the repository...") and a long internal plan; if this is not intended as user-facing documentation, consider either removing it from the repo or trimming it to a concise, project-appropriate design document.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The timestamp helpers (`chrono_like_timestamp`, `time_to_datetime`, `is_leap`) are now implemented in both `timings.rs` and `tracer.rs`; consider centralizing these in a shared module to avoid duplication and keep their behavior consistent.
- The `validate_remote_cache` tests mutate process environment variables globally (and even use `unsafe` around `set_var`/`remove_var`); consider wrapping env changes in a small helper that saves/restores the previous values to reduce flakiness when tests run in parallel and avoid `unsafe` here altogether.
- `plan.md` appears to include AI-style narrative text (e.g., "I'll help you analyze the repository...") and a long internal plan; if this is not intended as user-facing documentation, consider either removing it from the repo or trimming it to a concise, project-appropriate design document.
## Individual Comments
### Comment 1
<location path="src/timings.rs" line_range="31-32" />
<code_context>
+impl TimingStore {
+ pub fn load(path: &Path) -> Result<Self> {
+ if path.exists() {
+ let content = fs::read_to_string(path)?;
+ Ok(serde_json::from_str(&content).unwrap_or_default())
+ } else {
+ Ok(Self::default())
</code_context>
<issue_to_address>
**issue (bug_risk):** Avoid silently discarding corrupted timings.json by using `unwrap_or_default()`.
`serde_json::from_str(&content).unwrap_or_default()` makes any parse error indistinguishable from a legitimate "no data yet" case, which can mask file corruption or partial writes. Instead, consider propagating the `serde_json` error with `?`, or at least logging a warning and backing up/renaming the bad file before creating a new empty store so corruption is visible and not silently ignored.
</issue_to_address>
### Comment 2
<location path="src/tracer.rs" line_range="572-581" />
<code_context>
+fn chrono_like_timestamp(unix_ts: u64) -> String {
</code_context>
<issue_to_address>
**suggestion:** Time formatting helpers are duplicated between `timings.rs` and `tracer.rs`.
This module reimplements `chrono_like_timestamp`, `time_to_datetime`, and `is_leap` instead of reusing the versions in `timings.rs`. Please extract these helpers into a shared module (e.g., `timings` or a `utils` module) and call them from both places, adding small formatting/config options if needed rather than maintaining duplicate logic.
Suggested implementation:
```rust
fn chrono_like_timestamp(unix_ts: u64) -> String {
// Delegate to the shared implementation in the timings module
crate::timings::chrono_like_timestamp(unix_ts)
}
```
To fully implement your suggestion and remove duplicated logic:
1. Extract the shared helpers into a common module (e.g. `timings.rs` or `utils/time.rs`) if not already present:
- `pub fn chrono_like_timestamp(unix_ts: u64) -> String`
- `pub fn time_to_datetime(secs: i64, nanos: u32) -> (i32, u32, u32, u32, u32, u32)`
- `pub fn is_leap(year: i32) -> bool`
2. In `src/tracer.rs`, remove the local implementations of `time_to_datetime` and `is_leap` entirely and replace any internal calls with the shared versions:
- Add `use crate::timings::{chrono_like_timestamp, time_to_datetime, is_leap};` (or the appropriate module path).
- If you prefer to keep local wrapper fns for clarity, make them simple pass-through delegates as done for `chrono_like_timestamp`.
3. In `src/timings.rs` (or wherever the existing implementations live), ensure the functions are marked `pub` and are the single source of truth for the logic currently duplicated in `tracer.rs`.
4. Run `cargo fmt` and `cargo clippy` to confirm there are no unused functions left in `tracer.rs` and that all call sites now use the shared helpers.
</issue_to_address>
### Comment 3
<location path="plan.md" line_range="1" />
<code_context>
+I'll help you analyze the repository and create an improvement plan for a high-performance build system for Rust codebases. Let me start by gathering comprehensive information about the repository.Now let me explore the source directory structure and key files:Now I have a comprehensive view of the repository. Let me create a detailed improvement plan document.
+
+---
</code_context>
<issue_to_address>
**issue (typo):** Fix missing spaces and run-together sentences in the opening paragraph.
In particular, there are missing spaces after periods at "repository.Now" and "files:Now", which makes the sentences run together and harder to read.
```suggestion
I'll help you analyze the repository and create an improvement plan for a high-performance build system for Rust codebases. Let me start by gathering comprehensive information about the repository. Now let me explore the source directory structure and key files: Now I have a comprehensive view of the repository. Let me create a detailed improvement plan document.
```
</issue_to_address>
### Comment 4
<location path="plan.md" line_range="47-56" />
<code_context>
+ optimize.rs Auto-apply all optimizations to Cargo.toml/.cargo/config.toml
</code_context>
<issue_to_address>
**suggestion (typo):** Use a consistent filename: `optimize.rs` vs `optimizer.rs`.
The architecture overview lists `optimize.rs`, but the control flow section uses `optimizer.rs`. Please update both to use the same filename to avoid confusion for readers locating the source file.
Suggested implementation:
```
optimize.rs
```
The snippet you provided from the "How It's Organized" section already uses `optimize.rs`, so no change is needed there. The above edit assumes the "Control Flow" section (or any other sections) refer to `optimizer.rs`; those references should be updated to `optimize.rs` to keep the filename consistent with the architecture overview and the actual source file.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+31
to
+32
| let content = fs::read_to_string(path)?; | ||
| Ok(serde_json::from_str(&content).unwrap_or_default()) |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Avoid silently discarding corrupted timings.json by using unwrap_or_default().
serde_json::from_str(&content).unwrap_or_default() makes any parse error indistinguishable from a legitimate "no data yet" case, which can mask file corruption or partial writes. Instead, consider propagating the serde_json error with ?, or at least logging a warning and backing up/renaming the bad file before creating a new empty store so corruption is visible and not silently ignored.
Comment on lines
+572
to
+581
| fn chrono_like_timestamp(unix_ts: u64) -> String { | ||
| let secs = unix_ts as i64; | ||
| let nanos = 0u32; | ||
| let dt = time_to_datetime(secs, nanos); | ||
| format!( | ||
| "{:04}-{:02}-{:02} {:02}:{:02}", | ||
| dt.0, dt.1, dt.2, dt.3, dt.4 | ||
| ) | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
suggestion: Time formatting helpers are duplicated between timings.rs and tracer.rs.
This module reimplements chrono_like_timestamp, time_to_datetime, and is_leap instead of reusing the versions in timings.rs. Please extract these helpers into a shared module (e.g., timings or a utils module) and call them from both places, adding small formatting/config options if needed rather than maintaining duplicate logic.
Suggested implementation:
fn chrono_like_timestamp(unix_ts: u64) -> String {
// Delegate to the shared implementation in the timings module
crate::timings::chrono_like_timestamp(unix_ts)
}To fully implement your suggestion and remove duplicated logic:
- Extract the shared helpers into a common module (e.g.
timings.rsorutils/time.rs) if not already present:pub fn chrono_like_timestamp(unix_ts: u64) -> Stringpub fn time_to_datetime(secs: i64, nanos: u32) -> (i32, u32, u32, u32, u32, u32)pub fn is_leap(year: i32) -> bool
- In
src/tracer.rs, remove the local implementations oftime_to_datetimeandis_leapentirely and replace any internal calls with the shared versions:- Add
use crate::timings::{chrono_like_timestamp, time_to_datetime, is_leap};(or the appropriate module path). - If you prefer to keep local wrapper fns for clarity, make them simple pass-through delegates as done for
chrono_like_timestamp.
- Add
- In
src/timings.rs(or wherever the existing implementations live), ensure the functions are markedpuband are the single source of truth for the logic currently duplicated intracer.rs. - Run
cargo fmtandcargo clippyto confirm there are no unused functions left intracer.rsand that all call sites now use the shared helpers.
Comment on lines
+47
to
+56
| optimize.rs Auto-apply all optimizations to Cargo.toml/.cargo/config.toml | ||
| benchmark.rs Before/after build time comparison | ||
| profile.rs Scenario-aware build profiles (dev/test/ci/release) | ||
|
|
||
| cache.rs sccache management (enable/disable/remote) | ||
| linker.rs Fast linker configuration (mold/lld) | ||
| installer.rs Tool auto-installation (sccache, mold, nextest, etc.) | ||
|
|
||
| graph.rs Build graph analysis & critical path detection | ||
| workspace.rs Crate architecture analysis (LoC, compile weight) |
Contributor
There was a problem hiding this comment.
suggestion (typo): Use a consistent filename: optimize.rs vs optimizer.rs.
The architecture overview lists optimize.rs, but the control flow section uses optimizer.rs. Please update both to use the same filename to avoid confusion for readers locating the source file.
Suggested implementation:
optimize.rs
The snippet you provided from the "How It's Organized" section already uses optimize.rs, so no change is needed there. The above edit assumes the "Control Flow" section (or any other sections) refer to optimizer.rs; those references should be updated to optimize.rs to keep the filename consistent with the architecture overview and the actual source file.
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com> Signed-off-by: NxPKG <116948796+NxPKG@users.noreply.github.com>
PR Summary by QodoImplement timings storage, feature audit, CI enforcement, and interactive HTML dashboard
AI Description
Diagram
High-Level Assessment
Files changed (20)
|
Code Review by Qodo
1. Secrets logged in cache
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+141
to
+221
| // 2. Check environment variables | ||
| let endpoint = std::env::var("SCCACHE_ENDPOINT").ok(); | ||
| let bucket = std::env::var("SCCACHE_BUCKET").ok(); | ||
| let region = std::env::var("SCCACHE_REGION").ok(); | ||
| let dist_enabled = std::env::var("SCCACHE_DIST").ok(); | ||
|
|
||
| println!("\n Environment:"); | ||
| match &endpoint { | ||
| Some(v) => println!(" ✓ SCCACHE_ENDPOINT = {}", v.green()), | ||
| None => println!(" ✗ SCCACHE_ENDPOINT = {}", "not set".red()), | ||
| } | ||
| match &bucket { | ||
| Some(v) => println!(" ✓ SCCACHE_BUCKET = {}", v.green()), | ||
| None => println!(" ⚠ SCCACHE_BUCKET = {}", "not set (may not be needed)".yellow()), | ||
| } | ||
| match ®ion { | ||
| Some(v) => println!(" ✓ SCCACHE_REGION = {}", v.green()), | ||
| None => println!(" ⚠ SCCACHE_REGION = {}", "not set (may not be needed)".yellow()), | ||
| } | ||
| match &dist_enabled { | ||
| Some(v) if v == "true" || v == "1" => { | ||
| println!(" ✓ SCCACHE_DIST = {}", v.green()); | ||
| } | ||
| Some(v) => { | ||
| println!(" ⚠ SCCACHE_DIST = {}", v.yellow()); | ||
| println!(" Set SCCACHE_DIST=true to enable distributed caching."); | ||
| } | ||
| None => { | ||
| println!(" ⚠ SCCACHE_DIST = {}", "not set".yellow()); | ||
| println!(" Set SCCACHE_DIST=true to enable distributed caching."); | ||
| } | ||
| } | ||
|
|
||
| // 3. Test connectivity | ||
| println!("\n Connectivity test:"); | ||
| if endpoint.is_some() { | ||
| let start = std::time::Instant::now(); | ||
| let output = std::process::Command::new("sccache") | ||
| .args(["--dist-status"]) | ||
| .output(); | ||
| let elapsed = start.elapsed(); | ||
|
|
||
| match output { | ||
| Ok(out) => { | ||
| let stdout = String::from_utf8_lossy(&out.stdout); | ||
| let stderr = String::from_utf8_lossy(&out.stderr); | ||
| let combined = format!("{}{}", stdout, stderr); | ||
| let latency_ms = elapsed.as_millis(); | ||
|
|
||
| if out.status.success() { | ||
| if combined.to_lowercase().contains("connected") | ||
| || combined.to_lowercase().contains("ok") | ||
| || combined.to_lowercase().contains("available") | ||
| { | ||
| println!( | ||
| " ✓ Remote cache {} (latency: {}ms)", | ||
| "reachable".green(), | ||
| latency_ms.to_string().cyan() | ||
| ); | ||
| } else { | ||
| println!( | ||
| " ⚠ Remote cache responded but status unclear (latency: {}ms)", | ||
| latency_ms.to_string().cyan() | ||
| ); | ||
| } | ||
| } else { | ||
| println!( | ||
| " ✗ Remote cache {} (latency: {}ms)", | ||
| "unreachable".red(), | ||
| latency_ms.to_string().cyan() | ||
| ); | ||
| } | ||
|
|
||
| // Print any non-empty output | ||
| let trimmed = combined.trim(); | ||
| if !trimmed.is_empty() { | ||
| println!("\n sccache --dist-status output:\n"); | ||
| for line in trimmed.lines() { | ||
| println!(" {}", line.trim()); | ||
| } | ||
| } |
There was a problem hiding this comment.
1. Secrets logged in cache 🐞 Bug ⛨ Security
validate_remote_cache() prints SCCACHE_* environment variable values and echoes `sccache --dist-status` output to stdout, which can leak internal endpoints/credentials into CI logs. This is a security exposure when logs are persisted or publicly visible.
Agent Prompt
### Issue description
`validate_remote_cache()` prints potentially sensitive values (SCCACHE_ENDPOINT/BUCKET/REGION and command output) directly to stdout. In CI this can leak secrets or internal infrastructure details.
### Issue Context
This is a user-facing diagnostic command, but it is likely to be run in CI and the output ends up in logs.
### Fix Focus Areas
- src/cache.rs[141-221]
### Suggested fix
- Print only whether each env var is set, not the value (e.g., `SCCACHE_ENDPOINT = set`).
- If you still need values for troubleshooting, gate them behind an explicit `--verbose` / `--show-values` flag.
- Avoid printing full `sccache --dist-status` output by default; either summarize status or print only on verbose mode.
- Consider masking common secret patterns (URLs with credentials, tokens) if values must be shown.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+56
to
+86
| pub fn analyze_features(root: &Path) -> Result<FeatureAudit> { | ||
| let cargo_toml = get_cargo_toml_path(root); | ||
| if !cargo_toml.exists() { | ||
| return Ok(FeatureAudit::default()); | ||
| } | ||
|
|
||
| let content = fs::read_to_string(&cargo_toml)?; | ||
| let parsed: toml::Value = toml::from_str(&content)?; | ||
|
|
||
| let metadata = get_cached_metadata_with_deps()?; | ||
| let known = known_feature_suggestions(); | ||
|
|
||
| let mut suggestions = Vec::new(); | ||
|
|
||
| let direct_deps = parsed | ||
| .get("dependencies") | ||
| .and_then(|d| d.as_table()) | ||
| .map(|t| { | ||
| t.iter() | ||
| .map(|(k, _)| k.clone()) | ||
| .collect::<Vec<_>>() | ||
| }) | ||
| .unwrap_or_default(); | ||
|
|
||
| for dep_name in &direct_deps { | ||
| let package = metadata.packages.iter().find(|p| p.name == *dep_name); | ||
| let package = match package { | ||
| Some(p) => p, | ||
| None => continue, | ||
| }; | ||
|
|
There was a problem hiding this comment.
2. Feature audit ignores root 🐞 Bug ≡ Correctness
features::analyze_features(root) reads root/Cargo.toml but fetches cargo metadata via get_cached_metadata_with_deps() which is not scoped to root and is cached globally. This can produce incorrect/missing feature information when analyzing a different workspace path than the current process directory.
Agent Prompt
### Issue description
`analyze_features(root)` uses `get_cached_metadata_with_deps()` which runs `cargo metadata` without setting `current_dir` to `root`. Because metadata is cached globally, the audit can analyze the wrong workspace.
### Issue Context
`analyze_features` already takes a `root: &Path`, so it should use that consistently for both parsing Cargo.toml and running cargo metadata.
### Fix Focus Areas
- src/features.rs[56-86]
- src/utils.rs[31-38]
### Suggested fix
- Replace `get_cached_metadata_with_deps()` usage with a `cargo_metadata::MetadataCommand::new().current_dir(root).exec()?`.
- If caching is desired, cache by root path (keyed map) rather than a single global `OnceLock`.
- Add a test that uses a temp Cargo.toml with a dependency and verifies the metadata lookup is done relative to that temp root (or refactor to inject metadata for tests).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+28
to
+36
| impl TimingStore { | ||
| pub fn load(path: &Path) -> Result<Self> { | ||
| if path.exists() { | ||
| let content = fs::read_to_string(path)?; | ||
| Ok(serde_json::from_str(&content).unwrap_or_default()) | ||
| } else { | ||
| Ok(Self::default()) | ||
| } | ||
| } |
There was a problem hiding this comment.
3. Timings load drops history 🐞 Bug ☼ Reliability
TimingStore::load() uses serde_json::from_str(...).unwrap_or_default(), silently replacing corrupted or schema-incompatible timing files with an empty store. This can erase all historical timing data without any warning to the user.
Agent Prompt
### Issue description
`TimingStore::load()` silently falls back to a default (empty) store on parse errors, losing history.
### Issue Context
Timings are used for trend/regression/dashboard features; silent data loss makes these features unreliable.
### Fix Focus Areas
- src/timings.rs[28-36]
### Suggested fix
- On parse failure, return an error with context (preferred), or at least print a warning and preserve the original file (e.g., rename to `timings.json.bak`).
- Add `#[serde(default)]` to new/optional fields (e.g., `next_id`) so old files remain readable.
- Consider recomputing `next_id` as `max(id)+1` when loading if the stored value is missing/zero.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+131
to
+152
| fn collect_per_crate_timings(root: &Path) -> Result<Vec<PerCrateTiming>> { | ||
| // Run cargo build with --timings=json to get per-crate breakdown | ||
| let timings_dir = root.join("target").join("cargo-timings"); | ||
| // Clean previous timings so we only parse the new one | ||
| if timings_dir.exists() { | ||
| let _ = fs::remove_dir_all(&timings_dir); | ||
| } | ||
|
|
||
| let status = Command::new("cargo") | ||
| .args(["build", "--timings=json"]) | ||
| .current_dir(root) | ||
| .stdout(std::process::Stdio::null()) | ||
| .stderr(std::process::Stdio::null()) | ||
| .status() | ||
| .context("Failed to run cargo build --timings=json")?; | ||
|
|
||
| if !status.success() { | ||
| return Ok(Vec::new()); | ||
| } | ||
|
|
||
| parse_timings_json_dir(&timings_dir) | ||
| } |
There was a problem hiding this comment.
4. Trace runs extra build 🐞 Bug ➹ Performance
When --collect-timings is enabled, trace runs an additional cargo build --timings=json after already running cargo build during phase measurement, significantly increasing runtime and mutating build artifacts. It also deletes target/cargo-timings on each run, which can disrupt other tooling that relies on those files.
Agent Prompt
### Issue description
`trace --collect-timings` runs a second cargo build solely to collect timings and deletes `target/cargo-timings` beforehand. This is expensive and introduces side effects.
### Issue Context
`measure_phases()` already executes `cargo build` (plus test/clippy). The per-crate timings collection should avoid duplicating that work where possible.
### Fix Focus Areas
- src/tracer.rs[37-57]
- src/tracer.rs[131-152]
- src/tracer.rs[210-239]
### Suggested fix
- Run a single `cargo build --timings=json` when per-crate timings are requested, and reuse that run for the phase timing (or skip the separate build phase).
- Avoid deleting `target/cargo-timings`; instead, parse the newest timings file or write to a dedicated/unique directory.
- If a second build is unavoidable, clearly warn the user that it will rebuild and mutate artifacts.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+154
to
+183
| fn parse_timings_json_dir(dir: &Path) -> Result<Vec<PerCrateTiming>> { | ||
| if !dir.exists() { | ||
| return Ok(Vec::new()); | ||
| } | ||
| let mut results = Vec::new(); | ||
| let entries = fs::read_dir(dir)?; | ||
| for entry in entries { | ||
| let entry = entry?; | ||
| let path = entry.path(); | ||
| if path.extension().and_then(|s| s.to_str()) == Some("json") { | ||
| let content = fs::read_to_string(&path)?; | ||
| for line in content.lines() { | ||
| if line.trim().is_empty() { | ||
| continue; | ||
| } | ||
| if let Ok(val) = serde_json::from_str::<serde_json::Value>(line) { | ||
| if val.get("event").and_then(|v| v.as_str()) == Some("compiler_artifact") { | ||
| if let (Some(crate_name), Some(dur)) = ( | ||
| val.get("crate_name").and_then(|v| v.as_str()), | ||
| val.get("duration").and_then(|v| v.as_f64()), | ||
| ) { | ||
| results.push(PerCrateTiming { | ||
| crate_name: crate_name.to_string(), | ||
| duration_ms: dur as u64, | ||
| }); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
5. Per-crate json parsing fragile 🐞 Bug ≡ Correctness
parse_timings_json_dir() parses each timings file line-by-line and attempts to deserialize each line as a standalone JSON value, ignoring parse errors. If the timings output is a normal JSON document (not JSONL), per-crate timing data will be missing and the dashboard will be incorrect.
Agent Prompt
### Issue description
The per-crate timings parser assumes line-delimited JSON and ignores parse failures. This is brittle and can yield empty/incomplete results.
### Issue Context
The dashboard relies on `per_crate_data`; if parsing fails, users will see a fallback chart or incorrect breakdown.
### Fix Focus Areas
- src/tracer.rs[154-199]
### Suggested fix
- Parse the entire file content as JSON once (e.g., `serde_json::from_str(&content)`), then traverse the expected structure to extract crate durations.
- If multiple formats are possible, detect JSONL vs JSON document and support both.
- On parse failure, surface a warning that includes the file name and a short error summary so users know why per-crate data is missing.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
210
to
224
| None => { | ||
| println!(" ✔ codegen-units not set (defaults to 256 in dev)"); | ||
| score += 1; | ||
| let default_cu = 256i64; | ||
| if default_cu > overhead_threshold { | ||
| println!( | ||
| " ℹ codegen-units defaults to 256 — {} CPU cores detected, suggest reducing to {} (4× {} CPUs) to avoid overhead", | ||
| cpus, overhead_threshold, cpus | ||
| ); | ||
| } else { | ||
| println!( | ||
| " ✔ codegen-units defaults to 256 (well-matched to {} CPU cores)", | ||
| cpus | ||
| ); | ||
| score += 1; | ||
| } | ||
| } |
There was a problem hiding this comment.
6. Parallel audit zero silently 🐞 Bug ☼ Reliability
When codegen-units is unset and the default is deemed too high, check_parallel_build() prints only an informational message but returns score = 0 without adding an issue, so the audit summary can report fewer passed checks with no actionable reason. This makes the audit results misleading and hard to interpret, especially when used in CI gating.
Agent Prompt
### Issue description
`check_parallel_build()` can return 0 in a scenario where it only prints ℹ output and adds no issue. The audit summary uses the numeric score as "checks passed", so this yields confusing results.
### Issue Context
`build_audit::run()` increments `passed` by the returned score and prints `issues` separately.
### Fix Focus Areas
- src/build_audit.rs[28-71]
- src/build_audit.rs[210-241]
### Suggested fix
- Decide whether the ℹ case is a pass or a fail:
- If it’s informational, increment `score` (treat as pass).
- If it’s a problem, push an issue explaining what to change.
- Alternatively, return a richer result type (pass/warn/fail) instead of overloading an integer score.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![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: 13
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/regression.rs (1)
46-74: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRecord timings when no baseline exists too.
The new store is only saved through
compare_with_baseline; a firstregression --saveor any no-baseline regression run measuresstatsbut never writes those timings to the unified store.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/regression.rs` around lines 46 - 74, Record the measured timings in the unified store even when there is no baseline to compare against. In `run` within `regression.rs`, after `measure_current_builds` and any budget reporting, make sure the `TimingStore` is updated and saved for the no-baseline path as well, not only through `compare_with_baseline`; use the existing `store` and `store_path` flow so `--save` and first-run regressions persist `stats` consistently.
🟡 Minor comments (10)
src/ci.rs-252-266 (1)
252-266: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDon’t claim budget-only CI will fail.
build_workflow(false, Some(_))keepscontinue-on-error: true, but the summary always says “CI will fail on timeout” whenever a budget is set.Proposed fix
if let Some(budget) = opts.budget { println!( - " {} Regression budget set to {}s — CI will fail on timeout", + " {} Regression budget set to {}s — {}", "✓".green(), - budget.to_string().cyan() + budget.to_string().cyan(), + if opts.enforce_policy { + "CI will fail on timeout" + } else { + "CI will report regressions without failing" + } ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ci.rs` around lines 252 - 266, The CI summary in print_enforcement_summary is overclaiming failure behavior when only a budget is set. Update the budget message to distinguish between opts.enforce_policy and budget-only runs, so “CI will fail on timeout” is shown only when policy enforcement is enabled; otherwise describe the budget as reporting/recording timeouts without failing. Use the existing CiOptions fields and the print_enforcement_summary function to gate the wording appropriately.src/cache.rs-489-520 (1)
489-520: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winSerialize and restore the
SCCACHE_*env mutations in these tests. They mutate process-wide state and then remove it unconditionally, which can race with parallel tests or clobber pre-existing values. Use a shared test lock/serial helper and restore the previous values with RAII.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cache.rs` around lines 489 - 520, These tests mutate process-wide SCCACHE_* environment state in test_validate_remote_cache_with_env_vars and test_validate_remote_cache_without_dist, then unconditionally remove values, which can interfere with parallel tests or overwrite existing settings. Guard the tests with a shared serial/test lock and use RAII to save the prior values before set_var, then restore each variable’s original state after validate_remote_cache() completes.src/cli.rs-51-53 (1)
51-53: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject non-positive or non-finite CI budgets.
--budgetrepresents seconds, but the CLI currently accepts values like0, negative numbers, or non-finite floats, which can generate unusable enforcement settings.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cli.rs` around lines 51 - 53, The CLI budget option in the `budget` field currently accepts invalid seconds values, including zero, negatives, and non-finite floats. Add validation in `src/cli.rs` for the `budget: Option<f64>` argument (or a dedicated parser/helper used by the CLI) so only positive, finite values are accepted, and reject invalid inputs with a clear error before they reach downstream enforcement logic.src/timings.rs-120-142 (1)
120-142: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winTreat failed git commands as
unknown.When
git rev-parseexits non-zero,output()still returnsSome, so non-git workspaces can store an empty branch/commit instead ofunknown.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/timings.rs` around lines 120 - 142, Treat failed git lookups as unknown in get_git_branch and get_git_commit_hash: right now Command::output().ok() only handles process launch failures, so non-zero exits from git rev-parse can still produce empty strings. Update the logic in these functions to inspect the Command output status and only return the trimmed stdout when the command succeeds; otherwise fall back to "unknown" for both branch and commit hash.src/tracer.rs-51-54 (1)
51-54: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSurface per-crate collection failures when explicitly requested.
unwrap_or_default()hides I/O/toolchain failures, so users get an empty fallback chart with no explanation after passing--collect-timings.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tracer.rs` around lines 51 - 54, The per-crate timing collection in tracer::trace currently swallows failures with unwrap_or_default(), which hides errors when collect_per_crate is enabled. Update the collect_per_crate branch to handle the result from collect_per_crate_timings(&root) explicitly, surfacing any I/O or toolchain error to the caller instead of silently falling back, and keep the existing default-only behavior for the non-collection path.src/tracer.rs-445-445 (1)
445-445: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFix the invalid command in the empty-trend hint.
benchmark --saveis not defined in the provided CLI; this message sends users to a command that will fail. Usecargo accelerate benchmarkorcargo accelerate trend.Proposed fix
- document.getElementById('trend').innerHTML = '<p class="note">No historical timing data found. Run `cargo accelerate benchmark --save` or `cargo accelerate trend` to start collecting.</p>'; + document.getElementById('trend').innerHTML = '<p class="note">No historical timing data found. Run `cargo accelerate benchmark` or `cargo accelerate trend` to start collecting.</p>';🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tracer.rs` at line 445, The empty-trend hint in the tracer UI points users to an invalid command, so update the message in the trend rendering path to reference only supported commands. In the code that sets the `trend` element’s `innerHTML`, replace the `cargo accelerate benchmark --save` suggestion with a valid `cargo accelerate benchmark` or `cargo accelerate trend` invocation, keeping the rest of the hint text unchanged.src/utils.rs-81-82 (1)
81-82: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRemove the machine-size upper bound from the test.
Large CI or build hosts can legitimately expose 1024+ CPUs/threads, making this test fail despite
available_cpus()working correctly.Proposed fix
fn test_available_cpus_returns_positive() { let cpus = available_cpus(); assert!(cpus >= 1); - // Sanity check: unlikely to have more than 1024 CPUs - assert!(cpus < 1024); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils.rs` around lines 81 - 82, The test in available_cpus() currently hard-codes an upper-bound sanity check that asserts cpus is below 1024, which can fail on large CI/build hosts. Remove that machine-size limit from the test and keep only the assertions that validate the actual behavior of available_cpus(), using the available_cpus() test block or its surrounding assertion logic to locate it.src/timings.rs-88-99 (1)
88-99: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReturn the true median for even sample counts.
For an even number of timings, this returns the upper middle value instead of averaging the two middle values.
Proposed fix
} times.sort_by(|a, b| a.partial_cmp(b).unwrap_or(std::cmp::Ordering::Equal)); - Some(times[times.len() / 2]) + let mid = times.len() / 2; + if times.len() % 2 == 0 { + Some((times[mid - 1] + times[mid]) / 2.0) + } else { + Some(times[mid]) + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/timings.rs` around lines 88 - 99, The median_duration_ms method in Timings currently returns the upper middle value for even-sized samples instead of the true median. Update the median calculation logic after sorting the filtered times so that when the count is even, it averages the two middle values; keep the existing None behavior for empty input. Use the median_duration_ms function and its times vector as the place to make the change.plan.md-115-134 (1)
115-134: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winGap analysis describes issues this same PR already resolves.
This section (and the technical-debt table entry at line 329) states
timings.rsis empty/missing persistent storage as a current gap, but this PR stack implements the timing store elsewhere (src/timings.rs). If this document is meant as a forward-looking roadmap rather than a historical snapshot, consider adding a note clarifying which items are already implemented by this PR to avoid confusing future readers.Also applies to: 329-329
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plan.md` around lines 115 - 134, Clarify that the “Missing Performance Data Collection Layer” gap is already addressed by this PR so the roadmap does not contradict the codebase. Update the relevant plan section and the technical-debt table entry to mark the `timings.rs`/`src/timings.rs` storage work as implemented or add an explicit note that the item is historical/outdated, referencing the `timings.rs` and `regression.rs` items so future readers understand what remains versus what this PR already delivers.plan.md-1-1 (1)
1-1: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove leftover AI-assistant preamble from committed doc.
Line 1 reads as raw assistant chat output ("I'll help you analyze the repository...Now let me explore...") rather than document content. Trim it before merging as project documentation.
✏️ Proposed fix
-I'll help you analyze the repository and create an improvement plan for a high-performance build system for Rust codebases. Let me start by gathering comprehensive information about the repository.Now let me explore the source directory structure and key files:Now I have a comprehensive view of the repository. Let me create a detailed improvement plan document. - ---🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plan.md` at line 1, The document in plan.md contains leftover AI-assistant preamble instead of the intended plan content. Remove the conversational intro text from the start of the document and keep only the actual repository improvement plan, ensuring the final committed text reads like normal documentation.
🧹 Nitpick comments (6)
src/features.rs (1)
25-28: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse deterministic ordering for persisted suggestions.
HashMapserialization can reorderfeatures.tomlbetween runs, causing noisy diffs.BTreeMapkeeps the generated config stable with a small local change.Proposed refactor
-use std::collections::HashMap; +use std::collections::{BTreeMap, HashMap}; ... pub struct FeaturesConfig { - pub suggestions: HashMap<String, Vec<String>>, + pub suggestions: BTreeMap<String, Vec<String>>, } ... let mut config = FeaturesConfig { - suggestions: HashMap::new(), + suggestions: BTreeMap::new(), };Also applies to: 169-181
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/features.rs` around lines 25 - 28, Persisted suggestions use HashMap, which can serialize in non-deterministic order and churn the generated config. Update FeaturesConfig and the other matching suggestions type at the referenced struct declaration to use BTreeMap instead, keeping serialization stable while preserving the same data shape. Make the change in the relevant features config types and adjust any construction sites or imports that currently assume HashMap.src/graph.rs (1)
394-430: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winKeep the workspace package reference instead of re-looking it up by name.
find_workspace_member_dirsalready filters exact workspace packages, but Line 426 re-searches all metadata packages byname. Package names are not as safe as package identity across a full metadata graph, so this can associate the directory with the wrong package data when names collide.Suggested refactor
fn find_workspace_member_dirs<'a>( metadata: &'a cargo_metadata::Metadata, -) -> Vec<(&'a str, &'a Path)> { +) -> Vec<(&'a cargo_metadata::Package, &'a Path)> { metadata .packages .iter() .filter(|p| metadata.workspace_members.contains(&p.id)) .filter_map(|p| { let manifest = Path::new(&p.manifest_path); let dir = manifest.parent()?; - Some((p.name.as_str(), dir)) + Some((p, dir)) }) .collect() } @@ - for (name, dir) in &member_dirs { + for (package, dir) in member_dirs { + let name = package.name.as_str(); let loc = crate::workspace::count_rust_loc(dir).unwrap_or(0); - let package = metadata - .packages - .iter() - .find(|p| p.name.as_str() == *name) - .unwrap();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/graph.rs` around lines 394 - 430, The workspace member lookup in suggest_crate_splits is re-finding a package by name even though find_workspace_member_dirs already selected the exact workspace package, which can mismatch metadata when names collide. Update the flow to carry the package identity through from find_workspace_member_dirs and reuse that package reference directly in suggest_crate_splits instead of calling iter().find(...) on metadata.packages. Keep the fix localized around find_workspace_member_dirs and suggest_crate_splits so the directory is always paired with the correct package data.src/profile.rs (1)
1-1: 🚀 Performance & Scalability | 🔵 TrivialCPU detection may be inaccurate inside containers/CI runners.
available_cpus()relies onstd::thread::available_parallelism(), which per Rust's docs can overcount available parallelism in VMs/containers with CPU limits, and cgroup-based quota support has known gaps (e.g., cgroup v1 environments). Since this value now directly drivescodegen-unitswritten intoCargo.tomlforDev/Testprofiles viaapply_profile, a containerized CI runner reporting the host's full core count rather than its actual quota could produce an oversizedcodegen-unitsrecommendation.Worth keeping in mind if this heuristic is later extended to CI-generated profiles (
Scenario::Ci).Also applies to: 38-51
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/profile.rs` at line 1, The CPU-count heuristic used by apply_profile via available_cpus() can overestimate cores in containers/CI and lead to an oversized codegen-units value. Update the profile logic in profile.rs so apply_profile does not rely solely on available_parallelism-derived counts for Dev/Test; instead add a container/CI-aware fallback or cap based on quota-limited environments, and keep the behavior localized around available_cpus and apply_profile so future Scenario::Ci support can reuse the safer detection.src/cli.rs (1)
92-106: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd parser coverage for the new timing flags.
The new
Timingscommand and the positivetrace --collect-timingspath are not asserted in the parsing tests, so regressions in those CLI contracts could slip through.Also applies to: 490-517, 598-619
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cli.rs` around lines 92 - 106, Add parser test coverage for the new CLI contracts by extending the existing argument parsing tests around the Cli/Commands enum to assert the Timings subcommand parses with its command and last flags, and that the trace command accepts the positive --collect-timings path. Use the Cli parser entrypoint and the Timings and trace/collect_timings symbols so the new behavior is locked in and future regressions are caught.scripts/install_rust (2)
19-39: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate install/source logic between the two branches.
Lines 20-28 and 30-38 are identical except for the log message. Extract into a shared function to avoid drift if one branch is updated without the other.
♻️ Proposed refactor
+install_via_rustup() { + curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh $CI_FLAGS + CARGO_ENV_FILE="$HOME/.cargo/env" + if [[ -f "$CARGO_ENV_FILE" ]]; then + source "$CARGO_ENV_FILE" + else + echo -e "⚠️ ${red}Please start a new terminal session so that cargo is in your PATH.${reset}" + exit 1 + fi +} + if ! command -v cargo; then echo "⬇️ Installing rust..." - curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh $CI_FLAGS - CARGO_ENV_FILE="$HOME/.cargo/env" - if [[ -f "$CARGO_ENV_FILE" ]]; then - source "$CARGO_ENV_FILE" - else - echo -e "⚠️ ${red}Please start a new terminal session so that cargo is in your PATH.${reset}" - exit 1 - fi + install_via_rustup elif [[ ! -f "$HOME/.cargo/bin/cargo" ]]; then echo "⬇️ Cargo binary not found, reinstalling Rust..." - curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh $CI_FLAGS - CARGO_ENV_FILE="$HOME/.cargo/env" - if [[ -f "$CARGO_ENV_FILE" ]]; then - source "$CARGO_ENV_FILE" - else - echo -e "⚠️ ${red}Please start a new terminal session so that cargo is in your PATH.${reset}" - exit 1 - fi + install_via_rustup fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/install_rust` around lines 19 - 39, The Rust install script has duplicated install-and-source logic in the `if ! command -v cargo` and `elif [[ ! -f "$HOME/.cargo/bin/cargo" ]]` branches, which can drift over time. Extract the shared `curl`/`rustup` install, `CARGO_ENV_FILE` sourcing, and failure handling into a single helper/function in `scripts/install_rust`, then have both branches call it while keeping their specific log messages.
7-7: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider
set -o pipefailalongsideset -e.Without
pipefail, a failedcurlin thecurl | shpipelines (lines 21, 31) won't necessarily triggerset -esince onlysh's exit status is checked.🔧 Proposed fix
-set -e +set -eo pipefail🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/install_rust` at line 7, The script currently relies on set -e in install_rust, but the curl | sh pipelines can still hide curl failures because pipe exit status is not fully checked. Update the shell options near the top of the script to include pipefail alongside set -e so failures in the curl steps are propagated correctly; the relevant locations are the script’s top-level setup and the pipeline invocations in install_rust.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/cache.rs`:
- Around line 190-212: The remote cache status check in the connectivity summary
is treating a failed `sccache --dist-status` as only a warning, so the final
“configuration looks complete” message can still appear. Update the logic around
the `out.status.success()` branch in the cache status function so a non-zero
status records connectivity failure and prevents the overall summary from
reporting success; use the existing remote-cache status handling path (the same
block that prints reachable/unreachable) to propagate failure into the final
result, including the related summary path mentioned by the duplicate section.
- Around line 176-180: The sccache remote probe in the cache validation path can
block indefinitely when the scheduler is unreachable. Update the logic around
the `sccache --dist-status` call in the cache module to run with a timeout, and
handle timeout or process-hang cases gracefully by treating the probe as failed
instead of waiting forever. Use the existing endpoint check and the
`Command::new("sccache")` probe location to bound the execution and keep
`validate-remote` from hanging.
In `@src/ci.rs`:
- Around line 104-105: The audit workflow/templates are invoking a `--fail-fast`
flag that the audit CLI does not currently support, so update the generated
`Validate Build Policy` command and related hook/test references to stop passing
that option unless `main.rs`/audit routing actually implements it. Locate the
`audit` command handling in `src/main.rs` and the template generation in
`src/ci.rs`, then either add real `--fail-fast` support end-to-end or remove the
flag consistently from all generated workflow and pre-commit outputs.
- Around line 146-159: `run` is passing `CiOptions.budget` straight into
`build_workflow`, so invalid values can generate broken CI commands. Add
validation in `run` before the workflow is built: require `budget` to be finite
and greater than zero, and reject `0`, negative, `NaN`, or infinite inputs with
an error. If the same budget formatting is used elsewhere, apply the same guard
at the other `build_workflow` call site too so all workflow generation paths are
protected.
In `@src/features.rs`:
- Around line 56-78: The analyze_features path only reads the root Cargo.toml
and inspects its [dependencies], so workspace member manifests are missed and
workspaces can incorrectly report no dependencies. Update analyze_features to
use the metadata from get_cached_metadata_with_deps and iterate workspace member
manifest paths from metadata.workspace_members (or otherwise include
[workspace.dependencies]) so dependency collection is done from member
Cargo.toml files, while keeping the existing analyze_features and
known_feature_suggestions flow intact.
- Around line 229-235: The optimization logic in features.rs currently
overwrites any existing explicit features on table and inline-table dependencies
instead of preserving them. Update the dependency rewrite path that handles
dep.as_table_mut() and dep.as_inline_table_mut() to merge the current features
list with suggestion.recommended_features, de-duplicating the combined set, or
leave dependencies unchanged when features are already explicitly declared. Keep
the existing default-features handling, and make the change in the feature
optimization flow so --optimize does not drop user-specified features.
In `@src/graph.rs`:
- Line 425: The LoC scan in graph::build_split_candidates currently swallows
failures by using unwrap_or(0), which can write an incorrect splits.toml with
missing candidates. Update the count_rust_loc call site to propagate the error
instead of defaulting to zero, and add context that makes clear the failure
happened while scanning the workspace directory. Keep the fix localized around
the loc calculation in the split-candidate generation flow so the caller can
handle the error properly.
In `@src/timings.rs`:
- Around line 48-50: The public Timings::record method adds an externally
created BuildRun but does not advance next_id, so later
Timings::record_build_run calls can reuse an existing ID. Update record to
inspect the incoming run’s id and keep next_id in sync by setting it to one past
the highest recorded id, matching the behavior used by record_build_run and
preserving unique IDs for all future runs.
- Around line 29-32: The load logic in timings::load currently hides serde_json
parse failures by falling back to an empty store, which can overwrite existing
timings; change it to return the parse error with context instead of using
unwrap_or_default(), and after a successful deserialize, validate or recompute
the TimingStore next_id before returning so the loaded state stays consistent.
- Around line 220-222: The branch truncation in the timing logic uses byte
slicing, which can panic on UTF-8 boundaries. Update the branch handling in the
`timings.rs` logic around the `branch` assignment to truncate by characters
instead of bytes, using the existing `run.branch` value safely. Keep the same
behavior for short branch names, but for long ones derive the shortened string
from `chars()` so the `branch` variable is always valid UTF-8.
In `@src/tracer.rs`:
- Around line 564-566: Serialize the trend trace in tracer.rs instead of
interpolating cmd directly into the JSON string, since the current format string
in the trace-building code can inject unescaped command names into the dashboard
script. Update the trace creation logic around the scatter plot builder to
construct the payload with serde_json, following the same pattern used by the
other chart helpers, and ensure cmd is serialized as a JSON string rather than
concatenated into raw JavaScript.
- Around line 139-151: The timing collection in `parse_timings_json_dir` is
using `cargo build --timings=json`, which is no longer supported and won’t
produce the expected artifacts. Update the `Command::new("cargo")` invocation to
use a supported timing source such as `--timings` or `-Zbuild-analysis`, and
make sure failures are surfaced instead of silently returning `Ok(Vec::new())`
so the caller can see the error when `status.success()` is false.
In `@src/trend.rs`:
- Around line 97-104: The build trend recording in trend currently saves timings
even when cargo check or cargo build exits non-zero, because measure_cmd returns
a duration on failure. Update measure_cmd to return an error when the command
status is unsuccessful, and let the callers in trend skip
timings::record_build_run and store.save unless both measurements succeed. Use
the measure_cmd function and the build/check calls in trend as the fix points.
---
Outside diff comments:
In `@src/regression.rs`:
- Around line 46-74: Record the measured timings in the unified store even when
there is no baseline to compare against. In `run` within `regression.rs`, after
`measure_current_builds` and any budget reporting, make sure the `TimingStore`
is updated and saved for the no-baseline path as well, not only through
`compare_with_baseline`; use the existing `store` and `store_path` flow so
`--save` and first-run regressions persist `stats` consistently.
---
Minor comments:
In `@plan.md`:
- Around line 115-134: Clarify that the “Missing Performance Data Collection
Layer” gap is already addressed by this PR so the roadmap does not contradict
the codebase. Update the relevant plan section and the technical-debt table
entry to mark the `timings.rs`/`src/timings.rs` storage work as implemented or
add an explicit note that the item is historical/outdated, referencing the
`timings.rs` and `regression.rs` items so future readers understand what remains
versus what this PR already delivers.
- Line 1: The document in plan.md contains leftover AI-assistant preamble
instead of the intended plan content. Remove the conversational intro text from
the start of the document and keep only the actual repository improvement plan,
ensuring the final committed text reads like normal documentation.
In `@src/cache.rs`:
- Around line 489-520: These tests mutate process-wide SCCACHE_* environment
state in test_validate_remote_cache_with_env_vars and
test_validate_remote_cache_without_dist, then unconditionally remove values,
which can interfere with parallel tests or overwrite existing settings. Guard
the tests with a shared serial/test lock and use RAII to save the prior values
before set_var, then restore each variable’s original state after
validate_remote_cache() completes.
In `@src/ci.rs`:
- Around line 252-266: The CI summary in print_enforcement_summary is
overclaiming failure behavior when only a budget is set. Update the budget
message to distinguish between opts.enforce_policy and budget-only runs, so “CI
will fail on timeout” is shown only when policy enforcement is enabled;
otherwise describe the budget as reporting/recording timeouts without failing.
Use the existing CiOptions fields and the print_enforcement_summary function to
gate the wording appropriately.
In `@src/cli.rs`:
- Around line 51-53: The CLI budget option in the `budget` field currently
accepts invalid seconds values, including zero, negatives, and non-finite
floats. Add validation in `src/cli.rs` for the `budget: Option<f64>` argument
(or a dedicated parser/helper used by the CLI) so only positive, finite values
are accepted, and reject invalid inputs with a clear error before they reach
downstream enforcement logic.
In `@src/timings.rs`:
- Around line 120-142: Treat failed git lookups as unknown in get_git_branch and
get_git_commit_hash: right now Command::output().ok() only handles process
launch failures, so non-zero exits from git rev-parse can still produce empty
strings. Update the logic in these functions to inspect the Command output
status and only return the trimmed stdout when the command succeeds; otherwise
fall back to "unknown" for both branch and commit hash.
- Around line 88-99: The median_duration_ms method in Timings currently returns
the upper middle value for even-sized samples instead of the true median. Update
the median calculation logic after sorting the filtered times so that when the
count is even, it averages the two middle values; keep the existing None
behavior for empty input. Use the median_duration_ms function and its times
vector as the place to make the change.
In `@src/tracer.rs`:
- Around line 51-54: The per-crate timing collection in tracer::trace currently
swallows failures with unwrap_or_default(), which hides errors when
collect_per_crate is enabled. Update the collect_per_crate branch to handle the
result from collect_per_crate_timings(&root) explicitly, surfacing any I/O or
toolchain error to the caller instead of silently falling back, and keep the
existing default-only behavior for the non-collection path.
- Line 445: The empty-trend hint in the tracer UI points users to an invalid
command, so update the message in the trend rendering path to reference only
supported commands. In the code that sets the `trend` element’s `innerHTML`,
replace the `cargo accelerate benchmark --save` suggestion with a valid `cargo
accelerate benchmark` or `cargo accelerate trend` invocation, keeping the rest
of the hint text unchanged.
In `@src/utils.rs`:
- Around line 81-82: The test in available_cpus() currently hard-codes an
upper-bound sanity check that asserts cpus is below 1024, which can fail on
large CI/build hosts. Remove that machine-size limit from the test and keep only
the assertions that validate the actual behavior of available_cpus(), using the
available_cpus() test block or its surrounding assertion logic to locate it.
---
Nitpick comments:
In `@scripts/install_rust`:
- Around line 19-39: The Rust install script has duplicated install-and-source
logic in the `if ! command -v cargo` and `elif [[ ! -f "$HOME/.cargo/bin/cargo"
]]` branches, which can drift over time. Extract the shared `curl`/`rustup`
install, `CARGO_ENV_FILE` sourcing, and failure handling into a single
helper/function in `scripts/install_rust`, then have both branches call it while
keeping their specific log messages.
- Line 7: The script currently relies on set -e in install_rust, but the curl |
sh pipelines can still hide curl failures because pipe exit status is not fully
checked. Update the shell options near the top of the script to include pipefail
alongside set -e so failures in the curl steps are propagated correctly; the
relevant locations are the script’s top-level setup and the pipeline invocations
in install_rust.
In `@src/cli.rs`:
- Around line 92-106: Add parser test coverage for the new CLI contracts by
extending the existing argument parsing tests around the Cli/Commands enum to
assert the Timings subcommand parses with its command and last flags, and that
the trace command accepts the positive --collect-timings path. Use the Cli
parser entrypoint and the Timings and trace/collect_timings symbols so the new
behavior is locked in and future regressions are caught.
In `@src/features.rs`:
- Around line 25-28: Persisted suggestions use HashMap, which can serialize in
non-deterministic order and churn the generated config. Update FeaturesConfig
and the other matching suggestions type at the referenced struct declaration to
use BTreeMap instead, keeping serialization stable while preserving the same
data shape. Make the change in the relevant features config types and adjust any
construction sites or imports that currently assume HashMap.
In `@src/graph.rs`:
- Around line 394-430: The workspace member lookup in suggest_crate_splits is
re-finding a package by name even though find_workspace_member_dirs already
selected the exact workspace package, which can mismatch metadata when names
collide. Update the flow to carry the package identity through from
find_workspace_member_dirs and reuse that package reference directly in
suggest_crate_splits instead of calling iter().find(...) on metadata.packages.
Keep the fix localized around find_workspace_member_dirs and
suggest_crate_splits so the directory is always paired with the correct package
data.
In `@src/profile.rs`:
- Line 1: The CPU-count heuristic used by apply_profile via available_cpus() can
overestimate cores in containers/CI and lead to an oversized codegen-units
value. Update the profile logic in profile.rs so apply_profile does not rely
solely on available_parallelism-derived counts for Dev/Test; instead add a
container/CI-aware fallback or cap based on quota-limited environments, and keep
the behavior localized around available_cpus and apply_profile so future
Scenario::Ci support can reuse the safer detection.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cfdfcd11-c81b-4895-a5b9-ec1418e85d3b
📒 Files selected for processing (20)
README.mdplan.mdscripts/install_rustsrc/benchmark.rssrc/build_audit.rssrc/cache.rssrc/ci.rssrc/cli.rssrc/doctor.rssrc/features.rssrc/graph.rssrc/main.rssrc/optimizer.rssrc/profile.rssrc/regression.rssrc/timings.rssrc/tracer.rssrc/trend.rssrc/utils.rssrc/workspace.rs
Comment on lines
+176
to
+180
| if endpoint.is_some() { | ||
| let start = std::time::Instant::now(); | ||
| let output = std::process::Command::new("sccache") | ||
| .args(["--dist-status"]) | ||
| .output(); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Bound the sccache --dist-status probe with a timeout.
When SCCACHE_ENDPOINT is set, this external command can block indefinitely on an unreachable scheduler, hanging cargo accelerate cache validate-remote or CI automation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/cache.rs` around lines 176 - 180, The sccache remote probe in the cache
validation path can block indefinitely when the scheduler is unreachable. Update
the logic around the `sccache --dist-status` call in the cache module to run
with a timeout, and handle timeout or process-hang cases gracefully by treating
the probe as failed instead of waiting forever. Use the existing endpoint check
and the `Command::new("sccache")` probe location to bound the execution and keep
`validate-remote` from hanging.
Comment on lines
+190
to
+212
| if out.status.success() { | ||
| if combined.to_lowercase().contains("connected") | ||
| || combined.to_lowercase().contains("ok") | ||
| || combined.to_lowercase().contains("available") | ||
| { | ||
| println!( | ||
| " ✓ Remote cache {} (latency: {}ms)", | ||
| "reachable".green(), | ||
| latency_ms.to_string().cyan() | ||
| ); | ||
| } else { | ||
| println!( | ||
| " ⚠ Remote cache responded but status unclear (latency: {}ms)", | ||
| latency_ms.to_string().cyan() | ||
| ); | ||
| } | ||
| } else { | ||
| println!( | ||
| " ✗ Remote cache {} (latency: {}ms)", | ||
| "unreachable".red(), | ||
| latency_ms.to_string().cyan() | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Include failed connectivity in the final summary.
If sccache --dist-status exits non-zero, the function prints “unreachable” but can still print “Remote cache configuration looks complete” when the env vars are set.
Proposed fix
println!("\n Connectivity test:");
+ let mut remote_reachable = None;
if endpoint.is_some() {
let start = std::time::Instant::now();
let output = std::process::Command::new("sccache")
@@
if out.status.success() {
+ remote_reachable = Some(true);
if combined.to_lowercase().contains("connected")
@@
} else {
+ remote_reachable = Some(false);
println!(
@@
Err(e) => {
+ remote_reachable = Some(false);
println!(
@@
let all_set = sccache_available
&& endpoint.is_some()
&& dist_enabled
.as_deref()
.map(|v| v == "true" || v == "1")
- .unwrap_or(false);
+ .unwrap_or(false)
+ && remote_reachable != Some(false);Also applies to: 238-245
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/cache.rs` around lines 190 - 212, The remote cache status check in the
connectivity summary is treating a failed `sccache --dist-status` as only a
warning, so the final “configuration looks complete” message can still appear.
Update the logic around the `out.status.success()` branch in the cache status
function so a non-zero status records connectivity failure and prevents the
overall summary from reporting success; use the existing remote-cache status
handling path (the same block that prints reachable/unreachable) to propagate
failure into the final result, including the related summary path mentioned by
the duplicate section.
Comment on lines
+104
to
+105
| - name: Validate Build Policy | ||
| run: cargo accelerate audit --fail-fast |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Don’t generate --fail-fast until the audit CLI supports it.
The generated workflow and hook call cargo accelerate audit --fail-fast, but the provided src/main.rs audit routing only handles skip_size, skip_rustflags, skip_features, and skip_parallel. As generated, policy CI/pre-commit can fail before the audit runs; either wire a real --fail-fast audit option or remove the flag from the templates/tests.
Also applies to: 138-139, 378-378, 388-388, 393-393
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ci.rs` around lines 104 - 105, The audit workflow/templates are invoking
a `--fail-fast` flag that the audit CLI does not currently support, so update
the generated `Validate Build Policy` command and related hook/test references
to stop passing that option unless `main.rs`/audit routing actually implements
it. Locate the `audit` command handling in `src/main.rs` and the template
generation in `src/ci.rs`, then either add real `--fail-fast` support end-to-end
or remove the flag consistently from all generated workflow and pre-commit
outputs.
Comment on lines
+146
to
159
| pub fn run(opts: CiOptions) -> Result<()> { | ||
| println!( | ||
| "{}", | ||
| "Generating Optimized CI Configurations...".bold().cyan() | ||
| ); | ||
|
|
||
| let root = get_project_root().context("Could not find project root")?; | ||
|
|
||
| // 1. Create GitHub Actions workflow | ||
| // 1. Create GitHub Actions workflow with conditional enforcement | ||
| let github_dir = root.join(".github").join("workflows"); | ||
| fs::create_dir_all(&github_dir)?; | ||
|
|
||
| let workflow = build_workflow(opts.enforce_policy, opts.budget); | ||
| let workflow_path = github_dir.join("build.yml"); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Validate budget before writing the workflow.
budget comes from CLI as Option<f64> and is interpolated directly, so 0, negative, NaN, or infinite values can produce nonsensical CI commands.
Proposed fix
pub fn run(opts: CiOptions) -> Result<()> {
+ if let Some(budget) = opts.budget {
+ anyhow::ensure!(
+ budget.is_finite() && budget > 0.0,
+ "--budget must be a positive finite number of seconds"
+ );
+ }
+
println!(Also applies to: 226-233
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ci.rs` around lines 146 - 159, `run` is passing `CiOptions.budget`
straight into `build_workflow`, so invalid values can generate broken CI
commands. Add validation in `run` before the workflow is built: require `budget`
to be finite and greater than zero, and reject `0`, negative, `NaN`, or infinite
inputs with an error. If the same budget formatting is used elsewhere, apply the
same guard at the other `build_workflow` call site too so all workflow
generation paths are protected.
Comment on lines
+56
to
+78
| pub fn analyze_features(root: &Path) -> Result<FeatureAudit> { | ||
| let cargo_toml = get_cargo_toml_path(root); | ||
| if !cargo_toml.exists() { | ||
| return Ok(FeatureAudit::default()); | ||
| } | ||
|
|
||
| let content = fs::read_to_string(&cargo_toml)?; | ||
| let parsed: toml::Value = toml::from_str(&content)?; | ||
|
|
||
| let metadata = get_cached_metadata_with_deps()?; | ||
| let known = known_feature_suggestions(); | ||
|
|
||
| let mut suggestions = Vec::new(); | ||
|
|
||
| let direct_deps = parsed | ||
| .get("dependencies") | ||
| .and_then(|d| d.as_table()) | ||
| .map(|t| { | ||
| t.iter() | ||
| .map(|(k, _)| k.clone()) | ||
| .collect::<Vec<_>>() | ||
| }) | ||
| .unwrap_or_default(); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Analyze workspace member manifests, not only the workspace root.
run() passes get_project_root(), and that resolves to metadata.workspace_root; for typical workspaces, member dependencies live in member Cargo.toml files, so a root manifest with only [workspace] reports “No dependencies found to analyze.” Iterate metadata.workspace_members/manifest paths or include [workspace.dependencies] instead of only root [dependencies].
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/features.rs` around lines 56 - 78, The analyze_features path only reads
the root Cargo.toml and inspects its [dependencies], so workspace member
manifests are missed and workspaces can incorrectly report no dependencies.
Update analyze_features to use the metadata from get_cached_metadata_with_deps
and iterate workspace member manifest paths from metadata.workspace_members (or
otherwise include [workspace.dependencies]) so dependency collection is done
from member Cargo.toml files, while keeping the existing analyze_features and
known_feature_suggestions flow intact.
Comment on lines
+220
to
+222
| let branch = if run.branch.len() > 10 { | ||
| &run.branch[..10] | ||
| } else { |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Avoid byte slicing branch names.
&run.branch[..10] can panic if a UTF-8 branch name crosses a character boundary. Truncate by chars() instead.
Proposed fix
- let branch = if run.branch.len() > 10 {
- &run.branch[..10]
- } else {
- &run.branch
- };
+ let branch = run.branch.chars().take(10).collect::<String>();
println!(
"{:<4} {:<20} {:<10} {:<12} {:<12} {:<10} {:<10}",
run.id, ts, run.command, duration, run.profile, branch, label📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let branch = if run.branch.len() > 10 { | |
| &run.branch[..10] | |
| } else { | |
| let branch = run.branch.chars().take(10).collect::<String>(); | |
| println!( | |
| "{:<4} {:<20} {:<10} {:<12} {:<12} {:<10} {:<10}", | |
| run.id, ts, run.command, duration, run.profile, branch, label |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/timings.rs` around lines 220 - 222, The branch truncation in the timing
logic uses byte slicing, which can panic on UTF-8 boundaries. Update the branch
handling in the `timings.rs` logic around the `branch` assignment to truncate by
characters instead of bytes, using the existing `run.branch` value safely. Keep
the same behavior for short branch names, but for long ones derive the shortened
string from `chars()` so the `branch` variable is always valid UTF-8.
Comment on lines
+139
to
+151
| let status = Command::new("cargo") | ||
| .args(["build", "--timings=json"]) | ||
| .current_dir(root) | ||
| .stdout(std::process::Stdio::null()) | ||
| .stderr(std::process::Stdio::null()) | ||
| .status() | ||
| .context("Failed to run cargo build --timings=json")?; | ||
|
|
||
| if !status.success() { | ||
| return Ok(Vec::new()); | ||
| } | ||
|
|
||
| parse_timings_json_dir(&timings_dir) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
git ls-files src/tracer.rs src | sed -n '1,120p'Repository: khulnasoft/cargo-accelerate
Length of output: 516
🏁 Script executed:
ast-grep outline src/tracer.rs --view expandedRepository: khulnasoft/cargo-accelerate
Length of output: 2400
🏁 Script executed:
rg -n "timings|collect_per_crate|benchmark --save|--timings=json|cargo build" src -SRepository: khulnasoft/cargo-accelerate
Length of output: 6702
🌐 Web query:
Cargo --timings=json output stdout path docs
💡 Result:
As of July 2026, the --timings=json option in Cargo has been removed and is no longer supported [1]. Previously, this was an unstable feature that required the -Zunstable-options flag [2][3]. When it was available, it emitted JSON-formatted messages directly to stdout [1][3]. It did not output to a file, unlike the standard --timings flag, which writes an HTML report to the target/cargo-timings/ directory [4][5][6]. If you are looking for machine-readable build timing information, current documentation recommends using -Zbuild-analysis instead [1].
Citations:
- 1: fix(timing)!: remove
--timings=<FMT>optional format values rust-lang/cargo#16420 - 2: Update
--timings=jsonoutput include data similar to the HTML output rust-lang/cargo#10463 - 3: Tracking issue for -Ztimings rust-lang/cargo#7405
- 4: https://dev-doc.rust-lang.org/cargo/reference/timings.html
- 5: https://doc.rust-lang.org/cargo/commands/cargo-build.html
- 6: https://doc.rust-lang.org/cargo/reference/timings.html
🏁 Script executed:
sed -n '131,170p' src/tracer.rsRepository: khulnasoft/cargo-accelerate
Length of output: 1640
🏁 Script executed:
sed -n '80,100p' src/cli.rsRepository: khulnasoft/cargo-accelerate
Length of output: 894
🏁 Script executed:
sed -n '420,455p' src/tracer.rsRepository: khulnasoft/cargo-accelerate
Length of output: 1608
Replace --timings=json with a supported timing source
cargo build --timings=json is no longer supported on current Cargo, and the old JSON mode emitted data on stdout rather than target/cargo-timings/. This path now falls back to an empty dashboard on failure; surface the error and switch to a supported source (--timings/-Zbuild-analysis) instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/tracer.rs` around lines 139 - 151, The timing collection in
`parse_timings_json_dir` is using `cargo build --timings=json`, which is no
longer supported and won’t produce the expected artifacts. Update the
`Command::new("cargo")` invocation to use a supported timing source such as
`--timings` or `-Zbuild-analysis`, and make sure failures are surfaced instead
of silently returning `Ok(Vec::new())` so the caller can see the error when
`status.success()` is false.
Comment on lines
97
to
+104
| println!(" Measuring current build times..."); | ||
| let check_time = measure_cmd(&root, "check")?; | ||
| let build_time = measure_cmd(&root, "build")?; | ||
|
|
||
| // Record in unified timings store | ||
| timings::record_build_run(&mut store, "check", check_time, "trend", Some("trend-check")); | ||
| timings::record_build_run(&mut store, "build", build_time, "trend", Some("trend-build")); | ||
| store.save(&store_path)?; |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Don’t persist failed cargo measurements as valid trend data.
measure_cmd returns elapsed time even if cargo check or cargo build exits non-zero, and these lines now save that duration into the unified timing history. Make measure_cmd fail on non-success before recording.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/trend.rs` around lines 97 - 104, The build trend recording in trend
currently saves timings even when cargo check or cargo build exits non-zero,
because measure_cmd returns a duration on failure. Update measure_cmd to return
an error when the command status is unsuccessful, and let the callers in trend
skip timings::record_build_run and store.save unless both measurements succeed.
Use the measure_cmd function and the build/check calls in trend as the fix
points.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: ctx-eng <drlizabhola@gmail.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: ctx-eng <drlizabhola@gmail.com>
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.
Description
Related Issue
Type of Change
Checklist
cargo checkcompiles without warningscargo testpassescargo fmthas been runcargo clippyis cleanSummary by Sourcery
Add persistent build timing collection, richer tracing dashboard, CI policy/budget controls, feature flag auditing, and cross-crate split guidance to improve performance observability and optimization across Rust workspaces.
New Features:
Enhancements:
Documentation:
Tests:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation