orchestrata_ultimate.agents.volks.codedirector.review_checks

Reference › orchestrata_ultimate › agents › volks › codedirector › review_checks

review_checks holds deterministic AST and regex code-review checks that replace items of the LLM checklist. Each check_* function takes a file path, a parsed tree or the file lines, and returns a list of issues. The set covers import ordering and relative imports (check_imports), file completeness against the workspace file list, an entry-point guard, declared dependencies (check_dependency_declaration), function length (maximum 40 lines) and parameter count (maximum 5), placeholders (check_no_placeholder, check_todo_comments), error handling (check_error_handling, check_no_except_pass), type hints, mutable default values, docstrings, line length (default 88), security patterns such as eval, exec and hard-coded secrets (check_security), lint through ruff or flake8 (check_lint), and, for tests, mock usage and real-infrastructure use (check_no_mocks, check_real_infrastructure). _FIX_INSTRUCTIONS maps issue kinds to fix guidance. The QA volk (volk) is the importer, which runs them as its review gates.

At a glance

Class diagram

No classes; see Functions.

Dependency diagram

API

Constants

Functions

Source

Permalink: https://gitlab.com/MSamy/orchestrata-ultimate-architecture/-/blob/a4cbc2bdf/src/orchestrata_ultimate/agents/volks/codedirector/review_checks.py

"""Deterministic code review checks — no LLM needed.

Replaces checklist items 1, 4, 5, 6, 10, 11, 12, 13, 17, 18, 19, 21, 30
from the CODE_REVIEW_PROMPT with fast, reliable AST/regex analysis.

Item 2 (NO_MOCKING) is already handled by qa.py._validate_no_mocks.

Usage:
    from orchestrata_ultimate.agents.volks.codedirector.review_checks import run_deterministic_review
    issues = run_deterministic_review(workspace, files)
    # issues: list of (check_id, filepath, description)
"""

import ast
import os
import re
import subprocess
import sys

from orchestrata_ultimate.core.base.traces import traces

# Each issue is (check_id, filepath, description)
Issue = tuple[str, str, str]

# Stdlib top-level module names (common subset for fast lookup)
_STDLIB_MODULES: set[str] = set(sys.stdlib_module_names)


# ── Helpers ──────────────────────────────────────────────────────


def _parse_file(filepath: str) -> ast.Module | None:
    """Parse a Python file into an AST. Returns None on failure."""
    try:
        with open(filepath, encoding="utf-8") as f:
            source = f.read()
        return ast.parse(source, filename=filepath)
    except SyntaxError, OSError:
        return None


def _read_lines(filepath: str) -> list[str]:
    """Read file lines. Returns empty list on failure."""
    try:
        with open(filepath, encoding="utf-8") as f:
            return f.readlines()
    except OSError:
        return []


def _is_public(name: str) -> bool:
    """Check if a name is public (no leading underscore)."""
    return not name.startswith("_")


def _function_line_count(
    node: ast.FunctionDef | ast.AsyncFunctionDef,
) -> int:
    """Count logical lines in a function body (excludes docstring)."""
    body = node.body
    start = 0
    # Skip docstring
    if (
        body
        and isinstance(body[0], ast.Expr)
        and isinstance(body[0].value, ast.Constant)
    ):
        start = 1
    if not body[start:]:
        return 0
    first = body[start].lineno
    last = body[-1].end_lineno or body[-1].lineno
    return last - first + 1


def _get_pyproject_deps(workspace: str) -> set[str]:
    """Extract dependency names from pyproject.toml."""
    deps: set[str] = set()
    toml_path = os.path.join(workspace, "pyproject.toml")
    if not os.path.exists(toml_path):
        return deps
    try:
        with open(toml_path, encoding="utf-8") as f:
            content = f.read()
    except OSError:
        return deps

    # Simple regex extraction — avoids tomli dependency
    in_deps = False
    for line in content.splitlines():
        stripped = line.strip()
        if stripped in ("dependencies = [", "dependencies=["):
            in_deps = True
            continue
        if in_deps:
            if stripped == "]":
                break
            # Match "flask>=3.0" or "redis" etc
            m = re.match(r'["\']([a-zA-Z0-9_-]+)', stripped)
            if m:
                # Normalize: PyPI names are case-insensitive, dashes = underscores
                deps.add(m.group(1).lower().replace("-", "_"))
    return deps


# Directory-name prefixes for scaffold pseudo-dirs like "Module: src/".
_SKIP_DIR_PREFIXES = ("Module:", "Module ")


def _collect_project_modules(workspace: str) -> set[str]:
    """Discover local package names under ``src/`` and the workspace root.

    A name is a project module if it is a directory under ``src/``, or a
    workspace-root directory containing ``__init__.py``.

    Args:
        workspace: Path to project workspace root.

    Returns:
        Set of local package/module names.
    """
    project_modules: set[str] = set()
    src_dir = os.path.join(workspace, "src")
    if os.path.isdir(src_dir):
        for name in os.listdir(src_dir):
            if os.path.isdir(os.path.join(src_dir, name)):
                project_modules.add(name)
    for name in os.listdir(workspace):
        full = os.path.join(workspace, name)
        if os.path.isdir(full) and os.path.exists(os.path.join(full, "__init__.py")):
            project_modules.add(name)
    return project_modules


def _collect_workspace_files(workspace: str) -> set[str]:
    """Collect all workspace file paths (relative), skipping pseudo-dirs.

    Args:
        workspace: Path to project workspace root.

    Returns:
        Set of workspace-relative file paths.
    """
    workspace_files: set[str] = set()
    for root, dirs, filenames in os.walk(workspace):
        dirs[:] = [d for d in dirs if not d.startswith(_SKIP_DIR_PREFIXES)]
        for fname in filenames:
            rel = os.path.relpath(os.path.join(root, fname), workspace)
            workspace_files.add(rel)
    return workspace_files


def _collect_imported_modules(
    tree: ast.Module,
) -> list[tuple[str, int, str]]:
    """Extract all imported top-level module names with line numbers.

    Returns:
        List of (top_level_module, line_number, full_import_string).
    """
    imports = []
    # Only check module-level imports (tree.body), not function-level
    # inline imports. isort only reorders top-level imports, and
    # function-level lazy imports (e.g. `import asyncio` inside main())
    # are a valid pattern that shouldn't trigger ordering violations.
    for node in tree.body:
        if isinstance(node, ast.Import):
            for alias in node.names:
                top = alias.name.split(".")[0]
                imports.append((top, node.lineno, alias.name))
        elif isinstance(node, ast.ImportFrom):
            if node.module and node.level == 0:
                top = node.module.split(".")[0]
                imports.append((top, node.lineno, node.module))
    return imports


