Skip to content

cmux-layout-reducer-ffi: bookmarks_apply, the bookmark op reducer for the app (V-11, step A) - #31945

Merged
lawrencecchen merged 3 commits into
feat-cmux-nextfrom
rust-migration/bookmarks-ffi-step-a
Nov 2, 2026
Merged

lawrencecchen merged 3 commits into
feat-cmux-nextfrom
rust-migration/bookmarks-ffi-step-a

Conversation

@lawrencecchen

@lawrencecchen lawrencecchen commented Nov 2, 2026 •

Copy link
Copy Markdown
Contributor

Step A of a two-step FFI move (layer-ownership.md V-11, bead cx-e59e). The app keeps a hand copy of the daemon's bookmark rules (CmuxNextBookmarks/Model/BookmarkTree+Apply.swift) to show an edit before the daemon's echo. #31818 put those rules in cmux-bookmarks-core (the daemon uses it, with a parity test against the daemon's SQL ops). This PR exposes the crate's reducer to the app FFI. There is no Swift change.

Check the app FFI pin: expected, step A of a two-step FFI move. It clears with the re-pin PR that app-ffi-release.yml opens after this lands. Step B (after that re-pin is on feat) makes the app keep a confirmed tree from daemon listings plus a pending-edit log, compute the visible tree through this op, and delete BookmarkTree+Apply.swift, with its Mac suites.

What changes

  • cmux_layout_reducer_json op bookmarks_apply: {browser_profile_id, tree: [Bookmark], ops: [BookmarkOp], now_ms, seed?} -> {tree, results: [{result, changed} | {error_code, reason, message}]} (cmux_bookmarks_core::preview::respond). The ops use the daemon's op JSON. A pending op the visible tree refuses changes nothing and reports why, so an edit that the echo already holds does not move the view. reason names the rule, so the app keeps its localized refusal texts.
  • cmux-layout-reducer-ffi depends on cmux-bookmarks-core without its url feature: the app links no URL parser (no url/idna/ICU, about 30 packages, in cmux-app-ffi/Cargo.lock). Without it, validate_url checks the scheme shape, which accepts every URL the parser accepts in the crate's test corpus; so the app never refuses an edit the daemon takes, and one the daemon refuses comes back as the command's error.
  • Wiring: the cmux-app-ffi/Cargo.lock entry (written by hand in cargo's format; the cmux-tui.yml step cmux-layout-reducer-ffi (own workspace) checks it with cargo metadata + git diff --exit-code Cargo.lock), the release push path in app-ffi-release.yml (required by scripts/cmux-next/tests/app-ffi-release-paths.test.sh for every FFI input crate), and the app crate license gate's first-party GPL list.

Risk: high only because of the one push-path line in app-ffi-release.yml (a release workflow). The product change is one new op nothing calls yet.

Known gaps between the crate's Tree and the daemon (in tree.rs): it cannot check that a profile exists in browser_profiles or that an id is free in every profile, and it does not check a folder icon's syntax or stored asset. A daemon refusal of these reaches the app as the command's error.

Tests

No red step: the op did not exist on the base.

Observed / Not run

cmux-layout-reducer-ffi is its own Cargo workspace, so the fleet runs its tests on never-merge proof refs that build it inside the cmux-tui workspace (listed in stopped.txt):

  • rust-migration/bookmarks-step-a-ffiproof at f318241 = this PR's head 0e562a7 + one PROOF ONLY commit (workspace member, [workspace] dropped, lock entry).
  • rust-migration/bookmarks-step-a-ffiproof-red at b4b597c = the proof ref without the bookmarks_apply arm.

Command: scripts/ci/cmux-tui-rust-check.sh test bookmarks_apply (linux, through proof-step.py).

  • Red at b4b597c: step 140374ec848488eee8294e83, failed on its own check: bookmarks_apply::two_quick_edits_both_show_on_the_confirmed_tree FAIL (the op is unknown, so the call returns ERR_INVALID), also alone on the rerun; a_malformed_request_is_invalid passes there too (an unknown op is also ERR_INVALID).
  • Green at f318241: step 0b07137f01046993fd688beb, passed (2 tests run, 2 passed).
  • Clippy at f318241 (the FFI crate inside the workspace): step 9cd3870ed526af8b6ba6f1e2, passed.
  • The head is now a4eb6e6: cmux-bot merged feat-cmux-next, which brought two other moves' FFI changes (cmux-layout-reducer: sidebar row face rules and typed C ABI (step A of #31808, Rust only) #31925 sidebar face, agent cursor visibility: the rules in Rust (cmux-focus-core) and the app FFI op (B9 step A) #31934 agent cursor visibility). The merge keeps the bookmarks_apply arm, the lock entries and the release path line; app-ffi-release-paths.test.sh ok at this head.
  • Panic ratchet at the exact head a4eb6e6: step 5ab614a9940ba072525b0b29, pass (83 crates checked, 33 at deny). cmux-layout-reducer-ffi is its own Cargo workspace, so the ratchet's workspace run does not see it; the non-test lines this PR adds there are one match arm with map_err(..)? only (no indexing, unwrap, expect, panic, unreachable, todo or unimplemented).

Local (Python only): app-ffi-release-paths.test.sh ok; python3 -m unittest tests.test_app_crate_licenses ok; tests.test_cmux_next_route -k app_ffi ok; scripts/cmux-next/prepush-gate.py no new violations.

Not run: the cmux-app-ffi/Cargo.lock check (cargo metadata + git diff --exit-code Cargo.lock); the entry was written by hand in cargo's format. No Swift compile (no Swift change). No pre-merge FFI release exists, so the app half is not built against this op yet.

Changelog

none

… the app (V-11, step A of a two-step FFI move)

The app FFI's JSON channel gets the op `bookmarks_apply`: cmux-bookmarks-core
preview::respond ({browser_profile_id, tree, ops, now_ms, seed?} ->
{tree, results}). The app is to show its pending bookmark edits on the
daemon's confirmed tree by the daemon's own rules, and then delete its Swift
copy (BookmarkTree+Apply.swift): that is step B, after the re-pin.

cmux-layout-reducer-ffi links cmux-bookmarks-core without its `url`
feature, so the app links no URL parser (the scheme check accepts every URL
the parser accepts). Wiring: the cmux-app-ffi lock entry, the app FFI
release push path (app-ffi-release-paths.test.sh requires it) and the app
crate license gate's first-party list.

Test: tests/bookmarks_apply.rs (the op through the C ABI).
Check the app FFI pin: expected, step A of a two-step FFI move.
@lawrencecchen lawrencecchen added the risk:high Risk tier (docs/ci/risk-tiers.md) label Nov 2, 2026
@github-actions

github-actions Bot commented Nov 2, 2026 •

Copy link
Copy Markdown
Contributor

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

Review 0e562a7: 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=pr31945 -f ref=rust-migration/bookmarks-ffi-step-a -f platform=macos -f backend_mode=local

red: cmux-next checks (god files, concurrency, crash safety, l10n); no finished base run on a7f48bb to compare; do not merge until green; see lane-rules

Shipped in:

The feat-cmux-next push run https://github.com/manaflow-ai/cmux/actions/runs/41425819578 at f22cb91 failed: cmux-next checks (god files, concurrency, crash safety, l10n).
Those jobs last passed at e836232. This pull request is one of 5 merged since: #28950, #32044, #32084, #31945, #32102.
If the failure is in your change, fix forward on feat-cmux-next. Pull requests run only the tiers their paths reach (docs/ci/cmux-next-tiers.md); label a batch PR full-ci to run them all before merging.

@blacksmith-sh

blacksmith-sh Bot commented Nov 2, 2026

Copy link
Copy Markdown

Found 6 test failures on Blacksmith runners:

Failures

Test View Logs
/tmp/tmpqsujel67/summary-fail.json (live):/summary-fail.json (live): View Logs
/tmp/tmpweu38kfd/summary-missing.json (live):/summary-missing.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

Fix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need.

@teamleaderleo

Copy link
Copy Markdown
Collaborator

Review (codex gpt-6.1-sol xhigh, next-review): SHIP at 0e562a7

SHIP at 0e562a7
Visual verdict: no images; Leo tries the build

No findings. The release-path check and 120 Python tests passed; Rust tests could not run because Cargo is unavailable.

Follow-ups: none

@teamleaderleo teamleaderleo removed the risk:high Risk tier (docs/ci/risk-tiers.md) label Nov 2, 2026
@github-actions github-actions Bot added the risk:high Risk tier (docs/ci/risk-tiers.md) label Nov 2, 2026
@cmux-bot
cmux-bot Bot temporarily deployed to ci-routing-agent November 2, 2026 08:52 Inactive
@cursor

cursor Bot commented Nov 2, 2026

Copy link
Copy Markdown

Bugbot needs on-demand usage enabled

Bugbot 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.

@teamleaderleo

Copy link
Copy Markdown
Collaborator

Review (codex gpt-6.1-sol xhigh, next-review): SHIP at a4eb6e6

SHIP at a4eb6e6
Visual verdict: no images; Leo tries the build

No actionable findings. Focused shell/Python checks passed; Rust tests and Cargo metadata were unavailable because Cargo is not installed.

Follow-ups: none

@teamleaderleo teamleaderleo removed the risk:high Risk tier (docs/ci/risk-tiers.md) label Nov 2, 2026
@github-actions github-actions Bot added the risk:high Risk tier (docs/ci/risk-tiers.md) label Nov 2, 2026
@lawrencecchen

Copy link
Copy Markdown
Contributor Author

Review (independent-reviewer-rm-bookmarks-3): SHIP at a4eb6e6

Scope: correctness first. I read the full diff against origin/feat-cmux-next, cmux-bookmarks-core src/lib.rs, src/preview.rs, src/tree.rs, and the CI logs at this head. I did not compile or test on a local Mac.

Findings (no blocking problems)

  • The op arm. cmux_layout_reducer_json b"bookmarks_apply" (src/lib.rs:146-151) has the same shape as the other arms. It deserializes cmux_bookmarks_core::preview::Request (map_err(|_| ERR_INVALID)?), calls preview::respond(request) (by value, which matches the signature), and serializes the Response. It is inside the existing catch_unwind, so a panic gives ERR_PANIC. The added non-test lines have no indexing, unwrap, expect or panic. In tree.rs, every Vec::insert and remove index is clamped (insert_child, and the same-parent branch of move_to, which has the old < order.len() guard). Import recursion has a limit of MAX_BOOKMARK_DEPTH (64) from validate_import_nodes. Walks over parent chains and subtrees have limits (MAX_PARENT_CHAIN, SUBTREE_LEVELS), so a listing with a cycle cannot loop forever.
  • The contract. The request is {browser_profile_id, tree, ops, now_ms, seed?}. The response is {tree, results: [{result, changed} | {error_code, reason, message}]}. This agrees with preview.rs. BookmarkOp uses the tags create, update, move, delete and import, so the test's {"op":"update"} and {"op":"move"} are correct. I checked the expected values of the test by hand against tree.rs:
    • The update gives changed: true.
    • The move to another parent clamps the index to 0, and the listing then puts PAGE second, below FOLDER.
    • The folder URL update is refused with invalid_params/invalid_kind/"a folder has no url" (update: validate_url passes, then the InvalidKind check).
  • No URL parser in the app FFI. cmux-layout-reducer-ffi/Cargo.toml takes cmux-bookmarks-core without features. url is an optional dependency, and only cmux-tui-core turns it on, inside the cmux-tui workspace. cmux-app-ffi and cmux-layout-reducer-ffi are separate workspaces, and cmux-bookmarks-core is not a member of them. Thus feature unification and the dev-dependency url do not apply. cmux-app-ffi/Cargo.lock has no url, idna, form_urlencoded or percent-encoding entry. CI at this head (rust workspaces run 41416250796) compiles cmux-bookmarks-core with no url or idna crate.
  • The Cargo.lock entries. They are in the correct sort position (cmux-app-ffi < cmux-bookmarks-core < cmux-focus-core). They have no source or checksum, which is correct for a path crate. The dependencies are serde and serde_json, which agrees with the crate's non-dev, non-optional dependencies. cmux-bookmarks-core is the first dependency of cmux-layout-reducer-ffi. CI proves this at the exact head: job 125180906699 ran cargo clippy --locked --workspace --all-targets -- -D warnings and cargo test --locked --workspace in cmux-tui/crates/cmux-app-ffi, and both passed. --locked fails on a stale lock.
  • The tests in the FFI's own workspace. At this head, job 125180906753 ran cargo clippy --workspace --all-targets -D warnings and cargo test in cmux-layout-reducer-ffi. The two bookmarks_apply:: tests passed in the real configuration, with no url feature. The fleet proof used the cmux-tui workspace, where unification turns url on. Fleet: red 140374ec848488eee8294e83 shows two_quick_edits_both_show_on_the_confirmed_tree FAIL, green 0b07137f01046993fd688beb shows 2/2 passed, and clippy 9cd3870ed526af8b6ba6f1e2 passed. I read these logs.
  • app-ffi-release.yml. Against the feat tip, only one line changed: the push path cmux-tui/crates/cmux-bookmarks-core/**. scripts/cmux-next/tests/app-ffi-release-paths.test.sh is ok at this head.
  • The license gate. cmux-bookmarks-core is in FIRST_PARTY_GPL. The crate inherits GPL-3.0-or-later from the workspace. python3 -m unittest tests.test_app_crate_licenses passes (8 tests).
  • No Swift file changed. This agrees with the step A rule in the owner brief. The body has "Check the app FFI pin: expected, step A of a two-step FFI move".
  • The cmux-bot merge. It kept every op arm: base_y, resolve, resolve_drag, tab_drop, tab_refusal, sidebar_layout_reduce, tasks_preview, bookmarks_apply, focus_type_ahead, focus_child_window_key and agent_cursor_visibility. cmux-layout-reducer: sidebar row face rules and typed C ABI (step A of #31808, Rust only) #31925 (c94408d) and agent cursor visibility: the rules in Rust (cmux-focus-core) and the app FFI op (B9 step A) #31934 (69ddef1) are ancestors of the head. Against the current feat tip, the FFI crates are different only by this PR's lines.

CI classification at this head

  • Check the app FFI pin: expected for step A, and named in the body.
  • Script tests (pin-cmux-tui, embed-cef, ensure-cef store): red on pin-cmux-tui-windows-fetch.test.sh (exit 128). This is a base finding. The PR does not touch that script or its fixture (scripts/cmux-next/tests/lib/git-fixture.sh), and feat changed both after the merge base.

Non-blocking notes

  • The risk label. The PR has no risk:high label now. teamleaderleo removed it at 09:24Z, after github-actions re-added it at 08:43Z. This review is the independent receipt for the high tier. Put the label back if the tier gate needs it.
  • The panic ratchet. Step 5ab614a9940ba072525b0b29 was still running when I posted this review. This PR does not change cmux-bookmarks-core, and the FFI crate is outside the ratchet's workspace, so I read the added lines myself (see above).
  • For step B. respond rebuilds the whole tree from the full listing on each call. That is O(tree): JSON for up to 100k nodes, plus from_listing and listing(). If the app calls this on each edit on the main thread, measure it against the < 10 ms action budget. Also think about keeping the confirmed tree on the Rust side, so that one call costs only the work for the pending ops. This does not apply to step A, because nothing calls the op yet.
  • URL checks without the url feature. Only the scheme shape is checked. The design accepts this: the app shows an edit that the daemon then refuses, and that refusal comes back as the command's error. Step B must show that daemon refusal to the person.

@lawrencecchen lawrencecchen added the ready-to-land Reviewed at this head; next-autoland merges it into feat-cmux-next when its only reds are base reds label Nov 2, 2026
@lawrencecchen
lawrencecchen merged commit d2dd826 into feat-cmux-next Nov 2, 2026
286 of 293 checks passed
@lawrencecchen
lawrencecchen deleted the rust-migration/bookmarks-ffi-step-a branch November 2, 2026 10:01

This branch was previously deployed

3 inactive deployments
ci-routing-agent — a4eb6e66 Deployed Nov 2, 2026 by github-actions[bot] via cmux-next macOS placement #62192
artifacts — a4eb6e66 Deployed Nov 2, 2026 by cmux-bot[bot] via publish tree-addressed cmux-tui #100351
sentry-dsyms — a4eb6e66 Deployed Nov 2, 2026 by cmux-bot[bot] via upload cmux-tui dSYMs to Sentry #100351
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-to-land Reviewed at this head; next-autoland merges it into feat-cmux-next when its only reds are base reds risk:high Risk tier (docs/ci/risk-tiers.md)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants