feat(reason): peer-aware graph walk (#690) - #712
Conversation
Reviewer's GuideImplements peer-aware BFS graph walking for Sequence diagram for peer-aware BFS expand_bfs walksequenceDiagram
participant Reason as reason
participant BFS as expand_bfs
participant Store as MemoryStore
participant PeerDB as peer_sqlite3_DB
Reason->>BFS: expand_bfs(store, seeds, seed_scopes)
Note over BFS: Seed belief_id has owning_scope=peerA
BFS->>Store: edges_from_in_scope(belief_id, "peerA")
Store->>Store: _peer_conn("peerA")
Store->>PeerDB: SELECT * FROM edges WHERE src = belief_id
PeerDB-->>Store: rows
Store-->>BFS: list[Edge]
loop for each Edge dst
BFS->>Store: get_belief_in_scope(dst, "peerA")
Store->>PeerDB: SELECT * FROM beliefs WHERE id = dst
PeerDB-->>Store: row | None
Store-->>BFS: Belief | None
BFS->>BFS: create ScoredHop(owning_scope="peerA")
end
BFS-->>Reason: list[ScoredHop] with owning_scope threaded
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughThis PR adds federation-aware graph traversal to the BFS belief walk: seeds can target peer graph edges via ChangesPeer-aware federation in BFS graph walk
Sequence Diagram(s)sequenceDiagram
participant Caller
participant expand_bfs
participant MemoryStore
participant Frontier
participant ScoredHop
Caller->>expand_bfs: seeds + seed_scopes (id → owning_scope)
expand_bfs->>Frontier: init entries with (belief_id, owning_scope)
loop frontier entry
expand_bfs->>MemoryStore: edges_from_in_scope(current_id, owning_scope)
MemoryStore-->>expand_bfs: edges[]
loop each edge
expand_bfs->>MemoryStore: get_belief_in_scope(edge.dst, owning_scope)
MemoryStore-->>expand_bfs: belief | None
alt belief found
expand_bfs->>ScoredHop: create(belief, owning_scope)
expand_bfs->>Frontier: enqueue(neighbor_id, owning_scope)
end
end
end
expand_bfs-->>Caller: list[ScoredHop(owning_scope set)]
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related issues
Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
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 |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
MemoryStore.edges_from_in_scopeandget_belief_in_scope, catching a blanketsqlite3.OperationalErrorand returning empty/None will silently mask SQL or schema bugs; consider narrowing the exception handling (e.g., checking for missing-table errors) or at least logging unexpected failures to aid debugging. edges_from_in_scopeandget_belief_in_scopeduplicate most of the SQL fromedges_from/get_belief; you could factor the common query/row-mapping logic into a small helper that takes aConnectionto keep these code paths consistent and reduce drift over time.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `MemoryStore.edges_from_in_scope` and `get_belief_in_scope`, catching a blanket `sqlite3.OperationalError` and returning empty/None will silently mask SQL or schema bugs; consider narrowing the exception handling (e.g., checking for missing-table errors) or at least logging unexpected failures to aid debugging.
- `edges_from_in_scope` and `get_belief_in_scope` duplicate most of the SQL from `edges_from`/`get_belief`; you could factor the common query/row-mapping logic into a small helper that takes a `Connection` to keep these code paths consistent and reduce drift over time.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
[claim:review:noether:2026-05-12T02:49:12Z] |
|
Review verdict (noether): CODE-LGTM, one composition-gap to flag for follow-up. Verified
What's right
Composition gap (flag for follow-up, not a blocker)
So the AC bullet "BFS walk in This looks intentional given the size-cap and the PR title scoping ("peer-aware graph walk", not "peer-aware aelf reason CLI"), and it's the natural seam to split — wiring the seed side requires either UNIONing peer hits into Recommend: approve and merge this PR as substrate, file a follow-up issue for the CLI seed-fetch wiring (could be ~50 LOC + an end-to-end CLI test that asserts Minor observations (non-blocking)
Approving and adding |
|
[release:review:noether:2026-05-12T02:57:55Z] |
|
merge-train: blocked FF push to The |
|
Filed the CLI seed-fetch wiring follow-up flagged in the review: #713 (rook-tier, ~50 LOC). Tracks the gap between the BFS-level peer walk this PR ships and the user-facing |
|
[claim:review:pascal:2026-05-12T04:27:37Z] |
CodeQL flagged the unused 'import pytest' at line 22. The tests use plain assert statements and don't reference pytest fixtures, parametrize, or raises; the import was leftover from an earlier draft. Resolves the one outstanding review thread on PR #712.
|
[release:review:pascal:2026-05-12T04:30:54Z] |
|
merge-train: blocked one or more commits between main and The |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/store.py`:
- Around line 3089-3094: The row mappers can raise IndexError when columns are
missing, so update edges_from_in_scope and get_belief_in_scope to catch
column-level schema drift: wrap the conversion of fetched rows (the list
comprehensions calling _row_to_edge and _row_to_belief) in a try/except that
catches IndexError (and optionally KeyError) and returns [] (the same graceful
degradation used for sqlite3.OperationalError); apply the same change to the
other occurrence that uses _row_to_belief (the block around lines 3114-3119) so
missing columns don't propagate exceptions.
🪄 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
Run ID: c0ac79ef-1547-45db-98bc-7a93e39b9512
⛔ Files ignored due to path filters (1)
CHANGELOG.mdis excluded by!**/CHANGELOG.md
📒 Files selected for processing (5)
src/aelfrice/bfs_multihop.pysrc/aelfrice/cli.pysrc/aelfrice/reason.pysrc/aelfrice/store.pytests/test_peer_aware_reason.py
|
merge-train: blocked one or more commits between main and The |
CodeQL flagged the unused 'import pytest' at line 22. The tests use plain assert statements and don't reference pytest fixtures, parametrize, or raises; the import was leftover from an earlier draft. Resolves the one outstanding review thread on PR #712.
06279c2 to
58f828b
Compare
CodeQL flagged the unused 'import pytest' at line 22. The tests use plain assert statements and don't reference pytest fixtures, parametrize, or raises; the import was leftover from an earlier draft. Resolves the one outstanding review thread on PR #712.
|
merge-train: blocked branch is not fast-forward on The |
… reads Add two read-only helpers on MemoryStore that route to the local DB when owning_scope is None and to the cached read-only peer connection otherwise. Foundation for the peer-aware BFS walk (#690): the expand_bfs traversal needs scope-routed edge reads and belief materialisation when a frontier entry lands on a foreign belief. Both helpers tolerate unreachable peers and schema-drift peer DBs (missing edges/beliefs tables) by returning [] / None rather than raising — federation is opportunistic per #661. Closes-AC-1 of #690.
ScoredHop gains owning_scope (None = local, peer name = federation scope owning the belief). expand_bfs takes an optional seed_scopes dict mapping seed belief_id → owning_scope; the scope propagates from each frontier entry to its children so a walk that enters a peer DB stays inside that peer's edge graph. Edge and belief reads route through the new MemoryStore.edges_from_in_scope / get_belief_in_scope helpers so an unreachable peer or schema-drift peer DB degrades gracefully (empty hops, no exception). Backwards compatible: seed_scopes=None preserves byte-identical output for pre-federation callers. Three new tests cover the two-scope acceptance smoke (peer 2-hop neighbour reaches the walk output with owning_scope='peerA', SUPPORTS chain depth=1/depth=2 ordering, scope='global' visibility), local-seed preservation (owning_scope=None propagates), and unreachable-peer tolerance. Closes-AC-1 of #690.
…ging (#690) SuggestedUpdate gains owning_scope (None = local, peer name when the belief lives in a foreign DB). suggested_updates() propagates the scope from each ScoredHop to its +1 row, and looks up impasse-locus belief scopes via a {bid: scope} map built from the hops list. Per the #661 read-only federation contract, the slash skill's close-the-loop writeback must skip foreign rows (mutations targeting foreign ids raise ForeignBeliefError at the API surface — feedback/lock/delete already enforce this). The owning_scope tag is the surface that lets the skill detect which rows to skip. One new test covers the mixed-scope case: a local +1 row carries owning_scope=None and a peer +1 row carries owning_scope='peerA'. Closes-AC-5 of #690.
…_scope JSON (#690) Human output: peer-owned hops prepend '[scope:<name>] ' to the belief.id line so readers can tell a hop crossed the federation boundary. Local hops (owning_scope=None) print unchanged byte-for-byte to preserve existing test fixtures. JSON output (--json): both .hops[] and .suggested_updates[] rows gain an owning_scope key (None for local, peer name for foreign). This is the wire surface the slash skill's close-the-loop writeback reads to skip foreign rows. Closes-AC-2 of #690.
CodeQL flagged the unused 'import pytest' at line 22. The tests use plain assert statements and don't reference pytest fixtures, parametrize, or raises; the import was leftover from an earlier draft. Resolves the one outstanding review thread on PR #712.
58f828b to
eb699e1
Compare
|
merge-train: merged eb699e1 → |
Closes #690.
Sub-task of #650 (read-only federation umbrella). Extends
aelf reasonto follow edges originating in peer DBs when the seed lands on a foreign belief, completing the search→reason peer-aware parity flagged as a follow-up in the #655 / #670 CHANGELOG entry.What ships
Five atomic commits, all signed (G):
feat(store)—edges_from_in_scope(src, owning_scope)andget_belief_in_scope(belief_id, owning_scope). Route to local DB whenowning_scope=None, to the cached read-only peer connection (_peer_conn) when a peer name is passed. Unreachable peer / schema drift →[]/None, never raises.feat(bfs)—ScoredHopgainsowning_scope: str | None = None.expand_bfstakesseed_scopes: dict[str, str | None] | Noneand threads the scope through the frontier so a walk that enters a peer stays in that peer's edge graph. Edge fetches useedges_from_in_scope; belief materialisation usesget_belief_in_scope.feat(reason)—SuggestedUpdategainsowning_scope.suggested_updates()propagates scope from eachScoredHopto its+1row and from a{bid: scope}map for impasse-locus rows. This is the surface the slash skill's close-the-loop writeback reads to skip foreign rows (mutations on foreign ids raiseForeignBeliefErrorper v3.0 design decision: federation write model — multi-writer CRDT vs read-only #661 / feat(federation): read-only mechanics — knowledge_deps.json + SQLite ATTACH + foreign-ID rejection (#650 sub-task) #655).feat(cli)—aelf reasonhuman output annotates peer hops with[scope:<name>]before the belief id.--jsonaddsowning_scopekeys onhops[]andsuggested_updates[].docs(changelog)— unreleased entry under[3.0.0].Acceptance bullets from #690
test_expand_bfs_follows_peer_edges_two_hopscovers a SUPPORTS chain depth=1/depth=2.scope:<name>annotation (human) andowning_scope(JSON) —aelf reasonCLI patch.BFS_EDGE_WEIGHTSis type-keyed, not scope-keyed). Not separately tested in this PR; the type-keyed nature of the weight table is the proof.derive_pathskeys onpath[-1] == EDGE_CONTRADICTS, not scope. Not separately tested.suggested_updatesflag foreign belief ids —test_suggested_updates_flags_foreign_ids.tests/test_peer_aware_reason.pycovering BFS walk + 4 covering store helpers + 1 covering SuggestedUpdate; 8 new tests total).What this does NOT do
aelf_reasontool: not present onmain; nothing to extend. If it ships later, mirror the sameowning_scopefield on hops/updates payloads.reason()integration: this PR ships the building blocks (expand_bfspeer-aware,SuggestedUpdatescope flag, CLI surface). The CLI_cmd_reasonstill callsretrieve()for seeds — whenretrieve()returns peer seeds (post-feat(federation): read-only mechanics — knowledge_deps.json + SQLite ATTACH + foreign-ID rejection (#650 sub-task) #655), the caller needs to build theseed_scopesdict fromfind_foreign_owner()per seed. That is a small wiring follow-up; this PR keeps_cmd_reasonunchanged to scope-limit the diff. The seed-wiring change is one-line in_cmd_reasonand can land in a follow-up PR once we have a runner that exercises it end-to-end.Verification
uv run pytest: 3703 passed, 59 skipped, 75 xfailed in 108s — clean run, no regression.github/main: clean.seed_scopes=Nonekeyword default produces byte-identicalexpand_bfsoutput to pre-feat(reason): peer-aware graph walk (#650 sub-task) #690 callers;owning_scope=Nonedefault onScoredHop/SuggestedUpdatemeans existing test fixtures continue to construct these dataclasses positionally without TypeError.Pre-requisites
Summary by Sourcery
Extend peer-aware reasoning so BFS graph walks and suggested updates correctly handle beliefs stored in peer databases while preserving pre-federation behaviour.
New Features:
aelf reasonCLI output (human and JSON) and annotate peer hops with a scope tag.Enhancements:
aelf reasongraph walk and federation behaviour in the unreleased changelog entry.Tests:
Summary by CodeRabbit
New Features
Tests