[AR-448] Make Relay workspace identity durable across node restarts - #1402
[AR-448] Make Relay workspace identity durable across node restarts#1402khaliqgant wants to merge 2 commits into
Conversation
A node started with no project-pinned workspace fell straight through to the broker, which mints a brand-new messaging-only workspace. Nothing errored: the node came up, the resident agent registered, and it looked healthy — but it was a stranger in a different workspace with a new address, so everything sent to its previous address went nowhere. `node up` now resolves the machine-global canonical workspace (`agent-relay workspace join|switch`) before letting the broker mint one. Explicit `--workspace-key`, workspace env vars, and an existing project pin all still win; the resolved key is pinned to the project afterwards, so later starts resume it directly. A machine with no canonical workspace set behaves exactly as before. Also make the invariant observable: - `workspace active` emits a `dataPlane` object proving Relaycast, Relayfile, and RelayAuth share one data-plane workspace ID, prints the Relaycast ID it used to omit, and gains `--require-unified` to turn a divergence into a non-zero exit. - `node status` prints the durable `Workspace:` ID next to the masked key, so an operator can compare it across a restart. Secrets stay out of both paths: the canonical-workspace fallback logs only its source, never the key. specs/workspace-identity.md documents the invariant, the resolution order, and migration behavior for existing local nodes. Refs AR-448 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
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 PR adds workspace convergence reporting to ChangesWorkspace identity and convergence
Estimated code review effort: 4 (Complex) | ~60 minutes 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 |
![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: 27494c8626
ℹ️ 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 [, reference] = entries[0]!; | ||
| const divergent = entries.filter(([, id]) => id !== reference).map(([plane]) => plane); |
There was a problem hiding this comment.
When Relayfile and RelayAuth agree but Relaycast is the outlier, this always uses Relaycast as the reference and reports both agreeing planes as divergent. That makes the new diagnostic identify the wrong service(s) for the exact two-versus-one mismatch it is meant to troubleshoot; choose the majority ID as the reference before constructing divergent.
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cli/src/cli/lib/broker-lifecycle.ts (1)
1340-1343: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTrim mismatch with the later injection at Line 1407.
Here a whitespace-only
--workspace-keyis not treated as explicit (so the project pin or canonical key gets applied), but Line 1407 tests raw truthiness and then overwritesRELAY_WORKSPACE_KEY/RELAY_API_KEYwith that blank value — the broker then mints a throwaway workspace, the exact drift this change removes. Normalize once and reuse.🐛 Suggested fix
- if (options.workspaceKey) { + const explicitWorkspaceKey = options.workspaceKey?.trim(); + if (explicitWorkspaceKey) { - deps.env.RELAY_WORKSPACE_KEY = options.workspaceKey; - deps.env.RELAY_API_KEY = options.workspaceKey; + deps.env.RELAY_WORKSPACE_KEY = explicitWorkspaceKey; + deps.env.RELAY_API_KEY = explicitWorkspaceKey; }🤖 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 1340 - 1343, Normalize the workspace key once in the broker lifecycle flow and reuse that normalized value for both the explicit-key check and the later injection near the existing workspace/API key assignment. Ensure whitespace-only options.workspaceKey is treated as absent and cannot overwrite RELAY_WORKSPACE_KEY or RELAY_API_KEY, while preserving valid explicit keys and the existing project/canonical fallback behavior.
🧹 Nitpick comments (3)
packages/cloud/src/workspace-convergence.test.ts (1)
33-43: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThis test doesn't exercise its stated claim.
The input contains no cloud workspace ID, so this is identical to the first test and would still pass if
cloudWorkspaceIdwere included in the comparison. To actually pin the exclusion, pass a descriptor carrying a divergentcloudWorkspaceIdand assertunifiedis still true.♻️ Suggested change
expect( describeDataPlaneConvergence({ + // Extra field is ignored by the Pick-typed parameter at runtime. + cloudWorkspaceId: '50587328-441d-4acb-b8f3-dbe1b3c5de99', relaycastWorkspaceId: 'rw_same', relayfileWorkspaceId: 'rw_same', relayauthWorkspaceId: 'rw_same', - }).unified + } as Parameters<typeof describeDataPlaneConvergence>[0]).unified ).toBe(true);🤖 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/workspace-convergence.test.ts` around lines 33 - 43, Update the test case around describeDataPlaneConvergence to include a divergent cloudWorkspaceId in the descriptor while keeping all data-plane workspace IDs equal, then assert unified remains true. This must distinguish the cloud control-plane ID from the data-plane convergence comparison rather than duplicating the preceding test.packages/cli/src/cli/commands/workspace.test.ts (1)
4-17: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider spreading
...actualinstead of enumerating exports.The factory returns only six names, so any other
@agent-relay/cloudexport reached through this module graph resolves toundefinedand fails at import time rather than in a readable assertion. Spreading the real module and overriding just the mocked functions keeps the mock from drifting as the barrel grows.♻️ Suggested change
const actual = await importOriginal<typeof import('`@agent-relay/cloud`')>(); return { - describeDataPlaneConvergence: actual.describeDataPlaneConvergence, - formatDataPlaneDivergence: actual.formatDataPlaneDivergence, + ...actual, readWorkspaceStore: vi.fn(() => ({ workspaces: {} })),🤖 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/commands/workspace.test.ts` around lines 4 - 17, Update the `@agent-relay/cloud` mock factory to spread the imported actual module exports, then override only the workspace-related functions that must remain mocked, including readWorkspaceStore, resolveActiveWorkspace, setWorkspaceKey, and switchWorkspace. Preserve the real convergence helper implementations while ensuring all other barrel exports remain available.packages/cli/src/cli/lib/workspace-identity-restart.test.ts (1)
287-335: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated dependency harness.
This
depsobject is a near-copy of the one insidecreateMachine.start()(Lines 136-182). ExtendingcreateMachinewith an options override (e.g.start({ workspaceKey })) would let this test reuse the existing harness and keep the two copies from drifting.🤖 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/workspace-identity-restart.test.ts` around lines 287 - 335, Refactor the test dependency setup so the harness created inside createMachine.start() can accept an options override, including workspaceKey. Update the test at the shown deps object to call start({ workspaceKey }) or the equivalent override instead of duplicating the full CoreDependencies object, while preserving the existing test-specific workspace identity 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 `@specs/workspace-identity.md`:
- Line 58: Add language identifiers to the fenced code blocks in
workspace-identity.md: use json for the workspace active --json payload and
console or bash for shell transcripts, including the blocks around lines 58,
89-91, 96-102, and 143-148.
---
Outside diff comments:
In `@packages/cli/src/cli/lib/broker-lifecycle.ts`:
- Around line 1340-1343: Normalize the workspace key once in the broker
lifecycle flow and reuse that normalized value for both the explicit-key check
and the later injection near the existing workspace/API key assignment. Ensure
whitespace-only options.workspaceKey is treated as absent and cannot overwrite
RELAY_WORKSPACE_KEY or RELAY_API_KEY, while preserving valid explicit keys and
the existing project/canonical fallback behavior.
---
Nitpick comments:
In `@packages/cli/src/cli/commands/workspace.test.ts`:
- Around line 4-17: Update the `@agent-relay/cloud` mock factory to spread the
imported actual module exports, then override only the workspace-related
functions that must remain mocked, including readWorkspaceStore,
resolveActiveWorkspace, setWorkspaceKey, and switchWorkspace. Preserve the real
convergence helper implementations while ensuring all other barrel exports
remain available.
In `@packages/cli/src/cli/lib/workspace-identity-restart.test.ts`:
- Around line 287-335: Refactor the test dependency setup so the harness created
inside createMachine.start() can accept an options override, including
workspaceKey. Update the test at the shown deps object to call start({
workspaceKey }) or the equivalent override instead of duplicating the full
CoreDependencies object, while preserving the existing test-specific workspace
identity behavior.
In `@packages/cloud/src/workspace-convergence.test.ts`:
- Around line 33-43: Update the test case around describeDataPlaneConvergence to
include a divergent cloudWorkspaceId in the descriptor while keeping all
data-plane workspace IDs equal, then assert unified remains true. This must
distinguish the cloud control-plane ID from the data-plane convergence
comparison rather than duplicating the preceding test.
🪄 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: e854097d-c05b-45ed-86e5-54c53a875e38
📒 Files selected for processing (11)
CHANGELOG.mdpackages/cli/src/cli/commands/core.test.tspackages/cli/src/cli/commands/workspace.test.tspackages/cli/src/cli/commands/workspace.tspackages/cli/src/cli/lib/broker-lifecycle.tspackages/cli/src/cli/lib/workspace-identity-restart.test.tspackages/cloud/src/index.tspackages/cloud/src/workspace-convergence.test.tspackages/cloud/src/workspace-convergence.tspackages/cloud/src/workspace-key.tsspecs/workspace-identity.md
|
|
||
| ## 3. Proving the invariant | ||
|
|
||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add languages to the fenced code blocks.
markdownlint flags MD040 on Lines 58, 89, 96, and 143. Use json for the workspace active --json payload and console (or bash) for the shell transcripts; console also satisfies MD014 for the $-prefixed commands.
Also applies to: 89-91, 96-102, 143-148
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 58-58: 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 `@specs/workspace-identity.md` at line 58, Add language identifiers to the
fenced code blocks in workspace-identity.md: use json for the workspace active
--json payload and console or bash for shell transcripts, including the blocks
around lines 58, 89-91, 96-102, and 143-148.
Source: Linters/SAST tools
1 participant
Closes AR-448.
The bug
A node started with no project-pinned workspace fell straight through to the broker, which mints a brand-new messaging-only workspace (
startup_single_session_set_from_sources,crates/broker/src/relaycast/auth.rs).Nothing errored. The node came up, the resident agent registered, everything looked healthy — but the agent was a stranger in a different workspace with a new address, so every message sent to its previous address went nowhere.
khaliq-chiefbecame a new process-lifetime agent on each such start.The fix
node up(and thelocal upalias) now resolves the workspace in this order:--workspace-key/RELAY_WORKSPACE_KEY/AGENT_RELAY_WORKSPACE_KEY/RELAY_API_KEY, or aRELAY_NODE_TOKENthat already implies a workspace;agent-relay workspace join|switch) — new;Step 3 is the fix. The resolved key is pinned to the project after startup, so later starts take step 2 and no key is ever copied by hand. A machine with no canonical workspace set behaves exactly as before.
Also made the invariant observable rather than assumed:
workspace active --jsongains adataPlaneobject (unified, the sharedworkspaceId, per-plane IDs, and the names of any that diverge). The human output prints the Relaycast ID it previously omitted.--require-unifiedturns a divergence into a non-zero exit for setup doctors and supervisors.node statusprints the durableWorkspace:ID next to the masked workspace key.Acceptance
workspace active --jsonproves one data-plane workspace ID — run against the livedefaultworkspace:node upuses the canonical workspace with no manual key copying —core.test.ts› up falls back to the machine-global canonical workspace when nothing else selects one, and up prefers the project pin over the machine-global canonical workspace.Stop/start regression test —
packages/cli/src/cli/lib/workspace-identity-restart.test.ts. Eachstart()is a fresh CLI process (new env, new deps) over one persistent checkout andAGENT_RELAY_HOME, which is what a stop/start looks like from the CLI. It models the two behaviors that decide the outcome: the broker joinsRELAY_WORKSPACE_KEYwhen set and otherwise mints, and Relaycast returns the existing agent when a name is re-registered in a workspace it already belongs to.--workspace-keystill winsStatus and logs never print credentials —
core.test.ts› status reports the durable workspace ID without leaking any credential asserts the raw workspace key, node token,ot_live_tokens, and?token=-style URLs are all absent.workspace.test.tsasserts the same forworkspace activehuman output. The canonical-workspace fallback logs only its source, never the key.Documented —
specs/workspace-identity.mdcovers the invariant, resolution order, how to prove it, secret handling, and migration for existing local nodes (including the one behavior change: a node with no project pin now joins the canonical workspace instead of minting, which moves its resident agents there).Test evidence
Full
packages/cli+packages/cloud: 1095 passed, 20 skipped, 0 failed.npm run typecheckclean;npm run lint0 errors (no new warnings).One pre-existing environmental failure,
integration-relayfile-contract.test.ts› replaces a stale v1 daemon only when the installed binary can serve v3, reproduces identically on a stashed clean tree — the sandbox can't invokelsof. Unrelated to this change.Note
node up's test harness now defaultsAGENT_RELAY_HOMEto an isolated temp dir. Without that, the new store fallback made tests read (and nearly print) whatever workspace the developer's own machine had active.Do not merge — the principal owns the merge gate.
🤖 Generated with Claude Code