Repository navigation
Make cmux-tui programmability inventory enforceable - #8856
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a machine-readable cmux-tui protocol inventory and schema, runtime authorization and stream validation, drift checking with CI enforcement, and extensive updates to command, event, transport, secondary-protocol, binding, frontend, and programmability contracts. Changescmux-tui specification inventory
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 24 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (24 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR adds a machine-checked programmability inventory for
Confidence Score: 5/5Safe to merge — all changes are additive spec/tooling infrastructure plus two tightly-tested runtime hardening pieces with no existing behavior removed. The Rust additions (CommandProfile, PUBLIC_EVENT_CATALOG, authorize_command_profile) are all covered by new unit tests; the workspace_mutation fix is validated by a dedicated regression test; the Python checker is covered by 776 lines of unit tests run in CI; secondary-protocol parsing now correctly uses rust_enum_variants for all enum-typed protocols and a proper lookahead for the discriminant-typed MessageKind. Files Needing Attention: No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant Client
participant handle_command
participant authorize_command_profile
participant CommandProfile
participant PUBLIC_EVENT_CATALOG
participant MessageWriter
Client->>handle_command: Command (any variant)
handle_command->>authorize_command_profile: "&mut cmd"
authorize_command_profile->>CommandProfile: cmd.profile()
CommandProfile-->>authorize_command_profile: Control/Frontend/LocalAdmin/ProviderAuthority
alt LocalAdmin + not unix socket
authorize_command_profile-->>handle_command: Err (trusted local required)
else ProviderAuthority + bad authority
authorize_command_profile-->>handle_command: Err (authority zeroized)
else authorized
authorize_command_profile-->>handle_command: Ok(())
end
handle_command->>MessageWriter: start_public_stream(overflow, streams)
MessageWriter->>PUBLIC_EVENT_CATALOG: validate_public_event(overflow, memberships)
PUBLIC_EVENT_CATALOG-->>MessageWriter: Ok / Err(not in catalog)
loop every send_stream / send_initial / send_terminal
MessageWriter->>PUBLIC_EVENT_CATALOG: validate_public_event(value, memberships)
PUBLIC_EVENT_CATALOG-->>MessageWriter: Ok / Err(wrong stream)
end
MessageWriter-->>Client: streamed events
Reviews (12): Last reviewed commit: "Merge main into TUI programmability inve..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if inventory["mux_protocol"] != mux_protocol_version(): | ||
| fail( | ||
| "mux protocol drift, " | ||
| f"runtime is {mux_protocol_version()} and inventory is {inventory['mux_protocol']}" | ||
| ) |
There was a problem hiding this comment.
mux_protocol_version() is called twice in the drift check: once to compare and again inside the f-string that builds the error message. Each call opens and re-reads server.rs, re-runs the regex, and resolves the constant chain, resulting in two full file reads for every drift failure. Capture the value once and reuse it.
| if inventory["mux_protocol"] != mux_protocol_version(): | |
| fail( | |
| "mux protocol drift, " | |
| f"runtime is {mux_protocol_version()} and inventory is {inventory['mux_protocol']}" | |
| ) | |
| runtime_mux_version = mux_protocol_version() | |
| if inventory["mux_protocol"] != runtime_mux_version: | |
| fail( | |
| "mux protocol drift, " | |
| f"runtime is {runtime_mux_version} and inventory is {inventory['mux_protocol']}" | |
| ) |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Fixed in 4075b1b. The runtime protocol version is now read once and reused in the comparison and error.
— Claude Code
There was a problem hiding this comment.
Fixed in 4075b1b. The runtime protocol version is now read once and reused in the comparison and error.
— Claude Code
| for index in range(start, len(source)): | ||
| char = source[index] | ||
| if char == "{": | ||
| depth += 1 | ||
| elif char == "}": | ||
| depth -= 1 | ||
| if depth == 0: | ||
| return source[start:index] | ||
| fail(f"unterminated Rust enum {name}") | ||
| return "" | ||
|
|
||
|
|
||
| def camel_to_kebab(name: str) -> str: | ||
| return re.sub(r"(?<!^)(?=[A-Z])", "-", name).lower() | ||
|
|
There was a problem hiding this comment.
The pattern r"(?m)^ ([A-Z][A-Za-z0-9]*)\s*(?:,|\(|\{)" requires each variant to be followed by a comma, (, or {. A bare trailing variant without a trailing comma is not matched, causing the script to silently drop it from the runtime set. The compare() call then reports "not implemented" for an entry that is actually present, turning a real match into a false drift failure.
There was a problem hiding this comment.
Fixed in 4075b1b. One shared enum parser now accepts comma, tuple, struct, and bare final variants. The preceding regression-test commit proves the bare-final case failed before the fix.
— Claude Code
There was a problem hiding this comment.
Fixed in 4075b1b. One shared enum parser now accepts comma, tuple, struct, and bare final variants. The preceding regression-test commit proves the bare-final case failed before the fix.
— Claude Code
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 21
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cmux-tui/spec/events.md (2)
905-919: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMake omitted cursor metadata valid in the
colors-changedschema.The payload marks
cursor,cursor_style, andcursor_blinkas required, but the meaning explicitly says live palette events omit cursor metadata. Generated clients will either reject valid palette events or treat the contract inconsistently. Mark those fields optional for palette-only events or require explicitnullvalues and update the prose.🤖 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/events.md` around lines 905 - 919, Update the colors-changed schema and its documented behavior consistently: allow cursor, cursor_style, and cursor_blink to be omitted for live palette-only events, or require explicit null values and revise the prose accordingly. Preserve authoritative cursor metadata for default-color events and update the object definition near the colors-changed event.
255-295: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMake mutation identity fields both-or-neither.
origin?:stringandmutation_id?:stringare independently optional in every workspace delta, whilecommands.mddefines them as a paired idempotency identity. A one-sided event cannot be safely correlated with the originating mutation. Use a paired optional object or document that both fields are present together.🤖 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/events.md` around lines 255 - 295, Update the workspace delta payload definitions for workspace-added, workspace-closed, workspace-renamed, and workspace-moved so origin and mutation_id are represented as a single optional paired identity, or explicitly document that they must both be present together. Keep the existing fields and event-specific payloads unchanged otherwise, and align the schema with the idempotency contract in commands.md.
🤖 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-spec.yml:
- Around line 3-8: Update the workflow trigger configuration under on to also
run on push events targeting the main branch, while preserving the existing
pull_request path filters and workflow_dispatch trigger.
In `@cmux-tui/bindings/styles/python.md`:
- Line 19: Update the requirement to specify attach compatibility by protocol
version: preserve byte attach support for v5/v6, support render attach from v7
onward, and support browser attach only from v9. Keep resized replay handling
and the protocol-v5 byte fallback scoped to their applicable versions.
In `@cmux-tui/scripts/check-spec-inventory.py`:
- Around line 29-84: The validation logic is concentrated in oversized
validate_schema() and main() functions, triggering branch and statement-count
limits. Split each independent schema concern in main()—commands, events,
tui_actions, menu_actions, feature_families, secondary_protocols, and
protocol_domains—into focused named validate_* helpers, and decompose
validate_schema() into smaller checks for scalar constraints, strings, integers,
arrays, and objects while preserving validation behavior and error paths.
- Around line 386-406: Extend the machine-provider documentation coverage check
near undocumented_provider to also inspect
secondary["machine_provider_v1"]["events"], requiring each event name to appear
as `name` in provider_doc and failing with a clear event-specific message when
any are missing. Preserve the existing requests check and runtime enum
comparisons.
In `@cmux-tui/spec/cli.md`:
- Around line 207-216: Update the ordered-list markers for the examples in the
CLI documentation, including the item containing the shell script and the
following items, so every item uses “1.” consistently. Preserve the existing
ordering and example content.
- Line 81: Update the set-client-sizing CLI contract and verb table to document
the supported exclusive:true route by adding an --exclusive flag, including its
expected value and behavior; alternatively, explicitly state that exclusive
sizing is restricted to the socket/API and cannot be emitted through the CLI.
In `@cmux-tui/spec/commands.md`:
- Line 1485: Update the cursor_style row in the commands documentation table so
the pipe separators in its alternatives are escaped within the Markdown cell,
preserving the displayed options while preventing them from being parsed as
column delimiters.
- Around line 166-175: Update the common command envelope documentation and
related TypeScript bindings to use expected_terminal_revision for terminal
operations, including create-terminal and move-terminal, instead of
expected_revision. Ensure terminal handlers consume the renamed field
consistently while preserving expected_revision for workspace registry
operations.
- Around line 3383-3395: Restrict `report-agent` so untrusted callers cannot
claim `source:"hook"` authority; authenticate hook-originated reports or accept
them only through a restricted producer. Preserve hook precedence only for
validated hook reports, while handling caller-supplied socket reports normally.
- Around line 1466-1488: Update the CLI specification entry for the
terminal-defaults command to align with the expanded contract documented in
commands.md. Either document CLI flags for cursor, cursor_style, cursor_blink,
palette, and complete, or explicitly state that the CLI intentionally exposes
only the existing fg and bg subset; keep both specifications consistent.
- Around line 2302-2308: The v9 move-tab dispatcher must not unconditionally
return success after validation. Propagate the mux operation’s persistence
failure and changed state, returning an explicit error when the tab was not
committed or a result that includes committed/changed status and revision;
preserve the existing no-op behavior only when the requested move is already
effective.
- Around line 1072-1075: Update the durable mutation branch selection and
validation described in the command envelope so origin and mutation_id must be
provided together: reject requests containing exactly one before choosing the
durable or legacy path, and allow only both-present or both-absent identities.
Ensure a lone mutation_id cannot enter the durable mutation flow.
- Around line 648-651: Sanitize all public upstream error surfaces: in
cmux-tui/spec/commands.md lines 648-651, replace spawn and PTY raw error strings
with stable product-level codes and messages; in cmux-tui/spec/events.md lines
935-952, constrain browser-state.error to the same safe error contract while
retaining raw diagnostics only in internal telemetry. Do not expose vendor or
provider internals in command responses or browser events.
In `@cmux-tui/spec/machine-provider.md`:
- Around line 98-100: The machine-provider protocol must not return the shared
long-lived workspace authority to every authorized team member. Update the
provider authorization flow and private mirror handshake around
CMUX_PROVIDER_WORKSPACE_AUTHORITY to bind authorization to an authenticated
provider channel or issue scoped per-frontend/per-operation capabilities, while
preserving constant-time validation and provider-only rename/close access.
In `@cmux-tui/spec/terminal-host.md`:
- Around line 253-259: Align Resized replay framing between the producer and the
surface.rs consumer: decode and consume replay_len, validate that the remaining
payload length matches it, and pass only replay bytes to the VT state. Add or
update producer-consumer and cross-language fixtures covering resize replay, and
promote terminal-host v1 to implemented only after they pass.
In `@cmux-tui/spec/transports.md`:
- Around line 190-194: Enforce an Origin allowlist in the WebSocket listener
before accepting authenticated or reconnect-credential sessions. Reject requests
whose Origin is not explicitly trusted, including when the listener uses
non-loopback binding, and ensure the existing path handling cannot bypass this
check. Update the listener’s authentication/handshake flow rather than relying
on TCP peer or pairing approval alone.
- Around line 28-30: Validate session names in the server before any socket-path
construction or resolution, using the allowed pattern
`[A-Za-z0-9][A-Za-z0-9._-]{0,63}` and rejecting `.`, `..`, separators, and
control characters. Apply this at the server’s session-input boundary so raw
CLI/API callers are covered, and return the existing invalid-input error without
constructing or selecting a socket path.
- Around line 96-100: Enforce the documented 4,194,304-byte client-to-server
message limit in the Unix and relay JSON-lines reader implementations, using the
existing reader/parser symbols to track bytes before buffering an unterminated
line. Reject and close the receiver as soon as a message exceeds the limit,
excluding the delimiter, while preserving normal parsing for valid messages and
applying the same behavior consistently to both transports.
- Line 94: Update the child-process environment handling described in the socket
capability section to stop exporting CMUX_TUI_SOCKET and CMUX_MUX_SOCKET to
untrusted children. Remove the ambient full-session variables, or replace them
with an explicitly scoped capability that limits access to the intended surface
rather than the entire mux session.
- Around line 198-202: Update the timeout and retry guidance in the
“Concurrency, streams, and reconnect” section to distinguish local request
timeouts from server-side mutation cancellation. Require retryable
non-idempotent mutations to use durable mutation identities for deduplication,
or explicitly prohibit retrying them until the original outcome is reconciled.
- Around line 128-130: Update the relay authorization behavior described in the
SSH machine connector documentation so remote SSH relay clients use a restricted
relay profile or separate authorization class rather than being classified as
Unix. Ensure relay principals do not receive local-admin operations such as
shutdown-daemon and pairing-response by default, while preserving full
local-admin authority only for direct Unix socket access.
---
Outside diff comments:
In `@cmux-tui/spec/events.md`:
- Around line 905-919: Update the colors-changed schema and its documented
behavior consistently: allow cursor, cursor_style, and cursor_blink to be
omitted for live palette-only events, or require explicit null values and revise
the prose accordingly. Preserve authoritative cursor metadata for default-color
events and update the object definition near the colors-changed event.
- Around line 255-295: Update the workspace delta payload definitions for
workspace-added, workspace-closed, workspace-renamed, and workspace-moved so
origin and mutation_id are represented as a single optional paired identity, or
explicitly document that they must both be present together. Keep the existing
fields and event-specific payloads unchanged otherwise, and align the schema
with the idempotency contract in commands.md.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 94f34ed7-6467-4a66-b070-11bb0a08dce1
📒 Files selected for processing (21)
.github/workflows/cmux-tui-spec.ymlcmux-tui/bindings/generate.shcmux-tui/bindings/styles/go.mdcmux-tui/bindings/styles/java.mdcmux-tui/bindings/styles/python.mdcmux-tui/scripts/check-spec-inventory.pycmux-tui/spec/README.mdcmux-tui/spec/bindings.mdcmux-tui/spec/cli.mdcmux-tui/spec/commands.mdcmux-tui/spec/events.mdcmux-tui/spec/frontends.mdcmux-tui/spec/inventory.jsoncmux-tui/spec/inventory.schema.jsoncmux-tui/spec/machine-provider.mdcmux-tui/spec/native-frontend.mdcmux-tui/spec/plugins.mdcmux-tui/spec/programmability.mdcmux-tui/spec/provider-management.mdcmux-tui/spec/terminal-host.mdcmux-tui/spec/transports.md
| Relay grants the remote SSH principal the authority of the selected local Unix socket. Deployments must restrict SSH admission and the remote socket with the same care as direct socket access. | ||
|
|
||
| The server classifies relay traffic as Unix because relay terminates at the Unix socket. The remote SSH principal therefore receives local-admin operations, including `shutdown-daemon` and `pairing-response`. Deployments that need less authority must use a future distinct relay profile. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Do not grant remote SSH relay clients local-admin authority by default.
The documented SSH machine connector uses relay, yet relay traffic is classified as Unix and therefore receives shutdown-daemon and pairing-response. Use a restricted relay profile or separate authorization class; filesystem-equivalent local-admin privileges are too broad for a remote transport principal.
🤖 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/transports.md` around lines 128 - 130, Update the relay
authorization behavior described in the SSH machine connector documentation so
remote SSH relay clients use a restricted relay profile or separate
authorization class rather than being classified as Unix. Ensure relay
principals do not receive local-admin operations such as shutdown-daemon and
pairing-response by default, while preserving full local-admin authority only
for direct Unix socket access.
There was a problem hiding this comment.
Addressed as a current-version authority gap in 4075b1b. The contract accurately records that relay inherits Unix local-admin authority, and vNext now requires a restricted relay profile. Changing relay authorization is runtime scope.
— Claude Code
There was a problem hiding this comment.
Rate Limit Exceeded
@lawrencecchen have exceeded the limit for the number of chat messages per hour. Please wait 56 minutes and 43 seconds before sending another message.
| The current listener accepts every WebSocket Origin and request path. A browser challenge identifies only its TCP peer, which is normally loopback. Deployments must not treat pairing as an Origin check. vNext adds an explicit Origin allowlist and includes normalized Origin and path in the trusted approval prompt. | ||
|
|
||
| ### Bind Security | ||
|
|
||
| By default the listener accepts only an IP loopback address such as `127.0.0.1` or `[::1]`. cmux-tui refuses a non-loopback address unless `--ws-insecure-bind` is also present. This listener provides no TLS; for remote access, bind deliberately and place it behind a TLS-terminating, authenticated reverse proxy. A WebSocket client has the same authority as a Unix socket client: it can read terminal contents, type into PTYs, and mutate or close the session. | ||
| By default the listener accepts only an IP loopback address such as `127.0.0.1` or `[::1]`. cmux-tui refuses a non-loopback address unless `--ws-insecure-bind` is also present. This listener provides no TLS; for remote access, bind deliberately and place it behind a TLS-terminating, authenticated reverse proxy. An authenticated WebSocket client can read terminal contents, type into PTYs, and use ordinary control and frontend mutations, including closing session topology. It cannot use Unix-only `local-admin` commands: `shutdown-daemon` and `pairing-response` reject WebSocket callers. Provider-owned workspace commits also require their separate provider authority. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Enforce WebSocket Origin policy before accepting authenticated sessions.
The listener accepts every Origin and path, including when deliberately exposed beyond loopback. Static-token or reconnect-credential clients can then be driven cross-origin by a malicious web page. Add an Origin allowlist or equivalent trusted approval binding now; deferring it to vNext leaves the current exposed listener vulnerable.
🤖 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/transports.md` around lines 190 - 194, Enforce an Origin
allowlist in the WebSocket listener before accepting authenticated or
reconnect-credential sessions. Reject requests whose Origin is not explicitly
trusted, including when the listener uses non-loopback binding, and ensure the
existing path handling cannot bypass this check. Update the listener’s
authentication/handshake flow rather than relying on TCP peer or pairing
approval alone.
There was a problem hiding this comment.
Addressed as a current-version security gap in 4075b1b. The contract already records that every Origin is accepted and forbids treating pairing as an Origin check. The vNext requirements now explicitly require an Origin allowlist and Origin-aware challenges.
— Claude Code
There was a problem hiding this comment.
Rate Limit Exceeded
@lawrencecchen have exceeded the limit for the number of chat messages per hour. Please wait 56 minutes and 42 seconds before sending another message.
| ## Concurrency, streams, and reconnect | ||
|
|
||
| The v9 server begins commands serially in receive order on each connection. A blocking `wait-for` delays later commands on that connection. A client request timeout stops local waiting only; it does not cancel execution, and a late mutation may still commit. | ||
|
|
||
| Repeated `subscribe` calls and repeated attachments create independent server streams without public stream ids. Closing a local iterator on a shared connection does not cancel its server stream. A v9 client that needs independent cancellation uses a dedicated connection and closes it. On one shared connection, use at most one subscription and one attachment per surface. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Separate request timeouts from mutation cancellation or retry safely.
A client timeout only stops local waiting while the server may commit later. Retrying non-idempotent legacy mutations can therefore create duplicate work or state, and the client has no authoritative outcome. Require durable mutation identities for retryable mutations, or explicitly prohibit retries until the original result is reconciled.
🤖 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/transports.md` around lines 198 - 202, Update the timeout and
retry guidance in the “Concurrency, streams, and reconnect” section to
distinguish local request timeouts from server-side mutation cancellation.
Require retryable non-idempotent mutations to use durable mutation identities
for deduplication, or explicitly prohibit retrying them until the original
outcome is reconciled.
There was a problem hiding this comment.
Fixed in 4075b1b. The transport contract now prohibits retrying timed-out legacy non-idempotent mutations before reconciliation and requires durable retries to reuse the same operation identity.
— Claude Code
There was a problem hiding this comment.
Rate Limit Exceeded
@lawrencecchen have exceeded the limit for the number of chat messages per hour. Please wait 56 minutes and 41 seconds before sending another message.
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
|
Addressed both CodeRabbit outside-diff findings in 4075b1b: workspace delta origin and mutation_id are now explicitly both-present or both-absent, and colors-changed marks cursor metadata optional for palette-only events. — Codex |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmux-tui/spec/inventory.json`:
- Line 170: Replace the free-form route value for NewWorkspace with structured
fields identifying the authoritative ownership source and separate session-owned
and provider-owned target operations. Update the inventory checker to validate
both referenced command/protocol operations and reject missing or unknown
ownership signals instead of defaulting to new-workspace. Adjust the associated
route test to cover the structured declaration and fail-closed behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 921c869c-f98a-44f1-92b5-68027c93a2d2
📒 Files selected for processing (4)
cmux-tui/scripts/check-spec-inventory.pycmux-tui/scripts/test_check_spec_inventory.pycmux-tui/spec/inventory.jsoncmux-tui/spec/programmability.md
| def secondary_protocols() -> dict[str, object]: | ||
| host_source = (TUI / "crates/cmux-tui-core/src/terminal_host_protocol.rs").read_text() | ||
| host_body = rust_enum_body(host_source, "MessageKind") | ||
| host_messages = { | ||
| name: int(value) | ||
| for name, value in re.findall( | ||
| r"(?m)^ ([A-Z][A-Za-z0-9]*)\s*=\s*([0-9]+),", host_body | ||
| ) | ||
| } | ||
|
|
||
| provider_source = ( | ||
| TUI / "crates/cmux-tui-machine-protocol/src/lib.rs" | ||
| ).read_text() | ||
| request_body = rust_enum_body(provider_source, "ProviderRequest") | ||
| provider_requests = { | ||
| camel_to_snake(name) | ||
| for name in re.findall( | ||
| r"(?m)^ ([A-Z][A-Za-z0-9]*)\s*(?:\(|\{|,)", request_body | ||
| ) | ||
| } | ||
| event_body = rust_enum_body(provider_source, "ProviderEvent") | ||
| provider_events = { | ||
| camel_to_snake(name) | ||
| for name in re.findall( | ||
| r"(?m)^ ([A-Z][A-Za-z0-9]*)\s*(?:\(|\{|,)", event_body | ||
| ) | ||
| } | ||
|
|
||
| management_source = ( | ||
| TUI / "crates/cmux-tui-core/src/provider_management.rs" | ||
| ).read_text() | ||
| management_body = rust_enum_body(management_source, "Request") | ||
| management_operations = { | ||
| camel_to_snake(name) | ||
| for name in re.findall( | ||
| r"(?m)^ ([A-Z][A-Za-z0-9]*)\s*(?:\{|,)", management_body | ||
| ) | ||
| } | ||
| return { | ||
| "terminal_host_v1": host_messages, | ||
| "machine_provider_v1_requests": provider_requests, | ||
| "machine_provider_v1_events": provider_events, | ||
| "provider_management_v1": management_operations, | ||
| } |
There was a problem hiding this comment.
The inline regex patterns in secondary_protocols() all require at least one of ,, (, or { immediately after the variant name. A Rust enum variant written without a trailing comma, tuple body, or struct body (a bare final variant, which is idiomatic at the end of a Rust enum) is silently excluded from the extracted set.
For ProviderRequest/ProviderEvent, r"(?m)^ ([A-Z][A-Za-z0-9]*)\s*(?:\(|\{|,)" misses a bare trailing variant. For Request (management), r"(?m)^ ([A-Z][A-Za-z0-9]*)\s*(?:\{|,)" also misses bare or tuple-only variants. For MessageKind, the discriminant pattern =\s*([0-9]+), misses a final variant with no trailing comma.
This is the exact same class of bug that was previously fixed in rust_enum_variants() (which now uses a lookahead to accept bare final variants) but the secondary protocol parsing was not updated to use the shared function. When a new bare-final variant is added to one of these Rust enums but not to the inventory, the checker silently passes rather than reporting the undocumented surface.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmux-tui/crates/cmux-tui-core/src/server.rs`:
- Around line 2248-2268: Update authorize_command_profile to fail closed when
authorization metadata is inconsistent: replace the expect on
command.provider_authority() with an authorization error return when it yields
None, and replace the unreachable! branch for unmatched LocalAdmin commands with
an error return. Preserve successful authorization for valid ProviderAuthority
and trusted LocalAdmin commands.
In `@cmux-tui/scripts/check-spec-inventory.py`:
- Around line 148-163: Update rust_function_body to skip Rust string and
raw-string literal contents while tracking braces, reusing the literal-handling
logic from strip_rust_comments and rust_raw_string_end. Count braces only
outside literals so profile() and metadata() bodies are extracted completely;
leave the escaped function-name regex unchanged.
- Around line 318-325: Update action_variants() to pass comment-stripped
config.rs content to rust_enum_variants, matching action_metadata() and the
later rust_enum_variants call used by validate_tui_actions. Keep the Action enum
parsing and inventory comparison unchanged apart from using the shared
stripped-source behavior.
In `@cmux-tui/scripts/test_check_spec_inventory.py`:
- Around line 52-175: Add negative-path tests in RuntimeMetadataTests for
duplicate command profile metadata, public events with no runtime stream, and
action metadata dispatch mismatches, asserting each checker path rejects the
invalid Rust metadata. Replace the repeated CHECKER.TUI save/restore try/finally
blocks in the affected tests with unittest.mock.patch.object(CHECKER, "TUI",
tui) context managers, preserving each test’s existing assertions and isolation.
- Around line 297-302: Strengthen the test around CHECKER.event_names() by also
asserting that the real “tree-changed” serializer and “workspace-added” delta
name are present. Keep the existing negative assertions for comment-only entries
and restore CHECKER.TUI in the finally block.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2e7f3c43-d318-48e2-b9f8-4e8455f16f7a
📒 Files selected for processing (5)
cmux-tui/crates/cmux-tui-core/src/server.rscmux-tui/crates/cmux-tui/src/app.rscmux-tui/crates/cmux-tui/src/config.rscmux-tui/scripts/check-spec-inventory.pycmux-tui/scripts/test_check_spec_inventory.py
| try: | ||
| names = CHECKER.event_names() | ||
| self.assertNotIn("comment-only", names) | ||
| self.assertNotIn("nested-comment-only", names) | ||
| finally: | ||
| CHECKER.TUI = original_tui |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Assert the positive control too.
The test only asserts absence, so an over-aggressive strip_rust_comments that blanks real code would still pass. The fixture already contains a live tree-changed serializer and a workspace-added delta name — assert they survive.
💚 Strengthen the assertions
names = CHECKER.event_names()
+ self.assertIn("tree-changed", names)
+ self.assertIn("workspace-added", names)
self.assertNotIn("comment-only", names)
self.assertNotIn("nested-comment-only", names)📝 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.
| try: | |
| names = CHECKER.event_names() | |
| self.assertNotIn("comment-only", names) | |
| self.assertNotIn("nested-comment-only", names) | |
| finally: | |
| CHECKER.TUI = original_tui | |
| try: | |
| names = CHECKER.event_names() | |
| self.assertIn("tree-changed", names) | |
| self.assertIn("workspace-added", names) | |
| self.assertNotIn("comment-only", names) | |
| self.assertNotIn("nested-comment-only", names) | |
| finally: | |
| CHECKER.TUI = original_tui |
🤖 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/scripts/test_check_spec_inventory.py` around lines 297 - 302,
Strengthen the test around CHECKER.event_names() by also asserting that the real
“tree-changed” serializer and “workspace-added” delta name are present. Keep the
existing negative assertions for comment-only entries and restore CHECKER.TUI in
the finally block.
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
# Conflicts: # .github/workflows/cmux-tui-spec.yml # cmux-tui/bindings/generate.sh # cmux-tui/crates/cmux-tui-core/src/server.rs # cmux-tui/crates/cmux-tui/src/app.rs # cmux-tui/crates/cmux-tui/src/config.rs # cmux-tui/scripts/check-spec-inventory.py # cmux-tui/scripts/test_check_spec_inventory.py # cmux-tui/spec/README.md # cmux-tui/spec/bindings.md # cmux-tui/spec/cli.md # cmux-tui/spec/commands.md # cmux-tui/spec/events.md # cmux-tui/spec/frontends.md # cmux-tui/spec/inventory.json # cmux-tui/spec/inventory.schema.json # cmux-tui/spec/machine-agent.md # cmux-tui/spec/native-frontend.md # cmux-tui/spec/programmability.md # cmux-tui/spec/terminal-host.md
1 participant
Summary
Validation
python3 cmux-tui/scripts/check-spec-inventory.pypython3 -m json.tool cmux-tui/spec/inventory.jsonpython3 -m json.tool cmux-tui/spec/inventory.schema.jsonpython3 -m py_compile cmux-tui/scripts/check-spec-inventory.pybash -n cmux-tui/bindings/generate.shactionlint .github/workflows/cmux-tui-spec.ymlgit diff --check HEAD^ HEAD --Spec and tooling only. No runtime reload required.
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Adds a machine-checked programmability inventory for
cmux-tuiwith a CI gate that prevents drift across commands, events, actions, menu actions, and routes. This update derives authority and routing from runtime-owned metadata, tightens binding requirements, and fixes a small CLI edge case.New Features
spec/inventory.json+inventory.schema.json, the checkerscripts/check-spec-inventory.pywith tests (autoreview regressions, trailing-enum coverage, routing-gap detection, and comment-only event filtering), a CI workflow, and abindings/generate.shprecheck; the checker reads command authority fromCommand.profile(), derivesAction/MenuActionexecution and classification from runtime enums, enforces exhaustive routing, and ignores comment-only wire events.native-frontend.md,terminal-host.md(v1),provider-management.md(v1), andprogrammability.md; updated commands/events withregistry_id,generation,terminal_revision, andshort_id; added subscribe deltas and new events (frontend-projection-changed,terminal-registry-changed,status,client-list-invalidated, pairing events); explicitly documented current protocol risks; clarified auth/pairing and socket resolution (XDG → TMPDIR →/tmpwith short-path fallback); CLI ids stay numeric-only; default sidebar view isworkspaces.XDG_RUNTIME_DIR/TMPDIR//tmpresolution andCMUX_TUI_SOCKET, and documents v5/v6 attach behavior. Provider mutations usemutation_idandexpected_versionfor idempotency. Inventory enforces authority profiles and stream routes;Action/MenuActionmust declare a route or be marked frontend-only.Migration
spec/inventory.json, declare the route inActionExecution/MenuActionExecution, and runpython3 cmux-tui/scripts/check-spec-inventory.py.set-client-sizingCLI fix (allow restoring all clients when enabling without a selector; require--clientwhen disabling).Written for commit 092a5f4. Summary will update on new commits.
Summary by CodeRabbit
Note
Low Risk
Primarily spec, schema, and CI tooling with no broad runtime changes; the only behavior tweak is a narrow CLI validation path for client sizing.
Overview
Introduces a machine-checked programmability inventory for
cmux-tuiso mux commands, wire events, TUI/menu actions, feature families, and secondary protocols cannot drift from runtime without failing CI.Adds
spec/inventory.jsonandinventory.schema.json, plusscripts/check-spec-inventory.py(with unit tests) that validate the catalog against Rust-owned metadata—command authority fromCommand.profile(), action/menu execution routes, exhaustive routing, and comment-only event filtering. A new.github/workflows/cmux-tui-spec.ymlgate runs the inventory checker, SDK schema/resource boundary checks, and generated wire-layer verification.Spec and binding contracts are expanded and corrected: new/updated normative docs (
programmability.md,terminal-host.md,provider-management.md,native-frontend.md, etc.), inventory fields likeregistry_id,generation, and subscribe/event deltas; bindings must provide typed coverage per command profile across the declared SDK set.Separately fixes a small
set-client-sizingCLI edge case: enabling sizing without a selector can restore all clients; disabling requires an explicit--client.Reviewed by Cursor Bugbot for commit 092a5f4. Bugbot is set up for automated code reviews on this repo. Configure here.