[malibio]

Closes #2823

Problem

The local agent's get_related_nodes tool implements direction: "both" (its default) as two rel_ops::get_related_nodes calls, out then in, concatenated. Since #2822, a reverse relationship name (declared reverseName, or a built-in inverse like child_of) resolves to one traversal regardless of direction, so both calls returned the identical set and every node was reported twice with a doubled count.

Fix

After each call, if rel_ops rewrote the name to its forward form (the signal that it resolved as a reverse name and ignored the requested direction), stop the fan-out. This skips the redundant query instead of running it and deduplicating. Forward and built-in names still fan out to both directions as before.

Tests

New packages/agent/tests/get_related_nodes_both_direction.rs drives the real GraphToolExecutor::execute against a real SqliteStore:

• a reverse name with direction omitted or both returns each of 2 invoices once. Without the fix this fails with count 4.
• a forward name (mentions) with both still returns the union of outbound and inbound hits.

Pre-push gate passed.

🤖 Generated with Claude Code

[malibio]

Pragmatic Code Review — PR #2947

Review Type: Initial Review

Requirements Check

• ✅ get_related_nodes with direction: "both" (or omitted) no longer double-reports nodes for a declared reverseName. The new test fails with count 4 without the fix.
• ✅ The fix lives in packages/agent (exec_get_related_nodes). rel_ops is unchanged, which keeps the CLI's default --direction out behaviour the issue protects.
• ✅ Forward and built-in names still fan out to both directions and union the results. The mentions test covers this.
• ✅ The redundant query is skipped instead of run and then deduplicated. This is the more precise option the issue suggested.
• ✅ Project standards: no issue numbers in comments, no lint suppression, no compatibility shims, and comments explain why.

Verification of the stop signal (output.relationship_name != rel_type)

I traced rel_ops::resolve_relationship_name and rel_ops::get_related_nodes:

• Builtin / Forward / InboundForward always pass input.relationship_name through unchanged. The signal can never fire for them, so they never lose their in leg. ✅
• Built-in Reverse (child_of, has_member, mentioned_by, role_of): in BUILTIN_RELATIONSHIPS, every inverse is spelled differently from its forward name, so the name is always rewritten and the signal fires. ✅
• Declared-schema Reverse: the name is rewritten to rel.name. This is correct unless reverseName == name. See finding 1.
• "out"-first ordering: the order doesn't affect correctness for Reverse, because rel_ops returns the same in traversal for either requested direction, so whichever leg runs first gives the full answer. Output labels are correct because each hit carries output.direction / output.relationship_name.

Code Review Findings

1. 🟡 A self-named reverse relationship still double-counts
📁 packages/agent/src/local_agent/tools.rs:3555-3563

Schema validation (reject_reserved_relationship_names in packages/core/src/schema/mod.rs:641) rejects reserved names. It does not reject reverseName == name. Here is a case that gets through:

• Invoice.related -> Customer, with reverseName: "related".
• On a customer node, Customer doesn't declare related itself, so the name resolves to Reverse { forward_name: "related" }.
• The returned relationship_name equals rel_type, so the loop doesn't break. Both legs return the same inbound set, and each hit is reported twice.

The data shape is narrow but legal. The bug is the one this PR fixes, reached through a spelling the signal can't see.

Principle: make the signal depend on the fact that actually matters. For a Reverse resolution, the direction that ran differs from the direction requested; whether the name happens to change is secondary. A Reverse always returns direction: "in". Builtin, Forward and InboundForward echo the requested direction. Since "out" runs first, comparing the direction catches every Reverse on the first leg, whatever the names are:

if output.relationship_name != rel_type || output.direction != *dir {
    break;
}

The direction check alone is enough, given the out-first order. Keeping the name check as well makes the intent clearer. If you adopt this, update the comment so it says the out-first order matters, and add a self-named-reverse case to the test.

2. 🟢 Implicit contract with rel_ops
📁 packages/agent/src/local_agent/tools.rs:3556-3563

The agent works out the resolution kind by comparing output fields to its input. That depends on an unwritten rel_ops behaviour ("a Reverse rewrites the name/direction"). A typed signal would make the contract explicit, for example resolved_as_reverse: bool on GetRelatedOutput. That would still keep all behaviour changes in packages/agent, since it adds a field and changes nothing in rel_ops. This is optional: the doc comment on rel_ops::get_related_nodes already documents the rewrite, and the test pins it. Worth it only if more callers start to depend on this.

3. 🟢 Test does not cover the built-in inverse path
📁 packages/agent/tests/get_related_nodes_both_direction.rs:99-119

The code comment and the test's module doc both name child_of as a case this fixes. Only the declared-reverseName path is tested. A built-in inverse goes through a different resolver branch (builtin_forward_name, source_type: None, so no type narrowing), so one more short case would pin it: create a parent with a child via has_child, then call get_related_nodes(child, "child_of") with no direction and assert count 1.

4. 🟢 Nit: assert the labels too
📁 packages/agent/tests/get_related_nodes_both_direction.rs:111-117

The reverse test could also assert that each hit has direction == "in" and relationship_type == "billed_to". That would lock in the "label with the traversal that actually ran" contract this loop relies on.

What's good

• A minimal, targeted change (9 lines) at the layer the issue named.
• The test drives the real GraphToolExecutor::execute against a real SqliteStore, not mocks, and includes a guard test that forward names still union both directions.
• The comment explains why the fan-out stops, not only what the code does.

Recommendation

APPROVE

This is a net improvement: it removes a model-visible regression for every reverse name except the narrow self-named case. Finding 1 is a one-line change that makes the fix complete, and I recommend including it before merge. Findings 2–4 are optional.

[malibio]

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

[malibio]

Address-review summary (406e8df)

✅ #1 🟡 same-name reverseName still double-counted: fixed. The fan-out now stops when the traversal ran in a different direction than the one requested (output.direction != *dir). A reverse name always runs in and out is queried first, so this covers every reverse resolution. The comment now states that the out-first order matters. A new test, reverse_name_equal_to_forward_name_reports_each_node_once, returns count 4 under the old name-based check and 2 now.
✅ #3 🟢 built-in inverse: added builtin_reverse_name_with_both_direction_reports_each_node_once (child_of).
✅ #4 🟢 labels: the reverse test now asserts direction == "in" and relationship_type == "billed_to" on every hit.
⏭️ #2 🟢 typed resolved-as-reverse flag on GetRelatedOutput: skipped. It would change the core rel_ops API, which is outside this agent-lane fix. rel_ops already documents that a reverse name ignores the requested direction, and the four tests here pin that. Worth revisiting if a second caller needs the same inference.

Tests: all 4 in get_related_nodes_both_direction pass, and the pre-push gate passed.

Re-review decision: NO RE-REVIEW NEEDED. The logic change is the reviewer's own suggested line, and a test was shown failing before it and passing after; the rest are test additions.