refactor: split DaemonStarter into platform-aware launch strategies#373
Merged
Conversation
Extract platform detection (Linux/macOS/Windows) and per-platform ProcessBuilder construction into separate methods. No behavioral change on macOS/Linux — same setsid (Linux) and trap INT (macOS) logic. Windows launcher added: uses java.exe, double-quote escaping, redirected streams for console detachment, PIPE for stdin (no /dev/null). Key changes: - Platform enum with detect() based on os.name - resolveJavaBin(): java vs java.exe - quoteForShell(): single quotes (Unix) vs double quotes (Windows) - buildLinuxLauncher/buildMacOSLauncher/buildWindowsLauncher - All existing tests pass (no behavioral change on Unix) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
📝 WalkthroughWalkthroughPlatform-aware daemon startup was implemented by adding OS detection and replacing the single Unix-oriented flow with platform-specific Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Suggested labels
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error)
✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
aceclaw-cli/src/main/java/dev/aceclaw/cli/DaemonStarter.java (2)
100-109: Consider logging a warning for unknown platforms.When
os.namedoesn't match Windows or macOS patterns, the code silently falls back toLINUX. This could mask issues on unsupported platforms (e.g., FreeBSD, Solaris) where the Linux launcher might fail unexpectedly.🔧 Suggested improvement
static Platform detect() { String os = System.getProperty("os.name", "").toLowerCase(Locale.ROOT); if (os.contains("win")) return WINDOWS; if (os.contains("mac") || os.contains("darwin")) return MACOS; + if (!os.contains("linux") && !os.contains("nux")) { + LoggerFactory.getLogger(DaemonStarter.class) + .warn("Unknown OS '{}'; using Linux launcher", os); + } return LINUX; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@aceclaw-cli/src/main/java/dev/aceclaw/cli/DaemonStarter.java` around lines 100 - 109, Platform.detect() currently defaults to LINUX silently when os.name doesn't match Windows or macOS; update detect() to emit a warning when the OS string is unrecognized before returning LINUX. Locate the enum Platform and its detect() method and add a log/ warning (using the project's logger or System.err if none exists) that includes the raw System.getProperty("os.name", "") value and a note that Linux is being assumed; keep the returned values unchanged (WINDOWS, MACOS, LINUX) so behavior is preserved but a warning is recorded for unsupported or unexpected platforms.
202-222: UseNULdevice instead ofPIPEfor stdin on Windows.Windows provides the
NULdevice as an equivalent to Unix/dev/null. Redirecting stdin toNULis more explicit than usingPIPEand avoids holding a pipe resource open. Since the daemon doesn't read stdin, either approach is safe, butNULis the cleaner choice.🔧 Optional: use NUL device
- // Windows has no /dev/null; use Redirect.PIPE and don't write to it - // (the daemon doesn't read stdin, so this is safe) - pb.redirectInput(ProcessBuilder.Redirect.PIPE); + // Windows NUL device is equivalent to Unix /dev/null + pb.redirectInput(ProcessBuilder.Redirect.from(new File("NUL")));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@aceclaw-cli/src/main/java/dev/aceclaw/cli/DaemonStarter.java` around lines 202 - 222, Replace the stdin redirect in buildWindowsLauncher to use the Windows NUL device instead of a PIPE: change the call that currently does pb.redirectInput(ProcessBuilder.Redirect.PIPE) to pb.redirectInput(ProcessBuilder.Redirect.from(new java.io.File("NUL"))) (add import for java.io.File if needed); keep the existing output/error redirects (including DAEMON_LOG) unchanged so the daemon is detached without holding an unused pipe open.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@aceclaw-cli/src/main/java/dev/aceclaw/cli/DaemonStarter.java`:
- Around line 144-149: The constructed shell command in buildJavaCommand()
concatenates resolveJavaBin() without quoting, which can break if the Java path
contains spaces; update buildJavaCommand() to wrap the resolved java binary
using quoteForShell(resolveJavaBin()) (or an equivalent quoting helper) so both
the java path and the classpath are safely shell-quoted when building the final
command string.
---
Nitpick comments:
In `@aceclaw-cli/src/main/java/dev/aceclaw/cli/DaemonStarter.java`:
- Around line 100-109: Platform.detect() currently defaults to LINUX silently
when os.name doesn't match Windows or macOS; update detect() to emit a warning
when the OS string is unrecognized before returning LINUX. Locate the enum
Platform and its detect() method and add a log/ warning (using the project's
logger or System.err if none exists) that includes the raw
System.getProperty("os.name", "") value and a note that Linux is being assumed;
keep the returned values unchanged (WINDOWS, MACOS, LINUX) so behavior is
preserved but a warning is recorded for unsupported or unexpected platforms.
- Around line 202-222: Replace the stdin redirect in buildWindowsLauncher to use
the Windows NUL device instead of a PIPE: change the call that currently does
pb.redirectInput(ProcessBuilder.Redirect.PIPE) to
pb.redirectInput(ProcessBuilder.Redirect.from(new java.io.File("NUL"))) (add
import for java.io.File if needed); keep the existing output/error redirects
(including DAEMON_LOG) unchanged so the daemon is detached without holding an
unused pipe open.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f92dcb80-d95c-4d4a-b2e2-369498000561
📒 Files selected for processing (1)
aceclaw-cli/src/main/java/dev/aceclaw/cli/DaemonStarter.java
Comment on lines
+144
to
+149
| private static String buildJavaCommand() { | ||
| String classpath = System.getProperty("java.class.path"); | ||
| return resolveJavaBin() + " --enable-preview -cp " | ||
| + quoteForShell(classpath) | ||
| + " dev.aceclaw.daemon.AceClawDaemon"; | ||
| } |
There was a problem hiding this comment.
Java binary path is not quoted; may break with spaces in path.
resolveJavaBin() is concatenated directly into the shell command without quoting. While rare on Unix, non-standard Java installations (e.g., /opt/my java/...) would break the shell command. Only classpath is quoted via quoteForShell().
🔧 Proposed fix
private static String buildJavaCommand() {
String classpath = System.getProperty("java.class.path");
- return resolveJavaBin() + " --enable-preview -cp "
+ return quoteForShell(resolveJavaBin().toString()) + " --enable-preview -cp "
+ quoteForShell(classpath)
+ " dev.aceclaw.daemon.AceClawDaemon";
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private static String buildJavaCommand() { | |
| String classpath = System.getProperty("java.class.path"); | |
| return resolveJavaBin() + " --enable-preview -cp " | |
| + quoteForShell(classpath) | |
| + " dev.aceclaw.daemon.AceClawDaemon"; | |
| } | |
| private static String buildJavaCommand() { | |
| String classpath = System.getProperty("java.class.path"); | |
| return quoteForShell(resolveJavaBin().toString()) + " --enable-preview -cp " | |
| quoteForShell(classpath) | |
| " dev.aceclaw.daemon.AceClawDaemon"; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@aceclaw-cli/src/main/java/dev/aceclaw/cli/DaemonStarter.java` around lines
144 - 149, The constructed shell command in buildJavaCommand() concatenates
resolveJavaBin() without quoting, which can break if the Java path contains
spaces; update buildJavaCommand() to wrap the resolved java binary using
quoteForShell(resolveJavaBin()) (or an equivalent quoting helper) so both the
java path and the classpath are safely shell-quoted when building the final
command string.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Step 2 of #357: DaemonStarter platform-aware refactoring
What changed
DaemonStarter.startDaemonProcess() split into three platform-specific launchers:
buildLinuxLauncher()— setsid + /bin/sh (unchanged behavior)buildMacOSLauncher()— trap '' INT + exec (unchanged behavior)buildWindowsLauncher()— java.exe, redirected streams, PIPE stdinSupporting changes
Platformenum withdetect()based onos.nameresolveJavaBin()— returnsjava.exeon Windows,javaon UnixquoteForShell()— single quotes (Unix) vs double quotes (Windows)What did NOT change
Verification
Risk
Impact on next steps
Summary by CodeRabbit
Release Notes