Route CLI management through Core commands - #273
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR expands the core command contract with project and relay operations, updates daemon routing and lifecycle handling for those commands, and refactors CLI flows to use core command requests for project, daemon, and relay actions. ChangesCore Command Bus Expansion
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant requestCoreCommand
participant Daemon
participant CoreProjectActor
CLI->>requestCoreCommand: project/relay command
requestCoreCommand->>Daemon: POST core command
Daemon->>Daemon: routeCoreCommand(envelope)
Daemon->>CoreProjectActor: stop() / kill()
Daemon-->>requestCoreCommand: result
requestCoreCommand-->>CLI: response
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main.ts (1)
1274-1293: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
host killstill behaves likehost stopfor active project actors
host killandhost stopboth go throughCORE_COMMAND_NAMES.projectStop, and the daemon only sendsSIGTERM/SIGKILLin the legacy no-actor fallback. When aCoreProjectActorexists, both commands take the same gracefulactor.stop()path, so the force-kill semantics are missing.🤖 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/main.ts` around lines 1274 - 1293, The hostCmd "kill" action is still using the same CORE_COMMAND_NAMES.projectStop path as "stop", so it never gets force-kill behavior when a CoreProjectActor exists. Update the kill command flow in src/main.ts so it routes to a distinct kill/force-stop command or flag, and make the daemon-side handling in the project stop logic choose a hard termination path for CoreProjectActor instead of calling actor.stop(). Keep the existing stop behavior unchanged, but ensure hostCmd.command("kill") produces a different execution path from hostCmd.command("stop").
🧹 Nitpick comments (2)
src/daemon.ts (1)
876-899: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
projectRootvalidation betweenprojectEnsureandprojectStop.Both cases repeat the identical
typeof payload?.projectRoot !== "string" || !payload.projectRoot.trim()check and 400 response shape. Extracting a small helper would reduce duplication and keep future project-scoped commands consistent.♻️ Proposed helper extraction
+ private requireProjectRoot( + payload: { projectRoot?: unknown } | undefined, + ): { ok: true; projectRoot: string } | { ok: false; error: string } { + if (typeof payload?.projectRoot !== "string" || !payload.projectRoot.trim()) { + return { ok: false, error: "projectRoot is required" }; + } + return { ok: true, projectRoot: payload.projectRoot }; + } + switch (command) { ... case CORE_COMMAND_NAMES.projectEnsure: { - if (typeof payload?.projectRoot !== "string" || !payload.projectRoot.trim()) { - return { - status: 400, - body: { ok: false, id, command, error: "projectRoot is required" }, - }; - } + const check = this.requireProjectRoot(payload); + if (!check.ok) return { status: 400, body: { ok: false, id, command, error: check.error } }; return { status: 200, - body: { ok: true, id, command, issuedAt, result: { project: await this.ensureProject(payload.projectRoot) } }, + body: { ok: true, id, command, issuedAt, result: { project: await this.ensureProject(check.projectRoot) } }, }; + }🤖 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.ts` around lines 876 - 899, Duplicate projectRoot validation is repeated in the projectEnsure and projectStop branches of the daemon command handler. Extract the shared typeof payload?.projectRoot !== "string" || !payload.projectRoot.trim() check and the 400 response shape into a small helper in src/daemon.ts, then reuse it from the switch cases so project-scoped commands stay consistent and easier to extend.src/core-command-contract.ts (1)
45-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a concrete type instead of
unknownforrelay.
CoreStatusResult.relayandCoreRelayResult.relayare typedunknown, which forces every consumer to perform an unsafe cast (e.g.result.relay as { status?: string; relayUrl?: string }inmain.ts). SincegetRelayStatus()/enableRelay()/disableRelay()indaemon.tsreturn a well-defined shape (RelayStatusSnapshot | { status: "off" }), exposing that type here would remove the need for downstream casts and catch shape mismatches at compile time.Also applies to: 73-75
🤖 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-command-contract.ts` around lines 45 - 47, Replace the `unknown` type used for `relay` in `CoreStatusResult` and `CoreRelayResult` with the concrete relay response type returned by `getRelayStatus()`, `enableRelay()`, and `disableRelay()` in `daemon.ts` (the `RelayStatusSnapshot | { status: "off" }` shape). Update the shared contract in `src/core-command-contract.ts` so consumers like `main.ts` no longer need unsafe casts, and ensure any related type references or exports use the same symbol consistently.
🤖 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 `@src/main.ts`:
- Around line 1230-1231: The `serve` flow lost its legacy drift-repair behavior
because it now only calls `requestCoreCommand(CORE_COMMAND_NAMES.projectEnsure,
{ projectRoot })` and logs the PID, without handling stale/mismatched services.
Restore the `isAimuxBuildDriftError` / `waitForVerifiedProjectService` repair
path in the `serve` command handler, or move that retry-and-verify logic into
`projectEnsure` so `serve` still recovers old project services. Keep the
existing `projectEnsure` call and add the verification/repair branch around the
`serve` startup path where the service PID is reported.
---
Outside diff comments:
In `@src/main.ts`:
- Around line 1274-1293: The hostCmd "kill" action is still using the same
CORE_COMMAND_NAMES.projectStop path as "stop", so it never gets force-kill
behavior when a CoreProjectActor exists. Update the kill command flow in
src/main.ts so it routes to a distinct kill/force-stop command or flag, and make
the daemon-side handling in the project stop logic choose a hard termination
path for CoreProjectActor instead of calling actor.stop(). Keep the existing
stop behavior unchanged, but ensure hostCmd.command("kill") produces a different
execution path from hostCmd.command("stop").
---
Nitpick comments:
In `@src/core-command-contract.ts`:
- Around line 45-47: Replace the `unknown` type used for `relay` in
`CoreStatusResult` and `CoreRelayResult` with the concrete relay response type
returned by `getRelayStatus()`, `enableRelay()`, and `disableRelay()` in
`daemon.ts` (the `RelayStatusSnapshot | { status: "off" }` shape). Update the
shared contract in `src/core-command-contract.ts` so consumers like `main.ts` no
longer need unsafe casts, and ensure any related type references or exports use
the same symbol consistently.
In `@src/daemon.ts`:
- Around line 876-899: Duplicate projectRoot validation is repeated in the
projectEnsure and projectStop branches of the daemon command handler. Extract
the shared typeof payload?.projectRoot !== "string" ||
!payload.projectRoot.trim() check and the 400 response shape into a small helper
in src/daemon.ts, then reuse it from the switch cases so project-scoped commands
stay consistent and easier to extend.
🪄 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: 26764ed9-1e3b-46ae-b396-61f625afc103
📒 Files selected for processing (5)
src/core-command-contract.test.tssrc/core-command-contract.tssrc/daemon.test.tssrc/daemon.tssrc/main.ts
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Independent review finding fixed in 4f14c62: |
|
Independent review finding fixed in 4f14c62: |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/core-command-contract.ts (1)
80-86: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate shape between
CoreProjectKillResultandCoreProjectStopResult.Both interfaces are structurally identical (
{ project: CoreProjectServiceState | null }). Consider defining one as an alias of the other to avoid drift if the shapes diverge later.♻️ Proposed refactor
-export interface CoreProjectKillResult { - project: CoreProjectServiceState | null; -} +export type CoreProjectKillResult = CoreProjectStopResult;🤖 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-command-contract.ts` around lines 80 - 86, `CoreProjectKillResult` duplicates the same `{ project: CoreProjectServiceState | null }` shape as `CoreProjectStopResult`, so update the contract in `CoreProjectKillResult` to reuse the existing type instead of redefining it. Keep the shared result shape centralized in `src/core-command-contract.ts` by making one interface/type alias refer to the other, and verify any references to `CoreProjectKillResult` and `CoreProjectStopResult` still compile against the shared definition.
🤖 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 `@src/core-project-actor.ts`:
- Around line 92-99: The kill() method in coreProjectActor still only delegates
to stop(), so a forced termination request can hang if graceful cleanup stalls.
Update kill() to implement a true hard-stop path by adding a timeout and SIGKILL
fallback, or by moving the hard-termination logic into this method, while
keeping the existing log.warn call in coreProjectActor for the final
force-stopped event.
---
Nitpick comments:
In `@src/core-command-contract.ts`:
- Around line 80-86: `CoreProjectKillResult` duplicates the same `{ project:
CoreProjectServiceState | null }` shape as `CoreProjectStopResult`, so update
the contract in `CoreProjectKillResult` to reuse the existing type instead of
redefining it. Keep the shared result shape centralized in
`src/core-command-contract.ts` by making one interface/type alias refer to the
other, and verify any references to `CoreProjectKillResult` and
`CoreProjectStopResult` still compile against the shared definition.
🪄 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: 111b9b58-eee1-4511-aa8f-0ea27f28da5d
📒 Files selected for processing (6)
src/core-command-contract.test.tssrc/core-command-contract.tssrc/core-project-actor.tssrc/daemon.test.tssrc/daemon.tssrc/main.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/daemon.test.ts
- src/core-command-contract.test.ts
- src/daemon.ts
- src/main.ts
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core-project-actor.ts (1)
94-133: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAwait the in-flight cleanup before reusing the project root.
kill()can return after the timeout whilemux.cleanup()is still running in the background, so a freshstart()for the same project can overlap with teardown and race on tmux/process/metadata cleanup. Track the pending cleanup promise and await it before the next start, or add a stronger hard-stop path.🤖 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-project-actor.ts` around lines 94 - 133, The kill() flow in CoreProjectActor can time out and return while mux.cleanup() is still running, which allows a later start() for the same projectRoot to overlap with teardown. Update kill() to track the in-flight cleanup from mux.cleanup() in CoreProjectActor and ensure the pending cleanup is awaited or otherwise completed before the project root can be reused, or add a stronger hard-stop path that fully stops the tmux/process/metadata teardown before returning.
🤖 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.
Outside diff comments:
In `@src/core-project-actor.ts`:
- Around line 94-133: The kill() flow in CoreProjectActor can time out and
return while mux.cleanup() is still running, which allows a later start() for
the same projectRoot to overlap with teardown. Update kill() to track the
in-flight cleanup from mux.cleanup() in CoreProjectActor and ensure the pending
cleanup is awaited or otherwise completed before the project root can be reused,
or add a stronger hard-stop path that fully stops the tmux/process/metadata
teardown before returning.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 663c0516-762a-4d9c-9d2c-baed196f16f4
📒 Files selected for processing (3)
src/core-command-contract.tssrc/core-project-actor.test.tssrc/core-project-actor.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core-command-contract.ts
1 participant
Summary
Verification
Summary by CodeRabbit
projectRootfor project ensure/stop requests, returning HTTP 400 withprojectRoot is requiredwhen missing.