Dispatch ADR-060 invariant rules from every bulk write path

[mstomar125]

Closes #2960

Problem

ADR-060 synchronous invariant dispatch ran on every single-node create/update path but on none of the bulk paths, and the reactive engine deliberately skips RuleClass::Invariant. So a node written through bulk_create, bulk_create_hierarchy (+ _in_tx, _root_notify), bulk_create_hierarchy_trusted or bulk_update — markdown import, the daemon's directory/bulk import, the sync/skill pipelines — never had its invariants evaluated.

Change

• Every bulk path now runs in one NodeService::with_transaction (store _in_tx inserts/updates, buffered events), and dispatches invariants per row inside that transaction after the batch lands. Any row's reject returns the same NodeServiceError::PlayRuleRejected single-node writes return and rolls back the whole batch (plus any invariant action's writes and the buffered events).
• Creation: dispatch_invariant_rules_in_tx split into invariant_rules_for_creation (lookup, filtered to invariant rules, same sync/lifecycle no-ops) + run_creation_invariant_rules_in_tx (execution). Bulk looks rules up once per distinct node type; rows of a type with no invariant rule cost nothing (no Node built). Subtype matching is lookup_rules' ADR-078 ancestry fan-out, unchanged.
• Update: bulk_update calls dispatch_invariant_rules_for_update_in_tx per node with its real changed_properties (keys are per-node, so no per-type cache), on a node reflecting what the store persisted (version/title/lifecycle applied). Store bulk_update_in_tx extracted; bulk_update delegates to it.
• Trusted path decision — invariants still run. "Trusted" skips schema-shape validation of parser output; it says nothing about user-authored product rules, and it serves the daemon's user-facing directory import. Skipping would make those rules permanently unevaluated. Justified in its doc comment.
• with_transaction inside begin_batch_emit: the sync replay batches pages through bulk_create/bulk_update under a batch guard. with_transaction now parks and restores the outer batch and, on commit, deposits its events into it (node-keyed last-write-wins, relationship events flushed), instead of clobbering it.
• bulk_create was documented atomic but wasn't (per-row autocommit); it now is. Collection-name collision journaling is preserved post-commit, including collisions within the batch.
• Dead code removed: store batch_create_nodes, store bulk_create_hierarchy_root_notify, and NodeService::bulk_create_hierarchy_root_notify (callers in markdown import and daemon switched to bulk_create_hierarchy — identical behaviour). Bulk hierarchy events now carry the service's client_id like every other NodeService write.

Tests

New packages/core/tests/bulk_invariant_dispatch_test.rs (10 tests, each mutation-checked by disabling the dispatch / sync no-op / batch composition): whole-batch reject for bulk_create, bulk_create_hierarchy, trusted path, bulk_update (property_changed), markdown import; satisfying batch commits and runs non-reject actions; base-type rule on a subtype row; update rule only matches changed properties; sync-tagged bulk_create skips dispatch; bulk writes inside a batch guard deliver events on drop.

• bun run test:changed (scripts + full Rust workspace nextest): pass (pre-rebase; rebase onto main was patch-identical)
• post-rebase: bulk_invariant_dispatch_test 10/10, playbook_invariant_integration_test 30/30, playbook_engine_integration_test 12/12, merge_nodes_test 23/23; cargo test -p nodespace-core --lib 1898/1898 (pre-rebase)
• bun run quality:fix clean

🤖 Generated with Claude Code

[mstomar125]

Pragmatic Code Review: PR #3210, dispatch ADR-060 invariant rules from every bulk write path

Review Type: Initial (high effort, adversarial; data-integrity write path). Reviewed at 6246781d. I read the code; I did not run a build or tests because the machine is overloaded. Everything below comes from reading the code, and I say where I'm unsure.

