Skip to content

fix(recovery): make user-closed panes terminal#323

Merged
EtanHey merged 6 commits into
mainfrom
wt/cmuxlayer-worker-93vbw6
Jul 14, 2026
Merged

fix(recovery): make user-closed panes terminal#323
EtanHey merged 6 commits into
mainfrom
wt/cmuxlayer-worker-93vbw6

Conversation

@EtanHey

@EtanHey EtanHey commented Jul 14, 2026

Copy link
Copy Markdown
Owner

Summary

  • ingest cmux close-forensics before absence reconciliation, so a UI tab_close sets user_killed before crash recovery is evaluated
  • return attributed close events from the forensics sweep while preserving its best-effort failure contract
  • make successful close_surface calls terminal for matching managed agents
  • preserve genuine PTY-death recovery and the existing respawn ceiling

Root cause and reproduction

The lifecycle sweep reconciled/evicted missing surfaces and ran crash recovery before the asynchronous close-forensics tail. A user-closing a managed tab therefore looked identical to a disappeared PTY: crash_recover=true, a captured CLI session, and no user_killed marker.

The live event stream reproduced the race twice on 2026-07-14: a surface.closed event with origin=tab_close at 16:23:48.852Z was followed by a replacement surface and a crash-recovery respawn; closing the replacement at 16:24:22.664Z produced the same sequence again.

RED / counterfactual evidence

Before the fix, the two new regressions failed:

  • UI tab_close: newSplit was called once, resurrecting the agent
  • forced close_surface: user_killed remained false

After implementation, temporarily disabling only the two new terminal-intent writes made both tests fail again with user_killed=false. Restoring them returned the paired suite to green, including the existing genuine PTY-death respawn test.

Verification

  • env -u CMUX_SOCKET_PATH -u CMUX_DAEMON_SOCKET bunx vitest run tests/agent-engine.test.ts tests/close-forensics.test.ts tests/server.test.ts: 460/460 passed
  • focused paired regressions: UI close suppressed, forced close terminal, genuine PTY death still respawns (3/3 passed)
  • env -u CMUX_SOCKET_PATH -u CMUX_DAEMON_SOCKET bun run test: 104/104 files, 2,129/2,129 tests passed
  • env -u CMUX_SOCKET_PATH -u CMUX_DAEMON_SOCKET bun run typecheck: passed
  • env -u CMUX_SOCKET_PATH -u CMUX_DAEMON_SOCKET bun run build: passed
  • real-cmux read-only contract using freshly built dist + isolated daemon: ping, orphan denial, list/read, doctor, and graceful retire/autostart all passed
  • git diff --check: passed

Pre-commit CodeRabbit CLI review was attempted with the required three-minute bound; the service produced no findings before timeout. Manual diff review found no blocker.


Note

Medium Risk
Changes core lifecycle sweep ordering and crash-recovery eligibility via user_killed; incorrect surface matching could suppress legitimate recovery, though tests cover recycled refs and UUID vs ref fallbacks.

Overview
Fixes a sweep ordering race where missing surfaces were reconciled and crash recovery ran before cmux close-forensics could record operator intent, so UI tab closes looked like PTY deaths and agents with crash_recover=true were respawned.

Close forensics now runs at the start of each sweep (awaited, not fire-and-forget at the tail). The sweep returns attributed CloseForensicsEvent[] in addition to the emit count. New markIntentionalSurfaceCloses sets user_killed=true on registry agents whose surface_uuid matches forensics with origin tab_close or workspace_teardown, before reconcile/evict and recoverCrashedAgents.

close_surface marks matching managed agents terminal immediately after a successful close, using stable UUID when cmux exposes it and falling back to ref when observation is degraded—without marking stale UUID owners when a ref was recycled.

Tests cover UI close vs respawn, forced close terminal intent, recycled-ref safety, and concurrent eviction handling.

