From 91464564cde8ef12c9675bf1d9ea8cd5f01bd4d1 Mon Sep 17 00:00:00 2001 From: Time Attakc <89218912+time-attack@users.noreply.github.com> Date: Tue, 28 Jul 2026 14:11:54 -0700 Subject: [PATCH] fix(cycle): scope the extract_facts guard to its source and fix the drain advice (#2646, #3526) (#3528) Defect A (#3526): the empty-fence guard COUNT had no source_id predicate, so one pending legacy row in any mounted source jammed extract_facts for every source in the brain. The count now binds f.source_id = $1 to the run's sourceId (source-isolation invariant). Defect B: the guard advised `gbrain apply-migrations --yes`, which is a proven no-op once the v0.32.2 ledger entry is complete (the runner classifies it as already-applied and Phase B never re-runs). The warning (and cycle.ts's phase hint) now gives the drain path verified to work end-to-end on a real brain: `apply-migrations --force-retry 0.32.2` then `apply-migrations --yes` (Phase B is idempotent), or forget_fact per row. New test proves a pending legacy row in source A does not jam extraction for source B (fails on master, passes with the fix). Co-authored-by: Garry Tan Co-authored-by: Claude Opus 5 --- src/core/cycle.ts | 4 ++- src/core/cycle/extract-facts.ts | 43 ++++++++++++++++++++++-------- test/extract-facts-phase.test.ts | 45 ++++++++++++++++++++++++++++++++ 3 files changed, 80 insertions(+), 12 deletions(-) diff --git a/src/core/cycle.ts b/src/core/cycle.ts index 746b8911d..d79da17c9 100644 --- a/src/core/cycle.ts +++ b/src/core/cycle.ts @@ -1179,7 +1179,9 @@ async function runPhaseExtractFacts( summary: `extract_facts skipped: ${result.legacyRowsPending} legacy v0.31 facts pending fence backfill`, details: { legacyRowsPending: result.legacyRowsPending, - hint: 'gbrain apply-migrations --yes', + // A bare `apply-migrations --yes` no-ops once the v0.32.2 ledger + // entry is complete; the retry marker is what re-runs Phase B. + hint: 'gbrain apply-migrations --force-retry 0.32.2 && gbrain apply-migrations --yes', warnings: result.warnings, }, }; diff --git a/src/core/cycle/extract-facts.ts b/src/core/cycle/extract-facts.ts index 5c2d48343..102a5d0df 100644 --- a/src/core/cycle/extract-facts.ts +++ b/src/core/cycle/extract-facts.ts @@ -26,13 +26,17 @@ * * Empty-fence guard (Codex R2-#7; #2484; #2646): the phase refuses to do * its destructive reconciliation pass when genuinely-backfillable legacy - * rows still exist — `row_num IS NULL` (never fenced) AND `entity_slug` - * resolves to a live page in this source (so the v0_32_2 migration's - * Phase B could fence them) AND the row is not soft-expired - * (`expired_at IS NULL`). Status returns `warn` with a hint to run - * `gbrain apply-migrations --yes`. Without the guard, an interrupted - * upgrade where v0_32_2 hasn't run could leave the cycle silently - * misreporting "0 facts on people/alice" while legacy rows linger. + * rows still exist — in THIS run's source only (`source_id = sourceId`; + * a pending row in source A must not jam extraction for source B — the + * source-isolation invariant) — `row_num IS NULL` (never fenced) AND + * `entity_slug` resolves to a live page in this source (so the v0_32_2 + * migration's Phase B could fence them) AND the row is not soft-expired + * (`expired_at IS NULL`). Status returns `warn` with a hint to re-run + * the v0.32.2 fence backfill (`apply-migrations --force-retry 0.32.2` + * then `--yes` — a bare `--yes` is a no-op once the ledger says + * complete). Without the guard, an interrupted upgrade where v0_32_2 + * hasn't run could leave the cycle silently misreporting "0 facts on + * people/alice" while legacy rows linger. * * The live-page requirement (#2484) is load-bearing: the inline facts * writer keeps producing `row_num IS NULL, entity_slug IS NOT NULL` @@ -225,10 +229,17 @@ export async function runExtractFacts( // soft-expires legacy rows rather than deleting them, so counting // expired rows would leave the guard permanently stuck with no // supported way to drain the backlog. + // + // Source isolation (#3526): the count is scoped to THIS run's + // sourceId. The pre-fix query counted brain-wide, so a single pending + // legacy row in any mounted source jammed extract_facts for every + // source — a cross-source leak of one source's migration state into + // another's cycle (CLAUDE.md source-isolation invariant). const legacy = await engine.executeRaw<{ n: string }>( `SELECT COUNT(*) AS n FROM facts f - WHERE f.row_num IS NULL + WHERE f.source_id = $1 + AND f.row_num IS NULL AND f.entity_slug IS NOT NULL AND f.expired_at IS NULL AND EXISTS ( @@ -237,15 +248,25 @@ export async function runExtractFacts( AND p.slug = f.entity_slug AND p.deleted_at IS NULL )`, + [sourceId], ); const legacyCount = parseInt(legacy[0]?.n ?? '0', 10); result.legacyRowsPending = legacyCount; if (legacyCount > 0) { result.guardTriggered = true; + // Drain advice must actually work: a bare `apply-migrations --yes` + // is a no-op once the v0.32.2 ledger entry says complete (the + // runner classifies it as already-applied), so the sanctioned + // re-run path is the explicit retry marker first. Phase B is + // idempotent — it only touches `row_num IS NULL` rows and de-dupes + // against the existing fence — so the re-run is safe. Individual + // rows can instead be drained through `forget_fact` (soft-expired + // rows stop counting). result.warnings.push( - `extract_facts: ${legacyCount} legacy v0.31 fact rows (entity page present, not yet ` + - `fenced) pending fence backfill. Run \`gbrain apply-migrations --yes\` to complete ` + - `v0_32_2 before this phase can safely reconcile fence → DB.`, + `extract_facts: ${legacyCount} legacy v0.31 fact rows in source "${sourceId}" ` + + `(entity page present, not yet fenced) pending fence backfill. Re-run the v0.32.2 ` + + `fence backfill: \`gbrain apply-migrations --force-retry 0.32.2\` then ` + + `\`gbrain apply-migrations --yes\`. Or drain individual rows via \`forget_fact\`.`, ); return result; } diff --git a/test/extract-facts-phase.test.ts b/test/extract-facts-phase.test.ts index 2b53df545..899506492 100644 --- a/test/extract-facts-phase.test.ts +++ b/test/extract-facts-phase.test.ts @@ -672,6 +672,51 @@ describe('runExtractFacts — empty-fence guard (Codex R2-#7)', () => { }); describe('runExtractFacts — multi-source isolation', () => { + test('a pending legacy row in source A does NOT jam extraction for source B (#2646 source-scope)', async () => { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + await (engine as any).db.query( + `INSERT INTO sources (id, name, config) VALUES ('work', 'work', '{}'::jsonb) + ON CONFLICT (id) DO NOTHING`, + ); + + // Source "work": a genuine pending legacy row (row_num NULL, active, + // live backing page) — the exact shape that must gate work's cycle. + await engine.putPage('people/alice', { + title: 'people/alice', type: 'person', + compiled_truth: FACT_FENCE(`| 1 | work fence fact | fact | 1.0 | world | high | 2026-01-01 | | s | |`), + frontmatter: {}, timeline: '', + }, { sourceId: 'work' }); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + await (engine as any).db.query( + `INSERT INTO facts (source_id, entity_slug, fact, kind, visibility, notability, + valid_from, source, confidence) + VALUES ('work', 'people/alice', 'work legacy claim', 'fact', 'private', 'medium', + now(), 'mcp:put_page', 1.0)`, + ); + + // Source "default": clean — no legacy rows, one fenced page. + await putPage('people/bob', FACT_FENCE( + `| 1 | default fact | fact | 1.0 | world | high | 2026-01-01 | | s | |`, + )); + + // default's run must NOT be jammed by work's pending backlog. + const rDefault = await runExtractFacts(engine, { slugs: ['people/bob'], sourceId: 'default' }); + expect(rDefault.guardTriggered).toBe(false); + expect(rDefault.legacyRowsPending).toBe(0); + expect(rDefault.factsInserted).toBe(1); + + // work's own run still gates (discriminator stays sharp). + const rWork = await runExtractFacts(engine, { slugs: ['people/alice'], sourceId: 'work' }); + expect(rWork.guardTriggered).toBe(true); + expect(rWork.legacyRowsPending).toBe(1); + expect(rWork.factsInserted).toBe(0); + // The drain advice must be one that actually re-runs Phase B — a bare + // `apply-migrations --yes` no-ops once the ledger says complete. + expect(rWork.warnings.some(w => w.includes('--force-retry 0.32.2'))).toBe(true); + expect(rWork.warnings.some(w => w.includes('forget_fact'))).toBe(true); + expect(rWork.warnings.some(w => w.includes('source "work"'))).toBe(true); + }); + test('deleteFactsForPage scoping does not affect other sources', async () => { // Seed sources work + home. // eslint-disable-next-line @typescript-eslint/no-explicit-any