Skip to content

Commit 2de6550

Browse files
committed
fix(cycles): classify cycles whose only closing edges are speculative
Cycle detection (codegraph cycles / check --cycles / manifesto noCycles) previously treated every call edge identically, so a cycle held together only by a low-confidence dynamic-dispatch guess (dynamic = 1 AND confidence < 1) was reported the same as one backed entirely by confirmed static edges. A single fabricated resolver edge could therefore surface as a "confirmed" architectural cycle and fail CI. findCycles() now runs Tarjan a second time on the edge set with speculative edges removed and diffs the two SCC decompositions — any cycle that only exists in the full run is marked `speculative: true` instead of `confirmed`. Threads `dynamic` through getCallEdges (both the SQLite and native/rusqlite paths) so the classifier has the metadata it needs; the SCC primitive itself (JS and native) is unchanged and reused for both passes, so no engine can diverge on the classification. `codegraph cycles` gains `--exclude-speculative` and annotates speculative cycles in table/JSON output. `check --cycles` and the manifesto `noCycles` rule ignore speculative-only cycles by default via the new `check.excludeSpeculativeCycles` config default.
1 parent c5b9a34 commit 2de6550

18 files changed

Lines changed: 369 additions & 50 deletions

File tree

crates/codegraph-core/src/db/repository/graph_read.rs

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1682,12 +1682,16 @@ impl NativeDatabase {
16821682
}
16831683

16841684
/// Get all 'calls' edges.
1685+
///
1686+
/// Includes `dynamic` alongside `confidence` so consumers (e.g. cycle
1687+
/// detection, #1844) can distinguish confirmed static calls from
1688+
/// low-confidence dynamic-dispatch guesses.
16851689
#[napi]
16861690
pub fn get_call_edges(&self) -> napi::Result<Vec<NativeCallEdgeRow>> {
16871691
let conn = self.conn()?;
16881692
let mut stmt = conn
16891693
.prepare_cached(
1690-
"SELECT source_id, target_id, confidence FROM edges WHERE kind = 'calls' \
1694+
"SELECT source_id, target_id, confidence, dynamic FROM edges WHERE kind = 'calls' \
16911695
ORDER BY source_id, target_id",
16921696
)
16931697
.map_err(|e| napi::Error::from_reason(format!("get_call_edges prepare: {e}")))?;
@@ -1697,6 +1701,7 @@ impl NativeDatabase {
16971701
source_id: row.get("source_id")?,
16981702
target_id: row.get("target_id")?,
16991703
confidence: row.get("confidence")?,
1704+
dynamic: row.get("dynamic")?,
17001705
})
17011706
})
17021707
.map_err(|e| napi::Error::from_reason(format!("get_call_edges: {e}")))?;

crates/codegraph-core/src/db/repository/read_types.rs

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -145,6 +145,11 @@ pub struct NativeCallEdgeRow {
145145
pub source_id: i32,
146146
pub target_id: i32,
147147
pub confidence: Option<f64>,
148+
/// 0 or 1 — flags calls resolved via dynamic dispatch (vs. a direct
149+
/// static reference). Paired with `confidence` lets consumers (e.g.
150+
/// cycle detection, #1844) distinguish confirmed calls from
151+
/// low-confidence dynamic-dispatch guesses.
152+
pub dynamic: u32,
148153
}
149154

150155
/// File node row — mirrors `FileNodeRow`.

