feat(cli): add embeddable node lifecycle#1269
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
📝 WalkthroughWalkthroughAdds a process-safe embedded Node.js broker lifecycle API with structured startup, shutdown, status, output capture, dependency injection, package exports, documentation, and tests. ChangesEmbedded Node lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Host
participant startEmbeddedNode
participant runUpCommand
participant CoreRelay
Host->>startEmbeddedNode: startEmbeddedNode(options, runtime)
startEmbeddedNode->>runUpCommand: run up with embedded dependencies
runUpCommand->>CoreRelay: create relay and start broker
CoreRelay-->>startEmbeddedNode: readiness and lifecycle output
startEmbeddedNode-->>Host: return handle and completion
Host->>startEmbeddedNode: handle.stop('SIGTERM')
startEmbeddedNode->>runUpCommand: invoke registered shutdown handler
runUpCommand-->>Host: return structured completion
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 81c6267166
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| releaseHold: Deferred<void>, | ||
| dependencyOverrides: Partial<CoreDependencies> | ||
| ): CoreDependencies { | ||
| const env = { ...(runtime.env ?? dependencyOverrides.env ?? process.env) }; |
There was a problem hiding this comment.
When an embedder supplies runtime.env to isolate the broker, this clone is later passed through createDefaultRelay/createRuntimeClient into buildBrokerSpawnConfig, which still merges process.env as the parent environment before applying the supplied env. In embedded hosts with host-only RELAY_*, AGENT_RELAY_*, or secret variables, those values still leak into the spawned broker and can select the wrong workspace/state or expose host credentials despite the API contract saying the broker only sees the provided env.
Useful? React with 👍 / 👎.
| onStderr: (line) => recorder.emit('error', [`[broker] ${line}`]), | ||
| })); | ||
|
|
||
| return withDefaults({ |
There was a problem hiding this comment.
Embedded callers inherit withDefaults()'s cliScript value, which is derived from process.argv[1] (the host application's entrypoint), not from the installed agent-relay package. In a normal embedded app that means ensureBundledAgentRelayMcpCommand() looks next to the host script, fails to find dist/cli/agent-relay-mcp.js, and the broker falls back to npx -y agent-relay mcp for spawned agents, causing offline/version-mismatched embeds instead of using the bundled MCP server.
Useful? React with 👍 / 👎.
![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
🤖 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 `@packages/cli/src/cli/lib/node-embedded.ts`:
- Around line 110-113: Bound the output recorder in the emit function used by
the embedded node so it retains only a fixed maximum number of recent entries.
After adding each new entry to output, remove the oldest entries when the cap is
exceeded, preserving the final command snapshot while preventing unbounded
growth during long-lived runs.
- Around line 121-133: Update resultFrom so the error === undefined path always
returns a successful result without inspecting historical snapshot entries. For
actual failures, replace the error-only lookup with findLast to select the most
recent entry whose level is error or warn, preserving the existing fallback
message and returned output.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 339370b3-994f-4c19-973e-deced49943fb
📒 Files selected for processing (9)
CHANGELOG.mdpackages/cli/README.mdpackages/cli/package.jsonpackages/cli/src/cli/commands/core.tspackages/cli/src/cli/lib/broker-lifecycle.tspackages/cli/src/cli/lib/node-embedded.tspackages/cli/src/node-embedded.test.tspackages/cli/src/node-embedded.tspackages/cli/tsconfig.json
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
1 participant
Summary
agent-relay/node-embeddedwith structuredstartEmbeddedNode,downEmbeddedNode, andstatusEmbeddedNoderesultsrunUpCommand/runDownCommand/runStatusCommandlifecycle implementation throughCoreDependenciesinjectionBrokerLifecycleExitError, capture lifecycle signal handlers behind an idempotent handle, isolate the host environment, and route CLI/provider output without global console or inherited Python-provider stdio ownershipLifecycle ownership
Embedded startup supports foreground mode only. Any truthy
backgroundvalue returns a structured code-2 failure. The value forwarded torunUpCommandis authoritatively set tofalseafter spreading caller options, so stateful JavaScript getters/proxies cannot reach the detached-spawn path. The host forwards its own signals throughhandle.stop(); Relay does not install SIGINT/SIGTERM listeners on the host.Exit-site audit
All 14
deps.exitsites were traced on the final lifecycle implementation: 11 inrunUpCommand, 0 inrunDownCommand, and 3 inrunStatusCommand.runUpCommandexits at current lines 1129, 1133, 1148, 1195, and 1201 are structurally unreachable because embedded startup always forwards a plainbackground: false. Static truthy values and false-then-true accessors were independently probed with zero detached spawns.BrokerLifecycleExitError; host-process survival was independently exercised through both nested chains.runStatusCommandexits at lines 1520, 1531, and 1563 all receive the injected dependencies and return structured failures; stopped-after-wait, live-PID/API-unavailable, and invalid-wait paths were exercised.A static regression test pins the 11 + 0 + 3 count and bans raw
process.exit, SIGINT/SIGTERMprocess.on, directconsole.*, and inherited stdio from the embedded lifecycle implementation.Verification
npm run typechecknpm --prefix packages/cli run lint(passes with pre-existing warnings only)npm --prefix packages/cli run buildagent-relay/node-embeddednpm ci --dry-run --ignore-scriptsThe CodeQL regex annotation is pre-existing at blame commit
83a19438aband outside this PR's changed hunks.No publish or deploy performed.