From 2fc7780f837848ef34d0301311b9630cd41a4dfd Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Thu, 23 Apr 2026 07:05:31 -0700 Subject: [PATCH] fix(v24): guard budget_ledger + budget_reservations with IF EXISTS MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Garry flagged: migration v24 fires `ALTER TABLE budget_ledger ENABLE ROW LEVEL SECURITY` unconditionally. budget_ledger and budget_reservations are migration-only (v12) — not in schema.sql, not re-created on every initSchema. In the normal flow v12 runs before v24 so they exist, but two edge cases break that assumption: 1. An operator manually dropped them (budget data is regenerable from resolver call logs, so `DROP TABLE` is a reasonable cleanup move). 2. A brain was somehow running an old gbrain that lacked v12, and is only catching up now. Bare ALTER hits 42P01 (relation does not exist), aborts the transaction, and leaves schema_version at 23. On next initSchema, v24 retries and hits the same error — stuck in a loop. Fix: wrap each of the two budget ALTERs in IF EXISTS (SELECT 1 FROM information_schema.tables WHERE table_schema = 'public' AND table_name = '') THEN ... END IF; The other 8 tables are not guarded. schema.sql creates them idempotently on every initSchema run before migrations fire, so they are guaranteed to exist by the time v24 runs. Adding guards there would be unnecessary and make the SQL noisier. Also simplified the DECLARE/BEGIN structure: moved the non-BYPASSRLS early-exit to the top so the happy path reads cleanly without the outer IF. Tests: - test/migrate.test.ts: new assertion that both budget_* ALTERs are wrapped in information_schema.tables IF EXISTS blocks; BYPASSRLS gate assertion relaxed to match either phrasing. - Manual e2e: fresh Postgres init (v0→v24), then DROP TABLE budget_ledger + budget_reservations, reset version=23, re-run init. v24 applied cleanly, version advanced to 24, budget_* stayed dropped. Without the guard this would have errored out. --- src/core/migrate.ts | 44 +++++++++++++++++++++++++++++++------------- test/migrate.test.ts | 23 ++++++++++++++++++++++- 2 files changed, 53 insertions(+), 14 deletions(-) diff --git a/src/core/migrate.ts b/src/core/migrate.ts index 2f2d82fc3..6694bb894 100644 --- a/src/core/migrate.ts +++ b/src/core/migrate.ts @@ -713,19 +713,7 @@ export const MIGRATIONS: Migration[] = [ has_bypass BOOLEAN; BEGIN SELECT rolbypassrls INTO has_bypass FROM pg_roles WHERE rolname = current_user; - IF has_bypass THEN - ALTER TABLE access_tokens ENABLE ROW LEVEL SECURITY; - ALTER TABLE mcp_request_log ENABLE ROW LEVEL SECURITY; - ALTER TABLE minion_inbox ENABLE ROW LEVEL SECURITY; - ALTER TABLE minion_attachments ENABLE ROW LEVEL SECURITY; - ALTER TABLE subagent_messages ENABLE ROW LEVEL SECURITY; - ALTER TABLE subagent_tool_executions ENABLE ROW LEVEL SECURITY; - ALTER TABLE subagent_rate_leases ENABLE ROW LEVEL SECURITY; - ALTER TABLE gbrain_cycle_locks ENABLE ROW LEVEL SECURITY; - ALTER TABLE budget_ledger ENABLE ROW LEVEL SECURITY; - ALTER TABLE budget_reservations ENABLE ROW LEVEL SECURITY; - RAISE NOTICE 'v24: RLS enabled on 10 backfill tables (role % has BYPASSRLS)', current_user; - ELSE + IF NOT has_bypass THEN -- Fail the migration loudly instead of WARNING + version-bump. -- The runner unconditionally records schema_version on success, -- so a silent WARNING here would permanently lock the backfill out @@ -734,6 +722,36 @@ export const MIGRATIONS: Migration[] = [ -- and lets the next invocation retry after the role is fixed. RAISE EXCEPTION 'v24 rls_backfill_missing_tables: role % does not have BYPASSRLS privilege — cannot enable RLS safely. Re-run as postgres (or another BYPASSRLS role). The migration will retry automatically on the next initSchema call.', current_user; END IF; + + -- These 8 are guaranteed to exist: schema.sql creates them (idempotent + -- via IF NOT EXISTS) on every initSchema call, and initSchema runs + -- before this migration. Bare ALTER TABLE is safe. + ALTER TABLE access_tokens ENABLE ROW LEVEL SECURITY; + ALTER TABLE mcp_request_log ENABLE ROW LEVEL SECURITY; + ALTER TABLE minion_inbox ENABLE ROW LEVEL SECURITY; + ALTER TABLE minion_attachments ENABLE ROW LEVEL SECURITY; + ALTER TABLE subagent_messages ENABLE ROW LEVEL SECURITY; + ALTER TABLE subagent_tool_executions ENABLE ROW LEVEL SECURITY; + ALTER TABLE subagent_rate_leases ENABLE ROW LEVEL SECURITY; + ALTER TABLE gbrain_cycle_locks ENABLE ROW LEVEL SECURITY; + + -- budget_ledger + budget_reservations are migration-only (v12). Not + -- in schema.sql, not re-created on every initSchema. In normal flow + -- v12 runs before v24 so they exist, but if an operator manually + -- dropped them (unusual — budget data is regenerable from resolver + -- logs) or was pinned to a pre-v12 gbrain version when the table + -- went away, the bare ALTER would fail with 42P01 and abort v24. + -- information_schema.tables lookup makes the statement self-healing. + IF EXISTS (SELECT 1 FROM information_schema.tables + WHERE table_schema = 'public' AND table_name = 'budget_ledger') THEN + ALTER TABLE budget_ledger ENABLE ROW LEVEL SECURITY; + END IF; + IF EXISTS (SELECT 1 FROM information_schema.tables + WHERE table_schema = 'public' AND table_name = 'budget_reservations') THEN + ALTER TABLE budget_reservations ENABLE ROW LEVEL SECURITY; + END IF; + + RAISE NOTICE 'v24: RLS backfill complete (role % has BYPASSRLS)', current_user; END $$; `, }, diff --git a/test/migrate.test.ts b/test/migrate.test.ts index 047e9c4e7..6ebbbc31c 100644 --- a/test/migrate.test.ts +++ b/test/migrate.test.ts @@ -218,7 +218,28 @@ describe('migration v24 — rls_backfill_missing_tables', () => { const v24 = MIGRATIONS.find(m => m.version === 24); const sql = v24!.sql || ''; expect(sql).toContain('rolbypassrls'); - expect(sql).toContain('IF has_bypass THEN'); + // The gate can be either IF has_bypass / early-raise pattern. + expect(sql).toMatch(/IF (NOT )?has_bypass/); + }); + + // Self-healing guard: the budget_* tables are migration-only (v12). If an + // operator manually dropped them, or if a brain was somehow pinned to a + // pre-v12 version when those tables didn't exist, a bare `ALTER TABLE + // budget_ledger ...` would fail with 42P01 and abort v24. Wrapping those + // two ALTERs in an `IF EXISTS (information_schema.tables ...)` check lets + // the migration skip them silently instead of erroring out. The other 8 + // tables are created by schema.sql on every initSchema and don't need + // the guard — bare ALTER is fine. + test('guards budget_ledger + budget_reservations with information_schema.tables IF EXISTS', () => { + const v24 = MIGRATIONS.find(m => m.version === 24); + const sql = v24!.sql || ''; + // Both budget tables must be wrapped in an existence check. + expect(sql).toMatch( + /IF EXISTS \(SELECT 1 FROM information_schema\.tables[\s\S]{0,200}table_name = 'budget_ledger'\)[\s\S]{0,200}ALTER TABLE budget_ledger ENABLE ROW LEVEL SECURITY/, + ); + expect(sql).toMatch( + /IF EXISTS \(SELECT 1 FROM information_schema\.tables[\s\S]{0,200}table_name = 'budget_reservations'\)[\s\S]{0,200}ALTER TABLE budget_reservations ENABLE ROW LEVEL SECURITY/, + ); }); // Codex found: if v24 RAISE WARNINGs instead of raising on non-BYPASSRLS,