fix(graph): traverse SUPERSEDES in reverse so BFS surfaces the replacement (#1170) - #1188
Conversation
|
Warning Review limit reached
Next review available in: 38 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughBFS expansion now reverse-traverses ChangesSUPERSEDES traversal semantics
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant BFS as expand_bfs
participant Store as MemoryStore
participant Belief as get_belief_in_scope
BFS->>Store: Read outbound edges
BFS->>Store: Read inbound SUPERSEDES edges
BFS->>Belief: Fetch unvisited neighbour
Belief-->>BFS: Return belief for next frontier
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Reviewer's GuideThis PR fixes BFS traversal of SUPERSEDES edges to align with the canonical producer direction (src=new → dst=old), adds reverse traversal support via inbound edges, introduces deterministic inbound edge ordering and scoped accessors, adjusts fixtures and tests to match production semantics, and updates the BFS design docs to record the intended behavior and valence/weight rationale. Sequence diagram for reverse SUPERSEDES traversal in BFSsequenceDiagram
participant expand_bfs
participant Store
participant Belief
Note over expand_bfs,Store: BFS at node old_id
expand_bfs->>Store: edges_from_in_scope(old_id, scope)
Store-->>expand_bfs: outbound_edges
expand_bfs->>Store: edges_to_in_scope(old_id, scope)
Store-->>expand_bfs: inbound_edges (including SUPERSEDES src=new_id -> dst=old_id)
Note over expand_bfs: Build neighbours:
Note over expand_bfs: outbound: (dst, type, weight)
Note over expand_bfs: inbound SUPERSEDES: (src, type, weight)
expand_bfs->>Belief: get_belief_in_scope(new_id, scope)
Belief-->>expand_bfs: belief(new_id)
expand_bfs-->expand_bfs: enqueue new_id with path [SUPERSEDES] and updated score
Note over expand_bfs,Store: SUPERSEDES only traversed via edges_to_in_scope (reverse) and not outbound
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
PR-size soft capThis PR is over the advisory size threshold:
Bigger PRs collide with more open work, which under the parallel-session workflow tends to produce repeated This is advisory only — nothing is blocked. If the size is intentional (large refactor, module removal, generated code), apply the |
robotrocketscience
added
the
attn:review
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The neighbour collection and ranking logic in
expand_bfshas grown fairly complex; consider extracting the reverse/outbound traversal normalization into a small helper function to make the BFS loop easier to read and reason about. - Both
edges_toandedges_to_in_scoperepeat the sameSELECT * FROM edges WHERE dst = ? ORDER BY src, typeSQL; factoring this into a shared helper or constant would reduce duplication and keep the ordering contract in one place.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The neighbour collection and ranking logic in `expand_bfs` has grown fairly complex; consider extracting the reverse/outbound traversal normalization into a small helper function to make the BFS loop easier to read and reason about.
- Both `edges_to` and `edges_to_in_scope` repeat the same `SELECT * FROM edges WHERE dst = ? ORDER BY src, type` SQL; factoring this into a shared helper or constant would reduce duplication and keep the ordering contract in one place.
## Individual Comments
### Comment 1
<location path="src/aelfrice/bfs_multihop.py" line_range="221-230" />
<code_context>
+ neighbours: list[tuple[str, str, float]] = [
</code_context>
<issue_to_address>
**issue (bug_risk):** Avoid processing the same neighbour_id multiple times when multiple edges target it
With the new `neighbours` abstraction, multiple edges (e.g. outbound and reverse inbound) can reference the same `neighbour_id`. Since `visited` is only checked when building `candidates` and not inside the `for neighbour_id, edge_type, _edge_weight in ranked` loop, distinct edges to the same node can all end up in `ranked`, producing multiple `ScoredHop`s and `next_frontier` entries for the same belief in one step.
To preserve BFS-style “at most one expansion per node per hop”, either dedupe `candidates` by `neighbour_id` before sorting (e.g. keep only the best-ranked edge per neighbour), or add a `if neighbour_id in visited: continue` guard inside the `ranked` loop as a second safeguard.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Good catch — fixed in
#1170 adds a second route in, exactly as you describe: an outbound edge and a reverse-traversed inbound one can now name the same neighbour. Both cases now have tests. Ranking is strongest-first, so the copy already taken is the higher-scoring one — asserted, so a future change that reorders ranking can't silently start returning the weaker edge's path. Full suite: 6147 passed, 69 skipped, 75 xfailed. |
Review catch on PR #1188. `candidates` was filtered against `visited` before ranking, but the emit loop never re-checked, so a belief named twice within one hop was returned twice and charged the node budget twice. This predates #1170 — the `(src, dst, type)` PK permits two edge types between one pair, and `A -SUPPORTS-> B` plus `A -CITES-> B` reproduces it on main. #1170 adds a second route in, since an outbound edge and a reverse-traversed inbound one can now name the same neighbour. Ranking is strongest-first, so the copy already taken is the higher-scoring one; both cases are covered by tests.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/aelfrice/bfs_multihop.py`:
- Around line 78-95: Update the expand_bfs docstring to document mixed
traversal: follow ordinary edge types outbound and REVERSE_TRAVERSED_EDGE_TYPES
inbound, using the resulting neighbour rather than the stored destination for
ranking. Replace references to outbound-only edges_from_in_scope behavior and
dst-based ordering while preserving the existing traversal semantics.
- Around line 234-257: Update the neighbour selection flow before the `ranked`
slice in the BFS hop logic: sort `candidates` by the existing ranking, retain
only the first entry for each `neighbour_id`, then apply `nodes_per_hop` to the
deduplicated results. Keep the existing budget checks and processing loop, but
ensure duplicate IDs no longer consume top-k slots or require the within-hop
`visited` skip.
In `@src/aelfrice/store.py`:
- Around line 5166-5171: Update the comment near edges_to to remove the
incorrect claim that edges_from orders its results, and describe edges_to’s
deterministic ORDER BY src, type rationale independently. Keep the explanation
about physical row order, the sort cost, and the primary-key/index ordering
unchanged where applicable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5d508cbf-d974-46c9-aea0-e9ffefd1152d
📒 Files selected for processing (5)
docs/design/bfs_multihop.mdsrc/aelfrice/bfs_multihop.pysrc/aelfrice/store.pytests/test_bfs_multihop.pytests/test_cli_reason_wonder.py
|
[claim:review:Setr:2026-07-30T17:11:58Z] |
|
Approve the direction fix — but two things should change before merge, one of them a live correctness gap that the bot flagged and I've now reproduced. The core work is excellent, and the evidence for it is the strongest kind. Confirmed independently:
1. The duplicate-neighbour slot consumption is real, and it fires on the normal caseCodeRabbit's What makes this worth fixing rather than noting is which configuration triggers it. That is not a contrived graph — it is the steady state At
2. The
|
robotrocketscience
added
attn:unblock
|
[release:review:Setr:2026-07-30T17:17:13Z] |
|
[claim:review:Setr:2026-07-30T18:10:19Z] |
Review catch on PR #1188. `candidates` was filtered against `visited` before ranking, but the emit loop never re-checked, so a belief named twice within one hop was returned twice and charged the node budget twice. This predates #1170 — the `(src, dst, type)` PK permits two edge types between one pair, and `A -SUPPORTS-> B` plus `A -CITES-> B` reproduces it on main. #1170 adds a second route in, since an outbound edge and a reverse-traversed inbound one can now name the same neighbour. Ranking is strongest-first, so the copy already taken is the higher-scoring one; both cases are covered by tests.
|
Rebased onto 1. Dedup now happens before the Candidates are ranked, then deduped by neighbour id keeping the strongest edge to each, then sliced — so New test 2. 3. The Labelling once CI settles. |
Two edges in one hop can name the same neighbour — different types between one pair are permitted by the (src, dst, type) PK, and since #1170 an outbound edge and a reverse-traversed inbound one can collide. Deduplicating only at delivery, after the top-k slice, let the duplicate occupy a slot and dropped an otherwise-eligible belief: LOSER -CONTRADICTS-> WINNER, WINNER -SUPERSEDES-> LOSER, LOSER -SUPPORTS-> OTHER nodes_per_hop=2 -> 1 hop (OTHER dropped) That fixture is not contrived. resolve_contradiction acts on an existing CONTRADICTS edge and inserts SUPERSEDES between the same pair, and those are the two highest weights in the table, so the duplicate reliably lands at the top of the ranking. Dedupe now happens after ranking and before the slice, keeping the strongest edge to each neighbour, so nodes_per_hop counts distinct beliefs rather than distinct edges. The delivery-side visited check stays as defence in depth. Also updates expand_bfs's contract, which still described an outbound-only walk after #1170 made it mixed. Raised by CodeRabbit on #1188.
6b3db59 to
0dbe395
Compare
robotrocketscience
added
ready-to-merge
|
[release:review:Setr:2026-07-30T18:25:40Z] |
|
merge-train: blocked branch is not fast-forward on The |
github-actions
Bot
removed
the
ready-to-merge
…ement
Producers store SUPERSEDES as src=winner(new) -> dst=loser(old):
`contradiction.resolve_contradiction` writes (winner.id, loser.id) and the
triple extractor parses "X supersedes Y" as src=X. That direction is what the
edge type's name means, so it is kept. The walk was wrong, not the data.
`expand_bfs` followed it outbound at the weight table's highest value (0.90),
which is the exact inverse of the rationale the memo gives for that weight
("if the query hit A, the user almost certainly wants B"). A hit on the
current belief pulled its stale predecessor into the prompt at maximum path
score; a hit on the stale one pulled nothing, so the case 0.90 was chosen for
never fired. SUPERSEDES is now read from the inbound side and is not followed
outbound — bidirectional traversal would have restored the bug while the new
tests still passed, so it is reverse-only and asserted as such.
Every SUPERSEDES fixture in the BFS suite was written src=old -> dst=new, the
reverse of production, which is why the suite never caught this: it was
testing a direction the product does not write. Flipping the fixtures to the
production direction leaves all 32 pre-existing assertions passing unchanged
— the walk's *semantics* (old -> new) were always right.
`test_cli_reason_wonder`'s shared fixture is split: `wonder`'s seed picker and
random-walk generator reason about outbound degree, which a
production-direction SUPERSEDES edge necessarily inverts, so they get a plain
outbound chain and the one test that exercises the SUPERSEDES hop gets its own
fixture.
`edges_to` gains `ORDER BY src, type` (the BFS frontier is
determinism-load-bearing) plus a peer-scope sibling for federation parity.
Docs: corrects the weight-table rationale and the temporal-coherence example
in docs/design/bfs_multihop.md, both of which described the reverse
traversal as if it already existed, and records that the SUPERSEDES gap
between EDGE_VALENCE (0.0) and BFS_EDGE_WEIGHTS (0.90) is intentional — the
two answer different questions, and valence never traverses the edge at all,
so the direction question does not arise there.
Review catch on PR #1188. `candidates` was filtered against `visited` before ranking, but the emit loop never re-checked, so a belief named twice within one hop was returned twice and charged the node budget twice. This predates #1170 — the `(src, dst, type)` PK permits two edge types between one pair, and `A -SUPPORTS-> B` plus `A -CITES-> B` reproduces it on main. #1170 adds a second route in, since an outbound edge and a reverse-traversed inbound one can now name the same neighbour. Ranking is strongest-first, so the copy already taken is the higher-scoring one; both cases are covered by tests.
Two edges in one hop can name the same neighbour — different types between one pair are permitted by the (src, dst, type) PK, and since #1170 an outbound edge and a reverse-traversed inbound one can collide. Deduplicating only at delivery, after the top-k slice, let the duplicate occupy a slot and dropped an otherwise-eligible belief: LOSER -CONTRADICTS-> WINNER, WINNER -SUPERSEDES-> LOSER, LOSER -SUPPORTS-> OTHER nodes_per_hop=2 -> 1 hop (OTHER dropped) That fixture is not contrived. resolve_contradiction acts on an existing CONTRADICTS edge and inserts SUPERSEDES between the same pair, and those are the two highest weights in the table, so the duplicate reliably lands at the top of the ranking. Dedupe now happens after ranking and before the slice, keeping the strongest edge to each neighbour, so nodes_per_hop counts distinct beliefs rather than distinct edges. The delivery-side visited check stays as defence in depth. Also updates expand_bfs's contract, which still described an outbound-only walk after #1170 made it mixed. Raised by CodeRabbit on #1188.
0dbe395 to
4ecac50
Compare
robotrocketscience
added
the
ready-to-merge
github-actions
Bot
removed
the
ready-to-merge
|
merge-train: merged 4ecac50 → |
1 participant
Closes #1170. The retrieval half (AC3) is split to #1187 — see Scope below.
The direction question (AC1)
Two conventions were in play, and they disagreed:
contradiction.resolve_contradiction(src=winner.id, dst=loser.id), triple extractor ("X supersedes Y" →src=X)Canonical direction: the producers'. It is what the edge type's name means, and keeping it needs no data migration; flipping the producers would leave
SUPERSEDESreading backwards and require migrating existing edges. So the walk was wrong, not the data.The bug (AC2)
expand_bfsfollowed SUPERSEDES outbound at the table's highest weight, 0.90 — the exact inverse of the rationale the memo gives for that weight:which needs an old → new hop that did not exist. Measured before the fix: BFS from the old belief returned
[]; BFS from the new belief returned[('old', 0.9, ['SUPERSEDES'])]. So a hit on the current belief spent prompt budget at maximum path score on the exact claim the supersession was recorded to retire, and the case 0.90 was chosen for never fired at all.SUPERSEDES is now read from the inbound side via
REVERSE_TRAVERSED_EDGE_TYPESand is not also followed outbound — bidirectional traversal would restore the bug while all the new tests still passed, so reverse-only is asserted explicitly.The finding that explains why this survived
Every SUPERSEDES fixture in the BFS suite was written
src=old → dst=new— the reverse of what production writes. The suite was testing a direction the product does not produce, which is why a high-severity inversion sat behind 32 green tests.Flipping those fixtures to the production direction leaves all 32 pre-existing assertions passing unchanged. That is the strongest evidence the change is right: the walk's semantics (old → new) were always what the suite asserted; only the stored direction was wrong.
tests/test_cli_reason_wonder.py's shared fixture needed splitting rather than flipping:wonder's seed picker and its random-walk phantom generator reason about outbound degree ("a has 1 outbound edge, b has 1, c has 0 — tie broken by id-asc", per the test's own comment), which a production-direction SUPERSEDES edge necessarily inverts. Those tests get a plain outbound chain; the one test that exercises the SUPERSEDES hop gets its own fixture.Two tables (AC4)
EDGE_VALENCE[SUPERSEDES] = 0.0vsBFS_EDGE_WEIGHTS[SUPERSEDES] = 0.90is the starkest gap between the tables. Reconciled as intentional, not drift — neither number moves:EDGE_VALENCE = 0.0answers "does a feedback signal cross this edge?" No — reinforcing a replacement says nothing about the confidence of what it replaced. Valence never traverses SUPERSEDES at all, so the direction question this PR fixes does not arise there.BFS_EDGE_WEIGHTS = 0.90answers "how relevant is the belief on the other end?" Maximally. This is the one that had to be reversed to match its own rationale.Recorded in
docs/design/bfs_multihop.mdso it isn't re-litigated.Also
edges_togainsORDER BY src, type— the BFS frontier is determinism-load-bearing and the raw row order is physical layout (same class as fix(graph):propagate_valenceattenuates by the recipient's own posterior, double-counts fan-in, and is insertion-order dependent #1169'sedges_fromfix). Unlikeedges_fromthis one does cost a sort, since the PK is(src, dst, type); the row count perdstis the in-degree, which is small. Noted in the docstring.edges_to_in_scopefor federation parity withedges_from_in_scope.Scope — AC3 is deliberately not here
AC3 ("exclude or demote superseded beliefs at retrieval") is filed as #1187 rather than included, because it is a different risk class:
is_bfs_enabled→ False; the v1.3.0 acceptance criterion is that a fresh install must not change retrieval output). No bench needed.retrieve()output on the default path. Per this repo's protocol that wants a bench before any flip, and the flip itself is an operator call — the established pattern is exactly howuse_entity_persist_demotelanded (Entity-persistence demotion prior: a deterministic organic sink for coordination junk (follow-up to #1086/#1081) #1096, graduated separately in epic(retrieval): converge production hook onto retrieve_v2 — staged lanes are not on the live path #1107).#1187 carries the reproduction (
retrieve()currently returns['old', 'new']— superseded first), the suggesteduse_supersession_demoteshape, and the open demote-vs-exclude design question. Two-axis dup check run before filing: no existing open issue, andapply_supersession_demotehas no importer onmain.Acceptance criteria
EDGE_VALENCEandBFS_EDGE_WEIGHTSfor this typeVerification
REVERSE_TRAVERSED_EDGE_TYPESemptied (i.e. the old outbound behaviour) and all 6 fail, including two pre-existing ones.Summary by Sourcery
Reverse BFS traversal of SUPERSEDES edges so queries on superseded beliefs surface their replacements, and align tests, storage helpers, and documentation with the canonical edge direction.
Enhancements:
Tests:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation