From 9bfdd092386e6652fb086b8e8b079cffe7bc49c5 Mon Sep 17 00:00:00 2001 From: Colby Mchenry Date: Fri, 30 Oct 2026 19:53:53 +0000 Subject: [PATCH] feat(cli): codegraph cycles lists the files that import each other in a circle (#3099) New `codegraph cycles [path] [--json]` prints the groups of files that all reach each other through imports. It exits 1 when there are any and 0 when there are none, so `codegraph cycles || exit 1` works as a pre-commit or CI check. The command stays a thin wrapper over CodeGraph.findCircularDependencies(), but the method itself had to change. It had no callers, and on real repos its walk wasn't fit to print: - It reported one list per back edge, so the number depends on the walk: 1,490 overlapping lists on excalidraw, 87 on gin, 44 on flask. - It left out files. A walk that meets a visited file stops there, so a file whose loop closes through one is never listed (now pinned by a test: a>b, b>a, a>c, c>b). - It counted every edge kind, and most of the graph's edges aren't imports. An interface's call fans out to a test's fake, so src/extraction/parse-pool.ts "depended" on its own test. Two Next.js pages that link to each other were a cycle. gin's files were a 55-file knot, though Go forbids import cycles. With exit 1 those are failures nobody can fix. The method now takes the strongly connected components of the file import graph, one group per knot. The Tarjan pass moves from the viewer's map into src/graph/file-cycles.ts and both surfaces use it, so the viewer's "Circular imports between files" panel, which was reading every edge too, now reports the same groups. Dependent counts on the map keep their own edge list. Validation: new cli-cycles and file-cycles suites, plus cycles in the read-commands suite (mid-rebuild, and old schema indexes); 4 of the 10 findCircularDependencies and finder tests fail with main's method and all 15 pass with this one. Against the TypeScript compiler's own module resolver over the same files (kit oracle): excalidraw 3 groups, 369 files, all identical; codegraph's src/ 3 of 4 identical, and the fourth needs src/resolution/name-matcher.ts, which at 1.6 MB is over the default size limit and so isn't parsed. flask reports 2 groups and gin none. It runs in under a second on each. Type-only imports count, as they aren't told apart in the graph. The MCP tool from PR #889 is left out on purpose: a new tool is rarely picked up, and this needs no agent steering. Reimplements PR #1012 by @jiangyq9, with the exit code from the CLI in PR #889 by @goutham80808. Fixes #888 Co-Authored-By: Claude Opus 5.5 --- AGENTS.md | 4 +- CHANGELOG.md | 1 + README.md | 1 + __tests__/cli-cycles.test.ts | 116 ++++++++++++++ __tests__/file-cycles.test.ts | 146 ++++++++++++++++++ .../read-commands-during-rebuild.test.ts | 1 + site/src/content/docs/reference/cli.md | 13 ++ src/bin/codegraph.ts | 55 +++++++ src/db/queries.ts | 33 ++-- src/graph/file-cycles.ts | 89 +++++++++++ src/graph/queries.ts | 43 +----- src/index.ts | 17 +- src/ui-server/api/map.ts | 95 ++---------- 13 files changed, 475 insertions(+), 139 deletions(-) create mode 100644 __tests__/cli-cycles.test.ts create mode 100644 __tests__/file-cycles.test.ts create mode 100644 src/graph/file-cycles.ts diff --git a/AGENTS.md b/AGENTS.md index 01752bd63..3c1a7663e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -80,13 +80,13 @@ The public API surface is `src/index.ts` — the `CodeGraph` class wires all the - `src/extraction/` — `ExtractionOrchestrator`, tree-sitter wrappers, per-language extractors under `languages/` (one file per language), plus standalone extractors for non-tree-sitter formats (`svelte-extractor.ts`, `vue-extractor.ts`, `liquid-extractor.ts`, `dfm-extractor.ts` for Delphi). `parse-worker.ts` runs heavy parsing off the main thread. - `src/resolution/` — `ReferenceResolver` orchestrates `import-resolver.ts` (with `path-aliases.ts` for tsconfig path aliases + cargo workspace member globs), `name-matcher.ts`, and `frameworks/` (Express, Laravel, Rails, FastAPI, Django, Flask, Spring, Gin, Axum, ASP.NET, Vapor, React Router, Next.js — `nextjs.ts`: pages and `route.ts` handlers from files, `router.push` / `redirect` / `NextResponse.redirect` as `navigates` edges, with `next-router-synthesizer.ts` for `` — Expo Router, SvelteKit, Vue/Nuxt, Cargo workspaces). Frameworks emit `route` nodes and `references` edges. `callback-synthesizer.ts` holds the whole-graph synthesis passes (`SYNTH_PASSES`, merged in registry order — first-seen wins a duplicate pair) with the language gates; `tier-synthesizer.ts` is the cross-tier pass (a client's literal `fetch`/`axios` path onto its own route, a queue job onto its consumer, a bus / socket event onto its handler — `channel`, `tier`, `registeredAt` on every edge; registered before the in-process emitter pass so its more specific edge wins); `synth-utils.ts` has the helpers they share (`enclosingFn`, `enclosingValue`, `makeLineAt`). Express's `postExtract` composes `app.use('/prefix', router)` mounts onto a mounted file's route names, idempotently (the original path stays in `qualifiedName`). - `src/graph/` — `GraphTraverser` (BFS/DFS, impact radius, path finding) and `GraphQueryManager` (high-level queries), plus the shared query-time derivations more than one surface renders: `named-symbol-flow.ts` (the one path finder, behind `codegraph_explore`'s Flow section and the viewer's Flow strip), `dynamic-boundary-report.ts` (where the graph stops), `type-hierarchy.ts` (ancestors/subtypes and the implementation count explore prints and the viewer draws), - `dead-code.ts` (unreferenced symbols, and every reason a candidate is NOT claimed), `synthesized-hop.ts` (how a synthesized hop reads, in explore's Flow, the `codegraph_node` trail and ContextBuilder's call paths — a new synthesizer's wording goes here). A derivation that two callers render must live here, not in `ToolHandler` — two derivations eventually disagree. + `dead-code.ts` (unreferenced symbols, and every reason a candidate is NOT claimed), `synthesized-hop.ts` (how a synthesized hop reads, in explore's Flow, the `codegraph_node` trail and ContextBuilder's call paths — a new synthesizer's wording goes here), `file-cycles.ts` (circular imports, in `codegraph cycles` and the viewer's map). A derivation that two callers render must live here, not in `ToolHandler` — two derivations eventually disagree. - `src/context/` — `ContextBuilder` + formatter for markdown/JSON output. - `src/search/` — full-text query parser and helpers for FTS5. - `src/sync/` — `FileWatcher` (native FSEvents/inotify/RDCW) with debounce + filter, and git-hook helpers. - `src/mcp/` — MCP server (`MCPServer`, `tools.ts`, `transport.ts`). `server-instructions.ts` is what the server returns in the MCP `initialize` response — keep it in sync with the user-facing tool guidance. - `src/installer/` — see below. -- `src/bin/codegraph.ts` — CLI (commander). Subcommands: `install`, `uninstall`, `init`, `uninit`, `index`, `sync`, `status`, `unlock`, `query`, `explore`, `context`, `node`, `files`, `callers`, `callees`, `impact`, `affected`, `daemon`, `telemetry`, `upgrade`, `version`. Hidden from `--help`: `serve --mcp`, `prompt-hook`, and `ui`/`web` unless `CODEGRAPH_UI=1`. +- `src/bin/codegraph.ts` — CLI (commander). Subcommands: `install`, `uninstall`, `init`, `uninit`, `index`, `sync`, `status`, `unlock`, `query`, `explore`, `context`, `node`, `files`, `callers`, `callees`, `impact`, `affected`, `cycles`, `daemon`, `telemetry`, `upgrade`, `version`. Hidden from `--help`: `serve --mcp`, `prompt-hook`, and `ui`/`web` unless `CODEGRAPH_UI=1`. - `src/ui/` — terminal UI (shimmer progress, worker). - `src/ui-server/` -- read-only JSON API for the `codegraph ui` browser viewer (`api/`: `node`, `flow`, `map`, `screens`, `steps`, `deadcode`, `trails`, `program`, ...) plus static server; Svelte viewer lives in `ui/` (see `docs/design/codegraph-ui-design-spec.md`). `screens`/`steps`/`program` share one fold (`via`/`when` via `graph/branch-guards.ts`); `api/effects.ts` curates calls that leave the index; `api/route-roots.ts` names where a route's code starts. Derivations rendered by more than one surface belong in `src/graph/`, not `ToolHandler`. **Not released yet:** `codegraph ui` / `web` are refused (and hidden from `--help`) unless `CODEGRAPH_UI=1` is set — `src/bin/viewer-gate.ts`. Viewer changelog entries wait in `docs/viewer-launch-changelog.md`, not under `[Unreleased]`. At launch: delete the gate, move those entries back, drop the "not released yet" notes from `site/`. diff --git a/CHANGELOG.md b/CHANGELOG.md index 1c8b00e91..1f8a5584d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,7 @@ and adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). - `codegraph status` now shows how many of a language's files yielded symbols when that isn't all of them, and warns when none did, so a parser that isn't working no longer looks healthy, and `codegraph status --strict` exits non-zero on that or on an index that's missing, unfinished or out of date, so packaging and CI can gate on it (#1658). Thanks @ferrine. - `codegraph serve --mcp --no-telemetry` turns telemetry off for one MCP server entry, so a shared MCP config can opt out without an environment variable; a background daemon that's already running for the project keeps its own setting until it exits. Thanks @danusha2345. (#1908) +- New `codegraph cycles` command lists the groups of files that import each other in a circle, as text or `--json`, and exits 1 when it finds any, so it works as a pre-commit or CI check (#888). Thanks @jiangyq9 and @goutham80808. ### Fixes diff --git a/README.md b/README.md index 54ce5ba02..93735ce09 100644 --- a/README.md +++ b/README.md @@ -543,6 +543,7 @@ codegraph callers # Find what calls a function/method (--limit, codegraph callees # Find what a function/method calls (--limit, --json) codegraph impact # Analyze what code is affected by changing a symbol (--depth, --json) codegraph affected [files...] # Find test files affected by changes (see below) +codegraph cycles [path] # List circular imports between files (--json); exits 1 when there are any codegraph daemon # Manage background daemons — pick one to stop (alias: daemons) codegraph telemetry [on|off] # Show or change anonymous usage telemetry codegraph upgrade [version] # Update to the latest release (--check, --force) diff --git a/__tests__/cli-cycles.test.ts b/__tests__/cli-cycles.test.ts new file mode 100644 index 000000000..c83ec7fa0 --- /dev/null +++ b/__tests__/cli-cycles.test.ts @@ -0,0 +1,116 @@ +/** + * `codegraph cycles`: circular imports between files, end to end against the + * built binary. Exit 1 when there are any is the command's contract: it is what + * makes `codegraph cycles || exit 1` a pre-commit or CI check. + */ +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { spawnSync } from 'child_process'; +import * as fs from 'fs'; +import * as os from 'os'; +import * as path from 'path'; +import { CodeGraph } from '../src'; + +const BIN = path.resolve(__dirname, '../dist/bin/codegraph.js'); + +function run(cwd: string, ...args: string[]) { + const res = spawnSync(process.execPath, [BIN, 'cycles', cwd, ...args], { + encoding: 'utf-8', + env: { ...process.env, CODEGRAPH_NO_DAEMON: '1', CODEGRAPH_WASM_RELAUNCHED: '1' }, + }); + return { status: res.status, stdout: res.stdout, stderr: res.stderr }; +} + +async function indexProject(dir: string, files: Record): Promise { + for (const [rel, content] of Object.entries(files)) { + const abs = path.join(dir, rel); + fs.mkdirSync(path.dirname(abs), { recursive: true }); + fs.writeFileSync(abs, content); + } + const cg = CodeGraph.initSync(dir); + await cg.indexAll(); + cg.close(); +} + +/** `count` files in one import ring: each imports the next, the last the first. */ +function ring(count: number): Record { + const files: Record = {}; + for (let i = 0; i < count; i++) { + const next = (i + 1) % count; + files[`src/f${String(i).padStart(2, '0')}.ts`] = + `import { f${next} } from './f${String(next).padStart(2, '0')}';\nexport function f${i}() { return f${next}(); }\n`; + } + return files; +} + +describe('codegraph cycles', () => { + let dir: string; + + beforeEach(() => { + dir = fs.mkdtempSync(path.join(os.tmpdir(), 'codegraph-cycles-cli-')); + }); + afterEach(() => { + fs.rmSync(dir, { recursive: true, force: true }); + }); + + it('lists the files of a circular import and exits 1', async () => { + await indexProject(dir, { + 'src/a.ts': "import { b } from './b';\nexport function a() { return b(); }\n", + 'src/b.ts': "import { a } from './a';\nexport function b() { return a(); }\n", + 'src/plain.ts': "import { a } from './a';\nexport const plain = a;\n", + }); + + const res = run(dir); + expect(res.status).toBe(1); + expect(res.stdout).toContain('Circular imports: 1 group, 2 files'); + expect(res.stdout).toContain('src/a.ts'); + expect(res.stdout).toContain('src/b.ts'); + expect(res.stdout).not.toContain('src/plain.ts'); + }); + + it('prints the whole report as JSON and still exits 1', async () => { + await indexProject(dir, { + 'src/a.ts': "import { b } from './b';\nexport function a() { return b(); }\n", + 'src/b.ts': "import { a } from './a';\nexport function b() { return a(); }\n", + }); + + const res = run(dir, '--json'); + expect(res.status).toBe(1); + expect(JSON.parse(res.stdout)).toEqual({ count: 1, cycles: [['src/a.ts', 'src/b.ts']] }); + }); + + it('says so and exits 0 when the project has none', async () => { + await indexProject(dir, { + 'src/a.ts': 'export function a() { return 1; }\n', + 'src/b.ts': "import { a } from './a';\nexport function b() { return a(); }\n", + }); + + const res = run(dir); + expect(res.status).toBe(0); + expect(res.stdout).toContain('No circular imports found'); + + const json = run(dir, '--json'); + expect(json.status).toBe(0); + expect(JSON.parse(json.stdout)).toEqual({ count: 0, cycles: [] }); + }); + + it('shows the first files of a long group, and --json all of them', async () => { + await indexProject(dir, ring(14)); + + const res = run(dir); + expect(res.status).toBe(1); + expect(res.stdout).toContain('1. 14 files'); + expect(res.stdout).toContain('src/f11.ts'); + expect(res.stdout).not.toContain('src/f12.ts'); + expect(res.stdout).toContain('... and 2 more (--json lists them all)'); + + const parsed = JSON.parse(run(dir, '--json').stdout) as { cycles: string[][] }; + expect(parsed.cycles).toHaveLength(1); + expect(parsed.cycles[0]).toHaveLength(14); + }); + + it('fails plainly on a project with no index', () => { + const res = run(dir); + expect(res.status).toBe(1); + expect(res.stderr + res.stdout).toContain('not initialized'); + }); +}); diff --git a/__tests__/file-cycles.test.ts b/__tests__/file-cycles.test.ts new file mode 100644 index 000000000..7c23d228a --- /dev/null +++ b/__tests__/file-cycles.test.ts @@ -0,0 +1,146 @@ +/** + * What counts as a circular import: the strongly connected components of the + * file import graph, and nothing else the graph knows about two files. + */ +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import * as fs from 'fs'; +import * as os from 'os'; +import * as path from 'path'; +import { CodeGraph } from '../src'; +import { findFileCycles } from '../src/graph/file-cycles'; + +const pairs = (...edges: string[]) => + edges.map((edge) => { + const [source, target] = edge.split('>') as [string, string]; + return { source, target }; + }); + +describe('findFileCycles', () => { + it('finds nothing in a graph with no loop, a diamond included', () => { + expect(findFileCycles(pairs('a>b', 'a>c', 'b>d', 'c>d'))).toEqual([]); + expect(findFileCycles([])).toEqual([]); + }); + + it('reports a knot once, with every file in it', () => { + // a>b>a is a loop and so is a>c>b>a. A walk meets b first and has nothing + // left to do at c>b, so it reported a>b and left c out. + expect(findFileCycles(pairs('a>b', 'b>a', 'a>c', 'c>b'))).toEqual([['a', 'b', 'c']]); + // Two loops through one file are one group, however many ways round there are. + expect(findFileCycles(pairs('a>b', 'b>a', 'a>c', 'c>a'))).toEqual([['a', 'b', 'c']]); + }); + + it('keeps separate knots apart, smallest first, and leaves out the files between them', () => { + expect(findFileCycles(pairs('p>q', 'q>r', 'r>p', 'q>x', 'x>a', 'a>b', 'b>a'))).toEqual([ + ['a', 'b'], + ['p', 'q', 'r'], + ]); + }); + + it('gives the same report for the pairs in any order', () => { + const edges = pairs('a>b', 'b>c', 'c>a', 'd>e', 'e>d', 'c>d'); + expect(findFileCycles([...edges].reverse())).toEqual(findFileCycles(edges)); + }); + + it('walks a chain deeper than the call stack', () => { + const length = 200_000; + const chain = Array.from({ length }, (_, i) => ({ source: `f${i}`, target: `f${i + 1}` })); + chain.push({ source: `f${length}`, target: 'f0' }); + const cycles = findFileCycles(chain); + expect(cycles).toHaveLength(1); + expect(cycles[0]).toHaveLength(length + 1); + }); +}); + +describe('findCircularDependencies', () => { + let dir: string; + let cg: CodeGraph | null = null; + + async function index(files: Record): Promise { + for (const [rel, content] of Object.entries(files)) { + const abs = path.join(dir, rel); + fs.mkdirSync(path.dirname(abs), { recursive: true }); + fs.writeFileSync(abs, content); + } + cg = CodeGraph.initSync(dir); + await cg.indexAll(); + return cg; + } + + beforeEach(() => { + dir = fs.mkdtempSync(path.join(os.tmpdir(), 'codegraph-file-cycles-')); + }); + afterEach(() => { + cg?.close(); + cg = null; + fs.rmSync(dir, { recursive: true, force: true }); + }); + + it('finds a loop that runs through a barrel', async () => { + const graph = await index({ + 'src/index.ts': "export * from './a';\n", + 'src/a.ts': "import { helper } from './helper';\nexport function a() { return helper(); }\n", + 'src/helper.ts': "import { a } from './index';\nexport function helper() { return a; }\n", + }); + expect(graph.findCircularDependencies()).toEqual([['src/a.ts', 'src/helper.ts', 'src/index.ts']]); + }); + + it('finds a loop in Python', async () => { + const graph = await index({ + 'a.py': 'from b import bb\n\ndef aa():\n return bb()\n', + 'b.py': 'from a import aa\n\ndef bb():\n return aa()\n', + }); + expect(graph.findCircularDependencies()).toEqual([['a.py', 'b.py']]); + }); + + it('does not take an interface call reaching a test fake for an import', async () => { + // Production code imports nothing from the test. The interface's call fans + // out to every implementer, the fake included, so the file edge list holds + // the loop src>test>src that no import makes. + const graph = await index({ + 'src/pool.ts': [ + 'export interface Worker { post(task: string): void }', + 'export function run(worker: Worker) { worker.post("x"); }', + '', + ].join('\n'), + 'src/pool.test.ts': [ + "import { run, Worker } from './pool';", + 'class FakeWorker implements Worker { post(task: string): void { void task; } }', + 'export function check() { run(new FakeWorker()); }', + '', + ].join('\n'), + }); + const edgeList = graph.getFileDependencyPairs(); + expect(edgeList).toContainEqual({ source: 'src/pool.ts', target: 'src/pool.test.ts' }); + expect(edgeList).toContainEqual({ source: 'src/pool.test.ts', target: 'src/pool.ts' }); + expect(graph.findCircularDependencies()).toEqual([]); + }); + + it('does not take two pages that link to each other for an import', async () => { + const graph = await index({ + 'package.json': JSON.stringify({ name: 'site', dependencies: { next: '15.0.0', react: '19.0.0' } }), + 'app/page.tsx': [ + "import Link from 'next/link';", + 'export default function Home() { return About; }', + '', + ].join('\n'), + 'app/about/page.tsx': [ + "import Link from 'next/link';", + 'export default function About() { return Home; }', + '', + ].join('\n'), + }); + const edgeList = graph.getFileDependencyPairs(); + expect(edgeList).toContainEqual({ source: 'app/page.tsx', target: 'app/about/page.tsx' }); + expect(edgeList).toContainEqual({ source: 'app/about/page.tsx', target: 'app/page.tsx' }); + expect(graph.findCircularDependencies()).toEqual([]); + }); + + it('does not take the files of one Go package calling each other for an import', async () => { + const graph = await index({ + 'go.mod': 'module example.com/m\n\ngo 1.21\n', + 'one.go': 'package m\n\nfunc One() { Two() }\n', + 'two.go': 'package m\n\nfunc Two() { One() }\n', + }); + expect(graph.findCircularDependencies()).toEqual([]); + }); +}); diff --git a/__tests__/read-commands-during-rebuild.test.ts b/__tests__/read-commands-during-rebuild.test.ts index 29eba2cb8..75bf3cdcf 100644 --- a/__tests__/read-commands-during-rebuild.test.ts +++ b/__tests__/read-commands-during-rebuild.test.ts @@ -57,6 +57,7 @@ const READ_COMMANDS: ReadCommand[] = [ { name: 'context', args: ['context', 'getUserProfile', 'loadOrderState'], answer: /loadOrderState/ }, { name: 'explore', args: ['explore', 'getUserProfile', 'loadOrderState'], answer: /src\/order\.ts/ }, { name: 'affected', args: ['affected', 'src/order.ts'], answer: /src\/user\.test\.ts/ }, + { name: 'cycles', args: ['cycles'], answer: /No circular imports found/ }, { name: 'prompt-hook', args: ['prompt-hook'], prompt: 'how does getUserProfile reach loadOrderState?', answer: / # Find what calls a function/method (--limit, codegraph callees # Find what a function/method calls (--limit, --json) codegraph impact # Analyze what code is affected by changing a symbol (--depth, --json) codegraph affected [files...] # Find test files affected by changes (see below) +codegraph cycles [path] # List circular imports between files (--json); exits 1 when there are any codegraph daemon # Manage background daemons — pick one to stop (alias: daemons) codegraph telemetry [on|off] # Show or change anonymous usage telemetry codegraph upgrade [version] # Update to the latest release (--check, --force) @@ -67,6 +68,18 @@ codegraph impact AuthMiddleware --depth 3 Traces import dependencies transitively to find which test files are affected by changed source files. See [Affected Tests in CI](/codegraph/guides/affected-tests/) for options and a CI example. +## cycles + +Lists circular imports: groups of files that all import each other, directly or through others. Each group is reported once with every file in it, smallest group first, and `--json` prints the whole list. + +```bash +codegraph cycles # the project you're standing in +codegraph cycles --json # { "count": 2, "cycles": [["src/a.ts", "src/b.ts"], ...] } +codegraph cycles || exit 1 # as a pre-commit or CI check +``` + +It exits 1 when there are any and 0 when there are none. Only import statements count, type-only imports included. Two files that merely call each other, like the files of one Go package, are not a circular import. + ## ui :::note[Not released yet] diff --git a/src/bin/codegraph.ts b/src/bin/codegraph.ts index 869b31fec..fd574bacc 100644 --- a/src/bin/codegraph.ts +++ b/src/bin/codegraph.ts @@ -20,6 +20,7 @@ * codegraph callees Find what a function/method calls * codegraph impact Analyze what code is affected by changing a symbol * codegraph affected [files] Find test files affected by changes + * codegraph cycles [path] List circular imports between files * codegraph ui [path] Open the browser viewer (alias: web; not released yet — needs CODEGRAPH_UI=1) * codegraph upgrade [version] Update CodeGraph to the latest release */ @@ -2878,6 +2879,60 @@ program } }); +/** Files `codegraph cycles` prints per group; `--json` lists them all. */ +const CLI_CYCLE_FILES_SHOWN = 12; + +/** + * codegraph cycles [path] + * + * List groups of files that all import each other, directly or through others. + * Exits 1 when there are any, so it works as a pre-commit or CI check: + * + * codegraph cycles || exit 1 + */ +program + .command('cycles [path]') + .description('List circular imports between files (exits 1 when there are any)') + .option('-j, --json', 'Output as JSON') + .action(async (pathArg: string | undefined, options: { json?: boolean }) => { + const projectPath = resolveProjectPath(pathArg); + + try { + if (!isInitialized(projectPath)) { + error(`CodeGraph not initialized in ${projectPath}`); + process.exit(1); + } + + const cg = await openIndexToRead(projectPath); + try { + const cycles = cg.findCircularDependencies(); + if (options.json) { + console.log(JSON.stringify({ count: cycles.length, cycles }, null, 2)); + } else if (cycles.length === 0) { + success('No circular imports found'); + } else { + const files = cycles.reduce((sum, group) => sum + group.length, 0); + console.log(chalk.bold(`\nCircular imports: ${cycles.length} group${cycles.length === 1 ? '' : 's'}, ${files} files\n`)); + cycles.forEach((group, i) => { + console.log(chalk.cyan(`${i + 1}. ${group.length} files`)); + for (const file of group.slice(0, CLI_CYCLE_FILES_SHOWN)) console.log(` ${file}`); + if (group.length > CLI_CYCLE_FILES_SHOWN) { + console.log(chalk.dim(` ... and ${group.length - CLI_CYCLE_FILES_SHOWN} more (--json lists them all)`)); + } + console.log(); + }); + } + // exitCode, not exit(): a piped --json has to reach its reader whole. + if (cycles.length > 0) process.exitCode = 1; + } finally { + cg.destroy(); + } + } catch (err) { + error(`cycles failed: ${err instanceof Error ? err.message : String(err)}`); + process.exit(1); + } + }); + /** * codegraph install */ diff --git a/src/db/queries.ts b/src/db/queries.ts index 2e242d7ca..5435cf998 100644 --- a/src/db/queries.ts +++ b/src/db/queries.ts @@ -3360,16 +3360,8 @@ export class QueryBuilder { /** * Every ordered pair of files where one reaches into the other, once each. * - * The input a cycle finder wants: file-level circular dependencies are the - * strongly connected components of this graph. One query instead of the - * dependency lookup per file that {@link GraphQueryManager.findCircularDependencies} - * does — which matters because a cycle report is only interesting on a large - * repo, and that is exactly where a query per file stops being affordable. - * * `contains` is excluded (a file "contains" its own symbols, which is not a - * dependency), and so are same-file edges and low-confidence name matches: - * a cycle conjured by a common method name is a false alarm a reader cannot - * check. + * dependency), and so are same-file edges and low-confidence name matches. */ getCrossFileDependencyPairs(minConfidence: number): Array<{ source: string; target: string }> { return this.db @@ -3385,6 +3377,29 @@ export class QueryBuilder { .all(minConfidence) as Array<{ source: string; target: string }>; } + /** + * Every ordered pair of files where one imports the other, once each: the + * graph circular imports are read from. See `src/graph/file-cycles.ts`. + * + * Only `imports` edges, which the import resolver made from a statement in + * the file. Every other cross-file edge says something ran or was mentioned + * there, and a loop of those is not a circular import: an interface's call + * fans out to a test's fake that implements it, two pages link to each other, + * and the files of one Go package call each other freely. + */ + getFileImportPairs(): Array<{ source: string; target: string }> { + return this.db + .prepare( + `SELECT DISTINCT sn.file_path AS source, tn.file_path AS target + FROM edges e + JOIN nodes sn ON sn.id = e.source + JOIN nodes tn ON tn.id = e.target + WHERE e.kind = 'imports' + AND sn.file_path <> tn.file_path` + ) + .all() as Array<{ source: string; target: string }>; + } + /** * Every unresolved reference recorded in one FILE, ordered by line. * diff --git a/src/graph/file-cycles.ts b/src/graph/file-cycles.ts new file mode 100644 index 000000000..de1abc88f --- /dev/null +++ b/src/graph/file-cycles.ts @@ -0,0 +1,89 @@ +/** + * Circular imports between files, read by more than one surface: the + * `codegraph cycles` command (through `CodeGraph.findCircularDependencies`) and + * the viewer's map. + */ + +/** + * The strongly connected components of a file import graph, each with more than + * one file: groups of files that all reach each other through imports. A group + * is not a path. Two files that import each other are a group of two, and a + * 400-file knot is one group of 400, whatever the number of distinct loops that + * run through it, which depends on the order they are walked in. + * + * Files are sorted inside a group and groups by size, then first file, so two + * runs over one index print the same report. + */ +export function findFileCycles(pairs: ReadonlyArray<{ source: string; target: string }>): string[][] { + const adjacency = new Map(); + for (const pair of pairs) { + let out = adjacency.get(pair.source); + if (!out) adjacency.set(pair.source, (out = [])); + out.push(pair.target); + } + // Deterministic iteration: SQLite's DISTINCT ordering is not a contract. + const nodes = [...new Set([...adjacency.keys(), ...[...adjacency.values()].flat()])].sort(); + for (const list of adjacency.values()) list.sort(); + + return tarjan(nodes, (id) => adjacency.get(id) ?? []) + .filter((component) => component.length > 1) + .map((component) => component.slice().sort()) + .sort((a, b) => a.length - b.length || (a[0] ?? '').localeCompare(b[0] ?? '')); +} + +/** Tarjan's strongly connected components, iterative so a deep graph cannot blow the stack. */ +function tarjan(nodes: readonly string[], edgesOf: (id: string) => readonly string[]): string[][] { + const index = new Map(); + const low = new Map(); + const onStack = new Set(); + const stack: string[] = []; + const out: string[][] = []; + let counter = 0; + + for (const start of nodes) { + if (index.has(start)) continue; + const work: Array<{ id: string; edges: readonly string[]; at: number }> = [ + { id: start, edges: edgesOf(start), at: 0 }, + ]; + index.set(start, counter); + low.set(start, counter); + counter += 1; + stack.push(start); + onStack.add(start); + + while (work.length > 0) { + const frame = work[work.length - 1]; + if (frame === undefined) break; + if (frame.at < frame.edges.length) { + const next = frame.edges[frame.at]!; + frame.at += 1; + if (!index.has(next)) { + index.set(next, counter); + low.set(next, counter); + counter += 1; + stack.push(next); + onStack.add(next); + work.push({ id: next, edges: edgesOf(next), at: 0 }); + } else if (onStack.has(next)) { + low.set(frame.id, Math.min(low.get(frame.id) ?? 0, index.get(next) ?? 0)); + } + continue; + } + work.pop(); + if (low.get(frame.id) === index.get(frame.id)) { + const component: string[] = []; + for (;;) { + const popped = stack.pop(); + if (popped === undefined) break; + onStack.delete(popped); + component.push(popped); + if (popped === frame.id) break; + } + out.push(component); + } + const parent = work[work.length - 1]; + if (parent) low.set(parent.id, Math.min(low.get(parent.id) ?? 0, low.get(frame.id) ?? 0)); + } + } + return out; +} diff --git a/src/graph/queries.ts b/src/graph/queries.ts index e2af59335..717f4d662 100644 --- a/src/graph/queries.ts +++ b/src/graph/queries.ts @@ -7,6 +7,7 @@ import { Node, Edge, Context, Subgraph, EdgeKind } from '../types'; import { QueryBuilder } from '../db/queries'; import { GraphTraverser } from './traversal'; +import { findFileCycles } from './file-cycles'; /** * Graph query manager for complex queries @@ -219,48 +220,12 @@ export class GraphQueryManager { } /** - * Find circular dependencies in the graph + * Find circular imports between files * - * @returns Array of cycles, each cycle is an array of node IDs + * @returns Groups of files that all reach each other through imports, smallest first */ findCircularDependencies(): string[][] { - const files = this.queries.getAllFiles(); - const cycles: string[][] = []; - const visited = new Set(); - const recursionStack = new Set(); - - const dfs = (filePath: string, path: string[]): void => { - if (recursionStack.has(filePath)) { - // Found a cycle - const cycleStart = path.indexOf(filePath); - if (cycleStart !== -1) { - cycles.push(path.slice(cycleStart)); - } - return; - } - - if (visited.has(filePath)) { - return; - } - - visited.add(filePath); - recursionStack.add(filePath); - - const dependencies = this.getFileDependencies(filePath); - for (const dep of dependencies) { - dfs(dep, [...path, filePath]); - } - - recursionStack.delete(filePath); - }; - - for (const file of files) { - if (!visited.has(file.path)) { - dfs(file.path, []); - } - } - - return cycles; + return findFileCycles(this.queries.getFileImportPairs()); } /** diff --git a/src/index.ts b/src/index.ts index 777f5efd9..8cff94d95 100644 --- a/src/index.ts +++ b/src/index.ts @@ -2341,13 +2341,21 @@ export class CodeGraph { } /** - * Every ordered pair of files where one reaches into the other — the edge - * list a cycle finder runs on. See {@link QueryBuilder.getCrossFileDependencyPairs}. + * Every ordered pair of files where one reaches into the other. See + * {@link QueryBuilder.getCrossFileDependencyPairs}. */ getFileDependencyPairs(minConfidence = 0): Array<{ source: string; target: string }> { return this.queries.getCrossFileDependencyPairs(minConfidence); } + /** + * Every ordered pair of files where one imports the other — the edge list + * circular imports are found on. See {@link QueryBuilder.getFileImportPairs}. + */ + getFileImportPairs(): Array<{ source: string; target: string }> { + return this.queries.getFileImportPairs(); + } + /** * References from a symbol that never resolved to an indexed node — the * calls and type mentions that leave the index. Lets a reader account for @@ -2962,9 +2970,10 @@ export class CodeGraph { } /** - * Find circular dependencies in the codebase + * Find circular imports between files * - * @returns Array of cycles, each cycle is an array of file paths + * @returns Groups of file paths that all reach each other through imports, + * smallest group first */ findCircularDependencies(): string[][] { return this.graphManager.findCircularDependencies(); diff --git a/src/ui-server/api/map.ts b/src/ui-server/api/map.ts index 5f48bfd9e..c2181b134 100644 --- a/src/ui-server/api/map.ts +++ b/src/ui-server/api/map.ts @@ -36,6 +36,7 @@ import type { CodeGraph } from '../../index'; import type { EdgeKind, Language } from '../../types'; +import { findFileCycles } from '../../graph/file-cycles'; import { isTestFile } from '../../search/query-utils'; import { badRequest } from './respond'; import { UNCERTAIN_BELOW, toPosixPath, wireList, type WireList } from './wire'; @@ -719,11 +720,7 @@ export function buildMap(cg: CodeGraph, projectRoot: string, query: URLSearchPar } } - // ONE fetch of the file edge list, read twice: the cycle finder and the - // dependent counts are both questions about it, and it is the expensive query - // on this screen. - const filePairs = cg.getFileDependencyPairs(UNCERTAIN_BELOW); - const dependents = countDependents(filePairs, moduleOfFile); + const dependents = countDependents(cg.getFileDependencyPairs(UNCERTAIN_BELOW), moduleOfFile); const payload: WireMapPayload = { root, @@ -756,7 +753,7 @@ export function buildMap(cg: CodeGraph, projectRoot: string, query: URLSearchPar links: [...links.values()].sort( (a, b) => a.source.localeCompare(b.source) || a.target.localeCompare(b.target) ), - cycles: fileCycles(filePairs, moduleOfFile), + cycles: fileCycles(cg.getFileImportPairs(), moduleOfFile), excluded: { uncertainEdges, confidenceBelow: UNCERTAIN_BELOW }, index: { lastIndexedAt: cg.getLastIndexedAt(), @@ -807,34 +804,19 @@ function countDependents( } /** - * File-level circular dependencies, as strongly connected components. + * The circular imports inside the chosen root (see {@link findFileCycles}). * - * Tarjan over the one-query file edge list. Components of size 1 are not - * cycles (a file depending on itself is a same-file edge, already excluded), - * and a component longer than {@link MAX_CYCLE_LENGTH} is reported truncated - * rather than printed — a 200-file knot is a fact about the repository, not a - * list anybody reads. + * A component longer than {@link MAX_CYCLE_LENGTH} is reported truncated rather + * than printed — a 200-file knot is a fact about the repository, not a list + * anybody reads. */ function fileCycles( pairs: ReadonlyArray<{ source: string; target: string }>, moduleOfFile: Map ): WireMapPayload['cycles'] { - const adjacency = new Map(); - for (const pair of pairs) { - if (!moduleOfFile.has(pair.source) || !moduleOfFile.has(pair.target)) continue; - let out = adjacency.get(pair.source); - if (!out) adjacency.set(pair.source, (out = [])); - out.push(pair.target); - } - // Deterministic iteration: SQLite's DISTINCT ordering is not a contract. - const nodes = [...new Set([...adjacency.keys(), ...[...adjacency.values()].flat()])].sort(); - for (const list of adjacency.values()) list.sort(); - - const components = tarjan(nodes, (id) => adjacency.get(id) ?? []); - const cycles = components - .filter((component) => component.length > 1) - .map((component) => component.slice().sort()) - .sort((a, b) => a.length - b.length || (a[0] ?? '').localeCompare(b[0] ?? '')); + const cycles = findFileCycles( + pairs.filter((pair) => moduleOfFile.has(pair.source) && moduleOfFile.has(pair.target)) + ); const items = cycles.slice(0, MAX_FILE_CYCLES).map((files) => ({ size: files.length, @@ -849,63 +831,6 @@ function fileCycles( }; } -/** Tarjan's strongly connected components, iterative so a deep graph cannot blow the stack. */ -function tarjan(nodes: readonly string[], edgesOf: (id: string) => readonly string[]): string[][] { - const index = new Map(); - const low = new Map(); - const onStack = new Set(); - const stack: string[] = []; - const out: string[][] = []; - let counter = 0; - - for (const start of nodes) { - if (index.has(start)) continue; - const work: Array<{ id: string; edges: readonly string[]; at: number }> = [ - { id: start, edges: edgesOf(start), at: 0 }, - ]; - index.set(start, counter); - low.set(start, counter); - counter += 1; - stack.push(start); - onStack.add(start); - - while (work.length > 0) { - const frame = work[work.length - 1]; - if (frame === undefined) break; - if (frame.at < frame.edges.length) { - const next = frame.edges[frame.at]!; - frame.at += 1; - if (!index.has(next)) { - index.set(next, counter); - low.set(next, counter); - counter += 1; - stack.push(next); - onStack.add(next); - work.push({ id: next, edges: edgesOf(next), at: 0 }); - } else if (onStack.has(next)) { - low.set(frame.id, Math.min(low.get(frame.id) ?? 0, index.get(next) ?? 0)); - } - continue; - } - work.pop(); - if (low.get(frame.id) === index.get(frame.id)) { - const component: string[] = []; - for (;;) { - const popped = stack.pop(); - if (popped === undefined) break; - onStack.delete(popped); - component.push(popped); - if (popped === frame.id) break; - } - out.push(component); - } - const parent = work[work.length - 1]; - if (parent) low.set(parent.id, Math.min(low.get(parent.id) ?? 0, low.get(frame.id) ?? 0)); - } - } - return out; -} - /** * The roots the selector offers: the repository root plus every top-level * directory that holds indexed files, biggest first.