From 05d5acf96dda0d1517a812962327aeadac5df4c4 Mon Sep 17 00:00:00 2001 From: Szymon Iwacz Date: Mon, 10 Aug 2026 09:36:41 +0200 Subject: [PATCH 1/3] Apply ruff format to src and tests --- src/diffrat/analysis.py | 68 ++++++++---------------------- src/diffrat/analysis_backend.py | 1 - src/diffrat/checks.py | 6 +-- src/diffrat/content_hints.py | 16 ++------ src/diffrat/diff_parser.py | 4 +- src/diffrat/json_renderer.py | 21 +++------- src/diffrat/llm_client.py | 5 +-- src/diffrat/report.py | 14 ++----- src/diffrat/review_quality.py | 3 +- tests/test_analysis.py | 73 ++++++++------------------------- tests/test_analysis_backend.py | 12 ++---- tests/test_checks.py | 60 +++++++-------------------- tests/test_content_hints.py | 18 +++----- tests/test_json_renderer.py | 28 ++++--------- tests/test_llm_client.py | 9 +--- tests/test_report.py | 4 +- tests/test_review.py | 8 +--- tests/test_scoring.py | 2 +- 18 files changed, 87 insertions(+), 265 deletions(-) diff --git a/src/diffrat/analysis.py b/src/diffrat/analysis.py index b6099c9..7d37659 100644 --- a/src/diffrat/analysis.py +++ b/src/diffrat/analysis.py @@ -380,8 +380,7 @@ def _build_hints( focus_risk_hint( code="config_or_deps", message=( - "Config or dependency files changed — " - "review install and runtime impact" + "Config or dependency files changed — review install and runtime impact" ), ) ) @@ -395,9 +394,7 @@ def _build_hints( ) security_paths = [ - file_change.path - for file_change in summary.files - if _is_security_sensitive(file_change) + file_change.path for file_change in summary.files if _is_security_sensitive(file_change) ] if security_paths: preview = ", ".join(security_paths[:3]) @@ -420,18 +417,15 @@ def _build_hints( if len(ci_workflow_paths) > 3: preview = f"{preview}, +{len(ci_workflow_paths) - 3} more" has_ci_validator_in_diff = any( - _is_ci_directory_path(file_change.path) - for file_change in summary.files + _is_ci_directory_path(file_change.path) for file_change in summary.files ) if has_ci_validator_in_diff: message = ( - f"CI/workflow paths changed ({preview}) — " - "review CI/workflow changes carefully" + f"CI/workflow paths changed ({preview}) — review CI/workflow changes carefully" ) else: message = ( - f"CI/workflow paths changed ({preview}) — " - "confirm workflow contracts are validated" + f"CI/workflow paths changed ({preview}) — confirm workflow contracts are validated" ) configured_command = None if config is not None: @@ -446,9 +440,7 @@ def _build_hints( ) rename_paths = [ - file_change.path - for file_change in summary.files - if file_change.change_type in {"R", "C"} + file_change.path for file_change in summary.files if file_change.change_type in {"R", "C"} ] if rename_paths: preview = ", ".join(rename_paths[:3]) @@ -476,8 +468,7 @@ def _build_hints( focus_risk_hint( code="source_without_tests", message=( - f"Source changed without tests in diff ({preview}) — " - "confirm test coverage" + f"Source changed without tests in diff ({preview}) — confirm test coverage" ), ) ) @@ -491,8 +482,7 @@ def _build_hints( not has_tests_in_diff and source_additions >= SOURCE_HEAVY_MIN_ADDITIONS and summary.total_additions > 0 - and source_additions * 100 - >= SOURCE_HEAVY_PERCENT_THRESHOLD * summary.total_additions + and source_additions * 100 >= SOURCE_HEAVY_PERCENT_THRESHOLD * summary.total_additions ): hints.append( focus_risk_hint( @@ -505,9 +495,7 @@ def _build_hints( ) ) - non_binary_files = [ - file_change for file_change in summary.files if not file_change.binary - ] + non_binary_files = [file_change for file_change in summary.files if not file_change.binary] if non_binary_files and all( categorize_path(file_change.path) == "tests" for file_change in non_binary_files ): @@ -637,9 +625,7 @@ def _mixed_concerns_hint( return None source_ci_segments = sorted( - segment - for segment, cats in segment_categories.items() - if "source" in cats or "ci" in cats + segment for segment, cats in segment_categories.items() if "source" in cats or "ci" in cats ) if len(source_ci_segments) < MIXED_CONCERNS_MIN_SOURCE_CI_SEGMENTS: return None @@ -720,9 +706,7 @@ def _missing_test_file_hints( hints.append( focus_risk_hint( code="missing_test_file", - message=( - f"Changed {file_change.path} has no {test_rel} on disk" - ), + message=(f"Changed {file_change.path} has no {test_rel} on disk"), path=file_change.path, ) ) @@ -737,9 +721,7 @@ def _lockfile_consistency_hints( ) -> list[FocusRiskHint]: """Emit hints when lockfile and manifest changes are inconsistent.""" changed_lockfiles = [ - file_change.path - for file_change in summary.files - if is_lockfile_path(file_change.path) + file_change.path for file_change in summary.files if is_lockfile_path(file_change.path) ] changed_manifests = [ file_change.path @@ -765,18 +747,14 @@ def _lockfile_consistency_hints( if changed_manifests and not changed_lockfiles and cwd is not None: root = Path(cwd) - lockfiles_on_disk = [ - name for name in sorted(_LOCKFILE_BASENAMES) if (root / name).exists() - ] + lockfiles_on_disk = [name for name in sorted(_LOCKFILE_BASENAMES) if (root / name).exists()] if lockfiles_on_disk: preview = ", ".join(changed_manifests[:3]) if len(changed_manifests) > 3: preview = f"{preview}, +{len(changed_manifests) - 3} more" lockfile_preview = ", ".join(lockfiles_on_disk[:3]) if len(lockfiles_on_disk) > 3: - lockfile_preview = ( - f"{lockfile_preview}, +{len(lockfiles_on_disk) - 3} more" - ) + lockfile_preview = f"{lockfile_preview}, +{len(lockfiles_on_disk) - 3} more" hints.append( focus_risk_hint( code="manifest_without_lockfile", @@ -825,11 +803,7 @@ def _is_config_path( def _is_ci_path(parts_lower: tuple[str, ...]) -> bool: if parts_lower and parts_lower[0] == "ci": return True - return ( - len(parts_lower) >= 2 - and parts_lower[0] == ".github" - and parts_lower[1] == "workflows" - ) + return len(parts_lower) >= 2 and parts_lower[0] == ".github" and parts_lower[1] == "workflows" def _is_ci_directory_path(path: str) -> bool: @@ -841,11 +815,7 @@ def _is_ci_directory_path(path: str) -> bool: def _is_github_workflow_path(path: str) -> bool: posix = PurePosixPath(path.replace("\\", "/")) parts_lower = tuple(part.lower() for part in posix.parts) - return ( - len(parts_lower) >= 2 - and parts_lower[0] == ".github" - and parts_lower[1] == "workflows" - ) + return len(parts_lower) >= 2 and parts_lower[0] == ".github" and parts_lower[1] == "workflows" def _is_docs_path( @@ -912,11 +882,7 @@ def _is_ci_workflow_validator_path(path: str) -> bool: if parts_lower and parts_lower[0] == "ci": return True - if ( - len(parts_lower) >= 2 - and parts_lower[0] == ".github" - and parts_lower[1] == "workflows" - ): + if len(parts_lower) >= 2 and parts_lower[0] == ".github" and parts_lower[1] == "workflows": return True return False diff --git a/src/diffrat/analysis_backend.py b/src/diffrat/analysis_backend.py index 72c37cd..4870d40 100644 --- a/src/diffrat/analysis_backend.py +++ b/src/diffrat/analysis_backend.py @@ -41,4 +41,3 @@ def run_analysis( elif llm_result.error is not None: result = replace(result, llm_error=llm_result.error) return result - diff --git a/src/diffrat/checks.py b/src/diffrat/checks.py index cb26fb9..9fe49a9 100644 --- a/src/diffrat/checks.py +++ b/src/diffrat/checks.py @@ -74,11 +74,7 @@ def bandit_targets_for_paths(paths: list[str]) -> list[str]: def is_pip_audit_dependency_path(path: str) -> bool: """Return True when a changed path should trigger pip-audit.""" - return ( - is_pyproject_path(path) - or is_lockfile_path(path) - or is_dependency_manifest_path(path) - ) + return is_pyproject_path(path) or is_lockfile_path(path) or is_dependency_manifest_path(path) def mypy_targets_for_paths(paths: list[str]) -> list[str]: diff --git a/src/diffrat/content_hints.py b/src/diffrat/content_hints.py index 6e03ca4..0caf7c2 100644 --- a/src/diffrat/content_hints.py +++ b/src/diffrat/content_hints.py @@ -20,9 +20,7 @@ re.compile(r"BEGIN\s+(?:RSA\s+)?PRIVATE\s+KEY", re.IGNORECASE), re.compile(r"\bAKIA[0-9A-Z]{16}\b"), re.compile(r"\bsk-[a-zA-Z0-9]{20,}\b"), - re.compile( - r"""(?i)(?:api[_-]?key|secret|password|token|auth)\s*=\s*['"][^'"]{8,}['"]""" - ), + re.compile(r"""(?i)(?:api[_-]?key|secret|password|token|auth)\s*=\s*['"][^'"]{8,}['"]"""), ) _DEBUG_LEFTOVER_PATTERNS: tuple[re.Pattern[str], ...] = ( @@ -265,9 +263,7 @@ def _is_high_entropy_literal(value: str) -> bool: return False counts = Counter(value) length = len(value) - entropy = -sum( - (count / length) * math.log2(count / length) for count in counts.values() - ) + entropy = -sum((count / length) * math.log2(count / length) for count in counts.values()) return entropy >= _ENTROPY_THRESHOLD_BITS @@ -279,10 +275,7 @@ def _matches_broad_exception(line: str) -> bool: def _matches_hardcoded_url_or_ip(line: str) -> bool: - return ( - _HARDCODED_URL_PATTERN.search(line) is not None - or _IPV4_PATTERN.search(line) is not None - ) + return _HARDCODED_URL_PATTERN.search(line) is not None or _IPV4_PATTERN.search(line) is not None def _long_added_hunk_hints(file_diff: FileDiffContent) -> list[FocusRiskHint]: @@ -355,8 +348,7 @@ def _cli_flag_without_help_hints(file_diff: FileDiffContent) -> list[FocusRiskHi focus_risk_hint( code="cli_flag_without_help", message=( - f"CLI flag added without help text in {file_diff.path}: " - f"{text.strip()}" + f"CLI flag added without help text in {file_diff.path}: {text.strip()}" ), path=file_diff.path, line=line_no, diff --git a/src/diffrat/diff_parser.py b/src/diffrat/diff_parser.py index 54b4693..f369576 100644 --- a/src/diffrat/diff_parser.py +++ b/src/diffrat/diff_parser.py @@ -161,9 +161,7 @@ def parse_unified_diff( file_blocks = _split_patch_into_file_blocks(patch) if only_paths is not None: filtered_blocks = [ - block - for block in file_blocks - if _extract_path_from_block(block) in only_paths + block for block in file_blocks if _extract_path_from_block(block) in only_paths ] filtered_files = [ _parse_file_block( diff --git a/src/diffrat/json_renderer.py b/src/diffrat/json_renderer.py index a087489..b53d2ec 100644 --- a/src/diffrat/json_renderer.py +++ b/src/diffrat/json_renderer.py @@ -51,15 +51,9 @@ def render_review_json( brief: bool = False, ) -> str: """Render a review report as a JSON document for stdout.""" - result = ( - analysis - if analysis is not None - else run_analysis(summary, diff_content=diff_content) - ) - - sorted_entries = sort_file_entries( - summary.files, result.categories, result.risk_scores - ) + result = analysis if analysis is not None else run_analysis(summary, diff_content=diff_content) + + sorted_entries = sort_file_entries(summary.files, result.categories, result.risk_scores) sorted_paths = [entry[0].path for entry in sorted_entries] review_order = [entry[0].path for entry in review_order_entries(sorted_entries)] @@ -103,10 +97,7 @@ def render_review_json( ], "review_order": review_order, "files_by_category": files_by_category_mapping(sorted_entries), - "focus_risk": [ - _serialize_focus_risk_hint(hint) - for hint in sort_hints(list(result.hints)) - ], + "focus_risk": [_serialize_focus_risk_hint(hint) for hint in sort_hints(list(result.hints))], "review_quality": { "pillars": [ { @@ -181,9 +172,7 @@ def _serialize_changes( limits = { "max_files": MAX_CHANGE_FILES, "max_lines_per_file": ( - max_lines_per_file_limit - if max_lines_per_file_limit is not None - else MAX_LINES_PER_FILE + max_lines_per_file_limit if max_lines_per_file_limit is not None else MAX_LINES_PER_FILE ), } if diff_content is None: diff --git a/src/diffrat/llm_client.py b/src/diffrat/llm_client.py index 7fe54b1..c75220e 100644 --- a/src/diffrat/llm_client.py +++ b/src/diffrat/llm_client.py @@ -204,10 +204,7 @@ def _http_error_message(exc: urllib.error.HTTPError) -> str: if exc.code in (401, 403): if api_message: return f"LLM authentication failed (HTTP {exc.code}): {api_message}" - return ( - f"LLM authentication failed (HTTP {exc.code}) — check " - "DIFFRAT_LLM_API_KEY" - ) + return f"LLM authentication failed (HTTP {exc.code}) — check DIFFRAT_LLM_API_KEY" if exc.code == 404: if api_message: diff --git a/src/diffrat/report.py b/src/diffrat/report.py index 26e9972..a197bd9 100644 --- a/src/diffrat/report.py +++ b/src/diffrat/report.py @@ -25,11 +25,7 @@ def render_review_report( brief: bool = False, ) -> str: """Render a review-oriented text report for stdout.""" - result = ( - analysis - if analysis is not None - else run_analysis(summary, diff_content=diff_content) - ) + result = analysis if analysis is not None else run_analysis(summary, diff_content=diff_content) lines = [ "Review Report", @@ -85,9 +81,7 @@ def render_review_report( review_order_entries(sorted_entries), start=1 ): if file_change.binary: - lines.append( - f"{rank}. {file_change.path} [{category}] (binary)" - ) + lines.append(f"{rank}. {file_change.path} [{category}] (binary)") else: lines.append( f"{rank}. {file_change.path} [{category}] " @@ -152,9 +146,7 @@ def _format_file_line( risk_score: int, ) -> str: if file_change.binary: - return ( - f" {file_change.path} [{category}] risk={risk_score} (binary)" - ) + return f" {file_change.path} [{category}] risk={risk_score} (binary)" return ( f" {file_change.path} [{category}] risk={risk_score} " f"+{file_change.additions} -{file_change.deletions}" diff --git a/src/diffrat/review_quality.py b/src/diffrat/review_quality.py index 12c3d02..ebc2a16 100644 --- a/src/diffrat/review_quality.py +++ b/src/diffrat/review_quality.py @@ -103,8 +103,7 @@ def assert_registry_pillar_coverage() -> None: missing = sorted(set(HINT_SEVERITY_REGISTRY) - set(CODE_TO_PILLAR)) if missing: raise ValueError( - "HINT_SEVERITY_REGISTRY codes missing from CODE_TO_PILLAR: " - + ", ".join(missing) + "HINT_SEVERITY_REGISTRY codes missing from CODE_TO_PILLAR: " + ", ".join(missing) ) diff --git a/tests/test_analysis.py b/tests/test_analysis.py index 11c219b..7f682cd 100644 --- a/tests/test_analysis.py +++ b/tests/test_analysis.py @@ -133,17 +133,12 @@ def test_analyze_diff_ci_workflow_paths_hint_includes_configured_command() -> No ci_hints = [hint for hint in result.hints if hint.code == "ci_workflow_paths"] assert len(ci_hints) == 1 - assert ( - "python ci/validate-workflow-contracts.py --mode project" - in ci_hints[0].message - ) + assert "python ci/validate-workflow-contracts.py --mode project" in ci_hints[0].message def test_analyze_diff_no_ci_workflow_hint_for_source_only() -> None: summary = DiffSummary( - files=( - FileChange(path="src/diffrat/review.py", additions=5, deletions=0, binary=False), - ) + files=(FileChange(path="src/diffrat/review.py", additions=5, deletions=0, binary=False),) ) result = analyze_diff(summary) @@ -180,9 +175,7 @@ def test_analyze_diff_rename_or_move_hint() -> None: def test_analyze_diff_no_rename_or_move_hint_for_modify_only() -> None: summary = DiffSummary( - files=( - FileChange(path="src/foo.py", additions=1, deletions=0, binary=False), - ) + files=(FileChange(path="src/foo.py", additions=1, deletions=0, binary=False),) ) result = analyze_diff(summary) @@ -310,9 +303,7 @@ def test_analyze_diff_workflow_without_ci_validator_hint() -> None: result = analyze_diff(summary) - assert not any( - hint.code == "workflow_without_ci_validator" for hint in result.hints - ) + assert not any(hint.code == "workflow_without_ci_validator" for hint in result.hints) ci_hints = [hint for hint in result.hints if hint.code == "ci_workflow_paths"] assert len(ci_hints) == 1 assert ".github/workflows/validate-workflow-contracts.yml" in ci_hints[0].message @@ -340,9 +331,7 @@ def test_analyze_diff_no_workflow_without_ci_validator_when_ci_changed() -> None result = analyze_diff(summary) - assert not any( - hint.code == "workflow_without_ci_validator" for hint in result.hints - ) + assert not any(hint.code == "workflow_without_ci_validator" for hint in result.hints) assert any(hint.code == "ci_workflow_paths" for hint in result.hints) @@ -363,9 +352,7 @@ def test_analyze_diff_workflow_only_single_ci_hint() -> None: ci_hints = [hint for hint in result.hints if hint.code == "ci_workflow_paths"] assert len(ci_hints) == 1 assert not any(hint.code == "config_or_deps" for hint in result.hints) - assert not any( - hint.code == "workflow_without_ci_validator" for hint in result.hints - ) + assert not any(hint.code == "workflow_without_ci_validator" for hint in result.hints) def test_analyze_diff_large_single_file_hint() -> None: @@ -439,9 +426,7 @@ def test_analyze_diff_no_deletions_heavy_when_additions_exceed_deletions() -> No def test_analyze_diff_generated_file_touched_hint() -> None: summary = DiffSummary( - files=( - FileChange(path="api_pb2.py", additions=10, deletions=2, binary=False), - ) + files=(FileChange(path="api_pb2.py", additions=10, deletions=2, binary=False),) ) result = analyze_diff(summary) @@ -466,19 +451,13 @@ def test_analyze_diff_no_generated_file_touched_when_source_in_diff() -> None: def test_analyze_diff_generated_file_touched_for_lockfile_without_manifest() -> None: summary = DiffSummary( - files=( - FileChange(path="yarn.lock", additions=50, deletions=10, binary=False), - ) + files=(FileChange(path="yarn.lock", additions=50, deletions=10, binary=False),) ) result = analyze_diff(summary) - generated_hints = [ - hint for hint in result.hints if hint.code == "generated_file_touched" - ] - lockfile_hints = [ - hint for hint in result.hints if hint.code == "lockfile_without_manifest" - ] + generated_hints = [hint for hint in result.hints if hint.code == "generated_file_touched"] + lockfile_hints = [hint for hint in result.hints if hint.code == "lockfile_without_manifest"] assert len(generated_hints) == 1 assert "yarn.lock" in generated_hints[0].message assert len(lockfile_hints) == 1 @@ -532,9 +511,7 @@ def test_analyze_diff_missing_test_file_hint_when_test_absent(tmp_path: Path) -> (tmp_path / "src" / "diffrat" / "foo.py").write_text("x = 1\n") summary = DiffSummary( - files=( - FileChange(path="src/diffrat/foo.py", additions=1, deletions=0, binary=False), - ) + files=(FileChange(path="src/diffrat/foo.py", additions=1, deletions=0, binary=False),) ) result = analyze_diff(summary, cwd=str(tmp_path)) @@ -576,9 +553,7 @@ def test_analyze_diff_no_missing_test_file_when_test_exists(tmp_path: Path) -> N (tmp_path / "tests" / "test_foo.py").write_text("def test_foo() -> None: pass\n") summary = DiffSummary( - files=( - FileChange(path="src/diffrat/foo.py", additions=1, deletions=0, binary=False), - ) + files=(FileChange(path="src/diffrat/foo.py", additions=1, deletions=0, binary=False),) ) result = analyze_diff(summary, cwd=str(tmp_path)) @@ -588,9 +563,7 @@ def test_analyze_diff_no_missing_test_file_when_test_exists(tmp_path: Path) -> N def test_analyze_diff_no_missing_test_file_without_cwd() -> None: summary = DiffSummary( - files=( - FileChange(path="src/diffrat/foo.py", additions=1, deletions=0, binary=False), - ) + files=(FileChange(path="src/diffrat/foo.py", additions=1, deletions=0, binary=False),) ) result = analyze_diff(summary) @@ -603,9 +576,7 @@ def test_analyze_diff_no_missing_test_file_for_test_only_change(tmp_path: Path) (tmp_path / "tests" / "test_foo.py").write_text("def test_foo() -> None: pass\n") summary = DiffSummary( - files=( - FileChange(path="tests/test_foo.py", additions=1, deletions=0, binary=False), - ) + files=(FileChange(path="tests/test_foo.py", additions=1, deletions=0, binary=False),) ) result = analyze_diff(summary, cwd=str(tmp_path)) @@ -615,9 +586,7 @@ def test_analyze_diff_no_missing_test_file_for_test_only_change(tmp_path: Path) def test_analyze_diff_lockfile_without_manifest_hint() -> None: summary = DiffSummary( - files=( - FileChange(path="poetry.lock", additions=10, deletions=5, binary=False), - ) + files=(FileChange(path="poetry.lock", additions=10, deletions=5, binary=False),) ) result = analyze_diff(summary) @@ -646,9 +615,7 @@ def test_analyze_diff_manifest_without_lockfile_when_lockfile_on_disk( (tmp_path / "poetry.lock").write_text("lock\n") summary = DiffSummary( - files=( - FileChange(path="pyproject.toml", additions=1, deletions=0, binary=False), - ) + files=(FileChange(path="pyproject.toml", additions=1, deletions=0, binary=False),) ) result = analyze_diff(summary, cwd=str(tmp_path)) @@ -665,9 +632,7 @@ def test_analyze_diff_no_manifest_without_lockfile_when_no_lockfile_on_disk( (tmp_path / "pyproject.toml").write_text("[project]\n") summary = DiffSummary( - files=( - FileChange(path="pyproject.toml", additions=1, deletions=0, binary=False), - ) + files=(FileChange(path="pyproject.toml", additions=1, deletions=0, binary=False),) ) result = analyze_diff(summary, cwd=str(tmp_path)) @@ -695,9 +660,7 @@ def test_analyze_diff_no_manifest_without_lockfile_when_lockfile_changed( def test_analyze_diff_no_manifest_without_lockfile_without_cwd() -> None: summary = DiffSummary( - files=( - FileChange(path="pyproject.toml", additions=1, deletions=0, binary=False), - ) + files=(FileChange(path="pyproject.toml", additions=1, deletions=0, binary=False),) ) result = analyze_diff(summary) diff --git a/tests/test_analysis_backend.py b/tests/test_analysis_backend.py index 3e5d6b3..a8c0085 100644 --- a/tests/test_analysis_backend.py +++ b/tests/test_analysis_backend.py @@ -82,9 +82,7 @@ def fake_run_llm( truncated_files=False, ) summary = DiffSummary( - files=( - FileChange(path="src/diffrat/review.py", additions=1, deletions=0, binary=False), - ) + files=(FileChange(path="src/diffrat/review.py", additions=1, deletions=0, binary=False),) ) llm_config = LlmConfig(enabled=True, provider="openai", api_key="sk-test") @@ -108,9 +106,7 @@ def test_run_analysis_propagates_llm_error( ) summary = DiffSummary( - files=( - FileChange(path="src/diffrat/review.py", additions=1, deletions=0, binary=False), - ) + files=(FileChange(path="src/diffrat/review.py", additions=1, deletions=0, binary=False),) ) llm_config = LlmConfig(enabled=True, provider="openai", api_key="sk-test") @@ -128,9 +124,7 @@ def test_run_analysis_loads_llm_config_from_env_by_default( monkeypatch.delenv("DIFFRAT_LLM_BASE_URL", raising=False) summary = DiffSummary( - files=( - FileChange(path="src/diffrat/review.py", additions=1, deletions=0, binary=False), - ) + files=(FileChange(path="src/diffrat/review.py", additions=1, deletions=0, binary=False),) ) expected = analyze_diff(summary) diff --git a/tests/test_checks.py b/tests/test_checks.py index c5870d5..7728d07 100644 --- a/tests/test_checks.py +++ b/tests/test_checks.py @@ -58,9 +58,7 @@ def test_plan_checks_ci_validator_uses_config_override() -> None: specs = plan_checks(summary, config=config) - assert specs[0].display_command == ( - "python ci/validate-workflow-contracts.py --mode strict" - ) + assert specs[0].display_command == ("python ci/validate-workflow-contracts.py --mode strict") assert specs[0].argv == ( sys.executable, "ci/validate-workflow-contracts.py", @@ -71,9 +69,7 @@ def test_plan_checks_ci_validator_uses_config_override() -> None: def test_plan_checks_ignores_non_ci_validator_config_overrides() -> None: summary = DiffSummary( - files=( - FileChange(path="src/diffrat/review.py", additions=1, deletions=0, binary=False), - ) + files=(FileChange(path="src/diffrat/review.py", additions=1, deletions=0, binary=False),) ) config = DiffratConfig( checks={"pytest": "custom-pytest"}, @@ -105,9 +101,7 @@ def test_pytest_targets_maps_source_to_test_module() -> None: def test_pytest_targets_maps_non_diffrat_src_package() -> None: - assert pytest_targets_for_paths(["src/otherpkg/widget.py"]) == [ - "tests/test_widget.py" - ] + assert pytest_targets_for_paths(["src/otherpkg/widget.py"]) == ["tests/test_widget.py"] def test_pytest_targets_uses_test_module_directly() -> None: @@ -147,9 +141,7 @@ def test_mypy_targets_maps_source_modules() -> None: def test_mypy_targets_maps_non_diffrat_src_package() -> None: - assert mypy_targets_for_paths(["src/otherpkg/widget.py"]) == [ - "src/otherpkg/widget.py" - ] + assert mypy_targets_for_paths(["src/otherpkg/widget.py"]) == ["src/otherpkg/widget.py"] def test_mypy_targets_skips_tests_and_other_paths() -> None: @@ -167,9 +159,7 @@ def test_mypy_targets_deduplicates_and_sorts() -> None: def test_plan_checks_selects_mypy_for_source_paths() -> None: summary = DiffSummary( - files=( - FileChange(path="src/diffrat/review.py", additions=1, deletions=0, binary=False), - ) + files=(FileChange(path="src/diffrat/review.py", additions=1, deletions=0, binary=False),) ) specs = plan_checks(summary) @@ -239,16 +229,12 @@ def test_plan_checks_selects_ci_validator_when_configured() -> None: specs = plan_checks(summary, config=config) assert [spec.code for spec in specs] == ["ci_validator"] - assert specs[0].display_command == ( - "python ci/validate-workflow-contracts.py --mode project" - ) + assert specs[0].display_command == ("python ci/validate-workflow-contracts.py --mode project") def test_plan_checks_selects_pytest_for_source_paths() -> None: summary = DiffSummary( - files=( - FileChange(path="src/diffrat/review.py", additions=1, deletions=0, binary=False), - ) + files=(FileChange(path="src/diffrat/review.py", additions=1, deletions=0, binary=False),) ) specs = plan_checks(summary) @@ -305,16 +291,12 @@ def test_plan_checks_ci_validator_with_python_paths_when_configured() -> None: specs = plan_checks(summary, config=config) assert [spec.code for spec in specs] == ["ci_validator", "pytest", "mypy", "bandit"] - assert specs[0].display_command == ( - "python ci/validate-workflow-contracts.py --mode project" - ) + assert specs[0].display_command == ("python ci/validate-workflow-contracts.py --mode project") def test_plan_checks_selects_ruff_for_pyproject() -> None: summary = DiffSummary( - files=( - FileChange(path="pyproject.toml", additions=1, deletions=0, binary=False), - ) + files=(FileChange(path="pyproject.toml", additions=1, deletions=0, binary=False),) ) specs = plan_checks(summary) @@ -325,9 +307,7 @@ def test_plan_checks_selects_ruff_for_pyproject() -> None: def test_plan_checks_skips_ruff_for_other_config() -> None: summary = DiffSummary( - files=( - FileChange(path="requirements.txt", additions=1, deletions=0, binary=False), - ) + files=(FileChange(path="requirements.txt", additions=1, deletions=0, binary=False),) ) with patch("diffrat.checks.shutil.which", return_value=None): @@ -382,9 +362,7 @@ def test_is_pip_audit_dependency_path() -> None: def test_plan_checks_selects_bandit_when_on_path() -> None: summary = DiffSummary( - files=( - FileChange(path="src/diffrat/review.py", additions=1, deletions=0, binary=False), - ) + files=(FileChange(path="src/diffrat/review.py", additions=1, deletions=0, binary=False),) ) with patch("diffrat.checks.shutil.which", return_value="/usr/bin/bandit"): @@ -422,9 +400,7 @@ def test_plan_checks_bandit_multi_file_single_r_flag() -> None: specs = plan_checks(summary) bandit_spec = next(spec for spec in specs if spec.code == "bandit") - assert bandit_spec.display_command == ( - "bandit -r src/diffrat/review.py src/diffrat/scoring.py" - ) + assert bandit_spec.display_command == ("bandit -r src/diffrat/review.py src/diffrat/scoring.py") assert bandit_spec.argv == ( "/usr/bin/bandit", "-r", @@ -435,9 +411,7 @@ def test_plan_checks_bandit_multi_file_single_r_flag() -> None: def test_plan_checks_skips_bandit_when_missing() -> None: summary = DiffSummary( - files=( - FileChange(path="src/diffrat/review.py", additions=1, deletions=0, binary=False), - ) + files=(FileChange(path="src/diffrat/review.py", additions=1, deletions=0, binary=False),) ) with patch("diffrat.checks.shutil.which", return_value=None): @@ -449,9 +423,7 @@ def test_plan_checks_skips_bandit_when_missing() -> None: def test_plan_checks_selects_pip_audit_for_pyproject() -> None: summary = DiffSummary( - files=( - FileChange(path="pyproject.toml", additions=1, deletions=0, binary=False), - ) + files=(FileChange(path="pyproject.toml", additions=1, deletions=0, binary=False),) ) with patch("diffrat.checks.shutil.which", return_value="/usr/bin/pip-audit"): @@ -463,9 +435,7 @@ def test_plan_checks_selects_pip_audit_for_pyproject() -> None: def test_plan_checks_skips_pip_audit_when_missing() -> None: summary = DiffSummary( - files=( - FileChange(path="requirements.txt", additions=1, deletions=0, binary=False), - ) + files=(FileChange(path="requirements.txt", additions=1, deletions=0, binary=False),) ) with patch("diffrat.checks.shutil.which", return_value=None): diff --git a/tests/test_content_hints.py b/tests/test_content_hints.py index f0c89bb..8d2d100 100644 --- a/tests/test_content_hints.py +++ b/tests/test_content_hints.py @@ -16,22 +16,14 @@ FileDiffContent, ) -_FILTER_GOOD = ( - 'PROJECT_EXECUTOR_COMMENT_FILTER = "^/(execute-project|continue-project)$"' -) -_FILTER_TYPO = ( - 'PROJECT_EXECUTOR_COMMENT_FILTER = "^/(execute-project|continue-projec)$"' -) +_FILTER_GOOD = 'PROJECT_EXECUTOR_COMMENT_FILTER = "^/(execute-project|continue-project)$"' +_FILTER_TYPO = 'PROJECT_EXECUTOR_COMMENT_FILTER = "^/(execute-project|continue-projec)$"' _DOGFOOD_CONFIG = load_config(Path(__file__).resolve().parents[1]) def _dogfood_regex_typo_config() -> DiffratConfig: - rules = tuple( - rule - for rule in _DOGFOOD_CONFIG.content_rules - if rule.code == "regex_typo" - ) + rules = tuple(rule for rule in _DOGFOOD_CONFIG.content_rules if rule.code == "regex_typo") return DiffratConfig(checks={}, content_rules=rules) @@ -556,10 +548,10 @@ def test_content_hints_cli_flag_multiline_without_help() -> None: DiffHunk( header="@@ -10 +10,3 @@", lines=( - '+ review_parser.add_argument(', + "+ review_parser.add_argument(", '+ "--verbose",', '+ action="store_true",', - '+ )', + "+ )", ), ), ), diff --git a/tests/test_json_renderer.py b/tests/test_json_renderer.py index c07ea9e..6203e6b 100644 --- a/tests/test_json_renderer.py +++ b/tests/test_json_renderer.py @@ -59,9 +59,7 @@ def test_render_review_json_includes_changes() -> None: truncated_files=False, ) - payload = json.loads( - render_review_json(summary, mode="unstaged", diff_content=diff_content) - ) + payload = json.loads(render_review_json(summary, mode="unstaged", diff_content=diff_content)) assert payload["changes"]["files"][0]["path"] == "README.md" assert payload["changes"]["files"][0]["hunks"][0]["lines"] == ["+extra line"] @@ -154,9 +152,7 @@ def test_render_review_json_review_quality_shape() -> None: llm_findings=None, llm_error=None, ) - payload = json.loads( - render_review_json(summary, mode="unstaged", analysis=analysis) - ) + payload = json.loads(render_review_json(summary, mode="unstaged", analysis=analysis)) pillars = payload["review_quality"]["pillars"] understand = next(p for p in pillars if p["id"] == "understand") assert understand == { @@ -190,19 +186,13 @@ def test_render_review_json_focus_risk_includes_path_and_line_when_set() -> None risk_scores=(10,), ) - payload = json.loads( - render_review_json(summary, mode="unstaged", analysis=analysis) - ) + payload = json.loads(render_review_json(summary, mode="unstaged", analysis=analysis)) - secret_hint = next( - item for item in payload["focus_risk"] if item["code"] == "possible_secret" - ) + secret_hint = next(item for item in payload["focus_risk"] if item["code"] == "possible_secret") assert secret_hint["path"] == "src/a.py" assert secret_hint["line"] == 10 - docs_hint = next( - item for item in payload["focus_risk"] if item["code"] == "docs_touched" - ) + docs_hint = next(item for item in payload["focus_risk"] if item["code"] == "docs_touched") assert "path" not in docs_hint assert "line" not in docs_hint @@ -307,9 +297,7 @@ def test_render_review_json_includes_llm_findings_when_present() -> None: ) analysis = replace(analyze_diff(summary), llm_findings="LLM narrative.") - payload = json.loads( - render_review_json(summary, mode="unstaged", analysis=analysis) - ) + payload = json.loads(render_review_json(summary, mode="unstaged", analysis=analysis)) assert payload["llm_findings"] == "LLM narrative." assert payload["llm_status"] == "ok" @@ -328,9 +316,7 @@ def test_render_review_json_includes_llm_error_when_present() -> None: llm_error="LLM authentication failed (HTTP 401)", ) - payload = json.loads( - render_review_json(summary, mode="unstaged", analysis=analysis) - ) + payload = json.loads(render_review_json(summary, mode="unstaged", analysis=analysis)) assert payload["llm_status"] == "failed" assert payload["llm_error"] == "LLM authentication failed (HTTP 401)" diff --git a/tests/test_llm_client.py b/tests/test_llm_client.py index b723100..8a3d3b7 100644 --- a/tests/test_llm_client.py +++ b/tests/test_llm_client.py @@ -38,10 +38,7 @@ def test_resolve_chat_completions_url_custom_base() -> None: api_key="local", base_url="http://localhost:11434/v1", ) - assert ( - resolve_chat_completions_url(config) - == "http://localhost:11434/v1/chat/completions" - ) + assert resolve_chat_completions_url(config) == "http://localhost:11434/v1/chat/completions" def test_resolve_chat_completions_url_rejects_full_chat_completions_path() -> None: @@ -261,9 +258,7 @@ def test_run_llm_analysis_builds_prompt_from_diff_content() -> None: captured: dict[str, str] = {} def fake_urlopen(request: Any, timeout: float = 0) -> io.BytesIO: - captured["prompt"] = json.loads(request.data.decode("utf-8"))["messages"][-1][ - "content" - ] + captured["prompt"] = json.loads(request.data.decode("utf-8"))["messages"][-1]["content"] response = _success_response("ok") response.status = 200 # type: ignore[attr-defined] return response diff --git a/tests/test_report.py b/tests/test_report.py index 20ab244..bd66daa 100644 --- a/tests/test_report.py +++ b/tests/test_report.py @@ -189,9 +189,7 @@ def test_render_review_report_review_order_before_changes_and_caps_at_five() -> review_section = report.split("Review order", maxsplit=1)[1].split("Changes", maxsplit=1)[0] ranked_lines = [ - line - for line in review_section.splitlines() - if line.strip().startswith(tuple("12345.")) + line for line in review_section.splitlines() if line.strip().startswith(tuple("12345.")) ] assert len(ranked_lines) == 5 assert "6." not in review_section diff --git a/tests/test_review.py b/tests/test_review.py index 0a7c1ee..74fc4f6 100644 --- a/tests/test_review.py +++ b/tests/test_review.py @@ -317,9 +317,7 @@ def test_run_review_check_reports_failure( CheckSpec( code="ci_validator", argv=("python",), - display_command=( - "python ci/validate-workflow-contracts.py --mode project" - ), + display_command=("python ci/validate-workflow-contracts.py --mode project"), ) ], ) @@ -501,9 +499,7 @@ def test_run_review_fail_on_check_failure_precedence( CheckSpec( code="ci_validator", argv=("python",), - display_command=( - "python ci/validate-workflow-contracts.py --mode project" - ), + display_command=("python ci/validate-workflow-contracts.py --mode project"), ) ], ) diff --git a/tests/test_scoring.py b/tests/test_scoring.py index d60703f..2b5260b 100644 --- a/tests/test_scoring.py +++ b/tests/test_scoring.py @@ -100,7 +100,7 @@ def test_risk_score_line_share() -> None: has_tests_in_diff=True, security_sensitive=False, ) - # 10 lines = 100% share -> 50 points, source with tests -> no extra + # 10 lines = 100% share -> 50 points, source with tests -> no extra assert score == 50 From b67480f560c782f25b5445995383166d629065ef Mon Sep 17 00:00:00 2001 From: Szymon Iwacz Date: Mon, 10 Aug 2026 09:36:49 +0200 Subject: [PATCH 2/3] Silence bandit FPs in --check paths --- CHANGELOG.md | 5 +++++ src/diffrat/checks.py | 4 ++-- src/diffrat/review_quality.py | 39 +++++++++++++++++++---------------- 3 files changed, 28 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e3152d2..e757fdd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- `--check` bandit false positives on Diffrat itself (`subprocess` import/run in + `checks.py`; pillar id `"maintainable"` in `review_quality.py`) + ## [1.1.1] - 2026-08-09 ### Fixed diff --git a/src/diffrat/checks.py b/src/diffrat/checks.py index 9fe49a9..8911b59 100644 --- a/src/diffrat/checks.py +++ b/src/diffrat/checks.py @@ -4,7 +4,7 @@ import shlex import shutil -import subprocess +import subprocess # nosec B404 import sys from dataclasses import dataclass from pathlib import PurePosixPath @@ -254,7 +254,7 @@ def run_checks( ) ) continue - completed = subprocess.run( + completed = subprocess.run( # nosec B603 list(spec.argv), cwd=cwd, capture_output=True, diff --git a/src/diffrat/review_quality.py b/src/diffrat/review_quality.py index ebc2a16..23ee827 100644 --- a/src/diffrat/review_quality.py +++ b/src/diffrat/review_quality.py @@ -12,7 +12,10 @@ PillarId = Literal["understand", "focused", "maintainable"] PillarStatus = Literal["ok", "warn", "risk"] -_DEFAULT_PILLAR: PillarId = "maintainable" +# Pillar id label (JSON/API contract); not a credential. +PILLAR_MAINTAINABLE: PillarId = "maintainable" # nosec B105 + +_DEFAULT_PILLAR: PillarId = PILLAR_MAINTAINABLE @dataclass(frozen=True, slots=True) @@ -43,7 +46,7 @@ class ReviewQualityPillarResult: label="One thing well", ), ReviewQualityPillar( - id="maintainable", + id=PILLAR_MAINTAINABLE, label="Safe to change in six months", ), ) @@ -66,22 +69,22 @@ class ReviewQualityPillarResult: "wip_commits": "focused", "mixed_concerns": "focused", # maintainable — safety, tests, dependencies, fragile patterns - "security_sensitive_paths": "maintainable", - "ci_workflow_paths": "maintainable", - "possible_secret": "maintainable", - "dangerous_call": "maintainable", - "config_or_deps": "maintainable", - "suspicious_constant_change": "maintainable", - "tests_touched": "maintainable", - "source_without_tests": "maintainable", - "source_heavy_without_tests": "maintainable", - "ci_without_tests": "maintainable", - "missing_test_file": "maintainable", - "lockfile_without_manifest": "maintainable", - "manifest_without_lockfile": "maintainable", - "debug_leftover": "maintainable", - "broad_exception": "maintainable", - "hardcoded_url_or_ip": "maintainable", + "security_sensitive_paths": PILLAR_MAINTAINABLE, + "ci_workflow_paths": PILLAR_MAINTAINABLE, + "possible_secret": PILLAR_MAINTAINABLE, + "dangerous_call": PILLAR_MAINTAINABLE, + "config_or_deps": PILLAR_MAINTAINABLE, + "suspicious_constant_change": PILLAR_MAINTAINABLE, + "tests_touched": PILLAR_MAINTAINABLE, + "source_without_tests": PILLAR_MAINTAINABLE, + "source_heavy_without_tests": PILLAR_MAINTAINABLE, + "ci_without_tests": PILLAR_MAINTAINABLE, + "missing_test_file": PILLAR_MAINTAINABLE, + "lockfile_without_manifest": PILLAR_MAINTAINABLE, + "manifest_without_lockfile": PILLAR_MAINTAINABLE, + "debug_leftover": PILLAR_MAINTAINABLE, + "broad_exception": PILLAR_MAINTAINABLE, + "hardcoded_url_or_ip": PILLAR_MAINTAINABLE, } From 0917725922ba4abd57ea0e7835934228c9afa9f3 Mon Sep 17 00:00:00 2001 From: Szymon Iwacz Date: Mon, 10 Aug 2026 10:59:52 +0200 Subject: [PATCH 3/3] Add bandit CI gate and dogfood test --- .ai/stack-profiles/diffrat-cli.md | 1 + .../workflows/validate-workflow-contracts.yml | 7 +++++ CHANGELOG.md | 2 ++ README.md | 1 + pyproject.toml | 2 +- tests/test_bandit_self.py | 28 +++++++++++++++++++ 6 files changed, 40 insertions(+), 1 deletion(-) create mode 100644 tests/test_bandit_self.py diff --git a/.ai/stack-profiles/diffrat-cli.md b/.ai/stack-profiles/diffrat-cli.md index ce85180..42f3be5 100644 --- a/.ai/stack-profiles/diffrat-cli.md +++ b/.ai/stack-profiles/diffrat-cli.md @@ -19,6 +19,7 @@ pytest ruff format --check src tests ruff check . mypy . +bandit -r src/diffrat/checks.py src/diffrat/review_quality.py src/diffrat/scoring.py python -m diffrat --help diffrat --help ``` diff --git a/.github/workflows/validate-workflow-contracts.yml b/.github/workflows/validate-workflow-contracts.yml index 5b6def7..673dc76 100644 --- a/.github/workflows/validate-workflow-contracts.yml +++ b/.github/workflows/validate-workflow-contracts.yml @@ -46,5 +46,12 @@ jobs: - name: Mypy run: mypy . + - name: Bandit + run: > + bandit -r + src/diffrat/checks.py + src/diffrat/review_quality.py + src/diffrat/scoring.py + - name: Pytest run: pytest tests/ -q diff --git a/CHANGELOG.md b/CHANGELOG.md index cbae14f..675274e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 `./scripts/setup-ai-workflow.sh` first - CI runs `ruff format --check src tests`, `ruff check .`, and `mypy .` in addition to pytest (matches README / stack-profile quality gates) +- CI / `[dev]` include `bandit` on `--check` dogfood modules (`checks.py`, + `review_quality.py`, `scoring.py`) - One-shot `ruff format` on `src/` and `tests/` ## [1.1.1] - 2026-08-09 diff --git a/README.md b/README.md index 7bbfd7c..c265a65 100644 --- a/README.md +++ b/README.md @@ -339,6 +339,7 @@ pytest ruff format --check src tests ruff check . mypy . +bandit -r src/diffrat/checks.py src/diffrat/review_quality.py src/diffrat/scoring.py ``` ## Architecture and context diff --git a/pyproject.toml b/pyproject.toml index e7cd198..ce161ea 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -32,7 +32,7 @@ Issues = "https://github.com/szymoniwacz/diffrat/issues" Changelog = "https://github.com/szymoniwacz/diffrat/blob/main/CHANGELOG.md" [project.optional-dependencies] -dev = ["pytest>=8.0", "mypy>=1.8", "ruff>=0.4"] +dev = ["pytest>=8.0", "mypy>=1.8", "ruff>=0.4", "bandit>=1.7"] [project.scripts] diffrat = "diffrat.__main__:main" diff --git a/tests/test_bandit_self.py b/tests/test_bandit_self.py new file mode 100644 index 0000000..378ad29 --- /dev/null +++ b/tests/test_bandit_self.py @@ -0,0 +1,28 @@ +"""Dogfood: bandit stays clean on modules that --check commonly scans.""" + +from __future__ import annotations + +import shutil +import subprocess +from pathlib import Path + +import pytest + +_REPO_ROOT = Path(__file__).resolve().parents[1] +_BANDIT_TARGETS = ( + "src/diffrat/checks.py", + "src/diffrat/review_quality.py", + "src/diffrat/scoring.py", +) + + +@pytest.mark.skipif(shutil.which("bandit") is None, reason="bandit not installed") +def test_bandit_clean_on_check_dogfood_modules() -> None: + completed = subprocess.run( + ["bandit", "-r", *_BANDIT_TARGETS], + cwd=_REPO_ROOT, + capture_output=True, + text=True, + check=False, + ) + assert completed.returncode == 0, completed.stdout + completed.stderr