Route lifecycle CLI through daemon - #307
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: 24 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 lifecycle spawn/stop/kill/fork commands end-to-end: new daemon text routes proxying to per-project services, new API route contracts and text-payload renderers, corresponding installed shim CLI commands with a query-forwarding POST helper, updated ownership tests/documentation, and new tests across daemon and shim suites. ChangesLifecycle Commands End-to-End
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CLI as Installed Shim
participant Daemon
participant ProjectService
CLI->>Daemon: POST /core/lifecycle/spawn-text (project, tool, worktree)
Daemon->>Daemon: validate required query params
Daemon->>Daemon: check loopback-only access
Daemon->>ProjectService: POST agents.spawn (JSON, timeout)
ProjectService-->>Daemon: {ok, sessionId, status}
Daemon->>Daemon: renderCoreLifecycleSpawnLines(payload)
Daemon-->>CLI: text or JSON lines
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: 2
🧹 Nitpick comments (4)
scripts/installed-aimux-shim.sh (2)
135-169: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate status-classification logic with
aimux_post_text_route.
aimux_post_query_text_route(135-169) repeats almost the entire body ofaimux_post_text_route(101-133) — only the curl invocation differs (plain POST vs.--getwith extra args). Consider extracting the shared temp-file/trap/status-classification logic into a common helper parameterized by the curl invocation.🤖 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 `@scripts/installed-aimux-shim.sh` around lines 135 - 169, The status handling and temp-file/trap cleanup in aimux_post_query_text_route duplicates aimux_post_text_route almost exactly, so refactor the shared body into a common helper and keep only the curl invocation differences (plain POST versus --get with extra args) in the route-specific functions. Use the existing function names aimux_post_text_route and aimux_post_query_text_route to locate the duplicated logic, then parameterize the helper so both functions reuse the same response classification and cleanup flow.
289-504: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffRepeated flag-parsing boilerplate across lifecycle handlers.
--tool,--project,--worktree,--json/--no-openparsing blocks are duplicated nearly verbatim acrossaimux_try_lifecycle_spawn,_stop,_kill, and_fork. A shared parsing helper (or a data-driven loop) would reduce this ~250-line surface and the risk of divergent bugs like the ones 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 `@scripts/installed-aimux-shim.sh` around lines 289 - 504, The lifecycle handlers contain repeated command-line parsing logic, so the issue is duplicated flag handling across aimux_try_lifecycle_spawn, aimux_try_lifecycle_stop, aimux_try_lifecycle_kill, and aimux_try_lifecycle_fork. Refactor the shared parsing for --tool, --project, --worktree, --json, and --no-open into a reusable helper or a data-driven parser, then have each lifecycle function call it and keep only the route-specific arguments and required positional values.src/daemon.test.ts (1)
573-696: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGood coverage for the happy paths; consider adding a case for the ensureProject-failure/unhandled-exception path.
These tests cover success and one project-service error (404), but none currently exercise
ensureProject()throwing (e.g., viacoreActorMock.failStartFor) through a lifecycle text route. Given the related concern raised indaemon.ts(uncaught exception yielding a JSON body instead oftext/plain), a regression test here would lock in the fix.🤖 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 573 - 696, Add a regression test around the lifecycle text routes in daemon.test.ts that forces ensureProject() to fail, for example by using coreActorMock.failStartFor and then calling a lifecycle text endpoint through AimuxDaemon.routeRequest. Verify the response stays text/plain with the expected plain-text error body instead of falling back to a JSON unhandled-exception response, and anchor the test near the existing lifecycleSpawnText/lifecycleStopText cases so the ensureProject path is covered.src/daemon.ts (1)
418-453: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
stop/killimplicitly start a project service that wasn't running.
postProjectServiceJsonunconditionally callsensureProject()before proxying, soaimux stop/aimux killagainst a project with no running service will spin one up just to immediately fail with "session not found" from the downstream agents API — an unnecessary and surprising side effect for a stop/kill action.♻️ Suggested approach: make ensureProject opt-in per call site
private async postProjectServiceJson( projectRoot: string, routePath: string, body: Record<string, unknown>, + opts: { ensureProject?: boolean } = {}, ): Promise<...> { const resolvedRoot = this.resolveProjectRoot(projectRoot); try { - await this.ensureProject(resolvedRoot); + if (opts.ensureProject !== false) await this.ensureProject(resolvedRoot); const endpoint = loadMetadataEndpointByProjectId(getProjectIdFor(resolvedRoot)); if (!endpoint) return { ok: false, response: this.textError(503, `project service unavailable for ${resolvedRoot}`) }; ...Then pass
{ ensureProject: false }fromlifecycleStopTextRoute/lifecycleKillTextRoute.🤖 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 418 - 453, The lifecycle stop/kill paths are unexpectedly starting a project service because postProjectServiceJson always calls ensureProject() before proxying. Update postProjectServiceJson to make project initialization opt-in (for example via an ensureProject flag or similar), and keep the default behavior only for call sites that need it. Then adjust lifecycleStopTextRoute and lifecycleKillTextRoute to pass the non-starting behavior so stop/kill only proxy to an existing service and do not spin one up.
🤖 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/daemon.ts`:
- Around line 418-453: The lifecycle route in postProjectServiceJson lets
ensureProject() throw before the local try/catch, which breaks the text/plain
error contract. Move the ensureProject(resolvedRoot) call inside the try block
in src/daemon.ts so any startup or actor failures are converted through this
method’s existing textError(502, ...) path, keeping all postProjectServiceJson
failures consistent with the other route branches. After updating, verify the
daemon behavior through a local build/install and aimux restart as requested.
- Around line 455-542: The lifecycle text route handlers treat upstream JSON as
a success before verifying required response fields, which can render undefined
values. In lifecycleSpawnTextRoute, lifecycleStopTextRoute,
lifecycleKillTextRoute, and lifecycleForkTextRoute, validate
result.json.sessionId, status, previousStatus, and threadId before building the
payload or calling renderCoreLifecycle*Lines; if any field is missing or
malformed, return an error response instead of success. Use the existing payload
types and the postProjectServiceJson result handling to keep the checks close to
the response parsing.
---
Nitpick comments:
In `@scripts/installed-aimux-shim.sh`:
- Around line 135-169: The status handling and temp-file/trap cleanup in
aimux_post_query_text_route duplicates aimux_post_text_route almost exactly, so
refactor the shared body into a common helper and keep only the curl invocation
differences (plain POST versus --get with extra args) in the route-specific
functions. Use the existing function names aimux_post_text_route and
aimux_post_query_text_route to locate the duplicated logic, then parameterize
the helper so both functions reuse the same response classification and cleanup
flow.
- Around line 289-504: The lifecycle handlers contain repeated command-line
parsing logic, so the issue is duplicated flag handling across
aimux_try_lifecycle_spawn, aimux_try_lifecycle_stop, aimux_try_lifecycle_kill,
and aimux_try_lifecycle_fork. Refactor the shared parsing for --tool, --project,
--worktree, --json, and --no-open into a reusable helper or a data-driven
parser, then have each lifecycle function call it and keep only the
route-specific arguments and required positional values.
In `@src/daemon.test.ts`:
- Around line 573-696: Add a regression test around the lifecycle text routes in
daemon.test.ts that forces ensureProject() to fail, for example by using
coreActorMock.failStartFor and then calling a lifecycle text endpoint through
AimuxDaemon.routeRequest. Verify the response stays text/plain with the expected
plain-text error body instead of falling back to a JSON unhandled-exception
response, and anchor the test near the existing
lifecycleSpawnText/lifecycleStopText cases so the ensureProject path is covered.
In `@src/daemon.ts`:
- Around line 418-453: The lifecycle stop/kill paths are unexpectedly starting a
project service because postProjectServiceJson always calls ensureProject()
before proxying. Update postProjectServiceJson to make project initialization
opt-in (for example via an ensureProject flag or similar), and keep the default
behavior only for call sites that need it. Then adjust lifecycleStopTextRoute
and lifecycleKillTextRoute to pass the non-starting behavior so stop/kill only
proxy to an existing service and do not spin one up.
🪄 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: de861cbb-802c-4f7e-987b-8d0ef2deab6c
📒 Files selected for processing (8)
docs/command-ownership-inventory.mdscripts/installed-aimux-shim.shsrc/core-command-contract.tssrc/core-command-ownership.test.tssrc/core-text.tssrc/daemon.test.tssrc/daemon.tssrc/installed-shim.test.ts
|
Independent review findings resolved in 29113b1:
Verification: typecheck, lint, build, focused lifecycle/shim tests, and full vitest suite passed. |
1 participant
Summary
Verification
Summary by CodeRabbit
New Features
Bug Fixes