# ── Individual Checks ────────────────────────────────────────────


@traces(spec="031", story="Deterministic Review", reqs=["REQ-031-026"])
def check_imports(
    filepath: str,
    tree: ast.Module,
    project_modules: set[str],
) -> list[Issue]:
    """Check 1: IMPORTS — ordering and relative imports."""
    issues: list[Issue] = []
    rel_name = os.path.basename(filepath)

    # Detect relative imports
    for node in ast.walk(tree):
        if isinstance(node, ast.ImportFrom) and node.level > 0:
            issues.append(
                (
                    "IMPORTS",
                    rel_name,
                    f"L{node.lineno}: Relative import "
                    f"(level={node.level}). Use absolute imports.",
                )
            )

    # Check import ordering: stdlib -> third-party -> local
    imported = _collect_imported_modules(tree)
    prev_group = 0  # 1=stdlib, 2=third-party, 3=local
    for top, lineno, full in imported:
        if top in _STDLIB_MODULES:
            group = 1
        elif top in project_modules:
            group = 3
        else:
            group = 2
        if group < prev_group:
            issues.append(
                (
                    "IMPORTS",
                    rel_name,
                    f"L{lineno}: Import '{full}' is out of order "
                    f"(stdlib < third-party < local).",
                )
            )
        prev_group = group

    return issues


def check_file_completeness(
    filepath: str,
    tree: ast.Module,
    workspace_files: set[str],
    project_modules: set[str],
) -> list[Issue]:
    """Check 4: FILE_COMPLETENESS — imported local modules exist."""
    issues: list[Issue] = []
    rel_name = os.path.basename(filepath)

    imported = _collect_imported_modules(tree)
    for top, _lineno, full_path in imported:
        if top in _STDLIB_MODULES:
            continue
        if top not in project_modules:
            continue
        # Verify the module file exists
        candidates = [
            f"{top}.py",
            os.path.join(top, "__init__.py"),
            os.path.join("src", top, "__init__.py"),
        ]
        # Also check sub-modules for dotted imports
        parts = full_path.split(".")
        if len(parts) > 1:
            candidates.append(os.path.join(*parts[:-1], f"{parts[-1]}.py"))
            candidates.append(os.path.join("src", *parts[:-1], f"{parts[-1]}.py"))
            candidates.append(os.path.join("src", *parts) + ".py")
            candidates.append(os.path.join("src", *parts, "__init__.py"))
        found = any(c in workspace_files for c in candidates)
        if not found:
            issues.append(
                (
                    "FILE_COMPLETE",
                    rel_name,
                    f"L{_lineno}: Import '{full_path}' — "
                    f"module file not found in workspace.",
                )
            )

    return issues


def check_entry_point(
    filepath: str,
    lines: list[str],
    is_main_file: bool,
) -> list[Issue]:
    """Check 5: ENTRY_POINT — main guard present."""
    issues: list[Issue] = []
    rel_name = os.path.basename(filepath)

    if not is_main_file:
        return issues

    has_main_guard = any("__name__" in line and "__main__" in line for line in lines)
    if not has_main_guard:
        issues.append(
            (
                "ENTRY_POINT",
                rel_name,
                "Main file missing `if __name__ == '__main__':` guard.",
            )
        )
    return issues


@traces(spec="029", story="Deterministic Review", reqs=["REQ-029-068"])
def check_dependency_declaration(
    filepath: str,
    tree: ast.Module,
    declared_deps: set[str],
    project_modules: set[str],
) -> list[Issue]:
    """Check 6: DEPENDENCY_DECLARATION — third-party imports declared."""
    issues: list[Issue] = []
    rel_name = os.path.basename(filepath)

    if not declared_deps:
        return issues  # No pyproject.toml, skip check

    imported = _collect_imported_modules(tree)
    for top, lineno, _ in imported:
        if top in _STDLIB_MODULES:
            continue
        if top in project_modules:
            continue
        normalized = top.lower().replace("-", "_")
        # Common aliases: PIL->pillow, cv2->opencv-python, etc.
        aliases = {
            "pil": "pillow",
            "cv2": "opencv_python",
            "sklearn": "scikit_learn",
            "yaml": "pyyaml",
            "attr": "attrs",
            "bs4": "beautifulsoup4",
        }
        check_name = aliases.get(normalized, normalized)
        if check_name not in declared_deps:
            issues.append(
                (
                    "DEPS",
                    rel_name,
                    f"L{lineno}: '{top}' imported but not in "
                    f"pyproject.toml dependencies.",
                )
            )
    return issues


def check_function_length(filepath: str, tree: ast.Module) -> list[Issue]:
    """Check 10: FUNCTION_LENGTH — max 40 lines per function."""
    issues: list[Issue] = []
    rel_name = os.path.basename(filepath)

    for node in ast.walk(tree):
        if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)):
            length = _function_line_count(node)
            if length > 40:
                issues.append(
                    (
                        "FUNC_LENGTH",
                        rel_name,
                        f"L{node.lineno}: `{node.name}()` is {length} lines (max 40).",
                    )
                )
    return issues


def check_function_args(filepath: str, tree: ast.Module) -> list[Issue]:
    """Check 11: FUNCTION_ARGS — max 5 parameters."""
    issues: list[Issue] = []
    rel_name = os.path.basename(filepath)

    for node in ast.walk(tree):
        if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)):
            args = node.args
            count = len(args.args) + len(args.kwonlyargs)
            # Subtract 'self'/'cls'
            if args.args and args.args[0].arg in ("self", "cls"):
                count -= 1
            if count > 5:
                issues.append(
                    (
                        "FUNC_ARGS",
                        rel_name,
                        f"L{node.lineno}: `{node.name}()` has {count} params (max 5).",
                    )
                )
    return issues


def check_no_placeholder(filepath: str, tree: ast.Module) -> list[Issue]:
    """Check 12: NO_PLACEHOLDER — no pass/TODO/NotImplementedError."""
    issues: list[Issue] = []
    rel_name = os.path.basename(filepath)

    for node in ast.walk(tree):
        if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)):
            for child in ast.walk(node):
                # Bare `pass` in a function with only pass + docstring
                if isinstance(child, ast.Pass):
                    # Allow pass in exception handlers and abstract
                    parent_body = node.body
                    non_doc = [
                        n
                        for n in parent_body
                        if not (
                            isinstance(n, ast.Expr)
                            and isinstance(n.value, ast.Constant)
                        )
                    ]
                    if len(non_doc) == 1 and isinstance(non_doc[0], ast.Pass):
                        issues.append(
                            (
                                "PLACEHOLDER",
                                rel_name,
                                f"L{node.lineno}: `{node.name}()` "
                                f"has empty body (only pass).",
                            )
                        )
                        break

                # raise NotImplementedError
                if (
                    isinstance(child, ast.Raise)
                    and child.exc
                    and isinstance(child.exc, ast.Call)
                    and isinstance(child.exc.func, ast.Name)
                    and child.exc.func.id == "NotImplementedError"
                ):
                    issues.append(
                        (
                            "PLACEHOLDER",
                            rel_name,
                            f"L{child.lineno}: NotImplementedError in `{node.name}()`.",
                        )
                    )

    return issues


