fix(v0.18.1): address codex review (orchestrator wiring + fail-closed + identifier escape)

Four fixes from `/codex` review of the merged diff:

1. HIGH — wire migration v24 into the `gbrain apply-migrations`
   upgrade path. Without an orchestrator entry, `gbrain upgrade`'s
   post-upgrade step runs `apply-migrations --yes`, which walks the
   registry in `src/commands/migrations/index.ts`. The registry
   stopped at v0_18_0, so v24 never fired on upgrade (connectEngine
   and doctor do not call initSchema). New `v0_18_1.ts` orchestrator
   mirrors v0.18.0's Phase A: shells out to `gbrain init
   --migrate-only`, which triggers initSchema → runMigrations → v24
   applies. Registered in the migrations array.

2. HIGH — fail loudly when v24 runs under a non-BYPASSRLS role
   instead of RAISE WARNING-then-silently-bumping-version. The
   runner at migrate.ts:773 unconditionally calls
   `setConfig('version', String(m.version))` when a migration
   completes without throwing, so a WARNING-and-continue path would
   permanently lock the backfill out: schema_version=24 on the next
   run means `m.version > current` is false and v24 is skipped
   forever, even after the role gets BYPASSRLS. Changed `RAISE
   WARNING` → `RAISE EXCEPTION` so the transaction aborts,
   schema_version stays at 23, and a subsequent initSchema retries
   cleanly after the role is fixed. Test asserts the SQL uses
   EXCEPTION and does not use WARNING.

3. MEDIUM — escape double-quote characters in the remediation SQL
   output. doctor.ts was building `ALTER TABLE "public"."${n}"`
   with `n` un-escaped, so a pathological table name containing a
   literal `"` would break out of the quoted identifier and produce
   invalid copy-paste SQL. Double the `"` before interpolating,
   matching Postgres quoted-identifier escaping rules. Extremely
   rare in practice, cheap to get right.

4. LOW — CHANGELOG cleanup: corrected the upgrade-behavior claim
   (v24 runs via `apply-migrations --yes` through the new
   orchestrator, not during `gbrain doctor`) and split the "tables
   with RLS" row into two metrics (21 base-schema tables + 2
   migration-only budget_* tables = 23 managed total, all covered).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

CHANGELOG.md

@@ -9,7 +9,7 @@ All notable changes to GBrain will be documented in this file.
 
 A follow-up to v0.18.0 that closes a latent security gap the multi-source work didn't touch. `gbrain` itself had been quietly shipping 10 public tables without RLS for months: `access_tokens`, `mcp_request_log`, `minion_inbox`, `minion_attachments`, `subagent_messages`, `subagent_tool_executions`, `subagent_rate_leases`, `gbrain_cycle_locks`, `budget_ledger`, `budget_reservations`. On Supabase, every one of those was reachable by the anon key. `access_tokens` and the subagent conversation history tables are the ones that stand out.
 
-This release closes the gap on three fronts at once: the check gets widened (so future misses surface), the base schema gets the missing 8 `ENABLE RLS` statements for tables it tracks (so fresh installs are secure), and new schema migration v24 `rls_backfill_missing_tables` enables RLS on all 10 of the affected tables in existing brains automatically when `gbrain doctor` or `gbrain apply-migrations` runs. The sources + file_migration_ledger tables introduced in v0.18.0 already had RLS from day one.
+This release closes the gap on three fronts at once: the check gets widened (so future misses surface), the base schema gets the missing 8 `ENABLE RLS` statements for tables it tracks (so fresh installs are secure), and new schema migration v24 `rls_backfill_missing_tables` enables RLS on all 10 of the affected tables in existing brains automatically. The v0.18.1 orchestrator (`src/commands/migrations/v0_18_1.ts`) invokes `gbrain init --migrate-only` during `gbrain apply-migrations --yes`, which is what `gbrain post-upgrade` calls; v24 runs there, not inside `gbrain doctor` itself. The `sources` and `file_migration_ledger` tables introduced in v0.18.0 already had RLS from day one.
 
 The severity also upgrades from `warn` to `fail`. Missing RLS is a security issue, not a suggestion. `gbrain doctor` will exit 1 when any public table is missing RLS. If you wrap `gbrain doctor` in a cron or CI health check, expect it to turn red if anything is still open.
 
@@ -22,8 +22,9 @@ Measured against v0.18.0.
 | Metric | BEFORE v0.18.1 | AFTER v0.18.1 | Δ |
 |--------|----------------|---------------|---|
 | Tables in `public` covered by the doctor RLS check | 10 (hardcoded list) | all (every `pg_tables` row) | +100% coverage |
-| gbrain-managed public tables with RLS on fresh install | 13 of 21 | 21 of 21 | +8 in schema |
-| Tables backfilled by migration v24 on existing brains | 0 | 10 | budget_ledger + budget_reservations covered too |
+| gbrain-managed public tables with RLS after full migration chain | 13 of 23 | 23 of 23 | +10 total |
+| — of which added in base schema v0.18.1 | 13 of 21 | 21 of 21 | +8 in schema |
+| — of which added via migration v24 (budget_ledger + budget_reservations, still migration-only) | 0 | 10 | migration-only tables now covered |
 | `gbrain doctor` severity for missing RLS | warn (exit 0) | fail (exit 1) | breaking |
 | Escape hatch for intentional anon-readable tables | None (silent misses) | `GBRAIN:RLS_EXEMPT reason=...` pg comment | new capability |
 | Identifier-safe remediation SQL (hyphens, reserved words) | No (`ALTER TABLE public.<name>`) | Yes (`ALTER TABLE "public"."<name>"`) | correctness |
@@ -42,9 +43,10 @@ Credit: Garry's OpenClaw for the original PR widening the check (#336). Codex fo
 
 ## To take advantage of v0.18.1
 
-`gbrain upgrade` should do this automatically. Migration v24 runs on the next
-`initSchema()` call, which happens on every CLI or long-running command startup. If
-`gbrain doctor` warns about a partial migration or still reports missing RLS:
+`gbrain upgrade` should do this automatically. It runs `gbrain post-upgrade`,
+which calls `gbrain apply-migrations --yes`, which runs the v0.18.1 orchestrator
+(`gbrain init --migrate-only` → schema migration v24 applied). If `gbrain doctor`
+still reports missing RLS after upgrade:
 
 1. **Apply migrations manually:**
    ```bash
@@ -74,7 +76,8 @@ Credit: Garry's OpenClaw for the original PR widening the check (#336). Codex fo
 ### Itemized changes
 
 - **Schema fix (fresh installs):** `src/schema.sql` now enables RLS on all 21 public tables in the base schema (13 before v0.18.1, +8 added here: `access_tokens`, `mcp_request_log`, `minion_inbox`, `minion_attachments`, `subagent_messages`, `subagent_tool_executions`, `subagent_rate_leases`, `gbrain_cycle_locks`). `src/core/schema-embedded.ts` regenerated.
-- **Schema migration (existing installs):** New migration `v24 rls_backfill_missing_tables` in `src/core/migrate.ts` enables RLS on all 10 affected tables idempotently (the 8 above plus `budget_ledger` and `budget_reservations`, which remain migration-only per v12). Gated on `rolbypassrls` — if the current role does not hold BYPASSRLS, the migration raises a warning and skips, matching the pattern used by the base schema's RLS block. Numbered v24 to slot after v0.18.0's v20-v23 sources-migration wave.
+- **Schema migration (existing installs):** New migration `v24 rls_backfill_missing_tables` in `src/core/migrate.ts` enables RLS on all 10 affected tables idempotently (the 8 above plus `budget_ledger` and `budget_reservations`, which remain migration-only per v12). Gated on `rolbypassrls`; if the current role does not hold BYPASSRLS, the migration `RAISE EXCEPTION`s and aborts so `schema_version` stays at 23. Next `initSchema` call after switching to a bypass role retries cleanly. Numbered v24 to slot after v0.18.0's v20-v23 sources-migration wave.
+- **Upgrade orchestrator:** New `src/commands/migrations/v0_18_1.ts` wires v24 into the `gbrain apply-migrations --yes` path via `gbrain init --migrate-only` (mirrors v0.18.0's Phase A). Without this, `gbrain doctor` and `connectEngine()` never call `initSchema()`, so v24 would sit in the registry but never apply on upgrade.
 - **Doctor check widened:** `src/commands/doctor.ts` RLS check now scans every public table from `pg_tables`, not a hardcoded 10-name allowlist. Severity upgraded `warn → fail`. Success message shows table count. Failure message includes per-table quoted `ALTER TABLE "public"."<name>" ENABLE ROW LEVEL SECURITY;` remediation SQL.
 - **Escape hatch — "write it in blood":** Doctor reads `obj_description` for each non-RLS public table. Tables whose comment matches `^GBRAIN:RLS_EXEMPT\s+reason=\S.{3,}` count as explicitly exempt. Exempt tables are enumerated by name on every successful doctor run so the exemption list never goes invisible. No CLI subcommand — deliberate friction; operators must set the comment in psql.
 - **PGLite skip:** PGLite is embedded and single-user with no PostgREST; the RLS check now skips on PGLite with an explicit `ok` message ("Skipped — no PostgREST exposure, RLS not applicable") instead of the misleading `warn` it emitted before. Partial polish: pgvector, jsonb_integrity, and markdown_body_completeness checks still hit the same `getConnection()` throw → warn pattern on PGLite. Separate follow-up.

src/commands/doctor.ts

@@ -341,8 +341,13 @@ export async function runDoctor(engine: BrainEngine | null, args: string[], dbSo
         });
       } else {
         const names = gaps.join(', ');
+        // Double-escape " inside identifiers so a pathological table name
+        // like `weird"table` renders as `"weird""table"` in the remediation
+        // SQL (matches how Postgres parses quoted identifiers). Doubling
+        // any existing " is the minimum needed to keep the output valid
+        // copy-paste SQL. Extremely rare in practice but cheap to get right.
         const fixes = gaps
-          .map(n => `ALTER TABLE "public"."${n}" ENABLE ROW LEVEL SECURITY;`)
+          .map(n => `ALTER TABLE "public"."${n.replace(/"/g, '""')}" ENABLE ROW LEVEL SECURITY;`)
           .join(' ');
         const exemptInfo = exempt.length > 0
           ? ` (${exempt.length} other table(s) explicitly exempt.)`

src/commands/migrations/index.ts

@@ -19,6 +19,7 @@ import { v0_13_1 } from './v0_13_1.ts';
 import { v0_14_0 } from './v0_14_0.ts';
 import { v0_16_0 } from './v0_16_0.ts';
 import { v0_18_0 } from './v0_18_0.ts';
+import { v0_18_1 } from './v0_18_1.ts';
 
 export const migrations: Migration[] = [
   v0_11_0,
@@ -29,6 +30,7 @@ export const migrations: Migration[] = [
   v0_14_0,
   v0_16_0,
   v0_18_0,
+  v0_18_1,
 ];
 
 /** Look up a migration by exact version string. */

src/commands/migrations/v0_18_1.ts

@@ -0,0 +1,69 @@
+/**
+ * v0.18.1 migration orchestrator — RLS hardening.
+ *
+ * v0.18.1 ships one new schema migration: v24 `rls_backfill_missing_tables`.
+ * It enables Row Level Security on 10 gbrain-managed public tables that
+ * shipped without it: access_tokens, mcp_request_log, minion_inbox,
+ * minion_attachments, subagent_messages, subagent_tool_executions,
+ * subagent_rate_leases, gbrain_cycle_locks, budget_ledger, budget_reservations.
+ *
+ * Phase structure mirrors v0.18.0:
+ *   A. Schema — `gbrain init --migrate-only` runs the migration chain,
+ *      picking up v24 on brains currently at v23 (post-v0.18.0) or earlier.
+ *
+ * Without this orchestrator, the `apply-migrations` registry stops at
+ * v0.18.0 and the low-level schema migration in src/core/migrate.ts never
+ * fires on upgrade, because doctor + connectEngine never call initSchema().
+ */
+
+import { execSync } from 'child_process';
+import type { Migration, OrchestratorOpts, OrchestratorResult, OrchestratorPhaseResult } from './types.ts';
+
+// ── Phase A — Schema ────────────────────────────────────────
+
+function phaseASchema(opts: OrchestratorOpts): OrchestratorPhaseResult {
+  if (opts.dryRun) return { name: 'schema', status: 'skipped', detail: 'dry-run' };
+  try {
+    execSync('gbrain init --migrate-only', { stdio: 'inherit', timeout: 600_000, env: process.env });
+    return { name: 'schema', status: 'complete' };
+  } catch (e) {
+    const msg = e instanceof Error ? e.message : String(e);
+    return { name: 'schema', status: 'failed', detail: msg };
+  }
+}
+
+// ── Orchestrator ────────────────────────────────────────────
+
+async function orchestrator(opts: OrchestratorOpts): Promise<OrchestratorResult> {
+  const phases: OrchestratorPhaseResult[] = [];
+  phases.push(phaseASchema(opts));
+
+  const anyFailed = phases.some(p => p.status === 'failed');
+  const status: OrchestratorResult['status'] = anyFailed ? 'partial' : 'complete';
+
+  return {
+    version: '0.18.1',
+    status,
+    phases,
+    pending_host_work: 0,
+  };
+}
+
+// ── Export ──────────────────────────────────────────────────
+
+export const v0_18_1: Migration = {
+  version: '0.18.1',
+  featurePitch: {
+    headline: 'Row Level Security hardened on all public tables + escape hatch.',
+    description:
+      'v0.18.1 fixes a latent security gap: 10 gbrain-managed public tables ' +
+      'shipped without RLS. On Supabase, they were reachable by the anon key. ' +
+      'Migration v24 backfills RLS on existing brains automatically when ' +
+      '`gbrain apply-migrations` runs. `gbrain doctor` now scans every ' +
+      'public table (no hardcoded allowlist) and exits 1 on gaps. For tables ' +
+      'that should stay anon-readable on purpose, operators set a ' +
+      '`GBRAIN:RLS_EXEMPT reason=<why>` comment via psql. See ' +
+      'docs/guides/rls-and-you.md.',
+  },
+  orchestrator,
+};

src/core/migrate.ts

@@ -726,7 +726,13 @@ export const MIGRATIONS: Migration[] = [
           ALTER TABLE budget_reservations ENABLE ROW LEVEL SECURITY;
           RAISE NOTICE 'v24: RLS enabled on 10 backfill tables (role % has BYPASSRLS)', current_user;
         ELSE
-          RAISE WARNING 'v24: Skipping RLS backfill: role % does not have BYPASSRLS privilege. Run as postgres role to enable.', current_user;
+          -- 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
+          -- on future runs even after switching to a bypass role. Raising
+          -- aborts the transaction, leaves schema_version at the prior value,
+          -- 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;
       END $$;
     `,

test/migrate.test.ts

@@ -219,7 +219,18 @@ describe('migration v24 — rls_backfill_missing_tables', () => {
     const sql = v24!.sql || '';
     expect(sql).toContain('rolbypassrls');
     expect(sql).toContain('IF has_bypass THEN');
-    expect(sql).toMatch(/RAISE WARNING[^;]*BYPASSRLS/);
+  });
+
+  // Codex found: if v24 RAISE WARNINGs instead of raising on non-BYPASSRLS,
+  // the migration runner still bumps schema_version to 24, permanently
+  // skipping the backfill on future runs even after the role is fixed.
+  // The fix is to raise loudly so the transaction aborts, version stays
+  // at 23, and the next initSchema call retries after role reassignment.
+  test('fails loudly on non-BYPASSRLS roles instead of silently bumping version', () => {
+    const v24 = MIGRATIONS.find(m => m.version === 24);
+    const sql = v24!.sql || '';
+    expect(sql).toMatch(/RAISE EXCEPTION[^;]*BYPASSRLS/);
+    expect(sql).not.toMatch(/RAISE WARNING[^;]*BYPASSRLS/);
   });
 
   test('LATEST_VERSION has caught up to 24', () => {