From 6ec8dfc3cd55476998768785fad5e4ac458ac450 Mon Sep 17 00:00:00 2001 From: ReidenXerx Date: Thu, 6 Aug 2026 02:17:40 +0300 Subject: [PATCH] fix(analyze): never report a collapse from a non-numeric count MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The B2 check reported healthy runs as total graph-write collapses. A non-numeric `expected` (a graph implementation reporting no total, a lightweight pipeline result) does not skip the guards — it INVERTS them: `undefined < 100` is false, so the small-repo exemption never fires, and `0 >= undefined * 0.5` is `0 >= NaN`, also false, so the ratio check "passes" as well. Both bounds silently evaporate and every such run is flagged. That is precisely the failure this check was written to catch, reproduced inside the check itself: an unmeasurable quantity treated as a measured zero. Both sides are now validated as finite numbers before any comparison. `persisted` is also passed as UNKNOWN rather than zero when the DB was not demonstrably readable: `getLbugStats` flattens "no connection", "query threw" and "empty table" all into `edges: 0`, so `stats.nodes > 0` is used as independent evidence the read happened at all. Caught by the existing run-analyze suites, not by the new unit tests — those exercised the pure function with well-formed numbers and were blind to the integration's actual inputs. Both cases are now pinned. Co-Authored-By: Claude Opus 5 (1M context) --- gitnexus/src/core/index-freshness.ts | 30 ++++++++++++++++--- gitnexus/src/core/run-analyze.ts | 13 ++++++-- .../index-freshness-graph-collapse.test.ts | 18 +++++++++++ 3 files changed, 55 insertions(+), 6 deletions(-) diff --git a/gitnexus/src/core/index-freshness.ts b/gitnexus/src/core/index-freshness.ts index f063e446c..d8ba97841 100644 --- a/gitnexus/src/core/index-freshness.ts +++ b/gitnexus/src/core/index-freshness.ts @@ -38,11 +38,33 @@ export const GRAPH_WRITE_COLLAPSE_MIN_EDGES = 100; */ export function detectGraphWriteCollapse( expected: number, - persisted: number, + /** + * Relationships readable from the DB, or `undefined` when the count could + * not be READ at all (no connection, a query that threw). + * + * The distinction is load-bearing and was got wrong once: `getLbugStats` + * reports `edges: 0` for "no connection", "query threw" AND "empty table" + * alike, so passing it straight in made every run without a readable DB look + * like a total collapse. An unmeasurable count is not a measured zero — + * accepting `undefined` here is what keeps this check from committing the + * same confident-zero error it exists to catch. + */ + persisted: number | undefined, ): { expected: number; persisted: number } | undefined { - if (expected < GRAPH_WRITE_COLLAPSE_MIN_EDGES) return undefined; - if (persisted >= expected * GRAPH_WRITE_COLLAPSE_RATIO) return undefined; - return { expected, persisted }; + // Both sides must be REAL NUMBERS before any comparison. A non-numeric + // `expected` (a graph implementation that reports no total, a lightweight + // pipeline result) does not merely skip the guards — it INVERTS them: + // `undefined < 100` is false, so the min-edges exemption never fires, and + // `0 >= undefined * 0.5` is `0 >= NaN`, also false, so the ratio check + // "passes" too and a healthy run is reported as a total collapse. Comparing + // against a non-number is the one way this check can manufacture the exact + // false certainty it was written to prevent. + if (!Number.isFinite(expected) || !Number.isFinite(persisted as number)) return undefined; + const expectedCount = expected as number; + const persistedCount = persisted as number; + if (expectedCount < GRAPH_WRITE_COLLAPSE_MIN_EDGES) return undefined; + if (persistedCount >= expectedCount * GRAPH_WRITE_COLLAPSE_RATIO) return undefined; + return { expected: expectedCount, persisted: persistedCount }; } /** Stable machine-readable reasons an index cannot be certified complete. */ diff --git a/gitnexus/src/core/run-analyze.ts b/gitnexus/src/core/run-analyze.ts index 96c939304..45f117283 100644 --- a/gitnexus/src/core/run-analyze.ts +++ b/gitnexus/src/core/run-analyze.ts @@ -2539,11 +2539,20 @@ async function runFullAnalysisInner( // relationships out of memory may no longer be able to report a total, and // a false "your index is broken" is worse than a missed one. const expectedRelationships = pipelineResult.graph.relationshipCount; - const graphWriteCollapsed = detectGraphWriteCollapse(expectedRelationships, stats.edges); + // `getLbugStats` flattens "no connection", "query threw" and "empty table" + // into `edges: 0`, so a bare `stats.edges` would make every run without a + // readable DB look like a total collapse — the confident-zero error this + // check exists to catch. `nodes > 0` is independent evidence the DB was + // readable at all; without it the count is UNKNOWN, not zero. + const persistedRelationships = stats.nodes > 0 ? stats.edges : undefined; + const graphWriteCollapsed = detectGraphWriteCollapse( + expectedRelationships, + persistedRelationships, + ); if (graphWriteCollapsed) { log( `Warning: graph write incomplete — the pipeline produced ${expectedRelationships} ` + - `relationships but only ${stats.edges} are readable from the index. Recording the ` + + `relationships but only ${persistedRelationships} are readable from the index. Recording the ` + `index as INCOMPLETE (graph-write-collapsed) rather than fresh; re-run ` + `\`gitnexus analyze --force\`.`, ); diff --git a/gitnexus/test/unit/index-freshness-graph-collapse.test.ts b/gitnexus/test/unit/index-freshness-graph-collapse.test.ts index bcff4d732..1ff79ee61 100644 --- a/gitnexus/test/unit/index-freshness-graph-collapse.test.ts +++ b/gitnexus/test/unit/index-freshness-graph-collapse.test.ts @@ -40,6 +40,24 @@ describe('detectGraphWriteCollapse (B2 detection)', () => { expect(detectGraphWriteCollapse(1000, 4000)).toBeUndefined(); }); + // REGRESSION. A non-numeric `expected` does not merely skip the guards, it + // INVERTS them: `undefined < 100` is false so the small-repo exemption never + // fires, and `0 >= undefined * 0.5` is `0 >= NaN`, also false, so the ratio + // check "passes" too. Shipped briefly and reported healthy runs as total + // collapses — the exact false certainty this check exists to prevent. + it('never fires when the expected count is not a number', () => { + expect(detectGraphWriteCollapse(undefined as unknown as number, 0)).toBeUndefined(); + expect(detectGraphWriteCollapse(NaN, 0)).toBeUndefined(); + expect(detectGraphWriteCollapse(Infinity, 0)).toBeUndefined(); + }); + + it('never fires when the persisted count is not a number', () => { + // `getLbugStats` returns `{}` under some mocks/degraded paths, so + // `stats.edges` arrives as undefined rather than a measured zero. + expect(detectGraphWriteCollapse(23009, undefined)).toBeUndefined(); + expect(detectGraphWriteCollapse(23009, NaN)).toBeUndefined(); + }); + it('is fail-safe when the expected count is unavailable', () => { // An implementation that offloads relationships out of memory may report 0; // a false "your index is broken" is worse than a missed one.