Move host management commands to core sidecar#322
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 36 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds daemon-owned handling for ChangesProject service and restart routing
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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
♻️ Duplicate comments (1)
docs/command-ownership-inventory.md (1)
94-95: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winSame
--serveomission in the developer plumbing table.Row 95 mentions
host restart --openas unsupported but doesn't mention the supported--servevariant, mirroring the gap noted for the "Installed Shim Fast Paths" table above.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/command-ownership-inventory.md` around lines 94 - 95, The developer plumbing table entry for the host restart command is missing the supported `--serve` variant mentioned in the review. Update the `host restart` row in the command ownership inventory so it mirrors the “Installed Shim Fast Paths” wording by explicitly noting that `host restart --open` is unsupported while `host restart --serve` is supported, keeping the description aligned with the corresponding command semantics.
🧹 Nitpick comments (3)
src/installed-shim.test.ts (1)
443-465: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCumulative curl-log assertions don't verify which call produced each token.
This test runs 5 different commands (
serve,host stop,host kill,host restart,host restart --serve) against a shared, append-onlycurlLog, then assertsserve=1andproject=<path>appear somewhere in the aggregate log. This can't distinguish "the right request carriedserve=1" from "some other request happened to include it," so a bug that misattributesserve=1to the wrong subcommand would still pass.✅ Suggested per-call assertions
- expect(fixture.run(["host", "restart", "--serve"], {}, { cwd: projectDir }).stdout).toBe("host ok\n"); - - const curlLog = readFileSync(fixture.curlLog, "utf8"); - expect(curlLog).toContain("/core/project-serve-text"); - expect(curlLog).toContain("/core/project-stop-text"); - expect(curlLog).toContain("/core/project-kill-text"); - expect(curlLog).toContain("/core/project-restart-text"); - expect(curlLog).toContain(`project=${realpathSync(projectDir)}\n`); - expect(curlLog).toContain("serve=1\n"); + expect(fixture.run(["host", "restart", "--serve"], {}, { cwd: projectDir }).stdout).toBe("host ok\n"); + + const calls = readFileSync(fixture.curlLog, "utf8").split("URL=").filter(Boolean); + expect(calls.some((c) => c.includes("/core/project-restart-text") && c.includes("serve=1"))).toBe(true); + expect(calls.every((c) => c.includes(`project=${realpathSync(projectDir)}`))).toBe(true);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/installed-shim.test.ts` around lines 443 - 465, The current assertions in installed-shim.test.ts only inspect the combined curl log, so they can’t prove which command sent which parameters. Update the test around the fixture.run calls so each invocation is validated separately, using the existing helpers in makeFixture, run, and curlLog to assert the expected request path and tokens for each subcommand. Keep the checks scoped per call so the serve=1 and project=... entries are tied to the correct command, not just present somewhere in the shared log.src/daemon.test.ts (1)
2088-2102: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtend loopback/cli-only tests to cover the new project-service routes.
projectKillText,projectRestartText,projectServeText, andprojectStopTextwere added toLOCAL_CLI_TEXT_ROUTESindaemon.ts(same security-relevant restriction asrestartText/projectEnsureText), but these two test blocks only exerciserestartText/projectEnsureTextfor the loopback-only and cli-only rejections. Add the four new routes here to actually verify the restriction on the newly-introduced surface.Also applies to: 3844-3851
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/daemon.test.ts` around lines 2088 - 2102, The loopback-only and cli-only route coverage in daemon.test.ts is missing the newly added project service text routes. Extend the existing rejection checks that currently exercise restartText and projectEnsureText so they also cover projectKillText, projectRestartText, projectServeText, and projectStopText from LOCAL_CLI_TEXT_ROUTES in daemon.ts, ensuring the same 403 behavior is asserted for these new security-sensitive routes.src/core-text.ts (1)
281-283: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a dedicated payload type for
renderCoreProjectServeLines.Reusing
CoreProjectEnsureTextPayloadfor the "serve" renderer works today because the shapes are identical, but it's semantically misleading (aserveaction typed asEnsure). A small alias/type (e.g.CoreProjectServeTextPayload) would make the contract clearer for future maintainers, mirroring the separate types already introduced for stop/kill/restart in this same PR.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core-text.ts` around lines 281 - 283, `renderCoreProjectServeLines` is still typed with `CoreProjectEnsureTextPayload`, which is semantically misleading for a serve-specific renderer. Introduce a dedicated `CoreProjectServeTextPayload` type (or alias) alongside the existing stop/kill/restart payload types, then update `renderCoreProjectServeLines` to accept that type so the contract matches the action name and stays clear for future maintainers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/command-ownership-inventory.md`:
- Around line 42-50: The command inventory entry for aimux host restart is
missing the supported --serve flag variant, which is already permitted by
core-cli-routing.ts and covered by tests. Update the row for the host
stop/kill/restart command family in docs/command-ownership-inventory.md to
explicitly mention host restart --serve and keep the wording aligned with the
routing behavior and current command-family ownership notes.
---
Duplicate comments:
In `@docs/command-ownership-inventory.md`:
- Around line 94-95: The developer plumbing table entry for the host restart
command is missing the supported `--serve` variant mentioned in the review.
Update the `host restart` row in the command ownership inventory so it mirrors
the “Installed Shim Fast Paths” wording by explicitly noting that `host restart
--open` is unsupported while `host restart --serve` is supported, keeping the
description aligned with the corresponding command semantics.
---
Nitpick comments:
In `@src/core-text.ts`:
- Around line 281-283: `renderCoreProjectServeLines` is still typed with
`CoreProjectEnsureTextPayload`, which is semantically misleading for a
serve-specific renderer. Introduce a dedicated `CoreProjectServeTextPayload`
type (or alias) alongside the existing stop/kill/restart payload types, then
update `renderCoreProjectServeLines` to accept that type so the contract matches
the action name and stays clear for future maintainers.
In `@src/daemon.test.ts`:
- Around line 2088-2102: The loopback-only and cli-only route coverage in
daemon.test.ts is missing the newly added project service text routes. Extend
the existing rejection checks that currently exercise restartText and
projectEnsureText so they also cover projectKillText, projectRestartText,
projectServeText, and projectStopText from LOCAL_CLI_TEXT_ROUTES in daemon.ts,
ensuring the same 403 behavior is asserted for these new security-sensitive
routes.
In `@src/installed-shim.test.ts`:
- Around line 443-465: The current assertions in installed-shim.test.ts only
inspect the combined curl log, so they can’t prove which command sent which
parameters. Update the test around the fixture.run calls so each invocation is
validated separately, using the existing helpers in makeFixture, run, and
curlLog to assert the expected request path and tokens for each subcommand. Keep
the checks scoped per call so the serve=1 and project=... entries are tied to
the correct command, not just present somewhere in the shared log.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3a5e7905-e565-4468-bd18-6dd754e11c79
📒 Files selected for processing (10)
docs/command-ownership-inventory.mdscripts/installed-aimux-shim.shsrc/core-cli-routing.tssrc/core-command-contract.tssrc/core-command-ownership.test.tssrc/core-text.tssrc/daemon.test.tssrc/daemon.tssrc/installed-shim.test.tssrc/launcher-env.test.ts
|
Independent review finding fixed in 7bc55e9: |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/core-cli.test.ts (1)
313-354: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for
--openwhen no dashboard target is returned.Current tests cover
--openonly whendashboardTargetis present. Add a case forhost restart --serve --open(or a mocked restart with no dashboardTarget) to lock in the intended behavior once the missing-feedback gap inrunHostServiceis addressed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core-cli.test.ts` around lines 313 - 354, The host service restart test only covers `--open` when a dashboard target exists, so add coverage for the no-dashboard-target path. Update the `runHostService`-related test setup in `src/core-cli.test.ts` to mock a restart response without `dashboardTarget`, then verify `host restart --serve --open` still behaves as intended and does not require the target. Keep the assertions centered on `runHostService`, `requestCoreCommand`, and `tmuxOpenTarget` so the intended fallback behavior stays locked in.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/command-ownership-inventory.md`:
- Line 43: The command ownership table entry for `host restart` is being split
into extra markdown cells because the flag notation contains an unescaped pipe.
Update the affected table rows in the command ownership inventory so the
`--serve|--open` text is either rephrased without a pipe or escaped consistently
in both places, keeping the table to the expected column count and preserving
the note text.
In `@src/core-cli.ts`:
- Around line 190-224: The restart flow in runHostService silently ignores
--open when result.dashboardTarget is missing, so add an explicit user-facing
message or warning in that branch instead of doing nothing. Update the logic
after renderCoreProjectRestartLines to check restartArgs.open alongside
result.dashboardTarget and, when open was requested but no target exists, write
a clear notice through io.stderr or io.stdout indicating the open action could
not be performed; keep the existing openTmuxTargetFromCaller path for the
successful case.
---
Nitpick comments:
In `@src/core-cli.test.ts`:
- Around line 313-354: The host service restart test only covers `--open` when a
dashboard target exists, so add coverage for the no-dashboard-target path.
Update the `runHostService`-related test setup in `src/core-cli.test.ts` to mock
a restart response without `dashboardTarget`, then verify `host restart --serve
--open` still behaves as intended and does not require the target. Keep the
assertions centered on `runHostService`, `requestCoreCommand`, and
`tmuxOpenTarget` so the intended fallback behavior stays locked in.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: a65f93e5-7ab7-4bb2-857b-23358b2163c1
📒 Files selected for processing (13)
docs/command-ownership-inventory.mdscripts/installed-aimux-shim.shsrc/core-cli-open.tssrc/core-cli-routing.tssrc/core-cli.test.tssrc/core-cli.tssrc/core-command-contract.tssrc/core-command-ownership.test.tssrc/core-text.tssrc/daemon.test.tssrc/daemon.tssrc/installed-shim.test.tssrc/launcher-env.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/core-cli-routing.ts
- src/core-command-ownership.test.ts
- src/core-text.ts
- src/installed-shim.test.ts
|
Independent review follow-up fixed in 7ef2efb: the installed shim no longer performs a reduced raw |
|
Independent review follow-up fixed in 6ea4f36: outside-tmux |
|
Independent review follow-up fixed in 8f9f754: missing tmux after a successful outside-tmux |
|
Independent review follow-up fixed in df5cf6c: the installed shim now routes |
1 participant
Summary
Verification
Summary by CodeRabbit
serve,host stop,host kill, andhost restart.daemon restart(with--jsonand optional--projectwhere applicable).--openon host restarts now opens the related tmux dashboard/target after restarting.restart,host stop/kill/restart, and--json/--serve/--open.