From f9349ba07fa93748624e04ef2e063a6abc5ab5da Mon Sep 17 00:00:00 2001 From: Time Attakc <89218912+time-attack@users.noreply.github.com> Date: Tue, 28 Jul 2026 19:50:44 -0700 Subject: [PATCH] fix(doctor,cycle): stop permanent cycle_freshness FAILs on multi-source installs (#2540) (#3562) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the paths #3382 left open (its author said it narrowed the issue rather than closing it): 1. checkCycleFreshness iterates EVERY local_path source, so an install that nightly-dreams one vault via --dir showed a permanent FAIL for every other federated source — and for any source added minutes ago. 'Never completed a full cycle' is now a WARN with the dream/autopilot hint; a source that HAS cycled and then went stale still escalates through the 6h warn / 24h fail thresholds (the regression signal the check exists for). This is the reporter's actual case: the permanent red eroded doctor's signal until real staleness hid inside it. 2. resolveSourceForDir's exact-match lookup had no archived filter and no ORDER BY, so an archived (or duplicate) alias of the same path could shadow the active source; dream's archived guard then refused the stamp and the ACTIVE source stayed unstamped forever. The lookup now excludes archived rows and orders deterministically, matching the canonical-path fallback's posture. The fallback's fail-closed ambiguity handling is deliberately unchanged. 3. #3382's own regression test (ii) was environment-sensitive: it assumed unsetting OPENAI_API_KEY/ANTHROPIC_API_KEY makes the embed phase fail, which is false wherever another embedding provider resolves (the cycle then reports 'clean' and the test flips). It now fails the sync phase against a vanished checkout — deterministic on every machine, same property pinned (a genuinely failing enabled phase must prevent the stamp). New pins fail on unmodified master and pass here: never-cycled→warn (x2, doctor) and the archived-alias shadow (dream --dir stamp). Fixes #2540 Co-authored-by: Garry Tan Co-authored-by: Claude Opus 5 --- src/commands/doctor.ts | 14 +++++++-- src/core/cycle.ts | 11 ++++++- test/cycle-enabled-phase-completeness.test.ts | 22 +++++++++----- test/doctor-cycle-freshness.test.ts | 30 +++++++++++++++++-- test/dream-dir-source-stamp.test.ts | 22 ++++++++++++++ 5 files changed, 86 insertions(+), 13 deletions(-) diff --git a/src/commands/doctor.ts b/src/commands/doctor.ts index 1b514e18c..b43b1a24e 100644 --- a/src/commands/doctor.ts +++ b/src/commands/doctor.ts @@ -4349,8 +4349,18 @@ export async function checkCycleFreshness( : `'${source.id}'`; const raw = source.config?.last_full_cycle_at; if (typeof raw !== 'string') { + // #2540: WARN, not FAIL. This check iterates EVERY local_path source, + // so on a multi-source install where only some vaults are cycled + // (e.g. one nightly `gbrain dream --dir `), a never-cycled + // sibling source turned doctor permanently red — which erodes the + // check's signal until real staleness hides inside the noise (the + // reporter's install masked genuinely stale sources for weeks this + // way). "Never cycled" also fires on a source added minutes ago. + // A source that HAS cycled and then went stale still escalates + // through the warn/fail age thresholds below — that is the + // regression signal this check exists for. issues.push(`Source ${display} has never completed a full cycle`); - hasFailures = true; + hasWarnings = true; continue; } const last = new Date(raw).getTime(); @@ -4386,7 +4396,7 @@ export async function checkCycleFreshness( return { name: 'cycle_freshness', status: 'warn', - message: `${issues.join('; ')}.`, + message: `${issues.join('; ')}. Run \`gbrain dream --source \` to cycle a source, or start \`gbrain autopilot\`.`, }; } return { diff --git a/src/core/cycle.ts b/src/core/cycle.ts index d79da17c9..dcc18c1e8 100644 --- a/src/core/cycle.ts +++ b/src/core/cycle.ts @@ -895,8 +895,17 @@ export async function resolveSourceForDir( // (the cycleSourceId precedence) or 'default'. if (brainDir === null) return undefined; try { + // #2540: exclude archived rows (dream's --source guard refuses to stamp + // them, so an archived alias winning here means the stamp silently never + // lands and doctor's cycle_freshness stays red on a healthy install) and + // order deterministically so a duplicate registration of the same path + // can't shadow the active source on whichever row the engine scans first. + // Ordering matches listAllSources/sources-ops for operator-output parity. const rows = await engine.executeRaw<{ id: string }>( - `SELECT id FROM sources WHERE local_path = $1 LIMIT 1`, + `SELECT id FROM sources + WHERE local_path = $1 AND archived = false + ORDER BY (id = 'default') DESC, id + LIMIT 1`, [brainDir], ); if (rows[0]) return rows[0].id; diff --git a/test/cycle-enabled-phase-completeness.test.ts b/test/cycle-enabled-phase-completeness.test.ts index d62c8109c..e74bb009f 100644 --- a/test/cycle-enabled-phase-completeness.test.ts +++ b/test/cycle-enabled-phase-completeness.test.ts @@ -38,7 +38,7 @@ import { PGLiteEngine } from '../src/core/pglite-engine.ts'; import { resetPgliteState } from './helpers/reset-pglite.ts'; import { withEnv, emptyHome } from './helpers/with-env.ts'; import { runCycle, ALL_PHASES } from '../src/core/cycle.ts'; -import { mkdtempSync, writeFileSync } from 'fs'; +import { mkdtempSync, writeFileSync, rmSync } from 'fs'; import { execSync } from 'child_process'; import { tmpdir } from 'os'; import { join } from 'path'; @@ -139,19 +139,25 @@ describe('#2540 (i) — pack omitting optional phases, all enabled phases comple describe('#2540 (ii) — an enabled phase that never completes still prevents the stamp', () => { test('every selected phase failing reports status=failed and does NOT stamp last_full_cycle_at', async () => { - await withEnv({ GBRAIN_HOME: gbrainHome, OPENAI_API_KEY: undefined, ANTHROPIC_API_KEY: undefined }, async () => { + await withEnv({ GBRAIN_HOME: gbrainHome }, async () => { await seedSource('always-fails'); expect(await readLastFullCycleAt('always-fails')).toBeNull(); - // embed is a real, always-enabled phase (no pack gate, no config - // .enabled toggle). With no embedding provider key configured it - // deterministically fails — this is NOT the fix under test, it's - // the pre-existing "an enabled phase genuinely never completes" - // case the issue says must keep failing doctor's check. + // Deterministic, environment-independent failure: run the sync phase + // against a brain directory that no longer exists. The previous shape + // ('embed' with OPENAI_API_KEY/ANTHROPIC_API_KEY unset) was + // environment-sensitive — on a machine where any OTHER embedding + // provider resolves (Voyage, ZeroEntropy, a local endpoint, …), embed + // with zero stale chunks succeeds and the cycle reports 'clean', + // flipping this test's expectation. A vanished checkout fails the + // sync phase on every machine. This is NOT the fix under test; it's + // the pre-existing "an enabled phase genuinely never completes" case + // the issue says must keep failing doctor's check. + rmSync(brainDir, { recursive: true, force: true }); const report = await runCycle(engine, { brainDir, sourceId: 'always-fails', - phases: ['embed'], + phases: ['sync'], }); expect(report.status).toBe('failed'); diff --git a/test/doctor-cycle-freshness.test.ts b/test/doctor-cycle-freshness.test.ts index 1eebc216d..5baf75bcc 100644 --- a/test/doctor-cycle-freshness.test.ts +++ b/test/doctor-cycle-freshness.test.ts @@ -79,12 +79,38 @@ describe('doctor checkCycleFreshness', () => { expect(result.message).toMatch(/gbrain dream --source/); }); - test('source with NO last_full_cycle_at (never cycled) returns fail', async () => { + test('source with NO last_full_cycle_at (never cycled) returns warn, not fail (#2540)', async () => { + // #2540: never-cycled used to FAIL, which turned doctor permanently red + // on any install that doesn't cycle every local_path source (e.g. one + // nightly `dream --dir ` plus other federated sources) — and on + // any source added minutes ago. It surfaces as a warning; only a source + // that HAS cycled and then went stale escalates to fail. await engine.executeRaw(`UPDATE sources SET local_path = NULL WHERE id = 'default'`); await seed('virgin'); const result = await checkCycleFreshness(engine, { nowMs: NOW }); - expect(result.status).toBe('fail'); + expect(result.status).toBe('warn'); expect(result.message).toMatch(/never completed a full cycle/); + expect(result.message).toMatch(/gbrain dream --source/); + }); + + test('reporter case (#2540): one cycled vault + never-cycled siblings is warn, not permanent fail', async () => { + await engine.executeRaw(`UPDATE sources SET local_path = NULL WHERE id = 'default'`); + await seed('nightly-vault', agoH(2)); // the one vault dreamt via --dir + await seed('federated-a'); // never cycled + await seed('federated-b'); // never cycled + const result = await checkCycleFreshness(engine, { nowMs: NOW }); + expect(result.status).toBe('warn'); + expect(result.message).toMatch(/federated-a/); + expect(result.message).toMatch(/federated-b/); + expect(result.message).not.toMatch(/nightly-vault/); + }); + + test('a previously-cycled source gone stale still fails even next to never-cycled sources', async () => { + await engine.executeRaw(`UPDATE sources SET local_path = NULL WHERE id = 'default'`); + await seed('stale', agoH(72)); // real regression signal + await seed('virgin'); // never cycled — warn-only + const result = await checkCycleFreshness(engine, { nowMs: NOW }); + expect(result.status).toBe('fail'); }); test('mixed sources: highest severity wins (fail > warn > ok)', async () => { diff --git a/test/dream-dir-source-stamp.test.ts b/test/dream-dir-source-stamp.test.ts index 1e9d62969..06e948cf1 100644 --- a/test/dream-dir-source-stamp.test.ts +++ b/test/dream-dir-source-stamp.test.ts @@ -96,6 +96,28 @@ describe('gbrain dream --dir freshness stamp (#1869)', () => { expect(await readLastFullCycleAt('mothballed')).toBeNull(); }); }, 60_000); + + test('an ARCHIVED alias of the same path does not shadow the active source (#2540)', async () => { + await withEnv({ GBRAIN_HOME: gbrainHome }, async () => { + // Ordinary shape: a source was archived and re-added under a new id + // pointing at the same checkout. Seed the archived twin FIRST so a + // filterless `LIMIT 1` scan finds it first. + await seedSource('retired-twin', true); + await seedSource('active-twin', false); + + const report = await runDream(engine, ['--dir', brainDir, '--phase', 'lint', '--json']); + expect(report).toBeTruthy(); + if (report) expect(['ok', 'clean']).toContain(report.status); + + // Pre-fix, resolveSourceForDir's exact match had no `archived = false` + // filter and no ORDER BY, so the archived twin won the lookup; dream's + // archived guard then (correctly) refused to stamp it — and the ACTIVE + // source silently never got its stamp, leaving doctor's cycle_freshness + // permanently stale on a healthy install. + expect(await readLastFullCycleAt('active-twin')).not.toBeNull(); + expect(await readLastFullCycleAt('retired-twin')).toBeNull(); + }); + }, 60_000); }); /**