fix(registry): preserve explicit spawn roles#333
Conversation
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_c0958626-2aa7-4eb3-a55f-f8fdb78f4693) |
|
@coderabbitai review |
|
@greptileai review |
|
@codex review |
✅ Action performedReview finished.
|
|
You need to increase your spend limit or enable usage-based billing to run background agents. Go to Cursor |
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughDiscovered surface role overrides are resolved in ChangesRole authority propagation
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
![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 `@src/agent-registry.ts`:
- Around line 540-543: Update explicitRoleFor in AgentRegistry to wrap the
caller-supplied explicitRoleProvider invocation in try/catch, matching the
fail-closed behavior of getObserverId and getObserverEpoch. On provider errors,
return null while preserving the existing null result when no provider is
configured.
In `@src/server.ts`:
- Around line 2314-2346: The matchesBinding function should prioritize surface
UUID identity over workspace matching. Apply the workspace mismatch rejection
only when uuid is unavailable, so a matching surfaceUuid remains valid after a
surface moves workspaces; preserve the existing UUID comparison and role
resolution behavior in explicitRoleForDiscoveredSurface.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: ad8d41ac-2cd9-4bfd-af1b-201be09ee725
📒 Files selected for processing (5)
src/agent-registry.tssrc/server.tssrc/state-manager.tstests/agent-engine.test.tstests/server.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: test
🧰 Additional context used
📓 Path-based instructions (7)
tests/**/*.test.ts
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.test.ts: Mirror source layout in tests (src/foo.ts->tests/foo.test.ts).
Do not add integration tests that require a running cmux instance; tests should be fully mocked.
Files:
tests/agent-engine.test.tstests/server.test.ts
tests/**/*agent-engine*.test.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Agent engine tests should use 1-second timeouts for state-change detection.
Files:
tests/agent-engine.test.ts
src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.ts: Build the project with TypeScript (tsc) and keep source code compatible with Node 20+ and Zod-based typing.
Use theok(data)anderr(error)helpers for consistent MCP tool responses.
All MCP tool handlers must return{ content: TextContent[], structuredContent?, isError? }.
Files:
src/server.tssrc/state-manager.tssrc/agent-registry.ts
src/server.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Register all MCP tools in
server.ts, including the 33 tool handlers, and conditionally skip agent-lifecycle tools whenskipAgentLifecycle: true.
Files:
src/server.ts
src/state-manager.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Keep sidebar state synchronization logic in
state-manager.ts.
Files:
src/state-manager.ts
tests/server.test.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Server tests should mock the cmux client via
createServer({ exec, skipAgentLifecycle }).
Files:
tests/server.test.ts
src/agent-registry.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Maintain the registry of active agents across surfaces in
agent-registry.ts.
Files:
src/agent-registry.ts
🧠 Learnings (1)
📚 Learning: 2026-03-15T10:42:35.917Z
Learnt from: EtanHey
Repo: EtanHey/cmuxlayer PR: 1
File: tests/quality-tracking.test.ts:171-200
Timestamp: 2026-03-15T10:42:35.917Z
Learning: In tests/quality-tracking.test.ts for the cmuxlayer project, ensure that at or above 80% context quality degradation, behavior depends on depth: depth-0 agents receive a /compact command; depth > 0 agents are killed and logged (kill + log). Respawn of non-root agents is out of scope for v1. Treat the design doc quality tracking section as the authoritative source for this behavior, and align test expectations accordingly.
Applied to files:
tests/agent-engine.test.tstests/server.test.ts
🔇 Additional comments (5)
src/agent-registry.ts (1)
79-88: LGTM!Also applies to: 493-495, 508-508, 1117-1117, 1244-1244, 1256-1265, 1408-1421, 1471-1497, 1862-1865
src/state-manager.ts (1)
22-22: LGTM!Also applies to: 540-540, 570-575
tests/agent-engine.test.ts (1)
343-375: LGTM!src/server.ts (1)
7333-7333: LGTM!tests/server.test.ts (1)
18-18: LGTM!Also applies to: 6225-6450
| private explicitRoleFor(discovered: DiscoveredAgent): AgentRole | null { | ||
| return this.explicitRoleProvider?.(discovered) ?? null; | ||
| } | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
explicitRoleFor doesn't fail closed like the other provider callbacks.
getObserverId()/getObserverEpoch() in this same class wrap their provider calls in try/catch specifically to "fail closed" on an unexpected throw. explicitRoleFor invokes this.explicitRoleProvider?.(discovered) with no such guard, so a throw inside the caller-supplied explicitRoleProvider (e.g. explicitRoleForDiscoveredSurface in server.ts) would propagate uncaught through listMerged, syncAutoRecord, syncManagedRecordSurfaceMetadata, repairCandidateForDiscovery, and repairFromDiscovery — core registry operations invoked on every discovery/resync cycle.
🛡️ Proposed fix
private explicitRoleFor(discovered: DiscoveredAgent): AgentRole | null {
- return this.explicitRoleProvider?.(discovered) ?? null;
+ try {
+ return this.explicitRoleProvider?.(discovered) ?? null;
+ } catch {
+ // Losing explicit role authority must fail closed, matching
+ // getObserverId()/getObserverEpoch() above.
+ return null;
+ }
}📝 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.
| private explicitRoleFor(discovered: DiscoveredAgent): AgentRole | null { | |
| return this.explicitRoleProvider?.(discovered) ?? null; | |
| } | |
| private explicitRoleFor(discovered: DiscoveredAgent): AgentRole | null { | |
| try { | |
| return this.explicitRoleProvider?.(discovered) ?? null; | |
| } catch { | |
| // Losing explicit role authority must fail closed, matching | |
| // getObserverId()/getObserverEpoch() above. | |
| return null; | |
| } | |
| } |
🤖 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/agent-registry.ts` around lines 540 - 543, Update explicitRoleFor in
AgentRegistry to wrap the caller-supplied explicitRoleProvider invocation in
try/catch, matching the fail-closed behavior of getObserverId and
getObserverEpoch. On provider errors, return null while preserving the existing
null result when no provider is configured.
| const explicitRoleForDiscoveredSurface = ( | ||
| discovered: Pick< | ||
| DiscoveredAgent, | ||
| "surface_id" | "surface_uuid" | "workspace_id" | ||
| >, | ||
| ): AgentRole | null => { | ||
| const uuid = discovered.surface_uuid?.trim().toLowerCase() || null; | ||
| const workspace = discovered.workspace_id ?? null; | ||
| const matchesBinding = ( | ||
| override: { | ||
| role: AgentRole; | ||
| workspace: string | null; | ||
| surfaceUuid: string | null; | ||
| }, | ||
| ): boolean => { | ||
| if (workspace && override.workspace && workspace !== override.workspace) { | ||
| return false; | ||
| } | ||
| const overrideUuid = override.surfaceUuid?.trim().toLowerCase() || null; | ||
| return uuid === null ? true : overrideUuid === uuid; | ||
| }; | ||
| const direct = roleSurfaceOverrides.get(discovered.surface_id); | ||
| if (direct && matchesBinding(direct)) { | ||
| return direct.role; | ||
| } | ||
| if (!uuid) return null; | ||
| const stableMatches = [...roleSurfaceOverrides.values()].filter( | ||
| (override) => | ||
| override.surfaceUuid?.trim().toLowerCase() === uuid && | ||
| matchesBinding(override), | ||
| ); | ||
| return stableMatches.length === 1 ? stableMatches[0]!.role : null; | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n "roleSurfaceOverrides" src/server.tsRepository: EtanHey/cmuxlayer
Length of output: 829
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '2840,2905p' src/server.ts
printf '\n---\n'
sed -n '5690,5745p' src/server.ts
printf '\n---\n'
sed -n '7310,7355p' src/server.ts
printf '\n---\n'
sed -n '2308,2350p' src/server.tsRepository: EtanHey/cmuxlayer
Length of output: 7360
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '5688,5735p' src/server.ts
printf '\n---\n'
sed -n '5938,5995p' src/server.ts
printf '\n---\n'
sed -n '2848,2898p' src/server.tsRepository: EtanHey/cmuxlayer
Length of output: 6940
Workspace changes shouldn't invalidate a UUID match src/server.ts:2314-2346
matchesBinding rejects an override on workspace mismatch before it checks surface_uuid. Since override.workspace is only set at creation time and move_surface doesn't refresh it, moving a surface can make the next discovery pass drop its explicit role and fall back to inference. Apply the workspace check only when UUID identity is unavailable.
🤖 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/server.ts` around lines 2314 - 2346, The matchesBinding function should
prioritize surface UUID identity over workspace matching. Apply the workspace
mismatch rejection only when uuid is unavailable, so a matching surfaceUuid
remains valid after a surface moves workspaces; preserve the existing UUID
comparison and role resolution behavior in explicitRoleForDiscoveredSurface.
1 participant
Summary
new_split.roleas authoritative when lifecycle discovery creates or repairs the registry recordRED evidence
Before the implementation, the end-to-end
new_split→ registry → Fleet sidebar test produced 2 failures and 1 pass:role: worker: recordedorchestratorinstead ofworkerrole: orchestrator: recordedworkerinstead oforchestratororchestratorA subsequent recycled-ref safety test was also introduced RED before tightening UUID matching.
Verification
env -u CMUX_SOCKET_PATH -u CMUX_DAEMON_SOCKET bun run test— 104 files, 2210 tests passedbun run typecheck— passedbun run build— passedenv -u CMUX_SOCKET_PATH -u CMUX_DAEMON_SOCKET bun run pre-pr:harness— 4 files, 63 tests passedgit diff --check— passedspawn_agent,new_split, and non-errorcontrol_healthconfirmedScope
No renderer/layout implementation changed. Sidebar role content is verified through the published FleetSidebar snapshot in the end-to-end server test. This PR is intentionally left unmerged for lead review.
Note
Medium Risk
Changes agent registry role assignment and UUID/ref binding for role overrides during discovery and repair; behavior is covered by new E2E tests but affects fleet metadata correctness.
Overview
Explicit
new_split.roleis now authoritative when lifecycle discovery creates, repairs, or syncs registry records. CLI/title inference remains only when no explicit role was supplied at spawn.The server wires
explicitRoleProvideronAgentRegistryto resolve roles fromroleSurfaceOverrides, matching by surface ref and (when present) stable UUID plus workspace so a recycled pane ref cannot inherit the wrong override after UUID identity appears.ensureAutoRecordaccepts an optional explicit role ahead of the Claude→orchestrator / other→worker defaults. Repair and sync paths apply the same override to managed and auto-discovered rows.Tests cover
new_split→ registry → Fleet sidebar role persistence and managed spawn role cases.Reviewed by Cursor Bugbot for commit 3b85120. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Note
Preserve explicit spawn roles in agent registry during discovery and sync
explicitRoleProvideroption toAgentRegistry— a caller-supplied function that returns an authoritativeAgentRolefor a discovered agent based on its surface identity.StateManager.ensureAutoRecordto accept anexplicitRoleparameter, seeding new records with the caller-specified role instead of inferring it from the CLI type.syncAutoRecord,syncManagedRecordSurfaceMetadata) and the self-heal flow to update a managed record's role when it differs from the explicit role.server.tsby injecting anexplicitRoleForDiscoveredSurfacefunction derived fromroleSurfaceOverridesintoAgentRegistry.Macroscope summarized 3b85120.