[malibio]

Closes #3447. Implements ADR-089: each type declares which children its nodes may have and where they may sit, and the database enforces both rules on every write path.

What changed

Declaration

• The registry (CoreNodeType) carries the rules. collection, schema and date are must_be_root; code-block, ordered-list, horizontal-line, table, query, tool and database-settings are children: none; everything else, ai-chat included, is any / any.
• SchemaNode gains children and parent ({"rule": "any_except", "types": [...]}, {"rule": "must_have_parent_of", "types": [...]}; any is the default and is not stored). Seeded core schemas declare the registry's rules. create_schema / update_schema accept them for user-defined types.
• A subtype only tightens. A declaration that would relax its base is refused when the schema is saved; so is a base change that would leave an extending type relaxing it, and a tightening that an existing node already breaks. A core type's rules can't be changed.

Enforcement

• New derived table structural_rule: core rows seeded from the registry, every other type's rows written by triggers in the statement that writes its schema.
• Three generic triggers (structure_has_child_insert, structure_has_child_update, structure_node_retype) replace the per-type *_is_root_* triggers. They join type_ancestry, so a rule on a base type binds its subtypes and a named type covers its own.
• The store asks the same SQL before it writes, so create-with-parent, move, bulk import, generic relationship create, merge and retype return a TreeInvariantViolation naming the rule and the nodes. Rule names on the wire are now must_be_root, children_none, child_not_allowed, parent_required (replacing collection_not_root / schema_not_root); NodeServiceError::NotAContainer is gone.
• A must_have_parent_of node is never created or left without a parent: creating it as a root (single and bulk), moving it to the root and removing its parent edge are refused before anything is written. No trigger can see a missing edge, so this check is Rust-only.

Replaced

• NodeBehavior::can_have_children() is removed. Embedding aggregation asks the children rule and stops at a chat.
• A child of a chat is its own embedding root, so a node kept under a chat stays searchable.
• The frontend canHaveChildren / canBeChild plugin flags and pluginRegistry.canHaveChildren() are removed. The rule types and each core type's rules are generated from Rust (bun run gen:types); core-node-types.ts composes them down the extends chain and resolves user-defined rules from the loaded schemas. Indent, outdent and type conversion ask canHaveChild / canBeRoot first.

Not in this PR

• ai-chat-message's rule is not declared. That type and ai-chat-native / ai-chat-acp are not in the registry yet (Split ai-chat into an abstract base with ai-chat-native and ai-chat-pty subtypes #3446, Store AI chat messages as ai-chat-message child nodes with real edges #3448). must_have_parent_of is implemented and tested through user-defined types; declaring it on the message type is a one-line registry entry in the PR that adds the type.
• Drag-and-drop: the outline has no node drag-and-drop to gate.

Behaviour changes worth knowing

