[malibio]

Closes #3432

What changed

In core the has_role edge from the local PersonNode to the settings singleton records whose database this is and nothing else (ADR-083 §2, amended 2026-10-01). This makes the code match.

• seed_database_settings_if_needed creates the edge with empty properties instead of {role: "owner", status: "active"}.
• get_local_person resolves the person by the edge's existence. The role == "owner" filter is gone, along with the warning for "edges exist but none is the owner", which can no longer occur.
• It still warns when more than one has_role edge points at the singleton, and still falls back to the first person node when there is none.
• Comments in behaviors/mod.rs, core_schemas.rs, sqlite_store/relationships.rs and node_service.proto no longer describe role values on the edge.

No migration: reset the database.

Node type sequence

Relationship change only: the built-in has_role edge loses its two property keys. No node type, schema field, wire type or generated TypeScript changes. The docs (cloud-sync.md, data-layer.md, node-types.md, typed-relationships.md) already describe the new shape.

Tests

• The seeding test asserts the edge exists with properties == {}.
• test_get_local_person_ignores_non_owner_has_role_edges and ..._warns_when_has_role_edges_exist_without_owner_role are removed: both tested the role filter.
• New: a warning fires when a second has_role edge exists; no warning fires for the seeded property-less edge.
• bun run test:changed passes (rust and scripts tiers, including the Pro-boundary check).

🤖 Generated with Claude Code

[malibio]

Pragmatic Code Review — PR #3458

Review Type: Initial review (no prior reviews or comments on the PR). One commit, e7e59508, 5 files, +76 / -137.

Summary

The change does what ADR-083 §2 (amended 2026-10-01) asks: seeding writes the owner has_role edge with {}, and get_local_person resolves the person by the edge's existence. It is a net simplification, the diff is atomic, and no core code reads role or status from a has_role edge any more. No Critical findings. Two non-blocking improvements and a few nits follow.

Requirements Check (issue #3432)

• ✅ A fresh database's has_role edge has no role or status properties (test). test_seed_database_settings_singleton_with_owner_edge asserts edge.properties == {} (packages/core/src/services/node_service/mod.rs:8777-8787). Ran it: passes.
• ✅ get_local_person and set_local_person_identity behave as before. Ran the 11 get_local_person / seed_database_settings / set_local_person_identity lib tests: all pass. The two removed tests covered only the role filter, which no longer exists.
• ✅ No core code reads role or status from a has_role edge. Searched Rust, TS, Svelte, proto, CLI, daemon, desktop app, skill references and eval fixtures. Every remaining "role" edge property is on a user-declared relationship (assigned_to and similar test schemas), not on has_role. get_related_nodes_with_edges is no longer called from get_local_person and still has live callers (rel_ops.rs:735, integration tests).
• ✅ The Pro-boundary check stays at zero. Ran bun run scripts/check-pro-boundary.ts: zero in every class.
• ✅ / not independently verified bun run test:changed passes; bun run quality:fix clean. cargo fmt --all -- --check is clean and the targeted tests pass. I did not run the full test:changed tier; that part rests on the PR description.

Node type sequence

Relationship change only. No registry, schema field, wire type or generated TypeScript is involved, and I found no frontend reader of the edge's properties. components/node-types.md:55 and architecture/data-layer.md:73 in nodespace-docs already list has_role with no properties, and typed-relationships.md:111 states the rule. None of the Critical triggers applies: no undeclared key or edge property, no new literal node_type comparison, no lifecycle_status read, no second wire type, no stored ids, no new id.

Is what remains in get_local_person consistent with CLAUDE.md?

Yes, with one leftover (finding 1). The fallback to the first person node was introduced in #2449 for databases that predate the owner edge, which is exactly the kind of code CLAUDE.md bans. I do not rate it Critical here because the state is reachable in the current shape: has_role is a universal built-in that any client can delete, and deleting the owner person while another person exists also leaves no edge. In both cases the fallback returns the same node that seed_database_settings_if_needed re-attaches the edge to on the next open (first person). So it is a consistent view of a current-shape state, not a reader for an old shape, and the issue's second criterion asks for the behaviour to stay. The PR also removed the "predates ADR-037" rationale from the comments and the proto.

Findings

Critical Issues

None.

Suggested Improvements

1. 🟡 Important — zero owner edges is silent, and a test pins the silence
📁 packages/core/src/services/node_service/mod.rs:1752-1759 and 9097-9116

The fallback used to be silent because "no has_role edge at all" was the legitimate pre-ADR-037 state. That state is gone: seeding guarantees the edge on every open and repairs it when missing. Zero edges is now as much an anomaly as two edges, yet two edges warn and zero edges do not, and test_get_local_person_does_not_warn_when_no_has_role_edges_exist still asserts !warned. That assertion is the one remaining piece of behaviour whose only justification was the old shape.

Recommendation: emit a tracing::warn! on the fallback path too and flip that test to expect it, or at least drop the !warned assertion so the test no longer protects the old rationale. Principle: an invariant ("a database has exactly one owner edge") should be reported the same way whichever side it is broken from; a silent fallback hides the anomaly the sibling branch was written to surface.

2. 🟡 Important — with the role filter gone, a second edge makes the local identity order-dependent, and the new test accepts either answer
📁 packages/core/src/services/node_service/mod.rs:1740-1751, 9069-9074; packages/core/src/db/sqlite_store/relationships.rs:1856-1857

get_nodes_by_relationship has no ORDER BY, so "the first one returned" is whatever SQLite yields. Before this change a stray edge only mattered if it carried role: "owner". Now any bare has_role edge into the singleton counts, from any source node (no person-type check), and has_role is advertised to the CLI and the local agent as a universal name (packages/agent/src/local_agent/tools.rs:1250). The new test asserts local.id == seeded_id || local.id == other_id, which passes for either outcome and so does not pin a behaviour.

Recommendation, cheapest first: make the pick deterministic (for example order the traversal by the edge's creation time so the seeded edge wins) and assert local.id == seeded_id in the test. The warning already covers visibility, so this is not a blocker for a single-user database. Principle: deterministic resolution of identity; a test should fail when the behaviour it names changes.

Nitpicks

• 🟢 Nit: packages/core/src/services/node_service/mod.rs:1621-1622 — seed_local_person_if_needed still says "so an existing database is backfilled on next open too". The PR removed the same phrase from the sibling seed_database_settings_if_needed comment; removing it here keeps the two consistent with CLAUDE.md's no-backfill wording.
• 🟢 Nit: packages/skill/references/cli.md:363 — "hardcoded semantics — hierarchy, mentions, collection membership, and roles". In core has_role now means database ownership, not a role. Optional; changing it touches the SKILL.md drift check.
• 🟢 Suggestion (nodespace-docs, not this repo): the PR body says cloud-sync.md already describes the new shape. The amendment note at line 499 does, but lines 476 and 527 still say the edge "always carr[ies] the constant value {role: 'owner', status: 'active'}" in the present tense, and the HasRoleEdgeProperties interface at 501-508 is still shown under the note. Worth a direct-to-main cleanup.
• 🟢 Observation, out of scope: core does not reject caller-supplied properties on a built-in edge (packages/core/src/services/node_service/relationship.rs:997-1005 carries edge_data as-is). ADR-083 §2's rule is about what core writes, so this PR is compliant; enforcement would be a separate decision.

What is good

• The doc comments on seed_database_settings_if_needed and get_local_person now state the current rule and cite ADR-083 §2, with no issue numbers.
• The replacement tests cover both sides of the new warning (second edge warns; the seeded property-less edge does not).
• get_relationship_record's doc example moved from role/status on has_role to order on member_of, which matches the ADR's rule for built-in edges.

Verification basis

• Ran: the 11 targeted nextest lib tests (pass), scripts/check-pro-boundary.ts (zero), cargo fmt --all -- --check (clean).
• Read: the full diff, seed_database_settings_if_needed, get_local_person, set_local_person_identity, the store traversal SQL, every has_role hit in the repo, ADR-083 §2 with its amendment, the node type sequence, and the has_role rows in data-layer.md, typed-relationships.md, node-types.md and cloud-sync.md. The fallback's origin comes from git log -S (Prompt for local user identity during setup, writing to the seeded person node #2449).
• Inferred, not executed: that an unordered traversal can return the non-seeded person first (finding 2; no ORDER BY in the SQL, but I did not reproduce a flip), and that bun run test:changed and bun run quality:fix pass as the PR states.

Recommendation: APPROVE

All five acceptance criteria are met and nothing blocks the merge. Findings 1 and 2 are small and sit in the lines this PR already rewrote, so they are worth folding in here rather than filing separately.

[malibio]

Automated pragmatic-code-review: APPROVE (posted as a comment — GitHub blocks self-approval). All five acceptance criteria met; no Critical findings; two non-blocking improvements (silent zero-edge fallback, order-dependent pick with a second edge) and a few nits. Full findings in the review comment above.

[malibio]

Review addressed (936c8e7)

Addressed

• Finding 1 (zero edges silent): get_local_person now warns when it falls back because no has_role edge exists. The test is flipped and renamed test_get_local_person_warns_when_no_has_role_edge_exists.
• Finding 2 (order-dependent pick): with more than one edge it resolves the earliest-created node (tie-break on id), so the result no longer depends on SQL row order. The test now asserts the seeded person, not "either".
• Nit: removed "backfilled on next open" from the seed_local_person_if_needed comment.

Skipped

• packages/skill/references/cli.md "roles" wording: a generic description of the built-in name, generated from the skill rules; changing it is outside this issue.
• cloud-sync.md (nodespace-docs): the doc keeps the old text under an "Amended by ADR-083 §2" note that already says the constant is no longer written, the same convention it uses for its other superseded sections.
• Rejecting caller-supplied properties on built-in edges: out of scope, as the review notes.

Verification: bun run test:changed passes (rust and scripts tiers); bun run quality:fix leaves no changes.

Re-Review Decision

Decision: NO RE-REVIEW NEEDED

Rationale: about 20 lines in one function, implementing the reviewer's two recommendations as written, each pinned by a test that fails on the previous behaviour. No new functionality or design change.