fix(group): make the suppressed-stage signal actually reach its readers

Applies the mechanical findings from the code review of the previous commit.
That commit claimed cross-repo impact and trace stop reporting a narrowed graph
as complete. Trace did; impact did not, and two operator-facing messages said
something false. Four reviewers plus the cross-model pass converged on the same
two defects, and the untested seams were exactly where they were.

`runGroupImpact` recomputed the truncation reason and hardcoded its fallback, so
it could never emit 'suppressed-stage' -- the value the previous commit added to
the union and documented in the tool description. Every narrowed-but-readable
bridge was reported as 'incomplete-sync', telling the caller to repair a repo
that read fine. It now propagates the bridge's own reason, as cross-trace.ts
already did.

The preserve path stamped this run's request onto an older bridge. When no repo
can be read the database and registry are kept from an earlier sync, so
meta.json has to keep describing that sync; instead `{ ...existing,
...diagnostics }` overwrote its marker, leaving contracts.json, meta.json and
bridge.lbug describing three different runs. Currently masked by unreadable-repo
precedence, one loosened condition from a live wrong verdict.

`group contracts` printed "the last sync did not record which repos it could
read" after any exact-only sync: truncated was set with both repo lists empty,
so the message fell through to the wrong branch. It is now gated on the reason,
not the flag. `group impact` likewise blamed the local walk for a floor the flag
caused.

The tri-state reader is now defined once, in the leaf module whose own comment
says it exists so this exact duplication cannot recur -- it had been copied into
bridge-db.ts within one commit of that comment being true.

Both agent-facing descriptions now name the field. The previous commit added it
to three payloads and documented it on none.

Tests cover what shipped green: the preserve path for both artifacts (verified
by mutation -- reintroducing the stamp turns exactly one test red), and the
reason's REACHABILITY. The existing guard only asserted each reason is
described, which is why a documented-but-unemittable value passed it.

Also corrects a comment that said the marker is deliberately not folded into the
truncation triple. True when written; false one commit later.

tsc clean; eslint 0 errors; 1163/1163 across the group unit, group integration
and tool-schema suites.
This commit is contained in:
Gergo Magyar 2026-08-27 15:59:58 +00:00
parent 6ceac8b1f6
commit 1fbe0dc6bf
10 changed files with 149 additions and 55 deletions

View file

