[mstomar125]

Part of #3414 (the "Typed subtypes and edge fields" section, ADR-082 §2.2 and §5). This is the third of three PRs, and the last open section of that umbrella. It is stacked on #3625: the base is issue-3414-typed-subtypes, so the diff shows only this PR's commit. Retarget it to main once #3624 and #3625 land.

What changes

DaemonExtensions::edge_fields(declaration) adds typed fields to a core relationship.

• Storage. The fields live in the declaring extension's bucket of the edge's properties, properties.<extension id>.* (for example properties.fixture.permission on member_of), beside core's order.
• Validation. Every database the daemon opens validates that bucket on each edge write through its node API.

Core (nodespace_core::extensions):

• EdgeFieldDeclaration::new(relationship, extension_id, fields), with an optional .with_validator(|bucket| …) over the whole bucket for cross-field rules.

  • Fields are EdgeFields with a scalar type (text, number, boolean, date, datetime, or enum with its values), optionally required.
  • DataExtensions::edge_fields carries them, and NodeService::new_with_extensions builds the node service's registry.

• Where the check runs. The node service checks registered buckets at the three places a caller's edge properties enter, before anything is stored:

  • create_relationship, built-in branch, which covers both member_of paths, auto-ordered included;
  • write_relationship_in_tx, which covers declared relationships, Play add_relationship actions and multi-edge creates;
  • update_relationship_properties, for built-in and declared relationships.

  An idempotent create of an existing edge is checked too. Every other edge write in core builds its own properties ({} or {"order"}); the write-path inventory is in the review thread.

• What a bucket must satisfy. It must be an object of declared fields, each null or of its type (dates checked as node fields are, through NodeService::scalar_mismatch), with every required field set. Then the validator runs.

• Unregistered buckets. A bucket nobody registered is never read. That includes the build's own bucket on a relationship it declared nothing for.

Rejected at startup (through DataExtensions::check):

• A relationship other than member_of and has_role (EDGE_FIELD_RELATIONSHIPS). Core never rewrites the properties of those two. It recreates has_child edges on a cross-parent move or move_children_to_parent, and mentions edges follow the content, so a bucket on either would be silently dropped.
• An extension id that is not one, or that is order, the key core stores on its edges.
• A second declaration for one relationship and id.
• No fields.
• A field that is unnamed, repeated, of type array, object or link, an enum without values, a non-enum with values, or carries default (core never writes the bucket), indexed or targetType.

