[mstomar125]

Summary

bulk.rs's three write paths bypassed crud.rs's rebucket_and_validate sequence, which every other write path (all four of its own call sites) goes through: chain-aware validation via resolve_field_owners, then re-bucketing via bucket_properties_by_owner. Without it, an extending type's inherited field stayed in the node's own bucket instead of moving to its declaring ancestor's — e.g. an issue node (extending task) storing {"issue": {"status": ...}} instead of {"task": {"status": ...}} — which a base-scoped reader never looks in (per crud.rs's own comment on the read side of this gap).

• bulk_create / bulk_update: now call NodeService::rebucket_and_validate (widened from private to pub(crate) so bulk.rs can call it directly) instead of the non-bucketing validate_node_against_schema. bulk_update computes changed_properties from the post-rebucket state instead of a pre-rebucket snapshot, so the emitted diff matches what actually lands in storage (a no-op change for an unextended type, since bucket_properties_by_owner is a no-op reshuffle there).
• prepare_bulk_hierarchy_nodes (the shared preamble for bulk_create_hierarchy and bulk_create_hierarchy_in_tx): now resolves each unique type's full extends chain via resolve_field_owners and re-buckets before validating, replacing the own-schema-only get_schema_for_type fetch that never bucketed and validated with None for the chain (so an inherited field was reported as required-but-missing instead of being found in an ancestor's bucket). bulk_create_hierarchy_root_notify previously duplicated ~60 lines of that same (buggy) preamble inline instead of sharing prepare_bulk_hierarchy_nodes; it now delegates to the fixed function, so all three call sites (bulk_create_hierarchy, _in_tx, _root_notify) share one corrected implementation. bulk_create_hierarchy_trusted is intentionally unchanged — it deliberately skips all schema validation by design (trusted parser input), which is a separate, pre-existing posture this issue doesn't touch.
• Malformed schema, fails loudly: prepare_bulk_hierarchy_nodes's own-type schema fetch — a schema whose fields array is present but fails to deserialize — previously went through serde_json::from_value(fields_json).ok(), collapsing a parse failure to None, indistinguishable from "this type has no schema at all," so every row of that type was silently inserted unvalidated and un-bucketed. It now returns a real NodeServiceError::bulk_operation_failed naming the type and the parse error. (This check is scoped to the exact code this issue flagged — bulk_create/bulk_update and a malformed ancestor schema mid-chain still resolve through resolve_field_owners → get_schema_node, which has its own pre-existing "log a warning, treat as absent" posture shared by every ADR-078 write path, crud.rs's four rebucket_and_validate call sites included; widening that shared posture is a separate, larger change out of this issue's scope.)

Address-review fixes (second commit)

Review caught two real parity breaks in the first bulk_update pass, both fixed with new regression tests (mutation-verified — each test fails when its specific fix is reverted):

• rebucket_and_validate mutates updated.properties whenever the type has extends-chain fields, regardless of whether the caller's NodeUpdate included properties at all. The store write only sent the rebucketed value when update.properties was Some, so a content-only/lifecycle-only update could rebucket a stale wrong-bucket layout in memory, emit it in the NodeUpdated event, and never persist it. bulk_update now always re-persists the current properties value (matching crud.rs's update paths, which always do the same) and always diffs changed_properties from the pre-update baseline.
• rebucket_and_validate was called with apply_defaults hardcoded to false, so a bulk_update that changes node_type into an extends-chain type never defaulted the new type's missing fields before validating — unlike every crud.rs update path, which passes node_type_changed. bulk_update now tracks and passes it too.

Two further review findings were evaluated and not acted on:

• A claim that bulk_create's validation became stricter (full chain vs. own-type-only) doesn't hold: validate_node_against_schema (what bulk_create called before this PR) already resolved the full chain via resolve_field_owners — the issue itself says so. This PR only adds bucketing on top of already-chain-aware validation; validation strictness is unchanged.
• A performance observation that bulk_create/bulk_update resolve each row's chain individually (no per-unique-type cache, unlike prepare_bulk_hierarchy_nodes's new chain_cache) is accurate but describes pre-existing behavior (the prior validate_node_against_schema call already resolved per-row) — a genuine but separate performance improvement, not a correctness fix this issue asked for.

Out of scope (flagged, not fixed)

