fix(feedback): atomic posterior write, one transaction, and a lock floor (#1168) - #1183
Merged
Merged
Conversation
Reviewer's GuideMakes feedback posterior updates atomic and transactional, enforces a lock floor for passive feedback, and aligns all feedback writers and docs with those invariants. Sequence diagram for deferred feedback sweeper honoring lock floor and ownershipsequenceDiagram
participant Sweeper as sweep_deferred_feedback
participant Store as MemoryStore
participant DB as SQLite
loop each row in deferred_feedback_queue WHERE status='pending'
Sweeper->>DB: SELECT * FROM deferred_feedback_queue
DB-->>Sweeper: row(belief_id, id, ...)
Sweeper->>Store: get_belief(belief_id)
alt no belief
Sweeper->>DB: UPDATE deferred_feedback_queue SET status='cancelled'
Sweeper->>Sweeper: skipped_no_belief++, cancelled++
else belief exists
alt belief.lock_level == LOCK_USER
Sweeper->>DB: UPDATE deferred_feedback_queue SET status='cancelled'
Sweeper->>Sweeper: skipped_locked++, cancelled++
else foreign belief
Sweeper->>Store: assert_local_ownership(belief_id)
Store-->>Sweeper: ValueError
Sweeper->>DB: UPDATE deferred_feedback_queue SET status='cancelled'
Sweeper->>Sweeper: skipped_foreign++, cancelled++
else eligible
Sweeper->>Store: apply_deferred_alpha_beta_update(...)
Sweeper->>DB: UPDATE deferred_feedback_queue SET status='applied'
Sweeper->>Sweeper: applied++, epsilon_used+=epsilon
end
end
end
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
robotrocketscience
added
the
attn:review
|
Warning Review limit reached
Next review available in: 33 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 (14)
📝 WalkthroughWalkthroughFeedback posterior updates now use atomic SQLite increments with audit-row consistency. Passive feedback respects user locks, deferred feedback skips locked or foreign beliefs, and CLI/MCP responses distinguish audited-but-unapplied outcomes. Confirm remains exempt from the lock floor. ChangesFeedback concurrency and lock behavior
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant apply_feedback
participant MemoryStore
participant FeedbackHistory
Client->>apply_feedback: submit feedback
apply_feedback->>MemoryStore: begin immediate transaction
apply_feedback->>MemoryStore: atomically bump posterior or skip for user lock
apply_feedback->>FeedbackHistory: record feedback event
MemoryStore-->>apply_feedback: updated posterior
apply_feedback-->>Client: feedback result and lock status
Possibly related issues
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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![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 3 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="tests/test_implicit_feedback.py" line_range="426-435" />
<code_context>
+def test_sweep_does_not_bump_a_locked_belief() -> None:
</code_context>
<issue_to_address>
**suggestion (testing):** Add a complementary test case for `skipped_foreign` and cancellation behaviour on federated beliefs in the sweeper.
The sweeper now differentiates `skipped_locked` vs `skipped_foreign` and cancels both to prevent backlogs, but only the lock-floor path is exercised by tests. Please add a test that enqueues a belief with a foreign `belief_id` (e.g., one that fails `assert_local_ownership`), and asserts that it is counted in `skipped_foreign`, its status becomes `cancelled`, it is removed from `enqueued`, and no posterior or `feedback_history` row is created.
Suggested implementation:
```python
def test_sweep_does_not_bump_a_locked_belief() -> None:
"""Hypothesis: a user lock is never bumped by the implicit lane.
The sweeper writes alpha directly, bypassing apply_feedback, so before
#1168 the retrieval-driven +epsilon landed on locks — which
docs/user/LIMITATIONS.md and PRIVACY.md both promise cannot happen.
Falsifiable by any alpha change, or by the row staying enqueued."""
locked = _mk("b1", "apple banana")
locked.lock_level = LOCK_USER
locked.alpha = 9.0
locked.beta = 0.5
def test_sweep_cancels_foreign_belief() -> None:
"""Sweeper should cancel federated / foreign beliefs instead of processing them.
The sweeper distinguishes skipped_locked from skipped_foreign and must cancel
both to avoid backlogs. A belief whose id fails assert_local_ownership should:
- be counted in skipped_foreign
- have its status set to 'cancelled'
- be removed from the enqueued queue
- not produce a posterior or feedback_history row.
"""
store = MemoryStore()
# Arrange: create a belief that is definitely "foreign" for this installation.
# The exact shape should match assert_local_ownership, e.g. a non-local prefix.
foreign_belief = _mk("foreign:remote:123", "apple banana")
foreign_belief.status = "enqueued"
foreign_belief.lock_level = None # not locked; rejected purely for foreign id
foreign_belief.alpha = 1.0
foreign_belief.beta = 1.0
# Persist the foreign belief in the store as enqueued work
store.put_belief(foreign_belief)
# Act: run the implicit feedback sweeper over the store
sweep_result = sweep_implicit_feedback(store)
# Assert: the sweeper recognised and skipped the foreign belief
assert sweep_result.skipped_foreign == 1
assert sweep_result.skipped_locked == 0
# Assert: the belief itself was cancelled and is no longer enqueued
updated = store.get_belief(foreign_belief.belief_id)
assert updated is not None
assert updated.status == "cancelled"
enqueued_ids = {b.belief_id for b in store.iter_enqueued_beliefs()}
assert foreign_belief.belief_id not in enqueued_ids
# Assert: no posterior row was created for the foreign belief
posterior = store.get_posterior(foreign_belief.belief_id)
assert posterior is None
# Assert: no feedback history was written for the foreign belief
history = list(store.get_feedback_history(foreign_belief.belief_id))
assert history == []
```
To integrate this test with your actual code, please align the helper and API calls:
1. Replace `sweep_implicit_feedback(store)` with the real sweeper entry point (e.g. `implicit_feedback_sweeper.store_sweep(store)` or whatever is currently used in `test_sweep_does_not_bump_a_locked_belief`).
2. Ensure `MemoryStore.put_belief`, `iter_enqueued_beliefs`, `get_posterior`, and `get_feedback_history` match the actual store interface. If the queue of enqueued beliefs is exposed differently (e.g. `store.enqueued`, `store.list_enqueued()`), adapt the assertions accordingly.
3. Adjust the foreign `belief_id` format `"foreign:remote:123"` to whatever your `assert_local_ownership` implementation actually rejects (e.g. a different hostname, shard id, or prefix). The important part is that this id fails the ownership assertion so the sweeper takes the `skipped_foreign` path.
4. If statuses are represented as enums or constants rather than strings, replace `'cancelled'` with the appropriate status value.
5. Mirror how the existing sweeper tests fetch the sweep result and inspect counters (e.g. if they access `metrics.skipped_foreign` or `result["skipped_foreign"]`, match that convention here).
</issue_to_address>
### Comment 2
<location path="tests/test_mcp_server.py" line_range="492-496" />
<code_context>
assert any(h["id"] == bid for h in hits["hits"])
+ # #1168 lock floor: passive feedback on a lock is audited, not applied.
fb = tool_feedback(store, belief_id=bid, signal="used")
- assert fb["new_alpha"] > fb["prior_alpha"]
+ assert fb["kind"] == "feedback.locked_not_applied"
+ assert fb["new_alpha"] == fb["prior_alpha"]
+ assert len(store.list_feedback_events(belief_id=bid)) == 1
dem = tool_demote(store, belief_id=bid)
</code_context>
<issue_to_address>
**suggestion (testing):** Consider asserting the `error` payload for the locked-not-applied MCP feedback response.
To better exercise the MCP contract, please also assert that the `error` field is present and matches the expected explanatory string (or at least a meaningful message), since downstream clients may branch on both `kind` and `error` for lock-floor handling.
```suggestion
# #1168 lock floor: passive feedback on a lock is audited, not applied.
fb = tool_feedback(store, belief_id=bid, signal="used")
assert fb["kind"] == "feedback.locked_not_applied"
# Ensure the MCP contract surfaces an explanatory error message for locked-not-applied feedback.
assert "error" in fb
assert isinstance(fb["error"], str)
assert fb["error"].strip() != ""
assert fb["new_alpha"] == fb["prior_alpha"]
assert len(store.list_feedback_events(belief_id=bid)) == 1
```
</issue_to_address>
### Comment 3
<location path="CHANGELOG/v4.md" line_range="20" />
<code_context>
+- **Feedback lost most of its evidence under concurrency, and moved locks it promised not to ([#1168](https://github.com/robotrocketscience/aelfrice/issues/1168)).** `apply_feedback` was an unsynchronised read-modify-write — `get_belief`, arithmetic in Python, then a whole-row `update_belief` writing the stale snapshot back — across two autocommit transactions. Concurrent hook processes each added their delta to the same alpha they had all read, so the last writer won: 8 threads × 15 events land 16 of 121 units of evidence on the pre-fix code while `feedback_history` records all 120, leaving the posterior that drives L1 rerank permanently inconsistent with its own append-only log. The write is now a new atomic `MemoryStore.bump_posterior` (`SET alpha = alpha + ?`) sharing one `BEGIN IMMEDIATE` transaction with its audit row, via a new `transaction(immediate=True)` mode. Dropping the whole-row write also closes a second defect: a feedback event could previously revert `lock_level` / `origin` written by a concurrent `aelf lock`. **Lock floor:** `apply_feedback` never read `lock_level`, so `aelf feedback <locked-id> harmful` moved a user lock — and sentiment-derived turn valence hit *every* lock on *every* prompt, since L0 locks are always injected and so sit in every turn's pending set. `LIMITATIONS.md` and `PRIVACY.md` both promised this could not happen. Passive feedback (CLI `feedback`, MCP `aelf_feedback`, sentiment, retrieval exposure, valence propagation) now records the event and holds the posterior; the CLI says so and points at `aelf unlock`, and MCP returns a distinct `feedback.locked_not_applied` kind so the no-op cannot read as success. `aelf confirm` / `aelf_confirm` are exempt — `COMMANDS.md` defines them as explicit affirmation rather than passive feedback. The deferred sweeper (`aelf sweep-feedback`), which writes alpha directly, now honours the same lock floor plus the [#655](https://github.com/robotrocketscience/aelfrice/issues/655) federation-ownership check, draining ineligible queue rows rather than letting them build a backlog; new `skipped_locked` / `skipped_foreign` counters report it. `PHILOSOPHY.md`'s "one writer of (α, β)" claim is corrected to name the three deliberate direct-write paths and state the invariants that actually hold across them.
</code_context>
<issue_to_address>
**issue (typo):** Example arithmetic in the concurrency description appears inconsistent.
The example currently mixes 120 and 121 units of evidence even though 8×15 = 120. Please either adjust the numbers or add a brief note explaining why 121 appears so the arithmetic is clear to readers.
```suggestion
- **Feedback lost most of its evidence under concurrency, and moved locks it promised not to ([#1168](https://github.com/robotrocketscience/aelfrice/issues/1168)).** `apply_feedback` was an unsynchronised read-modify-write — `get_belief`, arithmetic in Python, then a whole-row `update_belief` writing the stale snapshot back — across two autocommit transactions. Concurrent hook processes each added their delta to the same alpha they had all read, so the last writer won: 8 threads × 15 events land 16 of 120 units of evidence on the pre-fix code while `feedback_history` records all 120, leaving the posterior that drives L1 rerank permanently inconsistent with its own append-only log. The write is now a new atomic `MemoryStore.bump_posterior` (`SET alpha = alpha + ?`) sharing one `BEGIN IMMEDIATE` transaction with its audit row, via a new `transaction(immediate=True)` mode. Dropping the whole-row write also closes a second defect: a feedback event could previously revert `lock_level` / `origin` written by a concurrent `aelf lock`. **Lock floor:** `apply_feedback` never read `lock_level`, so `aelf feedback <locked-id> harmful` moved a user lock — and sentiment-derived turn valence hit *every* lock on *every* prompt, since L0 locks are always injected and so sit in every turn's pending set. `LIMITATIONS.md` and `PRIVACY.md` both promised this could not happen. Passive feedback (CLI `feedback`, MCP `aelf_feedback`, sentiment, retrieval exposure, valence propagation) now records the event and holds the posterior; the CLI says so and points at `aelf unlock`, and MCP returns a distinct `feedback.locked_not_applied` kind so the no-op cannot read as success. `aelf confirm` / `aelf_confirm` are exempt — `COMMANDS.md` defines them as explicit affirmation rather than passive feedback. The deferred sweeper (`aelf sweep-feedback`), which writes alpha directly, now honours the same lock floor plus the [#655](https://github.com/robotrocketscience/aelfrice/issues/655) federation-ownership check, draining ineligible queue rows rather than letting them build a backlog; new `skipped_locked` / `skipped_foreign` counters report it. `PHILOSOPHY.md`'s "one writer of (α, β)" claim is corrected to name the three deliberate direct-write paths and state the invariants that actually hold across them.
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/aelfrice/feedback.py (1)
179-238: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick winTOCTOU: lock-floor decision is made before the write lock is acquired.
store.get_belief(belief_id)(line 181) and thelocked/posterior_appliedderivation (lines 200-201) happen beforewith store.transaction(immediate=True):(line 211) takes the write lock.bump_posteriorcorrectly re-derivesprior_alpha/prior_betafrom its own atomic result, so the increment is safe — but the decision of whether to increment at all is not: if a concurrentaelf lockcommits between line 181 and line 211, this call still computeslocked=Falsefrom the stale read and proceeds to move the posterior of a now-locked belief, defeating the exact "movement of locked beliefs" invariant this PR is meant to close.Move the belief read and lock/posterior decision inside the transaction, after
BEGIN IMMEDIATEhas taken the write lock, so the decision and the write observe the same committed state.🔒 Proposed fix: decide inside the write-locked transaction
store.assert_local_ownership(belief_id) - b: Belief | None = store.get_belief(belief_id) - if b is None: - raise ValueError(f"belief not found: {belief_id}") - - # Lock floor (`#1168`). ... - locked: bool = respect_lock and b.lock_level == LOCK_USER - posterior_applied: bool = update_posterior and not locked - - prior_alpha: float = b.alpha - prior_beta: float = b.beta timestamp: str = now if now is not None else _utc_now_iso() - # One transaction for the posterior write and its audit row (`#1168`): ... + # One transaction for the read-decide-write sequence (`#1168`): the lock + # floor decision must observe the same committed state as the write, + # or a lock committed between an earlier read and BEGIN IMMEDIATE is + # missed and the posterior still moves. with store.transaction(immediate=True): + b: Belief | None = store.get_belief(belief_id) + if b is None: + raise ValueError(f"belief not found: {belief_id}") + locked: bool = respect_lock and b.lock_level == LOCK_USER + posterior_applied: bool = update_posterior and not locked + prior_alpha: float = b.alpha + prior_beta: float = b.beta + if posterior_applied: d_alpha, d_beta = _bayesian_delta(valence) bumped = store.bump_posterior(belief_id, d_alpha, d_beta)🤖 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/feedback.py` around lines 179 - 238, Move the get_belief lookup and the locked/posterior_applied calculation into the store.transaction(immediate=True) block, after the write lock is acquired. Keep the not-found validation and snapshot initialization inside that transaction so the lock-floor decision and bump_posterior operation use the same committed belief state; preserve the existing audit-only behavior for locked beliefs.
🤖 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 `@docs/concepts/PHILOSOPHY.md`:
- Around line 110-112: Update the opening sentence in the philosophy section to
scope the single-path claim specifically to feedback-driven runtime belief
mutation, rather than all runtime belief mutation. Keep the existing reference
to apply_feedback and the following explanation of additional write paths
unchanged.
In `@src/aelfrice/cli.py`:
- Around line 3514-3532: Update the feed event construction and _feed_log_event
call associated with the result.skipped_locked branch so locked feedback is
represented as not applied, using a distinct locked/not-applied event kind or
posterior_applied=False. Preserve feedback.applied only for the else branch
where the posterior actually changes, keeping the CLI output consistent with the
emitted event.
In `@src/aelfrice/deferred_feedback.py`:
- Around line 347-371: The deferred-feedback processing flow must acquire BEGIN
IMMEDIATE before reloading the belief and evaluating LOCK_USER or
local-ownership eligibility. Move the lock and ownership checks inside that
transaction, and commit cancellation updates there; keep the posterior write in
the same transaction so eligibility and UPDATE beliefs are atomic.
In `@src/aelfrice/mcp_server.py`:
- Around line 749-758: Update the aelf_feedback docstring’s documented return
contract to include the feedback.locked_not_applied response kind and its error
field, alongside the existing feedback.applied, feedback.bad_signal, and
feedback.unknown_belief kinds. Keep the implementation in tool_feedback
unchanged.
In `@tests/test_mcp_server.py`:
- Around line 492-496: Extend the assertions in the lock-floor regression test
around tool_feedback so the returned beta value is also equal to its prior_beta
value. Keep the existing alpha and feedback-event assertions unchanged, ensuring
both posterior parameters are verified as unchanged.
---
Outside diff comments:
In `@src/aelfrice/feedback.py`:
- Around line 179-238: Move the get_belief lookup and the
locked/posterior_applied calculation into the store.transaction(immediate=True)
block, after the write lock is acquired. Keep the not-found validation and
snapshot initialization inside that transaction so the lock-floor decision and
bump_posterior operation use the same committed belief state; preserve the
existing audit-only behavior for locked beliefs.
🪄 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: f182d60f-295e-454d-8b77-ca0d97e0d671
📒 Files selected for processing (14)
CHANGELOG/v4.mddocs/concepts/PHILOSOPHY.mddocs/user/COMMANDS.mddocs/user/LIMITATIONS.mddocs/user/PRIVACY.mdsrc/aelfrice/cli.pysrc/aelfrice/deferred_feedback.pysrc/aelfrice/feedback.pysrc/aelfrice/mcp_server.pysrc/aelfrice/store.pytests/test_cli.pytests/test_feedback_atomicity.pytests/test_implicit_feedback.pytests/test_mcp_server.py
Merged
5 tasks
robotrocketscience
added a commit
that referenced
this pull request
Jul 29, 2026
Bot review on PR #1183 surfaced two real defects in the previous commits. The sweeper's lock and ownership checks ran before its `BEGIN IMMEDIATE`, so a lock landing between the check and the direct `UPDATE beliefs SET alpha` would move a belief the checks had just rejected — the same check-then-act class this issue is about. The transaction now opens first and the eligibility reads happen inside it. A statement-order test pins the structure and fails on the old ordering. The CLI printed "recorded but did not move it" while `_feed_log_event` still logged `feedback.applied`, so `aelf feed` reported a posterior move that never happened. It now emits `feedback.locked_not_applied` and carries `posterior_applied`. Also: document the new `feedback.locked_not_applied` kind in the `aelf_feedback` return contract; PHILOSOPHY.md's first sentence still read as an absolute single-writer claim two lines above the correction; correct the changelog's 120-vs-121 arithmetic (120 events, beta 1.0 -> 121.0). Test coverage from the same review: `skipped_foreign` and its cancellation, the MCP `error` payload, and the beta half of the lock-floor invariant. CodeQL: drop an import left unused by an earlier rewrite, narrow a worker `except BaseException` to `Exception`, and restructure a `pytest.fail` that made the following lines unreachable.
Owner
Author
|
Addressed the review in Sweeper TOCTOU ( Feed event mislabelled ( Also taken:
CodeQL: dropped an import left unused by an earlier rewrite, narrowed a worker Full suite: 6105 passed, 69 skipped, 75 xfailed. |
Owner
Author
|
Operator decision, 2026-07-29: the deferred lane keeps moving posteriors. This PR ships as-is on that point. The audit's fifth finding argued What the lane was genuinely missing — and what this PR adds — is the lock floor and the #655 federation-ownership check. Both now applied, with ineligible queue rows drained rather than left to accumulate. Not to be re-litigated without new evidence. |
5 tasks
robotrocketscience
added a commit
that referenced
this pull request
Jul 29, 2026
Bot review on PR #1183 surfaced two real defects in the previous commits. The sweeper's lock and ownership checks ran before its `BEGIN IMMEDIATE`, so a lock landing between the check and the direct `UPDATE beliefs SET alpha` would move a belief the checks had just rejected — the same check-then-act class this issue is about. The transaction now opens first and the eligibility reads happen inside it. A statement-order test pins the structure and fails on the old ordering. The CLI printed "recorded but did not move it" while `_feed_log_event` still logged `feedback.applied`, so `aelf feed` reported a posterior move that never happened. It now emits `feedback.locked_not_applied` and carries `posterior_applied`. Also: document the new `feedback.locked_not_applied` kind in the `aelf_feedback` return contract; PHILOSOPHY.md's first sentence still read as an absolute single-writer claim two lines above the correction; correct the changelog's 120-vs-121 arithmetic (120 events, beta 1.0 -> 121.0). Test coverage from the same review: `skipped_foreign` and its cancellation, the MCP `error` payload, and the beta half of the lock-floor invariant. CodeQL: drop an import left unused by an earlier rewrite, narrow a worker `except BaseException` to `Exception`, and restructure a `pytest.fail` that made the following lines unreachable.
robotrocketscience
force-pushed
the
fix/issue-1168-feedback-atomicity
branch
from
July 29, 2026 23:46
f6dce5d to
80e1315
Compare
Owner
Author
|
[claim:review:Setr:2026-07-30T17:33:02Z] |
Owner
Author
|
Approve on the merits. The diagnosis is right, the fix is right, and the lock-floor scoping is the right shape. Outstanding items are mechanical plus one genuine coverage gap in new code — details at the end. Verified
The race tests are a conjunction guard — neither half is individually pinnedWorth knowing before someone refactors this. The PR ships two independent mechanisms, and the tests only fail when both are removed: That is not a defect — each mechanism alone genuinely does prevent the lost update, so the redundancy is real defence rather than an illusion. And the PR's claim that "the race test fails against pre-fix code" is accurate, because pre-fix had neither. But it means a partial revert ships silently, and the PR argues the two mechanisms guard different things: Small thing to add, and it's the difference between "the tests confirm this works" and "the tests would notice if it stopped". Agreed on the deferred-sweeper gating questionDeclining to gate the implicit lane on That said, it deserves its own issue rather than living in a PR body, since the PR will close and the question won't. Outstanding before this can be labelled
I'd take 1, 3 and 4 as chores; 2 is worth doing properly. Re-ping me and I'll re-review promptly. |
robotrocketscience
added
attn:unblock
Owner
Author
|
[release:review:Setr:2026-07-30T17:36:25Z] |
Owner
Author
|
[claim:review:Setr:2026-07-30T18:13:36Z] |
Owner
Author
|
Rebased onto The rebase had a real semantic conflict worth flagging. #1184 landed if posterior_applied and propagate and _propagation_enabled():
prior_denom = prior_alpha + prior_beta
deltas = store.propagate_valence(
belief_id, valence,
src_confidence=(prior_alpha / prior_denom) if prior_denom > 0 else 0.0,
)The composition is what you'd want: a locked belief holds its posterior and does not propagate, so the held signal can't reach its neighbours by another route. And in that branch Sourcery, Sourcery, MCP CodeQL unreachable code — a false positive, now cleared structurally. CodeQL does not model Two of the three review items were requests for coverage that already shipped. Not a complaint about the bots — but worth recording, since acting on either would have added a redundant test that looks like new safety and isn't. |
robotrocketscience
added a commit
that referenced
this pull request
Jul 30, 2026
Bot review on PR #1183 surfaced two real defects in the previous commits. The sweeper's lock and ownership checks ran before its `BEGIN IMMEDIATE`, so a lock landing between the check and the direct `UPDATE beliefs SET alpha` would move a belief the checks had just rejected — the same check-then-act class this issue is about. The transaction now opens first and the eligibility reads happen inside it. A statement-order test pins the structure and fails on the old ordering. The CLI printed "recorded but did not move it" while `_feed_log_event` still logged `feedback.applied`, so `aelf feed` reported a posterior move that never happened. It now emits `feedback.locked_not_applied` and carries `posterior_applied`. Also: document the new `feedback.locked_not_applied` kind in the `aelf_feedback` return contract; PHILOSOPHY.md's first sentence still read as an absolute single-writer claim two lines above the correction; correct the changelog's 120-vs-121 arithmetic (120 events, beta 1.0 -> 121.0). Test coverage from the same review: `skipped_foreign` and its cancellation, the MCP `error` payload, and the beta half of the lock-floor invariant. CodeQL: drop an import left unused by an earlier rewrite, narrow a worker `except BaseException` to `Exception`, and restructure a `pytest.fail` that made the following lines unreachable.
robotrocketscience
force-pushed
the
fix/issue-1168-feedback-atomicity
branch
from
July 30, 2026 18:18
80e1315 to
8fb0bd1
Compare
robotrocketscience
added
ready-to-merge
Owner
Author
|
[release:review:Setr:2026-07-30T18:25:36Z] |
bump_posterior issues `SET alpha = alpha + ?, beta = beta + ?` so the increment is evaluated by SQLite against the committed row. update_belief cannot do this: it is a whole-row write of an in-memory snapshot, which makes every posterior move an unsynchronised read-modify-write and lets a stale snapshot clobber columns the caller never meant to touch. transaction(immediate=True) issues BEGIN IMMEDIATE so a block that reads and then writes something derived from that read takes the write lock up front and waits on busy_timeout instead of losing the upgrade race to SQLITE_BUSY. Ignored when nested or when a transaction is already open, so it is safe to pass unconditionally. Primitives only; no caller changes here.
… floor apply_feedback was an unsynchronised read-modify-write landing in a full-row update_belief, across two autocommit transactions. Two defects, one fix each. Lost updates: concurrent hook processes each added their delta to the same alpha they had all read, so the last writer won. 8 threads x 15 events land 16 of 121 units of evidence on the pre-fix code while feedback_history records all 120 — the projection and its own append-only log disagreed with no error signal. The write is now store.bump_posterior (atomic SQL increment), and it shares one BEGIN IMMEDIATE transaction with the audit row so the two can no longer diverge on a crash. Because the whole-row write is gone, a feedback event can no longer revert lock or provenance columns committed by a concurrent writer. Lock floor: apply_feedback never read lock_level, so `aelf feedback <locked-id> harmful` moved a user lock — and, worse, sentiment-derived turn valence hit every lock on every prompt, since L0 locks are always injected and so sit in every turn's pending set. docs/user/LIMITATIONS.md and docs/user/PRIVACY.md both promise passive feedback cannot move a lock. It now cannot: the event is still audited, the posterior holds, and the CLI says so and points at `aelf unlock`. MCP returns a distinct feedback.locked_not_applied kind so a caller cannot read the no-op as a silent success. `aelf confirm` / `aelf_confirm` pass respect_lock=False — COMMANDS.md defines them as explicit user affirmation, distinct from implicit feedback, and they keep their existing behaviour on locks. Two existing tests asserted feedback moving a locked posterior; both are rewritten to the documented behaviour. The new race tests set busy_timeout explicitly and carry their own wall-clock budget, so a regression fails on the conservation assertion rather than colliding with the suite-wide 5 s timeout that equals SQLite's default busy_timeout.
sweep_deferred_feedback writes alpha directly rather than going through apply_feedback. That stays — it owns its own per-row BEGIN IMMEDIATE and its queue-status bookkeeping — but it was also skipping the two invariants that endpoint enforces. A retrieval-driven +epsilon landed on user locks, which LIMITATIONS.md and PRIVACY.md both promise cannot happen, and on federated beliefs that are read-only through the local DB (#655). Both cases now drain the queue row instead of applying it, so an ineligible belief cannot accumulate a backlog that all lands at once if it later becomes eligible. New skipped_locked / skipped_foreign counters on SweepResult report it. Scope note: this does not gate the sweeper on the #1086 exposure-updates-posterior flag. The implicit lane predates #1086 (#191/#256) and is a materially different mechanism — a much smaller epsilon, behind a grace window that any explicit correction cancels — not the immediate exposure-as-endorsement bump #1086 removed. Whether the lane should exist at all is an operator call, not a bug fix.
PHILOSOPHY.md asserted "one writer of (α, β)". Three paths besides apply_feedback write it, each deliberately; the claim is replaced with the invariants that actually hold across all four (lock floor, federation read-only, an audit row per non-merge move, atomic increment). LIMITATIONS.md and PRIVACY.md promised passive feedback cannot move a lock while the code moved it; both now describe the enforced behaviour, name the signals it covers, and note the `aelf confirm` exemption. COMMANDS.md documents the no-op-on-lock outcome for `feedback` and the exemption for `confirm`.
Bot review on PR #1183 surfaced two real defects in the previous commits. The sweeper's lock and ownership checks ran before its `BEGIN IMMEDIATE`, so a lock landing between the check and the direct `UPDATE beliefs SET alpha` would move a belief the checks had just rejected — the same check-then-act class this issue is about. The transaction now opens first and the eligibility reads happen inside it. A statement-order test pins the structure and fails on the old ordering. The CLI printed "recorded but did not move it" while `_feed_log_event` still logged `feedback.applied`, so `aelf feed` reported a posterior move that never happened. It now emits `feedback.locked_not_applied` and carries `posterior_applied`. Also: document the new `feedback.locked_not_applied` kind in the `aelf_feedback` return contract; PHILOSOPHY.md's first sentence still read as an absolute single-writer claim two lines above the correction; correct the changelog's 120-vs-121 arithmetic (120 events, beta 1.0 -> 121.0). Test coverage from the same review: `skipped_foreign` and its cancellation, the MCP `error` payload, and the beta half of the lock-floor invariant. CodeQL: drop an import left unused by an earlier rewrite, narrow a worker `except BaseException` to `Exception`, and restructure a `pytest.fail` that made the following lines unreachable.
CodeQL reported every assertion after the transaction block as unreachable: it does not model `pytest.raises` as catching, so a bare `raise` inside the block is statically the end of the function. Raising through a helper keeps the test byte-identical in behaviour and clears the alert.
robotrocketscience
force-pushed
the
fix/issue-1168-feedback-atomicity
branch
from
July 30, 2026 18:33
8fb0bd1 to
954d85a
Compare
github-actions
Bot
removed
the
ready-to-merge
|
merge-train: merged 954d85a → |
3 tasks
This was referenced Jul 31, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1168.
Lost updates
apply_feedbackwas an unsynchronised read-modify-write —get_belief, arithmetic in Python, then a whole-rowupdate_beliefwriting the stale snapshot back — across two autocommit transactions withbusy_timeoutas the only concurrency control.Reproduced by the new race test against the pre-fix code (8 threads × 15 events on one belief):
The posterior that drives L1 rerank was permanently inconsistent with its own append-only log, with no error signal and no reconciliation path.
Fix: a new atomic
MemoryStore.bump_posteriorissuingSET alpha = alpha + ?, beta = beta + ?, sharing one transaction with the audit-row insert via a newtransaction(immediate=True)mode (BEGIN IMMEDIATE, so the write lock is taken up front and contention resolves as a wait rather than as a lost upgrade).Lock clobber
Because
update_beliefwrites every column from an in-memory snapshot, a feedback write landing on a pre-lock snapshot could revertlock_level/locked_at/origincommitted by a concurrentaelf lock.bump_posteriorwrites only the two posterior columns, so the window cannot exist.test_feedback_write_path_never_writes_lock_columnsproves this structurally rather than probabilistically — it traces the SQL the path emits and asserts nobeliefsUPDATE assigns any other column. That test also fails on the pre-fix code.Lock floor
apply_feedbacknever readlock_level.scoring.decay()had the floor, butdecay()is dead. So:aelf feedback <locked-id> harmfulmoved a user lock.sentiment_from_prose, L0 locks are injected on every prompt and so sit in every turn's pending set — ten "no"/"that's wrong" turns could drag a lock from μ=0.947 to μ=0.367.docs/user/LIMITATIONS.md:38anddocs/user/PRIVACY.md:78both promised this could not happen.Passive feedback now records the event and holds the posterior. Per your call on the scoping question:
aelf feedbackaelf unlock, exit 0aelf_feedbackkind: feedback.locked_not_appliedaelf confirm/aelf_confirmconfirmis exempt becauseCOMMANDS.md:30defines it as explicit user affirmation "distinct from ... implicit retrieval feedback". It passesrespect_lock=False; that is an explicit parameter, not a sniff of the user-supplied--sourcestring.Two existing tests asserted feedback moving a locked posterior (
test_cli.py::test_feedback_used_increments_alpha,test_mcp_server.py::test_end_to_end_lock_search_feedback_demote). Both are rewritten to the documented behaviour, and a positive test for the floor is added alongside.The three bypassing paths
deferred_feedback.sweep_deferred_feedbackBEGIN IMMEDIATEand queue bookkeeping, but now honours the lock floor and the #655 federation-ownership check, draining ineligible rows rather than letting them build a backlog. Newskipped_locked/skipped_foreigncounters.clamp_ghosts.clamp_ghost_alphalock_level='none'in its own WHERE clause and writes a reversingfeedback_historyrow; its module docstring covers the rest.PHILOSOPHY.md's "there is one writer of (α, β)" is replaced with the invariants that actually hold across all four writers.One thing I did NOT do — needs an operator call
The audit's fifth finding argues the deferred sweeper should be gated on the #1086
exposure_updates_posteriorflag (or thatis_enqueue_on_retrieve_enabledshould default to False), on the grounds that it re-instates the junk percolation #1086 removed.I did not do that, because I don't think it's a bug fix. The implicit lane shipped under #191/#256, before #1086, and it is a materially different mechanism: a much smaller epsilon (0.05 vs 1.0) behind a 30-minute grace window that any explicit correction or contradiction cancels. #1086 turned off the immediate exposure→posterior bump in
hook_search; it did not touch this lane. Whether the lane should exist at all — and whetheraelf sweep-feedbackshould become a posterior no-op by default — is a product decision, not something to fold into a concurrency fix. Flagging it rather than deciding it.The sixth finding (an
applied_to_posterior/d_alpha/d_betacolumn onfeedback_historyso the posterior becomes a genuine fold over the log) is a schema migration and belongs with the determinism umbrella #1157, whose first AC is making the log total. Not in scope here.Acceptance criteria
bump_posterior()issuingSET alpha = alpha + ?in a single statementfeedback_historyinsert in oneBEGIN IMMEDIATEapply_feedback; locked beliefs get an audit row and a warning, never a posterior moveVerification
lock_level =, and the sweeper test reportsapplied=1on a locked belief.busy_timeoutexplicitly (2000 ms) and carry their own@pytest.mark.timeout(30), so lock contention resolves as a wait and a regression fails on the conservation assertion — never colliding with the suite-wide 5 s default that equals SQLite'sbusy_timeout(AC5).Summary by Sourcery
Make posterior feedback updates atomic and transactional while enforcing a lock floor and updating surrounding surfaces and docs to match.
Bug Fixes:
Enhancements:
Documentation:
Tests:
Summary by CodeRabbit
Bug Fixes
Documentation