fix(cycles): document and test engine parity gap in function-level cycle detection#602
fix(cycles): document and test engine parity gap in function-level cycle detection#602carlos-alm merged 6 commits intomainfrom
Conversation
…cle detection The native (Rust) engine extracts more symbols and call edges than WASM, which changes the function-level dependency graph topology. This causes legitimate differences in cycle counts (e.g. 8 native vs 11 WASM) even though the Tarjan SCC algorithm is identical in both engines. Added documentation in cycles.ts explaining the root cause and a test demonstrating how additional edge precision can break false cycles. Fixes #597
|
Claude finished @carlos-alm's task —— View job PR Review: Function-Level Cycle Detection Engine Parity GapTodo List:
Root Cause AnalysisProblem Being Solved: Documents and tests a known discrepancy where native and WASM engines report different function-level cycle counts (e.g., 8 native vs 11 WASM) on the same codebase. Why This Approach: The root cause is legitimate - native engine extracts more symbols/calls (10883 nodes/4000 calls vs 10857/3986 WASM), leading to better resolution that breaks false cycles. The Tarjan SCC algorithm is identical in both engines; the difference comes from parsing precision. Documentation + tests is the right approach rather than trying to artificially force parity. Risk Assessment: LOW - This is purely documentation and testing of expected behavior, not a code change that could introduce bugs. Backlog Compliance
Code Quality AssessmentDocumentation Quality (
|
Greptile SummaryThis PR documents and tests the known engine parity gap for function-level cycle detection between the native Rust and WASM extractors. The change is purely additive: a JSDoc block is prepended to All feedback from the previous review round has been incorporated:
The two new tests are logically sound: the first correctly demonstrates that replacing Confidence Score: 5/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Codebase] --> B{Engine}
B -->|Native Rust| C[10883 nodes / 4000 calls]
B -->|WASM| D[10857 nodes / 3986 calls]
C --> E[More precise call-edge targets]
D --> F[Ambiguous call-edge targets]
E --> G[Fewer false cycles\ne.g. 8 function-level cycles]
F --> H[More false cycles\ne.g. 11 function-level cycles]
G --> I[File-level parity ✓\nimport resolution is engine-independent]
H --> I
Reviews (3): Last reviewed commit: "fix(test): rename test to reflect edge r..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
tests/graph/cycles.test.ts
Outdated
| it('both engines agree on identical edge sets', () => { | ||
| // The Tarjan SCC algorithm itself is deterministic. Any cycle count | ||
| // difference between engines comes from the graph they are fed, not | ||
| // from the algorithm. | ||
| const edges = [ | ||
| { source: 'a', target: 'b' }, | ||
| { source: 'b', target: 'c' }, | ||
| { source: 'c', target: 'a' }, | ||
| { source: 'd', target: 'e' }, | ||
| { source: 'e', target: 'd' }, | ||
| ]; | ||
| const result1 = findCyclesJS(edges); | ||
| const result2 = findCyclesJS(edges); | ||
| expect(result1).toEqual(result2); | ||
| expect(result1).toHaveLength(2); | ||
| }); |
There was a problem hiding this comment.
The test is named "both engines agree on identical edge sets" and its comment states "The Tarjan SCC algorithm itself is deterministic. Any cycle count difference between engines comes from the graph they are fed, not from the algorithm." However, the test body calls findCyclesJS twice and compares the two results — it never invokes the native engine. This makes result1 and result2 trivially equal (same function, same input, same call) and doesn't actually exercise the stated invariant.
To truly verify that both engines agree on identical edge sets, the test should compare findCyclesJS(edges) against native.detectCycles(edges). That invariant is already covered in the describe.skipIf(!hasNative)('Cycle detection: native vs JS parity', ...) suite above — so either this test should be moved/merged into that suite, or it should be renamed to reflect what it actually tests: "JS implementation is deterministic on repeated calls".
There was a problem hiding this comment.
Fixed. Renamed the test from "both engines agree on identical edge sets" to "JS cycle detection is deterministic on repeated calls" to accurately reflect what it tests (calling findCyclesJS twice). The actual cross-engine parity is already covered in the describe.skipIf(!hasNative) suite above.
tests/graph/cycles.test.ts
Outdated
| // Adding a precise edge a -> d (resolving a previously ambiguous call) | ||
| // removes the a -> c edge, breaking the cycle. |
There was a problem hiding this comment.
a -> d, should be c -> d
The comment reads "Adding a precise edge a -> d (resolving a previously ambiguous call) removes the a -> c edge". Looking at the actual resolvedEdges array below, the change is that the edge c -> a becomes c -> d — i.e. the source is c, not a. The comment incorrectly attributes the disambiguated edge to node a.
| // Adding a precise edge a -> d (resolving a previously ambiguous call) | |
| // removes the a -> c edge, breaking the cycle. | |
| // Adding a precise edge c -> d (resolving a previously ambiguous call) | |
| // removes the c -> a edge, breaking the cycle. |
There was a problem hiding this comment.
Fixed. Updated the comment to correctly say c -> d (resolving the c -> a edge), matching the actual edge data below.
src/domain/graph/cycles.ts
Outdated
| * resolved identically. The gap only manifests at function-level granularity | ||
| * where call-site extraction differs between the Rust and WASM parsers. | ||
| * | ||
| * See: https://github.com/nicobailon/codegraph/issues/597 |
There was a problem hiding this comment.
The JSDoc See: link references https://github.com/nicobailon/codegraph/issues/597, which is a different repository (nicobailon/codegraph) from this one (optave/codegraph). The same cross-repo link also appears in the test file at line 164. If issue #597 exists in this repo, the link should point to https://github.com/optave/codegraph/issues/597. If this is intentional (e.g., the issue was originally filed on a fork), a short note explaining the cross-repo reference would help future readers.
There was a problem hiding this comment.
Fixed. Updated both references (in cycles.ts line 24 and cycles.test.ts line 164) to point to https://github.com/optave/codegraph/issues/597.
…me (#602) - Fix issue link to point to optave/codegraph (not nicobailon/codegraph) - Correct inline comment: disambiguated edge is c -> d, not a -> d - Rename misleading test: "both engines agree on identical edge sets" only called findCyclesJS twice, renamed to "JS cycle detection is deterministic on repeated calls" to reflect what it actually tests
…to fix/cycle-parity-native-wasm-review Impact: 5 functions changed, 14 affected
|
Claude finished @carlos-alm's task —— View job PR Review: Engine Parity Gap Documentation and TestingTodo List:
Greptile Feedback AnalysisAll three issues have been properly addressed:
Comprehensive Code Review⭐ Documentation Quality (
|
tests/graph/cycles.test.ts
Outdated
| it('adding a precise edge can break a false cycle', () => { | ||
| // Demonstrates why native (more edges) can report FEWER cycles than WASM. | ||
| // With ambiguous resolution, a -> b -> c -> a forms a 3-node cycle. | ||
| // Adding a precise edge c -> d (resolving a previously ambiguous call) | ||
| // removes the c -> a edge, breaking the cycle. |
There was a problem hiding this comment.
The test name and its first-line comment both describe the change as adding a precise edge (c -> d), but resolvedEdges has the same number of edges (3) as ambiguousEdges (3) — the c -> a edge is replaced by c -> d, not supplemented. This creates a small conceptual mismatch for anyone reading the test in isolation, even though the inline comment on line 242 ("After resolving: c -> a becomes c -> d") does clarify the intent.
The block comment above (line 153) frames the native engine as resolving "more symbols and call edges," but the scenario modelled here is specifically the wrong-target case (an edge that pointed to the wrong node). Aligning the test title with what it actually demonstrates would make the intent self-evident without relying on the inline comment:
| it('adding a precise edge can break a false cycle', () => { | |
| // Demonstrates why native (more edges) can report FEWER cycles than WASM. | |
| // With ambiguous resolution, a -> b -> c -> a forms a 3-node cycle. | |
| // Adding a precise edge c -> d (resolving a previously ambiguous call) | |
| // removes the c -> a edge, breaking the cycle. | |
| it('resolving an ambiguous call edge to its correct target can break a false cycle', () => { | |
| // Demonstrates why native (more precise edge targets) can report FEWER cycles than WASM. | |
| // With ambiguous resolution, a -> b -> c -> a forms a 3-node cycle. | |
| // Resolving the ambiguous c -> a edge to its correct target c -> d | |
| // breaks the cycle. |
There was a problem hiding this comment.
Fixed. Renamed the test from "adding a precise edge can break a false cycle" to "resolving an ambiguous call edge to its correct target can break a false cycle" and updated the comment to say "more precise edge targets" instead of "more edges", matching the suggestion. The test models a replacement (same edge count), not an addition, and the title now reflects that.
Align test title and comment with actual behavior: the test models resolving an ambiguous call edge to a different target (replacement), not adding a new edge.
1 participant
Summary
src/domain/graph/cycles.tsRoot cause
The native engine extracts 10883 nodes / 4000 calls vs WASM's 10857 nodes / 3986 calls. The additional precision resolves previously ambiguous calls to their correct targets, which breaks false cycles that WASM reports. File-level cycles are unaffected because import resolution is engine-independent.
Test plan
findCyclesJSon identical edge setsFixes #597