Skip to content

fix: remember_event atomicity + dead code cleanup (post-porting review)#4

Merged
marlian merged 3 commits intomainfrom
fix/remember-event-atomicity
Apr 14, 2026
Merged

fix: remember_event atomicity + dead code cleanup (post-porting review)#4
marlian merged 3 commits intomainfrom
fix/remember-event-atomicity

Conversation

@marlian
Copy link
Copy Markdown
Owner

@marlian marlian commented Apr 14, 2026

Summary

Addresses Diana post-porting review on 8dddddf (the Node→Go port that landed via PR #2). Findings:

  • R4 — atomic remember_event (state-concurrency). CreateEvent wrote the event row on *sql.DB directly, before the observation-loop transaction opened. Any mid-loop DB error (SQLITE_BUSY, FK violation, disk full) left an orphan event row with zero attached observations. Fix: CreateEvent now accepts dbtx; the handler opens a single transaction covering event insert + observation loop. Rollback discards the whole unit.
  • R1 — scanEntityRecord dead code removed from events.go. Defined but never called.
  • R2 — EventTypeFilter field removed from ToolArgs. Declared but never read — silent no-op contract drift for any JSON caller passing event_type_filter. The existing EventType field already serves recall_events.
  • R3 — real-client proof was archaeology. The binary is already wired as the MCP server backing Claude Code (via ~/.claude.json + governor env files), Kilo, and Codex. Live tool calls (remember_batch, recall_entity) succeeded during this very review session. Removing the stale P1 entry from OPERATIONS.md.

Invariant added to OPERATIONS.md: remember_event must be atomic — event row and all attached observations commit together or not at all.

Test plan

  • TestRememberEventAtomicityOnMidLoopFailure installs a BEFORE INSERT trigger on entities that RAISE(ABORT) on a sentinel value. Pre-fix: before=0 after=1 (orphan event row left behind). Post-fix: passes.
  • Full go test ./... suite green on darwin/arm64.
  • CI matrix (ubuntu / macos / windows) on this branch.

Commits

  1. fix: make remember_event atomic against mid-loop DB errors
  2. chore: remove dead code flagged by Diana review
  3. docs: close R3 real-client debt — binary is already in production

marlian added 2 commits April 14, 2026 20:01
Previously, CreateEvent wrote the event row directly on *sql.DB before the
observation-loop transaction was opened. Any DB error during the loop
(SQLITE_BUSY, FK violation, disk full) left an orphan event row with zero
attached observations.

CreateEvent now accepts the dbtx interface (already implemented by *sql.DB
and *sql.Tx). The remember_event handler opens a single transaction for
both the event insert and the observation loop; rollback on any error
discards the whole unit.

Proof: TestRememberEventAtomicityOnMidLoopFailure installs a BEFORE INSERT
trigger on entities that RAISE(ABORT) on a sentinel value. Pre-fix this
test reported "before=0 after=1 (orphan event row left behind)"; post-fix
it passes.

Invariant added to OPERATIONS.md.

Addresses Diana review finding R4 on commit 8dddddf.
- scanEntityRecord (events.go): defined but never called. Sat next to live
  scanEventRecord / scanEntityObservation helpers, implying a code path
  that does not exist. Go silently accepts unreferenced package-level
  functions, so this survived compilation.
- EventTypeFilter (tools.go ToolArgs): declared but never read. The
  recall_events handler correctly uses args.EventType. A caller passing
  event_type_filter in JSON got no error and no filter — silent no-op
  contract drift.

Both eliminations. The existing EventType field already covers the
recall_events filter surface; wiring EventTypeFilter in parallel would
have been fake symmetry.

Addresses Diana review findings R1 and R2 on commit 8dddddf.
Copilot AI review requested due to automatic review settings April 14, 2026 18:03
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Improves correctness of remember_event by ensuring event creation and observation attachments commit atomically, while also removing unused/dead code surfaced during post-port review.

Changes:

  • Wrap remember_event event insert + observation attachment loop in a single transaction to prevent orphan event rows on mid-loop failure.
  • Refactor CreateEvent to accept a transaction-capable interface (dbtx) and remove unused scanEntityRecord.
  • Add a regression test proving rollback behavior on injected mid-loop failure; document the invariant in OPERATIONS.md.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
internal/store/tools.go Makes remember_event transactional end-to-end; removes unused EventTypeFilter arg field.
internal/store/parity_test.go Adds TestRememberEventAtomicityOnMidLoopFailure to prevent orphan events/entities on failures.
internal/store/events.go Updates CreateEvent to accept dbtx; removes dead scanEntityRecord.
OPERATIONS.md Documents the remember_event atomicity invariant with a concrete test reference.

Diana's R3 finding flagged the absence of a proof against Kilo or another
real external client. It was archaeology by the time it reached review:
the binary at /Users/zenosartori/workmem/workmem has been wired as the
MCP server backing Claude Code's memory and private_memory entries (via
~/.claude.json + governor env files), and is also live in Kilo and
Codex. The review session itself exercised the binary through real tool
calls (remember_batch, recall_entity) with correct results.

Removing the stale P1 entry from OPERATIONS.md.
@marlian marlian merged commit fa4f62c into main Apr 14, 2026
8 checks passed
@marlian marlian deleted the fix/remember-event-atomicity branch April 14, 2026 18:15
marlian added a commit that referenced this pull request Apr 14, 2026
- docs/TELEMETRY.md: full user-facing guide. Adapted from the Node
  telemetry.md, with privacy-strict mode documented (trade-off: ranking
  debug vs plaintext leak risk on sensitive backends). Includes example
  client config for Claude Code / governor env-file wiring.

- OPERATIONS.md invariants: telemetry is opt-in and never affects the
  success path; DB is physically separate from memory DB; strict mode
  sha256-hashes identifiers before disk. P1 "telemetry deferred" entry
  removed (condition met). P2 telemetry-schema entry removed (designed
  and implemented). Pre-Launch TODO no longer lists Kilo proof — already
  closed in PR #4.

- IMPLEMENTATION.md: new Step 3.4 Telemetry marked done with explicit
  Gate — enabled writes rows, disabled creates no DB, strict hashes.

- DECISION_LOG.md: append entry documenting the three Go-native
  refinements (nil-tolerant Client, no globals, SearchMetrics tuple)
  and the new privacy-strict mode. Explicitly rejects at-rest
  encryption with keychain for this iteration as over-scope.
marlian added a commit that referenced this pull request Apr 14, 2026
- docs/TELEMETRY.md: full user-facing guide. Adapted from the Node
  telemetry.md, with privacy-strict mode documented (trade-off: ranking
  debug vs plaintext leak risk on sensitive backends). Includes example
  client config for Claude Code / governor env-file wiring.

- OPERATIONS.md invariants: telemetry is opt-in and never affects the
  success path; DB is physically separate from memory DB; strict mode
  sha256-hashes identifiers before disk. P1 "telemetry deferred" entry
  removed (condition met). P2 telemetry-schema entry removed (designed
  and implemented). Pre-Launch TODO no longer lists Kilo proof — already
  closed in PR #4.

- IMPLEMENTATION.md: new Step 3.4 Telemetry marked done with explicit
  Gate — enabled writes rows, disabled creates no DB, strict hashes.

- DECISION_LOG.md: append entry documenting the three Go-native
  refinements (nil-tolerant Client, no globals, SearchMetrics tuple)
  and the new privacy-strict mode. Explicitly rejects at-rest
  encryption with keychain for this iteration as over-scope.
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