diff --git a/gitnexus/src/cli/ai-context.ts b/gitnexus/src/cli/ai-context.ts index ab2b1779b..7a4fb16fd 100644 --- a/gitnexus/src/cli/ai-context.ts +++ b/gitnexus/src/cli/ai-context.ts @@ -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 ` 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 diff --git a/gitnexus/src/cli/detect-changes-format.ts b/gitnexus/src/cli/detect-changes-format.ts index fc2d6b1dc..fb04757c0 100644 --- a/gitnexus/src/cli/detect-changes-format.ts +++ b/gitnexus/src/cli/detect-changes-format.ts @@ -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[] = []; diff --git a/gitnexus/src/cli/tool.ts b/gitnexus/src/cli/tool.ts index bba439e9e..e8bedaf20 100644 --- a/gitnexus/src/cli/tool.ts +++ b/gitnexus/src/cli/tool.ts @@ -42,9 +42,18 @@ async function getBackend(): Promise { * 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(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 … && ` 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) }); diff --git a/gitnexus/src/core/ingestion/di-extractors/index.ts b/gitnexus/src/core/ingestion/di-extractors/index.ts index 67dddc39b..66a42775d 100644 --- a/gitnexus/src/core/ingestion/di-extractors/index.ts +++ b/gitnexus/src/core/ingestion/di-extractors/index.ts @@ -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 = new Set(Object.values(SupportedLanguages)); diff --git a/gitnexus/src/core/lbug/query-batch.ts b/gitnexus/src/core/lbug/query-batch.ts index 3974ccd2d..aada83219 100644 --- a/gitnexus/src/core/lbug/query-batch.ts +++ b/gitnexus/src/core/lbug/query-batch.ts @@ -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. diff --git a/gitnexus/src/core/wiki/graph-queries.ts b/gitnexus/src/core/wiki/graph-queries.ts index 56be106ea..5ccbdf987 100644 --- a/gitnexus/src/core/wiki/graph-queries.ts +++ b/gitnexus/src/core/wiki/graph-queries.ts @@ -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 { const stepsById = new Map(); 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) ?? [] })); diff --git a/gitnexus/src/core/wiki/prompts.ts b/gitnexus/src/core/wiki/prompts.ts index 0a6902b7e..57c19077e 100644 --- a/gitnexus/src/core/wiki/prompts.ts +++ b/gitnexus/src/core/wiki/prompts.ts @@ -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'); } diff --git a/gitnexus/src/lib/utils.ts b/gitnexus/src/lib/utils.ts index f7b221949..2e25ecf97 100644 --- a/gitnexus/src/lib/utils.ts +++ b/gitnexus/src/lib/utils.ts @@ -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(items: readonly T[], size: number): T[][] { // `NaN` fails every comparison, so a bare `size < 1` lets it through and diff --git a/gitnexus/src/mcp/local/local-backend.ts b/gitnexus/src/mcp/local/local-backend.ts index 70fdc78ad..ec9656e10 100644 --- a/gitnexus/src/mcp/local/local-backend.ts +++ b/gitnexus/src/mcp/local/local-backend.ts @@ -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`, 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(); + 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, }); } } diff --git a/gitnexus/src/server/analyze-worker.ts b/gitnexus/src/server/analyze-worker.ts index dfc00565c..ad4632eb0 100644 --- a/gitnexus/src/server/analyze-worker.ts +++ b/gitnexus/src/server/analyze-worker.ts @@ -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 diff --git a/gitnexus/test/helpers/detect-changes-diff-args.ts b/gitnexus/test/helpers/detect-changes-diff-args.ts new file mode 100644 index 000000000..1f4d886b9 --- /dev/null +++ b/gitnexus/test/helpers/detect-changes-diff-args.ts @@ -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; +} diff --git a/gitnexus/test/integration/wiki-graph-queries-engine.test.ts b/gitnexus/test/integration/wiki-graph-queries-engine.test.ts index d39fbcbe6..5d7d7a4d7 100644 --- a/gitnexus/test/integration/wiki-graph-queries-engine.test.ts +++ b/gitnexus/test/integration/wiki-graph-queries-engine.test.ts @@ -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 () => { diff --git a/gitnexus/test/unit/detect-changes-eol.test.ts b/gitnexus/test/unit/detect-changes-eol.test.ts index 216a1c043..9ed43a9b4 100644 --- a/gitnexus/test/unit/detect-changes-eol.test.ts +++ b/gitnexus/test/unit/detect-changes-eol.test.ts @@ -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', diff --git a/gitnexus/test/unit/detect-changes-hunk-scale.test.ts b/gitnexus/test/unit/detect-changes-hunk-scale.test.ts index deba49574..77e72c681 100644 --- a/gitnexus/test/unit/detect-changes-hunk-scale.test.ts +++ b/gitnexus/test/unit/detect-changes-hunk-scale.test.ts @@ -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(); 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']); diff --git a/gitnexus/test/unit/shipped-skills-sync.test.ts b/gitnexus/test/unit/shipped-skills-sync.test.ts index dce17a15c..1896e9910 100644 --- a/gitnexus/test/unit/shipped-skills-sync.test.ts +++ b/gitnexus/test/unit/shipped-skills-sync.test.ts @@ -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 ( -// ... ) 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 + * (`` … ``), 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 ` ... `" etc.) that a + // bare indexOf would mistake for the real block (mirrors + // findSectionMarkerIndex in ai-context.ts, #1041). + const match = + /(?:^|\n)\r?\n([\s\S]*?)\n(?:\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 ` ... `" etc.) that a - // bare indexOf would mistake for the real block (mirrors - // findSectionMarkerIndex in ai-context.ts, #1041). - const match = - /(?:^|\n)\r?\n([\s\S]*?)\n(?:\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)); diff --git a/gitnexus/test/unit/tool-direct-cli.test.ts b/gitnexus/test/unit/tool-direct-cli.test.ts index 0db73d063..a34ca3cb2 100644 --- a/gitnexus/test/unit/tool-direct-cli.test.ts +++ b/gitnexus/test/unit/tool-direct-cli.test.ts @@ -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 … && ` 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); }); diff --git a/gitnexus/test/unit/utils.test.ts b/gitnexus/test/unit/utils.test.ts index a4899dff9..95fc73820 100644 --- a/gitnexus/test/unit/utils.test.ts +++ b/gitnexus/test/unit/utils.test.ts @@ -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 => 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((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); }); diff --git a/gitnexus/test/unit/wiki-graph-queries-list-binding.test.ts b/gitnexus/test/unit/wiki-graph-queries-list-binding.test.ts index dece8fdfe..bc8b3b46a 100644 --- a/gitnexus/test/unit/wiki-graph-queries-list-binding.test.ts +++ b/gitnexus/test/unit/wiki-graph-queries-list-binding.test.ts @@ -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) => { 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); });