Repository navigation
cmux-tui: integrate journaling with the native browser provider - #9726
Merged
Merged
Conversation
…at-cmux-tui-session-journal # Conflicts: # cmux-tui/bindings/rust/tests/catalog_manifest.rs # cmux-tui/crates/cmux-tui-core/src/resource_api.rs
…at-cmux-tui-session-journal
…ion-journal # Conflicts: # cmux-tui/bindings/rust/src/codec.rs # cmux-tui/bindings/rust/src/resource/client.rs
…at-cmux-tui-session-journal # Conflicts: # .github/workflows/cmux-tui.yml # cmux-tui/bindings/rust-sidebar/tests/runtime.rs # cmux-tui/crates/cmux-tui/src/ui/graphics_writer.rs
…at-cmux-tui-session-journal # Conflicts: # cmux-tui/crates/cmux-tui-core/src/browser.rs # cmux-tui/crates/cmux-tui-core/src/terminal_host_runtime.rs # cmux-tui/crates/cmux-tui-core/src/workspace_registry/public_projection_store.rs # cmux-tui/crates/cmux-tui-core/tests/browser_runtime.rs
lawrencecchen
marked this pull request as ready for review
August 10, 2026 16:33
Contributor
|
Too many files changed for review (223 files, 100 file limit). Bypass the limit by tagging |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 79
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
cmux-tui/crates/cmux-tui-core/src/workspace_registry/terminal_exit_store.rs (1)
146-153: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick winThe topology patch is applied twice in the same transaction.
Lines 147-149 add a new
apply_resource_patchcall. The identical call already exists at Lines 151-153. Both run inside the same transaction with the samesqlite_resource_revision, so the patch is applied twice for every terminal exit that carries topology.The second application repeats every insert and tombstone in the patch. It can fail on a uniqueness or identity constraint, and it doubles the write work even when it succeeds. Delete the duplicate block.
🐛 Proposed fix
- - if let Some((patch, _)) = topology { - apply_resource_patch(&tx, patch, sqlite_resource_revision)?; - } if let Some((patch, _)) = topology { apply_resource_patch(&tx, patch, sqlite_resource_revision)?; }🤖 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/workspace_registry/terminal_exit_store.rs` around lines 146 - 153, Remove the duplicate topology handling in the transaction by deleting one of the identical if-let blocks that calls apply_resource_patch with topology and sqlite_resource_revision. Keep exactly one application of the patch for each terminal exit.cmux-tui/crates/cmux-tui-core/src/workspace_registry/effect_store.rs (1)
180-220: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe interrupted-effect predicate is duplicated between the SELECT and the UPDATE.
Lines 184-190 and Lines 202-208 repeat the same
state = 'executing' AND NOT EXISTS (...)clause. The journal records appended at Line 212 are correct only while both copies stay identical. If one copy changes, receipts move toindeterminatewithout a journal record, or a journal record is appended for a receipt that staysexecuting.Extract the predicate into one constant and use it in both statements.
♻️ Proposed consolidation
const UNCORRELATED_EXECUTING_EFFECT_SQL: &str = "state = 'executing' AND NOT EXISTS ( SELECT 1 FROM resource_creation_receipts creation WHERE creation.idempotency_key = resource_effect_receipts.idempotency_key AND creation.execution_kind = 'effect' AND creation.state = 'executing' )";- let mut statement = transaction.prepare( - "SELECT idempotency_key, operation, intent_json - FROM resource_effect_receipts - WHERE state = 'executing' - AND NOT EXISTS ( - SELECT 1 FROM resource_creation_receipts creation - WHERE creation.idempotency_key = resource_effect_receipts.idempotency_key - AND creation.execution_kind = 'effect' - AND creation.state = 'executing' - ) - ORDER BY idempotency_key", - )?; + let mut statement = transaction.prepare(&format!( + "SELECT idempotency_key, operation, intent_json + FROM resource_effect_receipts + WHERE {UNCORRELATED_EXECUTING_EFFECT_SQL} + ORDER BY idempotency_key" + ))?;- transaction.execute( - "UPDATE resource_effect_receipts - SET state = 'indeterminate' - WHERE state = 'executing' - AND NOT EXISTS ( - SELECT 1 FROM resource_creation_receipts creation - WHERE creation.idempotency_key = resource_effect_receipts.idempotency_key - AND creation.execution_kind = 'effect' - AND creation.state = 'executing' - )", - [], - )?; + transaction.execute( + &format!( + "UPDATE resource_effect_receipts + SET state = 'indeterminate' + WHERE {UNCORRELATED_EXECUTING_EFFECT_SQL}" + ), + [], + )?;🤖 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/workspace_registry/effect_store.rs` around lines 180 - 220, Extract the duplicated interrupted-effect predicate from the receipt recovery flow into a single SQL constant, such as UNCORRELATED_EXECUTING_EFFECT_SQL, and interpolate or otherwise reuse it in both the SELECT that builds interrupted and the UPDATE that marks receipts indeterminate. Keep both statements semantically identical so every updated receipt receives the corresponding journal record.cmux-tui/bindings/zig/src/resource.zig (1)
1480-1486: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winRedact
attachment_leasein cloned error details.The new viewer controls send
attachment_leaseas a capability token.cloneRedactedkeeps any field thatisSecretFielddoes not list. If the server echoes the lease insideerror.details, the SDK stores the unredacted lease inlastResourceError, and callers that log the error expose it.🔒️ Proposed fix to redact the attachment lease
fn isSecretField(key: []const u8) bool { return std.mem.eql(u8, key, "token") or std.mem.eql(u8, key, "specifier") or std.mem.eql(u8, key, "credential") or std.mem.eql(u8, key, "secret") or + std.mem.eql(u8, key, "attachment_lease") or std.mem.eql(u8, key, "authority_secret"); }🤖 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/zig/src/resource.zig` around lines 1480 - 1486, Update isSecretField to classify the exact key "attachment_lease" as secret, ensuring cloneRedacted removes echoed attachment leases from cloned error details while preserving existing secret-field handling.cmux-tui/spec/resource-operations-v2.json (1)
4040-4066: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winState how
acceptedrelates tooutcome.
ViewerResizeResultandBrowserViewerResizeResultnow carry bothacceptedand the newoutcomeenum. Neither type declares a relationship between the two fields.This file states such relationships everywhere else.
TerminalSnapshotat line 2134 declares "running is true exactly when lifecycle is running".BrowserSnapshotat line 2243 declares "loading is true exactly while status is starting".Without a constraint, a client cannot tell whether
accepted: true, outcome: "passive"is reachable, and SDK authors will pick one field arbitrarily as the source of truth. The server test incmux-tui/crates/cmux-tui-core/src/server.rsshowsapplied,passive, andsupersededfor the resize path, so the intended mapping exists in the implementation but not in the contract.♻️ Proposed constraint for the two fields
"outcome": { "required": true, "type": { "kind": "ref", "name": "ViewAttachmentOutcome" } } }, - "extra": false + "extra": false, + "constraints": [ + "accepted is true exactly when outcome is applied.", + "size reports the current canonical grid for every outcome." + ] },Also applies to: 4083-4101
🤖 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/spec/resource-operations-v2.json` around lines 4040 - 4066, Update ViewerResizeResult and BrowserViewerResizeResult to declare the relationship between accepted and outcome: accepted must be true exactly for applied or passive outcomes, and false for superseded. Use the schema’s existing constraint/description mechanism so clients can rely on outcome as the authoritative mapping.Source: Coding guidelines
cmux-tui/crates/cmux-tui-core/src/mux.rs (2)
5268-5294: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winEmit the frontend projection revision from
FrontendProjectionChanged.The delta value carries
projection_revision, but this path emitscommit.revisionfromResourcePatchCommitinstead. SinceFrontendProjectionChangedserializes that field forfrontend-projection-changed, consumers can compare a resource revision against the projection revision from the value. Useprojection_revisionhere for consistency.🤖 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 5268 - 5294, The frontend-projection-changed payload currently uses the resource commit revision instead of the projection revision. In the visible projection update flow, ensure the serialized revision field uses the computed projection_revision value, matching the revision carried by FrontendProjectionChanged, while leaving unrelated resource commit handling unchanged.
7279-7290: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winTrack browser runtime shutdown ownership separately from the shared slot.
browser_runtimeandbrowser_runtime_for_providerreuse different runtime profiles but write the sameself.browser_runtimeslot. A provider runtime can be replaced while a non-provider surface still holds itsArc<BrowserRuntime>, andshutdown/Droponly shut down the final slot value. Keep a separate list or reference map of browser-runtime-owned surfaces, or avoid replacing the active runtime until all surfaces that own it are detached.🤖 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 7279 - 7290, The browser_runtime method must track shutdown ownership independently for provider and non-provider runtimes instead of overwriting the shared self.browser_runtime slot. Update the runtime ownership/lifecycle handling used by browser_runtime, browser_runtime_for_provider, shutdown, and Drop so every runtime held by active surfaces remains tracked and is shut down only after all owning surfaces detach; do not replace an active runtime while it still has owned 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 @.github/workflows/cmux-tui-artifacts.yml:
- Around line 142-165: Update the legacy manifest generation in the Python block
to exclude files whose names end with .txt from the binaries map, matching the
upstream build_manifest behavior. Keep other cmux-mux-* regular files included.
In @.github/workflows/cmux-tui-build-package.yml:
- Line 241: Add a functional smoke check for the staged cmux-tui-hook executable
in the workflow after packaging, invoking it with an agent, native-event, and
--help arguments and asserting a zero exit status. Cover platform-specific
executable naming, including cmux-tui-hook.exe on Windows, while preserving the
existing cmux-tui executable and permission checks.
In `@cmux-tui/bindings/cpp/src/resource_models.cpp`:
- Around line 1031-1034: Update TerminalSnapshot decoding in
cmux-tui/bindings/cpp/src/resource_models.cpp:1031-1034 to derive tab_id from
the authoritative tab_ids list, or remove tab_id from TerminalSnapshot in
cmux-tui/bindings/cpp/include/cmux/resource.hpp so callers use tab_ids; do not
retain the raw legacy_tab_id assignment. In
cmux-tui/bindings/cpp/tests/test_resource.cpp:1489-1494, assert tab_id for
legacy_attached, legacy_detached, and consistent_dual to pin the selected
semantics.
In `@cmux-tui/bindings/cpp/tests/consumer/main.cpp`:
- Around line 102-103: Update the test around pane_split.to_params() to assert
that the serialized viewport_width preserves the assigned numeric value 0.5, in
addition to the existing correlation-key assertion. Use the returned
split_params representation and its established numeric assertion style.
In
`@cmux-tui/bindings/examples/python-dev-orchestrator/tests/test_orchestrator.py`:
- Around line 37-49: Update the seeded screen layout in the canonical snapshot
test to use the full layout document shape produced by _create_screen, including
version, screen_id, active_pane_id, zoomed_pane_id, and root containing the
existing leaf node. Preserve the current pane, tab, and focus values while
ensuring the snapshot is decodable by the SDK layout decoders.
In
`@cmux-tui/bindings/examples/typescript-browser-controller/test/controller.test.ts`:
- Around line 155-158: Update the mocked browser.attach response in the attach
test to remove attachment_lease and provide an expected attach item payload
using snapshot, frame, or state, matching the stream_open attach-item parser
shape.
In `@cmux-tui/bindings/go/operations.go`:
- Around line 1736-1749: Add attachment lease validation requiring 1–128 bytes
before sending viewer control requests: update Terminal.ResizeViewer and
Terminal.ReleaseViewer in cmux-tui/bindings/go/operations.go:1736-1749, plus
Browser.ResizeViewer and Browser.ReleaseViewer in
cmux-tui/bindings/go/operations.go:1954-1972, to return a local argument error
for invalid values. In cmux-tui/bindings/typescript/src/resources.ts:3713-3736
and :3894-3917, validate attachmentLease in the corresponding Terminal and
Browser resizeViewer/releaseViewer methods using hasUtf8ByteLength.
- Around line 944-947: In the *TerminalSnapshot handling, reuse the field map
created by the required-field validation instead of declaring terminalFields and
unmarshalling raw again. Hoist that parsed map to the shared scope while
preserving the existing required-field checks and downstream terminal
processing.
In `@cmux-tui/bindings/go/options.go`:
- Around line 128-162: Align journal filter validation and serialization
defaults across both SDKs using one authoritative journal specification. In
cmux-tui/bindings/go/options.go:128-162, either add the TypeScript SDK’s regex
field and case_sensitive defaults or remove those defaults from TypeScript so
the server decides. In cmux-tui/bindings/typescript/src/resources.ts:1071-1103,
add validation for the start enum and classes enum, and reject max_sensitivity
set to "secret"; ensure both implementations produce identical wire behavior and
local validation results.
In `@cmux-tui/bindings/go/raw/README.md`:
- Line 3: Move the Go SDK coverage statement from the default README into the
locale-specific documentation source, then update every supported locale or
translation catalog with the corresponding localized text. Keep the coverage
details consistent across all locales.
In `@cmux-tui/bindings/go/stream.go`:
- Around line 434-452: Remove the empty-string initializer from openedID in the
stream-opening logic, declaring it without an initial value while preserving the
assignments in both operation branches.
In `@cmux-tui/bindings/java/src/com/cmux/Options.java`:
- Around line 455-460: Update the PaneSplit constructor to validate
viewportWidth after applying opt(viewportWidth), rejecting NaN and positive or
negative infinity before the value can reach Pane.split request dispatch;
preserve valid finite values and the existing optional-value behavior.
In `@cmux-tui/bindings/java/src/com/cmux/raw/FrontendJournalEventResize.java`:
- Around line 114-148: Update the Java builder validation for
FrontendJournalEventResize so the uint16 fields cellHeight, cellWidth, cols, and
rows reject values outside 0–65535 before construction or serialization. Prefer
applying this consistently in the Java generator for all uint16 builder setters,
or enforce the same checks during build(), while leaving non-uint16 setters
unchanged.
In `@cmux-tui/bindings/java/src/com/cmux/raw/GetBrowserProviderRequest.java`:
- Around line 22-38: Update the generator template for field-free request types,
not the generated Java file: preserve the Wire.object(value, ...) validation
call without assigning its result, and change equals implementations to use a
type check without binding the unused that pattern variable. Apply these
template changes to all field-free generated types.
In `@cmux-tui/bindings/java/src/com/cmux/raw/RegisterBrowserProviderRequest.java`:
- Around line 79-80: Update the generated toString implementation for
RegisterBrowserProviderRequest so secret-marked fields such as bearer_token
render a redacted placeholder, while toWire continues returning the actual
value. Apply this in the generator/template responsible for producing Java
toString methods, preserving normal rendering for non-secret fields.
In
`@cmux-tui/bindings/java/src/com/cmux/raw/UnregisterBrowserProviderRequest.java`:
- Around line 22-37: Update the generator template for field-less Java models so
UnregisterBrowserProviderRequest.fromWire retains the Wire.object validation
call without assigning its result, and equals uses an instanceof type check
without binding an unused that variable; apply the same equals change to
JournalFrontendEventResult. Affected sites:
cmux-tui/bindings/java/src/com/cmux/raw/UnregisterBrowserProviderRequest.java:22-37
and
cmux-tui/bindings/java/src/com/cmux/raw/JournalFrontendEventResult.java:37-41;
fix the shared template rather than editing generated files directly.
In `@cmux-tui/bindings/java/src/com/cmux/Session.java`:
- Around line 479-585: Update decodeJournalRecord so the entire journal-record
decoding path after the envelope-cursor validation is enclosed by the existing
IllegalArgumentException-to-ProtocolError handling, including the initial
Wire.object and sequence parsing calls. Preserve the envelope cursor check, and
add coverage for a non-object record and an invalid sequence value asserting
both produce ProtocolError.
In `@cmux-tui/bindings/python/.cmux-resource-api.json`:
- Around line 178-210: Update the canonical operation registry/catalog mapping
to contain exactly the 11 session.journal.* operations defined by
resource-operations-v2.json, remove the extra descriptor entry, and recompute
catalog_sha256 in the SDK resource descriptor to match the canonical catalog.
In `@cmux-tui/bindings/python/cmux/options.py`:
- Around line 151-155: Update JournalSubjectFilter to validate during dataclass
construction and reject instances where both kind and id are None, raising the
established argument-validation exception. Preserve creation when at least one
field is provided and keep _journal_options serialization unchanged.
- Around line 164-170: Update the classes field in JournalFilter to use the
existing JournalClass alias from models.py instead of Sequence[str], preserving
its optional sequence shape and default value.
In `@cmux-tui/bindings/python/cmux/resources.py`:
- Around line 4554-4573: Add the shared _validate_uint64_decimal helper near
_required_decimal, matching the canonical uint64 rules used by _mutation. Invoke
it for non-None expected_projection_revision in the frontend projection mutation
before constructing the request payload, while preserving omission of the field
when unset.
In `@cmux-tui/bindings/python/tests/test_resource_api.py`:
- Around line 709-716: Add tests in the resource API coverage around the
existing connection/opened fixture to assert the returned stream exposes
attachment_lease. Also add rejection cases for missing, non-string, empty, and
over-128-UTF-8-byte leases, verifying each is rejected through the protocol
validation path used by terminal and browser attachment.
- Around line 1300-1316: Update the loop over responses and expected in the
terminal snapshot test to use strict zip pairing, enabling Ruff B905 and
ensuring mismatched fixture lengths fail instead of silently skipping entries.
In `@cmux-tui/bindings/rust/src/resource/mod.rs`:
- Around line 40-55: Update the public options re-export in the resource module
to include both JournalRegexFilter and JournalRegexField alongside
SessionJournalOptions, allowing callers to construct journal regex filters
through the exposed API.
In `@cmux-tui/bindings/typescript/src/resources.ts`:
- Around line 872-887: Update the frontend projection decoding near
requiredString in the returned object to reject empty values for frontend_id,
window_id, and generation, matching the Go SDK’s non-empty identifier
validation. Preserve the existing type validation and error-handling behavior
while ensuring each decoded identity string is non-empty before constructing the
frozen result.
- Around line 1415-1422: Rename the record parameter in
validateSessionJournalStreamItem to journalRecord and update its sequence access
accordingly, leaving the validation behavior unchanged and avoiding shadowing
the module-level record() helper.
In `@cmux-tui/bindings/zig/src/resource.zig`:
- Around line 19605-19699: Extend the test "session journal encodes filters and
decodes typed records" to add fake journal items covering strict cursor
validation: one missing the envelope cursor and one whose cursor revision
differs from the record sequence. Consume each item through the journal stream
and assert MissingStreamCursor and StreamCursorMismatch respectively, while
preserving the existing successful record assertions.
- Around line 8790-8795: Update the legacy_field alias consistency check to
require that when legacy_tab_id is present, tab_ids contains exactly one element
and that element equals legacy_tab_id. Preserve the existing null-versus-empty
validation for absent values, and return error.InvalidTerminalPlacement for
multi-tab or mismatched payloads.
In `@cmux-tui/crates/cmux-tui-core/src/browser_provider.rs`:
- Around line 7-11: Replace the derived Debug implementation for
BrowserProviderAuthentication with a manual implementation that formats only the
variant name returned by name(), never the Bearer token. Keep Clone, PartialEq,
and Eq derives intact, and ensure embedded Debug output through
BrowserProviderRegistration, BrowserProviderTargetLease, and
BrowserProviderSnapshot remains redacted.
- Around line 122-165: Update wait_for_revision_change to return an explicit
outcome enum distinguishing Changed, TimedOut, and Canceled, and adjust its
callers accordingly so only Changed represents a provider revision update. Add
an overall deadline to wait_for_target, use bounded waits, and check canceled
promptly so it returns None on cancellation or timeout instead of waiting
indefinitely.
In `@cmux-tui/crates/cmux-tui-core/src/browser.rs`:
- Around line 1150-1152: Update the user-facing error returned by
runtime_endpoint to remove the internal environment variable name and
provider-specific terminology; state the product-level cause and one actionable
recovery step instead. Keep CMUX_MUX_CDP_URL available only in logs or advanced
help output.
- Around line 2063-2079: Serialize provider bootstrap admission in
prepare_provider_bootstrap_attempt using a pre-taken atomic claim, following
prepare_provider_session_replacement, and pass that claim into
bootstrap_surface_sync. Ensure only the claimed bootstrap path performs
BrowserRuntime::register and attach_to_target; if the claim is lost or dropped,
exit before CDP setup and do not publish a session.
- Around line 4017-4019: Update the guard in maybe_nudge_stalled_external to
also return for BrowserSource::Provider, preventing activate_target from
switching provider-owned tabs during frame-stall nudging; leave the existing
BrowserSource::Launched behavior unchanged.
In `@cmux-tui/crates/cmux-tui-core/src/journal_checkpoint.rs`:
- Around line 364-371: Update the checkpoint record validation around
validate_resource_changes and cursor_accepts so cursor revision continuity is
checked before applying changes. Compare the checkpoint cursor’s revision with
record.previous_resource_revision, and return Ok(false) when they differ;
preserve the existing resource-change validation and cursor_accepts checks for
matching records.
In `@cmux-tui/crates/cmux-tui-core/src/journal_kernel.rs`:
- Around line 369-388: The read_after logic in journal_kernel.rs must not derive
the record offset by subtracting next_sequence from oldest_sequence, because
sequence gaps can exist. Replace the start calculation with sequence-based
filtering over state.records, using the first record whose record.sequence is at
least next_sequence (for example via partition_point), then preserve the
existing limit, scanned_through, and page construction behavior.
In `@cmux-tui/crates/cmux-tui-core/src/lib.rs`:
- Around line 82-89: Implement a manual redacting Debug for JournalHookExec that
never exposes the complete argv array, while preserving useful non-sensitive
fields. Update JournalHookManifest to use a manual Debug implementation as well,
ensuring its embedded exec data is redacted instead of deriving Debug through
journal_hooks::exec; remove conflicting derives and keep the public API behavior
consistent.
In `@cmux-tui/crates/cmux-tui-core/src/mux/public_projections.rs`:
- Line 122: Update the restore-coverage test for
Surface::exited_terminal_placeholder_with_terminal_public_id in
public_projections.rs so it runs on Windows as well as Unix. Remove the broad
#[cfg(unix)] from the projection test and condition only the Unix PTY/runtime
sidecar assertions, or provide a Windows/no-sidecar test variant while
preserving the same projection restore coverage.
In `@cmux-tui/crates/cmux-tui-core/src/resource_router.rs`:
- Around line 1486-1493: Update the HostLaunchFailure handling in the resource
error conversion to replace failure.message with safe product-facing text
derived from failure.kind, while retaining failure.kind.reason_code() in the
structured details. Preserve the original message only through sanitized
internal logging and ensure it is not included in the API error body.
In `@cmux-tui/crates/cmux-tui-core/src/terminal_host_runtime.rs`:
- Around line 7409-7489: The timeout test should avoid depending on
standard-library RecvTimeoutError wording. In
detach_fence_reports_a_delayed_receipt_after_output_as_a_failure, assert against
the crate-owned error prefix such as “did not acknowledge Detach” while
preserving the existing timeout/failure behavior.
In `@cmux-tui/crates/cmux-tui-core/src/unix_process_scope.rs`:
- Around line 622-631: Update the notification calls in register, finalize, and
the “found more owned processes” completion path to use
self.changed.notify_all() instead of notify_one(). Preserve the existing locking
and state-update behavior, ensuring both the tracker loop and all finalize
waiters are awakened for each relevant scope change.
- Around line 658-669: Reset the tracker’s started state when the thread running
ProcessScopeTracker::run exits, including after a panic or lock failure, so a
later register/ensure_started call can spawn it again. Update ensure_started and
the spawned thread cleanup using the existing started synchronization, while
preserving the current single-thread startup behavior.
- Around line 386-400: Add a Linux-only behavioral test using
final_scan_gate_for_test that creates a child within a configured
UnixProcessScope and verifies the process-group fence causes both setsid and
setpgid to fail with EPERM, while normal child creation remains successful.
Reuse the existing scope setup and child-process test utilities, and synchronize
through the gate so assertions exercise the final-scan fence path.
- Around line 946-985: Replace the repeated fixed-point loop in
include_lineage_matches with a bounded upward parent walk for each process,
using a parents map and the existing present and owned sets to resolve ownership
in one pass. Memoize or reuse resolved ownership where appropriate, ensure
cycles and missing parents terminate safely, and insert every process in an
owned chain into matches without changing root or known-identity handling.
- Around line 896-910: Replace the private proc_signal_with_audittoken call and
manual MacAuditToken construction in signal_process_with with a supported macOS
process-termination mechanism. Reuse the stored ProcessIdentity validation
before invoking kill or the selected public API, preserving the requirement that
cleanup targets only the matching process instance and never a reused PID;
update signal_process accordingly.
In `@cmux-tui/crates/cmux-tui-core/src/workspace_registry.rs`:
- Around line 3746-3755: Update the schema-14 migration’s frontend_projections
UPDATE in workspace_registry.rs to increment projection_revision alongside
schema_version and payload. In
cmux-tui/crates/cmux-tui-core/src/workspace_registry/tests.rs lines 4615-4623,
assert that projections[0].projection_revision is greater than the revision
recorded before migration.
- Around line 3676-3700: Update normalize_journal_multiview_schema to use the
shared resource_tabs_needs_multiview_normalization detector instead of
performing its local index scan, and remove the legacy_content_identity scanning
block. Preserve the existing migration call when the shared detector reports
normalization is required.
In `@cmux-tui/crates/cmux-tui-core/src/workspace_registry/tests.rs`:
- Around line 4254-4324: Add a test alongside
current_schema_canonicalizes_equivalent_formatted_browser_view_predicate_once
that seeds the database through create_resource_schema, opens the registry,
records the index definition and PRAGMA schema_version, then reopens it and
asserts the schema version remains unchanged. Ensure the seeded index uses the
CREATE UNIQUE INDEX IF NOT EXISTS path and verify no normalization rewrite
occurs on the second open.
In `@cmux-tui/crates/cmux-tui-core/tests/browser_runtime.rs`:
- Around line 1671-1773: The disconnect wait in
provider_disconnect_reconnects_without_closing_canonical_browser_topology
currently accepts both Starting and Failed statuses without documenting the
intended contract. Assert the single expected status after disconnect, or add a
concise comment explaining why both classifications are valid so future status
changes remain intentional.
- Around line 128-184: Extract the shared register-browser-provider payload and
write/read logic from browser_provider and update_browser_provider into one
helper that accepts a mutable writer, request id, endpoint, tab_id, and
target_id. Have browser_provider call it with id 1 while retaining its
connection setup and BufReader return, and have update_browser_provider call it
with id 2 using the existing reader stream. Preserve each function’s current
success assertion context while allowing callers to supply different request ids
for repeated updates.
In `@cmux-tui/crates/cmux-tui/assets/agent-hooks/amp.ts`:
- Around line 20-26: Update the child.stdin handling in the amp.ts plugin and
the corresponding hook in pi.ts by binding child.stdin locally, returning early
when it is null, and using the guarded binding for both error handling and
payload termination. Preserve the existing spawn configuration and behavior when
stdin is available.
In `@cmux-tui/crates/cmux-tui/assets/agent-hooks/hermes.py`:
- Around line 38-42: Replace the per-event threading in the hook delivery path
around _append with a bounded worker queue and a fixed number of reusable
workers. Define and enforce an explicit overflow/backpressure policy when the
queue is full, while preserving delivery of native_event and payload to _append
without creating a new thread for each event.
In `@cmux-tui/crates/cmux-tui/src/agent_browser_provider.rs`:
- Around line 475-485: Update direct_page_url to validate that the parsed CDP
URL uses an explicit loopback host in addition to the existing ws scheme check,
rejecting non-loopback endpoints before rewriting the path and returning the
URL. Preserve the current error-context style and URL construction for valid
loopback hosts.
- Around line 350-372: Update select_workspace_target to use only the
authoritative workspace resource_id, skipping workspaces where it is absent
instead of falling back to key or workspace_index. Restrict terminal_ids to
terminal_resource_id and terminal_id, removing content_resource_id so terminal
matching cannot select an unrelated pane.
In `@cmux-tui/crates/cmux-tui/src/agent_hook_install.rs`:
- Around line 728-757: Document at the Windows timeout cleanup around
job.terminate() and the subsequent reader-thread joins that termination must
occur before joining readers. State that JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE
closes the pipe handles, allowing read_hermes_output to finish and preserving
the command deadline; keep this ordering unchanged and do not add an early
return between termination and both joins.
- Around line 877-881: Limit Hermes stderr included in user-visible errors by
adding a shared reported_stderr helper near the Hermes command handling code
that trims and truncates output to 512 bytes, preserving valid UTF-8 boundaries
and appending an ellipsis when truncated. Update both anyhow::ensure! calls for
Hermes status failures to use this helper instead of embedding
String::from_utf8_lossy(...).trim() directly, while retaining the full stderr
only for internal diagnostics.
- Around line 882-888: Update the Hermes plugin parsing logic in the surrounding
function to require the decoded JSON value to be a top-level array, returning an
error with context when it is not. Preserve the existing search for the
“cmux-tui-journal” plugin name once the array is validated, rather than treating
unexpected payload shapes as disabled.
- Around line 1104-1110: Update read_json_object to stat the path and validate
its length against MAX_CONFIG_BYTES before calling fs::read, while retaining the
existing post-read check for growth between operations. Apply the same pre-read
size validation to the plugin reads in install_helper at the locations
corresponding to the plugin paths, preserving their existing error handling and
behavior.
In `@cmux-tui/crates/cmux-tui/src/app.rs`:
- Around line 10202-10246: Refactor frontend_presentation_unchanged to obtain
the current value from frontend_presentation_snapshot and compare it directly
with last_frontend_presentation using FrontendPresentationSnapshot’s PartialEq
implementation. Remove the duplicated inline focus, resize, and viewport
derivation while preserving the existing false result when no previous snapshot
exists.
In `@cmux-tui/crates/cmux-tui/src/bin/cmux-tui-hook.rs`:
- Around line 126-152: Bound retries in retry_until so immediately retryable
failures cannot spin or repeatedly spawn connector threads for the full timeout.
Add a reasonable maximum-attempt condition (or wait for a genuine
socket-readiness signal) while preserving successful results, fatal-error
propagation, and timeout context.
In `@cmux-tui/crates/cmux-tui/src/cli/wire.rs`:
- Around line 174-184: Update the unsupported-capability error message in the
journal operation handling around required_server_capability to describe
unsupported journal operations generally, rather than only journal
subscriptions. Preserve the existing capability, error code, details,
retryability, and restart guidance.
In `@cmux-tui/crates/cmux-tui/src/main.rs`:
- Around line 1160-1163: Update the __agent-browser-provider entrypoint in main
so it invokes harden_provider_secret_process() and
discard_provider_secret_environment() before agent_browser_provider::run().
Preserve the existing early-exit dispatch while ensuring both provider secrets
are hardened and removed from the process environment before browser resolution.
In `@cmux-tui/crates/cmux-tui/tests/resource_cli_v2.rs`:
- Around line 1052-1065: Update the SIGINT timeout assertions in the journal
subscriber test around child.wait_timeout and the elapsed check to use a bounded
one-second deadline instead of 100 milliseconds. Preserve the existing timeout
panic, status.success assertion, and empty-stderr assertion.
In `@cmux-tui/crates/cmux-tui/tests/terminal_host_recovery.rs`:
- Around line 405-408: Update the journal-order assertion in the terminal
recovery test to first require that all expected records—workspace.run,
terminal.output, and terminal.exited—are present, then compare their unwrapped
sequence values. Preserve the existing diagnostic message while preventing
missing records from passing through Option ordering.
In `@cmux-tui/docs/browser-panes.md`:
- Line 53: Update the browser endpoint security documentation to remove the
claim that the unauthenticated loopback socket is protected by local process or
user boundaries. State that any local process able to reach the endpoint can
control the page, and document the local-client trust requirement unless
Unix-socket filesystem permissions or endpoint authentication is implemented.
In `@cmux-tui/spec/cli.md`:
- Around line 176-180: Remove the later duplicate `agent list|report` entry from
the command list, keeping the earlier complete `agent` command surface that
includes `hook emit` and `hook install|uninstall|status`.
In `@cmux-tui/spec/commands.md`:
- Line 1153: Update the new-browser-tab error table to match the no-launch
contract: remove “Chrome launch fails” from the standard error condition, or
explicitly limit it to failures on the development-only configured CDP URL path.
Keep connection, target creation, attachment, and setup failures covered.
- Around line 409-415: Escape the pipe characters in the inline-code values for
the authentication and bearer_token rows of the parameters table, including
`"none"|"bearer"` and `string|null`, so Markdown treats them as cell content and
preserves the constraint column.
In `@cmux-tui/spec/native-frontend.md`:
- Around line 68-73: Update the remote-creation state description to match the
CreationResolution.state wire contract: clarify that prepared is internal and
never sent, and map executing to pending only for the active generation during
wait; alternatively, replace those internal state names with their wire values
while preserving the documented created, not_applied, and indeterminate states.
In `@cmux-tui/spec/resource-operations-v2.json`:
- Around line 9140-9146: Disambiguate the cursor semantics used by
session.journal.subscribe: update the shared Cursor type definition to
explicitly document or model that generation is the immutable session ID and
revision is the journal sequence, ensuring consumers distinguish this from
SessionSnapshot.generation; alternatively introduce and use a distinct
JournalCursor type throughout the journal subscription contract. Keep a single
authoritative definition so cursor generation-change and gap-recovery logic
cannot compare incompatible values.
- Around line 915-1074: Update SessionJournalRecord so authority, causation_id,
and correlation_id are explicitly marked sensitive using the schema’s existing
sensitive-field convention. Preserve their current types, nullability, and
required status; only add the sensitivity metadata needed for generated SDK
Debug, toString, and log redaction.
- Around line 643-650: Update the JournalHookRetry.backoff_ms primitive
constraint to require a positive minimum delay, preventing zero-delay retries
while retaining the existing maximum and max_attempts constraints.
- Around line 8909-8944: Add "mutation.indeterminate" to the errors arrays for
session.journal.append, session.journal.checkpoint.create,
session.journal.hook.put, session.journal.producer.put, and
session.journal.segment.seal, preserving all existing errors and ordering
conventions.
- Around line 1113-1134: Document the attachment-lease contract in
cmux-tui/spec/resource-operations-v2.json: at 1113-1134, mark
ViewAttachmentStreamOpened as the browser and terminal attach result with since,
capability, and migration details; update browser.viewer.release at 5891 and
6664-6679, browser.viewer.resize at 6714-6722, terminal.viewer.release at 10581
and 11613-11628, and terminal.viewer.resize at 11663-11671 with the omission
behavior for attachment_lease. At 7086-7123, document and gate the required
lease behavior behind the view-attachment-lease-v1 capability so non-negotiating
clients retain the prior validation.invalid behavior.
In `@cmux-tui/spec/sdk-schema.json`:
- Around line 86-140: Update FrontendJournalEvent’s focus variant to require a
non-null pane_id for target:pane and enforce a contiguous ancestor identifier
chain; update its resize variant to add minimum: 1 constraints to cols, rows,
cell_width, and cell_height, while preserving the existing 1–10000 bounds for
cols and rows. Apply the changes at cmux-tui/spec/sdk-schema.json lines 86-140
and 177-208.
- Around line 9040-9056: Update the registration request schema around
authentication and bearer_token to add a constraint requiring bearer_token when
authentication is "bearer" and forbidding it when authentication is "none"; also
mark the token as requiring redaction in generated request representations such
as Debug or toString, rather than relying only on constraints that prevent
returning it.
- Around line 9057-9067: Add an explicit max_items bound to the required,
non-nullable targets array in the BrowserProviderTarget schema, following the
established bounded-array convention and choosing an appropriate protocol limit.
Keep the existing item reference and required/nullable settings unchanged.
- Around line 264-271: Rename the viewport variant’s uint64 field from target to
target_offset in the schema, the exported FrontendJournalEvent runtime type, and
every generated binding. Keep the focus variant’s FrontendFocusTarget target
field unchanged, and update all affected serialization, deserialization, and
field references to use target_offset consistently.
In `@cmux-tui/spec/session-journal.md`:
- Around line 398-401: Update the journal timeout description in the relevant
helper/receipt documentation to remove the unsupported two-second
commit-admission window, unless the implementation exposes a corresponding
constant that can be referenced directly. Keep the documented four-second helper
deadline and five-second installed-provider timeout aligned with the actual hook
behavior.
In `@cmux-tui/spec/terminal-host.md`:
- Around line 383-409: Update the discovery record documentation to state that
the host currently emits record_version 4, versions 1 through 3 remain
adoptable, and the source-ordered Detach/DetachAck shutdown fence is supported
only for record_version 4 or newer.
---
Outside diff comments:
In `@cmux-tui/bindings/zig/src/resource.zig`:
- Around line 1480-1486: Update isSecretField to classify the exact key
"attachment_lease" as secret, ensuring cloneRedacted removes echoed attachment
leases from cloned error details while preserving existing secret-field
handling.
In `@cmux-tui/crates/cmux-tui-core/src/mux.rs`:
- Around line 5268-5294: The frontend-projection-changed payload currently uses
the resource commit revision instead of the projection revision. In the visible
projection update flow, ensure the serialized revision field uses the computed
projection_revision value, matching the revision carried by
FrontendProjectionChanged, while leaving unrelated resource commit handling
unchanged.
- Around line 7279-7290: The browser_runtime method must track shutdown
ownership independently for provider and non-provider runtimes instead of
overwriting the shared self.browser_runtime slot. Update the runtime
ownership/lifecycle handling used by browser_runtime,
browser_runtime_for_provider, shutdown, and Drop so every runtime held by active
surfaces remains tracked and is shut down only after all owning surfaces detach;
do not replace an active runtime while it still has owned surfaces.
In `@cmux-tui/crates/cmux-tui-core/src/workspace_registry/effect_store.rs`:
- Around line 180-220: Extract the duplicated interrupted-effect predicate from
the receipt recovery flow into a single SQL constant, such as
UNCORRELATED_EXECUTING_EFFECT_SQL, and interpolate or otherwise reuse it in both
the SELECT that builds interrupted and the UPDATE that marks receipts
indeterminate. Keep both statements semantically identical so every updated
receipt receives the corresponding journal record.
In `@cmux-tui/crates/cmux-tui-core/src/workspace_registry/terminal_exit_store.rs`:
- Around line 146-153: Remove the duplicate topology handling in the transaction
by deleting one of the identical if-let blocks that calls apply_resource_patch
with topology and sqlite_resource_revision. Keep exactly one application of the
patch for each terminal exit.
In `@cmux-tui/spec/resource-operations-v2.json`:
- Around line 4040-4066: Update ViewerResizeResult and BrowserViewerResizeResult
to declare the relationship between accepted and outcome: accepted must be true
exactly for applied or passive outcomes, and false for superseded. Use the
schema’s existing constraint/description mechanism so clients can rely on
outcome as the authoritative mapping.
🪄 Autofix
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: a8f13f87-deb9-46f2-bbcf-f2d75a734d1f
⛔ Files ignored due to path filters (27)
cmux-tui/Cargo.lockis excluded by!**/*.lockcmux-tui/bindings/cpp/include/cmux/raw/generated/commands.hppis excluded by!**/generated/**cmux-tui/bindings/cpp/include/cmux/raw/generated/models.hppis excluded by!**/generated/**cmux-tui/bindings/cpp/src/raw/generated/protocol.cppis excluded by!**/generated/**cmux-tui/bindings/python/cmux/raw/_generated/.cmux-sdk-manifest.jsonis excluded by!**/_generated/**cmux-tui/bindings/python/cmux/raw/_generated/_schema.pyis excluded by!**/_generated/**cmux-tui/bindings/python/cmux/raw/_generated/client.pyis excluded by!**/_generated/**cmux-tui/bindings/python/cmux/raw/_generated/codec.pyis excluded by!**/_generated/**cmux-tui/bindings/python/cmux/raw/_generated/metadata.pyis excluded by!**/_generated/**cmux-tui/bindings/python/cmux/raw/_generated/models.pyis excluded by!**/_generated/**cmux-tui/bindings/rust/src/generated/.cmux-sdk-manifest.jsonis excluded by!**/generated/**cmux-tui/bindings/rust/src/generated/commands.rsis excluded by!**/generated/**cmux-tui/bindings/rust/src/generated/events.rsis excluded by!**/generated/**cmux-tui/bindings/rust/src/generated/metadata.rsis excluded by!**/generated/**cmux-tui/bindings/rust/src/generated/mod.rsis excluded by!**/generated/**cmux-tui/bindings/rust/src/generated/types.rsis excluded by!**/generated/**cmux-tui/bindings/typescript/src/raw/generated/.cmux-sdk-manifest.jsonis excluded by!**/generated/**cmux-tui/bindings/typescript/src/raw/generated/commands.tsis excluded by!**/generated/**cmux-tui/bindings/typescript/src/raw/generated/events.tsis excluded by!**/generated/**cmux-tui/bindings/typescript/src/raw/generated/index.tsis excluded by!**/generated/**cmux-tui/bindings/typescript/src/raw/generated/metadata.tsis excluded by!**/generated/**cmux-tui/bindings/typescript/src/raw/generated/types.tsis excluded by!**/generated/**cmux-tui/bindings/zig/src/raw/generated/.cmux-sdk-manifest.jsonis excluded by!**/generated/**cmux-tui/bindings/zig/src/raw/generated/presence_test.zigis excluded by!**/generated/**cmux-tui/bindings/zig/src/raw/generated/protocol.zigis excluded by!**/generated/**cmux-tui/dist/scripts/package_npm.pyis excluded by!**/dist/**cmux-tui/dist/scripts/package_pypi.pyis excluded by!**/dist/**
📒 Files selected for processing (196)
.github/workflows/cmux-tui-artifacts.yml.github/workflows/cmux-tui-build-package.yml.github/workflows/cmux-tui.ymlcmux-tui/.gitignorecmux-tui/Cargo.tomlcmux-tui/README.mdcmux-tui/bindings/ERGONOMICS.mdcmux-tui/bindings/conformance/runner.pycmux-tui/bindings/cpp/.cmux-resource-api.jsoncmux-tui/bindings/cpp/.cmux-sdk-manifest.jsoncmux-tui/bindings/cpp/include/cmux/resource.hppcmux-tui/bindings/cpp/src/resource.cppcmux-tui/bindings/cpp/src/resource_models.cppcmux-tui/bindings/cpp/tests/consumer/main.cppcmux-tui/bindings/cpp/tests/test_generated.cppcmux-tui/bindings/cpp/tests/test_resource.cppcmux-tui/bindings/examples/cpp-terminal-frontend/tests/frontend_test.cppcmux-tui/bindings/examples/python-agent-watchdog/watchdog.pycmux-tui/bindings/examples/python-dev-orchestrator/fake_cmux_server.pycmux-tui/bindings/examples/python-dev-orchestrator/tests/test_orchestrator.pycmux-tui/bindings/examples/rust-sidebar-monitor/tests/integration.rscmux-tui/bindings/examples/typescript-browser-controller/test/controller.test.tscmux-tui/bindings/examples/zig-session-supervisor/FRICTION.mdcmux-tui/bindings/go/.cmux-resource-api.jsoncmux-tui/bindings/go/client.gocmux-tui/bindings/go/internal/wirev2/operations.gocmux-tui/bindings/go/operations.gocmux-tui/bindings/go/options.gocmux-tui/bindings/go/raw/.cmux-sdk-manifest.jsoncmux-tui/bindings/go/raw/README.mdcmux-tui/bindings/go/raw/client_test.gocmux-tui/bindings/go/raw/generated_commands.gocmux-tui/bindings/go/raw/generated_events.gocmux-tui/bindings/go/raw/generated_metadata.gocmux-tui/bindings/go/raw/generated_presence_test.gocmux-tui/bindings/go/raw/generated_types.gocmux-tui/bindings/go/resource_api_test.gocmux-tui/bindings/go/resources.gocmux-tui/bindings/go/stream.gocmux-tui/bindings/go/values.gocmux-tui/bindings/java/.cmux-resource-api.jsoncmux-tui/bindings/java/src/com/cmux/Browser.javacmux-tui/bindings/java/src/com/cmux/Client.javacmux-tui/bindings/java/src/com/cmux/FrontendProjection.javacmux-tui/bindings/java/src/com/cmux/Options.javacmux-tui/bindings/java/src/com/cmux/Pane.javacmux-tui/bindings/java/src/com/cmux/ResourceStream.javacmux-tui/bindings/java/src/com/cmux/Results.javacmux-tui/bindings/java/src/com/cmux/Session.javacmux-tui/bindings/java/src/com/cmux/SessionJournalRecord.javacmux-tui/bindings/java/src/com/cmux/Snapshots.javacmux-tui/bindings/java/src/com/cmux/Terminal.javacmux-tui/bindings/java/src/com/cmux/internal/Operations.javacmux-tui/bindings/java/src/com/cmux/internal/Wire.javacmux-tui/bindings/java/src/com/cmux/raw/.cmux-sdk-manifest.jsoncmux-tui/bindings/java/src/com/cmux/raw/BrowserProviderAuthentication.javacmux-tui/bindings/java/src/com/cmux/raw/BrowserProviderSnapshot.javacmux-tui/bindings/java/src/com/cmux/raw/BrowserProviderTarget.javacmux-tui/bindings/java/src/com/cmux/raw/BrowserProviderUnregisterResult.javacmux-tui/bindings/java/src/com/cmux/raw/Commands.javacmux-tui/bindings/java/src/com/cmux/raw/CreateSurfaceWithReceiptRequest.javacmux-tui/bindings/java/src/com/cmux/raw/CreateSurfaceWithReceiptRequestOperation.javacmux-tui/bindings/java/src/com/cmux/raw/FrontendFocusTarget.javacmux-tui/bindings/java/src/com/cmux/raw/FrontendJournalEvent.javacmux-tui/bindings/java/src/com/cmux/raw/FrontendJournalEventFocus.javacmux-tui/bindings/java/src/com/cmux/raw/FrontendJournalEventResize.javacmux-tui/bindings/java/src/com/cmux/raw/FrontendJournalEventViewport.javacmux-tui/bindings/java/src/com/cmux/raw/GeneratedCmuxClient.javacmux-tui/bindings/java/src/com/cmux/raw/GetBrowserProviderRequest.javacmux-tui/bindings/java/src/com/cmux/raw/JournalFrontendEventRequest.javacmux-tui/bindings/java/src/com/cmux/raw/JournalFrontendEventResult.javacmux-tui/bindings/java/src/com/cmux/raw/Protocol.javacmux-tui/bindings/java/src/com/cmux/raw/ReceiptedSurfaceResult.javacmux-tui/bindings/java/src/com/cmux/raw/RegisterBrowserProviderRequest.javacmux-tui/bindings/java/src/com/cmux/raw/UnregisterBrowserProviderRequest.javacmux-tui/bindings/java/src/com/cmux/raw/ViewReleaseResult.javacmux-tui/bindings/java/src/com/cmux/raw/ViewResizeResult.javacmux-tui/bindings/java/tests/com/cmux/BrowserPointerFrameTest.javacmux-tui/bindings/java/tests/com/cmux/ResourceApiTest.javacmux-tui/bindings/java/tests/com/cmux/raw/GeneratedCoverageTest.javacmux-tui/bindings/python/.cmux-resource-api.jsoncmux-tui/bindings/python/cmux/_operations.pycmux-tui/bindings/python/cmux/_protocol.pycmux-tui/bindings/python/cmux/models.pycmux-tui/bindings/python/cmux/options.pycmux-tui/bindings/python/cmux/resources.pycmux-tui/bindings/python/tests/test_protocol.pycmux-tui/bindings/python/tests/test_resource_api.pycmux-tui/bindings/rust-sidebar/tests/runtime.rscmux-tui/bindings/rust/.cmux-resource-api.jsoncmux-tui/bindings/rust/src/codec.rscmux-tui/bindings/rust/src/resource/client.rscmux-tui/bindings/rust/src/resource/handles.rscmux-tui/bindings/rust/src/resource/mod.rscmux-tui/bindings/rust/src/resource/model.rscmux-tui/bindings/rust/src/resource/ops.rscmux-tui/bindings/rust/src/resource/options.rscmux-tui/bindings/rust/src/resource/stream.rscmux-tui/bindings/rust/src/resource/typed_stream.rscmux-tui/bindings/rust/src/resource/wire.rscmux-tui/bindings/rust/tests/clean_consumer.rscmux-tui/bindings/rust/tests/mock_server.rscmux-tui/bindings/rust/tests/operation_reachability.rscmux-tui/bindings/typescript/.cmux-resource-api.jsoncmux-tui/bindings/typescript/src/internal/operations.tscmux-tui/bindings/typescript/src/models.tscmux-tui/bindings/typescript/src/options.tscmux-tui/bindings/typescript/src/resource-protocol.tscmux-tui/bindings/typescript/src/resources.tscmux-tui/bindings/typescript/test/generated.test.tscmux-tui/bindings/typescript/test/resource-api.test.tscmux-tui/bindings/zig/.cmux-resource-api.jsoncmux-tui/bindings/zig/examples/watch.zigcmux-tui/bindings/zig/src/cmux.zigcmux-tui/bindings/zig/src/raw.zigcmux-tui/bindings/zig/src/resource.zigcmux-tui/crates/cmux-tui-cdp/src/client.rscmux-tui/crates/cmux-tui-core/Cargo.tomlcmux-tui/crates/cmux-tui-core/src/agent_hooks.rscmux-tui/crates/cmux-tui-core/src/browser.rscmux-tui/crates/cmux-tui-core/src/browser_provider.rscmux-tui/crates/cmux-tui-core/src/journal_checkpoint.rscmux-tui/crates/cmux-tui-core/src/journal_hooks.rscmux-tui/crates/cmux-tui-core/src/journal_ingress.rscmux-tui/crates/cmux-tui-core/src/journal_kernel.rscmux-tui/crates/cmux-tui-core/src/lib.rscmux-tui/crates/cmux-tui-core/src/mux.rscmux-tui/crates/cmux-tui-core/src/mux/public_projections.rscmux-tui/crates/cmux-tui-core/src/mux/resource_content.rscmux-tui/crates/cmux-tui-core/src/mux/resource_topology.rscmux-tui/crates/cmux-tui-core/src/resource.rscmux-tui/crates/cmux-tui-core/src/resource_api.rscmux-tui/crates/cmux-tui-core/src/resource_router.rscmux-tui/crates/cmux-tui-core/src/resource_router/auxiliary.rscmux-tui/crates/cmux-tui-core/src/resource_router/content.rscmux-tui/crates/cmux-tui-core/src/resource_router/topology.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/src/unix_process_scope.rscmux-tui/crates/cmux-tui-core/src/workspace_registry.rscmux-tui/crates/cmux-tui-core/src/workspace_registry/effect_store.rscmux-tui/crates/cmux-tui-core/src/workspace_registry/journal_extensions.rscmux-tui/crates/cmux-tui-core/src/workspace_registry/public_projection_store.rscmux-tui/crates/cmux-tui-core/src/workspace_registry/resource_store.rscmux-tui/crates/cmux-tui-core/src/workspace_registry/session_journal.rscmux-tui/crates/cmux-tui-core/src/workspace_registry/terminal_exit_store.rscmux-tui/crates/cmux-tui-core/src/workspace_registry/tests.rscmux-tui/crates/cmux-tui-core/tests/browser_runtime.rscmux-tui/crates/cmux-tui/Cargo.tomlcmux-tui/crates/cmux-tui/assets/agent-hooks/amp.tscmux-tui/crates/cmux-tui/assets/agent-hooks/hermes.pycmux-tui/crates/cmux-tui/assets/agent-hooks/hermes.yamlcmux-tui/crates/cmux-tui/assets/agent-hooks/opencode.jscmux-tui/crates/cmux-tui/assets/agent-hooks/pi.tscmux-tui/crates/cmux-tui/src/agent_browser_provider.rscmux-tui/crates/cmux-tui/src/agent_hook_install.rscmux-tui/crates/cmux-tui/src/app.rscmux-tui/crates/cmux-tui/src/bin/cmux-tui-hook.rscmux-tui/crates/cmux-tui/src/cli.rscmux-tui/crates/cmux-tui/src/cli/command.rscmux-tui/crates/cmux-tui/src/cli/wire.rscmux-tui/crates/cmux-tui/src/main.rscmux-tui/crates/cmux-tui/src/pty_input.rscmux-tui/crates/cmux-tui/src/session/mod.rscmux-tui/crates/cmux-tui/src/session/remote.rscmux-tui/crates/cmux-tui/src/session/tree.rscmux-tui/crates/cmux-tui/src/sidebar_projection.rscmux-tui/crates/cmux-tui/src/ui/graphics_writer.rscmux-tui/crates/cmux-tui/tests/cli.rscmux-tui/crates/cmux-tui/tests/resource_cli_v2.rscmux-tui/crates/cmux-tui/tests/terminal_host_recovery.rscmux-tui/crates/ghostty-vt-sys/build.rscmux-tui/docs/browser-panes.mdcmux-tui/docs/configuration.mdcmux-tui/docs/protocol.mdcmux-tui/scripts/check-resource-api-boundary.pycmux-tui/scripts/check-sdk-schema.pycmux-tui/scripts/test_check_resource_api_boundary.pycmux-tui/scripts/test_check_sdk_schema.pycmux-tui/spec/README.mdcmux-tui/spec/bindings.mdcmux-tui/spec/cli.mdcmux-tui/spec/commands.mdcmux-tui/spec/frontends.mdcmux-tui/spec/inventory.jsoncmux-tui/spec/native-frontend.mdcmux-tui/spec/programmability.mdcmux-tui/spec/resource-api-v2.jsoncmux-tui/spec/resource-api-v2.mdcmux-tui/spec/resource-operations-v2.jsoncmux-tui/spec/resource-operations-v2.mdcmux-tui/spec/sdk-schema.jsoncmux-tui/spec/session-journal.mdcmux-tui/spec/terminal-host.md
Comment on lines
1031
to
+1034
| return { | ||
| id_value<TerminalId>( | ||
| field(object, "id", "terminal"), "terminal id"), | ||
| std::move(tab_id), | ||
| legacy_tab_id, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
TerminalSnapshot::tab_id is left as a legacy echo, and no test pins its meaning. The decoder assigns the raw legacy field instead of deriving it from the authoritative tab_ids list, so the field is empty on a current server and populated only on a legacy server. The test suite asserts tab_ids for every shape but never asserts tab_id, so the divergence is invisible. The TypeScript binding exposes only tabIds, which is the intended end state.
cmux-tui/bindings/cpp/src/resource_models.cpp#L1031-L1034: replace thelegacy_tab_idinitializer with a value derived fromtab_ids.front(), or removetab_idfromTerminalSnapshotincmux-tui/bindings/cpp/include/cmux/resource.hppso consumers must readtab_ids.cmux-tui/bindings/cpp/tests/test_resource.cpp#L1489-L1494: assert the resultingtab_idfor thelegacy_attached,legacy_detached, andconsistent_dualcases, so the chosen semantics stay pinned.
📍 Affects 2 files
cmux-tui/bindings/cpp/src/resource_models.cpp#L1031-L1034(this comment)cmux-tui/bindings/cpp/tests/test_resource.cpp#L1489-L1494
🤖 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/cpp/src/resource_models.cpp` around lines 1031 - 1034,
Update TerminalSnapshot decoding in
cmux-tui/bindings/cpp/src/resource_models.cpp:1031-1034 to derive tab_id from
the authoritative tab_ids list, or remove tab_id from TerminalSnapshot in
cmux-tui/bindings/cpp/include/cmux/resource.hpp so callers use tab_ids; do not
retain the raw legacy_tab_id assignment. In
cmux-tui/bindings/cpp/tests/test_resource.cpp:1489-1494, assert tab_id for
legacy_attached, legacy_detached, and consistent_dual to pin the selected
semantics.
Source: Coding guidelines
Comment on lines
+102
to
103
| pane_split.viewport_width = 0.5; | ||
| auto split_params = pane_split.to_params(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the serialized viewport_width value.
Line [102] sets pane_split.viewport_width, but the test only checks the correlation key after to_params(). If serialization drops or mis-encodes viewport_width, this test still passes. Add an assertion for the encoded numeric value.
🤖 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/cpp/tests/consumer/main.cpp` around lines 102 - 103, Update
the test around pane_split.to_params() to assert that the serialized
viewport_width preserves the assigned numeric value 0.5, in addition to the
existing correlation-key assertion. Use the returned split_params representation
and its established numeric assertion style.
Comment on lines
+37
to
+49
| server.screens[screen_id] = { | ||
| "id": screen_id, | ||
| "workspace_id": workspace_id, | ||
| "name": None, | ||
| "index": 0, | ||
| "focused": True, | ||
| "layout": { | ||
| "kind": "leaf", | ||
| "pane_id": pane_id, | ||
| "tab_ids": [tab_id], | ||
| "active_tab_id": tab_id, | ||
| }, | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
The seeded screen carries a bare layout leaf, not a layout document.
_create_screen in cmux-tui/bindings/examples/python-dev-orchestrator/fake_cmux_server.py builds layout as a document with version, screen_id, active_pane_id, zoomed_pane_id, and root. This fixture assigns the leaf node directly.
_full_snapshot only deep-copies the value, so the current assertions still pass. The emitted snapshot is therefore not decodable by the SDK layout decoders, which require the document fields. The test name states the snapshot is canonical, so the fixture should match the shape the fake produces elsewhere.
🐛 Proposed fix to seed a valid layout document
"focused": True,
"layout": {
- "kind": "leaf",
- "pane_id": pane_id,
- "tab_ids": [tab_id],
- "active_tab_id": tab_id,
+ "version": 1,
+ "screen_id": screen_id,
+ "active_pane_id": pane_id,
+ "zoomed_pane_id": None,
+ "root": {
+ "kind": "leaf",
+ "pane_id": pane_id,
+ "tab_ids": [tab_id],
+ "active_tab_id": tab_id,
+ },
},
}📝 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.
Suggested change
| server.screens[screen_id] = { | |
| "id": screen_id, | |
| "workspace_id": workspace_id, | |
| "name": None, | |
| "index": 0, | |
| "focused": True, | |
| "layout": { | |
| "kind": "leaf", | |
| "pane_id": pane_id, | |
| "tab_ids": [tab_id], | |
| "active_tab_id": tab_id, | |
| }, | |
| } | |
| server.screens[screen_id] = { | |
| "id": screen_id, | |
| "workspace_id": workspace_id, | |
| "name": None, | |
| "index": 0, | |
| "focused": True, | |
| "layout": { | |
| "version": 1, | |
| "screen_id": screen_id, | |
| "active_pane_id": pane_id, | |
| "zoomed_pane_id": None, | |
| "root": { | |
| "kind": "leaf", | |
| "pane_id": pane_id, | |
| "tab_ids": [tab_id], | |
| "active_tab_id": tab_id, | |
| }, | |
| }, | |
| } |
🤖 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/examples/python-dev-orchestrator/tests/test_orchestrator.py`
around lines 37 - 49, Update the seeded screen layout in the canonical snapshot
test to use the full layout document shape produced by _create_screen, including
version, screen_id, active_pane_id, zoomed_pane_id, and root containing the
existing leaf node. Preserve the current pane, tab, and focus values while
ensuring the snapshot is decodable by the SDK layout decoders.
Comment on lines
+944
to
+947
| var terminalFields map[string]json.RawMessage | ||
| if err := json.Unmarshal(raw, &terminalFields); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
Reuse the already-parsed field map instead of unmarshalling raw twice.
The required-field check at lines 877-887 already unmarshals raw into fields for *TerminalSnapshot, because its required list is non-empty. This block parses the same bytes again. Hoist the parsed map so each snapshot is decoded once. This matters when a full session snapshot validates many terminals.
🤖 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/operations.go` around lines 944 - 947, In the
*TerminalSnapshot handling, reuse the field map created by the required-field
validation instead of declaring terminalFields and unmarshalling raw again.
Hoist that parsed map to the shared scope while preserving the existing
required-field checks and downstream terminal processing.
Comment on lines
+1736
to
+1749
| input["attachment_lease"] = options.AttachmentLease | ||
| input[wirev2.FieldCols] = options.Cols | ||
| input[wirev2.FieldRows] = options.Rows | ||
| merge(input, options.Extra) | ||
| return readValue[ViewerResizeResult]( | ||
| ctx, t.client, wirev2.TerminalViewerResize, input, "viewer resize result", | ||
| ) | ||
| } | ||
| func (t *Terminal) ReleaseViewer(ctx context.Context, options TerminalViewerReleaseOptions) (EmptyResult, error) { | ||
| func (t *Terminal) ReleaseViewer(ctx context.Context, options TerminalViewerReleaseOptions) (ViewerReleaseResult, error) { | ||
| input := t.route.params() | ||
| input["attachment_lease"] = options.AttachmentLease | ||
| merge(input, options.Extra) | ||
| return readValue[EmptyResult]( | ||
| ctx, t.client, wirev2.TerminalViewerRelease, input, "empty result", | ||
| return readValue[ViewerReleaseResult]( | ||
| ctx, t.client, wirev2.TerminalViewerRelease, input, "viewer release result", |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Only the C++ SDK bounds the attachment lease before it sends a viewer control request. cmux-tui/bindings/cpp/src/resource.cpp lines 2699-2703 reject a lease that is empty or longer than 128 bytes. The Go and TypeScript SDKs forward the caller value unchecked, so an empty lease reaches the server and returns a protocol error instead of a local argument error.
cmux-tui/bindings/go/operations.go#L1736-L1749: add a 1 to 128 byte check inTerminal.ResizeViewerandTerminal.ReleaseViewer, and apply the same check inBrowser.ResizeViewerandBrowser.ReleaseViewerat lines 1954-1972.cmux-tui/bindings/typescript/src/resources.ts#L3713-L3736: validateattachmentLeaseinTerminal.resizeViewerandTerminal.releaseViewerwith the existinghasUtf8ByteLengthhelper.cmux-tui/bindings/typescript/src/resources.ts#L3894-L3917: validateattachmentLeaseinBrowser.resizeViewerandBrowser.releaseViewerwith the same helper.
📍 Affects 2 files
cmux-tui/bindings/go/operations.go#L1736-L1749(this comment)cmux-tui/bindings/typescript/src/resources.ts#L3713-L3736cmux-tui/bindings/typescript/src/resources.ts#L3894-L3917
🤖 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/operations.go` around lines 1736 - 1749, Add attachment
lease validation requiring 1–128 bytes before sending viewer control requests:
update Terminal.ResizeViewer and Terminal.ReleaseViewer in
cmux-tui/bindings/go/operations.go:1736-1749, plus Browser.ResizeViewer and
Browser.ReleaseViewer in cmux-tui/bindings/go/operations.go:1954-1972, to return
a local argument error for invalid values. In
cmux-tui/bindings/typescript/src/resources.ts:3713-3736 and :3894-3917, validate
attachmentLease in the corresponding Terminal and Browser
resizeViewer/releaseViewer methods using hasUtf8ByteLength.
Comment on lines
+4254
to
+4324
| #[test] | ||
| fn current_schema_canonicalizes_equivalent_formatted_browser_view_predicate_once() { | ||
| let root = temp_root("current-schema-formatted-browser-predicate"); | ||
| let database = root.join(session_storage_component("session")).join(WORKSPACE_REGISTRY_FILE); | ||
| { | ||
| let registry = WorkspaceRegistry::open(&root, "session").unwrap(); | ||
| drop(registry); | ||
| } | ||
| let formatted = Connection::open(&database).unwrap(); | ||
| formatted | ||
| .execute_batch( | ||
| "DROP INDEX live_resource_browser_view; | ||
| CREATE UNIQUE INDEX live_resource_browser_view | ||
| ON resource_tabs(content_id) | ||
| WHERE (deleted_revision IS NULL) | ||
| AND (content_kind = 'browser');", | ||
| ) | ||
| .unwrap(); | ||
| let definition_before = formatted | ||
| .query_row( | ||
| "SELECT sql FROM sqlite_master | ||
| WHERE type = 'index' AND name = 'live_resource_browser_view'", | ||
| [], | ||
| |row| row.get::<_, String>(0), | ||
| ) | ||
| .unwrap(); | ||
| let schema_version_before = | ||
| formatted.query_row("PRAGMA schema_version", [], |row| row.get::<_, i64>(0)).unwrap(); | ||
| drop(formatted); | ||
|
|
||
| let reopened = WorkspaceRegistry::open(&root, "session").unwrap(); | ||
| let canonical_definition = reopened | ||
| .connection | ||
| .query_row( | ||
| "SELECT sql FROM sqlite_master | ||
| WHERE type = 'index' AND name = 'live_resource_browser_view'", | ||
| [], | ||
| |row| row.get::<_, String>(0), | ||
| ) | ||
| .unwrap(); | ||
| let schema_version_after_normalization = reopened | ||
| .connection | ||
| .query_row("PRAGMA schema_version", [], |row| row.get::<_, i64>(0)) | ||
| .unwrap(); | ||
| assert_ne!(canonical_definition, definition_before); | ||
| assert_eq!( | ||
| canonical_definition.split_whitespace().collect::<Vec<_>>().join(" "), | ||
| "CREATE UNIQUE INDEX live_resource_browser_view ON resource_tabs(content_id) WHERE content_kind = 'browser' AND deleted_revision IS NULL" | ||
| ); | ||
| assert!(schema_version_after_normalization > schema_version_before); | ||
| drop(reopened); | ||
|
|
||
| let reopened_again = WorkspaceRegistry::open(&root, "session").unwrap(); | ||
| let definition_after_second_open = reopened_again | ||
| .connection | ||
| .query_row( | ||
| "SELECT sql FROM sqlite_master | ||
| WHERE type = 'index' AND name = 'live_resource_browser_view'", | ||
| [], | ||
| |row| row.get::<_, String>(0), | ||
| ) | ||
| .unwrap(); | ||
| let schema_version_after_second_open = reopened_again | ||
| .connection | ||
| .query_row("PRAGMA schema_version", [], |row| row.get::<_, i64>(0)) | ||
| .unwrap(); | ||
| assert_eq!(definition_after_second_open, canonical_definition); | ||
| assert_eq!(schema_version_after_second_open, schema_version_after_normalization); | ||
| drop(reopened_again); | ||
| fs::remove_dir_all(root).unwrap(); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
This test does not cover an index created with IF NOT EXISTS.
The test drops live_resource_browser_view and recreates it manually. After normalization the index statement comes from migrate_resource_tabs_to_multiview, which omits IF NOT EXISTS. The canonical literal in resource_tabs_needs_multiview_normalization also omits that clause, so the second open matches.
create_resource_schema creates the same index with CREATE UNIQUE INDEX IF NOT EXISTS. No test opens a registry twice whose index came from that path. Add a case that seeds through create_resource_schema, reopens, and asserts PRAGMA schema_version is unchanged. That case pins the concern raised on cmux-tui/crates/cmux-tui-core/src/workspace_registry/resource_store.rs Line 234.
🤖 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/workspace_registry/tests.rs` around lines
4254 - 4324, Add a test alongside
current_schema_canonicalizes_equivalent_formatted_browser_view_predicate_once
that seeds the database through create_resource_schema, opens the registry,
records the index definition and PRAGMA schema_version, then reopens it and
asserts the schema version remains unchanged. Ensure the seeded index uses the
CREATE UNIQUE INDEX IF NOT EXISTS path and verify no normalization rewrite
occurs on the second open.
Comment on lines
+128
to
+184
| fn browser_provider( | ||
| path: &std::path::Path, | ||
| endpoint: String, | ||
| tab_id: &str, | ||
| target_id: &str, | ||
| ) -> BufReader<UnixStream> { | ||
| let mut stream = UnixStream::connect(path).unwrap(); | ||
| let mut line = json!({ | ||
| "id": 1, | ||
| "cmd": "register-browser-provider", | ||
| "provider_id": "browser-runtime-integration", | ||
| "endpoint": endpoint, | ||
| "authentication": "none", | ||
| "targets": [{"tab_id": tab_id, "target_id": target_id}], | ||
| }) | ||
| .to_string() | ||
| .into_bytes(); | ||
| line.push(b'\n'); | ||
| stream.write_all(&line).unwrap(); | ||
| let mut reader = BufReader::new(stream); | ||
| let mut response = String::new(); | ||
| reader.read_line(&mut response).unwrap(); | ||
| let response: Value = serde_json::from_str(&response).unwrap(); | ||
| assert_eq!(response["ok"], true, "browser provider registration failed: {response}"); | ||
| reader | ||
| } | ||
|
|
||
| fn update_browser_provider( | ||
| reader: &mut BufReader<UnixStream>, | ||
| endpoint: String, | ||
| tab_id: &str, | ||
| target_id: &str, | ||
| ) { | ||
| let mut line = json!({ | ||
| "id": 2, | ||
| "cmd": "register-browser-provider", | ||
| "provider_id": "browser-runtime-integration", | ||
| "endpoint": endpoint, | ||
| "authentication": "none", | ||
| "targets": [{"tab_id": tab_id, "target_id": target_id}], | ||
| }) | ||
| .to_string() | ||
| .into_bytes(); | ||
| line.push(b'\n'); | ||
| reader.get_mut().write_all(&line).unwrap(); | ||
| let mut response = String::new(); | ||
| reader.read_line(&mut response).unwrap(); | ||
| let response: Value = serde_json::from_str(&response).unwrap(); | ||
| assert_eq!(response["ok"], true, "browser provider update failed: {response}"); | ||
| } | ||
|
|
||
| fn surface_tab_id(mux: &Mux, surface: u64) -> String { | ||
| mux.surface(surface) | ||
| .and_then(|surface| surface.resource_identity().map(|identity| identity.tab_id.to_string())) | ||
| .expect("browser surface has a stable tab identity") | ||
| } | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Deduplicate the two provider-registration payload builders.
browser_provider and update_browser_provider build the same register-browser-provider line and differ only in the request id and the stream they write to. Extract one helper that takes the writer and the request id, then call it from both functions. The fixed ids 1 and 2 also limit the helpers to one update per connection; passing the id makes repeated updates possible.
🤖 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/tests/browser_runtime.rs` around lines 128 -
184, Extract the shared register-browser-provider payload and write/read logic
from browser_provider and update_browser_provider into one helper that accepts a
mutable writer, request id, endpoint, tab_id, and target_id. Have
browser_provider call it with id 1 while retaining its connection setup and
BufReader return, and have update_browser_provider call it with id 2 using the
existing reader stream. Preserve each function’s current success assertion
context while allowing callers to supply different request ids for repeated
updates.
Comment on lines
+1671
to
+1773
| let first_listener = TcpListener::bind("127.0.0.1:0").unwrap(); | ||
| let first_addr = first_listener.local_addr().unwrap(); | ||
| let second_listener = TcpListener::bind("127.0.0.1:0").unwrap(); | ||
| let second_addr = second_listener.local_addr().unwrap(); | ||
| let (disconnect_tx, disconnect_rx) = mpsc::channel(); | ||
| let first_server = thread::spawn(move || { | ||
| run_reconnect_provider_endpoint( | ||
| first_listener, | ||
| "target-first", | ||
| "session-first", | ||
| "Zmlyc3Q=", | ||
| Some(disconnect_rx), | ||
| ); | ||
| }); | ||
| let second_server = thread::spawn(move || { | ||
| run_reconnect_provider_endpoint( | ||
| second_listener, | ||
| "target-second", | ||
| "session-second", | ||
| "c2Vjb25k", | ||
| None, | ||
| ); | ||
| }); | ||
|
|
||
| let mux = Mux::new("browser-provider-reconnect-test", SurfaceOptions::default()); | ||
| let socket_path = std::env::temp_dir() | ||
| .join(format!( | ||
| "cmux-bp-reconnect-{}-{}", | ||
| std::process::id(), | ||
| SOCKET_SERIAL.fetch_add(1, std::sync::atomic::Ordering::Relaxed) | ||
| )) | ||
| .join("session.sock"); | ||
| server::serve(mux.clone(), Some(socket_path.clone())).unwrap(); | ||
| let surface = | ||
| mux.new_browser_tab("example.test".to_string(), None, Some((10, 5))).expect("browser tab"); | ||
| let tab_id = surface_tab_id(&mux, surface.id); | ||
| let mut provider = browser_provider( | ||
| &socket_path, | ||
| format!("ws://{first_addr}/devtools/browser/first"), | ||
| &tab_id, | ||
| "target-first", | ||
| ); | ||
| wait_for( | ||
| || { | ||
| let frame = surface.browser_frame()?; | ||
| (matches!(surface.browser_status(), Some(BrowserStatus::Live)) | ||
| && frame.session_id == "session-first" | ||
| && frame.data_b64 == "Zmlyc3Q=") | ||
| .then_some(()) | ||
| }, | ||
| Duration::from_secs(10), | ||
| ) | ||
| .expect("first provider target became live"); | ||
|
|
||
| disconnect_tx.send(()).unwrap(); | ||
| wait_for( | ||
| || { | ||
| matches!( | ||
| surface.browser_status(), | ||
| Some(BrowserStatus::Starting | BrowserStatus::Failed(_)) | ||
| ) | ||
| .then_some(()) | ||
| }, | ||
| Duration::from_secs(10), | ||
| ) | ||
| .expect("provider disconnect became observable"); | ||
| mux.with_state(|state| { | ||
| assert_eq!(state.surfaces.len(), 1); | ||
| assert!(state.surfaces.contains_key(&surface.id)); | ||
| }); | ||
|
|
||
| update_browser_provider( | ||
| &mut provider, | ||
| format!("ws://{second_addr}/devtools/browser/second"), | ||
| &tab_id, | ||
| "target-second", | ||
| ); | ||
| wait_for( | ||
| || { | ||
| let frame = surface.browser_frame()?; | ||
| (matches!(surface.browser_status(), Some(BrowserStatus::Live)) | ||
| && frame.session_id == "session-second" | ||
| && frame.data_b64 == "c2Vjb25k") | ||
| .then_some(()) | ||
| }, | ||
| Duration::from_secs(10), | ||
| ) | ||
| .expect("replacement provider target reattached"); | ||
| mux.with_state(|state| { | ||
| assert_eq!(state.surfaces.len(), 1); | ||
| assert!(state.surfaces.contains_key(&surface.id)); | ||
| }); | ||
|
|
||
| mux.close_surface(surface.id).unwrap(); | ||
| drop(provider); | ||
| mux.shutdown(); | ||
| server::cleanup(&socket_path); | ||
| first_server.join().unwrap(); | ||
| second_server.join().unwrap(); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
The disconnect assertion accepts two different statuses.
The wait accepts Starting or Failed(_). The test therefore passes whether the runtime reports a transient reconnect or a terminal failure. If the intended contract is one specific classification after a provider disconnect, assert that status. If both are valid, add a short comment that states why, so a future classification change does not silently keep passing.
🤖 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/tests/browser_runtime.rs` around lines 1671 -
1773, The disconnect wait in
provider_disconnect_reconnects_without_closing_canonical_browser_topology
currently accepts both Starting and Failed statuses without documenting the
intended contract. Assert the single expected status after disconnect, or add a
concise comment explaining why both classifications are valid so future status
changes remain intentional.
Comment on lines
+10202
to
+10246
| fn frontend_presentation_unchanged(&self) -> bool { | ||
| let Some(previous) = &self.last_frontend_presentation else { return false }; | ||
| let workspace = self.tree.active_workspace(); | ||
| let screen = workspace.and_then(|workspace| workspace.active_screen_ref()); | ||
| let pane = screen.and_then(|screen| screen.pane(screen.active_pane)); | ||
| let tab = pane.and_then(|pane| pane.tabs.get(pane.active_tab)); | ||
| let target = match self.focus { | ||
| FocusTarget::Pane => FrontendFocusTarget::Pane, | ||
| FocusTarget::MachineRail => FrontendFocusTarget::MachineRail, | ||
| FocusTarget::WorkspaceRail => FrontendFocusTarget::WorkspaceRail, | ||
| FocusTarget::TabsRail => FrontendFocusTarget::TabsRail, | ||
| FocusTarget::ProjectionRail(_) => FrontendFocusTarget::ProjectionRail, | ||
| }; | ||
| if previous.focus.target != target | ||
| || previous.focus.workspace_id.as_ref() | ||
| != workspace.and_then(|workspace| workspace.resource_id.as_ref()) | ||
| || previous.focus.screen_id.as_ref() | ||
| != screen.and_then(|screen| screen.resource_id.as_ref()) | ||
| || previous.focus.pane_id.as_ref() != pane.and_then(|pane| pane.resource_id.as_ref()) | ||
| || previous.focus.tab_id.as_ref() != tab.and_then(|tab| tab.public_id.as_ref()) | ||
| || previous.focus.content_id.as_ref() != tab.and_then(|tab| tab.content_id.as_ref()) | ||
| { | ||
| return false; | ||
| } | ||
| if previous.resize | ||
| != (FrontendResizeSnapshot { | ||
| cols: self.outer_size.0, | ||
| rows: self.outer_size.1, | ||
| cell_width: self.cell_pixels.0, | ||
| cell_height: self.cell_pixels.1, | ||
| }) | ||
| { | ||
| return false; | ||
| } | ||
| let (target, settled) = screen | ||
| .and_then(|screen| self.viewport_states.get(&screen.id)) | ||
| .map_or((self.viewport_offset, true), |motion| { | ||
| (motion.target.round() as u64, !motion.animating()) | ||
| }); | ||
| previous.viewport.screen_id.as_ref() | ||
| == screen.and_then(|screen| screen.resource_id.as_ref()) | ||
| && previous.viewport.offset == if settled { self.viewport_offset } else { target } | ||
| && previous.viewport.target == target | ||
| && previous.viewport.settled == settled | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Derive frontend_presentation_unchanged from frontend_presentation_snapshot.
frontend_presentation_unchanged re-derives the focus target, the resource ids, the resize tuple, and the viewport motion inline. frontend_presentation_snapshot derives the same values a few lines above. The two paths must stay identical, or the change detection will emit or drop journal events incorrectly. A future edit to one derivation will silently diverge from the other.
Compare the snapshot value instead. FrontendPresentationSnapshot already derives PartialEq, and journal_frontend_presentation already calls frontend_presentation_snapshot on the changed path.
♻️ Proposed refactor to keep one derivation
- fn frontend_presentation_unchanged(&self) -> bool {
- let Some(previous) = &self.last_frontend_presentation else { return false };
- let workspace = self.tree.active_workspace();
- let screen = workspace.and_then(|workspace| workspace.active_screen_ref());
- let pane = screen.and_then(|screen| screen.pane(screen.active_pane));
- let tab = pane.and_then(|pane| pane.tabs.get(pane.active_tab));
- let target = match self.focus {
- FocusTarget::Pane => FrontendFocusTarget::Pane,
- FocusTarget::MachineRail => FrontendFocusTarget::MachineRail,
- FocusTarget::WorkspaceRail => FrontendFocusTarget::WorkspaceRail,
- FocusTarget::TabsRail => FrontendFocusTarget::TabsRail,
- FocusTarget::ProjectionRail(_) => FrontendFocusTarget::ProjectionRail,
- };
- if previous.focus.target != target
- || previous.focus.workspace_id.as_ref()
- != workspace.and_then(|workspace| workspace.resource_id.as_ref())
- || previous.focus.screen_id.as_ref()
- != screen.and_then(|screen| screen.resource_id.as_ref())
- || previous.focus.pane_id.as_ref() != pane.and_then(|pane| pane.resource_id.as_ref())
- || previous.focus.tab_id.as_ref() != tab.and_then(|tab| tab.public_id.as_ref())
- || previous.focus.content_id.as_ref() != tab.and_then(|tab| tab.content_id.as_ref())
- {
- return false;
- }
- if previous.resize
- != (FrontendResizeSnapshot {
- cols: self.outer_size.0,
- rows: self.outer_size.1,
- cell_width: self.cell_pixels.0,
- cell_height: self.cell_pixels.1,
- })
- {
- return false;
- }
- let (target, settled) = screen
- .and_then(|screen| self.viewport_states.get(&screen.id))
- .map_or((self.viewport_offset, true), |motion| {
- (motion.target.round() as u64, !motion.animating())
- });
- previous.viewport.screen_id.as_ref()
- == screen.and_then(|screen| screen.resource_id.as_ref())
- && previous.viewport.offset == if settled { self.viewport_offset } else { target }
- && previous.viewport.target == target
- && previous.viewport.settled == settled
- }
+ fn frontend_presentation_unchanged(&self) -> bool {
+ self.last_frontend_presentation.as_ref()
+ == Some(&self.frontend_presentation_snapshot())
+ }📝 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.
Suggested change
| fn frontend_presentation_unchanged(&self) -> bool { | |
| let Some(previous) = &self.last_frontend_presentation else { return false }; | |
| let workspace = self.tree.active_workspace(); | |
| let screen = workspace.and_then(|workspace| workspace.active_screen_ref()); | |
| let pane = screen.and_then(|screen| screen.pane(screen.active_pane)); | |
| let tab = pane.and_then(|pane| pane.tabs.get(pane.active_tab)); | |
| let target = match self.focus { | |
| FocusTarget::Pane => FrontendFocusTarget::Pane, | |
| FocusTarget::MachineRail => FrontendFocusTarget::MachineRail, | |
| FocusTarget::WorkspaceRail => FrontendFocusTarget::WorkspaceRail, | |
| FocusTarget::TabsRail => FrontendFocusTarget::TabsRail, | |
| FocusTarget::ProjectionRail(_) => FrontendFocusTarget::ProjectionRail, | |
| }; | |
| if previous.focus.target != target | |
| || previous.focus.workspace_id.as_ref() | |
| != workspace.and_then(|workspace| workspace.resource_id.as_ref()) | |
| || previous.focus.screen_id.as_ref() | |
| != screen.and_then(|screen| screen.resource_id.as_ref()) | |
| || previous.focus.pane_id.as_ref() != pane.and_then(|pane| pane.resource_id.as_ref()) | |
| || previous.focus.tab_id.as_ref() != tab.and_then(|tab| tab.public_id.as_ref()) | |
| || previous.focus.content_id.as_ref() != tab.and_then(|tab| tab.content_id.as_ref()) | |
| { | |
| return false; | |
| } | |
| if previous.resize | |
| != (FrontendResizeSnapshot { | |
| cols: self.outer_size.0, | |
| rows: self.outer_size.1, | |
| cell_width: self.cell_pixels.0, | |
| cell_height: self.cell_pixels.1, | |
| }) | |
| { | |
| return false; | |
| } | |
| let (target, settled) = screen | |
| .and_then(|screen| self.viewport_states.get(&screen.id)) | |
| .map_or((self.viewport_offset, true), |motion| { | |
| (motion.target.round() as u64, !motion.animating()) | |
| }); | |
| previous.viewport.screen_id.as_ref() | |
| == screen.and_then(|screen| screen.resource_id.as_ref()) | |
| && previous.viewport.offset == if settled { self.viewport_offset } else { target } | |
| && previous.viewport.target == target | |
| && previous.viewport.settled == settled | |
| } | |
| fn frontend_presentation_unchanged(&self) -> bool { | |
| self.last_frontend_presentation.as_ref() | |
| == Some(&self.frontend_presentation_snapshot()) | |
| } |
🤖 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 10202 - 10246, Refactor
frontend_presentation_unchanged to obtain the current value from
frontend_presentation_snapshot and compare it directly with
last_frontend_presentation using FrontendPresentationSnapshot’s PartialEq
implementation. Remove the duplicated inline focus, resize, and viewport
derivation while preserving the existing false result when no previous snapshot
exists.
Comment on lines
+383
to
+409
| Discovery records use JSON `record_version:4`. Terminal and incarnation are | ||
| 32-character lowercase UUIDv4 hex, owner token and process nonce are | ||
| 64-character lowercase hex, the Unix-socket path is canonical, and the host | ||
| PID is nonzero. Record directories are mode `0700`; records and sockets are | ||
| mode `0600`. | ||
|
|
||
| ## Durability boundary | ||
|
|
||
| The append-only journal is exact while a mux daemon owns the authenticated host | ||
| tap. A host `Snapshot` preserves renderable terminal state across daemon | ||
| replacement, and the exit sidecar preserves the final process outcome, but the | ||
| host does not currently retain an acknowledged raw-output spool. Bytes emitted | ||
| while no daemon tap exists can therefore be recovered visually from a snapshot | ||
| but cannot be reconstructed as exact historical `terminal.output` records. The | ||
| mux commits a replacement checkpoint after it applies such a reconnect snapshot | ||
| and before it accepts the new live boundary. Restoration starts from that | ||
| durable terminal state, but consumers must not claim byte-exact output history | ||
| across an unplanned no-tap interval until a durable host spool exists. | ||
|
|
||
| ## Version compatibility | ||
|
|
||
| Protocol v1 carries the base snapshot and legacy replay stream. Protocol v2 | ||
| adds Kitty image aliases and cell-pixel metrics. Protocol v3 adds Kitty replay | ||
| state, Kitty quota controls, and the smart raw-byte stream. A v3 client may | ||
| negotiate v1 or v2 only in legacy mode; smart renderers require v3 and restart | ||
| their handshake on any gap or `ResyncRequired` frame. | ||
| state, Kitty quota controls, and the smart raw-byte stream. Protocol v4 adds | ||
| the launch activation barrier. A v4 client may negotiate v1 or v2 only in | ||
| legacy mode; smart renderers require v3 and restart their handshake on any gap | ||
| or `ResyncRequired` frame. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Document which record versions remain adoptable and which support the detach fence.
This section states that discovery records use record_version:4. The runtime still validates and adopts record versions 1, 2, and 3, and it derives detach-fence support from record_version >= 4. A reader cannot learn from this spec that Detach and DetachAck are unavailable on an older record, so a daemon author could assume the shutdown fence always exists.
State that the current host emits version 4, that versions 1 through 3 remain adoptable, and that the source-ordered detach fence requires version 4.
🤖 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/spec/terminal-host.md` around lines 383 - 409, Update the discovery
record documentation to state that the host currently emits record_version 4,
versions 1 through 3 remain adoptable, and the source-ordered Detach/DetachAck
shutdown fence is supported only for record_version 4 or newer.
This was referenced Aug 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Draft base PR for the journal-first stack. Successors: #9806, #9813, https://github.com/manaflow-ai/cmux/pull/9815.\n\nScope:\n- Carry append-only session journal work from #9434 onto the client-local multiview architecture from https://github.com/manaflow-ai/cmux/pull/9387.\n- Add an owner-scoped cmux-browser provider lease so browser resources attach to native cmux-browser tabs instead of isolated Chrome.\n- Project workspaces, screens, panes, and tabs one-to-one while keeping focus, active view, and sizing client-local.\n- Keep CDP endpoints and optional bearer capabilities connection-scoped and out of SQLite. Only logical browser topology is durable.\n- Expose workspace-scoped agent-browser routing through the provider.\n- Keep protocol v2 SDK metadata aligned with 100 raw commands.\n\nTesting:\n- Sealed cross-repo dogfood: cmux 98abef6 with cmux-browser 689d8dfc83783422a2415a9cb59e069e3b373524.\n- Verified two populated workspaces plus the preserved initial empty workspace, three screens total, four panes, seven mixed terminal/browser tabs, and three native browser targets.\n- Verified client-local focus: attached TUI selected Alpha/Review while the native browser projection remained on Beta.\n- Verified resize/reflow at 760x520, 1000x700, and 1200x780 with no stale pixels or topology drift.\n- Verified security live: 0600 Unix socket inside a 0700 directory, CDP bound only to 127.0.0.1, non-loopback registration rejected, conflicting owner rejected, provider state unchanged after both negatives.\n- Verified performance live: 100 pings in 0.78 s, 25 snapshots in 0.21 s, 60.4 MiB RSS for the headless helper plus four terminal hosts, and selected-tab-only frame streaming.\n- git diff --check.\n- cmake -S cmux-tui/bindings/cpp -B /tmp/cmux-cpp-sdk-9726 -DCMAKE_BUILD_TYPE=Release -DCMAKE_CXX_FLAGS="-Wall -Wextra -Wpedantic -Werror".\n- cmake --build /tmp/cmux-cpp-sdk-9726 --parallel.\n- ctest --test-dir /tmp/cmux-cpp-sdk-9726 --output-on-failure.\n\nCurrent blocker:\nThis PR remains draft. Do not land it until the journal contract and generated protocol gates are reliable. Successor PRs remain blocked on this PR.\n\nOwner closeout:\nFixed = native browser provider and session journal were not integrated with the client-local multiview model -> PR 9726 provides the provider, journal, checkpoints, topology, and generated protocol base for the stack.\nHow = provider leases keep live capabilities out of SQLite, while durable journal and topology data carry only recoverable state. This is principled because live transport ownership and recoverable semantic history stay separate.\nRisk = recovery policy, hibernation, fork, and host-loss classification are intentionally deferred to stacked successor PRs.
Note
High Risk
Large stack touching durable journal storage, process hooks, browser provider registration (local-admin), and broad protocol/SDK regeneration; recovery and hibernation are deferred to follow-up PRs per the draft description.
Overview
Integrates an append-only session journal (subscribe, checkpoints, hooks, producers, segments, frontend journal events) with the client-local multiview model, backed by rusqlite hooks, jsonschema, and new cmux-tui-core journal/process-fence tests in CI.
Browser tabs become attach-only: cmux-tui no longer launches or owns Chrome; a live cmux-browser process registers loopback CDP and tab→target mapping via
register-browser-provider. README and ergonomics docs reflect workspace-scoped agent-browser routing through that provider.Adds a
cmux-tui-hookbinary (built, staged, and shipped in npm/PyPI wheels; excluded from legacycmux-mux-*R2 double-publish). Terminal hooks and process scopes tie child lifecycle to journal semantics.Protocol v2 grows to 124 resource operations and 101 raw commands (browser provider, journal APIs,
journal-frontend-event, creation idempotency_key). All seven SDK bindings regenerate; C++ resource API addssession.journal.subscribe, typed journal records,FrontendProjection::put, and attachment leases on terminal/browser viewer resize/release withoutcome(applied/passive/superseded).CI builds the hook alongside
cmux-tui, verifies hook executability in npm/PyPI packages, uploads rustfmt patches on fmt failure, and installs TypeScript adapter deps for binding e2e.Reviewed by Cursor Bugbot for commit 80c70ba. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit