[lawrencecchen]

Summary

Follow-ups from the review of #34928 (set-layout-sizes, merged), points A to E of "Review (claude-reviewer-rds-batch): SHIP at ca012e6". No app path sends the command yet (STATUS 11385), so nothing a person sees changes.

• A. A value is exactly one pair. LayoutSizeWire keeps its two struct variants, now with #[serde(untagged, deny_unknown_fields)] on the enum. A value with both {split, ratio} and {pane, width}, or with any other field, is refused as a malformed request (bad request: ..., no error_code). Before, it decoded as a ratio, and the width was dropped silently, so the command answered ok:true for a half-applied value. An array value stays refused: serde gives untagged struct variants no sequence form. (My first version used newtypes over structs, which accept arrays; the review found it.)
• B. A NaN ratio is refused. An in-process caller of Mux::set_layout_sizes_in_transaction_checked_as with a NaN ratio got layout-sizes-not-persisted (the clamp keeps a NaN, and the store cannot persist one; JSON cannot carry a NaN). Now it gets LayoutSizesError::NanRatio (invalid-argument). An infinite ratio (JSON 1e39 reads as f32 infinity) clamps to 0.95, as set-split-ratio clamps it, which keeps commands.md's "same clamping and ranges". (My first version refused infinity too; the review found it.)
• C. Tests for the paths that had none: the ratio arm with the implied width from the batch's own earlier width, two screens, a batch of the current sizes (no change, no event), exactly one layout-changed event with the transaction echo, the capability in identify, a ratio value on the wire, and the refused shapes (both pairs, an array) with their bad request error.
• D. Docs. events.md lists set-layout-sizes among the commands whose transaction the layout-changed event echoes. commands.md states the strict value shape.
• E. LayoutSizeWire and its From impl move from server.rs to server/cmd_panes.rs (server.rs 3022 -> 3006 lines; still over its warn-only ceiling of 2978). The Command field keeps the type name, so check-sdk-schema.py maps it as before.

Testing

Focused commands: ./scripts/verify-cmux-tui-hosted.sh --filter layout_sizes and --filter a_non_finite_ratio_on_a_tree_split_is_refused (Linux test (linux)).

• Red at efe6dbf (test/rds-batch-followups-red-never-merge), run https://github.com/manaflow-ai/cmux/actions/runs/41810867146: set_layout_sizes_applies_a_trade_whole_or_refuses_it_whole FAILED (the both-pairs value was taken); 10 other tests passed.

• My first NaN test used a column split, where the implied-width check already refuses a NaN, so it passed before the fix and proved nothing. The tree-split test covers the path the review named: red at b19c850, run https://github.com/manaflow-ai/cmux/actions/runs/41813144619 FAILED; green at c7b0434, run https://github.com/manaflow-ai/cmux/actions/runs/41813156656.

• Review of this PR (SHIP at c7b0434 with should-dos): red at 20f0fa4 (test/rds-batch-r2-red-never-merge; the array value accepted, an infinite ratio refused), run https://github.com/manaflow-ai/cmux/actions/runs/41821943619; fix at d80a3c7, run https://github.com/manaflow-ai/cmux/actions/runs/41821947239 (--filter layout_).

• Green at 6002a22: runs https://github.com/manaflow-ai/cmux/actions/runs/41811365643 (layout_sizes, 11 passed), https://github.com/manaflow-ai/cmux/actions/runs/41811373538 (the wire test), https://github.com/manaflow-ai/cmux/actions/runs/41811381409 (the capability test).

• python3 cmux-tui/scripts/check-sdk-schema.py: ok (290 commands). PATH=/usr/local/go/bin:$PATH python3 cmux-tui/bindings/codegen/generate.py --check: ok.

