mirror of
https://github.com/abhigyanpatwari/GitNexus.git
synced 2026-08-28 05:25:25 +00:00
* fix(mcp): surface resolved repo/branch/indexed-at in the FTS-degraded warning Turns the generic "FTS indexes missing" message into a diagnostic that reveals what this MCP session actually resolved, so a CLI/MCP mismatch or stale-connection theory is visible in the warning text itself instead of requiring a separate debugging round-trip (#2767). * fix(mcp): stop swallowing real FTS query errors behind the missing-index message queryFTSViaExecutor previously collapsed 'index genuinely missing' and 'a real query/connection error occurred' into the same silent null, so a real failure could masquerade as the generic FTS-degraded message with no diagnostic trail — even when it happened on only some of the per-table queries while others succeeded. Classifies the failure (mirroring queryFTS's own check for this exact cypher call), always logs a non-benign error server-side regardless of overall outcome, and surfaces it (redacted) in the client warning only when every table failed (#2767). * fix(mcp): give --repair-fts a dedicated freshness signal for warm readers --repair-fts intentionally never restamps indexedAt (it doesn't regenerate the graph), so a long-lived MCP session's pool staleness check had no explicit signal that a repair happened, only the incidental file-identity delta. Reuses the existing (forensic-only) capabilities.fts.status field: repair-fts now stamps just that sub-field (everything else byte-identical), and ensureInitialized compares it as a third, independent reinit trigger alongside the existing stamp/identity checks, seeded at cold init too so a fresh process's first warm check doesn't false-trigger (#2767). * test(mcp): warm session picks up an out-of-band --repair-fts rebuild (#2767) New end-to-end integration test: a real writable LadybugDB session builds an index WITHOUT FTS, a real LocalBackend observes 'FTS indexes missing' through the real pool, a separate writable session performs the exact repair-fts writes (real createSearchFTSIndexes + the #2767 capability-only meta stamp), and the SAME still-warm backend re-queries successfully without a restart — closing the one end-to-end gap no existing test covered. Running this against the real engine surfaced a second real message shape for a missing FTS index ("doesn't have an index with name X", not just "does not exist") that the U2 classifier didn't recognize — fixed classifyFtsQueryError to match both, with a regression test pinning the exact observed string. * fix(review): address code-review findings on the #2767 FTS fix - Anchor classifyFtsQueryError to the exception class (mirroring isBenignDropFtsIndexError) instead of a bare substring search, so a real, differently-classed error that happens to echo the benign phrase in its body (e.g. an echoed user query) can't be misclassified as a benign missing-index (adversarial review). - Re-read the on-disk meta immediately before the --repair-fts capability stamp write instead of reusing the pre-rebuild snapshot, so a concurrent writer (e.g. the HTTP server's background embedding checkpoint job) landing mid-repair isn't silently reverted. - Surface a client-facing partial-result warning (mirroring the existing enrichmentDegraded convention) when some FTS tables succeed but at least one hits a real error, instead of only logging it server-side. - Update RepoMeta.capabilities' stale 'no programmatic readers' docstring now that ensureInitialized reads capabilities.fts.status. - Widen the warm-session integration test's polling deadline for more margin over the production 5s staleness-check throttle. * fix(ci): drop the cold-init loadMeta call ensureInitialized never needed It stole the mocked loadMeta call an unrelated upstream PDG test depends on (test/integration/impact-pdg-statement-precise.test.ts queues a single mockResolvedValueOnce for its own PDG-config read; the extra call consumed that slot before the PDG code ran, so it fell through to the mock's null default and epistemic came back undefined instead of 'pdg-intra-procedural'). Cold init now leaves lastObservedFtsStatus unseeded — the cost is at most one redundant initLbug call on the first warm check, which no-ops via a single fs.stat when nothing actually changed, not a real reopen. * fix(review): address tri-review findings on the #2767 FTS fix Fixes two P1s (misleading repair-fts advice on real query errors; embedding-checkpoint job silently reverting the capabilities.fts stamp for up to its 30-minute lifetime), five P2/P3s (stale indexedAt in warnings, extension-unavailable noise, mismatched log severity, a table-missing vs index-missing conflation confirmed against a live LadybugDB, and a reinit-watermark latching bug), and the four residual items already self-disclosed in this PR's description (shared FTS error classifier, consolidated per-pool observed-state map, a redactPaths whitespace gap, and an isolated ftsCapsChanged test). A /simplify pass afterward caught one more real bug: the extension-unavailable short-circuit only guarded the MCP pool path, so the CLI-path fix above it started surfacing spurious non-benign errors for the same expected degraded state the pool path stays silent on — now both paths agree. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Gergo Magyar <gergomagyar0@gmail.com> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
139 lines
6.1 KiB
TypeScript
139 lines
6.1 KiB
TypeScript
import { describe, it, expect, vi, beforeEach } from 'vitest';
|
|
|
|
// tri-review NEW-7: `lastObservedIndexedAt`/`lastObservedFtsStatus` used to be
|
|
// advanced BEFORE `initLbug` was awaited, unlike `lastObservedDbIdentity`
|
|
// (which is only advanced once `initLbug` confirms the pool rolled over). If
|
|
// `initLbug` threw, the watermark had already been latched to the new value —
|
|
// permanently hiding a failed reinit from every later staleness check, since
|
|
// a subsequent comparison against that same watermark would see no change.
|
|
// This isolates the fix: a poolKey with `identityChanged` always false (a
|
|
// nonexistent lbugPath — no backstop from the file-identity signal) must
|
|
// still retry after a transient `initLbug` failure.
|
|
|
|
const initLbugMock = vi.fn();
|
|
vi.mock('../../src/core/lbug/pool-adapter.js', async (importOriginal) => {
|
|
const actual = await importOriginal<typeof import('../../src/core/lbug/pool-adapter.js')>();
|
|
return {
|
|
...actual,
|
|
initLbug: (...args: any[]) => initLbugMock(...args),
|
|
isLbugReady: vi.fn().mockReturnValue(true),
|
|
};
|
|
});
|
|
|
|
const loadMetaMock = vi.fn();
|
|
vi.mock('../../src/storage/repo-manager.js', async (importOriginal) => {
|
|
const actual = await importOriginal<typeof import('../../src/storage/repo-manager.js')>();
|
|
return {
|
|
...actual,
|
|
loadMeta: (...args: any[]) => loadMetaMock(...args),
|
|
};
|
|
});
|
|
|
|
import { LocalBackend } from '../../src/mcp/local/local-backend';
|
|
|
|
describe('ensureInitialized reinit watermark (tri-review NEW-7)', () => {
|
|
const poolKey = '/tmp/nonexistent-repo/.gitnexus/lbug';
|
|
const repoHandle = { id: 'r1', name: 'r1', lbugPath: poolKey, indexedAt: 'v0' } as any;
|
|
let backend: any;
|
|
|
|
beforeEach(() => {
|
|
vi.clearAllMocks();
|
|
backend = new LocalBackend() as any;
|
|
// Seed the "warm, already initialized" precondition ensureInitialized
|
|
// requires to reach the staleness-check branch at all.
|
|
backend.initializedRepos.add(poolKey);
|
|
backend.lastStalenessCheck.set(poolKey, 0); // force past the 5s throttle
|
|
});
|
|
|
|
it('does not latch the fts-status watermark when initLbug throws, so the next staleness check retries', async () => {
|
|
loadMetaMock.mockResolvedValue({
|
|
indexedAt: 'v1',
|
|
capabilities: { fts: { status: 'available' } },
|
|
});
|
|
initLbugMock.mockRejectedValueOnce(new Error('lock timeout'));
|
|
|
|
await expect(backend.ensureInitialized(repoHandle)).rejects.toThrow('lock timeout');
|
|
|
|
// The failed reinit must NOT have advanced either watermark — a nonexistent
|
|
// lbugPath means dbIdentity never changes, so there is no other signal to
|
|
// fall back on for a retry.
|
|
expect(backend.lastObservedPoolState.get(poolKey)?.ftsStatus).toBeUndefined();
|
|
expect(backend.lastObservedPoolState.get(poolKey)?.indexedAt).toBeUndefined();
|
|
|
|
// Second staleness check (throttle reset again) with initLbug now succeeding.
|
|
backend.lastStalenessCheck.set(poolKey, 0);
|
|
initLbugMock.mockResolvedValueOnce(false); // "no real reopen needed" — still a completed call
|
|
|
|
await expect(backend.ensureInitialized(repoHandle)).resolves.toBeUndefined();
|
|
|
|
// The retry succeeded and the watermark is now current — proving the
|
|
// failed first attempt did not permanently suppress detection.
|
|
expect(backend.lastObservedPoolState.get(poolKey)?.ftsStatus).toBe('available');
|
|
expect(backend.lastObservedPoolState.get(poolKey)?.indexedAt).toBe('v1');
|
|
expect(initLbugMock).toHaveBeenCalledTimes(2);
|
|
});
|
|
});
|
|
|
|
describe('ensureInitialized ftsCapsChanged trigger, isolated from identityChanged (tri-review Residual-4)', () => {
|
|
// The only existing coverage for this reinit trigger is the integration
|
|
// test in fts-repair-warm-session.test.ts, where a REAL --repair-fts run
|
|
// also mutates the lbug file — so identityChanged is confounded with
|
|
// ftsCapsChanged there, and it's impossible to tell from that test alone
|
|
// whether ftsCapsChanged is actually load-bearing. This isolates it: a
|
|
// nonexistent lbugPath means statDbIdentity always resolves null, so
|
|
// identityChanged is provably false on every check — the ONLY way a reinit
|
|
// can fire here is via ftsCapsChanged (indexedAt is held constant too, so
|
|
// stampChanged is also false).
|
|
const poolKey = '/tmp/nonexistent-repo-caps-only/.gitnexus/lbug';
|
|
const repoHandle = { id: 'r2', name: 'r2', lbugPath: poolKey, indexedAt: 'same' } as any;
|
|
let backend: any;
|
|
|
|
beforeEach(() => {
|
|
vi.clearAllMocks();
|
|
backend = new LocalBackend() as any;
|
|
backend.initializedRepos.add(poolKey);
|
|
backend.lastStalenessCheck.set(poolKey, 0);
|
|
});
|
|
|
|
it('fires a reinit on a capabilities.fts.status change alone, with indexedAt and dbIdentity both unchanged', async () => {
|
|
// Seed a baseline observed state: same indexedAt the next loadMeta will
|
|
// report, a DIFFERENT ftsStatus, and dbIdentity null (matches what
|
|
// statDbIdentity will keep returning for this nonexistent path).
|
|
backend.lastObservedPoolState.set(poolKey, {
|
|
indexedAt: 'same',
|
|
ftsStatus: 'unavailable',
|
|
dbIdentity: null,
|
|
});
|
|
|
|
loadMetaMock.mockResolvedValue({
|
|
indexedAt: 'same', // unchanged — stampChanged must be false
|
|
capabilities: { fts: { status: 'available' } }, // changed — the only live signal
|
|
});
|
|
initLbugMock.mockResolvedValueOnce(true);
|
|
|
|
await backend.ensureInitialized(repoHandle);
|
|
|
|
// A reinit only happens inside the `if (stampChanged || identityChanged
|
|
// || ftsCapsChanged)` branch — initLbug being called at all here proves
|
|
// ftsCapsChanged fired, since the other two provably could not have.
|
|
expect(initLbugMock).toHaveBeenCalledTimes(1);
|
|
expect(backend.lastObservedPoolState.get(poolKey)?.ftsStatus).toBe('available');
|
|
});
|
|
|
|
it('does NOT fire a reinit when nothing observable changed (negative control)', async () => {
|
|
backend.lastObservedPoolState.set(poolKey, {
|
|
indexedAt: 'same',
|
|
ftsStatus: 'available',
|
|
dbIdentity: null,
|
|
});
|
|
|
|
loadMetaMock.mockResolvedValue({
|
|
indexedAt: 'same',
|
|
capabilities: { fts: { status: 'available' } }, // same as observed
|
|
});
|
|
|
|
await backend.ensureInitialized(repoHandle);
|
|
|
|
expect(initLbugMock).not.toHaveBeenCalled();
|
|
});
|
|
});
|