Skip to content

refactor: clean up cycles.rs code quality #65

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
userFRM merged 1 commit into from
Feb 27, 2026
Merged

refactor: clean up cycles.rs code quality #65

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

183 changes: 36 additions & 147 deletions crates/rpg-mcp/src/tools.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2938,6 +2938,18 @@ impl RpgServer {
.clone()
.unwrap_or_else(|| "length".to_string());

let excluded_paths = if !params.ignore_rpgignore.unwrap_or(false) {
let ignore_path = self.project_root.join(".rpgignore");
let (gitignore, err) = ignore::gitignore::Gitignore::new(&ignore_path);
if err.is_none() || ignore_path.exists() {
Some(gitignore)
} else {
None
}
} else {
None
};

let config = rpg_nav::cycles::CycleConfig {
max_cycles,
max_cycle_length: params.max_cycle_length.unwrap_or(20),
Expand All @@ -2947,167 +2959,44 @@ impl RpgServer {
include_files: params.include_files.unwrap_or(true),
cross_file_only,
cross_area_only,
ignore_rpgignore: params.ignore_rpgignore.unwrap_or(false),
project_root: Some(self.project_root.clone()),
excluded_paths,
};

let report = rpg_nav::cycles::detect_cycles(graph, &config);

// TOON-optimized output: https://github.com/toon-format/toon

let mut output = format!("{}# Circular Dependencies\n\n", notice);

// Summary: inline object (compact TOON)
// Format: cycles: {total: N, entities: N, files: N, areas: N, cross_file: N, cross_area: N}
output.push_str(&format!(
"cycles: {{total: {}, entities: {}, files: {}, areas: {}, cross_file: {}, cross_area: {}}}\n\n",
report.cycle_count,
report.entities_in_cycles,
report.files_in_cycles,
report.areas_in_cycles,
report.cross_file_count,
report.cross_area_count
));

if report.cycle_count > 0 {
// Length distribution: compact key=value pairs
output.push_str(&format!(
"length_dist: len2={} len3={} len4={} len5+={}\n\n",
report.length_distribution.length_2,
report.length_distribution.length_3,
report.length_distribution.length_4,
report.length_distribution.length_5_plus
));

// Area breakdown: TOON tabular format
// Format: area_breakdown[N]{area,cycles,len2,len3,len4+,files}:
// AreaName,cycles,len2,len3,len4+,files
if !report.area_breakdown.is_empty() {
output.push_str(&format!(
"area_breakdown[{}]{{area,cycles,len2,len3,len4+,files}}:\n",
report.area_breakdown.len()
));
for area in &report.area_breakdown {
output.push_str(&format!(
" {},{},{},{},{},{}\n",
area.area,
area.cycle_count,
area.length_2,
area.length_3,
area.length_4_plus,
area.file_count
));
}
output.push('\n');
}
}

// If no filters applied, show available areas and next step
let has_filters = params.max_cycles.is_some()
|| params.min_cycle_length.unwrap_or(2) > 2
|| params.area.is_some()
|| params.cross_file_only.unwrap_or(false)
|| params.cross_area_only.unwrap_or(false)
|| params.sort_by.is_some();

// Option 1: TOON-ify available_areas with count
if !has_filters {
// Only show areas that have cycles (from area_breakdown, sorted by cycle count).
// Showing all hierarchy areas including cycle-free ones is misleading noise.
let areas: Vec<String> = report
.area_breakdown
.iter()
.map(|ab| ab.area.clone())
.collect();
if !areas.is_empty() {
output.push_str(&format!("available_areas[{}]: ", areas.len()));
output.push_str(&format!("{}\n\n", areas.join(",")));
}
output.push_str(
"---\nnext_step: Use area/max_cycles/cross_file_only/cross_area_only to filter\n",
);
return Ok(output);
}

// Option 2: Show active filters when filters are applied
output.push_str(&format!(
"filters: {{area: {}, max_cycles: {}, cross_file: {}, cross_area: {}}}\n\n",
params.area.as_deref().unwrap_or("-"),
params
.max_cycles
.map(|n| n.to_string())
.unwrap_or_else(|| "-".to_string()),
cross_file_only,
params.cross_area_only.unwrap_or(false)
));

// Cycles: TOON tabular with file:entity format
// Format: cycles[N]{chain,len,files}:
// file:entity->file:entity->file:entity,len=N,files=file1,file2
let display_cycles: Vec<_> = report.cycles.iter().take(max_cycles).collect();

// Handle edge case: max_cycles = 0
if max_cycles == 0 {
output.push_str("cycles[0]: (none requested)\n");
let filter_summary = if has_filters {
Some(format!(
"area: {}, max_cycles: {}, cross_file: {}, cross_area: {}",
params.area.as_deref().unwrap_or("-"),
params
.max_cycles
.map(|n| n.to_string())
.unwrap_or_else(|| "-".to_string()),
cross_file_only,
params.cross_area_only.unwrap_or(false)
))
} else {
output.push_str(&format!(
"cycles[{}]{{chain,len,files}}:\n",
display_cycles.len()
));

for cycle in &display_cycles {
// Convert chain to file:entity format for clarity
let chain: Vec<String> = cycle
.cycle
.iter()
.map(|id| {
if let Some(entity) = graph.entities.get(id) {
let filename = entity
.file
.file_name()
.map(|n| n.to_string_lossy().to_string())
.unwrap_or_else(|| "unknown".to_string());
format!("{}:{}", filename, entity.name)
} else {
id.clone()
}
})
.collect();
let chain_str = chain.join("->");

// Extract just filenames from full paths
let files_str: Vec<String> = cycle
.files
.iter()
.map(|f| {
std::path::Path::new(f)
.file_name()
.map(|n| n.to_string_lossy().to_string())
.unwrap_or_else(|| f.clone())
})
.collect();
let files_display = files_str.join(",");

output.push_str(&format!(
" {},len={},{}\n",
chain_str, cycle.length, files_display
));
}
}

// Only show "...and N more" if max_cycles > 0 and there are actually more cycles
if max_cycles > 0 && report.cycle_count > max_cycles {
output.push_str(&format!(
"\n... and {} more. Use max_cycles to limit.\n",
report.cycle_count - max_cycles
));
}
None
};

output.push_str(
"\n---\nnext_step: Use area/max_cycles/cross_file_only/cross_area_only to filter\n",
);
let opts = rpg_nav::toon::CycleReportOptions {
has_filters,
max_cycles,
filter_summary,
};

Ok(output)
Ok(format!(
"{}{}",
notice,
rpg_nav::toon::format_cycle_report(&report, graph, &opts)
))
}
}