• Markdown import: a line indented below a numbered item or a code block attaches to the nearest node above that takes children.
• A schema's children / parent change only through update_schema; a generic update that changes them is refused.
• A chat can now have children (it was a leaf for embedding and for the editor's indent).
• A date node under a parent is now refused as must_be_root rather than as a hierarchy violation.
• Retyping a node into a leaf type while it has children is now refused by the database; the editor declines the conversion instead of sending it.

Tests

• Registry and wire types: rule shapes, tighten-only, composition.
• DDL: seeded rules, schema-declared rules following the schema node, each of the four refusals on insert, re-pointed edge and retype.
• structural_rules_test.rs (15 end-to-end tests): declaration, subtype tightening, tightening against existing nodes, root-only types and their subtypes, leaf types on create / move / relationship, bulk import, generic relationship, retype, the needs-a-parent paths, chat children as embedding roots.
• Frontend: rule composition, indent / outdent / type-change following the rules.
• bun run test:changed passes (frontend, scripts, browser, Rust); bun run quality:fix clean.

Node type sequence

Kind and category of every type are unchanged; this adds structural rules only. Docs updated in nodespace-docs (components/node-types.md structure lines, components/schema-management.md declaration syntax and enforcement).

🤖 Generated with Claude Code

[malibio]

Pragmatic Code Review: PR #3475

Review Type: Initial Review (no prior reviews or comments on the PR)

Recommendation: REQUEST CHANGES. One blocker: markdown import now fails outright on documents it used to accept. Everything else is an improvement or a nit.

Summary

The design is right and matches ADR-089: one declaration per type, one derived structural_rule table, and generic triggers that share their SQL with the Rust pre-checks (has_child_checks), so the two cannot drift. I checked the trigger SQL by hand for subtype coverage, named-type coverage, retype in both directions, in-place edge re-point and a type with no ancestry rows, and found no defect. User-supplied type names are bound as parameters in node_breaking_rules, and the triggers read them through json_extract / json_each, so there is no injection path. No CLAUDE.md violations in the added lines (no literal node_type comparisons, no suppressions, no issue numbers, no compatibility code).

What I ran:

• structural_rules_test:: passes (13 tests; the PR body says 14).
• structural-rules-editing.test.ts and core-node-types.test.ts pass (26 tests).
• Three probes against this branch from a scratch crate outside the repo (nothing added to the worktree). They confirm findings 1, 2 and 3 below.
• I did not re-run bun run test:changed or quality:fix.

Requirements Check

Acceptance criterion	Status	Notes
Schemas declare children / parent, registry carries them, a subtype can't relax (tests)	Met	Enforcement is a union over the extends chain, so the database cannot relax a base rule. The validation itself can be bypassed (finding 3).
Every write path refuses a violating has_child edge (tests per path)	Met, with a regression	Create, move, bulk, generic relationship, merge and retype are covered. Seeding has no test of its own; it goes through the same triggers. The markdown importer now produces trees the bulk path refuses (finding 1).
must_have_parent_of created without a qualifying parent is refused before anything is written (test)	Met	Tested on single create, bulk_create, bulk_create_hierarchy, move to root and edge delete, through user-defined types.
collection, schema, date root-only, including subtypes; old triggers removed	Met	A DDL test asserts the *_is_root_* triggers are gone.
The leaf types refuse children (tests)	Met	The end-to-end test covers 5 of 7; tool and database-settings are covered only by the seeded-rows and registry tests.
A non-message child of a chat is embedded as its own root (test)	Met	Root resolution and "aggregation stops at a chat" are tested. No test shows the child actually getting a stale marker or embedding.
can_have_children() and the frontend flags are gone; indent / outdent / drag follow the rules	Met	No leftover references. The outline has no drag-and-drop (only tabs and kanban use drag).
Follows the node type sequence; node-types.md updated	Met, one acknowledged deviation	node-types.md and schema-management.md carry the rules. The ai-chat-message declaration (ADR-089 §4, issue item 3) is absent because the type is not in the registry yet.
bun run test:changed passes; quality:fix clean	Not re-verified	See "What I ran".

Findings

Critical

1. [Critical] Markdown import now fails on documents with content nested under an indented numbered list or code block
packages/core/src/markdown/mod.rs:665-700, 749-751 (unchanged by the PR; broken by assert_bulk_rows_allowed in packages/core/src/db/sqlite_store/nodes.rs:3570, 3643)

prepare_nodes_from_markdown pushes every non-heading node with indent_level > 0 onto indent_stack, leaf types included, and the next deeper-indented line takes it as its parent. Before this PR the bulk path had no container check, so those trees were stored. Now the whole import is refused, as an opaque Internal error. Confirmed by running it:

- Install
  1. Download deps
     - bun
     - cargo

gives Internal("Bulk creation failed: children_none: ordered-list '…' cannot have children; a ordered-list is a leaf").

The same happens for a continuation paragraph under an indented numbered item, and for a deeper line after an indented fenced code block. This reaches create_nodes_from_markdown (CLI, agent tool, directory import) and update_root_from_markdown.

Scope: the trigger is narrow. I ran prepare_nodes_from_markdown over the 203 non-archived files in nodespace-docs and none trips it. Agent-generated and user markdown with bullets, numbered steps and nested detail will.

Recommendation: have the importer ask the registry before a node becomes a parent candidate. Don't push a type whose structure().children is None onto indent_stack; attach the deeper line to the nearest ancestor that takes children. Add an import test with the snippet above. Principle: a rule made binding must not turn a previously valid input into a total failure on a path the PR did not adapt.

Important

2. [Important] Changing extends can leave existing nodes violating another type's rule
packages/core/src/schema/mod.rs:1194-1281, packages/core/src/db/sqlite_store/structure.rs:566-641

node_breaking_rules checks nodes of the changed type only against that type's own rules in force. It does not check rules on other types that name the new base. Confirmed by running it: with journal declaring any_except [task] and an entry node under a journal, update_schema { schema_id: "entry", extends: "task" } succeeds. The existing entry stays under the journal, and a new entry there is refused as child_not_allowed.

The mirror case is a type that stops extending a type named in a must_have_parent_of list while its nodes hold such children.

ADR-089 §4 says tightening needs a check that no existing node violates it; its Consequences section also accepts "refused on their next move". Recommendation: when extends changes, also check the type's nodes as children and as parents against the rules that name the new or old chain, or state in the doc comment that only the type's own rules are checked. Principle: the validation should match the invariant it claims.

3. [Important] A generic node update on a schema node installs structural rules with no validation
packages/core/src/db/schema.rs (structural_rule_schema_update trigger), packages/core/src/schema/mod.rs:1073 (validate_structural_rules is reached only from create_schema / update_schema)

Confirmed by running it: NodeService::update_node("folder", …) with properties.parent = {"rule":"must_be_root","bogus":1} is accepted while a folder node sits under a parent, and structural_rules_in_force("folder") then returns MustBeRoot.

The trigger copies whatever the properties say, so this path skips the named-type existence check, the empty-list check, the tighten-only check and the existing-node check. It cannot relax a base rule, because enforcement unions the chain. It can install a rule that names a nonexistent type, or a must_have_parent_of list wider than its base's. In the second case the SQL requires every list in the chain to be satisfied, while compose_rules and the frontend's structuralRules report only the nearest list, so the editor and the database disagree.

The probe also shows deny_unknown_fields does not reject the extra key on a unit variant of these internally tagged enums.

Recommendation: validate the two keys where every schema-node write passes (SchemaNodeBehavior::validate for shape, plus the tighten-only check), or refuse changes to children / parent outside update_schema. Principle: one validation point for one invariant.

4. [Important] The retype pre-check inside a transaction reads outside it
packages/core/src/services/node_service/crud.rs:64-90, called from the _in_tx updates at crud.rs:1272, 1397, 1669

ensure_retype_allowed calls assert_retype_keeps_structure, which uses Reader::Pool. A pooled reader does not see the transaction's own writes. The triggers back up every case except the one with no trigger: a node moved to the root earlier in the same transaction and then retyped to a must_have_parent_of type passes, because the pool still shows its old parent. The reverse (created under a qualifying parent, then retyped in the same transaction) is falsely refused.

Verified by reading, not executed. It is reachable from playbook actions (playbook/actions.rs:2244). Reader::Tx already exists, so the fix is an _in_tx twin like the other guards. The module's own comment says this rule has no backstop, so its one check should be transaction-consistent.

5. [Important] An empty test was left behind
packages/core/src/behaviors/mod.rs:5282

#[test]
fn tool_node_does_not_have_children() {}

It asserts nothing and reads as coverage. Delete it; the registry test already pins tool as children: none.

Suggestions

6. [Suggestion] canTakeType does not ask canBeRoot
packages/desktop-app/src/lib/services/reactive-node-service.svelte.ts:481-488
A root node offered a must_have_parent_of type passes the editor's check and is refused by the database. No core type declares that rule today, so there is no user impact yet. Add if (!parentId && !canBeRoot(nodeType)) return false; when the message type lands.

7. [Suggestion] End-to-end leaf coverage omits tool and database-settings
packages/core/tests/it/structural_rules_test.rs:520-526
Add them to the loop if they can be created through the service, so all seven declared leaves are proven on the write paths.

8. [Suggestion] Reader::rows reads every column as text
packages/core/src/db/sqlite_store/structure.rs:72-94
Correct for the current callers; every selected column is TEXT and non-null. A future caller selecting a nullable or numeric column gets a runtime error. A one-line note on the helper is enough.

Nitpicks

• Nit: packages/nodespace-types/src/schema.rs:551-553: the eprintln! message contains a literal run of spaces ("for schema node"), from a string continuation that lost its backslash.
• Nit: packages/core/src/behaviors/mod.rs:1365: the doc comment still says "Can have children: false (query nodes are leaf nodes)"; that now lives in the registry.
• Nit: the PR body says 14 end-to-end tests; the module has 13.

Outside this PR's diff

• core/src/models/schema_node.rs and nodespace-types/src/schema.rs both define SchemaNode. The duplication predates this PR, but the PR had to add the two fields and a parse helper to each. Worth collapsing when that area is next touched.
• combineNodes / promoteChildren (reactive-node-service.svelte.ts:1293-1361) can pick a leaf node as the new parent for promoted children without asking canHaveChild. The old can_have_children() move check already refused this, so it is not a regression. I did not trace what the failed move does to the children when the merged node is then deleted.

[malibio]

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

[malibio]

Address-review summary

Pushed in a8328713 (review fixes) on top of two merges of main.

Addressed

Finding	Change
1 (Critical) Markdown import refused content nested under an indented numbered item or code block	prepare_nodes_from_markdown no longer makes a type that takes no children a parent candidate; the deeper line attaches to the nearest node above that does. New test test_content_nested_under_a_leaf_attaches_to_the_nearest_container uses the review's snippet plus a continuation line and an indented code block.
3 A generic update on a schema node installed rules with no validation	ensure_schema_structure_unchanged refuses a generic update (single, versioned, in-transaction, bulk) that changes a schema's children / parent; update_schema is the one path. SchemaNodeBehavior::validate refuses a rule of an unknown shape or with an empty list on every write path, create included. Tests a_generic_update_cannot_change_a_schemas_rules, a_malformed_rule_is_refused.
4 Retype pre-check inside a transaction read outside it	ensure_retype_allowed takes the caller's transaction and uses the _in_tx twin. Test a_retype_in_a_transaction_sees_the_transactions_own_moves covers both directions (the false pass and the false refusal).
5 Empty test	Deleted.
6 canTakeType did not ask canBeRoot	Added, with a frontend test.
7 Leaf coverage omitted tool and database-settings	Both are now in the end-to-end leaf loop.
8 Reader::rows reads every column as text	Noted on the helper.
Nits	Log message spacing fixed; stale "Can have children" comment removed; PR body test count corrected.

Documented, not extended

2 Changing extends can leave existing nodes violating another type's rule. The check covers the rules the type itself declares or inherits. Searching existing edges for rules on other types that name the new or old base needs the post-change ancestry of every subtype, before the change is written. ADR-089's Consequences accept that existing trees breaking a rule are refused on their next move, so the doc comment on validate_type_system_changes now states exactly that boundary instead of implying more.

Skipped

• Duplicate SchemaNode definitions in core and nodespace-types: predates this PR and is its own consolidation.
• combineNodes / promoteChildren choosing a leaf as the new parent: the old move check refused this too, so it is not a regression of this change.

Also changed since the review

main gained generated TypeScript. The hand-written registry mirror and its Rust-source reader are gone: SchemaChildrenRule, SchemaParentRule, StructuralRules and each core type's rules are generated from Rust, and core-node-types.ts keeps only the helper functions.

Verification

bun run test:changed passes on the review-fix commit (frontend, scripts, browser, generated-TypeScript drift, Rust). After merging the latest main: core and types suites 2614/2614, types:check in sync.

Re-Review Decision

Decision: RE-REVIEW NEEDED

Rationale: a critical finding was fixed with a logic change in the importer, two new guards were added on the update paths, and the frontend registry was reworked onto the generator after a merge.

[malibio]

Pragmatic Code Review: PR #3475

Review Type: Re-Review (reviewed head 188ff0c5, now a8328713)

Recommendation: REQUEST CHANGES. Every finding from the first review is addressed. Two new Important findings remain, both confirmed by running them: the importer fix now stores nested list content out of reading order, and update_schema reports success when asked to loosen a rule to any but changes nothing. Both are small fixes.

Previous Review Summary: 11 recommendations (1 Critical, 4 Important, 3 Suggestions, 3 nits). 10 addressed in code, 1 (finding 2) resolved by documenting the boundary, which I accept. 0 not addressed. 2 new Important findings, 1 new Suggestion.

What I ran

• structural_rules_test::: 15 of 15 pass.
• test_content_nested_under_a_leaf_attaches_to_the_nearest_container and a_retype_in_a_transaction_sees_the_transactions_own_moves: pass. The markdown:: and behaviors:: unit tests: 254 pass.
• structural-rules-editing.test.ts and core-node-types.test.ts: 23 pass.
• bun run types:check: generated TypeScript is in sync (53 files).
• Probes from a scratch crate outside the repo (nothing added to the worktree). They confirm new findings 1, 2 and 3.
• I did not re-run bun run test:changed or quality:fix.

Previous findings

#	Finding	Status
1 (Critical)	Markdown import refused content nested under an indented numbered item or code block	Fixed: the import no longer fails. The chosen placement breaks reading order (new finding 1).
2	Changing extends can leave existing nodes violating another type's rule	Accepted as documented. The doc comment on validate_type_system_changes now says exactly what is checked, and ADR-089's Consequences accept "refused on their next move". The comment and the code agree.
3	A generic update on a schema node installed rules with no validation	Fixed. ensure_schema_structure_unchanged is on update_node_unchecked, update_node_in_tx, the versioned update and the bulk update. update_node_unchecked_in_tx, which only the schema writers call, does not carry it, so update_schema, rename_schema_field and the friendly-name update are not blocked by it (probe: an add_fields call on a schema with rules succeeds; the other two I checked by reading). An update that sends the stored properties back unchanged passes. A malformed rule is refused on create too.
4	The retype pre-check inside a transaction read outside it	Fixed. All three _in_tx updates pass their transaction, and the new test covers both the false pass and the false refusal.
5	Empty test	Deleted.
6	canTakeType did not ask canBeRoot	Fixed, with a test.
7	Leaf coverage omitted tool and database-settings	Fixed: both are in the loop.
8	Reader::rows reads every column as text	Noted on the helper.
Nits	Log spacing, stale comment, test count	Fixed.

The strict rule-shape check in SchemaNodeBehavior::validate does not reject a seeded core schema or anything create_schema / update_schema writes: both write the serialized SchemaChildrenRule / SchemaParentRule, which the check deserializes back.

The generated TypeScript agrees with Rust and SQL. CORE_NODE_TYPES carries the registry's declared rules, and structuralRules composes them as compose_rules does: none wins, any_except lists add up, and the nearest parent declaration is in force. That last one matches the SQL (which requires every list in the chain) because a subtype's list must sit inside its base's, and the generic-update guard closes the path that could store a wider one.

Requirements Check

Acceptance criterion	Status
Schemas declare children / parent, registry carries them, a subtype can't relax	Met. A rule cannot be loosened back to any (new finding 2).
Every write path refuses a violating has_child edge	Met.
must_have_parent_of created without a qualifying parent is refused before anything is written	Met, now also inside a transaction.
collection, schema, date root-only, subtypes included; old triggers removed	Met.
The leaf types refuse children	Met: all seven are tested end to end.
A non-message child of a chat is its own embedding root	Met.
can_have_children() and the frontend flags are gone; the UI follows the rules	Met. The rule types and registry are generated, not hand-copied.
Node type sequence followed; node-types.md updated	Met, with the acknowledged ai-chat-message deferral.
bun run test:changed passes; quality:fix clean	Not re-verified by me; the author reports it passing.

Code Review Findings

Important

1. [Important] The importer fix stores nested content before the list it belongs to
packages/core/src/markdown/mod.rs:749-756, pinned by packages/core/src/markdown/markdown_test.rs:1291-1359

A line indented under a leaf now attaches to the nearest entry on indent_stack. An ordered list never takes its parent from that stack (mod.rs:686-687 uses the last paragraph or the heading), so the list and the lines under it land under different parents. Imported with title # Setup:

- Install
  1. Download deps
     - bun
     - cargo
  2. Build
     continuation line
- Run

is stored as:

# Setup
  Install
    bun
    cargo
    continuation line
  1. Download deps
  1. Build
  Run

bun and cargo now come before the step they detail, and continuation line before 2. Build. On main this document imported with those lines under their numbered items, so reading order held. The first review's blocker (the import failing) is gone, but this shape, common in agent-written markdown, now imports silently reordered. The new test asserts the reordered result (bun under Install) and never checks where the numbered items are.

Recommendation: attach a line indented under a leaf to the leaf's own parent, so it follows the leaf as a sibling. For example, push the leaf's parent_id (not its own id) onto indent_stack at the leaf's indent. Assert the stored outline order in the test, not just each node's parent. Principle: an importer may flatten structure a rule forbids, but it should not reorder the document.

Documents with a top-level numbered list and indented detail are unaffected: that list was never on the stack.

2. [Important] update_schema cannot loosen a rule to any, and reports success
packages/core/src/schema/mod.rs:1055-1066 (store_structural_rules), 3423-3437, 3619-3625; merge in packages/core/src/services/node_service/crud.rs:2565-2589

store_structural_rules leaves an any rule out of the properties, and update_node_unchecked_in_tx deep-merges them into the stored ones, so a key that is left out is never removed. Confirmed: a Folder schema created with children: none, parent: must_be_root, then update_schema { children: {"rule":"any"}, parent: {"rule":"any"} } returns {"success": true}. The stored properties and structural_rules_in_force are unchanged, and a child under a folder is still refused as children_none.

This predates the review fixes, and the first review missed it. It matters more now: the new guard makes update_schema the only path that may change a rule, so a user-defined type's rule can no longer be removed at all. ADR-089 §4 says loosening is always safe for existing data and expects it to work.

The same merge leaves stale keys when a rule changes variant: any_except [task] to none stores {"rule":"none","types":["task"]}. That is harmless today, because every reader keys on rule.

Recommendation: make the schema-definition write authoritative for the keys it owns. Remove children / parent from the merged properties when the new definition declares any, and replace the rule object instead of merging into it. Add a test that loosens each rule to any and then performs the write it had refused. The abstract flag has the same defect through the same code (confirmed: update_schema { abstract: false } returns success and the type stays abstract). It is outside this PR's diff, but the same fix covers it. Principle: an operation that reports success must have made the change.

Suggestion

3. [Suggestion] A generic create of a schema node is checked for shape only
packages/core/src/behaviors/mod.rs:1294-1322

Confirmed: create_node with node_type: "schema" and children: {"rule":"any_except","types":["no-such-type"]} is stored and enforced, and parent: {"rule":"must_be_root","types":["x"],"bogus":1} is accepted, because serde ignores extra keys on a unit variant of an internally tagged enum. The update paths are now closed; the create path still skips the named-type check that create_schema does. Impact is low: a rule naming a missing type matches nothing, and a new schema has no base to relax. If generic creation of schema nodes is meant to stay open, a one-line note on validate_structural_rule saying it checks shape only is enough.

Nitpicks

• Nit: packages/core/src/services/node_service/crud.rs:118-135: a generic update that sends {"rule":"any"} for a rule the schema does not store is refused as a change, because the comparison is on raw JSON (None against Some). The refusal is safe; the message says the rule "can only be changed" when nothing would change.
• Nit: packages/core/src/services/node_service/bulk.rs:928: the bulk update passes None to ensure_retype_allowed, so a batch that retypes a parent and its child together is checked against committed types. The triggers still refuse every wrong-typed pair, and a batch cannot move a node, so the unbacked "left without a parent" case cannot arise here. No change needed.

[malibio]

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

[malibio]

Address-review summary (re-review)

Pushed in c4b85975, plus a merge of the latest main (506df5e7).

Addressed

Finding	Change
1 (Important) The importer fix stored nested content before the list it belongs to	A line indented below a type that takes no children now goes under that node's own parent, so it follows it as a sibling. The review's document is stored as Install, 1. Download deps, bun, cargo, 1. Build, continuation line, Run. The test (test_content_nested_under_a_leaf_follows_it_in_reading_order) asserts the whole stored outline, depth-first, and that no leaf has children.
2 (Important) update_schema could not loosen a rule to any, and reported success	The schema-definition write owns abstract, children and parent: a value replaces the stored one whole, and one the definition no longer carries is removed from the row. Tests a_rule_can_be_loosened_back_to_any (kind change keeps no stale types; both rules back to any; the refused writes then go through; an update naming neither rule leaves them) and an_abstract_type_can_be_made_concrete_again (the same defect on abstract, fixed by the same code).
3 (Suggestion) Generic create of a schema node is checked for shape only	Documented on validate_structural_rule, with why that is sufficient: a rule naming a missing type matches nothing, and enforcement adds a subtype's rule to its base's.

Not changed

• Nit, ensure_schema_structure_unchanged refuses a generic update that sends {"rule":"any"} for a rule the schema does not store: the refusal is safe and points at the right operation.
• Nit, bulk update passes no transaction to the retype check: as the review notes, a batch cannot move a node, and the triggers cover every pair.

The test fixture for finding 1 no longer includes an indented code block: where an indented code block is parented (under the last paragraph) is existing importer behaviour this PR does not touch.

Verification

bun run test:changed passes on c4b85975 (frontend, scripts, browser, generated-TypeScript drift, Rust). After merging the latest main: core and types suites 2619/2619, types:check in sync.

Re-Review Decision

Decision: RE-REVIEW NEEDED

Rationale: both fixes change logic (importer parenting, and how a schema definition is written to the row), so they get one more narrow look.

[malibio]

Pragmatic Code Review: PR #3475

Review Type: Re-Review 2 (reviewed head a8328713, now 506df5e7; the change under review is c4b85975)

Recommendation: REQUEST CHANGES, for one small item. The update_schema fix is correct and complete. The importer fix is correct for every caller that passes a root id, but the daemon's single-file import passes none, and there the reordering the last review reported is still present. The fix is one line.

Previous Review Summary: 5 recommendations (2 Important, 1 Suggestion, 2 nits). Important 2 (update_schema cannot loosen a rule) is addressed in full. Important 1 (importer reorders nested content) is addressed for the callers that pass a root id and not for the one that passes none (finding 1 below). The Suggestion is addressed by the doc comment. The two nits were left as they are, which I accept. No other new Important findings; one Suggestion and three nits.

What I ran

• structural_rules_test::: 17 of 17 pass, including a_rule_can_be_loosened_back_to_any and an_abstract_type_can_be_made_concrete_again.
• --lib markdown::: 134 pass, including test_content_nested_under_a_leaf_follows_it_in_reading_order.
• A probe crate outside the repo (nothing added to the worktree, its target/ removed): eleven markdown shapes through prepare_nodes_from_markdown with and without a root id, and a sequence of schema writes against a real store.
• I did not re-run bun run test:changed or quality:fix, and I did not run the daemon's import end to end (see finding 1 for how it was confirmed).

Previous findings

#	Finding	Status
1 (Important)	The importer stored nested content before the list it belongs to	Fixed where the parser is given a root id. Still present in the daemon's single-file import (finding 1).
2 (Important)	update_schema could not loosen a rule to any, and reported success	Fixed. Verified below.
3 (Suggestion)	Generic create of a schema node is checked for shape only	Documented on validate_structural_rule. Accepted.
Nits	Refusal message for an explicit any; bulk retype check has no transaction	Unchanged, with reasons. Accepted.

Importer: what holds

• The whole PR's change to prepare_nodes_from_markdown against main is the 12 lines at packages/core/src/markdown/mod.rs:704 and 750-762. A leaf's stack entry is only ever read by a line indented deeper than the leaf, which on main was that leaf's child. A document with no such line imports exactly as it does on main.
• The stack stays strictly increasing in indent: every push follows a pop of all entries at the same or deeper indent. The same id can sit on it twice (a paragraph at indent 2, then a code block under it at indent 4, gives (para, 2), (para, 4)), and that is harmless: each entry pops on its own indent, and both name the same parent.
• A later line at the leaf's indent or shallower pops the ancestor entry before it picks a parent, so it cannot be attached to it by mistake. Confirmed with a second heading after the list (## B then an indented bullet): nothing leaks from the section above.
• With a root id, the review's document and the table, code-block, numbered-bullet (- 1. item) and intro-paragraph shapes all store in document order, and no leaf has a child.

One thing I checked and am not raising: a bullet that follows a nested numbered list at the list's own indent (- top / - mid / 1. step / - sibling of step) is stored ahead of the list. That is the same on main: a numbered list never takes its parent from the indent stack, so it is hoisted beside the bullet it sits under. This commit does not change it.

Schema definition write: what holds

Probe on a user schema created abstract, with children: any_except [task] and parent: must_be_root:

Write	Stored afterwards	Version
rename_schema_field	all three keys kept, rules in force unchanged	1 to 2
update_schema_field_friendly_name	all three kept	2 to 3
update_schema { add_fields }	all three kept	3 to 4
update_schema { children: any }	children gone, abstract and parent kept; in force (Any, MustBeRoot)	4 to 5
update_schema { abstract: false, parent: any }	none of the three stored; in force (Any, Any); not abstract	5 to 6
update_schema { children: none, parent: must_be_root }	both back, {"rule":"none"} with no stale types	6 to 7

• The two other callers of update_node_unchecked_in_tx (packages/core/src/services/node_service/schema.rs:695-714 and 953-972) build their properties without any of the three keys, so merge_schema_definition leaves the stored values alone. Nothing else calls it.
• The structural_rule rows are right after the two-statement write. structural_rule_schema_update fires on every UPDATE OF properties, deletes the type's rows and rebuilds them from new.properties, so the json_remove statement leaves them built from the final row. schema_core_status_fixed compares isCore only and is not tripped.
• One version bump per call. The json_remove statement does not touch version, and the event's node is the merged value with the cleared keys removed and version + 1, which matches the row.
• No null reaches storage through update_schema: the three keys are taken out of the written object before the merge, and the stored JSON in every row above has none. Core schemas are unaffected: collection keeps its stored parent, and task and ai-chat store none of the keys before or after an add_fields.
• A generic update can still store "abstract": null on a schema node (confirmed: update_node with {"abstract": null}). That path is not touched by this commit, it reads as "not abstract", and the next update_schema removes it. No action needed here.

Requirements Check

Acceptance criterion	Status
Schemas declare children / parent, registry carries them, a subtype can't relax	Met. A rule can now be loosened and tightened again.
Every write path refuses a violating has_child edge	Met.
must_have_parent_of created without a qualifying parent is refused before anything is written	Met.
collection, schema, date root-only, subtypes included; old triggers removed	Met.
The leaf types refuse children	Met.
A non-message child of a chat is its own embedding root	Met.
can_have_children() and the frontend flags are gone; the UI follows the rules	Met (not re-examined; unchanged since the last review).
Node type sequence followed; node-types.md updated	Met, with the acknowledged ai-chat-message deferral.
bun run test:changed passes; quality:fix clean	Not re-verified by me; the author reports it passing on c4b85975.

Code Review Findings

Important

1. [Important] A single-file import still stores nested content ahead of the list it belongs to
packages/core/src/markdown/mod.rs:756-761; caller packages/daemon/src/services/import_service.rs:1434-1436 and 1520

The new branch pushes the leaf's parent only when there is one (else if let Some(leaf_parent) = leaf_parent_id). Every other caller that imports a document body passes a root id, so a top-level leaf's parent is that root. import_markdown_content (the daemon's single-file import) calls prepare_nodes_from_markdown(&content_for_children, None) and maps a missing parent to the root afterwards. There, a leaf with no heading or paragraph above it has no parent, pushes nothing, and the lines indented under it fall through to the nearest bullet on the stack, which is the previous commit's behaviour.

Confirmed at the parser, with root_id: None, for the review's document:

Install
  bun
  cargo
  continuation line
1. Download deps
1. Build
Run
  quickly

The same input with a root id gives the order the new test asserts. A table and a code block under a bullet behave the same way (under table is stored under top, ahead of the table). The trigger is a file whose body starts with a list before any ## heading or paragraph, since the title line is stripped. I confirmed the parser output and read the caller's mapping (n.parent_id.clone().or_else(|| Some(root_id)), order preserved); I did not run the daemon import itself.

Recommendation: pass the root id at that call, as every other caller does: prepare_nodes_from_markdown(&content_for_children, Some(root_id.to_string())). The or_else at line 1520 then has nothing to do and can go. Note one side effect to accept or check: with a root id, a horizontal rule is placed under the document root rather than under the first heading, which is what the other import paths already do. Add the None case, or a daemon test for this document, so the two entry points cannot drift again. Principle: one parser should give one result for the same document, whichever entry point reaches it.

Suggestion

2. [Suggestion] update_schema now runs three extra statements on every call
packages/core/src/services/node_service/crud.rs:2591-2598, packages/core/src/db/sqlite_store/nodes.rs:1739-1747, packages/core/src/schema/mod.rs:3622-3627

update_schema sends null for each optional key the definition does not carry, and merge_schema_definition records a key as cleared whether or not it was stored. A schema with no rules and no abstract flag (the usual case) therefore runs three UPDATE node SET properties = json_remove(...) statements that change nothing, and each one fires the rule-rebuild trigger and the title FTS trigger. Record a key only when it was removed (if stored.remove(key).is_some()), and the common case runs none. Folding the rest into one statement (json_remove takes several paths) is optional. Principle: do no work for a no-op.

Nitpicks

• Nit: packages/core/tests/it/structural_rules_test.rs:420: the comment "An update that names neither rule leaves both as they are" sits above an update that names both and is expected to fail. It belongs above the Box half, eight lines down. That half does prove the claim; it does not check that abstract survives such an update (my probe confirms it does).
• Nit: packages/core/src/markdown/markdown_test.rs:1295: the test covers the root-id path only, which is why finding 1 was not caught.
• Nit: packages/core/src/db/sqlite_store/nodes.rs:1751-1766: the pre-existing remove_property_key, documented as "used by migrations", has no caller and now sits beside the new helper that does the same thing in a transaction. It predates this PR; deleting it here would follow the no-dead-code rule.

CLAUDE.md

Nothing in c4b85975 breaks a project rule: no compatibility code, no literal node_type comparison (the importer resolves the type through CoreNodeType::from_id and the registry's rule), no undeclared key, no issue numbers in comments.

[malibio]

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

[malibio]

Address-review summary (re-review 2)

The branch is now one commit on current main (see "Branch collapsed" below).

Addressed

Finding	Change
1 (Important) A single-file import still stored nested content ahead of its numbered item	import_markdown_content passes the root id to prepare_nodes_from_markdown, as every other import path does, and the or_else that mapped a missing parent to the root afterwards is gone. New daemon test single_file_import_keeps_content_under_a_numbered_item_in_reading_order imports the review's list-led document through that entry point and asserts the stored outline.
2 (Suggestion) update_schema ran three no-op json_remove statements on every call	merge_schema_definition records a key as cleared only when it was stored, so a schema with no rules and no abstract flag runs none.
Nit, misplaced test comment	Moved; the tightening step has its own comment.
Nit, remove_property_key had no caller	Deleted.

The nit that the parser test covers the root-id path only is answered by the daemon test: no production caller passes None for a document body any more.

Also changed

main changed count_roots to take include_archived; the new end-to-end test calls it with false.

Branch collapsed

The merge queue replays a PR's non-merge commits onto main, and this branch's conflict resolutions (the move onto generated TypeScript) lived in merge commits. The branch was collapsed into a single commit on top of 322da9ed with a tree identical to the reviewed-and-fixed one, and force-pushed.

Verification

On the collapsed commit: bun run quality:fix clean; bun run test:changed passes (frontend, scripts, browser, generated-TypeScript drift, Rust workspace).

Re-Review Decision

Decision: NO RE-REVIEW NEEDED

Rationale: the one Important finding is fixed with the change the review specified (one argument at one call site), proven by a test at that entry point. The rest is a guard that skips no-op statements, a deleted uncalled function and a comment. No design or logic beyond that changed since the reviewed head.

[malibio]

Merge queue: #3475 was taken out of the queue — it conflicts with main or with #3472, queued ahead of it, in: packages/core/src/behaviors/mod.rs, packages/core/src/models/core_schemas.rs, packages/desktop-app/src/lib/plugins/core-plugins.ts, packages/desktop-app/src/lib/types/generated/core-node-types.ts, packages/desktop-app/src/lib/types/generated/core-type-entry.ts, packages/desktop-app/src/tests/plugins/core-plugins.test.ts, packages/nodespace-types/src/bin/gen_ts.rs, packages/nodespace-types/src/core_type.rs. Rebase onto origin/main, push, and re-run bun run merge 3475.

[malibio]

Rebased onto main after the merge queue ejected this PR

The queue took #3475 out because it conflicted with #3472 (the ai-chat subtype split), queued ahead of it. #3472 has landed; this branch is rebased onto it and is still one commit.

What the rebase changed, beyond resolving the conflicts:

• ai-chat is now an abstract base. ai-chat-native and ai-chat-pty declare no structural rule of their own and inherit any / any; their behaviours no longer carry can_have_children.
• Split ai-chat into an abstract base with ai-chat-native and ai-chat-pty subtypes (closes #3446) #3472 kept the outline from offering indent under a chat (an editor choice, made while the chat viewer has no way to show non-message children) through the canHaveChildren plugin flag. That flag is removed here, so the choice is now an explicit check in indentNode, separate from the structural rule, which still lets a chat have children. The frontend test asserts indent under a chat is not offered while canHaveChild('ai-chat-native', 'text') is true.
• The generated registry entry carries both typedUpdate (from Split ai-chat into an abstract base with ai-chat-native and ai-chat-pty subtypes (closes #3446) #3472) and structure.
• The chat embedding-root test creates ai-chat-native and ai-chat-pty nodes.

The ai-chat-message rule is still not declared: that type is not in the registry yet.

Verification on the rebased commit: bun run quality:fix clean; bun run test:changed passes (frontend, scripts, browser, generated-TypeScript drift, Rust workspace). After one more rebase onto the latest main: core, types and daemon suites 3094/3094, types:check in sync.

No design change since the last review round, so no further re-review; re-queueing.