fix(store): a retired belief no longer gains evidence — get_belief filters valid_to (#1210) - #1214
Conversation
`get_belief` had no `valid_to` filter, so a soft-deleted belief stayed reachable by id even though the FTS prune kept it out of search. Everything downstream of it — BFS, `propagate_valence`, feedback, context assembly — still saw the tombstone. Measured consequence: one `aelf feedback` on a *neighbour* propagated into a retired belief, taking alpha 9.0 -> 9.9 and writing an audit row against it, invisibly. `aelf restore` then returned a belief at a posterior the user never endorsed. Propagation is on the default path, so this needed no flag to reach. `include_retired=False` is the new default; the opt-in is explicit and greppable at the callers that must address a tombstone: the id-collision guard in `insert_belief` (a tombstone still owns its primary key), the `retire`/`delete` lifecycle commands, the `lock` re-lock path, the migration existence checks, and the replay drift audit. `get_belief_in_scope` carries the flag through to its peer branch too — a retired belief in a peer store was as reachable through the walk as a local one. `spine_neighbors` opts in for the opposite reason: it already implements the correct policy itself — emit only active beliefs, but traverse *through* retired ones so a GC'd segment does not sever the chain. Taking the default collapsed that into the `is None` branch and dropped the rest of the chain. Test probes that exist to inspect a tombstone opt in likewise, and two comments asserting "get_belief does not filter on valid_to" are corrected. `soft_delete_belief`'s docstring claimed read-side queries already filtered `valid_to`; that was aspirational, and is now true. Closes #1210
Written against the invariant rather than the call sites, per the issue: with ~60 `get_belief` callers, a suite that pins each one individually says nothing about whether the sixty-first is safe. Distinguishing, not decorative — reverting the `valid_to` filter fails 6 of the 9, and reverting the `insert_belief` opt-in on its own fails the collision test with the UNIQUE constraint it exists to prevent. The three that survive a revert are the opt-in paths and the negative control, which should pass either way. The control matters: without `test_propagation_still_reaches_a_live _neighbour`, a bug that disabled propagation entirely would satisfy both "the retired belief gained no alpha" assertions on a store where nothing propagates to anything.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @robotrocketscience, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
robotrocketscience
added
the
attn:review
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (17)
📝 WalkthroughWalkthrough
ChangesRetired belief lifecycle
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Feedback caller
participant MemoryStore
participant spine_neighbors
participant Audit events
Feedback caller->>MemoryStore: Apply feedback to a live neighbour
MemoryStore->>spine_neighbors: Traverse related belief IDs
spine_neighbors->>MemoryStore: get_belief(id)
MemoryStore-->>spine_neighbors: Active belief or None for retired belief
MemoryStore->>Audit events: Record evidence only for active targets
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
Reviewer's GuideIntroduce lifecycle-aware filtering to get_belief so retired (soft-deleted) beliefs are excluded from default retrieval, with explicit include_retired opt-ins at a small set of lifecycle, audit, and migration call sites; ensure BFS, propagation, feedback, and peer-store traversal no longer surface retired beliefs while restore and tombstone-oriented operations continue to work, with tests asserting the invariant that retired beliefs cannot gain evidence and that required tombstone access paths remain functional. Sequence diagram for BFS peer traversal using lifecycle-aware get_belief_in_scopesequenceDiagram
participant BFSWalker
participant Store
participant PeerDB
BFSWalker->>Store: edges_from_in_scope(belief_id)
loop for each neighbour_id
Store->>Store: get_belief_in_scope(neighbour_id, owning_scope, include_retired=False)
alt owning_scope is None
Store->>Store: get_belief(neighbour_id, include_retired=False)
Store-->>BFSWalker: Belief or None (retired excluded by valid_to IS NULL)
else owning_scope is peer
Store->>PeerDB: SELECT * FROM beliefs WHERE id = neighbour_id AND valid_to IS NULL
PeerDB-->>Store: row or None (retired excluded)
Store-->>BFSWalker: Belief or None
end
end
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
[claim:review:garsecg:2026-07-30T20:17:50Z] |
Review: approvedVerified independently rather than taking the PR body's word for it. The distinguishing check reproduces. Reverting the one line that matters The 3 survivors are the two opt-in paths and the negative control, which Spot-checked the call sites that take the new default and could plausibly The Commits are atomic and signed. Discretion grep on added lines is clean. One adjacent gap — not this PR's, filing separately
Re-asserting a statement you previously retired is swallowed: Pre-existing, not introduced here, and outside #1210's scope (which is On the "noted, not fixed" item — Adding |
|
[claim:review:Toug:2026-07-30T20:22:06Z] |
robotrocketscience
added
ready-to-merge
|
[release:review:Toug:2026-07-30T20:22:11Z] |
github-actions
Bot
removed
the
ready-to-merge
|
merge-train: merged 87bf0b1 → |
1 participant
Closes #1210.
get_beliefhad novalid_tofilter.aelf retiresetsvalid_toand prunes the FTS row, so keyword search correctly stopped finding a retired belief — but everything reached by id rather than by search still saw it: BFS,propagate_valence, feedback, context assembly.Reproduced, and the consequence measured
A belief the user retired gained confidence and had an audit row written claiming evidence for it, while staying invisible in search — so nobody could see it happening.
aelf restorethen returned it at a posterior the user never endorsed. Propagation is on the default path; no flag was needed to reach this.After:
The one-line fix would have been wrong
The issue says so and it holds up: a blanket
AND valid_to IS NULLbreaksaelf restore, turning reversible curation into a one-way door — a worse defect than the one being fixed. So the filter is the default and the opt-in is explicit and greppable.include_retired=Trueat the callers that must address a tombstone:insert_beliefid-collision guard_cmd_delete_cmd_retirealready retiredbranch readsbelief.valid_tolockre-lock path (CLI + MCP)migrateexistence checksreplaydrift auditingest_logagainst the rows that existEverything else — ~50 sites across retrieval, BFS, propagation, feedback, promotion, classification, context assembly, wonder,
aelf core— takes the new default.Two of these are load-bearing rather than defensive, and both are pinned: reverting the
insert_beliefopt-in tripssqlite3.IntegrityError: UNIQUE constraint failed: beliefs.id, andtest_retire_already_retired_is_noopalready covers the retire one.get_belief_in_scopeneeded the filter in both branchesIts peer branch runs its own
SELECT * FROM beliefs WHERE id = ?. A retired belief in a peer store was as reachable through the walk as a local one, so filtering only the local branch would have left federated stores as a way back in.spine_neighborsopts in for the opposite reasonIt already implements the correct policy itself — emit only active beliefs, but traverse through retired ones so a GC'd segment does not sever the chain:
Taking the default collapsed that distinction into the
is Nonebranch and dropped the rest of the chain —test_neighbors_skip_but_continue_soft_deletedwent["b3"]->[]. That test caught it, andNonethere now means a genuinely dangling edge, which correctly stops the walk. Flagging it because it is the clearest case of the hazard the issue warned about: the blanket filter is wrong at sites that were already handling lifecycle correctly.Tests assert the invariant, not the call sites
Per the issue: with ~60 callers, a suite that pins each one says nothing about whether the sixty-first is safe.
tests/test_retired_belief_evidence_1210.pyasserts that a retired belief's posterior cannot move and that the callers which must reach a tombstone still can.Verified distinguishing rather than assumed: reverting the
valid_tofilter fails 6 of the 9. The 3 that survive are the opt-in paths and the negative control, which should pass either way. The control is deliberate — withouttest_propagation_still_reaches_a_live_neighbour, a bug that disabled propagation outright would satisfy both "gained no alpha" assertions on a store where nothing propagates to anything.Acceptance criteria
get_beliefexcludes soft-deleted beliefs by default.aelf restorestill works end to end (asserted, including re-entry into keyword search).feedback_historyrows via propagation from a neighbour.soft_delete_belief's docstring claim about read-side filtering is now true, and says where it is enforced.Out of scope per the issue: dangling-edge handling once a node is retired.
Test-side changes worth a look
Nine probes that exist to inspect a tombstone now pass
include_retired=True. Each was verified as a probe artifact rather than a product regression before being touched —aelf retirewas confirmed to still setvalid_tovia raw SQL while the test's own_valid_tohelper reportedNone. Two comments asserting "get_belief does not filter on valid_to" are corrected rather than left describing the old behaviour.Verification
scripts/check_migration_policy.pyagainstgithub/main:OK: no new migration entries(no schema change).Noted, not fixed
aelf lockon a retired statement leaves the belief retired but locked. That is unchanged by this PR — the opt-in preserves it rather than introducing it — but it looks like an unintended state. Separable from #1210; say the word and I will file it.Summary by Sourcery
Prevent retired beliefs from being treated as active content and ensure only explicit callers can access tombstones.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests:
Summary by CodeRabbit