mirror of
https://github.com/garrytan/gbrain.git
synced 2026-07-27 22:15:33 +00:00
fix(v24): guard budget_ledger + budget_reservations with IF EXISTS
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 = '<tbl>') 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.
This commit is contained in:
+31
-13
@@ -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 $$;
|
||||
`,
|
||||
},
|
||||
|
||||
+22
-1
@@ -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,
|
||||
|
||||
Reference in New Issue
Block a user