Repository navigation
Tab drag: forward index decisions to Rust (migration shard 002) - #34545
Conversation
Co-authored-by: Codex <199175422+chatgpt-codex-connector[bot]@users.noreply.github.com> Agent-Session: https://cmux.com/deeplink/agent?id=aid-0ecf34a54b76bbeec642
|
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)
web and size checks: green Shipped in:
|
Co-authored-by: Codex <199175422+chatgpt-codex-connector[bot]@users.noreply.github.com> Agent-Session: https://cmux.com/deeplink/agent?id=aid-0ecf34a54b76bbeec642
Co-authored-by: Codex <199175422+chatgpt-codex-connector[bot]@users.noreply.github.com> Agent-Session: https://cmux.com/deeplink/agent?id=aid-0ecf34a54b76bbeec642
…ag-policy-port-002
teamleaderleo
left a comment
There was a problem hiding this comment.
Review (claude-opus-5.5-medium-router-review): SHIP at 51a158b
teamleaderleo
left a comment
There was a problem hiding this comment.
Review (claude-opus-5.5 medium, router-review): SHIP at 51a158b
teamleaderleo
left a comment
There was a problem hiding this comment.
Review (claude-opus-5.5 medium, router-review): SHIP at 51a158b
|
Owned by an agent session · session |
Co-authored-by: Codex <199175422+chatgpt-codex-connector[bot]@users.noreply.github.com> Agent-Session: https://cmux.com/deeplink/agent?id=aid-462643789e72e77eafeb
|
Found 3 test failures on Blacksmith runners: Failures
|
|
The cmux-ci parity job |
|
Run 41821132135 dispatched and finished: failure in |
teamleaderleo
left a comment
There was a problem hiding this comment.
Review (subagent claude-opus-5-5 medium): SHIP at 9ed0723
Independent Opus review of the full diff from merge base ab7e724.
- Parity:
wireIndexmatches the original for every defined input. The one difference is thatfinal == Int.maxwithfinal > currentnow saturates instead of trapping, which is strictly safer; the test excludes that case correctly.cmux_tab_move_pane_final_indexmatches the original step by step: remove moving, clamp-insert, find the first later shown id present in pane-minus-moving, else count.cmux_tab_move_group_indexmatches the original's clamp-prefix count of members.clamp_indexhandlesInt.min/Int.maxwithout wrapping. - Identity: the call-local
StringInternerkeys on SwiftStringhashing and equality, so canonically equivalent strings and embedded NULs get the same id across display, pane, members and moving. The tests cover this across buffers. Members missing from display get fresh ids and cannot collide. - Test: the oracle is the verbatim body from 448f224; it was diffed against that commit and the only changes are the name and visibility. The sweep covers 12 sequences × 12 sequences × 8 ids × 9 indices, plus every subset of 8 members. Focused fleet parity step exit 0 at this head.
- FFI:
withUnsafeBufferPointerkeeps the arrays alive for the call. An empty array gives a nil base address with count 0, whichids()accepts. A non-empty array always has a base address, so the-1sentinel can't reach callers. Rust doesn't keep the pointers and has no panic path (the insert and slice bounds are clamped). - Concurrency: pure, nonisolated, no shared state.
- User-visible: none. Costs one small dictionary per drop.
- Nit (non-blocking): the same import-style inconsistency with #34563 (
internal importhere,@_implementationOnlythere) inCmuxNextBridge.
Evidence: fleet compile check dd5c9e887cfd77c489b709aa done at this head (package scope); parity step c2faad7f8ac29ccdd06d64f2 exit 0 at this head; cargo test for cmux-layout-reducer-ffi passes (74 tests, tab_drag 4/4).
Bugbot needs on-demand usage enabledBugbot uses usage-based billing for this team and requires on-demand usage to be enabled. A team admin can enable on-demand usage in the Cursor dashboard. |
A newer verdict blocks 8db933f (no independent SHIP at the head).
|
Review (claude/opus): SHIP at 8db933f Independent review: the three Rust index decisions match the prior Swift behavior across boundary, duplicate, Unicode/NUL, empty-list, and extreme-index cases; the ABI types and pinned FFI exports line up; public Swift signatures are unchanged; and the parity suite covers the migrated behavior. Non-blocking nits: the test import appears unused, and the PR body can be refreshed now that dependencies and gates are on base. |
…ag-policy-port-002
Bugbot needs on-demand usage enabledBugbot uses usage-based billing for this team and requires on-demand usage to be enabled. A team admin can enable on-demand usage in the Cursor dashboard. |
|
Review (claude/opus): SHIP at 2981fa6 Independent exact-head review: all three index decisions match the prior Swift behavior across boundaries, duplicates, Unicode/NUL, empty arrays, and extreme indices; the ABI/header and updated FFI pin line up; public APIs are unchanged; and parity coverage is comprehensive. The base merge leaves the migrated files correct. Non-blocking nit: the test file's CmuxNextAppCore import appears unused. |
![Fix with [code]smith](https://pr-comments-assets.blacksmith.sh/codesmith/fix-with-codesmith-light-3x.png)
TabMoveIndex now forwards all three index decisions to Rust through the existing public Swift signatures. A call-local dictionary assigns integer IDs using Swift String equality, preserving canonically equivalent Unicode and embedded NULs across every input buffer.
Leo’s request: “A Rosie-style migration harness for large code moves ... pick one Swift-logic-to-Rust move ... run it through the harness, and land the shards.”
This is shard 002 of the two-shard pilot. It stays on by default, preserves defined behavior, and can be rolled back by reverting this PR. The new parity suite compares against the original Swift bodies from 448f224, covering reordered and hidden tabs, duplicates, empty arrays, Unicode/NUL identity, group membership subsets, and extreme signed indices. The original Int.max increment trap is excluded from parity.
Depends on #34507 and the subsequent app-FFI pin. After both land, this branch must merge the current base before independent review. The migration owner runs the fixed build and parity gates and performs the merge.
Validation
9ed07234a434be2ce53f505fd36092edecd67b20(jobdd5c9e887cfd77c489b709aa).TabMoveIndexRustParityTestspassed at the exact head (fleet stepc2faad7f8ac29ccdd06d64f2).Performance
These are policy ownership counts, measured by inspecting the same TabMoveIndex source before and after; they are not runtime latency measurements.
Migration throughput report: awaiting the migration owner’s supplied report link.
Prior art
Read the original wire conversion from #15555, the daemon matrix conversion from #20253, and the source-path group-index fix at 19700ba. The path has no newer changes in the last 14 days. Read #34507’s header, index implementation, and Bridge dependency change. The prior-art and overlap helpers hit the shared GitHub quota; the available overlap cache is partial and cannot establish complete current open-PR coverage.
Changelog
none
🤖 Generated with Codex
Agent session: agent session
Open in [code]smith · Need help on this PR? Tag
@codesmith-botwith what you need.Summary by cubic
Forwards all three
TabMoveIndexindex decisions to Rust through the existing public Swift API. A call-local nonisolated string interner maps SwiftStringvalues to integer IDs, preserving canonical Unicode equivalence and embedded NULs without crossing the ABI. A parity suite compares against the original Swift bodies from the pre-move commit, covering reordered and hidden tabs, duplicates, empty arrays, Unicode/NUL identity, group membership subsets, and extreme signed indices; theInt.maxincrement trap case is excluded.This is shard 002 of the migration pilot. It stays on by default and preserves defined behavior; reverting it restores the Swift implementations. It depends on the prerequisite PR and app-FFI pin, and the branch has merged the current
feat-cmux-nextbase.Validation
Performance
Written for commit 2981fa6. Summary will update on new commits.