Skip to content

Add Windows compatibility#9

Open
brenk28 wants to merge 1 commit into
davo20019:masterfrom
brenk28:windows-implementation
Open

Add Windows compatibility#9
brenk28 wants to merge 1 commit into
davo20019:masterfrom
brenk28:windows-implementation

Conversation

@brenk28

@brenk28 brenk28 commented Feb 14, 2026

Copy link
Copy Markdown

Platform-specific changes:

  • Terminal/fs_utils: use cmd.exe /C on Windows, sh -c on Unix
  • CLI agent: use 'where' instead of 'which' for command discovery on Windows
  • Process control: add taskkill-based send_sigterm/send_sigkill for Windows
  • Health probes: cfg-gate sh/cmd.exe for ping commands
  • MCP client: cfg-gate process group signaling (Unix-only setpgid)
  • Slack/Telegram channels: cfg-gate Unix signal handlers
  • OAuth: use 127.0.0.1 instead of 0.0.0.0 for callback listener on Windows
  • SQLite: cfg-gate Unix file permissions, allow_acl attribute on Windows
  • System info tool: add Windows-specific system info collection
  • check_environment/service_status: use 'where' on Windows

Build and config:

  • Add build.bat for MSVC environment setup
  • Remove encryption from default features (requires OpenSSL on Windows)
  • Make db_security warn instead of bail when encryption feature is absent
  • Gate encryption-only imports behind #[cfg(feature = "encryption")]
  • Update README with Windows build instructions and OpenSSL setup
  • Add Windows service instructions to README

Dashboard fixes:

  • Fix rate limiting: empty bearer tokens no longer count as failed attempts
  • Log full dashboard token on startup for easier access

Platform-specific changes:
- Terminal/fs_utils: use cmd.exe /C on Windows, sh -c on Unix
- CLI agent: use 'where' instead of 'which' for command discovery on Windows
- Process control: add taskkill-based send_sigterm/send_sigkill for Windows
- Health probes: cfg-gate sh/cmd.exe for ping commands
- MCP client: cfg-gate process group signaling (Unix-only setpgid)
- Slack/Telegram channels: cfg-gate Unix signal handlers
- OAuth: use 127.0.0.1 instead of 0.0.0.0 for callback listener on Windows
- SQLite: cfg-gate Unix file permissions, allow_acl attribute on Windows
- System info tool: add Windows-specific system info collection
- check_environment/service_status: use 'where' on Windows

Build and config:
- Add build.bat for MSVC environment setup
- Remove encryption from default features (requires OpenSSL on Windows)
- Make db_security warn instead of bail when encryption feature is absent
- Gate encryption-only imports behind #[cfg(feature = "encryption")]
- Update README with Windows build instructions and OpenSSL setup
- Add Windows service instructions to README

Dashboard fixes:
- Fix rate limiting: empty bearer tokens no longer count as failed attempts
- Log full dashboard token on startup for easier access

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

@davo20019 davo20019 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

PR Review: Add Windows Compatibility

Nice work on the Windows porting — the #[cfg(unix)] / #[cfg(windows)] gating pattern is clean and idiomatic, and the coverage across tools, channels, and process control is thorough. A few items to address before merge:

High Priority

1. build.bat hardcodes paths
vcvarsall.bat path assumes VS 2022 Community in default location, and cd /d C:\projects\aidaemon is contributor-specific. Consider using %~dp0 for the script's directory and making the VS path discovery more robust (or just removing the cd).

2. restart_process() duplicated in slack.rs and telegram.rs
Both the #[cfg(unix)] and #[cfg(windows)] versions are identical copy-pastes across both files (4 copies total). Should be extracted to a shared utility to avoid divergence.

3. Dashboard token logged in plaintext (security regression)

info!("Dashboard token (expires in 24h): {}", tok);

Previously only logged a prefix (tok[..8]). Logging the full token means anyone with log access gets dashboard access. Suggest reverting to prefix-only or using debug! level.

4. wmic is deprecated
system.rs uses wmic for uptime and memory info, but Microsoft deprecated WMIC in Windows 11. Consider systeminfo or PowerShell as fallback, or at least handle the not-found case gracefully.

5. process_control.rs missing fallback for non-Unix/non-Windows targets
send_sigterm/send_sigkill now only have #[cfg(unix)] and #[cfg(windows)] impls. The old #[cfg(not(unix))] catch-all is gone. Add #[cfg(not(any(unix, windows)))] stubs to keep it compilable on other targets (same pattern you used in cli_agent.rs).

Medium Priority

6. Removing encryption from default features is a breaking change
Existing Linux/macOS users who cargo install aidaemon will silently lose encryption on upgrade. Combined with the bail!warn! change in db_security.rs, an existing encrypted DB will become unreadable without the user knowing why. Consider keeping encryption in defaults, or at minimum detecting an existing encrypted DB and bailing with a clear migration message.

7. cli_agent.rs Windows kill_process uses blocking I/O in async context
The kill_process function is async but the Windows impl calls std::process::Command::new("taskkill").status() which blocks the tokio runtime. Should use tokio::process::Command instead.

Low Priority

8. The OAuth rng.gen()rng.gen::<u8>() change looks like a type inference fix unrelated to Windows — might be worth a note in the PR description.

9. db_security.rs #[cfg(...)] on individual function parameters is unusual and hard to read. Consider a single parameter name with #[allow(unused)] or a let _ = config; inside the non-encryption block.

10. set_file_mode_0600(_path: &Path) — the _ prefix on _path means "intentionally unused" by convention, but it's actually used inside the #[cfg(unix)] block. Misleading naming.

What looks good

  • set_db_file_permissions no-op stub for Windows
  • Health probes sh -ccmd.exe /C
  • MCP client Windows env vars in SAFE_ENV_KEYS
  • Dashboard empty bearer token rate-limiting fix
  • not(any(unix, windows)) fallback in cli_agent.rs
  • README updates are comprehensive

@davo20019

Copy link
Copy Markdown
Owner

One broader question — have you tried running aidaemon under WSL2? Since it's a full Linux environment, everything should work out of the box (signals, shell commands, file permissions, process groups) with no code changes needed.

Curious whether WSL2 isn't an option for your use case, or if there's a specific reason you need native Windows support. That context would help evaluate the trade-off, since the #[cfg] gating across 19 files adds ongoing maintenance cost for every future change that touches process control or shell execution.

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