While writing a test against bulk_create_hierarchy with a schema created mid-test, I found SqliteStore::bulk_create_hierarchy's row-level validate_node_type check gates on a valid_node_types cache that's built once at SqliteStore::new time from the schema nodes already in the database, and isn't refreshed by handle_create_schema afterwards. In today's real usage this is masked because bulk_create_hierarchy's only live callers (markdown import) only ever create core types — but it means the same method can't insert a custom extends-chain type created after daemon startup, independent of this issue's bucketing bug. Filing as a separate follow-up rather than fixing here, since it's a different mechanism (a stale in-memory cache, not property bucketing) and touching it risked growing this fix beyond its scope.

Test plan

• New regression test: bulk_create re-buckets an inherited field into the ancestor's bucket (bulk_create_rebuckets_an_inherited_field_into_the_ancestors_bucket)
• New regression test: bulk_update re-buckets an inherited field, diffing changed_properties against the post-rebucket state (bulk_update_rebuckets_an_inherited_field_into_the_ancestors_bucket)
• New regression test: bulk_create_hierarchy re-buckets an inherited field for a bulk-imported row (bulk_create_hierarchy_rebuckets_an_inherited_field_into_the_ancestors_bucket)
• New regression test: a malformed schema fields array fails the bulk-hierarchy write loudly instead of silently skipping validation (bulk_create_hierarchy_fails_loudly_on_a_malformed_schema_fields_array)
• New regression test: a content-only bulk_update still self-heals a stale wrong-bucket layout, and doesn't drop the fix silently (bulk_update_self_heals_a_stale_wrong_bucket_even_on_a_content_only_change)
• New regression test: a node_type change via bulk_update defaults the new type's missing inherited field (bulk_update_applies_the_new_types_defaults_on_a_node_type_change)
• Verified all six new tests fail against the pre-fix code (reverted each specific fix in turn, confirmed the corresponding test fails for the expected reason, restored) before finalizing
• cargo test -p nodespace-core --lib — 1720 passed, 0 failed
• cargo clippy --all-targets -- -D warnings -D clippy::unused-async — clean
• bun run quality:fix — clean
• Full pre-push gate (test:all, cargo build --bin nodespaced, Tauri-seam integration tests) — green, on both commits

Closes #2881.

🤖 Generated with Claude Code

[mstomar125]

Second review round (post address-review fixes) found no correctness bugs — three efficiency/architecture observations, none acted on:

1. A malformed schema now aborts the whole bulk_create_hierarchy_root_notify batch, after markdown import already returned success (fire-and-forget). True, but not a new failure mode: prepare_bulk_hierarchy_nodes's per-row validation loop already ?-aborts the whole batch for any pre-existing validation failure (a required field missing, an enum mismatch) — this all-or-nothing transaction semantics predates this PR (bulk_create_hierarchy's own doc: "All inserts occur inside one transaction... rolled back atomically... Callers never observe a partial write"). The BackgroundImportFailed event this surfaces through is the existing, documented mechanism specifically for "the caller was already told this import succeeded... has no other way to learn it did not." My fix adds one more trigger to an existing, designed escape hatch — it doesn't change the batch's atomicity.

2. resolve_field_owners re-queries get_extends_parent_map() (a full extends-edge table scan) once per unique type in the batch, instead of hoisting it out. True. But resolve_type_chain's own doc says it's deliberately "resolved live rather than cached... the uncached resolver for write paths" — every other write path in the codebase (bulk_create/bulk_update, and all four of crud.rs's rebucket_and_validate call sites) calls this same uncached resolver once per row, not even per type. prepare_bulk_hierarchy_nodes's new per-unique-type caching is already a net improvement over that baseline. Hoisting the parent-map query out entirely would be a genuine, separate performance improvement, not a correctness fix — out of scope for this issue.

3. The new malformed-schema check's get_schema_for_type duplicates the fetch resolve_field_owners's internal get_schema_node performs moments later, for the same schema node. True — a bounded (per-unique-type, not per-row) second round trip, the direct cost of getting an explicit "fails loudly, names the type and the parse error" check without touching resolve_field_owners/get_schema_node's shared, documented "treat unparseable as absent" posture used everywhere else in ADR-078 write paths. Accepted as a reasonable, small, scoped cost.

All three are pre-existing patterns or inherent to already-accepted design elsewhere in the codebase, not regressions introduced by this PR. No further changes made; no re-review needed.