learningcircuit--local-deep-research/tests/security/test_silent_cleanup_hook.py

"""
Tests for the check-silent-cleanup pre-commit hook.

Ensures the hook correctly detects:
1. Silent exception handling in close() methods
2. Bare .close() calls in finally/except blocks
"""

import ast
import sys
from importlib import import_module
from pathlib import Path

# Add the pre-commit hooks directory to path
HOOKS_DIR = Path(__file__).parent.parent.parent / ".pre-commit-hooks"
sys.path.insert(0, str(HOOKS_DIR))

# Import the checker from the hook (must be after sys.path modification)
hook_module = import_module("check-silent-cleanup")  # noqa: E402
SilentCleanupChecker = hook_module.SilentCleanupChecker
BareCloseInFinallyChecker = hook_module.BareCloseInFinallyChecker


class TestSilentCleanupChecker:
    """Tests for the SilentCleanupChecker AST visitor."""

    def _check_code(self, code: str, filename: str = "src/module.py") -> list:
        """Helper to check code and return errors."""
        tree = ast.parse(code)
        checker = SilentCleanupChecker(filename)
        checker.visit(tree)
        return checker.errors

    # --- Detection: should flag ---

    def test_detects_except_exception_pass_in_close(self):
        """Should flag except Exception: pass inside close()."""
        code = """
class Foo:
    def close(self):
        try:
            self.client.close()
        except Exception:
            pass
"""
        errors = self._check_code(code)
        assert len(errors) == 1
        assert "Silent exception" in errors[0][1]

    def test_detects_bare_except_pass_in_close(self):
        """Should flag bare except: pass inside close()."""
        code = """
class Foo:
    def close(self):
        try:
            self.client.close()
        except:
            pass
"""
        errors = self._check_code(code)
        assert len(errors) == 1

    def test_detects_multiple_silent_blocks(self):
        """Should flag each silent block independently."""
        code = """
class Foo:
    def close(self):
        try:
            self.session.close()
        except Exception:
            pass
        try:
            self.engine.close()
        except Exception:
            pass
"""
        errors = self._check_code(code)
        assert len(errors) == 2

    def test_reports_correct_line_number(self):
        """Should report the line of the except handler, not the try."""
        code = """
class Foo:
    def close(self):
        x = 1
        y = 2
        try:
            self.client.close()
        except Exception:
            pass
"""
        errors = self._check_code(code)
        assert len(errors) == 1
        assert errors[0][0] == 8  # "except Exception:" is line 8

    # --- Allowances: should NOT flag ---

    def test_allows_logger_warning_in_handler(self):
        """Should allow except blocks that log a warning."""
        code = """
class Foo:
    def close(self):
        try:
            self.client.close()
        except Exception:
            logger.warning("Failed to close client")
"""
        errors = self._check_code(code)
        assert len(errors) == 0

    def test_allows_logger_exception_in_handler(self):
        """Should allow except blocks that use logger.exception."""
        code = """
class Foo:
    def close(self):
        try:
            self.client.close()
        except Exception:
            logger.exception("Close failed")
"""
        errors = self._check_code(code)
        assert len(errors) == 0

    def test_allows_logger_debug_in_handler(self):
        """Should allow any logger.* call."""
        code = """
class Foo:
    def close(self):
        try:
            self.client.close()
        except Exception:
            logger.debug("Ignoring close error")
"""
        errors = self._check_code(code)
        assert len(errors) == 0

    def test_allows_raise_in_handler(self):
        """Should allow except blocks that re-raise."""
        code = """
class Foo:
    def close(self):
        try:
            self.client.close()
        except Exception:
            raise
"""
        errors = self._check_code(code)
        assert len(errors) == 0

    def test_allows_specific_exception_type(self):
        """Should not flag handlers for specific exception types (e.g. OSError)."""
        code = """
class Foo:
    def close(self):
        try:
            self.client.close()
        except OSError:
            pass
"""
        errors = self._check_code(code)
        assert len(errors) == 0

    def test_ignores_non_close_methods(self):
        """Should not inspect methods other than close()."""
        code = """
class Foo:
    def cleanup(self):
        try:
            self.client.close()
        except Exception:
            pass

    def shutdown(self):
        try:
            self.engine.stop()
        except Exception:
            pass
"""
        errors = self._check_code(code)
        assert len(errors) == 0

    def test_ignores_standalone_close_function(self):
        """A top-level function named close() should still be checked."""
        code = """
def close():
    try:
        something.close()
    except Exception:
        pass
"""
        errors = self._check_code(code)
        assert len(errors) == 1

    def test_close_with_no_try_except(self):
        """close() with no try/except should pass cleanly."""
        code = """
class Foo:
    def close(self):
        self.client.close()
"""
        errors = self._check_code(code)
        assert len(errors) == 0

    def test_close_using_safe_close(self):
        """close() using safe_close() should pass."""
        code = """
class Foo:
    def close(self):
        from utilities.resource_utils import safe_close
        safe_close(self.client, "client")
"""
        errors = self._check_code(code)
        assert len(errors) == 0

    def test_mixed_silent_and_logged(self):
        """Should flag only the silent block, not the logged one."""
        code = """
class Foo:
    def close(self):
        try:
            self.session.close()
        except Exception:
            logger.warning("session close failed")
        try:
            self.engine.close()
        except Exception:
            pass
"""
        errors = self._check_code(code)
        assert len(errors) == 1
        # The flagged line should be the second except (line 10)
        assert errors[0][0] == 10


