Skip to content

Sync agent reverse-traversal guidance with the direction fix (closes #2831) - #3193

Merged
malibio merged 1 commit into
mainfrom
issue-2831-reverse-direction-guidance
Sep 27, 2026
Merged

malibio merged 1 commit into
mainfrom
issue-2831-reverse-direction-guidance

Conversation

@malibio

@malibio malibio commented Sep 27, 2026

Copy link
Copy Markdown
Collaborator

Summary

Follow-up to #2822. The two packages/agent model-facing copies of the reverse-traversal guidance still implied direction matters for a reverse name; it is ignored.

  • skill_rules.rs RELATIONSHIP_REVERSE_TRAVERSAL: imperative and prose state a reverseName / built-in inverse names exactly one traversal, so direction is ignored; prose now matches references/cli.md.
  • tools.rs def_get_related_nodes: direction and relationship_type schema descriptions say the same.
  • Drift guard skill_md_key_phrase retargeted to "`--direction` has nothing to select" so this half of the rule is pinned to the shipped skill.

Text-only; no behavior change.

Test plan

  • bun run test:changed (scripts + Rust workspace nextest) passed
  • Frontend baseline 279 files / 5402 tests passed

Closes #2831

🤖 Generated with Claude Code

@malibio

malibio commented Sep 27, 2026

Copy link
Copy Markdown
Collaborator Author

Pragmatic Code Review: PR #3193

Review Type: Initial review

Requirements Check (#2831)

  • ✅ (1) RELATIONSHIP_REVERSE_TRAVERSAL updated. Both imperative and prose now say that a reverse name (a declared reverseName or a built-in inverse such as child_of) fixes the direction, so direction is ignored for it, and that direction only picks an end when it is paired with the forward name.
  • ✅ (2) def_get_related_nodes schema updated. The direction description explains in/out for forward names and says the argument is ignored for reverse or built-in-inverse names. relationship_type gets a matching sentence.
  • ✅ Consistent with cli.md. Each of the two prose paragraphs (after JSON-unescaping) is an exact substring of packages/skill/references/cli.md (checked programmatically), so the agent copy and the shipped skill reference cannot drift.
  • ✅ Drift guard. The new key phrase "`--direction` has nothing to select" appears in cli.md (line ~317). It was also moved onto the sentence that carries the new rule, so the guard now fails if that sentence is dropped. This fixes the weakness the issue called out, where the old phrase would survive a stale paragraph.

Accuracy Against Behavior

  • packages/core/src/ops/rel_ops.rs get_related_nodes: a ResolvedRelName::Reverse name is rewritten to (forward_name, "in") and input.direction is dropped, which matches "direction is ignored". resolve_relationship_name_for_type returns Reverse for builtin_forward_name (child_of) and for declared reverse names, so naming child_of as the example is correct.
  • packages/agent/src/local_agent/tools.rs exec_get_related_nodes: with both, it queries out first and breaks when the direction that ran differs from the requested one. A reverse name therefore runs one query, so "each related node is returned once" / "returns each ADR once" is correct. For forward names, both still fans out to out and in, which matches "only matters for a forward name".
  • The in/out semantics in the new schema text ("nodes whose edge points at this node" / "nodes this node's edge points to") match the store's direction convention.

Code Review Findings

🔴 Critical: none.

🟡 Improvements: none blocking.

🟢 Observations / Nits

  • 📁 packages/agent/src/skill_rules.rs:393-398, informational, pre-existing, out of scope. I could not find any path where RELATIONSHIP_REVERSE_TRAVERSAL.imperative is interpolated into a seeded skill or the live local-agent prompt. Nothing references it by name, unlike COLLECTION_AT_CREATE_TIME, which is pulled in through format placeholders. Only the INTERACTION_RULES iteration in guard tests (agent_guidance.rs, skill_pipeline.rs, tools.rs) reads it. So the model-facing effect of this PR comes from the def_get_related_nodes schema text, and the imperative is kept accurate for when it is wired in. This contradicts the issue's claim that the imperative is "injected verbatim", but the PR does not need to change anything for it. If wiring it in matters, fold that into a separate prompt-content decision, not this PR.
  • 📁 packages/agent/src/local_agent/tools.rs:1262, Nit: the direction description is long for a per-argument schema field (about 330 chars). It is justified here because this is exactly where the model needs the signal, so no change is suggested.
  • Grep sweep: no other stale copies of the old "equivalently decided_by with direction in"-only wording remain in packages/agent. The tool's top-level description (tools.rs:1247) says nothing about direction beyond (direction 'both') for forward use, which is still correct. Other reverseName mentions (skill_rules.rs:57-58, 181-182; playbook references) are about schema creation, not traversal direction.
  • No issue numbers were added to code comments or docs (the new doc comment on the rule states the constraint directly), which follows the repo's documentation rule.

Recommendation: APPROVE

This change only touches guidance text. It meets both acceptance items, matches the core and handler behavior, is byte-consistent with cli.md, and makes the drift guard stronger. Tests passed via test:changed.

@malibio malibio left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@malibio

malibio commented Sep 27, 2026

Copy link
Copy Markdown
Collaborator Author

Address-review summary

Review verdict: APPROVE — no 🔴/🟡 findings.

  • ✅ Addressed: 0 (nothing required)
  • ⏭️ Skipped (🟢 observation): RELATIONSHIP_REVERSE_TRAVERSAL.imperative is not interpolated into any live prompt or seeded skill. Confirmed — its only consumers are the guard corpora in agent_guidance.rs and skill_pipeline.rs tests. Pre-existing and out of scope here: wiring a new rule into the resident prompt changes agent behavior and needs an eval (resident-prompt additions have measured as net-negative before). The model-facing fix for this issue lands through the get_related_nodes tool schema, which the model does read.
  • ⏭️ Skipped (nit): direction description length — kept; it is the one place the model reads this at call time.

No new commits. No re-review needed.

…2831)

The RELATIONSHIP_REVERSE_TRAVERSAL rule and get_related_nodes's tool schema
still described `direction` as meaningful for a reverse name. Since the
relationship get double-reversal fix, a reverseName (or built-in inverse)
always traverses the forward edge inbound and `direction` is ignored.

- Rule imperative + prose now state direction is a no-op for reverse
  names; prose matches references/cli.md.
- get_related_nodes `direction` / `relationship_type` descriptions say the
  same at tool-call time.
- Drift guard key phrase retargeted to "`--direction` has nothing to
  select", the half of the rule that had drifted unnoticed.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@malibio
malibio force-pushed the issue-2831-reverse-direction-guidance branch from 24da826 to ac1c558 Compare September 27, 2026 23:31
@malibio
malibio merged commit 72c58d0 into main Sep 27, 2026
@malibio
malibio deleted the issue-2831-reverse-direction-guidance branch September 27, 2026 23:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

packages/agent guidance/schema text not updated for #2791's reverse+direction fix

1 participant