diff --git a/src/core/import-file.ts b/src/core/import-file.ts index f988ee1cf..9d161cb2d 100644 --- a/src/core/import-file.ts +++ b/src/core/import-file.ts @@ -899,6 +899,19 @@ export async function importFromContent( } } + // Post-write read-back verification. + // + // After the transaction commits, the page MUST be resolvable via getPage. + // If the read-back returns null (or a stale content_hash), the operation + // fails LOUDLY — a non-zero exit + error surfaced to the ingest log — rather + // than reporting success. A write is not "done" until it is readable. + // + // This catches the silent-desync class: the page file exists on disk (or the + // git commit landed) but the DB index silently never picked it up. Without + // this guard, the operation reports success and the page is invisible to all + // reads (get_page, search, query) until someone notices the gap manually. + await verifyPageReadable(engine, slug, hash, sourceId, 'importFromContent'); + return { slug, status: 'imported', @@ -909,6 +922,66 @@ export async function importFromContent( }; } +/** + * Post-write read-back assertion. + * + * After a page write transaction commits, verify the page is resolvable via + * `getPage` and that its `content_hash` matches the hash we just wrote. If the + * read-back fails (page not found or stale hash), throw a loud error so the + * caller surfaces the failure instead of reporting success. + * + * This is the write-then-verify guard on the sync/write path: a write is not + * "done" until it is readable back. + */ +async function verifyPageReadable( + engine: BrainEngine, + slug: string, + expectedHash: string, + sourceId: string | undefined, + caller: string, +): Promise { + const readBack = await engine.getPage(slug, sourceId ? { sourceId } : undefined); + if (!readBack) { + // Log to ingest_log before throwing so the failure is durable and + // agent-inspectable, not just a transient stderr message. + try { + await engine.logIngest({ + source_type: 'write-verify-guard', + source_ref: slug, + pages_updated: [], + summary: `[${caller}] post-write read-back failed: page '${slug}' not found after write (source: ${sourceId ?? 'default'}). Silent desync — DB index did not pick up the write.`, + ...(sourceId ? { source_id: sourceId } : {}), + }); + } catch { + // Best-effort: don't mask the original failure if logIngest itself fails. + } + throw new Error( + `[${caller}] post-write read-back failed: page '${slug}' not found after write ` + + `(source: ${sourceId ?? 'default'}). The page was written but the DB index ` + + `did not pick it up. This indicates a silent desync — the operation must fail loudly.`, + ); + } + if (readBack.content_hash !== expectedHash) { + try { + await engine.logIngest({ + source_type: 'write-verify-guard', + source_ref: slug, + pages_updated: [], + summary: `[${caller}] post-write read-back failed: page '${slug}' has stale content_hash (expected ${expectedHash.slice(0, 12)}, got ${(readBack.content_hash ?? '').slice(0, 12)}; source: ${sourceId ?? 'default'}). Silent desync — DB index has a stale row.`, + ...(sourceId ? { source_id: sourceId } : {}), + }); + } catch { + // Best-effort. + } + throw new Error( + `[${caller}] post-write read-back failed: page '${slug}' has stale content_hash ` + + `(expected ${expectedHash.slice(0, 12)}, got ${(readBack.content_hash ?? '').slice(0, 12)}; ` + + `source: ${sourceId ?? 'default'}). The page was written but the DB index ` + + `has a stale row. This indicates a silent desync — the operation must fail loudly.`, + ); + } +} + /** * Import from a file path. Validates size, reads content, delegates to importFromContent. * @@ -1202,6 +1275,11 @@ export async function importCodeFile( } }); + // Post-write read-back verification. + // Same guard as the markdown path: a code page write is not "done" until + // it is readable back via getPage. + await verifyPageReadable(engine, slug, hash, sourceId, 'importCodeFile'); + // v0.20.0 Cathedral II Layer 5 (A1): extracted call-site edges persist // in code_edges_symbol (unresolved — we don't attempt within-file target // resolution here; getCallersOf / getCalleesOf match on to_symbol_qualified diff --git a/test/import-file.test.ts b/test/import-file.test.ts index 3e4231154..8c56c465c 100644 --- a/test/import-file.test.ts +++ b/test/import-file.test.ts @@ -8,8 +8,18 @@ import { MARKDOWN_CHUNKER_VERSION } from '../src/core/chunkers/recursive.ts'; const TMP = join(import.meta.dir, '.tmp-import-test'); // Minimal mock engine that tracks calls and supports transaction() +// +// Post-write read-back guard: the mock now simulates a real DB by storing pages written +// via putPage so getPage can read them back. The post-write read-back guard +// in importFromContent calls getPage after the transaction commits — a mock +// that always returns null (the pre-fix default) triggers the guard's +// "page not found after write" path. This is the correct loud-failure +// behavior for a real desync, but for the existing unit tests we need the +// mock to behave like a working DB where writes are readable. function mockEngine(overrides: Partial> = {}): BrainEngine { const calls: { method: string; args: any[] }[] = []; + // In-memory page store: slug → page row (simulates a real DB index). + const pageStore = new Map }>(); const track = (method: string) => (...args: any[]) => { calls.push({ method, args }); if (overrides[method]) return overrides[method](...args); @@ -20,7 +30,49 @@ function mockEngine(overrides: Partial> = {}): BrainEngine { get(_, prop: string) { if (prop === '_calls') return calls; if (prop === 'getTags') return overrides.getTags || (() => Promise.resolve([])); - if (prop === 'getPage') return overrides.getPage || (() => Promise.resolve(null)); + if (prop === 'getPage') { + return async (slug: string, opts?: { sourceId?: string }) => { + // If the test provides a custom getPage, call it first (for the + // "existing" check at the top of importFromContent). If it returns + // a value, use that. If it returns null, fall back to the in-memory + // store (for the read-back guard after the write). + // + // Read-back guard: when the override returns a non-null value, + // we still need the read-back to see the page as written by putPage. + // The override simulates the "existing page" state; the store has + // the post-write state. If the override and the store disagree on + // content_hash, the store wins (the write already committed). + if (overrides.getPage) { + const overrideResult = await overrides.getPage(slug, opts); + if (overrideResult) { + // If the page was written (store has it), the store's hash + // is the post-write hash. Merge: return the override's shape + // but with the store's content_hash (the committed value). + const stored = pageStore.get(slug); + if (stored) { + return { ...overrideResult, content_hash: stored.content_hash }; + } + return overrideResult; + } + } + return pageStore.get(slug) ?? null; + }; + } + if (prop === 'putPage') { + return async (slug: string, page: { content_hash?: string; title?: string; type?: string; frontmatter?: Record }, _opts?: { sourceId?: string }) => { + calls.push({ method: 'putPage', args: [slug, page, _opts] }); + if (overrides.putPage) overrides.putPage(slug, page, _opts); + // Always store the page so getPage can read it back (simulates DB index). + pageStore.set(slug, { + slug, + content_hash: page.content_hash ?? '', + title: page.title ?? '', + type: page.type ?? '', + frontmatter: page.frontmatter ?? {}, + }); + return Promise.resolve(undefined); + }; + } // transaction: just call the fn with the same engine (no real DB transaction in tests) if (prop === 'transaction') return async (fn: (tx: BrainEngine) => Promise) => fn(engine); return track(prop); diff --git a/test/write-verify-guard.test.ts b/test/write-verify-guard.test.ts new file mode 100644 index 000000000..7828d5007 --- /dev/null +++ b/test/write-verify-guard.test.ts @@ -0,0 +1,249 @@ +/** + * Post-write read-back verification tests. + * + * Reproduces the silent-desync case: a page write commits but the DB index + * silently never picks it up. Without the guard, importFromContent reports + * success. With the guard, it fails loudly (throws). + * + * Also verifies the happy path: a normal write passes read-back. + */ + +import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, test } from 'bun:test'; +import * as fs from 'node:fs'; +import * as path from 'node:path'; +import * as os from 'node:os'; +import { PGLiteEngine } from '../src/core/pglite-engine.ts'; +import { resetPgliteState } from './helpers/reset-pglite.ts'; +import { resetGateway } from '../src/core/ai/gateway.ts'; +import { importFromContent } from '../src/core/import-file.ts'; +import { operations } from '../src/core/operations.ts'; +import type { OperationContext } from '../src/core/operations.ts'; + +let engine: PGLiteEngine; +let tmpRoot: string; +let brainDir: string; + +beforeAll(async () => { + engine = new PGLiteEngine(); + await engine.connect({}); + await engine.initSchema(); +}); + +afterAll(async () => { + await engine.disconnect(); + resetGateway(); +}); + +beforeEach(async () => { + await resetPgliteState(engine); + resetGateway(); + tmpRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'gbrain-verify-')); + brainDir = path.join(tmpRoot, 'brain'); + fs.mkdirSync(brainDir, { recursive: true }); + await engine.setConfig('sync.repo_path', brainDir); +}); + +afterEach(() => { + fs.rmSync(tmpRoot, { recursive: true, force: true }); +}); + +describe('post-write read-back verification', () => { + test('normal write passes read-back and returns imported', async () => { + const slug = 'inbox/verify-happy'; + const content = '---\ntitle: Happy\n---\n\n# Body that should round-trip cleanly'; + const result = await importFromContent(engine, slug, content, { + noEmbed: true, + sourceId: 'default', + }); + + expect(result.status).toBe('imported'); + expect(result.slug).toBe(slug); + + // The page is resolvable via getPage in the same operation. + const page = await engine.getPage(slug, { sourceId: 'default' }); + expect(page).not.toBeNull(); + expect(page!.title).toBe('Happy'); + }); + + test('silent desync (page on disk, absent from index) fails loudly', async () => { + // Simulate the desync: write a page, then DELETE it from the DB + // between the transaction and the read-back. In production, this + // happens when the DB index silently fails to pick up the write + // (e.g. a trigger dropped, a partition routing error, or an index + // corruption). The guard must catch this and throw. + // + // We achieve this by wrapping the engine to intercept getPage + // after the transaction and return null (simulating an index miss). + + const slug = 'inbox/verify-desync'; + const content = '---\ntitle: Desync\n---\n\n# Body that will be silently lost'; + + // First, write the page normally to prove it works. + const result1 = await importFromContent(engine, slug, content, { + noEmbed: true, + sourceId: 'default', + }); + expect(result1.status).toBe('imported'); + + // Now simulate the desync: delete the page from the DB, then + // try to write it again. But this time, we intercept getPage + // to return null after the write (simulating an index miss). + // + // We do this by monkey-patching the engine's getPage method + // to return null for this specific slug on the NEXT call + // (which is the read-back). + await engine.executeRaw(`DELETE FROM pages WHERE slug = $1`, [slug]); + // Verify the page is gone (simulating the index miss state). + const gone = await engine.getPage(slug, { sourceId: 'default' }); + expect(gone).toBeNull(); + + // Now write again, but intercept getPage to simulate the desync. + // We need to intercept the read-back call specifically. Since + // importFromContent calls getPage twice (once for existing check + // at the top, once for read-back), we intercept the second call + // for this specific slug. + const slugCallCount = new Map(); + const originalGetPage = engine.getPage.bind(engine); + const interceptingGetPage = async ( + s: string, + opts?: { sourceId?: string }, + ) => { + const count = (slugCallCount.get(s) ?? 0) + 1; + slugCallCount.set(s, count); + // The first getPage call is the "existing" check at the top of + // importFromContent (line 561). The second is the read-back. + // We let the first call through (returning null since we deleted + // the page), but intercept the read-back to return null. + if (s === slug && count === 2) { + return null; // Simulate index miss on read-back. + } + return originalGetPage(s, opts); + }; + engine.getPage = interceptingGetPage as typeof engine.getPage; + + try { + // This should throw — the read-back fails, so the write is not "done". + await expect( + importFromContent(engine, slug, content, { + noEmbed: true, + sourceId: 'default', + }), + ).rejects.toThrow(/post-write read-back failed/); + } finally { + // Restore the original getPage. + engine.getPage = originalGetPage; + } + }); + + test('stale content_hash fails loudly', async () => { + // Simulate a stale hash: write a page, then corrupt the content_hash + // before the read-back. The guard should detect the mismatch. + const slug = 'inbox/verify-stale-hash'; + const content = '---\ntitle: Stale\n---\n\n# Body with stale hash'; + + // Write the page normally first. + await importFromContent(engine, slug, content, { + noEmbed: true, + sourceId: 'default', + }); + + // Corrupt the content_hash to simulate a stale row. + await engine.executeRaw( + `UPDATE pages SET content_hash = $1 WHERE slug = $2`, + ['stale-hash-value-that-does-not-match', slug], + ); + + // Now try to write again. The existing check will find the page + // (with the stale hash), so it won't be skipped. The write will + // commit, but the read-back should detect the stale hash... + // except wait — the write will UPDATE the hash, so the read-back + // will see the NEW hash. We need to intercept getPage to return + // the stale hash instead. + const slugCallCount = new Map(); + const originalGetPage = engine.getPage.bind(engine); + const stalePage = await originalGetPage(slug, { sourceId: 'default' }); + + // Write the stale hash back so the existing check sees a mismatch. + await engine.executeRaw( + `UPDATE pages SET content_hash = $1 WHERE slug = $2`, + ['stale-hash-value-that-does-not-match', slug], + ); + + const interceptingGetPage = async ( + s: string, + opts?: { sourceId?: string }, + ) => { + const count = (slugCallCount.get(s) ?? 0) + 1; + slugCallCount.set(s, count); + // The read-back call (second getPage for this slug) returns the + // stale page with the old hash. + if (s === slug && count === 2 && stalePage) { + return { ...stalePage, content_hash: 'stale-hash-value-that-does-not-match' }; + } + return originalGetPage(s, opts); + }; + engine.getPage = interceptingGetPage as typeof engine.getPage; + + try { + await expect( + importFromContent(engine, slug, content, { + noEmbed: true, + sourceId: 'default', + }), + ).rejects.toThrow(/stale content_hash/); + } finally { + engine.getPage = originalGetPage; + } + }); + + test('put_page operation surfaces read-back failure to the caller', async () => { + // Verify that the put_page MCP operation also surfaces the read-back + // failure (not just importFromContent directly). This is the path + // agents actually call. + const putPage = operations.find((o) => o.name === 'put_page')!; + const slug = 'inbox/verify-putpage-desync'; + const content = '---\ntitle: Desync\n---\n\n# Body that will be silently lost'; + + // Intercept getPage to simulate index miss on the read-back. + // Use a slug-specific counter to avoid interference from other tests + // that might call getPage on the same engine. + // + // The put_page handler calls getPage via importFromContent, which makes + // two getPage calls for this slug: + // 1. importFromContent's existing-page check + // 2. verifyPageReadable's read-back (the one we intercept) + const slugCallCount = new Map(); + const originalGetPage = engine.getPage.bind(engine); + const interceptingGetPage = async ( + s: string, + opts?: { sourceId?: string }, + ) => { + const count = (slugCallCount.get(s) ?? 0) + 1; + slugCallCount.set(s, count); + // Intercept the read-back call (callCount == 2 for this slug). + if (s === slug && count === 2) { + return null; + } + return originalGetPage(s, opts); + }; + engine.getPage = interceptingGetPage as typeof engine.getPage; + + const ctx: OperationContext = { + engine, + config: { engine: 'pglite' as const }, + logger: { info: () => {}, warn: () => {}, error: () => {} }, + dryRun: false, + remote: false, + sourceId: 'default', + agentIdentity: { name: 'Test Agent', email: 'test-agent@example.com' }, + } as OperationContext; + + try { + await expect( + putPage.handler(ctx, { slug, content }), + ).rejects.toThrow(/post-write read-back failed/); + } finally { + engine.getPage = originalGetPage; + } + }); +});