Conversation
…ucture Co-authored-by: Jurie <smit.jurie@gmail.com>
WalkthroughConsolidates shell/PowerShell coverage scripts to a new Python coverage runner with check/measure commands, and introduces shared linting core: common data models, formatters, severity utilities, and a BaseLinterCLI used to refactor Markdown and YAML linters. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (7)
scripts/coverage/runner.py (2)
15-24: Clean data model.The
CoverageResultdataclass is well-structured. Note thaterror_messageis defined but not currently populated byparse_coverage. Consider removing it if not needed, or document its intended future use.
47-69: Consider handling subprocess errors gracefully.If
poetryis not installed or not in PATH,subprocess.runwill raiseFileNotFoundError. While the shell scripts'set -ewill catch this, users invoking the Python module directly might benefit from a clearer error message.🔎 Optional: wrap subprocess call with try-except
def run_tests(self, generate_html: bool = True, generate_xml: bool = True) -> bool: ... + try: result = subprocess.run(cmd, capture_output=False) return result.returncode == 0 + except FileNotFoundError: + print("Error: 'poetry' command not found. Please install Poetry.") + return Falsetools/yaml_lint/linter.py (1)
50-73: LGTM! Directory processing added.The
check_directorymethod correctly discovers YAML files, applies exclusion filters, and processes each file. The logic for filtering excluded directories (line 66) correctly checks path components.The hardcoded
excluded_dirs(line 63) could be made configurable via the constructor config for better flexibility, but this is acceptable for the current refactoring scope.tools/core/cli/severity.py (1)
28-38: Consider adding explicit type hints for better clarity.The function accepts and returns generic
listtypes, but the docstring indicates these should belist[LintIssue]. Adding explicit type hints would improve IDE support and catch type errors earlier.🔎 Proposed type hints
-def filter_issues_by_severity(issues: list, min_severity: IssueSeverity) -> list: +def filter_issues_by_severity(issues: list[LintIssue], min_severity: IssueSeverity) -> list[LintIssue]: """Filter issues to only include those at or above the minimum severity.This would also require importing
LintIssue:from tools.core.models import IssueSeverity, LintIssuetools/core/models/lint_models.py (1)
31-43: Clarify the mixed 1-indexed/0-indexed convention.The docstring states
lineis 1-indexed whilecolumnis 0-indexed. While this may match certain tool conventions, it could lead to confusion. Consider either:
- Documenting this prominently in the module docstring
- Aligning both to 1-indexed (more common for user-facing output)
If intentional (e.g., matching LSP conventions), a brief note explaining why would help future maintainers.
tools/core/cli/formatters.py (1)
107-110: Consider reusingfilter_issues_by_severityfrom severity.py.The severity filtering logic here duplicates what's in
tools/core/cli/severity.py. Using the shared utility would improve DRY adherence.🔎 Proposed change
+from tools.core.cli.severity import filter_issues_by_severity ... # Filter by severity if specified - issues = report.issues - if min_severity is not None: - issues = [i for i in issues if i.severity.value <= min_severity.value] + issues = filter_issues_by_severity(report.issues, min_severity) if min_severity else report.issuesApply the same pattern to
format_report_json(lines 149-152).tools/core/cli/base.py (1)
24-28: Mutable class attribute forfile_extensions.Using
set()as a class-level default is safe here since subclasses override it with new set instances. However, if a subclass were to mutate (e.g.,self.file_extensions.add(...)) instead of replace, it would affect the base class. This is unlikely given the current pattern but worth noting.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
scripts/check-coverage.ps1scripts/check-coverage.shscripts/coverage/__init__.pyscripts/coverage/runner.pyscripts/measure-coverage.ps1scripts/measure-coverage.shtools/core/__init__.pytools/core/cli/__init__.pytools/core/cli/base.pytools/core/cli/formatters.pytools/core/cli/severity.pytools/core/models/__init__.pytools/core/models/lint_models.pytools/markdown_lint/cli.pytools/markdown_lint/linter.pytools/markdown_lint/models.pytools/yaml_lint/cli.pytools/yaml_lint/linter.pytools/yaml_lint/models.py
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: CR
Repo: JustAGhosT/codeflow-engine PR: 0
File: configs/.windsurfrules:0-0
Timestamp: 2025-12-23T20:38:33.968Z
Learning: Applies to configs/CodeFlow/actions/ai_linting_fixer/{agents,workflow}.py : Implement multi-step validation pipeline with syntax checking and regression detection in AI linting
📚 Learning: 2025-12-23T20:38:17.078Z
Learnt from: CR
Repo: JustAGhosT/codeflow-engine PR: 0
File: configs/.cursorrules:0-0
Timestamp: 2025-12-23T20:38:17.078Z
Learning: Maintain code coverage requirements
Applied to files:
scripts/check-coverage.shscripts/check-coverage.ps1
📚 Learning: 2025-12-23T20:38:33.968Z
Learnt from: CR
Repo: JustAGhosT/codeflow-engine PR: 0
File: configs/.windsurfrules:0-0
Timestamp: 2025-12-23T20:38:33.968Z
Learning: Applies to configs/CodeFlow/actions/quality_engine/engine.py : Use platform-specific tool substitution for Windows compatibility in quality engine
Applied to files:
scripts/check-coverage.shscripts/check-coverage.ps1
📚 Learning: 2025-12-23T20:38:44.482Z
Learnt from: CR
Repo: JustAGhosT/codeflow-engine PR: 0
File: docs/AGENT.md:0-0
Timestamp: 2025-12-23T20:38:44.482Z
Learning: Applies to docs/CodeFlow/quality/**/*.py : AI-powered code analysis and quality gates system should be implemented in `CodeFlow/quality/`
Applied to files:
scripts/check-coverage.shscripts/check-coverage.ps1
📚 Learning: 2025-12-23T20:38:33.968Z
Learnt from: CR
Repo: JustAGhosT/codeflow-engine PR: 0
File: configs/.windsurfrules:0-0
Timestamp: 2025-12-23T20:38:33.968Z
Learning: Applies to configs/CodeFlow/actions/quality_engine/engine.py : Implement custom severity classification for issues in quality engine
Applied to files:
tools/core/cli/severity.py
🧬 Code graph analysis (12)
scripts/coverage/__init__.py (1)
scripts/coverage/runner.py (1)
CoverageRunner(27-245)
tools/yaml_lint/models.py (1)
tools/core/models/lint_models.py (3)
FileReport(57-108)IssueSeverity(13-24)LintIssue(28-53)
tools/core/__init__.py (1)
tools/core/models/lint_models.py (3)
FileReport(57-108)IssueSeverity(13-24)LintIssue(28-53)
tools/yaml_lint/cli.py (2)
tools/core/cli/base.py (4)
BaseLinterCLI(17-235)_add_linting_arguments(119-124)create_linter(127-132)run(189-235)tools/yaml_lint/linter.py (1)
YAMLLinter(20-356)
scripts/coverage/runner.py (3)
codeflow_engine/core/validation/result.py (1)
success(51-53)tools/core/cli/base.py (2)
run(189-235)parse_args(134-136)codeflow_engine/core/files/io.py (1)
exists(95-105)
tools/core/cli/formatters.py (1)
tools/core/models/lint_models.py (4)
FileReport(57-108)IssueSeverity(13-24)LintIssue(28-53)has_issues(71-73)
tools/core/cli/__init__.py (3)
tools/core/cli/base.py (1)
BaseLinterCLI(17-235)tools/core/cli/formatters.py (5)
format_issue_json(60-76)format_issue_text(23-57)format_report_json(128-163)format_report_text(79-125)format_summary(166-181)tools/core/cli/severity.py (1)
get_severity_threshold(13-25)
tools/core/cli/base.py (3)
tools/core/cli/formatters.py (2)
format_report_json(128-163)format_report_text(79-125)tools/core/cli/severity.py (1)
get_severity_threshold(13-25)tools/yaml_lint/linter.py (3)
check_file(40-48)check_directory(50-73)fix_files(343-356)
tools/core/cli/severity.py (1)
tools/core/models/lint_models.py (1)
IssueSeverity(13-24)
tools/core/models/__init__.py (1)
tools/core/models/lint_models.py (3)
FileReport(57-108)IssueSeverity(13-24)LintIssue(28-53)
tools/markdown_lint/linter.py (1)
tools/core/models/lint_models.py (3)
FileReport(57-108)IssueSeverity(13-24)LintIssue(28-53)
tools/markdown_lint/models.py (1)
tools/core/models/lint_models.py (3)
FileReport(57-108)IssueSeverity(13-24)LintIssue(28-53)
🔇 Additional comments (31)
scripts/coverage/__init__.py (1)
1-5: LGTM!Clean package initialization that properly exposes the
CoverageRunnerclass. The__all__declaration ensures a well-defined public API.scripts/check-coverage.sh (1)
3-10: LGTM!The delegation to the unified Python runner is clean and preserves backward compatibility with the threshold parameter. The
set -eensures the script exits on any failure from the Python runner.scripts/measure-coverage.sh (1)
3-8: LGTM!Clean delegation to the unified Python coverage runner, consistent with the
check-coverage.shpattern.scripts/measure-coverage.ps1 (1)
2-7: LGTM!Clean delegation to the unified Python runner with appropriate PowerShell error handling via
$ErrorActionPreference = 'Stop'.scripts/check-coverage.ps1 (1)
2-11: LGTM!Clean delegation to the unified Python runner, preserving the threshold parameter for backward compatibility with existing workflows.
scripts/coverage/runner.py (7)
27-45: LGTM!Clean initialization with sensible defaults that match the CLI arguments.
71-87: LGTM!The methods correctly capture subprocess output. Consider logging
stderror checkingreturncodein a future iteration if debugging coverage failures becomes necessary.
89-135: LGTM!The parsing logic correctly extracts coverage metrics using regex. The substring check
self.module_name in lineworks for the expected report format. The sorted low-coverage files by ascending coverage is helpful for prioritization.
137-147: LGTM!Simple formatted output utilities. Minor nit: consider using consistent widths (42 vs 40 characters).
149-183: LGTM!Good logic flow: checks for existing coverage data before running tests, provides clear feedback on pass/fail, and shows module breakdown when coverage is insufficient.
242-245: LGTM!Helper method correctly extracts coverage percentage with a sensible default.
248-295: LGTM!Clean CLI implementation with well-documented arguments. The fallback
return 1on line 291 is technically unreachable due to thechoicesconstraint, but it's harmless defensive code.tools/core/models/__init__.py (1)
1-5: LGTM! Clean re-export pattern.The module provides a clear public API surface for shared linting models, following Python packaging best practices. The explicit
__all__declaration makes the public interface unambiguous.tools/markdown_lint/linter.py (1)
8-8: LGTM! Import path simplified.The import now consistently uses the centralized model definitions from
tools.core.models, removing the previous fallback pattern and aligning with the broader refactoring effort.tools/core/__init__.py (1)
1-5: LGTM! Convenient top-level re-export.This provides a convenient import path for consumers who want to use
from tools.core import ...instead offrom tools.core.models import ..., while maintaining a clean package structure.tools/core/cli/__init__.py (1)
1-21: LGTM! Centralized CLI utilities.The module successfully consolidates CLI infrastructure (base classes, formatters, severity handling) into a single re-export surface, enabling consistent CLI implementations across different linting tools.
tools/yaml_lint/models.py (1)
1-9: LGTM! Models centralized with backwards compatibility.The re-export maintains backwards compatibility while leveraging the centralized model definitions from
tools.core.models. This aligns with the broader refactoring strategy.tools/yaml_lint/linter.py (2)
7-7: LGTM! Import path updated.The import now uses the centralized model definitions, consistent with the broader refactoring.
40-48: LGTM! API compatibility method added.The
check_filemethod provides API compatibility with theBaseLinterCLIinterface while delegating to the existinglint_fileimplementation. This is a clean adapter pattern.tools/markdown_lint/models.py (1)
1-9: LGTM! Models centralized with backwards compatibility.The re-export maintains backwards compatibility while using centralized model definitions, mirroring the pattern used in the YAML linter.
tools/markdown_lint/cli.py (4)
10-16: LGTM! Clean CLI class declaration.The
MarkdownLinterCLIclass properly inherits fromBaseLinterCLIand defines all required metadata attributes, providing clear tool identification and supported file extensions.
18-91: LGTM! Comprehensive linting arguments.The argument definitions are well-structured with:
- Logical grouping under "Linting Options"
- Consistent naming convention using negative flags (
--no-*) withaction="store_false"- Clear help text for each option
- Rule codes referenced in help text (MD022, MD032, etc.) for traceability
93-110: LGTM! Factory method correctly implemented.The
create_lintermethod properly maps CLI arguments to theMarkdownLinterconfiguration dictionary, providing a clean separation between CLI argument parsing and linter instantiation.
113-116: LGTM! Simplified main entry point.The main function cleanly delegates to the CLI class's
run()method, following the new architecture pattern and reducing boilerplate code.tools/core/models/lint_models.py (2)
56-68: Well-structured dataclass with appropriate defaults.Good use of
field(default_factory=list)to avoid mutable default argument issues. Thefixed_contentbeinglist[str] | Noneallows distinguishing between "no fixes applied" and "empty content after fixes".
70-108: Clean convenience API for FileReport.The properties and methods provide a consistent interface for querying issues. The implementation correctly delegates to
get_issues_by_severityfor count methods, avoiding duplication.tools/core/cli/formatters.py (1)
1-20: Well-organized formatting utilities with TTY auto-detection.Good separation of concerns with distinct functions for text vs JSON output, and the ANSI color support with TTY detection (Line 96-97) is a nice touch for usability.
tools/yaml_lint/cli.py (2)
10-16: Clean adoption of the base class pattern.The class-level metadata (
tool_name,tool_description,tool_version,file_extensions) provides a clear contract for the base class, and the implementation follows the abstract method requirements properly.
76-79: Simplified entry point.The refactored
main()is now clean and delegates all logic to the base class. Good use of returning the exit code directly.tools/core/cli/base.py (2)
17-64: Well-designed extensible base class.The template method pattern with
_add_linting_argumentsandcreate_linteras abstract hooks provides good extensibility. The argument groups are logically organized and theArgumentDefaultsHelpFormatterimproves CLI usability.
189-230: Comprehensive run loop with fix workflow support.The
run()method handles the full lifecycle well: argument parsing, path processing, fix application (with dry-run support), output formatting, and exit code determination. The verbose logging for fix operations is helpful.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
@tembo address In tools/yaml_lint/cli.py around lines 62 to 73, the create_linter method builds In tools/core/cli/formatters.py around lines 118-120, the file path is printed In tools/core/cli/base.py around lines 149 to 158, the loop over input paths |
…rove yaml linter config merging
I addressed the feedback and made three fixes to improve the code structure:
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
tools/yaml_lint/cli.py (1)
62-74: Config merging issue has been resolved.The past review concern about incomplete config merging has been addressed. The config dict now properly merges
YAMLLinter.DEFAULT_CONFIGbefore applying CLI argument overrides, ensuring all required keys are present.tools/core/cli/formatters.py (1)
23-63: File path redundancy issue has been resolved.The past review concern about redundant file paths has been addressed. The
omit_fileparameter now allows callers to exclude the file path from individual issue formatting when it's already displayed as a header.tools/core/cli/base.py (2)
138-163: Path existence check has been added.The past review concern about silently ignoring non-existent paths has been addressed. The code now checks
path.exists()and prints a warning to stderr when verbose mode is enabled.
237-245: Exception handling improved, but check for potential NameError.The past review concern about silent exception swallowing has been addressed—exceptions are now printed with tracebacks when verbose.
However, there's a minor edge case: if
parse_args()on line 204 raises an exception (unlikely, but possible),parsed_argswon't be defined when line 240 tries to accessparsed_args.verbose, causing aNameError.🔎 Suggested defensive fix
except KeyboardInterrupt: return 130 except Exception as e: - if parsed_args.verbose > 0: + verbose = getattr(parsed_args, 'verbose', 0) if 'parsed_args' in locals() else 0 + if verbose > 0: import traceback traceback.print_exc() else: print(f"Error: {e}", file=sys.stderr) return 1Alternatively, default verbose to 0:
+ parsed_args = None try: parsed_args = self.parse_args(args) ... except Exception as e: - if parsed_args.verbose > 0: + if parsed_args and parsed_args.verbose > 0:
🧹 Nitpick comments (2)
scripts/coverage/runner.py (2)
71-87: Consider checking subprocess return codes.Both
get_coverage_reportmethods returnstdoutwithout checking if the coverage command succeeded. If the command fails, this could return error messages or empty output, leading to confusing parse failures downstream.🔎 Suggested approach: Check return code and handle errors
def get_coverage_report(self) -> str: """Get the coverage report output.""" result = subprocess.run( ["poetry", "run", "coverage", "report"], capture_output=True, text=True, ) + if result.returncode != 0: + raise RuntimeError(f"Coverage report failed: {result.stderr}") return result.stdout def get_coverage_report_with_missing(self) -> str: """Get the coverage report with missing lines.""" result = subprocess.run( ["poetry", "run", "coverage", "report", "--show-missing"], capture_output=True, text=True, ) + if result.returncode != 0: + raise RuntimeError(f"Coverage report failed: {result.stderr}") return result.stdout
89-135: Consider more precise module name matching.Line 114 uses substring matching (
self.module_name in line), which could theoretically match lines where the module name appears in other contexts. While this is unlikely in typical coverage reports, usingline.startswith(self.module_name)or a more precise pattern would be more robust.🔎 Optional refinement
for line in lines: # Parse TOTAL line if line.startswith("TOTAL"): match = re.search(r"(\d+(?:\.\d+)?)%", line) if match: total_coverage = float(match.group(1)) continue # Parse module lines - if self.module_name in line: + if line.strip().startswith(self.module_name): parts = line.split() if len(parts) >= 4:
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
scripts/coverage/runner.pytools/core/cli/base.pytools/core/cli/formatters.pytools/yaml_lint/cli.py
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: JustAGhosT/codeflow-engine PR: 0
File: docs/AGENT.md:0-0
Timestamp: 2025-12-23T20:38:44.482Z
Learning: Applies to docs/CodeFlow/actions/**/*.py : Actions (50+ automation actions) should be implemented in `CodeFlow/actions/` for PR analysis, issue creation, and quality gates
Learnt from: CR
Repo: JustAGhosT/codeflow-engine PR: 0
File: configs/.windsurfrules:0-0
Timestamp: 2025-12-23T20:38:33.968Z
Learning: Applies to configs/CodeFlow/actions/ai_linting_fixer/{agents,workflow}.py : Implement multi-step validation pipeline with syntax checking and regression detection in AI linting
📚 Learning: 2025-12-23T20:38:33.968Z
Learnt from: CR
Repo: JustAGhosT/codeflow-engine PR: 0
File: configs/.windsurfrules:0-0
Timestamp: 2025-12-23T20:38:33.968Z
Learning: Applies to configs/CodeFlow/actions/ai_linting_fixer/{agents,workflow}.py : Implement multi-step validation pipeline with syntax checking and regression detection in AI linting
Applied to files:
tools/yaml_lint/cli.py
🧬 Code graph analysis (3)
tools/core/cli/base.py (5)
tools/core/cli/formatters.py (2)
format_report_json(134-169)format_report_text(85-131)tools/core/cli/severity.py (1)
get_severity_threshold(13-25)tools/yaml_lint/cli.py (2)
_add_linting_arguments(18-60)create_linter(62-74)tools/yaml_lint/linter.py (3)
check_file(40-48)check_directory(50-73)fix_files(343-356)tools/markdown_lint/linter.py (3)
check_file(53-237)check_directory(820-846)fix_files(848-867)
tools/core/cli/formatters.py (1)
tools/core/models/lint_models.py (3)
IssueSeverity(13-24)LintIssue(28-53)has_issues(71-73)
scripts/coverage/runner.py (2)
tools/core/cli/base.py (2)
run(194-245)parse_args(134-136)codeflow_engine/core/files/io.py (1)
exists(95-105)
🔇 Additional comments (5)
scripts/coverage/runner.py (5)
1-13: LGTM! Clean module structure.The module docstring clearly states the purpose, and imports are appropriate for a cross-platform coverage runner.
15-25: LGTM! Well-structured data model.The dataclass provides clear fields for coverage metrics and status, with appropriate typing.
149-183: LGTM! Proper error handling and exit codes.The method correctly handles test failures, checks for coverage file existence, and returns appropriate exit codes based on threshold compliance.
185-197: Previous issue addressed: Test failures are now handled.The code now captures the test result (line 194) and warns the user if tests failed (lines 195-196), addressing the concern from the previous review. The function still returns 0, which aligns with its purpose as a "measure" operation rather than a quality gate.
250-293: LGTM! Well-structured CLI interface.The argparse setup is clean with clear commands, sensible defaults, and appropriate help messages. Command dispatching is straightforward and correct.
1 participant
Summary
Refactored scripts and tools folder structure to improve modularity, DRY principles, and code organization:
tools/core/for:Key Changes
tools/core/with shared models and CLI utilitiesMotivation
Reduce code duplication, improve maintainability, and create a more consistent tooling ecosystem across different linters and utilities.
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.