From bccd97c75ba42aef91f444486bf04af4bfea9d99 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Tue, 21 Jul 2026 14:46:59 -0700 Subject: [PATCH] fix(core): empty code files, multi-source cycle sync, root-level entity refs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three community-PR takeovers rebased onto master: - import-file (#840): importCodeFile skips empty (0-byte) code files instead of writing a page with empty compiled_truth that fails every reindex-code pass. When a previously-imported file BECOMES empty, the stale page is deleted (chunks cascade) instead of ghosting in search — mirrors the markdown importer's empty-content branch. - cycle (#1079): runPhaseSync now enumerates every registered, non-archived, sync-enabled source with a local checkout instead of syncing only the single brainDir-matched source. Preserves the SyncLockBusyError per-source skip (#1794), keeps the legacy brainDir fallback when no source covers it, and respects explicit --source scoping (#1503) so a scoped cycle stays single-source. Single-target result shape is unchanged. - link-extraction (#1101): ENTITY_REF_RE gains a root-level branch so [Name](action-tracker.md) resolves to slug 'action-tracker'. The bare branch requires the .md suffix and excludes '#', '/', ':' so anchors, non-markdown assets, and URLs don't emit bogus refs; the capture's .md is stripped in extractEntityRefs and dir is '' (not the filename). LINK_EXTRACTOR_VERSION_TS bumped so stamped pages re-extract. Takeover of #840, #1079, #1101 (rebased; flaws named in review fixed). Co-authored-by: nightlaro Co-authored-by: samvilian Co-authored-by: essexcanning Co-Authored-By: Claude Fable 5 --- src/core/cycle.ts | 176 +++++++++++++++++++++------- src/core/import-file.ts | 14 +++ src/core/link-extraction.ts | 25 ++-- test/core/cycle.serial.test.ts | 48 ++++++++ test/extract-stale.test.ts | 2 +- test/import-empty-code-file.test.ts | 64 ++++++++++ test/link-extraction.test.ts | 46 ++++++++ 7 files changed, 323 insertions(+), 52 deletions(-) create mode 100644 test/import-empty-code-file.test.ts diff --git a/src/core/cycle.ts b/src/core/cycle.ts index a10b1ca80..ba53ef640 100644 --- a/src/core/cycle.ts +++ b/src/core/cycle.ts @@ -917,51 +917,101 @@ async function runPhaseSync( dryRun: boolean, pull: boolean, willRunExtractPhase: boolean, + // #1503: explicit --source keeps the cycle single-source. Only the + // user-supplied opts.sourceId lands here (NOT the brainDir-derived + // cycleSourceId) — an unscoped autopilot cycle syncs every source. + explicitSourceId?: string, ): Promise { try { - const { performSync } = await import('../commands/sync.ts'); - // Resolve the per-source id so sync reads source-scoped last_commit - // instead of the global config key. The global key can drift out of - // git history (force push, GC) causing a full reimport of all files. - const sourceId = await resolveSourceForDir(engine, brainDir); - const result = await performSync(engine, { - repoPath: brainDir, - sourceId, - dryRun, - noPull: !pull, - noEmbed: true, // embed is a separate phase - noExtract: willRunExtractPhase, // dedupe ONLY when cycle's extract phase will also run. - // If extract isn't scheduled (e.g. `gbrain dream --phase sync`), - // sync's inline extract still runs to preserve prior behavior. - }); - const syncedCount = result.added + result.modified; - return { - phase: 'sync', - status: result.status === 'blocked_by_failures' ? 'warn' : 'ok', - duration_ms: 0, - summary: dryRun - ? `${syncedCount} page(s) would sync, ${result.deleted} would delete` - : `+${result.added} added, ~${result.modified} modified, -${result.deleted} deleted`, - details: { - added: result.added, - modified: result.modified, - deleted: result.deleted, - renamed: result.renamed, - chunksCreated: result.chunksCreated, - failedFiles: result.failedFiles ?? 0, - syncStatus: result.status, - dryRun, - }, - pagesAffected: result.pagesAffected, - }; - } catch (e) { - // v0.42.x (#1794): a single-flight collision — another sync already holds - // the per-source lock — is NOT a phase failure. The other run is doing the - // work; surfacing 'fail' would paint a healthy cron contention red and (with - // the heartbeat-aware takeover) this is now the expected outcome when a long - // sync overruns into the next cron tick. Report it as a skip. - const { SyncLockBusyError } = await import('../commands/sync.ts'); - if (e instanceof SyncLockBusyError) { + const { performSync, SyncLockBusyError } = await import('../commands/sync.ts'); + + // #1079: enumerate sync targets. An unscoped cycle syncs EVERY registered, + // non-archived, sync-enabled source with a local checkout — not just the + // single brainDir-matched source. brainDir itself still syncs when no + // registered source covers it (legacy pre-sources behavior, and the + // fallback when the sources table is empty/missing). + type SyncTarget = { sourceId: string | undefined; repoPath: string }; + const targets: SyncTarget[] = []; + if (explicitSourceId !== undefined) { + let repoPath = brainDir; + try { + const rows = await engine.executeRaw<{ local_path: string | null }>( + `SELECT local_path FROM sources WHERE id = $1 LIMIT 1`, + [explicitSourceId], + ); + if (rows[0]?.local_path) repoPath = rows[0].local_path; + } catch { /* sources table might not exist on very old brains */ } + targets.push({ sourceId: explicitSourceId, repoPath }); + } else { + try { + const rows = await engine.executeRaw<{ id: string; local_path: string; config: Record | null }>( + `SELECT id, local_path, config FROM sources + WHERE local_path IS NOT NULL AND local_path != '' AND NOT archived`, + ); + for (const r of rows) { + const cfg = (r.config || {}) as { syncEnabled?: boolean }; + if (cfg.syncEnabled === false) continue; + targets.push({ sourceId: r.id, repoPath: r.local_path }); + } + } catch { /* sources table might not exist on very old brains — legacy fallback below */ } + if (!targets.some((t) => t.repoPath === brainDir)) { + // Resolve the per-source id so sync reads source-scoped last_commit + // instead of the global config key. The global key can drift out of + // git history (force push, GC) causing a full reimport of all files. + targets.push({ sourceId: await resolveSourceForDir(engine, brainDir), repoPath: brainDir }); + } + } + + let added = 0, modified = 0, deleted = 0, renamed = 0, chunksCreated = 0, failedFiles = 0; + const pagesAffected: string[] = []; + const perSource: string[] = []; + let synced = 0, lockBusy = 0, errored = 0; + let anyBlocked = false; + let lastError: unknown; + let lastStatus: string | undefined; + + for (const t of targets) { + const label = t.sourceId ?? 'default'; + try { + const result = await performSync(engine, { + repoPath: t.repoPath, + sourceId: t.sourceId, + dryRun, + noPull: !pull, + noEmbed: true, // embed is a separate phase + noExtract: willRunExtractPhase, // dedupe ONLY when cycle's extract phase will also run. + // If extract isn't scheduled (e.g. `gbrain dream --phase sync`), + // sync's inline extract still runs to preserve prior behavior. + }); + synced++; + added += result.added; + modified += result.modified; + deleted += result.deleted; + renamed += result.renamed; + chunksCreated += result.chunksCreated; + failedFiles += result.failedFiles ?? 0; + pagesAffected.push(...(result.pagesAffected ?? [])); + if (result.status === 'blocked_by_failures') anyBlocked = true; + lastStatus = result.status; + perSource.push(`${label}: ${result.status} (+${result.added} ~${result.modified} -${result.deleted})`); + } catch (e) { + // v0.42.x (#1794): a single-flight collision — another sync already holds + // the per-source lock — is NOT a phase failure. The other run is doing the + // work; surfacing 'fail' would paint a healthy cron contention red and (with + // the heartbeat-aware takeover) this is now the expected outcome when a long + // sync overruns into the next cron tick. Report it as a skip. + if (SyncLockBusyError && e instanceof SyncLockBusyError) { + lockBusy++; + perSource.push(`${label}: lock_busy`); + continue; + } + errored++; + lastError = e; + perSource.push(`${label}: error (${e instanceof Error ? e.message : String(e)})`); + } + } + + if (synced === 0 && errored === 0 && lockBusy > 0) { return { phase: 'sync', status: 'skipped', @@ -970,6 +1020,44 @@ async function runPhaseSync( details: { syncStatus: 'lock_busy' }, }; } + if (synced === 0 && errored > 0) { + return { + phase: 'sync', + status: 'fail', + duration_ms: 0, + summary: 'sync phase failed', + details: targets.length > 1 ? { sources: perSource } : {}, + error: makeErrorFromException(lastError), + }; + } + + const syncedCount = added + modified; + const across = targets.length > 1 ? ` across ${targets.length} source(s)` : ''; + return { + phase: 'sync', + status: anyBlocked || errored > 0 ? 'warn' : 'ok', + duration_ms: 0, + summary: dryRun + ? `${syncedCount} page(s) would sync, ${deleted} would delete${across}` + : `+${added} added, ~${modified} modified, -${deleted} deleted${across}`, + details: { + added, + modified, + deleted, + renamed, + chunksCreated, + failedFiles, + // Single-target keeps the raw performSync status (pre-#1079 shape); + // multi-target aggregates. + syncStatus: targets.length === 1 + ? lastStatus + : errored > 0 ? 'partial_failure' : anyBlocked ? 'blocked_by_failures' : 'completed', + dryRun, + ...(targets.length > 1 ? { sources: perSource } : {}), + }, + pagesAffected, + }; + } catch (e) { return { phase: 'sync', status: 'fail', @@ -1679,7 +1767,7 @@ export async function runCycle( } else { progress.start('cycle.sync'); syncAttempted = true; // sync ran its work; undefined pagesAffected now means failure - const { result, duration_ms } = await timePhase(() => runPhaseSync(engine, brainDir, dryRun, pull, phases.includes('extract'))); + const { result, duration_ms } = await timePhase(() => runPhaseSync(engine, brainDir, dryRun, pull, phases.includes('extract'), opts.sourceId)); result.duration_ms = duration_ms; // Capture changed slugs for incremental extract. syncPagesAffected = (result as SyncPhaseResult).pagesAffected; diff --git a/src/core/import-file.ts b/src/core/import-file.ts index 4e7722abb..fda3d4a28 100644 --- a/src/core/import-file.ts +++ b/src/core/import-file.ts @@ -1070,6 +1070,20 @@ export async function importCodeFile( return { slug, status: 'skipped', chunks: 0, error: `Code file too large (${byteLength} bytes)` }; } + // Skip empty code files (#840). With zero bytes there is nothing to chunk + // and no compiled_truth to derive, but without this guard the page row is + // still written with empty compiled_truth — which then fails every + // subsequent `gbrain reindex-code` pass with `missing compiled_truth`. + // Empty files are legitimate in real repos (stub/placeholder files). If a + // previously-imported file BECAME empty, delete the stale page (chunks + // cascade) so it doesn't ghost in search — mirrors the markdown importer's + // empty-content branch, which deletes stale chunks. + if (byteLength === 0) { + const stale = await engine.getPage(slug, txOpts); + if (stale) await engine.deletePage(slug, txOpts); + return { slug, status: 'skipped', chunks: 0, error: 'empty code file' }; + } + // Vendor-neutral guardrail seam (observe-only, fail-open). Runs AFTER the // code size guard, BEFORE hash compute, code-chunking, embedding, and DB // write. Verdict ignored by design; no-op when no guardrail is registered. diff --git a/src/core/link-extraction.ts b/src/core/link-extraction.ts index c0fc2644a..788b1cc80 100644 --- a/src/core/link-extraction.ts +++ b/src/core/link-extraction.ts @@ -28,7 +28,7 @@ import { ensureWellFormed } from './text-safe.ts'; * OR updated_at > links_extracted_at`. It is an ISO-8601 string (NOT a number) — * the column is TIMESTAMPTZ and the predicate binds it as `::timestamptz`. */ -export const LINK_EXTRACTOR_VERSION_TS = '2026-05-31T00:00:00Z'; +export const LINK_EXTRACTOR_VERSION_TS = '2026-07-20T00:00:00Z'; // ─── Entity references ────────────────────────────────────────── @@ -92,11 +92,18 @@ const DIR_PATTERN = '(?:people|companies|meetings|concepts|deal|civic|project|pr * * Captures: name, slug (dir/name, possibly deeper). * - * The regex permits an optional `../` prefix (any number) and an optional - * `.md` suffix so the same function works for both filesystem and DB content. + * The regex permits optional `../` / `./` prefixes (any number) and an + * optional `.md` suffix so the same function works for both filesystem and DB + * content. + * + * #1101: root-level file references (`[Tracker](action-tracker.md)`) are also + * matched via the second alternative. The bare branch REQUIRES the `.md` + * suffix (kept inside the capture; extractEntityRefs strips it) so anchors + * (`#section`), non-markdown assets (`chart.png`), and URLs (`:`/`/` excluded + * by the class) don't produce bogus entity refs. */ const ENTITY_REF_RE = new RegExp( - `\\[([^\\]]+)\\]\\((?:\\.\\.\\/)*(${DIR_PATTERN}\\/[^)\\s]+?)(?:\\.md)?\\)`, + `\\[([^\\]]+)\\]\\((?:\\.\\.\\/|\\.\\/)*((?:${DIR_PATTERN}\\/[^)\\s]+?)|(?:[^)\\s\\/:#]+?\\.md))(?:\\.md)?\\)`, 'g', ); @@ -311,9 +318,13 @@ export function extractEntityRefs(content: string): EntityRef[] { const mdPattern = new RegExp(ENTITY_REF_RE.source, ENTITY_REF_RE.flags); while ((match = mdPattern.exec(stripped)) !== null) { const name = match[1]; - const fullPath = match[2]; - const slug = fullPath; - const dir = fullPath.split('/')[0]; + // #1101: the root-level branch captures the mandatory `.md` suffix + // (dir-prefixed captures already exclude it via the trailing optional + // group) — strip it so the slug matches the stored page slug. + const slug = match[2].endsWith('.md') ? match[2].slice(0, -3) : match[2]; + // Root-level refs have no entity dir; '' mirrors the generic-wikilink + // pass (2c below) rather than misreporting the filename as a dir. + const dir = slug.includes('/') ? slug.split('/')[0] : ''; refs.push({ name, slug, dir }); markdownRanges.push([match.index, match.index + match[0].length]); } diff --git a/test/core/cycle.serial.test.ts b/test/core/cycle.serial.test.ts index 247124d58..0bdc90927 100644 --- a/test/core/cycle.serial.test.ts +++ b/test/core/cycle.serial.test.ts @@ -625,3 +625,51 @@ describe('runCycle — onceForPhase bypasses only the named phase (issue #2860)' expect(await sharedEngine.getConfig('dream.patterns.enabled')).toBe('false'); }); }); + +// ─── multi-source sync (#1079) ───────────────────────────────────── +// +// An unscoped autopilot cycle must sync EVERY registered, non-archived, +// sync-enabled source with a local checkout — not just the single source +// whose local_path happens to match brainDir. Explicit --source (#1503) +// keeps the cycle single-source. + +describe('runCycle — multi-source sync (#1079)', () => { + beforeEach(async () => { + await truncateCycleLocks(sharedEngine); + await (sharedEngine as any).db.query('DELETE FROM sources'); + syncCalls = []; + }); + + test('unscoped cycle syncs every registered, non-archived, sync-enabled source', async () => { + await (sharedEngine as any).db.query( + `INSERT INTO sources (id, name, local_path, archived, config) VALUES + ('wiki', 'wiki', '/tmp/brain-1079-a', false, '{}'::jsonb), + ('code', 'code', '/tmp/brain-1079-b', false, '{}'::jsonb), + ('old', 'old', '/tmp/brain-1079-c', true, '{}'::jsonb), + ('paused', 'paused', '/tmp/brain-1079-d', false, '{"syncEnabled": false}'::jsonb), + ('remote', 'remote', NULL, false, '{}'::jsonb)`, + ); + await runCycle(sharedEngine, { brainDir: '/tmp/brain-1079-a', phases: ['sync'] }); + // archived, syncEnabled=false, and checkout-less sources are excluded. + expect(syncCalls.map(c => c.sourceId).sort()).toEqual(['code', 'wiki']); + }); + + test('brainDir not covered by any registered source still syncs (legacy fallback)', async () => { + await (sharedEngine as any).db.query( + `INSERT INTO sources (id, name, local_path) VALUES ('wiki', 'wiki', '/tmp/brain-1079-e')`, + ); + await runCycle(sharedEngine, { brainDir: '/tmp/brain-1079-f', phases: ['sync'] }); + expect(syncCalls.map(c => c.sourceId)).toEqual(['wiki', undefined]); + }); + + test('explicit --source keeps the cycle single-source (#1503)', async () => { + await (sharedEngine as any).db.query( + `INSERT INTO sources (id, name, local_path) VALUES + ('wiki', 'wiki', '/tmp/brain-1079-g'), + ('code', 'code', '/tmp/brain-1079-h')`, + ); + await runCycle(sharedEngine, { brainDir: '/tmp/brain-1079-g', phases: ['sync'], sourceId: 'code' }); + expect(syncCalls.length).toBe(1); + expect(syncCalls[0].sourceId).toBe('code'); + }); +}); diff --git a/test/extract-stale.test.ts b/test/extract-stale.test.ts index a725e309b..765b23ca0 100644 --- a/test/extract-stale.test.ts +++ b/test/extract-stale.test.ts @@ -188,7 +188,7 @@ describe('gbrain extract --stale', () => { await engine.putPage('companies/acme', companyPage('Acme', '[Alice](people/alice) advises [Acme](companies/acme).')); // Microsecond-precision updated_at, recent (after LINK_EXTRACTOR_VERSION_TS) so the // version arm doesn't fire — the edited arm is what must clear. - await engine.executeRaw(`UPDATE pages SET updated_at = '2026-06-02 08:18:58.999166+00'`); + await engine.executeRaw(`UPDATE pages SET updated_at = '2026-07-20 08:18:58.999166+00'`); expect(await engine.countStalePagesForExtraction({ versionTs: LINK_EXTRACTOR_VERSION_TS })).toBe(2); await runExtract(engine, ['--stale']); diff --git a/test/import-empty-code-file.test.ts b/test/import-empty-code-file.test.ts new file mode 100644 index 000000000..2c7c5950d --- /dev/null +++ b/test/import-empty-code-file.test.ts @@ -0,0 +1,64 @@ +/** + * Regression test for `importCodeFile` skipping empty (0-byte) code files (#840). + * + * Before this guard, an empty file produced a page row with empty + * compiled_truth, which then failed every subsequent `gbrain reindex-code` + * pass with `missing compiled_truth`. Empty files are legitimate in real + * repos (stub/placeholder files committed during refactors); the importer + * skips them the same way it skips files over MAX_FILE_SIZE — and when a + * previously-imported file BECOMES empty, the stale page is deleted (chunks + * cascade) instead of ghosting in search, mirroring the markdown importer's + * empty-content branch. + */ + +import { describe, test, expect, beforeAll, afterAll } from 'bun:test'; +import { PGLiteEngine } from '../src/core/pglite-engine.ts'; +import { importCodeFile } from '../src/core/import-file.ts'; + +let engine: PGLiteEngine; + +beforeAll(async () => { + engine = new PGLiteEngine(); + await engine.connect({}); + await engine.initSchema(); +}); + +afterAll(async () => { + await engine.disconnect(); +}); + +describe('importCodeFile — empty file guard (#840)', () => { + test('zero-byte content is skipped, not inserted', async () => { + const result = await importCodeFile(engine, 'src/empty.ts', '', { noEmbed: true }); + + expect(result.status).toBe('skipped'); + expect(result.chunks).toBe(0); + expect(result.error).toBe('empty code file'); + + // The skip must short-circuit before any page row is written, otherwise + // reindex-code will still trip on the missing compiled_truth. + const page = await engine.getPage(result.slug); + expect(page).toBeNull(); + }); + + test('file that BECOMES empty deletes the stale page instead of ghosting', async () => { + const first = await importCodeFile(engine, 'src/becomes-empty.ts', 'export const x = 1;\n', { noEmbed: true }); + expect(first.status).toBe('imported'); + expect(await engine.getPage(first.slug)).not.toBeNull(); + + const second = await importCodeFile(engine, 'src/becomes-empty.ts', '', { noEmbed: true }); + expect(second.status).toBe('skipped'); + expect(second.error).toBe('empty code file'); + + // Stale page (and its chunks, via FK cascade) must be gone. + expect(await engine.getPage(first.slug)).toBeNull(); + expect(await engine.getChunks(first.slug)).toEqual([]); + }); + + test('non-empty content still imports normally', async () => { + const result = await importCodeFile(engine, 'src/non-empty.ts', 'export const x = 1;\n', { noEmbed: true }); + + expect(result.status).toBe('imported'); + expect(result.chunks).toBeGreaterThan(0); + }); +}); diff --git a/test/link-extraction.test.ts b/test/link-extraction.test.ts index e0a741220..df1542b9f 100644 --- a/test/link-extraction.test.ts +++ b/test/link-extraction.test.ts @@ -1291,3 +1291,49 @@ describe('parseTimelineEntries — Format 3: inline [Source: ..., YYYY-MM-DD] ci expect(parseTimelineEntries('[Source: import batch, 2025-07-01]')).toHaveLength(0); }); }); + +// ─── #1101: root-level file references ([Name](file.md)) ────────────────── + +describe('extractEntityRefs — root-level file refs (#1101)', () => { + test('matches [Name](action-tracker.md) with .md stripped from the slug', () => { + const refs = extractEntityRefs('See the [Action Tracker](action-tracker.md) for status.'); + expect(refs.length).toBe(1); + expect(refs[0].name).toBe('Action Tracker'); + expect(refs[0].slug).toBe('action-tracker'); // NOT 'action-tracker.md' + expect(refs[0].dir).toBe(''); // no entity dir — must not misreport the filename as a dir + }); + + test('matches ./-prefixed root-level refs', () => { + const refs = extractEntityRefs('See [Tracker](./action-tracker.md).'); + expect(refs.length).toBe(1); + expect(refs[0].slug).toBe('action-tracker'); + }); + + test('does NOT match section anchors', () => { + expect(extractEntityRefs('Jump to [the section](#section).')).toEqual([]); + expect(extractEntityRefs('Jump to [it](action-tracker.md#section).')).toEqual([]); + }); + + test('does NOT match non-markdown assets', () => { + expect(extractEntityRefs('![alt](chart.png)')).toEqual([]); + expect(extractEntityRefs('[alt](chart.png)')).toEqual([]); + }); + + test('does NOT match external URLs or bare non-md tokens', () => { + expect(extractEntityRefs('[docs](https://example.com/foo.md)')).toEqual([]); + expect(extractEntityRefs('[x](mailto:someone@example.md)')).toEqual([]); + expect(extractEntityRefs('[x](tech)')).toEqual([]); + }); + + test('does NOT match non-whitelisted subdirectory paths', () => { + expect(extractEntityRefs('[random](notes/random.md)')).toEqual([]); + }); + + test('dir-prefixed refs are unchanged alongside root-level refs', () => { + const refs = extractEntityRefs( + '[Alice](../people/alice.md) tracked in [Tracker](action-tracker.md).', + ); + expect(refs.map(r => r.slug)).toEqual(['people/alice', 'action-tracker']); + expect(refs.map(r => r.dir)).toEqual(['people', '']); + }); +});