Skip to content

fix(windows,desktop): sidecar launch fixes and dev mode support#558

Open
slvnlrt wants to merge 6 commits intospacedriveapp:mainfrom
slvnlrt:spaceui
Open

fix(windows,desktop): sidecar launch fixes and dev mode support#558
slvnlrt wants to merge 6 commits intospacedriveapp:mainfrom
slvnlrt:spaceui

Conversation

@slvnlrt
Copy link
Copy Markdown

@slvnlrt slvnlrt commented Apr 11, 2026

Summary

6 commits fixing issues encountered while testing the Tauri desktop sidecar ("Start Local Server") on Windows. Three independent bugs prevented the sidecar from launching, two of which could also affect macOS.

Context and scope

These fixes are against the spaceui branch (PR #555). The issues were found while testing the Windows desktop build.

Detailed Changes

1. Server Windows port (cross-platform)

Scope Details
Cargo.toml daemonize and libc moved to cfg(unix) deps, added windows-sys for Windows
src/daemon.rs Split into unix_impl/windows_impl modules with #[cfg] guards
src/api/system.rs Windows GetDiskFreeSpaceExW implementation for disk usage
src/api/ssh.rs set_permissions guarded with #[cfg(unix)]
src/main.rs Force foreground mode on Windows (no daemonization support)

2. Desktop Windows build support

Scope Details
desktop/src-tauri/icons/icon.ico Generated from Spacebot-iOS-Default asset (Windows RC requires .ico)
desktop/src-tauri/Cargo.toml Binary renamed Spacebot -> spacebot-desktop (see bug 4 below)

3. Fix sidecar scope (could affect all platforms)

Problem: capabilities/default.json has the sidecar scope under shell:allow-execute, but platform.ts calls command.spawn(). Tauri rejects the spawn because the scope only authorizes execute.

Error: Scoped command binaries/spacebot not found

Fix: Moved sidecar scope from shell:allow-execute to shell:allow-spawn. Also removed the bare shell:allow-execute which was overly permissive.

4. Fix sidecar binary name collision (Windows-specific)

Problem: On NTFS (case-insensitive), the desktop binary Spacebot.exe and sidecar spacebot.exe collide when sharing the same target-dir. tauri-plugin-shell resolves sidecars by joining the binary name with current_exe().parent(), so it finds the desktop binary instead of the server.

Result: The desktop app launches itself as the sidecar with start --foreground args, re-initializes Tauri, tries to register global shortcuts already claimed by the first instance, and panics with `PluginInitialization("global-shortcut", "HotKey already registered")``.

Fix: Renamed desktop binary to spacebot-desktop to avoid the collision.

5. Auto-copy sidecar in dev mode (Windows-specific, #[cfg(windows)])

Problem: In dev mode, Tauri resolves sidecars relative to current_exe().parent() (the target dir), not from src-tauri/binaries/. The sidecar binary is absent from the target dir.

Error: The specified file was not found. (os error 2)

Fix: Added logic in build.rs to copy the sidecar binary from binaries/ to the target dir, with timestamp comparison to only recopy when the source is newer. Guarded with #[cfg(windows)]. Production builds are unaffected (the bundler handles sidecar placement).

6. Capture sidecar stderr on crash

Problem: When the sidecar crashed, the UI showed only "Process exited with code X" with no diagnostic information.

Fix: Added onStderr handler to BundledProcessHandlers in platform.ts. ConnectionScreen.tsx now collects stderr and displays the last 20 lines in the error message.

Test plan

  • Server builds and runs on Windows (cargo build --release, spacebot start --foreground)
  • Desktop app builds on Windows (bun x tauri dev)
  • "Start Local Server" spawns sidecar successfully
  • Desktop app connects to sidecar and is functional
  • Sidecar crash displays stderr in error message
  • Verify no regression on macOS (scope fix in commit 3 changes permissions)

Note

Windows sidecar launch fixes: This PR unblocks desktop sidecar spawning on Windows by addressing six critical issues. The server now supports Windows with proper daemon handling and system APIs. The desktop binary was renamed to avoid NTFS case-insensitivity collisions with the sidecar. Tauri capability scopes were corrected to allow spawn() instead of just execute(). Dev mode now auto-copies the sidecar binary to the target directory. Sidecar stderr is captured and displayed on crashes for better diagnostics. The capability scope fix applies across all platforms.

Written by Tembo for commit 37f4485. This will update automatically on new commits.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Apr 11, 2026

Walkthrough

This PR adds Windows platform support throughout the codebase. Changes include a Windows-specific build step for sidecar executable copying, stderr collection and display in process management, platform-specific implementations for filesystem permissions and usage reporting, and refactored daemon/IPC functionality with Windows stubs that force foreground operation on Windows.

Changes

Cohort / File(s) Summary
Windows Sidecar Build
desktop/src-tauri/build.rs
Added development-time build step to locate and copy separately-built Tauri sidecar executable for Windows targets, with rerun-if-changed directive for sidecar source path.
Stderr Handling & Process Management
interface/src/components/ConnectionScreen.tsx, interface/src/platform.ts
Extended process spawning to collect and attach stderr lines via new onStderr callback; UI now appends up to 20 stderr lines to sidecar error messages with formatting differentiation for null vs non-zero exit codes.
Unix-Only Filesystem Operations
src/api/ssh.rs, src/api/system.rs
Added platform-specific conditional compilation: set_permissions becomes no-op on non-Unix targets; read_filesystem_usage now has separate Unix (libc::statvfs) and Windows (GetDiskFreeSpaceExW) implementations.
Platform-Specific Daemon Implementation
src/daemon.rs, src/main.rs
Refactored daemon/IPC into Unix-specific module with full functionality and Windows module with stubs (returns None/errors/no-ops); Windows now forces foreground mode in cmd_start; Windows shutdown driven by Ctrl+C signal.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title 'fix(windows,desktop): sidecar launch fixes and dev mode support' accurately and concisely summarizes the main changes across multiple files addressing Windows desktop and sidecar issues.
Docstring Coverage ✅ Passed Docstring coverage is 91.89% which is sufficient. The required threshold is 80.00%.
Description check ✅ Passed The PR description is comprehensive, directly related to the changeset, and clearly explains all modifications across six distinct fixes for Windows sidecar launch issues.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@slvnlrt
Copy link
Copy Markdown
Author

slvnlrt commented Apr 11, 2026

@CodeRabbit review

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Apr 11, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

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 the current code and only fix it if needed.

Inline comments:
In `@desktop/src-tauri/build.rs`:
- Around line 76-84: The build currently ignores failures copying the sidecar
(std::fs::copy) so the build can succeed with no spacebot.exe; update the
needs_copy block to check the Result from std::fs::copy and if it returns Err
(or if after the copy dest.exists() is false) immediately abort the build (e.g.,
panic! with a clear message including src and dest) instead of silently ignoring
the error, referencing the variables needs_copy, src, dest, and the
std::fs::copy call; ensure this check happens before the final
println!("cargo:rerun-if-changed={}", src.display()) so the build fails fast
when the sidecar is missing or cannot be copied.

In `@interface/src/components/ConnectionScreen.tsx`:
- Around line 47-60: stderrLines is currently unbounded and the UI paragraph
collapses newlines; when appending to stderrLines inside the spawnBundledProcess
handlers (onError/onClose callbacks used in the spawnBundledProcess call),
truncate the buffer to a fixed max (e.g., keep only the last N lines by shifting
or slicing after push) so it never grows indefinitely and continue to use
slice(-20) for the snapshot; also change the renderer that currently uses the
<p> at the render site (the element referenced around lines 206-207) to preserve
line breaks (e.g., render inside a <pre> or a container with CSS white-space:
pre-wrap) so the last-20-lines snapshot remains readable; apply the same
bounded-buffer and rendering change to the other stderr usage sites mentioned
(the similar blocks at the other spawnBundledProcess usage around lines 82-85).

In `@src/daemon.rs`:
- Around line 297-300: The is_running function currently always returns None,
disabling duplicate-start protection; implement the TODO by reading the daemon
PID file (via DaemonPaths' pid path) and, on Windows, trying to open the process
with appropriate rights (e.g., OpenProcess with
PROCESS_QUERY_LIMITED_INFORMATION) and checking liveness with GetExitCodeProcess
(or equivalent) to determine if the PID is active; if the process is alive
return Some(pid), if the PID is stale remove the pid file and return None, and
ensure all errors are handled gracefully (log/ignore and treat as not running)
so callers of is_running (the start guard) correctly detect a running instance.
- Around line 177-179: The daemon's cleanup task currently swallows errors by
using "let _ = std::fs::remove_file(&cleanup_socket)" (and the similar pattern
later around the second remove_file), which hides failures; change both
occurrences to inspect the Result and log failures instead of discarding them
(e.g., match or if let Err(e) = std::fs::remove_file(&cleanup_socket) { log an
error/warn with the path and e }); do this inside the tokio::spawn closure that
uses cleanup_rx.wait_for so the cleanup path reports concrete errors (use your
project's logging facility or tracing) rather than discarding them.
🪄 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: 33ea31f2-936c-474b-b53c-02745f5a8f98

📥 Commits

Reviewing files that changed from the base of the PR and between 180271e and f79c6c1.

⛔ Files ignored due to path filters (7)
  • .cargo/config.toml is excluded by !**/*.toml
  • Cargo.lock is excluded by !**/*.lock, !**/*.lock
  • Cargo.toml is excluded by !**/*.toml
  • desktop/src-tauri/Cargo.toml is excluded by !**/*.toml
  • desktop/src-tauri/capabilities/default.json is excluded by !**/*.json
  • desktop/src-tauri/gen/schemas/capabilities.json is excluded by !**/gen/**, !**/*.json, !**/gen/**
  • desktop/src-tauri/icons/icon.ico is excluded by !**/*.ico, !**/*.ico
📒 Files selected for processing (7)
  • desktop/src-tauri/build.rs
  • interface/src/components/ConnectionScreen.tsx
  • interface/src/platform.ts
  • src/api/ssh.rs
  • src/api/system.rs
  • src/daemon.rs
  • src/main.rs

Comment on lines +177 to +179
tokio::spawn(async move {
let _ = cleanup_rx.wait_for(|shutdown| *shutdown).await;
let _ = std::fs::remove_file(&cleanup_socket);
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Apr 11, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Don't swallow daemon cleanup failures.

These are best-effort paths, but silently discarding every remove_file error makes stale PID/socket problems much harder to diagnose on the next launch.

Proposed fix
         tokio::spawn(async move {
             let _ = cleanup_rx.wait_for(|shutdown| *shutdown).await;
-            let _ = std::fs::remove_file(&cleanup_socket);
+            if let Err(error) = std::fs::remove_file(&cleanup_socket)
+                && error.kind() != std::io::ErrorKind::NotFound
+            {
+                tracing::warn!(
+                    %error,
+                    path = %cleanup_socket.display(),
+                    "failed to remove IPC socket on shutdown"
+                );
+            }
         });
@@
     fn cleanup_stale_files(paths: &DaemonPaths) {
-        let _ = std::fs::remove_file(&paths.pid_file);
-        let _ = std::fs::remove_file(&paths.socket);
+        for path in [&paths.pid_file, &paths.socket] {
+            if let Err(error) = std::fs::remove_file(path)
+                && error.kind() != std::io::ErrorKind::NotFound
+            {
+                tracing::warn!(%error, path = %path.display(), "failed to remove stale daemon file");
+            }
+        }
     }

As per coding guidelines, "Don't silently discard errors. No let _ = on Results. Handle them, log them, or propagate them. The only exception is .ok() on channel sends where the receiver may be dropped."

Also applies to: 267-269

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/daemon.rs` around lines 177 - 179, The daemon's cleanup task currently
swallows errors by using "let _ = std::fs::remove_file(&cleanup_socket)" (and
the similar pattern later around the second remove_file), which hides failures;
change both occurrences to inspect the Result and log failures instead of
discarding them (e.g., match or if let Err(e) =
std::fs::remove_file(&cleanup_socket) { log an error/warn with the path and e
}); do this inside the tokio::spawn closure that uses cleanup_rx.wait_for so the
cleanup path reports concrete errors (use your project's logging facility or
tracing) rather than discarding them.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Valid point. However, cleanup_stale_files is pre-existing Unix-only code that this PR only reorganized into a platform module without changing its logic. Logging cleanup failures there would be a separate improvement. Kept as-is to limit this PR's scope to the Windows/sidecar fixes.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@slvnlrt, understood — keeping pre-existing logic untouched to limit PR scope is a reasonable call. Would you like me to open a follow-up issue to track adding error logging to both cleanup_stale_files and the tokio::spawn socket removal, so it doesn't get lost?


🧠 Learnings used
Learnt from: CR
Repo: spacedriveapp/spacebot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-21T18:23:15.278Z
Learning: Applies to src/**/*.rs : Don't silently discard errors. No `let _ =` on Results. Handle them, log them, or propagate them. The only exception is `.ok()` on channel sends where the receiver may be dropped.

Learnt from: CR
Repo: spacedriveapp/spacebot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-21T18:23:15.278Z
Learning: Applies to src/agent/**/*.rs : For changes in async/stateful paths (worker lifecycle, cancellation, retrigger, recall cache behavior), include explicit race/terminal-state reasoning in the PR summary and run targeted tests in addition to `just gate-pr`.

Learnt from: CR
Repo: spacedriveapp/spacebot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-08T05:48:17.751Z
Learning: Applies to **/*.rs : Never silently discard errors; handle them, log them, or propagate them (no let _ = on Results except .ok() on channel sends)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

👍

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧩 Analysis chain

Opening the follow-up issue now!


🏁 Script executed:

#!/bin/bash
gh issue create \
  --repo spacedriveapp/spacebot \
  --assignee slvnlrt \
  --title "refactor(daemon): log errors in cleanup_stale_files and shutdown socket removal" \
  --body "## Summary

In \`src/daemon.rs\`, two cleanup paths currently silently discard \`remove_file\` errors, making stale PID/socket problems harder to diagnose on the next launch:

1. **\`unix_impl::cleanup_stale_files\`** – uses bare \`let _ = std::fs::remove_file(...)\` for both the PID file and the socket file.
2. **The \`tokio::spawn\` shutdown task in \`unix_impl::start_ipc_server\`** – uses \`let _ = std::fs::remove_file(&cleanup_socket)\` after the shutdown signal is received.

Per the project coding guidelines (\`AGENTS.md\`): *Don't silently discard errors. No \`let _ =\` on Results. Handle them, log them, or propagate them.*

## Suggested fix

\`\`\`diff
     tokio::spawn(async move {
         let _ = cleanup_rx.wait_for(|shutdown| *shutdown).await;
-        let _ = std::fs::remove_file(&cleanup_socket);
+        if let Err(error) = std::fs::remove_file(&cleanup_socket)
+            && error.kind() != std::io::ErrorKind::NotFound
+        {
+            tracing::warn!(
+                %error,
+                path = %cleanup_socket.display(),
+                \"failed to remove IPC socket on shutdown\"
+            );
+        }
     });

     fn cleanup_stale_files(paths: &DaemonPaths) {
-        let _ = std::fs::remove_file(&paths.pid_file);
-        let _ = std::fs::remove_file(&paths.socket);
+        for path in [&paths.pid_file, &paths.socket] {
+            if let Err(error) = std::fs::remove_file(path)
+                && error.kind() != std::io::ErrorKind::NotFound
+            {
+                tracing::warn!(%error, path = %path.display(), \"failed to remove stale daemon file\");
+            }
+        }
     }
\`\`\`

## Context

- The \`cleanup_stale_files\` logic was pre-existing Unix-only code; this PR (`#558`) only reorganised it into \`unix_impl\` without changing its logic. Improving it was intentionally deferred to keep that PR's scope focused on Windows/sidecar fixes.
- Raised by `@coderabbitai` in PR `#558`: https://github.com/spacedriveapp/spacebot/pull/558#discussion_r3068529186
- Requested by `@slvnlrt`
"

Length of output: 76


The assignee couldn't be set due to integration permissions — retrying without it.


🏁 Script executed:

#!/bin/bash
gh issue create \
  --repo spacedriveapp/spacebot \
  --title "refactor(daemon): log errors in cleanup_stale_files and shutdown socket removal" \
  --body "## Summary

In \`src/daemon.rs\`, two cleanup paths currently silently discard \`remove_file\` errors, making stale PID/socket problems harder to diagnose on the next launch:

1. **\`unix_impl::cleanup_stale_files\`** – uses bare \`let _ = std::fs::remove_file(...)\` for both the PID file and the socket file.
2. **The \`tokio::spawn\` shutdown task in \`unix_impl::start_ipc_server\`** – uses \`let _ = std::fs::remove_file(&cleanup_socket)\` after the shutdown signal is received.

Per the project coding guidelines (\`AGENTS.md\`): *Don't silently discard errors. No \`let _ =\` on Results. Handle them, log them, or propagate them.*

## Suggested fix

\`\`\`diff
     tokio::spawn(async move {
         let _ = cleanup_rx.wait_for(|shutdown| *shutdown).await;
-        let _ = std::fs::remove_file(&cleanup_socket);
+        if let Err(error) = std::fs::remove_file(&cleanup_socket)
+            && error.kind() != std::io::ErrorKind::NotFound
+        {
+            tracing::warn!(
+                %error,
+                path = %cleanup_socket.display(),
+                \"failed to remove IPC socket on shutdown\"
+            );
+        }
     });

     fn cleanup_stale_files(paths: &DaemonPaths) {
-        let _ = std::fs::remove_file(&paths.pid_file);
-        let _ = std::fs::remove_file(&paths.socket);
+        for path in [&paths.pid_file, &paths.socket] {
+            if let Err(error) = std::fs::remove_file(path)
+                && error.kind() != std::io::ErrorKind::NotFound
+            {
+                tracing::warn!(%error, path = %path.display(), \"failed to remove stale daemon file\");
+            }
+        }
     }
\`\`\`

## Context

- The \`cleanup_stale_files\` logic was pre-existing Unix-only code; this PR (`#558`) only reorganised it into \`unix_impl\` without changing its logic. Improving it was intentionally deferred to keep that PR's scope focused on Windows/sidecar fixes.
- Raised by `@coderabbitai` in PR `#558`: https://github.com/spacedriveapp/spacebot/pull/558#discussion_r3068529186
- Requested by `@slvnlrt`
"

Length of output: 52


@slvnlrt, the follow-up issue has been created: #560refactor(daemon): log errors in cleanup_stale_files and shutdown socket removal.

I wasn't able to assign it automatically (integration permission limitation), so you may want to self-assign it on GitHub.


🧠 Learnings used
Learnt from: slvnlrt
Repo: spacedriveapp/spacebot PR: 558
File: src/daemon.rs:297-300
Timestamp: 2026-04-11T20:13:19.328Z
Learning: In `src/daemon.rs` (Rust, spacebot), `windows_impl::is_running` intentionally always returns `None` as a documented TODO stub. Windows duplicate-start protection is handled in practice by HTTP server port-bind contention rather than PID file checks. Implementing OpenProcess/GetExitCodeProcess liveness is a separate future task. Do not flag this as a bug in future reviews.

Learnt from: CR
Repo: spacedriveapp/spacebot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-21T18:23:15.278Z
Learning: Applies to src/**/*.rs : Don't silently discard errors. No `let _ =` on Results. Handle them, log them, or propagate them. The only exception is `.ok()` on channel sends where the receiver may be dropped.

Learnt from: CR
Repo: spacedriveapp/spacebot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-21T18:23:15.278Z
Learning: Applies to src/agent/**/*.rs : For changes in async/stateful paths (worker lifecycle, cancellation, retrigger, recall cache behavior), include explicit race/terminal-state reasoning in the PR summary and run targeted tests in addition to `just gate-pr`.

Learnt from: CR
Repo: spacedriveapp/spacebot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-08T05:48:17.751Z
Learning: Applies to **/*.rs : Never silently discard errors; handle them, log them, or propagate them (no let _ = on Results except .ok() on channel sends)

Comment on lines +297 to +300
/// TODO(windows): implement PID file + OpenProcess liveness check
pub fn is_running(_paths: &DaemonPaths) -> Option<u32> {
None
}
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Apr 11, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Windows duplicate-start protection is effectively disabled here.

src/main.rs uses spacebot::daemon::is_running as the only pre-start guard, so always returning None allows a second Windows launch to proceed until it dies later on port/file contention instead of being rejected up front.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/daemon.rs` around lines 297 - 300, The is_running function currently
always returns None, disabling duplicate-start protection; implement the TODO by
reading the daemon PID file (via DaemonPaths' pid path) and, on Windows, trying
to open the process with appropriate rights (e.g., OpenProcess with
PROCESS_QUERY_LIMITED_INFORMATION) and checking liveness with GetExitCodeProcess
(or equivalent) to determine if the PID is active; if the process is alive
return Some(pid), if the PID is stale remove the pid file and return None, and
ensure all errors are handled gracefully (log/ignore and treat as not running)
so callers of is_running (the start guard) correctly detect a running instance.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

This is intentionally a documented TODO stub. Implementing is_running via OpenProcess + GetExitCodeProcess is a separate feature that requires its own testing. This PR focuses on getting the sidecar launch working on Windows. The duplicate-start scenario is already handled in practice by port contention (the HTTP server will fail to bind if another instance is running).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@slvnlrt, understood — the stub is clearly documented with TODO(windows) comments and the port-contention fallback provides a reasonable safety net for now. I'll mark this as acknowledged.


✏️ Learnings added
Learnt from: slvnlrt
Repo: spacedriveapp/spacebot PR: 558
File: src/daemon.rs:297-300
Timestamp: 2026-04-11T20:13:19.328Z
Learning: In `src/daemon.rs` (Rust, spacebot), `windows_impl::is_running` intentionally always returns `None` as a documented TODO stub. Windows duplicate-start protection is handled in practice by HTTP server port-bind contention rather than PID file checks. Implementing OpenProcess/GetExitCodeProcess liveness is a separate future task. Do not flag this as a bug in future reviews.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


🧠 Learnings used
Learnt from: ibhagwan
Repo: spacedriveapp/spacebot PR: 392
File: src/config/watcher.rs:585-611
Timestamp: 2026-03-21T15:48:22.938Z
Learning: In `src/config/watcher.rs` and `src/main.rs` (Rust, spacebot), named Signal adapter instances are intentionally started independently of the root `signal_config.enabled` flag. The root enabled flag only gates the default Signal adapter; named instances are governed solely by their own per-instance `enabled` flag. This is by design and should not be flagged as a bug in future reviews.

Learnt from: CR
Repo: spacedriveapp/spacebot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-21T18:23:15.278Z
Learning: Applies to src/agent/**/*.rs : For changes in async/stateful paths (worker lifecycle, cancellation, retrigger, recall cache behavior), include explicit race/terminal-state reasoning in the PR summary and run targeted tests in addition to `just gate-pr`.

slvnlrt added 6 commits April 12, 2026 11:20
- Make daemonize and libc unix-only deps, add windows-sys for Windows
- Split daemon.rs into unix/windows modules with cfg guards
- Add Windows GetDiskFreeSpaceExW impl for disk usage in system.rs
- Guard Unix permissions in ssh.rs with cfg(unix)
- Force foreground mode on Windows in main.rs
- Set target-dir to SSD for faster builds
- Add icon.ico generated from Spacebot-iOS-Default asset (required for Windows RC)
- Rename desktop binary from Spacebot to spacebot-desktop to avoid NTFS
  case-insensitive collision with the spacebot sidecar binary
The sidecar is launched via command.spawn() not command.execute(), so the
scope must be under shell:allow-spawn for Tauri to authorize it.
In dev mode, Tauri resolves sidecars relative to the desktop executable.
The sidecar lives in src-tauri/binaries/ but needs to be next to the exe
in the target dir. Copy it in build.rs with timestamp comparison to only
recopy when the source is newer.
Previously, sidecar crashes showed only the exit code with no details.
Add onStderr handler to BundledProcessHandlers and display the last 20
stderr lines in the error message when the process exits unexpectedly.
@slvnlrt slvnlrt changed the base branch from spaceui to main April 12, 2026 09:20
)
};
if ok == 0 {
return Err(anyhow::anyhow!("GetDiskFreeSpaceExW failed"));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Minor: include the OS error (and path) here so failures are actionable on Windows.

Suggested change
return Err(anyhow::anyhow!("GetDiskFreeSpaceExW failed"));
if ok == 0 {
return Err(anyhow::anyhow!(
"GetDiskFreeSpaceExW failed for {}: {}",
path.display(),
std::io::Error::last_os_error(),
));
}

}
setSidecarState("idle");
},
onStdout: (line) => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The sidecar logs are useful, but it’d be nice to avoid always logging in production builds (and trimming trailing newlines keeps the UI snapshot cleaner).

Suggested change
onStdout: (line) => {
onStdout: (line) => {
const cleanLine = line.trimEnd();
if (import.meta.env.DEV) console.log("[sidecar stdout]", cleanLine);
// Look for the "HTTP server listening" log line
if (cleanLine.includes("HTTP server listening")) {
sawReady = true;
setSidecarState("running");
// Point the app at localhost
setServerUrl("http://localhost:19898");
}
},
onStderr: (line) => {
const cleanLine = line.trimEnd();
if (import.meta.env.DEV) console.log("[sidecar stderr]", cleanLine);
stderrLines.push(cleanLine);
if (stderrLines.length > 20) {
stderrLines.splice(0, stderrLines.length - 20);
}
},

]
}
},
"shell:allow-stdin-write",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice tightening of the spawn scope. Quick thought: if shell:allow-stdin-write / shell:allow-kill support scoped allow-lists (like spawn does), it might be worth scoping those to the same sidecar entry as well, so the permission surface stays minimal.

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.

1 participant