• At the head d64337c (feat e8ff27a merged so that lint (linux) reaches this PR's crate; own-diff patch-id f97e5f1b4150 unchanged): run https://github.com/manaflow-ai/cmux/actions/runs/41825913708 (--filter layout): test (linux) passed, with a_nan_ratio_on_a_tree_split_is_refused_and_an_infinite_one_clamps and set_layout_sizes_applies_a_trade_whole_or_refuses_it_whole; lint (linux) passed.

Compile: fleet job ccc2b1a688d28cedf0989108 at the head d64337c: CmuxNext, app and cmux-tui passed (also 3d21132e0b1438b79e4e91d9 at d80a3c7 and 6817893c5323186f269cdb2d at 6002a22).

Observed / Not run

• Observed: the hosted runs, the schema and binding checks, and the fleet compile above. At the head lint (linux) passed. Before the feat merge it was red only on base findings: cmux-cloud-environment/src/lib.rs:262 (from feat(cloud): select NEXT backend at runtime in Rust #35133, fixed on feat by cmux-cloud-environment: collapse nested if (clippy base red) #35222) and the panic ratchet cmux-tui indexing_slicing 841 > 831 (all hits in crates/cmux-tui/src). clippy (changed cmux-tui packages) is red on crates/cmux-tui/src/cloud_environment.rs:22 (base); it checked cmux-tui-core with no error.
• Not run: macOS execution of the focused tests; the app side (KEYS owner).

Changelog

none

Proof

No capture: daemon command only; no app path sends it yet.

---

^{Open in [code]smith · Need help on this PR? Tag @codesmith-bot with what you need.}

---

Summary by cubic

Implements the review follow-ups for set-layout-sizes from #34928 and #35261: a value must be exactly one pair, and a non-finite ratio is refused. No app path sends the command yet, so nothing a person sees changes.

• A value with both {split, ratio} and {pane, width}, any other field, or an array is refused as malformed; previously it decoded as a ratio and silently dropped other fields.
• In-process callers get LayoutSizesError::NanRatio (invalid-argument) for a NaN ratio; an infinite ratio clamps to 0.95, matching set-split-ratio.
• LayoutSizeWire moves from server.rs to server/cmd_panes.rs.
• commands.md documents the strict value shape and events.md lists set-layout-sizes among commands whose transaction the layout-changed event echoes.

^{Written for commit d64337c. Summary will update on new commits.}

[github-actions]

cmux status _{(one comment, edited in place; each line is updated by the job that owns it)}

Review d80a3c7: HQ feed card · gallery before/after runs · checks

App of this head (app.zip, kept 1 day)

gh workflow run reload-build.yml -R manaflow-ai/cmux --ref feat-cmux-next -f tag=pr35261 -f ref=fix/layout-sizes-followups -f platform=macos -f backend_mode=local

web and size checks: green

Shipped in:

• cmux NEXT nightly 1.0.0-nightly.4184087390301 (08052d331e)
• cmux NEXT nightly 1.0.0-nightly.4184224384601 (a3abfc4160)
• cmux NEXT nightly 1.0.0-nightly.4184409870901 (78ee0e108e)
• cmux NEXT nightly 1.0.0-nightly.4184683079001 (64822b9434)
• cmux NEXT nightly 1.0.0-nightly.4184857252001 (8c9aae8b0d)
• cmux NEXT nightly 1.0.0-nightly.4185261809301 (697838c74d)
• cmux NEXT nightly 1.0.0-nightly.4185464793401 (68821b04eb)
• cmux NEXT nightly 1.0.0-nightly.4185545977501 (b659812803)
• cmux NEXT nightly 1.0.0-nightly.4185901282301 (b8d9ef9cab)
• cmux NEXT nightly 1.0.0-nightly.4185979991201 (8a134ee405)
• cmux NEXT nightly 1.0.0-nightly.4186744784501 (890be4f45a)
• cmux NEXT nightly 1.0.0-nightly.4186855704101 (53421cdccd)
• cmux NEXT nightly 1.0.0-nightly.4187086980301 (df4454e051)
• cmux NEXT nightly 1.0.0-nightly.4188039004101 (3e464b08ff)
• cmux NEXT nightly 1.0.0-nightly.4190094575701 (036f8aa538)
• cmux NEXT nightly 1.0.0-nightly.4191131679101 (e01bbae5c9)

[blacksmith-sh]

Found 11 test failures on Blacksmith runners:

Failures

Test	View Logs
/tmp/tmplszyfw48/summary-missing.json (live):/summary-missing.json (live):	View Logs
/tmp/tmps7eb3z3w/summary-fail.json (live):/summary-fail.json (live):	View Logs
[new_workspaces] debug.new_workspace timed 0 of 50 new workspace rows/ [new_workspaces] debug.new_workspace timed 0 of 50 new workspace rows	View Logs
[new_workspaces] the window draws no frames/[new_workspaces] the window draws no frames	View Logs
[tabs] debug.tab_switch timed 0 of 40 switches/ [tabs] debug.tab_switch timed 0 of 40 switches	View Logs
[tabs] the window draws no frames/[tabs] the window draws no frames	View Logs
imported web bundle inputs are not routed:/imported web bundle inputs are not routed:	View Logs
nightly cmux-tui resolve/nightly cmux-tui resolve	View Logs
nightly.yml must not read vars.NIGHTLY_SIGN while nightly-sign.yml is in shadow mode/ nightly.yml must not read vars.NIGHTLY_SIGN while nightly-sign.yml is in shadow mode	View Logs
test_the_committed_contract_is_the_workflows (main.RollupsFromNeeds.test_the_commit ted_contract_is_the_workflows)/ test_the_committed_contract_is_the_workflows (main.RollupsFromNeeds.test_the_commit ted_contract_is_the_workflows)	View Logs

...and 1 more test failures. View all on Blacksmith

^{Need help on this PR? Tag @codesmith-bot with what you need.}

[lawrencecchen]

Review (claude-reviewer-rds-batch): SHIP at c7b0434
reviewer-session: 4b6c20a4-1d7d-411c-ae1a-b1f27e82b482
R9: pass | The Layout row keeps one writer. The batch still commits only through the daemon plan screen.layout_sizes.set. This PR changes only how a value decodes, and adds one refusal before the plan stages anything.
perf claim: none

No defect must be fixed before merge. The four findings below are should-do. I read the full diff at the head, the serde 1.0.229 and serde_derive 1.0.229 code that these derives expand to (the versions in cmux-tui/Cargo.lock), the single-command paths, and the CI logs named below.

Verified

1. A: a value with both pairs, or with another key, is refused. With deny_unknown_fields, the generated field visitor has no ignore arm, so a key that the struct does not own is an unknown_field error (serde_derive de/struct_.rs:682, de/identifier.rs:268). {split, ratio, pane, width} fails SplitRatioWire on pane and PaneWidthWire on split, and the untagged enum then answers "data did not match any variant". {split, ratio, extra} fails both in the same way. {split, ratio} and {pane, width} still decode.
   • The field types do not change. The old and the new form both decode split/pane as u64 and ratio/width as f32 through the same ContentRefDeserializer. A float id (4.0), a string ("4") or null is refused as before. An integer ratio (1) is still read as 1.0.
   • The refusal is a request-level decode error: handle_message -> send_bad_request answers {"id":44,"ok":false,"error":"bad request: ..."} with no error_code, and nothing runs. This agrees with commands.md:32 and with the new sentence at commands.md:2895, except for the array shape in finding 1.
2. Schema and bindings. LayoutSizeSplitRatio and LayoutSizePaneWidth have additional_properties: false, and the server now agrees with them (before, it ignored extra keys). check-sdk-schema.py maps the Command field by its type name, and SetLayoutSizes { values: Vec<LayoutSizeWire> } in server.rs did not change, so the move has no effect on it. sdk-schema.json and api-compat.surface.json did not change, so no binding must be generated again. I ran generate.py --check (ok, all languages) and check-sdk-schema.py (ok: 290 commands, 90 events, 232 named types). Both variant types have only their two required fields, so no generated encoder sends another key. No app path or other in-repo client sends set-layout-sizes yet, and remote clients cannot send it (remote_relay/mod.rs:327; no lane allowlist names it).
3. NonFiniteRatio. The check runs before clamp_split_ratio. That order is necessary, because f32::clamp keeps a NaN. code() (invalid-argument), index() (Some(index)) and Display (value <index>: ...) cover the new variant. Only mux/layout_sizes.rs matches on LayoutSizesError; responses.rs:94 only calls code(). No other crate, FFI surface, Swift file or spec table names the type, and the code is an existing string. So nothing else needs the variant.
4. Tests and red/green.
   • Red 41810867146 at efe6dbf (test job 126509509217): set_layout_sizes_applies_a_trade_whole_or_refuses_it_whole FAILED at layout_receipts_shutdown.rs:270 (left: Bool(true)), so the both-pairs value was taken. The other 10 tests passed, including a_non_finite_ratio_is_refused_and_changes_nothing, as c7b0434 says.
   • Green at 6002a22: 41811365643 (11 of 11), 41811373538, 41811381409.
   • Red 41813144619 at b19c850 (efe6dbf plus the c7b0434 test, the same diff): a_non_finite_ratio_on_a_tree_split_is_refused FAILED at layout_sizes.rs:300 (left: None, right: Some(0)). Green 41813156656 at the head. Finding 2 says what this red shows.
   • a_ratio_value_uses_the_widths_the_batch_set_before_it is a real guard. With the widths before the batch (the first column is 1.0 after two_columns, as the feat(cmux-tui): set-layout-sizes commits a divider batch as one change or none (layout-sizes-batch-v1; daemon only, no app caller yet) #34928 red output shows), a 0.5 ratio implies a third column wider than 1.0. So a batch that used the old widths is refused, and the unwrap panics.
   • values_on_two_screens_are_refused_whole checks the variant, the index and the code.
   • Both event tests are deterministic. mux.subscribe() runs after the setup and makes an empty mailbox with no replay (event_bus.rs:194-201). The batch sends its LayoutChanged synchronously, before it returns (layout_sizes.rs:186). Between the subscribe and the drain, nothing else runs a command that sends one. So no setup event can be in the channel, and try_recv after the call sees all that the batch sent.
5. Repo rules. The PR adds no CI job, gate, perf row or budget. The tests are in the existing cmux-tui-core test modules that test (linux) runs. Red-first holds: the test commit comes before the fix, and the later tree-split test has its own red run on a never-merge branch. server.rs goes from 3022 to 3006 lines (the god-file step says "this change did not grow it"), and the other changed files are under their budgets. No feat commit after 4b1b786 touches these files, and the head merges cleanly with the feat tip b9aed49.
6. The reds at the head are base findings.
   • clippy (changed cmux-tui packages): only crates/cmux-tui/src/cloud_environment.rs:22 (needless_borrow, from feat(cloud): select NEXT backend at runtime in Rust #35133, still on the feat tip). The same job ran clippy -D warnings --all-targets on cmux-tui-core with no finding, so the new code is clippy-clean.
   • lint (linux): clippy stops at cmux-cloud-environment/src/lib.rs:262 (collapsible_if, from feat(cloud): select NEXT backend at runtime in Rust #35133), and the panic ratchet is red on cmux-tui: clippy::indexing_slicing 841 > 831, with every listed hit in crates/cmux-tui/src (this PR does not touch that crate). cargo fmt --check passed. The cmux-tui-core panic-ratchet rows are warnings, not red, and the new library code adds no arithmetic and no indexing.
   • cmux-next checks: check-default-cursor (CmuxNextDesign/Debug/NSCursor+DebugName.swift:10) and the base-red classifier. nightly-guards: the release-notary workflow tests. This PR touches none of these files.

Findings (all should-do)

1. server/cmd_panes.rs:35-53: a positional array value now decodes, as a ratio. A plain derived struct also gets a visit_seq, and ContentRefDeserializer::deserialize_struct accepts Content::Seq (serde private/de.rs:2349-2362). The old untagged struct variants had no visit_seq (serde_derive de/struct_.rs:70-73), so they refused an array.
   • Case: {"cmd":"set-layout-sizes","values":[[S,0.5]],"transaction":1} now applies {split:S, ratio:0.5} and answers ok:true. At the base it was bad request. [[P,0.4]], meant as a width, becomes a ratio on split P and answers layout-ratio-target-missing.
   • No client sends this today, so nothing breaks. But the PR title and commands.md:2895 ("a value that is not exactly {split, ratio} or {pane, width} ... is refused") are not true for this shape, and the schema has no array form.
   • Fix: go back to the two struct variants and put the attribute on the enum, #[serde(untagged, deny_unknown_fields)]. serde_derive gives the enum's container attributes to each untagged struct variant (de/enum_untagged.rs calls struct_::deserialize(.., cattrs, StructForm::Untagged(..))), so unknown keys are refused and there is still no visit_seq. I read this in serde_derive 1.0.229; I did not compile it. The newtypes and the destructuring From then go away. Add [[split, 0.5]] and {"split": .., "ratio": .., "extra": 1} to the wire test (ok:false, widths unchanged).
2. mux/resource_topology/layout_ops.rs:439-442 and mux/layout_sizes.rs:35-36: an infinite ratio is now refused, but set-split-ratio clamps it, and JSON can carry one. serde reads a JSON number into f32 with v as f32 (serde_core de/impls.rs:222-238), so "ratio": 1e39 (any value above f32::MAX) arrives as f32::INFINITY.
   • Case: {"cmd":"set-split-ratio","split":S,"ratio":1e39} sets 0.95 and answers ok:true, because set_split_ratio_inner clamps first (mux/layout_resize.rs:114). The base batch did the same. At the head, {"cmd":"set-layout-sizes","values":[{"split":S,"ratio":1e39}],"transaction":1} answers invalid-argument, "value 0: the split ratio is not a finite number".
   • This is against commands.md:2882 ("Every value means what its single command means, with the same clamping and ranges"), docs/protocol.md:187 ("Ratios are clamped to 0.05..0.95") and the schema's clamped_maximum. The comment "JSON cannot carry one" is wrong, and commands.md does not list this refusal.
   • A correction to my own feat(cmux-tui): set-layout-sizes commits a divider batch as one change or none (layout-sizes-batch-v1; daemon only, no app caller yet) #34928 review B: the base batch did not commit a NaN. The store refused it (workspace_registry/resource_store.rs:2239, resource_screen.rs:155), and the batch answered layout-sizes-not-persisted and sent a Status event. That is why red 41813144619 fails on index() == None. Its INFINITY iteration never ran, because the NaN iteration panicked first. So the base defect for NaN was the wrong error, and the head fixes that. The PR body sentence "with a NaN or infinite ratio committed it" is not correct.
   • Fix: refuse only a NaN (ratio.is_nan()), and let an infinite ratio clamp as the single command does. In a_non_finite_ratio_on_a_tree_split_is_refused, keep the NaN case (assert NonFiniteRatio { index: 0 }) and expect INFINITY to set 0.95. Correct the comment and the PR body.
3. Tests that do not pin what they name.
   • mux/tests/layout_sizes.rs:176 passes before and after the fix (the implied width refuses the value). Assert matches!(error, LayoutSizesError::NonFiniteRatio { index: 1 }), so that it guards the new refusal.
   • mux/tests/layout_sizes.rs:253 counts LayoutChanged events but does not check the echo. Assert that the one event carries the transaction "19" (my feat(cmux-tui): set-layout-sizes commits a divider batch as one change or none (layout-sizes-batch-v1; daemon only, no app caller yet) #34928 review C asked for this, and events.md now promises it).
   • No wire-level test sends a {split, ratio} value, so the SplitRatioWire decode has no runtime cover. Add one to the ok:true request in set_layout_sizes_applies_a_trade_whole_or_refuses_it_whole.
   • In that test, assert that both["error"] starts with "bad request" and that error_code is absent, as commands.md now says.
4. PR body, "Observed". It says that lint (linux) is red only on cmux-cloud-environment/src/lib.rs:262 and that base red: panic ratchet rows from #35133, #34360 and #35138 (cmux-tui indexing_slicing 841 > 831) #35180 fixes it. The same job is also red on the panic-ratchet row cmux-tui indexing_slicing 841 > 831. cmux-cloud-environment: collapse nested if (clippy base red) #35222 (6ab258d) already fixed lib.rs:262 on feat. clippy (changed cmux-tui packages) is red on crates/cmux-tui/src/cloud_environment.rs:22, and base red: panic ratchet rows from #35133, #34360 and #35138 (cmux-tui indexing_slicing 841 > 831) #35180 does not touch that file (it changes src/cli/cloud_environment.rs). All three are base findings (item 6 above). Correct the line and link them in a PR comment.

Observed / Not run

• Observed: the full diff; the serde and serde_derive sources named above; the single-command paths; the logs of test jobs 126509509217, 126511295894, 126511218460, 126511239466, 126517170711 and 126517220698, of lint job 126517213102 and of the failing head checks; fleet job 6817893c5323186f269cdb2d (cmux-next-swift-check at 6002a22, done). I ran generate.py --check and check-sdk-schema.py.
• Not run: no cargo, rustc or swift on this machine. I did not run the array case or the 1e39 case; they come from reading the serde code and the call paths. The macOS lint and test job at the head is still queued.

[lawrencecchen]

Review (claude-reviewer-rds-batch): SHIP at d64337c
reviewer-session: 4b6c20a4-1d7d-411c-ae1a-b1f27e82b482
R9: pass | The Layout row keeps one writer. The batch still commits only through the daemon plan screen.layout_sizes.set.
perf claim: none

This is a review of the push after my SHIP at c7b0434. I read git diff c7b04340dab d80a3c7f088 in full, and I checked the merge. The four should-do findings are all addressed. I have no new finding.

Verified

1. Findings 1 and 2 (d80a3c7).
   • LayoutSizeWire is again two struct variants, now with #[serde(untagged, deny_unknown_fields)] on the enum (server/cmd_panes.rs:35-40). An untagged struct variant gets the enum's container attributes and no visit_seq, so the server refuses an extra key and an array. The runtime test confirms both: at the red commit, the array value (id 46) answered {"data":{},"id":46,"ok":true}; at the head, it answers bad request: ....
   • Only a NaN is refused now (NanRatio, invalid-argument, before the clamp, layout_ops.rs:439-443). An infinite ratio clamps to 0.95, as set-split-ratio does. So the commands.md sentence "the same clamping and ranges" is true again, and commands.md needs no change. The comments now agree with the code: JSON cannot carry a NaN (serde_json refuses the token, and a finite f64 cast to f32 is never NaN), and 1e39 arrives as an infinite f32 that clamps.
   • code(), index() and Display cover the renamed variant. No other file names it.
2. Finding 3 (tests). Both NaN tests assert NanRatio { index }. The event test asserts that the one layout-changed event echoes "19". The wire test now sends a {split, ratio} value (ok:true, and the second column becomes as wide as the first). It also asserts that a both-pairs value and an array value answer bad request: ... with no error_code and change no width.
3. Red and green.
   • Red 41821943619 at 20f0fa4 (c7b0434 plus the tests only; test job 126547095045): 2 FAILED, 132 passed. a_nan_ratio_on_a_tree_split_is_refused_and_an_infinite_one_clamps failed at layout_sizes.rs:310 ("an infinite ratio is clamped ...: NonFiniteRatio { index: 0 }"). set_layout_sizes_applies_a_trade_whole_or_refuses_it_whole failed at layout_receipts_shutdown.rs:292 (the array value was taken).
   • Green 41825913708 at the head (test job 126560523414): all 12 layout-sizes tests pass, and every test result in the job is ok. lint (linux) (job 126560523372) passed. Its clippy -D warnings step now reaches cmux-tui-core and is clean, and the panic ratchet passes.
   • The fix commit made the NaN assertions stricter (NanRatio did not exist before it). The NaN refusal itself already had a red run at b19c850 (index() was None before the first fix).
4. The merge (d64337c). It is a clean automatic merge: git merge-tree --write-tree d80a3c7f088 e8ff27a312d gives the head's tree (6dd73b323ba). The own diff has the patch-id f97e5f1b4150 before and after the merge. The merge adds no change of the author's.
5. Compile. Fleet job ccc2b1a688d28cedf0989108 (cmux-next-swift-check at the head) is done, and Rust MSRV (1.91) passed at the head.
6. Checks at the head. No red comes from this PR's diff (cmux-tui-core Rust and two spec files):
   • cmux-next checks: Default cursor, the base-red classifier, and Web bundle key inputs (schemas/design-scale/design-scale.json from Design system foundation: one token source for Tailwind, core controls, token sheet #33787 is not routed; it came in with the feat merge).
   • nightly-guards: the release-notary workflow tests.
   • publish to R2: "API rate limit exceeded" when it saved an attestation. This is an infrastructure failure; run the same SHA again.
   • cmux-next swift test and cmux-next fast tier are pending.

Observed / Not run

• Observed: the diff c7b0434..d80a3c7, the merge tree, the logs of jobs 126547095045, 126560523414 and 126560523372, the logs of the failing head checks, and the fleet job.
• Not run: no cargo, rustc or swift on this machine.