From 0853491eb29e3499e66dd4bf4dfd7952189f6b71 Mon Sep 17 00:00:00 2001 From: Time Attakc <89218912+time-attack@users.noreply.github.com> Date: Thu, 23 Jul 2026 12:55:00 -0700 Subject: [PATCH] fix(schema-pack): narrow stats catch-all so masked errors surface, not fake 0 pages (#2466) (#3133) fetchCountRows and detectDeadPrefixes in src/core/schema-pack/stats.ts swallowed EVERY engine error into empty results, so any real failure printed 'Total pages: 0' + a vacuous 100% coverage on a populated brain. Both catches now swallow only isUndefinedTableError (pre-init brain, missing pages table) and rethrow everything else. Four regression tests: real non-zero count on a populated PGLite brain, rethrow on non-missing- table errors in both catch sites, and the missing-table degrade path. Takeover of #2493. Co-authored-by: Garry Tan Co-authored-by: javieraldape Co-authored-by: Claude Fable 5 --- src/core/schema-pack/stats.ts | 23 +++++++--- test/schema-pack-stats.test.ts | 83 ++++++++++++++++++++++++++++++++++ 2 files changed, 100 insertions(+), 6 deletions(-) diff --git a/src/core/schema-pack/stats.ts b/src/core/schema-pack/stats.ts index c3ad89c57..f1fa2e26b 100644 --- a/src/core/schema-pack/stats.ts +++ b/src/core/schema-pack/stats.ts @@ -18,6 +18,7 @@ import type { BrainEngine } from '../engine.ts'; import { loadActivePackBestEffort } from './best-effort.ts'; import type { OperationContext } from '../operations.ts'; +import { isUndefinedTableError } from '../utils.ts'; export interface StatsOpts { /** Single source scope. Omit + omit sourceIds for whole-brain aggregate. */ @@ -164,9 +165,17 @@ async function fetchCountRows(engine: BrainEngine, opts: StatsOpts): Promise(sql, params); - } catch { - // Empty / pre-init brain: pages table may not exist yet. - return []; + } catch (err) { + // ONLY swallow the genuine "pages table doesn't exist yet" case + // (empty / pre-init brain). #2466: the old bare `catch {}` masked + // EVERY error — so any engine-level failure (connection, version + // skew, a query incompatibility) was silently converted to 0 rows, + // printing "Total pages: 0" on a populated brain and cascading into + // false "100% coverage" + a starved `schema suggest`. Surface + // everything that is not a missing-table error so the real failure + // is visible instead of hidden behind a fake zero. + if (isUndefinedTableError(err)) return []; + throw err; } } @@ -204,9 +213,11 @@ async function detectDeadPrefixes( if (cnt === 0) { hints.push({ type: t.name, prefix }); } - } catch { - // Skip on engine error (no pages table yet, etc.). - continue; + } catch (err) { + // #2466: only skip on the genuine "no pages table yet" case; + // rethrow any other engine error so it isn't silently masked. + if (isUndefinedTableError(err)) continue; + throw err; } } } diff --git a/test/schema-pack-stats.test.ts b/test/schema-pack-stats.test.ts index 75df4e973..a27694c6c 100644 --- a/test/schema-pack-stats.test.ts +++ b/test/schema-pack-stats.test.ts @@ -231,6 +231,89 @@ describe('runStatsCore — JSON envelope shape', () => { }); }); +describe('runStatsCore — #2466 catch-narrowing (real count + error surfacing)', () => { + // #2466: `gbrain schema stats` reported "Total pages: 0" on a populated + // PGLite brain. The bug was a bare `catch {}` in fetchCountRows (and a + // sibling in detectDeadPrefixes) that converted ANY engine error into 0 + // rows. The COUNT query itself is valid on PGLite (proven below), so the + // regression pins two things: (a) a populated brain reports the real, + // non-zero count through the full runStatsCore path; (b) a non-missing- + // table engine error is rethrown, not masked into a fake zero. + + it('reports the real non-zero count on a populated PGLite brain (no false 0)', async () => { + await withEnv({ GBRAIN_SCHEMA_PACK: undefined }, async () => { + // Seed a realistic mix: typed, untyped, multiple types — like the + // 169-page brain in the bug report (scaled down). + for (let i = 0; i < 12; i++) { + const type = i % 3 === 0 ? '' : (i % 3 === 1 ? 'person' : 'company'); + await seedPage(`notes/p${i}`, { type, sourcePath: `notes/p${i}.md` }); + } + const result = await runStatsCore(ctxOf()); + // The core regression: NOT zero. + expect(result.aggregate.total_pages).toBe(12); + expect(result.aggregate.typed_pages).toBe(8); + expect(result.aggregate.untyped_pages).toBe(4); + // And coverage is the honest ratio, not the vacuous 1.0 a 0/0 prints. + expect(result.aggregate.coverage).not.toBe(1.0); + }); + }); + + it('fetchCountRows rethrows a non-missing-table engine error instead of masking it as 0 pages', async () => { + await withEnv({ GBRAIN_SCHEMA_PACK: undefined }, async () => { + // No pack → detectDeadPrefixes is skipped, isolating the throw to the + // fetchCountRows catch we narrowed. The count query (the GROUP BY one) + // throws a column-level error (SQLSTATE 42703) — the exact class the + // old bare `catch {}` swallowed into 0 rows; everything else succeeds. + __setPackLocatorForTests(() => null); + const boom = Object.assign(new Error('column "type" does not exist'), { code: '42703' }); + const stubEngine = { + executeRaw: async (sql: string) => { + if (/GROUP BY source_id/.test(sql)) throw boom; // the fetchCountRows query + return []; + }, + } as unknown as PGLiteEngine; + const ctx = { ...ctxOf(), engine: stubEngine } as unknown as OperationContext; + await expect(runStatsCore(ctx)).rejects.toThrow('column "type" does not exist'); + }); + }); + + it('fetchCountRows still degrades to empty (no throw) on a genuine missing pages table', async () => { + await withEnv({ GBRAIN_SCHEMA_PACK: undefined }, async () => { + // Pre-init brain shape: the count query hits a missing pages table + // (SQLSTATE 42P01). This is the ONLY case the narrowed catch swallows. + __setPackLocatorForTests(() => null); + const missing = Object.assign(new Error('relation "pages" does not exist'), { code: '42P01' }); + const stubEngine = { + executeRaw: async (sql: string) => { + if (/GROUP BY source_id/.test(sql)) throw missing; + return []; + }, + } as unknown as PGLiteEngine; + const ctx = { ...ctxOf(), engine: stubEngine } as unknown as OperationContext; + const result = await runStatsCore(ctx); + expect(result.aggregate.total_pages).toBe(0); + expect(result.per_source).toEqual([]); + }); + }); + + it('detectDeadPrefixes rethrows a non-missing-table error (sibling catch)', async () => { + await withEnv({ GBRAIN_HOME: tmpDir, GBRAIN_SCHEMA_PACK: 'tiny' }, async () => { + seedTinyPack('tiny', [{ name: 'person', prefix: 'people/' }]); + // fetchCountRows (the GROUP BY query) succeeds → []; the per-prefix + // dead-prefix LIKE query then throws a non-missing-table error, which + // must surface through the narrowed sibling catch. + const stubEngine = { + executeRaw: async (sql: string) => { + if (/GROUP BY source_id/.test(sql)) return []; // count query: empty brain, fine + throw Object.assign(new Error('division by zero'), { code: '22012' }); // the LIKE query + }, + } as unknown as PGLiteEngine; + const ctx = { ...ctxOf(), engine: stubEngine } as unknown as OperationContext; + await expect(runStatsCore(ctx)).rejects.toThrow('division by zero'); + }); + }); +}); + describe('runStatsCore — type/untyped split', () => { it('treats empty-string type as untyped (not its own bucket)', async () => { await withEnv({ GBRAIN_SCHEMA_PACK: undefined }, async () => {