def check_error_handling(filepath: str, tree: ast.Module) -> list[Issue]:
    """Check 13: ERROR_HANDLING — no bare except."""
    issues: list[Issue] = []
    rel_name = os.path.basename(filepath)

    for node in ast.walk(tree):
        if isinstance(node, ast.ExceptHandler):
            if node.type is None:
                issues.append(
                    (
                        "ERROR_HANDLING",
                        rel_name,
                        f"L{node.lineno}: Bare `except:` — "
                        f"use specific exception types.",
                    )
                )

    return issues


def _missing_param_hints(
    node: ast.FunctionDef | ast.AsyncFunctionDef,
    rel_name: str,
    start: int,
) -> list[Issue]:
    """Report params (from index ``start``) lacking a type annotation."""
    issues: list[Issue] = []
    for arg in node.args.args[start:]:
        if arg.annotation is None:
            issues.append(
                (
                    "TYPE_HINTS",
                    rel_name,
                    f"L{node.lineno}: `{node.name}()` "
                    f"param '{arg.arg}' has no type annotation.",
                )
            )
    return issues


def _check_function_type_hints(
    node: ast.FunctionDef | ast.AsyncFunctionDef,
    rel_name: str,
) -> list[Issue]:
    """Check one function's return and parameter annotations."""
    # __init__: check param annotations only (no return hint required).
    if node.name == "__init__":
        return _missing_param_hints(node, rel_name, 1)  # skip self
    if not _is_public(node.name):
        return []

    issues: list[Issue] = []
    if node.returns is None:
        issues.append(
            (
                "TYPE_HINTS",
                rel_name,
                f"L{node.lineno}: `{node.name}()` " f"missing return type annotation.",
            )
        )

    params = node.args.args
    start = 1 if params and params[0].arg in ("self", "cls") else 0
    issues.extend(_missing_param_hints(node, rel_name, start))
    return issues


def check_type_hints(filepath: str, tree: ast.Module) -> list[Issue]:
    """Check 17: TYPE_HINTS — public functions have annotations."""
    rel_name = os.path.basename(filepath)
    issues: list[Issue] = []
    for node in ast.walk(tree):
        if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)):
            issues.extend(_check_function_type_hints(node, rel_name))
    return issues


def check_mutable_defaults(filepath: str, tree: ast.Module) -> list[Issue]:
    """Check 18: DEFAULT_VALUES — no mutable defaults."""
    issues: list[Issue] = []
    rel_name = os.path.basename(filepath)

    mutable_types = (ast.List, ast.Dict, ast.Set)

    for node in ast.walk(tree):
        if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)):
            for default in node.args.defaults + node.args.kw_defaults:
                if default is not None and isinstance(default, mutable_types):
                    issues.append(
                        (
                            "MUTABLE_DEFAULT",
                            rel_name,
                            f"L{node.lineno}: `{node.name}()` has "
                            f"mutable default argument. Use None instead.",
                        )
                    )

    return issues


@traces(spec="031", story="Deterministic Review", reqs=["REQ-031-026"])
def check_docstrings(filepath: str, tree: ast.Module) -> list[Issue]:
    """Check 19: DOCSTRINGS — public classes and functions."""
    issues: list[Issue] = []
    rel_name = os.path.basename(filepath)

    for node in ast.walk(tree):
        if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)):
            if not _is_public(node.name):
                continue
            if not ast.get_docstring(node):
                issues.append(
                    (
                        "DOCSTRING",
                        rel_name,
                        f"L{node.lineno}: `{node.name}()` missing docstring.",
                    )
                )

        elif isinstance(node, ast.ClassDef):
            if not _is_public(node.name):
                continue
            if not ast.get_docstring(node):
                issues.append(
                    (
                        "DOCSTRING",
                        rel_name,
                        f"L{node.lineno}: `{node.name}` class missing docstring.",
                    )
                )

    return issues


def check_line_length(
    filepath: str, lines: list[str], max_len: int = 88
) -> list[Issue]:
    """Check 21: LINE_LENGTH — max characters per line."""
    issues: list[Issue] = []
    rel_name = os.path.basename(filepath)
    violations = 0

    for i, line in enumerate(lines, 1):
        stripped = line.rstrip("\n\r")
        if len(stripped) > max_len:
            # Allow long URLs and comments with URLs
            if "http://" in stripped or "https://" in stripped:
                continue
            violations += 1
            if violations <= 3:  # Cap reported instances
                issues.append(
                    (
                        "LINE_LENGTH",
                        rel_name,
                        f"L{i}: {len(stripped)} chars (max {max_len}).",
                    )
                )

    if violations > 3:
        issues.append(
            (
                "LINE_LENGTH",
                rel_name,
                f"...and {violations - 3} more lines over {max_len} chars.",
            )
        )
    return issues


def check_security(
    filepath: str,
    tree: ast.Module,
    lines: list[str],
) -> list[Issue]:
    """Check 30: SECURITY — eval/exec, hardcoded secrets."""
    issues: list[Issue] = []
    rel_name = os.path.basename(filepath)

    # AST: detect eval/exec calls
    for node in ast.walk(tree):
        if isinstance(node, ast.Call) and isinstance(node.func, ast.Name):
            if node.func.id in ("eval", "exec"):
                issues.append(
                    (
                        "SECURITY",
                        rel_name,
                        f"L{node.lineno}: `{node.func.id}()` call — "
                        f"potential code injection risk.",
                    )
                )

    # Regex: hardcoded secrets patterns
    secret_patterns = [
        (
            r"(?:password|passwd|secret|api_key|apikey|token|auth)"
            r'\s*=\s*["\'][^"\']{8,}["\']',
            "Possible hardcoded secret",
        ),
        (
            r"(?:sk-|pk_live_|sk_live_|ghp_|gho_|AKIA)[A-Za-z0-9]{10,}",
            "Possible API key/token",
        ),
    ]
    for i, line in enumerate(lines, 1):
        for pattern, desc in secret_patterns:
            if re.search(pattern, line, re.IGNORECASE):
                # Skip if it's a comment or os.getenv
                stripped = line.strip()
                if stripped.startswith("#"):
                    continue
                if "os.getenv" in line or "os.environ" in line:
                    continue
                issues.append(
                    (
                        "SECURITY",
                        rel_name,
                        f"L{i}: {desc}.",
                    )
                )
                break

    return issues


