Skip to content

Fix source-weight publication exactness - #105

Merged
protostatis merged 1 commit into
mainfrom
fix/source-weight-exact-mirror
Aug 14, 2026
Merged

Fix source-weight publication exactness#105
protostatis merged 1 commit into
mainfrom
fix/source-weight-exact-mirror

Conversation

@protostatis

Copy link
Copy Markdown
Owner

Summary

  • serialize source-weight publishers with an immediate SQLite transaction
  • reject empty, invalid, and stale versioned publications before they can mutate current state
  • make source_weights an exact compatibility mirror of the accepted versioned snapshot
  • preserve historical snapshots while pruning removed and legacy-null current rows
  • verify staged and published row counts and roll back the complete transaction on mismatch

Why

A shrinking accepted belief snapshot could leave removed sources behind in source_weights, allowing stale sources to continue influencing inference. Publication also needed explicit stale-writer rejection so an older updater could not prune or overwrite newer rows.

Invariants

  • stale versions never alter staged, current, or published state
  • equal-version retries produce an exact current mirror
  • current rows and the published snapshot have identical membership and values
  • empty snapshots cannot be staged or published
  • any prune or mirror verification failure rolls back staging, upserts, deletion, and publication

Validation

  • pytest tests/ -q: 255 passed, 1 skipped
  • focused source-weight/state tests: passed
  • Ruff import checks for changed files: passed
  • Python compileall: passed

Scope

This PR intentionally contains only the durable source-weight publication fix and focused tests. Reddit recovery scripts, incident specifications, and other dirty recovery-worktree artifacts are excluded.

@protostatis protostatis left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Sky's Code Review

This PR hardens source-weight publication with an explicit BEGIN IMMEDIATE transaction and layered guards: it rejects empty/invalid/stale versions, prunes removed and legacy-null rows atomically with upserts, and verifies the publish mirror against the staged snapshot before committing, rolling back on mismatch. The test coverage is excellent (shrinking snapshots, equal-version idempotency, stale rejection, legacy-null pruning, empty rejection, and rollback-on-failure via an injected trigger) and directly maps to the stated invariants. The core semantics — serialize publishers, prune non-members, null-safe mirror comparison — are correct, and the type(x) is not int check correctly rejects booleans. No security or production-breaking bugs found. Only minor, non-blocking observations about transaction commit/rollback visibility and over-strict typing.

Verdict: Approve

Comments

  • The newer_current check (source_weights rows with belief_version > requested) is now largely redundant since BEGIN IMMEDIATE serializes writers — but it's harmless defense-in-depth against any future non-transactional writer and documents the invariant clearly. Fine to keep.
  • The mirror verification query uses IS (null-safe) comparisons for accuracy/alpha/beta/sample_size, which is the correct choice since those columns may legitimately be NULL. It verifies membership overlap and value equality between current and snapshot rows, but does not independently assert the current table has no extra rows — that responsibility is delegated to the prune step above. The two together are sound, and the injected-trigger rollback test exercises the combined atomicity.
  • The empty-snapshot rejection (Refusing to stage or publish an empty source-weight snapshot) is a deliberate semantic choice that prevents a shrinking belief set from ever resulting in a completely empty source_weights table. It also means a brand-new database with no sources yet can never publish an initial empty state — confirm this is the intended behavior for cold-start, as an empty snapshot is now rejected at both stage and publish paths.

Reviewed by Sky — Unchained Sky engineering agent

Inline Comments (could not attach to lines)

crypto_sentiment_crawler/analysis/source_weights.py:210 — The explicit BEGIN IMMEDIATE is good for serializing writers and avoiding SQLITE_BUSY, but the diff fragment doesn't show the commit/rollback path. Confirm the surrounding try/except (the pre-existing await db.conn.commit() / rollback-on-exception) still runs after this manual BEGIN — otherwise failures after this point could leave a dangling open transaction. The rollback test passes, which is a good sign, but worth a quick visual check.

crypto_sentiment_crawler/analysis/source_weights.py:203type(belief_version) is not int is intentionally stricter than isinstance(..., int) — it rejects bool (subclass of int) and any numpy int types. That's correct for rejecting True/False, but be aware it will also reject numpy.int64 if a caller ever passes one. Consider documenting this deliberately-strict intent or using an explicit isinstance(x, bool) guard plus isinstance(x, int) if numeric-int interoperability is ever expected.

crypto_sentiment_crawler/analysis/source_weights.py:311 — The prune DELETE ... WHERE NOT EXISTS (snapshot.belief_version = ? AND snapshot.source = source_weights.source) correctly removes all current rows absent from the accepted snapshot, including legacy NULL-version rows. Note this relies on the upserts having completed first (they do, in order), so a just-upserted source is never pruned. Good ordering; no change needed.

@protostatis
protostatis merged commit bfc7917 into main Aug 14, 2026
3 checks passed
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.

1 participant