feat(diagnostics,minimal): incident collectors, mechanics in-crate - #864
Conversation
Unit 5 of the diagnostics series (epic #801). The wedged-system captures — network state, process tree, hang triage, and power history — land as mechanics in the `diagnostics` crate, parameterized by app inputs, with only the marker list and the six `collect_step!` wiring lines in `crates/minimal`. - `diagnostics::net` (R5.1): listening sockets (ss/netstat, Linux /proc/net fallback), interfaces with MACs masked to their vendor OUI, and routes — verbatim tool output, no typed re-serialization. - `diagnostics::procs` (R5.2): process table + hang triage for up to 8 pids matched by argv0 basename against a caller-supplied marker list. Linux is pure /proc (wchan/syscall/stack/fd), so it runs as microVM pid-1 with no external binaries; macOS uses `sample` + `lsof`. Recorded argv is scrubbed token-wise: any key=value token whose key trips the sensitive-key policy has its value replaced by the redaction placeholder. - `diagnostics::power` (R5.3): sleep/wake history (macOS pmset, Linux journalctl), event-capped. - All three run their command captures through a new lenient `diagnostics::capture::command_stdout`, which honors the lsof/ss exit-1-with-output convention (R5.4). The marker *data* (`PROCESS_MARKERS`) and the wiring stay in `minimal/src/diag/mod.rs`; the crate never names a process or a bundle path prefix — both are passed in — so the same mechanics serve the daemon-side capture (Unit 6). The net socket-probe stays out: it needs the CLI client and belongs to Unit 7. Refs: #801 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughChangesThis PR adds Unix diagnostic collectors for process, network, and power state, introduces command stdout handling for usable non-zero output, applies redaction, and integrates the collectors into bug bundles with end-to-end coverage. Incident diagnostics
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant cmd_bug
participant Diagnostics
participant Host
participant Bundle
cmd_bug->>Diagnostics: invoke process, network, and power collectors
Diagnostics->>Host: run commands or read platform data
Host-->>Diagnostics: diagnostic output
Diagnostics->>Bundle: redact and write incident artifacts
Bundle-->>cmd_bug: collector completion or recorded failure
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
Aside: A/B'd
|
|
@macroscope review |
|
Manual reviews triggered for commit All prior checks · these links stay valid even if you push more commits. |
|
Just FYI for future @mentions, I'm Review in progress. Results will be posted as check runs when complete. |
ApprovabilityVerdict: Needs human review 3 blocking correctness issues found. New feature adding ~600 lines of diagnostic collectors across three new modules. An unresolved high-severity comment identifies potential secret leakage in the redaction logic when secret values contain spaces, which warrants human review. You can customize Macroscope's approvability policy. Learn more. |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/diagnostics/src/procs.rs`:
- Around line 187-214: Move the synchronous proc_scrape invocation in
process_table onto a Tokio blocking thread using spawn_blocking, and await its
result within the existing collection flow. Preserve proc_scrape’s filesystem
logic and ensure both the join error and scrape result continue to propagate
through the current error-handling path so collect_step! can bound the async
task.
- Around line 247-255: Move the synchronous `/proc` operations in `proc_status`
off the async worker thread by invoking the function through `spawn_blocking`
from `process_tree`’s per-pid loop. Preserve the existing optional status
content and `OpenFds` counting behavior, while ensuring the async path awaits
the blocking task and handles task failure as no status result.
- Around line 89-136: Update hang_triage’s macOS sampling loop to avoid
sequential per-process timeouts exceeding the collector budget: run the sample
commands concurrently or enforce a shared overall deadline while preserving one
output/skip result per PID. Ensure lsof still runs within the remaining budget
and later PIDs are not abandoned because earlier sample calls each consume the
full timeout.
In `@crates/minimal/src/diag/mod.rs`:
- Around line 87-114: The host.hang-triage collection currently uses the shared
30-second collector budget, which is too short for its worst-case execution.
Update the collect_step invocation for diagnostics::procs::hang_triage to use a
dedicated timeout long enough for sampling up to eight PIDs and lsof, or
increase the applicable collector budget while preserving existing timeouts for
other steps.
🪄 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: 7b647057-f630-4389-9e0d-41ec4cf8003c
📒 Files selected for processing (7)
crates/diagnostics/src/capture.rscrates/diagnostics/src/lib.rscrates/diagnostics/src/net.rscrates/diagnostics/src/power.rscrates/diagnostics/src/procs.rscrates/minimal/src/diag/mod.rscrates/minimal/tests/bug.rs
Three review findings on the Unit 5 process collectors, all real: - `proc_scrape` walked all of /proc with synchronous std::fs from an async task, so the caller's `collect_step!` timeout could not preempt it. `ps` being absent — microVM pid-1, a starved host — is exactly when that read is most likely to wedge, and it would strand the worker thread instead of being bounded. It now runs on a blocking thread, matching the rule the CLI's state-listing collector already follows and the workspace standard against blocking in async. - `proc_status` had the same hazard from `process_tree`'s per-pid loop, with a smaller blast radius; same fix. - `hang_triage` sampled up to 8 pids sequentially at 15s each plus lsof — up to 130s against a 30s budget, so it was truncated mid-loop and later pids and lsof were lost, precisely on a wedged machine. The macOS samples now run concurrently, so the pass is bounded by one deadline rather than their sum: ps (5s) + samples (10s) + lsof (8s) ≈ 23s worst case. Raising the caller's budget instead would let `min bug` block for over two minutes on a hung host — the wrong direction for a tool whose job is to return evidence quickly. Sample results are sorted by pid so bundle contents stay deterministic regardless of completion order. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Macroscope review findings, both real and both in the direction that matters — under-masking: - `scrub_argv` split on `' '`, so a secret containing spaces was only half-masked: `--password=hunter two` masked `hunter` and emitted `two` verbatim. The cause is that argv boundaries are already gone by the time the text is scrubbed. Split by source: the `/proc` scrape keeps the real NUL-separated argv, so each element is now scrubbed whole (spaces included) with nothing over-redacted; `ps` output is irreversibly space-joined, so it is fail-closed — once a sensitive `key=` appears the rest of the line is withheld. A truncated process line is a far smaller loss than a partially-masked secret in a bundle that gets mailed out. - `mask_macs` documented itself as whitespace-delimited but split on `' '` only, so a tab-adjacent MAC (`ether\tf0:18:98:aa:bb:cc`) stayed inside a larger token and was emitted unmasked. It now walks maximal non-whitespace runs and copies every original separator byte-for-byte, so the capture still reads as the tool printed it. Also bound the journal read at the source (`journalctl -n`): the capture buffers a command's whole stdout before returning, and a boot's kernel journal can run to many megabytes of which 100 lines are kept. `-n` yields the newest lines, which is the end already filtered for. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Two more review findings on the collectors: - `hang_triage` took its pid set from one `process_table` snapshot and then read kernel state and open-file paths from those pids. If a matched process exits and its pid is recycled in between, the bundle records an unrelated process's fds and stack — exactly the "someone else's activity" the marker filter exists to keep out. Linux now re-checks each pid against its own `/proc/<pid>/cmdline` immediately before reading it, and only the pids that still match are handed to `lsof`; a pid that lost its identity is recorded as a skip. The read is async, so it costs no subprocess and no blocking. - `proc_net_listeners` walked tcp/tcp6/udp/unix but not udp6, so IPv6 UDP sockets vanished from the fallback capture on exactly the hosts that need it (no `ss`/`netstat`). Added. The macOS `sample` path keeps the snapshot pids: re-validating there costs another `ps` pass against a budget just tightened, and `sample` names the process it sampled in its own output, so a recycled pid is visible to the reader rather than silently misattributed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Unit 5 of the diagnostics series (spec: #802, epic: #801). Branches off
main— depends only on Units 1–2 (both merged), independent of the in-flight Unit 4 (#835). 750 insertions.What
The wedged-system captures — network state, process tree, hang triage, power history — land as mechanics in the
diagnosticscrate, parameterized by app inputs.crates/minimalkeeps only the marker data and the sixcollect_step!wiring lines, holding the crate rule at series end (R5.5).diagnostics::net(R5.1) —listening_sockets(ss/netstat, Linux/proc/netfallback),interfaces(MACs masked to their vendor OUI, token-wise so IPv6 shorthand survives),routes. Verbatim tool output, no typed re-serialization.diagnostics::procs(R5.2) — process table + hang triage for ≤8 pids matched by argv0 basename against a caller-suppliedmarkers: &[&str]. Linux path is pure/proc(wchan/syscall/kernel-stack/fd readlinks) so it runs as microVM pid-1 with no external binaries; macOS usessampleper pid + onelsof. Recorded argv is scrubbed token-wise: anykey=valuetoken whose key trips the sensitive-key policy has its value replaced by the redaction placeholder.diagnostics::power(R5.3) — sleep/wake history (macOSpmset, Linuxjournalctl), event-capped.diagnostics::capture::command_stdout(R5.4) — one lenient wrapper overcommand_capturethat all three collectors consume; honors thelsof/ssexit-1-with-output convention.Crate boundary
The crate never names a process or a bundle-path prefix — both
markersand thedestgroup ("host") are passed in — so the same mechanics serve the daemon-side capture (Unit 6, R6.5). The net socket-probe is intentionally not here: it needs the CLI client (crate::client::Client+minimald_rpc) and belongs to Unit 7.Refs: #801
🤖 Generated with Claude Code
Summary by CodeRabbit
Note
Add incident collectors for process, network, and power diagnostics to
minimal bugprocess_treeandhang_triagecollectors in procs.rs: captures a filtered/scrubbed process table and, for up to 8 matched pids, gathers stack traces (macOSsample, Linux/procwchan/syscall/stack) plus open file descriptors vialsof.ss/netstat//proc/netfallback), interfaces (ip addr/ifconfigwith MAC addresses masked to vendor OUI), and routes (ip route/netstat -rn).pmset(macOS) orjournalctl(Linux).cmd_bugin mod.rs, keyed to process markers:min,minimal,minimald,minvmd,__krun-vmm,gvproxy.scrub_flattenedwithholds the entire argument tail after a sensitivekey=token to prevent partial secret leakage from flattened ps output.Macroscope summarized 2e19967.