Daemon: DaemonExtensions::edge_fields delegates to the core value. DaemonExtensionsError::Data (non-exhaustive since #3624) carries DataExtensionsError::EdgeFields { relationship, extension_id, reason }.

With no declaration, the registry is empty and validate returns at once. Every edge write behaves as before, and undeclared keys on an edge stay a supported, untouched shape.

Acceptance mapping (#3414, typed subtypes and edge fields)

• build_shared_services takes edge-field declarations per relationship type (extension id, typed fields, optional validator). Extension edge fields are stored and validated in the extension's bucket (properties.<id>.*), and core never reads an unregistered bucket.
• Core fixture: an edge field on member_of.

With #3624 and #3625, every item of the section is covered.

Tests

Core fixture (packages/core/src/extensions/fixture_tests.rs, watched since #3625). The fixture declares permission (a required enum: admin, modify, read_only) and since (a date) on member_of in the fixture bucket, with a validator that makes admin need since.

• an_edge_field_on_member_of_is_stored_in_the_extension_bucket: both create paths store the bucket beside order.
• an_invalid_bucket_is_refused_on_create: six ways a bucket can be wrong, each refused with nothing stored:
  • a bad enum value;
  • a bucket that is not an object;
  • an undeclared key;
  • a missing required field;
  • the wrong type;
  • the validator's rule.
• an_invalid_bucket_is_refused_in_a_transaction: create_relationship_in_tx.
• an_invalid_bucket_is_refused_on_update_and_on_an_existing_edge.
• a_bucket_nobody_registered_is_left_alone: three cases:
  • another bucket on member_of;
  • the fixture's bucket on mentions;
  • the fixture's bucket with no declaration.
• a_node_service_with_a_refused_edge_field_declaration_is_refused: the refusal comes before anything is written.

Unit tests:

• extensions::tests::an_edge_field_declaration_on_member_of_or_has_role_is_accepted: every scalar type on both relationships.
• …a_malformed_edge_field_declaration_is_rejected: 13 malformed declarations, the duplicate, and acceptance across relationships and ids.

Daemon fixture (extension_points_fixture.rs, through build_shared_services with the isolated home and no model):

• an_edge_field_is_validated_in_every_database: two databases each refuse a bad value and store a good one in the bucket.
• an_edge_field_declaration_on_has_child_stops_startup.

Mutation check. 19 mutants, all killed. Every new test is killed by at least one:

Mutant	Killed by
C1: create_relationship built-in branch skips the check	…refused_on_create, …on_update_and_on_an_existing_edge
C2: write_relationship_in_tx skips it	…refused_in_a_transaction
C3: update_relationship_properties skips it	…on_update_and_on_an_existing_edge
C4: the node service ignores the declarations	the three refusal tests
C5: a refused declaration falls back to none	a_node_service_with_a_refused_edge_field_declaration_is_refused
C6: declarations apply to every relationship	a_bucket_nobody_registered_is_left_alone (the mentions case)
C7: a missing bucket is an error	a_bucket_nobody_registered_is_left_alone
C8: a non-object bucket passes	…refused_on_create
C9: undeclared keys pass	…refused_on_create
C10: required fields not enforced	…refused_on_create
C11: field types not checked	the three refusal tests
C12: validator not called	…refused_on_create
C13: any relationship accepted	a_malformed_edge_field_declaration_is_rejected, a_node_service_with_a_refused_…
C14: order accepted as an id	a_malformed_…
C15: a repeated declaration accepted	a_malformed_…
C16: datetime not an edge type	an_edge_field_declaration_on_member_of_or_has_role_is_accepted
C17: a field may carry a default	a_malformed_…
C18: DaemonExtensions::edge_fields drops it	both daemon tests
C19: add_to_collection drops caller data (existing code)	an_edge_field_on_member_of_is_stored_in_the_extension_bucket

Local runs (crate-scoped, under the machine slot):

• cargo clippy -p nodespace-core -p nodespace-daemon --all-targets -- -D warnings -D clippy::unused-async: clean.
• Core --lib extensions:: relationship member_of collection edge abstract unknown retype instantiable: 378/378, which includes core's existing relationship write-path tests.
• Daemon extension_points_fixture required_extensions services::extensions per_db: 26/26.
• Repo-wide checks:
  • bun test scripts/: 1441/1441;
  • the full frontend suite: 5875/5875;
  • fmt, Pro boundary, node-type literals and the extension-API version check: all pass.

Extension API

2.5 → 2.6 (minor). This adds a daemon hook: DaemonExtensions::edge_fields, EdgeFieldDeclaration and DataExtensions::edge_fields.

• The bump touches the Rust constant and its pin, the TypeScript constant, and the re-recorded surface snapshot (only version).
• Both watched fixtures change.

Documentation

NodeSpaceAI/nodespace-docs#109 (branch issue-3414-typed-subtypes) gains:

• an "Edge fields" section;
• failure-isolation rows for the startup refusal and a refused edge write;
• version 2.6.

"Not built yet" becomes "Outside this guide" (the §7 deferred points and the §5 daemon points with their own APIs), because every hook of this section is now built.

🤖 Generated with Claude Code

[mstomar125]

Pragmatic Code Review: PR #3626 (medium depth)

Review Type: Initial Review. The PR had no reviews or comments yet.

• Scope. The PR is stacked on Register subtype behaviours from build_shared_services in every database #3625, so this review covers its one commit (5f4cd501) against issue-3414-typed-subtypes.
• Context. I read the surrounding code at origin/issue-3414-edge-fields.
• Not run. I built and ran nothing, because the machine is shared.

Requirements Check

Sources: #3414 "Typed subtypes and edge fields", ADR-082 §2.2/§5/§8/§9, and ADR-083 §2.

• ✅ Declarations reach every database. build_shared_services takes edge-field declarations per relationship: a relationship, an extension id, typed EdgeFields and an optional validator.
  • The path is DaemonExtensions::edge_fields → DataExtensions::edge_fields → each database's NodeService::new_with_extensions (assembly.rs:414).
  • Core's default registers none.
• ✅ Storage. The fields are stored in properties.<extension id>.* next to order.
  • Core never writes a bucket: default is refused at startup, and no core path writes one.
• ✅ Validation coverage. Every node-API edge write that takes properties from a caller is validated. The inventory is below.
• ✅ Unregistered buckets are never read. The registry filters by relationship, then reads only properties[<registered id>].
  • The validator receives only its own bucket.
  • A test covers another bucket on member_of, the fixture's bucket on mentions, and a service with no declarations.
• ✅ Startup refusal. It runs through DataExtensions::check / build_shared_services, before anything is seeded. The test asserts that core's schemas were not seeded.
• ✅ Fixtures (§9).
  • Core: an edge field on member_of.
  • Daemon: validation in every database, and the has_child startup refusal.
• ✅ Versioning (§8). A new hook is a minor change, so 2.5 → 2.6 is right. It is applied to the Rust constant, its pin test, the TS constant and the surface snapshot, and both watched fixtures changed.
• ✅ ADR-083 §2 amendment. order is the only key core stores, and order is refused as a bucket name (CORE_EDGE_KEYS).
• ✅ No declarations, no change. The registry is empty and validate is a pure no-op. The only other edits are two pub(crate) visibility changes in crud.rs.

Write-path coverage (checked independently)

The PR body says the inventory "is in the review thread", but the PR has no thread yet. Here it is.

Path	Who builds the properties	Covered
create_relationship, built-in branch: auto-ordered member_of via SqliteStore::add_to_collection, explicit order, has_role, mentions	caller	✅ relationship.rs:862, before both idempotency checks
write_relationship_in_tx ← create_relationship_in_tx: declared relationships, invariant Play add_relationship, create_node_with_relationships (including the daemon's chat_messages)	caller	✅ :1203, after the in rewrite settles the stored name
update_relationship_properties: gRPC, Tauri command, rel_ops	caller	✅ :2287
Non-invariant Play add_relationship (actions.rs:1976) → create_relationship	caller	✅
Daemon gRPC CreateRelationship / UpdateRelationshipProperties → rel_ops → service	caller	✅
CollectionService (add_to_collection*, path resolve), markdown import, bulk_add_to_collections, has_role seeding	core: {} or {"order"}	n/a
Same-parent reorder and member reorders	json_set('$.order'), which keeps other keys	n/a
merge_nodes_in_tx repoint	UPDATE in_node/out_node, which keeps properties	n/a
Cross-parent move_node (nodes.rs:3279), move_children_to_parent_in_tx (:3390), mention sync	DELETE + INSERT of {"order"} / '{}', on has_child / mentions only	the reason those two are excluded

The direct SqliteStore writers (add_to_collection, update_relationship_properties) skip the check. They are store API, though, not the node API that ADR-082 §2.2 scopes enforcement to, and their only non-test callers pass {}.

Validating before the idempotency no-op is right. Without it, a create of an existing edge with an invalid bucket would report success for data that was neither valid nor stored. Refusing it is fail-fast input validation. It also makes the two member_of create paths behave alike: the auto-order path detects the existing edge inside the store and returns Ok(None).

Limiting declarations to member_of and has_role is sound.

• Checked. has_child cross-parent moves and move_children_to_parent_in_tx are DELETE + INSERT {"order": …}, and mention edges are recreated as '{}'.
• Nothing rewrites the other two. No core path rewrites member_of or has_role properties wholesale. Seeding creates has_role with {} only when it is missing, and set_local_person_identity touches no edge.
• Against the ADR. This is narrower than ADR-082 §2.2's wording ("core relationships"), but it matches the ADR's intent and both of its uses: permission on member_of, and roles on has_role per the ADR-083 §2 amendment.
• Fails loudly. An unsupported relationship is refused at startup instead of silently losing data later.
• Can widen later. Adding a relationship would be a minor change, and docs#109 records the limit.

Findings

No 🔴 Critical or 🟡 Important findings. None of the Critical-checklist items apply:

• no core-declared edge key;
• no node_type literal;
• no lifecycle_status read;
• EdgeField is reused from nodespace-types, not redefined;
• no compatibility code.

---

🟢 Suggestion: make the versioned surface cheap to widen
📁 packages/core/src/extensions/edge_fields.rs:24, :42; packages/core/src/extensions.rs:25

• EDGE_FIELD_RELATIONSHIPS type. pub const EDGE_FIELD_RELATIONSHIPS: [&str; 2] puts the length in the public type.
  • Admitting a third relationship later would therefore be a retype, which ADR-082 §8 counts as major, not an additive minor change.
  • pub const EDGE_FIELD_RELATIONSHIPS: &[&str] = &["member_of", "has_role"]; keeps widening additive.
• EdgeFieldValidator alias. It is re-exported, but no public signature uses it: with_validator is generic and the field is private.
  • That makes it versioned API surface with no consumer. Either drop it from the pub use, or use it in a signature.
• Validator context (optional). The validator sees only the bucket, which is enough for the fixture and for a permission rule.
  • If a rule might ever need the edge's endpoints or relationship name, a small #[non_exhaustive] context argument added now avoids a major bump later.
  • YAGNI is a fair reason to skip this one.

🟢 Suggestion: enum declarations are checked less strictly than schema edgeFields
📁 packages/core/src/extensions/edge_fields.rs:170-176 (compare packages/core/src/schema/mod.rs:1716, validate_edge_field_declarations)

• Duplicates. field_problem refuses an enum with no values but accepts duplicate coreValues. The schema-side validator rejects duplicates ("Each value must be unique").
• Why it matters. Write validation is unaffected, because any still matches. But the same wire type now has two acceptance rules. A one-line uniqueness check, or sharing the enum part of the schema validator, would keep one source of truth.
• Field names. They are checked only for being non-empty. That is low risk, since they are only used as map keys.

🟢 Suggestion: the text arm of type_mismatch is never exercised
📁 packages/core/src/extensions/edge_fields.rs:250-272

• The gap. The fixtures declare an enum and a date, so no new test hits the Text arm (!value.is_string()). Changing that arm to None survives every new test.
  • The number/boolean/datetime types share the scalar_mismatch arm, which the date case does kill.
• The fix. type_mismatch is a pure function. A small table-driven unit test, with one good and one bad value per type, closes the gap cheaply. It also pins the exact "must be …" wording that callers see.

🟢 Nit: comments suggest declared relationships can carry a registered bucket
📁 packages/core/src/services/node_service/relationship.rs:2285-2286. The same wording is in the PR body: "write_relationship_in_tx, which covers declared relationships".

• Declarations are limited to member_of and has_role, so the check can only ever match a built-in.
• Running it on declared names is harmless and future-proof. But "built-in or declared" reads as if a declared relationship could carry a bucket.
• Something like "checked for every relationship; only built-ins can have a registered bucket" would be accurate.

🟢 Nit: the doc list misses one refusal
📁 packages/core/src/extensions.rs:103-107

• The DataExtensions::edge_fields doc leaves out "a non-enum field with values", which field_problem refuses and which the PR body and docs list.
• Separately, an unnamed field's error reads field '': a field needs a name. Dropping the field '…': prefix in that one case reads better.

What's good

• One small check. EdgeFieldRegistry::validate is a small pure check, called at the three points where caller properties enter. Each call comes before any read or write.
  • No new store logic.
  • No double validation: declared relationships return early into the transactional path.
• Clear errors.
  • Refusal messages name the bucket, field, relationship and the expected type or values.
  • Startup refusals are typed (DataExtensionsError::EdgeFields { relationship, extension_id, reason }) inside non_exhaustive enums.
• Room to grow. EdgeFieldDeclaration has private fields and a builder, so it can grow without breaking callers.
• Same date rules as node fields. Reusing NodeService::scalar_mismatch gives edge date/datetime fields exactly the semantics node fields have.
• Thorough tests.
  • Every refusal asserts that nothing was stored.
  • They cover a create of an existing edge, the unregistered-bucket and no-declaration cases, and the daemon's per-database wiring.
  • The 19-mutant table is convincing.

Recommendation: APPROVE

This is a clear net improvement. It meets the section's acceptance criteria, ADR-082 §2.2/§5/§8/§9 and ADR-083 §2.

The suggestions are optional polish. Only the &[&str] const is worth doing before this ships in a core release, because changing it afterwards is a major extension-API bump.

[mstomar125]

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

[mstomar125]

Address review (round 1)

I addressed every 🟢 finding of the APPROVE review in 0b163cfe.

✅ Addressed

• Widenable surface. EDGE_FIELD_RELATIONSHIPS is now &[&str], so adding a relationship later is a minor change. The EdgeFieldValidator alias is no longer exported: with_validator takes any Fn(&Map<String, Value>) -> Result<(), String>, so nothing public names it. I kept the validator signature without a context argument, the YAGNI call the review allowed. A context can come later as a second, additive method.
• Duplicate enum values in a declaration are refused (the value 'a' is listed twice), matching validate_edge_field_declarations for schema edgeFields. A new case is added to a_malformed_edge_field_declaration_is_rejected.
• The Text arm and every other scalar type are now covered by extensions::edge_fields::tests::each_field_type_admits_its_values_and_refuses_others. The test checks text, number, boolean, date, datetime and enum: each admits its value, refuses another JSON type, and accepts null.
• Nit: the update path's comment now says only a built-in relationship can carry registered fields.
• Nit: the edge_fields doc lists every refusal, the non-enum-with-values one included. An unnamed field now reads a field needs a name, without the empty quotes.

Mutation check of the fixes. All 3 mutants are killed:

• the Text arm returning None (the mutant that survived before) is caught by each_field_type_admits_…;
• accepting repeated enum values is caught by a_malformed_edge_field_declaration_is_rejected;
• refusing null on an optional field is caught by each_field_type_admits_….

Local runs:

• clippy (core and daemon, --all-targets, -D warnings): clean.
• Core extensions:: relationship member_of: 209/209.
• Daemon extension_points_fixture services::extensions: 10/10.
• bun test scripts/: 1441/1441.
• fmt, Pro boundary and the version check: pass.

Coordination. Michael's open #3630 shares four files with this stack but none of the functions I changed. A merge-tree of the stack with #3630, and of the stack onto current main, are both clean.

Re-Review Decision

Decision: NO RE-REVIEW NEEDED

Rationale: These are small, low-risk changes: a const type, an un-exported alias, one more declaration check with its test, a new unit test, and comment and doc wording. Each is mutation-checked.

🤖 Generated with Claude Code

[mstomar125]

Rebased onto main after #3625 merged: git rebase --onto origin/main 06e90a0a, which drops #3625's pre-rebase commits. The new head is 48ab70ca, now based on main. The rebase had no conflicts and only this PR's two commits are replayed. check-extension-api-version passes (2.5 → 2.6), and the push check passed.

🤖 Generated with Claude Code