[malibio]

Closes #2807

Summary

person and project are core types but had no typed representation. The frontend read their fields out of the properties bag, which broke person forms once the two transports disagreed on the bag's shape. This follows the task pattern end to end, so core types are strongly typed. The properties bag and the storage namespace are now internal only.

Backend

• nodespace-types:
  • New PersonNode/ProjectNode wire structs and PersonNodeUpdate/ProjectNodeUpdate (tri-state: absent / null = clear / value).
  • node_to_typed_value now moves core fields out of properties for task, person and project (the documented "properties": {}), so each core field has one home.
  • New promoted_fields and flat_properties_view let machine consumers rebuild the storage-keyed map.
• Core: update_person_node/update_project_node check the node type, then send the typed patch through the generic update pipeline. That brings schema/enum validation, title_template, OCC, invariant dispatch, events, embeddings and mentions along without another copy of the pipeline.
• Proto, daemon, Tauri, dev-proxy, adapters: UpdatePersonNode/UpdateProjectNode end to end, mirroring UpdateTaskNode, including the typed current_node embed on a version conflict.
• Children tree: nodes are serialized through node_to_typed_value with the extends chain collapsed, so tree-loaded nodes match single-node reads. Salvaged from Read and write node properties flat on every transport (closes #2807) #2934.
• Machine consumers keep their flat, storage-keyed view:
  • The CLI's relationship get folds promoted fields back under their storage keys (due_date, not dueDate).
  • The agent's get_node, search summaries and create_node property count use flat_properties_view, so what the model sees is unchanged.

Frontend

• New PersonNode/ProjectNode interfaces and converters, wired into normalizeNodeData. nodeToTaskNode now keeps properties/title, which fixes user-defined task fields on main.
• TYPED_CORE_FIELDS registry (mirrors promoted_fields). It drives:
  • the dev-proxy's storage → wire conversion (including defaults and date normalization);
  • the store's routing;
  • resolveFieldValue/buildFieldWrite, used by GenericSchemaForm, Kanban and viewer field edits.
• Store: one generic updateTypedNode() replaces the 356-line updateTaskNode body. updateTaskNode/updatePersonNode/updateProjectNode are thin wrappers.
  • The per-field sequence guards and the OCC and play-rule handling are kept.
  • New: typed fields accumulate across the coordinator's single queued slot, so a superseded typed write no longer loses its fields.
  • updateNode() routes typed keys there, with onPersistSuccess/onPersistError passed through for Kanban reverts.
  • The plugin updater API is removed; it's dead now.
• Consumers: PersonSchemaForm and IdentityCard read firstName/lastName/email and write through the typed update; an emptied field is cleared. Project's form reads its typed fields through the registry.

Also in this PR

• schema_chain_blindness_guard_test was failing on main: Harden rename_schema_field: in-tx validation, version bump, chunked closure #3030 moved the rename checks into validate_schema_field_rename after the guard landed. The allowlist entry now points at the new function.

Docs

The nodespace-docs main branch has the layer-by-layer core-type recipe in creating-node-types.md, the serialization contract updated in data-layer.md (field access decided by type, not protection level), and the plugin-updater references removed.

Test plan

• Rust:
  • wire_contract tests for person and project, the move semantics and the flat view;
  • core service tests (write, clear, conflict, wrong type, vocabulary);
  • daemon RPC tests;
  • Tauri adapter contract test for person;
  • CLI and agent tests.
• Frontend:
  • new update-typed-node.test.ts (dispatch, accumulation, routing, mixed write, onPersistError);
  • updated form, Kanban, resolution and normalize tests;
  • the task clobber-regression suites pass unchanged on the generic path.
• e2e (real daemon through the dev-proxy): typed task, person and project update round-trips; typed tree nodes.
• Pre-push gate passes.

🤖 Generated with Claude Code

[malibio]

Pragmatic Code Review: PR #3033 (Type person and project nodes like task, #2807)

Review Type

Initial review. No earlier reviews or comments on the PR. Scope: 62 files, +2922/−1454, covering nodespace-types, core, proto, daemon, Tauri, dev-proxy, adapters, store, forms, Kanban, CLI, agent and tests.

Summary: This is a solid, well-scoped change and clearly a net positive. It implements the documented serialization contract instead of working around it:

• One home per core field. Typed conversion now moves core fields out of properties.
• Transports agree. The dev-proxy converter mirrors node_to_typed_value, and the children tree now serializes through the same function.
• Less duplication. The generic updateTypedNode() replaces 356 lines of task-only code.
• Less custom pipeline. Person and project writes reuse the generic update_node pipeline instead of copying it, so validation, title_template, OCC, invariants, events and embeddings come for free.
• Dead API removed. The plugin updater indirection is deleted.

I checked the consumer sweep broadly. No frontend code still reads a task, person or project core field through node.properties. The machine consumers (CLI relationship get, the agent's get_node, search summaries and resolve_query) use flat_properties_view. The two new unit suites pass locally (update-typed-node.test.ts and node-normalize.test.ts, 32/32).

The one substantive gap is in the store's accumulation guarantee: it holds only between typed writes, not when a generic write takes the coordinator's queued slot. Details below.

---

Requirements Check

#	Acceptance criterion	Status	Notes
1	Person shows stored first/last/email in Tauri and browser mode, loaded by single fetch or children tree	✅	Four pieces cover it: person_node_to_value, the dev-proxy storageNodeToApiFields, the children tree now using node_to_typed_value (with a Rust test), and PersonSchemaForm/IdentityCard reading node.firstName/lastName/email.
2	Editing a person field and tabbing away keeps the value after the daemon response and after reopen	✅	updatePersonNode goes to the typed RPC, backed by the e2e round-trip. Caveat: the queue race in finding 1.
3	Core fields travel top-level on every transport; no frontend code reads or writes core fields via node.properties	✅	Grep is clean outside tests and comments. Kanban, GenericSchemaForm and updateSchemaField all go through resolveFieldValue/buildFieldWrite.
4	Typed conversion leaves only user-protected fields in properties	✅	Implemented as "promoted fields are moved". Task priority/dates and project priority/dates are schema user-protected but promoted anyway. That matches the updated contract (field access is decided by type, not protection level), which is the right call.
5	Typed update RPCs for person and project with validation, title recompute and OCC	✅	Both delegate to update_node. Tests cover write, clear, conflict, wrong type and vocabulary. The daemon embeds a typed current_node on conflict.
6	Title preview and email duplicate check keep working	✅	PersonSchemaForm keeps pushComputedTitle and findDuplicateFor. GenericSchemaForm's preview now reads through resolveFieldValue.
7	Kanban grouping by task/project status still moves and persists cards	✅	resolveFieldWrite produces a typed change, and updateNode routes it to updateTypedNode with onPersistError passed through (tested).
8	User-defined fields on core types display and persist	✅	nodeToTaskNode keeps properties. TaskSchemaForm reads and writes flat. There is a test for a generic persist of a custom: field on a task.
9	Regression tests: DB-loaded person render, tree-loaded typed shape, typed person e2e round-trip	✅	person-schema-form tests, the get_children_tree_serves_typed_nodes_like_single_node_reads Rust test and e2e, and adapter-contract.e2e.ts for person and project.
10	quality:fix and the pre-push gate pass	✅	Per the PR; I did not re-run the full gate.

---

Code Review Findings

🟡 1. A typed write superseded by a generic write is never sent

📁 packages/desktop-app/src/lib/services/shared-node-store.svelte.ts:1589-1612 (routing and mixed-write split) and :3160-3173 (the only place pendingTypedFields is drained)

SimplePersistenceCoordinator has one queued slot per node, and typed and generic closures share it. Only the typed closure drains pendingTypedFields. So when a generic write replaces a queued typed write, the typed fields stay in pendingTypedFields and are never sent.

Sequence:

1. A write for node N is in flight.
2. A typed write is queued for N.
3. A generic write for N arrives, for example a content keystroke or the rest half of a mixed updateNode split.
4. The generic write takes the queued slot. The typed closure is rejected with OperationCancelledError, which the handle's catch treats as expected and ignores ("Superseded by a newer write, which carries this one's fields" — true only when the superseding write is typed).

Consequences:

• The server never receives the typed field.
• The generic write's success handler can re-apply the server's old value, because the local value equals the currentNode snapshot read at execution time. The field then visibly reverts with no error.
• The leftover fields go out later with the next typed write on that node, possibly minutes later: a ghost write.
• clearAll() (around line 3365) doesn't clear pendingTypedFields, so leftovers also survive a database hot-swap.

The mixed-write split in updateNode() triggers this every time a write is already in flight, and the PR's headline guarantee ("a superseded typed write no longer loses its fields") only holds typed-to-typed. The race class already exists on main for updateTaskNode, but this PR widens it to every typed updateNode caller.

Fix (small):

• Pull the drain into a helper, e.g. flushPendingTypedFields(nodeId), which takes the pending map, calls sendTypedUpdate and applies the sequence-guarded confirmation.
• Call it at the start of the generic updateNode closure, and in persistBatchedChanges, before the generic RPC. Both run under the same per-node serialization, so ordering and OCC still hold (the version is re-read after the typed response).
• Add this.pendingTypedFields.clear() to clearAll().

Tests:

• Add one for this case: a write in flight, then a typed write queued, then a generic write queued. Assert the typed fields reach updateXNode.
• The existing test splits a mixed write... only checks optimistic content, and its typed mock never resolves, so the generic half never runs. Make the typed mock resolve and assert that updateNode receives the content.

Principle: an invariant a design claims should hold under every interleaving the scheduler allows, not only the typed-only ones.

🟡 2. Four hand-synced copies of the core field set, with no drift guard

📁 packages/nodespace-types/src/convert.rs (promoted_fields), packages/desktop-app/src/lib/types/typed-core-fields.ts (TYPED_CORE_FIELDS, TYPED_CORE_DEFAULTS), packages/core/src/models/core_schemas.rs, and the PersonNode/ProjectNode/TaskNode structs

The same field set, with its storage-to-wire mapping and defaults, now lives in four places kept in sync by hand. Earlier architecture notes flag exactly this seam, parallel representations kept in sync by hand, as where bugs live.

Cheap guards:

• A Rust test asserting that, for each promoted type, the storage keys in promoted_fields(t) equal the non-relationship field names in get_core_schemas() for t.
• A test pinning the TS registry to the Rust one. The e2e contract test could compare storageNodeToApiFields output with the Tauri shape for all three types, or a JSON fixture could be generated from Rust.

Without these, adding a core field to person or project silently leaves it stuck in properties on one transport. That is the class of bug this issue exists to fix.

Principle: keep one source of truth, and where duplication is unavoidable, test it.

🟢 3. Dev-proxy and Rust conversion differ at the edges

📁 packages/desktop-app/src/lib/services/node-normalize.ts:159 vs packages/nodespace-types/src/convert.rs:295-330

storageNodeToApiFields reads properties[wire] ?? properties[storage] for every type, and its doc says it does so "as the Rust converters do". Only task_node_to_value does that. person_node_to_value and project_node_to_value read the storage key only, while without_promoted strips both spellings. Other small differences:

• Rust maps an invalid task priority to TaskPriority::default(); TS passes the raw string through.
• Rust also parses space-separated datetimes; TS normalizes only T-separated ones.
• Rust's .get("dueDate").or_else(..) doesn't fall back on an explicit null; TS ?? does.

None of this matters with data written through the typed paths, but the doc claims exact parity. Pick one rule. Given CLAUDE.md's no-compat stance, the cleanest is to drop the legacy camelCase fallback from both sides rather than extend it.

🟢 4. Callback semantics of routed and superseded typed writes

📁 shared-node-store.svelte.ts:1605-1611, :3168, :3206

• Mixed writes fire callbacks twice. options still carries onPersistSuccess/onPersistError into the generic half, so a mixed write fires each callback once per half. No production caller sends a mixed write today; Kanban sends one field. Either strip the callbacks from the generic half, or fire once when both halves settle.
• Callbacks can be skipped. A typed closure that finds an empty payload returns without calling onPersistSuccess. A superseded typed write's callbacks are dropped even though its fields ride along with the newer write. The generic path behaves the same way, so this is consistent. Worth one sentence in the method doc so Kanban-style callers don't assume one callback per call.

🟢 5. Typed routing assumes the node is already persisted

📁 shared-node-store.svelte.ts:1589-1612

On main, a Kanban or generic-form status write went through the generic closure, which creates the node when persistedNodeIds lacks it. The typed route never checks this, so a typed write to a node whose create hasn't landed gets NOT_FOUND. Also, mode: 'immediate' calls cancelPending, which can cancel a debounced generic write, including a create, that is not yet executing. The risk is low today because type conversion persists immediately, but a defensive persistedNodeIds.has(nodeId) gate that falls back to the generic path is cheap.

🟢 6. Two query readers still use their own field-resolution heuristic

📁 packages/desktop-app/src/lib/components/query/query-node-model.ts:150 (readFieldValue), packages/desktop-app/src/lib/components/query/table-row.svelte:67

Kanban moved to resolveFieldValue, but these two still resolve fields as camelCase top-level, then snake_case top-level, then properties. That still works for the typed fields. However, a user-defined field whose name camel-cases to node metadata is shadowed by the metadata: created_at becomes createdAt, and title and version collide directly. That was the exact precedence bug Kanban's old comment described. Move both to resolveFieldValue; table-row keeps its content special case. This keeps one resolution rule for every schema-driven reader.

🟢 7. Children tree now fails as a whole on one bad node

📁 packages/core/src/services/node_service/mod.rs:2827-2828

build_node_tree_guarded now propagates a single node's node_to_typed_value error, which fails the whole page load. The old code fell back to unwrap_or_else. SchemaNode::from_node deliberately swallows parse errors to limit this blast radius, so the risk is low. Acceptable, and consistent with nodes_to_typed_values, but worth knowing.

🟢 Nits

• Nit: schema-field-resolution.ts:59 turns '' into null for every typed field, including project status. The Tauri command deserializes status: null to None, so the update fails with "contains no changes" and shows a generic write-failure. It's guarded in Kanban and unlikely from an enum select; skipping the write for a required field would be cleaner.
• Nit: update_typed_fields (schema.rs:519) does a get_node read before update_node, which reads again. The soundness argument in the doc comment holds, since OCC catches a retype. It's one extra read per write and fine as is.
• Nit: sendTypedUpdate casts Record<string, unknown> to TaskNodeUpdate/PersonNodeUpdate/ProjectNodeUpdate. That is acceptable at a registry-driven boundary. A mapped type from TYPED_CORE_FIELDS would restore compile-time checking if the registry becomes as const.

---

What's done well

• update_person_node/update_project_node delegate to update_node instead of copying a 300-line pipeline, so person and project get invariant dispatch, which update_task_node still lacks. The type-check-then-OCC soundness argument is correct.
• The move semantics include flat_properties_view for machine consumers, so the agent's model-facing view and the CLI's --property shape don't change. The CLI test pins storage-key restoration.
• The schema_chain_blindness_guard allowlist repair is a correct, minimal fix for a test that was failing on main.
• The task clobber-regression suites pass unchanged on the generic path, which is good evidence the sequence-guard semantics were kept.

Recommendation: APPROVE

This is a net improvement that meets every acceptance criterion. I strongly recommend fixing 🟡1 in this PR: it's a small change (drain pending typed fields in the generic closure, clear them in clearAll, add one test), and it makes the PR's own headline guarantee true. 🟡2 is a cheap drift guard worth adding now. The rest are optional follow-ups.

[malibio]

Automated pragmatic-code-review: APPROVE (posted as a comment — GitHub blocks self-approval). Full findings in the review comment above.

[malibio]

Review addressed (commit e476d69)

✅ 🟡1 A typed write superseded by a generic write is never sent.

• New sendPendingTypedFields(nodeId) sends every typed field still pending for the node and applies the confirmation with the per-field sequence guards. It runs first in every closure for that node: typed, generic (updateNode) and batch. A generic or batch write that replaces a queued typed write therefore sends the typed fields ahead of its own RPC, at the right version.
• The pending map records the node type alongside the fields.
• clearAll() now clears the pending typed fields.
• Tests:
  • sends a typed write superseded in the queue by a generic write, ahead of it asserts the order typed(v1) → typed(v2) → generic(v3).
  • The mixed-write test now resolves the typed half and asserts the generic half carries { content } only.

✅ 🟡2 Drift guards.

• Rust: promoted_fields_match_each_typed_core_schema (core_schemas.rs) pins promoted_fields storage keys to each core schema's field names.
• TS: typed-core-fields.test.ts reads promoted_fields from convert.rs and asserts TYPED_CORE_FIELDS lists the same (storage, wire) pairs per type.

✅ 🟢3 Dev-proxy parity at the edges. storageNodeToApiFields now matches the Rust conversion in three places, each pinned by a test:

• The typed-key fallback applies to task only.
• A typed key present as null takes precedence over the storage key, as .get(wire).or_else(storage) does.
• Only datetimes with an offset (T- or space-separated) are reduced to a date; a datetime without an offset passes through, as normalize_date_field leaves it.

✅ 🟢4 Callbacks. On a mixed write, onPersistSuccess/onPersistError belong to the typed half and fire once. The empty-payload and superseded-write semantics are now in the method doc.

✅ 🟢5 Typed routing before create. Routing now requires the node to be in persistedNodeIds; a node still awaiting its create stays on the generic path. Test: keeps a node awaiting its create on the generic path, which fails with the gate inverted.

⏭️ Skipped

• 🟢6 (table-row/query-node-model resolution): these readers also resolve node metadata columns (created_at, title) and read the typed top-level key first, so they're correct for the new shape. Unifying them is a separate change.
• 🟢7 (per-node fallback in the children tree): a failed conversion should fail loudly. Silently serving an untyped node is the bug this PR fixes, and the error can only come from serde serialization.
• Nits: skipped for now. Nothing currently triggers the '' → null case for a required status, since enum selects never produce ''. The extra get_node read keeps the type check sound, and the typed-payload casts are contained in one switch.

Tests: frontend 5,301/5,301 and core lib 1,774 pass; the pre-push gate passes.

Re-review needed: yes. 🟡1 changes the generic and batch persistence closures.

[malibio]

Pragmatic Code Review (Re-Review): PR #3033, commit e476d69

Review Type

Re-Review. Scope is only e476d695 (a0f668ea..e476d695): 6 files, +281/−56.

Previous Review Summary

	Count
Previous recommendations	10 (7 numbered findings + 3 nits)
Addressed	5: 🟡1 flush of superseded typed writes, 🟡2 drift guards, 🟢3 dev-proxy parity, 🟢4 callback semantics, 🟢5 persisted gate
Not addressed (skipped with rationale)	5: 🟢6 query readers, 🟢7 children-tree blast radius, 3 nits
New issues	5: 2 🟡, 3 🟢

The skips are reasonable.

• 🟢6: the query readers do read the typed top-level key first, so they are correct for the new shape.
• 🟢7: failing loudly is the right call.
• Nits: none of them has a current trigger.

The addressed items land.

• 🟡1 core fix is correct. sendPendingTypedFields() sits inside the generic closure's outer try but outside the inner NOT_FOUND→create try. A failed typed flush therefore cannot trigger the create fallback. OCC from the flush reaches the generic OCC handler, which rehydrates from current_node and clears the queue. The pending map was already deleted before the send, which matches the typed path.
• Version handling is right. The generic closure reads currentNode (and so currentNode.version) after the flush. The batch closure also reads currentNode after the flush. The generic success handler's scoped-field snapshot is taken after the flush, so it already includes the typed confirmation and won't re-stamp stale typed values.
• clearAll() now clears pendingTypedFields.
• The ordering test (typed(v1) → typed(v2) → generic(v3)) pins the fix, and the mixed-write test now really checks the generic half.
• The drift guards work. The Rust test pins promoted_fields to each schema's fields. The TS test parses the promoted_fields match arms, and the ['person','project','task'] key assertion makes a parse miss fail loudly rather than pass vacuously.

---

Code Review Findings

🟡 A. The persisted gate sends typed fields down a path that silently drops them

📁 packages/desktop-app/src/lib/services/shared-node-store.svelte.ts:1601-1610 (gate), :2019-2031 (create branch), :1850-1860 (generic update payload)

When a typed node is not yet in persistedNodeIds, { status: 'active' } now takes the generic path. Neither branch of the generic closure can carry a top-level typed field:

• Create branch: it sends properties: currentNode.properties. The typed field now lives top-level on the node, not in properties, so the node is created with the schema default (planning/open).
• Update branch: this applies when the create lands before the closure runs. The closure sends { status: 'active' } to update_node. NodeUpdate (nodespace-types/src/node.rs:207) has no status field and no deny_unknown_fields, so serde drops it.

Both branches then report success, so the optimistic value stays on screen until the next reload. Before this commit, the same write failed loudly with NOT_FOUND and a write-failure toast. The window is narrow (a node whose create hasn't resolved), but the failure changed from loud to silent data loss. The new test (keeps a node awaiting its create on the generic path) only asserts the optimistic value and that the typed RPC was not called, so it doesn't catch this.

Fix (small):

• In the unpersisted branch, still record the typed fields in pendingTypedFields (write optimistically and bump the sequence numbers, but don't schedule a typed persist).
• Call await this.sendPendingTypedFields(nodeId) in the generic closure's create branch, after createNode and the version getNode. Do the same in the batch create and UNIQUE-race branches.

The create keeps its own path, the typed field goes out at the confirmed version, and "whoever runs next flushes" becomes true for creates as well.

Test: extend the new test so the create resolves, then assert that updateProjectNode receives { status: 'active' }.

Principle: a fallback must preserve the write's semantics, not only avoid the error.

🟡 B. A typed-flush failure inside a generic or batch closure takes the unrelated generic write down with it, and the typed caller never hears about it

📁 shared-node-store.svelte.ts:1820, :4563, :1627, doc at :3180-3186

The flush error becomes the generic write's own error. For the generic closure:

• rollbackUpdate() rewinds the generic write's bookkeeping, and the generic payload (content, custom: properties) is never sent.
• The generic caller's onPersistError fires for a failure in fields it never wrote. For a Kanban custom: column move, the card reverts even though its own field was never attempted.

The realistic trigger is a play-rule rejection (ADR-060 invariants on task status transitions) or a vocabulary validation error on the stranded typed field. The user's unrelated content edit is then dropped from the server, while local content stays optimistic until the next keystroke.

Callbacks go wrong in the other direction too:

• The typed write that was superseded had its callbacks dropped. The doc says its fields are "sent, and reported, by the write that replaced it", but the replacing write reports to its own caller. A Kanban typed move superseded by a generic write never gets onPersistSuccess (so chainOrigin leaks) or onPersistError (so the card isn't reverted).
• A mixed updateNode fires zero callbacks, not one, when a write is already in flight. The typed half queues, the generic half (callbacks stripped at :1627) replaces it, and the typed closure never runs.

Fix:

• Store the callbacks alongside the fields in the pendingTypedFields entry (for example callbacks: Array<{onPersistSuccess, onPersistError}>). sendPendingTypedFields() then fires them on its own outcome, whichever closure runs it.
• In the generic and batch closures, let OCC propagate (it has to, since the version is now unknown). For any other flush error, notify with the typed callbacks and a write-failure, then continue with the generic RPC, so one field's rejection doesn't also discard a different field's write.
• Add a failure-path test: queue a typed write, supersede it with a generic write, make the typed RPC reject with a non-OCC error. Assert the typed onPersistError fires, the generic RPC still runs, and createNode is not called.

Principle: isolate failure domains. An error should be reported to the caller that owns the failed intent.

🟢 C. The flush runs before the pending-move wait

📁 shared-node-store.svelte.ts:1820 vs :1832-1843; :4563 vs :4566-4578

Both closures wait on getPendingMoveOperation() specifically because a move bumps the server version. The flush runs before that wait, reading the pre-move version, so a stranded typed flush racing an indent/outdent gets a VERSION_CONFLICT, which then also fails the generic write through the OCC path. Moving the flush after the move wait costs nothing, and the version re-read after the flush still holds. The typed closure (:3245) never waits on pending moves either (this predates the PR). Putting the wait inside sendPendingTypedFields() fixes all three call sites at once.

🟢 D. Date normalization differs from Rust on the third branch

📁 packages/desktop-app/src/lib/services/node-normalize.ts:114-117 vs packages/nodespace-types/src/convert.rs:11-22

normalize_date_field has three branches:

1. YYYY-MM-DD passes through.
2. parse_from_rfc3339 gives the literal date in the value's own offset.
3. s.parse::<DateTime<Utc>>() converts to UTC before formatting.

OFFSET_DATETIME also accepts forms that likely only branch 3 parses: an offset without a colon (+0100), and possibly the space-separated form, depending on the chrono version's RFC 3339 separator handling. For those forms TS takes the literal prefix and Rust takes the UTC date. They disagree whenever the offset crosses midnight (2026-03-01T23:30:00-0500 gives 03-01 in TS and 03-02 in Rust). No write path produces these forms, so this is low risk. Either narrow the regex to T plus a colon offset (plus Z), or fix the doc's parity claim. The cleaner option, given CLAUDE.md's no-compat stance, is to delete Rust's branch 3 and the legacy dueDate-style read fallbacks, which are on-read format upgrades.

🟢 E. Pre-existing mirror race, worth a follow-up issue

📁 shared-node-store.svelte.ts:3238 (mode: 'immediate') with PersistenceCoordinator.cancelPending (:370-398)

This is the opposite direction of 🟡1, and it was not introduced here. A typed immediate write cancels a still-debounced generic content write for the same node, and the typed closure doesn't send content. Example: type in a task and click its checkbox within the 500 ms debounce. The fix is symmetric to this commit: the typed closure (or sendPendingTypedFields) should also re-send any content a cancelled generic write still owes. I'm noting it because this commit's "whoever runs next flushes" pattern is the natural place for it. Not required for this PR.

Nits

• Nit: sendPendingTypedFields() falls back to version ?? 1 when the node has left the store, and the generic closure calls it before its own !currentNode early return. Return early when this.nodes.get(nodeId) is missing, to avoid a guaranteed OCC round-trip.
• Nit: the TS drift test's arm regex (&\[([\s\S]*?)\]) depends on the literal-array formatting of promoted_fields. That's fine now, and it fails loudly if someone refactors to a const.

---

What's done well

• The flush is placed correctly with respect to the NOT_FOUND→create fallback, and the version is re-read after the flush. Both were the subtle parts.
• Extracting sendPendingTypedFields() removes the duplicated send-and-confirm block rather than copying it into two more closures.
• The drift guards chain correctly: TS is pinned to Rust, and Rust is pinned to the schemas.
• The commit message and the address-review comment state clearly what was skipped and why.

Recommendation: APPROVE

The commit fixes the headline 🟡1 correctly and closes every previously requested item. I strongly recommend fixing 🟡A before merge: it's a few lines, and it turns a new silent loss back into a correct write. 🟡B's callback-carrying change is the principled way to make the method doc's claim true. C, D and E can be follow-ups.

[malibio]

Automated pragmatic-code-review (re-review): APPROVE (posted as a comment — GitHub blocks self-approval). Full findings in the review comment above.

[malibio]

Re-review addressed (commit b382365)

✅ 🟡A: the persisted gate sent typed fields down a path that dropped them.

• The gate in updateNode routing is gone. updateTypedNode() now handles a node that hasn't been created yet: it stages the fields without scheduling a write of its own, because an immediate write would cancel the node's debounced create.
• The staged fields are sent right after each create path lands: the generic closure, the setNode closure and the batch closure.
• Test: stages typed fields on a node awaiting its create, and sends them once it lands asserts the order create → typed, and that the staging caller's onPersistSuccess fires.

✅ 🟡B: a typed-flush failure took down the generic write and notified the wrong caller.

• sendPendingTypedFields() now owns the typed write end to end: send, confirm, OCC hydration, play-rule and write-failure notifications, and handleTypedWriteFailure(). It never throws.
• Callbacks travel with the staged fields (PendingTypedWrite.callbacks), so the typed caller's onPersistError or onPersistSuccess fires whichever write ends up sending them.
• Test: a failed typed flush neither blocks the generic write nor reports to its caller checks three things: the generic RPC still goes out, the typed caller's onPersistError fires, and the generic caller's does not.

✅ 🟢C: the flush ran before the pending-move wait. The move wait now lives inside sendPendingTypedFields(), after the nothing-pending early return. An earlier draft waited first, which deadlocked a create against a move waiting on that create; the outdent-unpersisted suite caught it.

✅ 🟢D: date parity. Only the RFC 3339 branch of normalize_date_field is mirrored (seconds plus a Z/±hh:mm offset). Other forms pass through unchanged, and a doc comment explains why.

✅ 🟢E: an immediate write cancelled a still-debounced write (#3036). Fixed here rather than as a follow-up. When an immediate write arrives, the coordinator now starts the node's waiting debounced write instead of cancelling it, and the immediate write collapses behind it. A debounced write replacing another debounced write still cancels, since that's the intended keystroke collapse. Test: an immediate typed write does not discard a still-debounced content edit.

✅ Nit: sendPendingTypedFields() returns early when the node has left the store, instead of sending with a fallback version of 1.

Tests: frontend 5,303/5,303 pass; the pre-push gate passes.

Re-review needed: yes. The coordinator and the typed failure path changed.

[malibio]

Pragmatic Code Review (Re-Review 2): PR #3033, commit b382365

Review Type

Re-Review. Scope is only b3823653 (e476d695..b3823653): 4 files, +363/−213.

Previous Review Summary

	Count
Previous recommendations	7 (🟡A, 🟡B, 🟢C, 🟢D, 🟢E + 2 nits)
Addressed	6: A, B, C, D, E (fixed in this PR rather than deferred), and the missing-node nit
Not addressed	1: the drift-test regex nit. It was informational, so no action was needed
New issues	4, all 🟢 or non-blocking 🟡. None needs another fix-and-review round

The fixes are verified.

• 🟡A. The persisted gate is gone. An unpersisted node's typed fields are staged without a coordinator write (:3415) and flushed after each create path: generic :2067, setNode :2723, batch :4721. I traced both the generic and setNode create branches:

  • persistedNodeIds.add runs right after createNode resolves.
  • A typed write that arrives during the create RPC is staged, and the post-create flush picks it up.
  • A typed write that arrives during the follow-up getNode sees the node as persisted, queues behind the create, and becomes a no-op because the create's flush already sent it.
  • There is no gap in which a staged field is left behind.

• 🟡B. sendPendingTypedFields() catches everything and routes it to handleTypedWriteFailure(). Callbacks travel in PendingTypedWrite.callbacks and fire on the flush's own outcome, so the generic and batch closures can no longer fail because of the typed half. The new test checks both directions: the generic RPC still goes out, and only the typed caller's onPersistError fires.

• 🟢C. The move wait now sits inside the flush, after the nothing-staged early return. That ordering correctly avoids the create↔move deadlock in the common case (but see finding 1).

• 🟢D. I checked the regex against chrono 0.4.44 parse_rfc3339:

  • chrono requires seconds.
  • It accepts T, t or a space as the separator.
  • It accepts optional fractional seconds.
  • It accepts Z or z.
  • The offset colon is mandatory: timezone_offset(s, |s| scan::char(s, b':'), …).

  The new OFFSET_DATETIME matches all of these exactly (the /i covers t/z). The only remaining divergence is out-of-range values such as 2026-13-45T… or +99:99: chrono rejects them, while TS slices them. No writer produces those values, so this is negligible.

• 🟢E (An immediate write cancels a node's pending debounced content write #3036). The coordinator promotion is correct. See the analysis below.

---

Coordinator promotion analysis (focus item 1)

📁 packages/desktop-app/src/lib/services/shared-node-store.svelte.ts:215-231

• No recursion loop. waiting.operation() is executeOperation → runOperation, and the first statement of runOperation (executingOperations.add(nodeId)) runs synchronously, before any await. The recursive persist() therefore always takes the isExecuting branch and returns. Recursion depth is 1.
• The promoted write's promise settles. executeOperation was built with the original entry's resolve/reject, so runOperation's onDone/onError settle it. runOperation catches everything, so void does not produce an unhandled rejection.
• Bookkeeping stays consistent.
  • The recursive call overwrites pendingOperations[nodeId] with the queued placeholder (queuedPromise). hasPending() stays true throughout.
  • flushAndWaitForNodes() and waitForPersistence() await queuedPromise, which settles only after both writes. That is the right semantics for "fully persisted".
  • The promoted write's finally re-registers the queued write before clearing executingOperations, which is the existing gap-free handoff.
• cancelPending() and startBatch(). startBatch() cancels the debounced entry before any batch-immediate write exists, so there is nothing to promote there (unchanged). If a debounced write is pending when a batch commits, it is now promoted rather than dropped. That is one extra RPC and no loss.
• clearQueued(). An OCC in the promoted write clears the queued immediate write, which matches the existing OCC semantics for any queued write.
• Deletes are benign. deleteNode() removes the node from this.nodes and persistedNodeIds before persist(). So a promoted debounced generic write or setNode create hits its !currentNode early return (:2034, :2527) and makes no RPC. Nothing is created and deleted again, the version is not bumped, and the delete's captured node.version stays valid. Behavior is the same as before, apart from one no-op microtask.
• A debounced CREATE being promoted is fine.
  • Both create closures read getParentId() and structureTree at execution time, so an immediate structural updateNode gets the same parent it would have used before.
  • Typed writes on unpersisted nodes no longer call persist(), so they never promote a create early.
  • The indent re-trigger path (reTriggerPendingCreate) is still debounced-replaces-debounced and still cancels, as intended.
• Dependencies. A promoted write still awaits its own dependencies, and the queued immediate write keeps its own options. Neither can close a cycle through the other.

---

Code Review Findings

🟡 1. Move-wait circularity is now reachable from the create and typed paths (a 5 s stall, not a hang). Follow-up, non-blocking

📁 shared-node-store.svelte.ts:3121-3126 (move wait in the flush), reached from create paths :2067, :2723, :4721 and from the typed closure :3417; counterpart in reactive-node-service.svelte.ts:955-966.

indentNode falls through to the MOVE path when the node's create is executing (isOperationExecuting). The move then calls flushNodeSaves([nodeId]), which awaits that create's promise. If typed fields are staged at that point, the create closure's post-create flush finds a pending move and awaits it. The result is create → move → create, broken only by flushAndWaitForNodes' 5 s timeout. The steady-state typed path has the same shape: after rapid Tab-Tab, move #2 is still waiting on move #1 when a checkbox click lands.

The early return fixed the common (nothing-staged) case, which the outdent-unpersisted suite caught. The staged case still stalls. The generic closure (:1856) already had the same circular pattern before this PR, so this is an existing hazard class that this commit extends to two more call sites, not a new invention. It self-heals: after the timeout the move proceeds, and the flush re-reads the version after the wait.

Fix (follow-up): don't await a move from inside a coordinator operation that the move itself is flushing. For the create paths, the cheapest fix is to hand the staged fields to a fresh coordinator write instead of flushing inline:

PersistenceCoordinator.getInstance().persist(nodeId, () => this.sendPendingTypedFields(nodeId), { mode: 'immediate' });

That write queues behind the executing create. The move's flush has already captured the create's promise, not the queued one, so the move completes and the queued typed write then awaits it without a cycle. Open an issue that covers the generic :1856 instance too.

Principle: a lock-ordering or wait-for cycle. Never wait on a party that is waiting on you.

🟢 2. Promotion widens the existing single-slot supersede window for generic non-content writes

📁 shared-node-store.svelte.ts:215-231 with the queue branch :162-208 and capturedNonContentFields :1799

After a promotion, the immediate write sits in the queued slot for the whole promoted RPC, instead of executing at once. Any write that arrives during that RPC supersedes it, even a debounce-mode keystroke, because the isExecuting branch ignores mode.

• Typed fields survive this, because they are staged.
• A generic immediate write's capturedNonContentFields do not. Example: type text, press a shortcut that changes a custom: property, then keep typing within one RPC round-trip. The property change stays optimistic locally and never reaches the server.

This is the same loss that already happens whenever a generic write is queued behind an in-flight one; promotion only adds a new way into it. The window is tens of milliseconds, and pointer-driven property changes (Kanban) can't overlap with typing.

Fix (follow-up, same issue as 1 or its own): give generic non-content fields the same staging treatment pendingTypedFields got (merge on supersede, send from whichever closure runs), or merge the captured fields when persist() overwrites a queued entry.

Principle: latest-wins is only safe for idempotent full-state writes, and these are partial-field writes.

🟢 3. An OCC raised from a flush inside another closure can double-notify, and it drops staged callbacks unsettled

📁 shared-node-store.svelte.ts:3207-3250

• When decideRemoteUpdate skips hydration (node focused), the local version is not refreshed from current_node (:3243-3249). The generic closure that invoked the flush then sends its update with the stale version, hits a second OCC, and raises a second version-mismatch toast through the generic OCC handler. The generic write is also lost in that sub-case, so B's isolation holds for every error class except OCC-while-focused. This is rare: it needs a concurrent external write.
• this.pendingTypedFields.delete(nodeId) (:3213) discards any fields staged since the send, and their callbacks, without firing either callback. A Kanban caller's chainOrigin would leak, and nothing reverts its optimistic value if the node is not re-hydrated (focused branch). The old typed handler had the same delete; what's new is that the callbacks are now stored explicitly and dropped silently.

Fix (small, optional):

• Before deleting, fire onPersistError for the dropped entry's callbacks.
• Alternatively, keep the staged entry, since the next write re-reads the version after hydration.

For the double toast, a boolean return from sendPendingTypedFields() ('occ' | 'ok') would let the generic closure skip its own RPC after an OCC. Not required.

Principle: every promise or callback a caller registers should settle exactly once.

🟢 4. No coordinator-level test for promotion

📁 packages/desktop-app/src/tests/services/update-typed-node.test.ts (the only promotion test drives it through the store)

The store-level test (an immediate typed write does not discard a still-debounced content edit) pins the #3036 scenario and the typed-first ordering well. The coordinator is exported for direct testing, though, and the new branch has invariants worth pinning in isolation:

1. The promoted write's handle resolves and is not rejected with OperationCancelledError.
2. flushAndWaitForNodes([id]) issued during promotion waits for both writes.
3. Debounced replacing debounced still cancels, rejecting the first with OperationCancelledError.
4. An immediate delete after a debounced edit runs the edit closure first, then the delete.

These are cheap, and they guard a class the file has regressed on before (unsettled promises).

Nits

• Nit: PendingOperation.debounced?: boolean could be required (debounced: boolean), set explicitly on all four construction sites (immediate, the two placeholders, and debounced). With an optional flag, a future constructor that forgets it silently opts out of promotion.
• Nit: handle.promise.catch(() => {}) at :3427 is correct today because the closure never rejects. A short assertion-style comment already explains this. If dependencies are ever added to typed writes, a DependencyFailedError would be swallowed here, so consider narrowing it to OperationCancelledError.
• Nit: the fixture change in shared-node-store-extended.test.ts:250 (seeding from database) is justified: a viewer setNode with skipPersistence is now correctly "awaiting create". The comment explains why.

---

What's done well

• Failure ownership is now structurally right. One function owns send, confirm and failure for staged typed fields, and callbacks travel with the data. That makes "whoever runs next flushes" true for success and failure alike.
• The promotion fix is minimal and correct. It runs the waiting write and reuses the existing queue path, with no new state machine, and An immediate write cancels a node's pending debounced content write #3036 is closed in the same PR instead of being deferred.
• Nothing-staged returns early before any side effect, the move wait included. The comment explains the deadlock it prevents.
• The date regex now matches chrono's actual parser, and the doc states the deliberate non-mirroring of the UTC fallback.
• The tests assert RPC order and payloads, not just call counts.

Recommendation: APPROVE

Every item from the last round is addressed, and the coordinator change holds up under the settlement, bookkeeping, delete and create-promotion analysis above. No finding here warrants another fix-and-review round before merge.

• 🟡1 (a 5 s self-healing stall in a narrow race, extending an existing pattern) and 🟢2 (an existing supersede class, slightly widened) belong in a follow-up issue together.
• 🟢3 and 🟢4 are optional polish.

[malibio]

Automated pragmatic-code-review (re-review 2): APPROVE (posted as a comment — GitHub blocks self-approval). Full findings in the review comment above.

[malibio]

Re-review 2 addressed (commit ed2dba2)

✅ 🟢4: coordinator-level promotion tests. New persistence-coordinator-promotion.test.ts covers four cases:

• both writes resolve, and run in order;
• flushAndWaitForNodes waits for both;
• a debounced write replacing another debounced write still cancels;
• a pending edit runs before a following delete.

✅ Nits

• PendingOperation.debounced is now required and set at every construction site, so a new site can't silently opt out of promotion.
• The typed write's catch now swallows only OperationCancelledError; anything else is logged.

⏭️ Tracked in #3037:

• 🟡1: a move/create wait cycle. It recovers on its own via the 5 s flush timeout and shares its pattern with the generic closure.
• 🟢2: a queued generic write's non-content fields can be replaced, a gap that already existed and that promotion widens slightly.
• 🟢3: an OCC during a nested flush while the node is focused.

The reviewer marked all three as follow-ups. Each changes the write paths again, so each warrants its own review.

Tests: frontend 5,307/5,307 pass; the pre-push gate passes.

Re-review needed: no. This round adds tests, tightens a type, and adds logging for an unexpected error. No write-path logic changed.