mirror of
https://github.com/garrytan/gbrain.git
synced 2026-07-30 11:22:34 +00:00
review: empty-file skip must not enter the sync failure ledger; keep inline extract for non-brainDir sync targets
Two adversarial-review fixes on the #840/#1079 takeovers: 1. importCodeFile's empty-file skip returned skipped WITH an error string. performSync treats skipped-with-error as a parse failure (failedFiles → applySyncFailureGate), so any repo containing a legitimate empty code file (__init__.py, barrel stubs) would BLOCK last_commit for 3 consecutive syncs and then live in the ledger as auto-skipped — the fix recreated #840's spurious-failure complaint one layer up, and the became-empty branch reported its own successful stale-page deletion as a failure. The skip now carries no error, matching the content-hash short-circuit (banked via markCompleted, never ledgered). 2. runPhaseSync passed noExtract: willRunExtractPhase to EVERY sync target, but the cycle's extract phase (runPhaseExtract → extractForSlugs) walks brainDir only — slugs synced from other sources' checkouts are invisible to it, so their link/timeline extraction was silently dropped forever. Inline extract is now deduped only for the brainDir target; other targets keep it, matching standalone 'gbrain sync --source X' behavior. Pinned by a new cycle.serial test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Fable 5
parent
bccd97c75b
commit
8fa9bed815
+11
-4
@@ -978,10 +978,17 @@ async function runPhaseSync(
|
||||
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.
|
||||
noEmbed: true, // embed is a separate phase (runEmbedCore --stale is brain-wide,
|
||||
// so it covers every target here)
|
||||
// Dedupe inline extract ONLY for the target the cycle's extract
|
||||
// phase will actually cover: extract walks brainDir under
|
||||
// cycleSourceId (runPhaseExtract), so slugs synced from OTHER
|
||||
// checkouts are invisible to it — disabling their inline extract
|
||||
// would silently drop link/timeline extraction for every
|
||||
// non-brainDir source. If extract isn't scheduled at all (e.g.
|
||||
// `gbrain dream --phase sync`), inline extract runs everywhere
|
||||
// to preserve prior behavior.
|
||||
noExtract: willRunExtractPhase && t.repoPath === brainDir,
|
||||
});
|
||||
synced++;
|
||||
added += result.added;
|
||||
|
||||
@@ -1078,10 +1078,17 @@ export async function importCodeFile(
|
||||
// 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.
|
||||
//
|
||||
// NO `error` on the result: sync treats skipped-with-error as a parse
|
||||
// failure (sync.ts pushes it into failedFiles → applySyncFailureGate
|
||||
// BLOCKS last_commit on fresh failures). Empty files are benign and
|
||||
// common (`__init__.py`, barrel stubs) — they must skip like the
|
||||
// content-hash short-circuit (banked/checkpointed, never ledgered),
|
||||
// not block the bookmark.
|
||||
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' };
|
||||
return { slug, status: 'skipped', chunks: 0 };
|
||||
}
|
||||
|
||||
// Vendor-neutral guardrail seam (observe-only, fail-open). Runs AFTER the
|
||||
|
||||
@@ -662,6 +662,22 @@ describe('runCycle — multi-source sync (#1079)', () => {
|
||||
expect(syncCalls.map(c => c.sourceId)).toEqual(['wiki', undefined]);
|
||||
});
|
||||
|
||||
test('inline extract stays ON for non-brainDir targets when extract phase is scheduled', async () => {
|
||||
// The cycle's extract phase walks brainDir only (runPhaseExtract), so
|
||||
// dedupe (noExtract: true) is only valid for the brainDir target.
|
||||
// Other sources' checkouts are invisible to the extract phase — their
|
||||
// sync must keep inline extract or links/timeline are never extracted.
|
||||
await (sharedEngine as any).db.query(
|
||||
`INSERT INTO sources (id, name, local_path) VALUES
|
||||
('wiki', 'wiki', '/tmp/brain-1079-i'),
|
||||
('code', 'code', '/tmp/brain-1079-j')`,
|
||||
);
|
||||
await runCycle(sharedEngine, { brainDir: '/tmp/brain-1079-i', phases: ['sync', 'extract'] });
|
||||
const byId = new Map(syncCalls.map(c => [c.sourceId, c.noExtract]));
|
||||
expect(byId.get('wiki')).toBe(true); // covered by the extract phase — dedupe
|
||||
expect(byId.get('code')).toBe(false); // NOT covered — inline extract must run
|
||||
});
|
||||
|
||||
test('explicit --source keeps the cycle single-source (#1503)', async () => {
|
||||
await (sharedEngine as any).db.query(
|
||||
`INSERT INTO sources (id, name, local_path) VALUES
|
||||
|
||||
@@ -5,7 +5,7 @@
|
||||
* 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
|
||||
* skips them the same way it skips unchanged content (benign, no error) — 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.
|
||||
@@ -33,7 +33,11 @@ describe('importCodeFile — empty file guard (#840)', () => {
|
||||
|
||||
expect(result.status).toBe('skipped');
|
||||
expect(result.chunks).toBe(0);
|
||||
expect(result.error).toBe('empty code file');
|
||||
// NO error on the result: sync treats skipped-with-error as a parse
|
||||
// failure and applySyncFailureGate would BLOCK last_commit on any repo
|
||||
// containing an empty file. Empty files skip like the content-hash
|
||||
// short-circuit — benign, banked, never ledgered.
|
||||
expect(result.error).toBeUndefined();
|
||||
|
||||
// The skip must short-circuit before any page row is written, otherwise
|
||||
// reindex-code will still trip on the missing compiled_truth.
|
||||
@@ -48,7 +52,7 @@ describe('importCodeFile — empty file guard (#840)', () => {
|
||||
|
||||
const second = await importCodeFile(engine, 'src/becomes-empty.ts', '', { noEmbed: true });
|
||||
expect(second.status).toBe('skipped');
|
||||
expect(second.error).toBe('empty code file');
|
||||
expect(second.error).toBeUndefined(); // benign skip — must not enter the sync failure ledger
|
||||
|
||||
// Stale page (and its chunks, via FK cascade) must be gone.
|
||||
expect(await engine.getPage(first.slug)).toBeNull();
|
||||
|
||||
Reference in New Issue
Block a user