From 11fc43b4250210fefb70e067e8798c39f46803da Mon Sep 17 00:00:00 2001 From: jelsco <58397194+jelsco@users.noreply.github.com> Date: Thu, 28 May 2026 09:15:37 -0600 Subject: [PATCH] feat(impact): per-symbol processes field on byDepth items (#1867) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(impact): per-symbol processes field on byDepth items Today `impact` returns aggregated `affected_processes` at the top level but the per-symbol `byDepth` items don't say which processes each caller participates in. Consumers planning a deploy want to know if a given caller is hit by a daily cron, a webhook, or a user-facing route - each is a different deploy-risk profile - and that information requires a follow-up cypher query per symbol today. This change attaches `processes: [...]` to every `byDepth[depth][i]` item, listing the processes that symbol participates in: byDepth: { "1": [ { depth: 1, id: "Function:src/foo.ts:doStuff", name: "doStuff", ... processes: [ { id: "proc:cron_daily", label: "Daily cron", processType: "cron", step: 12 } ] } ] } The list is empty for symbols not in any process. Additive change, no breaking modifications to existing fields. Implementation: - A second chunked Cypher pass runs after the existing per-process aggregation pass, returning per-(symbol, process) rows. Same chunk size and MAX_CHUNKS as the aggregation pass, so worst-case adds 10 extra round-trips bounded by the same env var. - The enrichment pass is skipped entirely when `affectedProcesses.length === 0` (nothing to enrich) or `summaryOnly === true` (byDepth not returned anyway). - The aggregation query is unchanged - the new query has a distinct RETURN shape (`RETURN s.id AS sid, ...`) so an existing unit test that counts STEP_IN_PROCESS chunks was narrowed to match only the aggregation pattern. Tests: - New: byDepth items always have a `processes` field (default empty when no STEP_IN_PROCESS edges exist). - New: when STEP_IN_PROCESS rows exist, the matching byDepth item carries the right `{id, label, processType, step}` entry. - Updated: impact-batching-grouping test mock narrowed to count only aggregation chunks (the new per-symbol pass is covered separately). * style: apply prettier to gitnexus/src/mcp/local/local-backend.ts Pure line-wrap fix flagged by quality / format CI on PR #1867. Zero semantic change: prettier broke a chained .slice().map() across three lines instead of one. No test changes, no logic changes. * fix(impact): address PR review findings on per-symbol process enrichment - byDepth.processes doc now states each item carries processes (Finding 1) - move per-symbol STEP_IN_PROCESS enrichment post-pagination so symbols beyond the pre-pagination cap no longer get false-empty processes:[] (Finding 2); hoist CHUNK_SIZE/MAX_CHUNKS to function scope so the post-pagination pass can reference them - dedup per-symbol query with DISTINCT + MIN(r.step) per (symbol,process) pair (Finding 3) - suppress the per-symbol pass under summaryOnly, incl. impactByUid group fan-out, plus a test asserting the query never fires (Findings 4, 6) * fix(impact): address second-round review findings A-E Finding A (blocker): impactByUid passed summaryOnly:true, which drops the entire byDepth field. cross-impact.ts reads fan.byDepth to build the group by_depth output, so cross-repo by_depth was always {}. Replace with a new skipPerSymbolEnrichment option on _runImpactBFS that suppresses only the per-symbol STEP_IN_PROCESS pass while preserving byDepth. Finding B+D (blocker): rewrite the byDepth.processes tool description. Drop the stale "enrichment cap" wording (no longer true post-pagination), document the {id,label,processType,step} entry shape, and tell agents to cross-check affected_processes when partial:true. Finding C: bound the post-pagination per-symbol enrichment loop to MAX_CHUNKS*CHUNK_SIZE page IDs and surface partial:true when capped, so a large page cannot trigger unbounded DB round-trips (DoD 2.6). Finding E: add a test exercising the real impactByUid -> _runImpactBFS path asserting byDepth survives and the per-symbol query never fires. --------- Co-authored-by: scotjelinski <58397194+scotjelinski@users.noreply.github.com> Co-authored-by: Gergő Magyar --- gitnexus/src/mcp/local/local-backend.ts | 113 ++++++++- gitnexus/src/mcp/tools.ts | 2 +- gitnexus/test/unit/calltool-dispatch.test.ts | 223 ++++++++++++++++++ .../unit/impact-batching-grouping.test.ts | 10 +- 4 files changed, 339 insertions(+), 9 deletions(-) diff --git a/gitnexus/src/mcp/local/local-backend.ts b/gitnexus/src/mcp/local/local-backend.ts index cf9ab483b..5a9290394 100644 --- a/gitnexus/src/mcp/local/local-backend.ts +++ b/gitnexus/src/mcp/local/local-backend.ts @@ -2907,9 +2907,11 @@ export class LocalBackend { limit?: number; offset?: number; summaryOnly?: boolean; + skipPerSymbolEnrichment?: boolean; }, ): Promise { const { maxDepth, relationTypes, includeTests, minConfidence } = opts; + const skipPerSymbolEnrichment = opts.skipPerSymbolEnrichment ?? false; const hasExplicitLimit = typeof opts.limit === 'number' && Number.isFinite(opts.limit); const paginationLimit = hasExplicitLimit ? Math.max(1, Math.min(Math.trunc(opts.limit!), 10000)) @@ -3066,13 +3068,25 @@ export class LocalBackend { const directCount = (grouped[1] || []).length; let affectedProcesses: any[] = []; let affectedModules: any[] = []; + // Per-symbol process membership: maps impacted symbol id -> list of processes + // it participates in. Populated by a second chunked Cypher pass below when + // any process is affected at all. Surfaced as `processes: [...]` on each + // byDepth item so consumers can tell which caller belongs to which cron/ + // webhook/route without a follow-up query. + const perSymbolProcesses = new Map< + string, + Array<{ id: string; label: string; processType: string; step: number }> + >(); + + // Chunking bounds for batched DB round-trips. Declared at function scope so + // both the in-block enrichment passes and the post-pagination per-symbol + // process enrichment can reference them. + const CHUNK_SIZE = 100; + // Max number of chunks to process to avoid unbounded DB round-trips. + // Configurable via env IMPACT_MAX_CHUNKS, default 10 => max items = 1000 + const MAX_CHUNKS = parseInt(process.env.IMPACT_MAX_CHUNKS || '10', 10); if (impacted.length > 0) { - const CHUNK_SIZE = 100; - // Max number of chunks to process to avoid unbounded DB round-trips. - // Configurable via env IMPACT_MAX_CHUNKS, default 10 => max items = 1000 - const MAX_CHUNKS = parseInt(process.env.IMPACT_MAX_CHUNKS || '10', 10); - // ── Process enrichment: batched chunking (bounded by MAX_CHUNKS) ─ // Uses merged Cypher query (WITH + OPTIONAL MATCH) to fetch // process + entry point info in 1 round-trip per chunk. Converted to @@ -3218,6 +3232,10 @@ export class LocalBackend { })) .sort((a, b) => b.total_hits - a.total_hits); + // Per-symbol process membership is populated post-pagination (see below) + // so it covers exactly the symbols returned in byDepth, not a pre-capped + // flat slice that could miss depth-2+ symbols when depth-1 is large. + // ── Module enrichment: use same cap as process enrichment and parameterized queries const maxItems = Math.min(impacted.length, MAX_CHUNKS * CHUNK_SIZE); const cappedImpacted = impacted.slice(0, maxItems); @@ -3360,7 +3378,7 @@ export class LocalBackend { return base; } - // Apply limit/offset pagination per depth level + // Apply limit/offset pagination per depth level. const paginatedGrouped: Record = {}; let anyTruncated = false; for (const [depth, items] of Object.entries(grouped)) { @@ -3372,8 +3390,82 @@ export class LocalBackend { } } + // ── Per-symbol process membership enrichment (post-pagination) ─────── + // Runs after paginatedGrouped is built so we enrich only the IDs that + // actually appear in the response. This eliminates the false-empty + // processes:[] case where a depth-2+ symbol's flat position in `impacted` + // exceeded MAX_CHUNKS*CHUNK_SIZE even though it is returned by byDepth. + // Also uses DISTINCT + MIN(r.step) per (symbol, process) pair to avoid + // duplicate entries when a symbol has multiple STEP_IN_PROCESS edges. + // Skipped entirely when `skipPerSymbolEnrichment` is set (group cross-repo + // fan-out, which consumes byDepth but not byDepth[].processes); the + // attach-loop below still stamps an empty processes:[] for shape stability. + let perSymbolEnrichmentCapped = false; + if (affectedProcesses.length > 0 && !skipPerSymbolEnrichment) { + // Collect unique IDs from the paginated result in one pass. + const pageIds = new Set(); + for (const items of Object.values(paginatedGrouped)) { + for (const it of items) { + const id = String(it.id ?? ''); + if (id) pageIds.add(id); + } + } + // Bound the enrichment to the same ceiling as the aggregation pass + // (MAX_CHUNKS * CHUNK_SIZE) so a large paginated page cannot trigger + // unbounded DB round-trips (DoD 2.6). When capped, mark the result + // partial so callers know some returned symbols may carry an empty + // processes:[] that is a cap artifact, not a true absence. + const maxPageIds = MAX_CHUNKS * CHUNK_SIZE; + let pageIdArr = Array.from(pageIds); + if (pageIdArr.length > maxPageIds) { + pageIdArr = pageIdArr.slice(0, maxPageIds); + perSymbolEnrichmentCapped = true; + } + for (let i = 0; i < pageIdArr.length; i += CHUNK_SIZE) { + const chunkIds = pageIdArr.slice(i, i + CHUNK_SIZE); + try { + const rows = await executeParameterized( + repo.id, + ` + MATCH (s)-[r:CodeRelation {type: 'STEP_IN_PROCESS'}]->(p:Process) + WHERE s.id IN $ids + RETURN s.id AS sid, p.id AS pid, p.heuristicLabel AS pName, + p.processType AS pType, MIN(r.step) AS step + `, + { ids: chunkIds }, + ).catch(() => []); + for (const row of rows) { + const sid = row.sid ?? row[0]; + if (!sid) continue; + const procEntry = { + id: String(row.pid ?? row[1] ?? ''), + label: String(row.pName ?? row[2] ?? ''), + processType: String(row.pType ?? row[3] ?? ''), + step: Number(row.step ?? row[4] ?? -1), + }; + const list = perSymbolProcesses.get(String(sid)); + if (list) list.push(procEntry); + else perSymbolProcesses.set(String(sid), [procEntry]); + } + } catch (e) { + logQueryError('impact:per-symbol-process-chunk', e); + } + } + } + + // Attach processes field to each paginated item. + for (const items of Object.values(paginatedGrouped)) { + for (const it of items) { + it.processes = perSymbolProcesses.get(String(it.id)) ?? []; + } + } + return { ...base, + // Surface partial if the per-symbol enrichment was capped, even when the + // BFS traversal itself completed — some returned symbols may carry an + // empty processes:[] that is a cap artifact rather than a true absence. + ...(perSymbolEnrichmentCapped && { partial: true }), ...(anyTruncated && { pagination: { ...(Number.isFinite(paginationLimit) && { limit: paginationLimit }), @@ -3467,11 +3559,20 @@ export class LocalBackend { ]; try { + // skipPerSymbolEnrichment suppresses ONLY the per-symbol STEP_IN_PROCESS + // enrichment pass while preserving byDepth. Group-mode cross-repo fan-out + // may fan across many repos; the per-symbol pass adds up to MAX_CHUNKS + // extra round-trips per repo, which is unacceptable at group scale. But + // cross-impact fan-out DOES consume byDepth (cross-impact.ts reads + // fan.byDepth to populate group by_depth), so summaryOnly would wrongly + // drop it. Group callers do not consume byDepth[].processes, so skipping + // only that enrichment is the correct, targeted suppression. return await this._runImpactBFS(repo, sym, symType, dir, { maxDepth: opts.maxDepth, relationTypes, includeTests: opts.includeTests, minConfidence: opts.minConfidence, + skipPerSymbolEnrichment: true, }); } catch { return null; diff --git a/gitnexus/src/mcp/tools.ts b/gitnexus/src/mcp/tools.ts index 15b7dc7d4..6ee8b5488 100644 --- a/gitnexus/src/mcp/tools.ts +++ b/gitnexus/src/mcp/tools.ts @@ -336,7 +336,7 @@ Output includes: - summary: direct callers, processes affected, modules affected - affected_processes: which execution flows break and at which step - affected_modules: which functional areas are hit (direct vs indirect) -- byDepth: affected symbols grouped by traversal depth (paginated by limit/offset; omitted when summaryOnly:true — use byDepthCounts for totals per depth, pagination object when truncated) +- byDepth: affected symbols grouped by traversal depth (paginated by limit/offset; omitted when summaryOnly:true — use byDepthCounts for totals per depth, pagination object when truncated). Each item includes a processes:[{id,label,processType,step}] field listing the execution flows that symbol participates in. Empty when the symbol has no process membership. Can ALSO be empty when partial:true is set — either the process-aggregation pass hit its cap before detecting affected processes, or per-symbol enrichment was capped on a very large page. When partial:true, do NOT treat processes:[] as proof of no participation; cross-check the top-level affected_processes list. Depth groups: - d=1: WILL BREAK (direct callers/importers) diff --git a/gitnexus/test/unit/calltool-dispatch.test.ts b/gitnexus/test/unit/calltool-dispatch.test.ts index 9596d6b7a..55e9b9a6a 100644 --- a/gitnexus/test/unit/calltool-dispatch.test.ts +++ b/gitnexus/test/unit/calltool-dispatch.test.ts @@ -647,6 +647,229 @@ describe('LocalBackend.callTool', () => { expect(result.target).toBeDefined(); }); + it('impact byDepth items include a processes field (default empty when no processes)', async () => { + // Resolver returns target; BFS returns one frontier caller; no STEP_IN_PROCESS rows. + (executeParameterized as any).mockResolvedValue([ + { id: 'func:main', name: 'main', type: 'Function', filePath: 'src/index.ts' }, + ]); + (executeQuery as any).mockResolvedValue([ + { + id: 'func:caller', + name: 'caller', + type: 'Function', + filePath: 'src/uses-main.ts', + relType: 'CALLS', + confidence: 0.9, + }, + ]); + + const result = await backend.callTool('impact', { target: 'main', direction: 'upstream' }); + const d1 = result.byDepth?.[1] || result.byDepth?.['1'] || []; + expect(d1.length).toBeGreaterThan(0); + for (const item of d1) { + expect(item).toHaveProperty('processes'); + expect(Array.isArray(item.processes)).toBe(true); + } + }); + + it('impact populates byDepth processes when STEP_IN_PROCESS rows exist', async () => { + (executeParameterized as any).mockImplementation((_repoId: string, cypher: string) => { + // Symbol resolver name-lookup + if (cypher.includes('WHERE n.name =')) { + return Promise.resolve([ + { id: 'func:main', name: 'main', type: 'Function', filePath: 'src/index.ts' }, + ]); + } + // Aggregation pass (must return at least one row so per-symbol pass is gated open) + if (cypher.includes('COUNT(DISTINCT s.id)')) { + return Promise.resolve([ + { + pId: 'proc:cron_daily', + name: 'Daily cron', + heuristicLabel: 'Daily cron', + processType: 'cron', + entryPointId: 'func:cron_entry', + hits: 1, + minStep: 1, + stepCount: 5, + epName: 'cron_entry', + epType: 'Function', + epFilePath: 'src/cron.ts', + }, + ]); + } + // New per-symbol pass added by this change + if (cypher.includes('RETURN s.id AS sid')) { + return Promise.resolve([ + { + sid: 'func:caller', + pid: 'proc:cron_daily', + pName: 'Daily cron', + pType: 'cron', + step: 2, + }, + ]); + } + return Promise.resolve([]); + }); + (executeQuery as any).mockResolvedValue([ + { + id: 'func:caller', + name: 'caller', + type: 'Function', + filePath: 'src/uses-main.ts', + relType: 'CALLS', + confidence: 0.9, + }, + ]); + + const result = await backend.callTool('impact', { target: 'main', direction: 'upstream' }); + const d1 = result.byDepth?.[1] || result.byDepth?.['1'] || []; + const caller = d1.find((it: any) => it.id === 'func:caller'); + expect(caller).toBeDefined(); + expect(caller.processes).toHaveLength(1); + expect(caller.processes[0]).toMatchObject({ + id: 'proc:cron_daily', + label: 'Daily cron', + processType: 'cron', + step: 2, + }); + }); + + it('impact summaryOnly:true skips the per-symbol STEP_IN_PROCESS enrichment pass', async () => { + // Resolver returns target; BFS returns one caller; aggregation returns one process row. + (executeParameterized as any).mockImplementation((_repoId: string, cypher: string) => { + if (cypher.includes('WHERE n.name =')) { + return Promise.resolve([ + { id: 'func:main', name: 'main', type: 'Function', filePath: 'src/index.ts' }, + ]); + } + if (cypher.includes('COUNT(DISTINCT s.id)')) { + return Promise.resolve([ + { + pId: 'proc:daily', + name: 'Daily cron', + heuristicLabel: 'Daily cron', + processType: 'cron', + entryPointId: 'func:cron_entry', + hits: 1, + minStep: 1, + stepCount: 5, + epName: 'cron_entry', + epType: 'Function', + epFilePath: 'src/cron.ts', + }, + ]); + } + return Promise.resolve([]); + }); + (executeQuery as any).mockResolvedValue([ + { + id: 'func:caller', + name: 'caller', + type: 'Function', + filePath: 'src/a.ts', + relType: 'CALLS', + confidence: 0.9, + }, + ]); + + const result = await backend.callTool('impact', { + target: 'main', + direction: 'upstream', + summaryOnly: true, + }); + + // summaryOnly should return base fields only, no byDepth + expect(result.summary).toBeDefined(); + expect(result.byDepth).toBeUndefined(); + + // The per-symbol enrichment query contains 'RETURN s.id AS sid'; verify it + // was never called (the gate should have suppressed it). + const perSymbolCalls = (executeParameterized as any).mock.calls.filter( + ([, cypher]: [string, string]) => + typeof cypher === 'string' && cypher.includes('RETURN s.id AS sid'), + ); + expect(perSymbolCalls).toHaveLength(0); + }); + + it('impactByUid preserves byDepth while skipping per-symbol enrichment (group fan-out)', async () => { + // Regression guard for the cross-repo by_depth contract: impactByUid must + // suppress only the per-symbol STEP_IN_PROCESS pass, NOT the whole byDepth + // field. cross-impact.ts reads fan.byDepth to populate group `by_depth`; + // using summaryOnly here would silently empty it. + // + // impactByUid takes an explicit repoId and calls refreshRepos() internally. + // Use a fresh backend whose repo path is already absolute/resolved so the + // derived repoId stays stable across that refresh (an unresolved POSIX + // fixture path triggers the path-collision rehash and drops the key). + const resolvedRepoPath = path.resolve('/tmp/test-project'); + (listRegisteredRepos as any).mockResolvedValue([ + { ...MOCK_REPO_ENTRY, path: resolvedRepoPath }, + ]); + backend = new LocalBackend(); + await backend.init(); + + (executeParameterized as any).mockImplementation((_repoId: string, cypher: string) => { + // UID resolver + if (cypher.includes('WHERE n.id = $uid')) { + return Promise.resolve([ + { id: 'func:main', name: 'main', filePath: 'src/index.ts', type: 'Function' }, + ]); + } + // Aggregation pass (returns a process row so affectedProcesses > 0; if the + // per-symbol pass were not skipped, this would open its gate) + if (cypher.includes('COUNT(DISTINCT s.id)')) { + return Promise.resolve([ + { + pId: 'proc:daily', + name: 'Daily cron', + heuristicLabel: 'Daily cron', + processType: 'cron', + entryPointId: 'func:cron_entry', + hits: 1, + minStep: 1, + stepCount: 5, + epName: 'cron_entry', + epType: 'Function', + epFilePath: 'src/cron.ts', + }, + ]); + } + return Promise.resolve([]); + }); + (executeQuery as any).mockResolvedValue([ + { + id: 'func:caller', + name: 'caller', + type: 'Function', + filePath: 'src/uses-main.ts', + relType: 'CALLS', + confidence: 0.9, + }, + ]); + + const result = await backend.impactByUid('test-project', 'uid:main', 'upstream', { + maxDepth: 5, + relationTypes: ['CALLS'], + minConfidence: 0, + includeTests: true, + }); + + // byDepth must survive (Finding A regression guard) + expect(result).not.toBeNull(); + expect(result.byDepth).toBeDefined(); + const d1 = result.byDepth?.[1] || result.byDepth?.['1'] || []; + expect(d1.find((it: any) => it.id === 'func:caller')).toBeDefined(); + + // The per-symbol enrichment query must never fire under skipPerSymbolEnrichment + const perSymbolCalls = (executeParameterized as any).mock.calls.filter( + ([, cypher]: [string, string]) => + typeof cypher === 'string' && cypher.includes('RETURN s.id AS sid'), + ); + expect(perSymbolCalls).toHaveLength(0); + }); + it('dispatches detect_changes tool', async () => { // detect_changes calls execFileSync which we haven't mocked at module level, // so it will throw a git error — that's fine, we test the error path diff --git a/gitnexus/test/unit/impact-batching-grouping.test.ts b/gitnexus/test/unit/impact-batching-grouping.test.ts index 2a018d972..098d72cd7 100644 --- a/gitnexus/test/unit/impact-batching-grouping.test.ts +++ b/gitnexus/test/unit/impact-batching-grouping.test.ts @@ -97,7 +97,10 @@ describe('impact: batching and grouping', () => { executeParameterizedMock.mockImplementation(async (...args: any[]) => { const query = typeof args[1] === 'string' ? args[1] : String(args[0] ?? ''); const params = args[2] || {}; - if (query.includes('STEP_IN_PROCESS')) { + // Match only the aggregation chunk (which uses COUNT(DISTINCT s.id)), + // not the per-symbol enrichment pass added by impact byDepth processes + // (which also matches STEP_IN_PROCESS but has a different RETURN shape). + if (query.includes('STEP_IN_PROCESS') && query.includes('COUNT(DISTINCT s.id)')) { // Count ids passed in as params.ids const ids = Array.isArray(params.ids) ? params.ids : []; const cnt = ids.length; @@ -263,7 +266,10 @@ describe('impact: batching and grouping', () => { executeParameterizedMock.mockImplementation(async (...args: any[]) => { const query = typeof args[1] === 'string' ? args[1] : String(args[0] ?? ''); const params = args[2] || {}; - if (query.includes('STEP_IN_PROCESS')) { + // Match only the aggregation chunk (which uses COUNT(DISTINCT s.id)), + // not the per-symbol enrichment pass added by impact byDepth processes + // (which also matches STEP_IN_PROCESS but has a different RETURN shape). + if (query.includes('STEP_IN_PROCESS') && query.includes('COUNT(DISTINCT s.id)')) { const ids = Array.isArray(params.ids) ? params.ids : []; chunkSizes.push(ids.length); return [