Route core CLI commands through sidecar entry#285
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a core CLI entrypoint with command routing and handlers for host, daemon, projects, and remote operations. Launcher routing now sends core commands to the new entrypoint, and the shim/install path was updated accordingly. Tests cover command behavior and sidecar boundaries. ChangesCore CLI feature
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 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: 2
🧹 Nitpick comments (3)
src/core-cli.ts (1)
169-180: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNear-duplicate project-listing logic.
runDaemonProjectsandrunProjectsListboth fetchCORE_COMMAND_NAMES.projectsListand render aname badge pathline, differing only in badge text ("service"/"idle" vs "live"/"idle") and the empty-list message. Consider extracting a shared formatter parameterized by badge labels to avoid drift if the format changes later.Also applies to: 198-214
🤖 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.ts` around lines 169 - 180, The project-listing logic in runDaemonProjects and runProjectsList is duplicated and likely to drift. Extract a shared helper around the CORE_COMMAND_NAMES.projectsList fetch and the common name/badge/path rendering, and parameterize the badge labels so runDaemonProjects can use “service/idle” while runProjectsList uses “live/idle” and its empty-list message. Keep the shared formatting close to the existing runDaemonProjects and runProjectsList flow so both callers use the same formatter instead of maintaining separate copy-pasted loops.src/core-sidecar-boundary.test.ts (1)
58-65: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffBoundary check only covers direct string literals in
core-cli.ts, not transitive imports.This substring check on
core-cli.ts's own source won't catch a forbidden module being pulled in transitively through one ofcore-cli.ts's own imports (e.g., ifworktree.jsordaemon-state.jslater imports./main.js). For a stronger guarantee that the core CLI graph excludes the TUI/runtime, consider walking the import graph (or asserting against the builtdist/bundle) rather than just the entry file's literal text.🤖 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-sidecar-boundary.test.ts` around lines 58 - 65, The current boundary test in core-sidecar-boundary.test.ts only checks literal substrings in core-cli.ts, so it can miss forbidden modules brought in transitively through imports like worktree.js or daemon-state.js. Update the test around source() and the coreCli assertion to validate the full import graph from core-cli.ts, or switch it to assert against the built dist bundle, so it fails whenever ./main.js, ./multiplexer/, ./tmux/, ./dashboard/, or ./local-ui-server.js are reachable indirectly.src/core-cli.test.ts (1)
167-185: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest doesn't exercise the fallback
projectsmapping.This test sets
mocks.daemonState.projects.repobut sincemocks.daemonInfois alsonull,runDaemonStatus's text-output path short-circuits at the!daemoncheck (core-cli.ts Line 152-155) before ever printingKnown projects/Live project services, so the populateddaemonState.projectsfallback mapping (core-cli.ts Line 141-146) is never actually asserted. Consider adding a--jsonassertion (or settingdaemonInfonon-null with the fetch still rejecting) to verify the merged fallbackprojectsarray is correct.🤖 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 167 - 185, The daemon status test is only covering the offline text fallback and never verifies the merged fallback projects mapping. Update the `runDaemonStatus` coverage in `src/core-cli.test.ts` so it actually exercises the `daemonState.projects` fallback in `runDaemonStatus` and asserts the resulting `projects` data, either by adding a `--json` case or by keeping `daemonInfo` non-null while `requestCoreCommand` still rejects. Use the `runDaemonStatus` and `mocks.daemonState.projects` symbols to locate the relevant setup and expectations.
🤖 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-cli.ts`:
- Around line 25-38: The command argument filtering in commandArgs is duplicated
and can drift between core-cli and launcher-env, causing routing and parsing
mismatches. Extract the shared global-flag stripping logic into a single
reusable helper module (for example, a shared CLI args utility) and have both
commandArgs call sites use that same function so cliEntryFor and runCoreCli stay
consistent when flags change.
In `@src/launcher-env.ts`:
- Around line 39-50: `isCoreCliCommand` is duplicating the core-command routing
list from `runCoreCli`, so keep them in lockstep by extracting the
command/subcommand contract into a shared source and using it in both
`src/launcher-env.ts` and `src/core-cli.ts`. Update the routing logic around
`isCoreCliCommand` and the `runCoreCli` dispatch so they read from the same
centralized list of supported pairs, preventing one side from accepting a
command the other rejects.
---
Nitpick comments:
In `@src/core-cli.test.ts`:
- Around line 167-185: The daemon status test is only covering the offline text
fallback and never verifies the merged fallback projects mapping. Update the
`runDaemonStatus` coverage in `src/core-cli.test.ts` so it actually exercises
the `daemonState.projects` fallback in `runDaemonStatus` and asserts the
resulting `projects` data, either by adding a `--json` case or by keeping
`daemonInfo` non-null while `requestCoreCommand` still rejects. Use the
`runDaemonStatus` and `mocks.daemonState.projects` symbols to locate the
relevant setup and expectations.
In `@src/core-cli.ts`:
- Around line 169-180: The project-listing logic in runDaemonProjects and
runProjectsList is duplicated and likely to drift. Extract a shared helper
around the CORE_COMMAND_NAMES.projectsList fetch and the common name/badge/path
rendering, and parameterize the badge labels so runDaemonProjects can use
“service/idle” while runProjectsList uses “live/idle” and its empty-list
message. Keep the shared formatting close to the existing runDaemonProjects and
runProjectsList flow so both callers use the same formatter instead of
maintaining separate copy-pasted loops.
In `@src/core-sidecar-boundary.test.ts`:
- Around line 58-65: The current boundary test in core-sidecar-boundary.test.ts
only checks literal substrings in core-cli.ts, so it can miss forbidden modules
brought in transitively through imports like worktree.js or daemon-state.js.
Update the test around source() and the coreCli assertion to validate the full
import graph from core-cli.ts, or switch it to assert against the built dist
bundle, so it fails whenever ./main.js, ./multiplexer/, ./tmux/, ./dashboard/,
or ./local-ui-server.js are reachable indirectly.
🪄 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: d74caa05-5dc8-4012-a943-fe11fa7df68b
📒 Files selected for processing (5)
src/core-cli.test.tssrc/core-cli.tssrc/core-sidecar-boundary.test.tssrc/launcher-env.test.tssrc/launcher-env.ts
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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-cli-routing.ts`:
- Around line 23-29: The CLI argument guards in hasOnlyFlags and hasNoOptions
are still treating stray positional tokens as valid, so core routing can
silently accept malformed commands. Update these helpers in
src/core-cli-routing.ts so hasOnlyFlags permits only recognized flags and
rejects any non-flag positional arguments, and hasNoOptions requires the args
list to be empty rather than merely free of flags. This should make commands
routed through the core CLI handlers fail fast for inputs like host status foo,
projects list bar, and remote enable baz instead of ignoring the extra token.
- Around line 31-51: Update isValidCoreProjectEnsureArgs and hasProjectOption in
core-cli-routing so daemon project-ensure accepts both --project value and
--project=value. Right now hasProjectOption only checks for a separate next
token and the validation loop in isValidCoreProjectEnsureArgs rejects the equals
form before readOption() can handle it; adjust both checks to recognize the
inline assignment while still allowing only the supported flags and the
daemon/project-ensure prefix.
🪄 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: 322e3d67-7740-4c97-a8a4-a41169d9de67
📒 Files selected for processing (8)
scripts/install.shsrc/core-cli-routing.tssrc/core-cli.test.tssrc/core-cli.tssrc/launcher-bin.tssrc/launcher-env.test.tssrc/launcher-env.tssrc/one-shot-node-inventory.test.ts
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
Summary
Verification
Summary by CodeRabbit