@ -434,10 +434,17 @@ export function registerGroupCommands(program: Command): void {
// repos — reporting it as crossings understates a fan-out cap the
// same way #2787's totals did.
const dropped = (raw as { truncatedRepos?: string[] })?.truncatedRepos ?? [];
const reason = (raw as { truncationReason?: string })?.truncationReason;
// A floor caused by the flag is not a floor caused by the walk.
// Reported together, an operator re-runs the query or repairs a
// repo and gets the identical answer, because neither is the fix.
console.log(
dropped.length > 0
? ` risk is a LOWER BOUND — fan-out stopped early; crossings to ${dropped.length} repo(s) not traversed: ${dropped.join(', ')}`
: ' risk is a LOWER BOUND — the local impact walk did not complete (every bridge crossing was traversed)',
reason === 'suppressed-stage'
? ' risk is a LOWER BOUND — the last sync skipped a matching stage (--exact-only);' +
' re-run `gitnexus group sync` without it for the complete graph'
: dropped.length > 0
? ` risk is a LOWER BOUND — fan-out stopped early; crossings to ${dropped.length} repo(s) not traversed: ${dropped.join(', ')}`
: ' risk is a LOWER BOUND — the local impact walk did not complete (every bridge crossing was traversed)',
);
}
}
@ -530,6 +537,7 @@ export function registerGroupCommands(program: Command): void {
unreadableRepos,
missingRepos,
suppressedMatchStages,
truncationReason,
} = raw as {
contracts: Array<{
role: string;
@ -546,6 +554,7 @@ export function registerGroupCommands(program: Command): void {
}>;
truncated?: boolean;
suppressedMatchStages?: string[];
truncationReason?: string;
unreadableRepos?: string[];
missingRepos?: string[];
};
@ -581,7 +590,15 @@ export function registerGroupCommands(program: Command): void {
`\n Re-run \`gitnexus group sync\` without --exact-only for the complete set.`,
);
}
if (truncated) {
// Gated on the REASON, not just the flag. A suppressed stage sets
// `truncated` with both repo lists empty, which sent this block down
// its else-branch and printed "the last sync did not record which
// repos it could read" — a false statement, with the wrong remedy,
// about a sync that recorded them fine. The suppressed-stage warning
// above already said the true thing. When a repo gap co-occurs the
// reason is 'incomplete-sync' (the repo side takes precedence in
// `crossRepoCompleteness`), so this block still runs for it.
if (truncated && truncationReason !== 'suppressed-stage') {
// Counts above are a floor, not a census. Name the repos when the
// registry recorded them, and say so plainly when it did not — a
// listing that cannot say what it is missing is still incomplete.

View file

@ -12,7 +12,7 @@ import type {
MatchType,
} from './types.js';
import { BRIDGE_SCHEMA_QUERIES, BRIDGE_SCHEMA_VERSION } from './bridge-schema.js';
import { recordedRepoList } from './completeness.js';
import { recordedMatchStages, recordedRepoList } from './completeness.js';
import {
closeLbugConnection,
openLbugConnection,
@ -833,14 +833,8 @@ export async function readBridgeMeta(groupDir: string): Promise<BridgeMeta> {
// was there and could not be read.
if (unreadableRepos) meta.unreadableRepos = unreadableRepos;
else delete meta.unreadableRepos;
// Same absent-vs-empty rule, and all-or-nothing: an unreadable list is "not
// recorded", never "measured, nothing suppressed".
const known = ['exact', 'manifest', 'wildcard'];
const suppressed = Array.isArray(raw.suppressedMatchStages)
? raw.suppressedMatchStages.every((v) => typeof v === 'string' && known.includes(v))
? (raw.suppressedMatchStages as MatchType[])
: undefined
: undefined;
// Same absent-vs-empty rule, through the one shared reader.
const suppressed = recordedMatchStages(raw.suppressedMatchStages);
if (suppressed) meta.suppressedMatchStages = suppressed;
else delete meta.suppressedMatchStages;
if (repoListsUnreadable) meta.repoListsUnreadable = true;
@ -919,11 +913,13 @@ async function fileExists(filePath: string): Promise<boolean> {
*/
export async function refreshPreservedBridgeMeta(
groupDir: string,
diagnostics: {
missingRepos: string[];
unreadableRepos: string[];
suppressedMatchStages?: MatchType[];
},
// Deliberately NOT `suppressedMatchStages`. This path preserves an EARLIER
// sync's database, so stamping it with this run's request would claim the
// untouched bridge was built with a flag it never saw. The registry's own
// preserve write (`{ ...prior, missingRepos, unreadableRepos }`) omits it for
// exactly this reason, and the two artifacts have to agree about which run
// they describe.
diagnostics: { missingRepos: string[]; unreadableRepos: string[] },
): Promise<PreservedBridgeMetaOutcome> {
const dbPath = path.join(groupDir, 'bridge.lbug');
const [metaOnDisk, dbOnDisk] = await Promise.all([

View file

@ -13,7 +13,7 @@
* Nothing here imports anything but types. Keep it that way: the moment this
* file gains a runtime import, every consumer pays for it again.
*/
import type { GroupImpactTruncationReason } from './types.js';
import type { GroupImpactTruncationReason, MatchType } from './types.js';
/**
* A union rather than `Pick<GroupImpactResult, ...>` so the two states are
@ -113,6 +113,23 @@ export type CrossRepoCompleteness = TruncationFields & {
* marker that says "this is a floor, not a verdict" may never drift away from
* the flag that says the answer was cut short (#2787).
*/
/**
* Read a persisted `suppressedMatchStages` list.
*
* Sibling of `recordedRepoList` and here for the same stated reason: it had
* lived in two files verbatim, so tightening one would silently leave the other.
* All-or-nothing like its sibling a stale member (this repo has already
* retired `'bm25'` and `'embedding'`) makes the whole list unreadable rather
* than filtering down to `[]`, which on this field would mean "measured,
* nothing suppressed": a clean answer manufactured from a value we could not
* read.
*/
export function recordedMatchStages(value: unknown): MatchType[] | undefined {
if (!Array.isArray(value)) return undefined;
const known: MatchType[] = ['exact', 'manifest', 'wildcard'];
return value.every((v): v is MatchType => known.includes(v as MatchType)) ? value : undefined;
}
export function crossRepoCompleteness(input: CrossRepoCompletenessInput): CrossRepoCompleteness {
const incompleteRepos = [
...new Set([...(input.unreadableRepos ?? []), ...(input.missingRepos ?? [])]),

View file

@ -879,15 +879,21 @@ export async function runGroupImpact(
// and under-reporting a blast radius is the unsafe direction (an agent told
// LOW proceeds; told CRITICAL it stops). Marking the floor keeps the
// warning intact while making the incompleteness legible.
// Runtime limits first — they are what the caller can retry. 'incomplete-sync'
// is the remaining cause once nothing was merely cut short, and its remedy is
// a different one: re-run `gitnexus group sync`, not the query. Computed
// inline because `truncationFields` reads the reason ONLY on the truncated
// branch — naming it in a variable invited reading it on the complete path,
// where it would say 'incomplete-sync' about a complete result.
// Runtime limits first — they are what the caller can retry. Past those, the
// BRIDGE's own reason wins: it already distinguished an unreadable repo
// ('incomplete-sync', remedy: re-sync) from a stage the sync was asked to
// skip ('suppressed-stage', remedy: re-sync WITHOUT the flag). Hardcoding
// the fallback here overrode that and told every caller to repair a repo
// that read fine — and made the second value unreachable from this surface
// while the tool description promised it. `cross-trace.ts` re-spreads the
// bridge's fields for the same reason.
...truncationFields(
truncated,
fanoutTimedOut ? 'timeout' : runtimeTruncated ? 'partial' : 'incomplete-sync',
fanoutTimedOut
? 'timeout'
: runtimeTruncated
? 'partial'
: ((bridge.truncated ? bridge.truncationReason : undefined) ?? 'incomplete-sync'),
),
truncatedRepos: [...new Set([...truncatedRepos, ...bridge.incompleteRepos])],
summary: {

View file

@ -15,7 +15,7 @@ import {
type RepoMeta,
} from '../../storage/repo-manager.js';
import { crossRepoCompleteness } from './completeness.js';
import { recordedRepoList } from './completeness.js';
import { recordedMatchStages, recordedRepoList } from './completeness.js';
import { GroupNotFoundError, loadGroupConfig } from './config-parser.js';
import {
fileMatchesServicePrefix,
@ -33,7 +33,6 @@ import type {
CrossLink,
GroupConfig,
GroupContextResult,
MatchType,
StoredContract,
} from './types.js';
@ -361,26 +360,6 @@ async function loadContractRegistryResilient(
return { ok: true, registry, skippedCorrupt };
}
/**
* Read a persisted `suppressedMatchStages` list.
*
* Deliberately NOT `recordedRepoList`: that validates `string[]`, which is
* right for repo names and one notch too weak here. This repo has already
* retired MatchType members ('bm25', 'embedding'), so a stale value on disk is
* a real shape dropping non-members keeps an unknown stage name from
* reaching a caller typed as a live one. Absence stays absence (tri-state).
*/
function recordedMatchStages(value: unknown): MatchType[] | undefined {
if (!Array.isArray(value)) return undefined;
const known: MatchType[] = ['exact', 'manifest', 'wildcard'];
// All-or-nothing, exactly like `recordedRepoList`. Filtering would be worse
// than useless here: a stale `['bm25']` would survive as `[]`, which on this
// field MEANS "measured, nothing was suppressed" — a confident clean answer
// manufactured from a value we could not read, on the one field that exists
// to stop that conflation. An unreadable list is "not recorded".
return value.every((v): v is MatchType => known.includes(v as MatchType)) ? value : undefined;
}
/**
* Validate a boolean MCP parameter reject, never coerce.
*

View file

@ -762,11 +762,7 @@ export async function syncGroup(config: GroupConfig, opts?: SyncOptions): Promis
// confident wrong answer, and swallowing its failure would reinstate the
// very fail-open it closes. `writeContractRegistry` above is unguarded for
// the same reason, into the same directory.
await refreshPreservedBridgeMeta(groupDir, {
missingRepos,
unreadableRepos,
suppressedMatchStages,
});
await refreshPreservedBridgeMeta(groupDir, { missingRepos, unreadableRepos });
}
if (!everyRepoFailed) {

View file

@ -112,7 +112,11 @@ export function getResourceTemplates(): ResourceTemplate[] {
'three-state, and an ABSENT key is not an empty one: absent means the last sync never ' +
'recorded which repos it could read (provenance unknown — treat cross-repo answers for ' +
'this group as a floor), an empty list means the sync measured none, and a populated list ' +
'names the repos whose contracts are missing from the registry.',
'names the repos whose contracts are missing from the registry. suppressedMatchStages is ' +
'three-state the same way: absent is a registry predating the field, an empty list means ' +
'the sync skipped no matching stage, and a populated list names stages it was ASKED to ' +
'skip — those cross-link counts are a lower bound by request, and the remedy is to re-sync ' +
'without that flag rather than to repair a repo.',
mimeType: 'text/yaml',
},
];

View file

@ -854,7 +854,7 @@ WHEN TO USE: Discover groups before group_sync. Optional "name" returns a single
WHEN TO USE: After changing group.yaml or re-indexing member repos.
READ THE RESULT: \`missingRepos\` are configured repos with no entry in the registry (index them, or drop them from group.yaml); \`unreadableRepos\` ARE registered but this sync could not extract from them — the index would not open (version skew, lock, corruption), or an extractor failed partway — so NONE of their contracts are in this sync and a following group_impact / group_contracts is a lower bound, not a verdict. \`registryOutcome\` says what happened to the file, and the three values a call here can return each need a different response: 'written' — this run's contracts replaced contracts.json; 'preserved' — nothing could be read, so contracts.json was rewritten keeping the previous sync's contracts and cross-links verbatim and refreshing only \`missingRepos\`/\`unreadableRepos\` to describe THIS run (the file changed, the contracts in it did not, and they are as old as the last sync that succeeded); 'superseded' — nothing could be read, and another sync replaced contracts.json while this one waited for the group lock; that file was left untouched and this run's lists were NOT recorded, because they describe an older group state than what is on disk (so the registry is fresher than this response's diagnostics, not staler); 'no-prior-registry' — nothing could be read AND there was no previous contracts.json to carry forward, so none was written and this group has no contract registry on disk. Only 'no-prior-registry' means there is nothing to read: after it, group_contracts / group_impact have no registry at all rather than a stale one, so fix the repos above and re-run before trusting either.\n\nPARAMETERS ARE VALIDATED: \`exactOnly\` must be a real boolean — the string "false" is rejected, not coerced to true. The retired \`skipEmbeddings\` and \`allowStale\` parameters are refused by name; drop them from the call.`,
READ THE RESULT: \`missingRepos\` are configured repos with no entry in the registry (index them, or drop them from group.yaml); \`unreadableRepos\` ARE registered but this sync could not extract from them — the index would not open (version skew, lock, corruption), or an extractor failed partway — so NONE of their contracts are in this sync and a following group_impact / group_contracts is a lower bound, not a verdict. \`registryOutcome\` says what happened to the file, and the three values a call here can return each need a different response: 'written' — this run's contracts replaced contracts.json; 'preserved' — nothing could be read, so contracts.json was rewritten keeping the previous sync's contracts and cross-links verbatim and refreshing only \`missingRepos\`/\`unreadableRepos\` to describe THIS run (the file changed, the contracts in it did not, and they are as old as the last sync that succeeded); 'superseded' — nothing could be read, and another sync replaced contracts.json while this one waited for the group lock; that file was left untouched and this run's lists were NOT recorded, because they describe an older group state than what is on disk (so the registry is fresher than this response's diagnostics, not staler); 'no-prior-registry' — nothing could be read AND there was no previous contracts.json to carry forward, so none was written and this group has no contract registry on disk. Only 'no-prior-registry' means there is nothing to read: after it, group_contracts / group_impact have no registry at all rather than a stale one, so fix the repos above and re-run before trusting either. \`suppressedMatchStages\` names matching stages this sync was ASKED to skip, with the same three states as the repo lists: ABSENT means a registry written before the field existed, \`[]\` means this sync suppressed nothing, and a populated list means the cross-link set is a lower bound BY REQUEST — a later group_impact / group_contracts on it reports truncationReason 'suppressed-stage'.\n\nPARAMETERS ARE VALIDATED: \`exactOnly\` must be a real boolean — the string "false" is rejected, not coerced to true. The retired \`skipEmbeddings\` and \`allowStale\` parameters are refused by name; drop them from the call.`,
// Usually writes contracts.json, so conservatively non-idempotent even
// though output is deterministic for identical input. When no configured
// repo could be read it still rewrites the file, keeping the previous

View file

@ -24,6 +24,7 @@ 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 { GROUP_IMPACT_TRUNCATION_REASONS } from '../../../src/core/group/types.js';
import type {
GroupConfig,
StoredContract,
@ -159,3 +160,32 @@ describe('cross-repo completeness reflects a suppressed stage', () => {
expect(out.truncationReason).toBe('incomplete-sync');
});
});
/**
* The reason has to be REACHABLE, not just documented.
*
* A guard in `tools.test.ts` asserts every truncation reason is explained in
* the impact tool description. That check passed while `runGroupImpact`
* hardcoded its fallback and could never emit `'suppressed-stage'` a
* documented value no surface could produce. These pin the emitting side.
*/
describe('the suppressed-stage reason is reachable, not just documented', () => {
it('is the reason when a stage was suppressed and every repo read fine', () => {
const out = crossRepoCompleteness({
unreadableRepos: [],
missingRepos: [],
suppressedMatchStages: ['wildcard'],
provenanceUnknown: false,
inScope: () => true,
});
expect(out.truncated).toBe(true);
expect(out.truncationReason).toBe('suppressed-stage');
});
it('is a declared member of the reason union agents branch on', () => {
// If a future change drops it from the union, the tool description guard
// would still pass while every consumer lost the value.
expect(GROUP_IMPACT_TRUNCATION_REASONS).toContain('suppressed-stage');
});
});

View file

@ -329,6 +329,55 @@ describe('syncGroup with an unreadable index', () => {
expect(onDisk.unreadableRepos).toEqual(['app/backend']);
});
it('keeps the prior suppressedMatchStages instead of stamping this run request', async () => {
// The preserved registry describes an EARLIER sync. If this run's request
// were stamped onto it, a graph narrowed by `--exact-only` would be
// relabelled complete the moment a later plain sync failed to read
// anything — and `group_impact` reads exactly that field to decide whether
// its answer is a floor.
initLbugMock.mockRejectedValue(new Error(LBUG_VERSION_ERROR));
const contractsPath = path.join(groupDir, 'contracts.json');
fs.writeFileSync(
contractsPath,
JSON.stringify({ ...PRIOR_REGISTRY, suppressedMatchStages: ['wildcard'] }),
);
// This run asks for NO suppression, and fails to read anything.
const result = await syncGroup(makeConfig({ 'app/backend': 'backend-repo' }), { groupDir });
expect(result.registryOutcome).toBe('preserved');
// The result describes THIS run — it really did suppress nothing.
expect(result.suppressedMatchStages).toEqual([]);
const onDisk = JSON.parse(fs.readFileSync(contractsPath, 'utf8')) as Record<string, unknown>;
// The file still describes the sync that produced its contracts.
expect(onDisk.suppressedMatchStages).toEqual(['wildcard']);
});
it('does not stamp this run request onto the preserved bridge metadata', async () => {
// Same property one artifact over. `bridge.lbug` is untouched on this path,
// so its meta.json must keep describing the sync that built it; otherwise
// contracts.json, meta.json and the database describe three different runs.
initLbugMock.mockRejectedValue(new Error(LBUG_VERSION_ERROR));
fs.writeFileSync(path.join(groupDir, 'contracts.json'), JSON.stringify(PRIOR_REGISTRY));
await writeBridgeMeta(groupDir, {
version: 1,
generatedAt: '2026-01-01T00:00:00.000Z',
missingRepos: [],
unreadableRepos: [],
suppressedMatchStages: ['wildcard'],
});
await syncGroup(makeConfig({ 'app/backend': 'backend-repo' }), { groupDir });
const meta = await readBridgeMeta(groupDir);
expect(meta.suppressedMatchStages).toEqual(['wildcard']);
// ...while the diagnostics describing THIS run are refreshed, as before.
expect(meta.unreadableRepos).toEqual(['app/backend']);
});
it('writes nothing at all when there is no previous registry to preserve', async () => {
initLbugMock.mockRejectedValue(new Error(LBUG_VERSION_ERROR));