Repository navigation
feat(minimal,diagnostics): add the min bug host diagnostic bundle command - #816
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 3 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 ignored due to path filters (1)
📒 Files selected for processing (9)
📝 WalkthroughWalkthroughAdds reusable diagnostics utilities and a new ChangesDiagnostic bundle
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant Collectors
participant Diagnostics
participant Bundle
CLI->>Collectors: Run timed host collectors
Collectors->>Diagnostics: Gather and redact diagnostic data
Diagnostics-->>Collectors: Return facts, logs, and redacted values
Collectors->>Bundle: Write entries and manifest records
Bundle-->>CLI: Finalize archive and report counts
Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
2325e59 to
9a3c1b9
Compare
…mand Add the bug subcommand: a manifest-bearing minimal-diag-<ts>.tar.zst assembled through the diagnostics crate's BundleWriter. Host collectors (system, env, dirs report, config, state listing, logs) run under per-collector timeouts; any failure is recorded in the manifest and the run continues. The command mutates no state and spawns no daemons. Reusable mechanics land in the diagnostics crate so the daemon-side bundle can share them: redact_toml beside redact_json, masked_process_env over a caller-supplied allowlist predicate, newest_rotated log selection, disk_usage (statvfs), and first_stdout_line over command_capture. The CLI keeps only policy: the env allowlist, log prefixes, config paths, and archive layout. Refs: #801 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
9a3c1b9 to
444c1df
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
crates/diagnostics/src/redact.rs (1)
110-136: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant
is_sensitive_key(key)in leaf check.
child_maskalready includesis_sensitive_key(key)(line 114), so thechild_mask || is_sensitive_key(key)check on line 120 is always justchild_mask. This mirrors the JSON version's pattern but is dead logic. Consider simplifying toif child_maskfor clarity.♻️ Simplify redundant condition
leaf => { - if child_mask || is_sensitive_key(key) { + if child_mask { *leaf = toml_placeholder(leaf); } }🤖 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 `@crates/diagnostics/src/redact.rs` around lines 110 - 136, In redact_toml_value, simplify the leaf masking condition to check only child_mask, since it already incorporates is_sensitive_key(key). Preserve the existing placeholder replacement behavior.crates/minimal/tests/bug.rs (1)
50-65: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winPlant private-key canary content before asserting it is excluded.
No
client.keyis created, while production unconditionally records its skip. Add a distinct canary key file and verify that its bytes appear nowhere in the archive; this makes the privacy regression test meaningful.Also applies to: 146-168
🤖 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 `@crates/minimal/tests/bug.rs` around lines 50 - 65, Add a distinct canary private-key file such as client.key under the temporary configuration directory created by fake_config_dir, then update the archive assertions in the related test to verify the key’s bytes are absent. Keep the existing secret-redaction assertions and ensure the new canary content is not included anywhere in the archive.
🤖 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 `@crates/minimal/src/diag/collect.rs`:
- Around line 230-242: Update add_redacted_toml to inspect the source with
symlink-safe metadata before reading it, accept only regular files, and avoid
following symbolic links. Record rejected symlinks via BundleWriter::skip as
withheld, while preserving the existing not-found and unreadable handling for
valid regular files.
- Around line 278-279: The diagnostics collector must not perform the
synchronous recursive walk inline in the future poll. Update the code around
diagnostics::listing to run the traversal on a blocking-capable worker or
replace it with an asynchronous, cancellation-aware implementation, while
preserving the existing “listing state dir” context and allowing the surrounding
30-second timeout to remain effective.
- Around line 187-188: Update provider_dirs and its callers to return and
propagate a Result, preserving an empty result only for NotFound filesystem
errors. Replace the silent if-let/while-let handling around directory reads and
the additional filesystem operations at the referenced locations so permission
and transient I/O failures reach collect_step! and are recorded in the manifest.
---
Nitpick comments:
In `@crates/diagnostics/src/redact.rs`:
- Around line 110-136: In redact_toml_value, simplify the leaf masking condition
to check only child_mask, since it already incorporates is_sensitive_key(key).
Preserve the existing placeholder replacement behavior.
In `@crates/minimal/tests/bug.rs`:
- Around line 50-65: Add a distinct canary private-key file such as client.key
under the temporary configuration directory created by fake_config_dir, then
update the archive assertions in the related test to verify the key’s bytes are
absent. Keep the existing secret-redaction assertions and ensure the new canary
content is not included anywhere in the archive.
🪄 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: 03159cbb-cefd-4806-96d0-fd4ede02a864
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
crates/diagnostics/Cargo.tomlcrates/diagnostics/src/capture.rscrates/diagnostics/src/disk.rscrates/diagnostics/src/lib.rscrates/diagnostics/src/logs.rscrates/diagnostics/src/redact.rscrates/minimal/Cargo.tomlcrates/minimal/src/diag/collect.rscrates/minimal/src/diag/mod.rscrates/minimal/src/diag/redact.rscrates/minimal/src/dirs.rscrates/minimal/src/lib.rscrates/minimal/tests/bug.rs
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/diagnostics/src/redact.rs (1)
110-136: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant
is_sensitive_key(key)check on line 120.
child_maskon line 114 already incorporatesis_sensitive_key(key), sochild_mask || is_sensitive_key(key)on line 120 is always equivalent tochild_mask. Simplifying improves clarity and avoids implying there's a case wherechild_maskis false butis_sensitive_key(key)is true.♻️ Proposed simplification
leaf => { - if child_mask || is_sensitive_key(key) { + if child_mask { *leaf = toml_placeholder(leaf); } }🤖 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 `@crates/diagnostics/src/redact.rs` around lines 110 - 136, In redact_toml_value, simplify the leaf masking condition to rely only on child_mask, since it already includes is_sensitive_key(key). Remove the redundant second is_sensitive_key(key) check while preserving the existing placeholder behavior.
🤖 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.
Nitpick comments:
In `@crates/diagnostics/src/redact.rs`:
- Around line 110-136: In redact_toml_value, simplify the leaf masking condition
to rely only on child_mask, since it already includes is_sensitive_key(key).
Remove the redundant second is_sensitive_key(key) check while preserving the
existing placeholder behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 92f6e2cf-243a-485b-a6d7-a0c186fcfa07
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
crates/diagnostics/Cargo.tomlcrates/diagnostics/src/capture.rscrates/diagnostics/src/disk.rscrates/diagnostics/src/lib.rscrates/diagnostics/src/logs.rscrates/diagnostics/src/redact.rscrates/minimal/Cargo.tomlcrates/minimal/src/diag/collect.rscrates/minimal/src/diag/mod.rscrates/minimal/src/diag/redact.rscrates/minimal/src/dirs.rscrates/minimal/src/lib.rscrates/minimal/tests/bug.rs
🚧 Files skipped from review as they are similar to previous changes (12)
- crates/minimal/Cargo.toml
- crates/minimal/src/diag/redact.rs
- crates/diagnostics/src/disk.rs
- crates/diagnostics/src/capture.rs
- crates/diagnostics/src/lib.rs
- crates/minimal/tests/bug.rs
- crates/minimal/src/lib.rs
- crates/diagnostics/Cargo.toml
- crates/minimal/src/dirs.rs
- crates/diagnostics/src/logs.rs
- crates/minimal/src/diag/mod.rs
- crates/minimal/src/diag/collect.rs
Three review findings on the host collectors: - Filesystem errors are no longer reported as absence: provider_dirs returns Result (NotFound alone means never-spawned), and the loadouts and log-dir reads record non-NotFound errors instead of claiming the data does not exist. - The no-follow discipline now covers every content read, not just log tails: the O_NOFOLLOW + fstat open moves from add_file_tail into a shared open_regular_nofollow, and config TOMLs and the mesh-enrolment file read through it, so a symlinked config file cannot steer unrelated data into the bundle. - The synchronous listing walk runs under spawn_blocking so a wedged filesystem strands a blocking thread instead of the worker whose collect_step timeout is the failsafe; the crate fn documents the contract. Refs: #801 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Reverse-lexicographic name order is only correct for date-suffixed rolling files. logrotate-style numeric shifting (live file bare, .1 the most recently rotated) sorts in exactly the opposite direction — name order would select the oldest files and drop the live log. Modified-time order is rotation-scheme agnostic; name order (descending) breaks ties. Refs: #801 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| // ── host/system.json ───────────────────────────────────────────────────────── | ||
|
|
||
| #[derive(Serialize)] | ||
| struct SystemInfo { |
There was a problem hiding this comment.
This might be useful to collect in all our binaries - maybe move to diagnostics crate ?
There was a problem hiding this comment.
Done in 80206fc — SystemInfo and its probes (uname facts, euid, KVM availability, disk capacity) now live in diagnostics::system; the disk paths stay caller policy (system_info(&[state, cache, cwd])), so any binary can open its bundle with the same facts. The CLI's direct libc dependency went with the probe.
…tics crate Every bundle-producing binary wants the same opening facts (OS, arch, kernel, hostname, cpu count, euid, KVM availability, disk capacity), so SystemInfo and its probes move from the CLI collector into diagnostics::system, with the disk paths staying caller policy. The CLI keeps only its serialization into host/system.json, and its direct libc dependency goes with the probe. Refs: #801 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Unit 2 of the diagnostics series (spec: #802, epic: #801; depends on Unit 1, merged in #803).
What
min bug [--output <path>]assembles aminimal-diag-<UTC ts>.tar.zstthrough thediagnosticscrate'sBundleWriter::create(rooted,0600), honoring the same--minimal-dir/--config-diroverrides as the rest of the CLI. The command mutates nostate and spawns no daemons — diagnosing a wedged system must not change it.
Mechanics in-crate, policy in the CLI
Reusable mechanics land in
crates/diagnosticsrather than the CLI, since thedaemon-side bundle (Unit 6) needs the same behaviors — its own rotated volume logs,
allowlisted guest env, volume disk usage:
redact::redact_toml(beside the existingredact_json)redact::masked_process_env(is_allowed)logs::newest_rotated(dir, prefix, n)disk::disk_usage(statvfs)capture::first_stdout_lineuname)Example bundle
min bugon a dev Mac with one prior VM run — 8 entries, 0 errors, 17 ms:host/system.jsonhost/env.jsonHOME/SHELL/TERMverbatim per the allowlist,USER/LANGas<redacted:len=N>host/dirs.txtmin dirstableconfig/loadouts/dev.toml.redacted[vars]value masked (including a 1170-byteMINIMAL_MOTD)config/client-cert.jsonstate/listing.txtlogs/minvmd.log.2026-07-16providers/local-0/run.logmanifest.jsonThe manifest's four skips explain every absence:
config/client.key(nevercollected),
logs/minimald.log*(no files with that prefix — those begin toexist with Unit 3),
providers/local-0/boot.log(absent), andproviders/(1 instance present, daemon-side state not collected).
Refs: #801
🤖 Generated with Claude Code
Summary by CodeRabbit
min bugcommand to create a self-contained compressed diagnostic bundle.