docs/examples/CLI.md

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -492,6 +492,14 @@ Found 1 file-level cycle:
492492
src/parser.js -> src/constants.js -> src/parser.js
493493
```
494494

495+
Cycles whose only closing edge is a low-confidence dynamic call (resolved via
496+
a resolver heuristic rather than confirmed statically) are marked
497+
`[speculative]` rather than reported as confirmed structural cycles. Pass
498+
`--exclude-speculative` to drop them from the output entirely, or `--json` to
499+
get a `speculative: boolean` field per cycle. `codegraph check --cycles` and
500+
the manifesto `noCycles` rule ignore speculative-only cycles by default (see
501+
`check.excludeSpeculativeCycles` in [configuration](../guides/configuration.md)).
502+
495503
---
496504

497505
## export — Graph as DOT, Mermaid, JSON, GraphML, GraphSON, or Neo4j CSV

docs/guides/configuration.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -315,6 +315,7 @@ Toggles for the lightweight `codegraph check` command (separate from manifesto).
315315
| Key | Type | Default | Purpose |
316316
|-----|------|---------|---------|
317317
| `cycles` | `boolean` | `true` | Fail if cycles exist. |
318+
| `excludeSpeculativeCycles` | `boolean` | `true` | Don't fail on cycles whose only closing edges are low-confidence dynamic calls (`dynamic = 1 AND confidence < 1`) — resolver guesses rather than confirmed structural dependencies. Also honored by the manifesto `noCycles` rule. |
318319
| `blastRadius` | `number \| null` | `null` | Fail if any function's caller count exceeds this. |
319320
| `signatures` | `boolean` | `true` | Warn on signature changes in the diff. |
320321
| `boundaries` | `boolean` | `true` | Honor the `manifesto.boundaries` rules. |

src/cli/commands/cycles.ts

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import type { Cycle } from '../../domain/graph/cycles.js';
12
import { findCycles, formatCycles } from '../../domain/graph/cycles.js';
23
import { openGraph } from '../shared/open-graph.js';
34
import type { CommandDefinition } from '../types.js';
@@ -10,15 +11,20 @@ export const command: CommandDefinition = {
1011
['--functions', 'Function-level cycle detection'],
1112
['-T, --no-tests', 'Exclude test/spec files'],
1213
['--include-tests', 'Include test/spec files (overrides excludeTests config)'],
14+
[
15+
'--exclude-speculative',
16+
'Exclude cycles whose only closing edges are low-confidence dynamic calls',
17+
],
1318
['-j, --json', 'Output as JSON'],
1419
],
1520
execute(_args, opts, ctx) {
1621
const { db, close } = openGraph(opts as { db?: string });
17-
let cycles: string[][];
22+
let cycles: Cycle[];
1823
try {
1924
cycles = findCycles(db, {
2025
fileLevel: !opts.functions,
2126
noTests: ctx.resolveNoTests(opts),
27+
excludeSpeculative: Boolean(opts.excludeSpeculative),
2228
});
2329
} finally {
2430
close();

src/db/repository/graph-read.ts

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,12 +36,16 @@ export function getCallableNodes(db: BetterSqlite3Database): CallableNodeRow[] {
3636

3737
/**
3838
* Get all 'calls' edges. Ordered for determinism — see `getCallableNodes`.
39+
*
40+
* Includes `dynamic` alongside `confidence` so consumers (e.g. cycle
41+
* detection, #1844) can distinguish confirmed static calls from low-
42+
* confidence dynamic-dispatch guesses.
3943
*/
4044
export function getCallEdges(db: BetterSqlite3Database): CallEdgeRow[] {
4145
return cachedStmt(
4246
_getCallEdgesStmt,
4347
db,
44-
"SELECT source_id, target_id, confidence FROM edges WHERE kind = 'calls' ORDER BY source_id, target_id",
48+
"SELECT source_id, target_id, confidence, dynamic FROM edges WHERE kind = 'calls' ORDER BY source_id, target_id",
4549
).all();
4650
}
4751

src/db/repository/in-memory-repository.ts

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -574,7 +574,12 @@ export class InMemoryRepository extends Repository {
574574
getCallEdges(): CallEdgeRow[] {
575575
return [...this.#edges.values()]
576576
.filter((e) => e.kind === 'calls')
577-
.map((e) => ({ source_id: e.source_id, target_id: e.target_id, confidence: e.confidence }));
577+
.map((e) => ({
578+
source_id: e.source_id,
579+
target_id: e.target_id,
580+
confidence: e.confidence,
581+
dynamic: e.dynamic,
582+
}));
578583
}
579584

580585
getFileNodesAll(): FileNodeRow[] {

src/db/repository/native-repository.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -143,6 +143,7 @@ function toCallEdgeRow(r: NativeCallEdgeRow): CallEdgeRow {
143143
source_id: r.sourceId,
144144
target_id: r.targetId,
145145
confidence: r.confidence,
146+
dynamic: r.dynamic,
146147
};
147148
}
148149

src/domain/graph/cycles.ts

Lines changed: 105 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -3,27 +3,66 @@ import { loadNative } from '../../infrastructure/native.js';
33
import { isTestFile } from '../../infrastructure/test-filter.js';
44
import type { BetterSqlite3Database } from '../../types.js';
55

6-
type Edge = { source: string; target: string };
7-
type DbEdge = { source_id: number; target_id: number };
6+
type Edge = { source: string; target: string; speculative?: boolean };
7+
type DbEdge = {
8+
source_id: number;
9+
target_id: number;
10+
confidence?: number | null;
11+
dynamic?: 0 | 1;
12+
};
13+
14+
/**
15+
* A detected circular dependency, classified by how solid its evidence is.
16+
*/
17+
export interface Cycle {
18+
/** Node labels forming the cycle (file paths for file-level, `name|file` for function-level). */
19+
nodes: string[];
20+
/**
21+
* True when every edge that closes this cycle is a low-confidence dynamic
22+
* resolution (`dynamic = 1 AND confidence < 1`) — i.e. the cycle
23+
* disappears once those resolver guesses are excluded from the graph, so
24+
* it has no confirmed structural basis. See issue #1844.
25+
*/
26+
speculative: boolean;
27+
}
28+
29+
/**
30+
* An edge only counts as a low-confidence dynamic guess — not confirmed
31+
* structural evidence — when it's flagged dynamic *and* the resolver wasn't
32+
* fully confident about it. `confidence == null` is treated as confirmed
33+
* (unknown, not guessed) so a missing value never manufactures a false
34+
* "speculative" classification.
35+
*/
36+
function isSpeculative(e: DbEdge): boolean {
37+
return e.dynamic === 1 && typeof e.confidence === 'number' && e.confidence < 1;
38+
}
839

940
/**
1041
* Build a label-based edge list from DB rows, filtering to known nodes and
1142
* deduplicating. Self-loops are skipped (Tarjan treats them as trivial SCCs).
43+
*
44+
* When multiple DB edges collapse onto the same (source, target) label pair,
45+
* the pair is only marked `speculative` if *every* underlying edge is — one
46+
* confirmed edge between two nodes is enough to make that connection real,
47+
* even if a separate low-confidence dynamic call also happens to link them.
1248
*/
1349
function buildLabelEdges(dbEdges: DbEdge[], idToLabel: Map<number, string>): Edge[] {
14-
const edges: Edge[] = [];
15-
const seen = new Set<string>();
50+
const byPair = new Map<string, Edge>();
1651
for (const e of dbEdges) {
1752
if (e.source_id === e.target_id) continue;
1853
const src = idToLabel.get(e.source_id);
1954
const tgt = idToLabel.get(e.target_id);
2055
if (src === undefined || tgt === undefined) continue;
2156
const key = `${src}\0${tgt}`;
22-
if (seen.has(key)) continue;
23-
seen.add(key);
24-
edges.push({ source: src, target: tgt });
57+
const speculative = isSpeculative(e);
58+
const existing = byPair.get(key);
59+
if (!existing) {
60+
byPair.set(key, { source: src, target: tgt, speculative });
61+
} else if (existing.speculative && !speculative) {
62+
existing.speculative = false;
63+
}
2564
}
26-
return edges;
65+
return [...byPair.values()];
2766
}
2867

2968
function buildFileLevelEdges(db: BetterSqlite3Database, noTests: boolean): Edge[] {
@@ -42,26 +81,68 @@ function buildCallableEdges(db: BetterSqlite3Database, noTests: boolean): Edge[]
4281
return buildLabelEdges(getCallEdges(db), idToLabel);
4382
}
4483

84+
/** Run Tarjan's SCC (native when available, JS fallback otherwise) on a flat edge list. */
85+
function runTarjan(edges: Edge[]): string[][] {
86+
const native = loadNative();
87+
if (native) {
88+
return native.detectCycles(edges) as string[][];
89+
}
90+
return tarjanFromEdges(edges);
91+
}
92+
93+
/** Canonical, order-independent key for an SCC's node set. */
94+
function sccKey(nodes: string[]): string {
95+
return [...nodes].sort().join('\0');
96+
}
97+
98+
/**
99+
* Classify each cycle found in `edges` as confirmed or speculative.
100+
*
101+
* Runs Tarjan once on the full edge set (current behavior), then — only if
102+
* at least one edge is speculative — runs it again on the edges that remain
103+
* once low-confidence dynamic edges are removed. Removing edges can only
104+
* shrink or split SCCs, never grow them, so any full-graph cycle whose exact
105+
* node set doesn't reappear in the filtered run depended on a speculative
106+
* edge to close it.
107+
*/
108+
function classifyCycles(edges: Edge[]): Cycle[] {
109+
const allCycles = runTarjan(edges);
110+
if (allCycles.length === 0) return [];
111+
112+
if (!edges.some((e) => e.speculative)) {
113+
return allCycles.map((nodes) => ({ nodes, speculative: false }));
114+
}
115+
116+
const confirmedEdges = edges.filter((e) => !e.speculative);
117+
const confirmedCycles = runTarjan(confirmedEdges);
118+
const confirmedKeys = new Set(confirmedCycles.map(sccKey));
119+
120+
return allCycles.map((nodes) => ({
121+
nodes,
122+
speculative: !confirmedKeys.has(sccKey(nodes)),
123+
}));
124+
}
125+
45126
/**
46127
* Find cycles using Tarjan's SCC algorithm.
47128
*
48129
* Builds a label-based adjacency list directly from DB rows — no intermediate
49130
* CodeGraph construction. This is O(V + E) with minimal memory overhead.
131+
*
132+
* By default returns every detected cycle, each flagged `speculative` when
133+
* its only structural basis is a low-confidence dynamic edge. Pass
134+
* `excludeSpeculative: true` to drop those from the result entirely.
50135
*/
51136
export function findCycles(
52137
db: BetterSqlite3Database,
53-
opts: { fileLevel?: boolean; noTests?: boolean } = {},
54-
): string[][] {
138+
opts: { fileLevel?: boolean; noTests?: boolean; excludeSpeculative?: boolean } = {},
139+
): Cycle[] {
55140
const fileLevel = opts.fileLevel !== false;
56141
const noTests = opts.noTests || false;
57142

58143
const edges = fileLevel ? buildFileLevelEdges(db, noTests) : buildCallableEdges(db, noTests);
59-
60-
const native = loadNative();
61-
if (native) {
62-
return native.detectCycles(edges) as string[][];
63-
}
64-
return tarjanFromEdges(edges);
144+
const cycles = classifyCycles(edges);
145+
return opts.excludeSpeculative ? cycles.filter((c) => !c.speculative) : cycles;
65146
}
66147

67148
export function findCyclesJS(edges: Edge[]): string[][] {
@@ -136,19 +217,22 @@ function tarjanFromEdges(edges: Edge[]): string[][] {
136217
return sccs;
137218
}
138219

139-
export function formatCycles(cycles: string[][]): string {
220+
export function formatCycles(cycles: Cycle[]): string {
140221
if (cycles.length === 0) {
141222
return 'No circular dependencies detected.';
142223
}
143224

144225
const lines: string[] = [`Found ${cycles.length} circular dependency cycle(s):\n`];
145226
for (let i = 0; i < cycles.length; i++) {
146-
const cycle = cycles[i]!;
147-
lines.push(` Cycle ${i + 1} (${cycle.length} files):`);
148-
for (const file of cycle) {
227+
const { nodes, speculative } = cycles[i]!;
228+
const tag = speculative
229+
? ' [speculative — only closes via a low-confidence dynamic call]'
230+
: '';
231+
lines.push(` Cycle ${i + 1} (${nodes.length} files):${tag}`);
232+
for (const file of nodes) {
149233
lines.push(` -> ${file}`);
150234
}
151-
lines.push(` -> ${cycle[0]} (back to start)`);
235+
lines.push(` -> ${nodes[0]} (back to start)`);
152236
lines.push('');
153237
}
154238
return lines.join('\n');

src/features/check.ts

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import fs from 'node:fs';
33
import path from 'node:path';
44
import { findDbPath, openReadonlyOrFail } from '../db/index.js';
55
import { bfsTransitiveCallers } from '../domain/analysis/impact.js';
6+
import type { Cycle } from '../domain/graph/cycles.js';
67
import { findCycles } from '../domain/graph/cycles.js';
78
import { DEFAULTS, loadConfig } from '../infrastructure/config.js';
89
import { isTestFile } from '../infrastructure/test-filter.js';
@@ -317,16 +318,17 @@ export function parseDiffOutput(diffOutput: string): ParsedDiff {
317318

318319
interface CyclesResult {
319320
passed: boolean;
320-
cycles: string[][];
321+
cycles: Cycle[];
321322
}
322323

323324
export function checkNoNewCycles(
324325
db: BetterSqlite3Database,
325326
changedFiles: Set<string>,
326327
noTests: boolean,
328+
excludeSpeculative: boolean,
327329
): CyclesResult {
328-
const cycles = findCycles(db, { fileLevel: true, noTests });
329-
const involved = cycles.filter((cycle) => cycle.some((f) => changedFiles.has(f)));
330+
const cycles = findCycles(db, { fileLevel: true, noTests, excludeSpeculative });
331+
const involved = cycles.filter((cycle) => cycle.nodes.some((f) => changedFiles.has(f)));
330332
return { passed: involved.length === 0, cycles: involved };
331333
}
332334

@@ -797,6 +799,8 @@ function resolveCheckFlags(opts: CheckOpts, config: CodegraphConfig) {
797799
const checkConfig = config.check || ({} as CodegraphConfig['check']);
798800
return {
799801
enableCycles: opts.cycles ?? checkConfig.cycles ?? true,
802+
excludeSpeculativeCycles:
803+
checkConfig.excludeSpeculativeCycles ?? DEFAULTS.check.excludeSpeculativeCycles,
800804
enableSignatures: opts.signatures ?? checkConfig.signatures ?? true,
801805
enableBoundaries: opts.boundaries ?? checkConfig.boundaries ?? true,
802806
blastRadiusThreshold: opts.blastRadius ?? checkConfig.blastRadius ?? null,
@@ -816,7 +820,10 @@ function runPredicates(
816820
const predicates: PredicateResult[] = [];
817821

818822
if (flags.enableCycles) {
819-
predicates.push({ name: 'cycles', ...checkNoNewCycles(db, changedFiles, noTests) });
823+
predicates.push({
824+
name: 'cycles',
825+
...checkNoNewCycles(db, changedFiles, noTests, flags.excludeSpeculativeCycles),
826+
});
820827
}
821828
if (flags.blastRadiusThreshold != null) {
822829
predicates.push({

0 commit comments

Comments
 (0)