From 711039e756b3a3b6f1fb1219544e1133c8593efc Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sat, 15 Aug 2026 16:18:19 +0000 Subject: [PATCH 1/2] docs(planning): add duplication and consolidation audit Map duplicated logic across Rust, TypeScript, and Flutter, and classify what should stay dual-maintained versus what should merge. Links existing plans instead of restating them. Co-authored-by: joepmeindertsma --- planning/README.md | 1 + planning/duplication-and-consolidation.md | 423 ++++++++++++++++++++++ 2 files changed, 424 insertions(+) create mode 100644 planning/duplication-and-consolidation.md diff --git a/planning/README.md b/planning/README.md index 0571e2f05..debb86ce6 100644 --- a/planning/README.md +++ b/planning/README.md @@ -36,6 +36,7 @@ scratch document. When a plan becomes obsolete, delete it. | [`cleanup-update-encoding.md`](./cleanup-update-encoding.md) | **Active:** Refactor UPDATE frame encoding/decoding, unify parser, and remove magic numbers. | | [`commit-fanout-drive-isolation.md`](./commit-fanout-drive-isolation.md) | **Active:** Drive-scoped WS commit fan-out — closes the cross-tenant commit leak and the e2e 401-spillover flake; tracks the chatroom guest-drive client-hydration regression and its fix. | | [`structural-problems-index.md`](./structural-problems-index.md) | 2026-05-28 audit: ranked open structural issues with per-item plan files. Covers React Compiler / Resource proxy mismatch, subscription unification, save-state signals, Loro-as-authority, subject typing, and remaining actor-message Arc-wrap. | +| [`duplication-and-consolidation.md`](./duplication-and-consolidation.md) | **Audit (2026-08-15):** map of duplicated logic (commit ingest, Loro merge, wire protocol, ontology URLs, datatype tags, search/blobs, browser search/create UI, Flutter twins) and what should stay dual-maintained vs merge. Links existing plans; does not restate them. | | [`nextgraph-interop.md`](./nextgraph-interop.md) | **Proposal:** read/edit `did:ng:` (NextGraph) resources via a pluggable, scheme-routed Store backend; use Atomic components chrome-free in NextGraph apps. Builds on the elfa-tables proof. | | [`drive-reconciliation.md`](./drive-reconciliation.md) | **Proposal (2026-07-08):** replace the flat whole-drive VV hash with range-based set reconciliation (RBSR) so sync cost tracks the delta, not the drive; optional signed drive state root (reusing genesis `stateHash`) closes F1 + enables offline verification. Records the RBSR-over-hierarchy-Merkle and VV-leaves-first decisions before any build. | | [`sync-onboarding-ux.md`](./sync-onboarding-ux.md) | **Guideline:** the language, the rules of what can reach what, every account/device path, where each client's logic lives, and what is tested. Read before changing a sync or onboarding screen in any client. | diff --git a/planning/duplication-and-consolidation.md b/planning/duplication-and-consolidation.md new file mode 100644 index 000000000..9c4a964e8 --- /dev/null +++ b/planning/duplication-and-consolidation.md @@ -0,0 +1,423 @@ +# Duplication and consolidation audit (2026-08-15) + +Index of things that exist more than once, and whether they should stay that +way. This is a map, not a rewrite plan. Where a dedicated plan already owns +the work, this file links to it instead of restating it. + +**Related plans (do not duplicate):** + +| Plan | What it already covers | +| --- | --- | +| [`atomic-lib-runtime.md`](./atomic-lib-runtime.md) | Node API; blob/search/query owned by `atomic_lib`; Flutter/WASM as thin bindings | +| [`unified-sync.md`](./unified-sync.md) | One sync API; WS vs Iroh; remaining `handle_frame` gaps; AUTH/VV/blob-hash copies | +| [`unified-data-layer.md`](./unified-data-layer.md) | Browser ingress, outbox, subscriptions, dirty signals | +| [`loro-source-of-truth.md`](./loro-source-of-truth.md) + [`unify-resource-representations.md`](./unify-resource-representations.md) | Loro doc as authority; `PropVals` / `_cache` as derived | +| [`unify-subscription-primitives.md`](./unify-subscription-primitives.md) | Server `SUB` / `SUBSCRIBE` / `SUBSCRIBE_QUERY` → one match type | +| [`canvas-undo-consolidation.md`](./canvas-undo-consolidation.md) | Flutter Dart action stack vs Loro undo (Phase B still open) | +| [`structural-problems-index.md`](./structural-problems-index.md) | Ranked structural issues that overlap several of the above | +| [`sync-onboarding-ux.md`](./sync-onboarding-ux.md) | Browser ↔ Flutter twin map for pairing / servers / onboarding | + +## How to classify a duplicate + +1. **Must dual-maintain** — two languages, same bytes (signing, wire tags, genesis). Keep both; generate or golden-test. +2. **Should be one implementation** — same crate, same job, two call paths that can drift. +3. **Intentional twins** — browser UI and Flutter UI. Do not merge codebases; share specs and golden tests. +4. **Leftover** — deprecated path still imported, or a migration half-done. + +--- + +## Highest leverage + +These are the copies that actually cause bugs, or that keep growing in pairs. + +### 1. Three commit-ingest paths with different validation + +HTTP `/commit` and hub WS `COMMIT` go through `engine::ingest_commit_json` +(`lib/src/sync/engine.rs`). That is the intended hub. Two other paths still +apply commits with weaker or different `CommitOpts`: + +| Path | File | Signature | Timestamp | Rights | Previous-commit | +| --- | --- | --- | --- | --- | --- | +| Hub HTTP/WS | `server/src/handlers/commit.rs` → `ingest_commit_json` | yes | hub policy | yes | hub policy | +| Flutter WS receive | `lib/src/sync/ws_apply.rs::apply_commit_json` | **yes** | **no** | **no** | **no** | +| WASM `apply_commit` | `wasm/src/lib.rs` | own `CommitOpts` block | own | own | own | + +`ws_apply::apply_commit_json` is the Flutter WS ingest. It is not a thin +wrapper around `ingest_commit_json`. A rights/timestamp change on the hub +path does not automatically apply to mobile. + +`Resource::save` / `save_locally` / `save_as_genesis` / `save_remote` / +`apply_signed_commit` (`lib/src/resources.rs`) each construct another +`CommitOpts` literal. Same apply core, inconsistent policy. + +**Consolidate:** one `CommitIngestOpts` (already exists for hub vs peer) used +by `ws_apply` and WASM too. Collapse `save_*` to `AtomicNode::mutate` per +[`atomic-lib-runtime.md`](./atomic-lib-runtime.md). + +### 2. Loro merge/persist written twice + +`lib/src/sync/ws_apply.rs::resolve_update` + `persist_update` (~160 lines: +load snapshot, import, materialize, resolve drive, persist) parallels +`engine.rs` sync-push import and `resources.rs::merge_persisted_state`. + +The drive-spoof fix (F2 in `unified-sync.md`) lives on the `ws_apply` path +with its own tests. The engine path must not grow a second copy of that +check. + +**Consolidate:** one `import_and_materialize(subject, bytes) -> ResolvedUpdate` +used by live WS, Iroh, and engine push. + +### 3. Wire protocol in two languages + +Canonical spec: `docs/src/websockets.md`. + +| Side | File | ~lines | +| --- | --- | --- | +| Rust | `lib/src/sync/protocol.rs` | 969 | +| TypeScript | `browser/lib/src/ws-v2.ts` | 749 | + +Both files say "update the other in the same change." Drift today: + +- Rust has `HELLO` (`0x37`); TS `Tag` enum does not. +- Error classification: `protocol::classify_commit_error` vs string matchers + in `browser/lib/src/local-outbox.ts`. +- Legacy text frames (`LORO_SYNC_*`, `SYNC_VV`) still parsed in both + `lib/src/client/ws.rs` and `browser/lib/src/websockets.ts`. + +**Consolidate:** generate TS constants/encoders from the Rust module or from +the markdown spec. Until then, a round-trip fixture (Rust encode → TS decode +and back) is cheaper than codegen. + +`cleanup-update-encoding.md` already unified `decode_update` on the Rust +side; the remaining problem is the language boundary. + +### 4. Ontology URLs in three places + +| Source | Status | +| --- | --- | +| `lib/src/urls.rs` | Hand-written Rust constants (authoritative for server/lib) | +| `browser/lib/src/ontologies/*.ts` | Generated by `@tomic/cli` from live ontology resources | +| `browser/lib/src/urls.ts` | **Deprecated**, still imported | + +Remaining `urls.ts` importers (all in `@tomic/lib`): + +- `resource.ts` — `GENESIS`, `properties`, `instances` +- `store.ts` — `BLOB`, `endpoints`, `INTERNAL_ID` +- `websockets.ts` — `BLOB` +- `invites.ts` / `invites.test.ts` — `properties` +- `index.ts` — re-exports the whole deprecated module + +TS generation exists; Rust does not. Datatype URL strings are also copied +into `browser/lib/src/datatypes.ts` (`enum Datatype`). + +**Consolidate:** finish deleting `urls.ts`. Generate `urls.rs` from the same +ontology source as the TS files (or from `lib/defaults/`). + +### 5. Datatype tags: same table, two functions, different timing + +Load-bearing Loro tags (`atomicUrl`, `resourceArray`, `json`, `resource`, +plus cosmetic `markdown`/`slug`/`uri`/`date`/`timestamp`/`localizedText`): + +| | Rust | TypeScript | +| --- | --- | --- | +| Write | `datatype_tag()` in `lib/src/loro.rs`, at `set_property` | `datatypeTag()` in `browser/lib/src/datatypes.ts`, at **sign time** (`writeDatatypeTags`) | +| Read | `loro_value_to_atomic_value_tagged` | `normalizeLoroValue` / `rebuildCacheFromLoro` (subset) | + +TS does not emit a `resource` tag; it relies on the server heuristic for +nested objects. Untagged values fall back to Rust heuristics that TS does +not replicate (URL-shaped strings → `AtomicUrl`, `{...}` → nested resource). + +Validation regexes (`SLUG_REGEX`, `DATE_REGEX`, `LANG_TAG_REGEX`) are +copied between `lib/src/values.rs` and `browser/lib/src/datatypes.ts`. + +**Consolidate:** one JSON tag table + golden tests (the genesis-certificate +pattern in `lib/src/genesis.rs` ↔ `browser/lib/src/genesis.ts` is the model). +Writing tags at different times is a real drift risk: a crash between +`set` and `save` leaves an untagged doc. + +### 6. Subject types have drifted + +Rust `lib/src/subject.rs`: `Internal` / `External` / `Did`, `drive_hint`, +`pure_id()` equality (query params do not affect DID identity). + +TS `browser/lib/src/subject.ts`: branded `string`; `did:ad:` or `https?://` +only. No `internal:`, no `pure_id()` equality. + +Plan: [`subject-types-end-to-end.md`](./subject-types-end-to-end.md) +(started, consumer migration not done). Until that lands, TS string compare +and Rust `pure_id()` can disagree on the same DID. + +### 7. Search and blobs still live in the server crate + +Blocks the runtime plan. Concrete copies: + +- **Tantivy escape** implemented three times: `browser/lib/src/search.ts` + `escapeTantivyKey`, `lib/src/client/search.rs` `escape_tantivy_key`, and + `server/tests/it/file_search_repro.rs` (test re-implements the browser + helper). `SearchOpts` / `build_search_subject` are also dual. +- **Blob write admission:** HTTP in `server/src/handlers/blob.rs`, WS in + `lib/src/sync/engine.rs`. Same policy, two implementations. +- **Upload File-resource construction** (`save_file_and_create_resource`) and + **chunked download reconstruction** exist only in server handlers; WASM + talks to `Tree::Blobs` directly. + +`Storelike::search` in lib still builds a `/search` URL and fetches it — +an HTTP dependency the runtime plan wants gone. + +### 8. Dev server port is not one number + +Default atomic-server port is **9883**. Vite proxy and +`.env.development` point at **9885**. Hardcoded `9883` remains in: + +- `browser/data-browser/src/App.tsx` +- `browser/data-browser/src/hooks/useDevDrive.ts` (`DEV_SERVER`) +- `browser/data-browser/src/helpers/tauri.tsx` +- `browser/e2e/tests/test-utils.ts` (`SERVER_URL` fallback) +- placeholders in `SyncRoute.tsx` / `ConnectDeviceStep.tsx` + +AGENTS.md (Cloud section) and `browser/e2e/README.md` already warn that +misalignment silently repoints drives. This is config duplication with a +known failure mode, not an architecture issue. + +**Consolidate:** one env-driven origin; e2e and Vite read the same value; +UI placeholders should not hardcode a port. + +--- + +## Cross-language copies that should stay (with tests) + +These are the language boundary. Merging them into WASM-only would make +the browser unable to sign or speak the wire format without a round-trip. + +| Concern | Rust | TS | Dart | Tests today | +| --- | --- | --- | --- | --- | +| Commit JCS signing | `commit.rs` `serde_jcs` | `commit.ts` `fast-json-stable-stringify` | golden vectors | `browser/lib/src/sign.test.ts` vs Rust bytes | +| Genesis certificate | `genesis.rs` | `genesis.ts` | `signing_golden_vectors_test.dart` | Explicitly byte-identical | +| Agent secret envelope | `agents.rs` | `CryptoProvider.ts` | `atomic_auth.dart` (HTTP auth only) | Shared `genesis_test_vectors.json` | +| Pairing envelope | — | `browser/lib/src/pairing.ts` | `_parsePairingUri` in `pair_screen.dart` | Separate tests, **not** a shared fixture | +| Server URL normalize | — | `helpers/serverUrl.ts` | `atomic/server_url.dart` | Twin tests; comments require lockstep | +| Canvas fan/undo constants | — | `views/Canvas/fan-helpers.ts`, `history-helpers.ts` | `fan_helpers.dart`, `stroke_data.dart` | Comments "Matches Flutter"; no shared JSON | +| Filter operators | `storelike.rs` `FilterOperator` | `collection.ts` `valueMatches` | via WASM query | No shared fixture | + +**Do this, not a merge:** + +- Pairing: Flutter should call the same envelope rules as `pairing.ts` + (golden URI fixtures, or parse in Rust and expose via FRB). +- `normalizeServerUrl` / `isLocalAddress`: already documented twins; add + one shared test vector file both suites load. +- Filter operators: WASM already runs Rust queries locally; client-side + `valueMatches` is the live-membership shortcut. Either call WASM or + share operator fixtures. +- Canvas constants: a tiny JSON of `SCRUB_PIXELS_PER_HISTORY`, + `UNDO_STACK_LIMIT`, `BRANCH_GRACE_MS` imported by both. + +TS `CommitBuilder` still models legacy `set` / `push` / `remove` fields +that the server rejects. Dead API surface on the client. + +--- + +## Browser app-layer duplicates + +Not the same as the protocol copies. These are multiple UIs for one job +inside `data-browser`. + +### Search: overlay vs full-page route + +`SearchOverlay.tsx` (~430 lines) and `SearchRoute.tsx` (~252 lines) both: + +- call `useServerSearch` with drive/scope/filters +- keyboard-select results +- render `ResourceCard` lists and tag chips +- share `searchUtils.ts` for filter encoding + +Keep two shells (command palette vs `/app/search`). Extract one +`SearchResultsList` + query hook. + +Other search surfaces (`SearchBox`, `useLocalSearch`, table +`useResourceSearch`, `SettingsSearch`) are different jobs; do not merge +them into the overlay. + +### Create-resource: six entry points + +| Entry | Path | +| --- | --- | +| `/app/new` hub | `routes/NewResource/` | +| Per-class dialogs | `components/forms/NewForm/` | +| Sidebar / folder | `NewInstanceButton`, `QuickCreateRow` | +| Ontology page | `views/OntologyPage/CreateInstanceButton.tsx` | +| Table rows | `chunks/TablePage/QuickAddBar.tsx` | +| Context menu | `actions/resourceActions.tsx` | + +`BasicInstanceHandlers.ts` is already a class → handler registry. Route +the other entries through it instead of adding a seventh. + +### Four things named "history" + +| Name | What it actually is | +| --- | --- | +| `routes/History/` + `useVersions` | Loro OpLog time-travel for a resource | +| `views/Canvas/history-helpers.ts` | Canvas stroke undo + discarded branches (`localStorage`) | +| `useTableHistory` | In-memory table cell undo | +| `hooks/useDriveHistory.ts` | Recent **drives** in `localStorage` | + +Do not merge. Rename `useDriveHistory` → `useRecentDrives`. + +### Document v1 still shipped next to v2 + +`views/DocumentPage.tsx` (element-based) vs +`views/Document/DocumentV2FullPage.tsx` (TipTap + Loro). +`BasicInstanceHandlers` still registers both `document` and `documentV2`. +Finish the cutover, then delete v1 (grid item, card, class handler). + +Markdown editing is layered, not duplicated: `CollaborativeEditor` → +`AsyncMarkdownEditor` → `MarkdownInput` → `InputMarkdown` → table +`MarkdownCell`. Leave that stack. + +### Device pairing vs resource invites + +Easy to confuse, different security models: + +- Device pairing: `SyncRoute`, `PairingCode`, `ConnectToDeviceForm`, Flutter + `pair_screen.dart` +- Resource invite: `InvitePage`, `InviteForm`, `ShareRoute` + +Do not merge. The onboarding plan already insists on distinct vocabulary. + +--- + +## Flutter ↔ browser twins (do not merge UIs) + +Documented in [`sync-onboarding-ux.md`](./sync-onboarding-ux.md). Tauri is +not a third UI — it loads the same SPA. + +| Concern | Browser | Flutter | Gap | +| --- | --- | --- | --- | +| Canvas draw + undo scrub | `views/Canvas/` | `canvas/infinite_canvas.dart` | Phase B of canvas-undo plan: Dart `_allActions` stack still exists | +| Pairing UI | `SyncRoute` / `PairingFlowProvider` | `pair_screen.dart` | Flutter parses `atomic://pair` locally instead of sharing `pairing.ts` | +| Server URL | `helpers/serverUrl.ts` | `atomic/server_url.dart` | Twin, tested separately | +| Documents, tables, chat, AI | data-browser | absent | Expected; Flutter is canvas-first | + +`flutter/AGENTS.md` still says Loro is "the biggest remaining gap" and +strokes are stored as JSON. That contradicts +[`canvas-undo-consolidation.md`](./canvas-undo-consolidation.md) (Phase A +landed; tap-undo is Loro `UndoManager`). Stale agent context is its own +kind of duplication. + +Flutter `flutter/rust/src/api/simple.rs` (~1444 lines) is app-specific +canvas/folder/peer glue. WASM `wasm/src/lib.rs` is closer to a generic +node API. Per the runtime plan, Flutter should shrink toward WASM's +surface, not grow more canvas FFI. + +--- + +## Server-internal copies + +| Copy | Where | Action | +| --- | --- | --- | +| Subscribe `check_read` ×3 | `server/src/commit_monitor.rs` (`Subscribe`, `SubscribeDrive`, `SubscribeQuery`) | One helper; or fold into [`unify-subscription-primitives.md`](./unify-subscription-primitives.md) | +| `SUB` / `UNSUB` still hand-rolled | `server/src/handlers/web_sockets.rs` | Last actor-bound frames after GET/AUTH/COMMIT moved to the engine (`unified-sync.md` inventory item 1) | +| AUTH parse ×3 | `engine.rs`, `web_sockets.rs`, `peer.rs` | `unified-sync.md` inventory item 2 — still open | +| Compact-VV build ×2 | `peer.rs` vs browser `computeDriveSyncState` | inventory item 3 | +| Six `sync_drive_with_peer*` entry points | `lib/src/sync/peer.rs` | inventory item 5 | + +Query collection construction is already shared +(`construct_collection_from_params`). That is the pattern blob/search +should follow. + +--- + +## Giant files that mix jobs + +These are not copy-paste duplicates, but they prevent consolidation because +too many concerns share a type: + +| File | Lines | Mixes | +| --- | --- | --- | +| `browser/lib/src/store.ts` | 5358 | HTTP, WS, OPFS, outbox, subscriptions, drive sync | +| `browser/lib/src/resource.ts` | 3680 | cache, Loro, signing, undo, genesis, datatype tags | +| `lib/src/loro.rs` | 2961 | wrapper, tags, materialization, tests | +| `lib/src/commit.rs` | 2555 | builder, sign, apply, serialize | +| `lib/src/resources.rs` | 2323 | CRUD, save paths, Loro mirror | + +Splitting these is a prerequisite for +[`unified-data-layer.md`](./unified-data-layer.md), not a separate cleanup. + +Worker-bound copies (`STORAGE_BLOCKED_MARKER`, WASM URL helpers in +`client-db.ts` vs `client-db-open.ts` vs `wasm-url.ts`) are **intentional** +— Vite worker bundling. Do not merge; `client-db-open.test.ts` guards this. + +--- + +## Docs that overlap + +| Topic | Files | Keep | +| --- | --- | --- | +| Loro as authority | `AGENTS.md`, `loro-source-of-truth.md`, `unify-resource-representations.md`, comments in `loro.rs` / `resource.ts` | Planning docs; AGENTS.md should link, not retell | +| Sync / pairing | `sync-onboarding-ux.md`, `device-pairing.md`, `unified-sync.md`, both `AGENTS.md`s, `flutter/AGENTS.md` | `sync-onboarding-ux.md` for UX twins; `unified-sync.md` for protocol | +| Flutter Loro status | `flutter/AGENTS.md` (stale) vs `canvas-undo-consolidation.md` | Update Flutter AGENTS | + +`planning/README.md` already says protocol wire format lives in +`docs/src/websockets.md` and planning must not duplicate it. + +--- + +## What not to consolidate + +- Flutter canvas UI into React (or the reverse). +- Resource-invite flow into device-pairing. +- Canvas / table / OpLog / recent-drive "history" implementations. +- Store / OPFS / WASM layering (those are layers, not copies). +- Worker vs main-thread WASM URL helpers. +- `@tomic/svelte` (`browser/svelte/`) vs `@tomic/react` — framework bindings + over the same `@tomic/lib`. Tiny and appropriate. +- Commit signing / genesis encode — dual-maintain with golden vectors. +- Tauri "desktop app" — it is the SPA plus a thin origin helper. + +--- + +## Suggested order (small → structural) + +Work that is local and pays off without waiting on the runtime rewrite: + +1. **Port/env single source** — stop 9883/9885 drift. +2. **Delete `browser/lib/src/urls.ts`** — migrate the six remaining imports + to generated ontologies. +3. **Shared golden fixtures** for pairing URIs, `normalizeServerUrl`, + datatype tags, and Tantivy key escaping. +4. **`ws_apply::apply_commit_json` → `ingest_commit_json`** with an explicit + `CommitIngestOpts` for the Flutter/replica role. +5. **Extract `SearchResultsList`**; rename `useDriveHistory`. +6. **One `check_read` helper** in `commit_monitor.rs`. +7. **Update `flutter/AGENTS.md`** so it matches the Loro canvas path. + +Then the existing plans, in this order, because each removes a class of +copies rather than one function: + +8. [`unify-subscription-primitives.md`](./unify-subscription-primitives.md) — + kills the three subscribe maps and the three `check_read` blocks together. +9. [`atomic-lib-runtime.md`](./atomic-lib-runtime.md) blob + search move — + kills handler-owned semantics. +10. [`unified-data-layer.md`](./unified-data-layer.md) — kills the browser's + many ingresses; requires splitting `store.ts` / `resource.ts`. +11. Protocol codegen (`protocol.rs` ↔ `ws-v2.ts`) once the tag set is stable + under unified-sync. + +--- + +## File index + +**Ingest / Loro / protocol:** `lib/src/sync/engine.rs`, `lib/src/sync/ws_apply.rs`, +`lib/src/sync/protocol.rs`, `lib/src/resources.rs`, `lib/src/commit.rs`, +`lib/src/loro.rs`, `wasm/src/lib.rs`, `server/src/handlers/commit.rs`, +`server/src/handlers/web_sockets.rs`, `server/src/commit_monitor.rs` + +**TS mirrors:** `browser/lib/src/ws-v2.ts`, `websockets.ts`, `commit.ts`, +`resource.ts`, `datatypes.ts`, `subject.ts`, `urls.ts`, `ontologies/*.ts`, +`collection.ts`, `search.ts`, `pairing.ts`, `genesis.ts` + +**UI twins:** `browser/data-browser/src/helpers/serverUrl.ts` ↔ +`flutter/lib/atomic/server_url.dart`; `views/Canvas/` ↔ +`flutter/lib/canvas/`; `SyncRoute.tsx` ↔ `flutter/lib/screens/pair_screen.dart` + +**Search UI:** `browser/data-browser/src/routes/Search/SearchOverlay.tsx`, +`SearchRoute.tsx` From 4bb957e3467e73c29b7df8ade0199febe5d8f525 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sat, 15 Aug 2026 16:28:49 +0000 Subject: [PATCH 2/2] docs: add a consolidation contract that gates line count and behavior A cleanup is only allowed to land if characterization tests pin the old paths first, then scripts/consolidation-measure.py shows production lines in scope went down. Policy-splits (same shape, different CommitOpts) are called out so they cannot hide a behavior change. Co-authored-by: joepmeindertsma --- AGENTS.md | 5 + planning/README.md | 1 + planning/consolidation-contract.md | 203 ++++++++++++++++ planning/duplication-and-consolidation.md | 12 +- scripts/consolidation-measure.py | 282 ++++++++++++++++++++++ 5 files changed, 500 insertions(+), 3 deletions(-) create mode 100644 planning/consolidation-contract.md create mode 100755 scripts/consolidation-measure.py diff --git a/AGENTS.md b/AGENTS.md index 171ed81dc..b48d55b08 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -16,6 +16,11 @@ Use todo lists and checkboxes to track progress. Make sure to update the planning as you find new insights and see outdated planning text. Remove planning documents that are completed. +A change that claims to remove duplication must follow +[`planning/consolidation-contract.md`](./planning/consolidation-contract.md): +characterization tests on the old code first, then +`scripts/consolidation-measure.py` so production lines in scope go down. + ## Quick Dev Setup Use the Charlotte MCP server and navigate to `http://localhost:6747/app/dev-drive` to instantly create a fresh agent. diff --git a/planning/README.md b/planning/README.md index debb86ce6..249d30a9f 100644 --- a/planning/README.md +++ b/planning/README.md @@ -37,6 +37,7 @@ scratch document. When a plan becomes obsolete, delete it. | [`commit-fanout-drive-isolation.md`](./commit-fanout-drive-isolation.md) | **Active:** Drive-scoped WS commit fan-out — closes the cross-tenant commit leak and the e2e 401-spillover flake; tracks the chatroom guest-drive client-hydration regression and its fix. | | [`structural-problems-index.md`](./structural-problems-index.md) | 2026-05-28 audit: ranked open structural issues with per-item plan files. Covers React Compiler / Resource proxy mismatch, subscription unification, save-state signals, Loro-as-authority, subject typing, and remaining actor-message Arc-wrap. | | [`duplication-and-consolidation.md`](./duplication-and-consolidation.md) | **Audit (2026-08-15):** map of duplicated logic (commit ingest, Loro merge, wire protocol, ontology URLs, datatype tags, search/blobs, browser search/create UI, Flutter twins) and what should stay dual-maintained vs merge. Links existing plans; does not restate them. | +| [`consolidation-contract.md`](./consolidation-contract.md) | **Gate:** how a consolidation PR is allowed to land — characterization tests first, then `scripts/consolidation-measure.py` so production lines in scope go down, the largest file does not grow, and behavior is pinned before the edit. | | [`nextgraph-interop.md`](./nextgraph-interop.md) | **Proposal:** read/edit `did:ng:` (NextGraph) resources via a pluggable, scheme-routed Store backend; use Atomic components chrome-free in NextGraph apps. Builds on the elfa-tables proof. | | [`drive-reconciliation.md`](./drive-reconciliation.md) | **Proposal (2026-07-08):** replace the flat whole-drive VV hash with range-based set reconciliation (RBSR) so sync cost tracks the delta, not the drive; optional signed drive state root (reusing genesis `stateHash`) closes F1 + enables offline verification. Records the RBSR-over-hierarchy-Merkle and VV-leaves-first decisions before any build. | | [`sync-onboarding-ux.md`](./sync-onboarding-ux.md) | **Guideline:** the language, the rules of what can reach what, every account/device path, where each client's logic lives, and what is tested. Read before changing a sync or onboarding screen in any client. | diff --git a/planning/consolidation-contract.md b/planning/consolidation-contract.md new file mode 100644 index 000000000..2da221f62 --- /dev/null +++ b/planning/consolidation-contract.md @@ -0,0 +1,203 @@ +# Consolidation contract + +How a change that *claims* to remove duplication is allowed to land. + +The audit in [`duplication-and-consolidation.md`](./duplication-and-consolidation.md) +lists copies. This file is the gate: **a consolidation PR is not done until +production lines in scope went down, the remaining path is the one a reader +would look for, and behavior is pinned by tests written against the old code.** + +You cannot get this from a green CI run. CI does not know whether you deleted +a copy or inlined a third one. The steps below are the substitute for a +guarantee. + +## The three properties fight each other + +| You do | Lines | Legibility | Behavior | +| --- | --- | --- | --- | +| Delete a second implementation | down | up | preserved *only if* the two paths already agreed | +| Split `store.ts` into modules | **up** (imports, re-exports) | up | preserved | +| Generate TS from Rust | hand-written down, generated up | up | preserved *only if* golden vectors pin both | +| "Unify" two paths that already disagree | down | up | **changed** | + +So the rule is not "always do all three." The rule is: **pick the kind, then +satisfy that kind's gates.** A PR that only moves code between files fails +the line-count gate on purpose. Split a giant file in the *same* PR that +deletes a path, so the net is still down. + +## Kind + +Write this at the top of the PR, one of: + +1. **Delete-duplicate** — two implementations of the same job in the same + language. End state: one function, the other name is gone. +2. **Policy-split** — same shape, *different* behavior (example: + `ingest_commit_json` vs `ws_apply::apply_commit_json`). End state: one + function plus an explicit opts/preset. Any preset that tightens validation + is a behavior change and needs its own tests, not a "cleanup" label. +3. **Bind-twins** — two languages that must keep producing the same bytes + (signing, wire tags, pairing envelope, `normalizeServerUrl`). Do **not** + delete either copy. End state: both load one shared fixture under + `testdata/`, the way `testdata/pairing-request.json` already binds the + browser and the server. See [`TESTING_COVERAGE.md`](../TESTING_COVERAGE.md) + "one-sided contracts." +4. **Extract** — split a large file. Allowed only as part of kind 1 or 2 in + the same PR. Extract-only is rejected. + +If an equivalence test (step 2) fails, you do not have kind 1. You have +kind 2. Stop and say so. + +## Required sequence + +Do these in order. The tests in step 2 must land *before* the production +edit, on the unrefactored code. That is what makes "preserved functionality" +checkable instead of hoped-for. + +### 1. Name the job and the scope + +One sentence, plus the file paths the line counter will use. + +> Job: apply a signed commit JSON body to a `Db`. +> Scope: `lib/src/sync/ws_apply.rs`, `lib/src/sync/engine.rs`, +> `flutter/rust/src/api/simple/ws_sync.rs`. + +Scope is the files that implement the job, not the whole repo. Measuring +the repo hides a 200-line win inside noise. + +### 2. Pin behavior on the *current* code + +**Characterization tests first.** They must pass on `develop` before you +change production code. Commit them separately if that keeps the diff +readable. + +What to write depends on kind: + +**Kind 1 (true duplicates).** While both implementations still exist: + +```text +for case in fixtures { + assert_eq!(path_a(&case), path_b(&case)); +} +``` + +Same inputs, same outputs, including error strings where callers branch on +them. If this assertion fails, switch to kind 2. + +**Kind 2 (policy-split).** Do not force the paths to agree. Write one test +per preset that names the policy (`validate_rights: false` on the replica +path, etc.). After the merge, those tests still call the *same function* +with different opts. A later PR that turns a replica into a hub is then +obvious: a preset test fails. + +**Kind 3 (bind-twins).** One file under `testdata/` (or +`lib/src/genesis_test_vectors.json`). Both language suites load it. A field +rename breaks both, which is the point. Existing models: + +- `testdata/pairing-request.json` — browser sends, server accepts +- `lib/src/genesis_test_vectors.json` — Rust and TS byte-identical certs +- RBSR / drive-hash golden vectors in `lib/` and `browser/lib/` + +**Kind 4.** No extra tests beyond the kind-1/2 tests in the same PR. + +Also run the layer that [`TESTING_COVERAGE.md`](../TESTING_COVERAGE.md) +already names for this flow (protocol / glue / e2e). A unit test of the new +helper is not a substitute for the glue test that used to cover the deleted +path. + +### 3. Measure before + +```sh +scripts/consolidation-measure.py --write /tmp/consol-before.json -- \ + lib/src/sync/ws_apply.rs lib/src/sync/engine.rs +``` + +Paste the table into the PR. + +### 4. Make the change + +One remaining implementation. Do not leave the old name as a deprecated +alias unless it is a published API (`@tomic/lib` export, Flutter FFI). If +you must keep an alias, it is a one-line call through — not a second body — +and it counts as a public item, so the public-item gate will notice. + +### 5. Measure after, grep, tests + +```sh +scripts/consolidation-measure.py --baseline /tmp/consol-before.json -- \ + lib/src/sync/ws_apply.rs lib/src/sync/engine.rs +``` + +The script exits non-zero unless the gates below pass. + +Then: + +- Characterization / equivalence / golden tests still pass, **against the + same expected values** you recorded in step 2. Updating the fixtures in + the same PR as the refactor means you are not measuring preservation. +- `rg -n 'old_function_name'` is empty (changelog and this PR description + excepted). +- The coverage map gets a row if you added a shared fixture. + +## Gates + +`scripts/consolidation-measure.py` enforces the numeric ones. The rest are +the PR checklist. + +| Gate | Kind 1 / 2 / 4 | Kind 3 (bind-twins) | +| --- | --- | --- | +| Hand-written production non-blank lines in scope | **strictly down** | may stay flat or rise by the fixture; must not add a *third* copy | +| Largest file in scope, non-blank lines | must not grow | must not grow | +| Public items in scope (`pub ` / `export `) | must not grow | must not grow | +| Tests / generated files | excluded from the line budget; they may grow | the shared fixture is the feature | +| Equivalence or golden tests | pass before *and* after, fixtures unchanged | both languages load the same file | +| Deleted symbol | zero remaining references | n/a | + +Generated files (`GENERATED WITH`, `@generated`, flutter_rust_bridge output, +`ontologies/*.ts`) are excluded from "hand-written." Deleting a hand-written +`urls.ts` in favor of generated ontologies counts as a win even if the +generated file is large. + +## Legibility (not fully mechanical) + +The line counter cannot tell a clever helper from a maze. These are the +human checks; fail the PR if any is false: + +- A reader who knows the job name can find the remaining function from + `rg` without walking three wrappers. +- The PR description states the remaining path in one sentence + (`engine::ingest_commit_json` is the only apply). +- You did not rename something solely to make the grep for the old name + pass. +- Comments that said "keep in step with ``" are updated or + deleted so they do not describe a copy that is gone. + +If those are true and the numeric gates pass, legibility improved in the +only way this repo can check: **fewer places to read for the same job, +and the biggest file in scope did not get bigger.** + +## What this deliberately rejects + +- A "cleanup" that adds an abstraction layer and keeps both old paths as + callers — lines up, job still has two bodies. +- Splitting `store.ts` with no deleted path — lines up, job count unchanged. +- Merging `ws_apply::apply_commit_json` into the hub ingest *without* + naming the replica preset — that is a rights-check behavior change + disguised as dedup. +- Updating golden expected bytes in the same commit as the encoder change + and calling it "tests still pass." +- Measuring the whole repository so a 2 000-line feature hides a 50-line + duplication win, or the reverse. + +## PR checklist (paste into the description) + +```md +### Consolidation +- Kind: delete-duplicate | policy-split | bind-twins | extract+delete +- Job (one sentence): +- Scope paths: +- Characterization / golden tests committed before the production diff: yes +- `scripts/consolidation-measure.py --baseline` exits 0: yes +- Before / after table pasted below +- `rg` for the deleted name is empty: yes +- TESTING_COVERAGE.md updated if a shared fixture was added: yes / n/a +``` diff --git a/planning/duplication-and-consolidation.md b/planning/duplication-and-consolidation.md index 9c4a964e8..491fc59b7 100644 --- a/planning/duplication-and-consolidation.md +++ b/planning/duplication-and-consolidation.md @@ -1,8 +1,13 @@ # Duplication and consolidation audit (2026-08-15) Index of things that exist more than once, and whether they should stay that -way. This is a map, not a rewrite plan. Where a dedicated plan already owns -the work, this file links to it instead of restating it. +way. This is a map, not a rewrite plan. How a listed copy is allowed to land is +[`consolidation-contract.md`](./consolidation-contract.md): characterization +tests on the old code, then a line-count gate so the remaining path is smaller +and the old behavior is still pinned. + +Where a dedicated plan already owns the work, this file links to it instead of +restating it. **Related plans (do not duplicate):** @@ -377,7 +382,8 @@ Worker-bound copies (`STORAGE_BLOCKED_MARKER`, WASM URL helpers in ## Suggested order (small → structural) -Work that is local and pays off without waiting on the runtime rewrite: +Work that is local and pays off without waiting on the runtime rewrite. +Every item still has to pass [`consolidation-contract.md`](./consolidation-contract.md). 1. **Port/env single source** — stop 9883/9885 drift. 2. **Delete `browser/lib/src/urls.ts`** — migrate the six remaining imports diff --git a/scripts/consolidation-measure.py b/scripts/consolidation-measure.py new file mode 100755 index 000000000..7f1b3d764 --- /dev/null +++ b/scripts/consolidation-measure.py @@ -0,0 +1,282 @@ +#!/usr/bin/env python3 +"""Measure hand-written production code in a declared consolidation scope. + +See planning/consolidation-contract.md. Typical use: + + scripts/consolidation-measure.py --write /tmp/before.json -- path [path...] + # ... edit ... + scripts/consolidation-measure.py --baseline /tmp/before.json -- path [path...] + +Exits 1 when --baseline is set and a gate fails (production non-blank lines +did not drop, or max file / public items grew). +""" + +from __future__ import annotations + +import argparse +import json +import re +import sys +from pathlib import Path + +REPO = Path(__file__).resolve().parent.parent + +CODE_SUFFIXES = {".rs", ".ts", ".tsx", ".js", ".jsx", ".dart", ".py"} +SKIP_DIR_NAMES = { + "target", + "node_modules", + "dist", + ".git", +} +TEST_SUFFIXES = {".test.ts", ".test.tsx", ".test.js", ".spec.ts", ".spec.tsx"} +GENERATED_MARKERS = ( + "GENERATED WITH", + "@generated", + "auto-generated", + "Code generated by", + "flutter_rust_bridge", +) +CFG_TEST_RE = re.compile(r"(?m)^#\[cfg\(test\)\]\s*\n") +PUB_RES = { + ".rs": re.compile(r"^\s*pub\s"), + ".ts": re.compile(r"^export\s"), + ".tsx": re.compile(r"^export\s"), + ".js": re.compile(r"^export\s"), + ".jsx": re.compile(r"^export\s"), + ".dart": re.compile(r"^(class|enum|mixin|extension)\s"), +} + + +def split_rust_cfg_test(path: Path, text: str) -> tuple[str, str]: + """Split trailing Rust `#[cfg(test)]` modules out of the production budget.""" + if path.suffix != ".rs": + return text, "" + match = CFG_TEST_RE.search(text) + if not match: + return text, "" + return text[: match.start()], text[match.start() :] + + +def is_test_path(path: Path) -> bool: + name = path.name + if name.endswith("_test.dart") or name.endswith("_tests.rs"): + return True + if any(name.endswith(s) for s in TEST_SUFFIXES): + return True + if name in {"tests.rs", "test.rs"} or name.endswith("_e2e.rs"): + return True + parts_lower = [p.lower() for p in path.parts] + return any(p in {"tests", "test", "__tests__"} for p in parts_lower[:-1]) + + +def is_generated(path: Path, text: str) -> bool: + if "ontologies" in path.parts and path.suffix == ".ts": + return True + return any(marker in text[:800] for marker in GENERATED_MARKERS) + + +def iter_files(roots: list[Path]) -> list[Path]: + files: list[Path] = [] + for root in roots: + if not root.exists(): + raise SystemExit(f"path does not exist: {root}") + if root.is_file(): + files.append(root) + continue + for path in root.rglob("*"): + if not path.is_file() or path.suffix not in CODE_SUFFIXES: + continue + if any(part in SKIP_DIR_NAMES for part in path.parts): + continue + if "src/rust" in path.as_posix(): + continue + files.append(path) + return sorted(set(files)) + + +def count_file(path: Path) -> dict: + text = path.read_text(encoding="utf-8", errors="replace") + generated = is_generated(path, text) + test = is_test_path(path) + if generated or test: + prod_text, inline_test = text, "" + else: + prod_text, inline_test = split_rust_cfg_test(path, text) + prod_lines = prod_text.splitlines() + inline_test_nonblank = sum(1 for line in inline_test.splitlines() if line.strip()) + nonblank = sum(1 for line in prod_lines if line.strip()) + pub_re = PUB_RES.get(path.suffix) + public_items = 0 + if not generated and not test and pub_re: + public_items = sum(1 for line in prod_lines if pub_re.search(line)) + try: + rel = str(path.resolve().relative_to(REPO)) + except ValueError: + rel = path.as_posix() + return { + "path": rel, + "physical": len(prod_lines), + "nonblank": nonblank, + "public_items": public_items, + "generated": generated, + "test": test, + "inline_test_nonblank": inline_test_nonblank, + } + + +def summarize(file_rows: list[dict]) -> dict: + prod = [row for row in file_rows if not row["test"] and not row["generated"]] + tests = [row for row in file_rows if row["test"]] + generated = [row for row in file_rows if row["generated"] and not row["test"]] + max_row = max(prod, key=lambda row: row["nonblank"], default=None) + return { + "production_files": len(prod), + "production_nonblank": sum(row["nonblank"] for row in prod), + "production_physical": sum(row["physical"] for row in prod), + "public_items": sum(row["public_items"] for row in prod), + "max_file": max_row["path"] if max_row else None, + "max_file_nonblank": max_row["nonblank"] if max_row else 0, + "test_nonblank": sum(row["nonblank"] for row in tests) + + sum(row.get("inline_test_nonblank", 0) for row in prod), + "generated_nonblank": sum(row["nonblank"] for row in generated), + "files": file_rows, + } + + +def render(summary: dict) -> str: + return "\n".join( + [ + f"production files: {summary['production_files']}", + f"production non-blank: {summary['production_nonblank']}", + f"production physical: {summary['production_physical']}", + f"public items: {summary['public_items']}", + f"largest file: {summary['max_file']} ({summary['max_file_nonblank']})", + f"test non-blank: {summary['test_nonblank']} (excluded from budget)", + f"generated non-blank: {summary['generated_nonblank']} (excluded from budget)", + ] + ) + + +def compare(before: dict, after: dict) -> list[str]: + failures = [] + if after["production_nonblank"] >= before["production_nonblank"]: + failures.append( + "production non-blank lines did not decrease " + f"({before['production_nonblank']} → {after['production_nonblank']})" + ) + if after["max_file_nonblank"] > before["max_file_nonblank"]: + failures.append( + "largest file in scope grew " + f"({before['max_file']} {before['max_file_nonblank']} → " + f"{after['max_file']} {after['max_file_nonblank']})" + ) + if after["public_items"] > before["public_items"]: + failures.append( + "public items in scope grew " + f"({before['public_items']} → {after['public_items']})" + ) + return failures + + +def self_test() -> None: + import tempfile + + with tempfile.TemporaryDirectory() as td: + root = Path(td) + (root / "prod.rs").write_text( + "pub fn a() {}\n\n" + "pub fn b() {}\n\n" + "#[cfg(test)]\n" + "mod tests {\n" + " #[test]\n" + " fn t() {}\n" + "}\n" + ) + (root / "a.test.ts").write_text("export const t = 1;\n") + (root / "ontologies").mkdir() + (root / "ontologies" / "core.ts").write_text( + "/* GENERATED WITH @tomic/cli */\nexport const core = {};\n" + ) + rows = [count_file(p) for p in iter_files([root])] + summary = summarize(rows) + assert summary["production_files"] == 1, summary + assert summary["production_nonblank"] == 2, summary + assert summary["public_items"] == 2, summary + assert summary["test_nonblank"] == 6, summary + assert summary["generated_nonblank"] == 2, summary + + before = { + k: summary[k] + for k in ( + "production_nonblank", + "max_file", + "max_file_nonblank", + "public_items", + ) + } + (root / "prod.rs").write_text("pub fn a() {}\n") + after = summarize([count_file(p) for p in iter_files([root])]) + assert compare(before, after) == [], compare(before, after) + + grown = dict(before) + grown["production_nonblank"] = before["production_nonblank"] + 1 + assert compare(before, grown), "should fail when lines grow" + print("self-test ok") + + +def main(argv: list[str]) -> int: + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("paths", nargs="*", type=Path) + parser.add_argument("--write", type=Path, help="write JSON snapshot") + parser.add_argument("--baseline", type=Path, help="compare against a prior --write") + parser.add_argument("--json", action="store_true", help="print JSON instead of the table") + parser.add_argument("--self-test", action="store_true") + args = parser.parse_args(argv) + + if args.self_test: + self_test() + return 0 + + if not args.paths: + parser.error("pass one or more files or directories after --") + + roots = [p if p.is_absolute() else REPO / p for p in args.paths] + summary = summarize([count_file(p) for p in iter_files(roots)]) + payload = {key: value for key, value in summary.items() if key != "files"} + payload["files"] = [ + { + key: row[key] + for key in ("path", "nonblank", "public_items", "test", "generated") + } + for row in summary["files"] + ] + + if args.write: + args.write.write_text(json.dumps(payload, indent=2) + "\n") + + if args.json: + print(json.dumps(payload, indent=2)) + else: + print(render(summary)) + if len(summary["files"]) <= 30: + print() + for row in summary["files"]: + tag = " test" if row["test"] else " generated" if row["generated"] else "" + print(f" {row['nonblank']:5d} {row['path']}{tag}") + + if args.baseline: + before = json.loads(args.baseline.read_text()) + failures = compare(before, summary) + if failures: + print("\nGATE FAILED:", file=sys.stderr) + for failure in failures: + print(f" - {failure}", file=sys.stderr) + return 1 + print( + "\ngates passed (production lines down; max file and public items did not grow)" + ) + return 0 + + +if __name__ == "__main__": + sys.exit(main(sys.argv[1:]))