Skip to content

propose: neighbors OVERRIDDEN_BY.* dot-key traversal (#165)#186

Merged
HumanBean17 merged 2 commits into
masterfrom
plan/overridden-by-dot-key-traversal
May 20, 2026
Merged

propose: neighbors OVERRIDDEN_BY.* dot-key traversal (#165)#186
HumanBean17 merged 2 commits into
masterfrom
plan/overridden-by-dot-key-traversal

Conversation

@HumanBean17
Copy link
Copy Markdown
Owner

Summary

Test plan

  • Propose-only PR — no code changes
  • Reviewer approves locked decisions before implementation PR

Made with Cursor

Draft follow-up to landed DECLARES.* neighbors dot-keys: make override-axis
edge_summary keys navigable via stored OVERRIDES plus terminal rels.

Co-authored-by: Cursor <cursoragent@cursor.com>
Copy link
Copy Markdown
Owner Author

@HumanBean17 HumanBean17 left a comment

Choose a reason for hiding this comment

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

Critical review — propose-only PR

Verdict: Approve the propose with a few clarifications before the implementation PR. The core pivot (use stored [:OVERRIDES] instead of re-embedding IMPLEMENTS|EXTENDS + signature in the read path) is sound and already backed by existing tests (test_overrides_stored_neighbors_in_matches_override_axis_impl_ids).


What works well

  1. Same discoverability fix as #162/#171edge_summary advertises OVERRIDDEN_BY.* but neighbors rejects them today; the “what you see is what you can request” principle is consistent.

  2. Stored-edge traversal is the right call — Issue #165 originally framed this as embedding dispatch Cypher in neighbors. This propose correctly reverses that: override_axis_rollup_for keeps owning counts; traversal follows materialized [:OVERRIDES]. Virtual direction="out" on OVERRIDDEN_BY ↔ stored direction="in" on OVERRIDES from the declaration method is awkward but documented (locked decision #4) and matches current hint templates.

  3. Parallels landed DECLARES.* — registry in kuzu_queries.py, via_id on composed keys only, edge-row counts, edge_type echoes dot-key, no re-index / no ontology bump.

  4. Scope discipline — explicit out-of-scope, concrete test names, accurate supersedes list.


Please clarify or patch before implementation

1. Issue #165 body is stale (high)
The issue still says navigability requires signature-matching in the neighbors path. This propose’s point is that ontology 13+ [:OVERRIDES] makes that unnecessary. Please link this propose from #165 (or update the issue) when implementing so reviewers don’t re-litigate the old framing.

2. neighbors_v2 origin gate must split by axis (high)
Today all composed keys require a type Symbol (mcp_v2.py ~1738–1757). Implementation must branch DECLARES.* (type) vs OVERRIDDEN_BY.* (method). The propose says “partition by prefix” but not fail-fast vs partial success when mixing families on one origin (e.g. type + both DECLARES.* and OVERRIDDEN_BY.*). Recommend locking: reject the whole request if any requested composed key fails the origin gate, plus an explicit mixed-family test.

3. Rollup ↔ traversal equivalence must be mandatory (high)
edge_summary counts come from dispatch Cypher in override_axis_rollup_for, not from [:OVERRIDES]. The equivalence guard in the propose should be blocking for merge, not “keep or add”. Suggest: bank-chat requestAssignment + override_axis_rollup_smoke (diamond / middle-override if useful).

4. edge_filter interaction (medium)
#182 made edge_filter CALLS-only; composed keys are already rejected. Please add one line: no edge_filter on OVERRIDDEN_BY.* (same as DECLARES.*); terminal filtering is via NodeFilter only.

5. Base OVERRIDDEN_BY edge attrs (low)
Composed rows project terminal e.*; base key returns overrider methods via OVERRIDES but attrs semantics aren’t specified — lock whether attrs mirror flat OVERRIDES neighbors or stay minimal.

6. Hint test plan — success path (medium)
Grep sentinel for then neighbors(overrider_ids is good. Also add a positive hint test (mirror #171), e.g. expect neighbors(..., 'out', ['OVERRIDDEN_BY.DECLARES_CLIENT']) from describe-time templates. Note: test_hints_hv20_no_dotkey_edge_labels_in_rendered_neighbors_hints only applies to empty structural neighbors hints — not a blocker for success-path OVERRIDDEN_BY emissions.

7. Nits

  • “Ontology 13+” → repo is on 14 (cosmetic).
  • test_neighbors_overridden_by_dot_key_static_method_rejected — “if applicable”: either require a fixture or drop the name.

Merge recommendation

PR Action
#186 Approve & merge after optional propose tweaks above
Implementation Single PR per sequencing; move to propose/completed/ on land

Strong propose overall — main implementation risks are splitting the composed origin gate and treating rollup ↔ traversal parity as a hard invariant.

Lock axis-specific composed origin gates, merge-blocking rollup parity,
edge_filter and base-key attrs, expanded tests, and bank-chat parity example.

Co-authored-by: Cursor <cursoragent@cursor.com>
@HumanBean17 HumanBean17 merged commit dc1048a into master May 20, 2026
1 check passed
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.

1 participant