Skip to content

feat: add Windows platform support#83

Closed
csvkse wants to merge 1 commit intograykode:mainfrom
csvkse:feat/windows-support
Closed

feat: add Windows platform support#83
csvkse wants to merge 1 commit intograykode:mainfrom
csvkse:feat/windows-support

Conversation

@csvkse
Copy link
Copy Markdown

@csvkse csvkse commented Apr 29, 2026

Summary

  • Add sysinfo crate for Windows process info collection
  • Implement map_pid_to_sysinfo_open_paths using sysinfo
  • Implement get_process_info using sysinfo on Windows
  • Implement get_listening_ports using netstat on Windows
  • Add Windows-specific file_identity using size+mtime
  • Fix cmd_has_binary to handle Windows paths and .exe suffix

Test plan

  • Build on Windows with cargo build
  • Run abtop and verify process detection works
  • Test session discovery with Claude Code on Windows
  • Test port detection

- Add sysinfo dependency for Windows process info collection
- Implement map_pid_to_sysinfo_open_paths for Windows
- Implement get_process_info using sysinfo on Windows
- Implement get_listening_ports using netstat on Windows
- Add Windows-specific file_identity using size+mtime
- Fix cmd_has_binary to handle Windows paths and .exe suffix
yangbinbin48 pushed a commit to yangbinbin48/abtop that referenced this pull request May 1, 2026
Port process info collection, port discovery, file identity, and
open-path resolution to Windows using sysinfo crate and netstat.
Based on upstream PR graykode#83 by csvkse with API fixes for sysinfo 0.32.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Copy link
Copy Markdown
Owner

@graykode graykode left a comment

Choose a reason for hiding this comment

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

thanks for tackling Windows. security pass is clean — sysinfo + a hardcoded netstat -ano invocation, no new network/shell-injection/dotfile surface. a few correctness things before this can land:

  1. get_process_info will report cpu_pct ~= 0 forever on Windows. sysinfo's Process::cpu_usage() is a delta between refreshes — the docs explicitly say "this value will be 0 unless you call refresh_processes at least twice" with at least MINIMUM_CPU_UPDATE_INTERVAL between them. each tick this code does System::new() + one refresh_processes_specifics(...), so cpu_usage stays at 0 every tick. that breaks has_active_descendant (cpu_pct > cpu_threshold never fires) and the Working/Waiting threshold in app.rs. fix is either to keep a long-lived System between ticks and refresh twice, or to compute CPU% from proc.run_time() / lifetime ticks like the linux path does. as-is, every Windows session will look idle.

  2. cmd_has_binary change leaks Windows semantics into linux/macOS. the old impl was tok.rsplit('/').next() == name (case-sensitive, no .exe strip). the new impl splits on \ too, strips .exe, and uses eq_ignore_ascii_case. those are the right rules on Windows but they silently change matching on every other platform — Claude and claude.exe now match claude on linux/macOS. please gate the windows-only bits behind #[cfg(windows)] (or split into two impls), so non-windows behavior is unchanged.

  3. doc comment on cmd_has_binary is misleading. it says "Also accepts process name on Windows when command is empty," but the function doesn't do that — the proc.name() fallback lives in get_process_info. either move the comment or reword it.

minor:

  • src/collector/process.rs:332 clippy nit: rsplit(|c| c == '/' || c == '\\')rsplit(['/', '\\']).
  • map_pid_to_sysinfo_open_paths returns an empty paths vec because sysinfo 0.32 has no fd_list. the comment is honest about that, but it does mean the open-file discovery fallback that lsof/libproc provide is gone — Claude session discovery on Windows leans entirely on cwd + sessions/{PID}.json. worth a one-line note in README/AGENTS.md that this is a known Windows limitation.
  • netstat -ano is parsed by skipping 4 header lines and matching LISTENING. that's correct on en-US Windows 10/11 but could break on locales that translate the state column. fine for v1, just a heads-up.

overall the shape of the port/process plumbing is right — main blocker is the cpu% issue, second is the unintentional global change to cmd_has_binary. happy to re-review once those are addressed.

@graykode
Copy link
Copy Markdown
Owner

graykode commented May 2, 2026

heads up — #89 (yangbinbin48) is doing essentially the same thing in parallel. nearly identical sysinfo plumbing, same cmd_has_binary change, same file_identity cfg split. plan on my side is to land this PR as the base for Windows support, then cherry-pick #89's codex.rs Windows piece (the only meaningful unique part there) as a separate PR. closing #89 once that's done.

before approving I'd like the two correctness items from the earlier review to be addressed in commits on this branch (so the PR author stays consistent rather than me pushing on top):

  1. keep the sysinfo System alive across ticks. Process::cpu_usage() is a delta between refreshes, so a freshly built System always returns 0 — that's why every Windows process will look idle and the Working/Waiting threshold won't fire. fix: stash it in a OnceLock<Mutex<System>> (one for get_process_info, one for map_pid_to_sysinfo_open_paths) and refresh through that.
  2. scope the cmd_has_binary Windows-style matching behind #[cfg(windows)]. as written, the case-insensitive match + .exe strip + \ split applies on linux/macOS too, which silently changes matching behavior on those platforms. easiest is to split into two cfg-gated impls.

if it's easier I can push the patches directly (maintainerCanModify is on), but I'd rather you take them so the commit history stays cleanly attributed to you. let me know either way.

@graykode
Copy link
Copy Markdown
Owner

graykode commented May 2, 2026

closing as a duplicate of #89, which covers the same Windows core (sysinfo + netstat) and additionally extends the codex.rs side. plan is to land #89 as the Windows base, then take the dist/release-infrastructure pieces from #90 separately.

really appreciate the work though — a couple of details from this PR are still useful and I'll fold them into #89 during review:

thanks for kicking this off!

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.

3 participants