fix(cli): resolve the broker workspace through one precedence ladder - #1412
fix(cli): resolve the broker workspace through one precedence ladder#1412willwashburn wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughWorkspace startup now resolves workspaces from CLI, environment, repository, or machine-global sources. It persists workspace IDs, reports the selected source, joins active workspaces in fresh directories, and rejects conflicts between repository pins and node enrollment. ChangesWorkspace startup resolution
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant runUpCommand
participant WorkspaceResolver
participant NodeEnrollment
participant Broker
User->>runUpCommand: start broker or node
runUpCommand->>WorkspaceResolver: resolve workspace sources
WorkspaceResolver-->>runUpCommand: return selected workspace and origin
runUpCommand->>NodeEnrollment: validate repository and enrollment IDs
NodeEnrollment-->>runUpCommand: approve or reject startup
runUpCommand->>Broker: apply workspace selection
Broker-->>User: report joined or created workspace
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 |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
packages/cloud/src/project-workspace-key.ts (1)
162-171: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNarrow
dataDirinstead of casting.
dataDir as stringon Line 168 restates a fact the compiler can prove. Guard ondataDirfirst, then read the session. This removes the cast and keeps the flow explicit.♻️ Proposed refactor
- const dataDir = options.projectDataDir ?? projectDataDir(options.projectRoot); - const project = dataDir ? readProjectWorkspaceSession(dataDir) : undefined; - if (project) { - return { - key: project.workspaceKey, - source: 'project', - origin: projectWorkspaceKeyPath(dataDir as string), - ...(project.workspaceId ? { workspaceId: project.workspaceId } : {}), - }; - } + const dataDir = options.projectDataDir ?? projectDataDir(options.projectRoot); + if (dataDir) { + const project = readProjectWorkspaceSession(dataDir); + if (project) { + return { + key: project.workspaceKey, + source: 'project', + origin: projectWorkspaceKeyPath(dataDir), + ...(project.workspaceId ? { workspaceId: project.workspaceId } : {}), + }; + } + }🤖 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 `@packages/cloud/src/project-workspace-key.ts` around lines 162 - 171, Update the project workspace lookup around dataDir and readProjectWorkspaceSession so dataDir is narrowed before reading the session, then pass the narrowed value directly to projectWorkspaceKeyPath without the `as string` cast. Preserve the existing project result and undefined behavior.packages/cli/src/cli/lib/broker-lifecycle.ts (1)
1405-1413: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse the already-resolved pin instead of reading the file twice.
resolveWorkspaceForBrokerStartreads the pin at Line 1361, andapplyWorkspaceSelectionreads the same file again here. The two reads can also disagree if the file changes between them. Return the pinned session from resolution, or pass it intoapplyWorkspaceSelection, so the file is read once per start.🤖 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 `@packages/cli/src/cli/lib/broker-lifecycle.ts` around lines 1405 - 1413, Update resolveWorkspaceForBrokerStart and applyWorkspaceSelection to reuse the already-resolved pinned workspace session instead of calling readPinnedProjectWorkspaceSession again. Pass or return that resolved pin through the existing flow, while preserving the enrolled-node environment assignment and selection behavior.
🤖 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/README.md`:
- Around line 75-78: Label the fenced startup-output example in the README with
the text language identifier by changing its opening fence to ```text, leaving
the block contents unchanged.
In `@packages/cli/src/cli/lib/broker-lifecycle.ts`:
- Around line 1356-1359: Update the workspace-key resolution around
promoteWorkspaceKeyEnvAlias so it detects which supported environment variable
supplied the key before promotion, then sets origin to that variable instead of
hard-coding $RELAY_WORKSPACE_KEY. Preserve the existing key and source values
while ensuring diagnostics identify RELAY_WORKSPACE_KEY,
AGENT_RELAY_WORKSPACE_KEY, or RELAY_API_KEY accurately.
- Around line 1371-1374: Guard the machine-global workspace lookup in
resolveActiveWorkspaceSelection so readWorkspaceStore failures for malformed or
unreadable workspaces.json are treated as no active selection rather than
escaping runUpCommand. Catch the read error, emit a warning, and continue with
the existing fallback that can mint or use a lower-step workspace; preserve
normal selection behavior when the store is readable.
---
Nitpick comments:
In `@packages/cli/src/cli/lib/broker-lifecycle.ts`:
- Around line 1405-1413: Update resolveWorkspaceForBrokerStart and
applyWorkspaceSelection to reuse the already-resolved pinned workspace session
instead of calling readPinnedProjectWorkspaceSession again. Pass or return that
resolved pin through the existing flow, while preserving the enrolled-node
environment assignment and selection behavior.
In `@packages/cloud/src/project-workspace-key.ts`:
- Around line 162-171: Update the project workspace lookup around dataDir and
readProjectWorkspaceSession so dataDir is narrowed before reading the session,
then pass the narrowed value directly to projectWorkspaceKeyPath without the `as
string` cast. Preserve the existing project result and undefined behavior.
🪄 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: eea578ea-0278-41af-ac3e-c2e3b80d7e62
📒 Files selected for processing (13)
CHANGELOG.mdpackages/cli/README.mdpackages/cli/src/cli/commands/core.tspackages/cli/src/cli/commands/node.test.tspackages/cli/src/cli/commands/node.tspackages/cli/src/cli/lib/broker-lifecycle.test.tspackages/cli/src/cli/lib/broker-lifecycle.tspackages/cli/src/cli/lib/project-workspace-key.tspackages/cloud/src/index.tspackages/cloud/src/project-workspace-key.test.tspackages/cloud/src/project-workspace-key.tspackages/cloud/src/workspace-key.tspackages/harness-driver/src/client.ts
| ``` | ||
| Workspace source: repository pin (/repo/.agentworkforce/relay/workspace-key.json) | ||
| Workspace: joined rw_7ccfea89 | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language identifier to this fenced block.
Line 75 starts an unlabeled fenced block. markdownlint reports MD040. Use text for this startup-output example.
Proposed fix
-```
+```text📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ``` | |
| Workspace source: repository pin (/repo/.agentworkforce/relay/workspace-key.json) | |
| Workspace: joined rw_7ccfea89 | |
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 75-75: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@packages/cli/README.md` around lines 75 - 78, Label the fenced startup-output
example in the README with the text language identifier by changing its opening
fence to ```text, leaving the block contents unchanged.
Source: Linters/SAST tools
| const explicitEnvWorkspaceKey = promoteWorkspaceKeyEnvAlias(deps.env); | ||
| if (options.workspaceKey?.trim() || explicitEnvWorkspaceKey || deps.env.RELAY_NODE_TOKEN?.trim()) { | ||
| if (explicitEnvWorkspaceKey) { | ||
| return { key: explicitEnvWorkspaceKey, source: 'env', origin: '$RELAY_WORKSPACE_KEY' }; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Report the environment variable that actually supplied the key.
promoteWorkspaceKeyEnvAlias accepts RELAY_WORKSPACE_KEY, AGENT_RELAY_WORKSPACE_KEY, and RELAY_API_KEY, but origin is hard-coded to $RELAY_WORKSPACE_KEY. When an operator sets AGENT_RELAY_WORKSPACE_KEY or RELAY_API_KEY, the startup diagnostic names a variable that is not set. The PR objective requires the output to identify the conflicting or winning source.
Detect the source variable before promotion.
🐛 Proposed fix
+ const envAliasName = (['RELAY_WORKSPACE_KEY', 'AGENT_RELAY_WORKSPACE_KEY', 'RELAY_API_KEY'] as const).find(
+ (name) => deps.env[name]?.trim()
+ );
const explicitEnvWorkspaceKey = promoteWorkspaceKeyEnvAlias(deps.env);
if (explicitEnvWorkspaceKey) {
- return { key: explicitEnvWorkspaceKey, source: 'env', origin: '$RELAY_WORKSPACE_KEY' };
+ return { key: explicitEnvWorkspaceKey, source: 'env', origin: `$${envAliasName ?? 'RELAY_WORKSPACE_KEY'}` };
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const explicitEnvWorkspaceKey = promoteWorkspaceKeyEnvAlias(deps.env); | |
| if (options.workspaceKey?.trim() || explicitEnvWorkspaceKey || deps.env.RELAY_NODE_TOKEN?.trim()) { | |
| if (explicitEnvWorkspaceKey) { | |
| return { key: explicitEnvWorkspaceKey, source: 'env', origin: '$RELAY_WORKSPACE_KEY' }; | |
| } | |
| const envAliasName = (['RELAY_WORKSPACE_KEY', 'AGENT_RELAY_WORKSPACE_KEY', 'RELAY_API_KEY'] as const).find( | |
| (name) => deps.env[name]?.trim() | |
| ); | |
| const explicitEnvWorkspaceKey = promoteWorkspaceKeyEnvAlias(deps.env); | |
| if (explicitEnvWorkspaceKey) { | |
| return { key: explicitEnvWorkspaceKey, source: 'env', origin: `$${envAliasName ?? 'RELAY_WORKSPACE_KEY'}` }; | |
| } |
🤖 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 `@packages/cli/src/cli/lib/broker-lifecycle.ts` around lines 1356 - 1359,
Update the workspace-key resolution around promoteWorkspaceKeyEnvAlias so it
detects which supported environment variable supplied the key before promotion,
then sets origin to that variable instead of hard-coding $RELAY_WORKSPACE_KEY.
Preserve the existing key and source values while ensuring diagnostics identify
RELAY_WORKSPACE_KEY, AGENT_RELAY_WORKSPACE_KEY, or RELAY_API_KEY accurately.
| // Everything below the repository pin: the machine-global active workspace. | ||
| // Without this step a fresh checkout mints its own workspace even though the | ||
| // machine already has an active one selected. | ||
| return resolveActiveWorkspaceSelection(deps.env); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard the machine-global store read.
resolveActiveWorkspaceSelection calls readWorkspaceStore, which rethrows every error except ENOENT. A corrupt or unreadable ~/.agentworkforce/relay/workspaces.json (malformed JSON, EACCES) therefore throws here. This call runs at Line 1439, outside the try block that starts at Line 1614, so the throw escapes runUpCommand instead of producing a CLI error. Before this change the pin-only path never touched the machine-global store, so up did not fail for that reason.
Treat an unreadable store as "no selection" and warn, so startup continues and can still mint or use a lower step.
🛡️ Proposed fix
// Everything below the repository pin: the machine-global active workspace.
// Without this step a fresh checkout mints its own workspace even though the
// machine already has an active one selected.
- return resolveActiveWorkspaceSelection(deps.env);
+ // A corrupt or unreadable store must not abort startup: it is the lowest
+ // step on the ladder, so fall through to minting instead.
+ try {
+ return resolveActiveWorkspaceSelection(deps.env);
+ } catch (err) {
+ deps.warn(`Ignoring the machine-global workspace store: ${toErrorMessage(err)}`);
+ return undefined;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Everything below the repository pin: the machine-global active workspace. | |
| // Without this step a fresh checkout mints its own workspace even though the | |
| // machine already has an active one selected. | |
| return resolveActiveWorkspaceSelection(deps.env); | |
| // Everything below the repository pin: the machine-global active workspace. | |
| // Without this step a fresh checkout mints its own workspace even though the | |
| // machine already has an active one selected. | |
| // A corrupt or unreadable store must not abort startup: it is the lowest | |
| // step on the ladder, so fall through to minting instead. | |
| try { | |
| return resolveActiveWorkspaceSelection(deps.env); | |
| } catch (err) { | |
| deps.warn(`Ignoring the machine-global workspace store: ${toErrorMessage(err)}`); | |
| return undefined; | |
| } |
🤖 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 `@packages/cli/src/cli/lib/broker-lifecycle.ts` around lines 1371 - 1374, Guard
the machine-global workspace lookup in resolveActiveWorkspaceSelection so
readWorkspaceStore failures for malformed or unreadable workspaces.json are
treated as no active selection rather than escaping runUpCommand. Catch the read
error, emit a warning, and continue with the existing fallback that can mint or
use a lower-step workspace; preserve normal selection behavior when the store is
readable.
`agent-relay up` / `node up` hand-rolled a narrower workspace chain than the SDK: flag -> env -> repository pin -> give up, with a `RELAY_NODE_TOKEN` short-circuit at the top. Two failures fell out of it. A Cloud enrollment set `RELAY_NODE_TOKEN` and no workspace key, so the pin was skipped and the broker, left with no key candidates, minted a fresh workspace — re-homing an enrolled node out of its repository's workspace. And a fresh directory never consulted the machine-global store, so a first start minted a new workspace even when the machine already had an active one selected. Both now resolve through one ladder, documented in `resolveWorkspaceSelection` and in the CLI README: 1. --workspace-key / --wk 2. RELAY_WORKSPACE_KEY > AGENT_RELAY_WORKSPACE_KEY > RELAY_API_KEY 3. <project>/.agentworkforce/relay/workspace-key.json 4. the active entry in ~/.agentworkforce/relay/workspaces.json 5. create a workspace — only when nothing above resolves The repository pin always outranks the machine-global entry, and a node token selects node identity only, so it no longer suppresses steps 1-4. Startup prints the winning source (flag name, variable, or path — never key material) and says explicitly when it created a workspace rather than joined one. Each start now records the resolved workspace id on the pin, so a later start can detect a stored enrollment pointing at a different workspace and stop with both source paths named instead of silently choosing one. Fixes #1406 Fixes #1378 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P5FD3oEnWbMbAhjsdtmmiP
11ca028 to
cd457ea
Compare
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 `@packages/cli/README.md`:
- Around line 52-53: Update the workspace-resolution documentation section to
include agent-relay local up alongside agent-relay up and agent-relay node up,
making clear that all supported startup aliases follow the same precedence
ladder.
🪄 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: 7ac31281-8ba5-4bca-95b3-4112cf0b2c0f
📒 Files selected for processing (13)
CHANGELOG.mdpackages/cli/README.mdpackages/cli/src/cli/commands/core.tspackages/cli/src/cli/commands/node.test.tspackages/cli/src/cli/commands/node.tspackages/cli/src/cli/lib/broker-lifecycle.test.tspackages/cli/src/cli/lib/broker-lifecycle.tspackages/cli/src/cli/lib/project-workspace-key.tspackages/cloud/src/index.tspackages/cloud/src/project-workspace-key.test.tspackages/cloud/src/project-workspace-key.tspackages/cloud/src/workspace-key.tspackages/harness-driver/src/client.ts
🚧 Files skipped from review as they are similar to previous changes (12)
- CHANGELOG.md
- packages/harness-driver/src/client.ts
- packages/cloud/src/index.ts
- packages/cloud/src/project-workspace-key.test.ts
- packages/cli/src/cli/lib/project-workspace-key.ts
- packages/cli/src/cli/lib/broker-lifecycle.test.ts
- packages/cloud/src/workspace-key.ts
- packages/cli/src/cli/commands/core.ts
- packages/cli/src/cli/commands/node.test.ts
- packages/cli/src/cli/commands/node.ts
- packages/cli/src/cli/lib/broker-lifecycle.ts
- packages/cloud/src/project-workspace-key.ts
| `agent-relay up` and `agent-relay node up` resolve the workspace through one | ||
| precedence ladder. The first source that resolves wins: |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document the local up alias.
This section names agent-relay up and agent-relay node up, but not agent-relay local up. Add the alias so users can find the same workspace-resolution rules for every supported startup 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 `@packages/cli/README.md` around lines 52 - 53, Update the workspace-resolution
documentation section to include agent-relay local up alongside agent-relay up
and agent-relay node up, making clear that all supported startup aliases follow
the same precedence ladder.
Summary
Fixes #1406
Fixes #1378
agent-relay up/node uphand-rolled a narrower workspace chain than the SDK already had: flag → env → repository pin → give up, with aRELAY_NODE_TOKENshort-circuit at the very top. Both issues fall out of that one chain, at opposite ends.One correction to #1406's premise
node upnever readworkspaces.json. The global state that actually overrode chief's repository pin is the Fleet enrollment store:runNodeUpresolved an enrollment record and setRELAY_NODE_TOKEN— but no workspace key.resumePinnedProjectWorkspacethen returned early becauseRELAY_NODE_TOKENwas set, soworkspace-key.jsonwas never applied.startup_single_session_set_from_sourcestocreate_workspace()— minting offsha256(USER:cwd).So the enrolled node was re-homed out of its repository's workspace, and #1378's "fresh directory silently mints" is the same fall-through reached from the other side (no pin, and no step that consults the machine-global store). Writing the ladder once fixes both.
The ladder
Now implemented once in
resolveWorkspaceSelection(@agent-relay/cloud) and documented in the CLI README. First match wins:--workspace-key/--wkRELAY_WORKSPACE_KEY>AGENT_RELAY_WORKSPACE_KEY>RELAY_API_KEY<project>/.agentworkforce/relay/workspace-key.jsonactiveentry in~/.agentworkforce/relay/workspaces.jsonRules that follow:
node upin a fresh directory silently mints a new workspace instead of joining the account's active one #1378). A fresh directory joins the machine's active workspace when one exists; when a workspace really is created, startup says so instead of printing output identical to a join.relayWorkspaceIdand refuse, naming both source paths and both workspace ids — never key material.Startup output:
Changes
packages/cloud/src/project-workspace-key.ts— the ladder, with a per-steporiginstring for diagnostics;workspaceIdadded to the pin record;resolveActiveWorkspaceSelectionexposes step 4 alone for callers that inject their own fs.packages/cli/src/cli/lib/broker-lifecycle.ts—resumePinnedProjectWorkspace→resolveWorkspaceForBrokerStart+applyWorkspaceSelection; drops the node-token short-circuit, adds step 4, reports the winning source, announces mint-vs-join, persists the workspace id.packages/cli/src/cli/commands/node.ts— enrollment pickup is identity-only (runUpCommandnow owns workspace selection), plus the conflict guard.packages/harness-driver,commands/core.ts— plumbdefault_workspace_idoff/api/sessionso the id can be recorded.Applies to every broker start (
up,node up, thelocal upalias), not justnode up— two ladders is what #1406 asked us to stop having.Scope note
The Rust-side items in #1378 are deliberately not in this PR: join-by-name via
lookup_workspace, an account-level workspace list, and a TTY confirm before minting. The first two need a Relaycast API surface that doesn't exist yet, and the mint decision currently happens inside the broker child where no TTY interaction is possible. The reported symptom — a freshnode upminting instead of joining the account's active workspace — is fixed here at step 4, and minting is now announced rather than silent.Test Plan
Added regressions:
packages/cloud/src/project-workspace-key.test.ts— repository pin ahead of the machine-global entry with both present and pointing at different workspaces (node up: prefer the repository workspace over stale global active state #1406 acceptance criterion); per-sourceoriginnaming; assertion that noorigincontains its own key.packages/cli/src/cli/lib/broker-lifecycle.test.ts— pin wins over global store; pin applied even withRELAY_NODE_TOKENset (the exact re-homing condition); fresh directory joins the global active workspace (Firstnode upin a fresh directory silently mints a new workspace instead of joining the account's active one #1378); mint announced when nothing resolves;--workspace-keystill beats both; workspace id recorded on the pin; no key material in output.packages/cli/src/cli/commands/node.test.ts— conflicting enrollment vs pin exits 1 naming both sources with no credentials in the message; matching enrollment starts normally.Verification run:
npm run typecheck— clean.npx vitest run packages/cli packages/cloud packages/harness-driver— 1175 passed, 11 skipped, 0 failed.npx vitest run(full suite) — 1680 passed, 1 failed:packages/harnesses/.../local-host-sandbox.test.ts, which fails identically on a stashed (unmodified) tree — a corepack/sandbox environment issue, not this change.npm run lint— 0 errors; the warnings onbroker-lifecycle.tsare pre-existing and on untouched lines.One existing test changed meaning rather than breaking:
node.test.ts"resumes a project-pinned workspace…" asserted thatnode.tsitself wroteRELAY_WORKSPACE_KEY. That responsibility moved torunUpCommand, so it now asserts the invariant it was really protecting — a project with its own pin never reaches for the machine-global enrollment store — and the env-application half is covered inbroker-lifecycle.test.ts.🤖 Generated with Claude Code
https://claude.ai/code/session_01P5FD3oEnWbMbAhjsdtmmiP
Generated by Claude Code