diff --git a/.github/workflows/opencode-review.yml b/.github/workflows/opencode-review.yml index dc72cf2c..c78e63da 100644 --- a/.github/workflows/opencode-review.yml +++ b/.github/workflows/opencode-review.yml @@ -1169,7 +1169,7 @@ jobs: implementation_changed_files="$(mktemp)" changed_files_for_coverage >"$implementation_changed_files" - run_and_capture "Python implementation completeness scan" \ + run_and_capture "Implementation completeness scan" \ python3 "$GITHUB_WORKSPACE/scripts/ci/implementation_completeness_scan.py" \ --repo-root . \ --changed-files "$implementation_changed_files" diff --git a/scripts/ci/implementation_completeness_scan.py b/scripts/ci/implementation_completeness_scan.py index 8457147c..66662383 100644 --- a/scripts/ci/implementation_completeness_scan.py +++ b/scripts/ci/implementation_completeness_scan.py @@ -1,10 +1,11 @@ #!/usr/bin/env python3 -"""Detect executable Python placeholder implementations in changed runtime code.""" +"""Detect executable placeholder implementations in changed runtime code.""" from __future__ import annotations import argparse import ast +import re from dataclasses import dataclass from pathlib import Path from typing import Iterable @@ -105,6 +106,9 @@ def placeholder_reason( exc_name = dotted_name(only.exc) if exc_name == "NotImplementedError": return "raises NotImplementedError" + if isinstance(only, ast.Return) and only.value is not None: + if dotted_name(only.value) == "NotImplemented": + return "returns NotImplemented" return None @@ -159,6 +163,18 @@ def is_runtime_python_path(path: Path) -> bool: return not any(part in RUNTIME_TEST_PARTS for part in path.parts) +def is_runtime_rust_path(path: Path) -> bool: + """Return whether a path is a runtime Rust file rather than test support.""" + if path.suffix != ".rs": + return False + return not any(part in RUNTIME_TEST_PARTS for part in path.parts) + + +def is_runtime_source_path(path: Path) -> bool: + """Return whether a path is a supported runtime source file.""" + return is_runtime_python_path(path) or is_runtime_rust_path(path) + + def changed_paths_from_file(path: Path) -> list[Path]: """Read a newline-delimited changed-file list.""" if not path.exists(): @@ -181,26 +197,111 @@ def scan_python_file(repo_root: Path, relative_path: Path) -> list[Finding]: return visitor.findings +def rust_code_lines(source: str) -> list[tuple[int, str]]: + """Return Rust source lines with comments and string bodies blanked out.""" + code_lines: list[tuple[int, str]] = [] + block_comment_depth = 0 + in_string = False + escaped = False + for line_no, line in enumerate(source.splitlines(), start=1): + i = 0 + cleaned: list[str] = [] + while i < len(line): + char = line[i] + next_two = line[i : i + 2] + if block_comment_depth: + if next_two == "/*": + block_comment_depth += 1 + i += 2 + continue + if next_two == "*/": + block_comment_depth -= 1 + i += 2 + continue + i += 1 + continue + if in_string: + cleaned.append(" ") + if escaped: + escaped = False + elif char == "\\": + escaped = True + elif char == '"': + in_string = False + i += 1 + continue + if next_two == "//": + break + if next_two == "/*": + block_comment_depth += 1 + i += 2 + continue + if char == '"': + in_string = True + cleaned.append(" ") + i += 1 + continue + cleaned.append(char) + i += 1 + code_lines.append((line_no, "".join(cleaned))) + return code_lines + + +def nearest_rust_symbol(code_lines: list[tuple[int, str]], line_no: int) -> str: + """Return the nearest preceding Rust function name for a finding.""" + fn_pattern = re.compile(r"\bfn\s+([A-Za-z_][A-Za-z0-9_]*)") + for current_line_no, code in reversed(code_lines): + if current_line_no > line_no: + continue + match = fn_pattern.search(code) + if match: + return match.group(1) + return "rust module" + + +def scan_rust_file(repo_root: Path, relative_path: Path) -> list[Finding]: + """Scan one repository-relative Rust file for placeholder macros.""" + source_path = repo_root / relative_path + code_lines = rust_code_lines(source_path.read_text(encoding="utf-8")) + findings: list[Finding] = [] + macro_pattern = re.compile(r"\b(todo|unimplemented)\s*!") + for line_no, code in code_lines: + for match in macro_pattern.finditer(code): + macro_name = match.group(1) + findings.append( + Finding( + path=relative_path.as_posix(), + line=line_no, + symbol=nearest_rust_symbol(code_lines, line_no), + reason=f"`{macro_name}!` macro placeholder", + ) + ) + return findings + + def scan_changed_paths( repo_root: Path, changed_paths: Iterable[Path] ) -> tuple[list[Finding], list[str]]: - """Scan changed runtime Python paths and return findings plus parse errors.""" + """Scan changed runtime source paths and return findings plus parse errors.""" findings: list[Finding] = [] errors: list[str] = [] seen: set[str] = set() for relative_path in changed_paths: key = relative_path.as_posix() - if key in seen or not is_runtime_python_path(relative_path): + if key in seen or not is_runtime_source_path(relative_path): continue seen.add(key) source_path = repo_root / relative_path if not source_path.is_file(): continue - try: - findings.extend(scan_python_file(repo_root, relative_path)) - except SyntaxError as exc: - line = exc.lineno or 1 - errors.append(f"{key}:{line} could not be parsed: {exc.msg}") + if relative_path.suffix == ".py": + try: + findings.extend(scan_python_file(repo_root, relative_path)) + except SyntaxError as exc: + line = exc.lineno or 1 + errors.append(f"{key}:{line} could not be parsed: {exc.msg}") + else: + findings.extend(scan_rust_file(repo_root, relative_path)) return findings, errors @@ -209,7 +310,7 @@ def render_report(findings: list[Finding], errors: list[str], checked_count: int lines = [ "# Implementation Completeness Scan", "", - f"- Checked runtime Python files: {checked_count}", + f"- Checked runtime source files: {checked_count}", "- Declaration handling: typing.Protocol, abc.ABC, @abstractmethod, and @overload placeholders are treated as contracts, not executable missing implementations.", ] if errors: @@ -240,7 +341,7 @@ def render_report(findings: list[Finding], errors: list[str], checked_count: int lines.extend( [ "- Result: PASS", - "- Reason: no executable placeholder implementations were found in changed runtime Python files.", + "- Reason: no executable placeholder implementations were found in changed runtime source files.", ] ) return "\n".join(lines) + "\n" @@ -258,7 +359,7 @@ def main() -> int: runtime_paths = [ path for path in dict.fromkeys(changed_paths) - if is_runtime_python_path(path) and (repo_root / path).is_file() + if is_runtime_source_path(path) and (repo_root / path).is_file() ] findings, errors = scan_changed_paths(repo_root, runtime_paths) print(render_report(findings, errors, len(runtime_paths)), end="") diff --git a/tests/test_implementation_completeness_scan.py b/tests/test_implementation_completeness_scan.py index 157fa176..116d83c9 100644 --- a/tests/test_implementation_completeness_scan.py +++ b/tests/test_implementation_completeness_scan.py @@ -71,6 +71,10 @@ def merge(self): async def sync(): ... + + +def binary_op(): + return NotImplemented """, encoding="utf-8", ) @@ -85,12 +89,73 @@ async def sync(): ("create_user", "pass-only body"), ("Engine.merge", "raises NotImplementedError"), ("sync", "ellipsis-only body"), + ("binary_op", "returns NotImplemented"), ] report = scan.render_report(findings, errors, checked_count=1) assert "- Result: FAIL" in report assert "service/merge_engine.py:2 `create_user` - pass-only body" in report +def test_rust_placeholder_macros_fail_with_line_reasons(tmp_path: Path) -> None: + source = tmp_path / "src" / "lib.rs" + source.parent.mkdir() + source.write_text( + """pub fn implemented() -> &'static str { + "todo!() in a string is not executable" +} +// todo!() in a comment is not executable +pub fn missing() { + todo!("merge path not ported yet"); +} + +impl Engine { + pub fn run(&self) { + unimplemented!() + } +} +""", + encoding="utf-8", + ) + changed = write_changed_files(tmp_path, "src/lib.rs") + + findings, errors = scan.scan_changed_paths( + tmp_path, scan.changed_paths_from_file(changed) + ) + + assert errors == [] + assert [(finding.line, finding.symbol, finding.reason) for finding in findings] == [ + (6, "missing", "`todo!` macro placeholder"), + (11, "run", "`unimplemented!` macro placeholder"), + ] + report = scan.render_report(findings, errors, checked_count=1) + assert "- Result: FAIL" in report + assert "src/lib.rs:6 `missing` - `todo!` macro placeholder" in report + + +def test_rust_scanner_ignores_comments_and_strings(tmp_path: Path) -> None: + source = tmp_path / "src" / "module.rs" + source.parent.mkdir() + source.write_text( + """/* outer comment starts +/* nested comment mentions todo!() */ +still only a comment with unimplemented!() +*/ +todo!("module-level unfinished work"); + +pub fn implemented() { + let text = "escaped \\" todo!() stays in the string"; +} +""", + encoding="utf-8", + ) + + findings = scan.scan_rust_file(tmp_path, Path("src/module.rs")) + + assert [(finding.line, finding.symbol, finding.reason) for finding in findings] == [ + (5, "rust module", "`todo!` macro placeholder") + ] + + def test_tests_and_missing_files_are_ignored(tmp_path: Path) -> None: source = tmp_path / "tests" / "test_merge_engine.py" source.parent.mkdir() @@ -98,13 +163,14 @@ def test_tests_and_missing_files_are_ignored(tmp_path: Path) -> None: changed = write_changed_files( tmp_path, "tests/test_merge_engine.py", + "tests/test_lib.rs", "deleted_runtime_file.py", ) runtime_paths = [ path for path in scan.changed_paths_from_file(changed) - if scan.is_runtime_python_path(path) and (tmp_path / path).is_file() + if scan.is_runtime_source_path(path) and (tmp_path / path).is_file() ] findings, errors = scan.scan_changed_paths(tmp_path, runtime_paths) @@ -147,6 +213,12 @@ class Concrete: @abstractmethod def declared_abstract(self): pass + + +class Decorated: + @staticmethod + def concrete(self): + return None """ ) @@ -156,14 +228,19 @@ def declared_abstract(self): helper = tree.body[4].body[0] other_error = tree.body[5] abstract_method = tree.body[6].body[0] + decorated_method = tree.body[7].body[0] assert isinstance(helper, ast.FunctionDef) assert isinstance(other_error, ast.FunctionDef) assert isinstance(abstract_method, ast.FunctionDef) + assert isinstance(decorated_method, ast.FunctionDef) assert scan.is_abstract_or_overload(abstract_method) + assert not scan.is_abstract_or_overload(decorated_method) assert scan.placeholder_reason(helper) is None assert scan.placeholder_reason(other_error) is None assert scan.strip_docstring([]) == [] assert not scan.is_runtime_python_path(Path("README.md")) + assert scan.is_runtime_rust_path(Path("src/lib.rs")) + assert not scan.is_runtime_source_path(Path("tests/lib.rs")) def test_scan_reports_parse_errors_and_skips_duplicates(tmp_path: Path) -> None: