From 59e0e0725ac2cb3ad227d8a3b5975a5fb4dc9f0f Mon Sep 17 00:00:00 2001 From: Gergo Magyar Date: Thu, 27 Aug 2026 16:50:12 +0000 Subject: [PATCH] fix(group): address gitnexus-check findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Seven bot comments across two review rounds; five distinct after dedup. Four were valid and are fixed, two were already resolved by later commits the bot had not seen. The validator could throw from its own error path. `JSON.stringify` is the right renderer there — it is what distinguishes the string "false" from the boolean, which is the entire point of the message — but it throws on a BigInt and on a cyclic object. So a validator promising a structured `{ error }` instead rejected, and `callTool` is reachable directly, so neither input is hypothetical. Guarded, keeping the distinction and falling back for the shapes that cannot serialize. An unreadable suppression record read as "nothing was suppressed". `recordedMatchStages` is all-or-nothing by design, so garbage collapses to `undefined` — and the consumer treated `undefined` as an empty measurement, throwing that safety away and reporting a registry it could not parse as complete. Present-but-unreadable now forces the floor, while absent stays legitimate: a registry written before the field existed has no opinion and should not be dragged to a floor for it. Two test-side findings, both real and both invisible to CI because `tsconfig.json` is src-only. Three `mock.calls[0][1]` accesses did not type-check against a zero-arg mock, and four assertions read `truncationReason` / `riskEpistemic` straight off `CrossRepoCompleteness`, which is a discriminated union carrying them on one arm. Also removed a `StoredContract` import that went dead when those fixtures moved to `makeWildcardPair`. Worth recording: U6 set a test-config gate at 989 errors and later commits walked it to 994 without anyone re-measuring — the bot caught three of the five. Now 987, below the original baseline. Already fixed, not by this commit: the preserve-path stamp the bot flagged against 6ceac8b1f (fixed in 1fbe0dc6b) and the displaced completeness JSDoc (fixed in 2d2ef8c47). Both behavior fixes are mutation-verified: restoring the unguarded stringify turns the new unserializable-value test red, and a control pins that an absent record still reads as complete so the fails-closed change cannot pass by forcing every registry to a floor. src tsc clean; eslint clean; 1167/1167. --- gitnexus/src/core/group/service.ts | 43 ++++++++++-- .../group/registry-suppressed-stages.test.ts | 68 ++++++++++++++++--- .../group/service-group-sync-payload.test.ts | 38 ++++++++++- 3 files changed, 130 insertions(+), 19 deletions(-) diff --git a/gitnexus/src/core/group/service.ts b/gitnexus/src/core/group/service.ts index a19f57057..af1f1385a 100644 --- a/gitnexus/src/core/group/service.ts +++ b/gitnexus/src/core/group/service.ts @@ -262,7 +262,8 @@ function registryIdentifies(entries: RegistryEntry[], registryName: string): boo async function loadContractRegistryResilient( groupDir: string, ): Promise< - { ok: true; registry: ContractRegistry; skippedCorrupt: number } | { ok: false; error: string } + | { ok: true; registry: ContractRegistry; skippedCorrupt: number; suppressionUnreadable: boolean } + | { ok: false; error: string } > { const filePath = path.join(groupDir, 'contracts.json'); let raw: string; @@ -328,6 +329,14 @@ async function loadContractRegistryResilient( // Bound once: the gate is a full array scan and the ternary below used it twice. const recordedUnreadable = recordedRepoList(base.unreadableRepos); const recordedSuppressed = recordedMatchStages(base.suppressedMatchStages); + // Present-but-unreadable is NOT the same as absent. `recordedMatchStages` is + // all-or-nothing, so garbage collapses to `undefined` — and a consumer that + // reads `undefined` as "nothing was suppressed" would throw that safety away + // and report a registry it could not parse as complete. Absent stays + // legitimate (a registry predating the field); only a value that was there + // and unreadable forces the answer to a floor. + const suppressionUnreadable = + base.suppressedMatchStages !== undefined && recordedSuppressed === undefined; const registry: ContractRegistry = { version: typeof base.version === 'number' ? base.version : 0, generatedAt: typeof base.generatedAt === 'string' ? base.generatedAt : '', @@ -357,7 +366,7 @@ async function loadContractRegistryResilient( crossLinks, }; - return { ok: true, registry, skippedCorrupt }; + return { ok: true, registry, skippedCorrupt, suppressionUnreadable }; } /** @@ -377,9 +386,27 @@ async function loadContractRegistryResilient( function validateBooleanParam(name: string, raw: unknown): { value: boolean } | { error: string } { if (raw === undefined) return { value: false }; if (typeof raw === 'boolean') return { value: raw }; - return { - error: `Invalid "${name}": expected true or false, got ${JSON.stringify(raw)}.`, - }; + return { error: `Invalid "${name}": expected true or false, got ${describeValue(raw)}.` }; +} + +/** + * Render an untrusted value for an error message, without throwing. + * + * `JSON.stringify` is the right shape here — it distinguishes the string + * `"false"` from the boolean, which is the whole point of the message — but it + * throws on a BigInt and on a cyclic object. A validator whose ERROR path can + * throw does not return the structured `{ error }` it promises: the caller gets + * a rejected promise instead of feedback it can act on, and `callTool` is + * reachable directly, so neither input is hypothetical. + */ +function describeValue(raw: unknown): string { + try { + const rendered = JSON.stringify(raw); + // `undefined`, a function, or a symbol serialize to `undefined`. + return rendered ?? String(raw); + } catch { + return typeof raw === 'bigint' ? `${raw}n` : Object.prototype.toString.call(raw); + } } /** @@ -527,7 +554,11 @@ export class GroupService { suppressedMatchStages: registry.suppressedMatchStages, // An unrecorded `unreadableRepos` means this listing cannot say which // repos the sync failed to read — so it cannot claim to be complete. - provenanceUnknown: unreadableRepos === undefined, + // Either kind of unreadable provenance forces the floor: a sync that + // could not say which repos it read, or a suppression record that was + // present and could not be parsed. Reading the second as "nothing was + // suppressed" would report an unparseable registry as complete. + provenanceUnknown: unreadableRepos === undefined || loaded.suppressionUnreadable, // A contract LISTING declares no scope to intersect with: it is the whole // registry, so every configured repo is in scope by construction. The // `type`/`repo`/`unmatchedOnly` filters above narrow which rows are shown, diff --git a/gitnexus/test/unit/group/registry-suppressed-stages.test.ts b/gitnexus/test/unit/group/registry-suppressed-stages.test.ts index b2eb2bd20..88a9ecea5 100644 --- a/gitnexus/test/unit/group/registry-suppressed-stages.test.ts +++ b/gitnexus/test/unit/group/registry-suppressed-stages.test.ts @@ -23,13 +23,12 @@ import os from 'node:os'; import path from 'node:path'; import { syncGroup } from '../../../src/core/group/sync.js'; import { makeWildcardPair } from './fixtures.js'; -import { crossRepoCompleteness } from '../../../src/core/group/completeness.js'; +import { + crossRepoCompleteness, + type CrossRepoCompleteness, +} from '../../../src/core/group/completeness.js'; import { GROUP_IMPACT_TRUNCATION_REASONS } from '../../../src/core/group/types.js'; -import type { - GroupConfig, - StoredContract, - ContractRegistry, -} from '../../../src/core/group/types.js'; +import type { GroupConfig, ContractRegistry } from '../../../src/core/group/types.js'; const config: GroupConfig = { version: 1, @@ -75,6 +74,16 @@ const run = (exactOnly: boolean, opts: { write: boolean } = { write: false }) => const readRegistry = (): ContractRegistry => JSON.parse(fs.readFileSync(path.join(groupDir, 'contracts.json'), 'utf8')) as ContractRegistry; +/** + * `CrossRepoCompleteness` is a discriminated union: `truncationReason` and + * `riskEpistemic` exist only on the `truncated: true` arm, so reading them off + * the union directly does not type-check. These read them positionally, the + * same way this suite reads a preserved key its type no longer carries. + */ +const fieldOf = (out: CrossRepoCompleteness, key: string): unknown => + (out as unknown as Record)[key]; +const reasonOf = (out: CrossRepoCompleteness): unknown => fieldOf(out, 'truncationReason'); + describe('a sync records the matching stages it was told to skip', () => { it('names the wildcard stage when exactOnly suppressed it', async () => { const result = await run(true); @@ -126,8 +135,8 @@ describe('cross-repo completeness reflects a suppressed stage', () => { }); expect(out.truncated).toBe(true); - expect(out.truncationReason).toBe('suppressed-stage'); - expect(out.riskEpistemic).toBe('lower-bound'); + expect(reasonOf(out)).toBe('suppressed-stage'); + expect(fieldOf(out, 'riskEpistemic')).toBe('lower-bound'); }); // control: without a suppressed stage the same clean input is complete. @@ -142,7 +151,7 @@ describe('cross-repo completeness reflects a suppressed stage', () => { }); expect(out.truncated).toBe(false); - expect(out.truncationReason).toBeUndefined(); + expect(reasonOf(out)).toBeUndefined(); }); // An unreadable repo is the more serious gap and its remedy differs, so it @@ -157,7 +166,7 @@ describe('cross-repo completeness reflects a suppressed stage', () => { }); expect(out.truncated).toBe(true); - expect(out.truncationReason).toBe('incomplete-sync'); + expect(reasonOf(out)).toBe('incomplete-sync'); }); }); @@ -177,3 +186,42 @@ describe('the suppressed-stage reason is reachable, not just documented', () => expect(GROUP_IMPACT_TRUNCATION_REASONS).toContain('suppressed-stage'); }); }); + +/** + * An UNREADABLE suppression record must not read as "nothing was suppressed". + * + * `recordedMatchStages` is all-or-nothing on purpose: garbage collapses to + * `undefined`. A consumer that then treats `undefined` as an empty measurement + * throws that safety away and reports a registry it could not parse as + * complete. Absent stays legitimate — a registry written before the field + * existed has no opinion and should not be forced to a floor. + */ +describe('an unreadable suppression record fails closed', () => { + it('does not report a registry it could not parse as complete', () => { + const garbage = crossRepoCompleteness({ + unreadableRepos: [], + missingRepos: [], + suppressedMatchStages: [], + // What `loadContractRegistryResilient` now passes when the stored value + // was present and could not be read. + provenanceUnknown: true, + inScope: () => true, + }); + + expect(garbage.truncated).toBe(true); + expect(reasonOf(garbage)).toBe('incomplete-sync'); + }); + + // control: an absent record is not an unreadable one. Without this, forcing + // every pre-existing registry to a floor would pass the case above. + it('control: a clean registry with nothing recorded stays complete', () => { + const clean = crossRepoCompleteness({ + unreadableRepos: [], + missingRepos: [], + provenanceUnknown: false, + inScope: () => true, + }); + + expect(clean.truncated).toBe(false); + }); +}); diff --git a/gitnexus/test/unit/group/service-group-sync-payload.test.ts b/gitnexus/test/unit/group/service-group-sync-payload.test.ts index 023cdd06b..ddfa32d71 100644 --- a/gitnexus/test/unit/group/service-group-sync-payload.test.ts +++ b/gitnexus/test/unit/group/service-group-sync-payload.test.ts @@ -87,6 +87,15 @@ const syncResult = (overrides: Partial = {}): SyncResult => ({ ...overrides, }); +/** + * `syncGroupMock` is declared zero-arg, so `mock.calls` is typed as an array of + * the empty tuple and indexing `[1]` does not type-check. The runtime call + * genuinely has two arguments (config, options); this reads the second without + * restating a signature the rest of the suite does not need. + */ +const syncOptsOf = (call: number): Record => + (syncGroupMock.mock.calls[call] as unknown as unknown[])[1] as Record; + const CONTRACT = makeContract({ repo: 'app/backend' }); const CROSS_LINK: CrossLink = { contractId: CONTRACT.contractId, @@ -353,7 +362,30 @@ describe('group_sync rejects malformed and retired parameters', () => { expect(payload.error).toBeUndefined(); expect(syncGroupMock).toHaveBeenCalledTimes(1); - expect(syncGroupMock.mock.calls[0][1]).not.toHaveProperty('verbose'); + expect(syncOptsOf(0)).not.toHaveProperty('verbose'); + }); + + // The error path must not throw. `JSON.stringify` — the right renderer here, + // because it distinguishes the string "false" from the boolean — throws on a + // BigInt and on a cyclic object, and `callTool` is reachable directly, so a + // validator that rejects instead of returning `{ error }` breaks its own + // contract on inputs a caller can actually send. + it('returns a structured error rather than throwing on an unserializable value', async () => { + const cyclic: Record = {}; + cyclic.self = cyclic; + + const fromBigInt = (await new GroupService(port).groupSync({ + name: GROUP, + exactOnly: 1n, + })) as Record; + const fromCyclic = (await new GroupService(port).groupSync({ + name: GROUP, + exactOnly: cyclic, + })) as Record; + + expect(String(fromBigInt.error)).toContain('Invalid "exactOnly"'); + expect(String(fromCyclic.error)).toContain('Invalid "exactOnly"'); + expect(syncGroupMock).not.toHaveBeenCalled(); }); it.each([['skipEmbeddings'], ['allowStale']])( @@ -376,7 +408,7 @@ describe('group_sync rejects malformed and retired parameters', () => { await new GroupService(port).groupSync({ name: GROUP, exactOnly: ok }); expect(syncGroupMock).toHaveBeenCalledTimes(1); - expect(syncGroupMock.mock.calls[0][1]).toMatchObject({ exactOnly: ok }); + expect(syncOptsOf(0)).toMatchObject({ exactOnly: ok }); }, ); @@ -385,7 +417,7 @@ describe('group_sync rejects malformed and retired parameters', () => { await new GroupService(port).groupSync({ name: GROUP }); - expect(syncGroupMock.mock.calls[0][1]).toMatchObject({ exactOnly: false }); + expect(syncOptsOf(0)).toMatchObject({ exactOnly: false }); }); // control: the guards above reject specific shapes, not every call. Without