[malibio]

Summary

Closes #3032.

Two store paths used to report OCC conflicts inside anyhow message text, and the service parsed that text to build the error:

Store method	Old sentinel	Old service result
move_children_to_parent_in_tx	"VERSION_CONFLICT: node '<id>' …"	version_conflict(id, 0, 0)
delete_subtree_atomic	"version_conflict:<id>:<exp>:<act>"	parsed with unwrap_or fallbacks

Both now return Ok(Err(VersionConflict { node_id, expected, actual })), following the ADR-069 §2a convention that update_node_with_version_check_in_tx already uses. The service maps that value to NodeServiceError::VersionConflict by type, and both string parsers are deleted.

• New type: db::VersionConflict (exported from db). It implements Display and Error, so a caller can wrap it in an anyhow error when it has to fail a transaction closure. The store-level test helper does this so its transaction rolls back.
• Real versions: when the in-transaction gated DELETE removes no rows, move_children_to_parent_in_tx reads the child's persisted version inside the same transaction. It returns a conflict only if that version differs from the expected one. A child that is still at its expected version but has no has_child edge now fails with a plain error, instead of being reported as a conflict.
• Rollback is unchanged: a conflict can arrive after earlier children's edges were already swapped. The service still returns Err from its with_transaction closure, so those swaps roll back (there's a comment on this at the mapping site).

Unrelated fix included so the pre-push gate passes

main currently fails schema_chain_blindness_guard_test. #3030 moved rename_schema_field's own-fields read into validate_schema_field_rename, but the allowlist entry added by #2946 still names the old function. The second commit points the entry at the new function. The justification text is unchanged, and so is the read itself.

Tests

• Store: the stale-version test now downcasts to VersionConflict and checks the id, the expected version and the actual version. A new test checks that a root child at its correct version is not reported as a conflict.
• Service: the move_children_to_parent stale-version test, the delete_node conflict test and the concurrent-write race test all check the real expected_version/actual_version. In the race test, the losing move must report the writer's version + 1.
• The pre-push gate passed: test:all, the daemon build and e2e.

🤖 Generated with Claude Code

[malibio]

Pragmatic Code Review — PR #3035

Review Type: Initial Review

Summary

This is a clean, well-scoped change. Both store paths that encoded OCC conflicts in anyhow message text now return them as a typed value: move_children_to_parent_in_tx used the VERSION_CONFLICT: node '…' prefix, and delete_subtree_atomic used the version_conflict:<id>:<exp>:<act> sentinel. They now return Ok(Err(VersionConflict { node_id, expected, actual })), which follows the existing update_node_with_version_check_in_tx convention (ADR-069 §2a). Both service-side string parsers are gone, and the conflict now reports real versions instead of (0, 0). I ran the 13 targeted tests (cargo test -p nodespace-core --lib -- move_children_to_parent delete_node_version_conflict delete_subtree_atomic) and all of them pass.

Requirements Check

Acceptance criterion	Status	Notes
move_children_to_parent_in_tx reports a conflict as a typed value carrying the node id, the expected version and the actual version	✅	Result<Result<Vec<f64>, VersionConflict>>. actual is read inside the same tx after the gated DELETE removes nothing.
The service's strip_prefix("VERSION_CONFLICT: …") parsing is removed	✅	Removed in hierarchy.rs. The version_conflict: sentinel parser in crud.rs::delete_node is removed too.
NodeServiceError::VersionConflict from move_children_to_parent carries real versions, and tests assert them	✅	The store test, the service all-or-nothing test and the 50-iteration concurrent-write race test all assert node_id, expected and actual.
No other store path signals OCC conflicts through message text	✅	I grepped packages/ for version_conflict, VERSION_CONFLICT, version mismatch and strip_prefix/contains on error strings. The only remaining hits are the Tauri gRPC code label (Aborted → "VERSION_CONFLICT", which is a wire code, not parsing), the unrelated playbook schema-version validation, and tests. delete_subtree_atomic was the one other offender, and this PR converts it.

Commit-safety check (the key risk of Ok(Err) mid-loop)

When a later child conflicts, move_children_to_parent_in_tx returns Ok(Err(..)) after it has already swapped edges for the earlier children. That is safe only if every caller turns the conflict into Err inside its with_transaction closure. I checked every caller:

• hierarchy.rs::move_children_to_parent uses .map_err(query_failed)?.map_err(version_conflict)?, so the closure returns Err and the tx rolls back. tx.rs::with_transaction commits only on Ok.
• The sqlite_store/mod.rs test helper uses .await?.map_err(anyhow::Error::new), so it also returns Err. The added comment explains why.
• The node_service/mod.rs bump-conflict test calls .expect(..), which panics inside the closure. The dropped tx rolls back, and this is test-only.