Reviewed by Cursor Bugbot for commit 07a5ce0. Bugbot is set up for automated code reviews on this repo. Configure here.

Note

Mark user-closed panes as terminal before crash recovery in the agent engine

  • Agents whose surfaces are closed via the cmux UI (tab close or workspace teardown) are now marked user_killed=true before reconciliation, preventing them from being treated as crash candidates.
  • Close forensics sweep is now awaited at the start of each sweep cycle in AgentEngine.runSweepOnce, replacing the previous non-awaited tail call.
  • A new markIntentionalSurfaceCloses method filters forensics events by origin and updates the StateManager and lifecycle registry for matching agents.
  • The close_surface tool handler in server.ts also marks matching agents user_killed=true on surface close, using stable UUID matching with a ref fallback.
  • runCloseForensicsSweep in close-forensics.ts now returns the full CloseForensicsEvent[] array alongside the emitted count so callers can act on individual events.
  • Behavioral Change: close forensics now blocks each sweep cycle rather than running in the background, adding latency proportional to forensics sweep duration.

Macroscope summarized 07a5ce0.

Summary by CodeRabbit

  • Bug Fixes

    • Closing a surface now correctly marks associated agents as user-terminated.
    • Agents whose tabs are closed are no longer incorrectly treated as crashed or automatically restarted.
    • Surface closure handling now works when matching by either surface identifier.
  • Tests

    • Added coverage for crash recovery and forced surface closure scenarios.

@EtanHey

EtanHey commented Jul 14, 2026

Copy link
Copy Markdown
Owner Author

@coderabbitai review

@EtanHey

EtanHey commented Jul 14, 2026

Copy link
Copy Markdown
Owner Author

@codex review

@EtanHey

EtanHey commented Jul 14, 2026

Copy link
Copy Markdown
Owner Author

@cursor @BugBot review

@cursor

cursor Bot commented Jul 14, 2026

Copy link
Copy Markdown

Bugbot couldn't run - usage limit reached

Bugbot 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_5060debd-7c9b-4a94-9867-775f14553adb)

@cursor

cursor Bot commented Jul 14, 2026

Copy link
Copy Markdown

You need to increase your spend limit or enable usage-based billing to run background agents. Go to Cursor

@coderabbitai

coderabbitai Bot commented Jul 14, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai

coderabbitai Bot commented Jul 14, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@EtanHey, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 15 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: b37d5afc-6d62-41fa-acd2-7a13a444d975

📥 Commits

Reviewing files that changed from the base of the PR and between 55b6777 and 07a5ce0.

📒 Files selected for processing (4)
  • src/agent-engine.ts
  • src/server.ts
  • tests/agent-engine.test.ts
  • tests/server.test.ts
📝 Walkthrough

Walkthrough

Close-forensics sweeps now return emitted events and are awaited before AgentEngine reconciliation, allowing matching agents to become terminal user_killed records. The close_surface tool also updates matching agent and lifecycle registry records directly.

Changes

Surface close attribution

Layer / File(s) Summary
Expose close-forensics events
src/close-forensics.ts
Close-forensics results now include emitted events, with empty event arrays for early exits and failures.
Ingest closes before recovery
src/agent-engine.ts, tests/agent-engine.test.ts
AgentEngine awaits forensics at sweep start, marks matching agents as user_killed, and tests that closed tabs are not respawned during crash recovery.
Record direct surface closures
src/server.ts, tests/server.test.ts
close_surface marks agents matching either surface identifier as user_killed and preserves crash-recovery state.
Estimated code review effort: 4 (Complex) ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant AgentEngine
  participant closeForensicsRunner
  participant AgentState
  AgentEngine->>closeForensicsRunner: Run close-forensics sweep
  closeForensicsRunner-->>AgentEngine: Return tab_close events
  AgentEngine->>AgentState: Mark matching agents user_killed
  AgentEngine->>AgentState: Reconcile absence and crash recovery
