Let an edge field declare a closed enum value set (closes #2387)

[malibio]

Closes #2387.

EdgeField could be typed "enum" by string, but nothing declared what the permitted values were — so an access-control role on a relationship was free text end to end: unconstrained on write, a free-text box in the UI, and a default nothing checked. "Owner", "owner" and "onwer" were three distinct roles, and a typo produced a permission that silently grants nothing.

Decision: option 2 (core_values only)

Not the full SchemaField mirror. An edge enum is a fixed vocabulary — the motivating case is an RBAC role, where a user-extensible value set would mean a permission level nothing downstream knows how to check. user_values/extensible can be added later if a real use case appears; adding them now would be surface with no caller. The rationale is recorded on the field's doc comment, not just here.

What changed

• Type — EdgeField gains core_values: Option<Vec<EnumValue>>. One shared definition re-exported into core and the Tauri types, so it propagates with no conversion boundary to drop it.
• Declaration-time validation — validate_edge_field_declarations, wired into both the create and update schema paths, ahead of the write: coreValues is required on an enum edge field and rejected on any other type, values must be unique, and a declared default must be a member of the set.
• Write-time validation — validate_edge_data_against_fields on NodeService::create_relationship and update_relationship_properties. That is the one chokepoint every caller routes through, so the CLI's --edge-data, the daemon, the Tauri commands and the agent tools are covered by one check rather than each boundary validating separately.
• UI — the relationships modal renders a picker instead of a free-text input (both the per-row edit and the add-row form), and resolves a stored value to its declared label when read-only. getEnumValues/enumValueLabel are widened from SchemaField to an EnumFieldLike shape so edges reuse the node-side lookup rather than growing a second copy that could drift on how a value renders.
• Agent guidance — a new ENUM_EDGE_FIELDS rule in skill_rules.rs covering the RBAC shape, so an agent declaring one produces a valid value set.

Write validation is deliberately scoped to enum membership. An omitted or null value, a non-enum field, and an undeclared edge key are all left alone, so no existing caller changes acceptance — broader edge-value typing (numbers, dates, required-ness) is not enforced anywhere today and is not introduced here.

Drift gap closed along the way

render_schema_rules_block names each rule individually in a format string, so a rule registered in SCHEMA_RULES but omitted there is silently dropped from the shipped skill — and checked_in_skill_md_is_up_to_date still passes, because the generator and the checked-in copy agree on the same incomplete output. My rule hit exactly this. every_schema_rule_reaches_the_skill now asserts every registered rule actually lands in the skill; I verified it fails on the omission it is written for before restoring the template.

Testing

bun run test:all — exit 0. Frontend 4867 passed vs. a 4863 baseline (+4, the new tests); 57 Rust test binaries green, zero failures. bun run quality:fix clean (eslint, svelte-check, tsc, cargo fmt, clippy with -D warnings).

21 new backend tests and 4 new frontend tests, covering: the accepted RBAC shape; a value outside the set rejected and not persisted; wrong casing rejected; a non-string value rejected; omitted/null accepted; non-enum and undeclared keys passed through; the edit path validated with the prior value surviving a rejected edit; the declaration guards (missing/empty coreValues, coreValues on a non-enum, out-of-set default, non-string default, duplicate values); and six end-to-end tests through handle_create_schema/handle_update_schema covering the two call-site wirings, which review found had no coverage — a refactor could have dropped either and left every other test passing. Round-trip coverage keeps the property the existing tests hold — an enum edge field serializes to coreValues and deserializes back identically, and a non-enum field gains no empty key.

Scope: what this does NOT unblock

Worth stating plainly, because #2387's framing implies otherwise. The RBAC edge the issue motivates this with is a builtin relationship, and builtins cannot carry declared edgeFields at all — reject_reserved_relationship_names refuses any declaration named member_of/has_child/mentions/has_role, and set_schema_relationships re-checks it on the write path. So the issue's own example JSON ({"name": "member_of", ..., "edge_fields": [...]}) is rejected before it reaches any of this.

That is not a mis-scoping of the new check: with no declaration there is no edge_fields to validate against, so skipping builtins on the write path is the only correct behavior. But it means roles on has_role — today literally create_relationship(person, "has_role", settings, {"role": "owner", "status": "active"}) per ADR-037 — remain free text.

What lands is still real: any relationship you declare now gets a closed, validated vocabulary end to end, and #2387's acceptance criteria as written are met. The remaining design question (custom relationship for collection RBAC, vs. giving builtins an edge-field table) is filed as #2429 rather than guessed at here.

Two consequences for this PR, both from review:

• The ENUM_EDGE_FIELDS guidance states both limits outright rather than promising "every write path" — an agent following it will not emit the rejected member_of shape.
• schema_declaring_an_edge_field_on_a_builtin_relationship_is_rejected pins that behavior, so the guidance and the code cannot drift apart.

Also noted

addEdgeDraft starts empty, so a declared default is not pre-selected in the add-row picker, and nothing applies an edge field's default at write time either — an omitted enum key is stored absent. Both are pre-existing and true for every edge field type, not specific to enums; the declaration validator now says so explicitly, and the agent guidance warns against relying on a default. Left as-is rather than widening this change.

🤖 Generated with Claude Code

https://claude.ai/code/session_015xcKSmajXi7D4fCMsvaGMi

[malibio]

Code Review Summary

Verdict: APPROVE with one Improvement worth resolving before merge (or filing as an explicit follow-up).

This is a well-built, genuinely net-positive change. The design decision (option 2 — core_values only) is recorded with real reasoning rather than asserted, validation is placed at a single chokepoint rather than smeared across boundaries, and the tests assert the properties they claim to. The incidental fix — every_schema_rule_reaches_the_skill closing the render_schema_rules_block drift gap — is a genuine bonus catch and is exactly the right shape of guard (it compares against SCHEMA_RULES, the array a new rule is naturally added to, not against the template).

I verified the chokepoint claim, the builtin exclusion, the set_schema_relationships callers, and ran the new Rust tests (14 pass). One finding below is substantive; the rest are minor.

Findings

Improvements

1. member_of — the motivating RBAC case — cannot carry an enum edge field at all, and this PR does not change that. [Improvement]

You asked for a straight answer on this, so: yes, member_of is excluded from write-time validation, but the reason is deeper and more benign than a validation gap. It is not that the check was accidentally scoped to skip builtins — it is that a member_of edge field cannot be declared in the first place, on main or on this branch.

Two independent gates reject it:

• packages/core/src/schema/mod.rs:434 — reject_reserved_relationship_names refuses any declaration named after a builtin.
• packages/core/src/services/node_service/schema.rs:782 — set_schema_relationships re-checks the same predicate on the write path.

BUILTIN_RELATIONSHIP_NAMES (packages/core/src/models/schema.rs:55) is ["member_of", "has_child", "mentions", "has_role"]. So there is no schema declaration for member_of, therefore no edge_fields, therefore nothing for validate_edge_data_against_fields to validate. Placing the call inside the !is_builtin branch is correct given that — hoisting it out would be dead code, because the builtin branch never has a SchemaRelationship in hand to read edge_fields from.

So the finding is not "the check is mis-scoped." It is that the issue's stated motivation is not actually delivered by this PR, and nothing in the PR or the issue says so. The issue's own example is literally {"name": "member_of", "targetType": "collection", "edge_fields": [{"name": "role", "type": "enum", ...}]} — that exact JSON is rejected today by reject_reserved_relationship_names, before it ever reaches the new code.

This does not make the change wrong or unmergeable. What lands is real and correct: any custom relationship (assigned_to, works_on, a bespoke collection_membership) now gets a closed, validated vocabulary end to end. That is genuine value and the acceptance criteria as literally written are met. The problem is purely that the PR description and the skill prose both imply the RBAC-on-member_of case is now covered, and it is not.

Concretely, the RBAC work this was billed as a prerequisite for still has an unresolved design question ahead of it: either (a) collection RBAC uses a new custom relationship rather than builtin member_of, in which case this PR is a complete prerequisite and that should be stated; or (b) it must ride on member_of, in which case builtin relationships need a way to declare edge fields at all, and that is a separate and larger piece of work this PR does not begin.

Recommendation: Not a merge blocker. But before merge, either amend the PR body to state plainly that member_of edge fields remain undeclarable and name which of (a)/(b) is intended, or file the follow-up issue. Leaving it implicit is how a future reader concludes RBAC-on-member_of is solved, builds on that belief, and discovers the reserved-name rejection at implementation time. The skill_rules.rs prose is where this bites hardest — see finding 4.

2. The update_relationship_properties guard is nested inside if let Some(source) = self.get_node(source_id).await? — but this is benign. [Nit, verified not a bug]

You flagged this as a possible skip-path. I checked: it is unreachable in a way that matters. If source_id names no node, no edge can exist from it, so store.update_relationship_properties returns None and the method errors with "Relationship ... does not exist" at relationship.rs:1002. The write is rejected either way; only the error message differs. The existing structure (the schema-node declaration guard) already had this shape, and the new code correctly follows it rather than restructuring. No change needed.

3. A declared default is validated but never applied. [Improvement]

validate_edge_field_declarations correctly enforces that a default is a member of coreValues — this satisfies the acceptance criterion as written. But I searched packages/core/src/services/node_service/relationship.rs and found nothing that ever applies an edge field's default when a caller omits the key. create_relationship passes edge_data through verbatim for custom relationships (final_edge_data, line ~695), and create_relationship_allows_an_omitted_or_null_enum_edge_value confirms and locks in that an omitted role is simply stored absent.

So on the issue's own example — "required": true, "default": "viewer" — creating an edge without a role yields an edge with no role, not viewer. For an access-control field that is the security-relevant outcome the issue was written about: code reading the edge finds no role rather than the intended least-privilege default.

This is pre-existing behavior (required on an edge field is not enforced anywhere today either), and the PR is honest about that in the validate_edge_data_against_fields docblock. Correctly out of scope for this change. But the skill prose now actively advertises "required": true, "default": "viewer" to agents as a working shape, which will produce schemas whose authors reasonably expect a default to materialize. Worth a follow-up issue for edge-field default/required application, and worth not implying otherwise in the docs.

4. skill_rules.rs / cli.md prose overstates coverage on two points. [Improvement]

The ENUM_EDGE_FIELDS prose says: "Edge values are validated against the set on every write path (including --edge-data)", and offers {"name": "role", ..., "required": true, "default": "viewer"} as the model example, in a rule whose framing is "a role on a membership."

Both halves mislead an agent reading this as instruction:

• "every write path" is true only for custom relationships. A membership on builtin member_of — the example's own framing — cannot declare edge fields at all (finding 1).
• required is not enforced and default is not applied (finding 3), so the example's own attributes are decorative.

Because this text is the shipped agent-facing contract, an agent will confidently emit exactly the issue's member_of shape and get a reserved-name rejection it has no guidance for. Suggest tightening to "validated against the set on every relationship write path" and either dropping required/default from the example or noting they are declaration-time only. Small edit, and it is the one place where imprecision has a direct downstream cost.

5. set_schema_relationships callers in the playbook — not a gap. [Verified, no action]

You asked about graph_resolver.rs:639 and validation.rs:1219 bypassing validate_edge_field_declarations. Both are inside #[cfg(test)] mod tests { ... mod integration } (test module starts at graph_resolver.rs:361 and validation.rs:947). They are test fixtures, not production write paths. The only production callers are schema/mod.rs:762 (create) and :1291 (update), both correctly gated.

That said, the layering is worth noting: validate_edge_field_declarations lives in the handler layer, while its sibling reject_reserved_relationship_names is deliberately re-checked in set_schema_relationships (the service write path) precisely so it cannot be bypassed — the docblock at schema/mod.rs:432 says so explicitly. The new validation does not get that second line of defense. Given the greenfield posture and the absence of other callers this is acceptable; but mirroring the existing belt-and-braces pattern would cost ~5 lines and would make the two declaration validators structurally consistent rather than one being the exception.

Bypass audit (requested) — clean

I traced every production write path and found no bypass:

• packages/core/src/ops/rel_ops.rs:76,111 — both route through NodeService. This covers the CLI, daemon, Tauri commands and agent tools.
• packages/core/src/playbook/actions.rs:694 (execute_add_relationship) — routes through NodeService::create_relationship. Playbooks are covered.
• packages/core/src/services/collection_service.rs:426,534,579,784 — all member_of only, which has no declarable edge fields.
• Direct store.create_generic_relationship / store.add_to_collection calls outside NodeService are confined to tests and to sqlite_store internals.

The PR's claim that NodeService is "the one chokepoint every caller routes through" holds up.

Frontend

6. Select.Root with an empty-string value — sane, and matches the established pattern. [No action]

The new picker mirrors schema-field-leaf.svelte:49-65 almost exactly (uncontrolled value + onValueChange, label-or-placeholder in the trigger). enumValueLabel returns undefined for an empty/falsy value, so the || correctly falls through to Select ${formatColumn(field.name)}.... Consistent at both render sites.

Two small observations:

• The picker offers no way to clear a set value back to empty — once a role is chosen, the row is stuck with some role. The free-text input it replaces allowed clearing. For a required field that is arguably correct; for an optional enum edge field it is a small capability regression. schema-field-leaf has the same characteristic, so this is consistent rather than novel — mentioning it as a known edge, not asking for a fix here.
• The edgeInputKind fallback to 'text' when coreValues is empty is good defensive practice and the comment explains why. Worth keeping.

7. Widening SchemaField → EnumFieldLike is the right call. [Positive]

Structural typing over a shared minimal interface, rather than a second copy of the lookup, is exactly right — and the docblock states the why (drift between two surfaces rendering the same value) rather than the what. The RawEdgeField doc comment noting the absent userValues counterpart is a nice touch.

Test quality

Tests genuinely assert what they claim. Verified by running them (14 pass across both modules). Particularly good:

• create_relationship_rejects_an_undeclared_enum_edge_value asserts both the error content and that group.count == 0 — proving nothing was persisted, not merely that an error was returned.
• update_relationship_properties_validates_enum_edge_values asserts the original value survives the rejected edit, then that a legal edit still succeeds. That is a real state assertion, not a smoke test.
• test_edge_field_enum_round_trip asserts json.get("core_values").is_none() alongside the positive coreValues check — it would catch a serde rename regression, which a naive round-trip would not.
• every_schema_rule_reaches_the_skill matching on a 60-char prefix (with the reasoning for why an exact match is not a property that holds) is thoughtfully done.

Gaps, all minor:

• No test that handle_create_schema / handle_update_schema actually reject a bad edge enum declaration end to end. validate_edge_field_declarations is tested directly, but the two call-site wirings (schema/mod.rs:696, :1235) are not covered — a future refactor could drop either call and every test still passes.
• No frontend test for the picker itself. The widened helpers are well covered in schema-enum-values.test.ts, but edgeInputKind returning 'enum' vs the 'text' fallback, and formatEdgeValue's label resolution, are untested. Given the modal's existing test posture this is consistent, not a regression.

Standards compliance — clean

Checked the full diff against CLAUDE.md: no lint suppression, no #[allow(...)], no any / as any / @ts-, no raw console.*, no #[deprecated], no backward-compat or migration code, no .md docs added to this repo (packages/skill/references/cli.md is generated shipped content, not documentation), and no GitHub issue numbers embedded in code comments or doc content. Commit message follows the convention.

Nitpicks

• Nit: validate_edge_field_declarations duplicate detection is values[..i].iter().any(...) — O(n²). Irrelevant at realistic enum sizes and arguably clearer than a HashSet; noting only that the quadratic shape is deliberate, not overlooked.
• Nit: packages/core/src/services/node_service/relationship.rs — the non-string branch of validate_edge_data_against_fields open-codes a match producing "a boolean"/"a number"/…, while schema/mod.rs already has json_type_name for this. The two produce different strings ("a number" vs "number") so they are not drop-in interchangeable, but one shared helper would be DRYer.
• Nit: The // Bound to a local rather than chained off the `await?` comment in update_relationship_properties explains a real borrow-checker constraint — good "why" comment, exactly the kind CLAUDE.md asks for. Keeping it.

What I'd do before merge

Only finding 1 and finding 4 warrant action, and both are documentation-shaped rather than code-shaped:

1. State in the PR body (and ideally a follow-up issue) that builtin member_of still cannot declare edge fields, so the RBAC-on-collections case is not yet unblocked by this change.
2. Tighten the ENUM_EDGE_FIELDS prose so it does not promise "every write path" or imply required/default are enforced at write time.

Everything else is optional polish. The code itself is sound, well-tested, and clearly a net improvement to code health.

[malibio]

Verdict: APPROVE (posting as a comment — GitHub blocks self-approval on your own PR).

Full review posted as a PR comment above. Summary:

The change is sound, well-tested and a clear net improvement to code health. Validation is correctly placed at a single chokepoint, and I verified by tracing every production write path (ops/rel_ops, playbook actions, collection_service, direct store calls) that no bypass exists — the "one chokepoint" claim holds. All 14 new Rust tests pass locally. Standards compliance is clean: no lint suppression, no any, no raw console.*, no backward-compat/migration code, no docs added to this repo, no issue numbers in code comments.

Answers to the specific questions raised:

1. Is NodeService genuinely the only chokepoint? Yes — verified, no bypass.
2. Is the nested if let Some(source) guard a skip-path? No. A missing source node means no edge can exist, so the store call returns None and the write is rejected anyway. Only the error message differs.
3. Are builtins excluded, and does that break the motivating use case? This is the one finding that matters. member_of is excluded — but not because the check is mis-scoped. reject_reserved_relationship_names and set_schema_relationships both refuse any declaration named after a builtin, so member_of has no schema declaration and therefore no edge_fields to validate. The !is_builtin placement is correct given that. The real issue: the issue's motivating RBAC-on-member_of case is not delivered by this PR, and nothing says so. The issue's own member_of example JSON is rejected before it reaches the new code. What does land is real and valuable (custom relationships get closed validated vocabularies end to end), but the PR body and skill prose imply more coverage than exists.
4. Playbook set_schema_relationships callers? Both are inside #[cfg(test)] test fixtures, not production. No gap.
5. Svelte picker / empty-string Select.Root? Behaves sanely and mirrors the established schema-field-leaf.svelte pattern at both render sites.
6. Do the tests assert what they claim? Yes, and better than typical — the rejection tests assert nothing was persisted, and the update test asserts the original value survives a rejected edit.

Two documentation-shaped items worth resolving before merge (neither is a code blocker):

• State plainly that builtin member_of edge fields remain undeclarable, so the RBAC-on-collections work is not yet unblocked — or file the follow-up.
• Tighten the ENUM_EDGE_FIELDS skill prose: it promises validation on "every write path" (true only for custom relationships) and models required: true, default: \"viewer\" as a working shape, but edge-field default is validated at declaration time and never applied on write, and required is not enforced. Since this text is the shipped agent-facing contract, the imprecision has a direct downstream cost.

Not merging — that's your call.

[malibio]

Review addressed — 26ffb05

Thanks — the member_of finding was the right one to lead with, and the "not mis-scoped, but the motivation isn't delivered" framing is exactly correct.

Finding 1 — member_of / RBAC not unblocked. Confirmed independently before the review landed: builtins are rejected as declarations by reject_reserved_relationship_names and again by set_schema_relationships, so there is no edge_fields for the write-path check to read and skipping builtins is the only correct behavior. Also worth recording: roles already ride a builtin edge unvalidated today — create_relationship(person, "has_role", settings, {"role": "owner", "status": "active"}) per ADR-037 — so the exact hazard #2387 describes is live on has_role.

Addressed as documentation plus a pin, not code:

• PR body now has a "Scope: what this does NOT unblock" section stating it outright.
• Filed Builtin relationships cannot declare edge fields, so collection RBAC is still free text #2429 with the open design question (custom relationship for collection RBAC vs. an edge-field table for builtins), including the ADR-037 detail and acceptance criteria. Left unassigned per the backlog convention.
• schema_declaring_an_edge_field_on_a_builtin_relationship_is_rejected asserts the rejection, so the guidance and the behavior cannot drift.

Finding 4 — prose overstates coverage. Agreed on both halves. ENUM_EDGE_FIELDS no longer says "every write path" and no longer models required: true, default: "viewer" as if either were enforced. It now names both limits explicitly (builtin names are reserved so cannot carry edge fields; required/default are recorded but not applied, so an omitted key is stored absent) and the example is an access level on a custom relationship — a shape an agent can actually declare. Skill content regenerated.

Finding 3 — validated-but-never-applied default. Kept out of scope as you suggested, but no longer silent: validate_edge_field_declarations' docblock now says validating a default is not applying one, and the agent guidance warns against relying on it. Recorded in #2429's criteria too.

Test gap — uncovered call-site wirings. This was the most actionable item and is fixed: six end-to-end tests through handle_create_schema/handle_update_schema. You were right that a refactor could have dropped either wiring with every other test still passing. (These caught a real error in my first draft — the update param is add_relationships, not relationships.)

Finding 2 (nesting), 5 (playbook callers), 6/7 (frontend): agreed with your verification, no change.

Not done, deliberately: the second-line-of-defense re-check in set_schema_relationships mirroring reject_reserved_relationship_names. Reasonable suggestion, but the two production callers are both gated and covered by the new end-to-end tests, so I'd rather not add a redundant check without a bypass to point at. Happy to add it if you'd prefer the symmetry.

Also left alone: the O(n²) duplicate scan (bounded by a handful of values, and clearer than a HashSet) and the json_type_name duplication (different output strings, so not drop-in).

Verification: bun run test:all exit 0 — frontend 4867 passed (vs. 4863 baseline), 57 Rust binaries green, zero failures. quality:fix clean. Pre-push gate passed.