Fix stale surface refs and poisoned agent identity - #352
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_5ecaf468-6907-4b1a-944f-fd7e22ad3ce7) |
📝 WalkthroughWalkthroughThe server now captures stable surface UUIDs and observer epochs, validates topology before mutations, and uses identity-scoped delivery locks. Agent listing reports skipped invalid records. Resume routing tolerates malformed legacy metadata, and registry synchronization preserves repository ownership. ChangesSurface identity and agent resilience
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![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: 7db012d4a8
ℹ️ 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".
| if (!topology.workspaceBySurface.has(requestedSurface)) { | ||
| return { | ||
| surface: requestedSurface, | ||
| workspace: requestedWorkspace, | ||
| stableSurfaceIdentity: null, |
There was a problem hiding this comment.
When a complete legacy/ref-only topology does not contain requestedSurface, this branch returns a permissive route instead of failing closed. If that ref is recycled after the snapshot but before assertSurfaceMutationAllowed or the write, send_input, send_key, or even close_surface can mutate the new occupant because this route has no stable identity or effective current-binding assertion. Reject the absent ref just as the UUID path rejects an absent UUID.
AGENTS.md reference: AGENTS.md:L20-L25
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5432932. Complete ref-only topology now rejects an absent requested ref, and regression coverage proves no write occurs.
| surface.id ? [[surface.ref, surface.id] as const] : [], | ||
| ), | ||
| ); | ||
| captureSurfaceIdentities(stableIdByRef); |
There was a problem hiding this comment.
In list_surfaces, the pane and surface snapshots are collected across multiple awaits, but their observer epoch is never captured and revalidated before this call. If the cmux backend/socket is replaced after enumeration and before capture, captureSurfaceIdentities associates the old backend's UUIDs with the new epoch; subsequent raw mutations then resolve against those stale UUIDs and remain refused until another reconnect or server restart. Capture the epoch before enumeration and discard the identities if it changes.
AGENTS.md reference: AGENTS.md:L38-L39
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5432932. list_surfaces captures the observer epoch before enumeration and discards the identity snapshot if the epoch changes before capture; the regression then proves a later raw send binds only to the current observer.
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_73d4beac-fa62-4a89-907e-3c5fca449eff) |
|
Review round 1 addressed at exact head
Verification: focused/widened 396/396; full 2,387/2,387 (twice, including pre-push); build; pre-PR 63/63; live branch MCP probe 3 workspaces, 27/27 surfaces UUID-backed. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 54329321b5
ℹ️ 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 workspace = | ||
| topology.workspaceBySurface.get(currentRef) ?? requestedWorkspace; |
There was a problem hiding this comment.
When a caller supplies workspace, this resolver still enumerates every workspace and replaces the requested value with the workspace where the UUID currently appears. If the surface moved, or the caller accidentally combines a surface/UUID from workspace B with workspace A, send_input, send_command, send_key, rename_tab, or close_surface now silently mutates workspace B instead of failing as the previous workspace-scoped client call would. Constrain topology collection to requestedWorkspace or reject a binding found outside it.
AGENTS.md reference: AGENTS.md:L23-L25
Useful? React with 👍 / 👎.
| recordSurfaceWriteSuccess( | ||
| record.surface, | ||
| record.stableSurfaceIdentity, | ||
| context.surfaceObserverId, |
There was a problem hiding this comment.
For a ref-only background delivery, finishDelivery reads context.surfaceObserverId only after the asynchronous write has completed. If the backend/socket owner changes after the write attempt but before completion is recorded, the old backend's success or broken-pipe failure is attributed to the new observer's same mutable ref, which can clear or poison its liveness state and affect crash detection. Store the observer identity when the delivery starts, as withSurfaceWrite already does for foreground writes.
AGENTS.md reference: AGENTS.md:L38-L39
Useful? React with 👍 / 👎.
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_d15bf8e1-7b0d-4302-a11f-5096992d3b67) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 99ab695c5d
ℹ️ 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".
| workspace, | ||
| stableSurfaceIdentity: null, | ||
| assertCurrent: async () => { | ||
| const current = await collectSurfaceTopology(); |
There was a problem hiding this comment.
When a complete topology exposes no UUIDs, assertCurrent accepts any later complete ref-only snapshot containing the same ref, without checking that the observer epoch still matches the epoch captured during route resolution. If the socket/backend is replaced between resolution and a foreground or background mutation and the new backend also has requestedSurface, the write is authorized against that new occupant. Capture the resolution epoch in this route and reject when it changes before mutation.
AGENTS.md reference: AGENTS.md:L38-L39
Useful? React with 👍 / 👎.
| const screen = await client.readScreen(route.surface, { | ||
| workspace: route.workspace, |
There was a problem hiding this comment.
With force:false and a live backing agent, the stable route is checked before this read but not immediately before it or before stateMgr.transition below. If the UUID moves and its old ref is recycled during that window, this reads the replacement pane; a done signal there can mark the original agent done, after which the final pre-close binding check merely refuses the close while leaving the registry corrupted. Revalidate the route around the safety read and before applying consolidation.
AGENTS.md reference: AGENTS.md:L11-L16
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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/server.ts (1)
5019-5031: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAlign delivery tracking keys with the UUID lock key.
acquireSurfaceWrite(record.lockKey, record.delivery_id)stores/write-locks byuuid:<id>for stable surfaces, butlatestDeliveryBySurfaceandactiveDeliveryBySurfacestill userecord.surface.For stable surface deliveries,
getSurfaceWriteConflict(lockKey)can return the genericsurface uuid:<id> is busymessage instead ofdelivery <id> is still in progress, and the entry is not cleaned fromactiveDeliveryBySurfacewhenfinishDeliveryruns.Use one key namespace for the write lock and delivery tracking maps, or index
activeDeliveryBySurfacebyrecord.lockKeyas well.🤖 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 5019 - 5031, The startBackgroundDelivery tracking maps use record.surface while acquireSurfaceWrite uses record.lockKey, causing stable-surface lookups and cleanup to diverge. Update latestDeliveryBySurface, activeDeliveryBySurface, and the corresponding finishDelivery/prune logic to consistently use record.lockKey (or otherwise maintain equivalent lock-key indexing), preserving delivery-specific conflict messages and cleanup for stable UUID surfaces.
🤖 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/format.ts`:
- Around line 195-201: Normalize each skipped agent ID in the skippedAgents
warning within the formatting flow before joining them: remove control
characters and apply the same truncation policy used for healthy agent IDs,
reusing the existing normalization helper or logic. Keep the warning count and
rendering behavior unchanged while ensuring malformed IDs cannot corrupt the
response.
In `@src/server.ts`:
- Around line 10445-10454: In the catch block of evaluateServerAgentHealth, stop
returning the raw error message in skipped.error; log the full error server-side
using the existing server logging mechanism, then return a bounded, client-safe
reason instead. Preserve the agent_id and ensure non-Error values are normalized
for diagnostic logging without exposing filesystem paths or persisted record
contents in the response.
- Around line 5222-5223: Reduce repeated topology scans in the submit-verify
polling path around collectSurfaceTopology, resolveRawSurfaceMutationRoute,
assertCurrent, and beforeMutation by introducing a short-lived topology cache or
equivalent reuse mechanism. When an explicit workspace is available, limit
unresolved topology scans to that workspace, while preserving mutation
validation and the existing poll timeout behavior.
- Around line 7585-7590: Extract the duplicated route identity comparison into
one local backsResolvedRoute predicate after route resolution, using the
existing stableSurfaceIdentity/surface_uuid and surface_id fallback logic.
Replace both record-matching predicates, including the sites for
remainingLiveAgent and the pre-consolidation check, with calls to this predicate
and remove the redundant outer parentheses while preserving the
TERMINAL_AGENT_STATES filter.
In `@tests/server-agent-tools.test.ts`:
- Around line 6812-6852: Update the recycled-reference error assertions in
tests/server-agent-tools.test.ts (lines 6812-6852) and tests/server.test.ts
(lines 10603-10605) to match the reachable multiple-UUID refusal message,
including “Mutable surface ref”, the affected ref, and “observed for multiple
stable UUIDs; refusing”. Replace the existing “stable surface UUID|stale” and
“stable surface UUID” patterns; no production-code change is needed.
---
Outside diff comments:
In `@src/server.ts`:
- Around line 5019-5031: The startBackgroundDelivery tracking maps use
record.surface while acquireSurfaceWrite uses record.lockKey, causing
stable-surface lookups and cleanup to diverge. Update latestDeliveryBySurface,
activeDeliveryBySurface, and the corresponding finishDelivery/prune logic to
consistently use record.lockKey (or otherwise maintain equivalent lock-key
indexing), preserving delivery-specific conflict messages and cleanup for stable
UUID surfaces.
🪄 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 Plus
Run ID: 4f143d62-e97a-4924-a6ed-944f1750341b
📒 Files selected for processing (7)
src/agent-engine.tssrc/agent-registry.tssrc/format.tssrc/server.tstests/painpoint-e2e.test.tstests/server-agent-tools.test.tstests/server.test.ts
💤 Files with no reviewable changes (1)
- src/agent-registry.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
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/painpoint-e2e.test.tstests/server.test.tstests/server-agent-tools.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/agent-engine.tssrc/format.tssrc/server.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/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/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 (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/painpoint-e2e.test.tstests/server.test.tstests/server-agent-tools.test.ts
🔇 Additional comments (28)
src/format.ts (1)
162-184: LGTM!src/agent-engine.ts (1)
5045-5058: 🗄️ Data Integrity & IntegrationKeep
resumableconsistent with the omittedresume_command.When
buildResumeCommandthrows, this route reportsresumable: truebut excludesresume_command, so consumers should handle resume identity separately from an executable resume command.src/server.ts (14)
5164-5174: The linear scan allocates an entries array per call, but the surroundingcollectSurfaceTopology()enumeration dominates the cost. No action needed.
531-536: LGTM!Also applies to: 916-918
2220-2224: LGTM!Also applies to: 2347-2349, 2396-2398
3245-3256: LGTM!Also applies to: 3267-3267
3566-3589: LGTM!
3809-3809: LGTM!Also applies to: 3893-3894, 4531-4531, 4652-4652
5131-5162: LGTM!
5251-5346: LGTM!
5730-5730: LGTM!Also applies to: 5792-5804, 5837-5839
6722-6746: LGTM!Also applies to: 6833-6881, 6901-6926, 7033-7096, 7170-7193, 7387-7422
7537-7562: LGTM!Also applies to: 7633-7645, 7657-7678, 7691-7703, 7718-7722, 7751-7751, 7760-7792, 7813-7813, 7822-7822
8507-8512: LGTM!Also applies to: 8878-8878
5225-5249: 🗄️ Data Integrity & IntegrationSurface ref collisions across workspaces are already rejected by
collectSurfaceTopology.
collectSurfaceTopologybuildsworkspaceBySurfacewithout filtering workspaces and passes the pairs throughvalidateSurfaceIdentityBijection, which marks shared surface refs in one snapshot as non-bijective. The snapshot then becomes incomplete andresolveRawSurfaceMutationRouterefuses the ref-only path, so duplicate refs inside one topology snapshot cannot cause a wrong UUID/workspace binding.> Likely an incorrect or invalid review comment.
10463-10477: 🎯 Functional CorrectnessNo changes needed.
formatListAgentsdeclares the skipped-Agent third argument with the matching default shape, this call site matches that signature, andcountis only used to display the number of rendered healthy/filtered agent rows.tests/server.test.ts (4)
2602-2627: The exacttoHaveBeenCalledTimes(5)assertion is brittle against future topology caching, but it usefully pins the per-send transport call budget. No change needed.Also applies to: 2751-2774
1390-1390: LGTM!Also applies to: 1403-1403
3978-4001: LGTM!Also applies to: 4461-4484, 4612-4635
10464-10474: LGTM!Also applies to: 10514-10514, 10563-10573, 10634-10639
tests/server-agent-tools.test.ts (7)
768-780: LGTM!Also applies to: 832-832, 849-853, 866-871, 881-881
4313-4365: LGTM!Also applies to: 4372-4383
4385-4436: LGTM!
6490-6621: LGTM!
6623-6763: LGTM!
6765-6810: LGTM!Also applies to: 6854-6932
4366-4370: 🎯 Functional CorrectnessNo duplicated properties are present here.
The type annotation declares
agentsonce andskipped_agentsonce, so there is noTS2300duplicate identifier error to address.> Likely an incorrect or invalid review comment.tests/painpoint-e2e.test.ts (1)
777-803: LGTM!
| if (skippedAgents.length > 0) { | ||
| const skippedIds = skippedAgents.map((agent) => agent.agent_id).join(", "); | ||
| lines.push( | ||
| `\u2502 \u26a0 skipped ${skippedAgents.length} invalid agent row${skippedAgents.length === 1 ? "" : "s"}: ${skippedIds}`, | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Normalize skipped agent IDs before rendering the warning.
This code inserts agent_id values directly into the text response. A malformed row with a newline, control character, or very long ID can corrupt the response. Apply the same truncation policy used for healthy IDs and remove control characters before joining the values.
Proposed fix
- const skippedIds = skippedAgents.map((agent) => agent.agent_id).join(", ");
+ const skippedIds = skippedAgents
+ .map(({ agent_id }) =>
+ truncate(agent_id.replace(/[\u0000-\u001f\u007f]/g, " "), 18),
+ )
+ .join(", ");📝 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.
| if (skippedAgents.length > 0) { | |
| const skippedIds = skippedAgents.map((agent) => agent.agent_id).join(", "); | |
| lines.push( | |
| `\u2502 \u26a0 skipped ${skippedAgents.length} invalid agent row${skippedAgents.length === 1 ? "" : "s"}: ${skippedIds}`, | |
| ); | |
| } | |
| if (skippedAgents.length > 0) { | |
| const skippedIds = skippedAgents | |
| .map(({ agent_id }) => | |
| truncate(agent_id.replace(/[\u0000-\u001f\u007f]/g, " "), 18), | |
| ) | |
| .join(", "); | |
| lines.push( | |
| `\u2502 \u26a0 skipped ${skippedAgents.length} invalid agent row${skippedAgents.length === 1 ? "" : "s"}: ${skippedIds}`, | |
| ); | |
| } |
🤖 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/format.ts` around lines 195 - 201, Normalize each skipped agent ID in the
skippedAgents warning within the formatting flow before joining them: remove
control characters and apply the same truncation policy used for healthy agent
IDs, reusing the existing normalization helper or logic. Keep the warning count
and rendering behavior unchanged while ensuring malformed IDs cannot corrupt the
response.
| } catch (error) { | ||
| return { | ||
| agent: null, | ||
| skipped: { | ||
| agent_id: agent.agent_id, | ||
| error: | ||
| error instanceof Error ? error.message : String(error), | ||
| }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Bound the propagated error text before returning it to the client.
Line 10451 copies the raw upstream error message into the skipped_agents response. evaluateServerAgentHealth reads record fields and filesystem paths, including cli_session_path, launch_cwd, goal_file, and inbox paths under homedir(). An error thrown from any of those paths therefore embeds absolute filesystem paths or persisted record values in the tool response.
Return a bounded reason to the caller, and log the full message server-side for diagnostics.
🛡️ Proposed change
} catch (error) {
+ const detail =
+ error instanceof Error ? error.message : String(error);
+ console.error(
+ `[cmuxlayer] list_agents skipped ${agent.agent_id}:`,
+ detail,
+ );
return {
agent: null,
skipped: {
agent_id: agent.agent_id,
- error:
- error instanceof Error ? error.message : String(error),
+ error: detail.slice(0, 200),
},
};
}📝 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.
| } catch (error) { | |
| return { | |
| agent: null, | |
| skipped: { | |
| agent_id: agent.agent_id, | |
| error: | |
| error instanceof Error ? error.message : String(error), | |
| }, | |
| }; | |
| } | |
| } catch (error) { | |
| const detail = | |
| error instanceof Error ? error.message : String(error); | |
| console.error( | |
| `[cmuxlayer] list_agents skipped ${agent.agent_id}:`, | |
| detail, | |
| ); | |
| return { | |
| agent: null, | |
| skipped: { | |
| agent_id: agent.agent_id, | |
| error: detail.slice(0, 200), | |
| }, | |
| }; | |
| } |
🤖 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 10445 - 10454, In the catch block of
evaluateServerAgentHealth, stop returning the raw error message in
skipped.error; log the full error server-side using the existing server logging
mechanism, then return a bounded, client-safe reason instead. Preserve the
agent_id and ensure non-Error values are normalized for diagnostic logging
without exposing filesystem paths or persisted record contents in the response.
99ab695 to
9ba5a36
Compare
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_f2b408e3-d892-479f-8806-d1f5b55f4bfd) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9ba5a36ded
ℹ️ 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 targetRecord = resolveLatestSurfaceAgentRecord( | ||
| stateMgr, | ||
| args.surface, | ||
| route.surface, | ||
| ); |
There was a problem hiding this comment.
When resolveRawSurfaceMutationRoute follows a captured UUID from an old ref to a new one, the tracked record can still retain the old surface_id, so this ref-only lookup returns no target. For send_input with press_enter:true, that disables submit verification and can report delivery as successful even when the interactive composer never submitted; it also bypasses the multi-paragraph composer guard. The identical lookup in send_command has the same verification problem, so both should resolve the record using route.stableSurfaceIdentity when available.
AGENTS.md reference: AGENTS.md:L11-L16
Useful? React with 👍 / 👎.
| ? record.surface_uuid.toLowerCase() === requestedSurfaceKey || | ||
| record.surface_uuid.toLowerCase() === observedSurfaceUuid || | ||
| (observedSurfaceUuid === undefined && | ||
| record.surface_id === args.surface) | ||
| record.surface_id === route.surface) |
There was a problem hiding this comment.
When the close route has a stable UUID but findSurfaceByRef returns a surface without id—for example, a connector exposes UUIDs only through pane surface_ids—this fallback marks every record sharing the current mutable ref as user_killed, even records whose nonmatching UUID proves they belong to another agent. In a crash/resume collision this poisons the unrelated agent after the intended surface is closed; a stable route should match records only by route.stableSurfaceIdentity, reserving the ref fallback for genuinely ref-only routes.
AGENTS.md reference: AGENTS.md:L11-L16
Useful? React with 👍 / 👎.
| paneSurfaces, | ||
| workerSurfaceIds, | ||
| args.surface, | ||
| route.surface, |
There was a problem hiding this comment.
If a captured worker UUID has moved from its persisted surface_id to route.surface, workerSurfaceIds still contains only the old registry ref, while this call evaluates the new ref. chooseSurfaceClosePolicy therefore no longer recognizes the target as a worker and leaves its dedicated pane uncollapsed when its final tab closes. Build the worker set from the same UUID-aware topology used for the close route, as the lifecycle engine's close-policy path already does.
AGENTS.md reference: AGENTS.md:L20-L25
Useful? React with 👍 / 👎.
1 participant
Summary
list_agentsand keep healthy rows availableRegression coverage
send_to(agent_id)works withbrainlayerClaude [surface:199]list_agentsVerification
bun run test -- --maxWorkers=4— 106 files, 2,382 tests passedbun run buildbun run pre-pr— 63 tests passedScope
Note
High Risk
Changes core terminal I/O and close paths for all mutating tools; incorrect UUID binding could block legitimate operations or, if mis-implemented, still target wrong surfaces.
Overview
Terminal mutations (
send_input,send_command,send_key,move_surface,rename_tab,close_surface) now go throughresolveRawSurfaceMutationRoute, which captures first-seen ref→UUID mappings duringlist_surfacesand re-resolves the live ref before I/O. Recycled or ambiguous refs, missing UUIDs, and workspace mismatches fail closed instead of hitting a wrong pane; write locks and delivery attribution can key off stable UUIDs.Agent identity: auto-discovery sync no longer overwrites
repofrom tab titles (fixes poisoned names likebrainlayerClaude [surface:199]).list_agentsevaluates health per row and returnsskipped_agentsfor bad rows while keeping healthy ones; CLI output warns about skipped IDs.resolveAgentRoutetolerates corrupt resume metadata without breaking send.close_surface: live-agent checks and post-close state updates prefer stable UUID when available; recycled refs are refused rather than closing a new occupant.Reviewed by Cursor Bugbot for commit 9ba5a36. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Fix stale surface refs and poisoned agent identity by binding mutations to stable UUIDs
resolveRawSurfaceMutationRoutein server.ts that binds surface refs to stable UUIDs observed duringlist_surfaces, then asserts the binding is still valid before any mutation (send, move, rename, close, etc.).syncAutoRecordin agent-registry.ts to stop inferring and overwritingrepofromsurface_title, preventing identity corruption when titles contain surface suffixes.resolveAgentRoutein agent-engine.ts to swallow errors frombuildResumeCommandrather than throwing, returning a route withoutresume_commandinstead.list_agentsin server.ts now isolates per-agent health evaluation; failed agents are excluded from the count and reported in askipped_agentsfield rather than failing the entire response.send_input,send_command,send_key,move_surface,rename_tab,close_surface) now require a priorlist_surfacescall to establish UUID bindings; without it, operations on numeric refs may be refused when topology is complete enough to detect ambiguity.📊 Macroscope summarized 9ba5a36. 4 files reviewed, 0 issues evaluated, 0 issues filtered, 0 comments posted
🗂️ Filtered Issues
No issues evaluated.
Summary by CodeRabbit
New Features
Bug Fixes