Loading

Possibly related PRs

Poem

A tab closed softly, the bunny took note,
The agent was marked in its terminal coat.
No risky respawn, no frantic new split,
The sweep found the clue and handled it.
Hop, hop—clean state! 🐇

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: user-closed panes are now treated as terminal instead of recoverable.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch wt/cmuxlayer-worker-93vbw6

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/server.ts`:
- Around line 6819-6830: In the close_surface state sweep, wrap the
stateMgr.updateRecord call for each matching record in try/catch. Ignore the
expected “Agent not found” error caused by concurrent lifecycle eviction so
surface closure still succeeds, while rethrowing unexpected errors; keep
lifecycleRegistry.set for successfully updated records.
🪄 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: a8d4adaa-5d80-4477-94bd-b03855b9ae2f

📥 Commits

Reviewing files that changed from the base of the PR and between 05029b9 and 55b6777.

📒 Files selected for processing (5)
  • src/agent-engine.ts
  • src/close-forensics.ts
  • src/server.ts
  • tests/agent-engine.test.ts
  • tests/server.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (6)
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/server.test.ts
  • tests/agent-engine.test.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/**/*.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 the ok(data) and err(error) helpers for consistent MCP tool responses.
All MCP tool handlers must return { content: TextContent[], structuredContent?, isError? }.

Files:

  • src/server.ts
  • src/close-forensics.ts
  • src/agent-engine.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 when skipAgentLifecycle: true.

Files:

  • src/server.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/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
🧠 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/server.test.ts
  • tests/agent-engine.test.ts
🔇 Additional comments (12)
src/close-forensics.ts (4)

392-405: LGTM!


447-447: LGTM!


505-507: LGTM!


688-688: LGTM!

src/agent-engine.ts (5)

47-51: LGTM!


318-326: LGTM!


770-772: LGTM!


3629-3629: LGTM!


3647-3703: LGTM!

tests/agent-engine.test.ts (2)

39-39: LGTM!


3421-3502: LGTM!

tests/server.test.ts (1)

9735-9792: LGTM!

Comment thread src/server.ts

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 55b6777f51

ℹ️ 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".

Comment thread src/server.ts Outdated
Comment on lines +6821 to +6822
record.surface_id !== args.surface &&
record.surface_uuid !== args.surface

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Avoid marking recycled surface refs as user-killed

When close_surface is called on a mutable ref that has been recycled, this condition marks any persisted agent whose stale surface_id equals args.surface even if that agent has a different surface_uuid. The registry code elsewhere treats UUID-bearing records as authoritative specifically to avoid adopting recycled refs; marking the stale record user_killed here suppresses crash recovery for the original agent after its UUID disappears. Only fall back to surface_id matching for UUID-less records, or verify the live UUID before setting this flag.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in aa08dd3. close_surface now resolves the current ref to its observed stable UUID before closing; UUID-backed records match only the requested/observed UUID, while mutable-ref fallback is limited to UUID-less records. The new recycled-ref regression proves a stale UUID owner remains recoverable, and the normal stable-owner close remains terminal.

@cursor

cursor Bot commented Jul 14, 2026

Copy link
Copy Markdown

Bugbot couldn't run - usage limit reached

Bugbot 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_2b83a2e1-d216-4e24-a541-7de7cc01cfdb)

@EtanHey

EtanHey commented Jul 14, 2026

Copy link
Copy Markdown
Owner Author

@coderabbitai review

@EtanHey

EtanHey commented Jul 14, 2026

Copy link
Copy Markdown
Owner Author

@codex review

@coderabbitai

coderabbitai Bot commented Jul 14, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: aa08dd37bb

ℹ️ 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".

Comment thread src/server.ts Outdated
Comment on lines +6826 to +6829
const matchesClosedSurface = record.surface_uuid
? record.surface_uuid.toLowerCase() === requestedSurfaceKey ||
record.surface_uuid.toLowerCase() === observedSurfaceUuid
: record.surface_id === args.surface;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Fall back to the surface ref when UUID lookup is unavailable

When findSurfaceByRef cannot return a UUID (for example on older/ref-only surface enumeration or a transient listing failure), records that already have surface_uuid take this branch and are compared only against args.surface and undefined. In that case a forced close of the exact managed, crash-recoverable surface leaves user_killed false, so the later absence reconciliation can classify it as a recoverable crash and restart a pane the user explicitly closed. Please fall back to record.surface_id === args.surface when no observed UUID is available.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in d0e1109. close_surface now uses the observed stable UUID whenever enumeration succeeds, so a recycled ref cannot terminalize its stale owner; only when enumeration is unavailable/ref-only does an explicit close fall back to the requested mutable ref. The regression covers both the recycled-ref mismatch and the degraded UUID-unavailable path.

@cursor

cursor Bot commented Jul 14, 2026

Copy link
Copy Markdown

Bugbot couldn't run - usage limit reached

Bugbot 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_ccb1e207-de95-4451-8723-9d500fe0abd8)

@EtanHey

EtanHey commented Jul 14, 2026

Copy link
Copy Markdown
Owner Author

@coderabbitai review

@codex review

Final head is d0e1109. Please re-review the closed-pane recovery fix, including stable UUID matching, recycled-ref protection, concurrent state eviction, and the degraded UUID-unavailable fallback.

@coderabbitai

coderabbitai Bot commented Jul 14, 2026

Copy link
Copy Markdown
🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -e
rg -n "surface_uuid|recycled|eviction|user_killed|closeForensics" src/agent-engine.ts | head -80
echo "---close-forensics---"
rg -n "surface_uuid|recycled|eviction|uuid" src/close-forensics.ts | head -80

Length of output: 3422


🏁 Script executed:

#!/bin/bash
sed -n '3640,3710' src/agent-engine.ts
echo "=== uuid fallback ==="
sed -n '4390,4475' src/agent-engine.ts
echo "=== concurrent eviction / user_killed marking ==="
sed -n '4690,4900' src/agent-engine.ts
echo "=== close-forensics.ts full ==="
cat -n src/close-forensics.ts | sed -n '1,200p'

Length of output: 9096


🏁 Script executed:

#!/bin/bash
echo "=== forensics ingest wiring (3640-3710) ==="
sed -n '3640,3710p' src/agent-engine.ts

echo "=== uuid fallback (4390-4475) ==="
sed -n '4390,4475p' src/agent-engine.ts

echo "=== concurrent eviction / user_killed marking (4690-4900) ==="
sed -n '4690,4900p' src/agent-engine.ts

Length of output: 12002


@EtanHey Thanks for the pointer — I've kicked off a fresh review at d0e1109, and I'll specifically verify:

  • Stable UUID matching: markIntentionalSurfaceCloses (agent-engine.ts ~Line 3675) lowercases both the forensics cmux_surface_id and agent.surface_uuid before comparing, so casing differences won't cause a miss.
  • Recycled-ref protection: resolveAgentIoRoute (~Line 4404) requires a UUID-backed agent's binding to have provenance === "uuid" and the observed UUID to match exactly in a complete fresh topology before any I/O — refusing to touch a mutable ref if the UUID can't be re-proven. That's the mechanism guarding against a recycled surface:N ref being mistaken for the original pane.
  • Concurrent state eviction: markIntentionalSurfaceCloses wraps stateMgr.updateRecord in try/catch so a record removed concurrently (e.g., already evicted/purged) is simply skipped rather than throwing and breaking the sweep.
  • Degraded UUID-unavailable fallback: for UUID-less legacy records, resolveAgentIoRoute falls back to proving ownership via a complete, UUID-free topology where the binding provenance is "ref" and matches the persisted surface_id exactly — otherwise it throws rather than risk touching a recycled ref.

I'll confirm these hold up under the full incremental diff review and flag anything that doesn't line up with this reasoning.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@EtanHey

EtanHey commented Jul 14, 2026

Copy link
Copy Markdown
Owner Author

Final verification receipt for d0e1109:

  • Reproduced twice from live surface.closed events with origin: tab_close; the old sweep recovered before close forensics made the close intentional.
  • RED regressions covered UI tab close, explicit close_surface, recycled mutable refs, concurrent state eviction, unexpected persistence failure, and UUID-unavailable enumeration.
  • Counterfactual removal of the terminal writes re-failed the core regressions.
  • Full local suite: 104/104 files, 2131/2131 tests.
  • bun run typecheck, bun run build, and git diff --check: pass.
  • Read-only live runtime contract against an isolated daemon built from dist: ping, orphan denial, list/read, doctor, and retire-autostart all pass.
  • GitHub CI on this exact head: test and build-site pass.
  • CodeRabbit findings are addressed; final incremental review has no new actionable issue. Codex re-review is pending.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d0e11097be

ℹ️ 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".

Comment thread src/server.ts Outdated
record.surface_uuid.toLowerCase() === observedSurfaceUuid ||
(observedSurfaceUuid === undefined &&
record.surface_id === args.surface)
: record.surface_id === args.surface;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Restrict ref fallback when a UUID was observed

When cmux does return observedSurfaceUuid for the target, UUID-less records still match solely by the mutable surface_id here. If an older UUID-less state row still points at a ref that has since been recycled, forcing close_surface on the current UUID-backed surface will mark that stale row user_killed, suppressing crash recovery and allowing it to be purged even though stable identity was available to avoid the recycled-ref match. The ref fallback should only apply when the observation is UUID-unavailable/ref-only.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 568b40b. All mutable-ref fallback is now gated on observedSurfaceUuid being unavailable, including UUID-less legacy records. A RED regression added a UUID-less stale owner on a recycled ref and observed it incorrectly become user_killed:true; after the one-line guard, that stale row remains recoverable while the existing degraded UUID-unavailable explicit-close case remains terminal. Full 2131-test suite and live contract pass.

@cursor

cursor Bot commented Jul 14, 2026

Copy link
Copy Markdown

Bugbot couldn't run - usage limit reached

Bugbot 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_dd963762-eb49-4373-8d5e-6268eed3dac0)

@EtanHey

EtanHey commented Jul 14, 2026

Copy link
Copy Markdown
Owner Author

@coderabbitai review

@codex review

Final head is now 568b40b. Please verify the last UUID-less legacy-row recycled-ref guard; all mutable-ref fallback is restricted to UUID-unavailable observation, while the degraded explicit-close regression remains terminal.

@coderabbitai

coderabbitai Bot commented Jul 14, 2026

Copy link
Copy Markdown
🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -e
git log --oneline -5
echo "=== resolveAgentIoRoute region ==="
grep -n "resolveAgentIoRoute\|provenance\|UUID-unavailable\|uuid_unavailable\|legacy" src/agent-engine.ts | head -100

Length of output: 2580


🏁 Script executed:

#!/bin/bash
sed -n '2960,3080p' src/agent-engine.ts
echo "=== 3340,3470 ==="
sed -n '3340,3470p' src/agent-engine.ts
echo "=== 4390,4470 ==="
sed -n '4390,4470p' src/agent-engine.ts

Length of output: 12659


🏁 Script executed:

#!/bin/bash
git show 568b40b --stat
echo "=== diff ==="
git diff d0e1109 568b40b -- src/agent-engine.ts | head -200

Length of output: 575


🏁 Script executed:

#!/bin/bash
git diff d0e1109 568b40b -- src/server.ts tests/server.test.ts

Length of output: 1856


🏁 Script executed:

#!/bin/bash
sed -n '6760,6850p' src/server.ts

Length of output: 3563


🏁 Script executed:

#!/bin/bash
sed -n '9790,9910p' tests/server.test.ts

Length of output: 3984


🏁 Script executed:

#!/bin/bash
sed -n '9910,9990p' tests/server.test.ts

Length of output: 2793


This confirms both properties EtanHey asked me to verify at 568b40b:

  • Line 6828–6835 in src/server.ts: the UUID-less legacy-row fallback (record.surface_id === args.surface) is now gated by observedSurfaceUuid === undefined, so it only fires when the current observation genuinely cannot resolve a UUID for the closed ref — never when a live UUID is observed but simply belongs to a different (recycled) owner.
  • This lines up exactly with resolveAgentIoRoute's stricter guard (Lines 4404–4433 in src/agent-engine.ts), which also only permits the UUID-less ref fallback when a complete fresh topology proves zero UUID coverage (surfaceIdByRef.size === 0 && surfaceRefById.size === 0). Both code paths now share the same "UUID-unavailable observation" invariant.
  • The new regression test close_surface does not make a stale UUID owner terminal when its ref was recycled (tests/server.test.ts, Lines ~9846–9853, ~9899–9902) adds a second legacy record (worker-legacy-stale-ref-owner, no surface_uuid) sharing the recycled ref, and asserts it stays user_killed: false when the live topology does resolve a UUID (bbbbbbbb-cccc-...) for that ref — confirming the fallback no longer misfires.
  • Immediately after, the pre-existing worker-degraded-ref-owner case (Lines ~9910–9932) still asserts user_killed: true when surface enumeration itself fails (listWorkspaces rejects → observedSurfaceUuid stays undefined), so the genuinely degraded/UUID-unavailable explicit-close path remains terminal as intended.

No further issues found in this narrow diff (d0e1109568b40b, src/server.ts + tests/server.test.ts only); the fix is precisely scoped and the test coverage matches the guard's intent.


✏️ Learnings added
Learnt from: EtanHey
URL: https://github.com/EtanHey/cmuxlayer/pull/323

Timestamp: 2026-07-14T17:22:29.008Z
Learning: In cmuxlayer's `src/server.ts` `close_surface` tool handler, the UUID-less legacy-record fallback match (`record.surface_id === args.surface` for records without `surface_uuid`) must only be applied when `observedSurfaceUuid === undefined` (i.e., the live topology genuinely cannot resolve a UUID for the closed ref). This mirrors the invariant in `resolveAgentIoRoute` (`src/agent-engine.ts`), which only permits UUID-less ref-based terminal I/O when a complete fresh topology proves zero UUID coverage. Without this guard, a recycled mutable ref could be mistakenly attributed to a stale legacy record even when a live UUID is observed for that ref (belonging to a different, current owner).

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 568b40b939

ℹ️ 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".

Comment thread src/agent-engine.ts Outdated
events
.filter(
(event) =>
event.origin === "tab_close" &&

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Treat workspace teardown closes as terminal

When cmux emits surface.closed with origin: "workspace_teardown", this filter drops it even though it is the same app-level/user-close forensics path as a tab close. In that scenario a crash-recoverable agent whose workspace was closed by the operator is not marked user_killed before absence reconciliation, so it can be classified as an unintentional disappearance and respawned on a later sweep. Include workspace teardown in the terminal-intent set so closing a workspace does not resurrect its agents.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in cd1ac43. The pre-reconciliation intentional-close set now includes both app-level origins documented by close forensics: tab_close and workspace_teardown. The lifecycle regression is parameterized over both; workspace_teardown was RED with user_killed:false and is now GREEN, while the paired genuine PTY-crash resume test remains intact. Full 2132-test suite, typecheck/build, and live isolated runtime contract pass.

@cursor

cursor Bot commented Jul 14, 2026

Copy link
Copy Markdown

Bugbot couldn't run - usage limit reached

Bugbot 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_3a76b9e9-a8b0-4837-a7c0-750fc37c14c2)

@EtanHey

EtanHey commented Jul 14, 2026

Copy link
Copy Markdown
Owner Author

@coderabbitai review

@codex review

Final head is cd1ac43. Please re-review the intentional close-origin set: both tab_close and workspace_teardown are terminal before recovery; genuine PTY death still resumes.

@coderabbitai

coderabbitai Bot commented Jul 14, 2026

Copy link
Copy Markdown
🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -e
git log --oneline -8
echo "=== diff 568b40b..cd1ac43 ==="
git diff 568b40b cd1ac43 --stat
echo "=== close-forensics diff ==="
git diff 568b40b cd1ac43 -- src/close-forensics.ts

Length of output: 1011


🏁 Script executed:

#!/bin/bash
set -e
git diff 568b40b cd1ac43 -- src/agent-engine.ts
echo "=== context around change ==="
rg -n "workspace_teardown|tab_close|origin" src/agent-engine.ts | head -50

Length of output: 2698


🏁 Script executed:

#!/bin/bash
set -e
rg -n "origin" src/close-forensics.ts
echo "=== test diff snippet ==="
git diff 568b40b cd1ac43 -- tests/agent-engine.test.ts | head -200

Length of output: 6877


@EtanHey Verified at cd1ac43 — this is a clean, narrowly-scoped diff (src/agent-engine.ts + tests/agent-engine.test.ts only, vs 568b40b).

  • Intentional close-origin set: the forensics filter in runCloseForensicsBestEffort (Lines 3676–3681 of src/agent-engine.ts) now matches event.origin === "tab_close" || event.origin === "workspace_teardown", up from tab_close alone. The doc comment above it was updated to match. src/close-forensics.ts is untouched — it passes origin through generically from the payload, so there's no upstream allowlist gap to worry about.
  • Test coverage: the previous single-origin test was converted to it.each(["tab_close", "workspace_teardown"]), asserting for both origins that the matching agent is marked user_killed: true on the first sweep, then evicted (getAgentState(...)null) after the eviction confirmation window elapses, with mockClient.newSplit never called — i.e., no respawn attempt for either intentional-close origin.
  • Genuine PTY-death recovery: the adjacent test asserting respawn_attempts increments on genuine crash recovery (just above the modified block) is untouched by this diff, confirming the existing respawn/limit path for real PTY death is unaffected.

No issues found in this diff — the terminal-origin set is correctly widened and both new/old behaviors are covered by tests.


✏️ Learnings added
Learnt from: EtanHey
URL: https://github.com/EtanHey/cmuxlayer/pull/323

Timestamp: 2026-07-14T17:32:22.615Z
Learning: In cmuxlayer's `src/agent-engine.ts`, `runCloseForensicsBestEffort` treats both `tab_close` and `workspace_teardown` close-forensics event origins as terminal operator intent for a matching managed surface, persisting that intent before absence reconciliation can treat it as a recoverable crash (as of commit cd1ac43). Previously only `tab_close` was treated this way. Genuine PTY-death recovery (respawn with attempt limits) is a separate code path and remains unaffected by this origin allowlist.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@cursor

cursor Bot commented Jul 14, 2026

Copy link
Copy Markdown

Bugbot couldn't run - usage limit reached

Bugbot 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_15c9fd19-c70b-4f46-8ec7-2c94a500361a)

@EtanHey

EtanHey commented Jul 14, 2026

Copy link
Copy Markdown
Owner Author

@coderabbitai review

@codex review

Final conflict-free head is 07a5ce0 after merging current main. The only conflict was the src/agent-engine.ts import block; resolution retains both CloseForensicsSweepResult and main’s cleanScreenText. Merged-tree verification: 104/104 files, 2146/2146 tests, typecheck/build/diff-check, and live isolated runtime contract all pass.

@coderabbitai

coderabbitai Bot commented Jul 14, 2026

Copy link
Copy Markdown
🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -e
git log --oneline -5
echo "=== merge commit stat ==="
git show 07a5ce0 --stat | head -30
echo "=== import block in agent-engine.ts ==="
sed -n '1,40p' src/agent-engine.ts
echo "=== check cleanScreenText usage ==="
rg -n "cleanScreenText" src/agent-engine.ts
echo "=== check CloseForensicsSweepResult usage ==="
rg -n "CloseForensicsSweepResult" src/agent-engine.ts

Length of output: 3527


🏁 Script executed:

#!/bin/bash
set -e
echo "=== conflict markers check ==="
rg -n "^(<<<<<<<|=======|>>>>>>>)" src/agent-engine.ts src/fleet-sidebar.ts src/fleet-sidebar-cli.ts src/screen-parser.ts || echo "none found"
echo "=== close-forensics origin logic intact ==="
rg -n "tab_close|workspace_teardown" src/agent-engine.ts
echo "=== unrelated changes touching close/user_killed area ==="
git diff cd1ac43 07a5ce0 -- src/agent-engine.ts | head -150

Length of output: 6329


@EtanHey Verified at 07a5ce0 — this is a clean merge.

  • No leftover conflict markers: checked src/agent-engine.ts, src/fleet-sidebar.ts, src/fleet-sidebar-cli.ts, src/screen-parser.ts — none found.
  • Import block resolution: both imports coexist correctly, Line 51 import type { CloseForensicsSweepResult } from "./close-forensics.js"; and Line 52 import { cleanScreenText, parseScreen } from "./screen-parser.js"; — no duplication, no shadowing.
  • Close-forensics terminal-origin logic untouched: the tab_close/workspace_teardown allowlist from cd1ac43 (Lines 3746–3747) is present verbatim in the merged tree, with no interference from main's incoming changes.
  • Incoming main changes are orthogonal: git diff cd1ac43 07a5ce0 -- src/agent-engine.ts shows the merge only pulled in main's fleet-sidebar "working, no progress" screen-tracking feature (fleetScreenProgress, recordFleetScreenProgress, lastAgentProgressAtMs, fleetWorkingNoProgressTimeoutMs) — none of which touches the close_surface/user_killed/crash-recovery paths this PR modifies.

No issues found in this merge — the conflict resolution is correct and the PR's core fix remains intact alongside main's unrelated fleet-sidebar work.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@EtanHey
EtanHey merged commit a7c4556 into main Jul 14, 2026
5 checks passed

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 07a5ce0f72

ℹ️ 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".

Comment thread src/agent-engine.ts

private async runSweepOnce(): Promise<void> {
this.currentSweepScreenSignatures = new Map();
await this.runCloseForensicsBestEffort();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Import disk state before consuming close events

When another cmuxlayer process writes an agent state after this engine's previous sweep, that record is not present in this.registry.list() until registry.reconcile() runs. Because this line now consumes and advances close-forensics before that import, a tab_close for the newly-on-disk agent is skipped by markIntentionalSurfaceCloses; the same sweep then imports the record and later can crash-recover it because the close event will not be replayed. Consider reconciling/importing disk state before consuming forensics, or matching the events against stateMgr.listStates() as well.

Useful? React with 👍 / 👎.

Comment thread src/server.ts
Comment on lines +6834 to +6835
: observedSurfaceUuid === undefined &&
record.surface_id === args.surface;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve close intent for UUID-less records

For an existing managed agent whose state record has no surface_uuid (for example an upgraded/ref-only record), a successful enumeration of the target surface sets observedSurfaceUuid, so this fallback refuses to match the record by its current surface_id. A forced close_surface then closes the pane but leaves user_killed=false, making the intentional close eligible for crash recovery instead of terminal handling. The recycled-ref guard still needs to avoid stale UUID owners, but the current branch also drops the real UUID-less owner of the requested ref.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant