From 23003a216365029b645247d75d28cf13f8b792fc Mon Sep 17 00:00:00 2001 From: Javier Aldape Date: Fri, 31 Jul 2026 15:03:00 -0600 Subject: [PATCH] fix(facts): preserve remote fence writes (#3659) Co-authored-by: gbrain contributor Co-authored-by: Time Attakc <89218912+time-attack@users.noreply.github.com> --- src/core/import-file.ts | 42 ++++++++++++++++++++ src/core/pglite-engine.ts | 14 +++++-- src/core/postgres-engine.ts | 8 +++- test/e2e/system-of-record-invariant.test.ts | 44 ++++++++++++++++++++- test/insert-facts-batch.test.ts | 36 ++++++++--------- 5 files changed, 117 insertions(+), 27 deletions(-) diff --git a/src/core/import-file.ts b/src/core/import-file.ts index 4aa6f2460..8752863c3 100644 --- a/src/core/import-file.ts +++ b/src/core/import-file.ts @@ -39,6 +39,7 @@ import { normalizeAliasList } from './search/alias-normalize.ts'; import { isUndefinedTableError, warnOncePerProcess, validateSlug } from './utils.ts'; import { computeCorpusGeneration } from './contextual-retrieval-service.ts'; import { runGuardrails } from './guardrails.ts'; +import { FACTS_FENCE_BEGIN, FACTS_FENCE_END, parseFactsFence } from './facts-fence.ts'; /** * v0.20.0 Cathedral II Layer 8 D2 — markdown fence extraction helper. @@ -104,6 +105,27 @@ function fenceTagToPseudoPath(lang: string | undefined): string | null { */ const MAX_FENCES_PER_PAGE = Number.parseInt(process.env.GBRAIN_MAX_FENCES_PER_PAGE || '100', 10); +function extractFactsFenceBlock(body: string): string | null { + const beginIdx = body.indexOf(FACTS_FENCE_BEGIN); + if (beginIdx === -1) return null; + const endIdx = body.indexOf(FACTS_FENCE_END, beginIdx + FACTS_FENCE_BEGIN.length); + if (endIdx === -1) return null; + return body.slice(beginIdx, endIdx + FACTS_FENCE_END.length); +} + +function replaceOrAppendFactsFence(body: string, fenceBlock: string): string { + const beginIdx = body.indexOf(FACTS_FENCE_BEGIN); + if (beginIdx !== -1) { + const endIdx = body.indexOf(FACTS_FENCE_END, beginIdx + FACTS_FENCE_BEGIN.length); + if (endIdx !== -1) { + return body.slice(0, beginIdx) + fenceBlock + body.slice(endIdx + FACTS_FENCE_END.length); + } + } + + const sep = body.endsWith('\n') ? '\n' : '\n\n'; + return `${body}${sep}## Facts\n\n${fenceBlock}\n`; +} + /** * Walk the marked lexer output and extract recognizable code fences. * Returns one ChunkInput per fence whose language tag maps to a grammar @@ -548,6 +570,26 @@ export async function importFromContent( // hash-match skip) and (b) the hash short-circuit below reuses this row. const existing = await engine.getPage(slug, sourceId ? { sourceId } : undefined); + // #2044: remote get_page intentionally strips private facts rows. A + // documented get_page -> edit -> put_page round-trip can therefore arrive + // with an empty/missing Facts fence even though the existing page still has + // canonical fence rows. Preserve the old fence in that narrow case so the + // system-of-record markdown is not truncated by the privacy boundary. + if (opts.remote === true && existing?.compiled_truth) { + const incomingFacts = parseFactsFence(parsed.compiled_truth); + const existingFacts = parseFactsFence(existing.compiled_truth); + const existingFenceBlock = extractFactsFenceBlock(existing.compiled_truth); + if ( + incomingFacts.facts.length === 0 && + incomingFacts.warnings.length === 0 && + existingFacts.warnings.length === 0 && + existingFacts.facts.length > 0 && + existingFenceBlock + ) { + parsed.compiled_truth = replaceOrAppendFactsFence(parsed.compiled_truth, existingFenceBlock); + } + } + // #1035: absence of an explicit frontmatter `type:` on an EXISTING page // means "preserve the stored type", not "re-infer". Pre-fix, a round-trip // put (get_page → edit body → put_page without `type:`) silently regressed diff --git a/src/core/pglite-engine.ts b/src/core/pglite-engine.ts index f86bb2394..64e139f60 100644 --- a/src/core/pglite-engine.ts +++ b/src/core/pglite-engine.ts @@ -4309,7 +4309,11 @@ export class PGLiteEngine implements BrainEngine { $14, $15, $16, $17, $18, $19, $20 - ) RETURNING id` + ) + ON CONFLICT (source_id, source_markdown_slug, row_num) + WHERE row_num IS NOT NULL + DO NOTHING + RETURNING id` : `INSERT INTO facts ( source_id, entity_slug, fact, kind, visibility, notability, context, valid_from, valid_until, source, source_session, confidence, @@ -4323,12 +4327,16 @@ export class PGLiteEngine implements BrainEngine { $15, $16, $17, $18, $19, $20, $21 - ) RETURNING id`, + ) + ON CONFLICT (source_id, source_markdown_slug, row_num) + WHERE row_num IS NOT NULL + DO NOTHING + RETURNING id`, embedStr === null ? [ctx.source_id, entitySlug, input.fact, kind, visibility, notability, context, validFrom, validUntil, input.source, sourceSession, confidence, embeddedAt, input.row_num, input.source_markdown_slug, claimMetric, claimValue, claimUnit, claimPeriod, eventType] : [ctx.source_id, entitySlug, input.fact, kind, visibility, notability, context, validFrom, validUntil, input.source, sourceSession, confidence, embedStr, embeddedAt, input.row_num, input.source_markdown_slug, claimMetric, claimValue, claimUnit, claimPeriod, eventType], ); - out.push(ins.rows[0].id); + if (ins.rows[0]) out.push(ins.rows[0].id); } return out; }); diff --git a/src/core/postgres-engine.ts b/src/core/postgres-engine.ts index 149d0a298..22e58be76 100644 --- a/src/core/postgres-engine.ts +++ b/src/core/postgres-engine.ts @@ -4491,9 +4491,13 @@ export class PostgresEngine implements BrainEngine { ${input.row_num}, ${input.source_markdown_slug}, ${claimMetric}, ${claimValue}, ${claimUnit}, ${claimPeriod}, ${eventType} - ) RETURNING id + ) + ON CONFLICT (source_id, source_markdown_slug, row_num) + WHERE row_num IS NOT NULL + DO NOTHING + RETURNING id `; - out.push(Number(ins[0].id)); + if (ins[0]) out.push(Number(ins[0].id)); } return out; }); diff --git a/test/e2e/system-of-record-invariant.test.ts b/test/e2e/system-of-record-invariant.test.ts index 92f04de54..89399687e 100644 --- a/test/e2e/system-of-record-invariant.test.ts +++ b/test/e2e/system-of-record-invariant.test.ts @@ -38,11 +38,11 @@ import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { PGLiteEngine } from '../../src/core/pglite-engine.ts'; -import { importFromFile } from '../../src/core/import-file.ts'; +import { importFromContent, importFromFile } from '../../src/core/import-file.ts'; import { runExtractCore } from '../../src/commands/extract.ts'; import { extractTakes } from '../../src/core/cycle/extract-takes.ts'; import { runExtractFacts } from '../../src/core/cycle/extract-facts.ts'; -import { stripFactsFence } from '../../src/core/facts-fence.ts'; +import { parseFactsFence, stripFactsFence } from '../../src/core/facts-fence.ts'; let engine: PGLiteEngine; let brainDir: string; @@ -305,6 +305,46 @@ describe('get_page privacy strip via stripFactsFence({keepVisibility:["world"]}) expect(remoteBody).not.toContain('PRIVATE_DETAIL_PROOF'); // remote MCP strips expect(remoteBody).toContain('Founded Acme in 2017'); // world fact retained }); + + test('remote put_page round-trip preserves an existing private-only facts fence', async () => { + const slug = 'people/private-only-facts'; + await importFromContent(engine, slug, `--- +type: person +title: Private Only Facts +slug: ${slug} +--- + +# Private Only Facts + +## Facts + + +| # | claim | kind | confidence | visibility | notability | valid_from | valid_until | source | context | +|---|-------|------|------------|------------|------------|------------|-------------|--------|---------| +| 1 | PRIVATE_ONLY_FACT | preference | 0.9 | private | medium | 2026-07-30 | | meeting | | + +`, { noEmbed: true, sourceId: 'default' }); + + const trusted = await engine.getPage(slug, { sourceId: 'default' }); + expect(trusted).not.toBeNull(); + if (!trusted) return; + + const remoteBody = stripFactsFence(trusted.compiled_truth ?? '', { keepVisibility: ['world'] }); + expect(remoteBody).toContain('gbrain:facts:begin'); + expect(remoteBody).not.toContain('PRIVATE_ONLY_FACT'); + + await importFromContent(engine, slug, `--- +type: person +title: Private Only Facts +slug: ${slug} +--- + +${remoteBody}`, { noEmbed: true, sourceId: 'default', remote: true }); + + const after = await engine.getPage(slug, { sourceId: 'default' }); + expect(after?.compiled_truth).toContain('PRIVATE_ONLY_FACT'); + expect(parseFactsFence(after?.compiled_truth ?? '').facts).toHaveLength(1); + }); }); afterAll(() => { diff --git a/test/insert-facts-batch.test.ts b/test/insert-facts-batch.test.ts index e58c3adb4..8f7149f90 100644 --- a/test/insert-facts-batch.test.ts +++ b/test/insert-facts-batch.test.ts @@ -5,7 +5,7 @@ * - Batch insert N rows persists row_num + source_markdown_slug * - Empty batch is a no-op * - Returns ids in input-order - * - v51 partial UNIQUE index rolls back the whole batch on a collision + * - v51 partial UNIQUE index collisions are idempotently skipped * - deleteFactsForPage scopes by (source_id, source_markdown_slug); * never touches other pages or pre-v51 NULL-source_markdown_slug rows * - deleteFactsForPage on an empty page returns deleted:0 (idempotent) @@ -135,30 +135,26 @@ describe('engine.insertFacts — batch insert', () => { }); }); - test('v51 partial UNIQUE index rolls back the whole batch on collision', async () => { + test('v51 partial UNIQUE index collision skips duplicate rows without rolling back the batch', async () => { // Seed row #1 first. await engine.insertFacts([fixtureFact(1, { fact: 'seeded' })], { source_id: 'default' }); - // Now try to batch-insert rows that include a colliding row_num=1. - let threw = false; - try { - await engine.insertFacts( - [ - fixtureFact(2, { fact: 'second' }), - fixtureFact(1, { fact: 'collides' }), // row_num=1 on same (source_id, source_markdown_slug) - fixtureFact(3, { fact: 'third' }), - ], - { source_id: 'default' }, - ); - } catch { - threw = true; - } - expect(threw).toBe(true); + // Now try to batch-insert rows that include a colliding row_num=1. The + // duplicate is skipped, but the non-conflicting rows still land. + const r = await engine.insertFacts( + [ + fixtureFact(2, { fact: 'second' }), + fixtureFact(1, { fact: 'collides' }), // row_num=1 on same (source_id, source_markdown_slug) + fixtureFact(3, { fact: 'third' }), + ], + { source_id: 'default' }, + ); + expect(r.inserted).toBe(2); + expect(r.ids).toHaveLength(2); - // Verify the transaction rolled back — only the seeded row should remain. // eslint-disable-next-line @typescript-eslint/no-explicit-any - const rows = await (engine as any).db.query('SELECT fact FROM facts ORDER BY id'); - expect(rows.rows.map((r: { fact: string }) => r.fact)).toEqual(['seeded']); + const rows = await (engine as any).db.query('SELECT fact, row_num FROM facts ORDER BY row_num'); + expect(rows.rows.map((row: { fact: string }) => row.fact)).toEqual(['seeded', 'second', 'third']); }); test('different source_markdown_slug values DO NOT collide on the same row_num', async () => {