Skip to content

cmux-bookmarks-core: the bookmark op rules as a crate the daemon uses, plus the in-memory reducer for the app (V-11) - #31818

Merged
lawrencecchen merged 6 commits into
feat-cmux-nextfrom
rust-migration/bookmarks
Nov 2, 2026
Merged

lawrencecchen merged 6 commits into
feat-cmux-nextfrom
rust-migration/bookmarks

Conversation

@lawrencecchen

@lawrencecchen lawrencecchen commented Nov 2, 2026 •

Copy link
Copy Markdown
Contributor

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, and a second quick edit flickers away until its echo (layer-ownership.md V-11, bead cx-e59e). This PR moves the rules into a new pure crate, cmux-bookmarks-core, which the daemon uses now, and adds the in-memory reducer the app will call. It changes no app FFI input. After it lands, step A of a two-step FFI move adds the op bookmarks_apply to the app FFI (branch rust-migration/bookmarks-ffi-move); step B, after the re-pin, makes the app call it and deletes BookmarkTree+Apply.swift.

What changes

  • New crate cmux-tui/crates/cmux-bookmarks-core (no I/O; zero hits of the panic ratchet's deny lints). It holds the limits, the validation, the record and op types (Bookmark, BookmarkOp, ...) and Tree: one profile's tree in memory with the daemon's op reducer, step for step (same checks in the same order, same messages, same index clamping, same result JSON; every check runs before the first change, like the daemon's transaction). preview::respond is the request the app will send: a confirmed listing plus pending ops in, the visible tree plus one result per op out. Each refusal also names its rule (Reason), so the app can keep its localized refusal texts.
  • The daemon uses it, with no behavior change. workspace_registry/personal_bookmarks.rs re-exports the moved items and keeps its SQLite rows, replay ledger and journal. The op types serialize as before, so the replay fingerprint of a keyed op does not change. URLs are still parsed with url::Url (the crate's url feature, which cmux-tui-core turns on). Without the feature, the check is the scheme shape (has_url_scheme), which accepts every URL the parser accepts in the test corpus; that is what the app FFI will use, so it links no URL parser.
  • Wiring: a cmux-tui workspace member and its Cargo.lock entry; one comment in cmux-tui/scripts/check-sdk-schema.py.

Known gaps between Tree and the daemon (also in tree.rs): Tree holds one profile, so it cannot check that a profile exists in browser_profiles (it refuses another profile as unknown) or that an id is free in every profile; and it does not check a folder icon's syntax or that its stored asset exists. When the daemon refuses one of these, the app gets the command's error.

Follow-up: two Rust implementations of the reducer now exist (the daemon's SQL ops and Tree), held equal by the parity test. A later change can make the daemon apply each op through Tree on a loaded working set (the profile's tree, then write the changed rows), if a bench shows that is cheap at 100,000 nodes; then the SQL ops go.

Tests

  • cmux-bookmarks-core tests/bookmarks/tree.rs: the Swift BookmarkTreeTests of the hand copy (deleted with the CmuxNext unit suites in 36f2381) ported with the daemon's ids and refusals, plus the daemon rules the Swift copy lacked (import and replace, id and profile checks, icon only on a folder, no favicon key on an import), each refusal's Reason, the op JSON shape, the preview (two pending edits both show; the echo does not move the view), id minting, and the scheme check against url::Url::parse.
  • cmux-tui-core server/bookmark_parity_tests.rs: 1000 seeded random ops (4 seeds x 250 steps: creates, moves, deletes, updates, icons, imports and replaces, with bad parents, bad URLs, NUL titles, unknown ids). Each step reads the daemon's listing, applies the op to the daemon and to Tree, and requires the same tree, the same result and the same refusal code and message.

This is a move with no bug to reproduce: the crate does not exist on the base, so there is no red step. The parity test is the proof that the Rust reducer equals the daemon.

Observed / Not run

Observed (fleet):

  • Test cmux-tui-rust-check.sh test bookmark at 43dd4a9: step 17a8481bae447211ebc8f64b, 44 tests run, 44 passed (the 19 bookmark_tree:: tests of the crate, server::bookmarks::tests::parity::the_daemon_and_the_shared_reducer_give_the_same_tree_result_and_refusal, and the existing daemon bookmark tests). The head eedc9e0 only merges feat-cmux-next on top; the patch is the same.
  • Clippy at eedc9e0: step 92dda7db7281ab6a12a6f6c9, passed. (Step 77510e2b148e at 43dd4a9 failed on cmux-tui/src/plugin_manager.rs:233 suspicious_open_options, a base red fixed on feat by ca404c4; hence the merge of the base.)
  • Panic ratchet at 43dd4a9: step 9ff1b6682ea5a465dfa60b74 RED on acpmux clippy::indexing_slicing 697 > baseline 686 (base finding; this PR touches no acpmux file). No cmux-bookmarks-core hit is listed.
  • Head aaa5fcd did not compile (BookmarkOutcome, a daemon-only type, was cut with the moved records; fixed in 2473f93). Its fmt step 2d19e4be180c failed only on acpmux/src/hub/list_guard.rs and hub/mod.rs (base red).
  • Local: scripts/cmux-next/prepush-gate.py no new violations; cmux-tui/scripts/check-sdk-schema.py ok; .github/scripts/rust-workspaces.py check ok.

Not run: no Swift compile (no Swift change); no perf A/B (nothing calls the preview yet).

Changelog

none

…d the app share (V-11, PR 1 of 2)

The rules of bookmarks-v1 (limits, validation, op and record types) move out
of cmux-tui-core workspace_registry/personal_bookmarks.rs into a new pure
crate, cmux-bookmarks-core. The daemon re-exports them and keeps its SQLite
rows, replay ledger and journal; its behavior and the op JSON (the replay
fingerprint) do not change.

The crate adds Tree: one profile's tree in memory with the daemon's op
reducer, step for step (same checks, order, messages, clamping and result
JSON). cmux-layout-reducer-ffi exposes it to the app as the JSON op
`bookmarks_apply` ({browser_profile_id, tree, ops, now_ms, seed?} ->
{tree, results}), so the app can show its pending edits on the confirmed
tree by the daemon's rules and delete its Swift copy
(BookmarkTree+Apply.swift) in PR 2.

The URL parser stays out of the app FFI: the crate's `url` feature (the
daemon turns it on) parses with url::Url; without it the check is the
scheme shape, which accepts every URL the parser accepts.

