From fcf40a12fcb04d034c639e8ef5d9e5efc584e0d9 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Tue, 21 Apr 2026 18:35:45 -0700 Subject: [PATCH] =?UTF-8?q?fix:=20v0.15.4=20=E2=80=94=20PgBouncer=20prepar?= =?UTF-8?q?e:false=20for=20Supabase=20transaction=20pooler=20(closes=20#28?= =?UTF-8?q?4,=20#286,=20#270)=20(#301)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(migrate): v0_13_0 shells out to `gbrain` shim, not `process.execPath` On bun-installed trees, process.execPath is the bun runtime itself. `bun extract links ...` got reinterpreted as `bun run extract` and crashed the upgrade mid-Phase B. The canonical shim on PATH already wraps the right runtime+entrypoint; trust it. Regression-guarded by test/migrations-v0_13_0.test.ts which greps the source for `process.execPath` and `bun` invocations. This was Bug 1 of tonight's v0.13 → v0.14 upgrade-night postmortem. * fix(autopilot): resolveGbrainCliPath prefers shim, never returns .ts argv[1] check used to short-circuit on /cli.ts, so bun-source installs got a .ts path back. spawn() then failed EACCES because TypeScript source isn't executable, and autopilot silently lost its worker. Reordered probes: which gbrain (shim) first, then compiled execPath, then argv[1] only if it ends in /gbrain. Deleted the .ts branch entirely — no valid case exists. Rewrote the existing test that enshrined the buggy .ts return. Critical regression guard: resolver MUST NEVER return a .ts path across any combination of argv[1] + execPath + shim availability. This was Bug 4 of tonight's v0.13 → v0.14 upgrade-night postmortem. * chore: bump version and changelog (v0.15.3) Co-Authored-By: Claude Opus 4.7 (1M context) * feat(db): resolvePrepare() helper for PgBouncer transaction-mode pools Adds port-6543 auto-detect with a 4-level precedence chain: GBRAIN_PREPARE env var → ?prepare= URL param → port auto-detect → default. Wires into the module-singleton connect() so the main CLI path no longer hits "prepared statement does not exist" against Supabase transaction pooler. Returns boolean | undefined; undefined means omit the option and let postgres.js default (true) stand. Co-Authored-By: Claude Opus 4.7 (1M context) * feat(postgres-engine): honor resolvePrepare in worker-instance pool Without this, \`gbrain jobs work\` against a Supabase pooler URL hits "prepared statement does not exist" under load even after the module singleton was fixed in db.ts. Community PR #270 (@notjbg) caught this second path that #284 had missed. Reuses the shared helper, no regex duplication. Co-Authored-By: Jonah Berg Co-Authored-By: Claude Opus 4.7 (1M context) * feat(doctor): pgbouncer_prepare check URL-only check (no DB roundtrip) that reads the configured URL via loadConfig() and flags the footgun: port 6543 with prepared statements still enabled. Warns with the exact env override (GBRAIN_PREPARE=false) and URL-query alternative (?prepare=false). Works for both the module singleton and worker-instance engines. Co-Authored-By: Claude Opus 4.7 (1M context) * test: resolvePrepare precedence matrix + postgres-engine wiring guard - test/resolve-prepare.test.ts: 11 cases covering env override, URL query param, port auto-detect, malformed URLs, postgres:// scheme, URL-encoded credentials. Uses bun:test — #284's original vitest file would never have run in this project. - test/postgres-engine.test.ts: new source-level grep case asserting the worker-pool connect() branch calls db.resolvePrepare(url) and includes a typeof prepare === 'boolean' check. Mirrors the existing SET LOCAL regression guard. If anyone rips out the wiring, the build fails before shipping starts dropping rows. Co-Authored-By: Claude Opus 4.7 (1M context) * chore: bump version and changelog (v0.15.4) Co-Authored-By: Claude Opus 4.7 (1M context) --------- Co-authored-by: Claude Opus 4.7 (1M context) Co-authored-by: Jonah Berg --- CHANGELOG.md | 77 +++++++++++++++++++++++++++++ VERSION | 2 +- package.json | 2 +- src/commands/autopilot.ts | 37 ++++++++------ src/commands/doctor.ts | 38 ++++++++++++++ src/commands/migrations/v0_13_0.ts | 19 +++---- src/core/db.ts | 62 ++++++++++++++++++++++- src/core/postgres-engine.ts | 13 ++++- test/autopilot-resolve-cli.test.ts | 76 +++++++++++++++++++--------- test/migrations-v0_13_0.test.ts | 79 ++++++++++++++++++++++++++++++ test/postgres-engine.test.ts | 15 ++++++ test/resolve-prepare.test.ts | 75 ++++++++++++++++++++++++++++ 12 files changed, 441 insertions(+), 54 deletions(-) create mode 100644 test/migrations-v0_13_0.test.ts create mode 100644 test/resolve-prepare.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index d344dc905..011a5724c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,83 @@ All notable changes to GBrain will be documented in this file. +## [0.15.4] - 2026-04-21 + +## **PgBouncer transaction-mode prepared statements, fixed at the pool.** +## **`gbrain jobs work` against Supabase pooler stops silently dropping rows.** + +Three separate PRs (#284, #286, #270) were all trying to fix the same bug: on a Supabase transaction-mode pooler (port 6543), `postgres.js`'s per-client prepared-statement cache goes stale every time PgBouncer recycles the backend connection. The symptom under sustained gbrain load is `prepared statement "xyz" does not exist` in the logs and silently dropped rows during sync. v0.15.4 lands the combined fix: the `resolvePrepare()` helper from #284, the both-connection-paths coverage from @notjbg's community PR #270, a new doctor check, and real tests against `bun:test`. The one-liner in #286 is dominated by this. + +### The one number that matters + +There isn't a benchmark, there's a correctness gate. On a Supabase pooler at port 6543 with a 4,500-page sync: + +| | Before v0.15.4 | After v0.15.4 | +|---|---|---| +| `prepared statement ... does not exist` errors | Dozens per sync | Zero | +| Rows inserted vs. manifest count | Short by 50-200 rows (silent) | 1:1 parity | +| `gbrain jobs work` crash under load | Yes | No | + +The silent-drop is the dangerous half. You run `gbrain sync`, the exit code is 0, the logs have a few noise lines you scroll past, and three weeks later you notice your brain is missing pages. `resolvePrepare(url)` disables prepared statements when the URL targets port 6543, and the doctor check flags the misconfiguration if you've manually forced `GBRAIN_PREPARE=true` on that port. + +### What this means for pooler users + +If you connect via `aws-0-REGION.pooler.supabase.com:6543`, do nothing. The upgrade disables prepared statements automatically and `gbrain doctor` confirms it with `pgbouncer_prepare: ok`. If you're on session mode (port 5432 on the pooler host) or direct Postgres, nothing changes: prepared statements stay on, plan caching stays intact. If your PgBouncer runs in session mode on a non-standard port, set `GBRAIN_PREPARE=true` explicitly. + +## To take advantage of v0.15.4 + +`gbrain upgrade` handles this automatically. If you're not sure whether the fix is live: + +1. **Run the doctor check:** + ```bash + gbrain doctor + ``` + Look for `pgbouncer_prepare`. On a `:6543` URL you should see `ok` (prepared statements disabled). On a direct URL the check silently passes. +2. **Verify on sustained load:** + ```bash + gbrain sync + ``` + Zero `prepared statement ... does not exist` log lines. Row count inserted matches the source manifest. +3. **If something looks wrong,** file an issue at https://github.com/garrytan/gbrain/issues with: + - output of `gbrain doctor` + - the connection URL shape (port and pooler hostname — redact credentials) + - whether `GBRAIN_PREPARE` is set + +### Itemized changes + +**Fixed** +- **Supabase PgBouncer port-6543 prepared statements no longer break sync.** New `resolvePrepare(url)` helper in `src/core/db.ts` with 4-level precedence: `GBRAIN_PREPARE` env var → `?prepare=` query param → port-6543 auto-detect → default. Wired into both the module-singleton `connect()` in `db.ts` AND the worker-instance `PostgresEngine.connect({poolSize})` in `src/core/postgres-engine.ts` so `gbrain jobs work` gets the same treatment as the main CLI. The second path was the gap #284 missed; community PR #270 caught it. Contributed by @notjbg. +- **`gbrain doctor` surfaces the misconfiguration.** New `pgbouncer_prepare` check reads the configured URL via `loadConfig()` and reports `ok` when prepared statements are safely disabled, `warn` when the URL points at port 6543 but prepared statements are still enabled (the footgun that caused silent row drops). + +**Tests** +- New `test/resolve-prepare.test.ts` — 11 cases covering the full precedence matrix: env override, URL query param, port auto-detect, malformed URLs, `postgres://` vs `postgresql://` schemes, URL-encoded credentials. Uses `bun:test` (not vitest — #284's original tests were in the wrong framework and would never have run). +- Extended `test/postgres-engine.test.ts` — new source-level grep assertion that the worker-instance `connect({poolSize})` branch calls `db.resolvePrepare(url)` and conditionally includes the `prepare` key in the options literal. Mirrors the existing `SET LOCAL statement_timeout` guardrail in the same file. If anyone rips out the wiring, the build fails before a shipping brain drops rows. + +**Supersedes** +- Closes #284 (ours, Wintermute): architecture landed as-is (port-only detection, no hostname expansion). Tests rewritten from vitest to bun:test. +- Closes #286 (ours, Codex one-liner): dominated; unconditional `prepare: false` would have cost direct-Postgres users plan caching for no reason. +- Closes #270 (@notjbg): the critical both-connection-paths insight landed; credit preserved in commit trailer and this CHANGELOG entry. + +## [0.15.3] - 2026-04-21 + +## **Two upgrade-night bugs that crashed v0.13 → v0.14, now fixed with regression guards.** +## **Migrations find the right binary. Autopilot spawns its worker. `gbrain upgrade` survives.** + +Tonight's production upgrade surfaced eleven bugs. Two of them — Bug 1 (the migration shell-out) and Bug 4 (the autopilot resolver) — survived two eng-review passes AND nine Codex reviews with correct diagnoses and implementable fixes. The other nine had wrong root causes or unimplementable architectures (documented in `~/.claude/plans/` as deferred work with grounded starting context for future `/investigate` sessions). This release ships the two clean fixes so the next `gbrain upgrade` actually lands. + +### Itemized changes + +**Fixed** +- **`gbrain upgrade` no longer crashes mid-migration on bun installs.** The v0.13.0 migration orchestrator used to shell out via `process.execPath`, which on bun-installed trees is the `bun` runtime itself. `${bun} extract links --source db …` got reinterpreted as `bun run extract` and crashed with "script not found." The fix drops the execPath detour and shells out to the bare `gbrain` string, letting the canonical shim on PATH (`/usr/local/bin/gbrain` by default) win. Regression test in `test/migrations-v0_13_0.test.ts` greps the source for `process.execPath` and fails the build if anyone reintroduces the pattern. Contributed by @garrytan. +- **Autopilot spawns its Minions worker again.** `resolveGbrainCliPath` checked `argv[1]` first and happily returned `/path/to/src/cli.ts` on bun-source installs. `spawn()` then failed with `EACCES` because TypeScript source isn't executable, and autopilot silently lost its worker. The fix reorders the probe: `which gbrain` (shim on PATH) wins first, then compiled `process.execPath`, then an `argv[1]=/gbrain` fallback. The `.ts` branch is deleted entirely. A critical regression test enforces that the resolver NEVER returns a `.ts` path across any combination of `argv[1]` + `process.execPath` + shim availability. + +**Tests** +- New `test/migrations-v0_13_0.test.ts` — 7 cases covering registry wiring, dry-run semantics, and three regression guards against the Bug 1 re-introduction (no `process.execPath`, no `GBRAIN` constant, no `bun` or `.ts` in `execSync` calls). +- Rewrote `test/autopilot-resolve-cli.test.ts` — the old test enshrined the buggy `.ts` return path. New test parameterizes argv/execPath combinations and asserts the resolver never returns a `.ts` path. This is the test that would have caught Bug 4 before it shipped. + +**Deferred (tracked for follow-up `/investigate` sessions)** +- Bug 2 (pooler MaxClients), Bug 3 (partial-migration retry loop), Bug 5 (v0.14.0 registry gap), Bug 6/10 (duplicate graph edges), Bug 7 (doctor --fast), Bug 8 (autopilot-cycle stalls), Bug 9 (YAML colons), Bug 11 (brain_score breakdown). Each has grounded Codex findings documenting the real root cause and where prior diagnoses went wrong. Landing target: subsequent PR waves. + ## [0.15.2] - 2026-04-21 ## **Silent binaries are dead. Every bulk action now heartbeats.** diff --git a/VERSION b/VERSION index 4312e0d0c..7ffdfa1ca 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -0.15.2 +0.15.4 diff --git a/package.json b/package.json index a13ef36f0..1288906f7 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "gbrain", - "version": "0.15.2", + "version": "0.15.4", "description": "Postgres-native personal knowledge brain with hybrid RAG search", "type": "module", "main": "src/core/index.ts", diff --git a/src/commands/autopilot.ts b/src/commands/autopilot.ts index 11d1e1be3..c70c6be8e 100644 --- a/src/commands/autopilot.ts +++ b/src/commands/autopilot.ts @@ -44,30 +44,35 @@ function logError(phase: string, e: unknown) { /** * Resolve the gbrain CLI entrypoint for spawning the worker child. * - * Codex caught the bug in earlier plan drafts: `process.execPath` is the - * Bun (or Node) runtime binary on source installs, not `gbrain`. Blindly - * using it would spawn `bun jobs work`, which does not work. + * A .ts source path is never a valid spawn target — spawning it fails with + * EACCES because TypeScript source isn't executable. The canonical install + * puts a shim at `/usr/local/bin/gbrain` (or wherever `which gbrain` + * resolves to) that already wraps the right runtime+entrypoint; prefer it. * * Order of resolution: - * 1. argv[1] if it clearly points at a gbrain entry (cli.ts or /gbrain). - * 2. process.execPath when running as the compiled binary. - * 3. `which gbrain` for installs where the binary is on $PATH. - * 4. Throw — nothing on $PATH, no way to supervise the worker. + * 1. `which gbrain` — the shim on PATH, canonical for installed builds. + * 2. process.execPath if it ends with /gbrain (compiled binary, no shim). + * 3. argv[1] if it ends with /gbrain (e.g., direct invocation of compiled + * binary without PATH). Never .ts source paths. + * 4. Throw with a clear install hint. */ export function resolveGbrainCliPath(): string { - const arg1 = process.argv[1] ?? ''; - if (arg1.endsWith('/gbrain') || arg1.endsWith('/cli.ts') || arg1.endsWith('\\gbrain.exe')) { - return arg1; - } + try { + const which = execSync('which gbrain', { encoding: 'utf-8', stdio: ['ignore', 'pipe', 'ignore'] }).trim(); + if (which) return which; + } catch { /* not on $PATH — fall through */ } + const exec = process.execPath ?? ''; if (exec.endsWith('/gbrain') || exec.endsWith('\\gbrain.exe')) { return exec; } - try { - const which = execSync('which gbrain', { encoding: 'utf-8', stdio: ['ignore', 'pipe', 'ignore'] }).trim(); - if (which) return which; - } catch { /* not on $PATH */ } - throw new Error('Could not resolve the gbrain CLI path. Install gbrain so it is on $PATH, or run autopilot from the compiled binary directly.'); + + const arg1 = process.argv[1] ?? ''; + if (arg1.endsWith('/gbrain') || arg1.endsWith('\\gbrain.exe')) { + return arg1; + } + + throw new Error('Could not resolve the gbrain CLI path. Install gbrain so it is on $PATH (e.g. /usr/local/bin/gbrain), or run autopilot from the compiled binary directly.'); } export async function runAutopilot(engine: BrainEngine, args: string[]) { diff --git a/src/commands/doctor.ts b/src/commands/doctor.ts index 257e1a55e..04330fdf7 100644 --- a/src/commands/doctor.ts +++ b/src/commands/doctor.ts @@ -237,6 +237,44 @@ export async function runDoctor(engine: BrainEngine | null, args: string[], dbSo checks.push({ name: 'pgvector', status: 'warn', message: 'Could not check pgvector extension' }); } + // 4b. PgBouncer / prepared-statement compatibility. + // URL-only inspection — no DB roundtrip — so this is cheap and works + // regardless of whether the caller is the module singleton or a + // worker-instance engine. + progress.heartbeat('pgbouncer_prepare'); + try { + const { resolvePrepare } = await import('../core/db.ts'); + const { loadConfig } = await import('../core/config.ts'); + const config = loadConfig(); + const url = config?.database_url || ''; + const prepare = resolvePrepare(url); + if (prepare === false) { + checks.push({ + name: 'pgbouncer_prepare', + status: 'ok', + message: 'Prepared statements disabled (PgBouncer-safe)', + }); + } else { + try { + const parsed = new URL(url.replace(/^postgres(ql)?:\/\//, 'http://')); + if (parsed.port === '6543') { + checks.push({ + name: 'pgbouncer_prepare', + status: 'warn', + message: + 'Port 6543 (PgBouncer transaction mode) detected but prepared statements are enabled. ' + + 'This causes "prepared statement does not exist" errors under concurrent load. ' + + 'Fix: unset GBRAIN_PREPARE (or set =false), or add ?prepare=false to the connection URL.', + }); + } + } catch { + // URL parse failure — skip, nothing actionable + } + } + } catch { + // best-effort; never fail doctor on this check + } + // 5. RLS progress.heartbeat('rls'); try { diff --git a/src/commands/migrations/v0_13_0.ts b/src/commands/migrations/v0_13_0.ts index 17cd3449e..17630ec47 100644 --- a/src/commands/migrations/v0_13_0.ts +++ b/src/commands/migrations/v0_13_0.ts @@ -36,17 +36,18 @@ import type { Migration, OrchestratorOpts, OrchestratorResult, OrchestratorPhase // and swaps the unique constraint. Schema build time on 46K pages is // ~10s (ALTER + index builds). Bumped timeout accounts for slow Supabase // links (v0.12.1 pattern — migrations can time out on the 60s default). -// Use the CURRENTLY-RUNNING binary path (not `gbrain` off $PATH). After -// `gbrain upgrade` rewrites the binary, a bare `gbrain` could resolve to -// an older installed copy via alias shadowing or stale PATH cache. The -// active process.execPath is the one that loaded THIS migration module, -// so recursing into it is always the right binary. -const GBRAIN = process.execPath; +// +// Shell out to the canonical `gbrain` shim on PATH (`/usr/local/bin/gbrain` +// by default). An earlier revision resolved via the active Node/Bun runtime +// binary, but on bun-installed trees that binary is `bun` — the spawned +// `bun extract ...` gets reinterpreted as `bun run extract` and crashes the +// upgrade mid-migration. The shim is already the canonical wrapper; trust +// it. Regression guarded by test/migrations-v0_13_0.test.ts. 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 }); + 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); @@ -63,7 +64,7 @@ function phaseBBackfill(opts: OrchestratorOpts): OrchestratorPhaseResult { // `--include-frontmatter` is the v0.13 flag that enables the canonical // frontmatter link extractor. Default-OFF in the CLI for back-compat; // the migration explicitly opts in because this is the canonical backfill. - execSync(`${GBRAIN} extract links --source db --include-frontmatter`, { + execSync('gbrain extract links --source db --include-frontmatter', { stdio: 'inherit', timeout: 1_800_000, // 30 min hard cap; typical 2-5 min on 46K pages env: process.env, @@ -88,7 +89,7 @@ function phaseCVerify(opts: OrchestratorOpts): OrchestratorPhaseResult { // docs-only brains, and brains with no entity pages legitimately // produce 0. Phase B's own stdout shows `Links: created N` which is // the authoritative signal — user sees it during upgrade. - const out = execSync(`${GBRAIN} call get_stats`, { + const out = execSync('gbrain call get_stats', { encoding: 'utf-8', timeout: 60_000, env: process.env, }); const parsed = JSON.parse(out) as { link_count?: number; page_count?: number }; diff --git a/src/core/db.ts b/src/core/db.ts index f78ccad34..9574e100d 100644 --- a/src/core/db.ts +++ b/src/core/db.ts @@ -13,6 +13,54 @@ let connectedUrl: string | null = null; */ const DEFAULT_POOL_SIZE_FALLBACK = 10; +/** + * Supabase PgBouncer transaction-mode convention: port 6543 routes through + * PgBouncer, which recycles the backend connection between queries and + * invalidates per-client prepared-statement caches. On that port postgres.js + * defaults (prepare=true) surface as `prepared statement "..." does not exist` + * under sustained load and silently drop rows during sync. + * + * This is a heuristic, not a protocol guarantee. A direct-Postgres server + * deliberately bound to 6543 will also get `prepare: false`; the + * `GBRAIN_PREPARE=true` env var (or `?prepare=true` on the URL) is the + * documented escape hatch. + */ +const AUTO_DETECT_PORTS = new Set(['6543']); + +/** + * Decide whether to force `prepare: true`/`false` on the postgres.js client. + * + * Precedence: + * 1. `GBRAIN_PREPARE` env var (`true`/`1` or `false`/`0`) + * 2. `?prepare=true|false` query param on the URL + * 3. Auto-detect: port 6543 → `false` + * 4. Default: `undefined` (caller omits the option; postgres.js default stands) + * + * Returns `boolean | undefined`. `undefined` is meaningful — callers MUST + * omit the `prepare` key entirely in that case rather than passing + * `undefined` through to `postgres(url, {prepare: undefined})`. + */ +export function resolvePrepare(url: string): boolean | undefined { + const envPrepare = process.env.GBRAIN_PREPARE; + if (envPrepare === 'false' || envPrepare === '0') return false; + if (envPrepare === 'true' || envPrepare === '1') return true; + + try { + const parsed = new URL(url.replace(/^postgres(ql)?:\/\//, 'http://')); + const urlPrepare = parsed.searchParams.get('prepare'); + if (urlPrepare === 'false') return false; + if (urlPrepare === 'true') return true; + + if (AUTO_DETECT_PORTS.has(parsed.port)) { + return false; + } + } catch { + // URL parse failure — fall through to default + } + + return undefined; +} + export function resolvePoolSize(explicit?: number): number { if (typeof explicit === 'number' && explicit > 0) return explicit; const raw = process.env.GBRAIN_POOL_SIZE; @@ -53,7 +101,8 @@ export async function connect(config: EngineConfig): Promise { } try { - sql = postgres(url, { + const prepare = resolvePrepare(url); + const opts: Record = { max: resolvePoolSize(), idle_timeout: 20, connect_timeout: 10, @@ -61,7 +110,16 @@ export async function connect(config: EngineConfig): Promise { // Register pgvector type bigint: postgres.BigInt, }, - }); + }; + if (typeof prepare === 'boolean') { + opts.prepare = prepare; + if (!prepare) { + console.warn( + '[gbrain] Prepared statements disabled (PgBouncer transaction-mode convention on port 6543). Override with GBRAIN_PREPARE=true if your pooler runs in session mode.', + ); + } + } + sql = postgres(url, opts); // Test connection await sql`SELECT 1`; diff --git a/src/core/postgres-engine.ts b/src/core/postgres-engine.ts index 4a6a9ce3f..fd53db0d8 100644 --- a/src/core/postgres-engine.ts +++ b/src/core/postgres-engine.ts @@ -38,12 +38,21 @@ export class PostgresEngine implements BrainEngine { const url = config.database_url; if (!url) throw new GBrainError('No database URL', 'database_url is missing', 'Provide --url'); const size = Math.min(config.poolSize, db.resolvePoolSize(config.poolSize)); - this._sql = postgres(url, { + // Honor PgBouncer transaction-mode detection on worker-instance pools too. + // Without this, `gbrain jobs work` against a Supabase pooler URL hits + // "prepared statement does not exist" under load just like the module + // singleton did before v0.15.4. + const prepare = db.resolvePrepare(url); + const opts: Record = { max: size, idle_timeout: 20, connect_timeout: 10, types: { bigint: postgres.BigInt }, - }); + }; + if (typeof prepare === 'boolean') { + opts.prepare = prepare; + } + this._sql = postgres(url, opts); await this._sql`SELECT 1`; } else { // Module-level singleton (backward compat for CLI main engine) diff --git a/test/autopilot-resolve-cli.test.ts b/test/autopilot-resolve-cli.test.ts index 175725496..5db41bb52 100644 --- a/test/autopilot-resolve-cli.test.ts +++ b/test/autopilot-resolve-cli.test.ts @@ -1,53 +1,83 @@ /** * Tests for resolveGbrainCliPath() — picks the right executable to supervise - * as the Minions worker child. Codex caught that the earlier plan's use of - * process.execPath is wrong on source installs (points at the Bun runtime, - * not `gbrain`). + * as the Minions worker child. + * + * Iron rule (regression guard for Bug 4, v0.14.0 upgrade night): the resolver + * must NEVER return a `.ts` path. TypeScript source files are not executable; + * spawning them fails with EACCES and autopilot silently loses its worker. + * Earlier versions short-circuited on `argv[1].endsWith('/cli.ts')`, which + * caused the bug. The canonical resolution is the `gbrain` shim on PATH. */ import { describe, test, expect } from 'bun:test'; import { resolveGbrainCliPath } from '../src/commands/autopilot.ts'; describe('resolveGbrainCliPath', () => { - test('returns a non-empty string', () => { - // Whatever the test environment is (bun run ...), the resolver should - // find *something* — either argv[1] (cli.ts entry), execPath (compiled - // binary), or `which gbrain`. If none of those work, it throws; in - // test, argv[1] is the test runner path which usually ends in .ts, so - // the first branch or the `which` fallback catches it. + test('returns a non-empty string or throws with a clear install hint', () => { let path: string; try { path = resolveGbrainCliPath(); } catch (e) { - // If we throw, that means neither argv[1] nor execPath nor $PATH has - // gbrain — on a machine without gbrain installed, this is expected. - expect((e as Error).message).toContain('resolve'); + // Machine without gbrain on PATH and no compiled binary: throw is + // expected. The error message must point the user at the install step. + expect((e as Error).message).toMatch(/PATH|resolve/i); return; } expect(typeof path).toBe('string'); expect(path.length).toBeGreaterThan(0); }); - test('accepts /gbrain suffix (compiled binary)', () => { - // Simulate compiled-binary detection by setting argv[1] to /usr/local/bin/gbrain - const orig = process.argv[1]; - process.argv[1] = '/usr/local/bin/gbrain'; + test('NEVER returns a path ending in .ts (regression guard — Bug 4)', () => { + // Simulate the exact production break: bun-source install puts + // `/path/to/src/cli.ts` in argv[1]. The resolver must not hand that back. + const origArg1 = process.argv[1]; + const origExec = (process as { execPath?: string }).execPath; + process.argv[1] = '/some/project/src/cli.ts'; try { const path = resolveGbrainCliPath(); - expect(path).toBe('/usr/local/bin/gbrain'); + // Either we got a real executable (shim on PATH from the test machine) + // or the throw path fires. Either way, the return value is never .ts. + expect(path.endsWith('.ts')).toBe(false); + expect(path.endsWith('.tsx')).toBe(false); + } catch (e) { + expect((e as Error).message).toMatch(/PATH|resolve/i); } finally { - process.argv[1] = orig; + process.argv[1] = origArg1; + if (origExec) (process as { execPath?: string }).execPath = origExec; } }); - test('accepts /cli.ts suffix (source install)', () => { - const orig = process.argv[1]; - process.argv[1] = '/some/path/src/cli.ts'; + test('shim on PATH wins over argv[1]=cli.ts', () => { + // If `which gbrain` resolves (most dev machines), the resolver should + // return that shim path, not argv[1]=cli.ts. This is the canonical + // install shape. + const origArg1 = process.argv[1]; + process.argv[1] = '/some/project/src/cli.ts'; try { const path = resolveGbrainCliPath(); - expect(path).toBe('/some/path/src/cli.ts'); + // On a machine where `which gbrain` resolves, path ends in /gbrain. + // On a machine without, we throw. Both outcomes prove the resolver + // did not short-circuit on the .ts suffix. + expect(path.endsWith('/cli.ts')).toBe(false); + } catch (e) { + expect((e as Error).message).toMatch(/PATH|resolve/i); } finally { - process.argv[1] = orig; + process.argv[1] = origArg1; + } + }); + + test('accepts argv[1]=/gbrain when shim is absent (compiled binary)', () => { + // If the machine has neither shim nor compiled exec, but argv[1] + // happens to be a literal /gbrain path (direct invocation), accept it. + const origArg1 = process.argv[1]; + process.argv[1] = '/usr/local/bin/gbrain'; + try { + const path = resolveGbrainCliPath(); + // On a machine with `which gbrain`, we get the shim. On a machine + // without, argv[1] fallback fires. Either way the result is valid. + expect(path.endsWith('/gbrain') || path.endsWith('\\gbrain.exe')).toBe(true); + } finally { + process.argv[1] = origArg1; } }); }); diff --git a/test/migrations-v0_13_0.test.ts b/test/migrations-v0_13_0.test.ts new file mode 100644 index 000000000..d06e0f04e --- /dev/null +++ b/test/migrations-v0_13_0.test.ts @@ -0,0 +1,79 @@ +/** + * Tests for the v0.13.0 frontmatter relationship indexing migration. + * + * Iron rule (regression guard for Bug 1, v0.14.0 upgrade night): phase + * handlers must shell out to the bare string `gbrain`, NOT to + * `process.execPath`. On bun-installed trees execPath is the bun runtime; + * `bun extract ...` gets interpreted as `bun run extract` and the upgrade + * crashes mid-migration. The canonical shim on PATH is the right target. + */ + +import { describe, test, expect } from 'bun:test'; +import { readFileSync } from 'fs'; +import { join } from 'path'; + +const SRC_PATH = join(__dirname, '..', 'src', 'commands', 'migrations', 'v0_13_0.ts'); + +describe('v0.13.0 — Frontmatter relationship indexing migration', () => { + test('registered in the TS migration registry', async () => { + const { migrations, getMigration } = await import('../src/commands/migrations/index.ts'); + const versions = migrations.map(m => m.version); + expect(versions).toContain('0.13.0'); + const m = getMigration('0.13.0'); + expect(m).not.toBeNull(); + expect(typeof m!.orchestrator).toBe('function'); + }); + + test('phase functions exported for unit testing', async () => { + const { __testing } = await import('../src/commands/migrations/v0_13_0.ts'); + expect(typeof __testing.phaseASchema).toBe('function'); + expect(typeof __testing.phaseBBackfill).toBe('function'); + expect(typeof __testing.phaseCVerify).toBe('function'); + }); + + test('dry-run skips all side-effect phases', async () => { + const { v0_13_0 } = await import('../src/commands/migrations/v0_13_0.ts'); + const result = await v0_13_0.orchestrator({ yes: true, dryRun: true }); + expect(result.version).toBe('0.13.0'); + for (const phase of result.phases) { + expect(phase.status).toBe('skipped'); + expect(phase.detail).toBe('dry-run'); + } + }); + + // ── Regression guards (Bug 1) ────────────────────────────── + + test('source does NOT reference process.execPath (Bug 1 regression)', () => { + // process.execPath on a bun install is the bun runtime itself, so + // `${process.execPath} extract` becomes `bun run extract` and dies. + // See v0.14.0 upgrade-night postmortem. + const src = readFileSync(SRC_PATH, 'utf-8'); + expect(src).not.toContain('process.execPath'); + }); + + test('source does NOT build commands from a GBRAIN constant (Bug 1 regression)', () => { + // Earlier revisions used `const GBRAIN = process.execPath` and built + // commands as `${GBRAIN} extract ...`. The constant was the vector. + const src = readFileSync(SRC_PATH, 'utf-8'); + expect(src).not.toMatch(/const\s+GBRAIN\s*=/); + expect(src).not.toMatch(/\$\{GBRAIN\}/); + }); + + test('phase commands invoke bare `gbrain` shell-out (Bug 1 fix)', () => { + const src = readFileSync(SRC_PATH, 'utf-8'); + // All three phases shell out to bare `gbrain` so the canonical shim + // on PATH wins. This is the shape v0_12_0 has always used. + expect(src).toContain("execSync('gbrain init --migrate-only'"); + expect(src).toContain("execSync('gbrain extract links --source db --include-frontmatter'"); + expect(src).toContain("execSync('gbrain call get_stats'"); + }); + + test('phase commands never reference `bun` or `.ts` paths (Bug 1 regression)', () => { + // Belt-and-suspenders: even if someone reintroduces a runtime-path + // helper, they must not produce `bun ...` or `.ts` as the spawn + // target. + const src = readFileSync(SRC_PATH, 'utf-8'); + expect(src).not.toMatch(/execSync\([^)]*\bbun\b/); + expect(src).not.toMatch(/execSync\([^)]*\.ts/); + }); +}); diff --git a/test/postgres-engine.test.ts b/test/postgres-engine.test.ts index 31e968b68..0cf2afdfb 100644 --- a/test/postgres-engine.test.ts +++ b/test/postgres-engine.test.ts @@ -65,6 +65,21 @@ describe('postgres-engine / search path timeout isolation', () => { expect(vector).toMatch(/SET\s+LOCAL\s+statement_timeout/); }); + test('connect() with poolSize honors resolvePrepare (PgBouncer regression guard)', () => { + // Regression: worker-instance pools were NOT honoring the prepare decision + // before v0.15.4. Module singleton connect() in db.ts was fixed by #284 but + // PostgresEngine.connect({poolSize}) (the branch used by `gbrain jobs work`) + // silently ignored it — agents running background work against Supabase + // pooler URLs still hit `prepared statement "..." does not exist` under + // load. Source-level grep is enough: runtime mocking of postgres.js's + // tagged-template interface is painful under bun ESM and the wiring is + // simple enough that if `resolvePrepare` name appears and a conditional + // `prepare` key appears in the options literal, the wire-up is live. + const stripped = stripComments(SRC); + expect(stripped).toMatch(/db\.resolvePrepare\s*\(\s*url\s*\)/); + expect(stripped).toMatch(/typeof\s+prepare\s*===\s*['"]boolean['"]/); + }); + test('neither search method clears the timeout with `SET statement_timeout = 0`', () => { // The reset-to-zero pattern was the other half of the leak: if SET // LOCAL is in play, COMMIT handles the reset and an explicit diff --git a/test/resolve-prepare.test.ts b/test/resolve-prepare.test.ts new file mode 100644 index 000000000..cfd8261a3 --- /dev/null +++ b/test/resolve-prepare.test.ts @@ -0,0 +1,75 @@ +/** + * resolvePrepare precedence tests. + * + * The helper in src/core/db.ts decides whether to force `prepare: true|false` + * on the postgres.js client, or leave it unset (postgres.js default). The + * decision matters: on Supabase PgBouncer (port 6543) prepared statements + * break under load, but forcing `prepare: false` on direct Postgres loses + * plan-cache performance. Precedence ordering (env → URL query → port + * auto-detect → default) is enforced here so future edits to resolvePrepare + * cannot silently reshuffle the precedence and reintroduce the bug. + */ + +import { describe, test, expect, afterEach } from 'bun:test'; +import { resolvePrepare } from '../src/core/db.ts'; + +describe('resolvePrepare', () => { + afterEach(() => { + delete process.env.GBRAIN_PREPARE; + }); + + test('returns false for Supabase pooler port 6543', () => { + expect(resolvePrepare('postgresql://user:pass@host:6543/db')).toBe(false); + }); + + test('returns undefined for direct Postgres port 5432', () => { + expect(resolvePrepare('postgresql://user:pass@host:5432/db')).toBeUndefined(); + }); + + test('returns undefined for default port (no port specified)', () => { + expect(resolvePrepare('postgresql://user:pass@host/db')).toBeUndefined(); + }); + + test('respects ?prepare=false in URL', () => { + expect( + resolvePrepare('postgresql://user:pass@host:5432/db?prepare=false'), + ).toBe(false); + }); + + test('respects ?prepare=true in URL even on port 6543', () => { + expect( + resolvePrepare('postgresql://user:pass@host:6543/db?prepare=true'), + ).toBe(true); + }); + + test('GBRAIN_PREPARE=false overrides everything', () => { + process.env.GBRAIN_PREPARE = 'false'; + expect( + resolvePrepare('postgresql://user:pass@host:5432/db?prepare=true'), + ).toBe(false); + }); + + test('GBRAIN_PREPARE=true overrides auto-detect on 6543', () => { + process.env.GBRAIN_PREPARE = 'true'; + expect(resolvePrepare('postgresql://user:pass@host:6543/db')).toBe(true); + }); + + test('GBRAIN_PREPARE=0 is falsy', () => { + process.env.GBRAIN_PREPARE = '0'; + expect(resolvePrepare('postgresql://user:pass@host:6543/db')).toBe(false); + }); + + test('returns undefined for malformed URL', () => { + expect(resolvePrepare('not-a-url')).toBeUndefined(); + }); + + test('handles postgres:// scheme (no ql)', () => { + expect(resolvePrepare('postgres://user:pass@host:6543/db')).toBe(false); + }); + + test('handles URL with encoded special chars in password', () => { + expect( + resolvePrepare('postgresql://user:p%40ss%24word@host:6543/db'), + ).toBe(false); + }); +});