class TestFileExclusions:
    """Tests for file-level exclusion patterns via check_file()."""

    SILENT_CLOSE = """\
class Foo:
    def close(self):
        try:
            self.x.close()
        except Exception:
            pass
"""

    def _write_and_check(self, tmp_path, subdir, filename):
        """Write SILENT_CLOSE to subdir/filename and run check_file."""
        d = tmp_path / subdir
        d.mkdir(parents=True, exist_ok=True)
        f = d / filename
        f.write_text(self.SILENT_CLOSE)
        return hook_module.check_file(str(f))

    def test_allows_test_directory(self, tmp_path):
        """Should skip files under tests/."""
        assert self._write_and_check(tmp_path, "tests", "foo.py") is True

    def test_allows_test_prefix(self, tmp_path):
        """Should skip files starting with test_."""
        assert (
            self._write_and_check(tmp_path, "src", "test_something.py") is True
        )

    def test_allows_test_suffix(self, tmp_path):
        """Should skip files ending with _test.py."""
        assert (
            self._write_and_check(tmp_path, "src", "something_test.py") is True
        )

    def test_allows_llm_config(self, tmp_path):
        """Should skip config/llm_config.py (logging boundary)."""
        assert (
            self._write_and_check(tmp_path, "config", "llm_config.py") is True
        )

    def test_allows_rate_limiting_wrapper(self, tmp_path):
        """Should skip rate_limiting/llm/wrapper.py (logging boundary)."""
        assert (
            self._write_and_check(tmp_path, "rate_limiting/llm", "wrapper.py")
            is True
        )

    def test_flags_regular_source_file(self):
        """Should flag violations in normal source files."""
        tree = ast.parse(self.SILENT_CLOSE)
        checker = SilentCleanupChecker("src/engine.py")
        checker.visit(tree)
        assert len(checker.errors) == 1


class TestCheckFileIntegration:
    """Integration tests for check_file()."""

    def _write_and_check(self, tmp_path, code, filename="module.py"):
        """Write code to a temp file and run check_file on it.

        Uses a subdirectory named 'src' to avoid matching exclusion
        patterns like 'test_' that can appear in pytest's tmp_path.
        We also monkeypatch check_file to use a clean path for pattern
        matching while still reading from the real file.
        """
        src_dir = tmp_path / "src"
        src_dir.mkdir(exist_ok=True)
        filepath = src_dir / filename
        filepath.write_text(code)
        # check_file matches ALLOWED_PATTERNS against the full path.
        # pytest tmp dirs contain "pytest-" and "test_" which would match.
        # Use the checker directly to bypass path-based exclusions.
        import ast as _ast

        with open(filepath, "r", encoding="utf-8") as f:
            content = f.read()
        tree = _ast.parse(content, filename=str(filepath))
        checker = SilentCleanupChecker("src/" + filename)
        checker.visit(tree)
        return len(checker.errors) == 0

    def test_clean_file_passes(self, tmp_path):
        code = """
class Foo:
    def close(self):
        from utils import safe_close
        safe_close(self.client, "client")
"""
        assert self._write_and_check(tmp_path, code) is True

    def test_violation_fails(self, tmp_path):
        code = """
class Foo:
    def close(self):
        try:
            self.client.close()
        except Exception:
            pass
"""
        assert self._write_and_check(tmp_path, code) is False

    def test_non_python_file_passes(self, tmp_path):
        src_dir = tmp_path / "src"
        src_dir.mkdir()
        txt = src_dir / "readme.txt"
        txt.write_text("not python")
        assert hook_module.check_file(str(txt)) is True

    def test_syntax_error_passes(self, tmp_path):
        """Files with syntax errors are skipped (other tools handle them)."""
        src_dir = tmp_path / "src"
        src_dir.mkdir()
        bad = src_dir / "broken.py"
        bad.write_text("def close(self:\n    pass")
        assert hook_module.check_file(str(bad)) is True

    def test_nonexistent_file_fails(self):
        assert hook_module.check_file("/nonexistent/path/module.py") is False

    def test_empty_file_passes(self, tmp_path):
        src_dir = tmp_path / "src"
        src_dir.mkdir()
        empty = src_dir / "empty.py"
        empty.write_text("")
        assert hook_module.check_file(str(empty)) is True