Tests (run on the fleet; ids in the PR):
- cmux-bookmarks-core tests/bookmarks/tree.rs: the Swift BookmarkTreeTests
  (suite deleted in 36f2381) ported with daemon ids and refusals, plus
  imports, replace, ids, profiles, icons, op JSON, preview and the URL check.
- cmux-tui-core server/bookmark_parity_tests.rs: 1000 seeded random ops
  through the daemon and through Tree, same tree, result and refusal.
- cmux-layout-reducer-ffi tests/bookmarks_apply.rs: the op through the C ABI.

Expected red: Check the app FFI pin (cmux-layout-reducer-ffi now links
cmux-bookmarks-core); the re-pin follows the landing.
@lawrencecchen lawrencecchen added the risk:high Risk tier (docs/ci/risk-tiers.md) label Nov 2, 2026
@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.

@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 43dd4a9: 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=pr31818 -f ref=rust-migration/bookmarks -f platform=macos -f backend_mode=local

web and size checks: green

Shipped in:

@blacksmith-sh

blacksmith-sh Bot commented Nov 2, 2026 •

Copy link
Copy Markdown

Found 6 test failures on Blacksmith runners:

Failures

Test View Logs
main.DeveloperIdAppTests.test_download_prints_the_active_profile_only/
test_download_prints_the_active_profile_only
View Logs
stubs: /System/Library/Frameworks/Security.framework/Versions/A/Security has no stub/
Security has no stub
View Logs
stubs: CoreFoundation.tbd: install-name /System/Library/Frameworks/
CoreFoundation.framework/Versions/A/CoreFoundation, missing 1: ['_CFRetain']/
CoreFoundation, missing 1: ['_CFRetain']
View Logs
stubs: CoreFoundation.tbd: install-name /System/Library/Frameworks/
CoreFoundation.framework/Versions/A/CoreFoundation, missing 1: ['_log2']/
CoreFoundation, missing 1: ['_log2']
View Logs
test_every_compile_job_names_its_warm_sources (main.ThisTree.test_every_compile_job
_names_its_warm_sources)/
test_every_compile_job_names_its_warm_sources (main.ThisTree.test_every_compile_job
_names_its_warm_sources)
View Logs
test_the_repository_highlights_validate (main.DigestTests.test_the_repository_highl
ights_validate)/
test_the_repository_highlights_validate (main.DigestTests.test_the_repository_highl
ights_validate)
View Logs

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

…p's localized text

The preview result carries it as `reason`; the daemon's wire is unchanged.
… path and lock entry move to the one FFI move PR

The FFI rule (cmux-ad 04:35Z): one PR per FFI move with the Rust change, the
CCmuxAppFFI re-pin and the Swift call. So this PR changes no FFI input:
cmux-layout-reducer-ffi, app-ffi-release.yml, cmux-app-ffi/Cargo.lock and the
app crate license list are as on the base. The op is kept on
rust-migration/bookmarks-ffi-move.
@github-actions github-actions Bot added risk:normal Risk tier (docs/ci/risk-tiers.md) and removed risk:high Risk tier (docs/ci/risk-tiers.md) labels Nov 2, 2026
@lawrencecchen lawrencecchen changed the title cmux-bookmarks-core: one bookmark op reducer for the daemon and the app (V-11, Rust half) cmux-bookmarks-core: the bookmark op rules as a crate the daemon uses, plus the in-memory reducer for the app (V-11) Nov 2, 2026
@lawrencecchen

Copy link
Copy Markdown
Contributor Author

Review (independent-reviewer-rm-bookmarks): SHIP at eedc9e0

Correctness review against layer-ownership.md 1-3, REVIEW-STANDARD.md, REVIEW.md and CLAUDE.md. I read the code; I did not build or run tests on this machine.

