[mstomar125]

Summary

nodespace relationship get <id> --type <reverseName> --direction in resolved the reverse name to its declared forward form, then flipped the caller's --direction on top of that already-reversed name. Requesting --direction in on a reverse name therefore landed on the forward relationship's true outbound side, which structurally can never have edges for a normal (non-self-referential) declaration — so the call silently returned count: 0, exit 0, indistinguishable from "no edges exist".

Repro from the issue: Invoice.billed_to → Customer with reverseName: "invoices". --type billed_to --direction in and --type invoices (default out) both correctly returned 2; --type invoices --direction in incorrectly returned 0.

Root cause

packages/core/src/ops/rel_ops.rs::get_related_nodes resolves the supplied name via resolve_relationship_name, then for a ResolvedRelName::Reverse match, flipped input.direction ("in" → "out", else "in"). Since the default/no-flag path ("out" → flipped to "in") is the only one ever exercised in practice, this "flip the requested direction" design meant an explicit --direction in doubled the reversal instead of behaving as a no-op / intensifier, which is what a caller reasonably expects from a name that already reads as "the reverse".

Fix

A reverse name — a declared reverseName or a built-in's fixed inverse (child_of, etc.) — names exactly one traversal: the forward relationship, read from the target end. There is no second, meaningful direction it could mean, so --direction is now ignored once a name resolves as ResolvedRelName::Reverse: get_related_nodes always queries the resolved forward name with direction "in", regardless of what was requested. Every traversal previously reachable through the old flip is still reachable directly via the literal forward name (--type billed_to / --direction out), so nothing is lost — this was also already how packages/core/src/playbook/graph_resolver.rs's sibling resolution path (used by Play conditions) treated a reverse name, so this brings rel_ops::get_related_nodes in line with it.

This also fixes the identical double-reversal for a built-in's reverse spelling (e.g. child_of --direction in), which shares the same ResolvedRelName::Reverse code path.

Changes

• packages/core/src/ops/rel_ops.rs — drop the direction-flip for ResolvedRelName::Reverse; always resolve to (forward_name, "in"). Updated doc comments to explain why direction plays no part in this case.
• packages/core/tests/relationship_reverse_name_traversal_test.rs:
  • Updated reverse_name_flips_the_requested_direction (renamed reverse_name_ignores_the_requested_direction) — it previously asserted the buggy flip as intended behavior.
  • Added reverse_name_with_direction_in_matches_the_issue_repro — the exact Invoice/Customer repro from the issue, asserting all three spellings (billed_to --direction in, invoices, invoices --direction in) return the same 2 nodes.
  • Added builtin_reverse_name_ignores_the_requested_direction — the same fix for a built-in's reverse spelling.
  • Added self_referential_reverse_name_with_direction_in_does_not_return_the_wrong_edge — for a normal cross-type declaration the old bug always flipped to a structurally empty query, which doesn't prove much on its own; for a self-referential relationship the flipped query can be real and non-empty, so the old code would have silently returned a plausible-looking but wrong edge instead of a bare zero. Covers that case specifically.
• packages/skill/references/cli.md — corrected the --direction option docs and the "Traversing the reverse direction" section to state that --direction is ignored once --type resolves to a reverse name, with a worked example.

Test plan

• cargo test -p nodespace-core --test relationship_reverse_name_traversal_test — 14 passed (including the 4 new/updated regression tests)
• cargo test -p nodespace-core --test schema_relationship_declarations_test — 15 passed
• cargo test -p nodespace-core --test playbook_engine_integration_test --test relationship_editing_test --test core_task_link_relationships_test --test collection_membership_test --test object_valued_property_roundtrip_test — all passed (confirms the playbook graph-resolver path, which already used the correct fixed-direction behavior, is unaffected)
• bun run quality:fix clean (cargo fmt, cargo clippy --all-targets -D warnings -D clippy::unused-async)
• Full pre-push gate (test:all + cargo build --bin nodespaced + test:e2e) passed clean on push

Known downstream consequence (tracked separately)

packages/agent's get_related_nodes tool has a direction: "both" fan-out that queries "out" and "in" separately and concatenates with no dedup. That only worked for a reverse-resolved name by accident, as a side effect of the bug this PR fixes (the "in" call used to return near-always-empty results). After this fix both calls return the identical, correct result for a reverse-resolved name, so "both" now double-reports each related node. This can't be fixed inside rel_ops without breaking the CLI's own default (--direction defaults to "out", indistinguishable from an explicit "out" by the time it reaches rel_ops::get_related_nodes, and the issue's own acceptance criteria require the default-out/no-flag spelling to keep returning real data) — it needs to be fixed in packages/agent's own fan-out (dedup by node id, or don't fan out to both directions once a name resolves as reverse). Filed as #2823, unassigned — packages/agent is a separate ownership lane needing its own sign-off, out of scope here.

