Repository navigation
Conversation
📝 WalkthroughWalkthrough
ChangesSurface exit runtime pipeline
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant TerminalHost
participant SurfaceStager
participant Mux
participant App
TerminalHost->>TerminalHost: Calculate elapsed runtime_ms
TerminalHost->>SurfaceStager: Send Exit payload
SurfaceStager->>Mux: Emit SurfaceExited { surface, runtime_ms }
Mux->>App: Deliver surface-exited event
App->>App: Compare runtime_ms with configured threshold
App->>App: Retain or close exited surface
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 24 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (24 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThe PR completes the hosted-shell exit cleanup flow.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains; the previously reported surface-only tombstone omission and cross-language binding inconsistency are addressed at the current head. Important Files Changed
Sequence DiagramsequenceDiagram
participant Host as Terminal host
participant Core as cmux-tui-core
participant App as TUI frontend
participant Tree as Durable topology
Host-->>Core: Exit(runtime_ms)
Core-->>App: SurfaceExited(surface, runtime_ms)
alt Startup failure at or below threshold
App->>App: Retain exited surface
else Established shell
alt Last surface in workspace
App->>Tree: close-workspace
else Workspace has other surfaces
App->>Tree: close-surface
end
App->>App: Retire local surface state
end
Reviews (2): Last reviewed commit: "fix(tui): complete exited surface cleanu..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if self.surface_only == Some(surface) { | ||
| self.retire_surface_state(surface); | ||
| self.remove_surface_from_tree(surface); | ||
| self.quit = true; | ||
| return Ok(RenderAction::None); | ||
| } |
There was a problem hiding this comment.
When an established hosted PTY exits during cmux-tui attach --surface, this branch removes only the local state and quits before sending close-surface or close-workspace, causing the dead surface to remain in the server's durable workspace topology and reappear on a later attach.
Knowledge Base Used: cmux-tui
There was a problem hiding this comment.
Fixed in 78361d5. Established surface-only exits now enqueue the same close mutation before quitting; the new regression verifies the workspace is removed.
| #[serde(default)] | ||
| pub runtime_ms: Option<u64>, |
There was a problem hiding this comment.
The server and Rust binding add runtime_ms, but the Go, Python, TypeScript, and Java typed surface-exited models and conformance fixtures remain unchanged. Those consumers silently lose the metadata and cannot implement the documented established-shell versus startup-failure policy without bypassing their typed APIs.
Knowledge Base Used: cmux-tui
There was a problem hiding this comment.
Fixed in 78361d5. runtime_ms now exists in the canonical spec and Rust, Go, Python, TypeScript, and Java event models, with parser/type coverage in every binding.
|
Addressed Greptile's findings in
Reverified the TUI exit tests, all-target check, release build, and all five binding suites. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@cmux-tui/crates/cmux-tui/src/app.rs`:
- Around line 6363-6367: Update the runtime comparison in the abnormal
command-exit handling to use a strict “below threshold” check, so an exit
exactly at the configured threshold is treated as established/normal. Also
update startup_failure_exit_remains_visible_at_ghostty_threshold to use a
runtime value below the default threshold.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 602bfac3-f55a-4431-90c3-6250b046592d
📒 Files selected for processing (12)
cmux-tui/bindings/rust/src/lib.rscmux-tui/crates/cmux-tui-core/src/event_bus.rscmux-tui/crates/cmux-tui-core/src/mux.rscmux-tui/crates/cmux-tui-core/src/server.rscmux-tui/crates/cmux-tui-core/src/surface.rscmux-tui/crates/cmux-tui-core/src/terminal_host_protocol.rscmux-tui/crates/cmux-tui-core/src/terminal_host_runtime.rscmux-tui/crates/cmux-tui-core/tests/pty.rscmux-tui/crates/cmux-tui/src/app.rscmux-tui/crates/cmux-tui/src/config.rscmux-tui/crates/cmux-tui/src/session/remote.rscmux-tui/docs/protocol.md
There was a problem hiding this comment.
♻️ Duplicate comments (1)
cmux-tui/crates/cmux-tui/src/app.rs (1)
6356-6379: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThreshold comparison still treats an exit exactly at Ghostty's threshold as a startup failure.
This was already flagged in a prior review round: Ghostty's
abnormal-command-exit-runtimeis an exclusive threshold (exit strictly below the value is abnormal; exiting at exactly the threshold is normal/established). The current code still usesruntime_ms <= thresholdto retain as a startup failure andruntime_ms > thresholdto treat as established, which classifies an exit exactly at the configured threshold (e.g. the default 250ms) as abnormal instead of established.startup_failure_exit_remains_visible_at_ghostty_threshold(Line 16006) encodes this same off-by-one by assertingruntime_ms: Some(250)stays retained.🐛 Suggested fix
- if runtime_ms.is_some_and(|runtime_ms| { - runtime_ms > self.config.abnormal_command_exit_runtime_ms() - }) { + if runtime_ms.is_some_and(|runtime_ms| { + runtime_ms >= self.config.abnormal_command_exit_runtime_ms() + }) { self.close_established_exited_surface(surface); } @@ - if runtime_ms.is_some_and(|runtime_ms| { - runtime_ms <= self.config.abnormal_command_exit_runtime_ms() - }) { + if runtime_ms.is_some_and(|runtime_ms| { + runtime_ms < self.config.abnormal_command_exit_runtime_ms() + }) { return Ok(RenderAction::Draw); }Then update
startup_failure_exit_remains_visible_at_ghostty_thresholdto use a runtime strictly below the default threshold (e.g. 249ms) so it still exercises the "retained" path.Also applies to: 16006-16031
🤖 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 `@cmux-tui/crates/cmux-tui/src/app.rs` around lines 6356 - 6379, Correct the threshold comparisons in the SurfaceExited handler so runtime_ms equal to abnormal_command_exit_runtime_ms() is treated as established: use strictly below for retaining startup failures and the complementary established condition for closing them, including the surface_only path. Update startup_failure_exit_remains_visible_at_ghostty_threshold to use a below-threshold runtime such as 249ms.
🤖 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.
Duplicate comments:
In `@cmux-tui/crates/cmux-tui/src/app.rs`:
- Around line 6356-6379: Correct the threshold comparisons in the SurfaceExited
handler so runtime_ms equal to abnormal_command_exit_runtime_ms() is treated as
established: use strictly below for retaining startup failures and the
complementary established condition for closing them, including the surface_only
path. Update startup_failure_exit_remains_visible_at_ghostty_threshold to use a
below-threshold runtime such as 249ms.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ca3f227c-b8dd-4b8c-9363-6da9e14fd9ce
📒 Files selected for processing (12)
cmux-tui/bindings/go/events_test.gocmux-tui/bindings/go/types.gocmux-tui/bindings/java/src/com/cmux/CmuxEvent.javacmux-tui/bindings/java/src/com/cmux/SurfaceEvent.javacmux-tui/bindings/java/tests/com/cmux/JsonTest.javacmux-tui/bindings/python/cmux/client.pycmux-tui/bindings/python/tests/test_events.pycmux-tui/bindings/rust/src/lib.rscmux-tui/bindings/typescript/src/protocol/events.tscmux-tui/bindings/typescript/test/protocol-types.tscmux-tui/crates/cmux-tui/src/app.rscmux-tui/spec/events.md
# Conflicts: # cmux-tui/crates/cmux-tui/src/app.rs
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmux-tui/bindings/go/types.go (1)
429-431: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer
RuntimeMsoverRuntimeMSfor Go naming convention."MS" isn't a recognized Go initialism (unlike
ID,URL,UUID), soRuntimeMSreads inconsistently with idiomatic Go and may tripstaticcheck/revivenaming checks. Since this is a brand-new exported field, renaming now is essentially free.♻️ Proposed rename
- RuntimeMS *uint64 `json:"runtime_ms"` + RuntimeMs *uint64 `json:"runtime_ms"`🤖 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 `@cmux-tui/bindings/go/types.go` around lines 429 - 431, Rename the exported field RuntimeMS to RuntimeMs in the relevant event type, preserving its *uint64 type and json:"runtime_ms" tag so the external JSON contract remains unchanged.
🤖 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 `@cmux-tui/crates/cmux-tui/src/app.rs`:
- Around line 5990-6008: Update close_established_exited_surface to derive the
last-tab decision from a freshly queried authoritative session tree, not the
render-cadence-bound self.tree/tab_locations snapshot. Preserve closing only the
exited surface unless the fresh mux state confirms it is the workspace’s sole
remaining tab; ensure the change cannot invoke close_workspace based on stale
cached state.
- Around line 17750-17838: Add tests covering both missing branches: verify an
established surface exit in a workspace with additional tabs closes only that
surface and preserves the workspace, and verify a quick/startup-failure exit in
surface_only mode quits without explicitly closing the surface. Exercise the
relevant MuxEvent::SurfaceExited handling and assert the resulting
workspace/surface state and app.quit behavior.
---
Outside diff comments:
In `@cmux-tui/bindings/go/types.go`:
- Around line 429-431: Rename the exported field RuntimeMS to RuntimeMs in the
relevant event type, preserving its *uint64 type and json:"runtime_ms" tag so
the external JSON contract remains unchanged.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 27d643f5-1c3a-4200-982e-a294eebe0e35
📒 Files selected for processing (12)
cmux-tui/bindings/go/types.gocmux-tui/bindings/python/cmux/client.pycmux-tui/bindings/rust/src/lib.rscmux-tui/crates/cmux-tui-core/src/mux.rscmux-tui/crates/cmux-tui-core/src/server.rscmux-tui/crates/cmux-tui-core/src/surface.rscmux-tui/crates/cmux-tui-core/src/terminal_host_protocol.rscmux-tui/crates/cmux-tui-core/src/terminal_host_runtime.rscmux-tui/crates/cmux-tui/src/app.rscmux-tui/crates/cmux-tui/src/config.rscmux-tui/crates/cmux-tui/src/session/remote.rscmux-tui/docs/protocol.md
# Conflicts: # cmux-tui/bindings/typescript/test/protocol-types.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
cmux-tui/bindings/python/cmux/client.py (3)
11-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winResolve the new Ruff annotation warnings.
Ruff reports
UP035fortyping.Iterator/Dict/ListandUP007for the newUnionalias. If the declared Python target supports modern annotations, migrate consistently; otherwise confirm the Ruff target intentionally preserves compatibility.Also applies to: 309-309
🤖 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 `@cmux-tui/bindings/python/cmux/client.py` at line 11, Update the type imports and annotations in cmux’s Python client to resolve Ruff UP035 and UP007: replace typing.Iterator, Dict, and List, and modernize Union annotations consistently if the supported Python target allows it; otherwise configure or preserve the intended compatibility target so Ruff does not flag these imports. Apply the same change to the additional Union usage.Source: Linters/SAST tools
1193-1201: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winValidate newly typed protocol metadata at the JSON boundary.
float(...)accepts non-finite and out-of-range viewport widths, whileruntime_msaccepts strings, floats, booleans, and negative values. These can violate the public dataclass contracts and break layout or exit-threshold consumers. Use strict parsing helpers and raiseProtocolErrorfor malformed values.Also applies to: 1269-1269
🤖 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 `@cmux-tui/bindings/python/cmux/client.py` around lines 1193 - 1201, Validate protocol metadata at the JSON boundary before constructing the dataclass: update parsing for viewport_base_width and ViewportSplit.width to use strict finite, contract-range float helpers, and update runtime_ms parsing to accept only valid non-negative integer values, rejecting strings, floats, booleans, and negatives. Raise ProtocolError for every malformed or out-of-range value, while preserving None and missing-field behavior where supported.
48-50: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject boolean viewport widths.
boolis anintsubclass, sowidth=Truepasses both checks and sends JSONtrueinstead of a numeric fraction. Reject booleans and non-numeric values before callingmath.isfinite.Proposed fix
def _validate_viewport_pane_width(width: float) -> None: - if not math.isfinite(width) or not 0.1 <= width <= 1.0: + if ( + isinstance(width, bool) + or not isinstance(width, (int, float)) + or not math.isfinite(width) + or not 0.1 <= width <= 1.0 + ): raise ValueError("viewport pane width must be between 0.1 and 1.0")🤖 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 `@cmux-tui/bindings/python/cmux/client.py` around lines 48 - 50, Update _validate_viewport_pane_width to reject boolean and non-numeric inputs before calling math.isfinite, while preserving the existing finite 0.1–1.0 range validation for numeric widths.
🤖 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 `@cmux-tui/crates/cmux-tui/src/app.rs`:
- Around line 6987-7013: Update close_established_exited_surface and both
SurfaceExited call paths so Remote sessions do not synchronously invoke
self.session.refresh_tree() on the event-loop thread. Move the
workspace-membership and last-tab decision to the background worker, then apply
close_surface or close_workspace after the refreshed tree is available, or
delegate that atomic decision to the mux while preserving the existing close
behavior.
---
Outside diff comments:
In `@cmux-tui/bindings/python/cmux/client.py`:
- Line 11: Update the type imports and annotations in cmux’s Python client to
resolve Ruff UP035 and UP007: replace typing.Iterator, Dict, and List, and
modernize Union annotations consistently if the supported Python target allows
it; otherwise configure or preserve the intended compatibility target so Ruff
does not flag these imports. Apply the same change to the additional Union
usage.
- Around line 1193-1201: Validate protocol metadata at the JSON boundary before
constructing the dataclass: update parsing for viewport_base_width and
ViewportSplit.width to use strict finite, contract-range float helpers, and
update runtime_ms parsing to accept only valid non-negative integer values,
rejecting strings, floats, booleans, and negatives. Raise ProtocolError for
every malformed or out-of-range value, while preserving None and missing-field
behavior where supported.
- Around line 48-50: Update _validate_viewport_pane_width to reject boolean and
non-numeric inputs before calling math.isfinite, while preserving the existing
finite 0.1–1.0 range validation for numeric widths.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9bf96e79-8cc9-44d0-a7bd-59ada7412ff7
📒 Files selected for processing (13)
cmux-tui/bindings/go/types.gocmux-tui/bindings/java/tests/com/cmux/JsonTest.javacmux-tui/bindings/python/cmux/client.pycmux-tui/bindings/rust/src/lib.rscmux-tui/bindings/typescript/src/protocol/events.tscmux-tui/bindings/typescript/test/protocol-types.tscmux-tui/crates/cmux-tui-core/src/mux.rscmux-tui/crates/cmux-tui-core/src/server.rscmux-tui/crates/cmux-tui-core/src/surface.rscmux-tui/crates/cmux-tui/src/app.rscmux-tui/crates/cmux-tui/src/config.rscmux-tui/crates/cmux-tui/src/session/remote.rscmux-tui/docs/protocol.md
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai resume |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cmux-tui/bindings/rust/src/lib.rs (1)
1632-1645: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a legacy (no
runtime_ms) decode test alongside this one.This test only covers the field-present path. The sibling
title_changed_decodes_authoritative_titletest (right above, at line 1609) deliberately covers both the new-field and legacy-payload cases; this test should follow the same pattern given the PR explicitly targets accepting legacy emptysurface-exitedpayloads.✅ Suggested legacy-payload test
#[test] fn surface_exited_decodes_hosted_runtime() { let event = parse_event(serde_json::json!({ "event": "surface-exited", "surface": 7, "runtime_ms": 321, })); assert!(matches!( event, Event::SurfaceExited(SurfaceEvent { surface: 7, runtime_ms: Some(321) }) )); + + let legacy = parse_event(serde_json::json!({ + "event": "surface-exited", + "surface": 7, + })); + assert!(matches!( + legacy, + Event::SurfaceExited(SurfaceEvent { surface: 7, runtime_ms: None }) + )); }🤖 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 `@cmux-tui/bindings/rust/src/lib.rs` around lines 1632 - 1645, Add a sibling legacy-payload case to surface_exited_decodes_hosted_runtime that parses a surface-exited event without runtime_ms and asserts Event::SurfaceExited with the same surface and runtime_ms: None, matching the existing title_changed_decodes_authoritative_title test pattern.cmux-tui/crates/cmux-tui/src/session/mod.rs (1)
64-78: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winUse localized product text for stale layout errors.
rejection_message()is copied intoLayoutUndoError::Stale, allowing raw remote/server text to reach the UI. Preserve the structured error code, but use the localized stale-layout catalog message instead.As per coding guidelines, user-facing errors must not expose raw upstream messages or implementation details.
Suggested fix
- Some(LayoutUndoError::STALE_CODE) => LayoutUndoError::Stale( - remote - .rejection_message() - .unwrap_or(crate::localization::catalog().layout.layout_changed_before_undo) - .to_string(), - ) - .into(), + Some(LayoutUndoError::STALE_CODE) => LayoutUndoError::Stale( + crate::localization::catalog().layout.layout_changed_before_undo.to_string(), + ) + .into(),🤖 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 `@cmux-tui/crates/cmux-tui/src/session/mod.rs` around lines 64 - 78, Update normalize_remote_layout_undo_error so the LayoutUndoError::Stale variant always uses the localized catalog message layout_changed_before_undo, while preserving the existing stale rejection code mapping and error behavior for other codes.Source: Coding guidelines
🤖 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 `@cmux-tui/crates/cmux-tui-core/src/mux.rs`:
- Around line 6422-6431: Define a named constant for the unknown hosted runtime
sentinel and replace the bare u64::MAX in the SurfaceExited emission path with
that constant. Place the constant near the relevant mux runtime metadata
definitions so future comparisons can reuse the same symbol.
---
Outside diff comments:
In `@cmux-tui/bindings/rust/src/lib.rs`:
- Around line 1632-1645: Add a sibling legacy-payload case to
surface_exited_decodes_hosted_runtime that parses a surface-exited event without
runtime_ms and asserts Event::SurfaceExited with the same surface and
runtime_ms: None, matching the existing
title_changed_decodes_authoritative_title test pattern.
In `@cmux-tui/crates/cmux-tui/src/session/mod.rs`:
- Around line 64-78: Update normalize_remote_layout_undo_error so the
LayoutUndoError::Stale variant always uses the localized catalog message
layout_changed_before_undo, while preserving the existing stale rejection code
mapping and error behavior for other codes.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: fabe5535-0fdc-4b12-a524-ccd95650880e
📒 Files selected for processing (14)
cmux-tui/bindings/go/types.gocmux-tui/bindings/java/tests/com/cmux/JsonTest.javacmux-tui/bindings/python/cmux/client.pycmux-tui/bindings/rust/src/lib.rscmux-tui/bindings/typescript/src/protocol/events.tscmux-tui/bindings/typescript/test/protocol-types.tscmux-tui/crates/cmux-tui-core/src/mux.rscmux-tui/crates/cmux-tui-core/src/server.rscmux-tui/crates/cmux-tui-core/src/surface.rscmux-tui/crates/cmux-tui/src/app.rscmux-tui/crates/cmux-tui/src/config.rscmux-tui/crates/cmux-tui/src/session/mod.rscmux-tui/crates/cmux-tui/src/session/remote.rscmux-tui/docs/protocol.md
| self.emit(MuxEvent::SurfaceExited { | ||
| surface: id, | ||
| // Hosts predating runtime metadata still represent a process | ||
| // that reached the live stream, not a local/browser reap. | ||
| runtime_ms: Some(runtime_ms.unwrap_or(u64::MAX)), | ||
| }); | ||
| return; | ||
| } | ||
| self.remove_surface_after_registry(id); | ||
| self.emit(MuxEvent::SurfaceExited(id)); | ||
| self.emit(MuxEvent::SurfaceExited { surface: id, runtime_ms: None }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Consider naming the u64::MAX sentinel.
Hosts predating runtime metadata still represent a process that reached the live stream, not a local/browser reap. The logic is correct, but the bare u64::MAX magic value could be extracted into a named constant (e.g. UNKNOWN_HOSTED_RUNTIME_MS) to make the intent self-documenting at the call site and any future comparison sites.
🤖 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 `@cmux-tui/crates/cmux-tui-core/src/mux.rs` around lines 6422 - 6431, Define a
named constant for the unknown hosted runtime sentinel and replace the bare
u64::MAX in the SurfaceExited emission path with that constant. Place the
constant near the relevant mux runtime metadata definitions so future
comparisons can reuse the same symbol.
✅ Action performedReviews resumed. |
2 participants
Summary
Exitframes andsurface-exitedevents, while accepting legacy empty payloadsabnormal-command-exit-runtime, but explicitly close established shellsProblem
Typing
exitin an established cmux-tui shell left a dead hosted terminal in durable topology. The frontend only removed that surface from its local tree; it never sent the explicit close/tombstone required by hosted terminals.Verification
cargo +1.97.1 test -p cmux-tui-core— 404 passedcargo +1.97.1 check -p cmux-tui --all-targetscargo +1.97.1 build --release -p cmux-tuiexitin the last shell; cmux-tui exited cleanly with status 0exit; status 0 and no panicNeed help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Fixes TUI shells staying open after exit by auto-closing established hosted terminals and collapsing their workspace, so the TUI exits cleanly when the last shell finishes. Adds hosted child
runtime_msto exits to retain startup failures while closing established sessions.Bug Fixes
New Features
runtime_msonsurface-exitedevents and terminal-hostExitframes; accept legacy empty payloads.abnormal-command-exit-runtime(default 250ms) to retain short startup failures and close established exits.runtime_msvia remote sessions and all bindings (Go, Java, Python, TypeScript, Rust); update protocol docs.Written for commit 83d9c10. Summary will update on new commits.
Summary by CodeRabbit
surface-exitednow optionally includesruntime_msfor hosted terminal children (exposed across supported bindings).abnormal-command-exit-runtime_msconfig support (default: 250 ms) to improve abnormal exit decisions.surface-exitedpayload shape whenruntime_msis missing/null.surface-exitedevent contract and compatibility notes for older servers.