Claims checked against the code

  • Wire and fingerprint: Bookmark, BookmarkInput, BookmarkUpdate, BookmarkImportNode, BookmarkImport and BookmarkOp keep the base field order, tag = "op", rename_all = "kebab-case", flatten and every skip_serializing_if. The new attributes (default, deserialize_with = "present_nullable", the Deserialize derives) act only on deserialization, so serde_json::to_vec(&op) gives the same bytes and stored replay fingerprints stay valid. BookmarkOp::name() gives the same strings.
  • Errors: the Display strings ("bad request: " prefix for invalid_params only), the codes and every message text are the same as on base. The validate_* functions now return BookmarkError; ? in the daemon's anyhow functions wraps it, so server/bookmarks.rs:121 downcast_ref::<BookmarkError>() still finds the code. validate_profile_shape copies the validate_browser_profile_ref logic and gives the same message as before.
  • Removed items: nothing outside the crate uses MAX_BOOKMARK_{TITLE,URL,KEY}_BYTES (they are no longer re-exported); validate_browser_profile_ref stays in place for its other callers. BookmarkOutcome stays in the daemon.
  • url feature: cmux-tui-core turns it on (url::Url::parse). Without it, has_url_scheme accepts a superset of what the parser accepts, because control characters are refused before that check.
  • Tree against personal_bookmarks/ops.rs, one step at a time: the checks run in the same order in create, update, move, delete and import (including resolve_parent before the replace lookup, and the existing-id return before ensure_profile). Index clamping is the same: same parent min(len-1) with remove and insert, other parent min(count) after detach. Delete order is the same: by level, then by id, which matches SQLite BINARY collation. The replace lookup uses the same (created_ms, id) minimum, the same folder_depth-1+height depth check and the same ensure_capacity(old.len(), count), and mints count-1 ids. Nothing changes before all checks pass.
  • Panics: the crate denies unwrap, expect, panic, indexing and string slicing. order.remove has a guard (old < len) and every insert index is clamped. Recursion is limited by the depth validation. from_listing, listing and subtree_levels stop on cyclic or duplicate input.
  • Cargo.lock: the new package entry is in alphabetical order and lists url (optional and dev). cmux-tui-core gets the dependency. No other lock change.
  • App FFI: check-app-ffi-pin.sh --list-inputs does not list cmux-bookmarks-core or cmux-tui-core. No file under cmux-app-ffi, cmux-layout-reducer-ffi or .github/workflows/app-ffi-release.yml changed.
  • Fleet evidence (I did not run it again): test step 17a8481bae447211ebc8f64b at 43dd4a9 passed 44 of 44, including the parity and bookmark_tree tests. 43dd4a9..eedc9e0 does not change the crate, the parity/bookmark tests, personal_bookmarks or server/bookmarks.rs. Clippy step 92dda7db7281ab6a12a6f6c9 at eedc9e0 passed. Panic ratchet run 41391967912: the only red is acpmux: clippy::indexing_slicing 697 > baseline 686, a base finding in files this PR does not touch, with no cmux-bookmarks-core hit.

Non-blocking notes

  1. Parity coverage gaps: the random corpus never sends a create without an id, a profile other than default, ops that reach the depth or capacity limits, a bad icon string, or a replace import with two source_key folders that have the same created_ms (the tie-break by id). The core unit tests cover the depth limit. The known daemon-only checks (profile exists, global id uniqueness, icon syntax and asset) are documented in tree.rs.
  2. On a broken parent chain (cyclic or orphaned rows), the Tree refuses with invalid_params. The daemon uses a plain anyhow error with no BookmarkError code. This can only happen with corrupt data.
  3. Layer rule L5 / model item 4: the tree change logic now exists twice in Rust (SQL in the daemon, in memory in Tree), kept equal by the differential test. The validation is shared. This is better than the Swift copy it will replace, but it is still two copies. A later step can make the daemon use Tree (or make Tree the only rule source) so that one reducer remains.
  4. The parity test sets now_ms = 0, and every create and import gives created_ms, so the now_ms fallback is tested only by the core unit tests.

@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
@cmux-bot
cmux-bot Bot temporarily deployed to ci-routing-agent November 2, 2026 06:55 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.

@lawrencecchen
lawrencecchen merged commit 33cdf18 into feat-cmux-next Nov 2, 2026
161 of 162 checks passed
@lawrencecchen
lawrencecchen deleted the rust-migration/bookmarks branch November 2, 2026 06:59

This branch was previously deployed

3 inactive deployments
artifacts — f8911f0f Deployed Nov 2, 2026 by cmux-bot[bot] via publish tree-addressed cmux-tui #99880
sentry-dsyms — f8911f0f Deployed Nov 2, 2026 by cmux-bot[bot] via upload cmux-tui dSYMs to Sentry #99880
ci-routing-agent — f8911f0f Deployed Nov 2, 2026 by cmux-bot[bot] via cmux-tui tree preflight #99880
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:normal Risk tier (docs/ci/risk-tiers.md)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant