[malibio]

Closes #2910.

create_relationship enforces a one end by evicting the prior edge, not rejecting — so a write could silently steal an assignment. This makes that visible on every surface.

API shape (the design decision)

• NodeService::create_relationship / create_relationship_in_tx → Result<Vec<ReplacedEdge>> (source, relationship name, target of each evicted edge; empty for a plain create, an idempotent repeat, or a builtin).
• rel_ops::CreateRelOutput.replaced → proto CreateRelationshipResponse.replaced (repeated RelationshipEdge).
• Relationship groups gain far_cardinality (the other end's cardinality), so the viewer knows when a link can displace someone without fetching anything.

Surfaces

• CLI relationship create: prints a Replaced relationship: A --[r]--> B line per eviction; --json includes replaced.
• Agent tool create_relationship: result includes replaced: [{from_id, to_id, type}] + a note telling the model to tell the user which link was removed. (packages/agent/src/local_agent/ change — needs your sign-off per the issue.)
• Frontend: Tauri command + dev-proxy return { replaced }. Before an add whose far end is one, the viewer fetches the candidate's relationships and confirm()s if it's currently held by someone else — in both the modal and the single-select RelationshipField. In the modal, a local-one group (one with edge fields) also warns that its current link will be replaced. Declining writes nothing.
• relationship-viewer-service.ts: stale "daemon validates cardinality" comment replaced with the actual replace semantics.

Tests

• Core: replace tests assert the returned evicted edge (forward + reverse), empty on plain create and idempotent repeat; far_cardinality asserted on both sides.
• Agent: create_relationship_reports_replaced_edge (real NodeService).
• Frontend: farEndHolders / reassignmentPrompt unit tests; modal (decline → no write, confirm → write, no holder → no prompt, far many → no lookup); field one-to-one decline/confirm.
• Pre-push gate (full pyramid) passed.

🤖 Generated with Claude Code

[malibio]

Pragmatic Code Review: PR #3079 (closes #2910)

Review Type: Initial Review

Summary

This is a solid change and the layering is right. The backend now returns the edges it evicted (Vec<ReplacedEdge>) from create_relationship_in_tx. Every surface (proto/daemon, CLI, agent tool, Tauri, dev-proxy) forwards that value and does not recompute it. The public non-tx create_relationship sends every declared relationship through the tx twin, so the report is complete. Builtins, the idempotent repeat, and the auto-order short-circuits correctly return Vec::new(). The core tests cover both the forward and reverse eviction, and they also cover the idempotent repeat. The busy handling in the frontend is correct: window.confirm is synchronous, and nothing yields between busy = false and the next write()/runMutation() setting it again, so a second pick has no window to slip in.

One correctness bug blocks merge. The frontend's farEndHolders mirror match does not take the declaring schema into account. It therefore raises false "removes that link" warnings on the seeded person.tasks / project.tasks pair, which is the most common assign flow.

Requirements Check

• ✅ Design decision on the API shape: create_relationship returns Vec<ReplacedEdge>. It goes over the wire as CreateRelationshipResponse.replaced (RelationshipEdge), and the frontend receives { replaced }.
• ⚠️ The modal warns before an assign that would reassign. The warning is implemented, but it also fires for links that will not be removed (see finding 1).
• ✅ The relationship-viewer-service.ts doc comment now describes replace, not reject.
• ✅ CLI relationship create prints a Replaced relationship: line for each eviction, and --json includes replaced.
• ✅ The agent tool result includes replaced plus a note. The issue says agent-lane changes need Michael's sign-off; Michael requested this work himself, so that sign-off is implicit.

Code Review Findings

1. 🔴 [Critical/Blocker] farEndHolders counts other schemas' same-named relationships as holders, which produces false reassignment warnings
📁 packages/desktop-app/src/lib/services/relationship-grouping.ts (farEndHolders, ~L343-361)

The mirror match uses only relationshipName, the opposite direction, and cardinality === 'one'. get_node_relationships emits one inbound group per declaring schema, and several schemas can use the same name. The seeded core schemas already do this:

• person.tasks → task: reverse_name: "assignee", reverse_cardinality: One
• project.tasks → task: reverse_name: "project", reverse_cardinality: One

A task therefore has two inbound tasks groups, both with cardinality: 'one'. Consider a user who opens Bob's modal and links a task that belongs to Project P and is assigned to Alice. farEndHolders flattens both groups, and the prompt reads "Ship it" is currently linked to "Alice", "Project P". Linking it here removes that link. The backend never removes the project edge. The reverse-eviction loop in create_relationship_in_tx keeps only sources where type_satisfies(existing_source_type, declaring_type), so a project source is skipped. Worse, an unassigned task that belongs to a project raises a false warning on every link. The reverse case (linking from a project's modal) wrongly names the assignee in the same way.

This is the one thing the confirmation has to get right. A warning that fires on ordinary links teaches users to click through it (alarm fatigue), which defeats the purpose of the check.

Fix: scope the mirror to the same declaration. The smallest change is to also require mirror.reverseName === group.reverseName, which already separates assignee from project. A more robust fix is to have the backend emit the declaring schema on each group, so the frontend can require mirror.sourceType === declaring type. Today an outbound group's source_type is the concrete node type, which can be a subtype, so it cannot be compared for equality with the inbound group's declarer. Either way, add a unit test with two same-named inbound groups from different declarers (the person/project tasks case). The current "ignores same-direction and differently named groups" test does not cover this. Principle: the frontend's prediction must mirror the backend's actual eviction predicate, one source of truth for the rule.

2. 🟡 [Improvement] Evicted edges are reported in stored (forward) orientation, but the CLI's "Created" line repeats what the caller typed
📁 packages/core/src/services/node_service/relationship.rs L1308-1334; packages/cli/src/commands/relationship.rs L104-118

A write made through an in declaration's name is rewritten to the forward spelling by forward_endpoints, so ReplacedEdge uses the forward name and orientation. CreateRelOutput, however, repeats the input source_id/relationship_name/target_id. For an in write, the CLI then prints Created: A --[in_name]--> B next to Replaced: C --[forward_name]--> A, and a reader will struggle to match the two lines up. Either document on ReplacedEdge that it is always in stored forward orientation, or return the stored (forward) endpoints on CreateRelOutput as well so both lines use the same terms. Principle: principle of least astonishment and a consistent API contract.

3. 🟡 [Improvement] The frontend discards the actual replaced result after the write
📁 packages/desktop-app/src/lib/services/relationship-viewer-service.ts (addEdge)

The confirmation is a pre-write prediction, which is a TOCTOU window: another client can assign the candidate between the check and the write. addEdge returns the edges that were really evicted, but both callers ignore them apart from a log.info. A cheap safeguard is to show a brief notice when replaced contains an edge the user never confirmed. Not blocking. Principle: report what actually happened, not only what was predicted.

4. 🟢 [Nit] The Serialize + camelCase derive on ReplacedEdge is unused
📁 packages/core/src/services/node_service/relationship.rs L9-10

Nothing serializes ReplacedEdge directly: CreateRelOutput is not Serialize, and every surface maps the fields by hand. Drop the derive or use it, for example in the agent tool or the Tauri command, to avoid hand-copying the fields. As it stands, the derive implies a wire casing contract that nothing honors. Principle: YAGNI.

5. 🟢 [Nit] findDisplacedHolders loads the candidate's entire relationships view to check one group
📁 packages/desktop-app/src/lib/services/relationship-viewer-service.ts (findDisplacedHolders)

get_node_relationships runs one query per declared group, plus type-chain resolves, on every pick where the far end is one. This is fine at click-time scale. If it ever shows up in profiling, a targeted get_related_nodes(target, name, dir) call would do.

6. 🟢 [Nit] The agent's create_relationship tool description could mention replacement
📁 packages/agent/src/local_agent/tools.rs

The result note covers the model after the call. A single clause in the tool description ("on a single-valued end this replaces the existing link") would also help the model before it acts, for example to ask first. This is optional, and it should be measured before it becomes permanent (see the project's prompt-dilution findings).

Standards

No lint suppression, no backward-compat shims, and no issue numbers in code comments. createLogger is used, and window.confirm matches the existing pattern in conflicts-pane.svelte. The proto change is additive, and the dev-proxy mirrors the Tauri shape (204 → JSON { replaced }, which HttpAdapter.handleResponse handles).

Tests

The core eviction assertions (forward, reverse, idempotent), the agent tool test, the modal decline/confirm/no-holder/many-end paths, and the field decline/confirm paths are all good. The only gap is the shared-name-across-schemas case (finding 1).

Recommendation: REQUEST CHANGES. Fix finding 1 and add its test. Findings 2 and 3 are strongly recommended; the rest are optional.

[malibio]

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

[malibio]

Review addressed (e132428)

✅ 🔴 #1 farEndHolders over-matched same-named relationships — the mirror group is now matched on reverseName as well (added to RelationshipGroupView), mirroring the daemon's declaring-schema scoping. A task's project (project.tasks) is no longer reported as a holder when linking an assignee (person.tasks). Unit test added for two same-named inbound groups from different declarers.
✅ 🟡 #2 orientation — ReplacedEdge now documents that its endpoints are in stored forward orientation under the forward name.
✅ 🟢 #4 — dropped the unused Serialize derive.

⏭️ 🟡 #3 post-write notice for unpredicted evictions — skipped. It only matters when another client writes between the check and the click (sub-second), and the reload after the mutation shows the true state. Worth revisiting if multi-user editing arrives.
⏭️ 🟢 #5 lighter candidate lookup — skipped. It's a single fetch per click, triggered only when the far end is one.
⏭️ 🟢 #6 tool-description prose — skipped. Untested prompt additions have measurably hurt E4B before, and the in-result note covers the moment it matters.

Pre-push gate passed. Frontend relationship tests: 88 passed.

Re-review: not needed. The change is small and targeted: one added predicate, a new view-model field, a doc comment and a removed derive.