mirror of
https://github.com/abhigyanpatwari/GitNexus.git
synced 2026-08-28 05:25:25 +00:00
fix(group): stop cross-repo impact and trace claiming a narrowed graph is complete
Closes the half of the suppressed-stage finding that was deferred. The reviewers were right that deferring it was the weak point: the motivating harm was named as `group_impact` and cross-repo `trace` traversing a graph missing real edges, and those were exactly the surfaces left uncovered. The deferral rested on an assumption that does not hold. "It is already blind to this, so we do not make it worse" is false: `--exact-only` was inert before this PR, so the number of narrowed registries in the world goes from zero to nonzero exactly when this lands. The blindness was harmless only while narrowing was impossible. And silence there is not neutral -- `cross-impact.ts` documents `truncated: false` as an affirmative completeness claim, so those tools were about to start asserting a complete answer over a knowingly short graph. `suppressedMatchStages` now rides the bridge the same way `unreadableRepos` does: persisted in meta.json (no BRIDGE_SCHEMA_VERSION bump -- meta fields have this precedent), read back all-or-nothing, and carried across the preserve path through `refreshPreservedBridgeMeta`'s diagnostics so a preserved bridge keeps the marker of the sync that actually built it. `crossRepoCompleteness` folds it in, which is what makes this one change reach all three surfaces -- that function is by design the ONE computation behind the truncation triple. Precedence is explicit: an unreadable or unaccounted repo outranks a suppressed stage, because it is the more serious structural gap and its remedy has to be the one reported. `'suppressed-stage'` is a new member of the truncation-reason union rather than a reuse of `'incomplete-sync'`. The earlier decision not to touch that union was about not conflating remedies -- telling an agent to repair a repo that read fine, for a narrowing it requested. A distinct member preserves that reasoning while letting the answer stop claiming completeness, which is what reusing the existing member would have destroyed. The union's guard test did its job: adding a member failed the check that every reason is explained on the agent-facing surface, so the impact tool description now names this one and its distinct remedy (re-run WITHOUT the flag; nothing failed to read). `group status` and its CLI renderer surface it too, on the populated case only -- absent is a registry predating the field and empty is the ordinary clean sync; neither earns a line. Deliberately still not done, and why: a repo-wide unknown-parameter layer for every MCP tool. Five parameters are read by backends and declared in no schema (`subgroupExact`, `unmatchedOnly`, `showClusters`, `showProcesses`, and `verbose` until this branch declared it), and three tools dispatch with no schema entry at all, so a strict layer rejects working calls until each is reconciled. That reconciliation is the work; the layer is the cheap part. It also cannot produce the "was removed and is no longer accepted" message the retired-parameter guard exists to give. tsc clean; eslint 0 errors (2 pre-existing warnings); 1159/1159 across the group unit, group integration and tool-schema suites.
This commit is contained in:
parent
8f7c25dd3e
commit
6ceac8b1f6
10 changed files with 150 additions and 6 deletions
|
|
@ -135,6 +135,7 @@ export function registerGroupCommands(program: Command): void {
|
|||
>;
|
||||
missingRepos?: string[];
|
||||
unreadableRepos?: string[];
|
||||
suppressedMatchStages?: string[];
|
||||
};
|
||||
|
||||
console.log(' Repo index / contracts staleness:');
|
||||
|
|
@ -189,6 +190,18 @@ export function registerGroupCommands(program: Command): void {
|
|||
if ((st.missingRepos || []).length > 0) {
|
||||
console.log(`\n Last sync missing repos: ${st.missingRepos!.join(', ')}`);
|
||||
}
|
||||
// Only the populated case prints. Absent means a registry that predates
|
||||
// the field, and empty is the ordinary clean sync — neither is worth a
|
||||
// line, whereas a narrowed registry changes how every later answer
|
||||
// should be read.
|
||||
const skippedStages = st.suppressedMatchStages ?? [];
|
||||
if (skippedStages.length > 0) {
|
||||
console.log(
|
||||
`\n Last sync skipped matching stages: ${skippedStages.join(', ')}` +
|
||||
`\n Cross-links those stages would have found are absent by request.` +
|
||||
`\n Re-run \`gitnexus group sync\` without --exact-only for the complete set.`,
|
||||
);
|
||||
}
|
||||
} finally {
|
||||
await backend.dispose().catch(() => {});
|
||||
}
|
||||
|
|
|
|||
|
|
@ -3,7 +3,14 @@ import path from 'node:path';
|
|||
import { createHash } from 'node:crypto';
|
||||
import lbug from '@ladybugdb/core';
|
||||
import type { LbugValue } from '@ladybugdb/core';
|
||||
import type { BridgeHandle, BridgeMeta, StoredContract, CrossLink, RepoSnapshot } from './types.js';
|
||||
import type {
|
||||
BridgeHandle,
|
||||
BridgeMeta,
|
||||
StoredContract,
|
||||
CrossLink,
|
||||
RepoSnapshot,
|
||||
MatchType,
|
||||
} from './types.js';
|
||||
import { BRIDGE_SCHEMA_QUERIES, BRIDGE_SCHEMA_VERSION } from './bridge-schema.js';
|
||||
import { recordedRepoList } from './completeness.js';
|
||||
import {
|
||||
|
|
@ -826,6 +833,16 @@ 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;
|
||||
if (suppressed) meta.suppressedMatchStages = suppressed;
|
||||
else delete meta.suppressedMatchStages;
|
||||
if (repoListsUnreadable) meta.repoListsUnreadable = true;
|
||||
return meta;
|
||||
}
|
||||
|
|
@ -902,7 +919,11 @@ async function fileExists(filePath: string): Promise<boolean> {
|
|||
*/
|
||||
export async function refreshPreservedBridgeMeta(
|
||||
groupDir: string,
|
||||
diagnostics: { missingRepos: string[]; unreadableRepos: string[] },
|
||||
diagnostics: {
|
||||
missingRepos: string[];
|
||||
unreadableRepos: string[];
|
||||
suppressedMatchStages?: MatchType[];
|
||||
},
|
||||
): Promise<PreservedBridgeMetaOutcome> {
|
||||
const dbPath = path.join(groupDir, 'bridge.lbug');
|
||||
const [metaOnDisk, dbOnDisk] = await Promise.all([
|
||||
|
|
@ -969,6 +990,13 @@ export interface WriteBridgeInput {
|
|||
* contract those repos own.
|
||||
*/
|
||||
unreadableRepos?: string[];
|
||||
/**
|
||||
* Matching stages the sync was asked to skip. Recorded here for the same
|
||||
* reason `unreadableRepos` is: a later cross-repo query reads this bridge
|
||||
* with no access to the run that built it, and a graph narrowed by request
|
||||
* looks exactly like a complete one.
|
||||
*/
|
||||
suppressedMatchStages?: MatchType[];
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
@ -1322,6 +1350,9 @@ export async function writeBridgeUnlocked(
|
|||
// different claim from a bridge that never recorded the field. Omitted
|
||||
// only when the caller passed nothing to record.
|
||||
...(input.unreadableRepos ? { unreadableRepos: input.unreadableRepos } : {}),
|
||||
...(input.suppressedMatchStages
|
||||
? { suppressedMatchStages: input.suppressedMatchStages }
|
||||
: {}),
|
||||
});
|
||||
|
||||
return report;
|
||||
|
|
|
|||
|
|
@ -69,6 +69,12 @@ export interface CrossRepoCompletenessInput {
|
|||
*/
|
||||
unreadableRepos?: readonly string[];
|
||||
missingRepos?: readonly string[];
|
||||
/**
|
||||
* Matching stages the sync was asked to skip. Absent or empty means it
|
||||
* suppressed none; a populated list makes the answer a floor for a reason
|
||||
* that is neither a runtime limit nor an unreadable repo.
|
||||
*/
|
||||
suppressedMatchStages?: readonly string[];
|
||||
/** Computed by the caller; see `bridgeProvenanceUnknown` for the bridge one. */
|
||||
provenanceUnknown: boolean;
|
||||
/**
|
||||
|
|
@ -111,8 +117,17 @@ export function crossRepoCompleteness(input: CrossRepoCompletenessInput): CrossR
|
|||
const incompleteRepos = [
|
||||
...new Set([...(input.unreadableRepos ?? []), ...(input.missingRepos ?? [])]),
|
||||
].filter((repoPath) => input.inScope(repoPath));
|
||||
// An unreadable or unaccounted repo outranks a suppressed stage: it is the
|
||||
// more serious structural gap and its remedy (repair the repo, re-sync) has
|
||||
// to be the one reported. A suppressed stage only decides the reason when
|
||||
// the repo side is otherwise clean.
|
||||
const suppressed = (input.suppressedMatchStages ?? []).length > 0;
|
||||
const repoSideIncomplete = input.provenanceUnknown || incompleteRepos.length > 0;
|
||||
return {
|
||||
...truncationFields(input.provenanceUnknown || incompleteRepos.length > 0, 'incomplete-sync'),
|
||||
...truncationFields(
|
||||
repoSideIncomplete || suppressed,
|
||||
repoSideIncomplete ? 'incomplete-sync' : 'suppressed-stage',
|
||||
),
|
||||
incompleteRepos,
|
||||
};
|
||||
}
|
||||
|
|
|
|||
|
|
@ -856,6 +856,7 @@ export async function runGroupImpact(
|
|||
const bridge = crossRepoCompleteness({
|
||||
unreadableRepos: bridgePrep.meta.unreadableRepos,
|
||||
missingRepos: bridgePrep.meta.missingRepos,
|
||||
suppressedMatchStages: bridgePrep.meta.suppressedMatchStages,
|
||||
provenanceUnknown,
|
||||
inScope: (candidate) =>
|
||||
repoInSubgroup(candidate, subgroup) || repoInSubgroup(candidate, repoPath, true),
|
||||
|
|
|
|||
|
|
@ -264,6 +264,7 @@ function bridgeCompletenessFor(
|
|||
return crossRepoCompleteness({
|
||||
unreadableRepos: meta.unreadableRepos,
|
||||
missingRepos: meta.missingRepos,
|
||||
suppressedMatchStages: meta.suppressedMatchStages,
|
||||
provenanceUnknown: bridgeProvenanceUnknown(meta),
|
||||
inScope,
|
||||
});
|
||||
|
|
|
|||
|
|
@ -544,6 +544,7 @@ export class GroupService {
|
|||
const { incompleteRepos: _incompleteRepos, ...truncation } = crossRepoCompleteness({
|
||||
unreadableRepos,
|
||||
missingRepos,
|
||||
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,
|
||||
|
|
@ -888,6 +889,10 @@ export class GroupService {
|
|||
// "none" (see ContractRegistry), and a value we could not read is equally
|
||||
// unrecorded. Reporting either as an empty list is the same conflation.
|
||||
unreadableRepos: recordedRepoList(registry?.unreadableRepos),
|
||||
// Same tri-state, same reason: `group status` is where an operator goes
|
||||
// to ask "is this group's answer trustworthy right now", and a registry
|
||||
// narrowed on purpose is a different answer from a complete one.
|
||||
suppressedMatchStages: recordedMatchStages(registry?.suppressedMatchStages),
|
||||
repos: repoStatuses,
|
||||
};
|
||||
}
|
||||
|
|
|
|||
|
|
@ -762,7 +762,11 @@ 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 });
|
||||
await refreshPreservedBridgeMeta(groupDir, {
|
||||
missingRepos,
|
||||
unreadableRepos,
|
||||
suppressedMatchStages,
|
||||
});
|
||||
}
|
||||
|
||||
if (!everyRepoFailed) {
|
||||
|
|
@ -792,6 +796,7 @@ export async function syncGroup(config: GroupConfig, opts?: SyncOptions): Promis
|
|||
repoSnapshots,
|
||||
missingRepos,
|
||||
unreadableRepos,
|
||||
suppressedMatchStages,
|
||||
});
|
||||
} catch (err) {
|
||||
const msg = err instanceof Error ? err.message : String(err);
|
||||
|
|
|
|||
|
|
@ -146,6 +146,13 @@ export interface RepoHandle {
|
|||
* a retry. `'incomplete-sync'` is structural: the bridge itself was built from a
|
||||
* sync that could not read every configured repo, so those repos' contracts are
|
||||
* absent from every query against it until `gitnexus group sync` succeeds.
|
||||
* `'suppressed-stage'` is structural too but has its own remedy: the sync was
|
||||
* ASKED to skip a matching stage (`--exact-only`), so cross-links that stage
|
||||
* would have found are absent by request. Retrying returns the same floor, and
|
||||
* so does re-running the sync — the fix is to re-run it WITHOUT the flag. Kept a
|
||||
* separate member rather than folded into `'incomplete-sync'` precisely because
|
||||
* that remedy differs; telling an agent to repair a repo it read fine is the
|
||||
* failure this distinction exists to prevent.
|
||||
*
|
||||
* A runtime array rather than a bare type union: every value here has to be
|
||||
* explained on the agent-facing surface that returns it, and only an enumerable
|
||||
|
|
@ -154,7 +161,12 @@ export interface RepoHandle {
|
|||
* catch, so the list an agent is promised and the list the code can emit have
|
||||
* to come from the same place.
|
||||
*/
|
||||
export const GROUP_IMPACT_TRUNCATION_REASONS = ['timeout', 'partial', 'incomplete-sync'] as const;
|
||||
export const GROUP_IMPACT_TRUNCATION_REASONS = [
|
||||
'timeout',
|
||||
'partial',
|
||||
'incomplete-sync',
|
||||
'suppressed-stage',
|
||||
] as const;
|
||||
|
||||
export type GroupImpactTruncationReason = (typeof GROUP_IMPACT_TRUNCATION_REASONS)[number];
|
||||
|
||||
|
|
@ -366,4 +378,12 @@ export interface BridgeMeta {
|
|||
* Optional: a bridge written before this field existed does not record it.
|
||||
*/
|
||||
unreadableRepos?: string[];
|
||||
/**
|
||||
* Matching stages the sync that built this bridge was asked to skip.
|
||||
* PERSISTED, like `unreadableRepos` and unlike `repoListsUnreadable` — a
|
||||
* later `group_impact` or `trace` reads this bridge with no access to the run
|
||||
* that produced it, and a narrowed graph is otherwise indistinguishable from
|
||||
* a complete one. Same tri-state: absent is "not recorded".
|
||||
*/
|
||||
suppressedMatchStages?: MatchType[];
|
||||
}
|
||||
|
|
|
|||
|
|
@ -505,7 +505,7 @@ Handles disambiguation: when multiple symbols share the target name, returns ran
|
|||
EdgeType: CALLS, IMPORTS, EXTENDS, IMPLEMENTS, HAS_METHOD, HAS_PROPERTY, METHOD_OVERRIDES, METHOD_IMPLEMENTS, ACCESSES
|
||||
Confidence: 1.0 = certain, <0.8 = fuzzy match
|
||||
|
||||
GROUP MODE: set "repo" to "@<groupName>" for cross-repo impact anchored at the default member (lexicographically first key in group.yaml "repos"), or "@<groupName>/<groupRepoPath>" to choose the member (same path keys as in group.yaml). Phase-1 walk runs in that member; cross-boundary fan-out uses the group bridge. A cross entry with fanout_status:"not_attempted" proves the declared repository boundary, but its far endpoint has no graph symbol; do not interpret empty by_depth or affected_processes on that entry as a completed zero-impact walk. The fan-out attempts at most 50 neighbour crossings, strongest-confidence first. Any short answer carries truncated:true, truncatedRepos, riskEpistemic:"lower-bound" AND a truncationReason — dropping a crossing can only move risk DOWN, so treat that risk as a floor, never as a verdict. truncated:true does NOT always mean the fan-out ran out of room, so branch on truncationReason: the remedy differs. 'timeout' (the fan-out's wall-clock budget expired) and 'partial' (a neighbour crossing, or the local walk, was cut short) are runtime limits — the same query can return more on a retry or with a larger timeoutMs. 'incomplete-sync' is structural: the group bridge was built by a sync that could not say which repos it read, or that could not read an in-scope repo, so those repos' contracts are absent from EVERY query against this bridge, and truncatedRepos names them even when ZERO crossings to them were attempted. Retrying returns the same floor — run group_sync (\`gitnexus group sync\`) and query again.
|
||||
GROUP MODE: set "repo" to "@<groupName>" for cross-repo impact anchored at the default member (lexicographically first key in group.yaml "repos"), or "@<groupName>/<groupRepoPath>" to choose the member (same path keys as in group.yaml). Phase-1 walk runs in that member; cross-boundary fan-out uses the group bridge. A cross entry with fanout_status:"not_attempted" proves the declared repository boundary, but its far endpoint has no graph symbol; do not interpret empty by_depth or affected_processes on that entry as a completed zero-impact walk. The fan-out attempts at most 50 neighbour crossings, strongest-confidence first. Any short answer carries truncated:true, truncatedRepos, riskEpistemic:"lower-bound" AND a truncationReason — dropping a crossing can only move risk DOWN, so treat that risk as a floor, never as a verdict. truncated:true does NOT always mean the fan-out ran out of room, so branch on truncationReason: the remedy differs. 'timeout' (the fan-out's wall-clock budget expired) and 'partial' (a neighbour crossing, or the local walk, was cut short) are runtime limits — the same query can return more on a retry or with a larger timeoutMs. 'incomplete-sync' is structural: the group bridge was built by a sync that could not say which repos it read, or that could not read an in-scope repo, so those repos' contracts are absent from EVERY query against this bridge, and truncatedRepos names them even when ZERO crossings to them were attempted. Retrying returns the same floor — run group_sync (\`gitnexus group sync\`) and query again. 'suppressed-stage' is also structural but has a DIFFERENT remedy: the sync was asked to skip a matching stage (\`--exact-only\` / exactOnly), so cross-links that stage would have found are absent BY REQUEST. Re-running the sync unchanged returns the same floor — re-run it WITHOUT that flag. Do not report a repo as broken for this reason; nothing failed to read.
|
||||
|
||||
SERVICE: optional monorepo path prefix (case-sensitive path segments). When "repo" starts with "@", scopes the local impact walk and cross-repo symbol paths to files under that prefix; ignored for a normal indexed repo name.`,
|
||||
annotations: READ_ONLY_TOOL_ANNOTATIONS,
|
||||
|
|
|
|||
|
|
@ -23,6 +23,7 @@ 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 type {
|
||||
GroupConfig,
|
||||
StoredContract,
|
||||
|
|
@ -106,3 +107,55 @@ describe('a sync records the matching stages it was told to skip', () => {
|
|||
expect(registry).toHaveProperty('suppressedMatchStages');
|
||||
});
|
||||
});
|
||||
|
||||
/**
|
||||
* The half that matters to a later reader: does a narrowed graph still claim
|
||||
* to be complete? `crossRepoCompleteness` is the ONE computation behind the
|
||||
* truncation triple that `group_impact`, cross-repo `trace` and the contract
|
||||
* listing all return, so pinning it here covers all three.
|
||||
*/
|
||||
describe('cross-repo completeness reflects a suppressed stage', () => {
|
||||
it('reports a floor, with its own reason, when a stage was suppressed', () => {
|
||||
const out = crossRepoCompleteness({
|
||||
unreadableRepos: [],
|
||||
missingRepos: [],
|
||||
suppressedMatchStages: ['wildcard'],
|
||||
provenanceUnknown: false,
|
||||
inScope: () => true,
|
||||
});
|
||||
|
||||
expect(out.truncated).toBe(true);
|
||||
expect(out.truncationReason).toBe('suppressed-stage');
|
||||
expect(out.riskEpistemic).toBe('lower-bound');
|
||||
});
|
||||
|
||||
// control: without a suppressed stage the same clean input is complete.
|
||||
// Without this, hardcoding `truncated: true` would pass the case above.
|
||||
it('control: a clean sync with nothing suppressed is not truncated', () => {
|
||||
const out = crossRepoCompleteness({
|
||||
unreadableRepos: [],
|
||||
missingRepos: [],
|
||||
suppressedMatchStages: [],
|
||||
provenanceUnknown: false,
|
||||
inScope: () => true,
|
||||
});
|
||||
|
||||
expect(out.truncated).toBe(false);
|
||||
expect(out.truncationReason).toBeUndefined();
|
||||
});
|
||||
|
||||
// An unreadable repo is the more serious gap and its remedy differs, so it
|
||||
// has to win the reason slot rather than being masked by the flag.
|
||||
it('lets an unreadable repo outrank a suppressed stage in the reason', () => {
|
||||
const out = crossRepoCompleteness({
|
||||
unreadableRepos: ['app/backend'],
|
||||
missingRepos: [],
|
||||
suppressedMatchStages: ['wildcard'],
|
||||
provenanceUnknown: false,
|
||||
inScope: () => true,
|
||||
});
|
||||
|
||||
expect(out.truncated).toBe(true);
|
||||
expect(out.truncationReason).toBe('incomplete-sync');
|
||||
});
|
||||
});
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue