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