Expand Down
1 change: 1 addition & 0 deletions crates/rpg-nav/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ embeddings = ["dep:fastembed"]
rpg-core.workspace = true
anyhow.workspace = true
globset.workspace = true
ignore.workspace = true
strsim.workspace = true
toon-format.workspace = true
serde.workspace = true
Expand Down
72 changes: 17 additions & 55 deletions crates/rpg-nav/src/cycles.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,26 +9,6 @@ use rpg_core::graph::{EdgeKind, RPGraph};
use serde::Serialize;
use std::collections::{HashMap, HashSet};

fn glob_match(pattern: &str, path: &str) -> bool {
if pattern.contains('*') {
let parts: Vec<&str> = pattern.split('*').collect();
let mut pos = 0;
for part in parts {
if part.is_empty() {
continue;
}
if let Some(i) = path[pos..].find(part) {
pos += i + part.len();
} else {
return false;
}
}
true
} else {
path.contains(pattern)
}
}

/// Edge kinds that represent dependency relationships (not structural containment).
const DEPENDENCY_EDGE_KINDS: &[EdgeKind] = &[
EdgeKind::Imports,
Expand Down Expand Up @@ -76,10 +56,9 @@ pub struct CycleConfig {
pub cross_file_only: bool,
/// Only show cycles that span multiple hierarchy areas.
pub cross_area_only: bool,
/// Ignore .rpgignore rules.
pub ignore_rpgignore: bool,
/// Project root path for .rpgignore lookup.
pub project_root: Option<std::path::PathBuf>,
/// Paths to exclude (loaded from .rpgignore or other source by the caller).
/// Cycles where ALL entities match an excluded path are removed.
pub excluded_paths: Option<ignore::gitignore::Gitignore>,
}

impl Default for CycleConfig {
Expand All @@ -93,8 +72,7 @@ impl Default for CycleConfig {
include_files: true,
cross_file_only: false,
cross_area_only: false,
ignore_rpgignore: false,
project_root: None,
excluded_paths: None,
}
}
}
Expand Down Expand Up @@ -205,35 +183,19 @@ pub fn detect_cycles(graph: &RPGraph, config: &CycleConfig) -> CycleReport {
// Deduplicate cycles (same cycle can be found starting from different nodes)
let mut cycles = deduplicate_cycles(cycles);

// Filter based on .rpgignore if not ignored
if !config.ignore_rpgignore
&& let Some(ref project_root) = config.project_root
{
let rpgignore_path = project_root.join(".rpgignore");
if rpgignore_path.exists()
&& let Ok(patterns) = std::fs::read_to_string(&rpgignore_path)
{
let patterns: Vec<String> = patterns
.lines()
.filter(|l| !l.trim().is_empty() && !l.trim().starts_with('#'))
.map(|l| l.trim().to_string())
.collect();

if !patterns.is_empty() {
cycles.retain(|cycle| {
cycle.cycle.iter().any(|entity_id| {
if let Some(entity) = graph.entities.get(entity_id) {
let file_path = entity.file.display().to_string();
!patterns
.iter()
.any(|p| glob_match(p, &file_path) || file_path.contains(p))
} else {
true
}
})
});
}
}
// Filter out cycles where ALL entities are in excluded paths
if let Some(ref gitignore) = config.excluded_paths {
cycles.retain(|cycle| {
cycle.cycle.iter().any(|entity_id| {
if let Some(entity) = graph.entities.get(entity_id) {
!gitignore
.matched_path_or_any_parents(&entity.file, false)
.is_ignore()
} else {
true // keep cycles with unknown entities
}
})
});
}

// Filter by hierarchy area if specified (supports comma-separated multiple areas)
Expand Down
Loading