mirror of
https://github.com/abhigyanpatwari/GitNexus.git
synced 2026-11-12 08:41:34 +00:00
refactor: apply the reuse, simplification, efficiency and altitude cleanups (#2915)
The one that mattered: the degradation exit code was fixed at the wrong depth. `output()` has never inspected `partial` — it tests `error` only — so putting the check in `detectChangesCommand` left every other tool exiting 0 on a degraded run. `partial` is cross-tool vocabulary: query (enrichmentDegraded || ftsPartial), impact (!traversalComplete, perSymbolEnrichmentCapped) and the mode:'pdg' envelope all emit it. A truncated impact traversal returns a short caller set and an under-ranked risk, then exits 0 — so `gitnexus impact … && <edit>` proceeds, in the tool AGENTS.md makes a MUST gate before every edit. The justification also cited checkCommand as precedent, but checkCommand passes STRINGS too — it was the second command already hand-rolling around this gap, while output()'s docstring called itself "the one place that keeps scripted callers honest". output() now takes an optional renderer and fails on error OR partial; two hand-rolled sites go away and three tools are covered instead of one. truncated stays exit 0 (only the listing is capped) and checkCommand's cycleCount policy stays put. Efficiency, all re-measured on the 25k-node index: - The process lookup was chunked with LBUG_QUERY_BATCH_SIZE, calibrated for the opposite query shape — that constant is for a whole-node-table scan where more items amortise the scan, while this is an `id IN $ids` probe where round trips dominate. 20k ids: 617ms at 100, 261ms at 1000. New LBUG_ID_PROBE_BATCH_SIZE, documented against its sibling so they cannot be re-merged. This also settles the older "chunking this query is a regression" measurement — that was chunk=100. - The sort comparator re-coerced fields ChangedSymbolRow already types, O(n log n) redundant conversions (+31-38%). Row shape probed directly: alias-keyed, no positional keys, numeric columns are JS numbers. - exactlyMatchedPaths built two throwaway arrays; one loop instead (40k rows 11.4ms -> 4.5ms). - The integration fixture seeded 710 step edges one round trip at a time; one UNWIND instead. File wall time 6.91s -> 3.63s. Fixture size unchanged — its docstring records the threshold below which the bug stops reproducing, and the mutation check still fails 3/3 when ORDER BY step is reverted. Reuse and simplification: - CALL_EDGE_LIMIT existed in four places; its own docstring predicted the drift it then caused. prompts.ts owns it now — it is a zero-import leaf so the direction cannot cycle, and had graph-queries.ts owned it the four suites that vi.mock that module would have left slice(0, undefined), silently returning every edge in exactly the tests meant to police the cap. - Six dead positional row fallbacks survived the rewrite in the loop this branch re-indented, in the same PR that deleted the identical ABI from graph-queries.ts. - Two test files independently modelled the same labels() scalar-string quirk. Deleted the wiki one — the file's own new header says semantics belong in the real-engine test — and kept projectTypeColumn, the only instrument that can see the bug for the detect_changes query. - makeRepo onto the shared git bootstrap (the eleventh copy of the sequence the helper was extracted to own), the duplicate diff-args unwrapper merged into test/helpers, hand-rolled comparators onto compareCodeUnits, real-timer sleeps replaced by wave-released promises with a strengthened per-wave assertion. - Re-exports trimmed to what is actually imported, a cross-reference this branch invalidated by moving mapConcurrent, and a "~20% faster" claim that does not survive at real index sizes (1-9%; the 10x memory win does). Also adds the drift guard the new doc text lacked: fragment coverage for the partial/truncated paragraph in every skill copy and in the managed AGENTS.md / CLAUDE.md block. Falsifiability checked — none of those fragments exist at the merge base. Not done here, deliberately: 27 live labels(x)[0] projections remain across impact/context/query/trace and MCP resources, with four load-bearing workarounds that have begun depending on each other and one that fabricates rather than degrades. That is a semantic change to five agent-facing tools and wants its own PR, scoped to delete the workarounds too. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0139ycbqnAorkJGQaZQXUNuv
This commit is contained in:
Gergo Magyar
parent
d338eadd8d
commit
fc70427d60
18 changed files with 486 additions and 210 deletions
|
|
@ -198,18 +198,17 @@ ${tableBody}`
|
|||
`No \`${runnerPath}\` yet? Bootstrap with \`npx\`, \`bunx\`, or \`pnpm dlx\` — ` +
|
||||
'e.g. `bunx gitnexus@latest analyze` (npm 11 npx crash; #1939).';
|
||||
|
||||
// This block is injected into every user's repo and its total size is capped
|
||||
// by test (ai-context.test.ts, #856) — a new bullet or clause has to be paid
|
||||
// for by trimming an existing one.
|
||||
//
|
||||
// The detect_changes bullet carries the degraded-result rule (#2915): a run
|
||||
// that sets `partial` (a graph query failed) or `truncated` (the changed-symbol
|
||||
// listing was capped) is not the pre-commit gate passing, and `partial` pairs
|
||||
// routinely with changed_count:0 — the exact shape that printed "No changes
|
||||
// detected." and exited 0 on a broken analysis. Same reasoning as the
|
||||
// `risk: UNKNOWN` bullet below: the tool could not answer, so its zero is not
|
||||
// an all-clear. This block is injected into every user's repo and its size is
|
||||
// capped by test (ai-context.test.ts, #856), so the rule was paid for in the
|
||||
// same currency it costs: the header's "use GitNexus graph tools…" exhortation
|
||||
// (which the Always Do list restates as MUSTs, with commands) and the verbatim
|
||||
// repeat of `<runner> detect-changes` in the regression-compare example, whose
|
||||
// only delta from the command one sentence earlier is the flags.
|
||||
// an all-clear.
|
||||
return `${GITNEXUS_START_MARKER}
|
||||
# GitNexus — Code Intelligence
|
||||
|
||||
|
|
|
|||
|
|
@ -49,8 +49,7 @@ export function formatDetectChangesResult(result: unknown): string {
|
|||
if (payload.truncated) notes.push(t('tool.detectChanges.truncated'));
|
||||
|
||||
if ((summary.changed_count ?? 0) === 0) {
|
||||
const noChanges = t('tool.detectChanges.noChanges');
|
||||
return notes.length > 0 ? [...notes, noChanges].join('\n') : noChanges;
|
||||
return [...notes, t('tool.detectChanges.noChanges')].join('\n');
|
||||
}
|
||||
|
||||
const lines: string[] = [];
|
||||
|
|
|
|||
|
|
@ -42,9 +42,18 @@ async function getBackend(): Promise<LocalBackend> {
|
|||
* and write directly to the real stdout fd (#324).
|
||||
*
|
||||
* Falls back to stderr if the fd write fails (e.g., broken pipe).
|
||||
*
|
||||
* `render` is for the commands that print prose instead of JSON: they hand over
|
||||
* the STRUCTURED result and a formatter, so the payload stays visible to the
|
||||
* exit-code test below — pre-formatting it into a string would hide the very
|
||||
* fields that test reads.
|
||||
*/
|
||||
function output(data: any): void {
|
||||
const text = typeof data === 'string' ? data : JSON.stringify(data, null, 2);
|
||||
function output<T>(data: T, render?: (data: T) => string): void {
|
||||
const text = render
|
||||
? render(data)
|
||||
: typeof data === 'string'
|
||||
? data
|
||||
: JSON.stringify(data, null, 2);
|
||||
try {
|
||||
writeSync(1, text + '\n');
|
||||
} catch (err: any) {
|
||||
|
|
@ -56,18 +65,34 @@ function output(data: any): void {
|
|||
// Fallback: stderr (previous behavior, works on all platforms)
|
||||
process.stderr.write(text + '\n');
|
||||
}
|
||||
// Backend failures come back as `{ error }` payloads rather than throws
|
||||
// (#2469). Every tool command routes its result through here, so this is
|
||||
// the one place that keeps scripted callers honest: print the payload,
|
||||
// then exit non-zero.
|
||||
if (
|
||||
data &&
|
||||
typeof data === 'object' &&
|
||||
'error' in data &&
|
||||
typeof data.error === 'string' &&
|
||||
data.error.trim().length > 0
|
||||
) {
|
||||
process.exitCode = 1;
|
||||
// Every tool command routes its result through here, so this is the one place
|
||||
// that keeps scripted callers honest — `gitnexus impact … && <edit>` and
|
||||
// `gitnexus detect-changes && git commit` must not proceed on a result that
|
||||
// did not complete. Two shapes say so, and both exit non-zero:
|
||||
//
|
||||
// • `error` — a backend failure, returned as a payload rather than thrown
|
||||
// (#2469).
|
||||
// • `partial` — a step failed and was SWALLOWED (#2915), so the counts and
|
||||
// risk level are lower bounds a caller would otherwise read as clean. It
|
||||
// is cross-tool vocabulary, not detect_changes' private flag: `query`
|
||||
// raises it for degraded enrichment or a partial FTS failure, and `impact`
|
||||
// for an interrupted traversal or capped per-symbol enrichment — a short
|
||||
// caller set and an under-ranked risk, on the tool AGENTS.md makes a MUST
|
||||
// gate before every edit.
|
||||
//
|
||||
// One code for both, because `&&` cannot tell two apart and a "softer" code
|
||||
// for `partial` would invite exempting it again.
|
||||
//
|
||||
// NOT here: `truncated`, where only the LISTING is capped while the counts and
|
||||
// risk are computed over the full set — the verdict is sound, so failing on it
|
||||
// would fire on every large-but-healthy diff. Nor `partialProbe`, a narrower
|
||||
// per-candidate flag on ambiguous impact targets.
|
||||
if (data && typeof data === 'object') {
|
||||
const payload = data as { error?: unknown; partial?: unknown };
|
||||
const failed =
|
||||
(typeof payload.error === 'string' && payload.error.trim().length > 0) ||
|
||||
payload.partial === true;
|
||||
if (failed) process.exitCode = 1;
|
||||
}
|
||||
}
|
||||
|
||||
|
|
@ -337,18 +362,9 @@ export async function detectChangesCommand(options?: {
|
|||
if (Array.isArray(result.affected_processes))
|
||||
result.affected_processes = result.affected_processes.slice(0, limit);
|
||||
}
|
||||
output(formatDetectChangesResult(result));
|
||||
// `output()` only fails the exit code for an OBJECT payload carrying `error`
|
||||
// (#2469), and this command hands it a formatted STRING — so the status has to
|
||||
// be taken off the structured result here, the same object-first way
|
||||
// checkCommand does below. A hard failure (`error`, which the formatter also
|
||||
// renders as text) and a swallowed one (`partial`) mean the same thing to
|
||||
// `gitnexus detect-changes && git commit`: the gate did not complete, so the
|
||||
// commit must not proceed — one non-zero code, because `&&` cannot tell two
|
||||
// apart and a "softer" code for `partial` would invite exempting it again.
|
||||
// `truncated` is deliberately NOT a failure: only the LISTING was capped, and
|
||||
// the counts and risk level are still computed over every changed symbol.
|
||||
if (result?.error || result?.partial) process.exitCode = 1;
|
||||
// Hand over the structured result plus its formatter, not the formatted text:
|
||||
// `output()` reads `error` / `partial` off the payload to set the exit code.
|
||||
output(result, formatDetectChangesResult);
|
||||
}
|
||||
|
||||
export async function checkCommand(options?: {
|
||||
|
|
@ -370,9 +386,11 @@ export async function checkCommand(options?: {
|
|||
repo: options.repo,
|
||||
branch: options.branch,
|
||||
});
|
||||
// A rendering guard, not an exit-code decision — `output()` owns that. An
|
||||
// error payload carries no `cycles` array, so the prose branch below would
|
||||
// throw on it; print the structured payload and stop.
|
||||
if (result?.error) {
|
||||
output(result);
|
||||
process.exitCode = 1;
|
||||
return;
|
||||
}
|
||||
if (options.json) {
|
||||
|
|
@ -384,6 +402,8 @@ export async function checkCommand(options?: {
|
|||
result.cycles.map((cycle: { files: string[] }) => cycle.files.join(' -> ')).join('\n'),
|
||||
);
|
||||
}
|
||||
// Policy, not degradation: a clean run that FOUND cycles is `check` failing
|
||||
// its own check. `output()` deliberately knows nothing about it.
|
||||
if (result.cycleCount > 0) process.exitCode = 1;
|
||||
} catch (error) {
|
||||
output({ error: error instanceof Error ? error.message : String(error) });
|
||||
|
|
|
|||
|
|
@ -20,9 +20,12 @@ import { springDiResolver } from './spring.js';
|
|||
|
||||
/** The resolver contract lives in the leaf `./types.js` so an implementation
|
||||
* can depend on it without depending on this registry (which imports every
|
||||
* implementation). Re-exported here so consumers can keep importing the
|
||||
* contract and the registry from one place. */
|
||||
export type { DiInjectionMatch, DiProviderMatch, DiResolver } from './types.js';
|
||||
* implementation). The two match shapes are re-exported here because consumers
|
||||
* of the registry read them off its results — `pipeline-phases/di.ts` and the
|
||||
* Spring metadata modules import them from this module alongside
|
||||
* `DI_RESOLVERS`. `DiResolver` itself is NOT re-exported: only implementations
|
||||
* need it, and they import it from `./types.js` directly. */
|
||||
export type { DiInjectionMatch, DiProviderMatch } from './types.js';
|
||||
|
||||
/** All `SupportedLanguages` string values, for narrowing raw graph strings. */
|
||||
const SUPPORTED_LANGUAGE_VALUES: ReadonlySet<string> = new Set(Object.values(SupportedLanguages));
|
||||
|
|
|
|||
|
|
@ -19,15 +19,57 @@
|
|||
*/
|
||||
|
||||
/**
|
||||
* Items per graph query when the input list is caller-sized.
|
||||
* Items per graph query when each item makes the query do MORE WORK — an
|
||||
* `UNWIND` row, a per-item predicate, anything the engine pays for per item.
|
||||
*
|
||||
* Measured on a 25k-node index: for the `detect_changes` hunk→symbol query, 25
|
||||
* items cost 1025ms, 50 cost 642ms, **100 cost 476ms**, 200 cost 613ms and 800
|
||||
* cost 650ms — round-trip overhead dominates below 100, per-query cost above
|
||||
* it. The impact path's id lookups landed on the same number independently.
|
||||
* it. The impact path's id lookups landed on the same number independently,
|
||||
* and its list is bounded by `processLimit * maxSymbolsPerProcess` anyway, so
|
||||
* it rarely fills even one chunk.
|
||||
*
|
||||
* NOT the size for an id-list probe — see `LBUG_ID_PROBE_BATCH_SIZE`, which
|
||||
* measures an order of magnitude larger for the opposite reason. The two are
|
||||
* deliberately separate constants.
|
||||
*/
|
||||
export const LBUG_QUERY_BATCH_SIZE = 100;
|
||||
|
||||
/**
|
||||
* Items per graph query when the list is only a MEMBERSHIP TEST the engine
|
||||
* probes with — `WHERE n.id IN $ids` and nothing else scaling with it.
|
||||
*
|
||||
* Ten times `LBUG_QUERY_BATCH_SIZE` because the two shapes are opposites:
|
||||
*
|
||||
* - The hunk→symbol query above is an unlabeled `MATCH (n)`, a scan of the
|
||||
* whole node table that every batch pays in full. More items per batch means
|
||||
* fewer scans to amortise, so the cost curve turns UP past ~100.
|
||||
* - An `id IN $ids` probe does work proportional to the ids and nothing else.
|
||||
* There is no fixed cost to amortise, so a small batch is pure round-trip
|
||||
* overhead and the curve only turns up once a batch is big enough to
|
||||
* materialise a large list.
|
||||
*
|
||||
* Measured on this repo's index (25k nodes), `detect_changes`' symbol→process
|
||||
* lookup at concurrency 4, median of 3:
|
||||
*
|
||||
* | ids | chunk=100 | chunk=1000 | one query |
|
||||
* |--------|-----------|------------|-----------|
|
||||
* | 5,000 | 161ms | 88ms | 124ms |
|
||||
* | 20,000 | 617ms | 266ms | 336ms |
|
||||
*
|
||||
* At 20,000 ids the curve is already flat at 1,000 (250:374ms, 500:300ms,
|
||||
* 1000:261ms, 2000:248ms, 4000:269ms), so a larger batch buys ≤5% and gives
|
||||
* back the ceiling that is the whole point of chunking: the unchunked form
|
||||
* measured 1,238 MB at 100k ids and 4,002 MB at 500k (#2915). 1,000 is the
|
||||
* first size on the flat part.
|
||||
*
|
||||
* This also settles an earlier measurement that read chunking this query as a
|
||||
* regression (4,150 ids 129→152ms, 9,749 ids 238→325ms): that was chunk=100,
|
||||
* which is slower than not chunking at all above ~2,000 ids. At 1,000 the
|
||||
* chunked form beats both.
|
||||
*/
|
||||
export const LBUG_ID_PROBE_BATCH_SIZE = 1000;
|
||||
|
||||
/**
|
||||
* Query text above which a caller is assumed to be splicing a caller-sized list
|
||||
* into the query rather than binding it.
|
||||
|
|
|
|||
|
|
@ -14,15 +14,16 @@ import {
|
|||
pinRepo,
|
||||
} from '../lbug/pool-adapter.js';
|
||||
|
||||
const REPO_ID = '__wiki__';
|
||||
|
||||
/**
|
||||
* Rows kept by each call-edge query. One constant rather than one per query:
|
||||
* every such list reaches the LLM through `formatCallEdges` (prompts.ts), whose
|
||||
* `.slice(0, 30)` is the only reason a limit exists at all, so a second number
|
||||
* could only ever drift from it.
|
||||
* Rows kept by each call-edge query. Owned by prompts.ts, where the reason for
|
||||
* a limit lives: every one of these lists reaches the LLM through
|
||||
* `formatCallEdges`, which slices to the same value. Fetching rows that slice
|
||||
* would discard is waste, so the cut happens here too — but as the same number,
|
||||
* not a second one, because a second one could only ever drift from it.
|
||||
*/
|
||||
const CALL_EDGE_LIMIT = 30;
|
||||
import { CALL_EDGE_LIMIT } from './prompts.js';
|
||||
|
||||
const REPO_ID = '__wiki__';
|
||||
|
||||
/**
|
||||
* Touch the wiki DB connection to prevent idle timeout during long LLM calls.
|
||||
|
|
@ -148,9 +149,13 @@ async function withSteps(headers: ProcessHeader[]): Promise<ProcessInfo[]> {
|
|||
const stepsById = new Map<string, ProcessInfo['steps']>();
|
||||
for (const row of stepRows) {
|
||||
const pid = String(row.pid);
|
||||
const steps = stepsById.get(pid) ?? [];
|
||||
|
||||
let steps = stepsById.get(pid);
|
||||
if (!steps) {
|
||||
steps = [];
|
||||
stepsById.set(pid, steps);
|
||||
}
|
||||
steps.push(toProcessStep(row));
|
||||
stepsById.set(pid, steps);
|
||||
}
|
||||
|
||||
return headers.map((header) => ({ ...header, steps: stepsById.get(header.id) ?? [] }));
|
||||
|
|
|
|||
|
|
@ -170,6 +170,26 @@ export function formatDirectoryTree(filePaths: string[]): string {
|
|||
);
|
||||
}
|
||||
|
||||
/**
|
||||
* Call edges kept on a page. Declared here because this is where the
|
||||
* requirement is: a limit exists at all only because `formatCallEdges` renders
|
||||
* these lists into a prompt, and every such list reaches the LLM through it.
|
||||
*
|
||||
* The call-edge queries in `graph-queries.ts` import this value for their
|
||||
* Cypher `LIMIT`s rather than restate it. That is the same cut moved earlier —
|
||||
* fetching rows the slice below would discard is pure waste, and at module
|
||||
* scale it is most of the query (#2787) — so fetch and display must agree, and
|
||||
* a second number could only ever drift from this one.
|
||||
*
|
||||
* The import runs that way and not the other because this module has no
|
||||
* imports of its own. `graph-queries.ts` may read this; sending it back the
|
||||
* other way would give a pure template module a transitive dependency on the
|
||||
* LadybugDB pool adapter, and would make the cap vanish under the tests that
|
||||
* `vi.mock` `graph-queries.js` — a mock factory omitting the constant leaves
|
||||
* `slice(0, undefined)`, which keeps every edge.
|
||||
*/
|
||||
export const CALL_EDGE_LIMIT = 30;
|
||||
|
||||
/**
|
||||
* Format call edges as readable text.
|
||||
*/
|
||||
|
|
@ -178,7 +198,7 @@ export function formatCallEdges(
|
|||
): string {
|
||||
if (edges.length === 0) return 'None';
|
||||
return edges
|
||||
.slice(0, 30)
|
||||
.slice(0, CALL_EDGE_LIMIT)
|
||||
.map((e) => `${e.fromName} (${shortPath(e.fromFile)}) → ${e.toName} (${shortPath(e.toFile)})`)
|
||||
.join('\n');
|
||||
}
|
||||
|
|
|
|||
|
|
@ -126,9 +126,12 @@ export const stripWindowsLongPathPrefix = (
|
|||
* Returns an empty array for empty input, and never returns an empty slice, so
|
||||
* `for (const batch of chunk(xs, n))` always has something to work on.
|
||||
*
|
||||
* Callers batching a GRAPH QUERY should take the size from
|
||||
* `LBUG_QUERY_BATCH_SIZE` (`core/lbug/query-batch.ts`), which documents why a
|
||||
* query built from a caller-sized array needs a ceiling at all (#2915).
|
||||
* Callers batching a GRAPH QUERY should take the size from `core/lbug/query-batch.ts`,
|
||||
* which documents why a query built from a caller-sized array needs a ceiling at
|
||||
* all (#2915) and which of the two ceilings applies: `LBUG_QUERY_BATCH_SIZE` when
|
||||
* each item makes the query do more work, `LBUG_ID_PROBE_BATCH_SIZE` when it is a
|
||||
* plain `id IN $ids` probe and round trips dominate. They differ by 10x, in
|
||||
* opposite directions, for that reason.
|
||||
*/
|
||||
export function chunk<T>(items: readonly T[], size: number): T[][] {
|
||||
// `NaN` fails every comparison, so a bare `size < 1` lets it through and
|
||||
|
|
|
|||
|
|
@ -22,7 +22,7 @@ import { queryClassBeanMetadata } from './bean-metadata.js';
|
|||
import { querySpringAopMetadata } from './aop-metadata.js';
|
||||
import { isValidQueryParams } from '../../core/lbug/query-params.js';
|
||||
import { toDisplayLine } from './line-display.js';
|
||||
import { LBUG_QUERY_BATCH_SIZE } from '../../core/lbug/query-batch.js';
|
||||
import { LBUG_ID_PROBE_BATCH_SIZE, LBUG_QUERY_BATCH_SIZE } from '../../core/lbug/query-batch.js';
|
||||
import { chunk, mapConcurrent } from '../../lib/utils.js';
|
||||
import { pathSuffixOf } from './path-predicate.js';
|
||||
import { toOneBasedLine } from '../../core/ingestion/utils/line-base.js';
|
||||
|
|
@ -930,6 +930,23 @@ interface ChangedSymbolRow {
|
|||
endLine: number;
|
||||
}
|
||||
|
||||
/**
|
||||
* One row of the `detect_changes` symbol→process query (see `detectChanges`).
|
||||
*
|
||||
* Keyed by the query's `AS` aliases, like `ChangedSymbolRow` above and the wiki
|
||||
* row types (`core/wiki/graph-queries.ts`): the pool adapter returns
|
||||
* `getAll()`'s `Record<string, LbugValue>`, so a row has alias keys and never
|
||||
* the positional ones an older adapter offered.
|
||||
*/
|
||||
interface ProcessRow {
|
||||
nodeId: string;
|
||||
pid: string;
|
||||
label: string;
|
||||
processType: string;
|
||||
stepCount: number;
|
||||
step: number;
|
||||
}
|
||||
|
||||
export function buildDetectChangesDiffArgs(scope: string, baseRef?: string): string[] | null {
|
||||
// The prefix flags pin the `a/` + `b/` forms `parseDiffHunks` matches on.
|
||||
// Without them git honours the user's config: `diff.noprefix` emits
|
||||
|
|
@ -5262,8 +5279,13 @@ export class LocalBackend {
|
|||
// one batch at 1M nodes, +922 MB for the four in flight, paid even for a
|
||||
// one-file diff — enough to fail with `Buffer manager exception` on a
|
||||
// 268 MB pool). Stated batch-wide and `b`-free it plans as the first filter
|
||||
// under the scan instead: measured 10x less memory, ~20% faster, identical
|
||||
// rows. Safe because it is a provable superset of the correlated form —
|
||||
// under the scan instead: measured 10x less memory, identical rows. Both
|
||||
// that figure and the "~20% faster" this comment used to also claim come
|
||||
// from the 1M-node synthetic index where the blowup shows; the speed half
|
||||
// does not survive at real sizes — on this repo's 25k-node index the same
|
||||
// change measured 93ms against 85-92ms, inside the noise. Memory is the
|
||||
// reason to keep it. Safe because it is a provable superset of the
|
||||
// correlated form —
|
||||
// `n.filePath = b.path` implies `n.filePath IN $paths`, and
|
||||
// `n.filePath ENDS WITH b.suffix` implies some `$suffixes` entry matches —
|
||||
// so it cannot drop a row the correlated filter keeps.
|
||||
|
|
@ -5284,9 +5306,9 @@ export class LocalBackend {
|
|||
|
||||
// Batches run concurrently: each `executeParameterized` holds one connection
|
||||
// checked out of the per-repo pool for the duration of its query, which is
|
||||
// the safety rule documented alongside `mapConcurrent` (see the batching
|
||||
// rationale in core/lbug/query-batch.ts) — not the single-query sequential
|
||||
// rule the arm64 macOS module loop below follows.
|
||||
// the safety rule documented on `mapConcurrent` itself (`lib/utils.ts`; why
|
||||
// the list needs a ceiling at all is in core/lbug/query-batch.ts) — not the
|
||||
// single-query sequential rule the arm64 macOS module loop below follows.
|
||||
const batchResults = await mapConcurrent(
|
||||
chunk(bounds, LBUG_QUERY_BATCH_SIZE),
|
||||
(batch) =>
|
||||
|
|
@ -5313,12 +5335,17 @@ export class LocalBackend {
|
|||
// Same class as #2787, which this PR also fixes in graph-queries.ts. Sorted
|
||||
// here rather than in Cypher because the rows are already materialised;
|
||||
// (filePath, startLine, id) is a total key, `id` being unique per node.
|
||||
// Compared as the row type declares them (the engine returns STRING and
|
||||
// INT64 columns as JS strings and numbers), not re-coerced per comparison:
|
||||
// `String()`/`Number()` inside a comparator run O(n log n) times, measured
|
||||
// 31-38% of the sort (500k rows 786ms vs 571ms). Every other read of these
|
||||
// rows below trusts the same declaration.
|
||||
const symbolRows = batchResults.flatMap((rows) => (rows ?? []) as ChangedSymbolRow[]);
|
||||
symbolRows.sort(
|
||||
(a, b) =>
|
||||
compareCodeUnits(String(a.filePath), String(b.filePath)) ||
|
||||
Number(a.startLine) - Number(b.startLine) ||
|
||||
compareCodeUnits(String(a.id), String(b.id)),
|
||||
compareCodeUnits(a.filePath, b.filePath) ||
|
||||
a.startLine - b.startLine ||
|
||||
compareCodeUnits(a.id, b.id),
|
||||
);
|
||||
|
||||
// Prefer the exact path. A detect_changes path is ALWAYS repo-root-relative
|
||||
|
|
@ -5330,12 +5357,17 @@ export class LocalBackend {
|
|||
// So it degrades to a fallback: a path that produced an exact row keeps only
|
||||
// its exact rows, a path that produced none still widens. Decided on the
|
||||
// rows already fetched, so the scan above is still paid exactly once.
|
||||
const exactlyMatchedPaths = new Set(
|
||||
symbolRows.filter((row) => row.filePath === row.diffPath).map((row) => String(row.diffPath)),
|
||||
);
|
||||
//
|
||||
// Built in one pass: the `filter().map()` this replaces allocated two
|
||||
// throwaway arrays the size of the row set (40k rows 11.4ms → 4.5ms, 200k
|
||||
// rows 71.3ms → 26.6ms).
|
||||
const exactlyMatchedPaths = new Set<string>();
|
||||
for (const row of symbolRows) {
|
||||
if (row.filePath === row.diffPath) exactlyMatchedPaths.add(row.diffPath);
|
||||
}
|
||||
|
||||
for (const sym of symbolRows) {
|
||||
const diffPath = String(sym.diffPath);
|
||||
const diffPath = sym.diffPath;
|
||||
if (sym.filePath !== sym.diffPath && exactlyMatchedPaths.has(diffPath)) continue;
|
||||
const hunks = hunksByPath.get(diffPath) ?? [];
|
||||
if (!hunksOverlapRange(hunks, sym.startLine, sym.endLine)) continue;
|
||||
|
|
@ -5364,8 +5396,12 @@ export class LocalBackend {
|
|||
// diff measured 1,238 MB at 100k ids and 4,002 MB at 500k (#2915). The
|
||||
// merge below is a Map upsert keyed by process id, so a process reached
|
||||
// from two chunks simply accumulates its steps.
|
||||
// `LBUG_ID_PROBE_BATCH_SIZE`, not the hunk query's size: this is a pure
|
||||
// `id IN $ids` probe with no scan to amortise, so it wants a batch an
|
||||
// order of magnitude larger — 20k ids measured 617ms at 100 against 266ms
|
||||
// at 1,000. The contrast is documented on both constants.
|
||||
const processBatches = await mapConcurrent(
|
||||
chunk(Array.from(changedSymbols.keys()), LBUG_QUERY_BATCH_SIZE),
|
||||
chunk(Array.from(changedSymbols.keys()), LBUG_ID_PROBE_BATCH_SIZE),
|
||||
(ids) => executeParameterized(repo.lbugPath, processQuery, { ids }),
|
||||
{ onError: (error) => logQueryError('detect-changes:process-lookup', error) },
|
||||
);
|
||||
|
|
@ -5373,22 +5409,27 @@ export class LocalBackend {
|
|||
// from the result, so it is `partial` — not the clean "nothing to worry
|
||||
// about" it would otherwise look like.
|
||||
if (processBatches.includes(undefined)) queryDegraded = true;
|
||||
// Read by alias only. The rows are `getAll()` records (`pool-adapter.ts`),
|
||||
// so the `proc.label || proc[2]` positional fallbacks this loop used to
|
||||
// carry could never fire — and where a column IS legitimately falsy they
|
||||
// turned it into `undefined`: an empty heuristicLabel or a step numbered
|
||||
// 0 lost its own value. Same reason `graph-queries.ts` moved these
|
||||
// defaults from `||` to `??`; here there is nothing left to default to.
|
||||
for (const procs of processBatches) {
|
||||
for (const proc of procs ?? []) {
|
||||
const nodeId = proc.nodeId || proc[0];
|
||||
const pid = proc.pid || proc[1];
|
||||
for (const proc of (procs ?? []) as ProcessRow[]) {
|
||||
const pid = proc.pid;
|
||||
if (!affectedProcesses.has(pid)) {
|
||||
affectedProcesses.set(pid, {
|
||||
id: pid,
|
||||
name: proc.label || proc[2],
|
||||
process_type: proc.processType || proc[3],
|
||||
step_count: proc.stepCount || proc[4],
|
||||
name: proc.label,
|
||||
process_type: proc.processType,
|
||||
step_count: proc.stepCount,
|
||||
changed_steps: [],
|
||||
});
|
||||
}
|
||||
affectedProcesses.get(pid)!.changed_steps.push({
|
||||
symbol: changedSymbols.get(nodeId)?.name ?? nodeId,
|
||||
step: proc.step || proc[5],
|
||||
symbol: changedSymbols.get(proc.nodeId)?.name ?? proc.nodeId,
|
||||
step: proc.step,
|
||||
});
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -18,15 +18,12 @@ type BoundedCheckpointBeforeExit =
|
|||
|
||||
// The message shapes live in `analyze-worker-protocol.ts` — a declarations-only
|
||||
// leaf neither this entry module nor `analyze-worker-core.ts` sits downstream
|
||||
// of, which is what breaks the entry ⇄ core import cycle. Re-exported here (as
|
||||
// types, so the re-export is erased at runtime) to keep every existing import
|
||||
// site — `analyze-launch.ts` and the unit tests — working unchanged.
|
||||
export type {
|
||||
ProgressMessage,
|
||||
CompleteMessage,
|
||||
ErrorMessage,
|
||||
WorkerMessage,
|
||||
} from './analyze-worker-protocol.js';
|
||||
// of, which is what breaks the entry ⇄ core import cycle. The two shapes that
|
||||
// are imported from HERE are re-exported (as types, so the re-export is erased
|
||||
// at runtime): `WorkerMessage` by `analyze-launch.ts`, `CompleteMessage` by
|
||||
// `analyze-launch-collapse.test.ts`. Everything else imports the protocol module
|
||||
// directly, so nothing else belongs in this list.
|
||||
export type { CompleteMessage, WorkerMessage } from './analyze-worker-protocol.js';
|
||||
|
||||
function send(msg: WorkerMessage) {
|
||||
// No try/catch: if the IPC channel is gone, process.send throws
|
||||
|
|
|
|||
22
gitnexus/test/helpers/detect-changes-diff-args.ts
Normal file
22
gitnexus/test/helpers/detect-changes-diff-args.ts
Normal file
|
|
@ -0,0 +1,22 @@
|
|||
/**
|
||||
* The git arguments `detect_changes` itself runs, for tests that shell out to
|
||||
* the same diff the tool would.
|
||||
*
|
||||
* `buildDetectChangesDiffArgs` returns `null` for the one case no test here
|
||||
* drives — `compare` with no base ref — and a `null` reaching `execFileSync`
|
||||
* fails as a bare `TypeError` several frames from the test that caused it.
|
||||
* Both consumers (`detect-changes-eol`, `detect-changes-hunk-scale`) had
|
||||
* written the same three-line unwrap; this one names the scope in the message.
|
||||
*
|
||||
* The null-returning behaviour itself is asserted directly, on the real
|
||||
* function, in `test/unit/detect-changes-eol.test.ts`.
|
||||
*/
|
||||
|
||||
import { buildDetectChangesDiffArgs } from '../../src/mcp/local/local-backend.js';
|
||||
|
||||
/** `buildDetectChangesDiffArgs`, refusing the null instead of passing it on. */
|
||||
export function diffArgsFor(scope: string, baseRef?: string): string[] {
|
||||
const args = buildDetectChangesDiffArgs(scope, baseRef);
|
||||
if (!args) throw new Error(`scope "${scope}" must produce git diff arguments`);
|
||||
return args;
|
||||
}
|
||||
|
|
@ -33,6 +33,8 @@ import {
|
|||
getProcessesForFiles,
|
||||
initWikiDb,
|
||||
} from '../../src/core/wiki/graph-queries.js';
|
||||
import { CALL_EDGE_LIMIT } from '../../src/core/wiki/prompts.js';
|
||||
import { compareCodeUnits } from '../../src/lib/utils.js';
|
||||
|
||||
// ─── Fixture ──────────────────────────────────────────────────────────────
|
||||
|
||||
|
|
@ -48,10 +50,11 @@ const MODULE_FILES = [ALPHA, BETA];
|
|||
const pad = (n: number): string => String(n).padStart(2, '0');
|
||||
|
||||
/**
|
||||
* `CALL_EDGE_LIMIT` is 30 and private to graph-queries.ts. 40 bulk callers plus
|
||||
* the two hand-written intra-module edges put 42 rows in front of it, so the
|
||||
* LIMIT has to cut — and `bulkNN` sorts after both hand-written names, which
|
||||
* makes the kept set an exactly predictable ordered prefix.
|
||||
* 40 bulk callers plus the two hand-written intra-module edges put 42 rows in
|
||||
* front of `CALL_EDGE_LIMIT` (imported above from prompts.ts, the one place
|
||||
* that number lives), so the LIMIT has to cut — and `bulkNN` sorts after both
|
||||
* hand-written names, which makes the kept set an exactly predictable ordered
|
||||
* prefix.
|
||||
*/
|
||||
const BULK_CALLERS = Array.from({ length: 40 }, (_, i) => `bulk${pad(i)}`);
|
||||
|
||||
|
|
@ -117,13 +120,36 @@ const fn = (
|
|||
): string => `{id: 'Function:${file}:${name}', name: '${name}', filePath: '${file}',
|
||||
startLine: ${line}, endLine: ${line}, isExported: ${isExported}, content: '', description: ''}`;
|
||||
|
||||
const rel = (type: string, step = 0): string =>
|
||||
/** `step` is interpolated raw, so a caller may pass a Cypher expression. */
|
||||
const rel = (type: string, step: number | string = 0): string =>
|
||||
`[:CodeRelation {type: '${type}', confidence: 1.0, reason: 'seed', step: ${step}}]`;
|
||||
|
||||
const stepEdge = (proc: string, symbol: string, step: number): string =>
|
||||
`MATCH (s:Function), (p:Process)
|
||||
WHERE s.id = 'Function:${symbolFile.get(symbol)}:${symbol}' AND p.id = '${proc}'
|
||||
CREATE (s)-${rel('STEP_IN_PROCESS', step)}->(p)`;
|
||||
/** Every STEP_IN_PROCESS edge this fixture needs, seeded together below. */
|
||||
const ALL_STEP_EDGES = [
|
||||
...STEP_EDGES,
|
||||
...GAMMA_STEP_SYMBOLS.map((s, i) => ({ proc: 'proc-gamma', symbol: s.name, step: i + 1 })),
|
||||
{ proc: 'proc-blank', symbol: STEP_SYMBOLS[0].name, step: 1 },
|
||||
];
|
||||
|
||||
/**
|
||||
* All 714 step edges in ONE statement.
|
||||
*
|
||||
* `withTestLbugDB` runs each seed string as its own query, so one
|
||||
* `MATCH … CREATE` per edge is 714 round trips — measured at 6.91s wall for
|
||||
* this file against 0.19s for its mocked sibling, on a pool whose bare read
|
||||
* round trip is 0.53ms. The edges themselves are unchanged: the list keeps its
|
||||
* seeded order (see STEP_EDGES) and every row still resolves both endpoints by
|
||||
* id, so nothing about what the ordering tests below can observe moves.
|
||||
*/
|
||||
const stepEdges = (edges: typeof ALL_STEP_EDGES): string =>
|
||||
`UNWIND [${edges
|
||||
.map(
|
||||
(e) =>
|
||||
`{sid: 'Function:${symbolFile.get(e.symbol)}:${e.symbol}', pid: '${e.proc}', step: ${e.step}}`,
|
||||
)
|
||||
.join(', ')}] AS e
|
||||
MATCH (s:Function), (p:Process) WHERE s.id = e.sid AND p.id = e.pid
|
||||
CREATE (s)-${rel('STEP_IN_PROCESS', 'e.step')}->(p)`;
|
||||
|
||||
const SEED: string[] = [
|
||||
// Files
|
||||
|
|
@ -201,9 +227,7 @@ const SEED: string[] = [
|
|||
// still fall back to the id and 'unknown'. No steps either.
|
||||
`CREATE (p:Process {id: 'proc-null', label: 'LNull', stepCount: 0, communities: []})`,
|
||||
|
||||
...STEP_EDGES.map((e) => stepEdge(e.proc, e.symbol, e.step)),
|
||||
...GAMMA_STEP_SYMBOLS.map((s, i) => stepEdge('proc-gamma', s.name, i + 1)),
|
||||
stepEdge('proc-blank', STEP_SYMBOLS[0].name, 1),
|
||||
stepEdges(ALL_STEP_EDGES),
|
||||
];
|
||||
|
||||
/** The trace every `proc-NN` must come back with: 1..stepCount, ascending. */
|
||||
|
|
@ -253,7 +277,9 @@ withTestLbugDB(
|
|||
expect(byFile.get(ALPHA)).toEqual([{ name: 'alphaFn', type: 'Function' }]);
|
||||
// UNION arm 1 (Function, Class) and arm 2 (Method, via HAS_METHOD) each
|
||||
// carry a label — the subscript blanked both sites.
|
||||
expect([...(byFile.get(BETA) ?? [])].sort((a, b) => (a.name < b.name ? -1 : 1))).toEqual([
|
||||
expect(
|
||||
[...(byFile.get(BETA) ?? [])].sort((a, b) => compareCodeUnits(a.name, b.name)),
|
||||
).toEqual([
|
||||
{ name: 'BetaService', type: 'Class' },
|
||||
{ name: 'betaFn', type: 'Function' },
|
||||
{ name: 'serve', type: 'Method' },
|
||||
|
|
@ -279,11 +305,11 @@ withTestLbugDB(
|
|||
// 42 edges match; `ORDER BY fromName, toName, fromFile, toFile / LIMIT`
|
||||
// decides which 30 survive. Drop the LIMIT and this is 42 rows; drop the
|
||||
// ORDER BY and the engine picks an arbitrary 30 (#2787).
|
||||
expect(edges).toHaveLength(30);
|
||||
expect(edges).toHaveLength(CALL_EDGE_LIMIT);
|
||||
expect(edges.map((e) => e.fromName)).toEqual([
|
||||
'alphaFn',
|
||||
'betaFn',
|
||||
...BULK_CALLERS.slice(0, 28),
|
||||
...BULK_CALLERS.slice(0, CALL_EDGE_LIMIT - 2),
|
||||
]);
|
||||
expect(edges[0]).toEqual({
|
||||
fromFile: ALPHA,
|
||||
|
|
@ -344,7 +370,10 @@ withTestLbugDB(
|
|||
const [first] = await getAllProcesses();
|
||||
|
||||
expect([...new Set(first.steps.map((s) => s.type))]).toEqual(['Function']);
|
||||
expect([...new Set(first.steps.map((s) => s.filePath))].sort()).toEqual([ALPHA, BETA]);
|
||||
expect([...new Set(first.steps.map((s) => s.filePath))].sort(compareCodeUnits)).toEqual([
|
||||
ALPHA,
|
||||
BETA,
|
||||
]);
|
||||
});
|
||||
|
||||
it('keeps an empty label and type, and falls back only for a null one', async () => {
|
||||
|
|
|
|||
|
|
@ -5,15 +5,9 @@ import path from 'node:path';
|
|||
import { describe, expect, it } from 'vitest';
|
||||
import { buildDetectChangesDiffArgs } from '../../src/mcp/local/local-backend.js';
|
||||
import { parseDiffHunks } from '../../src/storage/git.js';
|
||||
import { diffArgsFor } from '../helpers/detect-changes-diff-args.js';
|
||||
import { commitAll, initGitRepo } from '../helpers/temp-git-repo.js';
|
||||
|
||||
/** The arguments `detect_changes` runs, refusing the null no test here expects. */
|
||||
function diffArgsFor(scope: string, baseRef?: string): string[] {
|
||||
const args = buildDetectChangesDiffArgs(scope, baseRef);
|
||||
if (!args) throw new Error(`scope "${scope}" must produce git diff arguments`);
|
||||
return args;
|
||||
}
|
||||
|
||||
/** The five flags every scope carries, ahead of its own ref/staging arguments. */
|
||||
const GUARD_FLAGS = [
|
||||
'diff',
|
||||
|
|
|
|||
|
|
@ -33,7 +33,7 @@ const { lbugMocks } = vi.hoisted(() => ({
|
|||
}));
|
||||
|
||||
vi.mock('../../src/core/lbug/pool-adapter.js', async (importOriginal) => {
|
||||
const actual = await importOriginal();
|
||||
const actual = await importOriginal<typeof import('../../src/core/lbug/pool-adapter.js')>();
|
||||
return { ...actual, ...lbugMocks };
|
||||
});
|
||||
|
||||
|
|
@ -47,7 +47,7 @@ vi.mock('../../src/storage/repo-manager.js', async (importOriginal) => {
|
|||
};
|
||||
});
|
||||
|
||||
import { buildDetectChangesDiffArgs, LocalBackend } from '../../src/mcp/local/local-backend.js';
|
||||
import { LocalBackend } from '../../src/mcp/local/local-backend.js';
|
||||
import { listRegisteredRepos, type RegistryEntry } from '../../src/storage/repo-manager.js';
|
||||
import {
|
||||
coalesceHunks,
|
||||
|
|
@ -55,7 +55,9 @@ import {
|
|||
hunksOverlapRange,
|
||||
parseDiffHunks,
|
||||
} from '../../src/storage/git.js';
|
||||
import { diffArgsFor } from '../helpers/detect-changes-diff-args.js';
|
||||
import { createTempDirPool } from '../helpers/temp-dir-pool.js';
|
||||
import { commitAll, initGitRepo } from '../helpers/temp-git-repo.js';
|
||||
|
||||
const tempDirs = createTempDirPool('gnx-hunk-scale-');
|
||||
|
||||
|
|
@ -64,9 +66,7 @@ function makeRepo(files: string[], lines: number): string {
|
|||
const repoDir = tempDirs.dir();
|
||||
mkdirSync(path.join(repoDir, '.gitnexus', 'lbug'), { recursive: true });
|
||||
writeFileSync(path.join(repoDir, '.gitnexus', 'meta.json'), '{}');
|
||||
execFileSync('git', ['init', '-q'], { cwd: repoDir });
|
||||
execFileSync('git', ['config', 'user.email', 'test@example.com'], { cwd: repoDir });
|
||||
execFileSync('git', ['config', 'user.name', 'Test'], { cwd: repoDir });
|
||||
initGitRepo(repoDir);
|
||||
for (const file of files) {
|
||||
mkdirSync(path.dirname(path.join(repoDir, file)), { recursive: true });
|
||||
writeFileSync(
|
||||
|
|
@ -74,8 +74,7 @@ function makeRepo(files: string[], lines: number): string {
|
|||
Array.from({ length: lines }, (_, i) => `line ${i + 1}`).join('\n') + '\n',
|
||||
);
|
||||
}
|
||||
execFileSync('git', ['add', '-A'], { cwd: repoDir });
|
||||
execFileSync('git', ['commit', '-q', '-m', 'init'], { cwd: repoDir });
|
||||
commitAll(repoDir, 'init');
|
||||
return repoDir;
|
||||
}
|
||||
|
||||
|
|
@ -102,13 +101,6 @@ function registerRepo(repoDir: string): void {
|
|||
vi.mocked(listRegisteredRepos).mockResolvedValue([entry]);
|
||||
}
|
||||
|
||||
/** The git arguments `detect_changes` itself runs for the scope these drive. */
|
||||
function unstagedDiffArgs(): string[] {
|
||||
const args = buildDetectChangesDiffArgs('unstaged');
|
||||
if (!args) throw new Error('unstaged scope must produce git diff arguments');
|
||||
return args;
|
||||
}
|
||||
|
||||
/** One bounded file in the hunk→symbol query's `$bounds` parameter. */
|
||||
interface QueryBound {
|
||||
path: string;
|
||||
|
|
@ -371,7 +363,7 @@ describe('#2915 detect_changes hunk scaling', () => {
|
|||
|
||||
// Non-vacuous: the diff really does parse to two entries for one path.
|
||||
const parsed = parseDiffHunks(
|
||||
execFileSync('git', unstagedDiffArgs(), { cwd: repoDir, encoding: 'utf-8' }),
|
||||
execFileSync('git', diffArgsFor('unstaged'), { cwd: repoDir, encoding: 'utf-8' }),
|
||||
);
|
||||
expect(parsed.map((fileDiff) => fileDiff.filePath)).toEqual(['code.py', 'code.py']);
|
||||
|
||||
|
|
|
|||
|
|
@ -170,6 +170,46 @@ describe('intended standard-skill improvements stay in every applicable copy', (
|
|||
}
|
||||
});
|
||||
|
||||
// Same shape as the UNKNOWN guard above, for the other half of the verdict:
|
||||
// `detect_changes` can come back SHORT — `partial` when a batched graph query
|
||||
// failed, `truncated` when the changed-symbol listing hit its cap — and both
|
||||
// read as a clean gate if the agent only looks at the count (#2915). The
|
||||
// wording differs per copy (the Cursor mirror compresses it to one blockquote
|
||||
// line), so the fragments here are the parts every copy shares.
|
||||
it('keeps the partial/truncated degradation guidance in every impact-analysis copy', () => {
|
||||
const required = [
|
||||
'`partial: true` (a graph query failed) or `truncated: true` (the changed-symbol',
|
||||
'listing was capped)',
|
||||
'a zero there means unseen, not unaffected.',
|
||||
'tick the pre-commit check.',
|
||||
];
|
||||
const copies = standardSkillCopies('gitnexus-impact-analysis');
|
||||
// Guard the guard: an empty copy list would make the loop below vacuous.
|
||||
expect(copies.length).toBeGreaterThan(1);
|
||||
for (const file of copies) {
|
||||
const content = fs.readFileSync(file, 'utf-8');
|
||||
for (const fragment of required) expect(content).toContain(fragment);
|
||||
}
|
||||
});
|
||||
|
||||
// The refactoring copies carry the same warning for the verification step a
|
||||
// refactor ends on: there, a short list reads as "only the expected files
|
||||
// changed" rather than as a low risk score.
|
||||
it('keeps the partial/truncated degradation guidance in every refactoring copy', () => {
|
||||
const required = [
|
||||
'`partial: true` (a graph query failed) or `truncated: true` (the changed-symbol',
|
||||
'listing was capped)',
|
||||
'is not proof that only the expected files changed.',
|
||||
'treat the refactor as verified.',
|
||||
];
|
||||
const copies = standardSkillCopies('gitnexus-refactoring');
|
||||
expect(copies.length).toBeGreaterThan(1);
|
||||
for (const file of copies) {
|
||||
const content = fs.readFileSync(file, 'utf-8');
|
||||
for (const fragment of required) expect(content).toContain(fragment);
|
||||
}
|
||||
});
|
||||
|
||||
it('documents the current tools, schema, and cross-repo trace in every guide copy', () => {
|
||||
const required = [
|
||||
'`route_map`',
|
||||
|
|
@ -216,41 +256,45 @@ describe('intended standard-skill improvements stay in every applicable copy', (
|
|||
});
|
||||
});
|
||||
|
||||
// The root AGENTS.md / CLAUDE.md machine-managed block (<!-- gitnexus:start -->
|
||||
// ... <!-- gitnexus:end -->) is regenerated by generateGitNexusContent
|
||||
// (src/cli/ai-context.ts) on every `gitnexus analyze`. The `risk: UNKNOWN`
|
||||
// Always-Do bullet and its Never-Do clause were hand-added INSIDE that region
|
||||
// instead of living in the template, so a real analyze run silently deleted
|
||||
// them on regeneration — twice (#2856's 8f8261021, then #2899's 9e602aef0,
|
||||
// which piggybacked an unrelated fetch-parsing fix and also regressed the
|
||||
// index stats 248612/565510/918 -> 42853/135955/758, itself evidence the
|
||||
// block had been rebuilt from a stale local index). ai-context.ts now
|
||||
// generates both lines directly regardless of `hasPdg` (see
|
||||
// ai-context.test.ts's hasPdg-independent UNKNOWN test), so a real analyze
|
||||
// cannot drop them again. This guard is the second line of defense: it reads
|
||||
// the committed docs themselves, so a hand-revert or a stale generator binary
|
||||
// landing the same regression fails here even if the template is fine.
|
||||
/**
|
||||
* The body of the root AGENTS.md / CLAUDE.md machine-managed block
|
||||
* (`<!-- gitnexus:start -->` … `<!-- gitnexus:end -->`), which
|
||||
* generateGitNexusContent (src/cli/ai-context.ts) regenerates on every
|
||||
* `gitnexus analyze`. Shared by the policy guards below.
|
||||
*/
|
||||
function extractManagedBlock(file: string): string {
|
||||
const content = fs.readFileSync(path.join(REPO_ROOT, file), 'utf-8');
|
||||
// Markers must occupy their own line — CLAUDE.md's "GitNexus rules"
|
||||
// section links to AGENTS.md with an inline prose mention of both
|
||||
// marker strings ("See the `<!-- gitnexus:start --> ... `" etc.) that a
|
||||
// bare indexOf would mistake for the real block (mirrors
|
||||
// findSectionMarkerIndex in ai-context.ts, #1041).
|
||||
const match =
|
||||
/(?:^|\n)<!-- gitnexus:start -->\r?\n([\s\S]*?)\n<!-- gitnexus:end -->(?:\r?\n|$)/.exec(
|
||||
content,
|
||||
);
|
||||
expect(match, `${file} must contain an own-line gitnexus:start/end block`).not.toBeNull();
|
||||
return match![1];
|
||||
}
|
||||
|
||||
// The `risk: UNKNOWN` Always-Do bullet and its Never-Do clause were hand-added
|
||||
// INSIDE the machine-managed region instead of living in the template, so a
|
||||
// real analyze run silently deleted them on regeneration — twice (#2856's
|
||||
// 8f8261021, then #2899's 9e602aef0, which piggybacked an unrelated
|
||||
// fetch-parsing fix and also regressed the index stats 248612/565510/918 ->
|
||||
// 42853/135955/758, itself evidence the block had been rebuilt from a stale
|
||||
// local index). ai-context.ts now generates both lines directly regardless of
|
||||
// `hasPdg` (see ai-context.test.ts's hasPdg-independent UNKNOWN test), so a
|
||||
// real analyze cannot drop them again. This guard is the second line of
|
||||
// defense: it reads the committed docs themselves, so a hand-revert or a stale
|
||||
// generator binary landing the same regression fails here even if the template
|
||||
// is fine.
|
||||
describe('root AGENTS.md / CLAUDE.md managed block keeps the risk: UNKNOWN policy (#2899)', () => {
|
||||
const REQUIRED_FRAGMENTS = [
|
||||
'MUST treat `risk: UNKNOWN` as unresolved, not as low.',
|
||||
'never read `UNKNOWN` as an all-clear',
|
||||
];
|
||||
|
||||
function extractManagedBlock(file: string): string {
|
||||
const content = fs.readFileSync(path.join(REPO_ROOT, file), 'utf-8');
|
||||
// Markers must occupy their own line — CLAUDE.md's "GitNexus rules"
|
||||
// section links to AGENTS.md with an inline prose mention of both
|
||||
// marker strings ("See the `<!-- gitnexus:start --> ... `" etc.) that a
|
||||
// bare indexOf would mistake for the real block (mirrors
|
||||
// findSectionMarkerIndex in ai-context.ts, #1041).
|
||||
const match =
|
||||
/(?:^|\n)<!-- gitnexus:start -->\r?\n([\s\S]*?)\n<!-- gitnexus:end -->(?:\r?\n|$)/.exec(
|
||||
content,
|
||||
);
|
||||
expect(match, `${file} must contain an own-line gitnexus:start/end block`).not.toBeNull();
|
||||
return match![1];
|
||||
}
|
||||
|
||||
it.each(['AGENTS.md', 'CLAUDE.md'])('%s managed block documents the policy', (file) => {
|
||||
const block = extractManagedBlock(file);
|
||||
for (const fragment of REQUIRED_FRAGMENTS) expect(block).toContain(fragment);
|
||||
|
|
@ -274,6 +318,25 @@ describe('root AGENTS.md / CLAUDE.md managed block keeps the risk: UNKNOWN polic
|
|||
);
|
||||
});
|
||||
|
||||
// The same second-line-of-defense reading for the OTHER thing the block now
|
||||
// says about the pre-commit gate: a `detect_changes` that came back `partial`
|
||||
// (a batched graph query failed) or `truncated` (the changed-symbol listing hit
|
||||
// its cap) has not cleared anything (#2915). It lives inside the machine-managed
|
||||
// region, so it survives only as long as ai-context.ts keeps generating it —
|
||||
// exactly the shape that was silently deleted twice above. Reading the
|
||||
// committed docs catches a stale generator binary or a hand-revert too.
|
||||
describe('root AGENTS.md / CLAUDE.md managed block keeps the degraded-detect_changes policy (#2915)', () => {
|
||||
const REQUIRED_FRAGMENTS = [
|
||||
'`partial: true` (query failed) or `truncated: true` (listing capped) is not a clean check',
|
||||
'a zero there means unseen, not unaffected; re-run it',
|
||||
];
|
||||
|
||||
it.each(['AGENTS.md', 'CLAUDE.md'])('%s managed block documents the policy', (file) => {
|
||||
const block = extractManagedBlock(file);
|
||||
for (const fragment of REQUIRED_FRAGMENTS) expect(block).toContain(fragment);
|
||||
});
|
||||
});
|
||||
|
||||
describe.each(FAMILY)('shipped copies of %s stay in sync', (name) => {
|
||||
const canonical = snapshotDir(path.join(REPO_ROOT, '.claude', 'skills', name));
|
||||
|
||||
|
|
|
|||
|
|
@ -120,6 +120,28 @@ describe('direct CLI tool commands', () => {
|
|||
expect(process.exitCode).toBe(1);
|
||||
});
|
||||
|
||||
// `partial` is cross-tool vocabulary, not detect_changes' private flag, and the
|
||||
// degraded shape is the dangerous one: it looks like a result. `impact` matters
|
||||
// most — AGENTS.md makes it the gate before every edit, so a truncated traversal
|
||||
// that exits 0 lets `gitnexus impact … && <edit>` proceed on a short caller set.
|
||||
it('fails closed when query degrades to a partial result', async () => {
|
||||
callToolMock.mockResolvedValue({ results: [], partial: true });
|
||||
const { queryCommand } = await import('../../src/cli/tool.js');
|
||||
|
||||
await queryCommand('auth flow');
|
||||
|
||||
expect(process.exitCode).toBe(1);
|
||||
});
|
||||
|
||||
it('fails closed when impact truncates its traversal', async () => {
|
||||
callToolMock.mockResolvedValue({ byDepth: {}, risk: 'LOW', partial: true });
|
||||
const { impactCommand } = await import('../../src/cli/tool.js');
|
||||
|
||||
await impactCommand('someSymbol', { direction: 'upstream' });
|
||||
|
||||
expect(process.exitCode).toBe(1);
|
||||
});
|
||||
|
||||
it('fails closed when context returns a backend error payload', async () => {
|
||||
callToolMock.mockResolvedValue({ error: 'Symbol not found: nope' });
|
||||
const { contextCommand } = await import('../../src/cli/tool.js');
|
||||
|
|
@ -173,8 +195,8 @@ describe('direct CLI tool commands', () => {
|
|||
await detectChangesCommand({});
|
||||
|
||||
expect(writeSyncMock).toHaveBeenCalledWith(1, expect.stringContaining('Error: index is stale'));
|
||||
// The formatter renders the error as TEXT, so `output()`'s object-payload
|
||||
// check never sees it — the status has to come off the structured result.
|
||||
// `output()` gets the structured result plus its formatter, so the
|
||||
// object-payload check sees the `error` the rendered prose hides.
|
||||
// `gitnexus detect-changes && git commit` must not proceed here.
|
||||
expect(process.exitCode).toBe(1);
|
||||
});
|
||||
|
|
|
|||
|
|
@ -41,6 +41,58 @@ describe('generateId', () => {
|
|||
});
|
||||
|
||||
describe('mapConcurrent', () => {
|
||||
/**
|
||||
* Yield to the event loop once the microtask queue is drained, which is
|
||||
* exactly when `mapConcurrent` has finished awaiting one wave and started the
|
||||
* next. `setImmediate` fires after microtasks by definition, so this waits on
|
||||
* the scheduler rather than on elapsed time.
|
||||
*/
|
||||
const settleWave = (): Promise<void> => new Promise((resolve) => setImmediate(resolve));
|
||||
|
||||
/**
|
||||
* Drive `mapConcurrent` over `itemCount` items whose promises are all held
|
||||
* open by hand, releasing everything in flight one batch at a time.
|
||||
*
|
||||
* Same idea as the ordering test above: real `setTimeout` sleeps only made
|
||||
* the same contract slower and jitter-dependent — a loaded shard could let a
|
||||
* 5ms item outlive the next scheduling decision. Here nothing settles until
|
||||
* this function says so, so `peak` is the scheduler's doing and nothing else.
|
||||
*
|
||||
* Returns how many items were in flight at the start of each batch, and the
|
||||
* highest number ever concurrently in flight.
|
||||
*/
|
||||
async function releaseInWaves(
|
||||
itemCount: number,
|
||||
concurrency: number,
|
||||
): Promise<{ started: number[]; peak: number }> {
|
||||
let inFlight = 0;
|
||||
let peak = 0;
|
||||
const holds: (() => void)[] = [];
|
||||
const settled = mapConcurrent(
|
||||
Array.from({ length: itemCount }, (_, i) => i),
|
||||
() =>
|
||||
new Promise<void>((resolve) => {
|
||||
inFlight += 1;
|
||||
peak = Math.max(peak, inFlight);
|
||||
holds.push(() => {
|
||||
inFlight -= 1;
|
||||
resolve();
|
||||
});
|
||||
}),
|
||||
{ concurrency },
|
||||
);
|
||||
|
||||
const started: number[] = [];
|
||||
for (let wave = 0; wave < Math.ceil(itemCount / concurrency); wave += 1) {
|
||||
started.push(holds.length);
|
||||
for (const release of holds.splice(0)) release();
|
||||
await settleWave();
|
||||
}
|
||||
|
||||
await settled;
|
||||
return { started, peak };
|
||||
}
|
||||
|
||||
it('returns results in INPUT order regardless of completion order', async () => {
|
||||
// Deterministic by construction: each item's promise is settled by hand in
|
||||
// an order chosen here, so the test cannot depend on how loaded the shard
|
||||
|
|
@ -69,19 +121,13 @@ describe('mapConcurrent', () => {
|
|||
});
|
||||
|
||||
it('never exceeds the concurrency limit', async () => {
|
||||
let inFlight = 0;
|
||||
let peak = 0;
|
||||
await mapConcurrent(
|
||||
Array.from({ length: 9 }, (_, i) => i),
|
||||
async () => {
|
||||
inFlight += 1;
|
||||
peak = Math.max(peak, inFlight);
|
||||
await new Promise((resolve) => setTimeout(resolve, 5));
|
||||
inFlight -= 1;
|
||||
},
|
||||
{ concurrency: 2 },
|
||||
);
|
||||
const { started, peak } = await releaseInWaves(9, 2);
|
||||
|
||||
// 9 items at concurrency 2: four full waves and a remainder of one. Nothing
|
||||
// ran outside a wave, which is what the peak below rests on — an
|
||||
// implementation that ignored `concurrency` would show 9 here and a peak
|
||||
// of 9.
|
||||
expect(started).toEqual([2, 2, 2, 2, 1]);
|
||||
expect(peak).toBe(2);
|
||||
});
|
||||
|
||||
|
|
@ -104,19 +150,9 @@ describe('mapConcurrent', () => {
|
|||
});
|
||||
|
||||
it('runs sequentially when concurrency is 1', async () => {
|
||||
let inFlight = 0;
|
||||
let peak = 0;
|
||||
await mapConcurrent(
|
||||
[1, 2, 3],
|
||||
async () => {
|
||||
inFlight += 1;
|
||||
peak = Math.max(peak, inFlight);
|
||||
await new Promise((resolve) => setTimeout(resolve, 1));
|
||||
inFlight -= 1;
|
||||
},
|
||||
{ concurrency: 1 },
|
||||
);
|
||||
const { started, peak } = await releaseInWaves(3, 1);
|
||||
|
||||
expect(started).toEqual([1, 1, 1]);
|
||||
expect(peak).toBe(1);
|
||||
});
|
||||
|
||||
|
|
|
|||
|
|
@ -46,6 +46,7 @@ import {
|
|||
getInterModuleCallEdges,
|
||||
getProcessesForFiles,
|
||||
} from '../../src/core/wiki/graph-queries.js';
|
||||
import { CALL_EDGE_LIMIT } from '../../src/core/wiki/prompts.js';
|
||||
|
||||
// ─── Fixture ──────────────────────────────────────────────────────────────
|
||||
|
||||
|
|
@ -65,17 +66,11 @@ const DISTANT_CALLER = MODULE_FILES[0];
|
|||
const DISTANT_CALLEE = MODULE_FILES[FILE_COUNT - 10];
|
||||
|
||||
/**
|
||||
* Mirrors the private `CALL_EDGE_LIMIT` in graph-queries.ts. Not exported there
|
||||
* on purpose — it exists only because `formatCallEdges` (prompts.ts) slices to
|
||||
* 30 — so this suite restates it and fails loudly if the two ever diverge.
|
||||
*/
|
||||
const CALL_EDGE_LIMIT = 30;
|
||||
|
||||
/**
|
||||
* More intra-module edges than `CALL_EDGE_LIMIT` (30), so the LIMIT the query
|
||||
* carries actually has something to cut. With the two hand-written edges below
|
||||
* the intra arm matches 42 rows; before these existed it matched 2, and no test
|
||||
* could tell a query that limits from one that doesn't.
|
||||
* More intra-module edges than `CALL_EDGE_LIMIT` (30, imported from prompts.ts
|
||||
* — the one place that number lives), so the LIMIT the query carries actually
|
||||
* has something to cut. With the two hand-written edges below the intra arm
|
||||
* matches 42 rows; before these existed it matched 2, and no test could tell a
|
||||
* query that limits from one that doesn't.
|
||||
*/
|
||||
const BULK_EDGES: Edge[] = Array.from({ length: 40 }, (_, i) => ({
|
||||
fromFile: MODULE_FILES[i % 5],
|
||||
|
|
@ -172,19 +167,6 @@ function answerProcessHeaders(query: string, paths: string[]): QueryRow[] {
|
|||
.slice(0, limit);
|
||||
}
|
||||
|
||||
/**
|
||||
* LadybugDB returns a node's label as a SCALAR string, so `labels(s)[0]` is a
|
||||
* 1-based character subscript on it — out of range, hence ''. `labels(s)[1]`
|
||||
* would be 'F'. Modelling that is what lets this suite see the regression that
|
||||
* described all 5,027 exported symbols to the LLM as `name ()`; the fake used
|
||||
* to hardcode 'Function' and could observe neither the bug nor the fix.
|
||||
*/
|
||||
const projectLabel = (query: string, label: string): string => {
|
||||
const subscript = /labels\(\w+\)\[(\d+)\]/.exec(query)?.[1];
|
||||
const index = Number(subscript);
|
||||
return subscript === undefined ? label : (label[index - 1] ?? '');
|
||||
};
|
||||
|
||||
/**
|
||||
* The seeded arrival order of each process's steps: 2, then 0, then 1.
|
||||
*
|
||||
|
|
@ -198,14 +180,21 @@ const projectLabel = (query: string, label: string): string => {
|
|||
*/
|
||||
const SEEDED_STEP_ORDER = [2, 0, 1];
|
||||
|
||||
/** One row per (process, step), the shape the grouped step query returns. */
|
||||
function answerProcessSteps(query: string, ids: string[]): QueryRow[] {
|
||||
/**
|
||||
* One row per (process, step), the shape the grouped step query returns.
|
||||
*
|
||||
* `type` is a plain label: what LadybugDB actually answers for a `labels(s)`
|
||||
* projection is the engine's business, and is pinned against a real engine in
|
||||
* test/integration/wiki-graph-queries-engine.test.ts. What is left here is the
|
||||
* row→object mapping.
|
||||
*/
|
||||
function answerProcessSteps(ids: string[]): QueryRow[] {
|
||||
return SEEDED_STEP_ORDER.flatMap((step) =>
|
||||
ids.map((pid) => ({
|
||||
pid,
|
||||
name: `${pid}-step${step}`,
|
||||
filePath: MODULE_FILES[step],
|
||||
type: projectLabel(query, 'Function'),
|
||||
type: 'Function',
|
||||
step,
|
||||
})),
|
||||
);
|
||||
|
|
@ -217,7 +206,7 @@ beforeEach(() => {
|
|||
executeParameterizedMock.mockImplementation(
|
||||
async (_repo: string, query: string, params: Record<string, unknown>) => {
|
||||
seen.push({ query, params });
|
||||
if (query.includes('p.id IN $ids')) return answerProcessSteps(query, params.ids as string[]);
|
||||
if (query.includes('p.id IN $ids')) return answerProcessSteps(params.ids as string[]);
|
||||
const paths = (params.paths ?? []) as string[];
|
||||
if (query.includes('STEP_IN_PROCESS')) return answerProcessHeaders(query, paths);
|
||||
return answerCallEdges(query, paths);
|
||||
|
|
@ -355,13 +344,13 @@ describe('#2915 wiki graph queries bind their file list', () => {
|
|||
expect(processes[0].steps.map((s) => s.step)).toEqual(SEEDED_STEP_ORDER);
|
||||
});
|
||||
|
||||
it('reads the step label and number off the named columns', async () => {
|
||||
it('reads the step number off its named column, including a genuine 0', async () => {
|
||||
const processes = await getProcessesForFiles(MODULE_FILES, 1);
|
||||
|
||||
// `labels(s)`, not `labels(s)[0]` — the subscript is an out-of-range
|
||||
// character index on a scalar string, so every step's type rendered as ''.
|
||||
expect(processes[0].steps.map((s) => s.type)).toEqual(['Function', 'Function', 'Function']);
|
||||
// A step genuinely numbered 0 keeps its own number.
|
||||
// A step genuinely numbered 0 keeps its own number — a falsy check on the
|
||||
// column would substitute an index or drop the step entirely. (What the
|
||||
// engine answers for the step's LABEL is asserted in
|
||||
// test/integration/wiki-graph-queries-engine.test.ts.)
|
||||
expect(processes[0].steps.map((s) => s.step)).toContain(0);
|
||||
});
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue