fix(analyze): never report a collapse from a non-numeric count

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) <noreply@anthropic.com>
This commit is contained in:
ReidenXerx 2026-08-06 02:17:40 +03:00
parent 629bf3a9ed
commit 6ec8dfc3cd
3 changed files with 55 additions and 6 deletions

View file

@ -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. */

View file

@ -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\`.`,
);

View file

@ -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.