Closes #2791

🤖 Generated with Claude Code

[mstomar125]

Review response (/pragmatic-code-review findings, addressed via /address-review)

Addressed:

1. Malformed direction silently accepted for a Reverse-resolved name. get_related_nodes now validates direction is exactly "in" or "out" up front, before resolving the name or touching the DB, uniformly for every ResolvedRelName branch. Previously a Forward/Builtin/InboundForward name eventually surfaced an "Invalid direction" error downstream in NodeService::get_related_nodes, but a Reverse-resolved name computed its direction internally and never re-validated the caller's original value — so a typo ("In", "", "both") would have silently succeeded instead of erroring. (Confirmed via two independent review passes; also confirmed this is not a regression introduced by this PR's core fix — the prior flip logic had the identical "any non-in value collapses to a valid result" property, it just did so less directly.) New test: malformed_direction_errors_for_both_forward_and_reverse_names.
2. GetRelatedInput.direction doc comment didn't mention it's ignored for a Reverse-resolved name — only the get_related_nodes doc comment ~300 lines away explained this. Added a local note pointing there.
3. Test robustness note. Two tests compare Vec<Value> results from two separate get_related_nodes calls (related_nodes, related_nodes) with no ORDER BY guarantee from the store. Currently safe (both calls now run the literal identical resolved query), but added a comment explaining why, and pointing future extensions at the sorted-ids() pattern already used in reverse_name_with_direction_in_matches_the_issue_repro for multi-row comparisons.

Evaluated and skipped, with reasoning:

• packages/agent's "both"-direction fan-out double-counts for a Reverse-resolved name post-fix. Real, but already tracked separately as get_related_nodes 'both' direction fan-out double-counts for a reverse-resolved relationship name #2823 (unassigned) with the reasoning for why it can't be fixed inside rel_ops (the CLI's own --direction default is "out", indistinguishable at the wire level from an explicit "out", and relationship get returns an empty success when a reverseName is combined with --direction in, instead of the helpful error an undeclared name gets #2791 requires the default/no-flag spelling to keep returning real data) — it needs to be fixed in packages/agent's own fan-out logic, a separate ownership lane. Already noted in this PR's description.
• InboundForward still passes input.direction through unvalidated-by-name-context (asymmetry vs. graph_resolver.rs, which hardcodes "in" for its own InboundForward arm). Pre-existing, unrelated to this diff — rel_ops.rs's CLI-facing path has an explicit --direction flag with a documented default of "out"; graph_resolver.rs's path-segment traversal has no direction concept at all. This is deliberate divergence between two different consumer models, not a bug this PR introduced or that relationship get returns an empty success when a reverseName is combined with --direction in, instead of the helpful error an undeclared name gets #2791 is about.
• Extract a shared helper for the now-identical Reverse arm logic in rel_ops.rs and graph_resolver.rs. The duplication is one trivial, definitionally-fixed line each ((forward_name, "in", source_type) — by definition of what "reverse" means, not an arbitrary policy that could independently drift). Extracting an abstraction would touch the playbook engine (graph_resolver.rs) for marginal benefit — disproportionate to a bug-fix PR.
• Expose is_reverse on GetRelatedOutput / a shared "both directions" helper, to help a future "both"-style caller avoid the get_related_nodes 'both' direction fan-out double-counts for a reverse-resolved relationship name #2823 class of bug. Would require a proto/wire change (GetRelatedNodesResponse) to actually reach callers — out of scope for this PR; left as a consideration for whoever picks up get_related_nodes 'both' direction fan-out double-counts for a reverse-resolved relationship name #2823.
• Sequential vs. concurrent await in packages/agent's "both" fan-out. Entirely inside packages/agent (separate ownership lane, needs sign-off) and a performance nit, not correctness — not touched.
• Test schema-declaration boilerplate not factored into a shared helper (like create_adr_pair). Checked against the file: create_adr_pair is reused only by the tests sharing its exact Adr/Reviewer schema; the majority of this file's existing tests (including several pre-existing before this PR) already declare distinct schemas inline. Consistent with established convention, not a new inconsistency.

All 15 tests in relationship_reverse_name_traversal_test.rs pass (including 2 new for this response), bun run quality:fix clean, full pre-push gate clean on push.

Re-Review Decision

Decision: NO RE-REVIEW NEEDED

Rationale: The one behavioral change (direction validation) is small, additive, purely defensive (rejects previously-silently-accepted garbage input with a clear error; never changes behavior for any valid "in"/"out" input), covered by a new test that passes, and doesn't touch the core resolution logic that was the subject of the original review. The rest are a doc comment and two explanatory code comments. Low-risk, straightforward, no new design surface.