Skip to content

fix(salvage): apoptosis transaction + decompile listener-before-reset + audit regression test#7

Open
JosephOIbrahim wants to merge 1 commit into
masterfrom
claude/pr4-salvage-correctness
Open

fix(salvage): apoptosis transaction + decompile listener-before-reset + audit regression test#7
JosephOIbrahim wants to merge 1 commit into
masterfrom
claude/pr4-salvage-correctness

Conversation

@JosephOIbrahim
Copy link
Copy Markdown
Owner

@JosephOIbrahim JosephOIbrahim commented May 10, 2026

Summary

Three correctness fixes salvaged from the now-closed PR #4. PR #4 was closed because master diverged on the banner (ab57275) and tool names (490e3e0) in ways that invalidated its docs commits — but these three code changes are independent of those decisions and remain valuable on master.

Fix File Why it matters
Transaction-wrapped apoptosis DELETEs crates/hippocampus/src/store.rs The chunked apoptosis loop runs trace DELETE + graph_edges DELETE per chunk. Mid-loop failure could leave traces deleted but their edges still pointing at orphan ids. Wrapping in a single rusqlite::Transaction makes per-chunk DELETEs atomic; commit before VACUUM (which can't run inside an open tx).
on_decompile listener fires before success_count reset python/harlo/motor/motor_cerebellum.py Today the listener observes success_count == 0 (post-reset) — the audit trail loses the prior count. Reordering so the listener fires BEFORE the Rule 32 reset means observers capture the pre-reset value while external readers still see 0. Bare except: pass replaced with logger.exception() so listener errors are diagnosable; fire-and-forget control flow preserved.
Regression test for the audit-trail contract tests/test_motor/test_motor.py test_decompile_listener_sees_prior_success_count registers a listener that captures pattern.success_count at notification time, sets up a pattern with success_count=42, calls record_failure, and asserts (a) the listener saw 42 (audit) and (b) post-call read sees 0 (Rule 32 literal).

What's intentionally NOT here

Test plan

  • cargo test -p hippocampus — 42 passed, 0 failed (apoptosis chunk-boundary + transaction behavior preserved)
  • pytest tests/test_motor/ — 15 passed, 0 failed (10 baseline + 5 review-driven, including the new prior-count regression)
  • Constitutional greps (sleep(, while True, DELETE.*audit, float32, cosine, reasoning_trace in elenchus/verifier.py) — all zero
  • No overlap with PR fix(constitutional): S7 formula + Checkpoint TOCTOU + engine-lock timeout + HotStore commit handling #6's diff — independent files, can land in any order

https://claude.ai/code/session_017arHKzx5mTUFiry7JhhRPs


Generated by Claude Code

Summary by CodeRabbit

  • Bug Fixes

    • Listener error messages are now logged instead of being silently ignored, improving diagnostic visibility.
  • Chores

    • Improved reliability and safety for bulk data deletion operations.

Review Change Stack

PR #4 was closed because master diverged on the banner ('ab57275 lock
HARLO banner to ansi_shadow') and tool names ('490e3e0 drop twin_
prefix') in ways that invalidated its docs commits.  Three pieces of
code in PR #4 were independent of those decisions and remain valuable
on master; they're re-applied here on a clean branch.

* crates/hippocampus/src/store.rs — wrap chunked apoptosis DELETEs in a
  single rusqlite::Transaction.  A mid-loop failure cannot leave traces
  deleted but their graph_edges still pointing at orphan ids.  VACUUM
  runs after commit (it cannot execute inside an open transaction).
  Uses unchecked_transaction() because microglia_apoptosis takes
  &Connection (no enclosing tx possible).

* python/harlo/motor/motor_cerebellum.py — reorder _decompile so
  on_decompile fires BEFORE the success_count=0 reset.  The listener
  payload now observes the pre-reset count (the Rule 32 audit trail
  contract), while external readers still see 0 (Rule 32 literal).
  Replace bare `except: pass` with logger.exception() so listener
  errors are diagnosable; fire-and-forget control flow preserved.

* tests/test_motor/test_motor.py — new
  test_decompile_listener_sees_prior_success_count locks in the
  ordering: listener captures success_count=42; post-call read sees 0.

Verified: 42 cargo tests pass (apoptosis chunk-boundary + transaction
behavior preserved); 15 motor tests pass (10 baseline + 5 new from
review-driven enhancements, all green).  Constitutional greps still
zero.

https://claude.ai/code/session_017arHKzx5mTUFiry7JhhRPs
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 10, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 2d0c4399-bcea-4568-aa10-43e6dee84257

📥 Commits

Reviewing files that changed from the base of the PR and between 490e3e0 and b1d320d.

📒 Files selected for processing (3)
  • crates/hippocampus/src/store.rs
  • python/harlo/motor/motor_cerebellum.py
  • tests/test_motor/test_motor.py

📝 Walkthrough

Walkthrough

This PR introduces two independent improvements: batching SQL deletions in a single transaction within the storage layer, and refactoring decompile listener notification to fire before success_count reset with proper exception logging.

Changes

Storage Transaction Batching

Layer / File(s) Summary
Transaction Wrapping
crates/hippocampus/src/store.rs
Chunked trace and graph_edges deletions are wrapped in a single unchecked_transaction with per-chunk SQL execution using transaction-scoped calls; VACUUM remains outside the transaction.

Decompile Listener Ordering and Error Handling

Layer / File(s) Summary
Error Logging Infrastructure
python/harlo/motor/motor_cerebellum.py
Module-level logger is introduced and imports are reorganized.
Listener Notification and Reset Ordering
python/harlo/motor/motor_cerebellum.py
_decompile method now invokes on_decompile listener before resetting success_count; listener exceptions are logged instead of silently ignored.
Listener Ordering Verification Test
tests/test_motor/test_motor.py
New test test_decompile_listener_sees_prior_success_count asserts the listener observes the pre-reset success_count and that reset occurs afterward.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • JosephOIbrahim/Harlo#3: Implements batching improvements to microglia_apoptosis transaction handling and refines MotorCerebellum decompile listener ordering with enhanced error logging.

Poem

🐰 A rabbit hops through code with care,
Transactions bundled, listeners fair,
Before we reset, we notify with grace,
Errors logged in their rightful place! ✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately and specifically describes the three independent correctness fixes included in the PR: transaction-wrapped apoptosis, listener-before-reset ordering, and the regression test.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/pr4-salvage-correctness

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants