fix(group): address gitnexus-check findings

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

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

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<string, unknown>)[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);
+  });
+});

gitnexus/test/unit/group/service-group-sync-payload.test.ts

@@ -87,6 +87,15 @@ const syncResult = (overrides: Partial<SyncResult> = {}): 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<string, unknown> =>
+  (syncGroupMock.mock.calls[call] as unknown as unknown[])[1] as Record<string, unknown>;
+
 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<string, unknown> = {};
+    cyclic.self = cyclic;
+
+    const fromBigInt = (await new GroupService(port).groupSync({
+      name: GROUP,
+      exactOnly: 1n,
+    })) as Record<string, unknown>;
+    const fromCyclic = (await new GroupService(port).groupSync({
+      name: GROUP,
+      exactOnly: cyclic,
+    })) as Record<string, unknown>;
+
+    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