Repository navigation
ClosingPanes rules into cmux-layout-reducer (closing_panes) over the app FFI, step A: Rust port, C ABI, Swift split and parity corpus (no behavior change) - #35886
Conversation
…, the geometry gathered in the wrapper; the rules ported to cmux-layout-reducer closing_panes with a C ABI (cmux_closing_panes_*); a seeded corpus test in both (no behavior change) Move 3 of the swift-logic plan (lead order): the springing-close rules of FRAME OWNER S3c/S3d leave Swift in two steps (the repo's app FFI path, as #35848). This is step A's code; the corpus of answers is recorded from the Swift rules on the fleet in the next commit. - ClosingPanesRules (Swift): the former ClosingPanes bodies with the geometry reads replaced by inputs, in the same order of operations; panes, scroll areas and row columns are numbers the wrapper hands out. The wrapper keeps ClosingPanes' API and every call site. - cmux-layout-reducer closing_panes: the same rules in Rust (Swift's min, max and Sequence.min/max kept, so ties and NaN answer the same), with unit tests. The per-frame rects reuses the handle's buffers (no allocation in steady state). - cmux-layout-reducer-ffi closing_panes: one handle per screen, typed calls, no JSON; begin and remove return 1 for an answer and 0 for none (documented beside OK), outputs written only on an answer, the Safety text forbids an out pointer overlapping an input; struct sizes and field offsets tested. - ClosingPanesCorpusTests (Swift), closing_panes_corpus (Rust) and the FFI test replay the same corpus: each call's answer and a hash of the whole state after it, bit for bit. The neighbour lists were already sorted by pane id; the gaps' min and max were read in Dictionary order (seeded per process) and are now read in pane order: only the sign of a zero gap could differ, never a value.
…losing_panes_state_hash), so step B's Swift test still compares the whole state after each corpus call
…ose rules for 160 seeded scripts, 1,767 calls (recorded on the fleet: step cf0ccca08931af31dadbcbf3 at a5b46f9, artifact sha256 1e485a58030f)
… unit test's expectation); clippy: plain boolean forms and Sequence.min(by:) as a loop (no behavior change; the corpus passed through both)
|
cmux status (one comment, edited in place; each line is updated by the job that owns it) Review App of this head (app.zip, kept 1 day)
red: cmux-next checks (god files, concurrency, crash safety, l10n); NEW: green on the base 7a89fde; do not merge until green; see lane-rules Shipped in:
The feat-cmux-next push run https://github.com/manaflow-ai/cmux/actions/runs/41927927728 at b5e96a1 failed: cmux-next checks (god files, concurrency, crash safety, l10n). @lawrencecchen |
|
Found 6 test failures on Blacksmith runners: Failures
|
|
Review (independent review subagent): FIX FIRST at 97eff4c One red of this PR's own blocks it. The port is otherwise correct and the evidence holds. Push the one-line fix (and the NaN guard below in the same push), then this is SHIP on that head if the ratchet goes green and nothing else changes. Blocking
Should fix in the same push (not blocking alone)
Checks
Not run by me: no build or test. I read the diff, the base, step B and the proof branch, and read the CI and fleet logs named above. |
…in Swift (red: Rust skips only abs < 0.5, so NaN travels)
…s; read the close order with get() (panic ratchet: indexing_slicing 70 > 69)
|
Review (independent review subagent): SHIP at be70ab9 I checked the delta The delta
Other comparisons checked for NaNEvery other comparison has the same form in Rust and Swift:
None differs on NaN. CI at this head
Carried over from my earlier review (not blocking step A)
Not run by me: no build or test. I read the delta, the red and green fleet logs (cmuxterm-hq runs 41920927806 and 41920927525) and this head's CI logs. |
![Fix with [code]smith](https://pr-comments-assets.blacksmith.sh/codesmith/fix-with-codesmith-light-3x.png)
Summary
Move 3 of the swift-logic plan (lead order): the springing-close rules of FRAME OWNER S3c/S3d (
ClosingPanes: which panes border a closing pane and the gaps it keeps, where each side shows per frame, the camera carries, the panes nothing moves over, a taken-back close's velocities) move tocmux-layout-reducer(closing_panes), called throughcmux-layout-reducer-ffi(cmux_closing_panes_*). This is step A of the repo's two-step app FFI move (as #35848): the Rust rules, the C ABI and the parity proof land now; the app keeps calling the Swift rules until theCCmuxAppFFIrelease that carries the new symbols is pinned. Step B (branchswift-logic/closing-panes-ffi-switch, head 189585c, ready) then switches Swift to the C calls and deletes the Swift rule copy.Owner: PL-motion-core (layout motion, OWNERS.md; FRAME OWNER S3c/S3d by Lawrence Chen, #34170, #35140); you may take this over.
Red test first: I read
ClosingPanesfor a real defect (NaN or negative frames, a kept handle off the pixel grid, row-column scroll off-by-one, left/right rounding asymmetry) and found none that changes behavior: rects are clamped withmax(0, ...), pixel placement is the caller's (pixelEdges), and the scroll travel restarts from where it is. So this move is behavior-neutral and proven by a corpus, not by a red test. One order dependence existed: the gaps'min()/max()were read in Dictionary order (seeded per process); they are now read in pane order (the neighbour lists were already sorted). Only the sign of a zero gap could differ, never its value.What changes:
ClosingPanes+Rules.swift(ClosingPanesRules): the formerClosingPanesbodies with the geometry reads replaced by inputs, same order of operations; panes, scroll areas and row columns are numbers the wrapper hands out.ClosingPanes.swiftkeeps its API and every call site; it gathers the inputs and maps the answers.ClosingPanesCorpusTests: 160 seeded scripts of closes (1,767 calls: 307 begin, 287 settle, 702 rects, 118 carry, 75 settle-carried, 125 row stops, 109 take-backs, 44 drops), recorded from the Swift rules on the fleet. Each line has the call's answer and an FNV-1a hash of the whole state after it (every side's neighbours, gap, place, travel start and end, carry, row travel, close order, awaiting set), so a difference anywhere in the state fails the line. Answers and state as IEEE-754 bit patterns.cmux-layout-reducer/src/closing_panes.rs: the rules in Rust, one operation at a time (Swift'smin/maxandSequence.min()/max()/min(by:)kept, so ties and NaN answer the same), with unit tests.rects(per display frame during a close) reuses the handle's buffers: no allocation in steady state.cmux-layout-reducer-ffi/src/closing_panes.rsand the header: one handle per screen, typed calls, no JSON. From Keyboard resize rules in Rust: cmux-layout-reducer shown_resize over the app FFI, proven equal to Swift on a 3,203-case corpus (step A) #35848's review notes: outputs are written only on an answer;begin/removereturn 1 for an answer and 0 for none, documented besideCMUX_LAYOUT_REDUCER_OK(also 0); the Safety text forbids anoutpointer overlapping an input; struct sizes and field offsets are tested.Numbers
swift-logic-lines.py(swift_logic_lines):ClosingPanesRulesbecomes onecmux_closing_panes_*call per method (PLATFORM row, rule c);ClosingPanesis MIXED 1 (its live-frame "moving" test stays Swift).Changelog
none
Observed / Not run
Observed (fleet):
fix/closing-panes-record-nomerge, this tree withrecord = true; never lands):recordCorpuspassed; artifact sha256 1e485a58030f, the committed fixture. A first attempt (9332b0b03dcf) was cancelled by the worker (its same-tree cmux-tui build was terminated), not by a test.ClosingPanesCorpusTests,PaneCloseKeptHandleTests,ClosedPaneContentStaysUntilLandingTests,PaneContentPolicyTests,PaneGroundCoversPaneTests: all 5 suites passed.cmux-tui-rust-check.sh test every_call_of_the_corpusstep 77cce0044edb16fff9d4722e:closing_panes_corpuspassed (answers and state, bit for bit);test closing_panesstep c6c8c577faddb37a00444a42: 5 of 5 unit tests passed;clippystep f3525fd319e219ea600a0198: green.test/closing-panes-ffi-proof-never-merge: this head plus the FFI crate in the cmux-tui workspace, as Keyboard resize rules in Rust: cmux-layout-reducer shown_resize over the app FFI, proven equal to Swift on a 3,203-case corpus (step A) #35848's proof; never lands): corpus through the C calls (step 54801c299cc2d36ec92dca35), null and unknown-kind refusals (06cd9119083d47d4cafef4f8), sizes and field offsets (008991d207639346d93eca03): all passed; clippy step 0ded8428bd69816e8e654345: green. This PR's own CI runs the FFI crate's tests in its own workspace.min(0, -0)to be-0(Swift keeps its first argument; the rule was right, the expectation wrong); clippy asked for plain boolean forms and a loop formin(by:)(no behavior change; the corpus passed through both).rectsisperf-hot: 4ms motion-frame):PaneMotionBench(CMUX_PANE_BENCH=1) at the base and this head on one worker (aws-m4pro-1), steps 94e030691940801b9534b97d and d0f1866cb1acee3f3a5e6f03: 0 frames over 8.33 ms in both; 30 panes close p95 1.072 vs 1.024 ms, columnClose p95 0.908 vs 0.922 ms, max 3.5 vs 3.3 ms. An earlier unpinned run of this commit on another worker (fbc2ddf437a9, aws-m4pro-1 vs the base on aws-m4pro-3) went over 8.33 ms incolumnandcolumnClose;columndoes not run ClosingPanes, and the pinned rerun is clean, so it was worker load.python3 scripts/cmux-next/prepush-gate.py: no new violations.cmux-next checks(step A of the two-step path; cmux-app-ffi: generate cmux_motion_ffi.h with cbindgen, fail on drift #35307, feat(tab-drag): expose index and reveal rules over app FFI #34507, cmux-rd: one typed app line for cmux rd serve|connect, read back through a C line reader (ABI 5; Mac Screen step A) #35828 landed the same way). The tip shows it until the repin PR lands.Not run:
pane-perf-terminal.pyandanim-perf-terminal.py(the motion-frame bench's terminal and Chromium rows, light and stress): not run on this head. Correction (review 6643086790): this head does change the per-frame Swift path a little:rectsnow builds a[FrameFact]array (with a per-framemovingvalue it does not use) and a row set, and looks up each neighbour edge linearly where it used a Dictionary; PaneMotionBench on one worker (base 94e030691940801b9534b97d, head d0f1866cb1acee3f3a5e6f03) keeps that in noise: 0 frames over 8.33 ms, 30-pane close p95 1.024 -> 1.072 ms, columnClose p95 0.922 -> 0.908 ms; they are due at step B, which switches the per-frame call to the FFI.CCmuxAppFFIpin (it imports the new symbols).Open in [code]smith · Need help on this PR? Tag
@codesmith-botwith what you need.Summary by cubic
Ports the springing-close rules (
ClosingPanes: which panes border a closing pane, the gaps it keeps, where each side shows per frame, the carries, take-backs) from Swift tocmux-layout-reducer(closing_panes), exposed through a C ABI (cmux_closing_panes_*) incmux-layout-reducer-ffi. Step A of a two-step move: the Rust rules, C ABI and parity proof land now; the app keeps running the Swift rules until step B switches it to the C calls.ClosingPanes+Rules.swift(the former bodies over plain numbers) andClosingPanes.swift(a wrapper that gathers inputs and maps answers), with no change to the API or call sites.min/maxtie and NaN behavior (a NaN row travel now means no travel, matching Swift);rectsreuses the handle's buffers, so steady state allocates nothing.beginandremovereturn 1 for an answer and 0 for none, and outputs are written only on an answer.get()instead of indexing to keep the panic ratchet (indexing_slicing70 > 69).Written for commit be70ab9. Summary will update on new commits.
Review fixes (FIX FIRST at 97eff4c, comment 6643086790)
closing_panes::tests::a_nan_row_travel_is_no_travel_as_in_swift. Fleet step 5301bde55a53ab7c139a6da5 (Linux,cmux-tui-rust-check.sh test a_nan_row_travel_is_no_travel): FAIL,assertion left == right failedat closing_panes.rs:779 (Rust took a NaN row travel; Swift'sguard abs(...) >= 0.5 else { continue }skips it).let moves = abs >= 0.5; if !moves { continue }) and reads the close order withget()(the panic ratchet's newindexing_slicinghit at closing_panes.rs:551). Fleet step c52fcf7cae91ab1337219d0a (test closing_panes): 6 tests run, 6 passed. The PR's ownclippy (changed cmux-tui packages)job, whose Rust panic ratchet step runs with--compare-base, passes at this head.