Requirements Check (vs #2960)

Requirement	Status
bulk_create dispatches creation invariants	✅ inside one tx, bulk.rs:205-233
bulk_create_hierarchy dispatches	✅ via insert_bulk_hierarchy_rows_in_tx
bulk_create_hierarchy_in_tx dispatches (schema description subtree)	✅ same helper, but untested
bulk_create_hierarchy_root_notify	✅ removed and folded into bulk_create_hierarchy. The two differed only in the store variant, and that variant was called with vec![] roots, so behavior is equivalent.
bulk_create_hierarchy_trusted dispatches	✅ (see decision 1 below)
bulk_update dispatches update invariants keyed on real changed_properties	✅ bulk.rs:912-935
Reject fails the whole batch and rolls back	✅ every path now runs inside a single with_transaction and the ? propagates
Sync-client no-op	✅ invariant_rules_for_creation / dispatch_invariant_rules_for_update_in_tx return early for SYNC_SERVICE_CLIENT_ID
ADR-078 subtype matching	✅ lookup_rules fans out to ancestors, and there is a test (..._base_type_rule_to_a_subtype_row)
Integration tests through bulk/import paths	✅ mostly (gaps listed below)

All six functions named in the issue are covered, and none of the bulk create/update paths skips dispatch any more. The core goal is met, and the refactor is a net simplification: two store methods gone, bulk_create is now actually atomic, and the per-row get_node notify loop is gone.

How I assessed the author's deliberate decisions

1. Trusted path still runs invariants. Agree. "Trusted" is about the parser's output shape, not user product rules. Directory import is a real user write, and skipping it would permanently bypass the rule because the reactive engine filters out invariants.
2. Dispatch runs after the whole batch is inserted. Acceptable, but the justification in the doc comment is wrong (see 🟡 Desktop shell with resizable panels #3).
3. Rules looked up once per type. Fine. The rule set could only change mid-batch through a concurrent play activation, which is negligible.
4. root_notify folded in. Verified equivalent (same preamble, same root embedding queue, vec![] stale-marker roots before).
5. Events stamped with the service client_id instead of None. Correct. It fixes the push-exclusion and local-origin gates for bulk hierarchy writes. Downstream is_local_node_create treats both None and a non-sync id as local, so nothing regresses.
6. Atomic bulk_create plus post-commit collision journaling. Semantics match the old per-row store.create_node: detect before the write, journal after commit, only active rows claim a name. But the new code is untested (see 🟡 Enhanced BaseNode with Hierarchical Circle Indicators + TextNode Implementation #4).
7. with_transaction may nest inside begin_batch_emit. Right direction. Before this PR, the sync per-row fallback (apply_page_per_row → create_node → with_transaction under the page guard) tripped the debug_assert in debug builds and clobbered the batch buffer in release. The implementation has a concurrency hole, though (🔴 below).

---

Code Review Findings

🔴 Critical

1. A batch guard dropped while another task's transaction has parked its buffer leaves an orphaned Batching state. Every later node event is then swallowed until some unrelated begin_batch_emit happens.
📁 packages/core/src/services/node_service/mod.rs:688-735 (park/restore), :528-540 (BatchEmitGuard::drop), :765-790 (post-commit merge)

batch_state is one process-wide slot shared by every clone, including the sync-tagged with_client(...) clone. The sync repo's reconnect.rs:321-334 says so explicitly: the page guard "also captures a concurrent FOREGROUND write". Here is a sequence that is possible on a multi-thread runtime, or on any runtime where the foreground tx yields on a SQL await:

1. Sync task: page_batch = ns.begin_batch_emit() → state Batching(a).
2. Foreground task: update_node → with_transaction gets the write guard and parks outer = Batching(a). State is now Transactional(b).
3. Sync task: apply_node_upserts_batched returns (its own writes are done) → drop(page_batch). BatchEmitGuard::drop does replace(state, Immediate) and gets Transactional(b). That isn't Batching, so the foreground tx's buffer b is silently discarded, and state becomes Immediate.
4. The foreground closure finishes: prev = replace(state, outer). prev is Immediate, so nothing is flushed, and state is now Batching(a) with no live guard.
5. From here on, every node-keyed emit_event and every committed with_transaction buffer goes into that orphaned map (mod.rs:770 merges into it). Nothing is broadcast. That means no UI live-updates, no event-driven push of local edits to cloud, and no reactive playbook triggers. This lasts until the next begin_batch_emit, which debug-asserts in debug builds and in release overwrites the orphan, losing page a's events and everything captured since. After the last page of a reconnect, the next guard could be hours away.

Before this PR the same interleaving also lost events. But it self-healed to Immediate, because with_transaction always reset to Immediate. The PR now legitimizes tx-under-batch and restores whatever it parked, which turns a transient loss into a persistent outage. Every step above is verified against the code. Whether it fires depends on a foreground tx overlapping a page-guard drop; that is plausible during a long reconnect replay while the user is editing, but I have not reproduced it.

Principle: shared mutable state needs an ownership protocol that holds under concurrency, not just under nesting on one call stack. "Restore what I saved" is only safe if nobody else could have changed ownership in between.

Suggested fix: keep the parked batch inside the slot so the guard's owner can still reach it. For example, BatchState::Transactional { buf, parked: Option<HashMap<..>> }:

• BatchEmitGuard::drop on Transactional { parked, .. } takes parked and flushes it, leaving buf alone.
• with_transaction restores Batching(parked) only if parked is still Some; otherwise it restores Immediate.
• The post-commit merge already falls back to "flush now" when the state isn't Batching.

Add a crate-internal regression test: open a guard, start a with_transaction whose closure awaits a Notify, drop the guard, release the closure, then assert the state is Immediate and both the batch's events and the tx's events were delivered.

---

🟡 Important

2. The mirror race (begin_batch_emit while another task's tx is active) is still unhandled, and the new doc overstates what is supported.
📁 mod.rs:2889-2900, doc at mod.rs:~654-660

The new with_transaction doc says a transaction "may run inside a caller's begin_batch_emit guard". It says the reverse is "unsupported", which reads as same-stack nesting only. Concurrently, sync's per-page begin_batch_emit can land while a foreground tx holds Transactional(b). In release, begin_batch_emit overwrites it with Batching, so the foreground tx's events are lost, and its restore then sets Immediate, so the sync page loses batching. This is pre-existing, not introduced here. It's the same root cause as #1, though, and the fix for #1 should cover it: begin_batch_emit seeing Transactional { parked: None } could set parked = Some(empty) instead of overwriting. Worth fixing together, and the doc should state the concurrency contract rather than the nesting contract.

3. The doc comment claims a guarantee the condition evaluator does not provide.
📁 packages/core/src/services/node_service/bulk.rs:252-254

"Dispatch runs after the whole batch is inserted, so a rule on a child row sees its parent's has_child edge already on tx"

Conditions are evaluated by execute_matched_invariant_rules_in_tx → GraphResolver::new(scoped) / CelScope::resolve(&scoped, ..). Both read through NodeService's pooled connections, not tx, so they cannot see this transaction's uncommitted rows or edges. That includes the parent itself when it is in the same batch, which it almost always is for an import. Only actions (execute_actions_in_tx) run on tx. A rule conditioned on parent.* evaluates as "absent" here, exactly as it does for create_node_with_parent, which the comment presents as the contrast case. Please correct the comment so nobody writes a rule, or a future change, that relies on it. The module doc in invariants.rs already states the right constraint for the single-node case.

Principle: comments that assert guarantees must be true. A wrong one is worse than none on a correctness-critical path.

4. The rewritten collection-collision journaling in bulk_create has no test.
📁 bulk.rs:171-196, :234-240

The old code got within-batch detection for free, because each store.create_node committed before the next row's lookup. The new code reimplements it by hand with a HashMap keyed on normalize_collection_name(content), while the stored-side lookup uses LOWER(title) = content.to_lowercase() with no trim. The logic looks right to me, but the two keys use different normalizations, and the exact case the comment calls out ("a sync page carrying two devices' same-named collections") is untested. No test in the repo calls bulk_create with collection nodes and asserts a CollectionNameCollision record. At minimum add: (a) a collision against a stored collection, (b) two same-named rows in one batch, (c) an inactive row not claiming the name.

5. Test gaps on the paths this PR changes.
📁 packages/core/tests/bulk_invariant_dispatch_test.rs

• bulk_create_hierarchy_in_tx (schema description subtree) now runs dispatch inside handle_create_schema's tx, so a user reject on text can now fail schema creation. That is behaviorally new and untested either way.
• A rollback under an outer batch guard is untested: a rejected bulk_create/bulk_update inside begin_batch_emit must discard only the tx's events and keep previously batched events. This is the ResetOnDrop/restore branch, and it's exactly what the sync fallback exercises.
• The sync-client no-op is tested for bulk_create only, not bulk_update.
• The markdown test uses sync_import: true. The default fire-and-forget branch (markdown/mod.rs:1132, whose call site this PR changed) is not exercised; there, a reject only surfaces as BackgroundImportFailed.

Principle: tests should cover every branch the change introduces, especially rollback and concurrency branches on a write path.

---

🟢 Suggestions

6. The write guard is held for per-row CEL evaluation on large imports. 📁 bulk.rs:299-303, :222-229. When a type has an invariant rule, a 10k-50k-row directory import (import_service.rs:665, all files in one call) now runs cel_scope_for plus a GraphResolver pool read plus a CEL eval per row while holding the single store writer. Foreground writes stall for that whole time. It's inherent to ADR-060's synchronous model, and there is zero cost when no rule matches, which is the common case. Also, one violating paragraph in one file now aborts the entire directory import, and the user only sees "Bulk node insertion failed; see daemon logs". Consider surfacing PlayRuleRejected's message in phase2_error.

7. The event burst is no longer paced. 📁 mod.rs:559 (flush_envelopes is synchronous). The old store.bulk_create_hierarchy notify loop awaited get_node between sends. Now N NodeCreated envelopes go out in one synchronous loop. For N > DOMAIN_EVENT_CHANNEL_CAPACITY (4096), slow receivers such as the push consumer will Lagged-drop. The trusted path already behaved this way (BatchEmitGuard flush), and single-doc markdown imports rarely exceed 4096 nodes, so the practical impact is low. Noting it for very large markdown files.

8. Stale snapshots across rows. 📁 bulk.rs:256-278. to_dispatch snapshots are built before any row's actions run. If row A's invariant action modifies row B, which is allowed because actions write through tx, B is dispatched with a pre-action snapshot. The single-node path can't hit this. It's an edge case; a one-line doc note would do.

9. Nit: a Vec<OrderedRuleRef> clone per row. 📁 bulk.rs:208, :300. rules_by_type[..].clone() allocates a String play_id per rule per row. run_creation_invariant_rules_in_tx could take &[OrderedRuleRef]. The cost is negligible next to the SQL.

10. Nit: SqliteStore::bulk_update and SqliteStore::bulk_create_hierarchy now have no production callers. Only store_concurrency_test.rs uses them, and pub hides the dead-code lint. Consider whether they should stay. At least bulk_create_hierarchy's still-present source: None notify loop is now a trap for any future caller (it bypasses invariant dispatch entirely).

11. Nit: downstream comments are now stale. nodespace-sync orchestrator.rs:780 and :991 still reference bulk_create_hierarchy_root_notify and "root-only" notification. That was already inaccurate before this PR; worth a follow-up in the sync repo.

Standards: no issue numbers in code comments, no lint suppressions, and the expect in with_transaction guards a real invariant. Comments explain why throughout (except #3).

Downstream sync contract: bulk_create/bulk_update under begin_batch_emit still coalesce into the page flush (tested). Fall back to per-row after an error still works, and is actually fixed: before, the per-row create_node under the guard tripped the debug_assert. Sync-tagged writes skip dispatch. bulk_create failing is now all-or-nothing, which suits the idempotent fallback better than the old partial prefix did. The only contract risk I found is the concurrency hole in 🔴 #1.

---

Recommendation: REQUEST CHANGES

The dispatch wiring itself is correct and complete, and the change is a real integrity improvement. The blocker is the BatchState park/restore race in #1. It can leave the process-wide event slot stuck in an ownerless Batching state that silently swallows UI, push and reactive-playbook events. It happens in the exact sync-reconnect-plus-foreground-edit scenario this nesting support was added for. Please fix #1 (ideally #2 alongside it) with a regression test, correct the #3 comment, and add the #4/#5 tests. The 🟢 items are optional.

[mstomar125]

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

[mstomar125]

Address-review summary (round 1) — commit ba9ee71

✅ Addressed

• 🔴 Set up Tauri + Svelte project structure #1 ownerless batch after a guard drops mid-transaction — BatchState::Transactional { buf, parked }: the open batch is parked inside the slot. BatchEmitGuard::drop takes and flushes parked when a transaction holds the slot; end_transaction restores Batching only if parked is still there. Regression test batch_guard_dropped_during_a_transaction_leaves_the_slot_immediate (a Notify-paused tx on another task); mutation-checked — reverting the drop change fails it.
• 🟡 Create design system foundation #2 begin_batch_emit during another task's tx — now parks the batch under the tx instead of overwriting its buffer; the tx's committed events deposit into it. Test batch_opened_during_a_transaction_receives_its_committed_events. with_transaction doc reworded around concurrency.
• 🟡 Desktop shell with resizable panels #3 wrong doc — corrected: actions (on tx) see the whole batch; conditions read pooled connections and don't see uncommitted rows, same as single-node dispatch.
• 🟡 Enhanced BaseNode with Hierarchical Circle Indicators + TextNode Implementation #4 collision journaling — within-batch matching now mirrors get_collection_by_name (lowercased content vs an earlier active row's lowercased title). Test covers stored collision (both sides journaled), in-batch collision (both sides), archived row freeing its name.
• 🟡 Basic storage integration #5 missing tests — added: schema description subtree via bulk_create_hierarchy_in_tx rejected → schema rolls back; rejected bulk_create under a batch guard emits nothing while a later write in the same guard still delivers; sync-tagged bulk_update skips dispatch.

⏭️ Skipped (with reasons)

• 🟡 Basic storage integration #5 async markdown import branch test — it spawns a detached task that reports failure via BackgroundImportFailed; the sync branch exercises the identical bulk_create_hierarchy call, and the failure-event plumbing already has its own test. Low marginal value.
• 🟢 Tauri desktop app launches successfully #6 surfacing the rejection message in the daemon's directory-import error — daemon UX, out of scope; the error is in daemon logs. Worth a follow-up if imports get rejected in practice.
• 🟢 Svelte frontend renders with placeholder panels #7 burst flush vs channel capacity — pre-existing batch semantics (a batch already flushes in one burst); not introduced here.
• 🟢 Development server works with hot reload #8 stale snapshots after an earlier row's action — invariant actions are depth-1 and target {trigger.node.id} in practice; a cross-row mutation is an edge the single-node path doesn't face either. Revisit if cross-node invariant actions become common.
• 🟢 Build process creates distributable package #9 slice instead of clone — OrderedRuleRef holds an Arc; the clone is only for types with rules. Negligible.
• 🟢 No compilation errors or warnings #10 store bulk_update/bulk_create_hierarchy test-only — still used by store-level concurrency tests; left as is.
• 🟢 Set up Tauri + Svelte project structure #11 sync repo comments naming bulk_create_hierarchy_root_notify — different repo; will note for the sync side.

🧪 Tests (targeted, one run at a time per machine-load guidance): bulk_invariant_dispatch_test 14/14; lib batch/BatchState tests 14/14; quality:fix clean.

Re-Review Decision

Decision: RE-REVIEW NEEDED
Rationale: the 🔴 fix changes with_transaction/BatchEmitGuard concurrency logic shared by every write path.

[mstomar125]

Pragmatic Code Review, Re-Review Round 1: PR #3210, commit ba9ee71b only

Review Type: Re-review (high effort, adversarial). Scope is git diff 6246781d ba9ee71b plus the address-review summary. I read the code only; I did not build or run tests because the machine is overloaded.

Verdict on the 🔴 #1 fix: fixed correctly

The new design moves the open batch into the slot as Transactional { buf, parked }. I checked it as a state machine over every operation that touches the slot:

Op	Immediate	Batching(m)	Transactional{buf, parked: None}	Transactional{buf, parked: Some(m)}
begin_batch_emit	→ Batching(∅)	debug_assert (2 guards)	→ parked = Some(∅)	debug_assert (2 guards)
guard drop	no-op	flush m → Immediate	no-op	parked.take(), flush m; buf untouched
tx start (write guard held)	→ T{∅, None}	→ T{∅, Some(m)}	unreachable (write guard serializes)	unreachable
tx end / unwind (end_transaction)	unreachable	unreachable	→ Immediate, return buf	→ Batching(m), return buf
emit_event / notifier	send	keyed → m; unkeyed → send	→ buf	→ buf
post-commit merge (lock released)	send	keyed → m	send	keyed → parked m

The fix relies on one invariant: a batch map exists in the slot (as Batching or parked) exactly when one guard is live. begin creates the map together with its guard. Guard drop removes whichever map is present, exactly once (Option::take / replace). Tx start and end move the map without dropping it. Nothing else touches it. The tx buffer is now only ever taken by end_transaction, and the ResetOnDrop flag stops it being taken twice. With that invariant, here are the two-task interleavings I walked:

• The original 🔴 sequence (G open → tx parks it → G drops → tx ends): G's drop flushes m, and the tx keeps buf. end_transaction sees parked: None and returns to Immediate. The post-commit merge then broadcasts. No orphan, and nothing is lost. ✅
• G drops, then G2 opens, both mid-tx (the reconnect loop's next page): G2's batch is parked. The tx ends by restoring Batching(m2), which G2 owns, and the tx's events join m2. They are delayed, not lost. ✅
• Tx ends and releases the lock, then G drops before the post-commit merge: the merge sees Immediate and broadcasts. ✅ If G2 opens in between, the events join m2. ✅ If another tx T2 starts in between, the events go into T2's parked. ✅
• Guard opened during a tx (old 🟡 Create design system foundation #2): the batch is parked instead of overwriting buf. The tx's committed events join it. ✅
• Rollback: buf is discarded, the parked batch is restored untouched, and there is no flush. ✅
• Panic in f / cancelled future: ResetOnDrop → end_transaction runs while the write guard is still held, restoring parked and discarding buf. end_transaction's own debug_assert!(false) can't fire during unwind, because only tx end ever replaces Transactional. ✅
• Double flush: not possible. parked goes through take() and buf is returned once. ✅
• Guard drop in Immediate: a clean no-op. It's only reachable after a second guard's drop already took the batch, which is the pre-existing multi-guard case. ✅

Can a debug_assert fire in legitimate use? Only the pre-existing "two live guards" assertion in begin_batch_emit, for example a user bulk_delete (which opens its own guard) running while the sync reconnect holds a page guard. That fired identically before this PR, and the PR neither introduces nor worsens it (see 🟢 #3). The Transactional-on-start and end_transaction asserts can't be reached.

Regression tests: batch_guard_dropped_during_a_transaction_leaves_the_slot_immediate fails against 6246781d: the old drop replaced Transactional with Immediate and flushed nothing, so "batched" is never received. That confirms the author's mutation claim. batch_opened_during_a_transaction_receives_its_committed_events also fails against 6246781d, via begin_batch_emit's old debug_assert!(Immediate). Both run on #[tokio::test]'s current-thread runtime; Notify stores a permit, so there's no lost-wakeup hang. They're sound.

Other items

• Desktop shell with resizable panels #3 (doc on conditions vs actions): corrected accurately at bulk.rs:255-259. ✅
• Enhanced BaseNode with Hierarchical Circle Indicators + TextNode Implementation #4 (collision matching): the within-batch key is now lowercased content against an earlier active row's lowercased title. That exactly mirrors get_collection_by_name (LOWER(title) = content.to_lowercase(), active only), so a batch detects what per-row creates would have. The new test covers a stored collision (both sides), an in-batch collision (both sides) and an archived row freeing its name. Each of its assertions would fail if the corresponding branch were broken, because the archived case only passes if lifecycle_status gates the claim. ✅
• Basic storage integration #5 (tests):
  • The schema-description test can't pass spuriously: it asserts Err and the schema's absence, and it fails on main, where there is no dispatch.
  • sync_tagged_bulk_update_... would fail if the no-op were removed, because the reject rule would fire.
  • rejected_bulk_create_inside_a_batch_guard_... is a guard against regression in the rollback-under-batch branch. It would also pass on 6246781d, which is fine because that branch was already correct; it just isn't a fail-without-fix test.
  • ✅ overall

Findings

🟡 Important

1. The with_transaction doc still describes the old model, and the summary's claim that it was reworded is inaccurate.
📁 packages/core/src/services/node_service/mod.rs:697-702 (and :686-690)

The summary says the doc was "reworded around concurrency", but the commit leaves this paragraph unchanged:

"A transaction may run inside a caller's begin_batch_emit guard (the reverse — a batch opened inside a transaction — is still unsupported). The batch's buffer is set aside while the transaction buffers its own events, then restored…"

That now contradicts the code. A batch opened during a transaction is supported and is exactly what batch_opened_during_a_transaction_receives_its_committed_events tests. The parked batch is also not necessarily restored. Similarly, :689 says the next caller sees batch_state "provably already been reset to Immediate", when it may now be Batching. BatchState's own doc at :484-489 is correct. Please make the with_transaction doc match it: parked in the slot, flushed by the guard if it drops first, restored only if still parked, and a batch opened mid-transaction gets parked. It's a comment-only fix, but this doc is the concurrency contract every write path relies on.

Principle: a contract doc that contradicts the implementation is worse than none, especially on shared concurrency state.

🟢 Suggestions (pre-existing, not introduced by this commit; follow-up candidates)

2. Another task's events can still land in the running transaction's buffer. 📁 mod.rs notifier Transactional { buf, .. } arm (~:1456), emit_event (~:3019); db/sqlite_store/nodes.rs:334-351. SqliteStore::create_node (and other non-tx store writes) release the write guard at the end of the execute statement, then call notify. Many NodeService methods likewise emit_event after a non-tx store write. If another task's with_transaction has taken the slot in between, that event is pushed into its buf and is discarded if that transaction rolls back. It also skips a parked batch. The misattribution is the same kind with_transaction's doc says was closed, but it comes from emitters outside the write guard. It existed on main (the Transactional arm is unchanged in substance), so it doesn't block this PR. The fix direction would be for emitters that don't hold the write guard to bypass buf: route to parked if present, otherwise send.

3. Two concurrent guards are still unsafe. A bulk_delete guard opened during a sync reconnect page guard hits begin_batch_emit's debug_assert in debug builds. In release it overwrites the first batch, and the first guard's drop then flushes the second's. Guards have no identity. This is pre-existing and unchanged. A guard token (generation id) would make drops precise. Follow-up.

4. Cross-task merge order under last-write-wins. If two transactions' post-commit merges run in the opposite order to their commits, which is possible because the merge runs after the lock is released, the batch keeps the older NodeUpdated snapshot for a node. Consumers that re-read by id are unaffected. Before this PR the same reordering existed in immediate broadcast order. Noting it only.

Skipped items

• Basic storage integration #5 async markdown branch: the reasoning holds. It's the same bulk_create_hierarchy call, and the failure event has its own test.
• Tauri desktop app launches successfully #6: out of scope, and the reasoning holds.
• Svelte frontend renders with placeholder panels #7: only partly holds. The trusted path already burst-flushed, but bulk_create_hierarchy (markdown/daemon per-file import) did lose its per-row pacing in this PR. Impact is low because single documents rarely exceed 4096 nodes. Accept.
• Development server works with hot reload #8: acceptable as a documented edge. The argument that "the single-node path doesn't face it" is really why it's a divergence, but invariant actions targeting other rows of the same batch are rare. Accept.
• Build process creates distributable package #9: the clone does allocate play_id: String per rule per row, but it's negligible next to the SQL. Accept.
• No compilation errors or warnings #10, Set up Tauri + Svelte project structure #11: accept.

Newly broken?

I found nothing. BatchEmitGuard::drop in Immediate is a no-op. The notifier's Transactional { buf, .. } arm keeps its previous behavior (see 🟢 #2 for the pre-existing gap). ResetOnDrop's bool flag is set immediately after the only non-unwind end_transaction, with nothing that can panic in between. The bulk.rs collision change only narrows who can claim a name (title required), which matches the stored-side lookup.

Recommendation: APPROVE

The 🔴 is genuinely fixed. I found no lost, double-flushed or orphaned batch in any interleaving of begin, drop, tx start/end, unwind, emit or merge across two tasks. The regression tests fail without the fix. Please fix the stale with_transaction doc (🟡 #1, comment-only) before merging; it doesn't need another review round. File 🟢 #2 and #3 as follow-ups. Both predate this PR, and #2 is a real event-loss path for writes that run concurrently with a rolled-back transaction.

[mstomar125]

Automated pragmatic-code-review (re-review of ba9ee71): APPROVE (posted as a comment — GitHub blocks self-approval). Fix the stale with_transaction doc (🟡 #1, comment-only) before merge. Full findings in the review comment above.

[mstomar125]

Address-review summary (round 2) — commit ac56096

Re-review verdict on ba9ee71: APPROVE (🔴 #1 park/restore race confirmed fixed across all two-task interleavings incl. unwind/rollback; both new regression tests fail on 6246781).

✅ Addressed

• 🟡 Stale with_transaction doc — now describes the actual model: an open batch (this task or another, open before or during the tx) is parked in the slot; a guard dropping mid-tx flushes it there and the tx ends with the slot Immediate, otherwise the tx restores it; committed events join a still-open batch or broadcast; rollback leaves the batch untouched. The "reset to Immediate" wording is corrected to "left Transactional". Comment-only change.

⏭️ Not addressed here (pre-existing, flagged as follow-ups by the reviewer)

• A non-tx write's post-guard notify/emit_event can land in another task's tx buffer and be dropped on its rollback.
• Batch guards have no identity; two concurrent guards remain unsafe.
• Post-commit merge order under last-write-wins can keep an older NodeUpdated snapshot (id re-readers unaffected).
  These predate this PR and belong in their own issue.

Re-Review Decision

Decision: NO RE-REVIEW NEEDED
Rationale: the only change since the APPROVE is doc-comment text; no logic changed.