diff --git a/src/mergeradar/analysis/context_builder.py b/src/mergeradar/analysis/context_builder.py index 335047a..de75241 100755 --- a/src/mergeradar/analysis/context_builder.py +++ b/src/mergeradar/analysis/context_builder.py @@ -1,6 +1,6 @@ from __future__ import annotations -from mergeradar.config import MergeRadarConfig +from mergeradar.config import DEFAULT_RISKY_CATEGORIES, MergeRadarConfig from mergeradar.models import AnalysisContext, ChangedFile @@ -20,9 +20,7 @@ def build_context( } risky_categories = ( - config.risky_categories - if config is not None - else {"database", "auth", "infra", "config", "api", "deps"} + config.risky_categories if config is not None else DEFAULT_RISKY_CATEGORIES ) return AnalysisContext( repo_path=repo_path, diff --git a/src/mergeradar/cli.py b/src/mergeradar/cli.py index 6e9cb00..a2b42fe 100755 --- a/src/mergeradar/cli.py +++ b/src/mergeradar/cli.py @@ -189,6 +189,10 @@ def _env_check() -> int | None: try: return int(val) except ValueError: + typer.echo( + f"Warning: MERGERADAR_CHECK='{val}' is not a valid integer, ignoring.", + err=True, + ) return None diff --git a/src/mergeradar/config.py b/src/mergeradar/config.py index 82bd732..1d6fb4b 100644 --- a/src/mergeradar/config.py +++ b/src/mergeradar/config.py @@ -7,6 +7,10 @@ from mergeradar.exceptions import ConfigError +DEFAULT_RISKY_CATEGORIES: frozenset[str] = frozenset( + {"database", "auth", "infra", "config", "api", "deps"} +) + @dataclass(slots=True) class RuleOverride: @@ -28,7 +32,7 @@ class MergeRadarConfig: keywords: dict[str, list[str]] = field(default_factory=dict) rule_overrides: dict[str, RuleOverride] = field(default_factory=dict) risky_categories: set[str] = field( - default_factory=lambda: {"database", "auth", "infra", "config", "api", "deps"} + default_factory=lambda: set(DEFAULT_RISKY_CATEGORIES) ) category_headings: dict[str, str] = field(default_factory=dict) custom_rules: list[CustomRuleDef] = field(default_factory=list) @@ -62,12 +66,12 @@ def _parse_required_str_list(raw: object, key: str) -> set[str]: return {str(item) for item in raw} -def _parse_str_dict(raw: object) -> dict[str, str]: - """Convert a TOML table to a dict of strings. Returns empty dict for non-dict input.""" +def _parse_str_dict(raw: object, key: str = "headings") -> dict[str, str]: + """Convert a TOML table to a dict of strings. Raises ConfigError for non-dict input.""" - if isinstance(raw, dict): - return {str(k): str(v) for k, v in raw.items()} - return {} + if not isinstance(raw, dict): + raise ConfigError(f"'{key}' must be a table (key-value pairs).") + return {str(k): str(v) for k, v in raw.items()} def _parse_config(data: dict[str, Any]) -> MergeRadarConfig: @@ -131,6 +135,8 @@ def _parse_config(data: dict[str, Any]) -> MergeRadarConfig: if "risky_categories" in data else {"database", "auth", "infra", "config", "api", "deps"} ), - category_headings=_parse_str_dict(data.get("headings")), + category_headings=( + _parse_str_dict(data["headings"], "headings") if "headings" in data else {} + ), custom_rules=custom_rules, ) diff --git a/src/mergeradar/renderers/markdown.py b/src/mergeradar/renderers/markdown.py index a65a24e..7ff0184 100755 --- a/src/mergeradar/renderers/markdown.py +++ b/src/mergeradar/renderers/markdown.py @@ -37,8 +37,8 @@ def render_markdown(report: RiskReport, config: MergeRadarConfig | None = None) for rule in report.triggered_rules: lines.append(f"- **[{rule.score:+d}] {rule.title}**") if rule.paths: - reason_prefix = rule.reason.split(": ", 1)[0] - lines.append(f" - {reason_prefix}:") + reason_text = rule.reason.split(": ", 1)[0] if ": " in rule.reason else rule.reason + lines.append(f" - {reason_text}:") for path in rule.paths: lines.append(f" - `{path}`") else: diff --git a/src/mergeradar/rules/base.py b/src/mergeradar/rules/base.py index ef4c7c9..d694940 100755 --- a/src/mergeradar/rules/base.py +++ b/src/mergeradar/rules/base.py @@ -69,5 +69,4 @@ def evaluate(self, context: AnalysisContext) -> TriggeredRule | None: if not paths: return None - shown_paths = paths[:3] - return self.trigger(f"{self.reason_prefix}: {', '.join(shown_paths)}", paths=shown_paths) + return self.trigger(f"{self.reason_prefix}: {', '.join(paths)}", paths=paths) diff --git a/src/mergeradar/rules/scope.py b/src/mergeradar/rules/scope.py index 7b4f8ec..7af8e8b 100755 --- a/src/mergeradar/rules/scope.py +++ b/src/mergeradar/rules/scope.py @@ -67,13 +67,18 @@ class LockfileOnlyRule(SimpleRule): def evaluate(self, context: AnalysisContext) -> TriggeredRule | None: """Return a stabilizing result for lockfile-only changes.""" - if not context.changed_files: - return None - + has_lockfile_change = False + has_non_lockfile_change = False for cf in context.changed_files: filename = PurePosixPath(cf.path).name.lower() - if cf.status == "D" or filename not in NORMALIZED_LOCKFILE_FILENAMES: - return None + if filename in NORMALIZED_LOCKFILE_FILENAMES: + if cf.status != "D": + has_lockfile_change = True + else: + has_non_lockfile_change = True + + if not has_lockfile_change or has_non_lockfile_change: + return None return self.trigger("Only lockfile changes detected.") diff --git a/src/mergeradar/utils/imports.py b/src/mergeradar/utils/imports.py index 355899c..3515b53 100644 --- a/src/mergeradar/utils/imports.py +++ b/src/mergeradar/utils/imports.py @@ -53,13 +53,13 @@ def _add_if_resolvable_module(modules: set[str], module_name: str, repo_path: Pa modules.add(module_name) -def parse_imports(content: str) -> set[str]: - """Return the set of absolute module names imported in Python source.""" - - try: - tree = ast.parse(content) - except SyntaxError: - return set() +def _extract_absolute_imports( + tree: ast.AST, + *, + filter_resolvable: bool = False, + repo_path: Path | None = None, +) -> set[str]: + """Extract absolute (level=0) imports from a parsed AST.""" modules: set[str] = set() for node in ast.walk(tree): @@ -71,11 +71,24 @@ def parse_imports(content: str) -> set[str]: modules.add(node.module) for alias in node.names: if alias.name != "*": - modules.add(f"{node.module}.{alias.name}") - + full = f"{node.module}.{alias.name}" + if filter_resolvable and repo_path is not None: + _add_if_resolvable_module(modules, full, repo_path) + else: + modules.add(full) return modules +def parse_imports(content: str) -> set[str]: + """Return the set of absolute module names imported in Python source.""" + + try: + tree = ast.parse(content) + except SyntaxError: + return set() + return _extract_absolute_imports(tree) + + def parse_imports_for_file(file_path: Path, repo_path: Path) -> set[str]: """Return imports from source, including relative imports resolved for a file.""" @@ -89,32 +102,24 @@ def parse_imports_for_file(file_path: Path, repo_path: Path) -> set[str]: except SyntaxError: return set() - modules: set[str] = set() + modules = _extract_absolute_imports(tree, filter_resolvable=True, repo_path=repo_path) + for node in ast.walk(tree): - if isinstance(node, ast.Import): - for alias in node.names: - modules.add(alias.name) - elif isinstance(node, ast.ImportFrom): - if node.level == 0 and node.module is not None: - modules.add(node.module) + if isinstance(node, ast.ImportFrom) and node.level > 0: + base = _relative_import_base(node, file_path, repo_path) + if base is None: + continue + if node.module: + modules.add(f"{base}.{node.module}") + for alias in node.names: + if alias.name != "*": + _add_if_resolvable_module( + modules, f"{base}.{node.module}.{alias.name}", repo_path + ) + else: for alias in node.names: if alias.name != "*": - _add_if_resolvable_module(modules, f"{node.module}.{alias.name}", repo_path) - elif node.level > 0: - base = _relative_import_base(node, file_path, repo_path) - if base is None: - continue - if node.module: - modules.add(f"{base}.{node.module}") - for alias in node.names: - if alias.name != "*": - _add_if_resolvable_module( - modules, f"{base}.{node.module}.{alias.name}", repo_path - ) - else: - for alias in node.names: - if alias.name != "*": - modules.add(f"{base}.{alias.name}") + modules.add(f"{base}.{alias.name}") return modules diff --git a/tests/test_cli.py b/tests/test_cli.py index 39736ab..3d269f9 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -57,6 +57,17 @@ def test_env_var_check_fails(monkeypatch: pytest.MonkeyPatch) -> None: assert result.exit_code == 5 +def test_env_var_check_invalid_warns(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setenv("MERGERADAR_CHECK", "not-a-number") + result = runner.invoke( + app, ["analyze", "--diff-file", "samples/auth-change.diff", "--format", "json"] + ) + + assert result.exit_code == 0 + assert "Warning" in result.stderr + assert "not-a-number" in result.stderr + + def test_env_var_check_overridden_by_cli(monkeypatch: pytest.MonkeyPatch) -> None: monkeypatch.setenv("MERGERADAR_CHECK", "8") result = runner.invoke( diff --git a/tests/test_config.py b/tests/test_config.py index dfaa9ab..3cc27d1 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -95,6 +95,11 @@ def test_parse_risky_categories_rejects_non_list() -> None: _parse_config({"risky_categories": "auth"}) +def test_parse_headings_as_non_dict_raises() -> None: + with pytest.raises(ConfigError, match="headings.*table"): + _parse_config({"headings": "not-a-table"}) + + def test_parse_custom_rule_requires_reason() -> None: with pytest.raises(ConfigError, match="reason must be a string"): _parse_config( diff --git a/tests/test_rules.py b/tests/test_rules.py index c8251b1..ed9816b 100755 --- a/tests/test_rules.py +++ b/tests/test_rules.py @@ -89,6 +89,56 @@ def test_deleted_lockfile_does_not_reduce_risk() -> None: assert not any(rule.id == "stability.lockfile_only" for rule in report.triggered_rules) +def test_lockfile_modify_and_delete_triggers_rule() -> None: + changed_files = enrich_changed_files( + [ + ChangedFile( + path="package-lock.json", + old_path=None, + status="M", + additions=10, + deletions=5, + ), + ChangedFile( + path="yarn.lock", + old_path="yarn.lock", + status="D", + additions=0, + deletions=100, + ), + ] + ) + context = build_context(repo_path=".", changed_files=changed_files) + report = score_context(context) + + assert any(rule.id == "stability.lockfile_only" for rule in report.triggered_rules) + + +def test_lockfile_modify_and_non_lockfile_delete_does_not_trigger_rule() -> None: + changed_files = enrich_changed_files( + [ + ChangedFile( + path="package-lock.json", + old_path=None, + status="M", + additions=10, + deletions=5, + ), + ChangedFile( + path="src/app.py", + old_path="src/app.py", + status="D", + additions=0, + deletions=20, + ), + ] + ) + context = build_context(repo_path=".", changed_files=changed_files) + report = score_context(context) + + assert not any(rule.id == "stability.lockfile_only" for rule in report.triggered_rules) + + def test_docs_only_change_reduces_risk() -> None: changed_files = enrich_changed_files( [