class TestIsSilentHandler:
    """Unit tests for _is_silent_handler static method."""

    def _make_handler(self, body_code):
        """Parse an except handler from code."""
        code = f"""
try:
    pass
except Exception:
{body_code}
"""
        tree = ast.parse(code)
        for node in ast.walk(tree):
            if isinstance(node, ast.ExceptHandler):
                return node
        raise ValueError("No ExceptHandler found")

    def test_pass_only_is_silent(self):
        handler = self._make_handler("    pass")
        assert SilentCleanupChecker._is_silent_handler(handler) is True

    def test_multiple_pass_is_silent(self):
        handler = self._make_handler("    pass\n    pass")
        assert SilentCleanupChecker._is_silent_handler(handler) is True

    def test_logger_call_is_not_silent(self):
        handler = self._make_handler('    logger.warning("fail")')
        assert SilentCleanupChecker._is_silent_handler(handler) is False

    def test_raise_is_not_silent(self):
        handler = self._make_handler("    raise")
        assert SilentCleanupChecker._is_silent_handler(handler) is False

    def test_assignment_is_not_silent(self):
        """An assignment (any non-pass statement) is not silent."""
        handler = self._make_handler("    x = 1")
        assert SilentCleanupChecker._is_silent_handler(handler) is False

    def test_print_call_is_not_silent(self):
        """A print() call (non-logger, non-pass) is not silent."""
        handler = self._make_handler('    print("error")')
        assert SilentCleanupChecker._is_silent_handler(handler) is False


class TestBareCloseInFinallyChecker:
    """Tests for the BareCloseInFinallyChecker AST visitor."""

    def _check_code(self, code: str, filename: str = "src/module.py") -> list:
        """Helper to check code and return errors."""
        tree = ast.parse(code)
        checker = BareCloseInFinallyChecker(filename)
        checker.visit(tree)
        return checker.errors

    # --- Detection: should flag ---

    def test_detects_bare_close_in_finally(self):
        """Should flag obj.close() in a finally block."""
        code = """
try:
    do_work()
finally:
    resource.close()
"""
        errors = self._check_code(code)
        assert len(errors) == 1
        assert "Bare .close()" in errors[0][1]

    def test_detects_bare_close_in_except(self):
        """Should flag obj.close() in an except block."""
        code = """
try:
    do_work()
except Exception:
    conn.close()
    raise
"""
        errors = self._check_code(code)
        assert len(errors) == 1

    def test_detects_bare_close_in_if_inside_finally(self):
        """Should flag obj.close() inside if-guard in finally."""
        code = """
try:
    do_work()
finally:
    if resource is not None:
        resource.close()
"""
        errors = self._check_code(code)
        assert len(errors) == 1

    def test_detects_bare_close_in_else_inside_finally(self):
        """Should flag obj.close() in else branch of if in finally."""
        code = """
try:
    do_work()
finally:
    if skip:
        pass
    else:
        resource.close()
"""
        errors = self._check_code(code)
        assert len(errors) == 1

    def test_detects_multiple_bare_closes(self):
        """Should flag each bare .close() independently."""
        code = """
try:
    do_work()
finally:
    session.close()
    engine.close()
"""
        errors = self._check_code(code)
        assert len(errors) == 2

    def test_reports_correct_line_number(self):
        """Should report the line of the .close() call."""
        code = """
try:
    do_work()
finally:
    x = 1
    resource.close()
"""
        errors = self._check_code(code)
        assert len(errors) == 1
        assert errors[0][0] == 6

    # --- Allowances: should NOT flag ---

    def test_allows_close_in_nested_try_except(self):
        """Should not flag .close() that's protected by its own try/except."""
        code = """
try:
    do_work()
finally:
    try:
        resource.close()
    except Exception:
        logger.warning("close failed")
"""
        errors = self._check_code(code)
        assert len(errors) == 0

    def test_allows_safe_close(self):
        """Should not flag safe_close() calls."""
        code = """
try:
    do_work()
finally:
    safe_close(resource, "resource")
"""
        errors = self._check_code(code)
        assert len(errors) == 0

    def test_allows_close_outside_finally(self):
        """Should not flag .close() calls in normal code."""
        code = """
resource = get_resource()
resource.close()
"""
        errors = self._check_code(code)
        assert len(errors) == 0

    def test_allows_close_with_args(self):
        """Should not flag .close(arg) — only no-arg .close()."""
        code = """
try:
    do_work()
finally:
    resource.close(force=True)
"""
        errors = self._check_code(code)
        assert len(errors) == 0

    def test_allows_non_close_method_in_finally(self):
        """Should not flag other method calls in finally."""
        code = """
try:
    do_work()
finally:
    resource.shutdown()
"""
        errors = self._check_code(code)
        assert len(errors) == 0

    def test_bare_except_handler_also_caught(self):
        """Should flag .close() in bare except: block."""
        code = """
try:
    do_work()
except:
    conn.close()
"""
        errors = self._check_code(code)
        assert len(errors) == 1

    def test_allows_plt_close(self):
        """Should not flag plt.close() — matplotlib figure cleanup, not a resource."""
        code = """
try:
    make_chart()
except Exception:
    plt.figure()
    plt.text(0.5, 0.5, "error")
    plt.savefig(path)
    plt.close()
"""
        errors = self._check_code(code)
        assert len(errors) == 0

    def test_allows_fig_close(self):
        """Should not flag fig.close() — matplotlib figure object."""
        code = """
try:
    make_chart()
finally:
    fig.close()
"""
        errors = self._check_code(code)
        assert len(errors) == 0