feat: enforce provenance-gated role placement#329
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_295cdbd8-1a99-428d-ad56-7dd6f7d53d0a) |
📝 WalkthroughWalkthroughAgent surface placement now uses canonical role columns and provenance-gated reconciliation. Surface mutations and input delivery use stable identity locks and route validation. Lifecycle boot failures gate connections and clean up pending sockets, while Return delivery explicitly transitions idle agents to working. ChangesRole placement and reconciliation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant CmuxLayerDaemon
participant AgentEngine
participant CmuxSurface
Client->>CmuxLayerDaemon: connect
CmuxLayerDaemon->>AgentEngine: await lifecycle start and boot ingestion
AgentEngine->>CmuxSurface: reconcile role placements
CmuxSurface-->>AgentEngine: topology and mutation results
AgentEngine-->>CmuxLayerDaemon: lifecycle ready
CmuxLayerDaemon-->>Client: activate connection
Possibly related PRs
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 |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| if (!leadPane) { | ||
| throw new Error("column 0 anchor is unavailable"); | ||
| } | ||
| const createdSeed = await this.client.newSplit("right", { |
There was a problem hiding this comment.
🟡 Medium src/agent-engine.ts:3785
In reconcileRolePlacements, when this.client.newSplit throws at line 3794 or the returned identity trips the collision check at line 3798–3805, the newly created seed surface/pane is left open with no cleanup. The seed variable is only assigned at line 3806 after those checks pass, so when an exception is thrown the finally block at line 3838 never sees a seed to clean up and the orphaned pane persists as an unmanaged terminal. Consider capturing the newSplit result into seed immediately and performing the collision/epoch validation inside the try/finally guarded cleanup, or using a temporary variable that is cleaned up on any throw.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @src/agent-engine.ts around line 3785:
In `reconcileRolePlacements`, when `this.client.newSplit` throws at line 3794 or the returned identity trips the collision check at line 3798–3805, the newly created seed surface/pane is left open with no cleanup. The `seed` variable is only assigned at line 3806 *after* those checks pass, so when an exception is thrown the `finally` block at line 3838 never sees a `seed` to clean up and the orphaned pane persists as an unmanaged terminal. Consider capturing the `newSplit` result into `seed` immediately and performing the collision/epoch validation inside the `try`/`finally` guarded cleanup, or using a temporary variable that is cleaned up on any throw.
| } | ||
|
|
||
| const transport = new SocketJsonRpcTransport(socket); | ||
| const mcpServer = createServer({ |
There was a problem hiding this comment.
🟠 High src/daemon.ts:560
When multiple sockets connect during boot, each connection independently calls createServer and assigns a new lifecycle engine to context.lifecycleSweepEngine before the shared lifecycleStartPromise resolves. The second connection overwrites the first engine, so the first engine's completion callback skips startSweep, while the second engine was never initialized. Boot can finish with lifecycle reconciliation and placement inactive. Serialize the lifecycle-start gate before constructing additional servers so all connections reuse the same initializing engine.
Also found in 1 other location(s)
src/agent-engine.ts:5565
markAgentWorkingis called only after the awaitedsendKeyoperation has returned and released its stable-surface write lock. For an idle agent, a concurrent idle reconciliation can acquire that lock in the gap, pass its idle-statebeforeMutationcheck, and move the pane even though Return has already submitted work. Mark the agent working while still inside the serialized mutation (for example in the Return operation's callback) so placement cannot yank a newly working pane.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @src/daemon.ts around line 560:
When multiple sockets connect during boot, each connection independently calls `createServer` and assigns a new lifecycle engine to `context.lifecycleSweepEngine` before the shared `lifecycleStartPromise` resolves. The second connection overwrites the first engine, so the first engine's completion callback skips `startSweep`, while the second engine was never initialized. Boot can finish with lifecycle reconciliation and placement inactive. Serialize the lifecycle-start gate before constructing additional servers so all connections reuse the same initializing engine.
Also found in 1 other location(s):
- src/agent-engine.ts:5565 -- `markAgentWorking` is called only after the awaited `sendKey` operation has returned and released its stable-surface write lock. For an idle agent, a concurrent idle reconciliation can acquire that lock in the gap, pass its idle-state `beforeMutation` check, and move the pane even though Return has already submitted work. Mark the agent working while still inside the serialized mutation (for example in the Return operation's callback) so placement cannot yank a newly working pane.
| : lockedRoute.surface_id; | ||
| await this.withSurfaceWrite(lockKey, async () => { | ||
| const route = await this.resolveFreshMutationRoute(threadId, "send_key"); | ||
| if ( |
There was a problem hiding this comment.
🟡 Medium src/app-server-runtime.ts:523
interruptTurn rejects the interrupt whenever surface_id changes between the two route resolutions, even when surface_uuid is unchanged. Surface refs are mutable, so a legitimate ref refresh for the same surface causes the interrupt to fail instead of sending Ctrl-C. The binding-changed check should compare normalized surface_uuid first and only fall back to comparing surface_id when no UUID exists, matching the logic in sendAgentCommand.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @src/app-server-runtime.ts around line 523:
`interruptTurn` rejects the interrupt whenever `surface_id` changes between the two route resolutions, even when `surface_uuid` is unchanged. Surface refs are mutable, so a legitimate ref refresh for the same surface causes the interrupt to fail instead of sending Ctrl-C. The binding-changed check should compare normalized `surface_uuid` first and only fall back to comparing `surface_id` when no UUID exists, matching the logic in `sendAgentCommand`.
| }); | ||
| } | ||
| } | ||
| } catch (error) { |
There was a problem hiding this comment.
🟡 Medium src/agent-engine.ts:3896
reconcileRolePlacements swallows every placement failure into the skipped summary array instead of throwing. The spawn caller awaits this method but ignores the returned summary, so an incomplete topology, a failed moveSurface, or an unavailable target column all leave the agent in the wrong column while the launch proceeds normally. Because idle reconciliation deliberately skips non-idle agents, the spawned agent can spend its entire working phase docked in the wrong role column, violating the deterministic two-column contract. Consider rethrowing placement errors (or surfacing them to the caller) when trigger === "spawn" so a failed docking attempt fails the spawn instead of being silently absorbed into summary.skipped.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @src/agent-engine.ts around line 3896:
`reconcileRolePlacements` swallows every placement failure into the `skipped` summary array instead of throwing. The spawn caller awaits this method but ignores the returned summary, so an incomplete topology, a failed `moveSurface`, or an unavailable target column all leave the agent in the wrong column while the launch proceeds normally. Because idle reconciliation deliberately skips non-idle agents, the spawned agent can spend its entire `working` phase docked in the wrong role column, violating the deterministic two-column contract. Consider rethrowing placement errors (or surfacing them to the caller) when `trigger === "spawn"` so a failed docking attempt fails the spawn instead of being silently absorbed into `summary.skipped`.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/app-server-runtime.ts (1)
513-535: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAlign
interruptTurnwith the UUID-preferred binding check
interruptTurncurrently rejects anysurface_idrotation even whensurface_uuidis unchanged, whileresolveFreshMutationRouteandsendAgentCommandallow that case. This can spuriously block a validCtrl-Con a UUID-bound surface; compare UUID first and only fall back tosurface_idwhen no UUID is present.🤖 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/app-server-runtime.ts` around lines 513 - 535, Update the binding validation in interruptTurn to prefer surface_uuid: when both routes have a UUID, accept matching UUIDs regardless of surface_id changes; only compare surface_id when no UUID is present. Keep the existing stale-binding error and sendKey flow unchanged.src/server.ts (1)
2973-3021: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftUnify
withSurfaceWritelock keys across writers
withSurfaceWritelocks byuuid:<stableSurfaceIdentity>when present, butsend_input/send_command/send_keyand background delivery still acquire the raw surface ref. That leaves the same live PTY in two lock namespaces, so a direct surface write can race with an agent relay on the same surface. Thread the stable UUID through every writer or normalize the lock key inwithSurfaceWrite.🤖 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 2973 - 3021, The withSurfaceWrite lock key is inconsistent with direct and background writers, allowing concurrent writes to the same PTY. Update all send_input, send_command, send_key, and background-delivery call sites to pass the same stableSurfaceIdentity, or otherwise normalize their keys through withSurfaceWrite so every writer uses one lock namespace while preserving existing ownership and release 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 `@src/agent-engine.ts`:
- Around line 3778-3808: Reorder the worker seed handling in the role-placement
flow: keep the collision validation for createdSeed before assignment, then
assign createdSeed to seed and targetPane, and only afterward call
assertSurfaceObserverEpochCurrent. Preserve the existing collision safeguards so
cleanup cannot close the agent’s own surface, while ensuring any epoch failure
after creation is covered by the finally cleanup.
- Around line 3838-3895: Update the seed cleanup in the moveSurface operation’s
finally block so validation, topology lookup, and closeSurface failures are
caught and logged as best-effort cleanup errors, following the existing
cleanupUnboundCreatedSurface pattern. Remove the throws from finally, preserve
observer-epoch and UUID-binding checks, and ensure cleanup failures cannot enter
the outer catch or add a duplicate summary.skipped entry after summary.moved has
already been updated.
In `@src/server.ts`:
- Around line 7402-7424: Pass splitOpts?.workspace as the workspace argument in
the withSurfaceWrite call inside newSplit, and apply the same change to the
corresponding moveSurface path. Preserve the existing lockKey, toolName, and
mutation behavior while ensuring both calls forward their available workspace
instead of relying on surface identification.
In `@tests/agent-engine.test.ts`:
- Around line 5599-5780: Add a regression test alongside the existing
reconcileRolePlacements test that makes seed closeSurface reject after
moveSurface succeeds. Assert the returned summary keeps the agent in
summary.moved and does not add a duplicate entry to summary.skipped, covering
the cleanup failure path without changing the successful move behavior.
---
Outside diff comments:
In `@src/app-server-runtime.ts`:
- Around line 513-535: Update the binding validation in interruptTurn to prefer
surface_uuid: when both routes have a UUID, accept matching UUIDs regardless of
surface_id changes; only compare surface_id when no UUID is present. Keep the
existing stale-binding error and sendKey flow unchanged.
In `@src/server.ts`:
- Around line 2973-3021: The withSurfaceWrite lock key is inconsistent with
direct and background writers, allowing concurrent writes to the same PTY.
Update all send_input, send_command, send_key, and background-delivery call
sites to pass the same stableSurfaceIdentity, or otherwise normalize their keys
through withSurfaceWrite so every writer uses one lock namespace while
preserving existing ownership and release 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: ASSERTIVE
Plan: Pro
Run ID: 0c9dacfe-b736-4bd8-a3c3-e585a60f0045
📒 Files selected for processing (16)
src/agent-engine.tssrc/agent-types.tssrc/app-server-runtime.tssrc/daemon.tssrc/layout-policy.tssrc/server.tssrc/state-manager.tssrc/surface-topology.tstests/agent-engine.test.tstests/app-server-runtime.test.tstests/daemon.test.tstests/layout-policy.test.tstests/resync-tool.test.tstests/server-agent-tools.test.tstests/sidebar-sync.test.tstests/state-manager.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Macroscope - Correctness Check
🧰 Additional context used
📓 Path-based instructions (6)
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/surface-topology.tssrc/agent-types.tssrc/state-manager.tssrc/daemon.tssrc/agent-engine.tssrc/layout-policy.tssrc/app-server-runtime.tssrc/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/**/*.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/state-manager.test.tstests/sidebar-sync.test.tstests/app-server-runtime.test.tstests/daemon.test.tstests/server-agent-tools.test.tstests/agent-engine.test.tstests/layout-policy.test.tstests/resync-tool.test.ts
src/agent-engine.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Implement agent lifecycle behavior in
agent-engine.ts, including spawning, monitoring, and quality tracking.
Files:
src/agent-engine.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/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
🧠 Learnings (2)
📚 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/state-manager.test.tstests/sidebar-sync.test.tstests/app-server-runtime.test.tstests/daemon.test.tstests/server-agent-tools.test.tstests/agent-engine.test.tstests/layout-policy.test.tstests/resync-tool.test.ts
📚 Learning: 2026-03-15T10:42:36.027Z
Learnt from: EtanHey
Repo: EtanHey/cmuxlayer PR: 1
File: tests/sidebar-sync.test.ts:79-279
Timestamp: 2026-03-15T10:42:36.027Z
Learning: In the cmuxlayer project, tests/sidebar-sync.test.ts should cover only the implemented channels: set-status, set-progress, and log. The rename-workspace and report_meta_block channels are intentionally deferred (per phase5-v2-cmux-sidebar-research.md) and must not be considered as missing test coverage. Do not flag or require tests for these two channels in this file.
Applied to files:
tests/sidebar-sync.test.ts
🪛 Biome (2.5.3)
src/agent-engine.ts
[error] 3841-3843: Unsafe usage of 'throw'.
(lint/correctness/noUnsafeFinally)
[error] 3856-3858: Unsafe usage of 'throw'.
(lint/correctness/noUnsafeFinally)
🔇 Additional comments (23)
src/agent-types.ts (1)
19-19: LGTM!Also applies to: 33-34
src/layout-policy.ts (1)
151-197: LGTM!Also applies to: 376-421, 443-443
tests/layout-policy.test.ts (1)
204-243: LGTM!Also applies to: 529-595, 664-752, 1218-1372, 1491-1596, 1632-1779
tests/resync-tool.test.ts (2)
77-99: LGTM!Also applies to: 943-1139, 1538-1559, 1661-2211
1784-1844: LGTM!Also applies to: 2212-2245
tests/state-manager.test.ts (1)
320-320: LGTM!src/surface-topology.ts (1)
4-4: LGTM!Also applies to: 313-313
src/agent-engine.ts (2)
25-25: LGTM!Also applies to: 55-65, 341-360, 600-601, 632-645, 1562-1566, 1765-1766, 2718-2718, 2963-2972, 3937-3952, 4046-4046, 4490-4492, 4760-4771, 5564-5566
3636-3654: 🩺 Stability & AvailabilityRole inference here is already guarded
inferRecordRoleonly raisesAgentRoleInferenceError, andinferRecordRoleOrNullconverts that tonull, so this sweep won't abort or skip sidebar/outbox work because of malformed role data.> Likely an incorrect or invalid review comment.src/state-manager.ts (1)
551-551: LGTM!tests/agent-engine.test.ts (1)
736-752: LGTM!Also applies to: 1119-1125, 1260-1263, 1408-1408, 1460-1466, 2170-2213, 2596-2651, 3744-3744, 5285-5598, 5782-5855, 9947-10021
src/app-server-runtime.ts (2)
285-366: LGTM!
469-511: LGTM!src/server.ts (5)
132-132: LGTM!Also applies to: 2063-2105, 2146-2254
7930-7966: LGTM!
7955-7959: 🎯 Functional CorrectnessVerify
markAgentWorkingguards its own state transition forallow_busyinterjections.This unconditionally calls
engine.markAgentWorking(args.agent_id)wheneverpress_enteris true, includingallow_busy: truenudges (e.g.dispatch_to_agent's stale-monitor nudge) that can legitimately target agents inerror/donestates. IfmarkAgentWorkingdoesn't itself gate on the current state (e.g. only transition fromidle/ready), this could resurrect a terminal agent's registry state to"working"just because a best-effort keystroke nudge landed on its dead pane.
7308-7315: 🩺 Stability & Availability
awaitLifecycleStart()gating is applied inconsistently across mutation tools.
list_agents,broadcast,resync_agents, andmy_agentsexplicitly callawaitLifecycleStart()and fail closed whencontext.lifecycleStartErroris set, butspawn_agent,new_worktree_split,spawn_in_workspace,send_to,send_to_agent, andinteractdo not. In the daemon (src/daemon.ts) this gap is covered because connections are gated beforecreateServer()'s tools become reachable, but any direct/non-daemon embedding ofcreateServer()(tests, orsrc/app-server-runtime.ts-style hosts) could still invoke placement-mutating tools likespawn_agentafter a failed boot-topology ingestion, contradicting the stated goal that "placement stays disabled if boot topology ingestion fails."Also applies to: 9110-9183, 9249-9397, 9398-9950, 10667-10807, 8012-8617
9479-9518: LGTM!Also applies to: 9533-9551, 9586-9592, 9654-9658, 9674-9674, 9692-9695, 9724-9728, 9744-9744, 9772-9774, 9882-9882
tests/app-server-runtime.test.ts (1)
592-617: LGTM!Also applies to: 678-728
tests/server-agent-tools.test.ts (1)
5309-5345: LGTM!Also applies to: 5916-5960
src/daemon.ts (1)
390-390: LGTM!Also applies to: 539-559, 568-586, 641-644
tests/daemon.test.ts (1)
21-21: LGTM!Also applies to: 1717-1843
tests/sidebar-sync.test.ts (1)
751-767: LGTM!
| let targetPane = topPaneInRoleColumn(panes.panes, role)?.ref ?? null; | ||
| let seed: CmuxNewSplitResult | null = null; | ||
| if (!targetPane && role === "worker") { | ||
| const leadPane = topPaneInRoleColumn(panes.panes, "orchestrator"); | ||
| if (!leadPane) { | ||
| throw new Error("column 0 anchor is unavailable"); | ||
| } | ||
| const createdSeed = await this.client.newSplit("right", { | ||
| pane: leadPane.ref, | ||
| surface: sourceRef, | ||
| workspace: agent.workspace_id, | ||
| type: "terminal", | ||
| stableSurfaceIdentity: agent.surface_uuid, | ||
| beforeMutation: () => | ||
| assertFreshAgentBinding(sourceRef, "worker-column seed"), | ||
| }); | ||
| this.assertSurfaceObserverEpochCurrent( | ||
| observerEpoch, | ||
| "role placement", | ||
| ); | ||
| if ( | ||
| createdSeed.surface === sourceRef || | ||
| (createdSeed.surface_id ?? null) === agent.surface_uuid | ||
| ) { | ||
| throw new Error( | ||
| "worker-column seed collided with the spawned surface binding", | ||
| ); | ||
| } | ||
| seed = createdSeed; | ||
| targetPane = seed.pane; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Worker-column seed can leak if the epoch check fires between creation and tracking.
seed = createdSeed (3806) only happens after assertSurfaceObserverEpochCurrent (3794) and the collision check (3798-3805) both succeed. If the epoch assertion throws right after newSplit actually creates the seed pane/surface in cmux, the local seed variable is never set, so the finally cleanup (if (seed) { ... }, 3839) never attempts to close it — the seed surface is permanently orphaned in the workspace.
The collision check must stay before assignment (assigning seed earlier would let finally accidentally closeSurface the agent's own real surface on a collision), but the epoch check can safely move after the collision check + assignment so any later failure still triggers cleanup of a surface already confirmed distinct from sourceRef.
🐛 Proposed fix: track the seed before any check that can still throw
const createdSeed = await this.client.newSplit("right", {
pane: leadPane.ref,
surface: sourceRef,
workspace: agent.workspace_id,
type: "terminal",
stableSurfaceIdentity: agent.surface_uuid,
beforeMutation: () =>
assertFreshAgentBinding(sourceRef, "worker-column seed"),
});
- this.assertSurfaceObserverEpochCurrent(
- observerEpoch,
- "role placement",
- );
if (
createdSeed.surface === sourceRef ||
(createdSeed.surface_id ?? null) === agent.surface_uuid
) {
throw new Error(
"worker-column seed collided with the spawned surface binding",
);
}
+ // Track the seed for cleanup before any further check that could
+ // still throw, so a later epoch failure can't leak this pane.
seed = createdSeed;
+ this.assertSurfaceObserverEpochCurrent(
+ observerEpoch,
+ "role placement",
+ );
targetPane = seed.pane;📝 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.
| let targetPane = topPaneInRoleColumn(panes.panes, role)?.ref ?? null; | |
| let seed: CmuxNewSplitResult | null = null; | |
| if (!targetPane && role === "worker") { | |
| const leadPane = topPaneInRoleColumn(panes.panes, "orchestrator"); | |
| if (!leadPane) { | |
| throw new Error("column 0 anchor is unavailable"); | |
| } | |
| const createdSeed = await this.client.newSplit("right", { | |
| pane: leadPane.ref, | |
| surface: sourceRef, | |
| workspace: agent.workspace_id, | |
| type: "terminal", | |
| stableSurfaceIdentity: agent.surface_uuid, | |
| beforeMutation: () => | |
| assertFreshAgentBinding(sourceRef, "worker-column seed"), | |
| }); | |
| this.assertSurfaceObserverEpochCurrent( | |
| observerEpoch, | |
| "role placement", | |
| ); | |
| if ( | |
| createdSeed.surface === sourceRef || | |
| (createdSeed.surface_id ?? null) === agent.surface_uuid | |
| ) { | |
| throw new Error( | |
| "worker-column seed collided with the spawned surface binding", | |
| ); | |
| } | |
| seed = createdSeed; | |
| targetPane = seed.pane; | |
| } | |
| let targetPane = topPaneInRoleColumn(panes.panes, role)?.ref ?? null; | |
| let seed: CmuxNewSplitResult | null = null; | |
| if (!targetPane && role === "worker") { | |
| const leadPane = topPaneInRoleColumn(panes.panes, "orchestrator"); | |
| if (!leadPane) { | |
| throw new Error("column 0 anchor is unavailable"); | |
| } | |
| const createdSeed = await this.client.newSplit("right", { | |
| pane: leadPane.ref, | |
| surface: sourceRef, | |
| workspace: agent.workspace_id, | |
| type: "terminal", | |
| stableSurfaceIdentity: agent.surface_uuid, | |
| beforeMutation: () => | |
| assertFreshAgentBinding(sourceRef, "worker-column seed"), | |
| }); | |
| if ( | |
| createdSeed.surface === sourceRef || | |
| (createdSeed.surface_id ?? null) === agent.surface_uuid | |
| ) { | |
| throw new Error( | |
| "worker-column seed collided with the spawned surface binding", | |
| ); | |
| } | |
| // Track the seed for cleanup before any further check that could | |
| // still throw, so a later epoch failure can't leak this pane. | |
| seed = createdSeed; | |
| this.assertSurfaceObserverEpochCurrent( | |
| observerEpoch, | |
| "role placement", | |
| ); | |
| targetPane = seed.pane; | |
| } |
🤖 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-engine.ts` around lines 3778 - 3808, Reorder the worker seed
handling in the role-placement flow: keep the collision validation for
createdSeed before assignment, then assign createdSeed to seed and targetPane,
and only afterward call assertSurfaceObserverEpochCurrent. Preserve the existing
collision safeguards so cleanup cannot close the agent’s own surface, while
ensuring any epoch failure after creation is covered by the finally cleanup.
| } finally { | ||
| if (seed) { | ||
| if (!seed.surface_id) { | ||
| throw new Error( | ||
| "worker-column seed has no stable UUID; refusing cleanup by mutable ref", | ||
| ); | ||
| } | ||
| const seedTopology = await this.collectObservedSurfaceTopology(); | ||
| const seedBinding = seedTopology?.complete | ||
| ? resolveAgentSurfaceBinding( | ||
| { | ||
| surface_id: seed.surface, | ||
| surface_uuid: seed.surface_id, | ||
| }, | ||
| seedTopology, | ||
| ) | ||
| : null; | ||
| if (!seedBinding || seedBinding.provenance !== "uuid") { | ||
| throw new Error( | ||
| `worker-column seed UUID ${seed.surface_id} is no longer uniquely bound; refusing cleanup`, | ||
| ); | ||
| } | ||
| await this.client.closeSurface(seedBinding.surfaceRef, { | ||
| workspace: seedBinding.workspaceId ?? seed.workspace, | ||
| stableSurfaceIdentity: seed.surface_id, | ||
| beforeMutation: async () => { | ||
| this.assertSurfaceObserverEpochCurrent( | ||
| observerEpoch, | ||
| "role placement seed cleanup", | ||
| ); | ||
| const freshSeedTopology = | ||
| await this.collectObservedSurfaceTopology(); | ||
| this.assertSurfaceObserverEpochCurrent( | ||
| observerEpoch, | ||
| "role placement seed cleanup", | ||
| ); | ||
| const freshSeedBinding = freshSeedTopology?.complete | ||
| ? resolveAgentSurfaceBinding( | ||
| { | ||
| surface_id: seed.surface, | ||
| surface_uuid: seed.surface_id, | ||
| }, | ||
| freshSeedTopology, | ||
| ) | ||
| : null; | ||
| if ( | ||
| !freshSeedBinding || | ||
| freshSeedBinding.provenance !== "uuid" || | ||
| freshSeedBinding.surfaceRef !== seedBinding.surfaceRef | ||
| ) { | ||
| throw new Error( | ||
| "worker-column seed binding changed before cleanup", | ||
| ); | ||
| } | ||
| }, | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Unsafe throw in finally can mask a successful move and double-report the same agent.
Biome flags exactly this pattern (noUnsafeFinally) at 3841-3843 and 3856-3858. Per ESLint's rule rationale, when return, throw, break, or continue is used in finally, control flow statements inside try and catch are overwritten, which is considered as unexpected behavior.
Concretely: if moveSurface succeeds and summary.moved.push(...) (3831) already ran, and the seed-cleanup logic inside this finally then throws (e.g. the seed's binding raced away before cleanup), that throw propagates to the outer catch (3896) which pushes a summary.skipped entry for the same agent_id. The candidate now appears in both summary.moved and summary.skipped, and the real cleanup failure reason is swallowed/replaced. This exact scenario isn't exercised by the new "serializes worker-column seed cleanup" test (its mocked closeSurface always succeeds).
Restructure so seed-cleanup failures are caught and logged as best-effort (matching cleanupUnboundCreatedSurface's pattern elsewhere in this file) instead of throwing out of the finally.
🐛 Proposed fix
} finally {
if (seed) {
- if (!seed.surface_id) {
- throw new Error(
- "worker-column seed has no stable UUID; refusing cleanup by mutable ref",
- );
- }
- const seedTopology = await this.collectObservedSurfaceTopology();
- const seedBinding = seedTopology?.complete
- ? resolveAgentSurfaceBinding(
- { surface_id: seed.surface, surface_uuid: seed.surface_id },
- seedTopology,
- )
- : null;
- if (!seedBinding || seedBinding.provenance !== "uuid") {
- throw new Error(
- `worker-column seed UUID ${seed.surface_id} is no longer uniquely bound; refusing cleanup`,
- );
- }
- await this.client.closeSurface(seedBinding.surfaceRef, {
- workspace: seedBinding.workspaceId ?? seed.workspace,
- stableSurfaceIdentity: seed.surface_id,
- beforeMutation: async () => { /* ...fresh checks... */ },
- });
+ try {
+ if (!seed.surface_id) {
+ throw new Error(
+ "worker-column seed has no stable UUID; refusing cleanup by mutable ref",
+ );
+ }
+ const seedTopology = await this.collectObservedSurfaceTopology();
+ const seedBinding = seedTopology?.complete
+ ? resolveAgentSurfaceBinding(
+ { surface_id: seed.surface, surface_uuid: seed.surface_id },
+ seedTopology,
+ )
+ : null;
+ if (!seedBinding || seedBinding.provenance !== "uuid") {
+ throw new Error(
+ `worker-column seed UUID ${seed.surface_id} is no longer uniquely bound; refusing cleanup`,
+ );
+ }
+ await this.client.closeSurface(seedBinding.surfaceRef, {
+ workspace: seedBinding.workspaceId ?? seed.workspace,
+ stableSurfaceIdentity: seed.surface_id,
+ beforeMutation: async () => { /* ...fresh checks... */ },
+ });
+ } catch (cleanupError) {
+ await this.logUnboundSurfaceCleanupWarning(
+ `role placement: failed to clean worker-column seed ${seed.surface} ` +
+ `(${seed.surface_id ?? "UUID unknown"}): ` +
+ `${cleanupError instanceof Error ? cleanupError.message : String(cleanupError)}`,
+ );
+ }
}
}📝 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.
| } finally { | |
| if (seed) { | |
| if (!seed.surface_id) { | |
| throw new Error( | |
| "worker-column seed has no stable UUID; refusing cleanup by mutable ref", | |
| ); | |
| } | |
| const seedTopology = await this.collectObservedSurfaceTopology(); | |
| const seedBinding = seedTopology?.complete | |
| ? resolveAgentSurfaceBinding( | |
| { | |
| surface_id: seed.surface, | |
| surface_uuid: seed.surface_id, | |
| }, | |
| seedTopology, | |
| ) | |
| : null; | |
| if (!seedBinding || seedBinding.provenance !== "uuid") { | |
| throw new Error( | |
| `worker-column seed UUID ${seed.surface_id} is no longer uniquely bound; refusing cleanup`, | |
| ); | |
| } | |
| await this.client.closeSurface(seedBinding.surfaceRef, { | |
| workspace: seedBinding.workspaceId ?? seed.workspace, | |
| stableSurfaceIdentity: seed.surface_id, | |
| beforeMutation: async () => { | |
| this.assertSurfaceObserverEpochCurrent( | |
| observerEpoch, | |
| "role placement seed cleanup", | |
| ); | |
| const freshSeedTopology = | |
| await this.collectObservedSurfaceTopology(); | |
| this.assertSurfaceObserverEpochCurrent( | |
| observerEpoch, | |
| "role placement seed cleanup", | |
| ); | |
| const freshSeedBinding = freshSeedTopology?.complete | |
| ? resolveAgentSurfaceBinding( | |
| { | |
| surface_id: seed.surface, | |
| surface_uuid: seed.surface_id, | |
| }, | |
| freshSeedTopology, | |
| ) | |
| : null; | |
| if ( | |
| !freshSeedBinding || | |
| freshSeedBinding.provenance !== "uuid" || | |
| freshSeedBinding.surfaceRef !== seedBinding.surfaceRef | |
| ) { | |
| throw new Error( | |
| "worker-column seed binding changed before cleanup", | |
| ); | |
| } | |
| }, | |
| }); | |
| } | |
| } | |
| } finally { | |
| if (seed) { | |
| try { | |
| if (!seed.surface_id) { | |
| throw new Error( | |
| "worker-column seed has no stable UUID; refusing cleanup by mutable ref", | |
| ); | |
| } | |
| const seedTopology = await this.collectObservedSurfaceTopology(); | |
| const seedBinding = seedTopology?.complete | |
| ? resolveAgentSurfaceBinding( | |
| { | |
| surface_id: seed.surface, | |
| surface_uuid: seed.surface_id, | |
| }, | |
| seedTopology, | |
| ) | |
| : null; | |
| if (!seedBinding || seedBinding.provenance !== "uuid") { | |
| throw new Error( | |
| `worker-column seed UUID ${seed.surface_id} is no longer uniquely bound; refusing cleanup`, | |
| ); | |
| } | |
| await this.client.closeSurface(seedBinding.surfaceRef, { | |
| workspace: seedBinding.workspaceId ?? seed.workspace, | |
| stableSurfaceIdentity: seed.surface_id, | |
| beforeMutation: async () => { | |
| this.assertSurfaceObserverEpochCurrent( | |
| observerEpoch, | |
| "role placement seed cleanup", | |
| ); | |
| const freshSeedTopology = | |
| await this.collectObservedSurfaceTopology(); | |
| this.assertSurfaceObserverEpochCurrent( | |
| observerEpoch, | |
| "role placement seed cleanup", | |
| ); | |
| const freshSeedBinding = freshSeedTopology?.complete | |
| ? resolveAgentSurfaceBinding( | |
| { | |
| surface_id: seed.surface, | |
| surface_uuid: seed.surface_id, | |
| }, | |
| freshSeedTopology, | |
| ) | |
| : null; | |
| if ( | |
| !freshSeedBinding || | |
| freshSeedBinding.provenance !== "uuid" || | |
| freshSeedBinding.surfaceRef !== seedBinding.surfaceRef | |
| ) { | |
| throw new Error( | |
| "worker-column seed binding changed before cleanup", | |
| ); | |
| } | |
| }, | |
| }); | |
| } catch (cleanupError) { | |
| await this.logUnboundSurfaceCleanupWarning( | |
| `role placement: failed to clean worker-column seed ${seed.surface} ` + | |
| `(${seed.surface_id ?? "UUID unknown"}): ` + | |
| `${cleanupError instanceof Error ? cleanupError.message : String(cleanupError)}`, | |
| ); | |
| } | |
| } | |
| } |
🧰 Tools
🪛 Biome (2.5.3)
[error] 3841-3843: Unsafe usage of 'throw'.
(lint/correctness/noUnsafeFinally)
[error] 3856-3858: Unsafe usage of 'throw'.
(lint/correctness/noUnsafeFinally)
🤖 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-engine.ts` around lines 3838 - 3895, Update the seed cleanup in the
moveSurface operation’s finally block so validation, topology lookup, and
closeSurface failures are caught and logged as best-effort cleanup errors,
following the existing cleanupUnboundCreatedSurface pattern. Remove the throws
from finally, preserve observer-epoch and UUID-binding checks, and ensure
cleanup failures cannot enter the outer catch or add a duplicate summary.skipped
entry after summary.moved has already been updated.
Source: Linters/SAST tools
| newSplit: async (direction, splitOpts) => { | ||
| const { | ||
| beforeMutation, | ||
| stableSurfaceIdentity, | ||
| ...clientOpts | ||
| } = splitOpts ?? {}; | ||
| await assertWorkspaceMutationAllowed( | ||
| "agent_engine", | ||
| splitOpts?.workspace, | ||
| ); | ||
| return client.newSplit(direction, splitOpts); | ||
| const mutate = async () => { | ||
| await beforeMutation?.(); | ||
| return client.newSplit(direction, clientOpts); | ||
| }; | ||
| return splitOpts?.surface | ||
| ? withSurfaceWrite(splitOpts.surface, mutate, { | ||
| toolName: "new_split", | ||
| lockKey: stableSurfaceIdentity | ||
| ? `uuid:${stableSurfaceIdentity.toLowerCase()}` | ||
| : splitOpts.surface, | ||
| }) | ||
| : mutate(); | ||
| }, |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map candidate symbols and read relevant slices without dumping huge files.
rg -n "withSurfaceWrite|assertSurfaceMutationAllowed|newSplit: async|moveSurface: async|closeSurface" src/server.ts
# Show the surrounding lines for the implementations and call sites.
sed -n '7300,7535p' src/server.tsRepository: EtanHey/cmuxlayer
Length of output: 10475
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read definitions if they live elsewhere in the repo.
rg -n "function withSurfaceWrite|const withSurfaceWrite|assertSurfaceMutationAllowed" srcRepository: EtanHey/cmuxlayer
Length of output: 1180
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the exact implementations around the helper and the mutation wiring.
sed -n '1,220p' src/server.ts | cat -n | sed -n '1,220p'Repository: EtanHey/cmuxlayer
Length of output: 7561
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the helper implementations that drive the policy check.
sed -n '2440,3015p' src/server.ts | cat -n | sed -n '1,620p'Repository: EtanHey/cmuxlayer
Length of output: 22038
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the two call sites with full surrounding context.
sed -n '7390,7510p' src/server.ts | cat -n | sed -n '1,180p'Repository: EtanHey/cmuxlayer
Length of output: 5349
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the policy helper and withSurfaceWrite implementation.
sed -n '2462,3005p' src/server.tsRepository: EtanHey/cmuxlayer
Length of output: 17043
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the full surface control-mode resolution path.
sed -n '2400,2475p' src/server.ts
rg -n "identify\\(|readSurfaceControlMode\\(" src/server.tsRepository: EtanHey/cmuxlayer
Length of output: 2827
Pass workspace into withSurfaceWrite for newSplit and moveSurface
These paths already have the workspace, but dropping it here makes assertSurfaceMutationAllowed(...) fall back to client.identify(surface). If that lookup fails, the check returns autonomous and the manual-mode gate is skipped. closeSurface already forwards workspace; do the same here.
🤖 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 7402 - 7424, Pass splitOpts?.workspace as the
workspace argument in the withSurfaceWrite call inside newSplit, and apply the
same change to the corresponding moveSurface path. Preserve the existing
lockKey, toolName, and mutation behavior while ensuring both calls forward their
available workspace instead of relying on surface identification.
| it("serializes worker-column seed cleanup by the seed's stable UUID", async () => { | ||
| const record = makeRecord({ | ||
| agent_id: "single-column-worker", | ||
| surface_id: "surface:single-column-worker", | ||
| surface_uuid: "11111111-2222-4333-8444-555555555555", | ||
| workspace_id: "ws:placement", | ||
| state: "idle", | ||
| role: "worker", | ||
| surface_provenance: "cmuxlayer_spawn", | ||
| }); | ||
| const leadRef = "surface:lead"; | ||
| const leadUuid = "aaaaaaaa-bbbb-4ccc-8ddd-eeeeeeeeeeee"; | ||
| const seedRef = "surface:worker-column-seed"; | ||
| const seedUuid = "33333333-4444-4555-8666-777777777777"; | ||
| let workerPane: "pane:left" | "pane:right" = "pane:left"; | ||
| let seedOpen = false; | ||
| const refreshLiveSurfaces = () => { | ||
| liveSurfaces = [ | ||
| { | ||
| ...makeSurface(leadRef), | ||
| id: leadUuid, | ||
| workspace_ref: "ws:placement", | ||
| }, | ||
| { | ||
| ...makeSurface(record.surface_id), | ||
| id: record.surface_uuid ?? undefined, | ||
| workspace_ref: "ws:placement", | ||
| }, | ||
| ...(seedOpen | ||
| ? [ | ||
| { | ||
| ...makeSurface(seedRef), | ||
| id: seedUuid, | ||
| workspace_ref: "ws:placement", | ||
| }, | ||
| ] | ||
| : []), | ||
| ]; | ||
| }; | ||
| refreshLiveSurfaces(); | ||
| stateMgr.writeState(record); | ||
| engine.getRegistry().set(record.agent_id, record); | ||
| (mockClient.listPanes as ReturnType<typeof vi.fn>).mockImplementation( | ||
| async () => { | ||
| const leftRefs = [ | ||
| leadRef, | ||
| ...(workerPane === "pane:left" ? [record.surface_id] : []), | ||
| ]; | ||
| const leftIds = [ | ||
| leadUuid, | ||
| ...(workerPane === "pane:left" ? [record.surface_uuid] : []), | ||
| ]; | ||
| const rightRefs = [ | ||
| ...(seedOpen ? [seedRef] : []), | ||
| ...(workerPane === "pane:right" ? [record.surface_id] : []), | ||
| ]; | ||
| const rightIds = [ | ||
| ...(seedOpen ? [seedUuid] : []), | ||
| ...(workerPane === "pane:right" ? [record.surface_uuid] : []), | ||
| ]; | ||
| return { | ||
| workspace_ref: "ws:placement", | ||
| window_ref: "window:placement", | ||
| panes: [ | ||
| { | ||
| ref: "pane:left", | ||
| index: 0, | ||
| focused: true, | ||
| surface_count: leftRefs.length, | ||
| surface_refs: leftRefs, | ||
| surface_ids: leftIds, | ||
| pixel_frame: leftFrame, | ||
| }, | ||
| ...(rightRefs.length > 0 | ||
| ? [ | ||
| { | ||
| ref: "pane:right", | ||
| index: 1, | ||
| focused: false, | ||
| surface_count: rightRefs.length, | ||
| surface_refs: rightRefs, | ||
| surface_ids: rightIds, | ||
| pixel_frame: rightFrame, | ||
| }, | ||
| ] | ||
| : []), | ||
| ], | ||
| }; | ||
| }, | ||
| ); | ||
| ( | ||
| mockClient.listPaneSurfaces as ReturnType<typeof vi.fn> | ||
| ).mockImplementation(async ({ pane }: { pane?: string }) => ({ | ||
| workspace_ref: "ws:placement", | ||
| window_ref: "window:placement", | ||
| pane_ref: pane, | ||
| surfaces: | ||
| pane === "pane:right" | ||
| ? [ | ||
| ...(seedOpen | ||
| ? [ | ||
| { | ||
| ...makeSurface(seedRef), | ||
| id: seedUuid, | ||
| workspace_ref: "ws:placement", | ||
| }, | ||
| ] | ||
| : []), | ||
| ...(workerPane === "pane:right" | ||
| ? [ | ||
| { | ||
| ...makeSurface(record.surface_id), | ||
| id: record.surface_uuid ?? undefined, | ||
| workspace_ref: "ws:placement", | ||
| }, | ||
| ] | ||
| : []), | ||
| ] | ||
| : [ | ||
| { | ||
| ...makeSurface(leadRef), | ||
| id: leadUuid, | ||
| workspace_ref: "ws:placement", | ||
| }, | ||
| ...(workerPane === "pane:left" | ||
| ? [ | ||
| { | ||
| ...makeSurface(record.surface_id), | ||
| id: record.surface_uuid ?? undefined, | ||
| workspace_ref: "ws:placement", | ||
| }, | ||
| ] | ||
| : []), | ||
| ], | ||
| })); | ||
| (mockClient.newSplit as ReturnType<typeof vi.fn>).mockImplementationOnce( | ||
| async (_direction, opts) => { | ||
| await opts.beforeMutation?.(); | ||
| seedOpen = true; | ||
| refreshLiveSurfaces(); | ||
| return { | ||
| workspace: "ws:placement", | ||
| surface: seedRef, | ||
| surface_id: seedUuid, | ||
| pane: "pane:right", | ||
| title: "", | ||
| type: "terminal", | ||
| }; | ||
| }, | ||
| ); | ||
| (mockClient.moveSurface as ReturnType<typeof vi.fn>).mockImplementationOnce( | ||
| async (opts) => { | ||
| await opts.beforeMutation?.(); | ||
| workerPane = "pane:right"; | ||
| return { | ||
| ok: true, | ||
| workspace: "ws:placement", | ||
| surface: opts.surface, | ||
| pane: "pane:right", | ||
| }; | ||
| }, | ||
| ); | ||
| (mockClient.closeSurface as ReturnType<typeof vi.fn>).mockImplementationOnce( | ||
| async (_surface, opts) => { | ||
| await opts.beforeMutation?.(); | ||
| seedOpen = false; | ||
| refreshLiveSurfaces(); | ||
| }, | ||
| ); | ||
|
|
||
| const summary = await engine.reconcileRolePlacements("idle"); | ||
|
|
||
| expect(summary.moved).toHaveLength(1); | ||
| expect(mockClient.closeSurface).toHaveBeenCalledWith( | ||
| seedRef, | ||
| expect.objectContaining({ | ||
| workspace: "ws:placement", | ||
| stableSurfaceIdentity: seedUuid, | ||
| beforeMutation: expect.any(Function), | ||
| }), | ||
| ); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Consider adding a regression test for seed-cleanup failure after a successful move.
This test only exercises the happy path (seed create → move → seed close all succeed). It would be worth adding a case where closeSurface for the seed rejects after moveSurface already succeeded, asserting summary.moved still contains the agent and summary.skipped does not gain a duplicate entry for it — this is exactly the scenario exposed by the unsafe-finally issue flagged in src/agent-engine.ts (3838-3895).
🤖 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 `@tests/agent-engine.test.ts` around lines 5599 - 5780, Add a regression test
alongside the existing reconcileRolePlacements test that makes seed closeSurface
reject after moveSurface succeeds. Assert the returned summary keeps the agent
in summary.moved and does not add a duplicate entry to summary.skipped, covering
the cleanup failure path without changing the successful move behavior.
1 participant
Summary
Contract
Boot order is: reconstitute registry -> ingest/re-register existing panes -> sweep provable idle leftovers -> placement live.
Reconciliation runs at spawn and idle only. It never yanks a working pane. Unknown provenance is operator-owned and is never moved or closed.
TDD evidence
Counterfactual RED coverage includes:
Verification
CodeRabbit rerun was unavailable due the free OSS rate limit; the required manual red/blue fallback completed cleanly.
Note
High Risk
Large changes to cmux topology mutations, boot gating, and automated surface moves; mistakes could relocate operator panes or block daemon clients until boot succeeds.
Overview
Introduces a two-column role contract (orchestrators column 0, workers column 1) backed by
surface_provenance: onlycmuxlayer_spawnsurfaces are auto-moved; discovered/operator panes stayunknownand untouched.reconcileRolePlacementsruns at spawn, when agents become idle, on boot, and each sweep. It uses stable UUID topology, observer epochs, andbeforeMutationchecks so working agents, recycled refs, and idle→working races never get yanked. Worker column seeding can split beside the lead pane and then close the seed by UUID.Spawn placement is simplified to dock at the top of the canonical role column (
topPaneInRoleColumn/deriveRoleColumnIndex), ignoring zero-area phantom panes instead of fill-based heuristics.Boot is stricter: registry reconstitute → forced discovery merge → boot reconcile → sidebar; discovery failure rejects init. The daemon holds new sockets until lifecycle startup succeeds and records
lifecycleStartErrorwhen it does not.Surface I/O locks and mutations key off stable UUIDs;
moveSurfaceis wired through server, app-server runtime, and reflow.markAgentWorkingruns only after successful Return on submit paths, not on staging-only sends.Reviewed by Cursor Bugbot for commit 8e80644. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Enforce provenance-gated role placement for spawned agent terminals
surface_provenanceonAgentRecord('cmuxlayer_spawn'|'unknown') so the engine can distinguish programmatically created terminals from auto-discovered ones.AgentEngine.reconcileRolePlacementswhich enforces a two-column layout: orchestrators dock into canonical column 0, workers into column 1, using stable UUID-based write locks and pre-mutation guards to avoid stale-ref races.unknown-provenance) agents are never moved.chooseAgentSpawnPlacementin layout-policy.ts to usetopPaneInRoleColumnand canonical column indices, removing majority/heuristic predicates; zero-area phantom panes are filtered from column derivation.lifecycleStartPromise; failures are recorded inlifecycleStartErrorand surfaced to callers rather than silently proceeding.markAgentWorkingtransitions an idle agent to working immediately when a command is dispatched (Enter key sent), reserving its placement slot.initializenow rejects on first-connect discovery failure instead of degrading to an'unknown'placement state, which is a behavioral change for callers that previously handled the degraded state.📊 Macroscope summarized 8e80644. 8 files reviewed, 0 issues evaluated, 0 issues filtered, 0 comments posted
🗂️ Filtered Issues
No issues evaluated.
Summary by CodeRabbit
New Features
Bug Fixes