def check_todo_comments(filepath: str, lines: list[str]) -> list[Issue]:
    """Check 12b: No TODO/FIXME/HACK comments left behind."""
    issues: list[Issue] = []
    rel_name = os.path.basename(filepath)

    for i, line in enumerate(lines, 1):
        stripped = line.strip()
        if stripped.startswith("#"):
            upper = stripped.upper()
            if any(tag in upper for tag in ("TODO", "FIXME", "HACK", "XXX")):
                issues.append(
                    (
                        "TODO",
                        rel_name,
                        f"L{i}: {stripped[:60]}",
                    )
                )
    return issues


_MOCK_PATTERNS = [
    (r"from\s+unittest\.mock\s+import", "unittest.mock import"),
    (r"import\s+unittest\.mock", "unittest.mock import"),
    (r"from\s+unittest\s+import\s+mock", "unittest import mock"),
    (r"Mock\s*\(", "Mock() instantiation"),
    (r"MagicMock\s*\(", "MagicMock() instantiation"),
    (r"AsyncMock\s*\(", "AsyncMock() instantiation"),
    (r"@\s*(mock\.)?patch\s*\(", "@patch decorator"),
    (r"mocker\.(patch|spy|stub|mock)\s*\(", "pytest-mock usage"),
    (r"create_autospec\s*\(", "create_autospec()"),
    (r"patch\.object\s*\(", "patch.object()"),
    (r"patch\.dict\s*\(", "patch.dict()"),
    (r"patch\.multiple\s*\(", "patch.multiple()"),
    (
        r"class\s+(Mock|Fake|Stub|Dummy)\w*\s*[\(:]",
        "custom mock/stub/fake class",
    ),
    (r"monkeypatch\.\w+\s*\(", "monkeypatch usage"),
]


def check_no_except_pass(filepath: str, tree: ast.Module) -> list[Issue]:
    """G224: Check for bare except: and except+pass patterns.

    Args:
        filepath: Path to the Python file.
        tree: Parsed AST.

    Returns:
        List of error-swallowing violation issues.
    """
    issues: list[Issue] = []
    for node in ast.walk(tree):
        if not isinstance(node, ast.ExceptHandler):
            continue
        if node.type is None:
            issues.append(
                (
                    "EXCEPT_PASS",
                    filepath,
                    f"L{node.lineno}: bare `except:` — must specify exception type",
                )
            )
        if node.body and len(node.body) == 1 and isinstance(node.body[0], ast.Pass):
            issues.append(
                (
                    "EXCEPT_PASS",
                    filepath,
                    f"L{node.lineno}: `except: pass` swallows errors — "
                    "log the exception at minimum",
                )
            )
    return issues


def check_no_mocks(filepath: str, lines: list[str]) -> list[Issue]:
    """Check test files for mock/stub/fake usage.

    Args:
        filepath: Path to the test file.
        lines: File lines.

    Returns:
        List of mock violation issues.
    """
    issues: list[Issue] = []
    rel_name = os.path.basename(filepath)

    for i, line in enumerate(lines, 1):
        raw = line.strip()
        if not raw or raw.startswith("#"):
            continue
        for pattern, desc in _MOCK_PATTERNS:
            if re.search(pattern, raw):
                issues.append(
                    (
                        "MOCK",
                        rel_name,
                        f"L{i}: {desc} — `{raw[:60]}`",
                    )
                )
                break  # One violation per line
    return issues


@traces(spec="031", story="Deterministic Review", reqs=["REQ-031-050"])
def check_real_infrastructure(
    filepath: str,
    lines: list[str],
) -> list[Issue]:
    """G223: Verify test files use real infrastructure, not skips/mocks.

    Checks:
    - No pytest.mark.skipif that skips on infra unavailability
    - No MagicMock/patch (redundant with check_no_mocks but explicit)
    - Tests importing redis/neo4j must actually connect (have host/URI)
    - No 'localhost' when REDIS_HOST/KG_ROUTING_URI env vars exist

    Args:
        filepath: Path to the test file.
        lines: Lines of the file.

    Returns:
        List of issues found.
    """
    issues: list[Issue] = []
    basename = os.path.basename(filepath)

    # Only check test files
    if not basename.startswith("test_"):
        return issues

    content = "\n".join(lines)

    # Check for skip decorators that dodge infrastructure
    skip_patterns = [
        (
            r"pytest\.mark\.skipif.*(?:redis|neo4j|unavailable|REDIS|NEO4J)",
            "Test skips when infrastructure is unavailable — "
            "tests must use real infrastructure",
        ),
        (
            r"pytest\.mark\.skip\b(?!if).*(?:redis|neo4j|infra)",
            "Test unconditionally skips infrastructure tests",
        ),
    ]
    for pattern, desc in skip_patterns:
        for i, line in enumerate(lines, 1):
            if re.search(pattern, line, re.IGNORECASE):
                issues.append(("REAL_INFRA", filepath, f"L{i}: {desc}"))

    # Check that tests importing redis actually set REDIS_HOST
    imports_redis = "import redis" in content
    imports_neo4j = "import neo4j" in content or "from neo4j" in content
    has_redis_host = (
        "REDIS_HOST" in content
        or 'os.environ.get("REDIS_HOST"' in content
        or "tests.infrastructure" in content
    )
    # KG_ROUTING_URI/KG_URI are the canonical graph-endpoint keys since the
    # 2026-07-27 Neo4j decommission; NEO4J_URI is still RECOGNISED here (so
    # pre-cutover files under review still match) but is never ADVISED --
    # the remediation text below names only keys that still exist.
    has_graph_uri = (
        "KG_ROUTING_URI" in content
        or "KG_URI" in content
        or "NEO4J_URI" in content
        or "resolve_uri" in content
        or "tests.infrastructure" in content
    )

    if imports_redis and not has_redis_host:
        issues.append(
            (
                "REAL_INFRA",
                filepath,
                "Imports redis but no REDIS_HOST configured — "
                "must connect to real Redis (set REDIS_HOST in .env)",
            )
        )

    if imports_neo4j and not has_graph_uri:
        issues.append(
            (
                "REAL_INFRA",
                filepath,
                "Imports the neo4j Bolt driver but no graph endpoint is "
                "configured — must connect to the real graph (set "
                "KG_ROUTING_URI or KG_URI)",
            )
        )

    return issues


@traces(spec="031", story="Deterministic Review", reqs=["REQ-031-021"])
def check_lint(filepath: str) -> list[Issue]:
    """Run ruff or flake8 on a file for undefined names and unused imports.

    Args:
        filepath: Absolute path to the Python file.

    Returns:
        List of lint issues found.
    """
    issues: list[Issue] = []
    rel_name = os.path.basename(filepath)

    # G195: Skip F401 on __init__.py — re-exports are intentional
    is_init = rel_name == "__init__.py"
    ruff_select = "F821,F811" if is_init else "F821,F811,F401"
    flake8_select = "E999,F821,F811" if is_init else "E999,F821,F811,F401"

    # Try linters: system PATH first, then orchestrata venv fallback
    linter_cmds = [
        ("ruff check", ruff_select),
        ("flake8", flake8_select),
        ("/opt/orchestrata/.venv/bin/ruff check", ruff_select),
        ("/opt/orchestrata/.venv/bin/flake8", flake8_select),
    ]
    for linter_cmd, select in linter_cmds:
        cmd = linter_cmd.split() + [f"--select={select}", filepath]
        try:
            result = subprocess.run(
                cmd,
                capture_output=True,
                text=True,
                timeout=10,
            )
            if result.returncode != 0 and result.stdout.strip():
                for line in result.stdout.strip().splitlines()[:5]:
                    # Trim absolute path to just filename:line:col: msg
                    msg = line.strip()
                    if filepath in msg:
                        msg = msg.replace(filepath, rel_name)
                    issues.append(("LINT", rel_name, msg))
            return issues  # Linter found — stop trying others
        except FileNotFoundError, PermissionError:
            continue  # Linter not installed or not executable, try next
        except subprocess.TimeoutExpired:
            return issues

    return issues


# ── G230: Input Validation Enforcement ────────────────────────────


_VALIDATOR_PREFIXES = ("isinstance", "validate", "check", "assert", "verify")
_VALIDATION_ERROR_TYPES = {"TypeError", "ValueError", "ValidationError"}


def _is_validator_call(child: ast.AST) -> bool:
    """True if ``child`` is a call to a validator-named function."""
    if not isinstance(child, ast.Call):
        return False
    func = child.func
    name = ""
    if isinstance(func, ast.Name):
        name = func.id
    elif isinstance(func, ast.Attribute):
        name = func.attr
    return any(name.startswith(p) for p in _VALIDATOR_PREFIXES)


def _is_validation_raise(child: ast.AST) -> bool:
    """True if ``child`` raises a TypeError/ValueError-style exception."""
    if not (isinstance(child, ast.Raise) and child.exc):
        return False
    exc = child.exc
    exc_name = ""
    if isinstance(exc, ast.Call) and isinstance(exc.func, ast.Name):
        exc_name = exc.func.id
    elif isinstance(exc, ast.Name):
        exc_name = exc.id
    return exc_name in _VALIDATION_ERROR_TYPES


def _has_validation_pattern(node: ast.AST) -> bool:
    """True if a function body contains any recognized validation pattern.

    Patterns: a validator-named call, a raise of a validation error type,
    an assert statement, or an ``if ...: raise`` guard clause.
    """
    for child in ast.walk(node):
        if _is_validator_call(child):
            return True
        if _is_validation_raise(child):
            return True
        if isinstance(child, ast.Assert):
            return True
        if isinstance(child, ast.If) and any(
            isinstance(s, ast.Raise) for s in child.body
        ):
            return True
    return False


def _validatable_params(
    node: ast.FunctionDef | ast.AsyncFunctionDef,
) -> list[ast.arg]:
    """Return the function's parameters excluding a leading self/cls."""
    params = node.args.args
    start = 1 if params and params[0].arg in ("self", "cls") else 0
    return params[start:]


def check_parameter_validation(
    filepath: str,
    tree: ast.Module,
) -> list[Issue]:
    """G230: Check that public methods validate their parameters.

    Every public method that accepts parameters (beyond self/cls)
    must contain at least one validation pattern:
    - isinstance() check
    - if param is None
    - if not param
    - raise TypeError/ValueError
    - assert statement
    - validator function call

    Args:
        filepath: Absolute path to the file.
        tree: Parsed AST.

    Returns:
        List of validation issues.
    """
    issues: list[Issue] = []
    rel_name = os.path.basename(filepath)

    # Skip test files and __init__.py
    if rel_name.startswith("test_") or rel_name == "conftest.py":
        return issues
    if rel_name == "__init__.py":
        return issues

    for node in ast.walk(tree):
        if not isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)):
            continue
        if node.name.startswith("_"):
            continue

        real_params = _validatable_params(node)
        if not real_params:
            continue

        if not _has_validation_pattern(node):
            issues.append(
                (
                    "INPUT_VALIDATION",
                    rel_name,
                    f"L{node.lineno}: `{node.name}("
                    f"{', '.join(p.arg for p in real_params)})` "
                    "— no input validation. Public methods must "
                    "validate parameters (isinstance, None check, "
                    "raise TypeError/ValueError).",
                )
            )

    return issues


# ── G226: Traceability Enforcement ───────────────────────────────


# Names exempt from traceability (they ARE the traceability infrastructure).
_TRACEABILITY_EXEMPT = {"traces", "main"}


def _has_traces_decorator(node: ast.AST) -> bool:
    """Check if a node has a @traces (or @x.traces) decorator."""
    for dec in getattr(node, "decorator_list", []):
        if isinstance(dec, ast.Call):
            func = dec.func
            if isinstance(func, ast.Name) and func.id == "traces":
                return True
            if isinstance(func, ast.Attribute) and func.attr == "traces":
                return True
        elif isinstance(dec, ast.Name) and dec.id == "traces":
            return True
    return False


def _needs_traces(
    node: ast.ClassDef | ast.FunctionDef | ast.AsyncFunctionDef,
) -> bool:
    """True if a public, non-exempt node is missing its @traces decorator."""
    return (
        not node.name.startswith("_")
        and node.name not in _TRACEABILITY_EXEMPT
        and not _has_traces_decorator(node)
    )


