Skip to content

feat(diagnostics,minimal): incident collectors, mechanics in-crate - #864

Merged
norrietaylor merged 4 commits into
mainfrom
feat/diag-unit5-collectors
Jul 21, 2026
Merged

feat(diagnostics,minimal): incident collectors, mechanics in-crate#864
norrietaylor merged 4 commits into
mainfrom
feat/diag-unit5-collectors

Conversation

@norrietaylor

@norrietaylor norrietaylor commented Jul 21, 2026

Copy link
Copy Markdown
Member

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 diagnostics crate, parameterized by app inputs. crates/minimal keeps only the marker data and the six collect_step! wiring lines, holding the crate rule at series end (R5.5).

diagnostics::net (R5.1)listening_sockets (ss/netstat, Linux /proc/net fallback), 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-supplied markers: &[&str]. Linux path is pure /proc (wchan/syscall/kernel-stack/fd readlinks) so it runs as microVM pid-1 with no external binaries; macOS uses sample per pid + one 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.

diagnostics::capture::command_stdout (R5.4) — one lenient wrapper over command_capture that all three collectors consume; honors the lsof/ss exit-1-with-output convention.

Crate boundary

The crate never names a process or a bundle-path prefix — both markers and the dest group ("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

  • New Features
    • Diagnostic bundles now include process tree capture, live hang triage evidence, network listening sockets, network interfaces, network routes, and host power-state history (Unix platforms).
    • Incident diagnostics expand with these additional, platform-aware collectors.
  • Improved Output Handling
    • Non-zero command failures that still provide usable stdout are retained to improve bundle completeness.
  • Privacy
    • Network interface MAC masking preserves vendor/OUI while masking device-specific portions; sensitive process command-line values remain redacted.
  • Tests
    • Added an integration test that verifies incident outputs are present and MAC masking is enforced.

Note

Add incident collectors for process, network, and power diagnostics to minimal bug

  • Adds process_tree and hang_triage collectors in procs.rs: captures a filtered/scrubbed process table and, for up to 8 matched pids, gathers stack traces (macOS sample, Linux /proc wchan/syscall/stack) plus open file descriptors via lsof.
  • Adds network collectors in net.rs: listening sockets (ss/netstat//proc/net fallback), interfaces (ip addr/ifconfig with MAC addresses masked to vendor OUI), and routes (ip route/netstat -rn).
  • Adds a power collector in power.rs: records recent sleep/wake transitions from pmset (macOS) or journalctl (Linux).
  • Wires all new collectors into cmd_bug in mod.rs, keyed to process markers: min, minimal, minimald, minvmd, __krun-vmm, gvproxy.
  • Scrubbing is fail-closed: scrub_flattened withholds the entire argument tail after a sensitive key= token to prevent partial secret leakage from flattened ps output.

Macroscope summarized 2e19967.

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>
@coderabbitai

coderabbitai Bot commented Jul 21, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Changes

This 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

Layer / File(s) Summary
Command capture contract and module exports
crates/diagnostics/src/capture.rs, crates/diagnostics/src/lib.rs
Adds CaptureError::NonZero, exports command_stdout, and exposes the new Unix diagnostic modules.
Process and hang evidence
crates/diagnostics/src/procs.rs
Captures filtered process trees, per-process status, hang evidence, and file descriptors with platform-specific fallbacks and argv redaction.
Network state capture and MAC redaction
crates/diagnostics/src/net.rs
Captures sockets, interfaces, and routes with Linux fallbacks and OUI-preserving MAC masking.
Power history capture
crates/diagnostics/src/power.rs
Collects macOS or kernel journal sleep/wake data, caps event history, and writes power.txt.
Bug bundle wiring and integration validation
crates/minimal/src/diag/mod.rs, crates/minimal/tests/bug.rs
Adds process markers, invokes collectors, and validates generated files and masked MAC addresses.

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
Loading

Possibly related PRs

Suggested reviewers: twitchyliquid64

Poem

A rabbit packed the sockets tight,
And traced the processes through the night.
MACs wore masks, power logs grew,
Sleep and wake left footprints too.
Into the bug bundle they all hop!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title is concise, conventional, and accurately summarizes the main change: adding incident collectors to diagnostics/minimal.
Description check ✅ Passed The description is detailed and covers scope, rationale, and links, but it omits the template's Testing and Checklist sections.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch

Comment @coderabbitai help to get the list of available commands.

@norrietaylor

Copy link
Copy Markdown
Member Author

Aside: A/B'd interfaces against netdev (structural enumeration)

Prompted by the "could a crate do this better" question, I ran a local A/B of this PR's diagnostics::net::interfaces (shell ifconfig/ip + the mask_macs token heuristic) vs. netdev (structured enumeration, MAC masked as a field). Same host, en0:

A — existing collector

en0: flags=8863<UP,BROADCAST,SMART,RUNNING,SIMPLEX,MULTICAST> mtu 1500
	options=6460<TSO4,TSO6,CHANNEL_IO,PARTIAL_CSUM,ZEROINVERT_CSUM>
	ether 52:40:51:<redacted>
	inet 192.168.1.118 netmask 0xffffff00 broadcast 192.168.1.255
	nd6 options=201<PERFORMNUD,DAD>
	media: autoselect
	status: active

B — netdev

en0 (index=15, type=Wireless80211, mac=52:40:51:<redacted>)
    inet  192.168.1.118/24
    gateway mac=74:ac:b9:<redacted>

Findings

  1. MAC masking agrees exactly — regex masked 14 tokens, netdev reports 14 real MACs, identical lists. macOS ifconfig emits no brd ff:ff:ff:ff:ff:ff, so the broadcast over-mask is a Linux-only (ip addr) artifact — and even there it only over-masks a non-sensitive token. The regex has no false-negative (every real MAC matches the pattern), so it's leak-safe. netdev's structural masking is cleaner in theory but fixes nothing real here.
  2. netdev drops fidelityflags/mtu/options/nd6/media/status: active are gone. status: active and the up/running flags are exactly the "why is there no network" signal — the spec's "raw bytes are the deliverable" argument, concretely.
  3. Cost + new surface — netdev pulls ~20 crates on macOS (objc2-*, objc2-core-wlan, plist, quick-xml, time, …) and surfaces the gateway MAC the raw path never captured (a new masking obligation).

Verdict: confirms the spec's bespoke-collector decision for the host path — keep this collector as-is. netdev's real payoff is Unit 6 daemon-side (enumerating interfaces from inside the microVM with no ip/ifconfig present), not here. Noting rather than acting.

@norrietaylor
norrietaylor marked this pull request as ready for review July 21, 2026 16:50
@norrietaylor

Copy link
Copy Markdown
Member Author

@macroscope review

@macroscopeapp

macroscopeapp Bot commented Jul 21, 2026

Copy link
Copy Markdown

Manual reviews triggered for commit 72291f0:

All prior checks · these links stay valid even if you push more commits.

@macroscopeapp

macroscopeapp Bot commented Jul 21, 2026

Copy link
Copy Markdown

Just FYI for future @mentions, I'm Macroscope-App, not Macroscope.

Review in progress. Results will be posted as check runs when complete.

Comment thread crates/diagnostics/src/procs.rs
Comment thread crates/diagnostics/src/procs.rs Outdated
Comment thread crates/diagnostics/src/power.rs
@macroscopeapp

macroscopeapp Bot commented Jul 21, 2026

Copy link
Copy Markdown

Approvability

Verdict: 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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 4bfdcb0 and 72291f0.

📒 Files selected for processing (7)
  • crates/diagnostics/src/capture.rs
  • crates/diagnostics/src/lib.rs
  • crates/diagnostics/src/net.rs
  • crates/diagnostics/src/power.rs
  • crates/diagnostics/src/procs.rs
  • crates/minimal/src/diag/mod.rs
  • crates/minimal/tests/bug.rs

Comment thread crates/diagnostics/src/procs.rs
Comment thread crates/diagnostics/src/procs.rs
Comment thread crates/diagnostics/src/procs.rs
Comment thread crates/minimal/src/diag/mod.rs
norrietaylor and others added 2 commits July 21, 2026 11:44
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>
Comment thread crates/diagnostics/src/procs.rs
Comment thread crates/diagnostics/src/net.rs Outdated
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants