From cb4ce5c5a593d785c0bf3a74b684742772f5130c Mon Sep 17 00:00:00 2001 From: Gergo Magyar Date: Wed, 12 Aug 2026 12:40:32 +0000 Subject: [PATCH] fix(core): reject a fractional chunk size, and stop the truncation note overclaiming (#2915) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All five from the gitnexus-check bot's pass on the previous push; two were introduced by the cleanup round that preceded it. `chunk` guarded with `Number.isFinite`, which admits a fractional size — and that one does not fail, it DUPLICATES. `slice` truncates its indices while `i` does not, so size 1.5 yields slice(0, 1.5) = items 0-1 then slice(1.5, 3) = items 1-2, putting item 1 in two batches; a caller batching a query would send it twice. A size is a count, so `Number.isInteger`. Unreachable today (every caller passes a constant) but the guard existed precisely for the unreachable case, and the NaN half of it was already there. `mapConcurrent`'s per-item degradation contract had a hole: `onError` is caller-supplied and was invoked outside a try, so a throwing reporter rejected `settle`, rejected the whole `Promise.all` wave, and discarded the neighbouring successes the function exists to preserve. Reporting a failure must not become one. The CLI truncation note asserted "the counts and risk level still cover all of them", which is true only when `truncated` fires alone — with `partial` the counts are summed from the batches that succeeded. It now varies: a distinct string when both flags are set, saying the counts are a lower bound. This is the same claim already corrected in the tool description; the CLI text still had the old one. The di-extractors contract docstring claimed the barrel re-exports everything from it. That stopped being true when the re-export was trimmed to what is actually imported, one commit earlier. The real-engine wiki test claimed to prepare "every exported query" and omitted `getInterModuleEdgesForOverview`, which `generateOverview` calls. Added — it aggregates in JS over `getInterFileCallEdges` rather than issuing its own Cypher, so the note says why it is in a prepare test. Verified and NOT changed: the bot's other error, that detect_changes compares 1-based hunks against 0-based graph lines. `bounds` is built from `coalesceHunksByPath`, which converts both ends at the grouping boundary (storage/git.ts), and two tests pin an edit landing on a symbol's last line. The remaining seven findings are changed-symbol heads-ups with no signature change; their callers' suites are green. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_0139ycbqnAorkJGQaZQXUNuv --- gitnexus/src/cli/detect-changes-format.ts | 8 +++++- gitnexus/src/cli/i18n/en.ts | 4 +++ gitnexus/src/cli/i18n/zh-CN.ts | 2 ++ .../src/core/ingestion/di-extractors/types.ts | 6 +++-- gitnexus/src/lib/utils.ts | 27 ++++++++++++++----- .../wiki-graph-queries-engine.test.ts | 5 ++++ gitnexus/test/unit/eval-formatters.test.ts | 6 +++++ gitnexus/test/unit/query-batch.test.ts | 9 +++++++ 8 files changed, 57 insertions(+), 10 deletions(-) diff --git a/gitnexus/src/cli/detect-changes-format.ts b/gitnexus/src/cli/detect-changes-format.ts index fb04757c0..98ecffa3e 100644 --- a/gitnexus/src/cli/detect-changes-format.ts +++ b/gitnexus/src/cli/detect-changes-format.ts @@ -46,7 +46,13 @@ export function formatDetectChangesResult(result: unknown): string { // Both lead the output — a caveat printed after the summary is read too late. const notes: string[] = []; if (payload.partial) notes.push(t('tool.detectChanges.partial')); - if (payload.truncated) notes.push(t('tool.detectChanges.truncated')); + // The plain truncation note reassures that the counts are whole. That is only + // true when the run did NOT also degrade — `changed_count` sums the batches + // that succeeded — so the two flags together get a different sentence. + if (payload.truncated) + notes.push( + t(payload.partial ? 'tool.detectChanges.truncatedDegraded' : 'tool.detectChanges.truncated'), + ); if ((summary.changed_count ?? 0) === 0) { return [...notes, t('tool.detectChanges.noChanges')].join('\n'); diff --git a/gitnexus/src/cli/i18n/en.ts b/gitnexus/src/cli/i18n/en.ts index 82fda6256..aaaf442cb 100644 --- a/gitnexus/src/cli/i18n/en.ts +++ b/gitnexus/src/cli/i18n/en.ts @@ -69,6 +69,10 @@ export const en = { 'PARTIAL RESULT: a graph query failed, so changed symbols may be missing. Do not read this as a clean pre-commit check.', 'tool.detectChanges.truncated': 'LISTING CAPPED: the changed-symbol list was capped, so it does not name every changed symbol. The counts and risk level still cover all of them.', + // The reassurance above is only true on its own. When the run also degraded, + // `changed_count` was summed from the batches that SUCCEEDED, so it is a floor. + 'tool.detectChanges.truncatedDegraded': + 'LISTING CAPPED: the changed-symbol list was capped. The run also degraded, so the counts are a lower bound, not a total.', 'tool.detectChanges.changesSummary': 'Changes: {{files}} files, {{symbols}} symbols', 'tool.detectChanges.affectedProcesses': 'Affected processes: {{count}}', 'tool.detectChanges.riskLevel': 'Risk level: {{risk}}', diff --git a/gitnexus/src/cli/i18n/zh-CN.ts b/gitnexus/src/cli/i18n/zh-CN.ts index 6d038bb7b..0ed1c7f1e 100644 --- a/gitnexus/src/cli/i18n/zh-CN.ts +++ b/gitnexus/src/cli/i18n/zh-CN.ts @@ -73,6 +73,8 @@ export const zhCN = { '结果不完整：图查询失败，可能遗漏已变更符号。请勿将其视为通过的提交前检查。', 'tool.detectChanges.truncated': '列表已截断：已变更符号列表被截断，未列出全部变更符号。计数与风险等级仍涵盖全部符号。', + 'tool.detectChanges.truncatedDegraded': + '列表已截断：已变更符号列表被截断。本次运行同时不完整，因此计数为下限而非总数。', 'tool.detectChanges.changesSummary': '变更：{{files}} 个文件，{{symbols}} 个符号', 'tool.detectChanges.affectedProcesses': '受影响流程：{{count}}', 'tool.detectChanges.riskLevel': '风险等级：{{risk}}', diff --git a/gitnexus/src/core/ingestion/di-extractors/types.ts b/gitnexus/src/core/ingestion/di-extractors/types.ts index ee3833f9a..5a0621263 100644 --- a/gitnexus/src/core/ingestion/di-extractors/types.ts +++ b/gitnexus/src/core/ingestion/di-extractors/types.ts @@ -5,8 +5,10 @@ * A leaf module by design: it imports nothing from this directory, so the * barrel (`./index.ts`, which aggregates the resolver *implementations*) and * each implementation (`./spring.ts`) can both depend on the contract without - * depending on each other. `./index.ts` re-exports everything here, so - * consumers may keep importing these types from the barrel. + * depending on each other. The barrel re-exports the two MATCH types, because + * consumers of the registry read them off its results; `DiResolver` is not + * re-exported, since only implementations need it and they import it from here + * directly. * * Mirrors the `import-resolvers/types.ts` split of contract from registry. */ diff --git a/gitnexus/src/lib/utils.ts b/gitnexus/src/lib/utils.ts index 2e25ecf97..0c5b7ad8a 100644 --- a/gitnexus/src/lib/utils.ts +++ b/gitnexus/src/lib/utils.ts @@ -135,12 +135,17 @@ export const stripWindowsLongPathPrefix = ( */ export function chunk(items: readonly T[], size: number): T[][] { // `NaN` fails every comparison, so a bare `size < 1` lets it through and - // `i += NaN` produces exactly one empty slice — the shape the docstring above - // promises never to return. `mapConcurrent`'s `Math.max(1, …)` propagates a - // NaN concurrency the same way, so the guard has to cover non-finite sizes - // rather than just small ones. - if (!Number.isFinite(size) || size < 1) { - throw new RangeError(`chunk size must be >= 1, got ${size}`); + // A size is a COUNT, so it has to be a positive integer — `Number.isInteger` + // rather than `Number.isFinite`, because a fractional size silently DUPLICATES + // items rather than failing: `slice` truncates its indices while `i` does not, + // so size 1.5 yields slice(0, 1.5) = items 0-1 and then slice(1.5, 3) = items + // 1-2, and item 1 lands in two batches. `NaN` is the other shape this rejects — + // it fails every comparison, so a bare `size < 1` lets it through and `i += NaN` + // produces exactly one empty slice, which the docstring promises never to + // return. `mapConcurrent`'s `Math.max(1, …)` propagates a NaN concurrency the + // same way. + if (!Number.isInteger(size) || size < 1) { + throw new RangeError(`chunk size must be a positive integer, got ${size}`); } const batches: T[][] = []; for (let i = 0; i < items.length; i += size) batches.push(items.slice(i, i + size)); @@ -171,7 +176,15 @@ export async function mapConcurrent( try { return await run(item); } catch (error) { - options.onError?.(error); + // Reporting a failure must not become a failure. `onError` is caller-supplied + // — a logger with a bad format string is enough — and an uncaught throw here + // rejects `settle`, which rejects the whole `Promise.all` wave and discards + // the neighbouring successes this function exists to preserve. + try { + options.onError?.(error); + } catch { + /* empty */ + } return undefined; } }; diff --git a/gitnexus/test/integration/wiki-graph-queries-engine.test.ts b/gitnexus/test/integration/wiki-graph-queries-engine.test.ts index 5d7d7a4d7..5633bbca2 100644 --- a/gitnexus/test/integration/wiki-graph-queries-engine.test.ts +++ b/gitnexus/test/integration/wiki-graph-queries-engine.test.ts @@ -29,6 +29,7 @@ import { getFilesWithExports, getInterFileCallEdges, getInterModuleCallEdges, + getInterModuleEdgesForOverview, getIntraModuleCallEdges, getProcessesForFiles, initWikiDb, @@ -259,6 +260,10 @@ withTestLbugDB( getInterModuleCallEdges(MODULE_FILES), getProcessesForFiles(MODULE_FILES), getAllProcesses(), + // Aggregates in JS over `getInterFileCallEdges`, so it issues no + // Cypher of its own — included anyway because this test claims to + // cover every exported query, and `generateOverview` calls it. + getInterModuleEdgesForOverview({ mod: MODULE_FILES, other: [GAMMA] }), ]), ).resolves.toBeDefined(); }); diff --git a/gitnexus/test/unit/eval-formatters.test.ts b/gitnexus/test/unit/eval-formatters.test.ts index b12cba6d2..c0349e80d 100644 --- a/gitnexus/test/unit/eval-formatters.test.ts +++ b/gitnexus/test/unit/eval-formatters.test.ts @@ -597,6 +597,12 @@ describe('formatDetectChangesResult', () => { expect(result.indexOf('PARTIAL RESULT')).toBe(0); expect(result.indexOf('LISTING CAPPED')).toBeGreaterThan(0); expect(result.indexOf('LISTING CAPPED')).toBeLessThan(result.indexOf('Changes: 40 files')); + // And it must NOT keep the truncated-only reassurance that the counts are + // whole: `changed_count` was summed from the batches that succeeded, so with + // `partial` it is a floor. Claiming otherwise here contradicts the note above + // it and the tool description. + expect(result).toContain('lower bound'); + expect(result).not.toContain('still cover all of them'); }); it('flags a capped listing that found nothing, alongside the no-changes line', () => { diff --git a/gitnexus/test/unit/query-batch.test.ts b/gitnexus/test/unit/query-batch.test.ts index dceb827ea..4acc0a964 100644 --- a/gitnexus/test/unit/query-batch.test.ts +++ b/gitnexus/test/unit/query-batch.test.ts @@ -47,4 +47,13 @@ describe('chunk', () => { // promises never to return, and one a caller reads as "nothing to query". expect(() => chunk([1], Number.NaN)).toThrow(RangeError); }); + + it('rejects a fractional size, which would DUPLICATE an item rather than fail', () => { + // The nastier sibling of the NaN case, because it produces a plausible-looking + // result instead of an empty one. `slice` truncates its indices but `i` does + // not, so size 1.5 gives slice(0, 1.5) = items 0-1 then slice(1.5, 3) = items + // 1-2: 'b' is in two batches, and a caller batching a query would send it + // twice. `Number.isFinite` admits this; only `Number.isInteger` rejects it. + expect(() => chunk(['a', 'b', 'c'], 1.5)).toThrow(RangeError); + }); });