@traces(spec="031", story="Deterministic Review", reqs=["REQ-031-026"])
def check_traceability(
    filepath: str,
    tree: ast.Module,
) -> list[Issue]:
    """G226: Verify public classes and functions have @traces decorator.

    Every public class and public function (not starting with _)
    must have a @traces(spec=, story=, reqs=[]) decorator for
    requirement traceability.

    Args:
        filepath: Absolute path to the file.
        tree: Parsed AST.

    Returns:
        List of traceability issues.
    """
    issues: list[Issue] = []
    rel_name = os.path.basename(filepath)

    # Skip test files — tests don't need @traces
    if rel_name.startswith("test_") or rel_name == "conftest.py":
        return issues

    # Skip __init__.py — re-exports, no logic
    if rel_name == "__init__.py":
        return issues

    for node in ast.iter_child_nodes(tree):
        if isinstance(node, ast.ClassDef) and _needs_traces(node):
            issues.append(
                (
                    "TRACEABILITY",
                    rel_name,
                    f"L{node.lineno}: class `{node.name}` "
                    "missing @traces(spec=, story=, reqs=[]) decorator.",
                )
            )
        elif isinstance(
            node, (ast.FunctionDef, ast.AsyncFunctionDef)
        ) and _needs_traces(node):
            issues.append(
                (
                    "TRACEABILITY",
                    rel_name,
                    f"L{node.lineno}: function `{node.name}()` "
                    "missing @traces(spec=, story=, reqs=[]) decorator.",
                )
            )

    return issues


# ── G226: File Path Validation ───────────────────────────────────


def check_file_paths(
    workspace: str,
    files: list[str],
) -> list[Issue]:
    """G226: Validate file paths for structure and naming.

    Checks:
    - Python file names are valid identifiers (no spaces, hyphens).
    - Package dirs contain __init__.py.
    - No stray .py files outside src/ or tests/.
    - No path traversal (.. segments).

    Args:
        workspace: Project workspace root.
        files: Relative file paths to check.

    Returns:
        List of issues found.
    """
    issues: list[Issue] = []

    for rel_path in files:
        if not rel_path.endswith(".py"):
            continue

        # Path traversal check
        if ".." in rel_path.split(os.sep):
            issues.append(
                (
                    "FILE_PATH",
                    rel_path,
                    "Path contains '..' traversal — not allowed.",
                )
            )
            continue

        # Valid Python filename check
        basename = os.path.basename(rel_path)
        stem = basename.removesuffix(".py")
        if stem != "__init__" and not stem.isidentifier():
            issues.append(
                (
                    "FILE_PATH",
                    rel_path,
                    f"'{basename}' is not a valid Python module name. "
                    "Use snake_case with no hyphens or spaces.",
                )
            )

        # Package __init__.py check — every directory containing
        # .py files under src/ must have __init__.py
        dirpath = os.path.dirname(rel_path)
        if dirpath and dirpath.startswith("src"):
            parts = dirpath.split(os.sep)
            for i in range(1, len(parts) + 1):
                pkg_dir = os.sep.join(parts[:i])
                init_path = os.path.join(workspace, pkg_dir, "__init__.py")
                if not os.path.exists(init_path):
                    issues.append(
                        (
                            "FILE_PATH",
                            os.path.join(pkg_dir, "__init__.py"),
                            f"Missing __init__.py — package '{pkg_dir}' "
                            "is not importable.",
                        )
                    )

    return issues


# ── Orchestrator ─────────────────────────────────────────────────


# app.py excluded — Flask factory pattern uses cli.py/__main__.py as entry.
_MAIN_FILES = {"cli.py", "__main__.py", "main.py"}


def _is_test_path(rel_path: str) -> bool:
    """True if a relative path names a test file."""
    return os.path.basename(rel_path).startswith("test_") or rel_path.startswith(
        "tests/"
    )


def _has_module_pseudo_segment(rel_path: str) -> bool:
    """True if any path segment is a scaffold ``Module:`` pseudo-segment."""
    return any(part.startswith(_SKIP_DIR_PREFIXES) for part in rel_path.split(os.sep))


def _discover_test_files(workspace: str, known_tests: list[str]) -> list[str]:
    """Walk the workspace for ``test_*.py`` files not already listed.

    Args:
        workspace: Path to project workspace root.
        known_tests: Test paths already collected from the files list.

    Returns:
        ``known_tests`` extended in place with any newly found test files.
    """
    skip_dirs = {".venv", "__pycache__", ".git", "site-packages", "node_modules"}
    for root, dirs, fnames in os.walk(workspace):
        dirs[:] = [
            d
            for d in dirs
            if d not in skip_dirs and not d.startswith(_SKIP_DIR_PREFIXES)
        ]
        for fname in fnames:
            if fname.startswith("test_") and fname.endswith(".py"):
                rel = os.path.relpath(os.path.join(root, fname), workspace)
                if rel not in known_tests:
                    known_tests.append(rel)
    return known_tests


def _common_file_checks(
    filepath: str,
    tree: ast.Module,
    lines: list[str],
    project_modules: set[str],
) -> list[Issue]:
    """Run the checks that apply to every file (source and test)."""
    issues: list[Issue] = []
    issues.extend(check_imports(filepath, tree, project_modules))
    issues.extend(check_function_length(filepath, tree))
    issues.extend(check_function_args(filepath, tree))
    issues.extend(check_error_handling(filepath, tree))
    issues.extend(check_mutable_defaults(filepath, tree))
    # G128-2: Skip LINE_LENGTH — black auto-format runs before review
    # and fixes everything it can. Remaining long lines are unfixable
    # (long strings, URLs, identifiers) and waste review cycles.
    issues.extend(check_no_mocks(filepath, lines))
    issues.extend(check_no_except_pass(filepath, tree))
    issues.extend(check_real_infrastructure(filepath, lines))
    issues.extend(check_security(filepath, tree, lines))
    issues.extend(check_lint(filepath))
    # G226: Traceability + G230: Input validation enforcement
    issues.extend(check_traceability(filepath, tree))
    issues.extend(check_parameter_validation(filepath, tree))
    return issues


def _source_only_checks(
    filepath: str,
    tree: ast.Module,
    lines: list[str],
    workspace_files: set[str],
    project_modules: set[str],
    declared_deps: set[str],
) -> list[Issue]:
    """Run the checks that apply to source files only (not tests)."""
    is_main = os.path.basename(filepath) in _MAIN_FILES
    issues: list[Issue] = []
    issues.extend(
        check_file_completeness(filepath, tree, workspace_files, project_modules)
    )
    issues.extend(check_entry_point(filepath, lines, is_main))
    issues.extend(
        check_dependency_declaration(filepath, tree, declared_deps, project_modules)
    )
    issues.extend(check_no_placeholder(filepath, tree))
    issues.extend(check_type_hints(filepath, tree))
    issues.extend(check_docstrings(filepath, tree))
    issues.extend(check_todo_comments(filepath, lines))
    return issues


@traces(
    spec="029",
    story="Deterministic Review",
    reqs=["REQ-029-067", "REQ-029-068"],
)
def run_deterministic_review(
    workspace: str,
    files: list[str],
) -> list[Issue]:
    """Run all deterministic checks on source files.

    Args:
        workspace: Path to project workspace root.
        files: List of relative file paths to review.

    Returns:
        List of (check_id, filepath, description) tuples.
    """
    workspace_files = _collect_workspace_files(workspace)
    project_modules = _collect_project_modules(workspace)
    declared_deps = _get_pyproject_deps(workspace)

    # G226: File path validation — runs before any code checks
    all_issues: list[Issue] = list(check_file_paths(workspace, files))

    # G-193-01: drop pseudo-files with a "Module:" segment, then split.
    real_files = [f for f in files if not _has_module_pseudo_segment(f)]
    py_source = [f for f in real_files if f.endswith(".py") and not _is_test_path(f)]
    py_tests = [f for f in real_files if f.endswith(".py") and _is_test_path(f)]
    _discover_test_files(workspace, py_tests)

    for rel_path in py_source + py_tests:
        filepath = os.path.join(workspace, rel_path)
        if not os.path.exists(filepath):
            continue

        tree = _parse_file(filepath)
        if tree is None:
            all_issues.append(
                ("SYNTAX", rel_path, "File has syntax errors — cannot parse.")
            )
            continue
        lines = _read_lines(filepath)

        all_issues.extend(_common_file_checks(filepath, tree, lines, project_modules))
        if not _is_test_path(rel_path):
            all_issues.extend(
                _source_only_checks(
                    filepath,
                    tree,
                    lines,
                    workspace_files,
                    project_modules,
                    declared_deps,
                )
            )

    return all_issues


# ── G227: Gated Review Stages ────────────────────────────────────
# Three strict gates. Each must pass before the next runs.
# Gate 1: Syntax — can the files even parse?
# Gate 2: Lint & Style — are they clean?
# Gate 3: Full checklist — architecture, traceability, quality


@traces(spec="031", story="Gated Review Stages", reqs=["REQ-031-017", "REQ-031-018"])
def review_gate_syntax(
    workspace: str,
    files: list[str],
) -> list[Issue]:
    """Gate 1: Syntax check. Can every file parse?

    If ANY file has a SyntaxError, stop here. Do not proceed
    to lint or checklist — fix syntax first.

    Args:
        workspace: Project workspace root.
        files: Relative file paths.

    Returns:
        List of SYNTAX issues. Empty means gate passed.
    """
    issues: list[Issue] = []

    # File path validation first
    issues.extend(check_file_paths(workspace, files))
    if issues:
        return issues

    py_files = [f for f in files if f.endswith(".py")]
    for rel_path in py_files:
        filepath = os.path.join(workspace, rel_path)
        if not os.path.exists(filepath):
            continue
        tree = _parse_file(filepath)
        if tree is None:
            issues.append(
                (
                    "SYNTAX",
                    rel_path,
                    "SyntaxError — file cannot be parsed as Python.",
                )
            )

    return issues


@traces(spec="031", story="Gated Review Stages", reqs=["REQ-031-021"])
def review_gate_lint(
    workspace: str,
    files: list[str],
) -> list[Issue]:
    """Gate 2: Lint and style. Are files clean?

    Only runs AFTER Gate 1 (syntax) passes. Checks:
    - Linter errors (flake8/ruff)
    - Import issues (absolute only, correct order)
    - Mutable defaults
    - No mocks, no except+pass
    - No bare except
    - Security issues

    Args:
        workspace: Project workspace root.
        files: Relative file paths.

    Returns:
        List of lint/style issues. Empty means gate passed.
    """
    issues: list[Issue] = []

    # Determine project modules for import validation
    project_modules = _collect_project_modules(workspace)

    py_files = [f for f in files if f.endswith(".py")]
    for rel_path in py_files:
        filepath = os.path.join(workspace, rel_path)
        if not os.path.exists(filepath):
            continue
        tree = _parse_file(filepath)
        if tree is None:
            continue  # Gate 1 would have caught this
        lines = _read_lines(filepath)

        issues.extend(check_lint(filepath))
        issues.extend(check_imports(filepath, tree, project_modules))
        issues.extend(check_mutable_defaults(filepath, tree))
        issues.extend(check_no_mocks(filepath, lines))
        issues.extend(check_no_except_pass(filepath, tree))
        issues.extend(check_security(filepath, tree, lines))
        issues.extend(check_real_infrastructure(filepath, lines))

    return issues


@traces(spec="031", story="Gated Review Stages", reqs=["REQ-031-025", "REQ-031-026"])
def review_gate_checklist(
    workspace: str,
    files: list[str],
) -> list[Issue]:
    """Gate 3: Full code review checklist.

    Only runs AFTER Gate 1 (syntax) and Gate 2 (lint) pass.
    Checks architecture, traceability, completeness, quality:
    - Function length (≤40 lines)
    - Function args (≤5 params)
    - Error handling
    - Traceability (@traces decorators)
    - File completeness
    - Entry points
    - Dependency declarations
    - No placeholders (NotImplementedError/pass/TODO)
    - Type hints
    - Docstrings

    Args:
        workspace: Project workspace root.
        files: Relative file paths.

    Returns:
        List of checklist issues. Empty means gate passed.
    """
    issues: list[Issue] = []

    workspace_files = _collect_workspace_files(workspace)
    project_modules = _collect_project_modules(workspace)
    declared_deps = _get_pyproject_deps(workspace)

    py_files = [f for f in files if f.endswith(".py")]
    for rel_path in py_files:
        filepath = os.path.join(workspace, rel_path)
        if not os.path.exists(filepath):
            continue
        tree = _parse_file(filepath)
        if tree is None:
            continue
        lines = _read_lines(filepath)

        # Quality checks — all files
        issues.extend(check_function_length(filepath, tree))
        issues.extend(check_function_args(filepath, tree))
        issues.extend(check_error_handling(filepath, tree))
        issues.extend(check_traceability(filepath, tree))
        issues.extend(check_parameter_validation(filepath, tree))

        # Source-only checks
        if not _is_test_path(rel_path):
            issues.extend(
                _source_only_checks(
                    filepath,
                    tree,
                    lines,
                    workspace_files,
                    project_modules,
                    declared_deps,
                )
            )

    return issues


# ── G226: Fix Instruction Generator ──────────────────────────────

# Every check type maps to a clear, actionable fix instruction
# that tells the LLM exactly what to do. No ambiguity.
_FIX_INSTRUCTIONS: dict[str, str] = {
    "SYNTAX": (
        "FIX: The file has a SyntaxError. Open the file, find the "
        "invalid Python, and rewrite it as valid Python code. "
        "Common causes: prose text instead of code, unclosed "
        "brackets, invalid indentation."
    ),
    "LINT": (
        "FIX: Run the linter message through the code. Fix the "
        "exact line and column referenced. Common fixes: remove "
        "unused imports, fix undefined names, add missing colons."
    ),
    "IMPORTS": (
        "FIX: Fix the import statement. Use absolute imports only "
        "(no relative imports). Ensure the module exists. Order: "
        "stdlib, third-party, local — separated by blank lines."
    ),
    "FUNC_LENGTH": (
        "FIX: Split the function into smaller helpers. Extract "
        "logical blocks into private functions with clear names. "
        "Each function must be ≤40 lines."
    ),
    "FUNC_ARGS": (
        "FIX: Reduce parameter count to ≤5. Group related params "
        "into a dataclass or TypedDict. Use **kwargs only if the "
        "function is a wrapper."
    ),
    "ERROR_HANDLING": (
        "FIX: Add try/except with specific exception types around "
        "the operation. Never use bare except:. Always log the "
        "exception: except SomeError as exc: logger.error(exc)"
    ),
    "MUTABLE_DEFAULT": (
        "FIX: Replace mutable default (list, dict, set) with None "
        "and initialize inside the function body: "
        "if param is None: param = []"
    ),
    "NO_MOCKS": (
        "FIX: Remove all unittest.mock usage. Test real behavior "
        "with real infrastructure. No MagicMock, patch, or AsyncMock."
    ),
    "EXCEPT_PASS": (
        "FIX: Replace 'except: pass' with specific exception type "
        "and logging: except SpecificError as exc: "
        "logger.debug('%s', exc)"
    ),
    "REAL_INFRA": (
        "FIX: Remove pytest.skip/skipif. Tests must use real "
        "infrastructure (Redis, the graph). Connect to actual services."
    ),
    "SECURITY": (
        "FIX: Remove the security vulnerability. No eval/exec with "
        "user input, no hardcoded secrets, no SQL injection via "
        "string concatenation. Use parameterized queries."
    ),
    "FILE_COMPLETE": (
        "FIX: The file is missing required content. Check that all "
        "classes and functions referenced by other files are defined."
    ),
    "ENTRY_POINT": (
        "FIX: Add 'if __name__ == \"__main__\":' guard to the main entry point file."
    ),
    "DEPENDENCY": (
        "FIX: Add the missing dependency to pyproject.toml under "
        "[project.dependencies]. Use the correct package name."
    ),
    "PLACEHOLDER": (
        "FIX: Replace NotImplementedError/pass/TODO with real "
        "implementation code. Every function must have a working body."
    ),
    "TYPE_HINTS": (
        "FIX: Add type hints to ALL public function parameters and "
        "return types. Python 3.14 syntax — use modern generics:\n"
        "  def func(name: str, items: list[int] = None) -> dict[str, Any]:\n"
        "  Use list[], dict[], set[], tuple[] (NOT List, Dict, Set, Tuple)\n"
        "  Use X | None (NOT Optional[X])\n"
        "  Use X | Y (NOT Union[X, Y])\n"
        "  __init__ needs param hints but no -> return hint."
    ),
    "DOCSTRING": (
        "FIX: Add a Google-style docstring. Format:\n"
        '    """One-line summary.\n\n'
        "    Args:\n        param: Description.\n\n"
        "    Returns:\n        Description.\n"
        '    """'
    ),
    "TODO": (
        "FIX: Remove the TODO/FIXME/HACK comment and implement "
        "the functionality now. No deferred work."
    ),
    "TRACEABILITY": (
        "FIX: Add @traces decorator to the class/function. Format:\n"
        "    @traces(spec='001', story='US-001', reqs=['REQ-001'])\n"
        "If orchestrata.core.traceability is not available, define:\n"
        "    def traces(**kw): return lambda f: f"
    ),
    "FILE_PATH": (
        "FIX: Rename the file to use a valid Python module name "
        "(snake_case, no hyphens, no spaces). Ensure all package "
        "directories have __init__.py."
    ),
    "INPUT_VALIDATION": (
        "FIX: Add input validation at the start of the method.\n"
        "  For each parameter, add one or more of:\n"
        "  - Type check: if not isinstance(param, expected_type): "
        "raise TypeError(...)\n"
        "  - None check: if param is None: raise ValueError(...)\n"
        "  - Range check: if param < 0: raise ValueError(...)\n"
        "  - Call a validator: validate_param(param)\n"
        "  Every public method must validate every parameter."
    ),
    "IMPORT_ERROR": (
        "FIX: The import fails at runtime. Check that the module "
        "exists, is installed, and the import path is correct."
    ),
}


@traces(spec="031", story="Gated Review Stages", reqs=["REQ-031-028"])
def get_fix_instructions(issues: list[Issue]) -> str:
    """Generate clear fix instructions for each issue.

    Args:
        issues: List of (check_id, filepath, description) tuples.

    Returns:
        Formatted fix instructions string for the LLM.
    """
    if not issues:
        return ""

    lines = ["FIX INSTRUCTIONS (mandatory — resolve ALL before handoff):\n"]
    seen_checks: set[str] = set()

    for check_id, filepath, desc in issues:
        instruction = _FIX_INSTRUCTIONS.get(
            check_id,
            f"FIX: Resolve the {check_id} issue in {filepath}.",
        )
        lines.append(f"  [{check_id}] {filepath}: {desc}")
        if check_id not in seen_checks:
            lines.append(f"    → {instruction}")
            seen_checks.add(check_id)
        lines.append("")

    lines.append(
        "CRITICAL: You MUST fix ALL issues above. Do NOT hand off "
        "or escalate until every single issue is resolved. "
        "Partial fixes are rejected."
    )
    return "\n".join(lines)


def format_review_report(issues: list[Issue]) -> str:
    """Format issues into a human-readable review report.

    Args:
        issues: List of (check_id, filepath, description) tuples.

    Returns:
        Formatted review string.
    """
    if not issues:
        return "DETERMINISTIC_REVIEW: PASS (0 issues)"

    # Group by check_id
    by_check: dict[str, list[tuple[str, str]]] = {}
    for check_id, filepath, desc in issues:
        by_check.setdefault(check_id, []).append((filepath, desc))

    lines = [
        f"DETERMINISTIC_REVIEW: FAIL ({len(issues)} issues)",
        "",
    ]
    for check_id, file_issues in sorted(by_check.items()):
        lines.append(f"[{check_id}] ({len(file_issues)} issues)")
        for filepath, desc in file_issues[:5]:
            lines.append(f"  - {desc}")
        if len(file_issues) > 5:
            lines.append(f"  ...and {len(file_issues) - 5} more")
        lines.append("")

    return "\n".join(lines)