From 3c667b0ca320b7be2c7745f48c510bf6d109be06 Mon Sep 17 00:00:00 2001 From: Dan Roujinsky Date: Fri, 11 Sep 2026 16:03:28 +0300 Subject: [PATCH] feat(graph): resolve Python imports and add a cycles command Add Python import-edge resolution to the graph and a new `graft cycles` command (plus a `graft_find_import_cycles` MCP tool) that reports every resolved file-to-file import cycle. Python imports: - Resolve absolute and relative (dotted) module specifiers to in-repo file nodes, honoring package `__init__` modules and disambiguating ambiguous suffixes by source root. - Flag lazy edges: imports inside functions/classes or under `if TYPE_CHECKING:` are marked `lazy` and carry their import line, so cycle reports can distinguish breakable edges from eager ones. - Edges gain optional `lazy`/`line` metadata; eager occurrences win over lazy ones on dedup. Cycles: - `findImportCycles` / `formatImportCycles` detect and render file-level import cycles across all languages, marking lazy edges and import lines. - Exposed as the `graft cycles` CLI command (workspace-aware) and the `graft_find_import_cycles` MCP tool, added to the MCP instructions within the existing <1000-char budget by trimming the prose. - `cycles` added to the tracked telemetry commands. Tests cover Python import extraction and resolution, cycle detection, the CLI command, the MCP tool/advertising, and the telemetry contract. --- TELEMETRY.md | 2 +- src/cli.ts | 31 ++++- src/graph/cycles.ts | 116 ++++++++++++++++++ src/graph/extract.ts | 37 +++++- src/graph/resolve.ts | 119 +++++++++++++++--- src/graph/types.ts | 2 + src/graph/workspace-cli.ts | 17 +++ src/mcp/instructions.ts | 15 +-- src/mcp/server.ts | 1 + src/mcp/tools.ts | 12 ++ src/telemetry/contract.ts | 2 +- test/graph-cycles-cli.test.ts | 94 +++++++++++++++ test/graph-cycles.test.ts | 118 ++++++++++++++++++ test/graph-python-imports.test.ts | 118 ++++++++++++++++++ test/graph-python-resolve.test.ts | 192 ++++++++++++++++++++++++++++++ test/mcp-server.test.ts | 3 +- test/mcp-tools.test.ts | 10 +- test/telemetry-contract.test.ts | 1 + 18 files changed, 859 insertions(+), 31 deletions(-) create mode 100644 src/graph/cycles.ts create mode 100644 test/graph-cycles-cli.test.ts create mode 100644 test/graph-cycles.test.ts create mode 100644 test/graph-python-imports.test.ts create mode 100644 test/graph-python-resolve.test.ts diff --git a/TELEMETRY.md b/TELEMETRY.md index dcb57d75e..b40b12042 100644 --- a/TELEMETRY.md +++ b/TELEMETRY.md @@ -54,7 +54,7 @@ review: - **Every number is a bucket.** A build reports `"200-999"` files, never `417`. An exact count next to a language set starts to fingerprint a specific repo; a bucket does not. -- **Every string is a member of a fixed set.** `command` is one of eight known +- **Every string is a member of a fixed set.** `command` is one of nine known subcommands, `code` is one of eleven known failure codes. A value outside the set is dropped, not sent — which is what stops a path, a symbol name, or a snippet of an error message from riding along inside a "string property". diff --git a/src/cli.ts b/src/cli.ts index 12130c774..ef906ede4 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -1,6 +1,6 @@ #!/usr/bin/env node /** - * `graft` CLI. Commands: build, ask, check, viz, mcp, callers, skeleton, grep, + * `graft` CLI. Commands: build, ask, check, viz, mcp, callers, cycles, skeleton, grep, * map, init. Git is the sync: commit graft/ and a clone has the graph. A * workspace parent (≥2 git children) federates query commands across children. */ @@ -29,6 +29,7 @@ import { runWorkspaceBuild, runWorkspaceCallers, runWorkspaceCheck, + runWorkspaceCycles, runWorkspaceGrep, runWorkspaceMap, } from "./graph/workspace-cli.js"; @@ -747,7 +748,7 @@ program program .command("mcp") - .description("Serve the graph over MCP (stdio) — exposes graft_find_code, graft_trace_calls, graft_find_all, graft_file_api, graft_repo_map and graft_check_freshness as tools") + .description("Serve the graph over MCP (stdio) — exposes code search, call tracing, import cycles, file APIs, repo maps and freshness checks as tools") .argument(...DIR_ARG) .action(async (dirArg: string | undefined) => { const dir = noteQuery(queryRoot(dirArg)); @@ -756,6 +757,32 @@ program startMcpServer(dir, globalOpts.dir, currentVersion); }); +program + .command("cycles") + .description("Find resolved file-to-file import cycles, with lazy edges and import lines ($0, no LLM)") + .argument(...DIR_ARG) + .option(...NO_REFRESH_FLAG) + .action(async (dirArg: string | undefined, opts: { refresh?: boolean }) => { + const dir = noteQuery(queryRoot(dirArg)); + await refreshBefore(dir, opts); + const root = resolve(dir); + const globalOpts = program.opts<{ dir?: string }>(); + if (readWorkspace(root, globalOpts.dir)) { + noteHit(runWorkspaceCycles(root, globalOpts.dir) > 0); + return; + } + const graph = loadGraphCached(contextDirFor(root, globalOpts.dir)); + if (!graph) { + console.error("✗ no graph — run graft build first"); + process.exit(1); + return; + } + const { findImportCycles, formatImportCycles } = await import("./graph/cycles.js"); + const cycles = findImportCycles(graph); + noteHit(cycles.length > 0); + process.stdout.write(formatImportCycles(cycles)); + }); + program .command("callers") .description( diff --git a/src/graph/cycles.ts b/src/graph/cycles.ts new file mode 100644 index 000000000..4b2fc9262 --- /dev/null +++ b/src/graph/cycles.ts @@ -0,0 +1,116 @@ +import type { EdgeV1, GraphV1 } from "./types.js"; + +export interface ImportCycle { + files: string[]; + edges: EdgeV1[]; +} + +export function formatImportCycles(cycles: ImportCycle[]): string { + if (cycles.length === 0) return "no import cycles found\n"; + return ( + cycles + .map((cycle, index) => { + const lines = [`Cycle ${index + 1} · ${cycle.files.length} files`]; + for (const edge of cycle.edges) { + const source = edge.line === undefined ? edge.source : `${edge.source}:${edge.line}`; + lines.push(` ${source} imports → ${edge.target}${edge.lazy ? " [lazy]" : ""}`); + } + return lines.join("\n"); + }) + .join("\n\n") + "\n" + ); +} + +/** Find every multi-file strongly connected component in the resolved import graph. */ +export function findImportCycles(graph: GraphV1): ImportCycle[] { + const files = graph.nodes.filter((node) => node.kind === "file").sort((a, b) => a.id.localeCompare(b.id)); + const fileIds = new Set(files.map((file) => file.id)); + const adjacency = new Map(files.map((file) => [file.id, [] as string[]])); + const importsBySource = new Map(); + for (const edge of graph.edges) { + if (edge.relation !== "imports" || !fileIds.has(edge.source) || !fileIds.has(edge.target)) continue; + adjacency.get(edge.source)!.push(edge.target); + const sourceEdges = importsBySource.get(edge.source); + if (sourceEdges) sourceEdges.push(edge); + else importsBySource.set(edge.source, [edge]); + } + for (const neighbours of adjacency.values()) neighbours.sort((a, b) => a.localeCompare(b)); + + let nextIndex = 0; + const index = new Map(); + const lowlink = new Map(); + const stack: string[] = []; + const onStack = new Set(); + const components: string[][] = []; + + const enter = (id: string): void => { + index.set(id, nextIndex); + lowlink.set(id, nextIndex); + nextIndex++; + stack.push(id); + onStack.add(id); + }; + + for (const file of files) { + if (index.has(file.id)) continue; + enter(file.id); + const work: Array<{ id: string; parent?: string; nextNeighbour: number }> = [ + { id: file.id, nextNeighbour: 0 }, + ]; + + while (work.length > 0) { + const frame = work.at(-1)!; + const neighbours = adjacency.get(frame.id)!; + if (frame.nextNeighbour < neighbours.length) { + const neighbour = neighbours[frame.nextNeighbour++]; + if (!index.has(neighbour)) { + enter(neighbour); + work.push({ id: neighbour, parent: frame.id, nextNeighbour: 0 }); + } else if (onStack.has(neighbour)) { + lowlink.set(frame.id, Math.min(lowlink.get(frame.id)!, index.get(neighbour)!)); + } + continue; + } + + work.pop(); + if (frame.parent !== undefined) { + lowlink.set(frame.parent, Math.min(lowlink.get(frame.parent)!, lowlink.get(frame.id)!)); + } + if (lowlink.get(frame.id) !== index.get(frame.id)) continue; + + const component: string[] = []; + let member: string; + do { + member = stack.pop()!; + onStack.delete(member); + component.push(member); + } while (member !== frame.id); + if (component.length > 1) components.push(component.sort((a, b) => a.localeCompare(b))); + } + } + + return components + .sort(compareMembers) + .map((members) => ({ + files: members, + edges: cycleEdges(importsBySource, members), + })); +} + +function compareMembers(a: string[], b: string[]): number { + return a.join("\0").localeCompare(b.join("\0")); +} + +function cycleEdges(importsBySource: Map, members: string[]): EdgeV1[] { + const memberSet = new Set(members); + return members + .flatMap((source) => importsBySource.get(source) ?? []) + .filter((edge) => memberSet.has(edge.target)) + .sort( + (a, b) => + a.source.localeCompare(b.source) || + a.target.localeCompare(b.target) || + (a.line ?? 0) - (b.line ?? 0) || + Number(a.lazy ?? false) - Number(b.lazy ?? false), + ); +} diff --git a/src/graph/extract.ts b/src/graph/extract.ts index d0f9dff19..f716ac635 100644 --- a/src/graph/extract.ts +++ b/src/graph/extract.ts @@ -93,6 +93,8 @@ export interface RawEdge { file: string; // the file this edge originates in (scopes name resolution) targetId?: string; // already-resolved target (contains) specifier?: string; // module path to resolve (imports / imported-symbol references) + lazy?: true; // present-only-when-true keeps eager imports byte-compatible + line?: number; // 1-based to match graph spans and editor coordinates name?: string; // symbol name to resolve (extends/implements/calls) viaMember?: boolean; // calls: was it `obj.foo()` (→ prefer method targets)? /** calls with viaMember: the receiver's resolved type name (from bindings / @@ -335,6 +337,7 @@ export interface WalkCtx { enclosingClass: string | null; // nearest enclosing class (py/ts `self`/`this`) goReceiverVar: string | null; // Go receiver var, e.g. `w` in `func (w *Worker)` importedSymbols: ReadonlyMap; + typeCheckingOnly: boolean; // R6 (Phase 2): which list we're inside while walking an `R6Class(...)` call's // arguments — set only for the direct span of a `public =`/`private =`/ // `active =` `list(...)`'s own entries (see walk()'s special-cased `argument` @@ -422,6 +425,7 @@ export function extractFile(rel: string, source: string, lang: Language): Extrac enclosingClass: null, goReceiverVar: null, importedSymbols, + typeCheckingOnly: false, rR6Access: null, rGenerics, rSuperClass: null, @@ -698,6 +702,28 @@ function walk(node: Parser.SyntaxNode, ctx: WalkCtx, out: NodeV1[], edges: RawEd } } + if (ctx.lang === "python" && ctx.enclosingKind === null && node.type === "if_statement") { + const condition = node.childForFieldName("condition"); + const consequence = node.childForFieldName("consequence"); + const isTypeChecking = + (condition?.type === "identifier" && condition.text === "TYPE_CHECKING") || + (condition?.type === "attribute" && + condition.childForFieldName("object")?.text === "typing" && + condition.childForFieldName("attribute")?.text === "TYPE_CHECKING"); + if (isTypeChecking && consequence) { + for (const child of node.namedChildren) { + walk( + child, + sameSyntaxNode(child, consequence) ? { ...ctx, typeCheckingOnly: true } : ctx, + out, + edges, + minted, + ); + } + return; + } + } + // not a definition — capture calls/imports/references, then descend with the same context // R's `call` node is also its ONLY vehicle for library()/require()/source() — // there's no separate import-statement grammar construct to key off, so isImport @@ -706,7 +732,16 @@ function walk(node: Parser.SyntaxNode, ctx: WalkCtx, out: NodeV1[], edges: RawEd const callTypes = CALL_TYPES[ctx.lang]; if (isImport(node, ctx.lang)) { const spec = importSpecifier(node, ctx.lang); - if (spec) edges.push({ source: ctx.rel, relation: "imports", specifier: spec, file: ctx.rel }); + if (spec) { + edges.push({ + source: ctx.rel, + relation: "imports", + specifier: spec, + file: ctx.rel, + line: node.startPosition.row + 1, + ...(ctx.enclosingKind !== null || ctx.typeCheckingOnly ? { lazy: true as const } : {}), + }); + } // Imported identifiers are declarations, not uses. The import-binding pass // above already recorded them, so do not descend and emit false references. return; diff --git a/src/graph/resolve.ts b/src/graph/resolve.ts index c78a61e6c..f370da4de 100644 --- a/src/graph/resolve.ts +++ b/src/graph/resolve.ts @@ -116,6 +116,9 @@ export function resolveEdges( // language convention mirrors the directory path under whatever source root the // project uses (`src/main/java/`, `src/`, …) — so the suffix is the portable key. const javaFilesBySuffix = new Map(); + // Python's import path omits both the source root and file extension, so its + // suffix key is the extensionless module path; package dirs also name __init__. + const pythonFilesBySuffix = new Map(); // C/C++ header resolution: a file's path-suffix (`net/socket.h`, `socket.h`) → its // file node ids, so an `#include` reached through an `-I` dir (not relative to the // including file) still resolves to the in-repo header when the suffix is unique. @@ -142,6 +145,22 @@ export function resolveEdges( const parts = toPosixPath(n.path).split("/"); for (let i = 0; i < parts.length; i++) push(javaFilesBySuffix, parts.slice(i).join("/"), n.id); } + if (PY_EXT.test(n.path)) { + const modulePath = toPosixPath(n.path).replace(/\.pyi?$/i, ""); + const modulePaths = [modulePath]; + if (modulePath.endsWith("/__init__") || modulePath === "__init__") { + modulePaths.push(posix.dirname(modulePath)); + } + for (const candidatePath of modulePaths) { + const parts = candidatePath === "." ? [] : candidatePath.split("/"); + for (let i = 0; i < parts.length; i++) { + push(pythonFilesBySuffix, parts.slice(i).join("/"), { + id: n.id, + root: parts.slice(0, i).join("/"), + }); + } + } + } if (C_EXT.test(n.path)) { const parts = toPosixPath(n.path).split("/"); for (let i = 0; i < parts.length; i++) push(cFilesBySuffix, parts.slice(i).join("/"), n.id); @@ -193,12 +212,31 @@ export function resolveEdges( } const out: EdgeV1[] = []; - const seen = new Set(); - const add = (source: string, target: string, relation: Relation, confidence: EdgeV1["confidence"]) => { + const edgeIndexByKey = new Map(); + const add = ( + source: string, + target: string, + relation: Relation, + confidence: EdgeV1["confidence"], + metadata: Pick = {}, + ) => { const key = `${source}\0${relation}\0${target}`; - if (seen.has(key)) return; - seen.add(key); - out.push({ source, target, relation, confidence }); + const edge: EdgeV1 = { + source, + target, + relation, + confidence, + ...(metadata.lazy ? { lazy: true as const } : {}), + ...(metadata.line === undefined ? {} : { line: metadata.line }), + }; + const existingIndex = edgeIndexByKey.get(key); + if (existingIndex !== undefined) { + // Import timing is semantic: an eager occurrence must win regardless of arrival order. + if (relation === "imports" && out[existingIndex].lazy && !edge.lazy) out[existingIndex] = edge; + return; + } + edgeIndexByKey.set(key, out.length); + out.push(edge); }; for (const e of rawEdges) { @@ -208,16 +246,18 @@ export function resolveEdges( const target = hasGoModules && e.file.endsWith(".go") ? resolveGoImport(e.specifier, opts.goModules!, goFilesByDir) - : e.file.endsWith(".java") - ? resolveJavaImport(e.specifier, javaFilesBySuffix) - : C_EXT.test(e.file) - ? resolveCInclude(e.specifier, e.file, byId, cFilesBySuffix) - : e.file.endsWith(".rs") - ? resolveRustUse(e.specifier, e.file, byId, rustCrateRoots) - : e.file.endsWith(".php") - ? resolvePhpUse(e.specifier, phpFilesBySuffix) - : resolveImport(e.specifier, e.file, byId); - add(e.source, target, "imports", "extracted"); + : PY_EXT.test(e.file) + ? resolvePythonImport(e.specifier, e.file, byId, pythonFilesBySuffix) + : e.file.endsWith(".java") + ? resolveJavaImport(e.specifier, javaFilesBySuffix) + : C_EXT.test(e.file) + ? resolveCInclude(e.specifier, e.file, byId, cFilesBySuffix) + : e.file.endsWith(".rs") + ? resolveRustUse(e.specifier, e.file, byId, rustCrateRoots) + : e.file.endsWith(".php") + ? resolvePhpUse(e.specifier, phpFilesBySuffix) + : resolveImport(e.specifier, e.file, byId); + add(e.source, target, "imports", "extracted", { lazy: e.lazy, line: e.line }); } else if (e.relation === "extends" || e.relation === "implements") { // `implements` also resolves to a `trait` — PHP models trait composition // (`use SomeTrait;`) as an implements edge, and a trait is a valid target. @@ -507,6 +547,55 @@ function resolveImport(spec: string, file: string, byId: Map): s return spec; } +/** Python source roots are not declared uniformly, so only a unique dotted + * module-path suffix is certain; a bare name may still be stdlib/third-party. + * A missing full module may name an attribute on its parent; retry that parent + * once, but never fall through an ambiguous full suffix. */ +interface PythonImportCandidate { + id: string; + root: string; +} + +function resolvePythonImport( + spec: string, + file: string, + byId: Map, + filesBySuffix: Map, +): string { + if (spec.startsWith(".")) { + const leadingDots = spec.match(/^\.+/)![0].length; + const packageParts = posix.dirname(toPosixPath(file)).split("/").filter((part) => part !== "."); + const parentHops = leadingDots - 1; + if (parentHops >= packageParts.length) return spec; + const moduleParts = spec.slice(leadingDots).split(".").filter(Boolean); + const modulePath = [...packageParts.slice(0, packageParts.length - parentHops), ...moduleParts].join("/"); + const candidates = [ + `${modulePath}.py`, + `${modulePath}.pyi`, + `${modulePath}/__init__.py`, + `${modulePath}/__init__.pyi`, + ].filter((candidate) => byId.has(candidate)); + return candidates.length === 1 ? candidates[0] : spec; + } + const hit = (module: string): string | "ambiguous" | null => { + const files = filesBySuffix.get(module.split(".").join("/")); + if (!files || files.length === 0) return null; + if (files.length === 1 && module.includes(".")) return files[0].id; + const local = files.filter(({ root }) => root === "" || file.startsWith(`${root}/`)); + return local.length === 1 ? local[0].id : "ambiguous"; + }; + + const direct = hit(spec); + if (direct === "ambiguous") return spec; + if (direct) return direct; + const dot = spec.lastIndexOf("."); + if (dot > 0) { + const enclosing = hit(spec.slice(0, dot)); + if (enclosing && enclosing !== "ambiguous") return enclosing; + } + return spec; +} + /** * Resolve a Java import's fully-qualified type name to an in-repo file node; * otherwise return the raw specifier (JDK or third-party type). diff --git a/src/graph/types.ts b/src/graph/types.ts index ff243e159..679011dbd 100644 --- a/src/graph/types.ts +++ b/src/graph/types.ts @@ -105,6 +105,8 @@ export interface EdgeV1 { target: string; // node id, or an unresolved module string for imports relation: Relation; confidence: Confidence; + lazy?: true; // present-only-when-true keeps old graphs and eager edges compatible + line?: number; // 1-based to match node spans and editor coordinates } /** A ranking scope: a sub-project discovered by project-marker files (`package.json`, diff --git a/src/graph/workspace-cli.ts b/src/graph/workspace-cli.ts index f6dc57e87..6b88d8241 100644 --- a/src/graph/workspace-cli.ts +++ b/src/graph/workspace-cli.ts @@ -11,13 +11,16 @@ import { patchBuildConfig, type BuildConfig } from "../util/state.js"; import type { EngineConfig } from "../ai/providers.js"; import { formatAsk } from "../ask/ask.js"; import type { Direction } from "./traverse.js"; +import { findImportCycles, formatImportCycles } from "./cycles.js"; import { + coverageNote, federateAsk, federateCallers, federateCheck, federateGrep, federateMap, formatGrepResult, + loadWorkspaceGraphs, migrationNote, splitWorkspace, zeroHitNote, @@ -129,6 +132,20 @@ export async function runWorkspaceCheck(root: string, override?: string): Promis if (!ok) process.exit(1); } +export function runWorkspaceCycles(root: string, override?: string): number { + const wg = loadWorkspaceGraphs(root, override); + let cycleCount = 0; + const sections = wg.loaded.map(({ child, graph }) => { + const cycles = findImportCycles(graph); + cycleCount += cycles.length; + return `## ${child}/\n${formatImportCycles(cycles).trimEnd()}`; + }); + const coverage = coverageNote(wg); + if (coverage) sections.push(coverage); + if (sections.length > 0) process.stdout.write(sections.join("\n\n") + "\n"); + return cycleCount; +} + export function runWorkspaceCallers( root: string, override: string | undefined, diff --git a/src/mcp/instructions.ts b/src/mcp/instructions.ts index 2375b7754..ad21e1310 100644 --- a/src/mcp/instructions.ts +++ b/src/mcp/instructions.ts @@ -4,15 +4,15 @@ * This is the one piece of graft prose that survives **tool deferral**. When a * host has more tools than its schema budget allows, it sends tool *names* only * and withholds the JSONSchemas until a `ToolSearch`-style lookup fetches them — - * measured in a real session: 111 tools deferred, of which graft's six arrived as - * six bare strings with no descriptions at all. The MCP spec's `instructions` + * measured in a real session: 111 tools deferred, of which graft's tools arrived as + * bare strings with no descriptions at all. The MCP spec's `instructions` * field is delivered on a separate track (Claude Code records it as its own * `mcp_instructions_delta` context layer), so it lands whole even then. Other * servers already rely on this — `claude-in-chrome` uses it for exactly the * batch-your-ToolSearch instruction below; graft used to send nothing. * * Two jobs, in this order: - * 1. Defuse the deferral tax. One lookup loads all six tools for the whole + * 1. Defuse the deferral tax. One lookup loads every query tool for the whole * session, so the cost is a single round trip, not two calls per use. An * agent that doesn't know this can only assume the worse reading. * 2. Say what each tool is FOR as a decision rule, not a feature summary — @@ -29,6 +29,7 @@ const TOOL_ORDER = [ 'graft_find_code', 'graft_find_all', 'graft_trace_calls', + 'graft_find_import_cycles', 'graft_file_api', 'graft_repo_map', ] as const; @@ -40,15 +41,15 @@ export function toolSearchQuery(prefix = 'mcp__graft__'): string { export function mcpInstructions(): string { return [ - 'This repo is indexed by graft: a prebuilt graph of every symbol, its file:line', - 'span, and who calls what. Prefer these tools over grep/read — one call usually', - 'replaces several file reads.', + 'graft indexes every symbol, file:line span, and dependency.', + 'Prefer it over grep/read; one call usually replaces several file reads.', '', - `**If these tools are deferred (names shown, schemas withheld), load them all in ONE lookup:** ToolSearch "${toolSearchQuery()}" — one round trip for the whole session. Never load them one at a time.`, + `**If schemas are deferred, load them in ONE lookup:** ToolSearch "${toolSearchQuery()}". Never load them one at a time.`, '', '- graft_find_code — "how does X work" / "where is Y": ranked hits, code inlined.', '- graft_find_all — when you need EVERY occurrence; find_code is top-N and misses some.', '- graft_trace_calls — who calls it, what it calls, blast radius before a rename.', + '- graft_find_import_cycles — untangle circular imports: every file-to-file cycle, lazy edges flagged.', '- graft_file_api — a file\'s whole API in ~200 tokens.', '- graft_repo_map — orientation in an unfamiliar repo.', '', diff --git a/src/mcp/server.ts b/src/mcp/server.ts index 05d29df6a..d852230e8 100644 --- a/src/mcp/server.ts +++ b/src/mcp/server.ts @@ -21,6 +21,7 @@ const TOOL_COMMAND: Record = { graft_find_code: 'ask', graft_find_all: 'grep', graft_trace_calls: 'callers', + graft_find_import_cycles: 'cycles', graft_file_api: 'skeleton', graft_repo_map: 'map', graft_check_freshness: 'check', diff --git a/src/mcp/tools.ts b/src/mcp/tools.ts index 1167ae307..4d02907e3 100644 --- a/src/mcp/tools.ts +++ b/src/mcp/tools.ts @@ -10,6 +10,7 @@ import { loadGraphCached } from '../graph/load.js'; import { ensureFreshChildren, ensureFreshGraph, refreshNote } from '../graph/refresh.js'; import { contextDirFor } from '../context/node-file.js'; import { resolveSymbol, edgeWalk, type Direction, type EdgeHit } from '../graph/traverse.js'; +import { findImportCycles, formatImportCycles } from '../graph/cycles.js'; import { callersSavings, headerOf, hitLine, looseNoteFor } from '../graph/traverse-cli.js'; import { withSavings, setInputRate } from '../context/savings.js'; import { sessionInputRate } from '../claude/session-metrics.js'; @@ -97,6 +98,12 @@ export const TOOLS: ToolDef[] = [ required: ['symbol'], }, }, + { + name: 'graft_find_import_cycles', + description: + 'Find every resolved file-to-file import cycle across all languages. Reports each import line and marks lazy edges within each cycle ($0, no LLM).', + inputSchema: { type: 'object', properties: {} }, + }, { name: 'graft_find_all', description: @@ -303,6 +310,11 @@ async function callSingleTool( const text = withSavings(body, callersSavings(w, results)); return { text, isError: false }; } + case 'graft_find_import_cycles': { + const w = loadGraphCached(contextDirFor(root, dirOverride)); + if (!w) return { text: NO_GRAPH, isError: true }; + return { text: formatImportCycles(findImportCycles(w)), isError: false }; + } case 'graft_find_all': { const pattern = String(args.pattern ?? ''); if (!pattern) return { text: 'graft_find_all requires a pattern', isError: true }; diff --git a/src/telemetry/contract.ts b/src/telemetry/contract.ts index 106631532..78db2e091 100644 --- a/src/telemetry/contract.ts +++ b/src/telemetry/contract.ts @@ -90,7 +90,7 @@ export type AgentHost = 'claude-code' | 'cursor' | 'mcp' | 'cli'; * what keeps a future `graft ` from becoming a data leak. */ export const TRACKED_COMMANDS = [ - 'ask', 'grep', 'callers', 'skeleton', 'map', 'check', 'blast', 'viz', + 'ask', 'grep', 'callers', 'cycles', 'skeleton', 'map', 'check', 'blast', 'viz', ] as const; export type TrackedCommand = (typeof TRACKED_COMMANDS)[number]; diff --git a/test/graph-cycles-cli.test.ts b/test/graph-cycles-cli.test.ts new file mode 100644 index 000000000..e69910992 --- /dev/null +++ b/test/graph-cycles-cli.test.ts @@ -0,0 +1,94 @@ +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { execFileSync, spawnSync } from "node:child_process"; +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { buildGraph } from "../src/graph/build.js"; +import { writeWorkspace } from "../src/graph/workspace.js"; +import { TOOLS, callTool } from "../src/mcp/tools.js"; + +test("cycles CLI refreshes the graph and matches the listed MCP tool", async () => { + const dir = mkdtempSync(join(tmpdir(), "graft-cycles-cli-")); + const pkg = join(dir, "pkg"); + const expected = [ + "Cycle 1 · 2 files", + " pkg/a.py:2 imports → pkg/b.py [lazy]", + " pkg/b.py:1 imports → pkg/a.py", + "", + ].join("\n"); + try { + mkdirSync(pkg, { recursive: true }); + writeFileSync(join(pkg, "__init__.py"), ""); + writeFileSync(join(pkg, "a.py"), "def load():\n return None\n"); + writeFileSync(join(pkg, "b.py"), "import pkg.a\n"); + execFileSync(process.execPath, ["--import", "tsx", "src/cli.ts", "build", dir], { stdio: "pipe" }); + + writeFileSync(join(pkg, "a.py"), "def load():\n import pkg.b\n"); + const cli = spawnSync(process.execPath, ["--import", "tsx", "src/cli.ts", "cycles", dir], { + encoding: "utf8", + }); + const mcp = await callTool(dir, "graft_find_import_cycles", {}); + + assert.deepEqual( + { + cli: { status: cli.status, stdout: cli.stdout }, + listed: TOOLS.some((tool) => tool.name === "graft_find_import_cycles"), + mcp, + }, + { + cli: { status: 0, stdout: expected }, + listed: true, + mcp: { text: expected, isError: false }, + }, + ); + } finally { + rmSync(dir, { recursive: true, force: true }); + } +}); + +test("cycles CLI federates every child graph from a workspace parent", async () => { + const parent = mkdtempSync(join(tmpdir(), "graft-cycles-workspace-")); + const children = ["repoA", "repoB"]; + try { + for (const child of children) { + const childDir = join(parent, child); + const pkg = join(childDir, "pkg"); + mkdirSync(join(childDir, ".git"), { recursive: true }); + mkdirSync(pkg, { recursive: true }); + writeFileSync(join(pkg, "__init__.py"), ""); + writeFileSync(join(pkg, "a.py"), "import pkg.b\n"); + writeFileSync(join(pkg, "b.py"), "import pkg.a\n"); + await buildGraph(childDir); + } + writeWorkspace(parent, { version: 1, children }); + + const cli = spawnSync( + process.execPath, + ["--import", "tsx", "src/cli.ts", "cycles", "--no-refresh", parent], + { encoding: "utf8" }, + ); + + assert.deepEqual( + { status: cli.status, stdout: cli.stdout, stderr: cli.stderr }, + { + status: 0, + stdout: [ + "## repoA/", + "Cycle 1 · 2 files", + " pkg/a.py:1 imports → pkg/b.py", + " pkg/b.py:1 imports → pkg/a.py", + "", + "## repoB/", + "Cycle 1 · 2 files", + " pkg/a.py:1 imports → pkg/b.py", + " pkg/b.py:1 imports → pkg/a.py", + "", + ].join("\n"), + stderr: "", + }, + ); + } finally { + rmSync(parent, { recursive: true, force: true }); + } +}); diff --git a/test/graph-cycles.test.ts b/test/graph-cycles.test.ts new file mode 100644 index 000000000..ab9469e6e --- /dev/null +++ b/test/graph-cycles.test.ts @@ -0,0 +1,118 @@ +import { test } from "node:test"; +import assert from "node:assert/strict"; +import type { EdgeV1, GraphV1, NodeV1, Relation } from "../src/graph/types.js"; + +function fileNode(id: string): NodeV1 { + return { + id, + name: id.split("/").at(-1)!, + kind: "file", + path: id, + span: "L1-L1", + signature: null, + exported: true, + origin: "ast", + body_hash: id, + summary_state: "pending", + summary: null, + crux: null, + }; +} + +function edge( + source: string, + target: string, + relation: Relation = "imports", + metadata: Pick = {}, +): EdgeV1 { + return { source, target, relation, confidence: "extracted", ...metadata }; +} + +function graph(nodeIds: string[], edges: EdgeV1[]): GraphV1 { + const nodes = nodeIds.map(fileNode); + return { + meta: { version: 1, nodeCount: nodes.length, edgeCount: edges.length, languages: ["python"] }, + nodes, + edges, + }; +} + +async function cyclesApi() { + try { + return await import("../src/graph/cycles.js"); + } catch { + assert.fail("the cycles core module should exist"); + } +} + +test("findImportCycles returns a resolved two-file SCC with import metadata", async () => { + const { findImportCycles } = await cyclesApi(); + const edges = [ + edge("a.py", "b.py", "imports", { lazy: true, line: 10 }), + edge("b.py", "a.py", "imports", { line: 20 }), + edge("solo.py", "solo.py", "imports", { line: 30 }), + edge("solo.py", "external.module", "imports", { line: 31 }), + edge("a.py", "solo.py", "references", { line: 32 }), + ]; + + assert.deepEqual(findImportCycles(graph(["a.py", "b.py", "solo.py"], edges)), [ + { + files: ["a.py", "b.py"], + edges: [ + edge("a.py", "b.py", "imports", { lazy: true, line: 10 }), + edge("b.py", "a.py", "imports", { line: 20 }), + ], + }, + ]); +}); + +test("findImportCycles orders disjoint and multi-file SCCs independently of graph arrival order", async () => { + const { findImportCycles } = await cyclesApi(); + const nodes = ["z.py", "y.py", "x.py", "d.py", "c.py"]; + const edges = [ + edge("z.py", "x.py", "imports", { line: 30 }), + edge("y.py", "z.py", "imports", { line: 20 }), + edge("x.py", "y.py", "imports", { line: 10 }), + edge("d.py", "c.py", "imports", { line: 50 }), + edge("c.py", "d.py", "imports", { line: 40 }), + ]; + const expected = [ + { + files: ["c.py", "d.py"], + edges: [edge("c.py", "d.py", "imports", { line: 40 }), edge("d.py", "c.py", "imports", { line: 50 })], + }, + { + files: ["x.py", "y.py", "z.py"], + edges: [ + edge("x.py", "y.py", "imports", { line: 10 }), + edge("y.py", "z.py", "imports", { line: 20 }), + edge("z.py", "x.py", "imports", { line: 30 }), + ], + }, + ]; + + assert.deepEqual(findImportCycles(graph(nodes, edges)), expected); + assert.deepEqual(findImportCycles(graph([...nodes].reverse(), [...edges].reverse())), expected); +}); + +test("formatImportCycles renders titled arrow blocks with line and lazy annotations", async () => { + const { findImportCycles, formatImportCycles } = await cyclesApi(); + assert.equal(typeof formatImportCycles, "function"); + const cycles = findImportCycles( + graph( + ["a.py", "b.py"], + [edge("a.py", "b.py", "imports", { lazy: true, line: 10 }), edge("b.py", "a.py", "imports", { line: 20 })], + ), + ); + + assert.equal( + formatImportCycles(cycles), + ["Cycle 1 · 2 files", " a.py:10 imports → b.py [lazy]", " b.py:20 imports → a.py", ""].join("\n"), + ); +}); + +test("formatImportCycles reports an empty result without failing", async () => { + const { formatImportCycles } = await cyclesApi(); + assert.equal(typeof formatImportCycles, "function"); + assert.equal(formatImportCycles([]), "no import cycles found\n"); +}); diff --git a/test/graph-python-imports.test.ts b/test/graph-python-imports.test.ts new file mode 100644 index 000000000..942d3d17b --- /dev/null +++ b/test/graph-python-imports.test.ts @@ -0,0 +1,118 @@ +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { join } from "node:path"; +import { tmpdir } from "node:os"; +import { extractFile } from "../src/graph/extract.js"; +import { calleesOf } from "../src/graph/traverse.js"; +import { readGraph } from "../src/graph/write.js"; + +function importEdges(source: string) { + return extractFile("app.py", source, "python").rawEdges.filter((edge) => edge.relation === "imports"); +} + +test("Python import metadata is additive", () => { + const dir = mkdtempSync(join(tmpdir(), "graft-python-imports-")); + const path = join(dir, "legacy-wiring.json"); + const fileNode = (id: string) => ({ + id, + name: id, + kind: "file", + path: id, + span: "L1-L1", + signature: null, + exported: true, + origin: "ast", + body_hash: id, + summary_state: "pending", + summary: null, + crux: null, + }); + + try { + writeFileSync( + path, + JSON.stringify({ + meta: { version: 1, nodeCount: 2, edgeCount: 1, languages: ["python"] }, + nodes: [fileNode("app.py"), fileNode("dep.py")], + edges: [{ source: "app.py", target: "dep.py", relation: "imports", confidence: "inferred" }], + }), + ); + const legacy = readGraph(path); + assert.ok(legacy, "a graph whose edges predate optional import metadata still loads"); + assert.deepEqual(calleesOf(legacy, legacy.nodes[0]!), [ + { node: legacy.nodes[1], id: "dep.py", relation: "imports", depth: 1 }, + ]); + + const edges = importEdges( + [ + "import top", + "if sys.version_info:", + " import conditional", + "from typing import TYPE_CHECKING as TC", + "if TC:", + " import aliased", + "", + ].join("\n"), + ); + assert.deepEqual( + edges.map(({ specifier, line, lazy }) => ({ specifier, line, lazy })), + [ + { specifier: "top", line: 1, lazy: undefined }, + { specifier: "conditional", line: 3, lazy: undefined }, + { specifier: "typing", line: 4, lazy: undefined }, + { specifier: "aliased", line: 6, lazy: undefined }, + ], + ); + } finally { + rmSync(dir, { recursive: true, force: true }); + } +}); + +test("Python definition-scoped imports are lazy", () => { + const edges = importEdges( + ["def load():", " import inside_function", "class Plugin:", " import inside_class", ""].join("\n"), + ); + + assert.deepEqual( + edges.map(({ specifier, line, lazy }) => ({ specifier, line, lazy })), + [ + { specifier: "inside_function", line: 2, lazy: true }, + { specifier: "inside_class", line: 4, lazy: true }, + ], + ); +}); + +test("Python bare TYPE_CHECKING defers only its consequence", () => { + const edges = importEdges( + [ + "from typing import TYPE_CHECKING", + "if TYPE_CHECKING:", + " import type_only", + "else:", + " import runtime_fallback", + "", + ].join("\n"), + ); + + assert.deepEqual( + edges.map(({ specifier, line, lazy }) => ({ specifier, line, lazy })), + [ + { specifier: "typing", line: 1, lazy: undefined }, + { specifier: "type_only", line: 3, lazy: true }, + { specifier: "runtime_fallback", line: 5, lazy: undefined }, + ], + ); +}); + +test("Python typing.TYPE_CHECKING imports are lazy", () => { + const edges = importEdges(["import typing", "if typing.TYPE_CHECKING:", " import models", ""].join("\n")); + + assert.deepEqual( + edges.map(({ specifier, line, lazy }) => ({ specifier, line, lazy })), + [ + { specifier: "typing", line: 1, lazy: undefined }, + { specifier: "models", line: 3, lazy: true }, + ], + ); +}); diff --git a/test/graph-python-resolve.test.ts b/test/graph-python-resolve.test.ts new file mode 100644 index 000000000..f9f2cf2a9 --- /dev/null +++ b/test/graph-python-resolve.test.ts @@ -0,0 +1,192 @@ +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { buildGraph } from "../src/graph/build.js"; +import { resolveEdges } from "../src/graph/resolve.js"; +import type { RawEdge } from "../src/graph/extract.js"; +import type { NodeV1 } from "../src/graph/types.js"; +import { readGraph, wiringPath } from "../src/graph/write.js"; + +function fileNode(id: string): NodeV1 { + return { + id, + name: id.split("/").at(-1)!, + kind: "file", + path: id, + span: "L1-L1", + signature: null, + exported: true, + origin: "ast", + body_hash: id, + summary_state: "pending", + summary: null, + crux: null, + }; +} + +function rawImport(file: string, specifier: string, metadata: Pick = {}): RawEdge { + return { source: file, relation: "imports", specifier, file, ...metadata }; +} + +function importTargets(nodes: NodeV1[], rawEdges: RawEdge[]): string[] { + return resolveEdges(nodes, rawEdges) + .filter((edge) => edge.relation === "imports") + .map((edge) => edge.target); +} + +test("Python absolute import resolution is suffix-precise", () => { + const importer = "consumer.py"; + const nodes = [ + importer, + "vendor/pkg/module.py", + "vendor/pkg/contracts.pyi", + "vendor/pkg/__init__.py", + "vendor/pkg/api.py", + "one/pkg/dup.py", + "two/pkg/dup.py", + "one/pkg/both.py", + "one/pkg/both.pyi", + ].map(fileNode); + const specs = ["pkg.module", "pkg.contracts", "pkg", "pkg.api.symbol", "pkg.dup", "pkg.both", "typing"]; + + assert.deepEqual( + importTargets(nodes, specs.map((specifier) => rawImport(importer, specifier))), + [ + "vendor/pkg/module.py", + "vendor/pkg/contracts.pyi", + "pkg", + "vendor/pkg/api.py", + "pkg.dup", + "pkg.both", + "typing", + ], + ); +}); + +test("Python suffix collisions resolve only within one importer root", () => { + const importer = "apps/a/src/service.py"; + const nodes = [ + importer, + "apps/a/src/db/models.py", + "apps/b/src/db/models.py", + "apps/a/db/models.py", + "x/shared/item.py", + "y/shared/item.py", + ].map(fileNode); + + assert.deepEqual( + importTargets(nodes, ["src.db.models", "db.models", "shared.item"].map((spec) => rawImport(importer, spec))), + ["apps/a/src/db/models.py", "db.models", "shared.item"], + ); +}); + +test("Python unique single-segment imports stay raw outside the importer source root", () => { + const importer = "apps/admin/src/service.py"; + const nodes = [importer, "apps/entities/src/explore/utils/logging.py"].map(fileNode); + + assert.deepEqual(importTargets(nodes, [rawImport(importer, "logging")]), ["logging"]); +}); + +test("Python unique single-segment imports resolve at the importer source root", () => { + const importer = "apps/figaro/src/service.py"; + const nodes = [importer, "apps/figaro/src/config.py"].map(fileNode); + + assert.deepEqual(importTargets(nodes, [rawImport(importer, "config")]), ["apps/figaro/src/config.py"]); +}); + +test("Python relative imports honor parent hops and stay raw when unresolved", () => { + const importer = "pkg/sub/deep/importer.py"; + const nodes = [ + importer, + "pkg/sub/deep/local.py", + "pkg/sub/shared.py", + "pkg/rootmod.py", + "outside.py", + ].map(fileNode); + + assert.deepEqual( + importTargets( + nodes, + [".local", "..shared", "...rootmod", ".missing", "....outside"].map((spec) => + rawImport(importer, spec), + ), + ), + ["pkg/sub/deep/local.py", "pkg/sub/shared.py", "pkg/rootmod.py", ".missing", "....outside"], + ); +}); + +test("Python build serializes resolved and unresolved imports with metadata", async () => { + const dir = mkdtempSync(join(tmpdir(), "graft-python-resolve-")); + const importer = "pkg/sub/importer.py"; + try { + for (const path of [ + "pkg/__init__.py", + "pkg/absolute.py", + "pkg/deferred.py", + "pkg/sub/__init__.py", + "pkg/sub/relative.py", + "one/ambiguous/target.py", + "two/ambiguous/target.py", + ]) { + mkdirSync(join(dir, path, ".."), { recursive: true }); + writeFileSync(join(dir, path), ""); + } + writeFileSync( + join(dir, importer), + [ + "import pkg.absolute", + "from .relative import Thing", + "import ambiguous.target", + "import os", + "", + "def late():", + " import pkg.deferred", + "", + ].join("\n"), + ); + + await buildGraph(dir); + const graph = readGraph(wiringPath(join(dir, "graft")))!; + const imports = graph.edges.filter((edge) => edge.relation === "imports" && edge.source === importer); + + assert.deepEqual(imports, [ + { source: importer, target: "ambiguous.target", relation: "imports", confidence: "extracted", line: 3 }, + { source: importer, target: "os", relation: "imports", confidence: "extracted", line: 4 }, + { source: importer, target: "pkg/absolute.py", relation: "imports", confidence: "extracted", line: 1 }, + { + source: importer, + target: "pkg/deferred.py", + relation: "imports", + confidence: "extracted", + lazy: true, + line: 7, + }, + { source: importer, target: "pkg/sub/relative.py", relation: "imports", confidence: "extracted", line: 2 }, + ]); + } finally { + rmSync(dir, { recursive: true, force: true }); + } +}); + +test("Python import dedupe is eager-wins in both arrival orders", () => { + const importer = "importer.py"; + const nodes = [importer, "target.py"].map(fileNode); + const eager = rawImport(importer, "target", { line: 4 }); + const lazy = rawImport(importer, "target", { lazy: true, line: 10 }); + const expected = [ + { + source: importer, + target: "target.py", + relation: "imports", + confidence: "extracted", + line: 4, + }, + ]; + + assert.deepEqual( + [resolveEdges(nodes, [lazy, eager]), resolveEdges(nodes, [eager, lazy])], + [expected, expected], + ); +}); diff --git a/test/mcp-server.test.ts b/test/mcp-server.test.ts index f0610e034..561d0681a 100644 --- a/test/mcp-server.test.ts +++ b/test/mcp-server.test.ts @@ -64,6 +64,7 @@ const ALL_TOOLS = [ 'graft_file_api', 'graft_check_freshness', 'graft_trace_calls', + 'graft_find_import_cycles', 'graft_find_all', 'graft_repo_map', ]; @@ -139,7 +140,7 @@ test('initialize carries instructions — the layer that survives tool deferral' // else, so this string has to carry both the pitch and the recovery instruction. assert.match(instructions, /ONE lookup/, 'tells the agent to batch the schema fetch'); assert.match(instructions, /select:mcp__graft__graft_find_code,/, 'gives a copy-pasteable query'); - for (const t of ['graft_find_code', 'graft_find_all', 'graft_trace_calls', 'graft_file_api', 'graft_repo_map']) { + for (const t of ['graft_find_code', 'graft_find_all', 'graft_trace_calls', 'graft_find_import_cycles', 'graft_file_api', 'graft_repo_map']) { assert.ok(instructions.includes(t), `names ${t}`); } // Observed sibling servers sit at 660–984 chars; nothing proves a longer one diff --git a/test/mcp-tools.test.ts b/test/mcp-tools.test.ts index a3ccf6905..388d772d7 100644 --- a/test/mcp-tools.test.ts +++ b/test/mcp-tools.test.ts @@ -72,12 +72,13 @@ function multiDirRepo(): string { return d; } -test('TOOLS lists the six tools with schemas', async () => { +test('TOOLS lists the seven tools with schemas', async () => { assert.deepEqual(TOOLS.map((t) => t.name), [ 'graft_find_code', 'graft_file_api', 'graft_check_freshness', 'graft_trace_calls', + 'graft_find_import_cycles', 'graft_find_all', 'graft_repo_map', ]); @@ -313,8 +314,11 @@ const RENAMES: Record = { graft_check: 'graft_check_freshness', }; -test('TOOLS advertises exactly the six new names — the roster does not grow', () => { - assert.deepEqual([...TOOLS.map((t) => t.name)].sort(), Object.values(RENAMES).sort()); +test('TOOLS advertises the six renamed tools plus import cycles', () => { + assert.deepEqual( + [...TOOLS.map((t) => t.name)].sort(), + [...Object.values(RENAMES), 'graft_find_import_cycles'].sort(), + ); }); test('every old tool name still resolves to its replacement', () => { diff --git a/test/telemetry-contract.test.ts b/test/telemetry-contract.test.ts index a9c652001..418c52de1 100644 --- a/test/telemetry-contract.test.ts +++ b/test/telemetry-contract.test.ts @@ -82,6 +82,7 @@ test('the contract lists exactly the seven documented events', () => { test('only the known commands are tracked', () => { assert.equal(isTrackedCommand('ask'), true); + assert.equal(isTrackedCommand('cycles'), true); assert.equal(isTrackedCommand('init'), false, 'init has its own event, not a query'); assert.equal(isTrackedCommand('telemetry'), false); assert.equal(isTrackedCommand('_telemetry-flush'), false);