rumdl_lib/
fix_coordinator.rs

1use crate::config::Config;
2use crate::lint_context::LintContext;
3use crate::rule::{LintWarning, Rule};
4use std::collections::hash_map::DefaultHasher;
5use std::collections::{HashMap, HashSet};
6use std::hash::{Hash, Hasher};
7
8/// Maximum number of fix iterations before stopping (same as Ruff)
9const MAX_ITERATIONS: usize = 100;
10
11/// Result of applying fixes iteratively
12///
13/// This struct provides named fields instead of a tuple to prevent
14/// confusion about the meaning of each value.
15#[derive(Debug, Clone)]
16pub struct FixResult {
17    /// Total number of rules that successfully applied fixes
18    pub rules_fixed: usize,
19    /// Number of fix iterations performed
20    pub iterations: usize,
21    /// Number of LintContext instances created during fixing
22    pub context_creations: usize,
23    /// Names of rules that applied fixes
24    pub fixed_rule_names: HashSet<String>,
25    /// Whether the fix process converged (content stabilized)
26    pub converged: bool,
27}
28
29/// Calculate hash of content for convergence detection
30fn hash_content(content: &str) -> u64 {
31    let mut hasher = DefaultHasher::new();
32    content.hash(&mut hasher);
33    hasher.finish()
34}
35
36/// Coordinates rule fixing to minimize the number of passes needed
37pub struct FixCoordinator {
38    /// Rules that should run before others (rule -> rules that depend on it)
39    dependencies: HashMap<&'static str, Vec<&'static str>>,
40}
41
42impl Default for FixCoordinator {
43    fn default() -> Self {
44        Self::new()
45    }
46}
47
48impl FixCoordinator {
49    pub fn new() -> Self {
50        let mut dependencies = HashMap::new();
51
52        // CRITICAL DEPENDENCIES:
53        // These dependencies prevent cascading issues that require multiple passes
54
55        // MD064 (multiple consecutive spaces) MUST run before:
56        // - MD010 (tabs->spaces) - MD010 replaces tabs with multiple spaces (e.g., 4),
57        //   which MD064 would incorrectly collapse back to 1 space if it ran after
58        dependencies.insert("MD064", vec!["MD010"]);
59
60        // MD010 (tabs->spaces) MUST run before:
61        // - MD007 (list indentation) - because tabs affect indent calculation
62        // - MD005 (list indent consistency) - same reason
63        dependencies.insert("MD010", vec!["MD007", "MD005"]);
64
65        // MD013 (line length) MUST run before:
66        // - MD009 (trailing spaces) - line wrapping might add trailing spaces that need cleanup
67        // - MD012 (multiple blanks) - reflowing can affect blank lines
68        // Note: MD013 now trims trailing whitespace during reflow to prevent mid-line spaces
69        dependencies.insert("MD013", vec!["MD009", "MD012"]);
70
71        // MD004 (list style) should run before:
72        // - MD007 (list indentation) - changing markers affects indentation
73        dependencies.insert("MD004", vec!["MD007"]);
74
75        // MD022/MD023 (heading spacing) should run before:
76        // - MD012 (multiple blanks) - heading fixes can affect blank lines
77        dependencies.insert("MD022", vec!["MD012"]);
78        dependencies.insert("MD023", vec!["MD012"]);
79
80        // MD070 (nested fence collision) MUST run before:
81        // - MD040 (code language) - MD070 changes block structure, making orphan fences into content
82        // - MD031 (blanks around fences) - same reason
83        dependencies.insert("MD070", vec!["MD040", "MD031"]);
84
85        Self { dependencies }
86    }
87
88    /// Get the optimal order for running rules based on dependencies
89    pub fn get_optimal_order<'a>(&self, rules: &'a [Box<dyn Rule>]) -> Vec<&'a dyn Rule> {
90        // Build a map of rule names to rules for quick lookup
91        let rule_map: HashMap<&str, &dyn Rule> = rules.iter().map(|r| (r.name(), r.as_ref())).collect();
92
93        // Build reverse dependencies (rule -> rules it depends on)
94        let mut reverse_deps: HashMap<&str, HashSet<&str>> = HashMap::new();
95        for (prereq, dependents) in &self.dependencies {
96            for dependent in dependents {
97                reverse_deps.entry(dependent).or_default().insert(prereq);
98            }
99        }
100
101        // Perform topological sort
102        let mut sorted = Vec::new();
103        let mut visited = HashSet::new();
104        let mut visiting = HashSet::new();
105
106        fn visit<'a>(
107            rule_name: &str,
108            rule_map: &HashMap<&str, &'a dyn Rule>,
109            reverse_deps: &HashMap<&str, HashSet<&str>>,
110            visited: &mut HashSet<String>,
111            visiting: &mut HashSet<String>,
112            sorted: &mut Vec<&'a dyn Rule>,
113        ) {
114            if visited.contains(rule_name) {
115                return;
116            }
117
118            if visiting.contains(rule_name) {
119                // Cycle detected, but we'll just skip it
120                return;
121            }
122
123            visiting.insert(rule_name.to_string());
124
125            // Visit dependencies first
126            if let Some(deps) = reverse_deps.get(rule_name) {
127                for dep in deps {
128                    if rule_map.contains_key(dep) {
129                        visit(dep, rule_map, reverse_deps, visited, visiting, sorted);
130                    }
131                }
132            }
133
134            visiting.remove(rule_name);
135            visited.insert(rule_name.to_string());
136
137            // Add this rule to sorted list
138            if let Some(&rule) = rule_map.get(rule_name) {
139                sorted.push(rule);
140            }
141        }
142
143        // Visit all rules
144        for rule in rules {
145            visit(
146                rule.name(),
147                &rule_map,
148                &reverse_deps,
149                &mut visited,
150                &mut visiting,
151                &mut sorted,
152            );
153        }
154
155        // Add any rules not in dependency graph
156        for rule in rules {
157            if !sorted.iter().any(|r| r.name() == rule.name()) {
158                sorted.push(rule.as_ref());
159            }
160        }
161
162        sorted
163    }
164
165    /// Apply fixes iteratively until no more fixes are needed or max iterations reached.
166    ///
167    /// This implements a Ruff-inspired fix loop that re-checks ALL rules after each fix
168    /// to detect cascading issues (e.g., MD046 creating code blocks that MD040 needs to fix).
169    pub fn apply_fixes_iterative(
170        &self,
171        rules: &[Box<dyn Rule>],
172        _all_warnings: &[LintWarning], // Kept for API compatibility, but we re-check all rules
173        content: &mut String,
174        config: &Config,
175        max_iterations: usize,
176    ) -> Result<FixResult, String> {
177        // Use the minimum of max_iterations parameter and MAX_ITERATIONS constant
178        let max_iterations = max_iterations.min(MAX_ITERATIONS);
179
180        // Get optimal rule order based on dependencies
181        let ordered_rules = self.get_optimal_order(rules);
182
183        let mut total_fixed = 0;
184        let mut total_ctx_creations = 0;
185        let mut iterations = 0;
186        let mut previous_hash = hash_content(content);
187
188        // Track which rules actually applied fixes
189        let mut fixed_rule_names = HashSet::new();
190
191        // Build set of unfixable rules for quick lookup
192        let unfixable_rules: HashSet<&str> = config.global.unfixable.iter().map(|s| s.as_str()).collect();
193
194        // Build set of fixable rules (if specified)
195        let fixable_rules: HashSet<&str> = config.global.fixable.iter().map(|s| s.as_str()).collect();
196        let has_fixable_allowlist = !fixable_rules.is_empty();
197
198        // Ruff-style fix loop: keep applying fixes until content stabilizes
199        while iterations < max_iterations {
200            iterations += 1;
201
202            // Create fresh context for this iteration
203            let ctx = LintContext::new(content, config.markdown_flavor(), None);
204            total_ctx_creations += 1;
205
206            let mut any_fix_applied = false;
207
208            // Check and fix each rule in dependency order
209            for rule in &ordered_rules {
210                // Skip disabled rules
211                if unfixable_rules.contains(rule.name()) {
212                    continue;
213                }
214                if has_fixable_allowlist && !fixable_rules.contains(rule.name()) {
215                    continue;
216                }
217
218                // Check if this rule has any current warnings
219                let warnings = match rule.check(&ctx) {
220                    Ok(w) => w,
221                    Err(_) => continue,
222                };
223
224                if warnings.is_empty() {
225                    continue;
226                }
227
228                // Check if any warnings are fixable
229                let has_fixable = warnings.iter().any(|w| w.fix.is_some());
230                if !has_fixable {
231                    continue;
232                }
233
234                // Apply fix
235                match rule.fix(&ctx) {
236                    Ok(fixed_content) => {
237                        if fixed_content != *content {
238                            *content = fixed_content;
239                            total_fixed += 1;
240                            any_fix_applied = true;
241                            fixed_rule_names.insert(rule.name().to_string());
242
243                            // Break to re-check all rules with the new content
244                            // This is the key difference from the old approach:
245                            // we always restart from the beginning after a fix
246                            break;
247                        }
248                    }
249                    Err(_) => {
250                        // Error applying fix, continue to next rule
251                        continue;
252                    }
253                }
254            }
255
256            // Check if content has stabilized (hash-based convergence)
257            let current_hash = hash_content(content);
258            if current_hash == previous_hash {
259                // Content unchanged - converged!
260                return Ok(FixResult {
261                    rules_fixed: total_fixed,
262                    iterations,
263                    context_creations: total_ctx_creations,
264                    fixed_rule_names,
265                    converged: true,
266                });
267            }
268            previous_hash = current_hash;
269
270            // If no fixes were applied this iteration, we've converged
271            if !any_fix_applied {
272                return Ok(FixResult {
273                    rules_fixed: total_fixed,
274                    iterations,
275                    context_creations: total_ctx_creations,
276                    fixed_rule_names,
277                    converged: true,
278                });
279            }
280        }
281
282        // Hit max iterations - did not converge
283        Ok(FixResult {
284            rules_fixed: total_fixed,
285            iterations,
286            context_creations: total_ctx_creations,
287            fixed_rule_names,
288            converged: false,
289        })
290    }
291}
292
293#[cfg(test)]
294mod tests {
295    use super::*;
296    use crate::config::GlobalConfig;
297    use crate::rule::{Fix, LintError, LintResult, LintWarning, Rule, RuleCategory, Severity};
298    use indexmap::IndexMap;
299    use std::sync::atomic::{AtomicUsize, Ordering};
300
301    /// Mock rule that checks content and applies fixes based on a condition
302    #[derive(Clone)]
303    struct ConditionalFixRule {
304        name: &'static str,
305        /// Function to check if content has issues
306        check_fn: fn(&str) -> bool,
307        /// Function to fix content
308        fix_fn: fn(&str) -> String,
309    }
310
311    impl Rule for ConditionalFixRule {
312        fn name(&self) -> &'static str {
313            self.name
314        }
315
316        fn check(&self, ctx: &LintContext) -> LintResult {
317            if (self.check_fn)(ctx.content) {
318                Ok(vec![LintWarning {
319                    line: 1,
320                    column: 1,
321                    end_line: 1,
322                    end_column: 1,
323                    message: format!("{} issue found", self.name),
324                    rule_name: Some(self.name.to_string()),
325                    severity: Severity::Error,
326                    fix: Some(Fix {
327                        range: 0..0,
328                        replacement: String::new(),
329                    }),
330                }])
331            } else {
332                Ok(vec![])
333            }
334        }
335
336        fn fix(&self, ctx: &LintContext) -> Result<String, LintError> {
337            Ok((self.fix_fn)(ctx.content))
338        }
339
340        fn description(&self) -> &'static str {
341            "Conditional fix rule for testing"
342        }
343
344        fn category(&self) -> RuleCategory {
345            RuleCategory::Whitespace
346        }
347
348        fn as_any(&self) -> &dyn std::any::Any {
349            self
350        }
351    }
352
353    // Simple mock rule for basic tests
354    #[derive(Clone)]
355    struct MockRule {
356        name: &'static str,
357        warnings: Vec<LintWarning>,
358        fix_content: String,
359    }
360
361    impl Rule for MockRule {
362        fn name(&self) -> &'static str {
363            self.name
364        }
365
366        fn check(&self, _ctx: &LintContext) -> LintResult {
367            Ok(self.warnings.clone())
368        }
369
370        fn fix(&self, _ctx: &LintContext) -> Result<String, LintError> {
371            Ok(self.fix_content.clone())
372        }
373
374        fn description(&self) -> &'static str {
375            "Mock rule for testing"
376        }
377
378        fn category(&self) -> RuleCategory {
379            RuleCategory::Whitespace
380        }
381
382        fn as_any(&self) -> &dyn std::any::Any {
383            self
384        }
385    }
386
387    #[test]
388    fn test_dependency_ordering() {
389        let coordinator = FixCoordinator::new();
390
391        let rules: Vec<Box<dyn Rule>> = vec![
392            Box::new(MockRule {
393                name: "MD009",
394                warnings: vec![],
395                fix_content: "".to_string(),
396            }),
397            Box::new(MockRule {
398                name: "MD013",
399                warnings: vec![],
400                fix_content: "".to_string(),
401            }),
402            Box::new(MockRule {
403                name: "MD010",
404                warnings: vec![],
405                fix_content: "".to_string(),
406            }),
407            Box::new(MockRule {
408                name: "MD007",
409                warnings: vec![],
410                fix_content: "".to_string(),
411            }),
412        ];
413
414        let ordered = coordinator.get_optimal_order(&rules);
415        let ordered_names: Vec<&str> = ordered.iter().map(|r| r.name()).collect();
416
417        // MD010 should come before MD007 (dependency)
418        let md010_idx = ordered_names.iter().position(|&n| n == "MD010").unwrap();
419        let md007_idx = ordered_names.iter().position(|&n| n == "MD007").unwrap();
420        assert!(md010_idx < md007_idx, "MD010 should come before MD007");
421
422        // MD013 should come before MD009 (dependency)
423        let md013_idx = ordered_names.iter().position(|&n| n == "MD013").unwrap();
424        let md009_idx = ordered_names.iter().position(|&n| n == "MD009").unwrap();
425        assert!(md013_idx < md009_idx, "MD013 should come before MD009");
426    }
427
428    #[test]
429    fn test_single_rule_fix() {
430        let coordinator = FixCoordinator::new();
431
432        // Rule that removes "BAD" from content
433        let rules: Vec<Box<dyn Rule>> = vec![Box::new(ConditionalFixRule {
434            name: "RemoveBad",
435            check_fn: |content| content.contains("BAD"),
436            fix_fn: |content| content.replace("BAD", "GOOD"),
437        })];
438
439        let mut content = "This is BAD content".to_string();
440        let config = Config {
441            global: GlobalConfig::default(),
442            per_file_ignores: HashMap::new(),
443            per_file_flavor: IndexMap::new(),
444            rules: Default::default(),
445            project_root: None,
446        };
447
448        let result = coordinator
449            .apply_fixes_iterative(&rules, &[], &mut content, &config, 5)
450            .unwrap();
451
452        assert_eq!(content, "This is GOOD content");
453        assert_eq!(result.rules_fixed, 1);
454        assert!(result.converged);
455    }
456
457    #[test]
458    fn test_cascading_fixes() {
459        // Simulates MD046 -> MD040 cascade:
460        // Rule1: converts "INDENT" to "FENCE" (like MD046 converting indented to fenced)
461        // Rule2: converts "FENCE" to "FENCE_LANG" (like MD040 adding language)
462        let coordinator = FixCoordinator::new();
463
464        let rules: Vec<Box<dyn Rule>> = vec![
465            Box::new(ConditionalFixRule {
466                name: "Rule1_IndentToFence",
467                check_fn: |content| content.contains("INDENT"),
468                fix_fn: |content| content.replace("INDENT", "FENCE"),
469            }),
470            Box::new(ConditionalFixRule {
471                name: "Rule2_FenceToLang",
472                check_fn: |content| content.contains("FENCE") && !content.contains("FENCE_LANG"),
473                fix_fn: |content| content.replace("FENCE", "FENCE_LANG"),
474            }),
475        ];
476
477        let mut content = "Code: INDENT".to_string();
478        let config = Config {
479            global: GlobalConfig::default(),
480            per_file_ignores: HashMap::new(),
481            per_file_flavor: IndexMap::new(),
482            rules: Default::default(),
483            project_root: None,
484        };
485
486        let result = coordinator
487            .apply_fixes_iterative(&rules, &[], &mut content, &config, 10)
488            .unwrap();
489
490        // Should reach final state in one run (internally multiple iterations)
491        assert_eq!(content, "Code: FENCE_LANG");
492        assert_eq!(result.rules_fixed, 2);
493        assert!(result.converged);
494        assert!(result.iterations >= 2, "Should take at least 2 iterations for cascade");
495    }
496
497    #[test]
498    fn test_indirect_cascade() {
499        // Simulates MD022 -> MD046 -> MD040 indirect cascade:
500        // Rule1: adds "BLANK" (like MD022 adding blank line)
501        // Rule2: only triggers if "BLANK" present, converts "CODE" to "FENCE"
502        // Rule3: converts "FENCE" to "FENCE_LANG"
503        let coordinator = FixCoordinator::new();
504
505        let rules: Vec<Box<dyn Rule>> = vec![
506            Box::new(ConditionalFixRule {
507                name: "Rule1_AddBlank",
508                check_fn: |content| content.contains("HEADING") && !content.contains("BLANK"),
509                fix_fn: |content| content.replace("HEADING", "HEADING BLANK"),
510            }),
511            Box::new(ConditionalFixRule {
512                name: "Rule2_CodeToFence",
513                // Only detects CODE as issue if BLANK is present (simulates CommonMark rule)
514                check_fn: |content| content.contains("BLANK") && content.contains("CODE"),
515                fix_fn: |content| content.replace("CODE", "FENCE"),
516            }),
517            Box::new(ConditionalFixRule {
518                name: "Rule3_AddLang",
519                check_fn: |content| content.contains("FENCE") && !content.contains("LANG"),
520                fix_fn: |content| content.replace("FENCE", "FENCE_LANG"),
521            }),
522        ];
523
524        let mut content = "HEADING CODE".to_string();
525        let config = Config {
526            global: GlobalConfig::default(),
527            per_file_ignores: HashMap::new(),
528            per_file_flavor: IndexMap::new(),
529            rules: Default::default(),
530            project_root: None,
531        };
532
533        let result = coordinator
534            .apply_fixes_iterative(&rules, &[], &mut content, &config, 10)
535            .unwrap();
536
537        // Key assertion: all fixes applied in single run
538        assert_eq!(content, "HEADING BLANK FENCE_LANG");
539        assert_eq!(result.rules_fixed, 3);
540        assert!(result.converged);
541    }
542
543    #[test]
544    fn test_unfixable_rules_skipped() {
545        let coordinator = FixCoordinator::new();
546
547        let rules: Vec<Box<dyn Rule>> = vec![Box::new(ConditionalFixRule {
548            name: "MD001",
549            check_fn: |content| content.contains("BAD"),
550            fix_fn: |content| content.replace("BAD", "GOOD"),
551        })];
552
553        let mut content = "BAD content".to_string();
554        let mut config = Config {
555            global: GlobalConfig::default(),
556            per_file_ignores: HashMap::new(),
557            per_file_flavor: IndexMap::new(),
558            rules: Default::default(),
559            project_root: None,
560        };
561        config.global.unfixable = vec!["MD001".to_string()];
562
563        let result = coordinator
564            .apply_fixes_iterative(&rules, &[], &mut content, &config, 5)
565            .unwrap();
566
567        assert_eq!(content, "BAD content"); // Should not be changed
568        assert_eq!(result.rules_fixed, 0);
569        assert!(result.converged);
570    }
571
572    #[test]
573    fn test_fixable_allowlist() {
574        let coordinator = FixCoordinator::new();
575
576        let rules: Vec<Box<dyn Rule>> = vec![
577            Box::new(ConditionalFixRule {
578                name: "AllowedRule",
579                check_fn: |content| content.contains("A"),
580                fix_fn: |content| content.replace("A", "X"),
581            }),
582            Box::new(ConditionalFixRule {
583                name: "NotAllowedRule",
584                check_fn: |content| content.contains("B"),
585                fix_fn: |content| content.replace("B", "Y"),
586            }),
587        ];
588
589        let mut content = "AB".to_string();
590        let mut config = Config {
591            global: GlobalConfig::default(),
592            per_file_ignores: HashMap::new(),
593            per_file_flavor: IndexMap::new(),
594            rules: Default::default(),
595            project_root: None,
596        };
597        config.global.fixable = vec!["AllowedRule".to_string()];
598
599        let result = coordinator
600            .apply_fixes_iterative(&rules, &[], &mut content, &config, 5)
601            .unwrap();
602
603        assert_eq!(content, "XB"); // Only A->X, B unchanged
604        assert_eq!(result.rules_fixed, 1);
605    }
606
607    #[test]
608    fn test_max_iterations_limit() {
609        let coordinator = FixCoordinator::new();
610
611        // Rule that always changes content (pathological case)
612        static COUNTER: AtomicUsize = AtomicUsize::new(0);
613
614        #[derive(Clone)]
615        struct AlwaysChangeRule;
616        impl Rule for AlwaysChangeRule {
617            fn name(&self) -> &'static str {
618                "AlwaysChange"
619            }
620            fn check(&self, _: &LintContext) -> LintResult {
621                Ok(vec![LintWarning {
622                    line: 1,
623                    column: 1,
624                    end_line: 1,
625                    end_column: 1,
626                    message: "Always".to_string(),
627                    rule_name: Some("AlwaysChange".to_string()),
628                    severity: Severity::Error,
629                    fix: Some(Fix {
630                        range: 0..0,
631                        replacement: String::new(),
632                    }),
633                }])
634            }
635            fn fix(&self, ctx: &LintContext) -> Result<String, LintError> {
636                COUNTER.fetch_add(1, Ordering::SeqCst);
637                Ok(format!("{}x", ctx.content))
638            }
639            fn description(&self) -> &'static str {
640                "Always changes"
641            }
642            fn category(&self) -> RuleCategory {
643                RuleCategory::Whitespace
644            }
645            fn as_any(&self) -> &dyn std::any::Any {
646                self
647            }
648        }
649
650        COUNTER.store(0, Ordering::SeqCst);
651        let rules: Vec<Box<dyn Rule>> = vec![Box::new(AlwaysChangeRule)];
652
653        let mut content = "test".to_string();
654        let config = Config {
655            global: GlobalConfig::default(),
656            per_file_ignores: HashMap::new(),
657            per_file_flavor: IndexMap::new(),
658            rules: Default::default(),
659            project_root: None,
660        };
661
662        let result = coordinator
663            .apply_fixes_iterative(&rules, &[], &mut content, &config, 5)
664            .unwrap();
665
666        // Should stop at max iterations
667        assert_eq!(result.iterations, 5);
668        assert!(!result.converged);
669        assert_eq!(COUNTER.load(Ordering::SeqCst), 5);
670    }
671
672    #[test]
673    fn test_empty_rules() {
674        let coordinator = FixCoordinator::new();
675        let rules: Vec<Box<dyn Rule>> = vec![];
676
677        let mut content = "unchanged".to_string();
678        let config = Config {
679            global: GlobalConfig::default(),
680            per_file_ignores: HashMap::new(),
681            per_file_flavor: IndexMap::new(),
682            rules: Default::default(),
683            project_root: None,
684        };
685
686        let result = coordinator
687            .apply_fixes_iterative(&rules, &[], &mut content, &config, 5)
688            .unwrap();
689
690        assert_eq!(result.rules_fixed, 0);
691        assert_eq!(result.iterations, 1);
692        assert!(result.converged);
693        assert_eq!(content, "unchanged");
694    }
695
696    #[test]
697    fn test_no_warnings_no_changes() {
698        let coordinator = FixCoordinator::new();
699
700        // Rule that finds no issues
701        let rules: Vec<Box<dyn Rule>> = vec![Box::new(ConditionalFixRule {
702            name: "NoIssues",
703            check_fn: |_| false, // Never finds issues
704            fix_fn: |content| content.to_string(),
705        })];
706
707        let mut content = "clean content".to_string();
708        let config = Config {
709            global: GlobalConfig::default(),
710            per_file_ignores: HashMap::new(),
711            per_file_flavor: IndexMap::new(),
712            rules: Default::default(),
713            project_root: None,
714        };
715
716        let result = coordinator
717            .apply_fixes_iterative(&rules, &[], &mut content, &config, 5)
718            .unwrap();
719
720        assert_eq!(content, "clean content");
721        assert_eq!(result.rules_fixed, 0);
722        assert!(result.converged);
723    }
724
725    #[test]
726    fn test_cyclic_dependencies_handled() {
727        let mut coordinator = FixCoordinator::new();
728
729        // Create a cycle: A -> B -> C -> A
730        coordinator.dependencies.insert("RuleA", vec!["RuleB"]);
731        coordinator.dependencies.insert("RuleB", vec!["RuleC"]);
732        coordinator.dependencies.insert("RuleC", vec!["RuleA"]);
733
734        let rules: Vec<Box<dyn Rule>> = vec![
735            Box::new(MockRule {
736                name: "RuleA",
737                warnings: vec![],
738                fix_content: "".to_string(),
739            }),
740            Box::new(MockRule {
741                name: "RuleB",
742                warnings: vec![],
743                fix_content: "".to_string(),
744            }),
745            Box::new(MockRule {
746                name: "RuleC",
747                warnings: vec![],
748                fix_content: "".to_string(),
749            }),
750        ];
751
752        // Should not panic or infinite loop
753        let ordered = coordinator.get_optimal_order(&rules);
754
755        // Should return all rules despite cycle
756        assert_eq!(ordered.len(), 3);
757    }
758
759    #[test]
760    fn test_fix_is_idempotent() {
761        // This is the key test for issue #271
762        let coordinator = FixCoordinator::new();
763
764        let rules: Vec<Box<dyn Rule>> = vec![
765            Box::new(ConditionalFixRule {
766                name: "Rule1",
767                check_fn: |content| content.contains("A"),
768                fix_fn: |content| content.replace("A", "B"),
769            }),
770            Box::new(ConditionalFixRule {
771                name: "Rule2",
772                check_fn: |content| content.contains("B") && !content.contains("C"),
773                fix_fn: |content| content.replace("B", "BC"),
774            }),
775        ];
776
777        let config = Config {
778            global: GlobalConfig::default(),
779            per_file_ignores: HashMap::new(),
780            per_file_flavor: IndexMap::new(),
781            rules: Default::default(),
782            project_root: None,
783        };
784
785        // First run
786        let mut content1 = "A".to_string();
787        let result1 = coordinator
788            .apply_fixes_iterative(&rules, &[], &mut content1, &config, 10)
789            .unwrap();
790
791        // Second run on same final content
792        let mut content2 = content1.clone();
793        let result2 = coordinator
794            .apply_fixes_iterative(&rules, &[], &mut content2, &config, 10)
795            .unwrap();
796
797        // Should be identical (idempotent)
798        assert_eq!(content1, content2);
799        assert_eq!(result2.rules_fixed, 0, "Second run should fix nothing");
800        assert!(result1.converged);
801        assert!(result2.converged);
802    }
803}