fix(cli): make workspace activation reversible and broker binding explicit - #1425
fix(cli): make workspace activation reversible and broker binding explicit#1425khaliqgant wants to merge 1 commit into
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. |
📝 WalkthroughWalkthroughThe CLI now supports workspace restoration and project rebinding. Workspace stores record prior selections. Broker connections record binding provenance. Node restarts resume project bindings. Detached startup failures and JSON output receive improved reporting. ChangesWorkspace and broker lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant runUpCommand
participant DetachedBroker
participant ReadinessPolling
participant ErrorFile
CLI->>runUpCommand: start detached broker
runUpCommand->>DetachedBroker: spawn child
runUpCommand->>ReadinessPolling: poll with child PID
DetachedBroker->>ErrorFile: persist redacted startup error
ReadinessPolling-->>runUpCommand: report child exit
runUpCommand->>ErrorFile: read and report child error
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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.
Actionable comments posted: 1
🤖 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 `@CHANGELOG.md`:
- Around line 18-20: Update the changelog by moving the node status
workspace-provenance entry from Fixed to Changed as its own concise,
impact-first bullet. Split the remaining enrolled-node restart binding
correction and detached startup child-failure correction into separate bullets
under Fixed, preserving their distinct user-visible impacts.
🪄 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: 66e3664b-ffc3-43d9-8ec1-0e311596887e
📒 Files selected for processing (16)
CHANGELOG.mdpackages/cli/README.mdpackages/cli/src/cli/bootstrap.test.tspackages/cli/src/cli/commands/core.test.tspackages/cli/src/cli/commands/node.test.tspackages/cli/src/cli/commands/node.tspackages/cli/src/cli/commands/workspace.test.tspackages/cli/src/cli/commands/workspace.tspackages/cli/src/cli/lib/broker-lifecycle.test.tspackages/cli/src/cli/lib/broker-lifecycle.tspackages/cli/src/cli/lib/workspace-session.test.tspackages/cli/src/cli/lib/workspace-session.tspackages/cli/src/cli/telemetry/client.test.tspackages/cli/src/cli/telemetry/client.tspackages/cloud/src/workspace-store.test.tspackages/cloud/src/workspace-store.ts
| ### Fixed | ||
|
|
||
| - `node status` reports whether its workspace came from `--workspace-key`, the environment, the project pin, or the broker fallback; enrolled-node restarts keep the project workspace binding, and detached startup surfaces early child failures without trying to kill an already exited process. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Move the status-output entry to Changed and split the unrelated fixes.
node status provenance is new output behavior. It is not a defect correction. Keep the enrolled-node restart and detached-child failure corrections as separate Fixed entries.
Proposed changelog update
### Changed
- `workspace create` warns on stderr when it changes the active workspace and records the prior name; named switches now record the same restore point, and first-run telemetry notices no longer contaminate JSON stdout.
+- `node status` now reports whether its workspace came from `--workspace-key`, the environment, the project pin, or the broker fallback.
### Fixed
-- `node status` reports whether its workspace came from `--workspace-key`, the environment, the project pin, or the broker fallback; enrolled-node restarts keep the project workspace binding, and detached startup surfaces early child failures without trying to kill an already exited process.
+- Enrolled-node restarts keep the project workspace binding.
+- Detached startup surfaces early child failures without trying to kill an already exited process.As per coding guidelines, use supported sections and concise impact-first bullets. Based on learnings, split a bullet when it bundles genuinely distinct user-visible changes.
🤖 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 `@CHANGELOG.md` around lines 18 - 20, Update the changelog by moving the node
status workspace-provenance entry from Fixed to Changed as its own concise,
impact-first bullet. Split the remaining enrolled-node restart binding
correction and detached startup child-failure correction into separate bullets
under Fixed, preserving their distinct user-visible impacts.
Sources: Coding guidelines, Learnings
![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: d5db49942a
ℹ️ 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".
| const file = deps.env.AGENT_RELAY_BACKGROUND_START_ERROR_FILE?.trim(); | ||
| if (!file) return; | ||
| try { | ||
| deps.fs.writeFileSync(file, `${message}\n`, 'utf-8'); |
There was a problem hiding this comment.
When an automatically loaded project .env sets AGENT_RELAY_BACKGROUND_START_ERROR_FILE, any foreground broker startup failure reaches recordBackgroundStartError and overwrites that arbitrary path with the current user's permissions. Because bootstrap.ts loads .env before command execution and this writer does not verify that it is actually running as the detached child, an untrusted repository can use a relative path traversal or absolute path to clobber files outside the project; derive or validate this path against the broker state directory instead of trusting the environment value.
Useful? React with 👍 / 👎.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 16 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/cli/README.md">
<violation number="1" location="packages/cli/README.md:76">
P2: `node up` also accepts the legacy `RELAY_API_KEY` workspace-key alias, and `RELAY_WORKSPACE_KEY` takes precedence over `AGENT_RELAY_WORKSPACE_KEY`; the documented environment step omits that supported input and leaves conflicting environments ambiguous. Including all aliases in resolver order would keep the binding documentation accurate.</violation>
<violation number="2" location="packages/cli/README.md:84">
P1: Enrolled-node restarts do not currently resume the project pin as documented: `RELAY_NODE_TOKEN` takes the broker-created fallback branch before the project pin is read, so the restarted broker can bind to a different workspace. The restart path should restore the project workspace key alongside the enrolled identity, or this documentation should be corrected to describe the actual precedence.</violation>
</file>
<file name="packages/cli/src/cli/lib/workspace-session.test.ts">
<violation number="1" location="packages/cli/src/cli/lib/workspace-session.test.ts:157">
P3: The test name says "without changing ... old enrollment" but the assertion `readProjectWorkspaceSession(projectDataDir)?.enrolledNodeId` expects `toBeUndefined()` — that means the old enrollment **is** changed (cleared). The name should say "clears old enrollment" to match the behavior and the PR description's stated intent.</violation>
</file>
<file name="packages/cli/src/cli/commands/workspace.ts">
<violation number="1" location="packages/cli/src/cli/commands/workspace.ts:85">
P3: `workspace create --json` is a no-op: the command always emits JSON, so this newly advertised flag does not change behavior. Removing the option (to preserve the existing always-JSON contract) or making output conditional on `o.json` would keep the CLI surface consistent with its implementation.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| directly by `node up`; workspace commands copy a selected key into the project | ||
| pin. Likewise, the Fleet enrollment store supplies node identity and a node | ||
| token, not a messaging workspace key. On an enrolled-node restart, Relay now | ||
| resumes the project pin alongside the enrolled identity so the two cannot drift. |
There was a problem hiding this comment.
P1: Enrolled-node restarts do not currently resume the project pin as documented: RELAY_NODE_TOKEN takes the broker-created fallback branch before the project pin is read, so the restarted broker can bind to a different workspace. The restart path should restore the project workspace key alongside the enrolled identity, or this documentation should be corrected to describe the actual precedence.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/cli/README.md, line 84:
<comment>Enrolled-node restarts do not currently resume the project pin as documented: `RELAY_NODE_TOKEN` takes the broker-created fallback branch before the project pin is read, so the restarted broker can bind to a different workspace. The restart path should restore the project workspace key alongside the enrolled identity, or this documentation should be corrected to describe the actual precedence.</comment>
<file context>
@@ -47,6 +47,47 @@ agent-relay node agent release <name>
+directly by `node up`; workspace commands copy a selected key into the project
+pin. Likewise, the Fleet enrollment store supplies node identity and a node
+token, not a messaging workspace key. On an enrolled-node restart, Relay now
+resumes the project pin alongside the enrolled identity so the two cannot drift.
+
+For detached startup failures, `node up --background` reports the child error
</file context>
| Broker workspace selection on `node up` is, in order: | ||
|
|
||
| 1. `--workspace-key` / `--wk`; | ||
| 2. `AGENT_RELAY_WORKSPACE_KEY` or `RELAY_WORKSPACE_KEY` from the environment; |
There was a problem hiding this comment.
P2: node up also accepts the legacy RELAY_API_KEY workspace-key alias, and RELAY_WORKSPACE_KEY takes precedence over AGENT_RELAY_WORKSPACE_KEY; the documented environment step omits that supported input and leaves conflicting environments ambiguous. Including all aliases in resolver order would keep the binding documentation accurate.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/cli/README.md, line 76:
<comment>`node up` also accepts the legacy `RELAY_API_KEY` workspace-key alias, and `RELAY_WORKSPACE_KEY` takes precedence over `AGENT_RELAY_WORKSPACE_KEY`; the documented environment step omits that supported input and leaves conflicting environments ambiguous. Including all aliases in resolver order would keep the binding documentation accurate.</comment>
<file context>
@@ -47,6 +47,47 @@ agent-relay node agent release <name>
+Broker workspace selection on `node up` is, in order:
+
+1. `--workspace-key` / `--wk`;
+2. `AGENT_RELAY_WORKSPACE_KEY` or `RELAY_WORKSPACE_KEY` from the environment;
+3. the current project's `.agentworkforce/relay/workspace-key.json` pin;
+4. a deterministic Relaycast workspace created by the broker when no key was supplied.
</file context>
| 2. `AGENT_RELAY_WORKSPACE_KEY` or `RELAY_WORKSPACE_KEY` from the environment; | |
| 2. `RELAY_WORKSPACE_KEY`, `AGENT_RELAY_WORKSPACE_KEY`, or the legacy `RELAY_API_KEY` from the environment (in that order); |
| setWorkspaceKey('default', 'rk_live_default', env); | ||
| writeProjectWorkspaceKey(projectDataDir, 'rk_live_old', { enrolledNodeId: 'node_old' }); | ||
|
|
||
| pinProjectWorkspaceSession({ workspaceKey: 'rk_live_default', projectDataDir, env }); |
There was a problem hiding this comment.
P3: The test name says "without changing ... old enrollment" but the assertion readProjectWorkspaceSession(projectDataDir)?.enrolledNodeId expects toBeUndefined() — that means the old enrollment is changed (cleared). The name should say "clears old enrollment" to match the behavior and the PR description's stated intent.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/cli/src/cli/lib/workspace-session.test.ts, line 157:
<comment>The test name says "without changing ... old enrollment" but the assertion `readProjectWorkspaceSession(projectDataDir)?.enrolledNodeId` expects `toBeUndefined()` — that means the old enrollment **is** changed (cleared). The name should say "clears old enrollment" to match the behavior and the PR description's stated intent.</comment>
<file context>
@@ -122,4 +146,18 @@ describe('workspace session persistence', () => {
+ setWorkspaceKey('default', 'rk_live_default', env);
+ writeProjectWorkspaceKey(projectDataDir, 'rk_live_old', { enrolledNodeId: 'node_old' });
+
+ pinProjectWorkspaceSession({ workspaceKey: 'rk_live_default', projectDataDir, env });
+
+ expect(readProjectWorkspaceKey(projectDataDir)).toBe('rk_live_default');
</file context>
| .description('Create a new workspace and store its key') | ||
| .argument('<name>', 'Workspace name') | ||
| .option('--base-url <url>', 'Override the API base URL') | ||
| .option('--json', 'Output the created workspace as JSON') |
There was a problem hiding this comment.
P3: workspace create --json is a no-op: the command always emits JSON, so this newly advertised flag does not change behavior. Removing the option (to preserve the existing always-JSON contract) or making output conditional on o.json would keep the CLI surface consistent with its implementation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/cli/src/cli/commands/workspace.ts, line 85:
<comment>`workspace create --json` is a no-op: the command always emits JSON, so this newly advertised flag does not change behavior. Removing the option (to preserve the existing always-JSON contract) or making output conditional on `o.json` would keep the CLI surface consistent with its implementation.</comment>
<file context>
@@ -78,13 +82,19 @@ export function registerWorkspaceCommands(
.description('Create a new workspace and store its key')
.argument('<name>', 'Workspace name')
.option('--base-url <url>', 'Override the API base URL')
+ .option('--json', 'Output the created workspace as JSON')
.option('--reveal-secrets', 'Include the raw workspace key in the output')
.action(async (name: string, o: Record<string, unknown>) => {
</file context>
1 participant
Summary
workspace createactivating, but record the displaced workspace and emit the required restore warning on stderrworkspace restorewith explicit no-record, deleted-record, and already-active errorsworkspace rebind <name>to pin only the current project and clear its old enrolled-node associationnode statusBinding diagnosis
node updoes not directly read the machine-global~/.agentworkforce/relay/workspaces.jsonactive selection. Its effective precedence is:--workspace-keyAGENT_RELAY_WORKSPACE_KEY/RELAY_WORKSPACE_KEY.agentworkforce/relay/workspace-key.jsonpinFleet enrollment contributes node identity/token, not the messaging workspace key. Workspace commands bridge the global named store into the project pin. The project file also acts as a broker-written cache after startup, which explains why editing it without fixing the startup authority did not stick.
I kept the original and addendum work together because the restore path, project rebind, restart behavior, and provenance all share the same workspace-session authority; splitting them would leave the reported recovery incomplete.
workspace switchdecisionYes: named
workspace switchoperations record the previous workspace through the same store primitive as create/restore. This is symmetric and makes every genuine active-workspace change reversible. Re-selecting the already-active workspace does not overwrite the restore point.Validation
Test Files 8 passed (8);Tests 177 passed (177)Test Files 3 passed (3);Tests 28 passed (28)Test Files 1 passed (1);Tests 10 passed | 4 skipped (14)npm run typecheck: passednpm run format:check: passednpm run lint: passed with 0 errors (76 existing warning-level findings)restoreandrebindgit diff --check: passedFull suite at commit
d5db49942, verbatim result:The sole failure is the existing relayfile stale-daemon contract:
@relayfile/clientdoes not complete itslsofownership probe within 1000 ms during the full run. The exact test passes in isolation. On untouchedorigin/main(f0179c81d), the same isolated-environment sequential full run also failed that relayfile test and additionally timed out indoctor:Accordingly, the definition-of-done item "existing test suite passes" remains baseline-limited; all feature-specific and static gates pass.
Safety
No real
workspace create,switch, orrestorecommand was executed. The broker was not stopped or restarted.