mirror of
https://github.com/nearai/ironclaw.git
synced 2026-09-03 08:06:01 +08:00
6895cdad9e
* fix(db): repair V6 migration checksum and guard against re-modification (#1328) PR #1151 modified the already-released migrations/V6__routines.sql in place, causing refinery's checksum validation to abort startup on every existing PostgreSQL deployment upgrading to v0.19.0. Revert V6 to its v0.18.0 content (V13 already applies the schema change incrementally and is idempotent for fresh installs that received the modified V6). Add a runtime checksum realignment step that rewrites refinery_schema_history rows whose stored checksum disagrees with the embedded SQL — this handles both populations of databases in the wild (pre-#1151 originals and post-#1151 fresh installs). Add migrations/checksums.lock pinning every migration's SipHasher13 checksum and a `released_migrations_are_immutable` cargo test that fails if any migration is modified or added without a matching lockfile entry. A second hard-coded sentinel test pins V6's literal v0.18.0 checksum so the guard cannot be defeated by editing both the migration and the lockfile in the same commit. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(db): address review feedback on migration_fixup (#2101) - Drop hard-coded `public.` schema qualifier from the existence probe so PostgreSQL resolves `refinery_schema_history` via the active search_path, matching how refinery itself locates the table and how the subsequent UPDATE statement is written. Without this, deployments using a non-default schema would silently skip the realignment. - Use `IS DISTINCT FROM` instead of `<>` so a corrupted row with a NULL checksum is repaired rather than silently skipped. - Add `explanation` field to `KnownDivergence` and use it in the realignment warning so future entries are not coupled to the V6/#1328 wording. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(db): narrowly whitelist V6 known-bad checksum (#2101) Per @serrrfirat's review, the previous IS DISTINCT FROM-based realignment would silently rewrite *any* non-canonical V6 checksum, masking unrelated corruption or manual tampering instead of narrowly exempting the one historical released mismatch. Add a `known_bad_checksums: &'static [u64]` field to `KnownDivergence` listing the exact historical bad value(s), and rewrite only rows whose stored checksum is in that whitelist via `WHERE checksum = ANY($4)`. Anything else is left alone so refinery still aborts startup loudly. The single known-bad V6 value (`11230857244097235596`) is the SipHasher13 of `git show 878a67cd:migrations/V6__routines.sql` (the post-#1151 content) and is pinned by a new sentinel test `v6_known_bad_checksum_matches_post_1151_content` so the whitelist cannot drift or be silently widened. Also adds an ignored bootstrap helper `compute_checksum_for_external_file` for computing checksums of external SQL files when adding future entries. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(db): assert in release + add postgres integration test (#2101) Address two follow-up review comments from @serrrfirat: 1. The defensive `debug_assert!` guarding against the canonical checksum being listed in `known_bad_checksums` is stripped in release builds, so the safety net was absent in production. `KNOWN_DIVERGENCES` has at most a handful of entries — switch to `assert!` so the guard runs in release too. Cost is one constant- time slice lookup per startup. 2. The `realign_diverged_checksums` SQL path was never exercised against a real database. Refactor into a thin pub wrapper plus an injectable `realign_diverged_checksums_with` inner helper, and add a `#[cfg(feature = "integration")]` test that: - skips gracefully if no DATABASE_URL is reachable - creates `refinery_schema_history` if missing - seeds a synthetic V99999 row with a deliberately-wrong checksum - calls the realignment with a custom divergence list (no collision with real V6 rows in shared CI databases) - asserts the row now holds the canonical checksum - re-runs the realignment and asserts a no-op Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(db): replace assert! with returned error to satisfy no-panics check (#2101) The previous commit changed `debug_assert!` → `assert!` to keep the canonical-in-known-bad-list guard active in release builds, but this trips the project's "No panics in production code" CI check (the regex matches `assert!` outside test attributes). Replace with an early `return Err(DatabaseError::Migration(...))` so the guard still runs in release builds — startup refuses to proceed with a misconfigured `KNOWN_DIVERGENCES` table — without using a panicking macro. This is also more idiomatic for a function that already returns `Result`. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(db): address review follow-ups on migration_fixup (#2101) - parse_lockfile() now panics on duplicate migration keys instead of silently overwriting earlier entries — a stray duplicate could mask the actual pinned checksum and weaken the immutability guard (Copilot review). - Add `rejects_canonical_in_known_bad_checksums` integration test exercising the defensive Err path that refuses startup when a KnownDivergence has its canonical checksum listed in its own known_bad_checksums list (serrrfirat review). - Document why `tracing::warn!` is intentional in the realignment fix-up despite CLAUDE.md's warning about info!/warn! corrupting the TUI: this code runs at startup before any channel/REPL/TUI is initialized, so terminal-rendering interference is impossible. If the call site ever moves later in startup, downgrade to debug! or pre-buffer (illblackdragon review). - Cross-reference comments in src/history/store.rs and src/setup/wizard.rs pointing each other out so future changes to the migration fix-up call site stay in sync (illblackdragon review). - Sort migrations/checksums.lock by parsed migration version (V1, V2, ..., V10, V11, ...) instead of lex order (V10 before V2). The resulting file reads in numeric order which makes review diffs easier to scan (illblackdragon review). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(db): consolidate migration entry points + advisory lock + no leaks (#2101) Address three Medium-severity findings from @serrrfirat's review: 1. **Duplicate call sites** — extract `run_postgres_migrations_with_fixup(client)` in `crate::db::migration_fixup` that bundles fix-up + refinery into a single function. Both `Store::run_migrations` and `SetupWizard::run_migrations_postgres` now call it. Eliminates the class of bug where a future entry point could forget the fix-up. The previous comment-based coupling was an interim measure. 2. **Concurrent startup race** — the new helper acquires `pg_advisory_lock(1328)` (issue number, easy to grep in `pg_locks`) before realignment and releases it after refinery returns, on every exit path including errors. Serializes concurrent migration runs across replicas — also hardens the pre-existing refinery race that has always existed for multi-replica starts. Uses session-level advisory lock (not `pg_advisory_xact_lock`) because refinery's `run_async` opens its own internal transactions. 3. **`Box::leak` in tests** — refactor `KnownDivergence` to be lifetime-generic (`KnownDivergence<'a>`). Production `KNOWN_DIVERGENCES` is `&[KnownDivergence<'static>]` — no external API change. Both integration tests now use stack-allocated `&[u64]` slices, no `Box::leak`. Removes the leak-sanitizer false positive. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>