From 3d83263ada5f6b06e2615edf3d95ede3eb6b52bf Mon Sep 17 00:00:00 2001 From: Colby McHenry Date: Fri, 30 Oct 2026 06:17:38 -0500 Subject: [PATCH] fix(explore): Flow reaches a named symbol a shorter route gets to first The named walk behind codegraph_explore's Flow (and the viewer's /api/flow) kept one route per node and replaced it only with a better route of the same length. In entry -> mid1 -> deep -> leaf beside entry -> mid2 -> leaf, leaf was reached first through the unnamed mid2, so the route through the named deep was turned away for being longer, and the query "entry deep leaf" got a Flow that dropped deep or leaf. On django, SQLCompiler.as_sql pre_sql_setup get_select ran as_sql -> get_qualify_sql -> get_select and left out pre_sql_setup (issue #2970). A node now keeps every route to it that no other route to it beats: one that got there no later, through as many named symbols, ending on no more unnamed hops. That is the same-depth rule of PR #2453 widened, so a longer route through more named symbols survives, while a longer route through fewer is still turned away and can't win the deepest-sink choice; that choice and the longest-chain ranking across seeds are unchanged. A route that beats one waiting at its own depth takes its place in the queue, so nodes are visited in the order they were reached, as before. A route never passes a node twice: with several routes per node, a cycle among named symbols (ping -> pong -> ping) would count them again and out-rank the flow it repeats. Each node's callees are read once per search, across seeds. The explore-exact-target fixture's Flow now runs through every symbol its query names, as django's does. Its compiler.py stays a per-symbol view only while the named and spine bodies outgrow the file's share, which django's do (18,390 chars against 8,895, measured on django/db at 11af25c0) and the miniature's no longer did once the oversized unnamed bridge left the spine, so its get_select grows to django's proportions; no explore render rule changes. Validation: new flow-named-longer-route.test.ts (issue repro through resolveNamedSymbolFlow and codegraph_explore, the bridge budget, no cycles) and the updated explore-exact-target fixture: 3 assertions fail with main's walk, all 11 pass with the fix; 64 other Flow tests pass. Explore A/B, both arms on one index per repo (expressjs/express 0dee2c52, gin-gonic/gin 0f09c3a9, django/db of django 11af25c0, excalidraw d3a9c2a1), 290 queries: 152 graph-derived #2970-shape queries all changed their lead chain, each to one more named symbol (37/37, 55/55, 60/60); of 125 random named-chain controls 11 changed and of 32 hand-written flow queries 1 (django's), each gaining a named symbol; no query lost one, no chain repeats a node, and every query whose Flow is unchanged returns a byte-identical answer. Largest answer 24,997 -> 24,998 chars (cap 25,000). getCallees reads per query set drop 35-50% (gin 657 -> 410, django 1207 -> 752, excalidraw 1407 -> 951); explore time unchanged within noise. Reimplements PR #3003 by @johnhldiver-beep. Fixes #2970 Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 1 + __tests__/explore-exact-target.test.ts | 10 +- __tests__/flow-named-longer-route.test.ts | 97 ++++++++++++++ src/graph/named-symbol-flow.ts | 149 ++++++++++++++-------- 4 files changed, 203 insertions(+), 54 deletions(-) create mode 100644 __tests__/flow-named-longer-route.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index abbe59c183..062995607c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -340,6 +340,7 @@ and adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). - In JavaScript and TypeScript, a value the file imports from your own project and passes along now links only to what it imports: a constant like `isCloudHosted`, a class like a `Star` model, a whole module (`import * as Slack from '../slack'`), or a stylesheet, image or GraphQL document (`import TCO21 from './themes/TCO21.scss'`) no longer links to a function elsewhere that only shares its name. Before, when the project had exactly one function of that name, such a value linked to it: outline's settings screens showed up among the callers of the server's `isCloudHosted()` check, and its stores among those of the `` icon, so `codegraph callers`, impact and `codegraph_explore` followed references that never happen. A component that wraps the function declared beside it, like `export default styled(Disclosure)`, and a page loaded lazily, like `lazy(() => import('../pages/Home'))`, still lead to that function. Re-index JavaScript and TypeScript projects after upgrading. - In JavaScript, an import of a TypeScript module written without an extension, like `import AudioModal from './audio_modal'` in a `.jsx` file next to `audio_modal.tsx`, now links to that module, as it already did from a TypeScript file. Before, a `.js` or `.jsx` file's imports of TypeScript modules were never followed, so in a codebase moving to TypeScript they were matched by name instead: an import under another name linked nothing, and a name that repeats could link to the wrong file, so mastodon's `` and `` tags rendered other components of those names than the ones each file imports. A JavaScript module beside a TypeScript one of the same name still comes first. Re-index JavaScript projects after upgrading. - `codegraph_explore`'s Flow now goes through the implementation your query names when an interface method has several that lead to the same place. Before, it took whichever one came first in the index: asking about prometheus's `Engine.execEvalStmt Queryable.Querier fanout.Querier NewMergeQuerier` traced the call through the TSDB's `DB.Querier` and left the `fanout.Querier` you named off the Flow. Any two routes of the same length are now settled this way, in favor of the one that passes through more of the symbols you named. +- `codegraph_explore`'s Flow now reaches every symbol your query names along a call path, even when a shorter route gets to one of them first through a function you didn't name. Before, for `entry deep leaf` where `entry` calls `deep` (through a helper) and `deep` calls `leaf`, but `entry` also reaches `leaf` through another helper, the Flow stopped at `deep` or skipped it, so a named step was missing and the agent went to read it; on Django, `SQLCompiler.as_sql pre_sql_setup get_select` left out `pre_sql_setup`. A Flow still goes through at most one function you didn't name between two you did, and never round a loop. Thanks @inth3shadows for the report and @johnhldiver-beep for the first fix. (#2970) - `codegraph_explore`'s "Dynamic-dispatch links among your symbols" list now shows every link on its own line. Before, the links from an interface method to its implementations all read alike, like `TrySet → TrySet`, so only one was listed, chosen by the order of the index rather than by your query: asking about gin's `tryToSetValue setter.TrySet multipartRequest.TrySet setByForm` showed `headerSource`'s implementation and hid `formSource`'s. Both ends of such a link are now named with their type, like `setter.TrySet → formSource.TrySet`. When there are more links than the list shows, the implementations your query names come first and those in tests or examples last, and a hop the Flow already shows is not repeated. - `codegraph_explore`, `codegraph_node`, `codegraph_callers`, `codegraph_callees`, `codegraph_impact` and the `codegraph callers`, `callees` and `impact` commands now find a symbol written with its package, the way Go code names it: `refresh.Discovery.refresh`, `storage.Queryable.Querier`, or `gin.Context.Next` in gin's root package. Before, such a name found nothing, so a question written that way lost the symbol and explore's Flow had nothing to connect. A name the search ranked below other results, like gin's `binding.Default`, is found as well, and a Python or Rust module followed by a class and method works the same way. - When a name has more definitions than one `codegraph_node` answer holds, every language and top-level folder it is defined in now shows up, in `codegraph_node` and `codegraph node`. Before, definitions filled the answer in file order, so forty in one folder could hide the only one in another language or a later folder behind a bare "+5 more". Now a language with no code in the answer gets one of its definitions in full, on top of the ones that fit, the list of the others starts with one from each folder, and the rest are counted by language and folder. Thanks @ferrine. diff --git a/__tests__/explore-exact-target.test.ts b/__tests__/explore-exact-target.test.ts index d54d36798f..b1d11958f3 100644 --- a/__tests__/explore-exact-target.test.ts +++ b/__tests__/explore-exact-target.test.ts @@ -107,7 +107,7 @@ class SQLCompiler: def get_select(self, with_col_aliases=False): # GET_SELECT_BODY -${filler('sel', 34)} +${filler('sel', 60)} return [] def get_qualify_sql(self): @@ -235,10 +235,14 @@ class IExact: return result.content?.[0]?.text ?? ''; }; - it('fixture sanity: the flow runs as_sql → get_qualify_sql → get_select and compiler.py goes focused', async () => { + it('fixture sanity: the flow runs as_sql → pre_sql_setup → setup_query → get_select and compiler.py goes focused', async () => { const text = await explore('SQLCompiler.as_sql pre_sql_setup get_select'); expect(text).toContain('**Flow (call path among the symbols you queried)'); - expect(text).toMatch(/1\. as_sql \(compiler\.py:\d+\)[\s\S]*2\. get_qualify_sql[\s\S]*3\. get_select/); + // Through every symbol the query names (#2970), as on django itself: the + // shorter route through the unnamed `get_qualify_sql` skipped `pre_sql_setup`. + // The named and spine bodies outgrow compiler.py's share, as django's do + // (18,390 chars against 8,895), which is what makes it a per-symbol view. + expect(text).toMatch(/1\. as_sql \(compiler\.py:\d+\)[\s\S]*2\. pre_sql_setup[\s\S]*3\. setup_query[\s\S]*4\. get_select/); expect(sectionFor(text, COMPILER), 'the family file renders as a per-symbol view').toContain('· focused'); }); diff --git a/__tests__/flow-named-longer-route.test.ts b/__tests__/flow-named-longer-route.test.ts new file mode 100644 index 0000000000..0bc3f24bfb --- /dev/null +++ b/__tests__/flow-named-longer-route.test.ts @@ -0,0 +1,97 @@ +/** + * Explore's Flow keeps a named symbol that a shorter route reaches first (#2970). + * + * The named walk kept one route per node, replaced only by a better one of the + * same length. In the issue's + * + * entry → mid1 → deep → leaf + * entry → mid2 → leaf + * + * `leaf` was reached first through the unnamed `mid2`, so the route through the + * named `deep` was turned away for being longer, and `entry deep leaf` got the + * Flow `entry → mid1 → deep`, without `leaf`. A node now keeps every route no + * other route to it beats on depth, named symbols and bridge streak together. + * + * What stays: the one-unnamed-bridge budget, the deepest-sink choice, and a + * route never going round a cycle, which a walk that kept routes by named + * count alone would do: `ping → pong → ping → pong → done` counts both twice. + */ +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import * as fs from 'fs'; +import * as os from 'os'; +import * as path from 'path'; +import CodeGraph from '../src/index'; +import { ToolHandler } from '../src/mcp/tools'; +import { resolveNamedSymbolFlow } from '../src/graph/named-symbol-flow'; + +const FILES: Record = { + // The issue's example. + 'src/a.ts': `export function entry() { mid1(); mid2(); } +export function mid1() { deep(); } +export function mid2() { leaf(); } +export function deep() { leaf(); } +export function leaf() { return 1; } +`, + // A named step past two unnamed ones on the way: still out of reach. + 'src/budget.ts': `export function origin() { relay(); hop(); } +function relay() { linker(); } +function linker() { dest(); } +function hop() { dest(); } +export function dest() { return 1; } +`, + // Two named functions calling each other on the way to a third. + 'src/cycle.ts': `export function ping(n: number): number { return n > 0 ? pong(n - 1) : done(); } +export function pong(n: number): number { return ping(n); } +export function done(): number { return 0; } +`, +}; + +let root = ''; +let cg: CodeGraph; + +beforeAll(async () => { + root = fs.mkdtempSync(path.join(os.tmpdir(), 'cg-flow-longer-route-')); + for (const [rel, content] of Object.entries(FILES)) { + fs.mkdirSync(path.dirname(path.join(root, rel)), { recursive: true }); + fs.writeFileSync(path.join(root, rel), content); + } + cg = await CodeGraph.init(root, { index: true }); +}); + +afterAll(() => { + cg?.close(); + if (root) fs.rmSync(root, { recursive: true, force: true }); +}); + +/** The lead chain, as names. */ +const lead = (query: string): string[] => (resolveNamedSymbolFlow(cg, query).chains[0]?.steps ?? []).map((s) => s.node.name); + +describe('a named symbol a shorter route reaches first (#2970)', () => { + it('stays on the Flow, through the named step', () => { + expect(lead('entry deep leaf')).toEqual(['entry', 'mid1', 'deep', 'leaf']); + }); + + it("is in codegraph_explore's Flow section", async () => { + const res = await new ToolHandler(cg).execute('codegraph_explore', { query: 'entry deep leaf' }); + const text = res.content?.[0]?.text ?? ''; + const flow = text.slice(text.indexOf('**Flow (call path among the symbols you queried)**')); + expect(flow).toMatch( + /^\*\*Flow[^\n]*\n\n1\. entry \(src\/a\.ts:1\)\n\s+↓ calls\n2\. mid1 \(src\/a\.ts:2\)\n\s+↓ calls\n3\. deep \(src\/a\.ts:4\)\n\s+↓ calls\n4\. leaf \(src\/a\.ts:5\)\n\n/, + ); + }); + + it('still reads one unnamed step, and no further', () => { + expect(lead('origin dest')).toEqual(['origin', 'hop', 'dest']); + expect(lead('entry leaf')).toEqual(['entry', 'mid2', 'leaf']); + }); + + it('never goes round a cycle', () => { + // As on main: the deepest route that passes each node once. + expect(lead('ping pong done')).toEqual(['pong', 'ping', 'done']); + const chains = resolveNamedSymbolFlow(cg, 'ping pong done', { maxChains: 3 }).chains; + for (const chain of chains) { + const ids = chain.steps.map((s) => s.node.id); + expect(new Set(ids).size).toBe(ids.length); + } + }); +}); diff --git a/src/graph/named-symbol-flow.ts b/src/graph/named-symbol-flow.ts index 599c41ae75..a9ed7008e8 100644 --- a/src/graph/named-symbol-flow.ts +++ b/src/graph/named-symbol-flow.ts @@ -391,6 +391,7 @@ const DIRECTED_VISIT_CAP = 12_000; /** How the `named` walk reached a node, and what the route there carries. */ interface WalkStep { + /** The route this one extends: its key in the walk's parent map. */ prev: string | null; edge: Edge | null; node: Node; @@ -402,6 +403,17 @@ interface WalkStep { named: number; } +/** A node's callees, read once per search however many routes reach it. */ +type CalleesOf = (id: string) => ReturnType; + +/** Whether the route ending at `key` passes `nodeId`: a walk back of at most `maxHops` steps. */ +function onRoute(parent: ReadonlyMap, key: string, nodeId: string): boolean { + for (let cur: string | null = key; cur !== null; cur = parent.get(cur)!.prev) { + if (parent.get(cur)!.node.id === nodeId) return true; + } + return false; +} + /** * Breadth-first over `calls` edges — synthesized ones included, which is what * carries a flow across a callback, a re-render or a JSX child. @@ -410,62 +422,91 @@ interface WalkStep { * at most `maxBridge` unnamed symbols may sit between two of them. That cap is * what bounds the frontier, so {@link NAMED_VISIT_CAP} is generous. * - * Two routes of the same length to one node are settled by the agent's naming, - * not by the order the index lists callees in: the route through more named - * symbols wins. An interface method calls every implementation, so - * prometheus's `Queryable.Querier` reaches `NewMergeQuerier` through - * `fanout.Querier` and the TSDB's `DB.Querier` alike; keeping whichever was - * listed first took the Flow through `DB.Querier` when the query named - * `fanout.Querier`. A rival route comes from another node at the same depth, - * and every node at one depth is expanded before any node at the next, so the - * choice is final before anything is reached through it. Ending on fewer - * unnamed hops ranks first, so a choice never spends bridge budget that a later - * hop needs (with the default budget of one, only a route node's streak can - * differ). + * A node keeps every route to it that no other route to it beats: one that got + * there no later, through as many named symbols, with no more unnamed hops to + * end on. One route per node lost a named symbol (#2970): in `entry → mid2 → + * leaf` beside `entry → mid1 → deep → leaf`, `leaf` was reached first through + * the unnamed `mid2`, so the route through the named `deep` was turned away for + * being longer, and a query naming all three got a Flow without `leaf`. A + * longer route that passes fewer named symbols is still turned away, so it + * can't win the deepest-sink choice over a better one. * - * Returns the parent map, so a caller can reconstruct any reached node's path. + * At one depth the same rule settles which route the agent's naming prefers, + * not the order the index lists callees in: an interface method calls every + * implementation, so prometheus's `Queryable.Querier` reaches `NewMergeQuerier` + * through `fanout.Querier` and the TSDB's `DB.Querier` alike, and the route + * through the named `fanout.Querier` beats the other. Every node at one depth is + * expanded before any node at the next, so a route beaten at its own depth is + * dropped before anything is reached through it. Ending on fewer unnamed hops + * is no worse, so no route spends bridge budget a later hop needs. + * + * Returns the parent map keyed by route, so a caller can reconstruct the path + * of any route that reached a named symbol (`reached`). */ function walkCalls( - cg: CodeGraph, + calleesOf: CalleesOf, seed: Node, named: ReadonlySet, maxHops: number, maxBridge: number ): { parent: Map; reached: string[] } { const parent = new Map(); - parent.set(seed.id, { - prev: null, - edge: null, - node: seed, - depth: 0, - streak: 0, - named: named.has(seed.id) ? 1 : 0, - }); - const queue: string[] = [seed.id]; + /** The routes each reached node keeps, by key; its size is the nodes reached. */ + const routes = new Map(); + const beaten = new Set(); + const queue: string[] = []; + /** Where each route waits in the queue. */ + const slot = new Map(); + const beats = (a: WalkStep, b: WalkStep) => a.depth <= b.depth && a.named >= b.named && a.streak <= b.streak; + const keep = (step: WalkStep): void => { + const kept = routes.get(step.node.id) ?? []; + if (kept.some((other) => beats(parent.get(other)!, step))) return; + const key = String(parent.size); + parent.set(key, step); + // A route beaten here is one of this depth, still waiting in the queue: the + // better route takes its place there, so the walk visits nodes in the order + // it reached them, whichever route won. + let place = -1; + const live: string[] = []; + for (const other of kept) { + if (!beats(step, parent.get(other)!)) { + live.push(other); + continue; + } + beaten.add(other); + const at = slot.get(other)!; + if (place < 0 || at < place) place = at; + } + live.push(key); + routes.set(step.node.id, live); + if (place < 0) { + place = queue.length; + queue.push(key); + } else { + queue[place] = key; + } + slot.set(key, place); + }; + keep({ prev: null, edge: null, node: seed, depth: 0, streak: 0, named: named.has(seed.id) ? 1 : 0 }); const reached: string[] = []; - for (let head = 0; head < queue.length && parent.size < NAMED_VISIT_CAP; head++) { - const id = queue[head]!; - const at = parent.get(id)!; - if (id !== seed.id && named.has(id)) reached.push(id); + for (let head = 0; head < queue.length && routes.size < NAMED_VISIT_CAP; head++) { + const key = queue[head]!; + if (beaten.has(key)) continue; + const at = parent.get(key)!; + const id = at.node.id; + if (id !== seed.id && named.has(id)) reached.push(key); if (at.depth >= maxHops - 1) continue; - for (const c of cg.getCallees(id)) { + for (const c of calleesOf(id)) { if (!FLOW_EDGE_KINDS.has(c.edge.kind)) continue; const isNamed = named.has(c.node.id); // A route node is a connector, not a symbol the reader would have named: // crossing one costs no bridge budget. const streak = isNamed ? 0 : c.node.kind === 'route' ? at.streak : at.streak + 1; if (streak > maxBridge) continue; - const depth = at.depth + 1; - const namedOnRoute = at.named + (isNamed ? 1 : 0); - const prior = parent.get(c.node.id); - if (prior) { - // Reached already: only a better route of the same length replaces it. - if (prior.depth !== depth) continue; - if (streak > prior.streak || (streak === prior.streak && namedOnRoute <= prior.named)) continue; - } else { - queue.push(c.node.id); - } - parent.set(c.node.id, { prev: id, edge: c.edge, node: c.node, depth, streak, named: namedOnRoute }); + // A route never passes a node twice: going round a cycle counts its named + // symbols again and would out-rank the flow it repeats. + if (onRoute(parent, key, c.node.id)) continue; + keep({ prev: key, edge: c.edge, node: c.node, depth: at.depth + 1, streak, named: at.named + (isNamed ? 1 : 0) }); } } return { parent, reached }; @@ -586,7 +627,7 @@ function walkBidirectional( } function chainTo( - parent: Map, + parent: ReadonlyMap, target: string ): FlowStep[] { const steps: FlowStep[] = []; @@ -636,22 +677,28 @@ export function resolveNamedSymbolFlow( if (steps) found.push(steps); } } else { + const callees = new Map>(); + const calleesOf: CalleesOf = (id) => { + let out = callees.get(id); + if (!out) { + out = cg.getCallees(id); + callees.set(id, out); + } + return out; + }; for (const seed of [...flow.named.values()].slice(0, MAX_SEEDS)) { - const { parent, reached } = walkCalls(cg, seed, namedIds, maxHops, maxBridge); + const { parent, reached } = walkCalls(calleesOf, seed, namedIds, maxHops, maxBridge); // Explore's rule: the DEEPEST named sink this seed can reach. Of two as // deep, the one whose route passes more named symbols, as in the walk. - let deepest: WalkStep | null = null; - for (const id of reached) { - const at = parent.get(id)!; - if ( - !deepest || - at.depth > deepest.depth || - (at.depth === deepest.depth && at.named > deepest.named) - ) { - deepest = at; + let deepest: string | null = null; + for (const key of reached) { + const at = parent.get(key)!; + const best = deepest === null ? null : parent.get(deepest)!; + if (!best || at.depth > best.depth || (at.depth === best.depth && at.named > best.named)) { + deepest = key; } } - if (deepest) found.push(chainTo(parent, deepest.node.id)); + if (deepest !== null) found.push(chainTo(parent, deepest)); } }