fix(graph): propagate_valence attenuates by the broker, credits once, caps mass (#1169) - #1184
Conversation
Reviewer's GuideRefactors Sequence diagram for feedback application and valence propagationsequenceDiagram
actor User
participant feedback_apply_feedback as apply_feedback
participant store as Store
participant store_propagate_valence as propagate_valence
User ->> feedback_apply_feedback: apply_feedback(belief_id, valence)
feedback_apply_feedback ->> store_propagate_valence: propagate_valence(belief_id, valence, src_confidence)
store_propagate_valence ->> store: edges_from(src_id)
store_propagate_valence ->> store: get_belief(current_id)
store_propagate_valence ->> store: get_belief(edge.dst)
store_propagate_valence -->> feedback_apply_feedback: deltas
feedback_apply_feedback -->> User: posterior updated with deltas
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthroughValence propagation now uses source confidence, credits each reachable belief once via the shallowest strongest path, orders graph reads deterministically, and enforces a total evidence-mass budget. Feedback wiring and tests were updated for these semantics. ChangesValence propagation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Feedback
participant MemoryStore
participant Graph
participant Beliefs
Feedback->>MemoryStore: propagate valence with source confidence
MemoryStore->>Graph: traverse ordered edges by hop
Graph-->>MemoryStore: candidate paths
MemoryStore->>Beliefs: apply one budgeted delta per recipient
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
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 found 4 issues, and left some high level feedback:
- Inside
propagate_valencethe BFS now callsget_belieffor every broker and destination on every hop, which could become a bottleneck on large graphs; consider carrying broker confidence in the frontier tuple and/or prefetching beliefs for the current frontier to avoid repeated per-edge DB lookups.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Inside `propagate_valence` the BFS now calls `get_belief` for every broker and destination on every hop, which could become a bottleneck on large graphs; consider carrying broker confidence in the frontier tuple and/or prefetching beliefs for the current frontier to avoid repeated per-edge DB lookups.
## Individual Comments
### Comment 1
<location path="src/aelfrice/store.py" line_range="4972" />
<code_context>
+ """
cur = self._conn.execute(
- "SELECT * FROM edges WHERE src = ?", (src,)
+ "SELECT * FROM edges WHERE src = ? ORDER BY dst, type", (src,)
)
return [_row_to_edge(r) for r in cur.fetchall()]
</code_context>
<issue_to_address>
**suggestion (performance):** Consider indexing `(src, dst, type)` to keep the new ORDER BY from regressing edge-lookup performance.
On large `edges` tables, this `ORDER BY dst, type` can turn an index-only `WHERE src = ?` lookup into an expensive sort unless a matching composite index exists. Please ensure the schema defines something like `CREATE INDEX ... ON edges(src, dst, type)` so `edges_from` remains O(k) over outgoing edges rather than incurring a sort.
</issue_to_address>
### Comment 2
<location path="src/aelfrice/store.py" line_range="5371-5372" />
<code_context>
+ candidates.append((edge.dst, delta, hops + 1))
+
+ # Strongest path first; `dst` breaks ties into a total order.
+ candidates.sort(key=lambda c: (-abs(c[1]), c[0]))
+
+ next_frontier: list[tuple[str, float, int]] = []
</code_context>
<issue_to_address>
**suggestion (performance):** The global sort by |delta| per hop may be more expensive than necessary; consider a lighter-weight selection strategy.
This sorting approach is semantically correct, but it makes each hop an `O(n log n)` operation in the hot loop when the frontier is large. If only the top N candidates are needed before the mass cap applies, a partial selection (e.g. `heapq.nlargest` or a running threshold) could be cheaper. At minimum, consider benchmarking on large graphs to confirm this full sort doesn’t dominate `propagate_valence` runtime.
```suggestion
# Strongest path first; `dst` breaks ties into a total order.
#
# When the frontier is large, a full O(n log n) sort can dominate
# runtime. Pre-select the strongest K candidates, then sort just
# that reduced set to preserve the original ordering semantics.
if candidates:
import heapq # local import to avoid touching module imports
# Soft cap; tune as appropriate for typical frontier sizes.
max_candidates_per_hop = 4096
if len(candidates) > max_candidates_per_hop:
# Pre-select the strongest subset by |delta|.
candidates = heapq.nlargest(
max_candidates_per_hop,
candidates,
key=lambda c: abs(c[1]),
)
# Final deterministic ordering: strongest path first; `dst`
# breaks ties into a total order.
candidates.sort(key=lambda c: (-abs(c[1]), c[0]))
```
</issue_to_address>
### Comment 3
<location path="tests/test_propagate_valence.py" line_range="275-296" />
<code_context>
+ )
+
+
+def test_total_injected_mass_is_capped( # AC4
+) -> None:
+ """One event cannot inject unbounded evidence into a wide graph.
+
+ Falsifiable by the summed absolute delta exceeding the cap."""
+ s = MemoryStore(":memory:")
+ s.insert_belief(_mk("A", alpha=9.0, beta=1.0))
+ for i in range(40):
+ s.insert_belief(_mk(f"B{i:02d}", alpha=5.0, beta=5.0))
+ s.insert_edge(
+ Edge(src="A", dst=f"B{i:02d}", type=EDGE_SUPPORTS, weight=1.0)
+ )
+ out = s.propagate_valence("A", valence=1.0, max_hops=3,
+ min_threshold=0.0001)
+ total = sum(abs(v) for v in out.values())
+ assert total <= 1.0 * 3 + 1e-9, f"mass {total} exceeds the cap"
+ # The cap binds here, so not every neighbour is reached — but the ones
+ # that are still get a full-strength delta rather than a diluted one.
+ assert out, "cap swallowed every delivery"
+ assert all(abs(v) > 0.0 for v in out.values())
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding an explicit test for negative valence with a custom max_total_mass
The property test already covers negative valence with the default cap, and this test covers positive valence with a custom max_total_mass. To better exercise the override logic for negative events, please add a focused unit test that sets valence < 0 with a custom max_total_mass (smaller than abs(valence) * max_hops) and asserts that the total absolute mass is bounded by that explicit cap.
```suggestion
def test_total_injected_mass_is_capped( # AC4
) -> None:
"""One event cannot inject unbounded evidence into a wide graph.
Falsifiable by the summed absolute delta exceeding the cap."""
s = MemoryStore(":memory:")
s.insert_belief(_mk("A", alpha=9.0, beta=1.0))
for i in range(40):
s.insert_belief(_mk(f"B{i:02d}", alpha=5.0, beta=5.0))
s.insert_edge(
Edge(src="A", dst=f"B{i:02d}", type=EDGE_SUPPORTS, weight=1.0)
)
out = s.propagate_valence(
"A",
valence=1.0,
max_hops=3,
min_threshold=0.0001,
)
total = sum(abs(v) for v in out.values())
assert total <= 1.0 * 3 + 1e-9, f"mass {total} exceeds the cap"
# The cap binds here, so not every neighbour is reached — but the ones
# that are still get a full-strength delta rather than a diluted one.
assert out, "cap swallowed every delivery"
assert all(abs(v) > 0.0 for v in out.values())
def test_total_injected_mass_is_capped_for_negative_valence_with_override() -> None:
"""Negative events are also bounded by an explicit max_total_mass override.
The explicit cap should bind even when it is smaller than
abs(valence) * max_hops."""
s = MemoryStore(":memory:")
s.insert_belief(_mk("A", alpha=9.0, beta=1.0))
for i in range(40):
s.insert_belief(_mk(f"B{i:02d}", alpha=5.0, beta=5.0))
s.insert_edge(
Edge(src="A", dst=f"B{i:02d}", type=EDGE_SUPPORTS, weight=1.0)
)
valence = -1.0
max_hops = 3
max_total_mass = 1.0 # strictly less than abs(valence) * max_hops == 3.0
out = s.propagate_valence(
"A",
valence=valence,
max_hops=max_hops,
min_threshold=0.0001,
max_total_mass=max_total_mass,
)
total = sum(abs(v) for v in out.values())
# The explicit cap must bind the total absolute mass.
assert total <= max_total_mass + 1e-9, f"mass {total} exceeds explicit cap"
# The cap must not swallow every delivery, and the delivered mass
# should reflect the negative valence.
assert out, "cap swallowed every delivery"
assert all(v < 0.0 for v in out.values())
```
</issue_to_address>
### Comment 4
<location path="tests/test_propagate_valence.py" line_range="351-360" />
<code_context>
+ edges=st.lists(
</code_context>
<issue_to_address>
**question (testing):** Property test currently excludes self-loops; consider whether they should be in scope
The Hypothesis strategy currently filters out `p[0] == p[1]`, so these cases are never exercised. If self-loops can exist in real stores, it would be good to include them here to verify mass and the visited set logic. If they’re forbidden at the store level, adding a short comment documenting that assumption would clarify why the filter is present.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Addressed the review in
No sort step, so Per-hop Self-loops — good catch, now in scope. Negative valence with a custom Full suite: 6099 passed, 69 skipped, 75 xfailed. |
c076ebe to
ace8994
Compare
|
[claim:review:Setr:2026-07-30T17:19:21Z] |
|
Approve the fix; one documented property does not hold. All four defects are real and the fixes are right. But the docstring makes a claim about which path credits a belief that the code does not implement, and that matters more than usual here — because this PR's own defect #1 existed for exactly that reason. The claim that failsBoth the docstring and the PR body say each belief is credited "with the largest-magnitude path that reaches it." It isn't. It's credited at the shallowest hop that reaches it, by the strongest path within that hop. A longer path can carry more mass, and when it does, it loses. Magnitude is non-increasing along a single path (every Why I'm raising this rather than noting it: defect #1 in this very PR existed because the docstring said "broker confidence" while the code multiplied by the recipient's. The code was wrong for years and the docstring is what made it look right. Landing a new docstring that overstates what the walk does re-creates that failure mode one size smaller, in the same function, in the PR that fixes it. Either is fine by me:
What I'd rather not see is the current pairing. It'd also be worth stating that the credit-once design leans on The rest verifiedAll four defects are real, the diagnoses are right, and the tests are load-bearing. Mutations run in isolation:
One note on a guard that isn't distinguishing
Merge orderThis should land before #1188. Two reasons, and they point the same way:
This one also needs a rebase ( |
|
[release:review:Setr:2026-07-30T17:27:36Z] |
|
[claim:review:Setr:2026-07-30T18:03:23Z] |
… caps mass Four compounding defects in one BFS, all in the same walk. Attenuation direction. The multiplier was the *recipient's* posterior while the docstring called it broker confidence. The amount of evidence a belief received was therefore proportional to how confident it already was: rich-get-richer on positive signal, and low-confidence junk structurally shielded from negative signal because its small factor pushed the delta under min_threshold. It now uses the confidence of the belief the signal travels *through*. `apply_feedback` supplies the source's confidence as of before the event, so an event cannot fold its own increment into the strength of its own propagation — the two existing integration tests that pin exact deltas already encoded this reading and pass unchanged. Fan-in and cycles. Delivery accumulated per in-edge, outside the visited guard, so a diamond delivered 2x and a 5-way fan-in 5x the source signal — one `aelf confirm` on the root of a convergent subgraph could add alpha += 20 at the convergence point. The source guard also covered only the source, so any bidirectional edge pair between two non-source beliefs re-delivered to them. Each belief is now credited exactly once per walk, by its largest-magnitude path. Determinism. edges_from had no ORDER BY, so row order was physical insertion order — the issue measured a 3.3x difference in the delta delivered to one node purely from edge insertion sequence. edges_from (and its peer-scope sibling) now order by (dst, type), and within a hop candidates are ranked by (-abs(delta), dst) so the strongest path wins with ids breaking ties. Mass cap. Total injected mass now defaults to abs(valence) * max_hops. Candidates are taken strongest-first, so when the budget binds it drops the weakest paths rather than diluting every delta. Adds a hypothesis property test asserting, for any graph shape: mass stays within budget, the source is never a recipient, no single delta exceeds the source valence, and the result is invariant to edge insertion order. The diamond characterization test that documented the double-count is rewritten to the corrected magnitudes.
The mass cap skipped any delivery that would overrun the budget. With the default cap that is unobservable (a single delta can never exceed abs(valence), which is at most the cap), but an explicitly small max_total_mass swallowed the whole event: a budget of 0.5 against a first delta of 0.9 returned nothing. The boundary delivery is now clipped to the remaining budget, so a small budget scales the walk down instead of cancelling it, and the total-mass invariant still holds exactly. The property test filtered self-loops out. insert_edge accepts src == dst (no guard, and the PK permits it), so a real store can hold one and the walk has to handle it — dropped by the source guard on the source, by the visited set elsewhere. The strategy now generates them. Adds a negative-valence case for the explicit budget override, and records in the edges_from docstring why the new ORDER BY needs no additional index: edges is PRIMARY KEY (src, dst, type), so its autoindex already yields that order under WHERE src = ? — EXPLAIN QUERY PLAN shows a plain index search with no temp b-tree at ~600 outbound edges from one belief.
propagate_valence credits each belief once, at the shallowest hop that reaches it, by the strongest path within that hop. The docstring and the changelog both said 'largest-magnitude path', which is a different rule and not the one implemented: magnitude is non-increasing along a single path, but two paths of different lengths are not comparable that way, so a longer chain of strong edges can carry more than a short weak one. A -RELATES_TO(0.3)-> X delivers 0.27 <- taken A -SUPPORTS-> B -SUPPORTS-> X delivers 0.81 <- not taken Shortest-path-wins is the right rule to keep: crediting by the maximum over all paths would mean deferring every delivery until the walk completes, and hop distance is itself evidence of relatedness. So this corrects the description rather than the behaviour, and adds a test that fails if a deeper hop is ever allowed to overwrite a shallower credit. This matters more than a wording nit here: defect (1) of #1169 existed because the docstring named broker confidence while the code multiplied by the recipient's. Raised in review on #1184.
ace8994 to
4f5f866
Compare
|
Rebased onto The path-credit question — corrected the description, not the behaviour. Shortest-path-wins is the right rule to keep. Crediting by the maximum over all paths would mean deferring every delivery until the walk completes, and hop distance is itself evidence of relatedness. What was wrong was the docstring and the changelog both saying "largest-magnitude path", which is a different rule: Both now say what the walk does, and a new test fails if a deeper hop is ever allowed to overwrite a shallower credit (verified: letting deeper hops overwrite fails it, and also fails Sourcery, negative valence + custom Sourcery,
The frontier is bounded by the mass cap, so Labelling once CI settles. |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
tests/test_propagate_valence.py (2)
266-266: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNit:
EDGE_RELATES_TOis already imported at module scope (line 23).The function-local re-import is redundant now that the header import set was expanded.
♻️ Suggested cleanup
- from aelfrice.models import EDGE_RELATES_TO - forward = [🤖 Prompt for 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. In `@tests/test_propagate_valence.py` at line 266, Remove the redundant function-local EDGE_RELATES_TO import in the affected test, since it is already available from the module-scope import in tests/test_propagate_valence.py.
247-256: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: close the store in the helpers/tests that don't.
_diamond_in_edge_order,_build_chain-style helpers, and the mass-cap tests leave theMemoryStorehandle open, unlike the newer property/shallow-path tests which usetry/finally: s.close(). Consistent teardown (or a fixture) keeps connection handling uniform across the module.🤖 Prompt for 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. In `@tests/test_propagate_valence.py` around lines 247 - 256, Add consistent MemoryStore cleanup to _diamond_in_edge_order, _build_chain-style helpers, and mass-cap tests by ensuring each created store is closed in a finally block or through the module’s existing fixture pattern. Preserve each helper’s current propagation and assertion behavior while guaranteeing teardown on success and failure.src/aelfrice/store.py (1)
5580-5580: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: Ruff RUF002 flags the
αin this docstring.If RUF002 is enforced in CI, spelling it
alpha(as the changelog entry does) clears the warning without losing meaning.✏️ Suggested wording tweak
- the root of a convergent subgraph could add α += ~20 to the + the root of a convergent subgraph could add alpha += ~20 to the🤖 Prompt for 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. In `@src/aelfrice/store.py` at line 5580, Optionally update the docstring text around the convergent subgraph explanation to replace the ambiguous Greek character “α” with the ASCII term “alpha,” preserving the existing meaning and wording otherwise.Source: Linters/SAST tools
🤖 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.
Nitpick comments:
In `@src/aelfrice/store.py`:
- Line 5580: Optionally update the docstring text around the convergent subgraph
explanation to replace the ambiguous Greek character “α” with the ASCII term
“alpha,” preserving the existing meaning and wording otherwise.
In `@tests/test_propagate_valence.py`:
- Line 266: Remove the redundant function-local EDGE_RELATES_TO import in the
affected test, since it is already available from the module-scope import in
tests/test_propagate_valence.py.
- Around line 247-256: Add consistent MemoryStore cleanup to
_diamond_in_edge_order, _build_chain-style helpers, and mass-cap tests by
ensuring each created store is closed in a finally block or through the module’s
existing fixture pattern. Preserve each helper’s current propagation and
assertion behavior while guaranteeing teardown on success and failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 06edc647-fc3e-4b4d-ba17-d5d6469fd6f3
📒 Files selected for processing (4)
CHANGELOG/v4.mdsrc/aelfrice/feedback.pysrc/aelfrice/store.pytests/test_propagate_valence.py
|
Sourcery, self-loops in the property test — already addressed, in commit The strategy does not filter # Self-loops are NOT filtered: `insert_edge` accepts src == dst
# (no guard, and the PK permits it), so a real store can hold one
# and the walk must handle it — a self-loop on the source is
# dropped by the source guard, one elsewhere by the visited set.Which is the right answer to the question it raises: self-loops are not forbidden at the store level, so they belong in scope, and they are. No change needed. |
|
merge-train: merged 4f5f866 → |
…y when it isn't The three-round cap was the normal termination, not a backstop. At l1_limit=50 the rounds reach 200, so a query whose 200 strongest matches are all retired returned an empty pack while current ones sat below it: superseded/matching available returned (before) after 150/300 150 50 50 200/300 100 0 50 250/300 50 0 50 That is the starvation #1187 set out to remove, moved out 4x rather than removed — my bound, introduced while fixing the original. Worse, it truncated silently. Nothing distinguished 'the store holds two survivors' from 'I stopped after three rounds with a hundred unread', which is the silent-cap failure #1160 is open about, and which I argued against on #1184 before shipping it a PR later. The cap is now high enough that binding means something pathological, and binding traces to stderr naming the limit reached and how many survivors were found. The comment's 'strictly more than the pre-#1187 behaviour' claim is corrected: at 200/300 both returned 0, so it was 'at least as much'. Closes #1205.
Closes #1169.
Four compounding defects in one BFS —
store.propagate_valence, the walk a feedback event takes to reach related beliefs. All four are on a default-on path.1. Attenuation direction
The multiplier was the recipient's posterior while the docstring called it broker confidence. So the amount of evidence a belief received was proportional to how confident it already was:
min_thresholdof 0.05 as soon as any edge multiplier was below 0.625. Low-confidence junk was structurally immune to the signal meant to remove it.Now the multiplier is the confidence of the belief the signal travels through.
A detail worth flagging. With the direction corrected, the first hop's broker is the source — and
propagate_valenceruns afterapply_feedbackhas already bumped the source. Reading the confidence back off the row would fold the event's own increment into the strength of its own propagation, a weaker form of exactly the self-reinforcement #1058 set out to remove. Soapply_feedbacknow passes the source's confidence as of before the event, via a newsrc_confidenceparameter.I didn't pick that by preference — two existing integration tests pin exact downstream deltas (
test_propagated_deltas_match_pure_walk_exactly,test_negative_signal_through_contradicts_penalizes_neighbor), and they only pass under the prior-confidence reading. They pass unchanged.2. Fan-in and reconvergence double-count
applied[dst] += deltaran for every in-edge traversed, outside thevisitedguard, which only prevented re-queuing. So:The issue's worked case: one
aelf confirmon the root of a 20-node convergent subgraph added α += ~20 at the convergence point, pinning it at the top of the posterior-weighted rerank with no user having endorsed it.3. Cycles between non-source beliefs
The guard was
if edge.dst == src_id: continue— the source only. The docstring claimed cycles were dropped; true for the source, false for everything else.A→B, B→C, C→Bdelivered 2.0× to B. Any bidirectionalRELATES_TOpair (written in both directions bywonder_ingest) orCONTRADICTSpair inside the hop radius hit this on every event.2 and 3 have one fix: each belief is credited exactly once per walk, by its largest-magnitude path.
4. Edge-insertion-order dependence
edges_fromwasSELECT * FROM edges WHERE src = ?with noORDER BY, so row order was(src, rowid)— physical insertion order, which SQLite does not contract. The issue measured a 3.3× difference in the delta delivered to one node purely from insertingA-SUPPORTS->Bbefore or afterA-RELATES_TO->C. AVACUUM, a migration, or a different query plan moves it, which breaks the "retrieval is a reproducible function of the write log" contract.edges_fromand its peer-scope sibling nowORDER BY dst, type, and within a hop candidates rank by(-abs(delta), dst)— so the strongest path propagates onward regardless of storage layout, matching the precedent already inbfs_multihop.py.Mass cap (AC4)
Total injected mass now defaults to
abs(valence) * max_hops, the invariant the issue proposed. Candidates are considered strongest-first, so when the budget binds it drops the weakest paths rather than diluting every delta. Without a cap, mass grows with graph size — a property of the substrate, not of the user's single click.Acceptance criteria
visitedguardORDER BYonedges_fromfor a total orderVerification
propagate_valenceand all 9 fail — including the hypothesis property test.test_reconvergent_paths_accumulate_delivery_once_onwardwas a characterization test of the double-count (it asserted C=0.5). Rewritten to the corrected magnitudes and renamed.Note on ordering with #1168
PR #1183 (#1168) also touches
feedback.pyaround the propagation call site. The two are independent fixes but will conflict trivially there; whichever merges second needs a one-hunk rebase. Flagging so the reviewer isn't surprised.One cleanup I deliberately left out
tests/test_propagate_valence.py's module docstring onmainopens with a session identity. I rewrote it, then reverted: removing the literal puts it on a-line in the diff, which trips the pre-push discretion grep, and that gate is not one to override. It needs an operator call on how to land it — flagging rather than working around it.Summary by Sourcery
Correct valence propagation semantics, determinism, and mass bounding for feedback walks through the belief graph.
Bug Fixes:
Enhancements:
Documentation:
Tests:
Summary by CodeRabbit
Bug Fixes
Tests