feat(daemon+cli): aceclaw dashboard subcommand — daemon serves bundled dist (#446)#477
Conversation
ⓘ You've reached your Qodo monthly free-tier limit. Reviews pause until next month — upgrade your plan to continue now, or link your paid account if you already have one. |
![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.
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughAdds a bundled production dashboard to the daemon (classpath-served SPA), accepts same-origin WebSocket handshakes, exposes dashboard metadata via ChangesDashboard Support Infrastructure
Sequence DiagramsequenceDiagram
participant User
participant CLI
participant Daemon
participant Browser
User->>CLI: aceclaw dashboard
CLI->>Daemon: GET /health/status
Daemon-->>CLI: { dashboard: { enabled, bundled, url } }
CLI->>Browser: open(url)
Browser->>Daemon: GET / (same-origin)
Daemon-->>Browser: index.html + assets
Browser->>Daemon: WebSocket /ws (Origin matches same-origin)
Daemon-->>Browser: WS connected + events
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (2 errors, 1 warning)
✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 👉 Get your free trial and get 200 agent minutes per Slack user (a $50 value). 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 51ae2b0bc4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| boolean dashboardBundled = WebSocketBridge.dashboardBundled(); | ||
| String dashboardUrl = config.webSocketEnabled() | ||
| ? "http://" + config.webSocketHost() + ":" + config.webSocketPort() | ||
| : ""; | ||
| router.setDashboardInfo(new RequestRouter.DashboardInfo( |
There was a problem hiding this comment.
health.status is now populated from config values before the bridge is actually started, so it can report dashboard.enabled=true and a URL even when webSocketBridge.start() later fails (for example, if the WS port is already in use and the daemon continues without the bridge). In that case aceclaw dashboard prints/opens a dead URL instead of surfacing that the dashboard is unavailable, which breaks the new discovery flow and makes failures hard to diagnose.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
aceclaw-daemon/src/test/java/dev/aceclaw/daemon/WebSocketBridgeTest.java (1)
244-269: ⚡ Quick winAdd a 127.0.0.1-origin assertion in the same zero-config test.
This test currently validates
Origin: http://localhost:{port}only. Since runtime behavior allows both localhost and 127.0.0.1, covering both here will reduce regressions in same-origin allowlist logic.Suggested test extension
var ws = HttpClient.newHttpClient().newWebSocketBuilder() .header("Origin", "http://localhost:" + port) .buildAsync(URI.create("ws://127.0.0.1:" + port + "/ws"), listener) .get(AWAIT_TIMEOUT_MS, TimeUnit.MILLISECONDS); assertThat(connected.await(AWAIT_TIMEOUT_MS, TimeUnit.MILLISECONDS)).isTrue(); bridge.broadcast("sess-1", "stream.text", Map.of("delta", "hello")); assertThat(queue.poll(AWAIT_TIMEOUT_MS, TimeUnit.MILLISECONDS)).isNotNull(); + var queue127 = new LinkedBlockingQueue<String>(); + var ws127 = HttpClient.newHttpClient().newWebSocketBuilder() + .header("Origin", "http://127.0.0.1:" + port) + .buildAsync(URI.create("ws://127.0.0.1:" + port + "/ws"), new BufferingListener(queue127::add)) + .get(AWAIT_TIMEOUT_MS, TimeUnit.MILLISECONDS); + bridge.broadcast("sess-1", "stream.text", Map.of("delta", "hello-127")); + assertThat(queue127.poll(AWAIT_TIMEOUT_MS, TimeUnit.MILLISECONDS)).isNotNull(); + ws.sendClose(WebSocket.NORMAL_CLOSURE, "bye").get(AWAIT_TIMEOUT_MS, TimeUnit.MILLISECONDS); + ws127.sendClose(WebSocket.NORMAL_CLOSURE, "bye").get(AWAIT_TIMEOUT_MS, TimeUnit.MILLISECONDS);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@aceclaw-daemon/src/test/java/dev/aceclaw/daemon/WebSocketBridgeTest.java` around lines 244 - 269, Extend the test acceptsSameOriginHandshakeWithoutAllowedOriginsConfig to also assert that an Origin of "http://127.0.0.1:{port}" is accepted: after the existing localhost-based connection and broadcast assertions (using bridge, port, queue, listener), open a second WebSocket using HttpClient.newWebSocketBuilder().header("Origin", "http://127.0.0.1:" + port) and connect to ws://127.0.0.1:{port}/ws, then wait for the connection via the same connected CountDownLatch or a new latch, broadcast via bridge.broadcast("sess-2", "stream.text", …) and assert the queue receives a message; finally close that second ws with sendClose. This mirrors the existing localhost flow but uses "127.0.0.1" to verify same-origin acceptance for the loopback IP.
🤖 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/AceClawMain.java`:
- Around line 638-658: Add a null-guard at the start of openInBrowser to
validate the url parameter: call Objects.requireNonNull(url, "url") (and import
java.util.Objects if needed) before constructing the cmd or ProcessBuilder so
downstream calls never receive a null; keep the existing behavior of starting
the helper process with ProcessBuilder and returning true on pb.start() (and
returning false on IOException) without introducing a wait-and-check for the
helper process exit code.
In `@aceclaw-daemon/src/main/java/dev/aceclaw/daemon/AceClawDaemon.java`:
- Around line 550-560: The health payload currently uses
config.webSocketEnabled() for the dashboard "enabled" flag, which can be
inaccurate if the bridge fails to start; change the logic so that
RequestRouter.DashboardInfo passed to router.setDashboardInfo uses the bridge's
actual runtime state (e.g., WebSocketBridge.isRunning() or the success/failure
result from the bridge startup sequence) instead of the config value, and update
the dashboard URL only when the bridge is confirmed running; ensure you adjust
the place(s) that call router.setDashboardInfo (and any initial default) so the
enabled flag and URL reflect the real runtime availability after the bridge
startup completes or fails.
In `@aceclaw-daemon/src/main/java/dev/aceclaw/daemon/WebSocketBridge.java`:
- Around line 226-238: The current friendly fallback only registers
instance.get("/", ...) when dashboardBundled is false, so non-root dashboard
URLs still return Javalin's default 404; change this to register a global 404
handler or a catch-all GET route when dashboardBundled is false. Specifically,
replace or augment the instance.get("/", ...) usage with a Javalin 404 error
handler (instance.error(404, ctx -> ...)) or a wildcard GET route (e.g.,
instance.get("/*", ctx -> ...)) so any unhandled path returns the same
plain-text message and status/content-type; keep the same message and use the
dashboardBundled check to only register this handler in -Pno-dashboard builds.
- Around line 249-252: Clear sameOriginAllowlist in stop() to remove stale ports
and ensure you pre-seed sameOriginAllowlist with "http://localhost:{port}" and
"http://127.0.0.1:{port}" before calling instance.start(port) (or any start
sequence) when the port is known; update the code paths that assign
sameOriginAllowlist (the constructor/initializer where sameOriginAllowlist =
Set.of(...)) to instead set these entries early on when starting and ensure
stop() empties or resets sameOriginAllowlist so old origins cannot be authorized
during restart windows.
---
Nitpick comments:
In `@aceclaw-daemon/src/test/java/dev/aceclaw/daemon/WebSocketBridgeTest.java`:
- Around line 244-269: Extend the test
acceptsSameOriginHandshakeWithoutAllowedOriginsConfig to also assert that an
Origin of "http://127.0.0.1:{port}" is accepted: after the existing
localhost-based connection and broadcast assertions (using bridge, port, queue,
listener), open a second WebSocket using
HttpClient.newWebSocketBuilder().header("Origin", "http://127.0.0.1:" + port)
and connect to ws://127.0.0.1:{port}/ws, then wait for the connection via the
same connected CountDownLatch or a new latch, broadcast via
bridge.broadcast("sess-2", "stream.text", …) and assert the queue receives a
message; finally close that second ws with sendClose. This mirrors the existing
localhost flow but uses "127.0.0.1" to verify same-origin acceptance for the
loopback IP.
🪄 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: e263c1c9-08fd-4b51-b783-1e2769eabb81
⛔ Files ignored due to path filters (1)
README.mdis excluded by!**/*.md
📒 Files selected for processing (10)
aceclaw-cli/src/main/java/dev/aceclaw/cli/AceClawMain.javaaceclaw-daemon/build.gradle.ktsaceclaw-daemon/src/main/java/dev/aceclaw/daemon/AceClawConfig.javaaceclaw-daemon/src/main/java/dev/aceclaw/daemon/AceClawDaemon.javaaceclaw-daemon/src/main/java/dev/aceclaw/daemon/RequestRouter.javaaceclaw-daemon/src/main/java/dev/aceclaw/daemon/WebSocketBridge.javaaceclaw-daemon/src/test/java/dev/aceclaw/daemon/WebSocketBridgeTest.javaaceclaw-dashboard/build.gradle.ktsbuild.gradle.ktssettings.gradle.kts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 35040a096b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| case "0.0.0.0", "::", "::0" -> "localhost"; | ||
| default -> webSocketBridge.host(); | ||
| }; | ||
| String dashboardUrl = "http://" + urlHost + ":" + webSocketBridge.port(); |
There was a problem hiding this comment.
When webSocket.host is set to an IPv6 literal like ::1, this builds dashboardUrl as http://::1:<port>, which is not a valid HTTP URL format for IPv6 hosts. As a result, aceclaw dashboard prints and tries to open an unusable URL even though the bridge is running. The host should be normalized to bracketed form (e.g. http://[::1]:3141) before concatenating the port.
Useful? React with 👍 / 👎.
| this.sameOriginAllowlist = Set.of( | ||
| "http://localhost:" + this.port, | ||
| "http://127.0.0.1:" + this.port); |
There was a problem hiding this comment.
The new same-origin whitelist only includes http://localhost:<port> and http://127.0.0.1:<port>. If the daemon is bound to IPv6 loopback and the dashboard is opened at http://[::1]:<port>, the browser Origin will be http://[::1]:<port>, which is rejected by isOriginAllowed unless users add manual config. That breaks the zero-config bundled dashboard flow for IPv6 localhost setups.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
aceclaw-daemon/src/main/java/dev/aceclaw/daemon/WebSocketBridge.java (1)
231-238:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse a real catch-all for the
-Pno-dashboardfallback.Line 232 only matches the literal
/, so/fooand other deep links still return Javalin’s default 404 instead of the rebuild guidance this mode is supposed to provide. Switch this to a 404 handler or wildcard GET fallback so every unmatched dashboard route gets the same message.In Javalin, does app.get("/") match only the exact root path, and what is the recommended way to return a custom 404 body for unmatched GET routes?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@aceclaw-daemon/src/main/java/dev/aceclaw/daemon/WebSocketBridge.java` around lines 231 - 238, The current fallback registers instance.get("/") which only matches the exact root and misses deep links; replace this with a catch-all 404 handler so every unmatched dashboard route returns the rebuild guidance. When dashboardBundled is false, register a global 404 handler (use instance.error(404, ...) or a wildcard GET like instance.get("/*", ...)) instead of instance.get("/"), and keep the same status, contentType, and result message; reference the dashboardBundled flag and the existing instance.get("/", ctx -> { ... }) block when making the change.aceclaw-daemon/src/main/java/dev/aceclaw/daemon/AceClawDaemon.java (1)
561-570:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPopulate
dashboardInfofrom the running bridge, not config.Lines 566-570 advertise the dashboard before
webSocketBridge.start()runs. If the bridge fails to bind,health.statusstill reportsenabled=truewith a live URL, and if the configured port is0it reportshttp://...:0instead of the bound port. Initialize this pessimistically, then update it after the start attempt fromwebSocketBridge.isRunning()andwebSocketBridge.port().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@aceclaw-daemon/src/main/java/dev/aceclaw/daemon/AceClawDaemon.java` around lines 561 - 570, The current code sets DashboardInfo from config before the WebSocketBridge has started, causing enabled=true and the configured port (possibly 0) to be advertised even if the bridge fails or binds to a different port; change the initialization so router.setDashboardInfo(...) is populated pessimistically using WebSocketBridge.dashboardBundled() and disabled/empty URL (enabled=false, dashboardUrl="") before start, then after calling webSocketBridge.start() update the DashboardInfo again by querying webSocketBridge.isRunning() and webSocketBridge.port() to set enabled and the actual URL (use "localhost" for host resolution like the existing switch) via router.setDashboardInfo(new RequestRouter.DashboardInfo(...)). Ensure any place that previously relied on config.webSocketEnabled()/config.webSocketPort() for advertised values uses the live webSocketBridge.isRunning()/port() after start.
🤖 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-daemon/src/main/java/dev/aceclaw/daemon/AceClawDaemon.java`:
- Around line 562-568: The dashboard URL builder uses urlHost from
config.webSocketHost() and naively concatenates ":"+port, which produces invalid
URLs for IPv6 literals like "::1"; update the logic in AceClawDaemon (where
urlHost is set and dashboardUrl constructed) to detect IPv6 literals (e.g., host
contains ':' and is not already bracketed) and wrap the literal in square
brackets before appending the port (so "[" + urlHost + "]:" + port); ensure
existing bracketed hosts are left unchanged and keep the same
config.webSocketEnabled() gate.
---
Duplicate comments:
In `@aceclaw-daemon/src/main/java/dev/aceclaw/daemon/AceClawDaemon.java`:
- Around line 561-570: The current code sets DashboardInfo from config before
the WebSocketBridge has started, causing enabled=true and the configured port
(possibly 0) to be advertised even if the bridge fails or binds to a different
port; change the initialization so router.setDashboardInfo(...) is populated
pessimistically using WebSocketBridge.dashboardBundled() and disabled/empty URL
(enabled=false, dashboardUrl="") before start, then after calling
webSocketBridge.start() update the DashboardInfo again by querying
webSocketBridge.isRunning() and webSocketBridge.port() to set enabled and the
actual URL (use "localhost" for host resolution like the existing switch) via
router.setDashboardInfo(new RequestRouter.DashboardInfo(...)). Ensure any place
that previously relied on config.webSocketEnabled()/config.webSocketPort() for
advertised values uses the live webSocketBridge.isRunning()/port() after start.
In `@aceclaw-daemon/src/main/java/dev/aceclaw/daemon/WebSocketBridge.java`:
- Around line 231-238: The current fallback registers instance.get("/") which
only matches the exact root and misses deep links; replace this with a catch-all
404 handler so every unmatched dashboard route returns the rebuild guidance.
When dashboardBundled is false, register a global 404 handler (use
instance.error(404, ...) or a wildcard GET like instance.get("/*", ...)) instead
of instance.get("/"), and keep the same status, contentType, and result message;
reference the dashboardBundled flag and the existing instance.get("/", ctx -> {
... }) block when making the change.
🪄 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: 0b72b500-c6f4-4c20-896a-d5358f2a44b8
⛔ Files ignored due to path filters (2)
.github/workflows/ci.ymlis excluded by!.github/**.github/workflows/release.ymlis excluded by!.github/**
📒 Files selected for processing (4)
aceclaw-cli/src/main/java/dev/aceclaw/cli/AceClawMain.javaaceclaw-daemon/src/main/java/dev/aceclaw/daemon/AceClawDaemon.javaaceclaw-daemon/src/main/java/dev/aceclaw/daemon/WebSocketBridge.javaaceclaw-dashboard/build.gradle.kts
✅ Files skipped from review due to trivial changes (1)
- aceclaw-dashboard/build.gradle.kts
🚧 Files skipped from review as they are similar to previous changes (1)
- aceclaw-cli/src/main/java/dev/aceclaw/cli/AceClawMain.java
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8c75826f76
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| this.sameOriginAllowlist = Set.of( | ||
| "http://localhost:" + this.port, | ||
| "http://127.0.0.1:" + this.port); |
There was a problem hiding this comment.
The same-origin allowlist is built only as http://localhost:<port> and http://127.0.0.1:<port>, but browsers serialize origins on default HTTP port 80 without the port suffix (for example http://localhost). If users set webSocket.port to 80, the bundled dashboard page loads from the daemon but its WebSocket handshake is rejected by isOriginAllowed, breaking the zero-config flow unless they manually add allowedOrigins.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 30717c1255
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| * user config. Bind host stays {@code localhost} so it's never exposed off-machine. | ||
| * Users who explicitly set {@code webSocket.enabled = false} keep their override. | ||
| */ | ||
| private static final boolean DEFAULT_WEBSOCKET_ENABLED = true; |
There was a problem hiding this comment.
Switching DEFAULT_WEBSOCKET_ENABLED to true silently turns on the bridge for existing configs that set webSocket.host but never set webSocket.enabled (previously defaulted off). In that scenario, a host like 0.0.0.0 or another non-loopback address becomes network-reachable by default, and WebSocketBridge.isOriginAllowed still permits clients with no Origin header, so remote non-browser clients can connect without being in allowedOrigins. This is a security regression introduced by the default flip and should remain opt-in unless the host is explicitly loopback.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5b911b3d5e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } else if (configuredHost.contains(":")) { | ||
| urlHost = "[" + configuredHost + "]"; |
There was a problem hiding this comment.
When webSocket.host is already configured in bracketed IPv6 form (for example [::1]), this branch wraps it again and publishes http://[[::1]]:<port>, which is not a valid URL and breaks aceclaw dashboard/startup hints for that config. This case is reachable because config parsing explicitly treats [::1] as a loopback host, so URL normalization should detect and preserve already-bracketed literals instead of blindly adding brackets.
Useful? React with 👍 / 👎.
1 participant
Closes #446. One command (aceclaw dashboard) replaces the 5-step manual setup.
Changes
Daemon
CLI
Build
Tests
Refinements beyond the original issue spec
Posted as comments on #446: same-origin gate, default-on for webSocket, URL printed on browser-open failure, --no-open flag, npm ci, -Pno-dashboard, startup hint.
Test plan
Summary by CodeRabbit
New Features
dashboardcommand prints the dashboard URL and can open it in your browser.Changes
Tests