[malibio]

Summary

• A generic update (single, in-transaction, bulk) that changes a schema node's fields is refused in ensure_schema_structure_unchanged, naming update_schema.
• A schema node created through the generic create with non-empty fields is refused. create_schema writes through a crate-private create_schema_node_in_tx that skips only that refusal.
• Test fixtures that built schemas with fields through the generic create now use handle_create_schema.
• New schema_fields_write_path_test covers each refusal, the unchanged-fields update, and create_schema / update_schema.
• Docs: schema-management.md states that fields has one write path.

Testing

• Workspace nextest passes, except 7 app-lib skill_setup tests that need packages/skill built (unrelated to this change).

🤖 Generated with Claude Code

[malibio]

Pragmatic Code Review: PR #3765 (issue #3753)

Review type: initial review (no prior reviews or comments).
Verdict: APPROVE (no blockers; two small improvements below, both optional).

Requirements check (issue #3753 acceptance criteria)

Criterion	Status
Generic update that changes fields refused on every path that calls ensure_schema_structure_unchanged (single, in-tx, batch), error names update_schema	Met. The check is added once, in the shared function, and all four call sites reach it: update_node_unchecked (crud.rs:1417), update_node_in_tx (:1683), prepare_update (:1952, behind update_node / update_with_version_check*), and bulk_update (bulk.rs:943).
Update that leaves fields unchanged still succeeds	Met (content/title_template patch, and the same list sent back).
Schema node cannot be created through generic create with fields	Met. ensure_schema_created_without_fields sits in ensure_creatable_in_batch, which is the single choke point for create_node, create_node_with_parent*, both bulk creates, hierarchy import (bulk.rs:154/597/731) and the playbook create action (insert_node_in_tx_no_invariant_dispatch -> insert_node_in_tx(.., false) -> ensure_creatable).
create_schema / update_schema still work	Met. create_schema uses the new crate-private create_schema_node_in_tx; update_schema, rename_schema_field, friendly-name and seed-update writes all go through update_node_unchecked_in_tx, which never calls ensure_schema_structure_unchanged (it only calls the core-status check, :1545).
Seeding / seed updates	Met. Core schema seeding goes through the store, not NodeService create; seed reconcile uses update_node_unchecked_in_tx (seed_updates.rs:405).
Schema unchanged after refusal	Met for update (fields and version asserted). Weak for create, see Improvement 1.
Each new test fails with refusal removed	Met for the two refusal tests by construction. The two other new tests are positive controls and are not meant to fail.
quality:fix / test:changed	I ran the whole nodespace-core nextest suite on the branch: 2981 passed, 5 skipped, 0 failed (includes the 4 new tests).

Docs side: nodespace-docs components/schema-management.md already states the one write path for fields (commit 1e69fd6).

Verification of the specific concerns

1. Update-path coverage / writer stays allowed: confirmed as above. update_node_unchecked (public) is refused, update_node_unchecked_in_tx (crate-private, the definition writer) is not. This matches how children / parent / context paths already behave, so no new bypass is created.
2. Create side: no other create path escapes. Every non-create_schema route funnels into ensure_creatable_in_batch; the only bypass is create_schema_node_in_tx, whose single caller is create_schema (schema/mod.rs:2366). The schema_definition: bool threading through create_node_in_tx_as / insert_node_in_tx is private and keeps the old public signatures unchanged. No production code (daemon, agent, cli, app-lib, frontend) creates a schema node with fields through generic create or sends fields in a generic update; remaining hits are tests, and they pass.
3. Retype: a retype of an existing node into schema while patching fields is also refused (existing has no fields, updated does). Good side effect, no action needed.
4. Strictness: the comparison is Value equality on the raw Option<&Value>, so absent vs [] counts as a change and is refused. That is the safe direction and matches the create-side rule ([] allowed on create, anything non-empty or non-array refused). Production schema rows are always written with a fields array by to_node / create_schema, so the update-side asymmetry cannot be hit by normal data. Key order does not matter for Value equality.
5. Fixture conversions: the playbook/participation/bulk tests moved from hand-built schema nodes to handle_create_schema. Meaning is preserved: same type ids (slug equals the old id), same fields; description and schemaVersion are now set through the real writer / a schemaVersion update (allowed, it is not fields). Dropping the stale "relationships": [] from the old fixtures is correct.

Findings

Critical Issues

None.

Suggested Improvements

• Improvement 1 - packages/core/tests/it/schema_fields_write_path_test.rs, a_schema_is_not_created_with_fields_through_the_generic_create: the "nothing was written" assertion is vacuous. Node::new assigns a UUID, so get_node("sidestep") is None whether or not the refusal exists. Assert instead on the effect, for example service.query_nodes(...)/filter by node_type == schema count before and after, or give the node a fixed id via Node::new_with_id and look that id up. Without it the "schema unchanged after each refusal" criterion is only checked on the update side.
• Improvement 2 - same file: create-side coverage is only the single create_node. The refusal is shared via ensure_creatable_in_batch, so correctness holds, but one bulk-create case (and the in-tx create) would pin the "every create path" claim in the doc comment, since a future refactor that splits the bulk path off would not be caught. update_node_in_tx (crud.rs:1683) likewise has no direct test; update_node, update_node_unchecked and bulk_update are covered.

Nitpicks

• Nit: crud.rs ensure_schema_created_without_fields error text interpolates node.id, which on a create is a freshly generated UUID the caller has never seen. Use node.content (the schema's name) so the message is actionable. It also returns invalid_update for a create; invalid_create/equivalent, if one exists, would read better.
• Nit: crud.rs doc comment on ensure_creatable mentions "hand-made core schema"; worth one clause saying it now also covers a schema carrying fields, so the list of what this function owes stays complete.

Recommendation

APPROVE. The change is minimal, reuses the existing ensure_schema_structure_unchanged pattern (ADR-089 precedent), keeps one clearly named crate-private bypass for the schema-definition writer, introduces no compatibility code or new undeclared keys, and the full core suite is green. Address Improvement 1 at your discretion before merge; it is a test-strength fix, not a behavior risk.

Known and unrelated: the 7 nodespace-app-lib skill_setup failures (packages/skill dist not built).

[malibio]

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

[malibio]

Review addressed (332575c)

Addressed:

• Vacuous "nothing written" assertion: the create test now uses a fixed id, so get_node("sidestep") really checks that no row landed.
• Create-side coverage: added a bulk_create case beside create_node.
• Refusal message names the schema by its name, not a generated id.
• ensure_creatable doc mentions the fields refusal.

Skipped:

• A direct update_node_in_tx test: crate-private, and it calls the same ensure_schema_structure_unchanged as the covered single, unchecked and bulk paths. Revisit if that call is ever split per path.

New tests pass; quality:fix clean.

Re-Review Decision

Decision: NO RE-REVIEW NEEDED

Rationale: The review was APPROVE with no critical or important findings. Changes are a test-strength fix, one added test case, an error-message string and a doc comment; no logic changed.