Claude finished @Yeraze's task —— View job

PR Review: test(migrations): guard against source-dependent migration crashes

• Gather context and read changed files
• Review test file quality and correctness
• Post final review

Overview

This is a well-motivated regression guard. The incident description in both the PR body and the file header clearly explains the blind spot: the existing migration test always ran against a zero-source DB, so the if (sources.length > 0) branches in source-dependent migrations (like 033's channel_database backfill) were never exercised by unit CI. This test closes that gap.

Code Quality

Strengths:

• The header comment (lines 1–17) is excellent — it explains why the test exists, the exact failure mode, and references the incident issues.
• The error message in the catch block (Migration ${m.number} (${m.name}) threw with a source registered) matches what the PR description says the regression test produces, making it easy to diagnose future failures.
• The sanity assertions at the bottom (lines 75–77) are a nice touch — they guard against the test silently proving nothing if migration 020 ever moves or the table name changes.

Potential issues:

1. Hard-coded column list in the INSERT may go stale (line 64–67). The test inserts a source with (id, name, type, config, enabled, createdAt, updatedAt). Migration 081 later adds displayOrder with DEFAULT 0, so this specific INSERT is safe today. But future migrations that add NOT NULL columns without defaults to the sources table after migration 020 could cause the pre-081 INSERT to fail before the displayOrder column is even added. This is a low-probability risk but a real one — consider noting it as a known fragility in the comment, or inserting after migration 020 completes and before the next migration runs (which the current code already does — just worth noting the dependency).

2. registeredAtMigration is always the migration 020 number (line 76–77). The variable is set to m.number at the moment the source is inserted, which is the number of the migration where the sources table first appears (020). The assertion expect(registeredAtMigration).toBeLessThan(33) is correct and will catch it if 020 is ever renumbered above 033. However, the assertion expect(registeredAtMigration).toBeGreaterThan(0) is trivially true if we got past the first migration — consider tightening to expect(registeredAtMigration).toBe(20) to make the test self-documenting and catch accidental reordering.

3. Only tests SQLite (line 28, new Database(':memory:')). This is noted implicitly by the if (!m.sqlite) continue guard. PostgreSQL/MySQL equivalents of 033's bug are not covered. This is an acceptable scope for the PR given the stated goal, but worth flagging as a known gap.

4. sqlite.close() is not in a finally block (line 79). If an earlier assertion throws (e.g. expect(sourceRegistered).toBe(true) at line 75), the DB handle won't be closed. For an in-memory DB this is harmless, but the pattern is slightly inconsistent with good resource management. A try/finally would be cleaner.

Minor Nits

• Line 51: m.sqlite(sqlite as any, getSetting, setSetting) — the as any cast works but is worth noting that the getSetting/setSetting helpers exactly mirror what the real migration runner passes, which is good.
• The registeredAtMigration variable is assigned but only used in assertions — could use const if extracted, but the current pattern is clear enough.

Verdict

Approve with suggestions. The core approach is sound and catches the exact regression class described. The two most actionable suggestions are:

1. Tighten expect(registeredAtMigration).toBeGreaterThan(0) → expect(registeredAtMigration).toBe(20) for self-documentation.
2. Wrap sqlite.close() in a try/finally.

Neither is a blocker — this test is already a meaningful improvement over the prior zero-source-only coverage.