Fix source-weight publication exactness - #105
Conversation
protostatis
left a comment
There was a problem hiding this comment.
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_currentcheck (source_weightsrows with belief_version > requested) is now largely redundant sinceBEGIN IMMEDIATEserializes 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 emptysource_weightstable. 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:203 — type(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.
Summary
source_weightsan exact compatibility mirror of the accepted versioned snapshotWhy
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
Validation
pytest tests/ -q: 255 passed, 1 skippedScope
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.