From 33a842a7cce6df7d88dcdacf1c0f8a48ff159a68 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Wed, 24 Jun 2026 15:19:14 -0700 Subject: [PATCH] fix(files): upload-raw persists small files + source-namespaced storage (#2297) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The small-file branch printed success and returned without copying the file into the brain repo or inserting a files row — every small text/PDF raw was silently lost. Both branches now copy into the repo and record a row via one shared writer. storage_path (globally UNIQUE) is source-namespaced so the same page slug in two sources can't ON CONFLICT-clobber each other; the physical copy stays repo-relative under the source's own repo root. A new --source flag strictly disambiguates a slug present in multiple sources (a scoped miss returns null, never falls back unscoped). Adds test/silent-failure-wave.test.ts covering all four wave fixes. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/commands/files.ts | 235 ++++++++++++++++++++++++++----- test/silent-failure-wave.test.ts | 212 ++++++++++++++++++++++++++++ 2 files changed, 409 insertions(+), 38 deletions(-) create mode 100644 test/silent-failure-wave.test.ts diff --git a/src/commands/files.ts b/src/commands/files.ts index a8990bfdd..ee2bb75f6 100644 --- a/src/commands/files.ts +++ b/src/commands/files.ts @@ -86,7 +86,7 @@ export async function runFiles(engine: BrainEngine, args: string[]) { console.error(`Usage: gbrain files [args]`); console.error(` list [slug] List files for a page (or all)`); console.error(` upload --page Upload file linked to page`); - console.error(` upload-raw --page [--type ] Smart upload with .redirect.yaml pointer`); + console.error(` upload-raw --page [--source ] [--type ] Smart upload with .redirect.yaml pointer`); console.error(` signed-url Generate signed URL for stored file`); console.error(` sync Upload directory to storage`); console.error(` verify Verify all uploads match local`); @@ -170,23 +170,126 @@ async function uploadFile(engine: BrainEngine, args: string[]) { console.log(`Uploaded: ${storagePath} (${humanSize(stat.size)})`); } +/** + * Resolve the on-disk brain repo root for a source (#2297). Multi-source: a + * source's `local_path` is its checkout; the legacy/default source falls back + * to the `sync.repo_path` config anchor written by `gbrain sync`. Returns null + * when no repo is resolvable (pure-DB brain) — callers must fail loud rather + * than silently claim a git write succeeded. + */ +export async function resolveRepoRoot(engine: BrainEngine, sourceId: string | null): Promise { + if (sourceId && sourceId !== 'default') { + const rows = await engine.executeRaw<{ local_path: string | null }>( + `SELECT local_path FROM sources WHERE id = $1`, + [sourceId], + ); + if (rows[0]?.local_path) return rows[0].local_path; + } + return await engine.getConfig('sync.repo_path'); +} + +/** + * Look up a page by slug to link the file row to a real page + source (#2297). + * Slug uniqueness is (source_id, slug); with only `--page ` we take the + * first live match. Returns null for a slug with no page (file row still records + * the slug + the default source). + */ +export async function lookupPageBySlug( + engine: BrainEngine, + pageSlug: string, + sourceHint?: string | null, +): Promise<{ id: number; source_id: string } | null> { + // Slug uniqueness is (source_id, slug), so a bare slug can match multiple + // sources (#2297). An explicit --source is honored STRICTLY: a scoped miss + // returns null (never falls back to an unscoped match, which could silently + // attach the file to the wrong source). Only a bare slug (no hint) takes the + // first live match deterministically (id ASC). + if (sourceHint) { + const scoped = await engine.executeRaw<{ id: number; source_id: string }>( + `SELECT id, source_id FROM pages WHERE slug = $1 AND source_id = $2 AND deleted_at IS NULL ORDER BY id LIMIT 1`, + [pageSlug, sourceHint], + ); + return scoped[0] ?? null; + } + const rows = await engine.executeRaw<{ id: number; source_id: string }>( + `SELECT id, source_id FROM pages WHERE slug = $1 AND deleted_at IS NULL ORDER BY id LIMIT 1`, + [pageSlug], + ); + return rows[0] ?? null; +} + +/** + * Build the globally-unique `files.storage_path` key (#2297). The schema's + * UNIQUE is on storage_path alone, but slugs only disambiguate within a source, + * so a bare `/` collides across sources. Prefix non-default sources + * with the source id so two sources can hold the same slug+filename without one + * ON CONFLICT-clobbering the other. Default-source paths stay byte-identical + * (back-compat). NOTE: this is the DB key, not the on-disk path — the physical + * copy lives at repoRelPath under the source's own repo root. + */ +export function namespacedStoragePath(sourceId: string | null, repoRelPath: string): string { + return sourceId && sourceId !== 'default' ? `${sourceId}/${repoRelPath}` : repoRelPath; +} + +/** + * Single canonical `files` row writer shared by BOTH the git (small-file) and + * cloud (large/media) branches of upload-raw (#2297, DRY). `storage_path` is + * UNIQUE in the schema, so it MUST be page-namespaced by the caller to avoid one + * page's attachment clobbering another's. metadata goes through executeRawJsonb + * (raw object, never JSON.stringify into a ::jsonb cast — the double-encode trap). + */ +export async function insertFileRow( + engine: BrainEngine, + row: { + sourceId: string | null; + pageId: number | null; + pageSlug: string | null; + filename: string; + storagePath: string; + mimeType: string | null; + size: number; + contentHash: string; + metadata: Record; + }, +): Promise { + await executeRawJsonb( + engine, + `INSERT INTO files (source_id, page_id, page_slug, filename, storage_path, mime_type, size_bytes, content_hash, metadata) + VALUES (COALESCE($1, 'default'), $2, $3, $4, $5, $6, $7, $8, $9::jsonb) + ON CONFLICT (storage_path) DO UPDATE SET + content_hash = EXCLUDED.content_hash, + size_bytes = EXCLUDED.size_bytes, + mime_type = EXCLUDED.mime_type, + page_id = EXCLUDED.page_id, + page_slug = EXCLUDED.page_slug`, + [row.sourceId, row.pageId, row.pageSlug, row.filename, row.storagePath, row.mimeType, row.size, row.contentHash], + [row.metadata], + ); +} + /** * Smart upload with size routing and .redirect.yaml pointer creation. * * Size routing: - * < 100 MB text/PDF → stays in git (brain repo), no cloud upload + * < 100 MB text/PDF → copied into the brain repo under /.raw/, recorded + * in the files table (storage = 'git') * >= 100 MB OR media → upload to cloud storage, create .redirect.yaml pointer + * in the repo, recorded in the files table (storage = cloud) * - * The .redirect.yaml pointer stays in the brain repo so git tracks what was stored. + * Both branches persist a files row AND write into the brain repo so git tracks + * what was stored. Neither silently returns success without persisting (#2297). */ async function uploadRaw(engine: BrainEngine, args: string[]) { const filePath = args.find(a => !a.startsWith('--')); const pageSlug = args.find((a, i) => args[i - 1] === '--page') || null; const fileType = args.find((a, i) => args[i - 1] === '--type') || null; + // --source disambiguates a slug that exists in more than one source (#2297); + // without it, a bare slug resolves to the first matching source. + const sourceHint = args.find((a, i) => args[i - 1] === '--source') || null; const noPointer = args.includes('--no-pointer'); if (!filePath || !existsSync(filePath)) { - console.error('Usage: gbrain files upload-raw --page [--type ] [--no-pointer]'); + console.error('Usage: gbrain files upload-raw --page [--source ] [--type ] [--no-pointer]'); process.exit(1); } @@ -197,13 +300,56 @@ async function uploadRaw(engine: BrainEngine, args: string[]) { const needsCloud = stat.size >= SIZE_THRESHOLD || isMedia; if (!needsCloud) { - // Small text/PDF files stay in git + // Small text/PDF files are copied INTO the brain repo and recorded in the + // files table (#2297). Previously this branch printed success and returned + // without copying anything or inserting a row — every small "raw" was + // silently lost. Resolve the page + source so the row links correctly and + // the destination is page-namespaced (storage_path is UNIQUE). + const page = pageSlug ? await lookupPageBySlug(engine, pageSlug, sourceHint) : null; + const sourceId = page?.source_id ?? sourceHint ?? 'default'; + const repoRoot = await resolveRepoRoot(engine, sourceId); + if (!repoRoot) { + console.error(JSON.stringify({ + success: false, + reason: 'no_repo_path', + message: 'No brain repo on disk (sources.local_path / sync.repo_path unset). ' + + 'Run `gbrain sync` to anchor a repo, or configure cloud storage for raw uploads.', + })); + process.exit(1); + } + const content = readFileSync(filePath); + const hash = createHash('sha256').update(content).digest('hex'); + // Physical copy lives at the repo-relative path UNDER the source's own repo + // root; the DB storage_path key is additionally source-namespaced so it stays + // globally unique across sources (#2297). + const repoRelPath = pageSlug + ? `${pageSlug}/.raw/${filename}` + : `unsorted/.raw/${hash.slice(0, 8)}-${filename}`; + const storagePath = namespacedStoragePath(sourceId, repoRelPath); + const destAbs = join(repoRoot, repoRelPath); + mkdirSync(dirname(destAbs), { recursive: true }); + writeFileSync(destAbs, content); + + await insertFileRow(engine, { + sourceId, + pageId: page?.id ?? null, + pageSlug, + filename, + storagePath, + mimeType, + size: stat.size, + contentHash: 'sha256:' + hash, + metadata: { ...(fileType ? { type: fileType } : {}), upload_method: 'git' }, + }); + console.log(JSON.stringify({ success: true, storage: 'git', - path: filePath, + path: storagePath, + abs_path: destAbs, size: stat.size, size_human: humanSize(stat.size), + hash: `sha256:${hash}`, })); return; } @@ -221,48 +367,61 @@ async function uploadRaw(engine: BrainEngine, args: string[]) { const storage = await createStorage(config.storage as any); const content = readFileSync(filePath); const hash = createHash('sha256').update(content).digest('hex'); - const storagePath = pageSlug ? `${pageSlug}/${filename}` : `unsorted/${hash.slice(0, 8)}-${filename}`; const bucket = (config.storage as any).bucket || 'brain-files'; + const page = pageSlug ? await lookupPageBySlug(engine, pageSlug, sourceHint) : null; + const sourceId = page?.source_id ?? sourceHint ?? 'default'; + // Source-namespace the cloud storage key so the same slug+filename in two + // sources doesn't collide on the UNIQUE storage_path (#2297). Default source + // keeps its historical path. + const cloudRelPath = pageSlug ? `${pageSlug}/${filename}` : `unsorted/${hash.slice(0, 8)}-${filename}`; + const storagePath = namespacedStoragePath(sourceId, cloudRelPath); + const method = content.length >= SIZE_THRESHOLD ? 'TUS resumable' : 'standard'; console.error(`Uploading ${humanSize(stat.size)} via ${method}...`); await storage.upload(storagePath, content, mimeType || undefined); - // Create .redirect.yaml pointer in the brain repo + // Create .redirect.yaml pointer IN THE BRAIN REPO, page-namespaced (#2297). + // Previously this was written next to the *input* file (filePath + + // '.redirect.yaml'), so the pointer never landed in the repo the comment + // claimed — git tracked nothing. Resolve the repo root and write it under + // the page's .raw/ sidecar so it travels with the page. let pointerPath: string | null = null; if (!noPointer && pageSlug) { - const { stringify } = await import('../core/yaml-lite.ts'); - const pointer = stringify({ - target: `supabase://${bucket}/${storagePath}`, - bucket, - storage_path: storagePath, - size: stat.size, - size_human: humanSize(stat.size), - hash: `sha256:${hash}`, - mime: mimeType || 'application/octet-stream', - uploaded: new Date().toISOString(), - ...(fileType ? { type: fileType } : {}), - }); - // Write pointer next to the original file - pointerPath = filePath + '.redirect.yaml'; - writeFileSync(pointerPath, pointer); - console.error(`Pointer written: ${pointerPath}`); + const repoRoot = await resolveRepoRoot(engine, sourceId); + if (repoRoot) { + const { stringify } = await import('../core/yaml-lite.ts'); + const pointer = stringify({ + target: `supabase://${bucket}/${storagePath}`, + bucket, + storage_path: storagePath, + size: stat.size, + size_human: humanSize(stat.size), + hash: `sha256:${hash}`, + mime: mimeType || 'application/octet-stream', + uploaded: new Date().toISOString(), + ...(fileType ? { type: fileType } : {}), + }); + pointerPath = join(repoRoot, pageSlug, '.raw', `${filename}.redirect.yaml`); + mkdirSync(dirname(pointerPath), { recursive: true }); + writeFileSync(pointerPath, pointer); + console.error(`Pointer written: ${pointerPath}`); + } else { + console.error('No brain repo on disk — skipping .redirect.yaml pointer (cloud upload + DB row still recorded).'); + } } - // Record in DB. files.metadata is JSONB — pass the object via - // executeRawJsonb with an explicit ::jsonb cast so post-v0.31 reads see - // an actual object, not a JSON-encoded string (D1 wave). - await executeRawJsonb( - engine, - `INSERT INTO files (page_slug, filename, storage_path, mime_type, size_bytes, content_hash, metadata) - VALUES ($1, $2, $3, $4, $5, $6, $7::jsonb) - ON CONFLICT (storage_path) DO UPDATE SET - content_hash = EXCLUDED.content_hash, - size_bytes = EXCLUDED.size_bytes, - mime_type = EXCLUDED.mime_type`, - [pageSlug, filename, storagePath, mimeType, stat.size, 'sha256:' + hash], - [{ type: fileType, upload_method: method }], - ); + await insertFileRow(engine, { + sourceId, + pageId: page?.id ?? null, + pageSlug, + filename, + storagePath, + mimeType, + size: stat.size, + contentHash: 'sha256:' + hash, + metadata: { ...(fileType ? { type: fileType } : {}), upload_method: method }, + }); // Output JSON for scripting console.log(JSON.stringify({ diff --git a/test/silent-failure-wave.test.ts b/test/silent-failure-wave.test.ts new file mode 100644 index 000000000..e98147394 --- /dev/null +++ b/test/silent-failure-wave.test.ts @@ -0,0 +1,212 @@ +/** + * Regression tests for the silent-failure cleanup wave (post-#2375). + * + * - #2251 updateSourceConfig array-branch guard (jsonb_each on a non-object) + * - #2330 getStats vs getHealth page-count parity (both exclude soft-deleted) + * - #2297 files upload-raw persistence helpers (page-namespaced, no clobber) + * + * Hermetic in-memory PGLite; Postgres parity covered by the e2e suite. The + * #2251 + #2330 fixes also land in postgres-engine.ts in lockstep. + */ +import { describe, test, expect, beforeAll, afterAll, beforeEach } from 'bun:test'; +import { PGLiteEngine } from '../src/core/pglite-engine.ts'; +import { resetPgliteState } from './helpers/reset-pglite.ts'; +import { resolveRepoRoot, lookupPageBySlug, insertFileRow, namespacedStoragePath } from '../src/commands/files.ts'; +import { categorizeCheck } from '../src/core/doctor-categories.ts'; + +let engine: PGLiteEngine; + +beforeAll(async () => { + engine = new PGLiteEngine(); + await engine.connect({}); + await engine.initSchema(); +}); + +afterAll(async () => { + await engine.disconnect(); +}); + +beforeEach(async () => { + await resetPgliteState(engine); +}); + +async function seedSource(id: string, localPath: string | null): Promise { + await engine.executeRaw( + `INSERT INTO sources (id, name, local_path, config, created_at) + VALUES ($1, $2, $3, '{}'::jsonb, NOW()) + ON CONFLICT (id) DO UPDATE SET local_path = EXCLUDED.local_path`, + [id, id, localPath], + ); +} + +async function seedPage(slug: string, opts: { deleted?: boolean; sourceId?: string } = {}): Promise { + const rows = await engine.executeRaw<{ id: number }>( + `INSERT INTO pages (source_id, slug, type, title, deleted_at) + VALUES ($1, $2, 'note', $2, ${opts.deleted ? 'NOW()' : 'NULL'}) + RETURNING id`, + [opts.sourceId ?? 'default', slug], + ); + return rows[0].id; +} + +describe('#2251 — updateSourceConfig array-branch guard', () => { + test('coerces a corrupted array config (with a non-object element) instead of throwing', async () => { + await seedSource('corrupt-arr', '/tmp/corrupt-arr'); + // Force the historical bad shape: a JSONB ARRAY whose elements include a + // non-object scalar. Pre-fix, the array branch ran jsonb_each() on the + // string element and the whole UPDATE aborted with + // "cannot call jsonb_each on a non-object". + await engine.executeRaw( + `UPDATE sources SET config = '["stray-string", {"federated": true}]'::jsonb WHERE id = $1`, + ['corrupt-arr'], + ); + + // Must NOT throw, and must return true (row matched). + const ok = await engine.updateSourceConfig('corrupt-arr', { tracked_branch: 'main' }); + expect(ok).toBe(true); + + const rows = await engine.executeRaw<{ typ: string; cfg: Record }>( + `SELECT jsonb_typeof(config) AS typ, config AS cfg FROM sources WHERE id = $1`, + ['corrupt-arr'], + ); + expect(rows[0].typ).toBe('object'); + // Object element flattened, scalar element skipped, patch merged. + expect(rows[0].cfg.federated).toBe(true); + expect(rows[0].cfg.tracked_branch).toBe('main'); + }); + + test('still coerces a double-encoded string config', async () => { + await seedSource('corrupt-str', '/tmp/corrupt-str'); + await engine.executeRaw( + `UPDATE sources SET config = to_jsonb('{"github_repo":"a/b"}'::text) WHERE id = $1`, + ['corrupt-str'], + ); + const ok = await engine.updateSourceConfig('corrupt-str', { federated: false }); + expect(ok).toBe(true); + const rows = await engine.executeRaw<{ typ: string; cfg: Record }>( + `SELECT jsonb_typeof(config) AS typ, config AS cfg FROM sources WHERE id = $1`, + ['corrupt-str'], + ); + expect(rows[0].typ).toBe('object'); + expect(rows[0].cfg.github_repo).toBe('a/b'); + expect(rows[0].cfg.federated).toBe(false); + }); +}); + +describe('#2330 — skill_surface check is categorized as skill (not vacuous meta)', () => { + test('the focused-doctor skill_surface check lands in the skill category', () => { + // Without this wiring the explicit skill_surface check would fall through to + // meta, leaving category_scores.skill a vacuous 100 — the exact dishonesty + // #2330 reports. categorizeCheck must map it to skill. + expect(categorizeCheck('skill_surface')).toBe('skill'); + }); +}); + +describe('#2330 — getStats and getHealth agree on page_count (both exclude soft-deleted)', () => { + test('soft-deleted pages are excluded from BOTH surfaces', async () => { + await seedPage('live/one'); + await seedPage('live/two'); + await seedPage('gone/three', { deleted: true }); + + const stats = await engine.getStats(); + const health = await engine.getHealth(); + + expect(stats.page_count).toBe(2); + expect(health.page_count).toBe(2); + expect(health.page_count).toBe(stats.page_count); + }); +}); + +describe('#2297 — upload-raw persistence helpers', () => { + test('resolveRepoRoot prefers source.local_path, falls back to sync.repo_path', async () => { + await seedSource('teamco', '/repos/teamco'); + expect(await resolveRepoRoot(engine, 'teamco')).toBe('/repos/teamco'); + + await engine.setConfig('sync.repo_path', '/repos/default'); + expect(await resolveRepoRoot(engine, 'default')).toBe('/repos/default'); + expect(await resolveRepoRoot(engine, null)).toBe('/repos/default'); + }); + + test('lookupPageBySlug returns the live page id + source, honoring the source hint', async () => { + const id = await seedPage('people/alice-example'); + const found = await lookupPageBySlug(engine, 'people/alice-example'); + expect(found?.id).toBe(id); + expect(found?.source_id).toBe('default'); + expect(await lookupPageBySlug(engine, 'people/nobody')).toBeNull(); + + // Same slug in a second source → the hint disambiguates (#2297). + await seedSource('teamco', '/repos/teamco'); + const teamId = await seedPage('people/alice-example', { sourceId: 'teamco' }); + const hinted = await lookupPageBySlug(engine, 'people/alice-example', 'teamco'); + expect(hinted?.id).toBe(teamId); + expect(hinted?.source_id).toBe('teamco'); + + // Explicit --source is STRICT: a scoped miss returns null even though the + // slug exists in `default` — no silent cross-source fallback. + expect(await lookupPageBySlug(engine, 'people/alice-example', 'no-such-source')).toBeNull(); + }); + + test('namespacedStoragePath keeps default-source paths but isolates other sources (#2297)', async () => { + // Default source: byte-identical to the historical path. + expect(namespacedStoragePath('default', 'people/alice/.raw/cv.pdf')).toBe('people/alice/.raw/cv.pdf'); + expect(namespacedStoragePath(null, 'people/alice/.raw/cv.pdf')).toBe('people/alice/.raw/cv.pdf'); + // Non-default source: prefixed so the same slug+filename can't collide on + // the globally-UNIQUE storage_path. + expect(namespacedStoragePath('teamco', 'people/alice/.raw/cv.pdf')).toBe('teamco/people/alice/.raw/cv.pdf'); + + // End to end: two sources, same slug+filename → two distinct rows. + await seedSource('src-a', '/repos/a'); + await seedSource('src-b', '/repos/b'); + const aId = await seedPage('shared/slug', { sourceId: 'src-a' }); + const bId = await seedPage('shared/slug', { sourceId: 'src-b' }); + await insertFileRow(engine, { + sourceId: 'src-a', pageId: aId, pageSlug: 'shared/slug', filename: 'f.pdf', + storagePath: namespacedStoragePath('src-a', 'shared/slug/.raw/f.pdf'), + mimeType: 'application/pdf', size: 1, contentHash: 'sha256:a', metadata: {}, + }); + await insertFileRow(engine, { + sourceId: 'src-b', pageId: bId, pageSlug: 'shared/slug', filename: 'f.pdf', + storagePath: namespacedStoragePath('src-b', 'shared/slug/.raw/f.pdf'), + mimeType: 'application/pdf', size: 2, contentHash: 'sha256:b', metadata: {}, + }); + const rows = await engine.executeRaw<{ n: number }>(`SELECT count(*)::int AS n FROM files`); + expect(rows[0].n).toBe(2); + }); + + test('insertFileRow persists a row; page-namespaced paths do not clobber across pages', async () => { + const aliceId = await seedPage('people/alice-example'); + const bobId = await seedPage('people/bob-example'); + + // Same filename, different page → page-namespaced storage_path keeps both. + await insertFileRow(engine, { + sourceId: 'default', pageId: aliceId, pageSlug: 'people/alice-example', + filename: 'resume.pdf', storagePath: 'people/alice-example/.raw/resume.pdf', + mimeType: 'application/pdf', size: 10, contentHash: 'sha256:a', metadata: { upload_method: 'git' }, + }); + await insertFileRow(engine, { + sourceId: 'default', pageId: bobId, pageSlug: 'people/bob-example', + filename: 'resume.pdf', storagePath: 'people/bob-example/.raw/resume.pdf', + mimeType: 'application/pdf', size: 20, contentHash: 'sha256:b', metadata: { upload_method: 'git' }, + }); + + const rows = await engine.executeRaw<{ n: number }>(`SELECT count(*)::int AS n FROM files`); + expect(rows[0].n).toBe(2); + + // Re-uploading the SAME storage_path upserts (no duplicate), and metadata + // round-trips as a real JSONB object (not a double-encoded string). + await insertFileRow(engine, { + sourceId: 'default', pageId: aliceId, pageSlug: 'people/alice-example', + filename: 'resume.pdf', storagePath: 'people/alice-example/.raw/resume.pdf', + mimeType: 'application/pdf', size: 99, contentHash: 'sha256:a2', metadata: { upload_method: 'git', type: 'cv' }, + }); + const after = await engine.executeRaw<{ n: number; size_bytes: number; meta_typ: string }>( + `SELECT count(*)::int AS n, + max(size_bytes) AS size_bytes, + jsonb_typeof((SELECT metadata FROM files WHERE storage_path = 'people/alice-example/.raw/resume.pdf')) AS meta_typ + FROM files`, + ); + expect(after[0].n).toBe(2); // upsert, not insert + expect(Number(after[0].size_bytes)).toBe(99); // updated row + expect(after[0].meta_typ).toBe('object'); // metadata is a real object + }); +});