delete_subtree_atomic owns its own tx and returns the conflict before any write. The dropped libsql tx rolls back, which is the same behavior the code had before when it returned Err.

No caller can commit a partial move. The doc comment on move_children_to_parent_in_tx states the obligation explicitly, which is the right place for it.

Code Review Findings

🟢 Suggestion: impl From<VersionConflict> for NodeServiceError
📁 packages/core/src/services/node_service/hierarchy.rs:764-766, packages/core/src/services/node_service/crud.rs:1939
The same closure, |c| NodeServiceError::version_conflict(c.node_id, c.expected, c.actual), now appears in two places. Any new typed-conflict store path would copy it again. A From impl in services/error.rs would reduce each site to .map_err(Into::into)? and keep the mapping in one place (DRY). This is optional, since two call sites is at the threshold.

🟢 Suggestion: a concurrently deleted child now surfaces as QueryFailed, not a conflict
📁 packages/core/src/db/sqlite_store/nodes.rs:2918-2944
The new affected == 0 branch correctly separates three cases: version moved (typed conflict), node gone ("Node not found while moving children") and no edge ("has no parent edge to replace"). This is more accurate than before, when all three were reported as VersionConflict(0, 0).

One behavior change: if a child's subtree is deleted after the service pre-validation but before the tx, the caller used to get Aborted, which fed the frontend's retry/rollback path. It now gets a generic QueryFailed. The service already maps a missing node to NodeServiceError::node_not_found in pre-validation, so the in-tx equivalent could reasonably be typed the same way. That would mean a second typed store outcome, and would not reintroduce string matching. The race window is narrow and the frontend presumably rolls back on any error, so this does not need to block the merge.

🟢 Nit: the root-child test asserts only a negative
📁 packages/core/src/db/sqlite_store/mod.rs:2259-2287
test_move_children_to_parent_store_root_child_is_not_a_conflict proves that the error is not a VersionConflict and that nothing moved. It would also pass if the branch failed for an unrelated reason, such as a SQL error. Asserting that err.to_string() mentions "no parent edge" pins the branch the test is named for. This is a test-precision point, not a correctness issue.

🟢 Nit: separate allowlist commit
📁 packages/core/tests/schema_chain_blindness_guard_test.rs
The allowlist retarget to validate_schema_field_rename is unrelated to #3032. I confirmed the function exists at schema.rs:820. It is isolated in its own commit with a clear message, and it was needed to get past a red guard on main, so bundling it here is pragmatic. A squash merge will fold it into the #3032 commit message, so mention it in the PR body if it isn't there already.

Standards

• No backward-compat shims, no #[allow(dead_code)], no deprecated paths, and both old parsers are deleted outright. ✅
• No issue numbers in code comments. Comments cite ADR-069 sections, which is the sanctioned form. ✅
• The comments explain why (rollback obligation, why the version is re-read in the same tx). ✅
• VersionConflict derives PartialEq/Eq and implements std::error::Error, so anyhow::Error::new and downcast_ref work in tests. The design is minimal and well-suited. ✅

Recommendation

APPROVE. All four acceptance criteria are met, the commit-safety invariant holds at every caller, and the tests assert real versions, including under a real multi-thread race. The remaining items are optional polish.

[malibio]

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

[malibio]

Review feedback addressed

This responds to the initial review, which recommended APPROVE and raised four optional suggestions.

✅ Addressed (2)

• From<VersionConflict> for NodeServiceError: added in services/error.rs. Both call sites (hierarchy.rs, crud.rs) now propagate the conflict with ?? instead of each repeating the mapping closure. The comment in hierarchy.rs that explains why the conflict has to become the closure's Err (so the transaction rolls back) now sits above the statement.
• Root-child test: it now also asserts the error text has no parent edge to replace, so an unrelated SQL error can no longer make it pass.

⏭️ Skipped (2)

• A concurrently deleted child now surfaces as QueryFailed, not a conflict. Before this PR, that case came back as a conflict with made-up 0/0 versions. Keeping it a conflict would mean inventing an actual version for a node that no longer exists. The batch still rolls back, and the window between pre-validation and the transaction is narrow. I'll revisit this if the frontend needs a typed not-found result for batch moves.
• Allowlist commit is unrelated to this issue: it's already its own commit, and the PR body calls it out.

🧪 Tests: 23 targeted core tests pass, clippy is clean, and the pre-push gate re-ran the full suite on push.

Re-review decision: NO RE-REVIEW NEEDED. These are optional-suggestion changes with no logic change: a From impl that replaces two identical closures, plus one extra test assertion.