experiment: graphrag-v2 optimizations#135
Merged
greynewell merged 5 commits intomainfrom Feb 25, 2026
Merged
Conversation
Add experimental GraphRAG mode with two call-graph tools that outperform symbol_context on SWE-bench (6/10 vs 5/10, 17% cheaper, 0 regressions): - explore_function: BFS call-graph traversal with domain annotations and cross-subsystem markers (← DIFFERENT SUBSYSTEM), up to 3 hops - find_connections: Find bridge functions between two domains/subsystems Activated via SUPERMODEL_EXPERIMENT=graphrag env var. Default mode (symbol_context) is unchanged. Also includes: - tool-variants.ts: experimental tool framings (search, split, annotate) - Integration tests for GraphRAG mode (5 new tests) - Updated README with GraphRAG documentation - .gitignore cleanup for benchmark artifacts Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Based on analysis of previous benchmark logs: - Remove find_connections (never used by agent, dead weight) - Support Class nodes in explore_function (fixes SYMBOL_NOT_FOUND for classes) - Add source code to explore_function output (saves a Read round-trip) - Put file paths before domain info in output (more prominent) - Fix instructions: remove "Use Task tool" (causes timeouts), max 2 MCP calls, add "never create standalone test scripts" guidance Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
explore-function.ts: - Validate direction parameter (reject invalid values with error) - Fix item numbering bug (was always "1.", now sequential) - Add empty-state messages for all BFS depths (not just d===2) - Add upstream BFS empty-state messages (was asymmetric with downstream) - Fix stale comment about "no source code" server.integration.test.ts: - Extract shared createServerFixture() to eliminate ~90 lines of duplication - Fix throw-in-EventEmitter bug (use error flag instead) - Move initialize call to beforeAll so tests don't depend on execution order Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
WalkthroughAdds an experiment-driven GraphRAG mode: new graph-based tools (explore_function, find_connections, several tool variants), server logic to select tool sets via SUPERMODEL_EXPERIMENT, updated integration tests and README, and ancillary packaging/gitignore changes. Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant Server as Server
participant Graph as Graph Index
participant Domains as Domain Map
Client->>Server: initialize() (env SUPERMODEL_EXPERIMENT=graphrag)
Server->>Server: read SUPERMODEL_EXPERIMENT
Server->>Server: select graphrag toolset (explore_function, find_connections)
Server-->>Client: ready (GraphRAG tools exposed)
Client->>Server: call explore_function(symbol, directory, direction, depth)
Server->>Graph: fetch/resolve graph for directory
Graph-->>Server: graph data
Server->>Domains: build partOf/domain map for nodes
Domains-->>Server: domain/subdomain info
Server->>Server: resolve root symbol -> node
Server->>Server: BFS traverse callers/callees (up to depth)
Server->>Server: annotate nodes (domain prefix, DIFFERENT SUBSYSTEM markers)
Server-->>Client: formatted report (root, source snippet, Via hierarchies)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
jonathanpopham
approved these changes
Feb 25, 2026
Collaborator
jonathanpopham
left a comment
jonathanpopham
left a comment
There was a problem hiding this comment.
1 is better than 2
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (7)
src/server.integration.test.ts (1)
87-99: Readiness check only verifies stdin is writable — not that the server is actually listening.The polling loop at line 95 checks
server.stdin?.writable, but that's true as soon as the child process spawns (the pipe is open from the OS side). It doesn't confirm the server has finished its startup and is ready to handle JSON-RPC.In practice, this works because
initializeis sent right after (line 106) and will block until the server responds. But if startup ever gets slower (e.g., heavy cache loading), theinitializecall at line 106 could hit the 5s timeout insendRequest.Just something to be aware of — not blocking, since it works today.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server.integration.test.ts` around lines 87 - 99, The readiness loop currently treats server.stdin?.writable as readiness which is insufficient; change the check in the polling loop that uses STARTUP_POLL_INTERVAL_MS and SERVER_STARTUP_TIMEOUT_MS to instead perform an active health/readiness probe against the server (for example by attempting a lightweight JSON-RPC ping or the actual initialize handshake via sendRequest/initialize with a short timeout) and only mark ready when that probe succeeds; keep existing exitCode and startupError checks but replace or augment the stdin.writable check with a successful probe so initialize won't hit the 5s sendRequest timeout if startup is slow.src/tools/find-connections.ts (2)
119-155: Potential duplicate bridge entries when domains overlap via fuzzy matching.Both loops scan independently — A→B and then B→A. If fuzzy matching causes a node to land in both
aNodesandbNodes(e.g., domain names are substrings of each other), you could get the same call edge reported twice from different perspectives.A simple
Set<string>to dedup the formatted bridge strings would fix this:♻️ Proposed dedup
// Find call edges between domain A and domain B in both directions - const bridges: string[] = []; + const bridgeSet = new Set<string>(); + const bridges: string[] = []; for (const aId of aNodes) { // ... existing A→B loop ... - bridges.push( - `\`${srcName}\` (${domainA}) calls \`${tgtName}\` (${domainB}) — ${srcFile} → ${tgtFile}` - ); + const entry = `\`${srcName}\` (${domainA}) calls \`${tgtName}\` (${domainB}) — ${srcFile} → ${tgtFile}`; + if (!bridgeSet.has(entry)) { bridgeSet.add(entry); bridges.push(entry); } } } for (const bId of bNodes) { // ... existing B→A loop ... - bridges.push( - `\`${srcName}\` (${domainB}) calls \`${tgtName}\` (${domainA}) — ${srcFile} → ${tgtFile}` - ); + const entry = `\`${srcName}\` (${domainB}) calls \`${tgtName}\` (${domainA}) — ${srcFile} → ${tgtFile}`; + if (!bridgeSet.has(entry)) { bridgeSet.add(entry); bridges.push(entry); } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/find-connections.ts` around lines 119 - 155, The two loops building bridge messages (iterating aNodes then bNodes using graph.callAdj) can produce duplicates when a node appears in both aNodes and bNodes; wrap the formatted bridge string generation (the pushes to bridges in the blocks that use srcName/tgtName, domainA/domainB, srcFile/tgtFile and normalizePath) with a deduplication step: maintain a Set<string> (e.g., seenBridges) and before pushing check seenBridges.has(formatted) and only push and add to the set if not present; alternatively dedupe after both loops by converting bridges to a Set and back to array. This targets the bridges array population logic around the for-loops that reference aNodes, bNodes, graph.callAdj, normalizePath, domainA and domainB.
52-70:collectDomainMembersonly collectsFunctionnodes — Class nodes are excluded.If a
Classnode acts as a bridge between two domains, it won't show up in results. This is fine if intentional (the description says "functions"), but it's worth knowing this differs fromexplore-function.tswhich handles bothFunctionandClassnodes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/find-connections.ts` around lines 52 - 70, collectDomainMembers currently only adds nodes whose first label is 'Function', so Class nodes that should act as bridges are omitted; update the filtering in collectDomainMembers (used in find-connections.ts) to include Class nodes as well (e.g., check node?.labels?.[0] === 'Function' || node?.labels?.[0] === 'Class' or use an includes test), making its behavior consistent with explore-function.ts; if the original intent was to restrict to functions, instead add a clear comment or rename the function to reflect that restriction.src/tools/explore-function.ts (3)
100-134:resolveDomaindoes a nested lookup per domain entry — fine for small domain counts, but worth knowing.For each domain in
domainIndex, it does anameIndex.get()+nodeById.get()to figure out if it's aSubdomainorDomainnode. With typical codebases (say, 5–20 domains), this is negligible. But if someone has a huge number of domains, this could add up since it's called once per node in the BFS output.If you ever see this on a profile, caching
nodeId → DomainInfoin aMapbuilt once (like you do withsubdomainToParent) would make it O(1) per lookup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/explore-function.ts` around lines 100 - 134, resolveDomain currently performs a nested nameIndex.get() + nodeById.get() loop for every domain entry which can be expensive at scale; instead precompute a Map<string, DomainInfo> cache (keyed by nodeId) once (similar to subdomainToParent) by iterating graph.domainIndex and resolving each domain node via graph.nameIndex and graph.nodeById to capture whether it's a Subdomain and its parent Domain, then update resolveDomain to first consult this cache for O(1) lookup (falling back to the existing per-entry logic only if the cache misses) so calls to resolveDomain become constant-time; refer to resolveDomain, graph.domainIndex, graph.nameIndex, graph.nodeById, subdomainToParent and the DomainInfo type when implementing.
245-253:forEachcallbacks implicitly return values — flagged by Biome lint.Think of it this way:
forEachsays "I don't care what you return," butlines.push(...)returns a number (the new array length). Biome flags this because it looks like you might've wanted.map()instead. It's harmless here, but afor...ofloop is cleaner and avoids the lint noise.🧹 Fix: use for...of instead of forEach
- lines.push('```' + ext); - if (startLine) { - displayLines.forEach((l, i) => lines.push(`${startLine + i}: ${l}`)); - } else { - displayLines.forEach(l => lines.push(l)); - } + lines.push('```' + ext); + if (startLine) { + for (let i = 0; i < displayLines.length; i++) { + lines.push(`${startLine + i}: ${displayLines[i]}`); + } + } else { + for (const l of displayLines) { + lines.push(l); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/explore-function.ts` around lines 245 - 253, The Biome lint flags implicit returns from the displayLines.forEach callbacks in explore-function.ts; replace those forEach calls with explicit loops to avoid returning values from the callbacks. Inside the block where lines.push('```' + ext) is used, change the conditional that currently uses displayLines.forEach((l, i) => ...) and displayLines.forEach(l => ...) to an index-based for loop when startLine is truthy (use i and access displayLines[i] to push `${startLine + i}: ${displayLines[i]}`) and to a for...of loop (for (const l of displayLines)) when startLine is falsy; keep the surrounding logic that handles truncated and MAX_SOURCE_LINES unchanged and still push the final '```' and empty line.
243-245: Inconsistent language mapping — raw extension used instead oflanguageFromExtension().On line 243 you do
filePath.split('.').pop()which gives you'ts','py', etc. Butsymbol-context.tsalready exportslanguageFromExtension()that maps.py→'python',.ts→'typescript', etc. This means your code fences will say```tshere but```typescriptinsymbol_contextoutput.♻️ Reuse the existing helper
import { findSymbol } from './symbol-context'; +import { languageFromExtension } from './symbol-context'; import { MAX_SOURCE_LINES } from '../constants';- const ext = filePath.split('.').pop() || ''; - lines.push('```' + ext); + const lang = languageFromExtension(filePath); + lines.push('```' + lang);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/explore-function.ts` around lines 243 - 245, Replace the raw extension extraction (const ext = filePath.split('.').pop() || ''; lines.push('```' + ext);) with a call to the existing helper languageFromExtension(filePath) so fences use the normalized language name; locate the usage in explore-function.ts where ext and the lines.push('```' + ext) are created and change it to compute const lang = languageFromExtension(filePath) and push lines.push('```' + lang) so your code fences match symbol_context's mapping.src/tools/tool-variants.ts (1)
267-299:annotateHandleris identical tosearchSymbolHandler— straight copy-paste.If you diff lines 50–84 against lines 267–299, the handler logic is exactly the same: validate
symbol, resolve graph,findSymbol,renderBriefSymbolContextfor top 3, append "more matches" note. Only the parameter name differs (queryvssymbol).Since these are experimental variants that might diverge, I get the rationale, but if they stay the same you could just reuse the handler:
♻️ Optional: reuse the handler
-const annotateHandler: HandlerFunction = async (client, args, defaultWorkdir) => { - const symbol = typeof args?.symbol === 'string' ? args.symbol.trim() : ''; - if (!symbol) { - return asErrorResult({ - type: 'validation_error', - message: 'Missing required "symbol" parameter.', - code: 'MISSING_SYMBOL', - recoverable: false, - }); - } - - const rawDir = args?.directory as string | undefined; - const directory = (rawDir && rawDir.trim()) || defaultWorkdir || process.cwd(); - - let graph: IndexedGraph; - try { - graph = await resolveOrFetchGraph(client, directory); - } catch (error: any) { - return asErrorResult({ type: 'internal_error', message: error.message, code: 'GRAPH_ERROR', recoverable: false }); - } - - const matches = findSymbol(graph, symbol); - if (matches.length === 0) { - return asTextContentResult(`No symbol matching "${symbol}" found.`); - } - - const parts = matches.slice(0, 3).map(node => renderBriefSymbolContext(graph, node)); - let result = parts.join('\n---\n\n'); - if (matches.length > 3) { - result += `\n\n*... and ${matches.length - 3} more matches.*`; - } - return asTextContentResult(result); -}; +// Reuses searchSymbolHandler logic — annotate differs only in tool metadata +const annotateHandler: HandlerFunction = async (client, args, defaultWorkdir) => { + // Remap 'symbol' → 'query' so we can reuse the same logic + const remapped = { ...args, query: args?.symbol }; + return searchSymbolHandler(client, remapped, defaultWorkdir); +};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/tool-variants.ts` around lines 267 - 299, The annotateHandler implementation duplicates searchSymbolHandler; to deduplicate, replace the copy-paste by delegating to or reusing the existing searchSymbolHandler: either export annotateHandler as a reference to searchSymbolHandler or implement annotateHandler as a thin wrapper that maps its args.parameter (symbol) to the same shape expected by searchSymbolHandler and then calls it, keeping shared logic in one place (reuse resolveOrFetchGraph, findSymbol, renderBriefSymbolContext only inside searchSymbolHandler). Ensure you update any exports or handler registrations to use the single shared function name (searchSymbolHandler) so there is one canonical implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Line 169: Update the README description for the explore_function parameter
named symbol to explicitly state it supports functions, classes, and methods
(e.g., "function, class, or method symbol") instead of just "Function name";
locate the reference to explore_function and the parameter symbol in the README
and change the wording so examples like UserService and UserService.create are
clearly valid.
- Around line 199-201: Update the README text instances that still state “Max 3
MCP calls total” to the corrected optimization guidance “Max 2 MCP calls total”;
locate the occurrences by searching for the exact string "Max 3 MCP calls total"
(and related mention of MCP call budget) and replace them with "Max 2 MCP calls
total" so lines around the numbered steps (the block containing steps 1–3) and
the repeated mention at 204–207 reflect the updated limit.
In `@src/server.ts`:
- Line 21: Remove the unused import findConnectionsEndpoint from the top-level
imports in src/server.ts; locate the import line that reads "import
findConnectionsEndpoint from './tools/find-connections';" and delete it, and
then verify setupHandlers() and any switch/case or references to the
find_connections tool no longer reference that symbol so there are no
unused-import warnings or broken references.
In `@src/tools/tool-variants.ts`:
- Around line 10-23: Remove the unused imports renderSymbolContext and
MAX_BATCH_SYMBOLS: edit the import that currently pulls renderSymbolContext from
'./symbol-context' to only import findSymbol and renderBriefSymbolContext, and
edit the import from '../constants' to drop MAX_BATCH_SYMBOLS while keeping
MAX_SYMBOL_CALLERS and MAX_SYMBOL_CALLEES; verify there are no remaining
references to renderSymbolContext or MAX_BATCH_SYMBOLS in the file (e.g., in
functions or constants) before committing.
---
Nitpick comments:
In `@src/server.integration.test.ts`:
- Around line 87-99: The readiness loop currently treats server.stdin?.writable
as readiness which is insufficient; change the check in the polling loop that
uses STARTUP_POLL_INTERVAL_MS and SERVER_STARTUP_TIMEOUT_MS to instead perform
an active health/readiness probe against the server (for example by attempting a
lightweight JSON-RPC ping or the actual initialize handshake via
sendRequest/initialize with a short timeout) and only mark ready when that probe
succeeds; keep existing exitCode and startupError checks but replace or augment
the stdin.writable check with a successful probe so initialize won't hit the 5s
sendRequest timeout if startup is slow.
In `@src/tools/explore-function.ts`:
- Around line 100-134: resolveDomain currently performs a nested nameIndex.get()
+ nodeById.get() loop for every domain entry which can be expensive at scale;
instead precompute a Map<string, DomainInfo> cache (keyed by nodeId) once
(similar to subdomainToParent) by iterating graph.domainIndex and resolving each
domain node via graph.nameIndex and graph.nodeById to capture whether it's a
Subdomain and its parent Domain, then update resolveDomain to first consult this
cache for O(1) lookup (falling back to the existing per-entry logic only if the
cache misses) so calls to resolveDomain become constant-time; refer to
resolveDomain, graph.domainIndex, graph.nameIndex, graph.nodeById,
subdomainToParent and the DomainInfo type when implementing.
- Around line 245-253: The Biome lint flags implicit returns from the
displayLines.forEach callbacks in explore-function.ts; replace those forEach
calls with explicit loops to avoid returning values from the callbacks. Inside
the block where lines.push('```' + ext) is used, change the conditional that
currently uses displayLines.forEach((l, i) => ...) and displayLines.forEach(l =>
...) to an index-based for loop when startLine is truthy (use i and access
displayLines[i] to push `${startLine + i}: ${displayLines[i]}`) and to a
for...of loop (for (const l of displayLines)) when startLine is falsy; keep the
surrounding logic that handles truncated and MAX_SOURCE_LINES unchanged and
still push the final '```' and empty line.
- Around line 243-245: Replace the raw extension extraction (const ext =
filePath.split('.').pop() || ''; lines.push('```' + ext);) with a call to the
existing helper languageFromExtension(filePath) so fences use the normalized
language name; locate the usage in explore-function.ts where ext and the
lines.push('```' + ext) are created and change it to compute const lang =
languageFromExtension(filePath) and push lines.push('```' + lang) so your code
fences match symbol_context's mapping.
In `@src/tools/find-connections.ts`:
- Around line 119-155: The two loops building bridge messages (iterating aNodes
then bNodes using graph.callAdj) can produce duplicates when a node appears in
both aNodes and bNodes; wrap the formatted bridge string generation (the pushes
to bridges in the blocks that use srcName/tgtName, domainA/domainB,
srcFile/tgtFile and normalizePath) with a deduplication step: maintain a
Set<string> (e.g., seenBridges) and before pushing check
seenBridges.has(formatted) and only push and add to the set if not present;
alternatively dedupe after both loops by converting bridges to a Set and back to
array. This targets the bridges array population logic around the for-loops that
reference aNodes, bNodes, graph.callAdj, normalizePath, domainA and domainB.
- Around line 52-70: collectDomainMembers currently only adds nodes whose first
label is 'Function', so Class nodes that should act as bridges are omitted;
update the filtering in collectDomainMembers (used in find-connections.ts) to
include Class nodes as well (e.g., check node?.labels?.[0] === 'Function' ||
node?.labels?.[0] === 'Class' or use an includes test), making its behavior
consistent with explore-function.ts; if the original intent was to restrict to
functions, instead add a clear comment or rename the function to reflect that
restriction.
In `@src/tools/tool-variants.ts`:
- Around line 267-299: The annotateHandler implementation duplicates
searchSymbolHandler; to deduplicate, replace the copy-paste by delegating to or
reusing the existing searchSymbolHandler: either export annotateHandler as a
reference to searchSymbolHandler or implement annotateHandler as a thin wrapper
that maps its args.parameter (symbol) to the same shape expected by
searchSymbolHandler and then calls it, keeping shared logic in one place (reuse
resolveOrFetchGraph, findSymbol, renderBriefSymbolContext only inside
searchSymbolHandler). Ensure you update any exports or handler registrations to
use the single shared function name (searchSymbolHandler) so there is one
canonical implementation.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
.gitignoreREADME.mdpackage.jsonsrc/server.integration.test.tssrc/server.tssrc/tools/explore-function.tssrc/tools/find-connections.tssrc/tools/symbol-context.tssrc/tools/tool-variants.ts
README.md
Outdated
| - File import statistics | ||
| #### `explore_function` | ||
|
|
||
| BFS traversal of a function's call graph. Shows callers, callees, and cross-subsystem boundaries with `← DIFFERENT SUBSYSTEM` markers. |
There was a problem hiding this comment.
Document class lookup support explicitly in explore_function params
Line 175 says symbol is a “Function name,” but this PR also adds Class-node support.
Suggest wording like “function, class, or method symbol” so users know class lookups are valid (e.g., UserService as well as UserService.create).
Also applies to: 175-175
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` at line 169, Update the README description for the
explore_function parameter named symbol to explicitly state it supports
functions, classes, and methods (e.g., "function, class, or method symbol")
instead of just "Function name"; locate the reference to explore_function and
the parameter symbol in the README and change the wording so examples like
UserService and UserService.create are clearly valid.
Comment on lines
+199
to
+201
| 1. Identify symbols from the issue and call `symbol_context` to explore them (batch via `symbols` array or parallel calls) | ||
| 2. Use Read/Grep to examine source code at identified locations | ||
| 3. Start editing by turn 3. Max 3 MCP calls total. |
There was a problem hiding this comment.
Update MCP call budget to match current optimization guidance
Small doc mismatch: Line 201 and Line 207 still say “Max 3 MCP calls total,” but this PR’s stated optimization is max 2 calls.
If this stays at 3, users will follow the slower pattern you just optimized away.
Also applies to: 204-207
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 199 - 201, Update the README text instances that
still state “Max 3 MCP calls total” to the corrected optimization guidance “Max
2 MCP calls total”; locate the occurrences by searching for the exact string
"Max 3 MCP calls total" (and related mention of MCP call budget) and replace
them with "Max 2 MCP calls total" so lines around the numbered steps (the block
containing steps 1–3) and the repeated mention at 204–207 reflect the updated
limit.
src/server.ts
Outdated
| annotateEndpoint, | ||
| } from './tools/tool-variants'; | ||
| import exploreFunctionEndpoint from './tools/explore-function'; | ||
| import findConnectionsEndpoint from './tools/find-connections'; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n 'findConnectionsEndpoint' --type tsRepository: supermodeltools/mcp
Length of output: 143
Remove unused findConnectionsEndpoint import on line 21
This import isn't referenced anywhere in the file. Since the switch statement in setupHandlers() doesn't use it and the PR is already removing the unused find_connections tool, we should clean up the import as well.
Diff
import exploreFunctionEndpoint from './tools/explore-function';
-import findConnectionsEndpoint from './tools/find-connections';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| import findConnectionsEndpoint from './tools/find-connections'; | |
| import exploreFunctionEndpoint from './tools/explore-function'; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/server.ts` at line 21, Remove the unused import findConnectionsEndpoint
from the top-level imports in src/server.ts; locate the import line that reads
"import findConnectionsEndpoint from './tools/find-connections';" and delete it,
and then verify setupHandlers() and any switch/case or references to the
find_connections tool no longer reference that symbol so there are no
unused-import warnings or broken references.
Comment on lines
+10
to
+23
| import { Tool } from '@modelcontextprotocol/sdk/types.js'; | ||
| import { Endpoint, HandlerFunction, asTextContentResult, asErrorResult } from '../types'; | ||
| import { | ||
| IndexedGraph, | ||
| resolveOrFetchGraph, | ||
| normalizePath, | ||
| } from '../cache/graph-cache'; | ||
| import { CodeGraphNode } from '../cache/graph-types'; | ||
| import { findSymbol, renderBriefSymbolContext, renderSymbolContext } from './symbol-context'; | ||
| import { | ||
| MAX_SYMBOL_CALLERS, | ||
| MAX_SYMBOL_CALLEES, | ||
| MAX_BATCH_SYMBOLS, | ||
| } from '../constants'; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that renderSymbolContext and MAX_BATCH_SYMBOLS are not used in tool-variants.ts
echo "=== Checking renderSymbolContext usage in tool-variants.ts ==="
rg 'renderSymbolContext' src/tools/tool-variants.ts
echo "=== Checking MAX_BATCH_SYMBOLS usage in tool-variants.ts ==="
rg 'MAX_BATCH_SYMBOLS' src/tools/tool-variants.ts
echo "=== Also verify the complete file to understand context ==="
wc -l src/tools/tool-variants.tsRepository: supermodeltools/mcp
Length of output: 392
Remove unused imports: renderSymbolContext and MAX_BATCH_SYMBOLS.
These two imports are dead code—they get pulled in but never actually used anywhere in the file. Think of it like ordering ingredients for a recipe but forgetting to use some of them. Cleaner to just remove them.
Remove unused imports
-import { findSymbol, renderBriefSymbolContext, renderSymbolContext } from './symbol-context';
+import { findSymbol, renderBriefSymbolContext } from './symbol-context';
import {
MAX_SYMBOL_CALLERS,
MAX_SYMBOL_CALLEES,
- MAX_BATCH_SYMBOLS,
} from '../constants';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tools/tool-variants.ts` around lines 10 - 23, Remove the unused imports
renderSymbolContext and MAX_BATCH_SYMBOLS: edit the import that currently pulls
renderSymbolContext from './symbol-context' to only import findSymbol and
renderBriefSymbolContext, and edit the import from '../constants' to drop
MAX_BATCH_SYMBOLS while keeping MAX_SYMBOL_CALLERS and MAX_SYMBOL_CALLEES;
verify there are no remaining references to renderSymbolContext or
MAX_BATCH_SYMBOLS in the file (e.g., in functions or constants) before
committing.
- README: document class/method support in explore_function params - README: update GraphRAG workflow to max 2 MCP calls, remove find_connections - server.ts: remove unused findConnectionsEndpoint import - tool-variants.ts: remove unused renderSymbolContext and MAX_BATCH_SYMBOLS imports Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
README.md (1)
201-201:⚠️ Potential issue | 🟡 MinorCall-budget guidance is still inconsistent (
3vs2)Line 201 still says “Max 3 MCP calls total,” while this PR’s optimization guidance is max 2 calls. This is the same mismatch previously flagged and appears partially fixed only for GraphRAG mode.
Quick doc fix
-3. Start editing by turn 3. Max 3 MCP calls total. +3. Start editing by turn 3. Max 2 MCP calls total.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 201, Update the inconsistent call-budget text so the README matches this PR’s optimization guidance: replace the sentence "Start editing by turn 3. Max 3 MCP calls total." with "Start editing by turn 3. Max 2 MCP calls total." and audit any other occurrences (including GraphRAG-related sections) to ensure all references to MCP call limits use "2" rather than "3".
🧹 Nitpick comments (7)
src/tools/explore-function.ts (2)
243-245: Source code fence uses raw file extension instead of language name.
symbol-context.tshas alanguageFromExtension()helper (line 466) that maps.ts→'typescript',.py→'python', etc. Here you're using the raw extension (e.g.,ts,py) as the code fence language. Both work for syntax highlighting in most renderers, so this is cosmetic — but if you want consistency across tools:Optional: reuse languageFromExtension
+import { findSymbol, languageFromExtension } from './symbol-context'; // ... - const ext = filePath.split('.').pop() || ''; - lines.push('```' + ext); + const lang = languageFromExtension(filePath); + lines.push('```' + lang);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/explore-function.ts` around lines 243 - 245, The code fence currently uses the raw extension captured in ext (const ext = filePath.split('.').pop() || '') when pushing lines.push('```' + ext), but we should use the canonical language name returned by the existing helper languageFromExtension; update the block in explore-function.ts to call languageFromExtension(filePath) (or pass ext if the helper accepts it) and use that value when calling lines.push('```' + lang) so the fenced code block uses a proper language token consistent with symbol-context.ts.
100-134:resolveDomainscans the entiredomainIndexon every call.Inside
describeNode, this runs for every BFS neighbor. With depth=3, a function with many callees could trigger hundreds ofresolveDomaincalls, each iterating the fulldomainIndexand doing anameIndexlookup.For most real-world graphs this is probably fine (domains are small), but if you ever hit perf issues, a quick win would be pre-computing a
nodeId → DomainInfomap once before the BFS starts — similar to howsubdomainToParentis built once.Not urgent, just something to keep in mind.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/explore-function.ts` around lines 100 - 134, The resolveDomain function currently iterates graph.domainIndex and uses graph.nameIndex on every call (called repeatedly from describeNode during BFS); precompute a nodeId → DomainInfo map once before the BFS (similar to how subdomainToParent is built) and change resolveDomain/describeNode to use that map; either (A) add a new buildNodeDomainMap(graph, subdomainToParent) that returns Map<string, DomainInfo> and replace per-call logic with a fast lookup, or (B) change resolveDomain signature to accept the precomputed Map<string, DomainInfo> and have the caller pass it in so resolveDomain no longer scans domainIndex or touches nameIndex during traversal.src/server.ts (2)
147-171: Experiment tool selection via switch is clean.One thing worth noting:
'minimal-schema'(line 153) and'minimal-instructions'(line 67) are intentionally different experiments — one swaps the tool definition, the other swaps the instructions text. Someone reading this for the first time might think they should match, so a brief comment could help:Optional: clarify the distinction with a comment
switch (experiment) { - case 'minimal-schema': + case 'minimal-schema': // Different from 'minimal-instructions' — this tests schema only, not instructions allTools = [{ tool: symbolContextMinimalTool, handler: symbolContextTool.handler }]; break;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server.ts` around lines 147 - 171, Add a short clarifying comment near the experiment selection in setupHandlers (where SUPERMODEL_EXPERIMENT is read) that explains 'minimal-schema' (which swaps the tool definition via symbolContextMinimalTool) is intentionally different from 'minimal-instructions' (which only alters instruction text elsewhere); reference the experiment names ('minimal-schema' and 'minimal-instructions') and the setupHandlers function so future readers understand these are distinct experiments and not a typo.
116-118: Server version is hardcoded as'0.0.1'while package is at0.10.0.This is the version reported in the MCP handshake
serverInfo. It might be intentional (MCP server protocol version vs package version), but if clients ever use this for compatibility checks, having it stuck at0.0.1could be misleading.Consider importing the version from
package.jsonor at least bumping this to match:Proposed fix
this.server = new McpServer( { name: 'supermodel_api', - version: '0.0.1', + version: '0.10.0', },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server.ts` around lines 116 - 118, The serverInfo object currently hardcodes version: '0.0.1' which is misleading; update the code that builds the serverInfo (the object with name: 'supermodel_api' and version property in src/server.ts) to read the real package version instead—import or require the package.json version and assign that value to the version field (or at minimum bump the literal to match package.json); ensure the import is referenced where the serverInfo object is constructed so the handshake reports the actual package version.src/tools/tool-variants.ts (2)
64-69: Simplified error handling loses useful error classification.When
resolveOrFetchGraphthrows, the other tools (explore-function.tsline 210,find-connections.tsline 103) useclassifyApiError(error)to give the user specific guidance (e.g., "rate limited — wait 30s", "invalid API key", "request timed out"). Here, all four handlers just pass througherror.messagewith a genericGRAPH_ERRORcode.This means if someone hits a rate limit while using the
search-symbolexperiment, they get"GRAPH_ERROR"instead of"RATE_LIMITED"with a retry suggestion.Since
classifyApiErroris already imported in sibling modules, it's a small change:Use classifyApiError for consistent error handling
+import { classifyApiError } from '../utils/api-helpers';Then in each handler's catch block (lines 67-68, 130-131, 193-194, 283-284):
} catch (error: any) { - return asErrorResult({ type: 'internal_error', message: error.message, code: 'GRAPH_ERROR', recoverable: false }); + return asErrorResult(classifyApiError(error)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/tool-variants.ts` around lines 64 - 69, The catch block around resolveOrFetchGraph currently converts all errors to a generic GRAPH_ERROR; instead call classifyApiError(error) and return its result via asErrorResult so rate limits, invalid keys, timeouts, etc. are preserved—replace the catch body to compute const classified = classifyApiError(error) and return asErrorResult(classified) (keep resolveOrFetchGraph, classifyApiError and asErrorResult references to locate code).
266-298:annotateHandleris functionally identical tosearchSymbolHandler.Both handlers do the same thing: validate a string param, resolve graph,
findSymbol, render up to 3 brief contexts, append "more matches" suffix. The only difference is the parameter name (symbolvsquery).This is fine for an experiment module where you're intentionally testing different framings, so just flagging for awareness — if these experiments consolidate later, this is an easy dedup target.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/tool-variants.ts` around lines 266 - 298, annotateHandler is functionally identical to searchSymbolHandler (both validate a string, call resolveOrFetchGraph, findSymbol, render up to 3 contexts with renderBriefSymbolContext and return via asTextContentResult/asErrorResult), so extract the shared logic into a single helper (e.g., handleSymbolLookup or buildSymbolHandler) that accepts the parameter name ("symbol" vs "query") or the extracted string and returns the same result shape; then have annotateHandler and searchSymbolHandler become thin wrappers that call that helper (reusing resolveOrFetchGraph, findSymbol, renderBriefSymbolContext, asTextContentResult, asErrorResult) to eliminate duplication.src/tools/find-connections.ts (1)
52-70:collectDomainMembersonly collects Function nodes, excluding Class nodes.Over in
explore-function.ts(lines 267, 283, 315, 331), the BFS filter includes both'Function'and'Class'labels. Here, only'Function'passes the filter. So if a Class node bridges two domains,find_connectionswon't find it.This might be intentional since classes don't "call" other functions the same way, but it's worth being consistent with the rest of the GraphRAG suite.
If you want consistency with explore-function.ts
if (node?.labels?.[0] === 'Function') { + members.add(id); + } + if (node?.labels?.[0] === 'Class') { members.add(id); }Or more concisely:
- if (node?.labels?.[0] === 'Function') { + const label = node?.labels?.[0]; + if (label === 'Function' || label === 'Class') { members.add(id); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/find-connections.ts` around lines 52 - 70, collectDomainMembers currently only adds nodes whose node.labels[0] === 'Function', which omits 'Class' nodes and causes inconsistency with explore-function.ts BFS filters; update collectDomainMembers to accept both 'Function' and 'Class' labels when iterating graph.domainIndex/memberIds (check node.labels for either value) so Class nodes are included in the returned Set and downstream find_connections can traverse class bridges just like functions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 182-195: Remove the stale find_connections documentation from
README.md: delete the entire `find_connections` subsection (the
parameters/output table and header) so the README only documents the actual
exposed tool(s). Ensure the docs now reflect the server's real API (remove
references to `find_connections` and only list the available tool(s), e.g.,
`symbol_context`) to avoid documenting a tool that isn't registered.
In `@src/tools/explore-function.ts`:
- Around line 245-253: The Biome lint flags the arrow callbacks in the display
block because the concise arrow returns a value (lines.push) — change the
forEach usages to avoid returning values: when startLine is truthy iterate with
for (const [i, l] of displayLines.entries()) and call lines.push(`${startLine +
i}: ${l}`) inside the loop; when startLine is falsy replace the
displayLines.forEach(...) with a single spread push like
lines.push(...displayLines). Keep the surrounding markers (the triple backtick,
truncated check, and closing push calls) unchanged and reference variables
displayLines, lines, startLine, truncated, sourceLines, and MAX_SOURCE_LINES
when making the edits.
In `@src/tools/find-connections.ts`:
- Around line 116-155: The bridge collection can emit duplicate entries when a
node appears in both aNodes and bNodes (collectDomainMembers does fuzzy
matching); deduplicate by using a Set instead of the bridges array (or filter
before pushing) in the two loops that iterate over graph.callAdj for aId and
bId: replace pushes into bridges with adds to a Set (e.g., bridgeSet.add(...))
and then convert to an array at the end so each unique string is only emitted
once; keep the same string formatting logic referencing srcNode/tgtNode,
domainA/domainB, and normalizePath.
---
Duplicate comments:
In `@README.md`:
- Line 201: Update the inconsistent call-budget text so the README matches this
PR’s optimization guidance: replace the sentence "Start editing by turn 3. Max 3
MCP calls total." with "Start editing by turn 3. Max 2 MCP calls total." and
audit any other occurrences (including GraphRAG-related sections) to ensure all
references to MCP call limits use "2" rather than "3".
---
Nitpick comments:
In `@src/server.ts`:
- Around line 147-171: Add a short clarifying comment near the experiment
selection in setupHandlers (where SUPERMODEL_EXPERIMENT is read) that explains
'minimal-schema' (which swaps the tool definition via symbolContextMinimalTool)
is intentionally different from 'minimal-instructions' (which only alters
instruction text elsewhere); reference the experiment names ('minimal-schema'
and 'minimal-instructions') and the setupHandlers function so future readers
understand these are distinct experiments and not a typo.
- Around line 116-118: The serverInfo object currently hardcodes version:
'0.0.1' which is misleading; update the code that builds the serverInfo (the
object with name: 'supermodel_api' and version property in src/server.ts) to
read the real package version instead—import or require the package.json version
and assign that value to the version field (or at minimum bump the literal to
match package.json); ensure the import is referenced where the serverInfo object
is constructed so the handshake reports the actual package version.
In `@src/tools/explore-function.ts`:
- Around line 243-245: The code fence currently uses the raw extension captured
in ext (const ext = filePath.split('.').pop() || '') when pushing
lines.push('```' + ext), but we should use the canonical language name returned
by the existing helper languageFromExtension; update the block in
explore-function.ts to call languageFromExtension(filePath) (or pass ext if the
helper accepts it) and use that value when calling lines.push('```' + lang) so
the fenced code block uses a proper language token consistent with
symbol-context.ts.
- Around line 100-134: The resolveDomain function currently iterates
graph.domainIndex and uses graph.nameIndex on every call (called repeatedly from
describeNode during BFS); precompute a nodeId → DomainInfo map once before the
BFS (similar to how subdomainToParent is built) and change
resolveDomain/describeNode to use that map; either (A) add a new
buildNodeDomainMap(graph, subdomainToParent) that returns Map<string,
DomainInfo> and replace per-call logic with a fast lookup, or (B) change
resolveDomain signature to accept the precomputed Map<string, DomainInfo> and
have the caller pass it in so resolveDomain no longer scans domainIndex or
touches nameIndex during traversal.
In `@src/tools/find-connections.ts`:
- Around line 52-70: collectDomainMembers currently only adds nodes whose
node.labels[0] === 'Function', which omits 'Class' nodes and causes
inconsistency with explore-function.ts BFS filters; update collectDomainMembers
to accept both 'Function' and 'Class' labels when iterating
graph.domainIndex/memberIds (check node.labels for either value) so Class nodes
are included in the returned Set and downstream find_connections can traverse
class bridges just like functions.
In `@src/tools/tool-variants.ts`:
- Around line 64-69: The catch block around resolveOrFetchGraph currently
converts all errors to a generic GRAPH_ERROR; instead call
classifyApiError(error) and return its result via asErrorResult so rate limits,
invalid keys, timeouts, etc. are preserved—replace the catch body to compute
const classified = classifyApiError(error) and return asErrorResult(classified)
(keep resolveOrFetchGraph, classifyApiError and asErrorResult references to
locate code).
- Around line 266-298: annotateHandler is functionally identical to
searchSymbolHandler (both validate a string, call resolveOrFetchGraph,
findSymbol, render up to 3 contexts with renderBriefSymbolContext and return via
asTextContentResult/asErrorResult), so extract the shared logic into a single
helper (e.g., handleSymbolLookup or buildSymbolHandler) that accepts the
parameter name ("symbol" vs "query") or the extracted string and returns the
same result shape; then have annotateHandler and searchSymbolHandler become thin
wrappers that call that helper (reusing resolveOrFetchGraph, findSymbol,
renderBriefSymbolContext, asTextContentResult, asErrorResult) to eliminate
duplication.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
.gitignoreREADME.mdpackage.jsonsrc/server.integration.test.tssrc/server.tssrc/tools/explore-function.tssrc/tools/find-connections.tssrc/tools/symbol-context.tssrc/tools/tool-variants.ts
Comment on lines
+116
to
+155
| // Find call edges between domain A and domain B in both directions | ||
| const bridges: string[] = []; | ||
|
|
||
| for (const aId of aNodes) { | ||
| const adj = graph.callAdj.get(aId); | ||
| if (!adj) continue; | ||
|
|
||
| // A calls B (downstream) | ||
| for (const targetId of adj.out) { | ||
| if (!bNodes.has(targetId)) continue; | ||
| const srcNode = graph.nodeById.get(aId); | ||
| const tgtNode = graph.nodeById.get(targetId); | ||
| const srcName = srcNode?.properties?.name as string || '(unknown)'; | ||
| const tgtName = tgtNode?.properties?.name as string || '(unknown)'; | ||
| const srcFile = normalizePath(srcNode?.properties?.filePath as string || ''); | ||
| const tgtFile = normalizePath(tgtNode?.properties?.filePath as string || ''); | ||
| bridges.push( | ||
| `\`${srcName}\` (${domainA}) calls \`${tgtName}\` (${domainB}) — ${srcFile} → ${tgtFile}` | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| for (const bId of bNodes) { | ||
| const adj = graph.callAdj.get(bId); | ||
| if (!adj) continue; | ||
|
|
||
| // B calls A (reverse direction) | ||
| for (const targetId of adj.out) { | ||
| if (!aNodes.has(targetId)) continue; | ||
| const srcNode = graph.nodeById.get(bId); | ||
| const tgtNode = graph.nodeById.get(targetId); | ||
| const srcName = srcNode?.properties?.name as string || '(unknown)'; | ||
| const tgtName = tgtNode?.properties?.name as string || '(unknown)'; | ||
| const srcFile = normalizePath(srcNode?.properties?.filePath as string || ''); | ||
| const tgtFile = normalizePath(tgtNode?.properties?.filePath as string || ''); | ||
| bridges.push( | ||
| `\`${srcName}\` (${domainB}) calls \`${tgtName}\` (${domainA}) — ${srcFile} → ${tgtFile}` | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential duplicate bridge entries when domains overlap.
Because collectDomainMembers uses fuzzy (substring) matching, it's possible for a node to end up in both aNodes and bNodes. Consider two domains "auth" and "auth-tokens" — a query for "auth" would match both.
If nodeX is in both sets and calls nodeY which is also in both sets, the edge X → Y gets pushed into bridges twice: once in the A→B loop (line 124) and once in the B→A loop (line 143).
A simple fix is to deduplicate with a Set:
Proposed fix — deduplicate bridges
// Find call edges between domain A and domain B in both directions
- const bridges: string[] = [];
+ const bridgeSet = new Set<string>();
for (const aId of aNodes) {
const adj = graph.callAdj.get(aId);
@@ // ... inside first loop:
- bridges.push(
- `\`${srcName}\` (${domainA}) calls \`${tgtName}\` (${domainB}) — ${srcFile} → ${tgtFile}`
- );
+ bridgeSet.add(
+ `\`${srcName}\` (${domainA}) calls \`${tgtName}\` (${domainB}) — ${srcFile} → ${tgtFile}`
+ );
// ... same for second loop, then:
+ const bridges = [...bridgeSet];
if (bridges.length === 0) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Find call edges between domain A and domain B in both directions | |
| const bridges: string[] = []; | |
| for (const aId of aNodes) { | |
| const adj = graph.callAdj.get(aId); | |
| if (!adj) continue; | |
| // A calls B (downstream) | |
| for (const targetId of adj.out) { | |
| if (!bNodes.has(targetId)) continue; | |
| const srcNode = graph.nodeById.get(aId); | |
| const tgtNode = graph.nodeById.get(targetId); | |
| const srcName = srcNode?.properties?.name as string || '(unknown)'; | |
| const tgtName = tgtNode?.properties?.name as string || '(unknown)'; | |
| const srcFile = normalizePath(srcNode?.properties?.filePath as string || ''); | |
| const tgtFile = normalizePath(tgtNode?.properties?.filePath as string || ''); | |
| bridges.push( | |
| `\`${srcName}\` (${domainA}) calls \`${tgtName}\` (${domainB}) — ${srcFile} → ${tgtFile}` | |
| ); | |
| } | |
| } | |
| for (const bId of bNodes) { | |
| const adj = graph.callAdj.get(bId); | |
| if (!adj) continue; | |
| // B calls A (reverse direction) | |
| for (const targetId of adj.out) { | |
| if (!aNodes.has(targetId)) continue; | |
| const srcNode = graph.nodeById.get(bId); | |
| const tgtNode = graph.nodeById.get(targetId); | |
| const srcName = srcNode?.properties?.name as string || '(unknown)'; | |
| const tgtName = tgtNode?.properties?.name as string || '(unknown)'; | |
| const srcFile = normalizePath(srcNode?.properties?.filePath as string || ''); | |
| const tgtFile = normalizePath(tgtNode?.properties?.filePath as string || ''); | |
| bridges.push( | |
| `\`${srcName}\` (${domainB}) calls \`${tgtName}\` (${domainA}) — ${srcFile} → ${tgtFile}` | |
| ); | |
| } | |
| } | |
| // Find call edges between domain A and domain B in both directions | |
| const bridgeSet = new Set<string>(); | |
| for (const aId of aNodes) { | |
| const adj = graph.callAdj.get(aId); | |
| if (!adj) continue; | |
| // A calls B (downstream) | |
| for (const targetId of adj.out) { | |
| if (!bNodes.has(targetId)) continue; | |
| const srcNode = graph.nodeById.get(aId); | |
| const tgtNode = graph.nodeById.get(targetId); | |
| const srcName = srcNode?.properties?.name as string || '(unknown)'; | |
| const tgtName = tgtNode?.properties?.name as string || '(unknown)'; | |
| const srcFile = normalizePath(srcNode?.properties?.filePath as string || ''); | |
| const tgtFile = normalizePath(tgtNode?.properties?.filePath as string || ''); | |
| bridgeSet.add( | |
| `\`${srcName}\` (${domainA}) calls \`${tgtName}\` (${domainB}) — ${srcFile} → ${tgtFile}` | |
| ); | |
| } | |
| } | |
| for (const bId of bNodes) { | |
| const adj = graph.callAdj.get(bId); | |
| if (!adj) continue; | |
| // B calls A (reverse direction) | |
| for (const targetId of adj.out) { | |
| if (!aNodes.has(targetId)) continue; | |
| const srcNode = graph.nodeById.get(bId); | |
| const tgtNode = graph.nodeById.get(targetId); | |
| const srcName = srcNode?.properties?.name as string || '(unknown)'; | |
| const tgtName = tgtNode?.properties?.name as string || '(unknown)'; | |
| const srcFile = normalizePath(srcNode?.properties?.filePath as string || ''); | |
| const tgtFile = normalizePath(tgtNode?.properties?.filePath as string || ''); | |
| bridgeSet.add( | |
| `\`${srcName}\` (${domainB}) calls \`${tgtName}\` (${domainA}) — ${srcFile} → ${tgtFile}` | |
| ); | |
| } | |
| } | |
| const bridges = [...bridgeSet]; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tools/find-connections.ts` around lines 116 - 155, The bridge collection
can emit duplicate entries when a node appears in both aNodes and bNodes
(collectDomainMembers does fuzzy matching); deduplicate by using a Set instead
of the bridges array (or filter before pushing) in the two loops that iterate
over graph.callAdj for aId and bId: replace pushes into bridges with adds to a
Set (e.g., bridgeSet.add(...)) and then convert to an array at the end so each
unique string is only emitted once; keep the same string formatting logic
referencing srcNode/tgtNode, domainA/domainB, and normalizePath.
- Remove find_connections documentation from README (tool removed from graphrag mode) - Fix forEach implicit return values in explore-function.ts (Biome lint) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Optimizations to GraphRAG tools based on SWE-bench log analysis + CodeRabbit fixes from #133.
Changes
Performance optimizations (c4031da):
find_connections— never used by agent in any benchmark runexplore_function— fixes SYMBOL_NOT_FOUND for class lookupsexplore_functionoutput — saves agent a Read round-tripBug fixes from CodeRabbit review (82d8b12):
directionparameter — reject invalid values with error instead of silent empty output1., now sequential1. 2. 3.createServerFixture(), eliminate ~90 lines duplicationthrowin EventEmitter callback, moveinitializetobeforeAllBenchmark Results (n=10, SWE-bench Verified)
High variance at n=10. Run 2 regressions had specific causes:
astropy-13579: MCP server precache ate into 300s timeoutastropy-13033: non-deterministic (PASS in run 1, FAIL in run 2)Test plan
npm run build— cleannpm run typecheck— cleannpm test— 71/71 pass🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests
Chores