Add Core command bus foundation#271
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 (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis PR adds a typed core-command protocol for daemon requests, including shared contract types, a client helper, daemon routing for ChangesCore Command Protocol
Node Runtime Spawn Inventory Test
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/daemon.ts (1)
1117-1157: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd exhaustive command handling instead of implicit if/else fallthrough.
The
statusbranch is reached simply becausecommand !== CORE_COMMAND_NAMES.ping, not because it was verified to equalCORE_COMMAND_NAMES.status. The return type here (CoreCommandResponse, not parameterized by the actual command) means TypeScript won't catch it if a third command is later added toCORE_COMMAND_NAMESwithout a corresponding branch — it would silently fall through and returnstatus-shaped data under the new command's name. The client-sideresponse.command !== commandcheck incore-command-client.tswouldn't catch this either, since the command name would still match.Using a
switchwith an explicitnever-exhaustiveness check oncommandin thedefaultcase would surface this at compile time as soon asCORE_COMMAND_NAMESgrows.♻️ Proposed refactor for exhaustive command routing
- const issuedAt = new Date().toISOString(); - if (command === CORE_COMMAND_NAMES.ping) { - return { status: 200, body: { ok: true, id, command, issuedAt, result: { pong: true } } }; - } - return { - status: 200, - body: { - ok: true, - id, - command, - issuedAt, - result: { - daemon: { - pid: process.pid, - port: getDaemonPort(), - serviceInfo: getProjectServiceManifest(), - }, - projects: this.listProjectsForRoute(), - updatedAt: this.state.updatedAt, - }, - }, - }; + const issuedAt = new Date().toISOString(); + switch (command) { + case CORE_COMMAND_NAMES.ping: + return { status: 200, body: { ok: true, id, command, issuedAt, result: { pong: true } } }; + case CORE_COMMAND_NAMES.status: + return { + status: 200, + body: { + ok: true, + id, + command, + issuedAt, + result: { + daemon: { + pid: process.pid, + port: getDaemonPort(), + serviceInfo: getProjectServiceManifest(), + }, + projects: this.listProjectsForRoute(), + updatedAt: this.state.updatedAt, + }, + }, + }; + default: { + const _exhaustive: never = command; + return { status: 400, body: { ok: false, id, command: _exhaustive, error: "unhandled core command" } }; + } + }🤖 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 1117 - 1157, The core command routing in routeCoreCommand currently treats every non-ping command as the status response, which can silently miss future CORE_COMMAND_NAMES additions. Replace the implicit if/else fallthrough with an explicit switch on command that handles CORE_COMMAND_NAMES.ping and CORE_COMMAND_NAMES.status separately, and add a default branch with a never exhaustiveness check so new commands in CoreCommandEnvelope/CoreCommandResponse are caught at compile time.
🤖 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.
Nitpick comments:
In `@src/daemon.ts`:
- Around line 1117-1157: The core command routing in routeCoreCommand currently
treats every non-ping command as the status response, which can silently miss
future CORE_COMMAND_NAMES additions. Replace the implicit if/else fallthrough
with an explicit switch on command that handles CORE_COMMAND_NAMES.ping and
CORE_COMMAND_NAMES.status separately, and add a default branch with a never
exhaustiveness check so new commands in CoreCommandEnvelope/CoreCommandResponse
are caught at compile time.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 558a0db8-0760-470e-90ba-cd808973f31d
📒 Files selected for processing (5)
src/core-command-client.tssrc/core-command-contract.test.tssrc/core-command-contract.tssrc/daemon.tssrc/one-shot-node-inventory.test.ts
|
Addressed CodeRabbit's exhaustive command routing finding in 8f78119 by switching Core command dispatch to an explicit switch with a never exhaustiveness guard. |
2 participants
Summary
Verification
Summary by CodeRabbit
pingandstatus.